Re: [PATCH v1 3/4] scsi - convert mvme146_scsi.c to platform device

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Geert,

Am 11.07.2022 um 19:16 schrieb Geert Uytterhoeven:
> On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
>> On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>> Convert the mvme147_scsi driver to a platform device driver.
>>> This is required for conversion of the driver to the DMA API.
>>>
>>> CC: linux-scsi@vger.kernel.org
>>> Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
>>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>
>> The patch looks correct to me, but the type cast for the address doesn't
>> seem right:
>>
>>> -       regs.SASR = (volatile unsigned char *)0xfffe4000;
>>> -       regs.SCMD = (volatile unsigned char *)0xfffe4001;
>>>
>>> -       hdata = shost_priv(mvme147_shost);
>>> +       mvme147_inst->base = mres->start;
>>> +       mvme147_inst->irq = ires->start;
>>> +
>>> +       regs.SASR = (volatile unsigned char *)mres->start;
>>> +       regs.SCMD = (volatile unsigned char *)(mres->start)+0x1;
>>
>> A resource would pass a phys_addr_t token, but the driver expects a
>> virtual address that should be an __iomem pointer. The MMIO area
>> already gets mapped into virtual addresses in arch/m68k/kernel/head.S,
>> so it makes sense to skip the extra ioremap() and just use the address,
>> but then you can't pass it as an IORESOUCRE_MEM token and should
>> use platform_data with the pointer instead.

OK, got it now (I had missed the physical/virtual mismatch entirely).

>>
>> The alternative is to do it the normal way and pass the physical address
>> as a resource, that you can pass into devm_platform_ioremap_resource()
>> or a similar helper.
>
> I would prefer the latter.  While head.S sets up the mapping,
> __ioremap() does not have support for this on the mvme platform,
> so this has to be added first. Look at the amiga and virt platforms
> for examples.

I see - doesn't look too hard to do, and should not affect any other 
existing code.
Is it worth adding the same support for Atari as well?

> If you do want to pass the virtual address as platform data (for now
> ;-), please provide the physical memory resource too, so we don't
> have to go through another synchronization step with the m68k and
> scsi trees later.

That's what I had intended to do, but I'd rather get this sorted once 
and for all.

Cheers,

	Michael


>
> Thanks!
>
> 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
>