Re: [PATCH 18/24] irqchip/gic-v5: Add GICv5 PPI support

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Tue, Apr 08, 2025 at 11:42:29PM +0200, Thomas Gleixner wrote:

[...]

> > +static int gicv5_ppi_irq_set_irqchip_state(struct irq_data *d,
> > +					   enum irqchip_irq_state which,
> > +					   bool val)
> > +{
> > +	u64 hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> > +
> > +	switch (which) {
> > +	case IRQCHIP_STATE_PENDING:
> > +		if (val) {
> > +			if (d->hwirq < 64)
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_SPENDR0_EL1);
> > +			else
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_SPENDR1_EL1);
> > +
> > +		} else {
> > +			if (d->hwirq < 64)
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_CPENDR0_EL1);
> > +			else
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_CPENDR1_EL1);
> > +		}
> > +
> > +		return 0;
> > +	case IRQCHIP_STATE_ACTIVE:
> > +		if (val) {
> > +			if (d->hwirq < 64)
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_SACTIVER0_EL1);
> > +			else
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_SACTIVER1_EL1);
> > +		} else {
> > +			if (d->hwirq < 64)
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_CACTIVER0_EL1);
> > +			else
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_CACTIVER1_EL1);
> > +		}
> 
> You already precalculate hwirq_id_bit. Can't you do something similar
> for the registers?
> 
> 	case IRQCHIP_STATE_PENDING:
>         	u32 reg = val ? SYS_ICC_PPI_SPENDR1_EL1 : SYS_ICC_PPI_SPENDR0_EL1;
> 
>                 write_sysreg_s(hwirq_id_bit, reg);
>                 return 0;
> 	case IRQCHIP_STATE_ACTIVE:
>                 ....
> 
> Ditto in the get_state() function.
> 
> No?

Can't do it like that, write_sysreg_s takes a register encoding, not a
u32 offset, I will see if I can wrap it in a macro, even though that
might obfuscate a bit rather than clarify.

> > +static asmlinkage void __exception_irq_entry
> > +gicv5_handle_irq(struct pt_regs *regs)
> > +{
> > +	u64 ia;
> > +	bool valid;
> > +	u32 hwirq;
> 
> See above
> 
> > +	ia = gicr_insn(GICV5_OP_GICR_CDIA);
> > +	valid = GICV5_GIC_CDIA_VALID(ia);
> 
> And please move that to the declaration lines

gicr_insn() is an instruction ACK'ing the IRQ, I would leave it like this
explicitly if you don't mind lest it gave the impression that it
is an initializer.

Thanks,
Lorenzo

> > +static int __init gicv5_init_domains(struct fwnode_handle *handle)
> > +{
> > +	gicv5_global_data.fwnode = handle;
> > +	gicv5_global_data.ppi_domain = irq_domain_create_linear(
> > +		handle, 128, &gicv5_irq_ppi_domain_ops, NULL);
> 
> The ever changing choice of coding styles across functions is really
> interesting. Obviously the length of 'gicv5_global_data.ppi_domain'
> forces ugly, but that does not mean it needs to be that way:
> 
>        struct irqdomain *d;
> 
>        d = irq_domain_create_linear(handle, 128, &gicv5_irq_ppi_domain_ops, NULL);
>        if (!d)
>        		return - ENOMEM;
> 
>        irq_domain_update_bus_token(d, DOMAIN_BUS_WIRED);
>        gicv5_global_data.fwnode = handle;
>        gicv5_global_data.ppi_domain = d;
>        return 0;
> 
> No?
> 
> > +static int __init gicv5_of_init(struct device_node *node,
> > +				struct device_node *parent)
> > +{
> > +	int ret;
> > +
> > +	ret = gicv5_init_domains(&node->fwnode);
> > +	if (ret)
> > +		return ret;
> > +
> > +	gicv5_set_cpuif_pribits();
> > +
> > +	ret = gicv5_starting_cpu(smp_processor_id());
> 
> You invoke the CPU hotplug callback for the boot CPU explicitly, but
> what the heck installs the actual hotplug callback for the secondary
> CPUs?
> 
> Thanks,
> 
>         tglx