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

[Chester Lin <clin@suse.com>]

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