[flashrom] [PATCH] Add a bunch of new/tested stuff and various small changes 25.

Sun Mar 13 14:44:34 CET 2016

On Sun, 13 Mar 2016 14:30:19 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> On 05.03.2016 22:42, Stefan Tauner wrote:
> > Tested mainboards:
> > OK:
> >  - ASRock Fatal1ty 970 Performance and P4i65G
> >    Reported by anonymous email message ID:
> >    932677687262b1300eaf14260999d9262c31 at guerrillamail.com
> >    The latter actually had a tested board enable already.
> >
> > Flash chips:
> >  - GigaDevice GD25VQ41B to PREW (+PREW)
> >    Reported by David Hendricks
> >  - Winbond W39V040FB to PREW (+EW)
> >    Reported by fjed on IRC
> >
> > Miscellaneous:
> >  - Change PCI IDs of "MS-6577 (Xenon)" board enable.
> >    The previous IDs contained the on-board display adapter which is
> >    disabled when a dedicated graphics card is installed.
> >  - Add a note to the README how to overcome the clang warning if only a
> >    single programmer is enabled.
> >  - Fix some typo and manpage problems found by lintian
> >  - r1920 introduced some explicit calls to pkg-config instead of $(PKG_CONFIG).
> >    This patch corrects that.
> >  - Add some overrides to the Makefile in case someone/something sets
> >    variables like CPPFLAGS or LDFLAGS as command line parameters
> 
> I couldn't find that in the patch.

This was already committed separately in r1931, thanks.

> > Signed-off-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
> > Acked-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
> > ---
> >  Makefile        | 34 ++++++++++++++---------------
> >  README          |  6 ++++++
> >  board_enable.c  |  2 +-
> >  flashchips.c    | 66 ++++++++++++++++++++++++++++-----------------------------
> >  flashrom.8.tmpl | 38 ++++++++++++++++++---------------
> >  pickit2_spi.c   |  2 +-
> >  print.c         |  4 ++--
> >  sb600spi.c      |  2 +-
> >  8 files changed, 82 insertions(+), 72 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 927105d..d9a70b3 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -661,22 +661,22 @@ override CONFIG_CH341A_SPI = no
> >  override CONFIG_DEDIPROG = no
> >  endif
> >  ifeq ($(CONFIG_ENABLE_LIBPCI_PROGRAMMERS), no)
> > -override CONFIG_INTERNAL = no 
> > -override CONFIG_NIC3COM = no 
> > -override CONFIG_GFXNVIDIA = no 
> > -override CONFIG_SATASII = no 
> > -override CONFIG_ATAHPT = no 
> > -override CONFIG_ATAVIA = no 
> > -override CONFIG_ATAPROMISE = no 
> > -override CONFIG_IT8212 = no 
> > -override CONFIG_DRKAISER = no 
> > -override CONFIG_NICREALTEK = no 
> > -override CONFIG_NICNATSEMI = no 
> > -override CONFIG_NICINTEL = no 
> > -override CONFIG_NICINTEL_SPI = no 
> > -override CONFIG_NICINTEL_EEPROM = no 
> > -override CONFIG_OGP_SPI = no 
> > -override CONFIG_SATAMV = no 
> > +override CONFIG_INTERNAL = no
> > +override CONFIG_NIC3COM = no
> > +override CONFIG_GFXNVIDIA = no
> > +override CONFIG_SATASII = no
> > +override CONFIG_ATAHPT = no
> > +override CONFIG_ATAVIA = no
> > +override CONFIG_ATAPROMISE = no
> > +override CONFIG_IT8212 = no
> > +override CONFIG_DRKAISER = no
> > +override CONFIG_NICREALTEK = no
> > +override CONFIG_NICNATSEMI = no
> > +override CONFIG_NICINTEL = no
> > +override CONFIG_NICINTEL_SPI = no
> > +override CONFIG_NICINTEL_EEPROM = no
> > +override CONFIG_OGP_SPI = no
> > +override CONFIG_SATAMV = no
> 
> Zero changes except whitespace. The changelog claims the overrides changed.

No that's not related I think. The whitespace is something I noticed
but forgot in the review. The override was something Uwe/Debian has
problems with... see above/r1931.

