[PATCH 1/6 v5] dmaengine: rcar-audmapp: independent from SH_DMAE_BASE v1

[PATCH 1/6 v5] dmaengine: rcar-audmapp: independent from SH_DMAE_BASE v1

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current Renesas Audio DMAC Peri Peri driver is based on
SH_DMAE_BASE driver which is used for Renesas SH-Mobile.
But, basically, SH_DMAE_BASE driver was created for
SuperH SoC, and it is not fully cared for DT.

For example, current SH_DMAE_BASE base driver will return
non-matching DMA channel if some non-SH_DMAE_BASE drivers
are probed.
So, sound driver will get wrong DMA channel
if Audio DMAC (= rcar-dma) was not probed,
but Audio DMAC Peri Peri (= SH_DMAE_BASE) was probed.

OTOH, many SH-Mobile series drivers are using SH_DMAE_BASE
driver, and Renesas R-Car series will not use it anymore.
Maintenance cost for fully cared DT support on SH_DMAE_BASE
will be very high
(and keeping compatibility will be very complex).

In addition, Audio DMAC Peri Peri itself is very simple device,
and, no SoC/board is using it from non-DT environment.

This patch simply removes current rcar-audmapp driver.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v4 -> v5

 - no change

 drivers/dma/sh/Kconfig                         |    6 -
 drivers/dma/sh/Makefile                        |    1 -
 drivers/dma/sh/rcar-audmapp.c                  |  376 ------------------------
 include/linux/platform_data/dma-rcar-audmapp.h |   34 ---
 4 files changed, 417 deletions(-)
 delete mode 100644 drivers/dma/sh/rcar-audmapp.c
 delete mode 100644 include/linux/platform_data/dma-rcar-audmapp.h

diff --git a/drivers/dma/sh/Kconfig b/drivers/dma/sh/Kconfig
index 8190ad2..725f6b4 100644
--- a/drivers/dma/sh/Kconfig
+++ b/drivers/dma/sh/Kconfig
@@ -51,12 +51,6 @@ config RCAR_HPB_DMAE
 	help
 	  Enable support for the Renesas R-Car series DMA controllers.
 
-config RCAR_AUDMAC_PP
-	tristate "Renesas R-Car Audio DMAC Peripheral Peripheral support"
-	depends on SH_DMAE_BASE
-	help
-	  Enable support for the Renesas R-Car Audio DMAC Peripheral Peripheral controllers.
-
 config RCAR_DMAC
 	tristate "Renesas R-Car Gen2 DMA Controller"
 	depends on ARCH_SHMOBILE || COMPILE_TEST
diff --git a/drivers/dma/sh/Makefile b/drivers/dma/sh/Makefile
index 2852f9d..489609b 100644
--- a/drivers/dma/sh/Makefile
+++ b/drivers/dma/sh/Makefile
@@ -15,5 +15,4 @@ obj-$(CONFIG_SH_DMAE) += shdma.o
 
 obj-$(CONFIG_SUDMAC) += sudmac.o
 obj-$(CONFIG_RCAR_HPB_DMAE) += rcar-hpbdma.o
-obj-$(CONFIG_RCAR_AUDMAC_PP) += rcar-audmapp.o
 obj-$(CONFIG_RCAR_DMAC) += rcar-dmac.o
diff --git a/drivers/dma/sh/rcar-audmapp.c b/drivers/dma/sh/rcar-audmapp.c
deleted file mode 100644
index d95bbdd..0000000
--- a/drivers/dma/sh/rcar-audmapp.c
+++ /dev/null
@@ -1,376 +0,0 @@
-/*
- * This is for Renesas R-Car Audio-DMAC-peri-peri.
- *
- * Copyright (C) 2014 Renesas Electronics Corporation
- * Copyright (C) 2014 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
- *
- * based on the drivers/dma/sh/shdma.c
- *
- * Copyright (C) 2011-2012 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
- * Copyright (C) 2009 Nobuhiro Iwamatsu <iwamatsu.nobuhiro@renesas.com>
- * Copyright (C) 2009 Renesas Solutions, Inc. All rights reserved.
- * Copyright (C) 2007 Freescale Semiconductor, Inc. All rights reserved.
- *
- * This is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- */
-#include <linux/delay.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/slab.h>
-#include <linux/dmaengine.h>
-#include <linux/of_dma.h>
-#include <linux/platform_data/dma-rcar-audmapp.h>
-#include <linux/platform_device.h>
-#include <linux/shdma-base.h>
-
-/*
- * DMA register
- */
-#define PDMASAR		0x00
-#define PDMADAR		0x04
-#define PDMACHCR	0x0c
-
-/* PDMACHCR */
-#define PDMACHCR_DE		(1 << 0)
-
-#define AUDMAPP_MAX_CHANNELS	29
-
-/* Default MEMCPY transfer size = 2^2 = 4 bytes */
-#define LOG2_DEFAULT_XFER_SIZE	2
-#define AUDMAPP_SLAVE_NUMBER	256
-#define AUDMAPP_LEN_MAX		(16 * 1024 * 1024)
-
-struct audmapp_chan {
-	struct shdma_chan shdma_chan;
-	void __iomem *base;
-	dma_addr_t slave_addr;
-	u32 chcr;
-};
-
-struct audmapp_device {
-	struct shdma_dev shdma_dev;
-	struct audmapp_pdata *pdata;
-	struct device *dev;
-	void __iomem *chan_reg;
-};
-
-struct audmapp_desc {
-	struct shdma_desc shdma_desc;
-	dma_addr_t src;
-	dma_addr_t dst;
-};
-
-#define to_shdma_chan(c) container_of(c, struct shdma_chan, dma_chan)
-
-#define to_chan(chan) container_of(chan, struct audmapp_chan, shdma_chan)
-#define to_desc(sdesc) container_of(sdesc, struct audmapp_desc, shdma_desc)
-#define to_dev(chan) container_of(chan->shdma_chan.dma_chan.device,	\
-				  struct audmapp_device, shdma_dev.dma_dev)
-
-static void audmapp_write(struct audmapp_chan *auchan, u32 data, u32 reg)
-{
-	struct audmapp_device *audev = to_dev(auchan);
-	struct device *dev = audev->dev;
-
-	dev_dbg(dev, "w %p : %08x\n", auchan->base + reg, data);
-
-	iowrite32(data, auchan->base + reg);
-}
-
-static u32 audmapp_read(struct audmapp_chan *auchan, u32 reg)
-{
-	return ioread32(auchan->base + reg);
-}
-
-static void audmapp_halt(struct shdma_chan *schan)
-{
-	struct audmapp_chan *auchan = to_chan(schan);
-	int i;
-
-	audmapp_write(auchan, 0, PDMACHCR);
-
-	for (i = 0; i < 1024; i++) {
-		if (0 = audmapp_read(auchan, PDMACHCR))
-			return;
-		udelay(1);
-	}
-}
-
-static void audmapp_start_xfer(struct shdma_chan *schan,
-			       struct shdma_desc *sdesc)
-{
-	struct audmapp_chan *auchan = to_chan(schan);
-	struct audmapp_device *audev = to_dev(auchan);
-	struct audmapp_desc *desc = to_desc(sdesc);
-	struct device *dev = audev->dev;
-	u32 chcr = auchan->chcr | PDMACHCR_DE;
-
-	dev_dbg(dev, "src/dst/chcr = %pad/%pad/%08x\n",
-		&desc->src, &desc->dst, chcr);
-
-	audmapp_write(auchan, desc->src,	PDMASAR);
-	audmapp_write(auchan, desc->dst,	PDMADAR);
-	audmapp_write(auchan, chcr,	PDMACHCR);
-}
-
-static int audmapp_get_config(struct audmapp_chan *auchan, int slave_id,
-			      u32 *chcr, dma_addr_t *dst)
-{
-	struct audmapp_device *audev = to_dev(auchan);
-	struct audmapp_pdata *pdata = audev->pdata;
-	struct audmapp_slave_config *cfg;
-	int i;
-
-	*chcr	= 0;
-	*dst	= 0;
-
-	if (!pdata) { /* DT */
-		*chcr = ((u32)slave_id) << 16;
-		auchan->shdma_chan.slave_id = (slave_id) >> 8;
-		return 0;
-	}
-
-	/* non-DT */
-
-	if (slave_id >= AUDMAPP_SLAVE_NUMBER)
-		return -ENXIO;
-
-	for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
-		if (cfg->slave_id = slave_id) {
-			*chcr	= cfg->chcr;
-			*dst	= cfg->dst;
-			return 0;
-		}
-
-	return -ENXIO;
-}
-
-static int audmapp_set_slave(struct shdma_chan *schan, int slave_id,
-			     dma_addr_t slave_addr, bool try)
-{
-	struct audmapp_chan *auchan = to_chan(schan);
-	u32 chcr;
-	dma_addr_t dst;
-	int ret;
-
-	ret = audmapp_get_config(auchan, slave_id, &chcr, &dst);
-	if (ret < 0)
-		return ret;
-
-	if (try)
-		return 0;
-
-	auchan->chcr		= chcr;
-	auchan->slave_addr	= slave_addr ? : dst;
-
-	return 0;
-}
-
-static int audmapp_desc_setup(struct shdma_chan *schan,
-			      struct shdma_desc *sdesc,
-			      dma_addr_t src, dma_addr_t dst, size_t *len)
-{
-	struct audmapp_desc *desc = to_desc(sdesc);
-
-	if (*len > (size_t)AUDMAPP_LEN_MAX)
-		*len = (size_t)AUDMAPP_LEN_MAX;
-
-	desc->src = src;
-	desc->dst = dst;
-
-	return 0;
-}
-
-static void audmapp_setup_xfer(struct shdma_chan *schan,
-			       int slave_id)
-{
-}
-
-static dma_addr_t audmapp_slave_addr(struct shdma_chan *schan)
-{
-	struct audmapp_chan *auchan = to_chan(schan);
-
-	return auchan->slave_addr;
-}
-
-static bool audmapp_channel_busy(struct shdma_chan *schan)
-{
-	struct audmapp_chan *auchan = to_chan(schan);
-	u32 chcr = audmapp_read(auchan, PDMACHCR);
-
-	return chcr & ~PDMACHCR_DE;
-}
-
-static bool audmapp_desc_completed(struct shdma_chan *schan,
-				   struct shdma_desc *sdesc)
-{
-	return true;
-}
-
-static struct shdma_desc *audmapp_embedded_desc(void *buf, int i)
-{
-	return &((struct audmapp_desc *)buf)[i].shdma_desc;
-}
-
-static const struct shdma_ops audmapp_shdma_ops = {
-	.halt_channel	= audmapp_halt,
-	.desc_setup	= audmapp_desc_setup,
-	.set_slave	= audmapp_set_slave,
-	.start_xfer	= audmapp_start_xfer,
-	.embedded_desc	= audmapp_embedded_desc,
-	.setup_xfer	= audmapp_setup_xfer,
-	.slave_addr	= audmapp_slave_addr,
-	.channel_busy	= audmapp_channel_busy,
-	.desc_completed	= audmapp_desc_completed,
-};
-
-static int audmapp_chan_probe(struct platform_device *pdev,
-			      struct audmapp_device *audev, int id)
-{
-	struct shdma_dev *sdev = &audev->shdma_dev;
-	struct audmapp_chan *auchan;
-	struct shdma_chan *schan;
-	struct device *dev = audev->dev;
-
-	auchan = devm_kzalloc(dev, sizeof(*auchan), GFP_KERNEL);
-	if (!auchan)
-		return -ENOMEM;
-
-	schan = &auchan->shdma_chan;
-	schan->max_xfer_len = AUDMAPP_LEN_MAX;
-
-	shdma_chan_probe(sdev, schan, id);
-
-	auchan->base = audev->chan_reg + 0x20 + (0x10 * id);
-	dev_dbg(dev, "%02d : %p / %p", id, auchan->base, audev->chan_reg);
-
-	return 0;
-}
-
-static void audmapp_chan_remove(struct audmapp_device *audev)
-{
-	struct shdma_chan *schan;
-	int i;
-
-	shdma_for_each_chan(schan, &audev->shdma_dev, i) {
-		BUG_ON(!schan);
-		shdma_chan_remove(schan);
-	}
-}
-
-static struct dma_chan *audmapp_of_xlate(struct of_phandle_args *dma_spec,
-					 struct of_dma *ofdma)
-{
-	dma_cap_mask_t mask;
-	struct dma_chan *chan;
-	u32 chcr = dma_spec->args[0];
-
-	if (dma_spec->args_count != 1)
-		return NULL;
-
-	dma_cap_zero(mask);
-	dma_cap_set(DMA_SLAVE, mask);
-
-	chan = dma_request_channel(mask, shdma_chan_filter, NULL);
-	if (chan)
-		to_shdma_chan(chan)->hw_req = chcr;
-
-	return chan;
-}
-
-static int audmapp_probe(struct platform_device *pdev)
-{
-	struct audmapp_pdata *pdata = pdev->dev.platform_data;
-	struct device_node *np = pdev->dev.of_node;
-	struct audmapp_device *audev;
-	struct shdma_dev *sdev;
-	struct dma_device *dma_dev;
-	struct resource *res;
-	int err, i;
-
-	if (np)
-		of_dma_controller_register(np, audmapp_of_xlate, pdev);
-	else if (!pdata)
-		return -ENODEV;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
-	audev = devm_kzalloc(&pdev->dev, sizeof(*audev), GFP_KERNEL);
-	if (!audev)
-		return -ENOMEM;
-
-	audev->dev	= &pdev->dev;
-	audev->pdata	= pdata;
-	audev->chan_reg	= devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(audev->chan_reg))
-		return PTR_ERR(audev->chan_reg);
-
-	sdev		= &audev->shdma_dev;
-	sdev->ops	= &audmapp_shdma_ops;
-	sdev->desc_size	= sizeof(struct audmapp_desc);
-
-	dma_dev			= &sdev->dma_dev;
-	dma_dev->copy_align	= LOG2_DEFAULT_XFER_SIZE;
-	dma_cap_set(DMA_SLAVE, dma_dev->cap_mask);
-
-	err = shdma_init(&pdev->dev, sdev, AUDMAPP_MAX_CHANNELS);
-	if (err < 0)
-		return err;
-
-	platform_set_drvdata(pdev, audev);
-
-	/* Create DMA Channel */
-	for (i = 0; i < AUDMAPP_MAX_CHANNELS; i++) {
-		err = audmapp_chan_probe(pdev, audev, i);
-		if (err)
-			goto chan_probe_err;
-	}
-
-	err = dma_async_device_register(dma_dev);
-	if (err < 0)
-		goto chan_probe_err;
-
-	return err;
-
-chan_probe_err:
-	audmapp_chan_remove(audev);
-	shdma_cleanup(sdev);
-
-	return err;
-}
-
-static int audmapp_remove(struct platform_device *pdev)
-{
-	struct audmapp_device *audev = platform_get_drvdata(pdev);
-	struct dma_device *dma_dev = &audev->shdma_dev.dma_dev;
-
-	dma_async_device_unregister(dma_dev);
-
-	audmapp_chan_remove(audev);
-	shdma_cleanup(&audev->shdma_dev);
-
-	return 0;
-}
-
-static const struct of_device_id audmapp_of_match[] = {
-	{ .compatible = "renesas,rcar-audmapp", },
-	{},
-};
-
-static struct platform_driver audmapp_driver = {
-	.probe		= audmapp_probe,
-	.remove		= audmapp_remove,
-	.driver		= {
-		.name	= "rcar-audmapp-engine",
-		.of_match_table = audmapp_of_match,
-	},
-};
-module_platform_driver(audmapp_driver);
-
-MODULE_AUTHOR("Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>");
-MODULE_DESCRIPTION("Renesas R-Car Audio DMAC peri-peri driver");
-MODULE_LICENSE("GPL");
diff --git a/include/linux/platform_data/dma-rcar-audmapp.h b/include/linux/platform_data/dma-rcar-audmapp.h
deleted file mode 100644
index 471fffe..0000000
--- a/include/linux/platform_data/dma-rcar-audmapp.h
+++ /dev/null
@@ -1,34 +0,0 @@
-/*
- * This is for Renesas R-Car Audio-DMAC-peri-peri.
- *
- * Copyright (C) 2014 Renesas Electronics Corporation
- * Copyright (C) 2014 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
- *
- * This file is based on the include/linux/sh_dma.h
- *
- * Header for the new SH dmaengine driver
- *
- * Copyright (C) 2010 Guennadi Liakhovetski <g.liakhovetski@gmx.de>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-#ifndef SH_AUDMAPP_H
-#define SH_AUDMAPP_H
-
-#include <linux/dmaengine.h>
-
-struct audmapp_slave_config {
-	int		slave_id;
-	dma_addr_t	src;
-	dma_addr_t	dst;
-	u32		chcr;
-};
-
-struct audmapp_pdata {
-	struct audmapp_slave_config *slave;
-	int slave_num;
-};
-
-#endif /* SH_AUDMAPP_H */
-- 
1.7.9.5

Re: [PATCH 1/6 v5] dmaengine: rcar-audmapp: independent from SH_DMAE_BASE v1

[Arnd Bergmann]

On Tuesday 20 January 2015 01:44:38 Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Current Renesas Audio DMAC Peri Peri driver is based on
> SH_DMAE_BASE driver which is used for Renesas SH-Mobile.
> But, basically, SH_DMAE_BASE driver was created for
> SuperH SoC, and it is not fully cared for DT.
> 
> For example, current SH_DMAE_BASE base driver will return
> non-matching DMA channel if some non-SH_DMAE_BASE drivers
> are probed.
> So, sound driver will get wrong DMA channel
> if Audio DMAC (= rcar-dma) was not probed,
> but Audio DMAC Peri Peri (= SH_DMAE_BASE) was probed.
> 
> OTOH, many SH-Mobile series drivers are using SH_DMAE_BASE
> driver, and Renesas R-Car series will not use it anymore.
> Maintenance cost for fully cared DT support on SH_DMAE_BASE
> will be very high
> (and keeping compatibility will be very complex).
> 
> In addition, Audio DMAC Peri Peri itself is very simple device,
> and, no SoC/board is using it from non-DT environment.
> 
> This patch simply removes current rcar-audmapp driver.
> 

I'm confused by the description above. From what I understand,
the purpose of the SH_DMAE_BASE driver is to multiplex between
the DMA engines that all share the same slaves on some of the
shmobile/r-mobile/r-car chips. If the audma driver does not
share its slaves with another dmaengine, it needs to register
its own of_dma_controller, which it does.

The problem you describe with getting the wrong channel seems
to me that the shdma_chan_filter() function matches any DMA
engine that was registered through shdma_init(), because its
device_alloc_chan_resources function pointer matches. This problem
could be avoided by adding some flag in shdma_dev as a bug-fix
(also for backports to stable kernels).

That said, together with patch 2, this seems like a useful
simplification, it just needs a better description in my mind.

	Arnd

Re: [PATCH 1/6 v5] dmaengine: rcar-audmapp: independent from SH_DMAE_BASE v1

[Kuninori Morimoto]

Hi Arnd, Laurent

Laurent, can you please correct my comment if it was wrong/un-understandable ?

> > Current Renesas Audio DMAC Peri Peri driver is based on
> > SH_DMAE_BASE driver which is used for Renesas SH-Mobile.
> > But, basically, SH_DMAE_BASE driver was created for
> > SuperH SoC, and it is not fully cared for DT.
> > 
> > For example, current SH_DMAE_BASE base driver will return
> > non-matching DMA channel if some non-SH_DMAE_BASE drivers
> > are probed.
> > So, sound driver will get wrong DMA channel
> > if Audio DMAC (= rcar-dma) was not probed,
> > but Audio DMAC Peri Peri (= SH_DMAE_BASE) was probed.
> > 
> > OTOH, many SH-Mobile series drivers are using SH_DMAE_BASE
> > driver, and Renesas R-Car series will not use it anymore.
> > Maintenance cost for fully cared DT support on SH_DMAE_BASE
> > will be very high
> > (and keeping compatibility will be very complex).
> > 
> > In addition, Audio DMAC Peri Peri itself is very simple device,
> > and, no SoC/board is using it from non-DT environment.
> > 
> > This patch simply removes current rcar-audmapp driver.
> > 
> 
> I'm confused by the description above. From what I understand,
> the purpose of the SH_DMAE_BASE driver is to multiplex between
> the DMA engines that all share the same slaves on some of the
> shmobile/r-mobile/r-car chips. If the audma driver does not
> share its slaves with another dmaengine, it needs to register
> its own of_dma_controller, which it does.
>
> The problem you describe with getting the wrong channel seems
> to me that the shdma_chan_filter() function matches any DMA
> engine that was registered through shdma_init(), because its
> device_alloc_chan_resources function pointer matches. This problem
> could be avoided by adding some flag in shdma_dev as a bug-fix
> (also for backports to stable kernels).
> 
> That said, together with patch 2, this seems like a useful
> simplification, it just needs a better description in my mind.

Historically we used SH_DMAE_BASE driver for DMAEngine when
SH-Mobile series. and now we have new R-Car series.

R-Car series DMAC <-> SH-Mbile series DMAC are similar, but
R-Car series DMAC has more advanced feature.
Then, adding new feature on SH_DMAE_BASE seems difficult (for many reasons)
So, we decided that R-Car series uses Laurent's rcar-dmac driver
(which is not SH_DMAE_BASE driver, so it doesn't use shdma_init)
instead of SH_DMAE_BASE driver.
But, because of timing reason, this audmapp which is used
from R-Car series was created as SH_DMAE_BASE driver.

SH_DMAE_BASE driver is mainly used from non-DT platform
(it is supporting DT, but difficult to fixup/understand today)
rcar-dmac is mainly used from DT platform.

Yes, SH_DMAE_BASE filter matches any DMAEngine,
but, it matches not only shdma_init base driver,
but matches with rcar-dmac.

From compatibility/complexity from point of view,
"audmapp independent from SH_DMAE_BASE" is easier than
"fixup SH_DMAE_BASE for DT".
Because no other R-Car SoC DMAC uses SH_DMAE_BASE driver

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/6 v5] dmaengine: rcar-audmapp: independent from SH_DMAE_BASE v1

[Arnd Bergmann]

On Wednesday 21 January 2015 01:25:51 Kuninori Morimoto wrote:
> Hi Arnd, Laurent
> 
> Laurent, can you please correct my comment if it was wrong/un-understandable ?
> 
> > > Current Renesas Audio DMAC Peri Peri driver is based on
> > > SH_DMAE_BASE driver which is used for Renesas SH-Mobile.
> > > But, basically, SH_DMAE_BASE driver was created for
> > > SuperH SoC, and it is not fully cared for DT.
> > > 
> > > For example, current SH_DMAE_BASE base driver will return
> > > non-matching DMA channel if some non-SH_DMAE_BASE drivers
> > > are probed.
> > > So, sound driver will get wrong DMA channel
> > > if Audio DMAC (= rcar-dma) was not probed,
> > > but Audio DMAC Peri Peri (= SH_DMAE_BASE) was probed.
> > > 
> > > OTOH, many SH-Mobile series drivers are using SH_DMAE_BASE
> > > driver, and Renesas R-Car series will not use it anymore.
> > > Maintenance cost for fully cared DT support on SH_DMAE_BASE
> > > will be very high
> > > (and keeping compatibility will be very complex).
> > > 
> > > In addition, Audio DMAC Peri Peri itself is very simple device,
> > > and, no SoC/board is using it from non-DT environment.
> > > 
> > > This patch simply removes current rcar-audmapp driver.
> > > 
> > 
> > I'm confused by the description above. From what I understand,
> > the purpose of the SH_DMAE_BASE driver is to multiplex between
> > the DMA engines that all share the same slaves on some of the
> > shmobile/r-mobile/r-car chips. If the audma driver does not
> > share its slaves with another dmaengine, it needs to register
> > its own of_dma_controller, which it does.
> >
> > The problem you describe with getting the wrong channel seems
> > to me that the shdma_chan_filter() function matches any DMA
> > engine that was registered through shdma_init(), because its
> > device_alloc_chan_resources function pointer matches. This problem
> > could be avoided by adding some flag in shdma_dev as a bug-fix
> > (also for backports to stable kernels).
> > 
> > That said, together with patch 2, this seems like a useful
> > simplification, it just needs a better description in my mind.
> 
> Historically we used SH_DMAE_BASE driver for DMAEngine when
> SH-Mobile series. and now we have new R-Car series.
> 
> R-Car series DMAC <-> SH-Mbile series DMAC are similar, but
> R-Car series DMAC has more advanced feature.
> Then, adding new feature on SH_DMAE_BASE seems difficult (for many reasons)
> So, we decided that R-Car series uses Laurent's rcar-dmac driver
> (which is not SH_DMAE_BASE driver, so it doesn't use shdma_init)
> instead of SH_DMAE_BASE driver.
> But, because of timing reason, this audmapp which is used
> from R-Car series was created as SH_DMAE_BASE driver.

My point was that the question whether to use SH_DMAE_BASE or
not should not depend on whether it is easy to do, but whether
it is *necessary*. We should not use it for drivers that do
not depend on the multiplexing, but we have to use it for the
ones that do.

By multiplexing, I mean drivers that specifically share a
common sh_dmae_slave_config array across multiple DMA engine
instances.

> SH_DMAE_BASE driver is mainly used from non-DT platform
> (it is supporting DT, but difficult to fixup/understand today)
> rcar-dmac is mainly used from DT platform.
> 
> Yes, SH_DMAE_BASE filter matches any DMAEngine,
> but, it matches not only shdma_init base driver,
> but matches with rcar-dmac.

How? shdma_chan_filter has this code

        /* Only support channels handled by this driver. */
        if (chan->device->device_alloc_chan_resources !            shdma_alloc_chan_resources)
                return false;

and that is only used by shdma_init(), which is called from
shdmac.c, sudmac.c, rcar-audmapp.c and rcar-hpbdma.c, but not
rcar-dmac.c

> From compatibility/complexity from point of view,
> "audmapp independent from SH_DMAE_BASE" is easier than
> "fixup SH_DMAE_BASE for DT".
> Because no other R-Car SoC DMAC uses SH_DMAE_BASE driver

Wouldn't this be enough to fix the bug (short term and for
backports, not in the long run)?

diff --git a/drivers/dma/sh/rcar-hpbdma.c b/drivers/dma/sh/rcar-hpbdma.c
index 6fef1b95c895..7a65740da3bd 100644
--- a/drivers/dma/sh/rcar-hpbdma.c
+++ b/drivers/dma/sh/rcar-hpbdma.c
@@ -604,6 +604,7 @@ static int hpb_dmae_probe(struct platform_device *pdev)
 
 	hpbdev->shdma_dev.ops = &hpb_dmae_ops;
 	hpbdev->shdma_dev.desc_size = sizeof(struct hpb_desc);
+	hpbdev->shdma_dev.multiplexed = true;
 	err = shdma_init(&pdev->dev, &hpbdev->shdma_dev, pdata->num_channels);
 	if (err < 0)
 		goto error;
diff --git a/drivers/dma/sh/shdma-base.c b/drivers/dma/sh/shdma-base.c
index 8ee383d339a5..6b04312394d3 100644
--- a/drivers/dma/sh/shdma-base.c
+++ b/drivers/dma/sh/shdma-base.c
@@ -284,6 +284,9 @@ bool shdma_chan_filter(struct dma_chan *chan, void *arg)
 	    shdma_alloc_chan_resources)
 		return false;
 
+	if (!sdev->multiplexed)
+		return false;
+
 	if (match < 0)
 		/* No slave requested - arbitrary channel */
 		return true;
diff --git a/drivers/dma/sh/shdma.h b/drivers/dma/sh/shdma.h
index 2c0a969adc9f..ef0112e19b0e 100644
--- a/drivers/dma/sh/shdma.h
+++ b/drivers/dma/sh/shdma.h
@@ -43,6 +43,7 @@ struct sh_dmae_device {
 	void __iomem *dmars;
 	unsigned int chcr_offset;
 	u32 chcr_ie_bit;
+	bool multiplexed;
 };
 
 struct sh_dmae_regs {
diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c
index aec8a84784a4..f9b38f165885 100644
--- a/drivers/dma/sh/shdmac.c
+++ b/drivers/dma/sh/shdmac.c
@@ -756,6 +756,7 @@ static int sh_dmae_probe(struct platform_device *pdev)
 
 	shdev->shdma_dev.ops = &sh_dmae_shdma_ops;
 	shdev->shdma_dev.desc_size = sizeof(struct sh_dmae_desc);
+	shdev->shdma_dev.multiplexed = true;
 	err = shdma_init(&pdev->dev, &shdev->shdma_dev,
 			      pdata->channel_num);
 	if (err < 0)


On a related topic, it seems you are very close to removing the
legacy sh_dmae_slave_config/hpb_dmae_slave_config arrays, the
only drivers that still rely on them are sound/soc/sh/siu_pcm.c,
drivers/usb/renesas_usbhs/fifo.c and drivers/tty/serial/sh-sci.c.
After those are converted to use dma_slave_config() and the normal
filter functions, a lot of obsolete code and data could just
go away.

	Arnd

Re: [PATCH 1/6 v5] dmaengine: rcar-audmapp: independent from SH_DMAE_BASE v1

[Kuninori Morimoto]

Hi Arnd

> How? shdma_chan_filter has this code
> 
>         /* Only support channels handled by this driver. */
>         if (chan->device->device_alloc_chan_resources !>             shdma_alloc_chan_resources)
>                 return false;
> 
> and that is only used by shdma_init(), which is called from
> shdmac.c, sudmac.c, rcar-audmapp.c and rcar-hpbdma.c, but not
> rcar-dmac.c
> 
> > From compatibility/complexity from point of view,
> > "audmapp independent from SH_DMAE_BASE" is easier than
> > "fixup SH_DMAE_BASE for DT".
> > Because no other R-Car SoC DMAC uses SH_DMAE_BASE driver
> 
> Wouldn't this be enough to fix the bug (short term and for
> backports, not in the long run)?

Thanks !
I double checked audmapp's issue, and your idea seems solved
my problem.

> On a related topic, it seems you are very close to removing the
> legacy sh_dmae_slave_config/hpb_dmae_slave_config arrays, the
> only drivers that still rely on them are sound/soc/sh/siu_pcm.c,
> drivers/usb/renesas_usbhs/fifo.c and drivers/tty/serial/sh-sci.c.
> After those are converted to use dma_slave_config() and the normal
> filter functions, a lot of obsolete code and data could just
> go away.

OK. I will investigate and work for these.

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/6 v5] dmaengine: rcar-audmapp: independent from SH_DMAE_BASE v1

[Laurent Pinchart]

Hi Arnd,

