Messages in this thread | | | Date | Fri, 21 Jan 2022 11:20:21 +0530 | Subject | Re: [Patch V2] memory: tegra: Add MC error logging on tegra186 onward | From | Ashish Mhetre <> |
| |
On 1/20/2022 6:23 PM, Dmitry Osipenko wrote: > External email: Use caution opening links or attachments > > > 20.01.2022 12:02, Ashish Mhetre пишет: >> 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 and use it on tegra194 and >> tegra234. >> >> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com> >> --- >> 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 >> >> drivers/memory/tegra/mc.c | 73 ++++++++++++++++++++++++++++++++++------- >> drivers/memory/tegra/mc.h | 16 +++++++++ >> drivers/memory/tegra/tegra186.c | 7 ++++ >> drivers/memory/tegra/tegra194.c | 5 +++ >> drivers/memory/tegra/tegra234.c | 5 +++ >> 5 files changed, 94 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c >> index bf3abb6..badebe8 100644 >> --- a/drivers/memory/tegra/mc.c >> +++ b/drivers/memory/tegra/mc.c >> @@ -508,7 +508,21 @@ 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 >> + >> +#if defined(CONFIG_ARCH_TEGRA_3x_SOC) || \ >> + defined(CONFIG_ARCH_TEGRA_114_SOC) || \ >> + defined(CONFIG_ARCH_TEGRA_124_SOC) || \ >> + defined(CONFIG_ARCH_TEGRA_132_SOC) || \ >> + defined(CONFIG_ARCH_TEGRA_210_SOC) || \ >> + defined(CONFIG_ARCH_TEGRA_186_SOC) || \ >> + defined(CONFIG_ARCH_TEGRA_194_SOC) || \ >> + defined(CONFIG_ARCH_TEGRA_234_SOC) > > Ifdefs are unnecessary, please remove them. They are okay for > tegra30_mc_ops, which is known to be used only by specific older SoC > versions, not okay in case of newer SoCs. > Okay, I'll remove these ifdefs in next version.
>> +irqreturn_t tegra30_mc_handle_irq(int irq, void *data) >> { >> struct tegra_mc *mc = data; >> unsigned long status; >> @@ -521,23 +535,64 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data) >> >> for_each_set_bit(bit, &status, 32) { >> const char *error = tegra_mc_status_names[bit] ?: "unknown"; >> + u32 status_reg = MC_ERR_STATUS, addr_reg = MC_ERR_ADR; >> const char *client = "unknown", *desc; >> const char *direction, *secure; >> phys_addr_t addr = 0; >> + u32 addr_hi_reg = 0; >> unsigned int i; >> char perm[7]; >> u8 id, type; >> u32 value; >> >> - value = mc_readl(mc, MC_ERR_STATUS); >> +#if defined(CONFIG_ARCH_TEGRA_186_SOC) || \ >> + defined(CONFIG_ARCH_TEGRA_194_SOC) || \ >> + defined(CONFIG_ARCH_TEGRA_234_SOC) > > Please drop these ifdefs. > Yes, will do in next version. >> + switch (bit) { >> + case MC_INT_DECERR_VPR: >> + status_reg = MC_ERR_VPR_STATUS; >> + addr_reg = MC_ERR_VPR_ADR; > > I mentioned previously that VPR is supported by T124+. Hence ifdefs are > incorrect. > Yes, I will remove them.
> ... >> + addr |= mc_readl(mc, addr_reg); >> >> if (value & MC_ERR_STATUS_RW) >> direction = "write"; >> @@ -591,9 +646,6 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data) >> break; >> } >> >> - value = mc_readl(mc, MC_ERR_ADR); > > Don't change the order of the code, just replace the MC_ERR_ADR here. > Okay, I'll keep the order as it is. I just thought that higher bits of address are read above, so why not read lower bits after that.
>> - addr |= value; >> - >> dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s%s)\n", >> client, secure, direction, &addr, error, >> desc, perm); >> @@ -604,11 +656,6 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data) >> >> 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] = { >> @@ -622,6 +669,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] = { >> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h >> index 062886e..9b1b0dc 100644 >> --- a/drivers/memory/tegra/mc.h >> +++ b/drivers/memory/tegra/mc.h >> @@ -44,6 +44,8 @@ >> #define MC_TIMING_CONTROL_DBG 0xf8 >> #define MC_TIMING_CONTROL 0xfc >> >> +#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) >> @@ -65,6 +67,18 @@ >> #define MC_ERR_STATUS_SECURITY BIT(17) >> #define MC_ERR_STATUS_RW BIT(16) >> >> +#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_ERR_ADR_HI 0x11fc > > Please put these regs right after the MC_TIMING_CONTROL. There is no > reason to separate them. > Sure, I'll update it in next version.
| |