Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42124 )
Change subject: spi: Share common logic of vendor specific bpbits<>region functions ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42124/3/src/drivers/spi/spi_common.... File src/drivers/spi/spi_common.c:
https://review.coreboot.org/c/coreboot/+/42124/3/src/drivers/spi/spi_common.... PS3, Line 103: MIN(bits->bp ? granularity << (bits->bp - 1) : 0, Rather than doing the bp_lower_th stuff, can we just invert bits->bp when CMP=1 for Macronix? So what Macronix calls BP3 we would put in bits->cmp, their BP2-0 we put in bits->bp, and then here we do
int effective_bp = bits->bp if (part->cmp_inverts_bp && bits->cmp) effective_bp = (1 << part->bp_bits) - 1 - bits->bp; protected_size = MIN(bits->bp ? granularity << (bits->bp - 1) : 0, flash->size);
(part->bp_bits would have to be 3 for the Macronix parts then, of course.) I think that should work out for the parts you're using, and it would just take one extra bit to encode this quirk.
https://review.coreboot.org/c/coreboot/+/42124/3/src/drivers/spi/spi_flash_i... File src/drivers/spi/spi_flash_internal.h:
https://review.coreboot.org/c/coreboot/+/42124/3/src/drivers/spi/spi_flash_i... PS3, Line 88: uint16_t has_tb : 1; /* top/bottom select available */ This is the structure I'm fighting so hard to keep small because we have to store so many instances of it in the data segment. Due to the alignment of `id`, if you just go 1 bit over 4 bytes you effectively increase space requirements by 50%.
We still have a couple of free bits in the _reserved_for_flags above, so if you can get rid of the bp_lower_th and make this work with only has_tb and cmp_inverts_bp, it should fit.
https://review.coreboot.org/c/coreboot/+/42124/3/src/drivers/spi/winbond.c File src/drivers/spi/winbond.c:
https://review.coreboot.org/c/coreboot/+/42124/3/src/drivers/spi/winbond.c@5... PS3, Line 503: .region_to_bpbits = winbond_region_to_bpbits, Why not just set the spi_flash_common functions here directly?