Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934279AbcCOHpV (ORCPT ); Tue, 15 Mar 2016 03:45:21 -0400 Received: from hqemgate14.nvidia.com ([216.228.121.143]:4058 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934112AbcCOHpP convert rfc822-to-8bit (ORCPT ); Tue, 15 Mar 2016 03:45:15 -0400 X-PGP-Universal: processed; by hqnvupgp08.nvidia.com on Tue, 15 Mar 2016 00:44:27 -0700 Message-ID: <56E7BD91.9040100@nvidia.com> Date: Tue, 15 Mar 2016 15:45:21 +0800 From: Wei Ni User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Eduardo Valentin CC: , , , , , , , Subject: Re: [PATCH V7 06/12] thermal: tegra: add a debugfs to show registers References: <1457665835-29880-1-git-send-email-wni@nvidia.com> <20160314190839.GB1872@localhost.localdomain> In-Reply-To: <20160314190839.GB1872@localhost.localdomain> X-Originating-IP: [10.19.224.146] X-ClientProxiedBy: DRBGMAIL102.nvidia.com (10.18.16.21) To HKMAIL101.nvidia.com (10.18.16.10) Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10504 Lines: 319 On 2016年03月15日 03:08, Eduardo Valentin wrote: > * PGP Signed by an unknown key > > On Fri, Mar 11, 2016 at 11:10:35AM +0800, Wei Ni wrote: >> Add a debugfs interface to show register contents for debug. >> >> Signed-off-by: Wei Ni >> --- >> drivers/thermal/tegra/soctherm.c | 143 ++++++++++++++++++++++++++++++++++++++- >> drivers/thermal/tegra/soctherm.h | 2 + >> 2 files changed, 142 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c >> index 52a33760e8e8..02ac6d2e5a20 100644 >> --- a/drivers/thermal/tegra/soctherm.c >> +++ b/drivers/thermal/tegra/soctherm.c >> @@ -15,6 +15,7 @@ >> * >> */ >> >> +#include >> #include >> #include >> #include >> @@ -33,14 +34,18 @@ >> >> #define SENSOR_CONFIG0 0 >> #define SENSOR_CONFIG0_STOP BIT(0) >> -#define SENSOR_CONFIG0_TALL_SHIFT 8 >> -#define SENSOR_CONFIG0_TCALC_OVER BIT(4) >> -#define SENSOR_CONFIG0_OVER BIT(3) >> #define SENSOR_CONFIG0_CPTR_OVER BIT(2) >> +#define SENSOR_CONFIG0_OVER BIT(3) >> +#define SENSOR_CONFIG0_TCALC_OVER BIT(4) >> +#define SENSOR_CONFIG0_TALL_MASK (0xfffff << 8) >> +#define SENSOR_CONFIG0_TALL_SHIFT 8 >> >> #define SENSOR_CONFIG1 4 >> +#define SENSOR_CONFIG1_TSAMPLE_MASK 0x3ff >> #define SENSOR_CONFIG1_TSAMPLE_SHIFT 0 >> +#define SENSOR_CONFIG1_TIDDQ_EN_MASK (0x3f << 15) >> #define SENSOR_CONFIG1_TIDDQ_EN_SHIFT 15 >> +#define SENSOR_CONFIG1_TEN_COUNT_MASK (0x3f << 24) >> #define SENSOR_CONFIG1_TEN_COUNT_SHIFT 24 >> #define SENSOR_CONFIG1_TEMP_ENABLE BIT(31) >> >> @@ -49,6 +54,14 @@ >> * because, it will be used by tegra_soctherm_fuse.c >> */ >> >> +#define SENSOR_STATUS0 0xc >> +#define SENSOR_STATUS0_VALID_MASK BIT(31) >> +#define SENSOR_STATUS0_CAPTURE_MASK 0xffff >> + >> +#define SENSOR_STATUS1 0x10 >> +#define SENSOR_STATUS1_TEMP_VALID_MASK BIT(31) >> +#define SENSOR_STATUS1_TEMP_MASK 0xffff >> + >> #define READBACK_VALUE_MASK 0xff00 >> #define READBACK_VALUE_SHIFT 8 >> #define READBACK_ADD_HALF BIT(7) >> @@ -73,6 +86,8 @@ struct tegra_soctherm { >> >> u32 *calib; >> struct tegra_soctherm_soc *soc; >> + >> + struct dentry *debugfs_dir; >> }; >> >> static int enable_tsensor(struct tegra_soctherm *tegra, >> @@ -140,6 +155,124 @@ static const struct thermal_zone_of_device_ops tegra_of_thermal_ops = { >> .get_temp = tegra_thermctl_get_temp, >> }; >> >> +#ifdef CONFIG_DEBUG_FS >> +static int regs_show(struct seq_file *s, void *data) >> +{ >> + struct platform_device *pdev = s->private; >> + struct tegra_soctherm *ts = platform_get_drvdata(pdev); >> + const struct tegra_tsensor *tsensors = ts->soc->tsensors; >> + u32 r, state; >> + int i; >> + >> + seq_puts(s, "-----TSENSE (convert HW)-----\n"); >> + >> + for (i = 0; i < ts->soc->num_tsensors; i++) { >> + r = readl(ts->regs + tsensors[i].base + SENSOR_CONFIG1); >> + state = REG_GET_MASK(r, SENSOR_CONFIG1_TEMP_ENABLE); >> + if (!state) >> + continue; >> + >> + seq_printf(s, "%s: ", tsensors[i].name); >> + >> + seq_printf(s, "En(%d) ", state); > > > Shouldnt the above if (!state) be place right here? Yes, it make sense, will do it. > > >> + state = REG_GET_MASK(r, SENSOR_CONFIG1_TIDDQ_EN_MASK); >> + seq_printf(s, "tiddq(%d) ", state); >> + state = REG_GET_MASK(r, SENSOR_CONFIG1_TEN_COUNT_MASK); >> + seq_printf(s, "ten_count(%d) ", state); >> + state = REG_GET_MASK(r, SENSOR_CONFIG1_TSAMPLE_MASK); >> + seq_printf(s, "tsample(%d) ", state + 1); >> + >> + r = readl(ts->regs + tsensors[i].base + SENSOR_STATUS1); >> + state = REG_GET_MASK(r, SENSOR_STATUS1_TEMP_VALID_MASK); >> + seq_printf(s, "Temp(%d/", state); >> + state = REG_GET_MASK(r, SENSOR_STATUS1_TEMP_MASK); >> + seq_printf(s, "%d) ", translate_temp(state)); >> + >> + r = readl(ts->regs + tsensors[i].base + SENSOR_STATUS0); >> + state = REG_GET_MASK(r, SENSOR_STATUS0_VALID_MASK); >> + seq_printf(s, "Capture(%d/", state); >> + state = REG_GET_MASK(r, SENSOR_STATUS0_CAPTURE_MASK); >> + seq_printf(s, "%d) ", state); >> + >> + r = readl(ts->regs + tsensors[i].base + SENSOR_CONFIG0); >> + state = REG_GET_MASK(r, SENSOR_CONFIG0_STOP); >> + seq_printf(s, "Stop(%d) ", state); >> + state = REG_GET_MASK(r, SENSOR_CONFIG0_TALL_MASK); >> + seq_printf(s, "Tall(%d) ", state); >> + state = REG_GET_MASK(r, SENSOR_CONFIG0_TCALC_OVER); >> + seq_printf(s, "Over(%d/", state); >> + state = REG_GET_MASK(r, SENSOR_CONFIG0_OVER); >> + seq_printf(s, "%d/", state); >> + state = REG_GET_MASK(r, SENSOR_CONFIG0_CPTR_OVER); >> + seq_printf(s, "%d) ", state); >> + >> + r = readl(ts->regs + tsensors[i].base + SENSOR_CONFIG2); >> + state = REG_GET_MASK(r, SENSOR_CONFIG2_THERMA_MASK); >> + seq_printf(s, "Therm_A/B(%d/", state); >> + state = REG_GET_MASK(r, SENSOR_CONFIG2_THERMB_MASK); >> + seq_printf(s, "%d)\n", (s16)state); >> + } >> + >> + r = readl(ts->regs + SENSOR_PDIV); >> + seq_printf(s, "PDIV: 0x%x\n", r); > > This is a matter of taste, but: > > Why here %x but all the above %d? This register value contain CPU/GPU/MEM/PLLX's pdiv values, and didn't parse them, so we need to use %x to print. For above messages, we doesn't dump registers simply, we parsed these Reg values, so we use %d for them, so that they are more readable. > > Reg dumps are typically %x. See above explanation, and for this debugfs, we called it as "reg_context", not "reg" simply. >> + >> + r = readl(ts->regs + SENSOR_HOTSPOT_OFF); >> + seq_printf(s, "HOTSPOT: 0x%x\n", r); >> + >> + seq_puts(s, "\n"); >> + seq_puts(s, "-----SOC_THERM-----\n"); >> + >> + r = readl(ts->regs + SENSOR_TEMP1); >> + state = REG_GET_MASK(r, SENSOR_TEMP1_CPU_TEMP_MASK); >> + seq_printf(s, "Temperatures: CPU(%d) ", translate_temp(state)); > > Maybe a line break before CPU, for consistency? -----TSENSE (convert HW)----- cpu0: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/27500) Capture(1/8483) Stop(0) Tall(16300) Over(1/1/1) Therm_A/B(1683/-1687) cpu1: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/26500) Capture(1/8665) Stop(0) Tall(16300) Over(1/1/1) Therm_A/B(1800/-1850) cpu2: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/29000) Capture(1/8395) Stop(0) Tall(16300) Over(1/1/1) Therm_A/B(1709/-1693) cpu3: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/27500) Capture(1/8413) Stop(0) Tall(16300) Over(1/1/1) Therm_A/B(1732/-1723) mem0: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/28000) Capture(1/8563) Stop(0) Tall(16300) Over(1/1/1) Therm_A/B(1670/-1689) mem1: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/26500) Capture(1/8700) Stop(0) Tall(16300) Over(1/1/1) Therm_A/B(1911/-1976) gpu: En(1) tiddq(1) ten_count(1) tsample(120) Temp(0/0) Capture(0/0) Stop(0) Tall(16300) Over(0/0/0) Therm_A/B(1714/-1730) pllx: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/28000) Capture(1/8477) Stop(0) Tall(16300) Over(1/1/1) Therm_A/B(1644/-1645) PDIV: 0x8888 HOTSPOT: 0xa0500 -----SOC_THERM----- Temperatures: CPU(29000) GPU(30500) PLLX(28000) MEM(28000) Thermtrip Any En(0) cpu En(1) Thresh(102500) gpu En(1) Thresh(103000) pll En(0) Thresh(105000) mem En(0) Thresh(103000) I paste this debufs output to here, it looks it is consistency :) > >> + state = REG_GET_MASK(r, SENSOR_TEMP1_GPU_TEMP_MASK); >> + seq_printf(s, " GPU(%d) ", translate_temp(state)); >> + r = readl(ts->regs + SENSOR_TEMP2); >> + state = REG_GET_MASK(r, SENSOR_TEMP2_PLLX_TEMP_MASK); >> + seq_printf(s, " PLLX(%d) ", translate_temp(state)); >> + state = REG_GET_MASK(r, SENSOR_TEMP2_MEM_TEMP_MASK); >> + seq_printf(s, " MEM(%d)\n", translate_temp(state)); >> + > > I got confused, I thought you would the above mapped as tsensors[i]. I > will check the code again. We have sensors of CPU0/1/2/3, MEM0/1, GPU and PLLX, and mapped as above tsensors[i]. Here are the group temperatures of CPU, MEM, GPU, PLLX. These groups aggregate the actual tsensors (count >= 1) and read out the max of all the sensors in a particular group as ‘group temperature’ in TEMP1/2. > > >> + return 0; >> +} >> + >> +static int regs_open(struct inode *inode, struct file *file) >> +{ >> + return single_open(file, regs_show, inode->i_private); >> +} >> + >> +static const struct file_operations regs_fops = { >> + .open = regs_open, >> + .read = seq_read, >> + .llseek = seq_lseek, >> + .release = single_release, >> +}; >> + >> +static void soctherm_debug_init(struct platform_device *pdev) >> +{ >> + struct tegra_soctherm *tegra = platform_get_drvdata(pdev); >> + struct dentry *root, *file; >> + >> + root = debugfs_create_dir("soctherm", NULL); >> + if (!root) { >> + dev_err(&pdev->dev, "failed to create debugfs directory\n"); >> + return; >> + } >> + >> + tegra->debugfs_dir = root; >> + >> + file = debugfs_create_file("reg_contents", 0644, root, >> + pdev, ®s_fops); >> + if (!file) >> + dev_err(&pdev->dev, "failed to create debugfs file\n"); > > Are we leaving the 'soctherm' dir left uncleaned in fail path here? I will fix it. > >> +} >> +#else >> +static inline void soctherm_debug_init(struct platform_device *pdev) >> +{ >> + return 0; > > I dont think we need this return for a void function. Yes, will fix it. > >> +} >> +#endif >> + >> static const struct of_device_id tegra_soctherm_of_match[] = { >> #ifdef CONFIG_ARCH_TEGRA_124_SOC >> { >> @@ -279,6 +412,8 @@ static int tegra_soctherm_probe(struct platform_device *pdev) >> } >> } >> >> + soctherm_debug_init(pdev); >> + >> return 0; >> >> disable_clocks: >> @@ -292,6 +427,8 @@ static int tegra_soctherm_remove(struct platform_device *pdev) >> { >> struct tegra_soctherm *tegra = platform_get_drvdata(pdev); >> >> + debugfs_remove_recursive(tegra->debugfs_dir); >> + >> clk_disable_unprepare(tegra->clock_tsensor); >> clk_disable_unprepare(tegra->clock_soctherm); >> >> diff --git a/drivers/thermal/tegra/soctherm.h b/drivers/thermal/tegra/soctherm.h >> index 69d317269af1..ec4b87616d01 100644 >> --- a/drivers/thermal/tegra/soctherm.h >> +++ b/drivers/thermal/tegra/soctherm.h >> @@ -16,7 +16,9 @@ >> #define __DRIVERS_THERMAL_TEGRA_SOCTHERM_H >> >> #define SENSOR_CONFIG2 8 >> +#define SENSOR_CONFIG2_THERMA_MASK (0xffff << 16) >> #define SENSOR_CONFIG2_THERMA_SHIFT 16 >> +#define SENSOR_CONFIG2_THERMB_MASK 0xffff >> #define SENSOR_CONFIG2_THERMB_SHIFT 0 >> >> #define SENSOR_PDIV 0x1c0 >> -- >> 1.9.1 >> > > * Unknown Key > * 0x7DA4E256 >