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

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

[Hans de Goede]

Hi All,

Here is v3 of my ohci and ehci-platform clks, phy and dt support patch-set.

This version is mostly the result of some discussions on irc with Maxime to
improve the dt-bindings. This has resulted in 2 changes:

1) Drop the clock-names from the dt-bindings, instead get clocks by index.

Since the intend is to be a generic ohci / ehci driver, the code does not
give any special meaning to any of the clocks, so simply just get as many
clocks as there are by index.

2) Change the compatible string to "mmio-ohci" resp. "mmio-ehci"

v2 was using allwinner,sun4i-ohci as compat string to avoid having to come
up with a generic name, but in retrospect this is the wrong thing to do. This
will likely lead to dts files for non allwinner boards using compatible =
"allwinner,sun4i-ohci", and if we then ever need to do some allwinner specific
quirks, these boards end up getting these quirks applied too.

dts files should use the generic mmio-ehci compatible as least prefered option,
ie: compatible = "allwinner,sun7i-a20-ehci", "mmio-ehci";

This will allow use of the generic driver while leaving the option open to
later add soc specific quirks without needing dt changes.

Regards,

Hans

[PATCH v3 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                   | 150 ++++++++++++++++++---
 2 files changed, 152 insertions(+), 20 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..a46d7ac 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>
@@ -28,6 +32,12 @@
 
 #define DRIVER_DESC "OHCI generic platform driver"
 
+#define OHCI_MAX_CLKS 3
+struct ohci_platform_priv {
+	struct clk *clks[OHCI_MAX_CLKS];
+	struct phy *phy;
+};
+
 static const char hcd_name[] = "ohci-platform";
 
 static int ohci_platform_reset(struct usb_hcd *hcd)
@@ -48,11 +58,69 @@ 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 =
+		(struct ohci_platform_priv *)hcd_to_ohci(hcd)->priv;
+	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 =
+		(struct ohci_platform_priv *)hcd_to_ohci(hcd)->priv;
+	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,12 +128,24 @@ 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;
+	struct ohci_platform_priv *priv;
+	int clk, irq, err;
 
+	/*
+	 * Use reasonable defaults so platforms don't have to provide these
+	 * with DT probing on ARM.
+	 */
 	if (!pdata) {
-		WARN_ON(1);
-		return -ENODEV;
+		pdata = dev->dev.platform_data = &ohci_platform_defaults;
+		/*
+		 * 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.
+		 */
+		err = dma_coerce_mask_and_coherent(&dev->dev,
+						   DMA_BIT_MASK(32));
+		if (err)
+			return err;
 	}
 
 	if (usb_disabled())
@@ -83,17 +163,39 @@ 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;
+
+	priv = (struct ohci_platform_priv *)hcd_to_ohci(hcd)->priv;
+
+	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;
+			}
+		}
+	}
+
+	platform_set_drvdata(dev, hcd);
 	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 +204,22 @@ 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_disable_unprepare(priv->clks[clk]);
+err_put_hcd:
+	usb_put_hcd(hcd);
 
 	return err;
 }
@@ -178,6 +281,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 +307,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 v3 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                   | 129 +++++++++++++++++----
 4 files changed, 131 insertions(+), 47 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..c7adcf5 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>
@@ -34,6 +37,12 @@
 
 #define DRIVER_DESC "EHCI generic platform driver"
 
+#define EHCI_MAX_CLKS 3
+struct ehci_platform_priv {
+	struct clk *clks[EHCI_MAX_CLKS];
+	struct phy *phy;
+};
+
 static const char hcd_name[] = "ehci-platform";
 
 static int ehci_platform_reset(struct usb_hcd *hcd)
@@ -64,28 +73,84 @@ 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 =
+		(struct ehci_platform_priv *)hcd_to_ehci(hcd)->priv;
+	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 =
+		(struct ehci_platform_priv *)hcd_to_ehci(hcd)->priv;
+	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 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;
@@ -107,17 +172,39 @@ 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;
+
+	priv = (struct ehci_platform_priv *)hcd_to_ehci(hcd)->priv;
+
+	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;
+			}
+		}
+	}
+
+	platform_set_drvdata(dev, hcd);
 	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 +213,22 @@ 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_disable_unprepare(priv->clks[clk]);
+err_put_hcd:
+	usb_put_hcd(hcd);
 
 	return err;
 }
@@ -206,6 +294,7 @@ 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", },
 	{}
 };
 
-- 
1.8.4.2

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

[Arnd Bergmann]

On Thursday 09 January 2014 18:57:05 Hans de Goede wrote:
> +       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;
> +                       }
> +               }
> +       }

Ah, very nice! This way it will actually work to replace a number
of older drivers that require a specific clock name.

I still think we should change the phy subsystem to allow the
same, that would make it more consistent here, and avoid the
need for coming up with a number of bogus phy names for random
drivers that can only ever have one phy.

	Arnd

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

[Hans de Goede]

Hi,

On 01/09/2014 07:07 PM, Arnd Bergmann wrote:
> On Thursday 09 January 2014 18:57:05 Hans de Goede wrote:
>> +       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;
>> +                       }
>> +               }
>> +       }
>
> Ah, very nice! This way it will actually work to replace a number
> of older drivers that require a specific clock name.

I'm glad you like it.
>
> I still think we should change the phy subsystem to allow the
> same, that would make it more consistent here, and avoid the
> need for coming up with a number of bogus phy names for random
> drivers that can only ever have one phy.

I'm not disagreeing, but that will have to be a battle for
another day. Currently the dt-bindings for phy actually
make phy-names mandatory at the bindings-description level, so
someone would first need to change the specification.

Documentation/devicetree/bindings/phy/phy-bindings.txt:

"""
PHY user node
=============

Required Properties:
phys : the phandle for the PHY device (used by the PHY subsystem)
phy-names : the names of the PHY corresponding to the PHYs present in the
             *phys* phandle
"""

Regards,

Hans

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

[Hans de Goede]

Hi,

On 01/09/2014 09:14 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 01/09/2014 08:57 PM, 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                   | 150 ++++++++++++++++++---
>>   2 files changed, 152 insertions(+), 20 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
>
>     OHCI controller always uses MMIO, and likewise EHCI. You don't need to specifically mention it.

Right, I'm only using it here because it is also used in the compatible string.

>
>> +
>> +Required properties:
>> +- compatible : "mmio-ohci"
>
>     Likewise, it's not a good name. Why not call it "platform-ohci"?

Because, as you would have known had you read the entire thread, people objected
against exactly that name because the "platform" bus thing is a Linux invention,
and other operating systems don't use the platform nomenclature for non pci
busses.

Regards,

Hans

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

[Sergei Shtylyov]

Hello.

On 01/09/2014 08:57 PM, 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                   | 150 ++++++++++++++++++---
>   2 files changed, 152 insertions(+), 20 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

    OHCI controller always uses MMIO, and likewise EHCI. You don't need to 
specifically mention it.

> +
> +Required properties:
> +- compatible : "mmio-ohci"

    Likewise, it's not a good name. Why not call it "platform-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";
> +	};

WBR, Sergei

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

[Alan Stern]

On Thu, 9 Jan 2014, 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>

Looking pretty good.  Still a couple of things to address...

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

I wonder if these routines couldn't be shared with non-DT setups.  We
could add an array of 3 struct clk pointers to the usb_ohci_pdata
structure.  Then if any of them are set, store these routines in the
power_* pointers.  (And likewise for ehci-platform.)  Something to
consider for a future patch...

> @@ -60,12 +128,24 @@ 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;
> +	struct ohci_platform_priv *priv;
> +	int clk, irq, err;
>  
> +	/*
> +	 * Use reasonable defaults so platforms don't have to provide these
> +	 * with DT probing on ARM.
> +	 */
>  	if (!pdata) {
> -		WARN_ON(1);
> -		return -ENODEV;
> +		pdata = dev->dev.platform_data = &ohci_platform_defaults;

This has a subtle bug.  Consider what will happen if ohci-platform is 
loaded, then unloaded and loaded again.

The existing ehci-platform driver has this same bug.  I definitely 
remember discussing at once or twice in the past, but evidently it has 
never been fixed.

> +		/*
> +		 * 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.
> +		 */
> +		err = dma_coerce_mask_and_coherent(&dev->dev,
> +						   DMA_BIT_MASK(32));
> +		if (err)
> +			return err;

The ehci-platform driver does this even when platform data is present.  
I guess you should do the same thing here (and lose the comment).

(Also, I dislike this alignment of the continuation line.  IMO it's a 
bad idea to match up with some particular element of the preceding 
line.  The style used in most of the USB stack is to indent 
continuation lines by two tab stops.)

>  	}
>  
>  	if (usb_disabled())

