[2/2] mtd: spi-nor: fix mr25h* definitions

Submitted by Bastian Stender on Jan. 16, 2017, 3:12 p.m.

Details

Message ID 20170116151214.11195-2-bst@pengutronix.de
State New
Delegated to: Cyrille Pitchen
Headers show

Commit Message

Bastian Stender Jan. 16, 2017, 3:12 p.m.
The previous definitions for mr25h* chips lead to unnecessary limits
regarding writesize and eraseblocksize for MRAM. MRAM is random access
memory rather than page, sector, or block organized memory.

Instead of 1 sector with 512KB the mr25h40 MRAM chip should be defined
as 512K sectors with 1 byte each. This makes it writesize and
eraseblocksize aligned and reflects MRAM functionality properly.

Signed-off-by: Bastian Stender <bst@pengutronix.de>
---
 drivers/mtd/spi-nor/spi-nor.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Marek Vasut Jan. 16, 2017, 4:07 p.m.
On 01/16/2017 04:12 PM, Bastian Stender wrote:
> The previous definitions for mr25h* chips lead to unnecessary limits
> regarding writesize and eraseblocksize for MRAM. MRAM is random access
> memory rather than page, sector, or block organized memory.
> 
> Instead of 1 sector with 512KB the mr25h40 MRAM chip should be defined
> as 512K sectors with 1 byte each. This makes it writesize and
> eraseblocksize aligned and reflects MRAM functionality properly.

Wouldn't this degrade the performance ? ie. if you write the flash, it
will send PP opcode + 1 byte in a loop, right ?

