Re: [PATCH 5.10.y-cip 11/36] irqchip: Add RZ/G2L IA55 Interrupt Controller driver

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Pavel,

Thank you for your review!

On 27.03.2024 14:07, Pavel Machek wrote:
> Hi!
> 
>> +++ b/drivers/irqchip/Makefile
>> @@ -51,6 +51,7 @@ obj-$(CONFIG_RDA_INTC)			+= irq-rda-intc.o
>>  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
>>  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
>>  obj-$(CONFIG_RENESAS_RZA1_IRQC)		+= irq-renesas-rza1.o
>> +obj-$(CONFIG_RENESAS_RZG2L_IRQC)	+= irq-renesas-rzg2l.o
>>  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
>>  obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
>>  obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
>> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
>> new file mode 100644
>> index 000000000000..cf99cd6b41c4
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
>> @@ -0,0 +1,394 @@
>> +#define IRQC_IRQ_START			1
>> +#define IRQC_IRQ_COUNT			8
>> +#define IRQC_TINT_START			(IRQC_IRQ_START + IRQC_IRQ_COUNT)
>> +#define IRQC_TINT_COUNT			32
> 
> The code hardwires assumption that interrupts are IRQ_START
> .. TINT_START .. TINT_START+TINT_COUNT.

IRQ 0 is NMI. I missed to add it in the diagram I attached to patch 10. The
intention there was to describe the hierarchy b/w the IRQ controllers.
Sorry for that.

The interrupts in interval [IRQC_IRQ_START, IRQC_IRQ_COUNT] (IRQ
interrupts) are interrupts for which dedicated pins are available. In the
above diagram (borrowed from hardware manual) these are described though
IRQ0-7. These dedicated pins have 1:1 mapping w/ the IRQs that IA55 IRQC
routes to GIC.

The interrupts in interval [IRQ_TINT_START, IRQ_TINT_COUNT] (TINT
interrupts) are interrupts that could be mapped to any GPIO pins.
In the above diagram GPIOINT0-127 specifies that any of the 127 pinctrl
pins could be mapped to these TINT interrupts.

IRQ interrupts and TINT interrupts and are configured differently in the
IA55 registers.



                                     ┌──────────┐          ┌──────────┐
NMI -------------------------------->│          │ SPIX     │          │
                                     │          ├─────────►│          │
                                     │          │          │          │
                                     │          │          │          │
             ┌────────┐IRQ0-7        │  IA55    │          │  GIC     │
Pin0 ───────►│        ├─────────────►│          │          │          │
             │        │              │          │ PPIY     │          │
...          │  GPIO  │              │          ├─────────►│          │
             │        │GPIOINT0-127  │          │          │          │
PinN ───────►│        ├─────────────►│          │          │          │
             └────────┘              └──────────┘          └──────────┘

> 
>> +	raw_spin_lock(&priv->lock);
>> +	if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT)
>> +		rzg2l_irq_eoi(d);
> 
> I'd do "< IRQC_TINT_START" here.

hw_irq 0 is NMI. IRQs IRQC_IRQ_START-IRQC_IRQ_COUNT are IRQ interrupts and
IRQs IRQC_TINT_START - IRQC_NUM_IRQ are TINT interrupts. These are
configured differently thus, the need to differentiate here.

> 
>> +static void rzg2l_irqc_irq_disable(struct irq_data *d)
>> +{
>> +	unsigned int hw_irq = irqd_to_hwirq(d);
>> +
>> +	if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) {
> 
> And here.

Same here.

> 
>> +	if (hwirq > (IRQC_NUM_IRQ - 1))
>> +		return -EINVAL;
> 
> '>= IRQC_NUM_IRQ' would be more clear here.

This could be updated, for sure. I don't know exactly the procedure for
that: should a patch be first published to upstream kernel and then here or
would you prefer to have it directly published here?

> 
>> +static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
>> +{
>> +	struct irq_domain *irq_domain, *parent_domain;
>> +	struct platform_device *pdev;
>> +	struct reset_control *resetn;
>> +	struct rzg2l_irqc_priv *priv;
>> +	int ret;
>> +
>> +	pdev = of_find_device_by_node(node);
>> +	if (!pdev)
>> +		return -ENODEV;
>> +
>> +	parent_domain = irq_find_host(parent);
>> +	if (!parent_domain) {
>> +		dev_err(&pdev->dev, "cannot find parent domain\n");
>> +		return -ENODEV;
>> +	}
> 
> I believe you'll need to put pdev in this and following error paths.

You're talking about put_device(&pdev->dev)?

I agree, that should be done on error path.

Would you prefer to post the patch here directly or 1st to the upstream
kernel and then backport it?

Thank you,
Claudiu Beznea

> 
> Best regards,
> 								Pavel