diff mbox series

[v6,02/13] dt-bindings: misc: Add bindings for HiSilicon usb hub and data role switch functionality on HiKey960

Message ID 20190420064019.57522-3-chenyu56@huawei.com (mailing list archive)
State New, archived
Headers show
Series Add support for usb on Hikey960 | expand

Commit Message

Chen Yu April 20, 2019, 6:40 a.m. UTC 
This patch adds binding documentation to support usb hub and usb
data role switch of Hisilicon HiKey960 Board.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Binghui Wang <wangbinghui@hisilicon.com>
Signed-off-by: Yu Chen <chenyu56@huawei.com>
---
v1:
* Fix some format errors as suggested by Sergei.
* Modify gpio description to use gpiod API.
v2:
* Remove information about Hikey.
* Fix gpio description.
* Remove device_type of endpoint.
v3:
* Remove property typec-vbus-enable-val.
* Add description of pinctrl-names.
* Add example for "hisilicon,gpio-hubv1"
* Add flag in gpiod properties.
---
---
 .../bindings/misc/hisilicon-hikey-usb.txt          | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt

Comments

Rob Herring April 25, 2019, 9:35 p.m. UTC | #1 
On Sat, Apr 20, 2019 at 02:40:08PM +0800, Yu Chen wrote:
> This patch adds binding documentation to support usb hub and usb
> data role switch of Hisilicon HiKey960 Board.

Sorry I've been slow to really review this, but I needed to look at the 
schematics to see what exactly is going on here.

I think this needs some changes to better reflect the h/w and utilize 
existing bindings. It should really be designed ignoring the muxing to 
start with. Define the binding for the TypeC connector and then the host 
hub and make sure they can coexist. Then overlay what you need to switch 
between the 2 modes which AFAICT is just a single GPIO. 

> 
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Binghui Wang <wangbinghui@hisilicon.com>
> Signed-off-by: Yu Chen <chenyu56@huawei.com>
> ---
> v1:
> * Fix some format errors as suggested by Sergei.
> * Modify gpio description to use gpiod API.
> v2:
> * Remove information about Hikey.
> * Fix gpio description.
> * Remove device_type of endpoint.
> v3:
> * Remove property typec-vbus-enable-val.
> * Add description of pinctrl-names.
> * Add example for "hisilicon,gpio-hubv1"
> * Add flag in gpiod properties.
> ---
> ---
>  .../bindings/misc/hisilicon-hikey-usb.txt          | 52 ++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
> new file mode 100644
> index 000000000000..422e844df719
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
> @@ -0,0 +1,52 @@
> +Support usb hub and usb data role switch of Hisilicon HiKey960 Board.
> +
> +-----------------------------
> +
> +Required properties:
> +- compatible: "hisilicon,gpio-hubv1","hisilicon,hikey960-usb"
> +- typec-vbus-gpios: gpio to control the vbus of typeC port

This should be a gpio regulator and then connected to 'vbus-supply' in a 
usb-connector node (see .../bindings/connectors/usb-connector.txt). 

Then you also need the RT1711HWSC TypeC controller in DT. That is 
typically the parent device of the connector node. 

> +- otg-switch-gpios: gpio to switch DP & DM between the hub and typeC port

This probably belongs in USB controller node.

> +- hub-vdd33-en-gpios: gpio to enable the power of hub

This too should be a gpio regulator and then in a hub node. We have 2 
ways to represent hubs. Either as an I2C device or as a child of the 
host controller. The latter is preferred, but I'm not too sure how the 
OF graph connection linking the controller to the TypeC connector will 
work with the usb bus binding.

