Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37262 )
Change subject: util: cbfstool: Check alignment at build time ......................................................................
Patch Set 11:
(2 comments)
- Top level FLASH supports specifying alignment by '%alignment'. If not specified default to 4k.
I feel that just makes things unnecessarily more complicated. We'll never use anything other than the default alignment and the "unaligned" marker in practice.
- The section flags also support 'UNALIGNED' that implies '%0'.
I don't see the point in having more than one way to do the same thing.
https://review.coreboot.org/c/coreboot/+/37262/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37262/11//COMMIT_MSG@17 PS11, Line 17: don't need to : be aligned, add a '%' after section
First time I glanced over the CL I thought "okay, % as in '% blocksize == 0', so that marks alignm […]
I would prefer a single character because I think a big UNALIGNED tag makes this a lot more optically "heavy" than it deserves to be compared to the rest of the file. Open to using a different character if people don't like the % (although I think it does fit best because of that modulo operator connection... you just have to think of it as '% blocksize != 0' instead).
https://review.coreboot.org/c/coreboot/+/37262/11/Documentation/lib/flashmap... File Documentation/lib/flashmap.md:
https://review.coreboot.org/c/coreboot/+/37262/11/Documentation/lib/flashmap... PS11, Line 114: must be aligned to erase block (e.g., 4k).
we could factor out the erase size definitions from src/drivers/spi/$vendor.c […]
Yeah, I don't think you can tie this to existing Kconfigs in any way. For example for CONFIG_SPI_FLASH_SPANSION, most of them seem to have 64K but S25FL128P has 256K erase blocks.
I think the way we would do this is to add a new
config SPI_FLASH_MIN_ERASE_BLOCK_SIZE int default 4096
and then add to spi_flash_cmd_erase() something like
if (erase_size > SPI_FLASH_MIN_ERASE_BLOCK_SIZE) ...print warning or die...
and then any board that wants to use a larger-than-4K block will need to override that in its Kconfig, and can also decide to use a custom FMAP only for configs that do that or something.
I don't think setting this belongs into the .fmd file in any way, because the .fmd file doesn't really know what board (variant) it's gonna be used on. In fact it might be shared among different board variants with different erase block sizes (as long as the sections are aligned to the larger size, it's fine). The mainboard Kconfig is the best place to decide those things, and then I would pass it into fmaptool as a command line parameter. But the individual sections inside the FMAP only need to be annotated with whether they need to be block-aligned or not, not with what the exact block size is.
But as I mentioned I don't think we need to do all of that complicated stuff until someone who really cares enough about non-4K chips comes along.