[PATCH 0/3] OF-platform PATA driver

[PATCH 0/3] OF-platform PATA driver

[Anton Vorontsov]

Hi all,

Here is the second spin of the OF-platform PATA driver and
related patches.

Changes since RFC:
- nuked drivers/ata/pata_platform.h;
- powerpc bits: proper localbus node added.


Thanks for the previous review! This time I'm collecting acks,
don't be shy to give 'em generously. ;-)


Good luck,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

[PATCH 1/3] [libata] pata_platform: make probe and remove functions device type neutral

[Anton Vorontsov]

Split pata_platform_{probe,remove} into two pieces:
1. pata_platform_{probe,remove} -- platform_device-dependant bits;
2. __ptata_platform_{probe,remove} -- device type neutral bits.

This is done to not duplicate code for the OF-platform driver.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/ata/pata_platform.c   |  139 ++++++++++++++++++++++++-----------------
 include/linux/pata_platform.h |    8 +++
 2 files changed, 90 insertions(+), 57 deletions(-)

diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
index ac03a90..6436c38 100644
--- a/drivers/ata/pata_platform.c
+++ b/drivers/ata/pata_platform.c
@@ -93,14 +93,9 @@ static struct ata_port_operations pata_platform_port_ops = {
 };
 
 static void pata_platform_setup_port(struct ata_ioports *ioaddr,
-				     struct pata_platform_info *info)
+				     unsigned int shift)
 {
-	unsigned int shift = 0;
-
 	/* Fixup the port shift for platforms that need it */
-	if (info && info->ioport_shift)
-		shift = info->ioport_shift;
-
 	ioaddr->data_addr	= ioaddr->cmd_addr + (ATA_REG_DATA    << shift);
 	ioaddr->error_addr	= ioaddr->cmd_addr + (ATA_REG_ERR     << shift);
 	ioaddr->feature_addr	= ioaddr->cmd_addr + (ATA_REG_FEATURE << shift);
@@ -114,8 +109,12 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr,
 }
 
 /**
- *	pata_platform_probe		-	attach a platform interface
- *	@pdev: platform device
+ *	__pata_platform_probe		-	attach a platform interface
+ *	@dev: device
+ *	@io_res: Resource representing I/O base
+ *	@ctl_res: Resource representing CTL base
+ *	@irq_res: Resource representing IRQ and its flags
+ *	@ioport_shift: I/O port shift
  *
  *	Register a platform bus IDE interface. Such interfaces are PIO and we
  *	assume do not support IRQ sharing.
@@ -135,42 +134,17 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr,
  *
  *	If no IRQ resource is present, PIO polling mode is used instead.
  */
-static int __devinit pata_platform_probe(struct platform_device *pdev)
+int __devinit __pata_platform_probe(struct device *dev,
+				    struct resource *io_res,
+				    struct resource *ctl_res,
+				    struct resource *irq_res,
+				    unsigned int ioport_shift)
 {
-	struct resource *io_res, *ctl_res;
 	struct ata_host *host;
 	struct ata_port *ap;
-	struct pata_platform_info *pp_info;
 	unsigned int mmio;
-	int irq;
-
-	/*
-	 * Simple resource validation ..
-	 */
-	if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
-		dev_err(&pdev->dev, "invalid number of resources\n");
-		return -EINVAL;
-	}
-
-	/*
-	 * Get the I/O base first
-	 */
-	io_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
-	if (io_res == NULL) {
-		io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-		if (unlikely(io_res == NULL))
-			return -EINVAL;
-	}
-
-	/*
-	 * Then the CTL base
-	 */
-	ctl_res = platform_get_resource(pdev, IORESOURCE_IO, 1);
-	if (ctl_res == NULL) {
-		ctl_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-		if (unlikely(ctl_res == NULL))
-			return -EINVAL;
-	}
+	int irq = 0;
+	int irq_flags = 0;
 
 	/*
 	 * Check for MMIO
@@ -181,14 +155,15 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)
 	/*
 	 * And the IRQ
 	 */
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		irq = 0;	/* no irq */
+	if (irq_res && irq_res->start > 0) {
+		irq = irq_res->start;
+		irq_flags = irq_res->flags;
+	}
 
 	/*
 	 * Now that that's out of the way, wire up the port..
 	 */
-	host = ata_host_alloc(&pdev->dev, 1);
+	host = ata_host_alloc(dev, 1);
 	if (!host)
 		return -ENOMEM;
 	ap = host->ports[0];
@@ -209,25 +184,24 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)
 	 * Handle the MMIO case
 	 */
 	if (mmio) {
-		ap->ioaddr.cmd_addr = devm_ioremap(&pdev->dev, io_res->start,
+		ap->ioaddr.cmd_addr = devm_ioremap(dev, io_res->start,
 				io_res->end - io_res->start + 1);
-		ap->ioaddr.ctl_addr = devm_ioremap(&pdev->dev, ctl_res->start,
+		ap->ioaddr.ctl_addr = devm_ioremap(dev, ctl_res->start,
 				ctl_res->end - ctl_res->start + 1);
 	} else {
-		ap->ioaddr.cmd_addr = devm_ioport_map(&pdev->dev, io_res->start,
+		ap->ioaddr.cmd_addr = devm_ioport_map(dev, io_res->start,
 				io_res->end - io_res->start + 1);
-		ap->ioaddr.ctl_addr = devm_ioport_map(&pdev->dev, ctl_res->start,
+		ap->ioaddr.ctl_addr = devm_ioport_map(dev, ctl_res->start,
 				ctl_res->end - ctl_res->start + 1);
 	}
 	if (!ap->ioaddr.cmd_addr || !ap->ioaddr.ctl_addr) {
-		dev_err(&pdev->dev, "failed to map IO/CTL base\n");
+		dev_err(dev, "failed to map IO/CTL base\n");
 		return -ENOMEM;
 	}
 
 	ap->ioaddr.altstatus_addr = ap->ioaddr.ctl_addr;
 
-	pp_info = pdev->dev.platform_data;
-	pata_platform_setup_port(&ap->ioaddr, pp_info);
+	pata_platform_setup_port(&ap->ioaddr, ioport_shift);
 
 	ata_port_desc(ap, "%s cmd 0x%llx ctl 0x%llx", mmio ? "mmio" : "ioport",
 		      (unsigned long long)io_res->start,
@@ -235,26 +209,77 @@ static int __devinit pata_platform_probe(struct platform_device *pdev)
 
 	/* activate */
 	return ata_host_activate(host, irq, irq ? ata_interrupt : NULL,
-				 pp_info ? pp_info->irq_flags : 0,
-				 &pata_platform_sht);
+				 irq_flags, &pata_platform_sht);
 }
+EXPORT_SYMBOL_GPL(__pata_platform_probe);
 
 /**
- *	pata_platform_remove	-	unplug a platform interface
- *	@pdev: platform device
+ *	__pata_platform_remove		-	unplug a platform interface
+ *	@dev: device
  *
  *	A platform bus ATA device has been unplugged. Perform the needed
  *	cleanup. Also called on module unload for any active devices.
  */
-static int __devexit pata_platform_remove(struct platform_device *pdev)
+int __devexit __pata_platform_remove(struct device *dev)
 {
-	struct device *dev = &pdev->dev;
 	struct ata_host *host = dev_get_drvdata(dev);
 
 	ata_host_detach(host);
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(__pata_platform_remove);
+
+static int __devinit pata_platform_probe(struct platform_device *pdev)
+{
+	struct resource *io_res;
+	struct resource *ctl_res;
+	struct resource *irq_res;
+	struct pata_platform_info *pp_info = pdev->dev.platform_data;
+
+	/*
+	 * Simple resource validation ..
+	 */
+	if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
+		dev_err(&pdev->dev, "invalid number of resources\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Get the I/O base first
+	 */
+	io_res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (io_res == NULL) {
+		io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (unlikely(io_res == NULL))
+			return -EINVAL;
+	}
+
+	/*
+	 * Then the CTL base
+	 */
+	ctl_res = platform_get_resource(pdev, IORESOURCE_IO, 1);
+	if (ctl_res == NULL) {
+		ctl_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (unlikely(ctl_res == NULL))
+			return -EINVAL;
+	}
+
+	/*
+	 * And the IRQ
+	 */
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (irq_res)
+		irq_res->flags = pp_info ? pp_info->irq_flags : 0;
+
+	return __pata_platform_probe(&pdev->dev, io_res, ctl_res, irq_res,
+				     pp_info ? pp_info->ioport_shift : 0);
+}
+
+static int __devexit pata_platform_remove(struct platform_device *pdev)
+{
+	return __pata_platform_remove(&pdev->dev);
+}
 
 static struct platform_driver pata_platform_driver = {
 	.probe		= pata_platform_probe,
diff --git a/include/linux/pata_platform.h b/include/linux/pata_platform.h
index 5799e8d..0b11f6b 100644
--- a/include/linux/pata_platform.h
+++ b/include/linux/pata_platform.h
@@ -15,4 +15,12 @@ struct pata_platform_info {
 	unsigned int irq_flags;
 };
 
+extern int __devinit __pata_platform_probe(struct device *dev,
+					   struct resource *io_res,
+					   struct resource *ctl_res,
+					   struct resource *irq_res,
+					   unsigned int ioport_shift);
+
+extern int __devexit __pata_platform_remove(struct device *dev);
+
 #endif /* __LINUX_PATA_PLATFORM_H */
-- 
1.5.2.2

Re: [PATCH 1/3] [libata] pata_platform: make probe and remove functions device type neutral

[Arnd Bergmann]

On Tuesday 27 November 2007, Anton Vorontsov wrote:
> Split pata_platform_{probe,remove} into two pieces:
> 1. pata_platform_{probe,remove} -- platform_device-dependant bits;
> 2. __ptata_platform_{probe,remove} -- device type neutral bits.
> 
> This is done to not duplicate code for the OF-platform driver.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

This driver nicely wraps around pata_platform library functions,
and provides OF platform bus bindings to the PATA devices.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/ata/Kconfig            |   10 +++++
 drivers/ata/Makefile           |    1 +
 drivers/ata/pata_of_platform.c |   86 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+), 0 deletions(-)
 create mode 100644 drivers/ata/pata_of_platform.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index ba63619..5a492fa 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -614,6 +614,16 @@ config PATA_PLATFORM
 
 	  If unsure, say N.
 
+config PATA_OF_PLATFORM
+	tristate "OpenFirmware platform device PATA support"
+	depends on PATA_PLATFORM && PPC_OF
+	help
+	  This option enables support for generic directly connected ATA
+	  devices commonly found on embedded systems with OpenFirmware
+	  bindings.
+
+	  If unsure, say N.
+
 config PATA_ICSIDE
 	tristate "Acorn ICS PATA support"
 	depends on ARM && ARCH_ACORN
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index b13feb2..ebcee64 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_PATA_IXP4XX_CF)	+= pata_ixp4xx_cf.o
 obj-$(CONFIG_PATA_SCC)		+= pata_scc.o
 obj-$(CONFIG_PATA_BF54X)	+= pata_bf54x.o
 obj-$(CONFIG_PATA_PLATFORM)	+= pata_platform.o
+obj-$(CONFIG_PATA_OF_PLATFORM)	+= pata_of_platform.o
 obj-$(CONFIG_PATA_ICSIDE)	+= pata_icside.o
 # Should be last but two libata driver
 obj-$(CONFIG_PATA_ACPI)		+= pata_acpi.o
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
new file mode 100644
index 0000000..e6c769c
--- /dev/null
+++ b/drivers/ata/pata_of_platform.c
@@ -0,0 +1,86 @@
+/*
+ * OF-platform PATA driver
+ *
+ * Copyright (c) 2007  MontaVista Software, Inc.
+ *                     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 (Version 2) as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/pata_platform.h>
+
+static int __devinit pata_of_platform_probe(struct of_device *ofdev,
+					    const struct of_device_id *match)
+{
+	int ret;
+	struct device_node *dn = ofdev->node;
+	struct resource io_res;
+	struct resource ctl_res;
+	struct resource irq_res;
+	unsigned int ioport_shift = 0;
+	uint32_t *prop;
+
+	ret = of_address_to_resource(dn, 0, &io_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get IO address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_address_to_resource(dn, 1, &ctl_res);
+	if (ret) {
+		dev_err(&ofdev->dev, "can't get CTL address from "
+			"device tree\n");
+		return -EINVAL;
+	}
+
+	ret = of_irq_to_resource(dn, 0, &irq_res);
+	if (ret == NO_IRQ)
+		irq_res.start = irq_res.end = -1;
+	else
+		irq_res.flags = 0;
+
+	prop = (uint32_t *)of_get_property(dn, "ioport-shift", NULL);
+	if (prop)
+		ioport_shift = *prop;
+
+	return __pata_platform_probe(&ofdev->dev, &io_res, &ctl_res, &irq_res,
+				     ioport_shift);
+}
+
+static int __devexit pata_of_platform_remove(struct of_device *ofdev)
+{
+	return __pata_platform_remove(&ofdev->dev);
+}
+
+static struct of_device_id pata_of_platform_match[] = {
+	{ .compatible = "pata-platform", },
+};
+
+static struct of_platform_driver pata_of_platform_driver = {
+	.name		= "pata_of_platform",
+	.match_table	= pata_of_platform_match,
+	.probe		= pata_of_platform_probe,
+	.remove		= __devexit_p(pata_of_platform_remove),
+};
+
+static int __init pata_of_platform_init(void)
+{
+	return of_register_platform_driver(&pata_of_platform_driver);
+}
+module_init(pata_of_platform_init);
+
+static void __exit pata_of_platform_exit(void)
+{
+	of_unregister_platform_driver(&pata_of_platform_driver);
+}
+module_exit(pata_of_platform_exit);
+
+MODULE_DESCRIPTION("OF-platform PATA driver");
+MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
+MODULE_LICENSE("GPL");
-- 
1.5.2.2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Olof Johansson]

On Tue, Nov 27, 2007 at 06:39:08PM +0300, Anton Vorontsov wrote:
> This driver nicely wraps around pata_platform library functions,
> and provides OF platform bus bindings to the PATA devices.

> +static struct of_device_id pata_of_platform_match[] = {
> +	{ .compatible = "pata-platform", },
> +};

"pata-platform" really means nothing outside of linux. A more
generic label would be useful.


-Olof

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Arnd Bergmann]

On Tuesday 27 November 2007, Olof Johansson wrote:
> On Tue, Nov 27, 2007 at 06:39:08PM +0300, Anton Vorontsov wrote:
> > This driver nicely wraps around pata_platform library functions,
> > and provides OF platform bus bindings to the PATA devices.
>=20
> > +static struct of_device_id pata_of_platform_match[] =3D {
> > +=A0=A0=A0=A0=A0{ .compatible =3D "pata-platform", },
> > +};
>=20
> "pata-platform" really means nothing outside of linux. A more
> generic label would be useful.

Maybe the name of the standards it supports? Could be
"ata-4", "ata-5" and the like, or the exact transfer mode, like
"pata-udma-5", "pata-pio-3", "sata-150", etc.

	Arnd <><

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Tue, Nov 27, 2007 at 10:32:58PM +0100, Arnd Bergmann wrote:
> On Tuesday 27 November 2007, Olof Johansson wrote:
> > On Tue, Nov 27, 2007 at 06:39:08PM +0300, Anton Vorontsov wrote:
> > > This driver nicely wraps around pata_platform library functions,
> > > and provides OF platform bus bindings to the PATA devices.
> > 
> > > +static struct of_device_id pata_of_platform_match[] = {
> > > +     { .compatible = "pata-platform", },
> > > +};
> > 
> > "pata-platform" really means nothing outside of linux. A more
> > generic label would be useful.

Agreed.

> Maybe the name of the standards it supports? Could be
> "ata-4", "ata-5" and the like, or the exact transfer mode, like
> "pata-udma-5", "pata-pio-3", "sata-150", etc.

You're quite optimistic about pata_platform capabilities. ;-)

As far as I know it is [obviously] supports PIO modes only. And so
far I was able to get max 5.28 MB/s read transfers. Which looks like
ideal case for PIO1 (CF I'm testing on is 3.0, max. PIO4).

I've modified pio_mask appropriately, plus I've tried to comment
out .set_mode = pata_platform_set_mode, and now it says:

ata5: PATA max PIO4 mmio cmd 0xf0000000 ctl 0xf000020c irq 24
ata5.00: CFA: TOSHIBA THNCF512MQG, 3.00, max PIO4
ata5.00: configured for PIO4
ata5.00: configured for PIO4

That looks good, but speed is the same. Oh well, it's another
matter.


Back to dts, I think pata-pio-X is good scheme. That way we can
pass pio_mask via device tree. Sounds reasonable?

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>>>This driver nicely wraps around pata_platform library functions,
>>>>and provides OF platform bus bindings to the PATA devices.

>>>>+static struct of_device_id pata_of_platform_match[] = {
>>>>+     { .compatible = "pata-platform", },
>>>>+};

>>>"pata-platform" really means nothing outside of linux. A more
>>>generic label would be useful.

> Agreed.

    Now you're quick to agree. :-)

>>Maybe the name of the standards it supports? Could be
>>"ata-4", "ata-5" and the like, or the exact transfer mode, like
>>"pata-udma-5", "pata-pio-3", "sata-150", etc.

> You're quite optimistic about pata_platform capabilities. ;-)

    Indeed. :-)

> As far as I know it is [obviously] supports PIO modes only. And so
> far I was able to get max 5.28 MB/s read transfers. Which looks like
> ideal case for PIO1 (CF I'm testing on is 3.0, max. PIO4).

    Believe me, it's a great speed even for PIO4. Most systems only show 3+ 
MiB/s in this mode according to hdparm.

> I've modified pio_mask appropriately, plus I've tried to comment
> out .set_mode = pata_platform_set_mode, and now it says:

> ata5: PATA max PIO4 mmio cmd 0xf0000000 ctl 0xf000020c irq 24
> ata5.00: CFA: TOSHIBA THNCF512MQG, 3.00, max PIO4
> ata5.00: configured for PIO4
> ata5.00: configured for PIO4

> That looks good, but speed is the same. Oh well, it's another
> matter.

> Back to dts, I think pata-pio-X is good scheme. That way we can
> pass pio_mask via device tree. Sounds reasonable?

    Grumble. Can't we pass this via some property other than "compatible"? I'm 
opposed to "ata-5" and the like in there as well cause it's not clear what 
information this would provide. Why people so love to make things complex WRT 
the "compatible" property -- instead of making the task of selecting a proper 
driver more simple, they tend to make it mode complex by trying to specify 
values that have quite little to do with the device's programming interface 
itself...

MBR, Sergei

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Wed, Nov 28, 2007 at 07:11:19PM +0300, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
> 
> >>>>This driver nicely wraps around pata_platform library functions,
> >>>>and provides OF platform bus bindings to the PATA devices.
> 
> >>>>+static struct of_device_id pata_of_platform_match[] = {
> >>>>+     { .compatible = "pata-platform", },
> >>>>+};
> 
> >>>"pata-platform" really means nothing outside of linux. A more
> >>>generic label would be useful.
> 
> > Agreed.
> 
>     Now you're quick to agree. :-)

I'm quick to change my mind either. ;-)

*BOOM*, I changed my mind.

> >>Maybe the name of the standards it supports? Could be
> >>"ata-4", "ata-5" and the like, or the exact transfer mode, like
> >>"pata-udma-5", "pata-pio-3", "sata-150", etc.
> 
> > You're quite optimistic about pata_platform capabilities. ;-)
> 
>     Indeed. :-)
> 
> > As far as I know it is [obviously] supports PIO modes only. And so
> > far I was able to get max 5.28 MB/s read transfers. Which looks like
> > ideal case for PIO1 (CF I'm testing on is 3.0, max. PIO4).
> 
>     Believe me, it's a great speed even for PIO4. Most systems only show 3+ 
> MiB/s in this mode according to hdparm.
> 
> > I've modified pio_mask appropriately, plus I've tried to comment
> > out .set_mode = pata_platform_set_mode, and now it says:
> 
> > ata5: PATA max PIO4 mmio cmd 0xf0000000 ctl 0xf000020c irq 24
> > ata5.00: CFA: TOSHIBA THNCF512MQG, 3.00, max PIO4
> > ata5.00: configured for PIO4
> > ata5.00: configured for PIO4
> 
> > That looks good, but speed is the same. Oh well, it's another
> > matter.
> 
> > Back to dts, I think pata-pio-X is good scheme. That way we can
> > pass pio_mask via device tree. Sounds reasonable?
> 
>     Grumble. Can't we pass this via some property other than "compatible"? I'm 
> opposed to "ata-5" and the like in there as well cause it's not clear what 
> information this would provide. Why people so love to make things complex WRT 
> the "compatible" property -- instead of making the task of selecting a proper 
> driver more simple, they tend to make it mode complex by trying to specify 
> values that have quite little to do with the device's programming interface 
> itself...

Ok, now I'm agree here. dts already specifying "fsl,mpc8349emitx-pata",
second compatible entry is okay to mean nothing outside Linux itself,
there are plenty of examples for such kind.

Remaining question: any preferred name for that property? pio-mode okay?
It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Sergei Shtylyov]

Anton Vorontsov wrote:

> Remaining question: any preferred name for that property? pio-mode okay?
> It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.

    I've already suggested "generic". A name "simple" also comes to my mind.

WBR, Sergei

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Fri, Nov 30, 2007 at 01:17:22PM +0300, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
> 
> >Remaining question: any preferred name for that property? pio-mode okay?
> >It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.
> 
>    I've already suggested "generic". A name "simple" also comes to my mind.

You've misread my question. I didn't ask about driver name, but pio-mode
property.


As for the driver name, it doesn't matter at all, as I've said already:
it's Linux specific anyway, and another compatible properties could be
added at any time, to a device tree and/or to the OF driver itself (if
some real OpenFirmware will pass some meaningful compatible property
that we'll have to match in that driver).

"generic" name is also bad one, it's confusing wrt ata_generic.c
driver (PCI). "simple" name doesn't tell anything at all. So, I'd
rather stick with -platform name.

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>>Remaining question: any preferred name for that property? pio-mode okay?
>>>It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.

>>   I've already suggested "generic". A name "simple" also comes to my mind.

> You've misread my question. I didn't ask about driver name, but pio-mode
> property.

    I'm OK with "pio-mode" then. Just don't think it makes much sense in the 
context of this driver which has no provision for the programming the mode 
timings (and if there were some provision, the *generic* platform driver 
couldn't handle it anyway).

> As for the driver name, it doesn't matter at all, as I've said already:
> it's Linux specific anyway, and another compatible properties could be
> added at any time, to a device tree and/or to the OF driver itself (if
> some real OpenFirmware will pass some meaningful compatible property
> that we'll have to match in that driver).

> "generic" name is also bad one, it's confusing wrt ata_generic.c
> driver (PCI).

    The "compatible" property doesn't have to contain the driver name, so 
there should be no confusion with the driver names. It's just different name 
spaces. :-)

> "simple" name doesn't tell anything at all. So, I'd rather stick with -platform name.

    Well, those two should be "generic-ata" and "simple-ata" of course. And it 
*does* tell that the driver just provides taskfile control, without the 
transfer timing control and other fancy stuff...

> Thanks,

MBR, Sergei

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Fri, Nov 30, 2007 at 02:05:01PM +0300, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
> 
> >>>Remaining question: any preferred name for that property? pio-mode okay?
> >>>It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.
> 
> >>  I've already suggested "generic". A name "simple" also comes to my mind.
> 
> >You've misread my question. I didn't ask about driver name, but pio-mode
> >property.
> 
>    I'm OK with "pio-mode" then. Just don't think it makes much sense in the 
> context of this driver which has no provision for the programming the mode 
> timings (and if there were some provision, the *generic* platform driver 
> couldn't handle it anyway).

What sense it makes then?.. We're specifying bus limitations wrt
PIO modes.

Oh, I think you meant that bus can't be reconfigured by generic
driver, that's true. But I didn't mean this as a purpose for
pio-mode property (though it still could be used for that
purpose by other drivers).

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>>Remaining question: any preferred name for that property? pio-mode okay?
>>>It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.

>>   I've already suggested "generic". A name "simple" also comes to my mind.

> You've misread my question. I didn't ask about driver name, but pio-mode
> property.

    Probably "max-pio-mode" is a better variant.

MBR, Sergei

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Anton Vorontsov]

On Fri, Nov 30, 2007 at 02:43:50PM +0300, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
> 
> >>>Remaining question: any preferred name for that property? pio-mode okay?
> >>>It's assuming that PIO6 capable bus supports PIO0 as well, thus no mask.
> 
> >>  I've already suggested "generic". A name "simple" also comes to my mind.
> 
> >You've misread my question. I didn't ask about driver name, but pio-mode
> >property.
> 
>    Probably "max-pio-mode" is a better variant.

Why? pio-mode in pata-platform context is just something highly
dependent on hardware, can not be touched. And we're passing
pio-mode information from the hardware dependent source --
device tree.

The only difference between pata-platform's max-pio-mode and
dedicated ata driver's pio-mode is that that for the first case
bus already configured for specified mode, and for the second
case dedicated driver should actually use this information
to configure hardware.

No need for max- prefix, I think.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Sergei Shtylyov]

Hello.

Arnd Bergmann wrote:

>>>This driver nicely wraps around pata_platform library functions,
>>>and provides OF platform bus bindings to the PATA devices.

>>>+static struct of_device_id pata_of_platform_match[] = {
>>>+     { .compatible = "pata-platform", },
>>>+};

>>"pata-platform" really means nothing outside of linux. A more
>>generic label would be useful.

> Maybe the name of the standards it supports? Could be
> "ata-4", "ata-5" and the like,

    It's not clear what info this would provide.

> or the exact transfer mode, like
> "pata-udma-5", "pata-pio-3", "sata-150", etc.

    I think this info should follow from the compatible property value 
implicitly, or maybe this info should be conveyed in some optional properties. 
It doesn't make sense to the generic platform driver anyway since it has no 
notion about the mode programming specifics. I think that as the device being 
driven is assumed to be a generic IDE device, the "compatible" property should 
contain "generic" or something alike (as well as usual board's and chip's names).

> 	Arnd <><

MBR, Sergei

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

On Tue, 27 Nov 2007 18:39:08 +0300 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
>
> +static int __devinit pata_of_platform_probe(struct of_device *ofdev,
> +					    const struct of_device_id *match)
> +{
> +	uint32_t *prop;

Make this "const uint32_t *prop" or (more kernel like) "const u32 *prop" ...

> +	prop = (uint32_t *)of_get_property(dn, "ioport-shift", NULL);

then you don't need this cast.  Anytime you use a cast to get rid of
"const", is probably a mistake.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] [libata] pata_of_platform: OF-Platform PATA device driver

[Olof Johansson]

On Tue, Nov 27, 2007 at 06:39:08PM +0300, Anton Vorontsov wrote:

> +static struct of_device_id pata_of_platform_match[] = {
> +	{ .compatible = "pata-platform", },
> +};

On top of previous comment about the compatible string being
inappropriate:

You should add a MODULE_DEVICE_TABLE() entry for this. Dave Woodhouse
pointed out it's needed for autoloading modules (i.e. by the fedora
installer, etc).


-Olof

[PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Anton Vorontsov]

This patch adds localbus and pata nodes to use CF IDE interface
on MPC8349E-mITX boards.

Patch also adds code to probe localbus.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/boot/dts/mpc8349emitx.dts    |   17 ++++++++++++++++-
 arch/powerpc/platforms/83xx/mpc834x_itx.c |   17 +++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts
index 5072f6d..7a97068 100644
--- a/arch/powerpc/boot/dts/mpc8349emitx.dts
+++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
@@ -249,6 +249,21 @@
 		device_type = "pci";
 	};
 
+	localbus@e0005000 {
+		#address-cells = <2>;
+		#size-cells = <1>;
+		compatible = "fsl,mpc8349emitx-localbus",
+			     "fsl,mpc8349e-localbus",
+			     "fsl,pq2pro-localbus";
+		reg = <e0005000 d8>;
+		ranges = <3 0 f0000000 210>;
 
-
+		pata@3,0 {
+			compatible = "fsl,mpc8349emitx-pata", "pata-platform";
+			reg = <3 0 10 3 20c 4>;
+			ioport-shift = <1>;
+			interrupts = <17 8>;
+			interrupt-parent = <&ipic>;
+		};
+	};
 };
diff --git a/arch/powerpc/platforms/83xx/mpc834x_itx.c b/arch/powerpc/platforms/83xx/mpc834x_itx.c
index aa76819..ea5f176 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/of_platform.h>
 
 #include <asm/system.h>
 #include <asm/atomic.h>
@@ -37,6 +38,22 @@
 
 #include "mpc83xx.h"
 
+static struct of_device_id mpc834x_itx_ids[] = {
+	{ .name = "localbus", },
+	{},
+};
+
+static int __init mpc834x_itx_declare_of_platform_devices(void)
+{
+	if (!machine_is(mpc834x_itx))
+		return 0;
+
+	of_platform_bus_probe(NULL, mpc834x_itx_ids, NULL);
+
+	return 0;
+}
+device_initcall(mpc834x_itx_declare_of_platform_devices);
+
 /* ************************************************************************
  *
  * Setup the architecture
-- 
1.5.2.2

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Sergei Shtylyov]

Hello.

Anton Vorontsov wrote:

> This patch adds localbus and pata nodes to use CF IDE interface
> on MPC8349E-mITX boards.

> Patch also adds code to probe localbus.

> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  arch/powerpc/boot/dts/mpc8349emitx.dts    |   17 ++++++++++++++++-
>  arch/powerpc/platforms/83xx/mpc834x_itx.c |   17 +++++++++++++++++
>  2 files changed, 33 insertions(+), 1 deletions(-)

> diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts
> index 5072f6d..7a97068 100644
> --- a/arch/powerpc/boot/dts/mpc8349emitx.dts
> +++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
> @@ -249,6 +249,21 @@
>  		device_type = "pci";
>  	};
>  
> +	localbus@e0005000 {
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		compatible = "fsl,mpc8349emitx-localbus",

    Board compatible bus?

> +			     "fsl,mpc8349e-localbus",
> +			     "fsl,pq2pro-localbus";
> +		reg = <e0005000 d8>;
> +		ranges = <3 0 f0000000 210>;
>  
> -
> +		pata@3,0 {
> +			compatible = "fsl,mpc8349emitx-pata", "pata-platform";
> +			reg = <3 0 10 3 20c 4>;
> +			ioport-shift = <1>;

    Bleh... that shift again. And this is surely not a good name for a 
property (where's I/O ports in your case?) -- why not call it "reg-shift" 
(well, I'd call it "reg-size" or "reg-stride" myself :-)?

MBR, Sergei

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Anton Vorontsov]

On Tue, Nov 27, 2007 at 06:49:13PM +0300, Sergei Shtylyov wrote:
> Hello.

Hi Sergei,

> Anton Vorontsov wrote:
> 
> >This patch adds localbus and pata nodes to use CF IDE interface
> >on MPC8349E-mITX boards.
> 
> >Patch also adds code to probe localbus.
> 
> >Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> >---
> > arch/powerpc/boot/dts/mpc8349emitx.dts    |   17 ++++++++++++++++-
> > arch/powerpc/platforms/83xx/mpc834x_itx.c |   17 +++++++++++++++++
> > 2 files changed, 33 insertions(+), 1 deletions(-)
> 
> >diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts 
> >b/arch/powerpc/boot/dts/mpc8349emitx.dts
> >index 5072f6d..7a97068 100644
> >--- a/arch/powerpc/boot/dts/mpc8349emitx.dts
> >+++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
> >@@ -249,6 +249,21 @@
> > 		device_type = "pci";
> > 	};
> > 
> >+	localbus@e0005000 {
> >+		#address-cells = <2>;
> >+		#size-cells = <1>;
> >+		compatible = "fsl,mpc8349emitx-localbus",
> 
>    Board compatible bus?

This is what Documentation/powerpc/booting-without-of.txt suggests
for localbuses. I'm following.

> >+			     "fsl,mpc8349e-localbus",
> >+			     "fsl,pq2pro-localbus";
> >+		reg = <e0005000 d8>;
> >+		ranges = <3 0 f0000000 210>;
> > 
> >-
> >+		pata@3,0 {
> >+			compatible = "fsl,mpc8349emitx-pata", 
> >"pata-platform";
> >+			reg = <3 0 10 3 20c 4>;
> >+			ioport-shift = <1>;
> 
>    Bleh... that shift again. And this is surely not a good name for a 
> property (where's I/O ports in your case?) -- why not call it "reg-shift" 
> (well, I'd call it "reg-size" or "reg-stride" myself :-)?

1. "shift" because pata_platform using that name. I don't see any
   reason to contrive indirections. ioport-shift is what the whole
   Linux kernel using nowadays, and ioport-shift dts property
   anyway Linux-specific.

   I'm just following todays' conventions.

   If you feel really bad about that, I think better to fix that in
   the source of the badness -- pata_platform. It's easy, I can do
   that. Would you ack patch that converts whole pata_platform and
   users? Would Paul ack it?

   Still, is there any hardware that needs not power of 2 stride?

2. "ioport" because shift^Wstride ;-) applies only to the io range
   (yes, it's obvious, but worth open-wording, no?).


And btw, I can get rid of ioport-shift at all. And do fixups in
the pata_of_platform driver via .compatible matching. But I don't
want: it feels bad to list every needs-to-fixup board in the common
driver. It also feels not so great creating something like
pata-platform-stride-{1,2,4,...} compatible stuff. Heh.


Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>>This patch adds localbus and pata nodes to use CF IDE interface
>>>on MPC8349E-mITX boards.

>>>Patch also adds code to probe localbus.

>>>Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>>>---
>>>arch/powerpc/boot/dts/mpc8349emitx.dts    |   17 ++++++++++++++++-
>>>arch/powerpc/platforms/83xx/mpc834x_itx.c |   17 +++++++++++++++++
>>>2 files changed, 33 insertions(+), 1 deletions(-)

>>>diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts 
>>>b/arch/powerpc/boot/dts/mpc8349emitx.dts
>>>index 5072f6d..7a97068 100644
>>>--- a/arch/powerpc/boot/dts/mpc8349emitx.dts
>>>+++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
>>>@@ -249,6 +249,21 @@
>>>		device_type = "pci";
>>>	};
>>>
>>>+	localbus@e0005000 {
>>>+		#address-cells = <2>;
>>>+		#size-cells = <1>;
>>>+		compatible = "fsl,mpc8349emitx-localbus",
>>
>>   Board compatible bus?

> This is what Documentation/powerpc/booting-without-of.txt suggests
> for localbuses. I'm following.

    Hm...

>>>+			     "fsl,mpc8349e-localbus",
>>>+			     "fsl,pq2pro-localbus";
>>>+		reg = <e0005000 d8>;
>>>+		ranges = <3 0 f0000000 210>;
>>>
>>>-
>>>+		pata@3,0 {
>>>+			compatible = "fsl,mpc8349emitx-pata", 
>>>"pata-platform";
>>>+			reg = <3 0 10 3 20c 4>;
>>>+			ioport-shift = <1>;

>>   Bleh... that shift again. And this is surely not a good name for a 
>>property (where's I/O ports in your case?) -- why not call it "reg-shift" 
>>(well, I'd call it "reg-size" or "reg-stride" myself :-)?

> 1. "shift" because pata_platform using that name. I don't see any
>    reason to contrive indirections. ioport-shift is what the whole
>    Linux kernel using nowadays, and ioport-shift dts property
>    anyway Linux-specific.

    It's just a bad name. There's not even I/O ports in this case (and 
moreover, the *real* I/O mapped device would always have a shift of 0, I bet 
-- larger strides are for memory mapped devices).

>    I'm just following todays' conventions.

>    If you feel really bad about that, I think better to fix that in
>    the source of the badness -- pata_platform. It's easy, I can do

    I only feel really bad about the "ioport" part, I can live with "shift" 
part. :-)

>    that. Would you ack patch that converts whole pata_platform and
>    users? Would Paul ack it?

    I don't understand -- why the property name should duplicate pata_platform 
field name? :-O

>    Still, is there any hardware that needs not power of 2 stride?

    Not really -- "size" just seems better, aesthetically. :-)

> 2. "ioport" because shift^Wstride ;-) applies only to the io range
>    (yes, it's obvious, but worth open-wording, no?).

    Contrarywise, to memory range.

> And btw, I can get rid of ioport-shift at all. And do fixups in
> the pata_of_platform driver via .compatible matching. But I don't
> want: it feels bad to list every needs-to-fixup board in the common
> driver. It also feels not so great creating something like
> pata-platform-stride-{1,2,4,...} compatible stuff. Heh.

    I didn't propose neither of that. :-)
    All I want is that "ioport-*" be renamed.

> Thanks,

MBR, Sergei

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Anton Vorontsov]

On Tue, Nov 27, 2007 at 07:46:13PM +0300, Sergei Shtylyov wrote:
[...]
> >>>+			ioport-shift = <1>;
> 
> >>  Bleh... that shift again. And this is surely not a good name for a 
> >>property (where's I/O ports in your case?) -- why not call it "reg-shift" 
> >>(well, I'd call it "reg-size" or "reg-stride" myself :-)?
> 
> >1. "shift" because pata_platform using that name. I don't see any
> >   reason to contrive indirections. ioport-shift is what the whole
> >   Linux kernel using nowadays, and ioport-shift dts property
> >   anyway Linux-specific.
> 
>    It's just a bad name. There's not even I/O ports in this case (and 
> moreover, the *real* I/O mapped device would always have a shift of 0, I 
> bet -- larger strides are for memory mapped devices).
> 
> >   I'm just following todays' conventions.
> 
> >   If you feel really bad about that, I think better to fix that in
> >   the source of the badness -- pata_platform. It's easy, I can do
> 
>    I only feel really bad about the "ioport" part, I can live with "shift" 
> part. :-)
> 
> >   that. Would you ack patch that converts whole pata_platform and
> >   users? Would Paul ack it?
> 
>    I don't understand -- why the property name should duplicate 
>    pata_platform field name? :-O

Because:

> >1.  [...] I don't see any reason to contrive indirections.

That is, different names for single thing is worse than single
bogus name.

>   Not really -- "size" just seems better, aesthetically. :-)

reg-size will look confusing. Is it ata registers' size? No,
can't be. So, what is it? It's stride/shift because of bus, on
which ata resides.

> >And btw, I can get rid of ioport-shift at all. And do fixups in
> >the pata_of_platform driver via .compatible matching. But I don't
> >want: it feels bad to list every needs-to-fixup board in the common
> >driver. It also feels not so great creating something like
> >pata-platform-stride-{1,2,4,...} compatible stuff. Heh.
> 
>    I didn't propose neither of that. :-)

Yup, that was "by the way"...

>    All I want is that "ioport-*" be renamed.

I give up.

The final name is..? I can think out wrong one, so you'd better
convoy me on that way. ;-) reg-shift sounds okay? Or reg-stride
better? No size, please.


Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>   All I want is that "ioport-*" be renamed.

> I give up.

    Don't. :-)

> The final name is..? I can think out wrong one, so you'd better
> convoy me on that way. ;-) reg-shift sounds okay?

    Yes.

> Thanks,

WBR, Sergei

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Anton Vorontsov]

[ Had cut too much in the previous reply ]

On Tue, Nov 27, 2007 at 07:46:13PM +0300, Sergei Shtylyov wrote:
[...]
> >2. "ioport" because shift^Wstride ;-) applies only to the io range
> >   (yes, it's obvious, but worth open-wording, no?).
> 
>    Contrarywise, to memory range.

By io range I meant "I/O base", in contrast to "CTL base".

There is no need to apply shifting for CTL. That's why ioport-*
appeared in the first place.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>>2. "ioport" because shift^Wstride ;-) applies only to the io range
>>>  (yes, it's obvious, but worth open-wording, no?).

>>   Contrarywise, to memory range.

> By io range I meant "I/O base", in contrast to "CTL base".

> There is no need to apply shifting for CTL. That's why ioport-*
> appeared in the first place.

    So, a matter of wrong terminology then. The thing that you meant by "I/O" 
is actually called "command block".

MBR, Sergei

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Anton Vorontsov]

On Tue, Nov 27, 2007 at 08:34:11PM +0300, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
> 
> >>>2. "ioport" because shift^Wstride ;-) applies only to the io range
> >>> (yes, it's obvious, but worth open-wording, no?).
> 
> >>  Contrarywise, to memory range.
> 
> >By io range I meant "I/O base", in contrast to "CTL base".
> 
> >There is no need to apply shifting for CTL. That's why ioport-*
> >appeared in the first place.
> 
>    So, a matter of wrong terminology then. The thing that you meant by 
>    "I/O" is actually called "command block".

Yes. And IO is the second name. It's used widespread in the
drivers/ide/.

Now you understand why I'm so reluctant to hanging up different
labels on the single thing? ;-)

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Sergei Shtylyov]

Anton Vorontsov wrote:

>>>>>2. "ioport" because shift^Wstride ;-) applies only to the io range
>>>>>(yes, it's obvious, but worth open-wording, no?).

>>>> Contrarywise, to memory range.

>>>By io range I meant "I/O base", in contrast to "CTL base".

>>>There is no need to apply shifting for CTL. That's why ioport-*
>>>appeared in the first place.

>>   So, a matter of wrong terminology then. The thing that you meant by 
>>   "I/O" is actually called "command block".

> Yes. And IO is the second name.

    I'd say the first place in drivers/ide belongs to the historic name 
"taskfile". The "command block" which is as ATA standard calls it, is hardly used.

> It's used widespread in the drivers/ide/.

    Don't remember seeing it.

> Now you understand why I'm so reluctant to hanging up different
> labels on the single thing? ;-)

    :-)

MBR, Sergei

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Anton Vorontsov]

On Tue, Nov 27, 2007 at 09:07:14PM +0300, Sergei Shtylyov wrote:
> Anton Vorontsov wrote:
> 
> >>>>>2. "ioport" because shift^Wstride ;-) applies only to the io range
> >>>>>(yes, it's obvious, but worth open-wording, no?).
> 
> >>>> Contrarywise, to memory range.
> 
> >>>By io range I meant "I/O base", in contrast to "CTL base".
> 
> >>>There is no need to apply shifting for CTL. That's why ioport-*
> >>>appeared in the first place.
> 
> >>   So, a matter of wrong terminology then. The thing that you meant by 
> >>   "I/O" is actually called "command block".
> 
> > Yes. And IO is the second name.
> 
>     I'd say the first place in drivers/ide belongs to the historic name 
> "taskfile". The "command block" which is as ATA standard calls it, is hardly used.
> 
> > It's used widespread in the drivers/ide/.
> 
>     Don't remember seeing it.

Oops, thinko -- not drivers/ide/, but include/linux/ide.h.
Grep for io_addr.

> > Now you understand why I'm so reluctant to hanging up different
> > labels on the single thing? ;-)
> 
>     :-)

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 3/3] [POWERPC] MPC8349E-mITX: introduce localbus and pata nodes

[Kumar Gala]

On Nov 27, 2007, at 9:49 AM, Sergei Shtylyov wrote:

> Hello.
>
> Anton Vorontsov wrote:
>
>> This patch adds localbus and pata nodes to use CF IDE interface
>> on MPC8349E-mITX boards.
>
>> Patch also adds code to probe localbus.
>
>> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>> ---
>> arch/powerpc/boot/dts/mpc8349emitx.dts    |   17 ++++++++++++++++-
>> arch/powerpc/platforms/83xx/mpc834x_itx.c |   17 +++++++++++++++++
>> 2 files changed, 33 insertions(+), 1 deletions(-)
>
>> diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/=20=

>> boot/dts/mpc8349emitx.dts
>> index 5072f6d..7a97068 100644
>> --- a/arch/powerpc/boot/dts/mpc8349emitx.dts
>> +++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
>> @@ -249,6 +249,21 @@
>> 		device_type =3D "pci";
>> 	};
>>
>> +	localbus@e0005000 {
>> +		#address-cells =3D <2>;
>> +		#size-cells =3D <1>;
>> +		compatible =3D "fsl,mpc8349emitx-localbus",
>
>    Board compatible bus?
>
>> +			     "fsl,mpc8349e-localbus",
>> +			     "fsl,pq2pro-localbus";
>> +		reg =3D <e0005000 d8>;
>> +		ranges =3D <3 0 f0000000 210>;
>>
>> -
>> +		pata@3,0 {
>> +			compatible =3D "fsl,mpc8349emitx-pata", =
"pata-platform";
>> +			reg =3D <3 0 10 3 20c 4>;
>> +			ioport-shift =3D <1>;
>
>    Bleh... that shift again. And this is surely not a good name for a
> property (where's I/O ports in your case?) -- why not call it "reg-=20
> shift"
> (well, I'd call it "reg-size" or "reg-stride" myself :-)?

I'm coming into this late, but if ioport-shift applies to reg (which I =20=

think it does) it should really be called "reg-shift".  The ePAPR is =20
using that property name:

Specifies in bytes how far the discrete device registers are separated =20=

from each other. The
individual register location is calculated by using following formula: =20=

=93registers address=94 <<
reg-shift. If unspecified the default value is 0.

- k

Re: [PATCH 0/3] OF-platform PATA driver

[Paul Mundt]

On Tue, Nov 27, 2007 at 06:37:08PM +0300, Anton Vorontsov wrote:
> Here is the second spin of the OF-platform PATA driver and
> related patches.
> 
So either the patches are missing, or I wasn't CC'ed on them. I'm going
to go out on a limb and assume the latter. If you wish me to Ack them,
I'm not going to start grovelling around list archives trying to find the
relevant posts.

This is now the second time this has happened, the first time I was only
made aware of this work as Jeff forwarded them along. CC'ing the authors
of code you are changing shouldn't be a cryptic requirement we have to
spell out in SubmittingPatches or so, especially if you're soliciting
Acked-bys. Please fix your development methodology, thanks.

Re: [PATCH 0/3] OF-platform PATA driver

[Anton Vorontsov]

On Wed, Nov 28, 2007 at 06:51:51PM +0900, Paul Mundt wrote:
> On Tue, Nov 27, 2007 at 06:37:08PM +0300, Anton Vorontsov wrote:
> > Here is the second spin of the OF-platform PATA driver and
> > related patches.
> > 
> So either the patches are missing, or I wasn't CC'ed on them. I'm going
> to go out on a limb and assume the latter. If you wish me to Ack them,
> I'm not going to start grovelling around list archives trying to find the
> relevant posts.

Point taken, fair enough. I'll Cc you explicitly next time.

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 0/3] OF-platform PATA driver

[Jeff Garzik]

Anton Vorontsov wrote:
> Hi all,
> 
> Here is the second spin of the OF-platform PATA driver and
> related patches.
> 
> Changes since RFC:
> - nuked drivers/ata/pata_platform.h;
> - powerpc bits: proper localbus node added.
> 
> 
> Thanks for the previous review! This time I'm collecting acks,
> don't be shy to give 'em generously. ;-)

if (ack_collected(PaulM))
	push(ACK)
else {
	/* do nothing */
}

Re: [PATCH 0/3] OF-platform PATA driver

[Anton Vorontsov]

On Sat, Dec 01, 2007 at 05:54:49PM -0500, Jeff Garzik wrote:
> Anton Vorontsov wrote:
> > Hi all,
> > 
> > Here is the second spin of the OF-platform PATA driver and
> > related patches.
> > 
> > Changes since RFC:
> > - nuked drivers/ata/pata_platform.h;
> > - powerpc bits: proper localbus node added.
> > 
> > 
> > Thanks for the previous review! This time I'm collecting acks,
> > don't be shy to give 'em generously. ;-)
> if (ack_collected(PaulM))
> 	push(ACK)
> else {
> 	/* do nothing */
> }

Yep, this is obvious.

Here is the algo I'm following:

while (1) {
	send_patches();

	if (ack_collected(PaulM) && ack_collected(PowerPC_people))
		break;

	sleep(wait_for_comments_timeout); <-- currently here.
}

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 0/3] OF-platform PATA driver

[Olof Johansson]

On Sun, Dec 02, 2007 at 02:58:10AM +0300, Anton Vorontsov wrote:
> On Sat, Dec 01, 2007 at 05:54:49PM -0500, Jeff Garzik wrote:
> while (1) {
> 	send_patches();
> 
> 	if (ack_collected(PaulM) && ack_collected(PowerPC_people))
> 		break;
> 
> 	sleep(wait_for_comments_timeout); <-- currently here.

I still haven't seen you address the compatible comments (that
pata-platform is suboptimal). Or did I miss some respin of the patches?


-Olof

Re: [PATCH 0/3] OF-platform PATA driver

[Anton Vorontsov]

On Sat, Dec 01, 2007 at 09:57:55PM -0600, Olof Johansson wrote:
> On Sun, Dec 02, 2007 at 02:58:10AM +0300, Anton Vorontsov wrote:
> > On Sat, Dec 01, 2007 at 05:54:49PM -0500, Jeff Garzik wrote:
> > while (1) {
> > 	send_patches();
> > 
> > 	if (ack_collected(PaulM) && ack_collected(PowerPC_people))
> > 		break;
> > 
> > 	sleep(wait_for_comments_timeout); <-- currently here.
> 
> I still haven't seen you address the compatible comments (that
> pata-platform is suboptimal). Or did I miss some respin of the patches?

I didn't resend these patches yet. You started the thread, but you hid
away from the discussion (you had been Cc'ed to every mail).

1. Arnd suggested {p,s}ata-pio-{1,2,3,..} or ata-{1,2,3,..} compatible
   scheme;
2. Sergei verbosely explained that that there is no reason to
   complicate compatible property. He suggested ata-generic, or
   ata-simple;

Mainly I was awaiting for your further comments. By now I tend to
follow Sergei's comments and rename compatible stuff to ata-generic.
Are you fine with it?

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH 0/3] OF-platform PATA driver

[Olof Johansson]

On Sun, Dec 02, 2007 at 02:46:17PM +0300, Anton Vorontsov wrote:
> On Sat, Dec 01, 2007 at 09:57:55PM -0600, Olof Johansson wrote:
> > On Sun, Dec 02, 2007 at 02:58:10AM +0300, Anton Vorontsov wrote:
> > > On Sat, Dec 01, 2007 at 05:54:49PM -0500, Jeff Garzik wrote:
> > > while (1) {
> > > 	send_patches();
> > > 
> > > 	if (ack_collected(PaulM) && ack_collected(PowerPC_people))
> > > 		break;
> > > 
> > > 	sleep(wait_for_comments_timeout); <-- currently here.
> > 
> > I still haven't seen you address the compatible comments (that
> > pata-platform is suboptimal). Or did I miss some respin of the patches?
> 
> I didn't resend these patches yet. You started the thread, but you hid
> away from the discussion (you had been Cc'ed to every mail).
> 
> 1. Arnd suggested {p,s}ata-pio-{1,2,3,..} or ata-{1,2,3,..} compatible
>    scheme;
> 2. Sergei verbosely explained that that there is no reason to
>    complicate compatible property. He suggested ata-generic, or
>    ata-simple;
> 
> Mainly I was awaiting for your further comments. By now I tend to
> follow Sergei's comments and rename compatible stuff to ata-generic.
> Are you fine with it?

Ah, sorry about that. The discussion got a bit noisy so I tuned it out.

Yes, I agree with Sergei: I don't think the pio/ata mode belongs there,
"ata-generic" (or "generic-ata") sounds good to me. If firmwares want to
specify pata/sata stuff as more specific compatible fields, then they are
free to, but the driver shouldn't depend on it as long as "ata-generic"
is in the list.

ATA/PIO modes could be communicated with separate properties as well,
but I don't see a need for it right now. Tuning PIO modes normally
involves changing bus parameters/cycle times, etc, and that' hardly
generic enough to be handled by this driver in the first place.

I will need to add a compatible field to it myself (our firmware uses
"electra-ide") + some minor changes, but I will do that on top of this
patch once it goes in since it involves removing the old code from
arch/powerpc/platforms/pasemi.


-Olof

Re: [PATCH 0/3] OF-platform PATA driver

[Arnd Bergmann]

On Sunday 02 December 2007, Olof Johansson wrote:

> Yes, I agree with Sergei: I don't think the pio/ata mode belongs there,
> "ata-generic" (or "generic-ata") sounds good to me. If firmwares want to
> specify pata/sata stuff as more specific compatible fields, then they are
> free to, but the driver shouldn't depend on it as long as "ata-generic"
> is in the list.

Yes, that makes sense. Let me just make it clear that I no longer think
we should have the PIO mode or the ATA level in the compatible property.
The PIO modes should really have their own property, if any, and the ATA
standard would need to be a property of the attached device, not a property
of the controller.

I can see a point in listing the ATA devices themselves in the device tree,
but it's probably not really worth it for the fdt setup, since they
can easily be probed at runtime.

> I will need to add a compatible field to it myself (our firmware uses
> "electra-ide") + some minor changes, but I will do that on top of this
> patch once it goes in since it involves removing the old code from
> arch/powerpc/platforms/pasemi.

I'd suggest to also have electra-ide hardcoded in the driver alongside
ata-generic, as a special case for backwards compatibility.

	Arnd <><

Re: [PATCH 0/3] OF-platform PATA driver

[Olof Johansson]

On Mon, Dec 03, 2007 at 12:40:21AM +0100, Arnd Bergmann wrote:
> On Sunday 02 December 2007, Olof Johansson wrote:
> 
> > Yes, I agree with Sergei: I don't think the pio/ata mode belongs there,
> > "ata-generic" (or "generic-ata") sounds good to me. If firmwares want to
> > specify pata/sata stuff as more specific compatible fields, then they are
> > free to, but the driver shouldn't depend on it as long as "ata-generic"
> > is in the list.
> 
> Yes, that makes sense. Let me just make it clear that I no longer think
> we should have the PIO mode or the ATA level in the compatible property.
> The PIO modes should really have their own property, if any, and the ATA
> standard would need to be a property of the attached device, not a property
> of the controller.

Right. And configuring PIO modes is something quite hardware specific,
which defeats the purpose of having a "generic" driver as a catch-all
for "simple" configs in the first place. If the hardware needs (much)
tuning it probably makes more sense to do it as a separate driver.

> I can see a point in listing the ATA devices themselves in the device tree,
> but it's probably not really worth it for the fdt setup, since they
> can easily be probed at runtime.

If the firmware probes them and/or knows about them. It's the kind of
thing that can change quite often. We don't list USB devices under the usb
controllers either, since they're quite volatile and the device tree is
(fairly) static on the DTS-based platforms.

> > I will need to add a compatible field to it myself (our firmware uses
> > "electra-ide") + some minor changes, but I will do that on top of this
> > patch once it goes in since it involves removing the old code from
> > arch/powerpc/platforms/pasemi.
> 
> I'd suggest to also have electra-ide hardcoded in the driver alongside
> ata-generic, as a special case for backwards compatibility.

Yep, that's my plan, I'll do that when I move it over.


-Olof