[PATCH 0/3] thermal: qoriq_thermal: support TMU 2.1

[PATCH 0/3] thermal: qoriq_thermal: support TMU 2.1

[Peng Fan (OSS)]

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

i.MX93 use same TMU IP as Qoriq, but with version 2.1, so update
the driver to support 2.1.

This patch also includes a fix for i.MX8MQ

Pankit Garg (1):
  thermal: qoriq_thermal: No need to program site adjustment register

Peng Fan (2):
  thermal: qoriq_thermal: only enable supported sensors
  thermal: qoriq: support version 2.1

 drivers/thermal/qoriq_thermal.c | 48 ++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 19 deletions(-)

-- 
2.37.1

[PATCH 1/3] thermal: qoriq_thermal: No need to program site adjustment register

[Peng Fan (OSS)]

From: Pankit Garg <pankit.garg@nxp.com>

No need to program site adjustment register, as programming
these registers do not give accurate value and also these
registers are not mentioned in Reference Manual.

Signed-off-by: Pankit Garg <pankit.garg@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/thermal/qoriq_thermal.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
index e58756323457..b806a0929459 100644
--- a/drivers/thermal/qoriq_thermal.c
+++ b/drivers/thermal/qoriq_thermal.c
@@ -208,8 +208,6 @@ static int qoriq_tmu_calibration(struct device *dev,
 
 static void qoriq_tmu_init_device(struct qoriq_tmu_data *data)
 {
-	int i;
-
 	/* Disable interrupt, using polling instead */
 	regmap_write(data->regmap, REGS_TIER, TIER_DISABLE);
 
@@ -220,8 +218,6 @@ static void qoriq_tmu_init_device(struct qoriq_tmu_data *data)
 	} else {
 		regmap_write(data->regmap, REGS_V2_TMTMIR, TMTMIR_DEFAULT);
 		regmap_write(data->regmap, REGS_V2_TEUMR(0), TEUMR0_V2);
-		for (i = 0; i < SITES_MAX; i++)
-			regmap_write(data->regmap, REGS_V2_TMSAR(i), TMSARA_V2);
 	}
 
 	/* Disable monitoring */
-- 
2.37.1

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

[Peng Fan (OSS)]

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 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
index b806a0929459..53748c4a5be1 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 */
 #define TMTMIR_DEFAULT	0x0000000f
@@ -105,6 +104,11 @@ static int tmu_get_temp(struct thermal_zone_device *tz, int *temp)
 	 * 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;
+
 	if (regmap_read_poll_timeout(qdata->regmap,
 				     REGS_TRITSR(qsensor->id),
 				     val,
@@ -128,15 +132,7 @@ static const struct thermal_zone_device_ops tmu_tz_ops = {
 static int qoriq_tmu_register_tmu_zone(struct device *dev,
 				       struct qoriq_tmu_data *qdata)
 {
-	int id;
-
-	if (qdata->ver == TMU_VER1) {
-		regmap_write(qdata->regmap, REGS_TMR,
-			     TMR_MSITE_ALL | TMR_ME | TMR_ALPF);
-	} else {
-		regmap_write(qdata->regmap, REGS_V2_TMSR, TMR_MSITE_ALL);
-		regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF_V2);
-	}
+	int id, sites = 0;
 
 	for (id = 0; id < SITES_MAX; id++) {
 		struct thermal_zone_device *tzd;
@@ -153,14 +149,26 @@ static int qoriq_tmu_register_tmu_zone(struct device *dev,
 			if (ret == -ENODEV)
 				continue;
 
-			regmap_write(qdata->regmap, REGS_TMR, TMR_DISABLE);
 			return ret;
 		}
 
+		if (qdata->ver == TMU_VER1)
+			sites |= 0x1 << (15 - id);
+		else
+			sites |= 0x1 << id;
+
 		if (devm_thermal_add_hwmon_sysfs(dev, tzd))
 			dev_warn(dev,
 				 "Failed to add hwmon sysfs attributes\n");
+	}
 
+	if (sites) {
+		if (qdata->ver == TMU_VER1) {
+			regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF | sites);
+		} else {
+			regmap_write(qdata->regmap, REGS_V2_TMSR, sites);
+			regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF_V2);
+		}
 	}
 
 	return 0;
-- 
2.37.1

[PATCH 3/3] thermal: qoriq: support version 2.1

[Peng Fan (OSS)]

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

i.MX93 use TMU version 2.1, which supports:
 - TRITSR_TP5(When this field is 1, you must add 0.5 K to the temperature
   that TEMP reports. For example, if TEMP is 300 K and TP5=1, then the
   final temperature is 300.5 K.)
 - Has 16 TTRCR register: Temperature Range Control (TTRCR0 - TTRCR15)

This patch is to add this support.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/thermal/qoriq_thermal.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
index 53748c4a5be1..c710449b0c50 100644
--- a/drivers/thermal/qoriq_thermal.c
+++ b/drivers/thermal/qoriq_thermal.c
@@ -50,6 +50,7 @@
 					    * Site Register
 					    */
 #define TRITSR_V	BIT(31)
+#define TRITSR_TP5	BIT(9)
 #define REGS_V2_TMSAR(n)	(0x304 + 16 * (n))	/* TMU monitoring
 						* site adjustment register
 						*/
@@ -117,10 +118,15 @@ static int tmu_get_temp(struct thermal_zone_device *tz, int *temp)
 				     10 * USEC_PER_MSEC))
 		return -ENODATA;
 
-	if (qdata->ver == TMU_VER1)
+	if (qdata->ver == TMU_VER1) {
 		*temp = (val & GENMASK(7, 0)) * MILLIDEGREE_PER_DEGREE;
-	else
-		*temp = kelvin_to_millicelsius(val & GENMASK(8, 0));
+	} else {
+		if (val & TRITSR_TP5)
+			*temp = milli_kelvin_to_millicelsius((val & GENMASK(8, 0)) *
+							     MILLIDEGREE_PER_DEGREE + 500);
+		else
+			*temp = kelvin_to_millicelsius(val & GENMASK(8, 0));
+	}
 
 	return 0;
 }
