Message-ID: <a83317dc-093d-4b7f-b22e-1ccb74ed2350@tuxon.dev>
Date: Thu, 22 May 2025 17:38:19 +0300
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: vkoul@...nel.org, kishon@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, p.zabel@...gutronix.de, geert+renesas@...der.be,
 magnus.damm@...il.com, yoshihiro.shimoda.uh@...esas.com, kees@...nel.org,
 gustavoars@...nel.org, biju.das.jz@...renesas.com,
 linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
 linux-hardening@...r.kernel.org, john.madieu.xa@...renesas.com,
 Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: 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:
>> Hi, Krzysztof,
>>
>> On 22.05.2025 10:03, Krzysztof Kozlowski wrote:
>>> On Wed, May 21, 2025 at 05:09:36PM GMT, Claudiu wrote:
>>>>  .../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.
>>>
>>>
>>>> +    $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.

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

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

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

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

> 
>>
>> The PHY driver parse this information and set the signal at proper moments.
>>
>>
>>>
>>>> +          - 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/

Powered by blists - more mailing lists