Patchwork [PATCH/FULL/SVN] Support for 4-bytes addressing SPI flash chips 32MB+ (256Mb+)

2015-01-15 09:59:59

Boris,

Thank you for your effort! This is indeed a very useful feature that flashrom has been
lacking for too long.

It is very nice that you not only implemented support for the Winbond chip you had,
but also took time to implement support for JESD216B specification and other variants of 4-byte
addressing.

As you may know, I'm not a flashrom maintainer, but as we agreed I have reviewed your patch and
tested it with Winbond W25Q256.V (the one I gave you) and also with MXIC MX25L25635F.

Here are some review comments:

- Equational macros (e.g. MIN_LENGTH_TO_SHOW_READ_PROGRESS) are defined
  without using parentheses around equations. That is risk-prone.

- In doit() the case when write is direct and the erasers are not is not handled,
  thus resulting in entering 4BA mode unnecessarily for direct writes.

- Conditions for use of direct/indirect functions are vague and error-prone.
  You could use feature bits inside some generic function to enter/exit
  4BA mode as needed. The current approach of entering the 4BA mode just once
  in doit() and never exiting it looks risky to me.
  The same relates to 4BA/usual functions. IMO, there is no need for most of the
  *_4ba() functions as the only difference from usual functions is address size.
  Feature bits could be used again.
  Copy-pasting the code is a brute force approach and leads, in the long run, to
  nasty bugs when one fixes or enhances one function and forgets to update dozens
  of siblings/copycats. Similar code must be contained within as little entities
  as possible. The level of copy-pasteness of the original flashrom code is already
  excessive and increasing it further is not the best idea.

- Use of a fishing-world word 'BAIT' many times throughout the code.
  Probably, 'BYTE' was intended. This typo is quite alarming for me.

- In spi_chip_read() some comments are left irrelevant. The code after this
  patch no more checks if the chip fits between lowest valid and 0xFFFFFF
  (which is anyway not relevant for 4BA chips). The check is not performed at
  all for 4BA chips.
  ffs() never returns numbers higher than 32 as it takes a 32-bit integer
  as an argument, so checking if the return value is greater than 33 yields
  an always-false. ffsll() should probably be used instead (which would result
  in problems with compiling for Windows using mingw32).

- In walk_eraseregions() variables percent_last and percent_current are
  initialized conditionally, which makes some compilers (e.g., mingw32msvc-gcc v4.2.1)
  complain about possible use of those variables uninitialized.

- In doit() the comment in the last 'else' and the debug message are incorrect/irrelevant.
  In fact this code block is only executed for all-direct case, not for always-4ba case,
  which is handled separately in the first 'if'. As to the debug message,
  saying 'Using direct 4-byte addressing commands' would be more informative than
  'No need to switch...'

- The change to serprog.c is hard to read. I would prefer a normal if/else
  instead of a ternary operator. That would be easier to read and understand.
  That's not the only place with ternary operator used with such long and
  complex arguments. This comment applies to all of those cases although
  if my earlier comment regarding copy-pasteness and superfluous _4ba functions
  is addressed, those monstrous ternary operators may just automagically go away.

- Happy New Year!
  I believe that the 'History of changes' in sfdp.c should contain dates
  from 2015, not from 2014. In other newly added places I guess it is also 2015, not 2014.
  ;)

- Looks like currently one can define a chip with size over 16MB but without
  FEATURE_4BA_SUPPORT and get strange results. Maybe it would be better to imply
  4BA support for any chips over 16MB and get rid of FEATURE_4BA_SUPPORT bit?

- What is this 'eraser.type' field is for? I don't see any actual use for it.
  Looks like an index field to me if I understand JESD216B right.
  Wouldn't it be more correct to search for a proper eraser using block size
  rather than 'eraser type' ? Current implementation forces developers to
  think about those 'eraser types' each time they add a new 4BA chip.
  Am I overlooking anything here?

- I hate magic numbers. They are a source of hard-to-identify bugs. I know that it wasn't you
  who originally put it into sfdp.c/sfdp_fill_flash(), but since you added 16 * 4
  to the existing 9 * 4, I would expect at least an added comment as to what
  those 4, 9 and 16 are. Better yet would be to replace them with meaningfully named
  macros.

- Things like this:

+       tmp32 =  ((unsigned int)buf[(4 * 0) + 0]);
+       tmp32 |= ((unsigned int)buf[(4 * 0) + 1]) << 8;
+       tmp32 |= ((unsigned int)buf[(4 * 0) + 2]) << 16;
+       tmp32 |= ((unsigned int)buf[(4 * 0) + 3]) << 24;

  make my eyes bleed. Why not use a loop or at least a macro?

  Besides, this operation looks to me like a usual big-endian to host-endian conversion,
  or in other words a network byte order to host byte order conversion.
  If so, then it's definitely better, for the sake of portability, to use a standard ntohl()
  function here and not to reinvent a non-portable architecture-bound bicycle.

- Having boolean variables for each possible SFDP version (having in mind that
  more may come in future) is a bad idea, IMO. Keeping SFDP version in a variable
  outside the flashctx structure and passing it as an argument to functions along
  with the flashctx structure pointer is an even worse idea. SFDP version is apparently
  a property of the chip. Hence it should be contained within the chip information
  structure. However I would recommend to limit SFDP revision number awareness to the
  probe_spi_sfdp() function and convert the revision number therein to feature
  bits like FEATURE_SFDP_4BA_SUPPORT (for SFDP 1.6+), etc.
  That is, sfdp_fill_flash() shouldn't take an extra 'sfdp_rev_15' argument and
  instead should operate purely based on feature bits.

- Added parentheses in calls to msg_cspew around "(Type %d)" in sfdp.c someplaces broke
  the meaning of the phrases. Why did you do that for? It's about "Erase Sector Type",
  not about "Erase Sector". In most cases that was the only change.
  I would revoke those changes (except for adding a missing '+ 1' in one of the calls).

- Special handling for '\b' in msg_cinfo() looks kludgy to me.
  I would probably change print() to have an extra argument like 'console_only'
  and created an extra macro for calling print() with that argument being false.
  That macro I would use instead of msg_cinfo() to output progress.
  Or maybe it can be managed using verbosity alone.
  Anyway that would be less obscure for other developers during their future efforts
  than this special '\b' handling.

Also, I couldn't manage to make this patch work with MXIC MX25L25635F chip which
supports all the 4BA modes (direct, EN4B, EAR). I tried various combinations,
but it couldn't properly erase the chip anyway. Didn't work in SFDP mode as well
(only SFDP 1.0 is supported by the chip). Hence, I couldn't check if it reads
back properly what was written. I didn't do any debugging though to find out
the reason.

The patch does indeed work for Winbond W25Q256.V.

Thanks again.

