| Message ID | 1480645653-36943-9-git-send-email-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Brian, Am Donnerstag, 1. Dezember 2016, 18:27:32 CET schrieb Brian Norris: > We need to add regulators to the CPU nodes, so cpufreq doesn't think it > can crank up the clock speed without changing the voltage. However, we > don't yet have the DT bindings to fully describe the Over Voltage > Protection (OVP) circuits on these boards. Without that description, we > might end up changing the voltage too much, too fast. > > Add the pwm-regulator descriptions and associate the CPU OPPs, but leave > them disabled. > > Signed-off-by: Brian Norris <briannorris@chromium.org> is there a specific reason for keeping this change separate? While it is nice for documentation reasons, as it stands now the previous patch introduces a regression (cpufreq trying to scale without regulators) and immediately fixes it here. So if you're ok with it, I'd like to merge this one back into the previous patch when applying. Heiko
Hi Heiko, On Wed, Dec 07, 2016 at 05:48:24PM +0100, Heiko Stuebner wrote: > Am Donnerstag, 1. Dezember 2016, 18:27:32 CET schrieb Brian Norris: > > We need to add regulators to the CPU nodes, so cpufreq doesn't think it > > can crank up the clock speed without changing the voltage. However, we > > don't yet have the DT bindings to fully describe the Over Voltage > > Protection (OVP) circuits on these boards. Without that description, we > > might end up changing the voltage too much, too fast. > > > > Add the pwm-regulator descriptions and associate the CPU OPPs, but leave > > them disabled. > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > is there a specific reason for keeping this change separate? Maybe not a great one. I figured they were somewhat controversial, so I at least wanted to split the "cpufreq patches" (i.e., this and the previous) from the main DTS(I) additions. I also figured we typically like to keep the base SoC changes separate from the board DTS(I) changes. > While it is nice for documentation reasons, as it stands now the previous > patch introduces a regression (cpufreq trying to scale without regulators) and > immediately fixes it here. Right. Additionally, as noted on the previous patch, we might do the same with EVB. But I don't know what the regulators are like for EVB. This is probably a bigger deal, since EVB has been working (allegedly) upstream for a while now. There's no way to split these up without either breaking compilation or breaking bisectability. For Kevin/Gru, they don't function at all before this series, so I figured some "settle" time wasn't a huge deal. > So if you're ok with it, I'd like to merge this one back into the previous > patch when applying. That'd be OK with me, as long as we're also confident about EVB. Maybe at a minimum, I should just patch in some empty regulator nodes, so cpufreq doesn't think there's no need to handle voltage. Brian
Am Mittwoch, 7. Dezember 2016, 09:09:17 CET schrieb Brian Norris: > Hi Heiko, > > On Wed, Dec 07, 2016 at 05:48:24PM +0100, Heiko Stuebner wrote: > > Am Donnerstag, 1. Dezember 2016, 18:27:32 CET schrieb Brian Norris: > > > We need to add regulators to the CPU nodes, so cpufreq doesn't think it > > > can crank up the clock speed without changing the voltage. However, we > > > don't yet have the DT bindings to fully describe the Over Voltage > > > Protection (OVP) circuits on these boards. Without that description, we > > > might end up changing the voltage too much, too fast. > > > > > > Add the pwm-regulator descriptions and associate the CPU OPPs, but leave > > > them disabled. > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > > is there a specific reason for keeping this change separate? > > Maybe not a great one. I figured they were somewhat controversial, so I > at least wanted to split the "cpufreq patches" (i.e., this and the > previous) from the main DTS(I) additions. I also figured we typically > like to keep the base SoC changes separate from the board DTS(I) > changes. I was scratching my head for a bit where this was affecting the evb, until I found the include at the end of patch5 :-) . > > While it is nice for documentation reasons, as it stands now the previous > > patch introduces a regression (cpufreq trying to scale without regulators) > > and immediately fixes it here. > > Right. Additionally, as noted on the previous patch, we might do the > same with EVB. But I don't know what the regulators are like for EVB. > This is probably a bigger deal, since EVB has been working (allegedly) > upstream for a while now. Yep, it was at least booting :-) . I guess I should wire it up again. My shiny new Gru somehow did take up its space recently. > There's no way to split these up without either breaking compilation or > breaking bisectability. For Kevin/Gru, they don't function at all before > this series, so I figured some "settle" time wasn't a huge deal. > > > So if you're ok with it, I'd like to merge this one back into the previous > > patch when applying. > > That'd be OK with me, as long as we're also confident about EVB. That somehow sounds unrelated, as this patch only touches gru stuff anyway. So if the evb breaks, it would do so after patch5 already. > Maybe at a minimum, I should just patch in some empty regulator nodes, > so cpufreq doesn't think there's no need to handle voltage. So I guess going forward we could do, describe the evb pwm regulators (in disabled state), add general OPPs, add gru with pwm regulators? I'll try to hook up my evb and check on the pwm-regulators in the schematics this week. Heiko
Am Dienstag, 13. Dezember 2016, 18:48:50 schrieb Heiko Stuebner: > Am Mittwoch, 7. Dezember 2016, 09:09:17 CET schrieb Brian Norris: > > Hi Heiko, > > > > On Wed, Dec 07, 2016 at 05:48:24PM +0100, Heiko Stuebner wrote: > > > Am Donnerstag, 1. Dezember 2016, 18:27:32 CET schrieb Brian Norris: > > > > We need to add regulators to the CPU nodes, so cpufreq doesn't think > > > > it > > > > can crank up the clock speed without changing the voltage. However, we > > > > don't yet have the DT bindings to fully describe the Over Voltage > > > > Protection (OVP) circuits on these boards. Without that description, > > > > we > > > > might end up changing the voltage too much, too fast. > > > > > > > > Add the pwm-regulator descriptions and associate the CPU OPPs, but > > > > leave > > > > them disabled. > > > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > > > > is there a specific reason for keeping this change separate? > > > > Maybe not a great one. I figured they were somewhat controversial, so I > > at least wanted to split the "cpufreq patches" (i.e., this and the > > previous) from the main DTS(I) additions. I also figured we typically > > like to keep the base SoC changes separate from the board DTS(I) > > changes. > > I was scratching my head for a bit where this was affecting the evb, until I > found the include at the end of patch5 :-) . > > > > While it is nice for documentation reasons, as it stands now the > > > previous > > > patch introduces a regression (cpufreq trying to scale without > > > regulators) > > > and immediately fixes it here. > > > > Right. Additionally, as noted on the previous patch, we might do the > > same with EVB. But I don't know what the regulators are like for EVB. > > This is probably a bigger deal, since EVB has been working (allegedly) > > upstream for a while now. > > Yep, it was at least booting :-) . I guess I should wire it up again. My > shiny new Gru somehow did take up its space recently. > > > There's no way to split these up without either breaking compilation or > > breaking bisectability. For Kevin/Gru, they don't function at all before > > this series, so I figured some "settle" time wasn't a huge deal. > > > > > So if you're ok with it, I'd like to merge this one back into the > > > previous > > > patch when applying. > > > > That'd be OK with me, as long as we're also confident about EVB. > > That somehow sounds unrelated, as this patch only touches gru stuff anyway. > So if the evb breaks, it would do so after patch5 already. > > > Maybe at a minimum, I should just patch in some empty regulator nodes, > > so cpufreq doesn't think there's no need to handle voltage. > > So I guess going forward we could do, describe the evb pwm regulators (in > disabled state), add general OPPs, add gru with pwm regulators? > > I'll try to hook up my evb and check on the pwm-regulators in the schematics > this week. Now I remember why I retired my evb ... ES1 silicon. Anyway, I was able to describe the rk808 pmic that is used in some basic way and was able to see a bit of cpu-scaling on the little cluster, but the big cluster never was able to scale in a stable way and always hung the system. I don't think that we should care about ES1 chips as they never reached any public but that leaves me without the ability to test. In another issue I read that Caesar also is using a rk3399evb sometimes, so maybe he can give that a try on real ES2 silicon and I've thus Cc'ed him. @Caesar I don't know how much the power rails differ between evb versions, so maybe you can also provide the necessary rk808 devicetree nodes please? Thanks Heiko
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi index 59b452504106..90adfb5cba38 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi @@ -172,6 +172,98 @@ vin-supply = <&ppvar_sys>; }; + ppvar_bigcpu: ppvar-bigcpu { + compatible = "pwm-regulator"; + regulator-name = "ppvar_bigcpu"; + /* + * OVP circuit requires special handling which is not yet + * represented. Keep disabled for now. + */ + status = "disabled"; + + pwms = <&pwm1 0 3337 0>; + + /* EC turns on w/ ap_core_en; always on for AP */ + regulator-always-on; + regulator-boot-on; + + regulator-min-microvolt = <798674>; + regulator-max-microvolt = <1302172>; + + pwm-supply = <&ppvar_sys>; + pwm-dutycycle-range = <100 0>; + pwm-dutycycle-unit = <100>; + }; + + ppvar_litcpu: ppvar-litcpu { + compatible = "pwm-regulator"; + regulator-name = "ppvar_litcpu"; + /* + * OVP circuit requires special handling which is not yet + * represented. Keep disabled for now. + */ + status = "disabled"; + + pwms = <&pwm2 0 3337 0>; + + /* EC turns on w/ ap_core_en; always on for AP */ + regulator-always-on; + regulator-boot-on; + + regulator-min-microvolt = <799065>; + regulator-max-microvolt = <1303738>; + + pwm-supply = <&ppvar_sys>; + pwm-dutycycle-range = <100 0>; + pwm-dutycycle-unit = <100>; + }; + + ppvar_gpu: ppvar-gpu { + compatible = "pwm-regulator"; + regulator-name = "ppvar_gpu"; + /* + * OVP circuit requires special handling which is not yet + * represented. Keep disabled for now. + */ + status = "disabled"; + + pwms = <&pwm0 0 3337 0>; + + /* EC turns on w/ ap_core_en; always on for AP */ + regulator-always-on; + regulator-boot-on; + + regulator-min-microvolt = <785782>; + regulator-max-microvolt = <1217729>; + + pwm-supply = <&ppvar_sys>; + pwm-dutycycle-range = <100 0>; + pwm-dutycycle-unit = <100>; + }; + + ppvar_centerlogic: ppvar-centerlogic { + compatible = "pwm-regulator"; + regulator-name = "ppvar_centerlogic"; + /* + * OVP circuit requires special handling which is not yet + * represented. Keep disabled for now. + */ + status = "disabled"; + + pwms = <&pwm3 0 3337 0>; + + /* EC turns on w/ ppvar_centerlogic_en; always on for AP */ + regulator-always-on; + regulator-boot-on; + + regulator-min-microvolt = <800069>; + regulator-max-microvolt = <1049692>; + + pwm-supply = <&ppvar_sys>; + pwm-dutycycle-range = <100 0>; + pwm-dutycycle-unit = <100>; + }; + /* Schematics call this PPVAR even though it's fixed */ ppvar_logic: ppvar-logic { compatible = "regulator-fixed"; @@ -444,6 +536,60 @@ }; }; +/* + * Set some suspend operating points to avoid OVP in suspend + * + * When we go into S3 ARM Trusted Firmware will transition our PWM regulators + * from wherever they're at back to the "default" operating point (whatever + * voltage we get when we set the PWM pins to "input"). + * + * This quick transition under light load has the possibility to trigger the + * regulator "over voltage protection" (OVP). + * + * To make extra certain that we don't hit this OVP at suspend time, we'll + * transition to a voltage that's much closer to the default (~1.0 V) so that + * there will not be a big jump. Technically we only need to get within 200 mV + * of the default voltage, but the speed here should be fast enough and we need + * suspend/resume to be rock solid. + */ + +&cluster0_opp { + opp05 { + opp-suspend; + }; +}; + +&cluster1_opp { + opp06 { + opp-suspend; + }; +}; + +&cpu_l0 { + cpu-supply = <&ppvar_litcpu>; +}; + +&cpu_l1 { + cpu-supply = <&ppvar_litcpu>; +}; + +&cpu_l2 { + cpu-supply = <&ppvar_litcpu>; +}; + +&cpu_l3 { + cpu-supply = <&ppvar_litcpu>; +}; + +&cpu_b0 { + cpu-supply = <&ppvar_bigcpu>; +}; + +&cpu_b1 { + cpu-supply = <&ppvar_bigcpu>; +}; + + &cru { assigned-clocks = <&cru PLL_GPLL>, <&cru PLL_CPLL>,
We need to add regulators to the CPU nodes, so cpufreq doesn't think it can crank up the clock speed without changing the voltage. However, we don't yet have the DT bindings to fully describe the Over Voltage Protection (OVP) circuits on these boards. Without that description, we might end up changing the voltage too much, too fast. Add the pwm-regulator descriptions and associate the CPU OPPs, but leave them disabled. Signed-off-by: Brian Norris <briannorris@chromium.org> --- arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 146 +++++++++++++++++++++++++++ 1 file changed, 146 insertions(+)