| Message ID | 20161117175752.16475-2-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, I'm so pleased to see this series. Thank you! On Thu, Nov 17, 2016 at 05:57:49PM +0000, Andre Przywara wrote: > This adds an MMIO subtest to the GIC test. > It accesses some generic GICv2 registers and does some sanity tests, > like checking for some of them being read-only. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > arm/gic.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > arm/unittests.cfg | 6 ++++ > lib/arm/asm/gic.h | 2 ++ > 3 files changed, 107 insertions(+) > > diff --git a/arm/gic.c b/arm/gic.c > index 638b8b1..ba2585b 100644 > --- a/arm/gic.c > +++ b/arm/gic.c > @@ -3,6 +3,7 @@ > * > * GICv2 > * + test sending/receiving IPIs > + * + MMIO access tests > * GICv3 > * + test sending/receiving IPIs > * > @@ -274,6 +275,98 @@ static struct gic gicv3 = { > }, > }; > > +static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig) > +{ > + u32 reg; > + > + writel(pattern, address); > + reg = readl(address); > + > + if (reg != orig) > + writel(orig, address); > + > + return reg == orig; > +} > + > +static bool test_readonly_32(void *address, bool razwi) > +{ > + u32 orig, pattern; > + > + orig = readl(address); > + if (razwi && orig) > + return false; > + > + pattern = 0xffffffff; > + if (orig != pattern) { > + if (!test_ro_pattern_32(address, pattern, orig)) > + return false; > + } > + > + pattern = 0xa5a55a5a; > + if (orig != pattern) { > + if (!test_ro_pattern_32(address, pattern, orig)) > + return false; > + } > + > + pattern = 0; > + if (orig != pattern) { > + if (!test_ro_pattern_32(address, pattern, orig)) > + return false; > + } > + > + return true; > +} > + > +static bool test_typer_v2(uint32_t reg) > +{ > + int nr_gic_cpus = ((reg >> 5) & 0x7) + 1; > + > + report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus, > + nr_gic_cpus); > + > + return true; This test function can be a void. > +} > + > +static int gic_test_mmio(int gic_version) > +{ > + u32 reg; > + int nr_irqs; > + void *gic_dist_base, *idreg; > + > + switch(gic_version) { > + case 0x2: > + gic_dist_base = gicv2_dist_base(); > + idreg = gic_dist_base + 0xfe8; I see below you introduce GICD_ICPIDR2, so I guess you can use it here. > + break; > + case 0x3: > + report_abort("GICv3 MMIO tests NYI"); > + return -1; can't reach this return > + default: > + report_abort("GIC version %d not supported", gic_version); > + return 0; can't reach this return > + } > + > + reg = readl(gic_dist_base + GICD_TYPER); > + nr_irqs = 32 * ((reg & 0x1f) + 1); Any reason to avoid using GICD_TYPER_IRQS() here? > + report("number of implemented SPIs: %d", 1, nr_irqs - 32); We usually just use printf for informational output (but we should probably add a 'report_info' in order to keep the prefixes. I can do that now.) Anyway, please s/1/true > + > + test_typer_v2(reg); > + > + report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR)); > + > + report("GICD_TYPER is read-only", > + test_readonly_32(gic_dist_base + GICD_TYPER, false)); > + report("GICD_IIDR is read-only", > + test_readonly_32(gic_dist_base + GICD_IIDR, false)); > + > + reg = readl(idreg); > + report("ICPIDR2 is read-only (0x%x)", > + test_readonly_32(idreg, false), > + reg); > + > + return 0; You may want %08x for all your register printing. Since you either abort or always return success, then this function can be a void. > +} > + > int main(int argc, char **argv) > { > char pfx[8]; > @@ -332,6 +425,12 @@ int main(int argc, char **argv) > } > ipi_test(); > > + } else if (!strcmp(argv[1], "mmio")) { > + report_prefix_push(argv[1]); > + > + gic_test_mmio(gic_version()); Any reason to pass gic_version() here instead of just using it in gic_test_mmio? > + > + report_prefix_pop(); > } else { > report_abort("Unknown subtest '%s'", argv[1]); > } > diff --git a/arm/unittests.cfg b/arm/unittests.cfg > index c7392c7..0162e5a 100644 > --- a/arm/unittests.cfg > +++ b/arm/unittests.cfg > @@ -67,6 +67,12 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8)) > extra_params = -machine gic-version=2 -append 'ipi' > groups = gic > > +[gicv2-mmio] > +file = gic.flat > +smp = $((($MAX_SMP < 8)?$MAX_SMP:8)) > +extra_params = -machine gic-version=2 -append 'mmio' > +groups = gic > + > [gicv3-ipi] > file = gic.flat > smp = $MAX_SMP > diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h > index c2267b6..cef748d 100644 > --- a/lib/arm/asm/gic.h > +++ b/lib/arm/asm/gic.h > @@ -10,10 +10,12 @@ > /* Distributor registers */ > #define GICD_CTLR 0x0000 > #define GICD_TYPER 0x0004 > +#define GICD_IIDR 0x0008 > #define GICD_IGROUPR 0x0080 > #define GICD_ISENABLER 0x0100 > #define GICD_IPRIORITYR 0x0400 > #define GICD_SGIR 0x0f00 > +#define GICD_ICPIDR2 0x0fe8 > > #define GICD_TYPER_IRQS(typer) ((((typer) & 0x1f) + 1) * 32) > #define GICD_INT_EN_SET_SGI 0x0000ffff > -- > 2.9.0 > > Thanks, drew
Hi Drew, On 18/11/16 13:06, Andrew Jones wrote: > Hi Andre, > > I'm so pleased to see this series. Thank you! > > On Thu, Nov 17, 2016 at 05:57:49PM +0000, Andre Przywara wrote: >> This adds an MMIO subtest to the GIC test. >> It accesses some generic GICv2 registers and does some sanity tests, >> like checking for some of them being read-only. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> arm/gic.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> arm/unittests.cfg | 6 ++++ >> lib/arm/asm/gic.h | 2 ++ >> 3 files changed, 107 insertions(+) >> >> diff --git a/arm/gic.c b/arm/gic.c >> index 638b8b1..ba2585b 100644 >> --- a/arm/gic.c >> +++ b/arm/gic.c >> @@ -3,6 +3,7 @@ >> * >> * GICv2 >> * + test sending/receiving IPIs >> + * + MMIO access tests >> * GICv3 >> * + test sending/receiving IPIs >> * >> @@ -274,6 +275,98 @@ static struct gic gicv3 = { >> }, >> }; >> >> +static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig) >> +{ >> + u32 reg; >> + >> + writel(pattern, address); >> + reg = readl(address); >> + >> + if (reg != orig) >> + writel(orig, address); >> + >> + return reg == orig; >> +} >> + >> +static bool test_readonly_32(void *address, bool razwi) >> +{ >> + u32 orig, pattern; >> + >> + orig = readl(address); >> + if (razwi && orig) >> + return false; >> + >> + pattern = 0xffffffff; >> + if (orig != pattern) { >> + if (!test_ro_pattern_32(address, pattern, orig)) >> + return false; >> + } >> + >> + pattern = 0xa5a55a5a; >> + if (orig != pattern) { >> + if (!test_ro_pattern_32(address, pattern, orig)) >> + return false; >> + } >> + >> + pattern = 0; >> + if (orig != pattern) { >> + if (!test_ro_pattern_32(address, pattern, orig)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> +static bool test_typer_v2(uint32_t reg) >> +{ >> + int nr_gic_cpus = ((reg >> 5) & 0x7) + 1; >> + >> + report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus, >> + nr_gic_cpus); >> + >> + return true; > > This test function can be a void. > >> +} >> + >> +static int gic_test_mmio(int gic_version) >> +{ >> + u32 reg; >> + int nr_irqs; >> + void *gic_dist_base, *idreg; >> + >> + switch(gic_version) { >> + case 0x2: >> + gic_dist_base = gicv2_dist_base(); >> + idreg = gic_dist_base + 0xfe8; > > I see below you introduce GICD_ICPIDR2, so I guess you can use it here. > >> + break; >> + case 0x3: >> + report_abort("GICv3 MMIO tests NYI"); >> + return -1; > > can't reach this return But we need to tell GCC about this, because otherwise we get all kind of warnings (including bogus "maybe unused" warnings). __attribute__ ((noreturn)) seems the way to go here, but this is currently giving me a hard time ... >> + default: >> + report_abort("GIC version %d not supported", gic_version); >> + return 0; > > can't reach this return > >> + } >> + >> + reg = readl(gic_dist_base + GICD_TYPER); >> + nr_irqs = 32 * ((reg & 0x1f) + 1); > > Any reason to avoid using GICD_TYPER_IRQS() here? On the first write I wasn't aware of it, on a second thought then I wanted to avoid using the macro copied from Linux. But you are right, I will use it here. > >> + report("number of implemented SPIs: %d", 1, nr_irqs - 32); > > We usually just use printf for informational output (but we should > probably add a 'report_info' in order to keep the prefixes. I can > do that now.) Anyway, please s/1/true I saw your patch, will use that. >> + >> + test_typer_v2(reg); >> + >> + report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR)); >> + >> + report("GICD_TYPER is read-only", >> + test_readonly_32(gic_dist_base + GICD_TYPER, false)); >> + report("GICD_IIDR is read-only", >> + test_readonly_32(gic_dist_base + GICD_IIDR, false)); >> + >> + reg = readl(idreg); >> + report("ICPIDR2 is read-only (0x%x)", >> + test_readonly_32(idreg, false), >> + reg); >> + >> + return 0; > > You may want %08x for all your register printing. > > Since you either abort or always return success, then this function can be > a void. > >> +} >> + >> int main(int argc, char **argv) >> { >> char pfx[8]; >> @@ -332,6 +425,12 @@ int main(int argc, char **argv) >> } >> ipi_test(); >> >> + } else if (!strcmp(argv[1], "mmio")) { >> + report_prefix_push(argv[1]); >> + >> + gic_test_mmio(gic_version()); > > Any reason to pass gic_version() here instead of just using it > in gic_test_mmio? Not really, I originally wanted to pass this variable on in a clean fashion to allow sharing tests. But using the function shouldn't make any difference anymore, so I can easily replace it. "Yes, will fix" to all the other comments. Thanks for having a look! Cheers, Andre. >> + >> + report_prefix_pop(); >> } else { >> report_abort("Unknown subtest '%s'", argv[1]); >> } >> diff --git a/arm/unittests.cfg b/arm/unittests.cfg >> index c7392c7..0162e5a 100644 >> --- a/arm/unittests.cfg >> +++ b/arm/unittests.cfg >> @@ -67,6 +67,12 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8)) >> extra_params = -machine gic-version=2 -append 'ipi' >> groups = gic >> >> +[gicv2-mmio] >> +file = gic.flat >> +smp = $((($MAX_SMP < 8)?$MAX_SMP:8)) >> +extra_params = -machine gic-version=2 -append 'mmio' >> +groups = gic >> + >> [gicv3-ipi] >> file = gic.flat >> smp = $MAX_SMP >> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h >> index c2267b6..cef748d 100644 >> --- a/lib/arm/asm/gic.h >> +++ b/lib/arm/asm/gic.h >> @@ -10,10 +10,12 @@ >> /* Distributor registers */ >> #define GICD_CTLR 0x0000 >> #define GICD_TYPER 0x0004 >> +#define GICD_IIDR 0x0008 >> #define GICD_IGROUPR 0x0080 >> #define GICD_ISENABLER 0x0100 >> #define GICD_IPRIORITYR 0x0400 >> #define GICD_SGIR 0x0f00 >> +#define GICD_ICPIDR2 0x0fe8 >> >> #define GICD_TYPER_IRQS(typer) ((((typer) & 0x1f) + 1) * 32) >> #define GICD_INT_EN_SET_SGI 0x0000ffff >> -- >> 2.9.0 >> >> > > Thanks, > drew >
Hi Andre, On 18/11/2016 14:06, Andrew Jones wrote: > Hi Andre, > > I'm so pleased to see this series. Thank you! > > On Thu, Nov 17, 2016 at 05:57:49PM +0000, Andre Przywara wrote: >> This adds an MMIO subtest to the GIC test. >> It accesses some generic GICv2 registers and does some sanity tests, >> like checking for some of them being read-only. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> arm/gic.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> arm/unittests.cfg | 6 ++++ >> lib/arm/asm/gic.h | 2 ++ >> 3 files changed, 107 insertions(+) >> >> diff --git a/arm/gic.c b/arm/gic.c >> index 638b8b1..ba2585b 100644 >> --- a/arm/gic.c >> +++ b/arm/gic.c >> @@ -3,6 +3,7 @@ >> * >> * GICv2 >> * + test sending/receiving IPIs >> + * + MMIO access tests >> * GICv3 >> * + test sending/receiving IPIs >> * >> @@ -274,6 +275,98 @@ static struct gic gicv3 = { >> }, >> }; >> >> +static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig) >> +{ >> + u32 reg; >> + >> + writel(pattern, address); >> + reg = readl(address); >> + >> + if (reg != orig) >> + writel(orig, address); >> + >> + return reg == orig; >> +} >> + >> +static bool test_readonly_32(void *address, bool razwi) >> +{ >> + u32 orig, pattern; >> + >> + orig = readl(address); >> + if (razwi && orig) >> + return false; >> + >> + pattern = 0xffffffff; >> + if (orig != pattern) { >> + if (!test_ro_pattern_32(address, pattern, orig)) >> + return false; >> + } >> + >> + pattern = 0xa5a55a5a; >> + if (orig != pattern) { >> + if (!test_ro_pattern_32(address, pattern, orig)) >> + return false; >> + } >> + >> + pattern = 0; >> + if (orig != pattern) { >> + if (!test_ro_pattern_32(address, pattern, orig)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> +static bool test_typer_v2(uint32_t reg) not: function name is a bit misleading, test_cpu_interfaces? Thanks Eric >> +{ >> + int nr_gic_cpus = ((reg >> 5) & 0x7) + 1; >> + >> + report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus, >> + nr_gic_cpus); >> + >> + return true; > > This test function can be a void. > >> +} >> + >> +static int gic_test_mmio(int gic_version) >> +{ >> + u32 reg; >> + int nr_irqs; >> + void *gic_dist_base, *idreg; >> + >> + switch(gic_version) { >> + case 0x2: >> + gic_dist_base = gicv2_dist_base(); >> + idreg = gic_dist_base + 0xfe8; > > I see below you introduce GICD_ICPIDR2, so I guess you can use it here. > >> + break; >> + case 0x3: >> + report_abort("GICv3 MMIO tests NYI"); >> + return -1; > > can't reach this return > >> + default: >> + report_abort("GIC version %d not supported", gic_version); >> + return 0; > > can't reach this return > >> + } >> + >> + reg = readl(gic_dist_base + GICD_TYPER); >> + nr_irqs = 32 * ((reg & 0x1f) + 1); > > Any reason to avoid using GICD_TYPER_IRQS() here? > >> + report("number of implemented SPIs: %d", 1, nr_irqs - 32); > > We usually just use printf for informational output (but we should > probably add a 'report_info' in order to keep the prefixes. I can > do that now.) Anyway, please s/1/true > >> + >> + test_typer_v2(reg); >> + >> + report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR)); >> + >> + report("GICD_TYPER is read-only", >> + test_readonly_32(gic_dist_base + GICD_TYPER, false)); >> + report("GICD_IIDR is read-only", >> + test_readonly_32(gic_dist_base + GICD_IIDR, false)); >> + >> + reg = readl(idreg); >> + report("ICPIDR2 is read-only (0x%x)", >> + test_readonly_32(idreg, false), >> + reg); >> + >> + return 0; > > You may want %08x for all your register printing. > > Since you either abort or always return success, then this function can be > a void. > >> +} >> + >> int main(int argc, char **argv) >> { >> char pfx[8]; >> @@ -332,6 +425,12 @@ int main(int argc, char **argv) >> } >> ipi_test(); >> >> + } else if (!strcmp(argv[1], "mmio")) { >> + report_prefix_push(argv[1]); >> + >> + gic_test_mmio(gic_version()); > > Any reason to pass gic_version() here instead of just using it > in gic_test_mmio? > >> + >> + report_prefix_pop(); >> } else { >> report_abort("Unknown subtest '%s'", argv[1]); >> } >> diff --git a/arm/unittests.cfg b/arm/unittests.cfg >> index c7392c7..0162e5a 100644 >> --- a/arm/unittests.cfg >> +++ b/arm/unittests.cfg >> @@ -67,6 +67,12 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8)) >> extra_params = -machine gic-version=2 -append 'ipi' >> groups = gic >> >> +[gicv2-mmio] >> +file = gic.flat >> +smp = $((($MAX_SMP < 8)?$MAX_SMP:8)) >> +extra_params = -machine gic-version=2 -append 'mmio' >> +groups = gic >> + >> [gicv3-ipi] >> file = gic.flat >> smp = $MAX_SMP >> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h >> index c2267b6..cef748d 100644 >> --- a/lib/arm/asm/gic.h >> +++ b/lib/arm/asm/gic.h >> @@ -10,10 +10,12 @@ >> /* Distributor registers */ >> #define GICD_CTLR 0x0000 >> #define GICD_TYPER 0x0004 >> +#define GICD_IIDR 0x0008 >> #define GICD_IGROUPR 0x0080 >> #define GICD_ISENABLER 0x0100 >> #define GICD_IPRIORITYR 0x0400 >> #define GICD_SGIR 0x0f00 >> +#define GICD_ICPIDR2 0x0fe8 >> >> #define GICD_TYPER_IRQS(typer) ((((typer) & 0x1f) + 1) * 32) >> #define GICD_INT_EN_SET_SGI 0x0000ffff >> -- >> 2.9.0 >> >> > > Thanks, > drew >
diff --git a/arm/gic.c b/arm/gic.c index 638b8b1..ba2585b 100644 --- a/arm/gic.c +++ b/arm/gic.c @@ -3,6 +3,7 @@ * * GICv2 * + test sending/receiving IPIs + * + MMIO access tests * GICv3 * + test sending/receiving IPIs * @@ -274,6 +275,98 @@ static struct gic gicv3 = { }, }; +static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig) +{ + u32 reg; + + writel(pattern, address); + reg = readl(address); + + if (reg != orig) + writel(orig, address); + + return reg == orig; +} + +static bool test_readonly_32(void *address, bool razwi) +{ + u32 orig, pattern; + + orig = readl(address); + if (razwi && orig) + return false; + + pattern = 0xffffffff; + if (orig != pattern) { + if (!test_ro_pattern_32(address, pattern, orig)) + return false; + } + + pattern = 0xa5a55a5a; + if (orig != pattern) { + if (!test_ro_pattern_32(address, pattern, orig)) + return false; + } + + pattern = 0; + if (orig != pattern) { + if (!test_ro_pattern_32(address, pattern, orig)) + return false; + } + + return true; +} + +static bool test_typer_v2(uint32_t reg) +{ + int nr_gic_cpus = ((reg >> 5) & 0x7) + 1; + + report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus, + nr_gic_cpus); + + return true; +} + +static int gic_test_mmio(int gic_version) +{ + u32 reg; + int nr_irqs; + void *gic_dist_base, *idreg; + + switch(gic_version) { + case 0x2: + gic_dist_base = gicv2_dist_base(); + idreg = gic_dist_base + 0xfe8; + break; + case 0x3: + report_abort("GICv3 MMIO tests NYI"); + return -1; + default: + report_abort("GIC version %d not supported", gic_version); + return 0; + } + + reg = readl(gic_dist_base + GICD_TYPER); + nr_irqs = 32 * ((reg & 0x1f) + 1); + report("number of implemented SPIs: %d", 1, nr_irqs - 32); + + test_typer_v2(reg); + + report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR)); + + report("GICD_TYPER is read-only", + test_readonly_32(gic_dist_base + GICD_TYPER, false)); + report("GICD_IIDR is read-only", + test_readonly_32(gic_dist_base + GICD_IIDR, false)); + + reg = readl(idreg); + report("ICPIDR2 is read-only (0x%x)", + test_readonly_32(idreg, false), + reg); + + return 0; +} + int main(int argc, char **argv) { char pfx[8]; @@ -332,6 +425,12 @@ int main(int argc, char **argv) } ipi_test(); + } else if (!strcmp(argv[1], "mmio")) { + report_prefix_push(argv[1]); + + gic_test_mmio(gic_version()); + + report_prefix_pop(); } else { report_abort("Unknown subtest '%s'", argv[1]); } diff --git a/arm/unittests.cfg b/arm/unittests.cfg index c7392c7..0162e5a 100644 --- a/arm/unittests.cfg +++ b/arm/unittests.cfg @@ -67,6 +67,12 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8)) extra_params = -machine gic-version=2 -append 'ipi' groups = gic +[gicv2-mmio] +file = gic.flat +smp = $((($MAX_SMP < 8)?$MAX_SMP:8)) +extra_params = -machine gic-version=2 -append 'mmio' +groups = gic + [gicv3-ipi] file = gic.flat smp = $MAX_SMP diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h index c2267b6..cef748d 100644 --- a/lib/arm/asm/gic.h +++ b/lib/arm/asm/gic.h @@ -10,10 +10,12 @@ /* Distributor registers */ #define GICD_CTLR 0x0000 #define GICD_TYPER 0x0004 +#define GICD_IIDR 0x0008 #define GICD_IGROUPR 0x0080 #define GICD_ISENABLER 0x0100 #define GICD_IPRIORITYR 0x0400 #define GICD_SGIR 0x0f00 +#define GICD_ICPIDR2 0x0fe8 #define GICD_TYPER_IRQS(typer) ((((typer) & 0x1f) + 1) * 32) #define GICD_INT_EN_SET_SGI 0x0000ffff
This adds an MMIO subtest to the GIC test. It accesses some generic GICv2 registers and does some sanity tests, like checking for some of them being read-only. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- arm/gic.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ arm/unittests.cfg | 6 ++++ lib/arm/asm/gic.h | 2 ++ 3 files changed, 107 insertions(+)