Juha-Matti, moro,
On Tue, Aug 19, 2014 at 10:09 AM, Juha-Matti Tilli
<juha-matti.tilli@xxxxxx> wrote:
>> This adds support for the Tegra SOCTHERM thermal sensing and management
>> system found in the Tegra124 system-on-chip. This initial driver supports
>> temperature polling for four thermal zones.
>
> I have several comments about this patch. Overall, the code looks clean,
> way cleaner than NVIDIA's internal soc_therm driver. I adopted your code
> to run on the internal firmware of a Tegra124 SoC. Additionally, I
> tested the code as-is on a Jetson TK1.
Thanks for the testing effort!
>
> My test shows that the temperature readings look sane and do vary with
> load, so the code seems to work. However, I have some concerns about the
> calibration values calculated by this code and the error handling of the
> code.
>
> Originally, I thought the fuse offsets were incorrect but as it turns
> out, the official Linux kernel starts counting the fuses at a different
> location than NVIDIA's internal kernel.
>
This is a major concern. Juha-Matti and Mikko, can you please cross
check if this driver is accessing to the correct fuse register
location?
> [snip]
>> +static int calculate_shared_calibration(struct tsensor_shared_calibration *r)
>> +{
>> + u32 val;
>> + u32 shifted_cp, shifted_ft;
>> + int err;
>> +
>> + err = tegra_fuse_readl(FUSE_TSENSOR8_CALIB, &val);
>> + if (err)
>> + return err;
>> + r->base_cp = val & FUSE_TSENSOR8_CALIB_CP_TS_BASE_MASK;
>> + r->base_ft = (val & FUSE_TSENSOR8_CALIB_FT_TS_BASE_MASK)
>> + >> FUSE_TSENSOR8_CALIB_FT_TS_BASE_SHIFT;
>> +
>> + err = tegra_fuse_readl(FUSE_SPARE_REALIGNMENT_REG_0, &val);
>> + if (err)
>> + return err;
>> + shifted_cp = sign_extend32(val, 5);
>> + val = ((val & FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_MASK)
>> + >> FUSE_SPARE_REALIGNMENT_REG_SHIFT_FT_SHIFT);
>> + shifted_ft = sign_extend32(val, 4);
>> +
>> + r->actual_temp_cp = 2 * NOMINAL_CALIB_CP_T124 + shifted_cp;
>> + r->actual_temp_ft = 2 * NOMINAL_CALIB_FT_T124 + shifted_ft;
>> +
>> + return 0;
>> +}
>> +
>> +static int calculate_tsensor_calibration(
>> + struct tegra_tsensor *sensor,
>> + struct tsensor_shared_calibration shared,
>> + u32 *calib
>> +)
>> +{
>> + u32 val;
>> + s32 actual_tsensor_ft, actual_tsensor_cp;
>> + s32 delta_sens, delta_temp;
>> + s32 mult, div;
>> + s16 therma, thermb;
>> + int err;
>> +
>> + err = tegra_fuse_readl(sensor->calib_fuse_offset, &val);
>> + if (err)
>> + return err;
>> +
>> + actual_tsensor_cp = (shared.base_cp * 64) + sign_extend32(val, 12);
>> + val = (val & FUSE_TSENSOR_CALIB_FT_TS_BASE_MASK)
>> + >> FUSE_TSENSOR_CALIB_FT_TS_BASE_SHIFT;
>> + actual_tsensor_ft = (shared.base_ft * 32) + sign_extend32(val, 12);
>> +
>> + delta_sens = actual_tsensor_ft - actual_tsensor_cp;
>> + delta_temp = shared.actual_temp_ft - shared.actual_temp_cp;
>> +
>> + mult = t124_tsensor_config.pdiv * t124_tsensor_config.tsample_ate;
>> + div = t124_tsensor_config.tsample * t124_tsensor_config.pdiv_ate;
>> +
>> + therma = div_s64((s64) delta_temp * (1LL << 13) * mult,
>> + (s64) delta_sens * div);
>> + thermb = div_s64(((s64) actual_tsensor_ft * shared.actual_temp_cp) -
>> + ((s64) actual_tsensor_cp * shared.actual_temp_ft),
>> + (s64) delta_sens);
>> +
>> + therma = div_s64((s64) therma * sensor->fuse_corr_alpha,
>> + (s64) 1000000LL);
>> + thermb = div_s64((s64) thermb * sensor->fuse_corr_alpha +
>> + sensor->fuse_corr_beta,
>> + (s64) 1000000LL);
>> +
>> + *calib = ((u16)(therma) << SENSOR_CONFIG2_THERMA_SHIFT) |
>> + ((u16)thermb << SENSOR_CONFIG2_THERMB_SHIFT);
>> +
>> + return 0;
>> +}
>> +
>> +static int enable_tsensor(struct tegra_soctherm *tegra,
>> + struct tegra_tsensor *sensor,
>> + struct tsensor_shared_calibration shared)
>> +{
>> + void * __iomem base = tegra->regs + sensor->base;
>> + unsigned int val;
>> + u32 calib;
>> + int err;
>> +
>> + err = calculate_tsensor_calibration(sensor, shared, &calib);
>> + if (err)
>> + return err;
>> +
>> + val = 0;
>> + val |= t124_tsensor_config.tall << SENSOR_CONFIG0_TALL_SHIFT;
>> + writel(val, base + SENSOR_CONFIG0);
>> +
>> + val = 0;
>> + val |= (t124_tsensor_config.tsample - 1) <<
>> + SENSOR_CONFIG1_TSAMPLE_SHIFT;
>> + val |= t124_tsensor_config.tiddq_en << SENSOR_CONFIG1_TIDDQ_EN_SHIFT;
>> + val |= t124_tsensor_config.ten_count << SENSOR_CONFIG1_TEN_COUNT_SHIFT;
>> + val |= SENSOR_CONFIG1_TEMP_ENABLE;
>> + writel(val, base + SENSOR_CONFIG1);
>> +
>> + writel(calib, base + SENSOR_CONFIG2);
>> +
>> + return 0;
>> +}
>
> This code writes different values to SENSOR_CONFIG2 registers than what
> the NVIDIA's internal soc_therm driver does. Because the calibration
> value calculation should be based on the same fuses that NVIDIA's
> internal driver reads, I believe the value calculated and eventually
> written to SENSOR_CONFIG2 should be identical with NVIDIA's internal
> driver. That is not the case now.
Well, I would suggest using the hardware documentation as a base here.
I don't have access to the internal driver, thus, it is hard to judge
what is right, what is wrong.
>
> I first loaded the NVIDIA's internal soc_therm driver and then loaded
> code adopted from your driver running on Tegra's firmware. Here's a log
> of how the CONFIG2 values are changed by this code to different values
> than NVIDIA's internal soc_therm driver originally sets them to:
>
> CONFIG2: 5b0f813 -> 5c6f7fb
> CONFIG2: 5e8f7cd -> 5fff7b4
> CONFIG2: 5bdf7ce -> 5d3f7b5
> CONFIG2: 596f831 -> 5aaf81a
> CONFIG2: 675f6b5 -> 68cf698
> CONFIG2: 6d6f641 -> 6eff623
> CONFIG2: 641f72b -> 659f710
> CONFIG2: 590f861 -> 5a5f84a
>
> On the left, there's the value set by NVIDIA's internal soc_therm driver
> and on the right, there's the value set by code adopted from your
> driver. My modifications to the code are minor, and I don't think they
> could explain why the CONFIG2 values set are different.
>
> If you want them, I can supply you the values of the fuses on my
> development board.
>
> The temperature readings look sane and do vary with load, but I think
> they're a bit different than what NVIDIA's internal soc_therm driver
hmm.. Based on a single sample?
> gives. I'm not entirely sure if the calibration values set by your
> driver or the calibration values set by NVIDIA's internal soc_therm
> driver are correct, but it seems to me that only one of these drivers
> can be correct.
The calibration values may have strong influence on high temperatures,
which turns out to be the most critical situation on drivers like
this.
>
> The values written to CONFIG1 and CONFIG0 do seem sane.
>
> Since there are divisions being done, some sort of explicit protection
> from divisions by zero should perhaps be considered, although this can
> happen only if the fuses for some reason read all zeroes. I'm not sure
> if that's possible with real hardware.
>
> Where does this code come from? It does not definitely come from
> NVIDIA's internal soc_therm driver, as it looks entirely different. And
> it calculates different calibration values. If you wrote the code, you
> should consider commenting it since there are a lot of complex
> calculations going on that are not obvious to the reader. For example,
> what do "cp" and "ft" mean? Are they acronyms? If so, they should be
> explained.
>
> [snip]
>> + for (i = 0; i < ARRAY_SIZE(tegra->thermctl_tzs); ++i) {
>> + struct tegra_thermctl_zone *zone =
>> + devm_kzalloc(&pdev->dev, sizeof(*zone), GFP_KERNEL);
>> + if (!zone) {
>> + err = -ENOMEM;
>
> Shouldn't this one have a --i line like the next IS_ERR block?
>
>> + goto unregister_tzs;
>> + }
>> +
>> + zone->temp_reg = tegra->regs + thermctl_temp_offsets[i];
>> + zone->temp_shift = thermctl_temp_shifts[i];
>> +
>> + tz = thermal_zone_of_sensor_register(
>> + &pdev->dev, i, zone, tegra_thermctl_get_temp, NULL);
>> + if (IS_ERR(tz)) {
>> + err = PTR_ERR(tz);
>> + dev_err(&pdev->dev, "failed to register sensor: %d\n",
>> + err);
>> + --i;
>> + goto unregister_tzs;
>> + }
>> +
>> + tegra->thermctl_tzs[i] = tz;
>> + }
--
Eduardo Bezerra Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
