Re: [PATCH v6 4/4] memory: tegra: Add MC error logging support for Tegra264

[Krzysztof Kozlowski]

On 30/01/2026 18:30, Ketan Patil wrote:
> In Tegra264, different components from memory subsystems like Memory
> Controller Fabric (MCF), HUB, HUB Common (HUBC), Side Band Shim (SBS)
> and channels have different interrupt lines for receiving memory
> controller error interrupts.
> 
> Add support for logging memory controller errors on Tegra264.
> - Add MC error handling flow for MCF, HUB, HUBC, SBS and channels.
> - Each of these components have different interrupt lines for MC error.
> - Register interrupt handlers for interrupts from these different MC
> components.
> - There is no global interrupt status register in Tegra264 unlike older
> Tegra chips.
> - There are common interrupt status registers in case of MCF, HUB, HUBC
> from which figure out the slice number or hub number responsible for
> generating interrupt and then read interrupt status register to find out
> type of violation.
> - Introduce new SoC specific fields in tegra_mc_soc like interrupt mask
> values for MCF, HUB, HUBC etc., which are SoC specific.
> 
> Signed-off-by: Ketan Patil <ketanp@xxxxxxxxxx>
> ---
>  drivers/memory/tegra/mc.c       |  35 +--
>  drivers/memory/tegra/mc.h       |  83 ++++++-
>  drivers/memory/tegra/tegra114.c |  13 +-
>  drivers/memory/tegra/tegra124.c |  32 ++-
>  drivers/memory/tegra/tegra186.c |  24 +-
>  drivers/memory/tegra/tegra194.c |  17 +-
>  drivers/memory/tegra/tegra20.c  |  23 +-
>  drivers/memory/tegra/tegra210.c |  16 +-
>  drivers/memory/tegra/tegra234.c |  17 +-
>  drivers/memory/tegra/tegra264.c | 428 +++++++++++++++++++++++++++++++-
>  drivers/memory/tegra/tegra30.c  |  13 +-
>  include/soc/tegra/mc.h          |  10 +-
>  12 files changed, 648 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index 49c470f7b1f7..a102a8ea7926 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -597,16 +597,16 @@ irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
>  		}
>  
>  		/* mask all interrupts to avoid flooding */
> -		status = mc_ch_readl(mc, channel, MC_INTSTATUS) & mc->soc->intmask;
> +		status = mc_ch_readl(mc, channel, MC_INTSTATUS) & mc->soc->intmasks[0].mask;
>  	} else {
> -		status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmask;
> +		status = mc_readl(mc, MC_INTSTATUS) & mc->soc->intmasks[0].mask;
>  	}
>  
>  	if (!status)
>  		return IRQ_NONE;
>  
>  	for_each_set_bit(bit, &status, 32) {
> -		const char *error = tegra_mc_status_names[bit] ?: "unknown";
> +		const char *error = tegra20_mc_status_names[bit] ?: "unknown";
>  		const char *client = "unknown", *desc;
>  		const char *direction, *secure;
>  		u32 status_reg, addr_reg;
> @@ -669,9 +669,11 @@ irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
>  					addr = mc_ch_readl(mc, channel, addr_hi_reg);
>  				else
>  					addr = mc_readl(mc, addr_hi_reg);
> -			} else {
> +			} else if (mc->soc->mc_addr_hi_mask) {
>  				addr = ((value >> MC_ERR_STATUS_ADR_HI_SHIFT) &
> -					MC_ERR_STATUS_ADR_HI_MASK);
> +					mc->soc->mc_addr_hi_mask);
> +			} else {
> +				WARN_ON(1);

You should handle it correctly instead. WARN reboots some systems and
this does not look like really impossible path, which would justify it.

>  			}
>  			addr <<= 32;
>  		}
> @@ -696,11 +698,11 @@ irqreturn_t tegra30_mc_handle_irq(int irq, void *data)
>  			}
>  		}
>  

...

> +
> +#define ERR_GENERALIZED_APERTURE_ID_SHIFT		0
> +#define ERR_GENERALIZED_APERTURE_ID_MASK		0x1F
> +#define ERR_GENERALIZED_CARVEOUT_APERTURE_ID_SHIFT	5
> +#define ERR_GENERALIZED_CARVEOUT_APERTURE_ID_MASK	0x1F
> +
>  static inline u32 tegra_mc_scale_percents(u64 val, unsigned int percents)
>  {
>  	val = val * percents;
> @@ -187,15 +255,18 @@ extern const struct tegra_mc_ops tegra30_mc_ops;
>  
>  #if defined(CONFIG_ARCH_TEGRA_186_SOC) || \
>      defined(CONFIG_ARCH_TEGRA_194_SOC) || \
> -    defined(CONFIG_ARCH_TEGRA_234_SOC) || \
> -    defined(CONFIG_ARCH_TEGRA_264_SOC)
> +    defined(CONFIG_ARCH_TEGRA_234_SOC)
>  extern const struct tegra_mc_ops tegra186_mc_ops;
>  #endif
>  
>  irqreturn_t tegra30_mc_handle_irq(int irq, void *data);
>  extern const irq_handler_t tegra30_mc_irq_handlers[];
> -extern const char * const tegra_mc_status_names[32];
> -extern const char * const tegra_mc_error_names[8];
> +extern const char * const tegra20_mc_status_names[32];
> +extern const char * const tegra20_mc_error_names[8];
> +int tegra186_mc_probe(struct tegra_mc *mc);
> +int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev);

This is such a confusing design... Function declared in mc.h header,
defined in a MODULE but used in a driver which can only be built in,
because it depends on some ARCH_foo stuf. If I deciphered it correctly
this fails and needs EXPORT_SYMBOL_GPL, but maybe I deciphered it wrong,
because code is confusing. Your drivers Makefile should not have
ARCH_TEGRA_FOO, but dedicated entries so the state of each driver is
obvious by reading Kconfig of this directory - it is built-in or tristate.


> +int tegra186_mc_resume(struct tegra_mc *mc);
> +void tegra186_mc_remove(struct tegra_mc *mc);
>  

_mc {


Best regards,
Krzysztof