[PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Rob Herring]

The Rockchip RK3399 DWC3 node has 'power-domain' property which isn't
allowed by the schema:

usb@fe900000: Unevaluated properties are not allowed ('power-domains' was unexpected)

Allow DWC3 nodes to have a single power-domains entry. We could instead
move the power-domains property to the parent wrapper node, but the
could be an ABI break (Linux shouldn't care). Also, we don't want to
encourage the pattern of wrapper nodes just to define resources such as
clocks, resets, power-domains, etc. when not necessary.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 6d78048c4613..bcefd1c2410a 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -91,6 +91,9 @@ properties:
         - usb2-phy
         - usb3-phy
 
+  power-domains:
+    maxItems: 1
+
   resets:
     minItems: 1
 
-- 
2.35.1

[PATCH 2/2] dt-bindings: usb: rockchip,dwc3: Move RK3399 to its own schema

[Rob Herring]

The rockchip,dwc3.yaml schema defines a single DWC3 node, but the RK3399
uses the discouraged parent wrapper node and child 'generic' DWC3 node.
The intent was to modify the RK3399 DTs to use a single node, but the DT
changes were rejected for ABI reasons. However, the schema was accepted
as-is.

To fix this, we need to move the RK3399 binding to its own schema file.
The RK3328 and RK3568 bindings are correct and use a single node.

Cc: Johan Jonker <jbx6244@gmail.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 .../bindings/usb/rockchip,dwc3.yaml           |  10 +-
 .../bindings/usb/rockchip,rk3399-dwc3.yaml    | 115 ++++++++++++++++++
 2 files changed, 119 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/rockchip,rk3399-dwc3.yaml

diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml b/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
index b3798d94d2fd..edb130c780e4 100644
--- a/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
@@ -29,7 +29,6 @@ select:
       contains:
         enum:
           - rockchip,rk3328-dwc3
-          - rockchip,rk3399-dwc3
           - rockchip,rk3568-dwc3
   required:
     - compatible
@@ -39,7 +38,6 @@ properties:
     items:
       - enum:
           - rockchip,rk3328-dwc3
-          - rockchip,rk3399-dwc3
           - rockchip,rk3568-dwc3
       - const: snps,dwc3
 
@@ -90,7 +88,7 @@ required:
 
 examples:
   - |
-    #include <dt-bindings/clock/rk3399-cru.h>
+    #include <dt-bindings/clock/rk3328-cru.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
 
     bus {
@@ -98,11 +96,11 @@ examples:
       #size-cells = <2>;
 
       usbdrd3_0: usb@fe800000 {
-        compatible = "rockchip,rk3399-dwc3", "snps,dwc3";
+        compatible = "rockchip,rk3328-dwc3", "snps,dwc3";
         reg = <0x0 0xfe800000 0x0 0x100000>;
         interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
-        clocks = <&cru SCLK_USB3OTG0_REF>, <&cru SCLK_USB3OTG0_SUSPEND>,
-                 <&cru ACLK_USB3OTG0>, <&cru ACLK_USB3_GRF>;
+        clocks = <&cru SCLK_USB3OTG_REF>, <&cru SCLK_USB3OTG_SUSPEND>,
+                <&cru ACLK_USB3OTG>;
         clock-names = "ref_clk", "suspend_clk",
                       "bus_clk", "grf_clk";
         dr_mode = "otg";
diff --git a/Documentation/devicetree/bindings/usb/rockchip,rk3399-dwc3.yaml b/Documentation/devicetree/bindings/usb/rockchip,rk3399-dwc3.yaml
new file mode 100644
index 000000000000..e39a8a3a7ab3
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/rockchip,rk3399-dwc3.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/rockchip,rk3399-dwc3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip RK3399 SuperSpeed DWC3 USB SoC controller
+
+maintainers:
+  - Heiko Stuebner <heiko@sntech.de>
+
+properties:
+  compatible:
+    const: rockchip,rk3399-dwc3
+
+  '#address-cells':
+    const: 2
+
+  '#size-cells':
+    const: 2
+
+  ranges: true
+
+  clocks:
+    items:
+      - description:
+          Controller reference clock, must to be 24 MHz
+      - description:
+          Controller suspend clock, must to be 24 MHz or 32 KHz
+      - description:
+          Master/Core clock, must to be >= 62.5 MHz for SS
+          operation and >= 30MHz for HS operation
+      - description:
+          USB3 aclk peri
+      - description:
+          USB3 aclk
+      - description:
+          Controller grf clock
+
+  clock-names:
+    items:
+      - const: ref_clk
+      - const: suspend_clk
+      - const: bus_clk
+      - const: aclk_usb3_rksoc_axi_perf
+      - const: aclk_usb3
+      - const: grf_clk
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: usb3-otg
+
+patternProperties:
+  '^usb@':
+    $ref: snps,dwc3.yaml#
+
+additionalProperties: false
+
+required:
+  - compatible
+  - '#address-cells'
+  - '#size-cells'
+  - ranges
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+
+examples:
+  - |
+    #include <dt-bindings/clock/rk3399-cru.h>
+    #include <dt-bindings/power/rk3399-power.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        usb {
+            compatible = "rockchip,rk3399-dwc3";
+            #address-cells = <2>;
+            #size-cells = <2>;
+            ranges;
+            clocks = <&cru SCLK_USB3OTG0_REF>, <&cru SCLK_USB3OTG0_SUSPEND>,
+              <&cru ACLK_USB3OTG0>, <&cru ACLK_USB3_RKSOC_AXI_PERF>,
+              <&cru ACLK_USB3>, <&cru ACLK_USB3_GRF>;
+            clock-names = "ref_clk", "suspend_clk",
+                    "bus_clk", "aclk_usb3_rksoc_axi_perf",
+                    "aclk_usb3", "grf_clk";
+            resets = <&cru SRST_A_USB3_OTG0>;
+            reset-names = "usb3-otg";
+
+            usb@fe800000 {
+                compatible = "snps,dwc3";
+                reg = <0x0 0xfe800000 0x0 0x100000>;
+                interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH 0>;
+                clocks = <&cru SCLK_USB3OTG0_REF>, <&cru ACLK_USB3OTG0>,
+                  <&cru SCLK_USB3OTG0_SUSPEND>;
+                clock-names = "ref", "bus_early", "suspend";
+                dr_mode = "otg";
+                phys = <&u2phy0_otg>, <&tcphy0_usb3>;
+                phy-names = "usb2-phy", "usb3-phy";
+                phy_type = "utmi_wide";
+                snps,dis_enblslpm_quirk;
+                snps,dis-u2-freeclk-exists-quirk;
+                snps,dis_u2_susphy_quirk;
+                snps,dis-del-phy-power-chg-quirk;
+                snps,dis-tx-ipgap-linecheck-quirk;
+                power-domains = <&power RK3399_PD_USB3>;
+            };
+        };
+    };
+...
-- 
2.35.1

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]


Hi,

Rob Herring <robh@kernel.org> writes:

> The Rockchip RK3399 DWC3 node has 'power-domain' property which isn't
> allowed by the schema:
>
> usb@fe900000: Unevaluated properties are not allowed ('power-domains' was unexpected)
>
> Allow DWC3 nodes to have a single power-domains entry. We could instead
> move the power-domains property to the parent wrapper node, but the
> could be an ABI break (Linux shouldn't care). Also, we don't want to
> encourage the pattern of wrapper nodes just to define resources such as
> clocks, resets, power-domains, etc. when not necessary.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index 6d78048c4613..bcefd1c2410a 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -91,6 +91,9 @@ properties:
>          - usb2-phy
>          - usb3-phy
>  
> +  power-domains:
> +    maxItems: 1

AFAICT this can be incorrect. Also, you could have Cc the dwc3
maintainer to get comments.

@Thinh, how many power rails does dwc3 need? I don't have access to a
databook anymore, but I have a vague memory that different parts of dwc3
could, potentially, be powered by completely separate supplies, no? Or
is that only the case for clock domains in dwc3?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 2/2] dt-bindings: usb: rockchip,dwc3: Move RK3399 to its own schema

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

Rob Herring <robh@kernel.org> writes:

> The rockchip,dwc3.yaml schema defines a single DWC3 node, but the RK3399
> uses the discouraged parent wrapper node and child 'generic' DWC3 node.

Why discouraged? Splitting those two separate devices (yes, they are
separate physical modules) has greatly simplified e.g. power management
and encapsulation of the core module.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 2/2] dt-bindings: usb: rockchip,dwc3: Move RK3399 to its own schema

[Rob Herring]

On Tue, Dec 20, 2022 at 1:37 AM Felipe Balbi <balbi@kernel.org> wrote:
>
> Rob Herring <robh@kernel.org> writes:
>
> > The rockchip,dwc3.yaml schema defines a single DWC3 node, but the RK3399
> > uses the discouraged parent wrapper node and child 'generic' DWC3 node.
>
> Why discouraged? Splitting those two separate devices (yes, they are
> separate physical modules) has greatly simplified e.g. power management
> and encapsulation of the core module.

Sometimes they are separate and that's fine, but often it's just
different clocks, resets, etc. and that's no different from every
other block. If there's wrapper registers or something clearly extra,
then I agree a wrapper parent node makes sense. Otherwise, for cases
like RK3399, I don't think it does, but we're stuck with it now.

Also, we have this pattern pretty much nowhere else and DWC3 is not special.

Rob

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Rob Herring]

On Tue, Dec 20, 2022 at 1:34 AM Felipe Balbi <balbi@kernel.org> wrote:
>
>
> Hi,
>
> Rob Herring <robh@kernel.org> writes:
>
> > The Rockchip RK3399 DWC3 node has 'power-domain' property which isn't
> > allowed by the schema:
> >
> > usb@fe900000: Unevaluated properties are not allowed ('power-domains' was unexpected)
> >
> > Allow DWC3 nodes to have a single power-domains entry. We could instead
> > move the power-domains property to the parent wrapper node, but the
> > could be an ABI break (Linux shouldn't care). Also, we don't want to
> > encourage the pattern of wrapper nodes just to define resources such as
> > clocks, resets, power-domains, etc. when not necessary.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > index 6d78048c4613..bcefd1c2410a 100644
> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > @@ -91,6 +91,9 @@ properties:
> >          - usb2-phy
> >          - usb3-phy
> >
> > +  power-domains:
> > +    maxItems: 1
>
> AFAICT this can be incorrect. Also, you could have Cc the dwc3
> maintainer to get comments.

When we have a user with more and know what each one is, then we can
extend it. All the other users (upstream), put 'power-domains' in the
wrapper node. But this is what we need now for RK3399.

I used get_maintainers.pl. If that's the wrong output, fix it please.

Rob

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1840 bytes --]


Hi,

