| Message ID | 20240506-rk-dts-additions-v4-0-271023ddfd40@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | RK3588 and Rock 5B dts additions: thermal, OPP and fan | expand |
On Mon, May 6, 2024 at 1:37 PM Alexey Charkov <alchark@gmail.com> wrote: > > This enables thermal monitoring and CPU DVFS on RK3588(s), as well as > active cooling on Radxa Rock 5B via the provided PWM fan. > > Some RK3588 boards use separate regulators to supply CPUs and their > respective memory interfaces, so this is handled by coupling those > regulators in affected boards' device trees to ensure that their > voltage is adjusted in step. > > This also enables the built-in thermal sensor (TSADC) for all boards > that don't currently have it enabled, using the default CRU based > emergency thermal reset. This default configuration only uses on-SoC > devices and doesn't rely on any external wiring, thus it should work > for all devices (tested only on Rock 5B though). > > The boards that have TSADC_SHUT signal wired to the PMIC reset line > can choose to override the default reset logic in favour of GPIO > driven (PMIC assisted) reset, but in my testing it didn't work on > Radxa Rock 5B - maybe I'm reading the schematic wrong and it doesn't > support PMIC assisted reset after all. > > Fan control on Rock 5B has been split into two intervals: let it spin > at the minimum cooling state between 55C and 65C, and then accelerate > if the system crosses the 65C mark - thanks to Dragan for suggesting. > This lets some cooling setups with beefier heatsinks and/or larger > fan fins to stay in the quietest non-zero fan state while still > gaining potential benefits from the airflow it generates, and > possibly avoiding noisy speeds altogether for some workloads. > > OPPs help actually scale CPU frequencies up and down for both cooling > and performance - tested on Rock 5B under varied loads. I've dropped > those OPPs that cause frequency reductions without accompanying decrease > in CPU voltage, as they don't seem to be adding much benefit in day to > day use, while the kernel log gets a number of "OPP is inefficient" lines. > > Note that this submission doesn't touch the SRAM read margin updates or > the OPP calibration based on silicon quality which the downstream driver > does and which were mentioned in [1]. It works as it is (also confirmed by > Sebastian in his follow-up message [2]), and it is stable in my testing on > Rock 5B, so it sounds better to merge a simple version first and then > extend when/if required. > > [1] https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/ > [2] https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ > > Signed-off-by: Alexey Charkov <alchark@gmail.com> > --- Hi Heiko, Do you think this can be merged for 6.11? Looks like there hasn't been any new feedback in a while, and it would be good to have frequency scaling in place for RK3588. Please let me know if you have any reservations or if we need any broader discussion. Thanks a lot, Alexey
Hello Alexey and Heiko, On 2024-05-28 11:49, Alexey Charkov wrote: > On Mon, May 6, 2024 at 1:37 PM Alexey Charkov <alchark@gmail.com> > wrote: >> >> This enables thermal monitoring and CPU DVFS on RK3588(s), as well as >> active cooling on Radxa Rock 5B via the provided PWM fan. >> >> Some RK3588 boards use separate regulators to supply CPUs and their >> respective memory interfaces, so this is handled by coupling those >> regulators in affected boards' device trees to ensure that their >> voltage is adjusted in step. >> >> This also enables the built-in thermal sensor (TSADC) for all boards >> that don't currently have it enabled, using the default CRU based >> emergency thermal reset. This default configuration only uses on-SoC >> devices and doesn't rely on any external wiring, thus it should work >> for all devices (tested only on Rock 5B though). >> >> The boards that have TSADC_SHUT signal wired to the PMIC reset line >> can choose to override the default reset logic in favour of GPIO >> driven (PMIC assisted) reset, but in my testing it didn't work on >> Radxa Rock 5B - maybe I'm reading the schematic wrong and it doesn't >> support PMIC assisted reset after all. >> >> Fan control on Rock 5B has been split into two intervals: let it spin >> at the minimum cooling state between 55C and 65C, and then accelerate >> if the system crosses the 65C mark - thanks to Dragan for suggesting. >> This lets some cooling setups with beefier heatsinks and/or larger >> fan fins to stay in the quietest non-zero fan state while still >> gaining potential benefits from the airflow it generates, and >> possibly avoiding noisy speeds altogether for some workloads. >> >> OPPs help actually scale CPU frequencies up and down for both cooling >> and performance - tested on Rock 5B under varied loads. I've dropped >> those OPPs that cause frequency reductions without accompanying >> decrease >> in CPU voltage, as they don't seem to be adding much benefit in day to >> day use, while the kernel log gets a number of "OPP is inefficient" >> lines. >> >> Note that this submission doesn't touch the SRAM read margin updates >> or >> the OPP calibration based on silicon quality which the downstream >> driver >> does and which were mentioned in [1]. It works as it is (also >> confirmed by >> Sebastian in his follow-up message [2]), and it is stable in my >> testing on >> Rock 5B, so it sounds better to merge a simple version first and then >> extend when/if required. >> >> [1] >> https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/ >> [2] >> https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ >> >> Signed-off-by: Alexey Charkov <alchark@gmail.com> >> --- > > Hi Heiko, > > Do you think this can be merged for 6.11? Looks like there hasn't been > any new feedback in a while, and it would be good to have frequency > scaling in place for RK3588. > > Please let me know if you have any reservations or if we need any > broader discussion. As I promised earlier, I was going to test this patch series in detail. Alas, I haven't managed to do that yet, :/ due to many reasons, but I still remain firmly committed to doing that. Is -rc4 the cutoff for 6.11? If so, there's still time and I'll do my best to test and review these patches as soon as possible.
Hi Dragan, Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic: > On 2024-05-28 11:49, Alexey Charkov wrote: > > On Mon, May 6, 2024 at 1:37 PM Alexey Charkov <alchark@gmail.com> > > wrote: > >> > >> This enables thermal monitoring and CPU DVFS on RK3588(s), as well as > >> active cooling on Radxa Rock 5B via the provided PWM fan. > >> > >> Some RK3588 boards use separate regulators to supply CPUs and their > >> respective memory interfaces, so this is handled by coupling those > >> regulators in affected boards' device trees to ensure that their > >> voltage is adjusted in step. > >> > >> This also enables the built-in thermal sensor (TSADC) for all boards > >> that don't currently have it enabled, using the default CRU based > >> emergency thermal reset. This default configuration only uses on-SoC > >> devices and doesn't rely on any external wiring, thus it should work > >> for all devices (tested only on Rock 5B though). > >> > >> The boards that have TSADC_SHUT signal wired to the PMIC reset line > >> can choose to override the default reset logic in favour of GPIO > >> driven (PMIC assisted) reset, but in my testing it didn't work on > >> Radxa Rock 5B - maybe I'm reading the schematic wrong and it doesn't > >> support PMIC assisted reset after all. > >> > >> Fan control on Rock 5B has been split into two intervals: let it spin > >> at the minimum cooling state between 55C and 65C, and then accelerate > >> if the system crosses the 65C mark - thanks to Dragan for suggesting. > >> This lets some cooling setups with beefier heatsinks and/or larger > >> fan fins to stay in the quietest non-zero fan state while still > >> gaining potential benefits from the airflow it generates, and > >> possibly avoiding noisy speeds altogether for some workloads. > >> > >> OPPs help actually scale CPU frequencies up and down for both cooling > >> and performance - tested on Rock 5B under varied loads. I've dropped > >> those OPPs that cause frequency reductions without accompanying > >> decrease > >> in CPU voltage, as they don't seem to be adding much benefit in day to > >> day use, while the kernel log gets a number of "OPP is inefficient" > >> lines. > >> > >> Note that this submission doesn't touch the SRAM read margin updates > >> or > >> the OPP calibration based on silicon quality which the downstream > >> driver > >> does and which were mentioned in [1]. It works as it is (also > >> confirmed by > >> Sebastian in his follow-up message [2]), and it is stable in my > >> testing on > >> Rock 5B, so it sounds better to merge a simple version first and then > >> extend when/if required. > >> > >> [1] > >> https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/ > >> [2] > >> https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ > >> > >> Signed-off-by: Alexey Charkov <alchark@gmail.com> > >> --- > > > > Hi Heiko, > > > > Do you think this can be merged for 6.11? Looks like there hasn't been > > any new feedback in a while, and it would be good to have frequency > > scaling in place for RK3588. > > > > Please let me know if you have any reservations or if we need any > > broader discussion. not really reservations, more like there was still discussion going on around the OPPs. Meanwhile we had more discussions regarding the whole speed binning Rockchip seems to do for rk3588 variants. And waiting for the testing Dragan wanted to do ;-) . So this should definitly make it into 6.11 though, as there is still a lot of time. > As I promised earlier, I was going to test this patch series in detail. > Alas, I haven't managed to do that yet, :/ due to many reasons, but > I still remain firmly committed to doing that. > > Is -rc4 the cutoff for 6.11? If so, there's still time and I'll do my > best to test and review these patches as soon as possible. As early as possible, the hard cutoff would be -rc6 though. I guess I'll just start picking the easy patches from the series. Heiko
On 2024-05-28 16:34, Heiko Stuebner wrote: > Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic: >> On 2024-05-28 11:49, Alexey Charkov wrote: >> > On Mon, May 6, 2024 at 1:37 PM Alexey Charkov <alchark@gmail.com> >> > wrote: >> >> >> >> This enables thermal monitoring and CPU DVFS on RK3588(s), as well as >> >> active cooling on Radxa Rock 5B via the provided PWM fan. >> >> >> >> Some RK3588 boards use separate regulators to supply CPUs and their >> >> respective memory interfaces, so this is handled by coupling those >> >> regulators in affected boards' device trees to ensure that their >> >> voltage is adjusted in step. >> >> >> >> This also enables the built-in thermal sensor (TSADC) for all boards >> >> that don't currently have it enabled, using the default CRU based >> >> emergency thermal reset. This default configuration only uses on-SoC >> >> devices and doesn't rely on any external wiring, thus it should work >> >> for all devices (tested only on Rock 5B though). >> >> >> >> The boards that have TSADC_SHUT signal wired to the PMIC reset line >> >> can choose to override the default reset logic in favour of GPIO >> >> driven (PMIC assisted) reset, but in my testing it didn't work on >> >> Radxa Rock 5B - maybe I'm reading the schematic wrong and it doesn't >> >> support PMIC assisted reset after all. >> >> >> >> Fan control on Rock 5B has been split into two intervals: let it spin >> >> at the minimum cooling state between 55C and 65C, and then accelerate >> >> if the system crosses the 65C mark - thanks to Dragan for suggesting. >> >> This lets some cooling setups with beefier heatsinks and/or larger >> >> fan fins to stay in the quietest non-zero fan state while still >> >> gaining potential benefits from the airflow it generates, and >> >> possibly avoiding noisy speeds altogether for some workloads. >> >> >> >> OPPs help actually scale CPU frequencies up and down for both cooling >> >> and performance - tested on Rock 5B under varied loads. I've dropped >> >> those OPPs that cause frequency reductions without accompanying >> >> decrease >> >> in CPU voltage, as they don't seem to be adding much benefit in day to >> >> day use, while the kernel log gets a number of "OPP is inefficient" >> >> lines. >> >> >> >> Note that this submission doesn't touch the SRAM read margin updates >> >> or >> >> the OPP calibration based on silicon quality which the downstream >> >> driver >> >> does and which were mentioned in [1]. It works as it is (also >> >> confirmed by >> >> Sebastian in his follow-up message [2]), and it is stable in my >> >> testing on >> >> Rock 5B, so it sounds better to merge a simple version first and then >> >> extend when/if required. >> >> >> >> [1] >> >> https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/ >> >> [2] >> >> https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ >> >> >> >> Signed-off-by: Alexey Charkov <alchark@gmail.com> >> >> --- >> > >> > Hi Heiko, >> > >> > Do you think this can be merged for 6.11? Looks like there hasn't been >> > any new feedback in a while, and it would be good to have frequency >> > scaling in place for RK3588. >> > >> > Please let me know if you have any reservations or if we need any >> > broader discussion. > > not really reservations, more like there was still discussion going on > around the OPPs. Meanwhile we had more discussions regarding the whole > speed binning Rockchip seems to do for rk3588 variants. > > And waiting for the testing Dragan wanted to do ;-) . I'm sorry for the delays. > So this should definitly make it into 6.11 though, as there is still > a lot of time. > >> As I promised earlier, I was going to test this patch series in >> detail. >> Alas, I haven't managed to do that yet, :/ due to many reasons, but >> I still remain firmly committed to doing that. >> >> Is -rc4 the cutoff for 6.11? If so, there's still time and I'll do my >> best to test and review these patches as soon as possible. > > As early as possible, the hard cutoff would be -rc6 though. > I guess I'll just start picking the easy patches from the series. I'll do my best to have the patches tested and reviewed in detail ASAP. As a suggestion, perhaps it would be better to take the series as a whole, so we don't bring partial merging into the mix.
Am Dienstag, 28. Mai 2024, 17:01:48 CEST schrieb Dragan Simic: > On 2024-05-28 16:34, Heiko Stuebner wrote: > > Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic: > >> On 2024-05-28 11:49, Alexey Charkov wrote: > >> > On Mon, May 6, 2024 at 1:37 PM Alexey Charkov <alchark@gmail.com> > >> > wrote: > >> >> > >> >> This enables thermal monitoring and CPU DVFS on RK3588(s), as well as > >> >> active cooling on Radxa Rock 5B via the provided PWM fan. > >> >> > >> >> Some RK3588 boards use separate regulators to supply CPUs and their > >> >> respective memory interfaces, so this is handled by coupling those > >> >> regulators in affected boards' device trees to ensure that their > >> >> voltage is adjusted in step. > >> >> > >> >> This also enables the built-in thermal sensor (TSADC) for all boards > >> >> that don't currently have it enabled, using the default CRU based > >> >> emergency thermal reset. This default configuration only uses on-SoC > >> >> devices and doesn't rely on any external wiring, thus it should work > >> >> for all devices (tested only on Rock 5B though). > >> >> > >> >> The boards that have TSADC_SHUT signal wired to the PMIC reset line > >> >> can choose to override the default reset logic in favour of GPIO > >> >> driven (PMIC assisted) reset, but in my testing it didn't work on > >> >> Radxa Rock 5B - maybe I'm reading the schematic wrong and it doesn't > >> >> support PMIC assisted reset after all. > >> >> > >> >> Fan control on Rock 5B has been split into two intervals: let it spin > >> >> at the minimum cooling state between 55C and 65C, and then accelerate > >> >> if the system crosses the 65C mark - thanks to Dragan for suggesting. > >> >> This lets some cooling setups with beefier heatsinks and/or larger > >> >> fan fins to stay in the quietest non-zero fan state while still > >> >> gaining potential benefits from the airflow it generates, and > >> >> possibly avoiding noisy speeds altogether for some workloads. > >> >> > >> >> OPPs help actually scale CPU frequencies up and down for both cooling > >> >> and performance - tested on Rock 5B under varied loads. I've dropped > >> >> those OPPs that cause frequency reductions without accompanying > >> >> decrease > >> >> in CPU voltage, as they don't seem to be adding much benefit in day to > >> >> day use, while the kernel log gets a number of "OPP is inefficient" > >> >> lines. > >> >> > >> >> Note that this submission doesn't touch the SRAM read margin updates > >> >> or > >> >> the OPP calibration based on silicon quality which the downstream > >> >> driver > >> >> does and which were mentioned in [1]. It works as it is (also > >> >> confirmed by > >> >> Sebastian in his follow-up message [2]), and it is stable in my > >> >> testing on > >> >> Rock 5B, so it sounds better to merge a simple version first and then > >> >> extend when/if required. > >> >> > >> >> [1] > >> >> https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/ > >> >> [2] > >> >> https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ > >> >> > >> >> Signed-off-by: Alexey Charkov <alchark@gmail.com> > >> >> --- > >> > > >> > Hi Heiko, > >> > > >> > Do you think this can be merged for 6.11? Looks like there hasn't been > >> > any new feedback in a while, and it would be good to have frequency > >> > scaling in place for RK3588. > >> > > >> > Please let me know if you have any reservations or if we need any > >> > broader discussion. > > > > not really reservations, more like there was still discussion going on > > around the OPPs. Meanwhile we had more discussions regarding the whole > > speed binning Rockchip seems to do for rk3588 variants. > > > > And waiting for the testing Dragan wanted to do ;-) . > > I'm sorry for the delays. Was definitly _not_ meant as blame ;-) . The series has just too many discussions threads to unravel on half an afternoon. > > So this should definitly make it into 6.11 though, as there is still > > a lot of time. > > > >> As I promised earlier, I was going to test this patch series in > >> detail. > >> Alas, I haven't managed to do that yet, :/ due to many reasons, but > >> I still remain firmly committed to doing that. > >> > >> Is -rc4 the cutoff for 6.11? If so, there's still time and I'll do my > >> best to test and review these patches as soon as possible. > > > > As early as possible, the hard cutoff would be -rc6 though. > > I guess I'll just start picking the easy patches from the series. > > I'll do my best to have the patches tested and reviewed in detail ASAP. > As a suggestion, perhaps it would be better to take the series as a > whole, > so we don't bring partial merging into the mix. Patches need to work individually anyway (in correct order of course), so like starting at the top with the general thermal stuff should not create issues ;-) Heiko
On 2024-05-28 17:16, Heiko Stuebner wrote: > Am Dienstag, 28. Mai 2024, 17:01:48 CEST schrieb Dragan Simic: >> On 2024-05-28 16:34, Heiko Stuebner wrote: >> > Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic: >> >> On 2024-05-28 11:49, Alexey Charkov wrote: >> >> > On Mon, May 6, 2024 at 1:37 PM Alexey Charkov <alchark@gmail.com> >> >> > wrote: >> >> >> >> >> >> This enables thermal monitoring and CPU DVFS on RK3588(s), as well as >> >> >> active cooling on Radxa Rock 5B via the provided PWM fan. >> >> >> >> >> >> Some RK3588 boards use separate regulators to supply CPUs and their >> >> >> respective memory interfaces, so this is handled by coupling those >> >> >> regulators in affected boards' device trees to ensure that their >> >> >> voltage is adjusted in step. >> >> >> >> >> >> This also enables the built-in thermal sensor (TSADC) for all boards >> >> >> that don't currently have it enabled, using the default CRU based >> >> >> emergency thermal reset. This default configuration only uses on-SoC >> >> >> devices and doesn't rely on any external wiring, thus it should work >> >> >> for all devices (tested only on Rock 5B though). >> >> >> >> >> >> The boards that have TSADC_SHUT signal wired to the PMIC reset line >> >> >> can choose to override the default reset logic in favour of GPIO >> >> >> driven (PMIC assisted) reset, but in my testing it didn't work on >> >> >> Radxa Rock 5B - maybe I'm reading the schematic wrong and it doesn't >> >> >> support PMIC assisted reset after all. >> >> >> >> >> >> Fan control on Rock 5B has been split into two intervals: let it spin >> >> >> at the minimum cooling state between 55C and 65C, and then accelerate >> >> >> if the system crosses the 65C mark - thanks to Dragan for suggesting. >> >> >> This lets some cooling setups with beefier heatsinks and/or larger >> >> >> fan fins to stay in the quietest non-zero fan state while still >> >> >> gaining potential benefits from the airflow it generates, and >> >> >> possibly avoiding noisy speeds altogether for some workloads. >> >> >> >> >> >> OPPs help actually scale CPU frequencies up and down for both cooling >> >> >> and performance - tested on Rock 5B under varied loads. I've dropped >> >> >> those OPPs that cause frequency reductions without accompanying >> >> >> decrease >> >> >> in CPU voltage, as they don't seem to be adding much benefit in day to >> >> >> day use, while the kernel log gets a number of "OPP is inefficient" >> >> >> lines. >> >> >> >> >> >> Note that this submission doesn't touch the SRAM read margin updates >> >> >> or >> >> >> the OPP calibration based on silicon quality which the downstream >> >> >> driver >> >> >> does and which were mentioned in [1]. It works as it is (also >> >> >> confirmed by >> >> >> Sebastian in his follow-up message [2]), and it is stable in my >> >> >> testing on >> >> >> Rock 5B, so it sounds better to merge a simple version first and then >> >> >> extend when/if required. >> >> >> >> >> >> [1] >> >> >> https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/ >> >> >> [2] >> >> >> https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ >> >> >> >> >> >> Signed-off-by: Alexey Charkov <alchark@gmail.com> >> >> >> --- >> >> > >> >> > Hi Heiko, >> >> > >> >> > Do you think this can be merged for 6.11? Looks like there hasn't been >> >> > any new feedback in a while, and it would be good to have frequency >> >> > scaling in place for RK3588. >> >> > >> >> > Please let me know if you have any reservations or if we need any >> >> > broader discussion. >> > >> > not really reservations, more like there was still discussion going on >> > around the OPPs. Meanwhile we had more discussions regarding the whole >> > speed binning Rockchip seems to do for rk3588 variants. >> > >> > And waiting for the testing Dragan wanted to do ;-) . >> >> I'm sorry for the delays. > > Was definitly _not_ meant as blame ;-) . Thanks, but I do blame myself a bit for not doing it earlier. :) > The series has just too many discussions threads to unravel on half > an afternoon. Yes, we've touched quite a few areas, for some of which I've already started working on the associated patches. That was one of the reasons for the delays. >> > So this should definitly make it into 6.11 though, as there is still >> > a lot of time. >> > >> >> As I promised earlier, I was going to test this patch series in >> >> detail. >> >> Alas, I haven't managed to do that yet, :/ due to many reasons, but >> >> I still remain firmly committed to doing that. >> >> >> >> Is -rc4 the cutoff for 6.11? If so, there's still time and I'll do my >> >> best to test and review these patches as soon as possible. >> > >> > As early as possible, the hard cutoff would be -rc6 though. >> > I guess I'll just start picking the easy patches from the series. >> >> I'll do my best to have the patches tested and reviewed in detail >> ASAP. >> As a suggestion, perhaps it would be better to take the series as a >> whole, >> so we don't bring partial merging into the mix. > > Patches need to work individually anyway (in correct order of course), > so like starting at the top with the general thermal stuff should not > create issues ;-) Of course, but I might actually have some comments and suggestions for some of the patches, and addressing those suggestions, if we end up agreeing on them, would be a bit messy if some of the patches in the series become merged first.
[sorry if you get this twice: I messed up and sent the previous one in HTML instead of plain text - resending now] On Tue, May 28, 2024 at 7:16 PM Heiko Stuebner <heiko@sntech.de> wrote: > > Am Dienstag, 28. Mai 2024, 17:01:48 CEST schrieb Dragan Simic: > > On 2024-05-28 16:34, Heiko Stuebner wrote: > > > Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic: > > >> On 2024-05-28 11:49, Alexey Charkov wrote: > > >> > On Mon, May 6, 2024 at 1:37 PM Alexey Charkov <alchark@gmail.com> > > >> > wrote: > > >> >> > > >> >> This enables thermal monitoring and CPU DVFS on RK3588(s), as well as > > >> >> active cooling on Radxa Rock 5B via the provided PWM fan. > > >> >> > > >> >> Some RK3588 boards use separate regulators to supply CPUs and their > > >> >> respective memory interfaces, so this is handled by coupling those > > >> >> regulators in affected boards' device trees to ensure that their > > >> >> voltage is adjusted in step. > > >> >> > > >> >> This also enables the built-in thermal sensor (TSADC) for all boards > > >> >> that don't currently have it enabled, using the default CRU based > > >> >> emergency thermal reset. This default configuration only uses on-SoC > > >> >> devices and doesn't rely on any external wiring, thus it should work > > >> >> for all devices (tested only on Rock 5B though). > > >> >> > > >> >> The boards that have TSADC_SHUT signal wired to the PMIC reset line > > >> >> can choose to override the default reset logic in favour of GPIO > > >> >> driven (PMIC assisted) reset, but in my testing it didn't work on > > >> >> Radxa Rock 5B - maybe I'm reading the schematic wrong and it doesn't > > >> >> support PMIC assisted reset after all. > > >> >> > > >> >> Fan control on Rock 5B has been split into two intervals: let it spin > > >> >> at the minimum cooling state between 55C and 65C, and then accelerate > > >> >> if the system crosses the 65C mark - thanks to Dragan for suggesting. > > >> >> This lets some cooling setups with beefier heatsinks and/or larger > > >> >> fan fins to stay in the quietest non-zero fan state while still > > >> >> gaining potential benefits from the airflow it generates, and > > >> >> possibly avoiding noisy speeds altogether for some workloads. > > >> >> > > >> >> OPPs help actually scale CPU frequencies up and down for both cooling > > >> >> and performance - tested on Rock 5B under varied loads. I've dropped > > >> >> those OPPs that cause frequency reductions without accompanying > > >> >> decrease > > >> >> in CPU voltage, as they don't seem to be adding much benefit in day to > > >> >> day use, while the kernel log gets a number of "OPP is inefficient" > > >> >> lines. > > >> >> > > >> >> Note that this submission doesn't touch the SRAM read margin updates > > >> >> or > > >> >> the OPP calibration based on silicon quality which the downstream > > >> >> driver > > >> >> does and which were mentioned in [1]. It works as it is (also > > >> >> confirmed by > > >> >> Sebastian in his follow-up message [2]), and it is stable in my > > >> >> testing on > > >> >> Rock 5B, so it sounds better to merge a simple version first and then > > >> >> extend when/if required. > > >> >> > > >> >> [1] > > >> >> https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/ > > >> >> [2] > > >> >> https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ > > >> >> > > >> >> Signed-off-by: Alexey Charkov <alchark@gmail.com> > > >> >> --- > > >> > > > >> > Hi Heiko, > > >> > > > >> > Do you think this can be merged for 6.11? Looks like there hasn't been > > >> > any new feedback in a while, and it would be good to have frequency > > >> > scaling in place for RK3588. > > >> > > > >> > Please let me know if you have any reservations or if we need any > > >> > broader discussion. > > > > > > not really reservations, more like there was still discussion going on > > > around the OPPs. Meanwhile we had more discussions regarding the whole > > > speed binning Rockchip seems to do for rk3588 variants. > > > > > > And waiting for the testing Dragan wanted to do ;-) . > > > > I'm sorry for the delays. > > Was definitly _not_ meant as blame ;-) . > > The series has just too many discussions threads to unravel on half > an afternoon. FWIW, I think the latest exchange we had with Quentin regarding the OPPs concluded in “false alarm”, given that this version of the series only introduces a subset of them which should apply to all RK3588(s) Performance binning here is more geared towards how low the voltages can go for a given frequency, and right now we’re only introducing the highest-voltage setting for each OPP. Thus the binning and/or intermediate frequencies should be possible to introduce at a later stage in a backwards compatible way (if deemed relevant). > > > So this should definitly make it into 6.11 though, as there is still > > > a lot of time. > > > > > >> As I promised earlier, I was going to test this patch series in > > >> detail. > > >> Alas, I haven't managed to do that yet, :/ due to many reasons, but > > >> I still remain firmly committed to doing that. > > >> > > >> Is -rc4 the cutoff for 6.11? If so, there's still time and I'll do my > > >> best to test and review these patches as soon as possible. > > > > > > As early as possible, the hard cutoff would be -rc6 though. > > > I guess I'll just start picking the easy patches from the series. > > > > I'll do my best to have the patches tested and reviewed in detail ASAP. > > As a suggestion, perhaps it would be better to take the series as a > > whole, > > so we don't bring partial merging into the mix. > > Patches need to work individually anyway (in correct order of course), > so like starting at the top with the general thermal stuff should not > create issues ;-) Indeed, those are self-contained and can be merged independently of the OPPs. Having OPPs without thermal is more risky though, but that doesn’t sound like what we’re after here :) Best regards, Alexey
Hi all, On 5/28/24 5:42 PM, Alexey Charkov wrote: > On Tue, 28 May 2024 at 19:16, Heiko Stuebner <heiko@sntech.de> wrote: > >> Am Dienstag, 28. Mai 2024, 17:01:48 CEST schrieb Dragan Simic: >>> On 2024-05-28 16:34, Heiko Stuebner wrote: >>>> Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic: >>>>> On 2024-05-28 11:49, Alexey Charkov wrote: >>>>>> On Mon, May 6, 2024 at 1:37 PM Alexey Charkov <alchark@gmail.com> >>>>>> wrote: >>>>>>> >>>>>>> This enables thermal monitoring and CPU DVFS on RK3588(s), as well >> as >>>>>>> active cooling on Radxa Rock 5B via the provided PWM fan. >>>>>>> >>>>>>> Some RK3588 boards use separate regulators to supply CPUs and their >>>>>>> respective memory interfaces, so this is handled by coupling those >>>>>>> regulators in affected boards' device trees to ensure that their >>>>>>> voltage is adjusted in step. >>>>>>> >>>>>>> This also enables the built-in thermal sensor (TSADC) for all >> boards >>>>>>> that don't currently have it enabled, using the default CRU based >>>>>>> emergency thermal reset. This default configuration only uses >> on-SoC >>>>>>> devices and doesn't rely on any external wiring, thus it should >> work >>>>>>> for all devices (tested only on Rock 5B though). >>>>>>> >>>>>>> The boards that have TSADC_SHUT signal wired to the PMIC reset line >>>>>>> can choose to override the default reset logic in favour of GPIO >>>>>>> driven (PMIC assisted) reset, but in my testing it didn't work on >>>>>>> Radxa Rock 5B - maybe I'm reading the schematic wrong and it >> doesn't >>>>>>> support PMIC assisted reset after all. >>>>>>> >>>>>>> Fan control on Rock 5B has been split into two intervals: let it >> spin >>>>>>> at the minimum cooling state between 55C and 65C, and then >> accelerate >>>>>>> if the system crosses the 65C mark - thanks to Dragan for >> suggesting. >>>>>>> This lets some cooling setups with beefier heatsinks and/or larger >>>>>>> fan fins to stay in the quietest non-zero fan state while still >>>>>>> gaining potential benefits from the airflow it generates, and >>>>>>> possibly avoiding noisy speeds altogether for some workloads. >>>>>>> >>>>>>> OPPs help actually scale CPU frequencies up and down for both >> cooling >>>>>>> and performance - tested on Rock 5B under varied loads. I've >> dropped >>>>>>> those OPPs that cause frequency reductions without accompanying >>>>>>> decrease >>>>>>> in CPU voltage, as they don't seem to be adding much benefit in >> day to >>>>>>> day use, while the kernel log gets a number of "OPP is inefficient" >>>>>>> lines. >>>>>>> >>>>>>> Note that this submission doesn't touch the SRAM read margin >> updates >>>>>>> or >>>>>>> the OPP calibration based on silicon quality which the downstream >>>>>>> driver >>>>>>> does and which were mentioned in [1]. It works as it is (also >>>>>>> confirmed by >>>>>>> Sebastian in his follow-up message [2]), and it is stable in my >>>>>>> testing on >>>>>>> Rock 5B, so it sounds better to merge a simple version first and >> then >>>>>>> extend when/if required. >>>>>>> >>>>>>> [1] >>>>>>> >> https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/ >>>>>>> [2] >>>>>>> >> https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ >>>>>>> >>>>>>> Signed-off-by: Alexey Charkov <alchark@gmail.com> >>>>>>> --- >>>>>> >>>>>> Hi Heiko, >>>>>> >>>>>> Do you think this can be merged for 6.11? Looks like there hasn't >> been >>>>>> any new feedback in a while, and it would be good to have frequency >>>>>> scaling in place for RK3588. >>>>>> >>>>>> Please let me know if you have any reservations or if we need any >>>>>> broader discussion. >>>> >>>> not really reservations, more like there was still discussion going on >>>> around the OPPs. Meanwhile we had more discussions regarding the whole >>>> speed binning Rockchip seems to do for rk3588 variants. >>>> >>>> And waiting for the testing Dragan wanted to do ;-) . >>> >>> I'm sorry for the delays. >> >> Was definitly _not_ meant as blame ;-) . >> >> The series has just too many discussions threads to unravel on half >> an afternoon. > > > FWIW, I think the latest exchange we had with Quentin regarding the OPPs > concluded in “false alarm”, given that this version of the series only > introduces a subset of them which should apply to all RK3588(s) > Correct. However... I'm wondering if we shouldn't somehow follow the same pattern we have used for the rk3399 OPPs? We have a file for the "true" RK3399 OPPs, then the OP1 variant and the RK3399T. We already know there are a few variants of RK3588 with different OPPs: RK3588(S/S2?), RK3588J and RK3588M. I wouldn't be surprised if the RK3582 (though this one has already one big cluster (or two big cores) fewer than RK3588) has different OPPs as well? So. We have already discussed that the OPPs in that patch are valid for RK3588(S) but they aren't for the other variants. In the downstream kernel, any OPP whose opp-supported-hw has a first value masked by BIT(1) return non-0 is supported by RK3588M. In the downstream kernel, any OPP whose opp-supported-hw has a first value masked by BIT(2) return non-0 is supported by RK3588J. This means that, for LITTLE clusters: - opp-1608000000 not supported on RK3588J - opp-1704000000 only supported on RK3588M (but already absent in this patch series) - opp-1800000000 only supported on RK3588(S), not RK3588J nor RK3588M For big clusters: - opp-1800000000 not supported on RK3588J - opp-2016000000 not supported on RK3588J - opp-2208000000 only supported on RK3588(S), not RK3588J nor RK3588M - opp-2256000000 only supported on RK3588(S), not RK3588J nor RK3588M - opp-2304000000 only supported on RK3588(S), not RK3588J nor RK3588M - opp-2352000000 only supported on RK3588(S), not RK3588J nor RK3588M - opp-2400000000 only supported on RK3588(S), not RK3588J nor RK3588M This is somehow also enforced in downstream kernel by removing the OPP nodes directly (hence, not even requiring the check of opp-supported-hw value), c.f.: https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588j.dtsi https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588m.dtsi You'll not that the RK3588J also has less OPPs for the GPU and NPU (but those should also be masked by the opp-supported-hw value). Cheers, Quentin
On Tue, May 28, 2024 at 8:08 PM Quentin Schulz <quentin.schulz@cherry.de> wrote: > > Hi all, > > On 5/28/24 5:42 PM, Alexey Charkov wrote: > > On Tue, 28 May 2024 at 19:16, Heiko Stuebner <heiko@sntech.de> wrote: > > > >> Am Dienstag, 28. Mai 2024, 17:01:48 CEST schrieb Dragan Simic: > >>> On 2024-05-28 16:34, Heiko Stuebner wrote: > >>>> Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic: > >>>>> On 2024-05-28 11:49, Alexey Charkov wrote: > >>>>>> On Mon, May 6, 2024 at 1:37 PM Alexey Charkov <alchark@gmail.com> > >>>>>> wrote: > >>>>>>> > >>>>>>> This enables thermal monitoring and CPU DVFS on RK3588(s), as well > >> as > >>>>>>> active cooling on Radxa Rock 5B via the provided PWM fan. > >>>>>>> > >>>>>>> Some RK3588 boards use separate regulators to supply CPUs and their > >>>>>>> respective memory interfaces, so this is handled by coupling those > >>>>>>> regulators in affected boards' device trees to ensure that their > >>>>>>> voltage is adjusted in step. > >>>>>>> > >>>>>>> This also enables the built-in thermal sensor (TSADC) for all > >> boards > >>>>>>> that don't currently have it enabled, using the default CRU based > >>>>>>> emergency thermal reset. This default configuration only uses > >> on-SoC > >>>>>>> devices and doesn't rely on any external wiring, thus it should > >> work > >>>>>>> for all devices (tested only on Rock 5B though). > >>>>>>> > >>>>>>> The boards that have TSADC_SHUT signal wired to the PMIC reset line > >>>>>>> can choose to override the default reset logic in favour of GPIO > >>>>>>> driven (PMIC assisted) reset, but in my testing it didn't work on > >>>>>>> Radxa Rock 5B - maybe I'm reading the schematic wrong and it > >> doesn't > >>>>>>> support PMIC assisted reset after all. > >>>>>>> > >>>>>>> Fan control on Rock 5B has been split into two intervals: let it > >> spin > >>>>>>> at the minimum cooling state between 55C and 65C, and then > >> accelerate > >>>>>>> if the system crosses the 65C mark - thanks to Dragan for > >> suggesting. > >>>>>>> This lets some cooling setups with beefier heatsinks and/or larger > >>>>>>> fan fins to stay in the quietest non-zero fan state while still > >>>>>>> gaining potential benefits from the airflow it generates, and > >>>>>>> possibly avoiding noisy speeds altogether for some workloads. > >>>>>>> > >>>>>>> OPPs help actually scale CPU frequencies up and down for both > >> cooling > >>>>>>> and performance - tested on Rock 5B under varied loads. I've > >> dropped > >>>>>>> those OPPs that cause frequency reductions without accompanying > >>>>>>> decrease > >>>>>>> in CPU voltage, as they don't seem to be adding much benefit in > >> day to > >>>>>>> day use, while the kernel log gets a number of "OPP is inefficient" > >>>>>>> lines. > >>>>>>> > >>>>>>> Note that this submission doesn't touch the SRAM read margin > >> updates > >>>>>>> or > >>>>>>> the OPP calibration based on silicon quality which the downstream > >>>>>>> driver > >>>>>>> does and which were mentioned in [1]. It works as it is (also > >>>>>>> confirmed by > >>>>>>> Sebastian in his follow-up message [2]), and it is stable in my > >>>>>>> testing on > >>>>>>> Rock 5B, so it sounds better to merge a simple version first and > >> then > >>>>>>> extend when/if required. > >>>>>>> > >>>>>>> [1] > >>>>>>> > >> https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/ > >>>>>>> [2] > >>>>>>> > >> https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ > >>>>>>> > >>>>>>> Signed-off-by: Alexey Charkov <alchark@gmail.com> > >>>>>>> --- > >>>>>> > >>>>>> Hi Heiko, > >>>>>> > >>>>>> Do you think this can be merged for 6.11? Looks like there hasn't > >> been > >>>>>> any new feedback in a while, and it would be good to have frequency > >>>>>> scaling in place for RK3588. > >>>>>> > >>>>>> Please let me know if you have any reservations or if we need any > >>>>>> broader discussion. > >>>> > >>>> not really reservations, more like there was still discussion going on > >>>> around the OPPs. Meanwhile we had more discussions regarding the whole > >>>> speed binning Rockchip seems to do for rk3588 variants. > >>>> > >>>> And waiting for the testing Dragan wanted to do ;-) . > >>> > >>> I'm sorry for the delays. > >> > >> Was definitly _not_ meant as blame ;-) . > >> > >> The series has just too many discussions threads to unravel on half > >> an afternoon. > > > > > > FWIW, I think the latest exchange we had with Quentin regarding the OPPs > > concluded in “false alarm”, given that this version of the series only > > introduces a subset of them which should apply to all RK3588(s) > > > > Correct. > > However... I'm wondering if we shouldn't somehow follow the same pattern > we have used for the rk3399 OPPs? We have a file for the "true" RK3399 > OPPs, then the OP1 variant and the RK3399T. > > We already know there are a few variants of RK3588 with different OPPs: > RK3588(S/S2?), RK3588J and RK3588M. I wouldn't be surprised if the > RK3582 (though this one has already one big cluster (or two big cores) > fewer than RK3588) has different OPPs as well? > > So. We have already discussed that the OPPs in that patch are valid for > RK3588(S) but they aren't for the other variants. Hmm. Looking at Rockchip sources [1] more closely as opposed to the Radxa tree I've been using as the basis, it does indeed show that RK3588J and RK3588M use a different OPP table altogether (frequencies are similar, but voltages differ). We currently have an RK3588J based board in the mainline tree (rk3588-edgeble-neu6b-io.dts), so it can't be ignored. However, given that Rockchip sources only differentiate those OPPs by SoC revision, and that is (currently?) known for each board at dtb compile time, it seems easier to just include two different OPP tables for RK3588(S) vs. RK3588J - thus avoiding all the headache with opp-supported-hw. Similar to RK3399, yes. Will split those out and send a separate version. > In the downstream kernel, any OPP whose opp-supported-hw has a first > value masked by BIT(1) return non-0 is supported by RK3588M. In the > downstream kernel, any OPP whose opp-supported-hw has a first value > masked by BIT(2) return non-0 is supported by RK3588J. > > This means that, for LITTLE clusters: > - opp-1608000000 not supported on RK3588J > - opp-1704000000 only supported on RK3588M (but already absent in this > patch series) > - opp-1800000000 only supported on RK3588(S), not RK3588J nor RK3588M > > For big clusters: > - opp-1800000000 not supported on RK3588J > - opp-2016000000 not supported on RK3588J > - opp-2208000000 only supported on RK3588(S), not RK3588J nor RK3588M > - opp-2256000000 only supported on RK3588(S), not RK3588J nor RK3588M > - opp-2304000000 only supported on RK3588(S), not RK3588J nor RK3588M > - opp-2352000000 only supported on RK3588(S), not RK3588J nor RK3588M > - opp-2400000000 only supported on RK3588(S), not RK3588J nor RK3588M > > This is somehow also enforced in downstream kernel by removing the OPP > nodes directly (hence, not even requiring the check of opp-supported-hw > value), c.f.: > https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588j.dtsi > https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588m.dtsi > > You'll not that the RK3588J also has less OPPs for the GPU and NPU (but > those should also be masked by the opp-supported-hw value). Same with DMC, but we don't currently have either DMC or NPU in the mainline tree, so it sounds like something to be dealt with later :) Best regards, Alexey [1] https://github.com/rockchip-linux/kernel/blob/develop-5.10/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
Hello Quentin, On 2024-05-28 18:08, Quentin Schulz wrote: > On 5/28/24 5:42 PM, Alexey Charkov wrote: >> On Tue, 28 May 2024 at 19:16, Heiko Stuebner <heiko@sntech.de> wrote: >>> Am Dienstag, 28. Mai 2024, 17:01:48 CEST schrieb Dragan Simic: >>>> On 2024-05-28 16:34, Heiko Stuebner wrote: >>>>> Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic: >>>>>> On 2024-05-28 11:49, Alexey Charkov wrote: >>>>>>> Do you think this can be merged for 6.11? Looks like there hasn't >>> been >>>>>>> any new feedback in a while, and it would be good to have >>>>>>> frequency >>>>>>> scaling in place for RK3588. >>>>>>> >>>>>>> Please let me know if you have any reservations or if we need any >>>>>>> broader discussion. >>>>> >>>>> not really reservations, more like there was still discussion going >>>>> on >>>>> around the OPPs. Meanwhile we had more discussions regarding the >>>>> whole >>>>> speed binning Rockchip seems to do for rk3588 variants. >>>>> >>>>> And waiting for the testing Dragan wanted to do ;-) . >>>> >>>> I'm sorry for the delays. >>> >>> Was definitly _not_ meant as blame ;-) . >>> >>> The series has just too many discussions threads to unravel on half >>> an afternoon. >> >> FWIW, I think the latest exchange we had with Quentin regarding the >> OPPs >> concluded in “false alarm”, given that this version of the series only >> introduces a subset of them which should apply to all RK3588(s) > > Correct. > > However... I'm wondering if we shouldn't somehow follow the same > pattern we have used for the rk3399 OPPs? We have a file for the > "true" RK3399 OPPs, then the OP1 variant and the RK3399T. If I'm not mistaken, the separate rk3399-*opp*.dtsi files were added when the need arose. > We already know there are a few variants of RK3588 with different > OPPs: RK3588(S/S2?), RK3588J and RK3588M. I wouldn't be surprised if > the RK3582 (though this one has already one big cluster (or two big > cores) fewer than RK3588) has different OPPs as well? Do we already have supported boards that use the RK3588J and RK3588M variants of the RK3588 SoC? If yes, we should separate the relevant OPPs into the separate .dtsi files, but if not, we should wait until the need arises. > So. We have already discussed that the OPPs in that patch are valid > for RK3588(S) but they aren't for the other variants. ... which applies currently if there are already other RK3588 variants in use on the supported boards. > In the downstream kernel, any OPP whose opp-supported-hw has a first > value masked by BIT(1) return non-0 is supported by RK3588M. In the > downstream kernel, any OPP whose opp-supported-hw has a first value > masked by BIT(2) return non-0 is supported by RK3588J. > > This means that, for LITTLE clusters: > - opp-1608000000 not supported on RK3588J > - opp-1704000000 only supported on RK3588M (but already absent in this > patch series) > - opp-1800000000 only supported on RK3588(S), not RK3588J nor RK3588M > > For big clusters: > - opp-1800000000 not supported on RK3588J > - opp-2016000000 not supported on RK3588J > - opp-2208000000 only supported on RK3588(S), not RK3588J nor RK3588M > - opp-2256000000 only supported on RK3588(S), not RK3588J nor RK3588M > - opp-2304000000 only supported on RK3588(S), not RK3588J nor RK3588M > - opp-2352000000 only supported on RK3588(S), not RK3588J nor RK3588M > - opp-2400000000 only supported on RK3588(S), not RK3588J nor RK3588M > > This is somehow also enforced in downstream kernel by removing the OPP > nodes directly (hence, not even requiring the check of > opp-supported-hw value), c.f.: > https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588j.dtsi > https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588m.dtsi > > You'll not that the RK3588J also has less OPPs for the GPU and NPU > (but those should also be masked by the opp-supported-hw value).
Hello Alexey and Quentin, On 2024-05-28 21:26, Alexey Charkov wrote: > On Tue, May 28, 2024 at 8:08 PM Quentin Schulz > <quentin.schulz@cherry.de> wrote: >> On 5/28/24 5:42 PM, Alexey Charkov wrote: >> > On Tue, 28 May 2024 at 19:16, Heiko Stuebner <heiko@sntech.de> wrote: >> >> Am Dienstag, 28. Mai 2024, 17:01:48 CEST schrieb Dragan Simic: >> >>> On 2024-05-28 16:34, Heiko Stuebner wrote: >> >>>> Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic: >> >>>>> On 2024-05-28 11:49, Alexey Charkov wrote: >> >>>>>> Hi Heiko, >> >>>>>> >> >>>>>> Do you think this can be merged for 6.11? Looks like there hasn't >> >> been >> >>>>>> any new feedback in a while, and it would be good to have frequency >> >>>>>> scaling in place for RK3588. >> >>>>>> >> >>>>>> Please let me know if you have any reservations or if we need any >> >>>>>> broader discussion. >> >>>> >> >>>> not really reservations, more like there was still discussion going on >> >>>> around the OPPs. Meanwhile we had more discussions regarding the whole >> >>>> speed binning Rockchip seems to do for rk3588 variants. >> >>>> >> >>>> And waiting for the testing Dragan wanted to do ;-) . >> >>> >> >>> I'm sorry for the delays. >> >> >> >> Was definitly _not_ meant as blame ;-) . >> >> >> >> The series has just too many discussions threads to unravel on half >> >> an afternoon. >> > >> > >> > FWIW, I think the latest exchange we had with Quentin regarding the OPPs >> > concluded in “false alarm”, given that this version of the series only >> > introduces a subset of them which should apply to all RK3588(s) >> >> Correct. >> >> However... I'm wondering if we shouldn't somehow follow the same >> pattern >> we have used for the rk3399 OPPs? We have a file for the "true" RK3399 >> OPPs, then the OP1 variant and the RK3399T. >> >> We already know there are a few variants of RK3588 with different >> OPPs: >> RK3588(S/S2?), RK3588J and RK3588M. I wouldn't be surprised if the >> RK3582 (though this one has already one big cluster (or two big cores) >> fewer than RK3588) has different OPPs as well? >> >> So. We have already discussed that the OPPs in that patch are valid >> for >> RK3588(S) but they aren't for the other variants. > > Hmm. Looking at Rockchip sources [1] more closely as opposed to the > Radxa tree I've been using as the basis, it does indeed show that > RK3588J and RK3588M use a different OPP table altogether (frequencies > are similar, but voltages differ). > > We currently have an RK3588J based board in the mainline tree > (rk3588-edgeble-neu6b-io.dts), so it can't be ignored. However, given > that Rockchip sources only differentiate those OPPs by SoC revision, > and that is (currently?) known for each board at dtb compile time, it > seems easier to just include two different OPP tables for RK3588(S) > vs. RK3588J - thus avoiding all the headache with opp-supported-hw. > Similar to RK3399, yes. > > Will split those out and send a separate version. Ah, you already answered my question about the existence of supported boards with the RK3588J and RK3588M variants. :) [2] I totally agree about splitting the OPPs into the separate .dtsi files, i.e. following the approach already established by the RK3399. Perhaps the new files should be named rk3588-opp.dtsi and rk3588j-opp.dtsi, to (almost fully) follow the already established naming scheme. Maybe the OPP data could instead be added to the already existing rk3588.dtsi, rk3588s.dtsi and rk3588j.dtsi files, which would actually make more sense because those .dtsi files already address the specific SoC variants, but the approach with the separate new rk3588*-opp.dtsi files seems cleaner from the implementation point of view, and is a bit safer. If you see a clean way for stuffing the OPP data into the already existing rk3588.dtsi, rk3588s.dtsi and rk3588j.dtsi files, instead of introducing new rk3588*-opp.dtsi files, I'd be happy to support it. [2] https://lore.kernel.org/linux-rockchip/7b09e18e850ff0832bd7236810b83e64@manjaro.org/ >> In the downstream kernel, any OPP whose opp-supported-hw has a first >> value masked by BIT(1) return non-0 is supported by RK3588M. In the >> downstream kernel, any OPP whose opp-supported-hw has a first value >> masked by BIT(2) return non-0 is supported by RK3588J. >> >> This means that, for LITTLE clusters: >> - opp-1608000000 not supported on RK3588J >> - opp-1704000000 only supported on RK3588M (but already absent in this >> patch series) >> - opp-1800000000 only supported on RK3588(S), not RK3588J nor RK3588M >> >> For big clusters: >> - opp-1800000000 not supported on RK3588J >> - opp-2016000000 not supported on RK3588J >> - opp-2208000000 only supported on RK3588(S), not RK3588J nor RK3588M >> - opp-2256000000 only supported on RK3588(S), not RK3588J nor RK3588M >> - opp-2304000000 only supported on RK3588(S), not RK3588J nor RK3588M >> - opp-2352000000 only supported on RK3588(S), not RK3588J nor RK3588M >> - opp-2400000000 only supported on RK3588(S), not RK3588J nor RK3588M >> >> This is somehow also enforced in downstream kernel by removing the OPP >> nodes directly (hence, not even requiring the check of >> opp-supported-hw >> value), c.f.: >> https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588j.dtsi >> https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588m.dtsi >> >> You'll not that the RK3588J also has less OPPs for the GPU and NPU >> (but >> those should also be masked by the opp-supported-hw value). > > Same with DMC, but we don't currently have either DMC or NPU in the > mainline tree, so it sounds like something to be dealt with later :) > > [1] > https://github.com/rockchip-linux/kernel/blob/develop-5.10/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
On 2024-05-29 02:35, Dragan Simic wrote: > On 2024-05-28 21:26, Alexey Charkov wrote: >> On Tue, May 28, 2024 at 8:08 PM Quentin Schulz >> <quentin.schulz@cherry.de> wrote: >>> On 5/28/24 5:42 PM, Alexey Charkov wrote: >>> > On Tue, 28 May 2024 at 19:16, Heiko Stuebner <heiko@sntech.de> wrote: >>> >> Am Dienstag, 28. Mai 2024, 17:01:48 CEST schrieb Dragan Simic: >>> >>> On 2024-05-28 16:34, Heiko Stuebner wrote: >>> >>>> Am Dienstag, 28. Mai 2024, 16:05:04 CEST schrieb Dragan Simic: >>> >>>>> On 2024-05-28 11:49, Alexey Charkov wrote: >>> >>>>>> Hi Heiko, >>> >>>>>> >>> >>>>>> Do you think this can be merged for 6.11? Looks like there hasn't >>> >> been >>> >>>>>> any new feedback in a while, and it would be good to have frequency >>> >>>>>> scaling in place for RK3588. >>> >>>>>> >>> >>>>>> Please let me know if you have any reservations or if we need any >>> >>>>>> broader discussion. >>> >>>> >>> >>>> not really reservations, more like there was still discussion going on >>> >>>> around the OPPs. Meanwhile we had more discussions regarding the whole >>> >>>> speed binning Rockchip seems to do for rk3588 variants. >>> >>>> >>> >>>> And waiting for the testing Dragan wanted to do ;-) . >>> >>> >>> >>> I'm sorry for the delays. >>> >> >>> >> Was definitly _not_ meant as blame ;-) . >>> >> >>> >> The series has just too many discussions threads to unravel on half >>> >> an afternoon. >>> > >>> > >>> > FWIW, I think the latest exchange we had with Quentin regarding the OPPs >>> > concluded in “false alarm”, given that this version of the series only >>> > introduces a subset of them which should apply to all RK3588(s) >>> >>> Correct. >>> >>> However... I'm wondering if we shouldn't somehow follow the same >>> pattern >>> we have used for the rk3399 OPPs? We have a file for the "true" >>> RK3399 >>> OPPs, then the OP1 variant and the RK3399T. >>> >>> We already know there are a few variants of RK3588 with different >>> OPPs: >>> RK3588(S/S2?), RK3588J and RK3588M. I wouldn't be surprised if the >>> RK3582 (though this one has already one big cluster (or two big >>> cores) >>> fewer than RK3588) has different OPPs as well? >>> >>> So. We have already discussed that the OPPs in that patch are valid >>> for >>> RK3588(S) but they aren't for the other variants. >> >> Hmm. Looking at Rockchip sources [1] more closely as opposed to the >> Radxa tree I've been using as the basis, it does indeed show that >> RK3588J and RK3588M use a different OPP table altogether (frequencies >> are similar, but voltages differ). >> >> We currently have an RK3588J based board in the mainline tree >> (rk3588-edgeble-neu6b-io.dts), so it can't be ignored. However, given >> that Rockchip sources only differentiate those OPPs by SoC revision, >> and that is (currently?) known for each board at dtb compile time, it >> seems easier to just include two different OPP tables for RK3588(S) >> vs. RK3588J - thus avoiding all the headache with opp-supported-hw. >> Similar to RK3399, yes. >> >> Will split those out and send a separate version. > > Ah, you already answered my question about the existence of supported > boards with the RK3588J and RK3588M variants. :) [2] I totally agree > about splitting the OPPs into the separate .dtsi files, i.e. following > the approach already established by the RK3399. > > Perhaps the new files should be named rk3588-opp.dtsi and > rk3588j-opp.dtsi, > to (almost fully) follow the already established naming scheme. Maybe > the OPP data could instead be added to the already existing > rk3588.dtsi, > rk3588s.dtsi and rk3588j.dtsi files, which would actually make more > sense > because those .dtsi files already address the specific SoC variants, > but > the approach with the separate new rk3588*-opp.dtsi files seems cleaner > from the implementation point of view, and is a bit safer. > > If you see a clean way for stuffing the OPP data into the already > existing > rk3588.dtsi, rk3588s.dtsi and rk3588j.dtsi files, instead of > introducing > new rk3588*-opp.dtsi files, I'd be happy to support it. > > [2] > https://lore.kernel.org/linux-rockchip/7b09e18e850ff0832bd7236810b83e64@manjaro.org/ On second thought, it makes more sense to rename and shuffle the RK3588 dtsi files a bit, to make it possible to have the OPPs already specified when a board dts file includes the dtsi file for a RK3588 variant. Thus, I went ahead and prepared an RFC patch that does exactly that. [3] Please, have a look at that RFC patch. To quote its description: --------------------------------------------------------------------------- Rename and modify the RK3588 dtsi files a bit, to make preparations for the ability to specify different CPU and GPU OPPs for each of the supported RK3588 SoC variants, including the RK3588J. As already discussed, some of the different RK3588 SoC variants require different OPPs, and it makes more sense to have the OPPs already defined when a board dts includes one of the SoC dtsi files (rk3588.dtsi, rk3588j.dtsi or rk3588s.dtsi), rather than requiring the board dts file to also include a separate rk3588*-opp.dtsi file. The choice of the SoC variant is already made by the inclusion of the SoC dtsi file, and it doesn't make much sense to, effectively, allow the board dts file to include and use an incompatible set of OPPs for the already selected SoC variant. No intended functional changes are introduced. (Still to be additionally verified if the patch moves forward from the RFC state.) --------------------------------------------------------------------------- [3] https://lore.kernel.org/linux-rockchip/673dcf47596e7bc8ba065034e339bb1bbf9cdcb0.1716948159.git.dsimic@manjaro.org/ >>> In the downstream kernel, any OPP whose opp-supported-hw has a first >>> value masked by BIT(1) return non-0 is supported by RK3588M. In the >>> downstream kernel, any OPP whose opp-supported-hw has a first value >>> masked by BIT(2) return non-0 is supported by RK3588J. >>> >>> This means that, for LITTLE clusters: >>> - opp-1608000000 not supported on RK3588J >>> - opp-1704000000 only supported on RK3588M (but already absent in >>> this >>> patch series) >>> - opp-1800000000 only supported on RK3588(S), not RK3588J nor RK3588M >>> >>> For big clusters: >>> - opp-1800000000 not supported on RK3588J >>> - opp-2016000000 not supported on RK3588J >>> - opp-2208000000 only supported on RK3588(S), not RK3588J nor RK3588M >>> - opp-2256000000 only supported on RK3588(S), not RK3588J nor RK3588M >>> - opp-2304000000 only supported on RK3588(S), not RK3588J nor RK3588M >>> - opp-2352000000 only supported on RK3588(S), not RK3588J nor RK3588M >>> - opp-2400000000 only supported on RK3588(S), not RK3588J nor RK3588M >>> >>> This is somehow also enforced in downstream kernel by removing the >>> OPP >>> nodes directly (hence, not even requiring the check of >>> opp-supported-hw >>> value), c.f.: >>> https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588j.dtsi >>> https://git.theobroma-systems.com/tiger-linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588m.dtsi >>> >>> You'll not that the RK3588J also has less OPPs for the GPU and NPU >>> (but >>> those should also be masked by the opp-supported-hw value). >> >> Same with DMC, but we don't currently have either DMC or NPU in the >> mainline tree, so it sounds like something to be dealt with later :) >> >> [1] >> https://github.com/rockchip-linux/kernel/blob/develop-5.10/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
This enables thermal monitoring and CPU DVFS on RK3588(s), as well as active cooling on Radxa Rock 5B via the provided PWM fan. Some RK3588 boards use separate regulators to supply CPUs and their respective memory interfaces, so this is handled by coupling those regulators in affected boards' device trees to ensure that their voltage is adjusted in step. This also enables the built-in thermal sensor (TSADC) for all boards that don't currently have it enabled, using the default CRU based emergency thermal reset. This default configuration only uses on-SoC devices and doesn't rely on any external wiring, thus it should work for all devices (tested only on Rock 5B though). The boards that have TSADC_SHUT signal wired to the PMIC reset line can choose to override the default reset logic in favour of GPIO driven (PMIC assisted) reset, but in my testing it didn't work on Radxa Rock 5B - maybe I'm reading the schematic wrong and it doesn't support PMIC assisted reset after all. Fan control on Rock 5B has been split into two intervals: let it spin at the minimum cooling state between 55C and 65C, and then accelerate if the system crosses the 65C mark - thanks to Dragan for suggesting. This lets some cooling setups with beefier heatsinks and/or larger fan fins to stay in the quietest non-zero fan state while still gaining potential benefits from the airflow it generates, and possibly avoiding noisy speeds altogether for some workloads. OPPs help actually scale CPU frequencies up and down for both cooling and performance - tested on Rock 5B under varied loads. I've dropped those OPPs that cause frequency reductions without accompanying decrease in CPU voltage, as they don't seem to be adding much benefit in day to day use, while the kernel log gets a number of "OPP is inefficient" lines. Note that this submission doesn't touch the SRAM read margin updates or the OPP calibration based on silicon quality which the downstream driver does and which were mentioned in [1]. It works as it is (also confirmed by Sebastian in his follow-up message [2]), and it is stable in my testing on Rock 5B, so it sounds better to merge a simple version first and then extend when/if required. [1] https://lore.kernel.org/linux-rockchip/CABjd4YzTL=5S7cS8ACNAYVa730WA3iGd5L_wP1Vn9=f83RCORA@mail.gmail.com/ [2] https://lore.kernel.org/linux-rockchip/pkyne4g2cln27dcdu3jm7bqdqpmd2kwkbguiolmozntjuiajrb@gvq4nupzna4o/ Signed-off-by: Alexey Charkov <alchark@gmail.com> --- Changes in v4: - Rebased against linux-rockchip/for-next - Reordered DT nodes alphabetically as pointed out by Diederik - Moved the TSADC enablement to per-board .dts/.dtsi files - Dropped extra "inefficient" OPPs (same voltage - lower frequencies) - Dropped second passive cooling trips altogether to keep things simple - Added a cooling map for passive GPU cooling (in a separate patch) - Link to v3: https://lore.kernel.org/r/20240229-rk-dts-additions-v3-0-6afe8473a631@gmail.com Changes in v3: - Added regulator coupling for EVB1 and QuartzPro64 - Enabled the TSADC for all boards in .dtsi, not just Rock 5B (thanks ChenYu) - Added comments regarding two passive cooling trips in each zone (thanks Dragan) - Fixed active cooling map numbering for Radxa Rock 5B (thanks Dragan) - Dropped Daniel's Acked-by tag from the Rock 5B fan patch, as there's been quite some churn there since the version he acknowledged - Link to v2: https://lore.kernel.org/r/20240130-rk-dts-additions-v2-0-c6222c4c78df@gmail.com Changes in v2: - Dropped the rfkill patch which Heiko has already applied - Set higher 'polling-delay-passive' (100 instead of 20) - Name all cooling maps starting from map0 in each respective zone - Drop 'contribution' properties from passive cooling maps - Link to v1: https://lore.kernel.org/r/20240125-rk-dts-additions-v1-0-5879275db36f@gmail.com --- Alexey Charkov (6): arm64: dts: rockchip: add thermal zones information on RK3588 arm64: dts: rockchip: enable thermal management on all RK3588 boards arm64: dts: rockchip: add passive GPU cooling on RK3588 arm64: dts: rockchip: enable automatic fan control on Rock 5B arm64: dts: rockchip: Add CPU/memory regulator coupling for RK3588 arm64: dts: rockchip: Add OPP data for CPU cores on RK3588 .../boot/dts/rockchip/rk3588-armsom-sige7.dts | 4 + .../dts/rockchip/rk3588-edgeble-neu6a-common.dtsi | 4 + arch/arm64/boot/dts/rockchip/rk3588-evb1-v10.dts | 16 ++ arch/arm64/boot/dts/rockchip/rk3588-ok3588-c.dts | 4 + .../arm64/boot/dts/rockchip/rk3588-quartzpro64.dts | 12 + arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34 ++- .../arm64/boot/dts/rockchip/rk3588-toybrick-x0.dts | 4 + .../arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi | 4 + arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts | 4 + arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 281 +++++++++++++++++++++ 10 files changed, 366 insertions(+), 1 deletion(-) --- base-commit: 160b088184ec81028ff67a5032be33f1baea4b67 change-id: 20240124-rk-dts-additions-a6d7b52787b9 Best regards,