Hi Geert,
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: Tuesday, October 26, 2021 5:40 PM
> To: N, Pandith <pandith.n@xxxxxxxxx>
> Cc: Vinod <vkoul@xxxxxxxxxx>; Eugeniy Paltsev
> <eugeniy.paltsev@xxxxxxxxxxxx>; dmaengine <dmaengine@xxxxxxxxxxxxxxx>;
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Sangannavar,
> Mallikarjunappa <mallikarjunappa.sangannavar@xxxxxxxxx>; Thokala, Srikanth
> <srikanth.thokala@xxxxxxxxx>; Demakkanavar, Kenchappa
> <kenchappa.demakkanavar@xxxxxxxxx>; Emil Renner Berthing
> <kernel@xxxxxxxx>; Michael Zhu <michael.zhu@xxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH V3 1/3] dmaengine: dw-axi-dmac: support
> DMAX_NUM_CHANNELS > 8
>
> Hi Pandith,
>
> On Fri, Oct 1, 2021 at 4:13 PM <pandith.n@xxxxxxxxx> wrote:
> > From: Pandith N <pandith.n@xxxxxxxxx>
> >
> > Added support for DMA controller with more than 8 channels.
> > DMAC register map changes based on number of channels.
> >
> > Enabling DMAC channel:
> > DMAC_CHENREG has to be used when number of channels <= 8
> > DMAC_CHENREG2 has to be used when number of channels > 8
> >
> > Configuring DMA channel:
> > CHx_CFG has to be used when number of channels <= 8
> > CHx_CFG2 has to be used when number of channels > 8
> >
> > Suspending and resuming channel:
> > DMAC_CHENREG has to be used when number of channels <= 8
> > DMAC_CHSUSPREG has to be used for suspending a channel > 8
> >
> > Signed-off-by: Pandith N <pandith.n@xxxxxxxxx>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>
> Thanks for your patch, which is now commit 824351668a413af7
> ("dmaengine: dw-axi-dmac: support DMAX_NUM_CHANNELS > 8") in
> dmaengine/next.
>
> > ---
> > Changes V1-->V2:
> > Initialize register values in channel resume and pause Removed
> > unwanted braces in flow control setting.
> >
> > Changes from v2->v3
> > check if channel is enabled, before suspending.
>
> I couldn't find these versions in the archive, so I don't know if my comments
> below were made before...
>
> > --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> > +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
>
> > @@ -1120,10 +1150,17 @@ static int dma_chan_pause(struct dma_chan
> > *dchan)
> >
> > spin_lock_irqsave(&chan->vc.lock, flags);
> >
> > - val = axi_dma_ioread32(chan->chip, DMAC_CHEN);
> > - val |= BIT(chan->id) << DMAC_CHAN_SUSP_SHIFT |
> > - BIT(chan->id) << DMAC_CHAN_SUSP_WE_SHIFT;
> > - axi_dma_iowrite32(chan->chip, DMAC_CHEN, val);
> > + if (chan->chip->dw->hdata->reg_map_8_channels) {
> > + val = axi_dma_ioread32(chan->chip, DMAC_CHEN);
> > + val |= BIT(chan->id) << DMAC_CHAN_SUSP_SHIFT |
> > + BIT(chan->id) << DMAC_CHAN_SUSP_WE_SHIFT;
> > + axi_dma_iowrite32(chan->chip, DMAC_CHEN, val);
> > + } else {
> > + val = 0;
>
> So unlike for the DMAC_CHEN register, you don't have to retain the old values
> for the DMAC_CHSUSPREG register?
>
DMAC_CHSUSPREG registers has definitions only for channel suspension.
CH_SUSP is written only if the corresponding channel write enable bit is set, hence no need to retain old values.
> > + val |= BIT(chan->id) << DMAC_CHAN_SUSP2_SHIFT |
> > + BIT(chan->id) << DMAC_CHAN_SUSP2_WE_SHIFT;
>
> Why not "val = BIT(...) | BIT(...)"?
>
Yes, this could be "val = BIT(...) | BIT(...)"
> > + axi_dma_iowrite32(chan->chip, DMAC_CHSUSPREG, val);
> > + }
> >
> > do {
> > if (axi_chan_irq_read(chan) & DWAXIDMAC_IRQ_SUSPENDED)
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But when
> I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