Rob Herring <robh@kernel.org> writes:
>> > The Rockchip RK3399 DWC3 node has 'power-domain' property which isn't
>> > allowed by the schema:
>> >
>> > usb@fe900000: Unevaluated properties are not allowed ('power-domains' was unexpected)
>> >
>> > Allow DWC3 nodes to have a single power-domains entry. We could instead
>> > move the power-domains property to the parent wrapper node, but the
>> > could be an ABI break (Linux shouldn't care). Also, we don't want to
>> > encourage the pattern of wrapper nodes just to define resources such as
>> > clocks, resets, power-domains, etc. when not necessary.
>> >
>> > Signed-off-by: Rob Herring <robh@kernel.org>
>> > ---
>> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> > index 6d78048c4613..bcefd1c2410a 100644
>> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> > @@ -91,6 +91,9 @@ properties:
>> >          - usb2-phy
>> >          - usb3-phy
>> >
>> > +  power-domains:
>> > +    maxItems: 1
>>
>> AFAICT this can be incorrect. Also, you could have Cc the dwc3
>> maintainer to get comments.
>
> When we have a user with more and know what each one is, then we can
> extend it. All the other users (upstream), put 'power-domains' in the

Won't that be an ABI break at that point? You'll change the maximum
number of power-domains.

> wrapper node. But this is what we need now for RK3399.
>
> I used get_maintainers.pl. If that's the wrong output, fix it please.

@Thinh, perhaps you should add dwc3 binding file to the list of
maintained files for you?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 2/2] dt-bindings: usb: rockchip,dwc3: Move RK3399 to its own schema

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

Rob Herring <robh@kernel.org> writes:

> On Tue, Dec 20, 2022 at 1:37 AM Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Rob Herring <robh@kernel.org> writes:
>>
>> > The rockchip,dwc3.yaml schema defines a single DWC3 node, but the RK3399
>> > uses the discouraged parent wrapper node and child 'generic' DWC3 node.
>>
>> Why discouraged? Splitting those two separate devices (yes, they are
>> separate physical modules) has greatly simplified e.g. power management
>> and encapsulation of the core module.
>
> Sometimes they are separate and that's fine, but often it's just
> different clocks, resets, etc. and that's no different from every
> other block.

Right, then the argument is that all other blocks are not modelling the
HW as they should :)

> If there's wrapper registers or something clearly extra, then I agree
> a wrapper parent node makes sense.

There's always wrapper-specific registers. Some wrappers even add custom
functionality. IIRC Qcom added a HW-based URB "scheduler" or some sort.

> Otherwise, for cases like RK3399, I don't think it does, but we're
> stuck with it now.
>
> Also, we have this pattern pretty much nowhere else and DWC3 is not
> special.

No, it's not. But it could just be the first example of the driver
actually modelling the underlying HW.

In any case, I was just curious with your opinion that this model is
discouraged, as it's not stated anywhere in the kernel's documentation.

Happy holidays

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Krzysztof Kozlowski]

On 23/12/2022 11:31, Felipe Balbi wrote:
> 
> Hi,
> 
> Rob Herring <robh@kernel.org> writes:
>>>> The Rockchip RK3399 DWC3 node has 'power-domain' property which isn't
>>>> allowed by the schema:
>>>>
>>>> usb@fe900000: Unevaluated properties are not allowed ('power-domains' was unexpected)
>>>>
>>>> Allow DWC3 nodes to have a single power-domains entry. We could instead
>>>> move the power-domains property to the parent wrapper node, but the
>>>> could be an ABI break (Linux shouldn't care). Also, we don't want to
>>>> encourage the pattern of wrapper nodes just to define resources such as
>>>> clocks, resets, power-domains, etc. when not necessary.
>>>>
>>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>> index 6d78048c4613..bcefd1c2410a 100644
>>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>> @@ -91,6 +91,9 @@ properties:
>>>>          - usb2-phy
>>>>          - usb3-phy
>>>>
>>>> +  power-domains:
>>>> +    maxItems: 1
>>>
>>> AFAICT this can be incorrect. Also, you could have Cc the dwc3
>>> maintainer to get comments.
>>
>> When we have a user with more and know what each one is, then we can
>> extend it. All the other users (upstream), put 'power-domains' in the
> 
> Won't that be an ABI break at that point? You'll change the maximum
> number of power-domains.

Usually extending properties (in flexible way) is not an ABI break. What
would be broken here if it becomes three at some point? Does Linux or
other SW depends now on this being equal to 1?


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Thinh Nguyen]

Hi,

On Fri, Dec 23, 2022, Felipe Balbi wrote:
> 
> Hi,
> 
> Rob Herring <robh@kernel.org> writes:
> >> > The Rockchip RK3399 DWC3 node has 'power-domain' property which isn't
> >> > allowed by the schema:
> >> >
> >> > usb@fe900000: Unevaluated properties are not allowed ('power-domains' was unexpected)
> >> >
> >> > Allow DWC3 nodes to have a single power-domains entry. We could instead
> >> > move the power-domains property to the parent wrapper node, but the
> >> > could be an ABI break (Linux shouldn't care). Also, we don't want to
> >> > encourage the pattern of wrapper nodes just to define resources such as
> >> > clocks, resets, power-domains, etc. when not necessary.
> >> >
> >> > Signed-off-by: Rob Herring <robh@kernel.org>
> >> > ---
> >> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
> >> >  1 file changed, 3 insertions(+)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> > index 6d78048c4613..bcefd1c2410a 100644
> >> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> > @@ -91,6 +91,9 @@ properties:
> >> >          - usb2-phy
> >> >          - usb3-phy
> >> >
> >> > +  power-domains:
> >> > +    maxItems: 1
> >>
> >> AFAICT this can be incorrect. Also, you could have Cc the dwc3
> >> maintainer to get comments.

Felipe is correct. We have 2 power-domains: Core domain and PMU.

> >
> > When we have a user with more and know what each one is, then we can
> > extend it. All the other users (upstream), put 'power-domains' in the
> 
> Won't that be an ABI break at that point? You'll change the maximum
> number of power-domains.
> 
> > wrapper node. But this is what we need now for RK3399.
> >
> > I used get_maintainers.pl. If that's the wrong output, fix it please.
> 
> @Thinh, perhaps you should add dwc3 binding file to the list of
> maintained files for you?
> 

Sure, if makes sense to do so. If there's no objection, I can also
maintain/review it.

I can create a patch after coming back from my break in 2 weeks.
Since I'm on a break at the moment, my response may be delayed.

Thanks,
Thinh

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Rob Herring]