On Wednesday 21 January 2015 15:19:48 Arnd Bergmann wrote:
> On Wednesday 21 January 2015 01:25:51 Kuninori Morimoto wrote:
> > Hi Arnd, Laurent
> > 
> > Laurent, can you please correct my comment if it was
> > wrong/un-understandable ?
> >
> >>> Current Renesas Audio DMAC Peri Peri driver is based on
> >>> SH_DMAE_BASE driver which is used for Renesas SH-Mobile.
> >>> But, basically, SH_DMAE_BASE driver was created for
> >>> SuperH SoC, and it is not fully cared for DT.
> >>> 
> >>> For example, current SH_DMAE_BASE base driver will return
> >>> non-matching DMA channel if some non-SH_DMAE_BASE drivers
> >>> are probed.
> >>> So, sound driver will get wrong DMA channel
> >>> if Audio DMAC (= rcar-dma) was not probed,
> >>> but Audio DMAC Peri Peri (= SH_DMAE_BASE) was probed.
> >>> 
> >>> OTOH, many SH-Mobile series drivers are using SH_DMAE_BASE
> >>> driver, and Renesas R-Car series will not use it anymore.
> >>> Maintenance cost for fully cared DT support on SH_DMAE_BASE
> >>> will be very high
> >>> (and keeping compatibility will be very complex).
> >>> 
> >>> In addition, Audio DMAC Peri Peri itself is very simple device,
> >>> and, no SoC/board is using it from non-DT environment.
> >>> 
> >>> This patch simply removes current rcar-audmapp driver.
> >> 
> >> I'm confused by the description above. From what I understand,
> >> the purpose of the SH_DMAE_BASE driver is to multiplex between
> >> the DMA engines that all share the same slaves on some of the
> >> shmobile/r-mobile/r-car chips. If the audma driver does not
> >> share its slaves with another dmaengine, it needs to register
> >> its own of_dma_controller, which it does.
> >> 
> >> The problem you describe with getting the wrong channel seems
> >> to me that the shdma_chan_filter() function matches any DMA
> >> engine that was registered through shdma_init(), because its
> >> device_alloc_chan_resources function pointer matches. This problem
> >> could be avoided by adding some flag in shdma_dev as a bug-fix
> >> (also for backports to stable kernels).
> >> 
> >> That said, together with patch 2, this seems like a useful
> >> simplification, it just needs a better description in my mind.
> > 
> > Historically we used SH_DMAE_BASE driver for DMAEngine when
> > SH-Mobile series. and now we have new R-Car series.
> > 
> > R-Car series DMAC <-> SH-Mbile series DMAC are similar, but
> > R-Car series DMAC has more advanced feature.
> > Then, adding new feature on SH_DMAE_BASE seems difficult (for many
> > reasons)
> > So, we decided that R-Car series uses Laurent's rcar-dmac driver
> > (which is not SH_DMAE_BASE driver, so it doesn't use shdma_init)
> > instead of SH_DMAE_BASE driver.
> > But, because of timing reason, this audmapp which is used
> > from R-Car series was created as SH_DMAE_BASE driver.
> 
> My point was that the question whether to use SH_DMAE_BASE or
> not should not depend on whether it is easy to do, but whether
> it is *necessary*. We should not use it for drivers that do
> not depend on the multiplexing, but we have to use it for the
> ones that do.
> 
> By multiplexing, I mean drivers that specifically share a
> common sh_dmae_slave_config array across multiple DMA engine
> instances.

Actually the R-Car platforms suffer from the same multiplexing issue. We have 
decided to implement a new R-Car DMAC driver due to the complexity of adding 
new features to the existing code base, aiming at a "start clean from scratch" 
approach.

Multiplexing isn't supported by the new driver. How to implement that properly 
will need to be discussed when and if needed.

> > SH_DMAE_BASE driver is mainly used from non-DT platform
> > (it is supporting DT, but difficult to fixup/understand today)
> > rcar-dmac is mainly used from DT platform.
> > 
> > Yes, SH_DMAE_BASE filter matches any DMAEngine,
> > but, it matches not only shdma_init base driver,
> > but matches with rcar-dmac.
> 
> How? shdma_chan_filter has this code
> 
>         /* Only support channels handled by this driver. */
>         if (chan->device->device_alloc_chan_resources !>             shdma_alloc_chan_resources)
>                 return false;
> 
> and that is only used by shdma_init(), which is called from
> shdmac.c, sudmac.c, rcar-audmapp.c and rcar-hpbdma.c, but not
> rcar-dmac.c
> 
> > From compatibility/complexity from point of view,
> > "audmapp independent from SH_DMAE_BASE" is easier than
> > "fixup SH_DMAE_BASE for DT".
> > Because no other R-Car SoC DMAC uses SH_DMAE_BASE driver
> 
> Wouldn't this be enough to fix the bug (short term and for
> backports, not in the long run)?
> 
> diff --git a/drivers/dma/sh/rcar-hpbdma.c b/drivers/dma/sh/rcar-hpbdma.c
> index 6fef1b95c895..7a65740da3bd 100644
> --- a/drivers/dma/sh/rcar-hpbdma.c
> +++ b/drivers/dma/sh/rcar-hpbdma.c
> @@ -604,6 +604,7 @@ static int hpb_dmae_probe(struct platform_device *pdev)
> 
>  	hpbdev->shdma_dev.ops = &hpb_dmae_ops;
>  	hpbdev->shdma_dev.desc_size = sizeof(struct hpb_desc);
> +	hpbdev->shdma_dev.multiplexed = true;
>  	err = shdma_init(&pdev->dev, &hpbdev->shdma_dev, pdata->num_channels);
>  	if (err < 0)
>  		goto error;
> diff --git a/drivers/dma/sh/shdma-base.c b/drivers/dma/sh/shdma-base.c
> index 8ee383d339a5..6b04312394d3 100644
> --- a/drivers/dma/sh/shdma-base.c
> +++ b/drivers/dma/sh/shdma-base.c
> @@ -284,6 +284,9 @@ bool shdma_chan_filter(struct dma_chan *chan, void *arg)
> shdma_alloc_chan_resources)
>  		return false;
> 
> +	if (!sdev->multiplexed)
> +		return false;
> +
>  	if (match < 0)
>  		/* No slave requested - arbitrary channel */
>  		return true;
> diff --git a/drivers/dma/sh/shdma.h b/drivers/dma/sh/shdma.h
> index 2c0a969adc9f..ef0112e19b0e 100644
> --- a/drivers/dma/sh/shdma.h
> +++ b/drivers/dma/sh/shdma.h
> @@ -43,6 +43,7 @@ struct sh_dmae_device {
>  	void __iomem *dmars;
>  	unsigned int chcr_offset;
>  	u32 chcr_ie_bit;
> +	bool multiplexed;
>  };
> 
>  struct sh_dmae_regs {
> diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c
> index aec8a84784a4..f9b38f165885 100644
> --- a/drivers/dma/sh/shdmac.c
> +++ b/drivers/dma/sh/shdmac.c
> @@ -756,6 +756,7 @@ static int sh_dmae_probe(struct platform_device *pdev)
> 
>  	shdev->shdma_dev.ops = &sh_dmae_shdma_ops;
>  	shdev->shdma_dev.desc_size = sizeof(struct sh_dmae_desc);
> +	shdev->shdma_dev.multiplexed = true;
>  	err = shdma_init(&pdev->dev, &shdev->shdma_dev,
>  			      pdata->channel_num);
>  	if (err < 0)
> 
> 
> On a related topic, it seems you are very close to removing the
> legacy sh_dmae_slave_config/hpb_dmae_slave_config arrays, the
> only drivers that still rely on them are sound/soc/sh/siu_pcm.c,
> drivers/usb/renesas_usbhs/fifo.c and drivers/tty/serial/sh-sci.c.
> After those are converted to use dma_slave_config() and the normal
> filter functions, a lot of obsolete code and data could just
> go away.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6 v5] dmaengine: rcar-audmapp: independent from SH_DMAE_BASE v1

