Re: [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module

[Conor Dooley <conor@kernel.org>]

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

On Wed, Jun 28, 2023 at 02:44:10PM +0800, Xingyu Wu wrote:
> On 2023/6/14 2:31, Krzysztof Kozlowski wrote:
> > On 13/06/2023 14:58, Xingyu Wu wrote:
> >> From: William Qiu <william.qiu@starfivetech.com>
> >> 
> >> Add documentation to describe StarFive System Controller Registers.
> >> 
> >> Co-developed-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> >> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> >> ---
> >>  .../soc/starfive/starfive,jh7110-syscon.yaml  | 62 +++++++++++++++++++
> >>  MAINTAINERS                                   |  7 +++
> >>  2 files changed, 69 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> >> 
> >> diff --git a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> >> new file mode 100644
> >> index 000000000000..a81190f8a54d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> >> @@ -0,0 +1,62 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/soc/starfive/starfive,jh7110-syscon.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: StarFive JH7110 SoC system controller
> >> +
> >> +maintainers:
> >> +  - William Qiu <william.qiu@starfivetech.com>
> >> +
> >> +description: |
> >> +  The StarFive JH7110 SoC system controller provides register information such
> >> +  as offset, mask and shift to configure related modules such as MMC and PCIe.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    oneOf:
> >> +      - items:
> >> +          - const: starfive,jh7110-sys-syscon
> >> +          - const: syscon
> >> +          - const: simple-mfd
> >> +      - items:
> >> +          - enum:
> >> +              - starfive,jh7110-aon-syscon
> >> +              - starfive,jh7110-stg-syscon
> >> +          - const: syscon
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  clock-controller:
> >> +    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
> >> +    type: object
> >> +
> >> +  "#power-domain-cells":
> >> +    const: 1
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +
> >> +allOf:
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            const: starfive,jh7110-aon-syscon
> >> +    then:
> >> +      required:
> >> +        - "#power-domain-cells"
> > 
> > Where did you implement the results of the discussion that only some
> > devices can have power and clock controller?
> > 
> > According to your code all of above - sys, aon and stg - have clock and
> > power controllers. If not, then the code is not correct, so please do
> > not respond with what is where (like you did last time) but actually
> > implement what you say.
> > 
> 
> Hi Krzysztof, I need to modify the code to implement it.
> If I drop the 'clock-controller' and '"#power-domain-cells"' in properites, and change to this:
> 
> --- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
> @@ -29,28 +29,33 @@ properties:
>    reg:
>      maxItems: 1
>  
> -  clock-controller:
> -    $ref: /schemas/clock/starfive,jh7110-pll.yaml#
> -    type: object
> -
> -  "#power-domain-cells":
> -    const: 1
> -
>  required:
>    - compatible
>    - reg
>  
>  allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: starfive,jh7110-sys-syscon
> +    then:
> +      properties:
> +        clock-controller:
> +          $ref: /schemas/clock/starfive,jh7110-pll.yaml#
> +          type: object

Why do this?
Why not define the property has you have been doing, but only allow it
on the syscons that support it?
See the section starting at L205 of example-schema.yaml.

> +
>    - if:
>        properties:
>          compatible:
>            contains:
>              const: starfive,jh7110-aon-syscon
>      then:
> -      required:
> -        - "#power-domain-cells"
> +      properties:
> +        "#power-domain-cells":
> +          const: 1
>  

> -additionalProperties: false
> +additionalProperties: true

Why do you need this?
Allowing "additionalProperties: true" sounds like you've got some prblem
that you are trying to hide...

> Would it be better to show that sys-syscon only has clock-controller and aon-syscon is power controller?

You should only permit the properties where they are valid, yes.

Cheers,
Conor.


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