With best regards -
Alexander Amelkin,
T-Platforms [http://www.t-platforms.com]

P.S. Please note that the patch did not cleanly apply to r1868. I had to modify it.

-----Original Message-----
From: flashrom [mailto:flashrom-bounces@flashrom.org] On Behalf Of Boris Baykov
Sent: Sunday, January 11, 2015 2:24 AM
To: flashrom@flashrom.org
Subject: [flashrom] [PATCH/FULL/SVN] Support for 4-bytes addressing SPI flash chips 32MB+ (256Mb+)

This patch is for build 0.9.7-r1868 The patch is made by SVN diff.
Also for review the patch is divided into 10 parts using diff(1).

Brief description
-----------------
The patch adds ability to read, write and erase flash chips
which require 4-bytes (32-bit) addressing to access to them.
These are 32MB (256Mb) and larger new flash chips. Flashrom
with this patch can support and program all (or most of all)
of them. Besides of manually definable chips in flashchips.c
this patch adds compliance to new JEDEC JESD216B standard
which describes SFDP revision 1.6 procedure to read flash
chip 4-bytes addressing details.

Patch structure
---------------
To make code review easier I've splitted the patch in 10 incremental
patches. These parts are made by standard Linux diff without using SVN.
Also I've added 2 extra patches (4a & 5a) to show how to define chips
using different types of 4-bytes addressing.

To apply the complete 4BA-patch you can directly apply SVN diff version
of the patch or you should apply 10 patches ones by one in the following
order: 1, 2, 3, 4, 5, 6, 7, 8, 9, 10.

Please note, that it's no need to apply patches 4a & 5a. Though you may
apply patch 4a and the result code will work with no issues. But please
don't try to use patch 5a because its code is not working with W25Q256*V
chip, and this patch is here only to provide an example how to add chips
with support of direct-4BA write and erase.

[PATCH 1/10] 4BA: Basic support for 4-bytes addressing mode extensions

[PATCH 2/10] 4BA: Flashrom integration for the 4-bytes addressing extensions

[PATCH 3/10] 4BA: Winbond W25Q256.V chip (32MB) declaration, 4-bytes addr mode

[PATCH 4/10] 4BA: Support for 4-bytes addressing via Extended Address Register

[PATCH 4a/10] 4BA: W25Q256.V declaration update to work using Ext.Addr.Reg
The 4a patch is made to show how can you define chips with access via
Extebded Address Register. This patch is fully working but not required.

[PATCH 5/10] 4BA: Support for new direct-4BA instructions + W25Q256.V update

[PATCH 5a/10] 4BA: W25Q256.V declaration update to work via direct funcs
This patch doesn't work with this chip. It's this just to show how new
direct-4ba functions can be used to define new chips.

[PATCH 6/10] 4BA: Progress visualization for long read, writes and erases

[PATCH 7/10] 4BA: SFDP 1.6 parser for compliance with JESD216B

[PATCH 8/10] 4BA: SFDP 1.6 simulation (for testing purposes)

[PATCH 9/10] 4BA: SFDP downgrade to 1.5 to support new Micron chips
Explanation why 1.6 downgraded to 1.5 is below.

[PATCH 10/10] 4BA: Some small visual changes for 4-byte addresses

Full description
----------------
This patch includes code for three types of 4-byte addressing:

1. Flash chip can be switched to 4-bytes addressing mode and
then all read/erase/program instructions will be switched from
3-bytes mode to 4-bytes mode. So, well known instructions like
03h (Read), 02h (Program) and 20h,52h,D8h (Erase) are becoming
one byte longer and accept 4-bytes address instead of 3-bytes.
4-bytes addressing mode is entered by the instruction B7h with
preceding Write Enable (WREN). This is default method.

2. Also Flash chip data with addresses more than 24-bit field
can address may be accessed by use of Extended Address Register
that has 1-byte size and stores high byte of 32-bit address.
So, we read flash from old 3-bytes addressing mode and write
high byte of address to special register. Then we have access
to full memory of a chip. Some chips may support this method
only.

3. Also some new commands avalable and implemented. Large size
flash chips support special instructions to use 4-bytes address
directly from 3-bytes addressing mode and without do switching
to 4-bytes mode. There are 13h (4BA Read), 12h (4BA Program)
and 21h,5Ch,DCh (4BA Erase), correspondingly. However not all
these commands are supported by all large flash chips. Some
chips support 13h only, some 13h,12h,21h and DCh, but not 5Ch.
This depends on the manufacturer of the chip.

The patch adds code for all 3 types of 4-bytes address access
and the exact method should be choosen by person who will
manage to add new large chips to flashchips.c source file.

I've added one new flash chip: Winbond W25Q256FV (32MB)
because this is the only chip I have now for testing.

Besides of manual including of new chips to Flashrom we may
use SFDP procedure to discover parameters of the exact flash
chip. More than a year ago JEDEC released new standard
JESD216 (revision B) which describes in detail how to receive
4-bytes addressing settings from the chip. This patch provides
new code to add compliance to JESD216B to Flashrom. This means
that Flashrom now can automatically detect 4-bytes addressing
features of large flash chips and use them to access data.

However not all things are so good. Unfortunately JESD216B
had come to manufacturer's production lines just 2-3 months
ago. So, we have many chips that have large size requires
32-bit addressing but their SFDP tables have revision lower
than 1.6 (described in JESD216B standard). For example my
W25Q256FV returns SFDP info with revision 1.0, first one :-)
This situation leads us to requirement to try to work now
without new SFDP info. And it's implemented in the patch too.

To test my parser code for SFDP revision 1.6 (from JESD216B)
I've added some simulation code to sfdp.c. This code builds
fake SFDP 1.6 tables in memory and passes them to the SFDP
parser. Surely this simulation code is switched off by default
and can be activated via hidden define on the top of sfdp.c

