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 <redacted>
---

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

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com