[PATCH] Add optional chip erase functionality to AT25 EEPROM driver.

[PATCH] Add optional chip erase functionality to AT25 EEPROM driver.

[Joe Burmeister]

Many, though not all, AT25s have an instruction for chip erase.
If there is one in the datasheet, it can be added to device tree.
Erase can then be done in userspace via the sysfs API with a new
"erase" device attribute. This matches the eeprom_93xx46 driver's
"erase".

Signed-off-by: Joe Burmeister <joe.burmeister@devtank.co.uk>
---
 .../devicetree/bindings/eeprom/at25.txt       |  2 +
 drivers/misc/eeprom/at25.c                    | 83 ++++++++++++++++++-
 2 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/eeprom/at25.txt b/Documentation/devicetree/bindings/eeprom/at25.txt
index b3bde97dc199..c65d11e14c7a 100644
--- a/Documentation/devicetree/bindings/eeprom/at25.txt
+++ b/Documentation/devicetree/bindings/eeprom/at25.txt
@@ -19,6 +19,7 @@ Optional properties:
 - spi-cpha : SPI shifted clock phase, as per spi-bus bindings.
 - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings.
 - read-only : this parameter-less property disables writes to the eeprom
+- chip_erase_instruction : Chip erase instruction for this AT25, often 0xc7 or 0x62.
 
 Obsolete legacy properties can be used in place of "size", "pagesize",
 "address-width", and "read-only":
@@ -39,4 +40,5 @@ Example:
 		pagesize = <64>;
 		size = <32768>;
 		address-width = <16>;
+		chip_erase_instruction = <0x62>;
 	};
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 99de6939cd5a..28141bc4028f 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -35,6 +35,7 @@ struct at25_data {
 	unsigned		addrlen;
 	struct nvmem_config	nvmem_config;
 	struct nvmem_device	*nvmem;
+	u8			erase_instr;
 };
 
 #define	AT25_WREN	0x06		/* latch the write enable */
@@ -59,6 +60,8 @@ struct at25_data {
  */
 #define	EE_TIMEOUT	25
 
+#define ERASE_TIMEOUT 2020
+
 /*-------------------------------------------------------------------------*/
 
 #define	io_limit	PAGE_SIZE	/* bytes */
@@ -304,6 +307,71 @@ static int at25_fw_to_chip(struct device *dev, struct spi_eeprom *chip)
 	return 0;
 }
 
