On Wed, 21 Dec 2022 00:02:36 +0000,
Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
>
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> NMI interrupt is not an external interrupt as compared to the IRQ0-7 and
> TINT0-31, this means we need to install the irq handler for NMI in the
> IRQC driver and not include it as part of IRQ domain.
>
> This patch skips mapping NMI interrupt as part of the IRQ domain
> hierarchy.
Does it mean nobody can connect anything to it? Where is the handler
you're mentioning for this NMI?
>
> Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
> v1 -> v2
> * New patch
> ---
> drivers/irqchip/irq-renesas-rzg2l.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> index 25fd8ee66565..7918fe201218 100644
> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -23,7 +23,8 @@
> #define IRQC_IRQ_COUNT 8
> #define IRQC_TINT_START (IRQC_IRQ_START + IRQC_IRQ_COUNT)
> #define IRQC_TINT_COUNT 32
> -#define IRQC_NUM_IRQ (IRQC_TINT_START + IRQC_TINT_COUNT)
> + /* IRQ0-7 + TINT0-31 */
> +#define IRQC_NUM_HIERARCHY_IRQ (IRQC_TINT_START + IRQC_TINT_COUNT - 1)
>
> #define ISCR 0x10
> #define IITSR 0x14
> @@ -58,7 +59,8 @@
>
> struct rzg2l_irqc_priv {
> void __iomem *base;
> - struct irq_fwspec fwspec[IRQC_NUM_IRQ];
> + /* IRQ0-7 + TINT0-31 will be part of hierarchy domain */
> + struct irq_fwspec fwspec[IRQC_NUM_HIERARCHY_IRQ];
> raw_spinlock_t lock;
> };
>
> @@ -99,7 +101,7 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
> raw_spin_lock(&priv->lock);
> if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT)
> rzg2l_irq_eoi(d);
> - else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ)
> + else if (hw_irq >= IRQC_TINT_START && hw_irq <= IRQC_NUM_HIERARCHY_IRQ)
> rzg2l_tint_eoi(d);
> raw_spin_unlock(&priv->lock);
> irq_chip_eoi_parent(d);
> @@ -109,7 +111,7 @@ 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) {
> + if (hw_irq >= IRQC_TINT_START && hw_irq <= IRQC_NUM_HIERARCHY_IRQ) {
> struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> u32 offset = hw_irq - IRQC_TINT_START;
> u32 tssr_offset = TSSR_OFFSET(offset);
> @@ -129,7 +131,7 @@ static void rzg2l_irqc_irq_enable(struct irq_data *d)
> {
> unsigned int hw_irq = irqd_to_hwirq(d);
>
> - if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) {
> + if (hw_irq >= IRQC_TINT_START && hw_irq <= IRQC_NUM_HIERARCHY_IRQ) {
> struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> unsigned long tint = (uintptr_t)d->chip_data;
> u32 offset = hw_irq - IRQC_TINT_START;
> @@ -228,7 +230,7 @@ static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type)
>
> if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT)
> ret = rzg2l_irq_set_type(d, type);
> - else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ)
> + else if (hw_irq >= IRQC_TINT_START && hw_irq <= IRQC_NUM_HIERARCHY_IRQ)
How about you define a "tint_hwirq()" helper that checks got the
boundaries? Same thing for the other IRQ type.
> ret = rzg2l_tint_set_edge(d, type);
> if (ret)
> return ret;
> @@ -280,7 +282,7 @@ static int rzg2l_irqc_alloc(struct irq_domain *domain, unsigned int virq,
> return -EINVAL;
> }
>
> - if (hwirq > (IRQC_NUM_IRQ - 1))
> + if (!hwirq || hwirq > IRQC_NUM_HIERARCHY_IRQ)
> return -EINVAL;
>
> ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &irqc_chip,
> @@ -288,7 +290,7 @@ static int rzg2l_irqc_alloc(struct irq_domain *domain, unsigned int virq,
> if (ret)
> return ret;
>
> - return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &priv->fwspec[hwirq]);
> + return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &priv->fwspec[hwirq - 1]);
> }
>
> static const struct irq_domain_ops rzg2l_irqc_domain_ops = {
> @@ -304,12 +306,12 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
> unsigned int i;
> int ret;
>
> - for (i = 0; i < IRQC_NUM_IRQ; i++) {
> + for (i = 1; i <= IRQC_NUM_HIERARCHY_IRQ; i++) {
> ret = of_irq_parse_one(np, i, &map);
> if (ret)
> return ret;
> of_phandle_args_to_fwspec(np, map.args, map.args_count,
> - &priv->fwspec[i]);
> + &priv->fwspec[i - 1]);
Starting the loop at 1 really is non-idiomatic, and I'd rather see
something like this:
for (i = 0; i < IRQC_NUM_HIERARCHY_IRQ; i++) {
ret = of_irq_parse_one(np, i + 1, &map);
if (ret)
return ret;
of_phandle_args_to_fwspec(np, map.args, map.args_count,
&priv->fwspec[i]);
}
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
