Nico Huber has posted comments on this change. ( https://review.coreboot.org/20505 )
Change subject: 4BA: Flashrom integration for the 4-bytes addressing extensions ......................................................................
Patch Set 3:
(6 comments)
Rather clumsy. I don't like to do invasive edits on other people's commits. Will write a follow-up, not sure if any line will stay.
https://review.coreboot.org/#/c/20505/3/flash.h File flash.h:
https://review.coreboot.org/#/c/20505/3/flash.h@176 PS3, Line 176: } four_bytes_addr_funcs; This makes 4BA look like an alien...
https://review.coreboot.org/#/c/20505/3/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/20505/3/flashrom.c@2227 PS3, Line 2227: if(flash->chip->feature_bits & FEATURE_4BA_SUPPORT) { missing space after `if`
https://review.coreboot.org/#/c/20505/3/flashrom.c@2240 PS3, Line 2240: if(flash->chip->four_bytes_addr_funcs.enter_4ba(flash)) { missing space
https://review.coreboot.org/#/c/20505/3/flashrom.c@2253 PS3, Line 2253: { Hmmm, what if another master expects the chip to be in 3BA mode?
https://review.coreboot.org/#/c/20505/3/spi.c File spi.c:
https://review.coreboot.org/#/c/20505/3/spi.c@115 PS3, Line 115: if (!(flash->chip->feature_bits & FEATURE_4BA_SUPPORT) && What if 4BA is supported? Shouldn't we do the same check with `1 << 32`?
(I can't tell. The whole `addrbase` thing looks like a layering violation to me that should be handled by programmers that actually do map the chip.)
https://review.coreboot.org/#/c/20505/3/spi25.c File spi25.c:
https://review.coreboot.org/#/c/20505/3/spi25.c@1019 PS3, Line 1019: rc = (flash->chip->feature_bits & FEATURE_4BA_SUPPORT) == 0 The condition should be checked in the called function, IMHO. This is too ugly...