Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <72fafc84-4986-926f-67ae-155f4fced88b@canonical.com>
Date:   Wed, 2 Mar 2022 20:44:21 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [Patch v4 4/4] memory: tegra: Add MC error logging on tegra186
 onward
Content-Language: en-US
To:     Ashish Mhetre <amhetre@nvidia.com>, robh+dt@kernel.org,
        thierry.reding@gmail.com, jonathanh@nvidia.com, digetx@gmail.com,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-tegra@vger.kernel.org
Cc:     vdumpa@nvidia.com, Snikam@nvidia.com
References: <1646210609-21943-1-git-send-email-amhetre@nvidia.com>
 <1646210609-21943-5-git-send-email-amhetre@nvidia.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
In-Reply-To: <1646210609-21943-5-git-send-email-amhetre@nvidia.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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


Best regards,
Krzysztof