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

[Thomas Gleixner]

On Tue, Apr 08 2025 at 12:50, Lorenzo Pieralisi wrote:
> +
> +static void gicv5_ppi_priority_init(void)
> +{
> +	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRIORITY_MI),
> +				 SYS_ICC_PPI_PRIORITYR0_EL1);

Just let stick it out. You have 100 characters. All over the place...

> +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?

> +static int gicv5_irq_ppi_domain_translate(struct irq_domain *d,
> +					  struct irq_fwspec *fwspec,
> +					  irq_hw_number_t *hwirq,
> +					  unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {

It'd be way more readable to invert this check

     if (!is_of_node(...))
     	return -EINVAL;

so that the subsequent checks are just a read through.

> +		if (fwspec->param_count < 3)
> +			return -EINVAL;
> +
> +		if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[1];
> +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}

> +static void gicv5_irq_ppi_domain_free(struct irq_domain *domain,
> +				      unsigned int virq, unsigned int nr_irqs)
> +{
> +	struct irq_data *d;
> +
> +	if (WARN_ON(nr_irqs != 1))

WARN_ON_ONCE ?

> +		return;
> +
> +	d = irq_domain_get_irq_data(domain, virq);
> +
> +	irq_set_handler(virq, NULL);
> +	irq_domain_reset_irq_data(d);
> +}
> +
> +static int gicv5_irq_ppi_domain_select(struct irq_domain *d,
> +				       struct irq_fwspec *fwspec,
> +				       enum irq_domain_bus_token bus_token)
> +{
> +	/* Not for us */
> +	if (fwspec->fwnode != d->fwnode)
> +		return 0;
> +
> +	if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI) {
> +		// only handle PPIs

Commenting the obvious?

> +		return 0;
> +	}
> +
> +	return (d == gicv5_global_data.ppi_domain);
> +}
> +
> +static const struct irq_domain_ops gicv5_irq_ppi_domain_ops = {
> +	.translate	= gicv5_irq_ppi_domain_translate,
> +	.alloc		= gicv5_irq_ppi_domain_alloc,
> +	.free		= gicv5_irq_ppi_domain_free,
> +	.select		= gicv5_irq_ppi_domain_select
> +};
> +
> +static inline void handle_irq_per_domain(u32 hwirq)
> +{
> +	u32 hwirq_id;
> +	struct irq_domain *domain = NULL;
> +	u8 hwirq_type = FIELD_GET(GICV5_HWIRQ_TYPE, hwirq);

So far you managed to comply with the documented reverse fir tree
ordering.

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

Why are you changing coding style in the middle of the code?

> +
> +	hwirq_id = FIELD_GET(GICV5_HWIRQ_ID, hwirq);
> +
> +	if (hwirq_type == GICV5_HWIRQ_TYPE_PPI)
> +		domain = gicv5_global_data.ppi_domain;
> +
> +	if (generic_handle_domain_irq(domain, hwirq_id)) {
> +		pr_err("Could not handle, hwirq = 0x%x", hwirq_id);

pr_err_once() perhaps?

> +		gicv5_hwirq_eoi(hwirq_id, hwirq_type);
> +	}
> +}
> +
> +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

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