All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Heiko Stübner" <heiko@sntech.de>
To: linux-rockchip@lists.infradead.org,
	Dragan Simic <dsimic@manjaro.org>,
	Diederik de Haas <didi.debian@cknow.org>
Cc: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
	linux-kernel@vger.kernel.org, Jonas Karlman <jonas@kwiboo.se>,
	Diederik de Haas <didi.debian@cknow.org>
Subject: Re: [PATCH v2] arm64: dts: rockchip: Add GPU OPP voltage ranges to RK356x SoC dtsi
Date: Sun, 30 Jun 2024 11:07:47 +0200	[thread overview]
Message-ID: <1894199.CQOukoFCf9@diego> (raw)
In-Reply-To: <2442162.AJoTavkB1d@bagend>

Am Sonntag, 30. Juni 2024, 00:01:41 CEST schrieb Diederik de Haas:
> On Saturday, 29 June 2024 18:39:02 CEST Dragan Simic wrote:
> > Add support for voltage ranges to the GPU OPPs defined in the SoC dtsi for
> > RK356x.  These voltage ranges are useful for RK356x-based boards that are
> > designed to use the same power supply for the GPU and NPU portions of the
> > SoC, which is described further in the following documents:
> > 
> >   - Rockchip RK3566 Hardware Design Guide, version 1.1.0, page 37
> >   - Rockchip RK3568 Hardware Design Guide, version 1.2, page 78
> 
> That was interesting to read, thanks.
> Now I understand the difference between rk809(-5) and rk817(-5).
> 
> But AFAIUI the above description described why there were separate tables for 
> rk809 and rk817 in v1. But that was dropped in v2. So it seems to me the 
> (commit) message should be updated accordingly?
> 
> I also expected that (for v1) there would be a similar construct as was 
> recently added for rk3588. But I should interpret Heiko's comments as that 
> strategy should not be applied to rk356x?

The issue I had was more about the #ifdef'ery and then having a board define
a constant to enable one or the other.

As far as I understood the description, the OPP itself is the same in
terms of frequency and voltage, just the regulator can't fully realize
that target voltage, so the solution is to allow a voltage range, to
also support the less-exact regulator.

On the rk3588 on the other hand the soc variants have different OPP
tables themselfs, because the soc itself only supports different
frequencies+voltages. So the solution here is the split of the OPPs so
that we don't mess around with /delete-node/ edits of one OPP table.

So TL;DR separate OPP tables are the way to go if the user needs different
freq+voltage values and voltage ranges allows boards to use less-adapted
regulators.


