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: <2339d3e2-1c1d-4005-b5bc-e115f3c7a6cc@amd.com>
Date:   Tue, 27 Sep 2022 11:14:44 -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>
 <64388266-1707-ee20-c3ab-edb67ada68dc@amd.com>
 <c0430eb7-d607-acfd-201a-db56db982309@gpxsee.org>
 <5f77987e-49bc-e035-19e0-52c25f4adc7e@amd.com>
 <68fdd2a3-b881-470b-c5b3-0f2fc881ed27@gpxsee.org>
From:   Lizhi Hou <lizhi.hou@amd.com>
In-Reply-To: <68fdd2a3-b881-470b-c5b3-0f2fc881ed27@gpxsee.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Sep 2022 18:14:45.9877
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 5dfd39a7-9081-4662-9cd1-08daa0b42886
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: DM6NAM11FT027.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4155
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 10:54, Martin Tůma wrote:
> On 27. 09. 22 19:18, Lizhi Hou wrote:
>>
>> On 9/27/22 09:46, Martin Tůma wrote:
>>> On 27. 09. 22 18:28, Lizhi Hou wrote:
>>>
>>>> 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?
>>>>
>>>
>>> I think even the "irq" parameter in the enable function is surplus 
>>> as the parent driver (the driver of the actual PCIe card) knows* 
>>> what PCI irq he has to allocate without XDMA providing the number.
>>>
>>> xdma_enable_user_irq(struct platform_device *pdev, u32 user_irq_index);
>>> xdma_disable_user_irq(struct platform_device *pdev, u32 
>>> user_irq_index);
>>>
>>> should be all that is needed.
>>>
>>> M.
>>>
>>> * something like:
>>> pci_irq_vector((pdev), PCI_BAR_ID) + NUM_C2H_CHANNELS + 
>>> NUM_H2C_CHANNELS
>>> can be used from the PCIe driver
>>
>> How does parent driver know the first few vectors will be assigned to 
>> DMA channel?  Parent diver should not assume the first 
>> (NUM_C2H_CHANNELS+NUM_H2C_CHANNELS) are for DMA channel.
>>
>> Parent driver passes the system IRQ range  to XDMA driver, and only 
>> XDMA driver knows what IRQs are used by DMA channel and what IRQs are 
>> mapped to user logic wires. I would keep the "u32 *irq" argument.
>>
>
> The parent driver knows how much DMA channels it wants/allocates. If 
> it is possible to allocate different IRQs than the first 
> NUM_C2H_CHANNELS + NUM_H2C_CHANNELS to the XDMA IP core, than that 
> parameter may be needed, but I haven't seen such HW. Moreover, every 
> parent driver author should IMHO know how the channel and user IRQs 
> are mapped in their specific HW configuration so this info can be 
> "hard-wired" in the parent driver, but I'm fine with it when the irq 
> parameter is kept anyway. All I really need is that the enable/disable 
> logic is split from the irq allocate/register logic so I can use the 
> other platform drivers.
>
> M.

Thanks Mark, all your comments are very helpful. And glad to know you 
are fine with irq parameter. The IRQ binding is set by xdma driver ( 
xdma_set_vector_reg() ). That means xdma driver is able to map any 
system IRQ to dma channel or user logic. That is why I prefer to keep 
irq parameter. Just a FYI.

Lizhi