[PATCH 1/3] mtd: spi-nor: Add die_cnt field and flags

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

Hi Cyrille,

> -----Original Message-----
> From: Cyrille Pitchen [mailto:cyrille.pitchen at atmel.com]
> Sent: Monday, October 24, 2016 3:55 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski at nokia.com>; linux-mtd at lists.infradead.org
> Cc: rfsw-patches at mlist.nokia.com; dwmw2 at infradead.org;
> computersforpeace at gmail.com; marek.vasut at gmail.com
> Subject: Re: [PATCH 1/3] mtd: spi-nor: Add die_cnt field and flags
> 
> Hi Marcin,
> 
> Le 24/10/2016 à 15:03, marcin.krzeminski at nokia.com a écrit :
> > From: Marcin Krzeminski <marcin.krzeminski at nokia.com>
> >
> > This commit adds new field and flags that could be used to signalize
> > that chip support die erase command.
> >
> > If DIE_ERASE flag will be selected but die_cnt field will be 0 chip
> > will not use chip erase command.
> >
> > Signed-off-by: Marcin Krzeminski <marcin.krzeminski at nokia.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 18 ++++++++++++++++++
> >  include/linux/mtd/spi-nor.h   |  5 +++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..3afe207 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -75,6 +75,7 @@ struct flash_info {
> >  					 * bit. Must be used with
> >  					 * SPI_NOR_HAS_LOCK.
> >  					 */
> > +#define DIE_ERASE			BIT(10)	/* Support for die erase cmd
> */
> >  };
> >
> >  #define JEDEC_MFR(info)	((info)->id[0])
> > @@ -295,6 +296,23 @@ static int erase_chip(struct spi_nor *nor)
> >  	return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0);  }
> >
> > +static int spi_nor_die_erase(struct spi_nor *nor, u32 addr) {
> > +	u8 buf[SPI_NOR_MAX_ADDR_WIDTH];
> > +	int i;
> > +
> > +	dev_dbg(nor->dev, "erase @ 0x%X\n", addr);
> > +
> > +	write_enable(nor);
> > +
> > +	for (i = nor->addr_width - 1; i >= 0; i--) {
> > +		buf[i] = addr & 0xff;
> > +		addr >>= 8;
> > +	}
> > +
> > +	return nor->write_reg(nor, SPINOR_OP_DIE_ERASE, buf,
> > +nor->addr_width);
> 
> If think you should first check whether nor->erase() is implemented by the
> SPI controller driver and call it instead of nor->write_reg().
> 
> For instance, some SPI controllers are mapped to the system memory to
> speed up Fast Read operations. In which case, they could also enable data
> cache to fasten even more the read operations.
> 
> So when the spi-nor framework calls nor->erase() or nor->write(), those
> handlers might need to invalidate the cache before performing the next
> read.
> 
> This is just an example but generally speaking, don't forget the nor->erase()
> handler! :)
> 
Thanks for extensive clarification, I will include this in v2.

Regards,
Marcin

> 
> > +}
> > +
> >  static int spi_nor_lock_and_prep(struct spi_nor *nor, enum
> > spi_nor_ops ops)  {
> >  	int ret = 0;
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index c425c7b..80154b2 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -50,6 +50,7 @@
> >  #define SPINOR_OP_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC
> chips */
> >  #define SPINOR_OP_BE_32K	0x52	/* Erase 32KiB block */
> >  #define SPINOR_OP_CHIP_ERASE	0xc7	/* Erase whole flash chip */
> > +#define SPINOR_OP_DIE_ERASE	0xc4	/* Erase whole die within chip
> */
> >  #define SPINOR_OP_SE		0xd8	/* Sector erase (usually
> 64KiB) */
> >  #define SPINOR_OP_RDID		0x9f	/* Read JEDEC ID */
> >  #define SPINOR_OP_RDCR		0x35	/* Read configuration register
> */
> > @@ -119,6 +120,7 @@ enum spi_nor_ops {  enum spi_nor_option_flags {
> >  	SNOR_F_USE_FSR		= BIT(0),
> >  	SNOR_F_HAS_SR_TB	= BIT(1),
> > +	SNOR_F_DIE_ERASE	= BIT(2),
> >  };
> >
> >  /**
> > @@ -136,6 +138,8 @@ enum spi_nor_option_flags {
> >   * @sst_write_second:	used by the SST write operation
> >   * @flags:		flag options for the current SPI-NOR (SNOR_F_*)
> >   * @cmd_buf:		used by the write_reg
> > + * @die_cnt:		number of dies in chip, if and
> SNOR_F_DIE_ERASE
> 
> The 1st flag is missing between "if" and "and", isn't it?
> > + * 			flasg is enabled CE command will be disabled
> 
> typo: s/flasg/flags/
> >   * @prepare:		[OPTIONAL] do some preparations for the
> >   *			read/write/erase/lock/unlock operations
> >   * @unprepare:		[OPTIONAL] do some post work after the
> > @@ -167,6 +171,7 @@ struct spi_nor {
> >  	bool			sst_write_second;
> >  	u32			flags;
> >  	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
> > +	u32			die_cnt;
> >
> >  	int (*prepare)(struct spi_nor *nor, enum spi_nor_ops ops);
> >  	void (*unprepare)(struct spi_nor *nor, enum spi_nor_ops ops);
> >
> 
> Best regards,
> 
> Cyrille