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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 */
> 





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux