Re: [PATCH v2 2/3] dma: amlogic: Add general DMA driver for A9

[Frank Li <Frank.li@nxp.com>]

On Mon, Feb 02, 2026 at 05:23:13PM +0800, Xianwei Zhao wrote:
> Hi Frank,
>    Thanks for your reply.
>
> On 2026/1/30 02:04, Frank Li wrote:
> > [You don't often get email from frank.li@nxp.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
>
> Will do.
>

Needn't this next time to save reader time. Just leave part, which need
discussion.

> > > +
...
> > > +             /* set dma address and len  to sglink*/
> > > +             sg_link->address = sg->dma_address;
> > > +             sg_link->ctl = FIELD_PREP(LINK_LEN, sg_dma_len(sg));
> > > +
> > > +             aml_chan->data_len += sg_dma_len(sg);
> > > +     }
> > > +     aml_chan->sg_link_cnt = idx;
> > > +
> > > +     return &aml_chan->desc;
> >
> > Here have problems, if caller
> >
> >          a = dmaengine_prep_slave_sg();
> >          ...
> >          b = dmaengine_prep_slave_sg();
> >
> >          submit(a); submit(b);
> >
> > API don't limit that prep_slave_sg() must follow submit().
> >
>
> Our DMA module uses a dedicated DMA channel per device and supports
> scatter-gather mode.
>
> In function "prep_slave_sg" , data list has been placed in the hardware
> executable queue,
> and the "submit" do nothing but assign cookies to prevent function
> dma_async_is_tx_complete called error.

Vnod can provide more comments. but according to my options, you should
not have such assumption. If one driver use dma provider 1 work, some daysi
later, switch to provider 2, it fail to work slience.

DMA doc already define API and hehavors, it'd better to follow it.

>
> When device_issue_pending is called, all data list will be processed.
>
> > > +}
> > > +
...
> > > +static void aml_dma_enable_chan(struct dma_chan *chan)
> > > +{
> > > +     struct aml_dma_chan *aml_chan = to_aml_dma_chan(chan);
> > > +     struct aml_dma_dev *aml_dma = aml_chan->aml_dma;
> > > +     struct aml_dma_sg_link *sg_link;
> > > +     int chan_id = aml_chan->chan_id;
> > > +     int idx = aml_chan->sg_link_cnt - 1;
> > > +
> > > +     /* the last sg set eoc flag */
> > > +     sg_link = &aml_chan->sg_link[idx];
> > > +     sg_link->ctl |= LINK_EOC;
> > > +     dma_wmb();
> >
> > why need it? regmap_write() already have dma_wmb();
> >
>
> regmap_write will call wmb() / iowmb(), not dma_wmb.
> So I think it's best to keep it.

It included dma_wmb() in iowmb(), some system use the same implemention for
iowmb to dwm_wmb().

All memory barray require comments.  Redundant dwm will cause others
confuse.

If it doesn't work without dma_wmb(), your wmb() will have bigger problem
in your system, which will be really hard to debug.

Frank

>
> > Frank
> > > +     if (aml_chan->direction == DMA_MEM_TO_DEV) {
> > > +             regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_ADDR,
> > > +                          aml_chan->sg_link_phys);
> > > +             regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_LEN, aml_chan->data_len);
> > > +             regmap_update_bits(aml_dma->regmap, RCH_INT_MASK, BIT(chan_id), 0);
> > > +             /* for rch (tx) need set cfg 0 to trigger start */
> > > +             regmap_write(aml_dma->regmap, aml_chan->reg_offs + RCH_CFG, 0);
> > > +     } else if (aml_chan->direction == DMA_DEV_TO_MEM) {
> > > +             regmap_write(aml_dma->regmap, aml_chan->reg_offs + WCH_ADDR,
> > > +                          aml_chan->sg_link_phys);
> > > +             regmap_write(aml_dma->regmap, aml_chan->reg_offs + WCH_LEN, aml_chan->data_len);
> > > +             regmap_update_bits(aml_dma->regmap, WCH_INT_MASK, BIT(chan_id), 0);
> > > +     }
> > > +}
...
> > > +
> > > +             .of_match_table = aml_dma_ids,
> > > +     },
> > > +};
> > > +
> > > +module_platform_driver(aml_dma_driver);
> > > +
> > > +MODULE_DESCRIPTION("GENERAL DMA driver for Amlogic");
> > > +MODULE_AUTHOR("Xianwei Zhao <xianwei.zhao@amlogic.com>");
> > > +MODULE_LICENSE("GPL");
> > >
> > > --
> > > 2.52.0
> > >

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic