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

From: Eugen.Hristev
Date: Fri Oct 16 2020 - 02:53:05 EST

Next message: Christoph Hellwig: "Re: [PATCH 1/8] powerpc/uaccess: Add unsafe_copy_from_user"
Previous message: Joakim Zhang: "RE: [PATCH 2/6] dt-bindings: can: flexcan: fix fsl,clk-source property"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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@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: Christoph Hellwig: "Re: [PATCH 1/8] powerpc/uaccess: Add unsafe_copy_from_user"
Previous message: Joakim Zhang: "RE: [PATCH 2/6] dt-bindings: can: flexcan: fix fsl,clk-source property"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]