RE: [Patch v4 4/4] memory: tegra: Add MC error logging on tegra186 onward

[Ashish Mhetre]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxx>
> Sent: Thursday, March 3, 2022 1:14 AM
> To: Ashish Mhetre <amhetre@xxxxxxxxxx>; robh+dt@xxxxxxxxxx;
> thierry.reding@xxxxxxxxx; Jonathan Hunter <jonathanh@xxxxxxxxxx>;
> digetx@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-tegra@xxxxxxxxxxxxxxx
> Cc: Krishna Reddy <vdumpa@xxxxxxxxxx>; Sachin Nikam
> <Snikam@xxxxxxxxxx>
> 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 <amhetre@xxxxxxxxxx>
> > ---
> >  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