Re: [PATCH V1 05/10] thermal: tegra: add sysfs to dump registers

From: Wei Ni
Date: Thu Jan 14 2016 - 06:01:07 EST

Next message: Martin Kepplinger: "[PATCH] iio: mma8452: add support for MMA8451Q"
Previous message: Michal Hocko: "Re: [RFC 1/3] oom, sysrq: Skip over oom victims and killed tasks"
In reply to: Thierry Reding: "Re: [PATCH V1 05/10] thermal: tegra: add sysfs to dump registers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2016å01æ13æ 23:17, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Jan 13, 2016 at 04:00:07PM +0800, Wei Ni wrote:
>> Add sysfs interface to dump registers for debug.
>
> You're adding a debugfs interface here, not sysfs.

Oh, sorry, should be debugfs.

>
>> diff --git a/drivers/thermal/tegra/tegra_soctherm.c b/drivers/thermal/tegra/tegra_soctherm.c
> [...]
>> +#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);
>> + struct tegra_tsensor *tsensors = ts->tsensors;
>> + u32 r, state;
>> + int i;
>> +
>> + seq_puts(s, "-----TSENSE (convert HW)-----\n");
>> +
>> + for (i = 0; tsensors[i].name; 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);
>> + 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);
>> + }
>
> This isn't really a register dump, it's a decoded form of the register
> contents. I think it'd be better to either provide a debugfs file with
> the raw register contents, perhaps with register name and offset, or a
> decoded version. Or both.

Yes, you are right, it's a decoded form of register contents.

>
>> +
>> + r = readl(ts->regs + SENSOR_PDIV);
>> + seq_printf(s, "PDIV: 0x%x\n", r);
>> +
>> + 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");
>
> I don't think these separators are any good. debugfs files are
> essentially for free, so if you have logical separation of the content,
> as indicated by the separator here, a better option would be to make
> these separate files.

This is the decoded form. To make it more readable, I add this separators, and I
will add more contents which should belong to it.
This debugfs file is something like:
root@tegra-ubuntu:/sys/kernel/debug/tegra_soctherm# cat regs
-----TSENSE (convert HW)-----
cpu0: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/26500) Capture(1/8471)
Stop(0) Tall(16300) Over(0/0/0) Therm_A/B(1683/-1687)
cpu1: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/25500) Capture(1/8653)
Stop(0) Tall(16300) Over(0/0/0) Therm_A/B(1800/-1850)
cpu2: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/28000) Capture(1/8386)
Stop(0) Tall(16300) Over(0/0/0) Therm_A/B(1709/-1693)
cpu3: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/26500) Capture(1/8402)
Stop(0) Tall(16300) Over(0/0/0) Therm_A/B(1732/-1723)
mem0: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/27000) Capture(1/8552)
Stop(0) Tall(16300) Over(0/0/0) Therm_A/B(1670/-1689)
mem1: En(1) tiddq(1) ten_count(1) tsample(120) Temp(1/25500) Capture(1/8691)
Stop(0) Tall(16300) Over(0/0/0) 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/26500) Capture(1/8465)
Stop(0) Tall(16300) Over(0/0/0) Therm_A/B(1644/-1645)
PDIV: 0x8888
HOTSPOT: 0xa0500

-----SOC_THERM-----
Temperatures: CPU(28000) GPU(29000) PLLX(26500) MEM(27000)

>
>> + 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));
>> + 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));
>
> Isn't this something that the thermal core support already exports via
> some interface?

Hmm, yes, I add them to here so that we can check all HW information in
this file.

>
>> +static int soctherm_debug_init(struct platform_device *pdev)
>> +{
>> + struct dentry *tegra_soctherm_root;
>> +
>> + tegra_soctherm_root = debugfs_create_dir("tegra_soctherm", NULL);
>
> You should check for potential failure here, otherwise you might end up
> in a situation where you create the soctherm files in the debugfs root,
> which is almost certainly not what you want.

Yes, you are right, will check the return value.

>
>> + debugfs_create_file("regs", 0644, tegra_soctherm_root,
>> + pdev, &regs_fops);
>> +
>> + return 0;
>> +}
>> +#else
>> +static inline int soctherm_debug_init(struct platform_device *pdev)
>> +{ return 0; }
>> +#endif
>> +
>> int tegra_soctherm_probe(struct platform_device *pdev,
>> struct tegra_tsensor *tsensors,
>> const struct tegra_tsensor_group **ttgs,
>> @@ -154,6 +276,10 @@ int tegra_soctherm_probe(struct platform_device *pdev,
>> if (!tegra)
>> return -ENOMEM;
>>
>> + dev_set_drvdata(&pdev->dev, tegra);
>> +
>> + tegra->tsensors = tsensors;
>> +
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> tegra->regs = devm_ioremap_resource(&pdev->dev, res);
>> if (IS_ERR(tegra->regs))
>> @@ -243,6 +369,8 @@ int tegra_soctherm_probe(struct platform_device *pdev,
>> tegra->thermctl_tzs[ttgs[i]->id] = tz;
>> }
>>
>> + soctherm_debug_init(pdev);
>
> If you never check and don't care about the return value, I suggest
> making the soctherm_debugfs_init() function return void. You should
> still check for errors within the function to make sure you're not
> doing anything you shouldn't, but you can silently ignore errors if
> you don't care, or perhaps give an indication as to what failed with
> a dev_warn() or similar.

Yes, you are right, I will change to return void.

>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
>

Next message: Martin Kepplinger: "[PATCH] iio: mma8452: add support for MMA8451Q"
Previous message: Michal Hocko: "Re: [RFC 1/3] oom, sysrq: Skip over oom victims and killed tasks"
In reply to: Thierry Reding: "Re: [PATCH V1 05/10] thermal: tegra: add sysfs to dump registers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]