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: <7600d6a0-79ab-143d-28c8-77320d7ba12c@amd.com>
Date:   Tue, 8 Nov 2022 09:55:48 -0800
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 V9 XDMA 1/2] dmaengine: xilinx: xdma: Add xilinx xdma
 driver
Content-Language: en-US
To:     Vinod Koul <vkoul@kernel.org>
CC:     <dmaengine@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <trix@redhat.com>, <tumic@gpxsee.org>, <max.zhen@amd.com>,
        <sonal.santan@amd.com>, <larry.liu@amd.com>, <brian.xu@amd.com>
References: <1666720638-33496-1-git-send-email-lizhi.hou@amd.com>
 <1666720638-33496-2-git-send-email-lizhi.hou@amd.com>
 <Y2Uidg/iuQdxwSaG@matsya> <73ab286c-f8c7-8c4b-0508-5df8dde8e305@amd.com>
 <Y2VSfjWZSWY0f2n4@matsya> <e1244ca1-18bd-d6c9-6101-9b20940fda72@amd.com>
 <Y2YOHx3RBJxdx9xz@matsya>
From:   Lizhi Hou <lizhi.hou@amd.com>
In-Reply-To: <Y2YOHx3RBJxdx9xz@matsya>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Nov 2022 17:55:50.4293
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: e5d0272f-956d-49df-4480-08dac1b27905
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: DM6NAM11FT113.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB7115
X-Spam-Status: No, score=-2.1 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 11/5/22 00:17, Vinod Koul wrote:
> On 04-11-22, 11:17, Lizhi Hou wrote:
>> On 11/4/22 10:57, Vinod Koul wrote:
>>> On 04-11-22, 09:57, Lizhi Hou wrote:
>>>> On 11/4/22 07:32, Vinod Koul wrote:
>>>>> On 25-10-22, 10:57, Lizhi Hou wrote:
>>>>>> +static inline int xdma_write_reg(struct xdma_device *xdev, u32 base, u32 reg,
>>>>>> +				 u32 val)
>>>>>> +{
>>>>>> +	return regmap_write(xdev->regmap, base + reg, val);
>>>>>> +}
>>>>> Do you really need one more level indirection?
>>>> Do you mean using readl / writel instead of regmap_* here?
>>> Nope, I refer to using regmap_write() intead of xdma_write_reg()
>> Ok. As you mentioned below,
>>
>>     why not move err into xdma_write_reg(), rather than adding in each
>>     helper!
>>
>> If I use regmap_write() directly, I will not be able to move err into
>> xdma_write_reg(). Having a inline function might be useful to add debug
>> code.  May I keep xdma_write_reg()?
> Okay, either way if xdma_write_reg() is doing only regmap_write() then
> no, if it has extra logic like logging on error etc then it makes sense
Got it. Thanks. :)
>
>>>>>> +failed:
>>>>>> +	xdma_free_desc(&sw_desc->vdesc);
>>>>> who will free sw_desc here?
>>>> sw_desc is freed by xdma_free_desc(). xdma_free_desc() is virt-dma callback
>>>> it converts struct virt_dma_desc pointer to driver sw_desc pointer and free
>>>> the whole thing.
>>> IN case of error, you are returning NULL, so allocated descriptor leaks
>> I meant the descriptor is freed inside xdma_free_desc() which is called
>> before 'return NULL'.
>>
>> xdma_free_desc(struct virt_dma_desc *vdesc)
>>
>> {
>>
>>          sw_desc = to_xdma_desc(vdesc);
>>
>>          .....
>>
>>          kfree(sw_desc);
>>
>> }
> ok
>
>>>>>> +#ifndef _PLATDATA_AMD_XDMA_H
>>>>>> +#define _PLATDATA_AMD_XDMA_H
>>>>>> +
>>>>>> +#include <linux/dmaengine.h>
>>>>>> +
>>>>>> +/**
>>>>>> + * struct xdma_chan_info - DMA channel information
>>>>>> + *	This information is used to match channel when request dma channel
>>>>>> + * @dir: Channel transfer direction
>>>>>> + */
>>>>>> +struct xdma_chan_info {
>>>>>> +	enum dma_transfer_direction dir;
>>>>>> +};
>>>>>> +
>>>>>> +#define XDMA_FILTER_PARAM(chan_info)	((void *)(chan_info))
>>>>>> +
>>>>>> +struct dma_slave_map;
>>>>>> +
>>>>>> +/**
>>>>>> + * struct xdma_platdata - platform specific data for XDMA engine
>>>>>> + * @max_dma_channels: Maximum dma channels in each direction
>>>>>> + */
>>>>>> +struct xdma_platdata {
>>>>>> +	u32 max_dma_channels;
>>>>>> +	u32 device_map_cnt;
>>>>>> +	struct dma_slave_map *device_map;
>>>>>> +};
>>>>> why do you need this plat data
>>>> max_dma_channels is used to specify the maximum dma channels will be used.
>>> What is the device mode, who creates this dma device, devicetree or
>>> something else?
>> This dma engine is on PCI device (exposed on PCI BAR). Thus, the pci device
>> driver creates this dma device.
> So it is a platform_device type? Why not make it something like auxdev?

With our FPGA device, the XDMA IP is populated by flattened device tree. 
Using platform device will support both device tree and non-dt case.


Thanks,
Lizhi

>