diff mbox series

[2/4] arm64: dts: ti: k3-j721e*: Cleanup disabled nodes at SoC dtsi level

Message ID 20201104224356.18040-3-nm@ti.com (mailing list archive)
State New, archived
Headers show
Series arm64: dts: ti: Cleanup mix of "okay" and "disabled" usage | expand

Commit Message

Nishanth Menon Nov. 4, 2020, 10:43 p.m. UTC
The device tree standard sets the default node behavior when status
property as enabled. There are many reasons for doing the same, number
of strings in device tree, default power management functionality etc
are few of the reasons.

In general, after a few rounds of discussions [1] there are few
options one could take when dealing with SoC dtsi and board dts

a. SoC dtsi provide nodes as a super-set default (aka enabled) state and
   to prevent messy board files, when more boards are added per SoC, we
   optimize and disable commonly un-used nodes in board-common.dtsi
b. SoC dtsi disables all hardware dependent nodes by default and board
   dts files enable nodes based on a need basis.
c. Subjectively pick and choose which nodes we will disable by default
   in SoC dtsi and over the years we can optimize things and change
   default state depending on the need.

While there are pros and cons on each of these approaches, the right
thing to do will be to stick with device tree default standards and
work within those established rules. So, we choose to go with option
(a).

Lets cleanup defaults of j721e SoC dtsi before this gets more harder
to cleanup later on and new SoCs are added.

The only functional difference between the dtb generated is
status='okay' is no longer necessary for mcasp10 and depends on the
default state.

[1] https://lore.kernel.org/linux-arm-kernel/20201027130701.GE5639@atomide.com/

Fixes: 1c4d35265fb2 ("arm64: dts: ti: k3-j721e-main: Add McASP nodes")
Fixes: 76921f15acc0 ("arm64: dts: ti: k3-j721e-main: Add DSS node")
Cc: Jyri Sarha <jsarha@ti.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
 .../dts/ti/k3-j721e-common-proc-board.dts     | 48 ++++++++++++++++++-
 arch/arm64/boot/dts/ti/k3-j721e-main.dtsi     | 26 ----------
 2 files changed, 47 insertions(+), 27 deletions(-)

Comments

Tomi Valkeinen Nov. 5, 2020, 7:25 a.m. UTC | #1
On 05/11/2020 00:43, Nishanth Menon wrote:
> The device tree standard sets the default node behavior when status
> property as enabled. There are many reasons for doing the same, number
> of strings in device tree, default power management functionality etc
> are few of the reasons.
> 
> In general, after a few rounds of discussions [1] there are few
> options one could take when dealing with SoC dtsi and board dts
> 
> a. SoC dtsi provide nodes as a super-set default (aka enabled) state and
>    to prevent messy board files, when more boards are added per SoC, we
>    optimize and disable commonly un-used nodes in board-common.dtsi
> b. SoC dtsi disables all hardware dependent nodes by default and board
>    dts files enable nodes based on a need basis.
> c. Subjectively pick and choose which nodes we will disable by default
>    in SoC dtsi and over the years we can optimize things and change
>    default state depending on the need.
> 
> While there are pros and cons on each of these approaches, the right
> thing to do will be to stick with device tree default standards and
> work within those established rules. So, we choose to go with option
> (a).
> 
> Lets cleanup defaults of j721e SoC dtsi before this gets more harder
> to cleanup later on and new SoCs are added.
> 
> The only functional difference between the dtb generated is
> status='okay' is no longer necessary for mcasp10 and depends on the
> default state.
> 
> [1] https://lore.kernel.org/linux-arm-kernel/20201027130701.GE5639@atomide.com/
> 
> Fixes: 1c4d35265fb2 ("arm64: dts: ti: k3-j721e-main: Add McASP nodes")
> Fixes: 76921f15acc0 ("arm64: dts: ti: k3-j721e-main: Add DSS node")
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  .../dts/ti/k3-j721e-common-proc-board.dts     | 48 ++++++++++++++++++-
>  arch/arm64/boot/dts/ti/k3-j721e-main.dtsi     | 26 ----------
>  2 files changed, 47 insertions(+), 27 deletions(-)
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi
Peter Ujfalusi Nov. 5, 2020, 7:32 a.m. UTC | #2
Nishanth,

