[PATCH 1/2] chipidea: ci_hdrc_imx: Allow handling the clock for an USB phy/hub

[PATCH 1/2] chipidea: ci_hdrc_imx: Allow handling the clock for an USB phy/hub

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

When using external USB PHY or USB hub, it is common that they require a clock
input.

Add a 'clk_phy' clock, so that it can be retrieved from the device tree and 
enabled in the driver, so that the clock can properly drive the external 
USB phy/hub.

Tested on a imx6q-udoo board, that connects via USBH1 to a USB2514 hub.

In this board the USB2514 is clocked from a 24MHz clock that comes from the 
imx6q CLKO2 pin.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 .../devicetree/bindings/usb/ci13xxx-imx.txt          |  2 ++
 drivers/usb/chipidea/ci_hdrc_imx.c                   | 20 +++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
index b4b5b79..07ba38c 100644
--- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
+++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
@@ -18,6 +18,8 @@ Optional properties:
 - vbus-supply: regulator for vbus
 - disable-over-current: disable over current detect
 - external-vbus-divider: enables off-chip resistor divider for Vbus
+- clocks: phandle to the clock that drives the USB hub
+- clock-names: must be "phy"
 
 Examples:
 usb at 02184000 { /* USB OTG */
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index bb5d976..a197748 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -27,6 +27,7 @@ struct ci_hdrc_imx_data {
 	struct usb_phy *phy;
 	struct platform_device *ci_pdev;
 	struct clk *clk;
+	struct clk *clk_phy;
 	struct imx_usbmisc_data *usbmisc_data;
 };
 
@@ -107,10 +108,22 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	data->clk_phy = devm_clk_get(&pdev->dev, "phy");
+	if (IS_ERR(data->clk_phy)) {
+		data->clk_phy = NULL;
+	} else {
+		ret = clk_prepare_enable(data->clk_phy);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Failed to enable clk_phy: %d\n", ret);
+			goto err_clk;
+		}
+	}
+
 	data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);
 	if (IS_ERR(data->phy)) {
 		ret = PTR_ERR(data->phy);
-		goto err_clk;
+		goto err_clk_phy;
 	}
 
 	pdata.phy = data->phy;
@@ -157,6 +170,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 
 disable_device:
 	ci_hdrc_remove_device(data->ci_pdev);
+err_clk_phy:
+	if (data->clk_phy)
+		clk_disable_unprepare(data->clk_phy);
 err_clk:
 	clk_disable_unprepare(data->clk);
 	return ret;
@@ -168,6 +184,8 @@ static int ci_hdrc_imx_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 	ci_hdrc_remove_device(data->ci_pdev);
+	if (data->clk_phy)
+		clk_disable_unprepare(data->clk_phy);
 	clk_disable_unprepare(data->clk);
 
 	return 0;
-- 
1.8.1.2

[PATCH 2/2] ARM: dts: imx6q-udoo: Add USB host support

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Udoo board has USBH1 port connected to a USB2514 hub.

Add support for it.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/boot/dts/imx6q-udoo.dts | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-udoo.dts b/arch/arm/boot/dts/imx6q-udoo.dts
index 1c7f7a1..109b997 100644
--- a/arch/arm/boot/dts/imx6q-udoo.dts
+++ b/arch/arm/boot/dts/imx6q-udoo.dts
@@ -19,6 +19,23 @@
 	memory {
 		reg = <0x10000000 0x40000000>;
 	};
+
+	regulators {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reg_usb_h1_vbus: regulator at 0 {
+			compatible = "regulator-fixed";
+			reg = <0>;
+			regulator-name = "usb_h1_vbus";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			enable-active-high;
+			startup-delay-us = <2>; /* USB2415 requires a POR of 1 us minimum */
+			gpio = <&gpio7 12 0>;
+		};
+	};
 };
 
 &fec {
@@ -29,7 +46,17 @@
 };
 
 &iomuxc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_hog>;
+
 	imx6q-udoo {
+		pinctrl_hog: hoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_GPIO_17__GPIO7_IO12 0x80000000
+				MX6QDL_PAD_NANDF_CS2__CCM_CLKO2 0x130b0
+			>;
+		};
+
 		pinctrl_enet: enetgrp {
 			fsl,pins = <MX6QDL_ENET_PINGRP1>;
 		};
@@ -54,6 +81,13 @@
 	status = "okay";
 };
 
+&usbh1 {
+	vbus-supply = <&reg_usb_h1_vbus>;
+	clocks = <&clks 201>;
+	clock-names = "phy";
+	status = "okay";
+};
+
 &usdhc3 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_usdhc3>;
-- 
1.8.1.2

[PATCH 1/2] chipidea: ci_hdrc_imx: Allow handling the clock for an USB phy/hub

[Shawn Guo]

This should be sent to USB folks.  Copy Peter and Alex.

