Re: [PATCH 5/7] dmaengine: at_xdmac: add support for sama7g5 based at_xdmac

From: Tudor.Ambarus
Date: Wed Sep 23 2020 - 03:08:09 EST

Next message: Abdul Anshad Azeez: "Re: Performance regressions in networking & storage benchmarks in Linux kernel 5.8"
Previous message: Amit Kachhap: "Re: [PATCH 6/6] kselftest/arm64: Check mte tagged user address in kernel"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx>
> +
> 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 */
>

Next message: Abdul Anshad Azeez: "Re: Performance regressions in networking & storage benchmarks in Linux kernel 5.8"
Previous message: Amit Kachhap: "Re: [PATCH 6/6] kselftest/arm64: Check mte tagged user address in kernel"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]