@@ -234,7 +240,7 @@ static void qoriq_tmu_init_device(struct qoriq_tmu_data *data)
 
 static const struct regmap_range qoriq_yes_ranges[] = {
 	regmap_reg_range(REGS_TMR, REGS_TSCFGR),
-	regmap_reg_range(REGS_TTRnCR(0), REGS_TTRnCR(3)),
+	regmap_reg_range(REGS_TTRnCR(0), REGS_TTRnCR(15)),
 	regmap_reg_range(REGS_V2_TEUMR(0), REGS_V2_TEUMR(2)),
 	regmap_reg_range(REGS_V2_TMSAR(0), REGS_V2_TMSAR(15)),
 	regmap_reg_range(REGS_IPBRR(0), REGS_IPBRR(1)),
-- 
2.37.1

RE: [PATCH 0/3] thermal: qoriq_thermal: support TMU 2.1

[Alice Guo]

Hi,

For this patchset, Reviewed-by: Alice Guo <alice.guo@nxp.com>.

Best Regards,
Alice Guo

> -----Original Message-----
> From: Peng Fan (OSS) <peng.fan@oss.nxp.com>
> Sent: Tuesday, May 16, 2023 4:38 PM
> To: rafael@kernel.org; daniel.lezcano@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de
> Cc: amitk@kernel.org; rui.zhang@intel.com; andrew.smirnov@gmail.com;
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org; Alice Guo
> <alice.guo@nxp.com>; Peng Fan <peng.fan@nxp.com>
> Subject: [PATCH 0/3] thermal: qoriq_thermal: support TMU 2.1
> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX93 use same TMU IP as Qoriq, but with version 2.1, so update the driver to
> support 2.1.
> 
> This patch also includes a fix for i.MX8MQ
> 
> Pankit Garg (1):
>   thermal: qoriq_thermal: No need to program site adjustment register
> 
> Peng Fan (2):
>   thermal: qoriq_thermal: only enable supported sensors
>   thermal: qoriq: support version 2.1
> 
>  drivers/thermal/qoriq_thermal.c | 48 ++++++++++++++++++++-------------
>  1 file changed, 29 insertions(+), 19 deletions(-)
> 
> --
> 2.37.1

RE: [PATCH 0/3] thermal: qoriq_thermal: support TMU 2.1

[Peng Fan]

Hi Daniel

> Subject: [PATCH 0/3] thermal: qoriq_thermal: support TMU 2.1

Are you fine with this patchset?

Thanks,
Peng.

> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX93 use same TMU IP as Qoriq, but with version 2.1, so update the driver
> to support 2.1.
> 
> This patch also includes a fix for i.MX8MQ
> 
> Pankit Garg (1):
>   thermal: qoriq_thermal: No need to program site adjustment register
> 
> Peng Fan (2):
>   thermal: qoriq_thermal: only enable supported sensors
>   thermal: qoriq: support version 2.1
> 
>  drivers/thermal/qoriq_thermal.c | 48 ++++++++++++++++++++-------------
>  1 file changed, 29 insertions(+), 19 deletions(-)
> 
> --
> 2.37.1

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

[Daniel Lezcano]

On 16/05/2023 10:37, Peng Fan (OSS) wrote:
> 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 +++++++++++++++++++-----------
>   1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
> index b806a0929459..53748c4a5be1 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 */
>   #define TMTMIR_DEFAULT	0x0000000f
> @@ -105,6 +104,11 @@ static int tmu_get_temp(struct thermal_zone_device *tz, int *temp)
>   	 * 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?

>   	if (regmap_read_poll_timeout(qdata->regmap,
>   				     REGS_TRITSR(qsensor->id),
>   				     val,
> @@ -128,15 +132,7 @@ static const struct thermal_zone_device_ops tmu_tz_ops = {
>   static int qoriq_tmu_register_tmu_zone(struct device *dev,
>   				       struct qoriq_tmu_data *qdata)
>   {
> -	int id;
> -
> -	if (qdata->ver == TMU_VER1) {
> -		regmap_write(qdata->regmap, REGS_TMR,
> -			     TMR_MSITE_ALL | TMR_ME | TMR_ALPF);
> -	} else {
> -		regmap_write(qdata->regmap, REGS_V2_TMSR, TMR_MSITE_ALL);
> -		regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF_V2);
> -	}
> +	int id, sites = 0;
>   
>   	for (id = 0; id < SITES_MAX; id++) {
>   		struct thermal_zone_device *tzd;
> @@ -153,14 +149,26 @@ static int qoriq_tmu_register_tmu_zone(struct device *dev,
>   			if (ret == -ENODEV)
>   				continue;
>   
> -			regmap_write(qdata->regmap, REGS_TMR, TMR_DISABLE);
>   			return ret;
>   		}
>   
> +		if (qdata->ver == TMU_VER1)
> +			sites |= 0x1 << (15 - id);
> +		else
> +			sites |= 0x1 << id;
> +
>   		if (devm_thermal_add_hwmon_sysfs(dev, tzd))
>   			dev_warn(dev,
>   				 "Failed to add hwmon sysfs attributes\n");
> +	}
>   
> +	if (sites) {
> +		if (qdata->ver == TMU_VER1) {
> +			regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF | sites);
> +		} else {
> +			regmap_write(qdata->regmap, REGS_V2_TMSR, sites);
> +			regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF_V2);
> +		}
>   	}
>   
>   	return 0;

-- 
<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

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

[Peng Fan]

> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
> sensors
> 
> On 16/05/2023 10:37, Peng Fan (OSS) wrote:
> > 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 +++++++++++++++++++-----------
> >   1 file changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/thermal/qoriq_thermal.c
> > b/drivers/thermal/qoriq_thermal.c index b806a0929459..53748c4a5be1
> > 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 */
> >   #define TMTMIR_DEFAULT	0x0000000f
> > @@ -105,6 +104,11 @@ static int tmu_get_temp(struct
> thermal_zone_device *tz, int *temp)
> >   	 * 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 invalid temperature.

Regards,
Peng.

> 
> >   	if (regmap_read_poll_timeout(qdata->regmap,
> >   				     REGS_TRITSR(qsensor->id),
> >   				     val,
> > @@ -128,15 +132,7 @@ static const struct thermal_zone_device_ops
> tmu_tz_ops = {
> >   static int qoriq_tmu_register_tmu_zone(struct device *dev,
> >   				       struct qoriq_tmu_data *qdata)
> >   {
> > -	int id;
> > -
> > -	if (qdata->ver == TMU_VER1) {
> > -		regmap_write(qdata->regmap, REGS_TMR,
> > -			     TMR_MSITE_ALL | TMR_ME | TMR_ALPF);
> > -	} else {
> > -		regmap_write(qdata->regmap, REGS_V2_TMSR,
> TMR_MSITE_ALL);
> > -		regmap_write(qdata->regmap, REGS_TMR, TMR_ME |
> TMR_ALPF_V2);
> > -	}
> > +	int id, sites = 0;
> >
> >   	for (id = 0; id < SITES_MAX; id++) {
> >   		struct thermal_zone_device *tzd;
> > @@ -153,14 +149,26 @@ static int qoriq_tmu_register_tmu_zone(struct
> device *dev,
> >   			if (ret == -ENODEV)
> >   				continue;
> >
> > -			regmap_write(qdata->regmap, REGS_TMR,
> TMR_DISABLE);
> >   			return ret;
> >   		}
> >
> > +		if (qdata->ver == TMU_VER1)
> > +			sites |= 0x1 << (15 - id);
> > +		else
> > +			sites |= 0x1 << id;
> > +
> >   		if (devm_thermal_add_hwmon_sysfs(dev, tzd))
> >   			dev_warn(dev,
> >   				 "Failed to add hwmon sysfs attributes\n");
> > +	}
> >
> > +	if (sites) {
> > +		if (qdata->ver == TMU_VER1) {
> > +			regmap_write(qdata->regmap, REGS_TMR,
> TMR_ME | TMR_ALPF | sites);
> > +		} else {
> > +			regmap_write(qdata->regmap, REGS_V2_TMSR,
> sites);
> > +			regmap_write(qdata->regmap, REGS_TMR,
> TMR_ME | TMR_ALPF_V2);
> > +		}
> >   	}
> >
> >   	return 0;
> 
> --
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.linaro.org%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C032080272b
> b84255c00b08db61c6a962%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> 7C0%7C638211278103002380%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> %7C%7C%7C&sdata=OBOfWzY2%2BAZDHWoiqwsdShvt0P6o3JYhSCmlz9wN
> Wzc%3D&reserved=0> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.facebook.com%2Fpages%2FLinaro&data=05%7C01%7Cpeng.fan%40nxp.c
> om%7C032080272bb84255c00b08db61c6a962%7C686ea1d3bc2b4c6fa92cd
> 99c5c301635%7C0%7C0%7C638211278103002380%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C3000%7C%7C%7C&sdata=PBMinw85VAW%2BvLnNtNnxui1e7
> kqJioymsokTrRRoB5c%3D&reserved=0> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitt
> er.com%2F%23!%2Flinaroorg&data=05%7C01%7Cpeng.fan%40nxp.com%7C
> 032080272bb84255c00b08db61c6a962%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C638211278103002380%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000%7C%7C%7C&sdata=Grd5C5p1cwjl6kwHUfhCFVwFMOhZjedAm5
> UnPoU5%2FUU%3D&reserved=0> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.linaro.org%2Flinaro-
> blog%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C032080272bb84255c
> 00b08db61c6a962%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> 638211278103002380%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=jWOAT8Eg0AzCePPrAsNtY%2FdZzNs2kvY2kGp8uO4I7a8%3D&re
> served=0> Blog

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

[Daniel Lezcano]

On 31/05/2023 14:05, Peng Fan wrote:
>> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
>> sensors
>>
>> On 16/05/2023 10:37, Peng Fan (OSS) wrote:
>>> 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 +++++++++++++++++++-----------
>>>    1 file changed, 19 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/thermal/qoriq_thermal.c
>>> b/drivers/thermal/qoriq_thermal.c index b806a0929459..53748c4a5be1
>>> 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 */
>>>    #define TMTMIR_DEFAULT	0x0000000f
>>> @@ -105,6 +104,11 @@ static int tmu_get_temp(struct
>> thermal_zone_device *tz, int *temp)
>>>    	 * 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 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 ?

-- 
<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

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

[Peng Fan]

> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
> sensors
> 
> On 31/05/2023 14:05, Peng Fan wrote:
> >> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable
> >> supported sensors
> >>
> >> On 16/05/2023 10:37, Peng Fan (OSS) wrote:
> >>> 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 +++++++++++++++++++-----------
> >>>    1 file changed, 19 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/thermal/qoriq_thermal.c
> >>> b/drivers/thermal/qoriq_thermal.c index b806a0929459..53748c4a5be1
> >>> 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 */
> >>>    #define TMTMIR_DEFAULT	0x0000000f
> >>> @@ -105,6 +104,11 @@ static int tmu_get_temp(struct
> >> thermal_zone_device *tz, int *temp)
> >>>    	 * 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
> > 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 ?

Agree, let me move the temp sensor enabling before register thermal zone.

Thanks,
Peng.

> 
> --
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.linaro.org%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C2a5070b5049
> 14957160008db61d37861%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C638211333111330055%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
> 7C%7C%7C&sdata=aJBF2UEqaCAqcAbHcKzbGReVv6pbYlyQ25riVxEdG08%3D
> &reserved=0> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.facebook.com%2Fpages%2FLinaro&data=05%7C01%7Cpeng.fan%40nxp.c
> om%7C2a5070b504914957160008db61d37861%7C686ea1d3bc2b4c6fa92cd
> 99c5c301635%7C0%7C0%7C638211333111330055%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C3000%7C%7C%7C&sdata=9l627puhL7hQgMlPWaKkCIDkQKGX
> TH49rEM0NFipvDs%3D&reserved=0> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitt
> er.com%2F%23!%2Flinaroorg&data=05%7C01%7Cpeng.fan%40nxp.com%7C
> 2a5070b504914957160008db61d37861%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C638211333111330055%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000%7C%7C%7C&sdata=%2B0Oa%2BrxHmGPga0%2BGQjOOX6Dxaiuj
> oPJhzwqBjjO%2B2Qo%3D&reserved=0> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.linaro.org%2Flinaro-
> blog%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C2a5070b5049149571
> 60008db61d37861%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> 638211333111330055%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=2lfwzPj3p%2BjowtVDc03plo1Ds%2BnT%2B2eSwdTQ9yQosfo%3
> D&reserved=0> Blog

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

[Peng Fan]

Hi Daniel,

> Subject: RE: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
> sensors
> 
> > Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
> > sensors
> >
> > On 31/05/2023 14:05, Peng Fan wrote:
> > >> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable
> > >> supported sensors
> > >>
> > >> On 16/05/2023 10:37, Peng Fan (OSS) wrote:
> > >>> 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 +++++++++++++++++++----------
> -
> > >>>    1 file changed, 19 insertions(+), 11 deletions(-)
> > >>>
> > >>> diff --git a/drivers/thermal/qoriq_thermal.c
> > >>> b/drivers/thermal/qoriq_thermal.c index
> b806a0929459..53748c4a5be1
> > >>> 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 */
> > >>>    #define TMTMIR_DEFAULT	0x0000000f
> > >>> @@ -105,6 +104,11 @@ static int tmu_get_temp(struct
> > >> thermal_zone_device *tz, int *temp)
> > >>>    	 * 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
> > > 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.

Thanks,
Peng.

> 
> Agree, let me move the temp sensor enabling before register thermal zone.
> 
> Thanks,
> Peng.
> 
> >
> > --
> >
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> >
> w.linaro.org%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C2a5070b5049
> >
> 14957160008db61d37861%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> >
> C0%7C638211333111330055%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> > wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
> >
> 7C%7C%7C&sdata=aJBF2UEqaCAqcAbHcKzbGReVv6pbYlyQ25riVxEdG08%3D
> > &reserved=0> Linaro.org │ Open source software for ARM SoCs
> >
> > Follow Linaro:
> >
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> >
> w.facebook.com%2Fpages%2FLinaro&data=05%7C01%7Cpeng.fan%40nxp.c
> >
> om%7C2a5070b504914957160008db61d37861%7C686ea1d3bc2b4c6fa92cd
> >
> 99c5c301635%7C0%7C0%7C638211333111330055%7CUnknown%7CTWFpb
> >
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> >
> 6Mn0%3D%7C3000%7C%7C%7C&sdata=9l627puhL7hQgMlPWaKkCIDkQKGX
> > TH49rEM0NFipvDs%3D&reserved=0> Facebook |
> >
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwit
> > t
> er.com%2F%23!%2Flinaroorg&data=05%7C01%7Cpeng.fan%40nxp.com%7C
> >
> 2a5070b504914957160008db61d37861%7C686ea1d3bc2b4c6fa92cd99c5c30
> >
> 1635%7C0%7C0%7C638211333111330055%7CUnknown%7CTWFpbGZsb3d8
> >
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> >
> D%7C3000%7C%7C%7C&sdata=%2B0Oa%2BrxHmGPga0%2BGQjOOX6Dxaiuj
> > oPJhzwqBjjO%2B2Qo%3D&reserved=0> Twitter |
> >
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> > w.linaro.org%2Flinaro-
> >
> blog%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C2a5070b5049149571
> >
> 60008db61d37861%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> >
> 638211333111330055%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> >
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> > %7C&sdata=2lfwzPj3p%2BjowtVDc03plo1Ds%2BnT%2B2eSwdTQ9yQosfo
> %3
> > D&reserved=0> Blog

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

[Daniel Lezcano]

On 01/06/2023 11:52, Peng Fan wrote:
> Hi Daniel,
> 
>> Subject: RE: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
>> sensors
>>
>>> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
>>> sensors
>>>
>>> On 31/05/2023 14:05, Peng Fan wrote:
>>>>> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable
>>>>> supported sensors
>>>>>
>>>>> On 16/05/2023 10:37, Peng Fan (OSS) wrote:
>>>>>> 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 +++++++++++++++++++----------
>> -
>>>>>>     1 file changed, 19 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/thermal/qoriq_thermal.c
>>>>>> b/drivers/thermal/qoriq_thermal.c index
>> b806a0929459..53748c4a5be1
>>>>>> 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 */
>>>>>>     #define TMTMIR_DEFAULT	0x0000000f
>>>>>> @@ -105,6 +104,11 @@ static int tmu_get_temp(struct
>>>>> thermal_zone_device *tz, int *temp)
>>>>>>     	 * 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
>>>> 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

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

[Sebastian Krzyszkowiak]

On piątek, 2 czerwca 2023 15:11:37 CEST Daniel Lezcano wrote:
> On 01/06/2023 11:52, Peng Fan wrote:
> > Hi Daniel,
> > 
> >> Subject: RE: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
> >> sensors
> >> 
> >>> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
> >>> sensors
> >>> 
> >>> On 31/05/2023 14:05, Peng Fan wrote:
> >>>>> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable
> >>>>> supported sensors
> >>>>> 
> >>>>> On 16/05/2023 10:37, Peng Fan (OSS) wrote:
> >>>>>> 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
> >>>>>>     +++++++++++++++++++----------
> >> 
> >> -
> >> 
> >>>>>>     1 file changed, 19 insertions(+), 11 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/drivers/thermal/qoriq_thermal.c
> >>>>>> b/drivers/thermal/qoriq_thermal.c index
> >> 
> >> b806a0929459..53748c4a5be1
> >> 
> >>>>>> 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 */
> >>>>> 
> >>>>>>     #define TMTMIR_DEFAULT	0x0000000f
> >>>>>> 
> >>>>>> @@ -105,6 +104,11 @@ static int tmu_get_temp(struct
> >>>>> 
> >>>>> thermal_zone_device *tz, int *temp)
> >>>>> 
> >>>>>>     	 * 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
> >>> 
> >>>> 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?

That's not what the problem is. It's about zones that *will* be created - 
since the driver only knows that a thermal zone isn't described in the device 
tree after it fails registering, it can't enable the site *before* the zone 
gets registered - so it happens afterwards. That's why it needs this check to 
not return a bogus initial value before the site gets actually enabled later 
in qoriq_tmu_register_tmu_zone.

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

I've sent a patch that solved this exact problem back in 2021 by checking 
whether a zone is described first with thermal_zone_of_get_sensor_id, but it 
was sitting out there ignored long enough that the function got removed from 
the kernel in 5d10f480f77b and I'm not exactly sure how to solve it cleanly 
without getting it back. Without a replacement, what Peng did seems like the 
only way to not reintroduce the problem 45038e03d633 was supposed to fix.

The patch: https://lore.kernel.org/linux-pm/20220301033402.415445-1-sebastian.krzyszkowiak@puri.sm/

Questions on how to proceed further: https://lore.kernel.org/linux-pm/
5800115.iIbC2pHGDl@pliszka/

Would be nice to get this finally resolved, the mainline TMU driver has been 
completely unreliable on i.MX8MQ for years now.

Thanks,
Sebastian

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

[Daniel Lezcano]

On 07/06/2023 08:01, Sebastian Krzyszkowiak wrote:
> On piątek, 2 czerwca 2023 15:11:37 CEST Daniel Lezcano wrote:
>> On 01/06/2023 11:52, Peng Fan wrote:
>>> Hi Daniel,
>>>
>>>> Subject: RE: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
>>>> sensors
>>>>
>>>>> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
>>>>> sensors
>>>>>
>>>>> On 31/05/2023 14:05, Peng Fan wrote:
>>>>>>> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable
>>>>>>> supported sensors
>>>>>>>
>>>>>>> On 16/05/2023 10:37, Peng Fan (OSS) wrote:
>>>>>>>> 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
>>>>>>>>      +++++++++++++++++++----------
>>>>
>>>> -
>>>>
>>>>>>>>      1 file changed, 19 insertions(+), 11 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/thermal/qoriq_thermal.c
>>>>>>>> b/drivers/thermal/qoriq_thermal.c index
>>>>
>>>> b806a0929459..53748c4a5be1
>>>>
>>>>>>>> 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 */
>>>>>>>
>>>>>>>>      #define TMTMIR_DEFAULT	0x0000000f
>>>>>>>>
>>>>>>>> @@ -105,6 +104,11 @@ static int tmu_get_temp(struct
>>>>>>>
>>>>>>> thermal_zone_device *tz, int *temp)
>>>>>>>
>>>>>>>>      	 * 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
>>>>>
>>>>>> 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?
> 
> That's not what the problem is. It's about zones that *will* be created -
> since the driver only knows that a thermal zone isn't described in the device
> tree after it fails registering, it can't enable the site *before* the zone
> gets registered - so it happens afterwards. That's why it needs this check to
> not return a bogus initial value before the site gets actually enabled later
> in qoriq_tmu_register_tmu_zone.

Sorry, I get the point but I don't see how that can happen:

qoriq_tmu_register_tmu_zone() calls devm_thermal_of_zone_register() for 
*all* sites regardless if they really exists or not.

Under the hood, the function devm_thermal_of_zone_register() calls 
thermal_of_zone_register(). This one fails when calling 
of_thermal_zone_find() because it does not exist and returns -ENODEV.

Hence, the thermal_zone_device_register_with_trips() is not called, the 
thermal zone is not created neither updated.

So I don't understand why the test:

+	regmap_read(qdata->regmap, REGS_TMR, &val);
+	if (!(val & TMR_ME))
+		return -EAGAIN;

is needed in the get_temp() ops as the thermal zone for this disabled 
site should not exist.

I'm not putting in question the series, just wanting to avoid a 
potential pointless check in an ops.


[ ... ]

-- 
<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

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

[Sebastian Krzyszkowiak]

On środa, 7 czerwca 2023 10:28:59 CEST Daniel Lezcano wrote:
> On 07/06/2023 08:01, Sebastian Krzyszkowiak wrote:
> > On piątek, 2 czerwca 2023 15:11:37 CEST Daniel Lezcano wrote:
> >> On 01/06/2023 11:52, Peng Fan wrote:
> >>> Hi Daniel,
> >>> 
> >>>> Subject: RE: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
> >>>> sensors
> >>>> 
> >>>>> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
> >>>>> sensors
> >>>>> 
> >>>>> On 31/05/2023 14:05, Peng Fan wrote:
> >>>>>>> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable
> >>>>>>> supported sensors
> >>>>>>> 
> >>>>>>> On 16/05/2023 10:37, Peng Fan (OSS) wrote:
> >>>>>>>> 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
> >>>>>>>>      +++++++++++++++++++----------
> >>>> 
> >>>> -
> >>>> 
> >>>>>>>>      1 file changed, 19 insertions(+), 11 deletions(-)
> >>>>>>>> 
> >>>>>>>> diff --git a/drivers/thermal/qoriq_thermal.c
> >>>>>>>> b/drivers/thermal/qoriq_thermal.c index
> >>>> 
> >>>> b806a0929459..53748c4a5be1
> >>>> 
> >>>>>>>> 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 */
> >>>>>>> 
> >>>>>>>>      #define TMTMIR_DEFAULT	0x0000000f
> >>>>>>>> 
> >>>>>>>> @@ -105,6 +104,11 @@ static int tmu_get_temp(struct
> >>>>>>> 
> >>>>>>> thermal_zone_device *tz, int *temp)
> >>>>>>> 
> >>>>>>>>      	 * 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
> >>>>> 
> >>>>>> 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?
> > 
> > That's not what the problem is. It's about zones that *will* be created -
> > since the driver only knows that a thermal zone isn't described in the
> > device tree after it fails registering, it can't enable the site *before*
> > the zone gets registered - so it happens afterwards. That's why it needs
> > this check to not return a bogus initial value before the site gets
> > actually enabled later in qoriq_tmu_register_tmu_zone.
> 
> Sorry, I get the point but I don't see how that can happen:
> 
> qoriq_tmu_register_tmu_zone() calls devm_thermal_of_zone_register() for
> *all* sites regardless if they really exists or not.
> 
> Under the hood, the function devm_thermal_of_zone_register() calls
> thermal_of_zone_register(). This one fails when calling
> of_thermal_zone_find() because it does not exist and returns -ENODEV.
> 
> Hence, the thermal_zone_device_register_with_trips() is not called, the
> thermal zone is not created neither updated.

Again - that's not the case the check is there for. It's there for zones that 
do exist and that do get registered, because REGS_TMR only gets set *after* 
all the zones are already registered (the driver as it is right now does not 
know which sites it should enable before registering the zones). Because of 
that, the first value a zone gets after being registered is always bogus, 
because no monitoring site has been enabled yet at all.

> So I don't understand why the test:
> 
> +	regmap_read(qdata->regmap, REGS_TMR, &val);
> +	if (!(val & TMR_ME))
> +		return -EAGAIN;
> 
> is needed in the get_temp() ops as the thermal zone for this disabled
> site should not exist.
> 
> I'm not putting in question the series, just wanting to avoid a
> potential pointless check in an ops.

It's definitely not pointless (it does workaround a real issue). Is it elegant? 
IMO bringing thermal_zone_of_get_sensor_id back (or doing something 
equivalent) instead would be much cleaner:)

Cheers,
Sebastian

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

[Daniel Lezcano]

On 07/06/2023 19:42, Sebastian Krzyszkowiak wrote:

[ ... ]

>> Hence, the thermal_zone_device_register_with_trips() is not called, the
>> thermal zone is not created neither updated.
> 
> Again - that's not the case the check is there for. It's there for zones that
> do exist and that do get registered, because REGS_TMR only gets set *after*
> all the zones are already registered (the driver as it is right now does not
> know which sites it should enable before registering the zones). Because of
> that, the first value a zone gets after being registered is always bogus,
> because no monitoring site has been enabled yet at all.

Ok, I misunderstood. I thought that was for failing registered thermal zone.

Would enabling the site in ops->change_mode do the trick ?







-- 
<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

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

[Sebastian Krzyszkowiak]

On środa, 7 czerwca 2023 21:10:18 CEST Daniel Lezcano wrote:
> On 07/06/2023 19:42, Sebastian Krzyszkowiak wrote:
> 
> [ ... ]
> 
> >> Hence, the thermal_zone_device_register_with_trips() is not called, the
> >> thermal zone is not created neither updated.
> > 
> > Again - that's not the case the check is there for. It's there for zones
> > that do exist and that do get registered, because REGS_TMR only gets set
> > *after* all the zones are already registered (the driver as it is right
> > now does not know which sites it should enable before registering the
> > zones). Because of that, the first value a zone gets after being
> > registered is always bogus, because no monitoring site has been enabled
> > yet at all.
> 
> Ok, I misunderstood. I thought that was for failing registered thermal zone.
> 
> Would enabling the site in ops->change_mode do the trick ?

Haven't tested, but from a quick glance at the core I believe it should!

Peng, would you like to try that approach out and update this patch?

Thanks,
Sebastian

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

[Peng Fan]

On 6/8/2023 3:10 AM, Daniel Lezcano wrote:
> Caution: This is an external email. Please take care when clicking links 
> or opening attachments. When in doubt, report the message using the 
> 'Report this email' button
> 
> 
> On 07/06/2023 19:42, Sebastian Krzyszkowiak wrote:
> 
> [ ... ]
> 
>>> Hence, the thermal_zone_device_register_with_trips() is not called, the
>>> thermal zone is not created neither updated.
>>
>> Again - that's not the case the check is there for. It's there for 
>> zones that
>> do exist and that do get registered, because REGS_TMR only gets set 
>> *after*
>> all the zones are already registered (the driver as it is right now 
>> does not
>> know which sites it should enable before registering the zones). 
>> Because of
>> that, the first value a zone gets after being registered is always bogus,
>> because no monitoring site has been enabled yet at all.
> 
> Ok, I misunderstood. I thought that was for failing registered thermal 
> zone.
> 
> Would enabling the site in ops->change_mode do the trick ?

No. ops->change_mode not able to do the trick.

devm_thermal_of_zone_register->thermal_zone_device_enable
->thermal_zone_device_set_mode->__thermal_zone_device_update.part.0
->__thermal_zone_get_temp

The thermal_zone_device_set_mode will call change_mode, if return
fail here, the thermal zone will fail to be registered.

Thanks,
Peng.

> 
> 
> 
> 
> 
> 
> 
> -- 
> <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
> 

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

[Sebastian Krzyszkowiak]

On czwartek, 15 czerwca 2023 04:29:01 CEST Peng Fan wrote:
> On 6/8/2023 3:10 AM, Daniel Lezcano wrote:
> >
> > [...]
> >
> > Ok, I misunderstood. I thought that was for failing registered thermal
> > zone.
> > 
> > Would enabling the site in ops->change_mode do the trick ?
> 
> No. ops->change_mode not able to do the trick.
> 
> devm_thermal_of_zone_register->thermal_zone_device_enable
> ->thermal_zone_device_set_mode->__thermal_zone_device_update.part.0
> ->__thermal_zone_get_temp
> 
> The thermal_zone_device_set_mode will call change_mode, if return
> fail here, the thermal zone will fail to be registered.
> 
> Thanks,
> Peng.

I think the idea is not to return a failure in ops->change_mode, but to move 
enabling the site in REGS_TMR/REGS_V2_TMSR register from 
qoriq_tmu_register_tmu_zone to ops->change_mode. This way the site will be 
enabled only for actually existing thermal zones, since those not described in 
the device tree won't reach thermal_zone_device_enable.

S.

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

[Peng Fan]

On 6/15/2023 10:53 AM, Sebastian Krzyszkowiak wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> On czwartek, 15 czerwca 2023 04:29:01 CEST Peng Fan wrote:
>> On 6/8/2023 3:10 AM, Daniel Lezcano wrote:
>>>
>>> [...]
>>>
>>> Ok, I misunderstood. I thought that was for failing registered thermal
>>> zone.
>>>
>>> Would enabling the site in ops->change_mode do the trick ?
>>
>> No. ops->change_mode not able to do the trick.
>>
>> devm_thermal_of_zone_register->thermal_zone_device_enable
>> ->thermal_zone_device_set_mode->__thermal_zone_device_update.part.0
>> ->__thermal_zone_get_temp
>>
>> The thermal_zone_device_set_mode will call change_mode, if return
>> fail here, the thermal zone will fail to be registered.
>>
>> Thanks,
>> Peng.
> 
> I think the idea is not to return a failure in ops->change_mode, but to move
> enabling the site in REGS_TMR/REGS_V2_TMSR register from
> qoriq_tmu_register_tmu_zone to ops->change_mode. 

But qoriq_tmu_register_tmu_zone will finally call ops->change_mode.

And it is per zone, so we not able to enable TMR_ME here.

This way the site will be
> enabled only for actually existing thermal zones, since those not described in
> the device tree won't reach thermal_zone_device_enable.

No. The TMR_ME is the gate for all sites.

Thanks,
Peng.
> 
> S.
> 
> 

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

[Daniel Lezcano]

On 15/06/2023 06:04, Peng Fan wrote:
> 
> 
> On 6/15/2023 10:53 AM, Sebastian Krzyszkowiak wrote:
>> Caution: This is an external email. Please take care when clicking 
>> links or opening attachments. When in doubt, report the message using 
>> the 'Report this email' button
>>
>>
>> On czwartek, 15 czerwca 2023 04:29:01 CEST Peng Fan wrote:
>>> On 6/8/2023 3:10 AM, Daniel Lezcano wrote:
>>>>
>>>> [...]
>>>>
>>>> Ok, I misunderstood. I thought that was for failing registered thermal
>>>> zone.
>>>>
>>>> Would enabling the site in ops->change_mode do the trick ?
>>>
>>> No. ops->change_mode not able to do the trick.
>>>
>>> devm_thermal_of_zone_register->thermal_zone_device_enable
>>> ->thermal_zone_device_set_mode->__thermal_zone_device_update.part.0
>>> ->__thermal_zone_get_temp
>>>
>>> The thermal_zone_device_set_mode will call change_mode, if return
>>> fail here, the thermal zone will fail to be registered.
>>>
>>> Thanks,
>>> Peng.
>>
>> I think the idea is not to return a failure in ops->change_mode, but 
>> to move
>> enabling the site in REGS_TMR/REGS_V2_TMSR register from
>> qoriq_tmu_register_tmu_zone to ops->change_mode. 
> 
> But qoriq_tmu_register_tmu_zone will finally call ops->change_mode.
> 
> And it is per zone, so we not able to enable TMR_ME here.
> 
> This way the site will be
>> enabled only for actually existing thermal zones, since those not 
>> described in
>> the device tree won't reach thermal_zone_device_enable.
> 
> No. The TMR_ME is the gate for all sites.

Is it possible to call:

regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF | sites);
regmap_write(qdata->regmap, REGS_V2_TMSR, sites);

in change_mode:

And after the loop:

regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF_V2);

?


> 
> Thanks,
> Peng.
>>
>> S.
>>
>>

-- 
<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

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

[Daniel Lezcano]

On 15/06/2023 06:04, Peng Fan wrote:
> 
> 
> On 6/15/2023 10:53 AM, Sebastian Krzyszkowiak wrote:
>> Caution: This is an external email. Please take care when clicking 
>> links or opening attachments. When in doubt, report the message using 
>> the 'Report this email' button
>>
>>
>> On czwartek, 15 czerwca 2023 04:29:01 CEST Peng Fan wrote:
>>> On 6/8/2023 3:10 AM, Daniel Lezcano wrote:
>>>>
>>>> [...]
>>>>
>>>> Ok, I misunderstood. I thought that was for failing registered thermal
>>>> zone.
>>>>
>>>> Would enabling the site in ops->change_mode do the trick ?
>>>
>>> No. ops->change_mode not able to do the trick.
>>>
>>> devm_thermal_of_zone_register->thermal_zone_device_enable
>>> ->thermal_zone_device_set_mode->__thermal_zone_device_update.part.0
>>> ->__thermal_zone_get_temp
>>>
>>> The thermal_zone_device_set_mode will call change_mode, if return
>>> fail here, the thermal zone will fail to be registered.
>>>
>>> Thanks,
>>> Peng.
>>
>> I think the idea is not to return a failure in ops->change_mode, but 
>> to move
>> enabling the site in REGS_TMR/REGS_V2_TMSR register from
>> qoriq_tmu_register_tmu_zone to ops->change_mode. 
> 
> But qoriq_tmu_register_tmu_zone will finally call ops->change_mode.
> 
> And it is per zone, so we not able to enable TMR_ME here.
> 
> This way the site will be
>> enabled only for actually existing thermal zones, since those not 
>> described in
>> the device tree won't reach thermal_zone_device_enable.
> 
> No. The TMR_ME is the gate for all sites.

What about the following change on top of your series:

diff --git a/drivers/thermal/qoriq_thermal.c 
b/drivers/thermal/qoriq_thermal.c
index c710449b0c50..ecf88bf13762 100644
--- a/drivers/thermal/qoriq_thermal.c
+++ b/drivers/thermal/qoriq_thermal.c
@@ -107,8 +107,6 @@ static int tmu_get_temp(struct thermal_zone_device 
*tz, int *temp)
  	 */

  	regmap_read(qdata->regmap, REGS_TMR, &val);
-	if (!(val & TMR_ME))
-		return -EAGAIN;

  	if (regmap_read_poll_timeout(qdata->regmap,
  				     REGS_TRITSR(qsensor->id),
@@ -131,14 +129,40 @@ static int tmu_get_temp(struct thermal_zone_device 
*tz, int *temp)
  	return 0;
  }

+static int qoriq_tmu_change_mode(struct thermal_zone_device *tz,
+				 enum thermal_device_mode mode)
+{
+	struct qoriq_sensor *qsensor = thermal_zone_device_priv(tz);
+	struct qoriq_tmu_data *qdata = qoriq_sensor_to_data(qsensor);
+	unsigned int site;
+	unsigned int value;
+	unsigned int mask;
+
+	if (qdata->ver == TMU_VER1) {
+		site = BIT(15 - qsensor->id);
+		mask = TMR_ME | TMR_ALPF | site;
+		value = mode == THERMAL_DEVICE_ENABLED ? mask : mask & ~site;
+		regmap_update_bits(qdata->regmap, REGS_TMR, mask, value);
+	} else {
+		site = BIT(qsensor->id);
+		mask = TMR_ME | TMR_ALPF_V2 | site;
+		value = mode == THERMAL_DEVICE_ENABLED ? mask : mask & ~site;
+		regmap_update_bits(qdata->regmap, REGS_V2_TMSR, mask, value);
+		regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF_V2);
+	}
+
+	return 0;
+}
+
  static const struct thermal_zone_device_ops tmu_tz_ops = {
  	.get_temp = tmu_get_temp,
+	.change_mode = qoriq_tmu_change_mode,
  };

  static int qoriq_tmu_register_tmu_zone(struct device *dev,
  				       struct qoriq_tmu_data *qdata)
  {
-	int id, sites = 0;
+	int id;

  	for (id = 0; id < SITES_MAX; id++) {
  		struct thermal_zone_device *tzd;
@@ -158,25 +182,11 @@ static int qoriq_tmu_register_tmu_zone(struct 
device *dev,
  			return ret;
  		}

-		if (qdata->ver == TMU_VER1)
-			sites |= 0x1 << (15 - id);
-		else
-			sites |= 0x1 << id;
-
  		if (devm_thermal_add_hwmon_sysfs(dev, tzd))
  			dev_warn(dev,
  				 "Failed to add hwmon sysfs attributes\n");
  	}

-	if (sites) {
-		if (qdata->ver == TMU_VER1) {
-			regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF | sites);
-		} else {
-			regmap_write(qdata->regmap, REGS_V2_TMSR, sites);
-			regmap_write(qdata->regmap, REGS_TMR, TMR_ME | TMR_ALPF_V2);
-		}
-	}
-
  	return 0;
  }


-- 
<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

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

[Peng Fan]

> Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported
> sensors
> 
> On 15/06/2023 06:04, Peng Fan wrote:
> >
> >
> > On 6/15/2023 10:53 AM, Sebastian Krzyszkowiak wrote:
> >> Caution: This is an external email. Please take care when clicking
> >> links or opening attachments. When in doubt, report the message using
> >> the 'Report this email' button
> >>
> >>
> >> On czwartek, 15 czerwca 2023 04:29:01 CEST Peng Fan wrote:
> >>> On 6/8/2023 3:10 AM, Daniel Lezcano wrote:
> >>>>
> >>>> [...]
> >>>>
> >>>> Ok, I misunderstood. I thought that was for failing registered
> >>>> thermal zone.
> >>>>
> >>>> Would enabling the site in ops->change_mode do the trick ?
> >>>
> >>> No. ops->change_mode not able to do the trick.
> >>>
> >>> devm_thermal_of_zone_register->thermal_zone_device_enable
> >>> ->thermal_zone_device_set_mode-
> >__thermal_zone_device_update.part.0
> >>> ->__thermal_zone_get_temp
> >>>
> >>> The thermal_zone_device_set_mode will call change_mode, if return
> >>> fail here, the thermal zone will fail to be registered.
> >>>
> >>> Thanks,
> >>> Peng.
> >>
> >> I think the idea is not to return a failure in ops->change_mode, but
> >> to move enabling the site in REGS_TMR/REGS_V2_TMSR register from
> >> qoriq_tmu_register_tmu_zone to ops->change_mode.
> >
> > But qoriq_tmu_register_tmu_zone will finally call ops->change_mode.
> >
> > And it is per zone, so we not able to enable TMR_ME here.
> >
> > This way the site will be
> >> enabled only for actually existing thermal zones, since those not
> >> described in the device tree won't reach thermal_zone_device_enable.
> >
> > No. The TMR_ME is the gate for all sites.
> 
> What about the following change on top of your series:
> 
> diff --git a/drivers/thermal/qoriq_thermal.c
> b/drivers/thermal/qoriq_thermal.c index c710449b0c50..ecf88bf13762
> 100644
> --- a/drivers/thermal/qoriq_thermal.c
> +++ b/drivers/thermal/qoriq_thermal.c
> @@ -107,8 +107,6 @@ static int tmu_get_temp(struct thermal_zone_device
> *tz, int *temp)
>   	 */
> 
>   	regmap_read(qdata->regmap, REGS_TMR, &val);
> -	if (!(val & TMR_ME))
> -		return -EAGAIN;
> 
>   	if (regmap_read_poll_timeout(qdata->regmap,
>   				     REGS_TRITSR(qsensor->id),
> @@ -131,14 +129,40 @@ static int tmu_get_temp(struct
> thermal_zone_device *tz, int *temp)
>   	return 0;
>   }
> 
> +static int qoriq_tmu_change_mode(struct thermal_zone_device *tz,
> +				 enum thermal_device_mode mode)
> +{
> +	struct qoriq_sensor *qsensor = thermal_zone_device_priv(tz);
> +	struct qoriq_tmu_data *qdata = qoriq_sensor_to_data(qsensor);
> +	unsigned int site;
> +	unsigned int value;
> +	unsigned int mask;
> +
> +	if (qdata->ver == TMU_VER1) {
> +		site = BIT(15 - qsensor->id);
> +		mask = TMR_ME | TMR_ALPF | site;
> +		value = mode == THERMAL_DEVICE_ENABLED ? mask : mask
> & ~site;
> +		regmap_update_bits(qdata->regmap, REGS_TMR, mask,
> value);
> +	} else {
> +		site = BIT(qsensor->id);
> +		mask = TMR_ME | TMR_ALPF_V2 | site;
> +		value = mode == THERMAL_DEVICE_ENABLED ? mask : mask
> & ~site;
> +		regmap_update_bits(qdata->regmap, REGS_V2_TMSR,
> mask, value);
> +		regmap_write(qdata->regmap, REGS_TMR, TMR_ME |
> TMR_ALPF_V2);

Per i.MX8MQ Reference manual:
MSITE:
Monitoring site select 0 - 2. By setting the select bit for a temperature sensor site,
 it is enabled and included in all monitoring functions. For proper operation, this
field should only change when monitoring is
disabled. If no site is selected, site 0 is monitored by default.

ME: Before enabling the TMU for monitoring, the TMU must be configured, 
see section Initialization Information. Failure to properly initialize the
configuration table may result in boundedly undefined
behavior.

So we must set the SITEs bits before enabling ME bit. So set TMR_ME when
each time call invoke mode violates the spec.

As I understand, change_mode is per zone, which means per msite for TMU,
but TMU_ME is a global gating bit which should not be set before all msites
are set.

Thanks,
Peng.

> +	}
> +
> +	return 0;
> +}
> +
>   static const struct thermal_zone_device_ops tmu_tz_ops = {
>   	.get_temp = tmu_get_temp,
> +	.change_mode = qoriq_tmu_change_mode,
>   };
> 
>   static int qoriq_tmu_register_tmu_zone(struct device *dev,
>   				       struct qoriq_tmu_data *qdata)
>   {
> -	int id, sites = 0;
> +	int id;
> 
>   	for (id = 0; id < SITES_MAX; id++) {
>   		struct thermal_zone_device *tzd;
> @@ -158,25 +182,11 @@ static int qoriq_tmu_register_tmu_zone(struct
> device *dev,
>   			return ret;
>   		}
> 
> -		if (qdata->ver == TMU_VER1)
> -			sites |= 0x1 << (15 - id);
> -		else
> -			sites |= 0x1 << id;
> -
>   		if (devm_thermal_add_hwmon_sysfs(dev, tzd))
>   			dev_warn(dev,
>   				 "Failed to add hwmon sysfs attributes\n");
>   	}
> 
> -	if (sites) {
> -		if (qdata->ver == TMU_VER1) {
> -			regmap_write(qdata->regmap, REGS_TMR,
> TMR_ME | TMR_ALPF | sites);
> -		} else {
> -			regmap_write(qdata->regmap, REGS_V2_TMSR,
> sites);
> -			regmap_write(qdata->regmap, REGS_TMR,
> TMR_ME | TMR_ALPF_V2);
> -		}
> -	}
> -
>   	return 0;
>   }
> 
> 
> --
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.linaro.org%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C5c60b05b18b
> 9442223cc08db6d967d30%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C638224265339069492%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%
> 7C%7C%7C&sdata=StV3sHe7fjGIMNjEYaDwiuTm8GpB6IDmoRhKoRpmUQY
> %3D&reserved=0> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.facebook.com%2Fpages%2FLinaro&data=05%7C01%7Cpeng.fan%40nxp.c
> om%7C5c60b05b18b9442223cc08db6d967d30%7C686ea1d3bc2b4c6fa92cd
> 99c5c301635%7C0%7C0%7C638224265339069492%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C3000%7C%7C%7C&sdata=AjK1yxq5PT60YfQRlxOBzlM2YXnhl6
> 9NWCQFRIUwVm0%3D&reserved=0> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitt
> er.com%2F%23!%2Flinaroorg&data=05%7C01%7Cpeng.fan%40nxp.com%7C
> 5c60b05b18b9442223cc08db6d967d30%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C638224265339069492%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000%7C%7C%7C&sdata=ViShZhpql%2FJ6xUGJ2M9acJtmKuG%2BFCyZ
> Ivy99cPpfxA%3D&reserved=0> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.linaro.org%2Flinaro-
> blog%2F&data=05%7C01%7Cpeng.fan%40nxp.com%7C5c60b05b18b944222
> 3cc08db6d967d30%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 38224265339069492%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%
> 7C&sdata=PCNIqUJPZve9x1sgsdkPtRjIKQXnr514I%2BUx%2FbLcGC0%3D&res
> erved=0> Blog

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

[Daniel Lezcano]

On 15/06/2023 14:07, Peng Fan wrote:

[ ... ]

> Per i.MX8MQ Reference manual:
> MSITE:
> Monitoring site select 0 - 2. By setting the select bit for a temperature sensor site,
>   it is enabled and included in all monitoring functions. For proper operation, this
> field should only change when monitoring is
> disabled. If no site is selected, site 0 is monitored by default.
> 
> ME: Before enabling the TMU for monitoring, the TMU must be configured,
> see section Initialization Information. Failure to properly initialize the
> configuration table may result in boundedly undefined
> behavior.
> 
> So we must set the SITEs bits before enabling ME bit. So set TMR_ME when
> each time call invoke mode violates the spec.
> 
> As I understand, change_mode is per zone, which means per msite for TMU,
> but TMU_ME is a global gating bit which should not be set before all msites
> are set.

Mmh, IIUC correctly the documentation, it says the monitoring must be 
disabled when changing the sites. So in the proposed code, we shall 
disable the TMU, update the site and then enable the TMU.

Can you give a try to see if that works? If yes, then can you submit a 
patch on top of this series. Meanwhile, I'll pick those changes.

Thanks
   -- Daniel


-- 
<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

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

[Peng Fan]

On 6/15/2023 9:49 PM, Daniel Lezcano wrote:
> Caution: This is an external email. Please take care when clicking links 
> or opening attachments. When in doubt, report the message using the 
> 'Report this email' button
> 
> 
> On 15/06/2023 14:07, Peng Fan wrote:
> 
> [ ... ]
> 
>> Per i.MX8MQ Reference manual:
>> MSITE:
>> Monitoring site select 0 - 2. By setting the select bit for a 
>> temperature sensor site,
>>   it is enabled and included in all monitoring functions. For proper 
>> operation, this
>> field should only change when monitoring is
>> disabled. If no site is selected, site 0 is monitored by default.
>>
>> ME: Before enabling the TMU for monitoring, the TMU must be configured,
>> see section Initialization Information. Failure to properly initialize 
>> the
>> configuration table may result in boundedly undefined
>> behavior.
>>
>> So we must set the SITEs bits before enabling ME bit. So set TMR_ME when
>> each time call invoke mode violates the spec.
>>
>> As I understand, change_mode is per zone, which means per msite for TMU,
>> but TMU_ME is a global gating bit which should not be set before all 
>> msites
>> are set.
> 
> Mmh, IIUC correctly the documentation, it says the monitoring must be
> disabled when changing the sites. So in the proposed code, we shall
> disable the TMU, update the site and then enable the TMU.
> 
> Can you give a try to see if that works? If yes, then can you submit a
> patch on top of this series. Meanwhile, I'll pick those changes.


I did a basic test on i.MX8MQ, it seems work. But I still have concern
because it volitates the spec, need disable TMU before updating MSITE.

And if we disable TMU when updating MSITE in your patch, there is 
potential risk that zone0 is reading temperature, while we disable TMU
and update MSITE for zone1. So zone0 may get invalid temperature because
TMU is disabled at this window.

no good idea from my side, unless we keep check TMU_ME when
getting temperature.

Thanks,
Peng.

> 
> Thanks
>    -- Daniel
> 
> 
> -- 
> <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
> 

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

[Daniel Lezcano]

On 16/06/2023 03:06, Peng Fan wrote:
> 
> 
> On 6/15/2023 9:49 PM, Daniel Lezcano wrote:
>> Caution: This is an external email. Please take care when clicking 
>> links or opening attachments. When in doubt, report the message using 
>> the 'Report this email' button
>>
>>
>> On 15/06/2023 14:07, Peng Fan wrote:
>>
>> [ ... ]
>>
>>> Per i.MX8MQ Reference manual:
>>> MSITE:
>>> Monitoring site select 0 - 2. By setting the select bit for a 
>>> temperature sensor site,
>>>   it is enabled and included in all monitoring functions. For proper 
>>> operation, this
>>> field should only change when monitoring is
>>> disabled. If no site is selected, site 0 is monitored by default.
>>>
>>> ME: Before enabling the TMU for monitoring, the TMU must be configured,
>>> see section Initialization Information. Failure to properly 
>>> initialize the
>>> configuration table may result in boundedly undefined
>>> behavior.
>>>
>>> So we must set the SITEs bits before enabling ME bit. So set TMR_ME when
>>> each time call invoke mode violates the spec.
>>>
>>> As I understand, change_mode is per zone, which means per msite for TMU,
>>> but TMU_ME is a global gating bit which should not be set before all 
>>> msites
>>> are set.
>>
>> Mmh, IIUC correctly the documentation, it says the monitoring must be
>> disabled when changing the sites. So in the proposed code, we shall
>> disable the TMU, update the site and then enable the TMU.
>>
>> Can you give a try to see if that works? If yes, then can you submit a
>> patch on top of this series. Meanwhile, I'll pick those changes.
> 
> 
> I did a basic test on i.MX8MQ, it seems work. But I still have concern
> because it volitates the spec, need disable TMU before updating MSITE.
> 
> And if we disable TMU when updating MSITE in your patch, there is 
> potential risk that zone0 is reading temperature, while we disable TMU
> and update MSITE for zone1. So zone0 may get invalid temperature because
> TMU is disabled at this window.
> 
> no good idea from my side, unless we keep check TMU_ME when
> getting temperature.

Yeah, that is a good point. We are ending up to do the TMU check in the 
get_temp() any. Thanks for taking the time to look it up.

   -- Daniel

-- 
<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

Re: [PATCH 0/3] thermal: qoriq_thermal: support TMU 2.1

[Daniel Lezcano]

On 16/05/2023 10:37, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX93 use same TMU IP as Qoriq, but with version 2.1, so update
> the driver to support 2.1.
> 
> This patch also includes a fix for i.MX8MQ
> 
> Pankit Garg (1):
>    thermal: qoriq_thermal: No need to program site adjustment register
> 
> Peng Fan (2):
>    thermal: qoriq_thermal: only enable supported sensors
>    thermal: qoriq: support version 2.1
> 
>   drivers/thermal/qoriq_thermal.c | 48 ++++++++++++++++++++-------------
>   1 file changed, 29 insertions(+), 19 deletions(-)

Applied, thanks


-- 
<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