| Message ID | 20180124143517.18469-2-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM: vGICv3: clean up optional DT properties | expand |
(+ Tools maintainers) Hi Andre, On 24/01/18 14:35, Andre Przywara wrote: > When creating a GICv3 devicetree node, we currently insert the > redistributor-stride and #redistributor-regions properties, with fixed > values which are actually the architected ones. But those properties are > optional and only needed to cover for broken platforms, where the values > differ from the architected one. This will never be the case for the I understand that the stride is defined by GICv3. But I don't think this is true for the number of regions. Looking at the spec, multiple regions seems to be allowed (see GICR_TYPER.Last). Did I miss anything? The rest looks good to me. > constructed DomU memory map. > So we drop those properties altogether and provide a clean and architected > GICv3 DT node for DomUs. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > tools/libxl/libxl_arm.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index 3e46554301..b5bba3cd33 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -524,14 +524,6 @@ static int make_gicv3_node(libxl__gc *gc, void *fdt) > res = fdt_property(fdt, "interrupt-controller", NULL, 0); > if (res) return res; > > - res = fdt_property_cell(fdt, "redistributor-stride", > - GUEST_GICV3_RDIST_STRIDE); > - if (res) return res; > - > - res = fdt_property_cell(fdt, "#redistributor-regions", > - GUEST_GICV3_RDIST_REGIONS); > - if (res) return res; > - > res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS, > 2, > gicd_base, gicd_size, > Cheers,
Hi, On 24/01/18 16:08, Julien Grall wrote: > (+ Tools maintainers) > > Hi Andre, > > On 24/01/18 14:35, Andre Przywara wrote: >> When creating a GICv3 devicetree node, we currently insert the >> redistributor-stride and #redistributor-regions properties, with fixed >> values which are actually the architected ones. But those properties are >> optional and only needed to cover for broken platforms, where the values >> differ from the architected one. This will never be the case for the > > I understand that the stride is defined by GICv3. But I don't think this > is true for the number of regions. Looking at the spec, multiple regions > seems to be allowed (see GICR_TYPER.Last). Did I miss anything? Well, the spec does indeed not say anything about it, but the DT binding description does: ============== Optional: ... - #redistributor-regions: The number of independent contiguous regions occupied by the redistributors. Required if more than one such region is present. ============== So we don't need it in our case, and in fact we don't implement *anything* to actually give the toolstack a choice. So we should consequently remove these lines, as they are pointless right now. Should we ever need to implement support for multiple regions, bringing this back is really our least concern. Cheers, Andre. > > The rest looks good to me. > >> constructed DomU memory map. >> So we drop those properties altogether and provide a clean and >> architected >> GICv3 DT node for DomUs. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> tools/libxl/libxl_arm.c | 8 -------- >> 1 file changed, 8 deletions(-) >> >> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c >> index 3e46554301..b5bba3cd33 100644 >> --- a/tools/libxl/libxl_arm.c >> +++ b/tools/libxl/libxl_arm.c >> @@ -524,14 +524,6 @@ static int make_gicv3_node(libxl__gc *gc, void *fdt) >> res = fdt_property(fdt, "interrupt-controller", NULL, 0); >> if (res) return res; >> - res = fdt_property_cell(fdt, "redistributor-stride", >> - GUEST_GICV3_RDIST_STRIDE); >> - if (res) return res; >> - >> - res = fdt_property_cell(fdt, "#redistributor-regions", >> - GUEST_GICV3_RDIST_REGIONS); >> - if (res) return res; >> - >> res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, >> ROOT_SIZE_CELLS, >> 2, >> gicd_base, gicd_size, >> > > Cheers, >
Hi Andre, On 24/01/18 16:35, Andre Przywara wrote: > On 24/01/18 16:08, Julien Grall wrote: >> (+ Tools maintainers) >> >> Hi Andre, >> >> On 24/01/18 14:35, Andre Przywara wrote: >>> When creating a GICv3 devicetree node, we currently insert the >>> redistributor-stride and #redistributor-regions properties, with fixed >>> values which are actually the architected ones. But those properties are >>> optional and only needed to cover for broken platforms, where the values >>> differ from the architected one. This will never be the case for the >> >> I understand that the stride is defined by GICv3. But I don't think this >> is true for the number of regions. Looking at the spec, multiple regions >> seems to be allowed (see GICR_TYPER.Last). Did I miss anything? > > Well, the spec does indeed not say anything about it, but the DT binding > description does: > ============== > Optional: > ... > - #redistributor-regions: The number of independent contiguous regions > occupied by the redistributors. Required if more than one such > region is present. > ============== > > So we don't need it in our case, and in fact we don't implement > *anything* to actually give the toolstack a choice. So we should > consequently remove these lines, as they are pointless right now. Should > we ever need to implement support for multiple regions, bringing this > back is really our least concern. I am not against this patch. However, the commit message should not induce that platform with more than 1 re-distributor regions are broken. So I don't see anything which would prevent us to provide a guest memory map with multiple re-distributor regions. Cheers,
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 3e46554301..b5bba3cd33 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -524,14 +524,6 @@ static int make_gicv3_node(libxl__gc *gc, void *fdt) res = fdt_property(fdt, "interrupt-controller", NULL, 0); if (res) return res; - res = fdt_property_cell(fdt, "redistributor-stride", - GUEST_GICV3_RDIST_STRIDE); - if (res) return res; - - res = fdt_property_cell(fdt, "#redistributor-regions", - GUEST_GICV3_RDIST_REGIONS); - if (res) return res; - res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS, 2, gicd_base, gicd_size,
When creating a GICv3 devicetree node, we currently insert the redistributor-stride and #redistributor-regions properties, with fixed values which are actually the architected ones. But those properties are optional and only needed to cover for broken platforms, where the values differ from the architected one. This will never be the case for the constructed DomU memory map. So we drop those properties altogether and provide a clean and architected GICv3 DT node for DomUs. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- tools/libxl/libxl_arm.c | 8 -------- 1 file changed, 8 deletions(-)