[PATCH 1/2] [ide] mmio ide support

[PATCH 1/2] [ide] mmio ide support

[Vitaly Bordug]

This adds support for MMIO IDE device like CompactFlash 
in TrueIDE mode.
 
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> 
Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

---

 drivers/ide/Kconfig           |    6 +
 drivers/ide/legacy/Makefile   |    2 
 drivers/ide/legacy/mmio-ide.c |  211 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+), 0 deletions(-)

diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
index b1a9b81..0dab799 100644
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -946,6 +946,12 @@ config BLK_DEV_MPC8xx_IDE
 
 	  If unsure, say N.
 
+config BLK_DEV_MMIOIDE
+	tristate "Memory Mapped IDE support"
+	help
+	  This is the IDE driver for Memory Mapped IDE devices. Like
+	  Compact Flash running in True IDE mode.
+
 choice
 	prompt "Type of MPC8xx IDE interface"
 	depends on BLK_DEV_MPC8xx_IDE
diff --git a/drivers/ide/legacy/Makefile b/drivers/ide/legacy/Makefile
index c797106..61bd21e 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_MMIOIDE)		+= mmio-ide.o
+
 # Last of all
 obj-$(CONFIG_BLK_DEV_HD)		+= hd.o
 
diff --git a/drivers/ide/legacy/mmio-ide.c b/drivers/ide/legacy/mmio-ide.c
new file mode 100644
index 0000000..77fb7dd
--- /dev/null
+++ b/drivers/ide/legacy/mmio-ide.c
@@ -0,0 +1,211 @@
+/*
+ * Memory Mapped IDE driver
+ *
+ * Author: Kumar Gala <galak@kernel.crashing.org>
+ *
+ * 2007 (c) MontaVista Software, Inc.  
+ * Vitaly Bordug <vitb@kernel.crashing.org> 
+ * Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * 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/mmio-ide.h>
+#include <asm/io.h>
+
+static struct {
+	void __iomem *mmio_ide_mapbase;
+	void __iomem *mmio_ide_alt_mapbase;
+	ide_hwif_t *hwif;
+	int index;
+} hwif_prop;
+
+static ide_hwif_t * __devinit mmio_ide_locate_hwif(void __iomem * base,
+            void __iomem * ctrl, struct mmio_ide_platform_data *pdata, int irq)
+{
+	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 += pdata->regaddr_step + pdata->byte_lanes_swapping;
+	for (i = IDE_ERROR_OFFSET; i <= IDE_STATUS_OFFSET;
+	     i++, port += pdata->regaddr_step)
+		hwif->hw.io_ports[i] = port;
+
+	hwif->hw.io_ports[IDE_CONTROL_OFFSET] = (unsigned long)ctrl +
+		6 * pdata->regaddr_step + pdata->byte_lanes_swapping;
+
+	memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->hw.io_ports));
+	hwif->hw.irq = hwif->irq = irq;
+	hwif->selectproc = pdata->selectproc;
+
+	hwif->hw.dma = NO_DMA;
+	hwif->hw.chipset = ide_generic;
+
+	hwif->mmio = 2;
+	pdata->mmiops(hwif);
+	hwif_prop.hwif = hwif;
+	hwif_prop.index = index;
+
+	return hwif;
+}
+
+static int __devinit mmio_ide_probe(struct platform_device *pdev)
+{
+	struct resource *res_base, *res_alt, *res_irq;
+	ide_hwif_t *hwif;
+	struct mmio_ide_platform_data *pdata;
+	int ret = 0;
+
+	pdata = (struct mmio_ide_platform_data*)pdev->dev.platform_data;
+
+	/* get a pointer to the register memory */
+	res_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res_alt = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+
+	if ((!res_base) || (!res_alt) || (!res_irq)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (!request_mem_region(res_base->start, res_base->end -
+	                                   res_base->start + 1, pdev->name)) {
+		dev_dbg(&pdev->dev, "%s: request_mem_region of base failed\n",
+		        pdev->name);
+		ret = -EBUSY;
+		goto out;
+	}
+
+	if (!request_mem_region(res_alt->start, res_alt->end -
+	                                    res_alt->start + 1, pdev->name)) {
+		dev_dbg(&pdev->dev, "%s: request_mem_region of alt failed\n",
+		        pdev->name);
+		ret = -EBUSY;
+		goto release_base;
+	}
+
+	hwif_prop.mmio_ide_mapbase = ioremap(res_base->start, res_base->end -
+	                                     res_base->start + 1);
+	if (!hwif_prop.mmio_ide_mapbase) {
+		ret = -ENOMEM;
+		goto release_alt;
+	}
+
+	hwif_prop.mmio_ide_alt_mapbase = ioremap(res_alt->start, res_alt->end -
+	                                         res_alt->start + 1);
+	if (!hwif_prop.mmio_ide_alt_mapbase) {
+		ret = -ENOMEM;
+		goto unmap_base;
+	}
+
+	hwif = mmio_ide_locate_hwif(hwif_prop.mmio_ide_mapbase,
+	               hwif_prop.mmio_ide_alt_mapbase, pdata, res_irq->start);
+
+	if (!hwif) {
+		ret = -ENODEV;
+		goto unmap_alt;
+	}
+	hwif->gendev.parent = &pdev->dev;
+	hwif->noprobe = 0;
+
+	probe_hwif_init(hwif);
+
+	platform_set_drvdata(pdev, hwif);
+	create_proc_ide_interfaces();
+
+	return 0;
+
+unmap_alt:
+	iounmap(hwif_prop.mmio_ide_alt_mapbase);
+unmap_base:
+	iounmap(hwif_prop.mmio_ide_mapbase);
+release_alt:
+	release_mem_region(res_alt->start, res_alt->end - res_alt->start + 1);
+release_base:
+	release_mem_region(res_base->start,
+	                   res_base->end - res_base->start + 1);
+out:
+	return ret;
+}
+
+static int __devexit mmio_ide_remove(struct platform_device *pdev)
+{
+	ide_hwif_t *hwif = pdev->dev.driver_data;
+	struct resource *res_base, *res_alt;
+
+	/* get a pointer to the register memory */
+	res_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res_alt = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+
+	release_mem_region(res_base->start,
+	                   res_base->end - res_base->start + 1);
+	release_mem_region(res_alt->start, res_alt->end - res_alt->start + 1);
+
+	platform_set_drvdata(pdev, NULL);
+
+	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;
+	}
+
+	iounmap(hwif_prop.mmio_ide_mapbase);
+	iounmap(hwif_prop.mmio_ide_alt_mapbase);
+
+	return 0;
+}
+
+static struct platform_driver mmio_ide_driver = {
+	.driver {
+		.name = "mmio-ide",
+	},
+	.probe = mmio_ide_probe,
+	.remove = __devexit_p(mmio_ide_remove),
+};
+
+static int __init mmio_ide_init(void)
+{
+	return platform_driver_register(&mmio_ide_driver);
+}
+
+static void __exit mmio_ide_exit(void)
+{
+	platform_driver_unregister(&mmio_ide_driver);
+}
+
+MODULE_DESCRIPTION("Memory Mapped IDE driver");
+MODULE_LICENSE("GPL");
+
+module_init(mmio_ide_init);
+module_exit(mmio_ide_exit);

