| Message ID | 1439297314-29118-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 11 August 2015 14:48:34 Geert Uytterhoeven wrote: > dma_cap_mask_t mask; > @@ -413,12 +413,11 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, > dma_cap_set(DMA_SLAVE, mask); > > if (pdata) > - slave_data = direction == DMA_MEM_TO_DEV ? > - (void *)pdata->slave_id_tx : > - (void *)pdata->slave_id_rx; > + slave_id = direction == DMA_MEM_TO_DEV ? > + pdata->slave_id_tx : pdata->slave_id_rx; > > chan = dma_request_slave_channel_compat(mask, shdma_chan_filter, > - slave_data, dev, > + (void *)(uintptr_t)slave_id, dev, > direction == DMA_MEM_TO_DEV ? "tx" : "rx"); > > dev_dbg(dev, "%s: %s: got channel %p\n", __func__, How about changing the type of the slave_id_rx/slave_id_tx fields to void*? That way, the hack can be moved to arch/sh/boards/board-sh7757lcr.c, which is now the only file passing data this way. Ideally, we'd also pass the shdma_chan_filter function pointer in pdata to avoid the link time dependency on a particular dmaengine driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" 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 Wed, Aug 12, 2015 at 4:50 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 11 August 2015 14:48:34 Geert Uytterhoeven wrote: >> dma_cap_mask_t mask; >> @@ -413,12 +413,11 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, >> dma_cap_set(DMA_SLAVE, mask); >> >> if (pdata) >> - slave_data = direction == DMA_MEM_TO_DEV ? >> - (void *)pdata->slave_id_tx : >> - (void *)pdata->slave_id_rx; >> + slave_id = direction == DMA_MEM_TO_DEV ? >> + pdata->slave_id_tx : pdata->slave_id_rx; >> >> chan = dma_request_slave_channel_compat(mask, shdma_chan_filter, >> - slave_data, dev, >> + (void *)(uintptr_t)slave_id, dev, >> direction == DMA_MEM_TO_DEV ? "tx" : "rx"); >> >> dev_dbg(dev, "%s: %s: got channel %p\n", __func__, > > How about changing the type of the slave_id_rx/slave_id_tx fields > to void*? That way, the hack can be moved to arch/sh/boards/board-sh7757lcr.c, > which is now the only file passing data this way. Ideally, we'd also And to the other SH boards... This is not limited to the mmcif driver. Ah, I see tmio_mmc_data already does it this way. This will break out-of-tree boards at compile-time though. Given the limited number of wired up DMA channels under arch/sh/, they may actually be more in use out-of-tree than in-tree? Alternatively, perhaps we should just drop pdata DMA configuration from some drivers? > pass the shdma_chan_filter function pointer in pdata to avoid the link > time dependency on a particular dmaengine driver. That's actually more tricky, as a NULL filter function means something completely different, and causes funny failures if the DTS lacks "dmas" properties, cfr. "dmaengine: shdma: Make dummy shdma_chan_filter() always return false". https://git.kernel.org/cgit/linux/kernel/git/vkoul/slave-dma.git/commit/?h=next&id=056f6c87028544de934f27caf95aa1545d585767 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 12 August 2015 17:13:31 Geert Uytterhoeven wrote: > Hi Arnd, > > On Wed, Aug 12, 2015 at 4:50 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 11 August 2015 14:48:34 Geert Uytterhoeven wrote: > >> dma_cap_mask_t mask; > >> @@ -413,12 +413,11 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, > >> dma_cap_set(DMA_SLAVE, mask); > >> > >> if (pdata) > >> - slave_data = direction == DMA_MEM_TO_DEV ? > >> - (void *)pdata->slave_id_tx : > >> - (void *)pdata->slave_id_rx; > >> + slave_id = direction == DMA_MEM_TO_DEV ? > >> + pdata->slave_id_tx : pdata->slave_id_rx; > >> > >> chan = dma_request_slave_channel_compat(mask, shdma_chan_filter, > >> - slave_data, dev, > >> + (void *)(uintptr_t)slave_id, dev, > >> direction == DMA_MEM_TO_DEV ? "tx" : "rx"); > >> > >> dev_dbg(dev, "%s: %s: got channel %p\n", __func__, > > > > How about changing the type of the slave_id_rx/slave_id_tx fields > > to void*? That way, the hack can be moved to arch/sh/boards/board-sh7757lcr.c, > > which is now the only file passing data this way. Ideally, we'd also > > And to the other SH boards... This is not limited to the mmcif driver. > > Ah, I see tmio_mmc_data already does it this way. I probably made a similar comment there, now I just did 'git grep slave_id_.x' > This will break out-of-tree boards at compile-time though. Given the limited > number of wired up DMA channels under arch/sh/, they may actually be > more in use out-of-tree than in-tree? > > Alternatively, perhaps we should just drop pdata DMA configuration from > some drivers? I think that would be helpful, but it would not work for this driver as long as we keep arch/sh/boards/board-sh7757lcr.c in the kernel. > > pass the shdma_chan_filter function pointer in pdata to avoid the link > > time dependency on a particular dmaengine driver. > > That's actually more tricky, as a NULL filter function means something > completely different, and causes funny failures if the DTS lacks "dmas" > properties, cfr. "dmaengine: shdma: Make dummy shdma_chan_filter() always > return false". > https://git.kernel.org/cgit/linux/kernel/git/vkoul/slave-dma.git/commit/?h=next&id=056f6c87028544de934f27caf95aa1545d585767 Ah, this is nasty, I had not thought of that case. This makes the entire dma_request_slave_channel_compat() interface (which I never liked, fwiw) dangerous to use, because a lot of other drivers will get the filter function pointer from pdata already (any driver that works with more than one dmaengine master has to). I wonder if we should enforce this in __dma_request_slave_channel_compat() by checking both the fn and fn_param arguments to ensure they are not NULL before calling the legacy fallback using __dma_request_channel. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" 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 Sat, Aug 15, 2015 at 10:26 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 12 August 2015 17:13:31 Geert Uytterhoeven wrote: >> On Wed, Aug 12, 2015 at 4:50 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Tuesday 11 August 2015 14:48:34 Geert Uytterhoeven wrote: >> >> dma_cap_mask_t mask; >> >> @@ -413,12 +413,11 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, >> >> dma_cap_set(DMA_SLAVE, mask); >> >> >> >> if (pdata) >> >> - slave_data = direction == DMA_MEM_TO_DEV ? >> >> - (void *)pdata->slave_id_tx : >> >> - (void *)pdata->slave_id_rx; >> >> + slave_id = direction == DMA_MEM_TO_DEV ? >> >> + pdata->slave_id_tx : pdata->slave_id_rx; >> >> >> >> chan = dma_request_slave_channel_compat(mask, shdma_chan_filter, >> >> - slave_data, dev, >> >> + (void *)(uintptr_t)slave_id, dev, >> >> direction == DMA_MEM_TO_DEV ? "tx" : "rx"); >> >> >> >> dev_dbg(dev, "%s: %s: got channel %p\n", __func__, >> > >> > How about changing the type of the slave_id_rx/slave_id_tx fields >> > to void*? That way, the hack can be moved to arch/sh/boards/board-sh7757lcr.c, >> > which is now the only file passing data this way. Ideally, we'd also >> >> And to the other SH boards... This is not limited to the mmcif driver. >> >> Ah, I see tmio_mmc_data already does it this way. > > I probably made a similar comment there, now I just did 'git grep slave_id_.x' :-) >> This will break out-of-tree boards at compile-time though. Given the limited >> number of wired up DMA channels under arch/sh/, they may actually be >> more in use out-of-tree than in-tree? >> >> Alternatively, perhaps we should just drop pdata DMA configuration from >> some drivers? > > I think that would be helpful, but it would not work for this driver as > long as we keep arch/sh/boards/board-sh7757lcr.c in the kernel. No, not for this driver. But there are several drivers (spi-rspi, spi-sh-msiof, sh-sci, sh_flctl) that have .slave*id* fields in platform data, but no platform code ever sets them. Despite platform code that fills in SHDMA_SLAVE_* values in sh_dmae_chan. These don't work anyway, and I don't think anyone is interested in fixing that. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" 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 Sat, Aug 15, 2015 at 10:26 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > How about changing the type of the slave_id_rx/slave_id_tx fields >> > to void*? That way, the hack can be moved to arch/sh/boards/board-sh7757lcr.c, >> > which is now the only file passing data this way. Ideally, we'd also >> > pass the shdma_chan_filter function pointer in pdata to avoid the link >> > time dependency on a particular dmaengine driver. >> >> That's actually more tricky, as a NULL filter function means something >> completely different, and causes funny failures if the DTS lacks "dmas" >> properties, cfr. "dmaengine: shdma: Make dummy shdma_chan_filter() always >> return false". >> https://git.kernel.org/cgit/linux/kernel/git/vkoul/slave-dma.git/commit/?h=next&id=056f6c87028544de934f27caf95aa1545d585767 > > Ah, this is nasty, I had not thought of that case. This makes the entire > dma_request_slave_channel_compat() interface (which I never liked, fwiw) > dangerous to use, because a lot of other drivers will get the filter > function pointer from pdata already (any driver that works with more > than one dmaengine master has to). > > I wonder if we should enforce this in __dma_request_slave_channel_compat() > by checking both the fn and fn_param arguments to ensure they are not > NULL before calling the legacy fallback using __dma_request_channel. Thanks, that seems to work fine. Will send a patch... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 17 August 2015 14:17:25 Geert Uytterhoeven wrote: > >> This will break out-of-tree boards at compile-time though. Given the limited > >> number of wired up DMA channels under arch/sh/, they may actually be > >> more in use out-of-tree than in-tree? > >> > >> Alternatively, perhaps we should just drop pdata DMA configuration from > >> some drivers? > > > > I think that would be helpful, but it would not work for this driver as > > long as we keep arch/sh/boards/board-sh7757lcr.c in the kernel. > > No, not for this driver. > > But there are several drivers (spi-rspi, spi-sh-msiof, sh-sci, sh_flctl) that > have .slave*id* fields in platform data, but no platform code ever sets them. > Despite platform code that fills in SHDMA_SLAVE_* values in sh_dmae_chan. > These don't work anyway, and I don't think anyone is interested in fixing that. Right, removing pdata support for those would be a useful cleanup. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c index 5a1fdd405b1af14f..51ac09b8a5770df2 100644 --- a/drivers/mmc/host/sh_mmcif.c +++ b/drivers/mmc/host/sh_mmcif.c @@ -403,7 +403,7 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, { struct dma_slave_config cfg = { 0, }; struct dma_chan *chan; - void *slave_data = NULL; + unsigned int slave_id = 0; struct resource *res; struct device *dev = sh_mmcif_host_to_dev(host); dma_cap_mask_t mask; @@ -413,12 +413,11 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, dma_cap_set(DMA_SLAVE, mask); if (pdata) - slave_data = direction == DMA_MEM_TO_DEV ? - (void *)pdata->slave_id_tx : - (void *)pdata->slave_id_rx; + slave_id = direction == DMA_MEM_TO_DEV ? + pdata->slave_id_tx : pdata->slave_id_rx; chan = dma_request_slave_channel_compat(mask, shdma_chan_filter, - slave_data, dev, + (void *)(uintptr_t)slave_id, dev, direction == DMA_MEM_TO_DEV ? "tx" : "rx"); dev_dbg(dev, "%s: %s: got channel %p\n", __func__,
On arm64: drivers/mmc/host/sh_mmcif.c: In function 'sh_mmcif_request_dma_one': drivers/mmc/host/sh_mmcif.c:417:4: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] (void *)pdata->slave_id_tx : ^ drivers/mmc/host/sh_mmcif.c:418:4: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] (void *)pdata->slave_id_rx; ^ Add an intermediate cast to "uintptr_t" to kill the compile warning. While we're at it, replace "void *slave_data" by "unsigned int slave_id" to avoid having to cast in two places, and because it's a DMA slave ID, not a platform data pointer. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/mmc/host/sh_mmcif.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)