On Fri, Dec 23, 2022 at 5:57 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> Hi,
>
> On Fri, Dec 23, 2022, Felipe Balbi wrote:
> >
> > Hi,
> >
> > Rob Herring <robh@kernel.org> writes:
> > >> > The Rockchip RK3399 DWC3 node has 'power-domain' property which isn't
> > >> > allowed by the schema:
> > >> >
> > >> > usb@fe900000: Unevaluated properties are not allowed ('power-domains' was unexpected)
> > >> >
> > >> > Allow DWC3 nodes to have a single power-domains entry. We could instead
> > >> > move the power-domains property to the parent wrapper node, but the
> > >> > could be an ABI break (Linux shouldn't care). Also, we don't want to
> > >> > encourage the pattern of wrapper nodes just to define resources such as
> > >> > clocks, resets, power-domains, etc. when not necessary.
> > >> >
> > >> > Signed-off-by: Rob Herring <robh@kernel.org>
> > >> > ---
> > >> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
> > >> >  1 file changed, 3 insertions(+)
> > >> >
> > >> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > >> > index 6d78048c4613..bcefd1c2410a 100644
> > >> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > >> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > >> > @@ -91,6 +91,9 @@ properties:
> > >> >          - usb2-phy
> > >> >          - usb3-phy
> > >> >
> > >> > +  power-domains:
> > >> > +    maxItems: 1
> > >>
> > >> AFAICT this can be incorrect. Also, you could have Cc the dwc3
> > >> maintainer to get comments.
>
> Felipe is correct. We have 2 power-domains: Core domain and PMU.

Power management unit? Performance management unit?

That doesn't change that the rk3399 is 1 and we're stuck with it. So I
can say 1 or 2 domains, or we add the 2nd domain when someone needs
it.

Rob

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Felipe Balbi]

Hi,

Rob Herring <robh@kernel.org> writes:
> On Fri, Dec 23, 2022 at 5:57 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>> > Rob Herring <robh@kernel.org> writes:
>> > >> > The Rockchip RK3399 DWC3 node has 'power-domain' property which isn't
>> > >> > allowed by the schema:
>> > >> >
>> > >> > usb@fe900000: Unevaluated properties are not allowed ('power-domains' was unexpected)
>> > >> >
>> > >> > Allow DWC3 nodes to have a single power-domains entry. We could instead
>> > >> > move the power-domains property to the parent wrapper node, but the
>> > >> > could be an ABI break (Linux shouldn't care). Also, we don't want to
>> > >> > encourage the pattern of wrapper nodes just to define resources such as
>> > >> > clocks, resets, power-domains, etc. when not necessary.
>> > >> >
>> > >> > Signed-off-by: Rob Herring <robh@kernel.org>
>> > >> > ---
>> > >> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
>> > >> >  1 file changed, 3 insertions(+)
>> > >> >
>> > >> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> > >> > index 6d78048c4613..bcefd1c2410a 100644
>> > >> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> > >> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> > >> > @@ -91,6 +91,9 @@ properties:
>> > >> >          - usb2-phy
>> > >> >          - usb3-phy
>> > >> >
>> > >> > +  power-domains:
>> > >> > +    maxItems: 1
>> > >>
>> > >> AFAICT this can be incorrect. Also, you could have Cc the dwc3
>> > >> maintainer to get comments.
>>
>> Felipe is correct. We have 2 power-domains: Core domain and PMU.
>
> Power management unit? Performance management unit?
>
> That doesn't change that the rk3399 is 1 and we're stuck with it. So I
> can say 1 or 2 domains, or we add the 2nd domain when someone needs
> it.

Isn't the snps,dwc3.yaml document supposed to document dwc3's view of
the world? In that case, dwc3 expects 2 power domains. It just so
happens that in rk3399 they are fed from the same power supply, but
dwc3' still thinks there are two of them. No?

It's a similar situation when you have multiple clock domains with the
same parent clock.

-- 
balbi

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Rob Herring]