> Signed-off-by: Bastian Stender <bst@pengutronix.de>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index c8399522c69d..6a39b18baa70 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -824,9 +824,9 @@ static const struct flash_info spi_nor_ids[] = {
>  	{ "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, SECT_4K) },
>  
>  	/* Everspin */
> -	{ "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> -	{ "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> -	{ "mr25h40",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> +	{ "mr25h256", CAT25_INFO(1,  32 * 1024, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> +	{ "mr25h10",  CAT25_INFO(1, 128 * 1024, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
> +	{ "mr25h40",  CAT25_INFO(1, 512 * 1024, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>  
>  	/* Fujitsu */
>  	{ "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },
>
Bastian Stender Jan. 17, 2017, 2:51 p.m.
Hi Marek,

On 01/16/2017 05:07 PM, Marek Vasut wrote:
> On 01/16/2017 04:12 PM, Bastian Stender wrote:
>> The previous definitions for mr25h* chips lead to unnecessary limits
>> regarding writesize and eraseblocksize for MRAM. MRAM is random access
>> memory rather than page, sector, or block organized memory.
>>
>> Instead of 1 sector with 512KB the mr25h40 MRAM chip should be defined
>> as 512K sectors with 1 byte each. This makes it writesize and
>> eraseblocksize aligned and reflects MRAM functionality properly.
>
> Wouldn't this degrade the performance ? ie. if you write the flash, it
> will send PP opcode + 1 byte in a loop, right ?

I don't think so, info->sector_size and info->n_sectors are used for 
calculating mtd->size and setting mtd->erasesize only. mtd->erasesize 
should be never used when SPI_NOR_NO_ERASE is set in info->flags as 
Cyrille pointed out.

Regards,
Bastian
Marek Vasut Jan. 18, 2017, 3:57 p.m.
On 01/17/2017 03:51 PM, Bastian Stender wrote:
> Hi Marek,

Hi!

> On 01/16/2017 05:07 PM, Marek Vasut wrote:
>> On 01/16/2017 04:12 PM, Bastian Stender wrote:
>>> The previous definitions for mr25h* chips lead to unnecessary limits
>>> regarding writesize and eraseblocksize for MRAM. MRAM is random access
>>> memory rather than page, sector, or block organized memory.
>>>
>>> Instead of 1 sector with 512KB the mr25h40 MRAM chip should be defined
>>> as 512K sectors with 1 byte each. This makes it writesize and
>>> eraseblocksize aligned and reflects MRAM functionality properly.
>>
>> Wouldn't this degrade the performance ? ie. if you write the flash, it
>> will send PP opcode + 1 byte in a loop, right ?
> 
> I don't think so, info->sector_size and info->n_sectors are used for
> calculating mtd->size and setting mtd->erasesize only. mtd->erasesize
> should be never used when SPI_NOR_NO_ERASE is set in info->flags as
> Cyrille pointed out.

Hmmm, does that answer my second question ?
Cyrille Pitchen Jan. 20, 2017, 2:15 p.m.
Le 18/01/2017 à 16:57, Marek Vasut a écrit :
> On 01/17/2017 03:51 PM, Bastian Stender wrote:
>> Hi Marek,
> 
> Hi!
> 
>> On 01/16/2017 05:07 PM, Marek Vasut wrote:
>>> On 01/16/2017 04:12 PM, Bastian Stender wrote:
>>>> The previous definitions for mr25h* chips lead to unnecessary limits
>>>> regarding writesize and eraseblocksize for MRAM. MRAM is random access
>>>> memory rather than page, sector, or block organized memory.
>>>>
>>>> Instead of 1 sector with 512KB the mr25h40 MRAM chip should be defined
>>>> as 512K sectors with 1 byte each. This makes it writesize and
>>>> eraseblocksize aligned and reflects MRAM functionality properly.
>>>
>>> Wouldn't this degrade the performance ? ie. if you write the flash, it
>>> will send PP opcode + 1 byte in a loop, right ?
>>
>> I don't think so, info->sector_size and info->n_sectors are used for
>> calculating mtd->size and setting mtd->erasesize only. mtd->erasesize
>> should be never used when SPI_NOR_NO_ERASE is set in info->flags as
>> Cyrille pointed out.
> 
> Hmmm, does that answer my second question ?
> 

Your second question is about Page Program with a single data byte, right?

Once in spi-nor.c, spi_nor_write() checks nor->page_size (=
info->page_size) to limit the amount of data written into the memory in a
single SPI command: spi_nor_write() itself doesn't check nor->mtd.erasesize
(= info->sector_size) but it doesn't mean that upper mtd layers had not
clamped the 'len' parameter before based on the mtd->erasesize limit.

Also I don't know whether the upper mtd layers expect cases where
mtd->writebufsize (= nor->page_size == 256) > mtd->erasesize (=
info->sector_size == 1).

If we also changed the page size to 1, we would keep the relation (page
size <= erase size) but for sure, spi-nor would also send many Page Program
commands with single data byte, hence a huge performance loss.

That's why I think we should take time to study to be sure the proposed
modification won't introduce any unwanted side effect.

Best regards,

Cyrille
Marek Vasut Jan. 21, 2017, 12:30 a.m.
On 01/20/2017 03:15 PM, Cyrille Pitchen wrote:
> Le 18/01/2017 à 16:57, Marek Vasut a écrit :
>> On 01/17/2017 03:51 PM, Bastian Stender wrote:
>>> Hi Marek,
>>
>> Hi!
>>
>>> On 01/16/2017 05:07 PM, Marek Vasut wrote:
>>>> On 01/16/2017 04:12 PM, Bastian Stender wrote:
>>>>> The previous definitions for mr25h* chips lead to unnecessary limits
>>>>> regarding writesize and eraseblocksize for MRAM. MRAM is random access
>>>>> memory rather than page, sector, or block organized memory.
>>>>>
>>>>> Instead of 1 sector with 512KB the mr25h40 MRAM chip should be defined
>>>>> as 512K sectors with 1 byte each. This makes it writesize and
>>>>> eraseblocksize aligned and reflects MRAM functionality properly.
>>>>
>>>> Wouldn't this degrade the performance ? ie. if you write the flash, it
>>>> will send PP opcode + 1 byte in a loop, right ?
>>>
>>> I don't think so, info->sector_size and info->n_sectors are used for
>>> calculating mtd->size and setting mtd->erasesize only. mtd->erasesize
>>> should be never used when SPI_NOR_NO_ERASE is set in info->flags as
>>> Cyrille pointed out.
>>
>> Hmmm, does that answer my second question ?
>>
> 
> Your second question is about Page Program with a single data byte, right?
> 
> Once in spi-nor.c, spi_nor_write() checks nor->page_size (=
> info->page_size) to limit the amount of data written into the memory in a
> single SPI command: spi_nor_write() itself doesn't check nor->mtd.erasesize
> (= info->sector_size) but it doesn't mean that upper mtd layers had not
> clamped the 'len' parameter before based on the mtd->erasesize limit.
> 
> Also I don't know whether the upper mtd layers expect cases where
> mtd->writebufsize (= nor->page_size == 256) > mtd->erasesize (=
> info->sector_size == 1).
> 
> If we also changed the page size to 1, we would keep the relation (page
> size <= erase size) but for sure, spi-nor would also send many Page Program
> commands with single data byte, hence a huge performance loss.

Yes, that's what my question was about. Looking at your explanation AND
at the patch 2/2, it now makes sense. Sorry for the noise.

> That's why I think we should take time to study to be sure the proposed
> modification won't introduce any unwanted side effect.

I don't think it will, and if it does, then it's a bug anyway.

Patch hide | download patch | download mbox

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c8399522c69d..6a39b18baa70 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -824,9 +824,9 @@  static const struct flash_info spi_nor_ids[] = {
 	{ "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, SECT_4K) },
 
 	/* Everspin */
-	{ "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
-	{ "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
-	{ "mr25h40",  CAT25_INFO(512 * 1024, 1, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
+	{ "mr25h256", CAT25_INFO(1,  32 * 1024, 256, 2, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
+	{ "mr25h10",  CAT25_INFO(1, 128 * 1024, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
+	{ "mr25h40",  CAT25_INFO(1, 512 * 1024, 256, 3, SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
 
 	/* Fujitsu */
 	{ "mb85rs1mt", INFO(0x047f27, 0, 128 * 1024, 1, SPI_NOR_NO_ERASE) },