Hi,
This is hopefully a conclusion of the thread we had (online[1] and
offline). 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.
What we have today is a bit of a mix of seemingly random set of
choices, however, predominantly following (a) and a few intermittent
cases of (b) and (c). While there are pros and cons on each of these
approaches, the right thing to do will be to stick with device tree
default (aka device tree standards) and work within those established
rules. So, lets cleanup and follow what the vast majority of SoC
platforms are doing and which also happens to be the path of least churn
for TI dts nodes as well.
Functionally the dtb output is same ->
a) As of v5.10-rc1:
am654: https://pastebin.ubuntu.com/p/G4P5vghpV3/
j7200: https://pastebin.ubuntu.com/p/SsG6JtPzR9/
j721e: https://pastebin.ubuntu.com/p/HWXmTwD6m8/
b) with this series applied:
am654: https://pastebin.ubuntu.com/p/h7MmHPQpRx/
j7200: https://pastebin.ubuntu.com/p/VXjQHhQNgn/
j721e: https://pastebin.ubuntu.com/p/2JMgftd4Xx/
The actual diff between the two versions being: https://pastebin.ubuntu.com/p/4rwy5qRY84/
Which is equivalent as per device tree standards, but uses lesser
redundant strings.
Thanks Tony, for sticking to the guns and providing us clear guidance
on this topic.
[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
Nishanth Menon (4):
arm64: dts: ti: k3-am65*: Cleanup disabled nodes at SoC dtsi level
arm64: dts: ti: k3-j721e*: Cleanup disabled nodes at SoC dtsi level
arm64: dts: ti: am65/j721e: Fix up un-necessary status set to "okay"
for crypto
arm64: dts: ti: k3-am654-base-board: Fix up un-necessary status set to
"okay" for USB
arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 9 ----
.../arm64/boot/dts/ti/k3-am654-base-board.dts | 24 ++++++----
.../dts/ti/k3-j721e-common-proc-board.dts | 48 ++++++++++++++++++-
arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 28 -----------
4 files changed, 63 insertions(+), 46 deletions(-)
--
2.29.2
The default state of a device tree node is "okay". There is no specific
use of explicitly adding status = "okay" in the SoC dtsi.
Fixes: 8ebcaaae8017 ("arm64: dts: ti: k3-j721e-main: Add crypto accelerator node")
Fixes: b366b2409c97 ("arm64: dts: ti: k3-am6: Add crypto accelarator node")
Cc: Keerthy <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
---
arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 1 -
arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 2 --
2 files changed, 3 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
index 21e50021dd83..2bd66a9e4b1d 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
@@ -119,7 +119,6 @@ crypto: crypto@4e00000 {
#address-cells = <2>;
#size-cells = <2>;
ranges = <0x0 0x04e00000 0x00 0x04e00000 0x0 0x30000>;
- status = "okay";
dmas = <&main_udmap 0xc000>, <&main_udmap 0x4000>,
<&main_udmap 0x4001>;
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
index b54332d6fdc5..9747c387385b 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
@@ -345,8 +345,6 @@ main_crypto: crypto@4e00000 {
#size-cells = <2>;
ranges = <0x0 0x04e00000 0x00 0x04e00000 0x0 0x30000>;
- status = "okay";
-
dmas = <&main_udmap 0xc000>, <&main_udmap 0x4000>,
<&main_udmap 0x4001>;
dma-names = "tx", "rx1", "rx2";
--
2.29.2
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/[email protected]/
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 <[email protected]>
Cc: Tomi Valkeinen <[email protected]>
Cc: Peter Ujfalusi <[email protected]>
Cc: Tony Lindgren <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
---
.../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";
};
&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 {
--
2.29.2
The default state of a device tree node is "okay". There is no specific
use of explicitly adding status = "okay" in the board dts.
Fixes: 7e7e7dd51d06 ("arm64: dts: ti: k3-am654-base-board: enable USB1")
Cc: Roger Quadros <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
---
arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 8 --------
1 file changed, 8 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
index 199c4d4e7539..744efcbb4f7f 100644
--- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
@@ -325,14 +325,6 @@ &sdhci1 {
disable-wp;
};
-&dwc3_1 {
- status = "okay";
-};
-
-&usb1_phy {
- status = "okay";
-};
-
&usb1 {
pinctrl-names = "default";
pinctrl-0 = <&usb1_pins_default>;
--
2.29.2
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 am654 SoC dtsi before this gets more harder
to cleanup later on and new SoCs are added.
The dtb generated is identical with the patch and it is just cleanup to
ensure we have a clean usage model
[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
Fixes: 9bcb631e9953 ("arm64: dts: ti: k3-am654-main: Add McASP nodes")
Fixes: fc539b90eda2 ("arm64: dts: ti: am654: Add DSS node")
Cc: Jyri Sarha <[email protected]>
Cc: Tomi Valkeinen <[email protected]>
Cc: Peter Ujfalusi <[email protected]>
Cc: Tony Lindgren <[email protected]>
Signed-off-by: Nishanth Menon <[email protected]>
---
arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 8 --------
arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 16 ++++++++++++++++
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
index 533525229a8d..21e50021dd83 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
@@ -770,8 +770,6 @@ mcasp0: mcasp@2b00000 {
clocks = <&k3_clks 104 0>;
clock-names = "fck";
power-domains = <&k3_pds 104 TI_SCI_PD_EXCLUSIVE>;
-
- status = "disabled";
};
mcasp1: mcasp@2b10000 {
@@ -789,8 +787,6 @@ mcasp1: mcasp@2b10000 {
clocks = <&k3_clks 105 0>;
clock-names = "fck";
power-domains = <&k3_pds 105 TI_SCI_PD_EXCLUSIVE>;
-
- status = "disabled";
};
mcasp2: mcasp@2b20000 {
@@ -808,8 +804,6 @@ mcasp2: mcasp@2b20000 {
clocks = <&k3_clks 106 0>;
clock-names = "fck";
power-domains = <&k3_pds 106 TI_SCI_PD_EXCLUSIVE>;
-
- status = "disabled";
};
cal: cal@6f03000 {
@@ -865,8 +859,6 @@ dss: dss@04a00000 {
interrupts = <GIC_SPI 166 IRQ_TYPE_EDGE_RISING>;
- status = "disabled";
-
dss_ports: ports {
#address-cells = <1>;
#size-cells = <0>;
diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
index d12dd89f3405..199c4d4e7539 100644
--- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
@@ -486,3 +486,19 @@ &cpsw_port1 {
phy-mode = "rgmii-rxid";
phy-handle = <&phy0>;
};
+
+&mcasp0 {
+ status = "disabled";
+};
+
+&mcasp1 {
+ status = "disabled";
+};
+
+&mcasp2 {
+ status = "disabled";
+};
+
+&dss {
+ status = "disabled";
+};
--
2.29.2
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 am654 SoC dtsi before this gets more harder
> to cleanup later on and new SoCs are added.
>
> The dtb generated is identical with the patch and it is just cleanup to
> ensure we have a clean usage model
>
> [1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> Fixes: 9bcb631e9953 ("arm64: dts: ti: k3-am654-main: Add McASP nodes")
> Fixes: fc539b90eda2 ("arm64: dts: ti: am654: Add DSS node")
> Cc: Jyri Sarha <[email protected]>
> Cc: Tomi Valkeinen <[email protected]>
> Cc: Peter Ujfalusi <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 8 --------
> arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 16 ++++++++++++++++
> 2 files changed, 16 insertions(+), 8 deletions(-)
Reviewed-by: Tomi Valkeinen <[email protected]>
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
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/[email protected]/
>
> 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 <[email protected]>
> Cc: Tomi Valkeinen <[email protected]>
> Cc: Peter Ujfalusi <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> .../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 <[email protected]>
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
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/[email protected]/
>
> 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 <[email protected]>
> Cc: Tomi Valkeinen <[email protected]>
> Cc: Peter Ujfalusi <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Signed-off-by: Nishanth Menon <[email protected]>
> ---
> .../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 <[email protected]>
- 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/[email protected]/
https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> 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 <[email protected]>
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/[email protected]/
> https://lore.kernel.org/linux-arm-kernel/[email protected]/
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 <[email protected]>
>
> 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/[email protected]/
- 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/[email protected]/
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 <[email protected]>
> >
> > 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/[email protected]/
>
>
> 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 <[email protected]>
>>>
>>> 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