Message-ID: <ecb5ff50-f5d5-ffe6-0732-faf5e51906f8@baylibre.com>
Date:   Tue, 17 Jan 2017 09:21:27 +0100
From:   Neil Armstrong <narmstrong@...libre.com>
To:     Olof Johansson <olof@...om.net>
Cc:     Andreas Färber <afaerber@...e.de>,
        Kevin Hilman <khilman@...libre.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        xypron.glpk@....de,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Carlo Caione <carlo@...one.org>,
        linux-amlogic@...ts.infradead.org,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4] ARM64: dts: meson-gx: Add reserved memory zone and
 usable memory range

On 01/17/2017 07:07 AM, Olof Johansson wrote:
> On Mon, Jan 16, 2017 at 2:39 AM, Neil Armstrong <narmstrong@...libre.com> wrote:
>> On 01/15/2017 03:43 PM, Andreas Färber wrote:
>>> Am 13.01.2017 um 21:03 schrieb Kevin Hilman:
>>>> Neil Armstrong <narmstrong@...libre.com> writes:
>>>>
>>>>> The Amlogic Meson GXBB/GXL/GXM secure monitor uses part of the memory space,
>>>>> this patch adds this reserved zone and redefines the usable memory range.
>>>>>
>>>>> The memory node is also moved from the dtsi files into the proper dts files
>>>>> to handle variants memory sizes.
>>>>>
>>>>> This patch also fixes the memory sizes for the following platforms :
>>>>> - gxl-s905x-p212 : 1GiB instead of 2GiB, a proper 2GiB dts should be pushed
>>>>> - gxm-s912-q201 : 1GiB instead of 2GiB, a proper 2GiB dts should be pushed
>>>>> - gxl-s905d-p231 : 1GiB instead of 2GiB, a proper 2GiB dts should be pushed
>>>>> - gxl-nexbox-a95x : 1GiB instead of 2GiB, a proper 2GiB dts should be pushed
>>>>>
>>>>> Signed-off-by: Neil Armstrong <narmstrong@...libre.com>
>>>>
>>>> Queued for v4.10-rc.
>>>
>>> What is the motivation for this change? I have a local U-Boot patch to
>>> detect the amount of memory available as done downstream, but U-Boot
>>> only updates the reg property that you seem to be abandoning here...
>>>
>>> So for devices that come in multiple RAM configurations - like R-Box Pro
>>> - this would require separate .dts files now! This looks very wrong to
>>> me, especially since I am not aware of other platforms doing the same.
>>> Instead, there's memory reservations for top and bottom done in U-Boot
>>> for reg, plus reserved-memory nodes for anything in the middle.
>>>
>>> Another thing to consider is that uEFI boot (bootefi) handles memory
>>> reservation differently yet again, on the bootloader level. I have had
>>> that working fine on Odroid-C2 and Vega S95.
>>>
>>> So if there's no bug this is fixing (none mentioned in commit message) I
>>> strongly object to this patch.
>>>
>>> Regards,
>>> Andreas
>>>
>>
>> Hi Andreas,
>>
>> Like I replied of my RFT patch :
>> I really disagree about relying on any work or properties added by any bootloader here, Amlogic SoCs has
>> a lot of u-boot versions in the field, and the Odroid-C2 is part of this.
>>
>> Even if Odroid-c2 is in mainline U-Boot or not, the mainline Linux kernel should work using
>> any U-boot version even with the one provided by Amlogic on their openlinux distribution channel.
>>
>> Handling multiple RAM configuration is another story, and the Arm-Soc and DT maintainers should give us
>> their advices.
> 
> Is there a way to detect what firmware is running and marking off
> memory from early kernel init instead? That'll take care of the
> concerns about memory size variance as well.
> 
>> Actually there is a severe bug fixed here that cause a huge crash if such memory is not reserved while
>> running stock u-boot version on various shipped products and Amlogic's own development boards.
>>
>> The bug is easily triggered by running :
>> # stress --vm 4 --vm-bytes 128M --timeout 10s &
>> [   46.937975] Bad mode in Error handler detected on CPU1, code 0xbf000000 -- SError
>> ...
>> [   47.058536] Internal error: Attempting to execute userspace memory: 8600000f [#3] PREEMPT SMP
>> ...
>>
>> Note this is a fix targeted for 4.10 to make the system stable and various users reported some severe
>> crash now the system has more drivers and read-world use-cases are running on Amlogic SoCs.
>>
>> Please feel free to push whatever changes that makes this memory reservation more coherent for 4.11,
>> and respect the behavior of already shipped u-boot version and mainline U-Boot, UEFI, whatever...
> 
> Technically we're not in regression territory here, since the platform
> is obviously still in bringup and these aren't bugs that have been
> introduced in this release. So I think we can take a little while to
> sort out if there's a solution that, even if not ideal, at least is on
> the path towards the proper fix and not away from it -- which this
> seems to be.
> 
> 
> -Olof
> 

Hi Olof, Andreas,

As I finally understand, the real issue here is the usage of the "linux,useable-memory" property that
overrides the reg property that is changed by the bootloader to provide the "real" memory size.

As I understand the mainline U-Boot does it right, and it's a good news, and it seems uEFI need to provide
some specialized memory range aswell, but the vendor U-Boot versions only provide the full memory range here.
It seems obvious that whatever range is provided by u-boot, the first 16MiB should be reserved.

The stress-ng package provides this "stress" command and is used to force the kernel to map more memory
zones, but I also got the issue while running a fully fledged Desktop Environment thanks to the
recently merged DRM driver.
You may not be able to trigger the issue since it seems Amlogic reduces this reserved size on GXL/GXM :
https://github.com/khadas/linux/commit/698df2c6cfbb0d1a9359743208e83517b31da6ce
But it should be confirmed.

Kevin asked me initially to handle this "start of ddr" reserved zone via a reserved-memory entry, but
at that time it seemed a better idea to use "linux,useable-memory", but I recon it may be an error.

I will push a v5 with a supplementary reserved-memory entry and will postpone the boards memory size
fixup for a future DTS cleanup.

Andreas, is this ok for you ?

This issue exists since forever on mainline linux, and even 4.9 has it.
Olof, How could a similar fix go in 4.9 stable ?

Thanks,
Neil

Powered by blists - more mailing lists