[v5,13/17] irqchip/riscv-intc: Add ACPI support for AIA

The RINTC subtype structure in MADT also has information about other
interrupt controllers. Save this information and provide interfaces to
retrieve them when required by corresponding drivers.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/irq.h     |  35 +++++++++++
 drivers/irqchip/irq-riscv-intc.c | 102 ++++++++++++++++++++++++++++++-
 2 files changed, 135 insertions(+), 2 deletions(-)

On Wed, May 01 2024 at 17:47, Sunil V L wrote:
> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> index 9e71c4428814..af7a2f78f0ee 100644
> --- a/drivers/irqchip/irq-riscv-intc.c
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -249,14 +249,105 @@ IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
>  IRQCHIP_DECLARE(andes, "andestech,cpu-intc", riscv_intc_init);
>  
>  #ifdef CONFIG_ACPI
> +struct rintc_data {
> +	u32 ext_intc_id;
> +	unsigned long hart_id;
> +	u64 imsic_addr;
> +	u32 imsic_size;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

> +};
> +
> +static u32 nr_rintc;
> +static struct rintc_data *rintc_acpi_data[NR_CPUS];
> +
> +int acpi_get_intc_index_hartid(u32 index, unsigned long *hartid)

Why int? All of these functions have strictly boolean return values:
success = true, fail = false, no?

Either bool or get rid of the pointer and let the function return
either the real hart id or an invalid one.

> +{
> +	if (index >= nr_rintc)
> +		return -1;
> +
> +	*hartid = rintc_acpi_data[index]->hart_id;
> +	return 0;

I.e.

	return index >= nr_rintc ? rintc_acpi_data[index]->hart_id : INVALID_HART_ID;

> +int acpi_get_ext_intc_parent_hartid(u8 id, u32 idx, unsigned long *hartid)
> +{
> +	int i, j = 0;
> +
> +	for (i = 0; i < nr_rintc; i++) {
> +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id) {
> +			if (idx == j) {
> +				*hartid = rintc_acpi_data[i]->hart_id;
> +				return 0;
> +			}
> +			j++;
> +		}
> +	}
> +
> +	return -1;
> +}
> +
> +void acpi_get_plic_nr_contexts(u8 id, int *nr_contexts)
> +{
> +	int i, j = 0;
> +
> +	for (i = 0; i < nr_rintc; i++) {
> +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id)
> +			j++;
> +	}
> +
> +	*nr_contexts = j;
> +}
> +
> +int acpi_get_plic_context(u8 id, u32 idx, int *context_id)
> +{
> +	int i, j = 0;
> +
> +	for (i = 0; i < nr_rintc; i++) {
> +		if (APLIC_PLIC_ID(rintc_acpi_data[i]->ext_intc_id) == id) {
> +			if (idx == j) {
> +				*context_id = IDC_CONTEXT_ID(rintc_acpi_data[i]->ext_intc_id);
> +				return 0;
> +			}
> +
> +			j++;
> +		}
> +	}

So that's the third incarnation of the same loop with the truly self
explaining variable and argument names.

    j is actually the index of the context which is associated to a
    given PLIC ID.

    idx is the context index to search for

Right? So why can't these things be named in a way which makes the
intent of the code clear?

Also why are all the arguments u8/u32? There is no hardware
involved. Simple 'unsigned int' is just fine and the u8/u32 is not bying
you anything here.

Aside of that these ugly macros can be completely avoided and the code
can be written without a copy & pasta orgy.

struct rintc_data {
	union {
		u32		ext_intc_id;
                struct {
                	u32	context_id	: 16,
                        			:  8,
                        	aplic_plic_id	:  8;
                }
        };
	unsigned long		hart_id;
	u64			imsic_addr;
	u32			imsic_size;
};

#define for_each_matching_plic(_plic, _plic_id)				\
	for (_plic = 0; _plic < nr_rintc; _plict++)			\
        	if (rintc_acpi_data[_plic]->aplic_plic_id != _plic_id)	\
                	continue;					\
                else

unsigned int acpi_get_plic_nr_contexts(unsigned int plic_id)
{
	unsigned int nctx = 0;

	for_each_matching_plic(plic, plic_id)
		nctx++;

	return nctx;
}

static struct rintc_data *get_plic_context(unsigned int plic_id, unsigned int ctxt_idx)
{
	unsigned int ctxt = 0;

	for_each_matching_plic(plic, plic_id) {
        	if (ctxt == ctxt_idx)
                	return rintc_acpi_data + plic;
        }
        return NULL;
}

unsigned long acpi_get_ext_intc_parent_hartid(unsigned int plic_id, unsigned int ctxt_idx)
{
        struct rintc_data *data = get_plic_context(plic_id, ctxt_idx);

        return data ? data->hart_id : INVALID_HART_ID;
}

unsigned int acpi_get_plic_context(unsigned int plic_id, unsigned int ctxt_idx)
{
        struct rintc_data *data = get_plic_context(plic_id, ctxt_idx);

        return data ? data->context_id : INVALID_CONTEXT;
}

Or something like that. Hmm?

> +int acpi_get_imsic_mmio_info(u32 index, struct resource *res)
> +{
> +	if (index >= nr_rintc)
> +		return -1;
> +
> +	res->start = rintc_acpi_data[index]->imsic_addr;
> +	res->end = res->start + rintc_acpi_data[index]->imsic_size - 1;
> +	res->flags = IORESOURCE_MEM;
> +	return 0;
> +}
> +
> +static struct fwnode_handle *ext_entc_get_gsi_domain_id(u32 gsi)
> +{
> +	return riscv_acpi_get_gsi_domain_id(gsi);
> +}

This wrapper is required because using riscv_acpi_get_gsi_domain_id()
directly is too obvious, right?

>  static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
>  				       const unsigned long end)
>  {
> -	struct fwnode_handle *fn;
>  	struct acpi_madt_rintc *rintc;
> +	struct fwnode_handle *fn;
> +	int rc;
>  
>  	rintc = (struct acpi_madt_rintc *)header;
> +	rintc_acpi_data[nr_rintc] = kzalloc(sizeof(*rintc_acpi_data[0]), GFP_KERNEL);
> +	if (!rintc_acpi_data[nr_rintc])
> +		return -ENOMEM;
> +
> +	rintc_acpi_data[nr_rintc]->ext_intc_id = rintc->ext_intc_id;
> +	rintc_acpi_data[nr_rintc]->hart_id = rintc->hart_id;
> +	rintc_acpi_data[nr_rintc]->imsic_addr = rintc->imsic_addr;
> +	rintc_acpi_data[nr_rintc]->imsic_size = rintc->imsic_size;
> +	nr_rintc++;
>  
>  	/*
>  	 * The ACPI MADT will have one INTC for each CPU (or HART)
> @@ -273,7 +364,14 @@ static int __init riscv_intc_acpi_init(union acpi_subtable_headers *header,
>  		return -ENOMEM;
>  	}
>  
> -	return riscv_intc_init_common(fn, &riscv_intc_chip);
> +	rc = riscv_intc_init_common(fn, &riscv_intc_chip);
> +	if (rc) {
> +		irq_domain_free_fwnode(fn);
> +		return rc;
> +	}

This looks like a completely unrelated bug fix. Please don't mix functional
changes and fixes.

Thanks,

        tglx