[PATCH] ide/macide: Convert Mac IDE driver to platform driver

[PATCH] ide/macide: Convert Mac IDE driver to platform driver

[Finn Thain]

Add platform devices for the Mac IDE controller variants. Convert the
macide module into a platform driver to support two of those variants.
For the third, use a generic "pata_platform" driver instead.
This enables automatic loading of the appropriate module and begins
the process of replacing the driver with libata alternatives.

Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Joshua Thompson <funaho@jurai.org>
References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to platform drivers")
References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy IDE driver")
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
This patch was tested successfully on a Powerbook 190 (MAC_IDE_BABOON)
using both pata_platform and ide_platform drivers.
The next step will be to try using these generic drivers with the other
IDE controller variants (MAC_IDE_QUADRA or MAC_IDE_PB) so that the macide
driver can be entirely replaced with libata drivers.
---
 arch/m68k/mac/config.c | 65 +++++++++++++++++++++++++++++++++++++++
 drivers/ide/macide.c   | 70 ++++++++++++++++++++++++++++--------------
 2 files changed, 112 insertions(+), 23 deletions(-)

diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 5c9f3a2d65388..9ad3a0d3481b1 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -24,6 +24,7 @@
 #include <linux/init.h>
 #include <linux/vt_kern.h>
 #include <linux/platform_device.h>
+#include <linux/ata_platform.h>
 #include <linux/adb.h>
 #include <linux/cuda.h>
 #include <linux/pmu.h>
@@ -940,6 +941,50 @@ static const struct resource mac_scsi_ccl_rsrc[] __initconst = {
 	},
 };
 
+static const struct resource mac_ide_quadra_rsrc[] __initconst = {
+	{
+		.flags = IORESOURCE_MEM,
+		.start = 0x50F1A000,
+		.end   = 0x50F1A103,
+	}, {
+		.flags = IORESOURCE_IRQ,
+		.start = IRQ_NUBUS_F,
+		.end   = IRQ_NUBUS_F,
+	},
+};
+
+static const struct resource mac_ide_pb_rsrc[] __initconst = {
+	{
+		.flags = IORESOURCE_MEM,
+		.start = 0x50F1A000,
+		.end   = 0x50F1A103,
+	}, {
+		.flags = IORESOURCE_IRQ,
+		.start = IRQ_NUBUS_C,
+		.end   = IRQ_NUBUS_C,
+	},
+};
+
+static const struct resource mac_pata_baboon_rsrc[] __initconst = {
+	{
+		.flags = IORESOURCE_MEM,
+		.start = 0x50F1A000,
+		.end   = 0x50F1A037,
+	}, {
+		.flags = IORESOURCE_MEM,
+		.start = 0x50F1A038,
+		.end   = 0x50F1A03B,
+	}, {
+		.flags = IORESOURCE_IRQ | IORESOURCE_IRQ_SHAREABLE,
+		.start = IRQ_BABOON_1,
+		.end   = IRQ_BABOON_1,
+	},
+};
+
+static const struct pata_platform_info mac_pata_baboon_data __initconst = {
+	.ioport_shift  = 2,
+};
+
 int __init mac_platform_init(void)
 {
 	phys_addr_t swim_base = 0;
@@ -1048,6 +1093,26 @@ int __init mac_platform_init(void)
 		break;
 	}
 
+	/*
+	 * IDE device
+	 */
+
+	switch (macintosh_config->ide_type) {
+	case MAC_IDE_QUADRA:
+		platform_device_register_simple("mac_ide", -1,
+			mac_ide_quadra_rsrc, ARRAY_SIZE(mac_ide_quadra_rsrc));
+		break;
+	case MAC_IDE_PB:
+		platform_device_register_simple("mac_ide", -1,
+			mac_ide_pb_rsrc, ARRAY_SIZE(mac_ide_pb_rsrc));
+		break;
+	case MAC_IDE_BABOON:
+		platform_device_register_resndata(NULL, "pata_platform", -1,
+			mac_pata_baboon_rsrc, ARRAY_SIZE(mac_pata_baboon_rsrc),
+			&mac_pata_baboon_data, sizeof(mac_pata_baboon_data));
+		break;
+	}
+
 	/*
 	 * Ethernet device
 	 */
diff --git a/drivers/ide/macide.c b/drivers/ide/macide.c
index 3c6bb8599303b..e16877ce719e9 100644
--- a/drivers/ide/macide.c
+++ b/drivers/ide/macide.c
@@ -18,10 +18,11 @@
 #include <linux/delay.h>
 #include <linux/ide.h>
 #include <linux/module.h>
+#include <linux/platform_device.h>
 
 #include <asm/macintosh.h>
-#include <asm/macints.h>
-#include <asm/mac_baboon.h>
+
+#define DRV_NAME "mac_ide"
 
 #define IDE_BASE 0x50F1A000	/* Base address of IDE controller */
 
@@ -109,42 +110,65 @@ static const char *mac_ide_name[] =
  * Probe for a Macintosh IDE interface
  */
 
-static int __init macide_init(void)
+static int mac_ide_probe(struct platform_device *pdev)
 {
-	unsigned long base;
-	int irq;
+	struct resource *mem, *irq;
 	struct ide_hw hw, *hws[] = { &hw };
 	struct ide_port_info d = macide_port_info;
+	struct ide_host *host;
+	int rc;
 
 	if (!MACH_IS_MAC)
 		return -ENODEV;
 
-	switch (macintosh_config->ide_type) {
-	case MAC_IDE_QUADRA:
-		base = IDE_BASE;
-		irq = IRQ_NUBUS_F;
-		break;
-	case MAC_IDE_PB:
-		base = IDE_BASE;
-		irq = IRQ_NUBUS_C;
-		break;
-	case MAC_IDE_BABOON:
-		base = BABOON_BASE;
-		d.port_ops = NULL;
-		irq = IRQ_BABOON_1;
-		break;
-	default:
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem)
+		return -ENODEV;
+
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!irq)
 		return -ENODEV;
+
+	if (!devm_request_mem_region(&pdev->dev, mem->start,
+				     resource_size(mem), DRV_NAME)) {
+		dev_err(&pdev->dev, "resources busy\n");
+		return -EBUSY;
 	}
 
 	printk(KERN_INFO "ide: Macintosh %s IDE controller\n",
 			 mac_ide_name[macintosh_config->ide_type - 1]);
 
-	macide_setup_ports(&hw, base, irq);
+	macide_setup_ports(&hw, mem->start, irq->start);
+
+	rc = ide_host_add(&d, hws, 1, &host);
+	if (rc)
+		goto release_mem;
+
+	platform_set_drvdata(pdev, host);
+	return 0;
+
+release_mem:
+	release_mem_region(mem->start, resource_size(mem));
+	return rc;
+}
+
+static int mac_ide_remove(struct platform_device *pdev)
+{
+	struct ide_host *host = dev_get_drvdata(&pdev->dev);
 
-	return ide_host_add(&d, hws, 1, NULL);
+	ide_host_remove(host);
+	return 0;
 }
 
-module_init(macide_init);
+static struct platform_driver mac_ide_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe  = mac_ide_probe,
+	.remove = mac_ide_remove,
+};
+
+module_platform_driver(mac_ide_driver);
 
+MODULE_ALIAS("platform:" DRV_NAME);
 MODULE_LICENSE("GPL");
-- 
2.26.2

Re: [PATCH] ide/macide: Convert Mac IDE driver to platform driver

[Geert Uytterhoeven]

Hi Finn,

On Tue, Aug 18, 2020 at 9:45 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> Add platform devices for the Mac IDE controller variants. Convert the
> macide module into a platform driver to support two of those variants.
> For the third, use a generic "pata_platform" driver instead.
> This enables automatic loading of the appropriate module and begins
> the process of replacing the driver with libata alternatives.
>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Joshua Thompson <funaho@jurai.org>
> References: commit 5ed0794cde593 ("m68k/atari: Convert Falcon IDE drivers to platform drivers")
> References: commit 7ad19a99ad431 ("ide: officially deprecated the legacy IDE driver")
> Tested-by: Stan Johnson <userm57@yahoo.com>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Thanks for your patch!

> --- a/arch/m68k/mac/config.c
> +++ b/arch/m68k/mac/config.c

> @@ -940,6 +941,50 @@ static const struct resource mac_scsi_ccl_rsrc[] __initconst = {
>         },
>  };
>
> +static const struct resource mac_ide_quadra_rsrc[] __initconst = {
> +       {
> +               .flags = IORESOURCE_MEM,
> +               .start = 0x50F1A000,
> +               .end   = 0x50F1A103,
> +       }, {
> +               .flags = IORESOURCE_IRQ,
> +               .start = IRQ_NUBUS_F,
> +               .end   = IRQ_NUBUS_F,
> +       },
> +};
> +
> +static const struct resource mac_ide_pb_rsrc[] __initconst = {
> +       {
> +               .flags = IORESOURCE_MEM,
> +               .start = 0x50F1A000,
> +               .end   = 0x50F1A103,
> +       }, {
> +               .flags = IORESOURCE_IRQ,
> +               .start = IRQ_NUBUS_C,
> +               .end   = IRQ_NUBUS_C,
> +       },
> +};

As the above two variants are almost identical, perhaps it makes sense
to drop one of them, drop the const, and override the irq values
dynamically?

