Re: [PATCH 4/5] arm64: dts: ti: Introduce base support for AM62x SoC

[Marc Zyngier]

On Fri, 11 Feb 2022 23:55:13 +0000,
Nishanth Menon <nm@xxxxxx> wrote:
> 
> On 11:33-20220211, Marc Zyngier wrote:
> > On Thu, 10 Feb 2022 19:34:59 +0000,
> > Nishanth Menon <nm@xxxxxx> wrote:
> > > 
> > > On 19:10-20220209, Marc Zyngier wrote:
> > > [...]
> > > 
> > > > > +&cbass_main {
> > > > > +	gic500: interrupt-controller@1800000 {
> > > > > +		compatible = "arm,gic-v3";
> > > > > +		#address-cells = <2>;
> > > > > +		#size-cells = <2>;
> > > > > +		ranges;
> > > > > +		#interrupt-cells = <3>;
> > > > > +		interrupt-controller;
> > > > > +		reg = <0x00 0x01800000 0x00 0x10000>,	/* GICD */
> > > > > +		      <0x00 0x01880000 0x00 0xC0000>;	/* GICR */
> > > > 
> > > > Usual rant: you are missing the GICC, GICH and GICV regions
> > > > that are implemented by the CPU. Cortex-A53 implements them
> > > > (they are not optional), so please describe them.
> > > > 
> > > 
> > > 
> > > -ECONFUSED. TRM for GIC500 refers to just GICD, GICR and ITS range[1].
> > 
> > And I'm not talking about the GIC, but of the CPU interface. The fact
> > that we describe both in the GIC binding doesn't mean they are
> > implemented by the same IP block (and the architecture is quite clear
> > about that).
> > 
> > > Same thing is indicated by Generic Interrupt Controller Architecture
> > > Specification[2] See table 1-1 (page 23).
> > > 
> > > I think you are expecting GICV3's backward compatibility mode (Table 1-2
> > > in page 24), But in K3 architecture, are_option meant for backward
> > > compatibility is set to true (aka no backward compatibility). I think
> > > this did popup sometime back as well (first k3 SoC)[3]. I think the more
> > > clearer description is available in [4].
> > 
> > No, this description is for the architecture as a whole. ARE being
> > disabled *int the GIC* doesn't mean it is disabled overall, and the
> > CPU is free to implement the CPU interface by any mean it wants as
> > long as it communicates with the GIC using the Stream Protocol.
> > Cortex-A32, A34, 35, A53, A57, A72 and A73 all implement both the
> > sysreg and MMIO CPU interfaces. Later ARM CPUs don't. Both can work
> > with GIC500.
> > 
> > > I believe the argumentation that GICC/H/V is mandatory for A53 if GIC500
> > > is used is not accurate. Please correct me if I am mistaken.
> > 
> > GIC500 is not involved at all, and A53 always implements both the
> > system register and MMIO interfaces. See the A53 TRM, chapter 9. The
> > only way to disable this interface is to assert GICCDISABLE, which
> > disables the whole of the CPU interface. Given that you have a (more
> > or less) functional system, it probably isn't the case.
> > 
> > See Table 9-1, which tells you where these registers are as an offset
> > from PERIPHBASE. Dumping these registers should show you that they are
> > indeed implemented and not solely a figment of my own imagination.
> 
> Thanks for explaining.. I don't see this is working in practise.. Let me
> know if I am making a mistake in my interpretation.
> 
> Quote from our internal integration spec (yep it leaves it to ARM cluster's
> use):
> ""
> Note: GIC periphery base tieoff to ARM corepacs for GIC v2 compatibility
> requires a dedicated unallocated space to be passed as input to ARM corepac.
> The CC internal region 0F00_0000-0x0F03_FFFF is assigned as GIC periphery
> base tieoff to the corepac.
> When GIC-500 is in v3 mode, and A72 with GICCDISABLE=0 and PERIPHBASE set:
> - the CPU interface registers are accessed via ICC* system register.
> - the GICC* regions (PERIPHBASE - PERIPHBASE+0x3FFFF) are reserved
>   and access will be Read as Zero / Write Ignored.
> So any writes/reads to this region would be trapped by ARM corepacs.
> ""

Not sure what the 'corepacs' are (the CPU cluster?). But what I
understand is that accesses to the GIC regions are kept internal to
the 'corepacs', which is exactly what is expected.

> 
> Anyways, Here is my report. I checked across all K3 devices (a72 and
> a53)
> AM65x: PERIPH_BASE = 0x6f000000 (a53)
> j721e: PERIPH_BASE = 0x6f000000 (a72)
> J7200: PERIPH_BASE = 0x6f000000 (a72)
> j721s2: PERIPH_BASE = 0x6f000000 (a72)
> AM64: PERIPH_BASE = 0x100000000 (a53)
> AM62: PERIPH_BASE = 0x100000000 (a53)
> 
> (side note: am64/62 needed the 0x6f.. address space for PCIe stuff.. but
> the address chosen has nothing in SoC fabric)
> 
> Tested at u-boot shell prompt (running at EL2):
> 
> If I understood the expectation correctly..I should be seeing offsets
> off [1]. Taking 'CPU Interface'/GICC as an example, [2] should be the
> registers I should be seeing. aka, at offset 0xfc from PERIPHBASE, i
> should see 0x0034443B.

If ICC_SRE_EL3.SRE is 1, this is more or less expected. You can only
use one or the other at any given time, not both.

The more important thing is that GICV is what we give to a VM running
in compat mode. With HCR_EL2.{AMO,FMO,IMO}={1,1,1} and
ICC_SRE_EL1.SRE==0, the guest can access a MMIO virtual GIC interface,
and the hypervisor does its magic.

> 
> Note: on K3 devices (in the 32bit address space), as in the
> description above, we have a null endpoint handler in the bus fabric
> that responds with 0x0 for read requests for invalid/reserved addresses.
> 
> What I see is 0x0 (and not IIDR) in all the address ranges - which matches ARM
> sending that region requests straight down to SoC level and SoC
> returning "ignore"..
> 
> On AM62, I attached Lauterbach. and tried to look at the addresses: [3]
> from cpu view and from bus view.
> 
> I also checked from kernel side with devmem to make sure to dump while
> kernel GICV3 is active.. I see the same thing as well..
> 
> Is there something TFA or someone has to do to "enable" this? I tried
> re-reading porting-guide.rst yet again to make sure we have'nt missed
> anything.

I expect the SRE settings to control all of this, most of which are
under NS control. You could easily check this by advertising the 3
missing regions in DT, booting an upstream kernel with KVM and boot a
GICv2 guest. KVM will also warn if the DT regions are advertised but
the HW doesn't actually support the MMIO accesses. Feel free to ping
me offline if you need the runs for this,

	M.

-- 
Without deviation from the norm, progress is not possible.