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