Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported sensors

On 01/06/2023 11:52, Peng Fan wrote:

Hi Daniel,

quoted

Subject: RE: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
sensors

quoted

Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
sensors

On 31/05/2023 14:05, Peng Fan wrote:

quoted

quoted

Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable
supported sensors

On 16/05/2023 10:37, Peng Fan (OSS) wrote:

quoted

From: Peng Fan <peng.fan@nxp.com>

There are MAX 16 sensors, but not all of them supported. Such as
i.MX8MQ, there are only 3 sensors. Enabling all 16 sensors will
touch reserved bits from i.MX8MQ reference mannual, and TMU will
stuck, temperature will not update anymore.

Fixes: 45038e03d633 ("thermal: qoriq: Enable all sensors before
registering them")
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
    drivers/thermal/qoriq_thermal.c | 30 +++++++++++++++++++----------

-

quoted

quoted

quoted

quoted

    1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/thermal/qoriq_thermal.c

b/drivers/thermal/qoriq_thermal.c index

b806a0929459..53748c4a5be1

quoted

quoted

quoted

quoted

100644

--- a/drivers/thermal/qoriq_thermal.c
+++ b/drivers/thermal/qoriq_thermal.c

@@ -31,7 +31,6 @@
    #define TMR_DISABLE	0x0
    #define TMR_ME		0x80000000
    #define TMR_ALPF	0x0c000000
-#define TMR_MSITE_ALL	GENMASK(15, 0)

    #define REGS_TMTMIR	0x008	/* Temperature measurement

interval Register */

quoted

    #define TMTMIR_DEFAULT	0x0000000f

@@ -105,6 +104,11 @@ static int tmu_get_temp(struct

thermal_zone_device *tz, int *temp)

quoted

    	 * within sensor range. TEMP is an 9 bit value representing
    	 * temperature in KelVin.
    	 */
+
+	regmap_read(qdata->regmap, REGS_TMR, &val);
+	if (!(val & TMR_ME))
+		return -EAGAIN;

How is this change related to what is described in the changelog?

devm_thermal_zone_of_sensor_register will invoke get temp, since we
reverted the 45038e03d633 did, we need to check TMR_ME to avoid

return

quoted

invalid temperature.

  From a higher perspective if the sensor won't be enabled, then the
thermal zone should not be registered, the get_temp won't happen on a
disabled sensor and this test won't be necessary, no ?

After thinking more, I'd prefer current logic.

We rely on devm_thermal_of_zone_register's return value to know
whether there is a valid zone, then set sites bit, and after collected
all site bits, we enable the thermal IP.

If move the enabling thermal IP before devm_thermal_of_zone_register,
We need check dtb thermal zone, to know which zone is valid for current
thermal IP. This would complicate the design.

So just checking the enabling bit in get temperature would be much
simpler, and there just a small window before enabling thermal IP.

If the thermal zone is not described, then the thermal zone won't be created as it fails with -ENODEV and thus get_temp won't be called on a disabled site, right?

Having test in the get_temp() ops is usually the sign there is something wrong with the driver initialization.



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel