On Tue, Oct 19, 2021 at 06:04:06PM +0200, Fabrice Gasnier wrote:

On 10/15/21 8:39 AM, Greg Kroah-Hartman wrote:

quoted

On Thu, Oct 14, 2021 at 02:38:55PM -0700, Doug Anderson wrote:

quoted

Hi,

On Tue, Sep 21, 2021 at 10:09 AM Matthias Kaehlcke [off-list ref] wrote:

quoted

Hi Greg,

are there any actions pending or can this land in usb-testing?

I confirmed that this series can be rebased on top of v5.15-rc2
without conflicts.

I'm quite interested to know what the next action items are, too. This
is one of the very few patches we have for trogdor (excluding MIPI
camera, which is a long story) that we're carrying downstream, so I'm
keenly interested in making sure it's unblocked (if, indeed, it's
blocked on anything).

If folks feel that this needs more review eyes before landing again
then I'll try to find some time in the next week or two. If it's just
waiting for the merge window to open/close so it can have maximal bake
time, that's cool too. Please yell if there's something that I can do
to help, though! :-)

I would love more review-eyes on this please.

Hi,

I noticed this series some time ago, and wanted to take a closer look.

The same issue this series address is seen on stm32 board for instance.
(arch/arm/boot/dts/stm32mp15xx-dkx.dtsi). On board HUB (not described in
the DT) is supplied by an always-on regulator.
So it could could be interesting/useful to address the same case ,
on stm32 boards, where USB2 (ehci-platform driver) is used currently.

I noticed a few things, especially on the dt-bindings. I've some
questions here.

In this series, RTS5411 is used. The dt-bindings documents it as a child
node of the USB controller. E.g.

&usb {
	usb_hub_2_0: hub@1 {
		...
	};

	usb_hub_3_0: hub@2 {
	};
}

I had a quick look at RTS5411 datasheet. It looks like there's an i2c
interface too.
- I guess the I2C interface isn't used in your case ?
  (I haven't checked what it could be used for...)

Correct, the i2c interface isn't used on my board.

Also the binding isn't completely new, it is based on the generic USB
binding (https://www.kernel.org/doc/Documentation/devicetree/bindings/usb/usb-device.txt)

In the stm32 boards (stm32mp15xx-dkx), there's an usb2514b chip
- that also could be wired on I2C interface (0R mount option)
- unused on stm32 boards by default

usb2514b chip already has a dt-bindings (with compatible), and a driver:
- drivers/usb/misc/usb251xb.c
- Documentation/devicetree/bindings/usb/usb251xb.txt

It is defined more as an i2c chip, so I'd expect it as an i2c child,
e.g. like:

&i2c {
	usb2514b@2c {
		compatible = "microchip,usb2514b";
		...
	};
};


This way, I don't see how it could be used together with onboard_usb_hub
driver ? (But I may have missed it)

Indeed, you can either use the i2c driver for the hub or the onboard_usb_hub
driver, but not both at the same time. The i2c driver requires the hub to be
powered before communicating with it over i2c, hence the power sequence
should not be delegated to the onboard_usb_hub driver.

Is it possible to use a phandle, instead of a child node ?

The child node is part of the generic USB binding. The onboard_usb_hub
driver needs it to find the USB device(s) that correspond to the hub,
to optionally power the hub off during system suspend when no wakeup
capable devices are connected.

However, in the stm32mp15xx-dkx case, i2c interface isn't wired/used by
default. So obviously the i2c driver isn't used. In this case, could the
"microchip,usb2514b" be listed in onboard_usb_hub driver ?
(wouldn't it be redundant ?)

You would use the compatible string of the generic USB binding, i.e.
"usbVID,PID", which would have to be added to the onboard_usb_hub driver.

In this case it would be a child node of the usb DT node... Maybe that's
more a question for Rob: would it be "legal" regarding existing
dt-bindings ?

The USB node is always there implicitly (the USB device exists), the only
difference is that the node is added explicitly (plus additional
properties).

There was a somewhat related long-winded discussion with Rob on an earlier
version of the driver/binding:
https://lore.kernel.org/linux-usb/1613055380.685661.519681.nullmailer@robh.at.kernel.org/ (local)