[PATCH 2/2] [POWERPC] mmio ide support for mpc8349-itx target

[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 |   17 +++++
 arch/powerpc/sysdev/fsl_soc.c          |  113 ++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts
index db0d003..b3e80ab 100644
--- a/arch/powerpc/boot/dts/mpc8349emitx.dts
+++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
@@ -42,7 +42,7 @@
 		#size-cells = <1>;
 		#interrupt-cells = <2>;
 		device_type = "soc";
-		ranges = <0 e0000000 00100000>;
+		ranges = <0 e0000000 1f000000>;
 		reg = <e0000000 00000200>;
 		bus-frequency = <0>;                    // from bootloader
 
@@ -229,6 +229,21 @@
 			descriptor-types-mask = <01010ebf>;
 		};
 
+		ide@10000000 {
+ 			#interrupt-cells = <2>;
+ 			interrupts = <17 8>;
+ 			interrupt-map = <0 0 0 1 700 17 8>;
+ 			interrupt-map-mask = <0>;
+ 
+ 			#size-cells = <1>;
+ 			#address-cells = <1>;
+ 			reg = <10000000 10 10000200 10>;
+ 
+ 			device_type = "ide";
+ 			compatible = "mmio-ide";
+ 			interrupt-parent = < &ipic >;
+ 		};
+
 		ipic: pic@700 {
 			interrupt-controller;
 			#address-cells = <0>;
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index cad1757..b3fe011 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -1103,3 +1103,116 @@ err:
 arch_initcall(cpm_smc_uart_of_init);
 
 #endif /* CONFIG_8xx */
+
+#ifdef CONFIG_MPC834x_ITX
+
+#include <linux/ide.h>
+#include <linux/mmio-ide.h>
+#include <asm-ppc/mpc83xx.h>
+
+static void mmio_ide_outsw(unsigned long port, void *addr, u32 count)
+{
+	_outsw_ns((void __iomem *)port, addr, count);
+}
+
+static void mmio_ide_insw(unsigned long port, void *addr, u32 count)
+{
+	_insw_ns((void __iomem *)port, addr, count);
+}
+
+void mmio_ide_mmiops (ide_hwif_t *hwif)
+{
+	default_hwif_mmiops(hwif);
+	hwif->OUTL = NULL;
+	hwif->OUTSW = mmio_ide_outsw;
+	hwif->OUTSL = NULL;
+	hwif->INL = NULL;
+	hwif->INSW = mmio_ide_insw;
+	hwif->INSL = NULL;
+}
+
+void mmio_ide_selectproc (ide_drive_t *drive)
+{
+	u8 stat;
+
+	stat = drive->hwif->INB(IDE_STATUS_REG);
+	if ((stat & READY_STAT) && (stat & BUSY_STAT))
+		drive->present = 0;
+	else
+		drive->present = 1;
+}
+
+static int __init fsl_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 mmio_ide_platform_data pdata = {
+			/* TODO: pass via OF? */
+			.byte_lanes_swapping = 0,
+			.regaddr_step        = 2,
+			.mmiops              = mmio_ide_mmiops,
+			.selectproc          = mmio_ide_selectproc,
+		};
+
+		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 = "mmio-ide";
+		res[2].flags = IORESOURCE_IRQ;;
+
+		pdev = platform_device_alloc("mmio-ide", 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;
+}
+
+arch_initcall(fsl_mmio_ide_of_init);
+
+#endif /* CONFIG_MPC834x_ITX */

Re: [PATCH 2/2] [POWERPC] mmio ide support for mpc8349-itx target

[Olof Johansson]

On Sat, Jul 07, 2007 at 01:49:00PM +0400, Vitaly Bordug wrote:
> 
> This updates relevant platform code
> (freescale mpc8349itx target) to make the CompactFlash
> work in TrueIDE mode.

Shouldn't you be writing a PCMCIA driver instead for the CF, so it
handles other devices as well? Then you get storage "for free", as
well as hotplug, etc.

It's not hard:

http://patchwork.ozlabs.org/linuxppc/patch?person=124&id=12044


-Olof

Re: [PATCH 2/2] [POWERPC] mmio ide support for mpc8349-itx target

[Arnd Bergmann]

On Saturday 07 July 2007, Olof Johansson wrote:
> On Sat, Jul 07, 2007 at 01:49:00PM +0400, Vitaly Bordug wrote:
> > 
> > This updates relevant platform code
> > (freescale mpc8349itx target) to make the CompactFlash
> > work in TrueIDE mode.
> 
> Shouldn't you be writing a PCMCIA driver instead for the CF, so it
> handles other devices as well? Then you get storage "for free", as
> well as hotplug, etc.

CF memory cards can be in either TrueIDE mode or PCMCIA mode. If you
only need to support memory cards, you should always use True-IDE
mode, because that offers a _much_ higher throughput with PIO mode
6 (25 MB/s) or UDMA mode 6 (133MB/s), compared to the PIO mode 0
(3.3 MB/s) in PCMCIA mode.

Of course, if the bus can only sustain PIO mode 0 anyway, you can
just as well do a PCMCIA driver, and get the ability to plug in
other cards, e.g. wlan or modem cards.

	Arnd <><

Re: [PATCH 2/2] [POWERPC] mmio ide support for mpc8349-itx target

[Sergei Shtylyov]

Hello.

Vitaly Bordug wrote:

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

    It would have been good if you tried to actualy compile your code. ;-)

> diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts
> index db0d003..b3e80ab 100644
> --- a/arch/powerpc/boot/dts/mpc8349emitx.dts
> +++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
> @@ -42,7 +42,7 @@
>  		#size-cells = <1>;
>  		#interrupt-cells = <2>;
>  		device_type = "soc";
> -		ranges = <0 e0000000 00100000>;
> +		ranges = <0 e0000000 1f000000>;
>  		reg = <e0000000 00000200>;
>  		bus-frequency = <0>;                    // from bootloader
>  
> @@ -229,6 +229,21 @@
>  			descriptor-types-mask = <01010ebf>;
>  		};
>  
> +		ide@10000000 {
> + 			#interrupt-cells = <2>;

    Hm, why define that prop for a node with no children?

> + 			interrupts = <17 8>;
> + 			interrupt-map = <0 0 0 1 700 17 8>;
> + 			interrupt-map-mask = <0>;
> + 
> + 			#size-cells = <1>;
> + 			#address-cells = <1>;

    Same question here.

> + 			reg = <10000000 10 10000200 10>;
> + 
> + 			device_type = "ide";

    I think that already adopted device type is "ata", not "ide".

> + 			compatible = "mmio-ide";
> + 			interrupt-parent = < &ipic >;
> + 		};
> +
>  		ipic: pic@700 {
>  			interrupt-controller;
>  			#address-cells = <0>;
> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
> index cad1757..b3fe011 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -1103,3 +1103,116 @@ err:
>  arch_initcall(cpm_smc_uart_of_init);
>  
>  #endif /* CONFIG_8xx */
> +
> +#ifdef CONFIG_MPC834x_ITX

    Erm, isn't this stuff misplaced? Is this really SoC device? I remember 
seeng this in the arch/ppc/ platform code before (in the internal tree)...

> +#include <linux/ide.h>
> +#include <linux/mmio-ide.h>
> +#include <asm-ppc/mpc83xx.h>
> +
> +static void mmio_ide_outsw(unsigned long port, void *addr, u32 count)
> +{
> +	_outsw_ns((void __iomem *)port, addr, count);
> +}
> +
> +static void mmio_ide_insw(unsigned long port, void *addr, u32 count)
> +{
> +	_insw_ns((void __iomem *)port, addr, count);
> +}
> +
> +void mmio_ide_mmiops (ide_hwif_t *hwif)
> +{
> +	default_hwif_mmiops(hwif);
> +	hwif->OUTL = NULL;

    OUTL() method no longer exists.

> +	hwif->OUTSW = mmio_ide_outsw;
> +	hwif->OUTSL = NULL;
> +	hwif->INL = NULL;

    And neither INL() does.

> +	hwif->INSW = mmio_ide_insw;
> +	hwif->INSL = NULL;
> +}
> +
> +void mmio_ide_selectproc (ide_drive_t *drive)
> +{
> +	u8 stat;
> +
> +	stat = drive->hwif->INB(IDE_STATUS_REG);
> +	if ((stat & READY_STAT) && (stat & BUSY_STAT))
> +		drive->present = 0;
> +	else
> +		drive->present = 1;
> +}
> +

    Heh, attempt to emulate hotplug. :-)

