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=SATLEXMB03.amd.com; pr=C
Message-ID: <64388266-1707-ee20-c3ab-edb67ada68dc@amd.com>
Date:   Tue, 27 Sep 2022 09:28:39 -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 V4 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: <1663871905-60498-1-git-send-email-lizhi.hou@amd.com>
 <1663871905-60498-3-git-send-email-lizhi.hou@amd.com>
 <a2b5c35d-ced2-ff83-c550-4bbf29973b80@gpxsee.org>
From:   Lizhi Hou <lizhi.hou@amd.com>
In-Reply-To: <a2b5c35d-ced2-ff83-c550-4bbf29973b80@gpxsee.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Sep 2022 16:28:41.3397
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 56115458-ce1a-4d5c-b791-08daa0a556f4
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=[SATLEXMB03.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT005.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6409
X-Spam-Status: No, score=-4.4 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 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