From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
On the Renesas RZ/G3S SoC, the USB PHY receives a signal from the system
controller that need to be 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.
Add the renesas,sysc-signals DT property to describe the relation b/w the
system controller and the USB PHY on the Renesas RZ/G3S. This property
provides a phandle to the system controller, along with the offset within
the system controller memory space that manages the signal and a bitmask
that indicates the specific bits required to control the signal.
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v3:
- replace renesas,sysc-signal with renesas,sysc-signals for case where
more than 1 signal should be described with this property
- Geert: due to this I dropped you tag
Changes in v2:
- none; this patch is new
.../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
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+ items:
+ - items:
+ - description: system controller phandle
+ - description: register offset associated with a signal
+ - 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
+ else:
+ properties:
+ renesas,sysc-signals: false
+
additionalProperties: false
examples:
--
2.43.0On 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. > + - description: register offset associated with a signal What signal? That's a phy. > + - 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. Best regards, Krzysztof
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. > >> + - 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. 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 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 controlling the USB PWRRDY signal and the bitmask within this register. 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? Thank you for your review, Claudiu
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? > >> >>> + - 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? > > 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. > 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? > > 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. Best regards, Krzysztof
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/
On 22/05/2025 14:46, Krzysztof Kozlowski wrote: >>>> >>>> + - 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. > Or you meant DTS? I asked about ABI which is not about in-kernel users. You can always change in-kernel users, so what would be any point of a binding and its ABI? Best regards, Krzysztof
On 22.05.2025 15:48, Krzysztof Kozlowski wrote: > On 22/05/2025 14:46, Krzysztof Kozlowski wrote: >>>>> >>>>> + - 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. >> > Or you meant DTS? Yes, I meant in tree DTS. > I asked about ABI which is not about in-kernel users. > You can always change in-kernel users, so what would be any point of a > binding and its ABI? > > Best regards, > Krzysztof
On 22/05/2025 16:39, Claudiu Beznea wrote: > > > On 22.05.2025 15:48, Krzysztof Kozlowski wrote: >> On 22/05/2025 14:46, Krzysztof Kozlowski wrote: >>>>>> >>>>>> + - 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. >>> >> Or you meant DTS? > > Yes, I meant in tree DTS. Rhetorical question (because we talked about this on mailing list many times enough): what would be the point of any ABI if you can break it and adjust in-tree DTS in the next patch? Best regards, Krzysztof
© 2016 - 2025 Red Hat, Inc.