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

[PATCH 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                   | 146 ++++++++++++++++++---
 2 files changed, 151 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..6846f1c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt
@@ -0,0 +1,25 @@
+Generic Platform OHCI controller
+
+Required properties:
+ - compatible: Should be "platform-ohci"
+ - reg: Address range of the ohci registers.
+ - interrupts: Should contain the ohci interrupt.
+
+Optional properties:
+ - clocks: array of clocks
+ - clock-names: clock names "ahb" and/or "ohci"
+ - phys: phy
+ - phy-names: "usb_phy"
+
+Example:
+
+	ohci0: ohci@0x01c14400 {
+		compatible = "platform-ohci";
+		reg = <0x01c14400 0x100>;
+		interrupts = <0 64 4>;
+		clocks = <&ahb_gates 2>, <&usb_clk 6>;
+		clock-names = "ahb", "ohci";
+		phys = <&usbphy 1>;
+		phy-names = "usb_phy";
+		status = "disabled";
+	};
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index f351ff5..b7abb44 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -14,11 +14,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 +31,12 @@
 
 #define DRIVER_DESC "OHCI generic platform driver"
 
+struct ohci_platform_priv {
+	struct clk *ahb_clk;
+	struct clk *ohci_clk;
+	struct phy *phy;
+};
+
 static const char hcd_name[] = "ohci-platform";
 
 static int ohci_platform_reset(struct usb_hcd *hcd)
@@ -48,11 +57,79 @@ 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 ret;
+
+	if (!IS_ERR(priv->ohci_clk)) {
+		ret = clk_prepare_enable(priv->ohci_clk);
+		if (ret)
+			return ret;
+	}
+
+	if (!IS_ERR(priv->ahb_clk)) {
+		ret = clk_prepare_enable(priv->ahb_clk);
+		if (ret)
+			goto err_disable_ohci_clk;
+	}
+
+	if (!IS_ERR(priv->phy)) {
+		ret = phy_init(priv->phy);
+		if (ret)
+			goto err_disable_ahb_clk;
+
+		ret = phy_power_on(priv->phy);
+		if (ret)
+			goto err_exit_phy;
+	}
+
+	return 0;
+
+err_exit_phy:
+	phy_exit(priv->phy);
+err_disable_ahb_clk:
+	if (!IS_ERR(priv->ahb_clk))
+		clk_disable_unprepare(priv->ahb_clk);
+err_disable_ohci_clk:
+	if (!IS_ERR(priv->ohci_clk))
+		clk_disable_unprepare(priv->ohci_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;
+
+	if (!IS_ERR(priv->phy)) {
+		phy_power_off(priv->phy);
+		phy_exit(priv->phy);
+	}
+
+	if (!IS_ERR(priv->ahb_clk))
+		clk_disable_unprepare(priv->ahb_clk);
+
+	if (!IS_ERR(priv->ohci_clk))
+		clk_disable_unprepare(priv->ohci_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 +137,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;
+	int 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 +171,30 @@ 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, "usb_phy");
+		if (IS_ERR(priv->phy) && PTR_ERR(priv->phy) == -EPROBE_DEFER) {
+			err = -EPROBE_DEFER;
+			goto err_put_hcd;
+		}
+
+		priv->ohci_clk = devm_clk_get(&dev->dev, "ohci");
+		priv->ahb_clk = devm_clk_get(&dev->dev, "ahb");
+	}
+
+	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 +203,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 +277,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 = "platform-ohci", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ohci_platform_ids);
+
 static const struct platform_device_id ohci_platform_table[] = {
 	{ "ohci-platform", 0 },
 	{ }
@@ -198,6 +303,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 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.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/usb/platform-ehci.txt      |  25 +++++
 .../devicetree/bindings/usb/vt8500-ehci.txt        |  12 --
 drivers/usb/host/ehci-platform.c                   | 125 +++++++++++++++++----
 3 files changed, 131 insertions(+), 31 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/platform-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..aba6da1
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/platform-ehci.txt
@@ -0,0 +1,25 @@
+Generic Platform EHCI controller
+
+Required properties:
+ - compatible: Should be "platform-ehci"
+   (legacy: via,vt8500-ehci" or "wm,prizm-ehci")
+ - reg: Address range of the ehci registers.
+ - interrupts: Should contain the ehci interrupt.
+
+Optional properties:
+ - clocks: array of clocks
+ - clock-names: clock names "ahb" and/or "ehci"
+ - phys: phy
+ - phy-names: "usb_phy"
+
+Example:
+
+usb: ehci@D8007100 {
+	compatible = "platform-ehci";
+	reg = <0xD8007100 0x200>;
+	interrupts = <1>;
+	clocks = <&ahb_gates 2>, <&usb_clk 6>;
+	clock-names = "ahb", "ehci";
+	phys = <&usbphy 1>;
+	phy-names = "usb_phy";
+};
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..01d9a97 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -18,6 +18,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 +26,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 +36,12 @@
 
 #define DRIVER_DESC "EHCI generic platform driver"
 
+struct ehci_platform_priv {
+	struct clk *ahb_clk;
+	struct clk *ehci_clk;
+	struct phy *phy;
+};
+
 static const char hcd_name[] = "ehci-platform";
 
 static int ehci_platform_reset(struct usb_hcd *hcd)
@@ -64,13 +72,79 @@ 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 ret;
+
+	if (!IS_ERR(priv->ehci_clk)) {
+		ret = clk_prepare_enable(priv->ehci_clk);
+		if (ret)
+			return ret;
+	}
+
+	if (!IS_ERR(priv->ahb_clk)) {
+		ret = clk_prepare_enable(priv->ahb_clk);
+		if (ret)
+			goto err_disable_ehci_clk;
+	}
+
+	if (!IS_ERR(priv->phy)) {
+		ret = phy_init(priv->phy);
+		if (ret)
+			goto err_disable_ahb_clk;
+
+		ret = phy_power_on(priv->phy);
+		if (ret)
+			goto err_exit_phy;
+	}
+
+	return 0;
+
+err_exit_phy:
+	phy_exit(priv->phy);
+err_disable_ahb_clk:
+	if (!IS_ERR(priv->ahb_clk))
+		clk_disable_unprepare(priv->ahb_clk);
+err_disable_ehci_clk:
+	if (!IS_ERR(priv->ehci_clk))
+		clk_disable_unprepare(priv->ehci_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;
+
+	if (!IS_ERR(priv->phy)) {
+		phy_power_off(priv->phy);
+		phy_exit(priv->phy);
+	}
+
+	if (!IS_ERR(priv->ahb_clk))
+		clk_disable_unprepare(priv->ahb_clk);
+
+	if (!IS_ERR(priv->ehci_clk))
+		clk_disable_unprepare(priv->ehci_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)
 {
@@ -85,7 +159,7 @@ static int ehci_platform_probe(struct platform_device *dev)
 
 	/*
 	 * use reasonable defaults so platforms don't have to provide these.
-	 * with DT probing on ARM, none of these are set.
+	 * with DT probing on ARM.
 	 */
 	if (!dev_get_platdata(&dev->dev))
 		dev->dev.platform_data = &ehci_platform_defaults;
@@ -107,17 +181,30 @@ 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, "usb_phy");
+		if (IS_ERR(priv->phy) && PTR_ERR(priv->phy) == -EPROBE_DEFER) {
+			err = -EPROBE_DEFER;
+			goto err_put_hcd;
+		}
+
+		priv->ehci_clk = devm_clk_get(&dev->dev, "ehci");
+		priv->ahb_clk = devm_clk_get(&dev->dev, "ahb");
+	}
+
+	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 +213,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;
 }
@@ -203,11 +288,13 @@ static int ehci_platform_resume(struct device *dev)
 #define ehci_platform_resume	NULL
 #endif /* CONFIG_PM */
 
-static const struct of_device_id vt8500_ehci_ids[] = {
+static const struct of_device_id ehci_platform_ids[] = {
 	{ .compatible = "via,vt8500-ehci", },
 	{ .compatible = "wm,prizm-ehci", },
+	{ .compatible = "platform-ehci", },
 	{}
 };
+MODULE_DEVICE_TABLE(of, ehci_platform_ids);
 
 static const struct platform_device_id ehci_platform_table[] = {
 	{ "ehci-platform", 0 },
@@ -229,7 +316,7 @@ static struct platform_driver ehci_platform_driver = {
 		.owner	= THIS_MODULE,
 		.name	= "ehci-platform",
 		.pm	= &ehci_platform_pm_ops,
-		.of_match_table = vt8500_ehci_ids,
+		.of_match_table = ehci_platform_ids,
 	}
 };
 
-- 
1.8.4.2

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

[Arnd Bergmann]

On Monday 06 January 2014, Hans de Goede wrote:
> +Required properties:
> + - compatible: Should be "platform-ohci"
> + - reg: Address range of the ohci registers.
> + - interrupts: Should contain the ohci interrupt.
> +
> +Optional properties:
> + - clocks: array of clocks
> + - clock-names: clock names "ahb" and/or "ohci"
> + - phys: phy
> + - phy-names: "usb_phy"

Maybe just "usb"? It obviously is a phy, so that part of the name
is a bit redundant. Actually, the "usb" part of the name also seems
redundant. Is it possible to make it an anonymous phy reference
without a phy-names property as we often do for clocks?

> +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 ret;
> +
> +	if (!IS_ERR(priv->ohci_clk)) {
> +		ret = clk_prepare_enable(priv->ohci_clk);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!IS_ERR(priv->ahb_clk)) {
> +		ret = clk_prepare_enable(priv->ahb_clk);
> +		if (ret)
> +			goto err_disable_ohci_clk;
> +	}
> +
> +	if (!IS_ERR(priv->phy)) {
> +		ret = phy_init(priv->phy);
> +		if (ret)
> +			goto err_disable_ahb_clk;
> +
> +		ret = phy_power_on(priv->phy);
> +		if (ret)
> +			goto err_exit_phy;
> +	}

Style-wise, I think I'd prefer not storing error values in the
ohci_platform_priv struct, but rather using NULL pointers for
when the clocks or phy references are unused.

> @@ -83,17 +171,30 @@ 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, "usb_phy");
> +		if (IS_ERR(priv->phy) && PTR_ERR(priv->phy) == -EPROBE_DEFER) {
> +			err = -EPROBE_DEFER;
> +			goto err_put_hcd;
> +		}
> +
> +		priv->ohci_clk = devm_clk_get(&dev->dev, "ohci");
> +		priv->ahb_clk = devm_clk_get(&dev->dev, "ahb");
> +	}