After I added SFDP 1.6 parser I found in Micron's datasheet
for MT25Q 512Mb chip (MT25QL512AB/MT25QU512AB) that the chip
returnes SFDP revision 1.5 and simultaniously provides the
required information about addressing mode switch, however
all information about direct-4BA functions isn't provided.
Then I decided to parse new SFDP 4-bytes info from SFDP
revision 1.5 besides of direct-4BA instructions. This
assumption is made for better support of Micron flash chips.
This should be checked and may be fixed later.

Also I've added progress visualization for read, erase and
write operations. It's turned out that seeing progress is
essential for reading and especially writing 32 MB of data
via SPI. The operation can take more then 10 minutes on
15 MHz frequency of SPI. So, it's good to see its progress.
I've added percents and slightly modified cli_output.c to
send percents to screen only but not to logfile.

Testing results
---------------
Both patches are tested with Winbond W25Q256FV flash chip
using Olimex ARM-USB-OCD-H programmer.

Read, Erase and Program functions are fully tested for
4-bytes addressing type 1 (4-bytes mode), type 2 (extended
addr. register) and partially tested for type 3 (new 4-bytes
instructions with 4-bytes address). My W25Q256FV has support
for 4BA_Read (13h) only, but doesn't support 4BA_Program (12h)
and 4BA_Erase instructions. So, direct 4BA program and erase
should be tested after.

Also I have to make an excuse about testing of SFDP rev 1.6
parser. I've tested it as good as I can using the simulation
code I described above but I don't have ability to test it
on real flash chips because I don't have a chip complied to
JESD216B with SFDP 1.6 now. This should be tested later too.

Patched files
-------------
chipdrivers.h
+ added functions declarations for spi4ba.c

cli_output.c
+ print() patched to skip strings which are starting from '\b'
  to prevent writing progress percents to logfile

flash.h
+ feature definitions added
+ added set of 4-bytes address functions to flashchip structure definition
+ added type field to block_eraser structure
+ added some definitions for progress visialization

flashchips.c
+ added definition for Winbond W25Q256BV/W25Q256FV chips

flashrom.c
+ added switch to 4-bytes addressing more for chips which support it
+ added progress visualization for erase/write (essensial for 32MB+ chips)
+ changed in address print %06x to %08x to support 32MB+ chips
+ added line to the flashrom's banner about this patch (temporary)

Makefile
+ added spi4ba.c

serprog.c
+ added 4-bytes addressing spi_nbyte_read call to serprog_spi_read

spi.c
+ fixed flash chip size check in spi_chip_read

spi25.c
+ added 4-bytes addressing spi_nbyte_read call to spi_read_chunked
+ added 4-bytes addressing spi_nbyte_program call to spi_write_chunked
+ added 4-bytes addressing spi_byte_program call to spi_chip_write_1
+ added progress visualization for read operation (essensial for 32MB+ chips)

sfdp.c
+ added compliance to JEDEC JESD216B standard (SFDP revision 1.6)
+ added parse of SFDP 4-Bytes Addressing features (probe_spi_sfdp,sfdp_fill_flash)
+ added parse of SFDP 4-Bytes Addressing Instruction Table (sfdp_parse_4ba_table)
+ added choosing of block_erasers for 4-bytes addressing (sfdp_*_uniform_eraser*)
+ added exact flash page_size recognision from 11th dword in SFDP rev 1.6
+ added fix for Micron chips to read 16th dword stating from SFDP revision 1.5
+ added simulation of JESD216B for any flash chips (just for testing)

Added files
-----------
spi4ba.h
+ definitions for 4-bytes addressing JEDEC commands
+ definitions for 16th dword bits of SFDP Basic Flash Param Table (JESD216B)
+ definitions for SFDP 4-Bytes Addressing Instruction Table (JESD216B)
+ functions declarations from spi4ba.c (same as in chipdrivers.h, just to see)

spi4ba.c
+ functions for enter 4-bytes addressing mode
+ functions for write Extended Address Register
+ functions for read/write/erase in 4-bytes addressing mode
+ functions for read/write/erase with Extended Address Register
+ functions for read/write/erase directly with 4-bytes address (from any mode)
+ functions to choose block_eraser by opcode for 4-bytes addressing for sfdp.c
+ functions to select proper block_eraser (ext.reg or 4-bytes addr) for sfdp.c

Signed-off-by: Boris Baykov <dev@borisbaykov.com>, Russia, Jan 2014
---

Property changes on: spi4ba.h

Submitter	Amelkin, Aleksandr
Date	2015-01-15 09:59:59
Message ID	<674465BAC0F69347905C11F02369E3AB9417B75A1B@ppmail.tp-local.ru>
Download	mbox \| patch
Permalink	/patch/4272/
State	New
Headers	show Return-path: <flashrom-bounces@flashrom.org> Envelope-to: patchwork@coreboot.org Received: from localhost ([127.0.0.1] helo=ra.coresystems.de) by ra.coresystems.de with esmtp (Exim 4.83) (envelope-from <flashrom-bounces@flashrom.org>) id 1YBhDy-0003z3-EO; Thu, 15 Jan 2015 11:00:22 +0100 Received: from mx1.t-platforms.ru ([80.85.246.77]) by ra.coresystems.de with esmtps (TLSv1:DHE-RSA-AES256-SHA:256) (Exim 4.83) (envelope-from <Aleksandr.Amelkin@t-platforms.ru>) id 1YBhDg-00013X-8b for flashrom@flashrom.org; Thu, 15 Jan 2015 11:00:19 +0100 Received: from ppmail.tp-local.ru ([192.168.5.26]) by mx1.t-platforms.ru with esmtp (Exim 4.82 (FreeBSD)) (envelope-from <Aleksandr.Amelkin@t-platforms.ru>) id 1YBhDd-000Daq-3D; Thu, 15 Jan 2015 13:00:01 +0300 Received: from ppmail.tp-local.ru ([192.168.5.26]) by ppmail.tp-local.ru ([192.168.5.26]) with mapi; Thu, 15 Jan 2015 13:00:00 +0300 From: "Amelkin, Aleksandr" <Aleksandr.Amelkin@t-platforms.ru> To: Boris Baykov <dev@borisbaykov.com>, "flashrom@flashrom.org" <flashrom@flashrom.org> Date: Thu, 15 Jan 2015 12:59:59 +0300 Thread-Topic: [flashrom] [PATCH/FULL/SVN] Support for 4-bytes addressing SPI flash chips 32MB+ (256Mb+) Thread-Index: AdAtO4x+JJGy+RvnRY6Ox2JsQeawIgC3eAvw Message-ID: <674465BAC0F69347905C11F02369E3AB9417B75A1B@ppmail.tp-local.ru> References: <112246451.20150111022357@borisbaykov.com> In-Reply-To: <112246451.20150111022357@borisbaykov.com> Accept-Language: ru-RU Content-Language: ru-RU X-MS-Has-Attach: X-MS-TNEF-Correlator: acceptlanguage: ru-RU MIME-Version: 1.0 X-SA-Exim-Connect-IP: 192.168.5.26 X-SA-Exim-Mail-From: Aleksandr.Amelkin@t-platforms.ru X-SA-Exim-Scanned: No (on mx1.t-platforms.ru); SAEximRunCond expanded to false X-Spam-Score: -2.5 (--) Cc: "Perova, Anna" <Anna.Perova@t-platforms.ru> Subject: Re: [flashrom] [PATCH/FULL/SVN] Support for 4-bytes addressing SPI flash chips 32MB+ (256Mb+) X-BeenThere: flashrom@flashrom.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: flashrom discussion and development mailing list <flashrom.flashrom.org> List-Unsubscribe: <http://www.flashrom.org/mailman/options/flashrom>, <mailto:flashrom-request@flashrom.org?subject=unsubscribe> List-Archive: <http://www.flashrom.org/pipermail/flashrom/> List-Post: <mailto:flashrom@flashrom.org> List-Help: <mailto:flashrom-request@flashrom.org?subject=help> List-Subscribe: <http://www.flashrom.org/mailman/listinfo/flashrom>, <mailto:flashrom-request@flashrom.org?subject=subscribe> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: flashrom-bounces@flashrom.org Sender: "flashrom" <flashrom-bounces@flashrom.org>

Patchwork [PATCH/FULL/SVN] Support for 4-bytes addressing SPI flash chips 32MB+ (256Mb+)

Comments

Patch