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

[Ketan Patil <ketanp@nvidia.com>]

On 17/02/26 14:01, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> 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@nvidia.com>
>> ---
>>   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.
Yes, I am taking care of this in the next revision.
>
>>                        }
>>                        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.
I am re-working the series to avoid making these functions public.
>
>> +int tegra186_mc_resume(struct tegra_mc *mc);
>> +void tegra186_mc_remove(struct tegra_mc *mc);
>>
> _mc {
>
>
> Best regards,
> Krzysztof


Thanks