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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250509-subtract-caramel-08d47ed3281c@spud>
Date: Fri, 9 May 2025 16:57:20 +0100
From: Conor Dooley <conor@...nel.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Nick Hu <nick.hu@...ive.com>, Cyan Yang <cyan.yang@...ive.com>,
	Samuel Holland <samuel.holland@...ive.com>,
	devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org,
	linux-kernel@...r.kernel.org, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Paul Walmsley <paul.walmsley@...ive.com>
Subject: Re: [PATCH] dt-bindings: power: Add SiFive Domain Management
 controllers

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@...ive.com>
> > Reviewed-by: Samuel Holland <samuel.holland@...ive.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@...ive.com>
> > +  - Nick Hu <nick.hu@...ive.com>
> > +  - Samuel Holland <samuel.holland@...ive.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.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)