[PATCH v2 0/2] Update ehci-platform driver to support devicetree

[PATCH v2 0/2] Update ehci-platform driver to support devicetree

[Tony Prisk]

This patchset updates the ehci-platform.c driver to allow device tree probing.
I have dropped support for the three function pointers (power_on, power_off
and power_suspend). If someone has knowledge of the power sequence functions
that are being implemented, these functions could be replaced (Sorry, I don't
know anything about implementing it).

port_power_(on_off) properties are not supported in DT as Alan Stern indicated
they are going to be removed.

v2:
* Add error checking for pdata memory allocation, and memory freeing in remove()

* Add no_io_watchdog field to usb_ehci_pdata as required by Florian Fainelli's
upcoming patchset and DT parsing for the field. This means we don't need to
patch this driver again after applying Florian's patchset, but will create a
minor merge-conflict.

* Changed compatible string to 'linux,ehci-platform' as a lot of the DT
properties are linux-specific.

* Changed properties to match those expected in usb-ehci binding.


Tony Prisk (2):
  USB: Update EHCI-platform driver to devicetree.
  USB: doc: Binding document for ehci-platform driver

 .../devicetree/bindings/usb/ehci-platform.txt      |   27 ++++
 drivers/usb/host/ehci-hcd.c                        |    5 -
 drivers/usb/host/ehci-platform.c                   |   61 ++++++-
 drivers/usb/host/ehci-vt8500.c                     |  171 --------------------
 include/linux/usb/ehci_pdriver.h                   |    1 +
 5 files changed, 88 insertions(+), 177 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt
 delete mode 100644 drivers/usb/host/ehci-vt8500.c

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/2] Update ehci-platform driver to support devicetree

[Tony Prisk]

This patchset updates the ehci-platform.c driver to allow device tree probing.
I have dropped support for the three function pointers (power_on, power_off
and power_suspend). If someone has knowledge of the power sequence functions
that are being implemented, these functions could be replaced (Sorry, I don't
know anything about implementing it).

port_power_(on_off) properties are not supported in DT as Alan Stern indicated
they are going to be removed.

v2:
* Add error checking for pdata memory allocation, and memory freeing in remove()

* Add no_io_watchdog field to usb_ehci_pdata as required by Florian Fainelli's
upcoming patchset and DT parsing for the field. This means we don't need to
patch this driver again after applying Florian's patchset, but will create a
minor merge-conflict.

* Changed compatible string to 'linux,ehci-platform' as a lot of the DT
properties are linux-specific.

* Changed properties to match those expected in usb-ehci binding.


Tony Prisk (2):
  USB: Update EHCI-platform driver to devicetree.
  USB: doc: Binding document for ehci-platform driver

 .../devicetree/bindings/usb/ehci-platform.txt      |   27 ++++
 drivers/usb/host/ehci-hcd.c                        |    5 -
 drivers/usb/host/ehci-platform.c                   |   61 ++++++-
 drivers/usb/host/ehci-vt8500.c                     |  171 --------------------
 include/linux/usb/ehci_pdriver.h                   |    1 +
 5 files changed, 88 insertions(+), 177 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt
 delete mode 100644 drivers/usb/host/ehci-vt8500.c

-- 
1.7.9.5

[PATCH v2 1/2] USB: Update EHCI-platform driver to devicetree.

[Tony Prisk]

This patch adds devicetree support to the EHCI-platform driver,
and removes the now unneeded ehci-vt8500.c

Existing platform properties are maintained, with the exception
the power_(on/off) and suspend function pointers.

Signed-off-by: Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>
---
 drivers/usb/host/ehci-hcd.c      |    5 --
 drivers/usb/host/ehci-platform.c |   61 +++++++++++++-
 drivers/usb/host/ehci-vt8500.c   |  171 --------------------------------------
 include/linux/usb/ehci_pdriver.h |    1 +
 4 files changed, 61 insertions(+), 177 deletions(-)
 delete mode 100644 drivers/usb/host/ehci-vt8500.c

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 6bf6c42..42c8e84 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1274,11 +1274,6 @@ MODULE_LICENSE ("GPL");
 #define PLATFORM_DRIVER		cns3xxx_ehci_driver
 #endif
 
-#ifdef CONFIG_ARCH_VT8500
-#include "ehci-vt8500.c"
-#define	PLATFORM_DRIVER		vt8500_ehci_driver
-#endif
-
 #ifdef CONFIG_PLAT_SPEAR
 #include "ehci-spear.c"
 #define PLATFORM_DRIVER		spear_ehci_hcd_driver
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index 764e010..f44fc6a 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -19,6 +19,7 @@
  * Licensed under the GNU/GPL. See COPYING for details.
  */
 #include <linux/platform_device.h>
+#include <linux/of.h>
 #include <linux/usb/ehci_pdriver.h>
 
 static int ehci_platform_reset(struct usb_hcd *hcd)
@@ -78,14 +79,60 @@ static const struct hc_driver ehci_platform_hc_driver = {
 	.clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
 };
 
+static u64 ehci_dma_mask = DMA_BIT_MASK(32);
+
 static int __devinit ehci_platform_probe(struct platform_device *dev)
 {
 	struct usb_hcd *hcd;
 	struct resource *res_mem;
 	struct usb_ehci_pdata *pdata = dev->dev.platform_data;
+	struct device_node *np = dev->dev.of_node;
 	int irq;
 	int err = -ENOMEM;
 
+	/* Are we being initialized from a DT-probed device? */
+	if (np) {
+		/*
+		 * No platform data is being passed, so initalize pdata.
+		 * Limitation: we can't support power_on, power_off or
+		 * power_suspend function pointers from DT.
+		 * TODO: The missing functions could be replaced with
+		 * power sequence handlers.
+		 */
+		pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
+		dev->dev.platform_data = pdata;
+
+		if (!pdata) {
+			pr_err("device tree platform data allocation failed\n");
+			return -ENOMEM;
+		}
+
+		/* Read the optional properties from DT node */
+		of_property_read_u32(np, "caps-offset", &pdata->caps_offset);
+		if (of_property_read_bool(np, "no_io_watchdog"))
+			pdata->no_io_watchdog = 1;
+		if (of_property_read_bool(np, "has-tt"))
+			pdata->has_tt = 1;
+		if (of_property_read_bool(np, "has-synopsys-hc-bug"))
+			pdata->has_synopsys_hc_bug = 1;
+
+		if (of_property_read_bool(np, "big-endian")) {
+			pdata->big_endian_desc = 1;
+			pdata->big_endian_mmio = 1;
+		} else {
+			if (of_property_read_bool(np, "big-endian-desc"))
+				pdata->big_endian_desc = 1;
+			if (of_property_read_bool(np, "big-endian-regs"))
+				pdata->big_endian_mmio = 1;
+		}
+		/* Right now device-tree probed devices don't get dma_mask set.
+		 * Since shared usb code relies on it, set it here for now.
+		 * Once we have dma capability bindings this can go away.
+		 */
+		if (!dev->dev.dma_mask)
+			dev->dev.dma_mask = &ehci_dma_mask;
+	}
+
 	if (!pdata) {
 		WARN_ON(1);
 		return -ENODEV;
@@ -101,7 +148,7 @@ static int __devinit ehci_platform_probe(struct platform_device *dev)
 	}
 	res_mem = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	if (!res_mem) {
-		pr_err("no memory recourse provided");
+		pr_err("no memory resource provided");
 		return -ENXIO;
 	}
 
@@ -163,6 +210,7 @@ static int __devexit ehci_platform_remove(struct platform_device *dev)
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
 	usb_put_hcd(hcd);
 	platform_set_drvdata(dev, NULL);
+	devm_kfree(&dev->dev, pdata);
 
 	if (pdata->power_off)
 		pdata->power_off(dev);
@@ -215,6 +263,16 @@ static const struct platform_device_id ehci_platform_table[] = {
 	{ "ehci-platform", 0 },
 	{ }
 };
+
+#ifdef CONFIG_OF
+static const struct of_device_id ehci_platform_ids[] = {
+	{ .compatible = "linux,ehci-platform", },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, ehci_platform_ids);
+#endif
+
 MODULE_DEVICE_TABLE(platform, ehci_platform_table);
 
 static const struct dev_pm_ops ehci_platform_pm_ops = {
@@ -231,5 +289,6 @@ static struct platform_driver ehci_platform_driver = {
 		.owner	= THIS_MODULE,
 		.name	= "ehci-platform",
 		.pm	= &ehci_platform_pm_ops,
+		.of_match_table = of_match_ptr(ehci_platform_ids),
 	}
 };
diff --git a/drivers/usb/host/ehci-vt8500.c b/drivers/usb/host/ehci-vt8500.c
deleted file mode 100644
index d3c9a3e..0000000
--- a/drivers/usb/host/ehci-vt8500.c
+++ /dev/null
@@ -1,171 +0,0 @@
-/*
- * drivers/usb/host/ehci-vt8500.c
- *
- * Copyright (C) 2010 Alexey Charkov <alchark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
- *
- * Based on ehci-au1xxx.c
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-#include <linux/of.h>
-#include <linux/platform_device.h>
-
-static int ehci_update_device(struct usb_hcd *hcd, struct usb_device *udev)
-{
-	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
-	int rc = 0;
-
-	if (!udev->parent) /* udev is root hub itself, impossible */
-		rc = -1;
-	/* we only support lpm device connected to root hub yet */
-	if (ehci->has_lpm && !udev->parent->parent) {
-		rc = ehci_lpm_set_da(ehci, udev->devnum, udev->portnum);
-		if (!rc)
-			rc = ehci_lpm_check(ehci, udev->portnum);
-	}
-	return rc;
-}
-
-static const struct hc_driver vt8500_ehci_hc_driver = {
-	.description		= hcd_name,
-	.product_desc		= "VT8500 EHCI",
-	.hcd_priv_size		= sizeof(struct ehci_hcd),
-
-	/*
-	 * generic hardware linkage
-	 */
-	.irq			= ehci_irq,
-	.flags			= HCD_MEMORY | HCD_USB2,
-
-	/*
-	 * basic lifecycle operations
-	 */
-	.reset			= ehci_setup,
-	.start			= ehci_run,
-	.stop			= ehci_stop,
-	.shutdown		= ehci_shutdown,
-
-	/*
-	 * managing i/o requests and associated device resources
-	 */
-	.urb_enqueue		= ehci_urb_enqueue,
-	.urb_dequeue		= ehci_urb_dequeue,
-	.endpoint_disable	= ehci_endpoint_disable,
-	.endpoint_reset		= ehci_endpoint_reset,
-
-	/*
-	 * scheduling support
-	 */
-	.get_frame_number	= ehci_get_frame,
-
-	/*
-	 * root hub support
-	 */
-	.hub_status_data	= ehci_hub_status_data,
-	.hub_control		= ehci_hub_control,
-	.bus_suspend		= ehci_bus_suspend,
-	.bus_resume		= ehci_bus_resume,
-	.relinquish_port	= ehci_relinquish_port,
-	.port_handed_over	= ehci_port_handed_over,
-
-	/*
-	 * call back when device connected and addressed
-	 */
-	.update_device =	ehci_update_device,
-
-	.clear_tt_buffer_complete	= ehci_clear_tt_buffer_complete,
-};
-
-static u64 vt8500_ehci_dma_mask = DMA_BIT_MASK(32);
-
-static int vt8500_ehci_drv_probe(struct platform_device *pdev)
-{
-	struct usb_hcd *hcd;
-	struct ehci_hcd *ehci;
-	struct resource *res;
-	int ret;
-
-	if (usb_disabled())
-		return -ENODEV;
-
-	/*
-	 * Right now device-tree probed devices don't get dma_mask set.
-	 * Since shared usb code relies on it, set it here for now.
-	 * Once we have dma capability bindings this can go away.
-	 */
-	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &vt8500_ehci_dma_mask;
-
-	if (pdev->resource[1].flags != IORESOURCE_IRQ) {
-		pr_debug("resource[1] is not IORESOURCE_IRQ");
-		return -ENOMEM;
-	}
-	hcd = usb_create_hcd(&vt8500_ehci_hc_driver, &pdev->dev, "VT8500");
-	if (!hcd)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	hcd->rsrc_start = res->start;
-	hcd->rsrc_len = resource_size(res);
-
-	hcd->regs = devm_request_and_ioremap(&pdev->dev, res);
-	if (!hcd->regs) {
-		pr_debug("ioremap failed");
-		ret = -ENOMEM;
-		goto err1;
-	}
-
-	ehci = hcd_to_ehci(hcd);
-	ehci->caps = hcd->regs;
-
-	ret = usb_add_hcd(hcd, pdev->resource[1].start,
-			  IRQF_SHARED);
-	if (ret == 0) {
-		platform_set_drvdata(pdev, hcd);
-		return ret;
-	}
-
-err1:
-	usb_put_hcd(hcd);
-	return ret;
-}
-
-static int vt8500_ehci_drv_remove(struct platform_device *pdev)
-{
-	struct usb_hcd *hcd = platform_get_drvdata(pdev);
-
-	usb_remove_hcd(hcd);
-	usb_put_hcd(hcd);
-	platform_set_drvdata(pdev, NULL);
-
-	return 0;
-}
-
-static const struct of_device_id vt8500_ehci_ids[] = {
-	{ .compatible = "via,vt8500-ehci", },
-	{ .compatible = "wm,prizm-ehci", },
-	{}
-};
-
-static struct platform_driver vt8500_ehci_driver = {
-	.probe		= vt8500_ehci_drv_probe,
-	.remove		= vt8500_ehci_drv_remove,
-	.shutdown	= usb_hcd_platform_shutdown,
-	.driver = {
-		.name	= "vt8500-ehci",
-		.owner	= THIS_MODULE,
-		.of_match_table = of_match_ptr(vt8500_ehci_ids),
-	}
-};

[PATCH v2 1/2] USB: Update EHCI-platform driver to devicetree.

[Tony Prisk]

This patch adds devicetree support to the EHCI-platform driver,
and removes the now unneeded ehci-vt8500.c

Existing platform properties are maintained, with the exception
the power_(on/off) and suspend function pointers.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
---
 drivers/usb/host/ehci-hcd.c      |    5 --
 drivers/usb/host/ehci-platform.c |   61 +++++++++++++-
 drivers/usb/host/ehci-vt8500.c   |  171 --------------------------------------
 include/linux/usb/ehci_pdriver.h |    1 +
 4 files changed, 61 insertions(+), 177 deletions(-)
 delete mode 100644 drivers/usb/host/ehci-vt8500.c

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 6bf6c42..42c8e84 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1274,11 +1274,6 @@ MODULE_LICENSE ("GPL");
 #define PLATFORM_DRIVER		cns3xxx_ehci_driver
 #endif
 
-#ifdef CONFIG_ARCH_VT8500
-#include "ehci-vt8500.c"
-#define	PLATFORM_DRIVER		vt8500_ehci_driver
-#endif
-
 #ifdef CONFIG_PLAT_SPEAR
 #include "ehci-spear.c"
 #define PLATFORM_DRIVER		spear_ehci_hcd_driver
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index 764e010..f44fc6a 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -19,6 +19,7 @@
  * Licensed under the GNU/GPL. See COPYING for details.
  */
 #include <linux/platform_device.h>
+#include <linux/of.h>
 #include <linux/usb/ehci_pdriver.h>
 
 static int ehci_platform_reset(struct usb_hcd *hcd)
@@ -78,14 +79,60 @@ static const struct hc_driver ehci_platform_hc_driver = {
 	.clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
 };
 
+static u64 ehci_dma_mask = DMA_BIT_MASK(32);
+
 static int __devinit ehci_platform_probe(struct platform_device *dev)
 {
 	struct usb_hcd *hcd;
 	struct resource *res_mem;
 	struct usb_ehci_pdata *pdata = dev->dev.platform_data;
+	struct device_node *np = dev->dev.of_node;
 	int irq;
 	int err = -ENOMEM;
 
+	/* Are we being initialized from a DT-probed device? */
+	if (np) {
+		/*
+		 * No platform data is being passed, so initalize pdata.
+		 * Limitation: we can't support power_on, power_off or
+		 * power_suspend function pointers from DT.
+		 * TODO: The missing functions could be replaced with
+		 * power sequence handlers.
+		 */
+		pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
+		dev->dev.platform_data = pdata;
+
+		if (!pdata) {
+			pr_err("device tree platform data allocation failed\n");
+			return -ENOMEM;
+		}
+
+		/* Read the optional properties from DT node */
+		of_property_read_u32(np, "caps-offset", &pdata->caps_offset);
+		if (of_property_read_bool(np, "no_io_watchdog"))
+			pdata->no_io_watchdog = 1;
+		if (of_property_read_bool(np, "has-tt"))
+			pdata->has_tt = 1;
+		if (of_property_read_bool(np, "has-synopsys-hc-bug"))
+			pdata->has_synopsys_hc_bug = 1;
+
+		if (of_property_read_bool(np, "big-endian")) {
+			pdata->big_endian_desc = 1;
+			pdata->big_endian_mmio = 1;
+		} else {
+			if (of_property_read_bool(np, "big-endian-desc"))
+				pdata->big_endian_desc = 1;
+			if (of_property_read_bool(np, "big-endian-regs"))
+				pdata->big_endian_mmio = 1;
+		}
+		/* Right now device-tree probed devices don't get dma_mask set.
+		 * Since shared usb code relies on it, set it here for now.
+		 * Once we have dma capability bindings this can go away.
+		 */
+		if (!dev->dev.dma_mask)
+			dev->dev.dma_mask = &ehci_dma_mask;
+	}
+
 	if (!pdata) {
 		WARN_ON(1);
 		return -ENODEV;
@@ -101,7 +148,7 @@ static int __devinit ehci_platform_probe(struct platform_device *dev)
 	}
 	res_mem = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	if (!res_mem) {
-		pr_err("no memory recourse provided");
+		pr_err("no memory resource provided");
 		return -ENXIO;
 	}
 
@@ -163,6 +210,7 @@ static int __devexit ehci_platform_remove(struct platform_device *dev)
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
 	usb_put_hcd(hcd);
 	platform_set_drvdata(dev, NULL);
+	devm_kfree(&dev->dev, pdata);
 
 	if (pdata->power_off)
 		pdata->power_off(dev);
@@ -215,6 +263,16 @@ static const struct platform_device_id ehci_platform_table[] = {
 	{ "ehci-platform", 0 },
 	{ }
 };
+
+#ifdef CONFIG_OF
+static const struct of_device_id ehci_platform_ids[] = {
+	{ .compatible = "linux,ehci-platform", },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, ehci_platform_ids);
+#endif
+
 MODULE_DEVICE_TABLE(platform, ehci_platform_table);
 
 static const struct dev_pm_ops ehci_platform_pm_ops = {
@@ -231,5 +289,6 @@ static struct platform_driver ehci_platform_driver = {
 		.owner	= THIS_MODULE,
 		.name	= "ehci-platform",
 		.pm	= &ehci_platform_pm_ops,
+		.of_match_table = of_match_ptr(ehci_platform_ids),
 	}
 };
diff --git a/drivers/usb/host/ehci-vt8500.c b/drivers/usb/host/ehci-vt8500.c
deleted file mode 100644
index d3c9a3e..0000000
--- a/drivers/usb/host/ehci-vt8500.c
+++ /dev/null
@@ -1,171 +0,0 @@
-/*
- * drivers/usb/host/ehci-vt8500.c
- *
- * Copyright (C) 2010 Alexey Charkov <alchark@gmail.com>
- *
- * Based on ehci-au1xxx.c
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-#include <linux/of.h>
-#include <linux/platform_device.h>
-
-static int ehci_update_device(struct usb_hcd *hcd, struct usb_device *udev)
-{
-	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
-	int rc = 0;
-
-	if (!udev->parent) /* udev is root hub itself, impossible */
-		rc = -1;
-	/* we only support lpm device connected to root hub yet */
-	if (ehci->has_lpm && !udev->parent->parent) {
-		rc = ehci_lpm_set_da(ehci, udev->devnum, udev->portnum);
-		if (!rc)
-			rc = ehci_lpm_check(ehci, udev->portnum);
-	}
-	return rc;
-}
-
-static const struct hc_driver vt8500_ehci_hc_driver = {
-	.description		= hcd_name,
-	.product_desc		= "VT8500 EHCI",
-	.hcd_priv_size		= sizeof(struct ehci_hcd),
-
-	/*
-	 * generic hardware linkage
-	 */
-	.irq			= ehci_irq,
-	.flags			= HCD_MEMORY | HCD_USB2,
-
-	/*
-	 * basic lifecycle operations
-	 */
-	.reset			= ehci_setup,
-	.start			= ehci_run,
-	.stop			= ehci_stop,
-	.shutdown		= ehci_shutdown,
-
-	/*
-	 * managing i/o requests and associated device resources
-	 */
-	.urb_enqueue		= ehci_urb_enqueue,
-	.urb_dequeue		= ehci_urb_dequeue,
-	.endpoint_disable	= ehci_endpoint_disable,
-	.endpoint_reset		= ehci_endpoint_reset,
-
-	/*
-	 * scheduling support
-	 */
-	.get_frame_number	= ehci_get_frame,
-
-	/*
-	 * root hub support
-	 */
-	.hub_status_data	= ehci_hub_status_data,
-	.hub_control		= ehci_hub_control,
-	.bus_suspend		= ehci_bus_suspend,
-	.bus_resume		= ehci_bus_resume,
-	.relinquish_port	= ehci_relinquish_port,
-	.port_handed_over	= ehci_port_handed_over,
-
-	/*
-	 * call back when device connected and addressed
-	 */
-	.update_device =	ehci_update_device,
-
-	.clear_tt_buffer_complete	= ehci_clear_tt_buffer_complete,
-};
-
-static u64 vt8500_ehci_dma_mask = DMA_BIT_MASK(32);
-
-static int vt8500_ehci_drv_probe(struct platform_device *pdev)
-{
-	struct usb_hcd *hcd;
-	struct ehci_hcd *ehci;
-	struct resource *res;
-	int ret;
-
-	if (usb_disabled())
-		return -ENODEV;
-
-	/*
-	 * Right now device-tree probed devices don't get dma_mask set.
-	 * Since shared usb code relies on it, set it here for now.
-	 * Once we have dma capability bindings this can go away.
-	 */
-	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &vt8500_ehci_dma_mask;
-
-	if (pdev->resource[1].flags != IORESOURCE_IRQ) {
-		pr_debug("resource[1] is not IORESOURCE_IRQ");
-		return -ENOMEM;
-	}
-	hcd = usb_create_hcd(&vt8500_ehci_hc_driver, &pdev->dev, "VT8500");
-	if (!hcd)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	hcd->rsrc_start = res->start;
-	hcd->rsrc_len = resource_size(res);
-
-	hcd->regs = devm_request_and_ioremap(&pdev->dev, res);
-	if (!hcd->regs) {
-		pr_debug("ioremap failed");
-		ret = -ENOMEM;
-		goto err1;
-	}
-
-	ehci = hcd_to_ehci(hcd);
-	ehci->caps = hcd->regs;
-
-	ret = usb_add_hcd(hcd, pdev->resource[1].start,
-			  IRQF_SHARED);
-	if (ret == 0) {
-		platform_set_drvdata(pdev, hcd);
-		return ret;
-	}
-
-err1:
-	usb_put_hcd(hcd);
-	return ret;
-}
-
-static int vt8500_ehci_drv_remove(struct platform_device *pdev)
-{
-	struct usb_hcd *hcd = platform_get_drvdata(pdev);
-
-	usb_remove_hcd(hcd);
-	usb_put_hcd(hcd);
-	platform_set_drvdata(pdev, NULL);
-
-	return 0;
-}
-
-static const struct of_device_id vt8500_ehci_ids[] = {
-	{ .compatible = "via,vt8500-ehci", },
-	{ .compatible = "wm,prizm-ehci", },
-	{}
-};
-
-static struct platform_driver vt8500_ehci_driver = {
-	.probe		= vt8500_ehci_drv_probe,
-	.remove		= vt8500_ehci_drv_remove,
-	.shutdown	= usb_hcd_platform_shutdown,
-	.driver = {
-		.name	= "vt8500-ehci",
-		.owner	= THIS_MODULE,
-		.of_match_table = of_match_ptr(vt8500_ehci_ids),
-	}
-};
-
-MODULE_ALIAS("platform:vt8500-ehci");
-MODULE_DEVICE_TABLE(of, vt8500_ehci_ids);
diff --git a/include/linux/usb/ehci_pdriver.h b/include/linux/usb/ehci_pdriver.h
index c9d09f8..e9f74fa 100644
--- a/include/linux/usb/ehci_pdriver.h
+++ b/include/linux/usb/ehci_pdriver.h
@@ -41,6 +41,7 @@ struct usb_ehci_pdata {
 	unsigned	big_endian_mmio:1;
 	unsigned	port_power_on:1;
 	unsigned	port_power_off:1;
+	unsigned	no_io_watchdog:1;
 
 	/* Turn on all power and clocks */
 	int (*power_on)(struct platform_device *pdev);
-- 
1.7.9.5

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Tony Prisk]