+static void _eeprom_at25_store_erase_locked(struct at25_data *at25)
+{
+	unsigned long	timeout, retries;
+	int				sr, status;
+	u8	cp;
+
+	cp = AT25_WREN;
+	status = spi_write(at25->spi, &cp, 1);
+	if (status < 0) {
+		dev_dbg(&at25->spi->dev, "ERASE WREN --> %d\n", status);
+		return;
+	}
+	cp = at25->erase_instr;
+	status = spi_write(at25->spi, &cp, 1);
+	if (status < 0) {
+		dev_dbg(&at25->spi->dev, "CHIP_ERASE --> %d\n", status);
+		return;
+	}
+	/* Wait for non-busy status */
+	timeout = jiffies + msecs_to_jiffies(ERASE_TIMEOUT);
+	retries = 0;
+	do {
+		sr = spi_w8r8(at25->spi, AT25_RDSR);
+		if (sr < 0 || (sr & AT25_SR_nRDY)) {
+			dev_dbg(&at25->spi->dev,
+				"rdsr --> %d (%02x)\n", sr, sr);
+			/* at HZ=100, this is sloooow */
+			msleep(1);
+			continue;
+		}
+		if (!(sr & AT25_SR_nRDY))
+			return;
+	} while (retries++ < 200 || time_before_eq(jiffies, timeout));
+
+	if ((sr < 0) || (sr & AT25_SR_nRDY)) {
+		dev_err(&at25->spi->dev,
+			"chip erase, timeout after %u msecs\n",
+			jiffies_to_msecs(jiffies -
+				(timeout - ERASE_TIMEOUT)));
+		status = -ETIMEDOUT;
+		return;
+	}
+}
+
+
+static ssize_t eeprom_at25_store_erase(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct at25_data *at25 = dev_get_drvdata(dev);
+	int erase = 0;
+
+	sscanf(buf, "%d", &erase);
+	if (erase) {
+		mutex_lock(&at25->lock);
+		_eeprom_at25_store_erase_locked(at25);
+		mutex_unlock(&at25->lock);
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_at25_store_erase);
+
+
 static int at25_probe(struct spi_device *spi)
 {
 	struct at25_data	*at25 = NULL;
@@ -311,6 +379,7 @@ static int at25_probe(struct spi_device *spi)
 	int			err;
 	int			sr;
 	int			addrlen;
+	int			has_erase;
 
 	/* Chip description */
 	if (!spi->dev.platform_data) {
@@ -352,6 +421,9 @@ static int at25_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, at25);
 	at25->addrlen = addrlen;
 
+	/* Optional chip erase instruction */
+	device_property_read_u8(&spi->dev, "chip_erase_instruction", &at25->erase_instr);
+
 	at25->nvmem_config.name = dev_name(&spi->dev);
 	at25->nvmem_config.dev = &spi->dev;
 	at25->nvmem_config.read_only = chip.flags & EE_READONLY;
@@ -370,17 +442,22 @@ static int at25_probe(struct spi_device *spi)
 	if (IS_ERR(at25->nvmem))
 		return PTR_ERR(at25->nvmem);
 
-	dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u\n",
+	has_erase = (!(chip.flags & EE_READONLY) && at25->erase_instr);
+
+	dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u%s\n",
 		(chip.byte_len < 1024) ? chip.byte_len : (chip.byte_len / 1024),
 		(chip.byte_len < 1024) ? "Byte" : "KByte",
 		at25->chip.name,
 		(chip.flags & EE_READONLY) ? " (readonly)" : "",
-		at25->chip.page_size);
+		at25->chip.page_size, (has_erase)?" <has erase>":"");
+
+	if (has_erase && device_create_file(&spi->dev, &dev_attr_erase))
+		dev_err(&spi->dev, "can't create erase interface\n");
+
 	return 0;
 }
 
 /*-------------------------------------------------------------------------*/
