Hi Krzysztof,

On Mon, Nov 21, 2022 at 6:11 PM Krzysztof Kozlowski
[off-list ref] wrote:

On 21/11/2022 17:36, Geert Uytterhoeven wrote:

quoted

On Mon, Nov 21, 2022 at 5:33 PM Krzysztof Kozlowski
[off-list ref] wrote:

quoted

On 21/11/2022 16:59, Herve Codina wrote:

quoted

On Mon, 21 Nov 2022 12:43:16 +0100
Krzysztof Kozlowski [off-list ref] wrote:

quoted

On 18/11/2022 11:23, Herve Codina wrote:

quoted

On Tue, 15 Nov 2022 15:04:17 +0100
Herve Codina [off-list ref] wrote:

quoted

On Tue, 15 Nov 2022 14:07:52 +0100

quoted

On 15/11/2022 14:05, Krzysztof Kozlowski wrote:

quoted

On 14/11/2022 12:15, Herve Codina wrote:

quoted

Add the h2mode property to force the USBs mode ie:
 - 2 hosts
or
 - 1 host and 1 device

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 .../bindings/clock/renesas,r9a06g032-sysctrl.yaml      | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml
index 95bf485c6cec..f9e0a58aa4fb 100644
--- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml
+++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml

@@ -39,6 +39,16 @@ properties:
   '#power-domain-cells':
     const: 0

+  renesas,h2mode:
+    description: |
+      Configure the USBs mode.
+        - <0> : the USBs are in 1 host and 1 device mode.
+        - <1> : the USBs are in 2 host mode.
+      If the property is not present, the value used is the one already present
+      in the CFG_USB register (from reset or set by the bootloader).
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]

0/1 are quite cryptic. Why not making it a string which is easy to read
and understand? Can be something like "two-hosts" and "one-host". Or
anything you find more readable...

...but actually you should rather make it a property of your USB
controller, not clock controller. You have two controllers and we have a
generic property for them - dr_mode.

Best regards,
Krzysztof

IMHO, this property in the USB controllers does not make sense.
Indeed each controller cannot have a different 'mode'.
Some controllers are USB host only (EHCI and OHCI) and the USBF
controller I worked on is device only.
'h2mode' allows to choose between host or device on one of the USB
but not at the USB controller level.

This property should be handle outside the USB controller nodes.

Currently, this node (declared as a clock node) is in fact a sysctrl
node and can do some configuration not related to clocks.

I agree with you something related to choosing USB Host/Device in
a clock node seems strange.

Some discussion were already opened related to this property and how
to handle it:
  https://lore.kernel.org/all/20221107182642.05a09f2f@bootlin.com/ (local)
  https://lore.kernel.org/all/20221107173614.474707d7@bootlin.com/ (local)

We advanced on this topic.

First, even if 'renesas,r9a06g032-sysctrl.yaml' is present in
the devicetree/bindings/clock/ directory, this node is really
a 'system controller' node:
- title: Renesas RZ/N1D (R9A06G032) System Controller
- compatible: renesas,r9a06g032-sysctrl

It handles clocks, power domains, some DMA routing, ...

Now, the property 'h2mode' allows to choose between:
  - 2 USB hosts
or
  - 1 USB host and 1 USB device.

This switching is system wide and has no reason to be done in
one specific USB controller. It can impact multiple devices and
PLL settings.

The 'renesas,r9a06g032-sysctrl' node, as the system control
node of our system, is the best candidate to handle the property.

Not necessarily. IIUC, you have:

1. sysctrl with some register(s) for choosing device mode
2. usb device or host at one address
3. usb host at separate address

Just to clarify, usb device and host controller are not provided by
the same IP.
We have an USB host at some address range (PCI OHCI/EHCI USB host
below a PCI bridge) and the USB device at some other address range
(below a AHB to someting bridge).
And I am not sure that only USB host or devices are affected by this
property change.

quoted

If so then:
A. Pretty often we have wrapper nodes for this purpose (USB, phy
wrappers or glues) which are usually needed to configure something for a
generic block (like Synopsys etc).

B. Pretty often the device (so your USB host or device) needs to poke
something in system controller registers, e.g. for power or some other
setup.

And we did it for some items (clocks and power).

quoted

Your case looks a lot like (B). We have many, many of such examples
already. Actually it is exactly like that, except that it affects
possibility of another device (e.g. choosing USB device blocks having
host there).

C. It looks a bit like a multi-serial-protocol interfaces (so
UART+I2C+SPI). The difference is that such cases have all these nodes
defined as a children of the protocol-wrapping device. Not here.

I would propose to go with (B) unless of course it's causes some crazy
architecture/code choices. Why? Because with exception of (C) we should
not define properties which represent DT node choices. IOW, Choosing a
node and compatible (e.g. usb controller as device) is enough to
describe the hardware. No need for other properties to control some
register in other block.

The issue with h2mode is that it affects several devices and these
devices should not be in a "running" state when the h2mode is changed.

Why the change should happen when device is running? And why this should
be anyway different than your existing hsmode property - it also will
happen when system and device are running.

quoted

PCI devices (host controllers) itself are not described in the DT. They
are automatically enumerated.

Aren't we talking about USB controller in a MMIO-based SoC?

quoted

Changing the property in USB device controller can leads to hang on
other busses. Indeed, changing this property when a device affected
by the property is running can lead to a bus hang.>
In order to do that from the USB device controller I need to synchronize
the other devices to wait for this setting before running.
1) probe sysctrl without setting h2mode
2) probe some devices (USB host and probably others)
   Stop at some point and wait for the h2mode property setting.

Why do you need to wait? Which device needs to wait? There are no such
devices... if they are then please bring entire DTS, not some pieces in
this patchset.

quoted

3) probe usb device -> Set h2mode property
4) allow devices waiting for the property setting to continue.

I don't get why do you need such order. Your sysctrl also probes any
time so old solution has exactly the same problem, doesn't it?

quoted

This synchronization seems pretty tricky and what to do if nobody
set the property (USB device controller not present or status="disabled"
for instance) ?

Setting this property in sysctrl probe avoid the need for all of this
synchronization:
1) probe sysctrl and set h2mode.
2) probe other devices (no need to wait for the setting as it is already done)

No, because other devices probe before sysctrl. If you bring here any
manual ordering, you are doing it wrong.

quoted

The probing of the other devices (or the starting of they running state)
is guaranteed as they all need some clocks and so cannot start without
having the sysctrl node already probed.
This sysctrl node handles the clocks.

Ah, so sysctrl is a clock controller for these?

Then still there are no other devices depending on your USB. The USB is
the owner of this property (specific bits in register), no one else.

1. There are two USB devices.
2. The USB drivers can be modular, the sysctrl driver cannot, as it is
   the main clock controller.

This does not change anything. Herve wrote:

quoted

probe some devices (USB host and probably others)

Why some can be probed earlier and some not, if there are no
dependencies? If there are dependencies, it's the same case with sysctrl
touching the register bit and the USB controller touching it (as well
via syscon, but that's obvious, I assume).

Where is the synchronization problem?

The h2mode bit (and probably a few other controls we haven't figured out
yet) in the sysctrl must be set before any of the USB devices is active.
Hence it's safest for the sysctrl to do this before any of the USB drivers
probes.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds