Re: [PATCH v4 21/26] irqchip/gic-v5: Add GICv5 IRS/SPI support

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Thu, May 29, 2025 at 09:45:19AM +0100, Alireza Sanaee wrote:

[...]

> > > cpus is a phandle? I think this is going to run into current
> > > discussion on what phandles to CPUs on an SMT system look like (Rob
> > > Herring and Mark Rutland have different views)
> > > https://lore.kernel.org/linux-arm-kernel/20250512080715.82-1-alireza.sanaee@huawei.com/  
> > 
> > I will make sure to steer clear of that then ;-), whatever the outcome
> > the current "cpus" bindings should continue to work as-is, right ?
> > 
> > > Anyhow this doesn't look right to me.
> > > I think it should be of_count_phandle_with_args()   
> > 
> > Aren't they equivalent in functionality if
> > of_count_phandle_with_args() cells_name == NULL ?
> > 
> > I will update the code but if the functionality provided is not the
> > same there is kernel code to fix (it is an example, there are others):
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-apple-aic.c?h=v6.15#n903
> 
> I think this is fine, as long as we have always len(reg) == 1 which
> is our current case in the dt.

I don't understand what you mean. "cpus" is a list of phandles, what
has "reg" got to do with it ?

"reg" of which node ? "cpu" ? why should it be that len(reg) == 1
always ? It describes MPIDR_EL1, it depends on the "cpus" _node_
#address-cells.

I am missing something from the thread above probably.

Lorenzo

