lkml.org 
[lkml]   [2020]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v9 2/2] Add Intel LGM SoC DMA support.
From
Date
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.

\
 
 \ /
  Last update: 2020-11-23 17:32    [W:0.100 / U:0.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site