Re: [PATCH v4 7/8] dmaengine: dw-edma: add flags at struct dw_edma_chip

[Serge Semin <fancer.lancer@gmail.com>]

On Thu, Mar 10, 2022 at 11:54:30AM -0600, Zhi Li wrote:
> On Thu, Mar 10, 2022 at 11:47 AM Serge Semin <fancer.lancer@gmail.com> wrote:
> >
> > On Wed, Mar 09, 2022 at 03:12:03PM -0600, Frank Li wrote:
> > > Allow PCI EP probe DMA locally and prevent use of remote MSI
> > > to remote PCI host.
> > >
> > > Add option to force 32bit DBI register access even on
> > > 64-bit systems. i.MX8 hardware only allowed 32bit register
> > > access.
> >
> > Could you please split this patch up into two? These flags are
> > unrelated thus adding them is two unrelated changes. That can be
> > implicitly inferred from your commit log and the patch title.
> 

> I don't think it needs to be separated.  It also show why need 32bit mask to
> control features and reserved futured extension capability .
> 
> The two flags were descriptions for EDMA chip features.

Both of these flags define separate platform features. Each of which
can be distinctively specific for a particular SoC, not only for the
i.MX8. Even though your log messages does mention i.MX8 hardware the
corresponding flag is defined as generic in the patch. As I said both
flags are unrelated to each other and can be independently specific to
one or another platform. So one SoC can be restricted to use 32bits
DBI IOs and do have the Synopsys PCIe End-point IP prototype kit
detected on the PCIe bus. Another SoC can have the eDMA core embedded
into the DW PCIe EP/Host controller, but with no 32-bits DBI IOs
requirement.

Secondly since both of these flags are unrelated then having a
monolithic patch as you suggest will harden the bisection procedure in
case of a bug is caused by one of these modifications. Bisecting will
lead straight to the bogus change if the patch is split up into two.

Thirdly as I said before your commit log states two distinctive
changes, which means the log can be split up into two together with
the logical changes. In addition referring to a particular change in
further commits will be more informative.

Finally please see [1] regarding the patches splitting up, logical changes,
bisection, etc.

[1] Documentation/process/submitting-patches.rst: "Separate your changes"

-Sergey

> 
> >
> > -Sergey
> >
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > Change from v3 to v4
> > >  - None
> > > Change from v2 to v3
> > >  - rework commit message
> > >  - Change to DW_EDMA_CHIP_32BIT_DBI
> > >  - using DW_EDMA_CHIP_LOCAL control msi
> > >  - Apply Bjorn's comments,
> > >         if (!j) {
> > >                control |= DW_EDMA_V0_LIE;
> > >                if (!(chan->chip->flags & DW_EDMA_CHIP_LOCAL))
> > >                                control |= DW_EDMA_V0_RIE;
> > >         }
> > >
> > >         if ((chan->chip->flags & DW_EDMA_CHIP_REG32BIT) ||
> > >               !IS_ENABLED(CONFIG_64BIT)) {
> > >           SET_CH_32(...);
> > >           SET_CH_32(...);
> > >        } else {
> > >           SET_CH_64(...);
> > >        }
> > >
> > >
> > > Change from v1 to v2
> > > - none
> > >  drivers/dma/dw-edma/dw-edma-v0-core.c | 20 ++++++++++++--------
> > >  include/linux/dma/edma.h              |  9 +++++++++
> > >  2 files changed, 21 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > > index 35f2adac93e46..00a00d68d44e7 100644
> > > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> > > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > > @@ -301,6 +301,7 @@ u32 dw_edma_v0_core_status_abort_int(struct dw_edma *dw, enum dw_edma_dir dir)
> > >  static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> > >  {
> > >       struct dw_edma_burst *child;
> > > +     struct dw_edma_chan *chan = chunk->chan;
> > >       struct dw_edma_v0_lli __iomem *lli;
> > >       struct dw_edma_v0_llp __iomem *llp;
> > >       u32 control = 0, i = 0;
> > > @@ -314,9 +315,11 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> > >       j = chunk->bursts_alloc;
> > >       list_for_each_entry(child, &chunk->burst->list, list) {
> > >               j--;
> > > -             if (!j)
> > > -                     control |= (DW_EDMA_V0_LIE | DW_EDMA_V0_RIE);
> > > -
> > > +             if (!j) {
> > > +                     control |= DW_EDMA_V0_LIE;
> > > +                     if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> > > +                             control |= DW_EDMA_V0_RIE;
> > > +             }
> > >               /* Channel control */
> > >               SET_LL_32(&lli[i].control, control);
> > >               /* Transfer size */
> > > @@ -414,15 +417,16 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
> > >               SET_CH_32(dw, chan->dir, chan->id, ch_control1,
> > >                         (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE));
> > >               /* Linked list */
> > > -             #ifdef CONFIG_64BIT
> > > -                     SET_CH_64(dw, chan->dir, chan->id, llp.reg,
> > > -                               chunk->ll_region.paddr);
> > > -             #else /* CONFIG_64BIT */
> > > +             if ((chan->dw->chip->flags & DW_EDMA_CHIP_32BIT_DBI) ||
> > > +                 !IS_ENABLED(CONFIG_64BIT)) {
> > >                       SET_CH_32(dw, chan->dir, chan->id, llp.lsb,
> > >                                 lower_32_bits(chunk->ll_region.paddr));
> > >                       SET_CH_32(dw, chan->dir, chan->id, llp.msb,
> > >                                 upper_32_bits(chunk->ll_region.paddr));
> > > -             #endif /* CONFIG_64BIT */
> > > +             } else {
> > > +                     SET_CH_64(dw, chan->dir, chan->id, llp.reg,
> > > +                               chunk->ll_region.paddr);
> > > +             }
> > >       }
> > >       /* Doorbell */
> > >       SET_RW_32(dw, chan->dir, doorbell,
> > > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> > > index c2039246fc08c..eea11b1d9e688 100644
> > > --- a/include/linux/dma/edma.h
> > > +++ b/include/linux/dma/edma.h
> > > @@ -33,6 +33,12 @@ enum dw_edma_map_format {
> > >       EDMA_MF_HDMA_COMPAT = 0x5
> > >  };
> > >
> > > +/* Probe EDMA engine locally and prevent generate MSI to host side*/
> > > +#define DW_EDMA_CHIP_LOCAL   BIT(0)
> > > +
> > > +/* Only support 32bit DBI register access */
> > > +#define DW_EDMA_CHIP_32BIT_DBI       BIT(1)
> > > +
> > >  /**
> > >   * struct dw_edma_chip - representation of DesignWare eDMA controller hardware
> > >   * @dev:              struct device of the eDMA controller
> > > @@ -40,6 +46,8 @@ enum dw_edma_map_format {
> > >   * @nr_irqs:          total dma irq number
> > >   * @ops                       DMA channel to IRQ number mapping
> > >   * @reg_base          DMA register base address
> > > + * @flags               - DW_EDMA_CHIP_LOCAL
> > > + *                      - DW_EDMA_CHIP_32BIT_DBI
> > >   * @ll_wr_cnt                 DMA write link list number
> > >   * @ll_rd_cnt                 DMA read link list number
> > >   * @rg_region                 DMA register region
> > > @@ -53,6 +61,7 @@ struct dw_edma_chip {
> > >       int                     id;
> > >       int                     nr_irqs;
> > >       const struct dw_edma_core_ops   *ops;
> > > +     u32                     flags;
> > >
> > >       void __iomem            *reg_base;
> > >
> > > --
> > > 2.24.0.rc1
> > >