Nico Huber has posted comments on this change. ( https://review.coreboot.org/20513 )
Change subject: 4BA: Basic support for 4-bytes addressing mode extensions ......................................................................
Patch Set 2: Code-Review+1
(12 comments)
Just nits, style and non-functional issues we can also handle in a !fixup, I suppose. I can do that if nobody beats me to it. Or shall I edit this change and you ack it again?
https://review.coreboot.org/#/c/20513/2/flash.h File flash.h:
https://review.coreboot.org/#/c/20513/2/flash.h@124 PS2, Line 124: #define FEATURE_4BA_ONLY (1 << 11) nit, doesn't align well
https://review.coreboot.org/#/c/20513/1/spi4ba.c File spi4ba.c:
https://review.coreboot.org/#/c/20513/1/spi4ba.c@127 PS1, Line 127: + 1) - 1
I'd eventually like to switch to using dynamically allocated memory so we don't duplicate all this code between 3BA and 4BA versions. I implemented that in a pending patch for the CrOS branch: https://chromium-review.googlesource.com/c/337202/1 . But that can happen later if desired.
Actually we don't need dynamically allocated memory, just the offset for the memcpy() would have to be adjusted (and the buffer has to big enough ofc).
But some abstraction like your new_spi_write_command() also crossed my mind during this review.
https://review.coreboot.org/#/c/20513/2/spi4ba.c File spi4ba.c:
https://review.coreboot.org/#/c/20513/2/spi4ba.c@46 PS2, Line 46: nit, spurious space
https://review.coreboot.org/#/c/20513/2/spi4ba.c@74 PS2, Line 74: same here
https://review.coreboot.org/#/c/20513/2/spi4ba.c@77 PS2, Line 77: msg_cerr("%s failed during command execution\n", __func__); Don't know the coding style very well, in coreboot a single- line body shouldn't have braces.
https://review.coreboot.org/#/c/20513/2/spi4ba.c@84 PS2, Line 84: uint8_t databyte) Inconsistent alignment, sometimes two tabs are used, sometimes tabs+spaces, here tabs only.
https://review.coreboot.org/#/c/20513/2/spi4ba.c@100 PS2, Line 100: (addr & 0xff), `>> 0` used in other places
https://review.coreboot.org/#/c/20513/2/spi4ba.c@127 PS2, Line 127: + 1) - 1 I don't get what the +1-1 should tell the reader?
Ok, guess now I understand it: +1 for the 4th address byte -1 for the single byte that is included in the 256
Would be more clear if we'd define something like JEDEC_4_BYTE_ADDR_BYTE_PROGRAM_OUTSIZE (6) and JEDEC_4_BYTE_ADDR_PROGRAM_OUTSIZE (5).
(This is all ugly shit, IMHO. But not due to this patch but the spi25 code that obviously was used as a template. The size of the static parts of the command should be implicitly given by the data, IMO. Currently the code suffers from redundancy that makes it har- der to read, i.e. I trust the compiler to do the calculations but not developers.)
https://review.coreboot.org/#/c/20513/2/spi4ba.c@175 PS2, Line 175: uint8_t *bytes, unsigned int len) alignment
https://review.coreboot.org/#/c/20513/2/spi4ba.c@193 PS2, Line 193: unsigned int blocklen) weirdest alignment
https://review.coreboot.org/#/c/20513/2/spi4ba.c@237 PS2, Line 237: 32 Is this specified somewhere? IIRC, for 3BA it was inconsistent across chips.
https://review.coreboot.org/#/c/20513/2/spi4ba.c@327 PS2, Line 327: } flashrom the project of redundancy? I just reviewed the same function three times...
-
Nico Huber (Code Review)