[v1,3/4] scsi - convert mvme146_scsi.c to platform device

Commit Message

Michael Schmitz July 9, 2022, 12:10 a.m. UTC

Convert the mvme147_scsi driver to a platform device driver.
This is required for conversion of the driver to the DMA API.

CC: [e-mail]
Link: https://lore.kernel.org/r/[e-mail]
Signed-off-by: Michael Schmitz <[e-mail]>
---
 drivers/scsi/mvme147.c | 86 ++++++++++++++++++++++++++++--------------
 1 file changed, 58 insertions(+), 28 deletions(-)

Comments

Arnd Bergmann July 10, 2022, 4:11 p.m. UTC | #1

On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <[e-mail]> wrote:
>
> Convert the mvme147_scsi driver to a platform device driver.
> This is required for conversion of the driver to the DMA API.
>
> CC: [e-mail]
> Link: https://lore.kernel.org/r/[e-mail]
> Signed-off-by: Michael Schmitz <[e-mail]>

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.

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.

       Arnd

Geert Uytterhoeven July 11, 2022, 7:16 a.m. UTC | #2

On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <[e-mail]> wrote:
> On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <[e-mail]> wrote:
> > Convert the mvme147_scsi driver to a platform device driver.
> > This is required for conversion of the driver to the DMA API.
> >
> > CC: [e-mail]
> > Link: https://lore.kernel.org/r/[e-mail]
> > Signed-off-by: Michael Schmitz <[e-mail]>
>
> 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.
>
> 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.

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.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Michael Schmitz July 11, 2022, 7:57 a.m. UTC | #3

Hi Geert,

Am 11.07.2022 um 19:16 schrieb Geert Uytterhoeven:
> On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <[e-mail]> wrote:
>> On Sat, Jul 9, 2022 at 2:10 AM Michael Schmitz <[e-mail]> wrote:
>>> Convert the mvme147_scsi driver to a platform device driver.
>>> This is required for conversion of the driver to the DMA API.
>>>
>>> CC: [e-mail]
>>> Link: https://lore.kernel.org/r/[e-mail]
>>> Signed-off-by: Michael Schmitz <[e-mail]>
>>
>> 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 -- [e-mail]
>
> 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
>

Geert Uytterhoeven July 11, 2022, 8:27 a.m. UTC | #4

Hi Michael,

On Mon, Jul 11, 2022 at 9:57 AM Michael Schmitz <[e-mail]> wrote:
> Am 11.07.2022 um 19:16 schrieb Geert Uytterhoeven:
> > On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <[e-mail]> wrote:
> >> 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).

And the __iomem is there so we can catch mistakes using sparse
(make C=2 path/to/file/.o).

> >> 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?

From a quick glance at arch/m68k/kernel/head.S, it seems that
on Atari there is no identity mapping (the high I/O area is mapped
to the virtual low area).  That means __ioremap() and iounmap()
wouldn't be symmetrical, but it can be done.

Note that on Amiga we only use the identity shortcut for Zorro III
memory (and only for the first half?), i.e. ioremap() on Zorro II I/O
does add new mappings.  Hence most Zorro II drivers use ZTWO_VADDR()
instead of ioremap().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Michael Schmitz July 11, 2022, 8:02 p.m. UTC | #5

Hi Geert,

Am 11.07.2022 um 20:27 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Mon, Jul 11, 2022 at 9:57 AM Michael Schmitz <[e-mail]> wrote:
>> Am 11.07.2022 um 19:16 schrieb Geert Uytterhoeven:
>>> On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <[e-mail]> wrote:
>>>> 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).
>
> And the __iomem is there so we can catch mistakes using sparse
> (make C=2 path/to/file/.o).

I'll need to make a habit of that ...

>>>> 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?
>
> From a quick glance at arch/m68k/kernel/head.S, it seems that
> on Atari there is no identity mapping (the high I/O area is mapped
> to the virtual low area).  That means __ioremap() and iounmap()
> wouldn't be symmetrical, but it can be done.

As I read it, it's the other way: virtual 0xffxxxxxx is mapped to phys. 
0x00ffxxxx, and all hardware addresses are given in the upper window 
(Medusa/Hades use that window only, and have identity mappings there).

So returning identity mapping in the high window would work AFAICS. I'll 
give that a try sometime.

> Note that on Amiga we only use the identity shortcut for Zorro III
> memory (and only for the first half?), i.e. ioremap() on Zorro II I/O
> does add new mappings.  Hence most Zorro II drivers use ZTWO_VADDR()
> instead of ioremap().

