diff mbox series

[v6,7/9] dmaengine: dw-edma: Add support for chip specific flags

Message ID 20220406152347.85908-8-Frank.Li@nxp.com (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series Enable designware PCI EP EDMA locally | expand

Commit Message

Frank Li April 6, 2022, 3:23 p.m. UTC
Add a "flags" field to the "struct dw_edma_chip" so that the controller
drivers can pass flags that are relevant to the platform.

DW_EDMA_CHIP_LOCAL - Used by the controller drivers accessing eDMA
locally. Local eDMA access doesn't require generating MSIs to the remote.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v5 to v6
 - use enum instead of define

Change from v4 to v5
 - split two two patch
 - rework commit message
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 | 9 ++++++---
 include/linux/dma/edma.h              | 9 +++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

Serge Semin April 13, 2022, 9:18 a.m. UTC | #1
On Wed, Apr 06, 2022 at 10:23:45AM -0500, Frank Li wrote:
> Add a "flags" field to the "struct dw_edma_chip" so that the controller
> drivers can pass flags that are relevant to the platform.
> 
> DW_EDMA_CHIP_LOCAL - Used by the controller drivers accessing eDMA
> locally. Local eDMA access doesn't require generating MSIs to the remote.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---

> Change from v5 to v6
>  - use enum instead of define

Hm, why have you decided to do that? I don't see a well justified
reason to use the enumeration here, but see my next comment for
details.

> 
> Change from v4 to v5
>  - split two two patch
>  - rework commit message
> 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 | 9 ++++++---
>  include/linux/dma/edma.h              | 9 +++++++++
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index 8ddc537d11fd6..30f8bfe6e5712 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 */
> diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> index c2039246fc08c..bec444e2939b2 100644
> --- a/include/linux/dma/edma.h
> +++ b/include/linux/dma/edma.h
> @@ -33,6 +33,14 @@ enum dw_edma_map_format {
>  	EDMA_MF_HDMA_COMPAT = 0x5
>  };
>  

> +/**
> + * enum dw_edma_chip_flags - Flags specific to an eDMA chip
> + * @DW_EDMA_CHIP_LOCAL:		eDMA is used locally by an endpoint
> + */
> +enum dw_edma_chip_flags {
> +	DW_EDMA_CHIP_LOCAL	= BIT(0),
> +};
> +
>  /**
>   * struct dw_edma_chip - representation of DesignWare eDMA controller hardware
>   * @dev:		 struct device of the eDMA controller
> @@ -53,6 +61,7 @@ struct dw_edma_chip {
>  	int			id;
>  	int			nr_irqs;
>  	const struct dw_edma_core_ops   *ops;
> +	enum dw_edma_chip_flags	flags;

There is no point in having the named enumeration here since the flags
field semantics is actually a bitfield rather than a single value. If
you want to stick to the enumerated flags, then please use the
anonymous enum like this:
+enum {
+	DW_EDMA_CHIP_LOCAL	= BIT(0),
+};
and explicit unsigned int type of the flags field.

-Sergey

>  
>  	void __iomem		*reg_base;
>  
> -- 
> 2.35.1
>
Manivannan Sadhasivam April 13, 2022, 9:28 a.m. UTC | #2
On Wed, Apr 13, 2022 at 12:18:37PM +0300, Serge Semin wrote:
> On Wed, Apr 06, 2022 at 10:23:45AM -0500, Frank Li wrote:
> > Add a "flags" field to the "struct dw_edma_chip" so that the controller
> > drivers can pass flags that are relevant to the platform.
> > 
> > DW_EDMA_CHIP_LOCAL - Used by the controller drivers accessing eDMA
> > locally. Local eDMA access doesn't require generating MSIs to the remote.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> 
> > Change from v5 to v6
> >  - use enum instead of define
> 
> Hm, why have you decided to do that? I don't see a well justified
> reason to use the enumeration here, but see my next comment for
> details.

It was me who suggested using the enums for flags instead of defines.
Enums helps with kdoc and it also provides a neat way to group flags together.

> 
> > 
> > Change from v4 to v5
> >  - split two two patch
> >  - rework commit message
> > 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 | 9 ++++++---
> >  include/linux/dma/edma.h              | 9 +++++++++
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > index 8ddc537d11fd6..30f8bfe6e5712 100644
> > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c

[...]

> > +	enum dw_edma_chip_flags	flags;
> 
> There is no point in having the named enumeration here since the flags
> field semantics is actually a bitfield rather than a single value. If
> you want to stick to the enumerated flags, then please use the
> anonymous enum like this:

I agree with using u32 for flags field but I don't agree with anonymous enums.
Enums with a name conveys information of what the enumerated types represent.
If you just look at your example below, it is difficult to guess the purpose of
this enum.

Thanks,
Mani

> +enum {
> +	DW_EDMA_CHIP_LOCAL	= BIT(0),
> +};
> and explicit unsigned int type of the flags field.
> 
> -Sergey
> 
> >  
> >  	void __iomem		*reg_base;
> >  
> > -- 
> > 2.35.1
> >
Serge Semin April 13, 2022, 11:53 a.m. UTC | #3
On Wed, Apr 13, 2022 at 02:58:08PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Apr 13, 2022 at 12:18:37PM +0300, Serge Semin wrote:
> > On Wed, Apr 06, 2022 at 10:23:45AM -0500, Frank Li wrote:
> > > Add a "flags" field to the "struct dw_edma_chip" so that the controller
> > > drivers can pass flags that are relevant to the platform.
> > > 
> > > DW_EDMA_CHIP_LOCAL - Used by the controller drivers accessing eDMA
> > > locally. Local eDMA access doesn't require generating MSIs to the remote.
> > > 
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > 
> > > Change from v5 to v6
> > >  - use enum instead of define
> > 
> > Hm, why have you decided to do that? I don't see a well justified
> > reason to use the enumeration here, but see my next comment for
> > details.
> 
> It was me who suggested using the enums for flags instead of defines.
> Enums helps with kdoc and it also provides a neat way to group flags together.
> 
> > 
> > > 
> > > Change from v4 to v5
> > >  - split two two patch
> > >  - rework commit message
> > > 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 | 9 ++++++---
> > >  include/linux/dma/edma.h              | 9 +++++++++
> > >  2 files changed, 15 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > > index 8ddc537d11fd6..30f8bfe6e5712 100644
> > > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> > > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> 
> [...]
> 

> > > +	enum dw_edma_chip_flags	flags;
> > 
> > There is no point in having the named enumeration here since the flags
> > field semantics is actually a bitfield rather than a single value. If
> > you want to stick to the enumerated flags, then please use the
> > anonymous enum like this:
> 
> I agree with using u32 for flags field but I don't agree with anonymous enums.
> Enums with a name conveys information of what the enumerated types represent.
> If you just look at your example below, it is difficult to guess the purpose of
> this enum.

I see your point. Ok, no anonymization then.) @Frank could you please update
the field type to unsigned int or u32 then? Personally I prefer having
"unsigned int" here, since that's the type used by the compiler if no
negative values is enumerated. Though u32 would be ok too.

-Serget

> 
> Thanks,
> Mani
> 
> > +enum {
> > +	DW_EDMA_CHIP_LOCAL	= BIT(0),
> > +};
> > and explicit unsigned int type of the flags field.
> > 
> > -Sergey
> > 
> > >  
> > >  	void __iomem		*reg_base;
> > >  
> > > -- 
> > > 2.35.1
> > >
diff mbox series

Patch

diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
index 8ddc537d11fd6..30f8bfe6e5712 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 */
diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
index c2039246fc08c..bec444e2939b2 100644
--- a/include/linux/dma/edma.h
+++ b/include/linux/dma/edma.h
@@ -33,6 +33,14 @@  enum dw_edma_map_format {
 	EDMA_MF_HDMA_COMPAT = 0x5
 };
 
+/**
+ * enum dw_edma_chip_flags - Flags specific to an eDMA chip
+ * @DW_EDMA_CHIP_LOCAL:		eDMA is used locally by an endpoint
+ */
+enum dw_edma_chip_flags {
+	DW_EDMA_CHIP_LOCAL	= BIT(0),
+};
+
 /**
  * struct dw_edma_chip - representation of DesignWare eDMA controller hardware
  * @dev:		 struct device of the eDMA controller
@@ -53,6 +61,7 @@  struct dw_edma_chip {
 	int			id;
 	int			nr_irqs;
 	const struct dw_edma_core_ops   *ops;
+	enum dw_edma_chip_flags	flags;
 
 	void __iomem		*reg_base;