[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:

(13 comments)

tx_flush support added here : https://review.coreboot.org/19865 
I'm working now on adding fmap support.

https://review.coreboot.org/#/c/19849/2//COMMIT_MSG
Commit Message:

PS2, Line 7: Adding
> Add
Done


PS2, Line 23: skylake
> Intel Skylake
Done


PS2, Line 26: fmap
> FMAP
Done


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

PS2, Line 242: region
> It's called "file" in CBFS (and I think "section" is more common for FMAP).
Ah yes, 'file' for CBFS, The API cbfs_locate_file_in_region() and the 'region_device' struct confused me in thinking it's called a 'region'. For FMAP, I used the 'area' naming because of the fmap_locate_area() API.


Line 258: 	  region in the CBFS.
> Why define two mechanisms? The FMAP seems like a more appropriate place to 
Two mechanisms because I don't really know much about how FMAP works, so I originally did it for CBFS (it was called cbfsconsole and renamed the APIs just yesterday). Aaron Durbin suggested FMAP is more appropriate, so I made it fmap-compatible (but not yet implemented), but there's no reason to throw about the cbfs-related code. Also, I think newbies (like me) will find it easier to do a 'cbfstool extract' than having to define a custom fmap, set it in the config, then use dd with specific offsets to copy out the console file from the rom. So.. using CBFS is just more convenient, and that's what I would use (and possibly other newbies), but Aaron (and possibly more advanced coreboot devs) would prefer to use FMAP.


Line 260: if CONSOLE_SPI_FLASH
> Just use depends on instead of this block.
Great idea. I copied this from the CONSOLE_CBMEM which uses an 'if'.


https://review.coreboot.org/#/c/19849/2/src/drivers/spi/Makefile.inc
File src/drivers/spi/Makefile.inc:

Line 22: 	| tr '\000' '\377' > $@
> Can't this go on a single line? Or at least the full dd piece?
Yes, the bs= was the long $(shell printf) line, which is why it was originally split. Once I moved it out into a variable, it became easier to fit in a single line.


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

Line 41: 	if (cbfs_boot_locate(&file, "console", NULL) == 0) {
> nit: probably looks better with
Will do, but I personally prefer it the way I did it.
And no lines go over 80 chars, so it's your gerrit I suppose.


Line 55: 		boot_device_init();
> Ya. I think we have almost every call that would touch the boot device in t
Yes, I know, but there's no guarantee that the prepare_program_locate API is calling it. Also, there's no guarantee the prepare_program_locate will be called/remain there (especially if we add FMAP support), so I preferred to keep the boot_device_init() call right next to the boot_device_rw() call in case of refactor. It's a no-op and it's safer, so let's keep it (also noticed it being redundantly called in existing parts of the code).


Line 60: 		 * the end of a previous log write.
> You're banking on the fact that console output can never have legitimate 0x
That's a good point, and yes, I am assuming the console is for plaintext only, and any binary is printed using the hexdump APIs. Is that not the case? If not, then I can do it the way you suggested.


Line 121: 		for (i = 0; i < LINE_BUFFER_SIZE - 1; i++) {
> Why not just store the current index in a global as well, rather than searc
No idea. will do. thx


Line 129: 		if (len >= LINE_BUFFER_SIZE - 1 ||
> Why not tie this to tx_flush instead?
I copied the spiconsole.c as a base and it didn't have tx_flush, so I didn't think of it. I think adding tx_flush support in a separate commit would be better.


https://review.coreboot.org/#/c/19849/2/src/include/console/flash.h
File src/include/console/flash.h:

Line 25: #define __CONSOLE_FLASH_ENABLE__	IS_ENABLED(CONFIG_CONSOLE_SPI_FLASH)
> All the consoles that are actually used then...
Yeah, all the other consoles I saw used it this way, it's usually also used with (&& ENV_RAMSTAGE), stuff like that. And I used it a lot when the flashconsole was not working in romstage, then the expression got simplified to this.


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