> +static int __init fsl_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 mmio_ide_platform_data pdata = {
> +			/* TODO: pass via OF? */
> +			.byte_lanes_swapping = 0,
> +			.regaddr_step        = 2,
> +			.mmiops              = mmio_ide_mmiops,
> +			.selectproc          = mmio_ide_selectproc,

    Definitely, you won't be able to pass methods via of_platform_device. :-)

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

    CodingStyle: space after comma, no space before paren.

[...]
> +		res[2].name = "mmio-ide";
> +		res[2].flags = IORESOURCE_IRQ;;

    One ; too many here. ;-)

MBR, Sergei

Re: [PATCH 2/2] [POWERPC] mmio ide support for mpc8349-itx target

[Segher Boessenkool]

>> +		ide@10000000 {
>> + 			#interrupt-cells = <2>;
>
>     Hm, why define that prop for a node with no children?
>
>> + 			interrupts = <17 8>;
>> + 			interrupt-map = <0 0 0 1 700 17 8>;
>> + 			interrupt-map-mask = <0>;

This map-mask makes no sense either.  And neither does
the map itself.

>> +
>> + 			#size-cells = <1>;
>> + 			#address-cells = <1>;
>
>     Same question here.

The ide node might want children, namely disks or optical
drives or such, but #size-cells should be 0.

>> + 			reg = <10000000 10 10000200 10>;
>> +
>> + 			device_type = "ide";
>
>     I think that already adopted device type is "ata", not "ide".

"ata" is not a hardware thing, "ide" is.  Or so I'm told.
I'd leave out the "device_type" completely though, certainly
if you can't point me at a published device binding :-)


Segher

Re: [PATCH 2/2] [POWERPC] mmio ide support for mpc8349-itx target

[Vitaly Bordug]

On Sat, 07 Jul 2007 20:46:46 +0400
Sergei Shtylyov wrote:

> > +
> > +#ifdef CONFIG_MPC834x_ITX  
> 
>     Erm, isn't this stuff misplaced? Is this really SoC device? I
> remember seeng this in the arch/ppc/ platform code before (in the
> internal tree)...

The point is to declare methods bsp, as mmio access may effectively vary (having bugs or intentionally) from platform to platform. All-in-one driver approach is nice but might be an issue to handle/maintain.

Thanks for looking at it, other notes make sense.

-- 
Sincerely, Vitaly

Re: [PATCH 2/2] [POWERPC] mmio ide support for mpc8349-itx target

[Sergei Shtylyov]

Hello.

Vitaly Bordug wrote:

>>>+
>>>+#ifdef CONFIG_MPC834x_ITX  

>>    Erm, isn't this stuff misplaced? Is this really SoC device? I
>>remember seeng this in the arch/ppc/ platform code before (in the
>>internal tree)...

> The point is to declare methods bsp, as mmio access may effectively vary (having bugs or intentionally) from platform to platform. All-in-one driver approach is nice but might be an issue to handle/maintain.

    You misunderstood. Why this was placed in fsl_soc.c and not in the proper 
platform code (as it was in the internal tree even)?

MBR, Sergei

Re: [PATCH 1/2] [ide] mmio ide support

[Arnd Bergmann]

On Saturday 07 July 2007, Vitaly Bordug wrote:
>=20
> This adds support for MMIO IDE device like CompactFlash=20
> in TrueIDE mode.
> =A0
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>=20
> Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

Hmm, are we still adding new IDE drivers? Do you also have a
driver for the new libata layer? I think it would even be
simpler to do.

You could also make it an of_platform_driver at the same time
instead of adding more cruft to fsl_soc.c. Since we're already
about to add the electra_ide.c driver in 2.6.23, I guess there
should really be _one_ driver that is able to handle all
of_device based ATA hosts.

	Arnd <><

Re: [PATCH 1/2] [ide] mmio ide support

[Sergei Shtylyov]

Arnd Bergmann wrote:

>>This adds support for MMIO IDE device like CompactFlash 
>>in TrueIDE mode.

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

> Hmm, are we still adding new IDE drivers? Do you also have a

    Yes, why not?

> driver for the new libata layer?

    No (sorry for replying for Vitaly but I know the situation).

> I think it would even be simpler to do.

    Wel have no interest in it right now.

> You could also make it an of_platform_driver at the same time
> instead of adding more cruft to fsl_soc.c. Since we're already

    That's a fair point (and fsl_soc.c doesn't seem proper place anyway).

> about to add the electra_ide.c driver in 2.6.23, I guess there
> should really be _one_ driver that is able to handle all
> of_device based ATA hosts.

    One driver to rule them all. :-)
    That may be not so simple as it seems...

MBR, Sergei

Re: [PATCH 1/2] [ide] mmio ide support

[Arnd Bergmann]

On Saturday 07 July 2007, Sergei Shtylyov wrote:
> Arnd Bergmann wrote:
> 
> >>This adds support for MMIO IDE device like CompactFlash 
> >>in TrueIDE mode.
> 
> >>Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> 
> >>Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>
> 
> > Hmm, are we still adding new IDE drivers? Do you also have a
> 
>     Yes, why not?

The last time someone (I think Akira Iguchi) wanted to add
a new powerpc specific IDE driver, that was rejected based
on the argument that drivers/ide/ is going away soon. Most
current distros have already moved over to using libata
exclusively.

> > about to add the electra_ide.c driver in 2.6.23, I guess there
> > should really be _one_ driver that is able to handle all
> > of_device based ATA hosts.
> 
>     One driver to rule them all. :-)
>     That may be not so simple as it seems...

It's certainly not easy to support all ATA chips that
have special capabilities and bugs, but the electra and the
mpc8349-itx both seem to support only the most basic ATA
PIO mode anyway, which does not require much special
handling beyond finding the right mmio addresses.

	Arnd <><

