2024-01-29 15:16:39

by Prabhakar

[permalink] [raw]
Subject: [PATCH 0/5] Add IAX45 support for RZ/Five SoC

From: Lad Prabhakar <[email protected]>

Hi All,

The IAX45 block on RZ/Five SoC is almost identical to the IRQC bock found
on the RZ/G2L family of SoCs.

IAX45 performs various interrupt controls including synchronization for the
external interrupts of NMI, IRQ, and GPIOINT and the interrupts of the
built-in peripheral interrupts output by each module. And it notifies the
interrupt to the PLIC.
- Select 32 TINT from 82 GPIOINT.
- Integration of bus error interrupts from system bus.
- Integration of ECC error interrupts from On-chip RAM.
- Indicate interrupt status. (NMI, IRQ, TINT, integrated bus error interrupt
and integrated ECC error interrupt)
- Setting of interrupt detection method. (NMI, IRQ and TINT)
- All interrupts are masked by INTMASK.
- Mask function for NMI, IRQ and TINT

This patch series adds support for IAX45 in the IRQC driver and enables this
on RZ/Five SoC.

Cheers,
Prabhakar

Lad Prabhakar (5):
dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document
RZ/Five SoC
irqchip/renesas-rzg2l: Add support for RZ/Five SoC
riscv: dts: renesas: r9a07g043f: Add IRQC node to RZ/Five SoC DTSI
arm64: dts: renesas: r9a07g043: Move interrupt-parent property to
common DTSI
riscv: dts: renesas: rzfive-smarc-som: Drop deleting interrupt
properties from ETH0/1 nodes

.../renesas,rzg2l-irqc.yaml | 27 ++++
arch/arm64/boot/dts/renesas/r9a07g043.dtsi | 1 +
arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 4 -
arch/riscv/boot/dts/renesas/r9a07g043f.dtsi | 76 ++++++++++
.../boot/dts/renesas/rzfive-smarc-som.dtsi | 16 ---
drivers/irqchip/irq-renesas-rzg2l.c | 132 +++++++++++++++++-
6 files changed, 232 insertions(+), 24 deletions(-)

--
2.34.1



2024-01-29 15:16:49

by Prabhakar

[permalink] [raw]
Subject: [PATCH 1/5] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document RZ/Five SoC

From: Lad Prabhakar <[email protected]>

Document RZ/Five (R9A07G043F) IRQC bindings. The IRQC block on RZ/Five SoC
is almost identical to one found on the RZ/G2L SoC with below differences,
* Additional BUS error interrupt
* Additional ECCRAM error interrupt
* Has additional mask control registers for NMI/IRQ/TINT

Hence new compatible string "renesas,r9a07g043f-irqc" is added for RZ/Five
SoC.

Signed-off-by: Lad Prabhakar <[email protected]>
---
.../renesas,rzg2l-irqc.yaml | 27 +++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
index d3b5aec0a3f7..3abc01e48934 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
@@ -23,6 +23,7 @@ properties:
compatible:
items:
- enum:
+ - renesas,r9a07g043f-irqc # RZ/Five
- renesas,r9a07g043u-irqc # RZ/G2UL
- renesas,r9a07g044-irqc # RZ/G2{L,LC}
- renesas,r9a07g054-irqc # RZ/V2L
@@ -88,6 +89,12 @@ properties:
- description: GPIO interrupt, TINT30
- description: GPIO interrupt, TINT31
- description: Bus error interrupt
+ - description: ECCRAM0 TIE1 interrupt
+ - description: ECCRAM0 TIE2 interrupt
+ - description: ECCRAM0 overflow interrupt
+ - description: ECCRAM1 TIE1 interrupt
+ - description: ECCRAM1 TIE2 interrupt
+ - description: ECCRAM1 overflow interrupt

interrupt-names:
minItems: 41
@@ -134,6 +141,12 @@ properties:
- const: tint30
- const: tint31
- const: bus-err
+ - const: eccram0-tie1
+ - const: eccram0-tie2
+ - const: eccram0-ovf
+ - const: eccram1-tie1
+ - const: eccram1-tie2
+ - const: eccram1-ovf

clocks:
maxItems: 2
@@ -180,6 +193,20 @@ allOf:
required:
- interrupt-names

+ - if:
+ properties:
+ compatible:
+ contains:
+ const: renesas,r9a07g043f-irqc
+ then:
+ properties:
+ interrupts:
+ minItems: 48
+ interrupt-names:
+ minItems: 48
+ required:
+ - interrupt-names
+
unevaluatedProperties: false

examples:
--
2.34.1


2024-01-29 15:17:11

by Prabhakar

[permalink] [raw]
Subject: [PATCH 3/5] riscv: dts: renesas: r9a07g043f: Add IRQC node to RZ/Five SoC DTSI

From: Lad Prabhakar <[email protected]>

Add the IRQC node to RZ/Five (R9A07G043F) SoC DTSI.

Signed-off-by: Lad Prabhakar <[email protected]>
---
arch/riscv/boot/dts/renesas/r9a07g043f.dtsi | 76 +++++++++++++++++++++
1 file changed, 76 insertions(+)

