On Tue, Aug 15, 2023 at 4:40 AM Andre Przywara <andre.przyw...@arm.com> wrote: > > 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?
IIRC the GIC manual says that the secure bit is set or cleared to select which bank of registers is accessed. And I suppose it is here to be more robust. ChenYu > Does anyone remember? > > Cheers, > Andre > > [1] > https://source.denx.de/u-boot/u-boot/-/commit/d5db7024aafc5ea603f3a34f83bb29a1eaa3cbe7#f377950449a5dba076796d9e48fbd21f5d6ac545_0_65 > > > cp15_write_scr(reg); > > } >
