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

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

[Wasim Khan]

From: Wasim Khan <wasim.khan@nxp.com>

Memory after gd->arch.resv_ram is reserved for MC block.
Use ALIGN_DOWN to avoid updating MC block for unaligned
address.

Signed-off-by: Wasim Khan <wasim.khan@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;
-- 
2.25.1

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

[Michael Walle]

Hi,

Am 2022-02-21 11:16, schrieb Wasim Khan:
> From: Wasim Khan <wasim.khan@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@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;

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

[Marc Zyngier]

On Mon, 21 Feb 2022 10:24:36 +0000,
Michael Walle <michael@walle.cc> wrote:
> 
> Hi,
> 
> Am 2022-02-21 11:16, schrieb Wasim Khan:
> > From: Wasim Khan <wasim.khan@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@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.

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

[Wasim Khan (OSS)]

Hi Marc, Zhiqiang

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Monday, February 21, 2022 4:23 PM
> To: Michael Walle <michael@walle.cc>
> Cc: Wasim Khan (OSS) <wasim.khan@oss.nxp.com>; sjg@chromium.org;
> Priyanka Jain <priyanka.jain@nxp.com>; treding@nvidia.com;
> twarren@nvidia.com; Varun Sethi <V.Sethi@nxp.com>; u-
> boot@lists.denx.de; Wasim Khan <wasim.khan@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@walle.cc> wrote:
> >
> > Hi,
> >
> > Am 2022-02-21 11:16, schrieb Wasim Khan:
> > > From: Wasim Khan <wasim.khan@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@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);

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