January 2018 - flashrom-gerrit

Change in flashrom[master]: buspirate_spi: Add support for variable serial speeds
by Shawn Anastasio (Code Review) 04 Jan '18

04 Jan '18

Hello David Hendricks, Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/23057 to look at the new patch set (#2). Change subject: buspirate_spi: Add support for variable serial speeds ...................................................................... buspirate_spi: Add support for variable serial speeds This patch sets the default baud rate for communication between the host device and the Bus Pirate for hardware versions 3.0 and greater to 2M baud. It also introduces the ability to manually set the baud rate via the added 'serialspeed' programmer parameter. This is done in two parts. Firstly, the requested serial speed is looked up in a table to determine the appropriate clock divisor and the divisor is sent to the bus pirate. Then, the system's baud rate for the selected serial port is set using serial.c's 'serialport_config'. This function's prototype had to be added to programmer.h. In testing, using the 2M baud rate was able to significantly decrease flash times (down from 20+ minutes to less than 2 minutes for an 8MB flash). Change-Id: I3706f17a94fdf056063f2ad4a5f0a219665cdcbf Signed-off-by: Shawn Anastasio <shawnanastasio(a)yahoo.com> --- M buspirate_spi.c M programmer.h 2 files changed, 122 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/57/23057/2 -- To view, visit https://review.coreboot.org/23057 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3706f17a94fdf056063f2ad4a5f0a219665cdcbf Gerrit-Change-Number: 23057 Gerrit-PatchSet: 2 Gerrit-Owner: Shawn Anastasio <shawnanastasio(a)yahoo.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Shawn Anastasio <shawnanastasio(a)yahoo.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: buspirate_spi: Add support for variable serial speeds
by Nico Huber (Code Review) 03 Jan '18

03 Jan '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23057 ) Change subject: buspirate_spi: Add support for variable serial speeds ...................................................................... Patch Set 1: (3 comments) https://review.coreboot.org/#/c/23057/1/buspirate_spi.c File buspirate_spi.c: https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@41 PS1, Line 41: const int divisor; > For the divisor, that algorithm doesn't always result in the > optimal divisor value (higher error than some other values). For which value exactly??? I tried all you add below and got the exact same numbers. Actually one of the lower ones would be more accurate if you don't round down in (4,000,000 / baud) but I don't see a reason for anything below 115200 anyway. > I > obtained the values used in the patch from this link: > http://unwind.se/buspirate-baudrate/#std250000. Don't care. > As for the older buspirates, I'm not sure what custom baud values > they support, if any. I could potentially add a hardware version > check to prevent this functionality on older boards. Yes, please do so. Actually, I think we should just use 2M for newer versions and stick to 115200 for older ones. Less options, less confusion and if anybody comes up with a pre-v3 model that could go faster, we can still add an option for that. https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@202 PS1, Line 202: goto out_shutdown; > I added the goto to prevent the code from breaking if more statements were added before the `out_shu […] I would just drop it. We already take care of it when we open the UART in buspirate_serialport_setup(). (IMHO, if any effect, reset- ting it here might hide bugs in other programs that just assume it's at 115200.) https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@352 PS1, Line 352: msg_pdbg("Detected Bus Pirate hardware %s\n", bp_commbuf); > The BP can support many variable baud rates and some may be more > appropriate in certain situations for different use cases. Instead > of trying to guess this, I added the parameter so that the user can > select the correct baud rate if they know it will work for their > use case. The wiki could be updated to reflect this information as > well. v3 pirates all have the USB to UART integrated, right? If so, I think it's fair to assume that there are no "different use cases". -- To view, visit https://review.coreboot.org/23057 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3706f17a94fdf056063f2ad4a5f0a219665cdcbf Gerrit-Change-Number: 23057 Gerrit-PatchSet: 1 Gerrit-Owner: Shawn Anastasio <shawnanastasio(a)yahoo.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Shawn Anastasio <shawnanastasio(a)yahoo.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 03 Jan 2018 23:16:41 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: buspirate_spi: Add support for variable serial speeds
by Shawn Anastasio (Code Review) 02 Jan '18

02 Jan '18

Shawn Anastasio has posted comments on this change. ( https://review.coreboot.org/23057 ) Change subject: buspirate_spi: Add support for variable serial speeds ...................................................................... Patch Set 1: (3 comments) Hi, thanks for the comments. I saw that patch earlier but didn't like how it enforced a single baud rate which might not always be compatible with your hardware, so I added the parameter. I've addressed some of your replies below. Please let me know which directions to take with them. Thanks again, Shawn Anastasio https://review.coreboot.org/#/c/23057/1/buspirate_spi.c File buspirate_spi.c: https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@41 PS1, Line 41: const int divisor; > Looks like the divisor is simply `4,000,000 / baud - 1`. Though, as […] For the divisor, that algorithm doesn't always result in the optimal divisor value (higher error than some other values). I obtained the values used in the patch from this link: http://unwind.se/buspirate-baudrate/#std250000. As for the older buspirates, I'm not sure what custom baud values they support, if any. I could potentially add a hardware version check to prevent this functionality on older boards. https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@202 PS1, Line 202: goto out_shutdown; > Actually, no goto needed here. […] I added the goto to prevent the code from breaking if more statements were added before the `out_shutdown` label with the thought that the compiler would optimize it out if it's not needed. I can remove it, however, if you believe it to be unnecessary. As for the baud change, I figured that since the Bus Pirate was resetting itself to 115200, the host should do the same, but you're right that it's unnecessary since we're closing the UART connection immediately after. I can remove it if you want. https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@352 PS1, Line 352: msg_pdbg("Detected Bus Pirate hardware %s\n", bp_commbuf); > Here the hardware version is stored in `bp_commbuf` can we use […] The BP can support many variable baud rates and some may be more appropriate in certain situations for different use cases. Instead of trying to guess this, I added the parameter so that the user can select the correct baud rate if they know it will work for their use case. The wiki could be updated to reflect this information as well. Since I don't have any BP 2 or older hardware to test, though, I could use this value to prevent baud rate changes entirely on the older hardware as mentioned above. -- To view, visit https://review.coreboot.org/23057 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3706f17a94fdf056063f2ad4a5f0a219665cdcbf Gerrit-Change-Number: 23057 Gerrit-PatchSet: 1 Gerrit-Owner: Shawn Anastasio <shawnanastasio(a)yahoo.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Shawn Anastasio <shawnanastasio(a)yahoo.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 02 Jan 2018 22:05:51 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: buspirate_spi: Add support for variable serial speeds
by Nico Huber (Code Review) 02 Jan '18

02 Jan '18

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23057 ) Change subject: buspirate_spi: Add support for variable serial speeds ...................................................................... Patch Set 1: (5 comments) Carl-Daniel pointed out on IRC that there already was a patch floating around: https://mail.coreboot.org/pipermail/flashrom/2012-November/010102.html He was also discussing pre-v3 buspirates. I guess we should filter them out unless we can test the patch with one. https://review.coreboot.org/#/c/23057/1/buspirate_spi.c File buspirate_spi.c: https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@41 PS1, Line 41: const int divisor; Looks like the divisor is simply `4,000,000 / baud - 1`. Though, as I've just learned, most buspirates come with an integrated serial UART to USB chip. So that raises the question if older buspirates support the BAUD switch at all and if it makes sense to use any- thing but the 2M? https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@200 PS1, Line 200: ret = buspirate_sendrecv(bp_commbuf, 1, 0); If you are going to overwrite `ret` below, you should check it here too and bail out in case. https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@201 PS1, Line 201: if ((ret = serialport_config(sp_fd, 115200))) > nit: extra parens IIRC, gcc wants those parens (because of the assignment). https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@202 PS1, Line 202: goto out_shutdown; Actually, no goto needed here. I'm also wondering why we should do this at all. We are not going to transfer any more data and the baud is reset to 115200 whenever we open the UART. https://review.coreboot.org/#/c/23057/1/buspirate_spi.c@352 PS1, Line 352: msg_pdbg("Detected Bus Pirate hardware %s\n", bp_commbuf); Here the hardware version is stored in `bp_commbuf` can we use that to decide which baud rate to chose? I'd prefer that over another option for the user to guess. -- To view, visit https://review.coreboot.org/23057 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3706f17a94fdf056063f2ad4a5f0a219665cdcbf Gerrit-Change-Number: 23057 Gerrit-PatchSet: 1 Gerrit-Owner: Shawn Anastasio <shawnanastasio(a)yahoo.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 02 Jan 2018 20:40:50 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: chipset_enable: Mark VX900 as tested
by build bot (Jenkins) (Code Review) 02 Jan '18

02 Jan '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23030 ) Change subject: chipset_enable: Mark VX900 as tested ...................................................................... Patch Set 4: Build Successful https://qa.coreboot.org/job/flashrom-customrules/998/ : SUCCESS -- To view, visit https://review.coreboot.org/23030 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I190253b0c1920747b710ed7155e78191cce139eb Gerrit-Change-Number: 23030 Gerrit-PatchSet: 4 Gerrit-Owner: Lubomir Rintel <lkundrak(a)v3.sk> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 02 Jan 2018 20:16:07 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: chipset_enable: Mark VX900 as tested
by Nico Huber (Code Review) 02 Jan '18

02 Jan '18

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/23030 ) Change subject: chipset_enable: Mark VX900 as tested ...................................................................... chipset_enable: Mark VX900 as tested I can confirm a successful reading and writing of MX25L8005 (SPI) on a HP t5550 Thin Client (AMI BIOS 786R9 v1.04). Change-Id: I190253b0c1920747b710ed7155e78191cce139eb Signed-off-by: Lubomir Rintel <lkundrak(a)v3.sk> Reviewed-on: https://review.coreboot.org/23030 Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M chipset_enable.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Nico Huber: Looks good to me, approved build bot (Jenkins): Verified diff --git a/chipset_enable.c b/chipset_enable.c index 5716ebf..a46651f 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1714,7 +1714,7 @@ {0x1106, 0x8324, OK, "VIA", "CX700", enable_flash_vt823x}, {0x1106, 0x8353, NT, "VIA", "VX800/VX820", enable_flash_vt_vx}, {0x1106, 0x8409, NT, "VIA", "VX855/VX875", enable_flash_vt_vx}, - {0x1106, 0x8410, NT, "VIA", "VX900", enable_flash_vt_vx}, + {0x1106, 0x8410, OK, "VIA", "VX900", enable_flash_vt_vx}, {0x1166, 0x0200, OK, "Broadcom", "OSB4", enable_flash_osb4}, {0x1166, 0x0205, OK, "Broadcom", "HT-1000", enable_flash_ht1000}, {0x17f3, 0x6030, OK, "RDC", "R8610/R3210", enable_flash_rdc_r8610}, -- To view, visit https://review.coreboot.org/23030 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I190253b0c1920747b710ed7155e78191cce139eb Gerrit-Change-Number: 23030 Gerrit-PatchSet: 4 Gerrit-Owner: Lubomir Rintel <lkundrak(a)v3.sk> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: vt_vx: check whether the chipset's MMIO range is configured
by build bot (Jenkins) (Code Review) 02 Jan '18

02 Jan '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23029 ) Change subject: vt_vx: check whether the chipset's MMIO range is configured ...................................................................... Patch Set 2: Build Successful https://qa.coreboot.org/job/flashrom-customrules/997/ : SUCCESS -- To view, visit https://review.coreboot.org/23029 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5d3a1ba695153e854e0979ae634f8ed97e6b6293 Gerrit-Change-Number: 23029 Gerrit-PatchSet: 2 Gerrit-Owner: Lubomir Rintel <lkundrak(a)v3.sk> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 02 Jan 2018 20:15:56 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: flashchips: Revise all 4BA chips
by build bot (Jenkins) (Code Review) 02 Jan '18

02 Jan '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/22423 ) Change subject: flashchips: Revise all 4BA chips ...................................................................... Patch Set 7: Build Successful https://qa.coreboot.org/job/flashrom-customrules/996/ : SUCCESS -- To view, visit https://review.coreboot.org/22423 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0598496ee7058e3b170684d366f58e4014e0e871 Gerrit-Change-Number: 22423 Gerrit-PatchSet: 7 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 02 Jan 2018 20:15:46 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: vt_vx: check whether the chipset's MMIO range is configured
by Nico Huber (Code Review) 02 Jan '18

02 Jan '18

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/23029 ) Change subject: vt_vx: check whether the chipset's MMIO range is configured ...................................................................... vt_vx: check whether the chipset's MMIO range is configured Avoid attempting to read the SPI bases from the location 0x00000000, all zeroes mean that the chipset's MMIO area is not enabled. Change-Id: I5d3a1ba695153e854e0979ae634f8ed97e6b6293 Signed-off-by: Lubomir Rintel <lkundrak(a)v3.sk> Reviewed-on: https://review.coreboot.org/23029 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> --- M chipset_enable.c 1 file changed, 8 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved David Hendricks: Looks good to me, approved diff --git a/chipset_enable.c b/chipset_enable.c index a499ba0..5716ebf 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1013,10 +1013,18 @@ switch(dev->device_id) { case 0x8353: /* VX800/VX820 */ spi0_mm_base = pci_read_long(dev, 0xbc) << 8; + if (spi0_mm_base == 0x0) { + msg_pdbg ("MMIO not enabled!\n"); + return ERROR_FATAL; + } break; case 0x8409: /* VX855/VX875 */ case 0x8410: /* VX900 */ mmio_base = pci_read_long(dev, 0xbc) << 8; + if (mmio_base == 0x0) { + msg_pdbg ("MMIO not enabled!\n"); + return ERROR_FATAL; + } mmio_base_physmapped = physmap("VIA VX MMIO register", mmio_base, SPI_CNTL_LEN); if (mmio_base_physmapped == ERROR_PTR) return ERROR_FATAL; -- To view, visit https://review.coreboot.org/23029 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I5d3a1ba695153e854e0979ae634f8ed97e6b6293 Gerrit-Change-Number: 23029 Gerrit-PatchSet: 2 Gerrit-Owner: Lubomir Rintel <lkundrak(a)v3.sk> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[master]: spi25: Revise decision when to enter/exit 4BA mode
by build bot (Jenkins) (Code Review) 02 Jan '18

02 Jan '18

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/22422 ) Change subject: spi25: Revise decision when to enter/exit 4BA mode ...................................................................... Patch Set 7: Build Successful https://qa.coreboot.org/job/flashrom-customrules/995/ : SUCCESS -- To view, visit https://review.coreboot.org/22422 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I877fe817f801fc54bd0ee2ce4e3ead324cbb3673 Gerrit-Change-Number: 22422 Gerrit-PatchSet: 7 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 02 Jan 2018 20:15:42 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit January 2018