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

2015-02-25 Thread Gregory CLEMENT
Hi Tyler, Eduardo,

On 24/02/2015 20:56, Tyler Hall wrote:
 Eduardo,
 
 On Tue, Feb 24, 2015 at 1:36 PM, Eduardo Valentin edubez...@gmail.com wrote:
 The fix seams reasonable. Although, it remains the question what is
 applicability to other Armada chips? Besides, shouldn't we simply use it
 by default? Also, do you plan to send updates in the DTS files?
 
 As far as I can tell, Armada 370 is already using the equivalent of
 this register I'd like to use in Armada XP. I'm not sure about the
 other mvebu platforms. I couldn't just change the device tree for XP
 to instantiate the 370 sensor, however, as they have different
 initialization routines. Possibly Eziquiel can comment on the
 significance of the differences between armadaxp_init_sensor() and
 armada370_init_sensor().
 
 I would like to change the default going forward, but I don't think it
 can be changed on platforms using an older DTB.

Here you introduced a new kind of thermal sensor, at least from the point
of view of the device tree. You used a new compatible string associated to
a different register.

By using it by default do you mean removing marvell,armadaxp-thermal
and adding armadaxp-filtered-thermal instead ?

Does that new thermal sensors only improve the stability or does it
also modify the value?

In the second case it will more or less break the user space expectation.

 
 I had planned to submit the dts change separately. It's not clear to
 me how that's supposed to be handled if they might go through
 different trees.

For this, there is no problem be handled in a different tree. At the end
we will need both the a new dts and a new driver to use it, so the fact that
the dts or the driver patch is merged in mainline first is not important.


Thanks,

Gregory


 
 Thanks,
 Tyler
 --
 To unsubscribe from this list: send the line unsubscribe devicetree in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 


-- 
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-02-25 Thread Gregory CLEMENT
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 tylerwh...@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 

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

2015-02-25 Thread Tyler Hall
Hi Gregory, Eduardo,

On Wed, Feb 25, 2015 at 05:10:14PM +0100, Gregory CLEMENT wrote:
 By using it by default do you mean removing marvell,armadaxp-thermal
 and adding armadaxp-filtered-thermal instead ?

Yes, replacing it in device tree. For me,
/sys/class/thermal/thermal_zone0/temp returns the same temperature but
with less variability between samples. My intent was to switch the
source of the data and not affect user space other than providing a
more stable reading.

 Tyler,
 In the meantime could you double check your values? The temperature on my 
 board
 seemed broken on my board. If needed I can check on an other board. By the way
 on which board/product did you try it?

I'm on a custom board with the 4-core MV78460. In addition to my
patch, this is new device tree entry.

thermal@184c4 {
   compatible = marvell,armadaxp-filtered-thermal;
   reg = 0x184c4 0x4
0x184d0 0x4;
status = okay;
};

I dumped some raw samples of the temperature fields in each of these
registers. This CSV contains the raw values converted to decimal.
http://pastebin.com/Umc3cy5L
The first column is the current register at 0x182b0 27:19 and the
second is the register at 0x184c4 9:1.

Here's a quick plot of a larger sample size while the temperature was rising.
https://imgur.com/E9HlSBx
The blue value is the current 0x182b0 register which seems to bounce
around the green value from 0x184c4.

I'll try a test of instantiating both at the same time and comparing
the final output of the driver.

On Wed, Feb 25, 2015 at 1:39 PM, Eduardo Valentin edubez...@gmail.com wrote:
 Does that new thermal sensors only improve the stability or does it
 also modify the value?

 In the second case it will more or less break the user space expectation.

 Yeah, I agree here. We need to understand if this is:
 (1) a fix of which register to use. In that case, the old dtbs are
 essentially wrong, and the driver would need to adapt, I would say.
 (2) a way to report better temperature extrapolations. then, this is
 essentially a new temp sensor, and we should have two separated
 compatible. in other words, just keep the patch the way it is.

This *might* be a different physical sensor, or it could be from the
same source with some averaging/filtering applied in hardware. The
conversion formula is the same, however, and I get similar raw values
from both.

 Yes. Typically I ask to see the complete series, even if I am not taking
 the DTS changes. That is, you send a complete series, with changes in
 the kernel code and in the DTS, copying the required audience (from
 kernel side and from DT side). Once the changes are accepted, the
 patches will be picked by the correct tree maintainer. It is common that
 DTS changes go via the platform tree, to avoid conflicts though.

Thanks for the clarification. I'll send both in the next version as I
suspect there will be a v2 of this change.
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-02-25 Thread Eduardo Valentin
On Wed, Feb 25, 2015 at 05:10:14PM +0100, Gregory CLEMENT wrote:
 Hi Tyler, Eduardo,
 
 On 24/02/2015 20:56, Tyler Hall wrote:
  Eduardo,
  
  On Tue, Feb 24, 2015 at 1:36 PM, Eduardo Valentin edubez...@gmail.com 
  wrote:
  The fix seams reasonable. Although, it remains the question what is
  applicability to other Armada chips? Besides, shouldn't we simply use it
  by default? Also, do you plan to send updates in the DTS files?
  
  As far as I can tell, Armada 370 is already using the equivalent of
  this register I'd like to use in Armada XP. I'm not sure about the
  other mvebu platforms. I couldn't just change the device tree for XP
  to instantiate the 370 sensor, however, as they have different
  initialization routines. Possibly Eziquiel can comment on the
  significance of the differences between armadaxp_init_sensor() and
  armada370_init_sensor().
  
  I would like to change the default going forward, but I don't think it
  can be changed on platforms using an older DTB.
 
 Here you introduced a new kind of thermal sensor, at least from the point
 of view of the device tree. You used a new compatible string associated to
 a different register.
 
 By using it by default do you mean removing marvell,armadaxp-thermal
 and adding armadaxp-filtered-thermal instead ?
 
 Does that new thermal sensors only improve the stability or does it
 also modify the value?
 
 In the second case it will more or less break the user space expectation.

Yeah, I agree here. We need to understand if this is:
(1) a fix of which register to use. In that case, the old dtbs are
essentially wrong, and the driver would need to adapt, I would say.
(2) a way to report better temperature extrapolations. then, this is
essentially a new temp sensor, and we should have two separated
compatible. in other words, just keep the patch the way it is.

 
  
  I had planned to submit the dts change separately. It's not clear to
  me how that's supposed to be handled if they might go through
  different trees.
 
 For this, there is no problem be handled in a different tree. At the end
 we will need both the a new dts and a new driver to use it, so the fact that
 the dts or the driver patch is merged in mainline first is not important.
 
 


Yes. Typically I ask to see the complete series, even if I am not taking
the DTS changes. That is, you send a complete series, with changes in
the kernel code and in the DTS, copying the required audience (from
kernel side and from DT side). Once the changes are accepted, the
patches will be picked by the correct tree maintainer. It is common that
DTS changes go via the platform tree, to avoid conflicts though.

In this way, we can have a look if the whole set of
changes are going to make sense, instead of guessing if future DTS
changes will be correct.

 Thanks,
 
 Gregory
 
 
  
  Thanks,
  Tyler
  --
  To unsubscribe from this list: send the line unsubscribe devicetree in
  the body of a message to majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
 
 
 -- 
 Gregory Clement, Free Electrons
 Kernel, drivers, real-time and embedded Linux
 development, consulting, training and support.
 http://free-electrons.com


signature.asc
Description: Digital signature


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

2015-02-25 Thread Ezequiel Garcia
On 02/25/2015 02:04 PM, Gregory CLEMENT wrote:

 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.
 

Actually, we based this driver in the vendor bootloader. The specs weren't
of much use back then.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-02-25 Thread Gregory CLEMENT
Hi Ezequiel,

On 25/02/2015 19:17, Ezequiel Garcia wrote:
 On 02/25/2015 02:04 PM, Gregory CLEMENT wrote:

 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.

 
 Actually, we based this driver in the vendor bootloader. The specs weren't
 of much use back then.
 


Thanks for your prompt feedback. So we don't have much more information about
the sensor. :(

I will try to get information from Marvell.


Tyler,
In the meantime could you double check your values? The temperature on my board
seemed broken on my board. If needed I can check on an other board. By the way
on which board/product did you try it?


Thanks,

Gregory

-- 
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-02-25 Thread Tyler Hall
Gregory,

I instantiated both configurations and verified I get essentially the
same temperature from both, with the original register having a
slightly larger spread. I didn't reproduce any outliers in this run,
but I'll keep it running for a while to see if any occur.

I'm running this to gather samples.
# while true; do echo $(/sys/class/thermal/thermal_zone0/temp),
$(/sys/class/thermal/thermal_zone1/temp); done

https://i.imgur.com/c5wfPuy.png

I just got some information from our vendor indicating there's an
erratum for the register I'm proposing to use. The workaround is to
read repeatedly until the value is consistent. I still have open
questions:
1. Why is the current register exhibiting behavior that would be
described by the erratum while the proposed register seems more
reliable?
2. What is the underlying hardware difference between the two?

This is my hardware revision, though the registers in question appear
in the functional spec for Armada XP, so I would think they would
apply to all flavors.
mvebu-soc-id: MVEBU SoC ID=0x7846, Rev=0x2

Hopefully either of our queries will be answered and we can make a
more informed decision on this.

Thanks,
Tyler

On Wed, Feb 25, 2015 at 2:47 PM, Tyler Hall tylerwh...@gmail.com wrote:
 Hi Gregory, Eduardo,

 On Wed, Feb 25, 2015 at 05:10:14PM +0100, Gregory CLEMENT wrote:
 By using it by default do you mean removing marvell,armadaxp-thermal
 and adding armadaxp-filtered-thermal instead ?

 Yes, replacing it in device tree. For me,
 /sys/class/thermal/thermal_zone0/temp returns the same temperature but
 with less variability between samples. My intent was to switch the
 source of the data and not affect user space other than providing a
 more stable reading.

 Tyler,
 In the meantime could you double check your values? The temperature on my 
 board
 seemed broken on my board. If needed I can check on an other board. By the 
 way
 on which board/product did you try it?

 I'm on a custom board with the 4-core MV78460. In addition to my
 patch, this is new device tree entry.

 thermal@184c4 {
compatible = 
 marvell,armadaxp-filtered-thermal;
reg = 0x184c4 0x4
 0x184d0 0x4;
 status = okay;
 };

 I dumped some raw samples of the temperature fields in each of these
 registers. This CSV contains the raw values converted to decimal.
 http://pastebin.com/Umc3cy5L
 The first column is the current register at 0x182b0 27:19 and the
 second is the register at 0x184c4 9:1.

 Here's a quick plot of a larger sample size while the temperature was rising.
 https://imgur.com/E9HlSBx
 The blue value is the current 0x182b0 register which seems to bounce
 around the green value from 0x184c4.

 I'll try a test of instantiating both at the same time and comparing
 the final output of the driver.

 On Wed, Feb 25, 2015 at 1:39 PM, Eduardo Valentin edubez...@gmail.com wrote:
 Does that new thermal sensors only improve the stability or does it
 also modify the value?

 In the second case it will more or less break the user space expectation.

 Yeah, I agree here. We need to understand if this is:
 (1) a fix of which register to use. In that case, the old dtbs are
 essentially wrong, and the driver would need to adapt, I would say.
 (2) a way to report better temperature extrapolations. then, this is
 essentially a new temp sensor, and we should have two separated
 compatible. in other words, just keep the patch the way it is.

 This *might* be a different physical sensor, or it could be from the
 same source with some averaging/filtering applied in hardware. The
 conversion formula is the same, however, and I get similar raw values
 from both.

 Yes. Typically I ask to see the complete series, even if I am not taking
 the DTS changes. That is, you send a complete series, with changes in
 the kernel code and in the DTS, copying the required audience (from
 kernel side and from DT side). Once the changes are accepted, the
 patches will be picked by the correct tree maintainer. It is common that
 DTS changes go via the platform tree, to avoid conflicts though.

 Thanks for the clarification. I'll send both in the next version as I
 suspect there will be a v2 of this change.
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-02-24 Thread Tyler Hall
Eduardo,

On Tue, Feb 24, 2015 at 1:36 PM, Eduardo Valentin edubez...@gmail.com wrote:
 The fix seams reasonable. Although, it remains the question what is
 applicability to other Armada chips? Besides, shouldn't we simply use it
 by default? Also, do you plan to send updates in the DTS files?

As far as I can tell, Armada 370 is already using the equivalent of
this register I'd like to use in Armada XP. I'm not sure about the
other mvebu platforms. I couldn't just change the device tree for XP
to instantiate the 370 sensor, however, as they have different
initialization routines. Possibly Eziquiel can comment on the
significance of the differences between armadaxp_init_sensor() and
armada370_init_sensor().

I would like to change the default going forward, but I don't think it
can be changed on platforms using an older DTB.

I had planned to submit the dts change separately. It's not clear to
me how that's supposed to be handled if they might go through
different trees.

Thanks,
Tyler
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-02-24 Thread Eduardo Valentin

Tyler,

On Tue, Feb 10, 2015 at 05:50:15PM -0500, 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.
 
 Signed-off-by: Tyler Hall tylerwh...@gmail.com
 ---
 
 If there's a better way to handle this than a separate binding, I'm open to
 suggestions.


The fix seams reasonable. Although, it remains the question what is
applicability to other Armada chips? Besides, shouldn't we simply use it
by default? Also, do you plan to send updates in the DTS files?


I would appreciate if you get a Review-by, from Ezequiel for instance,
before we apply this.


Ezequiel, any objections before I move forward with this one?

BR,

Eduardo Valentin
 
 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.
 
 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 = 315300UL,
 + .coef_m = 1000UL,
 + .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,
   },
 -- 
 2.3.0
 


signature.asc
Description: Digital signature


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

2015-02-10 Thread Tyler Hall
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.

Signed-off-by: Tyler Hall tylerwh...@gmail.com
---

If there's a better way to handle this than a separate binding, I'm open to
suggestions.

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.

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 = 315300UL,
+   .coef_m = 1000UL,
+   .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,
},
-- 
2.3.0

--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html