[coreboot-gerrit] Change in coreboot[master]: amd/pi/mainboards: Consolidate early duplicated code

Sat Apr 22 03:12:43 CEST 2017

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/18434 )

Change subject: amd/pi/mainboards: Consolidate early duplicated code
......................................................................

Patch Set 12:

(21 comments)

https://review.coreboot.org/#/c/18434/3//COMMIT_MSG
Commit Message:

Line 16: APUs also get BIST bits masked off.
> agree, need to check
I disagree.  This patch was to consolidate all the PI-based mainboards' romstage files.  Two of the boards won't boot without keeping the change.

https://review.coreboot.org/#/c/18434/11//COMMIT_MSG
Commit Message:

PS11, Line 12: e6af4be
> We've since abandoned that approach because of the complicated flow it crea
Agree.  That will require a little work, as we've discussed.

https://review.coreboot.org/#/c/18434/11/src/cpu/amd/pi/00660F01/fixme.c
File src/cpu/amd/pi/00660F01/fixme.c:

PS11, Line 98: Mask off
> Yes, I know you were rearranging, but we're just perpetuating badness.
comment clarified a little

https://review.coreboot.org/#/c/18434/12/src/cpu/amd/pi/00660F01/fixme.c
File src/cpu/amd/pi/00660F01/fixme.c:

Line 96: unsigned long car_bist_mask_bist(unsigned long bist)
> In patch set 3 you agreed BIST is separate change.
> Please revisit patchset 3 on all the things you previously agreed to check.
In patch set 3 you agreed BIST is separate change.

I missed that, but I disagree that it's a separate change.  his patch was to consolidate all the PI-based mainboards' romstage files.  Two of the boards won't boot without keeping the change.

https://review.coreboot.org/#/c/18434/3/src/cpu/amd/pi/car.c
File src/cpu/amd/pi/car.c:

Line 26: void cache_as_ram_main(unsigned long bist, unsigned long cpu_init_detectedx)
> Also agree.  Not sure the history of that.  It's misleading.
Renamed, although now the naming of the argument doesn't match  the prototype.

https://review.coreboot.org/#/c/18434/11/src/cpu/amd/pi/car.c
File src/cpu/amd/pi/car.c:

Line 30: 	if (IS_ENABLED(CONFIG_COLLECT_TIMESTAMPS))
> if (IS_ENABED())
Done

https://review.coreboot.org/#/c/18434/12/src/cpu/amd/pi/car.c
File src/cpu/amd/pi/car.c:

Line 31: 		timestamp_init(timestamp_get());
> I believe we may enter here twice with BSP with CAR contents maintained. Be
> I believe we may enter here twice with BSP with CAR contents maintained.

There is a reset during POST.  Is that what you mean?  I don't think the BSP will reenter here.  APs will try to run this but timestamp_init() checks for boot_cpu().

> My experience with f14 was that TSC rate changes while in AmdInitPost() and timestamps effectively appeared to run backwards.

Backwards sounds difficult to do.  I wonder if the TSC MSR gets overwritten.

I also recall that products vary on what the turnon P-state is.  IIRC F14h comes up in the slowest and AGESA turns it up to P0 as soon as it can.  However, this doesn't sound consistent with your experience either, as AMD parts have invariant TSCs and the frequency shouldn't change.

Line 35: 	/* Must come first to enable PCI MMCONF. */
> So you decide just ignore this?
Comment removed.  I believe the SMP issue you've cited still exists here and in the native AGESA.  Let's address that in a subsequent patch.

https://review.coreboot.org/#/c/18434/11/src/cpu/amd/pi/car.h
File src/cpu/amd/pi/car.h:

Line 20: /* Early initialization immediately after CAR setup */
> What's the general boot flow to call this function? And the other functions
misc. comments added

https://review.coreboot.org/#/c/18434/11/src/mainboard/amd/db-ft3b-lc/early_mainboard.c
File src/mainboard/amd/db-ft3b-lc/early_mainboard.c:

Line 30: 	outb(0x00, 0xcd7);
> If we're actually cleaning things up for the better I'm fine modifications.
Done

https://review.coreboot.org/#/c/18434/6/src/mainboard/amd/gardenia/romstage.c
File src/mainboard/amd/gardenia/romstage.c:

Line 41: 	disable_cache_as_ram();
> Add a comment that the location (after amdinitenv) and implementation of di
Have added to the common romstage patch:
https://review.coreboot.org/#/c/18495

https://review.coreboot.org/#/c/18434/11/src/mainboard/amd/lamar/early_mainboard.c
File src/mainboard/amd/lamar/early_mainboard.c:

Line 39: 	outb(0x01, 0xCD7);
> I know this is lifted from another file, but the inconsistency of the hex c
Done

Line 41: 	*(volatile u32 *) (AMD_SB_ACPI_MMIO_ADDR + 0xE00 + 0x40) &= ~(1 << 2); /* 24Mhz */
> Similarly, we shouldn't be open coding volatile writes.
Done

https://review.coreboot.org/#/c/18434/11/src/mainboard/amd/olivehillplus/early_mainboard.c
File src/mainboard/amd/olivehillplus/early_mainboard.c:

PS11, Line 30: 0xcd7
> PM_DATA and PM_INDEX....
Done

Line 37: 	/*
> comments > 80
Done

Line 40: 	 * been replicated from earlier development boards.
> Someone didn't want to put a variable declaration at the top of the functio
Done

https://review.coreboot.org/#/c/18434/12/src/mainboard/amd/olivehillplus/early_mainboard.c
File src/mainboard/amd/olivehillplus/early_mainboard.c:

Line 41: 	 */
> Interestingly, amd/olivehill board (not -plus) has following comment for sa
Yes, interesting.  It may be that this doesn't need to be here at all, for either board.  The ICS551M is a clock buffer but Olive Hill and Plus don't have that same device.  The "processor initialization is slow or incorrect" phrase is a little strange too.

PS12, Line 42: for(
> space between 'for' and '('
Done

PS12, Line 43: 0xCD6
> PM_INDEX
Done

https://review.coreboot.org/#/c/18434/11/src/mainboard/pcengines/apu2/early_mainboard.c
File src/mainboard/pcengines/apu2/early_mainboard.c:

Line 33: 	configure_gpio(ACPI_MMIO_BASE, IOMUX_GPIO_71, Function0, GPIO_71, setting);
> I'd hope in doing this we've fix the things that previously have plagued th
Done

https://review.coreboot.org/#/c/18434/11/src/southbridge/amd/pi/hudson/early_setup.c
File src/southbridge/amd/pi/hudson/early_setup.c:

Line 62: 
> if (IS_ENABLED(CONFIG_HUDSON_UART))
Done

-- 
To view, visit https://review.coreboot.org/18434
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id10580ecc439df10df26ca99881772f328e7bdd0
Gerrit-PatchSet: 12
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude at gmail.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes