Re: [PATCH v3] ARM: webasto-ccbv2: Add variant with 512MB RAM

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello,

On 18.05.21 12:57, Holger Assmann wrote:
> Add variant for 512MB RAM board.
> 
> Two firmware files will be generated - one for 256MB and 512MB
> respectively; the choice for shipment has to be done and depends on the
> underlying hardware.
> 
> Consequently, the query for memsize in lowlevel.c
> 
>    ( r0 < MX6_MMDC_P0_BASE_ADDR + X )
> 
> could be dropped since we are now running with a hard-coded X == SZ_512.
>
> However, we keep this part in order to allow for contextual code
> comprehension.

I don't think this reasoning is correct. The board entry point needs to
determine whether it is running after POR or was chainloaded by another
barebox. In the latter case, it needs to avoid loading the OP-TEE again
as that would fail. Rouven assumed that r0 would never point into SDRAM
after BootROM, but would do so after chainloading (because arm32 barebox
always passes along FDT in r0). I don't see why adding a second entry
point means that code could now be dropped.

(A comment might be in-order if you misunderstood the check's intention).

> Reported-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>

You can credit people that reviewed your patch under the ---
line like you did with v1 -> v2. Reported-by is when you fix
an issue reported about something already in tree.

> Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
> ---
> v2 -> v3:
> 
>  - keep omitting RAM-size-dependent handling in lowlevel.c
>  - rectify flash header handling in Makefile.imx
> 
> v1 -> v2:
> 
>  by Rouven Czerwinski <r.czerwinski@pengutronix.de>:
>   - just ship one single firmware version, depending on the underlying
>     board variant
> 
>  by Ahmad Fatoum <a.fatoum@pengutronix.de>:
>   - account for the 512MB variant by hard-coding memsize == SZ_512
> 
>  ...sh-header-imx6ul-webasto-ccbv2-256.imxcfg} |  0
>  ...ash-header-imx6ul-webasto-ccbv2-512.imxcfg | 88 +++++++++++++++++++
>  arch/arm/boards/webasto-ccbv2/lowlevel.c      |  2 +-
>  images/Makefile.imx                           |  4 +-
>  4 files changed, 92 insertions(+), 2 deletions(-)
>  rename arch/arm/boards/webasto-ccbv2/{flash-header-imx6ul-webasto-ccbv2.imxcfg => flash-header-imx6ul-webasto-ccbv2-256.imxcfg} (100%)
>  create mode 100644 arch/arm/boards/webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2-512.imxcfg
> 
> diff --git a/arch/arm/boards/webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2.imxcfg b/arch/arm/boards/webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2-256.imxcfg
> similarity index 100%
> rename from arch/arm/boards/webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2.imxcfg
> rename to arch/arm/boards/webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2-256.imxcfg
> diff --git a/arch/arm/boards/webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2-512.imxcfg b/arch/arm/boards/webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2-512.imxcfg
> new file mode 100644
> index 0000000000..d438a665f1
> --- /dev/null
> +++ b/arch/arm/boards/webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2-512.imxcfg
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +loadaddr 0x80000000
> +soc imx6
> +ivtofs 0x400
> +
> +/* Enable all clocks */
> +wm 32 0x020c4068 0xffffffff
> +wm 32 0x020c406c 0xffffffff
> +wm 32 0x020c4070 0xffffffff
> +wm 32 0x020c4074 0xffffffff
> +wm 32 0x020c4078 0xffffffff
> +wm 32 0x020c407c 0xffffffff
> +wm 32 0x020c4080 0xffffffff
> +
> +/* IOMUX */
> +/* DDR IO type */
> +wm 32 0x020E04B4 0x000C0000
> +wm 32 0x020E04AC 0x00000000
> +/* Clock */
> +wm 32 0x020E027C 0x00000028
> +/* Control */
> +wm 32 0x020E0250 0x00000028
> +wm 32 0x020E024C 0x00000028
> +wm 32 0x020E0490 0x00000028
> +wm 32 0x020E0288 0x00000028
> +wm 32 0x020E0270 0x00000000
> +wm 32 0x020E0260 0x00000028
> +wm 32 0x020E0264 0x00000028
> +wm 32 0x020E04A0 0x00000028
> +/* Data strobe */
> +wm 32 0x020E0494 0x00020000
> +wm 32 0x020E0280 0x00000028
> +wm 32 0x020E0284 0x00000028
> +/* Data */
> +wm 32 0x020E04B0 0x00020000
> +wm 32 0x020E0498 0x00000028
> +wm 32 0x020E04A4 0x00000028
> +wm 32 0x020E0244 0x00000028
> +wm 32 0x020E0248 0x00000028
> +
> +/* DDR Controller registers */
> +wm 32 0x021B001C 0x00008000
> +wm 32 0x021B0800 0xA1390003
> +/* Calibration values */
> +wm 32 0x021B080C 0x00090000
> +wm 32 0x021B083C 0x01580158
> +wm 32 0x021B0848 0x40405050
> +wm 32 0x021B0850 0x4040524C
> +wm 32 0x021B081C 0x33333333
> +wm 32 0x021B0820 0x33333333
> +wm 32 0x021B082C 0xf3333333
> +wm 32 0x021B0830 0xf3333333
> +/* END of calibration values */
> +wm 32 0x021B08C0 0x00921012
> +wm 32 0x021B08b8 0x00000800
> +
> +/* MMDC init */
> +wm 32 0x021B0004 0x0002002D
> +wm 32 0x021B0008 0x1b333030
> +wm 32 0x021B000C 0x676B52F3
> +wm 32 0x021B0010 0xB66D0B63
> +wm 32 0x021B0014 0x01FF00DB
> +/* Consider reducing RALAT (currently set to 5) */
> +wm 32 0x021B0018 0x00211740
> +wm 32 0x021B001C 0x00008000
> +wm 32 0x021B002C 0x000026D2
> +wm 32 0x021B0030 0x006B1023
> +wm 32 0x021B0040 0x0000004F
> +wm 32 0x021B0000 0x84180000
> +
> +/* Mode registers writes for CS0 */
> +wm 32 0x021B001C 0x02008032
> +wm 32 0x021B001C 0x00008033
> +wm 32 0x021B001C 0x00048031
> +wm 32 0x021B001C 0x15208030
> +wm 32 0x021B001C 0x04008040
> +
> +/* Final DDR setup */
> +wm 32 0x021B0020 0x00007800
> +wm 32 0x021B0818 0x00000227
> +wm 32 0x021B0004 0x0002556D
> +wm 32 0x021B0404 0x00011006
> +wm 32 0x021B001C 0x00000000
> +
> +/* Disable TZASC bypass */
> +wm 32 0x020E4024 0x00000001
> +
> +#include <mach/habv4-imx6-gencsf.h>
> diff --git a/arch/arm/boards/webasto-ccbv2/lowlevel.c b/arch/arm/boards/webasto-ccbv2/lowlevel.c
> index 8529ea3735..5b6115bed5 100644
> --- a/arch/arm/boards/webasto-ccbv2/lowlevel.c
> +++ b/arch/arm/boards/webasto-ccbv2/lowlevel.c
> @@ -48,7 +48,7 @@ static void noinline start_ccbv2(u32 r0)
>  	 */
>  	if(IS_ENABLED(CONFIG_FIRMWARE_CCBV2_OPTEE)
>  	   && !(r0 > MX6_MMDC_P0_BASE_ADDR
> -	        &&  r0 < MX6_MMDC_P0_BASE_ADDR + SZ_256M)) {
> +	        &&  r0 < MX6_MMDC_P0_BASE_ADDR + SZ_512M)) {
>  		get_builtin_firmware(ccbv2_optee_bin, &tee, &tee_size);
>  
>  		memset((void *)OPTEE_OVERLAY_LOCATION, 0, 0x1000);
> diff --git a/images/Makefile.imx b/images/Makefile.imx
> index 382493488b..b5e4b3841d 100644
> --- a/images/Makefile.imx
> +++ b/images/Makefile.imx
> @@ -366,7 +366,9 @@ $(call build_imx_habv4img, CONFIG_MACH_TECHNEXION_PICO_HOBBIT, start_imx6ul_pico
>  
>  $(call build_imx_habv4img, CONFIG_MACH_DIGI_CCIMX6ULSBCPRO, start_imx6ul_ccimx6ulsbcpro, digi-ccimx6ulsom/flash-header-imx6ul-ccimx6ulsbcpro, imx6ul-ccimx6ulsbcpro)
>  
> -$(call build_imx_habv4img, CONFIG_MACH_WEBASTO_CCBV2, start_imx6ul_ccbv2, webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2, imx6ul-webasto-ccbv2)
> +$(call build_imx_habv4img, CONFIG_MACH_WEBASTO_CCBV2, start_imx6ul_ccbv2, webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2-256, imx6ul-webasto-ccbv2-256m)
> +
> +$(call build_imx_habv4img, CONFIG_MACH_WEBASTO_CCBV2, start_imx6ul_ccbv2, webasto-ccbv2/flash-header-imx6ul-webasto-ccbv2-512, imx6ul-webasto-ccbv2-512m)
>  
>  # ----------------------- vf6xx based boards ---------------------------
>  pblb-$(CONFIG_MACH_VF610_TWR) += start_vf610_twr
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox