Re: [PATCH RESEND] mtd: spi-nor: Add support for S3AN spi-nor devices

[Ricardo Ribalda Delgado]

Hi Brian

Thanks for your comments, and sorry for sending v2 off list and also
not commenting directly to this mail. See my comments bellow

On Fri, May 6, 2016 at 2:56 AM, Brian Norris
<computersforpeace@xxxxxxxxx> wrote:
> On Wed, Apr 27, 2016 at 03:14:22PM +0200, Ricardo Ribalda Delgado wrote:

>
> Whoa this is fugly. Why does this mode exist? Can we ignore it and just
> use the poewr-of-two mode? If not, I think we need a bit more verbose of
> comments in the code to explain what/why we are doing.

Indeed it is :). This driver is for a device that is a bit special.

 FPGAs need to read the configuration from somewhere. Traditionally
this is done from a parallel flash, serial flash or another MCU that
bitbangs the configuration. The Spartan 3AN is a family of FPGA with a
serial flash embeeded in the chip:
http://www.xilinx.com/support/documentation/user_guides/ug333.pdf
This family allows you to remove one chip from your BOM.

This flash is mainly written by the FPGA tools, but it can also be
accessed by the FPGA fabric via an internal spi port,  and this is why
I have developed this driver.

The flash is almost a standard spi nor, but with some differences. The
major one being the addressing mode.

By default the flash is accessed using the Default Addressing Mode.
This mode provides an extra 3% space and the FPGA tools expect the
device to be in these mode.

The big disadvantage of this mode is that the 3 byte address is
calculated a bit differently. And therefore we need this nasty convert
function.

The FPGA can also be configured to use Power-of-two mode (standard
mode), but you lose 3% of space (a lot, considering the reduced
resources), it is an irreversible operation and this mode will corrupt
the original data. By default I believe that it is safer to keep the
initial mode. (Check from page 19 of the provided UG).


>

>
> Maybe instead of this flag, you can just use the mfr ID? So:
>
>         if (JEDEC_MFR(info) == SNOR_MFR_...) { // what would you call the "manfacturer" here anyway?
>

The MFR corresponds to Atmel, therefore we cannot use it :(. We need
to probe th whole ID, and I think this is more clean than:


if (id== ID1 || id==ID2...... || id=ID1000){
}

But if you think otherwise I do not have a strong objection to change it.

>
> Brian

Thanks for your review, if you think that there is something else
missing I will fwd v2 to the list, otherwise I will prepare a v2 with
your comments.


-- 
Ricardo Ribalda