Hi Marc, Zhiqiang > -----Original Message----- > From: Marc Zyngier <m...@kernel.org> > Sent: Monday, February 21, 2022 4:23 PM > To: Michael Walle <mich...@walle.cc> > Cc: Wasim Khan (OSS) <wasim.k...@oss.nxp.com>; s...@chromium.org; > Priyanka Jain <priyanka.j...@nxp.com>; tred...@nvidia.com; > twar...@nvidia.com; Varun Sethi <v.se...@nxp.com>; u- > b...@lists.denx.de; Wasim Khan <wasim.k...@nxp.com> > Subject: Re: [PATCH] armv8: fsl-layerscape: use previous aligned address for > gic_lpi_base > > On Mon, 21 Feb 2022 10:24:36 +0000, > Michael Walle <mich...@walle.cc> wrote: > > > > Hi, > > > > Am 2022-02-21 11:16, schrieb Wasim Khan: > > > From: Wasim Khan <wasim.k...@nxp.com> > > > > > > Memory after gd->arch.resv_ram is reserved for MC block. > > > Use ALIGN_DOWN to avoid updating MC block for unaligned address. > > > > I cannot really tell what you are trying to do here. But I know Marc > > has offered to also take a look at the GIC/LPI stuff. So I've put him > > on CC. > > > > -michael > > > > > > > > Signed-off-by: Wasim Khan <wasim.k...@nxp.com> > > > --- > > > arch/arm/cpu/armv8/fsl-layerscape/soc.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c > > > b/arch/arm/cpu/armv8/fsl-layerscape/soc.c > > > index d3a5cfaac1..746c93cf51 100644 > > > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c > > > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c > > > @@ -65,7 +65,7 @@ int ls_gic_rd_tables_init(void *blob) > > > u64 gic_lpi_base; > > > int ret; > > > > > > - gic_lpi_base = ALIGN(gd->arch.resv_ram - GIC_LPI_SIZE, SZ_64K); > > > + gic_lpi_base = ALIGN_DOWN(gd->arch.resv_ram - GIC_LPI_SIZE, > > > +SZ_64K); > > > ret = fdt_add_resv_mem_gic_rd_tables(blob, gic_lpi_base, > > > GIC_LPI_SIZE); > > > if (ret) > > > return ret; > > > > It is the usual accumulation of nonsense. We have > > #define ITS_MAX_LPI_NRBITS 16 > > which is not necessarily what the HW exposes > > #define PENDTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS), SZ_64K) > > The *base* of the pending table has to be 64kB aligned, but not its size. Yes, > that's a helpful shortcut, but that's still wrong. > > #define PROPTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS) / 8, > SZ_64K) > > This 64kB alignment is silly, specially considering the hardcoding of the > number of ID bits. > > #define GIC_LPI_SIZE ALIGN(cpu_numcores() * PENDTABLE_MAX_SZ + > \ > PROPTABLE_MAX_SZ, SZ_1M) > > This 1MB alignment doesn't exist. Convenience again? > > And then this patch adds some bizarre alignment for reasons that have > nothing to do with the GIC, but because there is so other reservations this > steps on, which probably means that the allocator is doing something wrong... > > The whole thing needs reworking from first principle, at which point it will > become clearer what this is trying to do. > > M. > > -- > Without deviation from the norm, progress is not possible.
Added Zhiqiang to respond to Marc's questions. Regarding this patch: if gd->arch.resv_ram points to some address which is not 64K aligned (as per current code), Using ALIGN can be problematic in that case. Ex: Currently MC reserved regions is [0x27_8000_0000 , 0x27_FFFF_FFFF] gd->arch.resv_ram point to 0x27_8000_0000. Now suppose if I have requirement to reserve a block (say XYZ) of 32KB size just before MC block, i will update gd->arch.resv_ram to point to 0x27_7FFF_8000. Now, Calling ' gic_lpi_base = ALIGN(gd->arch.resv_ram - GIC_LPI_SIZE, SZ_64K);' will eat up space reserved for XYZ. So, I think ALIGN_DOWN should be used for gic_lpi_base . gic_lpi_base = ALIGN_DOWN(gd->arch.resv_ram - GIC_LPI_SIZE, SZ_64K);
