| Message ID | 20170116151214.11195-2-bst@pengutronix.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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) }, >
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
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 ?
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
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.
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) },
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(-)