I think you have to check the clocks for -EPROBE_DEFER as well here, not
just the PHY. Otherwise you don't know the difference between "no clock
provided", "error getting the clock reference" and "clock controller not
initialized yet, try again".

> @@ -178,6 +277,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 = "platform-ohci", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ohci_platform_ids);

I never liked the "platform-*" naming for compatible properties, the
name of the driver is just based on a linux implementation detail
(the platform bus), which could change. How about just calling the
generic one "ohci" or "usb-ohci"?

	Arnd

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

[Hans de Goede]

Hi,

On 01/06/2014 08:16 AM, Arnd Bergmann wrote:
> On Monday 06 January 2014, Hans de Goede wrote:
>> +Required properties:
>> + - compatible: Should be "platform-ohci"
>> + - reg: Address range of the ohci registers.
>> + - interrupts: Should contain the ohci interrupt.
>> +
>> +Optional properties:
>> + - clocks: array of clocks
>> + - clock-names: clock names "ahb" and/or "ohci"
>> + - phys: phy
>> + - phy-names: "usb_phy"
>
> Maybe just "usb"? It obviously is a phy, so that part of the name
> is a bit redundant. Actually, the "usb" part of the name also seems
> redundant. Is it possible to make it an anonymous phy reference
> without a phy-names property as we often do for clocks?

