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

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

On 10/3/22 06:59, Martin Tůma wrote:
> On 29. 09. 22 1:58, 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 enable/disable user logic interrupt 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    | 65 ++++++++++++++++++++++++++++++++++++
>>   include/linux/dma/amd_xdma.h | 16 +++++++++
>>   3 files changed, 82 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..13f627445c4a 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,57 @@ static bool xdma_filter_fn(struct dma_chan 
>> *chan, void *param)
>>       return true;
>>   }
>>   +/**
>> + * xdma_disable_user_irq - Disable user interrupt
>> + * @pdev: Pointer to the platform_device structure
>> + * @user_irq_index: User IRQ index
>> + */
>> +void xdma_disable_user_irq(struct platform_device *pdev, u32 
>> user_irq_index)
>> +{
>> +    struct xdma_device *xdev = platform_get_drvdata(pdev);
>> +
>> +    if (xdev->h2c_chan_num + xdev->c2h_chan_num + user_irq_index >=
>> +        xdev->irq_num) {
>> +        xdma_err(xdev, "invalid user irq index");
>> +        return;
>> +    }
>> +
>> +    xdma_write_reg(xdev, XDMA_IRQ_BASE, XDMA_IRQ_USER_INT_EN_W1C,
>> +               (1 << user_irq_index));
>> +}
>> +EXPORT_SYMBOL(xdma_disable_user_irq);
>> +
>> +/**
>> + * xdma_enable_user_irq - Enable user logic interrupt
>> + * @pdev: Pointer to the platform_device structure
>> + * @user_irq_index: User logic IRQ wire index
>> + * @irq: Pointer to returning system IRQ number
>> + */
>> +int xdma_enable_user_irq(struct platform_device *pdev, u32 
>> user_irq_index,
>> +             u32 *irq)
>> +{
>> +    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;
>> +    }
>> +
>> +    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);
>> +        return ret;
>> +    }
>> +    *irq = user_irq + xdev->irq_start;
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL(xdma_enable_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..91fb02ff93a7
>> --- /dev/null
>> +++ b/include/linux/dma/amd_xdma.h
>> @@ -0,0 +1,16 @@
>> +/* 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_enable_user_irq(struct platform_device *pdev, u32 
>> user_irq_index,
>> +             u32 *irq);
>> +void xdma_disable_user_irq(struct platform_device *pdev, u32 
>> user_irq_index);
>> +
>> +#endif /* _DMAENGINE_AMD_XDMA_H */
>
> Hi,
> While rewriting our V4L2 driver to use your XDMA driver, I realized, 
> that the API for the user interrupts is still not fully usable. If
> the expected outcome is that the "parent" driver using the xdma driver 
> knows nothing about the IRQ allocation in XDMA, then the 
> xdma_enable_user_irq() function must be split into two separate 
> functions:
>
> int xdma_enable_user_irq(struct platform_device *pdev, u32 
> user_irq_index)
>
> and
>
> int xdma_get_system_irq(u32 user_irq_index)
>
> that returns the system IRQ number for the given user_irq_index.
> Because without it you can not get the system IRQ number without 
> enabling the irq at the same time which is usually not what you want.

Is this because the user logic you are using does not have 
disable/enable interrupt by itself?  I thought that the user logic IP 
would have its own register to enable/disable interrupt.

And xdma_enable_user_irq() will only be called once for creating the 
user logic platform device. Then, user logic IP driver would use its own 
register to control interrupts. The existing platform driver does not 
call any xdma_* to control interrupt.


Thanks,

Lizhi

>
> As a workaround, you can compute the system IRQ yourself with the 
> knowledge of the XDMA internals (this is what I have to do at the 
> moment in our driver), but then the "u32 *irq" parameter becomes 
> useless and can be removed anyway.
>
> M.