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

[Ketan Patil]

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@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.
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