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

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

[Hans de Goede]

Hi all,

And here is v5 of my ohci and ehci-platform clks, phy and dt support patch-set.

New since the last version is that the compatibility strings now are
"usb-ohci" and "usb-ehci", which should make everyone happy I hope.

Other then that there are no changes compared to v4.

Regards,

Hans

[PATCH v5 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>
---
 Documentation/devicetree/bindings/usb/usb-ohci.txt |  22 +++
 drivers/usb/host/ohci-platform.c                   | 164 ++++++++++++++++++---
 2 files changed, 162 insertions(+), 24 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/usb-ohci.txt

diff --git a/Documentation/devicetree/bindings/usb/usb-ohci.txt b/Documentation/devicetree/bindings/usb/usb-ohci.txt
new file mode 100644
index 0000000..f9d6c73
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb-ohci.txt
@@ -0,0 +1,22 @@
+USB OHCI controllers
+
+Required properties:
+- compatible : "usb-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 = "allwinner,sun4i-a10-ohci", "usb-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..60e125d 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 = "usb-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 v5 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.

Since ehci-platform is intended to handle any generic enough non pci ehci
device, add a "usb-ehci" compatibility string.

There already is a usb-ehci device-tree bindings document, update this
with clks and phy bindings info.

Although actually quite generic so far the via,vt8500 compatibilty string
had its own bindings document. Somehow we even ended up with 2 of them. Since
these provide no extra information over the generic usb-ehci documentation,
this patch removes them.

The ehci-ppc-of.c driver also claims the usb-ehci compatibility string,
even though it mostly is ibm,usb-ehci-440epx specific. ehci-platform.c is
not needed on ppc platforms, so add a !PPC_OF dependency to it to avoid
2 drivers claiming the same compatibility string getting build on ppc.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 Documentation/devicetree/bindings/usb/usb-ehci.txt |  25 +++-
 .../devicetree/bindings/usb/via,vt8500-ehci.txt    |  15 ---
 .../devicetree/bindings/usb/vt8500-ehci.txt        |  12 --
 drivers/usb/host/Kconfig                           |   1 +
 drivers/usb/host/ehci-platform.c                   | 149 +++++++++++++++++----
 5 files changed, 142 insertions(+), 60 deletions(-)
 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/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
index fa18612..fad10f3 100644
--- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
@@ -7,13 +7,14 @@ Required properties:
     (debug-port or other) can be also specified here, but only after
     definition of standard EHCI registers.
   - interrupts : one EHCI interrupt should be described here.
-If device registers are implemented in big endian mode, the device
-node should have "big-endian-regs" property.
-If controller implementation operates with big endian descriptors,
-"big-endian-desc" property should be specified.
-If both big endian registers and descriptors are used by the controller
-implementation, "big-endian" property can be specified instead of having
-both "big-endian-regs" and "big-endian-desc".
+
+Optional properties:
+ - big-endian-regs : boolean, set this for hcds with big-endian registers
+ - big-endian-desc : boolean, set this for hcds with big-endian descriptors
+ - big-endian : boolean, for hcds with big-endian-regs + big-endian-desc
+ - clocks : a list of phandle + clock specifier pairs
+ - phys : phandle + phy specifier pair
+ - phy-names : "usb"
 
 Example (Sequoia 440EPx):
     ehci@e0000300 {
@@ -23,3 +24,13 @@ Example (Sequoia 440EPx):
 	   reg = <0 e0000300 90 0 e0000390 70>;
 	   big-endian;
    };
+
+Example (Allwinner sun4i A10 SoC):
+   ehci0: ehci@0x01c14000 {
+	   compatible = "allwinner,sun4i-a10-ehci", "usb-ehci";
+	   reg = <0x01c14000 0x100>;
+	   interrupts = <39>;
+	   clocks = <&ahb_gates 1>;
+	   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/Kconfig b/drivers/usb/host/Kconfig
index a9707da..e28cbe0 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -255,6 +255,7 @@ config USB_EHCI_ATH79
 
 config USB_EHCI_HCD_PLATFORM
 	tristate "Generic EHCI driver for a platform device"
+	depends on !PPC_OF
 	default n
 	---help---
 	  Adds an EHCI host driver for a generic platform device, which
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index 7f30b71..5c4f892 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 = "usb-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 v5 2/2] ehci-platform: Add support for clks and phy passed through devicetree

[Tony Prisk]

On 14/01/14 11:45, Hans de Goede wrote:
> 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.
>
> Since ehci-platform is intended to handle any generic enough non pci ehci
> device, add a "usb-ehci" compatibility string.
>
> There already is a usb-ehci device-tree bindings document, update this
> with clks and phy bindings info.
>
> Although actually quite generic so far the via,vt8500 compatibilty string
> had its own bindings document. Somehow we even ended up with 2 of them. Since
> these provide no extra information over the generic usb-ehci documentation,
> this patch removes them.
>
> The ehci-ppc-of.c driver also claims the usb-ehci compatibility string,
> even though it mostly is ibm,usb-ehci-440epx specific. ehci-platform.c is
> not needed on ppc platforms, so add a !PPC_OF dependency to it to avoid
> 2 drivers claiming the same compatibility string getting build on ppc.
>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  25 +++-
>   .../devicetree/bindings/usb/via,vt8500-ehci.txt    |  15 ---
>   .../devicetree/bindings/usb/vt8500-ehci.txt        |  12 --
>   drivers/usb/host/Kconfig                           |   1 +
>   drivers/usb/host/ehci-platform.c                   | 149 +++++++++++++++++----
>   5 files changed, 142 insertions(+), 60 deletions(-)
>   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/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> index fa18612..fad10f3 100644
> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
> @@ -7,13 +7,14 @@ Required properties:
>       (debug-port or other) can be also specified here, but only after
>       definition of standard EHCI registers.
>     - interrupts : one EHCI interrupt should be described here.
> -If device registers are implemented in big endian mode, the device
> -node should have "big-endian-regs" property.
> -If controller implementation operates with big endian descriptors,
> -"big-endian-desc" property should be specified.
> -If both big endian registers and descriptors are used by the controller
> -implementation, "big-endian" property can be specified instead of having
> -both "big-endian-regs" and "big-endian-desc".
> +
> +Optional properties:
> + - big-endian-regs : boolean, set this for hcds with big-endian registers
> + - big-endian-desc : boolean, set this for hcds with big-endian descriptors
> + - big-endian : boolean, for hcds with big-endian-regs + big-endian-desc
> + - clocks : a list of phandle + clock specifier pairs
> + - phys : phandle + phy specifier pair
> + - phy-names : "usb"
>   
>   Example (Sequoia 440EPx):
>       ehci@e0000300 {
> @@ -23,3 +24,13 @@ Example (Sequoia 440EPx):
>   	   reg = <0 e0000300 90 0 e0000390 70>;
>   	   big-endian;
>      };
> +
> +Example (Allwinner sun4i A10 SoC):
> +   ehci0: ehci@0x01c14000 {
> +	   compatible = "allwinner,sun4i-a10-ehci", "usb-ehci";
> +	   reg = <0x01c14000 0x100>;
> +	   interrupts = <39>;
> +	   clocks = <&ahb_gates 1>;
> +	   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/Kconfig b/drivers/usb/host/Kconfig
> index a9707da..e28cbe0 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -255,6 +255,7 @@ config USB_EHCI_ATH79
>   
>   config USB_EHCI_HCD_PLATFORM
>   	tristate "Generic EHCI driver for a platform device"
> +	depends on !PPC_OF
>   	default n
>   	---help---
>   	  Adds an EHCI host driver for a generic platform device, which
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index 7f30b71..5c4f892 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 = "usb-ehci", },
>   	{}
>   };
> +MODULE_DEVICE_TABLE(of, vt8500_ehci_ids);
>   
>   static const struct platform_device_id ehci_platform_table[] = {
>   	{ "ehci-platform", 0 },


You have removed the documentation for the via,vt8500-ehci and 
wm,prizm-ehci bindings (which in itself isn't really an issue), and left 
them in the driver (again, not an issue) - except now we have two 
bindings without any binding documentation?!?

I'm not sure where the 'wm,prizm-ehci' string came from (presumably me 
some time ago), but it isn't used and could be dropped.
All WonderMedia SoCs use the 'via,vt8500-ehci' - Doing a quick grep of 
arch/arm/boot/dts shows no usage of 'wm,prizm-ehci'.

Arguably, the dtsi files for the WonderMedia SoCs could be updated to 
include 'usb-ehci' as a second compatible string, and the 
'via,vt8500-ehci' could be dropped from the driver - but it does mean 
that old dts files won't work with the current kernel (and I think 
that's a no-no).

Regards
Tony P

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

[Hans de Goede]

Hi,

On 01/14/2014 05:12 AM, Tony Prisk wrote:
> On 14/01/14 11:45, Hans de Goede wrote:
>> 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.
>>
>> Since ehci-platform is intended to handle any generic enough non pci ehci
>> device, add a "usb-ehci" compatibility string.
>>
>> There already is a usb-ehci device-tree bindings document, update this
>> with clks and phy bindings info.
>>
>> Although actually quite generic so far the via,vt8500 compatibilty string
>> had its own bindings document. Somehow we even ended up with 2 of them. Since
>> these provide no extra information over the generic usb-ehci documentation,
>> this patch removes them.
>>
>> The ehci-ppc-of.c driver also claims the usb-ehci compatibility string,
>> even though it mostly is ibm,usb-ehci-440epx specific. ehci-platform.c is
>> not needed on ppc platforms, so add a !PPC_OF dependency to it to avoid
>> 2 drivers claiming the same compatibility string getting build on ppc.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>   Documentation/devicetree/bindings/usb/usb-ehci.txt |  25 +++-
>>   .../devicetree/bindings/usb/via,vt8500-ehci.txt    |  15 ---
>>   .../devicetree/bindings/usb/vt8500-ehci.txt        |  12 --
>>   drivers/usb/host/Kconfig                           |   1 +
>>   drivers/usb/host/ehci-platform.c                   | 149 +++++++++++++++++----
>>   5 files changed, 142 insertions(+), 60 deletions(-)
>>   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/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> index fa18612..fad10f3 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
>> @@ -7,13 +7,14 @@ Required properties:
>>       (debug-port or other) can be also specified here, but only after
>>       definition of standard EHCI registers.
>>     - interrupts : one EHCI interrupt should be described here.
>> -If device registers are implemented in big endian mode, the device
>> -node should have "big-endian-regs" property.
>> -If controller implementation operates with big endian descriptors,
>> -"big-endian-desc" property should be specified.
>> -If both big endian registers and descriptors are used by the controller
>> -implementation, "big-endian" property can be specified instead of having
>> -both "big-endian-regs" and "big-endian-desc".
>> +
>> +Optional properties:
>> + - big-endian-regs : boolean, set this for hcds with big-endian registers
>> + - big-endian-desc : boolean, set this for hcds with big-endian descriptors
>> + - big-endian : boolean, for hcds with big-endian-regs + big-endian-desc
>> + - clocks : a list of phandle + clock specifier pairs
>> + - phys : phandle + phy specifier pair
>> + - phy-names : "usb"
>>   Example (Sequoia 440EPx):
>>       ehci@e0000300 {
>> @@ -23,3 +24,13 @@ Example (Sequoia 440EPx):
>>          reg = <0 e0000300 90 0 e0000390 70>;
>>          big-endian;
>>      };
>> +
>> +Example (Allwinner sun4i A10 SoC):
>> +   ehci0: ehci@0x01c14000 {
>> +       compatible = "allwinner,sun4i-a10-ehci", "usb-ehci";
>> +       reg = <0x01c14000 0x100>;
>> +       interrupts = <39>;
>> +       clocks = <&ahb_gates 1>;
>> +       phys = <&usbphy 1>;
>> +       phy-names = "usb";
>> +   };

<snip>

> You have removed the documentation for the via,vt8500-ehci and wm,prizm-ehci bindings (which in itself isn't really an issue), and left them in the driver (again, not an issue)

Correct.

> - except now we have two bindings without any binding documentation?!?

My reasoning behind is that the driver is really a generic usb-ehci driver, and the only reason
the 2 compatibility strings are left in there is for backwards compatibility. I don't think
compatibility strings only kept in the driver for backwards compatibility should be documented,
because documented suggests people can rely on them and then they may end up getting used for new things.

> I'm not sure where the 'wm,prizm-ehci' string came from (presumably me some time ago), but it isn't used and could be dropped.
> All WonderMedia SoCs use the 'via,vt8500-ehci' - Doing a quick grep of arch/arm/boot/dts shows no usage of 'wm,prizm-ehci'.
>
> Arguably, the dtsi files for the WonderMedia SoCs could be updated to include 'usb-ehci' as a second compatible string

Right, I think this would be a good thing to do regardless, also see all the existing dts files for other
boards already doing this (which is why we ended up using usb-ehci in the first place).

> and the 'via,vt8500-ehci' could be dropped from the driver - but it does mean that old dts files won't work with the current kernel (and I think that's a no-no).

Right, which is exactly why I did not remove them from the driver. But since as you indicate yourself
if we were to do things from scratch we would likely not put them in the driver it seems best to
leave them undocumented. Alternatively, if you see this as a problem I can add a note to usb-ehci.txt listing
these 2 compatibility strings as legacy compatiblity strings, which should not be used for new dts files.

Regards,

Hans

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

[Alan Stern]

On Mon, 13 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.

This is fine as far as I am concerned, except for one thing.

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

clk isn't initialized to anything...

> -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]);

