Re: [PATCH v3 20/25] irqchip/gic-v5: Add GICv5 PPI support

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, May 06 2025 at 14:23, Lorenzo Pieralisi wrote:
> +
> +static u8 pri_bits = 5;

__ro_after_init ?

> +#define GICV5_IRQ_PRI_MASK 0x1f

Please put a new line before the #define and use a TAB between the
symbol and the value.

> +#define GICV5_IRQ_PRI_MI \
> +		(GICV5_IRQ_PRI_MASK & GENMASK(4, 5 - pri_bits))

No line break required. You have 100 characters

> +#define READ_PPI_REG(irq, reg)							\
> +	({									\
> +		u64 __ppi_val;							\
> +										\
> +		if (irq < 64)							\
> +			__ppi_val = read_sysreg_s(SYS_ICC_PPI_##reg##R0_EL1);	\
> +		else								\
> +			__ppi_val = read_sysreg_s(SYS_ICC_PPI_##reg##R1_EL1);	\
> +		__ppi_val;							\
> +	})
> +
> +#define WRITE_PPI_REG(set, irq, bit, reg)					\
> +	do {									\
> +		if (set) {							\
> +			if (irq < 64)						\
> +				write_sysreg_s(bit, SYS_ICC_PPI_S##reg##R0_EL1);\
> +			else							\
> +				write_sysreg_s(bit, SYS_ICC_PPI_S##reg##R1_EL1);\
> +		} else {							\
> +			if (irq < 64)						\
> +				write_sysreg_s(bit, SYS_ICC_PPI_C##reg##R0_EL1);\
> +			else							\
> +				write_sysreg_s(bit, SYS_ICC_PPI_C##reg##R1_EL1);\
> +		}								\
> +	} while (0)

I'm not convinced that these need to be macros.

static __always_inline u64 read_ppi_sysreg_s(unsigned int irq, const unsigned int which)
{
        switch (which) {
        case PPI_HM:
        	return irq < 64 ? read_sysreg_s(SYS_ICC_PPI_HM_R0_EL1) :
                		  read_sysreg_s(SYS_ICC_PPI_HM_R1_EL1;
        case ....:

        default:
                BUILD_BUG_ON(1);
        }
}

static __always_inline void write_ppi_sysreg_s(unsigned int irq, bool set, const unsigned int which)
{
	u64 bit = BIT_ULL(irq % 64);

        switch (which) {  
        case PPI_HM:
        	if (irq < 64)
                	write_sysreg_s(bit, SYS_ICC_PPI_HM_R0_EL1);
                else
                	write_sysreg_s(bit, SYS_ICC_PPI_HM_R1_EL1;
                return;
        case ....:

        default:
                BUILD_BUG_ON(1);
        }
}

Or something like that.

> +static int gicv5_ppi_set_type(struct irq_data *d, unsigned int type)
> +{
> +	/*
> +	 * The PPI trigger mode is not configurable at runtime,
> +	 * therefore this function simply confirms that the `type`
> +	 * parameter matches what is present.
> +	 */
> +	u64 hmr = READ_PPI_REG(d->hwirq, HM);
> +
> +	switch (type) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +	case IRQ_TYPE_LEVEL_LOW:
> +		if (((hmr >> (d->hwirq % 64)) & 0x1) != GICV5_PPI_HM_LEVEL)
> +			return -EINVAL;

Blink!

How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
tests for level, no? So the test is interesting at best ...

Secondly this comparison is confusing at best especially given that you
mask with a hex constant (0x1) first.

     		if (hmr & BIT_UL(d->hwirq % 64))
                	return -EINVAL;

Aside of that why do you have a set_type() function if there is no way
to set the type?

> +
> +static int gicv5_ppi_irq_get_irqchip_state(struct irq_data *d,
> +					   enum irqchip_irq_state which,
> +					   bool *val)
> +{
> +	u64 pendr, activer, hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> +
> +	switch (which) {
> +	case IRQCHIP_STATE_PENDING:
> +		pendr = READ_PPI_REG(d->hwirq, SPEND);
> +
> +		*val = !!(pendr & hwirq_id_bit);
> +
> +		return 0;

		*val = !!(read_ppi_reg(d->hwirq, PPI_SPEND) & bit);
                return 0;

would take up less space and be readable.

> +	case IRQCHIP_STATE_ACTIVE:
> +		activer = READ_PPI_REG(d->hwirq, SACTIVE);
> +
> +		*val = !!(activer & hwirq_id_bit);
> +
> +		return 0;
> +	default:
> +		pr_debug("Unexpected PPI irqchip state\n");
> +	}
> +
> +	return -EINVAL;

Move the return into the default case.

> +static int __init gicv5_init_domains(struct fwnode_handle *handle)
> +{
> +	struct irq_domain *d;
> +
> +	d = irq_domain_create_linear(handle, PPI_NR, &gicv5_irq_ppi_domain_ops,
> +				     NULL);

Please use the full 100 charactes all over the place.

> +	if (!d)
> +		return -ENOMEM;
> +
> +	irq_domain_update_bus_token(d, DOMAIN_BUS_WIRED);
> +	gicv5_global_data.ppi_domain = d;
> +
> +	gicv5_global_data.fwnode = handle;

The random choices of seperating code with new lines are really
amazing.

> +static int __init gicv5_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	int ret;
> +
> +	ret = gicv5_init_domains(&node->fwnode);

        int ret = ....;

> +	if (ret)

Thanks,

        tglx