Re: [RFC PATCH 01/10] dma-engine: sun4i: Add a quirk to support different chips

[Maxime Ripard <maxime.ripard@bootlin.com>]

[-- Attachment #1.1: Type: text/plain, Size: 12664 bytes --]

Hi,

(you don't really need the RFC tag. RFC tags are usually meant to ask
comments on the general approach, not the implementation).

On Mon, Dec 03, 2018 at 12:23:08AM +0300, Mesih Kilinc wrote:
> Allwinner suniv F1C100s has similar DMA engine to sun4i. Several
> registers has different addresses. Total dma channels, endpoint counts
> and max burst counts are also different.
> 
> In order to support F1C100s add a quirk structure to hold IC specific
> data.
> 
> Signed-off-by: Mesih Kilinc <mesihkilinc@gmail.com>
> ---
>  drivers/dma/sun4i-dma.c | 138 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 106 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
> index f4ed3f1..e86b424 100644
> --- a/drivers/dma/sun4i-dma.c
> +++ b/drivers/dma/sun4i-dma.c
> @@ -16,6 +16,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of_dma.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> @@ -34,6 +35,8 @@
>  #define SUN4I_DMA_CFG_SRC_ADDR_MODE(mode)	((mode) << 5)
>  #define SUN4I_DMA_CFG_SRC_DRQ_TYPE(type)	(type)
>  
> +#define SUN4I_MAX_BURST	8
> +
>  /** Normal DMA register values **/
>  
>  /* Normal DMA source/destination data request type values */
> @@ -126,6 +129,32 @@
>  	 SUN4I_DDMA_PARA_DST_WAIT_CYCLES(2) |				\
>  	 SUN4I_DDMA_PARA_SRC_WAIT_CYCLES(2))
>  
> +/*
> + * Hardware channels / ports representation
> + *
> + * The hardware is used in several SoCs, with differing numbers
> + * of channels and endpoints. This structure ties those numbers
> + * to a certain compatible string.
> + */
> +struct sun4i_dma_config {
> +	u32 ndma_nr_max_channels;
> +	u32 ndma_nr_max_vchans;
> +
> +	u32 ddma_nr_max_channels;
> +	u32 ddma_nr_max_vchans;
> +
> +	u32 dma_nr_max_channels;
> +
> +        void (*set_dst_data_width)(u32 *p_cfg, s8 data_width);
> +        void (*set_src_data_width)(u32 *p_cfg, s8 data_width);

This should be indented with tabs, not spaces.

> +	int (*convert_burst)(u32 maxburst);
> +
> +	u8 ndma_drq_sdram;
> +	u8 ddma_drq_sdram;
> +
> +	u8 max_burst;

You'd be better off using a bitmask wit hthe supported bursts, like
we're doing in sun6i-dma.

> +};
> +
>  struct sun4i_dma_pchan {
>  	/* Register base of channel */
>  	void __iomem			*base;
> @@ -163,7 +192,7 @@ struct sun4i_dma_contract {
>  };
>  
>  struct sun4i_dma_dev {
> -	DECLARE_BITMAP(pchans_used, SUN4I_DMA_NR_MAX_CHANNELS);
> +	unsigned long *pchans_used;
>  	struct dma_device		slave;
>  	struct sun4i_dma_pchan		*pchans;
>  	struct sun4i_dma_vchan		*vchans;
> @@ -171,6 +200,7 @@ struct sun4i_dma_dev {
>  	struct clk			*clk;
>  	int				irq;
>  	spinlock_t			lock;
> +	const struct sun4i_dma_config *cfg;

This should be aligned to the rest of the members, using tabs.

>  };
>  
>  static struct sun4i_dma_dev *to_sun4i_dma_dev(struct dma_device *dev)
> @@ -193,7 +223,17 @@ static struct device *chan2dev(struct dma_chan *chan)
>  	return &chan->dev->device;
>  }
>  
> -static int convert_burst(u32 maxburst)
> +static void set_dst_data_width_a10(u32 *p_cfg, s8 data_width)
> +{
> +	*p_cfg |= SUN4I_DMA_CFG_DST_DATA_WIDTH(data_width);
> +}
> +
> +static void set_src_data_width_a10(u32 *p_cfg, s8 data_width)
> +{
> +	*p_cfg |= SUN4I_DMA_CFG_SRC_DATA_WIDTH(data_width);
> +}
> +
> +static int convert_burst_a10(u32 maxburst)
>  {
>  	if (maxburst > 8)
>  		return -EINVAL;
> @@ -226,15 +266,15 @@ static struct sun4i_dma_pchan *find_and_use_pchan(struct sun4i_dma_dev *priv,
>  	int i, max;
>  
>  	/*
> -	 * pchans 0-SUN4I_NDMA_NR_MAX_CHANNELS are normal, and
> -	 * SUN4I_NDMA_NR_MAX_CHANNELS+ are dedicated ones
> +	 * pchans 0-priv->cfg->ndma_nr_max_channels are normal, and
> +	 * priv->cfg->ndma_nr_max_channels+ are dedicated ones

This should be next to the structure you just created.

>  	 */
>  	if (vchan->is_dedicated) {
> -		i = SUN4I_NDMA_NR_MAX_CHANNELS;
> -		max = SUN4I_DMA_NR_MAX_CHANNELS;
> +		i = priv->cfg->ndma_nr_max_channels;
> +		max = priv->cfg->dma_nr_max_channels;
>  	} else {
>  		i = 0;
> -		max = SUN4I_NDMA_NR_MAX_CHANNELS;
> +		max = priv->cfg->ndma_nr_max_channels;
>  	}
>  
>  	spin_lock_irqsave(&priv->lock, flags);
> @@ -437,6 +477,7 @@ generate_ndma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
>  		      size_t len, struct dma_slave_config *sconfig,
>  		      enum dma_transfer_direction direction)
>  {
> +	struct sun4i_dma_dev *priv = to_sun4i_dma_dev(chan->device);
>  	struct sun4i_dma_promise *promise;
>  	int ret;
>  
> @@ -460,13 +501,13 @@ generate_ndma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
>  		sconfig->src_addr_width, sconfig->dst_addr_width);
>  
>  	/* Source burst */
> -	ret = convert_burst(sconfig->src_maxburst);
> +	ret = priv->cfg->convert_burst(sconfig->src_maxburst);
>  	if (ret < 0)
>  		goto fail;
>  	promise->cfg |= SUN4I_DMA_CFG_SRC_BURST_LENGTH(ret);
>  
>  	/* Destination burst */
> -	ret = convert_burst(sconfig->dst_maxburst);
> +	ret = priv->cfg->convert_burst(sconfig->dst_maxburst);
>  	if (ret < 0)
>  		goto fail;
>  	promise->cfg |= SUN4I_DMA_CFG_DST_BURST_LENGTH(ret);
> @@ -475,13 +516,13 @@ generate_ndma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
>  	ret = convert_buswidth(sconfig->src_addr_width);
>  	if (ret < 0)
>  		goto fail;
> -	promise->cfg |= SUN4I_DMA_CFG_SRC_DATA_WIDTH(ret);
> +	priv->cfg->set_src_data_width(&promise->cfg, ret);
>  
>  	/* Destination bus width */
>  	ret = convert_buswidth(sconfig->dst_addr_width);
>  	if (ret < 0)
>  		goto fail;
> -	promise->cfg |= SUN4I_DMA_CFG_DST_DATA_WIDTH(ret);
> +	priv->cfg->set_dst_data_width(&promise->cfg, ret);
>  
>  	return promise;
>  
> @@ -503,6 +544,7 @@ static struct sun4i_dma_promise *
>  generate_ddma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
>  		      size_t len, struct dma_slave_config *sconfig)
>  {
> +	struct sun4i_dma_dev *priv = to_sun4i_dma_dev(chan->device);
>  	struct sun4i_dma_promise *promise;
>  	int ret;
>  
> @@ -517,13 +559,13 @@ generate_ddma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
>  		SUN4I_DDMA_CFG_BYTE_COUNT_MODE_REMAIN;
>  
>  	/* Source burst */
> -	ret = convert_burst(sconfig->src_maxburst);
> +	ret = priv->cfg->convert_burst(sconfig->src_maxburst);
>  	if (ret < 0)
>  		goto fail;
>  	promise->cfg |= SUN4I_DMA_CFG_SRC_BURST_LENGTH(ret);
>  
>  	/* Destination burst */
> -	ret = convert_burst(sconfig->dst_maxburst);
> +	ret = priv->cfg->convert_burst(sconfig->dst_maxburst);
>  	if (ret < 0)
>  		goto fail;
>  	promise->cfg |= SUN4I_DMA_CFG_DST_BURST_LENGTH(ret);
> @@ -532,13 +574,13 @@ generate_ddma_promise(struct dma_chan *chan, dma_addr_t src, dma_addr_t dest,
>  	ret = convert_buswidth(sconfig->src_addr_width);
>  	if (ret < 0)
>  		goto fail;
> -	promise->cfg |= SUN4I_DMA_CFG_SRC_DATA_WIDTH(ret);
> +	priv->cfg->set_src_data_width(&promise->cfg, ret);
>  
>  	/* Destination bus width */
>  	ret = convert_buswidth(sconfig->dst_addr_width);
>  	if (ret < 0)
>  		goto fail;
> -	promise->cfg |= SUN4I_DMA_CFG_DST_DATA_WIDTH(ret);
> +	priv->cfg->set_dst_data_width(&promise->cfg, ret);
>  
>  	return promise;
>  
> @@ -615,6 +657,7 @@ static struct dma_async_tx_descriptor *
>  sun4i_dma_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest,
>  			  dma_addr_t src, size_t len, unsigned long flags)
>  {
> +	struct sun4i_dma_dev *priv = to_sun4i_dma_dev(chan->device);
>  	struct sun4i_dma_vchan *vchan = to_sun4i_dma_vchan(chan);
>  	struct dma_slave_config *sconfig = &vchan->cfg;
>  	struct sun4i_dma_promise *promise;
> @@ -631,8 +674,8 @@ sun4i_dma_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest,
>  	 */
>  	sconfig->src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>  	sconfig->dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> -	sconfig->src_maxburst = 8;
> -	sconfig->dst_maxburst = 8;
> +	sconfig->src_maxburst = priv->cfg->max_burst;
> +	sconfig->dst_maxburst = priv->cfg->max_burst;
>  
>  	if (vchan->is_dedicated)
>  		promise = generate_ddma_promise(chan, src, dest, len, sconfig);
> @@ -647,11 +690,13 @@ sun4i_dma_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest,
>  
>  	/* Configure memcpy mode */
>  	if (vchan->is_dedicated) {
> -		promise->cfg |= SUN4I_DMA_CFG_SRC_DRQ_TYPE(SUN4I_DDMA_DRQ_TYPE_SDRAM) |
> -				SUN4I_DMA_CFG_DST_DRQ_TYPE(SUN4I_DDMA_DRQ_TYPE_SDRAM);
> +		promise->cfg |=
> +			SUN4I_DMA_CFG_SRC_DRQ_TYPE(priv->cfg->ddma_drq_sdram) |
> +			SUN4I_DMA_CFG_DST_DRQ_TYPE(priv->cfg->ddma_drq_sdram);
>  	} else {
> -		promise->cfg |= SUN4I_DMA_CFG_SRC_DRQ_TYPE(SUN4I_NDMA_DRQ_TYPE_SDRAM) |
> -				SUN4I_DMA_CFG_DST_DRQ_TYPE(SUN4I_NDMA_DRQ_TYPE_SDRAM);
> +		promise->cfg |=
> +			SUN4I_DMA_CFG_SRC_DRQ_TYPE(priv->cfg->ndma_drq_sdram) |
> +			SUN4I_DMA_CFG_DST_DRQ_TYPE(priv->cfg->ndma_drq_sdram);
>  	}
>  
>  	/* Fill the contract with our only promise */
> @@ -666,6 +711,7 @@ sun4i_dma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf, size_t len,
>  			  size_t period_len, enum dma_transfer_direction dir,
>  			  unsigned long flags)
>  {
> +	struct sun4i_dma_dev *priv = to_sun4i_dma_dev(chan->device);
>  	struct sun4i_dma_vchan *vchan = to_sun4i_dma_vchan(chan);
>  	struct dma_slave_config *sconfig = &vchan->cfg;
>  	struct sun4i_dma_promise *promise;
> @@ -701,7 +747,7 @@ sun4i_dma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf, size_t len,
>  	if (dir == DMA_MEM_TO_DEV) {
>  		src = buf;
>  		dest = sconfig->dst_addr;
> -		endpoints = SUN4I_DMA_CFG_SRC_DRQ_TYPE(SUN4I_NDMA_DRQ_TYPE_SDRAM) |
> +		endpoints = SUN4I_DMA_CFG_SRC_DRQ_TYPE(priv->cfg->ndma_drq_sdram) |
>  			    SUN4I_DMA_CFG_DST_DRQ_TYPE(vchan->endpoint) |
>  			    SUN4I_DMA_CFG_DST_ADDR_MODE(SUN4I_NDMA_ADDR_MODE_IO);
>  	} else {
> @@ -709,7 +755,7 @@ sun4i_dma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf, size_t len,
>  		dest = buf;
>  		endpoints = SUN4I_DMA_CFG_SRC_DRQ_TYPE(vchan->endpoint) |
>  			    SUN4I_DMA_CFG_SRC_ADDR_MODE(SUN4I_NDMA_ADDR_MODE_IO) |
> -			    SUN4I_DMA_CFG_DST_DRQ_TYPE(SUN4I_NDMA_DRQ_TYPE_SDRAM);
> +			    SUN4I_DMA_CFG_DST_DRQ_TYPE(priv->cfg->ndma_drq_sdram);
>  	}
>  
>  	/*
> @@ -772,6 +818,7 @@ sun4i_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  			unsigned int sg_len, enum dma_transfer_direction dir,
>  			unsigned long flags, void *context)
>  {
> +	struct sun4i_dma_dev *priv = to_sun4i_dma_dev(chan->device);
>  	struct sun4i_dma_vchan *vchan = to_sun4i_dma_vchan(chan);
>  	struct dma_slave_config *sconfig = &vchan->cfg;
>  	struct sun4i_dma_promise *promise;
> @@ -797,11 +844,11 @@ sun4i_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  	if (vchan->is_dedicated) {
>  		io_mode = SUN4I_DDMA_ADDR_MODE_IO;
>  		linear_mode = SUN4I_DDMA_ADDR_MODE_LINEAR;
> -		ram_type = SUN4I_DDMA_DRQ_TYPE_SDRAM;
> +		ram_type = priv->cfg->ddma_drq_sdram;
>  	} else {
>  		io_mode = SUN4I_NDMA_ADDR_MODE_IO;
>  		linear_mode = SUN4I_NDMA_ADDR_MODE_LINEAR;
> -		ram_type = SUN4I_NDMA_DRQ_TYPE_SDRAM;
> +		ram_type = priv->cfg->ndma_drq_sdram;
>  	}
>  
>  	if (dir == DMA_MEM_TO_DEV)
> @@ -1130,6 +1177,10 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	priv->cfg = of_device_get_match_data(&pdev->dev);
> +	if (!priv->cfg)
> +		return -ENODEV;
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	priv->base = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(priv->base))
> @@ -1178,23 +1229,26 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>  
>  	priv->slave.dev = &pdev->dev;
>  
> -	priv->pchans = devm_kcalloc(&pdev->dev, SUN4I_DMA_NR_MAX_CHANNELS,
> +	priv->pchans = devm_kcalloc(&pdev->dev, priv->cfg->dma_nr_max_channels,
>  				    sizeof(struct sun4i_dma_pchan), GFP_KERNEL);
>  	priv->vchans = devm_kcalloc(&pdev->dev, SUN4I_DMA_NR_MAX_VCHANS,
>  				    sizeof(struct sun4i_dma_vchan), GFP_KERNEL);
> -	if (!priv->vchans || !priv->pchans)
> +	priv->pchans_used = devm_kcalloc(&pdev->dev,
> +			BITS_TO_LONGS(priv->cfg->dma_nr_max_channels),
> +			sizeof(unsigned long), GFP_KERNEL);

I'm not sure we really need a dynamic allocation here. Just keep the
bitmap, and use the bigger size.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel