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

From: Boris Brezillon
Date: Tue Sep 20 2016 - 07:21:12 EST


Hi Ricardo,

On Fri, 16 Sep 2016 21:33:52 +0200
Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx> 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.ribalda@xxxxxxxxx>
> ---
> v5:
> -Rebase on top of l2-mtd/master
> Suggested by: Cyrille Pitchen <cyrille.pitchen@xxxxxxxxx>:
> -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 <computersforpeace@xxxxxxxxx>
>
> -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