[coreboot] PC Engines apu2 boot regression

[Kyösti Mälkki]

On Sat, May 5, 2018 at 6:27 PM, Marshall Dawson
<marshalldawson3rd at gmail.com> wrote:
>> So did you hit problems with CAR_GLOBAL in agesa_get_dispatcher(),
>> when experimeting CZ vs ST? I don't see why this code works even for
>> dual-core ST but I did not evaluate CAR layout. Documentation part for
>> fixed MTRRs in gcccar.inc appears to be same.
>
>
> I don't remember specifically where the condition caused a problem.  It kind
> of seems like it was when the third core tried to run printk, wanted a
> global pointer to a global variable, and got back 0xffffffff.  It's been too
> long to say for certain.  IIRC, cores 0 & 1 received fixed MTRR settings
> allowing 0x30000-0x3ffff, but 2 & 3 started at 0x40000 and couldn't access
> the globals just above 0x30000.  It's worth noting that cores in Family 15h
> share their MTRRs within a compute unit, so it was never possible for 0 & 1
> to be anything but identical.
>

Every discovered core adds WB regions to MSR. So if you say the tested
CZ had separate compute units, while ST only had one, that would
indeed explain it.

>> Now, does this MTRR change have potential to actually become _the_
>> fix? I don't see why it could not. It's pretty much all that we1 can do
>> with binaryPI, anything else touches the blobs.
>
>
>> Will you sent patch?
>

Yes.

>
> I don't think _the_ fix has been pursued yet.  It's certainly lower priority
> than other things I have going on right now.  But in my mind, it's still a
> little too hacky.  It assumes the hardcoded value of BSP_STACK_BASE_ADDR
> stays consistent (which needs to also equal DCACHE_RAM_BASE).  Maybe a
> better approach is to allow all cores to have access to 100% of the CAR
> region.  Maybe just their own plus the globals area?  I haven't given much
> thought to why cores received differing regions in AMD's CAR setup code.
> What I would really enjoy seeing is moving CAR above 1MB and skipping all
> the fixed MTRR setup, but that's a pretty big rewrite.

Keep in mind, this code ends up in your RO bootblock. Give it some
high priority on StoneyRidge buglist.

> Kyosti, do you want to submit a patch since you're able to test it on other
> platforms?  I'd be OK with having the 64K region at BSP_STACK_BASE_ADDR
> being WB for all cores until a more robust solution is available.  I don't
> think this is a characteristic of PI blob implementations, by the way.  I
> would expect all the native AGESA platforms to have the same limitation too.
>

That's how I have the patch [1] prepared.

Kyösti

[1] https://review.coreboot.org/#/c/coreboot/+/26115