Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Message-ID: <2cb4da1b-bea8-245a-d9b2-ba08ae36ffcf@amd.com>
Date:   Tue, 4 Oct 2022 09:25:09 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH V5 XDMA 2/2] dmaengine: xilinx: xdma: Add user logic
 interrupt support
Content-Language: en-US
To:     =?UTF-8?Q?Martin_T=c5=afma?= <tumic@gpxsee.org>,
        <vkoul@kernel.org>, <dmaengine@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <trix@redhat.com>
CC:     <max.zhen@amd.com>, <sonal.santan@amd.com>, <larry.liu@amd.com>,
        <brian.xu@amd.com>
References: <1664409507-64079-1-git-send-email-lizhi.hou@amd.com>
 <1664409507-64079-3-git-send-email-lizhi.hou@amd.com>
 <33583d7f-8f8c-64a1-7b93-4e6aebb266a3@gpxsee.org>
 <1836ce13-101f-a863-41ea-bd49c21d4f60@amd.com>
 <daccee4a-ac3c-bfc1-4876-24e6ecf5bcf1@gpxsee.org>
From:   Lizhi Hou <lizhi.hou@amd.com>
In-Reply-To: <daccee4a-ac3c-bfc1-4876-24e6ecf5bcf1@gpxsee.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Oct 2022 16:25:12.1831
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 3ca536b2-a736-4075-484a-08daa6250322
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT106.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR12MB6709
X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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.