> > The values for the exact GPU OPP voltages and the lower limits for the GPU
> > OPP voltage ranges differ from the values found in the vendor kernel source
> > (cf. downstream commit f8b9431ee38e ("arm64: dts: rockchip: rk3568: support
> > adjust opp-table by otp")). [1][2]  
> 
> Why? In their latest update Rockchip changed it to the values as specified in 
> the links. My assumption is that based on extensive testing they did and/or 
> the feedback they got from the client/customers, they felt the need to change 
> it to the values they did.
> 
> I think we should follow their values unless we have an explicit and very good 
> reason to deviate from that.

Correct.
Values from some "random" Radxa kernel would also not be my
selection of choice.

In the mainline-kernel we always want the save choice - which in for me
is Rockchip's. If people want to experiment with other values on their own
boards to sort of overclock their chips, that's their prerogative.


Heiko


> > However, our values have served us well so far, so let's keep them for now,
> 
> And I don't think that qualifies as a (very) good reason.
> I think it's reasonable to assume that far more (stress) testing has been done 
> with the downstream code, then has happened with the upstream code.
> Hopefully that'll change in the future, but I don't think we're there yet.
> 
> When we/upstream adds npu support, I think we should also follow downstream's 
> OPP values, unless we have a very good reason to deviate from that.
> 
> > until we actually start supporting the CPU and GPU binning, together with
> > the related voltage adjustments.
> 
> I may not fully understand what you mean by that, but I think it's (again) 
> reasonable to assume that Rockchip has far more insight into this then we do.
> 
> Cheers,
>   Diederik
> 
> > [1]
> > https://github.com/rockchip-linux/kernel/commit/f8b9431ee38ed561650be7092ab
> > 93f564598daa9 [2]
> > https://raw.githubusercontent.com/rockchip-linux/kernel/f8b9431ee38ed561650
> > be7092ab93f564598daa9/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> 





_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

WARNING: multiple messages have this Message-ID (diff)
From: "Heiko Stübner" <heiko@sntech.de>
To: linux-rockchip@lists.infradead.org,
	Dragan Simic <dsimic@manjaro.org>,
	Diederik de Haas <didi.debian@cknow.org>
Cc: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
	linux-kernel@vger.kernel.org, Jonas Karlman <jonas@kwiboo.se>,
	Diederik de Haas <didi.debian@cknow.org>
Subject: Re: [PATCH v2] arm64: dts: rockchip: Add GPU OPP voltage ranges to RK356x SoC dtsi
Date: Sun, 30 Jun 2024 11:07:47 +0200	[thread overview]
Message-ID: <1894199.CQOukoFCf9@diego> (raw)
In-Reply-To: <2442162.AJoTavkB1d@bagend>

Am Sonntag, 30. Juni 2024, 00:01:41 CEST schrieb Diederik de Haas:
> On Saturday, 29 June 2024 18:39:02 CEST Dragan Simic wrote:
> > Add support for voltage ranges to the GPU OPPs defined in the SoC dtsi for
> > RK356x.  These voltage ranges are useful for RK356x-based boards that are
> > designed to use the same power supply for the GPU and NPU portions of the
> > SoC, which is described further in the following documents:
> > 
> >   - Rockchip RK3566 Hardware Design Guide, version 1.1.0, page 37
> >   - Rockchip RK3568 Hardware Design Guide, version 1.2, page 78
> 
> That was interesting to read, thanks.
> Now I understand the difference between rk809(-5) and rk817(-5).
> 
> But AFAIUI the above description described why there were separate tables for 
> rk809 and rk817 in v1. But that was dropped in v2. So it seems to me the 
> (commit) message should be updated accordingly?
> 
> I also expected that (for v1) there would be a similar construct as was 
> recently added for rk3588. But I should interpret Heiko's comments as that 
> strategy should not be applied to rk356x?

The issue I had was more about the #ifdef'ery and then having a board define
a constant to enable one or the other.

As far as I understood the description, the OPP itself is the same in
terms of frequency and voltage, just the regulator can't fully realize
that target voltage, so the solution is to allow a voltage range, to
also support the less-exact regulator.

On the rk3588 on the other hand the soc variants have different OPP
tables themselfs, because the soc itself only supports different
frequencies+voltages. So the solution here is the split of the OPPs so
that we don't mess around with /delete-node/ edits of one OPP table.

So TL;DR separate OPP tables are the way to go if the user needs different
freq+voltage values and voltage ranges allows boards to use less-adapted
regulators.


> > The values for the exact GPU OPP voltages and the lower limits for the GPU
> > OPP voltage ranges differ from the values found in the vendor kernel source
> > (cf. downstream commit f8b9431ee38e ("arm64: dts: rockchip: rk3568: support
> > adjust opp-table by otp")). [1][2]  
> 
> Why? In their latest update Rockchip changed it to the values as specified in 
> the links. My assumption is that based on extensive testing they did and/or 
> the feedback they got from the client/customers, they felt the need to change 
> it to the values they did.
> 
> I think we should follow their values unless we have an explicit and very good 
> reason to deviate from that.

Correct.
Values from some "random" Radxa kernel would also not be my
selection of choice.

In the mainline-kernel we always want the save choice - which in for me
is Rockchip's. If people want to experiment with other values on their own
boards to sort of overclock their chips, that's their prerogative.


Heiko


> > However, our values have served us well so far, so let's keep them for now,
> 
> And I don't think that qualifies as a (very) good reason.
> I think it's reasonable to assume that far more (stress) testing has been done 
> with the downstream code, then has happened with the upstream code.
> Hopefully that'll change in the future, but I don't think we're there yet.
> 
> When we/upstream adds npu support, I think we should also follow downstream's 
> OPP values, unless we have a very good reason to deviate from that.
> 
> > until we actually start supporting the CPU and GPU binning, together with
> > the related voltage adjustments.
> 
> I may not fully understand what you mean by that, but I think it's (again) 
> reasonable to assume that Rockchip has far more insight into this then we do.
> 
> Cheers,
>   Diederik
> 
> > [1]
> > https://github.com/rockchip-linux/kernel/commit/f8b9431ee38ed561650be7092ab
> > 93f564598daa9 [2]
> > https://raw.githubusercontent.com/rockchip-linux/kernel/f8b9431ee38ed561650
> > be7092ab93f564598daa9/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> 






  reply	other threads:[~2024-06-30  9:08 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-29 16:39 [PATCH v2] arm64: dts: rockchip: Add GPU OPP voltage ranges to RK356x SoC dtsi Dragan Simic
2024-06-29 16:39 ` Dragan Simic
2024-06-29 22:01 ` Diederik de Haas
2024-06-29 22:01   ` Diederik de Haas
2024-06-30  9:07   ` Heiko Stübner [this message]
2024-06-30  9:07     ` Heiko Stübner
2024-06-30 11:53     ` Diederik de Haas
2024-06-30 11:53       ` Diederik de Haas
2024-06-30 12:04   ` Dragan Simic
2024-06-30 12:04     ` Dragan Simic
2024-06-30 15:43     ` Diederik de Haas
2024-06-30 15:43       ` Diederik de Haas
2024-06-30 15:51       ` Dragan Simic
2024-06-30 15:51         ` Dragan Simic

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1894199.CQOukoFCf9@diego \
    --to=heiko@sntech.de \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=didi.debian@cknow.org \
    --cc=dsimic@manjaro.org \
    --cc=jonas@kwiboo.se \
    --cc=krzk+dt@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=robh@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.