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





[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux