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

[Amelkin, Aleksandr]

Boris,

> I wonder if this is helpful to may people. Thanks.

I'm sure it is or will be sooner or later.
We needed this feature for sure, although flashrom with ft2232h is painfully slow.
But that's another task (probably we could use pyspiflash instead).

> > - Use of a fishing-world word 'BAIT' many times throughout the code.
> >   Probably, 'BYTE' was intended. This typo is quite alarming for me.
> 
> 4BAIT is not BYTE )))
> It's just the short form of 4-Byte Addressing Instruction Table, a part of SFDP
> 1.6

Oh, sorry then. However, a comment explaining this acronym would save me from
from making wrong guesses. Alternatively, a name like 4BA_IT could help (but a
comment would be needed anyway).

> > - 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.
> 
> I used gcc Linux so I haven't checked it with mingw. I'm sorry for these
> warnings.

Nevermind. That's not actually a big problem.
 
> > - 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?
> 
> This field is used to change the required eraser from 4BA (or EREG) to
> 4BA_direct in sfdp_change_uniform_eraser_4ba_direct in sfdp.c

I just have a feeling that this field is not actually needed.
Not a big problem as well.

> > +       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?
> 
> This code is just a copy-paste from sfdp.c to sfdp.c to make parsing of new
> dwords same as all others. All previous dwords have same code. Macro is
> better.

That's another reason why copy-pasting is a problem. Had you modified the original code
for 4BA support, reviewers would effortlessly see in the patch that this part of the code is not yours.
When you copy-paste, all the copies look like your new code in the patch.
 
> > 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.
> 
> Try to uncomment FBA_USE_EXT_ADDR_REG_BY_DEFAULT define in sfdp.c
> It may work after.

I'll try that. Thanks.

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