Okay, I did not know that. --dry-run been working fine before.
+ /* Set freq to a safe value if transitioning to more than 1 bin*/
+ if (l2_index >= 2)
+ ret = clk_set_rate(l2_clk, policy->l2_rate[0]);
this doesn't look quite right to me, it should be l2_index != 0
. it breaks on any transition from one non-idle bin to another non-idle bin. strictly speaking, if both the current and target l2_freq
are not the idle_freq, you need to set it to idle first. i'm not sure if it's worth branching just to avoid setting the l2_freq
twice though (if it is either already idle or going to be idle)...so maybe you can just check l2_index != 0
.
also, if the ret
value from any clk_set_rate
or regulator_set_voltage_tol
is non-zero, i believe you are supposed to abort the operation and return the ret
value immediately. afaik this tells the kernel that the operation failed so it can handle accordingly (possibly by retrying). i handled this in my old patch set: https://github.com/facboy/openwrt/blob/pr2280/target/linux/ipq806x/patches-4.14/0049-cpufreq-dt-support-l2-cache-and-rpm-clocks-scaling.patch. though i notice i didn't handle errors in the fabric scaling.
i through the problem with transition from idle directly to high. Transition from idle to normal or from normal to idle shouldn't cause any problem. Am I wrong? About the error handling... Yes i should add all the code needed.
I also notice that following the original patch with setting the l2 rate at every cpu freq change does cause performance degradation. (About 50-60 mb lost)
no, the problem was with transition from high to normal or vice versa.
yes, it shouldn't set the l2 freq on every cpufreq change, only on the bin transitions...when you go from idle to anything else, or from 600/800/1000 to 1400/1725 (1200/1400 on ipq8064) or vice versa.
i updated the repo... Now i check if we are switching from 1 to 2 or 2 to 1 .... Switching directly from 0 to 2 (or 1) or 2 to 0 shouldn't cause any problem right?
Also now the changes handle all errors
thx, i took a quick look at https://github.com/Ansuel/openwrt/commits/ipq-improve, where is the check from 1 to 2 or 2 to 1? i couldn't see it.
mb.... didn't push... check now
think it looks ok, i will try it.
well... i remotely killed my router ahahaha
Now i really can't test any improvement... my bad...
haha...i've thought about rolling the dice on this a few times when i've been away. i have never done it :).
fwiw it's working ok on mine.
I was reworking the fab scaling driver to use better code and I """accidentally""" created a null pointer exception
@facboy if you want to have fun with a bricked router... I posted a new version of the fab scaling
It shouldn't cause kernel panic... To test it check with cat /sys/kernel/debug/clk/ebi1_a_clk/clk_rate
if it's set to 533000000 or 400000000 and it does actually scale with a mbw 32
(totally untested)
(I also notice that the driver set the frequency to nominal initially... this doesn't make sense as the driver is probed at boot and we need high frequency :/... Changed it...)
why do you NULL
ddr_fab_clk
but not apps_fab_clk
(on error)?
if (ret != -EPROBE_DEFER) {
pr_err("Failed to get DDR FABRIC clock: %d\n", ret);
ddr_fab_clk = NULL;
ret = -ENODEV;
}
btw, i tried it, it boots at least. clk_rate in kernel debug seems to be correct too.
No reason to do as the check in the scale fabrics make sure both are defined so it's sufficient to NULL one of them.
Mbw results?
I can try it if you want, if it doesn't brick the router.
These patches are the ones needed? https://github.com/Ansuel/openwrt/compare/f8424b1b...0a160310.diff
Or is it just the fab-scaling patch?
brick can totally be recovered with a normal tftp flash anyway...
Yes apply the diff file... Doesn't cause any problem.
@shelterx you need to use this!! https://github.com/Ansuel/openwrt/compare/f8424b1b...04a7fb48124f5eabd7e5324fa2c1eee0912b4d47.diff
This was just after I started mbw
$ cat /sys/kernel/debug/clk/ebi1_a_clk/clk_rate
533000000
mbw results:
AVG Method: MEMCPY Elapsed: 0.04318 MiB: 32.00000 Copy: 741.122 MiB/s
AVG Method: DUMB Elapsed: 0.18627 MiB: 32.00000 Copy: 171.798 MiB/s
AVG Method: MCBLOCK Elapsed: 0.04338 MiB: 32.00000 Copy: 737.654 MiB/s
mine much the same.
Looks good
In your option where should I put the L2 volt cpufreq definitions in the dts?
qcom,cache or cache ?
prolly not qualified to comment but it's pretty qcom specific, isn't it?