[PATCH] armv8: fsl-layerscape: use previous aligned address for gic_lpi_base

[Z.Q. Hou]

Hi Wasim, Marc and all,

> -----Original Message-----
> From: Wasim Khan (OSS) <wasim.khan at oss.nxp.com>
> Sent: 2022年2月21日 21:23
> To: Marc Zyngier <maz at kernel.org>; Michael Walle <michael at walle.cc>;
> Z.Q. Hou <zhiqiang.hou at nxp.com>
> Cc: Wasim Khan (OSS) <wasim.khan at oss.nxp.com>; sjg at chromium.org;
> Priyanka Jain <priyanka.jain at nxp.com>; treding at nvidia.com;
> twarren at nvidia.com; Varun Sethi <V.Sethi at nxp.com>;
> u-boot at lists.denx.de
> Subject: RE: [PATCH] armv8: fsl-layerscape: use previous aligned address for
> gic_lpi_base
> 
> Hi Marc, Zhiqiang
> 
> > -----Original Message-----
> > From: Marc Zyngier <maz at kernel.org>
> > Sent: Monday, February 21, 2022 4:23 PM
> > To: Michael Walle <michael at walle.cc>
> > Cc: Wasim Khan (OSS) <wasim.khan at oss.nxp.com>; sjg at chromium.org;
> > Priyanka Jain <priyanka.jain at nxp.com>; treding at nvidia.com;
> > twarren at nvidia.com; Varun Sethi <V.Sethi at nxp.com>; u-
> > boot at lists.denx.de; Wasim Khan <wasim.khan at 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 <michael at walle.cc> wrote:
> > >
> > > Hi,
> > >
> > > Am 2022-02-21 11:16, schrieb Wasim Khan:
> > > > From: Wasim Khan <wasim.khan at 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.khan at 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.

I've submit a patch to recode the whole gic-v3-its.c, with that patch we don't need these platform specific code anymore, I pasted its link below for your reference:
http://patchwork.ozlabs.org/project/uboot/patch/20220225134106.24186-1-Zhiqiang.Hou@nxp.com/

Thanks,
Zhiqiang

> 
> 
> 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);