On Fri, Aug 06, 2021 at 02:22:56PM +0200, Michal Simek wrote:
Based on DT spec you can have one memory node which multiple ranges or multiple nodes. fdt_fixup_memory_banks() is not implemented in a correct way when multiple memory nodes are present because all ranges are put it to the first memory node found. And next memory nodes are kept in DT which ends up in the same range specification in the same DT.
Here is what it is happening. Origin DT. memory@0 { device_type = "memory"; reg = <0x0 0x0 0x0 0x80000000>; };
memory@800000000 { device_type = "memory"; reg = <0x8 0x00000000 0x0 0x80000000>; };
After fdt_fixup_memory_banks()
memory@0 { device_type = "memory"; reg = <0x0 0x0 0x0 0x80000000>, <0x8 0x00000000 0x0 0x80000000>; };
memory@800000000 { device_type = "memory"; reg = <0x8 0x00000000 0x0 0x80000000>; };
As is visible memory@0 node got second range but there is still memory@800000000 node present and 2G range is listed twice.
The solution can't be that second node is removed because it can be referenced already that's why it is better for us to disable this option for now.
I don't object to the patch at all. The main use of fdt_fixup_memory_banks() is for the (at least used to be most common) case of device trees that didn't fill in memory size, so that it could be done at run-time, depending on the amount found. I do wonder if CONFIG_ARCH_FIXUP_FDT_MEMORY being default makes the most sense these days. Or maybe we just need some comments on fdt_fixup_memory_banks making it clear which way we implement the spec as it does allow for as you note, either way.