| Message ID | 20210805065429.27485-5-clin@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: initial NXP S32G2 support | expand |
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 Note: This DT is for running on the Cortex-A53 cores, but S32G2 also has Cortex-M7 cores. For Vybrid SoCs, DTs later got contributed to also run on its Cortex-M4 core: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/vf610.dtsi https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/vf500.dtsi vs. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/vf610m4.dtsi Should we plan for this in our file naming here and in following patches (e.g., s32g2-a53* vs. s32g2-m7*)? To me, a later concatenation of s32g274am7* would look awkward, and s32g274a-m7* would sort between -evb and -rdb2. > @@ -0,0 +1,98 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT > +/* * NXP S32G2 SoC family * ? @NXP: Are any models other than 274A in the queue that we should distinguish between s32g2.dtsi and s32g274a.dtsi here already? > + * Copyright (c) 2021 SUSE LLC > + */ > + > +#include <dt-bindings/interrupt-controller/arm-gic.h> > + > +/ { > + compatible = "fsl,s32g2"; > + interrupt-parent = <&gic>; > + #address-cells = <2>; > + #size-cells = <2>; > + > + cpus { > + #address-cells = <1>; > + #size-cells = <0>; > + > + cpu0: cpu@0 { > + device_type = "cpu"; > + compatible = "arm,cortex-a53"; > + reg = <0x0>; > + enable-method = "psci"; > + next-level-cache = <&cluster0_l2>; > + }; > + > + cpu1: cpu@1 { > + device_type = "cpu"; > + compatible = "arm,cortex-a53"; > + reg = <0x1>; > + enable-method = "psci"; > + next-level-cache = <&cluster0_l2>; > + }; > + > + cpu2: cpu@100 { > + device_type = "cpu"; > + compatible = "arm,cortex-a53"; > + reg = <0x100>; > + enable-method = "psci"; > + next-level-cache = <&cluster1_l2>; > + }; > + > + cpu3: cpu@101 { > + device_type = "cpu"; > + compatible = "arm,cortex-a53"; > + reg = <0x101>; > + enable-method = "psci"; > + next-level-cache = <&cluster1_l2>; > + }; > + > + cluster0_l2: l2-cache0 { > + compatible = "cache"; > + }; > + > + cluster1_l2: l2-cache1 { > + compatible = "cache"; > + }; > + }; > + > + pmu { > + compatible = "arm,cortex-a53-pmu"; > + interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>; interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>; > + }; > + > + timer { > + compatible = "arm,armv8-timer"; > + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>, > + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>, > + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>, > + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>; > + }; > + > + psci { > + compatible = "arm,psci-1.0"; > + method = "smc"; > + }; Should we move this into a /firmware node, to group with future OP-TEE? > + > + soc { > + compatible = "simple-bus"; > + interrupt-parent = <&gic>; Duplicate, already set on root node. > + #address-cells = <2>; > + #size-cells = <2>; Why? Does it have any peripherals that go beyond 32-bit space? For 64-bit Realtek platforms Rob had asked me to use 1, if possible. I do understand that for /memory nodes we do have high-memory addresses, so 2 for the root node looks correct. > + Please drop this white line. > + ranges; According to Rob, the /soc ranges should exclude any RAM ranges for safety reasons. Compare: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/realtek/rtd129x.dtsi If you're lacking the maximum RAM areas to carve out, NXP is in CC to help out :) and the EVB and RDB2 boards should give you starting numbers that could be enlarged later if needed. > + > + gic: interrupt-controller@50800000 { > + compatible = "arm,gic-v3"; > + #interrupt-cells = <3>; > + interrupt-controller; > + reg = <0 0x50800000 0 0x10000>, > + <0 0x50880000 0 0x200000>, > + <0 0x50400000 0 0x2000>, > + <0 0x50410000 0 0x2000>, > + <0 0x50420000 0 0x2000>; Please order reg after compatible by convention, and sort interrupt-controller or at least #interrupt-cells (applying to consumers) last, after the below one applying to this device itself. > + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | > + IRQ_TYPE_LEVEL_HIGH)>; > + }; CC'ing Marc for additional GIC scrutiny, often the sizes are wrong. > + }; > +}; Thanks, Andreas
On Thu, Aug 12, 2021 at 07:26:28PM +0200, Andreas Färber 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 > > Note: This DT is for running on the Cortex-A53 cores, but S32G2 also has > Cortex-M7 cores. For Vybrid SoCs, DTs later got contributed to also run > on its Cortex-M4 core: > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/vf610.dtsi > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/vf500.dtsi > vs. > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/vf610m4.dtsi > > Should we plan for this in our file naming here and in following patches > (e.g., s32g2-a53* vs. s32g2-m7*)? To me, a later concatenation of > s32g274am7* would look awkward, and s32g274a-m7* would sort between -evb > and -rdb2. > > > @@ -0,0 +1,98 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT > > +/* > > * NXP S32G2 SoC family > * > ? Will add it. > > @NXP: Are any models other than 274A in the queue that we should > distinguish between s32g2.dtsi and s32g274a.dtsi here already? > > > + * Copyright (c) 2021 SUSE LLC > > + */ > > + > > +#include <dt-bindings/interrupt-controller/arm-gic.h> > > + > > +/ { > > + compatible = "fsl,s32g2"; > > + interrupt-parent = <&gic>; > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + cpus { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + cpu0: cpu@0 { > > + device_type = "cpu"; > > + compatible = "arm,cortex-a53"; > > + reg = <0x0>; > > + enable-method = "psci"; > > + next-level-cache = <&cluster0_l2>; > > + }; > > + > > + cpu1: cpu@1 { > > + device_type = "cpu"; > > + compatible = "arm,cortex-a53"; > > + reg = <0x1>; > > + enable-method = "psci"; > > + next-level-cache = <&cluster0_l2>; > > + }; > > + > > + cpu2: cpu@100 { > > + device_type = "cpu"; > > + compatible = "arm,cortex-a53"; > > + reg = <0x100>; > > + enable-method = "psci"; > > + next-level-cache = <&cluster1_l2>; > > + }; > > + > > + cpu3: cpu@101 { > > + device_type = "cpu"; > > + compatible = "arm,cortex-a53"; > > + reg = <0x101>; > > + enable-method = "psci"; > > + next-level-cache = <&cluster1_l2>; > > + }; > > + > > + cluster0_l2: l2-cache0 { > > + compatible = "cache"; > > + }; > > + > > + cluster1_l2: l2-cache1 { > > + compatible = "cache"; > > + }; > > + }; > > + > > + pmu { > > + compatible = "arm,cortex-a53-pmu"; > > + interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>; > > interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>; Actually I traced the pmu_parse_irqs() and found that this SoC never falls into the pmu_has_irq_affinity() check because it's a percpu IRQ so the flow ends at pmu_parse_percpu_irq(). But it looks good to me to have an interrupt-affinity to indicate that each core has an associated PPI for PMU events as the binding file has suggested. > > > + }; > > + > > + timer { > > + compatible = "arm,armv8-timer"; > > + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>, > > + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>, > > + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>, > > + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>; > > + }; > > + > > + psci { > > + compatible = "arm,psci-1.0"; > > + method = "smc"; > > + }; > > Should we move this into a /firmware node, to group with future OP-TEE? > So far I can only see a few examples [e.g. ti/k3-*] which add the psci node into /firmware but logically we should do it. > > + > > + soc { > > + compatible = "simple-bus"; > > + interrupt-parent = <&gic>; > > Duplicate, already set on root node. Will remove it. > > > + #address-cells = <2>; > > + #size-cells = <2>; > > Why? Does it have any peripherals that go beyond 32-bit space? > For 64-bit Realtek platforms Rob had asked me to use 1, if possible. > I do understand that for /memory nodes we do have high-memory addresses, > so 2 for the root node looks correct. Actually it's a limitation due to [PATCH 7/8] "arm64: dts: s32g2: add memory nodes for evb and rdb2", which adds memory nodes to indicate maximum system RAM size combined by two separated memory banks, and the second bank starts from the offset 0x880000000 so that's why we need 64-bit address space here. Please feel free to let me know if any suggestion. > > > + > > Please drop this white line. > Will do. > > + ranges; > > According to Rob, the /soc ranges should exclude any RAM ranges for > safety reasons. Compare: > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/realtek/rtd129x.dtsi > > If you're lacking the maximum RAM areas to carve out, NXP is in CC to > help out :) and the EVB and RDB2 boards should give you starting numbers > that could be enlarged later if needed. I added memory nodes in [PATCH 7/8] "arm64: dts: s32g2: add memory nodes for evb and rdb2" to describe maximum RAM areas, which are based on the information we found in NXP BSP and boards. @NXP: Please feel free to correct me if anything wrong, thanks. > > > + > > + gic: interrupt-controller@50800000 { > > + compatible = "arm,gic-v3"; > > + #interrupt-cells = <3>; > > + interrupt-controller; > > + reg = <0 0x50800000 0 0x10000>, > > + <0 0x50880000 0 0x200000>, > > + <0 0x50400000 0 0x2000>, > > + <0 0x50410000 0 0x2000>, > > + <0 0x50420000 0 0x2000>; > > Please order reg after compatible by convention, and sort > interrupt-controller or at least #interrupt-cells (applying to > consumers) last, after the below one applying to this device itself. > Will do. > > + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | > > + IRQ_TYPE_LEVEL_HIGH)>; > > + }; > > CC'ing Marc for additional GIC scrutiny, often the sizes are wrong. > IIRC, gic-v3 shouldn't have this kind of interrupt specifier. It's my fault and will fix it. Feel free to let me know if any suggestions. > > + }; > > +}; > > Thanks, > Andreas > > -- > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer > HRB 36809 (AG Nürnberg) >
Hi Chester, On 13.08.21 05:28, Chester Lin wrote: > On Thu, Aug 12, 2021 at 07:26:28PM +0200, Andreas Färber wrote: >> 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 >> >> Note: This DT is for running on the Cortex-A53 cores, but S32G2 also has >> Cortex-M7 cores. For Vybrid SoCs, DTs later got contributed to also run >> on its Cortex-M4 core: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/vf610.dtsi >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/vf500.dtsi >> vs. >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/vf610m4.dtsi >> >> Should we plan for this in our file naming here and in following patches >> (e.g., s32g2-a53* vs. s32g2-m7*)? To me, a later concatenation of >> s32g274am7* would look awkward, and s32g274a-m7* would sort between -evb >> and -rdb2. >> >>> @@ -0,0 +1,98 @@ >>> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT >>> +/* >> >> * NXP S32G2 SoC family >> * >> ? > > Will add it. > >> >> @NXP: Are any models other than 274A in the queue that we should >> distinguish between s32g2.dtsi and s32g274a.dtsi here already? >> >>> + * Copyright (c) 2021 SUSE LLC >>> + */ >>> + >>> +#include <dt-bindings/interrupt-controller/arm-gic.h> >>> + >>> +/ { >>> + compatible = "fsl,s32g2"; >>> + interrupt-parent = <&gic>; >>> + #address-cells = <2>; >>> + #size-cells = <2>; [...] >>> + pmu { >>> + compatible = "arm,cortex-a53-pmu"; >>> + interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>; >> >> interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>; > > Actually I traced the pmu_parse_irqs() and found that this SoC never falls into > the pmu_has_irq_affinity() check because it's a percpu IRQ so the flow ends at > pmu_parse_percpu_irq(). But it looks good to me to have an interrupt-affinity > to indicate that each core has an associated PPI for PMU events as the binding > file has suggested. My comment was based on the DT binding requesting it also for PPI and previous review requests for me to add it elsewhere, so it's quite possible that your code analysis is correct. FWIW also the cache nodes were not evaluated last time I checked, although the specs do show those nodes and properties. https://github.com/devicetree-org/devicetree-specification/blob/v0.3/source/devicenodes.rst >>> + }; [...] >>> + >>> + soc { >>> + compatible = "simple-bus"; [...] >>> + #address-cells = <2>; >>> + #size-cells = <2>; >> >> Why? Does it have any peripherals that go beyond 32-bit space? >> For 64-bit Realtek platforms Rob had asked me to use 1, if possible. >> I do understand that for /memory nodes we do have high-memory addresses, >> so 2 for the root node looks correct. > > Actually it's a limitation due to [PATCH 7/8] "arm64: dts: s32g2: add memory > nodes for evb and rdb2", which adds memory nodes to indicate maximum system > RAM size combined by two separated memory banks, and the second bank starts > from the offset 0x880000000 so that's why we need 64-bit address space here. > Please feel free to let me know if any suggestion. The /memory nodes are on the root node and thus only use #address-cells and #size-cells from /, not from /soc here. Thus the only criterium is whether something within /soc needs it. PCI controllers may have 3 address cells, SPI/I2C controllers 1, so you can have multiple sizes within one DT, you just need suitable ranges wherever mappings to the parent address space are needed. (I thought you had that distinction in an earlier draft I reviewed.) Similarly, /reserved-memory would be outside the /soc node and use the root node's #(address|size)-cells. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt Note: I'm assuming that if someone were to add a DT for the Cortex-M7s, the address space mapping of any shared peripherals will likely differ, as would the interrupts obviously, so I assume no direct .dtsi sharing of /soc sub-nodes could sensibly be applied here. @NXP: Please correct me if I'm wrong in either M7 not having access to DDR RAM or large enough SRAM for Linux or another DT-consuming OS (and this line of thinking not being applicable then) or you having use cases to factor out some of the /soc sub-nodes for sharing between the two (which would then give another reason for #address-/size-cells = <1> for 32-bit Cortex-M7). [...] >>> + ranges; >> >> According to Rob, the /soc ranges should exclude any RAM ranges for >> safety reasons. Compare: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/realtek/rtd129x.dtsi >> >> If you're lacking the maximum RAM areas to carve out, NXP is in CC to >> help out :) and the EVB and RDB2 boards should give you starting numbers >> that could be enlarged later if needed. > > I added memory nodes in [PATCH 7/8] "arm64: dts: s32g2: add memory nodes for > evb and rdb2" to describe maximum RAM areas, which are based on the information > we found in NXP BSP and boards. Yes, as discussed in 7/8, you should be using here: ranges = <0x0 0 0x0 0x80000000>; /* excluding 4 GiB RAM */ https://github.com/devicetree-org/devicetree-specification/blob/v0.3/source/devicetree-basics.rst > @NXP: Please feel free to correct me if anything wrong, thanks. > >> >>> + >>> + gic: interrupt-controller@50800000 { >>> + compatible = "arm,gic-v3"; >>> + #interrupt-cells = <3>; >>> + interrupt-controller; >>> + reg = <0 0x50800000 0 0x10000>, >>> + <0 0x50880000 0 0x200000>, >>> + <0 0x50400000 0 0x2000>, >>> + <0 0x50410000 0 0x2000>, >>> + <0 0x50420000 0 0x2000>; >> >> Please order reg after compatible by convention, and sort >> interrupt-controller or at least #interrupt-cells (applying to >> consumers) last, after the below one applying to this device itself. >> > > Will do. > >>> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | >>> + IRQ_TYPE_LEVEL_HIGH)>; >>> + }; >> >> CC'ing Marc for additional GIC scrutiny, often the sizes are wrong. >> > > IIRC, gic-v3 shouldn't have this kind of interrupt specifier. It's my fault and > will fix it. Feel free to let me know if any suggestions. You probably mean the GIC_CPU_MASK_SIMPLE()? interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>; https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml The interrupts property will be used by KVM (or possibly Xen) for vGIC. Please ensure that the Kconfig you use for testing has KVM enabled. >>> + }; >>> +}; Cheers, Andreas
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. > > + <0 0x50400000 0 0x2000>, > > + <0 0x50410000 0 0x2000>, > > + <0 0x50420000 0 0x2000>; > > Please order reg after compatible by convention, and sort > interrupt-controller or at least #interrupt-cells (applying to > consumers) last, after the below one applying to this device itself. > > > + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | > > + IRQ_TYPE_LEVEL_HIGH)>; > > + }; > > CC'ing Marc for additional GIC scrutiny, often the sizes are wrong. There is more than just sizes. The interrupt specifier for the maintenance interrupt is also wrong. M.
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..> I will correct the size to 0x80000, thanks! > > > > + <0 0x50400000 0 0x2000>, > > > + <0 0x50410000 0 0x2000>, > > > + <0 0x50420000 0 0x2000>; > > > > Please order reg after compatible by convention, and sort > > interrupt-controller or at least #interrupt-cells (applying to > > consumers) last, after the below one applying to this device itself. > > > > > + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | > > > + IRQ_TYPE_LEVEL_HIGH)>; > > > + }; > > > > CC'ing Marc for additional GIC scrutiny, often the sizes are wrong. > > There is more than just sizes. The interrupt specifier for the > maintenance interrupt is also wrong. > > M. I will remove the wrong interrupt specifier. Thanks! Chester. > > -- > Without deviation from the norm, progress is not possible. >
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.
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
On Sat, 21 Aug 2021 13:39:04 +0100, Chester Lin <clin@suse.com> wrote: > > 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 Looks convincing enough. Trimming the RD range to 512kB is then the right thing to do. Thanks, M.
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 @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT +/* + * Copyright (c) 2021 SUSE LLC + */ + +#include <dt-bindings/interrupt-controller/arm-gic.h> + +/ { + compatible = "fsl,s32g2"; + interrupt-parent = <&gic>; + #address-cells = <2>; + #size-cells = <2>; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + + cpu0: cpu@0 { + device_type = "cpu"; + compatible = "arm,cortex-a53"; + reg = <0x0>; + enable-method = "psci"; + next-level-cache = <&cluster0_l2>; + }; + + cpu1: cpu@1 { + device_type = "cpu"; + compatible = "arm,cortex-a53"; + reg = <0x1>; + enable-method = "psci"; + next-level-cache = <&cluster0_l2>; + }; + + cpu2: cpu@100 { + device_type = "cpu"; + compatible = "arm,cortex-a53"; + reg = <0x100>; + enable-method = "psci"; + next-level-cache = <&cluster1_l2>; + }; + + cpu3: cpu@101 { + device_type = "cpu"; + compatible = "arm,cortex-a53"; + reg = <0x101>; + enable-method = "psci"; + next-level-cache = <&cluster1_l2>; + }; + + cluster0_l2: l2-cache0 { + compatible = "cache"; + }; + + cluster1_l2: l2-cache1 { + compatible = "cache"; + }; + }; + + pmu { + compatible = "arm,cortex-a53-pmu"; + interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>; + }; + + timer { + compatible = "arm,armv8-timer"; + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>, + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>, + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>, + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>; + }; + + psci { + compatible = "arm,psci-1.0"; + method = "smc"; + }; + + soc { + compatible = "simple-bus"; + interrupt-parent = <&gic>; + #address-cells = <2>; + #size-cells = <2>; + + ranges; + + gic: interrupt-controller@50800000 { + compatible = "arm,gic-v3"; + #interrupt-cells = <3>; + interrupt-controller; + reg = <0 0x50800000 0 0x10000>, + <0 0x50880000 0 0x200000>, + <0 0x50400000 0 0x2000>, + <0 0x50410000 0 0x2000>, + <0 0x50420000 0 0x2000>; + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | + IRQ_TYPE_LEVEL_HIGH)>; + }; + }; +};
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