Nico Huber has posted comments on this change. ( https://review.coreboot.org/22020 )
Change subject: spi25: Add native 4BA support ......................................................................
Patch Set 1:
(2 comments)
The code here looks fine, I just want to check my understanding of how you're deciding whether to use 4BA native instructions or extended address mode. As you're probably already aware, some chips support one but not the other, so we need to be a little careful.
I target to have all information (excluding erase instructions) about supported instructions in the feature bits.
From what I've seen, it's safer to use native 4BA instructions when they're available since they should work no matter what mode the chip is in.
This is exactly what (I think) I'm doing in the next patch. e.g. `FEATURE_4BA_READ` means the native 4BA read instruction is sup- ported and if it's set, we just use that. It only works, though, if `FEATURE_4BA_SUPPORT` is set too and `FEATURE_4BA_EXT_ADDR` is not. I have to admit I'm a bit clueless about the latter, e.g. are there chips that support an extended address register and native 4BA instructions? If yes, or if we just want to pre- pare for it, spi_prepare_address() will need a parameter to override the default choice, I guess.
Also, we should consider how we might override the default behavior in case the programmer can only operate in a particular mode (I'm thinking Intel chipsets and Dediprog programmers).
My preferred solution is to have more feature bits. One for each sup- ported instruction if need be. The programmer driver can choose for itself then what to do (or to bail out).
https://review.coreboot.org/#/c/22020/1/flash.h File flash.h:
https://review.coreboot.org/#/c/22020/1/flash.h@123 PS1, Line 123: #define FEATURE_4BA_EXT_ADDR (1 << 11) I shall comment the feature bits in my next iteration. This one:
/* Regular 3-byte operations can be used by writing the most signifcant address byte into an extended address register. */
https://review.coreboot.org/#/c/22020/1/spi25.c File spi25.c:
https://review.coreboot.org/#/c/22020/1/spi25.c@363 PS1, Line 363: flash->address_high_byte = addr_high;
The follow-up patch seems to use native 4BA instructions for read and write
On certain feature-bit combinations this can indeed be a problem.
-
Nico Huber (Code Review)