Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752997AbbBYREl (ORCPT ); Wed, 25 Feb 2015 12:04:41 -0500 Received: from down.free-electrons.com ([37.187.137.238]:36790 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752646AbbBYREj (ORCPT ); Wed, 25 Feb 2015 12:04:39 -0500 Message-ID: <54EE009E.8070206@free-electrons.com> Date: Wed, 25 Feb 2015 18:04:30 +0100 From: Gregory CLEMENT User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Tyler Hall , Ezequiel Garcia CC: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Zhang Rui , Eduardo Valentin Subject: Re: [PATCH] thermal: armada: read stable temp on Armada XP References: <1423608615-6575-1-git-send-email-tylerwhall@gmail.com> In-Reply-To: <1423608615-6575-1-git-send-email-tylerwhall@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5946 Lines: 166 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 > --- > > 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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/