Re: [PATCH 1/2] [ide] mmio ide support

[Bartlomiej Zolnierkiewicz]

Hi,

On Saturday 07 July 2007, Arnd Bergmann wrote:
> On Saturday 07 July 2007, Sergei Shtylyov wrote:
> > Arnd Bergmann wrote:
> > 
> > >>This adds support for MMIO IDE device like CompactFlash 
> > >>in TrueIDE mode.
> > 
> > >>Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> 
> > >>Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>
> > 
> > > Hmm, are we still adding new IDE drivers? Do you also have a
> > 
> >     Yes, why not?
> 
> The last time someone (I think Akira Iguchi) wanted to add
> a new powerpc specific IDE driver, that was rejected based

linux/drivers/ide/pci/scc_pata.c

New IDE host drivers are still fine.

> on the argument that drivers/ide/ is going away soon. Most
> current distros have already moved over to using libata
> exclusively.

The in-kernel default for PATA systems is still IDE subsystem.

Thanks,
Bart

Re: [PATCH 1/2] [ide] mmio ide support

[Linas Vepstas]

On Sun, Jul 08, 2007 at 03:15:41PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> > on the argument that drivers/ide/ is going away soon. Most
> > current distros have already moved over to using libata
> > exclusively.
> 
> The in-kernel default for PATA systems is still IDE subsystem.

In part because libata still does not yet work correctly 
for at least some (older) ide chipsets (like mine).

--linas

Re: [PATCH 1/2] [ide] mmio ide support

[Alan Cox]

> You could also make it an of_platform_driver at the same time
> instead of adding more cruft to fsl_soc.c. Since we're already
> about to add the electra_ide.c driver in 2.6.23, I guess there
> should really be _one_ driver that is able to handle all
> of_device based ATA hosts.

and do so using libata. Preferably ata_platform but it does look like
ata_of would make sense as a companion.

Alan

Re: [PATCH 1/2] [ide] mmio ide support

[Benjamin Herrenschmidt]

On Sat, 2007-07-07 at 21:02 +0100, Alan Cox wrote:
> > You could also make it an of_platform_driver at the same time
> > instead of adding more cruft to fsl_soc.c. Since we're already
> > about to add the electra_ide.c driver in 2.6.23, I guess there
> > should really be _one_ driver that is able to handle all
> > of_device based ATA hosts.
> 
> and do so using libata. Preferably ata_platform but it does look like
> ata_of would make sense as a companion.

I wouldn't push for it too much tho. It's perfectly fine to have
"helpers" generate xx_platform from the device-tree. Since I added the
generic dev_archdata to struct device, it's easy for the arch to keep
track of OF devices for anything, it doesn't have to be an of_platform
device or anything like that anymore.

One of the things I have in mind is to provide a way to register
"constructors" that are based on an OF match set. Those would then be
called by the arch code for every OF node that matches, and would then
"construct" the appropriate linux device. Could be some kind of platform
device, could even be PCI devs.

Ben.

Re: [PATCH 1/2] [ide] mmio ide support

[Olof Johansson]

On Sat, Jul 07, 2007 at 01:48:52PM +0400, Vitaly Bordug wrote:
> 
> This adds support for MMIO IDE device like CompactFlash 
> in TrueIDE mode.

Doesn't this duplicate most of pata_platform, but as the
no-longer-preferred legacy IDE device? Did you try using
pata_platform instead?


-Olof

Re: [PATCH 1/2] [ide] mmio ide support

[Vitaly Bordug]

On Sat, 7 Jul 2007 10:01:47 -0500
Olof Johansson wrote:

> On Sat, Jul 07, 2007 at 01:48:52PM +0400, Vitaly Bordug wrote:
> > 
> > This adds support for MMIO IDE device like CompactFlash 
> > in TrueIDE mode.
> 
> Doesn't this duplicate most of pata_platform, but as the
> no-longer-preferred legacy IDE device? Did you try using
> pata_platform instead?
> 
prolly true,
but as Sergei told upper, that's not an option so far due to internal reasons. But it is likely to be followed-up
that way a little bit later.

