Re: [PATCH] thermal: armada: read stable temp on Armada XP
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
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
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
[PATCH] thermal: armada: read stable temp on Armada XP
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
Re: gpio-pxa: getting GPIOs by devicetree phandle broken
The issue with multiple gpiochips per of-node could be worked around as followed I believe, comments? diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 08261f2..43984ab 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -47,11 +47,12 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data) ret = gc-of_xlate(gc, gg_data-gpiospec, gg_data-flags); if (ret 0) { /* We've found the gpio chip, but the translation failed. -* Return true to stop looking and return the translation -* error via out_gpio +* Store translation error in out_gpio. +* Return false to keep looking, as more than one GPIO chip +* could be registered per of-node. */ gg_data-out_gpio = ERR_PTR(ret); - return true; + return false; } gg_data-out_gpio = gpiochip_get_desc(gc, ret); As long as we're ok with multiple gpiochips per of-node, this would work for me. It'll change the preference of which chip returns the error in the case of multiple chips, but that's already undefined behavior. -- 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
gpio-pxa: getting GPIOs by devicetree phandle broken
Hi, Commit 7b8792b (gpiolib: of: Correct error handling in of_get_named_gpiod_flags) seems to break the ability to use DT bindings to reference this driver's GPIOs by phandle for banks above the first. The issue is that gpio-pxa registers multiple gpio chips - one for each bank - but they're all associated with the same DT node. The new behavior in of_gpiochip_find_and_xlate() causes gpiochip_find() to bail after the first chip matches and its xlate function fails. Previously it would try all chips associated with the phandle and pxa_gpio_of_xlate() would fail until it was called with the correct gpiochip. I think the new behavior of of_gpiochip_find_and_xlate() is reasonable, so I see a couple ways of fixing gpio-pxa. 1. Require child nodes in DT for each bank 2. Refactor gpio-pxa to only register one gpiochip There was a previously undocumented binding that required child nodes but it was removed by 5dbb7c6 (gpio: pxa: remove dead code). It's still present in the DT files that use marvell,mmp-gpio. Here's an example from pxa168.dtsi. gpio@d4019000 { compatible = marvell,mmp-gpio; #address-cells = 1; #size-cells = 1; ... gcb0: gpio@d4019000 { reg = 0xd4019000 0x4; }; ... }; I may proceed with option 1 since there's some precedent, but I'd appreciate any input. 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