From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Peng Fan <peng.fan@nxp.com>,
"Peng Fan (OSS)" <peng.fan@oss.nxp.com>,
"rafael@kernel.org" <rafael@kernel.org>,
"shawnguo@kernel.org" <shawnguo@kernel.org>,
"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>
Cc: "amitk@kernel.org" <amitk@kernel.org>,
"rui.zhang@intel.com" <rui.zhang@intel.com>,
"andrew.smirnov@gmail.com" <andrew.smirnov@gmail.com>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kernel@pengutronix.de" <kernel@pengutronix.de>,
"festevam@gmail.com" <festevam@gmail.com>,
dl-linux-imx <linux-imx@nxp.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
Alice Guo <alice.guo@nxp.com>
Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported sensors
Date: Fri, 2 Jun 2023 15:11:37 +0200 [thread overview]
Message-ID: <0eb717ac-82b1-8a76-58a2-394167e69b28@linaro.org> (raw)
In-Reply-To: <DU0PR04MB9417D574603B54AEF76480AE88499@DU0PR04MB9417.eurprd04.prod.outlook.com>
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
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Peng Fan <peng.fan@nxp.com>,
"Peng Fan (OSS)" <peng.fan@oss.nxp.com>,
"rafael@kernel.org" <rafael@kernel.org>,
"shawnguo@kernel.org" <shawnguo@kernel.org>,
"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>
Cc: "amitk@kernel.org" <amitk@kernel.org>,
"rui.zhang@intel.com" <rui.zhang@intel.com>,
"andrew.smirnov@gmail.com" <andrew.smirnov@gmail.com>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"kernel@pengutronix.de" <kernel@pengutronix.de>,
"festevam@gmail.com" <festevam@gmail.com>,
dl-linux-imx <linux-imx@nxp.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
Alice Guo <alice.guo@nxp.com>
Subject: Re: [PATCH 2/3] thermal: qoriq_thermal: only enable supported sensors
Date: Fri, 2 Jun 2023 15:11:37 +0200 [thread overview]
Message-ID: <0eb717ac-82b1-8a76-58a2-394167e69b28@linaro.org> (raw)
In-Reply-To: <DU0PR04MB9417D574603B54AEF76480AE88499@DU0PR04MB9417.eurprd04.prod.outlook.com>
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-06-02 13:12 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-16 8:37 [PATCH 0/3] thermal: qoriq_thermal: support TMU 2.1 Peng Fan (OSS)
2023-05-16 8:37 ` Peng Fan (OSS)
2023-05-16 8:37 ` [PATCH 1/3] thermal: qoriq_thermal: No need to program site adjustment register Peng Fan (OSS)
2023-05-16 8:37 ` Peng Fan (OSS)
2023-05-16 8:37 ` [PATCH 2/3] thermal: qoriq_thermal: only enable supported sensors Peng Fan (OSS)
2023-05-16 8:37 ` Peng Fan (OSS)
2023-05-31 11:03 ` Daniel Lezcano
2023-05-31 11:03 ` Daniel Lezcano
2023-05-31 12:05 ` Peng Fan
2023-05-31 12:05 ` Peng Fan
2023-05-31 12:35 ` Daniel Lezcano
2023-05-31 12:35 ` Daniel Lezcano
2023-05-31 13:00 ` Peng Fan
2023-05-31 13:00 ` Peng Fan
2023-06-01 9:52 ` Peng Fan
2023-06-01 9:52 ` Peng Fan
2023-06-02 13:11 ` Daniel Lezcano [this message]
2023-06-02 13:11 ` Daniel Lezcano
2023-06-07 6:01 ` Sebastian Krzyszkowiak
2023-06-07 6:01 ` Sebastian Krzyszkowiak
2023-06-07 8:28 ` Daniel Lezcano
2023-06-07 8:28 ` Daniel Lezcano
2023-06-07 17:42 ` Sebastian Krzyszkowiak
2023-06-07 17:42 ` Sebastian Krzyszkowiak
2023-06-07 19:10 ` Daniel Lezcano
2023-06-07 19:10 ` Daniel Lezcano
2023-06-12 6:23 ` Sebastian Krzyszkowiak
2023-06-15 2:29 ` Peng Fan
2023-06-15 2:29 ` Peng Fan
2023-06-15 2:53 ` Sebastian Krzyszkowiak
2023-06-15 2:53 ` Sebastian Krzyszkowiak
2023-06-15 4:04 ` Peng Fan
2023-06-15 4:04 ` Peng Fan
2023-06-15 10:36 ` Daniel Lezcano
2023-06-15 10:36 ` Daniel Lezcano
2023-06-15 11:48 ` Daniel Lezcano
2023-06-15 11:48 ` Daniel Lezcano
2023-06-15 12:07 ` Peng Fan
2023-06-15 12:07 ` Peng Fan
2023-06-15 13:49 ` Daniel Lezcano
2023-06-15 13:49 ` Daniel Lezcano
2023-06-16 1:06 ` Peng Fan
2023-06-16 1:06 ` Peng Fan
2023-06-16 9:01 ` Daniel Lezcano
2023-06-16 9:01 ` Daniel Lezcano
2023-05-16 8:37 ` [PATCH 3/3] thermal: qoriq: support version 2.1 Peng Fan (OSS)
2023-05-16 8:37 ` Peng Fan (OSS)
2023-05-23 1:59 ` [PATCH 0/3] thermal: qoriq_thermal: support TMU 2.1 Alice Guo
2023-05-23 1:59 ` Alice Guo
2023-05-29 3:35 ` Peng Fan
2023-05-29 3:35 ` Peng Fan
2023-06-16 9:02 ` Daniel Lezcano
2023-06-16 9:02 ` Daniel Lezcano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0eb717ac-82b1-8a76-58a2-394167e69b28@linaro.org \
--to=daniel.lezcano@linaro.org \
--cc=alice.guo@nxp.com \
--cc=amitk@kernel.org \
--cc=andrew.smirnov@gmail.com \
--cc=festevam@gmail.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=peng.fan@nxp.com \
--cc=peng.fan@oss.nxp.com \
--cc=rafael@kernel.org \
--cc=rui.zhang@intel.com \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.