Attention is currently required from: ChrisEric1 CECL, Nicholas Chin.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72057 )
Change subject: Add support for VIA VL805 USB 3 XHCI flashing ......................................................................
Patch Set 3:
(14 comments)
Patchset:
PS3: Hi Cristopher, fist of all: Thank you for contributing! This looks already quite good. Nevertheless I've a few thinks (most of them are quite small):
* Can you move the changes in `flashchips.c` into an own commit * The entry for the meson buildsystem ist missing (meson.build:148ff, in alphabetic order) ``` 'vl805' : { 'systems' : systems_hwaccess, 'deps' : [ libpci ], 'groups' : [ group_pci, group_internal ], 'srcs' : files('vl805.c', 'pcidev.c'), 'flags' : [ '-DCONFIG_VL805=1' ], }, ``` * Can you share a little bit about the 'original driver' and the 'logs' you mentioned in the comments?
File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/72057/comment/dc08e98d_90a1f463 PS3, Line 1371: VL806 Is there a different between VL805 and VL806? This is the only mention of the second chip
File vl805.c:
https://review.coreboot.org/c/flashrom/+/72057/comment/c7b0fe3b_e32443d1 PS3, Line 4: * Copyright (C) 2019, 2020 Carl-Daniel Hailfinger This is your copyright
https://review.coreboot.org/c/flashrom/+/72057/comment/cbe1f3cd_9fbc68df PS3, Line 27: //#include "hwaccess.h" Please remove this comment.
https://review.coreboot.org/c/flashrom/+/72057/comment/e6bcdfea_568b951f PS3, Line 28: #include "hwaccess_x86_io.h" This header is not needed, because the programmer doesn't do anything with I/O ports. (see line 137)
https://review.coreboot.org/c/flashrom/+/72057/comment/718912f2_570babfc PS3, Line 31: extern const struct dev_entry devs_vl805[]; This is not needed.
https://review.coreboot.org/c/flashrom/+/72057/comment/ba24dc43_400b35b0 PS3, Line 33: const struct dev_entry devs_vl805[] = { Make this `static`. It is only used in this file.
https://review.coreboot.org/c/flashrom/+/72057/comment/52f5f643_97b62c1e PS3, Line 38: static struct pci_dev *dev = NULL; Can you please move this global into a programmer data struct, like it is done on e.g. the it8212.c programmer.
https://review.coreboot.org/c/flashrom/+/72057/comment/1700ac6a_23201f0d PS3, Line 53: /* Some of the registers have unknown purpose and are just used inside the init sequence replay. */ Can you put these defines below the includes, ontop of all functions?
https://review.coreboot.org/c/flashrom/+/72057/comment/76a7c3b1_21a76f19 PS3, Line 72: uint32_t indata = 0; : unsigned int curwritecnt = 0; : unsigned int curreadcnt = 0; These variables don't need to be initialized.
https://review.coreboot.org/c/flashrom/+/72057/comment/6b8c3f70_3c5c602b PS3, Line 107: static const struct spi_master spi_master_vl805 = { Can this controller handle SPI flashes with 4-byte addresses? Then you should add `.features = SPI_MASTER_4BA`
https://review.coreboot.org/c/flashrom/+/72057/comment/531b4192_a4ffa1d1 PS3, Line 131: int vl805_init(const struct programmer_cfg *cfg); This prototype is not needed if the function is static.
https://review.coreboot.org/c/flashrom/+/72057/comment/6baa6082_11900120 PS3, Line 133: int vl805_init(const struct programmer_cfg *cfg) this should be `static`
https://review.coreboot.org/c/flashrom/+/72057/comment/d9f74b91_a1b81306 PS3, Line 137: #if defined(__i386__) || defined(__x86_64__) : if (rget_io_perms()) : return 1; : #endif This can be removed since the programmer doesn't deal with I/O ports
-
Thomas Heijligen (Code Review)