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

[Lizhi Hou <lizhi.hou@amd.com>]

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