[PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Hans de Goede]

Hi All,

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
this version should be 100% ready for merging upstream.

Changes since v3:
-Address all of Alan Stern's review comments
-Properly use put_clk in probe error path
-Don't do hcd_put until after power_off as power_off may use private hcd data

Regards,

Hans

[PATCH v4 1/2] ohci-platform: Add support for devicetree instantiation

[Hans de Goede]

Add support for ohci-platform instantiation from devicetree, including
optionally getting clks and a phy from devicetree, and enabling / disabling
those on power_on / off.

This should allow using ohci-platform from devicetree in various cases.
Specifically after this commit it can be used for the ohci controller found
on Allwinner sunxi SoCs.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/usb/mmio-ohci.txt          |  22 +++
 drivers/usb/host/ohci-platform.c                   | 164 ++++++++++++++++++---
 2 files changed, 162 insertions(+), 24 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/mmio-ohci.txt

diff --git a/Documentation/devicetree/bindings/usb/mmio-ohci.txt b/Documentation/devicetree/bindings/usb/mmio-ohci.txt
new file mode 100644
index 0000000..9c776ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/mmio-ohci.txt
@@ -0,0 +1,22 @@
+Generic MMIO OHCI controller
+
+Required properties:
+- compatible : "mmio-ohci"
+- reg : ohci controller register range (address and length)
+- interrupts : ohci controller interrupt
+
+Optional properties:
+- clocks : a list of phandle + clock specifier pairs
+- phys : phandle + phy specifier pair
+- phy-names : "usb"
+
+Example:
+
+	ohci0: ohci@0x01c14400 {
+		compatible = "mmio-ohci";
+		reg = <0x01c14400 0x100>;
+		interrupts = <64>;
+		clocks = <&usb_clk 6>, <&ahb_gates 2>;
+		phys = <&usbphy 1>;
+		phy-names = "usb";
+	};
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index f351ff5..31d9bdc 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -3,6 +3,7 @@
  *
  * Copyright 2007 Michael Buesch <m@bues.ch>
  * Copyright 2011-2012 Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org>
+ * Copyright 2014 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  *
  * Derived from the OCHI-SSB driver
  * Derived from the OHCI-PCI driver
@@ -14,11 +15,14 @@
  * Licensed under the GNU/GPL. See COPYING for details.
  */
 
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
 #include <linux/hrtimer.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/err.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/usb/ohci_pdriver.h>
 #include <linux/usb.h>
@@ -27,6 +31,13 @@
 #include "ohci.h"
 
 #define DRIVER_DESC "OHCI generic platform driver"
+#define OHCI_MAX_CLKS 3
+#define hcd_to_ohci_priv(h) ((struct ohci_platform_priv *)hcd_to_ohci(h)->priv)
+
+struct ohci_platform_priv {
+	struct clk *clks[OHCI_MAX_CLKS];
+	struct phy *phy;
+};
 
 static const char hcd_name[] = "ohci-platform";
 
@@ -48,11 +59,67 @@ static int ohci_platform_reset(struct usb_hcd *hcd)
 	return ohci_setup(hcd);
 }
 
+static int ohci_platform_power_on(struct platform_device *dev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(dev);
+	struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
+	int clk, ret;
+
+	for (clk = 0; priv->clks[clk] && clk < OHCI_MAX_CLKS; clk++) {
+		ret = clk_prepare_enable(priv->clks[clk]);
+		if (ret)
+			goto err_disable_clks;
+	}
+
+	if (priv->phy) {
+		ret = phy_init(priv->phy);
+		if (ret)
+			goto err_disable_clks;
+
+		ret = phy_power_on(priv->phy);
+		if (ret)
+			goto err_exit_phy;
+	}
+
+	return 0;
+
+err_exit_phy:
+	phy_exit(priv->phy);
+err_disable_clks:
+	while (--clk >= 0)
+		clk_disable_unprepare(priv->clks[clk]);
+
+	return ret;
+}
+
+static void ohci_platform_power_off(struct platform_device *dev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(dev);
+	struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
+	int clk;
+
+	if (priv->phy) {
+		phy_power_off(priv->phy);
+		phy_exit(priv->phy);
+	}
+
+	for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--)
+		if (priv->clks[clk])
+			clk_disable_unprepare(priv->clks[clk]);
+}
+
 static struct hc_driver __read_mostly ohci_platform_hc_driver;
 
 static const struct ohci_driver_overrides platform_overrides __initconst = {
-	.product_desc =	"Generic Platform OHCI controller",
-	.reset =	ohci_platform_reset,
+	.product_desc =		"Generic Platform OHCI controller",
+	.reset =		ohci_platform_reset,
+	.extra_priv_size =	sizeof(struct ohci_platform_priv),
+};
+
+static struct usb_ohci_pdata ohci_platform_defaults = {
+	.power_on =		ohci_platform_power_on,
+	.power_suspend =	ohci_platform_power_off,
+	.power_off =		ohci_platform_power_off,
 };
 
 static int ohci_platform_probe(struct platform_device *dev)
@@ -60,17 +127,23 @@ static int ohci_platform_probe(struct platform_device *dev)
 	struct usb_hcd *hcd;
 	struct resource *res_mem;
 	struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev);
-	int irq;
-	int err = -ENOMEM;
-
-	if (!pdata) {
-		WARN_ON(1);
-		return -ENODEV;
-	}
+	struct ohci_platform_priv *priv;
+	int clk, irq, err;
 
 	if (usb_disabled())
 		return -ENODEV;
 
+	/*
+	 * Use reasonable defaults so platforms don't have to provide these
+	 * with DT probing on ARM.
+	 */
+	if (!pdata)
+		pdata = &ohci_platform_defaults;
+
+	err = dma_coerce_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32));
+	if (err)
+		return err;
+
 	irq = platform_get_irq(dev, 0);
 	if (irq < 0) {
 		dev_err(&dev->dev, "no irq provided");
@@ -83,17 +156,40 @@ static int ohci_platform_probe(struct platform_device *dev)
 		return -ENXIO;
 	}
 