On 05/11/2020 0.43, Nishanth Menon wrote:
> The device tree standard sets the default node behavior when status
> property as enabled.

It should be:
When the status property is not present under a node, the "okay' value
is assumed.

Note: the device tree specification does not document default value as
such, see v0.3 (2.3.4, page 14).
Yes, the "okay" is used in case the status property is missing (by Linux
at least).

> There are many reasons for doing the same, number
> of strings in device tree,

with expense of loc and readability.

> default power management functionality etc

Right, so how does that helps with devices present in the SoC, but no
node at all? First thing which comes to mind is AASRC, we don't have
Linux driver for it (and no DT binding document), but that does not mean
that it is not present. How PM would take that into account?

> are few of the reasons.
> 
> In general, after a few rounds of discussions [1] there are few
> options one could take when dealing with SoC dtsi and board dts
> 
> a. SoC dtsi provide nodes as a super-set default (aka enabled) state and
>    to prevent messy board files, when more boards are added per SoC, we
>    optimize and disable commonly un-used nodes in board-common.dtsi
> b. SoC dtsi disables all hardware dependent nodes by default and board
>    dts files enable nodes based on a need basis.
> c. Subjectively pick and choose which nodes we will disable by default
>    in SoC dtsi and over the years we can optimize things and change
>    default state depending on the need.

For the record: c was not really an option. There were no subjectivity,
the reason was pragmatic.

We are all familiar with the Devicetree specification, but let me quote
from chapter 2.3.4:
"okay"
Indicates the device is operational.

"disabled"
Indicates that the device is not presently operational, but it might
become operational in the future (for example, something is not plugged
in, or switched off).
Refer to the device binding for details on what disabled means for a
given device.

The reason why we kept McASP nodes (and dss) disabled in the soc dtsi
file is that they are not operation in the form they present in there.
They _need_ additional properties to be operational and those properties
can only be added in the board dts file.

This is not remotely a subjective view, this is the opposite of
subjectivity.

As for things not owned by the OS we have the "reserved" status.

> While there are pros and cons on each of these approaches, the right
> thing to do will be to stick with device tree default standards and
> work within those established rules. So, we choose to go with option
> (a).
> 
> Lets cleanup defaults of j721e SoC dtsi before this gets more harder
> to cleanup later on and new SoCs are added.
> 
> The only functional difference between the dtb generated is
> status='okay' is no longer necessary for mcasp10 and depends on the
> default state.
> 
> [1] https://lore.kernel.org/linux-arm-kernel/20201027130701.GE5639@atomide.com/
> 
> Fixes: 1c4d35265fb2 ("arm64: dts: ti: k3-j721e-main: Add McASP nodes")
> Fixes: 76921f15acc0 ("arm64: dts: ti: k3-j721e-main: Add DSS node")
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  .../dts/ti/k3-j721e-common-proc-board.dts     | 48 ++++++++++++++++++-
>  arch/arm64/boot/dts/ti/k3-j721e-main.dtsi     | 26 ----------
>  2 files changed, 47 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> index 52e121155563..9416528caa8a 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
> @@ -540,6 +540,46 @@ &dss {
>  				 <&k3_clks 152 18>;	/* PLL23_HSDIV0 */
>  };
>  
> +&mcasp0 {
> +	status = "disabled";
> +};
> +
> +&mcasp1 {
> +	status = "disabled";
> +};
> +
> +&mcasp2 {
> +	status = "disabled";
> +};
> +
> +&mcasp3 {
> +	status = "disabled";
> +};
> +
> +&mcasp4 {
> +	status = "disabled";
> +};
> +
> +&mcasp5 {
> +	status = "disabled";
> +};
> +
> +&mcasp6 {
> +	status = "disabled";
> +};
> +
> +&mcasp7 {
> +	status = "disabled";
> +};
> +
> +&mcasp8 {
> +	status = "disabled";
> +};
> +
> +&mcasp9 {
> +	status = "disabled";
> +};
> +
>  &mcasp10 {
>  	#sound-dai-cells = <0>;
>  
> @@ -556,8 +596,10 @@ &mcasp10 {
>  	>;
>  	tx-num-evt = <0>;
>  	rx-num-evt = <0>;
> +};
>  
> -	status = "okay";
> +&mcasp11 {
> +	status = "disabled";
>  };

Looks much better in this way.
?

I always wondered what is _not_ used by the board...
But it is not really about that, we need to disable these nodes as they
are incomplete in dtsi, they are not operational...

>  &serdes0 {
> @@ -639,3 +681,7 @@ &pcie3_rc {
>  &pcie3_ep {
>  	status = "disabled";
>  };
> +
> +&dss {
> +	status = "disabled";
> +};
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> index e2a96b2c423c..b54332d6fdc5 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
> @@ -1327,8 +1327,6 @@ dss: dss@04a00000 {
>  				  "common_s1",
>  				  "common_s2";
>  
> -		status = "disabled";
> -
>  		dss_ports: ports {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> @@ -1350,8 +1348,6 @@ mcasp0: mcasp@2b00000 {
>  		clocks = <&k3_clks 174 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 174 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp1: mcasp@2b10000 {
> @@ -1369,8 +1365,6 @@ mcasp1: mcasp@2b10000 {
>  		clocks = <&k3_clks 175 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 175 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp2: mcasp@2b20000 {
> @@ -1388,8 +1382,6 @@ mcasp2: mcasp@2b20000 {
>  		clocks = <&k3_clks 176 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 176 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp3: mcasp@2b30000 {
> @@ -1407,8 +1399,6 @@ mcasp3: mcasp@2b30000 {
>  		clocks = <&k3_clks 177 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 177 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp4: mcasp@2b40000 {
> @@ -1426,8 +1416,6 @@ mcasp4: mcasp@2b40000 {
>  		clocks = <&k3_clks 178 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 178 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp5: mcasp@2b50000 {
> @@ -1445,8 +1433,6 @@ mcasp5: mcasp@2b50000 {
>  		clocks = <&k3_clks 179 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 179 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp6: mcasp@2b60000 {
> @@ -1464,8 +1450,6 @@ mcasp6: mcasp@2b60000 {
>  		clocks = <&k3_clks 180 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 180 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp7: mcasp@2b70000 {
> @@ -1483,8 +1467,6 @@ mcasp7: mcasp@2b70000 {
>  		clocks = <&k3_clks 181 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 181 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp8: mcasp@2b80000 {
> @@ -1502,8 +1484,6 @@ mcasp8: mcasp@2b80000 {
>  		clocks = <&k3_clks 182 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 182 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp9: mcasp@2b90000 {
> @@ -1521,8 +1501,6 @@ mcasp9: mcasp@2b90000 {
>  		clocks = <&k3_clks 183 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 183 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp10: mcasp@2ba0000 {
> @@ -1540,8 +1518,6 @@ mcasp10: mcasp@2ba0000 {
>  		clocks = <&k3_clks 184 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 184 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	mcasp11: mcasp@2bb0000 {
> @@ -1559,8 +1535,6 @@ mcasp11: mcasp@2bb0000 {
>  		clocks = <&k3_clks 185 1>;
>  		clock-names = "fck";
>  		power-domains = <&k3_pds 185 TI_SCI_PD_EXCLUSIVE>;
> -
> -		status = "disabled";
>  	};
>  
>  	watchdog0: watchdog@2200000 {
> 

There is no such a tag, but:
whatever-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Nishanth Menon Nov. 5, 2020, 2:08 p.m. UTC | #3
On 09:32-20201105, Peter Ujfalusi wrote:
> Nishanth,
> 
> On 05/11/2020 0.43, Nishanth Menon wrote:
> > The device tree standard sets the default node behavior when status
> > property as enabled.
> 
> It should be:
> When the status property is not present under a node, the "okay' value
> is assumed.

Thanks.. will update.

> 
> Note: the device tree specification does not document default value as
> such, see v0.3 (2.3.4, page 14).
> Yes, the "okay" is used in case the status property is missing (by Linux
> at least).

Maybe the spec update needs a formal release? Kumar's patch is merged:
https://github.com/devicetree-org/devicetree-specification/pull/33

on that exact same section, which you can see
https://github.com/devicetree-org/devicetree-specification/blob/master/source/chapter2-devicetree-basics.rst

Brings it to sync to:
https://elinux.org/Device_Tree_Linux#status_property

> 
> > There are many reasons for doing the same, number
> > of strings in device tree,
> 
> with expense of loc and readability.

The "readability" part is subjective a bit.. enabled and disabled both
have verbosity problem lets see how we can optimize as new boards come
in.

> 
> > default power management functionality etc
> 
> Right, so how does that helps with devices present in the SoC, but no
> node at all? First thing which comes to mind is AASRC, we don't have
> Linux driver for it (and no DT binding document), but that does not mean
> that it is not present. How PM would take that into account?

I think we are mixing topics here -> I was stating the motivation why
devicetree chose such as default. Do we have a suggestion to improve
the description in the commit?

> 
> > are few of the reasons.
> > 
> > In general, after a few rounds of discussions [1] there are few
> > options one could take when dealing with SoC dtsi and board dts
> > 
> > a. SoC dtsi provide nodes as a super-set default (aka enabled) state and
> >    to prevent messy board files, when more boards are added per SoC, we
> >    optimize and disable commonly un-used nodes in board-common.dtsi
> > b. SoC dtsi disables all hardware dependent nodes by default and board
> >    dts files enable nodes based on a need basis.
> > c. Subjectively pick and choose which nodes we will disable by default
> >    in SoC dtsi and over the years we can optimize things and change
> >    default state depending on the need.
> 
> For the record: c was not really an option. There were no subjectivity,
> the reason was pragmatic.


(c) some examples where we did pick that option (fixes):
https://lore.kernel.org/linux-arm-kernel/20201104224356.18040-4-nm@ti.com/
https://lore.kernel.org/linux-arm-kernel/20201104224356.18040-5-nm@ti.com/

> 
> The reason why we kept McASP nodes (and dss) disabled in the soc dtsi
> file is that they are not operation in the form they present in there.
> They _need_ additional properties to be operational and those properties
> can only be added in the board dts file.

I dont think we are changing anything in the output dtb files, we are
just leaving the defaults as dt defaults and set the disable state in
board dts OR common board dtsi.

> 
> This is not remotely a subjective view, this is the opposite of
> subjectivity.

the usage of McASP was'nt meant as (c).. it is (b). is there a better way
to describe this in a generic manner?

> 
> As for things not owned by the OS we have the "reserved" status.
Which is correct usage. I think your point with wkup_uart should be set as
reserved? I might have missed doing that - am I correct?

[...]
> >  
> > -	status = "okay";
> > +&mcasp11 {
> > +	status = "disabled";
> >  };
> 
> Looks much better in this way.
> ?
> 
> I always wondered what is _not_ used by the board...
> But it is not really about that, we need to disable these nodes as they
> are incomplete in dtsi, they are not operational...

Alright - what do we suggest we do?


Tony, Rob - I need some guidance here.

> 
> >  &serdes0 {
	[...]
> >  
> >  	watchdog0: watchdog@2200000 {
> > 
> 
> There is no such a tag, but:
> whatever-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

OK - I have no idea how B4 or patchworks pick that one as :D
Peter Ujfalusi Nov. 6, 2020, 11:32 a.m. UTC | #4
Nishanth,

On 05/11/2020 16.08, Nishanth Menon wrote:
> On 09:32-20201105, Peter Ujfalusi wrote:
>> Nishanth,
>>
>> On 05/11/2020 0.43, Nishanth Menon wrote:
>>> The device tree standard sets the default node behavior when status
>>> property as enabled.
>>
>> It should be:
>> When the status property is not present under a node, the "okay' value
>> is assumed.
> 
> Thanks.. will update.
> 
>>
>> Note: the device tree specification does not document default value as
>> such, see v0.3 (2.3.4, page 14).
>> Yes, the "okay" is used in case the status property is missing (by Linux
>> at least).
> 
> Maybe the spec update needs a formal release? Kumar's patch is merged:
> https://github.com/devicetree-org/devicetree-specification/pull/33
> 
> on that exact same section, which you can see
> https://github.com/devicetree-org/devicetree-specification/blob/master/source/chapter2-devicetree-basics.rst

I stand correct, I only checked the released version.

> Brings it to sync to:
> https://elinux.org/Device_Tree_Linux#status_property
> 
>>
>>> There are many reasons for doing the same, number
>>> of strings in device tree,
>>
>> with expense of loc and readability.
> 
> The "readability" part is subjective a bit.. enabled and disabled both
> have verbosity problem lets see how we can optimize as new boards come
> in.

I agree.

> 
>>
>>> default power management functionality etc
>>
>> Right, so how does that helps with devices present in the SoC, but no
>> node at all? First thing which comes to mind is AASRC, we don't have
>> Linux driver for it (and no DT binding document), but that does not mean
>> that it is not present. How PM would take that into account?
> 
> I think we are mixing topics here -> I was stating the motivation why
> devicetree chose such as default.

I don't question the fact that 'okay' is the default status if it is not
explicitly present. There is no better default than that.

> Do we have a suggestion to improve
> the description in the commit?

A bit later on that.

>>
>>> are few of the reasons.
>>>
>>> In general, after a few rounds of discussions [1] there are few
>>> options one could take when dealing with SoC dtsi and board dts
>>>
>>> a. SoC dtsi provide nodes as a super-set default (aka enabled) state and
>>>    to prevent messy board files, when more boards are added per SoC, we
>>>    optimize and disable commonly un-used nodes in board-common.dtsi
>>> b. SoC dtsi disables all hardware dependent nodes by default and board
>>>    dts files enable nodes based on a need basis.
>>> c. Subjectively pick and choose which nodes we will disable by default
>>>    in SoC dtsi and over the years we can optimize things and change
>>>    default state depending on the need.
>>
>> For the record: c was not really an option. There were no subjectivity,
>> the reason was pragmatic.
> 
> 
> (c) some examples where we did pick that option (fixes):
> https://lore.kernel.org/linux-arm-kernel/20201104224356.18040-4-nm@ti.com/
> https://lore.kernel.org/linux-arm-kernel/20201104224356.18040-5-nm@ti.com/

this is different, these patches just removing the "status = 'okay';"
lines where they are not needed and can be omitted to save few lines and
it does help on readablity.

>> The reason why we kept McASP nodes (and dss) disabled in the soc dtsi
>> file is that they are not operation in the form they present in there.
>> They _need_ additional properties to be operational and those properties
>> can only be added in the board dts file.
> 
> I dont think we are changing anything in the output dtb files,

Correct, the resulted dtb is identical. If the developer for upcoming
boards did check the schematics vs TRM vs dtsi and spot the things that
is not configured.
dtb check will complain when it is starting to check against the
documentation, but McASP is not yet converted to yaml and to be honest I
don't want to convert the current binding to be the binding. When it was
done it just moved pdata variables to DT and that was wrong.
This is off-topic a bit.

> we are
> just leaving the defaults as dt defaults and set the disable state in
> board dts OR common board dtsi.

Yes, we leave the non working/configured node 'okay' in dtsi and expect
that the board file author will know which node must be disabled because
it is incomplete.

>> This is not remotely a subjective view, this is the opposite of
>> subjectivity.
> 
> the usage of McASP was'nt meant as (c).. it is (b). is there a better way
> to describe this in a generic manner?

I had my saying on that ever since I have been taking care of audio on
TI SoCs ;)

I used similar analogy in a private thread around this, but imho it fits
the case neatly:
car == McASP

you don't put an 'okay' (as is ready, operational) stamp on the car in
the middle of the production line when the engine is not even installed.

>> As for things not owned by the OS we have the "reserved" status.
> Which is correct usage. I think your point with wkup_uart should be set as
> reserved? I might have missed doing that - am I correct?
> 
> [...]
>>>  
>>> -	status = "okay";
>>> +&mcasp11 {
>>> +	status = "disabled";
>>>  };
>>
>> Looks much better in this way.
>> ?
>>
>> I always wondered what is _not_ used by the board...
>> But it is not really about that, we need to disable these nodes as they
>> are incomplete in dtsi, they are not operational...
> 
> Alright - what do we suggest we do?

Not sure, I'm 'whatever' after [1] makes it to mainline or next.

> Tony, Rob - I need some guidance here.

I'm fine whatever way we take, but I think it is up to you to make the
call as the maintainer of the TI dts files... ;)

>>
>>>  &serdes0 {
> 	[...]
>>>  
>>>  	watchdog0: watchdog@2200000 {
>>>
>>
>> There is no such a tag, but:
>> whatever-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> OK - I have no idea how B4 or patchworks pick that one as :D

If we take this road, than I'm okay with it, but I'm going to take
silent protest (not sending acked-by or revired-by).
That should not stop you doing what you believe is best for the future!

fwiw, McASP will have sane handling for the variations of 'okay':
[1]
https://lore.kernel.org/alsa-devel/20201106072551.689-1-peter.ujfalusi@ti.com/

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Nishanth Menon Nov. 6, 2020, 9:46 p.m. UTC | #5
On 13:32-20201106, Peter Ujfalusi wrote:
[...]
> > 
> >>
> >>> default power management functionality etc
> >>
> >> Right, so how does that helps with devices present in the SoC, but no
> >> node at all? First thing which comes to mind is AASRC, we don't have
> >> Linux driver for it (and no DT binding document), but that does not mean
> >> that it is not present. How PM would take that into account?
> > 
> > I think we are mixing topics here -> I was stating the motivation why
> > devicetree chose such as default.
> 
> I don't question the fact that 'okay' is the default status if it is not
> explicitly present. There is no better default than that.

^^ -> Alright, that is all we are trying to do here: defaults in the
SoC.dtsi and specific cleanups (firmware reserved / board unused
disables) be done in a common board.dtsi (for now, there is no such
specific need, I guess).

[..]

> 
> >> The reason why we kept McASP nodes (and dss) disabled in the soc dtsi
> >> file is that they are not operation in the form they present in there.
> >> They _need_ additional properties to be operational and those properties
> >> can only be added in the board dts file.
> > 
> > I dont think we are changing anything in the output dtb files,
> 
> Correct, the resulted dtb is identical. If the developer for upcoming
> boards did check the schematics vs TRM vs dtsi and spot the things that
> is not configured.

Yes.

> 
> > we are
> > just leaving the defaults as dt defaults and set the disable state in
> > board dts OR common board dtsi.
> 
> Yes, we leave the non working/configured node 'okay' in dtsi and expect
> that the board file author will know which node must be disabled because
> it is incomplete.

Yes - I understand(and empathise) the implicit omission error risk we
are incurring here. I will add that to the commit message as well.

> >> This is not remotely a subjective view, this is the opposite of
> >> subjectivity.
> > 
> > the usage of McASP was'nt meant as (c).. it is (b). is there a better way
> > to describe this in a generic manner?
> 
> I had my saying on that ever since I have been taking care of audio on
> TI SoCs ;)
> 
> I used similar analogy in a private thread around this, but imho it fits
> the case neatly:
> car == McASP
> 
> you don't put an 'okay' (as is ready, operational) stamp on the car in
> the middle of the production line when the engine is not even installed.

Completely agree with you. we are just insisting that this be done in
either common board.dtsi OR board.dtsi where applicable.

> 

[..]

> > Alright - what do we suggest we do?
> 
> Not sure, I'm 'whatever' after [1] makes it to mainline or next.
[....]
> [1]
> https://lore.kernel.org/alsa-devel/20201106072551.689-1-peter.ujfalusi@ti.com/


I don't see the relationship between the series.. I think this series
brings no change in dtb, hence with OR without your driver cleanup
series, there is no practical regressions.
> 
> > Tony, Rob - I need some guidance here.
> 
> I'm fine whatever way we take, but I think it is up to you to make the
> call as the maintainer of the TI dts files... ;)

Yep - I have'nt seen a reason yet that must cause us to change from the
Device tree default approach in our debates.


As Tony already pointed out.. if we start seeing a lot more boards for
an SoC..
Instead of (reverse issue- where we have a lot of places where people are
doing "okay" problem):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=12afc0cf81210969756daecd7eb48b307f08faed

We should look at ways to consolidate in a common-board.dtsi

> 
> >>
> >>>  &serdes0 {
> > 	[...]
> >>>  
> >>>  	watchdog0: watchdog@2200000 {
> >>>
> >>
> >> There is no such a tag, but:
> >> whatever-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > 
> > OK - I have no idea how B4 or patchworks pick that one as :D
> 
> If we take this road, than I'm okay with it, but I'm going to take
> silent protest (not sending acked-by or revired-by).
> That should not stop you doing what you believe is best for the future!

OK - thanks for your review and the discussions, always appreciate
getting our views out there.

if there are no other comments, I will try and post a v2 over the
weekend.
Peter Ujfalusi Nov. 9, 2020, 7:49 a.m. UTC | #6
On 06/11/2020 23.46, Nishanth Menon wrote:
> On 13:32-20201106, Peter Ujfalusi wrote:
> [...]
>>>
>>>>
>>>>> default power management functionality etc
>>>>
>>>> Right, so how does that helps with devices present in the SoC, but no
>>>> node at all? First thing which comes to mind is AASRC, we don't have
>>>> Linux driver for it (and no DT binding document), but that does not mean
>>>> that it is not present. How PM would take that into account?
>>>
>>> I think we are mixing topics here -> I was stating the motivation why
>>> devicetree chose such as default.
>>
>> I don't question the fact that 'okay' is the default status if it is not
>> explicitly present. There is no better default than that.
> 
> ^^ -> Alright, that is all we are trying to do here: defaults in the
> SoC.dtsi and specific cleanups (firmware reserved / board unused
> disables) be done in a common board.dtsi (for now, there is no such
> specific need, I guess).

The default is what it is: default choice which suits most of the nodes.

If the node is not complete in it's present form then it is not in it's
default state. imho.

>>> Alright - what do we suggest we do?
>>
>> Not sure, I'm 'whatever' after [1] makes it to mainline or next.
> [....]
>> [1]
>> https://lore.kernel.org/alsa-devel/20201106072551.689-1-peter.ujfalusi@ti.com/
> 
> 
> I don't see the relationship between the series.. I think this series
> brings no change in dtb, hence with OR without your driver cleanup
> series, there is no practical regressions.

This series opens up the possibility of nodes leaking to dtb with known
broken state and the driver should have a better strategy than 'works by
luck' to handle it ;)

>>
>>> Tony, Rob - I need some guidance here.
>>
>> I'm fine whatever way we take, but I think it is up to you to make the
>> call as the maintainer of the TI dts files... ;)
> 
> Yep - I have'nt seen a reason yet that must cause us to change from the
> Device tree default approach in our debates.

Imho 'disabled' is the default for nodes like McASP as it is:
"Indicates that the device is not presently operational, but it might
become operational in the future" (for example, needed properties added
to the node).

>>>> There is no such a tag, but:
>>>> whatever-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>>
>>> OK - I have no idea how B4 or patchworks pick that one as :D
>>
>> If we take this road, than I'm okay with it, but I'm going to take
>> silent protest (not sending acked-by or revired-by).
>> That should not stop you doing what you believe is best for the future!
> 
> OK - thanks for your review and the discussions, always appreciate
> getting our views out there.
> 
> if there are no other comments, I will try and post a v2 over the
> weekend.

OK

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
index 52e121155563..9416528caa8a 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
@@ -540,6 +540,46 @@  &dss {
 				 <&k3_clks 152 18>;	/* PLL23_HSDIV0 */
 };
 
+&mcasp0 {
+	status = "disabled";
+};
+
+&mcasp1 {
+	status = "disabled";
+};
+
+&mcasp2 {
+	status = "disabled";
+};
+
+&mcasp3 {
+	status = "disabled";
+};
+
+&mcasp4 {
+	status = "disabled";
+};
+
+&mcasp5 {
+	status = "disabled";
+};
+
+&mcasp6 {
+	status = "disabled";
+};
+
+&mcasp7 {
+	status = "disabled";
+};
+
+&mcasp8 {
+	status = "disabled";
+};
+
+&mcasp9 {
+	status = "disabled";
+};
+
 &mcasp10 {
 	#sound-dai-cells = <0>;
 
@@ -556,8 +596,10 @@  &mcasp10 {
 	>;
 	tx-num-evt = <0>;
 	rx-num-evt = <0>;
+};
 
-	status = "okay";
+&mcasp11 {
+	status = "disabled";
 };
 
 &serdes0 {
@@ -639,3 +681,7 @@  &pcie3_rc {
 &pcie3_ep {
 	status = "disabled";
 };
+
+&dss {
+	status = "disabled";
+};
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
index e2a96b2c423c..b54332d6fdc5 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
@@ -1327,8 +1327,6 @@  dss: dss@04a00000 {
 				  "common_s1",
 				  "common_s2";
 
-		status = "disabled";
-
 		dss_ports: ports {
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -1350,8 +1348,6 @@  mcasp0: mcasp@2b00000 {
 		clocks = <&k3_clks 174 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 174 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp1: mcasp@2b10000 {
@@ -1369,8 +1365,6 @@  mcasp1: mcasp@2b10000 {
 		clocks = <&k3_clks 175 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 175 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp2: mcasp@2b20000 {
@@ -1388,8 +1382,6 @@  mcasp2: mcasp@2b20000 {
 		clocks = <&k3_clks 176 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 176 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp3: mcasp@2b30000 {
@@ -1407,8 +1399,6 @@  mcasp3: mcasp@2b30000 {
 		clocks = <&k3_clks 177 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 177 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp4: mcasp@2b40000 {
@@ -1426,8 +1416,6 @@  mcasp4: mcasp@2b40000 {
 		clocks = <&k3_clks 178 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 178 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp5: mcasp@2b50000 {
@@ -1445,8 +1433,6 @@  mcasp5: mcasp@2b50000 {
 		clocks = <&k3_clks 179 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 179 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp6: mcasp@2b60000 {
@@ -1464,8 +1450,6 @@  mcasp6: mcasp@2b60000 {
 		clocks = <&k3_clks 180 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 180 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp7: mcasp@2b70000 {
@@ -1483,8 +1467,6 @@  mcasp7: mcasp@2b70000 {
 		clocks = <&k3_clks 181 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 181 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp8: mcasp@2b80000 {
@@ -1502,8 +1484,6 @@  mcasp8: mcasp@2b80000 {
 		clocks = <&k3_clks 182 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 182 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp9: mcasp@2b90000 {
@@ -1521,8 +1501,6 @@  mcasp9: mcasp@2b90000 {
 		clocks = <&k3_clks 183 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 183 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp10: mcasp@2ba0000 {
@@ -1540,8 +1518,6 @@  mcasp10: mcasp@2ba0000 {
 		clocks = <&k3_clks 184 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 184 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	mcasp11: mcasp@2bb0000 {
@@ -1559,8 +1535,6 @@  mcasp11: mcasp@2bb0000 {
 		clocks = <&k3_clks 185 1>;
 		clock-names = "fck";
 		power-domains = <&k3_pds 185 TI_SCI_PD_EXCLUSIVE>;
-
-		status = "disabled";
 	};
 
 	watchdog0: watchdog@2200000 {