[v7,7/8] dt-bindings: misc: Add bindings for HiSilicon usb hub and data role switch functionality on HiKey960

Commit Message

John Stultz Dec. 12, 2019, 1:42 a.m. UTC

From: Yu Chen <[e-mail]>

This patch adds binding documentation to support usb hub and usb
data role switch of Hisilicon HiKey960 Board.

Cc: Greg Kroah-Hartman <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Mark Rutland <[e-mail]>
CC: ShuFan Lee <[e-mail]>
Cc: Heikki Krogerus <[e-mail]>
Cc: Suzuki K Poulose <[e-mail]>
Cc: Chunfeng Yun <[e-mail]>
Cc: Yu Chen <[e-mail]>
Cc: Felipe Balbi <[e-mail]>
Cc: Hans de Goede <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Jun Li <[e-mail]>
Cc: Valentin Schneider <[e-mail]>
Cc: Guillaume Gardet <[e-mail]>
Cc: Jack Pham <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Yu Chen <[e-mail]>
Signed-off-by: John Stultz <[e-mail]>
---
v3: Reworked as usb-role-switch intermediary

v7: Switched over to YAML dt binding description
---
 .../bindings/misc/hisilicon-hikey-usb.yaml    | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml

Comments

Rob Herring (Arm) Dec. 18, 2019, 4:37 p.m. UTC | #1

On Thu, Dec 12, 2019 at 01:42:32AM +0000, John Stultz wrote:
> From: Yu Chen <[e-mail]>
> 
> This patch adds binding documentation to support usb hub and usb
> data role switch of Hisilicon HiKey960 Board.
> 
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Mark Rutland <[e-mail]>
> CC: ShuFan Lee <[e-mail]>
> Cc: Heikki Krogerus <[e-mail]>
> Cc: Suzuki K Poulose <[e-mail]>
> Cc: Chunfeng Yun <[e-mail]>
> Cc: Yu Chen <[e-mail]>
> Cc: Felipe Balbi <[e-mail]>
> Cc: Hans de Goede <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>
> Cc: Jun Li <[e-mail]>
> Cc: Valentin Schneider <[e-mail]>
> Cc: Guillaume Gardet <[e-mail]>
> Cc: Jack Pham <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Yu Chen <[e-mail]>
> Signed-off-by: John Stultz <[e-mail]>
> ---
> v3: Reworked as usb-role-switch intermediary
> 
> v7: Switched over to YAML dt binding description
> ---
>  .../bindings/misc/hisilicon-hikey-usb.yaml    | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
> new file mode 100644
> index 000000000000..1fc3b198ef73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2019 Linaro Ltd.
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/misc/hisilicon-hikey-usb.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: HiKey960 onboard USB GPIO Hub
> +
> +maintainers:
> +  - John Stultz <[e-mail]>
> +
> +description: |
> +  Supports the onboard HiKey960 USB GPIO hub, which acts as a
> +  role-switch intermediary to detect the state of the USB-C
> +  port, to switch the hub into dual-role USB-C or host mode,
> +  which enables the onboard USB-A host ports.

Honestly I'm torn between whatever works for you because this is pretty 
"special" dev board design and it should more accurately match the 
hardware design. I think we can do the later and it doesn't really need 
anything new.

> +
> +  Schematics about the hub can be found here:
> +    https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: hisilicon,gpio_hubv1

As a whole this is HiSilicon specific, but really it is not. It's really 
just a hub, a mux, and connectors for which we have bindings for. I 
think you need to model the actual hub in DT. We have 2 ways already to 
describe hubs in DT: a I2C device or USB device. 

AIUI, the board looks something like this:

ctrl -> mux --> hub -> type-a connector
            +-> type-c connector

If the hub I2C is not used, then you could do something like this:

ctrl {
    mux-controls = <&usb_gpio_mux>;
    connector@0 {
	// type C connector binding
    };
    hub@1 {
	// USB device binding
    };
};

Or if I2C is used and the hub is under the I2C controller:

