[PATCH] xilinx: Disable ARCH_FIXUP_FDT_MEMORY

[PATCH] xilinx: Disable ARCH_FIXUP_FDT_MEMORY

[Michal Simek]

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.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 configs/xilinx_versal_virt_defconfig | 1 +
 configs/xilinx_zynq_virt_defconfig   | 1 +
 configs/xilinx_zynqmp_virt_defconfig | 1 +
 3 files changed, 3 insertions(+)

diff --git a/configs/xilinx_versal_virt_defconfig b/configs/xilinx_versal_virt_defconfig
index e67905178dee..c894d32a9259 100644
--- a/configs/xilinx_versal_virt_defconfig
+++ b/configs/xilinx_versal_virt_defconfig
@@ -13,6 +13,7 @@ CONFIG_COUNTER_FREQUENCY=100000000
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_FIT_VERBOSE=y
+# CONFIG_ARCH_FIXUP_FDT_MEMORY is not set
 CONFIG_BOOTDELAY=5
 CONFIG_USE_PREBOOT=y
 # CONFIG_DISPLAY_CPUINFO is not set
diff --git a/configs/xilinx_zynq_virt_defconfig b/configs/xilinx_zynq_virt_defconfig
index b4c7f11505c1..573a10fe221b 100644
--- a/configs/xilinx_zynq_virt_defconfig
+++ b/configs/xilinx_zynq_virt_defconfig
@@ -23,6 +23,7 @@ CONFIG_SPL_LOAD_FIT=y
 CONFIG_SPL_LOAD_FIT_ADDRESS=0x10000000
 # CONFIG_USE_SPL_FIT_GENERATOR is not set
 CONFIG_LEGACY_IMAGE_FORMAT=y
+# CONFIG_ARCH_FIXUP_FDT_MEMORY is not set
 CONFIG_USE_PREBOOT=y
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_FPGA=y
diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
index 623228053e47..2c888130fa59 100644
--- a/configs/xilinx_zynqmp_virt_defconfig
+++ b/configs/xilinx_zynqmp_virt_defconfig
@@ -21,6 +21,7 @@ CONFIG_FIT=y
 CONFIG_FIT_VERBOSE=y
 CONFIG_SPL_LOAD_FIT=y
 CONFIG_SPL_LOAD_FIT_ADDRESS=0x10000000
+# CONFIG_ARCH_FIXUP_FDT_MEMORY is not set
 CONFIG_USE_PREBOOT=y
 CONFIG_PREBOOT="scsi reset;usb reset"
 # CONFIG_DISPLAY_CPUINFO is not set
-- 
2.32.0

Re: [PATCH] xilinx: Disable ARCH_FIXUP_FDT_MEMORY

[Tom Rini]

[-- 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 --]

Re: [PATCH] xilinx: Disable ARCH_FIXUP_FDT_MEMORY

[Michal Simek]

On 8/6/21 8:46 PM, Tom Rini wrote:
> 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.

The multi memory node configuration is not that common and the most of
SOC are using one node with multiple ranges. It means I would say a lot
of SOCs are not affected.
Not sure how many SOCs are really using this feature that at run time
you detect amount of memory. I remember freescale powerquicks with
reading SPD eeprom on DIMMs and then one custom board with xilinx soc
which was produced with two memory sizes where detection was performed.

But anyway if you want me to add comment to that function to describe
what it is implemented now I can do it.
And of course the best would be to update this function handle both ways
but for us is it better to disable this for now till we have real need
to use it.
We have also done internally one implementation but I don't think it
works in generic way.

Thanks,
Michal

Re: [PATCH] xilinx: Disable ARCH_FIXUP_FDT_MEMORY

[Tom Rini]

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

On Mon, Aug 09, 2021 at 08:24:48AM +0200, Michal Simek wrote:
> 
> 
> On 8/6/21 8:46 PM, Tom Rini wrote:
> > 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.
> 
> The multi memory node configuration is not that common and the most of
> SOC are using one node with multiple ranges. It means I would say a lot
> of SOCs are not affected.

OK.

> Not sure how many SOCs are really using this feature that at run time
> you detect amount of memory. I remember freescale powerquicks with
> reading SPD eeprom on DIMMs and then one custom board with xilinx soc
> which was produced with two memory sizes where detection was performed.

I think there's still a large number of platforms that don't describe
the amount of memory in the device tree and let it be figured out at
"run time", even if there's not nearly the level of SPD EEPROM related
smarts as there are in some environments.  U-Boot's get_ram_size() is
usually good enough for those cases.  But it's been a long time since I
checked a wide variety of dts files, and that it gets the more than 4GB
case wrong too is why there are a number of platforms that opt-out.

> But anyway if you want me to add comment to that function to describe
> what it is implemented now I can do it.
> And of course the best would be to update this function handle both ways
> but for us is it better to disable this for now till we have real need
> to use it.
> We have also done internally one implementation but I don't think it
> works in generic way.

A comment for now would be a great start, thanks!

-- 
Tom

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

Re: [PATCH] xilinx: Disable ARCH_FIXUP_FDT_MEMORY

[Michal Simek]

On 8/9/21 6:31 PM, Tom Rini wrote:
> On Mon, Aug 09, 2021 at 08:24:48AM +0200, Michal Simek wrote:
>>
>>
>> On 8/6/21 8:46 PM, Tom Rini wrote:
>>> 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.
>>
>> The multi memory node configuration is not that common and the most of
>> SOC are using one node with multiple ranges. It means I would say a lot
>> of SOCs are not affected.
> 
> OK.
> 
>> Not sure how many SOCs are really using this feature that at run time
>> you detect amount of memory. I remember freescale powerquicks with
>> reading SPD eeprom on DIMMs and then one custom board with xilinx soc
>> which was produced with two memory sizes where detection was performed.
> 
> I think there's still a large number of platforms that don't describe
> the amount of memory in the device tree and let it be figured out at
> "run time", even if there's not nearly the level of SPD EEPROM related
> smarts as there are in some environments.  U-Boot's get_ram_size() is
> usually good enough for those cases.  But it's been a long time since I
> checked a wide variety of dts files, and that it gets the more than 4GB
> case wrong too is why there are a number of platforms that opt-out.
> 
>> But anyway if you want me to add comment to that function to describe
>> what it is implemented now I can do it.
>> And of course the best would be to update this function handle both ways
>> but for us is it better to disable this for now till we have real need
>> to use it.
>> We have also done internally one implementation but I don't think it
>> works in generic way.
> 
> A comment for now would be a great start, thanks!
> 

Just for a record. Here it is send.
https://lists.denx.de/pipermail/u-boot/2021-August/457629.html

Thanks,
Michal

Re: [PATCH] xilinx: Disable ARCH_FIXUP_FDT_MEMORY

[Michal Simek]

pá 6. 8. 2021 v 14:22 odesílatel Michal Simek <michal.simek@xilinx.com> napsal:
>
> 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.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>  configs/xilinx_versal_virt_defconfig | 1 +
>  configs/xilinx_zynq_virt_defconfig   | 1 +
>  configs/xilinx_zynqmp_virt_defconfig | 1 +
>  3 files changed, 3 insertions(+)
>
> diff --git a/configs/xilinx_versal_virt_defconfig b/configs/xilinx_versal_virt_defconfig
> index e67905178dee..c894d32a9259 100644
> --- a/configs/xilinx_versal_virt_defconfig
> +++ b/configs/xilinx_versal_virt_defconfig
> @@ -13,6 +13,7 @@ CONFIG_COUNTER_FREQUENCY=100000000
>  CONFIG_DISTRO_DEFAULTS=y
>  CONFIG_FIT=y
>  CONFIG_FIT_VERBOSE=y
> +# CONFIG_ARCH_FIXUP_FDT_MEMORY is not set
>  CONFIG_BOOTDELAY=5
>  CONFIG_USE_PREBOOT=y
>  # CONFIG_DISPLAY_CPUINFO is not set
> diff --git a/configs/xilinx_zynq_virt_defconfig b/configs/xilinx_zynq_virt_defconfig
> index b4c7f11505c1..573a10fe221b 100644
> --- a/configs/xilinx_zynq_virt_defconfig
> +++ b/configs/xilinx_zynq_virt_defconfig
> @@ -23,6 +23,7 @@ CONFIG_SPL_LOAD_FIT=y
>  CONFIG_SPL_LOAD_FIT_ADDRESS=0x10000000
>  # CONFIG_USE_SPL_FIT_GENERATOR is not set
>  CONFIG_LEGACY_IMAGE_FORMAT=y
> +# CONFIG_ARCH_FIXUP_FDT_MEMORY is not set
>  CONFIG_USE_PREBOOT=y
>  CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_FPGA=y
> diff --git a/configs/xilinx_zynqmp_virt_defconfig b/configs/xilinx_zynqmp_virt_defconfig
> index 623228053e47..2c888130fa59 100644
> --- a/configs/xilinx_zynqmp_virt_defconfig
> +++ b/configs/xilinx_zynqmp_virt_defconfig
> @@ -21,6 +21,7 @@ CONFIG_FIT=y
>  CONFIG_FIT_VERBOSE=y
>  CONFIG_SPL_LOAD_FIT=y
>  CONFIG_SPL_LOAD_FIT_ADDRESS=0x10000000
> +# CONFIG_ARCH_FIXUP_FDT_MEMORY is not set
>  CONFIG_USE_PREBOOT=y
>  CONFIG_PREBOOT="scsi reset;usb reset"
>  # CONFIG_DISPLAY_CPUINFO is not set
> --
> 2.32.0
>

Applied.
M

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs