Page MenuHomeFreeBSD

ipq4018: add initial IPQ4018/IPQ4019 support
ClosedPublic

Authored by adrian on Oct 17 2021, 4:11 PM.
Tags
None
Referenced Files
F86650735: D32538.id97013.diff
Sun, Jun 23, 12:41 PM
Unknown Object (File)
Sat, Jun 22, 2:16 PM
Unknown Object (File)
Mon, Jun 10, 11:10 AM
Unknown Object (File)
Wed, Jun 5, 5:57 PM
Unknown Object (File)
Sun, Jun 2, 3:56 PM
Unknown Object (File)
Sun, Jun 2, 4:00 AM
Unknown Object (File)
Sat, May 25, 9:47 PM
Unknown Object (File)
May 23 2024, 6:16 AM

Details

Summary

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.

Test Plan
  • compiled/booted on an IPQ4019 SoC AP

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 42193
Build 39081: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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).

In D32538#734122, @imp wrote:

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).

The binding file isn't dual licensed.

sys/arm/conf/ASUS_AC1300
2

whoops ;) lemme go fix that

In D32538#734113, @manu wrote:

DTS and bindings shouldn't be there, it's only for the upstream ones.

ok, then where should I put them instead?

sys/arm/arm/debug_monitor.c
75

This is a NOP. If this is not a NOP, you have bigger issues than this will solve.

969

#else
...
#endif
}

I think would be better. It will avoid new warnings.

sys/arm/conf/ASUS_AC1300
2

IIR IRC correctly, this is a 64-bit part that Adrian is bringing up in 32-bit mode.

29–33

We generally don't keep these around.

Also, why can't you use GENERIC?

36

This is redundant, remove it. It's in std.qca.

sys/arm/qualcomm/ipq4018_machdep.c
110

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

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)...
here and elsewhere.

35

why sysctl.h here?

sys/arm/qualcomm/ipq4018_mp.c
62

why are these empty?

sys/conf/options
1034

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

Oh, this is GPL'd? I missed it when I removed core from the review.
But apart from the comments, there's nothing GPL-y about it.
And it likely should be part of the bits that manu pulls in from upstream.

sys/contrib/device-tree/src/arm/qcom-ipq4018-rt-ac58u.dts
1

same comment as manu's here: Either pull this in from upstream, or put it elsewhere.

In D32538#734113, @manu wrote:

DTS and bindings shouldn't be there, it's only for the upstream ones.

ok, so where should I put them? I'm happy to move them around!

sys/arm/arm/debug_monitor.c
75

This is a NOP. If this is not a NOP, you have bigger issues than this will solve.

Ha yes :P

oh it was just me being explicit about it during bring-up. I can delete it if needed!

969

#else
...
#endif
}

I think would be better. It will avoid new warnings.

ok, I'll go do that!

sys/conf/options
1034

Please put this in a more-specific option file. It has no business in the global one.

i'm open to suggestions for where to put it!

sys/arm/qualcomm/ipq4018_mp.c
62

If there's only one CPU you can remove these. The default implementation handles this case.

sys/conf/options
1034

options.arm

ian added inline comments.
sys/arm/qualcomm/ipq4018_machdep.c
110

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.

adrian added inline comments.
sys/arm/conf/ASUS_AC1300
2

it's a quad core A7, so 32 bit armv7 only!

29–33

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

If there's only one CPU you can remove these. The default implementation handles this case.

They're placeholders for the other 3 cores I'm about to bring up once this stuff lands!

adrian marked an inline comment as done.

More comment addressing

adrian marked 2 inline comments as done.
adrian added inline comments.
sys/arm/qualcomm/ipq4018_machdep.h
35

copypasta. fixed!

sys/arm/qualcomm/ipq4018_machdep.c
110

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.

adrian added inline comments.
sys/arm/qualcomm/ipq4018_machdep.c
110

i can put another build option in there just to enable it when building this platform?

sys/arm/qualcomm/ipq4018_machdep.c
110

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

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

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.

jrtc27 added inline comments.
sys/arm/arm/debug_monitor.c
963

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–345

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
8

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).

29

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
66

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...).

92

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
3

This would normally be optional smp, and that looks like it should work here too

4

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–345

There's no trapframe echoed super early in boot, so all you get is this 32 bit value.

adrian added inline comments.
sys/arm/arm/debug_monitor.c
963

nope; the compiler isn't complaining about it.

I'll rename it.

sys/arm/conf/std.qca
8

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.

29

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
66

ok, lemme update the comment. I'd like to fix this properly later :)

92

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.

92

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!

adrian marked an inline comment as done.

Address more comments!

Let's see if this fixes where the tcsr.h file went in the review.

Shuffle the bindings to a new location!

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).

This revision was not accepted when it landed; it landed in state Needs Review.Oct 18 2021, 7:19 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.