[PATCH 1/2] mtd: spi-nor: provide default erase_sector implementation

[PATCH 1/2] mtd: spi-nor: provide default erase_sector implementation

[Brian Norris]

Some spi-nor drivers perform sector erase by duplicating their
write_reg() command. Let's not require that the driver fill this out,
and provide a default instead.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 30 ++++++++++++++++++++++++++----
 include/linux/mtd/spi-nor.h   |  3 ++-
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 49883905a434..58a435372be7 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -313,6 +313,29 @@ static void spi_nor_unlock_and_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
 }
 
 /*
+ * Initiate the erasure of a single sector
+ */
+static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
+{
+	u8 buf[nor->addr_width];
+	int i;
+
+	if (nor->erase)
+		return nor->erase(nor, addr);
+
+	/*
+	 * Default implementation, if driver doesn't have a specialized HW
+	 * control
+	 */
+	for (i = nor->addr_width - 1; i >= 0; i--) {
+		buf[i] = addr & 0xff;
+		addr >>= 8;
+	}
+
+	return nor->write_reg(nor, nor->erase_opcode, buf, nor->addr_width);
+}
+
+/*
  * Erase an address range on the nor chip.  The address range may extend
  * one or more erase sectors.  Return an error is there is a problem erasing.
  */
@@ -371,10 +394,9 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 		while (len) {
 			write_enable(nor);
 
-			if (nor->erase(nor, addr)) {
-				ret = -EIO;
+			ret = spi_nor_erase_sector(nor, addr);
+			if (ret)
 				goto erase_err;
-			}
 
 			addr += mtd->erasesize;
 			len -= mtd->erasesize;
@@ -1138,7 +1160,7 @@ static int set_quad_mode(struct spi_nor *nor, const struct flash_info *info)
 static int spi_nor_check(struct spi_nor *nor)
 {
 	if (!nor->dev || !nor->read || !nor->write ||
-		!nor->read_reg || !nor->write_reg || !nor->erase) {
+		!nor->read_reg || !nor->write_reg) {
 		pr_err("spi-nor: please fill all the necessary fields!\n");
 		return -EINVAL;
 	}
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index c8723b62c4cd..69898b675ade 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -144,7 +144,8 @@ struct mtd_info;
  * @read:		[DRIVER-SPECIFIC] read data from the SPI NOR
  * @write:		[DRIVER-SPECIFIC] write data to the SPI NOR
  * @erase:		[DRIVER-SPECIFIC] erase a sector of the SPI NOR
- *			at the offset @offs
+ *			at the offset @offs; if not provided by the driver,
+ *			spi-nor will send the erase opcode via write_reg()
  * @flash_lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
  * @flash_unlock:	[FLASH-SPECIFIC] unlock a region of the SPI NOR
  * @flash_is_locked:	[FLASH-SPECIFIC] check if a region of the SPI NOR is
-- 
2.6.0.rc2.230.g3dd15c0

[PATCH 2/2] mtd: m25p80: drop erase() callback

[Brian Norris]

Just use the spi-nor default instead.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 4b5d7a4655fd..822b6c73dd50 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -152,22 +152,6 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	return 0;
 }
 
-static int m25p80_erase(struct spi_nor *nor, loff_t offset)
-{
-	struct m25p *flash = nor->priv;
-
-	dev_dbg(nor->dev, "%dKiB at 0x%08x\n",
-		flash->spi_nor.mtd.erasesize / 1024, (u32)offset);
-
-	/* Set up command buffer. */
-	flash->command[0] = nor->erase_opcode;
-	m25p_addr2cmd(nor, offset, flash->command);
-
-	spi_write(flash->spi, flash->command, m25p_cmdsz(nor));
-
-	return 0;
-}
-
 /*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver
@@ -194,7 +178,6 @@ static int m25p_probe(struct spi_device *spi)
 	/* install the hooks */
 	nor->read = m25p80_read;
 	nor->write = m25p80_write;
-	nor->erase = m25p80_erase;
 	nor->write_reg = m25p80_write_reg;
 	nor->read_reg = m25p80_read_reg;
 
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH 1/2] mtd: spi-nor: provide default erase_sector implementation

[Brian Norris]

On Thu, Oct 29, 2015 at 06:24:20PM -0700, Brian Norris wrote:
> Some spi-nor drivers perform sector erase by duplicating their
> write_reg() command. Let's not require that the driver fill this out,
> and provide a default instead.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 30 ++++++++++++++++++++++++++----
>  include/linux/mtd/spi-nor.h   |  3 ++-
>  2 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 49883905a434..58a435372be7 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -313,6 +313,29 @@ static void spi_nor_unlock_and_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>  }
>  
>  /*
> + * Initiate the erasure of a single sector
> + */
> +static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
> +{
> +	u8 buf[nor->addr_width];

Hmm, I see some warnings about variable length arrays here. Probably
better to just set a MAX macro with the value of 4, since the extra byte
really doesn't matter that much.

I'll send v2.

Brian

Re: [PATCH 1/2] mtd: spi-nor: provide default erase_sector implementation

[Marek Vasut]

On Friday, October 30, 2015 at 10:05:33 PM, Brian Norris wrote:
> On Thu, Oct 29, 2015 at 06:24:20PM -0700, Brian Norris wrote:
> > Some spi-nor drivers perform sector erase by duplicating their
> > write_reg() command. Let's not require that the driver fill this out,
> > and provide a default instead.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> > 
> >  drivers/mtd/spi-nor/spi-nor.c | 30 ++++++++++++++++++++++++++----
> >  include/linux/mtd/spi-nor.h   |  3 ++-
> >  2 files changed, 28 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > b/drivers/mtd/spi-nor/spi-nor.c index 49883905a434..58a435372be7 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -313,6 +313,29 @@ static void spi_nor_unlock_and_unprep(struct spi_nor
> > *nor, enum spi_nor_ops ops)
> > 
> >  }
> >  
> >  /*
> > 
> > + * Initiate the erasure of a single sector
> > + */
> > +static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
> > +{
> > +	u8 buf[nor->addr_width];
> 
> Hmm, I see some warnings about variable length arrays here. Probably
> better to just set a MAX macro with the value of 4, since the extra byte
> really doesn't matter that much.

Hm yeah, this dynamic allocation is not great. You might want to put some
sort of assertion in place, to check whether addr_width isn't > MAX .

> I'll send v2.
> 
> Brian

Best regards,
Marek Vasut