Re: [PATCH 2/2] ARM: layerscape: ppa: Fix starting PPA

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

Hello Sascha,

On 20.11.23 15:44, Sascha Hauer wrote:
> With dba1c26f70 we replaced request_sdram_region() for the PPA with
> reserve_sdram_region(). The effect is that the region is marked as
> reserved and mapped non executable. While this is desired for EL2, it
> also has the effect that we can't start the PPA anymore from EL3.
> 
> Map the region cached/executable to start the PPA, then map it
> uncached/non executable once we are in EL2.
> 
> Fixes: dba1c26f70 ("arm: layerscape: ppa: reserve SDRAM region for PPA")
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-layerscape/ppa.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/mach-layerscape/ppa.c b/arch/arm/mach-layerscape/ppa.c
> index 521e6b89da..43145a7ece 100644
> --- a/arch/arm/mach-layerscape/ppa.c
> +++ b/arch/arm/mach-layerscape/ppa.c
> @@ -4,6 +4,7 @@
>  
>  #include <common.h>
>  #include <init.h>
> +#include <mmu.h>
>  #include <firmware.h>
>  #include <memory.h>
>  #include <linux/sizes.h>
> @@ -54,17 +55,11 @@ static int ppa_init(void *ppa, size_t ppa_size, void *sec_firmware_addr)
>  	int ret;
>  	u32 *boot_loc_ptr_l, *boot_loc_ptr_h;
>  	struct ccsr_scfg __iomem *scfg = (void *)(LSCH2_SCFG_ADDR);
> -	int el = current_el();
>  	struct fit_handle *fit;
>  	void *conf;
>  	const void *buf;
>  	unsigned long firmware_size;
>  
> -	if (el < 3) {
> -		printf("EL%d, skip ppa init\n", el);
> -		return 0;
> -	}
> -
>  	boot_loc_ptr_l = &scfg->scratchrw[1];
>  	boot_loc_ptr_h = &scfg->scratchrw[0];
>  
> @@ -115,6 +110,7 @@ int ls1046a_ppa_init(resource_size_t ppa_start, resource_size_t ppa_size)
>  	struct resource *res;
>  	void *ppa_fw;
>  	size_t ppa_fw_size;
> +	int el = current_el();
>  	int ret;
>  
>  	res = reserve_sdram_region("ppa", ppa_start, ppa_size);
> @@ -126,9 +122,13 @@ int ls1046a_ppa_init(resource_size_t ppa_start, resource_size_t ppa_size)
>  
>  	get_builtin_firmware(ppa_ls1046a_bin, &ppa_fw, &ppa_fw_size);
>  
> -	ret = ppa_init(ppa_fw, ppa_fw_size, (void *)ppa_start);
> -	if (ret)
> -		return ret;
> +	if (el == 3) {
> +		remap_range((void *)ppa_start, ppa_size, MAP_CACHED);
> +		ret = ppa_init(ppa_fw, ppa_fw_size, (void *)ppa_start);
> +		remap_range((void *)ppa_start, ppa_size, MAP_UNCACHED);
> +		if (ret)
> +			return ret;

Isn't this still racy? I think the EL2 mapping should already have the
PPA area uncached _before_ jumping into PPA.

Cheers,
Ahmad

> +	}
>  
>  	of_register_fixup(of_fixup_reserved_memory, res);
>  

-- 
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 |