whoops ;) lemme go fix that

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

#else
...
#endif
}

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

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

We generally don't keep these around.

Also, why can't you use GENERIC?

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

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

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.

why sysctl.h here?

why are these empty?

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

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.

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

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!

#else
...
#endif
}

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

ok, I'll go do that!

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!

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

options.arm

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.

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

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.

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!

copypasta. fixed!

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.

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

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.

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

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.

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.

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.

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

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

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

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.

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

Stray line

This file no longer exists?

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

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

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

nope; the compiler isn't complaining about it.

I'll rename it.

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.

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.

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

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.

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

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

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