> +
> +static const struct resource mac_pata_baboon_rsrc[] __initconst = {
> +       {
> +               .flags = IORESOURCE_MEM,
> +               .start = 0x50F1A000,
> +               .end   = 0x50F1A037,
> +       }, {
> +               .flags = IORESOURCE_MEM,
> +               .start = 0x50F1A038,
> +               .end   = 0x50F1A03B,
> +       }, {
> +               .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_SHAREABLE,
> +               .start = IRQ_BABOON_1,
> +               .end   = IRQ_BABOON_1,
> +       },
> +};
> +
> +static const struct pata_platform_info mac_pata_baboon_data __initconst = {
> +       .ioport_shift  = 2,
> +};

Just wondering: how is this implemented in drivers/ide/macide.c, which
doesn't use the platform info?

> --- a/drivers/ide/macide.c
> +++ b/drivers/ide/macide.c
> @@ -18,10 +18,11 @@
>  #include <linux/delay.h>
>  #include <linux/ide.h>
>  #include <linux/module.h>
> +#include <linux/platform_device.h>
>
>  #include <asm/macintosh.h>
> -#include <asm/macints.h>
> -#include <asm/mac_baboon.h>
> +
> +#define DRV_NAME "mac_ide"
>
>  #define IDE_BASE 0x50F1A000    /* Base address of IDE controller */

Do you still need this definition?
Yes, because it's still used to access IDE_IFR.
Ideally, that should be converted to use the base from the resource,
too.

>
> @@ -109,42 +110,65 @@ static const char *mac_ide_name[] =
>   * Probe for a Macintosh IDE interface
>   */
>
> -static int __init macide_init(void)
> +static int mac_ide_probe(struct platform_device *pdev)
>  {
> -       unsigned long base;
> -       int irq;
> +       struct resource *mem, *irq;
>         struct ide_hw hw, *hws[] = { &hw };
>         struct ide_port_info d = macide_port_info;
> +       struct ide_host *host;
> +       int rc;
>
>         if (!MACH_IS_MAC)
>                 return -ENODEV;
>
> -       switch (macintosh_config->ide_type) {
> -       case MAC_IDE_QUADRA:
> -               base = IDE_BASE;
> -               irq = IRQ_NUBUS_F;
> -               break;
> -       case MAC_IDE_PB:
> -               base = IDE_BASE;
> -               irq = IRQ_NUBUS_C;
> -               break;
> -       case MAC_IDE_BABOON:
> -               base = BABOON_BASE;
> -               d.port_ops = NULL;

How does the driver know not to use the special port_ops after
this change?

> -               irq = IRQ_BABOON_1;
> -               break;
> -       default:
> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!mem)
> +               return -ENODEV;
> +
> +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +       if (!irq)
>                 return -ENODEV;
> +
> +       if (!devm_request_mem_region(&pdev->dev, mem->start,
> +                                    resource_size(mem), DRV_NAME)) {
> +               dev_err(&pdev->dev, "resources busy\n");
> +               return -EBUSY;
>         }
>
>         printk(KERN_INFO "ide: Macintosh %s IDE controller\n",
>                          mac_ide_name[macintosh_config->ide_type - 1]);
>
> -       macide_setup_ports(&hw, base, irq);
> +       macide_setup_ports(&hw, mem->start, irq->start);
> +
> +       rc = ide_host_add(&d, hws, 1, &host);
> +       if (rc)
> +               goto release_mem;
> +
> +       platform_set_drvdata(pdev, host);

In general, it's safer to move the platform_set_drvdata() call before
the ide_host_add() call, as the IDE core may start calling into your
driver as soon as the host has been added.  Fortunately you're using
dev_get_drvdata() in the .remove() callback only, and not in other parts
of the driver.

> +       return 0;
> +
> +release_mem:
> +       release_mem_region(mem->start, resource_size(mem));

Not needed, as you used devm_*() for allocation.

> +       return rc;
> +}
> +
> +static int mac_ide_remove(struct platform_device *pdev)
> +{
> +       struct ide_host *host = dev_get_drvdata(&pdev->dev);
>
> -       return ide_host_add(&d, hws, 1, NULL);
> +       ide_host_remove(host);
> +       return 0;
>  }


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] ide/macide: Convert Mac IDE driver to platform driver

[Finn Thain]

On Wed, 9 Sep 2020, Geert Uytterhoeven wrote:

> 
> Thanks for your patch!
> 

Thanks for your review.

> > --- a/arch/m68k/mac/config.c +++ b/arch/m68k/mac/config.c
> 
> > @@ -940,6 +941,50 @@ static const struct resource mac_scsi_ccl_rsrc[] __initconst = {
> >         },
> >  };
> >
> > +static const struct resource mac_ide_quadra_rsrc[] __initconst = {
> > +       {
> > +               .flags = IORESOURCE_MEM,
> > +               .start = 0x50F1A000,
> > +               .end   = 0x50F1A103,
> > +       }, {
> > +               .flags = IORESOURCE_IRQ,
> > +               .start = IRQ_NUBUS_F,
> > +               .end   = IRQ_NUBUS_F,
> > +       },
> > +};
> > +
> > +static const struct resource mac_ide_pb_rsrc[] __initconst = {
> > +       {
> > +               .flags = IORESOURCE_MEM,
> > +               .start = 0x50F1A000,
> > +               .end   = 0x50F1A103,
> > +       }, {
> > +               .flags = IORESOURCE_IRQ,
> > +               .start = IRQ_NUBUS_C,
> > +               .end   = IRQ_NUBUS_C,
> > +       },
> > +};
> 
> As the above two variants are almost identical, perhaps it makes sense 
> to drop one of them, drop the const, and override the irq values 
> dynamically?
> 

I prefer a declarative or data-driven style, even if it takes a few more 
lines of code. But there is a compromise:

static const struct resource mac_ide_quadra_rsrc[] __initconst = {
	DEFINE_RES_MEM(0x50F1A000, 0x104),
	DEFINE_RES_IRQ(IRQ_NUBUS_F),
}

static const struct resource mac_ide_pb_rsrc[] __initconst = {
	DEFINE_RES_MEM(0x50F1A000, 0x104),
	DEFINE_RES_IRQ(IRQ_NUBUS_C),
}

The reason I didn't use these macros was to avoid making the reader go and 
look up their definitions. Anyway, would that style be preferred here?

I could do the same with the mac_ide_baboon_rsrc[] initializer:

static const struct resource mac_pata_baboon_rsrc[] __initconst = {
        DEFINE_RES_MEM(0x50F1A000, 0x38),
        DEFINE_RES_MEM(0x50F1A038, 0x04),
        DEFINE_RES_IRQ(IRQ_BABOON_1),
};

... but that would lose the IORESOURCE_IRQ_SHAREABLE flag. I'm not sure 
whether that matters (it's a vestige of macide.c).

> > +
> > +static const struct resource mac_pata_baboon_rsrc[] __initconst = {
> > +       {
> > +               .flags = IORESOURCE_MEM,
> > +               .start = 0x50F1A000,
> > +               .end   = 0x50F1A037,
> > +       }, {
> > +               .flags = IORESOURCE_MEM,
> > +               .start = 0x50F1A038,
> > +               .end   = 0x50F1A03B,
> > +       }, {
> > +               .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_SHAREABLE,
> > +               .start = IRQ_BABOON_1,
> > +               .end   = IRQ_BABOON_1,
> > +       },
> > +};
> > +
> > +static const struct pata_platform_info mac_pata_baboon_data __initconst = {
> > +       .ioport_shift  = 2,
> > +};
> 
> Just wondering: how is this implemented in drivers/ide/macide.c, which
> doesn't use the platform info?
> 

That factor of 4 is embedded in the address caclulation:

        for (i = 0; i < 8; i++)
                hw->io_ports_array[i] = base + i * 4;

> > --- a/drivers/ide/macide.c
> > +++ b/drivers/ide/macide.c
> > @@ -18,10 +18,11 @@
> >  #include <linux/delay.h>
> >  #include <linux/ide.h>
> >  #include <linux/module.h>
> > +#include <linux/platform_device.h>
> >
> >  #include <asm/macintosh.h>
> > -#include <asm/macints.h>
> > -#include <asm/mac_baboon.h>
> > +
> > +#define DRV_NAME "mac_ide"
> >
> >  #define IDE_BASE 0x50F1A000    /* Base address of IDE controller */
> 
> Do you still need this definition?
> Yes, because it's still used to access IDE_IFR.
> Ideally, that should be converted to use the base from the resource,
> too.
> 

Yes, that was my thought too. I can make the change if you like, but I 
can't test it until I set up the appropriate hardware (MAC_IDE_QUADRA or 
MAC_IDE_PB). I do own that hardware but it is located in Melbourne and it 
is now illegal to visit Melbourne without official papers. Besides, once I 
can test on that hardware I can replace the entire driver anyway, and 
this kind of refactoring would become moot.

> >
> > @@ -109,42 +110,65 @@ static const char *mac_ide_name[] =
> >   * Probe for a Macintosh IDE interface
> >   */
> >
> > -static int __init macide_init(void)
> > +static int mac_ide_probe(struct platform_device *pdev)
> >  {
> > -       unsigned long base;
> > -       int irq;
> > +       struct resource *mem, *irq;
> >         struct ide_hw hw, *hws[] = { &hw };
> >         struct ide_port_info d = macide_port_info;
> > +       struct ide_host *host;
> > +       int rc;
> >
> >         if (!MACH_IS_MAC)
> >                 return -ENODEV;
> >
> > -       switch (macintosh_config->ide_type) {
> > -       case MAC_IDE_QUADRA:
> > -               base = IDE_BASE;
> > -               irq = IRQ_NUBUS_F;
> > -               break;
> > -       case MAC_IDE_PB:
> > -               base = IDE_BASE;
> > -               irq = IRQ_NUBUS_C;
> > -               break;
> > -       case MAC_IDE_BABOON:
> > -               base = BABOON_BASE;
> > -               d.port_ops = NULL;
> 
> How does the driver know not to use the special port_ops after
> this change?
> 

The driver always uses the special port_ops after this change because it 
no longer handles the MAC_IDE_BABOON case. That case is handled by either 
drivers/ata/pata_platform.c or drivers/ide/ide_platform.c, depending on 
.config.

> > -               irq = IRQ_BABOON_1;
> > -               break;
> > -       default:
> > +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!mem)
> > +               return -ENODEV;
> > +
> > +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +       if (!irq)
> >                 return -ENODEV;
> > +
> > +       if (!devm_request_mem_region(&pdev->dev, mem->start,
> > +                                    resource_size(mem), DRV_NAME)) {
> > +               dev_err(&pdev->dev, "resources busy\n");
> > +               return -EBUSY;
> >         }
> >
> >         printk(KERN_INFO "ide: Macintosh %s IDE controller\n",
> >                          mac_ide_name[macintosh_config->ide_type - 1]);
> >
> > -       macide_setup_ports(&hw, base, irq);
> > +       macide_setup_ports(&hw, mem->start, irq->start);
> > +
> > +       rc = ide_host_add(&d, hws, 1, &host);
> > +       if (rc)
> > +               goto release_mem;
> > +
> > +       platform_set_drvdata(pdev, host);
> 
> In general, it's safer to move the platform_set_drvdata() call before
> the ide_host_add() call, as the IDE core may start calling into your
> driver as soon as the host has been added.  Fortunately you're using
> dev_get_drvdata() in the .remove() callback only, and not in other parts
> of the driver.
> 

Right.

> > +       return 0;
> > +
> > +release_mem:
> > +       release_mem_region(mem->start, resource_size(mem));
> 
> Not needed, as you used devm_*() for allocation.
> 

OK, I'll remove this. I put it there after I looked at falconide.c and 
wondered whether the automatic release would take place after both init 
failure and exit (or just exit). I see now that pata_gayle.c does it 
differently.

> > +       return rc;
> > +}
> > +
> > +static int mac_ide_remove(struct platform_device *pdev)
> > +{
> > +       struct ide_host *host = dev_get_drvdata(&pdev->dev);
> >
> > -       return ide_host_add(&d, hws, 1, NULL);
> > +       ide_host_remove(host);
> > +       return 0;
> >  }
> 
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH] ide/macide: Convert Mac IDE driver to platform driver

[Michael Schmitz]

Hi Finn,

Am 10.09.2020 um 12:23 schrieb Finn Thain:
>>> +       return 0;
>>> +
>>> +release_mem:
>>> +       release_mem_region(mem->start, resource_size(mem));
>>
>> Not needed, as you used devm_*() for allocation.
>>
>
> OK, I'll remove this. I put it there after I looked at falconide.c and
> wondered whether the automatic release would take place after both init
> failure and exit (or just exit). I see now that pata_gayle.c does it
> differently.

pata_gayle.c has probably seen more testing (in the platform 
environment), so I'd go with what's done there.

Cheers,

	Michael

Re: [PATCH] ide/macide: Convert Mac IDE driver to platform driver

[Geert Uytterhoeven]

Hi Finn,

On Thu, Sep 10, 2020 at 2:23 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Wed, 9 Sep 2020, Geert Uytterhoeven wrote:
> > > --- a/arch/m68k/mac/config.c +++ b/arch/m68k/mac/config.c
> >
> > > @@ -940,6 +941,50 @@ static const struct resource mac_scsi_ccl_rsrc[] __initconst = {
> > >         },
> > >  };
> > >
> > > +static const struct resource mac_ide_quadra_rsrc[] __initconst = {
> > > +       {
> > > +               .flags = IORESOURCE_MEM,
> > > +               .start = 0x50F1A000,
> > > +               .end   = 0x50F1A103,
> > > +       }, {
> > > +               .flags = IORESOURCE_IRQ,
> > > +               .start = IRQ_NUBUS_F,
> > > +               .end   = IRQ_NUBUS_F,
> > > +       },
> > > +};
> > > +
> > > +static const struct resource mac_ide_pb_rsrc[] __initconst = {
> > > +       {
> > > +               .flags = IORESOURCE_MEM,
> > > +               .start = 0x50F1A000,
> > > +               .end   = 0x50F1A103,
> > > +       }, {
> > > +               .flags = IORESOURCE_IRQ,
> > > +               .start = IRQ_NUBUS_C,
> > > +               .end   = IRQ_NUBUS_C,
> > > +       },
> > > +};
> >
> > As the above two variants are almost identical, perhaps it makes sense
> > to drop one of them, drop the const, and override the irq values
> > dynamically?
> >
>
> I prefer a declarative or data-driven style, even if it takes a few more
> lines of code. But there is a compromise:
>
> static const struct resource mac_ide_quadra_rsrc[] __initconst = {
>         DEFINE_RES_MEM(0x50F1A000, 0x104),
>         DEFINE_RES_IRQ(IRQ_NUBUS_F),
> }
>
> static const struct resource mac_ide_pb_rsrc[] __initconst = {
>         DEFINE_RES_MEM(0x50F1A000, 0x104),
>         DEFINE_RES_IRQ(IRQ_NUBUS_C),
> }
>
> The reason I didn't use these macros was to avoid making the reader go and
> look up their definitions. Anyway, would that style be preferred here?