diff --git a/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi b/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
index d7a66043f13b..d2272a0bfb61 100644
--- a/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
+++ b/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
@@ -50,6 +50,82 @@ &soc {
dma-noncoherent;
interrupt-parent = <&plic>;

+ irqc: interrupt-controller@110a0000 {
+ compatible = "renesas,r9a07g043f-irqc",
+ "renesas,rzg2l-irqc";
+ reg = <0 0x110a0000 0 0x20000>;
+ #interrupt-cells = <2>;
+ #address-cells = <0>;
+ interrupt-controller;
+ interrupts = <SOC_PERIPHERAL_IRQ(0) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(1) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(2) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(3) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(4) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(5) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(6) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(7) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(8) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(444) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(445) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(446) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(447) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(448) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(449) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(450) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(451) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(452) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(453) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(454) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(455) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(456) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(457) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(458) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(459) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(460) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(461) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(462) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(463) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(464) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(465) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(466) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(467) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(468) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(469) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(470) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(471) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(472) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(473) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(474) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(475) IRQ_TYPE_LEVEL_HIGH>,
+ <SOC_PERIPHERAL_IRQ(25) IRQ_TYPE_EDGE_RISING>,
+ <SOC_PERIPHERAL_IRQ(34) IRQ_TYPE_EDGE_RISING>,
+ <SOC_PERIPHERAL_IRQ(35) IRQ_TYPE_EDGE_RISING>,
+ <SOC_PERIPHERAL_IRQ(36) IRQ_TYPE_EDGE_RISING>,
+ <SOC_PERIPHERAL_IRQ(37) IRQ_TYPE_EDGE_RISING>,
+ <SOC_PERIPHERAL_IRQ(38) IRQ_TYPE_EDGE_RISING>,
+ <SOC_PERIPHERAL_IRQ(39) IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "nmi",
+ "irq0", "irq1", "irq2", "irq3",
+ "irq4", "irq5", "irq6", "irq7",
+ "tint0", "tint1", "tint2", "tint3",
+ "tint4", "tint5", "tint6", "tint7",
+ "tint8", "tint9", "tint10", "tint11",
+ "tint12", "tint13", "tint14", "tint15",
+ "tint16", "tint17", "tint18", "tint19",
+ "tint20", "tint21", "tint22", "tint23",
+ "tint24", "tint25", "tint26", "tint27",
+ "tint28", "tint29", "tint30", "tint31",
+ "bus-err", "eccram0-tie1", "eccram0-tie2",
+ "eccram0-ovf", "eccram1-tie1", "eccram1-tie2",
+ "eccram1-ovf";
+ clocks = <&cpg CPG_MOD R9A07G043_IAX45_CLK>,
+ <&cpg CPG_MOD R9A07G043_IAX45_PCLK>;
+ clock-names = "clk", "pclk";
+ power-domains = <&cpg>;
+ resets = <&cpg R9A07G043_IAX45_RESETN>;
+ };
+
plic: interrupt-controller@12c00000 {
compatible = "renesas,r9a07g043-plic", "andestech,nceplic100";
#interrupt-cells = <2>;
--
2.34.1


2024-01-29 15:17:15

by Prabhakar

[permalink] [raw]
Subject: [PATCH 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC

From: Lad Prabhakar <[email protected]>

The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared
to the RZ/G2L (family) SoC.

Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC
controller driver. Two new registers, IMSK and TMSK, are defined to
handle masking on RZ/Five SoC. The implementation utilizes a new data
structure, `struct rzg2l_irqc_data`, to determine mask support for a
specific controller instance.

Signed-off-by: Lad Prabhakar <[email protected]>
---
drivers/irqchip/irq-renesas-rzg2l.c | 132 +++++++++++++++++++++++++++-
1 file changed, 128 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index 9494fc26259c..949280f95c29 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -37,6 +37,8 @@
#define TSSEL_SHIFT(n) (8 * (n))
#define TSSEL_MASK GENMASK(7, 0)
#define IRQ_MASK 0x3
+#define IMSK 0x10010
+#define TMSK 0x10020

#define TSSR_OFFSET(n) ((n) % 4)
#define TSSR_INDEX(n) ((n) / 4)
@@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache {
u32 titsr[2];
};

+/**
+ * struct rzg2l_irqc_data - OF data structure
+ * @mask_supported: Indicates if mask registers are available
+ */
+struct rzg2l_irqc_data {
+ bool mask_supported;
+};
+
/**
* struct rzg2l_irqc_priv - IRQ controller private data structure
* @base: Controller's base address
+ * @data: OF data pointer
* @fwspec: IRQ firmware specific data
* @lock: Lock to serialize access to hardware registers
* @cache: Registers cache for suspend/resume
*/
static struct rzg2l_irqc_priv {
void __iomem *base;
+ const struct rzg2l_irqc_data *data;
struct irq_fwspec fwspec[IRQC_NUM_IRQ];
raw_spinlock_t lock;
struct rzg2l_irqc_reg_cache cache;
@@ -129,44 +141,136 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
irq_chip_eoi_parent(d);
}

+static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv,
+ unsigned int hwirq)
+{
+ u32 imsk = readl_relaxed(priv->base + IMSK);
+ u32 bit = BIT(hwirq - IRQC_IRQ_START);
+
+ writel_relaxed(imsk | bit, priv->base + IMSK);
+}
+
+static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv,
+ unsigned int hwirq)
+{
+ u32 imsk = readl_relaxed(priv->base + IMSK);
+ u32 bit = BIT(hwirq - IRQC_IRQ_START);
+
+ writel_relaxed(imsk & ~bit, priv->base + IMSK);
+}
+
+static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv,
+ unsigned int hwirq)
+{
+ u32 tmsk = readl_relaxed(priv->base + TMSK);
+ u32 bit = BIT(hwirq - IRQC_TINT_START);
+
+ writel_relaxed(tmsk | bit, priv->base + TMSK);
+}
+
+static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv,
+ unsigned int hwirq)
+{
+ u32 tmsk = readl_relaxed(priv->base + TMSK);
+ u32 bit = BIT(hwirq - IRQC_TINT_START);
+
+ writel_relaxed(tmsk & ~bit, priv->base + TMSK);
+}
+
+/* Must be called while priv->lock is held */
+static void rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq)
+{
+ if (!priv->data->mask_supported)
+ return;
+
+ if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT)
+ rzg2l_irqc_mask_irq_interrupt(priv, hwirq);
+ else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ)
+ rzg2l_irqc_mask_tint_interrupt(priv, hwirq);
+}
+
+static void rzg2l_irqc_mask(struct irq_data *d)
+{
+ struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
+
+ raw_spin_lock(&priv->lock);
+ rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d));
+ raw_spin_unlock(&priv->lock);
+ irq_chip_mask_parent(d);
+}
+
+/* Must be called while priv->lock is held */
+static void rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq)
+{
+ if (!priv->data->mask_supported)
+ return;
+
+ if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT)
+ rzg2l_irqc_unmask_irq_interrupt(priv, hwirq);
+ else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ)
+ rzg2l_irqc_unmask_tint_interrupt(priv, hwirq);
+}
+
+static void rzg2l_irqc_unmask(struct irq_data *d)
+{
+ struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
+
+ raw_spin_lock(&priv->lock);
+ rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d));
+ raw_spin_unlock(&priv->lock);
+ irq_chip_unmask_parent(d);
+}
+
static void rzg2l_irqc_irq_disable(struct irq_data *d)
{
+ struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
unsigned int hw_irq = irqd_to_hwirq(d);

if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) {
- struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
u32 offset = hw_irq - IRQC_TINT_START;
u32 tssr_offset = TSSR_OFFSET(offset);
u8 tssr_index = TSSR_INDEX(offset);
u32 reg;

raw_spin_lock(&priv->lock);
+ rzg2l_irqc_mask_once(priv, hw_irq);
reg = readl_relaxed(priv->base + TSSR(tssr_index));
reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset));
writel_relaxed(reg, priv->base + TSSR(tssr_index));
raw_spin_unlock(&priv->lock);
+ } else {
+ raw_spin_lock(&priv->lock);
+ rzg2l_irqc_mask_once(priv, hw_irq);
+ raw_spin_unlock(&priv->lock);
}
+
irq_chip_disable_parent(d);
}

static void rzg2l_irqc_irq_enable(struct irq_data *d)
{
+ struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
unsigned int hw_irq = irqd_to_hwirq(d);

if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) {
unsigned long tint = (uintptr_t)irq_data_get_irq_chip_data(d);
- struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
u32 offset = hw_irq - IRQC_TINT_START;
u32 tssr_offset = TSSR_OFFSET(offset);
u8 tssr_index = TSSR_INDEX(offset);
u32 reg;

raw_spin_lock(&priv->lock);
+ rzg2l_irqc_unmask_once(priv, hw_irq);
reg = readl_relaxed(priv->base + TSSR(tssr_index));
reg |= (TIEN | tint) << TSSEL_SHIFT(tssr_offset);
writel_relaxed(reg, priv->base + TSSR(tssr_index));
raw_spin_unlock(&priv->lock);
+ } else {
+ raw_spin_lock(&priv->lock);
+ rzg2l_irqc_unmask_once(priv, hw_irq);
+ raw_spin_unlock(&priv->lock);
}
+
irq_chip_enable_parent(d);
}

@@ -294,8 +398,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops = {
static const struct irq_chip irqc_chip = {
.name = "rzg2l-irqc",
.irq_eoi = rzg2l_irqc_eoi,
- .irq_mask = irq_chip_mask_parent,
- .irq_unmask = irq_chip_unmask_parent,
+ .irq_mask = rzg2l_irqc_mask,
+ .irq_unmask = rzg2l_irqc_unmask,
.irq_disable = rzg2l_irqc_irq_disable,
.irq_enable = rzg2l_irqc_irq_enable,
.irq_get_irqchip_state = irq_chip_get_parent_state,
@@ -371,9 +475,23 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
return 0;
}

+static const struct rzg2l_irqc_data rzfive_irqc_data = {
+ .mask_supported = true,
+};
+
+static const struct rzg2l_irqc_data rzg2l_irqc_default_data = {
+ .mask_supported = false,
+};
+
+static const struct of_device_id rzg2l_irqc_matches[] = {
+ { .compatible = "renesas,r9a07g043f-irqc", .data = &rzfive_irqc_data },
+ { }
+};
+
static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
{
struct irq_domain *irq_domain, *parent_domain;
+ const struct of_device_id *match;
struct platform_device *pdev;
struct reset_control *resetn;
int ret;
@@ -392,6 +510,12 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
if (!rzg2l_irqc_data)
return -ENOMEM;

+ match = of_match_node(rzg2l_irqc_matches, node);
+ if (match)
+ rzg2l_irqc_data->data = match->data;
+ else
+ rzg2l_irqc_data->data = &rzg2l_irqc_default_data;
+
rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
if (IS_ERR(rzg2l_irqc_data->base))
return PTR_ERR(rzg2l_irqc_data->base);
--
2.34.1


2024-01-29 15:17:55

by Prabhakar

[permalink] [raw]
Subject: [PATCH 5/5] riscv: dts: renesas: rzfive-smarc-som: Drop deleting interrupt properties from ETH0/1 nodes

From: Lad Prabhakar <[email protected]>

Now that we have enabled IRQC support for RZ/Five SoC switch to interrupt
mode for ethernet0/1 PHYs instead of polling mode.

Signed-off-by: Lad Prabhakar <[email protected]>
---
.../riscv/boot/dts/renesas/rzfive-smarc-som.dtsi | 16 ----------------
1 file changed, 16 deletions(-)

diff --git a/arch/riscv/boot/dts/renesas/rzfive-smarc-som.dtsi b/arch/riscv/boot/dts/renesas/rzfive-smarc-som.dtsi
index 72d9b6fba526..86b2f15375ec 100644
--- a/arch/riscv/boot/dts/renesas/rzfive-smarc-som.dtsi
+++ b/arch/riscv/boot/dts/renesas/rzfive-smarc-som.dtsi
@@ -7,22 +7,6 @@

#include <arm64/renesas/rzg2ul-smarc-som.dtsi>

-#if (!SW_ET0_EN_N)
-&eth0 {
- phy0: ethernet-phy@7 {
- /delete-property/ interrupt-parent;
- /delete-property/ interrupts;
- };
-};
-#endif
-
-&eth1 {
- phy1: ethernet-phy@7 {
- /delete-property/ interrupt-parent;
- /delete-property/ interrupts;
- };
-};
-
&sbc {
status = "disabled";
};
--
2.34.1


2024-01-29 15:30:12

by Prabhakar

[permalink] [raw]
Subject: [PATCH 4/5] arm64: dts: renesas: r9a07g043: Move interrupt-parent property to common DTSI

From: Lad Prabhakar <[email protected]>

Now that we have added support for IRQC to both RZ/Five and RZ/G2UL SoCs
we can move the interrupt-parent for pinctrl node back to the common
shared r9a07g043.dtsi file.

Signed-off-by: Lad Prabhakar <[email protected]>
---
arch/arm64/boot/dts/renesas/r9a07g043.dtsi | 1 +
arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 4 ----
2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
index 8721f4c9fa0f..d2365def1059 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
@@ -598,6 +598,7 @@ pinctrl: pinctrl@11030000 {
gpio-ranges = <&pinctrl 0 0 152>;
#interrupt-cells = <2>;
interrupt-controller;
+ interrupt-parent = <&irqc>;
clocks = <&cpg CPG_MOD R9A07G043_GPIO_HCLK>;
power-domains = <&cpg>;
resets = <&cpg R9A07G043_GPIO_RSTN>,
diff --git a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
index 2ab231572d95..0e931c88afa5 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
@@ -54,10 +54,6 @@ timer {
};
};

-&pinctrl {
- interrupt-parent = <&irqc>;
-};
-
&soc {
interrupt-parent = <&gic>;

--
2.34.1


2024-01-29 17:31:54

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH 1/5] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document RZ/Five SoC

On Mon, Jan 29, 2024 at 03:16:14PM +0000, Prabhakar wrote:
> From: Lad Prabhakar <[email protected]>
>
> Document RZ/Five (R9A07G043F) IRQC bindings. The IRQC block on RZ/Five SoC
> is almost identical to one found on the RZ/G2L SoC with below differences,
> * Additional BUS error interrupt
> * Additional ECCRAM error interrupt
> * Has additional mask control registers for NMI/IRQ/TINT
>
> Hence new compatible string "renesas,r9a07g043f-irqc" is added for RZ/Five
> SoC.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> ---
> .../renesas,rzg2l-irqc.yaml | 27 +++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> index d3b5aec0a3f7..3abc01e48934 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> @@ -23,6 +23,7 @@ properties:
> compatible:
> items:
> - enum:
> + - renesas,r9a07g043f-irqc # RZ/Five
> - renesas,r9a07g043u-irqc # RZ/G2UL
> - renesas,r9a07g044-irqc # RZ/G2{L,LC}
> - renesas,r9a07g054-irqc # RZ/V2L
> @@ -88,6 +89,12 @@ properties:
> - description: GPIO interrupt, TINT30
> - description: GPIO interrupt, TINT31
> - description: Bus error interrupt
> + - description: ECCRAM0 TIE1 interrupt
> + - description: ECCRAM0 TIE2 interrupt
> + - description: ECCRAM0 overflow interrupt
> + - description: ECCRAM1 TIE1 interrupt
> + - description: ECCRAM1 TIE2 interrupt
> + - description: ECCRAM1 overflow interrupt
>
> interrupt-names:
> minItems: 41
> @@ -134,6 +141,12 @@ properties:
> - const: tint30
> - const: tint31
> - const: bus-err
> + - const: eccram0-tie1
> + - const: eccram0-tie2
> + - const: eccram0-ovf
> + - const: eccram1-tie1
> + - const: eccram1-tie2
> + - const: eccram1-ovf

I think the restrictions already in the file become incorrect with this
patch:
- if:
properties:
compatible:
contains:
enum:
- renesas,r9a07g043u-irqc
- renesas,r9a08g045-irqc
then:
properties:
interrupts:
minItems: 42
interrupt-names:
minItems: 42
required:
- interrupt-names

This used to require all 42 interrupts for the two compatibles here
and at least the first 41 otherwise. Now you've increased the number of
interrupts to 48 thereby removing the upper limits on the existing
devices.

Given the commit message, I figure that providing 48 interrupts for
(at least some of) those devices would be incorrect?

Cheers,
Conor.

>
> clocks:
> maxItems: 2
> @@ -180,6 +193,20 @@ allOf:
> required:
> - interrupt-names
>
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: renesas,r9a07g043f-irqc
> + then:
> + properties:
> + interrupts:
> + minItems: 48
> + interrupt-names:
> + minItems: 48
> + required:
> + - interrupt-names
> +
> unevaluatedProperties: false
>
> examples:
> --
> 2.34.1
>


Attachments:
(No filename) (3.46 kB)
signature.asc (235.00 B)
Download all attachments

2024-01-30 11:24:13

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 1/5] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document RZ/Five SoC

Hi Prabhakar,

On Mon, Jan 29, 2024 at 6:30 PM Conor Dooley <[email protected]> wrote:
> On Mon, Jan 29, 2024 at 03:16:14PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > Document RZ/Five (R9A07G043F) IRQC bindings. The IRQC block on RZ/Five SoC
> > is almost identical to one found on the RZ/G2L SoC with below differences,
> > * Additional BUS error interrupt
> > * Additional ECCRAM error interrupt
> > * Has additional mask control registers for NMI/IRQ/TINT
> >
> > Hence new compatible string "renesas,r9a07g043f-irqc" is added for RZ/Five
> > SoC.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>

Thanks for your patch!

> > --- a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > @@ -23,6 +23,7 @@ properties:
> > compatible:
> > items:
> > - enum:
> > + - renesas,r9a07g043f-irqc # RZ/Five
> > - renesas,r9a07g043u-irqc # RZ/G2UL
> > - renesas,r9a07g044-irqc # RZ/G2{L,LC}
> > - renesas,r9a07g054-irqc # RZ/V2L
> > @@ -88,6 +89,12 @@ properties:
> > - description: GPIO interrupt, TINT30
> > - description: GPIO interrupt, TINT31
> > - description: Bus error interrupt
> > + - description: ECCRAM0 TIE1 interrupt

ECCRAM0 1bit error interrupt?

> > + - description: ECCRAM0 TIE2 interrupt

ECCRAM0 2bit error interrupt?

> > + - description: ECCRAM0 overflow interrupt

ECCRAM0 error overflow interrupt?

> > + - description: ECCRAM1 TIE1 interrupt
> > + - description: ECCRAM1 TIE2 interrupt
> > + - description: ECCRAM1 overflow interrupt

Likewise.

> > interrupt-names:
> > minItems: 41
> > @@ -134,6 +141,12 @@ properties:
> > - const: tint30
> > - const: tint31
> > - const: bus-err
> > + - const: eccram0-tie1
> > + - const: eccram0-tie2
> > + - const: eccram0-ovf
> > + - const: eccram1-tie1
> > + - const: eccram1-tie2
> > + - const: eccram1-ovf

Why not use the naming from the docs (all 6 include "ti")?
EC7TIE1_0, EC7TIE2_0, EC7TIOVF_0, EC7TIE1_1, EC7TIE2_1, EC7TIOVF_1
=> ec7tie1-0, ec7tie2-0, ec7tiovf-0, ...?

> I think the restrictions already in the file become incorrect with this
> patch:
> - if:
> properties:
> compatible:
> contains:
> enum:
> - renesas,r9a07g043u-irqc
> - renesas,r9a08g045-irqc
> then:
> properties:
> interrupts:
> minItems: 42
> interrupt-names:
> minItems: 42
> required:
> - interrupt-names
>
> This used to require all 42 interrupts for the two compatibles here
> and at least the first 41 otherwise. Now you've increased the number of
> interrupts to 48 thereby removing the upper limits on the existing
> devices.

I'm gonna repeat (and extend) my question from [1]: How come we thought
RZ/G2L and RZ/V2L do not have the bus error and ECCRAM interrupts?
Looks like most of the conditional handling can be removed (see below).

> Given the commit message, I figure that providing 48 interrupts for
> (at least some of) those devices would be incorrect?

Looks like all of RZ/G2L{,C}, RZ/V2L, RZ/G2UL, and RZ/Five support
all 48 interrupts. RZ/G3S lacks the final three for ECCRAM1.

[1] "Re: [PATCH v3 8/9] dt-bindings: interrupt-controller:
renesas,rzg2l-irqc: Document RZ/G3S"
https://lore.kernel.org/r/CAMuHMdX88KRnvJchUwrWcgmPooAESOT2492Nr1Z_5UMng3q__Q@mail.gmail.com

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-01-30 11:28:59

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 3/5] riscv: dts: renesas: r9a07g043f: Add IRQC node to RZ/Five SoC DTSI

