Make USB ports to work on HiKey960/970

As there's no upstream DT bindings for this driver, let's update its DT schema, while it is not too late.

Quoted 2 lines

As there's no upstream DT bindings for this driver, let's update its DT schema, while it is not too late.

So this is for 5.15-final?

Em Thu, 2 Sep 2021 13:40:28 +0200
Greg Kroah-Hartman <[email protected]> escreveu:

Quoted 4 lines · Mauro Carvalho Chehab

Quoted 2 lines

As there's no upstream DT bindings for this driver, let's update its DT schema, while it is not too late.

Quoted 1 line

So this is for 5.15-final?

It can either be for 5.15 or 5.16, as there aren't any compatible under arch/ which uses the DT schema there. All patches adding such compatible are on this series. So, whatever version this is applied should be OK.

Em Thu, 2 Sep 2021 15:10:53 +0200
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 7 lines

Em Thu, 2 Sep 2021 13:40:28 +0200
Greg Kroah-Hartman <[email protected]> escreveu:

Quoted 4 lines · Mauro Carvalho Chehab

Quoted 2 lines

As there's no upstream DT bindings for this driver, let's update its DT schema, while it is not too late.

Quoted 6 lines

Quoted 1 line

So this is for 5.15-final?

It can either be for 5.15 or 5.16, as there aren't any compatible under arch/ which uses the DT schema there. All patches adding such compatible are on this series. So, whatever version this is applied should be OK.

On a separate note, despite having "hisi_" on this driver's name, there's nothing there that is really HiSilicon specific. What this driver does is to control an USB HUB integrated inside a DT-based board, doing those functions:

- Power on/off the chip;
- reset the HUB;
- control its OTG switch;
- control power on/off for an USB type-C connector;
- set USB role as host or device.

This is used on both HiKey 960 and HiKey 970 with the following topology:

  +-----+      +--------+       +---------+
  | SoC | ---> | USB PHY|  ---> | USB HUB | ---> USB 3.0
  +-----+      +--------+       +---------+      and type-C ports 

Both Kirin 960 and 970 SoCs have a Synapsys IP (DWC 3).

Both HiKey 960 and 970 cards use Richtek RT1711H Type-C Chip Driver as part of the USB PHY logic, but they use different USB HUBs:

- HiKey 960 use a Microchip USB5734 HUB
- HiKey 970 use a TI TUSB8041 HUB

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 :-)

Quoted 43 lines

Em Thu, 2 Sep 2021 15:10:53 +0200
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 7 lines

Em Thu, 2 Sep 2021 13:40:28 +0200
Greg Kroah-Hartman <[email protected]> escreveu:

Quoted 4 lines · Mauro Carvalho Chehab

Quoted 2 lines

As there's no upstream DT bindings for this driver, let's update its DT schema, while it is not too late.

Quoted 6 lines

Quoted 1 line

So this is for 5.15-final?

It can either be for 5.15 or 5.16, as there aren't any compatible under arch/ which uses the DT schema there. All patches adding such compatible are on this series. So, whatever version this is applied should be OK.

On a separate note, despite having "hisi_" on this driver's name, there's nothing there that is really HiSilicon specific. What this driver does is to control an USB HUB integrated inside a DT-based board, doing those functions:

- Power on/off the chip;
- reset the HUB;
- control its OTG switch;
- control power on/off for an USB type-C connector;
- set USB role as host or device.

This is used on both HiKey 960 and HiKey 970 with the following topology:

  +-----+      +--------+       +---------+
  | SoC | ---> | USB PHY|  ---> | USB HUB | ---> USB 3.0
  +-----+      +--------+       +---------+      and type-C ports 

Both Kirin 960 and 970 SoCs have a Synapsys IP (DWC 3).

Both HiKey 960 and 970 cards use Richtek RT1711H Type-C Chip Driver as part of the USB PHY logic, but they use different USB HUBs:

- HiKey 960 use a Microchip USB5734 HUB
- HiKey 970 use a TI TUSB8041 HUB

Quoted 4 lines

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/[email protected]