I'm not a big fan of anonymous references, IE currently the ahci-platform
driver is using an anonymous clk reference, but for sunxi (and ie imx too)
it needs to be extended to 2 clks, which means using names, while
keeping compatibility with the older anonymous using dts (and board)
files.

So I can agree to dropping the _phy, but I would like to keep the "usb"
name I realize the chances are slim of ever needing 2 phys but never
say never ...

>
>> +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 ret;
>> +
>> +	if (!IS_ERR(priv->ohci_clk)) {
>> +		ret = clk_prepare_enable(priv->ohci_clk);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (!IS_ERR(priv->ahb_clk)) {
>> +		ret = clk_prepare_enable(priv->ahb_clk);
>> +		if (ret)
>> +			goto err_disable_ohci_clk;
>> +	}
>> +
>> +	if (!IS_ERR(priv->phy)) {
>> +		ret = phy_init(priv->phy);
>> +		if (ret)
>> +			goto err_disable_ahb_clk;
>> +
>> +		ret = phy_power_on(priv->phy);
>> +		if (ret)
>> +			goto err_exit_phy;
>> +	}
>
> Style-wise, I think I'd prefer not storing error values in the
> ohci_platform_priv struct, but rather using NULL pointers for
> when the clocks or phy references are unused.

This is a style I picked up from the mmc code, ie do a grep for
!IS_ERR in drivers/mmc/host/*.c, but I agree it is not the prettiest,
and on looking in other subsystems it is not common, so I'll convert
this to storing NULL on error to get the phy or clk.

>> @@ -83,17 +171,30 @@ 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, "usb_phy");
>> +		if (IS_ERR(priv->phy) && PTR_ERR(priv->phy) == -EPROBE_DEFER) {
>> +			err = -EPROBE_DEFER;
>> +			goto err_put_hcd;
>> +		}
>> +
>> +		priv->ohci_clk = devm_clk_get(&dev->dev, "ohci");
>> +		priv->ahb_clk = devm_clk_get(&dev->dev, "ahb");
>> +	}
>
> I think you have to check the clocks for -EPROBE_DEFER as well here, not
> just the PHY.

So far I've never seen clk_get return -EPROBE_DEFER, but I guess on some
platforms it can.

> Otherwise you don't know the difference between "no clock
> provided", "error getting the clock reference" and "clock controller not
> initialized yet, try again".

I guess of these 3 we really only want to continue on "no clock provided",
so I think something like this (for both clks and the phy) would be best:

		priv->ahb_clk = devm_clk_get(&dev->dev, "ahb");
		if (IS_ERR(priv->ahb_clk)) {
			err = PTR_ERR(priv->ahb_clk);
			if (err != -EINVAL && err != -ENODATA)
				goto err_put_hcd;
			priv->ahb_clk = NULL; /* No clock provided */
		}

To clarify -EINVAL will be returned when there is no clock-names, and
-ENODATA when the specified name is not found in clock-names.

>
>> @@ -178,6 +277,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 = "platform-ohci", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, ohci_platform_ids);
>
> I never liked the "platform-*" naming for compatible properties, the
> name of the driver is just based on a linux implementation detail
> (the platform bus), which could change. How about just calling the
> generic one "ohci" or "usb-ohci"?

usb-ohci seems free but usb-ehci are already taken by
drivers/usb/host/ehci-ppc-of.c which is as the name implies pretty ppc
specific. And since ehci-platform.c can be build on ppc too, we could
end up with 2 drivers claiming the same compatibility string on ppc.

Also uhci-platform.c is already using platform-uhci, so using
ohci-platform and ehci-platform seems consistent and avoids the
conflict with drivers/usb/host/ehci-ppc-of.c

Regards,

Hans

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

[Mark Rutland]

On Sun, Jan 05, 2014 at 11:04:39PM +0000, 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/platform-ohci.txt      |  25 ++++
>  drivers/usb/host/ohci-platform.c                   | 146 ++++++++++++++++++---
>  2 files changed, 151 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..6846f1c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt
> @@ -0,0 +1,25 @@
> +Generic Platform OHCI controller
> +
> +Required properties:
> + - compatible: Should be "platform-ohci"

To me, "platform-ohci" seems rather Linux-specific. Platform devices are
a Linux abstraction that don't correspond to any particular bus type in
reality.

We have some "*-generic" bindings. A name of that form might be more
appropriate. Or we could choose an arbitrary OHCI implementation's
vendor,ochi string and everything else can declare compatibility with
that.

> + - reg: Address range of the ohci registers.
> + - interrupts: Should contain the ohci interrupt.
> +
> +Optional properties:
> + - clocks: array of clocks
> + - clock-names: clock names "ahb" and/or "ohci"

A description of what the clocks are might be helpful.

How about something like:

- clocks: a list of phandle + clock specifier pairs, one for each entry
  in clock-names.

- clock-names: Should contain:
   * "ahb" - some description here.
   * "ohci" - some description here.

Thanks,
Mark.

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

[Alan Stern]

On Mon, 6 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>
> ---
>  .../devicetree/bindings/usb/platform-ohci.txt      |  25 ++++
>  drivers/usb/host/ohci-platform.c                   | 146 ++++++++++++++++++---
>  2 files changed, 151 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..6846f1c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt
> @@ -0,0 +1,25 @@
> +Generic Platform OHCI controller
> +
> +Required properties:
> + - compatible: Should be "platform-ohci"
> + - reg: Address range of the ohci registers.
> + - interrupts: Should contain the ohci interrupt.
> +
> +Optional properties:
> + - clocks: array of clocks
> + - clock-names: clock names "ahb" and/or "ohci"

Where does "ahb" come from, what does it mean, and how is it relevant 
to generic platforms?

What about platforms that use 3 clocks?

Alan Stern

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

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

[Mark Rutland]

On Sun, Jan 05, 2014 at 11:04:40PM +0000, 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.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/usb/platform-ehci.txt      |  25 +++++
>  .../devicetree/bindings/usb/vt8500-ehci.txt        |  12 --
>  drivers/usb/host/ehci-platform.c                   | 125 +++++++++++++++++----
>  3 files changed, 131 insertions(+), 31 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/platform-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..aba6da1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/platform-ehci.txt
> @@ -0,0 +1,25 @@
> +Generic Platform EHCI controller
> +
> +Required properties:
> + - compatible: Should be "platform-ehci"
> +   (legacy: via,vt8500-ehci" or "wm,prizm-ehci")

Is there any reason for these to be considered legacy? New devices could
just declare their compatibility with one of the existing
implementations, e.g:

ehci {
	compatible  = "some-vendor,ehci", "via,vt8500-ehci";
	...
}

No new compatible string needed in the driver, and we can easily add
quirks support later.

I'm also not a fan of "platform-*" naming, it's leaking a Linux-internal
implementation detail.

> + - reg: Address range of the ehci registers.
> + - interrupts: Should contain the ehci interrupt.
> +
> +Optional properties:
> + - clocks: array of clocks
> + - clock-names: clock names "ahb" and/or "ehci"

It would be nice to define clocks in terms of clock-names, as with my
OCHI comments.

[...]

> @@ -85,7 +159,7 @@ static int ehci_platform_probe(struct platform_device *dev)
>  
>  	/*
>  	 * use reasonable defaults so platforms don't have to provide these.
> -	 * with DT probing on ARM, none of these are set.
> +	 * with DT probing on ARM.

Did you mean to also get rid of the full-stop on the line before?

Thanks,
Mark.

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

[Arnd Bergmann]

On Monday 06 January 2014, Hans de Goede wrote:
> On 01/06/2014 08:16 AM, Arnd Bergmann wrote:
> > On Monday 06 January 2014, Hans de Goede wrote:
> >> +Required properties:
> >> + - compatible: Should be "platform-ohci"
> >> + - reg: Address range of the ohci registers.
> >> + - interrupts: Should contain the ohci interrupt.
> >> +
> >> +Optional properties:
> >> + - clocks: array of clocks
> >> + - clock-names: clock names "ahb" and/or "ohci"
> >> + - phys: phy
> >> + - phy-names: "usb_phy"
> >
> > Maybe just "usb"? It obviously is a phy, so that part of the name
> > is a bit redundant. Actually, the "usb" part of the name also seems
> > redundant. Is it possible to make it an anonymous phy reference
> > without a phy-names property as we often do for clocks?
> 
> I'm not a big fan of anonymous references, IE currently the ahci-platform
> driver is using an anonymous clk reference, but for sunxi (and ie imx too)
> it needs to be extended to 2 clks, which means using names, while
> keeping compatibility with the older anonymous using dts (and board)
> files.
> 
> So I can agree to dropping the _phy, but I would like to keep the "usb"
> name I realize the chances are slim of ever needing 2 phys but never
> say never ...

Ok, fair enough.

> > Style-wise, I think I'd prefer not storing error values in the
> > ohci_platform_priv struct, but rather using NULL pointers for
> > when the clocks or phy references are unused.
> 
> This is a style I picked up from the mmc code, ie do a grep for
> !IS_ERR in drivers/mmc/host/*.c, but I agree it is not the prettiest,
> and on looking in other subsystems it is not common, so I'll convert
> this to storing NULL on error to get the phy or clk.

Ok, thanks.

> >> @@ -83,17 +171,30 @@ 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, "usb_phy");
> >> +		if (IS_ERR(priv->phy) && PTR_ERR(priv->phy) == -EPROBE_DEFER) {
> >> +			err = -EPROBE_DEFER;
> >> +			goto err_put_hcd;
> >> +		}
> >> +
> >> +		priv->ohci_clk = devm_clk_get(&dev->dev, "ohci");
> >> +		priv->ahb_clk = devm_clk_get(&dev->dev, "ahb");
> >> +	}
> >
> > I think you have to check the clocks for -EPROBE_DEFER as well here, not
> > just the PHY.
> 
> So far I've never seen clk_get return -EPROBE_DEFER, but I guess on some
> platforms it can.

Right. Most clock controllers tend to be initialized at early boot
time, but some of them are on external PMICs that are only probed after
i2c or some other subsystem is up.

> > Otherwise you don't know the difference between "no clock
> > provided", "error getting the clock reference" and "clock controller not
> > initialized yet, try again".
> 
> I guess of these 3 we really only want to continue on "no clock provided",
> so I think something like this (for both clks and the phy) would be best:
> 
> 		priv->ahb_clk = devm_clk_get(&dev->dev, "ahb");
> 		if (IS_ERR(priv->ahb_clk)) {
> 			err = PTR_ERR(priv->ahb_clk);
> 			if (err != -EINVAL && err != -ENODATA)
> 				goto err_put_hcd;
> 			priv->ahb_clk = NULL; /* No clock provided */
> 		}
> 
> To clarify -EINVAL will be returned when there is no clock-names, and
> -ENODATA when the specified name is not found in clock-names.

Ok, looks good.

> >> @@ -178,6 +277,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 = "platform-ohci", },
> >> +	{ }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, ohci_platform_ids);
> >
> > I never liked the "platform-*" naming for compatible properties, the
> > name of the driver is just based on a linux implementation detail
> > (the platform bus), which could change. How about just calling the
> > generic one "ohci" or "usb-ohci"?
> 
> usb-ohci seems free but usb-ehci are already taken by
> drivers/usb/host/ehci-ppc-of.c which is as the name implies pretty ppc
> specific. And since ehci-platform.c can be build on ppc too, we could
> end up with 2 drivers claiming the same compatibility string on ppc.
> 
> Also uhci-platform.c is already using platform-uhci, so using
> ohci-platform and ehci-platform seems consistent and avoids the
> conflict with drivers/usb/host/ehci-ppc-of.c

Hmm, that file seems to do two things that ehci-platform doesn't:
It handles big-endian controllers and it has special initialization
for compatible="ibm,usb-ehci-440epx". It also uses a different way
to get to the resources because the driver dates back to before the
unification of platform_bus and of_platform_bus, but that can be
trivially changed.

I would hope that we can eventually merge the two drivers, and I'm
guessing that we will sooner or later need the big-endian support on
non-ppc machines anyway. The question is what to do about the 440epx
hack. It may be small enough that we can just leave it as a quirk
inside #ifdef CONFIG_PPC in the unified driver, or it may end up
being shared with arm64 X-Gene, which seems to share a common
ancestry with some of the ppc4xx socs.

	Arnd

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

[Hans de Goede]

Hi,

On 01/06/2014 04:45 PM, Mark Rutland wrote:
> On Sun, Jan 05, 2014 at 11:04:39PM +0000, 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/platform-ohci.txt      |  25 ++++
>>   drivers/usb/host/ohci-platform.c                   | 146 ++++++++++++++++++---
>>   2 files changed, 151 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..6846f1c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt
>> @@ -0,0 +1,25 @@
>> +Generic Platform OHCI controller
>> +
>> +Required properties:
>> + - compatible: Should be "platform-ohci"
>
> To me, "platform-ohci" seems rather Linux-specific. Platform devices are
> a Linux abstraction that don't correspond to any particular bus type in
> reality.
>
> We have some "*-generic" bindings. A name of that form might be more
> appropriate. Or we could choose an arbitrary OHCI implementation's
> vendor,ochi string and everything else can declare compatibility with
> that.

With the ehci patch I'll go for your suggestion to simply keep
via,vt8500-ehci as compat string for it, without adding a new generic
name. So for ohci I'll also go with the first platform to use it and
thus "allwinner,sun4i-ohci"


>
>> + - reg: Address range of the ohci registers.
>> + - interrupts: Should contain the ohci interrupt.
>> +
>> +Optional properties:
>> + - clocks: array of clocks
>> + - clock-names: clock names "ahb" and/or "ohci"
>
> A description of what the clocks are might be helpful.
>
> How about something like:
>
> - clocks: a list of phandle + clock specifier pairs, one for each entry
>    in clock-names.
>
> - clock-names: Should contain:
>     * "ahb" - some description here.
>     * "ohci" - some description here.

As Alan pointed out by asking "what is ahb" these
names are too platform specific (arm platform in this case),
for a generic driver. So I'm going to call them clk1, clk2 and
instead. It may seem a bit silly to use names at all in this
case, but having names makes things a lot easier
implementation wise.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 01/06/2014 04:49 PM, Alan Stern wrote:
> On Mon, 6 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>
>> ---
>>   .../devicetree/bindings/usb/platform-ohci.txt      |  25 ++++
>>   drivers/usb/host/ohci-platform.c                   | 146 ++++++++++++++++++---
>>   2 files changed, 151 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..6846f1c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt
>> @@ -0,0 +1,25 @@
>> +Generic Platform OHCI controller
>> +
>> +Required properties:
>> + - compatible: Should be "platform-ohci"
>> + - reg: Address range of the ohci registers.
>> + - interrupts: Should contain the ohci interrupt.
>> +
>> +Optional properties:
>> + - clocks: array of clocks
>> + - clock-names: clock names "ahb" and/or "ohci"
>
> Where does "ahb" come from, what does it mean, and how is it relevant
> to generic platforms?

ahb is an ARM specific thing, so your right it does not belong in a
generic driver. I'll use clk1 and clk2 as names in my next version.

> What about platforms that use 3 clocks?

Ah yes I see some platforms have 3 clocks, I'll also add a clk3.

Regards,

Hans

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

[Arnd Bergmann]

On Tuesday 07 January 2014 22:03:11 Hans de Goede wrote:
> >> +
> >> +Optional properties:
> >> + - clocks: array of clocks
> >> + - clock-names: clock names "ahb" and/or "ohci"
> >
> > Where does "ahb" come from, what does it mean, and how is it relevant
> > to generic platforms?
> 
> ahb is an ARM specific thing, so your right it does not belong in a
> generic driver. I'll use clk1 and clk2 as names in my next version.

While AHB is a bus created by ARM Ltd, it's not actually specific
to the ARM architecture. My guess is that it is in fact used on 95%
of all SoCs, so I would leave it at that. For the other clock, I
think that's actually the bus clock for the USB interface, so I would
not call it "ohci" but rather just "usb" or "phy".

I think it's important to distinguish the names and not just use
"clk1" and "clk2", because the driver may actually want to access
a particular clock in some scenario.

> > What about platforms that use 3 clocks?
> 
> Ah yes I see some platforms have 3 clocks, I'll also add a clk3.

I guess we should try to find at least one hardware data sheet
for an actual ohci implementation and look at what the clock
inputs are really called. A lot of the drivers seem to incorrectly
use the name for the clock signal inside of the soc, which tends
to be named after who provides it, not what it's used for.

	Arnd

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

[Hans de Goede]

Hi,

On 01/07/2014 10:16 PM, Arnd Bergmann wrote:
> On Tuesday 07 January 2014 22:03:11 Hans de Goede wrote:
>>>> +
>>>> +Optional properties:
>>>> + - clocks: array of clocks
>>>> + - clock-names: clock names "ahb" and/or "ohci"
>>>
>>> Where does "ahb" come from, what does it mean, and how is it relevant
>>> to generic platforms?
>>
>> ahb is an ARM specific thing, so your right it does not belong in a
>> generic driver. I'll use clk1 and clk2 as names in my next version.
>
> While AHB is a bus created by ARM Ltd, it's not actually specific
> to the ARM architecture. My guess is that it is in fact used on 95%
> of all SoCs, so I would leave it at that. For the other clock, I
> think that's actually the bus clock for the USB interface, so I would
> not call it "ohci" but rather just "usb" or "phy".
>
> I think it's important to distinguish the names and not just use
> "clk1" and "clk2", because the driver may actually want to access
> a particular clock in some scenario.

