| Message ID | 20170116151214.11195-1-bst@pengutronix.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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; >
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; >
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
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 >
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! > [...]
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;
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(-)