Hi Prabhakar,

On Mon, Jan 29, 2024 at 4:16 PM Prabhakar <[email protected]> wrote:
> From: Lad Prabhakar <[email protected]>
>
> Add the IRQC node to RZ/Five (R9A07G043F) SoC DTSI.

Thanks for your patch!

> Signed-off-by: Lad Prabhakar <[email protected]>

> --- a/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
> +++ b/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
> @@ -50,6 +50,82 @@ &soc {
> dma-noncoherent;
> interrupt-parent = <&plic>;
>
> + irqc: interrupt-controller@110a0000 {
> + compatible = "renesas,r9a07g043f-irqc",
> + "renesas,rzg2l-irqc";
> + reg = <0 0x110a0000 0 0x20000>;
> + #interrupt-cells = <2>;
> + #address-cells = <0>;
> + interrupt-controller;
> + interrupts = <SOC_PERIPHERAL_IRQ(0) IRQ_TYPE_LEVEL_HIGH>,

As this is the RZ/Five-specific .dtsi file, and not the common base
dtsi, you could avoid using SOC_PERIPHERAL_IRQ() here.
I am not sure what is most readable...

The rest LGTM (pending interrupt names review comments).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-01-30 11:36:28

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 5/5] riscv: dts: renesas: rzfive-smarc-som: Drop deleting interrupt properties from ETH0/1 nodes

On Mon, Jan 29, 2024 at 4:16 PM Prabhakar <[email protected]> wrote:
> From: Lad Prabhakar <[email protected]>
>
> Now that we have enabled IRQC support for RZ/Five SoC switch to interrupt
> mode for ethernet0/1 PHYs instead of polling mode.
>
> Signed-off-by: Lad Prabhakar <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-01-30 11:40:45

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC

Hi Prabhakar,

On Mon, Jan 29, 2024 at 4:16 PM Prabhakar <[email protected]> wrote:
> From: Lad Prabhakar <[email protected]>
>
> The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared
> to the RZ/G2L (family) SoC.
>
> Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC
> controller driver. Two new registers, IMSK and TMSK, are defined to
> handle masking on RZ/Five SoC. The implementation utilizes a new data
> structure, `struct rzg2l_irqc_data`, to determine mask support for a
> specific controller instance.
>
> Signed-off-by: Lad Prabhakar <[email protected]>

Thanks for your patch!

> --- a/drivers/irqchip/irq-renesas-rzg2l.c
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache {
> u32 titsr[2];
> };
>
> +/**
> + * struct rzg2l_irqc_data - OF data structure
> + * @mask_supported: Indicates if mask registers are available
> + */
> +struct rzg2l_irqc_data {

This structure has the same name as the single static struct
rzg2l_irqc_priv instance, which is confusing.

> + bool mask_supported;
> +};
> +
> /**
> * struct rzg2l_irqc_priv - IRQ controller private data structure
> * @base: Controller's base address
> + * @data: OF data pointer
> * @fwspec: IRQ firmware specific data
> * @lock: Lock to serialize access to hardware registers
> * @cache: Registers cache for suspend/resume
> */
> static struct rzg2l_irqc_priv {
> void __iomem *base;
> + const struct rzg2l_irqc_data *data;

Replacing this by a bool would avoid a pointer dereference in each user,
and allows you to make rzg2l_irqc_data etc. __initconst.

> struct irq_fwspec fwspec[IRQC_NUM_IRQ];
> raw_spinlock_t lock;
> struct rzg2l_irqc_reg_cache cache;

> @@ -371,9 +475,23 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
> return 0;
> }
>
> +static const struct rzg2l_irqc_data rzfive_irqc_data = {
> + .mask_supported = true,
> +};
> +
> +static const struct rzg2l_irqc_data rzg2l_irqc_default_data = {
> + .mask_supported = false,
> +};
> +
> +static const struct of_device_id rzg2l_irqc_matches[] = {
> + { .compatible = "renesas,r9a07g043f-irqc", .data = &rzfive_irqc_data },
> + { }
> +};
> +
> static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> {
> struct irq_domain *irq_domain, *parent_domain;
> + const struct of_device_id *match;
> struct platform_device *pdev;
> struct reset_control *resetn;
> int ret;
> @@ -392,6 +510,12 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> if (!rzg2l_irqc_data)
> return -ENOMEM;
>
> + match = of_match_node(rzg2l_irqc_matches, node);
> + if (match)
> + rzg2l_irqc_data->data = match->data;
> + else
> + rzg2l_irqc_data->data = &rzg2l_irqc_default_data;

Instead of matching a second time, I'd rather add a second
IRQCHIP_MATCH() entry with a different init function, passing the
actual rzg2l_irqc_data pointer.

> +
> rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
> if (IS_ERR(rzg2l_irqc_data->base))
> return PTR_ERR(rzg2l_irqc_data->base);

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-01-30 11:54:59

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 4/5] arm64: dts: renesas: r9a07g043: Move interrupt-parent property to common DTSI

On Mon, Jan 29, 2024 at 4:16 PM Prabhakar <[email protected]> wrote:
> From: Lad Prabhakar <[email protected]>
>
> Now that we have added support for IRQC to both RZ/Five and RZ/G2UL SoCs
> we can move the interrupt-parent for pinctrl node back to the common
> shared r9a07g043.dtsi file.
>
> Signed-off-by: Lad Prabhakar <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-01-30 13:07:53

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 1/5] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document RZ/Five SoC

