Currently the platforms that have very simplistic needs for IDE devices
are forced to invent their own driver for adding the built-in devices.
At the moment ARM is the in-tree user in this category, h8300 could be
switched to something more generic, and SH is roughly in the same
category as ARM.
The only constant that really tends to change between these platforms
are the I/O base and the IRQ, with everything else rather constant. The
ctl base could also change, but everyone seems to keep it at 0x206, so
it's debatable whether it's worth leaving this configurable or not.
I've hacked together a quick driver, 'ide-platform' (for lack of a
better name), that allows for these specifics to be passed in via
platform device resources (registering a device per-port) rather than
having to add more of these things to drivers/ide.
With this we can remove ide_arm (and the h8300 driver can likely be
reworked to use this too), and I don't have to invent a new driver for
SH that does effectively the same thing.
This is intended purely for the simple NO_DMA ide_generic case.. nothing
complicated.
What do people think about this, is there a better way to do this?
Signed-off-by: Paul Mundt <[email protected]>
--
drivers/ide/Makefile | 3 +-
drivers/ide/ide-platform.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
drivers/ide/ide.c | 3 ++
3 files changed, 65 insertions(+), 1 deletion(-)
commit f2da2c8c9b5e648ca9a43d9d3fed9bcd356f0efd
Author: Paul Mundt <[email protected]>
Date: Wed Oct 4 16:41:21 2006 +0900
ide: Simple platform device IDE driver.
This adds a trivial platform device IDE driver. Users are required to
register a couple of resources:
- I/O Base
- CTL Base
- IRQ
Signed-off-by: Paul Mundt <[email protected]>
diff --git a/drivers/ide/Makefile b/drivers/ide/Makefile
index 569fae7..24c9a0f 100644
--- a/drivers/ide/Makefile
+++ b/drivers/ide/Makefile
@@ -13,7 +13,8 @@ EXTRA_CFLAGS += -Idrivers/ide
obj-$(CONFIG_BLK_DEV_IDE) += pci/
-ide-core-y += ide.o ide-io.o ide-iops.o ide-lib.o ide-probe.o ide-taskfile.o
+ide-core-y += ide.o ide-io.o ide-iops.o ide-lib.o ide-probe.o ide-taskfile.o \
+ ide-platform.o
ide-core-$(CONFIG_BLK_DEV_CMD640) += pci/cmd640.o
diff --git a/drivers/ide/ide-platform.c b/drivers/ide/ide-platform.c
new file mode 100644
index 0000000..6d89878
--- /dev/null
+++ b/drivers/ide/ide-platform.c
@@ -0,0 +1,60 @@
+/*
+ * Generic IDE platform device driver
+ *
+ * Copyright (C) 2006 Paul Mundt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+#include <linux/init.h>
+#include <linux/ide.h>
+#include <linux/platform_device.h>
+
+/*
+ * Users use per-port registration with a simple set of 3 resources
+ * per port:
+ * - I/O Base (IORESOURCE_IO)
+ * - CTL Base (IORESOURCE_IO)
+ * - IRQ (IORESOURCE_IRQ)
+ */
+static int __devinit ide_platform_probe(struct platform_device *dev)
+{
+ struct resource *io_res, *ctl_res;
+ hw_regs_t hw;
+
+ if (unlikely(dev->num_resources != 3)) {
+ dev_err(&dev->dev, "invalid number of resources\n");
+ return -EINVAL;
+ }
+
+ io_res = platform_get_resource(dev, IORESOURCE_IO, 0);
+ if (unlikely(io_res == NULL))
+ return -EINVAL;
+
+ ctl_res = platform_get_resource(dev, IORESOURCE_IO, 1);
+ if (unlikely(ctl_res == NULL))
+ return -EINVAL;
+
+ memset(&hw, 0, sizeof(hw_regs_t));
+ ide_std_init_ports(&hw, io_res->start, ctl_res->start);
+
+ hw.irq = platform_get_irq(dev, 0);
+ hw.chipset = ide_generic;
+
+ return ide_register_hw(&hw, NULL);
+}
+
+static struct platform_driver ide_platform_driver = {
+ .probe = ide_platform_probe,
+ .driver = {
+ .name = "ide-platform",
+ .owner = THIS_MODULE,
+ },
+};
+
+void __init ide_platform_init(void)
+{
+ printk(KERN_INFO "ide-platform: platform device IDE interface\n");
+ platform_driver_register(&ide_platform_driver);
+}
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index 287a662..994cdd4 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -1782,6 +1782,7 @@ done:
}
extern void pnpide_init(void);
+extern void ide_platform_init(void);
extern void h8300_ide_init(void);
/*
@@ -1793,6 +1794,8 @@ #ifdef CONFIG_BLK_DEV_IDEPCI
ide_scan_pcibus(ide_scan_direction);
#endif /* CONFIG_BLK_DEV_IDEPCI */
+ ide_platform_init();
+
#ifdef CONFIG_ETRAX_IDE
{
extern void init_e100_ide(void);
Ar Mer, 2006-10-04 am 16:45 +0900, ysgrifennodd Paul Mundt:
> This is intended purely for the simple NO_DMA ide_generic case.. nothing
> complicated.
>
> What do people think about this, is there a better way to do this?
drivers/ide is going away over time. I think the concept is nice and
it's sort of reflected in the libata VLB drivers. I think it would be a
very good way to get good platform drivers for libata for the embedded
platforms.
Moving the existing drivers/ide stuff to a new drivers/ide variant is
wasted work however.
question: where is linux/ide-platform.h header?
On Oct 4, 2006, at 4:45 PM, Paul Mundt wrote:
> Currently the platforms that have very simplistic needs for IDE
> devices
> are forced to invent their own driver for adding the built-in devices.
> At the moment ARM is the in-tree user in this category, h8300 could be
> switched to something more generic, and SH is roughly in the same
> category as ARM.
>
> The only constant that really tends to change between these platforms
> are the I/O base and the IRQ, with everything else rather constant.
> The
> ctl base could also change, but everyone seems to keep it at 0x206, so
> it's debatable whether it's worth leaving this configurable or not.
>
> I've hacked together a quick driver, 'ide-platform' (for lack of a
> better name), that allows for these specifics to be passed in via
> platform device resources (registering a device per-port) rather than
> having to add more of these things to drivers/ide.
>
> With this we can remove ide_arm (and the h8300 driver can likely be
> reworked to use this too), and I don't have to invent a new driver for
> SH that does effectively the same thing.
>
> This is intended purely for the simple NO_DMA ide_generic case..
> nothing
> complicated.
>
> What do people think about this, is there a better way to do this?
>
> Signed-off-by: Paul Mundt <[email protected]>
>
> --
>
> drivers/ide/Makefile | 3 +-
> drivers/ide/ide-platform.c | 60 +++++++++++++++++++++++++++++++++
> ++++++++++++
> drivers/ide/ide.c | 3 ++
> 3 files changed, 65 insertions(+), 1 deletion(-)
>
> commit f2da2c8c9b5e648ca9a43d9d3fed9bcd356f0efd
> Author: Paul Mundt <[email protected]>
> Date: Wed Oct 4 16:41:21 2006 +0900
>
> ide: Simple platform device IDE driver.
>
> This adds a trivial platform device IDE driver. Users are
> required to
> register a couple of resources:
>
> - I/O Base
> - CTL Base
> - IRQ
>
> Signed-off-by: Paul Mundt <[email protected]>
>
> diff --git a/drivers/ide/Makefile b/drivers/ide/Makefile
> index 569fae7..24c9a0f 100644
> --- a/drivers/ide/Makefile
> +++ b/drivers/ide/Makefile
> @@ -13,7 +13,8 @@ EXTRA_CFLAGS += -Idrivers/ide
>
> obj-$(CONFIG_BLK_DEV_IDE) += pci/
>
> -ide-core-y += ide.o ide-io.o ide-iops.o ide-lib.o ide-probe.o ide-
> taskfile.o
> +ide-core-y += ide.o ide-io.o ide-iops.o ide-lib.o ide-probe.o ide-
> taskfile.o \
> + ide-platform.o
>
> ide-core-$(CONFIG_BLK_DEV_CMD640) += pci/cmd640.o
>
> diff --git a/drivers/ide/ide-platform.c b/drivers/ide/ide-platform.c
> new file mode 100644
> index 0000000..6d89878
> --- /dev/null
> +++ b/drivers/ide/ide-platform.c
> @@ -0,0 +1,60 @@
> +/*
> + * Generic IDE platform device driver
> + *
> + * Copyright (C) 2006 Paul Mundt
> + *
> + * This file is subject to the terms and conditions of the GNU
> General Public
> + * License. See the file "COPYING" in the main directory of this
> archive
> + * for more details.
> + */
> +#include <linux/init.h>
> +#include <linux/ide.h>
> +#include <linux/platform_device.h>
> +
> +/*
> + * Users use per-port registration with a simple set of 3 resources
> + * per port:
> + * - I/O Base (IORESOURCE_IO)
> + * - CTL Base (IORESOURCE_IO)
> + * - IRQ (IORESOURCE_IRQ)
> + */
> +static int __devinit ide_platform_probe(struct platform_device *dev)
> +{
> + struct resource *io_res, *ctl_res;
> + hw_regs_t hw;
> +
> + if (unlikely(dev->num_resources != 3)) {
> + dev_err(&dev->dev, "invalid number of resources\n");
> + return -EINVAL;
> + }
> +
> + io_res = platform_get_resource(dev, IORESOURCE_IO, 0);
> + if (unlikely(io_res == NULL))
> + return -EINVAL;
> +
> + ctl_res = platform_get_resource(dev, IORESOURCE_IO, 1);
> + if (unlikely(ctl_res == NULL))
> + return -EINVAL;
> +
> + memset(&hw, 0, sizeof(hw_regs_t));
> + ide_std_init_ports(&hw, io_res->start, ctl_res->start);
> +
> + hw.irq = platform_get_irq(dev, 0);
> + hw.chipset = ide_generic;
> +
> + return ide_register_hw(&hw, NULL);
> +}
> +
> +static struct platform_driver ide_platform_driver = {
> + .probe = ide_platform_probe,
> + .driver = {
> + .name = "ide-platform",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +void __init ide_platform_init(void)
> +{
> + printk(KERN_INFO "ide-platform: platform device IDE interface\n");
> + platform_driver_register(&ide_platform_driver);
> +}
> diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
> index 287a662..994cdd4 100644
> --- a/drivers/ide/ide.c
> +++ b/drivers/ide/ide.c
> @@ -1782,6 +1782,7 @@ done:
> }
>
> extern void pnpide_init(void);
> +extern void ide_platform_init(void);
> extern void h8300_ide_init(void);
>
> /*
> @@ -1793,6 +1794,8 @@ #ifdef CONFIG_BLK_DEV_IDEPCI
> ide_scan_pcibus(ide_scan_direction);
> #endif /* CONFIG_BLK_DEV_IDEPCI */
>
> + ide_platform_init();
> +
> #ifdef CONFIG_ETRAX_IDE
> {
> extern void init_e100_ide(void);
> -
> To unsubscribe from this list: send the line "unsubscribe linux-
> kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Wed, Oct 04, 2006 at 08:30:36PM +0900, girish wrote:
> question: where is linux/ide-platform.h header?
>
answer: there isn't one, as it's not needed. The reason for using
platform devices is so we get _away_ from this ridiculous static set of
definitions for I/O addresses and IRQs that are sprinkled around a lot
of these drivers.
On Wed, Oct 04, 2006 at 12:41:24PM +0100, Alan Cox wrote:
> Ar Mer, 2006-10-04 am 16:45 +0900, ysgrifennodd Paul Mundt:
> > This is intended purely for the simple NO_DMA ide_generic case.. nothing
> > complicated.
> >
> > What do people think about this, is there a better way to do this?
>
> drivers/ide is going away over time.
That was the impression I got.
> I think the concept is nice and it's sort of reflected in the libata
> VLB drivers. I think it would be a very good way to get good platform
> drivers for libata for the embedded platforms.
>
Ok, I wasn't sure if libata was intended for anything outside of the
SATA case (especially non-PCI), but if that's the way to go, I'll look
at hacking something up under libata.
> Moving the existing drivers/ide stuff to a new drivers/ide variant is
> wasted work however.
>
That's what I figured, thanks for clearing it up.
On Oct 5, 2006, at 5:02 AM, Paul Mundt wrote:
> On Wed, Oct 04, 2006 at 08:30:36PM +0900, girish wrote:
>> question: where is linux/ide-platform.h header?
>>
> answer: there isn't one, as it's not needed. The reason for using
> platform devices is so we get _away_ from this ridiculous static
> set of
> definitions for I/O addresses and IRQs that are sprinkled around a lot
> of these drivers.
i wrote wrong file name:
where is linux/platform_device.h file?
as per the patch :
+#include <linux/ide.h>
+#include <linux/platform_device.h>
+
+/*
+ * Users use per-port registration with a simple set of 3 resources
+ * per port:
+ * - I/O Base (IORESOURCE_IO)
+ * - CTL Base (IORESOURCE_IO)
+ * - IRQ (IORESOURCE_IRQ)
+ */
+static int __devinit ide_platform_probe(struct platform_device *dev)
+{
+ struct resource *io_res, *ctl_res;
+ hw_regs_t hw;
+
+ if (unlikely(dev->num_resources != 3)) {
+ dev_err(&dev->dev, "invalid number of resources\n");
+ return -EINVAL;
+ }
+
+ io_res = platform_get_resource(dev, IORESOURCE_IO, 0);
+ if (unlikely(io_res == NULL))
+ return -EINVAL;
+
Ar Iau, 2006-10-05 am 05:05 +0900, ysgrifennodd Paul Mundt:
> Ok, I wasn't sure if libata was intended for anything outside of the
> SATA case (especially non-PCI), but if that's the way to go, I'll look
> at hacking something up under libata.
Take a look at 2.6.18-mm or 2.6.19-* and you should see all you need in
that including the pcmcia driver and other users who set up much the
same way a generic platform device driver would.
On Wed, Oct 04, 2006 at 03:38:45PM +0100, Alan Cox wrote:
> Ar Iau, 2006-10-05 am 05:05 +0900, ysgrifennodd Paul Mundt:
> > Ok, I wasn't sure if libata was intended for anything outside of the
> > SATA case (especially non-PCI), but if that's the way to go, I'll look
> > at hacking something up under libata.
>
> Take a look at 2.6.18-mm or 2.6.19-* and you should see all you need in
> that including the pcmcia driver and other users who set up much the
> same way a generic platform device driver would.
>
Ok, just hacked this together quickly, how does it look? I'm booting
this on an SH-4A (R7780RP) from current git and all works fine..
Thankfully you did most of the heavy lifting :-)
--
drivers/ata/Kconfig | 9 ++
drivers/ata/Makefile | 1
drivers/ata/pata_platform.c | 198 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 208 insertions(+)
commit f257da330416413947c65dac20cefbd39231eb30
Author: Paul Mundt <[email protected]>
Date: Thu Oct 5 18:13:27 2006 +0900
ata: Simple generic platform device libata driver.
This adds a simple generic libata driver for directly-connected
and pre-configured platform devices commonly found on embedded
systems.
For most of these devices, the only variation ends up being the
I/O and CTL bases as well as the IRQ, with everything else
effectively behaving as a standard ISA bus PIO device.
Users of 'pata_platform' are required to pack in 3 resources per
port, namely, the aforementioned variations. Each port must be
registered independently, which doesn't end up being a big problem,
as most of the embedded use cases don't have more than one or two
ports anyways.
Signed-off-by: Paul Mundt <[email protected]>
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 3f4aa0c..70a5a08 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -482,6 +482,15 @@ config PATA_WINBOND
If unsure, say N.
+config PATA_PLATFORM
+ tristate "Generic platform device PATA support"
+ depends on EMBEDDED
+ help
+ This option enables support for generic directly connected ATA
+ devices commonly found on embedded systems.
+
+ If unsure, say N.
+
endif
endmenu
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 72243a6..408c8c9 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_PATA_VIA) += pata_via.o
obj-$(CONFIG_PATA_WINBOND) += pata_sl82c105.o
obj-$(CONFIG_PATA_SIS) += pata_sis.o
obj-$(CONFIG_PATA_TRIFLEX) += pata_triflex.o
+obj-$(CONFIG_PATA_PLATFORM) += pata_platform.o
# Should be last but one libata driver
obj-$(CONFIG_ATA_GENERIC) += ata_generic.o
# Should be last libata driver
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
new file mode 100644
index 0000000..b9de610
--- /dev/null
+++ b/drivers/ata/pata_platform.c
@@ -0,0 +1,198 @@
+/*
+ * Generic platform device PATA driver
+ *
+ * Copyright (C) 2006 Paul Mundt
+ *
+ * Based on pata_pcmcia:
+ *
+ * Copyright 2005-2006 Red Hat Inc <[email protected]>, all rights reserved.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <scsi/scsi_host.h>
+#include <linux/ata.h>
+#include <linux/libata.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME "pata_platform"
+#define DRV_VERSION "0.1.0"
+
+static int pio_mask = 1;
+
+/*
+ * Provide our own set_mode() as we don't want to change anything that has
+ * already been configured..
+ */
+static void pata_platform_set_mode(struct ata_port *ap)
+{
+ int i;
+
+ for (i = 0; i < ATA_MAX_DEVICES; i++) {
+ struct ata_device *dev = &ap->device[i];
+
+ if (ata_dev_enabled(dev)) {
+ /* We don't really care */
+ dev->pio_mode = dev->xfer_mode = XFER_PIO_0;
+ dev->xfer_shift = ATA_SHIFT_PIO;
+ dev->flags |= ATA_DFLAG_PIO;
+ }
+ }
+}
+
+static struct scsi_host_template pata_platform_sht = {
+ .module = THIS_MODULE,
+ .name = DRV_NAME,
+ .ioctl = ata_scsi_ioctl,
+ .queuecommand = ata_scsi_queuecmd,
+ .can_queue = ATA_DEF_QUEUE,
+ .this_id = ATA_SHT_THIS_ID,
+ .sg_tablesize = LIBATA_MAX_PRD,
+ .max_sectors = ATA_MAX_SECTORS,
+ .cmd_per_lun = ATA_SHT_CMD_PER_LUN,
+ .emulated = ATA_SHT_EMULATED,
+ .use_clustering = ATA_SHT_USE_CLUSTERING,
+ .proc_name = DRV_NAME,
+ .dma_boundary = ATA_DMA_BOUNDARY,
+ .slave_configure = ata_scsi_slave_config,
+ .bios_param = ata_std_bios_param,
+};
+
+static struct ata_port_operations pata_platform_port_ops = {
+ .set_mode = pata_platform_set_mode,
+
+ .port_disable = ata_port_disable,
+ .tf_load = ata_tf_load,
+ .tf_read = ata_tf_read,
+ .check_status = ata_check_status,
+ .exec_command = ata_exec_command,
+ .dev_select = ata_std_dev_select,
+
+ .freeze = ata_bmdma_freeze,
+ .thaw = ata_bmdma_thaw,
+ .error_handler = ata_bmdma_error_handler,
+ .post_internal_cmd = ata_bmdma_post_internal_cmd,
+
+ .qc_prep = ata_qc_prep,
+ .qc_issue = ata_qc_issue_prot,
+
+ .data_xfer = ata_pio_data_xfer_noirq,
+
+ .irq_handler = ata_interrupt,
+ .irq_clear = ata_bmdma_irq_clear,
+
+ .port_start = ata_port_start,
+ .port_stop = ata_port_stop,
+ .host_stop = ata_host_stop
+};
+
+/**
+ * pata_platform_probe - attach a platform interface
+ * @pdev: platform device
+ *
+ * Register a platform bus IDE interface. Such interfaces are PIO and we
+ * assume do not support IRQ sharing.
+ *
+ * Platform devices are expected to contain 3 resources per port:
+ *
+ * - I/O Base (IORESOURCE_IO)
+ * - CTL Base (IORESOURCE_IO)
+ * - IRQ (IORESOURCE_IRQ)
+ */
+static int __devinit pata_platform_probe(struct platform_device *pdev)
+{
+ struct resource *io_res, *ctl_res;
+ struct ata_probe_ent ae;
+ int ret = -EBUSY;
+
+ /*
+ * Simple resource validation ..
+ */
+ if (unlikely(pdev->num_resources != 3)) {
+ dev_err(&pdev->dev, "invalid number of resources\n");
+ return -EINVAL;
+ }
+
+ io_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ if (unlikely(io_res == NULL))
+ return -EINVAL;
+
+ ctl_res = platform_get_resource(pdev, IORESOURCE_IO, 1);
+ if (unlikely(ctl_res == NULL))
+ return -EINVAL;
+
+ /*
+ * Now that that's out of the way, wire up the port..
+ */
+ memset(&ae, 0, sizeof(struct ata_probe_ent));
+ INIT_LIST_HEAD(&ae.node);
+ ae.dev = &pdev->dev;
+ ae.port_ops = &pata_platform_port_ops;
+ ae.sht = &pata_platform_sht;
+ ae.n_ports = 1;
+ ae.pio_mask = pio_mask;
+ ae.irq = platform_get_irq(pdev, 0);
+ ae.irq_flags = 0;
+ ae.port_flags = ATA_FLAG_SLAVE_POSS | ATA_FLAG_SRST;
+ ae.port[0].cmd_addr = io_res->start;
+ ae.port[0].altstatus_addr = ctl_res->start;
+ ae.port[0].ctl_addr = ctl_res->start;
+ ata_std_ports(&ae.port[0]);
+
+ ret = ata_device_add(&ae);
+ if (unlikely(ret == 0))
+ return -ENODEV;
+
+ return 0;
+}
+
+/**
+ * pata_platform_remove - unplug a platform interface
+ * @pdev: platform device
+ *
+ * A platform bus ATA device has been unplugged. Perform the needed
+ * cleanup. Also called on module unload for any active devices.
+ */
+static int __devexit pata_platform_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct ata_host *host = dev_get_drvdata(dev);
+
+ ata_host_remove(host);
+ dev_set_drvdata(dev, NULL);
+
+ return 0;
+}
+
+static struct platform_driver pata_platform_driver = {
+ .probe = pata_platform_probe,
+ .remove = __devexit_p(pata_platform_remove),
+ .driver = {
+ .name = DRV_NAME,
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init pata_platform_init(void)
+{
+ return platform_driver_register(&pata_platform_driver);
+}
+
+static void __exit pata_platform_exit(void)
+{
+ platform_driver_unregister(&pata_platform_driver);
+}
+module_init(pata_platform_init);
+module_exit(pata_platform_exit);
+
+module_param(pio_mask, int, 0);
+
+MODULE_AUTHOR("Paul Mundt");
+MODULE_DESCRIPTION("low-level driver for platform device ATA");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(DRV_VERSION);
Hi Matthias,
On Wed, Oct 11, 2006 at 02:50:41PM +0200, Matthias Fuchs wrote:
> I tried you patch on our CPCI405 PowerPC board
> (arch/ppc/platforms/4xx/cpci405.c).
> It seems to work fine together with our onboard CompactFlash slot.
>
> Because our IDE registers are memory mapped, I had to patch the
> resource .start and .end address by subtracting _IO_BASE,
> so that the pata code can use the IO way to talk to the
> IDE registers.
>
> Perhaps it is a good idea to update the pata platform driver to be able to
> handle both _IO and _MEM resources. The _IO resources be be handled
> as it is already done by your code and for _MEM resources the pata platform
> driver can do the ioremapping as I currently do in my board setup.
>
Yes, that's one thing I was thinking of as well.. Here's a patch that
makes an attempt at that, can you give it a try and see if it works for
you? This applies on top of the earlier patch. None of the ARM, SH, or
H8300 cases need to do the remapping at least.
Hopefully this will clean up your setup mess a bit.. If this works for
you, I'll tidy it up a bit and submit the entire patch again.
drivers/ata/pata_platform.c | 103 ++++++++++++++++++++++++++++++++++++++------
1 file changed, 90 insertions(+), 13 deletions(-)
--
diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index 5d98332..a716e63 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -21,7 +21,7 @@ #include <linux/libata.h>
#include <linux/platform_device.h>
#define DRV_NAME "pata_platform"
-#define DRV_VERSION "0.1.0"
+#define DRV_VERSION "0.1.1"
static int pio_mask = 1;
@@ -45,6 +45,23 @@ static void pata_platform_set_mode(struc
}
}
+static void pata_platform_host_stop(struct ata_host *host)
+{
+ int i;
+
+ /*
+ * Unmap the bases for MMIO
+ */
+ for (i = 0; i < host->n_ports; i++) {
+ struct ata_port *ap = host->ports[i];
+
+ if (ap->flags & ATA_FLAG_MMIO) {
+ iounmap((void __iomem *)ap->ioaddr.ctl_addr);
+ iounmap((void __iomem *)ap->ioaddr.cmd_addr);
+ }
+ }
+}
+
static struct scsi_host_template pata_platform_sht = {
.module = THIS_MODULE,
.name = DRV_NAME,
@@ -88,7 +105,7 @@ static struct ata_port_operations pata_p
.port_start = ata_port_start,
.port_stop = ata_port_stop,
- .host_stop = ata_host_stop
+ .host_stop = pata_platform_host_stop
};
/**
@@ -100,14 +117,20 @@ static struct ata_port_operations pata_p
*
* Platform devices are expected to contain 3 resources per port:
*
- * - I/O Base (IORESOURCE_IO)
- * - CTL Base (IORESOURCE_IO)
+ * - I/O Base (IORESOURCE_IO or IORESOURCE_MEM)
+ * - CTL Base (IORESOURCE_IO or IORESOURCE_MEM)
* - IRQ (IORESOURCE_IRQ)
+ *
+ * If the base resources are both mem types, the ioremap() is handled
+ * here. For IORESOURCE_IO, it's assumed that there's no remapping
+ * necessary.
*/
static int __devinit pata_platform_probe(struct platform_device *pdev)
{
struct resource *io_res, *ctl_res;
struct ata_probe_ent ae;
+ unsigned int mmio;
+ int ret;
/*
* Simple resource validation ..
@@ -117,13 +140,31 @@ static int __devinit pata_platform_probe
return -EINVAL;
}
+ /*
+ * Get the I/O base first
+ */
io_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
- if (unlikely(io_res == NULL))
- return -EINVAL;
+ if (io_res == NULL) {
+ io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (unlikely(io_res == NULL))
+ return -EINVAL;
+ }
+ /*
+ * Then the CTL base
+ */
ctl_res = platform_get_resource(pdev, IORESOURCE_IO, 1);
- if (unlikely(ctl_res == NULL))
- return -EINVAL;
+ if (ctl_res == NULL) {
+ ctl_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (unlikely(ctl_res == NULL))
+ return -EINVAL;
+ }
+
+ /*
+ * Check for MMIO
+ */
+ mmio = (( io_res->flags == IORESOURCE_MEM) &&
+ (ctl_res->flags == IORESOURCE_MEM));
/*
* Now that that's out of the way, wire up the port..
@@ -138,15 +179,51 @@ static int __devinit pata_platform_probe
ae.irq = platform_get_irq(pdev, 0);
ae.irq_flags = 0;
ae.port_flags = ATA_FLAG_SLAVE_POSS | ATA_FLAG_SRST;
- ae.port[0].cmd_addr = io_res->start;
- ae.port[0].altstatus_addr = ctl_res->start;
- ae.port[0].ctl_addr = ctl_res->start;
+
+ /*
+ * Handle the MMIO case
+ */
+ if (mmio) {
+ ae.port_flags |= ATA_FLAG_MMIO;
+
+ ae.port[0].cmd_addr = (unsigned long)ioremap(io_res->start,
+ io_res->end - io_res->start + 1);
+ if (unlikely(!ae.port[0].cmd_addr)) {
+ dev_err(&pdev->dev, "failed to remap IO base\n");
+ return -ENXIO;
+ }
+
+ ae.port[0].ctl_addr = (unsigned long)ioremap(ctl_res->start,
+ ctl_res->end - ctl_res->start + 1);
+ if (unlikely(!ae.port[0].ctl_addr)) {
+ dev_err(&pdev->dev, "failed to remap CTL base\n");
+ ret = -ENXIO;
+ goto bad_remap;
+ }
+ } else {
+ ae.port[0].cmd_addr = io_res->start;
+ ae.port[0].ctl_addr = ctl_res->start;
+ }
+
+ ae.port[0].altstatus_addr = ae.port[0].ctl_addr;
+
ata_std_ports(&ae.port[0]);
- if (unlikely(ata_device_add(&ae) == 0))
- return -ENODEV;
+ if (unlikely(ata_device_add(&ae) == 0)) {
+ ret = -ENODEV;
+ goto add_failed;
+ }
return 0;
+
+add_failed:
+ if (ae.port[0].ctl_addr && mmio)
+ iounmap((void __iomem *)ae.port[0].ctl_addr);
+bad_remap:
+ if (ae.port[0].cmd_addr && mmio)
+ iounmap((void __iomem *)ae.port[0].cmd_addr);
+
+ return ret;
}
/**
On Thu, Oct 12, 2006 at 03:13:48PM +0900, Paul Mundt wrote:
> On Wed, Oct 11, 2006 at 02:50:41PM +0200, Matthias Fuchs wrote:
> > Perhaps it is a good idea to update the pata platform driver to be able to
> > handle both _IO and _MEM resources. The _IO resources be be handled
> > as it is already done by your code and for _MEM resources the pata platform
> > driver can do the ioremapping as I currently do in my board setup.
>
> Yes, that's one thing I was thinking of as well.. Here's a patch that
> makes an attempt at that, can you give it a try and see if it works for
> you? This applies on top of the earlier patch. None of the ARM, SH, or
> H8300 cases need to do the remapping at least.
It's likely that ARM will switch over to using the MMIO resources if
this patch makes it in. There's certain ARM platforms which would
benefit from this change (since inb() and friends are more complex
than they necessarily need to be.)
However, one issue needs to be solved before we could do that - how do
we handle the case where the IDE registers are on a 4 byte spacing
interval instead of the usual 1 byte?
I notice that this driver is calling ata_std_ports() which handles
the standard setup. Maybe that needs to become a little more inteligent?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
On Fri, Oct 13, 2006 at 08:52:19AM +0100, Russell King wrote:
> On Thu, Oct 12, 2006 at 03:13:48PM +0900, Paul Mundt wrote:
> > Yes, that's one thing I was thinking of as well.. Here's a patch that
> > makes an attempt at that, can you give it a try and see if it works for
> > you? This applies on top of the earlier patch. None of the ARM, SH, or
> > H8300 cases need to do the remapping at least.
>
> It's likely that ARM will switch over to using the MMIO resources if
> this patch makes it in. There's certain ARM platforms which would
> benefit from this change (since inb() and friends are more complex
> than they necessarily need to be.)
>
> However, one issue needs to be solved before we could do that - how do
> we handle the case where the IDE registers are on a 4 byte spacing
> interval instead of the usual 1 byte?
>
We could solve this in the driver, but it sounds like this is something
that libata should have some visibility of directly.
> I notice that this driver is calling ata_std_ports() which handles
> the standard setup. Maybe that needs to become a little more inteligent?
>
If we go this route, I suppose the easiest option will be simply to have
a private structure with a few function pointers for these sorts of
things, or we can simply have an element for the spacing interval if you
don't forsee needing to change the ioaddrs in any fashion beyond the
register spacing.. Which approach would you be more comfortable with?
Are there any other items that you're concerned with in the current
driver?
Thanks for the feedback!
Paul Mundt wrote:
> On Fri, Oct 13, 2006 at 08:52:19AM +0100, Russell King wrote:
>> On Thu, Oct 12, 2006 at 03:13:48PM +0900, Paul Mundt wrote:
>>> Yes, that's one thing I was thinking of as well.. Here's a patch that
>>> makes an attempt at that, can you give it a try and see if it works for
>>> you? This applies on top of the earlier patch. None of the ARM, SH, or
>>> H8300 cases need to do the remapping at least.
>> It's likely that ARM will switch over to using the MMIO resources if
>> this patch makes it in. There's certain ARM platforms which would
>> benefit from this change (since inb() and friends are more complex
>> than they necessarily need to be.)
>>
>> However, one issue needs to be solved before we could do that - how do
>> we handle the case where the IDE registers are on a 4 byte spacing
>> interval instead of the usual 1 byte?
>>
> We could solve this in the driver, but it sounds like this is something
> that libata should have some visibility of directly.
>
>> I notice that this driver is calling ata_std_ports() which handles
>> the standard setup. Maybe that needs to become a little more inteligent?
>>
> If we go this route, I suppose the easiest option will be simply to have
> a private structure with a few function pointers for these sorts of
> things, or we can simply have an element for the spacing interval if you
> don't forsee needing to change the ioaddrs in any fashion beyond the
> register spacing.. Which approach would you be more comfortable with?
> Are there any other items that you're concerned with in the current
> driver?
Here's the decision matrix for libata: Will the hardware use the
standard taskfile push/pull functions like ata_tf_load(), ata_tf_read(),
ata_exec_command(), etc.? If yes, simply replace ata_std_ports() call
with a call to your own function, written similarly to
pdc_ata_setup_port() in sata_promise.c.
If the hardware requires non-standard I/O accessors, or the register
sizes themselves changed, then you must implement your own taskfile I/O
functions, similar to k2_sata_tf_load(), k2_sata_tf_read(), and
k2_bmdma_setup_mmio() in sata_svw.c. For this case, data in struct
ata_ioports is largely up to you to manage, or ignore.
If there are special command setup or teardown operations, there are
other standard hooks to override as well.
Jeff