Re: [PATCH] sunxi: psci: remove redundant initialization from psci_arch_init

[Andre Przywara <andre.przywara@arm.com>]

On Wed, 31 May 2023 14:15:20 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi,

(CC:ing Marc and Chen-Yu as the original authors)

sorry for the delay, found that mouldering in my Drafts folder.

> The nonsec code overrides/handles these:
> - Setting PMR to 0xFF happens earlier, in _nonsec_init, so this is
>   redundant.
> - The NS bit is not yet set: it gets set later in _secure_monitor.
>   Trying to clear it here is pointless and misleading.

So superficially this looks alright, but I am still somewhat reluctant
to apply this, see below ...

> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  arch/arm/cpu/armv7/sunxi/psci.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> index e1d3638b5c..f866025c37 100644
> --- a/arch/arm/cpu/armv7/sunxi/psci.c
> +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> @@ -300,14 +300,10 @@ void __secure psci_arch_init(void)
>  	/* Set SGI15 priority to 0 */
>  	writeb(0, GICD_BASE + GICD_IPRIORITYRn + 15);
>  
> -	/* Be cool with non-secure */
> -	writel(0xff, GICC_BASE + GICC_PMR);
> -
>  	/* Switch FIQEn on */
>  	setbits_le32(GICC_BASE + GICC_CTLR, BIT(3));
>  
>  	reg = cp15_read_scr();
>  	reg |= BIT(2);  /* Enable FIQ in monitor mode */
> -	reg &= ~BIT(0); /* Secure mode */

I am scratching my head on this one: I see it deliberately being done in
the original patch by Marc[1], and wonder why. If I read the code and
the architecture correctly, this routine is called only(?) from the SMC
handler, which means we are in monitor mode, that only exists in secure
state. So the NS bit is irrelevant until we switch to another mode. And
we indeed set the bit later, before dropping back to non-secure. So I
agree that clearing the bit is pointless. Was it put in to be more
robust, for other potential callers of this function, for instance in
the boot path?

Does anyone remember?

Cheers,
Andre

[1]
https://source.denx.de/u-boot/u-boot/-/commit/d5db7024aafc5ea603f3a34f83bb29a1eaa3cbe7#f377950449a5dba076796d9e48fbd21f5d6ac545_0_65

>  	cp15_write_scr(reg);
>  }