Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Neill Corlett has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61288 )
Change subject: Add mediatek_i2c_spi interface ......................................................................
Patch Set 2:
(20 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/61288/comment/06a1a6eb_3ddab73a PS1, Line 7: .
nit: no period at the end of commit summaries
Done
Patchset:
PS1:
Hi Neill, thanks for your patch. […]
- It definitely looks like there's an opportunity for code sharing with mstarddc_spi, but the ISP process for our specific chip (TSUMOP82JUQ) requires extra steps that aren't included in that driver. - We had two different board designs, one of which connects the TSUM* to SMBus and the other to raw I2C. This driver is tested on both. The finished product will be using I2C. I've left the SMBus code in as a convenience so we can continue to flash our earlier prototypes. - GPIO numbers are specific to TSUMOP82JUQ. My understanding is that these particular GPIO lines, for that chip, are standard for connecting to SPI WP#. Documentation that we received from PanVision just says to poke them and doesn't provide further detail.
Patchset:
PS2: Thanks for the review!
File mediatek_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/61288/comment/475f3f5b_28279868 PS1, Line 29: #define ISP_PORT 0x92 >> 1 : #define DEBUG_PORT 0xb2 >> 1
Please wrap these in parentheses to avoid operation order problems: […]
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/d0dbe83f_1f14c121 PS1, Line 36:
nit: double blank line
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/29bc701b_e2c4601b PS1, Line 55: uint8_t* buf
The `*` unary operator should have a space before, not after: […]
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/fccf3bbf_fb9ace94 PS1, Line 66: d
with ret value so we know how the ioctl actually failed.
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/7a94d3e0_17e64427 PS1, Line 96: args.data->block[0]
just 'len' here, avoids needing to do that indirection there to check we are in fact memcpy to a hea […]
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/299da758_d0f925a0 PS1, Line 96: args.data->block
args. […]
This is copying the data out of the i2c_smbus_data, starting at 1, using the size specified by [0].
https://review.coreboot.org/c/flashrom/+/61288/comment/669167ec_40980ef1 PS1, Line 181: 2
ARRAY_SIZE(data) […]
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/dfdae254_e4fba6a4 PS1, Line 211: 3
ARRAY_SIZE(data)
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/322fbf98_68d6b397 PS1, Line 242: GPIO line
+1
This is specific to the revision of chip that our board uses (TSUMOP82JUQ).
https://review.coreboot.org/c/flashrom/+/61288/comment/53248304_ee88dc61 PS1, Line 249: 0x426, 7
What are these magic numbers?
The TSUMOP82JUQ has a GPIO line going to WP# on SPI, and these two registers (0x426 and 0x428) control its state. We're following a ISP guide given to us by PanVision that doesn't provide any additional details.
https://review.coreboot.org/c/flashrom/+/61288/comment/0062d36f_660ae9bd PS1, Line 266: {
Please move the brace to the following line
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/8bad9221_a8d57a63 PS1, Line 269: unsigned char
This assumes that the size of `unsigned char` is always 1 byte. I'd use `uint8_t`.
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/a63d8c5f_bcbe12f3 PS1, Line 271: sizeof
Along with the changes above, this should then be `ARRAY_SIZE`
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/7024df48_d34a665a PS1, Line 295: 0x7F
Please use lowercase for hex
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/6a3bcdaf_82dcf852 PS1, Line 330: {
Please move the brace to the following line
Done
https://review.coreboot.org/c/flashrom/+/61288/comment/19e4635e_c9418481 PS1, Line 348: }
If both attempts fail, can we assume that entering ISP has failed?
Observed behavior is that this can fail if the chip is already in ISP mode.
https://review.coreboot.org/c/flashrom/+/61288/comment/75eef19a_33ee7eea PS1, Line 360: {
Please move the brace to the following line
Done
-
Neill Corlett (Code Review)