Re: [PATCH v5 2/7] dt-bindings: soc: starfive: Add StarFive syscon module
From: Xingyu Wu
Date: Thu Jun 29 2023 - 02:46:45 EST
On 2023/6/29 1:34, Conor Dooley wrote:
> 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@xxxxxxxxxxxxxxxx>
>> >>
>> >> Add documentation to describe StarFive System Controller Registers.
>> >>
>> >> Co-developed-by: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
>> >> Signed-off-by: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
>> >> Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
>> >> ---
>> >> .../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@xxxxxxxxxxxxxxxx>
>> >> +
>> >> +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.
>
Yeah, following your advice, I modified the codes and there are two options:
--- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
+++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
@@ -41,6 +41,16 @@ required:
- reg
allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: starfive,jh7110-sys-syscon
+ then:
+ required:
+ - clock-controller
+ properties:
+ "#power-domain-cells": false
- if:
properties:
compatible:
contains:
const: starfive,jh7110-aon-syscon
then:
required:
- "#power-domain-cells"
+ properties:
+ clock-controller: false
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: starfive,jh7110-stg-syscon
+ then:
+ properties:
+ clock-controller: false
+ "#power-domain-cells": false
additionalProperties: false
Or :
--- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
+++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml
@@ -41,6 +41,17 @@ required:
- reg
allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: starfive,jh7110-sys-syscon
+ then:
+ required:
+ - clock-controller
+ else:
+ properties:
+ clock-controller: false
- if:
properties:
compatible:
contains:
const: starfive,jh7110-aon-syscon
then:
required:
- "#power-domain-cells"
+ else:
+ properties:
+ "#power-domain-cells": false
additionalProperties: false
Which one is better? Thanks.
Best regards,
Xingyu Wu