Shawn

On Thu, Nov 14, 2013 at 12:09:46AM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> When using external USB PHY or USB hub, it is common that they require a clock
> input.
> 
> Add a 'clk_phy' clock, so that it can be retrieved from the device tree and 
> enabled in the driver, so that the clock can properly drive the external 
> USB phy/hub.
> 
> Tested on a imx6q-udoo board, that connects via USBH1 to a USB2514 hub.
> 
> In this board the USB2514 is clocked from a 24MHz clock that comes from the 
> imx6q CLKO2 pin.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  .../devicetree/bindings/usb/ci13xxx-imx.txt          |  2 ++
>  drivers/usb/chipidea/ci_hdrc_imx.c                   | 20 +++++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> index b4b5b79..07ba38c 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -18,6 +18,8 @@ Optional properties:
>  - vbus-supply: regulator for vbus
>  - disable-over-current: disable over current detect
>  - external-vbus-divider: enables off-chip resistor divider for Vbus
> +- clocks: phandle to the clock that drives the USB hub
> +- clock-names: must be "phy"
>  
>  Examples:
>  usb at 02184000 { /* USB OTG */
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index bb5d976..a197748 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -27,6 +27,7 @@ struct ci_hdrc_imx_data {
>  	struct usb_phy *phy;
>  	struct platform_device *ci_pdev;
>  	struct clk *clk;
> +	struct clk *clk_phy;
>  	struct imx_usbmisc_data *usbmisc_data;
>  };
>  
> @@ -107,10 +108,22 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	data->clk_phy = devm_clk_get(&pdev->dev, "phy");
> +	if (IS_ERR(data->clk_phy)) {
> +		data->clk_phy = NULL;
> +	} else {
> +		ret = clk_prepare_enable(data->clk_phy);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"Failed to enable clk_phy: %d\n", ret);
> +			goto err_clk;
> +		}
> +	}
> +
>  	data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);
>  	if (IS_ERR(data->phy)) {
>  		ret = PTR_ERR(data->phy);
> -		goto err_clk;
> +		goto err_clk_phy;
>  	}
>  
>  	pdata.phy = data->phy;
> @@ -157,6 +170,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>  
>  disable_device:
>  	ci_hdrc_remove_device(data->ci_pdev);
> +err_clk_phy:
> +	if (data->clk_phy)
> +		clk_disable_unprepare(data->clk_phy);
>  err_clk:
>  	clk_disable_unprepare(data->clk);
>  	return ret;
> @@ -168,6 +184,8 @@ static int ci_hdrc_imx_remove(struct platform_device *pdev)
>  
>  	pm_runtime_disable(&pdev->dev);
>  	ci_hdrc_remove_device(data->ci_pdev);
> +	if (data->clk_phy)
> +		clk_disable_unprepare(data->clk_phy);
>  	clk_disable_unprepare(data->clk);
>  
>  	return 0;
> -- 
> 1.8.1.2
> 

[PATCH 1/2] chipidea: ci_hdrc_imx: Allow handling the clock for an USB phy/hub

[Russell King - ARM Linux]

On Thu, Nov 14, 2013 at 12:09:46AM -0200, Fabio Estevam wrote:
> @@ -107,10 +108,22 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	data->clk_phy = devm_clk_get(&pdev->dev, "phy");
> +	if (IS_ERR(data->clk_phy)) {
> +		data->clk_phy = NULL;
> +	} else {

Please stop using NULL as a indicator with functions which only return
failure as an error pointer.  Replace the above three lines with

	if (!IS_ERR(data->clk_phy)) {

> +		ret = clk_prepare_enable(data->clk_phy);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"Failed to enable clk_phy: %d\n", ret);
> +			goto err_clk;
> +		}
> +	}
> +
>  	data->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "fsl,usbphy", 0);
>  	if (IS_ERR(data->phy)) {
>  		ret = PTR_ERR(data->phy);
> -		goto err_clk;
> +		goto err_clk_phy;
>  	}
>  
>  	pdata.phy = data->phy;
> @@ -157,6 +170,9 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>  
>  disable_device:
>  	ci_hdrc_remove_device(data->ci_pdev);
> +err_clk_phy:
> +	if (data->clk_phy)

This test should not be necessary if you've nested the error cleanup.

> +		clk_disable_unprepare(data->clk_phy);
>  err_clk:
>  	clk_disable_unprepare(data->clk);
>  	return ret;
> @@ -168,6 +184,8 @@ static int ci_hdrc_imx_remove(struct platform_device *pdev)
>  
>  	pm_runtime_disable(&pdev->dev);
>  	ci_hdrc_remove_device(data->ci_pdev);
> +	if (data->clk_phy)

	if (!IS_ERR(data->clk_phy))

> +		clk_disable_unprepare(data->clk_phy);
>  	clk_disable_unprepare(data->clk);
>  
>  	return 0;
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] chipidea: ci_hdrc_imx: Allow handling the clock for an USB phy/hub

[Fabio Estevam]

On Thu, Nov 14, 2013 at 7:56 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Nov 14, 2013 at 12:09:46AM -0200, Fabio Estevam wrote:
>> @@ -107,10 +108,22 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
>>               return ret;
>>       }
>>
>> +     data->clk_phy = devm_clk_get(&pdev->dev, "phy");
>> +     if (IS_ERR(data->clk_phy)) {
>> +             data->clk_phy = NULL;
>> +     } else {
>
> Please stop using NULL as a indicator with functions which only return
> failure as an error pointer.  Replace the above three lines with

Thanks, fixed it in v2.

[PATCH 1/2] chipidea: ci_hdrc_imx: Allow handling the clock for an USB phy/hub

[Mark Rutland]

On Thu, Nov 14, 2013 at 02:09:46AM +0000, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> When using external USB PHY or USB hub, it is common that they require a clock
> input.
> 
> Add a 'clk_phy' clock, so that it can be retrieved from the device tree and 
> enabled in the driver, so that the clock can properly drive the external 
> USB phy/hub.

As this clock feeds the PHY, it should be described on the PHY node.
It's a property of the PHY itself.

> 
> Tested on a imx6q-udoo board, that connects via USBH1 to a USB2514 hub.
> 
> In this board the USB2514 is clocked from a 24MHz clock that comes from the 
> imx6q CLKO2 pin.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  .../devicetree/bindings/usb/ci13xxx-imx.txt          |  2 ++
>  drivers/usb/chipidea/ci_hdrc_imx.c                   | 20 +++++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> index b4b5b79..07ba38c 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -18,6 +18,8 @@ Optional properties:
>  - vbus-supply: regulator for vbus
>  - disable-over-current: disable over current detect
>  - external-vbus-divider: enables off-chip resistor divider for Vbus
> +- clocks: phandle to the clock that drives the USB hub

If you're using clock-names, define clocks in terms of it:

- clocks: a list of phandle + clock-specifier pairs corresponding to
  entries in clock-names.

> +- clock-names: must be "phy"

s/must be/should contain/

Thanks,
Mark.

[PATCH 2/2] ARM: dts: imx6q-udoo: Add USB host support

[Mark Rutland]

On Thu, Nov 14, 2013 at 02:09:47AM +0000, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Udoo board has USBH1 port connected to a USB2514 hub.
> 
> Add support for it.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/boot/dts/imx6q-udoo.dts | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6q-udoo.dts b/arch/arm/boot/dts/imx6q-udoo.dts
> index 1c7f7a1..109b997 100644
> --- a/arch/arm/boot/dts/imx6q-udoo.dts
> +++ b/arch/arm/boot/dts/imx6q-udoo.dts
> @@ -19,6 +19,23 @@
>  	memory {
>  		reg = <0x10000000 0x40000000>;
>  	};
> +
> +	regulators {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +

This should have a ranges property, or it's not a simple-bus.

Why do you even need this anyway? The regulators can live under the root
quite happily.

Thanks,
Mark.

[PATCH 2/2] ARM: dts: imx6q-udoo: Add USB host support

[Shawn Guo]

On Mon, Dec 02, 2013 at 11:53:03AM +0000, Mark Rutland wrote:
> On Thu, Nov 14, 2013 at 02:09:47AM +0000, Fabio Estevam wrote:
> > From: Fabio Estevam <fabio.estevam@freescale.com>
> > 
> > Udoo board has USBH1 port connected to a USB2514 hub.
> > 
> > Add support for it.
> > 
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> >  arch/arm/boot/dts/imx6q-udoo.dts | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/imx6q-udoo.dts b/arch/arm/boot/dts/imx6q-udoo.dts
> > index 1c7f7a1..109b997 100644
> > --- a/arch/arm/boot/dts/imx6q-udoo.dts
> > +++ b/arch/arm/boot/dts/imx6q-udoo.dts
> > @@ -19,6 +19,23 @@
> >  	memory {
> >  		reg = <0x10000000 0x40000000>;
> >  	};
> > +
> > +	regulators {
> > +		compatible = "simple-bus";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> 
> This should have a ranges property, or it's not a simple-bus.

You're right.  It's not a real bus.  We're having node 'regulators' be a
container for all those fixed regulators, so that we can put them
together and name them in the generic regulator at num way.

Since kernel does not refuse to probe 'simple-bus' that does not have
the 'ranges' property, we forgot about it.  This is the common pattern
used by a lot of existing DTS files.  If we want to improve the thing,
we may need to consider it as a global move.

Shawn

> 
> Why do you even need this anyway? The regulators can live under the root
> quite happily.