On Fri, Dec 30, 2022 at 2:43 AM Felipe Balbi <balbi@kernel.org> wrote:
>
>
> Hi,
>
> Rob Herring <robh@kernel.org> writes:
> > On Fri, Dec 23, 2022 at 5:57 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >> > Rob Herring <robh@kernel.org> writes:
> >> > >> > The Rockchip RK3399 DWC3 node has 'power-domain' property which isn't
> >> > >> > allowed by the schema:
> >> > >> >
> >> > >> > usb@fe900000: Unevaluated properties are not allowed ('power-domains' was unexpected)
> >> > >> >
> >> > >> > Allow DWC3 nodes to have a single power-domains entry. We could instead
> >> > >> > move the power-domains property to the parent wrapper node, but the
> >> > >> > could be an ABI break (Linux shouldn't care). Also, we don't want to
> >> > >> > encourage the pattern of wrapper nodes just to define resources such as
> >> > >> > clocks, resets, power-domains, etc. when not necessary.
> >> > >> >
> >> > >> > Signed-off-by: Rob Herring <robh@kernel.org>
> >> > >> > ---
> >> > >> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
> >> > >> >  1 file changed, 3 insertions(+)
> >> > >> >
> >> > >> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> > >> > index 6d78048c4613..bcefd1c2410a 100644
> >> > >> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> > >> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> > >> > @@ -91,6 +91,9 @@ properties:
> >> > >> >          - usb2-phy
> >> > >> >          - usb3-phy
> >> > >> >
> >> > >> > +  power-domains:
> >> > >> > +    maxItems: 1
> >> > >>
> >> > >> AFAICT this can be incorrect. Also, you could have Cc the dwc3
> >> > >> maintainer to get comments.
> >>
> >> Felipe is correct. We have 2 power-domains: Core domain and PMU.
> >
> > Power management unit? Performance management unit?
> >
> > That doesn't change that the rk3399 is 1 and we're stuck with it. So I
> > can say 1 or 2 domains, or we add the 2nd domain when someone needs
> > it.
>
> Isn't the snps,dwc3.yaml document supposed to document dwc3's view of
> the world? In that case, dwc3 expects 2 power domains. It just so
> happens that in rk3399 they are fed from the same power supply, but
> dwc3' still thinks there are two of them. No?

Yes. That is how bindings *should* be. However, RK3399 defined one PD
long ago and it's an ABI. So we are stuck with it. Everyone else put
power-domains in the parent because obviously the DWC3 has 0
power-domains.

> It's a similar situation when you have multiple clock domains with the
> same parent clock.

Yes, that's a common problem in clock bindings too. Not really
anything we can do about that other than require a detailed reference
manual with every binding and someone (me) reviewing the manual
against the binding. Neither of those are going to happen. Even on Arm
Primecell blocks which clearly (and publicly) document the clocks,
we've gotten these wrong (or .dts authors just didn't follow the
binding).

Rob

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Felipe Balbi]

Hi,

Rob Herring <robh@kernel.org> writes:
>> >> > >> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
>> >> > >> >  1 file changed, 3 insertions(+)
>> >> > >> >
>> >> > >> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> >> > >> > index 6d78048c4613..bcefd1c2410a 100644
>> >> > >> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> >> > >> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> >> > >> > @@ -91,6 +91,9 @@ properties:
>> >> > >> >          - usb2-phy
>> >> > >> >          - usb3-phy
>> >> > >> >
>> >> > >> > +  power-domains:
>> >> > >> > +    maxItems: 1
>> >> > >>
>> >> > >> AFAICT this can be incorrect. Also, you could have Cc the dwc3
>> >> > >> maintainer to get comments.
>> >>
>> >> Felipe is correct. We have 2 power-domains: Core domain and PMU.
>> >
>> > Power management unit? Performance management unit?
>> >
>> > That doesn't change that the rk3399 is 1 and we're stuck with it. So I
>> > can say 1 or 2 domains, or we add the 2nd domain when someone needs
>> > it.
>>
>> Isn't the snps,dwc3.yaml document supposed to document dwc3's view of
>> the world? In that case, dwc3 expects 2 power domains. It just so
>> happens that in rk3399 they are fed from the same power supply, but
>> dwc3' still thinks there are two of them. No?
>
> Yes. That is how bindings *should* be. However, RK3399 defined one PD
> long ago and it's an ABI. So we are stuck with it. Everyone else put

Are you confusing things, perhaps? DWC3, the block Synopsys licenses,
has, as Thinh confirmed, 2 internal power domains. How OEMs (TI, Intel,
Rockchip, Allwinner, etc) decide to integrate the IP into their systems
is something different. That is part of the (so-called)
wrapper. Different integrators will wrap Synopsys IP however they see
fit, as long as they can provide a suitable translation layer between
Synopsys own view of the world (its own interconnect implementation, of
which there are 3 to choose from, IIRC) and the rest of the SoC.

Perhaps what RK3399 did was provide a single power domain at the wrapper
level that feeds both of DWC3's own power domains, but DWC3 itself still
has 2 power domains, that's not something rockchip can change without
risking the loss of support from Synopsys, as it would not be Synopsys
IP anymore.

> power-domains in the parent because obviously the DWC3 has 0
> power-domains.

How did you come to this conclusion?

>> It's a similar situation when you have multiple clock domains with the
>> same parent clock.
>
> Yes, that's a common problem in clock bindings too. Not really
> anything we can do about that other than require a detailed reference
> manual with every binding and someone (me) reviewing the manual
> against the binding. Neither of those are going to happen. Even on Arm
> Primecell blocks which clearly (and publicly) document the clocks,
> we've gotten these wrong (or .dts authors just didn't follow the
> binding).

Heh

-- 
balbi

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Rob Herring]

On Fri, Dec 30, 2022 at 11:09 AM Felipe Balbi <balbi@kernel.org> wrote:
>
>
> Hi,
>
> Rob Herring <robh@kernel.org> writes:
> >> >> > >> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
> >> >> > >> >  1 file changed, 3 insertions(+)
> >> >> > >> >
> >> >> > >> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> >> > >> > index 6d78048c4613..bcefd1c2410a 100644
> >> >> > >> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> >> > >> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> >> > >> > @@ -91,6 +91,9 @@ properties:
> >> >> > >> >          - usb2-phy
> >> >> > >> >          - usb3-phy
> >> >> > >> >
> >> >> > >> > +  power-domains:
> >> >> > >> > +    maxItems: 1
> >> >> > >>
> >> >> > >> AFAICT this can be incorrect. Also, you could have Cc the dwc3
> >> >> > >> maintainer to get comments.
> >> >>
> >> >> Felipe is correct. We have 2 power-domains: Core domain and PMU.
> >> >
> >> > Power management unit? Performance management unit?
> >> >
> >> > That doesn't change that the rk3399 is 1 and we're stuck with it. So I
> >> > can say 1 or 2 domains, or we add the 2nd domain when someone needs
> >> > it.
> >>
> >> Isn't the snps,dwc3.yaml document supposed to document dwc3's view of
> >> the world? In that case, dwc3 expects 2 power domains. It just so
> >> happens that in rk3399 they are fed from the same power supply, but
> >> dwc3' still thinks there are two of them. No?
> >
> > Yes. That is how bindings *should* be. However, RK3399 defined one PD
> > long ago and it's an ABI. So we are stuck with it. Everyone else put
>
> Are you confusing things, perhaps? DWC3, the block Synopsys licenses,
> has, as Thinh confirmed, 2 internal power domains. How OEMs (TI, Intel,
> Rockchip, Allwinner, etc) decide to integrate the IP into their systems
> is something different. That is part of the (so-called)
> wrapper. Different integrators will wrap Synopsys IP however they see
> fit, as long as they can provide a suitable translation layer between
> Synopsys own view of the world (its own interconnect implementation, of
> which there are 3 to choose from, IIRC) and the rest of the SoC.
>
> Perhaps what RK3399 did was provide a single power domain at the wrapper
> level that feeds both of DWC3's own power domains, but DWC3 itself still
> has 2 power domains, that's not something rockchip can change without
> risking the loss of support from Synopsys, as it would not be Synopsys
> IP anymore.

Again, none of this matters. I'm documenting what is already in use
and an ABI, not what is correct. The time for correctness was when
this binding was added.

To move forward, how about something like this:

power-domains:
  description: Really there are 2 PDs, but some implementations
defined a single PD.
  minItems: 1
  items:
    - description: core
    - description: PMU

We unfortunately can't constrain this to Rockchip in the schema
because that specific information is in the parent node.

(kind of crappy descriptions too, but that's the amount of information I have.)

> > power-domains in the parent because obviously the DWC3 has 0
> > power-domains.
>
> How did you come to this conclusion?

By testing the schema against the in tree .dts files. To date, no one
other than Rockchip has power-domains in the DWC3 node.

Rob

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Thinh Nguyen]

On Tue, Jan 03, 2023, Rob Herring wrote:
> On Fri, Dec 30, 2022 at 11:09 AM Felipe Balbi <balbi@kernel.org> wrote:
> >
> >
> > Hi,
> >
> > Rob Herring <robh@kernel.org> writes:
> > >> >> > >> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
> > >> >> > >> >  1 file changed, 3 insertions(+)
> > >> >> > >> >
> > >> >> > >> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > >> >> > >> > index 6d78048c4613..bcefd1c2410a 100644
> > >> >> > >> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > >> >> > >> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > >> >> > >> > @@ -91,6 +91,9 @@ properties:
> > >> >> > >> >          - usb2-phy
> > >> >> > >> >          - usb3-phy
> > >> >> > >> >
> > >> >> > >> > +  power-domains:
> > >> >> > >> > +    maxItems: 1
> > >> >> > >>
> > >> >> > >> AFAICT this can be incorrect. Also, you could have Cc the dwc3
> > >> >> > >> maintainer to get comments.
> > >> >>
> > >> >> Felipe is correct. We have 2 power-domains: Core domain and PMU.
> > >> >
> > >> > Power management unit? Performance management unit?
> > >> >
> > >> > That doesn't change that the rk3399 is 1 and we're stuck with it. So I
> > >> > can say 1 or 2 domains, or we add the 2nd domain when someone needs
> > >> > it.
> > >>
> > >> Isn't the snps,dwc3.yaml document supposed to document dwc3's view of
> > >> the world? In that case, dwc3 expects 2 power domains. It just so
> > >> happens that in rk3399 they are fed from the same power supply, but
> > >> dwc3' still thinks there are two of them. No?
> > >
> > > Yes. That is how bindings *should* be. However, RK3399 defined one PD
> > > long ago and it's an ABI. So we are stuck with it. Everyone else put
> >
> > Are you confusing things, perhaps? DWC3, the block Synopsys licenses,
> > has, as Thinh confirmed, 2 internal power domains. How OEMs (TI, Intel,
> > Rockchip, Allwinner, etc) decide to integrate the IP into their systems
> > is something different. That is part of the (so-called)
> > wrapper. Different integrators will wrap Synopsys IP however they see
> > fit, as long as they can provide a suitable translation layer between
> > Synopsys own view of the world (its own interconnect implementation, of
> > which there are 3 to choose from, IIRC) and the rest of the SoC.
> >
> > Perhaps what RK3399 did was provide a single power domain at the wrapper
> > level that feeds both of DWC3's own power domains, but DWC3 itself still

Just for some additional context/use case, the power to the PMU (power
management unit) must always be on. If the device supports hibernation,
in hibernation, the power supply to the core can be turned off.

