From tegra186 onward, memory controllers support multiple channels.
Add memory controller channels in device tree and add support to map
address spaces of these channels in tegra MC driver.
When memory controller interrupt occurs, registers from these channels
are required to be read in order to get error information.
Add error logging support from tegra186 onward for memory controller
interrupts.
Ashish Mhetre (4):
arm64: tegra: Add memory controller channels
dt-bindings: memory: Update reg maxitems for tegra186
memory: tegra: Add memory controller channels support
memory: tegra: Add MC error logging on tegra186 onward
---
Changes in v4:
- Added memory controller channels support
- Added newlines after every break statement of all switch cases
- Fixed compile error with W=1 build
- Fixed the interrupt mask bit logic
Changes in v3:
- Removed unnecessary ifdefs
- Grouped newly added MC registers with existing MC registers
- Removed unnecessary initialization of variables
- Updated code to use newly added field 'has_addr_hi_reg' instead of ifdefs
Changes in v2:
- Updated patch subject and commit message
- Removed separate irq handlers
- Updated tegra30_mc_handle_irq to be used for tegra186 onwards as well
.../memory-controllers/nvidia,tegra186-mc.yaml | 2 +-
arch/arm64/boot/dts/nvidia/tegra186.dtsi | 7 +-
arch/arm64/boot/dts/nvidia/tegra194.dtsi | 21 +++-
arch/arm64/boot/dts/nvidia/tegra234.dtsi | 21 +++-
drivers/memory/tegra/mc.c | 108 ++++++++++++++++++---
drivers/memory/tegra/mc.h | 37 ++++++-
drivers/memory/tegra/tegra186.c | 67 +++++++++++++
drivers/memory/tegra/tegra194.c | 46 +++++++++
drivers/memory/tegra/tegra234.c | 1 +
include/soc/tegra/mc.h | 10 ++
10 files changed, 296 insertions(+), 24 deletions(-)
--
2.7.4
From tegra186 onwards, memory controller support multiple channels.
During the error interrupts from memory controller, corresponding
channels need to be accessed for logging error info and clearing the
interrupt.
So add address and size of these channels in device tree node of
tegra186, tegra194 and tegra234 memory controller.
Signed-off-by: Ashish Mhetre <[email protected]>
---
arch/arm64/boot/dts/nvidia/tegra186.dtsi | 7 ++++++-
arch/arm64/boot/dts/nvidia/tegra194.dtsi | 21 ++++++++++++++++++---
arch/arm64/boot/dts/nvidia/tegra234.dtsi | 21 ++++++++++++++++++---
3 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index e9b40f5..9c14404 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -521,7 +521,12 @@
mc: memory-controller@2c00000 {
compatible = "nvidia,tegra186-mc";
- reg = <0x0 0x02c00000 0x0 0xb0000>;
+ reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */
+ <0x0 0x02c10000 0x0 0x10000>, /* Broadcast channel */
+ <0x0 0x02c20000 0x0 0x10000>, /* MC0 */
+ <0x0 0x02c30000 0x0 0x10000>, /* MC1 */
+ <0x0 0x02c40000 0x0 0x10000>, /* MC2 */
+ <0x0 0x02c50000 0x0 0x10000>; /* MC3 */
interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
status = "disabled";
diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index c28bf4d..e19c56c 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -604,9 +604,24 @@
mc: memory-controller@2c00000 {
compatible = "nvidia,tegra194-mc";
- reg = <0x02c00000 0x100000>,
- <0x02b80000 0x040000>,
- <0x01700000 0x100000>;
+ reg = <0x02c00000 0x10000>, /* MC-SID */
+ <0x02c10000 0x10000>, /* MC Broadcast*/
+ <0x02c20000 0x10000>, /* MC0 */
+ <0x02c30000 0x10000>, /* MC1 */
+ <0x02c40000 0x10000>, /* MC2 */
+ <0x02c50000 0x10000>, /* MC3 */
+ <0x02b80000 0x10000>, /* MC4 */
+ <0x02b90000 0x10000>, /* MC5 */
+ <0x02ba0000 0x10000>, /* MC6 */
+ <0x02bb0000 0x10000>, /* MC7 */
+ <0x01700000 0x10000>, /* MC8 */
+ <0x01710000 0x10000>, /* MC9 */
+ <0x01720000 0x10000>, /* MC10 */
+ <0x01730000 0x10000>, /* MC11 */
+ <0x01740000 0x10000>, /* MC12 */
+ <0x01750000 0x10000>, /* MC13 */
+ <0x01760000 0x10000>, /* MC14 */
+ <0x01770000 0x10000>; /* MC15 */
interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
#interconnect-cells = <1>;
status = "disabled";
diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
index aaace60..6e33d2b 100644
--- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
@@ -507,9 +507,24 @@
mc: memory-controller@2c00000 {
compatible = "nvidia,tegra234-mc";
- reg = <0x02c00000 0x100000>,
- <0x02b80000 0x040000>,
- <0x01700000 0x100000>;
+ reg = <0x02c00000 0x10000>, /* MC-SID */
+ <0x02c10000 0x10000>, /* MC Broadcast*/
+ <0x02c20000 0x10000>, /* MC0 */
+ <0x02c30000 0x10000>, /* MC1 */
+ <0x02c40000 0x10000>, /* MC2 */
+ <0x02c50000 0x10000>, /* MC3 */
+ <0x02b80000 0x10000>, /* MC4 */
+ <0x02b90000 0x10000>, /* MC5 */
+ <0x02ba0000 0x10000>, /* MC6 */
+ <0x02bb0000 0x10000>, /* MC7 */
+ <0x01700000 0x10000>, /* MC8 */
+ <0x01710000 0x10000>, /* MC9 */
+ <0x01720000 0x10000>, /* MC10 */
+ <0x01730000 0x10000>, /* MC11 */
+ <0x01740000 0x10000>, /* MC12 */
+ <0x01750000 0x10000>, /* MC13 */
+ <0x01760000 0x10000>, /* MC14 */
+ <0x01770000 0x10000>; /* MC15 */
interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
#interconnect-cells = <1>;
status = "okay";
--
2.7.4
Add new function 'get_int_channel' in tegra_mc_soc struture which is
implemented by tegra SOCs which support multiple MC channels. This
function returns the channel which should be used to get the information
of interrupts.
Remove static from tegra30_mc_handle_irq and use it as interrupt handler
for MC interrupts on tegra186, tegra194 and tegra234 to log the errors.
Add error specific MC status and address register bits and use them on
tegra186, tegra194 and tegra234.
Add error logging for generalized carveout interrupt on tegra186, tegra194
and tegra234.
Add error logging for route sanity interrupt on tegra194 an tegra234.
Add register for higher bits of error address which is available on
tegra194 and tegra234.
Add a boolean variable 'has_addr_hi_reg' in tegra_mc_soc struture which
will be true if soc has register for higher bits of memory controller
error address. Set it true for tegra194 and tegra234.
Signed-off-by: Ashish Mhetre <[email protected]>
---
drivers/memory/tegra/mc.c | 102 ++++++++++++++++++++++++++++++++++------
drivers/memory/tegra/mc.h | 37 ++++++++++++++-
drivers/memory/tegra/tegra186.c | 45 ++++++++++++++++++
drivers/memory/tegra/tegra194.c | 44 +++++++++++++++++
drivers/memory/tegra/tegra234.c | 59 +++++++++++++++++++++++
include/soc/tegra/mc.h | 4 ++
6 files changed, 275 insertions(+), 16 deletions(-)
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index 3cda1d9..bb861a8 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -508,14 +508,32 @@ int tegra30_mc_probe(struct tegra_mc *mc)
return 0;
}
-static irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
+const struct tegra_mc_ops tegra30_mc_ops = {
+ .probe = tegra30_mc_probe,
+ .handle_irq = tegra30_mc_handle_irq,
+};
+#endif
+
+irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
{
struct tegra_mc *mc = data;
unsigned long status;
unsigned int bit;
+ int channel;
+
+ if (mc->soc->num_channels && mc->soc->get_int_channel) {
+ int err;
+
+ err = mc->soc->get_int_channel(mc, &channel);
+ if (err < 0)
+ return IRQ_NONE;
+
+ /* mask all interrupts to avoid flooding */
+ status = mc_ch_readl(mc, channel, MC_INTSTATUS) & mc->soc->intmask;
+ } else {
+ status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
+ }
- /* mask all interrupts to avoid flooding */
- status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
if (!status)
return IRQ_NONE;
@@ -523,18 +541,66 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
const char *error = tegra_mc_status_names[bit] ?: "unknown";
const char *client = "unknown", *desc;
const char *direction, *secure;
+ u32 status_reg, addr_reg;
+ u32 intmask = BIT(bit);
phys_addr_t addr = 0;
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+ u32 addr_hi_reg = 0;
+#endif
unsigned int i;
char perm[7];
u8 id, type;
u32 value;
- value = mc_readl(mc, MC_ERR_STATUS);
+ switch (intmask) {
+ case MC_INT_DECERR_VPR:
+ status_reg = MC_ERR_VPR_STATUS;
+ addr_reg = MC_ERR_VPR_ADR;
+ break;
+
+ case MC_INT_SECERR_SEC:
+ status_reg = MC_ERR_SEC_STATUS;
+ addr_reg = MC_ERR_SEC_ADR;
+ break;
+
+ case MC_INT_DECERR_MTS:
+ status_reg = MC_ERR_MTS_STATUS;
+ addr_reg = MC_ERR_MTS_ADR;
+ break;
+
+ case MC_INT_DECERR_GENERALIZED_CARVEOUT:
+ status_reg = MC_ERR_GENERALIZED_CARVEOUT_STATUS;
+ addr_reg = MC_ERR_GENERALIZED_CARVEOUT_ADR;
+ break;
+
+ case MC_INT_DECERR_ROUTE_SANITY:
+ status_reg = MC_ERR_ROUTE_SANITY_STATUS;
+ addr_reg = MC_ERR_ROUTE_SANITY_ADR;
+ break;
+
+ default:
+ status_reg = MC_ERR_STATUS;
+ addr_reg = MC_ERR_ADR;
+
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+ if (mc->soc->has_addr_hi_reg)
+ addr_hi_reg = MC_ERR_ADR_HI;
+#endif
+ break;
+ }
+
+ if (mc->soc->num_channels)
+ value = mc_ch_readl(mc, channel, status_reg);
+ else
+ value = mc_readl(mc, 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);
+ if (addr_hi_reg)
+ addr = mc_ch_readl(mc, channel, addr_hi_reg);
+ else
+ addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
+ MC_ERR_STATUS_ADR_HI_MASK);
addr <<= 32;
}
#endif
@@ -591,7 +657,10 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
break;
}
- value = mc_readl(mc, MC_ERR_ADR);
+ if (mc->soc->num_channels)
+ value = mc_ch_readl(mc, channel, addr_reg);
+ else
+ value = mc_readl(mc, addr_reg);
addr |= value;
dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s%s)\n",
@@ -600,17 +669,14 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
}
/* clear interrupts */
- mc_writel(mc, status, MC_INTSTATUS);
+ if (mc->soc->num_channels)
+ mc_ch_writel(mc, channel, status, MC_INTSTATUS);
+ else
+ mc_writel(mc, status, MC_INTSTATUS);
return IRQ_HANDLED;
}
-const struct tegra_mc_ops tegra30_mc_ops = {
- .probe = tegra30_mc_probe,
- .handle_irq = tegra30_mc_handle_irq,
-};
-#endif
-
const char *const tegra_mc_status_names[32] = {
[ 1] = "External interrupt",
[ 6] = "EMEM address decode error",
@@ -622,6 +688,8 @@ const char *const tegra_mc_status_names[32] = {
[12] = "VPR violation",
[13] = "Secure carveout violation",
[16] = "MTS carveout violation",
+ [17] = "Generalized carveout violation",
+ [20] = "Route Sanity error",
};
const char *const tegra_mc_error_names[8] = {
@@ -770,7 +838,11 @@ static int tegra_mc_probe(struct platform_device *pdev)
WARN(!mc->soc->client_id_mask, "missing client ID mask for this SoC\n");
- mc_writel(mc, mc->soc->intmask, MC_INTMASK);
+ if (mc->soc->num_channels)
+ mc_ch_writel(mc, MC_BROADCAST_CHANNEL, mc->soc->intmask,
+ MC_INTMASK);
+ else
+ mc_writel(mc, mc->soc->intmask, MC_INTMASK);
err = devm_request_irq(&pdev->dev, mc->irq, mc->soc->ops->handle_irq, 0,
dev_name(&pdev->dev), mc);
diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
index 062886e..3836c35 100644
--- a/drivers/memory/tegra/mc.h
+++ b/drivers/memory/tegra/mc.h
@@ -43,7 +43,21 @@
#define MC_EMEM_ARB_OVERRIDE 0xe8
#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_ROUTE_SANITY_STATUS 0x9c0
+#define MC_ERR_ROUTE_SANITY_ADR 0x9c4
+#define MC_ERR_GENERALIZED_CARVEOUT_STATUS 0xc00
+#define MC_ERR_GENERALIZED_CARVEOUT_ADR 0xc04
+#define MC_GLOBAL_INTSTATUS 0xf24
+#define MC_ERR_ADR_HI 0x11fc
+
+#define MC_INT_DECERR_ROUTE_SANITY BIT(20)
+#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)
@@ -78,6 +92,8 @@
#define MC_TIMING_UPDATE BIT(0)
+#define MC_BROADCAST_CHANNEL ~0
+
static inline u32 tegra_mc_scale_percents(u64 val, unsigned int percents)
{
val = val * percents;
@@ -92,6 +108,24 @@ icc_provider_to_tegra_mc(struct icc_provider *provider)
return container_of(provider, struct tegra_mc, provider);
}
+static inline u32 mc_ch_readl(const struct tegra_mc *mc, int ch,
+ unsigned long offset)
+{
+ if (ch == MC_BROADCAST_CHANNEL)
+ return readl_relaxed(mc->mcb_regs + offset);
+
+ return readl_relaxed(mc->mc_regs[ch] + offset);
+}
+
+static inline void mc_ch_writel(const struct tegra_mc *mc, int ch,
+ u32 value, unsigned long offset)
+{
+ if (ch == MC_BROADCAST_CHANNEL)
+ writel_relaxed(value, mc->mcb_regs + offset);
+ else
+ writel_relaxed(value, mc->mc_regs[ch] + offset);
+}
+
static inline u32 mc_readl(const struct tegra_mc *mc, unsigned long offset)
{
return readl_relaxed(mc->regs + offset);
@@ -156,6 +190,7 @@ extern const struct tegra_mc_ops tegra30_mc_ops;
extern const struct tegra_mc_ops tegra186_mc_ops;
#endif
+irqreturn_t tegra30_mc_handle_irq(int irq, void *data);
extern const char * const tegra_mc_status_names[32];
extern const char * const tegra_mc_error_names[8];
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index 59a4425..bce0237 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -16,6 +16,8 @@
#include <dt-bindings/memory/tegra186-mc.h>
#endif
+#include "mc.h"
+
#define MC_SID_STREAMID_OVERRIDE_MASK GENMASK(7, 0)
#define MC_SID_STREAMID_SECURITY_WRITE_ACCESS_DISABLED BIT(16)
#define MC_SID_STREAMID_SECURITY_OVERRIDE BIT(8)
@@ -164,6 +166,7 @@ const struct tegra_mc_ops tegra186_mc_ops = {
.resume = tegra186_mc_resume,
.probe_device = tegra186_mc_probe_device,
.map_regs = tegra186_mc_map_regs,
+ .handle_irq = tegra30_mc_handle_irq,
};
#if defined(CONFIG_ARCH_TEGRA_186_SOC)
@@ -891,11 +894,53 @@ static const struct tegra_mc_client tegra186_mc_clients[] = {
},
};
+static int tegra186_mc_get_channel(struct tegra_mc *mc, int *mc_channel)
+{
+ u32 g_intstatus;
+
+ g_intstatus = mc_ch_readl(mc, MC_BROADCAST_CHANNEL,
+ MC_GLOBAL_INTSTATUS);
+
+ switch (g_intstatus & mc->soc->int_channel_mask) {
+ case BIT(0):
+ *mc_channel = 0;
+ break;
+
+ case BIT(1):
+ *mc_channel = 1;
+ break;
+
+ case BIT(2):
+ *mc_channel = 2;
+ break;
+
+ case BIT(3):
+ *mc_channel = 3;
+ break;
+
+ case BIT(24):
+ *mc_channel = MC_BROADCAST_CHANNEL;
+ break;
+
+ default:
+ pr_err("Unknown interrupt source\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
const struct tegra_mc_soc tegra186_mc_soc = {
.num_clients = ARRAY_SIZE(tegra186_mc_clients),
.clients = tegra186_mc_clients,
.num_address_bits = 40,
.num_channels = 4,
+ .client_id_mask = 0xff,
+ .intmask = 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,
+ .int_channel_mask = 0x100000f,
+ .get_int_channel = tegra186_mc_get_channel,
};
#endif
diff --git a/drivers/memory/tegra/tegra194.c b/drivers/memory/tegra/tegra194.c
index 9400117..bc16567 100644
--- a/drivers/memory/tegra/tegra194.c
+++ b/drivers/memory/tegra/tegra194.c
@@ -1343,10 +1343,54 @@ static const struct tegra_mc_client tegra194_mc_clients[] = {
},
};
+static int tegra194_mc_get_channel(struct tegra_mc *mc, int *mc_channel)
+{
+ u32 g_intstatus;
+
+ g_intstatus = mc_ch_readl(mc, MC_BROADCAST_CHANNEL,
+ MC_GLOBAL_INTSTATUS);
+
+ switch (g_intstatus & mc->soc->int_channel_mask) {
+ case BIT(8):
+ *mc_channel = 0;
+ break;
+
+ case BIT(9):
+ *mc_channel = 1;
+ break;
+
+ case BIT(10):
+ *mc_channel = 2;
+ break;
+
+ case BIT(11):
+ *mc_channel = 3;
+ break;
+
+ case BIT(25):
+ *mc_channel = MC_BROADCAST_CHANNEL;
+ break;
+
+ default:
+ pr_err("Unknown interrupt source\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
const struct tegra_mc_soc tegra194_mc_soc = {
.num_clients = ARRAY_SIZE(tegra194_mc_clients),
.clients = tegra194_mc_clients,
.num_address_bits = 40,
.num_channels = 16,
+ .client_id_mask = 0xff,
+ .intmask = MC_INT_DECERR_ROUTE_SANITY |
+ 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,
+ .has_addr_hi_reg = true,
.ops = &tegra186_mc_ops,
+ .int_channel_mask = 0x2000f00,
+ .get_int_channel = tegra194_mc_get_channel,
};
diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
index 6335a13..8e09fc4 100644
--- a/drivers/memory/tegra/tegra234.c
+++ b/drivers/memory/tegra/tegra234.c
@@ -93,10 +93,69 @@ static const struct tegra_mc_client tegra234_mc_clients[] = {
},
};
+static int tegra234_mc_get_channel(struct tegra_mc *mc, int *mc_channel)
+{
+ u32 g_intstatus;
+
+ g_intstatus = mc_ch_readl(mc, MC_BROADCAST_CHANNEL,
+ MC_GLOBAL_INTSTATUS);
+
+ switch (g_intstatus & mc->soc->int_channel_mask) {
+ case BIT(8):
+ *mc_channel = 0;
+ break;
+
+ case BIT(9):
+ *mc_channel = 1;
+ break;
+
+ case BIT(10):
+ *mc_channel = 2;
+ break;
+
+ case BIT(11):
+ *mc_channel = 3;
+ break;
+
+ case BIT(12):
+ *mc_channel = 4;
+ break;
+
+ case BIT(13):
+ *mc_channel = 5;
+ break;
+
+ case BIT(14):
+ *mc_channel = 6;
+ break;
+
+ case BIT(15):
+ *mc_channel = 7;
+ break;
+
+ case BIT(25):
+ *mc_channel = MC_BROADCAST_CHANNEL;
+ break;
+
+ default:
+ pr_err("Unknown interrupt source\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
const struct tegra_mc_soc tegra234_mc_soc = {
.num_clients = ARRAY_SIZE(tegra234_mc_clients),
.clients = tegra234_mc_clients,
.num_address_bits = 40,
.num_channels = 16,
+ .intmask = MC_INT_DECERR_ROUTE_SANITY |
+ 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,
+ .has_addr_hi_reg = true,
.ops = &tegra186_mc_ops,
+ .int_channel_mask = 0x200ff00,
+ .get_int_channel = tegra234_mc_get_channel,
};
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 92f810c..1fe6a62 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -203,6 +203,8 @@ struct tegra_mc_soc {
const struct tegra_smmu_soc *smmu;
u32 intmask;
+ u32 int_channel_mask;
+ bool has_addr_hi_reg;
const struct tegra_mc_reset_ops *reset_ops;
const struct tegra_mc_reset *resets;
@@ -210,6 +212,8 @@ struct tegra_mc_soc {
const struct tegra_mc_icc_ops *icc_ops;
const struct tegra_mc_ops *ops;
+
+ int (*get_int_channel)(struct tegra_mc *mc, int *mc_channel);
};
struct tegra_mc {
--
2.7.4
On 02/03/2022 09:43, Ashish Mhetre wrote:
> Add new function 'get_int_channel' in tegra_mc_soc struture which is
> implemented by tegra SOCs which support multiple MC channels. This
> function returns the channel which should be used to get the information
> of interrupts.
> Remove static from tegra30_mc_handle_irq and use it as interrupt handler
> for MC interrupts on tegra186, tegra194 and tegra234 to log the errors.
> Add error specific MC status and address register bits and use them on
> tegra186, tegra194 and tegra234.
> Add error logging for generalized carveout interrupt on tegra186, tegra194
> and tegra234.
> Add error logging for route sanity interrupt on tegra194 an tegra234.
> Add register for higher bits of error address which is available on
> tegra194 and tegra234.
> Add a boolean variable 'has_addr_hi_reg' in tegra_mc_soc struture which
> will be true if soc has register for higher bits of memory controller
> error address. Set it true for tegra194 and tegra234.
>
> Signed-off-by: Ashish Mhetre <[email protected]>
> ---
> drivers/memory/tegra/mc.c | 102 ++++++++++++++++++++++++++++++++++------
> drivers/memory/tegra/mc.h | 37 ++++++++++++++-
> drivers/memory/tegra/tegra186.c | 45 ++++++++++++++++++
> drivers/memory/tegra/tegra194.c | 44 +++++++++++++++++
> drivers/memory/tegra/tegra234.c | 59 +++++++++++++++++++++++
> include/soc/tegra/mc.h | 4 ++
> 6 files changed, 275 insertions(+), 16 deletions(-)
>
(...)
>
> +static int tegra186_mc_get_channel(struct tegra_mc *mc, int *mc_channel)
> +{
> + u32 g_intstatus;
> +
> + g_intstatus = mc_ch_readl(mc, MC_BROADCAST_CHANNEL,
> + MC_GLOBAL_INTSTATUS);
> +
> + switch (g_intstatus & mc->soc->int_channel_mask) {
> + case BIT(0):
> + *mc_channel = 0;
> + break;
> +
> + case BIT(1):
> + *mc_channel = 1;
> + break;
> +
> + case BIT(2):
> + *mc_channel = 2;
> + break;
> +
> + case BIT(3):
> + *mc_channel = 3;
> + break;
> +
> + case BIT(24):
> + *mc_channel = MC_BROADCAST_CHANNEL;
> + break;
> +
> + default:
> + pr_err("Unknown interrupt source\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> const struct tegra_mc_soc tegra186_mc_soc = {
> .num_clients = ARRAY_SIZE(tegra186_mc_clients),
> .clients = tegra186_mc_clients,
> .num_address_bits = 40,
> .num_channels = 4,
> + .client_id_mask = 0xff,
> + .intmask = 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,
> + .int_channel_mask = 0x100000f,
> + .get_int_channel = tegra186_mc_get_channel,
> };
> #endif
> diff --git a/drivers/memory/tegra/tegra194.c b/drivers/memory/tegra/tegra194.c
> index 9400117..bc16567 100644
> --- a/drivers/memory/tegra/tegra194.c
> +++ b/drivers/memory/tegra/tegra194.c
> @@ -1343,10 +1343,54 @@ static const struct tegra_mc_client tegra194_mc_clients[] = {
> },
> };
>
> +static int tegra194_mc_get_channel(struct tegra_mc *mc, int *mc_channel)
Looks like 'mc' could be a pointer to const.
> +{
> + u32 g_intstatus;
Variable name just "status" because it looks like some
hungarian-notation-style...
The same in other places like this.
Best regards,
Krzysztof
On 02/03/2022 09:43, Ashish Mhetre wrote:
> From tegra186 onwards, memory controller support multiple channels.
> During the error interrupts from memory controller, corresponding
> channels need to be accessed for logging error info and clearing the
> interrupt.
> So add address and size of these channels in device tree node of
> tegra186, tegra194 and tegra234 memory controller.
>
> Signed-off-by: Ashish Mhetre <[email protected]>
> ---
> arch/arm64/boot/dts/nvidia/tegra186.dtsi | 7 ++++++-
> arch/arm64/boot/dts/nvidia/tegra194.dtsi | 21 ++++++++++++++++++---
> arch/arm64/boot/dts/nvidia/tegra234.dtsi | 21 ++++++++++++++++++---
> 3 files changed, 42 insertions(+), 7 deletions(-)
>
You send DTS patch before, which is wrong order and actually it points
my attention to probably ABI break.
DTS goes always separately from driver changes so it is not only ABI
break, but also non-bisectable.
Best regards,
Krzysztof
From tegra186 onwards, memory controller support multiple channels.
Add support for mapping address spaces of these channels.
During error interrupts from memory controller, appropriate registers
from these channels need to be accessed for logging error info.
Signed-off-by: Ashish Mhetre <[email protected]>
---
drivers/memory/tegra/mc.c | 6 ++++++
drivers/memory/tegra/tegra186.c | 21 +++++++++++++++++++++
drivers/memory/tegra/tegra194.c | 1 +
drivers/memory/tegra/tegra234.c | 1 +
include/soc/tegra/mc.h | 7 +++++++
5 files changed, 36 insertions(+)
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index bf3abb6..3cda1d9 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)
if (IS_ERR(mc->regs))
return PTR_ERR(mc->regs);
+ if (mc->soc->ops && mc->soc->ops->map_regs) {
+ err = mc->soc->ops->map_regs(mc, pdev);
+ if (err < 0)
+ return err;
+ }
+
mc->debugfs.root = debugfs_create_dir("mc", NULL);
if (mc->soc->ops && mc->soc->ops->probe) {
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index 3d15388..59a4425 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -139,11 +139,31 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
return 0;
}
+static int tegra186_mc_map_regs(struct tegra_mc *mc,
+ struct platform_device *pdev)
+{
+ struct resource *res;
+ int i;
+
+ mc->mcb_regs = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
+ if (IS_ERR(mc->mcb_regs))
+ return PTR_ERR(mc->mcb_regs);
+
+ for (i = 0; i < mc->soc->num_channels; i++) {
+ mc->mc_regs[i] = devm_platform_get_and_ioremap_resource(pdev, i + 2, &res);
+ if (IS_ERR(mc->mc_regs[i]))
+ return PTR_ERR(mc->mc_regs[i]);
+ }
+
+ return 0;
+}
+
const struct tegra_mc_ops tegra186_mc_ops = {
.probe = tegra186_mc_probe,
.remove = tegra186_mc_remove,
.resume = tegra186_mc_resume,
.probe_device = tegra186_mc_probe_device,
+ .map_regs = tegra186_mc_map_regs,
};
#if defined(CONFIG_ARCH_TEGRA_186_SOC)
@@ -875,6 +895,7 @@ const struct tegra_mc_soc tegra186_mc_soc = {
.num_clients = ARRAY_SIZE(tegra186_mc_clients),
.clients = tegra186_mc_clients,
.num_address_bits = 40,
+ .num_channels = 4,
.ops = &tegra186_mc_ops,
};
#endif
diff --git a/drivers/memory/tegra/tegra194.c b/drivers/memory/tegra/tegra194.c
index cab998b..9400117 100644
--- a/drivers/memory/tegra/tegra194.c
+++ b/drivers/memory/tegra/tegra194.c
@@ -1347,5 +1347,6 @@ const struct tegra_mc_soc tegra194_mc_soc = {
.num_clients = ARRAY_SIZE(tegra194_mc_clients),
.clients = tegra194_mc_clients,
.num_address_bits = 40,
+ .num_channels = 16,
.ops = &tegra186_mc_ops,
};
diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
index e22824a..6335a13 100644
--- a/drivers/memory/tegra/tegra234.c
+++ b/drivers/memory/tegra/tegra234.c
@@ -97,5 +97,6 @@ const struct tegra_mc_soc tegra234_mc_soc = {
.num_clients = ARRAY_SIZE(tegra234_mc_clients),
.clients = tegra234_mc_clients,
.num_address_bits = 40,
+ .num_channels = 16,
.ops = &tegra186_mc_ops,
};
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 1066b11..92f810c 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -13,6 +13,9 @@
#include <linux/irq.h>
#include <linux/reset-controller.h>
#include <linux/types.h>
+#include <linux/platform_device.h>
+
+#define MC_MAX_CHANNELS 16
struct clk;
struct device;
@@ -181,6 +184,7 @@ struct tegra_mc_ops {
int (*resume)(struct tegra_mc *mc);
irqreturn_t (*handle_irq)(int irq, void *data);
int (*probe_device)(struct tegra_mc *mc, struct device *dev);
+ int (*map_regs)(struct tegra_mc *mc, struct platform_device *pdev);
};
struct tegra_mc_soc {
@@ -194,6 +198,7 @@ struct tegra_mc_soc {
unsigned int atom_size;
u8 client_id_mask;
+ u8 num_channels;
const struct tegra_smmu_soc *smmu;
@@ -212,6 +217,8 @@ struct tegra_mc {
struct tegra_smmu *smmu;
struct gart_device *gart;
void __iomem *regs;
+ void __iomem *mcb_regs;
+ void __iomem *mc_regs[MC_MAX_CHANNELS];
struct clk *clk;
int irq;
--
2.7.4
On 02/03/2022 09:43, Ashish Mhetre wrote:
> From tegra186 onwards, memory controller support multiple channels.
> Add support for mapping address spaces of these channels.
> During error interrupts from memory controller, appropriate registers
> from these channels need to be accessed for logging error info.
>
> Signed-off-by: Ashish Mhetre <[email protected]>
> ---
> drivers/memory/tegra/mc.c | 6 ++++++
> drivers/memory/tegra/tegra186.c | 21 +++++++++++++++++++++
> drivers/memory/tegra/tegra194.c | 1 +
> drivers/memory/tegra/tegra234.c | 1 +
> include/soc/tegra/mc.h | 7 +++++++
> 5 files changed, 36 insertions(+)
>
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index bf3abb6..3cda1d9 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)
> if (IS_ERR(mc->regs))
> return PTR_ERR(mc->regs);
>
> + if (mc->soc->ops && mc->soc->ops->map_regs) {
> + err = mc->soc->ops->map_regs(mc, pdev);
> + if (err < 0)
> + return err;
> + }
> +
> mc->debugfs.root = debugfs_create_dir("mc", NULL);
>
> if (mc->soc->ops && mc->soc->ops->probe) {
> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> index 3d15388..59a4425 100644
> --- a/drivers/memory/tegra/tegra186.c
> +++ b/drivers/memory/tegra/tegra186.c
> @@ -139,11 +139,31 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
> return 0;
> }
>
> +static int tegra186_mc_map_regs(struct tegra_mc *mc,
> + struct platform_device *pdev)
> +{
> + struct resource *res;
> + int i;
> +
> + mc->mcb_regs = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
> + if (IS_ERR(mc->mcb_regs))
> + return PTR_ERR(mc->mcb_regs);
> +
> + for (i = 0; i < mc->soc->num_channels; i++) {
> + mc->mc_regs[i] = devm_platform_get_and_ioremap_resource(pdev, i + 2, &res);
> + if (IS_ERR(mc->mc_regs[i]))
> + return PTR_ERR(mc->mc_regs[i]);
This breaks the ABI, so I need Thierry's ack that such ABI break is
perfectly ok.
> + }
> +
> + return 0;
> +}
> +
Best regards,
Krzysztof
From tegra186 onwards, memory controller support multiple channels.
Reg items are updated with address and size of these channels.
Tegra186 has overall 5 memory controller channels. Tegra194 and tegra234
have overall 17 memory controller channels each.
There is 1 reg item for memory controller stream-id registers.
So update the reg maxItems to 18 in tegra186 devicetree documentation.
Signed-off-by: Ashish Mhetre <[email protected]>
---
.../devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
index 13c4c82..eb7ed00 100644
--- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
@@ -35,7 +35,7 @@ properties:
reg:
minItems: 1
- maxItems: 3
+ maxItems: 18
interrupts:
items:
--
2.7.4
On 03/03/2022 13:31, Dan Carpenter wrote:
> Hi Ashish,
>
> url: https://github.com/0day-ci/linux/commits/Ashish-Mhetre/memory-tegra-Add-MC-channels-and-error-logging/20220302-164625
> base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
> config: openrisc-randconfig-m031-20220302 (https://download.01.org/0day-ci/archive/20220303/[email protected]/config)
> compiler: or1k-linux-gcc (GCC) 11.2.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>
> New smatch warnings:
> drivers/memory/tegra/mc.c:593 tegra30_mc_handle_irq() error: uninitialized symbol 'channel'.
Ashish,
I mentioned with your v3 that it is expected for submitter to run
certain automatic tools:
"We not only expect to compile it but also compile with W=1, run sparse,
smatch and coccicheck. Then also test."
Judging by the output here, it could be that either you used old
compiler or did not run the checks.
Can you please confirm that you performed all the activities mentioned
before?
Best regards,
Krzysztof
Hi Ashish,
url: https://github.com/0day-ci/linux/commits/Ashish-Mhetre/memory-tegra-Add-MC-channels-and-error-logging/20220302-164625
base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: openrisc-randconfig-m031-20220302 (https://download.01.org/0day-ci/archive/20220303/[email protected]/config)
compiler: or1k-linux-gcc (GCC) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
New smatch warnings:
drivers/memory/tegra/mc.c:593 tegra30_mc_handle_irq() error: uninitialized symbol 'channel'.
vim +/channel +593 drivers/memory/tegra/mc.c
cc84c62c96f257 Ashish Mhetre 2022-03-02 516
cc84c62c96f257 Ashish Mhetre 2022-03-02 517 irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
89184651631713 Thierry Reding 2014-04-16 518 {
89184651631713 Thierry Reding 2014-04-16 519 struct tegra_mc *mc = data;
1c74d5c0de0c2c Dmitry Osipenko 2018-04-09 520 unsigned long status;
89184651631713 Thierry Reding 2014-04-16 521 unsigned int bit;
cc84c62c96f257 Ashish Mhetre 2022-03-02 522 int channel;
cc84c62c96f257 Ashish Mhetre 2022-03-02 523
cc84c62c96f257 Ashish Mhetre 2022-03-02 524 if (mc->soc->num_channels && mc->soc->get_int_channel) {
Let's assume "mc->soc->num_channels" is non-zero but there is no
mc->soc->get_int_channel() function.
cc84c62c96f257 Ashish Mhetre 2022-03-02 525 int err;
cc84c62c96f257 Ashish Mhetre 2022-03-02 526
cc84c62c96f257 Ashish Mhetre 2022-03-02 527 err = mc->soc->get_int_channel(mc, &channel);
cc84c62c96f257 Ashish Mhetre 2022-03-02 528 if (err < 0)
cc84c62c96f257 Ashish Mhetre 2022-03-02 529 return IRQ_NONE;
89184651631713 Thierry Reding 2014-04-16 530
89184651631713 Thierry Reding 2014-04-16 531 /* mask all interrupts to avoid flooding */
cc84c62c96f257 Ashish Mhetre 2022-03-02 532 status = mc_ch_readl(mc, channel, MC_INTSTATUS) & mc->soc->intmask;
cc84c62c96f257 Ashish Mhetre 2022-03-02 533 } else {
1c74d5c0de0c2c Dmitry Osipenko 2018-04-09 534 status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
cc84c62c96f257 Ashish Mhetre 2022-03-02 535 }
cc84c62c96f257 Ashish Mhetre 2022-03-02 536
bf3fbdfbec947c Dmitry Osipenko 2018-04-09 537 if (!status)
bf3fbdfbec947c Dmitry Osipenko 2018-04-09 538 return IRQ_NONE;
89184651631713 Thierry Reding 2014-04-16 539
89184651631713 Thierry Reding 2014-04-16 540 for_each_set_bit(bit, &status, 32) {
1079a66bc32ff0 Thierry Reding 2021-06-02 541 const char *error = tegra_mc_status_names[bit] ?: "unknown";
89184651631713 Thierry Reding 2014-04-16 542 const char *client = "unknown", *desc;
89184651631713 Thierry Reding 2014-04-16 543 const char *direction, *secure;
cc84c62c96f257 Ashish Mhetre 2022-03-02 544 u32 status_reg, addr_reg;
cc84c62c96f257 Ashish Mhetre 2022-03-02 545 u32 intmask = BIT(bit);
89184651631713 Thierry Reding 2014-04-16 546 phys_addr_t addr = 0;
cc84c62c96f257 Ashish Mhetre 2022-03-02 547 #ifdef CONFIG_PHYS_ADDR_T_64BIT
cc84c62c96f257 Ashish Mhetre 2022-03-02 548 u32 addr_hi_reg = 0;
cc84c62c96f257 Ashish Mhetre 2022-03-02 549 #endif
89184651631713 Thierry Reding 2014-04-16 550 unsigned int i;
89184651631713 Thierry Reding 2014-04-16 551 char perm[7];
89184651631713 Thierry Reding 2014-04-16 552 u8 id, type;
89184651631713 Thierry Reding 2014-04-16 553 u32 value;
89184651631713 Thierry Reding 2014-04-16 554
cc84c62c96f257 Ashish Mhetre 2022-03-02 555 switch (intmask) {
cc84c62c96f257 Ashish Mhetre 2022-03-02 556 case MC_INT_DECERR_VPR:
cc84c62c96f257 Ashish Mhetre 2022-03-02 557 status_reg = MC_ERR_VPR_STATUS;
cc84c62c96f257 Ashish Mhetre 2022-03-02 558 addr_reg = MC_ERR_VPR_ADR;
cc84c62c96f257 Ashish Mhetre 2022-03-02 559 break;
cc84c62c96f257 Ashish Mhetre 2022-03-02 560
cc84c62c96f257 Ashish Mhetre 2022-03-02 561 case MC_INT_SECERR_SEC:
cc84c62c96f257 Ashish Mhetre 2022-03-02 562 status_reg = MC_ERR_SEC_STATUS;
cc84c62c96f257 Ashish Mhetre 2022-03-02 563 addr_reg = MC_ERR_SEC_ADR;
cc84c62c96f257 Ashish Mhetre 2022-03-02 564 break;
cc84c62c96f257 Ashish Mhetre 2022-03-02 565
cc84c62c96f257 Ashish Mhetre 2022-03-02 566 case MC_INT_DECERR_MTS:
cc84c62c96f257 Ashish Mhetre 2022-03-02 567 status_reg = MC_ERR_MTS_STATUS;
cc84c62c96f257 Ashish Mhetre 2022-03-02 568 addr_reg = MC_ERR_MTS_ADR;
cc84c62c96f257 Ashish Mhetre 2022-03-02 569 break;
cc84c62c96f257 Ashish Mhetre 2022-03-02 570
cc84c62c96f257 Ashish Mhetre 2022-03-02 571 case MC_INT_DECERR_GENERALIZED_CARVEOUT:
cc84c62c96f257 Ashish Mhetre 2022-03-02 572 status_reg = MC_ERR_GENERALIZED_CARVEOUT_STATUS;
cc84c62c96f257 Ashish Mhetre 2022-03-02 573 addr_reg = MC_ERR_GENERALIZED_CARVEOUT_ADR;
cc84c62c96f257 Ashish Mhetre 2022-03-02 574 break;
cc84c62c96f257 Ashish Mhetre 2022-03-02 575
cc84c62c96f257 Ashish Mhetre 2022-03-02 576 case MC_INT_DECERR_ROUTE_SANITY:
cc84c62c96f257 Ashish Mhetre 2022-03-02 577 status_reg = MC_ERR_ROUTE_SANITY_STATUS;
cc84c62c96f257 Ashish Mhetre 2022-03-02 578 addr_reg = MC_ERR_ROUTE_SANITY_ADR;
cc84c62c96f257 Ashish Mhetre 2022-03-02 579 break;
cc84c62c96f257 Ashish Mhetre 2022-03-02 580
cc84c62c96f257 Ashish Mhetre 2022-03-02 581 default:
cc84c62c96f257 Ashish Mhetre 2022-03-02 582 status_reg = MC_ERR_STATUS;
cc84c62c96f257 Ashish Mhetre 2022-03-02 583 addr_reg = MC_ERR_ADR;
cc84c62c96f257 Ashish Mhetre 2022-03-02 584
cc84c62c96f257 Ashish Mhetre 2022-03-02 585 #ifdef CONFIG_PHYS_ADDR_T_64BIT
cc84c62c96f257 Ashish Mhetre 2022-03-02 586 if (mc->soc->has_addr_hi_reg)
cc84c62c96f257 Ashish Mhetre 2022-03-02 587 addr_hi_reg = MC_ERR_ADR_HI;
cc84c62c96f257 Ashish Mhetre 2022-03-02 588 #endif
cc84c62c96f257 Ashish Mhetre 2022-03-02 589 break;
cc84c62c96f257 Ashish Mhetre 2022-03-02 590 }
cc84c62c96f257 Ashish Mhetre 2022-03-02 591
cc84c62c96f257 Ashish Mhetre 2022-03-02 592 if (mc->soc->num_channels)
cc84c62c96f257 Ashish Mhetre 2022-03-02 @593 value = mc_ch_readl(mc, channel, status_reg);
Then "channel" is uninitialized here.
cc84c62c96f257 Ashish Mhetre 2022-03-02 594 else
cc84c62c96f257 Ashish Mhetre 2022-03-02 595 value = mc_readl(mc, status_reg);
89184651631713 Thierry Reding 2014-04-16 596
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Thursday, March 3, 2022 6:33 PM
> To: Dan Carpenter <[email protected]>; [email protected]; Ashish
> Mhetre <[email protected]>; [email protected];
> [email protected]; Jonathan Hunter <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; Krishna Reddy
> <[email protected]>; Sachin Nikam <[email protected]>
> Subject: Re: [Patch v4 4/4] memory: tegra: Add MC error logging on tegra186
> onward
>
> External email: Use caution opening links or attachments
>
>
> On 03/03/2022 13:31, Dan Carpenter wrote:
> > Hi Ashish,
> >
> > url:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2F0day-ci%2Flinux%2Fcommits%2FAshish-Mhetre%2Fmemory-tegra-
> Add-MC-channels-and-error-logging%2F20220302-
> 164625&data=04%7C01%7Camhetre%40nvidia.com%7C448e9570ac274b
> 7ed2f408d9fd162da7%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C
> 637819094016979779%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=
> dzcWTAMPikKWLFc4mkD%2FJPWQckiYrUzI9OOEEGvvDAA%3D&reserved
> =0
> > base:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftegra%2Flinux.git&dat
> a=04%7C01%7Camhetre%40nvidia.com%7C448e9570ac274b7ed2f408d9fd162
> da7%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C63781909401697
> 9779%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=pbpW4cT7C%2Fa
> qP8FJClKKdG4NdXpEGh0yBZPPk%2FeCSvU%3D&reserved=0 for-next
> > config: openrisc-randconfig-m031-20220302
> > (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdow
> > nload.01.org%2F0day-
> ci%2Farchive%2F20220303%2F202203031247.0bBX70B3-lk
> >
> p%40intel.com%2Fconfig&data=04%7C01%7Camhetre%40nvidia.com%7C
> 448e9
> >
> 570ac274b7ed2f408d9fd162da7%7C43083d15727340c1b7db39efd9ccc17a%7C
> 0%7C0
> >
> %7C637819094016979779%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQ
> >
> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=TO1bX5
> %2FM
> > PhUpf%2BnwSuHkB%2ByLEe4Mdn6Or%2BiZUrbeHpY%3D&reserved=0)
> > compiler: or1k-linux-gcc (GCC) 11.2.0
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <[email protected]>
> > Reported-by: Dan Carpenter <[email protected]>
> >
> > New smatch warnings:
> > drivers/memory/tegra/mc.c:593 tegra30_mc_handle_irq() error: uninitialized
> symbol 'channel'.
>
> Ashish,
>
> I mentioned with your v3 that it is expected for submitter to run certain
> automatic tools:
> "We not only expect to compile it but also compile with W=1, run sparse,
> smatch and coccicheck. Then also test."
>
> Judging by the output here, it could be that either you used old compiler or did
> not run the checks.
>
> Can you please confirm that you performed all the activities mentioned before?
>
Hi Krzysztof,
I had tested the code and verified that MC errors are getting logged as expected
with my changes.
I had also compiled kernel with W=1, ran sparse and made sure that there
aren't any warnings/errors with my changes.
However, I didn't run smatch and coccicheck because I was facing difficulties
in setting up these tools.
I'll make sure that patches in future are scrutinized by all of these tools.
> Best regards,
> Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Thursday, March 3, 2022 1:14 AM
> To: Ashish Mhetre <[email protected]>; [email protected];
> [email protected]; Jonathan Hunter <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]
> Cc: Krishna Reddy <[email protected]>; Sachin Nikam
> <[email protected]>
> Subject: Re: [Patch v4 4/4] memory: tegra: Add MC error logging on tegra186
> onward
>
> External email: Use caution opening links or attachments
>
>
> On 02/03/2022 09:43, Ashish Mhetre wrote:
> > Add new function 'get_int_channel' in tegra_mc_soc struture which is
> > implemented by tegra SOCs which support multiple MC channels. This
> > function returns the channel which should be used to get the
> > information of interrupts.
> > Remove static from tegra30_mc_handle_irq and use it as interrupt
> > handler for MC interrupts on tegra186, tegra194 and tegra234 to log the
> errors.
> > Add error specific MC status and address register bits and use them on
> > tegra186, tegra194 and tegra234.
> > Add error logging for generalized carveout interrupt on tegra186,
> > tegra194 and tegra234.
> > Add error logging for route sanity interrupt on tegra194 an tegra234.
> > Add register for higher bits of error address which is available on
> > tegra194 and tegra234.
> > Add a boolean variable 'has_addr_hi_reg' in tegra_mc_soc struture
> > which will be true if soc has register for higher bits of memory
> > controller error address. Set it true for tegra194 and tegra234.
> >
> > Signed-off-by: Ashish Mhetre <[email protected]>
> > ---
> > drivers/memory/tegra/mc.c | 102
> ++++++++++++++++++++++++++++++++++------
> > drivers/memory/tegra/mc.h | 37 ++++++++++++++-
> > drivers/memory/tegra/tegra186.c | 45 ++++++++++++++++++
> > drivers/memory/tegra/tegra194.c | 44 +++++++++++++++++
> > drivers/memory/tegra/tegra234.c | 59 +++++++++++++++++++++++
> > include/soc/tegra/mc.h | 4 ++
> > 6 files changed, 275 insertions(+), 16 deletions(-)
> >
>
> (...)
>
> >
> > +static int tegra186_mc_get_channel(struct tegra_mc *mc, int
> > +*mc_channel) {
> > + u32 g_intstatus;
> > +
> > + g_intstatus = mc_ch_readl(mc, MC_BROADCAST_CHANNEL,
> > + MC_GLOBAL_INTSTATUS);
> > +
> > + switch (g_intstatus & mc->soc->int_channel_mask) {
> > + case BIT(0):
> > + *mc_channel = 0;
> > + break;
> > +
> > + case BIT(1):
> > + *mc_channel = 1;
> > + break;
> > +
> > + case BIT(2):
> > + *mc_channel = 2;
> > + break;
> > +
> > + case BIT(3):
> > + *mc_channel = 3;
> > + break;
> > +
> > + case BIT(24):
> > + *mc_channel = MC_BROADCAST_CHANNEL;
> > + break;
> > +
> > + default:
> > + pr_err("Unknown interrupt source\n");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > const struct tegra_mc_soc tegra186_mc_soc = {
> > .num_clients = ARRAY_SIZE(tegra186_mc_clients),
> > .clients = tegra186_mc_clients,
> > .num_address_bits = 40,
> > .num_channels = 4,
> > + .client_id_mask = 0xff,
> > + .intmask = 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,
> > + .int_channel_mask = 0x100000f,
> > + .get_int_channel = tegra186_mc_get_channel,
> > };
> > #endif
> > diff --git a/drivers/memory/tegra/tegra194.c
> > b/drivers/memory/tegra/tegra194.c index 9400117..bc16567 100644
> > --- a/drivers/memory/tegra/tegra194.c
> > +++ b/drivers/memory/tegra/tegra194.c
> > @@ -1343,10 +1343,54 @@ static const struct tegra_mc_client
> tegra194_mc_clients[] = {
> > },
> > };
> >
> > +static int tegra194_mc_get_channel(struct tegra_mc *mc, int
> > +*mc_channel)
>
> Looks like 'mc' could be a pointer to const.
>
> > +{
> > + u32 g_intstatus;
>
> Variable name just "status" because it looks like some hungarian-notation-
> style...
>
> The same in other places like this.
>
Okay, I will update this in next version.
>
> Best regards,
> Krzysztof
On 02/03/2022 19:35, Krzysztof Kozlowski wrote:
> On 02/03/2022 09:43, Ashish Mhetre wrote:
>> From tegra186 onwards, memory controller support multiple channels.
>> Add support for mapping address spaces of these channels.
>> During error interrupts from memory controller, appropriate registers
>> from these channels need to be accessed for logging error info.
>>
>> Signed-off-by: Ashish Mhetre <[email protected]>
>> ---
>> drivers/memory/tegra/mc.c | 6 ++++++
>> drivers/memory/tegra/tegra186.c | 21 +++++++++++++++++++++
>> drivers/memory/tegra/tegra194.c | 1 +
>> drivers/memory/tegra/tegra234.c | 1 +
>> include/soc/tegra/mc.h | 7 +++++++
>> 5 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index bf3abb6..3cda1d9 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)
>> if (IS_ERR(mc->regs))
>> return PTR_ERR(mc->regs);
>>
>> + if (mc->soc->ops && mc->soc->ops->map_regs) {
>> + err = mc->soc->ops->map_regs(mc, pdev);
>> + if (err < 0)
>> + return err;
>> + }
>> +
>> mc->debugfs.root = debugfs_create_dir("mc", NULL);
>>
>> if (mc->soc->ops && mc->soc->ops->probe) {
>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>> index 3d15388..59a4425 100644
>> --- a/drivers/memory/tegra/tegra186.c
>> +++ b/drivers/memory/tegra/tegra186.c
>> @@ -139,11 +139,31 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)
>> return 0;
>> }
>>
>> +static int tegra186_mc_map_regs(struct tegra_mc *mc,
>> + struct platform_device *pdev)
>> +{
>> + struct resource *res;
>> + int i;
>> +
>> + mc->mcb_regs = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
>> + if (IS_ERR(mc->mcb_regs))
>> + return PTR_ERR(mc->mcb_regs);
>> +
>> + for (i = 0; i < mc->soc->num_channels; i++) {
>> + mc->mc_regs[i] = devm_platform_get_and_ioremap_resource(pdev, i + 2, &res);
>> + if (IS_ERR(mc->mc_regs[i]))
>> + return PTR_ERR(mc->mc_regs[i]);
>
> This breaks the ABI, so I need Thierry's ack that such ABI break is
> perfectly ok.
We should not break the DT ABI and so if all the reg entries it should
still be able to work.
Jon
--
nvpublic