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