From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751025AbdAQIVu (ORCPT ); Tue, 17 Jan 2017 03:21:50 -0500 Received: from mail-wm0-f47.google.com ([74.125.82.47]:37373 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750958AbdAQIVb (ORCPT ); Tue, 17 Jan 2017 03:21:31 -0500 Subject: Re: [PATCH v4] ARM64: dts: meson-gx: Add reserved memory zone and usable memory range To: Olof Johansson References: <1484129414-23325-1-git-send-email-narmstrong@baylibre.com> <7fcb8d94-840a-de2c-f43b-9123ccc65514@baylibre.com> Cc: =?UTF-8?Q?Andreas_F=c3=a4rber?= , Kevin Hilman , "devicetree@vger.kernel.org" , xypron.glpk@gmx.de, "linux-kernel@vger.kernel.org" , Carlo Caione , linux-amlogic@lists.infradead.org, "linux-arm-kernel@lists.infradead.org" From: Neil Armstrong Organization: Baylibre Message-ID: Date: Tue, 17 Jan 2017 09:21:27 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/17/2017 07:07 AM, Olof Johansson wrote: > On Mon, Jan 16, 2017 at 2:39 AM, Neil Armstrong wrote: >> On 01/15/2017 03:43 PM, Andreas Färber wrote: >>> Am 13.01.2017 um 21:03 schrieb Kevin Hilman: >>>> Neil Armstrong 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 >>>> >>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: narmstrong@baylibre.com (Neil Armstrong) Date: Tue, 17 Jan 2017 09:21:27 +0100 Subject: [PATCH v4] ARM64: dts: meson-gx: Add reserved memory zone and usable memory range In-Reply-To: References: <1484129414-23325-1-git-send-email-narmstrong@baylibre.com> <7fcb8d94-840a-de2c-f43b-9123ccc65514@baylibre.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 01/17/2017 07:07 AM, Olof Johansson wrote: > On Mon, Jan 16, 2017 at 2:39 AM, Neil Armstrong wrote: >> On 01/15/2017 03:43 PM, Andreas F?rber wrote: >>> Am 13.01.2017 um 21:03 schrieb Kevin Hilman: >>>> Neil Armstrong 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 >>>> >>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: narmstrong@baylibre.com (Neil Armstrong) Date: Tue, 17 Jan 2017 09:21:27 +0100 Subject: [PATCH v4] ARM64: dts: meson-gx: Add reserved memory zone and usable memory range In-Reply-To: References: <1484129414-23325-1-git-send-email-narmstrong@baylibre.com> <7fcb8d94-840a-de2c-f43b-9123ccc65514@baylibre.com> Message-ID: To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org On 01/17/2017 07:07 AM, Olof Johansson wrote: > On Mon, Jan 16, 2017 at 2:39 AM, Neil Armstrong wrote: >> On 01/15/2017 03:43 PM, Andreas F?rber wrote: >>> Am 13.01.2017 um 21:03 schrieb Kevin Hilman: >>>> Neil Armstrong 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 >>>> >>>> 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