[Arnd Bergmann]

On Thursday 22 January 2015 23:25:49 Laurent Pinchart wrote:
> 
> Actually the R-Car platforms suffer from the same multiplexing issue. We have 
> decided to implement a new R-Car DMAC driver due to the complexity of adding 
> new features to the existing code base, aiming at a "start clean from scratch" 
> approach.
> 
> Multiplexing isn't supported by the new driver. How to implement that properly 
> will need to be discussed when and if needed.
> 

Ok, I see. Do these chips also multiplex between dma engine instances
with different drivers, or only between similar dma engine IP blocks?

When we created the generic dmaengine binding, we intentionally
mandated the use of dma-names do allow you specify multiple connections
in one property, and if they have the same name, the dmaengine core
should be free to pick any of them.

I believe this was never implemented in Linux though, so the dmaengine
core picks the first one with a matching name and does not try any
others when it fails. We would need to come up with a good policy to
decide in which order to try the channels, but implementing any scheme
should not be too hard.

The current shdma multiplexing driver with the "renesas,shdma-mux" binding
implements a different scheme, but also incomplete: The binding documents
that it multiplexes between the dmaengine devices that are children of
the mux. The driver instead multiplexes between all dmaengine devices
that are registered through shdma_init() regardless of their location
in DT. Apparently this resulted in the correct behavior for all the
traditional SoCs on which all the dmaengines are multiplexed together,
but it broke for the r-car audmapp that is not multiplexed in the same
way.

	Arnd

Re: [PATCH 1/6 v5] dmaengine: rcar-audmapp: independent from SH_DMAE_BASE v1

[Laurent Pinchart]

Hi Arnd,

Revisiting this old topic.

On Friday 23 January 2015 14:16:38 Arnd Bergmann wrote:
> On Thursday 22 January 2015 23:25:49 Laurent Pinchart wrote:
> > Actually the R-Car platforms suffer from the same multiplexing issue. We
> > have decided to implement a new R-Car DMAC driver due to the complexity
> > of adding new features to the existing code base, aiming at a "start
> > clean from scratch" approach.
> > 
> > Multiplexing isn't supported by the new driver. How to implement that
> > properly will need to be discussed when and if needed.
> 
> Ok, I see. Do these chips also multiplex between dma engine instances
> with different drivers, or only between similar dma engine IP blocks?

Only between different instances of the same IP core.

> When we created the generic dmaengine binding, we intentionally
> mandated the use of dma-names do allow you specify multiple connections
> in one property, and if they have the same name, the dmaengine core
> should be free to pick any of them.
> 
> I believe this was never implemented in Linux though, so the dmaengine
> core picks the first one with a matching name and does not try any
> others when it fails. We would need to come up with a good policy to
> decide in which order to try the channels, but implementing any scheme
> should not be too hard.

The biggest problem I see there is when to decide on channel allocation. The 
current DMA engine API expects association between slaves and DMA engines to 
be decided at channel request time. However, drivers usually request channels 
at probe time, even if they rarely use the channels. We end up with resources 
being allocated and held when they could really be shared. The virtual DMA 
channels API tries to fix that but doesn't push back allocation to usage time.

How should we fix that ? And more importantly, is it worth fixing ? It looks 
like pretty much everybody works around the issue on platforms where the 
number of slaves exceeds the number of channels.

> The current shdma multiplexing driver with the "renesas,shdma-mux" binding
> implements a different scheme, but also incomplete: The binding documents
> that it multiplexes between the dmaengine devices that are children of
> the mux. The driver instead multiplexes between all dmaengine devices
> that are registered through shdma_init() regardless of their location
> in DT. Apparently this resulted in the correct behavior for all the
> traditional SoCs on which all the dmaengines are multiplexed together,
> but it broke for the r-car audmapp that is not multiplexed in the same
> way.

I don't really like that implementation given that it uses DT to describe a 
virtual mux. For platforms where the number of DMA engines is low (such as R-
Car where we have two system DMA engines) I think listing possible channels 
explicitly in DT should be fine. If we had a high number of interchangeable 
DMA engines it would be another story.

-- 
Regards,

Laurent Pinchart