[PATCH] mmc: sh_mmcif: Silence DMA slave ID compile warnings on 64-bit

[PATCH] mmc: sh_mmcif: Silence DMA slave ID compile warnings on 64-bit

[Geert Uytterhoeven]

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(-)

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__,
-- 
1.9.1

Re: [PATCH] mmc: sh_mmcif: Silence DMA slave ID compile warnings on 64-bit

[Arnd Bergmann]

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

Re: [PATCH] mmc: sh_mmcif: Silence DMA slave ID compile warnings on 64-bit

[Geert Uytterhoeven]

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

Re: [PATCH] mmc: sh_mmcif: Silence DMA slave ID compile warnings on 64-bit

[Arnd Bergmann]

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

Re: [PATCH] mmc: sh_mmcif: Silence DMA slave ID compile warnings on 64-bit

[Geert Uytterhoeven]

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

Re: [PATCH] mmc: sh_mmcif: Silence DMA slave ID compile warnings on 64-bit

[Geert Uytterhoeven]

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

Re: [PATCH] mmc: sh_mmcif: Silence DMA slave ID compile warnings on 64-bit

[Arnd Bergmann]

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