RE: [PATCH V3 1/3] dmaengine: dw-axi-dmac: support DMAX_NUM_CHANNELS > 8

["N, Pandith" <pandith.n@intel.com>]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Tuesday, October 26, 2021 5:40 PM
> To: N, Pandith <pandith.n@intel.com>
> Cc: Vinod <vkoul@kernel.org>; Eugeniy Paltsev
> <eugeniy.paltsev@synopsys.com>; dmaengine <dmaengine@vger.kernel.org>;
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Sangannavar,
> Mallikarjunappa <mallikarjunappa.sangannavar@intel.com>; Thokala, Srikanth
> <srikanth.thokala@intel.com>; Demakkanavar, Kenchappa
> <kenchappa.demakkanavar@intel.com>; Emil Renner Berthing
> <kernel@esmil.dk>; Michael Zhu <michael.zhu@starfivetech.com>
> 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@intel.com> wrote:
> > From: Pandith N <pandith.n@intel.com>
> >
> > 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@intel.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> 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