[v9,09/38] dma: cirrus: Convert to DT for Cirrus EP93xx

Commit Message

Nikita Shubin via B4 Relay March 26, 2024, 9:18 a.m. UTC

From: Nikita Shubin <[e-mail]>

Convert Cirrus EP93xx DMA to device tree usage:

- add OF ID match table with data
- add of_probe for device tree
- add xlate for m2m/m2p
- drop subsys_initcall code
- drop platform probe
- drop platform structs usage

From now on it only supports device tree probing.

Co-developed-by: Alexander Sverdlin <[e-mail]>
Signed-off-by: Alexander Sverdlin <[e-mail]>
Signed-off-by: Nikita Shubin <[e-mail]>
---
 drivers/dma/ep93xx_dma.c                 | 239 ++++++++++++++++++++++++-------
 include/linux/platform_data/dma-ep93xx.h |   6 +
 2 files changed, 191 insertions(+), 54 deletions(-)

Comments

Vinod Koul March 28, 2024, 4:31 a.m. UTC | #1

On 26-03-24, 12:18, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <[e-mail]>
> 
> Convert Cirrus EP93xx DMA to device tree usage:

Subsytem is dmaengine: pls fix that

> 
> - add OF ID match table with data
> - add of_probe for device tree
> - add xlate for m2m/m2p
> - drop subsys_initcall code
> - drop platform probe
> - drop platform structs usage
> 
> >From now on it only supports device tree probing.
> 
> Co-developed-by: Alexander Sverdlin <[e-mail]>
> Signed-off-by: Alexander Sverdlin <[e-mail]>
> Signed-off-by: Nikita Shubin <[e-mail]>
> ---
>  drivers/dma/ep93xx_dma.c                 | 239 ++++++++++++++++++++++++-------
>  include/linux/platform_data/dma-ep93xx.h |   6 +
>  2 files changed, 191 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
> index d6c60635e90d..17c8e2badee2 100644
> --- a/drivers/dma/ep93xx_dma.c
> +++ b/drivers/dma/ep93xx_dma.c
> @@ -20,6 +20,8 @@
>  #include <linux/dmaengine.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/of_dma.h>
> +#include <linux/overflow.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
> @@ -104,6 +106,11 @@
>  #define DMA_MAX_CHAN_BYTES		0xffff
>  #define DMA_MAX_CHAN_DESCRIPTORS	32
>  
> +enum ep93xx_dma_type {
> +	M2P_DMA,

Is this missing P2M?

> +	M2M_DMA,
> +};
> +
>  struct ep93xx_dma_engine;
>  static int ep93xx_dma_slave_config_write(struct dma_chan *chan,
>  					 enum dma_transfer_direction dir,
> @@ -129,11 +136,17 @@ struct ep93xx_dma_desc {
>  	struct list_head		node;
>  };
>  
> +struct ep93xx_dma_chan_cfg {
> +	u8				port;
> +	enum dma_transfer_direction	dir;

Why is direction stored here, it should be derived from the prep_xxx
call, that has direction as an argument

> +};
> +
>  /**
>   * struct ep93xx_dma_chan - an EP93xx DMA M2P/M2M channel
>   * @chan: dmaengine API channel
>   * @edma: pointer to the engine device
>   * @regs: memory mapped registers
> + * @dma_cfg: channel number, direction
>   * @irq: interrupt number of the channel
>   * @clk: clock used by this channel
>   * @tasklet: channel specific tasklet used for callbacks
> @@ -157,14 +170,12 @@ struct ep93xx_dma_desc {
>   * descriptor in the chain. When a descriptor is moved to the @active queue,
>   * the first and chained descriptors are flattened into a single list.
>   *
> - * @chan.private holds pointer to &struct ep93xx_dma_data which contains
> - * necessary channel configuration information. For memcpy channels this must
> - * be %NULL.
>   */
>  struct ep93xx_dma_chan {
>  	struct dma_chan			chan;
>  	const struct ep93xx_dma_engine	*edma;
>  	void __iomem			*regs;
> +	struct ep93xx_dma_chan_cfg	dma_cfg;
>  	int				irq;
>  	struct clk			*clk;
>  	struct tasklet_struct		tasklet;
> @@ -216,6 +227,11 @@ struct ep93xx_dma_engine {
>  	struct ep93xx_dma_chan	channels[] __counted_by(num_channels);
>  };
>  
> +struct ep93xx_edma_data {
> +	u32	id;
> +	size_t	num_channels;
> +};
> +
>  static inline struct device *chan2dev(struct ep93xx_dma_chan *edmac)
>  {
>  	return &edmac->chan.dev->device;
> @@ -318,10 +334,9 @@ static void m2p_set_control(struct ep93xx_dma_chan *edmac, u32 control)
>  
>  static int m2p_hw_setup(struct ep93xx_dma_chan *edmac)
>  {
> -	struct ep93xx_dma_data *data = edmac->chan.private;
>  	u32 control;
>  
> -	writel(data->port & 0xf, edmac->regs + M2P_PPALLOC);
> +	writel(edmac->dma_cfg.port & 0xf, edmac->regs + M2P_PPALLOC);
>  
>  	control = M2P_CONTROL_CH_ERROR_INT | M2P_CONTROL_ICE
>  		| M2P_CONTROL_ENABLE;
> @@ -458,16 +473,15 @@ static int m2p_hw_interrupt(struct ep93xx_dma_chan *edmac)
>  
>  static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
>  {
> -	const struct ep93xx_dma_data *data = edmac->chan.private;
>  	u32 control = 0;
>  
> -	if (!data) {
> +	if (edmac->dma_cfg.dir == DMA_MEM_TO_MEM) {
>  		/* This is memcpy channel, nothing to configure */
>  		writel(control, edmac->regs + M2M_CONTROL);
>  		return 0;
>  	}
>  
> -	switch (data->port) {
> +	switch (edmac->dma_cfg.port) {
>  	case EP93XX_DMA_SSP:
>  		/*
>  		 * This was found via experimenting - anything less than 5
> @@ -477,7 +491,7 @@ static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
>  		control = (5 << M2M_CONTROL_PWSC_SHIFT);
>  		control |= M2M_CONTROL_NO_HDSK;
>  
> -		if (data->direction == DMA_MEM_TO_DEV) {
> +		if (edmac->dma_cfg.dir == DMA_MEM_TO_DEV) {
>  			control |= M2M_CONTROL_DAH;
>  			control |= M2M_CONTROL_TM_TX;
>  			control |= M2M_CONTROL_RSS_SSPTX;
> @@ -493,7 +507,7 @@ static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
>  		 * This IDE part is totally untested. Values below are taken
>  		 * from the EP93xx Users's Guide and might not be correct.
>  		 */
> -		if (data->direction == DMA_MEM_TO_DEV) {
> +		if (edmac->dma_cfg.dir == DMA_MEM_TO_DEV) {
>  			/* Worst case from the UG */
>  			control = (3 << M2M_CONTROL_PWSC_SHIFT);
>  			control |= M2M_CONTROL_DAH;
> @@ -548,7 +562,6 @@ static void m2m_fill_desc(struct ep93xx_dma_chan *edmac)
>  
>  static void m2m_hw_submit(struct ep93xx_dma_chan *edmac)
>  {
> -	struct ep93xx_dma_data *data = edmac->chan.private;
>  	u32 control = readl(edmac->regs + M2M_CONTROL);
>  
>  	/*
> @@ -574,7 +587,7 @@ static void m2m_hw_submit(struct ep93xx_dma_chan *edmac)
>  	control |= M2M_CONTROL_ENABLE;
>  	writel(control, edmac->regs + M2M_CONTROL);
>  
> -	if (!data) {
> +	if (edmac->dma_cfg.dir == DMA_MEM_TO_MEM) {
>  		/*
>  		 * For memcpy channels the software trigger must be asserted
>  		 * in order to start the memcpy operation.
> @@ -636,7 +649,7 @@ static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
>  		 */
>  		if (ep93xx_dma_advance_active(edmac)) {
>  			m2m_fill_desc(edmac);
> -			if (done && !edmac->chan.private) {
> +			if (done && edmac->dma_cfg.dir == DMA_MEM_TO_MEM) {
>  				/* Software trigger for memcpy channel */
>  				control = readl(edmac->regs + M2M_CONTROL);
>  				control |= M2M_CONTROL_START;
> @@ -867,25 +880,22 @@ static dma_cookie_t ep93xx_dma_tx_submit(struct dma_async_tx_descriptor *tx)
>  static int ep93xx_dma_alloc_chan_resources(struct dma_chan *chan)
>  {
>  	struct ep93xx_dma_chan *edmac = to_ep93xx_dma_chan(chan);
> -	struct ep93xx_dma_data *data = chan->private;
>  	const char *name = dma_chan_name(chan);
>  	int ret, i;
>  
>  	/* Sanity check the channel parameters */
>  	if (!edmac->edma->m2m) {
> -		if (!data)
> +		if (edmac->dma_cfg.port < EP93XX_DMA_I2S1 ||
> +		    edmac->dma_cfg.port > EP93XX_DMA_IRDA)
>  			return -EINVAL;
> -		if (data->port < EP93XX_DMA_I2S1 ||
> -		    data->port > EP93XX_DMA_IRDA)
> -			return -EINVAL;
> -		if (data->direction != ep93xx_dma_chan_direction(chan))
> +		if (edmac->dma_cfg.dir != ep93xx_dma_chan_direction(chan))
>  			return -EINVAL;
>  	} else {
> -		if (data) {
> -			switch (data->port) {
> +		if (edmac->dma_cfg.dir != DMA_MEM_TO_MEM) {
> +			switch (edmac->dma_cfg.port) {
>  			case EP93XX_DMA_SSP:
>  			case EP93XX_DMA_IDE:
> -				if (!is_slave_direction(data->direction))
> +				if (!is_slave_direction(edmac->dma_cfg.dir))
>  					return -EINVAL;
>  				break;
>  			default:
> @@ -894,9 +904,6 @@ static int ep93xx_dma_alloc_chan_resources(struct dma_chan *chan)
>  		}
>  	}
>  
> -	if (data && data->name)
> -		name = data->name;
> -
>  	ret = clk_prepare_enable(edmac->clk);
>  	if (ret)
>  		return ret;
> @@ -1315,36 +1322,53 @@ static void ep93xx_dma_issue_pending(struct dma_chan *chan)
>  	ep93xx_dma_advance_work(to_ep93xx_dma_chan(chan));
>  }
>  
> -static int __init ep93xx_dma_probe(struct platform_device *pdev)
> +static struct ep93xx_dma_engine *ep93xx_dma_of_probe(struct platform_device *pdev)
>  {
> -	struct ep93xx_dma_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	const struct ep93xx_edma_data *data;
> +	struct device *dev = &pdev->dev;
>  	struct ep93xx_dma_engine *edma;
>  	struct dma_device *dma_dev;
> -	int ret, i;
> +	char dma_clk_name[5];
> +	int i;
>  
> -	edma = kzalloc(struct_size(edma, channels, pdata->num_channels), GFP_KERNEL);
> +	data = device_get_match_data(dev);
> +	if (!data)
> +		return ERR_PTR(dev_err_probe(dev, -ENODEV, "No device match found\n"));
> +
> +	edma = devm_kzalloc(dev, struct_size(edma, channels, data->num_channels),
> +			    GFP_KERNEL);
>  	if (!edma)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
> +	edma->m2m = data->id;
> +	edma->num_channels = data->num_channels;
>  	dma_dev = &edma->dma_dev;
> -	edma->m2m = platform_get_device_id(pdev)->driver_data;
> -	edma->num_channels = pdata->num_channels;
>  
>  	INIT_LIST_HEAD(&dma_dev->channels);
> -	for (i = 0; i < pdata->num_channels; i++) {
> -		const struct ep93xx_dma_chan_data *cdata = &pdata->channels[i];
> +	for (i = 0; i < edma->num_channels; i++) {
>  		struct ep93xx_dma_chan *edmac = &edma->channels[i];
>  
>  		edmac->chan.device = dma_dev;
> -		edmac->regs = cdata->base;
> -		edmac->irq = cdata->irq;
> +		edmac->regs = devm_platform_ioremap_resource(pdev, i);
> +		if (IS_ERR(edmac->regs))
> +			return edmac->regs;
> +
> +		edmac->irq = fwnode_irq_get(dev_fwnode(dev), i);
> +		if (edmac->irq < 0)
> +			return ERR_PTR(edmac->irq);
> +
>  		edmac->edma = edma;
>  
> -		edmac->clk = clk_get(NULL, cdata->name);
> +		if (edma->m2m)
> +			sprintf(dma_clk_name, "m2m%u", i);
> +		else
> +			sprintf(dma_clk_name, "m2p%u", i);
> +
> +		edmac->clk = devm_clk_get(dev, dma_clk_name);
>  		if (IS_ERR(edmac->clk)) {
> -			dev_warn(&pdev->dev, "failed to get clock for %s\n",
> -				 cdata->name);
> -			continue;
> +			dev_err_probe(dev, PTR_ERR(edmac->clk),
> +				      "no %s clock found\n", dma_clk_name);
> +			return ERR_CAST(edmac->clk);
>  		}
>  
>  		spin_lock_init(&edmac->lock);
> @@ -1357,6 +1381,90 @@ static int __init ep93xx_dma_probe(struct platform_device *pdev)
>  			      &dma_dev->channels);
>  	}
>  
> +	return edma;
> +}
> +
> +static bool ep93xx_m2p_dma_filter(struct dma_chan *chan, void *filter_param)
> +{
> +	struct ep93xx_dma_chan *echan = to_ep93xx_dma_chan(chan);
> +	struct ep93xx_dma_chan_cfg *cfg = filter_param;
> +
> +	if (cfg->dir != ep93xx_dma_chan_direction(chan))
> +		return false;
> +
> +	echan->dma_cfg = *cfg;
> +	return true;
> +}
> +
> +static struct dma_chan *ep93xx_m2p_dma_of_xlate(struct of_phandle_args *dma_spec,
> +					    struct of_dma *ofdma)
> +{
> +	struct ep93xx_dma_engine *edma = ofdma->of_dma_data;
> +	dma_cap_mask_t mask = edma->dma_dev.cap_mask;
> +	struct ep93xx_dma_chan_cfg dma_cfg;
> +	u8 port = dma_spec->args[0];
> +	u8 direction = dma_spec->args[1];
> +
> +	if (port > EP93XX_DMA_IRDA)
> +		return NULL;
> +
> +	if (!is_slave_direction(direction))
> +		return NULL;
> +
> +	dma_cfg.port = port;
> +	dma_cfg.dir = direction;
> +
> +	return __dma_request_channel(&mask, ep93xx_m2p_dma_filter, &dma_cfg, ofdma->of_node);
> +}
> +
> +static bool ep93xx_m2m_dma_filter(struct dma_chan *chan, void *filter_param)
> +{
> +	struct ep93xx_dma_chan *echan = to_ep93xx_dma_chan(chan);
> +	struct ep93xx_dma_chan_cfg *cfg = filter_param;
> +
> +	echan->dma_cfg = *cfg;
> +
> +	return true;
> +}
> +
> +static struct dma_chan *ep93xx_m2m_dma_of_xlate(struct of_phandle_args *dma_spec,
> +					    struct of_dma *ofdma)
> +{
> +	struct ep93xx_dma_engine *edma = ofdma->of_dma_data;
> +	dma_cap_mask_t mask = edma->dma_dev.cap_mask;
> +	struct ep93xx_dma_chan_cfg dma_cfg;
> +	u8 port = dma_spec->args[0];
> +	u8 direction = dma_spec->args[1];
> +
> +	if (!is_slave_direction(direction))
> +		return NULL;
> +
> +	switch (port) {
> +	case EP93XX_DMA_SSP:
> +	case EP93XX_DMA_IDE:
> +		break;
> +	default:
> +		return NULL;
> +	}
> +
> +	dma_cfg.port = port;
> +	dma_cfg.dir = direction;
> +
> +	return __dma_request_channel(&mask, ep93xx_m2m_dma_filter, &dma_cfg, ofdma->of_node);
> +}
> +
> +static int ep93xx_dma_probe(struct platform_device *pdev)
> +{
> +	struct ep93xx_dma_engine *edma;
> +	struct dma_device *dma_dev;
> +	int ret;
> +
> +	edma = ep93xx_dma_of_probe(pdev);
> +	if (!edma)
> +		return PTR_ERR(edma);
> +
> +	dma_dev = &edma->dma_dev;
> +
>  	dma_cap_zero(dma_dev->cap_mask);
>  	dma_cap_set(DMA_SLAVE, dma_dev->cap_mask);
>  	dma_cap_set(DMA_CYCLIC, dma_dev->cap_mask);
> @@ -1393,21 +1501,46 @@ static int __init ep93xx_dma_probe(struct platform_device *pdev)
>  	}
>  
>  	ret = dma_async_device_register(dma_dev);
> -	if (unlikely(ret)) {
> -		for (i = 0; i < edma->num_channels; i++) {
> -			struct ep93xx_dma_chan *edmac = &edma->channels[i];
> -			if (!IS_ERR_OR_NULL(edmac->clk))
> -				clk_put(edmac->clk);
> -		}
> -		kfree(edma);
> +	if (ret)
> +		return ret;
> +
> +	if (edma->m2m) {
> +		ret = of_dma_controller_register(pdev->dev.of_node, ep93xx_m2m_dma_of_xlate,
> +						 edma);
>  	} else {
> -		dev_info(dma_dev->dev, "EP93xx M2%s DMA ready\n",
> -			 edma->m2m ? "M" : "P");
> +		ret = of_dma_controller_register(pdev->dev.of_node, ep93xx_m2p_dma_of_xlate,
> +						 edma);
>  	}
> +	if (ret)
> +		goto err_dma_unregister;
> +
> +	dev_info(dma_dev->dev, "EP93xx M2%s DMA ready\n", edma->m2m ? "M" : "P");
> +
> +	return 0;
> +
> +err_dma_unregister:
> +	dma_async_device_unregister(dma_dev);
>  
>  	return ret;
>  }
>  
> +static const struct ep93xx_edma_data edma_m2p = {
> +	.id = M2P_DMA,
> +	.num_channels = 10,
> +};
> +
> +static const struct ep93xx_edma_data edma_m2m = {
> +	.id = M2M_DMA,
> +	.num_channels = 2,
> +};
> +
> +static const struct of_device_id ep93xx_dma_of_ids[] = {
> +	{ .compatible = "cirrus,ep9301-dma-m2p", .data = &edma_m2p },
> +	{ .compatible = "cirrus,ep9301-dma-m2m", .data = &edma_m2m },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ep93xx_dma_of_ids);
> +
>  static const struct platform_device_id ep93xx_dma_driver_ids[] = {
>  	{ "ep93xx-dma-m2p", 0 },
>  	{ "ep93xx-dma-m2m", 1 },
> @@ -1417,15 +1550,13 @@ static const struct platform_device_id ep93xx_dma_driver_ids[] = {
>  static struct platform_driver ep93xx_dma_driver = {
>  	.driver		= {
>  		.name	= "ep93xx-dma",
> +		.of_match_table = ep93xx_dma_of_ids,
>  	},
>  	.id_table	= ep93xx_dma_driver_ids,
> +	.probe		= ep93xx_dma_probe,
>  };
>  
> -static int __init ep93xx_dma_module_init(void)
> -{
> -	return platform_driver_probe(&ep93xx_dma_driver, ep93xx_dma_probe);
> -}
> -subsys_initcall(ep93xx_dma_module_init);
> +module_platform_driver(ep93xx_dma_driver);
>  
>  MODULE_AUTHOR("Mika Westerberg <[e-mail]>");
>  MODULE_DESCRIPTION("EP93xx DMA driver");
> diff --git a/include/linux/platform_data/dma-ep93xx.h b/include/linux/platform_data/dma-ep93xx.h
> index eb9805bb3fe8..9ec5cdd5a1eb 100644
> --- a/include/linux/platform_data/dma-ep93xx.h
> +++ b/include/linux/platform_data/dma-ep93xx.h
> @@ -3,8 +3,11 @@
>  #define __ASM_ARCH_DMA_H
>  
>  #include <linux/types.h>
> +#include <linux/device.h>
>  #include <linux/dmaengine.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/property.h>
> +#include <linux/string.h>
>  
>  /*
>   * M2P channels.
> @@ -70,6 +73,9 @@ struct ep93xx_dma_platform_data {
>  
>  static inline bool ep93xx_dma_chan_is_m2p(struct dma_chan *chan)
>  {
> +	if (device_is_compatible(chan->device->dev, "cirrus,ep9301-dma-m2p"))
> +		return true;
> +
>  	return !strcmp(dev_name(chan->device->dev), "ep93xx-dma-m2p");
>  }
>  
> 
> -- 
> 2.41.0
>

Alexander Sverdlin March 28, 2024, 7:21 a.m. UTC | #2

Hello Vinod!

On Thu, 2024-03-28 at 10:01 +0530, Vinod Koul wrote:
> On 26-03-24, 12:18, Nikita Shubin via B4 Relay wrote:
> > From: Nikita Shubin <[e-mail]>
> > 
> > Convert Cirrus EP93xx DMA to device tree usage:
> 
> Subsytem is dmaengine: pls fix that
> 
> > 
> > - add OF ID match table with data
> > - add of_probe for device tree
> > - add xlate for m2m/m2p
> > - drop subsys_initcall code
> > - drop platform probe
> > - drop platform structs usage
> > 
> > > From now on it only supports device tree probing.
> > 
> > Co-developed-by: Alexander Sverdlin <[e-mail]>
> > Signed-off-by: Alexander Sverdlin <[e-mail]>
> > Signed-off-by: Nikita Shubin <[e-mail]>
> > ---
> >   drivers/dma/ep93xx_dma.c                 | 239 ++++++++++++++++++++++++-------
> >   include/linux/platform_data/dma-ep93xx.h |   6 +
> >   2 files changed, 191 insertions(+), 54 deletions(-)
> > 
> > diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
> > index d6c60635e90d..17c8e2badee2 100644
> > --- a/drivers/dma/ep93xx_dma.c
> > +++ b/drivers/dma/ep93xx_dma.c

[...]

> >   
> > @@ -104,6 +106,11 @@
> >   #define DMA_MAX_CHAN_BYTES		0xffff
> >   #define DMA_MAX_CHAN_DESCRIPTORS	32
> >   
> > +enum ep93xx_dma_type {
> > +	M2P_DMA,
> 
> Is this missing P2M?

Not really. It's not the most obvious one, but anyway a way to enumerate
two types of DMA engines:

"7.1.1 DMA Features List
DMA specific features are:
• Ten fully independent, programmable DMA controller internal M2P/P2M
channels (5 Tx and 5 Rx).
• Two dedicated channels for Memory-to-Memory (M2M) and
Memory-to-External Peripheral Transfers (external M2P/P2M)."

Now the confusing part is that this "M2M" engine is actually used
to transfer to and from *some* devices, like SPI and IDE.
So both engines are capable of M2P and P2M, maybe Cirrus has named
two engines in a sub-optimal way decades ago and this is now a bit
historical naming.

> > +	M2M_DMA,
> > +};

Nikita Shubin March 28, 2024, 7:24 a.m. UTC | #3

Hello Vinod!

Thank you for looking into this.

On Thu, 2024-03-28 at 10:01 +0530, Vinod Koul wrote:
> On 26-03-24, 12:18, Nikita Shubin via B4 Relay wrote:
> > From: Nikita Shubin <[e-mail]>
> > 
> > +enum ep93xx_dma_type {
> > +       M2P_DMA,
> 
> Is this missing P2M?
> 
> > +       M2M_DMA,
> > +};
> > +

These are internal types used only to distinguish M2P/P2M and M2M
capable controllers in "of_device_id ep93xx_dma_of_ids[]".

So M2P_DMA is M2P/P2M, a can rename M2P_DMA to M2P_P2M_DMA to avoid
confusion.


> >  struct ep93xx_dma_engine;
> >  static int ep93xx_dma_slave_config_write(struct dma_chan *chan,
> >                                          enum
> > dma_transfer_direction dir,
> > @@ -129,11 +136,17 @@ struct ep93xx_dma_desc {
> >         struct list_head                node;
> >  };
> >  
> > +struct ep93xx_dma_chan_cfg {
> > +       u8                              port;
> > +       enum dma_transfer_direction     dir;
> 
> Why is direction stored here, it should be derived from the prep_xxx
> call, that has direction as an argument
> 
> 

M2P/P2M channels aren't unidirectional.

Citing "EP9xx User Guide":

"Ten fully independent, programmable DMA controller internal M2P/P2M
channels (5 Tx and 5 Rx)."

We need to return correct channel based on Device Tree provided data,
because we need direction in device_alloc_chan_resources() for hardware
setup before prepping.

May be i am mistaking somewhere.

Vinod Koul March 28, 2024, 7:27 a.m. UTC | #4

On 28-03-24, 08:21, Alexander Sverdlin wrote:
> Hello Vinod!
> 
> On Thu, 2024-03-28 at 10:01 +0530, Vinod Koul wrote:
> > On 26-03-24, 12:18, Nikita Shubin via B4 Relay wrote:
> > > From: Nikita Shubin <[e-mail]>
> > > 
> > > Convert Cirrus EP93xx DMA to device tree usage:
> > 
> > Subsytem is dmaengine: pls fix that
> > 
> > > 
> > > - add OF ID match table with data
> > > - add of_probe for device tree
> > > - add xlate for m2m/m2p
> > > - drop subsys_initcall code
> > > - drop platform probe
> > > - drop platform structs usage
> > > 
> > > > From now on it only supports device tree probing.
> > > 
> > > Co-developed-by: Alexander Sverdlin <[e-mail]>
> > > Signed-off-by: Alexander Sverdlin <[e-mail]>
> > > Signed-off-by: Nikita Shubin <[e-mail]>
> > > ---
> > >   drivers/dma/ep93xx_dma.c                 | 239 ++++++++++++++++++++++++-------
> > >   include/linux/platform_data/dma-ep93xx.h |   6 +
> > >   2 files changed, 191 insertions(+), 54 deletions(-)
> > > 
> > > diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
> > > index d6c60635e90d..17c8e2badee2 100644
> > > --- a/drivers/dma/ep93xx_dma.c
> > > +++ b/drivers/dma/ep93xx_dma.c
> 
> [...]
> 
> > >   
> > > @@ -104,6 +106,11 @@
> > >   #define DMA_MAX_CHAN_BYTES		0xffff
> > >   #define DMA_MAX_CHAN_DESCRIPTORS	32
> > >   
> > > +enum ep93xx_dma_type {
> > > +	M2P_DMA,
> > 
> > Is this missing P2M?
> 
> Not really. It's not the most obvious one, but anyway a way to enumerate
> two types of DMA engines:
> 
> "7.1.1 DMA Features List
> DMA specific features are:
> • Ten fully independent, programmable DMA controller internal M2P/P2M
> channels (5 Tx and 5 Rx).
> • Two dedicated channels for Memory-to-Memory (M2M) and
> Memory-to-External Peripheral Transfers (external M2P/P2M)."
> 
> Now the confusing part is that this "M2M" engine is actually used
> to transfer to and from *some* devices, like SPI and IDE.
> So both engines are capable of M2P and P2M, maybe Cirrus has named
> two engines in a sub-optimal way decades ago and this is now a bit
> historical naming.

Okay, thanks for clarifying (might be worthwhile to mention this in
comments)

Vinod Koul March 28, 2024, 7:28 a.m. UTC | #5

On 28-03-24, 10:24, Nikita Shubin wrote:
> Hello Vinod!
> 
> Thank you for looking into this.
> 
> On Thu, 2024-03-28 at 10:01 +0530, Vinod Koul wrote:
> > On 26-03-24, 12:18, Nikita Shubin via B4 Relay wrote:
> > > From: Nikita Shubin <[e-mail]>
> > > 
> > > +enum ep93xx_dma_type {
> > > +       M2P_DMA,
> > 
> > Is this missing P2M?
> > 
> > > +       M2M_DMA,
> > > +};
> > > +
> 
> These are internal types used only to distinguish M2P/P2M and M2M
> capable controllers in "of_device_id ep93xx_dma_of_ids[]".
> 
> So M2P_DMA is M2P/P2M, a can rename M2P_DMA to M2P_P2M_DMA to avoid
> confusion.
> 
> 
> > >  struct ep93xx_dma_engine;
> > >  static int ep93xx_dma_slave_config_write(struct dma_chan *chan,
> > >                                          enum
> > > dma_transfer_direction dir,
> > > @@ -129,11 +136,17 @@ struct ep93xx_dma_desc {
> > >         struct list_head                node;
> > >  };
> > >  
> > > +struct ep93xx_dma_chan_cfg {
> > > +       u8                              port;
> > > +       enum dma_transfer_direction     dir;
> > 
> > Why is direction stored here, it should be derived from the prep_xxx
> > call, that has direction as an argument
> > 
> > 
> 
> M2P/P2M channels aren't unidirectional.
> 
> Citing "EP9xx User Guide":
> 
> "Ten fully independent, programmable DMA controller internal M2P/P2M
> channels (5 Tx and 5 Rx)."
> 
> We need to return correct channel based on Device Tree provided data,
> because we need direction in device_alloc_chan_resources() for hardware
> setup before prepping.

Okay it sounds okay in that case...

> 
> May be i am mistaking somewhere.

Patch

diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index d6c60635e90d..17c8e2badee2 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -20,6 +20,8 @@ 
 #include <linux/dmaengine.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
+#include <linux/of_dma.h>
+#include <linux/overflow.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
@@ -104,6 +106,11 @@ 
 #define DMA_MAX_CHAN_BYTES		0xffff
 #define DMA_MAX_CHAN_DESCRIPTORS	32
 
+enum ep93xx_dma_type {
+	M2P_DMA,
+	M2M_DMA,
+};
+
 struct ep93xx_dma_engine;
 static int ep93xx_dma_slave_config_write(struct dma_chan *chan,
 					 enum dma_transfer_direction dir,
@@ -129,11 +136,17 @@  struct ep93xx_dma_desc {
 	struct list_head		node;
 };
 
+struct ep93xx_dma_chan_cfg {
+	u8				port;
+	enum dma_transfer_direction	dir;
+};
+
 /**
  * struct ep93xx_dma_chan - an EP93xx DMA M2P/M2M channel
  * @chan: dmaengine API channel
  * @edma: pointer to the engine device
  * @regs: memory mapped registers
+ * @dma_cfg: channel number, direction
  * @irq: interrupt number of the channel
  * @clk: clock used by this channel
  * @tasklet: channel specific tasklet used for callbacks
@@ -157,14 +170,12 @@  struct ep93xx_dma_desc {
  * descriptor in the chain. When a descriptor is moved to the @active queue,
  * the first and chained descriptors are flattened into a single list.
  *
- * @chan.private holds pointer to &struct ep93xx_dma_data which contains
- * necessary channel configuration information. For memcpy channels this must
- * be %NULL.
  */
 struct ep93xx_dma_chan {
 	struct dma_chan			chan;
 	const struct ep93xx_dma_engine	*edma;
 	void __iomem			*regs;
+	struct ep93xx_dma_chan_cfg	dma_cfg;
 	int				irq;
 	struct clk			*clk;
 	struct tasklet_struct		tasklet;
@@ -216,6 +227,11 @@  struct ep93xx_dma_engine {
 	struct ep93xx_dma_chan	channels[] __counted_by(num_channels);
 };
 
+struct ep93xx_edma_data {
+	u32	id;
+	size_t	num_channels;
+};
+
 static inline struct device *chan2dev(struct ep93xx_dma_chan *edmac)
 {
 	return &edmac->chan.dev->device;
@@ -318,10 +334,9 @@  static void m2p_set_control(struct ep93xx_dma_chan *edmac, u32 control)
 
 static int m2p_hw_setup(struct ep93xx_dma_chan *edmac)
 {
-	struct ep93xx_dma_data *data = edmac->chan.private;
 	u32 control;
 
-	writel(data->port & 0xf, edmac->regs + M2P_PPALLOC);
+	writel(edmac->dma_cfg.port & 0xf, edmac->regs + M2P_PPALLOC);
 
 	control = M2P_CONTROL_CH_ERROR_INT | M2P_CONTROL_ICE
 		| M2P_CONTROL_ENABLE;
@@ -458,16 +473,15 @@  static int m2p_hw_interrupt(struct ep93xx_dma_chan *edmac)
 
 static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
 {
-	const struct ep93xx_dma_data *data = edmac->chan.private;
 	u32 control = 0;
 
-	if (!data) {
+	if (edmac->dma_cfg.dir == DMA_MEM_TO_MEM) {
 		/* This is memcpy channel, nothing to configure */
 		writel(control, edmac->regs + M2M_CONTROL);
 		return 0;
 	}
 
-	switch (data->port) {
+	switch (edmac->dma_cfg.port) {
 	case EP93XX_DMA_SSP:
 		/*
 		 * This was found via experimenting - anything less than 5
@@ -477,7 +491,7 @@  static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
 		control = (5 << M2M_CONTROL_PWSC_SHIFT);
 		control |= M2M_CONTROL_NO_HDSK;
 
-		if (data->direction == DMA_MEM_TO_DEV) {
+		if (edmac->dma_cfg.dir == DMA_MEM_TO_DEV) {
 			control |= M2M_CONTROL_DAH;
 			control |= M2M_CONTROL_TM_TX;
 			control |= M2M_CONTROL_RSS_SSPTX;
@@ -493,7 +507,7 @@  static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
 		 * This IDE part is totally untested. Values below are taken
 		 * from the EP93xx Users's Guide and might not be correct.
 		 */
-		if (data->direction == DMA_MEM_TO_DEV) {
+		if (edmac->dma_cfg.dir == DMA_MEM_TO_DEV) {
 			/* Worst case from the UG */
 			control = (3 << M2M_CONTROL_PWSC_SHIFT);
 			control |= M2M_CONTROL_DAH;
@@ -548,7 +562,6 @@  static void m2m_fill_desc(struct ep93xx_dma_chan *edmac)
 
 static void m2m_hw_submit(struct ep93xx_dma_chan *edmac)
 {
-	struct ep93xx_dma_data *data = edmac->chan.private;
 	u32 control = readl(edmac->regs + M2M_CONTROL);
 
 	/*
@@ -574,7 +587,7 @@  static void m2m_hw_submit(struct ep93xx_dma_chan *edmac)
 	control |= M2M_CONTROL_ENABLE;
 	writel(control, edmac->regs + M2M_CONTROL);
 
-	if (!data) {
+	if (edmac->dma_cfg.dir == DMA_MEM_TO_MEM) {
 		/*
 		 * For memcpy channels the software trigger must be asserted
 		 * in order to start the memcpy operation.
@@ -636,7 +649,7 @@  static int m2m_hw_interrupt(struct ep93xx_dma_chan *edmac)
 		 */
 		if (ep93xx_dma_advance_active(edmac)) {
 			m2m_fill_desc(edmac);
-			if (done && !edmac->chan.private) {
+			if (done && edmac->dma_cfg.dir == DMA_MEM_TO_MEM) {
 				/* Software trigger for memcpy channel */
 				control = readl(edmac->regs + M2M_CONTROL);
 				control |= M2M_CONTROL_START;
@@ -867,25 +880,22 @@  static dma_cookie_t ep93xx_dma_tx_submit(struct dma_async_tx_descriptor *tx)
 static int ep93xx_dma_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct ep93xx_dma_chan *edmac = to_ep93xx_dma_chan(chan);
-	struct ep93xx_dma_data *data = chan->private;
 	const char *name = dma_chan_name(chan);
 	int ret, i;
 
 	/* Sanity check the channel parameters */
 	if (!edmac->edma->m2m) {
-		if (!data)
+		if (edmac->dma_cfg.port < EP93XX_DMA_I2S1 ||
+		    edmac->dma_cfg.port > EP93XX_DMA_IRDA)
 			return -EINVAL;
-		if (data->port < EP93XX_DMA_I2S1 ||
-		    data->port > EP93XX_DMA_IRDA)
-			return -EINVAL;
-		if (data->direction != ep93xx_dma_chan_direction(chan))
+		if (edmac->dma_cfg.dir != ep93xx_dma_chan_direction(chan))
 			return -EINVAL;
 	} else {
-		if (data) {
-			switch (data->port) {
+		if (edmac->dma_cfg.dir != DMA_MEM_TO_MEM) {
+			switch (edmac->dma_cfg.port) {
 			case EP93XX_DMA_SSP:
 			case EP93XX_DMA_IDE:
-				if (!is_slave_direction(data->direction))
+				if (!is_slave_direction(edmac->dma_cfg.dir))
 					return -EINVAL;
 				break;
 			default:
@@ -894,9 +904,6 @@  static int ep93xx_dma_alloc_chan_resources(struct dma_chan *chan)
 		}
 	}
 
-	if (data && data->name)
-		name = data->name;
-
 	ret = clk_prepare_enable(edmac->clk);
 	if (ret)
 		return ret;
@@ -1315,36 +1322,53 @@  static void ep93xx_dma_issue_pending(struct dma_chan *chan)
 	ep93xx_dma_advance_work(to_ep93xx_dma_chan(chan));
 }
 
-static int __init ep93xx_dma_probe(struct platform_device *pdev)
+static struct ep93xx_dma_engine *ep93xx_dma_of_probe(struct platform_device *pdev)
 {
-	struct ep93xx_dma_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	const struct ep93xx_edma_data *data;
+	struct device *dev = &pdev->dev;
 	struct ep93xx_dma_engine *edma;
 	struct dma_device *dma_dev;
-	int ret, i;
+	char dma_clk_name[5];
+	int i;
 
-	edma = kzalloc(struct_size(edma, channels, pdata->num_channels), GFP_KERNEL);
+	data = device_get_match_data(dev);
+	if (!data)
+		return ERR_PTR(dev_err_probe(dev, -ENODEV, "No device match found\n"));
+
+	edma = devm_kzalloc(dev, struct_size(edma, channels, data->num_channels),
+			    GFP_KERNEL);
 	if (!edma)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
+	edma->m2m = data->id;
+	edma->num_channels = data->num_channels;
 	dma_dev = &edma->dma_dev;
-	edma->m2m = platform_get_device_id(pdev)->driver_data;
-	edma->num_channels = pdata->num_channels;
 
 	INIT_LIST_HEAD(&dma_dev->channels);
-	for (i = 0; i < pdata->num_channels; i++) {
-		const struct ep93xx_dma_chan_data *cdata = &pdata->channels[i];
+	for (i = 0; i < edma->num_channels; i++) {
 		struct ep93xx_dma_chan *edmac = &edma->channels[i];
 
 		edmac->chan.device = dma_dev;
-		edmac->regs = cdata->base;
-		edmac->irq = cdata->irq;
+		edmac->regs = devm_platform_ioremap_resource(pdev, i);
+		if (IS_ERR(edmac->regs))
+			return edmac->regs;
+
+		edmac->irq = fwnode_irq_get(dev_fwnode(dev), i);
+		if (edmac->irq < 0)
+			return ERR_PTR(edmac->irq);
+
 		edmac->edma = edma;
 
-		edmac->clk = clk_get(NULL, cdata->name);
+		if (edma->m2m)
+			sprintf(dma_clk_name, "m2m%u", i);
+		else
+			sprintf(dma_clk_name, "m2p%u", i);
+
+		edmac->clk = devm_clk_get(dev, dma_clk_name);
 		if (IS_ERR(edmac->clk)) {
-			dev_warn(&pdev->dev, "failed to get clock for %s\n",
-				 cdata->name);
-			continue;
+			dev_err_probe(dev, PTR_ERR(edmac->clk),
+				      "no %s clock found\n", dma_clk_name);
+			return ERR_CAST(edmac->clk);
 		}
 
 		spin_lock_init(&edmac->lock);
@@ -1357,6 +1381,90 @@  static int __init ep93xx_dma_probe(struct platform_device *pdev)
 			      &dma_dev->channels);
 	}
 
+	return edma;
+}
+
+static bool ep93xx_m2p_dma_filter(struct dma_chan *chan, void *filter_param)
+{
+	struct ep93xx_dma_chan *echan = to_ep93xx_dma_chan(chan);
+	struct ep93xx_dma_chan_cfg *cfg = filter_param;
+
+	if (cfg->dir != ep93xx_dma_chan_direction(chan))
+		return false;
+
+	echan->dma_cfg = *cfg;
+	return true;
+}
+
+static struct dma_chan *ep93xx_m2p_dma_of_xlate(struct of_phandle_args *dma_spec,
+					    struct of_dma *ofdma)
+{
+	struct ep93xx_dma_engine *edma = ofdma->of_dma_data;
+	dma_cap_mask_t mask = edma->dma_dev.cap_mask;
+	struct ep93xx_dma_chan_cfg dma_cfg;
+	u8 port = dma_spec->args[0];
+	u8 direction = dma_spec->args[1];
+
+	if (port > EP93XX_DMA_IRDA)
+		return NULL;
+
+	if (!is_slave_direction(direction))
+		return NULL;
+
+	dma_cfg.port = port;
+	dma_cfg.dir = direction;
+
+	return __dma_request_channel(&mask, ep93xx_m2p_dma_filter, &dma_cfg, ofdma->of_node);
+}
+
+static bool ep93xx_m2m_dma_filter(struct dma_chan *chan, void *filter_param)
+{
+	struct ep93xx_dma_chan *echan = to_ep93xx_dma_chan(chan);
+	struct ep93xx_dma_chan_cfg *cfg = filter_param;
+
+	echan->dma_cfg = *cfg;
+
+	return true;
+}
+
+static struct dma_chan *ep93xx_m2m_dma_of_xlate(struct of_phandle_args *dma_spec,
+					    struct of_dma *ofdma)
+{
+	struct ep93xx_dma_engine *edma = ofdma->of_dma_data;
+	dma_cap_mask_t mask = edma->dma_dev.cap_mask;
+	struct ep93xx_dma_chan_cfg dma_cfg;
+	u8 port = dma_spec->args[0];
+	u8 direction = dma_spec->args[1];
+
+	if (!is_slave_direction(direction))
+		return NULL;
+
+	switch (port) {
+	case EP93XX_DMA_SSP:
+	case EP93XX_DMA_IDE:
+		break;
+	default:
+		return NULL;
+	}
+
+	dma_cfg.port = port;
+	dma_cfg.dir = direction;
+
+	return __dma_request_channel(&mask, ep93xx_m2m_dma_filter, &dma_cfg, ofdma->of_node);
+}
+
+static int ep93xx_dma_probe(struct platform_device *pdev)
+{
+	struct ep93xx_dma_engine *edma;
+	struct dma_device *dma_dev;
+	int ret;
+
+	edma = ep93xx_dma_of_probe(pdev);
+	if (!edma)
+		return PTR_ERR(edma);
+
+	dma_dev = &edma->dma_dev;
+
 	dma_cap_zero(dma_dev->cap_mask);
 	dma_cap_set(DMA_SLAVE, dma_dev->cap_mask);
 	dma_cap_set(DMA_CYCLIC, dma_dev->cap_mask);
@@ -1393,21 +1501,46 @@  static int __init ep93xx_dma_probe(struct platform_device *pdev)
 	}
 
 	ret = dma_async_device_register(dma_dev);
-	if (unlikely(ret)) {
-		for (i = 0; i < edma->num_channels; i++) {
-			struct ep93xx_dma_chan *edmac = &edma->channels[i];
-			if (!IS_ERR_OR_NULL(edmac->clk))
-				clk_put(edmac->clk);
-		}
-		kfree(edma);
+	if (ret)
+		return ret;
+
+	if (edma->m2m) {
+		ret = of_dma_controller_register(pdev->dev.of_node, ep93xx_m2m_dma_of_xlate,
+						 edma);
 	} else {
-		dev_info(dma_dev->dev, "EP93xx M2%s DMA ready\n",
-			 edma->m2m ? "M" : "P");
+		ret = of_dma_controller_register(pdev->dev.of_node, ep93xx_m2p_dma_of_xlate,
+						 edma);
 	}
+	if (ret)
+		goto err_dma_unregister;
+
+	dev_info(dma_dev->dev, "EP93xx M2%s DMA ready\n", edma->m2m ? "M" : "P");
+
+	return 0;
+
+err_dma_unregister:
+	dma_async_device_unregister(dma_dev);
 
 	return ret;
 }
 
+static const struct ep93xx_edma_data edma_m2p = {
+	.id = M2P_DMA,
+	.num_channels = 10,
+};
+
+static const struct ep93xx_edma_data edma_m2m = {
+	.id = M2M_DMA,
+	.num_channels = 2,
+};
+
+static const struct of_device_id ep93xx_dma_of_ids[] = {
+	{ .compatible = "cirrus,ep9301-dma-m2p", .data = &edma_m2p },
+	{ .compatible = "cirrus,ep9301-dma-m2m", .data = &edma_m2m },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ep93xx_dma_of_ids);
+
 static const struct platform_device_id ep93xx_dma_driver_ids[] = {
 	{ "ep93xx-dma-m2p", 0 },
 	{ "ep93xx-dma-m2m", 1 },
@@ -1417,15 +1550,13 @@  static const struct platform_device_id ep93xx_dma_driver_ids[] = {
 static struct platform_driver ep93xx_dma_driver = {
 	.driver		= {
 		.name	= "ep93xx-dma",
+		.of_match_table = ep93xx_dma_of_ids,
 	},
 	.id_table	= ep93xx_dma_driver_ids,
+	.probe		= ep93xx_dma_probe,
 };
 
-static int __init ep93xx_dma_module_init(void)
-{
-	return platform_driver_probe(&ep93xx_dma_driver, ep93xx_dma_probe);
-}
-subsys_initcall(ep93xx_dma_module_init);
+module_platform_driver(ep93xx_dma_driver);
 
 MODULE_AUTHOR("Mika Westerberg <[e-mail]>");
 MODULE_DESCRIPTION("EP93xx DMA driver");
diff --git a/include/linux/platform_data/dma-ep93xx.h b/include/linux/platform_data/dma-ep93xx.h
index eb9805bb3fe8..9ec5cdd5a1eb 100644
--- a/include/linux/platform_data/dma-ep93xx.h
+++ b/include/linux/platform_data/dma-ep93xx.h
@@ -3,8 +3,11 @@ 
 #define __ASM_ARCH_DMA_H
 
 #include <linux/types.h>
+#include <linux/device.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
+#include <linux/property.h>
+#include <linux/string.h>
 
 /*
  * M2P channels.
@@ -70,6 +73,9 @@  struct ep93xx_dma_platform_data {
 
 static inline bool ep93xx_dma_chan_is_m2p(struct dma_chan *chan)
 {
+	if (device_is_compatible(chan->device->dev, "cirrus,ep9301-dma-m2p"))
+		return true;
+
 	return !strcmp(dev_name(chan->device->dev), "ep93xx-dma-m2p");
 }