Re: [PATCH v3 05/12] dt-bindings: phy: renesas,usb2-phy: Add renesas,sysc-signals

On 22.05.2025 15:46, Krzysztof Kozlowski wrote:

On 22/05/2025 12:26, Claudiu Beznea wrote:

quoted

Hi, Krzysztof,

On 22.05.2025 10:03, Krzysztof Kozlowski wrote:

quoted

On Wed, May 21, 2025 at 05:09:36PM GMT, Claudiu wrote:

quoted

 .../bindings/phy/renesas,usb2-phy.yaml        | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml b/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
index 12f8d5d8af55..e1e773cba847 100644
--- a/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml

@@ -86,6 +86,16 @@ properties:
 
   dr_mode: true
 
+  renesas,sysc-signals:
+    description: System controller phandle, specifying the register
+      offset and bitmask associated with a specific system controller signal

This is 100% redundant information. system controller specifying system
controller signal.

Drop.

quoted

+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: system controller phandle

What for? Explain the usage. How is ut used by this hardware.

OK, I though I've explained in the renesas,sysc-signals description
section. I'll adjust it and move it here.

That description did not explain me at all. I mean really, which parts
explains the usage by hardware?

OK, I'll detail it.

quoted

quoted

quoted

+          - description: register offset associated with a signal

What signal? That's a phy.

Would you like me to specify here exactly the signal name? I tried to made
it generic as the system controller provides other signals to other IPs,
the intention was to use the same property for other IPs, if any. And kept
it generic in the idea it could be used in future, if any, for other
signals provided by the system controller to the USB PHY.

Bindings are not generic, so yes, you must explain here what hardware
aspect this is relevant to. What signal? Between whom?

OK

quoted

As explained in the commit description, on the Renesas RZ/G3S SoC, the USB
PHY receives a signal from the system controller that need to be

Interrupt? Some pin changes state? What is a signal? This property is in
the USB PHY device, not system controller.

It's just a generic signal (a line b/w 2 HW blocks, internal to the SoC)
that need to be controlled before/after power to the USB PHY block was
turned on/off.

The above schema is from cover letter a bit simplified. It details the
relation b/w USB blocks (USB CH0 uses PHY0 from USB PHY, USB CH1, uses PHY1
from USB PHY, SYSC controls and provides the PWRRDY signal that is
connected to the USB PHY):

                                   ┌──────────────────────────────┐
                                   │                              │
                                   │     USB CH0                  │
    ┌──────────────────────────┐   │┌───────────────────────────┐ │
    │                 ┌────────┐   ││host controller registers  │ │
    │                 │        │   ││function controller registers│
    │                 │ PHY0   │◄──┤└───────────────────────────┘ │
    │     USB PHY     │        │   └──────────────────────────────┘
    │                 └────────┘
    │                          │
    │┌──────────────┐ ┌────────┐
    ││USBPHY control│ │        │
    ││  registers   │ │ PHY1   │   ┌──────────────────────────────┐
    │└──────────────┘ │        │◄──┤     USB CH1                  │
    │                 └────────┘   │┌───────────────────────────┐ │
    └─▲────────────────────────┘   ││ host controller registers │ │
      │                            │└───────────────────────────┘ │
      │                            └──────────────────────────────┘
      │
      │
      │PWRRDY
      │
      │
      │
      │
    ┌────┐
    │SYSC│
    └────┘

Setting the bits at address specified by the renesas,sysc-signals allows
the SYSC to assert/de-assert the PWRRDY signal. Any settings on USB PHY
need to be done after this signal was de-asserted. It's like a reset signal
(in previous versions it was modeled as such but it wasn't accepted:
https://lore.kernel.org/all/20240822152801.602318-5-claudiu.beznea.uj@bp.renesas.com/ (local)).

I'll detailed in the next version. Do you prefer to have the above diagram
in the schema itself? Or maybe in patch description?

quoted

de-asserted/asserted when power is turned on/off. This signal, called
PWRRDY, is controlled through a specific register in the system controller
memory space.

With this property the intention is to specify to the USB PHY driver the
phandle to the SYSC, register offset within SYSC address space in charge of

This is obvious from the schema and I asked to drop redundant parts.

quoted

controlling the USB PWRRDY signal and the bitmask within this register.

So basically this last piece describes what this hardware needs to do
with system controller? Change some register?

Yes

quoted

The PHY driver parse this information and set the signal at proper moments.

quoted

quoted

+          - description: register bitmask associated with a signal
+
 if:
   properties:
     compatible:

@@ -117,6 +127,18 @@ allOf:
       required:
         - resets
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,usb2-phy-r9a08g045
+    then:
+      required:
+        - renesas,sysc-signals

That's ABI break.

There is no in kernel device tree users of "renesas,usb2-phy-r9a08g045"
compatible. It is introduced in patch 11/12 from this series. With this do
you still consider it ABI break?

Then this patch cannot be split from binding introducing the user. Don't
add unused/undocumented compatibles.

The initial documentation patch was accepted from previous iterations (from
v1 [1]). At that time we didn't know the full picture above.

Thank you,
Claudiu

[1]
https://lore.kernel.org/all/20240822152801.602318-1-claudiu.beznea.uj@bp.renesas.com/ (local)