> > 
> > > Potentially with cpu-cells as the argument depending on how that
> > > thread goes.
> > >   
> > > > +	if (ncpus < 0)
> > > > +		return -EINVAL;
> > > > +
> > > > +	niaffids = of_property_count_elems_of_size(node,
> > > > "arm,iaffids",
> > > > +						   sizeof(u16));
> > > > +	if (niaffids != ncpus)
> > > > +		return -EINVAL;
> > > > +  
> > > 	u16 *iaffids __free(kfree) = kcalloc(niaffids,
> > > sizeof(*iaffids), GFP_KERNEL);  
> > 
> > Maybe I should rewrite this in Rust :)
> > 
> > > > +	iaffids = kcalloc(niaffids, sizeof(*iaffids),
> > > > GFP_KERNEL);
> > > > +	if (!iaffids)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = of_property_read_u16_array(node, "arm,iaffids",
> > > > iaffids, niaffids);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	for (i = 0; i < ncpus; i++) {
> > > > +		struct device_node *cpu_node;
> > > > +		u32 cpu_phandle;
> > > > +		int cpu;
> > > > +
> > > > +		if (of_property_read_u32_index(node, "cpus", i,
> > > > &cpu_phandle))
> > > > +			continue;
> > > > +
> > > > +		cpu_node = of_find_node_by_phandle(cpu_phandle);
> > > >  
> > > 
> > > 		cpu_node = of_parse_phandle(node, "cpus", i);
> > > 
> > > not work here?  
> > 
> > I think it would.
> > 
> > >    
> > > > +		if (WARN_ON(!cpu_node))
> > > > +			continue;
> > > > +
> > > > +		cpu = of_cpu_node_to_id(cpu_node);  
> > > 
> > > If this is all you want then Ali's series gives you a helper
> > > 
> > > 		cpu = of_cpu_phandle_to_id(node, &cpu_node, i);
> > > 
> > > Though even better to have a helper that allows
> > > 		cpu = of_cpu_phandle_to_id(node, NULL, i); and
> > > handles the node put as internally.
> > > 
> > > Ali, any reason we can't do that?  Seems to be a fairly common
> > > pattern.
> > > 
> > > 
> > >    
> > > > +		of_node_put(cpu_node);
> > > > +		if (WARN_ON(cpu < 0))
> > > > +			continue;
> > > > +
> > > > +		if (iaffids[i] & ~iaffid_mask) {
> > > > +			pr_warn("CPU %d iaffid 0x%x exceeds IRS
> > > > iaffid bits\n",
> > > > +				cpu, iaffids[i]);
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		per_cpu(cpu_iaffid, cpu).iaffid = iaffids[i];
> > > > +		per_cpu(cpu_iaffid, cpu).valid = true;
> > > > +
> > > > +		/* We also know that the CPU is connected to
> > > > this IRS */
> > > > +		per_cpu(per_cpu_irs_data, cpu) = irs_data;
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}  
> > >   
> > > > diff --git a/drivers/irqchip/irq-gic-v5.c
> > > > b/drivers/irqchip/irq-gic-v5.c index
> > > > a50982e5d98816d88e4fca37cc0ac31684fb6c76..e58ff345dbfaf840b17ad63c4fdb6c227137cf4b
> > > > 100644 --- a/drivers/irqchip/irq-gic-v5.c +++
> > > > b/drivers/irqchip/irq-gic-v5.c
> > > >
> > > > +
> > > > +static int gicv5_spi_irq_set_irqchip_state(struct irq_data *d,
> > > > +					   enum
> > > > irqchip_irq_state which,
> > > > +					   bool val)
> > > > +{  
> > > 
> > > Similar to previous, I'd call the state parameter state rather than
> > > val.  
> > 
> > Right.
> > 
> > > > diff --git a/include/linux/irqchip/arm-gic-v5.h
> > > > b/include/linux/irqchip/arm-gic-v5.h index
> > > > 4ff0ba64d9840c3844671f7850bb3d81ba2eb1b6..187af307de9170d9569898cb1e50de376a38bd0a
> > > > 100644 --- a/include/linux/irqchip/arm-gic-v5.h +++
> > > > b/include/linux/irqchip/arm-gic-v5.h @@ -5,6 +5,8 @@
> > > >  #ifndef __LINUX_IRQCHIP_ARM_GIC_V5_H
> > > >  #define __LINUX_IRQCHIP_ARM_GIC_V5_H  
> > >   
> > > >  
> > > > +#define GICV5_NO_READ_ALLOC		0b0
> > > > +#define GICV5_READ_ALLOC		0b1
> > > > +#define GICV5_NO_WRITE_ALLOC		0b0
> > > > +#define GICV5_WRITE_ALLOC		0b1  
> > > 
> > > Given these are being written to fields called _RA and _WA
> > > so the defines provide value over 0 and 1 in appropriate places?
> > > Maybe just about. Anyhow, your code so on this up to you.
> > >   
> > > > +
> > > > +#define GICV5_NON_CACHE			0b00
> > > > +#define GICV5_WB_CACHE			0b01
> > > > +#define GICV5_WT_CACHE			0b10
> > > > +
> > > > +#define GICV5_NON_SHARE			0b00
> > > > +#define GICV5_OUTER_SHARE		0b10
> > > > +#define GICV5_INNER_SHARE		0b11
> > > > +
> > > > +#define GICV5_IRS_IDR1			0x0004
> > > > +#define GICV5_IRS_IDR2			0x0008
> > > > +#define GICV5_IRS_IDR5			0x0014
> > > > +#define GICV5_IRS_IDR6			0x0018
> > > > +#define GICV5_IRS_IDR7			0x001c
> > > > +#define GICV5_IRS_CR0			0x0080
> > > > +#define GICV5_IRS_CR1			0x0084
> > > > +#define GICV5_IRS_SPI_SELR		0x0108
> > > > +#define GICV5_IRS_SPI_CFGR		0x0114
> > > > +#define GICV5_IRS_SPI_STATUSR		0x0118
> > > > +#define GICV5_IRS_PE_SELR		0x0140
> > > > +#define GICV5_IRS_PE_STATUSR		0x0144
> > > > +#define GICV5_IRS_PE_CR0		0x0148  
> > > 
> > > Blank line here as this is end of register offsets.  
> > 
> > Yep, fixed it.
> > 
> > Thanks for having a look !
> > Lorenzo
> > 
> > > > +#define GICV5_IRS_IDR1_PRIORITY_BITS	GENMASK(22, 20)
> > > > +#define GICV5_IRS_IDR1_IAFFID_BITS	GENMASK(19, 16)  
> > > 
> > > 
> > >   
> > 
>