[PATCH 1/2] [IDE] Platform IDE driver (was: MMIO IDE driver)

This is now very similar to pata_platform.c, they both use
same platform data structure and same resources.

To achieve that, byte_lanes_swapping platform data variable
and platform specified iops removed from that driver. It's fine,
since those were never used anyway.

pata_platform and ide_platform are carrying same driver names,
to easily switch between these drivers, without need to touch
platform code.

Signed-off-by: Anton Vorontsov <redacted>
Signed-off-by: Vitaly Bordug <redacted>

---

 drivers/ide/Kconfig               |    8 ++
 drivers/ide/legacy/Makefile       |    2 
 drivers/ide/legacy/ide_platform.c |  181 +++++++++++++++++++++++++++++++++++++
 3 files changed, 191 insertions(+), 0 deletions(-)

diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
index b1a9b81..b5a44da 100644
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig

@@ -308,6 +308,14 @@ config IDE_GENERIC
 	help
 	  If unsure, say Y.
 
+config BLK_DEV_PLATFORM
+	tristate "Platform driver for IDE interfaces"
+	help
+	  This is the platform IDE driver, used mostly for Memory Mapped
+	  IDE devices, like Compact Flashes running in True IDE mode.
+
+	  If unsure, say N.
+
 config BLK_DEV_CMD640
 	bool "CMD640 chipset bugfix/support"
 	depends on X86

diff --git a/drivers/ide/legacy/Makefile b/drivers/ide/legacy/Makefile
index c797106..4098223 100644
--- a/drivers/ide/legacy/Makefile
+++ b/drivers/ide/legacy/Makefile

@@ -7,6 +7,8 @@ obj-$(CONFIG_BLK_DEV_UMC8672)		+= umc8672.o
 
 obj-$(CONFIG_BLK_DEV_IDECS)		+= ide-cs.o
 
+obj-$(CONFIG_BLK_DEV_PLATFORM)		+= ide_platform.o
+
 # Last of all
 obj-$(CONFIG_BLK_DEV_HD)		+= hd.o
 

diff --git a/drivers/ide/legacy/ide_platform.c b/drivers/ide/legacy/ide_platform.c
new file mode 100644
index 0000000..0b3f5b5
--- /dev/null
+++ b/drivers/ide/legacy/ide_platform.c

@@ -0,0 +1,181 @@
+/*
+ * Platform IDE driver
+ *
+ * Copyright (C) 2007 MontaVista Software
+ *
+ * Maintainer: Kumar Gala <galak@kernel.crashing.org>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/ide.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/pata_platform.h>
+#include <linux/io.h>
+
+static struct {
+	void __iomem *plat_ide_mapbase;
+	void __iomem *plat_ide_alt_mapbase;
+	ide_hwif_t *hwif;
+	int index;
+} hwif_prop;
+
+static ide_hwif_t *__devinit plat_ide_locate_hwif(void __iomem *base,
+	    void __iomem *ctrl, struct pata_platform_info *pdata, int irq,
+	    int mmio)
+{
+	unsigned long port = (unsigned long)base;
+	ide_hwif_t *hwif;
+	int index, i;
+
+	for (index = 0; index < MAX_HWIFS; ++index) {
+		hwif = ide_hwifs + index;
+		if (hwif->io_ports[IDE_DATA_OFFSET] == port)
+			goto found;
+	}
+
+	for (index = 0; index < MAX_HWIFS; ++index) {
+		hwif = ide_hwifs + index;
+		if (hwif->io_ports[IDE_DATA_OFFSET] == 0)
+			goto found;
+	}
+
+	return NULL;
+
+found:
+
+	hwif->hw.io_ports[IDE_DATA_OFFSET] = port;
+
+	port += (1 << pdata->ioport_shift);
+	for (i = IDE_ERROR_OFFSET; i <= IDE_STATUS_OFFSET;
+	     i++, port += (1 << pdata->ioport_shift))
+		hwif->hw.io_ports[i] = port;
+
+	hwif->hw.io_ports[IDE_CONTROL_OFFSET] = (unsigned long)ctrl;
+
+	memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->hw.io_ports));
+	hwif->hw.irq = hwif->irq = irq;
+
+	hwif->hw.dma = NO_DMA;
+	hwif->hw.chipset = ide_generic;
+
+	if (mmio) {
+		hwif->mmio = 1;
+		default_hwif_mmiops(hwif);
+	}
+
+	hwif_prop.hwif = hwif;
+	hwif_prop.index = index;
+
+	return hwif;
+}
+
+static int __devinit plat_ide_probe(struct platform_device *pdev)
+{
+	struct resource *res_base, *res_alt, *res_irq;
+	ide_hwif_t *hwif;
+	struct pata_platform_info *pdata;
+	int ret = 0;
+	int mmio = 0;
+
+	pdata = pdev->dev.platform_data;
+
+	/* get a pointer to the register memory */
+	res_base = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	res_alt = platform_get_resource(pdev, IORESOURCE_IO, 1);
+
+	if (!res_base || !res_alt) {
+		res_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		res_alt = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (!res_base || !res_alt) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		mmio = 1;
+	}
+
+	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res_irq) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (mmio) {
+		hwif_prop.plat_ide_mapbase = devm_ioremap(&pdev->dev,
+			res_base->start, res_base->end - res_base->start + 1);
+		hwif_prop.plat_ide_alt_mapbase = devm_ioremap(&pdev->dev,
+			res_alt->start, res_alt->end - res_alt->start + 1);
+	} else {
+		hwif_prop.plat_ide_mapbase = devm_ioport_map(&pdev->dev,
+			res_base->start, res_base->end - res_base->start + 1);
+		hwif_prop.plat_ide_alt_mapbase = devm_ioport_map(&pdev->dev,
+			res_alt->start, res_alt->end - res_alt->start + 1);
+	}
+
+	hwif = plat_ide_locate_hwif(hwif_prop.plat_ide_mapbase,
+	         hwif_prop.plat_ide_alt_mapbase, pdata, res_irq->start, mmio);
+
+	if (!hwif) {
+		ret = -ENODEV;
+		goto out;
+	}
+	hwif->gendev.parent = &pdev->dev;
+	hwif->noprobe = 0;
+
+	probe_hwif_init(hwif);
+
+	platform_set_drvdata(pdev, hwif);
+	ide_proc_register_port(hwif);
+
+	return 0;
+
+out:
+	return ret;
+}
+
+static int __devexit plat_ide_remove(struct platform_device *pdev)
+{
+	ide_hwif_t *hwif = pdev->dev.driver_data;
+
+	if (hwif != hwif_prop.hwif) {
+		dev_printk(KERN_DEBUG, &pdev->dev, "%s: hwif value error",
+		           pdev->name);
+	} else {
+		ide_unregister(hwif_prop.index);
+		hwif_prop.index = 0;
+		hwif_prop.hwif = NULL;
+	}
+
+	return 0;
+}
+
+static struct platform_driver platform_ide_driver = {
+	.driver {
+		.name = "pata_platform",
+	},
+	.probe = plat_ide_probe,
+	.remove = __devexit_p(plat_ide_remove),
+};
+
+static int __init platform_ide_init(void)
+{
+	return platform_driver_register(&platform_ide_driver);
+}
+
+static void __exit platform_ide_exit(void)
+{
+	platform_driver_unregister(&platform_ide_driver);
+}
+
+MODULE_DESCRIPTION("Platform IDE driver");
+MODULE_LICENSE("GPL");
+
+module_init(platform_ide_init);
+module_exit(platform_ide_exit);

This updates relevant platform code (freescale mpc8349itx target) 
to make the CompactFlash work in TrueIDE mode. 

Signed-off-by: Anton Vorontsov <redacted>
Signed-off-by: Vitaly Bordug <redacted>

---

 arch/powerpc/boot/dts/mpc8349emitx.dts    |    9 ++++
 arch/powerpc/platforms/83xx/mpc834x_itx.c |   70 +++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts
index db0d003..f8f0e8a 100644
--- a/arch/powerpc/boot/dts/mpc8349emitx.dts
+++ b/arch/powerpc/boot/dts/mpc8349emitx.dts

@@ -37,6 +37,15 @@
 		reg = <00000000 10000000>;
 	};
 
+	ide@f0000000 {
+		compatible = "mmio-ide";
+		device_type = "ide";
+		reg = <f0000000 10 f000020c 4>;
+		ioport_shift = <1>;
+		interrupts = <17 8>;
+		interrupt-parent = < &ipic >;
+	};
+
 	soc8349@e0000000 {
 		#address-cells = <1>;
 		#size-cells = <1>;

diff --git a/arch/powerpc/platforms/83xx/mpc834x_itx.c b/arch/powerpc/platforms/83xx/mpc834x_itx.c
index 40a0194..d63a104 100644
--- a/arch/powerpc/platforms/83xx/mpc834x_itx.c
+++ b/arch/powerpc/platforms/83xx/mpc834x_itx.c

@@ -23,6 +23,7 @@
 #include <linux/delay.h>
 #include <linux/seq_file.h>
 #include <linux/root_dev.h>
+#include <linux/pata_platform.h>
 
 #include <asm/system.h>
 #include <asm/atomic.h>

@@ -43,6 +44,75 @@ unsigned long isa_io_base = 0;
 unsigned long isa_mem_base = 0;
 #endif
 
+static int __init mmio_ide_of_init(void)
+{
+	struct device_node *np;
+	unsigned int i;
+
+	for (np = NULL, i = 0;
+	     (np = of_find_compatible_node(np, "ide", "mmio-ide")) != NULL;
+	     i++) {
+		int ret = 0;
+		struct resource res[3];
+		struct platform_device *pdev = NULL;
+		static struct pata_platform_info pdata;
+
+		memset(res, 0, sizeof(res));
+
+		ret = of_address_to_resource(np, 0, &res[0]);
+		if (ret) {
+			printk(KERN_ERR "mmio-ide.%d: unable to get "
+			       "resource from OF\n", i);
+			goto err0;
+		}
+
+		ret = of_address_to_resource(np, 1, &res[1]);
+		if (ret) {
+			printk(KERN_ERR "mmio-ide.%d: unable to get "
+			       "resource from OF\n", i);
+			goto err0;
+		}
+
+		res[2].start = res[2].end = irq_of_parse_and_map(np, 0);
+		if (res[2].start == NO_IRQ) {
+			printk(KERN_ERR "mmio-ide.%d: no IRQ\n", i);
+			goto err0;
+		}
+		res[2].name = "pata_platform";
+		res[2].flags = IORESOURCE_IRQ;
+
+		pdata.ioport_shift = *((u32 *)of_get_property(np,
+					"ioport_shift", NULL));
+
+		pdev = platform_device_alloc("pata_platform", i);
+		if (!pdev)
+			goto err1;
+
+		ret = platform_device_add_data(pdev, &pdata, sizeof(pdata));
+		if (ret)
+			goto err1;
+
+		ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
+		if (ret)
+			goto err1;
+
+		ret = platform_device_register(pdev);
+		if (ret)
+			goto err1;
+
+		continue;
+err1:
+		printk(KERN_ERR "mmio-ide.%d: registration failed\n", i);
+		platform_device_del(pdev); /* it will free everything */
+err0:
+		/* Even if some device failed, try others */
+		continue;
+	}
+
+	return 0;
+}
+device_initcall(mmio_ide_of_init);
+
 /* ************************************************************************
  *
  * Setup the architecture

Hello.

Vitaly Bordug wrote:

This updates relevant platform code (freescale mpc8349itx target) 
to make the CompactFlash work in TrueIDE mode. 

   Erm, I'm not sure it's worth submitting the platform device driver for 
PowerPC at this point, but well...

Signed-off-by: Anton Vorontsov <redacted>
Signed-off-by: Vitaly Bordug <redacted>

 arch/powerpc/boot/dts/mpc8349emitx.dts    |    9 ++++
 arch/powerpc/platforms/83xx/mpc834x_itx.c |   70 +++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 0 deletions(-)

    Why not "ata"?

+		reg = <f0000000 10 f000020c 4>;
+		ioport_shift = <1>;

    Please use hyphen, not underscore in the property names ("device_type" 
seems an only exception from this rule).
    And since using shift instead of size buys you nothing is this case, I'd 
prefer this property to be called reg-size or reg-stride.

[...]

[...]

    Unneeded initialization.

+		struct resource res[3];
+		struct platform_device *pdev = NULL;

    Another one.

+		static struct pata_platform_info pdata;
+
+		memset(res, 0, sizeof(res));
+
+		ret = of_address_to_resource(np, 0, &res[0]);
+		if (ret) {
+			printk(KERN_ERR "mmio-ide.%d: unable to get "
+			       "resource from OF\n", i);
+			goto err0;
+		}
+
+		ret = of_address_to_resource(np, 1, &res[1]);
+		if (ret) {
+			printk(KERN_ERR "mmio-ide.%d: unable to get "
+			       "resource from OF\n", i);
+			goto err0;

    Erm, these printk's are repetitive, isn't it better to put them under err0 
label?

+		}
+
+		res[2].start = res[2].end = irq_of_parse_and_map(np, 0);
+		if (res[2].start == NO_IRQ) {
+			printk(KERN_ERR "mmio-ide.%d: no IRQ\n", i);
+			goto err0;
+		}
+		res[2].name = "pata_platform";
+		res[2].flags = IORESOURCE_IRQ;
+
+		pdata.ioport_shift = *((u32 *)of_get_property(np,
+					"ioport_shift", NULL));
+
+		pdev = platform_device_alloc("pata_platform", i);
+		if (!pdev)
+			goto err1;

    Hm, not err0?

+
+		ret = platform_device_add_data(pdev, &pdata, sizeof(pdata));
+		if (ret)
+			goto err1;
+
+		ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res));
+		if (ret)
+			goto err1;
+
+		ret = platform_device_register(pdev);
+		if (ret)
+			goto err1;
+
+		continue;
+err1:
+		printk(KERN_ERR "mmio-ide.%d: registration failed\n", i);
+		platform_device_del(pdev); /* it will free everything */
+err0:
+		/* Even if some device failed, try others */
+		continue;
+	}
+
+	return 0;
+}
+device_initcall(mmio_ide_of_init);
+
 /* ************************************************************************
  *
  * Setup the architecture

MBR, Sergei

Hello, I wrote:

quoted

diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts
index db0d003..f8f0e8a 100644
--- a/arch/powerpc/boot/dts/mpc8349emitx.dts
+++ b/arch/powerpc/boot/dts/mpc8349emitx.dts

@@ -37,6 +37,15 @@

		reg = <00000000 10000000>;
	};

+	ide@f0000000 {
+		compatible = "mmio-ide";
+		device_type = "ide";

    Why not "ata"?

    Also, what mmio-ide in the compat properly means in the context of 
ide_platform which is able to handle both port and memory mapped IDE. I think 
we must get rid with this crap, and since this IDE register mapping is pretty 
much board specific, call it something like "mpc8349emitx-ide" instead.

MBR, Sergei

On Wed, Jul 25, 2007 at 09:54:07PM +0400, Sergei Shtylyov wrote:

    Also, what mmio-ide in the compat properly means in the context of 
ide_platform which is able to handle both port and memory mapped IDE.

I/O-space is only valid in the context of PCI, ISA, or similar buses, and
the bus-specific reg format indicates whether it's mmio-space or
io-space.

I think we must get rid with this crap, and since this IDE register
mapping is pretty much board specific, call it something like
"mpc8349emitx-ide" instead.

What is board specific about a set of standard IDE registers at a given
address?  Do we need to make board-specific glue code for all of the
various ns16550-compatibles out there as well?

-Scott

Hello.

Scott Wood wrote:

quoted

   Also, what mmio-ide in the compat properly means in the context of 
ide_platform which is able to handle both port and memory mapped IDE.

I/O-space is only valid in the context of PCI, ISA, or similar buses, and
the bus-specific reg format indicates whether it's mmio-space or
io-space.

    You could save time on lecturing me (and use it to look on the driver ;-).

quoted

I think we must get rid with this crap, and since this IDE register
mapping is pretty much board specific, call it something like
"mpc8349emitx-ide" instead.

What is board specific about a set of standard IDE registers at a given

    The regisrer mapping used is highly non-standard.

address?  Do we need to make board-specific glue code for all of the

    We're already in board specific code, so why the heck not? :-)

various ns16550-compatibles out there as well?

    I never suggested that -- what I did suggest was make of_serial.c 
recognize certain chip types and register them with 8250 driver.

-Scott

MBR, Sergei

Sergei Shtylyov wrote:

Hello.

Scott Wood wrote:

quoted

quoted

   Also, what mmio-ide in the compat properly means in the context of 
ide_platform which is able to handle both port and memory mapped IDE.

quoted

I/O-space is only valid in the context of PCI, ISA, or similar buses, and
the bus-specific reg format indicates whether it's mmio-space or
io-space.

   You could save time on lecturing me (and use it to look on the driver 
;-).

Sorry, I misread the question as being a mismatch between the 
capabilities of the device binding and the driver, not about the 
specific compatible name.  Something like "generic-ide" would probably 
be better.

quoted

What is board specific about a set of standard IDE registers at a given

   The regisrer mapping used is highly non-standard.

The gap between registers is nonstandard, but that's a fairly common 
type of noncompliance in embedded-land, and probably merits being 
supported in a generic way.  I wouldn't call it "highly" nonstandard.

Is there some other non-standardness that I'm missing?

   We're already in board specific code, so why the heck not? :-)

quoted

various ns16550-compatibles out there as well?

   I never suggested that -- what I did suggest was make of_serial.c 
recognize certain chip types and register them with 8250 driver.

What would be the advantage of maintaining a list of chips whose only 
difference is register spacing, rather than just using reg-shift and 
being done with it?

-Scott

Scott Wood wrote:

quoted

Scott Wood wrote:

quoted

quoted

quoted

   Also, what mmio-ide in the compat properly means in the context 
of ide_platform which is able to handle both port and memory mapped 
IDE.

quoted

quoted

I/O-space is only valid in the context of PCI, ISA, or similar buses, 
and
the bus-specific reg format indicates whether it's mmio-space or
io-space.

quoted

   You could save time on lecturing me (and use it to look on the 
driver ;-).

Sorry, I misread the question as being a mismatch between the 
capabilities of the device binding and the driver, not about the 
specific compatible name.

    That too. :-)

Something like "generic-ide" would probably be better.

    I strongly disagree with "generic" part. The generic IDE could only be 
said of 1:1 I/O mapped IDE ports, not about this fancy mapping.

quoted

quoted

What is board specific about a set of standard IDE registers at a given

quoted

   The regisrer mapping used is highly non-standard.

The gap between registers is nonstandard, but that's a fairly common 
type of noncompliance in embedded-land, and probably merits being 

    That is only a common variation of embedded non-compliancy (which doesn't 
make it a compliancy. ;-)
    There are worse cases in the bi-endian land, even with the standard 8-bit 
regs and 1-byte stride. *Hopefully*, this driver could also support those...

supported in a generic way.  I wouldn't call it "highly" nonstandard.

    Yeah, there are also 8250 "compatible" UARTs that use 32-bit memory 
accesses, and even worse -- with some registers mapped differently than on 
8250 (those can't be called compatible by any means), yet 8250.c drives all of 
them.  I'm not really sure it was such a good idea to merge, say Alchemy UART 
support into 8250.c.

Is there some other non-standardness that I'm missing?

    *Hopefully*, none.  The original Kumar's driver pretended to handle 
byte-lane swapping too (but that was ugly :-).

quoted

   We're already in board specific code, so why the heck not? :-)

quoted

quoted

various ns16550-compatibles out there as well?

quoted

   I never suggested that -- what I did suggest was make of_serial.c 
recognize certain chip types and register them with 8250 driver.

What would be the advantage of maintaining a list of chips whose only 

    Nobody's talking about the advantages, just about the device tree accepted 
practices (which we've already tried to bypass with MTD node -- causing a lot 
of bashing until David Woodhouse came to help :-).

difference is register spacing, rather than just using reg-shift and 
being done with it?

    Please read the linuxppc-dev archive's threads following form David's 
patches.  Or maybe Segher could repeat this for you. ;-)

-Scott

MBR, Sergei

Hello, I wrote:

quoted

quoted

quoted

quoted

  Also, what mmio-ide in the compat properly means in the context 
of ide_platform which is able to handle both port and memory mapped 
IDE.

quoted

quoted

quoted

I/O-space is only valid in the context of PCI, ISA, or similar buses, 
and
the bus-specific reg format indicates whether it's mmio-space or
io-space.

quoted

quoted

  You could save time on lecturing me (and use it to look on the 
driver ;-).

quoted

Sorry, I misread the question as being a mismatch between the 
capabilities of the device binding and the driver, not about the 
specific compatible name.

    That too. :-)

quoted

Something like "generic-ide" would probably be better.

    I strongly disagree with "generic" part. The generic IDE could only be 
said of 1:1 I/O mapped IDE ports, not about this fancy mapping.

    BTW, there's already something called drivers/ide/ide-generic.c... :-)

MBR, Sergei

quoted

   I never suggested that -- what I did suggest was make of_serial.c
recognize certain chip types and register them with 8250 driver.

What would be the advantage of maintaining a list of chips whose only
difference is register spacing, rather than just using reg-shift and
being done with it?

reg-shift alone isn't enough to know how to access the device
registers.  In the case of UARTs, they typically are internal
to some SoC, so the device driver can just look up what SoC it
is and will then know what to do.  For IDE though, as far as I
understand it typically is board-specific, done by some glue
logic off of some GPIOs or on an FPGA; I think detection is best
handled by platform code in this case, an "mmio-ide" shim driver
would be helpful of course.


Segher

Scott Wood wrote:

quoted

   Also, what mmio-ide in the compat properly means in the context of 
ide_platform which is able to handle both port and memory mapped IDE.
I think we must get rid with this crap, and since this IDE register
mapping is pretty much board specific, call it something like
"mpc8349emitx-ide" instead.

What is board specific about a set of standard IDE registers at a given
address?  Do we need to make board-specific glue code for all of the
various ns16550-compatibles out there as well?

    I acn undertand your complaint in the context of an OF driver (which we 
don't have yet) but "mmio-ide" just means nothing to the current driver, and 
it doesn't convery enough info on the programming interface for the 
conceivable OF driver, it also does need to know at least "reg-stride" (and 
maybe "reg-size" in case only 16/32-bit accesses can be used).  Well, if such 
driver will be written, I/O mapping support will probably be dropped from it, 
so indeed, calling it mmio-ide.c would make sense.  But that can be added when 
this driver is done, and for now I'd really prefer the board name to appear in 
the "compatible" prop (to which "mmio-ide" can be appended)...

-Scott

WBR, Sergei

Sergei Shtylyov wrote:

   I acn undertand your complaint in the context of an OF driver (which 
we don't have yet) but "mmio-ide" just means nothing to the current 
driver, and it doesn't convery enough info on the programming interface 
for the conceivable OF driver, it also does need to know at least 
"reg-stride" (and maybe "reg-size" in case only 16/32-bit accesses can 
be used).  Well, if such driver will be written, I/O mapping support 
will probably be dropped from it, so indeed, calling it mmio-ide.c would 
make sense.  But that can be added when this driver is done, and for now 

I don't think the details of what Linux code currently exists should 
drive the device tree binding.  That the current patches use 
platform_device glue code is an implementation detail (and one I'd 
rather see go away, in favor of a driver that supports both 
platform_device and of_device).

I'd really prefer the board name to appear in the "compatible" prop (to 
which "mmio-ide" can be appended)...

Sure, that's always good...  it was the "instead" that I objected to.

-Scott

On Wed, 25 Jul 2007 13:46:57 -0500
Scott Wood wrote:

Sergei Shtylyov wrote:

quoted

   I acn undertand your complaint in the context of an OF driver
(which we don't have yet) but "mmio-ide" just means nothing to the
current driver, and it doesn't convery enough info on the
programming interface for the conceivable OF driver, it also does
need to know at least "reg-stride" (and maybe "reg-size" in case
only 16/32-bit accesses can be used).  Well, if such driver will be
written, I/O mapping support will probably be dropped from it, so
indeed, calling it mmio-ide.c would make sense.  But that can be
added when this driver is done, and for now 

I don't think the details of what Linux code currently exists should 
drive the device tree binding.  That the current patches use 
platform_device glue code is an implementation detail (and one I'd 
rather see go away, in favor of a driver that supports both 
platform_device and of_device).

quoted

I'd really prefer the board name to appear in the "compatible" prop
(to which "mmio-ide" can be appended)...

Sure, that's always good...  it was the "instead" that I objected to.

Hmmm. So what is finally suggested devicetree node for this beast - can somebody refine?

I am a little bit confused about decided device_type and compatible fields...

-- 
Sincerely, Vitaly

Hello.

Vitaly Bordug wrote:

quoted

quoted

  I acn undertand your complaint in the context of an OF driver
(which we don't have yet) but "mmio-ide" just means nothing to the
current driver, and it doesn't convery enough info on the
programming interface for the conceivable OF driver, it also does
need to know at least "reg-stride" (and maybe "reg-size" in case
only 16/32-bit accesses can be used).  Well, if such driver will be
written, I/O mapping support will probably be dropped from it, so
indeed, calling it mmio-ide.c would make sense.  But that can be
added when this driver is done, and for now 

quoted

I don't think the details of what Linux code currently exists should 
drive the device tree binding.  That the current patches use 
platform_device glue code is an implementation detail (and one I'd 
rather see go away, in favor of a driver that supports both 
platform_device and of_device).

quoted

quoted

I'd really prefer the board name to appear in the "compatible" prop
(to which "mmio-ide" can be appended)...

quoted

Sure, that's always good...  it was the "instead" that I objected to.

Hmmm. So what is finally suggested devicetree node for this beast - can somebody refine?

I am a little bit confused about decided device_type

    My understanding is that "ata" has been already used, so there's no sense 
in introducing "ide". Anyway, Segher will just say that "device_type" shoudn't 
matter and even be present. ;-)

 > and compatible fields...

    In my understanding, as "mmio-ide" currectly makes no sense, it shouldn't 
even appear there.  And since "mpc8349emitx-cf" (or whatever would be most 
generic name for those boards with the same type of CF IDE mapping) should be 
imply the shift value, this property should also be optional, i.e. passing 
hard coded value with platform_device would do.

MBR, Sergei

quoted

quoted

+	ide@f0000000 {
+		compatible = "mmio-ide";
+		device_type = "ide";

quoted

    Why not "ata"?

The hardware is called (E)IDE, the protocol is called ATA.
Or that's what I was told -- I think there's some historic
revisionism involved, too.

    Also, what mmio-ide in the compat properly means in the context of
ide_platform which is able to handle both port and memory mapped IDE. 
I think
we must get rid with this crap, and since this IDE register mapping is 
pretty
much board specific, call it something like "mpc8349emitx-ide" instead.

"mmio-ide" simply is not specific enough.  The device_type
should go, too.

If this IDE interface is board-specific, thee "compatible"
property should include the board vendor name and board
name.  Oh, that's what "emitx" tries to do -- it could be
a bit clearer perhaps ;-)


Segher

The hardware is called (E)IDE, the protocol is called ATA.
Or that's what I was told -- I think there's some historic
revisionism involved, too.

ATA is the interface and standards for the ANSI standards based disk
attachment. IDE "Integrated Drive Electronics" is a marketing name used
to cover all sorts of ST412 compatible-ish early interfaces that moved
the brains onto the disk. IDE doesn't really mean much but "brains on
disk", ATA is a real standard.

quoted

The hardware is called (E)IDE, the protocol is called ATA.
Or that's what I was told -- I think there's some historic
revisionism involved, too.

ATA is the interface and standards for the ANSI standards based disk
attachment. IDE "Integrated Drive Electronics" is a marketing name used
to cover all sorts of ST412 compatible-ish early interfaces that moved
the brains onto the disk. IDE doesn't really mean much but "brains on
disk", ATA is a real standard.

Thanks for refreshing my memory.

We will have to support both names in OF device tree nodes, since
both names are used in many existing device trees.  For new nodes
with no precedent, like this "mmio-ide", let's require the more
correct "ata" name.


Segher

Hello.

Segher Boessenkool wrote:

quoted

quoted

quoted

+    ide@f0000000 {
+        compatible = "mmio-ide";
+        device_type = "ide";

quoted

quoted

    Why not "ata"?

The hardware is called (E)IDE, the protocol is called ATA.

    Sorry for not denouncing this earlier. :-)
    ATA is the name of ANSI standard describing IDE.

Or that's what I was told --

    Re-check your sources. ;-)

 > I think there's some historic revisionism involved, too.

    IDE was probably an initial name of the infamous disk hardware/protocol 
later standardized as ATA, EIDE (being more of a trademark) more or less 
equals to ATA-2.

quoted

    Also, what mmio-ide in the compat properly means in the context of
ide_platform which is able to handle both port and memory mapped IDE. 
I think
we must get rid with this crap, and since this IDE register mapping is 
pretty
much board specific, call it something like "mpc8349emitx-ide" instead.

"mmio-ide" simply is not specific enough.  The device_type

    Yes.

should go, too.

If this IDE interface is board-specific, thee "compatible"

    It's "thy", not "thee". ;-)

property should include the board vendor name and board
name.  Oh, that's what "emitx" tries to do -- it could be
a bit clearer perhaps ;-)

    Yeah, I forgot about the vondor's "fsl," prefix.

Segher

MBR, Sergei

On Wed, 25 Jul 2007 21:06:42 +0400
Sergei Shtylyov wrote:

Hello.

Vitaly Bordug wrote:

quoted

This updates relevant platform code (freescale mpc8349itx target) 
to make the CompactFlash work in TrueIDE mode. 

   Erm, I'm not sure it's worth submitting the platform device driver
for PowerPC at this point, but well...

As Ben already confirmed, it is OK to have "constructor" insert platform device if such is
required by design.

quoted

Signed-off-by: Anton Vorontsov <redacted>
Signed-off-by: Vitaly Bordug <redacted>

quoted

 arch/powerpc/boot/dts/mpc8349emitx.dts    |    9 ++++
 arch/powerpc/platforms/83xx/mpc834x_itx.c |   70
+++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 0
deletions(-)

quoted

diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts

b/arch/powerpc/boot/dts/mpc8349emitx.dts index db0d003..f8f0e8a
100644 --- a/arch/powerpc/boot/dts/mpc8349emitx.dts

+++ b/arch/powerpc/boot/dts/mpc8349emitx.dts

@@ -37,6 +37,15 @@
 		reg = <00000000 10000000>;
 	};
 
+	ide@f0000000 {
+		compatible = "mmio-ide";
+		device_type = "ide";

    Why not "ata"?

we already argued around this iirc. I have no preferences and will put whatever agreed appropriate finally in here.

quoted

+		reg = <f0000000 10 f000020c 4>;
+		ioport_shift = <1>;

    Please use hyphen, not underscore in the property names
("device_type" seems an only exception from this rule).
    And since using shift instead of size buys you nothing is this
case, I'd prefer this property to be called reg-size or reg-stride.

[...]

quoted

diff --git a/arch/powerpc/platforms/83xx/mpc834x_itx.c

b/arch/powerpc/platforms/83xx/mpc834x_itx.c index 40a0194..d63a104
100644 --- a/arch/powerpc/platforms/83xx/mpc834x_itx.c

+++ b/arch/powerpc/platforms/83xx/mpc834x_itx.c

[...]

quoted

@@ -43,6 +44,75 @@ unsigned long isa_io_base = 0;
 unsigned long isa_mem_base = 0;
 #endif
 
+static int __init mmio_ide_of_init(void)
+{
+	struct device_node *np;
+	unsigned int i;
+
+	for (np = NULL, i = 0;
+	     (np = of_find_compatible_node(np, "ide",

"mmio-ide")) != NULL;
+	     i++) {
+		int ret = 0;

    Unneeded initialization.

quoted

+		struct resource res[3];
+		struct platform_device *pdev = NULL;

    Another one.

ok

quoted

+		static struct pata_platform_info pdata;
+
+		memset(res, 0, sizeof(res));
+
+		ret = of_address_to_resource(np, 0, &res[0]);
+		if (ret) {
+			printk(KERN_ERR "mmio-ide.%d: unable to
get "
+			       "resource from OF\n", i);
+			goto err0;
+		}
+
+		ret = of_address_to_resource(np, 1, &res[1]);
+		if (ret) {
+			printk(KERN_ERR "mmio-ide.%d: unable to
get "
+			       "resource from OF\n", i);
+			goto err0;

    Erm, these printk's are repetitive, isn't it better to put them
under err0 label?

quoted

+		}
+
+		res[2].start = res[2].end =
irq_of_parse_and_map(np, 0);
+		if (res[2].start == NO_IRQ) {
+			printk(KERN_ERR "mmio-ide.%d: no IRQ\n",
i);
+			goto err0;
+		}
+		res[2].name = "pata_platform";
+		res[2].flags = IORESOURCE_IRQ;
+
+		pdata.ioport_shift = *((u32 *)of_get_property(np,
+					"ioport_shift", NULL));
+
+		pdev = platform_device_alloc("pata_platform", i);
+		if (!pdev)
+			goto err1;

    Hm, not err0?

agreed.

quoted

+
+		ret = platform_device_add_data(pdev, &pdata,
sizeof(pdata));
+		if (ret)
+			goto err1;
+
+		ret = platform_device_add_resources(pdev, res,
ARRAY_SIZE(res));
+		if (ret)
+			goto err1;
+
+		ret = platform_device_register(pdev);
+		if (ret)
+			goto err1;
+
+		continue;
+err1:
+		printk(KERN_ERR "mmio-ide.%d: registration
failed\n", i);
+		platform_device_del(pdev); /* it will free
everything */ +err0:
+		/* Even if some device failed, try others */
+		continue;
+	}
+
+	return 0;
+}
+device_initcall(mmio_ide_of_init);
+
 /*
************************************************************************
*
  * Setup the architecture

MBR, Sergei

-- 
Sincerely, Vitaly

Vitaly Bordug wrote:

Is this binding documented anywhere?  If it's a new binding, could we 
use reg-shift instead of ioport_shift?  This is what ns16550 uses, and 
in general dashes are preferable to underscores in device trees 
(device_type is an ugly, historical exception).

We really don't want code like this for every board with an MMIO IDE 
controller.  At the very least, make it a generic function in sysdev; 
better would be to make the driver also support of_platform devices.

-Scott

pata_platform and ide_platform are carrying same driver names,
to easily switch between these drivers, without need to touch
platform code.

Signed-off-by: Anton Vorontsov <redacted>
Signed-off-by: Vitaly Bordug <redacted>

Acked-by: Alan Cox <redacted>

On Wednesday 25 July 2007, Alan Cox wrote:

quoted

pata_platform and ide_platform are carrying same driver names,
to easily switch between these drivers, without need to touch
platform code.

Signed-off-by: Anton Vorontsov <redacted>
Signed-off-by: Vitaly Bordug <redacted>

Acked-by: Alan Cox <redacted>

added

On Jul 25, 2007, at 11:53 AM, Vitaly Bordug wrote:

This is now very similar to pata_platform.c, they both use
same platform data structure and same resources.

To achieve that, byte_lanes_swapping platform data variable
and platform specified iops removed from that driver. It's fine,
since those were never used anyway.

pata_platform and ide_platform are carrying same driver names,
to easily switch between these drivers, without need to touch
platform code.

Signed-off-by: Anton Vorontsov <redacted>
Signed-off-by: Vitaly Bordug <redacted>

Out of interest is this something that could exist in pata land?  I  
haven't really been following legacy/ide vs pata lately.

- k

Vitaly Bordug wrote:

This is now very similar to pata_platform.c, they both use
same platform data structure and same resources.

To achieve that, byte_lanes_swapping platform data variable
and platform specified iops removed from that driver. It's fine,
since those were never used anyway.

    Hopefully, PPC will never need them.

pata_platform and ide_platform are carrying same driver names,
to easily switch between these drivers, without need to touch
platform code.

    Poor Kumar, I guess he was surprised be being assigned a maintainer of the 
driver he didn't write (well, even if it borrowed some code from his earlier 
one ;-)...

+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/ide.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/pata_platform.h>
+#include <linux/io.h>
+
+static struct {
+	void __iomem *plat_ide_mapbase;
+	void __iomem *plat_ide_alt_mapbase;
+	ide_hwif_t *hwif;
+	int index;
+} hwif_prop;
+
+static ide_hwif_t *__devinit plat_ide_locate_hwif(void __iomem *base,
+	    void __iomem *ctrl, struct pata_platform_info *pdata, int irq,
+	    int mmio)
+{
+	unsigned long port = (unsigned long)base;
+	ide_hwif_t *hwif;
+	int index, i;
+
+	for (index = 0; index < MAX_HWIFS; ++index) {
+		hwif = ide_hwifs + index;
+		if (hwif->io_ports[IDE_DATA_OFFSET] == port)
+			goto found;
+	}
+
+	for (index = 0; index < MAX_HWIFS; ++index) {
+		hwif = ide_hwifs + index;
+		if (hwif->io_ports[IDE_DATA_OFFSET] == 0)
+			goto found;
+	}
+
+	return NULL;
+
+found:
+
+	hwif->hw.io_ports[IDE_DATA_OFFSET] = port;
+
+	port += (1 << pdata->ioport_shift);
+	for (i = IDE_ERROR_OFFSET; i <= IDE_STATUS_OFFSET;
+	     i++, port += (1 << pdata->ioport_shift))

    Looks like shift doesn't buy as anything, why not just use stride?

+		hwif->hw.io_ports[i] = port;
+
+	hwif->hw.io_ports[IDE_CONTROL_OFFSET] = (unsigned long)ctrl;
+
+	memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->hw.io_ports));
+	hwif->hw.irq = hwif->irq = irq;
+
+	hwif->hw.dma = NO_DMA;
+	hwif->hw.chipset = ide_generic;
+
+	if (mmio) {
+		hwif->mmio = 1;
+		default_hwif_mmiops(hwif);
+	}

    And why default_hwif_iops(hwif) is not called else?
    And I remember the previos variant was overriding INSW()/OUTSW() methods 
-- turned out to be unnecessary? :-)

+
+	hwif_prop.hwif = hwif;
+	hwif_prop.index = index;
+
+	return hwif;
+}
+
+static int __devinit plat_ide_probe(struct platform_device *pdev)
+{
+	struct resource *res_base, *res_alt, *res_irq;
+	ide_hwif_t *hwif;
+	struct pata_platform_info *pdata;
+	int ret = 0;
+	int mmio = 0;
+
+	pdata = pdev->dev.platform_data;
+
+	/* get a pointer to the register memory */
+	res_base = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	res_alt = platform_get_resource(pdev, IORESOURCE_IO, 1);
+
+	if (!res_base || !res_alt) {
+		res_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		res_alt = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (!res_base || !res_alt) {
+			ret = -ENOMEM;

    ENODEV or EINVAL you mean? :-)

+			goto out;

    Bleh... just return please.

+		}
+		mmio = 1;
+	}
+
+	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res_irq) {
+		ret = -EINVAL;
+		goto out;

    Bleh... return -EINVAL, please. Or maybe -ENODEV.

+	}
+

[...]

+	hwif = plat_ide_locate_hwif(hwif_prop.plat_ide_mapbase,
+	         hwif_prop.plat_ide_alt_mapbase, pdata, res_irq->start, mmio);
+
+	if (!hwif) {
+		ret = -ENODEV;
+		goto out;

    Bleh... please just

return -ENODEV.

+	}
+	hwif->gendev.parent = &pdev->dev;
+	hwif->noprobe = 0;
+
+	probe_hwif_init(hwif);
+
+	platform_set_drvdata(pdev, hwif);
+	ide_proc_register_port(hwif);
+
+	return 0;
+
+out:

    No need to abuse the function cleanup style when you have nothing to 
cleanup. :-/

+	return ret;
+}
+
+static int __devexit plat_ide_remove(struct platform_device *pdev)
+{
+	ide_hwif_t *hwif = pdev->dev.driver_data;
+
+	if (hwif != hwif_prop.hwif) {
+		dev_printk(KERN_DEBUG, &pdev->dev, "%s: hwif value error",
+		           pdev->name);
+	} else {
+		ide_unregister(hwif_prop.index);
+		hwif_prop.index = 0;
+		hwif_prop.hwif = NULL;
+	}
+

    I'd have interchanged the success/failure branches...

+	return 0;
+}
+
+static struct platform_driver platform_ide_driver = {
+	.driver {
+		.name = "pata_platform",
+	},
+	.probe = plat_ide_probe,
+	.remove = __devexit_p(plat_ide_remove),
+};
+
+static int __init platform_ide_init(void)
+{
+	return platform_driver_register(&platform_ide_driver);
+}
+
+static void __exit platform_ide_exit(void)
+{
+	platform_driver_unregister(&platform_ide_driver);
+}
+
+MODULE_DESCRIPTION("Platform IDE driver");
+MODULE_LICENSE("GPL");
+
+module_init(platform_ide_init);
+module_exit(platform_ide_exit);

Sergei Shtylyov wrote:

quoted

+	hwif->hw.io_ports[IDE_DATA_OFFSET] = port;
+
+	port += (1 << pdata->ioport_shift);
+	for (i = IDE_ERROR_OFFSET; i <= IDE_STATUS_OFFSET;
+	     i++, port += (1 << pdata->ioport_shift))

    Looks like shift doesn't buy as anything, why not just use stride?

It doesn't buy us anything in here, but it's conceivable that someone 
may want to write a driver that uses a shift in the I/O accessor rather 
than an array of port offsets, and it's easier to convert a shift to a 
stride than the other way around (not all architectures have an 
equivalent of the cntlzw innstruction, and shift makes it clear that the 
stride must be power-of-two).  Plus, using shift is consistent with what 
we do on ns16550.

-Scott

Hello.

Scott Wood wrote:

quoted

quoted

+    hwif->hw.io_ports[IDE_DATA_OFFSET] = port;
+
+    port += (1 << pdata->ioport_shift);
+    for (i = IDE_ERROR_OFFSET; i <= IDE_STATUS_OFFSET;
+         i++, port += (1 << pdata->ioport_shift))

    Looks like shift doesn't buy as anything, why not just use stride?

It doesn't buy us anything in here, but it's conceivable that someone 
may want to write a driver that uses a shift in the I/O accessor rather 
than an array of port offsets,

    It wouldn't be IDE driver then, and neither it would be libata which also 
does this another way this (despite pata_platform uses shifts too -- not in 
the accessors, so no speed loss).

and it's easier to convert a shift to a stride than the other way around

 > (not all architectures have an

equivalent of the cntlzw innstruction, and shift makes it clear that the 
stride must be power-of-two).  Plus, using shift is consistent with what 
we do on ns16550.

    Why the heck should we care about the UART code taling about IDE?!
    So, let me consider your argument purely speculative and invalid. ;-)

-Scott

WBR, Sergei

Sergei Shtylyov wrote:

quoted

It doesn't buy us anything in here, but it's conceivable that someone 
may want to write a driver that uses a shift in the I/O accessor 
rather than an array of port offsets,

   It wouldn't be IDE driver then, and neither it would be libata which 
also does this another way this (despite pata_platform uses shifts too 
-- not in the accessors, so no speed loss).

The device tree is not just for Linux.

quoted

equivalent of the cntlzw innstruction, and shift makes it clear that 
the stride must be power-of-two).  Plus, using shift is consistent 
with what we do on ns16550.

   Why the heck should we care about the UART code taling about IDE?!

Consistency?

   So, let me consider your argument purely speculative and invalid. ;-)

Consider it whatever you want. :-)

-Scott

Scott Wood wrote:

quoted

quoted

It doesn't buy us anything in here, but it's conceivable that someone 
may want to write a driver that uses a shift in the I/O accessor 
rather than an array of port offsets,

quoted

   It wouldn't be IDE driver then, and neither it would be libata 
which also does this another way this (despite pata_platform uses 
shifts too -- not in the accessors, so no speed loss).

The device tree is not just for Linux.

    Yeah, and I can't wait to see some other its users. ;-)
    This doesn't mean that shift is better anyway. If everyone considers it 
better, I give up. But be warned that shift (stride) is not the only property 
characterizing register accesses -- the regs might be only accessible as 
16/32-bit quantities, for example (16-bit is a real world example -- from 
Amiga or smth of that sort, IIRC).

quoted

quoted

equivalent of the cntlzw innstruction, and shift makes it clear that 
the stride must be power-of-two).  Plus, using shift is consistent 
with what we do on ns16550.

quoted

   Why the heck should we care about the UART code taling about IDE?!

Consistency?

    We're not obliged to be consistent with every piece of the kernel code.

-Scott

MBR, Sergei

    This doesn't mean that shift is better anyway. If everyone 
considers it
better, I give up. But be warned that shift (stride) is not the only 
property
characterizing register accesses -- the regs might be only accessible 
as
16/32-bit quantities, for example (16-bit is a real world example -- 
from
Amiga or smth of that sort, IIRC).

More importantly, "reg-shift" doesn't say what part of
the bigger words to access.  A common example is byte-wide
registers on a 32-bit-only bus; it's about 50%-50% between
connecting the registers to the low byte vs. connecting it
to the byte with the lowest address.

The only realistic way to handle all this is to put some
knowledge into the device driver.  This does of course
also mean that no "reg-shift" property is needed; the
device driver can look at your "compatible" property,
instead.

quoted

quoted

   Why the heck should we care about the UART code taling about IDE?!

quoted

Consistency?

    We're not obliged to be consistent with every piece of the kernel 
code.

It would be nice to not name similar properties in the
device tree dissimilarly.  Kernel code doesn't come into
the picture here.


Segher

Hello.

Segher Boessenkool wrote:

quoted

    This doesn't mean that shift is better anyway. If everyone 
considers it
better, I give up. But be warned that shift (stride) is not the only 
property
characterizing register accesses -- the regs might be only accessible as
16/32-bit quantities, for example (16-bit is a real world example -- from
Amiga or smth of that sort, IIRC).

