Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
On Sun, 31 Oct 2021 16:45:41 +,
"Z.Q. Hou" wrote:
>
>
>
> > -Original Message-
> > From: Marc Zyngier [mailto:m...@kernel.org]
> > Sent: 2021年10月29日 5:09
> > To: Michael Walle
> > Cc: u-boot@lists.denx.de; Vladimir Oltean ; Z.Q.
> > Hou
> > ; Bharat Gooty ;
> > Rayagonda Kokatanur ; Simon Glass
> > ; Priyanka Jain ; Tom Rini
> >
> > Subject: Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get
> > gic
> > lpi details"
> >
> > On Wed, 27 Oct 2021 17:54:54 +0100,
> > Michael Walle wrote:
> > >
> > > Stop using the device tree as a source for ad-hoc information.
> > >
> > > This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310.
> > >
> > > Signed-off-by: Michael Walle
> > > ---
> > > arch/arm/Kconfig| 2 -
> > > arch/arm/cpu/armv8/fsl-layerscape/soc.c | 27 +-
> > > arch/arm/include/asm/gic-v3.h | 4 +-
> > > arch/arm/lib/gic-v3-its.c | 66 +++--
> > > 4 files changed, 36 insertions(+), 63 deletions(-)
> > >
> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index
> > > 02f8306f15..86c1ebde05 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -82,8 +82,6 @@ config GICV3
> > >
> > > config GIC_V3_ITS
> > > bool "ARM GICV3 ITS"
> > > - select REGMAP
> > > - select SYSCON
> > > select IRQ
> > > help
> > > ARM GICV3 Interrupt translation service (ITS).
> > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > > b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > > index c0e100d21c..a08ed3f544 100644
> > > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> >
> > Why is this FSL specific?
> >
> > > @@ -41,11 +41,36 @@ DECLARE_GLOBAL_DATA_PTR; #endif
> > >
> > > #ifdef CONFIG_GIC_V3_ITS
> > > +#define PENDTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS), SZ_64K)
> > > +#define PROPTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS) / 8,
> > SZ_64K)
> >
> > This looks completely wrong.
> >
> > The pending table needs one bit per LPI, and the property table one byte
> > per LPI.
> > Here, you have it the other way around.
>
> It's a typo, will fix after the revert patch applied.
A typo that has the potential to corrupt to corrupt memory. This
clearly was never tested.
>
> > Also, the property table alignment requirement is 4kB, not 64kB,
> > and its size is defined as the maximum number of LPIs - 8192.
>
> As in the accessor gic_lpi_tables_init() there isn't alignment
> operation for both property table and pending table, we have to pass
> a 64KB alignment address, even though the property table only
> requires 4KB alignment.
So how about fixing it?
>
> >
> > Finally, ITS_MAX_LPI_NRBITS is hardcoded to 16, while it can
> > actually vary from 14 to 32 (and even further limited by some
> > hypervisors), depending on the implementation. Granted, this was
> > broken before this patch, and in most cases, 64k is more than
> > enough.
> >
>
> This is only for Layerscape platforms, so hardcoded to 16 bit works.
Let me say it again: hardcoding things for a specific platform for no
good reason is utterly wasteful. There is no reason to write SoC
specific code here, and allocating tables should be done in an
architectural way.
>
> > However, given that this defining the number of LPIs for the
> > lifetime of the system, it would be better to actually allocate
> > what the HW advertises (GICD_TYPER.IDbits, capped by
> > GICD_TYPER.num_LPIs).
> >
> > > +#define GIC_LPI_SIZE ALIGN(cpu_numcores() * PENDTABLE_MAX_SZ
> > > + \
> > > + PROPTABLE_MAX_SZ, SZ_1M)
> >
> > Why the 1MB alignment? There is no such requirement in the
> > architecture (64kB for the pending tables, 4kB for the property
> > table).
>
> This is definition of the size instead of address, 1MB size
> alignment is to ensure we have enough space to do address alignment,
> perhaps 64KB should be enough.
Again: straying out of the architecture requirement is wasteful. The
architecture states all the requirements. How about you follow them
instead of picking random numbers?
>
> >
> > > +static int fdt_add_resv_mem_gic_rd_tables(void *blob, u64 base,
> > > +size_t size) {
> > > + int err;
>
Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
Hi, Am 2021-10-31 17:45, schrieb Z.Q. Hou: -Original Message- From: Marc Zyngier [mailto:m...@kernel.org] Sent: 2021年10月29日 5:09 To: Michael Walle Cc: u-boot@lists.denx.de; Vladimir Oltean ; Z.Q. Hou ; Bharat Gooty ; Rayagonda Kokatanur ; Simon Glass ; Priyanka Jain ; Tom Rini Subject: Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details" On Wed, 27 Oct 2021 17:54:54 +0100, Michael Walle wrote: > > Stop using the device tree as a source for ad-hoc information. > > This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310. > > Signed-off-by: Michael Walle > --- > arch/arm/Kconfig| 2 - > arch/arm/cpu/armv8/fsl-layerscape/soc.c | 27 +- > arch/arm/include/asm/gic-v3.h | 4 +- > arch/arm/lib/gic-v3-its.c | 66 +++-- > 4 files changed, 36 insertions(+), 63 deletions(-) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index > 02f8306f15..86c1ebde05 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -82,8 +82,6 @@ config GICV3 > > config GIC_V3_ITS >bool "ARM GICV3 ITS" > - select REGMAP > - select SYSCON >select IRQ >help > ARM GICV3 Interrupt translation service (ITS). > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c > b/arch/arm/cpu/armv8/fsl-layerscape/soc.c > index c0e100d21c..a08ed3f544 100644 > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c Why is this FSL specific? > @@ -41,11 +41,36 @@ DECLARE_GLOBAL_DATA_PTR; #endif > > #ifdef CONFIG_GIC_V3_ITS > +#define PENDTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS), SZ_64K) > +#define PROPTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS) / 8, SZ_64K) This looks completely wrong. The pending table needs one bit per LPI, and the property table one byte per LPI. Here, you have it the other way around. It's a typo, will fix after the revert patch applied. Please keep me on CC. .. Finally, ITS_MAX_LPI_NRBITS is hardcoded to 16, while it can actually vary from 14 to 32 (and even further limited by some hypervisors), depending on the implementation. Granted, this was broken before this patch, and in most cases, 64k is more than enough. This is only for Layerscape platforms, so hardcoded to 16 bit works. But why is this layerscape only? Can't we make it so it works on any platform. If I understand Marc correctly, it should be possible. .. > + ret = gic_lpi_tables_init(gic_lpi_base, cpu_numcores()); This really should fetch the number of CPUs from the DT rather then some SoC specific black magic... Currently in most Layerscape platforms' DTS file there isn't cpu nodes. On Layerscape platforms the implemented core number can be get from GUT register. So this would be the perfect time to actually sync the device trees with linux. -michael
RE: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
> -Original Message-
> From: Marc Zyngier [mailto:m...@kernel.org]
> Sent: 2021年10月29日 5:09
> To: Michael Walle
> Cc: u-boot@lists.denx.de; Vladimir Oltean ; Z.Q. Hou
> ; Bharat Gooty ;
> Rayagonda Kokatanur ; Simon Glass
> ; Priyanka Jain ; Tom Rini
>
> Subject: Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get
> gic
> lpi details"
>
> On Wed, 27 Oct 2021 17:54:54 +0100,
> Michael Walle wrote:
> >
> > Stop using the device tree as a source for ad-hoc information.
> >
> > This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310.
> >
> > Signed-off-by: Michael Walle
> > ---
> > arch/arm/Kconfig| 2 -
> > arch/arm/cpu/armv8/fsl-layerscape/soc.c | 27 +-
> > arch/arm/include/asm/gic-v3.h | 4 +-
> > arch/arm/lib/gic-v3-its.c | 66 +++--
> > 4 files changed, 36 insertions(+), 63 deletions(-)
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index
> > 02f8306f15..86c1ebde05 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -82,8 +82,6 @@ config GICV3
> >
> > config GIC_V3_ITS
> > bool "ARM GICV3 ITS"
> > - select REGMAP
> > - select SYSCON
> > select IRQ
> > help
> > ARM GICV3 Interrupt translation service (ITS).
> > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > index c0e100d21c..a08ed3f544 100644
> > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>
> Why is this FSL specific?
>
> > @@ -41,11 +41,36 @@ DECLARE_GLOBAL_DATA_PTR; #endif
> >
> > #ifdef CONFIG_GIC_V3_ITS
> > +#define PENDTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS), SZ_64K)
> > +#define PROPTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS) / 8,
> SZ_64K)
>
> This looks completely wrong.
>
> The pending table needs one bit per LPI, and the property table one byte per
> LPI.
> Here, you have it the other way around.
It's a typo, will fix after the revert patch applied.
> Also, the property table alignment
> requirement is 4kB, not 64kB, and its size is defined as the maximum number of
> LPIs - 8192.
As in the accessor gic_lpi_tables_init() there isn't alignment operation for
both property table and pending table, we have to pass a 64KB alignment
address, even though the property table only requires 4KB alignment.
>
> Finally, ITS_MAX_LPI_NRBITS is hardcoded to 16, while it can actually vary
> from 14
> to 32 (and even further limited by some hypervisors), depending on the
> implementation. Granted, this was broken before this patch, and in most cases,
> 64k is more than enough.
>
This is only for Layerscape platforms, so hardcoded to 16 bit works.
> However, given that this defining the number of LPIs for the lifetime of the
> system,
> it would be better to actually allocate what the HW advertises
> (GICD_TYPER.IDbits,
> capped by GICD_TYPER.num_LPIs).
>
> > +#define GIC_LPI_SIZE ALIGN(cpu_numcores() * PENDTABLE_MAX_SZ
> > + \
> > + PROPTABLE_MAX_SZ, SZ_1M)
>
> Why the 1MB alignment? There is no such requirement in the architecture (64kB
> for the pending tables, 4kB for the property table).
This is definition of the size instead of address, 1MB size alignment is to
ensure we have enough space to do address alignment, perhaps 64KB should be
enough.
>
> > +static int fdt_add_resv_mem_gic_rd_tables(void *blob, u64 base,
> > +size_t size) {
> > + int err;
> > + struct fdt_memory gic_rd_tables;
> > +
> > + gic_rd_tables.start = base;
> > + gic_rd_tables.end = base + size - 1;
> > + err = fdtdec_add_reserved_memory(blob, "gic-rd-tables", _rd_tables,
> > +NULL, 0, NULL, 0);
> > + if (err < 0)
> > + debug("%s: failed to add reserved memory: %d\n", __func__, err);
> > +
> > + return err;
> > +}
> > +
> > int ls_gic_rd_tables_init(void *blob) {
> > + u64 gic_lpi_base;
> > int ret;
> >
> > - ret = gic_lpi_tables_init();
> > + gic_lpi_base = ALIGN(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;
> > +
> > + ret = gic_lpi_tables_init(gic_lpi_base, cpu_numcores());
>
> This really should fetch the number of CPUs from the DT rather then some SoC
> specific black magic...
C
Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
On Wed, Oct 27, 2021 at 06:54:54PM +0200, Michael Walle wrote: > Stop using the device tree as a source for ad-hoc information. > > This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310. > > Signed-off-by: Michael Walle With a quick change to make ns3 build but note the problem at runtime, applied to u-boot/master, thanks. And note that I expect a follow-up from interested parties to implement the required changes here correctly. -- Tom signature.asc Description: PGP signature
Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
Michael,
On Fri, 29 Oct 2021 12:49:21 +0100,
Michael Walle wrote:
>
> Hi,
>
> thanks for the review, as Tom already mentioned this is just
> a revert. I'm still unsure wether I should care or not. Actually,
> NXP or Broadcom should. OTOH it would be nice if the kontron_sl28
> can do kexec also with booti (and not just bootefi). Anyway, I
> still have some remarks.
>
> Am 2021-10-28 23:09, schrieb Marc Zyngier:
> > On Wed, 27 Oct 2021 17:54:54 +0100,
> > Michael Walle wrote:
> >>
> >> Stop using the device tree as a source for ad-hoc information.
> >>
> >> This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310.
> >>
> >> Signed-off-by: Michael Walle
> >> ---
>
> >> int ls_gic_rd_tables_init(void *blob)
> >> {
> >> + u64 gic_lpi_base;
> >>int ret;
> >>
> >> - ret = gic_lpi_tables_init();
> >> + gic_lpi_base = ALIGN(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;
> >> +
> >> + ret = gic_lpi_tables_init(gic_lpi_base, cpu_numcores());
> >
> > This really should fetch the number of CPUs from the DT rather then
> > some SoC specific black magic...
>
> Remember that this is the bootloader. There might be a chicken egg
> problem. I guess, usually the bootloader will fixup the number of cores
> in the DT. Therefore, if we rely on the DT here, it has to be fixed up
> before this code is run.
I appreciate that. However...
>
> Conceptually, I'm unsure if this should actually be a driver
> (UCLASS_IRQ). It's just setting up the interrupt controller, it doesn't
> provide any ops. So it might well be called from somewhere else instead
> of binding as a driver to the gic.
>
> If it will be a driver, then the call to gic_lpi_tables_init() should
> go away. Instead the driver should just set the tables up.
>
> If its not a driver, then gic_lpi_tables_init() (maybe renamed to
> gic_v3_lpi_tables_init()) should work without anything else and
> it should scan the device tree for the compatible node and fetch
> all needed information to set up the tables.
Exactly. This thing doesn't provide *any* service to u-boot itself. It
just grabs some memory, point a couple of registers at it, and flip a
bit. There is no actual ITS driver, so nothing makes use of it.
So this can be run as late as you want, long after you have worked out
how many CPUs you really have. Which means this can be done in a
SoC-agnostic way.
> In any case, all this code should be guarded by a Kconfig option which
> clearly states *why* this is needed.
Even more: if it is selected, it should be possible to disable this at
runtime (environment variable?) and leave the OS in charge of it. This
really should an opt-in, instead of being forced on the users.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
Hi,
thanks for the review, as Tom already mentioned this is just
a revert. I'm still unsure wether I should care or not. Actually,
NXP or Broadcom should. OTOH it would be nice if the kontron_sl28
can do kexec also with booti (and not just bootefi). Anyway, I
still have some remarks.
Am 2021-10-28 23:09, schrieb Marc Zyngier:
On Wed, 27 Oct 2021 17:54:54 +0100,
Michael Walle wrote:
Stop using the device tree as a source for ad-hoc information.
This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310.
Signed-off-by: Michael Walle
---
int ls_gic_rd_tables_init(void *blob)
{
+ u64 gic_lpi_base;
int ret;
- ret = gic_lpi_tables_init();
+ gic_lpi_base = ALIGN(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;
+
+ ret = gic_lpi_tables_init(gic_lpi_base, cpu_numcores());
This really should fetch the number of CPUs from the DT rather then
some SoC specific black magic...
Remember that this is the bootloader. There might be a chicken egg
problem. I guess, usually the bootloader will fixup the number of cores
in the DT. Therefore, if we rely on the DT here, it has to be fixed up
before this code is run.
Conceptually, I'm unsure if this should actually be a driver
(UCLASS_IRQ). It's just setting up the interrupt controller, it doesn't
provide any ops. So it might well be called from somewhere else instead
of binding as a driver to the gic.
If it will be a driver, then the call to gic_lpi_tables_init() should
go away. Instead the driver should just set the tables up.
If its not a driver, then gic_lpi_tables_init() (maybe renamed to
gic_v3_lpi_tables_init()) should work without anything else and
it should scan the device tree for the compatible node and fetch
all needed information to set up the tables.
In any case, all this code should be guarded by a Kconfig option which
clearly states *why* this is needed.
Thoughts?
--
-michael
Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
On Thu, Oct 28, 2021 at 10:09:25PM +0100, Marc Zyngier wrote: > On Wed, 27 Oct 2021 17:54:54 +0100, > Michael Walle wrote: > > > > Stop using the device tree as a source for ad-hoc information. > > > > This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310. > > > > Signed-off-by: Michael Walle > > --- > > arch/arm/Kconfig| 2 - > > arch/arm/cpu/armv8/fsl-layerscape/soc.c | 27 +- > > arch/arm/include/asm/gic-v3.h | 4 +- > > arch/arm/lib/gic-v3-its.c | 66 +++-- > > 4 files changed, 36 insertions(+), 63 deletions(-) > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > index 02f8306f15..86c1ebde05 100644 > > --- a/arch/arm/Kconfig > > +++ b/arch/arm/Kconfig > > @@ -82,8 +82,6 @@ config GICV3 > > > > config GIC_V3_ITS > > bool "ARM GICV3 ITS" > > - select REGMAP > > - select SYSCON > > select IRQ > > help > > ARM GICV3 Interrupt translation service (ITS). > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c > > b/arch/arm/cpu/armv8/fsl-layerscape/soc.c > > index c0e100d21c..a08ed3f544 100644 > > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c > > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c > > Why is this FSL specific? Note that this is a fairly direct revert. Based on your comments a follow-up patch to correct things is in order. -- Tom signature.asc Description: PGP signature
Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
On Wed, 27 Oct 2021 17:54:54 +0100,
Michael Walle wrote:
>
> Stop using the device tree as a source for ad-hoc information.
>
> This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310.
>
> Signed-off-by: Michael Walle
> ---
> arch/arm/Kconfig| 2 -
> arch/arm/cpu/armv8/fsl-layerscape/soc.c | 27 +-
> arch/arm/include/asm/gic-v3.h | 4 +-
> arch/arm/lib/gic-v3-its.c | 66 +++--
> 4 files changed, 36 insertions(+), 63 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 02f8306f15..86c1ebde05 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -82,8 +82,6 @@ config GICV3
>
> config GIC_V3_ITS
> bool "ARM GICV3 ITS"
> - select REGMAP
> - select SYSCON
> select IRQ
> help
> ARM GICV3 Interrupt translation service (ITS).
> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> index c0e100d21c..a08ed3f544 100644
> --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
Why is this FSL specific?
> @@ -41,11 +41,36 @@ DECLARE_GLOBAL_DATA_PTR;
> #endif
>
> #ifdef CONFIG_GIC_V3_ITS
> +#define PENDTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS), SZ_64K)
> +#define PROPTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS) / 8, SZ_64K)
This looks completely wrong.
The pending table needs one bit per LPI, and the property table one
byte per LPI. Here, you have it the other way around. Also, the
property table alignment requirement is 4kB, not 64kB, and its size is
defined as the maximum number of LPIs - 8192.
Finally, ITS_MAX_LPI_NRBITS is hardcoded to 16, while it can actually
vary from 14 to 32 (and even further limited by some hypervisors),
depending on the implementation. Granted, this was broken before this
patch, and in most cases, 64k is more than enough.
However, given that this defining the number of LPIs for the lifetime
of the system, it would be better to actually allocate what the HW
advertises (GICD_TYPER.IDbits, capped by GICD_TYPER.num_LPIs).
> +#define GIC_LPI_SIZE ALIGN(cpu_numcores() * PENDTABLE_MAX_SZ + \
> + PROPTABLE_MAX_SZ, SZ_1M)
Why the 1MB alignment? There is no such requirement in the
architecture (64kB for the pending tables, 4kB for the property
table).
> +static int fdt_add_resv_mem_gic_rd_tables(void *blob, u64 base, size_t size)
> +{
> + int err;
> + struct fdt_memory gic_rd_tables;
> +
> + gic_rd_tables.start = base;
> + gic_rd_tables.end = base + size - 1;
> + err = fdtdec_add_reserved_memory(blob, "gic-rd-tables", _rd_tables,
> + NULL, 0, NULL, 0);
> + if (err < 0)
> + debug("%s: failed to add reserved memory: %d\n", __func__, err);
> +
> + return err;
> +}
> +
> int ls_gic_rd_tables_init(void *blob)
> {
> + u64 gic_lpi_base;
> int ret;
>
> - ret = gic_lpi_tables_init();
> + gic_lpi_base = ALIGN(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;
> +
> + ret = gic_lpi_tables_init(gic_lpi_base, cpu_numcores());
This really should fetch the number of CPUs from the DT rather then
some SoC specific black magic...
> if (ret)
> debug("%s: failed to init gic-lpi-tables\n", __func__);
>
> diff --git a/arch/arm/include/asm/gic-v3.h b/arch/arm/include/asm/gic-v3.h
> index 35efec78c3..5131fabec4 100644
> --- a/arch/arm/include/asm/gic-v3.h
> +++ b/arch/arm/include/asm/gic-v3.h
> @@ -127,9 +127,9 @@
> #define GIC_REDISTRIBUTOR_OFFSET 0x2
>
> #ifdef CONFIG_GIC_V3_ITS
> -int gic_lpi_tables_init(void);
> +int gic_lpi_tables_init(u64 base, u32 max_redist);
> #else
> -int gic_lpi_tables_init(void)
> +int gic_lpi_tables_init(u64 base, u32 max_redist)
> {
> return 0;
> }
> diff --git a/arch/arm/lib/gic-v3-its.c b/arch/arm/lib/gic-v3-its.c
> index 2d3fdb600e..f6211a2d92 100644
> --- a/arch/arm/lib/gic-v3-its.c
> +++ b/arch/arm/lib/gic-v3-its.c
> @@ -5,8 +5,6 @@
> #include
> #include
> #include
> -#include
> -#include
> #include
> #include
> #include
> @@ -19,22 +17,15 @@ static u32 lpi_id_bits;
> #define LPI_PROPBASE_SZ ALIGN(BIT(LPI_NRBITS), SZ_64K)
> #define LPI_PENDBASE_SZ ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
This is marginally more correct, but you have to wonder why you have
the same thing defined twice...
> -/* Number of GIC re-distributors */
> -#define MAX_GIC_REDISTRIBUTORS 8
> -
> /*
> * gic_v3_its_priv - gic details
> *
> * @gicd_base: gicd base address
> * @gicr_base: gicr base address
> - * @lpi_base: gic lpi base address
> - * @num_redist: number of gic re-distributors
> */
> struct gic_v3_its_priv {
> ulong gicd_base;
> ulong gicr_base;
> - ulong lpi_base;
> - u32 num_redist;
>
[PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
Stop using the device tree as a source for ad-hoc information.
This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310.
Signed-off-by: Michael Walle
---
arch/arm/Kconfig| 2 -
arch/arm/cpu/armv8/fsl-layerscape/soc.c | 27 +-
arch/arm/include/asm/gic-v3.h | 4 +-
arch/arm/lib/gic-v3-its.c | 66 +++--
4 files changed, 36 insertions(+), 63 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 02f8306f15..86c1ebde05 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -82,8 +82,6 @@ config GICV3
config GIC_V3_ITS
bool "ARM GICV3 ITS"
- select REGMAP
- select SYSCON
select IRQ
help
ARM GICV3 Interrupt translation service (ITS).
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
index c0e100d21c..a08ed3f544 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
+++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
@@ -41,11 +41,36 @@ DECLARE_GLOBAL_DATA_PTR;
#endif
#ifdef CONFIG_GIC_V3_ITS
+#define PENDTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS), SZ_64K)
+#define PROPTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS) / 8, SZ_64K)
+#define GIC_LPI_SIZE ALIGN(cpu_numcores() * PENDTABLE_MAX_SZ + \
+ PROPTABLE_MAX_SZ, SZ_1M)
+static int fdt_add_resv_mem_gic_rd_tables(void *blob, u64 base, size_t size)
+{
+ int err;
+ struct fdt_memory gic_rd_tables;
+
+ gic_rd_tables.start = base;
+ gic_rd_tables.end = base + size - 1;
+ err = fdtdec_add_reserved_memory(blob, "gic-rd-tables", _rd_tables,
+NULL, 0, NULL, 0);
+ if (err < 0)
+ debug("%s: failed to add reserved memory: %d\n", __func__, err);
+
+ return err;
+}
+
int ls_gic_rd_tables_init(void *blob)
{
+ u64 gic_lpi_base;
int ret;
- ret = gic_lpi_tables_init();
+ gic_lpi_base = ALIGN(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;
+
+ ret = gic_lpi_tables_init(gic_lpi_base, cpu_numcores());
if (ret)
debug("%s: failed to init gic-lpi-tables\n", __func__);
diff --git a/arch/arm/include/asm/gic-v3.h b/arch/arm/include/asm/gic-v3.h
index 35efec78c3..5131fabec4 100644
--- a/arch/arm/include/asm/gic-v3.h
+++ b/arch/arm/include/asm/gic-v3.h
@@ -127,9 +127,9 @@
#define GIC_REDISTRIBUTOR_OFFSET 0x2
#ifdef CONFIG_GIC_V3_ITS
-int gic_lpi_tables_init(void);
+int gic_lpi_tables_init(u64 base, u32 max_redist);
#else
-int gic_lpi_tables_init(void)
+int gic_lpi_tables_init(u64 base, u32 max_redist)
{
return 0;
}
diff --git a/arch/arm/lib/gic-v3-its.c b/arch/arm/lib/gic-v3-its.c
index 2d3fdb600e..f6211a2d92 100644
--- a/arch/arm/lib/gic-v3-its.c
+++ b/arch/arm/lib/gic-v3-its.c
@@ -5,8 +5,6 @@
#include
#include
#include
-#include
-#include
#include
#include
#include
@@ -19,22 +17,15 @@ static u32 lpi_id_bits;
#define LPI_PROPBASE_SZALIGN(BIT(LPI_NRBITS), SZ_64K)
#define LPI_PENDBASE_SZALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
-/* Number of GIC re-distributors */
-#define MAX_GIC_REDISTRIBUTORS 8
-
/*
* gic_v3_its_priv - gic details
*
* @gicd_base: gicd base address
* @gicr_base: gicr base address
- * @lpi_base: gic lpi base address
- * @num_redist: number of gic re-distributors
*/
struct gic_v3_its_priv {
ulong gicd_base;
ulong gicr_base;
- ulong lpi_base;
- u32 num_redist;
};
static int gic_v3_its_get_gic_addr(struct gic_v3_its_priv *priv)
@@ -68,39 +59,13 @@ static int gic_v3_its_get_gic_addr(struct gic_v3_its_priv
*priv)
return 0;
}
-static int gic_v3_its_get_gic_lpi_addr(struct gic_v3_its_priv *priv)
-{
- struct regmap *regmap;
- struct udevice *dev;
- int ret;
-
- ret = uclass_get_device_by_driver(UCLASS_SYSCON,
- DM_DRIVER_GET(gic_lpi_syscon), );
- if (ret) {
- pr_err("%s: failed to get %s syscon device\n", __func__,
- DM_DRIVER_GET(gic_lpi_syscon)->name);
- return ret;
- }
-
- regmap = syscon_get_regmap(dev);
- if (!regmap) {
- pr_err("%s: failed to regmap for %s syscon device\n", __func__,
- DM_DRIVER_GET(gic_lpi_syscon)->name);
- return -ENODEV;
- }
- priv->lpi_base = regmap->ranges[0].start;
-
- priv->num_redist = dev_read_u32_default(dev, "max-gic-redistributors",
- MAX_GIC_REDISTRIBUTORS);
-
- return 0;
-}
-
/*
* Program the GIC LPI configuration tables for all
* the re-distributors and enable the LPI table
+ * base: Configuration table address
+ * num_redist: number of redistributors
*/
-int