ctrl {
    port@0 {
        mux-controls = <&usb_gpio_mux>;
        endpoint@0 { // mux state 0
		remote-endpoint = <&usb_c_connector_port>;
	};
        endpoint@1 { // mux state 1
		remote-endpoint = <&usb_hub_port>;
	};
};

The only new bindings you really need are adding 'mux-controls' to the 
USB host controller and the hub binding (we already have a few).

If the USB2 and USB3 signals come from 2 different host controller 
nodes, then I think it will need to look like the 2nd case regardless 
of I2C. (It's strange that USB3 was not routed to Type-C connector. Can 
you do USB2 on Type-C and USB3 on hub simultaneously? You need USB2 to 
enumerate, right?)

> +
> +  typec-vbus-gpios:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the typec-vbus gpio

This should be modeled as a GPIO regulator, and belongs as part of a 
connector node. See bindings/connector/usb-connector.txt.

> +
> +  otg-switch-gpios:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the otg-switch gpio

This would be the gpio-mux binding instead.

> +
> +  hub-vdd33-en-gpios:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the hub 3.3v power enablement gpio

This should be modeled as a GPIO regulator. 

What about the reset line on the hub?

> +
> +  usb-role-switch:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: Support role switch.

This normally is a controller property. Role switch is foreign to the 
hub, so doesn't really belong there for sure.

> +
> +  port:
> +    description: |
> +      any connector to the data bus of this controller should be modelled
> +      using the OF graph bindings specified, if the "usb-role-switch"
> +      property is used. Note for this driver, two ports are supported,
> +      the first being the endpoint that will be notified by this driver,
> +      and the second being the endpoint that notifies this driver of a
> +      role switch.
> +
> +
> +required:
> +  - compatible
> +  - typec-vbus-gpios
> +  - otg-switch-gpios
> +  - hub-vdd33-en-gpios
> +  - usb-role-switch
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    hisi_hikey_usb: hisi_hikey_usb {
> +        compatible = "hisilicon,gpio_hubv1";
> +        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>;
> +        usb-role-switch;
> +
> +        port {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            hikey_usb_ep0: endpoint@0 {
> +                reg = <0>;
> +                remote-endpoint = <&dwc3_role_switch>;
> +            };
> +            hikey_usb_ep1: endpoint@1 {
> +                reg = <1>;
> +                remote-endpoint = <&rt1711h_ep>;
> +            };
> +        };
> +    };
> -- 
> 2.17.1
>

John Stultz Dec. 18, 2019, 5:21 p.m. UTC | #2

On Wed, Dec 18, 2019 at 8:37 AM Rob Herring <[e-mail]> wrote:
>
> On Thu, Dec 12, 2019 at 01:42:32AM +0000, John Stultz wrote:
> > From: Yu Chen <[e-mail]>
> >
> > This patch adds binding documentation to support usb hub and usb
> > data role switch of Hisilicon HiKey960 Board.
> >
> > Cc: Greg Kroah-Hartman <[e-mail]>
> > Cc: Rob Herring <[e-mail]>
> > Cc: Mark Rutland <[e-mail]>
> > CC: ShuFan Lee <[e-mail]>
> > Cc: Heikki Krogerus <[e-mail]>
> > Cc: Suzuki K Poulose <[e-mail]>
> > Cc: Chunfeng Yun <[e-mail]>
> > Cc: Yu Chen <[e-mail]>
> > Cc: Felipe Balbi <[e-mail]>
> > Cc: Hans de Goede <[e-mail]>
> > Cc: Andy Shevchenko <[e-mail]>
> > Cc: Jun Li <[e-mail]>
> > Cc: Valentin Schneider <[e-mail]>
> > Cc: Guillaume Gardet <[e-mail]>
> > Cc: Jack Pham <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Signed-off-by: Yu Chen <[e-mail]>
> > Signed-off-by: John Stultz <[e-mail]>
> > ---
> > v3: Reworked as usb-role-switch intermediary
> >
> > v7: Switched over to YAML dt binding description
> > ---
> >  .../bindings/misc/hisilicon-hikey-usb.yaml    | 85 +++++++++++++++++++
> >  1 file changed, 85 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
> > new file mode 100644
> > index 000000000000..1fc3b198ef73
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
> > @@ -0,0 +1,85 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2019 Linaro Ltd.
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/misc/hisilicon-hikey-usb.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: HiKey960 onboard USB GPIO Hub
> > +
> > +maintainers:
> > +  - John Stultz <[e-mail]>
> > +
> > +description: |
> > +  Supports the onboard HiKey960 USB GPIO hub, which acts as a
> > +  role-switch intermediary to detect the state of the USB-C
> > +  port, to switch the hub into dual-role USB-C or host mode,
> > +  which enables the onboard USB-A host ports.
>
> Honestly I'm torn between whatever works for you because this is pretty
> "special" dev board design and it should more accurately match the
> hardware design. I think we can do the later and it doesn't really need
> anything new.
>
> > +
> > +  Schematics about the hub can be found here:
> > +    https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: hisilicon,gpio_hubv1
>
> As a whole this is HiSilicon specific, but really it is not. It's really
> just a hub, a mux, and connectors for which we have bindings for. I
> think you need to model the actual hub in DT. We have 2 ways already to
> describe hubs in DT: a I2C device or USB device.
>
> AIUI, the board looks something like this:
>
> ctrl -> mux --> hub -> type-a connector
>             +-> type-c connector
>
> If the hub I2C is not used, then you could do something like this:
>
> ctrl {
>     mux-controls = <&usb_gpio_mux>;
>     connector@0 {
>         // type C connector binding
>     };
>     hub@1 {
>         // USB device binding
>     };
> };

I can't say I totally grok all this, but I'll go digging to try to
better understand it.
I don't believe there is any I2C involved here, so I'll try the
approach you outline above.



> Or if I2C is used and the hub is under the I2C controller:
>
> ctrl {
>     port@0 {
>         mux-controls = <&usb_gpio_mux>;
>         endpoint@0 { // mux state 0
>                 remote-endpoint = <&usb_c_connector_port>;
>         };
>         endpoint@1 { // mux state 1
>                 remote-endpoint = <&usb_hub_port>;
>         };
> };
>
> The only new bindings you really need are adding 'mux-controls' to the
> USB host controller and the hub binding (we already have a few).
>
> If the USB2 and USB3 signals come from 2 different host controller
> nodes, then I think it will need to look like the 2nd case regardless
> of I2C. (It's strange that USB3 was not routed to Type-C connector. Can
> you do USB2 on Type-C and USB3 on hub simultaneously? You need USB2 to
> enumerate, right?)

Yea, it is strange, and I unfortunately don't know why only USB2 was
exported to the type-c connector.
And to my knowledge, you cannot use both the type-c and hub simultaneously.


> > +
> > +  typec-vbus-gpios:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle to the typec-vbus gpio
>
> This should be modeled as a GPIO regulator, and belongs as part of a
> connector node. See bindings/connector/usb-connector.txt.
>
> > +
> > +  otg-switch-gpios:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle to the otg-switch gpio
>
> This would be the gpio-mux binding instead.
>
> > +
> > +  hub-vdd33-en-gpios:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle to the hub 3.3v power enablement gpio
>
> This should be modeled as a GPIO regulator.
>
> What about the reset line on the hub?

Unknown. I don't have any details on that.


> > +
> > +  usb-role-switch:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description: Support role switch.
>
> This normally is a controller property. Role switch is foreign to the
> hub, so doesn't really belong there for sure.

So this part was critical to being able to get role switch
notification from the connector and to properly switch modes without
adding extra notifier gunk from the previous patch that folks didn't
like.

Trying to understand further,  your suggestion here is to re-model the
binding, as gpio regulators and gpio muxes, and use a usb-connector
node to describe them,  but I'm missing how I connect that to the
driver implementation I have? Is the idea to extend the rt1711h and
dwc3 drivers further to support the mux/hub bit (this part is fairly
foggy to me), completely removing the need for the misc driver?

I did take an attempt at something similar with an earlier iteration
of the patch set, where I was trying to move the vbus-gpio as a
gpio-regulator to be controlled by the rt1711h/tpcm core, but that
approach didn't work properly and Hans suggested I just go back to the
approach submitted here:
  https://lkml.org/lkml/2019/10/22/42

thanks
-john

Rob Herring (Arm) Dec. 18, 2019, 7:56 p.m. UTC | #3

On Wed, Dec 18, 2019 at 11:21 AM John Stultz <[e-mail]> wrote:
>
> On Wed, Dec 18, 2019 at 8:37 AM Rob Herring <[e-mail]> wrote:
> >
> > On Thu, Dec 12, 2019 at 01:42:32AM +0000, John Stultz wrote:
> > > From: Yu Chen <[e-mail]>
> > >
> > > This patch adds binding documentation to support usb hub and usb
> > > data role switch of Hisilicon HiKey960 Board.
> > >
> > > Cc: Greg Kroah-Hartman <[e-mail]>
> > > Cc: Rob Herring <[e-mail]>
> > > Cc: Mark Rutland <[e-mail]>
> > > CC: ShuFan Lee <[e-mail]>
> > > Cc: Heikki Krogerus <[e-mail]>
> > > Cc: Suzuki K Poulose <[e-mail]>
> > > Cc: Chunfeng Yun <[e-mail]>
> > > Cc: Yu Chen <[e-mail]>
> > > Cc: Felipe Balbi <[e-mail]>
> > > Cc: Hans de Goede <[e-mail]>
> > > Cc: Andy Shevchenko <[e-mail]>
> > > Cc: Jun Li <[e-mail]>
> > > Cc: Valentin Schneider <[e-mail]>
> > > Cc: Guillaume Gardet <[e-mail]>
> > > Cc: Jack Pham <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Signed-off-by: Yu Chen <[e-mail]>
> > > Signed-off-by: John Stultz <[e-mail]>
> > > ---
> > > v3: Reworked as usb-role-switch intermediary
> > >
> > > v7: Switched over to YAML dt binding description
> > > ---
> > >  .../bindings/misc/hisilicon-hikey-usb.yaml    | 85 +++++++++++++++++++
> > >  1 file changed, 85 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
> > > new file mode 100644
> > > index 000000000000..1fc3b198ef73
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
> > > @@ -0,0 +1,85 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +# Copyright 2019 Linaro Ltd.
> > > +%YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/misc/hisilicon-hikey-usb.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: HiKey960 onboard USB GPIO Hub
> > > +
> > > +maintainers:
> > > +  - John Stultz <[e-mail]>
> > > +
> > > +description: |
> > > +  Supports the onboard HiKey960 USB GPIO hub, which acts as a
> > > +  role-switch intermediary to detect the state of the USB-C
> > > +  port, to switch the hub into dual-role USB-C or host mode,
> > > +  which enables the onboard USB-A host ports.
> >
> > Honestly I'm torn between whatever works for you because this is pretty
> > "special" dev board design and it should more accurately match the
> > hardware design. I think we can do the later and it doesn't really need
> > anything new.
> >
> > > +
> > > +  Schematics about the hub can be found here:
> > > +    https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: hisilicon,gpio_hubv1
> >
> > As a whole this is HiSilicon specific, but really it is not. It's really
> > just a hub, a mux, and connectors for which we have bindings for. I
> > think you need to model the actual hub in DT. We have 2 ways already to
> > describe hubs in DT: a I2C device or USB device.
> >
> > AIUI, the board looks something like this:
> >
> > ctrl -> mux --> hub -> type-a connector
> >             +-> type-c connector
> >
> > If the hub I2C is not used, then you could do something like this:
> >
> > ctrl {
> >     mux-controls = <&usb_gpio_mux>;
> >     connector@0 {
> >         // type C connector binding
> >     };
> >     hub@1 {
> >         // USB device binding
> >     };
> > };
>
> I can't say I totally grok all this, but I'll go digging to try to
> better understand it.
> I don't believe there is any I2C involved here, so I'll try the
> approach you outline above.

Well, it is there in the schematics.

> > Or if I2C is used and the hub is under the I2C controller:
> >
> > ctrl {
> >     port@0 {
> >         mux-controls = <&usb_gpio_mux>;
> >         endpoint@0 { // mux state 0
> >                 remote-endpoint = <&usb_c_connector_port>;
> >         };
> >         endpoint@1 { // mux state 1
> >                 remote-endpoint = <&usb_hub_port>;
> >         };
> > };
> >
> > The only new bindings you really need are adding 'mux-controls' to the
> > USB host controller and the hub binding (we already have a few).
> >
> > If the USB2 and USB3 signals come from 2 different host controller
> > nodes, then I think it will need to look like the 2nd case regardless
> > of I2C. (It's strange that USB3 was not routed to Type-C connector. Can
> > you do USB2 on Type-C and USB3 on hub simultaneously? You need USB2 to
> > enumerate, right?)
>
> Yea, it is strange, and I unfortunately don't know why only USB2 was
> exported to the type-c connector.
> And to my knowledge, you cannot use both the type-c and hub simultaneously.
>
>
> > > +
> > > +  typec-vbus-gpios:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description: phandle to the typec-vbus gpio
> >
> > This should be modeled as a GPIO regulator, and belongs as part of a
> > connector node. See bindings/connector/usb-connector.txt.
> >
> > > +
> > > +  otg-switch-gpios:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description: phandle to the otg-switch gpio
> >
> > This would be the gpio-mux binding instead.
> >
> > > +
> > > +  hub-vdd33-en-gpios:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description: phandle to the hub 3.3v power enablement gpio
> >
> > This should be modeled as a GPIO regulator.
> >
> > What about the reset line on the hub?
>
> Unknown. I don't have any details on that.

You might just be getting lucky that it is pulled to the right state.


> > > +  usb-role-switch:
> > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > +    description: Support role switch.
> >
> > This normally is a controller property. Role switch is foreign to the
> > hub, so doesn't really belong there for sure.
>
> So this part was critical to being able to get role switch
> notification from the connector and to properly switch modes without
> adding extra notifier gunk from the previous patch that folks didn't
> like.
>
> Trying to understand further,  your suggestion here is to re-model the
> binding, as gpio regulators and gpio muxes, and use a usb-connector
> node to describe them,  but I'm missing how I connect that to the
> driver implementation I have?

Good question, but that shouldn't really dictate your binding design.

> Is the idea to extend the rt1711h and
> dwc3 drivers further to support the mux/hub bit (this part is fairly
> foggy to me), completely removing the need for the misc driver?

I imagine that you need some driver to determine the state of the mux.
Perhaps a usb-mux driver which is instantiated by the host controller
driver when it sees a mux-controls property. Sorry, haven't looked at
the driver side of this at all.

> I did take an attempt at something similar with an earlier iteration
> of the patch set, where I was trying to move the vbus-gpio as a
> gpio-regulator to be controlled by the rt1711h/tpcm core, but that
> approach didn't work properly and Hans suggested I just go back to the
> approach submitted here:
>   https://lkml.org/lkml/2019/10/22/42

I don't see why that would matter. If you need to sense the Vbus
state, then you do need a GPIO typically. But for an enable line, it's
just another level of abstraction.

Rob

John Stultz Jan. 22, 2020, 6:28 p.m. UTC | #4

(Sorry for the slow reply. The holidays and other priorities struck
and I'm only now just getting back to this!)

On Wed, Dec 18, 2019 at 11:57 AM Rob Herring <[e-mail]> wrote:
> On Wed, Dec 18, 2019 at 11:21 AM John Stultz <[e-mail]> wrote:
> > On Wed, Dec 18, 2019 at 8:37 AM Rob Herring <[e-mail]> wrote:
> > > As a whole this is HiSilicon specific, but really it is not. It's really
> > > just a hub, a mux, and connectors for which we have bindings for. I
> > > think you need to model the actual hub in DT. We have 2 ways already to
> > > describe hubs in DT: a I2C device or USB device.
> > >
> > > AIUI, the board looks something like this:
> > >
> > > ctrl -> mux --> hub -> type-a connector
> > >             +-> type-c connector
> > >
> > > If the hub I2C is not used, then you could do something like this:
> > >
> > > ctrl {
> > >     mux-controls = <&usb_gpio_mux>;
> > >     connector@0 {
> > >         // type C connector binding
> > >     };
> > >     hub@1 {
> > >         // USB device binding
> > >     };
> > > };
> >
> > I can't say I totally grok all this, but I'll go digging to try to
> > better understand it.
> > I don't believe there is any I2C involved here, so I'll try the
> > approach you outline above.
>
> Well, it is there in the schematics.

Fair point. Though at least for the USB5734 hub, I don't believe we've
made use of the i2c on it (at least that I can see).  From the
existing driver, the control is basically just 3 GPIOs:  type-c power,
hub power, and the mux/switch.

Trying to get my head around your suggestion:
ctrl {
    mux-controls = <&usb_gpio_mux>;
    connector@0 {
        // type C connector binding
    };
    hub@1 {
        // USB device binding
    };
};

The usb_gpio_mux would be the gpio mux switch.

For the "type C connector binding", that would probably be the rt1711h
type-c chip.

For the "USB device binding" that would be a yet to be implemented
USB5734 hub driver, I'm guessing?

Though I'm not sure I understand how the hub driver gets a signal to
power-on/power-off its gpio-regulator from the mux state?  I'm maybe
still missing some details on the mux infrastructure.

> > > The only new bindings you really need are adding 'mux-controls' to the
> > > USB host controller and the hub binding (we already have a few).

So this is a little confusing to me. Why is the host controller
involved?  It seems to me all of this is down-stream of the
controller, and it's just taking whatever the switch gives it.

I think the switch needs to be signalled from the rt1711h type-c
connector (when it detects the cable and determines the role). That
said, I'm not sure how it would think to control the mux in that case
(as that's pretty special case for this specific hardware). That's why
we're using the role notifier intermediary trick in the current code.

So I guess we could still have the roll notifier intermediary driver
which then controls the mux?

I know that's more a driver implementation detail and not really
hardware description, :) but I'm just trying to figure out how it's
going to come together and actually work.

> > Is the idea to extend the rt1711h and
> > dwc3 drivers further to support the mux/hub bit (this part is fairly
> > foggy to me), completely removing the need for the misc driver?
>
> I imagine that you need some driver to determine the state of the mux.
> Perhaps a usb-mux driver which is instantiated by the host controller
> driver when it sees a mux-controls property. Sorry, haven't looked at
> the driver side of this at all.
>
> > I did take an attempt at something similar with an earlier iteration
> > of the patch set, where I was trying to move the vbus-gpio as a
> > gpio-regulator to be controlled by the rt1711h/tpcm core, but that
> > approach didn't work properly and Hans suggested I just go back to the
> > approach submitted here:
> >   https://lkml.org/lkml/2019/10/22/42
>
> I don't see why that would matter. If you need to sense the Vbus
> state, then you do need a GPIO typically. But for an enable line, it's
> just another level of abstraction.

My concern is that I suspect the issue we had then was that the order
and the timing that we switch the 3 GPIOs ends up being important. In
the current implementation we can adjust them linearly together, where
as when I took a stab at having the vbus gpio was modeled as a
regulator and controlled independently by the rt1711h, the typec state
machine would get confused as I'm guessing the mux/switch wasn't where
it expected it to be when it wanted to change the state of the type-c
vbus.

Thoughts?

Thanks again for the review and feedback. And sorry to let so much
time (and mental context) pass.
-john

Patch

diff --git a/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
new file mode 100644
index 000000000000..1fc3b198ef73
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.yaml
@@ -0,0 +1,85 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019 Linaro Ltd.
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/misc/hisilicon-hikey-usb.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: HiKey960 onboard USB GPIO Hub
+
+maintainers:
+  - John Stultz <[e-mail]>
+
+description: |
+  Supports the onboard HiKey960 USB GPIO hub, which acts as a
+  role-switch intermediary to detect the state of the USB-C
+  port, to switch the hub into dual-role USB-C or host mode,
+  which enables the onboard USB-A host ports.
+
+  Schematics about the hub can be found here:
+    https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf
+
+properties:
+  compatible:
+    items:
+      - const: hisilicon,gpio_hubv1
+
+  typec-vbus-gpios:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the typec-vbus gpio
+
+  otg-switch-gpios:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the otg-switch gpio
+
+  hub-vdd33-en-gpios:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the hub 3.3v power enablement gpio
+
+  usb-role-switch:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: Support role switch.
+
+  port:
+    description: |
+      any connector to the data bus of this controller should be modelled
+      using the OF graph bindings specified, if the "usb-role-switch"
+      property is used. Note for this driver, two ports are supported,
+      the first being the endpoint that will be notified by this driver,
+      and the second being the endpoint that notifies this driver of a
+      role switch.
+
+
+required:
+  - compatible
+  - typec-vbus-gpios
+  - otg-switch-gpios
+  - hub-vdd33-en-gpios
+  - usb-role-switch
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    hisi_hikey_usb: hisi_hikey_usb {
+        compatible = "hisilicon,gpio_hubv1";
+        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>;
+        usb-role-switch;
+
+        port {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            hikey_usb_ep0: endpoint@0 {
+                reg = <0>;
+                remote-endpoint = <&dwc3_role_switch>;
+            };
+            hikey_usb_ep1: endpoint@1 {
+                reg = <1>;
+                remote-endpoint = <&rt1711h_ep>;
+            };
+        };
+    };