More importantly, "reg-shift" doesn't say what part of
the bigger words to access.  A common example is byte-wide
registers on a 32-bit-only bus; it's about 50%-50% between
connecting the registers to the low byte vs. connecting it
to the byte with the lowest address.

    We already have "big-endian" prop used in MPIC nodes, IIRC. Could try to 
"reuse" it here as well...

The only realistic way to handle all this is to put some
knowledge into the device driver.  This does of course
also mean that no "reg-shift" property is needed; the
device driver can look at your "compatible" property,
instead.

quoted

quoted

quoted

   Why the heck should we care about the UART code taling about IDE?!

quoted

quoted

Consistency?

quoted

    We're not obliged to be consistent with every piece of the kernel 
code.

It would be nice to not name similar properties in the
device tree dissimilarly.  Kernel code doesn't come into
the picture here.

    The "reg-shift" prop is yet unaccepted ad-hockery at this point. ;-)
    So, I don't see why we have to be consistent with it.

Segher

WBR, Sergei

quoted

More importantly, "reg-shift" doesn't say what part of
the bigger words to access.  A common example is byte-wide
registers on a 32-bit-only bus; it's about 50%-50% between
connecting the registers to the low byte vs. connecting it
to the byte with the lowest address.

   We already have "big-endian" prop used in MPIC nodes, IIRC. Could 
try to "reuse" it here as well...

Sure.  This would be an okay way to handle legacy devices that
are connected in inventive ways: add "reg-shift" and/or "big-endian"
properties.  We should make sure this is documented in the
appropriate bindings though, don't just assume it will work.

For non-legacy devices, please just use the "compatible"
property to figure out the endianness etc.; it is a bad idea
to make a "blablabla-big-endian" compatible value, but you can
almost often just use a more specific model name instead; and
typically the device has some other quirks anyway ;-)

quoted

It would be nice to not name similar properties in the
device tree dissimilarly.  Kernel code doesn't come into
the picture here.

   The "reg-shift" prop is yet unaccepted ad-hockery at this point. ;-)
   So, I don't see why we have to be consistent with it.

Don't treat your ad-hockery ad hoc, that way leads to insanity :-)

It's quite important to use good names for all new properties
you define, so you naturally end up with similar names for
similar purposes.  Of course it isn't a *requirement*, you're
right about that.


Segher

On Wed, 25 Jul 2007, Vitaly Bordug wrote:

This is now very similar to pata_platform.c, they both use
same platform data structure and same resources.

To achieve that, byte_lanes_swapping platform data variable
and platform specified iops removed from that driver. It's fine,
since those were never used anyway.

pata_platform and ide_platform are carrying same driver names,
to easily switch between these drivers, without need to touch
platform code.

Why? There's a drivers/ide/arm/ide_arm.c IDe driver that some platforms 
(not in the mainline) hack to access, e.g., CF cards in true-IDE mode. 
About a month ago I submitted a patch to arm-linux-kernel switching that 
driver to using platform-device. I got a reply, that it's not worth it now 
that IDE is slowly becoming obsolete, and the pata_platform serves the 
perpose perfectly well. I found this argument reasonable, I had the same 
doubt, just wanted to double-check. So, why do we now need a new legacy 
(a/drivers/ide/legacy/ide_platform.c) driver when a "modern" driver 
exists?

Thanks
Guennadi
---
Guennadi Liakhovetski

Guennadi Liakhovetski wrote:

quoted

This is now very similar to pata_platform.c, they both use
same platform data structure and same resources.

quoted

To achieve that, byte_lanes_swapping platform data variable
and platform specified iops removed from that driver. It's fine,
since those were never used anyway.

quoted

pata_platform and ide_platform are carrying same driver names,
to easily switch between these drivers, without need to touch
platform code.

Why? There's a drivers/ide/arm/ide_arm.c IDe driver that some platforms 
(not in the mainline) hack to access, e.g., CF cards in true-IDE mode. 
About a month ago I submitted a patch to arm-linux-kernel switching that 

    Wrong list to submit sych stuff, post to linux-ide.

driver to using platform-device. I got a reply, that it's not worth it now 
that IDE is slowly becoming obsolete, and the pata_platform serves the 
perpose perfectly well. I found this argument reasonable, I had the same

    Ignore such replies in the future. ;-)

doubt, just wanted to double-check. So, why do we now need a new legacy 
(a/drivers/ide/legacy/ide_platform.c) driver when a "modern" driver 
exists?

    Good question (I know the answer but won't tell ;-).

Thanks
Guennadi

MBR, Sergei

On Wed, 25 Jul 2007, Sergei Shtylyov wrote:

Guennadi Liakhovetski wrote:

quoted

quoted

This is now very similar to pata_platform.c, they both use
same platform data structure and same resources.

quoted

quoted

To achieve that, byte_lanes_swapping platform data variable
and platform specified iops removed from that driver. It's fine,
since those were never used anyway.

quoted

quoted

pata_platform and ide_platform are carrying same driver names,
to easily switch between these drivers, without need to touch
platform code.

quoted

Why? There's a drivers/ide/arm/ide_arm.c IDe driver that some platforms (not
in the mainline) hack to access, e.g., CF cards in true-IDE mode. About a
month ago I submitted a patch to arm-linux-kernel switching that 

   Wrong list to submit sych stuff, post to linux-ide.

Not entirely. The patch (or other patches in the series) would also touch 
ARM platforms in the mainline, currently using that driver. As I didn't 
have a chance to test them due to lack of hardware, I posted on arm, 
asking if anyone would test those platforms for me.

quoted

driver to using platform-device. I got a reply, that it's not worth it now
that IDE is slowly becoming obsolete, and the pata_platform serves the
perpose perfectly well. I found this argument reasonable, I had the same

   Ignore such replies in the future. ;-)

It was largely in accordance with my own opinion, so, I chose to accept 
it:-)

quoted

doubt, just wanted to double-check. So, why do we now need a new legacy
(a/drivers/ide/legacy/ide_platform.c) driver when a "modern" driver exists?

   Good question (I know the answer but won't tell ;-).

You've been very cooperative, thanks.

Thanks
Guennadi
---
Guennadi Liakhovetski

Guennadi Liakhovetski wrote:

quoted

quoted

quoted

This is now very similar to pata_platform.c, they both use
same platform data structure and same resources.

quoted

quoted

quoted

To achieve that, byte_lanes_swapping platform data variable
and platform specified iops removed from that driver. It's fine,
since those were never used anyway.

quoted

quoted

quoted

pata_platform and ide_platform are carrying same driver names,
to easily switch between these drivers, without need to touch
platform code.

quoted

quoted

Why? There's a drivers/ide/arm/ide_arm.c IDe driver that some platforms (not
in the mainline) hack to access, e.g., CF cards in true-IDE mode. About a
month ago I submitted a patch to arm-linux-kernel switching that 

quoted

  Wrong list to submit sych stuff, post to linux-ide.

Not entirely. The patch (or other patches in the series) would also touch 
ARM platforms in the mainline, currently using that driver. As I didn't 

    Was worth cross-posting to linux-ide anyway to get the IDE experts' 
feedback. ;-)

have a chance to test them due to lack of hardware, I posted on arm, 
asking if anyone would test those platforms for me.

   ... and they laughed at you? ;-)

quoted

quoted

driver to using platform-device. I got a reply, that it's not worth it now
that IDE is slowly becoming obsolete, and the pata_platform serves the
perpose perfectly well. I found this argument reasonable, I had the same

quoted

  Ignore such replies in the future. ;-)

It was largely in accordance with my own opinion, so, I chose to accept 
it:-)

    It's not clear why you decided to waste time on it then. :-)

quoted

quoted

doubt, just wanted to double-check. So, why do we now need a new legacy
(a/drivers/ide/legacy/ide_platform.c) driver when a "modern" driver exists?

quoted

  Good question (I know the answer but won't tell ;-).

You've been very cooperative, thanks.

    In fact, I also highly doubt that we need it.  What we'd need is an OF driver.

Thanks
Guennadi

WBR, Sergei

On Thu, 26 Jul 2007, Sergei Shtylyov wrote:

Guennadi Liakhovetski wrote:

quoted

quoted

  Wrong list to submit sych stuff, post to linux-ide.

quoted

Not entirely. The patch (or other patches in the series) would also touch
ARM platforms in the mainline, currently using that driver. As I didn't 

   Was worth cross-posting to linux-ide anyway to get the IDE experts'
feedback. ;-)

linux-arm* mailing lists do not allow cross-posting.

quoted

have a chance to test them due to lack of hardware, I posted on arm, asking
if anyone would test those platforms for me.

  ... and they laughed at you? ;-)

No, noone had that hardware either:-) Those who had preferred to forget 
about it, I guess.

quoted

It was largely in accordance with my own opinion, so, I chose to accept
it:-)

   It's not clear why you decided to waste time on it then. :-)

Because I myself was in the situation where my local version of the driver 
was filling with #ifdef's supporting all possible variations of our 
hardware, so, I switched it to platform_driver to clean up that mess. And 
then decided to ask if others would consider it useful. Just in case. I 
hoped they wouldn't.

quoted

quoted

quoted

doubt, just wanted to double-check. So, why do we now need a new legacy
(a/drivers/ide/legacy/ide_platform.c) driver when a "modern" driver
exists?

quoted

quoted

  Good question (I know the answer but won't tell ;-).

quoted

You've been very cooperative, thanks.

   In fact, I also highly doubt that we need it.  What we'd need is an OF
driver.

Great, thanks. Now we have to find out why Alan acked it (added to cc).

Thanks
Guennadi
---
Guennadi Liakhovetski

driver to using platform-device. I got a reply, that it's not worth it now 
that IDE is slowly becoming obsolete, and the pata_platform serves the 
perpose perfectly well. I found this argument reasonable, I had the same 
doubt, just wanted to double-check. So, why do we now need a new legacy 
(a/drivers/ide/legacy/ide_platform.c) driver when a "modern" driver 
exists?

We don't *need* it but some people still want to use old IDE and the
author was willing to make it neatly compatible so that anything that
works with the pata_platform should be able to use the ide_platform
driver and vice versa. For the shorter term that can only be a good thing
- arch code doesn't need to care about which driver is used, end users
can pick and it doesn't end up adding new ties between code and old IDE.

On Wed, 25 Jul 2007, Alan Cox wrote:

quoted

driver to using platform-device. I got a reply, that it's not worth it now 
that IDE is slowly becoming obsolete, and the pata_platform serves the 
perpose perfectly well. I found this argument reasonable, I had the same 
doubt, just wanted to double-check. So, why do we now need a new legacy 
(a/drivers/ide/legacy/ide_platform.c) driver when a "modern" driver 
exists?

We don't *need* it but some people still want to use old IDE and the
author was willing to make it neatly compatible so that anything that
works with the pata_platform should be able to use the ide_platform
driver and vice versa. For the shorter term that can only be a good thing
- arch code doesn't need to care about which driver is used, end users
can pick and it doesn't end up adding new ties between code and old IDE.

Ok, thanks for the explanation Alan. So, there's no technical argument, 
just "being nice to the users", and add a new driver, which we know we'll 
have to remove soon, thus having to persuade its users, who by that time 
will get used to it and will not want to invest money into switching to 
another one...

Thanks
Guennadi
---
Guennadi Liakhovetski

Hello.

Guennadi Liakhovetski wrote:

quoted

quoted

driver to using platform-device. I got a reply, that it's not worth it now 
that IDE is slowly becoming obsolete, and the pata_platform serves the 
perpose perfectly well. I found this argument reasonable, I had the same 
doubt, just wanted to double-check. So, why do we now need a new legacy 
(a/drivers/ide/legacy/ide_platform.c) driver when a "modern" driver 
exists?

quoted

We don't *need* it but some people still want to use old IDE and the
author was willing to make it neatly compatible so that anything that
works with the pata_platform should be able to use the ide_platform
driver and vice versa. For the shorter term that can only be a good thing
- arch code doesn't need to care about which driver is used, end users
can pick and it doesn't end up adding new ties between code and old IDE.

Ok, thanks for the explanation Alan. So, there's no technical argument, 
just "being nice to the users", and add a new driver, which we know we'll 
have to remove soon, thus having to persuade its users, who by that time 

    Define "soon". :-)

will get used to it and will not want to invest money into switching to 
another one...

    Invest into what if the drivers are functionally identical?

Thanks
Guennadi

MBR, Sergei

On Thu, 26 Jul 2007, Sergei Shtylyov wrote:

Guennadi Liakhovetski wrote:

quoted

Ok, thanks for the explanation Alan. So, there's no technical argument, 
just "being nice to the users", and add a new driver, which we know we'll 
have to remove soon, thus having to persuade its users, who by that time 

    Define "soon". :-)

"Soon" in this case means, we NOW have a replacement driver and we aim at 
deprecating the IDE driver in its favour.

quoted

will get used to it and will not want to invest money into switching to 
another one...

    Invest into what if the drivers are functionally identical?

For example, into porting user space (replace hdx with sdx). And even 
switching to a "functionally identical" driver requires a new complete 
testing cycle... No, I won't be in that situation, so, it's not my money 
I'm worrying about. But I know there are companies for whom releasing a 
new version of /etc/fstab is a considerable expense... Not that I cared 
about them anyway.

Thanks
Guennadi
---
Guennadi Liakhovetski

Hi,

On Thursday 26 July 2007, Guennadi Liakhovetski wrote:

On Wed, 25 Jul 2007, Alan Cox wrote:

quoted

quoted

driver to using platform-device. I got a reply, that it's not worth it now 
that IDE is slowly becoming obsolete, and the pata_platform serves the 
perpose perfectly well. I found this argument reasonable, I had the same 
doubt, just wanted to double-check. So, why do we now need a new legacy 
(a/drivers/ide/legacy/ide_platform.c) driver when a "modern" driver 
exists?

We don't *need* it but some people still want to use old IDE and the
author was willing to make it neatly compatible so that anything that
works with the pata_platform should be able to use the ide_platform
driver and vice versa. For the shorter term that can only be a good thing
- arch code doesn't need to care about which driver is used, end users
can pick and it doesn't end up adding new ties between code and old IDE.

Ok, thanks for the explanation Alan. So, there's no technical argument, 
just "being nice to the users", and add a new driver, which we know we'll 

There are some rough edges (especially older and/or rare hardware,
this goes for both cotrollers and devices) that SCSI/libata don't
handle and IDE subsystem do.

have to remove soon, thus having to persuade its users, who by that time 

Well, we've been hearing "soon" for two years now...

will get used to it and will not want to invest money into switching to 
another one...

PS wrt ide_arm.c changes, you really should have cc:ed the author... ;)

Thanks,
Bart

Hi,

On Wednesday 25 July 2007, Vitaly Bordug wrote:

This is now very similar to pata_platform.c, they both use
same platform data structure and same resources.

To achieve that, byte_lanes_swapping platform data variable
and platform specified iops removed from that driver. It's fine,
since those were never used anyway.

pata_platform and ide_platform are carrying same driver names,
to easily switch between these drivers, without need to touch
platform code.

Looks really good, thanks for working on this.

Signed-off-by: Anton Vorontsov <redacted>
Signed-off-by: Vitaly Bordug <redacted>

applied but it would be also nice to fix minor issues found by Sergei
(please just send a new version of the patch and I will replace this
version with the newer one in my tree)

patch #2/2 also looks fine and when issues raised by Sergei gets
addressed it will be merged

Thanks,
Bart