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


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