ipq4018: add initial IPQ4018/IPQ4019 support

[adrian]

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

[imp]

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

[manu]

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.

[adrian]

sys/arm/conf/ASUS_AC1300

3

whoops ;) lemme go fix that

[adrian]

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?

[adrian]

Fix comment, thanks bz!

[imp]

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.

973

#else
...
#endif
}

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

111

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

6

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.

36

why sysctl.h here?

sys/arm/qualcomm/ipq4018_mp.c

63

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

(On Diff #97009)

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

[adrian]

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!

[adrian]

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!

973

#else
...
#endif
}

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

ok, I'll go do that!

sys/conf/options

1034 ↗

(On Diff #97009)

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!

[andrew]

sys/arm/qualcomm/ipq4018_mp.c

63

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

sys/conf/options

1034 ↗

(On Diff #97009)

options.arm

[• ian]

sys/arm/qualcomm/ipq4018_machdep.c

111

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]

Address comments!

[adrian]

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

63

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]

More comment addressing

[adrian]

more updates!

[adrian]

sys/arm/qualcomm/ipq4018_machdep.h

36

copypasta. fixed!

[imp]

sys/arm/qualcomm/ipq4018_machdep.c

111

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]

sys/arm/qualcomm/ipq4018_machdep.c

111

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

[imp]

sys/arm/qualcomm/ipq4018_machdep.c

111

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.

111

For now, it's fine. Let's wait until things are further along with other soc support for qualcomm

[• ian]

sys/arm/qualcomm/ipq4018_machdep.c

111

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]

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

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

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

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

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

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

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

3

Stray line

sys/dts/arm/qcom-ipq4018-rt-ac58u.dts

6

This file no longer exists?

27

Is this a local diff or upstream's untidy code?

[adrian]

sys/dts/arm/qcom-ipq4018-rt-ac58u.dts

27

It's mine for now until i get the clock tree up, where i then /can/ reliably configure the baud rate.

[adrian]

sys/arm/arm/undefined.c

344

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

[adrian]

sys/arm/arm/debug_monitor.c

963

nope; the compiler isn't complaining about it.

I'll rename it.

sys/arm/conf/std.qca

7

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

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

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

91

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.

91

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

That's .. odd and let me see what the heck happened with my commit stack!

6

It's still in the commit stack; I am not sure why it is not showing up in the review!

[adrian]

Address more comments!

[adrian]

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

[adrian]

Shuffle the bindings to a new location!

[adrian]

Fix make_dtb.sh

[manu]

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