by Jan Dakinevich

[permalink] [raw]

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

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 <[email protected]> wrote:
>>
>>> On Sun 21 Apr 2024 at 20:14, Krzysztof Kozlowski <[email protected]> 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 <[email protected]>
>>>>>>> ---
>>>>>>>
>>>>>>> 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/[email protected]/
>>>
>>> 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.
>>
>>
>

--
Best regards
Jan Dakinevich

2024-04-22 16:00:24

by Jerome Brunet

[permalink] [raw]

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

On Mon 22 Apr 2024 at 17:31, Jan Dakinevich <[email protected]> wrote:

> On 4/22/24 10:57, Jerome Brunet wrote:
>>
>> On Mon 22 Apr 2024 at 09:16, Jerome Brunet <[email protected]> wrote:
>>
>>> On Sun 21 Apr 2024 at 20:14, Krzysztof Kozlowski <[email protected]> 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 <[email protected]>
>>>>>>> ---
>>>>>>>
>>>>>>> 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/[email protected]/
>>>
>>> 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.
>>
>>

--
Jerome