diff mbox series

[v4,2/6] arm64: dts: rockchip: enable thermal management on all RK3588 boards

Message ID 20240506-rk-dts-additions-v4-2-271023ddfd40@gmail.com (mailing list archive)
State New, archived
Headers show
Series RK3588 and Rock 5B dts additions: thermal, OPP and fan | expand

Commit Message

Alexey Charkov May 6, 2024, 9:36 a.m. UTC
This enables the on-chip thermal monitoring sensor (TSADC) on all
RK3588(s) boards that don't have it enabled yet. It provides temperature
monitoring for the SoC and emergency thermal shutdowns, and is thus
important to have in place before CPU DVFS is enabled, as high CPU
operating performance points can overheat the chip quickly in the
absence of thermal management.

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-armsom-sige7.dts          | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-common.dtsi | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts              | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts              | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts           | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi           | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts              | 4 ++++
 8 files changed, 32 insertions(+)

Comments

Diederik de Haas May 6, 2024, 12:28 p.m. UTC | #1
Hi,

On Monday, 6 May 2024 11:36:33 CEST Alexey Charkov wrote:
> This enables the on-chip thermal monitoring sensor (TSADC) on all
> RK3588(s) boards that don't have it enabled yet. It provides temperature
> monitoring for the SoC and emergency thermal shutdowns, and is thus
> important to have in place before CPU DVFS is enabled, as high CPU
> operating performance points can overheat the chip quickly in the
> absence of thermal management.
> 
> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
>  8 files changed, 32 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> b8e15b76a8a6..21e96c212dd8 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -742,6 +742,10 @@ regulator-state-mem {
>  	};
>  };
> 
> +&tsadc {
> +	status = "okay";
> +};
> +
>  &uart2 {
>  	pinctrl-0 = <&uart2m0_xfer>;
>  	status = "okay";

I built a kernel with v3 of your patch set and someone tested it on a ROCK 5B 
'for me' and it had the following line in dmesg:

rockchip-thermal fec00000.tsadc: Missing rockchip,grf property

I'm guessing that turned up due to enabling tsadc, but (also) in v4 I didn't 
see a change wrt "rockchip,grf".
Should that be done? (asking; I don't know)

Cheers,
  Diederik
Dragan Simic May 6, 2024, 12:52 p.m. UTC | #2
Hey Diederik and Alexey,

On 2024-05-06 14:28, Diederik de Haas wrote:
> On Monday, 6 May 2024 11:36:33 CEST Alexey Charkov wrote:
>> This enables the on-chip thermal monitoring sensor (TSADC) on all
>> RK3588(s) boards that don't have it enabled yet. It provides 
>> temperature
>> monitoring for the SoC and emergency thermal shutdowns, and is thus
>> important to have in place before CPU DVFS is enabled, as high CPU
>> operating performance points can overheat the chip quickly in the
>> absence of thermal management.
>> 
>> Signed-off-by: Alexey Charkov <alchark@gmail.com>
>> ---
>>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 
>> ++++
>>  8 files changed, 32 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
>> b8e15b76a8a6..21e96c212dd8 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> @@ -742,6 +742,10 @@ regulator-state-mem {
>>  	};
>>  };
>> 
>> +&tsadc {
>> +	status = "okay";
>> +};
>> +
>>  &uart2 {
>>  	pinctrl-0 = <&uart2m0_xfer>;
>>  	status = "okay";
> 
> I built a kernel with v3 of your patch set and someone tested it on a 
> ROCK 5B
> 'for me' and it had the following line in dmesg:
> 
> rockchip-thermal fec00000.tsadc: Missing rockchip,grf property
> 
> I'm guessing that turned up due to enabling tsadc, but (also) in v4 I 
> didn't
> see a change wrt "rockchip,grf".
> Should that be done? (asking; I don't know)

Nice catch!  As it turns out, having "rockchip,grf" defined isn't
needed for the RK3588, so this warning is of somewhat false nature.
In more detail, having "rockchip,grf" defined is actually required
only for some Rockchip SoCs, e.g. RK356x.

I can get this covered in my soon-to-be-submitted device-tree cleanup
patch series, if Alexey is fine with that.
Alexey Charkov May 6, 2024, 12:54 p.m. UTC | #3
Hello Diederik,

On Mon, May 6, 2024 at 4:29 PM Diederik de Haas <didi.debian@cknow.org> wrote:
>
> Hi,
>
> On Monday, 6 May 2024 11:36:33 CEST Alexey Charkov wrote:
> > This enables the on-chip thermal monitoring sensor (TSADC) on all
> > RK3588(s) boards that don't have it enabled yet. It provides temperature
> > monitoring for the SoC and emergency thermal shutdowns, and is thus
> > important to have in place before CPU DVFS is enabled, as high CPU
> > operating performance points can overheat the chip quickly in the
> > absence of thermal management.
> >
> > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
> >  8 files changed, 32 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> > b8e15b76a8a6..21e96c212dd8 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -742,6 +742,10 @@ regulator-state-mem {
> >       };
> >  };
> >
> > +&tsadc {
> > +     status = "okay";
> > +};
> > +
> >  &uart2 {
> >       pinctrl-0 = <&uart2m0_xfer>;
> >       status = "okay";
>
> I built a kernel with v3 of your patch set and someone tested it on a ROCK 5B
> 'for me' and it had the following line in dmesg:
>
> rockchip-thermal fec00000.tsadc: Missing rockchip,grf property
>
> I'm guessing that turned up due to enabling tsadc, but (also) in v4 I didn't
> see a change wrt "rockchip,grf".
> Should that be done? (asking; I don't know)

I'm getting the same. Neither the mainline TSADC driver [1], nor the
downstream one [2] seems to use the grf pointer on RK3588 at all. It
still works in spite of that warning, although I can't see how (or if)
it configures the reset mechanism without those GRF registers.

Best regards,
Alexey

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/rockchip_thermal.c#n818
[2] https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/thermal/rockchip_thermal.c#L961
Anand Moon May 8, 2024, 11:40 a.m. UTC | #4
Hi Alexey,

On Mon, 6 May 2024 at 18:24, Alexey Charkov <alchark@gmail.com> wrote:
>
> Hello Diederik,
>
> On Mon, May 6, 2024 at 4:29 PM Diederik de Haas <didi.debian@cknow.org> wrote:
> >
> > Hi,
> >
> > On Monday, 6 May 2024 11:36:33 CEST Alexey Charkov wrote:
> > > This enables the on-chip thermal monitoring sensor (TSADC) on all
> > > RK3588(s) boards that don't have it enabled yet. It provides temperature
> > > monitoring for the SoC and emergency thermal shutdowns, and is thus
> > > important to have in place before CPU DVFS is enabled, as high CPU
> > > operating performance points can overheat the chip quickly in the
> > > absence of thermal management.
> > >
> > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > > ---
> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
> > >  8 files changed, 32 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> > > b8e15b76a8a6..21e96c212dd8 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > @@ -742,6 +742,10 @@ regulator-state-mem {
> > >       };
> > >  };
> > >
> > > +&tsadc {
> > > +     status = "okay";
> > > +};
> > > +
> > >  &uart2 {
> > >       pinctrl-0 = <&uart2m0_xfer>;
> > >       status = "okay";
> >
> > I built a kernel with v3 of your patch set and someone tested it on a ROCK 5B
> > 'for me' and it had the following line in dmesg:
> >
> > rockchip-thermal fec00000.tsadc: Missing rockchip,grf property
> >
> > I'm guessing that turned up due to enabling tsadc, but (also) in v4 I didn't
> > see a change wrt "rockchip,grf".
> > Should that be done? (asking; I don't know)
>
> I'm getting the same. Neither the mainline TSADC driver [1], nor the
> downstream one [2] seems to use the grf pointer on RK3588 at all. It
> still works in spite of that warning, although I can't see how (or if)
> it configures the reset mechanism without those GRF registers.
>
> Best regards,
> Alexey
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/rockchip_thermal.c#n818
> [2] https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/thermal/rockchip_thermal.c#L961
>

If the following changes fix the warning.

Checking the Rockchip RK3588 TRM V1.0-Part1-20220309.pdf
PMU1GRF_SOC_CON3 which has tsadc_shut_reset_trigger_en bit
to control the Enable TSADC shut reset trigger for DDR fail safe.

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 85c25d5efdad..5490a44e093e 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -2662,6 +2662,7 @@ tsadc: tsadc@fec00000 {
                rockchip,hw-tshut-temp = <120000>;
                rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU 1:GPIO */
                rockchip,hw-tshut-polarity = <0>; /* tshut polarity
0:LOW 1:HIGH */
+               rockchip,pmu = <&pmu1grf>;
                pinctrl-0 = <&tsadc_gpio_func>;
                pinctrl-1 = <&tsadc_shut>;
                pinctrl-names = "gpio", "otpout";

Thanks
-Anand

> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Dragan Simic May 8, 2024, 11:46 a.m. UTC | #5
Hello Anand,

On 2024-05-08 13:40, Anand Moon wrote:
> On Mon, 6 May 2024 at 18:24, Alexey Charkov <alchark@gmail.com> wrote:
>> On Mon, May 6, 2024 at 4:29 PM Diederik de Haas 
>> <didi.debian@cknow.org> wrote:
>> > On Monday, 6 May 2024 11:36:33 CEST Alexey Charkov wrote:
>> > > This enables the on-chip thermal monitoring sensor (TSADC) on all
>> > > RK3588(s) boards that don't have it enabled yet. It provides temperature
>> > > monitoring for the SoC and emergency thermal shutdowns, and is thus
>> > > important to have in place before CPU DVFS is enabled, as high CPU
>> > > operating performance points can overheat the chip quickly in the
>> > > absence of thermal management.
>> > >
>> > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
>> > > ---
>> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
>> > >  8 files changed, 32 insertions(+)
>> > >
>> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> > > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
>> > > b8e15b76a8a6..21e96c212dd8 100644
>> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> > > @@ -742,6 +742,10 @@ regulator-state-mem {
>> > >       };
>> > >  };
>> > >
>> > > +&tsadc {
>> > > +     status = "okay";
>> > > +};
>> > > +
>> > >  &uart2 {
>> > >       pinctrl-0 = <&uart2m0_xfer>;
>> > >       status = "okay";
>> >
>> > I built a kernel with v3 of your patch set and someone tested it on a ROCK 5B
>> > 'for me' and it had the following line in dmesg:
>> >
>> > rockchip-thermal fec00000.tsadc: Missing rockchip,grf property
>> >
>> > I'm guessing that turned up due to enabling tsadc, but (also) in v4 I didn't
>> > see a change wrt "rockchip,grf".
>> > Should that be done? (asking; I don't know)
>> 
>> I'm getting the same. Neither the mainline TSADC driver [1], nor the
>> downstream one [2] seems to use the grf pointer on RK3588 at all. It
>> still works in spite of that warning, although I can't see how (or if)
>> it configures the reset mechanism without those GRF registers.
>> 
>> Best regards,
>> Alexey
>> 
>> [1] 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/rockchip_thermal.c#n818
>> [2] 
>> https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/thermal/rockchip_thermal.c#L961
>> 
> 
> If the following changes fix the warning.
> 
> Checking the Rockchip RK3588 TRM V1.0-Part1-20220309.pdf
> PMU1GRF_SOC_CON3 which has tsadc_shut_reset_trigger_en bit
> to control the Enable TSADC shut reset trigger for DDR fail safe.
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> index 85c25d5efdad..5490a44e093e 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -2662,6 +2662,7 @@ tsadc: tsadc@fec00000 {
>                 rockchip,hw-tshut-temp = <120000>;
>                 rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU 
> 1:GPIO */
>                 rockchip,hw-tshut-polarity = <0>; /* tshut polarity
> 0:LOW 1:HIGH */
> +               rockchip,pmu = <&pmu1grf>;
>                 pinctrl-0 = <&tsadc_gpio_func>;
>                 pinctrl-1 = <&tsadc_shut>;
>                 pinctrl-names = "gpio", "otpout";

Basically, the rockchip_thermal driver doesn't use GRF at all on
the RK3588(s), so virtually any value specified as "rockchip,pmu"
can eliminate the warning.

I'm already working on a rather large device-tree cleanup series,
and this is already fixed in it.  Are you fine with dropping your
patch as a separate one, and I'll tag you with Co-developed-by in
the relevant patch from my series?
Alexey Charkov May 8, 2024, 12:30 p.m. UTC | #6
Hello Dragan and Anand,

On Wed, May 8, 2024 at 3:46 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Anand,
>
> On 2024-05-08 13:40, Anand Moon wrote:
> > On Mon, 6 May 2024 at 18:24, Alexey Charkov <alchark@gmail.com> wrote:
> >> On Mon, May 6, 2024 at 4:29 PM Diederik de Haas
> >> <didi.debian@cknow.org> wrote:
> >> > On Monday, 6 May 2024 11:36:33 CEST Alexey Charkov wrote:
> >> > > This enables the on-chip thermal monitoring sensor (TSADC) on all
> >> > > RK3588(s) boards that don't have it enabled yet. It provides temperature
> >> > > monitoring for the SoC and emergency thermal shutdowns, and is thus
> >> > > important to have in place before CPU DVFS is enabled, as high CPU
> >> > > operating performance points can overheat the chip quickly in the
> >> > > absence of thermal management.
> >> > >
> >> > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> >> > > ---
> >> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
> >> > >  8 files changed, 32 insertions(+)
> >> > >
> >> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >> > > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> >> > > b8e15b76a8a6..21e96c212dd8 100644
> >> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >> > > @@ -742,6 +742,10 @@ regulator-state-mem {
> >> > >       };
> >> > >  };
> >> > >
> >> > > +&tsadc {
> >> > > +     status = "okay";
> >> > > +};
> >> > > +
> >> > >  &uart2 {
> >> > >       pinctrl-0 = <&uart2m0_xfer>;
> >> > >       status = "okay";
> >> >
> >> > I built a kernel with v3 of your patch set and someone tested it on a ROCK 5B
> >> > 'for me' and it had the following line in dmesg:
> >> >
> >> > rockchip-thermal fec00000.tsadc: Missing rockchip,grf property
> >> >
> >> > I'm guessing that turned up due to enabling tsadc, but (also) in v4 I didn't
> >> > see a change wrt "rockchip,grf".
> >> > Should that be done? (asking; I don't know)
> >>
> >> I'm getting the same. Neither the mainline TSADC driver [1], nor the
> >> downstream one [2] seems to use the grf pointer on RK3588 at all. It
> >> still works in spite of that warning, although I can't see how (or if)
> >> it configures the reset mechanism without those GRF registers.
> >>
> >> Best regards,
> >> Alexey
> >>
> >> [1]
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/rockchip_thermal.c#n818
> >> [2]
> >> https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/thermal/rockchip_thermal.c#L961
> >>
> >
> > If the following changes fix the warning.
> >
> > Checking the Rockchip RK3588 TRM V1.0-Part1-20220309.pdf
> > PMU1GRF_SOC_CON3 which has tsadc_shut_reset_trigger_en bit
> > to control the Enable TSADC shut reset trigger for DDR fail safe.
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > index 85c25d5efdad..5490a44e093e 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > @@ -2662,6 +2662,7 @@ tsadc: tsadc@fec00000 {
> >                 rockchip,hw-tshut-temp = <120000>;
> >                 rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU
> > 1:GPIO */
> >                 rockchip,hw-tshut-polarity = <0>; /* tshut polarity
> > 0:LOW 1:HIGH */
> > +               rockchip,pmu = <&pmu1grf>;
> >                 pinctrl-0 = <&tsadc_gpio_func>;
> >                 pinctrl-1 = <&tsadc_shut>;
> >                 pinctrl-names = "gpio", "otpout";
>
> Basically, the rockchip_thermal driver doesn't use GRF at all on
> the RK3588(s), so virtually any value specified as "rockchip,pmu"
> can eliminate the warning.

To me, specifying an arbitrary GRF in the device tree just to silence
a warning that the current driver emits sounds bad. If the GRF is not
needed for TSADC initialization on RK3588, then it should not be in
the device tree (and it looks like the case here) - otherwise the
device tree ceases to be a truthful description of the hardware.

I'm not sure if we need that "DDR fail safe" logic mentioned in the
TRM that Anand quoted, given that neither Rockchip downstream nor
current mainline driver implement it, and furthermore none of the
other SoC revisions supported by the driver mention it. If we do in
fact need it, we should probably first test it along with respective
driver code before committing to an upstream DT and thus making it
part of the ABI.

IMO this is more of a driver issue than a device tree issue: maybe a
small patch to demote this warning to an info message would be better?
It's harmless anyway.

Best regards,
Alexey
Dragan Simic May 8, 2024, 12:38 p.m. UTC | #7
Hello Alexey,

On 2024-05-08 14:30, Alexey Charkov wrote:
> On Wed, May 8, 2024 at 3:46 PM Dragan Simic <dsimic@manjaro.org> wrote:
>> On 2024-05-08 13:40, Anand Moon wrote:
>> > On Mon, 6 May 2024 at 18:24, Alexey Charkov <alchark@gmail.com> wrote:
>> >> On Mon, May 6, 2024 at 4:29 PM Diederik de Haas
>> >> <didi.debian@cknow.org> wrote:
>> >> > On Monday, 6 May 2024 11:36:33 CEST Alexey Charkov wrote:
>> >> > > This enables the on-chip thermal monitoring sensor (TSADC) on all
>> >> > > RK3588(s) boards that don't have it enabled yet. It provides temperature
>> >> > > monitoring for the SoC and emergency thermal shutdowns, and is thus
>> >> > > important to have in place before CPU DVFS is enabled, as high CPU
>> >> > > operating performance points can overheat the chip quickly in the
>> >> > > absence of thermal management.
>> >> > >
>> >> > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
>> >> > > ---
>> >> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
>> >> > >  8 files changed, 32 insertions(+)
>> >> > >
>> >> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> >> > > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
>> >> > > b8e15b76a8a6..21e96c212dd8 100644
>> >> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> >> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> >> > > @@ -742,6 +742,10 @@ regulator-state-mem {
>> >> > >       };
>> >> > >  };
>> >> > >
>> >> > > +&tsadc {
>> >> > > +     status = "okay";
>> >> > > +};
>> >> > > +
>> >> > >  &uart2 {
>> >> > >       pinctrl-0 = <&uart2m0_xfer>;
>> >> > >       status = "okay";
>> >> >
>> >> > I built a kernel with v3 of your patch set and someone tested it on a ROCK 5B
>> >> > 'for me' and it had the following line in dmesg:
>> >> >
>> >> > rockchip-thermal fec00000.tsadc: Missing rockchip,grf property
>> >> >
>> >> > I'm guessing that turned up due to enabling tsadc, but (also) in v4 I didn't
>> >> > see a change wrt "rockchip,grf".
>> >> > Should that be done? (asking; I don't know)
>> >>
>> >> I'm getting the same. Neither the mainline TSADC driver [1], nor the
>> >> downstream one [2] seems to use the grf pointer on RK3588 at all. It
>> >> still works in spite of that warning, although I can't see how (or if)
>> >> it configures the reset mechanism without those GRF registers.
>> >>
>> >> Best regards,
>> >> Alexey
>> >>
>> >> [1]
>> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/rockchip_thermal.c#n818
>> >> [2]
>> >> https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/thermal/rockchip_thermal.c#L961
>> >>
>> >
>> > If the following changes fix the warning.
>> >
>> > Checking the Rockchip RK3588 TRM V1.0-Part1-20220309.pdf
>> > PMU1GRF_SOC_CON3 which has tsadc_shut_reset_trigger_en bit
>> > to control the Enable TSADC shut reset trigger for DDR fail safe.
>> >
>> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> > index 85c25d5efdad..5490a44e093e 100644
>> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> > @@ -2662,6 +2662,7 @@ tsadc: tsadc@fec00000 {
>> >                 rockchip,hw-tshut-temp = <120000>;
>> >                 rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU
>> > 1:GPIO */
>> >                 rockchip,hw-tshut-polarity = <0>; /* tshut polarity
>> > 0:LOW 1:HIGH */
>> > +               rockchip,pmu = <&pmu1grf>;
>> >                 pinctrl-0 = <&tsadc_gpio_func>;
>> >                 pinctrl-1 = <&tsadc_shut>;
>> >                 pinctrl-names = "gpio", "otpout";
>> 
>> Basically, the rockchip_thermal driver doesn't use GRF at all on
>> the RK3588(s), so virtually any value specified as "rockchip,pmu"
>> can eliminate the warning.
> 
> To me, specifying an arbitrary GRF in the device tree just to silence
> a warning that the current driver emits sounds bad. If the GRF is not
> needed for TSADC initialization on RK3588, then it should not be in
> the device tree (and it looks like the case here) - otherwise the
> device tree ceases to be a truthful description of the hardware.

After thinking a bit more about it, I think you're right.  If the
rockchip_thermal driver ever gets changed to require use of GRF on
the RK3588(s), only then adding that property to the DT would be
the right thing to do.

> I'm not sure if we need that "DDR fail safe" logic mentioned in the
> TRM that Anand quoted, given that neither Rockchip downstream nor
> current mainline driver implement it, and furthermore none of the
> other SoC revisions supported by the driver mention it. If we do in
> fact need it, we should probably first test it along with respective
> driver code before committing to an upstream DT and thus making it
> part of the ABI.
> 
> IMO this is more of a driver issue than a device tree issue: maybe a
> small patch to demote this warning to an info message would be better?
> It's harmless anyway.

After having second thoughts, I'll see to improve the rockchip_thermal
driver to emit that warning only when having "rockchip,grf" specified
is actually needed for the particular Rockchip SoC.  That's how it 
should
behave, yelling about the wrong hardware description only when that's
actually the case.
Anand Moon May 8, 2024, 12:51 p.m. UTC | #8
Hi Dragan, Alexey,

On Wed, 8 May 2024 at 18:08, Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Alexey,
>
> On 2024-05-08 14:30, Alexey Charkov wrote:
> > On Wed, May 8, 2024 at 3:46 PM Dragan Simic <dsimic@manjaro.org> wrote:
> >> On 2024-05-08 13:40, Anand Moon wrote:
> >> > On Mon, 6 May 2024 at 18:24, Alexey Charkov <alchark@gmail.com> wrote:
> >> >> On Mon, May 6, 2024 at 4:29 PM Diederik de Haas
> >> >> <didi.debian@cknow.org> wrote:
> >> >> > On Monday, 6 May 2024 11:36:33 CEST Alexey Charkov wrote:
> >> >> > > This enables the on-chip thermal monitoring sensor (TSADC) on all
> >> >> > > RK3588(s) boards that don't have it enabled yet. It provides temperature
> >> >> > > monitoring for the SoC and emergency thermal shutdowns, and is thus
> >> >> > > important to have in place before CPU DVFS is enabled, as high CPU
> >> >> > > operating performance points can overheat the chip quickly in the
> >> >> > > absence of thermal management.
> >> >> > >
> >> >> > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> >> >> > > ---
> >> >> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
> >> >> > >  8 files changed, 32 insertions(+)
> >> >> > >
> >> >> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >> >> > > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> >> >> > > b8e15b76a8a6..21e96c212dd8 100644
> >> >> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >> >> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >> >> > > @@ -742,6 +742,10 @@ regulator-state-mem {
> >> >> > >       };
> >> >> > >  };
> >> >> > >
> >> >> > > +&tsadc {
> >> >> > > +     status = "okay";
> >> >> > > +};
> >> >> > > +
> >> >> > >  &uart2 {
> >> >> > >       pinctrl-0 = <&uart2m0_xfer>;
> >> >> > >       status = "okay";
> >> >> >
> >> >> > I built a kernel with v3 of your patch set and someone tested it on a ROCK 5B
> >> >> > 'for me' and it had the following line in dmesg:
> >> >> >
> >> >> > rockchip-thermal fec00000.tsadc: Missing rockchip,grf property
> >> >> >
> >> >> > I'm guessing that turned up due to enabling tsadc, but (also) in v4 I didn't
> >> >> > see a change wrt "rockchip,grf".
> >> >> > Should that be done? (asking; I don't know)
> >> >>
> >> >> I'm getting the same. Neither the mainline TSADC driver [1], nor the
> >> >> downstream one [2] seems to use the grf pointer on RK3588 at all. It
> >> >> still works in spite of that warning, although I can't see how (or if)
> >> >> it configures the reset mechanism without those GRF registers.
> >> >>
> >> >> Best regards,
> >> >> Alexey
> >> >>
> >> >> [1]
> >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/rockchip_thermal.c#n818
> >> >> [2]
> >> >> https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/thermal/rockchip_thermal.c#L961
> >> >>
> >> >
> >> > If the following changes fix the warning.
> >> >
> >> > Checking the Rockchip RK3588 TRM V1.0-Part1-20220309.pdf
> >> > PMU1GRF_SOC_CON3 which has tsadc_shut_reset_trigger_en bit
> >> > to control the Enable TSADC shut reset trigger for DDR fail safe.
> >> >
> >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >> > index 85c25d5efdad..5490a44e093e 100644
> >> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >> > @@ -2662,6 +2662,7 @@ tsadc: tsadc@fec00000 {
> >> >                 rockchip,hw-tshut-temp = <120000>;
> >> >                 rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU
> >> > 1:GPIO */
> >> >                 rockchip,hw-tshut-polarity = <0>; /* tshut polarity
> >> > 0:LOW 1:HIGH */
> >> > +               rockchip,pmu = <&pmu1grf>;
> >> >                 pinctrl-0 = <&tsadc_gpio_func>;
> >> >                 pinctrl-1 = <&tsadc_shut>;
> >> >                 pinctrl-names = "gpio", "otpout";
> >>
> >> Basically, the rockchip_thermal driver doesn't use GRF at all on
> >> the RK3588(s), so virtually any value specified as "rockchip,pmu"
> >> can eliminate the warning.
> >
> > To me, specifying an arbitrary GRF in the device tree just to silence
> > a warning that the current driver emits sounds bad. If the GRF is not
> > needed for TSADC initialization on RK3588, then it should not be in
> > the device tree (and it looks like the case here) - otherwise the
> > device tree ceases to be a truthful description of the hardware.
>
> After thinking a bit more about it, I think you're right.  If the
> rockchip_thermal driver ever gets changed to require use of GRF on
> the RK3588(s), only then adding that property to the DT would be
> the right thing to do.
>
> > I'm not sure if we need that "DDR fail safe" logic mentioned in the
> > TRM that Anand quoted, given that neither Rockchip downstream nor
> > current mainline driver implement it, and furthermore none of the
> > other SoC revisions supported by the driver mention it. If we do in
> > fact need it, we should probably first test it along with respective
> > driver code before committing to an upstream DT and thus making it
> > part of the ABI.
> >
> > IMO this is more of a driver issue than a device tree issue: maybe a
> > small patch to demote this warning to an info message would be better?
> > It's harmless anyway.
>
> After having second thoughts, I'll see to improve the rockchip_thermal
> driver to emit that warning only when having "rockchip,grf" specified
> is actually needed for the particular Rockchip SoC.  That's how it
> should
> behave, yelling about the wrong hardware description only when that's
> actually the case.

We need to handle the GRF TSADC for rk3588 tmu driver.

For rk3568 we control the GRF TSADC which seems to be missing in rk3588

[[0] https://elixir.bootlin.com/linux/v6.8.9/source/drivers/thermal/rockchip_thermal.c#L798

Thanks
-Anand
Alexey Charkov May 8, 2024, 1:21 p.m. UTC | #9
On Wed, May 8, 2024 at 4:51 PM Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Dragan, Alexey,
>
> On Wed, 8 May 2024 at 18:08, Dragan Simic <dsimic@manjaro.org> wrote:
> >
> > Hello Alexey,
> >
> > On 2024-05-08 14:30, Alexey Charkov wrote:
> > > On Wed, May 8, 2024 at 3:46 PM Dragan Simic <dsimic@manjaro.org> wrote:
> > >> On 2024-05-08 13:40, Anand Moon wrote:
> > >> > On Mon, 6 May 2024 at 18:24, Alexey Charkov <alchark@gmail.com> wrote:
> > >> >> On Mon, May 6, 2024 at 4:29 PM Diederik de Haas
> > >> >> <didi.debian@cknow.org> wrote:
> > >> >> > On Monday, 6 May 2024 11:36:33 CEST Alexey Charkov wrote:
> > >> >> > > This enables the on-chip thermal monitoring sensor (TSADC) on all
> > >> >> > > RK3588(s) boards that don't have it enabled yet. It provides temperature
> > >> >> > > monitoring for the SoC and emergency thermal shutdowns, and is thus
> > >> >> > > important to have in place before CPU DVFS is enabled, as high CPU
> > >> >> > > operating performance points can overheat the chip quickly in the
> > >> >> > > absence of thermal management.
> > >> >> > >
> > >> >> > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > >> >> > > ---
> > >> >> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
> > >> >> > >  8 files changed, 32 insertions(+)
> > >> >> > >
> > >> >> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > >> >> > > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> > >> >> > > b8e15b76a8a6..21e96c212dd8 100644
> > >> >> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > >> >> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > >> >> > > @@ -742,6 +742,10 @@ regulator-state-mem {
> > >> >> > >       };
> > >> >> > >  };
> > >> >> > >
> > >> >> > > +&tsadc {
> > >> >> > > +     status = "okay";
> > >> >> > > +};
> > >> >> > > +
> > >> >> > >  &uart2 {
> > >> >> > >       pinctrl-0 = <&uart2m0_xfer>;
> > >> >> > >       status = "okay";
> > >> >> >
> > >> >> > I built a kernel with v3 of your patch set and someone tested it on a ROCK 5B
> > >> >> > 'for me' and it had the following line in dmesg:
> > >> >> >
> > >> >> > rockchip-thermal fec00000.tsadc: Missing rockchip,grf property
> > >> >> >
> > >> >> > I'm guessing that turned up due to enabling tsadc, but (also) in v4 I didn't
> > >> >> > see a change wrt "rockchip,grf".
> > >> >> > Should that be done? (asking; I don't know)
> > >> >>
> > >> >> I'm getting the same. Neither the mainline TSADC driver [1], nor the
> > >> >> downstream one [2] seems to use the grf pointer on RK3588 at all. It
> > >> >> still works in spite of that warning, although I can't see how (or if)
> > >> >> it configures the reset mechanism without those GRF registers.
> > >> >>
> > >> >> Best regards,
> > >> >> Alexey
> > >> >>
> > >> >> [1]
> > >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/rockchip_thermal.c#n818
> > >> >> [2]
> > >> >> https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/thermal/rockchip_thermal.c#L961
> > >> >>
> > >> >
> > >> > If the following changes fix the warning.
> > >> >
> > >> > Checking the Rockchip RK3588 TRM V1.0-Part1-20220309.pdf
> > >> > PMU1GRF_SOC_CON3 which has tsadc_shut_reset_trigger_en bit
> > >> > to control the Enable TSADC shut reset trigger for DDR fail safe.
> > >> >
> > >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > >> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > >> > index 85c25d5efdad..5490a44e093e 100644
> > >> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > >> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > >> > @@ -2662,6 +2662,7 @@ tsadc: tsadc@fec00000 {
> > >> >                 rockchip,hw-tshut-temp = <120000>;
> > >> >                 rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU
> > >> > 1:GPIO */
> > >> >                 rockchip,hw-tshut-polarity = <0>; /* tshut polarity
> > >> > 0:LOW 1:HIGH */
> > >> > +               rockchip,pmu = <&pmu1grf>;
> > >> >                 pinctrl-0 = <&tsadc_gpio_func>;
> > >> >                 pinctrl-1 = <&tsadc_shut>;
> > >> >                 pinctrl-names = "gpio", "otpout";
> > >>
> > >> Basically, the rockchip_thermal driver doesn't use GRF at all on
> > >> the RK3588(s), so virtually any value specified as "rockchip,pmu"
> > >> can eliminate the warning.
> > >
> > > To me, specifying an arbitrary GRF in the device tree just to silence
> > > a warning that the current driver emits sounds bad. If the GRF is not
> > > needed for TSADC initialization on RK3588, then it should not be in
> > > the device tree (and it looks like the case here) - otherwise the
> > > device tree ceases to be a truthful description of the hardware.
> >
> > After thinking a bit more about it, I think you're right.  If the
> > rockchip_thermal driver ever gets changed to require use of GRF on
> > the RK3588(s), only then adding that property to the DT would be
> > the right thing to do.
> >
> > > I'm not sure if we need that "DDR fail safe" logic mentioned in the
> > > TRM that Anand quoted, given that neither Rockchip downstream nor
> > > current mainline driver implement it, and furthermore none of the
> > > other SoC revisions supported by the driver mention it. If we do in
> > > fact need it, we should probably first test it along with respective
> > > driver code before committing to an upstream DT and thus making it
> > > part of the ABI.
> > >
> > > IMO this is more of a driver issue than a device tree issue: maybe a
> > > small patch to demote this warning to an info message would be better?
> > > It's harmless anyway.
> >
> > After having second thoughts, I'll see to improve the rockchip_thermal
> > driver to emit that warning only when having "rockchip,grf" specified
> > is actually needed for the particular Rockchip SoC.  That's how it
> > should
> > behave, yelling about the wrong hardware description only when that's
> > actually the case.
>
> We need to handle the GRF TSADC for rk3588 tmu driver.
>
> For rk3568 we control the GRF TSADC which seems to be missing in rk3588

Please note that for RK3568 the GRF registers are used to actually
enable the thermal sensing (as can be seen by the link you posted),
which doesn't seem to be necessary on RK3588 as it works just fine
without it. Furthermore, RK3568 has a nearly identical GRF register in
PMU_GRF_SOC_CON3 (see RK3568 TRM part1 page 206) to the one you
proposed referencing in the DT for RK3588, but that register is not
used in the TSADC driver either. It only uses the TSADC_CON register
within SYS_GRF (offset 0x600) on RK3568.

The TRM for RK3588 doesn't document any TSADC_CON register, even
though there is a #define in the current upstream driver that mentions
an offset of 0x100 to that register within the GRF. I'm not sure it
means 0x100 within PMU1_GRF as you mentioned - do you have any
pointers to support that?

Best regards,
Alexey
Anand Moon May 9, 2024, 5:35 a.m. UTC | #10
Hi Alexey,

On Wed, 8 May 2024 at 18:52, Alexey Charkov <alchark@gmail.com> wrote:
>
> On Wed, May 8, 2024 at 4:51 PM Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > Hi Dragan, Alexey,
> >
> > On Wed, 8 May 2024 at 18:08, Dragan Simic <dsimic@manjaro.org> wrote:
> > >
> > > Hello Alexey,
> > >
> > > On 2024-05-08 14:30, Alexey Charkov wrote:
> > > > On Wed, May 8, 2024 at 3:46 PM Dragan Simic <dsimic@manjaro.org> wrote:
> > > >> On 2024-05-08 13:40, Anand Moon wrote:
> > > >> > On Mon, 6 May 2024 at 18:24, Alexey Charkov <alchark@gmail.com> wrote:
> > > >> >> On Mon, May 6, 2024 at 4:29 PM Diederik de Haas
> > > >> >> <didi.debian@cknow.org> wrote:
> > > >> >> > On Monday, 6 May 2024 11:36:33 CEST Alexey Charkov wrote:
> > > >> >> > > This enables the on-chip thermal monitoring sensor (TSADC) on all
> > > >> >> > > RK3588(s) boards that don't have it enabled yet. It provides temperature
> > > >> >> > > monitoring for the SoC and emergency thermal shutdowns, and is thus
> > > >> >> > > important to have in place before CPU DVFS is enabled, as high CPU
> > > >> >> > > operating performance points can overheat the chip quickly in the
> > > >> >> > > absence of thermal management.
> > > >> >> > >
> > > >> >> > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > > >> >> > > ---
> > > >> >> > >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts               | 4 ++++
> > > >> >> > >  8 files changed, 32 insertions(+)
> > > >> >> > >
> > > >> >> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > >> >> > > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> > > >> >> > > b8e15b76a8a6..21e96c212dd8 100644
> > > >> >> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > >> >> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > >> >> > > @@ -742,6 +742,10 @@ regulator-state-mem {
> > > >> >> > >       };
> > > >> >> > >  };
> > > >> >> > >
> > > >> >> > > +&tsadc {
> > > >> >> > > +     status = "okay";
> > > >> >> > > +};
> > > >> >> > > +
> > > >> >> > >  &uart2 {
> > > >> >> > >       pinctrl-0 = <&uart2m0_xfer>;
> > > >> >> > >       status = "okay";
> > > >> >> >
> > > >> >> > I built a kernel with v3 of your patch set and someone tested it on a ROCK 5B
> > > >> >> > 'for me' and it had the following line in dmesg:
> > > >> >> >
> > > >> >> > rockchip-thermal fec00000.tsadc: Missing rockchip,grf property
> > > >> >> >
> > > >> >> > I'm guessing that turned up due to enabling tsadc, but (also) in v4 I didn't
> > > >> >> > see a change wrt "rockchip,grf".
> > > >> >> > Should that be done? (asking; I don't know)
> > > >> >>
> > > >> >> I'm getting the same. Neither the mainline TSADC driver [1], nor the
> > > >> >> downstream one [2] seems to use the grf pointer on RK3588 at all. It
> > > >> >> still works in spite of that warning, although I can't see how (or if)
> > > >> >> it configures the reset mechanism without those GRF registers.
> > > >> >>
> > > >> >> Best regards,
> > > >> >> Alexey
> > > >> >>
> > > >> >> [1]
> > > >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/thermal/rockchip_thermal.c#n818
> > > >> >> [2]
> > > >> >> https://github.com/radxa/kernel/blob/stable-5.10-rock5/drivers/thermal/rockchip_thermal.c#L961
> > > >> >>
> > > >> >
> > > >> > If the following changes fix the warning.
> > > >> >
> > > >> > Checking the Rockchip RK3588 TRM V1.0-Part1-20220309.pdf
> > > >> > PMU1GRF_SOC_CON3 which has tsadc_shut_reset_trigger_en bit
> > > >> > to control the Enable TSADC shut reset trigger for DDR fail safe.
> > > >> >
> > > >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > >> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > >> > index 85c25d5efdad..5490a44e093e 100644
> > > >> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > >> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > >> > @@ -2662,6 +2662,7 @@ tsadc: tsadc@fec00000 {
> > > >> >                 rockchip,hw-tshut-temp = <120000>;
> > > >> >                 rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU
> > > >> > 1:GPIO */
> > > >> >                 rockchip,hw-tshut-polarity = <0>; /* tshut polarity
> > > >> > 0:LOW 1:HIGH */
> > > >> > +               rockchip,pmu = <&pmu1grf>;
> > > >> >                 pinctrl-0 = <&tsadc_gpio_func>;
> > > >> >                 pinctrl-1 = <&tsadc_shut>;
> > > >> >                 pinctrl-names = "gpio", "otpout";
> > > >>
> > > >> Basically, the rockchip_thermal driver doesn't use GRF at all on
> > > >> the RK3588(s), so virtually any value specified as "rockchip,pmu"
> > > >> can eliminate the warning.
> > > >
> > > > To me, specifying an arbitrary GRF in the device tree just to silence
> > > > a warning that the current driver emits sounds bad. If the GRF is not
> > > > needed for TSADC initialization on RK3588, then it should not be in
> > > > the device tree (and it looks like the case here) - otherwise the
> > > > device tree ceases to be a truthful description of the hardware.
> > >
> > > After thinking a bit more about it, I think you're right.  If the
> > > rockchip_thermal driver ever gets changed to require use of GRF on
> > > the RK3588(s), only then adding that property to the DT would be
> > > the right thing to do.
> > >
> > > > I'm not sure if we need that "DDR fail safe" logic mentioned in the
> > > > TRM that Anand quoted, given that neither Rockchip downstream nor
> > > > current mainline driver implement it, and furthermore none of the
> > > > other SoC revisions supported by the driver mention it. If we do in
> > > > fact need it, we should probably first test it along with respective
> > > > driver code before committing to an upstream DT and thus making it
> > > > part of the ABI.
> > > >
> > > > IMO this is more of a driver issue than a device tree issue: maybe a
> > > > small patch to demote this warning to an info message would be better?
> > > > It's harmless anyway.
> > >
> > > After having second thoughts, I'll see to improve the rockchip_thermal
> > > driver to emit that warning only when having "rockchip,grf" specified
> > > is actually needed for the particular Rockchip SoC.  That's how it
> > > should
> > > behave, yelling about the wrong hardware description only when that's
> > > actually the case.
> >
> > We need to handle the GRF TSADC for rk3588 tmu driver.
> >
> > For rk3568 we control the GRF TSADC which seems to be missing in rk3588
>
> Please note that for RK3568 the GRF registers are used to actually
> enable the thermal sensing (as can be seen by the link you posted),
> which doesn't seem to be necessary on RK3588 as it works just fine
> without it. Furthermore, RK3568 has a nearly identical GRF register in
> PMU_GRF_SOC_CON3 (see RK3568 TRM part1 page 206) to the one you
> proposed referencing in the DT for RK3588, but that register is not
> used in the TSADC driver either. It only uses the TSADC_CON register
> within SYS_GRF (offset 0x600) on RK3568.
>

I feel there is no PMUGRF configuration for TS-ADC on RK3588 SoC as per the TRM.
So we can ignore the warning.

> The TRM for RK3588 doesn't document any TSADC_CON register, even
> though there is a #define in the current upstream driver that mentions
> an offset of 0x100 to that register within the GRF. I'm not sure it
> means 0x100 within PMU1_GRF as you mentioned - do you have any
> pointers to support that?
>

Yes, you can drop them as there is no RK3588_GRF0_TSADC_CON register
to support such settings.

Thanks

-Anand
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-armsom-sige7.dts b/arch/arm64/boot/dts/rockchip/rk3588-armsom-sige7.dts
index 98c622b27647..c667704ba985 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-armsom-sige7.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-armsom-sige7.dts
@@ -673,6 +673,10 @@  regulator-state-mem {
 	};
 };
 
+&tsadc {
+	status = "okay";
+};
+
 &u2phy0 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-common.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-common.dtsi
index 709d348cf06b..03fd193be253 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-common.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-edgeble-neu6a-common.dtsi
@@ -466,3 +466,7 @@  regulator-state-mem {
 		};
 	};
 };
+
+&tsadc {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
index 7be2190244ba..7c3696a3ad3a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts
@@ -1131,6 +1131,10 @@  &sata0 {
 	status = "okay";
 };
 
+&tsadc {
+	status = "okay";
+};
+
 &u2phy0 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts b/arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts
index 009566d881f3..230e630820b4 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts
@@ -376,6 +376,10 @@  &sdmmc {
 	status = "okay";
 };
 
+&tsadc {
+	status = "okay";
+};
+
 &u2phy2 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index b8e15b76a8a6..21e96c212dd8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -742,6 +742,10 @@  regulator-state-mem {
 	};
 };
 
+&tsadc {
+	status = "okay";
+};
+
 &uart2 {
 	pinctrl-0 = <&uart2m0_xfer>;
 	status = "okay";
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts b/arch/arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts
index 9090c5c99f2a..d0021524e7f9 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts
@@ -648,6 +648,10 @@  regulator-state-mem {
 	};
 };
 
+&tsadc {
+	status = "okay";
+};
+
 &u2phy2 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
index 6b9206ce4a03..77bcf0f6b028 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
@@ -601,6 +601,10 @@  regulator-state-mem {
 	};
 };
 
+&tsadc {
+	status = "okay";
+};
+
 &uart2 {
 	pinctrl-0 = <&uart2m0_xfer>;
 	status = "okay";
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
index 8e2a07612d17..c671a61d3aef 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
@@ -697,6 +697,10 @@  regulator-state-mem {
 	};
 };
 
+&tsadc {
+	status = "okay";
+};
+
 &u2phy0 {
 	status = "okay";
 };