Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpufreq: Implement cpufreq support for Raspberry Pi #32

Open
lategoodbye opened this Issue May 23, 2018 · 50 comments

Comments

Projects
None yet
4 participants
@lategoodbye
Copy link
Owner

commented May 23, 2018

In a recent discuss someone requested for cpufreq support.

There are at least two options:

  1. implement a bogus clock driver and use cpufreq-dt (OPP version)
  2. implement a separate cpufreq-bcm2835 (non-OPP version)

Relevant discussions / patches:

Resulting TODOs:

@sergey-suloev

This comment has been minimized.

Copy link

commented May 23, 2018

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented May 23, 2018

Unfortunately me too, even after implementing recalc_rate with RPI_FIRMWARE_GET_CLOCK_MEASURED.

@nullr0ute

This comment has been minimized.

Copy link

commented Jun 4, 2018

By just catting /sys/devices/system/cpu/cpufreq/policy0/scaling_cur_freq in brief testing I've seen it touch on 900 on it's way between 600/1200.

The patch looks to be just for the original RPi3, is there just more DT bits required for the 2/3+ (and presumably the various 0/1 devices)?

@nullr0ute

This comment has been minimized.

Copy link

commented Jun 4, 2018

[ 66.251546] cpu cpu0: dev_pm_opp_set_rate: failed to find current OPP for freq 1400000000 (-34)

@nullr0ute

This comment has been minimized.

Copy link

commented Jun 4, 2018

On the 3+
[ 66.251546] cpu cpu0: dev_pm_opp_set_rate: failed to find current OPP for freq 1400000000 (-34)

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Jun 4, 2018

This isn't a topic which isn't fully matured yet. So i'm not interested in pushing a quick shot for 4.19.

Currently i'm not sure which approach is better. The DT has the disadvantage that we need to model the OPPs on board level instead of SoC. That's why i started with RPI 3.

@nullr0ute

This comment has been minimized.

Copy link

commented Jun 4, 2018

I realise, I'm just playing to see if it's something that would be usable in the interim across the devices I need to support.

0001-add-1.4-ghz-OPP-for-the-3B.patch.txt

So this appears to work on the 3B+

cat /sys/devices/system/cpu/cpufreq/policy0/scaling_available

600000 900000 1200000 1400000

No idea if the 3B+ supports 1.2 as well as 1.4 though. I suppose similar would work for the other devices

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Jun 4, 2018

AFAIK firmware supports arbitrary values. So this should be okay.

But this was only the easy part, because all driver connected to the VPU clock must handle clock changes now.

@nullr0ute

This comment has been minimized.

Copy link

commented Jun 4, 2018

I have no doubts it was the easy part, is there details of which drivers are affected?

The best I've managed to find on the freqs supported seems to be this section in the wikipedia docs:
https://en.wikipedia.org/wiki/Raspberry_Pi#Overclocking

@sergey-suloev

This comment has been minimized.

Copy link

commented Jun 4, 2018

If every driver needed to take CPU clock change into account that would be a hell. I am not sure how it is implemented in Rpi but normally it should be handled in only one driver, like ccu. Correct me if I am wrong.

@sergey-suloev

This comment has been minimized.

Copy link

commented Jun 4, 2018

@lategoodbye And what about voltages ? Usually OPP table contains freq + voltage, does Rpi need voltage change ?

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Jun 4, 2018

Please look at all driver which uses BCM2835_CLOCK_VPU for the notification changes:
https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/bcm283x.dtsi

The VC4 firmware takes care of the voltages.

@sergey-suloev

This comment has been minimized.

Copy link

commented Jun 4, 2018

@lategoodbye Is there a way to change(or create) only one , let's cal it "hub driver", and what would it cost ?
Maybe it is [compatible = "brcm,bcm2835-cprman"] ?

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Jun 4, 2018

Sorry, i don't understand question / intension?

@sergey-suloev

This comment has been minimized.

Copy link

commented Jun 4, 2018

@lategoodbye my point is that changing every driver in order to integrate OPP table doesn't make too much sense. In other words, it is stupid.
My question : can you imagine any way that can allow make such changes in only one, "hub", or managing driver, i.e. ccu.
I may be missing something, just want to understand. By "cost" I mean how much development effort it would take.

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Jun 4, 2018

I think there is a misunderstanding. You don't need to implement OPP table handling in every driver. But a lot of driver uses the VPU as clock input with its clock divider. So in case the VPU clock changes the clock divider should be adjusted, too. Otherwise the resulting frequency is wrong. Usually the divider settings are claimed by the relevant driver, so in case of a VPU clock change is makes sense to handle it by the relevant driver.

@nullr0ute

This comment has been minimized.

Copy link

commented Jun 4, 2018

@lategoodbye so that was my understanding around the drivers understanding that a clock would possibly change under them as opposed to being static. My query was is can we grep the DT or a string in drivers for which upstream drivers might be affected to work out what might be affected?

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Jun 4, 2018

As i wrote above grep for BCM2835_CLOCK_VPU within the DT.

@sergey-suloev

This comment has been minimized.

Copy link

commented Jun 4, 2018

@lategoodbye That's what I am talking about too. Can we have a special driver that will provide stable clocks for those drivers no matter if CPU clock changes or not?

@nullr0ute

This comment has been minimized.

Copy link

commented Aug 31, 2018

So having been running this for a little bit in Fedora one thing to note here when running something that I've noticed when running something that taxes the gpu too such as gnome on wayland we get shutdowns/lockups/poweroffs. I need to investigate more and get more debug.

The end driver might want to tie into the thermal framework and possibly the hwmon for undervoltage and use those for scaling decisions too.

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Aug 31, 2018

@nullr0ute Do you mean Sergey's patch?
Which governor did you use?

@nullr0ute

This comment has been minimized.

Copy link

commented Aug 31, 2018

Yes, Sergey's patch, and the ondemand (Fedora default) govenor

@sergey-suloev

This comment has been minimized.

Copy link

commented Sep 2, 2018

@nullr0ute Can it be due to your power supply limits ? I personally haven't tried the patch with GUI tasks but I haven't had any issues with some heavy tasks in bash.

@nullr0ute

This comment has been minimized.

Copy link

commented Sep 2, 2018

@sergey-suloev I don't believe so, it's a good quality Anker PSU, I will be investigating more this week. I think it's more thermal than power TBH but I wasn't remotely connected in to see any logs at the time. The RPi3 doesn't have a heatsink on it.

@nullr0ute

This comment has been minimized.

Copy link

commented Sep 4, 2018

For reference with more testing, it's thermal, I get the following:
kernel: thermal thermal_zone0: critical temperature reached (80 C), shutting down

lategoodbye pushed a commit that referenced this issue Sep 19, 2018

arm/arm64: smccc-1.1: Handle function result as parameters
If someone has the silly idea to write something along those lines:

	extern u64 foo(void);

	void bar(struct arm_smccc_res *res)
	{
		arm_smccc_1_1_smc(0xbad, foo(), res);
	}

they are in for a surprise, as this gets compiled as:

	0000000000000588 <bar>:
	 588:   a9be7bfd        stp     x29, x30, [sp, #-32]!
	 58c:   910003fd        mov     x29, sp
	 590:   f9000bf3        str     x19, [sp, #16]
	 594:   aa0003f3        mov     x19, x0
	 598:   aa1e03e0        mov     x0, x30
	 59c:   94000000        bl      0 <_mcount>
	 5a0:   94000000        bl      0 <foo>
	 5a4:   aa0003e1        mov     x1, x0
	 5a8:   d4000003        smc     #0x0
	 5ac:   b4000073        cbz     x19, 5b8 <bar+0x30>
	 5b0:   a9000660        stp     x0, x1, [x19]
	 5b4:   a9010e62        stp     x2, x3, [x19, #16]
	 5b8:   f9400bf3        ldr     x19, [sp, #16]
	 5bc:   a8c27bfd        ldp     x29, x30, [sp], #32
	 5c0:   d65f03c0        ret
	 5c4:   d503201f        nop

The call to foo "overwrites" the x0 register for the return value,
and we end up calling the wrong secure service.

A solution is to evaluate all the parameters before assigning
anything to specific registers, leading to the expected result:

	0000000000000588 <bar>:
	 588:   a9be7bfd        stp     x29, x30, [sp, #-32]!
	 58c:   910003fd        mov     x29, sp
	 590:   f9000bf3        str     x19, [sp, #16]
	 594:   aa0003f3        mov     x19, x0
	 598:   aa1e03e0        mov     x0, x30
	 59c:   94000000        bl      0 <_mcount>
	 5a0:   94000000        bl      0 <foo>
	 5a4:   aa0003e1        mov     x1, x0
	 5a8:   d28175a0        mov     x0, #0xbad
	 5ac:   d4000003        smc     #0x0
	 5b0:   b4000073        cbz     x19, 5bc <bar+0x34>
	 5b4:   a9000660        stp     x0, x1, [x19]
	 5b8:   a9010e62        stp     x2, x3, [x19, #16]
	 5bc:   f9400bf3        ldr     x19, [sp, #16]
	 5c0:   a8c27bfd        ldp     x29, x30, [sp], #32
	 5c4:   d65f03c0        ret

Reported-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Sep 19, 2018

I want to point out to this commit 0fe4d21

In the lack of a cooling device this was left empty. But with a cpufreq driver we have such a cooling device.

@sergey-suloev

This comment has been minimized.

Copy link

commented Sep 19, 2018

@lategoodbye could you provide the code that should be added, please ?

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Sep 19, 2018

It's not code, it's device tree settings. Yes, later

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Sep 26, 2018

Based on Peter's cpufreq patch i made a patch to use CPU0 of RPI 3 as cooling device.

Disclaimer: Only compile tested

@nullr0ute

This comment has been minimized.

Copy link

commented Sep 26, 2018

I'll give it a test run given I have a RPi I can regularly thermally trip :)

@sergey-suloev

This comment has been minimized.

Copy link

commented Sep 26, 2018

@lategoodbye thanks a lot

@nullr0ute

This comment has been minimized.

Copy link

commented Sep 26, 2018

Initial feedback, the cooling device shows up tmon and P trip point is there. Will test the actual trips shortly

@nullr0ute

This comment has been minimized.

Copy link

commented Sep 26, 2018

BIG 👍 It seems to work pretty well. I've not managed to have my original RPi3 shut down due to thermal. Remotely running tmon and following the current freq in /sys/devices/system/cpu/cpufreq/policy0/ I've not managed to trip thermal in basic testing. It has got above the 70 P but managed to keep it well below Critical 80 and seems to work pretty well 👍

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Sep 27, 2018

Thanks for testing. Maybe we should add a hysteresis of 2 degree.

@sergey-suloev

This comment has been minimized.

Copy link

commented Sep 27, 2018

@lategoodbye Stefan, is it possible to make Rpi3 work on 900MHz ?

@nullr0ute

This comment has been minimized.

Copy link

commented Sep 27, 2018

@sergey-suloev in my testing I see it running at 900mhz on occasion, it moves between 600/900/1200

@sergey-suloev

This comment has been minimized.

Copy link

commented Sep 27, 2018

@nullr0ute I can't make it work on 900 when I set manually with userspace gov. The real freq is always 1200.

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Sep 27, 2018

I think the 900 MHz which Peter is seeing is only internal in the cpufreq driver.

Edit: So i'm afraid that there is only minimum and maximum possible.

@nullr0ute

This comment has been minimized.

Copy link

commented Sep 27, 2018

I'm looking in /sys/devices/system/cpu/cpufreq/policy0/ is there other places that are more accurate?

@sergey-suloev

This comment has been minimized.

Copy link

commented Sep 27, 2018

@nullr0ute can you make it 900 manually ?

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Sep 27, 2018

@nullr0ute
AFAIK there is no chance to fetch actual core frequency via mailbox.

The more accurate way should be:
vcgencmd measure_clock arm

@sergey-suloev

This comment has been minimized.

Copy link

commented Sep 27, 2018

no matter how you fetch it, on my Rpi3 it is always 1200 after I set it manually to 900

@nullr0ute

This comment has been minimized.

Copy link

commented Sep 27, 2018

@lategoodbye unfortunately (last I looked at least) that tool isn't open source and doesn't work on aarch64 either

@sergey-suloev

This comment has been minimized.

Copy link

commented Sep 27, 2018

@nullr0ute it is open source and called userland
https://github.com/raspberrypi/userland

but there are definitely some issues with aarch64

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Oct 13, 2018

@nullr0ute Recently i tested Fedora Workstation 29 Beta 1.5 on my RPi 3 B+. The whole system periodically freeze completely (no USB power on keyboard, no reaction on screen). After blacklisting cpufreq_dt the system becomes usable (including debug UART). Just a warning cherry-picking just this cpufreq patch is very dangerous and can leads to data loss e.g. on sdhost interface.

@ggardet

This comment has been minimized.

Copy link

commented Mar 14, 2019

What is the current status? Is there any upstream effort on going?

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Mar 14, 2019

Currently not. The first step would be to adapt the relevant driver (sdhost, i2c, aux, spi, dpi) for dynamic clock changes.

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Mar 23, 2019

Okay, here is a untested patch to dynamically adjust AUX UART divisor:
https://gist.github.com/lategoodbye/5f4eea59e82c310519adf76dc095fe2e

Be aware this currently wont work with the implementation of clk-bcm2835, because it doesn't take VPU clock changes / Turbo mode into account.

@nullr0ute

This comment has been minimized.

Copy link

commented Mar 25, 2019

I wonder if the OPP framework is one way to achieve this, seems this series for qcom is similar to what needs to be achieved.

https://lists.freedesktop.org/archives/dri-devel/2019-March/211321.html

@lategoodbye

This comment has been minimized.

Copy link
Owner Author

commented Apr 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.