Re: [PATCH 4/8] arm64: dts: add NXP S32G2 support

Chester Lin
2021-08-21T12:39:24Z

Hi Marc,

On Fri, Aug 20, 2021 at 04:29:00PM +0100, Marc Zyngier wrote:
> On Fri, 20 Aug 2021 16:15:49 +0100,
> Chester Lin <clin@suse.com> wrote:
> >
> > On Fri, Aug 20, 2021 at 02:12:13PM +0100, Marc Zyngier wrote:
> > > On Thu, 12 Aug 2021 18:26:28 +0100,
> > > Andreas Färber <afaerber@suse.de> wrote:
> > > >
> > > > Hi Chester et al.,
> > > >
> > > > On 05.08.21 08:54, Chester Lin wrote:
> > > > > Add an initial dtsi file for generic SoC features of NXP S32G2.
> > > > >
> > > > > Signed-off-by: Chester Lin <clin@suse.com>
> > > > > ---
> > > > > arch/arm64/boot/dts/freescale/s32g2.dtsi | 98 ++++++++++++++++++++++++
> > > > > 1 file changed, 98 insertions(+)
> > > > > create mode 100644 arch/arm64/boot/dts/freescale/s32g2.dtsi
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> > > > > new file mode 100644
> > > > > index 000000000000..3321819c1a2d
> > > > > --- /dev/null
> > > > > +++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi
> > >
> > > [...]
> > >
> > > > > + gic: interrupt-controller@50800000 {
> > > > > + compatible = "arm,gic-v3";
> > > > > + #interrupt-cells = <3>;
> > > > > + interrupt-controller;
> > > > > + reg = <0 0x50800000 0 0x10000>,
> > > > > + <0 0x50880000 0 0x200000>,
> > >
> > > That's enough redistributor space for 16 CPUs. However, you only
> > > describe 4. Either the number of CPUs is wrong, the size is wrong, or
> > > the GIC has been configured for more cores than the SoC has.
> >
> > Confirmed the SoC can only find 4 redistributors:
> >
> > localhost:~ # dmesg | grep CPU
> > [ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd034]
> > [ 0.000000] Detected VIPT I-cache on CPU0
> > [ 0.000000] CPU features: detected: GIC system register CPU interface
> > [ 0.000000] CPU features: detected: ARM erratum 845719
> > [ 0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1
> > [ 0.000000] rcu: RCU restricting CPUs from NR_CPUS=480 to nr_cpu_ids=4.
> > [ 0.000000] GICv3: CPU0: found redistributor 0 region 0:0x0000000050880000
> > [ 0.063865] smp: Bringing up secondary CPUs ...
> > [ 0.068852] Detected VIPT I-cache on CPU1
> > [ 0.068894] GICv3: CPU1: found redistributor 1 region 0:0x00000000508a0000
> > [ 0.068963] CPU1: Booted secondary processor 0x0000000001 [0x410fd034]
> > [ 0.069809] Detected VIPT I-cache on CPU2
> > [ 0.069851] GICv3: CPU2: found redistributor 100 region 0:0x00000000508c0000
> > [ 0.069903] CPU2: Booted secondary processor 0x0000000100 [0x410fd034]
> > [ 0.070698] Detected VIPT I-cache on CPU3
> > [ 0.070722] GICv3: CPU3: found redistributor 101 region 0:0x00000000508e0000
> > [ 0.070749] CPU3: Booted secondary processor 0x0000000101 [0x410fd034]
> > [ 0.070847] smp: Brought up 1 node, 4 CPUs
> > <..snip..>
>
> That's not the correct way to find out. Each CPU tries to find its
> matching RD in the region. This doesn't mean there aren't more RDs
> present in the GIC.
>
> You need to iterate over all the RDs in the region until you find one
> that has GICR_TYPER.Last == 1. This will give you the actual count.
> Alternatively, you can check whether the RD at 508e0000 has that bit
> set. If it doesn't, then you know there are more RDs than CPUs.
>
> M.
>

Thanks for your guidance. Not sure if any debug log can be enabled for this
check so I temporarily add an ugly message as below:

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index e0f4debe64e1..5998306fff39 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -866,10 +866,11 @@ static int __gic_populate_rdist(struct redist_region *region, void __iomem *ptr)
gic_data_rdist_rd_base() = ptr;
gic_data_rdist()->phys_base = region->phys_base + offset;

- pr_info("CPU%d: found redistributor %lx region %d:%pa\n",
+ pr_info("CPU%d: found redistributor %lx region %d:%pa last: %d\n",
smp_processor_id(), mpidr,
(int)(region - gic_data.redist_regions),
- &gic_data_rdist()->phys_base);
+ &gic_data_rdist()->phys_base,
+ (typer & GICR_TYPER_LAST) ? 1 : 0);
return 0;
}

The following log shows that the "Last" bit (GICR_TYPER[4]) of RD at
508e0000 has been set.

localhost:~ # dmesg | grep GIC
[ 0.000000] CPU features: detected: GIC system register CPU interface
[ 0.000000] GICv3: 544 SPIs implemented
[ 0.000000] GICv3: 0 Extended SPIs implemented
[ 0.000000] GICv3: Distributor has no Range Selector support
[ 0.000000] GICv3: 16 PPIs implemented
[ 0.000000] GICv3: CPU0: found redistributor 0 region 0:0x0000000050880000 last: 0
[ 0.078745] GICv3: CPU1: found redistributor 1 region 0:0x00000000508a0000 last: 0
[ 0.089598] GICv3: CPU2: found redistributor 100 region 0:0x00000000508c0000 last: 0
[ 0.100395] GICv3: CPU3: found redistributor 101 region 0:0x00000000508e0000 last: 1