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

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

On 10/4/22 03:26, Martin Tůma wrote:
>
> On 03. 10. 22 17:52, Lizhi Hou wrote:
>>
>> 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.
>>
> Yes. Our HW has no "own" registers to disable the IRQs. As such 
> registers are already there in XDMA, there was no need for them.
>
>
>> 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.
>>
> As you can see, there is HW that was designed to use the XDMA 
> registers during normal operation and separate enable/disable and irq 
> mapping functions are required. Maybe it is not the best HW design, 
> but it can evidently happen and then the driver author is screwed with 
> the merged API.

Ok. To support this case, I will separate the API as you mentioned.


Thanks,

Lizhi

>
> M.
>
>>
>> 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.