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

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

[Vitaly Bordug]

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 <avorontsov@ru.mvista.com>
Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

---

 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);

[PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Vitaly Bordug]

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

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

---

 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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Sergei Shtylyov]

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 <avorontsov@ru.mvista.com>
> Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

>  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";

    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.

[...]
> 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
[...]
> @@ -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.

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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Scott Wood]

Vitaly Bordug wrote:
> 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 >;
> +	};
> +

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).

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

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

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

[Alan Cox]

> 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 <avorontsov@ru.mvista.com>
> Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

Acked-by: Alan Cox <alan@redhat.com>

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Sergei Shtylyov]

Hello, I wrote:

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

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

[Kumar Gala]

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 <avorontsov@ru.mvista.com>
> Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

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

- k

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Scott Wood]

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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Sergei Shtylyov]

Hello.

Scott Wood 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.

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

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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Sergei Shtylyov]

Scott Wood 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 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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Scott Wood]

Sergei Shtylyov wrote:
> Hello.
> 
> Scott Wood 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.
> 
>    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.

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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Scott Wood]

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

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

[Sergei Shtylyov]

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.

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

    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);

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

[Scott Wood]

Sergei Shtylyov wrote:
>>+	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

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

[Sergei Shtylyov]

Hello.

Scott Wood wrote:

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

Re: [PATCH 1/2] [IDE] Platform IDE driver

[Scott Wood]

Sergei Shtylyov wrote:
>> 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.

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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Sergei Shtylyov]

Scott Wood wrote:

>> Scott Wood 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.

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

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

    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 :-).

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

> 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

Re: [PATCH 1/2] [IDE] Platform IDE driver

[Sergei Shtylyov]

Scott Wood wrote:

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

    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).

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

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

> -Scott

MBR, Sergei

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

[Guennadi Liakhovetski]

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

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

[Sergei Shtylyov]

Guennadi Liakhovetski 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 

    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

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

[Guennadi Liakhovetski]

On Wed, 25 Jul 2007, Sergei Shtylyov wrote:

> Guennadi Liakhovetski 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 
> 
>    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.

> > 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:-)

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

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

[Sergei Shtylyov]

Guennadi Liakhovetski 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 

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

>>>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:-)

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

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

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

> Thanks
> Guennadi

WBR, Sergei

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

[Guennadi Liakhovetski]

On Thu, 26 Jul 2007, Sergei Shtylyov wrote:

> Guennadi Liakhovetski wrote:
> 
> > >   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. ;-)

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

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

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

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

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

[Alan Cox]

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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Sergei Shtylyov]

Hello, I 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.

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

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

MBR, Sergei

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Vitaly Bordug]

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

> 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...
> 
As Ben already confirmed, it is OK to have "constructor" insert platform device if such is
required by design.

> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>
> 
> >  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";
> 
>     Why not "ata"?
> 

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

> > +		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.
> 
> [...]
> > 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
> [...]
> > @@ -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.
> 
> > +		struct resource res[3];
> > +		struct platform_device *pdev = NULL;
> 
>     Another one.
> 

ok
> > +		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?
> 
agreed.

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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Vitaly Bordug]

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

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

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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Sergei Shtylyov]

Hello.

Vitaly Bordug 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.

> 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

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

[Bartlomiej Zolnierkiewicz]

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 <avorontsov@ru.mvista.com>
> Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

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

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

[Bartlomiej Zolnierkiewicz]

On Wednesday 25 July 2007, Alan Cox wrote:
> > 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 <avorontsov@ru.mvista.com>
> > Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>
> 
> Acked-by: Alan Cox <alan@redhat.com>

added

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

[Guennadi Liakhovetski]

On Wed, 25 Jul 2007, Alan Cox wrote:

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

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

[Sergei Shtylyov]

Hello.

Guennadi Liakhovetski wrote:

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

    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

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

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday 26 July 2007, Guennadi Liakhovetski wrote:
> On Wed, 25 Jul 2007, Alan Cox wrote:
> 
> > > 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

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

[Guennadi Liakhovetski]

On Thu, 26 Jul 2007, Sergei Shtylyov wrote:

> Guennadi Liakhovetski wrote:
> 
> > 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.

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

Re: [PATCH 1/2] [IDE] Platform IDE driver

[Segher Boessenkool]

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

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

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


Segher

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Segher Boessenkool]

>>> +	ide@f0000000 {
>>> +		compatible = "mmio-ide";
>>> +		device_type = "ide";
>
>>     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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Segher Boessenkool]

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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Alan Cox]

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

Re: [PATCH 1/2] [IDE] Platform IDE driver

[Sergei Shtylyov]

Hello.

Segher Boessenkool wrote:

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

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

> 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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Sergei Shtylyov]

Hello.

Segher Boessenkool wrote:

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

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

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

Re: [PATCH 1/2] [IDE] Platform IDE driver

[Segher Boessenkool]

>> 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 ;-)

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

Re: [PATCH 2/2] [POWERPC] MPC8349E-mITX: use platform IDE driver for CF interface

[Segher Boessenkool]

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