Re: [PATCH] xilinx: Disable ARCH_FIXUP_FDT_MEMORY

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]