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 [email protected]. 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 <[email protected]>");
> > > +MODULE_LICENSE("GPL");
> > >
> > > --
> > > 2.52.0
> > >