> 
> >  endif
> >  
> >  # Bitbanging SPI infrastructure, default off unless needed.
> > diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
> > index cb77e46..5de735b 100644
> > --- a/flashrom.8.tmpl
> > +++ b/flashrom.8.tmpl
> > @@ -1,9 +1,11 @@
> >  .\" Load the www device when using groff; provide a fallback for groff's MTO macro that formats email addresses.
> >  .ie \n[.g] \
> >  .  mso www.tmac
> > -.el \
> > -.  de MTO \\$2 \(la\\$1 \(ra\\$3
> > +.el \{
> > +.  de MTO
> > +     \\$2 \(la\\$1 \(ra\\$3 \
> >  .  .
> > +.\}
> >  .\" Create wrappers for .MTO and .URL that print only text on systems w/o groff or if not outputting to a HTML
> >  .\" device. To that end we need to distinguish HTML output on groff from other configurations first.
> >  .nr groffhtml 0
> 
> I have to trust you on this.

That one was found by lintian and reproduced locally (one has to enable
warnings explicitly for groff). I have also discussed that in #groff or
something and they said it was obviously an error. And I said: "Sure.
Obvious." and left :P

> > @@ -684,13 +680,18 @@ size (padding to 32 kB is required).
> >  .IP
> >  This is the first programmer module in flashrom that does not provide access to NOR flash chips but EEPROMs
> >  mounted on gigabit Ethernet cards based on Intel's 82580 NIC. Because EEPROMs normally do not announce their
> > -size nor allow to be identified, the controller relies on correct size values written to predefined addresses
> > -within the chip. Flashrom follows this scheme but assumes the minimum size of 16 kB (128 kb) if an unprogrammed
> > -EEPROM/card is detected. Intel specifies following EEPROMs to be compatible: Atmel AT25128, AT25256, Micron (ST)
> > -M95128, M95256 and OnSemi (Catalyst) CAT25CS128.
> > +size nor allow themselves to be identified, the controller relies on correct size values written to predefined
> > +addresses within the chip. Flashrom follows this scheme but assumes the minimum size of 16 kB (128 kb) if an
> > +unprogrammed EEPROM/card is detected. Intel specifies following EEPROMs to be compatible:
> > +Atmel AT25128, AT25256, Micron (ST) M95128, M95256 and OnSemi (Catalyst) CAT25CS128.
> 
> Sorry, -ENOPARSE. Does this mean we try to determine chip size by
> reading predefined locations of the EEPROM where size is customarily stored?

We don't - the hardware does and reports it back to us. This was a
simple grammar fix (found by and required for lintian).

> >  .SS
> >  .BR "ft2232_spi " programmer
> >  .IP
> > +This module supports various programmers based on FTDI FT2232/FT4232H/FT232H chips including the DLP Design
> > +DLP-USB1232H, openbiosprog-spi, Amontec JTAGkey/JTAGkey-tiny/JTAGkey-2, Dangerous Prototypes Bus Blaster,
> > +Olimex ARM-USB-TINY/-H, Olimex ARM-USB-OCD/-H, OpenMoko Neo1973 Debug board (V2+), TIAO/DIYGADGET USB
> > +Multi-Protocol Adapter (TUMPA), TUMPA Lite, GOEPEL PicoTAP and Google Servo v1/v2.
> > +.sp
> >  An optional parameter specifies the controller
> >  type and channel/interface/port it should support. For that you have to use the
> >  .sp
> > @@ -981,7 +985,7 @@ Please note that the linux_spi driver only works on Linux.
> >  .BR "mstarddc_spi " programmer
> >  .IP
> >  The Display Data Channel (DDC) is an I2C bus present on VGA and DVI connectors, that allows exchanging
> > -informations between a computer and attached displays. Its most common uses are getting display capabilities
> > +information between a computer and attached displays. Its most common uses are getting display capabilities
> >  through EDID (at I2C address 0x50) and sending commands to the display using the DDC/CI protocol (at address
> >  0x37). On displays driven by MSTAR SoCs, it is also possible to access the SoC firmware flash (connected to
> >  the Soc through another SPI bus) using an In-System Programming (ISP) port, usually at address 0x49.
> > @@ -1016,7 +1020,7 @@ Example that does not reset the display at the end of the operation:
> >  .sp
> >  .B "  flashrom \-p mstarddc_spi:dev=/dev/i2c-1:49,noreset=1
> >  .sp
> > -Please note that sending the reset command is also inhibited in the event an error occured during the operation.
> > +Please note that sending the reset command is also inhibited in the event an error occurred during the operation.
> 
> Please note that sending the reset command is also inhibited if an error
> occurred during the operation.

k

> >  To send the reset command afterwards, you can simply run flashrom once more, in chip probe mode (not specifying
> >  an operation), without the
> >  .B noreset
> > diff --git a/pickit2_spi.c b/pickit2_spi.c
> > index f1f60a2..f6aa676 100644
> > --- a/pickit2_spi.c
> > +++ b/pickit2_spi.c
> > @@ -400,7 +400,7 @@ static int pickit2_shutdown(void *data)
> >  
> >  int pickit2_spi_init(void)
> >  {
> > -	unsigned int usedevice = 0; // FIXME: allow to select one of multiple devices
> > +	unsigned int usedevice = 0; // FIXME: allows one to select one of multiple devices
> 
> The original sentence has grammar problems, but I wouldn't endorse the
> replacement either.
> "Allow selecting one of multiple devices" would have correct grammar and
> style.

I couldn't care less... :) but this is required to make lintian shut up.
Changed to your suggestion.
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner