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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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