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@xxxxxxx>
> ---
> 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
>
