| Message ID | 20220303184635.2603-3-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/5] dmaengine: dw-edma: fix dw_edma_probe() can't be call globally | expand |
On Thu, Mar 03, 2022 at 12:46:33PM -0600, Frank Li wrote: > Allow user don't enable remote MSI irq. > PCI ep probe dma locally, don't want to raise irq > to remote PCI host. > > Add option allow force 32bit register access even at > 64bit system. i.MX8 hardware only allowed 32bit register > access. > > Add option allow EP side probe dma. remote side dma is > continue physical memory, local memory is scatter list. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > Change from v1 to v2 > - none > > drivers/dma/dw-edma/dw-edma-core.c | 7 ++++++- > drivers/dma/dw-edma/dw-edma-v0-core.c | 14 ++++++++++---- > include/linux/dma/edma.h | 7 +++++++ > 3 files changed, 23 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c > index 0cb66434f9e14..a43bb26c8bf96 100644 > --- a/drivers/dma/dw-edma/dw-edma-core.c > +++ b/drivers/dma/dw-edma/dw-edma-core.c > @@ -336,6 +336,7 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer) > struct dw_edma_desc *desc; > u32 cnt = 0; > int i; > + bool b; > > if (!chan->configured) > return NULL; > @@ -424,7 +425,11 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer) > chunk->ll_region.sz += burst->sz; > desc->alloc_sz += burst->sz; > > - if (chan->dir == EDMA_DIR_WRITE) { > + b = (chan->dir == EDMA_DIR_WRITE); > + if (chan->chip->flags & DW_EDMA_CHIP_LOCAL_EP) > + b = !b; > + > + if (b) { I've added a patch that uses the xfer direction and channel direction to find out whether it is a read operation or not. Please take a look: https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/commit/?h=tracking-qcomlt-sdx55-drivers&id=d6a3f432204614ad8531321949a8a9e2c3e94c3b The patch could also make use of the "DW_EDMA_CHIP_LOCAL" flag if you prefer. > burst->sar = src_addr; > if (xfer->type == EDMA_XFER_CYCLIC) { > burst->dar = xfer->xfer.cyclic.paddr; > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > index 6e2f83e31a03a..d5c2415e2c616 100644 > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c > @@ -307,13 +307,18 @@ u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_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; > + u32 rie = 0; > int j; > > lli = chunk->ll_region.vaddr; > > + if (!(chan->chip->flags & DW_EDMA_CHIP_NO_MSI)) > + rie = DW_EDMA_V0_RIE; > + > if (chunk->cb) > control = DW_EDMA_V0_CB; > > @@ -321,7 +326,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) > list_for_each_entry(child, &chunk->burst->list, list) { > j--; > if (!j) > - control |= (DW_EDMA_V0_LIE | DW_EDMA_V0_RIE); > + control |= (DW_EDMA_V0_LIE | rie); I think the MSI makes sense only for eDMA access from remote. So how about reusing the same flag you used above? control |= DW_EDMA_V0_LIE; /* Raise MSI only if the eDMA is accessed by remote */ if (!(chan->chip->flags & DW_EDMA_CHIP_LOCAL)) control |= DW_EDMA_V0_RIE; > > /* Channel control */ > SET_LL_32(&lli[i].control, control); > @@ -420,15 +425,16 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first) > SET_CH_32(chip, chan->dir, chan->id, ch_control1, > (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE)); > /* Linked list */ > - #ifdef CONFIG_64BIT > + if (!(chan->chip->flags & DW_EDMA_CHIP_REG32BIT) && > + IS_ENABLED(CONFIG_64BIT)) { > SET_CH_64(chip, chan->dir, chan->id, llp.reg, > chunk->ll_region.paddr); Why you are doing 32bit access here only and not in other places? Thanks, Mani > - #else /* CONFIG_64BIT */ > + } else { > SET_CH_32(chip, chan->dir, chan->id, llp.lsb, > lower_32_bits(chunk->ll_region.paddr)); > SET_CH_32(chip, chan->dir, chan->id, llp.msb, > upper_32_bits(chunk->ll_region.paddr)); > - #endif /* CONFIG_64BIT */ > + } > } > /* Doorbell */ > SET_RW_32(chip, chan->dir, doorbell, > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h > index fcfbc0f47f83d..e74ee15d9832a 100644 > --- a/include/linux/dma/edma.h > +++ b/include/linux/dma/edma.h > @@ -33,6 +33,10 @@ enum dw_edma_map_format { > EDMA_MF_HDMA_COMPAT = 0x5 > }; > > +#define DW_EDMA_CHIP_NO_MSI BIT(0) > +#define DW_EDMA_CHIP_REG32BIT BIT(1) > +#define DW_EDMA_CHIP_LOCAL_EP BIT(2) As per my understanding the > + > /** > * struct dw_edma_chip - representation of DesignWare eDMA controller hardware > * @dev: struct device of the eDMA controller > @@ -40,6 +44,8 @@ enum dw_edma_map_format { > * @nr_irqs: total dma irq number > * reg64bit if support 64bit write to register > * @ops DMA channel to IRQ number mapping > + * @flags - DW_EDMA_CHIP_NO_MSI can't generate remote MSI irq > + * - DW_EDMA_CHIP_REG32BIT only support 32bit register write > * @wr_ch_cnt DMA write channel number > * @rd_ch_cnt DMA read channel number > * @rg_region DMA register region > @@ -53,6 +59,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 >
i On Fri, Mar 4, 2022 at 10:19 AM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > On Thu, Mar 03, 2022 at 12:46:33PM -0600, Frank Li wrote: > > Allow user don't enable remote MSI irq. > > PCI ep probe dma locally, don't want to raise irq > > to remote PCI host. > > > > Add option allow force 32bit register access even at > > 64bit system. i.MX8 hardware only allowed 32bit register > > access. > > > > Add option allow EP side probe dma. remote side dma is > > continue physical memory, local memory is scatter list. > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > Change from v1 to v2 > > - none > > > > drivers/dma/dw-edma/dw-edma-core.c | 7 ++++++- > > drivers/dma/dw-edma/dw-edma-v0-core.c | 14 ++++++++++---- > > include/linux/dma/edma.h | 7 +++++++ > > 3 files changed, 23 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c > > index 0cb66434f9e14..a43bb26c8bf96 100644 > > --- a/drivers/dma/dw-edma/dw-edma-core.c > > +++ b/drivers/dma/dw-edma/dw-edma-core.c > > @@ -336,6 +336,7 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer) > > struct dw_edma_desc *desc; > > u32 cnt = 0; > > int i; > > + bool b; > > > > if (!chan->configured) > > return NULL; > > @@ -424,7 +425,11 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer) > > chunk->ll_region.sz += burst->sz; > > desc->alloc_sz += burst->sz; > > > > - if (chan->dir == EDMA_DIR_WRITE) { > > + b = (chan->dir == EDMA_DIR_WRITE); > > + if (chan->chip->flags & DW_EDMA_CHIP_LOCAL_EP) > > + b = !b; > > + > > + if (b) { > > I've added a patch that uses the xfer direction and channel direction to > find out whether it is a read operation or not. Please take a look: > > https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/commit/?h=tracking-qcomlt-sdx55-drivers&id=d6a3f432204614ad8531321949a8a9e2c3e94c3b > I think your patch is better. Can I include your patch into this sequence ? > The patch could also make use of the "DW_EDMA_CHIP_LOCAL" flag if you > prefer. > > > burst->sar = src_addr; > > if (xfer->type == EDMA_XFER_CYCLIC) { > > burst->dar = xfer->xfer.cyclic.paddr; > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > > index 6e2f83e31a03a..d5c2415e2c616 100644 > > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c > > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c > > @@ -307,13 +307,18 @@ u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_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; > > + u32 rie = 0; > > int j; > > > > lli = chunk->ll_region.vaddr; > > > > + if (!(chan->chip->flags & DW_EDMA_CHIP_NO_MSI)) > > + rie = DW_EDMA_V0_RIE; > > + > > if (chunk->cb) > > control = DW_EDMA_V0_CB; > > > > @@ -321,7 +326,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) > > list_for_each_entry(child, &chunk->burst->list, list) { > > j--; > > if (!j) > > - control |= (DW_EDMA_V0_LIE | DW_EDMA_V0_RIE); > > + control |= (DW_EDMA_V0_LIE | rie); > > I think the MSI makes sense only for eDMA access from remote. So how > about reusing the same flag you used above? Understand, DW_EDMA_CHIP_NO_MSI is more direct. User can know what exactly control. DW_EDMA_CHIP_LOCAL is quite general. User don't know what to do from naming. I am okay for both naming. If I pick up your patch for dma dir, Only below two flags need, #define DW_EDMA_CHIP_NO_MSI BIT(0) #define DW_EDMA_CHIP_REG32BIT BIT(1) vs #define DW_EDMA_CHIP_LOCAL BIT(0) #define DW_EDMA_CHIP_REG32BIT BIT(1) which one is better? > > control |= DW_EDMA_V0_LIE; > > /* Raise MSI only if the eDMA is accessed by remote */ > if (!(chan->chip->flags & DW_EDMA_CHIP_LOCAL)) > control |= DW_EDMA_V0_RIE; > > > > > /* Channel control */ > > SET_LL_32(&lli[i].control, control); > > @@ -420,15 +425,16 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first) > > SET_CH_32(chip, chan->dir, chan->id, ch_control1, > > (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE)); > > /* Linked list */ > > - #ifdef CONFIG_64BIT > > + if (!(chan->chip->flags & DW_EDMA_CHIP_REG32BIT) && > > + IS_ENABLED(CONFIG_64BIT)) { > > SET_CH_64(chip, chan->dir, chan->id, llp.reg, > > chunk->ll_region.paddr); > > Why you are doing 32bit access here only and not in other places? > Only here access DBI register, other place is access dma link list, which is memory. > Thanks, > Mani > > > - #else /* CONFIG_64BIT */ > > + } else { > > SET_CH_32(chip, chan->dir, chan->id, llp.lsb, > > lower_32_bits(chunk->ll_region.paddr)); > > SET_CH_32(chip, chan->dir, chan->id, llp.msb, > > upper_32_bits(chunk->ll_region.paddr)); > > - #endif /* CONFIG_64BIT */ > > + } > > } > > /* Doorbell */ > > SET_RW_32(chip, chan->dir, doorbell, > > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h > > index fcfbc0f47f83d..e74ee15d9832a 100644 > > --- a/include/linux/dma/edma.h > > +++ b/include/linux/dma/edma.h > > @@ -33,6 +33,10 @@ enum dw_edma_map_format { > > EDMA_MF_HDMA_COMPAT = 0x5 > > }; > > > > +#define DW_EDMA_CHIP_NO_MSI BIT(0) > > +#define DW_EDMA_CHIP_REG32BIT BIT(1) > > +#define DW_EDMA_CHIP_LOCAL_EP BIT(2) > > As per my understanding the what's you mean? > > > + > > /** > > * struct dw_edma_chip - representation of DesignWare eDMA controller hardware > > * @dev: struct device of the eDMA controller > > @@ -40,6 +44,8 @@ enum dw_edma_map_format { > > * @nr_irqs: total dma irq number > > * reg64bit if support 64bit write to register > > * @ops DMA channel to IRQ number mapping > > + * @flags - DW_EDMA_CHIP_NO_MSI can't generate remote MSI irq > > + * - DW_EDMA_CHIP_REG32BIT only support 32bit register write > > * @wr_ch_cnt DMA write channel number > > * @rd_ch_cnt DMA read channel number > > * @rg_region DMA register region > > @@ -53,6 +59,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 > >
On Thu, Mar 03, 2022 at 12:46:33PM -0600, Frank Li wrote: > Allow user don't enable remote MSI irq. > PCI ep probe dma locally, don't want to raise irq > to remote PCI host. > > Add option allow force 32bit register access even at > 64bit system. i.MX8 hardware only allowed 32bit register > access. > > Add option allow EP side probe dma. remote side dma is > continue physical memory, local memory is scatter list. s/Allow user don't enable remote MSI/Allow user to prevent use of remote MSI/ ? s/irq/IRQ/ (several, and in comment below) s/ep/EP/ (capitalize consistently) s/dma/DMA/ (several) s/Add option allow force 32bit/Add option to force 32-bit/ s/even at 64bit system/even on 64-bit systems/ s/remote side/Remote side/ (start sentence with capital letter) s/Add option allow EP side probe dma/Add option to allow EP to probe DMA/ (I'm not quite sure what this means). s/continue physical memory/contiguous physical memory/ ? Rewrap above to fill 75 columns. > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > Change from v1 to v2 > - none > > drivers/dma/dw-edma/dw-edma-core.c | 7 ++++++- > drivers/dma/dw-edma/dw-edma-v0-core.c | 14 ++++++++++---- > include/linux/dma/edma.h | 7 +++++++ > 3 files changed, 23 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c > index 0cb66434f9e14..a43bb26c8bf96 100644 > --- a/drivers/dma/dw-edma/dw-edma-core.c > +++ b/drivers/dma/dw-edma/dw-edma-core.c > @@ -336,6 +336,7 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer) > struct dw_edma_desc *desc; > u32 cnt = 0; > int i; > + bool b; > > if (!chan->configured) > return NULL; > @@ -424,7 +425,11 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer) > chunk->ll_region.sz += burst->sz; > desc->alloc_sz += burst->sz; > > - if (chan->dir == EDMA_DIR_WRITE) { > + b = (chan->dir == EDMA_DIR_WRITE); > + if (chan->chip->flags & DW_EDMA_CHIP_LOCAL_EP) > + b = !b; > + > + if (b) { > burst->sar = src_addr; > if (xfer->type == EDMA_XFER_CYCLIC) { > burst->dar = xfer->xfer.cyclic.paddr; > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > index 6e2f83e31a03a..d5c2415e2c616 100644 > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c > @@ -307,13 +307,18 @@ u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_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; > + u32 rie = 0; > int j; > > lli = chunk->ll_region.vaddr; > > + if (!(chan->chip->flags & DW_EDMA_CHIP_NO_MSI)) > + rie = DW_EDMA_V0_RIE; > + > if (chunk->cb) > control = DW_EDMA_V0_CB; > > @@ -321,7 +326,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) > list_for_each_entry(child, &chunk->burst->list, list) { > j--; > if (!j) > - control |= (DW_EDMA_V0_LIE | DW_EDMA_V0_RIE); > + control |= (DW_EDMA_V0_LIE | rie); Could drop "rie" and do this, which would keep DW_EDMA_CHIP_NO_MSI closer to where it's used: if (!j) { control |= DW_EDMA_V0_LIE; if (!(chan->chip->flags & DW_EDMA_CHIP_NO_MSI)) control |= DW_EDMA_V0_RIE; } > /* Channel control */ > SET_LL_32(&lli[i].control, control); > @@ -420,15 +425,16 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first) > SET_CH_32(chip, chan->dir, chan->id, ch_control1, > (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE)); > /* Linked list */ > - #ifdef CONFIG_64BIT > + if (!(chan->chip->flags & DW_EDMA_CHIP_REG32BIT) && > + IS_ENABLED(CONFIG_64BIT)) { > SET_CH_64(chip, chan->dir, chan->id, llp.reg, > chunk->ll_region.paddr); > - #else /* CONFIG_64BIT */ > + } else { > SET_CH_32(chip, chan->dir, chan->id, llp.lsb, > lower_32_bits(chunk->ll_region.paddr)); > SET_CH_32(chip, chan->dir, chan->id, llp.msb, > upper_32_bits(chunk->ll_region.paddr)); > - #endif /* CONFIG_64BIT */ I think this would read better if you reversed the sense: if ((chan->chip->flags & DW_EDMA_CHIP_REG32BIT) || !IS_ENABLED(CONFIG_64BIT)) { SET_CH_32(...); SET_CH_32(...); } else { SET_CH_64(...); } > + } > } > /* Doorbell */ > SET_RW_32(chip, chan->dir, doorbell, > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h > index fcfbc0f47f83d..e74ee15d9832a 100644 > --- a/include/linux/dma/edma.h > +++ b/include/linux/dma/edma.h > @@ -33,6 +33,10 @@ enum dw_edma_map_format { > EDMA_MF_HDMA_COMPAT = 0x5 > }; > > +#define DW_EDMA_CHIP_NO_MSI BIT(0) > +#define DW_EDMA_CHIP_REG32BIT BIT(1) > +#define DW_EDMA_CHIP_LOCAL_EP BIT(2) > + > /** > * struct dw_edma_chip - representation of DesignWare eDMA controller hardware > * @dev: struct device of the eDMA controller > @@ -40,6 +44,8 @@ enum dw_edma_map_format { > * @nr_irqs: total dma irq number > * reg64bit if support 64bit write to register > * @ops DMA channel to IRQ number mapping > + * @flags - DW_EDMA_CHIP_NO_MSI can't generate remote MSI irq > + * - DW_EDMA_CHIP_REG32BIT only support 32bit register write Looks like DW_EDMA_CHIP_LOCAL_EP should be documented here, too? > * @wr_ch_cnt DMA write channel number > * @rd_ch_cnt DMA read channel number > * @rg_region DMA register region > @@ -53,6 +59,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 >
On Fri, Mar 04, 2022 at 11:16:11AM -0600, Zhi Li wrote: > i > > On Fri, Mar 4, 2022 at 10:19 AM Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > > > On Thu, Mar 03, 2022 at 12:46:33PM -0600, Frank Li wrote: > > > Allow user don't enable remote MSI irq. > > > PCI ep probe dma locally, don't want to raise irq > > > to remote PCI host. > > > > > > Add option allow force 32bit register access even at > > > 64bit system. i.MX8 hardware only allowed 32bit register > > > access. > > > > > > Add option allow EP side probe dma. remote side dma is > > > continue physical memory, local memory is scatter list. > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > --- > > > Change from v1 to v2 > > > - none > > > > > > drivers/dma/dw-edma/dw-edma-core.c | 7 ++++++- > > > drivers/dma/dw-edma/dw-edma-v0-core.c | 14 ++++++++++---- > > > include/linux/dma/edma.h | 7 +++++++ > > > 3 files changed, 23 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c > > > index 0cb66434f9e14..a43bb26c8bf96 100644 > > > --- a/drivers/dma/dw-edma/dw-edma-core.c > > > +++ b/drivers/dma/dw-edma/dw-edma-core.c > > > @@ -336,6 +336,7 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer) > > > struct dw_edma_desc *desc; > > > u32 cnt = 0; > > > int i; > > > + bool b; > > > > > > if (!chan->configured) > > > return NULL; > > > @@ -424,7 +425,11 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer) > > > chunk->ll_region.sz += burst->sz; > > > desc->alloc_sz += burst->sz; > > > > > > - if (chan->dir == EDMA_DIR_WRITE) { > > > + b = (chan->dir == EDMA_DIR_WRITE); > > > + if (chan->chip->flags & DW_EDMA_CHIP_LOCAL_EP) > > > + b = !b; > > > + > > > + if (b) { > > > > I've added a patch that uses the xfer direction and channel direction to > > find out whether it is a read operation or not. Please take a look: > > > > https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/commit/?h=tracking-qcomlt-sdx55-drivers&id=d6a3f432204614ad8531321949a8a9e2c3e94c3b > > > > I think your patch is better. Can I include your patch into this sequence ? > Yes, feel free to. Also, there is one more cleanup patch I've added: https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/commit/?h=tracking-qcomlt-sdx55-drivers&id=85eb58bd078000dd938487f560cee5d259f06577 > > The patch could also make use of the "DW_EDMA_CHIP_LOCAL" flag if you > > prefer. > > > > > burst->sar = src_addr; > > > if (xfer->type == EDMA_XFER_CYCLIC) { > > > burst->dar = xfer->xfer.cyclic.paddr; > > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > > > index 6e2f83e31a03a..d5c2415e2c616 100644 > > > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c > > > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c > > > @@ -307,13 +307,18 @@ u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_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; > > > + u32 rie = 0; > > > int j; > > > > > > lli = chunk->ll_region.vaddr; > > > > > > + if (!(chan->chip->flags & DW_EDMA_CHIP_NO_MSI)) > > > + rie = DW_EDMA_V0_RIE; > > > + > > > if (chunk->cb) > > > control = DW_EDMA_V0_CB; > > > > > > @@ -321,7 +326,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) > > > list_for_each_entry(child, &chunk->burst->list, list) { > > > j--; > > > if (!j) > > > - control |= (DW_EDMA_V0_LIE | DW_EDMA_V0_RIE); > > > + control |= (DW_EDMA_V0_LIE | rie); > > > > I think the MSI makes sense only for eDMA access from remote. So how > > about reusing the same flag you used above? > > Understand, DW_EDMA_CHIP_NO_MSI is more direct. User can know what > exactly control. > Right but isn't using "DW_EDMA_CHIP_LOCAL" implies NO_MSI? Or is there any endpoint implementation that makes use of MSIs also? > DW_EDMA_CHIP_LOCAL is quite general. User don't know what to do from naming. > > I am okay for both naming. > > If I pick up your patch for dma dir, Only below two flags need, > #define DW_EDMA_CHIP_NO_MSI BIT(0) > #define DW_EDMA_CHIP_REG32BIT BIT(1) > > vs > #define DW_EDMA_CHIP_LOCAL BIT(0) > #define DW_EDMA_CHIP_REG32BIT BIT(1) How about "DW_EDMA_CHIP_32BIT_DBI"? Also add comments for both flags. > > which one is better? > > > > > control |= DW_EDMA_V0_LIE; > > > > /* Raise MSI only if the eDMA is accessed by remote */ > > if (!(chan->chip->flags & DW_EDMA_CHIP_LOCAL)) > > control |= DW_EDMA_V0_RIE; > > > > > > > > /* Channel control */ > > > SET_LL_32(&lli[i].control, control); > > > @@ -420,15 +425,16 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first) > > > SET_CH_32(chip, chan->dir, chan->id, ch_control1, > > > (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE)); > > > /* Linked list */ > > > - #ifdef CONFIG_64BIT > > > + if (!(chan->chip->flags & DW_EDMA_CHIP_REG32BIT) && > > > + IS_ENABLED(CONFIG_64BIT)) { > > > SET_CH_64(chip, chan->dir, chan->id, llp.reg, > > > chunk->ll_region.paddr); > > > > Why you are doing 32bit access here only and not in other places? > > > > Only here access DBI register, other place is access dma link list, > which is memory. > Okay. Then you need to specify it clearly in commit message as well. Thanks, Mani > > Thanks, > > Mani > > > > > - #else /* CONFIG_64BIT */ > > > + } else { > > > SET_CH_32(chip, chan->dir, chan->id, llp.lsb, > > > lower_32_bits(chunk->ll_region.paddr)); > > > SET_CH_32(chip, chan->dir, chan->id, llp.msb, > > > upper_32_bits(chunk->ll_region.paddr)); > > > - #endif /* CONFIG_64BIT */ > > > + } > > > } > > > /* Doorbell */ > > > SET_RW_32(chip, chan->dir, doorbell, > > > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h > > > index fcfbc0f47f83d..e74ee15d9832a 100644 > > > --- a/include/linux/dma/edma.h > > > +++ b/include/linux/dma/edma.h > > > @@ -33,6 +33,10 @@ enum dw_edma_map_format { > > > EDMA_MF_HDMA_COMPAT = 0x5 > > > }; > > > > > > +#define DW_EDMA_CHIP_NO_MSI BIT(0) > > > +#define DW_EDMA_CHIP_REG32BIT BIT(1) > > > +#define DW_EDMA_CHIP_LOCAL_EP BIT(2) > > > > As per my understanding the > > what's you mean? > > > > > > + > > > /** > > > * struct dw_edma_chip - representation of DesignWare eDMA controller hardware > > > * @dev: struct device of the eDMA controller > > > @@ -40,6 +44,8 @@ enum dw_edma_map_format { > > > * @nr_irqs: total dma irq number > > > * reg64bit if support 64bit write to register > > > * @ops DMA channel to IRQ number mapping > > > + * @flags - DW_EDMA_CHIP_NO_MSI can't generate remote MSI irq > > > + * - DW_EDMA_CHIP_REG32BIT only support 32bit register write > > > * @wr_ch_cnt DMA write channel number > > > * @rd_ch_cnt DMA read channel number > > > * @rg_region DMA register region > > > @@ -53,6 +59,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 > > >
diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c index 0cb66434f9e14..a43bb26c8bf96 100644 --- a/drivers/dma/dw-edma/dw-edma-core.c +++ b/drivers/dma/dw-edma/dw-edma-core.c @@ -336,6 +336,7 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer) struct dw_edma_desc *desc; u32 cnt = 0; int i; + bool b; if (!chan->configured) return NULL; @@ -424,7 +425,11 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer) chunk->ll_region.sz += burst->sz; desc->alloc_sz += burst->sz; - if (chan->dir == EDMA_DIR_WRITE) { + b = (chan->dir == EDMA_DIR_WRITE); + if (chan->chip->flags & DW_EDMA_CHIP_LOCAL_EP) + b = !b; + + if (b) { burst->sar = src_addr; if (xfer->type == EDMA_XFER_CYCLIC) { burst->dar = xfer->xfer.cyclic.paddr; diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c index 6e2f83e31a03a..d5c2415e2c616 100644 --- a/drivers/dma/dw-edma/dw-edma-v0-core.c +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c @@ -307,13 +307,18 @@ u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_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; + u32 rie = 0; int j; lli = chunk->ll_region.vaddr; + if (!(chan->chip->flags & DW_EDMA_CHIP_NO_MSI)) + rie = DW_EDMA_V0_RIE; + if (chunk->cb) control = DW_EDMA_V0_CB; @@ -321,7 +326,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) list_for_each_entry(child, &chunk->burst->list, list) { j--; if (!j) - control |= (DW_EDMA_V0_LIE | DW_EDMA_V0_RIE); + control |= (DW_EDMA_V0_LIE | rie); /* Channel control */ SET_LL_32(&lli[i].control, control); @@ -420,15 +425,16 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first) SET_CH_32(chip, chan->dir, chan->id, ch_control1, (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE)); /* Linked list */ - #ifdef CONFIG_64BIT + if (!(chan->chip->flags & DW_EDMA_CHIP_REG32BIT) && + IS_ENABLED(CONFIG_64BIT)) { SET_CH_64(chip, chan->dir, chan->id, llp.reg, chunk->ll_region.paddr); - #else /* CONFIG_64BIT */ + } else { SET_CH_32(chip, chan->dir, chan->id, llp.lsb, lower_32_bits(chunk->ll_region.paddr)); SET_CH_32(chip, chan->dir, chan->id, llp.msb, upper_32_bits(chunk->ll_region.paddr)); - #endif /* CONFIG_64BIT */ + } } /* Doorbell */ SET_RW_32(chip, chan->dir, doorbell, diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h index fcfbc0f47f83d..e74ee15d9832a 100644 --- a/include/linux/dma/edma.h +++ b/include/linux/dma/edma.h @@ -33,6 +33,10 @@ enum dw_edma_map_format { EDMA_MF_HDMA_COMPAT = 0x5 }; +#define DW_EDMA_CHIP_NO_MSI BIT(0) +#define DW_EDMA_CHIP_REG32BIT BIT(1) +#define DW_EDMA_CHIP_LOCAL_EP BIT(2) + /** * struct dw_edma_chip - representation of DesignWare eDMA controller hardware * @dev: struct device of the eDMA controller @@ -40,6 +44,8 @@ enum dw_edma_map_format { * @nr_irqs: total dma irq number * reg64bit if support 64bit write to register * @ops DMA channel to IRQ number mapping + * @flags - DW_EDMA_CHIP_NO_MSI can't generate remote MSI irq + * - DW_EDMA_CHIP_REG32BIT only support 32bit register write * @wr_ch_cnt DMA write channel number * @rd_ch_cnt DMA read channel number * @rg_region DMA register region @@ -53,6 +59,7 @@ struct dw_edma_chip { int id; int nr_irqs; const struct dw_edma_core_ops *ops; + u32 flags; void __iomem *reg_base;
Allow user don't enable remote MSI irq. PCI ep probe dma locally, don't want to raise irq to remote PCI host. Add option allow force 32bit register access even at 64bit system. i.MX8 hardware only allowed 32bit register access. Add option allow EP side probe dma. remote side dma is continue physical memory, local memory is scatter list. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- Change from v1 to v2 - none drivers/dma/dw-edma/dw-edma-core.c | 7 ++++++- drivers/dma/dw-edma/dw-edma-v0-core.c | 14 ++++++++++---- include/linux/dma/edma.h | 7 +++++++ 3 files changed, 23 insertions(+), 5 deletions(-)