Hi Ricardo,

On Fri, 16 Sep 2016 21:33:52 +0200
Ricardo Ribalda Delgado <ricardo.riba...@gmail.com> wrote:

> Xilinx Spartan-3AN FPGAs contain an In-System Flash where they keep
> their configuration data and (optionally) some user data.
> 
> The protocol of this flash follows most of the spi-nor standard. With
> the following differences:
> 
> - Page size might not be a power of two.
> - The address calculation (default addressing mode).
> - The spi nor commands used.
> 
> Protocol is described on Xilinx User Guide UG333
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.riba...@gmail.com>
> ---
> v5:
> -Rebase on top of l2-mtd/master
> Suggested by: Cyrille Pitchen <cyrille.pitc...@atmel.com>:
> -Fix to+1 bug
> -Move all address conversions to spi-nor
> -Replace pr_dev with dev_err
> 
> v4:
> -Rebase on top of l2-mtd/master
> 
> v3:
> -Rebase on top of mtd-next
> -Rename ADDR_NATIVE to ADDR_DEFAULT to follow UG333 naming
> -Fix bug on probe
> 
> v2: Suggested by Brian Norris <computersforpe...@gmail.com>
> 
> -Remove inline qualifier
> -Improve documentation of Default Addressing Mode
> -Convert function callbacks into SNOR_F_
> -Fix missmatch braces
> -Improve documentation of SPI_S3AN flag
>  drivers/mtd/spi-nor/spi-nor.c | 122 
> ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/mtd/spi-nor.h   |  12 +++++
>  2 files changed, 129 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d0fc165d7d66..94c5fd870058 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -75,6 +75,12 @@ struct flash_info {
>                                        * bit. Must be used with
>                                        * SPI_NOR_HAS_LOCK.
>                                        */
> +#define      SPI_S3AN                BIT(10) /*
> +                                      * Xilinx Spartan 3AN In-System Flash
> +                                      * (MFR cannot be used for probing
> +                                      * because it has the same value as
> +                                      * ATMEL flashes)
> +                                     */
>  };
>  
>  #define JEDEC_MFR(info)      ((info)->id[0])
> @@ -217,6 +223,21 @@ static inline int set_4byte(struct spi_nor *nor, const 
> struct flash_info *info,
>               return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1);
>       }
>  }
> +
> +static int s3an_sr_ready(struct spi_nor *nor)
> +{
> +     int ret;
> +     u8 val;
> +
> +     ret = nor->read_reg(nor, SPINOR_OP_XRDSR, &val, 1);
> +     if (ret < 0) {
> +             dev_err(nor->dev, "error %d reading XRDSR\n", (int) ret);
> +             return ret;
> +     }
> +
> +     return !!(val & XSR_RDY);
> +}
> +
>  static inline int spi_nor_sr_ready(struct spi_nor *nor)
>  {
>       int sr = read_sr(nor);
> @@ -238,7 +259,9 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor)
>  static int spi_nor_ready(struct spi_nor *nor)
>  {
>       int sr, fsr;
> -     sr = spi_nor_sr_ready(nor);
> +
> +     sr = nor->flags & SNOR_F_READY_XSR_RDY ? s3an_sr_ready(nor) :
> +                                              spi_nor_sr_ready(nor);

Nit: I find

        if (nor->flags & SNOR_F_READY_XSR_RDY)
                sr = s3an_sr_ready(nor);
        else
                sr = spi_nor_sr_ready(nor);

more readable.

>       if (sr < 0)
>               return sr;
>       fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
> @@ -320,6 +343,22 @@ static void spi_nor_unlock_and_unprep(struct spi_nor 
> *nor, enum spi_nor_ops ops)
>  }
>  
>  /*
> + * This code converts an address to the Default Address Mode, that has non
> + * power of two page sizes. We must support this mode because it is the 
> default
> + * mode supported by Xilinx tools, it can access the whole flash area and
> + * changing over to the Power-of-two mode is irreversible and corrupts the
> + * original data.
> + */
> +static loff_t spi_nor_s3an_addr_convert(struct spi_nor *nor, unsigned int 
> addr)
> +{
> +     unsigned int offset;
> +
> +     offset = (nor->page_size == 264) ? (addr % 264) : (addr % 528);

Why not just
        
        offset = addr % nor->page_size;

> +
> +     return ((addr - offset) << 1) | offset;
> +}
> +

Sorry for this useless review :-).

Regards,

Boris

Reply via email to