Attention is currently required from: Thomas Heijligen, Nicholas Chin.
ChrisEric1 CECL 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:
(15 comments)
Patchset:
PS3:
Hi Cristopher, […]
Hello!
1. I can move the IS25LP080D part that was added in another commit, but since I don't actually have the chip, I cannot test it anyways, so I am just going to remove it, unless someone reports that its needed for another machine.
2. Would something like this work?
... 'srcs' : files('usbblaster_spi.c'), 'flags' : [ '-DCONFIG_USBBLASTER_SPI=1' ], }, 'vl805' : { 'systems' : systems_hwaccess, 'deps' : [ libpci ], 'groups' : [ group_pci, group_internal ], 'srcs' : files('vl805.c', 'pcidev.c'), 'flags' : [ '-DCONFIG_VL805=1' ], }, }
active_programmer_count = 0 foreach p_name, p_data : programmer ...
3. This driver is based off of the work from https://github.com/CE1CECL/flashrom-vl805/commit/1448afa15f9a8dddb3e539349e2... for flashrom v1.1, which has since been ported to mainline by me. However, the patch set that you found earlier didn't actually work for me on my Raspberry Pi 4 Model B, pretty sure it was just missing the RDID4 thing. What even is RDID4 for anyways? Does SPI_MASTER_4BA solve it? (I'm not home at the moment, will need to test later today). I did actually not keep my logs for my errors but IIRC it said something like it found an unknown chip and it was in unknown state? Either way, the RDID4 fixed it for me at least.
I will mark the comments I was able to solve as resolved though since I'm not some, I am only compiling it, making sure it builds.
PS3:
Original patch CB:50264
This is the non-working patch I mentioned earlier.
File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/72057/comment/83326430_52c97c3f PS3, Line 1371: VL806
Is there a different between VL805 and VL806? This is the only mention of the second chip
According to https://www.via-labs.com/product_show.php?id=48 they are the same, but I don't have a vl806, only a vl805, so I removed the vl806 mention in the next patch.
File vl805.c:
https://review.coreboot.org/c/flashrom/+/72057/comment/21384537_19d4d7e5 PS3, Line 4: * Copyright (C) 2019, 2020 Carl-Daniel Hailfinger
This is your copyright
Fixed in next patch.
https://review.coreboot.org/c/flashrom/+/72057/comment/96c14bff_0643c450 PS3, Line 27: //#include "hwaccess.h"
Please remove this comment.
I wasn't sure what it was for so I commented it out, next patch will remove it.
https://review.coreboot.org/c/flashrom/+/72057/comment/3849653b_f6c9019c PS3, Line 28: #include "hwaccess_x86_io.h"
This header is not needed, because the programmer doesn't do anything with I/O ports. […]
Fixed in next patch, although it does say the vl805 FW version, it might be part of SPI.
https://review.coreboot.org/c/flashrom/+/72057/comment/7531093b_a1821826 PS3, Line 31: extern const struct dev_entry devs_vl805[];
This is not needed.
Fixed in next patch.
https://review.coreboot.org/c/flashrom/+/72057/comment/023fa7cf_f7c6f764 PS3, Line 33: const struct dev_entry devs_vl805[] = {
Make this `static`. It is only used in this file.
Fixed in next patch, which is coming later sometime today.
https://review.coreboot.org/c/flashrom/+/72057/comment/559ce626_75b7849c 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. […]
Cannot do that, this part is outside of the global which needs it: Sorry, had a hard time formatting, so it will look weird in gerrit.
static struct dev_entry devs_vl805[] = { {0x1106, 0x3483, OK, "VIA", "VL805"}, {0}, };
https://review.coreboot.org/c/flashrom/+/72057/comment/59a9cca0_978f7121 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?
Done, will be in next patch.
https://review.coreboot.org/c/flashrom/+/72057/comment/cfd98314_2dda6ba3 PS3, Line 72: uint32_t indata = 0; : unsigned int curwritecnt = 0; : unsigned int curreadcnt = 0;
These variables don't need to be initialized.
I don't understand what you are trying to say, if you remove the highlighted part, it fails to build, unless you mean to rework it?
https://review.coreboot.org/c/flashrom/+/72057/comment/77242e19_7fd4cf05 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 `. […]
I have no clue right now, but I added it in to see if it works later.
https://review.coreboot.org/c/flashrom/+/72057/comment/aa723886_7d0d2e84 PS3, Line 131: int vl805_init(const struct programmer_cfg *cfg);
This prototype is not needed if the function is static.
Removed in next patch.
https://review.coreboot.org/c/flashrom/+/72057/comment/db7629bf_2c0785cf PS3, Line 133: int vl805_init(const struct programmer_cfg *cfg)
this should be `static`
`static int vl805_init(const struct programmer_cfg *cfg)` in next patch
https://review.coreboot.org/c/flashrom/+/72057/comment/44753936_ec44152a 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
Fixed in next patch
-
ChrisEric1 CECL (Code Review)