[PATCH 1/2] ARM: dts: Enable USB 3503 hub on exynos5250-snow

[PATCH 1/2] ARM: dts: Enable USB 3503 hub on exynos5250-snow

[Vivek Gautam]

The exynos5250-snow has a SMSC USB3503 connected in
hardware only mode like a PHY. Enable support for it,
and add necessary 'reset-gpio' for it.

This is in correspondance to similar patch by Mark Brown
7c1b0ec ARM: dts: Enable USB hub on Arndale

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---

Based on 'for-next' branch of kgene's linux-samsung tree.

 arch/arm/boot/dts/exynos5250-pinctrl.dtsi |    7 +++++++
 arch/arm/boot/dts/exynos5250-snow.dts     |   14 ++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250-pinctrl.dtsi b/arch/arm/boot/dts/exynos5250-pinctrl.dtsi
index 9a49e68..bd8c8f1 100644
--- a/arch/arm/boot/dts/exynos5250-pinctrl.dtsi
+++ b/arch/arm/boot/dts/exynos5250-pinctrl.dtsi
@@ -706,6 +706,13 @@
 			samsung,pin-pud = <0>;
 			samsung,pin-drv = <0>;
 		};
+
+		usb_hub_reset: usb-hub-reset {
+			samsung,pins = "gpe1-0";
+			samsung,pin-function = <1>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
 	};
 
 	pinctrl@10d10000 {
diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
index 1ce1088..2e48f27 100644
--- a/arch/arm/boot/dts/exynos5250-snow.dts
+++ b/arch/arm/boot/dts/exynos5250-snow.dts
@@ -200,6 +200,20 @@
 		samsung,vbus-gpio = <&gpx1 1 0>;
 	};
 
+	usb_hub_bus {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		// SMSC USB3503 connected in hardware only mode as a PHY
+		usb_hub {
+			compatible = "smsc,usb3503a";
+			reset-gpios = <&gpe1 0 1>;
+			pinctrl-0 = <&usb_hub_reset>;
+			pinctrl-names = "default";
+		};
+	};
+
 	fixed-rate-clocks {
 		xxti {
 			compatible = "samsung,clock-xxti";
-- 
1.7.10.4

[PATCH 2/2] ARM: dts: Enable USB 3503 hub on exynos5420-peach-pit

[Vivek Gautam]

The exynos5420-peach-pit has a SMSC USB3503 connected in
hardware only mode like a PHY. Enable support for it,
and add necessary 'reset-gpio' for it.

This is in correspondance to similar patch by Mark Brown
7c1b0ec ARM: dts: Enable USB hub on Arndale

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---

Based on 'for-next' branch of kgene's linux-samsung tree with
following patch:
[PATCH v4 2/2] ARM: dts: Add peach-pit board support
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg29950.html

 arch/arm/boot/dts/exynos5420-peach-pit.dts |   14 ++++++++++++++
 arch/arm/boot/dts/exynos5420-pinctrl.dtsi  |    7 +++++++
 2 files changed, 21 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
index fae33dd..24c1e87 100644
--- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
@@ -58,6 +58,20 @@
 		pinctrl-0 = <&pwm0_out>;
 		pinctrl-names = "default";
 	};
+
+	usb_hub_bus {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		// SMSC USB3503 connected in hardware only mode as a PHY
+		usb_hub {
+			compatible = "smsc,usb3503a";
+			reset-gpios = <&gpx2 4 1>;
+			pinctrl-0 = <&usb_hub_reset>;
+			pinctrl-names = "default";
+		};
+	};
 };
 
 &pinctrl_0 {
diff --git a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
index e62c8eb..8add143 100644
--- a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
+++ b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
@@ -66,6 +66,13 @@
 			samsung,pin-pud = <0>;
 			samsung,pin-drv = <0>;
 		};
+
+		usb_hub_reset: usb-hub-reset {
+			samsung,pins = "gpx2-4";
+			samsung,pin-function = <1>;
+			samsung,pin-pud = <0>;
+			samsung,pin-drv = <0>;
+		};
 	};
 
 	pinctrl@13410000 {
-- 
1.7.10.4

Re: [PATCH 1/2] ARM: dts: Enable USB 3503 hub on exynos5250-snow

[Andreas Färber]

Hello,

Am 05.05.2014 14:30, schrieb Vivek Gautam:
> The exynos5250-snow has a SMSC USB3503 connected in
> hardware only mode like a PHY. Enable support for it,
> and add necessary 'reset-gpio' for it.
> 
> This is in correspondance to similar patch by Mark Brown
> 7c1b0ec ARM: dts: Enable USB hub on Arndale
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> ---
> 
> Based on 'for-next' branch of kgene's linux-samsung tree.
> 
>  arch/arm/boot/dts/exynos5250-pinctrl.dtsi |    7 +++++++
>  arch/arm/boot/dts/exynos5250-snow.dts     |   14 ++++++++++++++
>  2 files changed, 21 insertions(+)

The same snippet as for snow also fixed my USB issues on spring.
Should it go into exynos5250-cros-common.dtsi instead?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH 1/2] ARM: dts: Enable USB 3503 hub on exynos5250-snow

[Tomasz Figa]

Hi Vivek,

On 05.05.2014 14:30, Vivek Gautam wrote:
> The exynos5250-snow has a SMSC USB3503 connected in
> hardware only mode like a PHY. Enable support for it,
> and add necessary 'reset-gpio' for it.
> 
> This is in correspondance to similar patch by Mark Brown
> 7c1b0ec ARM: dts: Enable USB hub on Arndale
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> ---
> 
> Based on 'for-next' branch of kgene's linux-samsung tree.
> 
>  arch/arm/boot/dts/exynos5250-pinctrl.dtsi |    7 +++++++
>  arch/arm/boot/dts/exynos5250-snow.dts     |   14 ++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5250-pinctrl.dtsi b/arch/arm/boot/dts/exynos5250-pinctrl.dtsi
> index 9a49e68..bd8c8f1 100644
> --- a/arch/arm/boot/dts/exynos5250-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/exynos5250-pinctrl.dtsi
> @@ -706,6 +706,13 @@
>  			samsung,pin-pud = <0>;
>  			samsung,pin-drv = <0>;
>  		};
> +
> +		usb_hub_reset: usb-hub-reset {
> +			samsung,pins = "gpe1-0";
> +			samsung,pin-function = <1>;
> +			samsung,pin-pud = <0>;
> +			samsung,pin-drv = <0>;
> +		};

This is not a generic pin config group and should not be added to this
file. Btw. it just sets the pin to output, which is what the consumer
driver can do as well by calling gpio_direction_output. Do you really
need this?

