| Message ID | 20240419125812.983409-5-jan.dakinevich@salutedevices.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add A1 Soc audio clock controller driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On 19/04/2024 14:58, Jan Dakinevich wrote: > Add device tree bindings for A1 SoC audio clock and reset controllers. > > Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> This is still RFC, so not ready. Limited, incomplete review follows. Full review will be provided when the work is ready. Drop "driver" references, e.g. from subject. Bindings are about hardware. .... > + > + clocks: > + maxItems: 26 > + items: > + - description: input main peripheral bus clock > + - description: input dds_in > + - description: input fixed pll div2 > + - description: input fixed pll div3 > + - description: input hifi_pll > + - description: input oscillator (usually at 24MHz) > + additionalItems: > + oneOf: > + - description: slv_sclk[0-9] - slave bit clocks provided by external components > + - description: slv_lrclk[0-9]- slave sample clocks provided by external components What does it mean the clocks are optional? Pins could be not routed? It's really rare case that clocks within the SoC are optional, so every such case is questionable. > + > + clock-names: > + maxItems: 26 > + items: > + - const: pclk > + - const: dds_in > + - const: fclk_div2 > + - const: fclk_div3 > + - const: hifi_pll > + - const: xtal > + additionalItems: > + oneOf: > + - pattern: "^slv_sclk[0-9]$" > + - pattern: "^slv_lrclk[0-9]$" > + > +required: > + - compatible > + - '#clock-cells' > + - reg > + - clocks > + - clock-names > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: amlogic,a1-audio-clkc > + then: > + required: > + - '#reset-cells' > + else: > + properties: > + '#reset-cells': false > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/amlogic,a1-pll-clkc.h> > + #include <dt-bindings/clock/amlogic,a1-peripherals-clkc.h> > + #include <dt-bindings/clock/amlogic,a1-audio-clkc.h> > + audio { > + #address-cells = <2>; > + #size-cells = <2>; > + > + clkc_audio: clock-controller@fe050000 { > + compatible = "amlogic,a1-audio-clkc"; > + reg = <0x0 0xfe050000 0x0 0xb0>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + clocks = <&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>, > + <&clkc_periphs CLKID_DDS_IN>, > + <&clkc_pll CLKID_FCLK_DIV2>, > + <&clkc_pll CLKID_FCLK_DIV3>, > + <&clkc_pll CLKID_HIFI_PLL>, > + <&xtal>; > + clock-names = "pclk", > + "dds_in", > + "fclk_div2", > + "fclk_div3", > + "hifi_pll", > + "xtal"; Make it complete - list all clocks. > + }; > + > + clkc_audio_vad: clock-controller@fe054800 { Just keep one example. It's basically almost the same. Best regards, Krzysztof
On Fri, 19 Apr 2024 15:58:10 +0300, Jan Dakinevich wrote: > Add device tree bindings for A1 SoC audio clock and reset controllers. > > Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> > --- > > This controller has 6 mandatory and up to 20 optional clocks. To describe > this, I use 'additionalItems'. It produces correct processed-schema.json: > > "clock-names": { > "maxItems": 26, > "items": [ > { > "const": "pclk" > }, > { > "const": "dds_in" > }, > { > "const": "fclk_div2" > }, > { > "const": "fclk_div3" > }, > { > "const": "hifi_pll" > }, > { > "const": "xtal" > } > ], > "additionalItems": { > "oneOf": [ > { > "pattern": "^slv_sclk[0-9]$" > }, > { > "pattern": "^slv_lrclk[0-9]$" > } > ] > }, > "type": "array", > "minItems": 6 > }, > > and it behaves as expected. However, the checking is followed by > complaints like this: > > Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' > > And indeed, 'additionalItems' has boolean type in meta-schema. So, how to > do it right? > --- > .../bindings/clock/amlogic,a1-audio-clkc.yaml | 124 ++++++++++++++++++ > .../dt-bindings/clock/amlogic,a1-audio-clkc.h | 122 +++++++++++++++++ > .../reset/amlogic,meson-a1-audio-reset.h | 29 ++++ > 3 files changed, 275 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > create mode 100644 include/dt-bindings/clock/amlogic,a1-audio-clkc.h > create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clocks:additionalItems: {'oneOf': [{'description': 'slv_sclk[0-9] - slave bit clocks provided by external components'}, {'description': 'slv_lrclk[0-9]- slave sample clocks provided by external components'}]} is not of type 'boolean' from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' from schema $id: http://devicetree.org/meta-schemas/string-array.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clocks: 'oneOf' conditional failed, one must be fixed: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clocks: 'anyOf' conditional failed, one must be fixed: 'items' is not one of ['maxItems', 'description', 'deprecated'] hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values. 'additionalItems' is not one of ['maxItems', 'description', 'deprecated'] hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values. 'maxItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf'] 'items' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf'] 'additionalItems' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf'] {'oneOf': [{'description': 'slv_sclk[0-9] - slave bit clocks provided by external components'}, {'description': 'slv_lrclk[0-9]- slave sample clocks provided by external components'}]} is not of type 'boolean' hint: Arrays must be described with a combination of minItems/maxItems/items True was expected hint: Arrays must be described with a combination of minItems/maxItems/items 1 was expected hint: Only "maxItems" is required for a single entry if there are no constraints defined for the values. hint: cell array properties must define how many entries and what the entries are when there is more than one entry. from schema $id: http://devicetree.org/meta-schemas/clocks.yaml# 'maxItems' is not one of ['type', 'description', 'dependencies', 'dependentRequired', 'dependentSchemas', 'properties', 'patternProperties', 'additionalProperties', 'unevaluatedProperties', 'deprecated', 'required', 'not', 'allOf', 'anyOf', 'oneOf', '$ref'] 'items' is not one of ['type', 'description', 'dependencies', 'dependentRequired', 'dependentSchemas', 'properties', 'patternProperties', 'additionalProperties', 'unevaluatedProperties', 'deprecated', 'required', 'not', 'allOf', 'anyOf', 'oneOf', '$ref'] 'additionalItems' is not one of ['type', 'description', 'dependencies', 'dependentRequired', 'dependentSchemas', 'properties', 'patternProperties', 'additionalProperties', 'unevaluatedProperties', 'deprecated', 'required', 'not', 'allOf', 'anyOf', 'oneOf', '$ref'] 'type' is a required property hint: DT nodes ("object" type in schemas) can only use a subset of json-schema keywords from schema $id: http://devicetree.org/meta-schemas/clocks.yaml# Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.example.dtb: /example-0/audio/clock-controller@fe054800: failed to match any schema with compatible: ['amlogic,a1-audio2-clkc'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240419125812.983409-5-jan.dakinevich@salutedevices.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote: > Add device tree bindings for A1 SoC audio clock and reset controllers. > > Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> > --- > > This controller has 6 mandatory and up to 20 optional clocks. To describe > this, I use 'additionalItems'. It produces correct processed-schema.json: > > "clock-names": { > "maxItems": 26, > "items": [ > { > "const": "pclk" > }, > { > "const": "dds_in" > }, > { > "const": "fclk_div2" > }, > { > "const": "fclk_div3" > }, > { > "const": "hifi_pll" > }, > { > "const": "xtal" > } > ], > "additionalItems": { > "oneOf": [ > { > "pattern": "^slv_sclk[0-9]$" > }, > { > "pattern": "^slv_lrclk[0-9]$" > } > ] > }, > "type": "array", > "minItems": 6 > }, > > and it behaves as expected. However, the checking is followed by > complaints like this: > > Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' > > And indeed, 'additionalItems' has boolean type in meta-schema. So, how to > do it right? The meta-schemas are written both to prevent nonsense that json-schema allows by default (e.g additionalitems (wrong case)) and constraints to follow the patterns we expect. I'm happy to loosen the latter case if there's really a need. Generally, most bindings shouldn't be using 'additionalItems' at all as all entries should be defined, but there's a few exceptions. Here, the only reasoning I see is 26 entries is a lot to write out, but that wouldn't really justify it. As Krzysztof pointed out, you either have the clocks in the h/w or you don't, so saying they are variable is suspect. Rob
On 4/19/24 17:06, Krzysztof Kozlowski wrote: > On 19/04/2024 14:58, Jan Dakinevich wrote: >> Add device tree bindings for A1 SoC audio clock and reset controllers. >> >> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> > > This is still RFC, so not ready. > > Limited, incomplete review follows. Full review will be provided when > the work is ready. > > Drop "driver" references, e.g. from subject. Bindings are about hardware. > > > .... > >> + >> + clocks: >> + maxItems: 26 >> + items: >> + - description: input main peripheral bus clock >> + - description: input dds_in >> + - description: input fixed pll div2 >> + - description: input fixed pll div3 >> + - description: input hifi_pll >> + - description: input oscillator (usually at 24MHz) >> + additionalItems: >> + oneOf: >> + - description: slv_sclk[0-9] - slave bit clocks provided by external components >> + - description: slv_lrclk[0-9]- slave sample clocks provided by external components > > What does it mean the clocks are optional? Pins could be not routed? Yes exactly. Pins could be routed in any combination or could be not routed at all. It is determined by schematics and that how external codecs are configured. > It's really rare case that clocks within the SoC are optional, so every > such case is questionable. > > >> + >> + clock-names: >> + maxItems: 26 >> + items: >> + - const: pclk >> + - const: dds_in >> + - const: fclk_div2 >> + - const: fclk_div3 >> + - const: hifi_pll >> + - const: xtal >> + additionalItems: >> + oneOf: >> + - pattern: "^slv_sclk[0-9]$" >> + - pattern: "^slv_lrclk[0-9]$" >> + >> +required: >> + - compatible >> + - '#clock-cells' >> + - reg >> + - clocks >> + - clock-names >> + >> +allOf: >> + - if: >> + properties: >> + compatible: >> + contains: >> + const: amlogic,a1-audio-clkc >> + then: >> + required: >> + - '#reset-cells' >> + else: >> + properties: >> + '#reset-cells': false >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/amlogic,a1-pll-clkc.h> >> + #include <dt-bindings/clock/amlogic,a1-peripherals-clkc.h> >> + #include <dt-bindings/clock/amlogic,a1-audio-clkc.h> >> + audio { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + clkc_audio: clock-controller@fe050000 { >> + compatible = "amlogic,a1-audio-clkc"; >> + reg = <0x0 0xfe050000 0x0 0xb0>; >> + #clock-cells = <1>; >> + #reset-cells = <1>; >> + clocks = <&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>, >> + <&clkc_periphs CLKID_DDS_IN>, >> + <&clkc_pll CLKID_FCLK_DIV2>, >> + <&clkc_pll CLKID_FCLK_DIV3>, >> + <&clkc_pll CLKID_HIFI_PLL>, >> + <&xtal>; >> + clock-names = "pclk", >> + "dds_in", >> + "fclk_div2", >> + "fclk_div3", >> + "hifi_pll", >> + "xtal"; > > Make it complete - list all clocks. > You mean, all optional clocks should be mentioned here. Right? >> + }; >> + >> + clkc_audio_vad: clock-controller@fe054800 { > > Just keep one example. It's basically almost the same. > The worth of this duplication is to show how a clock from second controller (<&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>) is used by first one. May be it would be better to keep it... What do you think? > > > Best regards, > Krzysztof >
On 4/20/24 00:09, Rob Herring wrote: > On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote: >> Add device tree bindings for A1 SoC audio clock and reset controllers. >> >> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> --- >> >> This controller has 6 mandatory and up to 20 optional clocks. To describe >> this, I use 'additionalItems'. It produces correct processed-schema.json: >> >> "clock-names": { >> "maxItems": 26, >> "items": [ >> { >> "const": "pclk" >> }, >> { >> "const": "dds_in" >> }, >> { >> "const": "fclk_div2" >> }, >> { >> "const": "fclk_div3" >> }, >> { >> "const": "hifi_pll" >> }, >> { >> "const": "xtal" >> } >> ], >> "additionalItems": { >> "oneOf": [ >> { >> "pattern": "^slv_sclk[0-9]$" >> }, >> { >> "pattern": "^slv_lrclk[0-9]$" >> } >> ] >> }, >> "type": "array", >> "minItems": 6 >> }, >> >> and it behaves as expected. However, the checking is followed by >> complaints like this: >> >> Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' >> >> And indeed, 'additionalItems' has boolean type in meta-schema. So, how to >> do it right? > > The meta-schemas are written both to prevent nonsense that json-schema > allows by default (e.g additionalitems (wrong case)) and constraints to > follow the patterns we expect. I'm happy to loosen the latter case if > there's really a need. > > Generally, most bindings shouldn't be using 'additionalItems' at all as > all entries should be defined, but there's a few exceptions. Here, the > only reasoning I see is 26 entries is a lot to write out, but that > wouldn't really justify it. Writing a lot of entries don't scary me too much, but the reason is that the existence of optional clock sources depends on schematics. Also, we unable to declare dt-nodes for 'clocks' array in any generic way, because their declaration would depends on that what is actually connected to the SoC (dt-node could be "fixed-clock" with specific rate or something else). By the way, I don't know any example (neither for A1 SoC nor for other Amlogic's SoCs) where these optional clocks are used, but they are allowed by hw. This is my understanding of this controller. I hope, Jerome Brunet will clarify how it actually works. > As Krzysztof pointed out, you either have > the clocks in the h/w or you don't, so saying they are variable is > suspect. > > Rob
On 20/04/2024 16:48, Jan Dakinevich wrote: >>> + clock-names = "pclk", >>> + "dds_in", >>> + "fclk_div2", >>> + "fclk_div3", >>> + "hifi_pll", >>> + "xtal"; >> >> Make it complete - list all clocks. >> > > You mean, all optional clocks should be mentioned here. Right? Yes. > >>> + }; >>> + >>> + clkc_audio_vad: clock-controller@fe054800 { >> >> Just keep one example. It's basically almost the same. >> > > The worth of this duplication is to show how a clock from second > controller (<&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>) is used by first > one. May be it would be better to keep it... What do you think? I don't understand what is worth here. Using clocks is kind of obvious? What's special? Best regards, Krzysztof
On 4/21/24 17:02, Krzysztof Kozlowski wrote: > On 20/04/2024 16:48, Jan Dakinevich wrote: >>>> + clock-names = "pclk", >>>> + "dds_in", >>>> + "fclk_div2", >>>> + "fclk_div3", >>>> + "hifi_pll", >>>> + "xtal"; >>> >>> Make it complete - list all clocks. >>> >> >> You mean, all optional clocks should be mentioned here. Right? > > Yes. > >> >>>> + }; >>>> + >>>> + clkc_audio_vad: clock-controller@fe054800 { >>> >>> Just keep one example. It's basically almost the same. >>> >> >> The worth of this duplication is to show how a clock from second >> controller (<&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>) is used by first >> one. May be it would be better to keep it... What do you think? > > I don't understand what is worth here. Using clocks is kind of obvious? > What's special? > The special is that the clock "pclk" for "clkc_audio" must be <&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>. This thing is not obvious. I can keep only "clkc_audio" node here, but reference to "clkc_audio_vad" will be undefined in example. Is it okay? > Best regards, > Krzysztof >
On 20/04/2024 18:15, Jan Dakinevich wrote: > > > On 4/20/24 00:09, Rob Herring wrote: >> On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote: >>> Add device tree bindings for A1 SoC audio clock and reset controllers. >>> >>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>> --- >>> >>> This controller has 6 mandatory and up to 20 optional clocks. To describe >>> this, I use 'additionalItems'. It produces correct processed-schema.json: >>> >>> "clock-names": { >>> "maxItems": 26, >>> "items": [ >>> { >>> "const": "pclk" >>> }, >>> { >>> "const": "dds_in" >>> }, >>> { >>> "const": "fclk_div2" >>> }, >>> { >>> "const": "fclk_div3" >>> }, >>> { >>> "const": "hifi_pll" >>> }, >>> { >>> "const": "xtal" >>> } >>> ], >>> "additionalItems": { >>> "oneOf": [ >>> { >>> "pattern": "^slv_sclk[0-9]$" >>> }, >>> { >>> "pattern": "^slv_lrclk[0-9]$" >>> } >>> ] >>> }, >>> "type": "array", >>> "minItems": 6 >>> }, >>> >>> and it behaves as expected. However, the checking is followed by >>> complaints like this: >>> >>> Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' >>> >>> And indeed, 'additionalItems' has boolean type in meta-schema. So, how to >>> do it right? >> >> The meta-schemas are written both to prevent nonsense that json-schema >> allows by default (e.g additionalitems (wrong case)) and constraints to >> follow the patterns we expect. I'm happy to loosen the latter case if >> there's really a need. >> >> Generally, most bindings shouldn't be using 'additionalItems' at all as >> all entries should be defined, but there's a few exceptions. Here, the >> only reasoning I see is 26 entries is a lot to write out, but that >> wouldn't really justify it. > > Writing a lot of entries don't scary me too much, but the reason is that > the existence of optional clock sources depends on schematics. Also, we Aren't you documenting SoC component, not a board? So how exactly it depends on schematics? SoC is done or not done... > unable to declare dt-nodes for 'clocks' array in any generic way, > because their declaration would depends on that what is actually > connected to the SoC (dt-node could be "fixed-clock" with specific rate > or something else). So these are clock inputs to the SoC? > > By the way, I don't know any example (neither for A1 SoC nor for other > Amlogic's SoCs) where these optional clocks are used, but they are > allowed by hw. > > This is my understanding of this controller. I hope, Jerome Brunet will > clarify how it actually works. Best regards, Krzysztof
On 21/04/2024 17:35, Jan Dakinevich wrote: >>>> >>>>> + }; >>>>> + >>>>> + clkc_audio_vad: clock-controller@fe054800 { >>>> >>>> Just keep one example. It's basically almost the same. >>>> >>> >>> The worth of this duplication is to show how a clock from second >>> controller (<&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>) is used by first >>> one. May be it would be better to keep it... What do you think? >> >> I don't understand what is worth here. Using clocks is kind of obvious? >> What's special? >> > > The special is that the clock "pclk" for "clkc_audio" must be > <&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>. This thing is not obvious. I So you want to document non-obvious SoC architecture via example, not via actual documentation. Plus you want to document it for purpose of ...? Isn't this SoC component, so once you write DTSI it is done? I fail to see any logic in this, but maybe the binding is kind of special, misrepresented or hardware is different? But the subject clearly states it is part of SoC, so dunno... > can keep only "clkc_audio" node here, but reference to "clkc_audio_vad" > will be undefined in example. Is it okay? Just like all phandles. Best regards, Krzysztof
On 4/21/24 21:14, Krzysztof Kozlowski wrote: > On 20/04/2024 18:15, Jan Dakinevich wrote: >> >> >> On 4/20/24 00:09, Rob Herring wrote: >>> On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote: >>>> Add device tree bindings for A1 SoC audio clock and reset controllers. >>>> >>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>> --- >>>> >>>> This controller has 6 mandatory and up to 20 optional clocks. To describe >>>> this, I use 'additionalItems'. It produces correct processed-schema.json: >>>> >>>> "clock-names": { >>>> "maxItems": 26, >>>> "items": [ >>>> { >>>> "const": "pclk" >>>> }, >>>> { >>>> "const": "dds_in" >>>> }, >>>> { >>>> "const": "fclk_div2" >>>> }, >>>> { >>>> "const": "fclk_div3" >>>> }, >>>> { >>>> "const": "hifi_pll" >>>> }, >>>> { >>>> "const": "xtal" >>>> } >>>> ], >>>> "additionalItems": { >>>> "oneOf": [ >>>> { >>>> "pattern": "^slv_sclk[0-9]$" >>>> }, >>>> { >>>> "pattern": "^slv_lrclk[0-9]$" >>>> } >>>> ] >>>> }, >>>> "type": "array", >>>> "minItems": 6 >>>> }, >>>> >>>> and it behaves as expected. However, the checking is followed by >>>> complaints like this: >>>> >>>> Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' >>>> >>>> And indeed, 'additionalItems' has boolean type in meta-schema. So, how to >>>> do it right? >>> >>> The meta-schemas are written both to prevent nonsense that json-schema >>> allows by default (e.g additionalitems (wrong case)) and constraints to >>> follow the patterns we expect. I'm happy to loosen the latter case if >>> there's really a need. >>> >>> Generally, most bindings shouldn't be using 'additionalItems' at all as >>> all entries should be defined, but there's a few exceptions. Here, the >>> only reasoning I see is 26 entries is a lot to write out, but that >>> wouldn't really justify it. >> >> Writing a lot of entries don't scary me too much, but the reason is that >> the existence of optional clock sources depends on schematics. Also, we > > Aren't you documenting SoC component, not a board? Yes, I'm documenting SoC component. And the feature of this component is that its external clock inputs are not mandatory. > So how exactly it > depends on schematics? SoC is done or not done... > Schematics determines which external clock sources exist. >> unable to declare dt-nodes for 'clocks' array in any generic way, >> because their declaration would depends on that what is actually >> connected to the SoC (dt-node could be "fixed-clock" with specific rate >> or something else). > > So these are clock inputs to the SoC? > Yes, these are clock inputs to the SoC, and external hardware could be connected to them. >> >> By the way, I don't know any example (neither for A1 SoC nor for other >> Amlogic's SoCs) where these optional clocks are used, but they are >> allowed by hw. >> >> This is my understanding of this controller. I hope, Jerome Brunet will >> clarify how it actually works. > > Best regards, > Krzysztof >
On Sun 21 Apr 2024 at 20:14, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On 20/04/2024 18:15, Jan Dakinevich wrote: >> >> >> On 4/20/24 00:09, Rob Herring wrote: >>> On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote: >>>> Add device tree bindings for A1 SoC audio clock and reset controllers. >>>> >>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>> --- >>>> >>>> This controller has 6 mandatory and up to 20 optional clocks. To describe >>>> this, I use 'additionalItems'. It produces correct processed-schema.json: >>>> >>>> "clock-names": { >>>> "maxItems": 26, >>>> "items": [ >>>> { >>>> "const": "pclk" >>>> }, >>>> { >>>> "const": "dds_in" >>>> }, >>>> { >>>> "const": "fclk_div2" >>>> }, >>>> { >>>> "const": "fclk_div3" >>>> }, >>>> { >>>> "const": "hifi_pll" >>>> }, >>>> { >>>> "const": "xtal" >>>> } >>>> ], >>>> "additionalItems": { >>>> "oneOf": [ >>>> { >>>> "pattern": "^slv_sclk[0-9]$" >>>> }, >>>> { >>>> "pattern": "^slv_lrclk[0-9]$" >>>> } >>>> ] >>>> }, >>>> "type": "array", >>>> "minItems": 6 >>>> }, >>>> >>>> and it behaves as expected. However, the checking is followed by >>>> complaints like this: >>>> >>>> Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' >>>> >>>> And indeed, 'additionalItems' has boolean type in meta-schema. So, how to >>>> do it right? >>> >>> The meta-schemas are written both to prevent nonsense that json-schema >>> allows by default (e.g additionalitems (wrong case)) and constraints to >>> follow the patterns we expect. I'm happy to loosen the latter case if >>> there's really a need. >>> >>> Generally, most bindings shouldn't be using 'additionalItems' at all as >>> all entries should be defined, but there's a few exceptions. Here, the >>> only reasoning I see is 26 entries is a lot to write out, but that >>> wouldn't really justify it. >> >> Writing a lot of entries don't scary me too much, but the reason is that >> the existence of optional clock sources depends on schematics. Also, we > > Aren't you documenting SoC component, not a board? So how exactly it > depends on schematics? SoC is done or not done... > >> unable to declare dt-nodes for 'clocks' array in any generic way, >> because their declaration would depends on that what is actually >> connected to the SoC (dt-node could be "fixed-clock" with specific rate >> or something else). > > So these are clock inputs to the SoC? > Yes, possibly. Like an external crystal or a set clocks provided by an external codec where the codec is the clock master of the link. This is same case as the AXG that was discussed here: https://lore.kernel.org/linux-devicetree/20230808194811.113087-1-alexander.stein@mailbox.org/ IMO, like the AXG, only the pclk is a required clock. All the others - master and slave clocks - are optional. The controller is designed to operate with grounded inputs >> >> By the way, I don't know any example (neither for A1 SoC nor for other >> Amlogic's SoCs) where these optional clocks are used, but they are >> allowed by hw. Those scenario exists and have been tested. There is just no dts using that upstream because they are all mostly copy of the AML ref design. >> >> This is my understanding of this controller. I hope, Jerome Brunet will >> clarify how it actually works. > I think the simpliest way to deal with this to just list all the clocks with 'minItems = 1'. It is going be hard to read with a lot of '<0>,' in the DTS when do need those slave clocks but at least the binding doc will be simple. > Best regards, > Krzysztof If you are going ahead with this, please name the file amlogic,axg-audio-clkc.yaml because this is really the first controller of the type and is meant to be documented in the same file. You are free to handle the conversion of the AXG at the same time if you'd like. It would be much appreciated if you do.
On Fri 19 Apr 2024 at 15:58, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > Add device tree bindings for A1 SoC audio clock and reset controllers. > > Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> > --- > > This controller has 6 mandatory and up to 20 optional clocks. To describe > this, I use 'additionalItems'. It produces correct processed-schema.json: > > "clock-names": { > "maxItems": 26, > "items": [ > { > "const": "pclk" > }, > { > "const": "dds_in" > }, > { > "const": "fclk_div2" > }, > { > "const": "fclk_div3" > }, > { > "const": "hifi_pll" > }, > { > "const": "xtal" > } > ], > "additionalItems": { > "oneOf": [ > { > "pattern": "^slv_sclk[0-9]$" > }, > { > "pattern": "^slv_lrclk[0-9]$" > } > ] > }, > "type": "array", > "minItems": 6 > }, > > and it behaves as expected. However, the checking is followed by > complaints like this: > > Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' > > And indeed, 'additionalItems' has boolean type in meta-schema. So, how to > do it right? > --- > .../bindings/clock/amlogic,a1-audio-clkc.yaml | 124 ++++++++++++++++++ > .../dt-bindings/clock/amlogic,a1-audio-clkc.h | 122 +++++++++++++++++ > .../reset/amlogic,meson-a1-audio-reset.h | 29 ++++ > 3 files changed, 275 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > create mode 100644 include/dt-bindings/clock/amlogic,a1-audio-clkc.h > create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h > > diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > new file mode 100644 > index 000000000000..40a0af3635f4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml > @@ -0,0 +1,124 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/amlogic,a1-audio-clkc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Amlogic A1 Audio Clock Control Unit and Reset Controller > + > +maintainers: > + - Neil Armstrong <neil.armstrong@linaro.org> > + - Jerome Brunet <jbrunet@baylibre.com> > + - Jan Dakinevich <jan.dakinevich@salutedevices.com> > + > +properties: > + compatible: > + enum: > + - amlogic,a1-audio-clkc > + - amlogic,a1-audio-vad-clkc > + > + '#clock-cells': > + const: 1 > + > + '#reset-cells': > + const: 1 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 26 > + items: > + - description: input main peripheral bus clock > + - description: input dds_in > + - description: input fixed pll div2 > + - description: input fixed pll div3 > + - description: input hifi_pll > + - description: input oscillator (usually at 24MHz) > + additionalItems: > + oneOf: > + - description: slv_sclk[0-9] - slave bit clocks provided by external components > + - description: slv_lrclk[0-9]- slave sample clocks provided by external components > + > + clock-names: > + maxItems: 26 > + items: > + - const: pclk > + - const: dds_in > + - const: fclk_div2 > + - const: fclk_div3 > + - const: hifi_pll > + - const: xtal > + additionalItems: > + oneOf: > + - pattern: "^slv_sclk[0-9]$" > + - pattern: "^slv_lrclk[0-9]$" > + > +required: > + - compatible > + - '#clock-cells' > + - reg > + - clocks > + - clock-names > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: amlogic,a1-audio-clkc > + then: > + required: > + - '#reset-cells' > + else: > + properties: > + '#reset-cells': false > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/amlogic,a1-pll-clkc.h> > + #include <dt-bindings/clock/amlogic,a1-peripherals-clkc.h> > + #include <dt-bindings/clock/amlogic,a1-audio-clkc.h> > + audio { > + #address-cells = <2>; > + #size-cells = <2>; > + > + clkc_audio: clock-controller@fe050000 { > + compatible = "amlogic,a1-audio-clkc"; > + reg = <0x0 0xfe050000 0x0 0xb0>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + clocks = <&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>, > + <&clkc_periphs CLKID_DDS_IN>, > + <&clkc_pll CLKID_FCLK_DIV2>, > + <&clkc_pll CLKID_FCLK_DIV3>, > + <&clkc_pll CLKID_HIFI_PLL>, > + <&xtal>; > + clock-names = "pclk", > + "dds_in", > + "fclk_div2", > + "fclk_div3", > + "hifi_pll", > + "xtal"; > + }; > + > + clkc_audio_vad: clock-controller@fe054800 { > + compatible = "amlogic,a1-audio2-clkc"; > + reg = <0x0 0xfe054800 0x0 0x20>; > + #clock-cells = <1>; > + clocks = <&clkc_periphs CLKID_AUDIO>, > + <&clkc_periphs CLKID_DDS_IN>, > + <&clkc_pll CLKID_FCLK_DIV2>, > + <&clkc_pll CLKID_FCLK_DIV3>, > + <&clkc_pll CLKID_HIFI_PLL>, > + <&xtal>; > + clock-names = "pclk", > + "dds_in", > + "fclk_div2", > + "fclk_div3", > + "hifi_pll", > + "xtal"; > + }; > + }; > diff --git a/include/dt-bindings/clock/amlogic,a1-audio-clkc.h b/include/dt-bindings/clock/amlogic,a1-audio-clkc.h > new file mode 100644 > index 000000000000..6534d1878816 > --- /dev/null > +++ b/include/dt-bindings/clock/amlogic,a1-audio-clkc.h > @@ -0,0 +1,122 @@ > +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */ > +/* > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved. > + * > + * Author: Jan Dakinevich <jan.dakinevich@salutedevices.com> > + */ > + > +#ifndef __A1_AUDIO_CLKC_BINDINGS_H > +#define __A1_AUDIO_CLKC_BINDINGS_H > + > +#define AUD_CLKID_DDR_ARB 1 > +#define AUD_CLKID_TDMIN_A 2 > +#define AUD_CLKID_TDMIN_B 3 > +#define AUD_CLKID_TDMIN_LB 4 > +#define AUD_CLKID_LOOPBACK 5 > +#define AUD_CLKID_TDMOUT_A 6 > +#define AUD_CLKID_TDMOUT_B 7 > +#define AUD_CLKID_FRDDR_A 8 > +#define AUD_CLKID_FRDDR_B 9 > +#define AUD_CLKID_TODDR_A 10 > +#define AUD_CLKID_TODDR_B 11 > +#define AUD_CLKID_SPDIFIN 12 > +#define AUD_CLKID_RESAMPLE 13 > +#define AUD_CLKID_EQDRC 14 > +#define AUD_CLKID_LOCKER 15 > +#define AUD_CLKID_MST_A_MCLK_SEL 16 > +#define AUD_CLKID_MST_A_MCLK_DIV 17 > +#define AUD_CLKID_MST_A_MCLK 18 > +#define AUD_CLKID_MST_B_MCLK_SEL 19 > +#define AUD_CLKID_MST_B_MCLK_DIV 20 > +#define AUD_CLKID_MST_B_MCLK 21 > +#define AUD_CLKID_MST_C_MCLK_SEL 22 > +#define AUD_CLKID_MST_C_MCLK_DIV 23 > +#define AUD_CLKID_MST_C_MCLK 24 > +#define AUD_CLKID_MST_D_MCLK_SEL 25 > +#define AUD_CLKID_MST_D_MCLK_DIV 26 > +#define AUD_CLKID_MST_D_MCLK 27 > +#define AUD_CLKID_SPDIFIN_CLK_SEL 28 > +#define AUD_CLKID_SPDIFIN_CLK_DIV 29 > +#define AUD_CLKID_SPDIFIN_CLK 30 > +#define AUD_CLKID_RESAMPLE_CLK_SEL 31 > +#define AUD_CLKID_RESAMPLE_CLK_DIV 32 > +#define AUD_CLKID_RESAMPLE_CLK 33 > +#define AUD_CLKID_LOCKER_IN_CLK_SEL 34 > +#define AUD_CLKID_LOCKER_IN_CLK_DIV 35 > +#define AUD_CLKID_LOCKER_IN_CLK 36 > +#define AUD_CLKID_LOCKER_OUT_CLK_SEL 37 > +#define AUD_CLKID_LOCKER_OUT_CLK_DIV 38 > +#define AUD_CLKID_LOCKER_OUT_CLK 39 > +#define AUD_CLKID_EQDRC_CLK_SEL 40 > +#define AUD_CLKID_EQDRC_CLK_DIV 41 > +#define AUD_CLKID_EQDRC_CLK 42 > +#define AUD_CLKID_MST_A_SCLK_PRE_EN 43 > +#define AUD_CLKID_MST_A_SCLK_DIV 44 > +#define AUD_CLKID_MST_A_SCLK_POST_EN 45 > +#define AUD_CLKID_MST_A_SCLK 46 > +#define AUD_CLKID_MST_B_SCLK_PRE_EN 47 > +#define AUD_CLKID_MST_B_SCLK_DIV 48 > +#define AUD_CLKID_MST_B_SCLK_POST_EN 49 > +#define AUD_CLKID_MST_B_SCLK 50 > +#define AUD_CLKID_MST_C_SCLK_PRE_EN 51 > +#define AUD_CLKID_MST_C_SCLK_DIV 52 > +#define AUD_CLKID_MST_C_SCLK_POST_EN 53 > +#define AUD_CLKID_MST_C_SCLK 54 > +#define AUD_CLKID_MST_D_SCLK_PRE_EN 55 > +#define AUD_CLKID_MST_D_SCLK_DIV 56 > +#define AUD_CLKID_MST_D_SCLK_POST_EN 57 > +#define AUD_CLKID_MST_D_SCLK 58 > +#define AUD_CLKID_MST_A_LRCLK_DIV 59 > +#define AUD_CLKID_MST_A_LRCLK 60 > +#define AUD_CLKID_MST_B_LRCLK_DIV 61 > +#define AUD_CLKID_MST_B_LRCLK 62 > +#define AUD_CLKID_MST_C_LRCLK_DIV 63 > +#define AUD_CLKID_MST_C_LRCLK 64 > +#define AUD_CLKID_MST_D_LRCLK_DIV 65 > +#define AUD_CLKID_MST_D_LRCLK 66 > +#define AUD_CLKID_TDMIN_A_SCLK_SEL 67 > +#define AUD_CLKID_TDMIN_A_SCLK_PRE_EN 68 > +#define AUD_CLKID_TDMIN_A_SCLK_POST_EN 69 > +#define AUD_CLKID_TDMIN_A_SCLK 70 > +#define AUD_CLKID_TDMIN_A_LRCLK 71 > +#define AUD_CLKID_TDMIN_B_SCLK_SEL 72 > +#define AUD_CLKID_TDMIN_B_SCLK_PRE_EN 73 > +#define AUD_CLKID_TDMIN_B_SCLK_POST_EN 74 > +#define AUD_CLKID_TDMIN_B_SCLK 75 > +#define AUD_CLKID_TDMIN_B_LRCLK 76 > +#define AUD_CLKID_TDMIN_LB_SCLK_SEL 77 > +#define AUD_CLKID_TDMIN_LB_SCLK_PRE_EN 78 > +#define AUD_CLKID_TDMIN_LB_SCLK_POST_EN 79 > +#define AUD_CLKID_TDMIN_LB_SCLK 80 > +#define AUD_CLKID_TDMIN_LB_LRCLK 81 > +#define AUD_CLKID_TDMOUT_A_SCLK_SEL 82 > +#define AUD_CLKID_TDMOUT_A_SCLK_PRE_EN 83 > +#define AUD_CLKID_TDMOUT_A_SCLK_POST_EN 84 > +#define AUD_CLKID_TDMOUT_A_SCLK 85 > +#define AUD_CLKID_TDMOUT_A_LRCLK 86 > +#define AUD_CLKID_TDMOUT_B_SCLK_SEL 87 > +#define AUD_CLKID_TDMOUT_B_SCLK_PRE_EN 88 > +#define AUD_CLKID_TDMOUT_B_SCLK_POST_EN 89 > +#define AUD_CLKID_TDMOUT_B_SCLK 90 > +#define AUD_CLKID_TDMOUT_B_LRCLK 91 > + > +#define AUD_CLKID_VAD_DDR_ARB 1 > +#define AUD_CLKID_VAD_PDM 2 > +#define AUD_CLKID_VAD_TDMIN 3 > +#define AUD_CLKID_VAD_TODDR 4 > +#define AUD_CLKID_VAD 5 > +#define AUD_CLKID_VAD_AUDIOTOP 6 > +#define AUD_CLKID_VAD_MCLK_SEL 7 > +#define AUD_CLKID_VAD_MCLK_DIV 8 > +#define AUD_CLKID_VAD_MCLK 9 > +#define AUD_CLKID_VAD_CLK_SEL 10 > +#define AUD_CLKID_VAD_CLK_DIV 11 > +#define AUD_CLKID_VAD_CLK 12 > +#define AUD_CLKID_VAD_PDM_DCLK_SEL 13 > +#define AUD_CLKID_VAD_PDM_DCLK_DIV 14 > +#define AUD_CLKID_VAD_PDM_DCLK 15 > +#define AUD_CLKID_VAD_PDM_SYSCLK_SEL 16 > +#define AUD_CLKID_VAD_PDM_SYSCLK_DIV 17 > +#define AUD_CLKID_VAD_PDM_SYSCLK 18 > + Again, please split the a1 and a1-vad controller. They are independent and there is no reason to use a single file for them > +#endif /* __A1_AUDIO_CLKC_BINDINGS_H */ > diff --git a/include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h b/include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h > new file mode 100644 > index 000000000000..653fddba1d8f > --- /dev/null > +++ b/include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */ > +/* > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved. > + * > + * Author: Jan Dakinevich <jan.dakinevich@salutedevices.com> > + */ > + > +#ifndef _DT_BINDINGS_AMLOGIC_MESON_A1_AUDIO_RESET_H > +#define _DT_BINDINGS_AMLOGIC_MESON_A1_AUDIO_RESET_H > + > +#define AUD_RESET_DDRARB 0 > +#define AUD_RESET_TDMIN_A 1 > +#define AUD_RESET_TDMIN_B 2 > +#define AUD_RESET_TDMIN_LB 3 > +#define AUD_RESET_LOOPBACK 4 > +#define AUD_RESET_TDMOUT_A 5 > +#define AUD_RESET_TDMOUT_B 6 > +#define AUD_RESET_FRDDR_A 7 > +#define AUD_RESET_FRDDR_B 8 > +#define AUD_RESET_TODDR_A 9 > +#define AUD_RESET_TODDR_B 10 > +#define AUD_RESET_SPDIFIN 11 > +#define AUD_RESET_RESAMPLE 12 > +#define AUD_RESET_EQDRC 13 > +#define AUD_RESET_LOCKER 14 > +#define AUD_RESET_TOACODEC 30 > +#define AUD_RESET_CLKTREE 31 > + > +#endif /* _DT_BINDINGS_AMLOGIC_MESON_A1_AUDIO_RESET_H */
On Sat 20 Apr 2024 at 17:48, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > On 4/19/24 17:06, Krzysztof Kozlowski wrote: >> On 19/04/2024 14:58, Jan Dakinevich wrote: >>> Add device tree bindings for A1 SoC audio clock and reset controllers. >>> >>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >> >> This is still RFC, so not ready. >> >> Limited, incomplete review follows. Full review will be provided when >> the work is ready. >> >> Drop "driver" references, e.g. from subject. Bindings are about hardware. >> >> >> .... >> >>> + >>> + clocks: >>> + maxItems: 26 >>> + items: >>> + - description: input main peripheral bus clock >>> + - description: input dds_in >>> + - description: input fixed pll div2 >>> + - description: input fixed pll div3 >>> + - description: input hifi_pll >>> + - description: input oscillator (usually at 24MHz) >>> + additionalItems: >>> + oneOf: >>> + - description: slv_sclk[0-9] - slave bit clocks provided by external components >>> + - description: slv_lrclk[0-9]- slave sample clocks provided by external components >> >> What does it mean the clocks are optional? Pins could be not routed? > > Yes exactly. Pins could be routed in any combination or could be not > routed at all. It is determined by schematics and that how external > codecs are configured. > >> It's really rare case that clocks within the SoC are optional, so every >> such case is questionable. >> >> >>> + >>> + clock-names: >>> + maxItems: 26 >>> + items: >>> + - const: pclk >>> + - const: dds_in >>> + - const: fclk_div2 >>> + - const: fclk_div3 >>> + - const: hifi_pll >>> + - const: xtal >>> + additionalItems: >>> + oneOf: >>> + - pattern: "^slv_sclk[0-9]$" >>> + - pattern: "^slv_lrclk[0-9]$" >>> + >>> +required: >>> + - compatible >>> + - '#clock-cells' >>> + - reg >>> + - clocks >>> + - clock-names >>> + >>> +allOf: >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + const: amlogic,a1-audio-clkc >>> + then: >>> + required: >>> + - '#reset-cells' >>> + else: >>> + properties: >>> + '#reset-cells': false >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/clock/amlogic,a1-pll-clkc.h> >>> + #include <dt-bindings/clock/amlogic,a1-peripherals-clkc.h> >>> + #include <dt-bindings/clock/amlogic,a1-audio-clkc.h> >>> + audio { >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + >>> + clkc_audio: clock-controller@fe050000 { >>> + compatible = "amlogic,a1-audio-clkc"; >>> + reg = <0x0 0xfe050000 0x0 0xb0>; >>> + #clock-cells = <1>; >>> + #reset-cells = <1>; >>> + clocks = <&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>, >>> + <&clkc_periphs CLKID_DDS_IN>, >>> + <&clkc_pll CLKID_FCLK_DIV2>, >>> + <&clkc_pll CLKID_FCLK_DIV3>, >>> + <&clkc_pll CLKID_HIFI_PLL>, >>> + <&xtal>; >>> + clock-names = "pclk", >>> + "dds_in", >>> + "fclk_div2", >>> + "fclk_div3", >>> + "hifi_pll", >>> + "xtal"; >> >> Make it complete - list all clocks. >> > > You mean, all optional clocks should be mentioned here. Right? > >>> + }; >>> + >>> + clkc_audio_vad: clock-controller@fe054800 { >> >> Just keep one example. It's basically almost the same. >> > > The worth of this duplication is to show how a clock from second > controller (<&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>) is used by first > one. May be it would be better to keep it... What do you think? If you think that is worth mentioning, make it part of the documentation, not the example. > >> >> >> Best regards, >> Krzysztof >>
On Mon 22 Apr 2024 at 09:16, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Sun 21 Apr 2024 at 20:14, Krzysztof Kozlowski <krzk@kernel.org> wrote: > >> On 20/04/2024 18:15, Jan Dakinevich wrote: >>> >>> >>> On 4/20/24 00:09, Rob Herring wrote: >>>> On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote: >>>>> Add device tree bindings for A1 SoC audio clock and reset controllers. >>>>> >>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>>> --- >>>>> >>>>> This controller has 6 mandatory and up to 20 optional clocks. To describe >>>>> this, I use 'additionalItems'. It produces correct processed-schema.json: >>>>> >>>>> "clock-names": { >>>>> "maxItems": 26, >>>>> "items": [ >>>>> { >>>>> "const": "pclk" >>>>> }, >>>>> { >>>>> "const": "dds_in" >>>>> }, >>>>> { >>>>> "const": "fclk_div2" >>>>> }, >>>>> { >>>>> "const": "fclk_div3" >>>>> }, >>>>> { >>>>> "const": "hifi_pll" >>>>> }, >>>>> { >>>>> "const": "xtal" >>>>> } >>>>> ], >>>>> "additionalItems": { >>>>> "oneOf": [ >>>>> { >>>>> "pattern": "^slv_sclk[0-9]$" >>>>> }, >>>>> { >>>>> "pattern": "^slv_lrclk[0-9]$" >>>>> } >>>>> ] >>>>> }, >>>>> "type": "array", >>>>> "minItems": 6 >>>>> }, >>>>> >>>>> and it behaves as expected. However, the checking is followed by >>>>> complaints like this: >>>>> >>>>> Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' >>>>> >>>>> And indeed, 'additionalItems' has boolean type in meta-schema. So, how to >>>>> do it right? >>>> >>>> The meta-schemas are written both to prevent nonsense that json-schema >>>> allows by default (e.g additionalitems (wrong case)) and constraints to >>>> follow the patterns we expect. I'm happy to loosen the latter case if >>>> there's really a need. >>>> >>>> Generally, most bindings shouldn't be using 'additionalItems' at all as >>>> all entries should be defined, but there's a few exceptions. Here, the >>>> only reasoning I see is 26 entries is a lot to write out, but that >>>> wouldn't really justify it. >>> >>> Writing a lot of entries don't scary me too much, but the reason is that >>> the existence of optional clock sources depends on schematics. Also, we >> >> Aren't you documenting SoC component, not a board? So how exactly it >> depends on schematics? SoC is done or not done... >> >>> unable to declare dt-nodes for 'clocks' array in any generic way, >>> because their declaration would depends on that what is actually >>> connected to the SoC (dt-node could be "fixed-clock" with specific rate >>> or something else). >> >> So these are clock inputs to the SoC? >> > > Yes, possibly. > Like an external crystal or a set clocks provided by an external codec > where the codec is the clock master of the link. > > This is same case as the AXG that was discussed here: > https://lore.kernel.org/linux-devicetree/20230808194811.113087-1-alexander.stein@mailbox.org/ > > IMO, like the AXG, only the pclk is a required clock. > All the others - master and slave clocks - are optional. > The controller is designed to operate with grounded inputs Looking again at the implementation of the controller, there is a clear indication in patch 3 that the controller interface is the same as the AXG and that the above statement is true. The AXG had 8 master clocks wired in. The A1 just has 5 - and 3 grounded master clocks. This is why you to had to provide a mux input table to skip the grounded inputs. You would not have to do so if the controller was properly declared with the 8 master clock input, as it actually is. It also shows that it is a bad idea to name input after what is coming in (like you do with "dds_in" or "fclk_div2") instead of what they actually are like in the AXG (mst0, mst1, etc ...) > >>> >>> By the way, I don't know any example (neither for A1 SoC nor for other >>> Amlogic's SoCs) where these optional clocks are used, but they are >>> allowed by hw. > > Those scenario exists and have been tested. There is just no dts using > that upstream because they are all mostly copy of the AML ref design. > >>> >>> This is my understanding of this controller. I hope, Jerome Brunet will >>> clarify how it actually works. >> > > I think the simpliest way to deal with this to just list all the clocks > with 'minItems = 1'. It is going be hard to read with a lot of '<0>,' in > the DTS when do need those slave clocks but at least the binding doc > will be simple. > >> Best regards, >> Krzysztof > > If you are going ahead with this, please name the file > amlogic,axg-audio-clkc.yaml because this is really the first controller > of the type and is meant to be documented in the same file. > > You are free to handle the conversion of the AXG at the same time if > you'd like. It would be much appreciated if you do.
On 4/22/24 10:57, Jerome Brunet wrote: > > On Mon 22 Apr 2024 at 09:16, Jerome Brunet <jbrunet@baylibre.com> wrote: > >> On Sun 21 Apr 2024 at 20:14, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >>> On 20/04/2024 18:15, Jan Dakinevich wrote: >>>> >>>> >>>> On 4/20/24 00:09, Rob Herring wrote: >>>>> On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote: >>>>>> Add device tree bindings for A1 SoC audio clock and reset controllers. >>>>>> >>>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>>>> --- >>>>>> >>>>>> This controller has 6 mandatory and up to 20 optional clocks. To describe >>>>>> this, I use 'additionalItems'. It produces correct processed-schema.json: >>>>>> >>>>>> "clock-names": { >>>>>> "maxItems": 26, >>>>>> "items": [ >>>>>> { >>>>>> "const": "pclk" >>>>>> }, >>>>>> { >>>>>> "const": "dds_in" >>>>>> }, >>>>>> { >>>>>> "const": "fclk_div2" >>>>>> }, >>>>>> { >>>>>> "const": "fclk_div3" >>>>>> }, >>>>>> { >>>>>> "const": "hifi_pll" >>>>>> }, >>>>>> { >>>>>> "const": "xtal" >>>>>> } >>>>>> ], >>>>>> "additionalItems": { >>>>>> "oneOf": [ >>>>>> { >>>>>> "pattern": "^slv_sclk[0-9]$" >>>>>> }, >>>>>> { >>>>>> "pattern": "^slv_lrclk[0-9]$" >>>>>> } >>>>>> ] >>>>>> }, >>>>>> "type": "array", >>>>>> "minItems": 6 >>>>>> }, >>>>>> >>>>>> and it behaves as expected. However, the checking is followed by >>>>>> complaints like this: >>>>>> >>>>>> Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' >>>>>> >>>>>> And indeed, 'additionalItems' has boolean type in meta-schema. So, how to >>>>>> do it right? >>>>> >>>>> The meta-schemas are written both to prevent nonsense that json-schema >>>>> allows by default (e.g additionalitems (wrong case)) and constraints to >>>>> follow the patterns we expect. I'm happy to loosen the latter case if >>>>> there's really a need. >>>>> >>>>> Generally, most bindings shouldn't be using 'additionalItems' at all as >>>>> all entries should be defined, but there's a few exceptions. Here, the >>>>> only reasoning I see is 26 entries is a lot to write out, but that >>>>> wouldn't really justify it. >>>> >>>> Writing a lot of entries don't scary me too much, but the reason is that >>>> the existence of optional clock sources depends on schematics. Also, we >>> >>> Aren't you documenting SoC component, not a board? So how exactly it >>> depends on schematics? SoC is done or not done... >>> >>>> unable to declare dt-nodes for 'clocks' array in any generic way, >>>> because their declaration would depends on that what is actually >>>> connected to the SoC (dt-node could be "fixed-clock" with specific rate >>>> or something else). >>> >>> So these are clock inputs to the SoC? >>> >> >> Yes, possibly. >> Like an external crystal or a set clocks provided by an external codec >> where the codec is the clock master of the link. >> >> This is same case as the AXG that was discussed here: >> https://lore.kernel.org/linux-devicetree/20230808194811.113087-1-alexander.stein@mailbox.org/ >> >> IMO, like the AXG, only the pclk is a required clock. >> All the others - master and slave clocks - are optional. >> The controller is designed to operate with grounded inputs > > Looking again at the implementation of the controller, there is a clear > indication in patch 3 that the controller interface is the same as the > AXG and that the above statement is true. > > The AXG had 8 master clocks wired in. The A1 just has 5 - and 3 grounded > master clocks. This is why you to had to provide a mux input table to > skip the grounded inputs. You would not have to do so if the controller was > properly declared with the 8 master clock input, as it actually is. > For simplicity, I could make something like this in device tree: clocks = <&clk0, &clk1, &clk2, &clk3, &clk4, 0, 0, 0> clock-names = <"mst_in0", "mst_in1", "mst_in2" "mst_in2" "mst_in3" "mst_in4" "mst_in5" "mst_in6" "mst_in7"> But I don't see in the doc that the last 3 clocks are grounded to anywhere. It will be just community's assumption about internals of the controller. Anyway, I still don't understand what to do with external slv_* clocks. I can do the same as in example above: list slv_(s|lr)clk[0-9] in "clock-names" and fill the rest if "clocks" by "0" phandles. > It also shows that it is a bad idea to name input after what is coming > in (like you do with "dds_in" or "fclk_div2") instead of what they > actually are like in the AXG (mst0, mst1, etc ...) > I agree, these are not the best names. >> >>>> >>>> By the way, I don't know any example (neither for A1 SoC nor for other >>>> Amlogic's SoCs) where these optional clocks are used, but they are >>>> allowed by hw. >> >> Those scenario exists and have been tested. There is just no dts using >> that upstream because they are all mostly copy of the AML ref design. >> >>>> >>>> This is my understanding of this controller. I hope, Jerome Brunet will >>>> clarify how it actually works. >>> >> >> I think the simpliest way to deal with this to just list all the clocks >> with 'minItems = 1'. It is going be hard to read with a lot of '<0>,' in >> the DTS when do need those slave clocks but at least the binding doc >> will be simple. >> >>> Best regards, >>> Krzysztof >> >> If you are going ahead with this, please name the file >> amlogic,axg-audio-clkc.yaml because this is really the first controller >> of the type and is meant to be documented in the same file. >> >> You are free to handle the conversion of the AXG at the same time if >> you'd like. It would be much appreciated if you do. > >
On Mon 22 Apr 2024 at 17:31, Jan Dakinevich <jan.dakinevich@salutedevices.com> wrote: > On 4/22/24 10:57, Jerome Brunet wrote: >> >> On Mon 22 Apr 2024 at 09:16, Jerome Brunet <jbrunet@baylibre.com> wrote: >> >>> On Sun 21 Apr 2024 at 20:14, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> >>>> On 20/04/2024 18:15, Jan Dakinevich wrote: >>>>> >>>>> >>>>> On 4/20/24 00:09, Rob Herring wrote: >>>>>> On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote: >>>>>>> Add device tree bindings for A1 SoC audio clock and reset controllers. >>>>>>> >>>>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>>>>> --- >>>>>>> >>>>>>> This controller has 6 mandatory and up to 20 optional clocks. To describe >>>>>>> this, I use 'additionalItems'. It produces correct processed-schema.json: >>>>>>> >>>>>>> "clock-names": { >>>>>>> "maxItems": 26, >>>>>>> "items": [ >>>>>>> { >>>>>>> "const": "pclk" >>>>>>> }, >>>>>>> { >>>>>>> "const": "dds_in" >>>>>>> }, >>>>>>> { >>>>>>> "const": "fclk_div2" >>>>>>> }, >>>>>>> { >>>>>>> "const": "fclk_div3" >>>>>>> }, >>>>>>> { >>>>>>> "const": "hifi_pll" >>>>>>> }, >>>>>>> { >>>>>>> "const": "xtal" >>>>>>> } >>>>>>> ], >>>>>>> "additionalItems": { >>>>>>> "oneOf": [ >>>>>>> { >>>>>>> "pattern": "^slv_sclk[0-9]$" >>>>>>> }, >>>>>>> { >>>>>>> "pattern": "^slv_lrclk[0-9]$" >>>>>>> } >>>>>>> ] >>>>>>> }, >>>>>>> "type": "array", >>>>>>> "minItems": 6 >>>>>>> }, >>>>>>> >>>>>>> and it behaves as expected. However, the checking is followed by >>>>>>> complaints like this: >>>>>>> >>>>>>> Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' >>>>>>> >>>>>>> And indeed, 'additionalItems' has boolean type in meta-schema. So, how to >>>>>>> do it right? >>>>>> >>>>>> The meta-schemas are written both to prevent nonsense that json-schema >>>>>> allows by default (e.g additionalitems (wrong case)) and constraints to >>>>>> follow the patterns we expect. I'm happy to loosen the latter case if >>>>>> there's really a need. >>>>>> >>>>>> Generally, most bindings shouldn't be using 'additionalItems' at all as >>>>>> all entries should be defined, but there's a few exceptions. Here, the >>>>>> only reasoning I see is 26 entries is a lot to write out, but that >>>>>> wouldn't really justify it. >>>>> >>>>> Writing a lot of entries don't scary me too much, but the reason is that >>>>> the existence of optional clock sources depends on schematics. Also, we >>>> >>>> Aren't you documenting SoC component, not a board? So how exactly it >>>> depends on schematics? SoC is done or not done... >>>> >>>>> unable to declare dt-nodes for 'clocks' array in any generic way, >>>>> because their declaration would depends on that what is actually >>>>> connected to the SoC (dt-node could be "fixed-clock" with specific rate >>>>> or something else). >>>> >>>> So these are clock inputs to the SoC? >>>> >>> >>> Yes, possibly. >>> Like an external crystal or a set clocks provided by an external codec >>> where the codec is the clock master of the link. >>> >>> This is same case as the AXG that was discussed here: >>> https://lore.kernel.org/linux-devicetree/20230808194811.113087-1-alexander.stein@mailbox.org/ >>> >>> IMO, like the AXG, only the pclk is a required clock. >>> All the others - master and slave clocks - are optional. >>> The controller is designed to operate with grounded inputs >> >> Looking again at the implementation of the controller, there is a clear >> indication in patch 3 that the controller interface is the same as the >> AXG and that the above statement is true. >> > The AXG had 8 master clocks wired in. The A1 just has 5 - and 3 grounded >> master clocks. This is why you to had to provide a mux input table to >> skip the grounded inputs. You would not have to do so if the controller was >> properly declared with the 8 master clock input, as it actually is. >> > > For simplicity, I could make something like this in device tree: > > clocks = <&clk0, > &clk1, > &clk2, > &clk3, > &clk4, > 0, > 0, > 0> > clock-names = <"mst_in0", > "mst_in1", > "mst_in2" > "mst_in2" > "mst_in3" > "mst_in4" > "mst_in5" > "mst_in6" > "mst_in7"> > > But I don't see in the doc that the last 3 clocks are grounded to > anywhere. It will be just community's assumption about internals of the > controller. Maybe so. Given how much we know about the amlogic HW, there will always be assumptions (... and corrections). It is a fare assumption to make. Looking at definitions for the master clocks or the TDM clocks, you can see that the HW is same, only what is wired in has changed. The register definition of the master clocks is still the same. You may still put a '6' in the register, you just don't know where it goes. Physically it may exist IMO. I'll agree only the 5 first mst inputs are documented ATM. Go ahead with a different interface but at least fix the names please. > > Anyway, I still don't understand what to do with external slv_* clocks. > I can do the same as in example above: list slv_(s|lr)clk[0-9] in > "clock-names" and fill the rest if "clocks" by "0" phandles. > You don't have to put them in the example when there is only <0> to the end. >> It also shows that it is a bad idea to name input after what is coming >> in (like you do with "dds_in" or "fclk_div2") instead of what they >> actually are like in the AXG (mst0, mst1, etc ...) >> > > I agree, these are not the best names. > >>> >>>>> >>>>> By the way, I don't know any example (neither for A1 SoC nor for other >>>>> Amlogic's SoCs) where these optional clocks are used, but they are >>>>> allowed by hw. >>> >>> Those scenario exists and have been tested. There is just no dts using >>> that upstream because they are all mostly copy of the AML ref design. >>> >>>>> >>>>> This is my understanding of this controller. I hope, Jerome Brunet will >>>>> clarify how it actually works. >>>> >>> >>> I think the simpliest way to deal with this to just list all the clocks >>> with 'minItems = 1'. It is going be hard to read with a lot of '<0>,' in >>> the DTS when do need those slave clocks but at least the binding doc >>> will be simple. >>> >>>> Best regards, >>>> Krzysztof >>> >>> If you are going ahead with this, please name the file >>> amlogic,axg-audio-clkc.yaml because this is really the first controller >>> of the type and is meant to be documented in the same file. >>> >>> You are free to handle the conversion of the AXG at the same time if >>> you'd like. It would be much appreciated if you do. >> >>
On 4/22/24 17:31, Jan Dakinevich wrote: > > > On 4/22/24 10:57, Jerome Brunet wrote: >> >> On Mon 22 Apr 2024 at 09:16, Jerome Brunet <jbrunet@baylibre.com> wrote: >> >>> On Sun 21 Apr 2024 at 20:14, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> >>>> On 20/04/2024 18:15, Jan Dakinevich wrote: >>>>> >>>>> >>>>> On 4/20/24 00:09, Rob Herring wrote: >>>>>> On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote: >>>>>>> Add device tree bindings for A1 SoC audio clock and reset controllers. >>>>>>> >>>>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> >>>>>>> --- >>>>>>> >>>>>>> This controller has 6 mandatory and up to 20 optional clocks. To describe >>>>>>> this, I use 'additionalItems'. It produces correct processed-schema.json: >>>>>>> >>>>>>> "clock-names": { >>>>>>> "maxItems": 26, >>>>>>> "items": [ >>>>>>> { >>>>>>> "const": "pclk" >>>>>>> }, >>>>>>> { >>>>>>> "const": "dds_in" >>>>>>> }, >>>>>>> { >>>>>>> "const": "fclk_div2" >>>>>>> }, >>>>>>> { >>>>>>> "const": "fclk_div3" >>>>>>> }, >>>>>>> { >>>>>>> "const": "hifi_pll" >>>>>>> }, >>>>>>> { >>>>>>> "const": "xtal" >>>>>>> } >>>>>>> ], >>>>>>> "additionalItems": { >>>>>>> "oneOf": [ >>>>>>> { >>>>>>> "pattern": "^slv_sclk[0-9]$" >>>>>>> }, >>>>>>> { >>>>>>> "pattern": "^slv_lrclk[0-9]$" >>>>>>> } >>>>>>> ] >>>>>>> }, >>>>>>> "type": "array", >>>>>>> "minItems": 6 >>>>>>> }, >>>>>>> >>>>>>> and it behaves as expected. However, the checking is followed by >>>>>>> complaints like this: >>>>>>> >>>>>>> Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' >>>>>>> >>>>>>> And indeed, 'additionalItems' has boolean type in meta-schema. So, how to >>>>>>> do it right? >>>>>> >>>>>> The meta-schemas are written both to prevent nonsense that json-schema >>>>>> allows by default (e.g additionalitems (wrong case)) and constraints to >>>>>> follow the patterns we expect. I'm happy to loosen the latter case if >>>>>> there's really a need. >>>>>> >>>>>> Generally, most bindings shouldn't be using 'additionalItems' at all as >>>>>> all entries should be defined, but there's a few exceptions. Here, the >>>>>> only reasoning I see is 26 entries is a lot to write out, but that >>>>>> wouldn't really justify it. >>>>> >>>>> Writing a lot of entries don't scary me too much, but the reason is that >>>>> the existence of optional clock sources depends on schematics. Also, we >>>> >>>> Aren't you documenting SoC component, not a board? So how exactly it >>>> depends on schematics? SoC is done or not done... >>>> >>>>> unable to declare dt-nodes for 'clocks' array in any generic way, >>>>> because their declaration would depends on that what is actually >>>>> connected to the SoC (dt-node could be "fixed-clock" with specific rate >>>>> or something else). >>>> >>>> So these are clock inputs to the SoC? >>>> >>> >>> Yes, possibly. >>> Like an external crystal or a set clocks provided by an external codec >>> where the codec is the clock master of the link. >>> >>> This is same case as the AXG that was discussed here: >>> https://lore.kernel.org/linux-devicetree/20230808194811.113087-1-alexander.stein@mailbox.org/ >>> >>> IMO, like the AXG, only the pclk is a required clock. >>> All the others - master and slave clocks - are optional. >>> The controller is designed to operate with grounded inputs >> >> Looking again at the implementation of the controller, there is a clear >> indication in patch 3 that the controller interface is the same as the >> AXG and that the above statement is true. >>> The AXG had 8 master clocks wired in. The A1 just has 5 - and 3 grounded >> master clocks. This is why you to had to provide a mux input table to >> skip the grounded inputs. You would not have to do so if the controller was >> properly declared with the 8 master clock input, as it actually is. >> > > For simplicity, I could make something like this in device tree: > > clocks = <&clk0, > &clk1, > &clk2, > &clk3, > &clk4, > 0, > 0, > 0> > clock-names = <"mst_in0", > "mst_in1", > "mst_in2" > "mst_in2" > "mst_in3" > "mst_in4" > "mst_in5" > "mst_in6" > "mst_in7"> > > But I don't see in the doc that the last 3 clocks are grounded to > anywhere. It will be just community's assumption about internals of the > controller. > > Anyway, I still don't understand what to do with external slv_* clocks. > I can do the same as in example above: list slv_(s|lr)clk[0-9] in > "clock-names" and fill the rest if "clocks" by "0" phandles. > Sorry, I missed that you suggested similar thing: >>> I think the simpliest way to deal with this to just list all the clocks >>> with 'minItems = 1'. It is going be hard to read with a lot of '<0>,' in >>> the DTS when do need those slave clocks but at least the binding doc >>> will be simple. But, may be it could be better to claim that all clocks are mandatory and list all of them (including slv_*)? So, 'minItems = 1' can be omitted. What do you think? clocks = <&pclk, &clk0, &clk1, &clk2, &clk3, &clk4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0>; clock-names = <"pclk", "mst_in0", "mst_in1", "mst_in2" "mst_in2" "mst_in3" "mst_in4" "mst_in5" "mst_in6" "mst_in7", "slv_sclk0", "slv_sclk1", "slv_sclk2", "slv_sclk3", "slv_sclk4", "slv_sclk5", "slv_sclk6", "slv_sclk7", "slv_sclk8", "slv_sclk9", "slv_lrclk0", "slv_lrclk1", "slv_lrclk2", "slv_lrclk3", "slv_lrclk4", "slv_lrclk5", "slv_lrclk6", "slv_lrclk7", "slv_lrclk8", "slv_lrclk9">; >> It also shows that it is a bad idea to name input after what is coming >> in (like you do with "dds_in" or "fclk_div2") instead of what they >> actually are like in the AXG (mst0, mst1, etc ...) >> > > I agree, these are not the best names. > >>> >>>>> >>>>> By the way, I don't know any example (neither for A1 SoC nor for other >>>>> Amlogic's SoCs) where these optional clocks are used, but they are >>>>> allowed by hw. >>> >>> Those scenario exists and have been tested. There is just no dts using >>> that upstream because they are all mostly copy of the AML ref design. >>> >>>>> >>>>> This is my understanding of this controller. I hope, Jerome Brunet will >>>>> clarify how it actually works. >>>> >>> >>> I think the simpliest way to deal with this to just list all the clocks >>> with 'minItems = 1'. It is going be hard to read with a lot of '<0>,' in >>> the DTS when do need those slave clocks but at least the binding doc >>> will be simple. >>> >>>> Best regards, >>>> Krzysztof >>> >>> If you are going ahead with this, please name the file >>> amlogic,axg-audio-clkc.yaml because this is really the first controller >>> of the type and is meant to be documented in the same file. >>> >>> You are free to handle the conversion of the AXG at the same time if >>> you'd like. It would be much appreciated if you do. >> >> >
diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml new file mode 100644 index 000000000000..40a0af3635f4 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml @@ -0,0 +1,124 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/amlogic,a1-audio-clkc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Amlogic A1 Audio Clock Control Unit and Reset Controller + +maintainers: + - Neil Armstrong <neil.armstrong@linaro.org> + - Jerome Brunet <jbrunet@baylibre.com> + - Jan Dakinevich <jan.dakinevich@salutedevices.com> + +properties: + compatible: + enum: + - amlogic,a1-audio-clkc + - amlogic,a1-audio-vad-clkc + + '#clock-cells': + const: 1 + + '#reset-cells': + const: 1 + + reg: + maxItems: 1 + + clocks: + maxItems: 26 + items: + - description: input main peripheral bus clock + - description: input dds_in + - description: input fixed pll div2 + - description: input fixed pll div3 + - description: input hifi_pll + - description: input oscillator (usually at 24MHz) + additionalItems: + oneOf: + - description: slv_sclk[0-9] - slave bit clocks provided by external components + - description: slv_lrclk[0-9]- slave sample clocks provided by external components + + clock-names: + maxItems: 26 + items: + - const: pclk + - const: dds_in + - const: fclk_div2 + - const: fclk_div3 + - const: hifi_pll + - const: xtal + additionalItems: + oneOf: + - pattern: "^slv_sclk[0-9]$" + - pattern: "^slv_lrclk[0-9]$" + +required: + - compatible + - '#clock-cells' + - reg + - clocks + - clock-names + +allOf: + - if: + properties: + compatible: + contains: + const: amlogic,a1-audio-clkc + then: + required: + - '#reset-cells' + else: + properties: + '#reset-cells': false + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/amlogic,a1-pll-clkc.h> + #include <dt-bindings/clock/amlogic,a1-peripherals-clkc.h> + #include <dt-bindings/clock/amlogic,a1-audio-clkc.h> + audio { + #address-cells = <2>; + #size-cells = <2>; + + clkc_audio: clock-controller@fe050000 { + compatible = "amlogic,a1-audio-clkc"; + reg = <0x0 0xfe050000 0x0 0xb0>; + #clock-cells = <1>; + #reset-cells = <1>; + clocks = <&clkc_audio_vad AUD_CLKID_VAD_AUDIOTOP>, + <&clkc_periphs CLKID_DDS_IN>, + <&clkc_pll CLKID_FCLK_DIV2>, + <&clkc_pll CLKID_FCLK_DIV3>, + <&clkc_pll CLKID_HIFI_PLL>, + <&xtal>; + clock-names = "pclk", + "dds_in", + "fclk_div2", + "fclk_div3", + "hifi_pll", + "xtal"; + }; + + clkc_audio_vad: clock-controller@fe054800 { + compatible = "amlogic,a1-audio2-clkc"; + reg = <0x0 0xfe054800 0x0 0x20>; + #clock-cells = <1>; + clocks = <&clkc_periphs CLKID_AUDIO>, + <&clkc_periphs CLKID_DDS_IN>, + <&clkc_pll CLKID_FCLK_DIV2>, + <&clkc_pll CLKID_FCLK_DIV3>, + <&clkc_pll CLKID_HIFI_PLL>, + <&xtal>; + clock-names = "pclk", + "dds_in", + "fclk_div2", + "fclk_div3", + "hifi_pll", + "xtal"; + }; + }; diff --git a/include/dt-bindings/clock/amlogic,a1-audio-clkc.h b/include/dt-bindings/clock/amlogic,a1-audio-clkc.h new file mode 100644 index 000000000000..6534d1878816 --- /dev/null +++ b/include/dt-bindings/clock/amlogic,a1-audio-clkc.h @@ -0,0 +1,122 @@ +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */ +/* + * Copyright (c) 2024, SaluteDevices. All Rights Reserved. + * + * Author: Jan Dakinevich <jan.dakinevich@salutedevices.com> + */ + +#ifndef __A1_AUDIO_CLKC_BINDINGS_H +#define __A1_AUDIO_CLKC_BINDINGS_H + +#define AUD_CLKID_DDR_ARB 1 +#define AUD_CLKID_TDMIN_A 2 +#define AUD_CLKID_TDMIN_B 3 +#define AUD_CLKID_TDMIN_LB 4 +#define AUD_CLKID_LOOPBACK 5 +#define AUD_CLKID_TDMOUT_A 6 +#define AUD_CLKID_TDMOUT_B 7 +#define AUD_CLKID_FRDDR_A 8 +#define AUD_CLKID_FRDDR_B 9 +#define AUD_CLKID_TODDR_A 10 +#define AUD_CLKID_TODDR_B 11 +#define AUD_CLKID_SPDIFIN 12 +#define AUD_CLKID_RESAMPLE 13 +#define AUD_CLKID_EQDRC 14 +#define AUD_CLKID_LOCKER 15 +#define AUD_CLKID_MST_A_MCLK_SEL 16 +#define AUD_CLKID_MST_A_MCLK_DIV 17 +#define AUD_CLKID_MST_A_MCLK 18 +#define AUD_CLKID_MST_B_MCLK_SEL 19 +#define AUD_CLKID_MST_B_MCLK_DIV 20 +#define AUD_CLKID_MST_B_MCLK 21 +#define AUD_CLKID_MST_C_MCLK_SEL 22 +#define AUD_CLKID_MST_C_MCLK_DIV 23 +#define AUD_CLKID_MST_C_MCLK 24 +#define AUD_CLKID_MST_D_MCLK_SEL 25 +#define AUD_CLKID_MST_D_MCLK_DIV 26 +#define AUD_CLKID_MST_D_MCLK 27 +#define AUD_CLKID_SPDIFIN_CLK_SEL 28 +#define AUD_CLKID_SPDIFIN_CLK_DIV 29 +#define AUD_CLKID_SPDIFIN_CLK 30 +#define AUD_CLKID_RESAMPLE_CLK_SEL 31 +#define AUD_CLKID_RESAMPLE_CLK_DIV 32 +#define AUD_CLKID_RESAMPLE_CLK 33 +#define AUD_CLKID_LOCKER_IN_CLK_SEL 34 +#define AUD_CLKID_LOCKER_IN_CLK_DIV 35 +#define AUD_CLKID_LOCKER_IN_CLK 36 +#define AUD_CLKID_LOCKER_OUT_CLK_SEL 37 +#define AUD_CLKID_LOCKER_OUT_CLK_DIV 38 +#define AUD_CLKID_LOCKER_OUT_CLK 39 +#define AUD_CLKID_EQDRC_CLK_SEL 40 +#define AUD_CLKID_EQDRC_CLK_DIV 41 +#define AUD_CLKID_EQDRC_CLK 42 +#define AUD_CLKID_MST_A_SCLK_PRE_EN 43 +#define AUD_CLKID_MST_A_SCLK_DIV 44 +#define AUD_CLKID_MST_A_SCLK_POST_EN 45 +#define AUD_CLKID_MST_A_SCLK 46 +#define AUD_CLKID_MST_B_SCLK_PRE_EN 47 +#define AUD_CLKID_MST_B_SCLK_DIV 48 +#define AUD_CLKID_MST_B_SCLK_POST_EN 49 +#define AUD_CLKID_MST_B_SCLK 50 +#define AUD_CLKID_MST_C_SCLK_PRE_EN 51 +#define AUD_CLKID_MST_C_SCLK_DIV 52 +#define AUD_CLKID_MST_C_SCLK_POST_EN 53 +#define AUD_CLKID_MST_C_SCLK 54 +#define AUD_CLKID_MST_D_SCLK_PRE_EN 55 +#define AUD_CLKID_MST_D_SCLK_DIV 56 +#define AUD_CLKID_MST_D_SCLK_POST_EN 57 +#define AUD_CLKID_MST_D_SCLK 58 +#define AUD_CLKID_MST_A_LRCLK_DIV 59 +#define AUD_CLKID_MST_A_LRCLK 60 +#define AUD_CLKID_MST_B_LRCLK_DIV 61 +#define AUD_CLKID_MST_B_LRCLK 62 +#define AUD_CLKID_MST_C_LRCLK_DIV 63 +#define AUD_CLKID_MST_C_LRCLK 64 +#define AUD_CLKID_MST_D_LRCLK_DIV 65 +#define AUD_CLKID_MST_D_LRCLK 66 +#define AUD_CLKID_TDMIN_A_SCLK_SEL 67 +#define AUD_CLKID_TDMIN_A_SCLK_PRE_EN 68 +#define AUD_CLKID_TDMIN_A_SCLK_POST_EN 69 +#define AUD_CLKID_TDMIN_A_SCLK 70 +#define AUD_CLKID_TDMIN_A_LRCLK 71 +#define AUD_CLKID_TDMIN_B_SCLK_SEL 72 +#define AUD_CLKID_TDMIN_B_SCLK_PRE_EN 73 +#define AUD_CLKID_TDMIN_B_SCLK_POST_EN 74 +#define AUD_CLKID_TDMIN_B_SCLK 75 +#define AUD_CLKID_TDMIN_B_LRCLK 76 +#define AUD_CLKID_TDMIN_LB_SCLK_SEL 77 +#define AUD_CLKID_TDMIN_LB_SCLK_PRE_EN 78 +#define AUD_CLKID_TDMIN_LB_SCLK_POST_EN 79 +#define AUD_CLKID_TDMIN_LB_SCLK 80 +#define AUD_CLKID_TDMIN_LB_LRCLK 81 +#define AUD_CLKID_TDMOUT_A_SCLK_SEL 82 +#define AUD_CLKID_TDMOUT_A_SCLK_PRE_EN 83 +#define AUD_CLKID_TDMOUT_A_SCLK_POST_EN 84 +#define AUD_CLKID_TDMOUT_A_SCLK 85 +#define AUD_CLKID_TDMOUT_A_LRCLK 86 +#define AUD_CLKID_TDMOUT_B_SCLK_SEL 87 +#define AUD_CLKID_TDMOUT_B_SCLK_PRE_EN 88 +#define AUD_CLKID_TDMOUT_B_SCLK_POST_EN 89 +#define AUD_CLKID_TDMOUT_B_SCLK 90 +#define AUD_CLKID_TDMOUT_B_LRCLK 91 + +#define AUD_CLKID_VAD_DDR_ARB 1 +#define AUD_CLKID_VAD_PDM 2 +#define AUD_CLKID_VAD_TDMIN 3 +#define AUD_CLKID_VAD_TODDR 4 +#define AUD_CLKID_VAD 5 +#define AUD_CLKID_VAD_AUDIOTOP 6 +#define AUD_CLKID_VAD_MCLK_SEL 7 +#define AUD_CLKID_VAD_MCLK_DIV 8 +#define AUD_CLKID_VAD_MCLK 9 +#define AUD_CLKID_VAD_CLK_SEL 10 +#define AUD_CLKID_VAD_CLK_DIV 11 +#define AUD_CLKID_VAD_CLK 12 +#define AUD_CLKID_VAD_PDM_DCLK_SEL 13 +#define AUD_CLKID_VAD_PDM_DCLK_DIV 14 +#define AUD_CLKID_VAD_PDM_DCLK 15 +#define AUD_CLKID_VAD_PDM_SYSCLK_SEL 16 +#define AUD_CLKID_VAD_PDM_SYSCLK_DIV 17 +#define AUD_CLKID_VAD_PDM_SYSCLK 18 + +#endif /* __A1_AUDIO_CLKC_BINDINGS_H */ diff --git a/include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h b/include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h new file mode 100644 index 000000000000..653fddba1d8f --- /dev/null +++ b/include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */ +/* + * Copyright (c) 2024, SaluteDevices. All Rights Reserved. + * + * Author: Jan Dakinevich <jan.dakinevich@salutedevices.com> + */ + +#ifndef _DT_BINDINGS_AMLOGIC_MESON_A1_AUDIO_RESET_H +#define _DT_BINDINGS_AMLOGIC_MESON_A1_AUDIO_RESET_H + +#define AUD_RESET_DDRARB 0 +#define AUD_RESET_TDMIN_A 1 +#define AUD_RESET_TDMIN_B 2 +#define AUD_RESET_TDMIN_LB 3 +#define AUD_RESET_LOOPBACK 4 +#define AUD_RESET_TDMOUT_A 5 +#define AUD_RESET_TDMOUT_B 6 +#define AUD_RESET_FRDDR_A 7 +#define AUD_RESET_FRDDR_B 8 +#define AUD_RESET_TODDR_A 9 +#define AUD_RESET_TODDR_B 10 +#define AUD_RESET_SPDIFIN 11 +#define AUD_RESET_RESAMPLE 12 +#define AUD_RESET_EQDRC 13 +#define AUD_RESET_LOCKER 14 +#define AUD_RESET_TOACODEC 30 +#define AUD_RESET_CLKTREE 31 + +#endif /* _DT_BINDINGS_AMLOGIC_MESON_A1_AUDIO_RESET_H */
Add device tree bindings for A1 SoC audio clock and reset controllers. Signed-off-by: Jan Dakinevich <jan.dakinevich@salutedevices.com> --- This controller has 6 mandatory and up to 20 optional clocks. To describe this, I use 'additionalItems'. It produces correct processed-schema.json: "clock-names": { "maxItems": 26, "items": [ { "const": "pclk" }, { "const": "dds_in" }, { "const": "fclk_div2" }, { "const": "fclk_div3" }, { "const": "hifi_pll" }, { "const": "xtal" } ], "additionalItems": { "oneOf": [ { "pattern": "^slv_sclk[0-9]$" }, { "pattern": "^slv_lrclk[0-9]$" } ] }, "type": "array", "minItems": 6 }, and it behaves as expected. However, the checking is followed by complaints like this: Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' And indeed, 'additionalItems' has boolean type in meta-schema. So, how to do it right? --- .../bindings/clock/amlogic,a1-audio-clkc.yaml | 124 ++++++++++++++++++ .../dt-bindings/clock/amlogic,a1-audio-clkc.h | 122 +++++++++++++++++ .../reset/amlogic,meson-a1-audio-reset.h | 29 ++++ 3 files changed, 275 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml create mode 100644 include/dt-bindings/clock/amlogic,a1-audio-clkc.h create mode 100644 include/dt-bindings/reset/amlogic,meson-a1-audio-reset.h