for another example of this.

Em Thu, 2 Sep 2021 15:56:36 +0200
Greg Kroah-Hartman <[email protected]> escreveu:

Quoted 49 lines · Mauro Carvalho Chehab

Quoted 47 lines

Em Thu, 2 Sep 2021 15:10:53 +0200
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 7 lines

Em Thu, 2 Sep 2021 13:40:28 +0200
Greg Kroah-Hartman <[email protected]> escreveu:

Quoted 4 lines · Mauro Carvalho Chehab

Quoted 2 lines

As there's no upstream DT bindings for this driver, let's update its DT schema, while it is not too late.

Quoted 6 lines

Quoted 1 line

So this is for 5.15-final?

It can either be for 5.15 or 5.16, as there aren't any compatible under arch/ which uses the DT schema there. All patches adding such compatible are on this series. So, whatever version this is applied should be OK.

On a separate note, despite having "hisi_" on this driver's name, there's nothing there that is really HiSilicon specific. What this driver does is to control an USB HUB integrated inside a DT-based board, doing those functions:

- Power on/off the chip;
- reset the HUB;
- control its OTG switch;
- control power on/off for an USB type-C connector;
- set USB role as host or device.

This is used on both HiKey 960 and HiKey 970 with the following topology:

  +-----+      +--------+       +---------+
  | SoC | ---> | USB PHY|  ---> | USB HUB | ---> USB 3.0
  +-----+      +--------+       +---------+      and type-C ports 

Both Kirin 960 and 970 SoCs have a Synapsys IP (DWC 3).

Both HiKey 960 and 970 cards use Richtek RT1711H Type-C Chip Driver as part of the USB PHY logic, but they use different USB HUBs:

- HiKey 960 use a Microchip USB5734 HUB
- HiKey 970 use a TI TUSB8041 HUB

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 :-)

Quoted 3 lines

Try looking at:

	https://lore.kernel.org/r/[email protected]

for another example of this.

(C/C Matthias here).

Interesting to know that someone else is also needing to add support for USB chips.

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;

(quoted diff) +0 -0

 	- 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.

Quoted 8 lines · Mauro Carvalho Chehab

Em Thu, 2 Sep 2021 15:56:36 +0200
Greg Kroah-Hartman <[email protected]> escreveu:

Quoted 6 lines · Mauro Carvalho Chehab

Quoted 4 lines

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 :-)

Quoted 8 lines

Quoted 3 lines

Try looking at:

    https://lore.kernel.org/r/[email protected]

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 ...

Quoted 12 lines

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.

Quoted 4 lines

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.

Quoted 47 lines · Matthias Kaehlcke

Quoted 8 lines · Mauro Carvalho Chehab

Em Thu, 2 Sep 2021 15:56:36 +0200
Greg Kroah-Hartman <[email protected]> escreveu:

Quoted 6 lines · Mauro Carvalho Chehab

Quoted 4 lines

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 :-)

Quoted 8 lines

Quoted 3 lines

Try looking at:

    https://lore.kernel.org/r/[email protected]

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 ...

Quoted 12 lines

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.

Quoted 4 lines

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;

Quoted 6 lines

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.

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.

Quoted 5 lines · John Stultz

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.

Quoted 4 lines

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.

Quoted 7 lines · Matthias Kaehlcke

Quoted 5 lines · John Stultz

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.

Quoted 7 lines

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.

Quoted 10 lines

Quoted 4 lines

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.

Quoted 7 lines

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. :)

Quoted 7 lines · John Stultz

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 10 lines

Quoted 4 lines

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.

Quoted 7 lines

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. :)

As there's no upstream DT bindings for this driver, let's update its DT schema, while it is not too late.

While here, add error messages, in order to help discovering problems during probing time.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/misc/hisi_hikey_usb.c | 81 +++++++++++++++--------------------
 1 file changed, 35 insertions(+), 46 deletions(-)

drivers/misc/hisi_hikey_usb.c +0 -0

index 989d7d129469..8be7d28cdd71 100644