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

[Chen-Yu Tsai]

On Tue, Aug 15, 2023 at 4:40 AM Andre Przywara <andre.przywara at arm.com> wrote:
>
> On Wed, 31 May 2023 14:15:20 -0600
> Sam Edwards <cfsworks at 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 at 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);
> >  }
>