Add a binding document for ehci-platform driver.

Signed-off-by: Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>
---
 .../devicetree/bindings/usb/ehci-platform.txt      |   27 ++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt

diff --git a/Documentation/devicetree/bindings/usb/ehci-platform.txt b/Documentation/devicetree/bindings/usb/ehci-platform.txt
new file mode 100644
index 0000000..930b19e
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/ehci-platform.txt
@@ -0,0 +1,27 @@
+Generic Platform EHCI Controller
+-----------------------------------------------------
+
+Required properties:
+- compatible : "linux,ehci-platform"
+- reg : Should contain 1 register ranges(address and length)
+- interrupts : EHCI controller interrupt
+
+Optional properties:
+- caps-offset : offset to the capabilities register (default = 0)
+- has-tt : controller has transaction translator(s).
+- has-synopsys-hc-bug : controller has the synopsys hc bug
+- no-io-watchdog : controller does not need io watchdog
+
+- big-endian : descriptors and registers are both big endian. This
+  is the equivalent of specifying big-endian-desc and big-endian-regs.
+OR
+- big-endian-desc : descriptors are in big-endian format
+- big-endian-regs : mmio is in big-endian format
+
+Example:
+	ehci@d8007c00 {
+		compatible = "ehci-platform";
+		reg = <0xd8007c00 0x200>;
+		interrupts = <43>;
+		has-tt;
+	};
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Tony Prisk]

Add a binding document for ehci-platform driver.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
---
 .../devicetree/bindings/usb/ehci-platform.txt      |   27 ++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt

diff --git a/Documentation/devicetree/bindings/usb/ehci-platform.txt b/Documentation/devicetree/bindings/usb/ehci-platform.txt
new file mode 100644
index 0000000..930b19e
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/ehci-platform.txt
@@ -0,0 +1,27 @@
+Generic Platform EHCI Controller
+-----------------------------------------------------
+
+Required properties:
+- compatible : "linux,ehci-platform"
+- reg : Should contain 1 register ranges(address and length)
+- interrupts : EHCI controller interrupt
+
+Optional properties:
+- caps-offset : offset to the capabilities register (default = 0)
+- has-tt : controller has transaction translator(s).
+- has-synopsys-hc-bug : controller has the synopsys hc bug
+- no-io-watchdog : controller does not need io watchdog
+
+- big-endian : descriptors and registers are both big endian. This
+  is the equivalent of specifying big-endian-desc and big-endian-regs.
+OR
+- big-endian-desc : descriptors are in big-endian format
+- big-endian-regs : mmio is in big-endian format
+
+Example:
+	ehci at d8007c00 {
+		compatible = "ehci-platform";
+		reg = <0xd8007c00 0x200>;
+		interrupts = <43>;
+		has-tt;
+	};
-- 
1.7.9.5

Re: [PATCH v2 1/2] USB: Update EHCI-platform driver to devicetree.

[Alan Stern]

On Sun, 21 Oct 2012, Tony Prisk wrote:

> This patch adds devicetree support to the EHCI-platform driver,
> and removes the now unneeded ehci-vt8500.c
> 
> Existing platform properties are maintained, with the exception
> the power_(on/off) and suspend function pointers.

Unfortunately this can't be accepted as is, because Florian's patches 
are ahead of yours in the queue.  You'll have to respin this after his 
stuff has been merged.

> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -19,6 +19,7 @@
>   * Licensed under the GNU/GPL. See COPYING for details.
>   */
>  #include <linux/platform_device.h>
> +#include <linux/of.h>
>  #include <linux/usb/ehci_pdriver.h>
>  
>  static int ehci_platform_reset(struct usb_hcd *hcd)
> @@ -78,14 +79,60 @@ static const struct hc_driver ehci_platform_hc_driver = {
>  	.clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
>  };
>  
> +static u64 ehci_dma_mask = DMA_BIT_MASK(32);

Just out of curiosity...  Instead of adding this same code (which will
all have to be removed later) to millions of drivers, why doesn't
somebody add support for DMA masks into DT?

> +		 * No platform data is being passed, so initalize pdata.
> +		 * Limitation: we can't support power_on, power_off or
> +		 * power_suspend function pointers from DT.
> +		 * TODO: The missing functions could be replaced with
> +		 * power sequence handlers.
> +		 */
> +		pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
> +		dev->dev.platform_data = pdata;
> +
> +		if (!pdata) {
> +			pr_err("device tree platform data allocation failed\n");

Don't use pr_err; use dev_err.

> @@ -101,7 +148,7 @@ static int __devinit ehci_platform_probe(struct platform_device *dev)
>  	}
>  	res_mem = platform_get_resource(dev, IORESOURCE_MEM, 0);
>  	if (!res_mem) {
> -		pr_err("no memory recourse provided");
> +		pr_err("no memory resource provided");

This has already been fixed in Florian's patches.

>  
> @@ -163,6 +210,7 @@ static int __devexit ehci_platform_remove(struct platform_device *dev)
>  	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
>  	usb_put_hcd(hcd);
>  	platform_set_drvdata(dev, NULL);
> +	devm_kfree(&dev->dev, pdata);

This isn't needed.  That's the whole point of the devm_* family of 
routines; the resources they allocate are automatically released when 
the device is removed.

> --- a/include/linux/usb/ehci_pdriver.h
> +++ b/include/linux/usb/ehci_pdriver.h
> @@ -41,6 +41,7 @@ struct usb_ehci_pdata {
>  	unsigned	big_endian_mmio:1;
>  	unsigned	port_power_on:1;
>  	unsigned	port_power_off:1;
> +	unsigned	no_io_watchdog:1;

This is also in Florian's patches.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] USB: Update EHCI-platform driver to devicetree.

[Alan Stern]

On Sun, 21 Oct 2012, Tony Prisk wrote:

> This patch adds devicetree support to the EHCI-platform driver,
> and removes the now unneeded ehci-vt8500.c
> 
> Existing platform properties are maintained, with the exception
> the power_(on/off) and suspend function pointers.

Unfortunately this can't be accepted as is, because Florian's patches 
are ahead of yours in the queue.  You'll have to respin this after his 
stuff has been merged.

> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -19,6 +19,7 @@
>   * Licensed under the GNU/GPL. See COPYING for details.
>   */
>  #include <linux/platform_device.h>
> +#include <linux/of.h>
>  #include <linux/usb/ehci_pdriver.h>
>  
>  static int ehci_platform_reset(struct usb_hcd *hcd)
> @@ -78,14 +79,60 @@ static const struct hc_driver ehci_platform_hc_driver = {
>  	.clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
>  };
>  
> +static u64 ehci_dma_mask = DMA_BIT_MASK(32);

Just out of curiosity...  Instead of adding this same code (which will
all have to be removed later) to millions of drivers, why doesn't
somebody add support for DMA masks into DT?

> +		 * No platform data is being passed, so initalize pdata.
> +		 * Limitation: we can't support power_on, power_off or
> +		 * power_suspend function pointers from DT.
> +		 * TODO: The missing functions could be replaced with
> +		 * power sequence handlers.
> +		 */
> +		pdata = devm_kzalloc(&dev->dev, sizeof(*pdata), GFP_KERNEL);
> +		dev->dev.platform_data = pdata;
> +
> +		if (!pdata) {
> +			pr_err("device tree platform data allocation failed\n");

Don't use pr_err; use dev_err.

> @@ -101,7 +148,7 @@ static int __devinit ehci_platform_probe(struct platform_device *dev)
>  	}
>  	res_mem = platform_get_resource(dev, IORESOURCE_MEM, 0);
>  	if (!res_mem) {
> -		pr_err("no memory recourse provided");
> +		pr_err("no memory resource provided");

This has already been fixed in Florian's patches.

>  
> @@ -163,6 +210,7 @@ static int __devexit ehci_platform_remove(struct platform_device *dev)
>  	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
>  	usb_put_hcd(hcd);
>  	platform_set_drvdata(dev, NULL);
> +	devm_kfree(&dev->dev, pdata);

This isn't needed.  That's the whole point of the devm_* family of 
routines; the resources they allocate are automatically released when 
the device is removed.

> --- a/include/linux/usb/ehci_pdriver.h
> +++ b/include/linux/usb/ehci_pdriver.h
> @@ -41,6 +41,7 @@ struct usb_ehci_pdata {
>  	unsigned	big_endian_mmio:1;
>  	unsigned	port_power_on:1;
>  	unsigned	port_power_off:1;
> +	unsigned	no_io_watchdog:1;

This is also in Florian's patches.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Florian Fainelli]

Le dimanche 21 octobre 2012 00:10:32, Tony Prisk a écrit :
> Add a binding document for ehci-platform driver.
> 
> Signed-off-by: Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>
> ---
>  .../devicetree/bindings/usb/ehci-platform.txt      |   27
> ++++++++++++++++++++ 1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/ehci-platform.txt
> b/Documentation/devicetree/bindings/usb/ehci-platform.txt new file mode
> 100644
> index 0000000..930b19e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ehci-platform.txt
> @@ -0,0 +1,27 @@
> +Generic Platform EHCI Controller
> +-----------------------------------------------------
> +
> +Required properties:
> +- compatible : "linux,ehci-platform"

Here you changed to linux,ehci-platform, which is good, but in the example you 
forgot to update it.

Otherwise, this is ok with me.

> +- reg : Should contain 1 register ranges(address and length)
> +- interrupts : EHCI controller interrupt
> +
> +Optional properties:
> +- caps-offset : offset to the capabilities register (default = 0)
> +- has-tt : controller has transaction translator(s).
> +- has-synopsys-hc-bug : controller has the synopsys hc bug
> +- no-io-watchdog : controller does not need io watchdog
> +
> +- big-endian : descriptors and registers are both big endian. This
> +  is the equivalent of specifying big-endian-desc and big-endian-regs.
> +OR
> +- big-endian-desc : descriptors are in big-endian format
> +- big-endian-regs : mmio is in big-endian format
> +
> +Example:
> +	ehci@d8007c00 {
> +		compatible = "ehci-platform";
> +		reg = <0xd8007c00 0x200>;
> +		interrupts = <43>;
> +		has-tt;
> +	};

-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Florian Fainelli]

Le dimanche 21 octobre 2012 00:10:32, Tony Prisk a ?crit :
> Add a binding document for ehci-platform driver.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> ---
>  .../devicetree/bindings/usb/ehci-platform.txt      |   27
> ++++++++++++++++++++ 1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/ehci-platform.txt
> b/Documentation/devicetree/bindings/usb/ehci-platform.txt new file mode
> 100644
> index 0000000..930b19e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ehci-platform.txt
> @@ -0,0 +1,27 @@
> +Generic Platform EHCI Controller
> +-----------------------------------------------------
> +
> +Required properties:
> +- compatible : "linux,ehci-platform"

Here you changed to linux,ehci-platform, which is good, but in the example you 
forgot to update it.

Otherwise, this is ok with me.

> +- reg : Should contain 1 register ranges(address and length)
> +- interrupts : EHCI controller interrupt
> +
> +Optional properties:
> +- caps-offset : offset to the capabilities register (default = 0)
> +- has-tt : controller has transaction translator(s).
> +- has-synopsys-hc-bug : controller has the synopsys hc bug
> +- no-io-watchdog : controller does not need io watchdog
> +
> +- big-endian : descriptors and registers are both big endian. This
> +  is the equivalent of specifying big-endian-desc and big-endian-regs.
> +OR
> +- big-endian-desc : descriptors are in big-endian format
> +- big-endian-regs : mmio is in big-endian format
> +
> +Example:
> +	ehci at d8007c00 {
> +		compatible = "ehci-platform";
> +		reg = <0xd8007c00 0x200>;
> +		interrupts = <43>;
> +		has-tt;
> +	};

-- 
Florian

Re: [PATCH v2 1/2] USB: Update EHCI-platform driver to devicetree.

[Alan Stern]

On Sun, 21 Oct 2012, Tony Prisk wrote:

> +static u64 ehci_dma_mask = DMA_BIT_MASK(32);

...

> +		/* Right now device-tree probed devices don't get dma_mask set.
> +		 * Since shared usb code relies on it, set it here for now.
> +		 * Once we have dma capability bindings this can go away.
> +		 */
> +		if (!dev->dev.dma_mask)
> +			dev->dev.dma_mask = &ehci_dma_mask;

Grant:

I have seen this sort of thing added to many drivers.  Why can't this 
be centralized in a way that will apply once and for all to all 
appropriate DT-based drivers?

Eventually DMA capabilities will be supported properly in DT, right?  
Then all these additions made to hundreds(?) of drivers will have to be
removed.  Why not start the ball rolling now and prevent things from
getting even worse?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/20/2012 04:10 PM, Tony Prisk wrote:
> Add a binding document for ehci-platform driver.
> 
> Signed-off-by: Tony Prisk <linux-ci5G2KO2hbZ+pU9mqzGVBQ@public.gmane.org>
> ---
>  .../devicetree/bindings/usb/ehci-platform.txt      |   27 ++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/ehci-platform.txt b/Documentation/devicetree/bindings/usb/ehci-platform.txt
> new file mode 100644
> index 0000000..930b19e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ehci-platform.txt
> @@ -0,0 +1,27 @@
> +Generic Platform EHCI Controller
> +-----------------------------------------------------
> +
> +Required properties:
> +- compatible : "linux,ehci-platform"

That compatible value doesn't look right. The HW isn't defined by Linux.
The binding is supposed to represent HW, not any single OS's use of that
HW or the way its driver works.

Something like "usb,ehci" might be more appropriate. Certainly, the
value should not be "linux,", nor derived from Linux's driver name.

> +Optional properties:
> +- caps-offset : offset to the capabilities register (default = 0)
> +- has-tt : controller has transaction translator(s).
> +- has-synopsys-hc-bug : controller has the synopsys hc bug

That would normally be determined by the driver based on the particular
compatible value that is in device tree.

> +- no-io-watchdog : controller does not need io watchdog
> +
> +- big-endian : descriptors and registers are both big endian. This
> +  is the equivalent of specifying big-endian-desc and big-endian-regs.
> +OR
> +- big-endian-desc : descriptors are in big-endian format
> +- big-endian-regs : mmio is in big-endian format

Hmmm. That looks odd. Presumably if those properties aren't specified,
the default is little-endian? Shouldn't this be a tri-state: big,
little, native, with default native? I don't know what the EHCI
specification mandates here (and if it does mandate something, the
default should match the specification). Isn't this something that
readl/writel would take care of, or are there cases where the register
endianness of just this one HW block mismatches all other HW blocks?

> +Example:
> +	ehci@d8007c00 {
> +		compatible = "ehci-platform";
> +		reg = <0xd8007c00 0x200>;
> +		interrupts = <43>;
> +		has-tt;
> +	};
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/20/2012 04:10 PM, Tony Prisk wrote:
> Add a binding document for ehci-platform driver.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> ---
>  .../devicetree/bindings/usb/ehci-platform.txt      |   27 ++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/ehci-platform.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/ehci-platform.txt b/Documentation/devicetree/bindings/usb/ehci-platform.txt
> new file mode 100644
> index 0000000..930b19e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ehci-platform.txt
> @@ -0,0 +1,27 @@
> +Generic Platform EHCI Controller
> +-----------------------------------------------------
> +
> +Required properties:
> +- compatible : "linux,ehci-platform"

That compatible value doesn't look right. The HW isn't defined by Linux.
The binding is supposed to represent HW, not any single OS's use of that
HW or the way its driver works.

Something like "usb,ehci" might be more appropriate. Certainly, the
value should not be "linux,", nor derived from Linux's driver name.

> +Optional properties:
> +- caps-offset : offset to the capabilities register (default = 0)
> +- has-tt : controller has transaction translator(s).
> +- has-synopsys-hc-bug : controller has the synopsys hc bug

That would normally be determined by the driver based on the particular
compatible value that is in device tree.

> +- no-io-watchdog : controller does not need io watchdog
> +
> +- big-endian : descriptors and registers are both big endian. This
> +  is the equivalent of specifying big-endian-desc and big-endian-regs.
> +OR
> +- big-endian-desc : descriptors are in big-endian format
> +- big-endian-regs : mmio is in big-endian format

Hmmm. That looks odd. Presumably if those properties aren't specified,
the default is little-endian? Shouldn't this be a tri-state: big,
little, native, with default native? I don't know what the EHCI
specification mandates here (and if it does mandate something, the
default should match the specification). Isn't this something that
readl/writel would take care of, or are there cases where the register
endianness of just this one HW block mismatches all other HW blocks?

> +Example:
> +	ehci at d8007c00 {
> +		compatible = "ehci-platform";
> +		reg = <0xd8007c00 0x200>;
> +		interrupts = <43>;
> +		has-tt;
> +	};
> 

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Mon, 22 Oct 2012, Stephen Warren wrote:

> On 10/20/2012 04:10 PM, Tony Prisk wrote:
> > Add a binding document for ehci-platform driver.

> > +Optional properties:
> > +- caps-offset : offset to the capabilities register (default = 0)
> > +- has-tt : controller has transaction translator(s).
> > +- has-synopsys-hc-bug : controller has the synopsys hc bug
> 
> That would normally be determined by the driver based on the particular
> compatible value that is in device tree.

I don't understand this comment.  Isn't "has-synopsys-hc-bug" the 
compatible value in question?

> > +- big-endian : descriptors and registers are both big endian. This
> > +  is the equivalent of specifying big-endian-desc and big-endian-regs.
> > +OR
> > +- big-endian-desc : descriptors are in big-endian format
> > +- big-endian-regs : mmio is in big-endian format
> 
> Hmmm. That looks odd. Presumably if those properties aren't specified,
> the default is little-endian? Shouldn't this be a tri-state: big,
> little, native, with default native? I don't know what the EHCI
> specification mandates here (and if it does mandate something, the
> default should match the specification). Isn't this something that
> readl/writel would take care of, or are there cases where the register
> endianness of just this one HW block mismatches all other HW blocks?

The EHCI spec assumes a PCI implementation; it doesn't consider other
sorts.  And it doesn't say anything about the endianness of multi-byte
descriptors in memory.

Yes, there are cases where one HW block has an endianness that doesn't
match other HW blocks.  Or to be more accurate, it doesn't match what
the other HW blocks expect.  For example, on ARM readl and writel
expect to do byte-swapping but some particular EHCI blocks don't need
it.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Mon, 22 Oct 2012, Stephen Warren wrote:

> On 10/20/2012 04:10 PM, Tony Prisk wrote:
> > Add a binding document for ehci-platform driver.

> > +Optional properties:
> > +- caps-offset : offset to the capabilities register (default = 0)
> > +- has-tt : controller has transaction translator(s).
> > +- has-synopsys-hc-bug : controller has the synopsys hc bug
> 
> That would normally be determined by the driver based on the particular
> compatible value that is in device tree.

I don't understand this comment.  Isn't "has-synopsys-hc-bug" the 
compatible value in question?

> > +- big-endian : descriptors and registers are both big endian. This
> > +  is the equivalent of specifying big-endian-desc and big-endian-regs.
> > +OR
> > +- big-endian-desc : descriptors are in big-endian format
> > +- big-endian-regs : mmio is in big-endian format
> 
> Hmmm. That looks odd. Presumably if those properties aren't specified,
> the default is little-endian? Shouldn't this be a tri-state: big,
> little, native, with default native? I don't know what the EHCI
> specification mandates here (and if it does mandate something, the
> default should match the specification). Isn't this something that
> readl/writel would take care of, or are there cases where the register
> endianness of just this one HW block mismatches all other HW blocks?

The EHCI spec assumes a PCI implementation; it doesn't consider other
sorts.  And it doesn't say anything about the endianness of multi-byte
descriptors in memory.

Yes, there are cases where one HW block has an endianness that doesn't
match other HW blocks.  Or to be more accurate, it doesn't match what
the other HW blocks expect.  For example, on ARM readl and writel
expect to do byte-swapping but some particular EHCI blocks don't need
it.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/22/2012 11:34 AM, Alan Stern wrote:
> On Mon, 22 Oct 2012, Stephen Warren wrote:
> 
>> On 10/20/2012 04:10 PM, Tony Prisk wrote:
>>> Add a binding document for ehci-platform driver.
> 
>>> +Optional properties:
>>> +- caps-offset : offset to the capabilities register (default = 0)
>>> +- has-tt : controller has transaction translator(s).
>>> +- has-synopsys-hc-bug : controller has the synopsys hc bug
>>
>> That would normally be determined by the driver based on the particular
>> compatible value that is in device tree.
> 
> I don't understand this comment.  Isn't "has-synopsys-hc-bug" the 
> compatible value in question?

"compatible value" in this context means that value of the property
named "compatible".

>>> +- big-endian : descriptors and registers are both big endian. This
>>> +  is the equivalent of specifying big-endian-desc and big-endian-regs.
>>> +OR
>>> +- big-endian-desc : descriptors are in big-endian format
>>> +- big-endian-regs : mmio is in big-endian format
>>
>> Hmmm. That looks odd. Presumably if those properties aren't specified,
>> the default is little-endian? Shouldn't this be a tri-state: big,
>> little, native, with default native? I don't know what the EHCI
>> specification mandates here (and if it does mandate something, the
>> default should match the specification). Isn't this something that
>> readl/writel would take care of, or are there cases where the register
>> endianness of just this one HW block mismatches all other HW blocks?
> 
> The EHCI spec assumes a PCI implementation; it doesn't consider other
> sorts.  And it doesn't say anything about the endianness of multi-byte
> descriptors in memory.

OK, so does this binding default to assuming little-endian (which I
assume matches PCI), unless the big-endian properties are given? Is the
case of little-endian EHCI registers on a big-endian CPU a common enough
thing that adding a third state native-endian wouldn't be useful?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/22/2012 11:34 AM, Alan Stern wrote:
> On Mon, 22 Oct 2012, Stephen Warren wrote:
> 
>> On 10/20/2012 04:10 PM, Tony Prisk wrote:
>>> Add a binding document for ehci-platform driver.
> 
>>> +Optional properties:
>>> +- caps-offset : offset to the capabilities register (default = 0)
>>> +- has-tt : controller has transaction translator(s).
>>> +- has-synopsys-hc-bug : controller has the synopsys hc bug
>>
>> That would normally be determined by the driver based on the particular
>> compatible value that is in device tree.
> 
> I don't understand this comment.  Isn't "has-synopsys-hc-bug" the 
> compatible value in question?

"compatible value" in this context means that value of the property
named "compatible".

>>> +- big-endian : descriptors and registers are both big endian. This
>>> +  is the equivalent of specifying big-endian-desc and big-endian-regs.
>>> +OR
>>> +- big-endian-desc : descriptors are in big-endian format
>>> +- big-endian-regs : mmio is in big-endian format
>>
>> Hmmm. That looks odd. Presumably if those properties aren't specified,
>> the default is little-endian? Shouldn't this be a tri-state: big,
>> little, native, with default native? I don't know what the EHCI
>> specification mandates here (and if it does mandate something, the
>> default should match the specification). Isn't this something that
>> readl/writel would take care of, or are there cases where the register
>> endianness of just this one HW block mismatches all other HW blocks?
> 
> The EHCI spec assumes a PCI implementation; it doesn't consider other
> sorts.  And it doesn't say anything about the endianness of multi-byte
> descriptors in memory.

OK, so does this binding default to assuming little-endian (which I
assume matches PCI), unless the big-endian properties are given? Is the
case of little-endian EHCI registers on a big-endian CPU a common enough
thing that adding a third state native-endian wouldn't be useful?

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Mon, 22 Oct 2012, Stephen Warren wrote:

> >>> +- has-tt : controller has transaction translator(s).
> >>> +- has-synopsys-hc-bug : controller has the synopsys hc bug
> >>
> >> That would normally be determined by the driver based on the particular
> >> compatible value that is in device tree.
> > 
> > I don't understand this comment.  Isn't "has-synopsys-hc-bug" the 
> > compatible value in question?
> 
> "compatible value" in this context means that value of the property
> named "compatible".

I see.  But why would it be done this way instead having a separate 
property?

And doesn't the same reasoning apply to has-tt?  Doesn't that mean the 
driver would have to match four different hardware types?  What happens 
if a third characteristic like these comes around; would the driver 
then have to check against eight different types?

> >>> +- big-endian : descriptors and registers are both big endian. This
> >>> +  is the equivalent of specifying big-endian-desc and big-endian-regs.
> >>> +OR
> >>> +- big-endian-desc : descriptors are in big-endian format
> >>> +- big-endian-regs : mmio is in big-endian format
> >>
> >> Hmmm. That looks odd. Presumably if those properties aren't specified,
> >> the default is little-endian? Shouldn't this be a tri-state: big,
> >> little, native, with default native? I don't know what the EHCI
> >> specification mandates here (and if it does mandate something, the
> >> default should match the specification). Isn't this something that
> >> readl/writel would take care of, or are there cases where the register
> >> endianness of just this one HW block mismatches all other HW blocks?
> > 
> > The EHCI spec assumes a PCI implementation; it doesn't consider other
> > sorts.  And it doesn't say anything about the endianness of multi-byte
> > descriptors in memory.
> 
> OK, so does this binding default to assuming little-endian (which I
> assume matches PCI), unless the big-endian properties are given?

Yes, that is the intention.  The ehci-hcd driver works the same way; it 
assumes little-endian unless USB_EHCI_BIG_ENDIAN_DESC or 
USB_EHCI_BIG_ENDIAN_MMIO is set.  (That will have to change in the 
future, though.)

>  Is the
> case of little-endian EHCI registers on a big-endian CPU a common enough
> thing that adding a third state native-endian wouldn't be useful?

I'm not sure how to answer.  Little-endian EHCI registers are very
common, even among big-endian CPUs (they are probably the majority, in
fact).  I don't see how adding native-endian would help; the readl() 
function always assumes the values it reads are little-endian, so in 
that sense little-endian _is_ the native state.

Also, as far as I know there aren't any examples of big-endian EHCI
registers on systems with little-endian CPUs.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Mon, 22 Oct 2012, Stephen Warren wrote:

> >>> +- has-tt : controller has transaction translator(s).
> >>> +- has-synopsys-hc-bug : controller has the synopsys hc bug
> >>
> >> That would normally be determined by the driver based on the particular
> >> compatible value that is in device tree.
> > 
> > I don't understand this comment.  Isn't "has-synopsys-hc-bug" the 
> > compatible value in question?
> 
> "compatible value" in this context means that value of the property
> named "compatible".

I see.  But why would it be done this way instead having a separate 
property?

And doesn't the same reasoning apply to has-tt?  Doesn't that mean the 
driver would have to match four different hardware types?  What happens 
if a third characteristic like these comes around; would the driver 
then have to check against eight different types?

> >>> +- big-endian : descriptors and registers are both big endian. This
> >>> +  is the equivalent of specifying big-endian-desc and big-endian-regs.
> >>> +OR
> >>> +- big-endian-desc : descriptors are in big-endian format
> >>> +- big-endian-regs : mmio is in big-endian format
> >>
> >> Hmmm. That looks odd. Presumably if those properties aren't specified,
> >> the default is little-endian? Shouldn't this be a tri-state: big,
> >> little, native, with default native? I don't know what the EHCI
> >> specification mandates here (and if it does mandate something, the
> >> default should match the specification). Isn't this something that
> >> readl/writel would take care of, or are there cases where the register
> >> endianness of just this one HW block mismatches all other HW blocks?
> > 
> > The EHCI spec assumes a PCI implementation; it doesn't consider other
> > sorts.  And it doesn't say anything about the endianness of multi-byte
> > descriptors in memory.
> 
> OK, so does this binding default to assuming little-endian (which I
> assume matches PCI), unless the big-endian properties are given?

Yes, that is the intention.  The ehci-hcd driver works the same way; it 
assumes little-endian unless USB_EHCI_BIG_ENDIAN_DESC or 
USB_EHCI_BIG_ENDIAN_MMIO is set.  (That will have to change in the 
future, though.)

>  Is the
> case of little-endian EHCI registers on a big-endian CPU a common enough
> thing that adding a third state native-endian wouldn't be useful?

I'm not sure how to answer.  Little-endian EHCI registers are very
common, even among big-endian CPUs (they are probably the majority, in
fact).  I don't see how adding native-endian would help; the readl() 
function always assumes the values it reads are little-endian, so in 
that sense little-endian _is_ the native state.

Also, as far as I know there aren't any examples of big-endian EHCI
registers on systems with little-endian CPUs.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/22/2012 01:00 PM, Alan Stern wrote:
> On Mon, 22 Oct 2012, Stephen Warren wrote:
> 
>>>>> +- has-tt : controller has transaction translator(s).
>>>>> +- has-synopsys-hc-bug : controller has the synopsys hc bug
>>>>
>>>> That would normally be determined by the driver based on the particular
>>>> compatible value that is in device tree.
>>>
>>> I don't understand this comment.  Isn't "has-synopsys-hc-bug" the 
>>> compatible value in question?
>>
>> "compatible value" in this context means that value of the property
>> named "compatible".
> 
> I see.  But why would it be done this way instead having a separate 
> property?

Well, I did say normally:-)

I can certainly see an argument for representing these differences using
custom properties, rather than deriving the information from the
compatible value. It's probably be OK to do so for something generic
like this; it's just perhaps not always the default choice.

Do note that even though this binding document dictates a particular
value for the compatible property, every device tree should additionally
add a separate value alongside it to indicate the specific HW model
that's actually present, so that if some device-specific bug-fix or
workaround needs to be applied, the model can be identified anyway.

So, rather than:

compatible = "usb-ehci";

You should always have e.g.:

compatible = "nvidia,tegra20-ehci", "usb-ehci";

Given that, there is then always enough information in the device tree
for the driver to be able to derive the other values from the compatible
value.

Whether you want to derive the information, or whether you want to
explicitly represent it via properties, is a decision to make based on
the trade-offs.

Oh, and I note that quite a few device trees already use compatible
value "usb-ehci" in their device-trees. Care needs to be taken not to
usurp that value from any existing device drivers if that was to be
picked as the compatible value required by this binding.

> And doesn't the same reasoning apply to has-tt?  Doesn't that mean the 
> driver would have to match four different hardware types?  What happens 
> if a third characteristic like these comes around; would the driver 
> then have to check against eight different types?

No, the compatible value represents the model, so you'd have a table like:

compatible          -> bugX has_tt
nvidia,tegra20-ehci -> 0    1
vendor1,foo-ehci    -> 0    1
vendor2,bar-ehci    -> 1    1
vendor3,baz-ehci    -> 0    1
vendor4,qux-ehci    -> 0    1
...

So the table size isn't related to the number of options. The table size
is probably bigger than subset of options combinations that make sense.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/22/2012 01:00 PM, Alan Stern wrote:
> On Mon, 22 Oct 2012, Stephen Warren wrote:
> 
>>>>> +- has-tt : controller has transaction translator(s).
>>>>> +- has-synopsys-hc-bug : controller has the synopsys hc bug
>>>>
>>>> That would normally be determined by the driver based on the particular
>>>> compatible value that is in device tree.
>>>
>>> I don't understand this comment.  Isn't "has-synopsys-hc-bug" the 
>>> compatible value in question?
>>
>> "compatible value" in this context means that value of the property
>> named "compatible".
> 
> I see.  But why would it be done this way instead having a separate 
> property?

Well, I did say normally:-)

I can certainly see an argument for representing these differences using
custom properties, rather than deriving the information from the
compatible value. It's probably be OK to do so for something generic
like this; it's just perhaps not always the default choice.

Do note that even though this binding document dictates a particular
value for the compatible property, every device tree should additionally
add a separate value alongside it to indicate the specific HW model
that's actually present, so that if some device-specific bug-fix or
workaround needs to be applied, the model can be identified anyway.

So, rather than:

compatible = "usb-ehci";

You should always have e.g.:

compatible = "nvidia,tegra20-ehci", "usb-ehci";

Given that, there is then always enough information in the device tree
for the driver to be able to derive the other values from the compatible
value.

Whether you want to derive the information, or whether you want to
explicitly represent it via properties, is a decision to make based on
the trade-offs.

Oh, and I note that quite a few device trees already use compatible
value "usb-ehci" in their device-trees. Care needs to be taken not to
usurp that value from any existing device drivers if that was to be
picked as the compatible value required by this binding.

> And doesn't the same reasoning apply to has-tt?  Doesn't that mean the 
> driver would have to match four different hardware types?  What happens 
> if a third characteristic like these comes around; would the driver 
> then have to check against eight different types?

No, the compatible value represents the model, so you'd have a table like:

compatible          -> bugX has_tt
nvidia,tegra20-ehci -> 0    1
vendor1,foo-ehci    -> 0    1
vendor2,bar-ehci    -> 1    1
vendor3,baz-ehci    -> 0    1
vendor4,qux-ehci    -> 0    1
...

So the table size isn't related to the number of options. The table size
is probably bigger than subset of options combinations that make sense.

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Mon, 22 Oct 2012, Stephen Warren wrote:

> > I see.  But why would it be done this way instead having a separate 
> > property?
> 
> Well, I did say normally:-)
> 
> I can certainly see an argument for representing these differences using
> custom properties, rather than deriving the information from the
> compatible value. It's probably be OK to do so for something generic
> like this; it's just perhaps not always the default choice.
> 
> Do note that even though this binding document dictates a particular
> value for the compatible property, every device tree should additionally
> add a separate value alongside it to indicate the specific HW model
> that's actually present, so that if some device-specific bug-fix or
> workaround needs to be applied, the model can be identified anyway.
> 
> So, rather than:
> 
> compatible = "usb-ehci";
> 
> You should always have e.g.:
> 
> compatible = "nvidia,tegra20-ehci", "usb-ehci";
> 
> Given that, there is then always enough information in the device tree
> for the driver to be able to derive the other values from the compatible
> value.

