Change in flashrom[master]: buspirate: Add option for setting the aux pin

[Sean Anderson (Code Review)]

Sean Anderson has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43608 )

Change subject: buspirate: Add option for setting the aux pin
......................................................................

buspirate: Add option for setting the aux pin

This adds a parameter to drive the aux pin low (or high if you
explicitly want the previous behavior). Some boards need to have a reset
pin driven low before the firmware can be safely flashed. With the Bus
Pirate, this is most easily done with the auxiliary pin.

Change-Id: Ieeecfdf1afc06dadda9b8f99547cd74854ca6775
Signed-off-by: Sean Anderson 
---
M buspirate_spi.c
M flashrom.8.tmpl
2 files changed, 33 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/43608/1

diff --git a/buspirate_spi.c b/buspirate_spi.c
index fdfc0e4..1ccb107 100644
--- a/buspirate_spi.c
+++ b/buspirate_spi.c
@@ -228,6 +228,7 @@
    int serialspeed_index = -1;
    int ret = 0;
    int pullup = 0;
+	int aux = 1;

dev = extract_programmer_param("dev");
    if (dev && !strlen(dev)) {
@@ -277,6 +278,17 @@
    }
    free(tmp);

+	tmp = extract_programmer_param("aux");
+	if (tmp) {
+		if (strcasecmp("high", tmp) == 0)
+			; /* Default */
+		else if (strcasecmp("low", tmp) == 0)
+			aux = 0;
+		else
+			msg_perr("Invalid AUX state, driving high by default.\n");
+	}
+	free(tmp);
+
    /* Default buffer size is 19: 16 bytes data, 3 bytes control. */
 #define DEFAULT_BUFSIZE (16 + 3)
    bp_commbuf = malloc(DEFAULT_BUFSIZE);
@@ -520,11 +532,18 @@
    }

/* Initial setup (SPI peripherals config): Enable power, CS high, AUX */
-	bp_commbuf[0] = 0x40 | 0x0b;
+	bp_commbuf[0] = 0x40 | 0x09;
    if (pullup == 1) {
    	bp_commbuf[0] |= (1 << 2);
    	msg_pdbg("Enabling pull-up resistors.\n");
    }
+	if (aux) {
+		bp_commbuf[0] |= (1 << 1);
+		msg_pdbg("Driving AUX high.\n");
+	} else {
+		msg_pdbg("Driving AUX low.\n");
+	}
+
    ret = buspirate_sendrecv(bp_commbuf, 1, 1);
    if (ret)
    	return 1;
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
index db50d59..caf860c 100644
--- a/flashrom.8.tmpl
+++ b/flashrom.8.tmpl
@@ -902,6 +902,19 @@
 .URLB "http://dangerousprototypes.com/docs/Practical_guide_to_Bus_Pirate_pull-up_re..." \
 "in a guide by dangerousprototypes" .
 Only the external supply voltage (Vpu) is supported as of this writing.
+.sp
+An optional aux parameter specifies the state of the Bus Pirate auxiliary pin.
+This may be used to drive the auxiliary pin high or low before a transfer.
+Syntax is
+.sp
+.B "  flashrom -p buspirate_spi:aux=state"
+.sp
+where
+.B state
+can be
+.BR high " or " low .
+The default
+.BR state " is " high .
 .SS
 .BR "pickit2_spi " programmer
 .IP

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43608 )

Change subject: buspirate: Add option for setting the aux pin
......................................................................

Patch Set 1: Code-Review+1

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43608 )

Change subject: buspirate: Add option for setting the aux pin
......................................................................

Patch Set 1: Code-Review+1

(3 comments)

https://review.coreboot.org/c/flashrom/+/43608/1/buspirate_spi.c 
File buspirate_spi.c:

https://review.coreboot.org/c/flashrom/+/43608/1/buspirate_spi.c@284 
PS1, Line 284: ; /* Default */
Nit: I'd prefer to explicitly write a value to `aux` here, instead of using the value programmed 50 lines ago. Looks like the pullups code does something like that, though.

https://review.coreboot.org/c/flashrom/+/43608/1/buspirate_spi.c@290 
PS1, Line 290: free(tmp);
I'd put this inside the if-block, but I see no one else does it here...

https://review.coreboot.org/c/flashrom/+/43608/1/buspirate_spi.c@534 
PS1, Line 534: AUX
AUX is handled later on now