... but it gets used here.  The compiler should have warned about this.

After fixing that, you can add

Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>

Alan Stern

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

[Alan Stern]

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

> 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.
> 
> Since ehci-platform is intended to handle any generic enough non pci ehci
> device, add a "usb-ehci" compatibility string.
> 
> There already is a usb-ehci device-tree bindings document, update this
> with clks and phy bindings info.
> 
> Although actually quite generic so far the via,vt8500 compatibilty string
> had its own bindings document. Somehow we even ended up with 2 of them. Since
> these provide no extra information over the generic usb-ehci documentation,
> this patch removes them.
> 
> The ehci-ppc-of.c driver also claims the usb-ehci compatibility string,
> even though it mostly is ibm,usb-ehci-440epx specific. ehci-platform.c is
> not needed on ppc platforms, so add a !PPC_OF dependency to it to avoid
> 2 drivers claiming the same compatibility string getting build on ppc.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Same comment as the ohci-platform patch.

Oh, and one other thing I almost missed.  (This applies to both 
patches.)

> +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++) {

Here and in ehci_platform_remove, you need to check clk < EHCI_MAX_CLKS
_before_ dereferencing priv->clks[clk].  Yes, it probably won't make
any difference in real life, but it's still wrong in principle.  In
fact, this is the sort of thing a static checker might find.

Alan Stern

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

[Hans de Goede]

Hi,

On 01/14/2014 08:08 PM, Alan Stern wrote:
> On Mon, 13 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.
>
> This is fine as far as I am concerned, except for one thing.
>
>> @@ -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;
>
> clk isn't initialized to anything...

Good catch, will fix and then do a v6 with this + the issue from your other mail fixed
and I'll add your ack.

>
>> -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]);
>
> ... but it gets used here.  The compiler should have warned about this.

Should have yes, but it doesn't (I've just double checked).

> After fixing that, you can add
>
> Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>

Thanks & Regards,

Hans