| Message ID | 1461047395-6532-1-git-send-email-dirk.behme@de.bosch.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | f7c7e97097915643a86051fe557b41a87c837172 |
| Delegated to: | Simon Horman |
| Headers | show |
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. [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0471b/index.html On Tue, Apr 19, 2016 at 08:29:55AM +0200, Dirk Behme wrote: > From: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > > There are some requirements about the GIC-400 memory layout and its > mapping if using 64k aligned base addresses like on r8a7795. > > See e.g. > > http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9 > > Map the whole memory range instead of only 0x2000. This will fix > the issue that some hypervisors, e.g. Xen, fail to handle the > interrupts correctly. > > Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> > --- > Note: This patch is against renesas-drivers-2016-04-12-v4.6-rc3 > > arch/arm64/boot/dts/renesas/r8a7795.dtsi | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > index 8be9424..d880fd4 100644 > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > @@ -160,9 +160,9 @@ > #address-cells = <0>; > interrupt-controller; > reg = <0x0 0xf1010000 0 0x1000>, > - <0x0 0xf1020000 0 0x2000>, > + <0x0 0xf1020000 0 0x20000>, > <0x0 0xf1040000 0 0x20000>, > - <0x0 0xf1060000 0 0x2000>; > + <0x0 0xf1060000 0 0x20000>; > interrupts = <GIC_PPI 9 > (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; > }; > -- > 2.8.0 >
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? Best regards Dirk > [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0471b/index.html > > On Tue, Apr 19, 2016 at 08:29:55AM +0200, Dirk Behme wrote: >> From: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> >> >> There are some requirements about the GIC-400 memory layout and its >> mapping if using 64k aligned base addresses like on r8a7795. >> >> See e.g. >> >> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9 >> >> Map the whole memory range instead of only 0x2000. This will fix >> the issue that some hypervisors, e.g. Xen, fail to handle the >> interrupts correctly. >> >> Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> >> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> >> --- >> Note: This patch is against renesas-drivers-2016-04-12-v4.6-rc3 >> >> arch/arm64/boot/dts/renesas/r8a7795.dtsi | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi >> index 8be9424..d880fd4 100644 >> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi >> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi >> @@ -160,9 +160,9 @@ >> #address-cells = <0>; >> interrupt-controller; >> reg = <0x0 0xf1010000 0 0x1000>, >> - <0x0 0xf1020000 0 0x2000>, >> + <0x0 0xf1020000 0 0x20000>, >> <0x0 0xf1040000 0 0x20000>, >> - <0x0 0xf1060000 0 0x2000>; >> + <0x0 0xf1060000 0 0x20000>; >> interrupts = <GIC_PPI 9 >> (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; >> }; >> -- >> 2.8.0 >> >
[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. > Best regards > > Dirk > > > >[1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0471b/index.html > > > >On Tue, Apr 19, 2016 at 08:29:55AM +0200, Dirk Behme wrote: > >>From: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > >> > >>There are some requirements about the GIC-400 memory layout and its > >>mapping if using 64k aligned base addresses like on r8a7795. > >> > >>See e.g. > >> > >>http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9 > >> > >>Map the whole memory range instead of only 0x2000. This will fix > >>the issue that some hypervisors, e.g. Xen, fail to handle the > >>interrupts correctly. > >> > >>Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > >>Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> > >>--- > >>Note: This patch is against renesas-drivers-2016-04-12-v4.6-rc3 > >> > >> arch/arm64/boot/dts/renesas/r8a7795.dtsi | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >>diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > >>index 8be9424..d880fd4 100644 > >>--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi > >>+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > >>@@ -160,9 +160,9 @@ > >> #address-cells = <0>; > >> interrupt-controller; > >> reg = <0x0 0xf1010000 0 0x1000>, > >>- <0x0 0xf1020000 0 0x2000>, > >>+ <0x0 0xf1020000 0 0x20000>, > >> <0x0 0xf1040000 0 0x20000>, > >>- <0x0 0xf1060000 0 0x2000>; > >>+ <0x0 0xf1060000 0 0x20000>; > >> interrupts = <GIC_PPI 9 > >> (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; > >> }; > >>-- > >>2.8.0 > >> > >
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. M.
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? Best regards Dirk
CC Marc, lakml On Tue, Apr 19, 2016 at 8:29 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote: > From: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > > There are some requirements about the GIC-400 memory layout and its > mapping if using 64k aligned base addresses like on r8a7795. > > See e.g. > > http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9 > > Map the whole memory range instead of only 0x2000. This will fix > the issue that some hypervisors, e.g. Xen, fail to handle the > interrupts correctly. > > Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> Based on my understanding below Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > Note: This patch is against renesas-drivers-2016-04-12-v4.6-rc3 > > arch/arm64/boot/dts/renesas/r8a7795.dtsi | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > index 8be9424..d880fd4 100644 > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > @@ -160,9 +160,9 @@ > #address-cells = <0>; > interrupt-controller; > reg = <0x0 0xf1010000 0 0x1000>, > - <0x0 0xf1020000 0 0x2000>, > + <0x0 0xf1020000 0 0x20000>, > <0x0 0xf1040000 0 0x20000>, > - <0x0 0xf1060000 0 0x2000>; > + <0x0 0xf1060000 0 0x20000>; > interrupts = <GIC_PPI 9 > (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; > }; Region 0: 4 KiB-pages 0xf1011000-0xf101ffff are aliased to 0xf1010000-0xf1010fff, but we need the first 4 KiB only. Region 1: 4 KiB-pages 0xf1021000-0xf102ffff are aliased to 0xf1020000-0xf1020fff, 4 KiB-pages 0xf1030000-0xf103ffff are all zeroes, probably due to non-secure mode? Region 2: 4 KiB-pages 0xf1041000-0xf104ffff are aliased to 0xf1040000-0xf1040fff, 4 KiB-pages 0xf1050000-0xf105ffff are all zeroes, probably due to non-secure mode? Region 3: 4 KiB-pages 0xf1061000-0xf106ffff are aliased to 0xf1060000-0xf1060fff, 4 KiB-pages 0xf1070000-0xf107ffff are all zeroes, probably due to non-secure mode? Region 2 already had a 128 KiB size before, which allowed to use 8 KiB at 0xf104f000. An 8 KiB size for regions 1 and 3 indeed didn't make much sense, as this covered two identical (aliased) 4 KiB pages, instead of two different pages at offset 0xf000. 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
On 10/05/16 14:33, Geert Uytterhoeven wrote: > CC Marc, lakml > > On Tue, Apr 19, 2016 at 8:29 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote: >> From: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> >> >> There are some requirements about the GIC-400 memory layout and its >> mapping if using 64k aligned base addresses like on r8a7795. >> >> See e.g. >> >> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9 >> >> Map the whole memory range instead of only 0x2000. This will fix >> the issue that some hypervisors, e.g. Xen, fail to handle the >> interrupts correctly. >> >> Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> >> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> > > Based on my understanding below > Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> > >> --- >> Note: This patch is against renesas-drivers-2016-04-12-v4.6-rc3 >> >> arch/arm64/boot/dts/renesas/r8a7795.dtsi | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi >> index 8be9424..d880fd4 100644 >> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi >> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi >> @@ -160,9 +160,9 @@ >> #address-cells = <0>; >> interrupt-controller; >> reg = <0x0 0xf1010000 0 0x1000>, >> - <0x0 0xf1020000 0 0x2000>, >> + <0x0 0xf1020000 0 0x20000>, >> <0x0 0xf1040000 0 0x20000>, >> - <0x0 0xf1060000 0 0x2000>; >> + <0x0 0xf1060000 0 0x20000>; >> interrupts = <GIC_PPI 9 >> (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; >> }; > > Region 0: > 4 KiB-pages 0xf1011000-0xf101ffff are aliased to 0xf1010000-0xf1010fff, > but we need the first 4 KiB only. > > Region 1: > 4 KiB-pages 0xf1021000-0xf102ffff are aliased to 0xf1020000-0xf1020fff, > 4 KiB-pages 0xf1030000-0xf103ffff are all zeroes, probably due to > non-secure mode? No. This 4kB page only contain a single register (GICC_DIR), which is WO/RAZ. > > Region 2: > 4 KiB-pages 0xf1041000-0xf104ffff are aliased to 0xf1040000-0xf1040fff, > 4 KiB-pages 0xf1050000-0xf105ffff are all zeroes, probably due to > non-secure mode? Neither. The aliases are an unused feature of GIC400 exposing the other CPUs view of the same registers... > > Region 3: > 4 KiB-pages 0xf1061000-0xf106ffff are aliased to 0xf1060000-0xf1060fff, > 4 KiB-pages 0xf1070000-0xf107ffff are all zeroes, probably due to > non-secure mode? Same as region 1. > > Region 2 already had a 128 KiB size before, which allowed to use 8 KiB at > 0xf104f000. No. This region (GICH) only needs the first 256 bytes or so. The rest is either RAZ/WI or useless stuff. > > An 8 KiB size for regions 1 and 3 indeed didn't make much sense, as this > covered two identical (aliased) 4 KiB pages, instead of two different pages > at offset 0xf000. While we're at it, adding a pointer to the documentation (GIC400 and SBSA) would be tremendously useful, as it'd avoid misinterpreting the various bits. Thanks, M.
On 10.05.2016 16:17, Marc Zyngier wrote: > On 10/05/16 14:33, Geert Uytterhoeven wrote: >> CC Marc, lakml >> >> On Tue, Apr 19, 2016 at 8:29 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote: >>> From: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> >>> >>> There are some requirements about the GIC-400 memory layout and its >>> mapping if using 64k aligned base addresses like on r8a7795. >>> >>> See e.g. >>> >>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9 >>> >>> Map the whole memory range instead of only 0x2000. This will fix >>> the issue that some hypervisors, e.g. Xen, fail to handle the >>> interrupts correctly. >>> >>> Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> >>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> >> >> Based on my understanding below >> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> >> >>> --- >>> Note: This patch is against renesas-drivers-2016-04-12-v4.6-rc3 >>> >>> arch/arm64/boot/dts/renesas/r8a7795.dtsi | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi >>> index 8be9424..d880fd4 100644 >>> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi >>> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi >>> @@ -160,9 +160,9 @@ >>> #address-cells = <0>; >>> interrupt-controller; >>> reg = <0x0 0xf1010000 0 0x1000>, >>> - <0x0 0xf1020000 0 0x2000>, >>> + <0x0 0xf1020000 0 0x20000>, >>> <0x0 0xf1040000 0 0x20000>, >>> - <0x0 0xf1060000 0 0x2000>; >>> + <0x0 0xf1060000 0 0x20000>; >>> interrupts = <GIC_PPI 9 >>> (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; >>> }; >> >> Region 0: >> 4 KiB-pages 0xf1011000-0xf101ffff are aliased to 0xf1010000-0xf1010fff, >> but we need the first 4 KiB only. >> >> Region 1: >> 4 KiB-pages 0xf1021000-0xf102ffff are aliased to 0xf1020000-0xf1020fff, >> 4 KiB-pages 0xf1030000-0xf103ffff are all zeroes, probably due to >> non-secure mode? > > No. This 4kB page only contain a single register (GICC_DIR), which is > WO/RAZ. > >> >> Region 2: >> 4 KiB-pages 0xf1041000-0xf104ffff are aliased to 0xf1040000-0xf1040fff, >> 4 KiB-pages 0xf1050000-0xf105ffff are all zeroes, probably due to >> non-secure mode? > > Neither. The aliases are an unused feature of GIC400 exposing the other > CPUs view of the same registers... > >> >> Region 3: >> 4 KiB-pages 0xf1061000-0xf106ffff are aliased to 0xf1060000-0xf1060fff, >> 4 KiB-pages 0xf1070000-0xf107ffff are all zeroes, probably due to >> non-secure mode? > > Same as region 1. > >> >> Region 2 already had a 128 KiB size before, which allowed to use 8 KiB at >> 0xf104f000. > > No. This region (GICH) only needs the first 256 bytes or so. The rest is > either RAZ/WI or useless stuff. > >> >> An 8 KiB size for regions 1 and 3 indeed didn't make much sense, as this >> covered two identical (aliased) 4 KiB pages, instead of two different pages >> at offset 0xf000. > > While we're at it, adding a pointer to the documentation (GIC400 and > SBSA) would be tremendously useful, as it'd avoid misinterpreting the > various bits. If anybody could give a short description I could copy & paste into the commit message that would be quite helpful :) Best regards Dirk
On 10/05/16 16:29, Dirk Behme wrote: > On 10.05.2016 16:17, Marc Zyngier wrote: >> On 10/05/16 14:33, Geert Uytterhoeven wrote: >>> CC Marc, lakml >>> >>> On Tue, Apr 19, 2016 at 8:29 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote: >>>> From: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> >>>> >>>> There are some requirements about the GIC-400 memory layout and its >>>> mapping if using 64k aligned base addresses like on r8a7795. >>>> >>>> See e.g. >>>> >>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=21550029f709072aacf3b9 >>>> >>>> Map the whole memory range instead of only 0x2000. This will fix >>>> the issue that some hypervisors, e.g. Xen, fail to handle the >>>> interrupts correctly. >>>> >>>> Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> >>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> >>> >>> Based on my understanding below >>> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> >>>> --- >>>> Note: This patch is against renesas-drivers-2016-04-12-v4.6-rc3 >>>> >>>> arch/arm64/boot/dts/renesas/r8a7795.dtsi | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi >>>> index 8be9424..d880fd4 100644 >>>> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi >>>> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi >>>> @@ -160,9 +160,9 @@ >>>> #address-cells = <0>; >>>> interrupt-controller; >>>> reg = <0x0 0xf1010000 0 0x1000>, >>>> - <0x0 0xf1020000 0 0x2000>, >>>> + <0x0 0xf1020000 0 0x20000>, >>>> <0x0 0xf1040000 0 0x20000>, >>>> - <0x0 0xf1060000 0 0x2000>; >>>> + <0x0 0xf1060000 0 0x20000>; >>>> interrupts = <GIC_PPI 9 >>>> (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; >>>> }; >>> >>> Region 0: >>> 4 KiB-pages 0xf1011000-0xf101ffff are aliased to 0xf1010000-0xf1010fff, >>> but we need the first 4 KiB only. >>> >>> Region 1: >>> 4 KiB-pages 0xf1021000-0xf102ffff are aliased to 0xf1020000-0xf1020fff, >>> 4 KiB-pages 0xf1030000-0xf103ffff are all zeroes, probably due to >>> non-secure mode? >> >> No. This 4kB page only contain a single register (GICC_DIR), which is >> WO/RAZ. >> >>> >>> Region 2: >>> 4 KiB-pages 0xf1041000-0xf104ffff are aliased to 0xf1040000-0xf1040fff, >>> 4 KiB-pages 0xf1050000-0xf105ffff are all zeroes, probably due to >>> non-secure mode? >> >> Neither. The aliases are an unused feature of GIC400 exposing the other >> CPUs view of the same registers... >> >>> >>> Region 3: >>> 4 KiB-pages 0xf1061000-0xf106ffff are aliased to 0xf1060000-0xf1060fff, >>> 4 KiB-pages 0xf1070000-0xf107ffff are all zeroes, probably due to >>> non-secure mode? >> >> Same as region 1. >> >>> >>> Region 2 already had a 128 KiB size before, which allowed to use 8 KiB at >>> 0xf104f000. >> >> No. This region (GICH) only needs the first 256 bytes or so. The rest is >> either RAZ/WI or useless stuff. >> >>> >>> An 8 KiB size for regions 1 and 3 indeed didn't make much sense, as this >>> covered two identical (aliased) 4 KiB pages, instead of two different pages >>> at offset 0xf000. >> >> While we're at it, adding a pointer to the documentation (GIC400 and >> SBSA) would be tremendously useful, as it'd avoid misinterpreting the >> various bits. > > > If anybody could give a short description I could copy & paste into > the commit message that would be quite helpful :) Well, there is my reply to an earlier email in this very thread, as well as the xen commit which quotes my original commit (which you could also refer to). I'll leave it to you to eradicate the swear words (or to leave them in place as a testimony of what 4 years of GIC hacking can do to an otherwise sane person). Thanks, M.
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 :)
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? Best regards Dirk
Hi Dirk, On Wed, May 18, 2016 at 10:00 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote: > 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? The initial r8a7796.dtsi in renesas-drivers is a very preliminary one, just to get things going. The final one will be added through properly submitted and reviewed patches. 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
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.
On 19.05.2016 04:16, Simon Horman wrote: > 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. Great, many thanks for your support! Best regards Dirk
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi index 8be9424..d880fd4 100644 --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi @@ -160,9 +160,9 @@ #address-cells = <0>; interrupt-controller; reg = <0x0 0xf1010000 0 0x1000>, - <0x0 0xf1020000 0 0x2000>, + <0x0 0xf1020000 0 0x20000>, <0x0 0xf1040000 0 0x20000>, - <0x0 0xf1060000 0 0x2000>; + <0x0 0xf1060000 0 0x20000>; interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>; };