* Re: [PATCH] thermal: rockchip: shut up GRF warning
2025-08-18 17:26 [PATCH] thermal: rockchip: shut up GRF warning Sebastian Reichel
@ 2025-08-18 18:44 ` Heiko Stübner
2025-08-18 19:23 ` Sebastian Reichel
2025-08-19 10:19 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Heiko Stübner @ 2025-08-18 18:44 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
Sebastian Reichel
Cc: linux-pm, linux-arm-kernel, linux-rockchip, linux-kernel, kernel,
Sebastian Reichel
Hi Sebastian,
Am Montag, 18. August 2025, 19:26:15 Mitteleuropäische Sommerzeit schrieb Sebastian Reichel:
> Most of the recent Rockchip devices do not have a GRF associated
> with the tsadc IP. Let's avoid printing a warning on those devices.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
thanks a lot for tracking down the GRF usage for all the soc variants :-)
> ---
> drivers/thermal/rockchip_thermal.c | 53 +++++++++++++++++++++++++++++++++-----
> 1 file changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index 3beff9b6fac3abe8948b56132b618ff1bed57217..1e8091cebd6673ab39fa0c4dee835c68aeb7e8b5 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -1099,6 +1114,8 @@ static const struct rockchip_tsadc_chip px30_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* 2 channels for tsadc */
>
> + .grf_mode = GRF_MANDATORY,
> +
> .tshut_mode = TSHUT_MODE_CRU, /* default TSHUT via CRU */
> .tshut_temp = 95000,
>
> @@ -1123,6 +1140,8 @@ static const struct rockchip_tsadc_chip rv1108_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 1, /* one channel for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
nit: I guess instead of adding an empty line, you could also just drop
the empty line above, to bring the "older" variants into the form
rk3576 and rk3588 use.
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
[...]
> @@ -1321,6 +1354,7 @@ static const struct rockchip_tsadc_chip rk3576_tsadc_data = {
> /* top, big_core, little_core, ddr, npu, gpu */
> .chn_offset = 0,
> .chn_num = 6, /* six channels for tsadc */
> + .grf_mode = GRF_NONE,
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1345,6 +1379,7 @@ static const struct rockchip_tsadc_chip rk3588_tsadc_data = {
> /* top, big_core0, big_core1, little_core, center, gpu, npu */
> .chn_offset = 0,
> .chn_num = 7, /* seven channels for tsadc */
> + .grf_mode = GRF_NONE,
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
[...]
> @@ -1621,12 +1656,16 @@ static int rockchip_configure_from_dt(struct device *dev,
> return -EINVAL;
> }
>
> - /* The tsadc wont to handle the error in here since some SoCs didn't
> - * need this property.
> - */
> - thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> - if (IS_ERR(thermal->grf))
> - dev_warn(dev, "Missing rockchip,grf property\n");
> + if (thermal->chip->grf_mode != GRF_NONE) {
> + thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> + if (IS_ERR(thermal->grf)) {
> + ret = PTR_ERR(thermal->grf);
> + if (thermal->chip->grf_mode == GRF_OPTIONAL)
> + dev_warn(dev, "Missing rockchip,grf property\n");
I guess it might make it easier for people seeing the log, if we could
insert an "optional" into that message for the optional tier.
> + else
> + return dev_err_probe(dev, ret, "Missing rockchip,grf property\n");
> + }
> + }
>
> rockchip_get_trim_configuration(dev, np, thermal);
Overall, though
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Thanks
Heiko
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] thermal: rockchip: shut up GRF warning
2025-08-18 18:44 ` Heiko Stübner
@ 2025-08-18 19:23 ` Sebastian Reichel
2025-08-18 19:47 ` Heiko Stübner
0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Reichel @ 2025-08-18 19:23 UTC (permalink / raw)
To: Heiko Stübner
Cc: Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
linux-pm, linux-arm-kernel, linux-rockchip, linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 4280 bytes --]
Hi,
On Mon, Aug 18, 2025 at 08:44:15PM +0200, Heiko Stübner wrote:
> Hi Sebastian,
>
> Am Montag, 18. August 2025, 19:26:15 Mitteleuropäische Sommerzeit schrieb Sebastian Reichel:
> > Most of the recent Rockchip devices do not have a GRF associated
> > with the tsadc IP. Let's avoid printing a warning on those devices.
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
>
> thanks a lot for tracking down the GRF usage for all the soc variants :-)
>
> > ---
> > drivers/thermal/rockchip_thermal.c | 53 +++++++++++++++++++++++++++++++++-----
> > 1 file changed, 46 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> > index 3beff9b6fac3abe8948b56132b618ff1bed57217..1e8091cebd6673ab39fa0c4dee835c68aeb7e8b5 100644
> > --- a/drivers/thermal/rockchip_thermal.c
> > +++ b/drivers/thermal/rockchip_thermal.c
> > @@ -1099,6 +1114,8 @@ static const struct rockchip_tsadc_chip px30_tsadc_data = {
> > .chn_offset = 0,
> > .chn_num = 2, /* 2 channels for tsadc */
> >
> > + .grf_mode = GRF_MANDATORY,
> > +
> > .tshut_mode = TSHUT_MODE_CRU, /* default TSHUT via CRU */
> > .tshut_temp = 95000,
> >
> > @@ -1123,6 +1140,8 @@ static const struct rockchip_tsadc_chip rv1108_tsadc_data = {
> > .chn_offset = 0,
> > .chn_num = 1, /* one channel for tsadc */
> >
> > + .grf_mode = GRF_NONE,
> > +
>
> nit: I guess instead of adding an empty line, you could also just drop
> the empty line above, to bring the "older" variants into the form
> rk3576 and rk3588 use.
Ack.
>
> > .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> > .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> > .tshut_temp = 95000,
>
> [...]
>
> > @@ -1321,6 +1354,7 @@ static const struct rockchip_tsadc_chip rk3576_tsadc_data = {
> > /* top, big_core, little_core, ddr, npu, gpu */
> > .chn_offset = 0,
> > .chn_num = 6, /* six channels for tsadc */
> > + .grf_mode = GRF_NONE,
> > .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> > .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> > .tshut_temp = 95000,
> > @@ -1345,6 +1379,7 @@ static const struct rockchip_tsadc_chip rk3588_tsadc_data = {
> > /* top, big_core0, big_core1, little_core, center, gpu, npu */
> > .chn_offset = 0,
> > .chn_num = 7, /* seven channels for tsadc */
> > + .grf_mode = GRF_NONE,
> > .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> > .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> > .tshut_temp = 95000,
>
> [...]
>
> > @@ -1621,12 +1656,16 @@ static int rockchip_configure_from_dt(struct device *dev,
> > return -EINVAL;
> > }
> >
> > - /* The tsadc wont to handle the error in here since some SoCs didn't
> > - * need this property.
> > - */
> > - thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> > - if (IS_ERR(thermal->grf))
> > - dev_warn(dev, "Missing rockchip,grf property\n");
> > + if (thermal->chip->grf_mode != GRF_NONE) {
> > + thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> > + if (IS_ERR(thermal->grf)) {
> > + ret = PTR_ERR(thermal->grf);
> > + if (thermal->chip->grf_mode == GRF_OPTIONAL)
> > + dev_warn(dev, "Missing rockchip,grf property\n");
>
> I guess it might make it easier for people seeing the log, if we could
> insert an "optional" into that message for the optional tier.
Sure, I can add an "optional". I'm not sure how "optional" they
really are, though. Code like this looks quite fishy to me:
if (grf)
regmap_write(grf, ..., RK3568_GRF_TSADC_TSEN);
I marked these as optional, as the driver should probe without the
GRF. But to me it looks like most platforms with optional GRF
support should have been made mandatory in the first place.
Greetings,
-- Sebastian
>
> > + else
> > + return dev_err_probe(dev, ret, "Missing rockchip,grf property\n");
> > + }
> > + }
> >
> > rockchip_get_trim_configuration(dev, np, thermal);
>
> Overall, though
>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Thanks.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] thermal: rockchip: shut up GRF warning
2025-08-18 19:23 ` Sebastian Reichel
@ 2025-08-18 19:47 ` Heiko Stübner
0 siblings, 0 replies; 9+ messages in thread
From: Heiko Stübner @ 2025-08-18 19:47 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
linux-pm, linux-arm-kernel, linux-rockchip, linux-kernel, kernel
Am Montag, 18. August 2025, 21:23:54 Mitteleuropäische Sommerzeit schrieb Sebastian Reichel:
> On Mon, Aug 18, 2025 at 08:44:15PM +0200, Heiko Stübner wrote:
> > Am Montag, 18. August 2025, 19:26:15 Mitteleuropäische Sommerzeit schrieb Sebastian Reichel:
> > > @@ -1621,12 +1656,16 @@ static int rockchip_configure_from_dt(struct device *dev,
> > > return -EINVAL;
> > > }
> > >
> > > - /* The tsadc wont to handle the error in here since some SoCs didn't
> > > - * need this property.
> > > - */
> > > - thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> > > - if (IS_ERR(thermal->grf))
> > > - dev_warn(dev, "Missing rockchip,grf property\n");
> > > + if (thermal->chip->grf_mode != GRF_NONE) {
> > > + thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> > > + if (IS_ERR(thermal->grf)) {
> > > + ret = PTR_ERR(thermal->grf);
> > > + if (thermal->chip->grf_mode == GRF_OPTIONAL)
> > > + dev_warn(dev, "Missing rockchip,grf property\n");
> >
> > I guess it might make it easier for people seeing the log, if we could
> > insert an "optional" into that message for the optional tier.
>
> Sure, I can add an "optional". I'm not sure how "optional" they
> really are, though. Code like this looks quite fishy to me:
>
> if (grf)
> regmap_write(grf, ..., RK3568_GRF_TSADC_TSEN);
>
> I marked these as optional, as the driver should probe without the
> GRF. But to me it looks like most platforms with optional GRF
> support should have been made mandatory in the first place.
okay ... then I take it back, and we should leave out the optional.
That way people may wonder and possibly verify their soc variant
or build setup.
Heiko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] thermal: rockchip: shut up GRF warning
2025-08-18 17:26 [PATCH] thermal: rockchip: shut up GRF warning Sebastian Reichel
2025-08-18 18:44 ` Heiko Stübner
@ 2025-08-19 10:19 ` kernel test robot
2025-08-19 12:42 ` Robin Murphy
2025-08-20 14:23 ` Diederik de Haas
3 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2025-08-19 10:19 UTC (permalink / raw)
To: Sebastian Reichel, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Heiko Stuebner
Cc: llvm, oe-kbuild-all, linux-pm, linux-arm-kernel, linux-rockchip,
linux-kernel, kernel, Sebastian Reichel
Hi Sebastian,
kernel test robot noticed the following build warnings:
[auto build test WARNING on c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9]
url: https://github.com/intel-lab-lkp/linux/commits/Sebastian-Reichel/thermal-rockchip-shut-up-GRF-warning/20250819-012807
base: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
patch link: https://lore.kernel.org/r/20250818-thermal-rockchip-grf-warning-v1-1-134152c97097%40kernel.org
patch subject: [PATCH] thermal: rockchip: shut up GRF warning
config: riscv-randconfig-001-20250819 (https://download.01.org/0day-ci/archive/20250819/202508191909.EIZEWxiJ-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 93d24b6b7b148c47a2fa228a4ef31524fa1d9f3f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250819/202508191909.EIZEWxiJ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508191909.EIZEWxiJ-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> Warning: drivers/thermal/rockchip_thermal.c:133 struct member 'grf_mode' not described in 'rockchip_tsadc_chip'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] thermal: rockchip: shut up GRF warning
2025-08-18 17:26 [PATCH] thermal: rockchip: shut up GRF warning Sebastian Reichel
2025-08-18 18:44 ` Heiko Stübner
2025-08-19 10:19 ` kernel test robot
@ 2025-08-19 12:42 ` Robin Murphy
2025-08-19 13:56 ` Sebastian Reichel
2025-08-20 14:23 ` Diederik de Haas
3 siblings, 1 reply; 9+ messages in thread
From: Robin Murphy @ 2025-08-19 12:42 UTC (permalink / raw)
To: Sebastian Reichel, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Heiko Stuebner
Cc: linux-pm, linux-arm-kernel, linux-rockchip, linux-kernel, kernel
On 18/08/2025 6:26 pm, Sebastian Reichel wrote:
> Most of the recent Rockchip devices do not have a GRF associated
> with the tsadc IP. Let's avoid printing a warning on those devices.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> drivers/thermal/rockchip_thermal.c | 53 +++++++++++++++++++++++++++++++++-----
> 1 file changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index 3beff9b6fac3abe8948b56132b618ff1bed57217..1e8091cebd6673ab39fa0c4dee835c68aeb7e8b5 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -50,6 +50,18 @@ enum adc_sort_mode {
> ADC_INCREMENT,
> };
>
> +/*
> + * The GRF availability depends on the specific SoC
> + * GRF_NONE: the SoC does not have a GRF associated with the tsadc
> + * GRF_OPTIONAL: the SoC has a GRF, but the driver can work without it
> + * GRF_MANDATORY: the SoC has a GRF and it is required for proper operation
> + */
> +enum tsadc_grf_mode {
> + GRF_NONE,
> + GRF_OPTIONAL,
> + GRF_MANDATORY,
> +};
> +
> #include "thermal_hwmon.h"
>
> /**
> @@ -97,6 +109,9 @@ struct rockchip_tsadc_chip {
> enum tshut_mode tshut_mode;
> enum tshut_polarity tshut_polarity;
>
> + /* GRF availability */
> + enum tsadc_grf_mode grf_mode;
> +
> /* Chip-wide methods */
> void (*initialize)(struct regmap *grf,
> void __iomem *reg, enum tshut_polarity p);
> @@ -1099,6 +1114,8 @@ static const struct rockchip_tsadc_chip px30_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* 2 channels for tsadc */
>
> + .grf_mode = GRF_MANDATORY,
> +
> .tshut_mode = TSHUT_MODE_CRU, /* default TSHUT via CRU */
> .tshut_temp = 95000,
>
> @@ -1123,6 +1140,8 @@ static const struct rockchip_tsadc_chip rv1108_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 1, /* one channel for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1148,6 +1167,8 @@ static const struct rockchip_tsadc_chip rk3228_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 1, /* one channel for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1173,6 +1194,8 @@ static const struct rockchip_tsadc_chip rk3288_tsadc_data = {
> .chn_offset = 1,
> .chn_num = 2, /* two channels for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1198,6 +1221,8 @@ static const struct rockchip_tsadc_chip rk3328_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 1, /* one channels for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
> .tshut_mode = TSHUT_MODE_CRU, /* default TSHUT via CRU */
> .tshut_temp = 95000,
>
> @@ -1222,6 +1247,8 @@ static const struct rockchip_tsadc_chip rk3366_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* two channels for tsadc */
>
> + .grf_mode = GRF_OPTIONAL,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1247,6 +1274,8 @@ static const struct rockchip_tsadc_chip rk3368_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* two channels for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1272,6 +1301,8 @@ static const struct rockchip_tsadc_chip rk3399_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* two channels for tsadc */
>
> + .grf_mode = GRF_OPTIONAL,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1297,6 +1328,8 @@ static const struct rockchip_tsadc_chip rk3568_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* two channels for tsadc */
>
> + .grf_mode = GRF_OPTIONAL,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1321,6 +1354,7 @@ static const struct rockchip_tsadc_chip rk3576_tsadc_data = {
> /* top, big_core, little_core, ddr, npu, gpu */
> .chn_offset = 0,
> .chn_num = 6, /* six channels for tsadc */
> + .grf_mode = GRF_NONE,
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1345,6 +1379,7 @@ static const struct rockchip_tsadc_chip rk3588_tsadc_data = {
> /* top, big_core0, big_core1, little_core, center, gpu, npu */
> .chn_offset = 0,
> .chn_num = 7, /* seven channels for tsadc */
> + .grf_mode = GRF_NONE,
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1572,7 +1607,7 @@ static int rockchip_configure_from_dt(struct device *dev,
> struct device_node *np,
> struct rockchip_thermal_data *thermal)
> {
> - u32 shut_temp, tshut_mode, tshut_polarity;
> + u32 shut_temp, tshut_mode, tshut_polarity, ret;
>
> if (of_property_read_u32(np, "rockchip,hw-tshut-temp", &shut_temp)) {
> dev_warn(dev,
> @@ -1621,12 +1656,16 @@ static int rockchip_configure_from_dt(struct device *dev,
> return -EINVAL;
> }
>
> - /* The tsadc wont to handle the error in here since some SoCs didn't
> - * need this property.
> - */
> - thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> - if (IS_ERR(thermal->grf))
> - dev_warn(dev, "Missing rockchip,grf property\n");
> + if (thermal->chip->grf_mode != GRF_NONE) {
> + thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> + if (IS_ERR(thermal->grf)) {
> + ret = PTR_ERR(thermal->grf);
> + if (thermal->chip->grf_mode == GRF_OPTIONAL)
> + dev_warn(dev, "Missing rockchip,grf property\n");
> + else
> + return dev_err_probe(dev, ret, "Missing rockchip,grf property\n");
> + }
> + }
Nit: Does the lookup itself need to be made conditional? I think I'd
also agree that the "optional" mode seems suspect, so potentially it
could be a whole lot simpler, e.g.:
thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
if (IS_ERR(thermal->grf) && thermal->chip->grf_required)
return dev_err_probe(dev, PTR_ERR(thermal->grf),
"Missing rockchip,grf property\n");
Thanks,
Robin.
>
> rockchip_get_trim_configuration(dev, np, thermal);
>
>
> ---
> base-commit: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
> change-id: 20250818-thermal-rockchip-grf-warning-05f7f56286a2
>
> Best regards,
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] thermal: rockchip: shut up GRF warning
2025-08-19 12:42 ` Robin Murphy
@ 2025-08-19 13:56 ` Sebastian Reichel
2025-08-19 14:13 ` Heiko Stübner
0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Reichel @ 2025-08-19 13:56 UTC (permalink / raw)
To: Robin Murphy
Cc: Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
Heiko Stuebner, linux-pm, linux-arm-kernel, linux-rockchip,
linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]
Hello Robin,
On Tue, Aug 19, 2025 at 01:42:42PM +0100, Robin Murphy wrote:
> > + if (thermal->chip->grf_mode != GRF_NONE) {
> > + thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> > + if (IS_ERR(thermal->grf)) {
> > + ret = PTR_ERR(thermal->grf);
> > + if (thermal->chip->grf_mode == GRF_OPTIONAL)
> > + dev_warn(dev, "Missing rockchip,grf property\n");
> > + else
> > + return dev_err_probe(dev, ret, "Missing rockchip,grf property\n");
> > + }
> > + }
>
> Nit: Does the lookup itself need to be made conditional? I think I'd
> also agree that the "optional" mode seems suspect, so potentially it
> could be a whole lot simpler, e.g.:
>
> thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> if (IS_ERR(thermal->grf) && thermal->chip->grf_required)
> return dev_err_probe(dev, PTR_ERR(thermal->grf),
> "Missing rockchip,grf property\n");
I came up with the enum, because I think most platforms having
"optional" GRF support actually require it, so I want to keep the
warning. At the same time I don't want to mark them as GRF required
at this point, since that is potentially a DT ABI break. It really
needs to be checked per platform in the TRM and/or by testing on
real HW. With this patch we can easily handle this platform by
platform by moving them from GRF_OPTIONAL to GRF_MANDATORY without
affecting the unchecked platforms. Also switching a platform from
optional to required needs to be reflected in the DT binding. So
this involves a lot of work. I think it makes sense to carry the
slightly complex check in the driver's probe routine for now.
Greetings,
-- Sebastian
>
> Thanks,
> Robin.
>
> > rockchip_get_trim_configuration(dev, np, thermal);
> >
> > ---
> > base-commit: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
> > change-id: 20250818-thermal-rockchip-grf-warning-05f7f56286a2
> >
> > Best regards,
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] thermal: rockchip: shut up GRF warning
2025-08-19 13:56 ` Sebastian Reichel
@ 2025-08-19 14:13 ` Heiko Stübner
0 siblings, 0 replies; 9+ messages in thread
From: Heiko Stübner @ 2025-08-19 14:13 UTC (permalink / raw)
To: Robin Murphy, Sebastian Reichel
Cc: Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
linux-pm, linux-arm-kernel, linux-rockchip, linux-kernel, kernel
Am Dienstag, 19. August 2025, 15:56:42 Mitteleuropäische Sommerzeit schrieb Sebastian Reichel:
> Hello Robin,
>
> On Tue, Aug 19, 2025 at 01:42:42PM +0100, Robin Murphy wrote:
> > > + if (thermal->chip->grf_mode != GRF_NONE) {
> > > + thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> > > + if (IS_ERR(thermal->grf)) {
> > > + ret = PTR_ERR(thermal->grf);
> > > + if (thermal->chip->grf_mode == GRF_OPTIONAL)
> > > + dev_warn(dev, "Missing rockchip,grf property\n");
> > > + else
> > > + return dev_err_probe(dev, ret, "Missing rockchip,grf property\n");
> > > + }
> > > + }
> >
> > Nit: Does the lookup itself need to be made conditional? I think I'd
> > also agree that the "optional" mode seems suspect, so potentially it
> > could be a whole lot simpler, e.g.:
> >
> > thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> > if (IS_ERR(thermal->grf) && thermal->chip->grf_required)
> > return dev_err_probe(dev, PTR_ERR(thermal->grf),
> > "Missing rockchip,grf property\n");
>
> I came up with the enum, because I think most platforms having
> "optional" GRF support actually require it, so I want to keep the
> warning. At the same time I don't want to mark them as GRF required
> at this point, since that is potentially a DT ABI break. It really
> needs to be checked per platform in the TRM and/or by testing on
> real HW. With this patch we can easily handle this platform by
> platform by moving them from GRF_OPTIONAL to GRF_MANDATORY without
> affecting the unchecked platforms. Also switching a platform from
> optional to required needs to be reflected in the DT binding. So
> this involves a lot of work. I think it makes sense to carry the
> slightly complex check in the driver's probe routine for now.
I did go digging now ... there are 3 variants marked as "optional":
rk3366_tsadc_data -> rockchip,rk3366-tsadc
never added any DTS
rk3399_tsadc_data -> rockchip,rk3399-tsadc
commit 95c27ba7bd92 ("arm64: dts: rockchip: add thermal nodes for rk3399 SoCs")
added the tsdadc node together with its rockchip,grf reference
rk3568_tsadc_data rockchip,rk3568-tsadc
commit 1330875dc2a3 ("arm64: dts: rockchip: add rk3568 tsadc nodes")
added the tsdadc node together with its rockchip,grf reference
So none of the platforms had ever a phase where we had the tsadc without
its grf-reference. So making the GRF mandatory for those, will not create
breakage. We could even tighten the binding to make that explicit.
Heiko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] thermal: rockchip: shut up GRF warning
2025-08-18 17:26 [PATCH] thermal: rockchip: shut up GRF warning Sebastian Reichel
` (2 preceding siblings ...)
2025-08-19 12:42 ` Robin Murphy
@ 2025-08-20 14:23 ` Diederik de Haas
3 siblings, 0 replies; 9+ messages in thread
From: Diederik de Haas @ 2025-08-20 14:23 UTC (permalink / raw)
To: Sebastian Reichel, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Heiko Stuebner
Cc: linux-pm, linux-arm-kernel, linux-rockchip, linux-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 7220 bytes --]
Hi Sebastian,
On Mon Aug 18, 2025 at 7:26 PM CEST, Sebastian Reichel wrote:
> Most of the recent Rockchip devices do not have a GRF associated
> with the tsadc IP. Let's avoid printing a warning on those devices.
Thanks for this patch :-)
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> drivers/thermal/rockchip_thermal.c | 53 +++++++++++++++++++++++++++++++++-----
> 1 file changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index 3beff9b6fac3abe8948b56132b618ff1bed57217..1e8091cebd6673ab39fa0c4dee835c68aeb7e8b5 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -50,6 +50,18 @@ enum adc_sort_mode {
> ADC_INCREMENT,
> };
>
> +/*
> + * The GRF availability depends on the specific SoC
> + * GRF_NONE: the SoC does not have a GRF associated with the tsadc
> + * GRF_OPTIONAL: the SoC has a GRF, but the driver can work without it
> + * GRF_MANDATORY: the SoC has a GRF and it is required for proper operation
> + */
> +enum tsadc_grf_mode {
> + GRF_NONE,
> + GRF_OPTIONAL,
> + GRF_MANDATORY,
> +};
> +
> #include "thermal_hwmon.h"
>
> /**
> @@ -97,6 +109,9 @@ struct rockchip_tsadc_chip {
> enum tshut_mode tshut_mode;
> enum tshut_polarity tshut_polarity;
>
> + /* GRF availability */
> + enum tsadc_grf_mode grf_mode;
> +
> /* Chip-wide methods */
> void (*initialize)(struct regmap *grf,
> void __iomem *reg, enum tshut_polarity p);
> @@ -1099,6 +1114,8 @@ static const struct rockchip_tsadc_chip px30_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* 2 channels for tsadc */
>
> + .grf_mode = GRF_MANDATORY,
> +
> .tshut_mode = TSHUT_MODE_CRU, /* default TSHUT via CRU */
> .tshut_temp = 95000,
>
> @@ -1123,6 +1140,8 @@ static const struct rockchip_tsadc_chip rv1108_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 1, /* one channel for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1148,6 +1167,8 @@ static const struct rockchip_tsadc_chip rk3228_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 1, /* one channel for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1173,6 +1194,8 @@ static const struct rockchip_tsadc_chip rk3288_tsadc_data = {
> .chn_offset = 1,
> .chn_num = 2, /* two channels for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1198,6 +1221,8 @@ static const struct rockchip_tsadc_chip rk3328_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 1, /* one channels for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
> .tshut_mode = TSHUT_MODE_CRU, /* default TSHUT via CRU */
> .tshut_temp = 95000,
>
> @@ -1222,6 +1247,8 @@ static const struct rockchip_tsadc_chip rk3366_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* two channels for tsadc */
>
> + .grf_mode = GRF_OPTIONAL,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1247,6 +1274,8 @@ static const struct rockchip_tsadc_chip rk3368_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* two channels for tsadc */
>
> + .grf_mode = GRF_NONE,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1272,6 +1301,8 @@ static const struct rockchip_tsadc_chip rk3399_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* two channels for tsadc */
>
> + .grf_mode = GRF_OPTIONAL,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1297,6 +1328,8 @@ static const struct rockchip_tsadc_chip rk3568_tsadc_data = {
> .chn_offset = 0,
> .chn_num = 2, /* two channels for tsadc */
>
> + .grf_mode = GRF_OPTIONAL,
> +
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1321,6 +1354,7 @@ static const struct rockchip_tsadc_chip rk3576_tsadc_data = {
> /* top, big_core, little_core, ddr, npu, gpu */
> .chn_offset = 0,
> .chn_num = 6, /* six channels for tsadc */
> + .grf_mode = GRF_NONE,
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1345,6 +1379,7 @@ static const struct rockchip_tsadc_chip rk3588_tsadc_data = {
> /* top, big_core0, big_core1, little_core, center, gpu, npu */
> .chn_offset = 0,
> .chn_num = 7, /* seven channels for tsadc */
> + .grf_mode = GRF_NONE,
> .tshut_mode = TSHUT_MODE_GPIO, /* default TSHUT via GPIO give PMIC */
> .tshut_polarity = TSHUT_LOW_ACTIVE, /* default TSHUT LOW ACTIVE */
> .tshut_temp = 95000,
> @@ -1572,7 +1607,7 @@ static int rockchip_configure_from_dt(struct device *dev,
> struct device_node *np,
> struct rockchip_thermal_data *thermal)
> {
> - u32 shut_temp, tshut_mode, tshut_polarity;
> + u32 shut_temp, tshut_mode, tshut_polarity, ret;
>
> if (of_property_read_u32(np, "rockchip,hw-tshut-temp", &shut_temp)) {
> dev_warn(dev,
> @@ -1621,12 +1656,16 @@ static int rockchip_configure_from_dt(struct device *dev,
> return -EINVAL;
> }
>
> - /* The tsadc wont to handle the error in here since some SoCs didn't
> - * need this property.
> - */
> - thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> - if (IS_ERR(thermal->grf))
> - dev_warn(dev, "Missing rockchip,grf property\n");
> + if (thermal->chip->grf_mode != GRF_NONE) {
> + thermal->grf = syscon_regmap_lookup_by_phandle(np, "rockchip,grf");
> + if (IS_ERR(thermal->grf)) {
> + ret = PTR_ERR(thermal->grf);
> + if (thermal->chip->grf_mode == GRF_OPTIONAL)
> + dev_warn(dev, "Missing rockchip,grf property\n");
> + else
> + return dev_err_probe(dev, ret, "Missing rockchip,grf property\n");
> + }
> + }
>
> rockchip_get_trim_configuration(dev, np, thermal);
I tested this patch on the following devices and found no regressions:
- Rock64 (rk3328)
- RockPro64 (rk3399)
- Quartz64 Model B (rk3566)
- NanoPi R5S (rk3568)
When tested on my Rock 5B (rk3588), I no longer saw this warning:
rockchip-thermal fec00000.tsadc: Missing rockchip,grf property
And found no regressions, so
Tested-by: Diederik de Haas <didi.debian@cknow.org>
Cheers,
Diederik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread