Hi Thomas,
thanks for your feedback!
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Wednesday, October 2, 2024 11:51 AM
> Subject: Re: [PATCH 5/6] irqchip: Add RZ/V2H(P) Interrupt Control Unit (ICU) driver
>
> On Tue, Sep 17 2024 at 18:32, Fabrizio Castro wrote:
> > +
> > +/* DT "interrupts" indexes */
> > +#define ICU_IRQ_START 1
> > +#define ICU_IRQ_COUNT 16
> > +#define ICU_TINT_START (ICU_IRQ_START + ICU_IRQ_COUNT)
> > +#define ICU_TINT_COUNT 32
> > +#define ICU_NUM_IRQ (ICU_TINT_START + ICU_TINT_COUNT)
> > +
> > +/* Registers */
> > +#define ICU_NSCNT 0x00
> > +#define ICU_NSCLR 0x04
> > +#define ICU_NITSR 0x08
> > +#define ICU_ISCTR 0x10
> > +#define ICU_ISCLR 0x14
> > +#define ICU_IITSR 0x18
> > +#define ICU_TSCTR 0x20
> > +#define ICU_TSCLR 0x24
> > +#define ICU_TITSR(k) (0x28 + (k) * 4)
> > +#define ICU_TSSR(k) (0x30 + (k) * 4)
> > +
> > +/* NMI */
> > +#define ICU_NMI_EDGE_FALLING 0
> > +#define ICU_NMI_EDGE_RISING 1
> > +
> > +#define ICU_NSCNT_NSTAT BIT(0)
> > +#define ICU_NSCNT_NSTAT_DETECTED 1
> > +
> > +#define ICU_NSCLR_NCLR BIT(0)
> > +
> > +/* IRQ */
> > +#define ICU_IRQ_LEVEL_LOW 0
> > +#define ICU_IRQ_EDGE_FALLING 1
> > +#define ICU_IRQ_EDGE_RISING 2
> > +#define ICU_IRQ_EDGE_BOTH 3
> > +
> > +#define ICU_IITSR_IITSEL_PREP(iitsel, n) ((iitsel) << ((n) * 2))
> > +#define ICU_IITSR_IITSEL_GET(iitsr, n) (((iitsr) >> ((n) * 2)) & 0x03)
> > +#define ICU_IITSR_IITSEL_MASK(n) ICU_IITSR_IITSEL_PREP(0x03, n)
> > +
> > +/* TINT */
> > +#define ICU_TINT_EDGE_RISING 0
> > +#define ICU_TINT_EDGE_FALLING 1
> > +#define ICU_TINT_LEVEL_HIGH 2
> > +#define ICU_TINT_LEVEL_LOW 3
> > +
> > +#define ICU_TSSR_K(tint_nr) ((tint_nr) / 4)
> > +#define ICU_TSSR_TSSEL_N(tint_nr) ((tint_nr) % 4)
> > +#define ICU_TSSR_TSSEL_PREP(tssel, n) ((tssel) << ((n) * 8))
> > +#define ICU_TSSR_TSSEL_MASK(n) ICU_TSSR_TSSEL_PREP(0x7F, n)
> > +#define ICU_TSSR_TIEN(n) (BIT(7) << ((n) * 8))
> > +
> > +#define ICU_TITSR_K(tint_nr) ((tint_nr) / 16)
> > +#define ICU_TITSR_TITSEL_N(tint_nr) ((tint_nr) % 16)
> > +#define ICU_TITSR_TITSEL_PREP(titsel, n) ICU_IITSR_IITSEL_PREP(titsel, n)
> > +#define ICU_TITSR_TITSEL_MASK(n) ICU_IITSR_IITSEL_MASK(n)
> > +#define ICU_TITSR_TITSEL_GET(titsr, n) ICU_IITSR_IITSEL_GET(titsr, n)
> > +
> > +#define ICU_TINT_EXTRACT_HWIRQ(x) FIELD_GET(GENMASK(15, 0), (x))
> > +#define ICU_TINT_EXTRACT_GPIOINT(x) FIELD_GET(GENMASK(31, 16), (x))
> > +#define ICU_PB5_TINT 0x55
> > +
> > +/**
> > + * struct rzv2h_icu_priv - Interrupt Control Unit controller private data
> > + * structure.
>
> If you need a line break, then please format it so:
>
> * struct rzv2h_icu_priv - Interrupt Control Unit controller private data
> * structure.
Since I have 100 chars, I'll just get rid of this line break.
>
> This makes it readable.
>
> > +static void rzv2h_clear_nmi_int(struct rzv2h_icu_priv *priv)
> > +{
> > + u32 nscnt = readl_relaxed(priv->base + ICU_NSCNT);
> > +
> > + if ((nscnt & ICU_NSCNT_NSTAT) == ICU_NSCNT_NSTAT_DETECTED)
> > + writel_relaxed(ICU_NSCLR_NCLR, priv->base + ICU_NSCLR);
> > +}
> > +
> > +static void rzv2h_clear_irq_int(struct rzv2h_icu_priv *priv, unsigned int hwirq)
> > +{
> > + unsigned int irq_nr = hwirq - ICU_IRQ_START;
> > + u32 isctr, iitsr, iitsel;
> > + u32 bit = BIT(irq_nr);
> > +
> > + isctr = readl_relaxed(priv->base + ICU_ISCTR);
> > + iitsr = readl_relaxed(priv->base + ICU_IITSR);
> > + iitsel = ICU_IITSR_IITSEL_GET(iitsr, irq_nr);
>
> Not that I care about the performance of your device, but why do you
> need to read back the type configuration. It's known and cached in
> irq_data, no?
>
> Also this is invoked from eoi(), so why would the bit not be set when
> the interrupt is type edge and has fired? It should be set which means
> the ISCTR read is pointless too. Unless I'm missing something,
Both rzv2h_clear_irq_int and rzv2h_clear_tint_int are also called from
the irq_set_type path, that's why all the checks.
As you pointed out, no need for all those checks from within the eoi
path, therefore I'll implement whatever is required straight in
rzv2h_icu_eoi, to improve on performance.
>
> > +static void rzv2h_clear_tint_int(struct rzv2h_icu_priv *priv,
> > + unsigned int hwirq)
>
> No line break required.
Agreed.
>
> > +{
> > + unsigned int tint_nr = hwirq - ICU_TINT_START;
> > + int titsel_n = ICU_TITSR_TITSEL_N(tint_nr);
> > + u32 tsctr, titsr, titsel;
> > + u32 bit = BIT(tint_nr);
> > + int k = tint_nr / 16;
> > +
> > + tsctr = readl_relaxed(priv->base + ICU_TSCTR);
> > + titsr = readl_relaxed(priv->base + ICU_TITSR(k));
> > + titsel = ICU_TITSR_TITSEL_GET(titsr, titsel_n);
>
> Same comment.
Agreed.
>
> > +static void rzv2h_icu_eoi(struct irq_data *d)
> > +{
> > + struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
> > + unsigned int hw_irq = irqd_to_hwirq(d);
> > +
> > + raw_spin_lock(&priv->lock);
>
> scoped_guard(raw_spinlock, ....) {
Good point!
>
> > + if (hw_irq >= ICU_TINT_START)
> > + rzv2h_clear_tint_int(priv, hw_irq);
> > + else if (hw_irq >= ICU_IRQ_START)
> > + rzv2h_clear_irq_int(priv, hw_irq);
> > + else
> > + rzv2h_clear_nmi_int(priv);
>
> Huch. Is NMI a real NMI or just some unmaskable regular interrupt?
>
> If it's a real NMI, then you _cannot_ take the spinlock here.
It's not a real NMI, as it's mapped to SPI 0.
>
>
> > + raw_spin_unlock(&priv->lock);
> > +
> > + irq_chip_eoi_parent(d);
> > +}
> > +
> > +static void rzv2h_tint_irq_endisable(struct irq_data *d, bool enable)
> > +{
> > + struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
> > + unsigned int hw_irq = irqd_to_hwirq(d);
> > + u32 tint_nr, tssel_n, k, tssr;
> > +
> > + if (hw_irq < ICU_TINT_START)
> > + return;
> > +
> > + tint_nr = hw_irq - ICU_TINT_START;
> > + k = ICU_TSSR_K(tint_nr);
> > + tssel_n = ICU_TSSR_TSSEL_N(tint_nr);
> > +
> > + raw_spin_lock(&priv->lock);
>
> guard()
Agreed.
>
> > + tssr = readl_relaxed(priv->base + ICU_TSSR(k));
> > + if (enable)
> > + tssr |= ICU_TSSR_TIEN(tssel_n);
> > + else
> > + tssr &= ~ICU_TSSR_TIEN(tssel_n);
> > + writel_relaxed(tssr, priv->base + ICU_TSSR(k));
> > + raw_spin_unlock(&priv->lock);
> > +}
>
> > + raw_spin_lock(&priv->lock);
>
> guard()
Agreed.
>
> > +static const struct irq_domain_ops rzv2h_icu_domain_ops = {
> > + .alloc = rzv2h_icu_alloc,
> > + .free = irq_domain_free_irqs_common,
> > + .translate = irq_domain_translate_twocell,
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
Will fix.
>
> > +};
> > +
> > +static int rzv2h_icu_parse_interrupts(struct rzv2h_icu_priv *priv,
> > + struct device_node *np)
>
> Please get rid of the line breaks. You have 100 characters.
Thanks for pointing this out, I'll go through the whole file adjust accordingly.
I'll send a new version soon.
Thanks,
Fab
>
> Thanks,
>
> tglx
