Re: [PATCH] dt-bindings: power: Add SiFive Domain Management controllers

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4921 bytes --]

On Fri, May 09, 2025 at 08:40:28AM +0200, Krzysztof Kozlowski wrote:
> On Fri, May 09, 2025 at 10:16:04AM GMT, Nick Hu wrote:
> > SiFive Domain Management controller includes the following components
> > - SiFive Tile Management Controller
> > - SiFive Cluster Management Controller
> > - SiFive Core Complex Management Controller
> > 
> > These controllers control the clock and power domain of the
> > corresponding domain.
> > 
> > Signed-off-by: Nick Hu <nick.hu@sifive.com>
> > Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
> > ---
> >  .../devicetree/bindings/power/sifive,tmc.yaml | 89 +++++++++++++++++++
> 
> Where is a patch with the driver (user of the binding)?
> 
> >  1 file changed, 89 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/sifive,tmc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/power/sifive,tmc.yaml b/Documentation/devicetree/bindings/power/sifive,tmc.yaml
> > new file mode 100644
> > index 000000000000..7ed4f290b94b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/sifive,tmc.yaml
> > @@ -0,0 +1,89 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/power/sifive,tmc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SiFive Domain Management Controller
> > +
> > +maintainers:
> > +  - Cyan Yang <cyan.yang@sifive.com>
> > +  - Nick Hu <nick.hu@sifive.com>
> > +  - Samuel Holland <samuel.holland@sifive.com>
> > +
> > +description: |
> > +  This is the device tree binding for the following SiFive Domain Management Controllers.
> 
> Explain the hardware, not that "binding is a binding for ...".
> 
> Also, wrap according to Linux coding style.
> 
> 
> > +  - Tile Management Controller
> > +      - TMC0
> > +      - TMC1
> > +      - TMC2
> > +      - TMC3
> > +  - Subsystem Management Controller
> > +      - SMC0
> > +      - SMC1
> > +      - SMC2
> > +      - SMC3
> > +  - Cluster Management Controller
> > +      - CMC2
> > +      - CMC3
> > +  SiFive Domain Management Controllers support the SiFive Quiet Interface
> > +  Protocol (SQIP) starting from the Version 1. The control method is
> > +  different from the Version 0, making them incompatible.
> > +
> > +allOf:
> > +  - $ref: power-domain.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - {}
> > +          - pattern: "^sifive,[ts]mc0$"
> > +      - items:
> > +          - {}
> > +          - pattern: "^sifive,[ts]mc3$"
> > +          - pattern: "^sifive,[ts]mc2$"
> > +          - pattern: "^sifive,[ts]mc1$"
> > +      - items:
> > +          - {}
> > +          - pattern: "^sifive,[ts]mc2$"
> > +          - pattern: "^sifive,[ts]mc1$"
> > +      - items:
> > +          - {}
> > +          - pattern: "^sifive,[ts]mc1$"
> > +      - items:
> > +          - {}
> > +          - const: sifive,cmc3
> > +          - const: sifive,cmc2
> > +      - items:
> > +          - {}
> > +          - const: sifive,cmc2
> 
> All of this is just unexpected. Why any compatible should come with
> these?

It's also not quite correct either, right? Or may not be correct at
least. It permits "xxx", "tmc2", "smc1" and "xxx", "smc2", "tmc1"
which mean that the smc and tmc must be identical in terms of
programming model.

> You need to use SoC specific compatibles.

I think there's some slack to provide here, sifive are upstreaming it in
advance of there being customers (or customers ready to upstream) and this
format allows us to accept bindings/drivers and the customer will have
to add a soc-specific compatible in order to actually use these in a
dts. I think it's better to accept something along these lines than
stall out until a customer decides to upstream their user. That said, I
would expect this to come (as you mentioned above) with the driver.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  sifive,feature-level:
> > +    description: |
> > +      Supported power features. This property is absent if the full set of features
> > +      is supported
> 
> Compatible defines this. Drop.
> 
> 
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: ["nopg", "ceasepg", "runonlypg"]
> > +
> > +  "#power-domain-cells":
> > +    const: 0
> > +
> > +if:
> > +  not:
> > +    properties:
> > +      compatible:
> > +        contains:
> > +          pattern: "^sifive,[tsc]mc3$"
> > +then:
> > +  properties:
> > +    sifive,feature-level: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> 
> Missing example.

You can't actually make an example that passes validation when the
soc-specific compatibles are not added, so this would require adding
some.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv