[Patch V1 0/4] memory: tegra: Update mc interrupts

[Patch V1 0/4] memory: tegra: Update mc interrupts

[Ashish Mhetre]

Update MC code to add interrupt handling and error logging support.
All basic interrupts and corresponding error logging supported by T186 and
T194 is added.

Ashish Mhetre (4):
  memory: tegra: Add support for mc interrupts
  memory: tegra: Add interrupt mask
  memory: tegra: add mc-err support for T186
  memory: tegra: add mc-err support for T194

 drivers/memory/tegra/mc.c       |  14 ++++-
 drivers/memory/tegra/mc.h       |  26 ++++++++-
 drivers/memory/tegra/tegra114.c |   1 +
 drivers/memory/tegra/tegra124.c |   2 +
 drivers/memory/tegra/tegra186.c | 118 ++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/tegra194.c | 124 ++++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/tegra20.c  |   6 +-
 drivers/memory/tegra/tegra210.c |   1 +
 drivers/memory/tegra/tegra30.c  |   1 +
 include/soc/tegra/mc.h          |   7 ++-
 10 files changed, 294 insertions(+), 6 deletions(-)

-- 
2.7.4

[Patch V1 1/4] memory: tegra: Add support for mc interrupts

[Ashish Mhetre]

Implement new structure for function related to mc interrupts.
Move handle_irq into this structure.
Add support for clearing interrupts.

Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
---
 drivers/memory/tegra/mc.c       | 14 +++++++++++---
 drivers/memory/tegra/mc.h       |  1 +
 drivers/memory/tegra/tegra114.c |  1 +
 drivers/memory/tegra/tegra124.c |  2 ++
 drivers/memory/tegra/tegra186.c | 14 ++++++++++++++
 drivers/memory/tegra/tegra194.c | 12 ++++++++++++
 drivers/memory/tegra/tegra20.c  |  6 +++++-
 drivers/memory/tegra/tegra210.c |  1 +
 drivers/memory/tegra/tegra30.c  |  1 +
 include/soc/tegra/mc.h          |  7 ++++++-
 10 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index 3c5aae7..3b3f052 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -604,9 +604,12 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+const struct tegra_mc_interrupt_ops tegra30_mc_interrupt_ops = {
+	.handle_irq = tegra30_mc_handle_irq,
+};
+
 const struct tegra_mc_ops tegra30_mc_ops = {
 	.probe = tegra30_mc_probe,
-	.handle_irq = tegra30_mc_handle_irq,
 };
 #endif
 
@@ -765,16 +768,21 @@ static int tegra_mc_probe(struct platform_device *pdev)
 			return err;
 	}
 
-	if (mc->soc->ops && mc->soc->ops->handle_irq) {
+	if (mc->soc->interrupt_ops && mc->soc->interrupt_ops->handle_irq) {
 		mc->irq = platform_get_irq(pdev, 0);
 		if (mc->irq < 0)
 			return mc->irq;
 
 		WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n");
 
+		/* clear any mc-errs that occurred before. */
+		if (mc->soc->interrupt_ops->clear_interrupt)
+			mc->soc->interrupt_ops->clear_interrupt(mc);
+
 		mc_writel(mc, mc->soc->intmask, MC_INTMASK);
 
-		err = devm_request_irq(&pdev->dev, mc->irq, mc->soc->ops->handle_irq, 0,
+		err = devm_request_irq(&pdev->dev, mc->irq,
+				       mc->soc->interrupt_ops->handle_irq, 0,
 				       dev_name(&pdev->dev), mc);
 		if (err < 0) {
 			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n", mc->irq,
diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
index 1e49298..f1fd457 100644
--- a/drivers/memory/tegra/mc.h
+++ b/drivers/memory/tegra/mc.h
@@ -144,6 +144,7 @@ extern const struct tegra_mc_soc tegra194_mc_soc;
     defined(CONFIG_ARCH_TEGRA_210_SOC)
 int tegra30_mc_probe(struct tegra_mc *mc);
 extern const struct tegra_mc_ops tegra30_mc_ops;
+extern const struct tegra_mc_interrupt_ops tegra30_mc_interrupt_ops;
 #endif
 
 #if defined(CONFIG_ARCH_TEGRA_186_SOC) || \
diff --git a/drivers/memory/tegra/tegra114.c b/drivers/memory/tegra/tegra114.c
index 4135057..f7b8dd9 100644
--- a/drivers/memory/tegra/tegra114.c
+++ b/drivers/memory/tegra/tegra114.c
@@ -1114,4 +1114,5 @@ const struct tegra_mc_soc tegra114_mc_soc = {
 	.resets = tegra114_mc_resets,
 	.num_resets = ARRAY_SIZE(tegra114_mc_resets),
 	.ops = &tegra30_mc_ops,
+	.interrupt_ops = &tegra30_mc_interrupt_ops,
 };
diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
index d780a84..8b704c1 100644
--- a/drivers/memory/tegra/tegra124.c
+++ b/drivers/memory/tegra/tegra124.c
@@ -1275,6 +1275,7 @@ const struct tegra_mc_soc tegra124_mc_soc = {
 	.num_resets = ARRAY_SIZE(tegra124_mc_resets),
 	.icc_ops = &tegra124_mc_icc_ops,
 	.ops = &tegra30_mc_ops,
+	.interrupt_ops = &tegra30_mc_interrupt_ops,
 };
 #endif /* CONFIG_ARCH_TEGRA_124_SOC */
 
@@ -1307,5 +1308,6 @@ const struct tegra_mc_soc tegra132_mc_soc = {
 	.num_resets = ARRAY_SIZE(tegra124_mc_resets),
 	.icc_ops = &tegra124_mc_icc_ops,
 	.ops = &tegra30_mc_ops,
+	.interrupt_ops = &tegra30_mc_interrupt_ops,
 };
 #endif /* CONFIG_ARCH_TEGRA_132_SOC */
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index e65eac5..b548b6a 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -12,6 +12,8 @@
 
 #include <soc/tegra/mc.h>
 
+#include "mc.h"
+
 #if defined(CONFIG_ARCH_TEGRA_186_SOC)
 #include <dt-bindings/memory/tegra186-mc.h>
 #endif
@@ -20,6 +22,8 @@
 #define MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED BIT(16)
 #define MC_SID_STREAMID_SECURITY_OVERRIDE BIT(8)
 
+#define MC_INTSTATUS_CLEAR			0x00033340
+
 static void tegra186_mc_program_sid(struct tegra_mc *mc)
 {
 	unsigned int i;
@@ -137,6 +141,15 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
 	return 0;
 }
 
+static void tegra186_mc_clear_interrupt(struct tegra_mc *mc)
+{
+	mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS);
+}
+
+const struct tegra_mc_interrupt_ops tegra186_mc_interrupt_ops = {
+	.clear_interrupt = tegra186_mc_clear_interrupt,
+};
+
 const struct tegra_mc_ops tegra186_mc_ops = {
 	.probe = tegra186_mc_probe,
 	.remove = tegra186_mc_remove,
@@ -874,5 +887,6 @@ const struct tegra_mc_soc tegra186_mc_soc = {
 	.clients = tegra186_mc_clients,
 	.num_address_bits = 40,
 	.ops = &tegra186_mc_ops,
+	.interrupt_ops = &tegra186_mc_interrupt_ops,
 };
 #endif
diff --git a/drivers/memory/tegra/tegra194.c b/drivers/memory/tegra/tegra194.c
index cab998b..19f135f 100644
--- a/drivers/memory/tegra/tegra194.c
+++ b/drivers/memory/tegra/tegra194.c
@@ -9,6 +9,17 @@
 
 #include "mc.h"
 
+#define MC_INTSTATUS_CLEAR			0x00133340
+
+static void tegra194_mc_clear_interrupt(struct tegra_mc *mc)
+{
+	mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS);
+}
+
+const struct tegra_mc_interrupt_ops tegra194_mc_interrupt_ops = {
+	.clear_interrupt = tegra194_mc_clear_interrupt,
+};
+
 static const struct tegra_mc_client tegra194_mc_clients[] = {
 	{
 		.id = TEGRA194_MEMORY_CLIENT_PTCR,
@@ -1348,4 +1359,5 @@ const struct tegra_mc_soc tegra194_mc_soc = {
 	.clients = tegra194_mc_clients,
 	.num_address_bits = 40,
 	.ops = &tegra186_mc_ops,
+	.interrupt_ops = &tegra194_mc_interrupt_ops,
 };
diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c
index fcd7738..3bcb2ca 100644
--- a/drivers/memory/tegra/tegra20.c
+++ b/drivers/memory/tegra/tegra20.c
@@ -786,11 +786,15 @@ static irqreturn_t tegra20_mc_handle_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static const struct tegra_mc_interrupt_ops tegra20_mc_interrupt_ops = {
+	.handle_irq = tegra20_mc_handle_irq,
+};
+
 static const struct tegra_mc_ops tegra20_mc_ops = {
 	.probe = tegra20_mc_probe,
 	.suspend = tegra20_mc_suspend,
 	.resume = tegra20_mc_resume,
-	.handle_irq = tegra20_mc_handle_irq,
+	.interrupt_ops = tegra20_mc_interrupt_ops,
 };
 
 const struct tegra_mc_soc tegra20_mc_soc = {
diff --git a/drivers/memory/tegra/tegra210.c b/drivers/memory/tegra/tegra210.c
index 8ab6498..d7ed163 100644
--- a/drivers/memory/tegra/tegra210.c
+++ b/drivers/memory/tegra/tegra210.c
@@ -1287,4 +1287,5 @@ const struct tegra_mc_soc tegra210_mc_soc = {
 	.resets = tegra210_mc_resets,
 	.num_resets = ARRAY_SIZE(tegra210_mc_resets),
 	.ops = &tegra30_mc_ops,
+	.interrupt_ops = &tegra30_mc_interrupt_ops,
 };
diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
index 8431635..bb5ff68 100644
--- a/drivers/memory/tegra/tegra30.c
+++ b/drivers/memory/tegra/tegra30.c
@@ -1400,4 +1400,5 @@ const struct tegra_mc_soc tegra30_mc_soc = {
 	.num_resets = ARRAY_SIZE(tegra30_mc_resets),
 	.icc_ops = &tegra30_mc_icc_ops,
 	.ops = &tegra30_mc_ops,
+	.interrupt_ops = &tegra30_mc_interrupt_ops,
 };
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 1066b11..debc47b 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -170,6 +170,11 @@ struct tegra_mc_icc_ops {
 						void *data);
 };
 
+struct tegra_mc_interrupt_ops {
+	void (*clear_interrupt)(struct tegra_mc *mc);
+	irqreturn_t (*handle_irq)(int irq, void *data);
+};
+
 struct tegra_mc_ops {
 	/*
 	 * @probe: Callback to set up SoC-specific bits of the memory controller. This is called
@@ -179,7 +184,6 @@ struct tegra_mc_ops {
 	void (*remove)(struct tegra_mc *mc);
 	int (*suspend)(struct tegra_mc *mc);
 	int (*resume)(struct tegra_mc *mc);
-	irqreturn_t (*handle_irq)(int irq, void *data);
 	int (*probe_device)(struct tegra_mc *mc, struct device *dev);
 };
 
@@ -205,6 +209,7 @@ struct tegra_mc_soc {
 
 	const struct tegra_mc_icc_ops *icc_ops;
 	const struct tegra_mc_ops *ops;
+	const struct tegra_mc_interrupt_ops *interrupt_ops;
 };
 
 struct tegra_mc {
-- 
2.7.4

[Patch V1 2/4] memory: tegra: Add interrupt mask

[Ashish Mhetre]

Add interrupt masks for all supported interrupts on tegra MCs.
Update interrupt mask value for T186 and T194 as per supported interrupts.

Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
---
 drivers/memory/tegra/mc.h       | 4 ++++
 drivers/memory/tegra/tegra186.c | 4 ++++
 drivers/memory/tegra/tegra194.c | 4 ++++
 3 files changed, 12 insertions(+)

diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
index f1fd457..2d4f495 100644
--- a/drivers/memory/tegra/mc.h
+++ b/drivers/memory/tegra/mc.h
@@ -44,6 +44,10 @@
 #define MC_TIMING_CONTROL_DBG				0xf8
 #define MC_TIMING_CONTROL				0xfc
 
+#define MC_INT_DECERR_ROUTE_SANITY			BIT(20)
+#define MC_INT_WCAM_ERR					BIT(19)
+#define MC_INT_SCRUB_ECC_WR_ACK				BIT(18)
+#define MC_INT_DECERR_GENERALIZED_CARVEOUT		BIT(17)
 #define MC_INT_DECERR_MTS				BIT(16)
 #define MC_INT_SECERR_SEC				BIT(13)
 #define MC_INT_DECERR_VPR				BIT(12)
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index b548b6a..6766cc4 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -886,6 +886,10 @@ const struct tegra_mc_soc tegra186_mc_soc = {
 	.num_clients = ARRAY_SIZE(tegra186_mc_clients),
 	.clients = tegra186_mc_clients,
 	.num_address_bits = 40,
+	.intmask = MC_INT_WCAM_ERR | MC_INT_SCRUB_ECC_WR_ACK |
+		   MC_INT_DECERR_GENERALIZED_CARVEOUT | MC_INT_DECERR_MTS |
+		   MC_INT_SECERR_SEC | MC_INT_DECERR_VPR |
+		   MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM,
 	.ops = &tegra186_mc_ops,
 	.interrupt_ops = &tegra186_mc_interrupt_ops,
 };
diff --git a/drivers/memory/tegra/tegra194.c b/drivers/memory/tegra/tegra194.c
index 19f135f..76ba3da 100644
--- a/drivers/memory/tegra/tegra194.c
+++ b/drivers/memory/tegra/tegra194.c
@@ -1358,6 +1358,10 @@ const struct tegra_mc_soc tegra194_mc_soc = {
 	.num_clients = ARRAY_SIZE(tegra194_mc_clients),
 	.clients = tegra194_mc_clients,
 	.num_address_bits = 40,
+	.intmask = MC_INT_DECERR_ROUTE_SANITY | MC_INT_WCAM_ERR |
+		   MC_INT_DECERR_GENERALIZED_CARVEOUT | MC_INT_DECERR_MTS |
+		   MC_INT_SECERR_SEC | MC_INT_DECERR_VPR |
+		   MC_INT_SECURITY_VIOLATION | MC_INT_DECERR_EMEM,
 	.ops = &tegra186_mc_ops,
 	.interrupt_ops = &tegra194_mc_interrupt_ops,
 };
-- 
2.7.4

[Patch V1 3/4] memory: tegra: add mc-err support for T186

[Ashish Mhetre]

Add all mc-errors supported by T186.
Implement mc interrupt handling routine for T186.

Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
---
 drivers/memory/tegra/mc.h       |  17 +++++++
 drivers/memory/tegra/tegra186.c | 100 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
index 2d4f495..7817492 100644
--- a/drivers/memory/tegra/mc.h
+++ b/drivers/memory/tegra/mc.h
@@ -44,6 +44,15 @@
 #define MC_TIMING_CONTROL_DBG				0xf8
 #define MC_TIMING_CONTROL				0xfc
 
+#define MC_ERR_VPR_STATUS				0x654
+#define MC_ERR_VPR_ADR					0x658
+#define MC_ERR_SEC_STATUS				0x67c
+#define MC_ERR_SEC_ADR					0x680
+#define MC_ERR_MTS_STATUS				0x9b0
+#define MC_ERR_MTS_ADR					0x9b4
+#define MC_ERR_GENERALIZED_CARVEOUT_STATUS		0xc00
+#define MC_ERR_GENERALIZED_CARVEOUT_ADR			0xc04
+
 #define MC_INT_DECERR_ROUTE_SANITY			BIT(20)
 #define MC_INT_WCAM_ERR					BIT(19)
 #define MC_INT_SCRUB_ECC_WR_ACK				BIT(18)
@@ -159,6 +168,14 @@ extern const struct tegra_mc_ops tegra186_mc_ops;
 extern const char * const tegra_mc_status_names[32];
 extern const char * const tegra_mc_error_names[8];
 
+struct tegra_mc_error {
+	u32 int_bit;
+	const char *msg;
+	u32 status_reg;
+	u32 addr_reg;
+	u32 addr_reg_hi;
+};
+
 /*
  * These IDs are for internal use of Tegra ICC drivers. The ID numbers are
  * chosen such that they don't conflict with the device-tree ICC node IDs.
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index 6766cc4..4f3ae71 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -146,8 +146,107 @@ static void tegra186_mc_clear_interrupt(struct tegra_mc *mc)
 	mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS);
 }
 
+static const struct tegra_mc_error int_mc_errors[] = {
+	{
+		.int_bit = MC_INT_DECERR_EMEM,
+		.msg = "EMEM address decode error",
+		.status_reg = MC_ERR_STATUS,
+		.addr_reg = MC_ERR_ADR,
+	},
+	{
+		.int_bit = MC_INT_SECURITY_VIOLATION,
+		.msg = "non secure access to secure region",
+		.status_reg = MC_ERR_STATUS,
+		.addr_reg = MC_ERR_ADR,
+	},
+	{
+		.int_bit = MC_INT_DECERR_VPR,
+		.msg = "MC request violates VPR requirements",
+		.status_reg = MC_ERR_VPR_STATUS,
+		.addr_reg = MC_ERR_VPR_ADR,
+	},
+	{
+		.int_bit = MC_INT_SECERR_SEC,
+		.msg = "MC request violated SEC carveout requirements",
+		.status_reg = MC_ERR_SEC_STATUS,
+		.addr_reg = MC_ERR_SEC_ADR,
+	},
+	{
+		.int_bit = MC_INT_DECERR_MTS,
+		.msg = "MTS carveout access violation",
+		.status_reg = MC_ERR_MTS_STATUS,
+		.addr_reg = MC_ERR_MTS_ADR,
+	},
+	{
+		.int_bit = MC_INT_DECERR_GENERALIZED_CARVEOUT,
+		.msg = "GSC access violation",
+		.status_reg = MC_ERR_GENERALIZED_CARVEOUT_STATUS,
+		.addr_reg = MC_ERR_GENERALIZED_CARVEOUT_ADR,
+	},
+};
+
+static irqreturn_t tegra186_mc_handle_irq(int irq, void *data)
+{
+	struct tegra_mc *mc = data;
+	unsigned long status;
+	unsigned int bit;
+
+	status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
+	if (!status)
+		return IRQ_NONE;
+
+	for_each_set_bit(bit, &status, 32) {
+		const char *error = int_mc_errors[bit].msg ?: "unknown";
+		const char *client = "unknown";
+		const char *direction, *secure;
+		phys_addr_t addr = 0;
+		unsigned int i;
+		u8 id;
+		u32 value;
+
+		value = mc_readl(mc, int_mc_errors[bit].status_reg);
+
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+		if (mc->soc->num_address_bits > 32) {
+			addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
+				MC_ERR_STATUS_ADR_HI_MASK);
+			addr <<= 32;
+		}
+#endif
+		addr |= mc_readl(mc, int_mc_errors[bit].addr_reg);
+
+		if (value & MC_ERR_STATUS_RW)
+			direction = "write";
+		else
+			direction = "read";
+
+		if (value & MC_ERR_STATUS_SECURITY)
+			secure = "secure ";
+		else
+			secure = "";
+
+		id = value & mc->soc->client_id_mask;
+
+		for (i = 0; i < mc->soc->num_clients; i++) {
+			if (mc->soc->clients[i].id == id) {
+				client = mc->soc->clients[i].name;
+				break;
+			}
+		}
+
+		dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s\n",
+				    client, secure, direction, &addr, error);
+	}
+
+	/* clear interrupts */
+	mc_writel(mc, status, MC_INTSTATUS);
+
+	return IRQ_HANDLED;
+}
+
 const struct tegra_mc_interrupt_ops tegra186_mc_interrupt_ops = {
 	.clear_interrupt = tegra186_mc_clear_interrupt,
+	.handle_irq = tegra186_mc_handle_irq,
 };
 
 const struct tegra_mc_ops tegra186_mc_ops = {
@@ -886,6 +985,7 @@ const struct tegra_mc_soc tegra186_mc_soc = {
 	.num_clients = ARRAY_SIZE(tegra186_mc_clients),
 	.clients = tegra186_mc_clients,
 	.num_address_bits = 40,
+	.client_id_mask = 0xff,
 	.intmask = MC_INT_WCAM_ERR | MC_INT_SCRUB_ECC_WR_ACK |
 		   MC_INT_DECERR_GENERALIZED_CARVEOUT | MC_INT_DECERR_MTS |
 		   MC_INT_SECERR_SEC | MC_INT_DECERR_VPR |
-- 
2.7.4

[Patch V1 4/4] memory: tegra: add mc-err support for T194

[Ashish Mhetre]

Add all basic mc-errors supported by T194.
Implement mc interrupt handling routine for T194.

Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
---
 drivers/memory/tegra/mc.h       |   2 +
 drivers/memory/tegra/tegra194.c | 108 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+)

diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
index 7817492..1d881e7 100644
--- a/drivers/memory/tegra/mc.h
+++ b/drivers/memory/tegra/mc.h
@@ -43,6 +43,7 @@
 #define MC_EMEM_ARB_OVERRIDE				0xe8
 #define MC_TIMING_CONTROL_DBG				0xf8
 #define MC_TIMING_CONTROL				0xfc
+#define MC_ERR_ADR_HI					0x11fc
 
 #define MC_ERR_VPR_STATUS				0x654
 #define MC_ERR_VPR_ADR					0x658
@@ -50,6 +51,7 @@
 #define MC_ERR_SEC_ADR					0x680
 #define MC_ERR_MTS_STATUS				0x9b0
 #define MC_ERR_MTS_ADR					0x9b4
+#define MC_ERR_GENERALIZED_CARVEOUT_STATUS_1		0xbfc
 #define MC_ERR_GENERALIZED_CARVEOUT_STATUS		0xc00
 #define MC_ERR_GENERALIZED_CARVEOUT_ADR			0xc04
 
diff --git a/drivers/memory/tegra/tegra194.c b/drivers/memory/tegra/tegra194.c
index 76ba3da..a0af6a0 100644
--- a/drivers/memory/tegra/tegra194.c
+++ b/drivers/memory/tegra/tegra194.c
@@ -4,6 +4,7 @@
  */
 
 #include <soc/tegra/mc.h>
+#include <linux/platform_device.h>
 
 #include <dt-bindings/memory/tegra194-mc.h>
 
@@ -16,8 +17,114 @@ static void tegra194_mc_clear_interrupt(struct tegra_mc *mc)
 	mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS);
 }
 
+static const struct tegra_mc_error int_mc_errors[] = {
+	{
+		.int_bit = MC_INT_DECERR_EMEM,
+		.msg = "EMEM address decode error",
+		.status_reg = MC_ERR_STATUS,
+		.addr_reg = MC_ERR_ADR,
+		.addr_reg_hi = MC_ERR_ADR_HI,
+	},
+	{
+		.int_bit = MC_INT_SECURITY_VIOLATION,
+		.msg = "non secure access to secure region",
+		.status_reg = MC_ERR_STATUS,
+		.addr_reg = MC_ERR_ADR,
+		.addr_reg_hi = MC_ERR_ADR_HI,
+	},
+	{
+		.int_bit = MC_INT_DECERR_VPR,
+		.msg = "MC request violates VPR requirements",
+		.status_reg = MC_ERR_VPR_STATUS,
+		.addr_reg = MC_ERR_VPR_ADR,
+	},
+	{
+		.int_bit = MC_INT_SECERR_SEC,
+		.msg = "MC request violated SEC carveout requirements",
+		.status_reg = MC_ERR_SEC_STATUS,
+		.addr_reg = MC_ERR_SEC_ADR,
+	},
+	{
+		.int_bit = MC_INT_DECERR_MTS,
+		.msg = "MTS carveout access violation",
+		.status_reg = MC_ERR_MTS_STATUS,
+		.addr_reg = MC_ERR_MTS_ADR,
+	},
+	{
+		.int_bit = MC_INT_DECERR_GENERALIZED_CARVEOUT,
+		.msg = "GSC access violation",
+		.status_reg = MC_ERR_GENERALIZED_CARVEOUT_STATUS,
+		.addr_reg = MC_ERR_GENERALIZED_CARVEOUT_ADR,
+		.addr_reg_hi = MC_ERR_GENERALIZED_CARVEOUT_STATUS_1,
+	},
+};
+
+static irqreturn_t tegra194_mc_handle_irq(int irq, void *data)
+{
+	struct tegra_mc *mc = data;
+	unsigned long status;
+	unsigned int bit;
+
+	status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
+	if (!status)
+		return IRQ_NONE;
+
+	for_each_set_bit(bit, &status, 32) {
+		const char *error = int_mc_errors[bit].msg ?: "unknown";
+		const char *client = "unknown";
+		const char *direction, *secure;
+		phys_addr_t addr = 0;
+		unsigned int i;
+		u8 id;
+		u32 value;
+
+		value = mc_readl(mc, int_mc_errors[bit].status_reg);
+
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+		if (mc->soc->num_address_bits > 32) {
+			if (int_mc_errors[bit].addr_reg_hi)
+				addr = mc_readl(mc,
+						int_mc_errors[bit].addr_reg_hi);
+			else
+				addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
+					MC_ERR_STATUS_ADR_HI_MASK);
+			addr <<= 32;
+		}
+#endif
+		addr |= mc_readl(mc, int_mc_errors[bit].addr_reg);
+
+		if (value & MC_ERR_STATUS_RW)
+			direction = "write";
+		else
+			direction = "read";
+
+		if (value & MC_ERR_STATUS_SECURITY)
+			secure = "secure ";
+		else
+			secure = "";
+
+		id = value & mc->soc->client_id_mask;
+
+		for (i = 0; i < mc->soc->num_clients; i++) {
+			if (mc->soc->clients[i].id == id) {
+				client = mc->soc->clients[i].name;
+				break;
+			}
+		}
+
+		dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s\n",
+				    client, secure, direction, &addr, error);
+	}
+
+	/* clear interrupts */
+	mc_writel(mc, status, MC_INTSTATUS);
+
+	return IRQ_HANDLED;
+}
+
 const struct tegra_mc_interrupt_ops tegra194_mc_interrupt_ops = {
 	.clear_interrupt = tegra194_mc_clear_interrupt,
+	.handle_irq = tegra194_mc_handle_irq,
 };
 
 static const struct tegra_mc_client tegra194_mc_clients[] = {
@@ -1358,6 +1465,7 @@ const struct tegra_mc_soc tegra194_mc_soc = {
 	.num_clients = ARRAY_SIZE(tegra194_mc_clients),
 	.clients = tegra194_mc_clients,
 	.num_address_bits = 40,
+	.client_id_mask = 0xff,
 	.intmask = MC_INT_DECERR_ROUTE_SANITY | MC_INT_WCAM_ERR |
 		   MC_INT_DECERR_GENERALIZED_CARVEOUT | MC_INT_DECERR_MTS |
 		   MC_INT_SECERR_SEC | MC_INT_DECERR_VPR |
-- 
2.7.4

Re: [Patch V1 1/4] memory: tegra: Add support for mc interrupts

[Krzysztof Kozlowski]

On 11/01/2022 19:45, Ashish Mhetre wrote:
> Implement new structure for function related to mc interrupts.

s/mc/MC/

> Move handle_irq into this structure.
> Add support for clearing interrupts.

The subject says you are adding support for MC interrupts, so before
they were not supported at all?

Here you also mention clearing of interrupts - another new feature. One
commit for refactoring (adding new structure) which does not change
functionality, second commit for adding new feature.

Different question - why do you need new structure for just two function
pointers? Why these different IRQ handling functions cannot be in
tegra_mc_ops? To me, it's unnecessary code complexity (plus performance
impact, but it's not that important). If this is really, really needed,
please describe the rationale in the commit message.

> 
> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
> ---
>  drivers/memory/tegra/mc.c       | 14 +++++++++++---
>  drivers/memory/tegra/mc.h       |  1 +
>  drivers/memory/tegra/tegra114.c |  1 +
>  drivers/memory/tegra/tegra124.c |  2 ++
>  drivers/memory/tegra/tegra186.c | 14 ++++++++++++++
>  drivers/memory/tegra/tegra194.c | 12 ++++++++++++
>  drivers/memory/tegra/tegra20.c  |  6 +++++-
>  drivers/memory/tegra/tegra210.c |  1 +
>  drivers/memory/tegra/tegra30.c  |  1 +
>  include/soc/tegra/mc.h          |  7 ++++++-
>  10 files changed, 54 insertions(+), 5 deletions(-)
> 


Best regards,
Krzysztof

Re: [Patch V1 2/4] memory: tegra: Add interrupt mask

[Krzysztof Kozlowski]

On 11/01/2022 19:45, Ashish Mhetre wrote:
> Add interrupt masks for all supported interrupts on tegra MCs.
> Update interrupt mask value for T186 and T194 as per supported interrupts.

The commit subject is too generic. The commit description need to also
describe why you are doing it and what you want achieve.



Best regards,
Krzysztof

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Krzysztof Kozlowski]

Cc in the future also Dmitry, because he was involved in these drivers
quite a lot.

On 11/01/2022 19:45, Ashish Mhetre wrote:
> Add all mc-errors supported by T186.
> Implement mc interrupt handling routine for T186.

Here and in other patches - please use Tegra186 (and similar) to be
consistent with existing upstream naming.

> 
> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
> ---
>  drivers/memory/tegra/mc.h       |  17 +++++++
>  drivers/memory/tegra/tegra186.c | 100 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
> index 2d4f495..7817492 100644
> --- a/drivers/memory/tegra/mc.h
> +++ b/drivers/memory/tegra/mc.h
> @@ -44,6 +44,15 @@
>  #define MC_TIMING_CONTROL_DBG				0xf8
>  #define MC_TIMING_CONTROL				0xfc
>  
> +#define MC_ERR_VPR_STATUS				0x654
> +#define MC_ERR_VPR_ADR					0x658
> +#define MC_ERR_SEC_STATUS				0x67c
> +#define MC_ERR_SEC_ADR					0x680
> +#define MC_ERR_MTS_STATUS				0x9b0
> +#define MC_ERR_MTS_ADR					0x9b4
> +#define MC_ERR_GENERALIZED_CARVEOUT_STATUS		0xc00
> +#define MC_ERR_GENERALIZED_CARVEOUT_ADR			0xc04
> +
>  #define MC_INT_DECERR_ROUTE_SANITY			BIT(20)
>  #define MC_INT_WCAM_ERR					BIT(19)
>  #define MC_INT_SCRUB_ECC_WR_ACK				BIT(18)
> @@ -159,6 +168,14 @@ extern const struct tegra_mc_ops tegra186_mc_ops;
>  extern const char * const tegra_mc_status_names[32];
>  extern const char * const tegra_mc_error_names[8];
>  
> +struct tegra_mc_error {
> +	u32 int_bit;

Where is it used (read)?

> +	const char *msg;
> +	u32 status_reg;
> +	u32 addr_reg;

Please describe all these fields with kerneldoc.

> +	u32 addr_reg_hi;

Looks unused.

> +};
> +
>  /*
>   * These IDs are for internal use of Tegra ICC drivers. The ID numbers are
>   * chosen such that they don't conflict with the device-tree ICC node IDs.
> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> index 6766cc4..4f3ae71 100644
> --- a/drivers/memory/tegra/tegra186.c
> +++ b/drivers/memory/tegra/tegra186.c
> @@ -146,8 +146,107 @@ static void tegra186_mc_clear_interrupt(struct tegra_mc *mc)
>  	mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS);
>  }
>  
> +static const struct tegra_mc_error int_mc_errors[] = {
> +	{
> +		.int_bit = MC_INT_DECERR_EMEM,
> +		.msg = "EMEM address decode error",
> +		.status_reg = MC_ERR_STATUS,
> +		.addr_reg = MC_ERR_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_SECURITY_VIOLATION,
> +		.msg = "non secure access to secure region",
> +		.status_reg = MC_ERR_STATUS,
> +		.addr_reg = MC_ERR_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_DECERR_VPR,
> +		.msg = "MC request violates VPR requirements",
> +		.status_reg = MC_ERR_VPR_STATUS,
> +		.addr_reg = MC_ERR_VPR_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_SECERR_SEC,
> +		.msg = "MC request violated SEC carveout requirements",
> +		.status_reg = MC_ERR_SEC_STATUS,
> +		.addr_reg = MC_ERR_SEC_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_DECERR_MTS,
> +		.msg = "MTS carveout access violation",
> +		.status_reg = MC_ERR_MTS_STATUS,
> +		.addr_reg = MC_ERR_MTS_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_DECERR_GENERALIZED_CARVEOUT,
> +		.msg = "GSC access violation",
> +		.status_reg = MC_ERR_GENERALIZED_CARVEOUT_STATUS,
> +		.addr_reg = MC_ERR_GENERALIZED_CARVEOUT_ADR,
> +	},
> +};
> +
> +static irqreturn_t tegra186_mc_handle_irq(int irq, void *data)
> +{
> +	struct tegra_mc *mc = data;
> +	unsigned long status;
> +	unsigned int bit;
> +
> +	status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	for_each_set_bit(bit, &status, 32) {

Don't you need bitops.h for this?

> +		const char *error = int_mc_errors[bit].msg ?: "unknown";

int_mc_errors has size of 6, but it's index (the bit variable) can be 20
or even 32. Are you sure indices are used properly or maybe
int_mc_errors missed initializer per-index?

> +		const char *client = "unknown";
> +		const char *direction, *secure;
> +		phys_addr_t addr = 0;
> +		unsigned int i;
> +		u8 id;
> +		u32 value;

Keep order in reversed xmas tree plus name it in a meaningful way. You
read here several registers, so which one is value? Probably you meant
status?

> +
> +		value = mc_readl(mc, int_mc_errors[bit].status_reg);
> +
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> +		if (mc->soc->num_address_bits > 32) {
> +			addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
> +				MC_ERR_STATUS_ADR_HI_MASK);
> +			addr <<= 32;
> +		}
> +#endif
> +		addr |= mc_readl(mc, int_mc_errors[bit].addr_reg);
> +
> +		if (value & MC_ERR_STATUS_RW)
> +			direction = "write";
> +		else
> +			direction = "read";
> +
> +		if (value & MC_ERR_STATUS_SECURITY)
> +			secure = "secure ";
> +		else
> +			secure = "";
> +
> +		id = value & mc->soc->client_id_mask;
> +
> +		for (i = 0; i < mc->soc->num_clients; i++) {
> +			if (mc->soc->clients[i].id == id) {
> +				client = mc->soc->clients[i].name;
> +				break;
> +			}
> +		}
> +
> +		dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s\n",
> +				    client, secure, direction, &addr, error);
> +	}
> +
> +	/* clear interrupts */
> +	mc_writel(mc, status, MC_INTSTATUS);> +
> +	return IRQ_HANDLED;

I don't think you are actually handling these errors as stated in commit
message. You only log them. Please adjust the commit subject and message
to mention the actual purpose/action of error handling.

> +}
> +
>  const struct tegra_mc_interrupt_ops tegra186_mc_interrupt_ops = {
>  	.clear_interrupt = tegra186_mc_clear_interrupt,
> +	.handle_irq = tegra186_mc_handle_irq,
>  };
>  
>  const struct tegra_mc_ops tegra186_mc_ops = {
> @@ -886,6 +985,7 @@ const struct tegra_mc_soc tegra186_mc_soc = {
>  	.num_clients = ARRAY_SIZE(tegra186_mc_clients),
>  	.clients = tegra186_mc_clients,
>  	.num_address_bits = 40,
> +	.client_id_mask = 0xff,
>  	.intmask = MC_INT_WCAM_ERR | MC_INT_SCRUB_ECC_WR_ACK |
>  		   MC_INT_DECERR_GENERALIZED_CARVEOUT | MC_INT_DECERR_MTS |
>  		   MC_INT_SECERR_SEC | MC_INT_DECERR_VPR |
> 



Best regards,
Krzysztof

Re: [Patch V1 1/4] memory: tegra: Add support for mc interrupts

[Dmitry Osipenko]

11.01.2022 21:45, Ashish Mhetre пишет:
>  
> @@ -765,16 +768,21 @@ static int tegra_mc_probe(struct platform_device *pdev)
>  			return err;
>  	}
>  
> -	if (mc->soc->ops && mc->soc->ops->handle_irq) {
> +	if (mc->soc->interrupt_ops && mc->soc->interrupt_ops->handle_irq) {
>  		mc->irq = platform_get_irq(pdev, 0);
>  		if (mc->irq < 0)
>  			return mc->irq;
>  
>  		WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n");
>  
> +		/* clear any mc-errs that occurred before. */

s/mc-errs/Memory Controller errors/

> +		if (mc->soc->interrupt_ops->clear_interrupt)
> +			mc->soc->interrupt_ops->clear_interrupt(mc);

There is no explanation of this change neither in the code, nor in the
commit message. Please always provide detailed descriptions for a
non-trivial changes.

Interrupts aren't cleared intentionally by the driver, otherwise you'll
never know about early-boot memory faults which happened before the
probe. Hence this change is incorrect.

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Dmitry Osipenko]

11.01.2022 21:45, Ashish Mhetre пишет:
> Add all mc-errors supported by T186.
> Implement mc interrupt handling routine for T186.
> 
> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
> ---
>  drivers/memory/tegra/mc.h       |  17 +++++++
>  drivers/memory/tegra/tegra186.c | 100 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
> index 2d4f495..7817492 100644
> --- a/drivers/memory/tegra/mc.h
> +++ b/drivers/memory/tegra/mc.h
> @@ -44,6 +44,15 @@
>  #define MC_TIMING_CONTROL_DBG				0xf8
>  #define MC_TIMING_CONTROL				0xfc
>  

this empty line is unnecessary

> +#define MC_ERR_VPR_STATUS				0x654
> +#define MC_ERR_VPR_ADR					0x658
> +#define MC_ERR_SEC_STATUS				0x67c
> +#define MC_ERR_SEC_ADR					0x680
> +#define MC_ERR_MTS_STATUS				0x9b0
> +#define MC_ERR_MTS_ADR					0x9b4
> +#define MC_ERR_GENERALIZED_CARVEOUT_STATUS		0xc00
> +#define MC_ERR_GENERALIZED_CARVEOUT_ADR			0xc04
> +
>  #define MC_INT_DECERR_ROUTE_SANITY			BIT(20)
>  #define MC_INT_WCAM_ERR					BIT(19)
>  #define MC_INT_SCRUB_ECC_WR_ACK				BIT(18)
> @@ -159,6 +168,14 @@ extern const struct tegra_mc_ops tegra186_mc_ops;
>  extern const char * const tegra_mc_status_names[32];
>  extern const char * const tegra_mc_error_names[8];
>  
> +struct tegra_mc_error {
> +	u32 int_bit;
> +	const char *msg;
> +	u32 status_reg;
> +	u32 addr_reg;
> +	u32 addr_reg_hi;
> +};
> +
>  /*
>   * These IDs are for internal use of Tegra ICC drivers. The ID numbers are
>   * chosen such that they don't conflict with the device-tree ICC node IDs.
> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> index 6766cc4..4f3ae71 100644
> --- a/drivers/memory/tegra/tegra186.c
> +++ b/drivers/memory/tegra/tegra186.c
> @@ -146,8 +146,107 @@ static void tegra186_mc_clear_interrupt(struct tegra_mc *mc)
>  	mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS);
>  }
>  
> +static const struct tegra_mc_error int_mc_errors[] = {
> +	{
> +		.int_bit = MC_INT_DECERR_EMEM,
> +		.msg = "EMEM address decode error",
> +		.status_reg = MC_ERR_STATUS,
> +		.addr_reg = MC_ERR_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_SECURITY_VIOLATION,
> +		.msg = "non secure access to secure region",
> +		.status_reg = MC_ERR_STATUS,
> +		.addr_reg = MC_ERR_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_DECERR_VPR,
> +		.msg = "MC request violates VPR requirements",
> +		.status_reg = MC_ERR_VPR_STATUS,
> +		.addr_reg = MC_ERR_VPR_ADR,
> +	},

I see that these VPR registers present on all SoCs starting with T124.
It doesn't look like you need the separate IRQ handlers at all, instead
please extend the common T30 handler. For example, you may add a
switch-case statements to handle those T124+ specific bits differently.

static irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
{
...
	switch (bit) {
	case MC_INT_DECERR_VPR:
		status_reg = MC_ERR_VPR_STATUS;
		addr_reg   = MC_ERR_VPR_ADR;
		break;
	...
	default:
		status_reg = MC_ERR_STATUS;
		addr_reg   = MC_ERR_ADR;
	}

	value = mc_readl(mc, status_reg);
	...
	
	value = mc_readl(mc, addr_reg);

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Dmitry Osipenko]

11.01.2022 21:45, Ashish Mhetre пишет:
> +static const struct tegra_mc_error int_mc_errors[] = {
> +	{
> +		.int_bit = MC_INT_DECERR_EMEM,
> +		.msg = "EMEM address decode error",
> +		.status_reg = MC_ERR_STATUS,
> +		.addr_reg = MC_ERR_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_SECURITY_VIOLATION,
> +		.msg = "non secure access to secure region",
> +		.status_reg = MC_ERR_STATUS,
> +		.addr_reg = MC_ERR_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_DECERR_VPR,
> +		.msg = "MC request violates VPR requirements",
> +		.status_reg = MC_ERR_VPR_STATUS,
> +		.addr_reg = MC_ERR_VPR_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_SECERR_SEC,
> +		.msg = "MC request violated SEC carveout requirements",
> +		.status_reg = MC_ERR_SEC_STATUS,
> +		.addr_reg = MC_ERR_SEC_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_DECERR_MTS,
> +		.msg = "MTS carveout access violation",
> +		.status_reg = MC_ERR_MTS_STATUS,
> +		.addr_reg = MC_ERR_MTS_ADR,
> +	},
> +	{
> +		.int_bit = MC_INT_DECERR_GENERALIZED_CARVEOUT,
> +		.msg = "GSC access violation",
> +		.status_reg = MC_ERR_GENERALIZED_CARVEOUT_STATUS,
> +		.addr_reg = MC_ERR_GENERALIZED_CARVEOUT_ADR,
> +	},
> +};
> +
> +static irqreturn_t tegra186_mc_handle_irq(int irq, void *data)
> +{
> +	struct tegra_mc *mc = data;
> +	unsigned long status;
> +	unsigned int bit;
> +
> +	status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	for_each_set_bit(bit, &status, 32) {
> +		const char *error = int_mc_errors[bit].msg ?: "unknown";

int_mc_errors[bit] isn't what you need and .int_bit is unused, which
suggests that all this code doesn't work and was untested. Please don't
send untested patches.

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Dmitry Osipenko]

11.01.2022 21:45, Ashish Mhetre пишет:
>  #define MC_INT_DECERR_ROUTE_SANITY			BIT(20)
>  #define MC_INT_WCAM_ERR					BIT(19)
>  #define MC_INT_SCRUB_ECC_WR_ACK				BIT(18)

I don't see where these errors are handled in the code. Is documentation
that explains these bits publicly available?

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Dmitry Osipenko]

12.01.2022 14:22, Dmitry Osipenko пишет:
> 11.01.2022 21:45, Ashish Mhetre пишет:
>>  #define MC_INT_DECERR_ROUTE_SANITY			BIT(20)
>>  #define MC_INT_WCAM_ERR					BIT(19)
>>  #define MC_INT_SCRUB_ECC_WR_ACK				BIT(18)
> 
> I don't see where these errors are handled in the code. Is documentation
> that explains these bits publicly available?
> 

MC_INT_SCRUB_ECC_WR_ACK shouldn't be a error.

Re: [Patch V1 1/4] memory: tegra: Add support for mc interrupts

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]

Hi Ashish,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on v5.16 next-20220112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ashish-Mhetre/memory-tegra-Update-mc-interrupts/20220112-024847
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: arm-defconfig (https://download.01.org/0day-ci/archive/20220113/202201130230.8UdZpJ3c-lkp(a)intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/71553a1a735ae07293d43df685262f85e6453170
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ashish-Mhetre/memory-tegra-Update-mc-interrupts/20220112-024847
        git checkout 71553a1a735ae07293d43df685262f85e6453170
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/memory/tegra/tegra20.c:797:10: error: 'const struct tegra_mc_ops' has no member named 'interrupt_ops'
     797 |         .interrupt_ops = tegra20_mc_interrupt_ops,
         |          ^~~~~~~~~~~~~
>> drivers/memory/tegra/tegra20.c:797:26: error: incompatible types when initializing type 'int (*)(struct tegra_mc *, struct device *)' using type 'struct tegra_mc_interrupt_ops'
     797 |         .interrupt_ops = tegra20_mc_interrupt_ops,
         |                          ^~~~~~~~~~~~~~~~~~~~~~~~


vim +797 drivers/memory/tegra/tegra20.c

   792	
   793	static const struct tegra_mc_ops tegra20_mc_ops = {
   794		.probe = tegra20_mc_probe,
   795		.suspend = tegra20_mc_suspend,
   796		.resume = tegra20_mc_resume,
 > 797		.interrupt_ops = tegra20_mc_interrupt_ops,
   798	};
   799	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [Patch V1 1/4] memory: tegra: Add support for mc interrupts

[Ashish Mhetre]

On 1/12/2022 1:59 AM, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 11/01/2022 19:45, Ashish Mhetre wrote:
>> Implement new structure for function related to mc interrupts.
> 
> s/mc/MC/
> 
Okay, I'll update these.
>> Move handle_irq into this structure.
>> Add support for clearing interrupts.
> 
> The subject says you are adding support for MC interrupts, so before
> they were not supported at all?
> 
Interrupt handling and error logging is not supported from Tegra186
onward. So the patches are for adding supported interrupts and logging
them from Tegra186 onward. But you are right, subject/commit message is
misleading. I'll update it in next version.

> Here you also mention clearing of interrupts - another new feature. One
> commit for refactoring (adding new structure) which does not change
> functionality, second commit for adding new feature.
> > Different question - why do you need new structure for just two function
> pointers? Why these different IRQ handling functions cannot be in
> tegra_mc_ops? To me, it's unnecessary code complexity (plus performance
> impact, but it's not that important). If this is really, really needed,
> please describe the rationale in the commit message.
>
clearing_interrupts() won't be needed. As pointed by Dmitry, we should
be logging early boot MC interrupts as well instead of clearing them.
Also, I'll keep handling irq inside tegra_mc_ops instead of adding new
structure.

>>
>> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
>> ---
>>   drivers/memory/tegra/mc.c       | 14 +++++++++++---
>>   drivers/memory/tegra/mc.h       |  1 +
>>   drivers/memory/tegra/tegra114.c |  1 +
>>   drivers/memory/tegra/tegra124.c |  2 ++
>>   drivers/memory/tegra/tegra186.c | 14 ++++++++++++++
>>   drivers/memory/tegra/tegra194.c | 12 ++++++++++++
>>   drivers/memory/tegra/tegra20.c  |  6 +++++-
>>   drivers/memory/tegra/tegra210.c |  1 +
>>   drivers/memory/tegra/tegra30.c  |  1 +
>>   include/soc/tegra/mc.h          |  7 ++++++-
>>   10 files changed, 54 insertions(+), 5 deletions(-)
>>
> 
> 
> Best regards,
> Krzysztof

Re: [Patch V1 2/4] memory: tegra: Add interrupt mask

[Ashish Mhetre]

On 1/12/2022 2:02 AM, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 11/01/2022 19:45, Ashish Mhetre wrote:
>> Add interrupt masks for all supported interrupts on tegra MCs.
>> Update interrupt mask value for T186 and T194 as per supported interrupts.
> 
> The commit subject is too generic. The commit description need to also
> describe why you are doing it and what you want achieve.
> 
> 
> 
> Best regards,
> Krzysztof
Sure, I'll update the commit message.

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Ashish Mhetre]

On 1/12/2022 2:26 AM, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> Cc in the future also Dmitry, because he was involved in these drivers
> quite a lot.
> 
I'll make sure of that in future patches.

> On 11/01/2022 19:45, Ashish Mhetre wrote:
>> Add all mc-errors supported by T186.
>> Implement mc interrupt handling routine for T186.
> 
> Here and in other patches - please use Tegra186 (and similar) to be
> consistent with existing upstream naming.
> 
Okay, I'll update the naming.

>>
>> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
>> ---
>>   drivers/memory/tegra/mc.h       |  17 +++++++
>>   drivers/memory/tegra/tegra186.c | 100 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 117 insertions(+)
>>
>> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
>> index 2d4f495..7817492 100644
>> --- a/drivers/memory/tegra/mc.h
>> +++ b/drivers/memory/tegra/mc.h
>> @@ -44,6 +44,15 @@
>>   #define MC_TIMING_CONTROL_DBG                                0xf8
>>   #define MC_TIMING_CONTROL                            0xfc
>>
>> +#define MC_ERR_VPR_STATUS                            0x654
>> +#define MC_ERR_VPR_ADR                                       0x658
>> +#define MC_ERR_SEC_STATUS                            0x67c
>> +#define MC_ERR_SEC_ADR                                       0x680
>> +#define MC_ERR_MTS_STATUS                            0x9b0
>> +#define MC_ERR_MTS_ADR                                       0x9b4
>> +#define MC_ERR_GENERALIZED_CARVEOUT_STATUS           0xc00
>> +#define MC_ERR_GENERALIZED_CARVEOUT_ADR                      0xc04
>> +
>>   #define MC_INT_DECERR_ROUTE_SANITY                   BIT(20)
>>   #define MC_INT_WCAM_ERR                                      BIT(19)
>>   #define MC_INT_SCRUB_ECC_WR_ACK                              BIT(18)
>> @@ -159,6 +168,14 @@ extern const struct tegra_mc_ops tegra186_mc_ops;
>>   extern const char * const tegra_mc_status_names[32];
>>   extern const char * const tegra_mc_error_names[8];
>>
>> +struct tegra_mc_error {
>> +     u32 int_bit;
> 
> Where is it used (read)?
> 
Oops, int_bit should be getting used in handle_irq for checking type of
interrupt. I'll update it in next version.

>> +     const char *msg;
>> +     u32 status_reg;
>> +     u32 addr_reg;
> 
> Please describe all these fields with kerneldoc.
> 
>> +     u32 addr_reg_hi;
> 
> Looks unused.
> 
It's getting used in patch 4. I'll update it in next version.

>> +};
>> +
>>   /*
>>    * These IDs are for internal use of Tegra ICC drivers. The ID numbers are
>>    * chosen such that they don't conflict with the device-tree ICC node IDs.
>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>> index 6766cc4..4f3ae71 100644
>> --- a/drivers/memory/tegra/tegra186.c
>> +++ b/drivers/memory/tegra/tegra186.c
>> @@ -146,8 +146,107 @@ static void tegra186_mc_clear_interrupt(struct tegra_mc *mc)
>>        mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS);
>>   }
>>
>> +static const struct tegra_mc_error int_mc_errors[] = {
>> +     {
>> +             .int_bit = MC_INT_DECERR_EMEM,
>> +             .msg = "EMEM address decode error",
>> +             .status_reg = MC_ERR_STATUS,
>> +             .addr_reg = MC_ERR_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_SECURITY_VIOLATION,
>> +             .msg = "non secure access to secure region",
>> +             .status_reg = MC_ERR_STATUS,
>> +             .addr_reg = MC_ERR_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_DECERR_VPR,
>> +             .msg = "MC request violates VPR requirements",
>> +             .status_reg = MC_ERR_VPR_STATUS,
>> +             .addr_reg = MC_ERR_VPR_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_SECERR_SEC,
>> +             .msg = "MC request violated SEC carveout requirements",
>> +             .status_reg = MC_ERR_SEC_STATUS,
>> +             .addr_reg = MC_ERR_SEC_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_DECERR_MTS,
>> +             .msg = "MTS carveout access violation",
>> +             .status_reg = MC_ERR_MTS_STATUS,
>> +             .addr_reg = MC_ERR_MTS_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_DECERR_GENERALIZED_CARVEOUT,
>> +             .msg = "GSC access violation",
>> +             .status_reg = MC_ERR_GENERALIZED_CARVEOUT_STATUS,
>> +             .addr_reg = MC_ERR_GENERALIZED_CARVEOUT_ADR,
>> +     },
>> +};
>> +
>> +static irqreturn_t tegra186_mc_handle_irq(int irq, void *data)
>> +{
>> +     struct tegra_mc *mc = data;
>> +     unsigned long status;
>> +     unsigned int bit;
>> +
>> +     status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
>> +     if (!status)
>> +             return IRQ_NONE;
>> +
>> +     for_each_set_bit(bit, &status, 32) {
> 
> Don't you need bitops.h for this?
> 
It didn't throw any errors when I built it. bitops.h is mostly getting
included from one of the existing includes.
Probably of_device.h -> of.h -> bitops.h

>> +             const char *error = int_mc_errors[bit].msg ?: "unknown";
> 
> int_mc_errors has size of 6, but it's index (the bit variable) can be 20
> or even 32. Are you sure indices are used properly or maybe
> int_mc_errors missed initializer per-index?
> 
That's true, I have not implemented it correctly. I should have looped
over int_mc_errors[] and should have checked .int_bit with bit and used
index of that element.

>> +             const char *client = "unknown";
>> +             const char *direction, *secure;
>> +             phys_addr_t addr = 0;
>> +             unsigned int i;
>> +             u8 id;
>> +             u32 value;
> 
> Keep order in reversed xmas tree plus name it in a meaningful way. You
> read here several registers, so which one is value? Probably you meant
> status?
> 
Yes, I meant status. But tegra30_mc_handle_irq() is implemented in
similar way. So I went with same names. I'll update the names and order.

>> +
>> +             value = mc_readl(mc, int_mc_errors[bit].status_reg);
>> +
>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> +             if (mc->soc->num_address_bits > 32) {
>> +                     addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
>> +                             MC_ERR_STATUS_ADR_HI_MASK);
>> +                     addr <<= 32;
>> +             }
>> +#endif
>> +             addr |= mc_readl(mc, int_mc_errors[bit].addr_reg);
>> +
>> +             if (value & MC_ERR_STATUS_RW)
>> +                     direction = "write";
>> +             else
>> +                     direction = "read";
>> +
>> +             if (value & MC_ERR_STATUS_SECURITY)
>> +                     secure = "secure ";
>> +             else
>> +                     secure = "";
>> +
>> +             id = value & mc->soc->client_id_mask;
>> +
>> +             for (i = 0; i < mc->soc->num_clients; i++) {
>> +                     if (mc->soc->clients[i].id == id) {
>> +                             client = mc->soc->clients[i].name;
>> +                             break;
>> +                     }
>> +             }
>> +
>> +             dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s\n",
>> +                                 client, secure, direction, &addr, error);
>> +     }
>> +
>> +     /* clear interrupts */
>> +     mc_writel(mc, status, MC_INTSTATUS);> +
>> +     return IRQ_HANDLED;
> 
> I don't think you are actually handling these errors as stated in commit
> message. You only log them. Please adjust the commit subject and message
> to mention the actual purpose/action of error handling.
> 
Yes, I'll update the commit message.

>> +}
>> +
>>   const struct tegra_mc_interrupt_ops tegra186_mc_interrupt_ops = {
>>        .clear_interrupt = tegra186_mc_clear_interrupt,
>> +     .handle_irq = tegra186_mc_handle_irq,
>>   };
>>
>>   const struct tegra_mc_ops tegra186_mc_ops = {
>> @@ -886,6 +985,7 @@ const struct tegra_mc_soc tegra186_mc_soc = {
>>        .num_clients = ARRAY_SIZE(tegra186_mc_clients),
>>        .clients = tegra186_mc_clients,
>>        .num_address_bits = 40,
>> +     .client_id_mask = 0xff,
>>        .intmask = MC_INT_WCAM_ERR | MC_INT_SCRUB_ECC_WR_ACK |
>>                   MC_INT_DECERR_GENERALIZED_CARVEOUT | MC_INT_DECERR_MTS |
>>                   MC_INT_SECERR_SEC | MC_INT_DECERR_VPR |
>>
> 
> 
> 
> Best regards,
> Krzysztof

Re: [Patch V1 1/4] memory: tegra: Add support for mc interrupts

[Ashish Mhetre]

On 1/12/2022 1:43 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 11.01.2022 21:45, Ashish Mhetre пишет:
>>
>> @@ -765,16 +768,21 @@ static int tegra_mc_probe(struct platform_device *pdev)
>>                        return err;
>>        }
>>
>> -     if (mc->soc->ops && mc->soc->ops->handle_irq) {
>> +     if (mc->soc->interrupt_ops && mc->soc->interrupt_ops->handle_irq) {
>>                mc->irq = platform_get_irq(pdev, 0);
>>                if (mc->irq < 0)
>>                        return mc->irq;
>>
>>                WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n");
>>
>> +             /* clear any mc-errs that occurred before. */
> 
> s/mc-errs/Memory Controller errors/
> 
Sure, I'll update these in next version.
>> +             if (mc->soc->interrupt_ops->clear_interrupt)
>> +                     mc->soc->interrupt_ops->clear_interrupt(mc);
> 
> There is no explanation of this change neither in the code, nor in the
> commit message. Please always provide detailed descriptions for a
> non-trivial changes.
> 
> Interrupts aren't cleared intentionally by the driver, otherwise you'll
> never know about early-boot memory faults which happened before the
> probe. Hence this change is incorrect.
That's true, we should be logging early-boot memory faults as well.
Ideally there shouldn't be any early-boot faults as all clients will
be up after MC, right? But I agree that we should be checking and
logging if any interrupt is present.

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Ashish Mhetre]

On 1/12/2022 4:31 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 11.01.2022 21:45, Ashish Mhetre пишет:
>> Add all mc-errors supported by T186.
>> Implement mc interrupt handling routine for T186.
>>
>> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
>> ---
>>   drivers/memory/tegra/mc.h       |  17 +++++++
>>   drivers/memory/tegra/tegra186.c | 100 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 117 insertions(+)
>>
>> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
>> index 2d4f495..7817492 100644
>> --- a/drivers/memory/tegra/mc.h
>> +++ b/drivers/memory/tegra/mc.h
>> @@ -44,6 +44,15 @@
>>   #define MC_TIMING_CONTROL_DBG                                0xf8
>>   #define MC_TIMING_CONTROL                            0xfc
>>
> 
> this empty line is unnecessary
> 
I'll fix this in next version.

>> +#define MC_ERR_VPR_STATUS                            0x654
>> +#define MC_ERR_VPR_ADR                                       0x658
>> +#define MC_ERR_SEC_STATUS                            0x67c
>> +#define MC_ERR_SEC_ADR                                       0x680
>> +#define MC_ERR_MTS_STATUS                            0x9b0
>> +#define MC_ERR_MTS_ADR                                       0x9b4
>> +#define MC_ERR_GENERALIZED_CARVEOUT_STATUS           0xc00
>> +#define MC_ERR_GENERALIZED_CARVEOUT_ADR                      0xc04
>> +
>>   #define MC_INT_DECERR_ROUTE_SANITY                   BIT(20)
>>   #define MC_INT_WCAM_ERR                                      BIT(19)
>>   #define MC_INT_SCRUB_ECC_WR_ACK                              BIT(18)
>> @@ -159,6 +168,14 @@ extern const struct tegra_mc_ops tegra186_mc_ops;
>>   extern const char * const tegra_mc_status_names[32];
>>   extern const char * const tegra_mc_error_names[8];
>>
>> +struct tegra_mc_error {
>> +     u32 int_bit;
>> +     const char *msg;
>> +     u32 status_reg;
>> +     u32 addr_reg;
>> +     u32 addr_reg_hi;
>> +};
>> +
>>   /*
>>    * These IDs are for internal use of Tegra ICC drivers. The ID numbers are
>>    * chosen such that they don't conflict with the device-tree ICC node IDs.
>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>> index 6766cc4..4f3ae71 100644
>> --- a/drivers/memory/tegra/tegra186.c
>> +++ b/drivers/memory/tegra/tegra186.c
>> @@ -146,8 +146,107 @@ static void tegra186_mc_clear_interrupt(struct tegra_mc *mc)
>>        mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS);
>>   }
>>
>> +static const struct tegra_mc_error int_mc_errors[] = {
>> +     {
>> +             .int_bit = MC_INT_DECERR_EMEM,
>> +             .msg = "EMEM address decode error",
>> +             .status_reg = MC_ERR_STATUS,
>> +             .addr_reg = MC_ERR_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_SECURITY_VIOLATION,
>> +             .msg = "non secure access to secure region",
>> +             .status_reg = MC_ERR_STATUS,
>> +             .addr_reg = MC_ERR_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_DECERR_VPR,
>> +             .msg = "MC request violates VPR requirements",
>> +             .status_reg = MC_ERR_VPR_STATUS,
>> +             .addr_reg = MC_ERR_VPR_ADR,
>> +     },
> 
> I see that these VPR registers present on all SoCs starting with T124.
> It doesn't look like you need the separate IRQ handlers at all, instead
> please extend the common T30 handler. For example, you may add a
> switch-case statements to handle those T124+ specific bits differently.
> 
> static irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
> {
> ...
>          switch (bit) {
>          case MC_INT_DECERR_VPR:
>                  status_reg = MC_ERR_VPR_STATUS;
>                  addr_reg   = MC_ERR_VPR_ADR;
>                  break;
>          ...
>          default:
>                  status_reg = MC_ERR_STATUS;
>                  addr_reg   = MC_ERR_ADR;
>          }
> 
>          value = mc_readl(mc, status_reg);
>          ...
> 
>          value = mc_readl(mc, addr_reg);
Okay. I'll use same handler as Tegra30 with additional Tegra186 onward
bits.
Also, shall I change name of tegra30_mc_handle_irq() to
tegra_mc_handle_irq() as we are using it across all Tegra SOCs ?

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Ashish Mhetre]

On 1/12/2022 4:32 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 11.01.2022 21:45, Ashish Mhetre пишет:
>> +static const struct tegra_mc_error int_mc_errors[] = {
>> +     {
>> +             .int_bit = MC_INT_DECERR_EMEM,
>> +             .msg = "EMEM address decode error",
>> +             .status_reg = MC_ERR_STATUS,
>> +             .addr_reg = MC_ERR_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_SECURITY_VIOLATION,
>> +             .msg = "non secure access to secure region",
>> +             .status_reg = MC_ERR_STATUS,
>> +             .addr_reg = MC_ERR_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_DECERR_VPR,
>> +             .msg = "MC request violates VPR requirements",
>> +             .status_reg = MC_ERR_VPR_STATUS,
>> +             .addr_reg = MC_ERR_VPR_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_SECERR_SEC,
>> +             .msg = "MC request violated SEC carveout requirements",
>> +             .status_reg = MC_ERR_SEC_STATUS,
>> +             .addr_reg = MC_ERR_SEC_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_DECERR_MTS,
>> +             .msg = "MTS carveout access violation",
>> +             .status_reg = MC_ERR_MTS_STATUS,
>> +             .addr_reg = MC_ERR_MTS_ADR,
>> +     },
>> +     {
>> +             .int_bit = MC_INT_DECERR_GENERALIZED_CARVEOUT,
>> +             .msg = "GSC access violation",
>> +             .status_reg = MC_ERR_GENERALIZED_CARVEOUT_STATUS,
>> +             .addr_reg = MC_ERR_GENERALIZED_CARVEOUT_ADR,
>> +     },
>> +};
>> +
>> +static irqreturn_t tegra186_mc_handle_irq(int irq, void *data)
>> +{
>> +     struct tegra_mc *mc = data;
>> +     unsigned long status;
>> +     unsigned int bit;
>> +
>> +     status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
>> +     if (!status)
>> +             return IRQ_NONE;
>> +
>> +     for_each_set_bit(bit, &status, 32) {
>> +             const char *error = int_mc_errors[bit].msg ?: "unknown";
> 
> int_mc_errors[bit] isn't what you need and .int_bit is unused, which
> suggests that all this code doesn't work and was untested. Please don't
> send untested patches.
Yes, my bad. I will update this in next patch.
Actually I made sure that the patches build without errors and also made
sure that they does not break anything. As for reproducing each of these
memory controller errors, I agree that I haven't done that.

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Ashish Mhetre]

On 1/12/2022 4:54 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 12.01.2022 14:22, Dmitry Osipenko пишет:
>> 11.01.2022 21:45, Ashish Mhetre пишет:
>>>   #define MC_INT_DECERR_ROUTE_SANITY                  BIT(20)
>>>   #define MC_INT_WCAM_ERR                                     BIT(19)
>>>   #define MC_INT_SCRUB_ECC_WR_ACK                             BIT(18)
>>
>> I don't see where these errors are handled in the code. Is documentation
>> that explains these bits publicly available?
>>
MC_INT_DECERR_ROUTE_SANITY is supposed to be part a of Tegra194
interrupts. I have missed adding it and will update in next version.
MC_INT_WCAM_ERR is a MC channel error and is not applicable in upstream.
I'll remove it.
These bits are defined in same documentation where other errors are
defined.

> 
> MC_INT_SCRUB_ECC_WR_ACK shouldn't be a error.
Okay, I 'll remove it.

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Dmitry Osipenko]

19.01.2022 11:53, Ashish Mhetre пишет:
> 
> 
> On 1/12/2022 4:31 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 11.01.2022 21:45, Ashish Mhetre пишет:
>>> Add all mc-errors supported by T186.
>>> Implement mc interrupt handling routine for T186.
>>>
>>> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
>>> ---
>>>   drivers/memory/tegra/mc.h       |  17 +++++++
>>>   drivers/memory/tegra/tegra186.c | 100
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 117 insertions(+)
>>>
>>> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
>>> index 2d4f495..7817492 100644
>>> --- a/drivers/memory/tegra/mc.h
>>> +++ b/drivers/memory/tegra/mc.h
>>> @@ -44,6 +44,15 @@
>>>   #define MC_TIMING_CONTROL_DBG                                0xf8
>>>   #define MC_TIMING_CONTROL                            0xfc
>>>
>>
>> this empty line is unnecessary
>>
> I'll fix this in next version.
> 
>>> +#define MC_ERR_VPR_STATUS                            0x654
>>> +#define MC_ERR_VPR_ADR                                       0x658
>>> +#define MC_ERR_SEC_STATUS                            0x67c
>>> +#define MC_ERR_SEC_ADR                                       0x680
>>> +#define MC_ERR_MTS_STATUS                            0x9b0
>>> +#define MC_ERR_MTS_ADR                                       0x9b4
>>> +#define MC_ERR_GENERALIZED_CARVEOUT_STATUS           0xc00
>>> +#define MC_ERR_GENERALIZED_CARVEOUT_ADR                      0xc04
>>> +
>>>   #define MC_INT_DECERR_ROUTE_SANITY                   BIT(20)
>>>   #define MC_INT_WCAM_ERR                                      BIT(19)
>>>   #define MC_INT_SCRUB_ECC_WR_ACK                              BIT(18)
>>> @@ -159,6 +168,14 @@ extern const struct tegra_mc_ops tegra186_mc_ops;
>>>   extern const char * const tegra_mc_status_names[32];
>>>   extern const char * const tegra_mc_error_names[8];
>>>
>>> +struct tegra_mc_error {
>>> +     u32 int_bit;
>>> +     const char *msg;
>>> +     u32 status_reg;
>>> +     u32 addr_reg;
>>> +     u32 addr_reg_hi;
>>> +};
>>> +
>>>   /*
>>>    * These IDs are for internal use of Tegra ICC drivers. The ID
>>> numbers are
>>>    * chosen such that they don't conflict with the device-tree ICC
>>> node IDs.
>>> diff --git a/drivers/memory/tegra/tegra186.c
>>> b/drivers/memory/tegra/tegra186.c
>>> index 6766cc4..4f3ae71 100644
>>> --- a/drivers/memory/tegra/tegra186.c
>>> +++ b/drivers/memory/tegra/tegra186.c
>>> @@ -146,8 +146,107 @@ static void tegra186_mc_clear_interrupt(struct
>>> tegra_mc *mc)
>>>        mc_writel(mc, MC_INTSTATUS_CLEAR, MC_INTSTATUS);
>>>   }
>>>
>>> +static const struct tegra_mc_error int_mc_errors[] = {
>>> +     {
>>> +             .int_bit = MC_INT_DECERR_EMEM,
>>> +             .msg = "EMEM address decode error",
>>> +             .status_reg = MC_ERR_STATUS,
>>> +             .addr_reg = MC_ERR_ADR,
>>> +     },
>>> +     {
>>> +             .int_bit = MC_INT_SECURITY_VIOLATION,
>>> +             .msg = "non secure access to secure region",
>>> +             .status_reg = MC_ERR_STATUS,
>>> +             .addr_reg = MC_ERR_ADR,
>>> +     },
>>> +     {
>>> +             .int_bit = MC_INT_DECERR_VPR,
>>> +             .msg = "MC request violates VPR requirements",
>>> +             .status_reg = MC_ERR_VPR_STATUS,
>>> +             .addr_reg = MC_ERR_VPR_ADR,
>>> +     },
>>
>> I see that these VPR registers present on all SoCs starting with T124.
>> It doesn't look like you need the separate IRQ handlers at all, instead
>> please extend the common T30 handler. For example, you may add a
>> switch-case statements to handle those T124+ specific bits differently.
>>
>> static irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
>> {
>> ...
>>          switch (bit) {
>>          case MC_INT_DECERR_VPR:
>>                  status_reg = MC_ERR_VPR_STATUS;
>>                  addr_reg   = MC_ERR_VPR_ADR;
>>                  break;
>>          ...
>>          default:
>>                  status_reg = MC_ERR_STATUS;
>>                  addr_reg   = MC_ERR_ADR;
>>          }
>>
>>          value = mc_readl(mc, status_reg);
>>          ...
>>
>>          value = mc_readl(mc, addr_reg);
> Okay. I'll use same handler as Tegra30 with additional Tegra186 onward
> bits.
> Also, shall I change name of tegra30_mc_handle_irq() to
> tegra_mc_handle_irq() as we are using it across all Tegra SOCs ?

T20 won't use it, no need to change the name.

Re: [Patch V1 1/4] memory: tegra: Add support for mc interrupts

[Dmitry Osipenko]

19.01.2022 11:47, Ashish Mhetre пишет:
> 
> 
> On 1/12/2022 1:43 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 11.01.2022 21:45, Ashish Mhetre пишет:
>>>
>>> @@ -765,16 +768,21 @@ static int tegra_mc_probe(struct
>>> platform_device *pdev)
>>>                        return err;
>>>        }
>>>
>>> -     if (mc->soc->ops && mc->soc->ops->handle_irq) {
>>> +     if (mc->soc->interrupt_ops &&
>>> mc->soc->interrupt_ops->handle_irq) {
>>>                mc->irq = platform_get_irq(pdev, 0);
>>>                if (mc->irq < 0)
>>>                        return mc->irq;
>>>
>>>                WARN(!mc->soc->client_id_mask, "missing client ID mask
>>> for this SoC\n");
>>>
>>> +             /* clear any mc-errs that occurred before. */
>>
>> s/mc-errs/Memory Controller errors/
>>
> Sure, I'll update these in next version.
>>> +             if (mc->soc->interrupt_ops->clear_interrupt)
>>> +                     mc->soc->interrupt_ops->clear_interrupt(mc);
>>
>> There is no explanation of this change neither in the code, nor in the
>> commit message. Please always provide detailed descriptions for a
>> non-trivial changes.
>>
>> Interrupts aren't cleared intentionally by the driver, otherwise you'll
>> never know about early-boot memory faults which happened before the
>> probe. Hence this change is incorrect.
> That's true, we should be logging early-boot memory faults as well.
> Ideally there shouldn't be any early-boot faults as all clients will
> be up after MC, right? But I agree that we should be checking and
> logging if any interrupt is present.

Normally there won't be any errors during early boot, otherwise they
need to be fixed.

Re: [Patch V1 3/4] memory: tegra: add mc-err support for T186

[Dmitry Osipenko]

19.01.2022 12:09, Ashish Mhetre пишет:
> 
> 
> On 1/12/2022 4:54 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 12.01.2022 14:22, Dmitry Osipenko пишет:
>>> 11.01.2022 21:45, Ashish Mhetre пишет:
>>>>   #define MC_INT_DECERR_ROUTE_SANITY                  BIT(20)
>>>>   #define MC_INT_WCAM_ERR                                     BIT(19)
>>>>   #define MC_INT_SCRUB_ECC_WR_ACK                             BIT(18)
>>>
>>> I don't see where these errors are handled in the code. Is documentation
>>> that explains these bits publicly available?
>>>
> MC_INT_DECERR_ROUTE_SANITY is supposed to be part a of Tegra194
> interrupts. I have missed adding it and will update in next version.
> MC_INT_WCAM_ERR is a MC channel error and is not applicable in upstream.
> I'll remove it.
> These bits are defined in same documentation where other errors are
> defined.

Is this documentation publicly available? I only have access to the
public TRM that can be downloaded from the NVIDIA website and there are
no register sets there for newer SoCs, AFAICS. If TRM was updated and
now contains registers, then I'll try to get the latest version.

Re: [Patch V1 1/4] memory: tegra: Add support for mc interrupts

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2789 bytes --]

Hi Ashish,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on v5.16 next-20220120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ashish-Mhetre/memory-tegra-Update-mc-interrupts/20220112-024847
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20220121/202201210912.gUHr0zcb-lkp(a)intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/71553a1a735ae07293d43df685262f85e6453170
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ashish-Mhetre/memory-tegra-Update-mc-interrupts/20220112-024847
        git checkout 71553a1a735ae07293d43df685262f85e6453170
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/memory/tegra/tegra20.c:797:10: error: 'const struct tegra_mc_ops' has no member named 'interrupt_ops'
     797 |         .interrupt_ops = tegra20_mc_interrupt_ops,
         |          ^~~~~~~~~~~~~
>> drivers/memory/tegra/tegra20.c:797:26: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     797 |         .interrupt_ops = tegra20_mc_interrupt_ops,
         |                          ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/memory/tegra/tegra20.c:797:26: note: (near initialization for 'tegra20_mc_ops')
   drivers/memory/tegra/tegra20.c:797:26: error: incompatible types when initializing type 'int (*)(struct tegra_mc *)' using type 'struct tegra_mc_interrupt_ops'
   cc1: some warnings being treated as errors


vim +797 drivers/memory/tegra/tegra20.c

   792	
   793	static const struct tegra_mc_ops tegra20_mc_ops = {
   794		.probe = tegra20_mc_probe,
   795		.suspend = tegra20_mc_suspend,
   796		.resume = tegra20_mc_resume,
 > 797		.interrupt_ops = tegra20_mc_interrupt_ops,
   798	};
   799	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org