Re: [PATCH 3/4] irqchip/ath79-misc: convert to a platform driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] irqchip/ath79-misc: ath79_misc_intc_probe() clobbers QCA956x
  timer mask
--

commit 60ba869db8725142b0f8869ed514d502d7719988
Author: Rosen Penev <rosenp@gmail.com>

irqchip/ath79-misc: convert to a platform driver

This commit converts the ath79-misc interrupt controller driver from
IRQCHIP_DECLARE() to a platform driver using device-managed resources.

> diff --git a/drivers/irqchip/irq-ath79-misc.c b/drivers/irqchip/irq-ath79-misc.c
> index 5cedea5234e05..b06ece446746c 100644
> --- a/drivers/irqchip/irq-ath79-misc.c
> +++ b/drivers/irqchip/irq-ath79-misc.c
[ ... ]
> @@ -124,65 +125,54 @@ static const struct irq_domain_ops misc_irq_domain_ops = {
>  	.map = misc_map,
>  };
>  
> -static void __init ath79_misc_intc_domain_init(
> -	struct irq_domain *domain, int irq)
> -{
> -	void __iomem *base = domain->host_data;
> -
> -	/* Disable and clear all interrupts */
> -	__raw_writel(0, base + AR71XX_RESET_REG_MISC_INT_ENABLE);
> -	__raw_writel(0, base + AR71XX_RESET_REG_MISC_INT_STATUS);
> -
> -	irq_set_chained_handler_and_data(irq, ath79_misc_irq_handler, domain);
> -}
> -
> -static int __init ath79_misc_intc_of_init(
> -	struct device_node *node, struct device_node *parent)
> +static int ath79_misc_intc_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct irq_domain *domain;
>  	void __iomem *base;
>  	int irq;
>  
> -	irq = irq_of_parse_and_map(node, 0);
> -	if (!irq) {
> -		pr_err("Failed to get MISC IRQ\n");
> -		return -EINVAL;
> -	}
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
>  
> -	base = of_iomap(node, 0);
> -	if (!base) {
> -		pr_err("Failed to get MISC IRQ registers\n");
> -		return -ENOMEM;
> -	}
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
>  
> -	domain = irq_domain_create_linear(of_fwnode_handle(node), ATH79_MISC_IRQ_COUNT,
> -				&misc_irq_domain_ops, base);
> -	if (!domain) {
> -		pr_err("Failed to add MISC irqdomain\n");
> -		return -EINVAL;
> -	}
> +	domain = devm_irq_domain_create_linear(dev, dev_fwnode(dev),
> +					       ATH79_MISC_IRQ_COUNT,
> +					       &misc_irq_domain_ops, base);
> +	if (!domain)
> +		return dev_err_probe(dev, -ENOMEM, "failed to add the irq domain\n");
> +
> +	/* Disable and clear all interrupts */
> +	__raw_writel(0, base + AR71XX_RESET_REG_MISC_INT_ENABLE);
> +	__raw_writel(0, base + AR71XX_RESET_REG_MISC_INT_STATUS);
>  
> -	ath79_misc_intc_domain_init(domain, irq);
> +	irq_set_chained_handler_and_data(irq, ath79_misc_irq_handler, domain);
>  	ath79_misc_domain = domain;
> +
>  	return 0;
>  }
>  
> -static int __init ar7100_misc_intc_of_init(
> -	struct device_node *node, struct device_node *parent)
> +static int ar7100_misc_intc_probe(struct platform_device *pdev,
> +				  struct device_node *parent)
>  {
>  	ath79_misc_irq_chip.irq_mask_ack = ar71xx_misc_irq_mask;
> -	return ath79_misc_intc_of_init(node, parent);
> +	return ath79_misc_intc_probe(pdev);
>  }
>  
> -IRQCHIP_DECLARE(ar7100_misc_intc, "qca,ar7100-misc-intc",
> -		ar7100_misc_intc_of_init);
> -
> -static int __init ar7240_misc_intc_of_init(
> -	struct device_node *node, struct device_node *parent)
> +static int ar7240_misc_intc_probe(struct platform_device *pdev,
> +				  struct device_node *parent)
>  {
>  	ath79_misc_irq_chip.irq_ack = ar724x_misc_irq_ack;
> -	return ath79_misc_intc_of_init(node, parent);
> +	return ath79_misc_intc_probe(pdev);
>  }
>  
> -IRQCHIP_DECLARE(ar7240_misc_intc, "qca,ar7240-misc-intc",
> -		ar7240_misc_intc_of_init);
> +IRQCHIP_PLATFORM_DRIVER_BEGIN(ath79_misc_intc)
> +IRQCHIP_MATCH("qca,ar7100-misc-intc", ar7100_misc_intc_probe)
> +IRQCHIP_MATCH("qca,ar7240-misc-intc", ar7240_misc_intc_probe)
> +IRQCHIP_PLATFORM_DRIVER_END(ath79_misc_intc)

[Severity: High]
Does probing ath79_misc_intc at device_initcall time clobber
MISC_INT_MIPS_SI_TIMERINT_MASK in AR71XX_RESET_REG_MISC_INT_ENABLE on
QCA956x?

With IRQCHIP_DECLARE(), this driver ran during init_IRQ() before
time_init() -> plat_time_init() -> of_clk_init() called
qca956x_clocks_init():

arch/mips/ath79/clock.c:qca956x_clocks_init() {
	...
	/*
	 * QCA956x timer init workaround has to be applied right before setting
	 * up the clock. Else, there will be no jiffies
	 */
	u32 misc;

	misc = ath79_reset_rr(AR71XX_RESET_REG_MISC_INT_ENABLE);
	misc |= MISC_INT_MIPS_SI_TIMERINT_MASK;
	ath79_reset_wr(AR71XX_RESET_REG_MISC_INT_ENABLE, misc);
	...
}

Moving registration to IRQCHIP_PLATFORM_DRIVER_BEGIN() runs
ath79_misc_intc_probe() after time_init(), where the unconditional write of
0 to AR71XX_RESET_REG_MISC_INT_ENABLE clears MISC_INT_MIPS_SI_TIMERINT_MASK
and breaks the CPU timer interrupt on QCA956x, TP9343, and QCN550x SoCs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112224445.213324-1-rosenp@gmail.com?part=3