[PATCH 1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices

[PATCH 1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices

[Alexander Stein]

Binding specify order of clocks as:
1. "sclk"
2. "lrclk"
3. "mclk"
Adjust clocks accordingly. Fixes warnings:
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-0: clock-names:0: 'sclk' was expected
        from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-0: clock-names:1: 'lrclk' was expected
        from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-0: clock-names:2: 'mclk' was expected
        from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-1: clock-names:0: 'sclk' was expected
        from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-1: clock-names:1: 'lrclk' was expected
        from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-1: clock-names:2: 'mclk' was expected
        from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-2: clock-names:0: 'sclk' was expected
        from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-2: clock-names:1: 'lrclk' was expected
        from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-2: clock-names:2: 'mclk' was expected
        from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#

Signed-off-by: Alexander Stein <alexander.stein@mailbox.org>
---
 arch/arm64/boot/dts/amlogic/meson-g12.dtsi | 24 +++++++++++-----------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12.dtsi
index 6a1f4dcf6488..3ae6875707fb 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12.dtsi
@@ -15,10 +15,10 @@ tdmif_a: audio-controller-0 {
 		compatible = "amlogic,axg-tdm-iface";
 		#sound-dai-cells = <0>;
 		sound-name-prefix = "TDM_A";
-		clocks = <&clkc_audio AUD_CLKID_MST_A_MCLK>,
-			 <&clkc_audio AUD_CLKID_MST_A_SCLK>,
-			 <&clkc_audio AUD_CLKID_MST_A_LRCLK>;
-		clock-names = "mclk", "sclk", "lrclk";
+		clocks = <&clkc_audio AUD_CLKID_MST_A_SCLK>,
+			 <&clkc_audio AUD_CLKID_MST_A_LRCLK>,
+			 <&clkc_audio AUD_CLKID_MST_A_MCLK>;
+		clock-names = "sclk", "lrclk", "mclk";
 		status = "disabled";
 	};
 
@@ -26,10 +26,10 @@ tdmif_b: audio-controller-1 {
 		compatible = "amlogic,axg-tdm-iface";
 		#sound-dai-cells = <0>;
 		sound-name-prefix = "TDM_B";
-		clocks = <&clkc_audio AUD_CLKID_MST_B_MCLK>,
-			 <&clkc_audio AUD_CLKID_MST_B_SCLK>,
-			 <&clkc_audio AUD_CLKID_MST_B_LRCLK>;
-		clock-names = "mclk", "sclk", "lrclk";
+		clocks = <&clkc_audio AUD_CLKID_MST_B_SCLK>,
+			 <&clkc_audio AUD_CLKID_MST_B_LRCLK>,
+			 <&clkc_audio AUD_CLKID_MST_B_MCLK>;
+		clock-names = "sclk", "lrclk", "mclk";
 		status = "disabled";
 	};
 
@@ -37,10 +37,10 @@ tdmif_c: audio-controller-2 {
 		compatible = "amlogic,axg-tdm-iface";
 		#sound-dai-cells = <0>;
 		sound-name-prefix = "TDM_C";
-		clocks = <&clkc_audio AUD_CLKID_MST_C_MCLK>,
-			 <&clkc_audio AUD_CLKID_MST_C_SCLK>,
-			 <&clkc_audio AUD_CLKID_MST_C_LRCLK>;
-		clock-names = "mclk", "sclk", "lrclk";
+		clocks = <&clkc_audio AUD_CLKID_MST_C_SCLK>,
+			 <&clkc_audio AUD_CLKID_MST_C_LRCLK>,
+			 <&clkc_audio AUD_CLKID_MST_C_MCLK>;
+		clock-names = "sclk", "lrclk", "mclk";
 		status = "disabled";
 	};
 };
-- 
2.41.0

[PATCH 2/2] arm64: dts: meson-g12: Fix compatible for amlogic,g12a-tdmin

[Alexander Stein]

amlogic,axg-tdmin is not listed as compatible to g12a-tdmin in bindings.
Remove superfluous compatible. Fixes the dtbs_check warnings:
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller@300: compatible: ['amlogic,g12a-tdmin', 'amlogic,axg-tdmin'] is too long
 from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-formatters.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller@340: compatible: ['amlogic,g12a-tdmin', 'amlogic,axg-tdmin'] is too long
 from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-formatters.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller@380: compatible: ['amlogic,g12a-tdmin', 'amlogic,axg-tdmin'] is too long
 from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-formatters.yaml#
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller@3c0: compatible: ['amlogic,g12a-tdmin', 'amlogic,axg-tdmin'] is too long
 from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-formatters.yaml#

Signed-off-by: Alexander Stein <alexander.stein@mailbox.org>
---
 arch/arm64/boot/dts/amlogic/meson-g12.dtsi | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12.dtsi
index 3ae6875707fb..eb442aaf57e4 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12.dtsi
@@ -195,8 +195,7 @@ arb: reset-controller@280 {
 		};
 
 		tdmin_a: audio-controller@300 {
-			compatible = "amlogic,g12a-tdmin",
-				     "amlogic,axg-tdmin";
+			compatible = "amlogic,g12a-tdmin";
 			reg = <0x0 0x300 0x0 0x40>;
 			sound-name-prefix = "TDMIN_A";
 			resets = <&clkc_audio AUD_RESET_TDMIN_A>;
@@ -211,8 +210,7 @@ tdmin_a: audio-controller@300 {
 		};
 
 		tdmin_b: audio-controller@340 {
-			compatible = "amlogic,g12a-tdmin",
-				     "amlogic,axg-tdmin";
+			compatible = "amlogic,g12a-tdmin";
 			reg = <0x0 0x340 0x0 0x40>;
 			sound-name-prefix = "TDMIN_B";
 			resets = <&clkc_audio AUD_RESET_TDMIN_B>;
@@ -227,8 +225,7 @@ tdmin_b: audio-controller@340 {
 		};
 
 		tdmin_c: audio-controller@380 {
-			compatible = "amlogic,g12a-tdmin",
-				     "amlogic,axg-tdmin";
+			compatible = "amlogic,g12a-tdmin";
 			reg = <0x0 0x380 0x0 0x40>;
 			sound-name-prefix = "TDMIN_C";
 			resets = <&clkc_audio AUD_RESET_TDMIN_C>;
@@ -243,8 +240,7 @@ tdmin_c: audio-controller@380 {
 		};
 
 		tdmin_lb: audio-controller@3c0 {
-			compatible = "amlogic,g12a-tdmin",
-				     "amlogic,axg-tdmin";
+			compatible = "amlogic,g12a-tdmin";
 			reg = <0x0 0x3c0 0x0 0x40>;
 			sound-name-prefix = "TDMIN_LB";
 			resets = <&clkc_audio AUD_RESET_TDMIN_LB>;
-- 
2.41.0

Re: [PATCH 1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices

[Kevin Hilman]

DT maintainers,

Alexander Stein <alexander.stein@mailbox.org> writes:

> Binding specify order of clocks as:
> 1. "sclk"
> 2. "lrclk"
> 3. "mclk"
> Adjust clocks accordingly. Fixes warnings:

I understand this patch is to fix DT warnings (and thank you Alexander
for fixing warnings!) ... *but* the underlying requirement being
enforced by the schema here seems completely wrong to me, and a step
backwards.

Sorry if this is a FAQ someplace, but I couldn't find an explanation for
this.  One of the main goals of introducing names in the first place was
to get rid of ordering requirements.  Now the DT schema is enforcing
ordering requirements, but the drivers don't need ordering, so what is
the point of enforcing ordering requirements?

Kevin

Re: [PATCH 1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices

[Krzysztof Kozlowski]

On 10/08/2023 19:15, Kevin Hilman wrote:
> DT maintainers,
> 
> Alexander Stein <alexander.stein@mailbox.org> writes:
> 
>> Binding specify order of clocks as:
>> 1. "sclk"
>> 2. "lrclk"
>> 3. "mclk"
>> Adjust clocks accordingly. Fixes warnings:
> 
> I understand this patch is to fix DT warnings (and thank you Alexander
> for fixing warnings!) ... *but* the underlying requirement being
> enforced by the schema here seems completely wrong to me, and a step
> backwards.
> 
> Sorry if this is a FAQ someplace, but I couldn't find an explanation for
> this.  One of the main goals of introducing names in the first place was
> to get rid of ordering requirements.

Not entirely. The names was just a helper for cases when order is not
fixed, but even with the names for every regular case the order was
always strict. We always expect these to be ordered.

> Now the DT schema is enforcing
> ordering requirements, but the drivers don't need ordering, so what is
> the point of enforcing ordering requirements?

Because names are not everything. One OS implementation might still take
by indices, even if names are provided, so you cannot change the order.

Few bindings allow relaxed approach here, but these are written that way
to allow mixing order.

For few other bindings (e.g. newer Qualcomm clock controllers) we just
dropped the names entirely, because they bring little value and also
code for lookup by name is slower than by index.

Best regards,
Krzysztof

Re: [PATCH 1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices

[Kevin Hilman]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 10/08/2023 19:15, Kevin Hilman wrote:
>> DT maintainers,
>> 
>> Alexander Stein <alexander.stein@mailbox.org> writes:
>> 
>>> Binding specify order of clocks as:
>>> 1. "sclk"
>>> 2. "lrclk"
>>> 3. "mclk"
>>> Adjust clocks accordingly. Fixes warnings:
>> 
>> I understand this patch is to fix DT warnings (and thank you Alexander
>> for fixing warnings!) ... *but* the underlying requirement being
>> enforced by the schema here seems completely wrong to me, and a step
>> backwards.
>> 
>> Sorry if this is a FAQ someplace, but I couldn't find an explanation for
>> this.  One of the main goals of introducing names in the first place was
>> to get rid of ordering requirements.
>
> Not entirely. The names was just a helper for cases when order is not
> fixed,

Exactly, "when order is not fixed."  This is the case for lots of
hardware today (not just clocks), and is precisely what we need to
describe multiple, optional and independent (unordered) clocks.

> but even with the names for every regular case the order was
> always strict. We always expect these to be ordered.

I'm not sure who the "we" is you're referring to, but this expectation
is new to me, and honestly a bit surprising.  Before DT schema, driver &
DT writers could happily describe their hardware using names for
optional and unordered resources.

>> Now the DT schema is enforcing
>> ordering requirements, but the drivers don't need ordering, so what is
>> the point of enforcing ordering requirements?
>
> Because names are not everything.  One OS implementation might still
> take by indices, even if names are provided, so you cannot change the
> order.

Wait, isn't this an "OS-ism" imposing requirements on the DT that are
not at all about describing hardware?

Strictly ordering resources in DT that are completely independent (and
unordered!) in hardware seems to be a big step away from the general
guidance of "describe the hardware, not OS-isms".

And so far, we've only been talking about clocks, but the ordering
implications here apply to resets, pinctrl, regulators and probably
others as well.  All of these subsystems today have some way to describe
unordered & independent resources using names.  Yet, what you are
implying here applies to all of these subsystems: even where names are
used, these resources must be strictly ordered in DT.  From my
perspective, this is a new requirement.  Do you have any pointers to
where this was discussed & decided?  Admittedly, I do not follow DT
schema developments closely, but new requirements like this have
implications that I hope were discussed publically.

> Few bindings allow relaxed approach here, but these are written that way
> to allow mixing order.

Right, "mixing order" is another way of saying unordered, which is an
accurate description of lots of hardware out there.

> For few other bindings (e.g. newer Qualcomm clock controllers) we just
> dropped the names entirely, because they bring little value and also
> code for lookup by name is slower than by index.

I can see that names might bring little value if there aren't
independent optional clocks.  I can also see that making that choice for
perf reasons being a design choice, but that also a case of an OS design
choice impacting the DT, and not really about describing the hardware.

IMO, this design should be a choice of the driver writer who is most
likely to best understand the hardware.  Being forced into that strict
ordering requirement by DT schema when that is not an accurate
description of the hardware seems to be enforcing the wrong thing for
the wrong reasons.

Now that I've ranted a bit, maybe our time would be better spent if we
get practical and discuss a concrete example.  This other thread[1] has
a specific example, and ends with a specific question from Jerome about
how one should actually model a specific piece of hardware with current
DT schema.

Kevin

[1] https://lore.kernel.org/linux-amlogic/1jsf8r9v1v.fsf@starbuckisacylon.baylibre.com/

Re: [PATCH 1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices

[Krzysztof Kozlowski]

On 17/08/2023 23:53, Kevin Hilman wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
> 
>> On 10/08/2023 19:15, Kevin Hilman wrote:
>>> DT maintainers,
>>>
>>> Alexander Stein <alexander.stein@mailbox.org> writes:
>>>
>>>> Binding specify order of clocks as:
>>>> 1. "sclk"
>>>> 2. "lrclk"
>>>> 3. "mclk"
>>>> Adjust clocks accordingly. Fixes warnings:
>>>
>>> I understand this patch is to fix DT warnings (and thank you Alexander
>>> for fixing warnings!) ... *but* the underlying requirement being
>>> enforced by the schema here seems completely wrong to me, and a step
>>> backwards.
>>>
>>> Sorry if this is a FAQ someplace, but I couldn't find an explanation for
>>> this.  One of the main goals of introducing names in the first place was
>>> to get rid of ordering requirements.
>>
>> Not entirely. The names was just a helper for cases when order is not
>> fixed,
> 
> Exactly, "when order is not fixed."  This is the case for lots of
> hardware today (not just clocks), and is precisely what we need to
> describe multiple, optional and independent (unordered) clocks.

Minority of bindings require relaxed order, literally I remember just few.

> 
>> but even with the names for every regular case the order was
>> always strict. We always expect these to be ordered.
> 
> I'm not sure who the "we" is you're referring to, but this expectation

DT maintainers. Previously only Rob, now "we".

> is new to me, and honestly a bit surprising.  Before DT schema, driver &
> DT writers could happily describe their hardware using names for
> optional and unordered resources.

You talk only about Linux and not even all of its drivers (because
several took by IDs), what about all other users of bindings?

> 
>>> Now the DT schema is enforcing
>>> ordering requirements, but the drivers don't need ordering, so what is
>>> the point of enforcing ordering requirements?
>>
>> Because names are not everything.  One OS implementation might still
>> take by indices, even if names are provided, so you cannot change the
>> order.
> 
> Wait, isn't this an "OS-ism" imposing requirements on the DT that are
> not at all about describing hardware?
> 
> Strictly ordering resources in DT that are completely independent (and
> unordered!) in hardware seems to be a big step away from the general
> guidance of "describe the hardware, not OS-isms".

Bindings do not describe hardware (although this is usually what I write
in emails to simplify the discussion), but the interface between various
implementations and representation of hardware (DTS).

> 
> And so far, we've only been talking about clocks, but the ordering
> implications here apply to resets, pinctrl, regulators and probably
> others as well.  All of these subsystems today have some way to describe
> unordered & independent resources using names.  Yet, what you are
> implying here applies to all of these subsystems: even where names are
> used, these resources must be strictly ordered in DT.

Yes, we expect them since long time as strictly ordered, unless
exception is needed - which is then encoded in the bindings. Every email
from Rob since some years was saying this, recent my talks and my emails.

>  From my
> perspective, this is a new requirement.  Do you have any pointers to
> where this was discussed & decided?  Admittedly, I do not follow DT
> schema developments closely, but new requirements like this have
> implications that I hope were discussed publically.

I don't have resources, except that I recall vaguely that Rob said
original ePAPR or PowerPC was requiring it.

> 
>> Few bindings allow relaxed approach here, but these are written that way
>> to allow mixing order.
> 
> Right, "mixing order" is another way of saying unordered, which is an
> accurate description of lots of hardware out there.
> 
>> For few other bindings (e.g. newer Qualcomm clock controllers) we just
>> dropped the names entirely, because they bring little value and also
>> code for lookup by name is slower than by index.
> 
> I can see that names might bring little value if there aren't
> independent optional clocks.  I can also see that making that choice for
> perf reasons being a design choice, but that also a case of an OS design
> choice impacting the DT, and not really about describing the hardware.
> 
> IMO, this design should be a choice of the driver writer who is most
> likely to best understand the hardware.  Being forced into that strict
> ordering requirement by DT schema when that is not an accurate
> description of the hardware seems to be enforcing the wrong thing for
> the wrong reasons.

No one enforces strict order when the hardware is not like this. I wrote
it before in the quote above and again in this email.

> 
> Now that I've ranted a bit, maybe our time would be better spent if we
> get practical and discuss a concrete example.  This other thread[1] has
> a specific example, and ends with a specific question from Jerome about
> how one should actually model a specific piece of hardware with current
> DT schema.

The same as other cases. I cannot find examples, because there are
really just few real world cases like this. Maybe
qcom,sc7280-lpass-rx-macro.

But remember - people confuse clock which is not strictly required. E.g.
if UFS is not yet implemented, we can skip UFS-clock parent in some
clock-controller because UFS driver does not exist thus we do not care
about proper parenting and frequency of UFS.

Now, that UFS clock is not optional. It must be there for proper
representation of hardware. We only allow to skip it if OS drivers are
half-developed.


Best regards,
Krzysztof

Re: [PATCH 1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices

[Jerome Brunet]

On Fri 18 Aug 2023 at 10:58, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 17/08/2023 23:53, Kevin Hilman wrote:
>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
>> 
>>> On 10/08/2023 19:15, Kevin Hilman wrote:
>>>> DT maintainers,
>>>>
>>>> Alexander Stein <alexander.stein@mailbox.org> writes:
>>>>
>>>>> Binding specify order of clocks as:
>>>>> 1. "sclk"
>>>>> 2. "lrclk"
>>>>> 3. "mclk"
>>>>> Adjust clocks accordingly. Fixes warnings:
>>>>
>>>> I understand this patch is to fix DT warnings (and thank you Alexander
>>>> for fixing warnings!) ... *but* the underlying requirement being
>>>> enforced by the schema here seems completely wrong to me, and a step
>>>> backwards.
>>>>
>>>> Sorry if this is a FAQ someplace, but I couldn't find an explanation for
>>>> this.  One of the main goals of introducing names in the first place was
>>>> to get rid of ordering requirements.
>>>
>>> Not entirely. The names was just a helper for cases when order is not
>>> fixed,
>> 
>> Exactly, "when order is not fixed."  This is the case for lots of
>> hardware today (not just clocks), and is precisely what we need to
>> describe multiple, optional and independent (unordered) clocks.
>
> Minority of bindings require relaxed order, literally I remember just few.
>

*require* is the keyword here. Yes, a minority of interfaces can't be
described as ordered, mostly because they have more than 1 optional
ressource.

Still the majority out there uses names and have little care for the
order (or IDs). They are only describe has ordered because that is the
simple way to do it with 'schema'

>> 
>>> but even with the names for every regular case the order was
>>> always strict. We always expect these to be ordered.
>> 
>> I'm not sure who the "we" is you're referring to, but this expectation
>
> DT maintainers. Previously only Rob, now "we".

AFAIK, kernel maintainers enforces the rule of the linux kernel.
These rules are part of the kernel itself, so they can be discussed and
clarified if needed, like checkpatch.pl or the code of conduct.

It does not seems to be the case here and, maybe, it is partly the problem.

>
>> is new to me, and honestly a bit surprising.  Before DT schema, driver &
>> DT writers could happily describe their hardware using names for
>> optional and unordered resources.
>
> You talk only about Linux and not even all of its drivers (because
> several took by IDs), what about all other users of bindings?

Do those drivers which take IDs specify the 'clock-names' as a required
property ? If that's the case, shouldn't we question that ?

What is the point of having a required property if it is purely
decorative ?

The property 'clock-names' is required with bindings discussed here.
I assume that if an interface describes a required property, users of
the interface will have to pay attention to it

No one is questionning that the order is strict if "-names' is not part
of interface or optionnal (that last one would be weird)

>
>> 
>>>> Now the DT schema is enforcing
>>>> ordering requirements, but the drivers don't need ordering, so what is
>>>> the point of enforcing ordering requirements?
>>>
>>> Because names are not everything.  One OS implementation might still
>>> take by indices, even if names are provided, so you cannot change the
>>> order.
>> 
>> Wait, isn't this an "OS-ism" imposing requirements on the DT that are
>> not at all about describing hardware?
>> 
>> Strictly ordering resources in DT that are completely independent (and
>> unordered!) in hardware seems to be a big step away from the general
>> guidance of "describe the hardware, not OS-isms".
>
> Bindings do not describe hardware (although this is usually what I write
> in emails to simplify the discussion), but the interface between various
> implementations and representation of hardware (DTS).
>
>> 
>> And so far, we've only been talking about clocks, but the ordering
>> implications here apply to resets, pinctrl, regulators and probably
>> others as well.  All of these subsystems today have some way to describe
>> unordered & independent resources using names.  Yet, what you are
>> implying here applies to all of these subsystems: even where names are
>> used, these resources must be strictly ordered in DT.
>
> Yes, we expect them since long time as strictly ordered, unless
> exception is needed - which is then encoded in the bindings. Every email
> from Rob since some years was saying this, recent my talks and my emails.
>
>>  From my
>> perspective, this is a new requirement.  Do you have any pointers to
>> where this was discussed & decided?  Admittedly, I do not follow DT
>> schema developments closely, but new requirements like this have
>> implications that I hope were discussed publically.
>
> I don't have resources, except that I recall vaguely that Rob said
> original ePAPR or PowerPC was requiring it.
>
>> 
>>> Few bindings allow relaxed approach here, but these are written that way
>>> to allow mixing order.
>> 
>> Right, "mixing order" is another way of saying unordered, which is an
>> accurate description of lots of hardware out there.
>> 
>>> For few other bindings (e.g. newer Qualcomm clock controllers) we just
>>> dropped the names entirely, because they bring little value and also
>>> code for lookup by name is slower than by index.
>> 
>> I can see that names might bring little value if there aren't
>> independent optional clocks.  I can also see that making that choice for
>> perf reasons being a design choice, but that also a case of an OS design
>> choice impacting the DT, and not really about describing the hardware.
>> 
>> IMO, this design should be a choice of the driver writer who is most
>> likely to best understand the hardware.  Being forced into that strict
>> ordering requirement by DT schema when that is not an accurate
>> description of the hardware seems to be enforcing the wrong thing for
>> the wrong reasons.
>
> No one enforces strict order when the hardware is not like this. I wrote
> it before in the quote above and again in this email.

Which HW is actually 'like this', ordered among ressources ?
In every schematics and datasheets I have, things get named. Order is
not even a topic.

Most 'schema' have been described like this because it is easy and
natural to just list your ressources. I did not realize when schema was
introduced that order mattered. I'm pretty sure I'm not alone there.

>
>> 
>> Now that I've ranted a bit, maybe our time would be better spent if we
>> get practical and discuss a concrete example.  This other thread[1] has
>> a specific example, and ends with a specific question from Jerome about
>> how one should actually model a specific piece of hardware with current
>> DT schema.
>
> The same as other cases. I cannot find examples, because there are
> really just few real world cases like this. Maybe
> qcom,sc7280-lpass-rx-macro.
>

Ok, I'll go with 'oneOf'. We'll see how it goes

> But remember - people confuse clock which is not strictly required. E.g.
> if UFS is not yet implemented, we can skip UFS-clock parent in some
> clock-controller because UFS driver does not exist thus we do not care
> about proper parenting and frequency of UFS.
>
> Now, that UFS clock is not optional. It must be there for proper
> representation of hardware. We only allow to skip it if OS drivers are
> half-developed.
>
>
> Best regards,
> Krzysztof

Re: [PATCH 1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices

[Jerome Brunet]

>
>>> 
>>>> but even with the names for every regular case the order was
>>>> always strict. We always expect these to be ordered.
>>> 
>>> I'm not sure who the "we" is you're referring to, but this expectation
>>
>> DT maintainers. Previously only Rob, now "we".
>
> AFAIK, kernel maintainers enforces the rule of the linux kernel.
> These rules are part of the kernel itself, so they can be discussed and
> clarified if needed, like checkpatch.pl or the code of conduct.
>
> It does not seems to be the case here and, maybe, it is partly the problem.
>

I take that back, you have written the requirement and stated that
pattern name matching is discouraged in 2020.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=73f76a41c4ed7def5dc2ec7c33c7e9f94e601a20

Though the reason why this became a requirement and why name matching is
discouraged is not entirely clear, nor that it is well known in the
community considering how heavily it is still used.

Re: [PATCH 1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices

[Krzysztof Kozlowski]

On 18/08/2023 13:39, Jerome Brunet wrote:
> 
> On Fri 18 Aug 2023 at 10:58, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 17/08/2023 23:53, Kevin Hilman wrote:
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
>>>
>>>> On 10/08/2023 19:15, Kevin Hilman wrote:
>>>>> DT maintainers,
>>>>>
>>>>> Alexander Stein <alexander.stein@mailbox.org> writes:
>>>>>
>>>>>> Binding specify order of clocks as:
>>>>>> 1. "sclk"
>>>>>> 2. "lrclk"
>>>>>> 3. "mclk"
>>>>>> Adjust clocks accordingly. Fixes warnings:
>>>>>
>>>>> I understand this patch is to fix DT warnings (and thank you Alexander
>>>>> for fixing warnings!) ... *but* the underlying requirement being
>>>>> enforced by the schema here seems completely wrong to me, and a step
>>>>> backwards.
>>>>>
>>>>> Sorry if this is a FAQ someplace, but I couldn't find an explanation for
>>>>> this.  One of the main goals of introducing names in the first place was
>>>>> to get rid of ordering requirements.
>>>>
>>>> Not entirely. The names was just a helper for cases when order is not
>>>> fixed,
>>>
>>> Exactly, "when order is not fixed."  This is the case for lots of
>>> hardware today (not just clocks), and is precisely what we need to
>>> describe multiple, optional and independent (unordered) clocks.
>>
>> Minority of bindings require relaxed order, literally I remember just few.
>>
> 
> *require* is the keyword here. Yes, a minority of interfaces can't be
> described as ordered, mostly because they have more than 1 optional
> ressource.
> 
> Still the majority out there uses names and have little care for the
> order (or IDs). They are only describe has ordered because that is the
> simple way to do it with 'schema'
> 
>>>
>>>> but even with the names for every regular case the order was
>>>> always strict. We always expect these to be ordered.
>>>
>>> I'm not sure who the "we" is you're referring to, but this expectation
>>
>> DT maintainers. Previously only Rob, now "we".
> 
> AFAIK, kernel maintainers enforces the rule of the linux kernel.
> These rules are part of the kernel itself, so they can be discussed and
> clarified if needed, like checkpatch.pl or the code of conduct.
> 
> It does not seems to be the case here and, maybe, it is partly the problem.
> 
>>
>>> is new to me, and honestly a bit surprising.  Before DT schema, driver &
>>> DT writers could happily describe their hardware using names for
>>> optional and unordered resources.
>>
>> You talk only about Linux and not even all of its drivers (because
>> several took by IDs), what about all other users of bindings?
> 
> Do those drivers which take IDs specify the 'clock-names' as a required
> property ? If that's the case, shouldn't we question that ?
> 
> What is the point of having a required property if it is purely
> decorative ?

No, it is not. I am repeating myself, but okay. In such particular case,
the interface is for different users and we allow one user to use names
and other to use indices. We do not force users (so OS/firmware) to
implement both of ways. The user can choose, because that's what binding
is saying. Since however binding is saying that order and clock-names
matter, we however force DTS to implement both to satisfy users.

Other bindings can do differently and I brought the example of Qualcomm
where we removed the names. Problem? No.

> 
> The property 'clock-names' is required with bindings discussed here.
> I assume that if an interface describes a required property, users of
> the interface will have to pay attention to it
> 
> No one is questionning that the order is strict if "-names' is not part
> of interface or optionnal (that last one would be weird)
> 
>>
>>>
>>>>> Now the DT schema is enforcing
>>>>> ordering requirements, but the drivers don't need ordering, so what is
>>>>> the point of enforcing ordering requirements?
>>>>
>>>> Because names are not everything.  One OS implementation might still
>>>> take by indices, even if names are provided, so you cannot change the
>>>> order.
>>>
>>> Wait, isn't this an "OS-ism" imposing requirements on the DT that are
>>> not at all about describing hardware?
>>>
>>> Strictly ordering resources in DT that are completely independent (and
>>> unordered!) in hardware seems to be a big step away from the general
>>> guidance of "describe the hardware, not OS-isms".
>>
>> Bindings do not describe hardware (although this is usually what I write
>> in emails to simplify the discussion), but the interface between various
>> implementations and representation of hardware (DTS).
>>
>>>
>>> And so far, we've only been talking about clocks, but the ordering
>>> implications here apply to resets, pinctrl, regulators and probably
>>> others as well.  All of these subsystems today have some way to describe
>>> unordered & independent resources using names.  Yet, what you are
>>> implying here applies to all of these subsystems: even where names are
>>> used, these resources must be strictly ordered in DT.
>>
>> Yes, we expect them since long time as strictly ordered, unless
>> exception is needed - which is then encoded in the bindings. Every email
>> from Rob since some years was saying this, recent my talks and my emails.
>>
>>>  From my
>>> perspective, this is a new requirement.  Do you have any pointers to
>>> where this was discussed & decided?  Admittedly, I do not follow DT
>>> schema developments closely, but new requirements like this have
>>> implications that I hope were discussed publically.
>>
>> I don't have resources, except that I recall vaguely that Rob said
>> original ePAPR or PowerPC was requiring it.
>>
>>>
>>>> Few bindings allow relaxed approach here, but these are written that way
>>>> to allow mixing order.
>>>
>>> Right, "mixing order" is another way of saying unordered, which is an
>>> accurate description of lots of hardware out there.
>>>
>>>> For few other bindings (e.g. newer Qualcomm clock controllers) we just
>>>> dropped the names entirely, because they bring little value and also
>>>> code for lookup by name is slower than by index.
>>>
>>> I can see that names might bring little value if there aren't
>>> independent optional clocks.  I can also see that making that choice for
>>> perf reasons being a design choice, but that also a case of an OS design
>>> choice impacting the DT, and not really about describing the hardware.
>>>
>>> IMO, this design should be a choice of the driver writer who is most
>>> likely to best understand the hardware.  Being forced into that strict
>>> ordering requirement by DT schema when that is not an accurate
>>> description of the hardware seems to be enforcing the wrong thing for
>>> the wrong reasons.
>>
>> No one enforces strict order when the hardware is not like this. I wrote
>> it before in the quote above and again in this email.
> 
> Which HW is actually 'like this', ordered among ressources ?
> In every schematics and datasheets I have, things get named. Order is
> not even a topic.

Exactly, but the topic for hardware is which clocks are required and
which are not.

Now, the actual language of bindings and DTS have limitations. Just like
everything, C as well. I wrote that bindings do not describe the
hardware (in contrary to I usually write in responses trying to simplify
it), but the interface, thus the rules of the interface apply.

Devices which have truly optional clocks, can have optional clocks.
What's the problem here?

Devices, which people don't care, or, for which people try to sneak
optional clocks, even though they are not optional, should use fixed
clocks because that's how the interface works between different systems.
clock-names is just a helper.

I don't understand what is the problem for your device. You (or the
developers) did not have to require clock-names. No one of DT team asked
for this, I think. If you implemented that way, you are bound by the
interface. If you also implemented strictly ordered list, you are bound
by that requirement as well. If you had implemented it other way, you
would be bound by other way. It is as simple as that.

> 
> Most 'schema' have been described like this because it is easy and
> natural to just list your ressources. I did not realize when schema was
> introduced that order mattered. I'm pretty sure I'm not alone there.

DT schema was always requiring it, since day zero, because this is how
it worked. Do you want to say that you implemented strict list in the
binding, then use different order in DTS and the validation passed thus
you were happy? I doubt. Your validation could not pass and this should
tell you - oh, schema is requiring the order, either I accept it or
change something somewhere.

Best regards,
Krzysztof

Re: [PATCH 1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices

[Neil Armstrong]

Hi,

On Tue, 08 Aug 2023 18:17:52 +0200, Alexander Stein wrote:
> Binding specify order of clocks as:
> 1. "sclk"
> 2. "lrclk"
> 3. "mclk"
> Adjust clocks accordingly. Fixes warnings:
> arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-0: clock-names:0: 'sclk' was expected
>         from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
> arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-0: clock-names:1: 'lrclk' was expected
>         from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
> arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-0: clock-names:2: 'mclk' was expected
>         from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
> arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-1: clock-names:0: 'sclk' was expected
>         from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
> arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-1: clock-names:1: 'lrclk' was expected
>         from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
> arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-1: clock-names:2: 'mclk' was expected
>         from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
> arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-2: clock-names:0: 'sclk' was expected
>         from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
> arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-2: clock-names:1: 'lrclk' was expected
>         from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
> arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtb: audio-controller-2: clock-names:2: 'mclk' was expected
>         from schema $id: http://devicetree.org/schemas/sound/amlogic,axg-tdm-iface.yaml#
> 
> [...]

Thanks, Applied to https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git (v6.7/arm64-dt)

[1/2] arm64: dts: meson-g12: Fix clock order for amlogic,axg-tdm-iface devices
      https://git.kernel.org/amlogic/c/c92997482e70c67ce7b9b32344fe85c4b0cb701d
[2/2] arm64: dts: meson-g12: Fix compatible for amlogic,g12a-tdmin
      https://git.kernel.org/amlogic/c/a42b8f639298c57f119c6d5f55a0c0c87259096a

These changes has been applied on the intermediate git tree [1].

The v6.7/arm64-dt branch will then be sent via a formal Pull Request to the Linux SoC maintainers
for inclusion in their intermediate git branches in order to be sent to Linus during
the next merge window, or sooner if it's a set of fixes.

In the cases of fixes, those will be merged in the current release candidate
kernel and as soon they appear on the Linux master branch they will be
backported to the previous Stable and Long-Stable kernels [2].

The intermediate git branches are merged daily in the linux-next tree [3],
people are encouraged testing these pre-release kernels and report issues on the
relevant mailing-lists.

If problems are discovered on those changes, please submit a signed-off-by revert
patch followed by a corrective changeset.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git
[2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git
[3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git

-- 
Neil