diff mbox series

[V4,XDMA,2/2] dmaengine: xilinx: xdma: Add user logic interrupt support

Message ID 1663871905-60498-3-git-send-email-lizhi.hou@amd.com (mailing list archive)
State Superseded
Headers show
Series xilinx XDMA driver | expand

Commit Message

Lizhi Hou Sept. 22, 2022, 6:38 p.m. UTC
The Xilinx DMA/Bridge Subsystem for PCIe (XDMA) provides up to 16 user
interrupt wires to user logic that generate interrupts to the host.
This patch adds APIs to register/unregister interrupt handler for a given
interrupt wire index.

Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
Signed-off-by: Sonal Santan <sonal.santan@amd.com>
Signed-off-by: Max Zhen <max.zhen@amd.com>
Signed-off-by: Brian Xu <brian.xu@amd.com>
---
 MAINTAINERS                  |  1 +
 drivers/dma/xilinx/xdma.c    | 80 ++++++++++++++++++++++++++++++++++++
 include/linux/dma/amd_xdma.h | 17 ++++++++
 3 files changed, 98 insertions(+)
 create mode 100644 include/linux/dma/amd_xdma.h

Comments

Martin Tůma Sept. 27, 2022, 3:31 p.m. UTC | #1
On 22. 09. 22 20:38, Lizhi Hou wrote:
> The Xilinx DMA/Bridge Subsystem for PCIe (XDMA) provides up to 16 user
> interrupt wires to user logic that generate interrupts to the host.
> This patch adds APIs to register/unregister interrupt handler for a given
> interrupt wire index.
> 
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
> Signed-off-by: Sonal Santan <sonal.santan@amd.com>
> Signed-off-by: Max Zhen <max.zhen@amd.com>
> Signed-off-by: Brian Xu <brian.xu@amd.com>
> ---
>   MAINTAINERS                  |  1 +
>   drivers/dma/xilinx/xdma.c    | 80 ++++++++++++++++++++++++++++++++++++
>   include/linux/dma/amd_xdma.h | 17 ++++++++
>   3 files changed, 98 insertions(+)
>   create mode 100644 include/linux/dma/amd_xdma.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c1be0b2e378a..019d84b2b086 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21691,6 +21691,7 @@ L:	dmaengine@vger.kernel.org
>   S:	Supported
>   F:	drivers/dma/xilinx/xdma-regs.h
>   F:	drivers/dma/xilinx/xdma.c
> +F:	include/linux/dma/amd_xdma.h
>   F:	include/linux/platform_data/amd_xdma.h
>   
>   XILINX ZYNQMP DPDMA DRIVER
> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
> index 58a57e03bef5..884942cd2a37 100644
> --- a/drivers/dma/xilinx/xdma.c
> +++ b/drivers/dma/xilinx/xdma.c
> @@ -25,6 +25,7 @@
>   #include <linux/dmapool.h>
>   #include <linux/regmap.h>
>   #include <linux/dmaengine.h>
> +#include <linux/dma/amd_xdma.h>
>   #include <linux/platform_device.h>
>   #include <linux/platform_data/amd_xdma.h>
>   #include <linux/dma-mapping.h>
> @@ -736,6 +737,7 @@ static int xdma_set_vector_reg(struct xdma_device *xdev, u32 vec_tbl_start,
>   static int xdma_irq_init(struct xdma_device *xdev)
>   {
>   	u32 irq = xdev->irq_start;
> +	u32 user_irq_start;
>   	int i, j, ret;
>   
>   	/* return failure if there are not enough IRQs */
> @@ -786,6 +788,18 @@ static int xdma_irq_init(struct xdma_device *xdev)
>   		goto failed;
>   	}
>   
> +	/* config user IRQ registers if needed */
> +	user_irq_start = xdev->h2c_chan_num + xdev->c2h_chan_num;
> +	if (xdev->irq_num > user_irq_start) {
> +		ret = xdma_set_vector_reg(xdev, XDMA_IRQ_USER_VEC_NUM,
> +					  user_irq_start,
> +					  xdev->irq_num - user_irq_start);
> +		if (ret) {
> +			xdma_err(xdev, "failed to set user vectors: %d", ret);
> +			goto failed;
> +		}
> +	}
> +
>   	/* enable interrupt */
>   	ret = xdma_enable_intr(xdev);
>   	if (ret) {
> @@ -816,6 +830,72 @@ static bool xdma_filter_fn(struct dma_chan *chan, void *param)
>   	return true;
>   }
>   
> +/**
> + * xdma_free_user_irq - Free user interrupt
> + * @pdev: Pointer to the platform_device structure
> + * @user_irq_index: User IRQ index
> + * @arg: User interrupt cookie
> + */
> +void xdma_free_user_irq(struct platform_device *pdev, u32 user_irq_index,
> +			void *arg)
> +{
> +	struct xdma_device *xdev = platform_get_drvdata(pdev);
> +	u32 user_irq;
> +
> +	user_irq = xdev->h2c_chan_num + xdev->c2h_chan_num + user_irq_index;
> +	if (user_irq >= xdev->irq_num) {
> +		xdma_err(xdev, "invalid user irq index");
> +		return;
> +	}
> +	user_irq += xdev->irq_start;
> +
> +	xdma_write_reg(xdev, XDMA_IRQ_BASE, XDMA_IRQ_USER_INT_EN_W1C,
> +		       (1 << user_irq_index));
> +
> +	free_irq(user_irq, arg);
> +}
> +EXPORT_SYMBOL(xdma_free_user_irq);
> +
> +/**
> + * xdma_request_user_irq - Register user interrupt
> + * @pdev: Pointer to the platform_device structure
> + * @user_irq_index: User IRQ index
> + * @flags: Handling flags
> + * @handler: User interrupt handler
> + * @arg: User interrupt cookie
> + */
> +int xdma_request_user_irq(struct platform_device *pdev, u32 user_irq_index,
> +			  irq_handler_t handler, void *arg, ulong flags)
> +{
> +	struct xdma_device *xdev = platform_get_drvdata(pdev);
> +	u32 user_irq;
> +	int ret;
> +
> +	user_irq = xdev->h2c_chan_num + xdev->c2h_chan_num + user_irq_index;
> +	if (user_irq >= xdev->irq_num) {
> +		xdma_err(xdev, "invalid user irq index");
> +		return -EINVAL;
> +	}
> +	user_irq += xdev->irq_start;
> +
> +	ret = request_irq(user_irq, handler, flags, "xdma-user", arg);
> +	if (ret) {
> +		xdma_err(xdev, "request irq failed, %d", ret);
> +		return ret;
> +	}
> +
> +	ret = xdma_write_reg(xdev, XDMA_IRQ_BASE, XDMA_IRQ_USER_INT_EN_W1S,
> +			     (1 << user_irq_index));
> +	if (ret) {
> +		xdma_err(xdev, "set user irq mask failed, %d", ret);
> +		free_irq(user_irq, arg);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(xdma_request_user_irq);
> +
>   /**
>    * xdma_remove - Driver remove function
>    * @pdev: Pointer to the platform_device structure
> diff --git a/include/linux/dma/amd_xdma.h b/include/linux/dma/amd_xdma.h
> new file mode 100644
> index 000000000000..b70486121ca6
> --- /dev/null
> +++ b/include/linux/dma/amd_xdma.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2022, Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef _DMAENGINE_AMD_XDMA_H
> +#define _DMAENGINE_AMD_XDMA_H
> +
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +
> +int xdma_request_user_irq(struct platform_device *pdev, u32 user_irq_index,
> +			  irq_handler_t handler, void *arg, ulong flags);
> +void xdma_free_user_irq(struct platform_device *pdev, u32 user_irq_index,
> +			void *arg);
> +
> +#endif /* _DMAENGINE_AMD_XDMA_H */

Hi,
The user IRQ logic is unusable, if you have other IP cores with linux 
drivers (eg. spi or i2c) connected to that IRQs like we (and many other 
users as well) have on our PCIe card. You can't use the already existing
xiic or xilinx-spi drivers with your xdma driver!

You must split the "IRQ enable" and "IRQ request" operations for your 
driver to be usable with the other platform drivers in linux. Instead of
"xdma_request_user_irq" there must be something like 
"xdma_enable_user_irq" that only enables the IRQ in the IP block, but 
does not "eat" the IRQ so you can pass it to the initialization of
the platform driver of xiic or xilinx-spi.

M.
Lizhi Hou Sept. 27, 2022, 4:28 p.m. UTC | #2
On 9/27/22 08:31, Martin Tůma wrote:
> On 22. 09. 22 20:38, Lizhi Hou wrote:
>> The Xilinx DMA/Bridge Subsystem for PCIe (XDMA) provides up to 16 user
>> interrupt wires to user logic that generate interrupts to the host.
>> This patch adds APIs to register/unregister interrupt handler for a 
>> given
>> interrupt wire index.
>>
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>> Signed-off-by: Sonal Santan <sonal.santan@amd.com>
>> Signed-off-by: Max Zhen <max.zhen@amd.com>
>> Signed-off-by: Brian Xu <brian.xu@amd.com>
>> ---
>>   MAINTAINERS                  |  1 +
>>   drivers/dma/xilinx/xdma.c    | 80 ++++++++++++++++++++++++++++++++++++
>>   include/linux/dma/amd_xdma.h | 17 ++++++++
>>   3 files changed, 98 insertions(+)
>>   create mode 100644 include/linux/dma/amd_xdma.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c1be0b2e378a..019d84b2b086 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -21691,6 +21691,7 @@ L:    dmaengine@vger.kernel.org
>>   S:    Supported
>>   F:    drivers/dma/xilinx/xdma-regs.h
>>   F:    drivers/dma/xilinx/xdma.c
>> +F:    include/linux/dma/amd_xdma.h
>>   F:    include/linux/platform_data/amd_xdma.h
>>     XILINX ZYNQMP DPDMA DRIVER
>> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
>> index 58a57e03bef5..884942cd2a37 100644
>> --- a/drivers/dma/xilinx/xdma.c
>> +++ b/drivers/dma/xilinx/xdma.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/dmapool.h>
>>   #include <linux/regmap.h>
>>   #include <linux/dmaengine.h>
>> +#include <linux/dma/amd_xdma.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/platform_data/amd_xdma.h>
>>   #include <linux/dma-mapping.h>
>> @@ -736,6 +737,7 @@ static int xdma_set_vector_reg(struct xdma_device 
>> *xdev, u32 vec_tbl_start,
>>   static int xdma_irq_init(struct xdma_device *xdev)
>>   {
>>       u32 irq = xdev->irq_start;
>> +    u32 user_irq_start;
>>       int i, j, ret;
>>         /* return failure if there are not enough IRQs */
>> @@ -786,6 +788,18 @@ static int xdma_irq_init(struct xdma_device *xdev)
>>           goto failed;
>>       }
>>   +    /* config user IRQ registers if needed */
>> +    user_irq_start = xdev->h2c_chan_num + xdev->c2h_chan_num;
>> +    if (xdev->irq_num > user_irq_start) {
>> +        ret = xdma_set_vector_reg(xdev, XDMA_IRQ_USER_VEC_NUM,
>> +                      user_irq_start,
>> +                      xdev->irq_num - user_irq_start);
>> +        if (ret) {
>> +            xdma_err(xdev, "failed to set user vectors: %d", ret);
>> +            goto failed;
>> +        }
>> +    }
>> +
>>       /* enable interrupt */
>>       ret = xdma_enable_intr(xdev);
>>       if (ret) {
>> @@ -816,6 +830,72 @@ static bool xdma_filter_fn(struct dma_chan 
>> *chan, void *param)
>>       return true;
>>   }
>>   +/**
>> + * xdma_free_user_irq - Free user interrupt
>> + * @pdev: Pointer to the platform_device structure
>> + * @user_irq_index: User IRQ index
>> + * @arg: User interrupt cookie
>> + */
>> +void xdma_free_user_irq(struct platform_device *pdev, u32 
>> user_irq_index,
>> +            void *arg)
>> +{
>> +    struct xdma_device *xdev = platform_get_drvdata(pdev);
>> +    u32 user_irq;
>> +
>> +    user_irq = xdev->h2c_chan_num + xdev->c2h_chan_num + 
>> user_irq_index;
>> +    if (user_irq >= xdev->irq_num) {
>> +        xdma_err(xdev, "invalid user irq index");
>> +        return;
>> +    }
>> +    user_irq += xdev->irq_start;
>> +
>> +    xdma_write_reg(xdev, XDMA_IRQ_BASE, XDMA_IRQ_USER_INT_EN_W1C,
>> +               (1 << user_irq_index));
>> +
>> +    free_irq(user_irq, arg);
>> +}
>> +EXPORT_SYMBOL(xdma_free_user_irq);
>> +
>> +/**
>> + * xdma_request_user_irq - Register user interrupt
>> + * @pdev: Pointer to the platform_device structure
>> + * @user_irq_index: User IRQ index
>> + * @flags: Handling flags
>> + * @handler: User interrupt handler
>> + * @arg: User interrupt cookie
>> + */
>> +int xdma_request_user_irq(struct platform_device *pdev, u32 
>> user_irq_index,
>> +              irq_handler_t handler, void *arg, ulong flags)
>> +{
>> +    struct xdma_device *xdev = platform_get_drvdata(pdev);
>> +    u32 user_irq;
>> +    int ret;
>> +
>> +    user_irq = xdev->h2c_chan_num + xdev->c2h_chan_num + 
>> user_irq_index;
>> +    if (user_irq >= xdev->irq_num) {
>> +        xdma_err(xdev, "invalid user irq index");
>> +        return -EINVAL;
>> +    }
>> +    user_irq += xdev->irq_start;
>> +
>> +    ret = request_irq(user_irq, handler, flags, "xdma-user", arg);
>> +    if (ret) {
>> +        xdma_err(xdev, "request irq failed, %d", ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = xdma_write_reg(xdev, XDMA_IRQ_BASE, XDMA_IRQ_USER_INT_EN_W1S,
>> +                 (1 << user_irq_index));
>> +    if (ret) {
>> +        xdma_err(xdev, "set user irq mask failed, %d", ret);
>> +        free_irq(user_irq, arg);
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL(xdma_request_user_irq);
>> +
>>   /**
>>    * xdma_remove - Driver remove function
>>    * @pdev: Pointer to the platform_device structure
>> diff --git a/include/linux/dma/amd_xdma.h b/include/linux/dma/amd_xdma.h
>> new file mode 100644
>> index 000000000000..b70486121ca6
>> --- /dev/null
>> +++ b/include/linux/dma/amd_xdma.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2022, Advanced Micro Devices, Inc.
>> + */
>> +
>> +#ifndef _DMAENGINE_AMD_XDMA_H
>> +#define _DMAENGINE_AMD_XDMA_H
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +
>> +int xdma_request_user_irq(struct platform_device *pdev, u32 
>> user_irq_index,
>> +              irq_handler_t handler, void *arg, ulong flags);
>> +void xdma_free_user_irq(struct platform_device *pdev, u32 
>> user_irq_index,
>> +            void *arg);
>> +
>> +#endif /* _DMAENGINE_AMD_XDMA_H */
>
> Hi,
> The user IRQ logic is unusable, if you have other IP cores with linux 
> drivers (eg. spi or i2c) connected to that IRQs like we (and many 
> other users as well) have on our PCIe card. You can't use the already 
> existing
> xiic or xilinx-spi drivers with your xdma driver!
>
> You must split the "IRQ enable" and "IRQ request" operations for your 
> driver to be usable with the other platform drivers in linux. Instead of
> "xdma_request_user_irq" there must be something like 
> "xdma_enable_user_irq" that only enables the IRQ in the IP block, but 
> does not "eat" the IRQ so you can pass it to the initialization of
> the platform driver of xiic or xilinx-spi.
>
> M.

Okay, I got the point. How about changing request/remove APIs to 
enable/disable APIs as below

      xdma_enable_user_irq(struct platform_device *pdev, u32 
user_irq_index, u32 *irq)

             user_irq_index: user logic interrupt wire index. (XDMA 
driver determines how system IRQs are mapped to DMA channels and user 
logic wires)

             irq: IRQ number returned for registering interrupt handler 
(request_irq()) or passing to existing platform driver.

     xdma_disable_user_irq(struct platform_device *pdev, u32 user_irq_index)

Does this make sense to you?


Thanks,

Lizhi
Martin Tůma Sept. 27, 2022, 4:46 p.m. UTC | #3
On 27. 09. 22 18:28, Lizhi Hou wrote:

> Okay, I got the point. How about changing request/remove APIs to 
> enable/disable APIs as below
> 
>       xdma_enable_user_irq(struct platform_device *pdev, u32 
> user_irq_index, u32 *irq)
> 
>              user_irq_index: user logic interrupt wire index. (XDMA 
> driver determines how system IRQs are mapped to DMA channels and user 
> logic wires)
> 
>              irq: IRQ number returned for registering interrupt handler 
> (request_irq()) or passing to existing platform driver.
> 
>      xdma_disable_user_irq(struct platform_device *pdev, u32 
> user_irq_index)
> 
> Does this make sense to you?
> 

I think even the "irq" parameter in the enable function is surplus as 
the parent driver (the driver of the actual PCIe card) knows* what PCI 
irq he has to allocate without XDMA providing the number.

xdma_enable_user_irq(struct platform_device *pdev, u32 user_irq_index);
xdma_disable_user_irq(struct platform_device *pdev, u32 user_irq_index);

should be all that is needed.

M.

* something like:
pci_irq_vector((pdev), PCI_BAR_ID) + NUM_C2H_CHANNELS + NUM_H2C_CHANNELS
can be used from the PCIe driver
Lizhi Hou Sept. 27, 2022, 5:18 p.m. UTC | #4
On 9/27/22 09:46, Martin Tůma wrote:
> On 27. 09. 22 18:28, Lizhi Hou wrote:
>
>> Okay, I got the point. How about changing request/remove APIs to 
>> enable/disable APIs as below
>>
>>       xdma_enable_user_irq(struct platform_device *pdev, u32 
>> user_irq_index, u32 *irq)
>>
>>              user_irq_index: user logic interrupt wire index. (XDMA 
>> driver determines how system IRQs are mapped to DMA channels and user 
>> logic wires)
>>
>>              irq: IRQ number returned for registering interrupt 
>> handler (request_irq()) or passing to existing platform driver.
>>
>>      xdma_disable_user_irq(struct platform_device *pdev, u32 
>> user_irq_index)
>>
>> Does this make sense to you?
>>
>
> I think even the "irq" parameter in the enable function is surplus as 
> the parent driver (the driver of the actual PCIe card) knows* what PCI 
> irq he has to allocate without XDMA providing the number.
>
> xdma_enable_user_irq(struct platform_device *pdev, u32 user_irq_index);
> xdma_disable_user_irq(struct platform_device *pdev, u32 user_irq_index);
>
> should be all that is needed.
>
> M.
>
> * something like:
> pci_irq_vector((pdev), PCI_BAR_ID) + NUM_C2H_CHANNELS + NUM_H2C_CHANNELS
> can be used from the PCIe driver

How does parent driver know the first few vectors will be assigned to 
DMA channel?  Parent diver should not assume the first 
(NUM_C2H_CHANNELS+NUM_H2C_CHANNELS) are for DMA channel.

Parent driver passes the system IRQ range  to XDMA driver, and only XDMA 
driver knows what IRQs are used by DMA channel and what IRQs are mapped 
to user logic wires. I would keep the "u32 *irq" argument.


Thanks,

Lizhi
Martin Tůma Sept. 27, 2022, 5:54 p.m. UTC | #5
On 27. 09. 22 19:18, Lizhi Hou wrote:
> 
> On 9/27/22 09:46, Martin Tůma wrote:
>> On 27. 09. 22 18:28, Lizhi Hou wrote:
>>
>>> Okay, I got the point. How about changing request/remove APIs to 
>>> enable/disable APIs as below
>>>
>>>       xdma_enable_user_irq(struct platform_device *pdev, u32 
>>> user_irq_index, u32 *irq)
>>>
>>>              user_irq_index: user logic interrupt wire index. (XDMA 
>>> driver determines how system IRQs are mapped to DMA channels and user 
>>> logic wires)
>>>
>>>              irq: IRQ number returned for registering interrupt 
>>> handler (request_irq()) or passing to existing platform driver.
>>>
>>>      xdma_disable_user_irq(struct platform_device *pdev, u32 
>>> user_irq_index)
>>>
>>> Does this make sense to you?
>>>
>>
>> I think even the "irq" parameter in the enable function is surplus as 
>> the parent driver (the driver of the actual PCIe card) knows* what PCI 
>> irq he has to allocate without XDMA providing the number.
>>
>> xdma_enable_user_irq(struct platform_device *pdev, u32 user_irq_index);
>> xdma_disable_user_irq(struct platform_device *pdev, u32 user_irq_index);
>>
>> should be all that is needed.
>>
>> M.
>>
>> * something like:
>> pci_irq_vector((pdev), PCI_BAR_ID) + NUM_C2H_CHANNELS + NUM_H2C_CHANNELS
>> can be used from the PCIe driver
> 
> How does parent driver know the first few vectors will be assigned to 
> DMA channel?  Parent diver should not assume the first 
> (NUM_C2H_CHANNELS+NUM_H2C_CHANNELS) are for DMA channel.
> 
> Parent driver passes the system IRQ range  to XDMA driver, and only XDMA 
> driver knows what IRQs are used by DMA channel and what IRQs are mapped 
> to user logic wires. I would keep the "u32 *irq" argument.
> 

The parent driver knows how much DMA channels it wants/allocates. If it 
is possible to allocate different IRQs than the first NUM_C2H_CHANNELS + 
NUM_H2C_CHANNELS to the XDMA IP core, than that parameter may be needed, 
but I haven't seen such HW. Moreover, every parent driver author should 
IMHO know how the channel and user IRQs are mapped in their specific HW 
configuration so this info can be "hard-wired" in the parent driver, but 
I'm fine with it when the irq parameter is kept anyway. All I really 
need is that the enable/disable logic is split from the irq 
allocate/register logic so I can use the other platform drivers.

M.
Lizhi Hou Sept. 27, 2022, 6:14 p.m. UTC | #6
On 9/27/22 10:54, Martin Tůma wrote:
> On 27. 09. 22 19:18, Lizhi Hou wrote:
>>
>> On 9/27/22 09:46, Martin Tůma wrote:
>>> On 27. 09. 22 18:28, Lizhi Hou wrote:
>>>
>>>> Okay, I got the point. How about changing request/remove APIs to 
>>>> enable/disable APIs as below
>>>>
>>>>       xdma_enable_user_irq(struct platform_device *pdev, u32 
>>>> user_irq_index, u32 *irq)
>>>>
>>>>              user_irq_index: user logic interrupt wire index. (XDMA 
>>>> driver determines how system IRQs are mapped to DMA channels and 
>>>> user logic wires)
>>>>
>>>>              irq: IRQ number returned for registering interrupt 
>>>> handler (request_irq()) or passing to existing platform driver.
>>>>
>>>>      xdma_disable_user_irq(struct platform_device *pdev, u32 
>>>> user_irq_index)
>>>>
>>>> Does this make sense to you?
>>>>
>>>
>>> I think even the "irq" parameter in the enable function is surplus 
>>> as the parent driver (the driver of the actual PCIe card) knows* 
>>> what PCI irq he has to allocate without XDMA providing the number.
>>>
>>> xdma_enable_user_irq(struct platform_device *pdev, u32 user_irq_index);
>>> xdma_disable_user_irq(struct platform_device *pdev, u32 
>>> user_irq_index);
>>>
>>> should be all that is needed.
>>>
>>> M.
>>>
>>> * something like:
>>> pci_irq_vector((pdev), PCI_BAR_ID) + NUM_C2H_CHANNELS + 
>>> NUM_H2C_CHANNELS
>>> can be used from the PCIe driver
>>
>> How does parent driver know the first few vectors will be assigned to 
>> DMA channel?  Parent diver should not assume the first 
>> (NUM_C2H_CHANNELS+NUM_H2C_CHANNELS) are for DMA channel.
>>
>> Parent driver passes the system IRQ range  to XDMA driver, and only 
>> XDMA driver knows what IRQs are used by DMA channel and what IRQs are 
>> mapped to user logic wires. I would keep the "u32 *irq" argument.
>>
>
> The parent driver knows how much DMA channels it wants/allocates. If 
> it is possible to allocate different IRQs than the first 
> NUM_C2H_CHANNELS + NUM_H2C_CHANNELS to the XDMA IP core, than that 
> parameter may be needed, but I haven't seen such HW. Moreover, every 
> parent driver author should IMHO know how the channel and user IRQs 
> are mapped in their specific HW configuration so this info can be 
> "hard-wired" in the parent driver, but I'm fine with it when the irq 
> parameter is kept anyway. All I really need is that the enable/disable 
> logic is split from the irq allocate/register logic so I can use the 
> other platform drivers.
>
> M.

Thanks Mark, all your comments are very helpful. And glad to know you 
are fine with irq parameter. The IRQ binding is set by xdma driver ( 
xdma_set_vector_reg() ). That means xdma driver is able to map any 
system IRQ to dma channel or user logic. That is why I prefer to keep 
irq parameter. Just a FYI.

Lizhi
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index c1be0b2e378a..019d84b2b086 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21691,6 +21691,7 @@  L:	dmaengine@vger.kernel.org
 S:	Supported
 F:	drivers/dma/xilinx/xdma-regs.h
 F:	drivers/dma/xilinx/xdma.c
+F:	include/linux/dma/amd_xdma.h
 F:	include/linux/platform_data/amd_xdma.h
 
 XILINX ZYNQMP DPDMA DRIVER
diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c
index 58a57e03bef5..884942cd2a37 100644
--- a/drivers/dma/xilinx/xdma.c
+++ b/drivers/dma/xilinx/xdma.c
@@ -25,6 +25,7 @@ 
 #include <linux/dmapool.h>
 #include <linux/regmap.h>
 #include <linux/dmaengine.h>
+#include <linux/dma/amd_xdma.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/amd_xdma.h>
 #include <linux/dma-mapping.h>
@@ -736,6 +737,7 @@  static int xdma_set_vector_reg(struct xdma_device *xdev, u32 vec_tbl_start,
 static int xdma_irq_init(struct xdma_device *xdev)
 {
 	u32 irq = xdev->irq_start;
+	u32 user_irq_start;
 	int i, j, ret;
 
 	/* return failure if there are not enough IRQs */
@@ -786,6 +788,18 @@  static int xdma_irq_init(struct xdma_device *xdev)
 		goto failed;
 	}
 
+	/* config user IRQ registers if needed */
+	user_irq_start = xdev->h2c_chan_num + xdev->c2h_chan_num;
+	if (xdev->irq_num > user_irq_start) {
+		ret = xdma_set_vector_reg(xdev, XDMA_IRQ_USER_VEC_NUM,
+					  user_irq_start,
+					  xdev->irq_num - user_irq_start);
+		if (ret) {
+			xdma_err(xdev, "failed to set user vectors: %d", ret);
+			goto failed;
+		}
+	}
+
 	/* enable interrupt */
 	ret = xdma_enable_intr(xdev);
 	if (ret) {
@@ -816,6 +830,72 @@  static bool xdma_filter_fn(struct dma_chan *chan, void *param)
 	return true;
 }
 
+/**
+ * xdma_free_user_irq - Free user interrupt
+ * @pdev: Pointer to the platform_device structure
+ * @user_irq_index: User IRQ index
+ * @arg: User interrupt cookie
+ */
+void xdma_free_user_irq(struct platform_device *pdev, u32 user_irq_index,
+			void *arg)
+{
+	struct xdma_device *xdev = platform_get_drvdata(pdev);
+	u32 user_irq;
+
+	user_irq = xdev->h2c_chan_num + xdev->c2h_chan_num + user_irq_index;
+	if (user_irq >= xdev->irq_num) {
+		xdma_err(xdev, "invalid user irq index");
+		return;
+	}
+	user_irq += xdev->irq_start;
+
+	xdma_write_reg(xdev, XDMA_IRQ_BASE, XDMA_IRQ_USER_INT_EN_W1C,
+		       (1 << user_irq_index));
+
+	free_irq(user_irq, arg);
+}
+EXPORT_SYMBOL(xdma_free_user_irq);
+
+/**
+ * xdma_request_user_irq - Register user interrupt
+ * @pdev: Pointer to the platform_device structure
+ * @user_irq_index: User IRQ index
+ * @flags: Handling flags
+ * @handler: User interrupt handler
+ * @arg: User interrupt cookie
+ */
+int xdma_request_user_irq(struct platform_device *pdev, u32 user_irq_index,
+			  irq_handler_t handler, void *arg, ulong flags)
+{
+	struct xdma_device *xdev = platform_get_drvdata(pdev);
+	u32 user_irq;
+	int ret;
+
+	user_irq = xdev->h2c_chan_num + xdev->c2h_chan_num + user_irq_index;
+	if (user_irq >= xdev->irq_num) {
+		xdma_err(xdev, "invalid user irq index");
+		return -EINVAL;
+	}
+	user_irq += xdev->irq_start;
+
+	ret = request_irq(user_irq, handler, flags, "xdma-user", arg);
+	if (ret) {
+		xdma_err(xdev, "request irq failed, %d", ret);
+		return ret;
+	}
+
+	ret = xdma_write_reg(xdev, XDMA_IRQ_BASE, XDMA_IRQ_USER_INT_EN_W1S,
+			     (1 << user_irq_index));
+	if (ret) {
+		xdma_err(xdev, "set user irq mask failed, %d", ret);
+		free_irq(user_irq, arg);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(xdma_request_user_irq);
+
 /**
  * xdma_remove - Driver remove function
  * @pdev: Pointer to the platform_device structure
diff --git a/include/linux/dma/amd_xdma.h b/include/linux/dma/amd_xdma.h
new file mode 100644
index 000000000000..b70486121ca6
--- /dev/null
+++ b/include/linux/dma/amd_xdma.h
@@ -0,0 +1,17 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2022, Advanced Micro Devices, Inc.
+ */
+
+#ifndef _DMAENGINE_AMD_XDMA_H
+#define _DMAENGINE_AMD_XDMA_H
+
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+
+int xdma_request_user_irq(struct platform_device *pdev, u32 user_irq_index,
+			  irq_handler_t handler, void *arg, ulong flags);
+void xdma_free_user_irq(struct platform_device *pdev, u32 user_irq_index,
+			void *arg);
+
+#endif /* _DMAENGINE_AMD_XDMA_H */