Skip to contentSkip to conversation

ARM: dts: imx: Pass 'chosen' and 'memory' nodes

PATCH New Fabio Estevam 8 participants 17 messages Jan 19, 2017 – Feb 7, 2017

Subsystems (2)
Device tree bindings Arm i.MX and Layerscape
lore
Share & tools
lore

Fabio Estevam

author
#0 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Commit 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi") causes boot issues when the bootloader does not create a 'chosen' node if such node is not present in the dtb.

The reason for the boot failure is well explained by Javier Martinez
Canillas: "the decompressor relies on a pre-existing chosen node to be available to insert the command line and merge other ATAGS info."

, so pass an empty 'chosen' node to fix the boot problem.

This issue has been seen in the kernelci reports with Barebox as bootloader.

Also pass the 'memory' node in order to fix boot issues on the SolidRun iMX6 platforms.

Fixes 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi")
Reported-by kernelci.org bot <[email protected]>
Reported-by Russell King <[email protected]>
Signed-off-by Fabio Estevam <[email protected]>

---
Changes since v1:
- Also pass the memory node.

 arch/arm/boot/dts/imx1.dtsi    | 2 ++
 arch/arm/boot/dts/imx23.dtsi   | 2 ++
 arch/arm/boot/dts/imx25.dtsi   | 2 ++
 arch/arm/boot/dts/imx27.dtsi   | 2 ++
 arch/arm/boot/dts/imx28.dtsi   | 2 ++
 arch/arm/boot/dts/imx31.dtsi   | 2 ++
 arch/arm/boot/dts/imx35.dtsi   | 2 ++
 arch/arm/boot/dts/imx50.dtsi   | 2 ++
 arch/arm/boot/dts/imx51.dtsi   | 2 ++
 arch/arm/boot/dts/imx53.dtsi   | 2 ++
 arch/arm/boot/dts/imx6qdl.dtsi | 2 ++
 arch/arm/boot/dts/imx6sl.dtsi  | 2 ++
 arch/arm/boot/dts/imx6sx.dtsi  | 2 ++
 arch/arm/boot/dts/imx6ul.dtsi  | 2 ++
 arch/arm/boot/dts/imx7s.dtsi   | 2 ++
 15 files changed, 30 insertions(+)
arch/arm/boot/dts/imx1.dtsi +2 -0
index dd3de38..7ef0d36 100644
@@ -18,6 +18,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		gpio0 = &gpio1;
arch/arm/boot/dts/imx23.dtsi +2 -0
index 96eae64..7bb662c 100644
@@ -16,6 +16,8 @@
 	#size-cells = <1>;
 
 	interrupt-parent = <&icoll>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		gpio0 = &gpio0;
arch/arm/boot/dts/imx25.dtsi +2 -0
index 213d86e..f328ecc 100644
@@ -14,6 +14,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		ethernet0 = &fec;
arch/arm/boot/dts/imx27.dtsi +2 -0
index 6a7cb9e..3271ec49 100644
@@ -19,6 +19,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		ethernet0 = &fec;
arch/arm/boot/dts/imx28.dtsi +2 -0
index 905bdb5..5f47558 100644
@@ -17,6 +17,8 @@
 	#size-cells = <1>;
 
 	interrupt-parent = <&icoll>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		ethernet0 = &mac0;