> > has 2 power domains, that's not something rockchip can change without
> > risking the loss of support from Synopsys, as it would not be Synopsys
> > IP anymore.
> 
> Again, none of this matters. I'm documenting what is already in use
> and an ABI, not what is correct. The time for correctness was when
> this binding was added.

That's unfortunate. That makes this very difficult to maintain if we
can't rectify a mistake.

> 
> To move forward, how about something like this:
> 
> power-domains:
>   description: Really there are 2 PDs, but some implementations
> defined a single PD.
>   minItems: 1
>   items:
>     - description: core
>     - description: PMU
> 
> We unfortunately can't constrain this to Rockchip in the schema
> because that specific information is in the parent node.
> 
> (kind of crappy descriptions too, but that's the amount of information I have.)

Can we omit mentioning min or maxItems? While it may not be desired,
it's not a hard requirement right? This can help avoid some confusion
with devicetree documentation and dwc3 databook.

Thanks,
Thinh

> 
> > > power-domains in the parent because obviously the DWC3 has 0
> > > power-domains.
> >
> > How did you come to this conclusion?
> 
> By testing the schema against the in tree .dts files. To date, no one
> other than Rockchip has power-domains in the DWC3 node.
> 

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Rob Herring]

On Mon, Jan 9, 2023 at 1:40 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Tue, Jan 03, 2023, Rob Herring wrote:
> > On Fri, Dec 30, 2022 at 11:09 AM Felipe Balbi <balbi@kernel.org> wrote:
> > >
> > >
> > > Hi,
> > >
> > > Rob Herring <robh@kernel.org> writes:
> > > >> >> > >> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
> > > >> >> > >> >  1 file changed, 3 insertions(+)
> > > >> >> > >> >
> > > >> >> > >> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > >> >> > >> > index 6d78048c4613..bcefd1c2410a 100644
> > > >> >> > >> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > >> >> > >> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > >> >> > >> > @@ -91,6 +91,9 @@ properties:
> > > >> >> > >> >          - usb2-phy
> > > >> >> > >> >          - usb3-phy
> > > >> >> > >> >
> > > >> >> > >> > +  power-domains:
> > > >> >> > >> > +    maxItems: 1
> > > >> >> > >>
> > > >> >> > >> AFAICT this can be incorrect. Also, you could have Cc the dwc3
> > > >> >> > >> maintainer to get comments.
> > > >> >>
> > > >> >> Felipe is correct. We have 2 power-domains: Core domain and PMU.
> > > >> >
> > > >> > Power management unit? Performance management unit?
> > > >> >
> > > >> > That doesn't change that the rk3399 is 1 and we're stuck with it. So I
> > > >> > can say 1 or 2 domains, or we add the 2nd domain when someone needs
> > > >> > it.
> > > >>
> > > >> Isn't the snps,dwc3.yaml document supposed to document dwc3's view of
> > > >> the world? In that case, dwc3 expects 2 power domains. It just so
> > > >> happens that in rk3399 they are fed from the same power supply, but
> > > >> dwc3' still thinks there are two of them. No?
> > > >
> > > > Yes. That is how bindings *should* be. However, RK3399 defined one PD
> > > > long ago and it's an ABI. So we are stuck with it. Everyone else put
> > >
> > > Are you confusing things, perhaps? DWC3, the block Synopsys licenses,
> > > has, as Thinh confirmed, 2 internal power domains. How OEMs (TI, Intel,
> > > Rockchip, Allwinner, etc) decide to integrate the IP into their systems
> > > is something different. That is part of the (so-called)
> > > wrapper. Different integrators will wrap Synopsys IP however they see
> > > fit, as long as they can provide a suitable translation layer between
> > > Synopsys own view of the world (its own interconnect implementation, of
> > > which there are 3 to choose from, IIRC) and the rest of the SoC.
> > >
> > > Perhaps what RK3399 did was provide a single power domain at the wrapper
> > > level that feeds both of DWC3's own power domains, but DWC3 itself still
>
> Just for some additional context/use case, the power to the PMU (power
> management unit) must always be on. If the device supports hibernation,
> in hibernation, the power supply to the core can be turned off.

Things in an always-on PD may or may not be described in
'power-domains', so from a DT perspective I would say 1 domain is
perfectly valid here.

I suppose the PMU could be in a PD which can be gated off, but any
hibernation features would be lost.

> > > has 2 power domains, that's not something rockchip can change without
> > > risking the loss of support from Synopsys, as it would not be Synopsys
> > > IP anymore.
> >
> > Again, none of this matters. I'm documenting what is already in use
> > and an ABI, not what is correct. The time for correctness was when
> > this binding was added.
>
> That's unfortunate. That makes this very difficult to maintain if we
> can't rectify a mistake.

Shrug. What's unfortunate is only a limited number of people can
review bindings to be correct in this aspect. And I'm not one of them.

We deal with this all the time already. It's just amplified when it is
shared IP. Would I like less variation? Yes, but it's not a
showstopper.

