Patchwork Cache SPI RDID response

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2012-10-03 22:59:24
Message ID <506CC34C.9000300@gmx.net>
Download mbox | patch
Permalink /patch/3770/
State Not for merge
Headers show

Comments

Carl-Daniel Hailfinger - 2012-10-03 22:59:24
Not for merge... yet.
It works for dummy, nothing else was tested.
Limitations/bugs mentioned in the patch.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
David Hendricks - 2012-10-12 02:16:40
On Wed, Oct 3, 2012 at 3:59 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006@gmx.net> wrote:

> Not for merge... yet.
> It works for dummy, nothing else was tested.
> Limitations/bugs mentioned in the patch.
>
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
>
> Index: flashrom-spi_cache_rdid/spi.c
> ===================================================================
> --- flashrom-spi_cache_rdid/spi.c       (Revision 1611)
> +++ flashrom-spi_cache_rdid/spi.c       (Arbeitskopie)
> @@ -24,18 +24,46 @@
>
>  #include <strings.h>
>  #include <string.h>
> +#include <stdlib.h>
>  #include "flash.h"
>  #include "flashchips.h"
>  #include "chipdrivers.h"
>  #include "programmer.h"
>  #include "spi.h"
>
> +/* FIXME: We want a per-command cache, not just a RDID cache.
> + * FIXME: We should cache this for spi_send_multicommand programmers as
> well.
> + */
> +struct rdidcache {
> +       int available;
> +       unsigned char *readarr;
> +} rdidcache = {0};
> +
>  int spi_send_command(struct flashctx *flash, unsigned int writecnt,
>                      unsigned int readcnt, const unsigned char *writearr,
>                      unsigned char *readarr)
>  {
> -       return flash->pgm->spi.command(flash, writecnt, readcnt, writearr,
> +       int ret;
> +       unsigned char *tmp;
> +
> +       if ((writearr[0] == JEDEC_RDID) && (rdidcache.available >=
> readcnt)) {
> +               memcpy(readarr, rdidcache.readarr, readcnt);
> +               return 0;
> +       }
> +       ret = flash->pgm->spi.command(flash, writecnt, readcnt, writearr,
>                                        readarr);
> +       if (!ret && (writearr[0] == JEDEC_RDID) && (rdidcache.available <
> readcnt)) {
> +               tmp = realloc(rdidcache.readarr, readcnt);
> +               if (!tmp) {
> +                       /* Oh well. Don't cache stuff, then. No problem. */
> +                       msg_perr("Doom due to OOM! Brace for impact!\n");
> +                       return ret;
> +               }
> +               rdidcache.readarr = tmp;
> +               rdidcache.available = readcnt;
> +               memcpy(rdidcache.readarr, readarr, readcnt);
> +       }
> +       return ret;
>  }
>
>  int spi_send_multicommand(struct flashctx *flash, struct spi_command
> *cmds)
>

Interesting approach. However, I think 3-byte vs. 4-byte RDID commands and
caching REMS might make patching spi_send_command rather messy.

I made a patch that takes a different route by changing
probe_spi_{rdid,rdid4,rems} functions instead. My patch can be viewed via
Gerrit on Chromium.org @
https://gerrit.chromium.org/gerrit/#/c/35376/2 (doesn't
apply cleanly against upsteam currently).
Stefan Tauner - 2012-10-12 02:44:25
On Thu, 11 Oct 2012 19:16:40 -0700
David Hendricks <dhendrix@google.com> wrote:

> On Wed, Oct 3, 2012 at 3:59 PM, Carl-Daniel Hailfinger <
> c-d.hailfinger.devel.2006@gmx.net> wrote:
> 
> > Not for merge... yet.
> > It works for dummy, nothing else was tested.
> > Limitations/bugs mentioned in the patch.
> >
>
> Interesting approach. However, I think 3-byte vs. 4-byte RDID commands and
> caching REMS might make patching spi_send_command rather messy.
> 
> I made a patch that takes a different route by changing
> probe_spi_{rdid,rdid4,rems} functions instead. My patch can be viewed via
> Gerrit on Chromium.org @
> https://gerrit.chromium.org/gerrit/#/c/35376/2 (doesn't
> apply cleanly against upsteam currently).
> 

Much better, but still wrong :) We work around a stupid probing loop
instead fixing the root cause (verbose prints will still be way too
verbose with this patch). If there are good reasons to do it this way,
then i have no problem with it, but if we just hack this into the SPI
code because it is easier to implement and come to a consensus, then
i'll make the latter hard :P

Fixing the probing loop has been on my todo list for a long time and i
will work on it as soon as the other architectural changes are merged
(status register stuff, check_trans etc). We should postpone the
discussion until then IMHO. I suggest that chromium uses David's method
till then and someone reviews my other patches soon ;) OTOH it would
not hurt to integrate this into upstream with one exception:
it would introdcue even more conflicts between open patches:

David: some of this heavily conflicts with my "Generify
probe_spi_rdid_generic() and add probe_spi_rdid_edi()." and other
patches from that set. Maybe it would be better if you leave out the
compare_id() introduction to get less conflicts later.
David Hendricks - 2012-10-12 03:43:48
On Thu, Oct 11, 2012 at 7:44 PM, Stefan Tauner <
stefan.tauner@student.tuwien.ac.at> wrote:

> On Thu, 11 Oct 2012 19:16:40 -0700
> David Hendricks <dhendrix@google.com> wrote:
>
> > On Wed, Oct 3, 2012 at 3:59 PM, Carl-Daniel Hailfinger <
> > c-d.hailfinger.devel.2006@gmx.net> wrote:
> >
> > > Not for merge... yet.
> > > It works for dummy, nothing else was tested.
> > > Limitations/bugs mentioned in the patch.
> > >
> >
> > Interesting approach. However, I think 3-byte vs. 4-byte RDID commands
> and
> > caching REMS might make patching spi_send_command rather messy.
> >
> > I made a patch that takes a different route by changing
> > probe_spi_{rdid,rdid4,rems} functions instead. My patch can be viewed via
> > Gerrit on Chromium.org @
> > https://gerrit.chromium.org/gerrit/#/c/35376/2 (doesn't
> > apply cleanly against upsteam currently).
> >
>
> Much better, but still wrong :) We work around a stupid probing loop
> instead fixing the root cause (verbose prints will still be way too
> verbose with this patch). If there are good reasons to do it this way,
> then i have no problem with it, but if we just hack this into the SPI
> code because it is easier to implement and come to a consensus, then
> i'll make the latter hard :P
>
> Fixing the probing loop has been on my todo list for a long time and i
> will work on it as soon as the other architectural changes are merged
> (status register stuff, check_trans etc). We should postpone the
> discussion until then IMHO. I suggest that chromium uses David's method
> till then and someone reviews my other patches soon ;) OTOH it would
> not hurt to integrate this into upstream with one exception:
> it would introdcue even more conflicts between open patches:
>
> David: some of this heavily conflicts with my "Generify
> probe_spi_rdid_generic() and add probe_spi_rdid_edi()." and other
> patches from that set. Maybe it would be better if you leave out the
> compare_id() introduction to get less conflicts later.
>

Haha, totally agreed :-) We really should make the probe loop smarter about
caching results rather than hacking this into the chip code.

You also have a good point about conflicts in my patch. Though there will
be plenty of conflicts without the compare_id() stuff anyway... I'll just
revert my change when we get a proper method implemented.

Patch

Index: flashrom-spi_cache_rdid/spi.c
===================================================================
--- flashrom-spi_cache_rdid/spi.c	(Revision 1611)
+++ flashrom-spi_cache_rdid/spi.c	(Arbeitskopie)
@@ -24,18 +24,46 @@ 
 
 #include <strings.h>
 #include <string.h>
+#include <stdlib.h>
 #include "flash.h"
 #include "flashchips.h"
 #include "chipdrivers.h"
 #include "programmer.h"
 #include "spi.h"
 
+/* FIXME: We want a per-command cache, not just a RDID cache.
+ * FIXME: We should cache this for spi_send_multicommand programmers as well.
+ */
+struct rdidcache {
+	int available;
+	unsigned char *readarr;
+} rdidcache = {0};
+
 int spi_send_command(struct flashctx *flash, unsigned int writecnt,
 		     unsigned int readcnt, const unsigned char *writearr,
 		     unsigned char *readarr)
 {
-	return flash->pgm->spi.command(flash, writecnt, readcnt, writearr,
+	int ret;
+	unsigned char *tmp;
+
+	if ((writearr[0] == JEDEC_RDID) && (rdidcache.available >= readcnt)) {
+		memcpy(readarr, rdidcache.readarr, readcnt);
+		return 0;
+	}
+	ret = flash->pgm->spi.command(flash, writecnt, readcnt, writearr,
 				       readarr);
+	if (!ret && (writearr[0] == JEDEC_RDID) && (rdidcache.available < readcnt)) {
+		tmp = realloc(rdidcache.readarr, readcnt);
+		if (!tmp) {
+			/* Oh well. Don't cache stuff, then. No problem. */
+			msg_perr("Doom due to OOM! Brace for impact!\n");
+			return ret;
+		}
+		rdidcache.readarr = tmp;
+		rdidcache.available = readcnt;
+		memcpy(rdidcache.readarr, readarr, readcnt);
+	}
+	return ret;
 }
 
 int spi_send_multicommand(struct flashctx *flash, struct spi_command *cmds)