Hi Marc,
On Mon, Apr 29, 2019 at 12:07 PM Marc Zyngier <marc.zyngier@xxxxxxx> 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@xxxxxxxxx>
> > --- /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@xxxxxxxxxxxxxx
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