+	hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev,
+			dev_name(&dev->dev));
+	if (!hcd)
+		return -ENOMEM;
+
+	platform_set_drvdata(dev, hcd);
+	dev->dev.platform_data = pdata;
+	priv = hcd_to_ohci_priv(hcd);
+
+	if (pdata == &ohci_platform_defaults && dev->dev.of_node) {
+		priv->phy = devm_phy_get(&dev->dev, "usb");
+		if (IS_ERR(priv->phy)) {
+			err = PTR_ERR(priv->phy);
+			if (err == -EPROBE_DEFER)
+				goto err_put_hcd;
+			priv->phy = NULL;
+		}
+
+		for (clk = 0; clk < OHCI_MAX_CLKS; clk++) {
+			priv->clks[clk] = of_clk_get(dev->dev.of_node, clk);
+			if (IS_ERR(priv->clks[clk])) {
+				err = PTR_ERR(priv->clks[clk]);
+				if (err == -EPROBE_DEFER)
+					goto err_put_clks;
+				priv->clks[clk] = NULL;
+				break;
+			}
+		}
+	}
+
 	if (pdata->power_on) {
 		err = pdata->power_on(dev);
 		if (err < 0)
-			return err;
-	}
-
-	hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev,
-			dev_name(&dev->dev));
-	if (!hcd) {
-		err = -ENOMEM;
-		goto err_power;
+			goto err_put_clks;
 	}
 
 	hcd->rsrc_start = res_mem->start;
@@ -102,21 +198,25 @@ static int ohci_platform_probe(struct platform_device *dev)
 	hcd->regs = devm_ioremap_resource(&dev->dev, res_mem);
 	if (IS_ERR(hcd->regs)) {
 		err = PTR_ERR(hcd->regs);
-		goto err_put_hcd;
+		goto err_power;
 	}
 	err = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (err)
-		goto err_put_hcd;
-
-	platform_set_drvdata(dev, hcd);
+		goto err_power;
 
 	return err;
 
-err_put_hcd:
-	usb_put_hcd(hcd);
 err_power:
 	if (pdata->power_off)
 		pdata->power_off(dev);
+err_put_clks:
+	while (--clk >= 0)
+		clk_put(priv->clks[clk]);
+err_put_hcd:
+	if (pdata == &ohci_platform_defaults)
+		dev->dev.platform_data = NULL;
+
+	usb_put_hcd(hcd);
 
 	return err;
 }
@@ -125,13 +225,22 @@ static int ohci_platform_remove(struct platform_device *dev)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
 	struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev);
+	struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
+	int clk;
 
 	usb_remove_hcd(hcd);
-	usb_put_hcd(hcd);
 
 	if (pdata->power_off)
 		pdata->power_off(dev);
 
+	for (clk = 0; priv->clks[clk] && clk < OHCI_MAX_CLKS; clk++)
+		clk_put(priv->clks[clk]);
+
+	usb_put_hcd(hcd);
+
+	if (pdata == &ohci_platform_defaults)
+		dev->dev.platform_data = NULL;
+
 	return 0;
 }
 
@@ -178,6 +287,12 @@ static int ohci_platform_resume(struct device *dev)
 #define ohci_platform_resume	NULL
 #endif /* CONFIG_PM */
 
+static const struct of_device_id ohci_platform_ids[] = {
+	{ .compatible = "mmio-ohci", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ohci_platform_ids);
+
 static const struct platform_device_id ohci_platform_table[] = {
 	{ "ohci-platform", 0 },
 	{ }
@@ -198,6 +313,7 @@ static struct platform_driver ohci_platform_driver = {
 		.owner	= THIS_MODULE,
 		.name	= "ohci-platform",
 		.pm	= &ohci_platform_pm_ops,
+		.of_match_table = ohci_platform_ids,
 	}
 };
 
-- 
1.8.4.2

[PATCH v4 2/2] ehci-platform: Add support for clks and phy passed through devicetree

[Hans de Goede]

Currently ehci-platform is only used in combination with devicetree when used
with some Via socs. By extending it to (optionally) get clks and a phy from
devicetree, and enabling / disabling those on power_on / off, it can be used
more generically. Specifically after this commit it can be used for the
ehci controller on Allwinner sunxi SoCs.

Somehow we've ended up with 2 device-bindings documents for ehci-platform.c,
this patch renames and updates one to mmio-ehci.txt to reflect that this
is a generic ehci driver, and removes the other.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/usb/mmio-ehci.txt          |  22 +++
 .../devicetree/bindings/usb/via,vt8500-ehci.txt    |  15 ---
 .../devicetree/bindings/usb/vt8500-ehci.txt        |  12 --
 drivers/usb/host/ehci-platform.c                   | 149 +++++++++++++++++----
 4 files changed, 145 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/mmio-ehci.txt
 delete mode 100644 Documentation/devicetree/bindings/usb/via,vt8500-ehci.txt
 delete mode 100644 Documentation/devicetree/bindings/usb/vt8500-ehci.txt

diff --git a/Documentation/devicetree/bindings/usb/mmio-ehci.txt b/Documentation/devicetree/bindings/usb/mmio-ehci.txt
new file mode 100644
index 0000000..bc0f967
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/mmio-ehci.txt
@@ -0,0 +1,22 @@
+Generic MMIO EHCI controller
+
+Required properties:
+- compatible : "via,vt8500-ehci", "wm,prizm-ehci", or "mmio-ehci"
+- reg : ehci controller register range (address and length)
+- interrupts : ehci controller interrupt
+
+Optional properties:
+- clocks : a list of phandle + clock specifier pairs
+- phys : phandle + phy specifier pair
+- phy-names : "usb"
+
+Example:
+
+	ehci@d8007900 {
+		compatible = "via,vt8500-ehci", "mmio-ehci";
+		reg = <0xd8007900 0x200>;
+		interrupts = <43>;
+		clocks = <&usb_clk 6>, <&ahb_gates 2>;
+		phys = <&usbphy 1>;
+		phy-names = "usb";
+	};
diff --git a/Documentation/devicetree/bindings/usb/via,vt8500-ehci.txt b/Documentation/devicetree/bindings/usb/via,vt8500-ehci.txt
deleted file mode 100644
index 17b3ad1..0000000
--- a/Documentation/devicetree/bindings/usb/via,vt8500-ehci.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-VIA/Wondermedia VT8500 EHCI Controller
------------------------------------------------------
-
-Required properties:
-- compatible : "via,vt8500-ehci"
-- reg : Should contain 1 register ranges(address and length)
-- interrupts : ehci controller interrupt
-
-Example:
-
-	ehci@d8007900 {
-		compatible = "via,vt8500-ehci";
-		reg = <0xd8007900 0x200>;
-		interrupts = <43>;
-	};
diff --git a/Documentation/devicetree/bindings/usb/vt8500-ehci.txt b/Documentation/devicetree/bindings/usb/vt8500-ehci.txt
deleted file mode 100644
index 5fb8fd6..0000000
--- a/Documentation/devicetree/bindings/usb/vt8500-ehci.txt
+++ /dev/null
@@ -1,12 +0,0 @@
-VIA VT8500 and Wondermedia WM8xxx SoC USB controllers.
-
-Required properties:
- - compatible: Should be "via,vt8500-ehci" or "wm,prizm-ehci".
- - reg: Address range of the ehci registers. size should be 0x200
- - interrupts: Should contain the ehci interrupt.
-
-usb: ehci@D8007100 {
-	compatible = "wm,prizm-ehci", "usb-ehci";
-	reg = <0xD8007100 0x200>;
-	interrupts = <1>;
-};
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index 7f30b71..dd51644 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -3,6 +3,7 @@
  *
  * Copyright 2007 Steven Brown <sbrown-aLUkz7fEuXBWk0Htik3J/w@public.gmane.org>
  * Copyright 2010-2012 Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org>
+ * Copyright 2014 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  *
  * Derived from the ohci-ssb driver
  * Copyright 2007 Michael Buesch <m@bues.ch>
@@ -18,6 +19,7 @@
  *
  * Licensed under the GNU/GPL. See COPYING for details.
  */
+#include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
@@ -25,6 +27,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
@@ -33,6 +36,13 @@
 #include "ehci.h"
 
 #define DRIVER_DESC "EHCI generic platform driver"
+#define EHCI_MAX_CLKS 3
+#define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv)
+
+struct ehci_platform_priv {
+	struct clk *clks[EHCI_MAX_CLKS];
+	struct phy *phy;
+};
 
 static const char hcd_name[] = "ehci-platform";
 
@@ -64,38 +74,90 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
 	return 0;
 }
 