The idea here is to have a generic driver, if a driver needs to know
about a specific clock, it will likely be another device specific
driver and it can use its own dt-bindings and clock names. I believe
that for a generic driver meant to cover common hardware configs,
simply having X clks and then on power_on enabling clk1, then clk2,
then clk3, etc. and on power off do the same in reverse other is
a good approach.

>
>>> What about platforms that use 3 clocks?
>>
>> Ah yes I see some platforms have 3 clocks, I'll also add a clk3.
>
> I guess we should try to find at least one hardware data sheet
> for an actual ohci implementation and look at what the clock
> inputs are really called. A lot of the drivers seem to incorrectly
> use the name for the clock signal inside of the soc, which tends
> to be named after who provides it, not what it's used for.

I don't know about data-sheets, but an example of a driver
with 3 clocks is drivers/usb/host/ohci-at91.c fwiw it uses
"uhpck", "hclk" and "usb_clk" as clk names.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 01/06/2014 08:50 AM, Hans de Goede wrote:

<snip>

>> Otherwise you don't know the difference between "no clock
>> provided", "error getting the clock reference" and "clock controller not
>> initialized yet, try again".
>
> I guess of these 3 we really only want to continue on "no clock provided",
> so I think something like this (for both clks and the phy) would be best:
>
>          priv->ahb_clk = devm_clk_get(&dev->dev, "ahb");
>          if (IS_ERR(priv->ahb_clk)) {
>              err = PTR_ERR(priv->ahb_clk);
>              if (err != -EINVAL && err != -ENODATA)
>                  goto err_put_hcd;
>              priv->ahb_clk = NULL; /* No clock provided */
>          }
>
> To clarify -EINVAL will be returned when there is no clock-names, and
> -ENODATA when the specified name is not found in clock-names.

Ok, so I've got this wrong, if there is no clk by that name specified
in dt -ENOENT will be returned. Actually -ENOENT is the only
error clk_get and thus devm_clk_get will ever return.

So it seems that clk_get currently is not properly passing along
probe-deferral. To make things future proof I will add a probe
deferral check to the next version of my patch.

Regards,

Hans

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

[Alan Stern]

On Tue, 7 Jan 2014, Hans de Goede wrote:

> Hi,
> 
> On 01/07/2014 10:16 PM, Arnd Bergmann wrote:
> > On Tuesday 07 January 2014 22:03:11 Hans de Goede wrote:
> >>>> +
> >>>> +Optional properties:
> >>>> + - clocks: array of clocks
> >>>> + - clock-names: clock names "ahb" and/or "ohci"
> >>>
> >>> Where does "ahb" come from, what does it mean, and how is it relevant
> >>> to generic platforms?
> >>
> >> ahb is an ARM specific thing, so your right it does not belong in a
> >> generic driver. I'll use clk1 and clk2 as names in my next version.
> >
> > While AHB is a bus created by ARM Ltd, it's not actually specific
> > to the ARM architecture. My guess is that it is in fact used on 95%
> > of all SoCs, so I would leave it at that. For the other clock, I
> > think that's actually the bus clock for the USB interface, so I would
> > not call it "ohci" but rather just "usb" or "phy".
> >
> > I think it's important to distinguish the names and not just use
> > "clk1" and "clk2", because the driver may actually want to access
> > a particular clock in some scenario.
> 
> The idea here is to have a generic driver, if a driver needs to know
> about a specific clock, it will likely be another device specific
> driver and it can use its own dt-bindings and clock names. I believe
> that for a generic driver meant to cover common hardware configs,
> simply having X clks and then on power_on enabling clk1, then clk2,
> then clk3, etc. and on power off do the same in reverse other is
> a good approach.

I agree.

Alan Stern


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