Patchwork dediprog: Use command 0x0B only on older SF100s

login
register
about
Submitter David Hendricks
Date 2015-08-02 22:09:40
Message ID <CAAAYX_irhAtcjOBxgOvR+aBSY9DS7dqOEEmJANyPznFa62CoTA@mail.gmail.com>
Download mbox | patch
Permalink /patch/4312/
State Accepted
Headers show

Comments

David Hendricks - 2015-08-02 22:09:40
As per e-mail with Dediprog, command 0x0B (which is not listed in the
command spec) is used to set voltage level on older Dediprogs. SF100
V6.0.0 and newer as well as all SF600 programmers do not support it.

This patch renames dediprog_device_init() to something more
appropriate  and adds comments for clarity, and only runs it
conditionally if we cannot query the devicestring during init.

Tested with SF100 V:5.1.5 and SF600 V:7.1.4 (with subsequent patches)
Signed-off-by: David Hendricks <dhendrix@chromium.org>

  msg_perr("Command A failed (%s)!\n", usb_strerror());
@@ -595,6 +598,7 @@
  msg_perr("Unexpected response to init!\n");
  return 1;
  }
+
  return 0;
 }

@@ -853,11 +857,15 @@
  if (register_shutdown(dediprog_shutdown, NULL))
  return 1;

- /* Perform basic setup. */
- if (dediprog_device_init())
- return 1;
- if (dediprog_check_devicestring())
- return 1;
+ /* Try reading the devicestring. If that fails and the device is old
+ * (FW < 6.0.0) then we need to try the "set voltage" command and then
+ * attempt to read the devicestring again. */
+ if (dediprog_check_devicestring()) {
+ if (dediprog_set_voltage())
+ return 1;
+ if (dediprog_check_devicestring())
+ return 1;
+ }

  /* Set all possible LEDs as soon as possible to indicate activity.
  * Because knowing the firmware version is required to set the LEDs
correctly we need to this after
David Woodhouse - 2015-08-03 23:50:25
On Sun, 2015-08-02 at 15:09 -0700, David Hendricks wrote:
> Tested with SF100 V:5.1.5 and SF600 V:7.1.4 (with subsequent patches)

Where are these subsequent patches of which you speak? I have SF600
testers who would be happy to try them out...
David Hendricks - 2015-08-04 07:37:19
On Mon, Aug 3, 2015 at 4:50 PM, David Woodhouse <dwmw2@infradead.org> wrote:

> On Sun, 2015-08-02 at 15:09 -0700, David Hendricks wrote:
> > Tested with SF100 V:5.1.5 and SF600 V:7.1.4 (with subsequent patches)
>
> Where are these subsequent patches of which you speak? I have SF600
> testers who would be happy to try them out...
>

I was just referring to the earlier patches that you and Stefan had posted
earlier (based off Simon Glass's work):
http://www.flashrom.org/pipermail/flashrom/2015-July/013759.html

<insert rant about SCM and code review process>
David Woodhouse - 2015-08-04 10:35:42
On Tue, 2015-08-04 at 00:37 -0700, David Hendricks wrote:
> 
> 
> On Mon, Aug 3, 2015 at 4:50 PM, David Woodhouse <dwmw2@infradead.org> 
> wrote:
> > On Sun, 2015-08-02 at 15:09 -0700, David Hendricks wrote:
> > > Tested with SF100 V:5.1.5 and SF600 V:7.1.4 (with subsequent 
> > patches)
> > 
> > Where are these subsequent patches of which you speak? I have SF600
> > testers who would be happy to try them out...
> I was just referring to the earlier patches that you and Stefan had 
> posted earlier (based off Simon Glass's work): 
> http://www.flashrom.org/pipermail/flashrom/2015-July/013759.html

Hm. I could have sworn I'd tried simply disabling the call to
dediprog_device_init(), and my SF600Plus tester (FW 7.1.1) still told
me it wasn't working — or that it was working for reads but not
erase/writes.

Am I missing something more that your patch does? 

> <insert rant about SCM and code review process>

Yeah. FWIW my set of patches is at
http://git.infradead.org/users/dwmw2/flashrom.git — it's not
*impossible* to work sanely, even if you are stuck with a project which
for some reason is still stuck in the 20th century with Subversion.
David Woodhouse - 2015-08-14 09:09:55
On Sun, 2015-08-02 at 15:09 -0700, David Hendricks wrote:
> 
> Tested with SF100 V:5.1.5 and SF600 V:7.1.4 (with subsequent patches)
> Signed-off-by: David Hendricks <dhendrix@chromium.org>

Can you push a git tree somewhere with those 'subsequent patches'
please? I know you said they're just the ones I posted, but I could
have sworn I'd *tried* what you're doing in this patch, and my testers
said it didn't work (I only have SF100 locally).

http://git.infradead.org/users/dwmw2/flashrom.git has my version of
those 'subsequent patches' — can you confirm that your patch works on
top of that?
David Hendricks - 2015-08-16 01:26:38
Voilà!

https://chromium-review.googlesource.com/#/c/293924/

Freed of the shackles of Subversion and Patchwork, maybe now I can get some
real work done on this thing.

On Fri, Aug 14, 2015 at 2:09 AM, David Woodhouse <dwmw2@infradead.org>
wrote:

> On Sun, 2015-08-02 at 15:09 -0700, David Hendricks wrote:
> >
> > Tested with SF100 V:5.1.5 and SF600 V:7.1.4 (with subsequent patches)
> > Signed-off-by: David Hendricks <dhendrix@chromium.org>
>
> Can you push a git tree somewhere with those 'subsequent patches'
> please? I know you said they're just the ones I posted, but I could
> have sworn I'd *tried* what you're doing in this patch, and my testers
> said it didn't work (I only have SF100 locally).
>
> http://git.infradead.org/users/dwmw2/flashrom.git has my version of
> those 'subsequent patches' — can you confirm that your patch works on
> top of that?
>
> --
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse@intel.com                              Intel Corporation
>
> _______________________________________________
> flashrom mailing list
> flashrom@flashrom.org
> http://www.flashrom.org/mailman/listinfo/flashrom
>
Stefan Tauner - 2016-01-24 22:44:10
On Sun, 2 Aug 2015 15:09:40 -0700
David Hendricks <dhendrix@chromium.org> wrote:

> As per e-mail with Dediprog, command 0x0B (which is not listed in the
> command spec) is used to set voltage level on older Dediprogs. SF100
> V6.0.0 and newer as well as all SF600 programmers do not support it.
> 
> This patch renames dediprog_device_init() to something more
> appropriate  and adds comments for clarity, and only runs it
> conditionally if we cannot query the devicestring during init.
> 
> Tested with SF100 V:5.1.5 and SF600 V:7.1.4 (with subsequent patches)
> Signed-off-by: David Hendricks <dhendrix@chromium.org>

As I said on gerrit, I would really like to know what this really does.
There is another function to change the SPI VCC so this is probably
something else...

Patch

Index: dediprog.c
===================================================================
--- dediprog.c (revision 1896)
+++ dediprog.c (working copy)
@@ -75,6 +75,7 @@ 
  CMD_READ_PROG_INFO = 0x08,
  CMD_SET_VCC = 0x09,
  CMD_SET_STANDALONE = 0x0A,
+ CMD_SET_VOLTAGE = 0x0B, /* Only in firmware older than 6.0.0 */
  CMD_GET_BUTTON = 0x11,
  CMD_GET_UID = 0x12,
  CMD_SET_CS = 0x14,
@@ -579,13 +580,15 @@ 
  return 0;
 }

-static int dediprog_device_init(void)
+/* Only use dediprog_set_voltage on SF100 programmers with firmware older
+ * than V6.0.0. Newer programmers (including all SF600s) do not support it. */
+static int dediprog_set_voltage(void)
 {
  int ret;
  char buf[0x1];

  memset(buf, 0, sizeof(buf));
- ret = usb_control_msg(dediprog_handle, REQTYPE_OTHER_IN, 0x0B, 0x0, 0x0,
+ ret = usb_control_msg(dediprog_handle, REQTYPE_OTHER_IN,
CMD_SET_VOLTAGE, 0x0, 0x0,
       buf, 0x1, DEFAULT_TIMEOUT);
  if (ret < 0) {