Attention is currently required from: Thomas Heijligen, Nicholas Chin.

View Change

15 comments:

• Patchset:

  • Patch Set #3:

    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/1448afa15f9a8dddb3e539349e2697996f3c72b2 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.

  • Patch Set #3:

    Original patch CB:50264

    This is the non-working patch I mentioned earlier.

• File flashrom.8.tmpl:

  • Patch Set #3, 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:

  • Patch Set #3, Line 4: * Copyright (C) 2019, 2020 Carl-Daniel Hailfinger

    This is your copyright

    Fixed in next patch.

  • Patch Set #3, 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.

  • Patch Set #3, 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.

  • Patch Set #3, Line 31: extern const struct dev_entry devs_vl805[];

    This is not needed.

    Fixed in next patch.

  • Patch Set #3, 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.

  • Patch Set #3, 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},
    };

  • Patch Set #3, 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.

  • Patch Set #3, 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?

  • Patch Set #3, 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.

  • Patch Set #3, Line 131: int vl805_init(const struct programmer_cfg *cfg);

    This prototype is not needed if the function is static.

    Removed in next patch.

  • Patch Set #3, 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

  • Patch Set #3, 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

To view, visit change 72057. To unsubscribe, or for help writing mail filters, visit settings.