I see ZTWO_VADDR() already returns void __iomem* ... Fixing this patch 
would sort out the m68k drivers, leaving SGI. I'll have to see whether I 
can dig up a SGI workstation with this driver; might be easier than 
getting hold of any Amgias here Dowh Under.

Cheers,

	Michael


>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
>
> 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
>

Michael Schmitz July 12, 2022, 3:27 a.m. UTC | #6

Hi Geert,

I just realized the commit message subject for this patch needs 
correcting. Will that mess up your workflow (assuming I retain the 
subject line for the announcement mail)?

Cheers,

	Michael


Am 12.07.2022 um 08:02 schrieb Michael Schmitz:
> Hi Geert,
>
> Am 11.07.2022 um 20:27 schrieb Geert Uytterhoeven:
>> Hi Michael,
>>
>> On Mon, Jul 11, 2022 at 9:57 AM Michael Schmitz <[e-mail]>
>> wrote:
>>> Am 11.07.2022 um 19:16 schrieb Geert Uytterhoeven:
>>>> On Sun, Jul 10, 2022 at 6:12 PM Arnd Bergmann <[e-mail]> wrote:
>>>>> 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).
>>
>> And the __iomem is there so we can catch mistakes using sparse
>> (make C=2 path/to/file/.o).
>
> I'll need to make a habit of that ...
>
>>>>> 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?
>>
>> From a quick glance at arch/m68k/kernel/head.S, it seems that
>> on Atari there is no identity mapping (the high I/O area is mapped
>> to the virtual low area).  That means __ioremap() and iounmap()
>> wouldn't be symmetrical, but it can be done.
>
> As I read it, it's the other way: virtual 0xffxxxxxx is mapped to phys.
> 0x00ffxxxx, and all hardware addresses are given in the upper window
> (Medusa/Hades use that window only, and have identity mappings there).
>
> So returning identity mapping in the high window would work AFAICS. I'll
> give that a try sometime.
>
>> Note that on Amiga we only use the identity shortcut for Zorro III
>> memory (and only for the first half?), i.e. ioremap() on Zorro II I/O
>> does add new mappings.  Hence most Zorro II drivers use ZTWO_VADDR()
>> instead of ioremap().
>
> I see ZTWO_VADDR() already returns void __iomem* ... Fixing this patch
> would sort out the m68k drivers, leaving SGI. I'll have to see whether I
> can dig up a SGI workstation with this driver; might be easier than
> getting hold of any Amgias here Dowh Under.
>
> Cheers,
>
>     Michael
>
>
>>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>
>> --
>> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
>> [e-mail]
>>
>> 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
>>

Geert Uytterhoeven July 12, 2022, 6:57 a.m. UTC | #7

Hi Michael,

On Tue, Jul 12, 2022 at 5:27 AM Michael Schmitz <[e-mail]> wrote:
> I just realized the commit message subject for this patch needs
> correcting. Will that mess up your workflow (assuming I retain the
> subject line for the announcement mail)?

Np, I'm used to editing subject lines all the time ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

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

Michael Schmitz July 12, 2022, 7:59 a.m. UTC | #8

Hi Geert,

thanks - v2 sent with these edits just now.

Cheers,

	Michael


Am 12.07.2022 um 18:57 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Tue, Jul 12, 2022 at 5:27 AM Michael Schmitz <[e-mail]> wrote:
>> I just realized the commit message subject for this patch needs
>> correcting. Will that mess up your workflow (assuming I retain the
>> subject line for the announcement mail)?
>
> Np, I'm used to editing subject lines all the time ;-)
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
>
> 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
>

Patch

diff --git a/drivers/scsi/mvme147.c b/drivers/scsi/mvme147.c
index 472fa043094f..aa6476c3e70c 100644
--- a/drivers/scsi/mvme147.c
+++ b/drivers/scsi/mvme147.c
@@ -3,6 +3,7 @@ 
 #include <linux/mm.h>
 #include <linux/blkdev.h>
 #include <linux/interrupt.h>
+#include <linux/platform_device.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -85,40 +86,50 @@  static struct scsi_host_template mvme147_host_template = {
 	.cmd_size		= sizeof(struct scsi_pointer),
 };
 
-static struct Scsi_Host *mvme147_shost;
-
-static int __init mvme147_init(void)
+static int __init mvme147_scsi_probe(struct platform_device *pdev)
 {
+	struct resource *mres, *ires;
+	struct Scsi_Host *mvme147_inst;
 	wd33c93_regs regs;
 	struct WD33C93_hostdata *hdata;
 	int error = -ENOMEM;
 
-	if (!MACH_IS_MVME147)
-		return 0;
+	mres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mres)
+		return -ENODEV;
+
+	ires = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!ires)
+		return -ENODEV;
+
+	if (!request_mem_region(mres->start, resource_size(mres), "wd33c93"))
+		return -EBUSY;
 
-	mvme147_shost = scsi_host_alloc(&mvme147_host_template,
+	mvme147_inst = scsi_host_alloc(&mvme147_host_template,
 			sizeof(struct WD33C93_hostdata));
-	if (!mvme147_shost)
+	if (!mvme147_inst)
 		goto err_out;
-	mvme147_shost->base = 0xfffe4000;
-	mvme147_shost->irq = MVME147_IRQ_SCSI_PORT;
 
-	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;
+
+	hdata = shost_priv(mvme147_inst);
 	hdata->no_sync = 0xff;
 	hdata->fast = 0;
 	hdata->dma_mode = CTRL_DMA;
 
-	wd33c93_init(mvme147_shost, regs, dma_setup, dma_stop, WD33C93_FS_8_10);
+	wd33c93_init(mvme147_inst, regs, dma_setup, dma_stop, WD33C93_FS_8_10);
 
-	error = request_irq(MVME147_IRQ_SCSI_PORT, mvme147_intr, 0,
-			"MVME147 SCSI PORT", mvme147_shost);
+	error = request_irq(ires->start, mvme147_intr, 0,
+			"MVME147 SCSI PORT", mvme147_inst);
 	if (error)
 		goto err_unregister;
-	error = request_irq(MVME147_IRQ_SCSI_DMA, mvme147_intr, 0,
-			"MVME147 SCSI DMA", mvme147_shost);
+	error = request_irq(ires->start+1, mvme147_intr, 0,
+			"MVME147 SCSI DMA", mvme147_inst);
 	if (error)
 		goto err_free_irq;
 #if 0	/* Disabled; causes problems booting */
@@ -133,30 +144,49 @@  static int __init mvme147_init(void)
 	m147_pcc->dma_cntrl = 0x00;	/* ensure DMA is stopped */
 	m147_pcc->dma_intr = 0x89;	/* Ack and enable ints */
 
-	error = scsi_add_host(mvme147_shost, NULL);
+	error = scsi_add_host(mvme147_inst, &pdev->dev);
 	if (error)
 		goto err_free_irq;
-	scsi_scan_host(mvme147_shost);
+
+	platform_set_drvdata(pdev, mvme147_inst);
+
+	scsi_scan_host(mvme147_inst);
 	return 0;
 
 err_free_irq:
-	free_irq(MVME147_IRQ_SCSI_PORT, mvme147_shost);
+	free_irq(ires->start, mvme147_inst);
 err_unregister:
-	scsi_host_put(mvme147_shost);
+	scsi_host_put(mvme147_inst);
 err_out:
 	return error;
 }
 
-static void __exit mvme147_exit(void)
+static int __exit mvme147_scsi_remove(struct platform_device *pdev)
 {
-	scsi_remove_host(mvme147_shost);
+	struct Scsi_Host *mvme147_inst = platform_get_drvdata(pdev);
+	struct resource *mres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct resource *ires = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+
+	scsi_remove_host(mvme147_inst);
 
 	/* XXX Make sure DMA is stopped! */
-	free_irq(MVME147_IRQ_SCSI_PORT, mvme147_shost);
-	free_irq(MVME147_IRQ_SCSI_DMA, mvme147_shost);
+	free_irq(ires->start, mvme147_inst);
+	free_irq(ires->start+1, mvme147_inst);
 
-	scsi_host_put(mvme147_shost);
+	scsi_host_put(mvme147_inst);
+	release_mem_region(mres->start, resource_size(mres));
+	return 0;
 }
 
-module_init(mvme147_init);
-module_exit(mvme147_exit);
+static struct platform_driver mvme147_scsi_driver = {
+	.remove = __exit_p(mvme147_scsi_remove),
+	.driver   = {
+		.name	= "mvme147-scsi",
+	},
+};
+
+module_platform_driver_probe(mvme147_scsi_driver, mvme147_scsi_probe);
+
+MODULE_DESCRIPTION("MVME147 built-in SCSI");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:mvme147-scsi");