[flashrom] [PATCH 3/3] Add support for WCH CH341A as an SPI programmer.

Stefan Tauner stefan.tauner at alumni.tuwien.ac.at
Sun Jan 31 23:16:12 CET 2016


On Wed, 27 Jan 2016 03:12:11 +0200
Urja Rannikko <urjaman at gmail.com> wrote:

> Hi,
> 
> Review-ish... with some only-commentary included too.
> 
> On Mon, Jan 18, 2016 at 1:28 AM, Stefan Tauner
> <stefan.tauner at alumni.tuwien.ac.at> wrote:
> > Signed-off-by: Urja Rannikko <urjaman at gmail.com>
> > Signed-off-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
> > ---
> >  Makefile        |  23 ++-
> >  ch341a_spi.c    | 531 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  flashrom.8.tmpl |  12 +-
> >  flashrom.c      |  12 ++
> >  programmer.h    |  13 ++
> >  5 files changed, 588 insertions(+), 3 deletions(-)
> >  create mode 100644 ch341a_spi.c
> >
> > +                       free_idx = (free_idx + 1) % USB_IN_TRANSFERS; /* Increment (and wrap around). */
> Yeah this obvious construct is something I didnt use because I'm such
> an embedded guy that I'd never ever use % in performance related
> code... (i think i had some if idx>=USB_IN_TRANSFERS) idx = 0
> thing)... thanks for making it saner.

:)

> I had a fancier delay implementation done (all the way upto ms scale
> using the I2C commands), but since i didnt get that fully tested, this
> one will do, i'll post it as an improvement later.

Please do. I don't deem it that important but since you have
investigated the whole issue in such detail it would be a waste to not
exploit this knowledge.

> > +       unsigned int i;
> > +       for (i = 0; i < readcnt; i++) {
> > +               /* FIXME: does working on words instead of bytes improve speed? */
> ... drop the comment? i dont really care, but i think our consensus on
> IRC was that a words-wide implementation would have more issues
> (alignment and such) than performance.
> Or make a comment with that info.

It's gone.

> > +               *readarr++ = swap_byte(rbuf[writecnt + i]);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct spi_master spi_master_ch341a_spi = {
> > +       .type           = SPI_CONTROLLER_CH341A_SPI,
> > +       /* TODO: flashrom's current maximum is 256 B. Device was tested on Linux to accept atleast 16 kB. */
> > +       .max_data_read  = 16 * 1024,
> > +       .max_data_write = 16 * 1024,
> On linux. Not on windows usb "stack" i think... set these back to 1k?
> Or test for the limit (maybe something like 4k?) and change the 1000ms
> timeout, since i think the big packet would timeout on windows if
> nothing else failed...

As discussed, I have tested up to 128 kB on Windows (in VM) and on
Linux with a bigger timeout and it works fine. We have agreed upon 4 kB
chunks now and using VLAs (I had an intermediate version that used
bigger chunks and relied on mallocs for them).

> With atleast the limits dropped (or tested),  this is:
> Acked-by: Urja Rannikko <urjaman at gmail.com>

Thanks, r1921.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner




More information about the flashrom mailing list