On Wed, 31 May 2023 14:15:20 -0600 Sam Edwards <cfswo...@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 <cfswo...@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); > }