-
 static const struct of_device_id at25_of_match[] = {
 	{ .compatible = "atmel,at25", },
 	{ }
-- 
2.20.1

Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.

[Greg Kroah-Hartman]

On Fri, Aug 09, 2019 at 01:53:55PM +0100, Joe Burmeister wrote:
> +static void _eeprom_at25_store_erase_locked(struct at25_data *at25)
> +{
> +	unsigned long	timeout, retries;
> +	int				sr, status;
> +	u8	cp;
> +
> +	cp = AT25_WREN;
> +	status = spi_write(at25->spi, &cp, 1);
> +	if (status < 0) {
> +		dev_dbg(&at25->spi->dev, "ERASE WREN --> %d\n", status);
> +		return;
> +	}
> +	cp = at25->erase_instr;
> +	status = spi_write(at25->spi, &cp, 1);
> +	if (status < 0) {
> +		dev_dbg(&at25->spi->dev, "CHIP_ERASE --> %d\n", status);
> +		return;
> +	}
> +	/* Wait for non-busy status */
> +	timeout = jiffies + msecs_to_jiffies(ERASE_TIMEOUT);
> +	retries = 0;
> +	do {
> +		sr = spi_w8r8(at25->spi, AT25_RDSR);
> +		if (sr < 0 || (sr & AT25_SR_nRDY)) {
> +			dev_dbg(&at25->spi->dev,
> +				"rdsr --> %d (%02x)\n", sr, sr);
> +			/* at HZ=100, this is sloooow */
> +			msleep(1);
> +			continue;
> +		}
> +		if (!(sr & AT25_SR_nRDY))
> +			return;
> +	} while (retries++ < 200 || time_before_eq(jiffies, timeout));
> +
> +	if ((sr < 0) || (sr & AT25_SR_nRDY)) {
> +		dev_err(&at25->spi->dev,
> +			"chip erase, timeout after %u msecs\n",
> +			jiffies_to_msecs(jiffies -
> +				(timeout - ERASE_TIMEOUT)));
> +		status = -ETIMEDOUT;
> +		return;
> +	}
> +}
> +
> +

No need for 2 lines :(

> +static ssize_t eeprom_at25_store_erase(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	struct at25_data *at25 = dev_get_drvdata(dev);
> +	int erase = 0;
> +
> +	sscanf(buf, "%d", &erase);
> +	if (erase) {
> +		mutex_lock(&at25->lock);
> +		_eeprom_at25_store_erase_locked(at25);
> +		mutex_unlock(&at25->lock);
> +	}
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_at25_store_erase);
> +
> +

Same here.

Also, where is the Documentation/ABI/ update for the new sysfs file?

>  static int at25_probe(struct spi_device *spi)
>  {
>  	struct at25_data	*at25 = NULL;
> @@ -311,6 +379,7 @@ static int at25_probe(struct spi_device *spi)
>  	int			err;
>  	int			sr;
>  	int			addrlen;
> +	int			has_erase;
>  
>  	/* Chip description */
>  	if (!spi->dev.platform_data) {
> @@ -352,6 +421,9 @@ static int at25_probe(struct spi_device *spi)
>  	spi_set_drvdata(spi, at25);
>  	at25->addrlen = addrlen;
>  
> +	/* Optional chip erase instruction */
> +	device_property_read_u8(&spi->dev, "chip_erase_instruction", &at25->erase_instr);
> +
>  	at25->nvmem_config.name = dev_name(&spi->dev);
>  	at25->nvmem_config.dev = &spi->dev;
>  	at25->nvmem_config.read_only = chip.flags & EE_READONLY;
> @@ -370,17 +442,22 @@ static int at25_probe(struct spi_device *spi)
>  	if (IS_ERR(at25->nvmem))
>  		return PTR_ERR(at25->nvmem);
>  
> -	dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u\n",
> +	has_erase = (!(chip.flags & EE_READONLY) && at25->erase_instr);
> +
> +	dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u%s\n",
>  		(chip.byte_len < 1024) ? chip.byte_len : (chip.byte_len / 1024),
>  		(chip.byte_len < 1024) ? "Byte" : "KByte",
>  		at25->chip.name,
>  		(chip.flags & EE_READONLY) ? " (readonly)" : "",
> -		at25->chip.page_size);
> +		at25->chip.page_size, (has_erase)?" <has erase>":"");
> +
> +	if (has_erase && device_create_file(&spi->dev, &dev_attr_erase))
> +		dev_err(&spi->dev, "can't create erase interface\n");

You just raced with userspace and lost :(

Please create an attribute group and add it to the .dev_groups pointer
in struct driver so it can be created properly in a race-free manner.

See the thread at:
	https://lore.kernel.org/r/20190731124349.4474-2-gregkh@linuxfoundation.org
for the details on how to do that.

thanks,

greg k-h

Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.

[Joe Burmeister]

Hi Greg,

On 09/08/2019 14:00, Greg Kroah-Hartman wrote:
> On Fri, Aug 09, 2019 at 01:53:55PM +0100, Joe Burmeister wrote:
>> +static void _eeprom_at25_store_erase_locked(struct at25_data *at25)
>> +{
>> +	unsigned long	timeout, retries;
>> +	int				sr, status;
>> +	u8	cp;
>> +
>> +	cp = AT25_WREN;
>> +	status = spi_write(at25->spi, &cp, 1);
>> +	if (status < 0) {
>> +		dev_dbg(&at25->spi->dev, "ERASE WREN --> %d\n", status);
>> +		return;
>> +	}
>> +	cp = at25->erase_instr;
>> +	status = spi_write(at25->spi, &cp, 1);
>> +	if (status < 0) {
>> +		dev_dbg(&at25->spi->dev, "CHIP_ERASE --> %d\n", status);
>> +		return;
>> +	}
>> +	/* Wait for non-busy status */
>> +	timeout = jiffies + msecs_to_jiffies(ERASE_TIMEOUT);
>> +	retries = 0;
>> +	do {
>> +		sr = spi_w8r8(at25->spi, AT25_RDSR);
>> +		if (sr < 0 || (sr & AT25_SR_nRDY)) {
>> +			dev_dbg(&at25->spi->dev,
>> +				"rdsr --> %d (%02x)\n", sr, sr);
>> +			/* at HZ=100, this is sloooow */
>> +			msleep(1);
>> +			continue;
>> +		}
>> +		if (!(sr & AT25_SR_nRDY))
>> +			return;
>> +	} while (retries++ < 200 || time_before_eq(jiffies, timeout));
>> +
>> +	if ((sr < 0) || (sr & AT25_SR_nRDY)) {
>> +		dev_err(&at25->spi->dev,
>> +			"chip erase, timeout after %u msecs\n",
>> +			jiffies_to_msecs(jiffies -
>> +				(timeout - ERASE_TIMEOUT)));
>> +		status = -ETIMEDOUT;
>> +		return;
>> +	}
>> +}
>> +
>> +
> No need for 2 lines :(

Sorry, other coding conventions I'm used to.


>> +static ssize_t eeprom_at25_store_erase(struct device *dev,
>> +					 struct device_attribute *attr,
>> +					 const char *buf, size_t count)
>> +{
>> +	struct at25_data *at25 = dev_get_drvdata(dev);
>> +	int erase = 0;
>> +
>> +	sscanf(buf, "%d", &erase);
>> +	if (erase) {
>> +		mutex_lock(&at25->lock);
>> +		_eeprom_at25_store_erase_locked(at25);
>> +		mutex_unlock(&at25->lock);
>> +	}
>> +
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_at25_store_erase);
>> +
>> +
> Same here.
>
> Also, where is the Documentation/ABI/ update for the new sysfs file?

There isn't anything for the existing SPI EEPROM stuff I can see.

Would I have to document what was already there to add my bit?


>>   static int at25_probe(struct spi_device *spi)
>>   {
>>   	struct at25_data	*at25 = NULL;
>> @@ -311,6 +379,7 @@ static int at25_probe(struct spi_device *spi)
>>   	int			err;
>>   	int			sr;
>>   	int			addrlen;
>> +	int			has_erase;
>>   
>>   	/* Chip description */
>>   	if (!spi->dev.platform_data) {
>> @@ -352,6 +421,9 @@ static int at25_probe(struct spi_device *spi)
>>   	spi_set_drvdata(spi, at25);
>>   	at25->addrlen = addrlen;
>>   
>> +	/* Optional chip erase instruction */
>> +	device_property_read_u8(&spi->dev, "chip_erase_instruction", &at25->erase_instr);
>> +
>>   	at25->nvmem_config.name = dev_name(&spi->dev);
>>   	at25->nvmem_config.dev = &spi->dev;
>>   	at25->nvmem_config.read_only = chip.flags & EE_READONLY;
>> @@ -370,17 +442,22 @@ static int at25_probe(struct spi_device *spi)
>>   	if (IS_ERR(at25->nvmem))
>>   		return PTR_ERR(at25->nvmem);
>>   
>> -	dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u\n",
>> +	has_erase = (!(chip.flags & EE_READONLY) && at25->erase_instr);
>> +
>> +	dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u%s\n",
>>   		(chip.byte_len < 1024) ? chip.byte_len : (chip.byte_len / 1024),
>>   		(chip.byte_len < 1024) ? "Byte" : "KByte",
>>   		at25->chip.name,
>>   		(chip.flags & EE_READONLY) ? " (readonly)" : "",
>> -		at25->chip.page_size);
>> +		at25->chip.page_size, (has_erase)?" <has erase>":"");
>> +
>> +	if (has_erase && device_create_file(&spi->dev, &dev_attr_erase))
>> +		dev_err(&spi->dev, "can't create erase interface\n");
> You just raced with userspace and lost :(
>
> Please create an attribute group and add it to the .dev_groups pointer
> in struct driver so it can be created properly in a race-free manner.
>
> See the thread at:
> 	https://lore.kernel.org/r/20190731124349.4474-2-gregkh@linuxfoundation.org
> for the details on how to do that.

Clearly I didn't know about that. I'll do some reading when I've got a 
bit of time and try a again.


> thanks,
>
> greg k-h

Cheers,

Joe

Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.

[Greg Kroah-Hartman]

On Fri, Aug 09, 2019 at 02:18:24PM +0100, Joe Burmeister wrote:
> Hi Greg,
> 
> On 09/08/2019 14:00, Greg Kroah-Hartman wrote:
> > On Fri, Aug 09, 2019 at 01:53:55PM +0100, Joe Burmeister wrote:
> > > +static void _eeprom_at25_store_erase_locked(struct at25_data *at25)
> > > +{
> > > +	unsigned long	timeout, retries;
> > > +	int				sr, status;
> > > +	u8	cp;
> > > +
> > > +	cp = AT25_WREN;
> > > +	status = spi_write(at25->spi, &cp, 1);
> > > +	if (status < 0) {
> > > +		dev_dbg(&at25->spi->dev, "ERASE WREN --> %d\n", status);
> > > +		return;
> > > +	}
> > > +	cp = at25->erase_instr;
> > > +	status = spi_write(at25->spi, &cp, 1);
> > > +	if (status < 0) {
> > > +		dev_dbg(&at25->spi->dev, "CHIP_ERASE --> %d\n", status);
> > > +		return;
> > > +	}
> > > +	/* Wait for non-busy status */
> > > +	timeout = jiffies + msecs_to_jiffies(ERASE_TIMEOUT);
> > > +	retries = 0;
> > > +	do {
> > > +		sr = spi_w8r8(at25->spi, AT25_RDSR);
> > > +		if (sr < 0 || (sr & AT25_SR_nRDY)) {
> > > +			dev_dbg(&at25->spi->dev,
> > > +				"rdsr --> %d (%02x)\n", sr, sr);
> > > +			/* at HZ=100, this is sloooow */
> > > +			msleep(1);
> > > +			continue;
> > > +		}
> > > +		if (!(sr & AT25_SR_nRDY))
> > > +			return;
> > > +	} while (retries++ < 200 || time_before_eq(jiffies, timeout));
> > > +
> > > +	if ((sr < 0) || (sr & AT25_SR_nRDY)) {
> > > +		dev_err(&at25->spi->dev,
> > > +			"chip erase, timeout after %u msecs\n",
> > > +			jiffies_to_msecs(jiffies -
> > > +				(timeout - ERASE_TIMEOUT)));
> > > +		status = -ETIMEDOUT;
> > > +		return;
> > > +	}
> > > +}
> > > +
> > > +
> > No need for 2 lines :(
> 
> Sorry, other coding conventions I'm used to.

checkpatch.pl should have warned you about this, you did run that before
sending your patch out, right?

> > > +static ssize_t eeprom_at25_store_erase(struct device *dev,
> > > +					 struct device_attribute *attr,
> > > +					 const char *buf, size_t count)
> > > +{
> > > +	struct at25_data *at25 = dev_get_drvdata(dev);
> > > +	int erase = 0;
> > > +
> > > +	sscanf(buf, "%d", &erase);
> > > +	if (erase) {
> > > +		mutex_lock(&at25->lock);
> > > +		_eeprom_at25_store_erase_locked(at25);
> > > +		mutex_unlock(&at25->lock);
> > > +	}
> > > +
> > > +	return count;
> > > +}
> > > +
> > > +static DEVICE_ATTR(erase, S_IWUSR, NULL, eeprom_at25_store_erase);
> > > +
> > > +
> > Same here.
> > 
> > Also, where is the Documentation/ABI/ update for the new sysfs file?
> 
> There isn't anything for the existing SPI EEPROM stuff I can see.
> 
> Would I have to document what was already there to add my bit?

Yes, someone has to, sorry :)

thanks,

greg k-h

Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.

[Mark Rutland]

On Fri, Aug 09, 2019 at 01:53:55PM +0100, Joe Burmeister wrote:
> Many, though not all, AT25s have an instruction for chip erase.
> If there is one in the datasheet, it can be added to device tree.
> Erase can then be done in userspace via the sysfs API with a new
> "erase" device attribute. This matches the eeprom_93xx46 driver's
> "erase".
> 
> Signed-off-by: Joe Burmeister <joe.burmeister@devtank.co.uk>
> ---
>  .../devicetree/bindings/eeprom/at25.txt       |  2 +
>  drivers/misc/eeprom/at25.c                    | 83 ++++++++++++++++++-
>  2 files changed, 82 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/eeprom/at25.txt b/Documentation/devicetree/bindings/eeprom/at25.txt
> index b3bde97dc199..c65d11e14c7a 100644
> --- a/Documentation/devicetree/bindings/eeprom/at25.txt
> +++ b/Documentation/devicetree/bindings/eeprom/at25.txt
> @@ -19,6 +19,7 @@ Optional properties:
>  - spi-cpha : SPI shifted clock phase, as per spi-bus bindings.
>  - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings.
>  - read-only : this parameter-less property disables writes to the eeprom
> +- chip_erase_instruction : Chip erase instruction for this AT25, often 0xc7 or 0x62.

This should be using '-' rather than '_', as per general DT conventions
and as with the existing properties.

>  Obsolete legacy properties can be used in place of "size", "pagesize",
>  "address-width", and "read-only":
> @@ -39,4 +40,5 @@ Example:
>  		pagesize = <64>;
>  		size = <32768>;
>  		address-width = <16>;
> +		chip_erase_instruction = <0x62>;

[...]

> +	/* Optional chip erase instruction */
> +	device_property_read_u8(&spi->dev, "chip_erase_instruction", &at25->erase_instr);

This will not behave as you expect, since you didn't mark the property as
8-bits.

Read this as a u32 into the existing val temporary variable, as is done
for pagesize. You can add a warnign if it's out-of-range.

Thanks,
Mark.

Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.

[Joe Burmeister]

Hi Mark,


On 09/08/2019 14:54, Mark Rutland wrote:
> On Fri, Aug 09, 2019 at 01:53:55PM +0100, Joe Burmeister wrote:
>> Many, though not all, AT25s have an instruction for chip erase.
>> If there is one in the datasheet, it can be added to device tree.
>> Erase can then be done in userspace via the sysfs API with a new
>> "erase" device attribute. This matches the eeprom_93xx46 driver's
>> "erase".
>>
>> Signed-off-by: Joe Burmeister <joe.burmeister@devtank.co.uk>
>> ---
>>   .../devicetree/bindings/eeprom/at25.txt       |  2 +
>>   drivers/misc/eeprom/at25.c                    | 83 ++++++++++++++++++-
>>   2 files changed, 82 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/eeprom/at25.txt b/Documentation/devicetree/bindings/eeprom/at25.txt
>> index b3bde97dc199..c65d11e14c7a 100644
>> --- a/Documentation/devicetree/bindings/eeprom/at25.txt
>> +++ b/Documentation/devicetree/bindings/eeprom/at25.txt
>> @@ -19,6 +19,7 @@ Optional properties:
>>   - spi-cpha : SPI shifted clock phase, as per spi-bus bindings.
>>   - spi-cpol : SPI inverse clock polarity, as per spi-bus bindings.
>>   - read-only : this parameter-less property disables writes to the eeprom
>> +- chip_erase_instruction : Chip erase instruction for this AT25, often 0xc7 or 0x62.
> This should be using '-' rather than '_', as per general DT conventions
> and as with the existing properties.
>
>>   Obsolete legacy properties can be used in place of "size", "pagesize",
>>   "address-width", and "read-only":
>> @@ -39,4 +40,5 @@ Example:
>>   		pagesize = <64>;
>>   		size = <32768>;
>>   		address-width = <16>;
>> +		chip_erase_instruction = <0x62>;
> [...]
>
>> +	/* Optional chip erase instruction */
>> +	device_property_read_u8(&spi->dev, "chip_erase_instruction", &at25->erase_instr);
> This will not behave as you expect, since you didn't mark the property as
> 8-bits.

Rats, I forgot to update the documentation. In my device tree I have 
already found the /bit 8/ bit.

> Read this as a u32 into the existing val temporary variable, as is done
> for pagesize. You can add a warnign if it's out-of-range.

32bit would certainly read better in the device tree. I'll do that.

> Thanks,
> Mark.


Cheers,


Joe

RE: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.

[David Laight]

From: Joe Burmeister
> Sent: 09 August 2019 13:54
> 
> Many, though not all, AT25s have an instruction for chip erase.
> If there is one in the datasheet, it can be added to device tree.
> Erase can then be done in userspace via the sysfs API with a new
> "erase" device attribute. This matches the eeprom_93xx46 driver's
> "erase".

Is it actually worth doing though?

I'm guessing that device erase can easily take over a minute.

When I looked at 'device erase' on an EEPROM it took just as long
as erasing the sectors one at a time - but without the warm cosy
feeling that progress was being made.

Not only that you can't really interrupt the erase, so either
the application has to sleep uninterruptibly for the duration
or you have to have some kind of 'device busy' response while
it is done asynchronously.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.

[Joe Burmeister]

On 12/08/2019 16:51, David Laight wrote:
> From: Joe Burmeister
>> Sent: 09 August 2019 13:54
>>
>> Many, though not all, AT25s have an instruction for chip erase.
>> If there is one in the datasheet, it can be added to device tree.
>> Erase can then be done in userspace via the sysfs API with a new
>> "erase" device attribute. This matches the eeprom_93xx46 driver's
>> "erase".
> Is it actually worth doing though?
>
> I'm guessing that device erase can easily take over a minute.


That must depend on the AT25. The one we're using (AT25F512A), as it's 
setup, it's fast enough. The datasheet states "The CHIP ERASE cycle time 
typically is 2 seconds.". I've not timed it's because as I said, seamed 
fast enough.


If you can't erase it, then it's basically write once, or you expose it 
with spi_dev to Flashrom to erase it.


> When I looked at 'device erase' on an EEPROM it took just as long
> as erasing the sectors one at a time - but without the warm cosy
> feeling that progress was being made.


I didn't look at sector erase as I'm only interested in erasing it all 
and there was a command for it. I figured if someone wanted sector by 
sector they would implement it.



> Not only that you can't really interrupt the erase, so either
> the application has to sleep uninterruptibly for the duration
> or you have to have some kind of 'device busy' response while
> it is done asynchronously.


That's true, but as I said, it's fast enough it's not an issue for us.

> 	David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Regards,

Joe

Re: [PATCH] Add optional chip erase functionality to AT25 EEPROM driver.

[Rob Herring]

On Fri, Aug 09, 2019 at 01:53:55PM +0100, Joe Burmeister wrote:
> Many, though not all, AT25s have an instruction for chip erase.
> If there is one in the datasheet, it can be added to device tree.
> Erase can then be done in userspace via the sysfs API with a new
> "erase" device attribute. This matches the eeprom_93xx46 driver's
> "erase".
> 
> Signed-off-by: Joe Burmeister <joe.burmeister@devtank.co.uk>
> ---
>  .../devicetree/bindings/eeprom/at25.txt       |  2 +

Please split bindings to a separate patch.

>  drivers/misc/eeprom/at25.c                    | 83 ++++++++++++++++++-
>  2 files changed, 82 insertions(+), 3 deletions(-)