Yes, I get it.

> Whether you want to derive the information, or whether you want to
> explicitly represent it via properties, is a decision to make based on
> the trade-offs.
> 
> Oh, and I note that quite a few device trees already use compatible
> value "usb-ehci" in their device-trees. Care needs to be taken not to
> usurp that value from any existing device drivers if that was to be
> picked as the compatible value required by this binding.

Right.  I think Tony's new binding should use compatible value 
"usb-ehci-platform".  It will essentially be a superset of "usb-ehci".

> > And doesn't the same reasoning apply to has-tt?  Doesn't that mean the 
> > driver would have to match four different hardware types?  What happens 
> > if a third characteristic like these comes around; would the driver 
> > then have to check against eight different types?
> 
> No, the compatible value represents the model, so you'd have a table like:
> 
> compatible          -> bugX has_tt
> nvidia,tegra20-ehci -> 0    1
> vendor1,foo-ehci    -> 0    1
> vendor2,bar-ehci    -> 1    1
> vendor3,baz-ehci    -> 0    1
> vendor4,qux-ehci    -> 0    1
> ...
> 
> So the table size isn't related to the number of options. The table size
> is probably bigger than subset of options combinations that make sense.

Under the circumstances, and considering that usb-ehci-platform will be
rather generic, IMO it will be better to make has-tt,
has-synopsys-hc-bug, and no-io-watchdog separate properties, each
naturally defaulting to "not present".  That will avoid the need to 
update the driver's table each time a new board comes along.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Mon, 22 Oct 2012, Stephen Warren wrote:

> > I see.  But why would it be done this way instead having a separate 
> > property?
> 
> Well, I did say normally:-)
> 
> I can certainly see an argument for representing these differences using
> custom properties, rather than deriving the information from the
> compatible value. It's probably be OK to do so for something generic
> like this; it's just perhaps not always the default choice.
> 
> Do note that even though this binding document dictates a particular
> value for the compatible property, every device tree should additionally
> add a separate value alongside it to indicate the specific HW model
> that's actually present, so that if some device-specific bug-fix or
> workaround needs to be applied, the model can be identified anyway.
> 
> So, rather than:
> 
> compatible = "usb-ehci";
> 
> You should always have e.g.:
> 
> compatible = "nvidia,tegra20-ehci", "usb-ehci";
> 
> Given that, there is then always enough information in the device tree
> for the driver to be able to derive the other values from the compatible
> value.

Yes, I get it.

> Whether you want to derive the information, or whether you want to
> explicitly represent it via properties, is a decision to make based on
> the trade-offs.
> 
> Oh, and I note that quite a few device trees already use compatible
> value "usb-ehci" in their device-trees. Care needs to be taken not to
> usurp that value from any existing device drivers if that was to be
> picked as the compatible value required by this binding.

Right.  I think Tony's new binding should use compatible value 
"usb-ehci-platform".  It will essentially be a superset of "usb-ehci".

> > And doesn't the same reasoning apply to has-tt?  Doesn't that mean the 
> > driver would have to match four different hardware types?  What happens 
> > if a third characteristic like these comes around; would the driver 
> > then have to check against eight different types?
> 
> No, the compatible value represents the model, so you'd have a table like:
> 
> compatible          -> bugX has_tt
> nvidia,tegra20-ehci -> 0    1
> vendor1,foo-ehci    -> 0    1
> vendor2,bar-ehci    -> 1    1
> vendor3,baz-ehci    -> 0    1
> vendor4,qux-ehci    -> 0    1
> ...
> 
> So the table size isn't related to the number of options. The table size
> is probably bigger than subset of options combinations that make sense.

Under the circumstances, and considering that usb-ehci-platform will be
rather generic, IMO it will be better to make has-tt,
has-synopsys-hc-bug, and no-io-watchdog separate properties, each
naturally defaulting to "not present".  That will avoid the need to 
update the driver's table each time a new board comes along.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/23/2012 08:10 AM, Alan Stern wrote:
> On Mon, 22 Oct 2012, Stephen Warren wrote:
> 
>>> I see.  But why would it be done this way instead having a separate 
>>> property?
>>
>> Well, I did say normally:-)
>>
>> I can certainly see an argument for representing these differences using
>> custom properties, rather than deriving the information from the
>> compatible value. It's probably be OK to do so for something generic
>> like this; it's just perhaps not always the default choice.
>>
>> Do note that even though this binding document dictates a particular
>> value for the compatible property, every device tree should additionally
>> add a separate value alongside it to indicate the specific HW model
>> that's actually present, so that if some device-specific bug-fix or
>> workaround needs to be applied, the model can be identified anyway.
>>
>> So, rather than:
>>
>> compatible = "usb-ehci";
>>
>> You should always have e.g.:
>>
>> compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>
>> Given that, there is then always enough information in the device tree
>> for the driver to be able to derive the other values from the compatible
>> value.
> 
> Yes, I get it.
> 
>> Whether you want to derive the information, or whether you want to
>> explicitly represent it via properties, is a decision to make based on
>> the trade-offs.
>>
>> Oh, and I note that quite a few device trees already use compatible
>> value "usb-ehci" in their device-trees. Care needs to be taken not to
>> usurp that value from any existing device drivers if that was to be
>> picked as the compatible value required by this binding.
> 
> Right.  I think Tony's new binding should use compatible value 
> "usb-ehci-platform".  It will essentially be a superset of "usb-ehci".

I know this is bike-shedding a little bit, but...

The word "platform" isn't really about describing the HW, but rather is
derived from the Linux SW used to program that HW. DT should be purely
about describing the HW.

Perhaps "usb-ehci-generic" or "usb-ehci-simple" would be better?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/23/2012 08:10 AM, Alan Stern wrote:
> On Mon, 22 Oct 2012, Stephen Warren wrote:
> 
>>> I see.  But why would it be done this way instead having a separate 
>>> property?
>>
>> Well, I did say normally:-)
>>
>> I can certainly see an argument for representing these differences using
>> custom properties, rather than deriving the information from the
>> compatible value. It's probably be OK to do so for something generic
>> like this; it's just perhaps not always the default choice.
>>
>> Do note that even though this binding document dictates a particular
>> value for the compatible property, every device tree should additionally
>> add a separate value alongside it to indicate the specific HW model
>> that's actually present, so that if some device-specific bug-fix or
>> workaround needs to be applied, the model can be identified anyway.
>>
>> So, rather than:
>>
>> compatible = "usb-ehci";
>>
>> You should always have e.g.:
>>
>> compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>
>> Given that, there is then always enough information in the device tree
>> for the driver to be able to derive the other values from the compatible
>> value.
> 
> Yes, I get it.
> 
>> Whether you want to derive the information, or whether you want to
>> explicitly represent it via properties, is a decision to make based on
>> the trade-offs.
>>
>> Oh, and I note that quite a few device trees already use compatible
>> value "usb-ehci" in their device-trees. Care needs to be taken not to
>> usurp that value from any existing device drivers if that was to be
>> picked as the compatible value required by this binding.
> 
> Right.  I think Tony's new binding should use compatible value 
> "usb-ehci-platform".  It will essentially be a superset of "usb-ehci".

I know this is bike-shedding a little bit, but...

The word "platform" isn't really about describing the HW, but rather is
derived from the Linux SW used to program that HW. DT should be purely
about describing the HW.

Perhaps "usb-ehci-generic" or "usb-ehci-simple" would be better?

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Tue, 23 Oct 2012, Stephen Warren wrote:

> >> So, rather than:
> >>
> >> compatible = "usb-ehci";
> >>
> >> You should always have e.g.:
> >>
> >> compatible = "nvidia,tegra20-ehci", "usb-ehci";
> >>
> >> Given that, there is then always enough information in the device tree
> >> for the driver to be able to derive the other values from the compatible
> >> value.
> > 
> > Yes, I get it.
> > 
> >> Whether you want to derive the information, or whether you want to
> >> explicitly represent it via properties, is a decision to make based on
> >> the trade-offs.
> >>
> >> Oh, and I note that quite a few device trees already use compatible
> >> value "usb-ehci" in their device-trees. Care needs to be taken not to
> >> usurp that value from any existing device drivers if that was to be
> >> picked as the compatible value required by this binding.
> > 
> > Right.  I think Tony's new binding should use compatible value 
> > "usb-ehci-platform".  It will essentially be a superset of "usb-ehci".
> 
> I know this is bike-shedding a little bit, but...
> 
> The word "platform" isn't really about describing the HW, but rather is
> derived from the Linux SW used to program that HW. DT should be purely
> about describing the HW.
> 
> Perhaps "usb-ehci-generic" or "usb-ehci-simple" would be better?

Neither of those really describes the hardware either.  Which name gets 
used seems pretty arbitrary.

Nothing intrinsically distinguishes this class of hardware.  The only
thing these devices have in common is that they can be managed by
Linux's ehci-platform driver.  For that purpose, "usb-ehci-platform"
makes more sense than "usb-ehci-generic" or "usb-ehci-simple".

