[PATCH v4 21/26] irqchip/gic-v5: Add GICv5 IRS/SPI support
Alireza Sanaee
alireza.sanaee at huawei.com
Thu May 29 04:17:21 PDT 2025
On Thu, 29 May 2025 11:32:06 +0200
Lorenzo Pieralisi <lpieralisi at kernel.org> wrote:
> 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
Hi Lorenzo,
This goes back to the Rob and Mark discussion in regards to
representing SMTs.
With SMTs if cpu-node reg property has
more than one entry, then you probably have to count them and also ref
them differently.
But if every single SMT is a cpu-node in the DT (which is indeed how
things are currently), it is fine.
Thanks,
Alireza
>
> > >
> > > > 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)
> > > >
> > > >
> > > >
> > >
> >
>
>
More information about the linux-arm-kernel
mailing list