This test belongs at the start of the function.  It is more fundamental 
than the stuff you inserted.

> @@ -83,17 +163,39 @@ 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;
> +
> +	priv = (struct ohci_platform_priv *)hcd_to_ohci(hcd)->priv;

Please define an inline routine or a macro for this messy computation.

Alan Stern

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

[Sergei Shtylyov]

On 01/09/2014 10:48 PM, Hans de Goede wrote:
> Hi,
>
> On 01/09/2014 09:14 PM, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 01/09/2014 08:57 PM, 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                   | 150
>>> ++++++++++++++++++---
>>>   2 files changed, 152 insertions(+), 20 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

>>     OHCI controller always uses MMIO, and likewise EHCI. You don't need to
>> specifically mention it.

> Right, I'm only using it here because it is also used in the compatible string.

    Please drop it.

>>> +
>>> +Required properties:
>>> +- compatible : "mmio-ohci"

>>     Likewise, it's not a good name. Why not call it "platform-ohci"?

> Because, as you would have known had you read the entire thread, people objected
> against exactly that name because the "platform" bus thing is a Linux invention,
> and other operating systems don't use the platform nomenclature for non pci
> busses.

    I wonder where were all those people when "xhci-platform" compatible got 
adopted in drivers/usb/host/xhci-plat.c? :-P

    Anyway, I want to suggest "usb-[eo]hci" of which "usb-ehci" binding has 
even already documented in Documentation/devicetree/bindings/usb/usb-ehci.txt.
Both these "compatible" values are used as backups in the multiple described 
bindings of the platform-specific [EO]HCI controllers. I really don't see why 
you should invent anything new (and so poorly named).

> Regards,

> Hans

WBR, Sergei

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

[Hans de Goede]

Hi,

On 01/09/2014 10:08 PM, Sergei Shtylyov wrote:
> On 01/09/2014 10:48 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 01/09/2014 09:14 PM, Sergei Shtylyov wrote:
>>> Hello.
>>>
>>> On 01/09/2014 08:57 PM, 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                   | 150
>>>> ++++++++++++++++++---
>>>>   2 files changed, 152 insertions(+), 20 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
>
>>>     OHCI controller always uses MMIO, and likewise EHCI. You don't need to
>>> specifically mention it.
>
>> Right, I'm only using it here because it is also used in the compatible string.
>
>     Please drop it.
>
>>>> +
>>>> +Required properties:
>>>> +- compatible : "mmio-ohci"
>
>>>     Likewise, it's not a good name. Why not call it "platform-ohci"?
>
>> Because, as you would have known had you read the entire thread, people objected
>> against exactly that name because the "platform" bus thing is a Linux invention,
>> and other operating systems don't use the platform nomenclature for non pci
>> busses.
>
>     I wonder where were all those people when "xhci-platform" compatible got adopted in drivers/usb/host/xhci-plat.c? :-P
>
>     Anyway, I want to suggest "usb-[eo]hci" of which "usb-ehci" binding has even already documented in Documentation/devicetree/bindings/usb/usb-ehci.txt.
> Both these "compatible" values are used as backups in the multiple described bindings of the platform-specific [EO]HCI controllers. I really don't see why you should invent anything new (and so poorly named).

This too has already been discussed, again please read the entire thread including review of earlier versions of the patch.

We cannot usb usb-ehci, because the existing usb-ehci compatible string is not used for a generic usb controller, but
for a ppc specific one which needs model specific setup, see the driver currently implementing compatible = usb-ehci.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 01/09/2014 09:36 PM, Alan Stern wrote:
> On Thu, 9 Jan 2014, 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>
>
> Looking pretty good.  Still a couple of things to address...
>
>> +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,
>>   };
>
> I wonder if these routines couldn't be shared with non-DT setups.  We
> could add an array of 3 struct clk pointers to the usb_ohci_pdata
> structure.  Then if any of them are set, store these routines in the
> power_* pointers.  (And likewise for ehci-platform.)  Something to
> consider for a future patch...

