Re: [PATCH] thermal: armada: read stable temp on Armada XP

From: Gregory CLEMENT <gregory.clement@free-electrons.com>
To: Tyler Hall <tylerwhall@gmail.com>,
	Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, Zhang Rui <rui.zhang@intel.com>,
	Eduardo Valentin <edubezval@gmail.com>
Subject: Re: [PATCH] thermal: armada: read stable temp on Armada XP
Date: Wed, 25 Feb 2015 18:04:30 +0100	[thread overview]
Message-ID: <54EE009E.8070206@free-electrons.com> (raw)
In-Reply-To: <1423608615-6575-1-git-send-email-tylerwhall@gmail.com>

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@gmail.com>
> ---
> 
> 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