Re: [PATCH] arm64: dts: r8a7795: Increase the size of GIC-400 mapped registers

[Simon Horman <horms@verge.net.au>]

On Wed, May 18, 2016 at 10:00:52AM +0200, Dirk Behme wrote:
> Hi Geert and Simon,
> 
> On 16.05.2016 10:12, Simon Horman wrote:
> >Hi Dirk,
> >
> >On Tue, May 03, 2016 at 07:48:32PM +0200, Dirk Behme wrote:
> >>Hi Simon,
> >>
> >>On 29.04.2016 12:35, Marc Zyngier wrote:
> >>>On Fri, 29 Apr 2016 09:43:45 +1000
> >>>Simon Horman <horms@verge.net.au> wrote:
> >>>
> >>>>[Cc Mark Zyngier, linux-arm-kernel]
> >>>>
> >>>>Hi Dirk,
> >>>>
> >>>>On Thu, Apr 28, 2016 at 07:41:57AM +0200, Dirk Behme wrote:
> >>>>>Hi Simon,
> >>>>>
> >>>>>On 28.04.2016 01:30, Simon Horman wrote:
> >>>>>>Hi Dirk,
> >>>>>>
> >>>>>>I understand that there is an issue here but I'm not yet able
> >>>>>>to convince myself that this is the correct solution.
> >>>>>>
> >>>>>>In revision r0p1 of the CoreLink GIC-400 Generic Interrupt Controller
> >>>>>>Technical Reference Manual[1] I see in Section 3.2. "GIC-400 register map"
> >>>>>>that the size of both the CPU interfaces and Virtual CPU interfaces are
> >>>>>>0x2000 bytes. And assuming that the hardware follows the specification it
> >>>>>>appears that DT is correctly describing the hardware.
> >>>>>
> >>>>>
> >>>>>I think you are missing the details described by ARM in
> >>>>>
> >>>>>http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9
> >>>>>
> >>>>>
> >>>>>Maybe Julien could help if you have some more doubts?
> >>>>
> >>>>I guess I am confused.
> >>>>
> >>>>I see that there is now handling of the case where the region size is
> >>>>128Kbytes. But I'm still not seeing the bit which describes that the
> >>>>GIC-400  has a region size of 128Kbytes. Perhaps the later is somehow
> >>>>implied by the former. Or perhaps I need to check with the hw team.
> >>>
> >>>Please have a look at the SBSA document, and in particular the
> >>>Appendix-F (registration and selling your soul required - only kidding):
> >>>
> >>>http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029/index.html
> >>>
> >>>This requires that, in order for the two halves of GICV to be trappable
> >>>*separately* by a hypervisor using 64kB pages at Stage-2, the two 4kB
> >>>pages that describe that region are aliased as such:
> >>>- the first 4kB page is aliased 16 times over a 64kB region
> >>>- the second 4kB page is aliased 16 times over another contiguous 64kB
> >>>  region
> >>>
> >>>This means that your GIC is indeed covering a 128kB region, with the
> >>>mapping corresponding to the GICv2 memory map located at offset 0xf000
> >>>from the base of that 128kB region. Also, this GICV requirement also
> >>>applies to GICC (most likely because the two regions use the same
> >>>decoding logic).
> >>>
> >>>The OS must of course be aware of this (see gic_check_eoimode in the
> >>>GIC driver). Of course, almost nobody got that right (I only know of
> >>>the APM Xgene-1 so far). If you actually did, great!
> >>>
> >>>Also, the ACPI spec fails to recognize this by not providing the length
> >>>of the region, meaning that those who got it right with DT are likely
> >>>to get it wrong with ACPI, and vice-versa.
> >>>
> >>>It's a wonderful world.
> >>
> >>
> >>Could this patch be applied, then?
> >
> >Yes I have now done so :)
> 
> 
> In parallel, a r8a7796.dtsi was added where this change is missing, now:
> 
> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/tree/arch/arm64/boot/dts/renesas/r8a7796.dtsi?h=topic/gen3-latest
> 
> Do you need a new patch to add this to the r8a7796.dtsi, too? Or could you
> apply
> 
> https://git.kernel.org/cgit/linux/kernel/git/geert/renesas-drivers.git/commit/arch/arm64/boot/dts/renesas?h=topic/gen3-latest&id=f7c7e97097915643a86051fe557b41a87c837172
> 
> to the r8a7796.dtsi, too?

Hi Dirk,

The copy of r8a7796.dtsi in that tree is from a topic branch and
it is not targeted at upstream in that form.

I am working on patches to add r8a7796.dtsi amongst other things
and will include this change there.