Attention is currently required from: qianfan, Angel Pons.
Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70529 )
Change subject: Add initial CH347T SPI programmer ......................................................................
Patch Set 3:
(11 comments)
Commit Message:
PS3: It would probably be good to specify that this only supports mode 1 (vendor protocol) right now and not mode 2 (HID protocol). It would also be good to specify what settings are currently hard coded, such as the clock speed, spi mode, and that this uses CS0.
File ch347t_spi.c:
https://review.coreboot.org/c/flashrom/+/70529/comment/bd27da55_992ba351 PS2, Line 34: CH347_CMD_SPI_CONTROL, Maybe make it more clear that this controls the CS lines?
File ch347t_spi.c:
https://review.coreboot.org/c/flashrom/+/70529/comment/29aaace6_bf9ff960 PS3, Line 30: #define CH347_MAX_DATA_READ 4096 This probably could be set to a higher value. I've tried it with a max read size of 64KiB (64*1024) and it saves about a second with a 16MiB flash chip. The read command takes in a 32 bit integer to technically this could be 4GiB, though that would be impractical due to memory constraints.
https://review.coreboot.org/c/flashrom/+/70529/comment/e5f39547_68b349d0 PS3, Line 41: enum spi_prescaler { : SPI_BAUDRATEPRESCALER_2 = 0x00, : SPI_BAUDRATEPRESCALER_4 = 0x08, : SPI_BAUDRATEPRESCALER_8 = 0x10, : SPI_BAUDRATEPRESCALER_16 = 0x18, : SPI_BAUDRATEPRESCALER_32 = 0x20, : SPI_BAUDRATEPRESCALER_64 = 0x28, : SPI_BAUDRATEPRESCALER_128 = 0x30, : SPI_BAUDRATEPRESCALER_256 = 0x38 : }; Alternatively the prescaler values could start from 1 and then the formula would be freq = 60 MHz / prescaler
https://review.coreboot.org/c/flashrom/+/70529/comment/21d6553b_4d1f6fbc PS3, Line 84: enum spi_nss { : SPI_NSS_SOFT = 0x0200, : SPI_NSS_HARD = 0x0000 : }; I would rename these to be clearer what it does, maybe `enum spi_cs`, `SPI_CS_SOFTWARE`, and `SPI_CS_HARDWARE`.
https://review.coreboot.org/c/flashrom/+/70529/comment/77fa3553_9f7465e0 PS3, Line 179: buf, n + 3, This may go over the 512 byte max packet size of the write endpoint since n can be up to 512 due to the previous code. That said, I have seen the vendor library attempt to send over 700 bytes in one packet...
https://review.coreboot.org/c/flashrom/+/70529/comment/c3d94cd2_683d8bfc PS3, Line 330: ch347t_spi_spi_send_command The two "spi"s are redundant
https://review.coreboot.org/c/flashrom/+/70529/comment/6ca21b37_d0fb5d29 PS3, Line 367: libusb_release_interface(handle, 0); : libusb_attach_kernel_driver(handle, 0); Interface 2, see comment in `ch347t_spi_init`
https://review.coreboot.org/c/flashrom/+/70529/comment/93dcfb48_99039daa PS3, Line 420: 0 Mode 1 uses interface 2, and the programmer currently fails if the vendor kernel driver is loaded.
https://review.coreboot.org/c/flashrom/+/70529/comment/246a7deb_2b375514 PS3, Line 425: 0 Interface 2
https://review.coreboot.org/c/flashrom/+/70529/comment/caadb730_61eeb329 PS3, Line 442: release_interface: : libusb_release_interface(handle, 0); : close_handle: : libusb_attach_kernel_driver(handle, 0); Interface 2
-
Nicholas Chin (Code Review)