Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema

[Matthias Kaehlcke]

On Thu, Sep 02, 2021 at 11:29:49AM -0700, John Stultz wrote:
> On Thu, Sep 2, 2021 at 10:28 AM Matthias Kaehlcke <mka@xxxxxxxxxxxx> wrote:
> > On Thu, Sep 02, 2021 at 04:35:29PM +0200, Mauro Carvalho Chehab wrote:
> > > Em Thu, 2 Sep 2021 15:56:36 +0200
> > > Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> escreveu:
> > > > On Thu, Sep 02, 2021 at 03:38:20PM +0200, Mauro Carvalho Chehab wrote:
> > > > > While I'm not sure how generic this driver can be, I'm thinking that
> > > > > maybe a future patch could rename it to 'generic-usb-hub' or
> > > > > something similar - finding a good name is always the hardest
> > > > > part :-)
> > > >
> > > > Try looking at:
> > > >     https://lore.kernel.org/r/20210813195228.2003500-1-mka@xxxxxxxxxxxx
> > > > for another example of this.
> > >
> > > (C/C Matthias here).
> > >
> > > Interesting to know that someone else is also needing to add support
> > > for USB chips.
> >
> > Yeah, there were several attempts over the years, but so far none of
> > them landed upstream ...
> >
> > > Yet, the approach took there won't work with HiKey 960/970, as
> > > it needs to control not only the regulator, but it should also
> > > work as as usb-role-switch.
> > >
> > > So, besides controlling the regulator, which seems to be basically what
> > > the onboard_usb_hub_driver does, but it should also be able to:
> > >
> > >       - (optionally) reset the HUB;
> > >       - control its OTG switch;
> > >       - control power on/off for an USB type-C connector;
> > >       - set USB role as host or device.
> >
> > > Perhaps it would be possible to merge both approaches by modifying
> > > hisi_hikey_usb in order to add the extra bits required by the boards that
> > > Matthias is currently working, and requiring the GPIOs for OTG and
> > > type-C connectors only if DT contains usb-role-switch.
> >
> > I'm not convinced that a hub driver should be in charge of role switching.
> > I wonder if the hub part could remain separate, and the role switching be
> > done by a dedicated driver that interacts with the hub driver through
> > some interface. From the above list the hub driver could still be in charge
> > of:
> >
> > - (optionally) reset the HUB;
> > - control power on/off for an USB type-C connector;
> >
> > Maybe the hub driver could implement a reset controller to allow the role
> > switching driver to switch it on and off (including type-C power).
> >
> > The role switch driver (a leaner version of hisi_hikey_usb) could model
> > the mux and switch the hub on and off, without being concerned about all
> > the details.
> 
> Apologies, I may be misunderstanding you, but I think the missing
> issue there is: "what triggers the hub driver to switch it on or off?"
> 
> For the hikey960/970 boards, removing/plugging in the usb-c cable is
> what we use to enable/disable the mux/hub.
> 
> The current iteration (of which there have been many) of hikey hub
> driver uses the role switch notification as its trigger. It's not
> really controlling the role switching, just using that signal. That's
> why the driver works as an intermediary/relay of the roleswitch
> notification.

Apologies too, my terminology wasn't very clear, I had little exposure to
OTG so far.

The hisi_hikey_usb driver doesn't control the role of the USB controller,
however it deals with platform specific role switching stuff, like muxing
the USB PHY to the hub (host mode) or directly to the type-C port (device
mode), or controlling the power of the type-C port.

> We had earlier efforts that had hacks to specific drivers as well as
> attempts to add notifiers on role switches (but those were terribly
> racy), so the intermediary/relay approach has been a great improvement
> on reliability with little impact to other drivers.

I can see how raciness can be a problem. I'm not proprosing to use
notifiers in the driver that deals with the hub, from the hub's
perspective it is connected to a host port and it shouldn't have to care
about OTG.

But the 'hub driver' could expose a synchronous interface that allows the
hisi_hikey_usb driver to power the hub on and off (or keep it in reset).
That would maintain the relay approach, but without having a driver that
tries to do too many things at once. For example the onboard_usb_hub driver
has the option to power the hub down during suspend if no wakeup capable
devices are connected downstream, I'm not convinced that this and the
handling of the mux should be done by the same driver.