LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
To: John Stultz <john.stultz@linaro.org>
Cc: Matthias Kaehlcke <mka@chromium.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
Date: Thu, 2 Sep 2021 23:42:18 +0200	[thread overview]
Message-ID: <20210902234218.5d04d1fa@coco.lan> (raw)
In-Reply-To: <CALAqxLW_wwRFaWASsy6rFHir23MfFU3psq6pjavDeUF5hNS5OA@mail.gmail.com>

Em Thu, 2 Sep 2021 13:31:45 -0700
John Stultz <john.stultz@linaro.org> escreveu:

> 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?
> 
> 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.
> 
> 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.
> 
> Again, apologies if I'm being thick headed. :)

I agree with John here: this driver itself is really simple. It has less 
than 300 lines of code, as it just turns the regulator on/off, has one GPIO 
for the reset pin of the HUB, and two other GPIOs to turn on/off Type-C
power and to switch OTG phy. The actual OTG implementation is done at
the DWC3 driver and the Type-C support is provided by the RT1711H driver.

Splitting its code on two separate drivers, being one just to replace
the 20-30 lines of code that control the regulator sounds overkill
and will require a glue between those drivers that will likely be 
bigger than that. 

So, I guess the best is to keep this as a separate platform-specific
driver.

Thanks,
Mauro

  parent reply	other threads:[~2021-09-02 21:42 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-02 11:28 [PATCH v3 0/4] Make USB ports to work on HiKey960/970 Mauro Carvalho Chehab
2021-09-02 11:28 ` [PATCH v3 1/4] dt-bindings: misc: add schema for USB hub on Kirin devices Mauro Carvalho Chehab
2021-09-02 11:28 ` [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema Mauro Carvalho Chehab
2021-09-02 11:40   ` Greg Kroah-Hartman
2021-09-02 13:10     ` Mauro Carvalho Chehab
2021-09-02 13:38       ` Mauro Carvalho Chehab
2021-09-02 13:56         ` Greg Kroah-Hartman
2021-09-02 14:35           ` Mauro Carvalho Chehab
2021-09-02 17:28             ` Matthias Kaehlcke
2021-09-02 18:29               ` John Stultz
2021-09-02 20:03                 ` Matthias Kaehlcke
2021-09-02 20:31                   ` John Stultz
2021-09-02 21:34                     ` Matthias Kaehlcke
2021-09-02 21:42                     ` Mauro Carvalho Chehab [this message]
2021-09-02 18:45   ` John Stultz
2021-09-02 11:28 ` [PATCH v3 3/4] arm64: dts: hisilicon: Add usb mux hub for hikey970 Mauro Carvalho Chehab
2021-09-02 11:28 ` [PATCH v3 4/4] arm64: dts: hisilicon: Add usb mux hub for hikey960 Mauro Carvalho Chehab

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210902234218.5d04d1fa@coco.lan \
    --to=mchehab+huawei@kernel.org \
    --cc=arnd@arndb.de \
    --cc=dianders@chromium.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=john.stultz@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=mauro.chehab@huawei.com \
    --cc=mka@chromium.org \
    --cc=robh@kernel.org \
    --subject='Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).