[PATCH 0/3] Minor fix and new Chip IDs for spi-nor.

[PATCH 0/3] Minor fix and new Chip IDs for spi-nor.

[Michal Suchanek]

Hello,

I tried attaching random flash chips with an ICP cable and found that Linux does not have the chip IDs.

Thanks

Michal

Michal Suchanek (3):
  MTD: m25p80: fix write return value.
  MTD: spi-nor: Add Pm25LD020 and GD25Q41B chip ID.
  MTD: spi-nor: add flag to not use sector erase.

 drivers/mtd/devices/m25p80.c  | 2 +-
 drivers/mtd/spi-nor/spi-nor.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.1.4

[PATCH 1/3] MTD: m25p80: fix write return value.

[Michal Suchanek]

The 'retlen' points to a variable representing the number of data bytes
written/read (see include/linux/mtd/mtd.h) by the current invocation of
the function. This variable must be set, not incremented.

v2: clearer commit message

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
Acked-by: Marek Vasut <marex@denx.de>
---
 drivers/mtd/devices/m25p80.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7c8b169..0b2bc21 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
 
 	spi_sync(spi, &m);
 
-	*retlen += m.actual_length - cmd_sz;
+	*retlen = m.actual_length - cmd_sz;
 }
 
 static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
-- 
2.1.4

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Marek Vasut]

On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote:
> The 'retlen' points to a variable representing the number of data bytes
> written/read (see include/linux/mtd/mtd.h) by the current invocation of
> the function. This variable must be set, not incremented.
> 
> v2: clearer commit message

This V2 goes past the diffstat, so that when the patch is applied, it
doesn't end up in the log.

> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> Acked-by: Marek Vasut <marex@denx.de>
> ---
>  drivers/mtd/devices/m25p80.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Aka. V<n> changes go here.

I don't think there's a need to repost tho :)

> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7c8b169..0b2bc21 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t
> to, size_t len,
> 
>  	spi_sync(spi, &m);
> 
> -	*retlen += m.actual_length - cmd_sz;
> +	*retlen = m.actual_length - cmd_sz;
>  }
> 
>  static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

Best regards,
Marek Vasut

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Marek Vasut]

On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote:
> The 'retlen' points to a variable representing the number of data bytes
> written/read (see include/linux/mtd/mtd.h) by the current invocation of
> the function. This variable must be set, not incremented.
> 
> v2: clearer commit message

This V2 goes past the diffstat, so that when the patch is applied, it
doesn't end up in the log.

> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> Acked-by: Marek Vasut <marex@denx.de>
> ---
>  drivers/mtd/devices/m25p80.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Aka. V<n> changes go here.

I don't think there's a need to repost tho :)

> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7c8b169..0b2bc21 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t
> to, size_t len,
> 
>  	spi_sync(spi, &m);
> 
> -	*retlen += m.actual_length - cmd_sz;
> +	*retlen = m.actual_length - cmd_sz;
>  }
> 
>  static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

Best regards,
Marek Vasut

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Brian Norris]

On Thu, Apr 30, 2015 at 03:33:47PM +0200, Michal Suchanek wrote:
> The 'retlen' points to a variable representing the number of data bytes
> written/read (see include/linux/mtd/mtd.h) by the current invocation of
> the function. This variable must be set, not incremented.
> 
> v2: clearer commit message
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> Acked-by: Marek Vasut <marex@denx.de>
> ---
>  drivers/mtd/devices/m25p80.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7c8b169..0b2bc21 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>  
>  	spi_sync(spi, &m);
>  
> -	*retlen += m.actual_length - cmd_sz;
> +	*retlen = m.actual_length - cmd_sz;

This is very wrong. It gets a little better once you add your next
patches (which are also not good) since those patches reinterpret the
usage of retlen, but this one definitely does *not* stand a lone.

I'll admit the API is a little odd here, but the callers of this
function (see spi_nor_write()) actually depend on calling this multiple
times, with the value incrementing each time so we get a summary total.
You're now clobbering this value.

I'm willing to accept patches to improve this API, if you think that
would help. Or to add comments that document this.

>  }
>  
>  static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

Also, I'm a little confused because you sent two different patch series
very close to each other, and this patch is in both of them. Please
don't do that. Either send a single patch series that contains all
patches (and is versions v2, v3, etc., as you revise the whole thing) or
send completely independent patch series. Don't include the same patch
in different series.

Anyway, please consider my comments, and when you have something better,
please resend everything. I'm not going to take either series as-is.

Thanks,
Brian

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Brian Norris]

On Thu, Apr 30, 2015 at 03:33:47PM +0200, Michal Suchanek wrote:
> The 'retlen' points to a variable representing the number of data bytes
> written/read (see include/linux/mtd/mtd.h) by the current invocation of
> the function. This variable must be set, not incremented.
> 
> v2: clearer commit message
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> Acked-by: Marek Vasut <marex@denx.de>
> ---
>  drivers/mtd/devices/m25p80.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7c8b169..0b2bc21 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>  
>  	spi_sync(spi, &m);
>  
> -	*retlen += m.actual_length - cmd_sz;
> +	*retlen = m.actual_length - cmd_sz;

This is very wrong. It gets a little better once you add your next
patches (which are also not good) since those patches reinterpret the
usage of retlen, but this one definitely does *not* stand a lone.

I'll admit the API is a little odd here, but the callers of this
function (see spi_nor_write()) actually depend on calling this multiple
times, with the value incrementing each time so we get a summary total.
You're now clobbering this value.

I'm willing to accept patches to improve this API, if you think that
would help. Or to add comments that document this.

>  }
>  
>  static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

Also, I'm a little confused because you sent two different patch series
very close to each other, and this patch is in both of them. Please
don't do that. Either send a single patch series that contains all
patches (and is versions v2, v3, etc., as you revise the whole thing) or
send completely independent patch series. Don't include the same patch
in different series.

Anyway, please consider my comments, and when you have something better,
please resend everything. I'm not going to take either series as-is.

Thanks,
Brian

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Michal Suchanek]

Hello,

On 21 May 2015 at 01:45, Brian Norris <computersforpeace@gmail.com> wrote:
> On Thu, Apr 30, 2015 at 03:33:47PM +0200, Michal Suchanek wrote:
>> The 'retlen' points to a variable representing the number of data bytes
>> written/read (see include/linux/mtd/mtd.h) by the current invocation of
>> the function. This variable must be set, not incremented.
>>
>> v2: clearer commit message
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>> Acked-by: Marek Vasut <marex@denx.de>
>> ---
>>  drivers/mtd/devices/m25p80.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 7c8b169..0b2bc21 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>>
>>       spi_sync(spi, &m);
>>
>> -     *retlen += m.actual_length - cmd_sz;
>> +     *retlen = m.actual_length - cmd_sz;
>
> This is very wrong. It gets a little better once you add your next
> patches (which are also not good) since those patches reinterpret the
> usage of retlen, but this one definitely does *not* stand a lone.
>
> I'll admit the API is a little odd here, but the callers of this
> function (see spi_nor_write()) actually depend on calling this multiple
> times, with the value incrementing each time so we get a summary total.
> You're now clobbering this value.
>
> I'm willing to accept patches to improve this API, if you think that
> would help. Or to add comments that document this.

Yes, the only user of the retlen value ignores it but passes it on so
without the following patch this one makes the passed on value
different from before.

For m25p80 this would be fixed by truncating the write in the driver
and setting actual_length appropriately rather than returning an error
when the message is too long. It might possibly break other drivers,
though.

Thanks

Michal

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Michal Suchanek]

Hello,

On 21 May 2015 at 01:45, Brian Norris <computersforpeace@gmail.com> wrote:
> On Thu, Apr 30, 2015 at 03:33:47PM +0200, Michal Suchanek wrote:
>> The 'retlen' points to a variable representing the number of data bytes
>> written/read (see include/linux/mtd/mtd.h) by the current invocation of
>> the function. This variable must be set, not incremented.
>>
>> v2: clearer commit message
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>> Acked-by: Marek Vasut <marex@denx.de>
>> ---
>>  drivers/mtd/devices/m25p80.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 7c8b169..0b2bc21 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>>
>>       spi_sync(spi, &m);
>>
>> -     *retlen += m.actual_length - cmd_sz;
>> +     *retlen = m.actual_length - cmd_sz;
>
> This is very wrong. It gets a little better once you add your next
> patches (which are also not good) since those patches reinterpret the
> usage of retlen, but this one definitely does *not* stand a lone.
>
> I'll admit the API is a little odd here, but the callers of this
> function (see spi_nor_write()) actually depend on calling this multiple
> times, with the value incrementing each time so we get a summary total.
> You're now clobbering this value.
>
> I'm willing to accept patches to improve this API, if you think that
> would help. Or to add comments that document this.

Yes, the only user of the retlen value ignores it but passes it on so
without the following patch this one makes the passed on value
different from before.

For m25p80 this would be fixed by truncating the write in the driver
and setting actual_length appropriately rather than returning an error
when the message is too long. It might possibly break other drivers,
though.

Thanks

Michal

[PATCH 2/3] MTD: spi-nor: Add Pm25LD020 and GD25Q41B chip ID.

[Michal Suchanek]

First chip reads Pm25LD020 or Pm25L0020. Found on some WD HDD PCB.
Identified as PMC Pm25LD020.
Flash read does not return consistent data which explains why the disk died.

Second chip reads something like 25Q41BT. Found on Esspif ESP8266 based
ESP-01 board. Identified as Elm Techchnology GD25Q41B.
Datasheet is not clear about block size so erasing blocks may not work
as expected.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 14a5d23..de73cf5 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -551,6 +551,7 @@ static const struct spi_device_id spi_nor_ids[] = {
 	{ "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },
 
 	/* GigaDevice */
+	{ "gd25q41b", INFO(0xc84013, 0, 64 * 1024,  8, SECT_4K) },
 	{ "gd25q32", INFO(0xc84016, 0, 64 * 1024,  64, SECT_4K) },
 	{ "gd25q64", INFO(0xc84017, 0, 64 * 1024, 128, SECT_4K) },
 	{ "gd25q128", INFO(0xc84018, 0, 64 * 1024, 256, SECT_4K) },
@@ -589,6 +590,7 @@ static const struct spi_device_id spi_nor_ids[] = {
 	/* PMC */
 	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
 	{ "pm25lv010",   INFO(0,        0, 32 * 1024,    4, SECT_4K_PMC) },
+	{ "pm25ld020",   INFO(0x7f9d22, 0, 64 * 1024,    4, SECT_4K) },
 	{ "pm25lq032",   INFO(0x7f9d46, 0, 64 * 1024,   64, SECT_4K) },
 
 	/* Spansion -- single (large) sector size only, at least
-- 
2.1.4

[PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Michal Suchanek]

The sector size of the flash memory is unclear from datasheet or may
possibly vary between chips so add a flag to always use 4k blocks.

Currently 4k blocks are always used when possible but in the future
somebody might want to do some optimizations with sector erase.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index de73cf5..371e91c 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -55,6 +55,7 @@ struct flash_info {
 #define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
 #define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
 #define	USE_FSR			0x80	/* use flag status register */
+#define	NO_SPINOR_OP_SE	       0x100	/* do not erase sectors */
 };
 
 #define JEDEC_MFR(info)	((info)->id[0])
@@ -333,6 +334,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	/* REVISIT in some cases we could speed up erasing large regions
 	 * by using SPINOR_OP_SE instead of SPINOR_OP_BE_4K.  We may have set up
 	 * to use "small sector erase", but that's not always optimal.
+	 * The block size for SPINOR_OP_SE is unknown/untested for some flashes.
 	 */
 
 	/* "sector"-at-a-time erase */
@@ -551,7 +553,7 @@ static const struct spi_device_id spi_nor_ids[] = {
 	{ "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },
 
 	/* GigaDevice */
-	{ "gd25q41b", INFO(0xc84013, 0, 64 * 1024,  8, SECT_4K) },
+	{ "gd25q41b", INFO(0xc84013, 0, 64 * 1024,  8, SECT_4K | NO_SPINOR_OP_SE) },
 	{ "gd25q32", INFO(0xc84016, 0, 64 * 1024,  64, SECT_4K) },
 	{ "gd25q64", INFO(0xc84017, 0, 64 * 1024, 128, SECT_4K) },
 	{ "gd25q128", INFO(0xc84018, 0, 64 * 1024, 256, SECT_4K) },
-- 
2.1.4

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Marek Vasut]

On Thursday, April 30, 2015 at 11:13:12 PM, Michal Suchanek wrote:
> The sector size of the flash memory is unclear from datasheet or may
> possibly vary between chips so add a flag to always use 4k blocks.
> 
> Currently 4k blocks are always used when possible but in the future
> somebody might want to do some optimizations with sector erase.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

I _think_ you might be able to determine the size, no ?

One way is to ask the vendor, but you can also try something like:
1) erase the whole SPI NOR
2) overwrite it with zeroes (or ones ? I think it should be all ones after 
erasing).
3) Erase sector 0
4) Read some 128 KiB back
5) Observe what is the difference.

Best regards,
Marek Vasut

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Marek Vasut]

On Thursday, April 30, 2015 at 11:13:12 PM, Michal Suchanek wrote:
> The sector size of the flash memory is unclear from datasheet or may
> possibly vary between chips so add a flag to always use 4k blocks.
> 
> Currently 4k blocks are always used when possible but in the future
> somebody might want to do some optimizations with sector erase.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

I _think_ you might be able to determine the size, no ?

One way is to ask the vendor, but you can also try something like:
1) erase the whole SPI NOR
2) overwrite it with zeroes (or ones ? I think it should be all ones after 
erasing).
3) Erase sector 0
4) Read some 128 KiB back
5) Observe what is the difference.

Best regards,
Marek Vasut

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Michal Suchanek]

On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
> On Thursday, April 30, 2015 at 11:13:12 PM, Michal Suchanek wrote:
>> The sector size of the flash memory is unclear from datasheet or may
>> possibly vary between chips so add a flag to always use 4k blocks.
>>
>> Currently 4k blocks are always used when possible but in the future
>> somebody might want to do some optimizations with sector erase.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>
> I _think_ you might be able to determine the size, no ?
>
> One way is to ask the vendor, but you can also try something like:
> 1) erase the whole SPI NOR
> 2) overwrite it with zeroes (or ones ? I think it should be all ones after
> erasing).
> 3) Erase sector 0
> 4) Read some 128 KiB back
> 5) Observe what is the difference.
>

I can determine it for this particular chip. However, when the vendor
datasheet says the block is 64/32K it might mean that chips with this
ID can have either block size.

It's a value that we don't use anyway so I just mark it as unknown
here for future reference.

Thanks

Michal

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Michal Suchanek]

On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
> On Thursday, April 30, 2015 at 11:13:12 PM, Michal Suchanek wrote:
>> The sector size of the flash memory is unclear from datasheet or may
>> possibly vary between chips so add a flag to always use 4k blocks.
>>
>> Currently 4k blocks are always used when possible but in the future
>> somebody might want to do some optimizations with sector erase.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>
> I _think_ you might be able to determine the size, no ?
>
> One way is to ask the vendor, but you can also try something like:
> 1) erase the whole SPI NOR
> 2) overwrite it with zeroes (or ones ? I think it should be all ones after
> erasing).
> 3) Erase sector 0
> 4) Read some 128 KiB back
> 5) Observe what is the difference.
>

I can determine it for this particular chip. However, when the vendor
datasheet says the block is 64/32K it might mean that chips with this
ID can have either block size.

It's a value that we don't use anyway so I just mark it as unknown
here for future reference.

Thanks

Michal

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Jonas Gorski]

Hi,

On Fri, May 1, 2015 at 9:05 AM, Michal Suchanek <hramrach@gmail.com> wrote:
> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
>> On Thursday, April 30, 2015 at 11:13:12 PM, Michal Suchanek wrote:
>>> The sector size of the flash memory is unclear from datasheet or may
>>> possibly vary between chips so add a flag to always use 4k blocks.
>>>
>>> Currently 4k blocks are always used when possible but in the future
>>> somebody might want to do some optimizations with sector erase.
>>>
>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>
>> I _think_ you might be able to determine the size, no ?
>>
>> One way is to ask the vendor, but you can also try something like:
>> 1) erase the whole SPI NOR
>> 2) overwrite it with zeroes (or ones ? I think it should be all ones after
>> erasing).
>> 3) Erase sector 0
>> 4) Read some 128 KiB back
>> 5) Observe what is the difference.
>>
>
> I can determine it for this particular chip. However, when the vendor
> datasheet says the block is 64/32K it might mean that chips with this
> ID can have either block size.
>
> It's a value that we don't use anyway so I just mark it as unknown
> here for future reference.

It will be used if MTD_SPI_NOR_USE_4K_SECTORS is unset, so you should
add some code to properly handle that case.

Also I'd suggest switching the order of 2 and 3, so you add the flag
handling first and then add support for a flash chip with this issue.


Regards
Jonas

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Jonas Gorski]

Hi,

On Fri, May 1, 2015 at 9:05 AM, Michal Suchanek <hramrach@gmail.com> wrote:
> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
>> On Thursday, April 30, 2015 at 11:13:12 PM, Michal Suchanek wrote:
>>> The sector size of the flash memory is unclear from datasheet or may
>>> possibly vary between chips so add a flag to always use 4k blocks.
>>>
>>> Currently 4k blocks are always used when possible but in the future
>>> somebody might want to do some optimizations with sector erase.
>>>
>>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>
>> I _think_ you might be able to determine the size, no ?
>>
>> One way is to ask the vendor, but you can also try something like:
>> 1) erase the whole SPI NOR
>> 2) overwrite it with zeroes (or ones ? I think it should be all ones after
>> erasing).
>> 3) Erase sector 0
>> 4) Read some 128 KiB back
>> 5) Observe what is the difference.
>>
>
> I can determine it for this particular chip. However, when the vendor
> datasheet says the block is 64/32K it might mean that chips with this
> ID can have either block size.
>
> It's a value that we don't use anyway so I just mark it as unknown
> here for future reference.

It will be used if MTD_SPI_NOR_USE_4K_SECTORS is unset, so you should
add some code to properly handle that case.

Also I'd suggest switching the order of 2 and 3, so you add the flag
handling first and then add support for a flash chip with this issue.


Regards
Jonas

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Marek Vasut]

On Friday, May 01, 2015 at 09:05:15 AM, Michal Suchanek wrote:
> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, April 30, 2015 at 11:13:12 PM, Michal Suchanek wrote:
> >> The sector size of the flash memory is unclear from datasheet or may
> >> possibly vary between chips so add a flag to always use 4k blocks.
> >> 
> >> Currently 4k blocks are always used when possible but in the future
> >> somebody might want to do some optimizations with sector erase.
> >> 
> >> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> > 
> > I _think_ you might be able to determine the size, no ?
> > 
> > One way is to ask the vendor, but you can also try something like:
> > 1) erase the whole SPI NOR
> > 2) overwrite it with zeroes (or ones ? I think it should be all ones
> > after erasing).
> > 3) Erase sector 0
> > 4) Read some 128 KiB back
> > 5) Observe what is the difference.
> 
> I can determine it for this particular chip. However, when the vendor
> datasheet says the block is 64/32K it might mean that chips with this
> ID can have either block size.

http://www.chingistek.com/img/Product_Files/Pm25LD010020datasheet%20v04.pdf
page 21:

SECTOR_ER (20h) erases 4kByte sector.
BLOCK_ER (d8h) erases 64kByte sector.

http://www.gigadevice.com/product/download/366.html?locale=en_US
page 27-28:

Sector Erase (SE) (20h) erases 4kByte sector
64KB Block Erase (BE) (d8h) erases 64kByte sector

> It's a value that we don't use anyway so I just mark it as unknown
> here for future reference.

Looks like standard SPI NOR opcodes [1], nothing unknown there ;-)

[1] include/linux/mtd/spi-nor.h

Best regards,
Marek Vasut

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Marek Vasut]

On Friday, May 01, 2015 at 09:05:15 AM, Michal Suchanek wrote:
> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, April 30, 2015 at 11:13:12 PM, Michal Suchanek wrote:
> >> The sector size of the flash memory is unclear from datasheet or may
> >> possibly vary between chips so add a flag to always use 4k blocks.
> >> 
> >> Currently 4k blocks are always used when possible but in the future
> >> somebody might want to do some optimizations with sector erase.
> >> 
> >> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> > 
> > I _think_ you might be able to determine the size, no ?
> > 
> > One way is to ask the vendor, but you can also try something like:
> > 1) erase the whole SPI NOR
> > 2) overwrite it with zeroes (or ones ? I think it should be all ones
> > after erasing).
> > 3) Erase sector 0
> > 4) Read some 128 KiB back
> > 5) Observe what is the difference.
> 
> I can determine it for this particular chip. However, when the vendor
> datasheet says the block is 64/32K it might mean that chips with this
> ID can have either block size.

http://www.chingistek.com/img/Product_Files/Pm25LD010020datasheet%20v04.pdf
page 21:

SECTOR_ER (20h) erases 4kByte sector.
BLOCK_ER (d8h) erases 64kByte sector.

http://www.gigadevice.com/product/download/366.html?locale=en_US
page 27-28:

Sector Erase (SE) (20h) erases 4kByte sector
64KB Block Erase (BE) (d8h) erases 64kByte sector

> It's a value that we don't use anyway so I just mark it as unknown
> here for future reference.

Looks like standard SPI NOR opcodes [1], nothing unknown there ;-)

[1] include/linux/mtd/spi-nor.h

Best regards,
Marek Vasut

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Michal Suchanek]

Hello,

On 1 May 2015 at 16:20, Marek Vasut <marex@denx.de> wrote:
> On Friday, May 01, 2015 at 09:05:15 AM, Michal Suchanek wrote:
>> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
>> I can determine it for this particular chip. However, when the vendor
>> datasheet says the block is 64/32K it might mean that chips with this
>> ID can have either block size.
>
> http://www.chingistek.com/img/Product_Files/Pm25LD010020datasheet%20v04.pdf
> page 21:
>
> SECTOR_ER (20h) erases 4kByte sector.
> BLOCK_ER (d8h) erases 64kByte sector.
>
> http://www.gigadevice.com/product/download/366.html?locale=en_US
> page 27-28:
>
> Sector Erase (SE) (20h) erases 4kByte sector
> 64KB Block Erase (BE) (d8h) erases 64kByte sector
>

It's pretty much the same as the datasheet I used
http://www.elm-tech.com/en/products/spi-flash-memory/gd25q41/gd25q41.pdf

It mentions both
32KB Block Erase (BE) (52H)
and
64KB Block Erase (BE) (D8H)

So the chip probably tries its best to be compatible with any command
set and this last patch is not needed. The memory organization table
on page 7 is not all that reassuring, though.

Thanks

Michal

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Michal Suchanek]

Hello,

On 1 May 2015 at 16:20, Marek Vasut <marex@denx.de> wrote:
> On Friday, May 01, 2015 at 09:05:15 AM, Michal Suchanek wrote:
>> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
>> I can determine it for this particular chip. However, when the vendor
>> datasheet says the block is 64/32K it might mean that chips with this
>> ID can have either block size.
>
> http://www.chingistek.com/img/Product_Files/Pm25LD010020datasheet%20v04.pdf
> page 21:
>
> SECTOR_ER (20h) erases 4kByte sector.
> BLOCK_ER (d8h) erases 64kByte sector.
>
> http://www.gigadevice.com/product/download/366.html?locale=en_US
> page 27-28:
>
> Sector Erase (SE) (20h) erases 4kByte sector
> 64KB Block Erase (BE) (d8h) erases 64kByte sector
>

It's pretty much the same as the datasheet I used
http://www.elm-tech.com/en/products/spi-flash-memory/gd25q41/gd25q41.pdf

It mentions both
32KB Block Erase (BE) (52H)
and
64KB Block Erase (BE) (D8H)

So the chip probably tries its best to be compatible with any command
set and this last patch is not needed. The memory organization table
on page 7 is not all that reassuring, though.

Thanks

Michal

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Marek Vasut]

On Monday, May 04, 2015 at 01:11:03 PM, Michal Suchanek wrote:
> Hello,

Hi!

> On 1 May 2015 at 16:20, Marek Vasut <marex@denx.de> wrote:
> > On Friday, May 01, 2015 at 09:05:15 AM, Michal Suchanek wrote:
> >> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
> >> I can determine it for this particular chip. However, when the vendor
> >> datasheet says the block is 64/32K it might mean that chips with this
> >> ID can have either block size.
> > 
> > http://www.chingistek.com/img/Product_Files/Pm25LD010020datasheet%20v04.p
> > df page 21:
> > 
> > SECTOR_ER (20h) erases 4kByte sector.
> > BLOCK_ER (d8h) erases 64kByte sector.
> > 
> > http://www.gigadevice.com/product/download/366.html?locale=en_US
> > page 27-28:
> > 
> > Sector Erase (SE) (20h) erases 4kByte sector
> > 64KB Block Erase (BE) (d8h) erases 64kByte sector
> 
> It's pretty much the same as the datasheet I used
> http://www.elm-tech.com/en/products/spi-flash-memory/gd25q41/gd25q41.pdf
> 
> It mentions both
> 32KB Block Erase (BE) (52H)
> and
> 64KB Block Erase (BE) (D8H)

The SPI NOR framework will use 0xbe opcode, no problem.

> So the chip probably tries its best to be compatible with any command
> set and this last patch is not needed. The memory organization table
> on page 7 is not all that reassuring, though.

Which exact part do you refer to please ?

Best regards,
Marek Vasut

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Marek Vasut]

On Monday, May 04, 2015 at 01:11:03 PM, Michal Suchanek wrote:
> Hello,

Hi!

> On 1 May 2015 at 16:20, Marek Vasut <marex@denx.de> wrote:
> > On Friday, May 01, 2015 at 09:05:15 AM, Michal Suchanek wrote:
> >> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
> >> I can determine it for this particular chip. However, when the vendor
> >> datasheet says the block is 64/32K it might mean that chips with this
> >> ID can have either block size.
> > 
> > http://www.chingistek.com/img/Product_Files/Pm25LD010020datasheet%20v04.p
> > df page 21:
> > 
> > SECTOR_ER (20h) erases 4kByte sector.
> > BLOCK_ER (d8h) erases 64kByte sector.
> > 
> > http://www.gigadevice.com/product/download/366.html?locale=en_US
> > page 27-28:
> > 
> > Sector Erase (SE) (20h) erases 4kByte sector
> > 64KB Block Erase (BE) (d8h) erases 64kByte sector
> 
> It's pretty much the same as the datasheet I used
> http://www.elm-tech.com/en/products/spi-flash-memory/gd25q41/gd25q41.pdf
> 
> It mentions both
> 32KB Block Erase (BE) (52H)
> and
> 64KB Block Erase (BE) (D8H)

The SPI NOR framework will use 0xbe opcode, no problem.

> So the chip probably tries its best to be compatible with any command
> set and this last patch is not needed. The memory organization table
> on page 7 is not all that reassuring, though.

Which exact part do you refer to please ?

Best regards,
Marek Vasut

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Michal Suchanek]

On 4 May 2015 at 14:12, Marek Vasut <marex@denx.de> wrote:
> On Monday, May 04, 2015 at 01:11:03 PM, Michal Suchanek wrote:
>> Hello,
>
> Hi!
>
>> On 1 May 2015 at 16:20, Marek Vasut <marex@denx.de> wrote:
>> > On Friday, May 01, 2015 at 09:05:15 AM, Michal Suchanek wrote:
>> >> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
>> >> I can determine it for this particular chip. However, when the vendor
>> >> datasheet says the block is 64/32K it might mean that chips with this
>> >> ID can have either block size.
>> >
>> > http://www.chingistek.com/img/Product_Files/Pm25LD010020datasheet%20v04.p
>> > df page 21:
>> >
>> > SECTOR_ER (20h) erases 4kByte sector.
>> > BLOCK_ER (d8h) erases 64kByte sector.
>> >
>> > http://www.gigadevice.com/product/download/366.html?locale=en_US
>> > page 27-28:
>> >
>> > Sector Erase (SE) (20h) erases 4kByte sector
>> > 64KB Block Erase (BE) (d8h) erases 64kByte sector
>>
>> It's pretty much the same as the datasheet I used
>> http://www.elm-tech.com/en/products/spi-flash-memory/gd25q41/gd25q41.pdf
>>
>> It mentions both
>> 32KB Block Erase (BE) (52H)
>> and
>> 64KB Block Erase (BE) (D8H)
>
> The SPI NOR framework will use 0xbe opcode, no problem.
>
>> So the chip probably tries its best to be compatible with any command
>> set and this last patch is not needed. The memory organization table
>> on page 7 is not all that reassuring, though.
>
> Which exact part do you refer to please ?

Start of page 7 where it says sector size 32/64K in either datasheet.

It can refer to both BE opcode variants being supported but it's quite unclear.

Write protection seems to be calculated in 4k sectors and not blocks
so the block size does not seem very relevant.

Thanks

Michal

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Michal Suchanek]

On 4 May 2015 at 14:12, Marek Vasut <marex@denx.de> wrote:
> On Monday, May 04, 2015 at 01:11:03 PM, Michal Suchanek wrote:
>> Hello,
>
> Hi!
>
>> On 1 May 2015 at 16:20, Marek Vasut <marex@denx.de> wrote:
>> > On Friday, May 01, 2015 at 09:05:15 AM, Michal Suchanek wrote:
>> >> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
>> >> I can determine it for this particular chip. However, when the vendor
>> >> datasheet says the block is 64/32K it might mean that chips with this
>> >> ID can have either block size.
>> >
>> > http://www.chingistek.com/img/Product_Files/Pm25LD010020datasheet%20v04.p
>> > df page 21:
>> >
>> > SECTOR_ER (20h) erases 4kByte sector.
>> > BLOCK_ER (d8h) erases 64kByte sector.
>> >
>> > http://www.gigadevice.com/product/download/366.html?locale=en_US
>> > page 27-28:
>> >
>> > Sector Erase (SE) (20h) erases 4kByte sector
>> > 64KB Block Erase (BE) (d8h) erases 64kByte sector
>>
>> It's pretty much the same as the datasheet I used
>> http://www.elm-tech.com/en/products/spi-flash-memory/gd25q41/gd25q41.pdf
>>
>> It mentions both
>> 32KB Block Erase (BE) (52H)
>> and
>> 64KB Block Erase (BE) (D8H)
>
> The SPI NOR framework will use 0xbe opcode, no problem.
>
>> So the chip probably tries its best to be compatible with any command
>> set and this last patch is not needed. The memory organization table
>> on page 7 is not all that reassuring, though.
>
> Which exact part do you refer to please ?

Start of page 7 where it says sector size 32/64K in either datasheet.

It can refer to both BE opcode variants being supported but it's quite unclear.

Write protection seems to be calculated in 4k sectors and not blocks
so the block size does not seem very relevant.

Thanks

Michal

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Marek Vasut]

