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 4:
(7 comments)
https://review.coreboot.org/#/c/19849/2/src/console/Kconfig File src/console/Kconfig:
Line 258: file in the CBFS.
As noted on irc by Nico cbfstool can extract an fmap region from the image
Yep, very happy about that. Now the only annoyance is the custom fmd. Do we want to delete the tested/working cbfs code ? or keep it to give the choice to the user? Now that I know how to use the fmap, either option is fine with me, so it really depends on what you want to see upstreamed.
https://review.coreboot.org/#/c/19849/4/src/console/Kconfig File src/console/Kconfig:
Line 239: select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY
select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY if !COMMON_CBFS_SPI_WRAPPER
Done
https://review.coreboot.org/#/c/19849/2/src/drivers/spi/flashconsole.c File src/drivers/spi/flashconsole.c:
Line 55: cbfs_file_data(&cbfs_region, &file);
prepare_program_locate() is really only for ensuring vboot hooks in. I don'
I see, the only one to implement prepare is the vboot locator, and the boot_device_init() is also called in the boot_region_properties, so the prepare is not needed. thx. Good point also about removing boot_device_init. will do.
Line 90: if (offset >= cbfs_size) {
You should have a struct region_device that you rdev_chain() from the root
Yeah, I did exactly that in the commit that adds FMAP support: https://review.coreboot.org/#/c/19866/2 I removed the size var and use the region_device_sz() API instead as well.
https://review.coreboot.org/#/c/19849/4/src/drivers/spi/flashconsole.c File src/drivers/spi/flashconsole.c:
PS4, Line 46: car_set_var(g_rdev, NULL); : car_set_var(g_offset, 0); : car_set_var(g_line_offset, 0);
bss and CAR_GLOBAL are by definition set to 0. No need to do this.
It doesn't matter. It's the init function, it needs to set them to sensible values, regardless of what they were set to before. init does get called again at the start of each stage, is the value in car kept as it was or reset to 0? Better to avoid the confusion.
Line 57: cbfs_size = region_device_sz(&cbfs_region);
So at this point you should just initialize your global struct region_devic
Yeah, that's what I did, once I squash my commits, it will be easier to review.
PS4, Line 98: memset(line_buffer, 0, LINE_BUFFER_SIZE);
unnecessary for same reasons above.
The buffer is used locally to find the offset to start from. Since adding the g_line_offset variable, it's not needed and I've removed it in the most recent commit. A squash is indeed necessary..
-
Youness Alaoui (Code Review)