>  	};
>  
>  	pinctrl@10d10000 {
> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
> index 1ce1088..2e48f27 100644
> --- a/arch/arm/boot/dts/exynos5250-snow.dts
> +++ b/arch/arm/boot/dts/exynos5250-snow.dts
> @@ -200,6 +200,20 @@
>  		samsung,vbus-gpio = <&gpx1 1 0>;
>  	};
>  
> +	usb_hub_bus {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <0>;

Why do you need this bus?

> +
> +		// SMSC USB3503 connected in hardware only mode as a PHY

Wrong comment style. Also basically the comment says exactly the same as
could be inferred from the node below.

> +		usb_hub {
> +			compatible = "smsc,usb3503a";
> +			reset-gpios = <&gpe1 0 1>;
> +			pinctrl-0 = <&usb_hub_reset>;
> +			pinctrl-names = "default";

As above, do you really need to use pinctrl here?

I believe the same comments apply for patch 2/2 as well.

Best regards,
Tomasz

Re: [PATCH 1/2] ARM: dts: Enable USB 3503 hub on exynos5250-snow

[Tomasz Figa]

On 08.05.2014 17:59, Andreas Färber wrote:
> Hello,
> 
> Am 05.05.2014 14:30, schrieb Vivek Gautam:
>> The exynos5250-snow has a SMSC USB3503 connected in
>> hardware only mode like a PHY. Enable support for it,
>> and add necessary 'reset-gpio' for it.
>>
>> This is in correspondance to similar patch by Mark Brown
>> 7c1b0ec ARM: dts: Enable USB hub on Arndale
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> ---
>>
>> Based on 'for-next' branch of kgene's linux-samsung tree.
>>
>>  arch/arm/boot/dts/exynos5250-pinctrl.dtsi |    7 +++++++
>>  arch/arm/boot/dts/exynos5250-snow.dts     |   14 ++++++++++++++
>>  2 files changed, 21 insertions(+)
> 
> The same snippet as for snow also fixed my USB issues on spring.
> Should it go into exynos5250-cros-common.dtsi instead?

The GPIO pins used are different, so I don't think the hub node added by
these patches could be shared.

Also, I believe all you need to add is a simple node as follows

usb_hub {
	compatible = "smsc,usb3503a";
	reset-gpios = <&gpe1 0 1>;
};

at root level (see my comments to this patch in another reply), which is
affordable to be duplicated for every platform that requires it,
especially if the reset-gpios property differs between boards.

Best regards,
Tomasz

Re: [PATCH 1/2] ARM: dts: Enable USB 3503 hub on exynos5250-snow

[Julius Werner]

We originally tried using this driver on ChromiumOS and never got it
to work reliably. IIRC the issue was that if the hub had already been
initialized by firmware, the USB stack might enumerate it before the
usb3503 driver is probed and then the later reset will silently
disrupt that connection. (I think I tried to force the 3503 to probe
earlier as well, and there was some other issue with that although I
don't recall the details.)

This will not be an issue for the Snow and Peach_Pi(t) boards (since
neither of them shipped with firmware that supports this hub), but it
will be an issue for Spring and Skate. On ChromiumOS we decided to
carry a local (and admittedly ugly) patch to pull that reset line from
the USB PHY driver instead, since that's the only way I could get it
to work in all cases (see http://crosreview.com/58963).

This doesn't mean I'm against this patch per se, just wanted to point
out the trade-offs.

Re: [PATCH 1/2] ARM: dts: Enable USB 3503 hub on exynos5250-snow

[Vivek Gautam]

Hi Julius,


On Thu, May 29, 2014 at 4:07 AM, Julius Werner <jwerner@chromium.org> wrote:
> We originally tried using this driver on ChromiumOS and never got it
> to work reliably. IIRC the issue was that if the hub had already been
> initialized by firmware, the USB stack might enumerate it before the
> usb3503 driver is probed and then the later reset will silently
> disrupt that connection. (I think I tried to force the 3503 to probe
> earlier as well, and there was some other issue with that although I
> don't recall the details.)

Ok, there was already a patch posted by you for this[1], which had
quite a much discussion
on it.
Would you like to give some pointers based on that ?
One that Olof had suggested was to use gpio-reset driver which is yet
to make to mainline.
But i think with that too we need to take care of the timing for
resetting the hub before PHY
gets reset.

>
> This will not be an issue for the Snow and Peach_Pi(t) boards (since
> neither of them shipped with firmware that supports this hub), but it
> will be an issue for Spring and Skate. On ChromiumOS we decided to
> carry a local (and admittedly ugly) patch to pull that reset line from
> the USB PHY driver instead, since that's the only way I could get it
> to work in all cases (see http://crosreview.com/58963).
>
> This doesn't mean I'm against this patch per se, just wanted to point
> out the trade-offs.

Thanks for raising the concern, this needs to be addressed.

[1] https://lkml.org/lkml/2013/7/9/486


-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

Re: [PATCH 1/2] ARM: dts: Enable USB 3503 hub on exynos5250-snow

[Julius Werner]

> Ok, there was already a patch posted by you for this[1], which had
> quite a much discussion
> on it.
> Would you like to give some pointers based on that ?
> One that Olof had suggested was to use gpio-reset driver which is yet
> to make to mainline.
> But i think with that too we need to take care of the timing for
> resetting the hub before PHY
> gets reset.

Oh, right, I remember now. Seems like there wasn't much consensus on
how to solve the problem there, and I think I didn't really have time
to work on it any more either.

I think coupling in another driver to reset the device isn't a bad
idea... this could even be usb3503 if you modified it a little, and it
could also address Tomasz' concerns from that thread that the solution
should be extensible to other HSIC devices that need a different reset
sequence. But you need some mechanism to hook the two drivers together
and make sure phy-samsung-usb2 synchronously calls the reset function
at exactly the right point to ensure the timing works right.