Message-ID: <37b4b284-0da5-c602-82a2-2b672f89891f@ti.com>
Date:   Fri, 6 Nov 2020 13:32:51 +0200
From:   Peter Ujfalusi <peter.ujfalusi@...com>
To:     Nishanth Menon <nm@...com>
CC:     Roger Quadros <rogerq@...com>, Keerthy <j-keerthy@...com>,
        Jyri Sarha <jsarha@...com>,
        Tomi Valkeinen <tomi.valkeinen@...com>,
        Lokesh Vutla <lokeshvutla@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Tony Lindgren <tony@...mide.com>,
        Tero Kristo <t-kristo@...com>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/4] arm64: dts: ti: k3-j721e*: Cleanup disabled nodes at
 SoC dtsi level

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@...0000 {
>>>
>>
>> There is no such a tag, but:
>> whatever-by: Peter Ujfalusi <peter.ujfalusi@...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

Powered by blists - more mailing lists