Re: [PATCH v1] memory: tegra20: Add debug statistics

[Dmitry Osipenko <digetx@gmail.com>]

18.03.2021 18:23, Krzysztof Kozlowski пишет:
...
>> +	mc->debugfs.root = debugfs_create_dir("mc", NULL);
>> +	if (!mc->debugfs.root)
>> +		dev_err(&pdev->dev, "failed to create debugfs directory\n");
> 
> It's error pointer, not null, but anyway there is no need for handling
> debugfs error. See Greg KH's commits like "remove pointless check for
> debugfs_create_dir()".

Indeed!

>> +
>> +	if (mc->soc->init) {
>> +		err = mc->soc->init(mc);
>> +		if (err < 0)
>> +			dev_err(&pdev->dev,
>> +				"failed to register initialize SoC driver: %d\n",
> 
> "failed to initialize SoC driver:...."

Good catch!

>> +				err);
>> +	}
>> +
>>  	err = tegra_mc_reset_setup(mc);
>>  	if (err < 0)
>>  		dev_err(&pdev->dev, "failed to register reset controller: %d\n",
>> diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c
>> index 29ecf02805a0..513c07104296 100644
>> --- a/drivers/memory/tegra/tegra20.c
>> +++ b/drivers/memory/tegra/tegra20.c
>> @@ -3,6 +3,8 @@
>>   * Copyright (C) 2012 NVIDIA CORPORATION.  All rights reserved.
>>   */
>>  
>> +#include <linux/bitfield.h>
>> +#include <linux/delay.h>
>>  #include <linux/of_device.h>
>>  #include <linux/slab.h>
>>  #include <linux/string.h>
>> @@ -11,6 +13,77 @@
>>  
>>  #include "mc.h"
>>  
>> +#define MC_STAT_CONTROL				0x90
>> +#define MC_STAT_EMC_CLOCK_LIMIT			0xa0
>> +#define MC_STAT_EMC_CLOCKS			0xa4
>> +#define MC_STAT_EMC_CONTROL_0			0xa8
>> +#define MC_STAT_EMC_CONTROL_1			0xac
>> +#define MC_STAT_EMC_COUNT_0			0xb8
>> +#define MC_STAT_EMC_COUNT_1			0xbc
>> +
>> +#define MC_STAT_CONTROL_CLIENT_ID		GENMASK(13,  8)
>> +#define MC_STAT_CONTROL_EVENT			GENMASK(23, 16)
>> +#define MC_STAT_CONTROL_PRI_EVENT		GENMASK(25, 24)
>> +#define MC_STAT_CONTROL_FILTER_CLIENT_ENABLE	GENMASK(26, 26)
>> +#define MC_STAT_CONTROL_FILTER_PRI		GENMASK(29, 28)
>> +
>> +#define MC_STAT_CONTROL_PRI_EVENT_HP		0
>> +#define MC_STAT_CONTROL_PRI_EVENT_TM		1
>> +#define MC_STAT_CONTROL_PRI_EVENT_BW		2
>> +
>> +#define MC_STAT_CONTROL_FILTER_PRI_DISABLE	0
>> +#define MC_STAT_CONTROL_FILTER_PRI_NO		1
>> +#define MC_STAT_CONTROL_FILTER_PRI_YES		2
>> +
>> +#define MC_STAT_CONTROL_EVENT_QUALIFIED		0
>> +#define MC_STAT_CONTROL_EVENT_ANY_READ		1
>> +#define MC_STAT_CONTROL_EVENT_ANY_WRITE		2
>> +#define MC_STAT_CONTROL_EVENT_RD_WR_CHANGE	3
>> +#define MC_STAT_CONTROL_EVENT_SUCCESSIVE	4
>> +#define MC_STAT_CONTROL_EVENT_ARB_BANK_AA	5
>> +#define MC_STAT_CONTROL_EVENT_ARB_BANK_BB	6
>> +#define MC_STAT_CONTROL_EVENT_PAGE_MISS		7
>> +#define MC_STAT_CONTROL_EVENT_AUTO_PRECHARGE	8
>> +
>> +#define EMC_GATHER_RST				(0 << 8)
>> +#define EMC_GATHER_CLEAR			(1 << 8)
>> +#define EMC_GATHER_DISABLE			(2 << 8)
>> +#define EMC_GATHER_ENABLE			(3 << 8)
>> +
>> +#define MC_STAT_SAMPLE_TIME_USEC		16000
>> +
>> +/* we store collected statistics as a fixed point values */
>> +#define MC_FX_FRAC_SCALE			100
>> +
>> +struct tegra20_mc_stat_gather {
>> +	unsigned int pri_filter;
>> +	unsigned int pri_event;
>> +	unsigned int result;
>> +	unsigned int client;
>> +	unsigned int event;
>> +	bool client_enb;
>> +};
>> +
>> +struct tegra20_mc_stat {
>> +	struct tegra20_mc_stat_gather gather0;
>> +	struct tegra20_mc_stat_gather gather1;
>> +	unsigned int sample_time_usec;
>> +	struct tegra_mc *mc;
>> +};
>> +
>> +struct tegra20_mc_client_stat {
>> +	unsigned int events;
>> +	unsigned int arb_high_prio;
>> +	unsigned int arb_timeout;
>> +	unsigned int arb_bandwidth;
>> +	unsigned int rd_wr_change;
>> +	unsigned int successive;
>> +	unsigned int page_miss;
>> +	unsigned int auto_precharge;
>> +	unsigned int arb_bank_aa;
>> +	unsigned int arb_bank_bb;
>> +};
>> +
>>  static const struct tegra_mc_client tegra20_mc_clients[] = {
>>  	{
>>  		.id = 0x00,
>> @@ -356,6 +429,274 @@ static const struct tegra_mc_icc_ops tegra20_mc_icc_ops = {
>>  	.set = tegra20_mc_icc_set,
>>  };
>>  
>> +static u32 tegra20_mc_stat_gather_control(struct tegra20_mc_stat_gather *g)
> 
> "g" looks like pointer to const here

I'll add couple more consts to the code for consistency.

>> +{
>> +	u32 control;
>> +
>> +	control  = FIELD_PREP(MC_STAT_CONTROL_EVENT, g->event);
>> +	control |= FIELD_PREP(MC_STAT_CONTROL_CLIENT_ID, g->client);
>> +	control |= FIELD_PREP(MC_STAT_CONTROL_PRI_EVENT, g->pri_event);
>> +	control |= FIELD_PREP(MC_STAT_CONTROL_FILTER_PRI, g->pri_filter);
>> +	control |= FIELD_PREP(MC_STAT_CONTROL_FILTER_CLIENT_ENABLE, g->client_enb);
>> +
>> +	return control;
>> +}
...
>> +static void tegra20_mc_collect_stats(struct tegra_mc *mc,
>> +				     struct tegra20_mc_client_stat *stats)
>> +{
>> +	const struct tegra_mc_client *client0, *client1;
>> +	unsigned int i, clienta, clientb;
> 
> Define the clienta and clientb inside the loop, to reduce the scope.
> Unless it is used between the loop iterations?

okay

>> +
>> +	/* collect memory controller utilization percent for each client */
>> +	for (i = 0; i < mc->soc->num_clients; i += 2) {
>> +		client0 = &mc->soc->clients[i];
>> +		client1 = &mc->soc->clients[i + 1];
>> +
>> +		if (i + 1 == mc->soc->num_clients)
>> +			client1 = NULL;
>> +
>> +		tegra20_mc_stat_events(mc, client0, client1,
>> +				       MC_STAT_CONTROL_FILTER_PRI_DISABLE,
>> +				       MC_STAT_CONTROL_PRI_EVENT_HP,
>> +				       MC_STAT_CONTROL_EVENT_QUALIFIED,
>> +				       &stats[i + 0].events,
>> +				       &stats[i + 1].events);
>> +	}
>> +
>> +	/* collect more info from active clients */
>> +	for (i = 0; i < mc->soc->num_clients; i++) {
>> +		clientb = mc->soc->num_clients;
>> +
>> +		for (client0 = NULL; i < mc->soc->num_clients; i++) {
>> +			if (stats[i].events) {
>> +				client0 = &mc->soc->clients[i];
>> +				clienta = i++;
>> +				break;
>> +			}
>> +		}
> 
> Could all clients have 0 events ending with "clienta" being undefined?
> "client0" would be non-NULL because of loop before.

As per 6.8.5.3 of C99 standard, the declaration of a for-loop "is
reached in the order of execution before the first evaluation of the
controlling expression".

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

>> +
>> +		for (client1 = NULL; i < mc->soc->num_clients; i++) {
>> +			if (stats[i].events) {
>> +				client1 = &mc->soc->clients[i];
>> +				clientb = i;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (!client0 && !client1)
>> +			break;

this means that both client0 and client1 are set t0 NULL here if all
clients have 0 events.