(It also allows the class to enlarge over time as the driver becomes
more capable -- is that a bad thing?  Are DT board descriptions written
for a later kernel supposed to be unusable with an earlier kernel that
doesn't support the same features?)

In essense, we're trying to say that the HW is compatible with a 
particular driver rather than with some other type of HW.  Maybe DT 
wasn't intended to provide this kind of information, but it seems like 
a logical thing to do.  Unless DT already offers some other way to 
accomplish the same thing?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Tue, 23 Oct 2012, Stephen Warren wrote:

> >> So, rather than:
> >>
> >> compatible = "usb-ehci";
> >>
> >> You should always have e.g.:
> >>
> >> compatible = "nvidia,tegra20-ehci", "usb-ehci";
> >>
> >> Given that, there is then always enough information in the device tree
> >> for the driver to be able to derive the other values from the compatible
> >> value.
> > 
> > Yes, I get it.
> > 
> >> Whether you want to derive the information, or whether you want to
> >> explicitly represent it via properties, is a decision to make based on
> >> the trade-offs.
> >>
> >> Oh, and I note that quite a few device trees already use compatible
> >> value "usb-ehci" in their device-trees. Care needs to be taken not to
> >> usurp that value from any existing device drivers if that was to be
> >> picked as the compatible value required by this binding.
> > 
> > Right.  I think Tony's new binding should use compatible value 
> > "usb-ehci-platform".  It will essentially be a superset of "usb-ehci".
> 
> I know this is bike-shedding a little bit, but...
> 
> The word "platform" isn't really about describing the HW, but rather is
> derived from the Linux SW used to program that HW. DT should be purely
> about describing the HW.
> 
> Perhaps "usb-ehci-generic" or "usb-ehci-simple" would be better?

Neither of those really describes the hardware either.  Which name gets 
used seems pretty arbitrary.

Nothing intrinsically distinguishes this class of hardware.  The only
thing these devices have in common is that they can be managed by
Linux's ehci-platform driver.  For that purpose, "usb-ehci-platform"
makes more sense than "usb-ehci-generic" or "usb-ehci-simple".

(It also allows the class to enlarge over time as the driver becomes
more capable -- is that a bad thing?  Are DT board descriptions written
for a later kernel supposed to be unusable with an earlier kernel that
doesn't support the same features?)

In essense, we're trying to say that the HW is compatible with a 
particular driver rather than with some other type of HW.  Maybe DT 
wasn't intended to provide this kind of information, but it seems like 
a logical thing to do.  Unless DT already offers some other way to 
accomplish the same thing?

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/23/2012 11:59 AM, Alan Stern wrote:
> On Tue, 23 Oct 2012, Stephen Warren wrote:
> 
>>>> So, rather than:
>>>>
>>>> compatible = "usb-ehci";
>>>>
>>>> You should always have e.g.:
>>>>
>>>> compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>>>
>>>> Given that, there is then always enough information in the device tree
>>>> for the driver to be able to derive the other values from the compatible
>>>> value.
>>>
>>> Yes, I get it.
>>>
>>>> Whether you want to derive the information, or whether you want to
>>>> explicitly represent it via properties, is a decision to make based on
>>>> the trade-offs.
>>>>
>>>> Oh, and I note that quite a few device trees already use compatible
>>>> value "usb-ehci" in their device-trees. Care needs to be taken not to
>>>> usurp that value from any existing device drivers if that was to be
>>>> picked as the compatible value required by this binding.
>>>
>>> Right.  I think Tony's new binding should use compatible value 
>>> "usb-ehci-platform".  It will essentially be a superset of "usb-ehci".
>>
>> I know this is bike-shedding a little bit, but...
>>
>> The word "platform" isn't really about describing the HW, but rather is
>> derived from the Linux SW used to program that HW. DT should be purely
>> about describing the HW.
>>
>> Perhaps "usb-ehci-generic" or "usb-ehci-simple" would be better?
> 
> Neither of those really describes the hardware either.  Which name gets 
> used seems pretty arbitrary.
> 
> Nothing intrinsically distinguishes this class of hardware.  The only
> thing these devices have in common is that they can be managed by
> Linux's ehci-platform driver.

I don't agree. They're all EHCI USB controllers (or all EHCI USB
controllers that don't require custom drivers due to quirks), which is a
much more general statement than anything related to which driver Linux
uses for them.

> For that purpose, "usb-ehci-platform"
> makes more sense than "usb-ehci-generic" or "usb-ehci-simple".
> 
> (It also allows the class to enlarge over time as the driver becomes
> more capable -- is that a bad thing?  Are DT board descriptions written
> for a later kernel supposed to be unusable with an earlier kernel that
> doesn't support the same features?)
> 
> In essense, we're trying to say that the HW is compatible with a 
> particular driver rather than with some other type of HW.

Using a compatible value in order to pick a specific driver in Linux is
explicitly against the device tree design principles; DT is supposed to
represent just the hardware.

> Maybe DT
> wasn't intended to provide this kind of information, but it seems like 
> a logical thing to do.  Unless DT already offers some other way to 
> accomplish the same thing?

The typical way is for the Linux driver to declare that is supports a
variety of different HW models.

Admittedly this is annoying when there are many HW models that otherwise
wouldn't need any driver changes, hence the desire to (additionally)
include some generic value in the compatible field, but again, what that
generic value is should be driven by the HW itself, not the SW that's
using it.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/23/2012 11:59 AM, Alan Stern wrote:
> On Tue, 23 Oct 2012, Stephen Warren wrote:
> 
>>>> So, rather than:
>>>>
>>>> compatible = "usb-ehci";
>>>>
>>>> You should always have e.g.:
>>>>
>>>> compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>>>
>>>> Given that, there is then always enough information in the device tree
>>>> for the driver to be able to derive the other values from the compatible
>>>> value.
>>>
>>> Yes, I get it.
>>>
>>>> Whether you want to derive the information, or whether you want to
>>>> explicitly represent it via properties, is a decision to make based on
>>>> the trade-offs.
>>>>
>>>> Oh, and I note that quite a few device trees already use compatible
>>>> value "usb-ehci" in their device-trees. Care needs to be taken not to
>>>> usurp that value from any existing device drivers if that was to be
>>>> picked as the compatible value required by this binding.
>>>
>>> Right.  I think Tony's new binding should use compatible value 
>>> "usb-ehci-platform".  It will essentially be a superset of "usb-ehci".
>>
>> I know this is bike-shedding a little bit, but...
>>
>> The word "platform" isn't really about describing the HW, but rather is
>> derived from the Linux SW used to program that HW. DT should be purely
>> about describing the HW.
>>
>> Perhaps "usb-ehci-generic" or "usb-ehci-simple" would be better?
> 
> Neither of those really describes the hardware either.  Which name gets 
> used seems pretty arbitrary.
> 
> Nothing intrinsically distinguishes this class of hardware.  The only
> thing these devices have in common is that they can be managed by
> Linux's ehci-platform driver.

I don't agree. They're all EHCI USB controllers (or all EHCI USB
controllers that don't require custom drivers due to quirks), which is a
much more general statement than anything related to which driver Linux
uses for them.

> For that purpose, "usb-ehci-platform"
> makes more sense than "usb-ehci-generic" or "usb-ehci-simple".
> 
> (It also allows the class to enlarge over time as the driver becomes
> more capable -- is that a bad thing?  Are DT board descriptions written
> for a later kernel supposed to be unusable with an earlier kernel that
> doesn't support the same features?)
> 
> In essense, we're trying to say that the HW is compatible with a 
> particular driver rather than with some other type of HW.

Using a compatible value in order to pick a specific driver in Linux is
explicitly against the device tree design principles; DT is supposed to
represent just the hardware.

> Maybe DT
> wasn't intended to provide this kind of information, but it seems like 
> a logical thing to do.  Unless DT already offers some other way to 
> accomplish the same thing?

The typical way is for the Linux driver to declare that is supports a
variety of different HW models.

Admittedly this is annoying when there are many HW models that otherwise
wouldn't need any driver changes, hence the desire to (additionally)
include some generic value in the compatible field, but again, what that
generic value is should be driven by the HW itself, not the SW that's
using it.

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Tue, 23 Oct 2012, Stephen Warren wrote:

> > Nothing intrinsically distinguishes this class of hardware.  The only
> > thing these devices have in common is that they can be managed by
> > Linux's ehci-platform driver.
> 
> I don't agree. They're all EHCI USB controllers (or all EHCI USB
> controllers that don't require custom drivers due to quirks), which is a
> much more general statement than anything related to which driver Linux
> uses for them.

That's true, but it doesn't distinguish these devices.  There are other
EHCI controllers with no quirks that nevertheless can't be handled by
ehci-platform because they have requirements (_not_ quirks) that
ehci-platform is unable to deal with currently -- clocks or power
supplies or special register settings or PHYs or etc.

> > In essense, we're trying to say that the HW is compatible with a 
> > particular driver rather than with some other type of HW.
> 
> Using a compatible value in order to pick a specific driver in Linux is
> explicitly against the device tree design principles; DT is supposed to
> represent just the hardware.
> 
> > Maybe DT
> > wasn't intended to provide this kind of information, but it seems like 
> > a logical thing to do.  Unless DT already offers some other way to 
> > accomplish the same thing?
> 
> The typical way is for the Linux driver to declare that is supports a
> variety of different HW models.
> 
> Admittedly this is annoying when there are many HW models that otherwise
> wouldn't need any driver changes, hence the desire to (additionally)
> include some generic value in the compatible field, but again, what that
> generic value is should be driven by the HW itself, not the SW that's
> using it.

Okay, fine.  But then what should the binding documentation specify for
the compatible value?  A list of all the HW models that the driver
currently supports?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Tue, 23 Oct 2012, Stephen Warren wrote:

> > Nothing intrinsically distinguishes this class of hardware.  The only
> > thing these devices have in common is that they can be managed by
> > Linux's ehci-platform driver.
> 
> I don't agree. They're all EHCI USB controllers (or all EHCI USB
> controllers that don't require custom drivers due to quirks), which is a
> much more general statement than anything related to which driver Linux
> uses for them.

That's true, but it doesn't distinguish these devices.  There are other
EHCI controllers with no quirks that nevertheless can't be handled by
ehci-platform because they have requirements (_not_ quirks) that
ehci-platform is unable to deal with currently -- clocks or power
supplies or special register settings or PHYs or etc.

> > In essense, we're trying to say that the HW is compatible with a 
> > particular driver rather than with some other type of HW.
> 
> Using a compatible value in order to pick a specific driver in Linux is
> explicitly against the device tree design principles; DT is supposed to
> represent just the hardware.
> 
> > Maybe DT
> > wasn't intended to provide this kind of information, but it seems like 
> > a logical thing to do.  Unless DT already offers some other way to 
> > accomplish the same thing?
> 
> The typical way is for the Linux driver to declare that is supports a
> variety of different HW models.
> 
> Admittedly this is annoying when there are many HW models that otherwise
> wouldn't need any driver changes, hence the desire to (additionally)
> include some generic value in the compatible field, but again, what that
> generic value is should be driven by the HW itself, not the SW that's
> using it.

Okay, fine.  But then what should the binding documentation specify for
the compatible value?  A list of all the HW models that the driver
currently supports?

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Rob Herring]

On 10/23/2012 02:33 PM, Alan Stern wrote:
> On Tue, 23 Oct 2012, Stephen Warren wrote:
> 
>>> Nothing intrinsically distinguishes this class of hardware.  The only
>>> thing these devices have in common is that they can be managed by
>>> Linux's ehci-platform driver.
>>
>> I don't agree. They're all EHCI USB controllers (or all EHCI USB
>> controllers that don't require custom drivers due to quirks), which is a
>> much more general statement than anything related to which driver Linux
>> uses for them.
> 
> That's true, but it doesn't distinguish these devices.  There are other
> EHCI controllers with no quirks that nevertheless can't be handled by
> ehci-platform because they have requirements (_not_ quirks) that
> ehci-platform is unable to deal with currently -- clocks or power
> supplies or special register settings or PHYs or etc.

But what if the h/w has quirks you haven't yet discovered? You need the
compatible property to be specific now, so if there are any future
driver changes needed the DT does not have to change (as it may be in
firmware which you can't change).

>>> In essense, we're trying to say that the HW is compatible with a 
>>> particular driver rather than with some other type of HW.
>>
>> Using a compatible value in order to pick a specific driver in Linux is
>> explicitly against the device tree design principles; DT is supposed to
>> represent just the hardware.
>>
>>> Maybe DT
>>> wasn't intended to provide this kind of information, but it seems like 
>>> a logical thing to do.  Unless DT already offers some other way to 
>>> accomplish the same thing?
>>
>> The typical way is for the Linux driver to declare that is supports a
>> variety of different HW models.
>>
>> Admittedly this is annoying when there are many HW models that otherwise
>> wouldn't need any driver changes, hence the desire to (additionally)
>> include some generic value in the compatible field, but again, what that
>> generic value is should be driven by the HW itself, not the SW that's
>> using it.
> 
> Okay, fine.  But then what should the binding documentation specify for
> the compatible value?  A list of all the HW models that the driver
> currently supports?

The binding docs should be independent of the driver. There may be
fields the driver ignores. So you need to document all defined
compatible strings. It is fine if the driver only has "usb-ehci", but it
is not fine for the DT to only have that.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Rob Herring]

On 10/23/2012 02:33 PM, Alan Stern wrote:
> On Tue, 23 Oct 2012, Stephen Warren wrote:
> 
>>> Nothing intrinsically distinguishes this class of hardware.  The only
>>> thing these devices have in common is that they can be managed by
>>> Linux's ehci-platform driver.
>>
>> I don't agree. They're all EHCI USB controllers (or all EHCI USB
>> controllers that don't require custom drivers due to quirks), which is a
>> much more general statement than anything related to which driver Linux
>> uses for them.
> 
> That's true, but it doesn't distinguish these devices.  There are other
> EHCI controllers with no quirks that nevertheless can't be handled by
> ehci-platform because they have requirements (_not_ quirks) that
> ehci-platform is unable to deal with currently -- clocks or power
> supplies or special register settings or PHYs or etc.

But what if the h/w has quirks you haven't yet discovered? You need the
compatible property to be specific now, so if there are any future
driver changes needed the DT does not have to change (as it may be in
firmware which you can't change).

>>> In essense, we're trying to say that the HW is compatible with a 
>>> particular driver rather than with some other type of HW.
>>
>> Using a compatible value in order to pick a specific driver in Linux is
>> explicitly against the device tree design principles; DT is supposed to
>> represent just the hardware.
>>
>>> Maybe DT
>>> wasn't intended to provide this kind of information, but it seems like 
>>> a logical thing to do.  Unless DT already offers some other way to 
>>> accomplish the same thing?
>>
>> The typical way is for the Linux driver to declare that is supports a
>> variety of different HW models.
>>
>> Admittedly this is annoying when there are many HW models that otherwise
>> wouldn't need any driver changes, hence the desire to (additionally)
>> include some generic value in the compatible field, but again, what that
>> generic value is should be driven by the HW itself, not the SW that's
>> using it.
> 
> Okay, fine.  But then what should the binding documentation specify for
> the compatible value?  A list of all the HW models that the driver
> currently supports?

The binding docs should be independent of the driver. There may be
fields the driver ignores. So you need to document all defined
compatible strings. It is fine if the driver only has "usb-ehci", but it
is not fine for the DT to only have that.

Rob

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Tue, 23 Oct 2012, Rob Herring wrote:

> On 10/23/2012 02:33 PM, Alan Stern wrote:
> > On Tue, 23 Oct 2012, Stephen Warren wrote:
> > 
> >>> Nothing intrinsically distinguishes this class of hardware.  The only
> >>> thing these devices have in common is that they can be managed by
> >>> Linux's ehci-platform driver.
> >>
> >> I don't agree. They're all EHCI USB controllers (or all EHCI USB
> >> controllers that don't require custom drivers due to quirks), which is a
> >> much more general statement than anything related to which driver Linux
> >> uses for them.
> > 
> > That's true, but it doesn't distinguish these devices.  There are other
> > EHCI controllers with no quirks that nevertheless can't be handled by
> > ehci-platform because they have requirements (_not_ quirks) that
> > ehci-platform is unable to deal with currently -- clocks or power
> > supplies or special register settings or PHYs or etc.
> 
> But what if the h/w has quirks you haven't yet discovered? You need the
> compatible property to be specific now, so if there are any future
> driver changes needed the DT does not have to change (as it may be in
> firmware which you can't change).

That argument applies always, doesn't it?  There's always a chance that 
you might discover a new quirk in a device for which the DT board file 
has already been written and committed to firmware.  The board file 
could declare that the device is compatible with something older, but 
the new quirk might ruin that compatibility.

> > Okay, fine.  But then what should the binding documentation specify for
> > the compatible value?  A list of all the HW models that the driver
> > currently supports?
> 
> The binding docs should be independent of the driver. There may be
> fields the driver ignores. So you need to document all defined
> compatible strings. It is fine if the driver only has "usb-ehci", but it
> is not fine for the DT to only have that.

Under the circumstances, do we really need a new binding document for 
the ehci-platform driver?  We should be able to use the existing 
usb-ehci binding, perhaps with some new properties added:

	has-synopsys-hc-bug
	no-io-watchdog
	has-tt

We probably can omit has-synopsys-hc-bug, as that is specific to one
type of MIPS ATH79 controller.  The driver can check for it explicitly,
if necessary.

In fact, it's not clear that these new properties need to be added now.  
They can be added over time as needed, as existing drivers are
converted over to DT.  Or is it preferable to document these things
now, preemptively as it were?

The one that matters most and is most clearly a property of the HW is
"has-tt".  IMO it should be added right away, even though there may
already be DT board files that could specify it but don't.  Right now,
for example, the ehci-xilinx-of driver checks instead for the
"support-usb-fs" property, as defined in
Documentation/devicetree/bindings/xilinx.txt.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Tue, 23 Oct 2012, Rob Herring wrote:

> On 10/23/2012 02:33 PM, Alan Stern wrote:
> > On Tue, 23 Oct 2012, Stephen Warren wrote:
> > 
> >>> Nothing intrinsically distinguishes this class of hardware.  The only
> >>> thing these devices have in common is that they can be managed by
> >>> Linux's ehci-platform driver.
> >>
> >> I don't agree. They're all EHCI USB controllers (or all EHCI USB
> >> controllers that don't require custom drivers due to quirks), which is a
> >> much more general statement than anything related to which driver Linux
> >> uses for them.
> > 
> > That's true, but it doesn't distinguish these devices.  There are other
> > EHCI controllers with no quirks that nevertheless can't be handled by
> > ehci-platform because they have requirements (_not_ quirks) that
> > ehci-platform is unable to deal with currently -- clocks or power
> > supplies or special register settings or PHYs or etc.
> 
> But what if the h/w has quirks you haven't yet discovered? You need the
> compatible property to be specific now, so if there are any future
> driver changes needed the DT does not have to change (as it may be in
> firmware which you can't change).

That argument applies always, doesn't it?  There's always a chance that 
you might discover a new quirk in a device for which the DT board file 
has already been written and committed to firmware.  The board file 
could declare that the device is compatible with something older, but 
the new quirk might ruin that compatibility.

> > Okay, fine.  But then what should the binding documentation specify for
> > the compatible value?  A list of all the HW models that the driver
> > currently supports?
> 
> The binding docs should be independent of the driver. There may be
> fields the driver ignores. So you need to document all defined
> compatible strings. It is fine if the driver only has "usb-ehci", but it
> is not fine for the DT to only have that.

Under the circumstances, do we really need a new binding document for 
the ehci-platform driver?  We should be able to use the existing 
usb-ehci binding, perhaps with some new properties added:

	has-synopsys-hc-bug
	no-io-watchdog
	has-tt

We probably can omit has-synopsys-hc-bug, as that is specific to one
type of MIPS ATH79 controller.  The driver can check for it explicitly,
if necessary.

In fact, it's not clear that these new properties need to be added now.  
They can be added over time as needed, as existing drivers are
converted over to DT.  Or is it preferable to document these things
now, preemptively as it were?

The one that matters most and is most clearly a property of the HW is
"has-tt".  IMO it should be added right away, even though there may
already be DT board files that could specify it but don't.  Right now,
for example, the ehci-xilinx-of driver checks instead for the
"support-usb-fs" property, as defined in
Documentation/devicetree/bindings/xilinx.txt.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Sebastian Andrzej Siewior]

On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
> Under the circumstances, do we really need a new binding document for 
> the ehci-platform driver?  We should be able to use the existing 
> usb-ehci binding, perhaps with some new properties added:
> 
> 	has-synopsys-hc-bug
> 	no-io-watchdog
> 	has-tt

On the PCI side we have VID, PID which is used for quirks. Sometimes we have a
revision ID which can be used to figure out if "this quirk" is still required.
The PCI driver probes by class so the ehci driver does not have a large table
of VID/PID for each controller out there. And the USB controller in two
different Intel boards has a different PID so a quirk, if required, could be
applied only to the specific mainboard.

Based on this we need atleast two compatible entries one "HW-Specific"
followed by a generic one (similar to PCI class).
Doing it the PCI way we would have to have table and figure out which
HW-specific compatible entry sets the TT flag and which one does the
no-io-watchdog. Having has-tt in compatible isn't right.

I'm all with Alan here, to make a shortcut and allow Linux specific properties
which describe a specific quirk in less code lines. Other OS can just ignore
those and build their table based on the compatible entry if they want to.

So usb-ehci should be fine. It is a generic USB-EHCI controller after all.
Quirks or no quirks, the register layout is the same, the functionality is the
same. If you can't map memory >4GiB then you need a quirk for this but you may
discover it way too late.
If your platform driver requires extra code for the PHY then it is still an
EHCI controller. The PHY wasn't setup by the bootloader / bios so Linux has to
deal with it.

> We probably can omit has-synopsys-hc-bug, as that is specific to one
> type of MIPS ATH79 controller.  The driver can check for it explicitly,
> if necessary.
> 
> In fact, it's not clear that these new properties need to be added now.  
> They can be added over time as needed, as existing drivers are
> converted over to DT.  Or is it preferable to document these things
> now, preemptively as it were?

I would add it only if required / has users.

> Alan Stern

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Sebastian Andrzej Siewior]

On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
> Under the circumstances, do we really need a new binding document for 
> the ehci-platform driver?  We should be able to use the existing 
> usb-ehci binding, perhaps with some new properties added:
> 
> 	has-synopsys-hc-bug
> 	no-io-watchdog
> 	has-tt

On the PCI side we have VID, PID which is used for quirks. Sometimes we have a
revision ID which can be used to figure out if "this quirk" is still required.
The PCI driver probes by class so the ehci driver does not have a large table
of VID/PID for each controller out there. And the USB controller in two
different Intel boards has a different PID so a quirk, if required, could be
applied only to the specific mainboard.

Based on this we need atleast two compatible entries one "HW-Specific"
followed by a generic one (similar to PCI class).
Doing it the PCI way we would have to have table and figure out which
HW-specific compatible entry sets the TT flag and which one does the
no-io-watchdog. Having has-tt in compatible isn't right.

I'm all with Alan here, to make a shortcut and allow Linux specific properties
which describe a specific quirk in less code lines. Other OS can just ignore
those and build their table based on the compatible entry if they want to.

So usb-ehci should be fine. It is a generic USB-EHCI controller after all.
Quirks or no quirks, the register layout is the same, the functionality is the
same. If you can't map memory >4GiB then you need a quirk for this but you may
discover it way too late.
If your platform driver requires extra code for the PHY then it is still an
EHCI controller. The PHY wasn't setup by the bootloader / bios so Linux has to
deal with it.

> We probably can omit has-synopsys-hc-bug, as that is specific to one
> type of MIPS ATH79 controller.  The driver can check for it explicitly,
> if necessary.
> 
> In fact, it's not clear that these new properties need to be added now.  
> They can be added over time as needed, as existing drivers are
> converted over to DT.  Or is it preferable to document these things
> now, preemptively as it were?

I would add it only if required / has users.

> Alan Stern

Sebastian

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:
> On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
>> Under the circumstances, do we really need a new binding document for 
>> the ehci-platform driver?

It seems reasonable to add the new properties to usb-ehci.txt, since
they do describe the HW.

>> We should be able to use the existing 
>> usb-ehci binding, perhaps with some new properties added:
>>
>> 	has-synopsys-hc-bug
>> 	no-io-watchdog
>> 	has-tt

That sounds fine to me.

However, there is an implementation issue here. I believe the way Linux
searches for a driver for a particular node is:

for every driver that's registered
    if the driver's supported compatible list matches the device
        use the driver

(See drivers/base/platform.c:platform_match() which implements the if
statement above, and I assume the driver core implements the outer for
loop above)

That means that if the generic driver supports compatible="usb-ehci", it
may "steal" device nodes that have
compatible="something-custom","usb-ehci", even if there's a driver
specifically for "something-custom". We would need to re-arrange the
driver matching code to:

for each compatible value in the node:
    for each driver that's registered:
        if the driver supports the compatible value:
            use the driver.

> On the PCI side we have VID, PID which is used for quirks. Sometimes we have a
> revision ID which can be used to figure out if "this quirk" is still required.
> The PCI driver probes by class so the ehci driver does not have a large table
> of VID/PID for each controller out there. And the USB controller in two
> different Intel boards has a different PID so a quirk, if required, could be
> applied only to the specific mainboard.
> 
> Based on this we need atleast two compatible entries one "HW-Specific"
> followed by a generic one (similar to PCI class).
> Doing it the PCI way we would have to have table and figure out which
> HW-specific compatible entry sets the TT flag and which one does the
> no-io-watchdog. Having has-tt in compatible isn't right.

Yes, the driver could easily bind to anything compatible with
"usb-ehci", then use the HW-specific compatible value to index into a
quirk table in the same way that specific PCI IDs index into a quirk table.

I agree that having separate compatible values like usb-ehci and
usb-ehci-with-tt probably doesn't make sense here.

> I'm all with Alan here, to make a shortcut and allow Linux specific properties
> which describe a specific quirk in less code lines. Other OS can just ignore
> those and build their table based on the compatible entry if they want to.

We should absolutely avoid Linux-specific properties where possible.

That said, what Linux-specific properties are you talking about? The
properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
are all purely a description of HW, aren't they.

> So usb-ehci should be fine. It is a generic USB-EHCI controller after all.
> Quirks or no quirks, the register layout is the same, the functionality is the
> same. If you can't map memory >4GiB then you need a quirk for this but you may
> discover it way too late.
> If your platform driver requires extra code for the PHY then it is still an
> EHCI controller. The PHY wasn't setup by the bootloader / bios so Linux has to
> deal with it.
> 
>> We probably can omit has-synopsys-hc-bug, as that is specific to one
>> type of MIPS ATH79 controller.  The driver can check for it explicitly,
>> if necessary.
>>
>> In fact, it's not clear that these new properties need to be added now.  
>> They can be added over time as needed, as existing drivers are
>> converted over to DT.  Or is it preferable to document these things
>> now, preemptively as it were?

It's best to define the binding up-front so it doesn't churn, where
possible. This will also ensure that multiple people don't try to update
the binding document to add the same feature in different ways.

> I would add it only if required / has users.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:
> On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
>> Under the circumstances, do we really need a new binding document for 
>> the ehci-platform driver?

It seems reasonable to add the new properties to usb-ehci.txt, since
they do describe the HW.

>> We should be able to use the existing 
>> usb-ehci binding, perhaps with some new properties added:
>>
>> 	has-synopsys-hc-bug
>> 	no-io-watchdog
>> 	has-tt

That sounds fine to me.

However, there is an implementation issue here. I believe the way Linux
searches for a driver for a particular node is:

for every driver that's registered
    if the driver's supported compatible list matches the device
        use the driver

(See drivers/base/platform.c:platform_match() which implements the if
statement above, and I assume the driver core implements the outer for
loop above)

That means that if the generic driver supports compatible="usb-ehci", it
may "steal" device nodes that have
compatible="something-custom","usb-ehci", even if there's a driver
specifically for "something-custom". We would need to re-arrange the
driver matching code to:

for each compatible value in the node:
    for each driver that's registered:
        if the driver supports the compatible value:
            use the driver.

> On the PCI side we have VID, PID which is used for quirks. Sometimes we have a
> revision ID which can be used to figure out if "this quirk" is still required.
> The PCI driver probes by class so the ehci driver does not have a large table
> of VID/PID for each controller out there. And the USB controller in two
> different Intel boards has a different PID so a quirk, if required, could be
> applied only to the specific mainboard.
> 
> Based on this we need atleast two compatible entries one "HW-Specific"
> followed by a generic one (similar to PCI class).
> Doing it the PCI way we would have to have table and figure out which
> HW-specific compatible entry sets the TT flag and which one does the
> no-io-watchdog. Having has-tt in compatible isn't right.

Yes, the driver could easily bind to anything compatible with
"usb-ehci", then use the HW-specific compatible value to index into a
quirk table in the same way that specific PCI IDs index into a quirk table.

I agree that having separate compatible values like usb-ehci and
usb-ehci-with-tt probably doesn't make sense here.

> I'm all with Alan here, to make a shortcut and allow Linux specific properties
> which describe a specific quirk in less code lines. Other OS can just ignore
> those and build their table based on the compatible entry if they want to.

We should absolutely avoid Linux-specific properties where possible.

That said, what Linux-specific properties are you talking about? The
properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
are all purely a description of HW, aren't they.

> So usb-ehci should be fine. It is a generic USB-EHCI controller after all.
> Quirks or no quirks, the register layout is the same, the functionality is the
> same. If you can't map memory >4GiB then you need a quirk for this but you may
> discover it way too late.
> If your platform driver requires extra code for the PHY then it is still an
> EHCI controller. The PHY wasn't setup by the bootloader / bios so Linux has to
> deal with it.
> 
>> We probably can omit has-synopsys-hc-bug, as that is specific to one
>> type of MIPS ATH79 controller.  The driver can check for it explicitly,
>> if necessary.
>>
>> In fact, it's not clear that these new properties need to be added now.  
>> They can be added over time as needed, as existing drivers are
>> converted over to DT.  Or is it preferable to document these things
>> now, preemptively as it were?

It's best to define the binding up-front so it doesn't churn, where
possible. This will also ensure that multiple people don't try to update
the binding document to add the same feature in different ways.

> I would add it only if required / has users.

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/24/2012 08:57 AM, Alan Stern wrote:
> On Tue, 23 Oct 2012, Rob Herring wrote:
> 
>> On 10/23/2012 02:33 PM, Alan Stern wrote:
>>> On Tue, 23 Oct 2012, Stephen Warren wrote:
>>>
>>>>> Nothing intrinsically distinguishes this class of hardware.  The only
>>>>> thing these devices have in common is that they can be managed by
>>>>> Linux's ehci-platform driver.
>>>>
>>>> I don't agree. They're all EHCI USB controllers (or all EHCI USB
>>>> controllers that don't require custom drivers due to quirks), which is a
>>>> much more general statement than anything related to which driver Linux
>>>> uses for them.
>>>
>>> That's true, but it doesn't distinguish these devices.  There are other
>>> EHCI controllers with no quirks that nevertheless can't be handled by
>>> ehci-platform because they have requirements (_not_ quirks) that
>>> ehci-platform is unable to deal with currently -- clocks or power
>>> supplies or special register settings or PHYs or etc.
>>
>> But what if the h/w has quirks you haven't yet discovered? You need the
>> compatible property to be specific now, so if there are any future
>> driver changes needed the DT does not have to change (as it may be in
>> firmware which you can't change).
> 
> That argument applies always, doesn't it?  There's always a chance that 
> you might discover a new quirk in a device for which the DT board file 
> has already been written and committed to firmware.  The board file 
> could declare that the device is compatible with something older, but 
> the new quirk might ruin that compatibility.

If the board file /only/ declares that the device is compatible with the
older/most-generic thing, that would be a bug in the .dts file. As a
general rule, the device tree should be fixed, although there may be
reasons to work around some buggy device trees in the kernel we should
avoid it if possible.

Device tree files always need a completely specific value in the
compatible property, even when less-specific/more-generic values are
also present. So for example, the Linux driver might only care about the
following existing:

compatible = "usb-ehci".

However, any actual EHCI controller is always some specific model, so
the compatible value must define which specific model it is, e.g.:

compatible = "nvidia,tegra20-ehci", "usb-ehci".

Now lets say the Tegra30 EHCI controller is identical to the Tegra20
controller. That needs to be explicitly represented too:

compatible = "nvidia,tegra30-ehci", "nvidia,tegra20-ehci", "usb-ehci".

In that case, the driver might still only declare support for
"nvidia,tegra20-ehci", but "nvidia,tegra30-ehci" is added to be explicit
about the actual HW model.

This doesn't continue forever though; you typically only list the
specific HW model, the base specific model it's compatible with, and any
generic value needed. So, a hypothetical Tegra40 EHCI controller would be:

compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".

Given that, there is always something to key any newly required quirk
off, so even if the need for a quirk is found later, the device tree
already contains the information needed to trigger it.

This is why quirks should always be keyed off compatible values, since
they're guaranteed to be there in a correctly written device tree.

Generic HW features (such as has-tt) don't need to be derived from the
compatible value (although they could be), since they are something
that's known up-front and hence should be present in any non-buggy
device tree from day one. That's why the binding needs to thought out
ahead of time, or if necessary (e.g. if expanded to support both EHCI
and XHCI and XYZHCI which might require extra standard properties)
evolved in a backwards-compatible fashion.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/24/2012 08:57 AM, Alan Stern wrote:
> On Tue, 23 Oct 2012, Rob Herring wrote:
> 
>> On 10/23/2012 02:33 PM, Alan Stern wrote:
>>> On Tue, 23 Oct 2012, Stephen Warren wrote:
>>>
>>>>> Nothing intrinsically distinguishes this class of hardware.  The only
>>>>> thing these devices have in common is that they can be managed by
>>>>> Linux's ehci-platform driver.
>>>>
>>>> I don't agree. They're all EHCI USB controllers (or all EHCI USB
>>>> controllers that don't require custom drivers due to quirks), which is a
>>>> much more general statement than anything related to which driver Linux
>>>> uses for them.
>>>
>>> That's true, but it doesn't distinguish these devices.  There are other
>>> EHCI controllers with no quirks that nevertheless can't be handled by
>>> ehci-platform because they have requirements (_not_ quirks) that
>>> ehci-platform is unable to deal with currently -- clocks or power
>>> supplies or special register settings or PHYs or etc.
>>
>> But what if the h/w has quirks you haven't yet discovered? You need the
>> compatible property to be specific now, so if there are any future
>> driver changes needed the DT does not have to change (as it may be in
>> firmware which you can't change).
> 
> That argument applies always, doesn't it?  There's always a chance that 
> you might discover a new quirk in a device for which the DT board file 
> has already been written and committed to firmware.  The board file 
> could declare that the device is compatible with something older, but 
> the new quirk might ruin that compatibility.

If the board file /only/ declares that the device is compatible with the
older/most-generic thing, that would be a bug in the .dts file. As a
general rule, the device tree should be fixed, although there may be
reasons to work around some buggy device trees in the kernel we should
avoid it if possible.

Device tree files always need a completely specific value in the
compatible property, even when less-specific/more-generic values are
also present. So for example, the Linux driver might only care about the
following existing:

compatible = "usb-ehci".

However, any actual EHCI controller is always some specific model, so
the compatible value must define which specific model it is, e.g.:

compatible = "nvidia,tegra20-ehci", "usb-ehci".

Now lets say the Tegra30 EHCI controller is identical to the Tegra20
controller. That needs to be explicitly represented too:

compatible = "nvidia,tegra30-ehci", "nvidia,tegra20-ehci", "usb-ehci".

In that case, the driver might still only declare support for
"nvidia,tegra20-ehci", but "nvidia,tegra30-ehci" is added to be explicit
about the actual HW model.

This doesn't continue forever though; you typically only list the
specific HW model, the base specific model it's compatible with, and any
generic value needed. So, a hypothetical Tegra40 EHCI controller would be:

compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".

Given that, there is always something to key any newly required quirk
off, so even if the need for a quirk is found later, the device tree
already contains the information needed to trigger it.

This is why quirks should always be keyed off compatible values, since
they're guaranteed to be there in a correctly written device tree.

Generic HW features (such as has-tt) don't need to be derived from the
compatible value (although they could be), since they are something
that's known up-front and hence should be present in any non-buggy
device tree from day one. That's why the binding needs to thought out
ahead of time, or if necessary (e.g. if expanded to support both EHCI
and XHCI and XYZHCI which might require extra standard properties)
evolved in a backwards-compatible fashion.

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Florian Fainelli]

On Wednesday 24 October 2012 10:16:31 Stephen Warren wrote:
> On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:
> > On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
> >> Under the circumstances, do we really need a new binding document for 
> >> the ehci-platform driver?
> 
> It seems reasonable to add the new properties to usb-ehci.txt, since
> they do describe the HW.
> 
> >> We should be able to use the existing 
> >> usb-ehci binding, perhaps with some new properties added:
> >>
> >> 	has-synopsys-hc-bug
> >> 	no-io-watchdog
> >> 	has-tt
> 
> That sounds fine to me.
> 
> However, there is an implementation issue here. I believe the way Linux
> searches for a driver for a particular node is:
> 
> for every driver that's registered
>     if the driver's supported compatible list matches the device
>         use the driver
> 
> (See drivers/base/platform.c:platform_match() which implements the if
> statement above, and I assume the driver core implements the outer for
> loop above)
> 
> That means that if the generic driver supports compatible="usb-ehci", it
> may "steal" device nodes that have
> compatible="something-custom","usb-ehci", even if there's a driver
> specifically for "something-custom". We would need to re-arrange the
> driver matching code to:
> 
> for each compatible value in the node:
>     for each driver that's registered:
>         if the driver supports the compatible value:
>             use the driver.
> 
> > On the PCI side we have VID, PID which is used for quirks. Sometimes we have a
> > revision ID which can be used to figure out if "this quirk" is still required.
> > The PCI driver probes by class so the ehci driver does not have a large table
> > of VID/PID for each controller out there. And the USB controller in two
> > different Intel boards has a different PID so a quirk, if required, could be
> > applied only to the specific mainboard.
> > 
> > Based on this we need atleast two compatible entries one "HW-Specific"
> > followed by a generic one (similar to PCI class).
> > Doing it the PCI way we would have to have table and figure out which
> > HW-specific compatible entry sets the TT flag and which one does the
> > no-io-watchdog. Having has-tt in compatible isn't right.
> 
> Yes, the driver could easily bind to anything compatible with
> "usb-ehci", then use the HW-specific compatible value to index into a
> quirk table in the same way that specific PCI IDs index into a quirk table.

Sounds good.

> 
> I agree that having separate compatible values like usb-ehci and
> usb-ehci-with-tt probably doesn't make sense here.
> 
> > I'm all with Alan here, to make a shortcut and allow Linux specific properties
> > which describe a specific quirk in less code lines. Other OS can just ignore
> > those and build their table based on the compatible entry if they want to.
> 
> We should absolutely avoid Linux-specific properties where possible.
> 
> That said, what Linux-specific properties are you talking about? The
> properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
> are all purely a description of HW, aren't they.

has-tt and has-synopsys-hc-bug are certainly hardware properties, while
no-io-watchdog is a Linux driver software workaround for a hardware issue,
so that's kind of in a grey zone to decide whether this describes hardware or
not. Let's just assume that this is a hardware issue :)

> 
> > So usb-ehci should be fine. It is a generic USB-EHCI controller after all.
> > Quirks or no quirks, the register layout is the same, the functionality is the
> > same. If you can't map memory >4GiB then you need a quirk for this but you may
> > discover it way too late.
> > If your platform driver requires extra code for the PHY then it is still an
> > EHCI controller. The PHY wasn't setup by the bootloader / bios so Linux has to
> > deal with it.
> > 
> >> We probably can omit has-synopsys-hc-bug, as that is specific to one
> >> type of MIPS ATH79 controller.  The driver can check for it explicitly,
> >> if necessary.
> >>
> >> In fact, it's not clear that these new properties need to be added now.  
> >> They can be added over time as needed, as existing drivers are
> >> converted over to DT.  Or is it preferable to document these things
> >> now, preemptively as it were?
> 
> It's best to define the binding up-front so it doesn't churn, where
> possible. This will also ensure that multiple people don't try to update
> the binding document to add the same feature in different ways.

Agreed, we do support these properties in the non-DT case, so I see no reason
why we should not document them in the binding too.

> 
> > I would add it only if required / has users.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Florian Fainelli]

On Wednesday 24 October 2012 10:16:31 Stephen Warren wrote:
> On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:
> > On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
> >> Under the circumstances, do we really need a new binding document for 
> >> the ehci-platform driver?
> 
> It seems reasonable to add the new properties to usb-ehci.txt, since
> they do describe the HW.
> 
> >> We should be able to use the existing 
> >> usb-ehci binding, perhaps with some new properties added:
> >>
> >> 	has-synopsys-hc-bug
> >> 	no-io-watchdog
> >> 	has-tt
> 
> That sounds fine to me.
> 
> However, there is an implementation issue here. I believe the way Linux
> searches for a driver for a particular node is:
> 
> for every driver that's registered
>     if the driver's supported compatible list matches the device
>         use the driver
> 
> (See drivers/base/platform.c:platform_match() which implements the if
> statement above, and I assume the driver core implements the outer for
> loop above)
> 
> That means that if the generic driver supports compatible="usb-ehci", it
> may "steal" device nodes that have
> compatible="something-custom","usb-ehci", even if there's a driver
> specifically for "something-custom". We would need to re-arrange the
> driver matching code to:
> 
> for each compatible value in the node:
>     for each driver that's registered:
>         if the driver supports the compatible value:
>             use the driver.
> 
> > On the PCI side we have VID, PID which is used for quirks. Sometimes we have a
> > revision ID which can be used to figure out if "this quirk" is still required.
> > The PCI driver probes by class so the ehci driver does not have a large table
> > of VID/PID for each controller out there. And the USB controller in two
> > different Intel boards has a different PID so a quirk, if required, could be
> > applied only to the specific mainboard.
> > 
> > Based on this we need atleast two compatible entries one "HW-Specific"
> > followed by a generic one (similar to PCI class).
> > Doing it the PCI way we would have to have table and figure out which
> > HW-specific compatible entry sets the TT flag and which one does the
> > no-io-watchdog. Having has-tt in compatible isn't right.
> 
> Yes, the driver could easily bind to anything compatible with
> "usb-ehci", then use the HW-specific compatible value to index into a
> quirk table in the same way that specific PCI IDs index into a quirk table.

Sounds good.

> 
> I agree that having separate compatible values like usb-ehci and
> usb-ehci-with-tt probably doesn't make sense here.
> 
> > I'm all with Alan here, to make a shortcut and allow Linux specific properties
> > which describe a specific quirk in less code lines. Other OS can just ignore
> > those and build their table based on the compatible entry if they want to.
> 
> We should absolutely avoid Linux-specific properties where possible.
> 
> That said, what Linux-specific properties are you talking about? The
> properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
> are all purely a description of HW, aren't they.

has-tt and has-synopsys-hc-bug are certainly hardware properties, while
no-io-watchdog is a Linux driver software workaround for a hardware issue,
so that's kind of in a grey zone to decide whether this describes hardware or
not. Let's just assume that this is a hardware issue :)

> 
> > So usb-ehci should be fine. It is a generic USB-EHCI controller after all.
> > Quirks or no quirks, the register layout is the same, the functionality is the
> > same. If you can't map memory >4GiB then you need a quirk for this but you may
> > discover it way too late.
> > If your platform driver requires extra code for the PHY then it is still an
> > EHCI controller. The PHY wasn't setup by the bootloader / bios so Linux has to
> > deal with it.
> > 
> >> We probably can omit has-synopsys-hc-bug, as that is specific to one
> >> type of MIPS ATH79 controller.  The driver can check for it explicitly,
> >> if necessary.
> >>
> >> In fact, it's not clear that these new properties need to be added now.  
> >> They can be added over time as needed, as existing drivers are
> >> converted over to DT.  Or is it preferable to document these things
> >> now, preemptively as it were?
> 
> It's best to define the binding up-front so it doesn't churn, where
> possible. This will also ensure that multiple people don't try to update
> the binding document to add the same feature in different ways.

Agreed, we do support these properties in the non-DT case, so I see no reason
why we should not document them in the binding too.

> 
> > I would add it only if required / has users.

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Stephen Warren wrote:

> On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:
> > On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
> >> Under the circumstances, do we really need a new binding document for 
> >> the ehci-platform driver?
> 
> It seems reasonable to add the new properties to usb-ehci.txt, since
> they do describe the HW.
> 
> >> We should be able to use the existing 
> >> usb-ehci binding, perhaps with some new properties added:
> >>
> >> 	has-synopsys-hc-bug
> >> 	no-io-watchdog
> >> 	has-tt
> 
> That sounds fine to me.
> 
> However, there is an implementation issue here. I believe the way Linux
> searches for a driver for a particular node is:
> 
> for every driver that's registered
>     if the driver's supported compatible list matches the device
>         use the driver
> 
> (See drivers/base/platform.c:platform_match() which implements the if
> statement above, and I assume the driver core implements the outer for
> loop above)

Yes, it does.

> That means that if the generic driver supports compatible="usb-ehci", it
> may "steal" device nodes that have
> compatible="something-custom","usb-ehci", even if there's a driver
> specifically for "something-custom". We would need to re-arrange the
> driver matching code to:
> 
> for each compatible value in the node:
>     for each driver that's registered:
>         if the driver supports the compatible value:
>             use the driver.

Which might be difficult since the inner loop would be controlled by
the outer code in the driver core.

How do we determine which existing drivers claim to support usb-ehci?  
A quick search under arch/ and drivers/ turns up nothing but
drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
match should be easy enough, and then "usb-ehci" would be safe to use
in ehci-platform.c.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Stephen Warren wrote:

> On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:
> > On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
> >> Under the circumstances, do we really need a new binding document for 
> >> the ehci-platform driver?
> 
> It seems reasonable to add the new properties to usb-ehci.txt, since
> they do describe the HW.
> 
> >> We should be able to use the existing 
> >> usb-ehci binding, perhaps with some new properties added:
> >>
> >> 	has-synopsys-hc-bug
> >> 	no-io-watchdog
> >> 	has-tt
> 
> That sounds fine to me.
> 
> However, there is an implementation issue here. I believe the way Linux
> searches for a driver for a particular node is:
> 
> for every driver that's registered
>     if the driver's supported compatible list matches the device
>         use the driver
> 
> (See drivers/base/platform.c:platform_match() which implements the if
> statement above, and I assume the driver core implements the outer for
> loop above)

Yes, it does.

> That means that if the generic driver supports compatible="usb-ehci", it
> may "steal" device nodes that have
> compatible="something-custom","usb-ehci", even if there's a driver
> specifically for "something-custom". We would need to re-arrange the
> driver matching code to:
> 
> for each compatible value in the node:
>     for each driver that's registered:
>         if the driver supports the compatible value:
>             use the driver.

Which might be difficult since the inner loop would be controlled by
the outer code in the driver core.

How do we determine which existing drivers claim to support usb-ehci?  
A quick search under arch/ and drivers/ turns up nothing but
drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
match should be easy enough, and then "usb-ehci" would be safe to use
in ehci-platform.c.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Florian Fainelli]

On Wednesday 24 October 2012 12:38:42 Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
> > On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:
> > > On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
> > >> Under the circumstances, do we really need a new binding document for 
> > >> the ehci-platform driver?
> > 
> > It seems reasonable to add the new properties to usb-ehci.txt, since
> > they do describe the HW.
> > 
> > >> We should be able to use the existing 
> > >> usb-ehci binding, perhaps with some new properties added:
> > >>
> > >> 	has-synopsys-hc-bug
> > >> 	no-io-watchdog
> > >> 	has-tt
> > 
> > That sounds fine to me.
> > 
> > However, there is an implementation issue here. I believe the way Linux
> > searches for a driver for a particular node is:
> > 
> > for every driver that's registered
> >     if the driver's supported compatible list matches the device
> >         use the driver
> > 
> > (See drivers/base/platform.c:platform_match() which implements the if
> > statement above, and I assume the driver core implements the outer for
> > loop above)
> 
> Yes, it does.
> 
> > That means that if the generic driver supports compatible="usb-ehci", it
> > may "steal" device nodes that have
> > compatible="something-custom","usb-ehci", even if there's a driver
> > specifically for "something-custom". We would need to re-arrange the
> > driver matching code to:
> > 
> > for each compatible value in the node:
> >     for each driver that's registered:
> >         if the driver supports the compatible value:
> >             use the driver.
> 
> Which might be difficult since the inner loop would be controlled by
> the outer code in the driver core.
> 
> How do we determine which existing drivers claim to support usb-ehci?  
> A quick search under arch/ and drivers/ turns up nothing but
> drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
> match should be easy enough, and then "usb-ehci" would be safe to use
> in ehci-platform.c.

As long as no one enables both ehci-platform and ehci-ppc-of at the same time
there is no problem. ehci-ppc-of should be removed in favor of ehci-platform
and make sure that the specific quirk in ehci-ppc-of also gets ported, other 
that I see no issue using "usb-ehci" as the least detailed compatible property
name.
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Florian Fainelli]

On Wednesday 24 October 2012 12:38:42 Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
> > On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:
> > > On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
> > >> Under the circumstances, do we really need a new binding document for 
> > >> the ehci-platform driver?
> > 
> > It seems reasonable to add the new properties to usb-ehci.txt, since
> > they do describe the HW.
> > 
> > >> We should be able to use the existing 
> > >> usb-ehci binding, perhaps with some new properties added:
> > >>
> > >> 	has-synopsys-hc-bug
> > >> 	no-io-watchdog
> > >> 	has-tt
> > 
> > That sounds fine to me.
> > 
> > However, there is an implementation issue here. I believe the way Linux
> > searches for a driver for a particular node is:
> > 
> > for every driver that's registered
> >     if the driver's supported compatible list matches the device
> >         use the driver
> > 
> > (See drivers/base/platform.c:platform_match() which implements the if
> > statement above, and I assume the driver core implements the outer for
> > loop above)
> 
> Yes, it does.
> 
> > That means that if the generic driver supports compatible="usb-ehci", it
> > may "steal" device nodes that have
> > compatible="something-custom","usb-ehci", even if there's a driver
> > specifically for "something-custom". We would need to re-arrange the
> > driver matching code to:
> > 
> > for each compatible value in the node:
> >     for each driver that's registered:
> >         if the driver supports the compatible value:
> >             use the driver.
> 
> Which might be difficult since the inner loop would be controlled by
> the outer code in the driver core.
> 
> How do we determine which existing drivers claim to support usb-ehci?  
> A quick search under arch/ and drivers/ turns up nothing but
> drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
> match should be easy enough, and then "usb-ehci" would be safe to use
> in ehci-platform.c.

As long as no one enables both ehci-platform and ehci-ppc-of at the same time
there is no problem. ehci-ppc-of should be removed in favor of ehci-platform
and make sure that the specific quirk in ehci-ppc-of also gets ported, other 
that I see no issue using "usb-ehci" as the least detailed compatible property
name.
--
Florian

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Stephen Warren wrote:

> We should absolutely avoid Linux-specific properties where possible.
> 
> That said, what Linux-specific properties are you talking about? The
> properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
> are all purely a description of HW, aren't they.

"has-tt" is definitely a description of the HW.

"has-synopsys-hc-bug" is too, although determining whether or not it 
should apply to a particular controller might be difficult.  I'm 
inclined not to include it among the properties.

"no-io-watchdog" is not the greatest name.  It describes to controllers 
that always do generate IRQs for I/O events when they are supposed to 
(and hence the driver doesn't need to set up a watchdog timer to detect 
I/O completions that didn't generate an IRQ).  So while the concept is 
HW-specific, the name refers to a driver implementation issue.  A 
better name might be something like "reliable-IRQs".  Again, it's not 
such an easy thing to test for.  Almost all the existing drivers leave 
it unset.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Stephen Warren wrote:

> We should absolutely avoid Linux-specific properties where possible.
> 
> That said, what Linux-specific properties are you talking about? The
> properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
> are all purely a description of HW, aren't they.

"has-tt" is definitely a description of the HW.

"has-synopsys-hc-bug" is too, although determining whether or not it 
should apply to a particular controller might be difficult.  I'm 
inclined not to include it among the properties.

"no-io-watchdog" is not the greatest name.  It describes to controllers 
that always do generate IRQs for I/O events when they are supposed to 
(and hence the driver doesn't need to set up a watchdog timer to detect 
I/O completions that didn't generate an IRQ).  So while the concept is 
HW-specific, the name refers to a driver implementation issue.  A 
better name might be something like "reliable-IRQs".  Again, it's not 
such an easy thing to test for.  Almost all the existing drivers leave 
it unset.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/24/2012 10:38 AM, Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
>> On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:
>>> On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
>>>> Under the circumstances, do we really need a new binding document for 
>>>> the ehci-platform driver?
>>
>> It seems reasonable to add the new properties to usb-ehci.txt, since
>> they do describe the HW.
>>
>>>> We should be able to use the existing 
>>>> usb-ehci binding, perhaps with some new properties added:
>>>>
>>>> 	has-synopsys-hc-bug
>>>> 	no-io-watchdog
>>>> 	has-tt
>>
>> That sounds fine to me.
>>
>> However, there is an implementation issue here. I believe the way Linux
>> searches for a driver for a particular node is:
>>
>> for every driver that's registered
>>     if the driver's supported compatible list matches the device
>>         use the driver
>>
>> (See drivers/base/platform.c:platform_match() which implements the if
>> statement above, and I assume the driver core implements the outer for
>> loop above)
> 
> Yes, it does.
> 
>> That means that if the generic driver supports compatible="usb-ehci", it
>> may "steal" device nodes that have
>> compatible="something-custom","usb-ehci", even if there's a driver
>> specifically for "something-custom". We would need to re-arrange the
>> driver matching code to:
>>
>> for each compatible value in the node:
>>     for each driver that's registered:
>>         if the driver supports the compatible value:
>>             use the driver.
> 
> Which might be difficult since the inner loop would be controlled by
> the outer code in the driver core.
> 
> How do we determine which existing drivers claim to support usb-ehci?  
> A quick search under arch/ and drivers/ turns up nothing but
> drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
> match should be easy enough, and then "usb-ehci" would be safe to use
> in ehci-platform.c.

It's not drivers that claim to support usb-ehci, but device tree files
that contain nodes that include "usb-ehci" in their compatible value,
yet need to be handled by a driver that isn't the generic USB EHCI
driver, and that driver binds to the other more specific compatible value:

> $ grep -HrnI usb-ehci arch/*/boot/dts
> arch/arm/boot/dts/spear3xx.dtsi:76:			compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/at91sam9x5.dtsi:399:			compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> arch/arm/boot/dts/spear13xx.dtsi:145:			compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear13xx.dtsi:152:			compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/tegra20.dtsip:215:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/tegra20.dtsip:224:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/tegra20.dtsip:232:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/spear600.dtsi:88:			compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear600.dtsi:96:			compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/at91sam9g45.dtsi:392:			compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> arch/powerpc/boot/dts/sequoia.dts:156:			compatible = "ibm,usb-ehci-440epx", "usb-ehci";
> arch/powerpc/boot/dts/wii.dts:120:			compatible = "nintendo,hollywood-usb-ehci",
> arch/powerpc/boot/dts/wii.dts:121:					"usb-ehci";
> arch/powerpc/boot/dts/canyonlands.dts:167:			compatible = "ibm,usb-ehci-460ex", "usb-ehci";

and then search for all those other compatible values in drivers. The
ARM instances certainly all have this issue (although perhaps it's worth
investigating if a generic could work for them instead). The PPC
instances need more investigation; the values don't show up in of match
tables but rather in code.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/24/2012 10:38 AM, Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
>> On 10/24/2012 09:26 AM, Sebastian Andrzej Siewior wrote:
>>> On Wed, Oct 24, 2012 at 10:57:00AM -0400, Alan Stern wrote:
>>>> Under the circumstances, do we really need a new binding document for 
>>>> the ehci-platform driver?
>>
>> It seems reasonable to add the new properties to usb-ehci.txt, since
>> they do describe the HW.
>>
>>>> We should be able to use the existing 
>>>> usb-ehci binding, perhaps with some new properties added:
>>>>
>>>> 	has-synopsys-hc-bug
>>>> 	no-io-watchdog
>>>> 	has-tt
>>
>> That sounds fine to me.
>>
>> However, there is an implementation issue here. I believe the way Linux
>> searches for a driver for a particular node is:
>>
>> for every driver that's registered
>>     if the driver's supported compatible list matches the device
>>         use the driver
>>
>> (See drivers/base/platform.c:platform_match() which implements the if
>> statement above, and I assume the driver core implements the outer for
>> loop above)
> 
> Yes, it does.
> 
>> That means that if the generic driver supports compatible="usb-ehci", it
>> may "steal" device nodes that have
>> compatible="something-custom","usb-ehci", even if there's a driver
>> specifically for "something-custom". We would need to re-arrange the
>> driver matching code to:
>>
>> for each compatible value in the node:
>>     for each driver that's registered:
>>         if the driver supports the compatible value:
>>             use the driver.
> 
> Which might be difficult since the inner loop would be controlled by
> the outer code in the driver core.
> 
> How do we determine which existing drivers claim to support usb-ehci?  
> A quick search under arch/ and drivers/ turns up nothing but
> drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
> match should be easy enough, and then "usb-ehci" would be safe to use
> in ehci-platform.c.

It's not drivers that claim to support usb-ehci, but device tree files
that contain nodes that include "usb-ehci" in their compatible value,
yet need to be handled by a driver that isn't the generic USB EHCI
driver, and that driver binds to the other more specific compatible value:

> $ grep -HrnI usb-ehci arch/*/boot/dts
> arch/arm/boot/dts/spear3xx.dtsi:76:			compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/at91sam9x5.dtsi:399:			compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> arch/arm/boot/dts/spear13xx.dtsi:145:			compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear13xx.dtsi:152:			compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/tegra20.dtsip:215:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/tegra20.dtsip:224:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/tegra20.dtsip:232:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/spear600.dtsi:88:			compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear600.dtsi:96:			compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/at91sam9g45.dtsi:392:			compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> arch/powerpc/boot/dts/sequoia.dts:156:			compatible = "ibm,usb-ehci-440epx", "usb-ehci";
> arch/powerpc/boot/dts/wii.dts:120:			compatible = "nintendo,hollywood-usb-ehci",
> arch/powerpc/boot/dts/wii.dts:121:					"usb-ehci";
> arch/powerpc/boot/dts/canyonlands.dts:167:			compatible = "ibm,usb-ehci-460ex", "usb-ehci";

and then search for all those other compatible values in drivers. The
ARM instances certainly all have this issue (although perhaps it's worth
investigating if a generic could work for them instead). The PPC
instances need more investigation; the values don't show up in of match
tables but rather in code.

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/24/2012 10:44 AM, Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
>> We should absolutely avoid Linux-specific properties where possible.
>>
>> That said, what Linux-specific properties are you talking about? The
>> properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
>> are all purely a description of HW, aren't they.
> 
> "has-tt" is definitely a description of the HW.

OK.

> "has-synopsys-hc-bug" is too, although determining whether or not it 
> should apply to a particular controller might be difficult.  I'm 
> inclined not to include it among the properties.
> 
> "no-io-watchdog" is not the greatest name.  It describes to controllers 
> that always do generate IRQs for I/O events when they are supposed to 
> (and hence the driver doesn't need to set up a watchdog timer to detect 
> I/O completions that didn't generate an IRQ).  So while the concept is 
> HW-specific, the name refers to a driver implementation issue.  A 
> better name might be something like "reliable-IRQs".  Again, it's not 
> such an easy thing to test for.  Almost all the existing drivers leave 
> it unset.

OK, I'd be inclined to drive those last two by quirks then, since they
aren't architectural features of EHCI but rather implementation issues.
And indeed have the quirk table have a "reliable IRQs" field instead of
"no IO watchdog", to minimize the table size.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/24/2012 10:44 AM, Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
>> We should absolutely avoid Linux-specific properties where possible.
>>
>> That said, what Linux-specific properties are you talking about? The
>> properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
>> are all purely a description of HW, aren't they.
> 
> "has-tt" is definitely a description of the HW.

OK.

> "has-synopsys-hc-bug" is too, although determining whether or not it 
> should apply to a particular controller might be difficult.  I'm 
> inclined not to include it among the properties.
> 
> "no-io-watchdog" is not the greatest name.  It describes to controllers 
> that always do generate IRQs for I/O events when they are supposed to 
> (and hence the driver doesn't need to set up a watchdog timer to detect 
> I/O completions that didn't generate an IRQ).  So while the concept is 
> HW-specific, the name refers to a driver implementation issue.  A 
> better name might be something like "reliable-IRQs".  Again, it's not 
> such an easy thing to test for.  Almost all the existing drivers leave 
> it unset.

OK, I'd be inclined to drive those last two by quirks then, since they
aren't architectural features of EHCI but rather implementation issues.
And indeed have the quirk table have a "reliable IRQs" field instead of
"no IO watchdog", to minimize the table size.

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Stephen Warren wrote:

> Device tree files always need a completely specific value in the
> compatible property, even when less-specific/more-generic values are
> also present. So for example, the Linux driver might only care about the
> following existing:
> 
> compatible = "usb-ehci".
> 
> However, any actual EHCI controller is always some specific model, so
> the compatible value must define which specific model it is, e.g.:
> 
> compatible = "nvidia,tegra20-ehci", "usb-ehci".
> 
> Now lets say the Tegra30 EHCI controller is identical to the Tegra20
> controller. That needs to be explicitly represented too:
> 
> compatible = "nvidia,tegra30-ehci", "nvidia,tegra20-ehci", "usb-ehci".
> 
> In that case, the driver might still only declare support for
> "nvidia,tegra20-ehci", but "nvidia,tegra30-ehci" is added to be explicit
> about the actual HW model.
> 
> This doesn't continue forever though; you typically only list the
> specific HW model, the base specific model it's compatible with, and any
> generic value needed. So, a hypothetical Tegra40 EHCI controller would be:
> 
> compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".
> 
> Given that, there is always something to key any newly required quirk
> off, so even if the need for a quirk is found later, the device tree
> already contains the information needed to trigger it.

Okay.  It appears that quite a few .dts/.dtsi files mention "usb-ehci", 
for no apparent reason (given that the drivers don't list it):

[stern@iolanthe usb-3.6]$ find arch -name '*.dts*' | xargs grep usb-ehci
arch/mips/cavium-octeon/octeon_3xxx.dts:                                compatible = "cavium,octeon-6335-ehci","usb-ehci";
arch/mips/cavium-octeon/octeon_68xx.dts:                                compatible = "cavium,octeon-6335-ehci","usb-ehci";
arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
arch/arm/boot/dts/at91sam9x5.dtsi:                      compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ehci", "usb-ehci";
arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ehci", "usb-ehci";
arch/arm/boot/dts/spear3xx.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
arch/arm/boot/dts/at91sam9g45.dtsi:                     compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
arch/powerpc/boot/dts/wii.dts:                  compatible = "nintendo,hollywood-usb-ehci",
arch/powerpc/boot/dts/wii.dts:                                  "usb-ehci";
arch/powerpc/boot/dts/sequoia.dts:                      compatible = "ibm,usb-ehci-440epx", "usb-ehci";
arch/powerpc/boot/dts/canyonlands.dts:                  compatible = "ibm,usb-ehci-460ex", "usb-ehci";

Is there a real reason that I'm not aware of?  Or can all these entries
safely be removed?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Stephen Warren wrote:

> Device tree files always need a completely specific value in the
> compatible property, even when less-specific/more-generic values are
> also present. So for example, the Linux driver might only care about the
> following existing:
> 
> compatible = "usb-ehci".
> 
> However, any actual EHCI controller is always some specific model, so
> the compatible value must define which specific model it is, e.g.:
> 
> compatible = "nvidia,tegra20-ehci", "usb-ehci".
> 
> Now lets say the Tegra30 EHCI controller is identical to the Tegra20
> controller. That needs to be explicitly represented too:
> 
> compatible = "nvidia,tegra30-ehci", "nvidia,tegra20-ehci", "usb-ehci".
> 
> In that case, the driver might still only declare support for
> "nvidia,tegra20-ehci", but "nvidia,tegra30-ehci" is added to be explicit
> about the actual HW model.
> 
> This doesn't continue forever though; you typically only list the
> specific HW model, the base specific model it's compatible with, and any
> generic value needed. So, a hypothetical Tegra40 EHCI controller would be:
> 
> compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".
> 
> Given that, there is always something to key any newly required quirk
> off, so even if the need for a quirk is found later, the device tree
> already contains the information needed to trigger it.

Okay.  It appears that quite a few .dts/.dtsi files mention "usb-ehci", 
for no apparent reason (given that the drivers don't list it):

[stern at iolanthe usb-3.6]$ find arch -name '*.dts*' | xargs grep usb-ehci
arch/mips/cavium-octeon/octeon_3xxx.dts:                                compatible = "cavium,octeon-6335-ehci","usb-ehci";
arch/mips/cavium-octeon/octeon_68xx.dts:                                compatible = "cavium,octeon-6335-ehci","usb-ehci";
arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
arch/arm/boot/dts/at91sam9x5.dtsi:                      compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ehci", "usb-ehci";
arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ehci", "usb-ehci";
arch/arm/boot/dts/spear3xx.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
arch/arm/boot/dts/at91sam9g45.dtsi:                     compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
arch/powerpc/boot/dts/wii.dts:                  compatible = "nintendo,hollywood-usb-ehci",
arch/powerpc/boot/dts/wii.dts:                                  "usb-ehci";
arch/powerpc/boot/dts/sequoia.dts:                      compatible = "ibm,usb-ehci-440epx", "usb-ehci";
arch/powerpc/boot/dts/canyonlands.dts:                  compatible = "ibm,usb-ehci-460ex", "usb-ehci";

Is there a real reason that I'm not aware of?  Or can all these entries
safely be removed?

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Florian Fainelli]

On Wednesday 24 October 2012 12:54:05 Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
> > Device tree files always need a completely specific value in the
> > compatible property, even when less-specific/more-generic values are
> > also present. So for example, the Linux driver might only care about the
> > following existing:
> > 
> > compatible = "usb-ehci".
> > 
> > However, any actual EHCI controller is always some specific model, so
> > the compatible value must define which specific model it is, e.g.:
> > 
> > compatible = "nvidia,tegra20-ehci", "usb-ehci".
> > 
> > Now lets say the Tegra30 EHCI controller is identical to the Tegra20
> > controller. That needs to be explicitly represented too:
> > 
> > compatible = "nvidia,tegra30-ehci", "nvidia,tegra20-ehci", "usb-ehci".
> > 
> > In that case, the driver might still only declare support for
> > "nvidia,tegra20-ehci", but "nvidia,tegra30-ehci" is added to be explicit
> > about the actual HW model.
> > 
> > This doesn't continue forever though; you typically only list the
> > specific HW model, the base specific model it's compatible with, and any
> > generic value needed. So, a hypothetical Tegra40 EHCI controller would be:
> > 
> > compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".
> > 
> > Given that, there is always something to key any newly required quirk
> > off, so even if the need for a quirk is found later, the device tree
> > already contains the information needed to trigger it.
> 
> Okay.  It appears that quite a few .dts/.dtsi files mention "usb-ehci", 
> for no apparent reason (given that the drivers don't list it):
> 
> [stern@iolanthe usb-3.6]$ find arch -name '*.dts*' | xargs grep usb-ehci
> arch/mips/cavium-octeon/octeon_3xxx.dts:                                compatible = "cavium,octeon-6335-ehci","usb-ehci";
> arch/mips/cavium-octeon/octeon_68xx.dts:                                compatible = "cavium,octeon-6335-ehci","usb-ehci";
> arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/at91sam9x5.dtsi:                      compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear3xx.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/at91sam9g45.dtsi:                     compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> arch/powerpc/boot/dts/wii.dts:                  compatible = "nintendo,hollywood-usb-ehci",
> arch/powerpc/boot/dts/wii.dts:                                  "usb-ehci";
> arch/powerpc/boot/dts/sequoia.dts:                      compatible = "ibm,usb-ehci-440epx", "usb-ehci";
> arch/powerpc/boot/dts/canyonlands.dts:                  compatible = "ibm,usb-ehci-460ex", "usb-ehci";
> 
> Is there a real reason that I'm not aware of?  Or can all these entries
> safely be removed?

Apart from the powerpc entries for which a real driver exists, the others were
probably added in the hope that sooner or later, someone will come up with
a matching driver, and the corresponding dts file would not even have to be
updated to benefit from this.
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Florian Fainelli]

On Wednesday 24 October 2012 12:54:05 Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
> > Device tree files always need a completely specific value in the
> > compatible property, even when less-specific/more-generic values are
> > also present. So for example, the Linux driver might only care about the
> > following existing:
> > 
> > compatible = "usb-ehci".
> > 
> > However, any actual EHCI controller is always some specific model, so
> > the compatible value must define which specific model it is, e.g.:
> > 
> > compatible = "nvidia,tegra20-ehci", "usb-ehci".
> > 
> > Now lets say the Tegra30 EHCI controller is identical to the Tegra20
> > controller. That needs to be explicitly represented too:
> > 
> > compatible = "nvidia,tegra30-ehci", "nvidia,tegra20-ehci", "usb-ehci".
> > 
> > In that case, the driver might still only declare support for
> > "nvidia,tegra20-ehci", but "nvidia,tegra30-ehci" is added to be explicit
> > about the actual HW model.
> > 
> > This doesn't continue forever though; you typically only list the
> > specific HW model, the base specific model it's compatible with, and any
> > generic value needed. So, a hypothetical Tegra40 EHCI controller would be:
> > 
> > compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".
> > 
> > Given that, there is always something to key any newly required quirk
> > off, so even if the need for a quirk is found later, the device tree
> > already contains the information needed to trigger it.
> 
> Okay.  It appears that quite a few .dts/.dtsi files mention "usb-ehci", 
> for no apparent reason (given that the drivers don't list it):
> 
> [stern at iolanthe usb-3.6]$ find arch -name '*.dts*' | xargs grep usb-ehci
> arch/mips/cavium-octeon/octeon_3xxx.dts:                                compatible = "cavium,octeon-6335-ehci","usb-ehci";
> arch/mips/cavium-octeon/octeon_68xx.dts:                                compatible = "cavium,octeon-6335-ehci","usb-ehci";
> arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/tegra20.dtsi:         compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arch/arm/boot/dts/at91sam9x5.dtsi:                      compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear3xx.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ehci", "usb-ehci";
> arch/arm/boot/dts/at91sam9g45.dtsi:                     compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> arch/powerpc/boot/dts/wii.dts:                  compatible = "nintendo,hollywood-usb-ehci",
> arch/powerpc/boot/dts/wii.dts:                                  "usb-ehci";
> arch/powerpc/boot/dts/sequoia.dts:                      compatible = "ibm,usb-ehci-440epx", "usb-ehci";
> arch/powerpc/boot/dts/canyonlands.dts:                  compatible = "ibm,usb-ehci-460ex", "usb-ehci";
> 
> Is there a real reason that I'm not aware of?  Or can all these entries
> safely be removed?

Apart from the powerpc entries for which a real driver exists, the others were
probably added in the hope that sooner or later, someone will come up with
a matching driver, and the corresponding dts file would not even have to be
updated to benefit from this.
--
Florian

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Rob Herring]

On 10/24/2012 11:44 AM, Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
>> We should absolutely avoid Linux-specific properties where possible.
>>
>> That said, what Linux-specific properties are you talking about? The
>> properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
>> are all purely a description of HW, aren't they.
> 
> "has-tt" is definitely a description of the HW.

Can you spell out tt.

> "has-synopsys-hc-bug" is too, although determining whether or not it 
> should apply to a particular controller might be difficult.  I'm 
> inclined not to include it among the properties.

What happens when there are 2 synopsys hc bugs? Something more specific
about what the bug is would be better.

> "no-io-watchdog" is not the greatest name.  It describes to controllers 
> that always do generate IRQs for I/O events when they are supposed to 
> (and hence the driver doesn't need to set up a watchdog timer to detect 
> I/O completions that didn't generate an IRQ).  So while the concept is 
> HW-specific, the name refers to a driver implementation issue.  A 
> better name might be something like "reliable-IRQs".  Again, it's not 
> such an easy thing to test for.  Almost all the existing drivers leave 
> it unset.

Shouldn't the default be reliable irqs? What about "unreliable-irqs"?

Rob

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Rob Herring]

On 10/24/2012 11:44 AM, Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
>> We should absolutely avoid Linux-specific properties where possible.
>>
>> That said, what Linux-specific properties are you talking about? The
>> properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
>> are all purely a description of HW, aren't they.
> 
> "has-tt" is definitely a description of the HW.

Can you spell out tt.

> "has-synopsys-hc-bug" is too, although determining whether or not it 
> should apply to a particular controller might be difficult.  I'm 
> inclined not to include it among the properties.

What happens when there are 2 synopsys hc bugs? Something more specific
about what the bug is would be better.

> "no-io-watchdog" is not the greatest name.  It describes to controllers 
> that always do generate IRQs for I/O events when they are supposed to 
> (and hence the driver doesn't need to set up a watchdog timer to detect 
> I/O completions that didn't generate an IRQ).  So while the concept is 
> HW-specific, the name refers to a driver implementation issue.  A 
> better name might be something like "reliable-IRQs".  Again, it's not 
> such an easy thing to test for.  Almost all the existing drivers leave 
> it unset.

Shouldn't the default be reliable irqs? What about "unreliable-irqs"?

Rob

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Stephen Warren wrote:

> > How do we determine which existing drivers claim to support usb-ehci?  
> > A quick search under arch/ and drivers/ turns up nothing but
> > drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
> > match should be easy enough, and then "usb-ehci" would be safe to use
> > in ehci-platform.c.
> 
> It's not drivers that claim to support usb-ehci, but device tree files
> that contain nodes that include "usb-ehci" in their compatible value,
> yet need to be handled by a driver that isn't the generic USB EHCI
> driver, and that driver binds to the other more specific compatible value:
> 
> > $ grep -HrnI usb-ehci arch/*/boot/dts
> > arch/arm/boot/dts/spear3xx.dtsi:76:			compatible = "st,spear600-ehci", "usb-ehci";
> > arch/arm/boot/dts/at91sam9x5.dtsi:399:			compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> > arch/arm/boot/dts/spear13xx.dtsi:145:			compatible = "st,spear600-ehci", "usb-ehci";
> > arch/arm/boot/dts/spear13xx.dtsi:152:			compatible = "st,spear600-ehci", "usb-ehci";
> > arch/arm/boot/dts/tegra20.dtsip:215:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> > arch/arm/boot/dts/tegra20.dtsip:224:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> > arch/arm/boot/dts/tegra20.dtsip:232:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> > arch/arm/boot/dts/spear600.dtsi:88:			compatible = "st,spear600-ehci", "usb-ehci";
> > arch/arm/boot/dts/spear600.dtsi:96:			compatible = "st,spear600-ehci", "usb-ehci";
> > arch/arm/boot/dts/at91sam9g45.dtsi:392:			compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> > arch/powerpc/boot/dts/sequoia.dts:156:			compatible = "ibm,usb-ehci-440epx", "usb-ehci";
> > arch/powerpc/boot/dts/wii.dts:120:			compatible = "nintendo,hollywood-usb-ehci",
> > arch/powerpc/boot/dts/wii.dts:121:					"usb-ehci";
> > arch/powerpc/boot/dts/canyonlands.dts:167:			compatible = "ibm,usb-ehci-460ex", "usb-ehci";
> 
> and then search for all those other compatible values in drivers. The
> ARM instances certainly all have this issue (although perhaps it's worth
> investigating if a generic could work for them instead). The PPC
> instances need more investigation; the values don't show up in of match
> tables but rather in code.

Ah, now I'm starting to get the picture.

So by listing "usb-ehci" in its device ID table, a driver would
essentially be saying that it can handle _all_ USB EHCI controllers.  
(Which means that the entry in ehci-ppc-of.c is questionable; perhaps
the intention is that this driver really does handle all EHCI
controllers on any PPC-based OpenFirmware platform bus.)

> This doesn't continue forever though; you typically only list the
> specific HW model, the base specific model it's compatible with, and any
> generic value needed. So, a hypothetical Tegra40 EHCI controller would be:
> 
> compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".

What's the reason for listing the generic value if drivers can't key
off it?  Does it contain any information that's not already present in 
the specific values?

It sounds like the ehci-platform driver should simply list all the
specific HW device types (or base HW device types) that it handles, and
it shouldn't include "usb-ehci" in the list.  As more DT board files
are created or as ehci-platform becomes capable to taking over from
other drivers, its device list would grow.

And it sounds like the only property we really need to add to the 
usb-ehci binding at the moment is "has-tt".

Alan Stern

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Stephen Warren wrote:

> > How do we determine which existing drivers claim to support usb-ehci?  
> > A quick search under arch/ and drivers/ turns up nothing but
> > drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
> > match should be easy enough, and then "usb-ehci" would be safe to use
> > in ehci-platform.c.
> 
> It's not drivers that claim to support usb-ehci, but device tree files
> that contain nodes that include "usb-ehci" in their compatible value,
> yet need to be handled by a driver that isn't the generic USB EHCI
> driver, and that driver binds to the other more specific compatible value:
> 
> > $ grep -HrnI usb-ehci arch/*/boot/dts
> > arch/arm/boot/dts/spear3xx.dtsi:76:			compatible = "st,spear600-ehci", "usb-ehci";
> > arch/arm/boot/dts/at91sam9x5.dtsi:399:			compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> > arch/arm/boot/dts/spear13xx.dtsi:145:			compatible = "st,spear600-ehci", "usb-ehci";
> > arch/arm/boot/dts/spear13xx.dtsi:152:			compatible = "st,spear600-ehci", "usb-ehci";
> > arch/arm/boot/dts/tegra20.dtsip:215:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> > arch/arm/boot/dts/tegra20.dtsip:224:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> > arch/arm/boot/dts/tegra20.dtsip:232:		compatible = "nvidia,tegra20-ehci", "usb-ehci";
> > arch/arm/boot/dts/spear600.dtsi:88:			compatible = "st,spear600-ehci", "usb-ehci";
> > arch/arm/boot/dts/spear600.dtsi:96:			compatible = "st,spear600-ehci", "usb-ehci";
> > arch/arm/boot/dts/at91sam9g45.dtsi:392:			compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> > arch/powerpc/boot/dts/sequoia.dts:156:			compatible = "ibm,usb-ehci-440epx", "usb-ehci";
> > arch/powerpc/boot/dts/wii.dts:120:			compatible = "nintendo,hollywood-usb-ehci",
> > arch/powerpc/boot/dts/wii.dts:121:					"usb-ehci";
> > arch/powerpc/boot/dts/canyonlands.dts:167:			compatible = "ibm,usb-ehci-460ex", "usb-ehci";
> 
> and then search for all those other compatible values in drivers. The
> ARM instances certainly all have this issue (although perhaps it's worth
> investigating if a generic could work for them instead). The PPC
> instances need more investigation; the values don't show up in of match
> tables but rather in code.

Ah, now I'm starting to get the picture.

So by listing "usb-ehci" in its device ID table, a driver would
essentially be saying that it can handle _all_ USB EHCI controllers.  
(Which means that the entry in ehci-ppc-of.c is questionable; perhaps
the intention is that this driver really does handle all EHCI
controllers on any PPC-based OpenFirmware platform bus.)

> This doesn't continue forever though; you typically only list the
> specific HW model, the base specific model it's compatible with, and any
> generic value needed. So, a hypothetical Tegra40 EHCI controller would be:
> 
> compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".

What's the reason for listing the generic value if drivers can't key
off it?  Does it contain any information that's not already present in 
the specific values?

It sounds like the ehci-platform driver should simply list all the
specific HW device types (or base HW device types) that it handles, and
it shouldn't include "usb-ehci" in the list.  As more DT board files
are created or as ehci-platform becomes capable to taking over from
other drivers, its device list would grow.

And it sounds like the only property we really need to add to the 
usb-ehci binding at the moment is "has-tt".

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Rob Herring wrote:

> On 10/24/2012 11:44 AM, Alan Stern wrote:
> > On Wed, 24 Oct 2012, Stephen Warren wrote:
> > 
> >> We should absolutely avoid Linux-specific properties where possible.
> >>
> >> That said, what Linux-specific properties are you talking about? The
> >> properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
> >> are all purely a description of HW, aren't they.
> > 
> > "has-tt" is definitely a description of the HW.
> 
> Can you spell out tt.

It stands for Transaction Translator.  The acronym is a standard one, 
used in the USB specs.

> > "has-synopsys-hc-bug" is too, although determining whether or not it 
> > should apply to a particular controller might be difficult.  I'm 
> > inclined not to include it among the properties.
> 
> What happens when there are 2 synopsys hc bugs? Something more specific
> about what the bug is would be better.

We will leave it out.

> > "no-io-watchdog" is not the greatest name.  It describes to controllers 
> > that always do generate IRQs for I/O events when they are supposed to 
> > (and hence the driver doesn't need to set up a watchdog timer to detect 
> > I/O completions that didn't generate an IRQ).  So while the concept is 
> > HW-specific, the name refers to a driver implementation issue.  A 
> > better name might be something like "reliable-IRQs".  Again, it's not 
> > such an easy thing to test for.  Almost all the existing drivers leave 
> > it unset.
> 
> Shouldn't the default be reliable irqs? What about "unreliable-irqs"?

I don't know why the default was set the way it was.  That was before 
my time as EHCI maintainer.  Right now only a few EHCI drivers claim to 
have reliable IRQs.

Avoiding the watchdog timer is a fairly minor optimization in any case.  
It fires only once every 100 ms, and only while I/O is in progress.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Rob Herring wrote:

> On 10/24/2012 11:44 AM, Alan Stern wrote:
> > On Wed, 24 Oct 2012, Stephen Warren wrote:
> > 
> >> We should absolutely avoid Linux-specific properties where possible.
> >>
> >> That said, what Linux-specific properties are you talking about? The
> >> properties discussed here (has-synopsys-hc-bug, no-io-watchdog, has-tt)
> >> are all purely a description of HW, aren't they.
> > 
> > "has-tt" is definitely a description of the HW.
> 
> Can you spell out tt.

It stands for Transaction Translator.  The acronym is a standard one, 
used in the USB specs.

> > "has-synopsys-hc-bug" is too, although determining whether or not it 
> > should apply to a particular controller might be difficult.  I'm 
> > inclined not to include it among the properties.
> 
> What happens when there are 2 synopsys hc bugs? Something more specific
> about what the bug is would be better.

We will leave it out.

> > "no-io-watchdog" is not the greatest name.  It describes to controllers 
> > that always do generate IRQs for I/O events when they are supposed to 
> > (and hence the driver doesn't need to set up a watchdog timer to detect 
> > I/O completions that didn't generate an IRQ).  So while the concept is 
> > HW-specific, the name refers to a driver implementation issue.  A 
> > better name might be something like "reliable-IRQs".  Again, it's not 
> > such an easy thing to test for.  Almost all the existing drivers leave 
> > it unset.
> 
> Shouldn't the default be reliable irqs? What about "unreliable-irqs"?

I don't know why the default was set the way it was.  That was before 
my time as EHCI maintainer.  Right now only a few EHCI drivers claim to 
have reliable IRQs.

Avoiding the watchdog timer is a fairly minor optimization in any case.  
It fires only once every 100 ms, and only while I/O is in progress.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Florian Fainelli wrote:

> As long as no one enables both ehci-platform and ehci-ppc-of at the same time
> there is no problem. ehci-ppc-of should be removed in favor of ehci-platform
> and make sure that the specific quirk in ehci-ppc-of also gets ported, other 
> that I see no issue using "usb-ehci" as the least detailed compatible property
> name.

Suppose a DT board file is created for a oontroller which ehci-platform
can't handle.  Then by your proposal, the board file shouldn't have
"usb-ehci" in its compatible property.

Now later on, suppose ehci-platform is enhanced so that it can manage
that controller.  It's too late to update the board file because the
information has already been written to various firmwares.  The
enhanced ehci-platform would have to include a special entry to match
the controller.

Since this reasoning applies every time ehci-platform is updated, it 
seems reasonable to use the same approach right from the beginning.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Florian Fainelli wrote:

> As long as no one enables both ehci-platform and ehci-ppc-of at the same time
> there is no problem. ehci-ppc-of should be removed in favor of ehci-platform
> and make sure that the specific quirk in ehci-ppc-of also gets ported, other 
> that I see no issue using "usb-ehci" as the least detailed compatible property
> name.

Suppose a DT board file is created for a oontroller which ehci-platform
can't handle.  Then by your proposal, the board file shouldn't have
"usb-ehci" in its compatible property.

Now later on, suppose ehci-platform is enhanced so that it can manage
that controller.  It's too late to update the board file because the
information has already been written to various firmwares.  The
enhanced ehci-platform would have to include a special entry to match
the controller.

Since this reasoning applies every time ehci-platform is updated, it 
seems reasonable to use the same approach right from the beginning.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/24/2012 11:46 AM, Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
>>> How do we determine which existing drivers claim to support usb-ehci?  
>>> A quick search under arch/ and drivers/ turns up nothing but
>>> drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
>>> match should be easy enough, and then "usb-ehci" would be safe to use
>>> in ehci-platform.c.
>>
>> It's not drivers that claim to support usb-ehci, but device tree files
>> that contain nodes that include "usb-ehci" in their compatible value,
>> yet need to be handled by a driver that isn't the generic USB EHCI
>> driver, and that driver binds to the other more specific compatible value:
>>
>>> $ grep -HrnI usb-ehci arch/*/boot/dts
...
>> and then search for all those other compatible values in drivers. The
>> ARM instances certainly all have this issue (although perhaps it's worth
>> investigating if a generic could work for them instead). The PPC
>> instances need more investigation; the values don't show up in of match
>> tables but rather in code.
> 
> Ah, now I'm starting to get the picture.
> 
> So by listing "usb-ehci" in its device ID table, a driver would
> essentially be saying that it can handle _all_ USB EHCI controllers.  

Yes, exactly.

> (Which means that the entry in ehci-ppc-of.c is questionable; perhaps
> the intention is that this driver really does handle all EHCI
> controllers on any PPC-based OpenFirmware platform bus.)

Yes, that seems questionable, although perhaps within the context of
only enabling the driver on PPC specifically, it may be reasonable.

>> This doesn't continue forever though; you typically only list the
>> specific HW model, the base specific model it's compatible with, and any
>> generic value needed. So, a hypothetical Tegra40 EHCI controller would be:
>>
>> compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".
> 
> What's the reason for listing the generic value if drivers can't key
> off it?  Does it contain any information that's not already present in 
> the specific values?

This may or may not be a mistake.

The idea is that usb-ehci is included in the device node's compatible
list iff the HW is 100% compatible with a "usb-ehci" HW device, and
hence a pure usb-ehci driver can handle the hardware without any
additional knowledge. (That's true in general for any compatible value).

It's quite possible that this often gets translated to the subtly
different "the HW is an EHCI controller, so it gets
compatible="usb-ehci"" when writing .dts files.

So yes we probably should remove compatible="usb-ehci" from any device
node for HW which really isn't a pure EHCI device. That would presumably
require looking at the existing custom drivers and/or HW specs to
determine what, if anything, is different about the HW.

Related, at least on Tegra, the PHY handling is IIRC pretty tightly
coupled into the EHCI driver. We probably should have separate nodes
for, and drivers for, the PHYs instead. I don't know if that would
remove all the non-standard attributes of the Tegra EHCI HW and hence
allow Tegra's EHCI to be handled by the generic driver. From memory,
there are certainly some HW workarounds in the Tegra EHCI driver that
would need to be ported though.

> It sounds like the ehci-platform driver should simply list all the
> specific HW device types (or base HW device types) that it handles, and
> it shouldn't include "usb-ehci" in the list.  As more DT board files
> are created or as ehci-platform becomes capable to taking over from
> other drivers, its device list would grow.

That's certainly a reasonable way to go too. I think the only downside
with that approach is that every new device needs a kernel update to add
it to the table, whereas using a generic compatible="usb-ehci" wouldn't,
assuming no quirks were required for the new device.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/24/2012 11:46 AM, Alan Stern wrote:
> On Wed, 24 Oct 2012, Stephen Warren wrote:
> 
>>> How do we determine which existing drivers claim to support usb-ehci?  
>>> A quick search under arch/ and drivers/ turns up nothing but
>>> drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
>>> match should be easy enough, and then "usb-ehci" would be safe to use
>>> in ehci-platform.c.
>>
>> It's not drivers that claim to support usb-ehci, but device tree files
>> that contain nodes that include "usb-ehci" in their compatible value,
>> yet need to be handled by a driver that isn't the generic USB EHCI
>> driver, and that driver binds to the other more specific compatible value:
>>
>>> $ grep -HrnI usb-ehci arch/*/boot/dts
...
>> and then search for all those other compatible values in drivers. The
>> ARM instances certainly all have this issue (although perhaps it's worth
>> investigating if a generic could work for them instead). The PPC
>> instances need more investigation; the values don't show up in of match
>> tables but rather in code.
> 
> Ah, now I'm starting to get the picture.
> 
> So by listing "usb-ehci" in its device ID table, a driver would
> essentially be saying that it can handle _all_ USB EHCI controllers.  

Yes, exactly.

> (Which means that the entry in ehci-ppc-of.c is questionable; perhaps
> the intention is that this driver really does handle all EHCI
> controllers on any PPC-based OpenFirmware platform bus.)

Yes, that seems questionable, although perhaps within the context of
only enabling the driver on PPC specifically, it may be reasonable.

>> This doesn't continue forever though; you typically only list the
>> specific HW model, the base specific model it's compatible with, and any
>> generic value needed. So, a hypothetical Tegra40 EHCI controller would be:
>>
>> compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".
> 
> What's the reason for listing the generic value if drivers can't key
> off it?  Does it contain any information that's not already present in 
> the specific values?

This may or may not be a mistake.

The idea is that usb-ehci is included in the device node's compatible
list iff the HW is 100% compatible with a "usb-ehci" HW device, and
hence a pure usb-ehci driver can handle the hardware without any
additional knowledge. (That's true in general for any compatible value).

It's quite possible that this often gets translated to the subtly
different "the HW is an EHCI controller, so it gets
compatible="usb-ehci"" when writing .dts files.

So yes we probably should remove compatible="usb-ehci" from any device
node for HW which really isn't a pure EHCI device. That would presumably
require looking at the existing custom drivers and/or HW specs to
determine what, if anything, is different about the HW.

Related, at least on Tegra, the PHY handling is IIRC pretty tightly
coupled into the EHCI driver. We probably should have separate nodes
for, and drivers for, the PHYs instead. I don't know if that would
remove all the non-standard attributes of the Tegra EHCI HW and hence
allow Tegra's EHCI to be handled by the generic driver. From memory,
there are certainly some HW workarounds in the Tegra EHCI driver that
would need to be ported though.

> It sounds like the ehci-platform driver should simply list all the
> specific HW device types (or base HW device types) that it handles, and
> it shouldn't include "usb-ehci" in the list.  As more DT board files
> are created or as ehci-platform becomes capable to taking over from
> other drivers, its device list would grow.

That's certainly a reasonable way to go too. I think the only downside
with that approach is that every new device needs a kernel update to add
it to the table, whereas using a generic compatible="usb-ehci" wouldn't,
assuming no quirks were required for the new device.

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Florian Fainelli]

On Wednesday 24 October 2012 14:04:12 Alan Stern wrote:
> On Wed, 24 Oct 2012, Florian Fainelli wrote:
> 
> > As long as no one enables both ehci-platform and ehci-ppc-of at the same time
> > there is no problem. ehci-ppc-of should be removed in favor of ehci-platform
> > and make sure that the specific quirk in ehci-ppc-of also gets ported, other 
> > that I see no issue using "usb-ehci" as the least detailed compatible property
> > name.
> 
> Suppose a DT board file is created for a oontroller which ehci-platform
> can't handle.  Then by your proposal, the board file shouldn't have
> "usb-ehci" in its compatible property.
> 
> Now later on, suppose ehci-platform is enhanced so that it can manage
> that controller.  It's too late to update the board file because the
> information has already been written to various firmwares.  The
> enhanced ehci-platform would have to include a special entry to match
> the controller.

In any case you are supposed to use a compatible property which describes
as much as possible your hardware, and this one should have the precedence
if a special treatment is required, so I see no problem with this approach.

> 
> Since this reasoning applies every time ehci-platform is updated, it 
> seems reasonable to use the same approach right from the beginning.
> 
> Alan Stern
> 
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Florian Fainelli]

On Wednesday 24 October 2012 14:04:12 Alan Stern wrote:
> On Wed, 24 Oct 2012, Florian Fainelli wrote:
> 
> > As long as no one enables both ehci-platform and ehci-ppc-of at the same time
> > there is no problem. ehci-ppc-of should be removed in favor of ehci-platform
> > and make sure that the specific quirk in ehci-ppc-of also gets ported, other 
> > that I see no issue using "usb-ehci" as the least detailed compatible property
> > name.
> 
> Suppose a DT board file is created for a oontroller which ehci-platform
> can't handle.  Then by your proposal, the board file shouldn't have
> "usb-ehci" in its compatible property.
> 
> Now later on, suppose ehci-platform is enhanced so that it can manage
> that controller.  It's too late to update the board file because the
> information has already been written to various firmwares.  The
> enhanced ehci-platform would have to include a special entry to match
> the controller.

In any case you are supposed to use a compatible property which describes
as much as possible your hardware, and this one should have the precedence
if a special treatment is required, so I see no problem with this approach.

> 
> Since this reasoning applies every time ehci-platform is updated, it 
> seems reasonable to use the same approach right from the beginning.
> 
> Alan Stern
> 
-- 
Florian

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Mitch Bradley]

On 10/24/2012 8:09 AM, Stephen Warren wrote:
> On 10/24/2012 11:46 AM, Alan Stern wrote:
>> On Wed, 24 Oct 2012, Stephen Warren wrote:
>>
>>>> How do we determine which existing drivers claim to support usb-ehci?  
>>>> A quick search under arch/ and drivers/ turns up nothing but
>>>> drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
>>>> match should be easy enough, and then "usb-ehci" would be safe to use
>>>> in ehci-platform.c.
>>>
>>> It's not drivers that claim to support usb-ehci, but device tree files
>>> that contain nodes that include "usb-ehci" in their compatible value,
>>> yet need to be handled by a driver that isn't the generic USB EHCI
>>> driver, and that driver binds to the other more specific compatible value:
>>>
>>>> $ grep -HrnI usb-ehci arch/*/boot/dts
> ...
>>> and then search for all those other compatible values in drivers. The
>>> ARM instances certainly all have this issue (although perhaps it's worth
>>> investigating if a generic could work for them instead). The PPC
>>> instances need more investigation; the values don't show up in of match
>>> tables but rather in code.
>>
>> Ah, now I'm starting to get the picture.
>>
>> So by listing "usb-ehci" in its device ID table, a driver would
>> essentially be saying that it can handle _all_ USB EHCI controllers.  


Actually, it means that the driver can handle at least USB EHCI
controllers that are 100% compatible with the EHCI spec (requiring
nothing extra).  The driver might be able to handle almost-compatible
controllers, possibly with the help of additional properties.

If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that
a given version of that driver will work, but it's worth a try in the
event that no more-specific driver is matched.


> 
> Yes, exactly.
> 
>> (Which means that the entry in ehci-ppc-of.c is questionable; perhaps
>> the intention is that this driver really does handle all EHCI
>> controllers on any PPC-based OpenFirmware platform bus.)
> 
> Yes, that seems questionable, although perhaps within the context of
> only enabling the driver on PPC specifically, it may be reasonable.
> 
>>> This doesn't continue forever though; you typically only list the
>>> specific HW model, the base specific model it's compatible with, and any
>>> generic value needed. So, a hypothetical Tegra40 EHCI controller would be:
>>>
>>> compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".
>>
>> What's the reason for listing the generic value if drivers can't key
>> off it?  Does it contain any information that's not already present in 
>> the specific values?
> 
> This may or may not be a mistake.
> 
> The idea is that usb-ehci is included in the device node's compatible
> list iff the HW is 100% compatible with a "usb-ehci" HW device, and
> hence a pure usb-ehci driver can handle the hardware without any
> additional knowledge. (That's true in general for any compatible value).
> 
> It's quite possible that this often gets translated to the subtly
> different "the HW is an EHCI controller, so it gets
> compatible="usb-ehci"" when writing .dts files.
> 
> So yes we probably should remove compatible="usb-ehci" from any device
> node for HW which really isn't a pure EHCI device. That would presumably
> require looking at the existing custom drivers and/or HW specs to
> determine what, if anything, is different about the HW.
> 
> Related, at least on Tegra, the PHY handling is IIRC pretty tightly
> coupled into the EHCI driver. We probably should have separate nodes
> for, and drivers for, the PHYs instead. I don't know if that would
> remove all the non-standard attributes of the Tegra EHCI HW and hence
> allow Tegra's EHCI to be handled by the generic driver. From memory,
> there are certainly some HW workarounds in the Tegra EHCI driver that
> would need to be ported though.
> 
>> It sounds like the ehci-platform driver should simply list all the
>> specific HW device types (or base HW device types) that it handles, and
>> it shouldn't include "usb-ehci" in the list.  As more DT board files
>> are created or as ehci-platform becomes capable to taking over from
>> other drivers, its device list would grow.
> 
> That's certainly a reasonable way to go too. I think the only downside
> with that approach is that every new device needs a kernel update to add
> it to the table, whereas using a generic compatible="usb-ehci" wouldn't,
> assuming no quirks were required for the new device.
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Mitch Bradley]

On 10/24/2012 8:09 AM, Stephen Warren wrote:
> On 10/24/2012 11:46 AM, Alan Stern wrote:
>> On Wed, 24 Oct 2012, Stephen Warren wrote:
>>
>>>> How do we determine which existing drivers claim to support usb-ehci?  
>>>> A quick search under arch/ and drivers/ turns up nothing but
>>>> drivers/usb/host/ehci-ppc-of.c.  Changing it to a more HW-specific
>>>> match should be easy enough, and then "usb-ehci" would be safe to use
>>>> in ehci-platform.c.
>>>
>>> It's not drivers that claim to support usb-ehci, but device tree files
>>> that contain nodes that include "usb-ehci" in their compatible value,
>>> yet need to be handled by a driver that isn't the generic USB EHCI
>>> driver, and that driver binds to the other more specific compatible value:
>>>
>>>> $ grep -HrnI usb-ehci arch/*/boot/dts
> ...
>>> and then search for all those other compatible values in drivers. The
>>> ARM instances certainly all have this issue (although perhaps it's worth
>>> investigating if a generic could work for them instead). The PPC
>>> instances need more investigation; the values don't show up in of match
>>> tables but rather in code.
>>
>> Ah, now I'm starting to get the picture.
>>
>> So by listing "usb-ehci" in its device ID table, a driver would
>> essentially be saying that it can handle _all_ USB EHCI controllers.  


Actually, it means that the driver can handle at least USB EHCI
controllers that are 100% compatible with the EHCI spec (requiring
nothing extra).  The driver might be able to handle almost-compatible
controllers, possibly with the help of additional properties.

If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that
a given version of that driver will work, but it's worth a try in the
event that no more-specific driver is matched.


> 
> Yes, exactly.
> 
>> (Which means that the entry in ehci-ppc-of.c is questionable; perhaps
>> the intention is that this driver really does handle all EHCI
>> controllers on any PPC-based OpenFirmware platform bus.)
> 
> Yes, that seems questionable, although perhaps within the context of
> only enabling the driver on PPC specifically, it may be reasonable.
> 
>>> This doesn't continue forever though; you typically only list the
>>> specific HW model, the base specific model it's compatible with, and any
>>> generic value needed. So, a hypothetical Tegra40 EHCI controller would be:
>>>
>>> compatible = "nvidia,tegra40-ehci", "nvidia,tegra20-ehci", "usb-ehci".
>>
>> What's the reason for listing the generic value if drivers can't key
>> off it?  Does it contain any information that's not already present in 
>> the specific values?
> 
> This may or may not be a mistake.
> 
> The idea is that usb-ehci is included in the device node's compatible
> list iff the HW is 100% compatible with a "usb-ehci" HW device, and
> hence a pure usb-ehci driver can handle the hardware without any
> additional knowledge. (That's true in general for any compatible value).
> 
> It's quite possible that this often gets translated to the subtly
> different "the HW is an EHCI controller, so it gets
> compatible="usb-ehci"" when writing .dts files.
> 
> So yes we probably should remove compatible="usb-ehci" from any device
> node for HW which really isn't a pure EHCI device. That would presumably
> require looking at the existing custom drivers and/or HW specs to
> determine what, if anything, is different about the HW.
> 
> Related, at least on Tegra, the PHY handling is IIRC pretty tightly
> coupled into the EHCI driver. We probably should have separate nodes
> for, and drivers for, the PHYs instead. I don't know if that would
> remove all the non-standard attributes of the Tegra EHCI HW and hence
> allow Tegra's EHCI to be handled by the generic driver. From memory,
> there are certainly some HW workarounds in the Tegra EHCI driver that
> would need to be ported though.
> 
>> It sounds like the ehci-platform driver should simply list all the
>> specific HW device types (or base HW device types) that it handles, and
>> it shouldn't include "usb-ehci" in the list.  As more DT board files
>> are created or as ehci-platform becomes capable to taking over from
>> other drivers, its device list would grow.
> 
> That's certainly a reasonable way to go too. I think the only downside
> with that approach is that every new device needs a kernel update to add
> it to the table, whereas using a generic compatible="usb-ehci" wouldn't,
> assuming no quirks were required for the new device.
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Mitch Bradley wrote:

> >> Ah, now I'm starting to get the picture.
> >>
> >> So by listing "usb-ehci" in its device ID table, a driver would
> >> essentially be saying that it can handle _all_ USB EHCI controllers.  
> 
> 
> Actually, it means that the driver can handle at least USB EHCI
> controllers that are 100% compatible with the EHCI spec (requiring
> nothing extra).  The driver might be able to handle almost-compatible
> controllers, possibly with the help of additional properties.

Unfortunately that's not saying very much.  The EHCI spec describes
only PCI-based controllers.  Every other sort does require something
extra.

> If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that
> a given version of that driver will work, but it's worth a try in the
> event that no more-specific driver is matched.

While I haven't checked in detail, it seems quite likely that this 
would not work with some of the devices that have "usb-ehci" listed in 
their compatible values.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Mitch Bradley wrote:

> >> Ah, now I'm starting to get the picture.
> >>
> >> So by listing "usb-ehci" in its device ID table, a driver would
> >> essentially be saying that it can handle _all_ USB EHCI controllers.  
> 
> 
> Actually, it means that the driver can handle at least USB EHCI
> controllers that are 100% compatible with the EHCI spec (requiring
> nothing extra).  The driver might be able to handle almost-compatible
> controllers, possibly with the help of additional properties.

Unfortunately that's not saying very much.  The EHCI spec describes
only PCI-based controllers.  Every other sort does require something
extra.

> If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that
> a given version of that driver will work, but it's worth a try in the
> event that no more-specific driver is matched.

While I haven't checked in detail, it seems quite likely that this 
would not work with some of the devices that have "usb-ehci" listed in 
their compatible values.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Stephen Warren wrote:

> > What's the reason for listing the generic value if drivers can't key
> > off it?  Does it contain any information that's not already present in 
> > the specific values?
> 
> This may or may not be a mistake.
> 
> The idea is that usb-ehci is included in the device node's compatible
> list iff the HW is 100% compatible with a "usb-ehci" HW device, and

But there is no such thing as a "usb-ehci" HW device.  That's part of 
the reason this whole discussion got started.

> hence a pure usb-ehci driver can handle the hardware without any
> additional knowledge. (That's true in general for any compatible value).

To as great an extent as is reasonable, ehci-platform is supposed to 
be that "pure usb-ehci driver".

> It's quite possible that this often gets translated to the subtly
> different "the HW is an EHCI controller, so it gets
> compatible="usb-ehci"" when writing .dts files.
> 
> So yes we probably should remove compatible="usb-ehci" from any device
> node for HW which really isn't a pure EHCI device. That would presumably
> require looking at the existing custom drivers and/or HW specs to
> determine what, if anything, is different about the HW.

Of course, removing it from the board files in the new kernel won't
have any effect on old board files embedded in firmware.  Those old 
firmwares then might not work with newer kernels.

> Related, at least on Tegra, the PHY handling is IIRC pretty tightly
> coupled into the EHCI driver. We probably should have separate nodes
> for, and drivers for, the PHYs instead. I don't know if that would
> remove all the non-standard attributes of the Tegra EHCI HW and hence
> allow Tegra's EHCI to be handled by the generic driver. From memory,
> there are certainly some HW workarounds in the Tegra EHCI driver that
> would need to be ported though.

In fact, ehci-tegra is probably the _most_ non-standard of the EHCI 
drivers.

> > It sounds like the ehci-platform driver should simply list all the
> > specific HW device types (or base HW device types) that it handles, and
> > it shouldn't include "usb-ehci" in the list.  As more DT board files
> > are created or as ehci-platform becomes capable to taking over from
> > other drivers, its device list would grow.
> 
> That's certainly a reasonable way to go too. I think the only downside
> with that approach is that every new device needs a kernel update to add
> it to the table, whereas using a generic compatible="usb-ehci" wouldn't,
> assuming no quirks were required for the new device.

New devices could list an older device they are upwardly compatible 
with.  Then no kernel update would be needed.  Now, I agree that a 
generic entry would be more logical, but there's no generic "standard" 
hardware for it to describe.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Wed, 24 Oct 2012, Stephen Warren wrote:

> > What's the reason for listing the generic value if drivers can't key
> > off it?  Does it contain any information that's not already present in 
> > the specific values?
> 
> This may or may not be a mistake.
> 
> The idea is that usb-ehci is included in the device node's compatible
> list iff the HW is 100% compatible with a "usb-ehci" HW device, and

But there is no such thing as a "usb-ehci" HW device.  That's part of 
the reason this whole discussion got started.

> hence a pure usb-ehci driver can handle the hardware without any
> additional knowledge. (That's true in general for any compatible value).

To as great an extent as is reasonable, ehci-platform is supposed to 
be that "pure usb-ehci driver".

> It's quite possible that this often gets translated to the subtly
> different "the HW is an EHCI controller, so it gets
> compatible="usb-ehci"" when writing .dts files.
> 
> So yes we probably should remove compatible="usb-ehci" from any device
> node for HW which really isn't a pure EHCI device. That would presumably
> require looking at the existing custom drivers and/or HW specs to
> determine what, if anything, is different about the HW.

Of course, removing it from the board files in the new kernel won't
have any effect on old board files embedded in firmware.  Those old 
firmwares then might not work with newer kernels.

> Related, at least on Tegra, the PHY handling is IIRC pretty tightly
> coupled into the EHCI driver. We probably should have separate nodes
> for, and drivers for, the PHYs instead. I don't know if that would
> remove all the non-standard attributes of the Tegra EHCI HW and hence
> allow Tegra's EHCI to be handled by the generic driver. From memory,
> there are certainly some HW workarounds in the Tegra EHCI driver that
> would need to be ported though.

In fact, ehci-tegra is probably the _most_ non-standard of the EHCI 
drivers.

> > It sounds like the ehci-platform driver should simply list all the
> > specific HW device types (or base HW device types) that it handles, and
> > it shouldn't include "usb-ehci" in the list.  As more DT board files
> > are created or as ehci-platform becomes capable to taking over from
> > other drivers, its device list would grow.
> 
> That's certainly a reasonable way to go too. I think the only downside
> with that approach is that every new device needs a kernel update to add
> it to the table, whereas using a generic compatible="usb-ehci" wouldn't,
> assuming no quirks were required for the new device.

New devices could list an older device they are upwardly compatible 
with.  Then no kernel update would be needed.  Now, I agree that a 
generic entry would be more logical, but there's no generic "standard" 
hardware for it to describe.

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Sebastian Andrzej Siewior]

On Wed, Oct 24, 2012 at 08:55:27AM -1000, Mitch Bradley wrote:
> >> So by listing "usb-ehci" in its device ID table, a driver would
> >> essentially be saying that it can handle _all_ USB EHCI controllers.  
> 
> 
> Actually, it means that the driver can handle at least USB EHCI
> controllers that are 100% compatible with the EHCI spec (requiring
> nothing extra).  The driver might be able to handle almost-compatible
> controllers, possibly with the help of additional properties.
> 
> If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that
> a given version of that driver will work, but it's worth a try in the
> event that no more-specific driver is matched.

Not sure fallback is a good term here. The of parses the compatible from left
to right. If the device specific entry is not found (in the driver) then end
up with usb-ehci. If we need a quirk later on we add the device specific entry
to the driver (which will match before usb-ehci is found) and we could use
this entry to apply the quirk. That way you can apply quirks without touch the
firmware / device tree.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Sebastian Andrzej Siewior]

On Wed, Oct 24, 2012 at 08:55:27AM -1000, Mitch Bradley wrote:
> >> So by listing "usb-ehci" in its device ID table, a driver would
> >> essentially be saying that it can handle _all_ USB EHCI controllers.  
> 
> 
> Actually, it means that the driver can handle at least USB EHCI
> controllers that are 100% compatible with the EHCI spec (requiring
> nothing extra).  The driver might be able to handle almost-compatible
> controllers, possibly with the help of additional properties.
> 
> If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that
> a given version of that driver will work, but it's worth a try in the
> event that no more-specific driver is matched.

Not sure fallback is a good term here. The of parses the compatible from left
to right. If the device specific entry is not found (in the driver) then end
up with usb-ehci. If we need a quirk later on we add the device specific entry
to the driver (which will match before usb-ehci is found) and we could use
this entry to apply the quirk. That way you can apply quirks without touch the
firmware / device tree.

Sebastian

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Thu, 25 Oct 2012, Sebastian Andrzej Siewior wrote:

> On Wed, Oct 24, 2012 at 08:55:27AM -1000, Mitch Bradley wrote:
> > >> So by listing "usb-ehci" in its device ID table, a driver would
> > >> essentially be saying that it can handle _all_ USB EHCI controllers.  
> > 
> > 
> > Actually, it means that the driver can handle at least USB EHCI
> > controllers that are 100% compatible with the EHCI spec (requiring
> > nothing extra).  The driver might be able to handle almost-compatible
> > controllers, possibly with the help of additional properties.
> > 
> > If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that
> > a given version of that driver will work, but it's worth a try in the
> > event that no more-specific driver is matched.
> 
> Not sure fallback is a good term here. The of parses the compatible from left
> to right. If the device specific entry is not found (in the driver) then end
> up with usb-ehci. If we need a quirk later on we add the device specific entry
> to the driver (which will match before usb-ehci is found) and we could use
> this entry to apply the quirk. That way you can apply quirks without touch the
> firmware / device tree.

What happens if the drivers get probed in the wrong order?  That is, if 
ehci-platform gets probed before ehci-spear (or whatever)?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Thu, 25 Oct 2012, Sebastian Andrzej Siewior wrote:

> On Wed, Oct 24, 2012 at 08:55:27AM -1000, Mitch Bradley wrote:
> > >> So by listing "usb-ehci" in its device ID table, a driver would
> > >> essentially be saying that it can handle _all_ USB EHCI controllers.  
> > 
> > 
> > Actually, it means that the driver can handle at least USB EHCI
> > controllers that are 100% compatible with the EHCI spec (requiring
> > nothing extra).  The driver might be able to handle almost-compatible
> > controllers, possibly with the help of additional properties.
> > 
> > If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that
> > a given version of that driver will work, but it's worth a try in the
> > event that no more-specific driver is matched.
> 
> Not sure fallback is a good term here. The of parses the compatible from left
> to right. If the device specific entry is not found (in the driver) then end
> up with usb-ehci. If we need a quirk later on we add the device specific entry
> to the driver (which will match before usb-ehci is found) and we could use
> this entry to apply the quirk. That way you can apply quirks without touch the
> firmware / device tree.

What happens if the drivers get probed in the wrong order?  That is, if 
ehci-platform gets probed before ehci-spear (or whatever)?

Alan Stern

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/25/2012 04:23 AM, Sebastian Andrzej Siewior wrote:
> On Wed, Oct 24, 2012 at 08:55:27AM -1000, Mitch Bradley wrote:
>>>> So by listing "usb-ehci" in its device ID table, a driver would
>>>> essentially be saying that it can handle _all_ USB EHCI controllers.  
>>
>>
>> Actually, it means that the driver can handle at least USB EHCI
>> controllers that are 100% compatible with the EHCI spec (requiring
>> nothing extra).  The driver might be able to handle almost-compatible
>> controllers, possibly with the help of additional properties.
>>
>> If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that
>> a given version of that driver will work, but it's worth a try in the
>> event that no more-specific driver is matched.
> 
> Not sure fallback is a good term here. The of parses the compatible from left
> to right. If the device specific entry is not found (in the driver) then end
> up with usb-ehci. If we need a quirk later on we add the device specific entry
> to the driver (which will match before usb-ehci is found) and we could use
> this entry to apply the quirk. That way you can apply quirks without touch the
> firmware / device tree.

That's the theory on how the compatible values are used, but in
practice, each driver is tested against all the values in a node before
moving on to the next driver, so the (current kernel's implementation of
the) matching order is not what's needed to make that work.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Stephen Warren]

On 10/25/2012 04:23 AM, Sebastian Andrzej Siewior wrote:
> On Wed, Oct 24, 2012 at 08:55:27AM -1000, Mitch Bradley wrote:
>>>> So by listing "usb-ehci" in its device ID table, a driver would
>>>> essentially be saying that it can handle _all_ USB EHCI controllers.  
>>
>>
>> Actually, it means that the driver can handle at least USB EHCI
>> controllers that are 100% compatible with the EHCI spec (requiring
>> nothing extra).  The driver might be able to handle almost-compatible
>> controllers, possibly with the help of additional properties.
>>
>> If a DT node lists "usb-ehci" as a "fallback", it's not guaranteed that
>> a given version of that driver will work, but it's worth a try in the
>> event that no more-specific driver is matched.
> 
> Not sure fallback is a good term here. The of parses the compatible from left
> to right. If the device specific entry is not found (in the driver) then end
> up with usb-ehci. If we need a quirk later on we add the device specific entry
> to the driver (which will match before usb-ehci is found) and we could use
> this entry to apply the quirk. That way you can apply quirks without touch the
> firmware / device tree.

That's the theory on how the compatible values are used, but in
practice, each driver is tested against all the values in a node before
moving on to the next driver, so the (current kernel's implementation of
the) matching order is not what's needed to make that work.

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Sebastian Andrzej Siewior]

On Thu, Oct 25, 2012 at 10:36:14AM -0400, Alan Stern wrote:
> What happens if the drivers get probed in the wrong order?  That is, if 
> ehci-platform gets probed before ehci-spear (or whatever)?

The "wrong" driver may get loaded. Right now, you should have them all in
one driver. For instance the crypto node in mpc8315erdb.dts:
|   crypto@30000 {
|           compatible = "fsl,sec3.3", "fsl,sec3.1", "fsl,sec3.0",
|                        "fsl,sec2.4", "fsl,sec2.2", "fsl,sec2.1",
|                        "fsl,sec2.0";
…

The higher the version, the more features are available by the hardware.
The driver [0] probes only for "fsl,sec2.0" but it uses
of_device_is_compatible() to check for the other entries. You could also add
all 7 compatible entries to the driver and distinguish them by the driver_data
field. This is an implementation detail.
However, having two drivers, one for "fsl,sec3.3" and one for "fsl,sec2.0",
would "randomly" load one of the two driver depending on link order and so on.

[0] drivers/crypto/talitos.c

> 
> Alan Stern
> 

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Sebastian Andrzej Siewior]

On Thu, Oct 25, 2012 at 10:36:14AM -0400, Alan Stern wrote:
> What happens if the drivers get probed in the wrong order?  That is, if 
> ehci-platform gets probed before ehci-spear (or whatever)?

The "wrong" driver may get loaded. Right now, you should have them all in
one driver. For instance the crypto node in mpc8315erdb.dts:
|   crypto at 30000 {
|           compatible = "fsl,sec3.3", "fsl,sec3.1", "fsl,sec3.0",
|                        "fsl,sec2.4", "fsl,sec2.2", "fsl,sec2.1",
|                        "fsl,sec2.0";
?

The higher the version, the more features are available by the hardware.
The driver [0] probes only for "fsl,sec2.0" but it uses
of_device_is_compatible() to check for the other entries. You could also add
all 7 compatible entries to the driver and distinguish them by the driver_data
field. This is an implementation detail.
However, having two drivers, one for "fsl,sec3.3" and one for "fsl,sec2.0",
would "randomly" load one of the two driver depending on link order and so on.

[0] drivers/crypto/talitos.c

> 
> Alan Stern
> 

Sebastian

Re: [PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Fri, 26 Oct 2012, Sebastian Andrzej Siewior wrote:

> On Thu, Oct 25, 2012 at 10:36:14AM -0400, Alan Stern wrote:
> > What happens if the drivers get probed in the wrong order?  That is, if 
> > ehci-platform gets probed before ehci-spear (or whatever)?
> 
> The "wrong" driver may get loaded.

That's my point.  That's why ehci-platform.c should not claim to match 
all devices listing "usb-ehci" in their compatible property.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] USB: doc: Binding document for ehci-platform driver

[Alan Stern]

On Fri, 26 Oct 2012, Sebastian Andrzej Siewior wrote:

> On Thu, Oct 25, 2012 at 10:36:14AM -0400, Alan Stern wrote:
> > What happens if the drivers get probed in the wrong order?  That is, if 
> > ehci-platform gets probed before ehci-spear (or whatever)?
> 
> The "wrong" driver may get loaded.

That's my point.  That's why ehci-platform.c should not claim to match 
all devices listing "usb-ehci" in their compatible property.

Alan Stern