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.