| Message ID | 20200914140956.221432-1-eugen.hristev@microchip.com |
|---|---|
| Headers | show |
| Series | dmaengine: add support for sama7g5 based at_xdmac | expand |
Hi, Eugen, On 9/14/20 5:09 PM, Eugen Hristev wrote: > Separate register defines into header file. > This is required to support a slightly different version of the register > map in new hardware versions of the XDMAC. > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> > --- > drivers/dma/at_xdmac.c | 143 +-------------------------------- > drivers/dma/at_xdmac_regs.h | 154 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 155 insertions(+), 142 deletions(-) > create mode 100644 drivers/dma/at_xdmac_regs.h Even with the sama7g5 support there is a single .c file that includes the .h. I wouldn't split the registers definitions in a dedicated file. Cheers, ta
On 9/14/20 5:09 PM, Eugen Hristev wrote: > Add new header file for the at_xdmac regs definition to the proper > MAINTAINERS entry. > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index b5cfab015bd6..312ba6ae5fc7 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11361,6 +11361,7 @@ F: Documentation/devicetree/bindings/dma/atmel-dma.txt > F: drivers/dma/at_hdmac.c > F: drivers/dma/at_hdmac_regs.h > F: drivers/dma/at_xdmac.c > +F: drivers/dma/at_xdmac_regs.h A dedicated entry for at_xdmac_regs.h will not be needed, but still we're here, let's shrink these lines to only one. A change like the one from below is welcomed: +F: drivers/dma/at_* ta > F: include/dt-bindings/dma/at91.h > F: include/linux/platform_data/dma-atmel.h > >
On 9/14/20 5:09 PM, Eugen Hristev wrote: > The PERID in the CC register for mem2mem operations must match an unused > PERID. > The PERID field is 7 bits, but the selected value is 0x3f. > On later products we can have more reserved PERIDs for actual peripherals, > thus this needs to be increased to maximum size. > Changing the value to 0x7f, which is the maximum for 7 bits field. > Maybe it is worth to explain that for memory-to-memory transfers, PERID should be set to an unused peripheral ID, and the maximum value seems the safest. Anyway with or without this addressed, one can add: Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> > --- > drivers/dma/at_xdmac.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c > index fab19e00a7be..81bb90206092 100644 > --- a/drivers/dma/at_xdmac.c > +++ b/drivers/dma/at_xdmac.c > @@ -726,7 +726,7 @@ at_xdmac_interleaved_queue_desc(struct dma_chan *chan, > * match the one of another channel. If not, it could lead to spurious > * flag status. > */ > - u32 chan_cc = AT_XDMAC_CC_PERID(0x3f) > + u32 chan_cc = AT_XDMAC_CC_PERID(0x7f) > | AT_XDMAC_CC_DIF(0) > | AT_XDMAC_CC_SIF(0) > | AT_XDMAC_CC_MBSIZE_SIXTEEN > @@ -908,7 +908,7 @@ at_xdmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, > * match the one of another channel. If not, it could lead to spurious > * flag status. > */ > - u32 chan_cc = AT_XDMAC_CC_PERID(0x3f) > + u32 chan_cc = AT_XDMAC_CC_PERID(0x7f) > | AT_XDMAC_CC_DAM_INCREMENTED_AM > | AT_XDMAC_CC_SAM_INCREMENTED_AM > | AT_XDMAC_CC_DIF(0) > @@ -1014,7 +1014,7 @@ static struct at_xdmac_desc *at_xdmac_memset_create_desc(struct dma_chan *chan, > * match the one of another channel. If not, it could lead to spurious > * flag status. > */ > - u32 chan_cc = AT_XDMAC_CC_PERID(0x3f) > + u32 chan_cc = AT_XDMAC_CC_PERID(0x7f) > | AT_XDMAC_CC_DAM_UBS_AM > | AT_XDMAC_CC_SAM_INCREMENTED_AM > | AT_XDMAC_CC_DIF(0) >
On 9/23/20 8:30 AM, Tudor Ambarus - M18064 wrote: > On 9/14/20 5:09 PM, Eugen Hristev wrote: >> The PERID in the CC register for mem2mem operations must match an unused >> PERID. >> The PERID field is 7 bits, but the selected value is 0x3f. >> On later products we can have more reserved PERIDs for actual peripherals, >> thus this needs to be increased to maximum size. >> Changing the value to 0x7f, which is the maximum for 7 bits field. >> > > Maybe it is worth to explain that for memory-to-memory transfers, PERID > should be set to an unused peripheral ID, and the maximum value seems the > safest. Anyway with or without this addressed, one can add: > :) I somehow misread your commit message, you already described that, it's fine. > Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> >> --- >> drivers/dma/at_xdmac.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >>
On 9/14/20 5:09 PM, Eugen Hristev wrote: > SAMA7G5 SoC uses a slightly different variant of the AT_XDMAC. > Added support by a new compatible and a layout struct that copes > to the specific version considering the compatible string. > Only the differences in register map are present in the layout struct. > I reworked the register access for this part that has the differences. > Also the Source/Destination Interface bits are no longer valid for this > variant of the XDMAC. Thus, the layout also has a bool for specifying > whether these bits are required or not. > > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> > --- > drivers/dma/at_xdmac.c | 99 ++++++++++++++++++++++++++++++------- > drivers/dma/at_xdmac_regs.h | 9 ---- > 2 files changed, 82 insertions(+), 26 deletions(-) > > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c > index 81bb90206092..874484a4e79f 100644 > --- a/drivers/dma/at_xdmac.c > +++ b/drivers/dma/at_xdmac.c > @@ -38,6 +38,27 @@ enum atc_status { > AT_XDMAC_CHAN_IS_PAUSED, > }; > > +struct at_xdmac_layout { > + /* Global Channel Read Suspend Register */ > + u8 grs; > + /* Global Write Suspend Register */ > + u8 gws; > + /* Global Channel Read Write Suspend Register */ > + u8 grws; > + /* Global Channel Read Write Resume Register */ > + u8 grwr; > + /* Global Channel Software Request Register */ > + u8 gswr; > + /* Global channel Software Request Status Register */ > + u8 gsws; > + /* Global Channel Software Flush Request Register */ > + u8 gswf; > + /* Channel reg base */ > + u8 chan_cc_reg_base; > + /* Source/Destination Interface must be specified or not */ > + bool sdif; > +}; > + > /* ----- Channels ----- */ > struct at_xdmac_chan { > struct dma_chan chan; > @@ -71,6 +92,7 @@ struct at_xdmac { > struct clk *clk; > u32 save_gim; > struct dma_pool *at_xdmac_desc_pool; > + const struct at_xdmac_layout *layout; > struct at_xdmac_chan chan[]; > }; > > @@ -103,9 +125,33 @@ struct at_xdmac_desc { > struct list_head xfer_node; > } __aligned(sizeof(u64)); > > +static struct at_xdmac_layout at_xdmac_sama5d4_layout = { static const struct > + .grs = 0x28, > + .gws = 0x2C, > + .grws = 0x30, > + .grwr = 0x34, > + .gswr = 0x38, > + .gsws = 0x3C, > + .gswf = 0x40, > + .chan_cc_reg_base = 0x50, > + .sdif = true, > +}; > + > +static struct at_xdmac_layout at_xdmac_sama7g5_layout = { static const struct > + .grs = 0x30, > + .gws = 0x38, > + .grws = 0x40, > + .grwr = 0x44, > + .gswr = 0x48, > + .gsws = 0x4C, > + .gswf = 0x50, > + .chan_cc_reg_base = 0x60, one may find these plain offsets as too raw and probably prefer some defines for them, but I too think that the members of the struct are self-explanatory, so I'm ok either way. > + .sdif = false, > +}; > + > static inline void __iomem *at_xdmac_chan_reg_base(struct at_xdmac *atxdmac, unsigned int chan_nb) > { > - return atxdmac->regs + (AT_XDMAC_CHAN_REG_BASE + chan_nb * 0x40); > + return atxdmac->regs + (atxdmac->layout->chan_cc_reg_base + chan_nb * 0x40); > } > > #define at_xdmac_read(atxdmac, reg) readl_relaxed((atxdmac)->regs + (reg)) > @@ -204,8 +250,10 @@ static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan, > first->active_xfer = true; > > /* Tell xdmac where to get the first descriptor. */ > - reg = AT_XDMAC_CNDA_NDA(first->tx_dma_desc.phys) > - | AT_XDMAC_CNDA_NDAIF(atchan->memif); > + reg = AT_XDMAC_CNDA_NDA(first->tx_dma_desc.phys); > + if (atxdmac->layout->sdif) > + reg |= AT_XDMAC_CNDA_NDAIF(atchan->memif); > + > at_xdmac_chan_write(atchan, AT_XDMAC_CNDA, reg); > > /* > @@ -400,6 +448,7 @@ static int at_xdmac_compute_chan_conf(struct dma_chan *chan, > enum dma_transfer_direction direction) > { > struct at_xdmac_chan *atchan = to_at_xdmac_chan(chan); > + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); > int csize, dwidth; > > if (direction == DMA_DEV_TO_MEM) { > @@ -407,12 +456,14 @@ static int at_xdmac_compute_chan_conf(struct dma_chan *chan, > AT91_XDMAC_DT_PERID(atchan->perid) > | AT_XDMAC_CC_DAM_INCREMENTED_AM > | AT_XDMAC_CC_SAM_FIXED_AM > - | AT_XDMAC_CC_DIF(atchan->memif) > - | AT_XDMAC_CC_SIF(atchan->perif) > | AT_XDMAC_CC_SWREQ_HWR_CONNECTED > | AT_XDMAC_CC_DSYNC_PER2MEM > | AT_XDMAC_CC_MBSIZE_SIXTEEN > | AT_XDMAC_CC_TYPE_PER_TRAN; > + if (atxdmac->layout->sdif) > + atchan->cfg |= AT_XDMAC_CC_DIF(atchan->memif) > + | AT_XDMAC_CC_SIF(atchan->perif); very odd for me to see the "|" operator on the next line. I find it hard to read and somehow exhausting. I would put it on the first line even if throughout the driver it's on the next line. > + > csize = ffs(atchan->sconfig.src_maxburst) - 1; > if (csize < 0) { > dev_err(chan2dev(chan), "invalid src maxburst value\n"); > @@ -430,12 +481,14 @@ static int at_xdmac_compute_chan_conf(struct dma_chan *chan, > AT91_XDMAC_DT_PERID(atchan->perid) > | AT_XDMAC_CC_DAM_FIXED_AM > | AT_XDMAC_CC_SAM_INCREMENTED_AM > - | AT_XDMAC_CC_DIF(atchan->perif) > - | AT_XDMAC_CC_SIF(atchan->memif) > | AT_XDMAC_CC_SWREQ_HWR_CONNECTED > | AT_XDMAC_CC_DSYNC_MEM2PER > | AT_XDMAC_CC_MBSIZE_SIXTEEN > | AT_XDMAC_CC_TYPE_PER_TRAN; > + if (atxdmac->layout->sdif) > + atchan->cfg |= AT_XDMAC_CC_DIF(atchan->perif) > + | AT_XDMAC_CC_SIF(atchan->memif); > + > csize = ffs(atchan->sconfig.dst_maxburst) - 1; > if (csize < 0) { > dev_err(chan2dev(chan), "invalid src maxburst value\n"); > @@ -711,6 +764,7 @@ at_xdmac_interleaved_queue_desc(struct dma_chan *chan, > struct data_chunk *chunk) > { > struct at_xdmac_desc *desc; > + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); > u32 dwidth; > unsigned long flags; > size_t ublen; > @@ -727,10 +781,10 @@ at_xdmac_interleaved_queue_desc(struct dma_chan *chan, > * flag status. > */ > u32 chan_cc = AT_XDMAC_CC_PERID(0x7f) > - | AT_XDMAC_CC_DIF(0) > - | AT_XDMAC_CC_SIF(0) > | AT_XDMAC_CC_MBSIZE_SIXTEEN > | AT_XDMAC_CC_TYPE_MEM_TRAN; > + if (atxdmac->layout->sdif) > + chan_cc |= AT_XDMAC_CC_DIF(0) | AT_XDMAC_CC_SIF(0); there is a comment above chan_cc init that explains that we have to use interface 0 for both source and destination, so maybe we can get rid of this explicit OR with zero and update the comment for sama7g5 case. > > dwidth = at_xdmac_align_width(chan, src | dst | chunk->size); > if (chunk->size >= (AT_XDMAC_MBR_UBC_UBLEN_MAX << dwidth)) { > @@ -893,6 +947,7 @@ at_xdmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, > size_t len, unsigned long flags) > { > struct at_xdmac_chan *atchan = to_at_xdmac_chan(chan); > + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); > struct at_xdmac_desc *first = NULL, *prev = NULL; > size_t remaining_size = len, xfer_size = 0, ublen; > dma_addr_t src_addr = src, dst_addr = dest; > @@ -911,12 +966,13 @@ at_xdmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, > u32 chan_cc = AT_XDMAC_CC_PERID(0x7f) > | AT_XDMAC_CC_DAM_INCREMENTED_AM > | AT_XDMAC_CC_SAM_INCREMENTED_AM > - | AT_XDMAC_CC_DIF(0) > - | AT_XDMAC_CC_SIF(0) > | AT_XDMAC_CC_MBSIZE_SIXTEEN > | AT_XDMAC_CC_TYPE_MEM_TRAN; > unsigned long irqflags; > > + if (atxdmac->layout->sdif) > + chan_cc |= AT_XDMAC_CC_DIF(0) | AT_XDMAC_CC_SIF(0); same here > + > dev_dbg(chan2dev(chan), "%s: src=%pad, dest=%pad, len=%zd, flags=0x%lx\n", > __func__, &src, &dest, len, flags); > > @@ -999,6 +1055,7 @@ static struct at_xdmac_desc *at_xdmac_memset_create_desc(struct dma_chan *chan, > int value) > { > struct at_xdmac_desc *desc; > + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); > unsigned long flags; > size_t ublen; > u32 dwidth; > @@ -1017,11 +1074,11 @@ static struct at_xdmac_desc *at_xdmac_memset_create_desc(struct dma_chan *chan, > u32 chan_cc = AT_XDMAC_CC_PERID(0x7f) > | AT_XDMAC_CC_DAM_UBS_AM > | AT_XDMAC_CC_SAM_INCREMENTED_AM > - | AT_XDMAC_CC_DIF(0) > - | AT_XDMAC_CC_SIF(0) > | AT_XDMAC_CC_MBSIZE_SIXTEEN > | AT_XDMAC_CC_MEMSET_HW_MODE > | AT_XDMAC_CC_TYPE_MEM_TRAN; > + if (atxdmac->layout->sdif) > + chan_cc |= AT_XDMAC_CC_DIF(0) | AT_XDMAC_CC_SIF(0); same here > > dwidth = at_xdmac_align_width(chan, dst_addr); > > @@ -1297,7 +1354,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie, > mask = AT_XDMAC_CC_TYPE | AT_XDMAC_CC_DSYNC; > value = AT_XDMAC_CC_TYPE_PER_TRAN | AT_XDMAC_CC_DSYNC_PER2MEM; > if ((desc->lld.mbr_cfg & mask) == value) { > - at_xdmac_write(atxdmac, AT_XDMAC_GSWF, atchan->mask); > + at_xdmac_write(atxdmac, atxdmac->layout->gswf, atchan->mask); > while (!(at_xdmac_chan_read(atchan, AT_XDMAC_CIS) & AT_XDMAC_CIS_FIS)) > cpu_relax(); > } > @@ -1355,7 +1412,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie, > * FIFO flush ensures that data are really written. > */ > if ((desc->lld.mbr_cfg & mask) == value) { > - at_xdmac_write(atxdmac, AT_XDMAC_GSWF, atchan->mask); > + at_xdmac_write(atxdmac, atxdmac->layout->gswf, atchan->mask); > while (!(at_xdmac_chan_read(atchan, AT_XDMAC_CIS) & AT_XDMAC_CIS_FIS)) > cpu_relax(); > } > @@ -1620,7 +1677,7 @@ static int at_xdmac_device_pause(struct dma_chan *chan) > return 0; > > spin_lock_irqsave(&atchan->lock, flags); > - at_xdmac_write(atxdmac, AT_XDMAC_GRWS, atchan->mask); > + at_xdmac_write(atxdmac, atxdmac->layout->grws, atchan->mask); > while (at_xdmac_chan_read(atchan, AT_XDMAC_CC) > & (AT_XDMAC_CC_WRIP | AT_XDMAC_CC_RDIP)) > cpu_relax(); > @@ -1643,7 +1700,7 @@ static int at_xdmac_device_resume(struct dma_chan *chan) > return 0; > } > > - at_xdmac_write(atxdmac, AT_XDMAC_GRWR, atchan->mask); > + at_xdmac_write(atxdmac, atxdmac->layout->grwr, atchan->mask); > clear_bit(AT_XDMAC_CHAN_IS_PAUSED, &atchan->status); > spin_unlock_irqrestore(&atchan->lock, flags); > > @@ -1845,6 +1902,10 @@ static int at_xdmac_probe(struct platform_device *pdev) > atxdmac->regs = base; > atxdmac->irq = irq; > > + atxdmac->layout = of_device_get_match_data(&pdev->dev); > + if (!atxdmac->layout) > + return -ENODEV; I would get the data upper in the function, after getting irq. If data is not provided, you would spare some ops that will be done gratuitously. With these addressed one may add: Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > + > atxdmac->clk = devm_clk_get(&pdev->dev, "dma_clk"); > if (IS_ERR(atxdmac->clk)) { > dev_err(&pdev->dev, "can't get dma_clk\n"); > @@ -1988,6 +2049,10 @@ static const struct dev_pm_ops atmel_xdmac_dev_pm_ops = { > static const struct of_device_id atmel_xdmac_dt_ids[] = { > { > .compatible = "atmel,sama5d4-dma", > + .data = &at_xdmac_sama5d4_layout, > + }, { > + .compatible = "microchip,sama7g5-dma", > + .data = &at_xdmac_sama7g5_layout, > }, { > /* sentinel */ > } > diff --git a/drivers/dma/at_xdmac_regs.h b/drivers/dma/at_xdmac_regs.h > index 3f7dda4c5743..7b4b4e24de70 100644 > --- a/drivers/dma/at_xdmac_regs.h > +++ b/drivers/dma/at_xdmac_regs.h > @@ -22,13 +22,6 @@ > #define AT_XDMAC_GE 0x1C /* Global Channel Enable Register */ > #define AT_XDMAC_GD 0x20 /* Global Channel Disable Register */ > #define AT_XDMAC_GS 0x24 /* Global Channel Status Register */ > -#define AT_XDMAC_GRS 0x28 /* Global Channel Read Suspend Register */ > -#define AT_XDMAC_GWS 0x2C /* Global Write Suspend Register */ > -#define AT_XDMAC_GRWS 0x30 /* Global Channel Read Write Suspend Register */ > -#define AT_XDMAC_GRWR 0x34 /* Global Channel Read Write Resume Register */ > -#define AT_XDMAC_GSWR 0x38 /* Global Channel Software Request Register */ > -#define AT_XDMAC_GSWS 0x3C /* Global channel Software Request Status Register */ > -#define AT_XDMAC_GSWF 0x40 /* Global Channel Software Flush Request Register */ > #define AT_XDMAC_VERSION 0xFFC /* XDMAC Version Register */ > > /* Channel relative registers offsets */ > @@ -134,8 +127,6 @@ > #define AT_XDMAC_CSUS 0x30 /* Channel Source Microblock Stride */ > #define AT_XDMAC_CDUS 0x34 /* Channel Destination Microblock Stride */ > > -#define AT_XDMAC_CHAN_REG_BASE 0x50 /* Channel registers base address */ > - > /* Microblock control members */ > #define AT_XDMAC_MBR_UBC_UBLEN_MAX 0xFFFFFFUL /* Maximum Microblock Length */ > #define AT_XDMAC_MBR_UBC_NDE (0x1 << 24) /* Next Descriptor Enable */ >
On 23.09.2020 10:08, Tudor Ambarus - M18064 wrote: > On 9/14/20 5:09 PM, Eugen Hristev wrote: >> SAMA7G5 SoC uses a slightly different variant of the AT_XDMAC. >> Added support by a new compatible and a layout struct that copes >> to the specific version considering the compatible string. >> Only the differences in register map are present in the layout struct. >> I reworked the register access for this part that has the differences. >> Also the Source/Destination Interface bits are no longer valid for this >> variant of the XDMAC. Thus, the layout also has a bool for specifying >> whether these bits are required or not. >> >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> >> --- >> drivers/dma/at_xdmac.c | 99 ++++++++++++++++++++++++++++++------- >> drivers/dma/at_xdmac_regs.h | 9 ---- >> 2 files changed, 82 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c >> index 81bb90206092..874484a4e79f 100644 >> --- a/drivers/dma/at_xdmac.c >> +++ b/drivers/dma/at_xdmac.c >> @@ -38,6 +38,27 @@ enum atc_status { >> AT_XDMAC_CHAN_IS_PAUSED, >> }; >> >> +struct at_xdmac_layout { >> + /* Global Channel Read Suspend Register */ >> + u8 grs; >> + /* Global Write Suspend Register */ >> + u8 gws; >> + /* Global Channel Read Write Suspend Register */ >> + u8 grws; >> + /* Global Channel Read Write Resume Register */ >> + u8 grwr; >> + /* Global Channel Software Request Register */ >> + u8 gswr; >> + /* Global channel Software Request Status Register */ >> + u8 gsws; >> + /* Global Channel Software Flush Request Register */ >> + u8 gswf; >> + /* Channel reg base */ >> + u8 chan_cc_reg_base; >> + /* Source/Destination Interface must be specified or not */ >> + bool sdif; >> +}; >> + >> /* ----- Channels ----- */ >> struct at_xdmac_chan { >> struct dma_chan chan; >> @@ -71,6 +92,7 @@ struct at_xdmac { >> struct clk *clk; >> u32 save_gim; >> struct dma_pool *at_xdmac_desc_pool; >> + const struct at_xdmac_layout *layout; >> struct at_xdmac_chan chan[]; >> }; >> >> @@ -103,9 +125,33 @@ struct at_xdmac_desc { >> struct list_head xfer_node; >> } __aligned(sizeof(u64)); >> >> +static struct at_xdmac_layout at_xdmac_sama5d4_layout = { > > static const struct > >> + .grs = 0x28, >> + .gws = 0x2C, >> + .grws = 0x30, >> + .grwr = 0x34, >> + .gswr = 0x38, >> + .gsws = 0x3C, >> + .gswf = 0x40, >> + .chan_cc_reg_base = 0x50, >> + .sdif = true, >> +}; >> + >> +static struct at_xdmac_layout at_xdmac_sama7g5_layout = { > > static const struct > >> + .grs = 0x30, >> + .gws = 0x38, >> + .grws = 0x40, >> + .grwr = 0x44, >> + .gswr = 0x48, >> + .gsws = 0x4C, >> + .gswf = 0x50, >> + .chan_cc_reg_base = 0x60, > > one may find these plain offsets as too raw and probably prefer some defines > for them, but I too think that the members of the struct are self-explanatory, > so I'm ok either way. > >> + .sdif = false, >> +}; >> + >> static inline void __iomem *at_xdmac_chan_reg_base(struct at_xdmac *atxdmac, unsigned int chan_nb) >> { >> - return atxdmac->regs + (AT_XDMAC_CHAN_REG_BASE + chan_nb * 0x40); >> + return atxdmac->regs + (atxdmac->layout->chan_cc_reg_base + chan_nb * 0x40); >> } >> >> #define at_xdmac_read(atxdmac, reg) readl_relaxed((atxdmac)->regs + (reg)) >> @@ -204,8 +250,10 @@ static void at_xdmac_start_xfer(struct at_xdmac_chan *atchan, >> first->active_xfer = true; >> >> /* Tell xdmac where to get the first descriptor. */ >> - reg = AT_XDMAC_CNDA_NDA(first->tx_dma_desc.phys) >> - | AT_XDMAC_CNDA_NDAIF(atchan->memif); >> + reg = AT_XDMAC_CNDA_NDA(first->tx_dma_desc.phys); >> + if (atxdmac->layout->sdif) >> + reg |= AT_XDMAC_CNDA_NDAIF(atchan->memif); >> + >> at_xdmac_chan_write(atchan, AT_XDMAC_CNDA, reg); >> >> /* >> @@ -400,6 +448,7 @@ static int at_xdmac_compute_chan_conf(struct dma_chan *chan, >> enum dma_transfer_direction direction) >> { >> struct at_xdmac_chan *atchan = to_at_xdmac_chan(chan); >> + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); >> int csize, dwidth; >> >> if (direction == DMA_DEV_TO_MEM) { >> @@ -407,12 +456,14 @@ static int at_xdmac_compute_chan_conf(struct dma_chan *chan, >> AT91_XDMAC_DT_PERID(atchan->perid) >> | AT_XDMAC_CC_DAM_INCREMENTED_AM >> | AT_XDMAC_CC_SAM_FIXED_AM >> - | AT_XDMAC_CC_DIF(atchan->memif) >> - | AT_XDMAC_CC_SIF(atchan->perif) >> | AT_XDMAC_CC_SWREQ_HWR_CONNECTED >> | AT_XDMAC_CC_DSYNC_PER2MEM >> | AT_XDMAC_CC_MBSIZE_SIXTEEN >> | AT_XDMAC_CC_TYPE_PER_TRAN; >> + if (atxdmac->layout->sdif) >> + atchan->cfg |= AT_XDMAC_CC_DIF(atchan->memif) >> + | AT_XDMAC_CC_SIF(atchan->perif); > > very odd for me to see the "|" operator on the next line. I find it hard to > read and somehow exhausting. I would put it on the first line even if throughout > the driver it's on the next line. > >> + >> csize = ffs(atchan->sconfig.src_maxburst) - 1; >> if (csize < 0) { >> dev_err(chan2dev(chan), "invalid src maxburst value\n"); >> @@ -430,12 +481,14 @@ static int at_xdmac_compute_chan_conf(struct dma_chan *chan, >> AT91_XDMAC_DT_PERID(atchan->perid) >> | AT_XDMAC_CC_DAM_FIXED_AM >> | AT_XDMAC_CC_SAM_INCREMENTED_AM >> - | AT_XDMAC_CC_DIF(atchan->perif) >> - | AT_XDMAC_CC_SIF(atchan->memif) >> | AT_XDMAC_CC_SWREQ_HWR_CONNECTED >> | AT_XDMAC_CC_DSYNC_MEM2PER >> | AT_XDMAC_CC_MBSIZE_SIXTEEN >> | AT_XDMAC_CC_TYPE_PER_TRAN; >> + if (atxdmac->layout->sdif) >> + atchan->cfg |= AT_XDMAC_CC_DIF(atchan->perif) >> + | AT_XDMAC_CC_SIF(atchan->memif); >> + >> csize = ffs(atchan->sconfig.dst_maxburst) - 1; >> if (csize < 0) { >> dev_err(chan2dev(chan), "invalid src maxburst value\n"); >> @@ -711,6 +764,7 @@ at_xdmac_interleaved_queue_desc(struct dma_chan *chan, >> struct data_chunk *chunk) >> { >> struct at_xdmac_desc *desc; >> + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); >> u32 dwidth; >> unsigned long flags; >> size_t ublen; >> @@ -727,10 +781,10 @@ at_xdmac_interleaved_queue_desc(struct dma_chan *chan, >> * flag status. >> */ >> u32 chan_cc = AT_XDMAC_CC_PERID(0x7f) >> - | AT_XDMAC_CC_DIF(0) >> - | AT_XDMAC_CC_SIF(0) >> | AT_XDMAC_CC_MBSIZE_SIXTEEN >> | AT_XDMAC_CC_TYPE_MEM_TRAN; >> + if (atxdmac->layout->sdif) >> + chan_cc |= AT_XDMAC_CC_DIF(0) | AT_XDMAC_CC_SIF(0); > > there is a comment above chan_cc init that explains that we have to use > interface 0 for both source and destination, so maybe we can get rid of > this explicit OR with zero and update the comment for sama7g5 case. > >> >> dwidth = at_xdmac_align_width(chan, src | dst | chunk->size); >> if (chunk->size >= (AT_XDMAC_MBR_UBC_UBLEN_MAX << dwidth)) { >> @@ -893,6 +947,7 @@ at_xdmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, >> size_t len, unsigned long flags) >> { >> struct at_xdmac_chan *atchan = to_at_xdmac_chan(chan); >> + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); >> struct at_xdmac_desc *first = NULL, *prev = NULL; >> size_t remaining_size = len, xfer_size = 0, ublen; >> dma_addr_t src_addr = src, dst_addr = dest; >> @@ -911,12 +966,13 @@ at_xdmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, >> u32 chan_cc = AT_XDMAC_CC_PERID(0x7f) >> | AT_XDMAC_CC_DAM_INCREMENTED_AM >> | AT_XDMAC_CC_SAM_INCREMENTED_AM >> - | AT_XDMAC_CC_DIF(0) >> - | AT_XDMAC_CC_SIF(0) >> | AT_XDMAC_CC_MBSIZE_SIXTEEN >> | AT_XDMAC_CC_TYPE_MEM_TRAN; >> unsigned long irqflags; >> >> + if (atxdmac->layout->sdif) >> + chan_cc |= AT_XDMAC_CC_DIF(0) | AT_XDMAC_CC_SIF(0); > > same here > >> + >> dev_dbg(chan2dev(chan), "%s: src=%pad, dest=%pad, len=%zd, flags=0x%lx\n", >> __func__, &src, &dest, len, flags); >> >> @@ -999,6 +1055,7 @@ static struct at_xdmac_desc *at_xdmac_memset_create_desc(struct dma_chan *chan, >> int value) >> { >> struct at_xdmac_desc *desc; >> + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); >> unsigned long flags; >> size_t ublen; >> u32 dwidth; >> @@ -1017,11 +1074,11 @@ static struct at_xdmac_desc *at_xdmac_memset_create_desc(struct dma_chan *chan, >> u32 chan_cc = AT_XDMAC_CC_PERID(0x7f) >> | AT_XDMAC_CC_DAM_UBS_AM >> | AT_XDMAC_CC_SAM_INCREMENTED_AM >> - | AT_XDMAC_CC_DIF(0) >> - | AT_XDMAC_CC_SIF(0) >> | AT_XDMAC_CC_MBSIZE_SIXTEEN >> | AT_XDMAC_CC_MEMSET_HW_MODE >> | AT_XDMAC_CC_TYPE_MEM_TRAN; >> + if (atxdmac->layout->sdif) >> + chan_cc |= AT_XDMAC_CC_DIF(0) | AT_XDMAC_CC_SIF(0); > > same here > >> >> dwidth = at_xdmac_align_width(chan, dst_addr); >> >> @@ -1297,7 +1354,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie, >> mask = AT_XDMAC_CC_TYPE | AT_XDMAC_CC_DSYNC; >> value = AT_XDMAC_CC_TYPE_PER_TRAN | AT_XDMAC_CC_DSYNC_PER2MEM; >> if ((desc->lld.mbr_cfg & mask) == value) { >> - at_xdmac_write(atxdmac, AT_XDMAC_GSWF, atchan->mask); >> + at_xdmac_write(atxdmac, atxdmac->layout->gswf, atchan->mask); >> while (!(at_xdmac_chan_read(atchan, AT_XDMAC_CIS) & AT_XDMAC_CIS_FIS)) >> cpu_relax(); >> } >> @@ -1355,7 +1412,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie, >> * FIFO flush ensures that data are really written. >> */ >> if ((desc->lld.mbr_cfg & mask) == value) { >> - at_xdmac_write(atxdmac, AT_XDMAC_GSWF, atchan->mask); >> + at_xdmac_write(atxdmac, atxdmac->layout->gswf, atchan->mask); >> while (!(at_xdmac_chan_read(atchan, AT_XDMAC_CIS) & AT_XDMAC_CIS_FIS)) >> cpu_relax(); >> } >> @@ -1620,7 +1677,7 @@ static int at_xdmac_device_pause(struct dma_chan *chan) >> return 0; >> >> spin_lock_irqsave(&atchan->lock, flags); >> - at_xdmac_write(atxdmac, AT_XDMAC_GRWS, atchan->mask); >> + at_xdmac_write(atxdmac, atxdmac->layout->grws, atchan->mask); >> while (at_xdmac_chan_read(atchan, AT_XDMAC_CC) >> & (AT_XDMAC_CC_WRIP | AT_XDMAC_CC_RDIP)) >> cpu_relax(); >> @@ -1643,7 +1700,7 @@ static int at_xdmac_device_resume(struct dma_chan *chan) >> return 0; >> } >> >> - at_xdmac_write(atxdmac, AT_XDMAC_GRWR, atchan->mask); >> + at_xdmac_write(atxdmac, atxdmac->layout->grwr, atchan->mask); >> clear_bit(AT_XDMAC_CHAN_IS_PAUSED, &atchan->status); >> spin_unlock_irqrestore(&atchan->lock, flags); >> >> @@ -1845,6 +1902,10 @@ static int at_xdmac_probe(struct platform_device *pdev) >> atxdmac->regs = base; >> atxdmac->irq = irq; >> >> + atxdmac->layout = of_device_get_match_data(&pdev->dev); >> + if (!atxdmac->layout) >> + return -ENODEV; > > I would get the data upper in the function, after getting irq. If data is > not provided, you would spare some ops that will be done gratuitously. Hi Tudor, This would be difficult to do as atxdmac is allocated just a few lines before. Also, actually the get_match_data should not fail normally. If this would fail it would mean that the driver is probed to a wrong driver compatible... which should not happen. Thanks for reviewing. I am sending v2. Eugen > > With these addressed one may add: > Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> + >> atxdmac->clk = devm_clk_get(&pdev->dev, "dma_clk"); >> if (IS_ERR(atxdmac->clk)) { >> dev_err(&pdev->dev, "can't get dma_clk\n"); >> @@ -1988,6 +2049,10 @@ static const struct dev_pm_ops atmel_xdmac_dev_pm_ops = { >> static const struct of_device_id atmel_xdmac_dt_ids[] = { >> { >> .compatible = "atmel,sama5d4-dma", >> + .data = &at_xdmac_sama5d4_layout, >> + }, { >> + .compatible = "microchip,sama7g5-dma", >> + .data = &at_xdmac_sama7g5_layout, >> }, { >> /* sentinel */ >> } >> diff --git a/drivers/dma/at_xdmac_regs.h b/drivers/dma/at_xdmac_regs.h >> index 3f7dda4c5743..7b4b4e24de70 100644 >> --- a/drivers/dma/at_xdmac_regs.h >> +++ b/drivers/dma/at_xdmac_regs.h >> @@ -22,13 +22,6 @@ >> #define AT_XDMAC_GE 0x1C /* Global Channel Enable Register */ >> #define AT_XDMAC_GD 0x20 /* Global Channel Disable Register */ >> #define AT_XDMAC_GS 0x24 /* Global Channel Status Register */ >> -#define AT_XDMAC_GRS 0x28 /* Global Channel Read Suspend Register */ >> -#define AT_XDMAC_GWS 0x2C /* Global Write Suspend Register */ >> -#define AT_XDMAC_GRWS 0x30 /* Global Channel Read Write Suspend Register */ >> -#define AT_XDMAC_GRWR 0x34 /* Global Channel Read Write Resume Register */ >> -#define AT_XDMAC_GSWR 0x38 /* Global Channel Software Request Register */ >> -#define AT_XDMAC_GSWS 0x3C /* Global channel Software Request Status Register */ >> -#define AT_XDMAC_GSWF 0x40 /* Global Channel Software Flush Request Register */ >> #define AT_XDMAC_VERSION 0xFFC /* XDMAC Version Register */ >> >> /* Channel relative registers offsets */ >> @@ -134,8 +127,6 @@ >> #define AT_XDMAC_CSUS 0x30 /* Channel Source Microblock Stride */ >> #define AT_XDMAC_CDUS 0x34 /* Channel Destination Microblock Stride */ >> >> -#define AT_XDMAC_CHAN_REG_BASE 0x50 /* Channel registers base address */ >> - >> /* Microblock control members */ >> #define AT_XDMAC_MBR_UBC_UBLEN_MAX 0xFFFFFFUL /* Maximum Microblock Length */ >> #define AT_XDMAC_MBR_UBC_NDE (0x1 << 24) /* Next Descriptor Enable */ >> >
This series adds support for sama7g5-based at_xdmac. In sama7g5, the XDMAC is present in a slightly modified form. This series tries to adapt the existing driver to the differences. According to the compatible string, we select one of two possible layouts, which hold the differences in registermap and supported features. Thanks! Eugen Hristev (7): dmaengine: at_xdmac: separate register defines into header file MAINTAINERS: add dma/at_xdmac_regs.h to XDMAC driver entry dt-bindings: dmaengine: at_xdmac: add compatible with microchip,sama7g5 dmaengine: at_xdmac: adapt perid for mem2mem operations dmaengine: at_xdmac: add support for sama7g5 based at_xdmac dt-bindings: dmaengine: at_xdmac: add optional microchip,m2m property dmaengine: at_xdmac: add AXI priority support and recommended settings .../devicetree/bindings/dma/atmel-xdma.txt | 9 +- MAINTAINERS | 1 + drivers/dma/at_xdmac.c | 273 +++++++----------- drivers/dma/at_xdmac_regs.h | 161 +++++++++++ 4 files changed, 281 insertions(+), 163 deletions(-) create mode 100644 drivers/dma/at_xdmac_regs.h