Re: [PATCH v9 2/2] Add Intel LGM SoC DMA support.

["Reddy, MallikarjunaX" <mallikarjunax.reddy@linux.intel.com>]

Hi Vinod,

Thanks for your valuable review comments. Please see my comments inline.

On 11/21/2020 8:17 PM, Vinod Koul wrote:
> On 20-11-20, 19:30, Reddy, MallikarjunaX wrote:
>> Hi Vinod,
>>
>> Thanks for the review. My comments inline.
>>
>> On 11/19/2020 1:38 AM, Vinod Koul wrote:
>>> On 12-11-20, 13:38, Amireddy Mallikarjuna reddy wrote:
>>>> Add DMA controller driver for Lightning Mountain (LGM) family of SoCs.
>>>>
>>>> The main function of the DMA controller is the transfer of data from/to any
>>>> peripheral to/from the memory. A memory to memory copy capability can also
>>>> be configured.
>>>>
>>>> This ldma driver is used for configure the device and channnels for data
>>>> and control paths.
>>>>
>>>> Signed-off-by: Amireddy Mallikarjuna reddy <mallikarjunax.reddy@linux.intel.com>
>>>> ---
>>>> v1:
>>>> - Initial version.
>>> You have a cover letter, use that to keep track of these changes
>> ok.
>>>> +++ b/drivers/dma/lgm/Kconfig
>>>> @@ -0,0 +1,9 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>> +config INTEL_LDMA
>>>> +	bool "Lightning Mountain centralized low speed DMA and high speed DMA controllers"
>>> Do we have any other speeds :D
>> No other speeds :-)
> Right, so possibly drop the speed characterization here!
"Lightning Mountain centralized DMA controller"
>
>>>> +++ b/drivers/dma/lgm/lgm-dma.c
>>>> @@ -0,0 +1,1742 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Lightning Mountain centralized low speed and high speed DMA controller driver
>>>> + *
>>>> + * Copyright (c) 2016 ~ 2020 Intel Corporation.
>>> I think you mean 2016 - 2020, a dash which refers to duration
>> ok.
>>>> +struct dw2_desc {
>>>> +	struct {
>>>> +		u32 len		:16;
>>>> +		u32 res0	:7;
>>>> +		u32 bofs	:2;
>>>> +		u32 res1	:3;
>>>> +		u32 eop		:1;
>>>> +		u32 sop		:1;
>>>> +		u32 c		:1;
>>>> +		u32 own		:1;
>>>> +	} __packed field;
>>> Another one, looks like folks adding dmaengine patches love this
>>> approach, second one for the day..
>>>
>>> Now why do you need the bit fields, why not use register defines and
>>> helpers in bitfield.h to help configure the fields See FIELD_GET,
>>> FIELD_PREP etc
>> Let me check on this...
>>>> +struct dma_dev_ops {
>>>> +	int (*device_alloc_chan_resources)(struct dma_chan *chan);
>>>> +	void (*device_free_chan_resources)(struct dma_chan *chan);
>>>> +	int (*device_config)(struct dma_chan *chan,
>>>> +			     struct dma_slave_config *config);
>>>> +	int (*device_pause)(struct dma_chan *chan);
>>>> +	int (*device_resume)(struct dma_chan *chan);
>>>> +	int (*device_terminate_all)(struct dma_chan *chan);
>>>> +	void (*device_synchronize)(struct dma_chan *chan);
>>>> +	enum dma_status (*device_tx_status)(struct dma_chan *chan,
>>>> +					    dma_cookie_t cookie,
>>>> +					    struct dma_tx_state *txstate);
>>>> +	struct dma_async_tx_descriptor *(*device_prep_slave_sg)
>>>> +		(struct dma_chan *chan, struct scatterlist *sgl,
>>>> +		unsigned int sg_len, enum dma_transfer_direction direction,
>>>> +		unsigned long flags, void *context);
>>>> +	void (*device_issue_pending)(struct dma_chan *chan);
>>>> +};
>>> Heh! why do you have a copy of dmaengine ops here?
>> Ok, i will remove the ops and update the code accordingly.
>>>> +static int ldma_chan_desc_cfg(struct ldma_chan *c, dma_addr_t desc_base,
>>>> +			      int desc_num)
>>>> +{
>>>> +	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
>>>> +
>>>> +	if (!desc_num) {
>>>> +		dev_err(d->dev, "Channel %d must allocate descriptor first\n",
>>>> +			c->nr);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	if (desc_num > DMA_MAX_DESC_NUM) {
>>>> +		dev_err(d->dev, "Channel %d descriptor number out of range %d\n",
>>>> +			c->nr, desc_num);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ldma_chan_desc_hw_cfg(c, desc_base, desc_num);
>>>> +
>>>> +	c->flags |= DMA_HW_DESC;
>>>> +	c->desc_cnt = desc_num;
>>>> +	c->desc_phys = desc_base;
>>> So you have a custom API which is used to configure this flag, a number
>>> and an address. The question is why, can you please help explain this?
>> LDMA used as general purpose dma(ver == DMA_VER22) and also supports DMA
>> capability for GSWIP in their network packet processing.( ver > DMA_VER22)
> Whats GSWIP?
>
>> Each Ingress(IGP) & Egress(EGP) ports of CQM use a dma channel.
> CQM?
GSWIP stands for  Gigabit Switch IP, and CQM is Central Queue Manager.

GSWIP & CQM are the clients for the DMA. These are used in networking 
purpose to increase transfer rates for peripheral like the GSWIP LAN switch.
>
>> desc needs to be configure for each dma channel and the remapped address of
>> the IGP & EGP is desc base adress.
> Why should this address not passed as src_addr/dst_addr?
src_addr/dst_addr is the data pointer. Data pointer indicates address 
pointer of data buffer.

ldma_chan_desc_cfg() carries the descriptor address.

The descriptor list entry contains the data pointer, which points to the 
data section in the memory.

So we should not use src_addr/dst_addr as desc base address.
>
>> CQM client is using ldma_chan_desc_cfg() to configure the descriptior.
>>>> +static void dma_issue_pending(struct dma_chan *chan)
>>>> +{
>>>> +	struct ldma_chan *c = to_ldma_chan(chan);
>>>> +	struct ldma_dev *d = to_ldma_dev(c->vchan.chan.device);
>>>> +	unsigned long flags;
>>>> +
>>>> +	if (d->ver == DMA_VER22) {
>>> why is this specific to this version?
>> Only dma0 instance (ver == DMA_VER22) is used as a general purpose slave
>> DMA. we set both control and datapath here.
>> Other instances (ver > DMA_VER22) we set only control path. data path is
>> taken care by dma client(GSWIP).
>> Only thing needs to do is get the channel, set the descriptor and just 'ON'
>> the channel.
>>
>> CQM is highly low level register configurable/programmable take care about
>> the the packet processing through the register configurations.
> DMAengine fwk take care of channel management for clients and
> transaction management, if you not going to do transactions then why
> bother with dmaengine ?
dma0 instance (ver == DMA_VER22) uses DMAengine framework for both 
channel management and transaction management.

Other instances (ver > DMA_VER22) uses DMAengine mainly for channel 
management.
To initiate the transaction client needs to ON the corresponding 
channel, So dmaengine ops are using to 'ON'  and 'OFF' the channels.