Message-ID: <CALAqxLVgyyDmoKAxrKp-GvkBZ4hfbMgEBThPeVmJ7EZ6GzLjFw@mail.gmail.com>
Date:   Wed, 22 Jan 2020 10:28:03 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     Rob Herring <robh@...nel.org>
Cc:     lkml <linux-kernel@...r.kernel.org>, Yu Chen <chenyu56@...wei.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mark Rutland <mark.rutland@....com>,
        ShuFan Lee <shufan_lee@...htek.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Chunfeng Yun <chunfeng.yun@...iatek.com>,
        Felipe Balbi <balbi@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Jun Li <lijun.kernel@...il.com>,
        Valentin Schneider <valentin.schneider@....com>,
        Guillaume Gardet <Guillaume.Gardet@....com>,
        Jack Pham <jackp@...eaurora.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH v7 7/8] dt-bindings: misc: Add bindings for HiSilicon usb
 hub and data role switch functionality on HiKey960

(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 <robh@...nel.org> wrote:
> On Wed, Dec 18, 2019 at 11:21 AM John Stultz <john.stultz@...aro.org> wrote:
> > On Wed, Dec 18, 2019 at 8:37 AM Rob Herring <robh@...nel.org> 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

Powered by blists - more mailing lists