Message ID | 00ee44fe6ecdce1c783c3cc3b1b9a62b498dcdb2.1597736545.git.fthain@telegraphics.com.au |
---|---|
State | Superseded |
Delegated to: | David Miller |
Headers | show |
Series | ide/macide: Convert Mac IDE driver to platform driver | expand |
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
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 >
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
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
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 > >
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
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.)
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");