I don't like automatically setting the power_ platform_data members much.

But I've already been thinking about moving the clks member to platform_data,
and simply exporting ohci_platform_power_* so that other drivers can use them
and put them in the platform_data members themselves. That is indeed something
for another patch though.

>> @@ -60,12 +128,24 @@ 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;
>> +	struct ohci_platform_priv *priv;
>> +	int clk, irq, err;
>>
>> +	/*
>> +	 * Use reasonable defaults so platforms don't have to provide these
>> +	 * with DT probing on ARM.
>> +	 */
>>   	if (!pdata) {
>> -		WARN_ON(1);
>> -		return -ENODEV;
>> +		pdata = dev->dev.platform_data = &ohci_platform_defaults;
>
> This has a subtle bug.  Consider what will happen if ohci-platform is
> loaded, then unloaded and loaded again.

Right, so we need to add a "dev->dev.platform_data == &ohci_platform_defaults"
check on remove an then set platform_data to NULL, good point, will fix in the
next version.

>
> The existing ehci-platform driver has this same bug.  I definitely
> remember discussing at once or twice in the past, but evidently it has
> never been fixed.

If you agree with the above fix I'll also throw it into the next revision
of the ehci patch.

>
>> +		/*
>> +		 * 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.
>> +		 */
>> +		err = dma_coerce_mask_and_coherent(&dev->dev,
>> +						   DMA_BIT_MASK(32));
>> +		if (err)
>> +			return err;
>
> The ehci-platform driver does this even when platform data is present.
> I guess you should do the same thing here (and lose the comment).
>
> (Also, I dislike this alignment of the continuation line.  IMO it's a
> bad idea to match up with some particular element of the preceding
> line.  The style used in most of the USB stack is to indent
> continuation lines by two tab stops.)

Ok, will fix.

>
>>   	}
>>
>>   	if (usb_disabled())
>
> This test belongs at the start of the function.  It is more fundamental
> than the stuff you inserted.
>

Ok, will fix.

>> @@ -83,17 +163,39 @@ 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;
>> +
>> +	priv = (struct ohci_platform_priv *)hcd_to_ohci(hcd)->priv;
>
> Please define an inline routine or a macro for this messy computation.

Ok, will fix.

Before I do a v4, one question:

Ma Haijun does not like the use of OHCI_MAX_CLKS:

 >> +#define OHCI_MAX_CLKS 3
 >
 > I still recommend using of_count_phandle_with_args(np, "clocks",
 > "#clock-cells") to determine the number of clocks or the existence of clocks
 > property (-ENOENT)

His suggestion would mean dynamically allocating the clks array and having
clks_count variable. I still think this is a bit overkill, but I can do that for
v4 if you want, so what do you prefer ?

Regards,

Hans

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

[Alan Stern]

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

> >> +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,
> >>   };
> >
> > I wonder if these routines couldn't be shared with non-DT setups.  We
> > could add an array of 3 struct clk pointers to the usb_ohci_pdata
> > structure.  Then if any of them are set, store these routines in the
> > power_* pointers.  (And likewise for ehci-platform.)  Something to
> > consider for a future patch...
> 
> I don't like automatically setting the power_ platform_data members much.

Me either.

> But I've already been thinking about moving the clks member to platform_data,
> and simply exporting ohci_platform_power_* so that other drivers can use them
> and put them in the platform_data members themselves. That is indeed something
> for another patch though.

It would require building ohci-platform into the kernel instead of 
making it a separate module.  That shouldn't matter much for 
embedded/SoC systems, though.  It seems like a good approach.

> >>   	if (!pdata) {
> >> -		WARN_ON(1);
> >> -		return -ENODEV;
> >> +		pdata = dev->dev.platform_data = &ohci_platform_defaults;
> >
> > This has a subtle bug.  Consider what will happen if ohci-platform is
> > loaded, then unloaded and loaded again.
> 
> Right, so we need to add a "dev->dev.platform_data == &ohci_platform_defaults"
> check on remove an then set platform_data to NULL, good point, will fix in the
> next version.

Exactly.

> > The existing ehci-platform driver has this same bug.  I definitely
> > remember discussing at once or twice in the past, but evidently it has
> > never been fixed.
> 
> If you agree with the above fix I'll also throw it into the next revision
> of the ehci patch.

Thank you.

> Before I do a v4, one question:
> 
> Ma Haijun does not like the use of OHCI_MAX_CLKS:
> 
>  >> +#define OHCI_MAX_CLKS 3
>  >
>  > I still recommend using of_count_phandle_with_args(np, "clocks",
>  > "#clock-cells") to determine the number of clocks or the existence of clocks
>  > property (-ENOENT)
> 
> His suggestion would mean dynamically allocating the clks array and having
> clks_count variable. I still think this is a bit overkill, but I can do that for
> v4 if you want, so what do you prefer ?

Space isn't a big consideration, because we're not likely to see a 
system with more a few controllers.  I say keep it simple.  If someone 
comes up with a controller using more than 3 clocks, we can change to 
dynamic allocation then.

Alan Stern

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

[Sergei Shtylyov]

Hello.

On 01/10/2014 12:47 PM, Hans de Goede wrote:

> Hi,

    I would like to know why you dropped me from the To: list when replying. I 
have hardly noticed your reply.

>>>>> 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                   | 150
>>>>> ++++++++++++++++++---
>>>>>   2 files changed, 152 insertions(+), 20 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

>>>>     OHCI controller always uses MMIO, and likewise EHCI. You don't need to
>>>> specifically mention it.

>>> Right, I'm only using it here because it is also used in the compatible
>>> string.

>>     Please drop it.

>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : "mmio-ohci"

>>>>     Likewise, it's not a good name. Why not call it "platform-ohci"?

>>> Because, as you would have known had you read the entire thread, people
>>> objected
>>> against exactly that name because the "platform" bus thing is a Linux
>>> invention,
>>> and other operating systems don't use the platform nomenclature for non pci
>>> busses.

>>     I wonder where were all those people when "xhci-platform" compatible got
>> adopted in drivers/usb/host/xhci-plat.c? :-P

    I remember I suggested "usb-xhci" but the author didn't want to do it and 
I said that I have no strong opinion (along with Alan Stern, IIRC), so we have 
it now...

>>     Anyway, I want to suggest "usb-[eo]hci" of which "usb-ehci" binding has
>> even already documented in Documentation/devicetree/bindings/usb/usb-ehci.txt.
>> Both these "compatible" values are used as backups in the multiple described
>> bindings of the platform-specific [EO]HCI controllers. I really don't see
>> why you should invent anything new (and so poorly named).

> This too has already been discussed, again please read the entire thread
> including review of earlier versions of the patch.

    Sorry, I have been generally lacking the time to read linux-usb recently.
I only bumped into your patch randomly, despite me being interested in this 
matter for my own platform...

> We cannot usb usb-ehci, because the existing usb-ehci compatible string is not
> used for a generic usb controller, but
> for a ppc specific one  which needs model specific setup, see the driver
> currently implementing compatible = usb-ehci.

    Not true about it being entirely PPC specific: the driver actually looks 
for another "compatible" property to make it truly PPC specific (and apply 
some errata fixes for that specific PPC platform) and "usb-ehci" is used in 
many non-PPC binding examples as a backup still. Valentine Barshak's did his 
work on ehci-ppc-of.c when DT was used only on PPC and SPARC, so it was kind 
of a honest mistake on his part to name the driver so that people would think 
it's PPC specific (not only his, it seems, as the driver was based on the 
other two). This does not seem a fatal mistake, and we could limit the driver 
to match only on that IBM 440EPx platform it was designed for and tested on, 
making ehci-platform.c handle "usb-ehci". This is more work but I believe it's 
the Right Thing. The problem is that we'll have to do this atomically with 
ehci-platform.c change since 2 other PPC platforms use "usb-ehci" as a backup 
to their specific "compatible" props and don't have the platform-specific 
drivers for them, and we probably wouldn't want to break them.
I would like to help you here but the matter is not urgent for me, so cannot 
work on it right now... :-(
    Anyway, I'm strongly against such names as "mmio-[eo]hci". But I could 
live with just "[eo]hci" if you don't follow my advice and unravel the 
ehci-ppc-of.c knot...

> Regards,

> Hans

WBR, Sergei