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.
-
Youness Alaoui (Code Review)