Messages in this thread | | | Date | Tue, 4 Oct 2022 09:25:09 -0700 | Subject | Re: [PATCH V5 XDMA 2/2] dmaengine: xilinx: xdma: Add user logic interrupt support | From | Lizhi Hou <> |
| |
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.
| |