Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41750 )
Change subject: spi/macronix: Add support for boot media protection ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c File src/drivers/spi/macronix.c:
https://review.coreboot.org/c/coreboot/+/41750/5/src/drivers/spi/macronix.c@... PS5, Line 335: macronix_set_write_protection(const struct spi_flash *flash, Not really happy about duplicating so much logic for every vendor. Isn't there some way we can unify this? What are the actual differences between the Macronix and Winbond protection schemes? To me it just looks like they're naming things a bit different (e.g. what Winbond calls the CMP bit is essentially just the highest BP bit in Macronix-lingo) and some chips only support a subset (but that's more between different chips of the same vendor than between vendors... for example, the TB bit you're not implementing here is actually supported on some Macronix chips like MX25V1635F, just not the ones you're adding right now).
I think this is going to be a giant mess if we try to keep block protection code completely vendor specific and keep adding it for more and more vendors. We only just spent a lot of effort to merge and unify vendor-specific code recently (e.g. patches ending in CB:38379), I'd hate to add it all back for block protection stuff now.
Is there any way you could manage to encode the differences between these in cofiguration structures and factor out code that works for both of them?