Hi Prabhakar,

On Tue, Jan 30, 2024 at 1:59 PM Lad, Prabhakar
<[email protected]> wrote:
> On Tue, Jan 30, 2024 at 11:13 AM Geert Uytterhoeven
> <[email protected]> wrote:
> > On Mon, Jan 29, 2024 at 6:30 PM Conor Dooley <[email protected]> wrote:
> > > On Mon, Jan 29, 2024 at 03:16:14PM +0000, Prabhakar wrote:
> > > > From: Lad Prabhakar <[email protected]>
> > > >
> > > > Document RZ/Five (R9A07G043F) IRQC bindings. The IRQC block on RZ/Five SoC
> > > > is almost identical to one found on the RZ/G2L SoC with below differences,
> > > > * Additional BUS error interrupt
> > > > * Additional ECCRAM error interrupt
> > > > * Has additional mask control registers for NMI/IRQ/TINT
> > > >
> > > > Hence new compatible string "renesas,r9a07g043f-irqc" is added for RZ/Five
> > > > SoC.
> > > >
> > > > Signed-off-by: Lad Prabhakar <[email protected]>
> >
> > > > --- a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > > > @@ -134,6 +141,12 @@ properties:
> > > > - const: tint30
> > > > - const: tint31
> > > > - const: bus-err
> > > > + - const: eccram0-tie1
> > > > + - const: eccram0-tie2
> > > > + - const: eccram0-ovf
> > > > + - const: eccram1-tie1
> > > > + - const: eccram1-tie2
> > > > + - const: eccram1-ovf
> >
> > Why not use the naming from the docs (all 6 include "ti")?
> > EC7TIE1_0, EC7TIE2_0, EC7TIOVF_0, EC7TIE1_1, EC7TIE2_1, EC7TIOVF_1
> > => ec7tie1-0, ec7tie2-0, ec7tiovf-0, ...?
> >
> Agreed.
>
> > > I think the restrictions already in the file become incorrect with this
> > > patch:
> > > - if:
> > > properties:
> > > compatible:
> > > contains:
> > > enum:
> > > - renesas,r9a07g043u-irqc
> > > - renesas,r9a08g045-irqc
> > > then:
> > > properties:
> > > interrupts:
> > > minItems: 42
> > > interrupt-names:
> > > minItems: 42
> > > required:
> > > - interrupt-names
> > >
> > > This used to require all 42 interrupts for the two compatibles here
> > > and at least the first 41 otherwise. Now you've increased the number of
> > > interrupts to 48 thereby removing the upper limits on the existing
> > > devices.
> >
> > I'm gonna repeat (and extend) my question from [1]: How come we thought
> > RZ/G2L and RZ/V2L do not have the bus error and ECCRAM interrupts?
> >
> Hmm not sure how this was missed earlier.
>
> > Looks like most of the conditional handling can be removed (see below).
> >
> > > Given the commit message, I figure that providing 48 interrupts for
> > > (at least some of) those devices would be incorrect?
> >
> > Looks like all of RZ/G2L{,C}, RZ/V2L, RZ/G2UL, and RZ/Five support
> > all 48 interrupts. RZ/G3S lacks the final three for ECCRAM1.
> >
> Agreed for RZ/G2L{,C}, RZ/V2L, RZ/G2UL, and RZ/Five, but for RZ/G3S it
> becomes tricky the interrupts for ECCRAM0/1 are combined hence they
> have just 3 interrupts. How do you propose the above interrupt naming?

I guess it doesn't hurt to have an index 0 on a part that has only a
single set?

Alternatives would be to
1. Drop the index completely on RZ/G3S, complicating bindings and
driver,
1. Drop the index for the first set, and use index 2 for the second set,
causing the names to differ even more on parts with 2 sets.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-01-30 13:42:52

by Prabhakar

[permalink] [raw]
Subject: Re: [PATCH 1/5] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document RZ/Five SoC

Hi Geert,

Thank you for the review.

On Tue, Jan 30, 2024 at 11:13 AM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jan 29, 2024 at 6:30 PM Conor Dooley <[email protected]> wrote:
> > On Mon, Jan 29, 2024 at 03:16:14PM +0000, Prabhakar wrote:
> > > From: Lad Prabhakar <[email protected]>
> > >
> > > Document RZ/Five (R9A07G043F) IRQC bindings. The IRQC block on RZ/Five SoC
> > > is almost identical to one found on the RZ/G2L SoC with below differences,
> > > * Additional BUS error interrupt
> > > * Additional ECCRAM error interrupt
> > > * Has additional mask control registers for NMI/IRQ/TINT
> > >
> > > Hence new compatible string "renesas,r9a07g043f-irqc" is added for RZ/Five
> > > SoC.
> > >
> > > Signed-off-by: Lad Prabhakar <[email protected]>
>
> Thanks for your patch!
>
> > > --- a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > > @@ -23,6 +23,7 @@ properties:
> > > compatible:
> > > items:
> > > - enum:
> > > + - renesas,r9a07g043f-irqc # RZ/Five
> > > - renesas,r9a07g043u-irqc # RZ/G2UL
> > > - renesas,r9a07g044-irqc # RZ/G2{L,LC}
> > > - renesas,r9a07g054-irqc # RZ/V2L
> > > @@ -88,6 +89,12 @@ properties:
> > > - description: GPIO interrupt, TINT30
> > > - description: GPIO interrupt, TINT31
> > > - description: Bus error interrupt
> > > + - description: ECCRAM0 TIE1 interrupt
>
> ECCRAM0 1bit error interrupt?
>
OK.

> > > + - description: ECCRAM0 TIE2 interrupt
>
> ECCRAM0 2bit error interrupt?
>
OK.
> > > + - description: ECCRAM0 overflow interrupt
>
> ECCRAM0 error overflow interrupt?
>
> > > + - description: ECCRAM1 TIE1 interrupt
> > > + - description: ECCRAM1 TIE2 interrupt
> > > + - description: ECCRAM1 overflow interrupt
>
> Likewise.
>
OK.

> > > interrupt-names:
> > > minItems: 41
> > > @@ -134,6 +141,12 @@ properties:
> > > - const: tint30
> > > - const: tint31
> > > - const: bus-err
> > > + - const: eccram0-tie1
> > > + - const: eccram0-tie2
> > > + - const: eccram0-ovf
> > > + - const: eccram1-tie1
> > > + - const: eccram1-tie2
> > > + - const: eccram1-ovf
>
> Why not use the naming from the docs (all 6 include "ti")?
> EC7TIE1_0, EC7TIE2_0, EC7TIOVF_0, EC7TIE1_1, EC7TIE2_1, EC7TIOVF_1
> => ec7tie1-0, ec7tie2-0, ec7tiovf-0, ...?
>
Agreed.

