Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 2 Sep 2021 14:34:25 -0700
From:   Matthias Kaehlcke <mka@chromium.org>
To:     John Stultz <john.stultz@linaro.org>
Cc:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh@kernel.org>, linuxarm@huawei.com,
        mauro.chehab@huawei.com, Arnd Bergmann <arnd@arndb.de>,
        lkml <linux-kernel@vger.kernel.org>,
        Douglas Anderson <dianders@chromium.org>
Subject: Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
Message-ID: <YTFDYWsTSYlbbIje@google.com>
References: <d990e75f28c443c0c5a5fc857b87acc4be3f9464.1630581434.git.mchehab+huawei@kernel.org>
 <YTC4LPDem9uKXyMd@kroah.com>
 <20210902151053.7ddfbe3a@coco.lan>
 <20210902153820.5624b57f@coco.lan>
 <YTDYFCH/DbK1SFVv@kroah.com>
 <20210902163529.734b8e0e@coco.lan>
 <YTEJuK50e5PBBJfO@google.com>
 <CALAqxLUxgRCUweDcE1WWV1gTsstZAvjUT6ZuTij5Mro8MHKi8Q@mail.gmail.com>
 <YTEuA+Z3f50RVGgX@google.com>
 <CALAqxLW_wwRFaWASsy6rFHir23MfFU3psq6pjavDeUF5hNS5OA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CALAqxLW_wwRFaWASsy6rFHir23MfFU3psq6pjavDeUF5hNS5OA@mail.gmail.com>
Precedence: bulk

On Thu, Sep 02, 2021 at 01:31:45PM -0700, John Stultz wrote:
> On Thu, Sep 2, 2021 at 1:03 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > On Thu, Sep 02, 2021 at 11:29:49AM -0700, John Stultz wrote:
> > > 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.
> 
> True, though the exact configuration of powering type-c port, the mux
> and the hub power that we want to set depends on the role.
> 
> > > 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.
> 
> I'm not sure I'm totally following your proposal, so apologies if I
> have it wrong.
> It seems you're suggesting to move just the hub power logic out of the
> hisi hub driver, and utilize the onboard_usb_hub driver for that?

Yes

> I guess that could be done, but I also feel like it's really just the
> regulator control, which is a pretty small part of the hisi hub driver
> logic.
> Additionally, if you look at the logic that handles the different
> setting configurations needed for the different roles:
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/hisi_hikey_usb.c#n97
> 
> You'll see the ordering of powering things on/off and switching the
> mux is a bit subtle.

Indeed, that would make things a bit more involved.

> So while I guess we could call some onboard_usb_hub hook for the hub
> power on/off calls (hub_power_ctrl() in the hisi driver), I'm not sure
> that really is saving much logic wise, and splits the details across
> an additional driver in an already somewhat complicated config.

+ probing, tearing down and potentially reset.

It might not be worth as long as only a single hub model (or compatible
ones) is supported, things might look different if other power on/off/reset
sequences need to be added.

> Again, apologies if I'm being thick headed. :)

no worries, same here :)

I have no objections to leaving the driver structure as is, especially
since it already has been in upstream for a while, I was mainly looking
for possible synergies.