[Sean Anderson (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/43608

to look at the new patch set (#2).

Change subject: buspirate: Add option for setting the aux pin
......................................................................

buspirate: Add option for setting the aux pin

This adds a parameter to drive the aux pin low (or high if you
explicitly want the previous behavior). Some boards need to have a reset
pin driven low before the firmware can be safely flashed. With the Bus
Pirate, this is most easily done with the auxiliary pin.

Change-Id: Ieeecfdf1afc06dadda9b8f99547cd74854ca6775
Signed-off-by: Sean Anderson 
---
M buspirate_spi.c
M flashrom.8.tmpl
2 files changed, 35 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/43608/2

[Sean Anderson (Code Review)]

Sean Anderson has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43608 )

Change subject: buspirate: Add option for setting the aux pin
......................................................................

Patch Set 2:

(2 comments)

Ok, changed in response to comments.

https://review.coreboot.org/c/flashrom/+/43608/1/buspirate_spi.c 
File buspirate_spi.c:

https://review.coreboot.org/c/flashrom/+/43608/1/buspirate_spi.c@284 
PS1, Line 284: ; /* Default */
> Nit: I'd prefer to explicitly write a value to `aux` here, instead of using the value programmed 50  […]

Ack

https://review.coreboot.org/c/flashrom/+/43608/1/buspirate_spi.c@534 
PS1, Line 534: AUX
> AUX is handled later on now

Ack

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43608 )

Change subject: buspirate: Add option for setting the aux pin
......................................................................

Patch Set 2: Code-Review+2

[Nico Huber (Code Review)]

Attention is currently required from: Sean Anderson.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43608 )

Change subject: buspirate: Add option for setting the aux pin
......................................................................

Patch Set 2:

(1 comment)

Patchset:

PS2: 
Hi, I noticed this old patch while merging CB:54887. Looks like
this one needed a manual rebase in any case.

[S] Change in flashrom[master]: buspirate: Add option for setting the aux pin

[Sean Anderson (Code Review)]

Attention is currently required from: Sean Anderson.

Hello build bot (Jenkins), Paul Menzel, Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/43608

to look at the new patch set (#3).

Change subject: buspirate: Add option for setting the aux pin
......................................................................

buspirate: Add option for setting the aux pin

This adds a parameter to drive the aux pin low (or high if you
explicitly want the previous behavior). Some boards need to have a reset
pin driven low before the firmware can be safely flashed. With the Bus
Pirate, this is most easily done with the auxiliary pin.

Change-Id: Ieeecfdf1afc06dadda9b8f99547cd74854ca6775
Signed-off-by: Sean Anderson 
---
M buspirate_spi.c
M doc/classic_cli_manpage.rst
2 files changed, 42 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/43608/3

[S] Change in flashrom[master]: buspirate: Add option for setting the aux pin

[Sean Anderson (Code Review)]

Attention is currently required from: Sean Anderson.

Hello build bot (Jenkins), Paul Menzel, Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/43608

to look at the new patch set (#4).

Change subject: buspirate: Add option for setting the aux pin
......................................................................

buspirate: Add option for setting the aux pin

This adds a parameter to drive the aux pin low (or high if you
explicitly want the previous behavior). Some boards need to have a reset
pin driven low before the firmware can be safely flashed. With the Bus
Pirate, this is most easily done with the auxiliary pin.

Change-Id: Ieeecfdf1afc06dadda9b8f99547cd74854ca6775
Signed-off-by: Sean Anderson 
---
M buspirate_spi.c
M doc/classic_cli_manpage.rst
2 files changed, 42 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/43608/4

[S] Change in flashrom[master]: buspirate: Add option for setting the aux pin

[Sean Anderson (Code Review)]

Attention is currently required from: Sean Anderson.

Hello build bot (Jenkins), Paul Menzel, Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/43608

to look at the new patch set (#5).

Change subject: buspirate: Add option for setting the aux pin
......................................................................

buspirate: Add option for setting the aux pin

This adds a parameter to drive the aux pin low (or high if you
explicitly want the previous behavior). Some boards need to have a reset
pin driven low before the firmware can be safely flashed. With the Bus
Pirate, this is most easily done with the auxiliary pin.

Change-Id: Ieeecfdf1afc06dadda9b8f99547cd74854ca6775
Signed-off-by: Sean Anderson 
---
M buspirate_spi.c
M doc/classic_cli_manpage.rst
2 files changed, 42 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/43608/5

[S] Change in flashrom[master]: buspirate: Add option for setting the aux pin

[Thomas Heijligen (Code Review)]

Attention is currently required from: Sean Anderson.

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43608 )

Change subject: buspirate: Add option for setting the aux pin
......................................................................

Patch Set 5: Code-Review+2

(1 comment)

File buspirate_spi.c:

https://review.coreboot.org/c/flashrom/+/43608/comment/7189e983_4074a682 
PS5, Line 657: AUX
I think this is now wrong. The AUX bit is added later.

[S] Change in flashrom[master]: buspirate: Add option for setting the aux pin

[Sean Anderson (Code Review)]

Attention is currently required from: Sean Anderson.

Hello build bot (Jenkins), Thomas Heijligen, Paul Menzel, Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/43608

to look at the new patch set (#6).

Change subject: buspirate: Add option for setting the aux pin
......................................................................

buspirate: Add option for setting the aux pin

This adds a parameter to drive the aux pin low (or high if you
explicitly want the previous behavior). Some boards need to have a reset
pin driven low before the firmware can be safely flashed. With the Bus
Pirate, this is most easily done with the auxiliary pin.

Change-Id: Ieeecfdf1afc06dadda9b8f99547cd74854ca6775
Signed-off-by: Sean Anderson 
---
M buspirate_spi.c
M doc/classic_cli_manpage.rst
2 files changed, 43 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/43608/6

[S] Change in flashrom[master]: buspirate: Add option for setting the aux pin

[Sean Anderson (Code Review)]

Attention is currently required from: Thomas Heijligen.

Sean Anderson has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43608 )

Change subject: buspirate: Add option for setting the aux pin
......................................................................

Patch Set 6:

(1 comment)

File buspirate_spi.c:

https://review.coreboot.org/c/flashrom/+/43608/comment/f2fdbc7e_c62007e0 
PS5, Line 657: AUX
> I think this is now wrong. The AUX bit is added later.

Done

[S] Change in flashrom[master]: buspirate: Add option for setting the aux pin

[Thomas Heijligen (Code Review)]

Attention is currently required from: Sean Anderson.

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43608 )

Change subject: buspirate: Add option for setting the aux pin
......................................................................

Patch Set 6: Code-Review+2

[S] Change in flashrom[master]: buspirate: Add option for setting the aux pin

[Anastasia Klimchuk (Code Review)]

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/43608 )

Change subject: buspirate: Add option for setting the aux pin
......................................................................

buspirate: Add option for setting the aux pin

This adds a parameter to drive the aux pin low (or high if you
explicitly want the previous behavior). Some boards need to have a reset
pin driven low before the firmware can be safely flashed. With the Bus
Pirate, this is most easily done with the auxiliary pin.

Change-Id: Ieeecfdf1afc06dadda9b8f99547cd74854ca6775
Signed-off-by: Sean Anderson 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/43608
Tested-by: build bot (Jenkins) 
Reviewed-by: Thomas Heijligen 
---
M buspirate_spi.c
M doc/classic_cli_manpage.rst
2 files changed, 46 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Thomas Heijligen: Looks good to me, approved

diff --git a/buspirate_spi.c b/buspirate_spi.c
index d3114c4..72c28b0 100644
--- a/buspirate_spi.c
+++ b/buspirate_spi.c
@@ -327,6 +327,7 @@
    int ret = 0;
    bool pullup = false;
    bool psu = false;
+	bool aux = true;
    unsigned char *bp_commbuf;
    int bp_commbufsize;

@@ -389,6 +390,17 @@
    }
    free(tmp);

+	tmp = extract_programmer_param_str(cfg, "aux");
+	if (tmp) {
+		if (strcasecmp("high", tmp) == 0)
+			; /* Default */
+		else if (strcasecmp("low", tmp) == 0)
+			aux = false;
+		else
+			msg_perr("Invalid AUX state, driving high by default.\n");
+	}
+	free(tmp);
+
    /* Default buffer size is 19: 16 bytes data, 3 bytes control. */
 #define DEFAULT_BUFSIZE (16 + 3)
    bp_commbuf = malloc(DEFAULT_BUFSIZE);
@@ -642,8 +654,8 @@
    	goto init_err_cleanup_exit;
    }

-	/* Initial setup (SPI peripherals config): Enable power, CS high, AUX */
-	bp_commbuf[0] = 0x40 | 0x0b;
+	/* Initial setup (SPI peripherals config): Enable power, CS high */
+	bp_commbuf[0] = 0x40 | 0x09;
    if (pullup) {
    	bp_commbuf[0] |= (1 << 2);
    	msg_pdbg("Enabling pull-up resistors.\n");
@@ -652,6 +664,12 @@
    	bp_commbuf[0] |= (1 << 3);
    	msg_pdbg("Enabling PSUs.\n");
    }
+	if (aux) {
+		bp_commbuf[0] |= (1 << 1);
+		msg_pdbg("Driving AUX high.\n");
+	} else {
+		msg_pdbg("Driving AUX low.\n");
+	}
    ret = buspirate_sendrecv(bp_commbuf, 1, 1);
    if (ret)
    	goto init_err_cleanup_exit;
diff --git a/doc/classic_cli_manpage.rst b/doc/classic_cli_manpage.rst
index d46d9ca..45ef4aa 100644
--- a/doc/classic_cli_manpage.rst
+++ b/doc/classic_cli_manpage.rst
@@ -780,6 +780,14 @@
 This allows the bus pirate to power the ROM chip directly. This may also be used to provide the required pullup voltage
 (when using the **pullups** option), by connecting the Bus Pirate's Vpu input to the appropriate Vcc pin.

+An optional aux parameter specifies the state of the Bus Pirate auxiliary pin.
+This may be used to drive the auxiliary pin high or low before a transfer.
+Syntax is::
+
+        flashrom -p buspirate_spi:aux=state
+
+where ``state`` can be ``high`` or ``low``. The default ``state`` is ``high``.
+

pickit2_spi programmer
 ^^^^^^^^^^^^^^^^^^^^^^