[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 at 0 {
>>>>         device_type = "memory";
>>>>         reg = <0x0 0x0 0x0 0x80000000>;
>>>> };
>>>>
>>>> memory at 800000000 {
>>>>         device_type = "memory";
>>>>         reg = <0x8 0x00000000 0x0 0x80000000>;
>>>> };
>>>>
>>>> After fdt_fixup_memory_banks()
>>>>
>>>> memory at 0 {
>>>>         device_type = "memory";
>>>>         reg = <0x0 0x0 0x0 0x80000000>, <0x8 0x00000000 0x0 0x80000000>;
>>>> };
>>>>
>>>> memory at 800000000 {
>>>>         device_type = "memory";
>>>>         reg = <0x8 0x00000000 0x0 0x80000000>;
>>>> };
>>>>
>>>> As is visible memory at 0 node got second range but there is still
>>>> memory at 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