[1/2] mtd: spi-nor: make n_sectors in flash_info 32 bit wide

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

Details

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

Commit Message

Bastian Stender Jan. 16, 2017, 3:12 p.m.
Since MRAM chips (like the Everspin mr25h40) are not sector organized
they should be defined as n_sectors * 1 byte sectors. To be able to
store the higher number of sectors n_sectors should be an unsigned 32
bit integer just like sector_size.

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

Comments

Marek Vasut Jan. 16, 2017, 4:06 p.m.
On 01/16/2017 04:12 PM, Bastian Stender wrote:
> Since MRAM chips (like the Everspin mr25h40) are not sector organized
> they should be defined as n_sectors * 1 byte sectors. To be able to
> store the higher number of sectors n_sectors should be an unsigned 32
> bit integer just like sector_size.
> 
> Signed-off-by: Bastian Stender <bst@pengutronix.de>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index da7cd69d4857..c8399522c69d 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -55,7 +55,7 @@ struct flash_info {
>  	 * necessarily called a "sector" by the vendor.
>  	 */
>  	unsigned	sector_size;
> -	u16		n_sectors;
> +	unsigned	n_sectors;

Make this u32 then , to make things consistent .

>  	u16		page_size;
>  	u16		addr_width;
>
Cyrille Pitchen Jan. 16, 2017, 5:34 p.m.
Hi Bastian,

Le 16/01/2017 à 16:12, Bastian Stender a écrit :
> Since MRAM chips (like the Everspin mr25h40) are not sector organized
> they should be defined as n_sectors * 1 byte sectors. To be able to
> store the higher number of sectors n_sectors should be an unsigned 32
> bit integer just like sector_size.
> 

Could you comment a little bit more on what this series improves or fixes?

Swapping the info->sector_size and info->n_sectors doesn't change the value
of mtd->size:
mtd->size = info->sector_size * info->n_sectors;

info->n_sectors is used nowhere else whereas info->sector_size is only used
once more to initialized mtd->erasesize but due to the SPI_NOR_NO_ERASE
info->flags, mtd->erasesize should be never used.

However changing the type of n_sectors from u16 to unsigned int would
increase the size of the struct flash_info hence the size of the
spi_nor_ids[] array. So I don't know whether it's worth it.

However maybe swapping the values would better respect the meaning of
sector "size" and "number".

Best regards,

Cyrille

> Signed-off-by: Bastian Stender <bst@pengutronix.de>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index da7cd69d4857..c8399522c69d 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -55,7 +55,7 @@ struct flash_info {
>  	 * necessarily called a "sector" by the vendor.
>  	 */
>  	unsigned	sector_size;
> -	u16		n_sectors;
> +	unsigned	n_sectors;
>  
>  	u16		page_size;
>  	u16		addr_width;
>
Bastian Stender Jan. 17, 2017, 10:47 a.m.
Hi Cyrille,

On 01/16/2017 06:34 PM, Cyrille Pitchen wrote:
> Le 16/01/2017 à 16:12, Bastian Stender a écrit :
>> Since MRAM chips (like the Everspin mr25h40) are not sector
>> organized they should be defined as n_sectors * 1 byte sectors. To
>> be able to store the higher number of sectors n_sectors should be
>> an unsigned 32 bit integer just like sector_size.
>>
>
> Could you comment a little bit more on what this series improves or
> fixes?

Sure. I want to create multiple writeable partitions on a mr25h40 MRAM 
chip. This does not currently work because drivers/mtd/mtdpart.c makes 
sure that partitions start on erase block boundaries and end on erase 
blocks. That is obviously not the case for multiple partitions with the 
current definition:

{ "mr25h40",  CAT25_INFO(1, 512 * 1024, 256, 3, SPI_NOR_NO_ERASE | 
SPI_NOR_NO_FR) },

So I get

mtd: partition "mtd2" doesn't start on an erase block boundary -- force 
read-only

Adding a check for SPI_NOR_NO_ERASE does not seem to be the way to go:

http://lists.infradead.org/pipermail/linux-mtd/2017-January/071386.html

Setting the erasesize to 1 neither:

http://lists.infradead.org/pipermail/linux-mtd/2014-March/052410.html

I could use SECT_4K, but I'd like to use the advantage of MRAM without 
limitations.

> Swapping the info->sector_size and info->n_sectors doesn't change the
> value of mtd->size: mtd->size = info->sector_size * info->n_sectors;

To be clear: mtd->size is perfectly fine, no need to change it.

> info->n_sectors is used nowhere else whereas info->sector_size is
> only used once more to initialized mtd->erasesize but due to the
> SPI_NOR_NO_ERASE info->flags, mtd->erasesize should be never used.

mtd->erasesize gets used even with SPI_NOR_NO_ERASE set in the check 
mentioned above in mtd_mod_by_eb().

> However changing the type of n_sectors from u16 to unsigned int
> would increase the size of the struct flash_info hence the size of
> the spi_nor_ids[] array. So I don't know whether it's worth it.
>
> However maybe swapping the values would better respect the meaning
> of sector "size" and "number".

MRAM is not page, sector, or block organized memory, so I do not know 
about this one.

Regards,
Bastian Stender
Cyrille Pitchen Jan. 20, 2017, 1:53 p.m.
Le 17/01/2017 à 11:47, Bastian Stender a écrit :
> Hi Cyrille,
> 
> On 01/16/2017 06:34 PM, Cyrille Pitchen wrote:
>> Le 16/01/2017 à 16:12, Bastian Stender a écrit :
>>> Since MRAM chips (like the Everspin mr25h40) are not sector
>>> organized they should be defined as n_sectors * 1 byte sectors. To
>>> be able to store the higher number of sectors n_sectors should be
>>> an unsigned 32 bit integer just like sector_size.
>>>
>>
>> Could you comment a little bit more on what this series improves or
>> fixes?
> 
> Sure. I want to create multiple writeable partitions on a mr25h40 MRAM
> chip. This does not currently work because drivers/mtd/mtdpart.c makes sure
> that partitions start on erase block boundaries and end on erase blocks.
> That is obviously not the case for multiple partitions with the current
> definition:
>

OK now I better understand your issue so let's take it as a bug since it
prevents you from using full features of the memory. However we have to be
cautious because the modification you propose may have unwanted side
effects hence may introduce some regressions.

For instance, currently the erase size >= page size. After your patch it
will no longer be the case: I guess it should be OK for spi-nor.c but I
have no real idea whether the upper mtd layers expect such a thing. We have
to study!


> { "mr25h40",  CAT25_INFO(1, 512 * 1024, 256, 3, SPI_NOR_NO_ERASE |
> SPI_NOR_NO_FR) },
> 
> So I get
> 
> mtd: partition "mtd2" doesn't start on an erase block boundary -- force
> read-only
> 
> Adding a check for SPI_NOR_NO_ERASE does not seem to be the way to go:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2017-January/071386.html
> 
> Setting the erasesize to 1 neither:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052410.html
> 
> I could use SECT_4K, but I'd like to use the advantage of MRAM without
> limitations.
> 
>> Swapping the info->sector_size and info->n_sectors doesn't change the
>> value of mtd->size: mtd->size = info->sector_size * info->n_sectors;
> 
> To be clear: mtd->size is perfectly fine, no need to change it.
> 
>> info->n_sectors is used nowhere else whereas info->sector_size is
>> only used once more to initialized mtd->erasesize but due to the
>> SPI_NOR_NO_ERASE info->flags, mtd->erasesize should be never used.
> 
> mtd->erasesize gets used even with SPI_NOR_NO_ERASE set in the check
> mentioned above in mtd_mod_by_eb().
> 
>> However changing the type of n_sectors from u16 to unsigned int
>> would increase the size of the struct flash_info hence the size of
>> the spi_nor_ids[] array. So I don't know whether it's worth it.
>>
>> However maybe swapping the values would better respect the meaning
>> of sector "size" and "number".
> 
> MRAM is not page, sector, or block organized memory, so I do not know about
> this one.
> 
> Regards,
> Bastian Stender
>
Marek Vasut Jan. 21, 2017, 12:25 a.m.
On 01/20/2017 02:53 PM, Cyrille Pitchen wrote:
> Le 17/01/2017 à 11:47, Bastian Stender a écrit :
>> Hi Cyrille,
>>
>> On 01/16/2017 06:34 PM, Cyrille Pitchen wrote:
>>> Le 16/01/2017 à 16:12, Bastian Stender a écrit :
>>>> Since MRAM chips (like the Everspin mr25h40) are not sector
>>>> organized they should be defined as n_sectors * 1 byte sectors. To
>>>> be able to store the higher number of sectors n_sectors should be
>>>> an unsigned 32 bit integer just like sector_size.
>>>>
>>>
>>> Could you comment a little bit more on what this series improves or
>>> fixes?
>>
>> Sure. I want to create multiple writeable partitions on a mr25h40 MRAM
>> chip. This does not currently work because drivers/mtd/mtdpart.c makes sure
>> that partitions start on erase block boundaries and end on erase blocks.
>> That is obviously not the case for multiple partitions with the current
>> definition:
>>
> 
> OK now I better understand your issue so let's take it as a bug since it
> prevents you from using full features of the memory. However we have to be
> cautious because the modification you propose may have unwanted side
> effects hence may introduce some regressions.

This clearly feels like an improvement, not a bugfix .. unless I missed
something?

> For instance, currently the erase size >= page size. After your patch it
> will no longer be the case: I guess it should be OK for spi-nor.c but I
> have no real idea whether the upper mtd layers expect such a thing. We have
> to study!
> 
[...]

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 da7cd69d4857..c8399522c69d 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -55,7 +55,7 @@  struct flash_info {
 	 * necessarily called a "sector" by the vendor.
 	 */
 	unsigned	sector_size;
-	u16		n_sectors;
+	unsigned	n_sectors;
 
 	u16		page_size;
 	u16		addr_width;