Re: [RFC PATCH] ARM: mach-shmobile: Parse DT to get ARCH timer memory region

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Julien Grall <julien.grall@arm.com>
Cc: Oleksandr Tyshchenko <olekstysh@gmail.com>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Simon Horman <horms@verge.net.au>,
	Magnus Damm <magnus.damm@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Subject: Re: [RFC PATCH] ARM: mach-shmobile: Parse DT to get ARCH timer memory region
Date: Mon, 13 May 2019 11:56:03 +0200	[thread overview]
Message-ID: <CAMuHMdUR1jUwyTna5Q6xksvu1xEZKT1HPPVmMvi1YBZ1YmqhDw@mail.gmail.com> (raw)
In-Reply-To: <e64d7f2f-209e-cf7d-6ddc-88d338b1c010@arm.com>

Hi Julien,

On Mon, May 13, 2019 at 11:20 AM Julien Grall <julien.grall@arm.com> wrote:
> On 5/10/19 5:22 PM, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >
> > Don't use hardcoded address, retrieve it from device-tree instead.
> >
> > And besides, this patch fixes the memory error when running
> > on top of Xen hypervisor:
> >
> > (XEN) traps.c:1999:d0v0 HSR=0x93830007 pc=0xc0b097f8 gva=0xf0805000
> >        gpa=0x000000e6080000
> >
> > Which shows that VCPU0 in Dom0 is trying to access an address in memory
> > it is not allowed to access (0x000000e6080000).
> > Put simply, Xen doesn't know that it is a device's register memory
> > since it wasn't described in a host device tree (which Xen parses)
> > and as the result this memory region wasn't assigned to Dom0 at
> > domain creation time.
> >
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >
> > ---
> >
> > This patch is meant to get feedback from the community before
> > proceeding further. If we decide to go this direction, all Gen2
> > device-trees should be updated (add memory region) before
> > this patch going in.
> >
> > e.g. r8a7790.dtsi:
> >
> > ...
> > timer {
> >       compatible = "arm,armv7-timer";
> >       interrupts-extended = <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> >                             <&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> >                             <&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> >                             <&gic GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
> > +      reg = <0 0xe6080000 0 0x1000>;
>
> This looks incorrect, the "arm,armv7-timer" bindings doesn't offer you
> the possibility to specify an MMIO region. This makes sense because it
> is meant to describe the Arch timer that is only access via co-processor
> registers.
>
> Looking at the code, I think the MMIO region corresponds to the
> coresight (based on the register name). So you may want to describe the
> coresight in the Device-Tree.

This is the "counter module", not the ARM v7 timer, cfr. Mark Rutland's
response in an earlier discussion about describing this in DT in
"Re: Architecture Timer on R-Car Gen2"
https://lore.kernel.org/linux-renesas-soc/20170705113335.GE25115@leverpostej/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds