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

[John Stultz]

On Wed, Dec 18, 2019 at 8:37 AM Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Thu, Dec 12, 2019 at 01:42:32AM +0000, John Stultz wrote:
> > From: Yu Chen <chenyu56@xxxxxxxxxx>
> >
> > This patch adds binding documentation to support usb hub and usb
> > data role switch of Hisilicon HiKey960 Board.
> >
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > CC: ShuFan Lee <shufan_lee@xxxxxxxxxxx>
> > Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > Cc: Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx>
> > Cc: Yu Chen <chenyu56@xxxxxxxxxx>
> > Cc: Felipe Balbi <balbi@xxxxxxxxxx>
> > Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> > Cc: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > Cc: Jun Li <lijun.kernel@xxxxxxxxx>
> > Cc: Valentin Schneider <valentin.schneider@xxxxxxx>
> > Cc: Guillaume Gardet <Guillaume.Gardet@xxxxxxx>
> > Cc: Jack Pham <jackp@xxxxxxxxxxxxxx>
> > Cc: linux-usb@xxxxxxxxxxxxxxx
> > Cc: devicetree@xxxxxxxxxxxxxxx
> > Signed-off-by: Yu Chen <chenyu56@xxxxxxxxxx>
> > Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> > ---
> > 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 <john.stultz@xxxxxxxxxx>
> > +
> > +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