linux-kernel - Re: [PATCH] thermal: rockchip: shut up GRF warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b6cqwtj73twqxstslbhuulkgsmpds2hdyfsn7yewllkbtj7jz3@2kk74kgtefvp>
Date: Mon, 18 Aug 2025 21:23:54 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Heiko Stübner <heiko@...ech.de>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Zhang Rui <rui.zhang@...el.com>, 
	Lukasz Luba <lukasz.luba@....com>, linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH] thermal: rockchip: shut up GRF warning

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@...labora.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@...ech.de>

Thanks.

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)