On Wed, 27 Jan 2016 03:12:11 +0200 Urja Rannikko <[email protected]> wrote:
> Hi, > > Review-ish... with some only-commentary included too. > > On Mon, Jan 18, 2016 at 1:28 AM, Stefan Tauner > <[email protected]> wrote: > > Signed-off-by: Urja Rannikko <[email protected]> > > Signed-off-by: Stefan Tauner <[email protected]> > > --- > > 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 <[email protected]> Thanks, r1921. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
