diff mbox series

[RFC,v3,4/6] dt-bindings: clock: meson: document A1 SoC audio clock controller driver

Message ID 20240419125812.983409-5-jan.dakinevich@salutedevices.com (mailing list archive)
State New, archived
Headers show
Series Add A1 Soc audio clock controller driver | expand

Commit Message

Jan Dakinevich April 19, 2024, 12:58 p.m. UTC
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

Comments

Krzysztof Kozlowski April 19, 2024, 2:06 p.m. UTC | #1
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
Rob Herring April 19, 2024, 4:44 p.m. UTC | #2
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.
Rob Herring April 19, 2024, 9:09 p.m. UTC | #3
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
Jan Dakinevich April 20, 2024, 2:48 p.m. UTC | #4
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
>
Jan Dakinevich April 20, 2024, 4:15 p.m. UTC | #5
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
Krzysztof Kozlowski April 21, 2024, 2:02 p.m. UTC | #6
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
Jan Dakinevich April 21, 2024, 3:35 p.m. UTC | #7
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
>
Krzysztof Kozlowski April 21, 2024, 6:14 p.m. UTC | #8
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
Krzysztof Kozlowski April 21, 2024, 6:17 p.m. UTC | #9
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
Jan Dakinevich April 21, 2024, 9:03 p.m. UTC | #10
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
>
Jerome Brunet April 22, 2024, 7:16 a.m. UTC | #11
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.
Jerome Brunet April 22, 2024, 7:40 a.m. UTC | #12
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 */
Jerome Brunet April 22, 2024, 7:43 a.m. UTC | #13
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
>>
Jerome Brunet April 22, 2024, 7:57 a.m. UTC | #14
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.
Jan Dakinevich April 22, 2024, 2:31 p.m. UTC | #15
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.
> 
>
Jerome Brunet April 22, 2024, 3:38 p.m. UTC | #16
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.
>> 
>>
Jan Dakinevich April 22, 2024, 3:43 p.m. UTC | #17
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 mbox series

Patch

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 */