Re: [flashrom] [PATCH] Add support for SST26VF064B

[Stefan Tauner]

On Mon, 31 Mar 2014 22:18:48 -0700
Wei Hu  wrote:
> Datasheet available at
> http://ww1.microchip.com/downloads/en/DeviceDoc/20005119D.pdf

Hi,

thank you for your patch! I have reviewed it and I think you got a few
things wrong:
- Most importantly, I think the block erase sizes are non-uniform
  (for opcode 0xD8). See note 11 of Table 5.1 and Section 3.0 etc.
- Also, the ULBPR requires a WREN opcode sent before it. OTOH your
  spi_disable_blockprotect_generic() calls seems useless (all bits of
  the status register are read-only).
- The spi_prettyprint_status_register_sst25 function is completely
  wrong for these chips.
- The OTP memory, and other advanced features were not documented at
  all.
- Debatably the name of the chip should include the optional A that
  indicates the disable default state of the WP/Hold feature of this
  chip variant.

I have attached my counterproposal that also adds support for the
SST26VF032B(A). Please take a look and tell me if you disagree with
anything I have changed, thanks.

[PATCH] Add support for SST26VF064B

[Wei Hu]

Stefan,

Thanks for your review and your new patch. Much appreciated.

On Sun, May 4, 2014 at 1:06 PM, Stefan Tauner
 wrote:
> On Mon, 31 Mar 2014 22:18:48 -0700
> Wei Hu  wrote:
> > Datasheet available at
> > http://ww1.microchip.com/downloads/en/DeviceDoc/20005119D.pdf
>
> Hi,
>
> thank you for your patch! I have reviewed it and I think you got a few
> things wrong:
> Most importantly, I think the block erase sizes are non-uniform
> (for opcode 0xD8). See note 11 of Table 5.1 and Section 3.0 etc.

Thanks for fixing the non-uniform layout! Learned something new today.
> Also, the ULBPR requires a WREN opcode sent before it. OTOH your
> spi_disable_blockprotect_generic() calls seems useless (all bits of
> the status register are read-only).

I know spi_disable_blockprotect_generic() is useless here, but I
thought it was a good idea to call the generic function first in
general? As a side effect of calling this function I'm sending WREN,
so I didn't send on explicitly.
> The spi_prettyprint_status_register_sst25 function is completely
> wrong for these chips.
> The OTP memory, and other advanced features were not documented at
> all.

What's OTP? I didn't see it in the datasheet. Why do you want that?
> Debatably the name of the chip should include the optional A that
> indicates the disable default state of the WP/Hold feature of this
> chip variant.
>
> I have attached my counterproposal that also adds support for the
> SST26VF032B(A). Please take a look and tell me if you disagree with
> anything I have changed, thanks.