> +- pinctrl-names: pin configuration state name ("default")
> +- pinctrl-0: pinctrl config
> +
> +Example
> +-----
> +
> +	hisi_hikey_usb: hisi_hikey_usb {
> +		compatible = "hisilicon,hikey960-usb";
> +		typec-vbus-gpios = <&gpio25 2 GPIO_ACTIVE_HIGH>;
> +		otg-switch-gpios = <&gpio25 6 GPIO_ACTIVE_HIGH>;
> +		hub-vdd33-en-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usbhub5734_pmx_func>;
> +
> +		port {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			hikey_usb_ep: endpoint@0 {
> +				reg = <0>;
> +				remote-endpoint = <&dwc3_role_switch_notify>;
> +			};
> +		};
> +	};
> +
> +	hisi_hikey_usb: hisi_hikey_usb {

Perhaps doesn't matter with my above feedback, but I don't understand 
the point of this 2nd example.

> +		compatible = "hisilicon,gpio-hubv1";
> +		typec-vbus-gpios = <&gpio25 2 GPIO_ACTIVE_LOW>;
> +		otg-switch-gpios = <&gpio25 6 GPIO_ACTIVE_HIGH>;
> +		hub-vdd33-en-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usbhub5734_pmx_func>;
> +
> +		port {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			hikey_usb_ep: endpoint@0 {
> +				reg = <0>;
> +				remote-endpoint = <&dwc3_role_switch_notify>;
> +			};
> +		};
> +	};
> -- 
> 2.15.0-rc2
>
Chen Yu April 30, 2019, 6:07 a.m. UTC | #2 
Hi Rob,

On 2019/4/26 5:35, Rob Herring wrote:
> On Sat, Apr 20, 2019 at 02:40:08PM +0800, Yu Chen wrote:
>> This patch adds binding documentation to support usb hub and usb
>> data role switch of Hisilicon HiKey960 Board.
> 
> Sorry I've been slow to really review this, but I needed to look at the 
> schematics to see what exactly is going on here.
> 
> I think this needs some changes to better reflect the h/w and utilize 
> existing bindings. It should really be designed ignoring the muxing to 
> start with. Define the binding for the TypeC connector and then the host 
> hub and make sure they can coexist. Then overlay what you need to switch 
> between the 2 modes which AFAICT is just a single GPIO. 
> 
>>
>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: John Stultz <john.stultz@linaro.org>
>> Cc: Binghui Wang <wangbinghui@hisilicon.com>
>> Signed-off-by: Yu Chen <chenyu56@huawei.com>
>> ---
>> v1:
>> * Fix some format errors as suggested by Sergei.
>> * Modify gpio description to use gpiod API.
>> v2:
>> * Remove information about Hikey.
>> * Fix gpio description.
>> * Remove device_type of endpoint.
>> v3:
>> * Remove property typec-vbus-enable-val.
>> * Add description of pinctrl-names.
>> * Add example for "hisilicon,gpio-hubv1"
>> * Add flag in gpiod properties.
>> ---
>> ---
>>  .../bindings/misc/hisilicon-hikey-usb.txt          | 52 ++++++++++++++++++++++
>>  1 file changed, 52 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
>>
>> diff --git a/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
>> new file mode 100644
>> index 000000000000..422e844df719
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
>> @@ -0,0 +1,52 @@
>> +Support usb hub and usb data role switch of Hisilicon HiKey960 Board.
>> +
>> +-----------------------------
>> +
>> +Required properties:
>> +- compatible: "hisilicon,gpio-hubv1","hisilicon,hikey960-usb"
>> +- typec-vbus-gpios: gpio to control the vbus of typeC port
> 
> This should be a gpio regulator and then connected to 'vbus-supply' in a 
> usb-connector node (see .../bindings/connectors/usb-connector.txt). 
Currently usb-connector node has no "vbus-supply" property and
I do not find process that handles vbus-supply in RT1711H TypeC driver.

> 
> Then you also need the RT1711HWSC TypeC controller in DT. That is 
> typically the parent device of the connector node. 
> 
>> +- otg-switch-gpios: gpio to switch DP & DM between the hub and typeC port
> 
> This probably belongs in USB controller node.
> 
The otg-switch-gpios controls a mux like fsusb30mux. It is related to
the board design of HiKey960. And the state of the mux is decided by
the typeC port state. So I think it is not so good to make it belongs
in USB controller node.