+static int ehci_platform_power_on(struct platform_device *dev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(dev);
+	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
+	int clk, ret;
+
+	for (clk = 0; priv->clks[clk] && clk < EHCI_MAX_CLKS; clk++) {
+		ret = clk_prepare_enable(priv->clks[clk]);
+		if (ret)
+			goto err_disable_clks;
+	}
+
+	if (priv->phy) {
+		ret = phy_init(priv->phy);
+		if (ret)
+			goto err_disable_clks;
+
+		ret = phy_power_on(priv->phy);
+		if (ret)
+			goto err_exit_phy;
+	}
+
+	return 0;
+
+err_exit_phy:
+	phy_exit(priv->phy);
+err_disable_clks:
+	while (--clk >= 0)
+		clk_disable_unprepare(priv->clks[clk]);
+
+	return ret;
+}
+
+static void ehci_platform_power_off(struct platform_device *dev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(dev);
+	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
+	int clk;
+
+	if (priv->phy) {
+		phy_power_off(priv->phy);
+		phy_exit(priv->phy);
+	}
+
+	for (clk = EHCI_MAX_CLKS - 1; clk >= 0; clk--)
+		if (priv->clks[clk])
+			clk_disable_unprepare(priv->clks[clk]);
+}
+
 static struct hc_driver __read_mostly ehci_platform_hc_driver;
 
 static const struct ehci_driver_overrides platform_overrides __initconst = {
-	.reset =	ehci_platform_reset,
+	.reset =		ehci_platform_reset,
+	.extra_priv_size =	sizeof(struct ehci_platform_priv),
 };
 
-static struct usb_ehci_pdata ehci_platform_defaults;
+static struct usb_ehci_pdata ehci_platform_defaults = {
+	.power_on =		ehci_platform_power_on,
+	.power_suspend =	ehci_platform_power_off,
+	.power_off =		ehci_platform_power_off,
+};
 
 static int ehci_platform_probe(struct platform_device *dev)
 {
 	struct usb_hcd *hcd;
 	struct resource *res_mem;
-	struct usb_ehci_pdata *pdata;
-	int irq;
-	int err;
+	struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
+	struct ehci_platform_priv *priv;
+	int clk, irq, err;
 
 	if (usb_disabled())
 		return -ENODEV;
 
 	/*
-	 * use reasonable defaults so platforms don't have to provide these.
-	 * with DT probing on ARM, none of these are set.
+	 * Use reasonable defaults so platforms don't have to provide these
+	 * with DT probing on ARM.
 	 */
-	if (!dev_get_platdata(&dev->dev))
-		dev->dev.platform_data = &ehci_platform_defaults;
+	if (!pdata)
+		pdata = &ehci_platform_defaults;
 
 	err = dma_coerce_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32));
 	if (err)
 		return err;
 
-	pdata = dev_get_platdata(&dev->dev);
-
 	irq = platform_get_irq(dev, 0);
 	if (irq < 0) {
 		dev_err(&dev->dev, "no irq provided");
@@ -107,17 +169,40 @@ static int ehci_platform_probe(struct platform_device *dev)
 		return -ENXIO;
 	}
 
+	hcd = usb_create_hcd(&ehci_platform_hc_driver, &dev->dev,
+			     dev_name(&dev->dev));
+	if (!hcd)
+		return -ENOMEM;
+
+	platform_set_drvdata(dev, hcd);
+	dev->dev.platform_data = pdata;
+	priv = hcd_to_ehci_priv(hcd);
+
+	if (pdata == &ehci_platform_defaults && dev->dev.of_node) {
+		priv->phy = devm_phy_get(&dev->dev, "usb");
+		if (IS_ERR(priv->phy)) {
+			err = PTR_ERR(priv->phy);
+			if (err == -EPROBE_DEFER)
+				goto err_put_hcd;
+			priv->phy = NULL;
+		}
+
+		for (clk = 0; clk < EHCI_MAX_CLKS; clk++) {
+			priv->clks[clk] = of_clk_get(dev->dev.of_node, clk);
+			if (IS_ERR(priv->clks[clk])) {
+				err = PTR_ERR(priv->clks[clk]);
+				if (err == -EPROBE_DEFER)
+					goto err_put_clks;
+				priv->clks[clk] = NULL;
+				break;
+			}
+		}
+	}
+
 	if (pdata->power_on) {
 		err = pdata->power_on(dev);
 		if (err < 0)
-			return err;
-	}
-
-	hcd = usb_create_hcd(&ehci_platform_hc_driver, &dev->dev,
-			     dev_name(&dev->dev));
-	if (!hcd) {
-		err = -ENOMEM;
-		goto err_power;
+			goto err_put_clks;
 	}
 
 	hcd->rsrc_start = res_mem->start;
@@ -126,21 +211,25 @@ static int ehci_platform_probe(struct platform_device *dev)
 	hcd->regs = devm_ioremap_resource(&dev->dev, res_mem);
 	if (IS_ERR(hcd->regs)) {
 		err = PTR_ERR(hcd->regs);
-		goto err_put_hcd;
+		goto err_power;
 	}
 	err = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (err)
-		goto err_put_hcd;
-
-	platform_set_drvdata(dev, hcd);
+		goto err_power;
 
 	return err;
 
-err_put_hcd:
-	usb_put_hcd(hcd);
 err_power:
 	if (pdata->power_off)
 		pdata->power_off(dev);
+err_put_clks:
+	while (--clk >= 0)
+		clk_put(priv->clks[clk]);
+err_put_hcd:
+	if (pdata == &ehci_platform_defaults)
+		dev->dev.platform_data = NULL;
+
+	usb_put_hcd(hcd);
 
 	return err;
 }
@@ -149,13 +238,19 @@ static int ehci_platform_remove(struct platform_device *dev)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
 	struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
+	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
+	int clk;
 
 	usb_remove_hcd(hcd);
-	usb_put_hcd(hcd);
 
 	if (pdata->power_off)
 		pdata->power_off(dev);
 
+	for (clk = 0; priv->clks[clk] && clk < EHCI_MAX_CLKS; clk++)
+		clk_put(priv->clks[clk]);
+
+	usb_put_hcd(hcd);
+
 	if (pdata == &ehci_platform_defaults)
 		dev->dev.platform_data = NULL;
 
@@ -206,8 +301,10 @@ static int ehci_platform_resume(struct device *dev)
 static const struct of_device_id vt8500_ehci_ids[] = {
 	{ .compatible = "via,vt8500-ehci", },
 	{ .compatible = "wm,prizm-ehci", },
+	{ .compatible = "mmio-ehci", },
 	{}
 };
+MODULE_DEVICE_TABLE(of, vt8500_ehci_ids);
 
 static const struct platform_device_id ehci_platform_table[] = {
 	{ "ehci-platform", 0 },
-- 
1.8.4.2

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Hans de Goede]

Hi,

On 01/11/2014 12:50 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 01/11/2014 01:46 AM, Hans de Goede wrote:
>
>> Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
>> this version should be 100% ready for merging upstream.
>
>     I see you've decided to completely ignore my opinion. NAK, FWIW.

I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING,
everyone except you seems to be fine with mmio and and one point in time we
need to make a decision and move forward.

Regards,

Hans

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Bjørn Mork]

Hans de Goede <hdegoede@redhat.com> writes:
> On 01/11/2014 12:50 AM, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 01/11/2014 01:46 AM, Hans de Goede wrote:
>>
>>> Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
>>> this version should be 100% ready for merging upstream.
>>
>>     I see you've decided to completely ignore my opinion. NAK, FWIW.
>
> I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING,
> everyone except you seems to be fine with mmio and and one point in time we
> need to make a decision and move forward.

FWIW I agree with your decision - not that I think that matters to
anyone :-)

But it would have been nice if you summarized the discussion in the
cover letter, including the reason behind your decision.  Just so that
Sergei and other reviewers would see that you didn't actually ignore any
opinion.  You just didn't agree, and you had your reasons to do so.


Bjørn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Hans de Goede]

Hi,

On 01/11/2014 01:10 AM, Sergei Shtylyov wrote:
> On 01/11/2014 01:52 AM, Hans de Goede wrote:
>
>>>> Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
>>>> this version should be 100% ready for merging upstream.
>
>>>     I see you've decided to completely ignore my opinion. NAK, FWIW.
>
>> I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING,
>> everyone except you seems to be fine with mmio
>
>     Everyone being couple of people reading this thread? Of which only one being familiar with the real [EO]HCI hardware? :-)

You are aware that I've worked on qemu's USB emulation for 3 years full-time,
including things like making the ehci emulation Windows XP compatible and
many other hcd emulation fixes?

And Alan Stern's credentials speak for themselves.

Comments like this do not really help to get your own comments taken serious.

>     The name is just plain stupid if you want to know my real opinion.

Your opinion has been noted, but so far your entire objection seems to be
you not liking the name and now calling it stupid, while there have been
actually *technical* arguments against the other prefixes.

> And you've posted v3 with this name used first only what, 1.5 days ago?
>
>> and and one point in time we
>> need to make a decision and move forward.
>
>     I see the first version of your patches posted on January 6th, today is 11th (just starting here). You must be in a very big hurry to get this merged, I see. :-)

This is not about being in a hurry, this is about not wanting to waste
time on what is quickly turning into pure bike-shedding.

>     Serious changes like this are not really done at such pace. Only the stuff that someone else has to painfully fix up later... :-(

If you're suggestion that the mmio- prefix will cause breakage later now
is your time to actually make an actual technical argument. "just plain
stupid" does not help to get your comments taken serious.

Regards,

Hans

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Bjørn Mork]

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes:

>> FWIW I agree with your decision - not that I think that matters to
>> anyone :-)
>
>    It matters at least to me, potential user of this stuff.

Sorry for not being clear about what "that" was referring to.

What I meant to say was: "not that I think *my opinion on the subject*
matters to anyone."


Bjørn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Sergei Shtylyov]

Hello.

On 01/11/2014 01:46 AM, Hans de Goede wrote:

> Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
> this version should be 100% ready for merging upstream.

    I see you've decided to completely ignore my opinion. NAK, FWIW.

> Changes since v3:
> -Address all of Alan Stern's review comments
> -Properly use put_clk in probe error path
> -Don't do hcd_put until after power_off as power_off may use private hcd data
> Regards,

> Hans

WBR, Sergei

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Sergei Shtylyov]

On 01/11/2014 01:52 AM, Hans de Goede wrote:

>>> Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
>>> this version should be 100% ready for merging upstream.

>>     I see you've decided to completely ignore my opinion. NAK, FWIW.

> I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING,
> everyone except you seems to be fine with mmio

    Everyone being couple of people reading this thread? Of which only one 
being familiar with the real [EO]HCI hardware? :-)
    The name is just plain stupid if you want to know my real opinion.
And you've posted v3 with this name used first only what, 1.5 days ago?

> and and one point in time we
> need to make a decision and move forward.

    I see the first version of your patches posted on January 6th, today is 
11th (just starting here). You must be in a very big hurry to get this merged, 
I see. :-)
    Serious changes like this are not really done at such pace. Only the stuff 
that someone else has to painfully fix up later... :-(

> Regards,

> Hans

WBR, Sergei

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Sergei Shtylyov]

Hello.

On 01/11/2014 02:08 AM, Bjørn Mork wrote:

>>>> Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
>>>> this version should be 100% ready for merging upstream.

>>>      I see you've decided to completely ignore my opinion. NAK, FWIW.

>> I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING,
>> everyone except you seems to be fine with mmio and and one point in time we
>> need to make a decision and move forward.

> FWIW I agree with your decision - not that I think that matters to
> anyone :-)

    It matters at least to me, potential user of this stuff. It's a pity that 
I haven't done this myself (in fact, I had other idea than this: writing a 
specific driver for my platform). As they say "if you want it done right, do 
it yourself".

> But it would have been nice if you summarized the discussion in the
> cover letter, including the reason behind your decision.  Just so that

    The reason seems quite clear: Hans wants it to be merged really fast.

> Sergei and other reviewers would see that you didn't actually ignore any
> opinion.  You just didn't agree, and you had your reasons to do so.

    Hans hasn't replied to my last mail, so I felt ignored, yes.

> Bjørn

WBR, Sergei

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Sergei Shtylyov]

Hello.

On 01/11/2014 02:24 AM, Hans de Goede wrote:

>>>>> Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
>>>>> this version should be 100% ready for merging upstream.

>>>>     I see you've decided to completely ignore my opinion. NAK, FWIW.

>>> I'm sorry but the whole prefix thing has become a thing of
>>> -ETOOMUCHBIKESHEDDING,
>>> everyone except you seems to be fine with mmio

>>     Everyone being couple of people reading this thread? Of which only one
>> being familiar with the real [EO]HCI hardware? :-)

> You are aware that I've worked on qemu's USB emulation for 3 years full-time,
> including things like making the ehci emulation Windows XP compatible and
> many other hcd emulation fixes?

    Then you should have known that all [EO]HCI controllers use MMIO, and it's 
pointless to call them "MMIO [EO]HCI controller" in your title and in your 
"compatible" prop. Because you plainly create an impression that there's some 
other kind of [EO]HCI controllers out there, e.g. I/O mapped (for which a 
separate binding would be needed?).

> And Alan Stern's credentials speak for themselves.

    Yes, I meant Alan by [EO]HCI expert. He's not DT expert, unfortunately, AFAIK.

> Comments like this do not really help to get your own comments taken serious.

    Well, if nothing else helps... :-(

>>     The name is just plain stupid if you want to know my real opinion.

> Your opinion has been noted, but so far your entire objection seems to be
> you not liking the name and now calling it stupid, while there have been
> actually *technical* arguments against the other prefixes.

    I didn't hear your reply to my last mail which laid out some plan of 
dealing with ehci-ppc-of.c. Your "technical" argument against using "usb-ehci" 
didn't really make much sense to me and I told you why and what to do to get 
things right. You chose to not even reply to this and instead post "100% 
fready for merge" patchset.
    Sigh, it's a pity Alistair Popple's patches [1], [2] didn't get merged. He 
simply disappeared somewhere.

>> And you've posted v3 with this name used first only what, 1.5 days ago?

>>> and and one point in time we
>>> need to make a decision and move forward.

>>     I see the first version of your patches posted on January 6th, today is
>> 11th (just starting here). You must be in a very big hurry to get this
>> merged, I see. :-)

> This is not about being in a hurry, this is about not wanting to waste
> time on what is quickly turning into pure bike-shedding.

    Do you think calling other people's opinions "pure bike-shedding" helps 
the discussion?

>>     Serious changes like this are not really done at such pace. Only the
>> stuff that someone else has to painfully fix up later... :-(

> If you're suggestion that the mmio- prefix will cause breakage later now
> is your time to actually make an actual technical argument. "just plain
> stupid" does not help to get your comments taken serious.

    The worst thing is once the bindings get merged, they're "set in stone" 
and it won't be easy to fix, if at all possible... :-(

> Regards,

> Hans

[1] http://marc.info/?l=linux-usb&m=138508645219604
[2] http://marc.info/?l=linuxppc-embedded&m=138508628409947

Re: [PATCH v4 1/2] ohci-platform: Add support for devicetree instantiation

[Tony Prisk]

On 11/01/14 11:46, Hans de Goede wrote:
> Add support for ohci-platform instantiation from devicetree, including
> optionally getting clks and a phy from devicetree, and enabling / disabling
> those on power_on / off.
>
> This should allow using ohci-platform from devicetree in various cases.
> Specifically after this commit it can be used for the ohci controller found
> on Allwinner sunxi SoCs.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   .../devicetree/bindings/usb/mmio-ohci.txt          |  22 +++
>   drivers/usb/host/ohci-platform.c                   | 164 ++++++++++++++++++---
>   2 files changed, 162 insertions(+), 24 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/usb/mmio-ohci.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/mmio-ohci.txt b/Documentation/devicetree/bindings/usb/mmio-ohci.txt
> new file mode 100644
> index 0000000..9c776ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/mmio-ohci.txt
> @@ -0,0 +1,22 @@
> +Generic MMIO OHCI controller
> +
> +Required properties:
> +- compatible : "mmio-ohci"
> +- reg : ohci controller register range (address and length)
> +- interrupts : ohci controller interrupt
> +
> +Optional properties:
> +- clocks : a list of phandle + clock specifier pairs
> +- phys : phandle + phy specifier pair
> +- phy-names : "usb"
> +
> +Example:
> +
> +	ohci0: ohci@0x01c14400 {
> +		compatible = "mmio-ohci";
> +		reg = <0x01c14400 0x100>;
> +		interrupts = <64>;
> +		clocks = <&usb_clk 6>, <&ahb_gates 2>;
> +		phys = <&usbphy 1>;
> +		phy-names = "usb";
> +	};
>
> ... <snip> ....
>
> @@ -83,17 +156,40 @@ static int ohci_platform_probe(struct platform_device *dev)
>   		return -ENXIO;
>   	}
>   
> +	hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev,
> +			dev_name(&dev->dev));
> +	if (!hcd)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(dev, hcd);
> +	dev->dev.platform_data = pdata;
> +	priv = hcd_to_ohci_priv(hcd);
> +
> +	if (pdata == &ohci_platform_defaults && dev->dev.of_node) {
> +		priv->phy = devm_phy_get(&dev->dev, "usb");
> +		if (IS_ERR(priv->phy)) {
> +			err = PTR_ERR(priv->phy);
> +			if (err == -EPROBE_DEFER)
> +				goto err_put_hcd;
> +			priv->phy = NULL;
> +		}
> +
> +		for (clk = 0; clk < OHCI_MAX_CLKS; clk++) {
> +			priv->clks[clk] = of_clk_get(dev->dev.of_node, clk);
> +			if (IS_ERR(priv->clks[clk])) {
> +				err = PTR_ERR(priv->clks[clk]);
> +				if (err == -EPROBE_DEFER)
> +					goto err_put_clks;
> +				priv->clks[clk] = NULL;
> +				break;
> +			}
> +		}
> +	}
>

Given that you have limited the clock parsing to OHCI_MAX_CLKS, there 
should be some kind of warning/error message if the DT contains more 
than OHCI_MAX_CLKS - otherwise you have silent failures when the clocks 
aren't configured.

There is also no note in the binding to indicate this limitation and you 
shouldn't expect people writing DT files to go digging through the 
source to check for this.

Regards
Tony P

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Alan Stern]

On Fri, 10 Jan 2014, Hans de Goede wrote:

> Hi,
> 
> On 01/11/2014 12:50 AM, Sergei Shtylyov wrote:
> > Hello.
> >
> > On 01/11/2014 01:46 AM, Hans de Goede wrote:
> >
> >> Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
> >> this version should be 100% ready for merging upstream.
> >
> >     I see you've decided to completely ignore my opinion. NAK, FWIW.
> 
> I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING,
> everyone except you seems to be fine with mmio and and one point in time we
> need to make a decision and move forward.

If this isn't beating a dead horse...  Maybe everyone can agree on a
name like ohci-generic or generic-ohci.  That seems like a pretty good
description of the hardware that the platform driver can handle.

Alan Stern

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Tony Prisk]

On 12/01/14 11:30, Alan Stern wrote:
> On Fri, 10 Jan 2014, Hans de Goede wrote:
>
>> Hi,
>>
>> On 01/11/2014 12:50 AM, Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> On 01/11/2014 01:46 AM, Hans de Goede wrote:
>>>
>>>> Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
>>>> this version should be 100% ready for merging upstream.
>>>      I see you've decided to completely ignore my opinion. NAK, FWIW.
>> I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING,
>> everyone except you seems to be fine with mmio and and one point in time we
>> need to make a decision and move forward.
> If this isn't beating a dead horse...  Maybe everyone can agree on a
> name like ohci-generic or generic-ohci.  That seems like a pretty good
> description of the hardware that the platform driver can handle.
>
> Alan Stern
I prefer the -generic option, although generic- is equally fine - Having 
said that, I don't really care if it's called mmio either (although this 
does seem less 'descriptive').

Regards
Tony Prisk
--
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 v4 1/2] ohci-platform: Add support for devicetree instantiation

[Hans de Goede]

Hi,

Thanks for the review. Note I did my best to ensure my patches
would not break vt8500 support, but if you could run some tests
with them that would be great.

On 01/11/2014 09:37 AM, Tony Prisk wrote:
> On 11/01/14 11:46, Hans de Goede wrote:
>> Add support for ohci-platform instantiation from devicetree, including
>> optionally getting clks and a phy from devicetree, and enabling / disabling
>> those on power_on / off.
>>
>> This should allow using ohci-platform from devicetree in various cases.
>> Specifically after this commit it can be used for the ohci controller found
>> on Allwinner sunxi SoCs.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>   .../devicetree/bindings/usb/mmio-ohci.txt          |  22 +++
>>   drivers/usb/host/ohci-platform.c                   | 164 ++++++++++++++++++---
>>   2 files changed, 162 insertions(+), 24 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/usb/mmio-ohci.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/mmio-ohci.txt b/Documentation/devicetree/bindings/usb/mmio-ohci.txt
>> new file mode 100644
>> index 0000000..9c776ed
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/mmio-ohci.txt
>> @@ -0,0 +1,22 @@
>> +Generic MMIO OHCI controller
>> +
>> +Required properties:
>> +- compatible : "mmio-ohci"
>> +- reg : ohci controller register range (address and length)
>> +- interrupts : ohci controller interrupt
>> +
>> +Optional properties:
>> +- clocks : a list of phandle + clock specifier pairs
>> +- phys : phandle + phy specifier pair
>> +- phy-names : "usb"
>> +
>> +Example:
>> +
>> +    ohci0: ohci@0x01c14400 {
>> +        compatible = "mmio-ohci";
>> +        reg = <0x01c14400 0x100>;
>> +        interrupts = <64>;
>> +        clocks = <&usb_clk 6>, <&ahb_gates 2>;
>> +        phys = <&usbphy 1>;
>> +        phy-names = "usb";
>> +    };
>>
>> ... <snip> ....
>>
>> @@ -83,17 +156,40 @@ static int ohci_platform_probe(struct platform_device *dev)
>>           return -ENXIO;
>>       }
>> +    hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev,
>> +            dev_name(&dev->dev));
>> +    if (!hcd)
>> +        return -ENOMEM;
>> +
>> +    platform_set_drvdata(dev, hcd);
>> +    dev->dev.platform_data = pdata;
>> +    priv = hcd_to_ohci_priv(hcd);
>> +
>> +    if (pdata == &ohci_platform_defaults && dev->dev.of_node) {
>> +        priv->phy = devm_phy_get(&dev->dev, "usb");
>> +        if (IS_ERR(priv->phy)) {
>> +            err = PTR_ERR(priv->phy);
>> +            if (err == -EPROBE_DEFER)
>> +                goto err_put_hcd;
>> +            priv->phy = NULL;
>> +        }
>> +
>> +        for (clk = 0; clk < OHCI_MAX_CLKS; clk++) {
>> +            priv->clks[clk] = of_clk_get(dev->dev.of_node, clk);
>> +            if (IS_ERR(priv->clks[clk])) {
>> +                err = PTR_ERR(priv->clks[clk]);
>> +                if (err == -EPROBE_DEFER)
>> +                    goto err_put_clks;
>> +                priv->clks[clk] = NULL;
>> +                break;
>> +            }
>> +        }
>> +    }
>>
>
> Given that you have limited the clock parsing to OHCI_MAX_CLKS, there should be some kind of warning/error message if the DT contains more than OHCI_MAX_CLKS - otherwise you have silent failures when the clocks aren't configured.

