From: <Tudor.Ambarus@microchip.com>
To: <Eugen.Hristev@microchip.com>, <vkoul@kernel.org>,
<robh+dt@kernel.org>, <Ludovic.Desroches@microchip.com>
Cc: <dmaengine@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, <Nicolas.Ferre@microchip.com>
Subject: Re: [PATCH 5/7] dmaengine: at_xdmac: add support for sama7g5 based at_xdmac
Date: Wed, 23 Sep 2020 07:08:03 +0000 [thread overview]
Message-ID: <02ed5fc6-d2f9-e717-a0f1-2c08ef556abb@microchip.com> (raw)
In-Reply-To: <20200914140956.221432-6-eugen.hristev@microchip.com>
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 */
>
next prev parent reply other threads:[~2020-09-23 7:08 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-14 14:09 [PATCH 0/7] dmaengine: add support for sama7g5 based at_xdmac Eugen Hristev
2020-09-14 14:09 ` [PATCH 1/7] dmaengine: at_xdmac: separate register defines into header file Eugen Hristev
2020-09-23 5:07 ` Tudor.Ambarus
2020-09-14 14:09 ` [PATCH 2/7] MAINTAINERS: add dma/at_xdmac_regs.h to XDMAC driver entry Eugen Hristev
2020-09-23 5:12 ` Tudor.Ambarus
2020-09-14 14:09 ` [PATCH 3/7] dt-bindings: dmaengine: at_xdmac: add compatible with microchip,sama7g5 Eugen Hristev
2020-09-22 23:32 ` Rob Herring
2020-09-14 14:09 ` [PATCH 4/7] dmaengine: at_xdmac: adapt perid for mem2mem operations Eugen Hristev
2020-09-23 5:30 ` Tudor.Ambarus
2020-09-23 5:35 ` Tudor.Ambarus
2020-09-14 14:09 ` [PATCH 5/7] dmaengine: at_xdmac: add support for sama7g5 based at_xdmac Eugen Hristev
2020-09-23 7:08 ` Tudor.Ambarus [this message]
2020-10-16 6:52 ` Eugen.Hristev
2020-09-14 14:09 ` [PATCH 6/7] dt-bindings: dmaengine: at_xdmac: add optional microchip,m2m property Eugen Hristev
2020-09-22 23:33 ` Rob Herring
2020-10-16 6:45 ` Eugen.Hristev
2020-10-16 7:06 ` Vinod Koul
2020-10-16 7:09 ` Eugen.Hristev
2020-09-14 14:09 ` [PATCH 7/7] dmaengine: at_xdmac: add AXI priority support and recommended settings Eugen Hristev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=02ed5fc6-d2f9-e717-a0f1-2c08ef556abb@microchip.com \
--to=tudor.ambarus@microchip.com \
--cc=Eugen.Hristev@microchip.com \
--cc=Ludovic.Desroches@microchip.com \
--cc=Nicolas.Ferre@microchip.com \
--cc=devicetree@vger.kernel.org \
--cc=dmaengine@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).