Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[Marc Zyngier <maz@kernel.org>]

Michael,

On Fri, 29 Oct 2021 12:49:21 +0100,
Michael Walle <michael@walle.cc> 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 <michael@walle.cc> wrote:
> >> 
> >> Stop using the device tree as a source for ad-hoc information.
> >> 
> >> This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310.
> >> 
> >> Signed-off-by: Michael Walle <michael@walle.cc>
> >> ---
> 
> >>  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.