Adding such a test would be non trivial, and is just not worth it IMHO. There is one special case of a controller with 3 clocks,
all others have 0-2 clocks, there are 0 known controllers with > 3 clocks. And if one shows up then the person writing the
dts will figure this out quickly enough, and using old kernels with newer dts files has never been a supported scenario.

> There is also no note in the binding to indicate this limitation and you shouldn't expect people writing DT files to go digging through the source to check for this.

I've been told by several people that dt-binding documentation should never contain implementation details, since it
is supposed to be platform agnostic, etc. So people writing dts files who need to know implementation details, are
actually expected to look into the implementation. And in my experience with re-using some existing drivers for
other SoC's, that is exactly what one always ends up doing.

Regards,

Hans

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Tomasz Figa]

Hi,

[Cc'ing DT maintainers directly]

On 12.01.2014 04:04, Tony Prisk wrote:
> On 12/01/14 11:30, Alan Stern wrote:
>> On Fri, 10 Jan 2014, Hans de Goede wrote:
>>
>>> Hi,
>>>
>>> On 01/11/2014 12:50 AM, Sergei Shtylyov wrote:
>>>> Hello.
>>>>
>>>> On 01/11/2014 01:46 AM, Hans de Goede wrote:
>>>>
>>>>> Here is v4 of my ohci and ehci-platform clks, phy and dt support
>>>>> patch-set,
>>>>> this version should be 100% ready for merging upstream.
>>>>      I see you've decided to completely ignore my opinion. NAK, FWIW.
>>> I'm sorry but the whole prefix thing has become a thing of
>>> -ETOOMUCHBIKESHEDDING,
>>> everyone except you seems to be fine with mmio and and one point in
>>> time we
>>> need to make a decision and move forward.
>> If this isn't beating a dead horse...  Maybe everyone can agree on a
>> name like ohci-generic or generic-ohci.  That seems like a pretty good
>> description of the hardware that the platform driver can handle.
>>
>> Alan Stern
> I prefer the -generic option, although generic- is equally fine - Having
> said that, I don't really care if it's called mmio either (although this
> does seem less 'descriptive').

Grepping over existing dts files, I can find several occurrences of 
"usb-ehci" compatible string:

at91sam9g45.dtsi:                       compatible = 
"atmel,at91sam9g45-ehci", "usb-ehci";
at91sam9x5.dtsi:                        compatible = 
"atmel,at91sam9g45-ehci", "usb-ehci";
omap3.dtsi:                             compatible = "ti,ehci-omap", 
"usb-ehci";
omap4.dtsi:                             compatible = "ti,ehci-omap", 
"usb-ehci";
omap5.dtsi:                             compatible = "ti,ehci-omap", 
"usb-ehci";
sama5d3.dtsi:                   compatible = "atmel,at91sam9g45-ehci", 
"usb-ehci";
spear13xx.dtsi:                 compatible = "st,spear600-ehci", "usb-ehci";
spear13xx.dtsi:                 compatible = "st,spear600-ehci", "usb-ehci";
spear3xx.dtsi:                  compatible = "st,spear600-ehci", "usb-ehci";
spear600.dtsi:                  compatible = "st,spear600-ehci", "usb-ehci";
spear600.dtsi:                  compatible = "st,spear600-ehci", "usb-ehci";
tegra114.dtsi:          compatible = "nvidia,tegra30-ehci", "usb-ehci";
tegra114.dtsi:          compatible = "nvidia,tegra30-ehci", "usb-ehci";
tegra20.dtsi:           compatible = "nvidia,tegra20-ehci", "usb-ehci";
tegra20.dtsi:           compatible = "nvidia,tegra20-ehci", "usb-ehci";
tegra20.dtsi:           compatible = "nvidia,tegra20-ehci", "usb-ehci";
tegra30.dtsi:           compatible = "nvidia,tegra30-ehci", "usb-ehci";
tegra30.dtsi:           compatible = "nvidia,tegra30-ehci", "usb-ehci";
tegra30.dtsi:           compatible = "nvidia,tegra30-ehci", "usb-ehci";

Same for "usb-ohci":

arch/arm/boot/dts/at91rm9200.dtsi:                      compatible = 
"atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/at91sam9260.dtsi:                     compatible = 
"atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/at91sam9263.dtsi:                     compatible = 
"atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/at91sam9g45.dtsi:                     compatible = 
"atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/at91sam9n12.dtsi:                     compatible = 
"atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/at91sam9x5.dtsi:                      compatible = 
"atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/lpc32xx.dtsi:                 compatible = 
"nxp,ohci-nxp", "usb-ohci";
arch/arm/boot/dts/omap3.dtsi:                           compatible = 
"ti,ohci-omap3", "usb-ohci";
arch/arm/boot/dts/omap4.dtsi:                           compatible = 
"ti,ohci-omap3", "usb-ohci";
arch/arm/boot/dts/omap5.dtsi:                           compatible = 
"ti,ohci-omap3", "usb-ohci";
arch/arm/boot/dts/sama5d3.dtsi:                 compatible = 
"atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/spear13xx.dtsi:                       compatible = 
"st,spear600-ohci", "usb-ohci";
arch/arm/boot/dts/spear13xx.dtsi:                       compatible = 
"st,spear600-ohci", "usb-ohci";
arch/arm/boot/dts/spear3xx.dtsi:                        compatible = 
"st,spear600-ohci", "usb-ohci";
arch/arm/boot/dts/spear3xx.dtsi:                        compatible = 
"st,spear600-ohci", "usb-ohci";
arch/arm/boot/dts/spear600.dtsi:                        compatible = 
"st,spear600-ohci", "usb-ohci";
arch/arm/boot/dts/spear600.dtsi:                        compatible = 
"st,spear600-ohci", "usb-ohci";

For "usb-ehci" there is even a documentation file [1], while "usb-ohci" 
seems to be undocumented.

[1] Documentation/devicetree/bindings/usb/usb-ehci.txt

Aren't they both something that should be accounted for in this series?

Best regards,
Tomasz

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Hans de Goede]

Hi,

