Re: [PATCH 08/12] soc: fsl: qe: Convert to generic IRQ chip

["Christophe Leroy (CS GROUP)" <chleroy@kernel.org>]

Hi Paul,

Le 03/07/2026 à 15:30, Paul Louvel a écrit :
> The generic IRQ chip framework is available to handle IRQ chips. Using
> this framework for the QE interrupt controller allows to simplify the
> driver. Indeed, the framework internally handles operations coded
> directly in the driver.
> 
> Add a select dependency to GENERIC_IRQ_CHIP in the PPC platform Kconfig.
> 
> Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
> ---
>   arch/powerpc/platforms/Kconfig   |   1 +
>   drivers/soc/fsl/qe/qe_ports_ic.c | 103 ++++++++++++++++++++++++++-------------
>   2 files changed, 70 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> index c4e61843d9d9..b0b3a80f8cde 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -232,6 +232,7 @@ config QE_GPIO
>   	bool "QE GPIO support"
>   	depends on QUICC_ENGINE
>   	select GPIOLIB
> +	select GENERIC_IRQ_CHIP
>   	help
>   	  Say Y here if you're going to use hardware that connects to the
>   	  QE GPIOs.
> diff --git a/drivers/soc/fsl/qe/qe_ports_ic.c b/drivers/soc/fsl/qe/qe_ports_ic.c
> index c8b73b0aa233..d022aa224f6d 100644
> --- a/drivers/soc/fsl/qe/qe_ports_ic.c
> +++ b/drivers/soc/fsl/qe/qe_ports_ic.c
> @@ -20,63 +20,65 @@ struct qepic_data {
>   	void __iomem *reg;
>   	struct irq_domain *host;
>   	int irq;
> +	struct irq_chip_generic *gc;
>   };
>   
>   static void qepic_mask(struct irq_data *d)
>   {
> -	struct qepic_data *data = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>   
> -	clrbits32(data->reg + CEPIMR, 1 << (31 - irqd_to_hwirq(d)));
> +	clrbits32(gc->reg_base + ct->regs.mask, d->mask);

Is there a real added value with this change ?

Previously we had:

00000000 <qepic_mask>:
    0:	81 03 00 18 	lwz     r8,24(r3)
    4:	81 28 00 00 	lwz     r9,0(r8)
    8:	7c 00 04 ac 	hwsync
    c:	81 29 00 10 	lwz     r9,16(r9)
   10:	0c 09 00 00 	twi     0,r9,0
   14:	4c 00 01 2c 	isync
   18:	80 e3 00 08 	lwz     r7,8(r3)
   1c:	3d 40 80 00 	lis     r10,-32768
   20:	81 08 00 00 	lwz     r8,0(r8)
   24:	7d 4a 3c 30 	srw     r10,r10,r7
   28:	7d 29 50 78 	andc    r9,r9,r10
   2c:	7c 00 04 ac 	hwsync
   30:	91 28 00 10 	stw     r9,16(r8)
   34:	4e 80 00 20 	blr

Now we have:

00000000 <qepic_mask>:
    0:	80 e3 00 18 	lwz     r7,24(r3)
    4:	81 03 00 10 	lwz     r8,16(r3)
    8:	81 27 00 00 	lwz     r9,0(r7)
    c:	81 48 00 94 	lwz     r10,148(r8)
   10:	7d 29 52 14 	add     r9,r9,r10
   14:	7c 00 04 ac 	hwsync
   18:	81 49 00 00 	lwz     r10,0(r9)
   1c:	0c 0a 00 00 	twi     0,r10,0
   20:	4c 00 01 2c 	isync
   24:	80 c3 00 00 	lwz     r6,0(r3)
   28:	81 27 00 00 	lwz     r9,0(r7)
   2c:	81 08 00 94 	lwz     r8,148(r8)
   30:	7d 4a 30 78 	andc    r10,r10,r6
   34:	7d 29 42 14 	add     r9,r9,r8
   38:	7c 00 04 ac 	hwsync
   3c:	91 49 00 00 	stw     r10,0(r9)
   40:	4e 80 00 20 	blr

We now have three more indirect loads (8x lwz instead of 5x), for 
loading some value which is already known at compile time.


>   }
>   
>   static void qepic_unmask(struct irq_data *d)
>   {
> -	struct qepic_data *data = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>   
> -	setbits32(data->reg + CEPIMR, 1 << (31 - irqd_to_hwirq(d)));
> +	setbits32(gc->reg_base + ct->regs.mask, d->mask);
>   }
>   
>   static void qepic_end(struct irq_data *d)
>   {
> -	struct qepic_data *data = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>   
> -	out_be32(data->reg + CEPIER, 1 << (31 - irqd_to_hwirq(d)));
> +	out_be32(gc->reg_base + ct->regs.eoi, d->mask);
> +}
> +
> +static void qepic_calc_mask(struct irq_data *d)
> +{
> +	d->mask = 1 << (31 - irqd_to_hwirq(d));
>   }
>   
>   static int qepic_set_type(struct irq_data *d, unsigned int flow_type)
>   {
> -	struct qepic_data *data = irq_data_get_irq_chip_data(d);
> -	unsigned int vec = (unsigned int)irqd_to_hwirq(d);
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>   
>   	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
>   	case IRQ_TYPE_EDGE_FALLING:
> -		setbits32(data->reg + CEPICR, 1 << (31 - vec));
> +		setbits32(gc->reg_base + ct->regs.type, d->mask);
>   		return 0;
>   	case IRQ_TYPE_EDGE_BOTH:
>   	case IRQ_TYPE_NONE:
> -		clrbits32(data->reg + CEPICR, 1 << (31 - vec));
> +		clrbits32(gc->reg_base + ct->regs.type, d->mask);
>   		return 0;
>   	}
>   	return -EINVAL;
>   }
>   
> -static struct irq_chip qepic = {
> -	.name = "QEPIC",
> -	.irq_mask = qepic_mask,
> -	.irq_unmask = qepic_unmask,
> -	.irq_eoi = qepic_end,
> -	.irq_set_type = qepic_set_type,
> -};
> -
>   static void qepic_cascade(struct irq_desc *desc)
>   {
>   	struct qepic_data *data = irq_desc_get_handler_data(desc);
> +	struct irq_chip_type *ct = data->gc->chip_types;
>   	struct irq_chip *chip = irq_desc_get_chip(desc);
>   	unsigned long event, bit;
>   
>   	chained_irq_enter(chip, desc);
>   
> -	event = in_be32(data->reg + CEPIER);
> +	event = in_be32(data->gc->reg_base + ct->regs.eoi);
>   	if (!event) {
>   		handle_bad_irq(desc);
>   		goto out;
> @@ -89,33 +91,64 @@ static void qepic_cascade(struct irq_desc *desc)
>   	chained_irq_exit(chip, desc);
>   }
>   
> -static int qepic_host_map(struct irq_domain *h, unsigned int virq, irq_hw_number_t hw)
> +static int qepic_chip_init(struct irq_chip_generic *gc)
>   {
> -	irq_set_chip_data(virq, h->host_data);
> -	irq_set_chip_and_handler(virq, &qepic, handle_fasteoi_irq);
> +	struct irq_chip_type *ct = gc->chip_types;
> +
> +	ct->regs.mask = CEPIMR;
> +	ct->chip.irq_mask = qepic_mask;
> +	ct->chip.irq_unmask = qepic_unmask;
> +	ct->regs.eoi = CEPIER;
> +	ct->chip.irq_eoi = qepic_end;
> +	ct->regs.type = CEPICR;
> +	ct->chip.irq_set_type = qepic_set_type;
> +	ct->chip.irq_calc_mask = qepic_calc_mask;

Are ct->regs.mask, ct->regs.eoi and ct->regs.type used anywhere else 
than locally in qepic_{mask/unmask/end/set_type} ?

Christophe

> +
>   	return 0;
>   }
>   
> -static const struct irq_domain_ops qepic_host_ops = {
> -	.map = qepic_host_map,
> -};
> +static int qepic_domain_init(struct irq_domain *d)
> +{
> +	struct qepic_data *data = d->host_data;
>   
> -static void qepic_remove(void *res)
> +	irq_set_chained_handler_and_data(data->irq, qepic_cascade, data);
> +
> +	return 0;
> +}
> +
> +static void qepic_domain_exit(struct irq_domain *d)
>   {
> -	struct qepic_data *data = res;
> +	struct qepic_data *data = d->host_data;
>   
>   	irq_set_chained_handler_and_data(data->irq, NULL, NULL);
> -	irq_domain_remove(data->host);
>   }
>   
>   static int qepic_probe(struct platform_device *pdev)
>   {
> +	struct irq_domain_chip_generic_info dgc_info = {
> +		.name		= "QEPIC",
> +		.handler	= handle_fasteoi_irq,
> +		.irqs_per_chip	= 32,
> +		.num_ct		= 1,
> +		.init		= qepic_chip_init,
> +	};
> +	struct irq_domain_info d_info = {
> +		.fwnode		= of_fwnode_handle(pdev->dev.of_node),
> +		.domain_flags	= IRQ_DOMAIN_FLAG_DESTROY_GC,
> +		.size		= 32,
> +		.hwirq_max	= 32,
> +		.ops		= &irq_generic_chip_ops,
> +		.dgc_info	= &dgc_info,
> +		.init		= qepic_domain_init,
> +		.exit		= qepic_domain_exit,
> +	};
>   	struct device *dev = &pdev->dev;
>   	struct qepic_data *data;
>   
>   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;
> +	d_info.host_data = data;
>   
>   	data->reg = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(data->reg))
> @@ -125,14 +158,16 @@ static int qepic_probe(struct platform_device *pdev)
>   	if (data->irq < 0)
>   		return data->irq;
>   
> -	data->host = irq_domain_create_linear(dev_fwnode(dev), 32, &qepic_host_ops, data);
> -	if (!data->host)
> -		return -ENODEV;
> +	data->host = devm_irq_domain_instantiate(dev, &d_info);
> +	if (IS_ERR(data->host))
> +		return PTR_ERR(data->host);
>   
> -	irq_set_chained_handler_and_data(data->irq, qepic_cascade, data);
> -
> -	return devm_add_action_or_reset(dev, qepic_remove, data);
> +	data->gc = irq_get_domain_generic_chip(data->host, 0);
> +	if (!data->gc)
> +		return -ENODEV;
> +	data->gc->reg_base = data->reg;
>   
> +	return 0;
>   }
>   
>   static const struct of_device_id qepic_match[] = {
>