| Message ID | 2195960.ext3DPnloV@wuerfel (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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)