[PATCH] thermal: rockchip: shut up GRF warning

[PATCH] thermal: rockchip: shut up GRF warning

[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>
---
 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);
 

---
base-commit: c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
change-id: 20250818-thermal-rockchip-grf-warning-05f7f56286a2

Best regards,
-- 
Sebastian Reichel <sre@kernel.org>

Re: [PATCH] thermal: rockchip: shut up GRF warning

[Heiko Stübner]

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

Re: [PATCH] thermal: rockchip: shut up GRF warning

[Sebastian Reichel]

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

Re: [PATCH] thermal: rockchip: shut up GRF warning

[Heiko Stübner]

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

Re: [PATCH] thermal: rockchip: shut up GRF warning

[kernel test robot]

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

Re: [PATCH] thermal: rockchip: shut up GRF warning

[Robin Murphy]

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,

Re: [PATCH] thermal: rockchip: shut up GRF warning

[Sebastian Reichel]

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

Re: [PATCH] thermal: rockchip: shut up GRF warning

[Heiko Stübner]

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

Re: [PATCH] thermal: rockchip: shut up GRF warning

[Diederik de Haas]

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