On Monday, May 04, 2015 at 03:18:56 PM, Michal Suchanek wrote:
> On 4 May 2015 at 14:12, Marek Vasut <marex@denx.de> wrote:
> > On Monday, May 04, 2015 at 01:11:03 PM, Michal Suchanek wrote:
> >> Hello,
> > 
> > Hi!
> > 
> >> On 1 May 2015 at 16:20, Marek Vasut <marex@denx.de> wrote:
> >> > On Friday, May 01, 2015 at 09:05:15 AM, Michal Suchanek wrote:
> >> >> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
> >> >> I can determine it for this particular chip. However, when the vendor
> >> >> datasheet says the block is 64/32K it might mean that chips with this
> >> >> ID can have either block size.
> >> > 
> >> > http://www.chingistek.com/img/Product_Files/Pm25LD010020datasheet%20v0
> >> > 4.p df page 21:
> >> > 
> >> > SECTOR_ER (20h) erases 4kByte sector.
> >> > BLOCK_ER (d8h) erases 64kByte sector.
> >> > 
> >> > http://www.gigadevice.com/product/download/366.html?locale=en_US
> >> > page 27-28:
> >> > 
> >> > Sector Erase (SE) (20h) erases 4kByte sector
> >> > 64KB Block Erase (BE) (d8h) erases 64kByte sector
> >> 
> >> It's pretty much the same as the datasheet I used
> >> http://www.elm-tech.com/en/products/spi-flash-memory/gd25q41/gd25q41.pdf
> >> 
> >> It mentions both
> >> 32KB Block Erase (BE) (52H)
> >> and
> >> 64KB Block Erase (BE) (D8H)
> > 
> > The SPI NOR framework will use 0xbe opcode, no problem.
> > 
> >> So the chip probably tries its best to be compatible with any command
> >> set and this last patch is not needed. The memory organization table
> >> on page 7 is not all that reassuring, though.
> > 
> > Which exact part do you refer to please ?
> 
> Start of page 7 where it says sector size 32/64K in either datasheet.
> 
> It can refer to both BE opcode variants being supported but it's quite
> unclear.

My guess here would be that the internal organisation of the SPI NOR is
in 4k blocks, which is no surprise really. My understanding is that opcode
0x52 erases 8x4k sector (ie. 32k of data) while 0xd8 erases 16x4k sector
(ie. 64k of data). I don't see any problem here -- there are two different 
opcodes which do two different things and their behavior matches the one on 
various other SPI NORs.

> Write protection seems to be calculated in 4k sectors and not blocks
> so the block size does not seem very relevant.

See above. Does it make sense now please ?

Best regards,
Marek Vasut

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Marek Vasut]

On Monday, May 04, 2015 at 03:18:56 PM, Michal Suchanek wrote:
> On 4 May 2015 at 14:12, Marek Vasut <marex@denx.de> wrote:
> > On Monday, May 04, 2015 at 01:11:03 PM, Michal Suchanek wrote:
> >> Hello,
> > 
> > Hi!
> > 
> >> On 1 May 2015 at 16:20, Marek Vasut <marex@denx.de> wrote:
> >> > On Friday, May 01, 2015 at 09:05:15 AM, Michal Suchanek wrote:
> >> >> On 1 May 2015 at 01:13, Marek Vasut <marex@denx.de> wrote:
> >> >> I can determine it for this particular chip. However, when the vendor
> >> >> datasheet says the block is 64/32K it might mean that chips with this
> >> >> ID can have either block size.
> >> > 
> >> > http://www.chingistek.com/img/Product_Files/Pm25LD010020datasheet%20v0
> >> > 4.p df page 21:
> >> > 
> >> > SECTOR_ER (20h) erases 4kByte sector.
> >> > BLOCK_ER (d8h) erases 64kByte sector.
> >> > 
> >> > http://www.gigadevice.com/product/download/366.html?locale=en_US
> >> > page 27-28:
> >> > 
> >> > Sector Erase (SE) (20h) erases 4kByte sector
> >> > 64KB Block Erase (BE) (d8h) erases 64kByte sector
> >> 
> >> It's pretty much the same as the datasheet I used
> >> http://www.elm-tech.com/en/products/spi-flash-memory/gd25q41/gd25q41.pdf
> >> 
> >> It mentions both
> >> 32KB Block Erase (BE) (52H)
> >> and
> >> 64KB Block Erase (BE) (D8H)
> > 
> > The SPI NOR framework will use 0xbe opcode, no problem.
> > 
> >> So the chip probably tries its best to be compatible with any command
> >> set and this last patch is not needed. The memory organization table
> >> on page 7 is not all that reassuring, though.
> > 
> > Which exact part do you refer to please ?
> 
> Start of page 7 where it says sector size 32/64K in either datasheet.
> 
> It can refer to both BE opcode variants being supported but it's quite
> unclear.

My guess here would be that the internal organisation of the SPI NOR is
in 4k blocks, which is no surprise really. My understanding is that opcode
0x52 erases 8x4k sector (ie. 32k of data) while 0xd8 erases 16x4k sector
(ie. 64k of data). I don't see any problem here -- there are two different 
opcodes which do two different things and their behavior matches the one on 
various other SPI NORs.

> Write protection seems to be calculated in 4k sectors and not blocks
> so the block size does not seem very relevant.

See above. Does it make sense now please ?

Best regards,
Marek Vasut

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Michal Suchanek]

On 4 May 2015 at 15:35, Marek Vasut <marex@denx.de> wrote:
> On Monday, May 04, 2015 at 03:18:56 PM, Michal Suchanek wrote:
>> On 4 May 2015 at 14:12, Marek Vasut <marex@denx.de> wrote:
>> > On Monday, May 04, 2015 at 01:11:03 PM, Michal Suchanek wrote:
>> >>
>> >> It mentions both
>> >> 32KB Block Erase (BE) (52H)
>> >> and
>> >> 64KB Block Erase (BE) (D8H)
>> >
>> > The SPI NOR framework will use 0xbe opcode, no problem.
>> >
>> >> So the chip probably tries its best to be compatible with any command
>> >> set and this last patch is not needed. The memory organization table
>> >> on page 7 is not all that reassuring, though.
>> >
>> > Which exact part do you refer to please ?
>>
>> Start of page 7 where it says sector size 32/64K in either datasheet.
>>
>> It can refer to both BE opcode variants being supported but it's quite
>> unclear.
>
> My guess here would be that the internal organisation of the SPI NOR is
> in 4k blocks, which is no surprise really. My understanding is that opcode
> 0x52 erases 8x4k sector (ie. 32k of data) while 0xd8 erases 16x4k sector
> (ie. 64k of data). I don't see any problem here -- there are two different
> opcodes which do two different things and their behavior matches the one on
> various other SPI NORs.
>
>> Write protection seems to be calculated in 4k sectors and not blocks
>> so the block size does not seem very relevant.
>
> See above. Does it make sense now please ?
>

Yes,

makes sense.

Thanks

Michal

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Michal Suchanek]

On 4 May 2015 at 15:35, Marek Vasut <marex@denx.de> wrote:
> On Monday, May 04, 2015 at 03:18:56 PM, Michal Suchanek wrote:
>> On 4 May 2015 at 14:12, Marek Vasut <marex@denx.de> wrote:
>> > On Monday, May 04, 2015 at 01:11:03 PM, Michal Suchanek wrote:
>> >>
>> >> It mentions both
>> >> 32KB Block Erase (BE) (52H)
>> >> and
>> >> 64KB Block Erase (BE) (D8H)
>> >
>> > The SPI NOR framework will use 0xbe opcode, no problem.
>> >
>> >> So the chip probably tries its best to be compatible with any command
>> >> set and this last patch is not needed. The memory organization table
>> >> on page 7 is not all that reassuring, though.
>> >
>> > Which exact part do you refer to please ?
>>
>> Start of page 7 where it says sector size 32/64K in either datasheet.
>>
>> It can refer to both BE opcode variants being supported but it's quite
>> unclear.
>
> My guess here would be that the internal organisation of the SPI NOR is
> in 4k blocks, which is no surprise really. My understanding is that opcode
> 0x52 erases 8x4k sector (ie. 32k of data) while 0xd8 erases 16x4k sector
> (ie. 64k of data). I don't see any problem here -- there are two different
> opcodes which do two different things and their behavior matches the one on
> various other SPI NORs.
>
>> Write protection seems to be calculated in 4k sectors and not blocks
>> so the block size does not seem very relevant.
>
> See above. Does it make sense now please ?
>

Yes,

makes sense.

Thanks

Michal

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Marek Vasut]

On Monday, May 04, 2015 at 03:39:44 PM, Michal Suchanek wrote:
> On 4 May 2015 at 15:35, Marek Vasut <marex@denx.de> wrote:
> > On Monday, May 04, 2015 at 03:18:56 PM, Michal Suchanek wrote:
> >> On 4 May 2015 at 14:12, Marek Vasut <marex@denx.de> wrote:
> >> > On Monday, May 04, 2015 at 01:11:03 PM, Michal Suchanek wrote:
> >> >> It mentions both
> >> >> 32KB Block Erase (BE) (52H)
> >> >> and
> >> >> 64KB Block Erase (BE) (D8H)
> >> > 
> >> > The SPI NOR framework will use 0xbe opcode, no problem.
> >> > 
> >> >> So the chip probably tries its best to be compatible with any command
> >> >> set and this last patch is not needed. The memory organization table
> >> >> on page 7 is not all that reassuring, though.
> >> > 
> >> > Which exact part do you refer to please ?
> >> 
> >> Start of page 7 where it says sector size 32/64K in either datasheet.
> >> 
> >> It can refer to both BE opcode variants being supported but it's quite
> >> unclear.
> > 
> > My guess here would be that the internal organisation of the SPI NOR is
> > in 4k blocks, which is no surprise really. My understanding is that
> > opcode 0x52 erases 8x4k sector (ie. 32k of data) while 0xd8 erases 16x4k
> > sector (ie. 64k of data). I don't see any problem here -- there are two
> > different opcodes which do two different things and their behavior
> > matches the one on various other SPI NORs.
> > 
> >> Write protection seems to be calculated in 4k sectors and not blocks
> >> so the block size does not seem very relevant.
> > 
> > See above. Does it make sense now please ?
> 
> Yes,
> 
> makes sense.

I'm glad to hear this got cleared up, thanks ! :)

Best regards,
Marek Vasut

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Marek Vasut]

On Monday, May 04, 2015 at 03:39:44 PM, Michal Suchanek wrote:
> On 4 May 2015 at 15:35, Marek Vasut <marex@denx.de> wrote:
> > On Monday, May 04, 2015 at 03:18:56 PM, Michal Suchanek wrote:
> >> On 4 May 2015 at 14:12, Marek Vasut <marex@denx.de> wrote:
> >> > On Monday, May 04, 2015 at 01:11:03 PM, Michal Suchanek wrote:
> >> >> It mentions both
> >> >> 32KB Block Erase (BE) (52H)
> >> >> and
> >> >> 64KB Block Erase (BE) (D8H)
> >> > 
> >> > The SPI NOR framework will use 0xbe opcode, no problem.
> >> > 
> >> >> So the chip probably tries its best to be compatible with any command
> >> >> set and this last patch is not needed. The memory organization table
> >> >> on page 7 is not all that reassuring, though.
> >> > 
> >> > Which exact part do you refer to please ?
> >> 
> >> Start of page 7 where it says sector size 32/64K in either datasheet.
> >> 
> >> It can refer to both BE opcode variants being supported but it's quite
> >> unclear.
> > 
> > My guess here would be that the internal organisation of the SPI NOR is
> > in 4k blocks, which is no surprise really. My understanding is that
> > opcode 0x52 erases 8x4k sector (ie. 32k of data) while 0xd8 erases 16x4k
> > sector (ie. 64k of data). I don't see any problem here -- there are two
> > different opcodes which do two different things and their behavior
> > matches the one on various other SPI NORs.
> > 
> >> Write protection seems to be calculated in 4k sectors and not blocks
> >> so the block size does not seem very relevant.
> > 
> > See above. Does it make sense now please ?
> 
> Yes,
> 
> makes sense.

I'm glad to hear this got cleared up, thanks ! :)

Best regards,
Marek Vasut

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Rafał Miłecki]

On 30 April 2015 at 23:13, Michal Suchanek <hramrach@gmail.com> wrote:
> The sector size of the flash memory is unclear from datasheet or may
> possibly vary between chips so add a flag to always use 4k blocks.
>
> Currently 4k blocks are always used when possible but in the future
> somebody might want to do some optimizations with sector erase.

AFAIK most sources call 4K a sector and 64K a block. I think we should
try to stick to that (even if some current code doesn't). So it would
be nice to call 4K a sector (instead of block).

Re: [PATCH 3/3] MTD: spi-nor: add flag to not use sector erase.

[Rafał Miłecki]

On 30 April 2015 at 23:13, Michal Suchanek <hramrach@gmail.com> wrote:
> The sector size of the flash memory is unclear from datasheet or may
> possibly vary between chips so add a flag to always use 4k blocks.
>
> Currently 4k blocks are always used when possible but in the future
> somebody might want to do some optimizations with sector erase.

AFAIK most sources call 4K a sector and 64K a block. I think we should
try to stick to that (even if some current code doesn't). So it would
be nice to call 4K a sector (instead of block).

[PATCH 0/3] Using SPI NOR flah on sunxi.

[Michal Suchanek]

Hello,

I tried to connect a SPI NOR flash to my sunxi board and due to the current
sunxi SPI driver limitations it does not work.

The SPI driver returns an error when more than 64 bytes are transferred at once
due to lack of DMA support.

I tried to hack in an option to limit the transfer size and discovered that the
return value of write in m25p80 driver is wrong and never checked.

This might be useful while working with other devices with limited driver support.

Thanks

Michal

Michal Suchanek (3):
  MTD: m25p80: fix write return value.
  MTD: spi-nor: check for short writes in spi_nor_write.
  MTD: m25p80: Add option to limit SPI transfer size.

 Documentation/devicetree/bindings/mtd/m25p80.txt |  5 +++
 drivers/mtd/devices/m25p80.c                     | 21 ++++++++++--
 drivers/mtd/spi-nor/spi-nor.c                    | 42 ++++++++++--------------
 3 files changed, 40 insertions(+), 28 deletions(-)

-- 
2.1.4

[PATCH 1/3] MTD: m25p80: fix write return value.

[Michal Suchanek]

The size of written data was added to user supplied value rather than
written at the provided address.

Signed-off-by: Michal Suchanek <hramrach@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7c8b169..0b2bc21 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
 
 	spi_sync(spi, &m);
 
-	*retlen += m.actual_length - cmd_sz;
+	*retlen = m.actual_length - cmd_sz;
 }
 
 static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
-- 
2.1.4

[PATCH 1/3] MTD: m25p80: fix write return value.

[Michal Suchanek]

The size of written data was added to user supplied value rather than
written at the provided address.

Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/devices/m25p80.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7c8b169..0b2bc21 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
 
 	spi_sync(spi, &m);
 
-	*retlen += m.actual_length - cmd_sz;
+	*retlen = m.actual_length - cmd_sz;
 }
 
 static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
-- 
2.1.4

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Marek Vasut]

On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote:
> The size of written data was added to user supplied value rather than
> written at the provided address.

You might want to work on the commit message a little, something like
the following, but feel free to reword as seen fit.

The 'retlen' points to a variable representing the number of data bytes 
written/read (see include/linux/mtd/mtd.h) by the current invocation of
the function. This variable must be set, not incremented.

Otherwise, the patch is correct I believe:

Acked-by: Marek Vasut <marex@denx.de>

> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7c8b169..0b2bc21 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t
> to, size_t len,
> 
>  	spi_sync(spi, &m);
> 
> -	*retlen += m.actual_length - cmd_sz;
> +	*retlen = m.actual_length - cmd_sz;
>  }
> 
>  static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

Best regards,
Marek Vasut

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Marek Vasut]

On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote:
> The size of written data was added to user supplied value rather than
> written at the provided address.

You might want to work on the commit message a little, something like
the following, but feel free to reword as seen fit.

The 'retlen' points to a variable representing the number of data bytes 
written/read (see include/linux/mtd/mtd.h) by the current invocation of
the function. This variable must be set, not incremented.

Otherwise, the patch is correct I believe:

Acked-by: Marek Vasut <marex@denx.de>

> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7c8b169..0b2bc21 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t
> to, size_t len,
> 
>  	spi_sync(spi, &m);
> 
> -	*retlen += m.actual_length - cmd_sz;
> +	*retlen = m.actual_length - cmd_sz;
>  }
> 
>  static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

Best regards,
Marek Vasut

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Marek Vasut]

On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote:
> The size of written data was added to user supplied value rather than
> written at the provided address.

You might want to work on the commit message a little, something like
the following, but feel free to reword as seen fit.

The 'retlen' points to a variable representing the number of data bytes 
written/read (see include/linux/mtd/mtd.h) by the current invocation of
the function. This variable must be set, not incremented.

Otherwise, the patch is correct I believe:

Acked-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/mtd/devices/m25p80.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7c8b169..0b2bc21 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t
> to, size_t len,
> 
>  	spi_sync(spi, &m);
> 
> -	*retlen += m.actual_length - cmd_sz;
> +	*retlen = m.actual_length - cmd_sz;
>  }
> 
>  static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

Best regards,
Marek Vasut

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Michal Suchanek]

On 30 April 2015 at 20:43, Marek Vasut <marex@denx.de> wrote:
> On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote:
>> The size of written data was added to user supplied value rather than
>> written at the provided address.
>
> You might want to work on the commit message a little, something like
> the following, but feel free to reword as seen fit.
>
> The 'retlen' points to a variable representing the number of data bytes
> written/read (see include/linux/mtd/mtd.h) by the current invocation of
> the function. This variable must be set, not incremented.
>
> Otherwise, the patch is correct I believe:
>
> Acked-by: Marek Vasut <marex@denx.de>
>

ok, I will send an updated version.

Thanks

Michal

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Michal Suchanek]

On 30 April 2015 at 20:43, Marek Vasut <marex@denx.de> wrote:
> On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote:
>> The size of written data was added to user supplied value rather than
>> written at the provided address.
>
> You might want to work on the commit message a little, something like
> the following, but feel free to reword as seen fit.
>
> The 'retlen' points to a variable representing the number of data bytes
> written/read (see include/linux/mtd/mtd.h) by the current invocation of
> the function. This variable must be set, not incremented.
>
> Otherwise, the patch is correct I believe:
>
> Acked-by: Marek Vasut <marex@denx.de>
>

ok, I will send an updated version.

Thanks

Michal

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Michal Suchanek]

On 30 April 2015 at 20:43, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote:
>> The size of written data was added to user supplied value rather than
>> written at the provided address.
>
> You might want to work on the commit message a little, something like
> the following, but feel free to reword as seen fit.
>
> The 'retlen' points to a variable representing the number of data bytes
> written/read (see include/linux/mtd/mtd.h) by the current invocation of
> the function. This variable must be set, not incremented.
>
> Otherwise, the patch is correct I believe:
>
> Acked-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>

ok, I will send an updated version.

Thanks

Michal