> > To move forward, how about something like this:
> >
> > power-domains:
> >   description: Really there are 2 PDs, but some implementations
> > defined a single PD.
> >   minItems: 1
> >   items:
> >     - description: core
> >     - description: PMU
> >
> > We unfortunately can't constrain this to Rockchip in the schema
> > because that specific information is in the parent node.
> >
> > (kind of crappy descriptions too, but that's the amount of information I have.)
>
> Can we omit mentioning min or maxItems? While it may not be desired,
> it's not a hard requirement right? This can help avoid some confusion
> with devicetree documentation and dwc3 databook.

Why? Don't you want to catch someone defining 3 domains?

Rob

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property

[Thinh Nguyen]

On Mon, Jan 09, 2023, Rob Herring wrote:
> On Mon, Jan 9, 2023 at 1:40 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> > On Tue, Jan 03, 2023, Rob Herring wrote:
> > > On Fri, Dec 30, 2022 at 11:09 AM Felipe Balbi <balbi@kernel.org> wrote:
> > > >
> > > >
> > > > Hi,
> > > >
> > > > Rob Herring <robh@kernel.org> writes:
> > > > >> >> > >> >  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 3 +++
> > > > >> >> > >> >  1 file changed, 3 insertions(+)
> > > > >> >> > >> >
> > > > >> >> > >> > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > > >> >> > >> > index 6d78048c4613..bcefd1c2410a 100644
> > > > >> >> > >> > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > > >> >> > >> > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> > > > >> >> > >> > @@ -91,6 +91,9 @@ properties:
> > > > >> >> > >> >          - usb2-phy
> > > > >> >> > >> >          - usb3-phy
> > > > >> >> > >> >
> > > > >> >> > >> > +  power-domains:
> > > > >> >> > >> > +    maxItems: 1
> > > > >> >> > >>
> > > > >> >> > >> AFAICT this can be incorrect. Also, you could have Cc the dwc3
> > > > >> >> > >> maintainer to get comments.
> > > > >> >>
> > > > >> >> Felipe is correct. We have 2 power-domains: Core domain and PMU.
> > > > >> >
> > > > >> > Power management unit? Performance management unit?
> > > > >> >
> > > > >> > That doesn't change that the rk3399 is 1 and we're stuck with it. So I
> > > > >> > can say 1 or 2 domains, or we add the 2nd domain when someone needs
> > > > >> > it.
> > > > >>
> > > > >> Isn't the snps,dwc3.yaml document supposed to document dwc3's view of
> > > > >> the world? In that case, dwc3 expects 2 power domains. It just so
> > > > >> happens that in rk3399 they are fed from the same power supply, but
> > > > >> dwc3' still thinks there are two of them. No?
> > > > >
> > > > > Yes. That is how bindings *should* be. However, RK3399 defined one PD
> > > > > long ago and it's an ABI. So we are stuck with it. Everyone else put
> > > >
> > > > Are you confusing things, perhaps? DWC3, the block Synopsys licenses,
> > > > has, as Thinh confirmed, 2 internal power domains. How OEMs (TI, Intel,
> > > > Rockchip, Allwinner, etc) decide to integrate the IP into their systems
> > > > is something different. That is part of the (so-called)
> > > > wrapper. Different integrators will wrap Synopsys IP however they see
> > > > fit, as long as they can provide a suitable translation layer between
> > > > Synopsys own view of the world (its own interconnect implementation, of
> > > > which there are 3 to choose from, IIRC) and the rest of the SoC.
> > > >
> > > > Perhaps what RK3399 did was provide a single power domain at the wrapper
> > > > level that feeds both of DWC3's own power domains, but DWC3 itself still
> >
> > Just for some additional context/use case, the power to the PMU (power
> > management unit) must always be on. If the device supports hibernation,
> > in hibernation, the power supply to the core can be turned off.
> 
> Things in an always-on PD may or may not be described in

I'm just providing additional info, and not everything is necessarily
needed for the DT description.

> 'power-domains', so from a DT perspective I would say 1 domain is
> perfectly valid here.
> 
> I suppose the PMU could be in a PD which can be gated off, but any
> hibernation features would be lost.
> 

Some devices have both the core and the PMU in the same power domain,
which may be the case for RK3399. However, the PMUs may be implemented
in a separate power rail than the core.

> > > > has 2 power domains, that's not something rockchip can change without
> > > > risking the loss of support from Synopsys, as it would not be Synopsys
> > > > IP anymore.
> > >
> > > Again, none of this matters. I'm documenting what is already in use
> > > and an ABI, not what is correct. The time for correctness was when
> > > this binding was added.
> >
> > That's unfortunate. That makes this very difficult to maintain if we
> > can't rectify a mistake.
> 
> Shrug. What's unfortunate is only a limited number of people can
> review bindings to be correct in this aspect. And I'm not one of them.
> 
> We deal with this all the time already. It's just amplified when it is
> shared IP. Would I like less variation? Yes, but it's not a
> showstopper.
> 
> > > To move forward, how about something like this:
> > >
> > > power-domains:
> > >   description: Really there are 2 PDs, but some implementations
> > > defined a single PD.
> > >   minItems: 1
> > >   items:
> > >     - description: core
> > >     - description: PMU
> > >
> > > We unfortunately can't constrain this to Rockchip in the schema
> > > because that specific information is in the parent node.
> > >
> > > (kind of crappy descriptions too, but that's the amount of information I have.)
> >
> > Can we omit mentioning min or maxItems? While it may not be desired,
> > it's not a hard requirement right? This can help avoid some confusion
> > with devicetree documentation and dwc3 databook.
> 
> Why? Don't you want to catch someone defining 3 domains?
> 

My concern was more about "maxItems: 1" may cause some confusion. If we
can't say "maxItems: 2", omitting it or using "minItems: 1" seems to be
a better option.

As you mentioned, we can't do much about it now that it's part of the
DT. I've provided the info you need to make the appropriate change.
Looks like there's no perfect solution. Please make the change you see
best fit.

Thanks,
Thinh