This adds required IPQ4018/IPQ4019 SoC support to boot.
It also includes support for disabling the ARMv7 hardware
breakpoint / debug stuff at compile time as this is
required for the IPQ SoCs, and printing out the undefined
instruction itself.
Details
Details
- Reviewers
manu - Group Reviewers
ARM - Commits
- rG015ff812d6b7: ipq4018: add initial IPQ4018/IPQ4019 support
- compiled/booted on an IPQ4019 SoC AP
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Removed core from reviewer. This tripped over the GPL check in herald for SPDX lines, but the OR MIT means it's fine and doesn't require approval (which is why I removed core blocking rather than approved it with my core hat on).
| sys/arm/conf/ASUS_AC1300 | ||
|---|---|---|
| 3 | whoops ;) lemme go fix that | |
| sys/arm/arm/debug_monitor.c | ||
|---|---|---|
| 75 ↗ | (On Diff #97009) | This is a NOP. If this is not a NOP, you have bigger issues than this will solve. |
| 969 ↗ | (On Diff #97009) | #else I think would be better. It will avoid new warnings. |
| sys/arm/conf/ASUS_AC1300 | ||
| 3 | IIR IRC correctly, this is a 64-bit part that Adrian is bringing up in 32-bit mode. | |
| 30–34 | We generally don't keep these around. Also, why can't you use GENERIC? | |
| 37 | This is redundant, remove it. It's in std.qca. | |
| sys/arm/qualcomm/ipq4018_machdep.c | ||
| 110 ↗ | (On Diff #97009) | We generally don't commit #if 0 code to the tree. If EARLY_PRINTF isn't right, then find a better way. Though this code looks very 'early system bringup-y rather than what we'd normally commit to the tree since it's kinda fragile... |
| sys/arm/qualcomm/ipq4018_machdep.h | ||
| 5 ↗ | (On Diff #97009) | I'd ditch all rights reserved. I doubt that you have a need for the Buenos Aires Convention (now that it's almost 20 years expired)... |
| 35 ↗ | (On Diff #97009) | why sysctl.h here? |
| sys/arm/qualcomm/ipq4018_mp.c | ||
| 62 ↗ | (On Diff #97009) | why are these empty? |
| sys/conf/options | ||
| 1034 ↗ | (On Diff #97009) | Please put this in a more-specific option file. It has no business in the global one. |
| sys/contrib/device-tree/include/dt-bindings/soc/qcom,tcsr.h | ||
| 10 ↗ | (On Diff #97009) | Oh, this is GPL'd? I missed it when I removed core from the review. |
| sys/contrib/device-tree/src/arm/qcom-ipq4018-rt-ac58u.dts | ||
| 1 ↗ | (On Diff #97009) | same comment as manu's here: Either pull this in from upstream, or put it elsewhere. |
| sys/arm/arm/debug_monitor.c | ||
|---|---|---|
| 75 ↗ | (On Diff #97009) |
Ha yes :P oh it was just me being explicit about it during bring-up. I can delete it if needed! |
| 969 ↗ | (On Diff #97009) |
ok, I'll go do that! |
| sys/conf/options | ||
| 1034 ↗ | (On Diff #97009) |
i'm open to suggestions for where to put it! |
| sys/arm/qualcomm/ipq4018_machdep.c | ||
|---|---|---|
| 110 ↗ | (On Diff #97009) | This review-comment is just wrong in this case. The code-comment block explains exactly why it must be #if 0, not #if EARLY_PRINTF. |
| sys/arm/conf/ASUS_AC1300 | ||
|---|---|---|
| 3 | it's a quad core A7, so 32 bit armv7 only! | |
| 30–34 | I'll likely end up using this some more during SMP bring-up and some trustzone work once the rest of this stuff is up and going. | |
| sys/arm/qualcomm/ipq4018_mp.c | ||
| 62 ↗ | (On Diff #97009) |
They're placeholders for the other 3 cores I'm about to bring up once this stuff lands! |
| sys/arm/qualcomm/ipq4018_machdep.h | ||
|---|---|---|
| 35 ↗ | (On Diff #97009) | copypasta. fixed! |
| sys/arm/qualcomm/ipq4018_machdep.c | ||
|---|---|---|
| 110 ↗ | (On Diff #97009) | No. It is right. If 0 is wrong and the comment is wrong on that point. If it is optional, it needs a new option name if EARLY_PRINTF isn't right. |
| sys/arm/qualcomm/ipq4018_machdep.c | ||
|---|---|---|
| 110 ↗ | (On Diff #97009) | i can put another build option in there just to enable it when building this platform? |
| sys/arm/qualcomm/ipq4018_machdep.c | ||
|---|---|---|
| 110 ↗ | (On Diff #97009) | Since this is early bring up, this comment should have "should" read in the RFC sense: if there is a reason to keep it for a while and evolve it as the code matures, that's cool too. |
| 110 ↗ | (On Diff #97009) | For now, it's fine. Let's wait until things are further along with other soc support for qualcomm |
| sys/arm/qualcomm/ipq4018_machdep.c | ||
|---|---|---|
| 110 ↗ | (On Diff #97009) | No, it is wrong. Look at the existing armv7 code that's checked in already. The SOCs that have maintainers all have early_putc code wrapped in #if 0. For a good reason. If you want to go add soc-specific options for every soc so that #if 0 can be replaced with something like #ifdef RK3288_EARLY_PRINTF, then by all means knock yourself out. But fix the existing code before you start complaining about something not matching existing practice. Also, this code is NOT just for one-time early bringup. It's for every time booting fails in early boot, like when something breaks in loader, or uboot. That's why we leave this stuff in there wrapped in #if 0. |
| sys/arm/arm/debug_monitor.c | ||
|---|---|---|
| 963 ↗ | (On Diff #97015) | Does this not cause warnings for the various now-unused static functions like dbg_arch_supported? Also why ARMV7? The code below suggests this also exists for v6, so should just be ARM. |
| sys/arm/arm/undefined.c | ||
| 344 ↗ | (On Diff #97015) | If you're printing out the instruction bytes it's probably also worth printing out whether or not it's a thumb instruction? Otherwise you need to go stare at SPSR in the dumped trapframe to figure out how to even decode the thing. |
| sys/arm/conf/std.qca | ||
| 7 ↗ | (On Diff #97015) | This seems to be done all over the place in sys/arm/conf, but I don't understand why, shouldn't bsd.cpu.mk be doing this bad on MACHINE_ARCH (or perhaps by specifying CPUTYPE here)? No other architecture does this (well, except mips with its own NIH mips-specific ARCH_FLAGS variable, but we all know mips is not a good example to follow for pretty much anything). |
| 28 ↗ | (On Diff #97015) | This will need addressing in a better way for GENERIC... that or figure out if there's way to make it work, I don't know the details here |
| sys/arm/qualcomm/ipq4018_machdep.c | ||
| 65 ↗ | (On Diff #97015) | FIXME or XXX this and explain *why* the current code doesn't work. Or maybe we should fix the issue like I described on IRC rather than committing this kind of hack (so GENERIC kernels work on this board, for example...). |
| 91 ↗ | (On Diff #97015) | I assume this is only needed for early printf? If so it's entirely unsurprising that things explode if you don't reserve it, and the comment could do a better job of pointing at it being that. |
| sys/arm/qualcomm/std.ipq4018 | ||
| 2 ↗ | (On Diff #97015) | This would normally be optional smp, and that looks like it should work here too |
| 3 ↗ | (On Diff #97015) | Stray line |
| sys/dts/arm/qcom-ipq4018-rt-ac58u.dts | ||
| 6 ↗ | (On Diff #97015) | This file no longer exists? |
| 27 ↗ | (On Diff #97015) | Is this a local diff or upstream's untidy code? |
| sys/dts/arm/qcom-ipq4018-rt-ac58u.dts | ||
|---|---|---|
| 27 ↗ | (On Diff #97015) | It's mine for now until i get the clock tree up, where i then /can/ reliably configure the baud rate. |
| sys/arm/arm/undefined.c | ||
|---|---|---|
| 344 ↗ | (On Diff #97015) | There's no trapframe echoed super early in boot, so all you get is this 32 bit value. |
| sys/arm/arm/debug_monitor.c | ||
|---|---|---|
| 963 ↗ | (On Diff #97015) | nope; the compiler isn't complaining about it. I'll rename it. |
| sys/arm/conf/std.qca | ||
| 7 ↗ | (On Diff #97015) | Yeah, I noticed this too. We should likely review this (and the early printf stuff) later on and see if we can clean it up across the board. |
| 28 ↗ | (On Diff #97015) | Yeah I agree too. I'll have to go see if there's a way to probe this at boot time but I don't know if we have access to the needed memory without loading TZ modules. |
| sys/arm/qualcomm/ipq4018_machdep.c | ||
| 65 ↗ | (On Diff #97015) | ok, lemme update the comment. I'd like to fix this properly later :) |
| 91 ↗ | (On Diff #97015) |
|
| 91 ↗ | (On Diff #97015) | Nope, it's required to boot normally! That's what the comment is pointing out. It's actually not needed for early printf. I have a feeling there's something that has crept into the arm boot process which has been hidden because it seems /every/ platform uses devmaps that cover the initial console. :) |
| sys/dts/arm/qcom-ipq4018-rt-ac58u.dts | ||
| 6 ↗ | (On Diff #97015) | That's .. odd and let me see what the heck happened with my commit stack! |
| 6 ↗ | (On Diff #97015) | It's still in the commit stack; I am not sure why it is not showing up in the review! |
Comment Actions
Do you need to commit arm/qualcomm/ipq4018_mp.c now ?
Since it's not needed as you don't support SMP (and don't even compile the file since it's gated by option smp).