| Message ID | 20201104224356.18040-3-nm@ti.com |
|---|---|
| State | Accepted |
| Commit | 5d1bedf252db3ec2becb9f43c55e0f33af1fd7fc |
| Headers | show |
| Series | [1/4] arm64: dts: ti: k3-am65*: Cleanup disabled nodes at SoC dtsi level | expand |
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
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
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 -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
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
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. -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
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 --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 {
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(-)