Message-ID: <CAMuHMdU3i7vqs9hd7rfvYH8QtqvwUB5vgsa_fwo5L4E3DQ_d1Q@mail.gmail.com>
Date:   Mon, 29 Apr 2019 13:21:39 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Marc Zyngier <marc.zyngier@....com>
Cc:     Geert Uytterhoeven <geert+renesas@...der.be>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Simon Horman <horms@...ge.net.au>,
        Magnus Damm <magnus.damm@...il.com>,
        Chris Brandt <chris.brandt@...esas.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] irqchip: Add Renesas RZ/A1 Interrupt Controller driver

Hi Marc,

On Mon, Apr 29, 2019 at 12:07 PM Marc Zyngier <marc.zyngier@....com> wrote:
> On 29/04/2019 10:36, Geert Uytterhoeven wrote:
> > Add a driver for the Renesas RZ/A1 Interrupt Controller.
> >
> > This supports using up to 8 external interrupts on RZ/A1, with
> > configurable sense select.
> >
> > NMI edge select is not yet supported.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>

> > --- /dev/null
> > +++ b/drivers/irqchip/irq-renesas-rza1.c

> > +static void rza1_irqc_eoi(struct irq_data *d)
> > +{
> > +     struct rza1_irqc_priv *priv = irq_data_to_priv(d);
> > +     unsigned int bit = BIT(irqd_to_hwirq(d));
>
> Please use u32 instead of "unsigned int" for something that operates on
> HW registers.

Even for 16-bit registers?

> > +     u16 tmp;
> > +
> > +     tmp = readw(priv->base + IRQRR);
>
> Same thing here. It's less confusing to use a u32 and mask out the top
> bits if needed rather than having this implicit cast (applies all over
> the code).

... so yes.

>
> > +     if (tmp & bit)
> > +             writew(GENMASK(IRQC_NUM_IRQ - 1, 0) & ~bit, priv->base + IRQRR);
>
> Please use the _relaxed accessors all over the driver, you really do not
> need a DSB on each of these accesses.

OK.

> > +static int rza1_irqc_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +     struct rza1_irqc_priv *priv = irq_data_to_priv(d);
> > +     unsigned int hw_irq = irqd_to_hwirq(d);
> > +     u16 sense, tmp;
> > +
> > +     switch (type & IRQ_TYPE_SENSE_MASK) {
> > +     case IRQ_TYPE_LEVEL_LOW:
> > +             sense = ICR1_IRQS_LEVEL_LOW;
> > +             break;
> > +
> > +     case IRQ_TYPE_EDGE_FALLING:
> > +             sense = ICR1_IRQS_EDGE_FALLING;
> > +             break;
> > +
> > +     case IRQ_TYPE_EDGE_RISING:
> > +             sense = ICR1_IRQS_EDGE_RISING;
> > +             break;
> > +
> > +     case IRQ_TYPE_EDGE_BOTH:
> > +             sense = ICR1_IRQS_EDGE_BOTH;
> > +             break;
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     tmp = readw(priv->base + ICR1);
> > +     tmp &= ~ICR1_IRQS_MASK(hw_irq);
> > +     tmp |= ICR1_IRQS(hw_irq, sense);
> > +     writew(tmp, priv->base + ICR1);
> > +     return 0;
>
> Don't you need to propagate the trigger configuration to the parent irqchip?

No, the line to the parent GIC is always configured for high-level.

> > +static int rza1_irqc_alloc(struct irq_domain *domain, unsigned int virq,
> > +                        unsigned int nr_irqs, void *arg)
> > +{
> > +     struct rza1_irqc_priv *priv = domain->host_data;
> > +     struct irq_fwspec *fwspec = arg;
> > +     struct irq_fwspec spec;
> > +     int ret;
> > +
> > +     ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
> > +                                         &priv->chip, priv);
> > +     if (ret)
> > +             return ret;
> > +
> > +     spec.fwnode = &priv->dev->of_node->fwnode;
> > +     spec.param_count = 3;
> > +     spec.param[0] = GIC_SPI;
> > +     spec.param[1] = priv->gic_spi_base + fwspec->param[0];
> > +     spec.param[2] = IRQ_TYPE_LEVEL_HIGH;
>
> This is related to my earlier question: Does this block turn everything
> into level interrupts?

That is my understanding of the hardware:
  - Low-level interrupts are cleared when input becomes high again,
  - Rising/falling/both edge interrupts are cleared by reading+writing IRQRR.

FTR, the Hardware User Manual is available from
https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza1h.html#documents
(Section 7. Interrupt Controller).

> > +static int rza1_irqc_probe(struct platform_device *pdev)
> > +{

> > +     priv->chip.name = dev_name(dev);
>
> name should normally be used to identify the overall "class" of

OK, replacing by "rza1-irqc".

> interrupt. .device is what should be used for the device itself.

You mean .parent_device?
Been there, done that: if I fill that in with "dev", it fails with

    gpio-keys keyboard: Unable to claim irq 41; error -13
    gpio-keys: probe of keyboard failed with error -13

due to the call to pm_runtime_get_sync() in irq_chip_pm_get() failing.
This driver doesn't have (and doesn't need) Runtime PM.

> > +struct rza1_irqc_info rza1_irqc_info = {
> > +     .gic_spi_base = 0,
> > +};
>
> To answer your question in the cover letter, I'd rather this came from
> DT. And otherwise, it should be be static.

(Oops, forget the "static const")

Using a custom property, or derived from 8 interrupt specifiers in the
interrupts property?

> It otherwise looks good to me. If you respin it quickly enough, I'm
> happy to take it for 5.2.

Thanks, will do tomorrow, so Chris (in NC.US; let's hope he doesn't
celebrate Golden Week) has a chance to comment...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Powered by blists - more mailing lists