I think the DEFINE_RES_*() are sufficiently common (well, in pre-DT
platforms ;-)

> I could do the same with the mac_ide_baboon_rsrc[] initializer:
>
> static const struct resource mac_pata_baboon_rsrc[] __initconst = {
>         DEFINE_RES_MEM(0x50F1A000, 0x38),
>         DEFINE_RES_MEM(0x50F1A038, 0x04),
>         DEFINE_RES_IRQ(IRQ_BABOON_1),
> };
>
> ... but that would lose the IORESOURCE_IRQ_SHAREABLE flag. I'm not sure
> whether that matters (it's a vestige of macide.c).

You can still use DEFINE_RES_NAMED() to pass the flags.
Would you consider that to be a good compromise?

> > > +static const struct resource mac_pata_baboon_rsrc[] __initconst = {
> > > +       {
> > > +               .flags = IORESOURCE_MEM,
> > > +               .start = 0x50F1A000,
> > > +               .end   = 0x50F1A037,
> > > +       }, {
> > > +               .flags = IORESOURCE_MEM,
> > > +               .start = 0x50F1A038,
> > > +               .end   = 0x50F1A03B,
> > > +       }, {
> > > +               .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_SHAREABLE,
> > > +               .start = IRQ_BABOON_1,
> > > +               .end   = IRQ_BABOON_1,
> > > +       },
> > > +};
> > > +
> > > +static const struct pata_platform_info mac_pata_baboon_data __initconst = {
> > > +       .ioport_shift  = 2,
> > > +};
> >
> > Just wondering: how is this implemented in drivers/ide/macide.c, which
> > doesn't use the platform info?
>
> That factor of 4 is embedded in the address caclulation:
>
>         for (i = 0; i < 8; i++)
>                 hw->io_ports_array[i] = base + i * 4;

IC. But in the new code, the platform info is passed for Baboon only,
while the old code used it for all variants.

> > > --- a/drivers/ide/macide.c
> > > +++ b/drivers/ide/macide.c
> > > @@ -18,10 +18,11 @@
> > >  #include <linux/delay.h>
> > >  #include <linux/ide.h>
> > >  #include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > >
> > >  #include <asm/macintosh.h>
> > > -#include <asm/macints.h>
> > > -#include <asm/mac_baboon.h>
> > > +
> > > +#define DRV_NAME "mac_ide"
> > >
> > >  #define IDE_BASE 0x50F1A000    /* Base address of IDE controller */
> >
> > Do you still need this definition?
> > Yes, because it's still used to access IDE_IFR.
> > Ideally, that should be converted to use the base from the resource,
> > too.
> >
>
> Yes, that was my thought too. I can make the change if you like, but I
> can't test it until I set up the appropriate hardware (MAC_IDE_QUADRA or
> MAC_IDE_PB). I do own that hardware but it is located in Melbourne and it
> is now illegal to visit Melbourne without official papers. Besides, once I
> can test on that hardware I can replace the entire driver anyway, and
> this kind of refactoring would become moot.

OK.

> > > @@ -109,42 +110,65 @@ static const char *mac_ide_name[] =
> > >   * Probe for a Macintosh IDE interface
> > >   */
> > >
> > > -static int __init macide_init(void)
> > > +static int mac_ide_probe(struct platform_device *pdev)
> > >  {
> > > -       unsigned long base;
> > > -       int irq;
> > > +       struct resource *mem, *irq;
> > >         struct ide_hw hw, *hws[] = { &hw };
> > >         struct ide_port_info d = macide_port_info;
> > > +       struct ide_host *host;
> > > +       int rc;
> > >
> > >         if (!MACH_IS_MAC)
> > >                 return -ENODEV;
> > >
> > > -       switch (macintosh_config->ide_type) {
> > > -       case MAC_IDE_QUADRA:
> > > -               base = IDE_BASE;
> > > -               irq = IRQ_NUBUS_F;
> > > -               break;
> > > -       case MAC_IDE_PB:
> > > -               base = IDE_BASE;
> > > -               irq = IRQ_NUBUS_C;
> > > -               break;
> > > -       case MAC_IDE_BABOON:
> > > -               base = BABOON_BASE;
> > > -               d.port_ops = NULL;
> >
> > How does the driver know not to use the special port_ops after
> > this change?
> >
>
> The driver always uses the special port_ops after this change because it
> no longer handles the MAC_IDE_BABOON case. That case is handled by either

non-MAC_IDE_BABOON case?

> drivers/ata/pata_platform.c or drivers/ide/ide_platform.c, depending on
> .config.

Ideally, we do need to differentiate, right?

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] ide/macide: Convert Mac IDE driver to platform driver

[Finn Thain]

On Thu, 10 Sep 2020, Geert Uytterhoeven wrote:

> On Thu, Sep 10, 2020 at 2:23 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > I prefer a declarative or data-driven style, even if it takes a few 
> > more lines of code. But there is a compromise:
> >
> > static const struct resource mac_ide_quadra_rsrc[] __initconst = {
> >         DEFINE_RES_MEM(0x50F1A000, 0x104),
> >         DEFINE_RES_IRQ(IRQ_NUBUS_F),
> > }
> >
> > static const struct resource mac_ide_pb_rsrc[] __initconst = {
> >         DEFINE_RES_MEM(0x50F1A000, 0x104),
> >         DEFINE_RES_IRQ(IRQ_NUBUS_C),
> > }
> >
> > The reason I didn't use these macros was to avoid making the reader go and
> > look up their definitions. Anyway, would that style be preferred here?
> 
> I think the DEFINE_RES_*() are sufficiently common (well, in pre-DT
> platforms ;-)
> 

OK, I'll use the macros in v2.

> > I could do the same with the mac_ide_baboon_rsrc[] initializer:
> >
> > static const struct resource mac_pata_baboon_rsrc[] __initconst = {
> >         DEFINE_RES_MEM(0x50F1A000, 0x38),
> >         DEFINE_RES_MEM(0x50F1A038, 0x04),
> >         DEFINE_RES_IRQ(IRQ_BABOON_1),
> > };
> >
> > ... but that would lose the IORESOURCE_IRQ_SHAREABLE flag. I'm not sure
> > whether that matters (it's a vestige of macide.c).
> 
> You can still use DEFINE_RES_NAMED() to pass the flags.
> Would you consider that to be a good compromise?
> 

Sure. I was happy with v1. I'm not that worried about brevity.

Anyway, the question remains, is the flag actually needed. I can't see a 
need for it so I'll omit the flag in v2 and ask Stan to test again.

> > > > +static const struct resource mac_pata_baboon_rsrc[] __initconst = {
> > > > +       {
> > > > +               .flags = IORESOURCE_MEM,
> > > > +               .start = 0x50F1A000,
> > > > +               .end   = 0x50F1A037,
> > > > +       }, {
> > > > +               .flags = IORESOURCE_MEM,
> > > > +               .start = 0x50F1A038,
> > > > +               .end   = 0x50F1A03B,
> > > > +       }, {
> > > > +               .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_SHAREABLE,
> > > > +               .start = IRQ_BABOON_1,
> > > > +               .end   = IRQ_BABOON_1,
> > > > +       },
> > > > +};
> > > > +
> > > > +static const struct pata_platform_info mac_pata_baboon_data __initconst = {
> > > > +       .ioport_shift  = 2,
> > > > +};
> > >
> > > Just wondering: how is this implemented in drivers/ide/macide.c, which
> > > doesn't use the platform info?
> >
> > That factor of 4 is embedded in the address caclulation:
> >
> >         for (i = 0; i < 8; i++)
> >                 hw->io_ports_array[i] = base + i * 4;
> 
> IC. But in the new code, the platform info is passed for Baboon only,
> while the old code used it for all variants.
> 

Correct. Is that a problem for some reason?

> > > > --- a/drivers/ide/macide.c
> > > > +++ b/drivers/ide/macide.c
> > > > @@ -18,10 +18,11 @@
> > > >  #include <linux/delay.h>
> > > >  #include <linux/ide.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/platform_device.h>
> > > >
> > > >  #include <asm/macintosh.h>
> > > > -#include <asm/macints.h>
> > > > -#include <asm/mac_baboon.h>
> > > > +
> > > > +#define DRV_NAME "mac_ide"
> > > >
> > > >  #define IDE_BASE 0x50F1A000    /* Base address of IDE controller */
> > >
> > > Do you still need this definition?
> > > Yes, because it's still used to access IDE_IFR.
> > > Ideally, that should be converted to use the base from the resource,
> > > too.
> > >
> >
> > Yes, that was my thought too. I can make the change if you like, but I
> > can't test it until I set up the appropriate hardware (MAC_IDE_QUADRA or
> > MAC_IDE_PB). I do own that hardware but it is located in Melbourne and it
> > is now illegal to visit Melbourne without official papers. Besides, once I
> > can test on that hardware I can replace the entire driver anyway, and
> > this kind of refactoring would become moot.
> 
> OK.
> 
> > > > @@ -109,42 +110,65 @@ static const char *mac_ide_name[] =
> > > >   * Probe for a Macintosh IDE interface
> > > >   */
> > > >
> > > > -static int __init macide_init(void)
> > > > +static int mac_ide_probe(struct platform_device *pdev)
> > > >  {
> > > > -       unsigned long base;
> > > > -       int irq;
> > > > +       struct resource *mem, *irq;
> > > >         struct ide_hw hw, *hws[] = { &hw };
> > > >         struct ide_port_info d = macide_port_info;
> > > > +       struct ide_host *host;
> > > > +       int rc;
> > > >
> > > >         if (!MACH_IS_MAC)
> > > >                 return -ENODEV;
> > > >
> > > > -       switch (macintosh_config->ide_type) {
> > > > -       case MAC_IDE_QUADRA:
> > > > -               base = IDE_BASE;
> > > > -               irq = IRQ_NUBUS_F;
> > > > -               break;
> > > > -       case MAC_IDE_PB:
> > > > -               base = IDE_BASE;
> > > > -               irq = IRQ_NUBUS_C;
> > > > -               break;
> > > > -       case MAC_IDE_BABOON:
> > > > -               base = BABOON_BASE;
> > > > -               d.port_ops = NULL;
> > >
> > > How does the driver know not to use the special port_ops after
> > > this change?
> > >
> >
> > The driver always uses the special port_ops after this change because it
> > no longer handles the MAC_IDE_BABOON case. That case is handled by either
> 
> non-MAC_IDE_BABOON case?
> 
> > drivers/ata/pata_platform.c or drivers/ide/ide_platform.c, depending on
> > .config.
> 
> Ideally, we do need to differentiate, right?
> 

Sorry, I'm lost.

The commit log explains that the macide driver is only intended to support 
two of the three variants, because the generic drivers are already able to 
support the third variant (Baboon). Stan confirmed this on his PB 190.

But, IIUC, you seem to want macide to continue to support all three 
variants (?) The existing code to implement that has an old bug that 
reassigns d.port_ops when it is const. IMO, the const is correct because 
macide should concern itself with mac hardware quirks and should not try 
to mimic a generic driver by setting d.port_ops = NULL. Does that make 
sense?

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 

Re: [PATCH] ide/macide: Convert Mac IDE driver to platform driver

[Geert Uytterhoeven]

Hi Finn,

On Fri, Sep 11, 2020 at 1:05 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Thu, 10 Sep 2020, Geert Uytterhoeven wrote:
> > On Thu, Sep 10, 2020 at 2:23 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > > > How does the driver know not to use the special port_ops after
> > > > this change?
> > > >
> > >
> > > The driver always uses the special port_ops after this change because it
> > > no longer handles the MAC_IDE_BABOON case. That case is handled by either
> >
> > non-MAC_IDE_BABOON case?
> >
> > > drivers/ata/pata_platform.c or drivers/ide/ide_platform.c, depending on
> > > .config.
> >
> > Ideally, we do need to differentiate, right?
> >
>
> Sorry, I'm lost.
>
> The commit log explains that the macide driver is only intended to support
> two of the three variants, because the generic drivers are already able to
> support the third variant (Baboon). Stan confirmed this on his PB 190.
>
> But, IIUC, you seem to want macide to continue to support all three
> variants (?) The existing code to implement that has an old bug that
> reassigns d.port_ops when it is const. IMO, the const is correct because
> macide should concern itself with mac hardware quirks and should not try
> to mimic a generic driver by setting d.port_ops = NULL. Does that make
> sense?

Sorry, I completely missed that the Baboon case registers a "pata_platform"
device instead of a "macide" device.  Hence please ignore my comments
related to that.  Sorry for the noise.

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] ide/macide: Convert Mac IDE driver to platform driver

[Finn Thain]

On Fri, 11 Sep 2020, Geert Uytterhoeven wrote:

> 
> Sorry, I completely missed that the Baboon case registers a 
> "pata_platform" device instead of a "macide" device.  Hence please 
> ignore my comments related to that.  Sorry for the noise.
> 

No problem. That misunderstanding got me thinking about implications 
stemming from my patch that I may have overlooked. Here's what I found.

1) Your presumption that the old macide driver would keep supporting all 
variants does make sense, as that would delay disruption for as long as 
possible (i.e. for as long as the IDE subsystem remains).

However, if my patch does not get merged until 2021, that disruption would 
not arrive earlier than promised by commit 7ad19a99ad431 ("ide: officially 
deprecated the legacy IDE driver").

2) My patch omitted a mac_defconfig update that would enable an 
alternative driver for the Baboon case. I will remedy this in v2.

3) It turns out that the Debian/m68k kernel config has 
CONFIG_BLK_DEV_PLATFORM=m. This will work fine with this patch. (I assume 
that Debian developers will replace CONFIG_BLK_DEV_PLATFORM with 
CONFIG_PATA_PLATFORM prior to the removal of the IDE subsystem next year.)