> 
> -Olof


-- 
Sincerely, Vitaly

-- 
Sincerely, Vitaly

Re: [PATCH 1/2] [ide] mmio ide support

[Sergei Shtylyov]

Hello.

Vitaly Bordug wrote:

> This adds support for MMIO IDE device like CompactFlash 
> in TrueIDE mode.

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

[...]

> diff --git a/drivers/ide/legacy/mmio-ide.c b/drivers/ide/legacy/mmio-ide.c
> new file mode 100644
> index 0000000..77fb7dd
> --- /dev/null
> +++ b/drivers/ide/legacy/mmio-ide.c
[...]
> +	hwif->mmio = 2;

     That hwif->mmio is single-bit field now, so can only be 0 or 1.

MBR, Sergei

Re: [PATCH 1/2] [ide] mmio ide support

[Alan Cox]

On Sat, 07 Jul 2007 13:48:52 +0400
Vitaly Bordug <vitb@kernel.crashing.org> wrote:

> 
> This adds support for MMIO IDE device like CompactFlash 
> in TrueIDE mode.

Really we should be working towards libata support for all new devices.
This looks like a candidate for the existing (or a little enhanced)
pata_platform driver.

> +config BLK_DEV_MMIOIDE
> +	tristate "Memory Mapped IDE support"

Please pick a better description. This isn't a generic option for
enabling MMIO based IDE as you make it sound.


Also we have an accepted match name for ATA platform devices - and adding
another one messes it up irrespective of whether you want libata or
legacy IDE support. If you use the same matches then your platform code,
and everyone elses platform code can work with both drivers, except for
hotpluggability.

Other bugs

- Your remove code releases the resources before the hwif which means it
races another user trying to claim the resource
- Be careful with ide_unregister. It exists and you can call it but its
actually not very safe and there are lots of unfixed races in the IDE
layer if you do


The "should we have a legacy ide driver that matches the libata
pata_platform" question I don't really care about. Its a waste of effort
in many ways but if you've written the code the work is done so why not
use it.

However it needs to be *compatible* so that platform devices can be
claimed by either so the kernel build can pick legacy IDE v libata and
not have to #ifdef all the platform code.

Alan

Re: [PATCH 1/2] [ide] mmio ide support

[Vitaly Bordug]

On Sat, 7 Jul 2007 21:13:06 +0100
Alan Cox wrote:

> On Sat, 07 Jul 2007 13:48:52 +0400
> Vitaly Bordug <vitb@kernel.crashing.org> wrote:
> 
> > 
> > This adds support for MMIO IDE device like CompactFlash 
> > in TrueIDE mode.
> 
> Really we should be working towards libata support for all new
> devices. This looks like a candidate for the existing (or a little
> enhanced) pata_platform driver.
> 

Yes I am aware of it, yet, the code was created for IDE subsystem due to internal reasons,
and I thought to better make it available for others at least. We'll prolly pick it up to move
to libata/pata_platform but not instantly afaict now.

> > +config BLK_DEV_MMIOIDE
> > +	tristate "Memory Mapped IDE support"
> 
> Please pick a better description. This isn't a generic option for
> enabling MMIO based IDE as you make it sound.
> 
> 
> Also we have an accepted match name for ATA platform devices - and
> adding another one messes it up irrespective of whether you want
> libata or legacy IDE support. If you use the same matches then your
> platform code, and everyone elses platform code can work with both
> drivers, except for hotpluggability.
> 
> Other bugs
> 
> - Your remove code releases the resources before the hwif which means
> it races another user trying to claim the resource
> - Be careful with ide_unregister. It exists and you can call it but
> its actually not very safe and there are lots of unfixed races in the
> IDE layer if you do
> 

OK, makes sense.
> 
> The "should we have a legacy ide driver that matches the libata
> pata_platform" question I don't really care about. Its a waste of
> effort in many ways but if you've written the code the work is done
> so why not use it.
> 
> However it needs to be *compatible* so that platform devices can be
> claimed by either so the kernel build can pick legacy IDE v libata and
> not have to #ifdef all the platform code.
> 

Sounds good. I'll look forward to address the issues, thanks.

> Alan


-- 
Sincerely, Vitaly