arch/arm/boot/dts/imx31.dtsi +2 -0
index c0a5d5f..afd4106 100644
@@ -12,6 +12,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		serial0 = &uart1;
arch/arm/boot/dts/imx35.dtsi +2 -0
index 6f7b943..e08e574 100644
@@ -13,6 +13,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		ethernet0 = &fec;
arch/arm/boot/dts/imx50.dtsi +2 -0
index fe0221e..46329ff 100644
@@ -17,6 +17,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		ethernet0 = &fec;
arch/arm/boot/dts/imx51.dtsi +2 -0
index 33526ca..c6ffc7b 100644
@@ -19,6 +19,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		ethernet0 = &fec;
arch/arm/boot/dts/imx53.dtsi +2 -0
index ca51dc0..2d56f26 100644
@@ -19,6 +19,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		ethernet0 = &fec;
arch/arm/boot/dts/imx6qdl.dtsi +2 -0
index 89b834f..c88a373 100644
@@ -16,6 +16,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		ethernet0 = &fec;
arch/arm/boot/dts/imx6sl.dtsi +2 -0
index 19cbd87..89e8315 100644
@@ -14,6 +14,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		ethernet0 = &fec;
arch/arm/boot/dts/imx6sx.dtsi +2 -0
index 10f3330..f52d017 100644
@@ -15,6 +15,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		can0 = &flexcan1;
arch/arm/boot/dts/imx6ul.dtsi +2 -0
index 0f69a3f..1d93601 100644
@@ -15,6 +15,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		ethernet0 = &fec1;
arch/arm/boot/dts/imx7s.dtsi +2 -0
index 8db1eb9..1757789 100644
@@ -50,6 +50,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };
 
 	aliases {
 		gpio0 = &gpio1;
Signature · 1 line
2.7.4
#1 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Quoted 46 lines · Fabio Estevam

Commit 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi") causes boot issues when the bootloader does not create a 'chosen' node if such node is not present in the dtb.

The reason for the boot failure is well explained by Javier Martinez
Canillas: "the decompressor relies on a pre-existing chosen node to be available to insert the command line and merge other ATAGS info."

, so pass an empty 'chosen' node to fix the boot problem.

This issue has been seen in the kernelci reports with Barebox as bootloader.

Also pass the 'memory' node in order to fix boot issues on the SolidRun iMX6 platforms.

Fixes 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi")
Reported-by kernelci.org bot <[email protected]>
Reported-by Russell King <[email protected]>
Signed-off-by Fabio Estevam <[email protected]>

---
Changes since v1:
- Also pass the memory node.

 arch/arm/boot/dts/imx1.dtsi    | 2 ++
 arch/arm/boot/dts/imx23.dtsi   | 2 ++
 arch/arm/boot/dts/imx25.dtsi   | 2 ++
 arch/arm/boot/dts/imx27.dtsi   | 2 ++
 arch/arm/boot/dts/imx28.dtsi   | 2 ++
 arch/arm/boot/dts/imx31.dtsi   | 2 ++
 arch/arm/boot/dts/imx35.dtsi   | 2 ++
 arch/arm/boot/dts/imx50.dtsi   | 2 ++
 arch/arm/boot/dts/imx51.dtsi   | 2 ++
 arch/arm/boot/dts/imx53.dtsi   | 2 ++
 arch/arm/boot/dts/imx6qdl.dtsi | 2 ++
 arch/arm/boot/dts/imx6sl.dtsi  | 2 ++
 arch/arm/boot/dts/imx6sx.dtsi  | 2 ++
 arch/arm/boot/dts/imx6ul.dtsi  | 2 ++
 arch/arm/boot/dts/imx7s.dtsi   | 2 ++
 15 files changed, 30 insertions(+)
arch/arm/boot/dts/imx1.dtsi +0 -0
index dd3de38..7ef0d36 100644

arch/arm/boot/dts/imx1.dtsi line 22

Quoted 5 lines
arch/arm/boot/dts/imx1.dtsi +2 -0
@@ -18,6 +18,8 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
+	memory { device_type = "memory"; reg = <0 0>; };

Would it be nice to add a comment about why this was added? Something to prevent a cleanup like "remove empty nodes and invalid memory configurations".

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
reviewer
#2 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Hi Uwe,

Quoted 3 lines · Uwe Kleine-K?nig

Would it be nice to add a comment about why this was added? Something to prevent a cleanup like "remove empty nodes and invalid memory configurations".

Do you mean something like this?

/* "chosen" and "memory" nodes are mandatory */
chosen {};
memory { device_type = "memory"; reg = <0 0>; };

#3 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Quoted 9 lines · Fabio Estevam

Hi Uwe,

Quoted 3 lines · Uwe Kleine-K?nig

Would it be nice to add a comment about why this was added? Something to prevent a cleanup like "remove empty nodes and invalid memory configurations".

Quoted 5 lines

Do you mean something like this?

/* "chosen" and "memory" nodes are mandatory */
chosen {};
memory { device_type = "memory"; reg = <0 0>; };

Not very helpful comment. Something like:

/*
* The decompressor relies on a pre-existing chosen node to be
* available to insert the command line and merge other ATAGS
* info.
*/

Is it difficult to fix the decompressor?

I didn't understood the breakage regarding the memory node good enough to suggest a comment for that.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#4 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Quoted 18 lines · Uwe Kleine-K?nig
Quoted 9 lines · Fabio Estevam

Hi Uwe,

Quoted 3 lines · Uwe Kleine-K?nig

Would it be nice to add a comment about why this was added? Something to prevent a cleanup like "remove empty nodes and invalid memory configurations".

Quoted 5 lines

Do you mean something like this?

/* "chosen" and "memory" nodes are mandatory */
chosen {};
memory { device_type = "memory"; reg = <0 0>; };

Not very helpful comment. Something like:

Quoted 7 lines

/*
* The decompressor relies on a pre-existing chosen node to be
* available to insert the command line and merge other ATAGS
* info.
*/

Is it difficult to fix the decompressor?

... and that comment would be wrong. Yes, the decompressor relies on it, as do some uboot versions.

Quoted 2 lines

I didn't understood the breakage regarding the memory node good enough to suggest a comment for that.

A missing memory node appears to prevent some uboot versions supplying any kind of memory layout to the kernel, which then causes the kernel to crash very early during boot.

Again, this is not using appended DTB - this is using a separately loaded DTB in uboot. uboot fails to update the dtb if these nodes are missing.

Frankly, I think the original change (removing the skeleton.dtsi include) was misguided and needs to be reverted - the change is imho built upon an incorrect assumption that nothing in skeleton.dtsi is required. That's clearly false.

Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
reviewer
#5 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Quoted 4 lines · Russell King - ARM Linux

Frankly, I think the original change (removing the skeleton.dtsi include) was misguided and needs to be reverted - the change is imho built upon an incorrect assumption that nothing in skeleton.dtsi is required. That's clearly false.

Wouldn't this v2 patch restore things back to normal?

#6 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

In reply to Russell King - ARM Linux
Quoted 25 lines · Russell King - ARM Linux
Quoted 24 lines · Uwe Kleine-K?nig
Quoted 9 lines · Fabio Estevam

Hi Uwe,

Quoted 3 lines · Uwe Kleine-K?nig

Would it be nice to add a comment about why this was added? Something to prevent a cleanup like "remove empty nodes and invalid memory configurations".

Quoted 5 lines

Do you mean something like this?

/* "chosen" and "memory" nodes are mandatory */
chosen {};
memory { device_type = "memory"; reg = <0 0>; };

Not very helpful comment. Something like:

/*
* The decompressor relies on a pre-existing chosen node to be
* available to insert the command line and merge other ATAGS
* info.
*/

Quoted 4 lines
Quoted 1 line

Is it difficult to fix the decompressor?

... and that comment would be wrong. Yes, the decompressor relies on it, as do some uboot versions.

Good that we talked about this, otherwise I would have removed that once the decompressor is fixed. So the comment must be

/*
* The decompressor and also some versions of U-Boot rely on a
* pre-existing /chosen node to be available to insert the
* command line and merge other ATAGS info.
* Also for U-Boot there must be a pre-existing /memory node. */

Quoted 7 lines
Quoted 2 lines

I didn't understood the breakage regarding the memory node good enough to suggest a comment for that.

A missing memory node appears to prevent some uboot versions supplying any kind of memory layout to the kernel, which then causes the kernel to crash very early during boot.

Quoted 7 lines

Again, this is not using appended DTB - this is using a separately loaded DTB in uboot. uboot fails to update the dtb if these nodes are missing.

Frankly, I think the original change (removing the skeleton.dtsi include) was misguided and needs to be reverted - the change is imho built upon an incorrect assumption that nothing in skeleton.dtsi is required. That's clearly false.

I tend to agree as I assume it's not only U-Boot on i.MX but on all ARM platforms.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#7 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Quoted 56 lines · Uwe Kleine-K?nig
Quoted 25 lines · Russell King - ARM Linux
Quoted 24 lines · Uwe Kleine-K?nig
Quoted 9 lines · Fabio Estevam

Hi Uwe,

Quoted 3 lines · Uwe Kleine-K?nig

Would it be nice to add a comment about why this was added? Something to prevent a cleanup like "remove empty nodes and invalid memory configurations".

Quoted 5 lines

Do you mean something like this?

/* "chosen" and "memory" nodes are mandatory */
chosen {};
memory { device_type = "memory"; reg = <0 0>; };

Not very helpful comment. Something like:

/*
* The decompressor relies on a pre-existing chosen node to be
* available to insert the command line and merge other ATAGS
* info.
*/

Quoted 4 lines
Quoted 1 line

Is it difficult to fix the decompressor?

... and that comment would be wrong. Yes, the decompressor relies on it, as do some uboot versions.

Good that we talked about this, otherwise I would have removed that once the decompressor is fixed. So the comment must be

/*
* The decompressor and also some versions of U-Boot rely on a
* pre-existing /chosen node to be available to insert the
* command line and merge other ATAGS info.
* Also for U-Boot there must be a pre-existing /memory node. */

Quoted 7 lines
Quoted 2 lines

I didn't understood the breakage regarding the memory node good enough to suggest a comment for that.

A missing memory node appears to prevent some uboot versions supplying any kind of memory layout to the kernel, which then causes the kernel to crash very early during boot.

Quoted 7 lines

Again, this is not using appended DTB - this is using a separately loaded DTB in uboot. uboot fails to update the dtb if these nodes are missing.

Frankly, I think the original change (removing the skeleton.dtsi include) was misguided and needs to be reverted - the change is imho built upon an incorrect assumption that nothing in skeleton.dtsi is required. That's clearly false.

Quoted 6 lines

I tend to agree as I assume it's not only U-Boot on i.MX but on all ARM platforms.

Best regards
Uwe

Adding back the chosen nodes is a bandaid that papers over the actual bug in the decompressor.

A comment about Fabio's attempt to fix the decompressor noted a possible issue with the method taken to fix the problem, but then the discussion of fixing the decompressor was dropped and v3 of the patch to add chosen into a bunch of .dtsi files was applied by Shawn.

Can someone please help Fabio to create a correct patch to fix the decompressor?

Note that the ePAPR v1.1 does require at least one memory node in section 3.1, so the portion of v3 of the patch that adds a memory node is valid in terms of following the standard, not just a U-boot requirement.

Thanks,

Frank

maintainer
#8 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Quoted 72 lines · Frank Rowand
Quoted 56 lines · Uwe Kleine-K?nig
Quoted 25 lines · Russell King - ARM Linux
Quoted 24 lines · Uwe Kleine-K?nig
Quoted 9 lines · Fabio Estevam

Hi Uwe,

Quoted 3 lines · Uwe Kleine-K?nig

Would it be nice to add a comment about why this was added? Something to prevent a cleanup like "remove empty nodes and invalid memory configurations".

Quoted 5 lines

Do you mean something like this?

/* "chosen" and "memory" nodes are mandatory */
chosen {};
memory { device_type = "memory"; reg = <0 0>; };

Not very helpful comment. Something like:

/*
* The decompressor relies on a pre-existing chosen node to be
* available to insert the command line and merge other ATAGS
* info.
*/

Quoted 4 lines
Quoted 1 line

Is it difficult to fix the decompressor?

... and that comment would be wrong. Yes, the decompressor relies on it, as do some uboot versions.

Good that we talked about this, otherwise I would have removed that once the decompressor is fixed. So the comment must be

/*
* The decompressor and also some versions of U-Boot rely on a
* pre-existing /chosen node to be available to insert the
* command line and merge other ATAGS info.
* Also for U-Boot there must be a pre-existing /memory node. */

Quoted 7 lines
Quoted 2 lines

I didn't understood the breakage regarding the memory node good enough to suggest a comment for that.

A missing memory node appears to prevent some uboot versions supplying any kind of memory layout to the kernel, which then causes the kernel to crash very early during boot.

Quoted 7 lines

Again, this is not using appended DTB - this is using a separately loaded DTB in uboot. uboot fails to update the dtb if these nodes are missing.

Frankly, I think the original change (removing the skeleton.dtsi include) was misguided and needs to be reverted - the change is imho built upon an incorrect assumption that nothing in skeleton.dtsi is required. That's clearly false.

Quoted 6 lines

I tend to agree as I assume it's not only U-Boot on i.MX but on all ARM platforms.

Best regards
Uwe

Adding back the chosen nodes is a bandaid that papers over the actual bug in the decompressor.

A comment about Fabio's attempt to fix the decompressor noted a possible issue with the method taken to fix the problem, but then the discussion of fixing the decompressor was dropped and v3 of the patch to add chosen into a bunch of .dtsi files was applied by Shawn.

Quoted 4 lines

Can someone please help Fabio to create a correct patch to fix the decompressor?

Note that the ePAPR v1.1 does require at least one memory node in

We have a DT spec now, let's stop pointing people to ePAPR.

Quoted 3 lines

section 3.1, so the portion of v3 of the patch that adds a memory node is valid in terms of following the standard, not just a U-boot requirement.

I'd have no issue making chosen be required. Any platform with a serial console should be setting stdout-path anyway. Just add it to the spec. :) We should also add a dtc check as well if we do that.

Rob

#9 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Quoted 84 lines · Rob Herring
Quoted 72 lines · Frank Rowand
Quoted 56 lines · Uwe Kleine-K?nig
Quoted 25 lines · Russell King - ARM Linux
Quoted 24 lines · Uwe Kleine-K?nig
Quoted 9 lines · Fabio Estevam

Hi Uwe,

Quoted 3 lines · Uwe Kleine-K?nig

Would it be nice to add a comment about why this was added? Something to prevent a cleanup like "remove empty nodes and invalid memory configurations".

Quoted 5 lines

Do you mean something like this?

/* "chosen" and "memory" nodes are mandatory */
chosen {};
memory { device_type = "memory"; reg = <0 0>; };

Not very helpful comment. Something like:

/*
* The decompressor relies on a pre-existing chosen node to be
* available to insert the command line and merge other ATAGS
* info.
*/

Quoted 4 lines
Quoted 1 line

Is it difficult to fix the decompressor?

... and that comment would be wrong. Yes, the decompressor relies on it, as do some uboot versions.

Good that we talked about this, otherwise I would have removed that once the decompressor is fixed. So the comment must be

/*
* The decompressor and also some versions of U-Boot rely on a
* pre-existing /chosen node to be available to insert the
* command line and merge other ATAGS info.
* Also for U-Boot there must be a pre-existing /memory node. */

Quoted 7 lines
Quoted 2 lines

I didn't understood the breakage regarding the memory node good enough to suggest a comment for that.

A missing memory node appears to prevent some uboot versions supplying any kind of memory layout to the kernel, which then causes the kernel to crash very early during boot.

Quoted 7 lines

Again, this is not using appended DTB - this is using a separately loaded DTB in uboot. uboot fails to update the dtb if these nodes are missing.

Frankly, I think the original change (removing the skeleton.dtsi include) was misguided and needs to be reverted - the change is imho built upon an incorrect assumption that nothing in skeleton.dtsi is required. That's clearly false.

Quoted 6 lines

I tend to agree as I assume it's not only U-Boot on i.MX but on all ARM platforms.

Best regards
Uwe

Adding back the chosen nodes is a bandaid that papers over the actual bug in the decompressor.

A comment about Fabio's attempt to fix the decompressor noted a possible issue with the method taken to fix the problem, but then the discussion of fixing the decompressor was dropped and v3 of the patch to add chosen into a bunch of .dtsi files was applied by Shawn.

Quoted 4 lines

Can someone please help Fabio to create a correct patch to fix the decompressor?

Note that the ePAPR v1.1 does require at least one memory node in

We have a DT spec now, let's stop pointing people to ePAPR.

Quoted 3 lines

section 3.1, so the portion of v3 of the patch that adds a memory node is valid in terms of following the standard, not just a U-boot requirement.

Quoted 6 lines

I'd have no issue making chosen be required. Any platform with a serial console should be setting stdout-path anyway. Just add it to the spec. :) We should also add a dtc check as well if we do that.

Rob

Sounds good to me.

-Frank

#10 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

In reply to Frank Rowand
Quoted 10 lines · Frank Rowand

Adding back the chosen nodes is a bandaid that papers over the actual bug in the decompressor.

A comment about Fabio's attempt to fix the decompressor noted a possible issue with the method taken to fix the problem, but then the discussion of fixing the decompressor was dropped and v3 of the patch to add chosen into a bunch of .dtsi files was applied by Shawn.

Can someone please help Fabio to create a correct patch to fix the decompressor?

I refuse to assist, see below.

The reason this has come up is that this problem has _nothing_ to do with the decompressor - see recent context on the linux-arm-kernel list.

The removal of skeleton.dtsi from iMX6 DTS files has caused a regression in the current -rc kernels when an iMX6 board is booted with uboot using a _separated_ device tree file.

In that situation, the decompressor has _nothing_ at all to do with modifying the DT binary file. The DT binary file ends up being passed to the kernel from the boot loader unmodified by the decompressor. Remember that the decompressor's modification of the *appended* DT file is a band-aid to allow an *ATAG* based system to continue working. That doesn't apply to iMX6.

The lack of the memory node means that uboot fails to add a memory node to the DT binary file before passing control to the decompressor, which in turn causes the kernel to be booted with a DT binary file which says that the system has no memory. That causes the kernel to crash in the very early stages of boot, before the console is up and running. So it's a nasty silent crash.

So, while it may be desirable to fix the decompressor, it's completely pointless as far as whether Fabio's patch gets merged, because this _regression_ has nothing to do with the decompressor.

We need this regression fixed before 4.10 comes out so systems don't break when 4.10 comes out.

Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
#12 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

In reply to Russell King - ARM Linux

Hello,

Quoted 9 lines · Russell King - ARM Linux
Quoted 8 lines · Frank Rowand

Adding back the chosen nodes is a bandaid that papers over the actual bug in the decompressor.

A comment about Fabio's attempt to fix the decompressor noted a possible issue with the method taken to fix the problem, but then the discussion of fixing the decompressor was dropped and v3 of the patch to add chosen into a bunch of .dtsi files was applied by Shawn.

Quoted 8 lines
Quoted 2 lines

Can someone please help Fabio to create a correct patch to fix the decompressor?

I refuse to assist, see below.

The reason this has come up is that this problem has _nothing_ to do with the decompressor - see recent context on the linux-arm-kernel list.

Several people reported problems with the patch that removes skeleton.dtsi. There are at least:

- Russell, who saw U-Boot failing to specify the amount of RAM;
- somebody (I forgot and I'm lazy so I didn't look up the name) who noticed some i.MX6 boards fail in kernelci with barebox because barebox assumed /chosen to be present; and
- somebody else (...) who reported the ATAGS_DTB_COMPAT failure (also wrongly called "decompressor failure" in this thread).

I agree that for each single of the above issues it is worth to readd the /chosen and /memory nodes for now. Still all three are IMHO worth to be fixed.

barebox is already fixed in v2017.01.0 (commit f49b415b9231 (of: base:
add chosen node if it does not exist when adding initrd),
https://git.pengutronix.de/cgit/barebox/commit/?id=f49b415b9231).

Looking at the ATAGS_DTB_COMPAT problem: The code in question is (if I understood correctly) merge_fdt_bootargs in arch/arm/boot/compressed/atags_to_fdt.c:

	setprop_string(fdt, "/chosen", "bootargs", cmdline);

looking further:

	static int node_offset(void *fdt, const char *node_path)
	{       
		int offset = fdt_path_offset(fdt, node_path);
		if (offset == -FDT_ERR_NOTFOUND)
			offset = fdt_add_subnode(fdt, 0, node_path);
		return offset;
	}

	static int setprop_string(void *fdt, const char *node_path,
				  const char *property, const char *string)
	{
		int offset = node_offset(fdt, node_path);
		if (offset < 0)
			return offset;
		return fdt_setprop_string(fdt, offset, property, string);
	}

So it seems "/chosen" not being there should be handled. I don't have a reproducer for this problem, but it doesn't look that hard to fix with DEBUG_LL and a system that shows the problem.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#13 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

In reply to Russell King - ARM Linux

Hi,

Quoted 8 lines
Quoted 2 lines · Russell King - ARM Linux

/We need this regression fixed before 4.10 comes out so systems don't/ /break when 4.10 comes out./

It will be fixed in the next rc with the following commit:

https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/commit/?h=imx/fixes&id=a971c5545c3d45a1e33fda6e57913bb75aaa20c9
<https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/commit/?h=imx/fixes&id=a971c5545c3d45a1e33fda6e57913bb75aaa20c9>

i want to report that the combination i.MX28 + imx-bootlets (requires CONFIG_ARM_APPENDED_DTB since bootlets can't handle DTBs) was also affected before and fixed by this patch.

Btw are Arnd and Olof informed about this urgent patch?

#14 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Am 01.02.2017 um 14:30 schrieb Stefan Wahren:

Quoted 13 lines

Hi,

Quoted 7 lines
Quoted 2 lines · Russell King - ARM Linux

/We need this regression fixed before 4.10 comes out so systems don't/ /break when 4.10 comes out./

It will be fixed in the next rc with the following commit:

https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/commit/?h=imx/fixes&id=a971c5545c3d45a1e33fda6e57913bb75aaa20c9
<https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/commit/?h=imx/fixes&id=a971c5545c3d45a1e33fda6e57913bb75aaa20c9>

Quoted 5 lines

i want to report that the combination i.MX28 + imx-bootlets (requires CONFIG_ARM_APPENDED_DTB since bootlets can't handle DTBs) was also affected before and fixed by this patch.

Btw are Arnd and Olof informed about this urgent patch?

@Shawn

Okay, 4.10-rc7 is out but this fix isn't applied :-(

Is there a chance for 4.10 or do we have to wait for 4.10.1?

Maybe the subject of the patch is to harmless.

reviewer
#15 lore raw

PATCH v2 1/1 ARM: dts: imx: Pass 'chosen' and 'memory' nodes

Hi Stefan,

Quoted 9 lines · Stefan Wahren
Quoted 1 line

Btw are Arnd and Olof informed about this urgent patch?

@Shawn

Okay, 4.10-rc7 is out but this fix isn't applied :-(

Is there a chance for 4.10 or do we have to wait for 4.10.1?

Maybe the subject of the patch is to harmless.

The patch is in the arm-soc 'fixes' branch:
https://git.kernel.org/cgit/linux/kernel/git/arm/arm-soc.git/commit/?h=fixes&id=a971c5545c3d45a1e33fda6e57913bb75aaa20c9

Olof/Arnd,

Will this branch be sent to Linus so that it lands at 4.10 final?

Thanks