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

Lorenzo Pieralisi
2025-05-29T09:32:15Z

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)
> > >
> > >
> > >
> >
>