| Message ID | 1450353133-15992-4-git-send-email-tn@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/12/15 11:52, Tomasz Nowicki wrote: > On systems supporting GICv3 and above, in MADT GICC structures, the > field of GICR Base Address holds the 64-bit physical address of the > associated Redistributor if the GIC Redistributors are not in the > always-on power domain, so instead of init GICR regions via GIC > redistributor structure(s), init it with GICR base address in GICC > structures in that case. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com> > --- > drivers/irqchip/irq-gic-v3.c | 98 ++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 89 insertions(+), 9 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index c4b929c..0528e82 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -39,6 +39,7 @@ > struct redist_region { > void __iomem *redist_base; > phys_addr_t phys_base; > + bool single_redist; > }; > > struct gic_chip_data { > @@ -435,6 +436,9 @@ static int gic_populate_rdist(void) > return 0; > } > > + if (gic_data.redist_regions[i].single_redist) > + break; > + > if (gic_data.redist_stride) { > ptr += gic_data.redist_stride; > } else { > @@ -965,6 +969,7 @@ IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init); > #ifdef CONFIG_ACPI > static struct redist_region *redist_regs __initdata; > static u32 nr_redist_regions __initdata; > +static bool single_redist; > > static int __init > gic_acpi_register_redist(phys_addr_t phys_base, u64 size) > @@ -979,7 +984,8 @@ gic_acpi_register_redist(phys_addr_t phys_base, u64 size) > } > > redist_regs[count].phys_base = phys_base; > - redist_regs[count++].redist_base = redist_base; > + redist_regs[count].redist_base = redist_base; nit: move the count++ out of the access in the previous patch, this will make this series a bit easier to follow. > + redist_regs[count++].single_redist = single_redist; What is that single_redist for? Is that because you can't rely on GICR_TYPER.Last? > return 0; > } > > @@ -993,6 +999,48 @@ gic_acpi_parse_madt_redist(struct acpi_subtable_header *header, > return gic_acpi_register_redist(redist->base_address, redist->length); > } > > +static int __init > +gic_acpi_parse_madt_gicc(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + struct acpi_madt_generic_interrupt *gicc; > + void __iomem *redist_base; > + u64 typer; > + u32 size; > + > + gicc = (struct acpi_madt_generic_interrupt *)header; > + redist_base = ioremap(gicc->gicr_base_address, SZ_64K * 2); > + if (!redist_base) > + return -ENOMEM; > + > + typer = readq_relaxed(redist_base + GICR_TYPER); > + /* don't map reserved page as it's buggy to access it */ > + size = (typer & GICR_TYPER_VLPIS) ? SZ_64K * 3 : SZ_64K * 2; > + iounmap(redist_base); What a mess. If you discover a !VLPIS redistributor, why do you have to unmap it to remap it again? Also, please map the whole region for the redistributor as we have on the DT side (4 64kB pages for VLPIS capable redistributors). Also, the spec says: "On systems supporting GICv3 and above, this field holds the 64-bit physical address of the associated Redistributor. If all of the GIC Redistributors are in the always-on power domain, GICR structures should be used to describe the Redistributors instead, and this field must be set to 0." which triggers two questions: - Can you access always the GICR_TYPER register without waking the redistributor up? - How do you cope with situations where some redistributors are in the always-on domain, and some are not? > + return gic_acpi_register_redist(gicc->gicr_base_address, size); > +} > + > +static int __init gic_acpi_collect_gicr_base(void) > +{ > + acpi_tbl_entry_handler redist_parser; > + enum acpi_madt_type type; > + > + if (single_redist) { > + type = ACPI_MADT_TYPE_GENERIC_INTERRUPT; > + redist_parser = gic_acpi_parse_madt_gicc; > + } else { > + type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR; > + redist_parser = gic_acpi_parse_madt_redist; > + } > + > + /* Collect redistributor base addresses in GICR entries */ > + if (acpi_table_parse_madt(type, redist_parser, 0) > 0) > + return 0; > + > + pr_info("No valid GICR entries exist\n"); > + return -ENODEV; > +} > + > static int __init gic_acpi_match_gicr(struct acpi_subtable_header *header, > const unsigned long end) > { > @@ -1000,6 +1048,42 @@ static int __init gic_acpi_match_gicr(struct acpi_subtable_header *header, > return 0; > } > > +static int __init gic_acpi_match_gicc(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + struct acpi_madt_generic_interrupt *gicc = > + (struct acpi_madt_generic_interrupt *)header; > + > + /* > + * If GICC is enabled and has valid gicr base address, then it means > + * GICR base is presented via GICC > + */ > + if ((gicc->flags & ACPI_MADT_ENABLED) && gicc->gicr_base_address) > + return 0; > + > + return -ENODEV; > +} > + > +static int __init gic_acpi_count_gicr_regions(void) > +{ > + int count; > + > + /* Count how many redistributor regions we have */ > + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, > + gic_acpi_match_gicr, 0); > + if (count > 0) { > + single_redist = false; > + return count; > + } > + > + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, > + gic_acpi_match_gicc, 0); > + if (count > 0) > + single_redist = true; > + > + return count; > +} > + Here, you seem to consider GICR and GICC tables to be mutually exclusive. I don't think the spec says so. > static bool __init acpi_validate_gic_table(struct acpi_subtable_header *header, > struct acpi_probe_entry *ape) > { > @@ -1011,8 +1095,7 @@ static bool __init acpi_validate_gic_table(struct acpi_subtable_header *header, > return false; > > /* We need to do that exercise anyway, the sooner the better */ > - count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, > - gic_acpi_match_gicr, 0); > + count = gic_acpi_count_gicr_regions(); > if (count <= 0) > return false; > > @@ -1028,7 +1111,7 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end) > struct acpi_madt_generic_distributor *dist; > struct fwnode_handle *domain_handle; > void __iomem *dist_base; > - int i, err, count; > + int i, err; > > /* Get distributor base address */ > dist = (struct acpi_madt_generic_distributor *)header; > @@ -1051,12 +1134,9 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end) > goto out_dist_unmap; > } > > - count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, > - gic_acpi_parse_madt_redist, 0); > - if (count <= 0) { > - err = -ENODEV; > + err = gic_acpi_collect_gicr_base(); > + if (err) > goto out_redist_unmap; > - } > > domain_handle = irq_domain_alloc_fwnode(dist_base); > if (!domain_handle) { > Thanks, M.
On 01/12/2016 08:03 PM, Marc Zyngier wrote: > On 17/12/15 11:52, Tomasz Nowicki wrote: >> On systems supporting GICv3 and above, in MADT GICC structures, the >> field of GICR Base Address holds the 64-bit physical address of the >> associated Redistributor if the GIC Redistributors are not in the >> always-on power domain, so instead of init GICR regions via GIC >> redistributor structure(s), init it with GICR base address in GICC >> structures in that case. >> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> >> --- >> drivers/irqchip/irq-gic-v3.c | 98 ++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 89 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c >> index c4b929c..0528e82 100644 >> --- a/drivers/irqchip/irq-gic-v3.c >> +++ b/drivers/irqchip/irq-gic-v3.c >> @@ -39,6 +39,7 @@ >> struct redist_region { >> void __iomem *redist_base; >> phys_addr_t phys_base; >> + bool single_redist; >> }; >> >> struct gic_chip_data { >> @@ -435,6 +436,9 @@ static int gic_populate_rdist(void) >> return 0; >> } >> >> + if (gic_data.redist_regions[i].single_redist) >> + break; >> + >> if (gic_data.redist_stride) { >> ptr += gic_data.redist_stride; >> } else { >> @@ -965,6 +969,7 @@ IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init); >> #ifdef CONFIG_ACPI >> static struct redist_region *redist_regs __initdata; >> static u32 nr_redist_regions __initdata; >> +static bool single_redist; >> >> static int __init >> gic_acpi_register_redist(phys_addr_t phys_base, u64 size) >> @@ -979,7 +984,8 @@ gic_acpi_register_redist(phys_addr_t phys_base, u64 size) >> } >> >> redist_regs[count].phys_base = phys_base; >> - redist_regs[count++].redist_base = redist_base; >> + redist_regs[count].redist_base = redist_base; > > nit: move the count++ out of the access in the previous patch, this will > make this series a bit easier to follow. OK. > >> + redist_regs[count++].single_redist = single_redist; > > What is that single_redist for? Is that because you can't rely on > GICR_TYPER.Last? Yes, there is no GICR_TYPER.Last bit for some redistributors, as it's valid for redistributor regions. > >> return 0; >> } >> >> @@ -993,6 +999,48 @@ gic_acpi_parse_madt_redist(struct acpi_subtable_header *header, >> return gic_acpi_register_redist(redist->base_address, redist->length); >> } >> >> +static int __init >> +gic_acpi_parse_madt_gicc(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + struct acpi_madt_generic_interrupt *gicc; >> + void __iomem *redist_base; >> + u64 typer; >> + u32 size; >> + >> + gicc = (struct acpi_madt_generic_interrupt *)header; >> + redist_base = ioremap(gicc->gicr_base_address, SZ_64K * 2); >> + if (!redist_base) >> + return -ENOMEM; >> + >> + typer = readq_relaxed(redist_base + GICR_TYPER); >> + /* don't map reserved page as it's buggy to access it */ >> + size = (typer & GICR_TYPER_VLPIS) ? SZ_64K * 3 : SZ_64K * 2; >> + iounmap(redist_base); > > What a mess. If you discover a !VLPIS redistributor, why do you have to > unmap it to remap it again? Also, please map the whole region for the I think I missed something here, I didn't know it's GICv3 or v4, I need to check the GICR_TYPER first, then decide map 2 or 4 SZ_64K pages. > redistributor as we have on the DT side (4 64kB pages for VLPIS capable > redistributors). > > Also, the spec says: > > "On systems supporting GICv3 and above, this field holds the 64-bit > physical address of the associated Redistributor. If all of the GIC > Redistributors are in the always-on power domain, GICR structures should > be used to describe the Redistributors instead, and this field must be > set to 0." > > which triggers two questions: > - Can you access always the GICR_TYPER register without waking the > redistributor up? I missed this part, can you suggest how can we do that? accessing some register before access to redistributor? > - How do you cope with situations where some redistributors are in the > always-on domain, and some are not? I'm not sure if there is such hardware, if yes, do we need to fix the spec first? > >> + return gic_acpi_register_redist(gicc->gicr_base_address, size); >> +} >> + >> +static int __init gic_acpi_collect_gicr_base(void) >> +{ >> + acpi_tbl_entry_handler redist_parser; >> + enum acpi_madt_type type; >> + >> + if (single_redist) { >> + type = ACPI_MADT_TYPE_GENERIC_INTERRUPT; >> + redist_parser = gic_acpi_parse_madt_gicc; >> + } else { >> + type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR; >> + redist_parser = gic_acpi_parse_madt_redist; >> + } >> + >> + /* Collect redistributor base addresses in GICR entries */ >> + if (acpi_table_parse_madt(type, redist_parser, 0) > 0) >> + return 0; >> + >> + pr_info("No valid GICR entries exist\n"); >> + return -ENODEV; >> +} >> + >> static int __init gic_acpi_match_gicr(struct acpi_subtable_header *header, >> const unsigned long end) >> { >> @@ -1000,6 +1048,42 @@ static int __init gic_acpi_match_gicr(struct acpi_subtable_header *header, >> return 0; >> } >> >> +static int __init gic_acpi_match_gicc(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + struct acpi_madt_generic_interrupt *gicc = >> + (struct acpi_madt_generic_interrupt *)header; >> + >> + /* >> + * If GICC is enabled and has valid gicr base address, then it means >> + * GICR base is presented via GICC >> + */ >> + if ((gicc->flags & ACPI_MADT_ENABLED) && gicc->gicr_base_address) >> + return 0; >> + >> + return -ENODEV; >> +} >> + >> +static int __init gic_acpi_count_gicr_regions(void) >> +{ >> + int count; >> + >> + /* Count how many redistributor regions we have */ >> + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, >> + gic_acpi_match_gicr, 0); >> + if (count > 0) { >> + single_redist = false; >> + return count; >> + } >> + >> + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, >> + gic_acpi_match_gicc, 0); >> + if (count > 0) >> + single_redist = true; >> + >> + return count; >> +} >> + > > Here, you seem to consider GICR and GICC tables to be mutually > exclusive. I don't think the spec says so. Good question, I will ask Charles first about it. Thanks Hanjun
On 12/01/16 15:05, Hanjun Guo wrote: > On 01/12/2016 08:03 PM, Marc Zyngier wrote: >> On 17/12/15 11:52, Tomasz Nowicki wrote: >>> On systems supporting GICv3 and above, in MADT GICC structures, the >>> field of GICR Base Address holds the 64-bit physical address of the >>> associated Redistributor if the GIC Redistributors are not in the >>> always-on power domain, so instead of init GICR regions via GIC >>> redistributor structure(s), init it with GICR base address in GICC >>> structures in that case. >>> >>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> >>> --- >>> drivers/irqchip/irq-gic-v3.c | 98 ++++++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 89 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c >>> index c4b929c..0528e82 100644 >>> --- a/drivers/irqchip/irq-gic-v3.c >>> +++ b/drivers/irqchip/irq-gic-v3.c >>> @@ -39,6 +39,7 @@ >>> struct redist_region { >>> void __iomem *redist_base; >>> phys_addr_t phys_base; >>> + bool single_redist; >>> }; >>> >>> struct gic_chip_data { >>> @@ -435,6 +436,9 @@ static int gic_populate_rdist(void) >>> return 0; >>> } >>> >>> + if (gic_data.redist_regions[i].single_redist) >>> + break; >>> + >>> if (gic_data.redist_stride) { >>> ptr += gic_data.redist_stride; >>> } else { >>> @@ -965,6 +969,7 @@ IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init); >>> #ifdef CONFIG_ACPI >>> static struct redist_region *redist_regs __initdata; >>> static u32 nr_redist_regions __initdata; >>> +static bool single_redist; >>> >>> static int __init >>> gic_acpi_register_redist(phys_addr_t phys_base, u64 size) >>> @@ -979,7 +984,8 @@ gic_acpi_register_redist(phys_addr_t phys_base, u64 size) >>> } >>> >>> redist_regs[count].phys_base = phys_base; >>> - redist_regs[count++].redist_base = redist_base; >>> + redist_regs[count].redist_base = redist_base; >> >> nit: move the count++ out of the access in the previous patch, this will >> make this series a bit easier to follow. > > OK. > >> >>> + redist_regs[count++].single_redist = single_redist; >> >> What is that single_redist for? Is that because you can't rely on >> GICR_TYPER.Last? > > Yes, there is no GICR_TYPER.Last bit for some redistributors, > as it's valid for redistributor regions. > >> >>> return 0; >>> } >>> >>> @@ -993,6 +999,48 @@ gic_acpi_parse_madt_redist(struct acpi_subtable_header *header, >>> return gic_acpi_register_redist(redist->base_address, redist->length); >>> } >>> >>> +static int __init >>> +gic_acpi_parse_madt_gicc(struct acpi_subtable_header *header, >>> + const unsigned long end) >>> +{ >>> + struct acpi_madt_generic_interrupt *gicc; >>> + void __iomem *redist_base; >>> + u64 typer; >>> + u32 size; >>> + >>> + gicc = (struct acpi_madt_generic_interrupt *)header; >>> + redist_base = ioremap(gicc->gicr_base_address, SZ_64K * 2); >>> + if (!redist_base) >>> + return -ENOMEM; >>> + >>> + typer = readq_relaxed(redist_base + GICR_TYPER); >>> + /* don't map reserved page as it's buggy to access it */ >>> + size = (typer & GICR_TYPER_VLPIS) ? SZ_64K * 3 : SZ_64K * 2; >>> + iounmap(redist_base); >> >> What a mess. If you discover a !VLPIS redistributor, why do you have to >> unmap it to remap it again? Also, please map the whole region for the > > I think I missed something here, I didn't know it's GICv3 or v4, I need > to check the GICR_TYPER first, then decide map 2 or 4 SZ_64K pages. But if you find out it is a v3, you already have it mapped, so why unmap and then remap in this case? > >> redistributor as we have on the DT side (4 64kB pages for VLPIS capable >> redistributors). >> >> Also, the spec says: >> >> "On systems supporting GICv3 and above, this field holds the 64-bit >> physical address of the associated Redistributor. If all of the GIC >> Redistributors are in the always-on power domain, GICR structures should >> be used to describe the Redistributors instead, and this field must be >> set to 0." >> >> which triggers two questions: >> - Can you access always the GICR_TYPER register without waking the >> redistributor up? > > I missed this part, can you suggest how can we do that? accessing some > register before access to redistributor? This redistributor may be in a power-domain that is off. Are you guaranteed that you can access GICR_TYPER even when it is off? > >> - How do you cope with situations where some redistributors are in the >> always-on domain, and some are not? > > I'm not sure if there is such hardware, if yes, do we need to fix > the spec first? It is something that should definitely be clarified. Can we end-up in a situation where some redistributors are described via the GICR structure, and some via the GICC structure? The spec is a bit ambiguous. >> >>> + return gic_acpi_register_redist(gicc->gicr_base_address, size); >>> +} >>> + >>> +static int __init gic_acpi_collect_gicr_base(void) >>> +{ >>> + acpi_tbl_entry_handler redist_parser; >>> + enum acpi_madt_type type; >>> + >>> + if (single_redist) { >>> + type = ACPI_MADT_TYPE_GENERIC_INTERRUPT; >>> + redist_parser = gic_acpi_parse_madt_gicc; >>> + } else { >>> + type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR; >>> + redist_parser = gic_acpi_parse_madt_redist; >>> + } >>> + >>> + /* Collect redistributor base addresses in GICR entries */ >>> + if (acpi_table_parse_madt(type, redist_parser, 0) > 0) >>> + return 0; >>> + >>> + pr_info("No valid GICR entries exist\n"); >>> + return -ENODEV; >>> +} >>> + >>> static int __init gic_acpi_match_gicr(struct acpi_subtable_header *header, >>> const unsigned long end) >>> { >>> @@ -1000,6 +1048,42 @@ static int __init gic_acpi_match_gicr(struct acpi_subtable_header *header, >>> return 0; >>> } >>> >>> +static int __init gic_acpi_match_gicc(struct acpi_subtable_header *header, >>> + const unsigned long end) >>> +{ >>> + struct acpi_madt_generic_interrupt *gicc = >>> + (struct acpi_madt_generic_interrupt *)header; >>> + >>> + /* >>> + * If GICC is enabled and has valid gicr base address, then it means >>> + * GICR base is presented via GICC >>> + */ >>> + if ((gicc->flags & ACPI_MADT_ENABLED) && gicc->gicr_base_address) >>> + return 0; >>> + >>> + return -ENODEV; >>> +} >>> + >>> +static int __init gic_acpi_count_gicr_regions(void) >>> +{ >>> + int count; >>> + >>> + /* Count how many redistributor regions we have */ >>> + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, >>> + gic_acpi_match_gicr, 0); >>> + if (count > 0) { >>> + single_redist = false; >>> + return count; >>> + } >>> + >>> + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, >>> + gic_acpi_match_gicc, 0); >>> + if (count > 0) >>> + single_redist = true; >>> + >>> + return count; >>> +} >>> + >> >> Here, you seem to consider GICR and GICC tables to be mutually >> exclusive. I don't think the spec says so. > > Good question, I will ask Charles first about it. I just did, and he agreed that the spec is ambiguous - but that your interpretation is probably the correct one. It would be good fix it though. Thanks, M.
On 12.01.2016 13:03, Marc Zyngier wrote: > + typer = readq_relaxed(redist_base + GICR_TYPER); > + /* don't map reserved page as it's buggy to access it */ > + size = (typer & GICR_TYPER_VLPIS) ? SZ_64K * 3 : SZ_64K * 2; [...] > Also, please map the whole region for the > redistributor as we have on the DT side (4 64kB pages for VLPIS capable > redistributors). Hanjun, is it something you had problem with? Tomasz
On 12.01.2016 17:16, Marc Zyngier wrote: >>> >>Also, the spec says: >>> >> >>> >>"On systems supporting GICv3 and above, this field holds the 64-bit >>> >>physical address of the associated Redistributor. If all of the GIC >>> >>Redistributors are in the always-on power domain, GICR structures should >>> >>be used to describe the Redistributors instead, and this field must be >>> >>set to 0." >>> >> >>> >>which triggers two questions: >>> >>- Can you access always the GICR_TYPER register without waking the >>> >>redistributor up? >> > >> >I missed this part, can you suggest how can we do that? accessing some >> >register before access to redistributor? > This redistributor may be in a power-domain that is off. Are you > guaranteed that you can access GICR_TYPER even when it is off? > >> > >>> >>- How do you cope with situations where some redistributors are in the >>> >>always-on domain, and some are not? >> > >> >I'm not sure if there is such hardware, if yes, do we need to fix >> >the spec first? > It is something that should definitely be clarified. Can we end-up in a > situation where some redistributors are described via the GICR > structure, and some via the GICC structure? The spec is a bit ambiguous. > To recap: 1. GICR and GICC subtables should be mutually exclusive, but ACPI spec should be clear about this. 2. We need to know if we can access GICR_TYPER without waking the redistributor up. Tirumalesh, can you please advice? Tomasz
On 2016/1/13 0:45, Tomasz Nowicki wrote: > On 12.01.2016 13:03, Marc Zyngier wrote: > > > + typer = readq_relaxed(redist_base + GICR_TYPER); > > + /* don't map reserved page as it's buggy to access it */ > > + size = (typer & GICR_TYPER_VLPIS) ? SZ_64K * 3 : SZ_64K * 2; > > [...] > >> Also, please map the whole region for the >> redistributor as we have on the DT side (4 64kB pages for VLPIS capable >> redistributors). > > Hanjun, is it something you had problem with? Yes, for D02, it use 3 64kB pages for GICv4, it removed the reserved page. In DT case, it has "stride" binding so it can support no standard cases, seems that we can introduce "stride" for ACPI too. Thanks Hanjun
On 13/01/16 01:52, Hanjun Guo wrote: > > > On 2016/1/13 0:45, Tomasz Nowicki wrote: >> On 12.01.2016 13:03, Marc Zyngier wrote: >> >> > + typer = readq_relaxed(redist_base + GICR_TYPER); >> > + /* don't map reserved page as it's buggy to access it */ >> > + size = (typer & GICR_TYPER_VLPIS) ? SZ_64K * 3 : SZ_64K * 2; >> >> [...] >> >>> Also, please map the whole region for the >>> redistributor as we have on the DT side (4 64kB pages for VLPIS capable >>> redistributors). >> >> Hanjun, is it something you had problem with? > > Yes, for D02, it use 3 64kB pages for GICv4, it removed the reserved > page. In DT case, it has "stride" binding so it can support no standard > cases, seems that we can introduce "stride" for ACPI too. I don't think there is any need for that, assuming this system only exposes its redistributors via the ACPI GICC structure. In which case, I don't think it hurts to have overlapping mappings: For redistributors A and B, we never touch A[3] (the reserved page), but we will access B[0]. Thanks, M.
On 2016/1/13 16:35, Marc Zyngier wrote: > On 13/01/16 01:52, Hanjun Guo wrote: >> >> >> On 2016/1/13 0:45, Tomasz Nowicki wrote: >>> On 12.01.2016 13:03, Marc Zyngier wrote: >>> >>> > + typer = readq_relaxed(redist_base + GICR_TYPER); >>> > + /* don't map reserved page as it's buggy to access it */ >>> > + size = (typer & GICR_TYPER_VLPIS) ? SZ_64K * 3 : SZ_64K * 2; >>> >>> [...] >>> >>>> Also, please map the whole region for the >>>> redistributor as we have on the DT side (4 64kB pages for VLPIS capable >>>> redistributors). >>> >>> Hanjun, is it something you had problem with? >> >> Yes, for D02, it use 3 64kB pages for GICv4, it removed the reserved >> page. In DT case, it has "stride" binding so it can support no standard >> cases, seems that we can introduce "stride" for ACPI too. > > I don't think there is any need for that, assuming this system only > exposes its redistributors via the ACPI GICC structure. In which case, I > don't think it hurts to have overlapping mappings: For redistributors A > and B, we never touch A[3] (the reserved page), but we will access B[0]. I think so, thanks for the clarify. I think we need to clarify the spec first and interpret it in the right way. Thanks Hanjun
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index c4b929c..0528e82 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -39,6 +39,7 @@ struct redist_region { void __iomem *redist_base; phys_addr_t phys_base; + bool single_redist; }; struct gic_chip_data { @@ -435,6 +436,9 @@ static int gic_populate_rdist(void) return 0; } + if (gic_data.redist_regions[i].single_redist) + break; + if (gic_data.redist_stride) { ptr += gic_data.redist_stride; } else { @@ -965,6 +969,7 @@ IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init); #ifdef CONFIG_ACPI static struct redist_region *redist_regs __initdata; static u32 nr_redist_regions __initdata; +static bool single_redist; static int __init gic_acpi_register_redist(phys_addr_t phys_base, u64 size) @@ -979,7 +984,8 @@ gic_acpi_register_redist(phys_addr_t phys_base, u64 size) } redist_regs[count].phys_base = phys_base; - redist_regs[count++].redist_base = redist_base; + redist_regs[count].redist_base = redist_base; + redist_regs[count++].single_redist = single_redist; return 0; } @@ -993,6 +999,48 @@ gic_acpi_parse_madt_redist(struct acpi_subtable_header *header, return gic_acpi_register_redist(redist->base_address, redist->length); } +static int __init +gic_acpi_parse_madt_gicc(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_madt_generic_interrupt *gicc; + void __iomem *redist_base; + u64 typer; + u32 size; + + gicc = (struct acpi_madt_generic_interrupt *)header; + redist_base = ioremap(gicc->gicr_base_address, SZ_64K * 2); + if (!redist_base) + return -ENOMEM; + + typer = readq_relaxed(redist_base + GICR_TYPER); + /* don't map reserved page as it's buggy to access it */ + size = (typer & GICR_TYPER_VLPIS) ? SZ_64K * 3 : SZ_64K * 2; + iounmap(redist_base); + return gic_acpi_register_redist(gicc->gicr_base_address, size); +} + +static int __init gic_acpi_collect_gicr_base(void) +{ + acpi_tbl_entry_handler redist_parser; + enum acpi_madt_type type; + + if (single_redist) { + type = ACPI_MADT_TYPE_GENERIC_INTERRUPT; + redist_parser = gic_acpi_parse_madt_gicc; + } else { + type = ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR; + redist_parser = gic_acpi_parse_madt_redist; + } + + /* Collect redistributor base addresses in GICR entries */ + if (acpi_table_parse_madt(type, redist_parser, 0) > 0) + return 0; + + pr_info("No valid GICR entries exist\n"); + return -ENODEV; +} + static int __init gic_acpi_match_gicr(struct acpi_subtable_header *header, const unsigned long end) { @@ -1000,6 +1048,42 @@ static int __init gic_acpi_match_gicr(struct acpi_subtable_header *header, return 0; } +static int __init gic_acpi_match_gicc(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_madt_generic_interrupt *gicc = + (struct acpi_madt_generic_interrupt *)header; + + /* + * If GICC is enabled and has valid gicr base address, then it means + * GICR base is presented via GICC + */ + if ((gicc->flags & ACPI_MADT_ENABLED) && gicc->gicr_base_address) + return 0; + + return -ENODEV; +} + +static int __init gic_acpi_count_gicr_regions(void) +{ + int count; + + /* Count how many redistributor regions we have */ + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, + gic_acpi_match_gicr, 0); + if (count > 0) { + single_redist = false; + return count; + } + + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, + gic_acpi_match_gicc, 0); + if (count > 0) + single_redist = true; + + return count; +} + static bool __init acpi_validate_gic_table(struct acpi_subtable_header *header, struct acpi_probe_entry *ape) { @@ -1011,8 +1095,7 @@ static bool __init acpi_validate_gic_table(struct acpi_subtable_header *header, return false; /* We need to do that exercise anyway, the sooner the better */ - count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, - gic_acpi_match_gicr, 0); + count = gic_acpi_count_gicr_regions(); if (count <= 0) return false; @@ -1028,7 +1111,7 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end) struct acpi_madt_generic_distributor *dist; struct fwnode_handle *domain_handle; void __iomem *dist_base; - int i, err, count; + int i, err; /* Get distributor base address */ dist = (struct acpi_madt_generic_distributor *)header; @@ -1051,12 +1134,9 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end) goto out_dist_unmap; } - count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, - gic_acpi_parse_madt_redist, 0); - if (count <= 0) { - err = -ENODEV; + err = gic_acpi_collect_gicr_base(); + if (err) goto out_redist_unmap; - } domain_handle = irq_domain_alloc_fwnode(dist_base); if (!domain_handle) {