Re: [Buildroot] [PATCH 2/2] configs/icnova-a20-adb4006: add uboot patch for reliable bootclk

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Ludwig Kormann <ludwig.kormann@ict42.de>
Cc: buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH 2/2] configs/icnova-a20-adb4006: add uboot patch for reliable bootclk
Date: Fri, 11 Oct 2024 08:34:50 +0200	[thread overview]
Message-ID: <20241011083450.50b5e81c@windsurf> (raw)
In-Reply-To: <20241010091531.1943185-2-ludwig.kormann@ict42.de>

Hello Ludwig,

On Thu, 10 Oct 2024 11:15:31 +0200
Ludwig Kormann <ludwig.kormann@ict42.de> wrote:

> Up until now cpu clock gets initialized at 384 MHz, which is
> the highest supported cpu clock.
> 
> Recent A20 batches show an increased percentage of modules
> reacting very sensitive to operating conditions outside the
> specifications.
> 
> The cpu dies very shortly after PLLs, core frequency or cpu
> voltage are missconfigured. E.g.:
> - uboot SPL selects 384 MHz as cpu clock which requires a cpu
>   voltage of at least 1.1 V.
> - Linux CPU Frequency scaling with most sun7i dts will reduce
>   cpu voltage down to 1.0 V.
> - When intiating a reboot or reset from linux the cpu voltage
>   may keep the 1.0 V configuration and the cpu dies during SPL
>   initialization.
> 
> Therefore reduce cpu clock at uboot SPL initialization down
> to 144 MHz from 384 MHz.
> 
> Use patch until KConfig option in uboot becomes available.
> 
> Signed-off-by: Ludwig Kormann <ludwig.kormann@ict42.de>

Thanks as well for your patch! Some comments below.

> ---
>  .../uboot-sun7i-a20-reliable-bootclk.patch            | 11 +++++++++++
>  configs/icnova-a20-adb4006_defconfig                  |  1 +
>  2 files changed, 12 insertions(+)
>  create mode 100644 board/in-circuit/icnova-a20-adb4006/uboot-patches/uboot-sun7i-a20-reliable-bootclk.patch
> 
> diff --git a/board/in-circuit/icnova-a20-adb4006/uboot-patches/uboot-sun7i-a20-reliable-bootclk.patch b/board/in-circuit/icnova-a20-adb4006/uboot-patches/uboot-sun7i-a20-reliable-bootclk.patch
> new file mode 100644
> index 0000000000..ce83f59e4b
> --- /dev/null
> +++ b/board/in-circuit/icnova-a20-adb4006/uboot-patches/uboot-sun7i-a20-reliable-bootclk.patch
> @@ -0,0 +1,11 @@
> +--- uboot/arch/arm/include/asm/arch-sunxi/clock_sun4i.h	2024-10-10 09:34:01.346401734 +0200
> ++++ uboot/arch/arm/include/asm/arch-sunxi/clock_sun4i.h.modified	2024-10-10 09:33:55.146402528 +0200

We require all patches to have a description, a Signed-off-by: line and
an Upstream: tag. Also, since U-Boot is using Git upstream, we strongly
prefer a patch generated by "git format-patch".

> diff --git a/configs/icnova-a20-adb4006_defconfig b/configs/icnova-a20-adb4006_defconfig
> index 948b054d83..4cf7826346 100644
> --- a/configs/icnova-a20-adb4006_defconfig
> +++ b/configs/icnova-a20-adb4006_defconfig
> @@ -38,6 +38,7 @@ BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG=y
>  BR2_TARGET_UBOOT_CUSTOM_VERSION=y
>  BR2_TARGET_UBOOT_CUSTOM_VERSION_VALUE="2024.10"
>  BR2_TARGET_UBOOT_BOARD_DEFCONFIG="icnova-a20-adb4006"
> +BR2_TARGET_UBOOT_PATCH="board/in-circuit/icnova-a20-adb4006/uboot-patches"

Instead of this, could you use BR2_GLOBAL_PATCH_DIRECTORIES? It allows
to patch all packages, not just u-boot, and is anyway going to be
needed to provide the hashes, as I suggested in my review of your PATCH
1/2.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot