| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87bk02ydzk.ffs@tglx>
Date: Wed, 02 Oct 2024 12:51:27 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Fabrizio Castro <fabrizio.castro.jz@...esas.com>, Philipp Zabel
<p.zabel@...gutronix.de>, Geert Uytterhoeven <geert+renesas@...der.be>
Cc: Fabrizio Castro <fabrizio.castro.jz@...esas.com>, Magnus Damm
<magnus.damm@...il.com>, linux-kernel@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, Chris Paterson
<Chris.Paterson2@...esas.com>, Biju Das <biju.das.jz@...renesas.com>, Lad
Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
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.
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,
> +static void rzv2h_clear_tint_int(struct rzv2h_icu_priv *priv,
> + unsigned int hwirq)
No line break required.
> +{
> + 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.
> +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, ....) {
> + 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.
> + 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()
> + 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()
> +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
> +};
> +
> +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,
tglx
Powered by blists - more mailing lists