[coreboot-gerrit] Change in coreboot[master]: console/flashsconsole: Add spi flash console for debugging

[Youness Alaoui (Code Review)]

Youness Alaoui has posted comments on this change. ( https://review.coreboot.org/19849 )

Change subject: console/flashsconsole: Add spi flash console for debugging
......................................................................


Patch Set 2:

(3 comments)

https://review.coreboot.org/#/c/19849/2/src/console/Kconfig
File src/console/Kconfig:

Line 258: 	  region in the CBFS.
> What do you mean with "custom fmd"? Just integrate this in the default FMAP
This was discussed on IRC with Aaron. Adding it to the default FMAP would make everyone have a CONSOLE area (with size 0 if disabled), which is not practical for such a pre-bootable-board-porting debugging feature. 
I think anyone wants to use that feature, they can set a custom fmap, or use the cbfs feature instead if they don't want to bother.

IRC log for posterity :

May 24 14:17:47 <KaKaRoTo>	adurbin, well, I figured if I added it, it would have a size of zero if flashconsole is disabled, but the area would still be present
May 24 14:19:01 <KaKaRoTo>	unless I replace '##FMAP_SIZE##" by "0x100\n\tCONSOLE ...." so the default-x86.fmd file isn't modified, but that's hacky. other possibility is having one default fmd with and one without the console, but I don't like the redundancy
May 24 14:26:03 <adurbin>	KaKaRoTo: i would just leave it alone and just test for it dynamically first in the driver. it gets complicated pretty quickly since you need to allocate your area with proper alignment. our tooling for doing that sorta thing automatically in fmap is non-existent
May 24 14:26:42 <KaKaRoTo>	adurbin, right, didn't think about alignment (although not needed anyways since I don't do any erase operations anymore)
May 24 14:27:05 <adurbin>	ahh. well, if there are no erase paths then it doesn't matter as much


https://review.coreboot.org/#/c/19849/9/src/drivers/spi/flashconsole.c
File src/drivers/spi/flashconsole.c:

Line 93: 			car_set_var(g_rdev, rdev);
> Here as well (since due to the way the console fills up and has to be manua
The FMAP and CBFS APIs already do prints, such as :
"FMAP: Found "FLASH" version 1.1 at e00000."
and "FMAP: area %s found @ %x (%d bytes)"
same with CBFS APIs.
Also, those lines will not appear because the console is not yet initialized. They would only appear in cbmem, and flashconsole should not be used if cbmem is available (in which case, they are helpful to tell us we forgot to disable the option).


Line 167
> nit: just pull this into _tx_flush() so you don't need to have it twice?
Good idea. I hadn't noticed that all the 'risky' code was in tx_flush now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I74a297b94f6881d8c27cbe5168f161d8331c3df3
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Youness Alaoui <snifikino at gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Youness Alaoui <snifikino at gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes