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

[Michael Walle]

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 at 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 at 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.

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