Hi Tyler,
On 10/02/2015 23:50, Tyler Hall wrote:
> The current register being used to read the temperature returns a noisy value
> that is prone to variance and occasional outliers. The value in the thermal
> manager control and status register appears to have the same scale but much
> less variability.
>
> Add a "marvell,armadaxp-filtered-thermal" config which is set up to read from
> the Thermal Manager Control and Status Register at 0x184c4 rather than the
> Thermal Sensor Status Register at 0x182b0. The only difference is the
> temperature value shift. The original "marvell,armadaxp-thermal" is retained
> for device tree compatibility.
>
> This also fixes Armada XP clearing the disable bit in the wrong register. Bit 0
> of the sensor register was being cleared but that bit is read-only. The disable
> bit doesn't seem to have an effect on the temperature sensor value, however, so
> this doesn't make a material difference.
>
> This problem was detected when running with the watchdog(8) daemon polling the
> thermal value. In one instance I observed a single read of over 200 degrees C
> which caused a spurious watchdog-initiated reboot. I have since observed
> individual outliers of ~20 degrees C. With this change and the corresponding
> device tree update, the temperature is much more stable.
I tried your patch on a OpenBlocks AX3. Without it I observed a temperature of
47°C:
# cat /sys/class/thermal/thermal_zone0/temp
47233
Whereas with your patch I got a temperature of 228°C!
# cat /sys/class/thermal/thermal_zone0/temp
228065
It should have an error somewhere in the values used.
>
> Signed-off-by: Tyler Hall <tylerwhall@xxxxxxxxx>
> ---
>
> If there's a better way to handle this than a separate binding, I'm open to
> suggestions.
Now that I thought more about it, I believe that it would make more
sens extending the current binding by adding a new optional named
register, indeed at the end we use the same sensor.
The binding would become:
thermal@182b0 {
compatible = "marvell,armadaxp-thermal";
reg = <0x182b0 0x4
0x184d0 0x4
0x184c4 0x4>;
reg-names = "sensor", "ctrl", "filt-sens";
status = "okay";
};
and then in the probe function we got something like
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "filt-sens");
if res {
priv->sensor = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(priv->sensor))
return PTR_ERR(priv->sensor);
} else {
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
priv->sensor = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(priv->sensor))
return PTR_ERR(priv->sensor);
}
My concern was that by introducing a new compatible string we don't
prevent to have 2 thermal nodes for the same hardware.
>
> My conclusions about these registers are based on experimental data. The
> documentation is very sparse, but the Thermal Manager Control and Status
> Register looks like the preferred register given the way it is laid out in the
> public spec.
Ezequiel,
as you worked on this do you know why we used the Thermal Sensor Status Register
instead of the Thermal Manager Control and Status Register ?
My first guess is that the giving the name of the registers the 1st one made
more sens to use for a thermal sensor.
Thanks,
Gregory
>
> I have the small corresponding patch to the dts which I can submit separately.
>
> Thanks
> Tyler
>
> .../devicetree/bindings/thermal/armada-thermal.txt | 8 ++++++++
> drivers/thermal/armada_thermal.c | 13 +++++++++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> index 4698e0e..0d6a3f1 100644
> --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> @@ -7,6 +7,14 @@ Required properties:
> marvell,armada375-thermal
> marvell,armada380-thermal
> marvell,armadaxp-thermal
> + marvell,armadaxp-filtered-thermal
> +
> + Note: "marvell,armadaxp-filtered-thermal" is adjusted to read
> + the hardware-filtered value in the thermal manager
> + control/status register rather than the raw sensor value in the
> + thermal sensor status register. "marvell,armadaxp-thermal"
> + remains for backward compatibility. The sensor reg address must
> + also point to the appropriate register.
>
> - reg: Device's register space.
> Two entries are expected, see the examples below.
> diff --git a/drivers/thermal/armada_thermal.c b/drivers/thermal/armada_thermal.c
> index c2556cf..d3c2ad3 100644
> --- a/drivers/thermal/armada_thermal.c
> +++ b/drivers/thermal/armada_thermal.c
> @@ -196,6 +196,15 @@ static const struct armada_thermal_data armadaxp_data = {
> .coef_div = 13825,
> };
>
> +static const struct armada_thermal_data armadaxp_filt_data = {
> + .init_sensor = armadaxp_init_sensor,
> + .temp_shift = 1,
> + .temp_mask = 0x1ff,
> + .coef_b = 3153000000UL,
> + .coef_m = 10000000UL,
> + .coef_div = 13825,
> +};
> +
> static const struct armada_thermal_data armada370_data = {
> .is_valid = armada_is_valid,
> .init_sensor = armada370_init_sensor,
> @@ -236,6 +245,10 @@ static const struct of_device_id armada_thermal_id_table[] = {
> .data = &armadaxp_data,
> },
> {
> + .compatible = "marvell,armadaxp-filtered-thermal",
> + .data = &armadaxp_filt_data,
> + },
> + {
> .compatible = "marvell,armada370-thermal",
> .data = &armada370_data,
> },
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