>> +- hub-vdd33-en-gpios: gpio to enable the power of hub
> 
> This too should be a gpio regulator and then in a hub node. We have 2 
> ways to represent hubs. Either as an I2C device or as a child of the 
> host controller. The latter is preferred, but I'm not too sure how the 
> OF graph connection linking the controller to the TypeC connector will 
> work with the usb bus binding.
> 
There is no particular code except the power control for the hub.
The i2c on the hub is not used. So it can not be an I2C device.
Is there such an example that make the hub as a child of the host controller
and control its power?

>> +- pinctrl-names: pin configuration state name ("default")
>> +- pinctrl-0: pinctrl config
>> +
>> +Example
>> +-----
>> +
>> +	hisi_hikey_usb: hisi_hikey_usb {
>> +		compatible = "hisilicon,hikey960-usb";
>> +		typec-vbus-gpios = <&gpio25 2 GPIO_ACTIVE_HIGH>;
>> +		otg-switch-gpios = <&gpio25 6 GPIO_ACTIVE_HIGH>;
>> +		hub-vdd33-en-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&usbhub5734_pmx_func>;
>> +
>> +		port {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			hikey_usb_ep: endpoint@0 {
>> +				reg = <0>;
>> +				remote-endpoint = <&dwc3_role_switch_notify>;
>> +			};
>> +		};
>> +	};
>> +
>> +	hisi_hikey_usb: hisi_hikey_usb {
> 
> Perhaps doesn't matter with my above feedback, but I don't understand 
> the point of this 2nd example.
> 
OK. I will remove it.
>> +		compatible = "hisilicon,gpio-hubv1";
>> +		typec-vbus-gpios = <&gpio25 2 GPIO_ACTIVE_LOW>;
>> +		otg-switch-gpios = <&gpio25 6 GPIO_ACTIVE_HIGH>;
>> +		hub-vdd33-en-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&usbhub5734_pmx_func>;
>> +
>> +		port {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			hikey_usb_ep: endpoint@0 {
>> +				reg = <0>;
>> +				remote-endpoint = <&dwc3_role_switch_notify>;
>> +			};
>> +		};
>> +	};
>> -- 
>> 2.15.0-rc2
>>
> 
> .
> 

Thanks
- Yu Chen
Rob Herring May 1, 2019, 4:27 p.m. UTC | #3 
On Tue, Apr 30, 2019 at 1:08 AM Chen Yu <chenyu56@huawei.com> wrote:
>
> Hi Rob,
>
> On 2019/4/26 5:35, Rob Herring wrote:
> > On Sat, Apr 20, 2019 at 02:40:08PM +0800, Yu Chen wrote:
> >> This patch adds binding documentation to support usb hub and usb
> >> data role switch of Hisilicon HiKey960 Board.
> >
> > Sorry I've been slow to really review this, but I needed to look at the
> > schematics to see what exactly is going on here.
> >
> > I think this needs some changes to better reflect the h/w and utilize
> > existing bindings. It should really be designed ignoring the muxing to
> > start with. Define the binding for the TypeC connector and then the host
> > hub and make sure they can coexist. Then overlay what you need to switch
> > between the 2 modes which AFAICT is just a single GPIO.
> >
> >>
> >> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> >> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: John Stultz <john.stultz@linaro.org>
> >> Cc: Binghui Wang <wangbinghui@hisilicon.com>
> >> Signed-off-by: Yu Chen <chenyu56@huawei.com>
> >> ---
> >> v1:
> >> * Fix some format errors as suggested by Sergei.
> >> * Modify gpio description to use gpiod API.
> >> v2:
> >> * Remove information about Hikey.
> >> * Fix gpio description.
> >> * Remove device_type of endpoint.
> >> v3:
> >> * Remove property typec-vbus-enable-val.
> >> * Add description of pinctrl-names.
> >> * Add example for "hisilicon,gpio-hubv1"
> >> * Add flag in gpiod properties.
> >> ---
> >> ---
> >>  .../bindings/misc/hisilicon-hikey-usb.txt          | 52 ++++++++++++++++++++++
> >>  1 file changed, 52 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
> >> new file mode 100644
> >> index 000000000000..422e844df719
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
> >> @@ -0,0 +1,52 @@
> >> +Support usb hub and usb data role switch of Hisilicon HiKey960 Board.
> >> +
> >> +-----------------------------
> >> +
> >> +Required properties:
> >> +- compatible: "hisilicon,gpio-hubv1","hisilicon,hikey960-usb"
> >> +- typec-vbus-gpios: gpio to control the vbus of typeC port
> >
> > This should be a gpio regulator and then connected to 'vbus-supply' in a
> > usb-connector node (see .../bindings/connectors/usb-connector.txt).
> Currently usb-connector node has no "vbus-supply" property and
> I do not find process that handles vbus-supply in RT1711H TypeC driver.

The patch[1] adding that is posted to the list and may not have landed yet.

Whether the RT1711H TypeC driver handles it or not is not a binding problem.

> > Then you also need the RT1711HWSC TypeC controller in DT. That is
> > typically the parent device of the connector node.
> >
> >> +- otg-switch-gpios: gpio to switch DP & DM between the hub and typeC port
> >
> > This probably belongs in USB controller node.
> >
> The otg-switch-gpios controls a mux like fsusb30mux. It is related to
> the board design of HiKey960. And the state of the mux is decided by
> the typeC port state. So I think it is not so good to make it belongs
> in USB controller node.

Let me put it this way. The gpio property belongs wherever the mux is
represented. In this case, I would expect the graph port representing
the HS port to have 2 endpoints representing the 2 mux outputs. We
don't generally put properties in the endpoint or port nodes, but the
parent nodes.

> >> +- hub-vdd33-en-gpios: gpio to enable the power of hub
> >
> > This too should be a gpio regulator and then in a hub node. We have 2
> > ways to represent hubs. Either as an I2C device or as a child of the
> > host controller. The latter is preferred, but I'm not too sure how the
> > OF graph connection linking the controller to the TypeC connector will
> > work with the usb bus binding.
> >
> There is no particular code except the power control for the hub.
> The i2c on the hub is not used. So it can not be an I2C device.
> Is there such an example that make the hub as a child of the host controller
> and control its power?

Yes, bindings/usb/usb-device.txt.

Rob

[1] https://www.spinics.net/lists/kernel/msg3089136.html
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
new file mode 100644
index 000000000000..422e844df719
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
@@ -0,0 +1,52 @@ 
+Support usb hub and usb data role switch of Hisilicon HiKey960 Board.
+
+-----------------------------
+
+Required properties:
+- compatible: "hisilicon,gpio-hubv1","hisilicon,hikey960-usb"
+- typec-vbus-gpios: gpio to control the vbus of typeC port
+- otg-switch-gpios: gpio to switch DP & DM between the hub and typeC port
+- hub-vdd33-en-gpios: gpio to enable the power of hub
+- pinctrl-names: pin configuration state name ("default")
+- pinctrl-0: pinctrl config
+
+Example
+-----
+
+	hisi_hikey_usb: hisi_hikey_usb {
+		compatible = "hisilicon,hikey960-usb";
+		typec-vbus-gpios = <&gpio25 2 GPIO_ACTIVE_HIGH>;
+		otg-switch-gpios = <&gpio25 6 GPIO_ACTIVE_HIGH>;
+		hub-vdd33-en-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&usbhub5734_pmx_func>;
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			hikey_usb_ep: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&dwc3_role_switch_notify>;
+			};
+		};
+	};
+
+	hisi_hikey_usb: hisi_hikey_usb {
+		compatible = "hisilicon,gpio-hubv1";
+		typec-vbus-gpios = <&gpio25 2 GPIO_ACTIVE_LOW>;
+		otg-switch-gpios = <&gpio25 6 GPIO_ACTIVE_HIGH>;
+		hub-vdd33-en-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&usbhub5734_pmx_func>;
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			hikey_usb_ep: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&dwc3_role_switch_notify>;
+			};
+		};
+	};