On 01/12/2014 02:04 PM, Tomasz Figa wrote:
> Hi,
>
> [Cc'ing DT maintainers directly]
>

<snip>

>>> Alan Stern Wrote:
>> I prefer the -generic option, although generic- is equally fine - Having
>> said that, I don't really care if it's called mmio either (although this
>> does seem less 'descriptive').

I can do a v5 changing the compatible string to generix-Xhci, if that will
put an end to all this discussion, then again, there may be a better way, see
below.

>
> Grepping over existing dts files, I can find several occurrences of "usb-ehci" compatible string:
>
> at91sam9g45.dtsi:                       compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> at91sam9x5.dtsi:                        compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> omap3.dtsi:                             compatible = "ti,ehci-omap", "usb-ehci";
> omap4.dtsi:                             compatible = "ti,ehci-omap", "usb-ehci";
> omap5.dtsi:                             compatible = "ti,ehci-omap", "usb-ehci";
> sama5d3.dtsi:                   compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
> spear13xx.dtsi:                 compatible = "st,spear600-ehci", "usb-ehci";
> spear13xx.dtsi:                 compatible = "st,spear600-ehci", "usb-ehci";
> spear3xx.dtsi:                  compatible = "st,spear600-ehci", "usb-ehci";
> spear600.dtsi:                  compatible = "st,spear600-ehci", "usb-ehci";
> spear600.dtsi:                  compatible = "st,spear600-ehci", "usb-ehci";
> tegra114.dtsi:          compatible = "nvidia,tegra30-ehci", "usb-ehci";
> tegra114.dtsi:          compatible = "nvidia,tegra30-ehci", "usb-ehci";
> tegra20.dtsi:           compatible = "nvidia,tegra20-ehci", "usb-ehci";
> tegra20.dtsi:           compatible = "nvidia,tegra20-ehci", "usb-ehci";
> tegra20.dtsi:           compatible = "nvidia,tegra20-ehci", "usb-ehci";
> tegra30.dtsi:           compatible = "nvidia,tegra30-ehci", "usb-ehci";
> tegra30.dtsi:           compatible = "nvidia,tegra30-ehci", "usb-ehci";
> tegra30.dtsi:           compatible = "nvidia,tegra30-ehci", "usb-ehci";
>
> Same for "usb-ohci":
>
> arch/arm/boot/dts/at91rm9200.dtsi:                      compatible = "atmel,at91rm9200-ohci", "usb-ohci";
> arch/arm/boot/dts/at91sam9260.dtsi:                     compatible = "atmel,at91rm9200-ohci", "usb-ohci";
> arch/arm/boot/dts/at91sam9263.dtsi:                     compatible = "atmel,at91rm9200-ohci", "usb-ohci";
> arch/arm/boot/dts/at91sam9g45.dtsi:                     compatible = "atmel,at91rm9200-ohci", "usb-ohci";
> arch/arm/boot/dts/at91sam9n12.dtsi:                     compatible = "atmel,at91rm9200-ohci", "usb-ohci";
> arch/arm/boot/dts/at91sam9x5.dtsi:                      compatible = "atmel,at91rm9200-ohci", "usb-ohci";
> arch/arm/boot/dts/lpc32xx.dtsi:                 compatible = "nxp,ohci-nxp", "usb-ohci";
> arch/arm/boot/dts/omap3.dtsi:                           compatible = "ti,ohci-omap3", "usb-ohci";
> arch/arm/boot/dts/omap4.dtsi:                           compatible = "ti,ohci-omap3", "usb-ohci";
> arch/arm/boot/dts/omap5.dtsi:                           compatible = "ti,ohci-omap3", "usb-ohci";
> arch/arm/boot/dts/sama5d3.dtsi:                 compatible = "atmel,at91rm9200-ohci", "usb-ohci";
> arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ohci", "usb-ohci";
> arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ohci", "usb-ohci";
> arch/arm/boot/dts/spear3xx.dtsi:                        compatible = "st,spear600-ohci", "usb-ohci";
> arch/arm/boot/dts/spear3xx.dtsi:                        compatible = "st,spear600-ohci", "usb-ohci";
> arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ohci", "usb-ohci";
> arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ohci", "usb-ohci";
>
> For "usb-ehci" there is even a documentation file [1], while "usb-ohci" seems to be undocumented.
>
> [1] Documentation/devicetree/bindings/usb/usb-ehci.txt
>
> Aren't they both something that should be accounted for in this series?

I agree that usb-Xhci would be the best compatible strings to use.

The problem with usb-ehci is that there already is a ppc specific driver binding to that
compatible string, doing various ppc specific controller initialization.

Thinking more about this, there is one possible solution though, the ehci-ppc-of.c is
guarded in Kconfig with:

         depends on PPC_OF

If we add an inverted check to the Kconfig option for platform-ehci.c, ie:

config USB_EHCI_HCD_PLATFORM
         tristate "Generic EHCI driver for a platform device"
	depends on !PPC_OF

Then we can be certain that we don't end up with 2 drivers claiming the
usb-ehci compatible on ppc platforms. I've done some quick research
and it seems that ehci-platform.c is only used on arm and mips devices, so
excluding its use on ppc should not be an issue.

Then later on someone, who has the actual hardware to test, can merge the
ppc specific quirk handling into ehci-platform,c and ehci-ppc-of.c can go
away entirely.

Alan, if you agree this is the best way forward, I'll do a v5 with the proposed
changes.

Regards,

Hans

Re: [PATCH v4 0/2] ohci and ehci-platform clks, phy and dt support

[Alan Stern]

On Mon, 13 Jan 2014, Hans de Goede wrote:

> I agree that usb-Xhci would be the best compatible strings to use.
> 
> The problem with usb-ehci is that there already is a ppc specific driver binding to that
> compatible string, doing various ppc specific controller initialization.
> 
> Thinking more about this, there is one possible solution though, the ehci-ppc-of.c is
> guarded in Kconfig with:
> 
>          depends on PPC_OF
> 
> If we add an inverted check to the Kconfig option for platform-ehci.c, ie:
> 
> config USB_EHCI_HCD_PLATFORM
>          tristate "Generic EHCI driver for a platform device"
> 	depends on !PPC_OF
> 
> Then we can be certain that we don't end up with 2 drivers claiming the
> usb-ehci compatible on ppc platforms. I've done some quick research
> and it seems that ehci-platform.c is only used on arm and mips devices, so
> excluding its use on ppc should not be an issue.
> 
> Then later on someone, who has the actual hardware to test, can merge the
> ppc specific quirk handling into ehci-platform,c and ehci-ppc-of.c can go
> away entirely.
> 
> Alan, if you agree this is the best way forward, I'll do a v5 with the proposed
> changes.

That's okay with me.

Alan Stern