> > I think the restrictions already in the file become incorrect with this
> > patch:
> > - if:
> > properties:
> > compatible:
> > contains:
> > enum:
> > - renesas,r9a07g043u-irqc
> > - renesas,r9a08g045-irqc
> > then:
> > properties:
> > interrupts:
> > minItems: 42
> > interrupt-names:
> > minItems: 42
> > required:
> > - interrupt-names
> >
> > This used to require all 42 interrupts for the two compatibles here
> > and at least the first 41 otherwise. Now you've increased the number of
> > interrupts to 48 thereby removing the upper limits on the existing
> > devices.
>
> I'm gonna repeat (and extend) my question from [1]: How come we thought
> RZ/G2L and RZ/V2L do not have the bus error and ECCRAM interrupts?
>
Hmm not sure how this was missed earlier.

> Looks like most of the conditional handling can be removed (see below).
>
> > Given the commit message, I figure that providing 48 interrupts for
> > (at least some of) those devices would be incorrect?
>
> Looks like all of RZ/G2L{,C}, RZ/V2L, RZ/G2UL, and RZ/Five support
> all 48 interrupts. RZ/G3S lacks the final three for ECCRAM1.
>
Agreed for RZ/G2L{,C}, RZ/V2L, RZ/G2UL, and RZ/Five, but for RZ/G3S it
becomes tricky the interrupts for ECCRAM0/1 are combined hence they
have just 3 interrupts. How do you propose the above interrupt naming?

> [1] "Re: [PATCH v3 8/9] dt-bindings: interrupt-controller:
> renesas,rzg2l-irqc: Document RZ/G3S"
> https://lore.kernel.org/r/CAMuHMdX88KRnvJchUwrWcgmPooAESOT2492Nr1Z_5UMng3q__Q@mail.gmail.com
>
Sorry I missed this thread.

Cheers,
Prabhakar

2024-01-30 16:02:32

by Prabhakar

[permalink] [raw]
Subject: Re: [PATCH 1/5] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document RZ/Five SoC

Hi Geert,

On Tue, Jan 30, 2024 at 1:06 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Tue, Jan 30, 2024 at 1:59 PM Lad, Prabhakar
> <[email protected]> wrote:
> > On Tue, Jan 30, 2024 at 11:13 AM Geert Uytterhoeven
> > <[email protected]> wrote:
> > > On Mon, Jan 29, 2024 at 6:30 PM Conor Dooley <[email protected]> wrote:
> > > > On Mon, Jan 29, 2024 at 03:16:14PM +0000, Prabhakar wrote:
> > > > > From: Lad Prabhakar <[email protected]>
> > > > >
> > > > > Document RZ/Five (R9A07G043F) IRQC bindings. The IRQC block on RZ/Five SoC
> > > > > is almost identical to one found on the RZ/G2L SoC with below differences,
> > > > > * Additional BUS error interrupt
> > > > > * Additional ECCRAM error interrupt
> > > > > * Has additional mask control registers for NMI/IRQ/TINT
> > > > >
> > > > > Hence new compatible string "renesas,r9a07g043f-irqc" is added for RZ/Five
> > > > > SoC.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > >
> > > > > --- a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > > > > @@ -134,6 +141,12 @@ properties:
> > > > > - const: tint30
> > > > > - const: tint31
> > > > > - const: bus-err
> > > > > + - const: eccram0-tie1
> > > > > + - const: eccram0-tie2
> > > > > + - const: eccram0-ovf
> > > > > + - const: eccram1-tie1
> > > > > + - const: eccram1-tie2
> > > > > + - const: eccram1-ovf
> > >
> > > Why not use the naming from the docs (all 6 include "ti")?
> > > EC7TIE1_0, EC7TIE2_0, EC7TIOVF_0, EC7TIE1_1, EC7TIE2_1, EC7TIOVF_1
> > > => ec7tie1-0, ec7tie2-0, ec7tiovf-0, ...?
> > >
> > Agreed.
> >
> > > > I think the restrictions already in the file become incorrect with this
> > > > patch:
> > > > - if:
> > > > properties:
> > > > compatible:
> > > > contains:
> > > > enum:
> > > > - renesas,r9a07g043u-irqc
> > > > - renesas,r9a08g045-irqc
> > > > then:
> > > > properties:
> > > > interrupts:
> > > > minItems: 42
> > > > interrupt-names:
> > > > minItems: 42
> > > > required:
> > > > - interrupt-names
> > > >
> > > > This used to require all 42 interrupts for the two compatibles here
> > > > and at least the first 41 otherwise. Now you've increased the number of
> > > > interrupts to 48 thereby removing the upper limits on the existing
> > > > devices.
> > >
> > > I'm gonna repeat (and extend) my question from [1]: How come we thought
> > > RZ/G2L and RZ/V2L do not have the bus error and ECCRAM interrupts?
> > >
> > Hmm not sure how this was missed earlier.
> >
> > > Looks like most of the conditional handling can be removed (see below).
> > >
> > > > Given the commit message, I figure that providing 48 interrupts for
> > > > (at least some of) those devices would be incorrect?
> > >
> > > Looks like all of RZ/G2L{,C}, RZ/V2L, RZ/G2UL, and RZ/Five support
> > > all 48 interrupts. RZ/G3S lacks the final three for ECCRAM1.
> > >
> > Agreed for RZ/G2L{,C}, RZ/V2L, RZ/G2UL, and RZ/Five, but for RZ/G3S it
> > becomes tricky the interrupts for ECCRAM0/1 are combined hence they
> > have just 3 interrupts. How do you propose the above interrupt naming?
>
> I guess it doesn't hurt to have an index 0 on a part that has only a
> single set?
>
Let's go with this option...

> Alternatives would be to
> 1. Drop the index completely on RZ/G3S, complicating bindings and
> driver,
> 1. Drop the index for the first set, and use index 2 for the second set,
> causing the names to differ even more on parts with 2 sets.
>
..instead of complicating.

Cheers,
Prabhakar

2024-01-31 19:10:23

by Prabhakar

