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

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

[Hans de Goede]

Hi All,

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

Changes since v1:
-Various improvements to dt-bindings as suggested by various people
-Support up-to 3 clocks

Regards,

Hans

[PATCH v2 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/platform-ohci.txt      |  25 ++++
 drivers/usb/host/ohci-platform.c                   | 148 ++++++++++++++++++---
 2 files changed, 153 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/platform-ohci.txt

diff --git a/Documentation/devicetree/bindings/usb/platform-ohci.txt b/Documentation/devicetree/bindings/usb/platform-ohci.txt
new file mode 100644
index 0000000..44bfa57
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt
@@ -0,0 +1,25 @@
+Generic Platform OHCI controller
+
+Required properties:
+- compatible : "allwinner,sun4i-ohci"
+- reg : ohci controller register range (address and length)
+- interrupts : ohci controller interrupt
+
+Optional properties:
+- clocks : a list of phandle + clock specifier pairs, one for each entry
+   in clock-names.
+- clock-names : "clk0", "clk1", ...
+- phys : phy
+- phy-names : "phy0"
+
+Example:
+
+	ohci0: ohci@0x01c14400 {
+		compatible = "allwinner,sun4i-ohci";
+		reg = <0x01c14400 0x100>;
+		interrupts = <64>;
+		clocks = <&usb_clk 6>, <&ahb_gates 2>;
+		clock-names = "clk0", "clk1";
+		phys = <&usbphy 1>;
+		phy-names = "phy0";
+	};
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index f351ff5..7cab768 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;
+	int clk, irq, err;
+	char name[8];
 
+	/*
+	 * 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,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;
+
+	if (pdata == &ohci_platform_defaults) {
+		struct ohci_platform_priv *priv = (struct ohci_platform_priv *)
+						  hcd_to_ohci(hcd)->priv;
+
+		priv->phy = devm_phy_get(&dev->dev, "phy0");
+		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++) {
+			snprintf(name, sizeof(name), "clk%d", clk);
+			priv->clks[clk] = devm_clk_get(&dev->dev, name);
+			if (IS_ERR(priv->clks[clk])) {
+				err = PTR_ERR(priv->clks[clk]);
+				if (err == -EPROBE_DEFER)
+					goto err_put_hcd;
+				priv->clks[clk] = NULL;
+			}
+		}
+	}
+
+	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_hcd;
 	}
 
 	hcd->rsrc_start = res_mem->start;
@@ -102,21 +205,19 @@ 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_hcd:
+	usb_put_hcd(hcd);
 
 	return err;
 }
@@ -178,6 +279,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 = "allwinner,sun4i-ohci", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ohci_platform_ids);
+
 static const struct platform_device_id ohci_platform_table[] = {
 	{ "ohci-platform", 0 },
 	{ }
@@ -198,6 +305,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 v2 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 platform-ehci.txt to reflect that this
is a generic platform driver, and removes the other.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/usb/platform-ehci.txt      |  25 ++++
 .../devicetree/bindings/usb/via,vt8500-ehci.txt    |  15 ---
 .../devicetree/bindings/usb/vt8500-ehci.txt        |  12 --
 drivers/usb/host/ehci-platform.c                   | 126 +++++++++++++++++----
 4 files changed, 131 insertions(+), 47 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/platform-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/platform-ehci.txt b/Documentation/devicetree/bindings/usb/platform-ehci.txt
new file mode 100644
index 0000000..56c478d
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/platform-ehci.txt
@@ -0,0 +1,25 @@
+Generic Platform EHCI controller
+
+Required properties:
+- compatible : "via,vt8500-ehci" or "wm,prizm-ehci"
+- reg : ehci controller register range (address and length)
+- interrupts : ehci controller interrupt
+
+Optional properties:
+- clocks : a list of phandle + clock specifier pairs, one for each entry
+   in clock-names.
+- clock-names : "clk0", "clk1", ...
+- phys : phy
+- phy-names : "phy0"
+
+Example:
+
+	ehci@d8007900 {
+		compatible = "via,vt8500-ehci";
+		reg = <0xd8007900 0x200>;
+		interrupts = <43>;
+		clocks = <&usb_clk 6>, <&ahb_gates 2>;
+		clock-names = "clk0", "clk1";
+		phys = <&usbphy 1>;
+		phy-names = "phy0";
+	};
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..ce76659 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;
+	int clk, irq, err;
+	char name[8];
 
 	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,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;
+
+	if (pdata == &ehci_platform_defaults) {
+		struct ehci_platform_priv *priv = (struct ehci_platform_priv *)
+						  hcd_to_ehci(hcd)->priv;
+
+		priv->phy = devm_phy_get(&dev->dev, "phy0");
+		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++) {
+			snprintf(name, sizeof(name), "clk%d", clk);
+			priv->clks[clk] = devm_clk_get(&dev->dev, name);
+			if (IS_ERR(priv->clks[clk])) {
+				err = PTR_ERR(priv->clks[clk]);
+				if (err == -EPROBE_DEFER)
+					goto err_put_hcd;
+				priv->clks[clk] = NULL;
+			}
+		}
+	}
+
+	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_hcd;
 	}
 
 	hcd->rsrc_start = res_mem->start;
@@ -126,21 +214,19 @@ 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_hcd:
+	usb_put_hcd(hcd);
 
 	return err;
 }
-- 
1.8.4.2

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

[Maxime Ripard]

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

On Wed, Jan 08, 2014 at 05:30:08PM +0100, 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.
> 
> Somehow we've ended up with 2 device-bindings documents for ehci-platform.c,
> this patch renames and updates one to platform-ehci.txt to reflect that this
> is a generic platform driver, and removes the other.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/usb/platform-ehci.txt      |  25 ++++
>  .../devicetree/bindings/usb/via,vt8500-ehci.txt    |  15 ---
>  .../devicetree/bindings/usb/vt8500-ehci.txt        |  12 --
>  drivers/usb/host/ehci-platform.c                   | 126 +++++++++++++++++----
>  4 files changed, 131 insertions(+), 47 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/platform-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/platform-ehci.txt b/Documentation/devicetree/bindings/usb/platform-ehci.txt
> new file mode 100644
> index 0000000..56c478d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/platform-ehci.txt
> @@ -0,0 +1,25 @@
> +Generic Platform EHCI controller
> +
> +Required properties:
> +- compatible : "via,vt8500-ehci" or "wm,prizm-ehci"
> +- reg : ehci controller register range (address and length)
> +- interrupts : ehci controller interrupt
> +
> +Optional properties:
> +- clocks : a list of phandle + clock specifier pairs, one for each entry
> +   in clock-names.
> +- clock-names : "clk0", "clk1", ...
> +- phys : phy
> +- phy-names : "phy0"
> +
> +Example:
> +
> +	ehci@d8007900 {
> +		compatible = "via,vt8500-ehci";
> +		reg = <0xd8007900 0x200>;
> +		interrupts = <43>;
> +		clocks = <&usb_clk 6>, <&ahb_gates 2>;
> +		clock-names = "clk0", "clk1";

I'm really not convinced by this either. It prevents you from doing
anything useful out of these clocks, and the only thing you can
actually do with it is calling clk_get, and that's pretty much it.

What if you some platform needs to adjust the rate of one of the two?
Or wants to cut one but not the other for any reason? You're just
screwed, and you can do anything about it, because you have no way of
telling what "clk0" is used for.

Especially, since what you really want is to access the clocks by
index, that you can do with of_clk_get.

Calling it "bus" and "phy" or whatever it's used for both provide a
way of differentiating the two, yet being rather generic. And if we
need to add a third one, I'm pretty sure we will be able to come up
with a generic name then.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

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

[Florian Fainelli]

Hello,

2014/1/8 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> 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/platform-ohci.txt      |  25 ++++
>  drivers/usb/host/ohci-platform.c                   | 148 ++++++++++++++++++---
>  2 files changed, 153 insertions(+), 20 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/platform-ohci.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/platform-ohci.txt b/Documentation/devicetree/bindings/usb/platform-ohci.txt
> new file mode 100644
> index 0000000..44bfa57
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt

ohci-mmio might be a better name than platform maybe?

[snip]

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

Do we really need to cap this to OHCI_MAX_CLKS, the next driver which
has 4 clocks will have to bump this, so maybe a linked-list would be
best?

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

Although I do value the idea of having DT probing for ohci-platform,
ehci-platform et al, I am not sure if this really belongs in the
generic OHCI platform driver, or if we should rather have small glue
drivers which register the ohci-platform driver and which provides all
the platform-specific power_on, power_off, suspend callbacks to the
ohci platform driver? Such glue driver would be the one which gets
probed based off a specific compatible string a

At first glance it does look like this covers 80% of the existing OHCI
drivers out there, so this is good. We might also want to support
clock pointers provided via platform_data so we can remove ohci-at91,
ohci-ep93xx, ohci-spear and friends in the future.

ohci-ppc-of could also probably be removed once we add quirks and
endian properties parsing to ohci-platform?

> +
> +       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;
> +       int clk, irq, err;
> +       char name[8];
>
> +       /*
> +        * 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,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;
> +
> +       if (pdata == &ohci_platform_defaults) {
> +               struct ohci_platform_priv *priv = (struct ohci_platform_priv *)
> +                                                 hcd_to_ohci(hcd)->priv;
> +
> +               priv->phy = devm_phy_get(&dev->dev, "phy0");

Does this work with a more than one OHCI controller which could
possibly have a dedicated PHY?

> +               if (IS_ERR(priv->phy)) {
> +                       err = PTR_ERR(priv->phy);
> +                       if (err == -EPROBE_DEFER)
> +                               goto err_put_hcd;
> +                       priv->phy = NULL;
> +               }

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

[Hans de Goede]

Hi,

On 01/08/2014 07:16 PM, Maxime Ripard wrote:
> On Wed, Jan 08, 2014 at 05:30:08PM +0100, 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.
>>
>> Somehow we've ended up with 2 device-bindings documents for ehci-platform.c,
>> this patch renames and updates one to platform-ehci.txt to reflect that this
>> is a generic platform driver, and removes the other.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>   .../devicetree/bindings/usb/platform-ehci.txt      |  25 ++++
>>   .../devicetree/bindings/usb/via,vt8500-ehci.txt    |  15 ---
>>   .../devicetree/bindings/usb/vt8500-ehci.txt        |  12 --
>>   drivers/usb/host/ehci-platform.c                   | 126 +++++++++++++++++----
>>   4 files changed, 131 insertions(+), 47 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/usb/platform-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/platform-ehci.txt b/Documentation/devicetree/bindings/usb/platform-ehci.txt
>> new file mode 100644
>> index 0000000..56c478d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/platform-ehci.txt
>> @@ -0,0 +1,25 @@
>> +Generic Platform EHCI controller
>> +
>> +Required properties:
>> +- compatible : "via,vt8500-ehci" or "wm,prizm-ehci"
>> +- reg : ehci controller register range (address and length)
>> +- interrupts : ehci controller interrupt
>> +
>> +Optional properties:
>> +- clocks : a list of phandle + clock specifier pairs, one for each entry
>> +   in clock-names.
>> +- clock-names : "clk0", "clk1", ...
>> +- phys : phy
>> +- phy-names : "phy0"
>> +
>> +Example:
>> +
>> +	ehci@d8007900 {
>> +		compatible = "via,vt8500-ehci";
>> +		reg = <0xd8007900 0x200>;
>> +		interrupts = <43>;
>> +		clocks = <&usb_clk 6>, <&ahb_gates 2>;
>> +		clock-names = "clk0", "clk1";
>
> I'm really not convinced by this either. It prevents you from doing
> anything useful out of these clocks, and the only thing you can
> actually do with it is calling clk_get, and that's pretty much it.
>
> What if you some platform needs to adjust the rate of one of the two?

Then it needs its own driver. This is intended as a binding for a
*generic* driver, which is meant to cover simple straight forward
non-pci ohci cases. For more complex cases a separate driver will
need to be written.

I must say I'm becoming a bit unhappy with how the reviews of devicetree
bindings are being done. In one case it is not generic enough (ahci-sunxi).

If I then try to make it more generic in a case where that can actually
be done as the hardware is pretty straight forward, it is not specific enough.
You can simply not have both!

> Or wants to cut one but not the other for any reason?

This is another example of non generic behavior, requiring a separate
(small using the existing ohci core) platform glue driver, like the *19* we
already have.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 01/08/2014 09:36 PM, Florian Fainelli wrote:
> Hello,
>
> 2014/1/8 Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> 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/platform-ohci.txt      |  25 ++++
>>   drivers/usb/host/ohci-platform.c                   | 148 ++++++++++++++++++---
>>   2 files changed, 153 insertions(+), 20 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/usb/platform-ohci.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/platform-ohci.txt b/Documentation/devicetree/bindings/usb/platform-ohci.txt
>> new file mode 100644
>> index 0000000..44bfa57
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt
>
> ohci-mmio might be a better name than platform maybe?

We already have platform-uhci too, and this matches the name of
the .c file so that people can actually correlate the 2.

>
> [snip]
>
>> +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;
>> +       }
>
> Do we really need to cap this to OHCI_MAX_CLKS, the next driver which
> has 4 clocks will have to bump this, so maybe a linked-list would be
> best?

I can make OHCI_MAX_CLKS 4 or 8 if that makes you happy, other then
that I greatly prefer to keep this KISS. Devices with 3 clks are already
quite rare, so its not like we're going to need to bump this every other
week a linked list where a simple array suffices is just overkill and
unnecessarily complicates things.

>
>> +
>> +       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;
>> +       }
>
> Although I do value the idea of having DT probing for ohci-platform,
> ehci-platform et al, I am not sure if this really belongs in the
> generic OHCI platform driver, or if we should rather have small glue
> drivers which register the ohci-platform driver and which provides all
> the platform-specific power_on, power_off, suspend callbacks to the
> ohci platform driver? Such glue driver would be the one which gets
> probed based off a specific compatible string a

This can already be done by having a platform driver with its own
compatible string providing platform data with the hooks. The whole idea
here is to avoid having to write such a glue driver for every other soc,
so cover the common simple case, and use glue drivers for more complicated
scenarios.

> At first glance it does look like this covers 80% of the existing OHCI
> drivers out there, so this is good. We might also want to support
> clock pointers provided via platform_data so we can remove ohci-at91,
> ohci-ep93xx, ohci-spear and friends in the future.

Lets first get the generic devicetree support merged as is, if after
that people want to extend it, so that (more) existing ohci drivers can
be removed, they are very much welcome to do so. Preferably people
with access to the hardware, so that they can actually test the changes.

> ohci-ppc-of could also probably be removed once we add quirks and
> endian properties parsing to ohci-platform?

Add-on patches extending my initial work are welcome. Again probably
best done by people who actually have access to the relevant hw.

Regards,

Hans

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

[Alan Stern]

On Thu, 9 Jan 2014, Hans de Goede wrote:

> >> +Optional properties:
> >> +- clocks : a list of phandle + clock specifier pairs, one for each entry
> >> +   in clock-names.
> >> +- clock-names : "clk0", "clk1", ...
> >> +- phys : phy
> >> +- phy-names : "phy0"
> >> +
> >> +Example:
> >> +
> >> +	ehci@d8007900 {
> >> +		compatible = "via,vt8500-ehci";
> >> +		reg = <0xd8007900 0x200>;
> >> +		interrupts = <43>;
> >> +		clocks = <&usb_clk 6>, <&ahb_gates 2>;
> >> +		clock-names = "clk0", "clk1";
> >
> > I'm really not convinced by this either. It prevents you from doing
> > anything useful out of these clocks, and the only thing you can
> > actually do with it is calling clk_get, and that's pretty much it.
> >
> > What if you some platform needs to adjust the rate of one of the two?
> 
> Then it needs its own driver. This is intended as a binding for a
> *generic* driver, which is meant to cover simple straight forward
> non-pci ohci cases. For more complex cases a separate driver will
> need to be written.
> 
> I must say I'm becoming a bit unhappy with how the reviews of devicetree
> bindings are being done. In one case it is not generic enough (ahci-sunxi).
> 
> If I then try to make it more generic in a case where that can actually
> be done as the hardware is pretty straight forward, it is not specific enough.
> You can simply not have both!
> 
> > Or wants to cut one but not the other for any reason?
> 
> This is another example of non generic behavior, requiring a separate
> (small using the existing ohci core) platform glue driver, like the *19* we
> already have.

Would DT allow ehci-platform.c to access the clocks by their index in
the array, rather than by name?  After all, you don't really care about
the names at all, since the driver knows nothing about the clocks' 
functions.

The same is true of the phy entry.

Alan Stern

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

[Hans de Goede]

Hi,

On 01/09/2014 04:47 PM, Alan Stern wrote:
> On Thu, 9 Jan 2014, Hans de Goede wrote:
>
>>>> +Optional properties:
>>>> +- clocks : a list of phandle + clock specifier pairs, one for each entry
>>>> +   in clock-names.
>>>> +- clock-names : "clk0", "clk1", ...
>>>> +- phys : phy
>>>> +- phy-names : "phy0"
>>>> +
>>>> +Example:
>>>> +
>>>> +	ehci@d8007900 {
>>>> +		compatible = "via,vt8500-ehci";
>>>> +		reg = <0xd8007900 0x200>;
>>>> +		interrupts = <43>;
>>>> +		clocks = <&usb_clk 6>, <&ahb_gates 2>;
>>>> +		clock-names = "clk0", "clk1";
>>>
>>> I'm really not convinced by this either. It prevents you from doing
>>> anything useful out of these clocks, and the only thing you can
>>> actually do with it is calling clk_get, and that's pretty much it.
>>>
>>> What if you some platform needs to adjust the rate of one of the two?
>>
>> Then it needs its own driver. This is intended as a binding for a
>> *generic* driver, which is meant to cover simple straight forward
>> non-pci ohci cases. For more complex cases a separate driver will
>> need to be written.
>>
>> I must say I'm becoming a bit unhappy with how the reviews of devicetree
>> bindings are being done. In one case it is not generic enough (ahci-sunxi).
>>
>> If I then try to make it more generic in a case where that can actually
>> be done as the hardware is pretty straight forward, it is not specific enough.
>> You can simply not have both!
>>
>>> Or wants to cut one but not the other for any reason?
>>
>> This is another example of non generic behavior, requiring a separate
>> (small using the existing ohci core) platform glue driver, like the *19* we
>> already have.
>
> Would DT allow ehci-platform.c to access the clocks by their index in
> the array, rather than by name?  After all, you don't really care about
> the names at all, since the driver knows nothing about the clocks'
> functions.

Yes, actually I've been discussing this with Maxime on irc and I'm
just done preparing a v3 doing exactly that. I was just checking mail
for any more review comments before pushing out v3 :)

> The same is true of the phy entry.

The phy entry must have a name, this is specified in the phy bindings
in 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
"""

Now that I've dropped the clock-names, I've changed the ugly "phy0" name
to "usb" as the phy pointed to is supposed to be an usb phy.

Regards,

Hans