Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Quentin Schulz]

Hi all,

On 6/17/24 8:28 PM, Alexey Charkov wrote:
> RK3588j is the 'industrial' variant of RK3588, and it uses a different
> set of OPPs both in terms of allowed frequencies and in terms of
> applicable voltages at each frequency setpoint.
> 
> Add the OPPs that apply to RK3588j (and apparently RK3588m too) to
> enable dynamic CPU frequency scaling.
> 
> OPP values are derived from Rockchip downstream sources [1] by taking
> only those OPPs which have the highest frequency for a given voltage
> level and dropping the rest (if they are included, the kernel complains
> at boot time about them being inefficient)
> 
> [1] https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> 

Funny stuff actually. Rockchip have their own downstream cpufreq driver 
which autodetects at runtime the SoC variant and filter out OPPs based 
on that info. And this patch is based on those values and filters.

However, they also decided that maybe this isn't the best way to do it 
and they decided to have an rk3588j.dtsi where they remove some of those 
OPPs instead of just updating the mask/filter in their base dtsi 
(rk3588s.dtsi downstream). See 
https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588j.dtsi

So...

> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
>   arch/arm64/boot/dts/rockchip/rk3588j.dtsi | 108 ++++++++++++++++++++++++++++++
>   1 file changed, 108 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> index 0bbeee399a63..b7e69553857b 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> @@ -5,3 +5,111 @@
>    */
>   
>   #include "rk3588-extra.dtsi"
> +
> +/ {
> +	cluster0_opp_table: opp-table-cluster0 {
> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		opp-1416000000 {
> +			opp-hz = /bits/ 64 <1416000000>;
> +			opp-microvolt = <750000 750000 950000>;
> +			clock-latency-ns = <40000>;
> +			opp-suspend;
> +		};
> +		opp-1608000000 {
> +			opp-hz = /bits/ 64 <1608000000>;
> +			opp-microvolt = <887500 887500 950000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-1704000000 {
> +			opp-hz = /bits/ 64 <1704000000>;
> +			opp-microvolt = <937500 937500 950000>;
> +			clock-latency-ns = <40000>;
> +		};

None of those are valid according to Rockchip, we should have

		opp-1296000000 {
			opp-hz = /bits/ 64 <1296000000>;
			opp-microvolt = <750000 750000 950000>;
			clock-latency-ns = <40000>;
			opp-suspend;
		};

instead?

and...

> +	};
> +
> +	cluster1_opp_table: opp-table-cluster1 {
> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		opp-1416000000 {
> +			opp-hz = /bits/ 64 <1416000000>;
> +			opp-microvolt = <750000 750000 950000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-1608000000 {
> +			opp-hz = /bits/ 64 <1608000000>;
> +			opp-microvolt = <787500 787500 950000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-1800000000 {
> +			opp-hz = /bits/ 64 <1800000000>;
> +			opp-microvolt = <875000 875000 950000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-2016000000 {
> +			opp-hz = /bits/ 64 <2016000000>;
> +			opp-microvolt = <950000 950000 950000>;
> +			clock-latency-ns = <40000>;
> +		};

opp-1800000000 and opp-2016000000 should be removed.

and...

> +	};
> +
> +	cluster2_opp_table: opp-table-cluster2 {
> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		opp-1416000000 {
> +			opp-hz = /bits/ 64 <1416000000>;
> +			opp-microvolt = <750000 750000 950000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-1608000000 {
> +			opp-hz = /bits/ 64 <1608000000>;
> +			opp-microvolt = <787500 787500 950000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-1800000000 {
> +			opp-hz = /bits/ 64 <1800000000>;
> +			opp-microvolt = <875000 875000 950000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-2016000000 {
> +			opp-hz = /bits/ 64 <2016000000>;
> +			opp-microvolt = <950000 950000 950000>;
> +			clock-latency-ns = <40000>;
> +		};

opp-1800000000 and opp-2016000000 should be removed as well.

Did I misunderstand what Rockchip did here? Adding Kever in Cc at least 
for awareness on Rockchip side :)

I guess another option could be to mark those above as

turbo-mode;

though no clue what this would entail. From a glance at cpufreq, it 
seems that for Rockchip since we use the default cpufreq-dt, it would 
mark those as unusable, so essentially a no-op, so better remove them 
entirely?

Cheers,
Quentin

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Alexey Charkov]

Hi Quentin,

On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>
> Hi all,
>
> On 6/17/24 8:28 PM, Alexey Charkov wrote:
> > RK3588j is the 'industrial' variant of RK3588, and it uses a different
> > set of OPPs both in terms of allowed frequencies and in terms of
> > applicable voltages at each frequency setpoint.
> >
> > Add the OPPs that apply to RK3588j (and apparently RK3588m too) to
> > enable dynamic CPU frequency scaling.
> >
> > OPP values are derived from Rockchip downstream sources [1] by taking
> > only those OPPs which have the highest frequency for a given voltage
> > level and dropping the rest (if they are included, the kernel complains
> > at boot time about them being inefficient)
> >
> > [1] https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> >
>
> Funny stuff actually. Rockchip have their own downstream cpufreq driver
> which autodetects at runtime the SoC variant and filter out OPPs based
> on that info. And this patch is based on those values and filters.
>
> However, they also decided that maybe this isn't the best way to do it
> and they decided to have an rk3588j.dtsi where they remove some of those
> OPPs instead of just updating the mask/filter in their base dtsi
> (rk3588s.dtsi downstream). See
> https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588j.dtsi

Funny stuff indeed! Judging by the comments in the file you
referenced, those higher OPP values constitute an 'overdrive' mode,
and apparently the chip shouldn't stay in those for prolonged periods
of time. However, I couldn't locate any dts file inside Rockchip
sources that would include this rk3588j.dtsi - so not sure if we
should follow what it says too zealously.

> So...
>
> > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > ---
> >   arch/arm64/boot/dts/rockchip/rk3588j.dtsi | 108 ++++++++++++++++++++++++++++++
> >   1 file changed, 108 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> > index 0bbeee399a63..b7e69553857b 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> > @@ -5,3 +5,111 @@
> >    */
> >
> >   #include "rk3588-extra.dtsi"
> > +
> > +/ {
> > +     cluster0_opp_table: opp-table-cluster0 {
> > +             compatible = "operating-points-v2";
> > +             opp-shared;
> > +
> > +             opp-1416000000 {
> > +                     opp-hz = /bits/ 64 <1416000000>;
> > +                     opp-microvolt = <750000 750000 950000>;
> > +                     clock-latency-ns = <40000>;
> > +                     opp-suspend;
> > +             };
> > +             opp-1608000000 {
> > +                     opp-hz = /bits/ 64 <1608000000>;
> > +                     opp-microvolt = <887500 887500 950000>;
> > +                     clock-latency-ns = <40000>;
> > +             };
> > +             opp-1704000000 {
> > +                     opp-hz = /bits/ 64 <1704000000>;
> > +                     opp-microvolt = <937500 937500 950000>;
> > +                     clock-latency-ns = <40000>;
> > +             };
>
> None of those are valid according to Rockchip, we should have

Well, valid but more taxing on the hardware apparently.

>                 opp-1296000000 {
>                         opp-hz = /bits/ 64 <1296000000>;
>                         opp-microvolt = <750000 750000 950000>;
>                         clock-latency-ns = <40000>;
>                         opp-suspend;
>                 };
>
> instead?

I dropped this one because it uses a lower frequency but same voltage
as the 1.416 GHz one, thus being 'inefficient' as the thermal
subsystem says in the logs. It can be added back if we decide to
remove opp-1416000000.

> and...
>
> > +     };
> > +
> > +     cluster1_opp_table: opp-table-cluster1 {
> > +             compatible = "operating-points-v2";
> > +             opp-shared;
> > +
> > +             opp-1416000000 {
> > +                     opp-hz = /bits/ 64 <1416000000>;
> > +                     opp-microvolt = <750000 750000 950000>;
> > +                     clock-latency-ns = <40000>;
> > +             };
> > +             opp-1608000000 {
> > +                     opp-hz = /bits/ 64 <1608000000>;
> > +                     opp-microvolt = <787500 787500 950000>;
> > +                     clock-latency-ns = <40000>;
> > +             };
> > +             opp-1800000000 {
> > +                     opp-hz = /bits/ 64 <1800000000>;
> > +                     opp-microvolt = <875000 875000 950000>;
> > +                     clock-latency-ns = <40000>;
> > +             };
> > +             opp-2016000000 {
> > +                     opp-hz = /bits/ 64 <2016000000>;
> > +                     opp-microvolt = <950000 950000 950000>;
> > +                     clock-latency-ns = <40000>;
> > +             };
>
> opp-1800000000 and opp-2016000000 should be removed.
>
> and...
>
> > +     };
> > +
> > +     cluster2_opp_table: opp-table-cluster2 {
> > +             compatible = "operating-points-v2";
> > +             opp-shared;
> > +
> > +             opp-1416000000 {
> > +                     opp-hz = /bits/ 64 <1416000000>;
> > +                     opp-microvolt = <750000 750000 950000>;
> > +                     clock-latency-ns = <40000>;
> > +             };
> > +             opp-1608000000 {
> > +                     opp-hz = /bits/ 64 <1608000000>;
> > +                     opp-microvolt = <787500 787500 950000>;
> > +                     clock-latency-ns = <40000>;
> > +             };
> > +             opp-1800000000 {
> > +                     opp-hz = /bits/ 64 <1800000000>;
> > +                     opp-microvolt = <875000 875000 950000>;
> > +                     clock-latency-ns = <40000>;
> > +             };
> > +             opp-2016000000 {
> > +                     opp-hz = /bits/ 64 <2016000000>;
> > +                     opp-microvolt = <950000 950000 950000>;
> > +                     clock-latency-ns = <40000>;
> > +             };
>
> opp-1800000000 and opp-2016000000 should be removed as well.
>
> Did I misunderstand what Rockchip did here? Adding Kever in Cc at least
> for awareness on Rockchip side :)
>
> I guess another option could be to mark those above as
>
> turbo-mode;
>
> though no clue what this would entail. From a glance at cpufreq, it
> seems that for Rockchip since we use the default cpufreq-dt, it would
> mark those as unusable, so essentially a no-op, so better remove them
> entirely?

I believe the opp core just marks 'turbo-mode' opps as
CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to the
cpufreq core. But then to actually use those boost frequencies I would
guess the governor needs to somehow know the power/thermal constraints
of the chip?.. Don't know where that is defined.

Best,
Alexey

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Heiko Stübner]

Am Samstag, 15. Februar 2025, 19:59:44 MEZ schrieb Alexey Charkov:
> On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
> > On 6/17/24 8:28 PM, Alexey Charkov wrote:
> > > RK3588j is the 'industrial' variant of RK3588, and it uses a different
> > > set of OPPs both in terms of allowed frequencies and in terms of
> > > applicable voltages at each frequency setpoint.
> > >
> > > Add the OPPs that apply to RK3588j (and apparently RK3588m too) to
> > > enable dynamic CPU frequency scaling.
> > >
> > > OPP values are derived from Rockchip downstream sources [1] by taking
> > > only those OPPs which have the highest frequency for a given voltage
> > > level and dropping the rest (if they are included, the kernel complains
> > > at boot time about them being inefficient)
> > >
> > > [1] https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > >
> >
> > Funny stuff actually. Rockchip have their own downstream cpufreq driver
> > which autodetects at runtime the SoC variant and filter out OPPs based
> > on that info. And this patch is based on those values and filters.
> >
> > However, they also decided that maybe this isn't the best way to do it
> > and they decided to have an rk3588j.dtsi where they remove some of those
> > OPPs instead of just updating the mask/filter in their base dtsi
> > (rk3588s.dtsi downstream). See
> > https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> 
> Funny stuff indeed! Judging by the comments in the file you
> referenced, those higher OPP values constitute an 'overdrive' mode,
> and apparently the chip shouldn't stay in those for prolonged periods
> of time. However, I couldn't locate any dts file inside Rockchip
> sources that would include this rk3588j.dtsi - so not sure if we
> should follow what it says too zealously.
> 
> > So...
> >
> > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > > ---
> > >   arch/arm64/boot/dts/rockchip/rk3588j.dtsi | 108 ++++++++++++++++++++++++++++++
> > >   1 file changed, 108 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> > > index 0bbeee399a63..b7e69553857b 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> > > @@ -5,3 +5,111 @@
> > >    */
> > >
> > >   #include "rk3588-extra.dtsi"
> > > +
> > > +/ {
> > > +     cluster0_opp_table: opp-table-cluster0 {
> > > +             compatible = "operating-points-v2";
> > > +             opp-shared;
> > > +
> > > +             opp-1416000000 {
> > > +                     opp-hz = /bits/ 64 <1416000000>;
> > > +                     opp-microvolt = <750000 750000 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +                     opp-suspend;
> > > +             };
> > > +             opp-1608000000 {
> > > +                     opp-hz = /bits/ 64 <1608000000>;
> > > +                     opp-microvolt = <887500 887500 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +             };
> > > +             opp-1704000000 {
> > > +                     opp-hz = /bits/ 64 <1704000000>;
> > > +                     opp-microvolt = <937500 937500 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +             };
> >
> > None of those are valid according to Rockchip, we should have
> 
> Well, valid but more taxing on the hardware apparently.
> 
> >                 opp-1296000000 {
> >                         opp-hz = /bits/ 64 <1296000000>;
> >                         opp-microvolt = <750000 750000 950000>;
> >                         clock-latency-ns = <40000>;
> >                         opp-suspend;
> >                 };
> >
> > instead?
> 
> I dropped this one because it uses a lower frequency but same voltage
> as the 1.416 GHz one, thus being 'inefficient' as the thermal
> subsystem says in the logs. It can be added back if we decide to
> remove opp-1416000000.
> 
> > and...
> >
> > > +     };
> > > +
> > > +     cluster1_opp_table: opp-table-cluster1 {
> > > +             compatible = "operating-points-v2";
> > > +             opp-shared;
> > > +
> > > +             opp-1416000000 {
> > > +                     opp-hz = /bits/ 64 <1416000000>;
> > > +                     opp-microvolt = <750000 750000 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +             };
> > > +             opp-1608000000 {
> > > +                     opp-hz = /bits/ 64 <1608000000>;
> > > +                     opp-microvolt = <787500 787500 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +             };
> > > +             opp-1800000000 {
> > > +                     opp-hz = /bits/ 64 <1800000000>;
> > > +                     opp-microvolt = <875000 875000 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +             };
> > > +             opp-2016000000 {
> > > +                     opp-hz = /bits/ 64 <2016000000>;
> > > +                     opp-microvolt = <950000 950000 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +             };
> >
> > opp-1800000000 and opp-2016000000 should be removed.
> >
> > and...
> >
> > > +     };
> > > +
> > > +     cluster2_opp_table: opp-table-cluster2 {
> > > +             compatible = "operating-points-v2";
> > > +             opp-shared;
> > > +
> > > +             opp-1416000000 {
> > > +                     opp-hz = /bits/ 64 <1416000000>;
> > > +                     opp-microvolt = <750000 750000 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +             };
> > > +             opp-1608000000 {
> > > +                     opp-hz = /bits/ 64 <1608000000>;
> > > +                     opp-microvolt = <787500 787500 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +             };
> > > +             opp-1800000000 {
> > > +                     opp-hz = /bits/ 64 <1800000000>;
> > > +                     opp-microvolt = <875000 875000 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +             };
> > > +             opp-2016000000 {
> > > +                     opp-hz = /bits/ 64 <2016000000>;
> > > +                     opp-microvolt = <950000 950000 950000>;
> > > +                     clock-latency-ns = <40000>;
> > > +             };
> >
> > opp-1800000000 and opp-2016000000 should be removed as well.
> >
> > Did I misunderstand what Rockchip did here? Adding Kever in Cc at least
> > for awareness on Rockchip side :)
> >
> > I guess another option could be to mark those above as
> >
> > turbo-mode;
> >
> > though no clue what this would entail. From a glance at cpufreq, it
> > seems that for Rockchip since we use the default cpufreq-dt, it would
> > mark those as unusable, so essentially a no-op, so better remove them
> > entirely?
> 
> I believe the opp core just marks 'turbo-mode' opps as
> CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to the
> cpufreq core. But then to actually use those boost frequencies I would
> guess the governor needs to somehow know the power/thermal constraints
> of the chip?.. Don't know where that is defined.

personally I don't believe in "boost" frequencies - except when they are
declared officially.

Rockchip for the longest time maintains that running the chip outside
the declared power/rate envelope can reduce its overall lifetime.

So for Rockchip in mainline a "it runs stable for me" has never been a
suitable reasoning ;-) .

So while the situation might be strange for the rk3588j, I really only want
correct frequencies here please - not any pseudo "turbo freqs".

I don't care that much what people do on their own device{s/trees}, but
the devicetrees we supply centrally (and to u-boot, etc) should only
contain frequencies vetted by the manufacturer.


Heiko

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Dragan Simic]

Hello all,

On 2025-02-15 21:30, Heiko Stübner wrote:
> Am Samstag, 15. Februar 2025, 19:59:44 MEZ schrieb Alexey Charkov:
>> On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz 
>> <quentin.schulz@cherry.de> wrote:
>> > On 6/17/24 8:28 PM, Alexey Charkov wrote:
>> > > RK3588j is the 'industrial' variant of RK3588, and it uses a different
>> > > set of OPPs both in terms of allowed frequencies and in terms of
>> > > applicable voltages at each frequency setpoint.
>> > >
>> > > Add the OPPs that apply to RK3588j (and apparently RK3588m too) to
>> > > enable dynamic CPU frequency scaling.
>> > >
>> > > OPP values are derived from Rockchip downstream sources [1] by taking
>> > > only those OPPs which have the highest frequency for a given voltage
>> > > level and dropping the rest (if they are included, the kernel complains
>> > > at boot time about them being inefficient)
>> > >
>> > > [1] https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> > >
>> >
>> > Funny stuff actually. Rockchip have their own downstream cpufreq driver
>> > which autodetects at runtime the SoC variant and filter out OPPs based
>> > on that info. And this patch is based on those values and filters.
>> >
>> > However, they also decided that maybe this isn't the best way to do it
>> > and they decided to have an rk3588j.dtsi where they remove some of those
>> > OPPs instead of just updating the mask/filter in their base dtsi
>> > (rk3588s.dtsi downstream). See
>> > https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> 
>> Funny stuff indeed! Judging by the comments in the file you
>> referenced, those higher OPP values constitute an 'overdrive' mode,
>> and apparently the chip shouldn't stay in those for prolonged periods
>> of time. However, I couldn't locate any dts file inside Rockchip
>> sources that would include this rk3588j.dtsi - so not sure if we
>> should follow what it says too zealously.
>> 
>> > So...
>> >
>> > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
>> > > ---
>> > >   arch/arm64/boot/dts/rockchip/rk3588j.dtsi | 108 ++++++++++++++++++++++++++++++
>> > >   1 file changed, 108 insertions(+)
>> > >
>> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> > > index 0bbeee399a63..b7e69553857b 100644
>> > > --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>> > > @@ -5,3 +5,111 @@
>> > >    */
>> > >
>> > >   #include "rk3588-extra.dtsi"
>> > > +
>> > > +/ {
>> > > +     cluster0_opp_table: opp-table-cluster0 {
>> > > +             compatible = "operating-points-v2";
>> > > +             opp-shared;
>> > > +
>> > > +             opp-1416000000 {
>> > > +                     opp-hz = /bits/ 64 <1416000000>;
>> > > +                     opp-microvolt = <750000 750000 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +                     opp-suspend;
>> > > +             };
>> > > +             opp-1608000000 {
>> > > +                     opp-hz = /bits/ 64 <1608000000>;
>> > > +                     opp-microvolt = <887500 887500 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +             };
>> > > +             opp-1704000000 {
>> > > +                     opp-hz = /bits/ 64 <1704000000>;
>> > > +                     opp-microvolt = <937500 937500 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +             };
>> >
>> > None of those are valid according to Rockchip, we should have
>> 
>> Well, valid but more taxing on the hardware apparently.
>> 
>> >                 opp-1296000000 {
>> >                         opp-hz = /bits/ 64 <1296000000>;
>> >                         opp-microvolt = <750000 750000 950000>;
>> >                         clock-latency-ns = <40000>;
>> >                         opp-suspend;
>> >                 };
>> >
>> > instead?
>> 
>> I dropped this one because it uses a lower frequency but same voltage
>> as the 1.416 GHz one, thus being 'inefficient' as the thermal
>> subsystem says in the logs. It can be added back if we decide to
>> remove opp-1416000000.
>> 
>> > and...
>> >
>> > > +     };
>> > > +
>> > > +     cluster1_opp_table: opp-table-cluster1 {
>> > > +             compatible = "operating-points-v2";
>> > > +             opp-shared;
>> > > +
>> > > +             opp-1416000000 {
>> > > +                     opp-hz = /bits/ 64 <1416000000>;
>> > > +                     opp-microvolt = <750000 750000 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +             };
>> > > +             opp-1608000000 {
>> > > +                     opp-hz = /bits/ 64 <1608000000>;
>> > > +                     opp-microvolt = <787500 787500 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +             };
>> > > +             opp-1800000000 {
>> > > +                     opp-hz = /bits/ 64 <1800000000>;
>> > > +                     opp-microvolt = <875000 875000 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +             };
>> > > +             opp-2016000000 {
>> > > +                     opp-hz = /bits/ 64 <2016000000>;
>> > > +                     opp-microvolt = <950000 950000 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +             };
>> >
>> > opp-1800000000 and opp-2016000000 should be removed.
>> >
>> > and...
>> >
>> > > +     };
>> > > +
>> > > +     cluster2_opp_table: opp-table-cluster2 {
>> > > +             compatible = "operating-points-v2";
>> > > +             opp-shared;
>> > > +
>> > > +             opp-1416000000 {
>> > > +                     opp-hz = /bits/ 64 <1416000000>;
>> > > +                     opp-microvolt = <750000 750000 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +             };
>> > > +             opp-1608000000 {
>> > > +                     opp-hz = /bits/ 64 <1608000000>;
>> > > +                     opp-microvolt = <787500 787500 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +             };
>> > > +             opp-1800000000 {
>> > > +                     opp-hz = /bits/ 64 <1800000000>;
>> > > +                     opp-microvolt = <875000 875000 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +             };
>> > > +             opp-2016000000 {
>> > > +                     opp-hz = /bits/ 64 <2016000000>;
>> > > +                     opp-microvolt = <950000 950000 950000>;
>> > > +                     clock-latency-ns = <40000>;
>> > > +             };
>> >
>> > opp-1800000000 and opp-2016000000 should be removed as well.
>> >
>> > Did I misunderstand what Rockchip did here? Adding Kever in Cc at least
>> > for awareness on Rockchip side :)
>> >
>> > I guess another option could be to mark those above as
>> >
>> > turbo-mode;
>> >
>> > though no clue what this would entail. From a glance at cpufreq, it
>> > seems that for Rockchip since we use the default cpufreq-dt, it would
>> > mark those as unusable, so essentially a no-op, so better remove them
>> > entirely?
>> 
>> I believe the opp core just marks 'turbo-mode' opps as
>> CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to the
>> cpufreq core. But then to actually use those boost frequencies I would
>> guess the governor needs to somehow know the power/thermal constraints
>> of the chip?.. Don't know where that is defined.
> 
> personally I don't believe in "boost" frequencies - except when they 
> are
> declared officially.
> 
> Rockchip for the longest time maintains that running the chip outside
> the declared power/rate envelope can reduce its overall lifetime.

FWIW, as an additional data point, some other SoC manufacturers
officially state that the expected lifetime of their chips gets
reduced when they are run at higher temperatures, which are still
within the permitted temperature ranges.

> So for Rockchip in mainline a "it runs stable for me" has never been a
> suitable reasoning ;-) .
> 
> So while the situation might be strange for the rk3588j, I really only 
> want
> correct frequencies here please - not any pseudo "turbo freqs".
> 
> I don't care that much what people do on their own device{s/trees}, but
> the devicetrees we supply centrally (and to u-boot, etc) should only
> contain frequencies vetted by the manufacturer.

Totally agreed, "works for me" is simply not applicable here.  As we
know, some samples of the same CPU or SoC may be luckier when it comes
to the silicon lottery that got the binned, meaning that only the most
conservative frequencies and voltages, as assigned by the manufacturer,
are what we can provide as the OPPs.

I'm having a more detailed look into this, and I'll come back with,
hopefully, some additional comments. :)

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Alexey Charkov]

Hi Heiko,

On Sat, Feb 15, 2025 at 11:30 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Samstag, 15. Februar 2025, 19:59:44 MEZ schrieb Alexey Charkov:
> > On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
> > > On 6/17/24 8:28 PM, Alexey Charkov wrote:
> > > > RK3588j is the 'industrial' variant of RK3588, and it uses a different
> > > > set of OPPs both in terms of allowed frequencies and in terms of
> > > > applicable voltages at each frequency setpoint.
> > > >
> > > > Add the OPPs that apply to RK3588j (and apparently RK3588m too) to
> > > > enable dynamic CPU frequency scaling.
> > > >
> > > > OPP values are derived from Rockchip downstream sources [1] by taking
> > > > only those OPPs which have the highest frequency for a given voltage
> > > > level and dropping the rest (if they are included, the kernel complains
> > > > at boot time about them being inefficient)
> > > >
> > > > [1] https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > >
> > >
> > > Funny stuff actually. Rockchip have their own downstream cpufreq driver
> > > which autodetects at runtime the SoC variant and filter out OPPs based
> > > on that info. And this patch is based on those values and filters.
> > >
> > > However, they also decided that maybe this isn't the best way to do it
> > > and they decided to have an rk3588j.dtsi where they remove some of those
> > > OPPs instead of just updating the mask/filter in their base dtsi
> > > (rk3588s.dtsi downstream). See
> > > https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> >
> > Funny stuff indeed! Judging by the comments in the file you
> > referenced, those higher OPP values constitute an 'overdrive' mode,
> > and apparently the chip shouldn't stay in those for prolonged periods
> > of time. However, I couldn't locate any dts file inside Rockchip
> > sources that would include this rk3588j.dtsi - so not sure if we
> > should follow what it says too zealously.
> >
> > > So...
> > >
> > > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > > > ---
> > > >   arch/arm64/boot/dts/rockchip/rk3588j.dtsi | 108 ++++++++++++++++++++++++++++++
> > > >   1 file changed, 108 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> > > > index 0bbeee399a63..b7e69553857b 100644
> > > > --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> > > > +++ b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> > > > @@ -5,3 +5,111 @@
> > > >    */
> > > >
> > > >   #include "rk3588-extra.dtsi"
> > > > +
> > > > +/ {
> > > > +     cluster0_opp_table: opp-table-cluster0 {
> > > > +             compatible = "operating-points-v2";
> > > > +             opp-shared;
> > > > +
> > > > +             opp-1416000000 {
> > > > +                     opp-hz = /bits/ 64 <1416000000>;
> > > > +                     opp-microvolt = <750000 750000 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +                     opp-suspend;
> > > > +             };
> > > > +             opp-1608000000 {
> > > > +                     opp-hz = /bits/ 64 <1608000000>;
> > > > +                     opp-microvolt = <887500 887500 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +             };
> > > > +             opp-1704000000 {
> > > > +                     opp-hz = /bits/ 64 <1704000000>;
> > > > +                     opp-microvolt = <937500 937500 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +             };
> > >
> > > None of those are valid according to Rockchip, we should have
> >
> > Well, valid but more taxing on the hardware apparently.
> >
> > >                 opp-1296000000 {
> > >                         opp-hz = /bits/ 64 <1296000000>;
> > >                         opp-microvolt = <750000 750000 950000>;
> > >                         clock-latency-ns = <40000>;
> > >                         opp-suspend;
> > >                 };
> > >
> > > instead?
> >
> > I dropped this one because it uses a lower frequency but same voltage
> > as the 1.416 GHz one, thus being 'inefficient' as the thermal
> > subsystem says in the logs. It can be added back if we decide to
> > remove opp-1416000000.
> >
> > > and...
> > >
> > > > +     };
> > > > +
> > > > +     cluster1_opp_table: opp-table-cluster1 {
> > > > +             compatible = "operating-points-v2";
> > > > +             opp-shared;
> > > > +
> > > > +             opp-1416000000 {
> > > > +                     opp-hz = /bits/ 64 <1416000000>;
> > > > +                     opp-microvolt = <750000 750000 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +             };
> > > > +             opp-1608000000 {
> > > > +                     opp-hz = /bits/ 64 <1608000000>;
> > > > +                     opp-microvolt = <787500 787500 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +             };
> > > > +             opp-1800000000 {
> > > > +                     opp-hz = /bits/ 64 <1800000000>;
> > > > +                     opp-microvolt = <875000 875000 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +             };
> > > > +             opp-2016000000 {
> > > > +                     opp-hz = /bits/ 64 <2016000000>;
> > > > +                     opp-microvolt = <950000 950000 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +             };
> > >
> > > opp-1800000000 and opp-2016000000 should be removed.
> > >
> > > and...
> > >
> > > > +     };
> > > > +
> > > > +     cluster2_opp_table: opp-table-cluster2 {
> > > > +             compatible = "operating-points-v2";
> > > > +             opp-shared;
> > > > +
> > > > +             opp-1416000000 {
> > > > +                     opp-hz = /bits/ 64 <1416000000>;
> > > > +                     opp-microvolt = <750000 750000 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +             };
> > > > +             opp-1608000000 {
> > > > +                     opp-hz = /bits/ 64 <1608000000>;
> > > > +                     opp-microvolt = <787500 787500 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +             };
> > > > +             opp-1800000000 {
> > > > +                     opp-hz = /bits/ 64 <1800000000>;
> > > > +                     opp-microvolt = <875000 875000 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +             };
> > > > +             opp-2016000000 {
> > > > +                     opp-hz = /bits/ 64 <2016000000>;
> > > > +                     opp-microvolt = <950000 950000 950000>;
> > > > +                     clock-latency-ns = <40000>;
> > > > +             };
> > >
> > > opp-1800000000 and opp-2016000000 should be removed as well.
> > >
> > > Did I misunderstand what Rockchip did here? Adding Kever in Cc at least
> > > for awareness on Rockchip side :)
> > >
> > > I guess another option could be to mark those above as
> > >
> > > turbo-mode;
> > >
> > > though no clue what this would entail. From a glance at cpufreq, it
> > > seems that for Rockchip since we use the default cpufreq-dt, it would
> > > mark those as unusable, so essentially a no-op, so better remove them
> > > entirely?
> >
> > I believe the opp core just marks 'turbo-mode' opps as
> > CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to the
> > cpufreq core. But then to actually use those boost frequencies I would
> > guess the governor needs to somehow know the power/thermal constraints
> > of the chip?.. Don't know where that is defined.
>
> personally I don't believe in "boost" frequencies - except when they are
> declared officially.
>
> Rockchip for the longest time maintains that running the chip outside
> the declared power/rate envelope can reduce its overall lifetime.
>
> So for Rockchip in mainline a "it runs stable for me" has never been a
> suitable reasoning ;-) .

My key concern here was perhaps that we are looking at a file inside
the Rockchip source tree which looks relevant by the name of it, but
doesn't actually get included anywhere for any of the boards. This may
or may not constitute an endorsement by Rockchip of any OPPs listed
there :-D

I haven't seen a TRM for the J variant, nor do I have a board with
RK3588J to run tests, so it would be great if Kever could chime in
with how these OPPs are different from the others (or not).

> So while the situation might be strange for the rk3588j, I really only want
> correct frequencies here please - not any pseudo "turbo freqs".
>
> I don't care that much what people do on their own device{s/trees}, but
> the devicetrees we supply centrally (and to u-boot, etc) should only
> contain frequencies vetted by the manufacturer.

Fair enough. Let's just try and get another data point on whether
these frequencies are vetted or not.

Best regards,
Alexey

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Quentin Schulz]

Hi all,

On 2/16/25 1:32 PM, Alexey Charkov wrote:
> Hi Heiko,
> 
> On Sat, Feb 15, 2025 at 11:30 PM Heiko Stübner <heiko@sntech.de> wrote:
>>
>> Am Samstag, 15. Februar 2025, 19:59:44 MEZ schrieb Alexey Charkov:
>>> On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>> On 6/17/24 8:28 PM, Alexey Charkov wrote:
>>>>> RK3588j is the 'industrial' variant of RK3588, and it uses a different
>>>>> set of OPPs both in terms of allowed frequencies and in terms of
>>>>> applicable voltages at each frequency setpoint.
>>>>>
>>>>> Add the OPPs that apply to RK3588j (and apparently RK3588m too) to
>>>>> enable dynamic CPU frequency scaling.
>>>>>
>>>>> OPP values are derived from Rockchip downstream sources [1] by taking
>>>>> only those OPPs which have the highest frequency for a given voltage
>>>>> level and dropping the rest (if they are included, the kernel complains
>>>>> at boot time about them being inefficient)
>>>>>
>>>>> [1] https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>>>>>
>>>>
>>>> Funny stuff actually. Rockchip have their own downstream cpufreq driver
>>>> which autodetects at runtime the SoC variant and filter out OPPs based
>>>> on that info. And this patch is based on those values and filters.
>>>>
>>>> However, they also decided that maybe this isn't the best way to do it
>>>> and they decided to have an rk3588j.dtsi where they remove some of those
>>>> OPPs instead of just updating the mask/filter in their base dtsi
>>>> (rk3588s.dtsi downstream). See
>>>> https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>>>
>>> Funny stuff indeed! Judging by the comments in the file you
>>> referenced, those higher OPP values constitute an 'overdrive' mode,
>>> and apparently the chip shouldn't stay in those for prolonged periods
>>> of time. However, I couldn't locate any dts file inside Rockchip
>>> sources that would include this rk3588j.dtsi - so not sure if we
>>> should follow what it says too zealously.
>>>

That was a clear oversight on my side.

The commit adding support for the J/M OPPs in rk3588s.dtsi downstream 
just precedes the one adding the removal of the J OPPs in rk3588j.dtsi, 
so not sure what was the intended effect there. I'll open a ticket with 
them to see if I can get some answer until/unless Kever chimes in.

>>>> So...
>>>>
>>>>> Signed-off-by: Alexey Charkov <alchark@gmail.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/rockchip/rk3588j.dtsi | 108 ++++++++++++++++++++++++++++++
>>>>>    1 file changed, 108 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>>>>> index 0bbeee399a63..b7e69553857b 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
>>>>> @@ -5,3 +5,111 @@
>>>>>     */
>>>>>
>>>>>    #include "rk3588-extra.dtsi"
>>>>> +
>>>>> +/ {
>>>>> +     cluster0_opp_table: opp-table-cluster0 {
>>>>> +             compatible = "operating-points-v2";
>>>>> +             opp-shared;
>>>>> +
>>>>> +             opp-1416000000 {
>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
>>>>> +                     opp-microvolt = <750000 750000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +                     opp-suspend;
>>>>> +             };
>>>>> +             opp-1608000000 {
>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
>>>>> +                     opp-microvolt = <887500 887500 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1704000000 {
>>>>> +                     opp-hz = /bits/ 64 <1704000000>;
>>>>> +                     opp-microvolt = <937500 937500 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>
>>>> None of those are valid according to Rockchip, we should have
>>>
>>> Well, valid but more taxing on the hardware apparently.
>>>
>>>>                  opp-1296000000 {
>>>>                          opp-hz = /bits/ 64 <1296000000>;
>>>>                          opp-microvolt = <750000 750000 950000>;
>>>>                          clock-latency-ns = <40000>;
>>>>                          opp-suspend;
>>>>                  };
>>>>
>>>> instead?
>>>
>>> I dropped this one because it uses a lower frequency but same voltage
>>> as the 1.416 GHz one, thus being 'inefficient' as the thermal
>>> subsystem says in the logs. It can be added back if we decide to
>>> remove opp-1416000000.

That was exactly my point, the 1.296GHz OPP would then be the highest 
frequency at that voltage.

>>>
>>>> and...
>>>>
>>>>> +     };
>>>>> +
>>>>> +     cluster1_opp_table: opp-table-cluster1 {
>>>>> +             compatible = "operating-points-v2";
>>>>> +             opp-shared;
>>>>> +
>>>>> +             opp-1416000000 {
>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
>>>>> +                     opp-microvolt = <750000 750000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1608000000 {
>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
>>>>> +                     opp-microvolt = <787500 787500 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1800000000 {
>>>>> +                     opp-hz = /bits/ 64 <1800000000>;
>>>>> +                     opp-microvolt = <875000 875000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-2016000000 {
>>>>> +                     opp-hz = /bits/ 64 <2016000000>;
>>>>> +                     opp-microvolt = <950000 950000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>
>>>> opp-1800000000 and opp-2016000000 should be removed.
>>>>
>>>> and...
>>>>
>>>>> +     };
>>>>> +
>>>>> +     cluster2_opp_table: opp-table-cluster2 {
>>>>> +             compatible = "operating-points-v2";
>>>>> +             opp-shared;
>>>>> +
>>>>> +             opp-1416000000 {
>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
>>>>> +                     opp-microvolt = <750000 750000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1608000000 {
>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
>>>>> +                     opp-microvolt = <787500 787500 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1800000000 {
>>>>> +                     opp-hz = /bits/ 64 <1800000000>;
>>>>> +                     opp-microvolt = <875000 875000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-2016000000 {
>>>>> +                     opp-hz = /bits/ 64 <2016000000>;
>>>>> +                     opp-microvolt = <950000 950000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>
>>>> opp-1800000000 and opp-2016000000 should be removed as well.
>>>>
>>>> Did I misunderstand what Rockchip did here? Adding Kever in Cc at least
>>>> for awareness on Rockchip side :)
>>>>
>>>> I guess another option could be to mark those above as
>>>>
>>>> turbo-mode;
>>>>
>>>> though no clue what this would entail. From a glance at cpufreq, it
>>>> seems that for Rockchip since we use the default cpufreq-dt, it would
>>>> mark those as unusable, so essentially a no-op, so better remove them
>>>> entirely?
>>>
>>> I believe the opp core just marks 'turbo-mode' opps as
>>> CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to the
>>> cpufreq core. But then to actually use those boost frequencies I would
>>> guess the governor needs to somehow know the power/thermal constraints
>>> of the chip?.. Don't know where that is defined.
>>
>> personally I don't believe in "boost" frequencies - except when they are
>> declared officially.
>>
>> Rockchip for the longest time maintains that running the chip outside
>> the declared power/rate envelope can reduce its overall lifetime.
>>
>> So for Rockchip in mainline a "it runs stable for me" has never been a
>> suitable reasoning ;-) .
> 
> My key concern here was perhaps that we are looking at a file inside
> the Rockchip source tree which looks relevant by the name of it, but
> doesn't actually get included anywhere for any of the boards. This may
> or may not constitute an endorsement by Rockchip of any OPPs listed
> there :-D
> 

Will ask for confirmation through their ticket system.

> I haven't seen a TRM for the J variant, nor do I have a board with
> RK3588J to run tests, so it would be great if Kever could chime in
> with how these OPPs are different from the others (or not).
> 

I do have access to some J variant product but that won't help if you 
need to run the boards for months/years at the highest freq in a 
temperature-controlled chamber at the highest supported temperature for 
the SoC. I don't think there's a TRM for the J variant, that wouldn't 
make sense as it's the exact same SoC, just a different binning. 
Similarly to what was done for RK3588S, RK3588S2, RK3582, RK3583, etc.. 
I don't think there's a TRM for those, but there's a datasheet, c.f. 
https://www.lcsc.com/datasheet/lcsc_datasheet_2403201054_Rockchip-RK3588J_C22364189.pdf 
but that says 1.6GHz for the A76 cores and 1.3GHz for the A55 while the 
the j-m OPP in rk3558s.dtsi downstream list the highest as 1.7GHz (but 
rk3588j.dtsi removes it).

Will report if Rockchip has answered on their ticket system.

Cheers,
Quentin

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Quentin Schulz]

Hi all,

On 2/16/25 1:32 PM, Alexey Charkov wrote:
> Hi Heiko,
> 
> On Sat, Feb 15, 2025 at 11:30 PM Heiko Stübner <heiko@sntech.de> wrote:
>>
>> Am Samstag, 15. Februar 2025, 19:59:44 MEZ schrieb Alexey Charkov:
>>> On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz <quentin.schulz@cherry.de> wrote:
>>>> On 6/17/24 8:28 PM, Alexey Charkov wrote:
[...]
>>>>> +     };
>>>>> +
>>>>> +     cluster2_opp_table: opp-table-cluster2 {
>>>>> +             compatible = "operating-points-v2";
>>>>> +             opp-shared;
>>>>> +
>>>>> +             opp-1416000000 {
>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
>>>>> +                     opp-microvolt = <750000 750000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1608000000 {
>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
>>>>> +                     opp-microvolt = <787500 787500 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-1800000000 {
>>>>> +                     opp-hz = /bits/ 64 <1800000000>;
>>>>> +                     opp-microvolt = <875000 875000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>> +             opp-2016000000 {
>>>>> +                     opp-hz = /bits/ 64 <2016000000>;
>>>>> +                     opp-microvolt = <950000 950000 950000>;
>>>>> +                     clock-latency-ns = <40000>;
>>>>> +             };
>>>>
>>>> opp-1800000000 and opp-2016000000 should be removed as well.
>>>>
>>>> Did I misunderstand what Rockchip did here? Adding Kever in Cc at least
>>>> for awareness on Rockchip side :)
>>>>
>>>> I guess another option could be to mark those above as
>>>>
>>>> turbo-mode;
>>>>
>>>> though no clue what this would entail. From a glance at cpufreq, it
>>>> seems that for Rockchip since we use the default cpufreq-dt, it would
>>>> mark those as unusable, so essentially a no-op, so better remove them
>>>> entirely?
>>>
>>> I believe the opp core just marks 'turbo-mode' opps as
>>> CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to the
>>> cpufreq core. But then to actually use those boost frequencies I would
>>> guess the governor needs to somehow know the power/thermal constraints
>>> of the chip?.. Don't know where that is defined.
>>
>> personally I don't believe in "boost" frequencies - except when they are
>> declared officially.
>>
>> Rockchip for the longest time maintains that running the chip outside
>> the declared power/rate envelope can reduce its overall lifetime.
>>
>> So for Rockchip in mainline a "it runs stable for me" has never been a
>> suitable reasoning ;-) .
> 
> My key concern here was perhaps that we are looking at a file inside
> the Rockchip source tree which looks relevant by the name of it, but
> doesn't actually get included anywhere for any of the boards. This may
> or may not constitute an endorsement by Rockchip of any OPPs listed
> there :-D
> 

Rockchip support stated:

"""
If you run overdrive mode, you do not need to include rk3588j.dtsi, and 
you can change it to #incldue rk3588.dtsi to ensure that the maximum 
frequency can reach 2GHz

below picture from datasheet.
"""

The picture is the table 3-2 Recommended operating conditions, page 30 
from the RK3588J datasheet, e.g. 
https://www.lcsc.com/datasheet/lcsc_datasheet_2403201054_Rockchip-RK3588J_C22364189.pdf

What is overdrive?

"""
Overdrive mode brings higher frequency, and the voltage will increase 
accordingly. Under
the overdrive mode for a long time, the chipset may shorten the 
lifetime, especially in
high temperature condition
"""

according to the same datasheet, end of the same table, page 31.

so this seems like a turbo-mode frequency to me.

Additionally, the note for the "normal mode" (the one with the lower 
freqs) states:

"""
Normal mode means the chipset works under safety voltage and frequency. 
For the
industrial environment, highly recommend to keep in normal mode, the 
lifetime is
reasonably guaranteed.
"""

I believe "industrial environment" means RK3588J used in the 
temperatures that aren't OK for the standard RK3588 variant?

> I haven't seen a TRM for the J variant, nor do I have a board with
> RK3588J to run tests, so it would be great if Kever could chime in
> with how these OPPs are different from the others (or not).
> 
>> So while the situation might be strange for the rk3588j, I really only want
>> correct frequencies here please - not any pseudo "turbo freqs".
>>
>> I don't care that much what people do on their own device{s/trees}, but
>> the devicetrees we supply centrally (and to u-boot, etc) should only
>> contain frequencies vetted by the manufacturer.
> 
> Fair enough. Let's just try and get another data point on whether
> these frequencies are vetted or not.
> 

So the higher freqs seems to be vetted (and used by default on 
Rockchip's BSP kernel), it just feels like you aren't really supposed to 
run those higher frequencies all the time? And especially not in 
"industrial environment"?

I would assume we want to stay on the safer side and remove those higher 
frequencies? Heiko?

Cheers,
Quentin

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Dragan Simic]

Hello Quentin,

On 2025-03-12 11:15, Quentin Schulz wrote:
> On 2/16/25 1:32 PM, Alexey Charkov wrote:
>> On Sat, Feb 15, 2025 at 11:30 PM Heiko Stübner <heiko@sntech.de> 
>> wrote:
>>> Am Samstag, 15. Februar 2025, 19:59:44 MEZ schrieb Alexey Charkov:
>>>> On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz 
>>>> <quentin.schulz@cherry.de> wrote:
>>>>> On 6/17/24 8:28 PM, Alexey Charkov wrote:
> [...]
>>>>>> +     };
>>>>>> +
>>>>>> +     cluster2_opp_table: opp-table-cluster2 {
>>>>>> +             compatible = "operating-points-v2";
>>>>>> +             opp-shared;
>>>>>> +
>>>>>> +             opp-1416000000 {
>>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
>>>>>> +                     opp-microvolt = <750000 750000 950000>;
>>>>>> +                     clock-latency-ns = <40000>;
>>>>>> +             };
>>>>>> +             opp-1608000000 {
>>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
>>>>>> +                     opp-microvolt = <787500 787500 950000>;
>>>>>> +                     clock-latency-ns = <40000>;
>>>>>> +             };
>>>>>> +             opp-1800000000 {
>>>>>> +                     opp-hz = /bits/ 64 <1800000000>;
>>>>>> +                     opp-microvolt = <875000 875000 950000>;
>>>>>> +                     clock-latency-ns = <40000>;
>>>>>> +             };
>>>>>> +             opp-2016000000 {
>>>>>> +                     opp-hz = /bits/ 64 <2016000000>;
>>>>>> +                     opp-microvolt = <950000 950000 950000>;
>>>>>> +                     clock-latency-ns = <40000>;
>>>>>> +             };
>>>>> 
>>>>> opp-1800000000 and opp-2016000000 should be removed as well.
>>>>> 
>>>>> Did I misunderstand what Rockchip did here? Adding Kever in Cc at 
>>>>> least
>>>>> for awareness on Rockchip side :)
>>>>> 
>>>>> I guess another option could be to mark those above as
>>>>> 
>>>>> turbo-mode;
>>>>> 
>>>>> though no clue what this would entail. From a glance at cpufreq, it
>>>>> seems that for Rockchip since we use the default cpufreq-dt, it 
>>>>> would
>>>>> mark those as unusable, so essentially a no-op, so better remove 
>>>>> them
>>>>> entirely?
>>>> 
>>>> I believe the opp core just marks 'turbo-mode' opps as
>>>> CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to 
>>>> the
>>>> cpufreq core. But then to actually use those boost frequencies I 
>>>> would
>>>> guess the governor needs to somehow know the power/thermal 
>>>> constraints
>>>> of the chip?.. Don't know where that is defined.
>>> 
>>> personally I don't believe in "boost" frequencies - except when they 
>>> are
>>> declared officially.
>>> 
>>> Rockchip for the longest time maintains that running the chip outside
>>> the declared power/rate envelope can reduce its overall lifetime.
>>> 
>>> So for Rockchip in mainline a "it runs stable for me" has never been 
>>> a
>>> suitable reasoning ;-) .
>> 
>> My key concern here was perhaps that we are looking at a file inside
>> the Rockchip source tree which looks relevant by the name of it, but
>> doesn't actually get included anywhere for any of the boards. This may
>> or may not constitute an endorsement by Rockchip of any OPPs listed
>> there :-D
> 
> Rockchip support stated:
> 
> """
> If you run overdrive mode, you do not need to include rk3588j.dtsi,
> and you can change it to #incldue rk3588.dtsi to ensure that the
> maximum frequency can reach 2GHz
> 
> below picture from datasheet.
> """
> 
> The picture is the table 3-2 Recommended operating conditions, page 30
> from the RK3588J datasheet, e.g.
> https://www.lcsc.com/datasheet/lcsc_datasheet_2403201054_Rockchip-RK3588J_C22364189.pdf
> 
> What is overdrive?
> 
> """
> Overdrive mode brings higher frequency, and the voltage will increase
> accordingly. Under
> the overdrive mode for a long time, the chipset may shorten the
> lifetime, especially in high temperature condition
> """
> 
> according to the same datasheet, end of the same table, page 31.
> 
> so this seems like a turbo-mode frequency to me.
> 
> Additionally, the note for the "normal mode" (the one with the lower
> freqs) states:
> 
> """
> Normal mode means the chipset works under safety voltage and frequency. 
> For the
> industrial environment, highly recommend to keep in normal mode, the 
> lifetime is
> reasonably guaranteed.
> """
> 
> I believe "industrial environment" means RK3588J used in the
> temperatures that aren't OK for the standard RK3588 variant?

Thanks a lot for obtaining these clarifications!

Yes, I'd say that in this case "industrial environment" boils down
to the extended temperature range for the RK3588J variant.

>> I haven't seen a TRM for the J variant, nor do I have a board with
>> RK3588J to run tests, so it would be great if Kever could chime in
>> with how these OPPs are different from the others (or not).
>> 
>>> So while the situation might be strange for the rk3588j, I really 
>>> only want
>>> correct frequencies here please - not any pseudo "turbo freqs".
>>> 
>>> I don't care that much what people do on their own device{s/trees}, 
>>> but
>>> the devicetrees we supply centrally (and to u-boot, etc) should only
>>> contain frequencies vetted by the manufacturer.
>> 
>> Fair enough. Let's just try and get another data point on whether
>> these frequencies are vetted or not.
> 
> So the higher freqs seems to be vetted (and used by default on
> Rockchip's BSP kernel), it just feels like you aren't really supposed
> to run those higher frequencies all the time? And especially not in
> "industrial environment"?
> 
> I would assume we want to stay on the safer side and remove those
> higher frequencies? Heiko?

I agree, we should remove the higher-frequency OPPs.  I'd like
to go through everything once again in detail and prepare a patch
that removes the unsafe OPPs, and you could review it once it's
on the ML, to make sure it's fine.

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Dragan Simic]

Hello all,

On 2025-03-12 11:34, Dragan Simic wrote:
> On 2025-03-12 11:15, Quentin Schulz wrote:
>> On 2/16/25 1:32 PM, Alexey Charkov wrote:
>>> On Sat, Feb 15, 2025 at 11:30 PM Heiko Stübner <heiko@sntech.de> 
>>> wrote:
>>>> Am Samstag, 15. Februar 2025, 19:59:44 MEZ schrieb Alexey Charkov:
>>>>> On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz 
>>>>> <quentin.schulz@cherry.de> wrote:
>>>>>> On 6/17/24 8:28 PM, Alexey Charkov wrote:
>> [...]
>>>>>>> +     };
>>>>>>> +
>>>>>>> +     cluster2_opp_table: opp-table-cluster2 {
>>>>>>> +             compatible = "operating-points-v2";
>>>>>>> +             opp-shared;
>>>>>>> +
>>>>>>> +             opp-1416000000 {
>>>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
>>>>>>> +                     opp-microvolt = <750000 750000 950000>;
>>>>>>> +                     clock-latency-ns = <40000>;
>>>>>>> +             };
>>>>>>> +             opp-1608000000 {
>>>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
>>>>>>> +                     opp-microvolt = <787500 787500 950000>;
>>>>>>> +                     clock-latency-ns = <40000>;
>>>>>>> +             };
>>>>>>> +             opp-1800000000 {
>>>>>>> +                     opp-hz = /bits/ 64 <1800000000>;
>>>>>>> +                     opp-microvolt = <875000 875000 950000>;
>>>>>>> +                     clock-latency-ns = <40000>;
>>>>>>> +             };
>>>>>>> +             opp-2016000000 {
>>>>>>> +                     opp-hz = /bits/ 64 <2016000000>;
>>>>>>> +                     opp-microvolt = <950000 950000 950000>;
>>>>>>> +                     clock-latency-ns = <40000>;
>>>>>>> +             };
>>>>>> 
>>>>>> opp-1800000000 and opp-2016000000 should be removed as well.
>>>>>> 
>>>>>> Did I misunderstand what Rockchip did here? Adding Kever in Cc at 
>>>>>> least
>>>>>> for awareness on Rockchip side :)
>>>>>> 
>>>>>> I guess another option could be to mark those above as
>>>>>> 
>>>>>> turbo-mode;
>>>>>> 
>>>>>> though no clue what this would entail. From a glance at cpufreq, 
>>>>>> it
>>>>>> seems that for Rockchip since we use the default cpufreq-dt, it 
>>>>>> would
>>>>>> mark those as unusable, so essentially a no-op, so better remove 
>>>>>> them
>>>>>> entirely?
>>>>> 
>>>>> I believe the opp core just marks 'turbo-mode' opps as
>>>>> CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to 
>>>>> the
>>>>> cpufreq core. But then to actually use those boost frequencies I 
>>>>> would
>>>>> guess the governor needs to somehow know the power/thermal 
>>>>> constraints
>>>>> of the chip?.. Don't know where that is defined.
>>>> 
>>>> personally I don't believe in "boost" frequencies - except when they 
>>>> are
>>>> declared officially.
>>>> 
>>>> Rockchip for the longest time maintains that running the chip 
>>>> outside
>>>> the declared power/rate envelope can reduce its overall lifetime.
>>>> 
>>>> So for Rockchip in mainline a "it runs stable for me" has never been 
>>>> a
>>>> suitable reasoning ;-) .
>>> 
>>> My key concern here was perhaps that we are looking at a file inside
>>> the Rockchip source tree which looks relevant by the name of it, but
>>> doesn't actually get included anywhere for any of the boards. This 
>>> may
>>> or may not constitute an endorsement by Rockchip of any OPPs listed
>>> there :-D
>> 
>> Rockchip support stated:
>> 
>> """
>> If you run overdrive mode, you do not need to include rk3588j.dtsi,
>> and you can change it to #incldue rk3588.dtsi to ensure that the
>> maximum frequency can reach 2GHz
>> 
>> below picture from datasheet.
>> """
>> 
>> The picture is the table 3-2 Recommended operating conditions, page 30
>> from the RK3588J datasheet, e.g.
>> https://www.lcsc.com/datasheet/lcsc_datasheet_2403201054_Rockchip-RK3588J_C22364189.pdf
>> 
>> What is overdrive?
>> 
>> """
>> Overdrive mode brings higher frequency, and the voltage will increase
>> accordingly. Under
>> the overdrive mode for a long time, the chipset may shorten the
>> lifetime, especially in high temperature condition
>> """
>> 
>> according to the same datasheet, end of the same table, page 31.
>> 
>> so this seems like a turbo-mode frequency to me.
>> 
>> Additionally, the note for the "normal mode" (the one with the lower
>> freqs) states:
>> 
>> """
>> Normal mode means the chipset works under safety voltage and 
>> frequency. For the
>> industrial environment, highly recommend to keep in normal mode, the 
>> lifetime is
>> reasonably guaranteed.
>> """
>> 
>> I believe "industrial environment" means RK3588J used in the
>> temperatures that aren't OK for the standard RK3588 variant?
> 
> Thanks a lot for obtaining these clarifications!
> 
> Yes, I'd say that in this case "industrial environment" boils down
> to the extended temperature range for the RK3588J variant.
> 
>>> I haven't seen a TRM for the J variant, nor do I have a board with
>>> RK3588J to run tests, so it would be great if Kever could chime in
>>> with how these OPPs are different from the others (or not).
>>> 
>>>> So while the situation might be strange for the rk3588j, I really 
>>>> only want
>>>> correct frequencies here please - not any pseudo "turbo freqs".
>>>> 
>>>> I don't care that much what people do on their own device{s/trees}, 
>>>> but
>>>> the devicetrees we supply centrally (and to u-boot, etc) should only
>>>> contain frequencies vetted by the manufacturer.
>>> 
>>> Fair enough. Let's just try and get another data point on whether
>>> these frequencies are vetted or not.
>> 
>> So the higher freqs seems to be vetted (and used by default on
>> Rockchip's BSP kernel), it just feels like you aren't really supposed
>> to run those higher frequencies all the time? And especially not in
>> "industrial environment"?
>> 
>> I would assume we want to stay on the safer side and remove those
>> higher frequencies? Heiko?
> 
> I agree, we should remove the higher-frequency OPPs.  I'd like
> to go through everything once again in detail and prepare a patch
> that removes the unsafe OPPs, and you could review it once it's
> on the ML, to make sure it's fine.

Just as a note, everything above (and even a bit more) is confirmed
and clearly described in the publicly available RK3588J datasheet,
which I'll provide as a reference in my upcoming patch.

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Heiko Stuebner]

Am Donnerstag, 13. März 2025, 11:42:17 MEZ schrieb Dragan Simic:
> Hello all,
> 
> On 2025-03-12 11:34, Dragan Simic wrote:
> > On 2025-03-12 11:15, Quentin Schulz wrote:
> >> On 2/16/25 1:32 PM, Alexey Charkov wrote:
> >>> On Sat, Feb 15, 2025 at 11:30 PM Heiko Stübner <heiko@sntech.de> 
> >>> wrote:
> >>>> Am Samstag, 15. Februar 2025, 19:59:44 MEZ schrieb Alexey Charkov:
> >>>>> On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz 
> >>>>> <quentin.schulz@cherry.de> wrote:
> >>>>>> On 6/17/24 8:28 PM, Alexey Charkov wrote:
> >> [...]
> >>>>>>> +     };
> >>>>>>> +
> >>>>>>> +     cluster2_opp_table: opp-table-cluster2 {
> >>>>>>> +             compatible = "operating-points-v2";
> >>>>>>> +             opp-shared;
> >>>>>>> +
> >>>>>>> +             opp-1416000000 {
> >>>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
> >>>>>>> +                     opp-microvolt = <750000 750000 950000>;
> >>>>>>> +                     clock-latency-ns = <40000>;
> >>>>>>> +             };
> >>>>>>> +             opp-1608000000 {
> >>>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
> >>>>>>> +                     opp-microvolt = <787500 787500 950000>;
> >>>>>>> +                     clock-latency-ns = <40000>;
> >>>>>>> +             };
> >>>>>>> +             opp-1800000000 {
> >>>>>>> +                     opp-hz = /bits/ 64 <1800000000>;
> >>>>>>> +                     opp-microvolt = <875000 875000 950000>;
> >>>>>>> +                     clock-latency-ns = <40000>;
> >>>>>>> +             };
> >>>>>>> +             opp-2016000000 {
> >>>>>>> +                     opp-hz = /bits/ 64 <2016000000>;
> >>>>>>> +                     opp-microvolt = <950000 950000 950000>;
> >>>>>>> +                     clock-latency-ns = <40000>;
> >>>>>>> +             };
> >>>>>> 
> >>>>>> opp-1800000000 and opp-2016000000 should be removed as well.
> >>>>>> 
> >>>>>> Did I misunderstand what Rockchip did here? Adding Kever in Cc at 
> >>>>>> least
> >>>>>> for awareness on Rockchip side :)
> >>>>>> 
> >>>>>> I guess another option could be to mark those above as
> >>>>>> 
> >>>>>> turbo-mode;
> >>>>>> 
> >>>>>> though no clue what this would entail. From a glance at cpufreq, 
> >>>>>> it
> >>>>>> seems that for Rockchip since we use the default cpufreq-dt, it 
> >>>>>> would
> >>>>>> mark those as unusable, so essentially a no-op, so better remove 
> >>>>>> them
> >>>>>> entirely?
> >>>>> 
> >>>>> I believe the opp core just marks 'turbo-mode' opps as
> >>>>> CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to 
> >>>>> the
> >>>>> cpufreq core. But then to actually use those boost frequencies I 
> >>>>> would
> >>>>> guess the governor needs to somehow know the power/thermal 
> >>>>> constraints
> >>>>> of the chip?.. Don't know where that is defined.
> >>>> 
> >>>> personally I don't believe in "boost" frequencies - except when they 
> >>>> are
> >>>> declared officially.
> >>>> 
> >>>> Rockchip for the longest time maintains that running the chip 
> >>>> outside
> >>>> the declared power/rate envelope can reduce its overall lifetime.
> >>>> 
> >>>> So for Rockchip in mainline a "it runs stable for me" has never been 
> >>>> a
> >>>> suitable reasoning ;-) .
> >>> 
> >>> My key concern here was perhaps that we are looking at a file inside
> >>> the Rockchip source tree which looks relevant by the name of it, but
> >>> doesn't actually get included anywhere for any of the boards. This 
> >>> may
> >>> or may not constitute an endorsement by Rockchip of any OPPs listed
> >>> there :-D
> >> 
> >> Rockchip support stated:
> >> 
> >> """
> >> If you run overdrive mode, you do not need to include rk3588j.dtsi,
> >> and you can change it to #incldue rk3588.dtsi to ensure that the
> >> maximum frequency can reach 2GHz
> >> 
> >> below picture from datasheet.
> >> """
> >> 
> >> The picture is the table 3-2 Recommended operating conditions, page 30
> >> from the RK3588J datasheet, e.g.
> >> https://www.lcsc.com/datasheet/lcsc_datasheet_2403201054_Rockchip-RK3588J_C22364189.pdf
> >> 
> >> What is overdrive?
> >> 
> >> """
> >> Overdrive mode brings higher frequency, and the voltage will increase
> >> accordingly. Under
> >> the overdrive mode for a long time, the chipset may shorten the
> >> lifetime, especially in high temperature condition
> >> """
> >> 
> >> according to the same datasheet, end of the same table, page 31.
> >> 
> >> so this seems like a turbo-mode frequency to me.
> >> 
> >> Additionally, the note for the "normal mode" (the one with the lower
> >> freqs) states:
> >> 
> >> """
> >> Normal mode means the chipset works under safety voltage and 
> >> frequency. For the
> >> industrial environment, highly recommend to keep in normal mode, the 
> >> lifetime is
> >> reasonably guaranteed.
> >> """
> >> 
> >> I believe "industrial environment" means RK3588J used in the
> >> temperatures that aren't OK for the standard RK3588 variant?
> > 
> > Thanks a lot for obtaining these clarifications!
> > 
> > Yes, I'd say that in this case "industrial environment" boils down
> > to the extended temperature range for the RK3588J variant.
> > 
> >>> I haven't seen a TRM for the J variant, nor do I have a board with
> >>> RK3588J to run tests, so it would be great if Kever could chime in
> >>> with how these OPPs are different from the others (or not).
> >>> 
> >>>> So while the situation might be strange for the rk3588j, I really 
> >>>> only want
> >>>> correct frequencies here please - not any pseudo "turbo freqs".
> >>>> 
> >>>> I don't care that much what people do on their own device{s/trees}, 
> >>>> but
> >>>> the devicetrees we supply centrally (and to u-boot, etc) should only
> >>>> contain frequencies vetted by the manufacturer.
> >>> 
> >>> Fair enough. Let's just try and get another data point on whether
> >>> these frequencies are vetted or not.
> >> 
> >> So the higher freqs seems to be vetted (and used by default on
> >> Rockchip's BSP kernel), it just feels like you aren't really supposed
> >> to run those higher frequencies all the time? And especially not in
> >> "industrial environment"?
> >> 
> >> I would assume we want to stay on the safer side and remove those
> >> higher frequencies? Heiko?
> > 
> > I agree, we should remove the higher-frequency OPPs.  I'd like
> > to go through everything once again in detail and prepare a patch
> > that removes the unsafe OPPs, and you could review it once it's
> > on the ML, to make sure it's fine.
> 
> Just as a note, everything above (and even a bit more) is confirmed
> and clearly described in the publicly available RK3588J datasheet,
> which I'll provide as a reference in my upcoming patch.

so just to reiterate my stance, in mainline I really only want frequencies
that are not possibly influencing the lifetime of the chip.

It doesn't even matter about the variant we're talking about being
industrial :-) . When someone is using mainline I want them to be
reasonable assured that we don't have stuff in here that may affect
the lifetime of their board.

All gambling on performance for possible lifetime reduction people
can do on their own ... for example with a dt-overlay ;-) .

So TL;DR, I agree to both Quentin and Dragan


Heiko

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Dragan Simic]

Hello Heiko,

On 2025-03-13 20:00, Heiko Stuebner wrote:
> Am Donnerstag, 13. März 2025, 11:42:17 MEZ schrieb Dragan Simic:
>> On 2025-03-12 11:34, Dragan Simic wrote:
>> > On 2025-03-12 11:15, Quentin Schulz wrote:
>> >> On 2/16/25 1:32 PM, Alexey Charkov wrote:
>> >>> On Sat, Feb 15, 2025 at 11:30 PM Heiko Stübner <heiko@sntech.de>
>> >>> wrote:
>> >>>> Am Samstag, 15. Februar 2025, 19:59:44 MEZ schrieb Alexey Charkov:
>> >>>>> On Tue, Feb 11, 2025 at 7:32 PM Quentin Schulz
>> >>>>> <quentin.schulz@cherry.de> wrote:
>> >>>>>> On 6/17/24 8:28 PM, Alexey Charkov wrote:
>> >> [...]
>> >>>>>>> +     };
>> >>>>>>> +
>> >>>>>>> +     cluster2_opp_table: opp-table-cluster2 {
>> >>>>>>> +             compatible = "operating-points-v2";
>> >>>>>>> +             opp-shared;
>> >>>>>>> +
>> >>>>>>> +             opp-1416000000 {
>> >>>>>>> +                     opp-hz = /bits/ 64 <1416000000>;
>> >>>>>>> +                     opp-microvolt = <750000 750000 950000>;
>> >>>>>>> +                     clock-latency-ns = <40000>;
>> >>>>>>> +             };
>> >>>>>>> +             opp-1608000000 {
>> >>>>>>> +                     opp-hz = /bits/ 64 <1608000000>;
>> >>>>>>> +                     opp-microvolt = <787500 787500 950000>;
>> >>>>>>> +                     clock-latency-ns = <40000>;
>> >>>>>>> +             };
>> >>>>>>> +             opp-1800000000 {
>> >>>>>>> +                     opp-hz = /bits/ 64 <1800000000>;
>> >>>>>>> +                     opp-microvolt = <875000 875000 950000>;
>> >>>>>>> +                     clock-latency-ns = <40000>;
>> >>>>>>> +             };
>> >>>>>>> +             opp-2016000000 {
>> >>>>>>> +                     opp-hz = /bits/ 64 <2016000000>;
>> >>>>>>> +                     opp-microvolt = <950000 950000 950000>;
>> >>>>>>> +                     clock-latency-ns = <40000>;
>> >>>>>>> +             };
>> >>>>>>
>> >>>>>> opp-1800000000 and opp-2016000000 should be removed as well.
>> >>>>>>
>> >>>>>> Did I misunderstand what Rockchip did here? Adding Kever in Cc at
>> >>>>>> least
>> >>>>>> for awareness on Rockchip side :)
>> >>>>>>
>> >>>>>> I guess another option could be to mark those above as
>> >>>>>>
>> >>>>>> turbo-mode;
>> >>>>>>
>> >>>>>> though no clue what this would entail. From a glance at cpufreq,
>> >>>>>> it
>> >>>>>> seems that for Rockchip since we use the default cpufreq-dt, it
>> >>>>>> would
>> >>>>>> mark those as unusable, so essentially a no-op, so better remove
>> >>>>>> them
>> >>>>>> entirely?
>> >>>>>
>> >>>>> I believe the opp core just marks 'turbo-mode' opps as
>> >>>>> CPUFREQ_BOOST_FREQ, and cpufreq-dt only passes that flag along to
>> >>>>> the
>> >>>>> cpufreq core. But then to actually use those boost frequencies I
>> >>>>> would
>> >>>>> guess the governor needs to somehow know the power/thermal
>> >>>>> constraints
>> >>>>> of the chip?.. Don't know where that is defined.
>> >>>>
>> >>>> personally I don't believe in "boost" frequencies - except when they
>> >>>> are
>> >>>> declared officially.
>> >>>>
>> >>>> Rockchip for the longest time maintains that running the chip
>> >>>> outside
>> >>>> the declared power/rate envelope can reduce its overall lifetime.
>> >>>>
>> >>>> So for Rockchip in mainline a "it runs stable for me" has never been
>> >>>> a
>> >>>> suitable reasoning ;-) .
>> >>>
>> >>> My key concern here was perhaps that we are looking at a file inside
>> >>> the Rockchip source tree which looks relevant by the name of it, but
>> >>> doesn't actually get included anywhere for any of the boards. This
>> >>> may
>> >>> or may not constitute an endorsement by Rockchip of any OPPs listed
>> >>> there :-D
>> >>
>> >> Rockchip support stated:
>> >>
>> >> """
>> >> If you run overdrive mode, you do not need to include rk3588j.dtsi,
>> >> and you can change it to #incldue rk3588.dtsi to ensure that the
>> >> maximum frequency can reach 2GHz
>> >>
>> >> below picture from datasheet.
>> >> """
>> >>
>> >> The picture is the table 3-2 Recommended operating conditions, page 30
>> >> from the RK3588J datasheet, e.g.
>> >> https://www.lcsc.com/datasheet/lcsc_datasheet_2403201054_Rockchip-RK3588J_C22364189.pdf
>> >>
>> >> What is overdrive?
>> >>
>> >> """
>> >> Overdrive mode brings higher frequency, and the voltage will increase
>> >> accordingly. Under
>> >> the overdrive mode for a long time, the chipset may shorten the
>> >> lifetime, especially in high temperature condition
>> >> """
>> >>
>> >> according to the same datasheet, end of the same table, page 31.
>> >>
>> >> so this seems like a turbo-mode frequency to me.
>> >>
>> >> Additionally, the note for the "normal mode" (the one with the lower
>> >> freqs) states:
>> >>
>> >> """
>> >> Normal mode means the chipset works under safety voltage and
>> >> frequency. For the
>> >> industrial environment, highly recommend to keep in normal mode, the
>> >> lifetime is
>> >> reasonably guaranteed.
>> >> """
>> >>
>> >> I believe "industrial environment" means RK3588J used in the
>> >> temperatures that aren't OK for the standard RK3588 variant?
>> >
>> > Thanks a lot for obtaining these clarifications!
>> >
>> > Yes, I'd say that in this case "industrial environment" boils down
>> > to the extended temperature range for the RK3588J variant.
>> >
>> >>> I haven't seen a TRM for the J variant, nor do I have a board with
>> >>> RK3588J to run tests, so it would be great if Kever could chime in
>> >>> with how these OPPs are different from the others (or not).
>> >>>
>> >>>> So while the situation might be strange for the rk3588j, I really
>> >>>> only want
>> >>>> correct frequencies here please - not any pseudo "turbo freqs".
>> >>>>
>> >>>> I don't care that much what people do on their own device{s/trees},
>> >>>> but
>> >>>> the devicetrees we supply centrally (and to u-boot, etc) should only
>> >>>> contain frequencies vetted by the manufacturer.
>> >>>
>> >>> Fair enough. Let's just try and get another data point on whether
>> >>> these frequencies are vetted or not.
>> >>
>> >> So the higher freqs seems to be vetted (and used by default on
>> >> Rockchip's BSP kernel), it just feels like you aren't really supposed
>> >> to run those higher frequencies all the time? And especially not in
>> >> "industrial environment"?
>> >>
>> >> I would assume we want to stay on the safer side and remove those
>> >> higher frequencies? Heiko?
>> >
>> > I agree, we should remove the higher-frequency OPPs.  I'd like
>> > to go through everything once again in detail and prepare a patch
>> > that removes the unsafe OPPs, and you could review it once it's
>> > on the ML, to make sure it's fine.
>> 
>> Just as a note, everything above (and even a bit more) is confirmed
>> and clearly described in the publicly available RK3588J datasheet,
>> which I'll provide as a reference in my upcoming patch.
> 
> so just to reiterate my stance, in mainline I really only want 
> frequencies
> that are not possibly influencing the lifetime of the chip.
> 
> It doesn't even matter about the variant we're talking about being
> industrial :-) . When someone is using mainline I want them to be
> reasonable assured that we don't have stuff in here that may affect
> the lifetime of their board.
> 
> All gambling on performance for possible lifetime reduction people
> can do on their own ... for example with a dt-overlay ;-) .
> 
> So TL;DR, I agree to both Quentin and Dragan

Thanks!  Indeed, we must provide only the OPPs that are declared
by the manufacturer to be always safe for the particular SoC
variant.  The RK3588J is actually a good example, because it, in
theory, can run safely at higher OPPs as well, but only when not
enjoying the extended temperature range that the RK3588J, as an
SoC variant targeted at industrial applications, is specifically
made (or binned) for.

Thus, we must support only the RK3588J OPPs that are declared to
be safe throughout the entire extended temperature range, while
anyone who actually can assure that their RK3588J-based board is
never going to run within the extended temperature range, probably
may safely apply an overlay that adds the higher OPPs.  As we
obviously can't know what will be the temperature conditions, we
may provide only the lower OPPs that are always safe.

I'll finish the patch and send it over tomorrow or so...  I still
need to go through the changes once again, to make 100% sure I've
missed nothing, and that I haven't included anything extraneous. :)

Re: [PATCH v5 7/8] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588j

[Dragan Simic]

Hello all,

On 2025-03-13 20:43, Dragan Simic wrote:
> On 2025-03-13 20:00, Heiko Stuebner wrote:
>> Am Donnerstag, 13. März 2025, 11:42:17 MEZ schrieb Dragan Simic:
>>> On 2025-03-12 11:34, Dragan Simic wrote:
>>> Just as a note, everything above (and even a bit more) is confirmed
>>> and clearly described in the publicly available RK3588J datasheet,
>>> which I'll provide as a reference in my upcoming patch.
>> 
>> so just to reiterate my stance, in mainline I really only want 
>> frequencies
>> that are not possibly influencing the lifetime of the chip.
>> 
>> It doesn't even matter about the variant we're talking about being
>> industrial :-) . When someone is using mainline I want them to be
>> reasonable assured that we don't have stuff in here that may affect
>> the lifetime of their board.
>> 
>> All gambling on performance for possible lifetime reduction people
>> can do on their own ... for example with a dt-overlay ;-) .
>> 
>> So TL;DR, I agree to both Quentin and Dragan
> 
> Thanks!  Indeed, we must provide only the OPPs that are declared
> by the manufacturer to be always safe for the particular SoC
> variant.  The RK3588J is actually a good example, because it, in
> theory, can run safely at higher OPPs as well, but only when not
> enjoying the extended temperature range that the RK3588J, as an
> SoC variant targeted at industrial applications, is specifically
> made (or binned) for.
> 
> Thus, we must support only the RK3588J OPPs that are declared to
> be safe throughout the entire extended temperature range, while
> anyone who actually can assure that their RK3588J-based board is
> never going to run within the extended temperature range, probably
> may safely apply an overlay that adds the higher OPPs.  As we
> obviously can't know what will be the temperature conditions, we
> may provide only the lower OPPs that are always safe.
> 
> I'll finish the patch and send it over tomorrow or so...  I still
> need to go through the changes once again, to make 100% sure I've
> missed nothing, and that I haven't included anything extraneous. :)

For future reference and for anyone interested, below is the link
to the above-mentioned patch.

https://lore.kernel.org/linux-rockchip/f929da061de35925ea591c969f985430e23c4a7e.1742526811.git.dsimic@manjaro.org/T/#u

Re: [PATCH v5 8/8] arm64: dts: rockchip: Split GPU OPPs of RK3588 and RK3588j

[Quentin Schulz]

Hi all,

On 6/17/24 8:28 PM, Alexey Charkov wrote:
> RK3588j uses a different set of OPPs for its GPU, both in terms of
> allowed frequencies and in terms of voltages.
> 
> Move the GPU OPPs table into per-variant .dtsi files to accommodate
> for this difference.
> 
> The table for RK3588j is adapted from Rockchip downstream sources [1],
> while RK3588 one is moved verbatim into the per-variant .dtsi file.
> The values provided for RK3588 in the downstream sources match those
> in the original commit.
> 
> [1] https://github.com/rockchip-linux/kernel/blob/604cec4004abe5a96c734f2fab7b74809d2d742f/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> 
> Fixes: 6fca4edb93d3 ("arm64: dts: rockchip: Add rk3588 GPU node")
> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
>   arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 38 -------------------------
>   arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi  | 41 +++++++++++++++++++++++++++
>   arch/arm64/boot/dts/rockchip/rk3588j.dtsi     | 33 +++++++++++++++++++++
>   3 files changed, 74 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> index 758aff5e040b..3d918874aa02 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> @@ -451,46 +451,8 @@ gpu: gpu@fb000000 {
>   			     <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH 0>,
>   			     <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH 0>;
>   		interrupt-names = "job", "mmu", "gpu";
> -		operating-points-v2 = <&gpu_opp_table>;
>   		power-domains = <&power RK3588_PD_GPU>;
>   		status = "disabled";
> -
> -		gpu_opp_table: opp-table {
> -			compatible = "operating-points-v2";
> -
> -			opp-300000000 {
> -				opp-hz = /bits/ 64 <300000000>;
> -				opp-microvolt = <675000 675000 850000>;
> -			};
> -			opp-400000000 {
> -				opp-hz = /bits/ 64 <400000000>;
> -				opp-microvolt = <675000 675000 850000>;
> -			};
> -			opp-500000000 {
> -				opp-hz = /bits/ 64 <500000000>;
> -				opp-microvolt = <675000 675000 850000>;
> -			};
> -			opp-600000000 {
> -				opp-hz = /bits/ 64 <600000000>;
> -				opp-microvolt = <675000 675000 850000>;
> -			};
> -			opp-700000000 {
> -				opp-hz = /bits/ 64 <700000000>;
> -				opp-microvolt = <700000 700000 850000>;
> -			};
> -			opp-800000000 {
> -				opp-hz = /bits/ 64 <800000000>;
> -				opp-microvolt = <750000 750000 850000>;
> -			};
> -			opp-900000000 {
> -				opp-hz = /bits/ 64 <900000000>;
> -				opp-microvolt = <800000 800000 850000>;
> -			};
> -			opp-1000000000 {
> -				opp-hz = /bits/ 64 <1000000000>;
> -				opp-microvolt = <850000 850000 850000>;
> -			};
> -		};
>   	};
>   
>   	usb_host0_xhci: usb@fc000000 {
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
> index 35bbc3c2134f..0f1a77697351 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi
> @@ -114,6 +114,43 @@ opp-2400000000 {
>   			clock-latency-ns = <40000>;
>   		};
>   	};
> +
> +	gpu_opp_table: opp-table {
> +		compatible = "operating-points-v2";
> +
> +		opp-300000000 {
> +			opp-hz = /bits/ 64 <300000000>;
> +			opp-microvolt = <675000 675000 850000>;
> +		};
> +		opp-400000000 {
> +			opp-hz = /bits/ 64 <400000000>;
> +			opp-microvolt = <675000 675000 850000>;
> +		};
> +		opp-500000000 {
> +			opp-hz = /bits/ 64 <500000000>;
> +			opp-microvolt = <675000 675000 850000>;
> +		};
> +		opp-600000000 {
> +			opp-hz = /bits/ 64 <600000000>;
> +			opp-microvolt = <675000 675000 850000>;
> +		};
> +		opp-700000000 {
> +			opp-hz = /bits/ 64 <700000000>;
> +			opp-microvolt = <700000 700000 850000>;
> +		};
> +		opp-800000000 {
> +			opp-hz = /bits/ 64 <800000000>;
> +			opp-microvolt = <750000 750000 850000>;
> +		};
> +		opp-900000000 {
> +			opp-hz = /bits/ 64 <900000000>;
> +			opp-microvolt = <800000 800000 850000>;
> +		};
> +		opp-1000000000 {
> +			opp-hz = /bits/ 64 <1000000000>;
> +			opp-microvolt = <850000 850000 850000>;
> +		};
> +	};
>   };
>   
>   &cpu_b0 {
> @@ -147,3 +184,7 @@ &cpu_l2 {
>   &cpu_l3 {
>   	operating-points-v2 = <&cluster0_opp_table>;
>   };
> +
> +&gpu {
> +	operating-points-v2 = <&gpu_opp_table>;
> +};
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> index b7e69553857b..bce72bac4503 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588j.dtsi
> @@ -80,6 +80,35 @@ opp-2016000000 {
>   			clock-latency-ns = <40000>;
>   		};
>   	};
> +
> +	gpu_opp_table: opp-table {
> +		compatible = "operating-points-v2";
> +
> +		opp-300000000 {
> +			opp-hz = /bits/ 64 <300000000>;
> +			opp-microvolt = <750000 750000 850000>;
> +		};
> +		opp-400000000 {
> +			opp-hz = /bits/ 64 <400000000>;
> +			opp-microvolt = <750000 750000 850000>;
> +		};
> +		opp-500000000 {
> +			opp-hz = /bits/ 64 <500000000>;
> +			opp-microvolt = <750000 750000 850000>;
> +		};
> +		opp-600000000 {
> +			opp-hz = /bits/ 64 <600000000>;
> +			opp-microvolt = <750000 750000 850000>;
> +		};
> +		opp-700000000 {
> +			opp-hz = /bits/ 64 <700000000>;
> +			opp-microvolt = <750000 750000 850000>;
> +		};
> +		opp-850000000 {
> +			opp-hz = /bits/ 64 <800000000>;
> +			opp-microvolt = <787500 787500 850000>;
> +		};

Same remark as for the CPU OPPs, here Rockchip removes opp-850000000 for 
the GPU OPPs on RK3588J.

Cheers,
Quentin