[permalink] [raw]
Subject: Re: [PATCH 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC

Hi Geert,

Thank you for the review.

On Tue, Jan 30, 2024 at 11:38 AM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jan 29, 2024 at 4:16 PM Prabhakar <prabhakar.csengg@gmailcom> wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared
> > to the RZ/G2L (family) SoC.
> >
> > Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC
> > controller driver. Two new registers, IMSK and TMSK, are defined to
> > handle masking on RZ/Five SoC. The implementation utilizes a new data
> > structure, `struct rzg2l_irqc_data`, to determine mask support for a
> > specific controller instance.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
>
> Thanks for your patch!
>
> > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache {
> > u32 titsr[2];
> > };
> >
> > +/**
> > + * struct rzg2l_irqc_data - OF data structure
> > + * @mask_supported: Indicates if mask registers are available
> > + */
> > +struct rzg2l_irqc_data {
>
> This structure has the same name as the single static struct
> rzg2l_irqc_priv instance, which is confusing.
>
Agreed, I will rename it to rzg2l_irqc_of_data

> > + bool mask_supported;
> > +};
> > +
> > /**
> > * struct rzg2l_irqc_priv - IRQ controller private data structure
> > * @base: Controller's base address
> > + * @data: OF data pointer
> > * @fwspec: IRQ firmware specific data
> > * @lock: Lock to serialize access to hardware registers
> > * @cache: Registers cache for suspend/resume
> > */
> > static struct rzg2l_irqc_priv {
> > void __iomem *base;
> > + const struct rzg2l_irqc_data *data;
>
> Replacing this by a bool would avoid a pointer dereference in each user,
> and allows you to make rzg2l_irqc_data etc. __initconst.
>
Do you mean just add "bool mask_supported" here and get rid of struct
rzg2l_irqc_data ? Can you please elaborate here..

> > struct irq_fwspec fwspec[IRQC_NUM_IRQ];
> > raw_spinlock_t lock;
> > struct rzg2l_irqc_reg_cache cache;
>
> > @@ -371,9 +475,23 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
> > return 0;
> > }
> >
> > +static const struct rzg2l_irqc_data rzfive_irqc_data = {
> > + .mask_supported = true,
> > +};
> > +
> > +static const struct rzg2l_irqc_data rzg2l_irqc_default_data = {
> > + .mask_supported = false,
> > +};
> > +
> > +static const struct of_device_id rzg2l_irqc_matches[] = {
> > + { .compatible = "renesas,r9a07g043f-irqc", .data = &rzfive_irqc_data },
> > + { }
> > +};
> > +
> > static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> > {
> > struct irq_domain *irq_domain, *parent_domain;
> > + const struct of_device_id *match;
> > struct platform_device *pdev;
> > struct reset_control *resetn;
> > int ret;
> > @@ -392,6 +510,12 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> > if (!rzg2l_irqc_data)
> > return -ENOMEM;
> >
> > + match = of_match_node(rzg2l_irqc_matches, node);
> > + if (match)
> > + rzg2l_irqc_data->data = match->data;
> > + else
> > + rzg2l_irqc_data->data = &rzg2l_irqc_default_data;
>
> Instead of matching a second time, I'd rather add a second
> IRQCHIP_MATCH() entry with a different init function, passing the
> actual rzg2l_irqc_data pointer.
>
OK, or rather just pass true/false instead of rzg2l_irqc_of_data pointer.?

Cheers,
Prabhakar

2024-02-01 08:43:09

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC

Hi Prabhakar,

On Wed, Jan 31, 2024 at 7:36 PM Lad, Prabhakar
<[email protected]> wrote:
> On Tue, Jan 30, 2024 at 11:38 AM Geert Uytterhoeven
> <[email protected]> wrote:
> > On Mon, Jan 29, 2024 at 4:16 PM Prabhakar <[email protected]> wrote:
> > > From: Lad Prabhakar <[email protected]>
> > >
> > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared
> > > to the RZ/G2L (family) SoC.
> > >
> > > Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC
> > > controller driver. Two new registers, IMSK and TMSK, are defined to
> > > handle masking on RZ/Five SoC. The implementation utilizes a new data
> > > structure, `struct rzg2l_irqc_data`, to determine mask support for a
> > > specific controller instance.
> > >
> > > Signed-off-by: Lad Prabhakar <[email protected]>
> >
> > > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache {
> > > u32 titsr[2];
> > > };
> > >
> > > +/**
> > > + * struct rzg2l_irqc_data - OF data structure
> > > + * @mask_supported: Indicates if mask registers are available
> > > + */
> > > +struct rzg2l_irqc_data {
> >
> > This structure has the same name as the single static struct
> > rzg2l_irqc_priv instance, which is confusing.
> >
> Agreed, I will rename it to rzg2l_irqc_of_data
>
> > > + bool mask_supported;
> > > +};
> > > +
> > > /**
> > > * struct rzg2l_irqc_priv - IRQ controller private data structure
> > > * @base: Controller's base address
> > > + * @data: OF data pointer
> > > * @fwspec: IRQ firmware specific data
> > > * @lock: Lock to serialize access to hardware registers
> > > * @cache: Registers cache for suspend/resume
> > > */
> > > static struct rzg2l_irqc_priv {
> > > void __iomem *base;
> > > + const struct rzg2l_irqc_data *data;
> >
> > Replacing this by a bool would avoid a pointer dereference in each user,
> > and allows you to make rzg2l_irqc_data etc. __initconst.
> >
> Do you mean just add "bool mask_supported" here and get rid of struct
> rzg2l_irqc_data ? Can you please elaborate here..

Either add "bool mask_supported" here, or add a copy of the full
struct rzg2l_irqc_data (see below).

>
> > > struct irq_fwspec fwspec[IRQC_NUM_IRQ];
> > > raw_spinlock_t lock;
> > > struct rzg2l_irqc_reg_cache cache;
> >
> > > @@ -371,9 +475,23 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
> > > return 0;
> > > }
> > >
> > > +static const struct rzg2l_irqc_data rzfive_irqc_data = {
> > > + .mask_supported = true,
> > > +};
> > > +
> > > +static const struct rzg2l_irqc_data rzg2l_irqc_default_data = {
> > > + .mask_supported = false,
> > > +};
> > > +
> > > +static const struct of_device_id rzg2l_irqc_matches[] = {
> > > + { .compatible = "renesas,r9a07g043f-irqc", .data = &rzfive_irqc_data },
> > > + { }
> > > +};
> > > +
> > > static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> > > {
> > > struct irq_domain *irq_domain, *parent_domain;
> > > + const struct of_device_id *match;
> > > struct platform_device *pdev;
> > > struct reset_control *resetn;
> > > int ret;
> > > @@ -392,6 +510,12 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> > > if (!rzg2l_irqc_data)
> > > return -ENOMEM;
> > >
> > > + match = of_match_node(rzg2l_irqc_matches, node);
> > > + if (match)
> > > + rzg2l_irqc_data->data = match->data;
> > > + else
> > > + rzg2l_irqc_data->data = &rzg2l_irqc_default_data;
> >
> > Instead of matching a second time, I'd rather add a second
> > IRQCHIP_MATCH() entry with a different init function, passing the
> > actual rzg2l_irqc_data pointer.
> >
> OK, or rather just pass true/false instead of rzg2l_irqc_of_data pointer.?

Yes, that would be fine for me, too.
It all depends on whether you plan to add, or see a need for adding,
more flags or other fields in the future (and even for flags, you could
combine them in an unsigned long).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-01 13:06:15

by Prabhakar

[permalink] [raw]
Subject: Re: [PATCH 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC

Hi Geert,

On Thu, Feb 1, 2024 at 8:34 AM Geert Uytterhoeven <geert@linux-m68korg> wrote:
>
> Hi Prabhakar,
>
> On Wed, Jan 31, 2024 at 7:36 PM Lad, Prabhakar
> <[email protected]> wrote:
> > On Tue, Jan 30, 2024 at 11:38 AM Geert Uytterhoeven
> > <[email protected]> wrote:
> > > On Mon, Jan 29, 2024 at 4:16 PM Prabhakar <[email protected]> wrote:
> > > > From: Lad Prabhakar <[email protected]>
> > > >
> > > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared
> > > > to the RZ/G2L (family) SoC.
> > > >
> > > > Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC
> > > > controller driver. Two new registers, IMSK and TMSK, are defined to
> > > > handle masking on RZ/Five SoC. The implementation utilizes a new data
> > > > structure, `struct rzg2l_irqc_data`, to determine mask support for a
> > > > specific controller instance.
> > > >
> > > > Signed-off-by: Lad Prabhakar <[email protected]>
> > >
> > > > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache {
> > > > u32 titsr[2];
> > > > };
> > > >
> > > > +/**
> > > > + * struct rzg2l_irqc_data - OF data structure
> > > > + * @mask_supported: Indicates if mask registers are available
> > > > + */
> > > > +struct rzg2l_irqc_data {
> > >
> > > This structure has the same name as the single static struct
> > > rzg2l_irqc_priv instance, which is confusing.
> > >
> > Agreed, I will rename it to rzg2l_irqc_of_data
> >
> > > > + bool mask_supported;
> > > > +};
> > > > +
> > > > /**
> > > > * struct rzg2l_irqc_priv - IRQ controller private data structure
> > > > * @base: Controller's base address
> > > > + * @data: OF data pointer
> > > > * @fwspec: IRQ firmware specific data
> > > > * @lock: Lock to serialize access to hardware registers
> > > > * @cache: Registers cache for suspend/resume
> > > > */
> > > > static struct rzg2l_irqc_priv {
> > > > void __iomem *base;
> > > > + const struct rzg2l_irqc_data *data;
> > >
> > > Replacing this by a bool would avoid a pointer dereference in each user,
> > > and allows you to make rzg2l_irqc_data etc. __initconst.
> > >
> > Do you mean just add "bool mask_supported" here and get rid of struct
> > rzg2l_irqc_data ? Can you please elaborate here..
>
> Either add "bool mask_supported" here, or add a copy of the full
> struct rzg2l_irqc_data (see below).
>
OK, i'll keep a copy of struct rzg2l_irqc_data here...
> >
> > > > struct irq_fwspec fwspec[IRQC_NUM_IRQ];
> > > > raw_spinlock_t lock;
> > > > struct rzg2l_irqc_reg_cache cache;
> > >
> > > > @@ -371,9 +475,23 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
> > > > return 0;
> > > > }
> > > >
> > > > +static const struct rzg2l_irqc_data rzfive_irqc_data = {
> > > > + .mask_supported = true,
> > > > +};
> > > > +
> > > > +static const struct rzg2l_irqc_data rzg2l_irqc_default_data = {
> > > > + .mask_supported = false,
> > > > +};
> > > > +
> > > > +static const struct of_device_id rzg2l_irqc_matches[] = {
> > > > + { .compatible = "renesas,r9a07g043f-irqc", .data = &rzfive_irqc_data },
> > > > + { }
> > > > +};
> > > > +
> > > > static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> > > > {
> > > > struct irq_domain *irq_domain, *parent_domain;
> > > > + const struct of_device_id *match;
> > > > struct platform_device *pdev;
> > > > struct reset_control *resetn;
> > > > int ret;
> > > > @@ -392,6 +510,12 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> > > > if (!rzg2l_irqc_data)
> > > > return -ENOMEM;
> > > >
> > > > + match = of_match_node(rzg2l_irqc_matches, node);
> > > > + if (match)
> > > > + rzg2l_irqc_data->data = match->data;
> > > > + else
> > > > + rzg2l_irqc_data->data = &rzg2l_irqc_default_data;
> > >
> > > Instead of matching a second time, I'd rather add a second
> > > IRQCHIP_MATCH() entry with a different init function, passing the
> > > actual rzg2l_irqc_data pointer.
> > >
> > OK, or rather just pass true/false instead of rzg2l_irqc_of_data pointer.?
>
> Yes, that would be fine for me, too.
> It all depends on whether you plan to add, or see a need for adding,
> more flags or other fields in the future (and even for flags, you could
> combine them in an unsigned long).
>
I'll keep the bool flag for now and update it as and when it grows.
I see future SoCs with a lot of other interrupts being supported by
this block.

Cheers,
Prabhakar

> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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

2024-02-01 13:11:38

by Prabhakar

[permalink] [raw]
Subject: Re: [PATCH 3/5] riscv: dts: renesas: r9a07g043f: Add IRQC node to RZ/Five SoC DTSI

Hi Geert,

Thank you for the review.

On Tue, Jan 30, 2024 at 11:25 AM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Prabhakar,
>
> On Mon, Jan 29, 2024 at 4:16 PM Prabhakar <prabhakar.csengg@gmailcom> wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > Add the IRQC node to RZ/Five (R9A07G043F) SoC DTSI.
>
> Thanks for your patch!
>
> > Signed-off-by: Lad Prabhakar <[email protected]>
>
> > --- a/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
> > +++ b/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
> > @@ -50,6 +50,82 @@ &soc {
> > dma-noncoherent;
> > interrupt-parent = <&plic>;
> >
> > + irqc: interrupt-controller@110a0000 {
> > + compatible = "renesas,r9a07g043f-irqc",
> > + "renesas,rzg2l-irqc";
> > + reg = <0 0x110a0000 0 0x20000>;
> > + #interrupt-cells = <2>;
> > + #address-cells = <0>;
> > + interrupt-controller;
> > + interrupts = <SOC_PERIPHERAL_IRQ(0) IRQ_TYPE_LEVEL_HIGH>,
>
> As this is the RZ/Five-specific .dtsi file, and not the common base
> .dtsi, you could avoid using SOC_PERIPHERAL_IRQ() here.
> I am not sure what is most readable...
>
Ok, ill switch to the usual way here..

>+ <SOC_PERIPHERAL_IRQ(25) IRQ_TYPE_EDGE_RISING>,
In RZ/Five HW manual this is documented as LEVEL_HIGH and RZ/G2UL this
is documented as EDGE. I ve internally asked for clarification with
the HW team. If it's the same for both the SoCs I'll move this node to
common SoC dtsi and just update the compat string in rz/five specific
dtsi.

Cheers,
Prabhakar

> The rest LGTM (pending interrupt names review comments).
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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

2024-02-02 09:58:15

by Prabhakar

[permalink] [raw]
Subject: Re: [PATCH 1/5] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document RZ/Five SoC

Hi Conor,

Thank you for the review.

On Mon, Jan 29, 2024 at 5:30 PM Conor Dooley <[email protected]> wrote:
>
> On Mon, Jan 29, 2024 at 03:16:14PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <[email protected]>
> >
> > Document RZ/Five (R9A07G043F) IRQC bindings. The IRQC block on RZ/Five SoC
> > is almost identical to one found on the RZ/G2L SoC with below differences,
> > * Additional BUS error interrupt
> > * Additional ECCRAM error interrupt
> > * Has additional mask control registers for NMI/IRQ/TINT
> >
> > Hence new compatible string "renesas,r9a07g043f-irqc" is added for RZ/Five
> > SoC.
> >
> > Signed-off-by: Lad Prabhakar <[email protected]>
> > ---
> > .../renesas,rzg2l-irqc.yaml | 27 +++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > index d3b5aec0a3f7..3abc01e48934 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> > @@ -23,6 +23,7 @@ properties:
> > compatible:
> > items:
> > - enum:
> > + - renesas,r9a07g043f-irqc # RZ/Five
> > - renesas,r9a07g043u-irqc # RZ/G2UL
> > - renesas,r9a07g044-irqc # RZ/G2{L,LC}
> > - renesas,r9a07g054-irqc # RZ/V2L
> > @@ -88,6 +89,12 @@ properties:
> > - description: GPIO interrupt, TINT30
> > - description: GPIO interrupt, TINT31
> > - description: Bus error interrupt
> > + - description: ECCRAM0 TIE1 interrupt
> > + - description: ECCRAM0 TIE2 interrupt
> > + - description: ECCRAM0 overflow interrupt
> > + - description: ECCRAM1 TIE1 interrupt
> > + - description: ECCRAM1 TIE2 interrupt
> > + - description: ECCRAM1 overflow interrupt
> >
> > interrupt-names:
> > minItems: 41
> > @@ -134,6 +141,12 @@ properties:
> > - const: tint30
> > - const: tint31
> > - const: bus-err
> > + - const: eccram0-tie1
> > + - const: eccram0-tie2
> > + - const: eccram0-ovf
> > + - const: eccram1-tie1
> > + - const: eccram1-tie2
> > + - const: eccram1-ovf
>
> I think the restrictions already in the file become incorrect with this
> patch:
> - if:
> properties:
> compatible:
> contains:
> enum:
> - renesas,r9a07g043u-irqc
> - renesas,r9a08g045-irqc
> then:
> properties:
> interrupts:
> minItems: 42
> interrupt-names:
> minItems: 42
> required:
> - interrupt-names
>
> This used to require all 42 interrupts for the two compatibles here
> and at least the first 41 otherwise. Now you've increased the number of
> interrupts to 48 thereby removing the upper limits on the existing
> devices.
>
> Given the commit message, I figure that providing 48 interrupts for
> (at least some of) those devices would be incorrect?
>
Agreed, I will fix this in the next version.

Cheers,
Prabhakar