Repository navigation
soc: mt6735: Add SMP fix so more than 1 cpu works - #160
Kaloyan501 wants to merge 1 commit into
Conversation
20261021_225021.mp4 |
|
Proof it works |
|
CPUs also show up in htop and pass a stress test |
BotchedRPR
left a comment
BotchedRPR
left a comment
There was a problem hiding this comment.
This stuff is huge. Give me a few days to review.
b9060a2 to
c4e765e
Compare
|
I might be misunderstanding the purpose of this PR, but usually it is the kernel which brings up secondary CPUs, right? Bootloaders typically only run with one enabled. |
Read the description on the original repo, I just ported it over to uniLoader. If it works it works. |
|
Quote from the original repo README: MT6735 exposes a broken PSCI implementation that expects the kernel to power up the CPU's power domain on its own after the PSCI request has been sent. Work around that by booting up the CPUs before Linux starts and passing them via a spin-table |
|
Basically kernel cant bring the cpus up itself cause LK is broken (who would have expected that) |
Not lk but atf |
|
Can you indent with tabs, not spaces? also surely there's a better way to get this without parsing the board dt like that, hm |
God dammit, my text editor for some reason inputs spaces instead of a tab when I press the tab key. Will fix. |
|
Also there is definitely a better way to do it, I couldn't think of one at the time, any ideas are welcome! |
|
The indentations using spaces should be fixed now. |
chmcax
left a comment
chmcax
left a comment
There was a problem hiding this comment.
this is really dirty
also, this should be done in a small shim separate from uniLoader
| #include <lib/debug.h> | ||
|
|
||
| extern struct board_data board_ops; | ||
| void *board_dtb; |
There was a problem hiding this comment.
nitpick: there has to be a better way to do this
There was a problem hiding this comment.
I mean boards being able to access the dtb from early and late init is a good thing. What do you suggest as a better way, some function that returns the dtb or something else?
I disagree |
I mean this basically is just that but within a uniLoader "driver" |
That shim already exists but is used with zImage64, check the original repo linked in the first message of this PR. The whole idea of porting it to uniLoader is to not have a shim in a shim. Also there is currently no other way I know of to bring up the secondary cpus of the MT6735 up on a mainline kernel apart from this shim (and subsequently the code in this PR). As for any inconsistencies and weirdness, this is literally a direct copy paste job of the code from the original repo, just with modifications of how the DT is read. Will see your other comments on the code itself tomorrow. |
d015318 to
66076cb
Compare
| ; | ||
| } | ||
|
|
||
| writel(readl(ctl) | SRAM_ISOINT_B, ctl); |
There was a problem hiding this comment.
We need to write to this memory address 4 times? Do the values change? If so, why is there no waiting period to allow it to change? Shouldn't the bitwise ops be done just once if no?
There was a problem hiding this comment.
Same answer as the hardcoded values, original repo does it like that, no idea why or if its needed for some weird janky reason.
| extern char cpu_entry_end[]; | ||
| extern uint64_t cpu_spin_table[3]; | ||
|
|
||
| #define SPM_BASE 0x10000000UL |
There was a problem hiding this comment.
/methinks this could be a driver.
There was a problem hiding this comment.
Should I move the whole file to driver/ or is that up for debate?
There was a problem hiding this comment.
yeah, ideally move it to drivers/mediatek
|
Also latest changes are untested on device, just typing this here so I wont forget to check tomorrow. |
|
Submitting code that you don't understand isn't really all that good. This code needs a refactor and if its not you who will do it then I am not sure who will... No ill intentions on my part here but I'm not going to accept this code as it looks right now nor could help you much with a refactor even if I personally understand it, I don't have a device to test changes. Nothing stands in your way of understanding the code and testing the refactor though |
|
Ok, I will try to research the purpose of the magic values and get back to you if I find their purpose. |
|
Ok, I think I found the purpose of some of them, will make a refactor with explanations where needed. |
|
Magic value 0x84000003 - PSCI_CPU_ON_AARCH32 as defined in https://github.com/ARM-software/arm-trusted-firmware/blob/6355f2347aec8bf6ad74867c2b0c996e10546ad4/include/bl31/services/psci.h |
|
Also should I try to rewrite the ASM in C directly? |
|
About writing 4 times to the same memory address: downstream does it that way too. mt_spm_mtcmos.c CPU1 power-on branch, SPM Unlock Write. |
|
Also what I linked is the ground truth of all of this from the looks of it, so now I can answer any questions about magic numbers and such when needed. |
|
Also downstream code has udelay(1) on the register writes, I did not see that in the original code this PR is based on, the payload runs fine, but there is a non-0% chance that is just luck. If needed, I can add them. |
|
Also you said this code needs a refactor, I do not want to guess, please tell me in bulk what exactly it needs if you can, so I can do it in one go. |
| } | ||
| } | ||
|
|
||
| node = fdt_path_offset(fdt, "/reserved-memory/spin-table@0"); |
There was a problem hiding this comment.
whole reserve-memory part: @BotchedRPR @ivoszbg there should be a common reserve-memory function imo, there's probably gonna be other drivers that would need this
BotchedRPR
left a comment
BotchedRPR
left a comment
There was a problem hiding this comment.
You added your dirty files here
Remove them so that I can genuinely review
|
My gitignore got nuked, sorry about that. Will fix. |
0bec63c to
9ea7c93
Compare
MT6735 exposes a broken PSCI implementation that expects the kernel to power up the CPU's power domain on its own after the PSCI request has been sent. Work around that by booting up the CPUs before Linux starts and later passing them via a spin-table in the device's DTS. Based on [1]. [1]: https://gitlab.com/mt6735-mainline/mt6735-smp-fix Signed-off-by: Kaloyan Donev <donevkaloyan@gmail.com>
|
There is a regression, watchdog disabling does not work. |
|
Everything is fine, issue was caused by my outdated defconfig |
Adds a SMP fix specific to the MT6735.
Almost verbatim copy of this repo: https://gitlab.com/mt6735-mainline/mt6735-smp-fix
License and the original owner allow integrating the code into this repo.
Screenshot of permission confirmation and link to public chat message can be provided if needed.
The diffs contain things like a WARNING while compiling and a running smp fix thing, which I think are nice, but if they are not neccessary I can remove them.