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

[Marek Vasut]

On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado 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
> 
> Reviewed-by: Cyrille Pitchen <cyrille.pitchen@xxxxxxxxx>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>
> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> Cc: Brian Norris <computersforpeace@xxxxxxxxx>
> ---

Hi, minor questions/nits below.

[...]

> -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 | 134 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/mtd/spi-nor.h   |  12 ++++
>  2 files changed, 141 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d0fc165d7d66..cf0410d4e258 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)
> +					 */

I have possibly off-topic question. Altera has something very similar --
EPCS/EPCQ flash which cannot be detected using standard READID .
Would this patch help with supporting those degenerate flashes too?

>  };
>  
>  #define JEDEC_MFR(info)	((info)->id[0])

[...]

> @@ -1170,8 +1241,15 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  
>  	for (i = 0; i < len; ) {
>  		ssize_t written;
> +		loff_t addr = to + i;
> +
> +		if (hweight32(nor->page_size) == 1) {
> +			page_offset = addr & (nor->page_size - 1);
> +		} else {
> +			uint64_t aux = addr;
>  
> -		page_offset = (to + i) & (nor->page_size - 1);
> +			page_offset = do_div(aux, nor->page_size);
> +		}

Why is this part necessary ?

>  		WARN_ONCE(page_offset,
>  			  "Writing at offset %zu into a NOR page. Writing partial pages may decrease reliability and increase wear of NOR flash.",
>  			  page_offset);

[...]

> +static int s3an_nor_scan(const struct flash_info *info, 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;
> +	}
> +
> +	nor->erase_opcode = SPINOR_OP_XSE;
> +	nor->program_opcode = SPINOR_OP_XPP;
> +	nor->read_opcode = SPINOR_OP_READ;
> +	nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
> +
> +	/* Flash in Power of 2 mode */
> +	if (val & XSR_PAGESIZE) {
> +		nor->page_size = (nor->page_size == 264) ? 256 : 512;

264 is due to some ECC ?

> +		nor->mtd.writebufsize = nor->page_size;
> +		nor->mtd.size = 8 * nor->page_size * info->n_sectors;
> +		nor->mtd.erasesize = 8 * nor->page_size;
> +	} else {
> +		nor->flags |= SNOR_F_S3AN_ADDR_DEFAULT;
> +	}
> +
> +	return 0;
> +}

[...]

-- 
Best regards,
Marek Vasut