Change in flashrom[master]: digilent_spi: add a driver for the iCEblink40 development board - flashrom-gerrit

23 Apr 2018

      Lubomir Rintel has posted comments on this change. ( https://review.coreboot.org/23338 )

Change subject: digilent_spi: add a driver for the iCEblink40 development board
......................................................................

Patch Set 5:

(5 comments)

https://review.coreboot.org/#/c/23338/4/digilent_spi.c
File digilent_spi.c:

https://review.coreboot.org/#/c/23338/4/digilent_spi.c@221
PS4, Line 221: }
...
  Looking at iceBurn, the meaning of the bytes seems
known? 2-byte […] Added some more checking of the response.

https://review.coreboot.org/#/c/23338/4/digilent_spi.c@230
PS4, Line 230: 	ret = do_command(req, sizeof(req), res, sizeof(res));
...
  This seems flawed or at least confuses me too much. If
I read iceBurn […] They got this wrong. In fact, they end up padding the read count
in the callers of __ICE40SPIPort.io() and then chop the padding from the response. E.g.:

    def read(self, addr, size):
        return self.io([self.CMD_FAST_READ...], size+5)[5:]

Eek. I suspect that they didn't grok this might be the reason iceBurn might not
support readback of the memory.

https://review.coreboot.org/#/c/23338/4/digilent_spi.c@236
PS4, Line 236: 		return -1;
...
  iceBurn pads with zeros, aiui. And not by `readcnt`
bytes but only […] Yes, but their read count already contains the write size -- see
above. They perhaps didn't realize it, or just didn't bother tidying up their code
after they did.

The 0x00 vs. 0xff doesn't seem to matter. I don't remember why i chose 0xff,
perhaps I've seen it in the trace from the proprietary digilent's tool.

https://review.coreboot.org/#/c/23338/4/digilent_spi.c@246
PS4, Line 246: 		if ((res[1] & 0x40) == 0) {
...
  Maybe we should check `tx_len` as well. True.
Done.

https://review.coreboot.org/#/c/23338/4/digilent_spi.c@276
PS4, Line 276: 
             : 	ret = spi_start_io(re
...
  These are about the payload size, i.e. data actually
read / written […] Raised to 252, empirically determined as the actual limit of the
payload the hardware can handle.

Not sure if an inner loop would significantly speed this up, I'd prefer keeping things
simple now.

-- 
To view, visit https://review.coreboot.org/23338
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7ffcd9a2db4395816f0e8b6ce6c3b0d8e930c9e6
Gerrit-Change-Number: 23338
Gerrit-PatchSet: 5
Gerrit-Owner: Lubomir Rintel &lt;lkundrak(a)v3.sk&gt;
Gerrit-Reviewer: Lubomir Rintel &lt;lkundrak(a)v3.sk&gt;
Gerrit-Reviewer: Nico Huber &lt;nico.h(a)gmx.de&gt;
Gerrit-Reviewer: Paul Menzel &lt;paulepanter(a)users.sourceforge.net&gt;
Gerrit-Reviewer: build bot (Jenkins) &lt;no-reply(a)coreboot.org&gt;
Gerrit-Comment-Date: Mon, 23 Apr 2018 21:15:40 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No