| Message ID | 20231225080349.17222-1-Takahiro.Kuwano@infineon.com |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map | expand |
Hi, > -static void spi_nor_set_mtd_info(struct spi_nor *nor) > +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor) > +{ > + struct spi_nor_erase_map *map = &nor->params->erase_map; > + struct spi_nor_erase_region *region = map->regions; > + struct mtd_info *mtd = &nor->mtd; > + struct mtd_erase_region_info *mtd_region; > + u32 erase_size; > + u8 erase_mask; > + int n_regions, i, j; > + > + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) > + ; Please put that into a helper which returns the number of regions. FWIW, I really dislike the magic around encoding all sorts of stuff into the offset. It makes the code just hard to read. > + > + n_regions = i + 1; > + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region), > + GFP_KERNEL); Who's the owner? mtd->dev or nor->dev? > + if (!mtd_region) > + return -ENOMEM; > + > + for (i = 0; i < n_regions; i++) { > + if (region[i].offset & SNOR_OVERLAID_REGION) { Btw. what is an overlaid region? I couldn't find any comment about it. > + erase_size = region[i].size; > + } else { > + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK; > + > + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) { > + if (erase_mask & BIT(j)) { > + erase_size = map->erase_type[j].size; > + break; > + } > + } > + } > + mtd_region[i].erasesize = erase_size; > + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size); > + mtd_region[i].offset = region[i].offset & > + ~SNOR_ERASE_FLAGS_MASK; > + } > + > + mtd->numeraseregions = n_regions; > + mtd->eraseregions = mtd_region; > + > + return 0; > +} > + > +static int spi_nor_set_mtd_info(struct spi_nor *nor) > { > struct mtd_info *mtd = &nor->mtd; > struct device *dev = nor->dev; > @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor > *nor) > mtd->_resume = spi_nor_resume; > mtd->_get_device = spi_nor_get_device; > mtd->_put_device = spi_nor_put_device; > + > + if (spi_nor_has_uniform_erase(nor)) > + return 0; > + > + return spi_nor_set_mtd_eraseregions(nor); mtd->erasesize is set somewhere else, please move it into this function, because it will also have a special case for the non_uniform flashes. Maybe we'll need our own erasesize stored together with the opcode. Also this should be written as if (!spi_nor_has_uniform_erase(nor)) spi_nor_set_mtd_eraseregions(nor); return 0; -michael > } > > static int spi_nor_hw_reset(struct spi_nor *nor) > @@ -3531,7 +3580,9 @@ int spi_nor_scan(struct spi_nor *nor, const char > *name, > return ret; > > /* No mtd_info fields should be used up to this point. */ > - spi_nor_set_mtd_info(nor); > + ret = spi_nor_set_mtd_info(nor); > + if (ret) > + return ret; > > dev_info(dev, "%s (%lld Kbytes)\n", info->name, > (long long)mtd->size >> 10);
On 1/5/2024 9:23 PM, Michael Walle wrote: > Hi, > >> -static void spi_nor_set_mtd_info(struct spi_nor *nor) >> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor) >> +{ >> + struct spi_nor_erase_map *map = &nor->params->erase_map; >> + struct spi_nor_erase_region *region = map->regions; >> + struct mtd_info *mtd = &nor->mtd; >> + struct mtd_erase_region_info *mtd_region; >> + u32 erase_size; >> + u8 erase_mask; >> + int n_regions, i, j; >> + >> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) >> + ; > > Please put that into a helper which returns the number of regions. > Yes, I will do it. > FWIW, I really dislike the magic around encoding all sorts of stuff > into the offset. It makes the code just hard to read. > > >> + >> + n_regions = i + 1; >> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region), >> + GFP_KERNEL); > > Who's the owner? mtd->dev or nor->dev? > I think it should be nor->dev. The mtd device is not yet registered at this point. >> + if (!mtd_region) >> + return -ENOMEM; >> + >> + for (i = 0; i < n_regions; i++) { >> + if (region[i].offset & SNOR_OVERLAID_REGION) { > > Btw. what is an overlaid region? I couldn't find any comment > about it. > It is the remaining part of regular sector that overlaid by 4KB sectors. In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on bottom address, 128KB is overlaid region. The erase opcode for this region is same as 256KB sectors. >> + erase_size = region[i].size; >> + } else { >> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK; >> + >> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) { >> + if (erase_mask & BIT(j)) { >> + erase_size = map->erase_type[j].size; >> + break; >> + } >> + } >> + } >> + mtd_region[i].erasesize = erase_size; >> + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size); >> + mtd_region[i].offset = region[i].offset & >> + ~SNOR_ERASE_FLAGS_MASK; >> + } >> + >> + mtd->numeraseregions = n_regions; >> + mtd->eraseregions = mtd_region; >> + >> + return 0; >> +} >> + >> +static int spi_nor_set_mtd_info(struct spi_nor *nor) >> { >> struct mtd_info *mtd = &nor->mtd; >> struct device *dev = nor->dev; >> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor) >> mtd->_resume = spi_nor_resume; >> mtd->_get_device = spi_nor_get_device; >> mtd->_put_device = spi_nor_put_device; >> + >> + if (spi_nor_has_uniform_erase(nor)) >> + return 0; >> + >> + return spi_nor_set_mtd_eraseregions(nor); > > mtd->erasesize is set somewhere else, please move it into this > function, because it will also have a special case for the > non_uniform flashes. Maybe we'll need our own erasesize stored > together with the opcode. > Let me introduce params->erasesize which set through SFDP parse, then mtd->erasesize = nor->params->erasesize; like as other 'size' params. > Also this should be written as > > if (!spi_nor_has_uniform_erase(nor)) > spi_nor_set_mtd_eraseregions(nor); > > return 0; > > -michael > OK, thanks! >> } >> >> static int spi_nor_hw_reset(struct spi_nor *nor) >> @@ -3531,7 +3580,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, >> return ret; >> >> /* No mtd_info fields should be used up to this point. */ >> - spi_nor_set_mtd_info(nor); >> + ret = spi_nor_set_mtd_info(nor); >> + if (ret) >> + return ret; >> >> dev_info(dev, "%s (%lld Kbytes)\n", info->name, >> (long long)mtd->size >> 10);
On 1/12/24 07:14, Takahiro Kuwano wrote: > On 1/5/2024 9:23 PM, Michael Walle wrote: >> Hi, >> >>> -static void spi_nor_set_mtd_info(struct spi_nor *nor) >>> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor) >>> +{ >>> + struct spi_nor_erase_map *map = &nor->params->erase_map; >>> + struct spi_nor_erase_region *region = map->regions; >>> + struct mtd_info *mtd = &nor->mtd; >>> + struct mtd_erase_region_info *mtd_region; >>> + u32 erase_size; >>> + u8 erase_mask; >>> + int n_regions, i, j; >>> + >>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) >>> + ; >> >> Please put that into a helper which returns the number of regions. >> > Yes, I will do it. > >> FWIW, I really dislike the magic around encoding all sorts of stuff >> into the offset. It makes the code just hard to read. >> >> >>> + >>> + n_regions = i + 1; >>> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region), >>> + GFP_KERNEL); >> >> Who's the owner? mtd->dev or nor->dev? >> > I think it should be nor->dev. > The mtd device is not yet registered at this point. > >>> + if (!mtd_region) >>> + return -ENOMEM; >>> + >>> + for (i = 0; i < n_regions; i++) { >>> + if (region[i].offset & SNOR_OVERLAID_REGION) { >> >> Btw. what is an overlaid region? I couldn't find any comment >> about it. >> > It is the remaining part of regular sector that overlaid by 4KB sectors. > In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on > bottom address, 128KB is overlaid region. The erase opcode for this region is > same as 256KB sectors. > In other words, the overlaid region is what remains from a sector that's not covered by smaller erase types. _____ 128KB <- 128KB overlaid region _____ 4KB _____ ..... 30 other 4KB sectors _____ 4KB _____ the overlaid region does not have a dedicated 128 KB erase command and instead relies on the bigger erase type, 256KB. When the 256KB erase is issued on the 128KB overlaid region, just the 128KB of the overlaid region are erased. Did I remember correctly? Maybe we can describe this somewhere ... Cheers, ta
On 12/25/23 08:03, tkuw584924@gmail.com wrote: > From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > > Some of Infineon SPI NOR flash devices support hybrid sector layout that > overlays 4KB sectors on a 256KB sector and SPI NOR framework recognizes > that by parsing SMPT and construct params->erase_map. The hybrid sector > layout is similar to CFI flash devices that have small sectors on top > and/or bottom address. In case of CFI flash devices, the erase map > information is parsed through CFI table and populated into > mtd->eraseregions so that users can create MTD partitions that aligned with > small sector boundaries. This patch provides the same capability to SPI > NOR flash devices that have non-uniform erase map. > > Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > --- > The s25hs512t has 32 x 4KB, 1 x 128KB, and 255 x 256KB sectors by default. > Let's think about creating three MTD partitions that align to region > boundary. > > BEFORE applying this patch, 1st and 2nd region are forced to read-only: > > ---kernel log--- > ... > spi-nor spi0.0: s25hs512t (65536 Kbytes) > 3 fixed-partitions partitions found on MTD device spi0.0 > Creating 3 MTD partitions on "spi0.0": > 0x000000000000-0x000000020000 : "4KB x 32" > mtd: partition "4KB x 32" doesn't end on an erase/write block -- force read-only > 0x000000020000-0x000000040000 : "128KB x 1" > mtd: partition "128KB x 1" doesn't start on an erase/write block boundary -- force read-only > 0x000000040000-0x000004000000 : "256KB x 255" > ... > > ---MTD Info--- > zynq> mtd_debug info /dev/mtd0 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_ROM > mtd.size = 131072 (128K) > mtd.erasesize = 262144 (256K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > > zynq> mtd_debug info /dev/mtd1 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_ROM > mtd.size = 131072 (128K) > mtd.erasesize = 262144 (256K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > > zynq> mtd_debug info /dev/mtd2 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_NANDFLASH > mtd.size = 66846720 (63M) > mtd.erasesize = 262144 (256K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > > > > AFTER applying this patch, erasesize is correctly informed and no read-only: > > ---kernel log--- > ... > spi-nor spi0.0: s25hs512t (65536 Kbytes) > 3 fixed-partitions partitions found on MTD device spi0.0 > Creating 3 MTD partitions on "spi0.0": > 0x000000000000-0x000000020000 : "4KB x 32" > 0x000000020000-0x000000040000 : "128KB x 1" > 0x000000040000-0x000004000000 : "256KB x 255" > ... > > ---MTD Info--- > zynq> mtd_debug info /dev/mtd0 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_NANDFLASH > mtd.size = 131072 (128K) > mtd.erasesize = 4096 (4K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > > zynq> mtd_debug info /dev/mtd1 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_NANDFLASH > mtd.size = 131072 (128K) > mtd.erasesize = 131072 (128K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > > zynq> mtd_debug info /dev/mtd2 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_NANDFLASH > mtd.size = 66846720 (63M) > mtd.erasesize = 262144 (256K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > nice! > --- > drivers/mtd/spi-nor/core.c | 55 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 53 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 1b0c6770c14e..e512491733a8 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -3408,7 +3408,51 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > return info; > } > > -static void spi_nor_set_mtd_info(struct spi_nor *nor) > +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor) > +{ > + struct spi_nor_erase_map *map = &nor->params->erase_map; > + struct spi_nor_erase_region *region = map->regions; shall we have some consts above? > + struct mtd_info *mtd = &nor->mtd; some prefer a reverse xmas tree, thus put the definition from above below the declaration from below. > + struct mtd_erase_region_info *mtd_region; > + u32 erase_size; > + u8 erase_mask; put the u8 last to avoid stack padding > + int n_regions, i, j; unsigned int > + > + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) > + ; > + > + n_regions = i + 1; all this just to get the number of regions? how about saving the number of regions somewhere and use it everywhere? > + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region), > + GFP_KERNEL); > + if (!mtd_region) > + return -ENOMEM; > + > + for (i = 0; i < n_regions; i++) { > + if (region[i].offset & SNOR_OVERLAID_REGION) { > + erase_size = region[i].size; > + } else { > + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK; > + > + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) { > + if (erase_mask & BIT(j)) { > + erase_size = map->erase_type[j].size; > + break; > + } > + } this for, determining the erase size, deserves a dedicated method. Too many indentation levels. > + } > + mtd_region[i].erasesize = erase_size; > + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size); > + mtd_region[i].offset = region[i].offset & > + ~SNOR_ERASE_FLAGS_MASK; > + } > + > + mtd->numeraseregions = n_regions; > + mtd->eraseregions = mtd_region; > + > + return 0; > +} > + > +static int spi_nor_set_mtd_info(struct spi_nor *nor) > { > struct mtd_info *mtd = &nor->mtd; > struct device *dev = nor->dev; > @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor) > mtd->_resume = spi_nor_resume; > mtd->_get_device = spi_nor_get_device; > mtd->_put_device = spi_nor_put_device; > + > + if (spi_nor_has_uniform_erase(nor)) > + return 0; > + > + return spi_nor_set_mtd_eraseregions(nor); > } > > static int spi_nor_hw_reset(struct spi_nor *nor) > @@ -3531,7 +3580,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > return ret; > > /* No mtd_info fields should be used up to this point. */ > - spi_nor_set_mtd_info(nor); > + ret = spi_nor_set_mtd_info(nor); > + if (ret) > + return ret; > > dev_info(dev, "%s (%lld Kbytes)\n", info->name, > (long long)mtd->size >> 10);
On 12/25/23 08:03, tkuw584924@gmail.com wrote: > From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > > Some of Infineon SPI NOR flash devices support hybrid sector layout that > overlays 4KB sectors on a 256KB sector and SPI NOR framework recognizes > that by parsing SMPT and construct params->erase_map. The hybrid sector > layout is similar to CFI flash devices that have small sectors on top > and/or bottom address. In case of CFI flash devices, the erase map > information is parsed through CFI table and populated into > mtd->eraseregions so that users can create MTD partitions that aligned with > small sector boundaries. This patch provides the same capability to SPI > NOR flash devices that have non-uniform erase map. > > Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > --- > The s25hs512t has 32 x 4KB, 1 x 128KB, and 255 x 256KB sectors by default. > Let's think about creating three MTD partitions that align to region > boundary. > > BEFORE applying this patch, 1st and 2nd region are forced to read-only: > > ---kernel log--- > ... > spi-nor spi0.0: s25hs512t (65536 Kbytes) > 3 fixed-partitions partitions found on MTD device spi0.0 > Creating 3 MTD partitions on "spi0.0": > 0x000000000000-0x000000020000 : "4KB x 32" > mtd: partition "4KB x 32" doesn't end on an erase/write block -- force read-only > 0x000000020000-0x000000040000 : "128KB x 1" > mtd: partition "128KB x 1" doesn't start on an erase/write block boundary -- force read-only > 0x000000040000-0x000004000000 : "256KB x 255" > ... > > ---MTD Info--- > zynq> mtd_debug info /dev/mtd0 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_ROM > mtd.size = 131072 (128K) > mtd.erasesize = 262144 (256K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > > zynq> mtd_debug info /dev/mtd1 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_ROM > mtd.size = 131072 (128K) > mtd.erasesize = 262144 (256K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > > zynq> mtd_debug info /dev/mtd2 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_NANDFLASH > mtd.size = 66846720 (63M) > mtd.erasesize = 262144 (256K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > > > > AFTER applying this patch, erasesize is correctly informed and no read-only: > > ---kernel log--- > ... > spi-nor spi0.0: s25hs512t (65536 Kbytes) > 3 fixed-partitions partitions found on MTD device spi0.0 > Creating 3 MTD partitions on "spi0.0": > 0x000000000000-0x000000020000 : "4KB x 32" > 0x000000020000-0x000000040000 : "128KB x 1" > 0x000000040000-0x000004000000 : "256KB x 255" > ... > > ---MTD Info--- > zynq> mtd_debug info /dev/mtd0 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_NANDFLASH > mtd.size = 131072 (128K) > mtd.erasesize = 4096 (4K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > > zynq> mtd_debug info /dev/mtd1 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_NANDFLASH > mtd.size = 131072 (128K) > mtd.erasesize = 131072 (128K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > > zynq> mtd_debug info /dev/mtd2 > mtd.type = MTD_NORFLASH > mtd.flags = MTD_CAP_NANDFLASH > mtd.size = 66846720 (63M) > mtd.erasesize = 262144 (256K) > mtd.writesize = 16 > mtd.oobsize = 0 > regions = 0 > nice! > --- > drivers/mtd/spi-nor/core.c | 55 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 53 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 1b0c6770c14e..e512491733a8 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -3408,7 +3408,51 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > return info; > } > > -static void spi_nor_set_mtd_info(struct spi_nor *nor) > +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor) > +{ > + struct spi_nor_erase_map *map = &nor->params->erase_map; > + struct spi_nor_erase_region *region = map->regions; shall we have some consts above? > + struct mtd_info *mtd = &nor->mtd; some prefer a reverse xmas tree, thus put the definition from above below the declaration from below. > + struct mtd_erase_region_info *mtd_region; > + u32 erase_size; > + u8 erase_mask; put the u8 last to avoid stack padding > + int n_regions, i, j; unsigned int > + > + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) > + ; > + > + n_regions = i + 1; all this just to get the number of regions? how about saving the number of regions somewhere and use it everywhere? > + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region), > + GFP_KERNEL); > + if (!mtd_region) > + return -ENOMEM; > + > + for (i = 0; i < n_regions; i++) { > + if (region[i].offset & SNOR_OVERLAID_REGION) { > + erase_size = region[i].size; > + } else { > + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK; > + > + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) { > + if (erase_mask & BIT(j)) { > + erase_size = map->erase_type[j].size; > + break; > + } > + } this for, determining the erase size, deserves a dedicated method. Too many indentation levels. Cheers, ta
>> + struct mtd_erase_region_info *mtd_region; >> + u32 erase_size; >> + u8 erase_mask; > > put the u8 last to avoid stack padding I don't think that is a thing. Even if it were, it might clash with the RCT. I couldn't find anything about how automatic variables are placed in memory. I'd say its not specified in the standard and the compiler is free to do optimizations here or just keep the contents in registers (?!). Any stytle recommendations for spi-nor? I prefer RCT, but if we want to say declaration order doesn't matter, I'm fine with that too. >> + >> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) >> + ; >> + >> + n_regions = i + 1; > > all this just to get the number of regions? how about saving the number > of regions somewhere and use it everywhere? This whole region thing should be rewritten to not store these magic bits in the offset. -michael
On 1/12/24 12:01, Michael Walle wrote: >>> + struct mtd_erase_region_info *mtd_region; >>> + u32 erase_size; >>> + u8 erase_mask; >> >> put the u8 last to avoid stack padding > > I don't think that is a thing. Even if it were, it might clash > with the RCT. RCT as in reverse Christmas tree? if we put u8 at the end we'll respect RCT and without padding holes in the stack. > > I couldn't find anything about how automatic variables are placed > in memory. I'd say its not specified in the standard and the compiler > is free to do optimizations here or just keep the contents in > registers (?!). > I can't tell. > Any stytle recommendations for spi-nor? I prefer RCT, but if we want > to say declaration order doesn't matter, I'm fine with that too. RCT or CT with stack padding in mind :). But that's just common sense, I guess. > >>> + >>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) >>> + ; >>> + >>> + n_regions = i + 1; >> >> all this just to get the number of regions? how about saving the number >> of regions somewhere and use it everywhere? > > This whole region thing should be rewritten to not store these magic > bits in the offset. > yeah, probably.
>>>> + struct mtd_erase_region_info *mtd_region; >>>> + u32 erase_size; >>>> + u8 erase_mask; >>> >>> put the u8 last to avoid stack padding >> >> I don't think that is a thing. Even if it were, it might clash >> with the RCT. > > RCT as in reverse Christmas tree? yes > if we put u8 at the end we'll respect RCT and without padding holes > in the stack. It might clash, not in this particular example. >> I couldn't find anything about how automatic variables are placed >> in memory. I'd say its not specified in the standard and the compiler >> is free to do optimizations here or just keep the contents in >> registers (?!). >> > > I can't tell. > >> Any stytle recommendations for spi-nor? I prefer RCT, but if we want >> to say declaration order doesn't matter, I'm fine with that too. > > RCT or CT with stack padding in mind :). But that's just common sense, > I > guess. Again, that's not a thing. So IMHO order doesn't matter so we should just say RCT, i.e. the same as netdev. -michael
On 1/12/2024 4:14 PM, Takahiro Kuwano wrote: > On 1/5/2024 9:23 PM, Michael Walle wrote: >> Hi, >> >>> -static void spi_nor_set_mtd_info(struct spi_nor *nor) >>> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor) >>> +{ >>> + struct spi_nor_erase_map *map = &nor->params->erase_map; >>> + struct spi_nor_erase_region *region = map->regions; >>> + struct mtd_info *mtd = &nor->mtd; >>> + struct mtd_erase_region_info *mtd_region; >>> + u32 erase_size; >>> + u8 erase_mask; >>> + int n_regions, i, j; >>> + >>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) >>> + ; >> >> Please put that into a helper which returns the number of regions. >> > Yes, I will do it. > >> FWIW, I really dislike the magic around encoding all sorts of stuff >> into the offset. It makes the code just hard to read. >> >> >>> + >>> + n_regions = i + 1; >>> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region), >>> + GFP_KERNEL); >> >> Who's the owner? mtd->dev or nor->dev? >> > I think it should be nor->dev. > The mtd device is not yet registered at this point. > >>> + if (!mtd_region) >>> + return -ENOMEM; >>> + >>> + for (i = 0; i < n_regions; i++) { >>> + if (region[i].offset & SNOR_OVERLAID_REGION) { >> >> Btw. what is an overlaid region? I couldn't find any comment >> about it. >> > It is the remaining part of regular sector that overlaid by 4KB sectors. > In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on > bottom address, 128KB is overlaid region. The erase opcode for this region is > same as 256KB sectors. > >>> + erase_size = region[i].size; >>> + } else { >>> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK; >>> + >>> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) { >>> + if (erase_mask & BIT(j)) { >>> + erase_size = map->erase_type[j].size; >>> + break; >>> + } >>> + } >>> + } >>> + mtd_region[i].erasesize = erase_size; >>> + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size); >>> + mtd_region[i].offset = region[i].offset & >>> + ~SNOR_ERASE_FLAGS_MASK; >>> + } >>> + >>> + mtd->numeraseregions = n_regions; >>> + mtd->eraseregions = mtd_region; >>> + >>> + return 0; >>> +} >>> + >>> +static int spi_nor_set_mtd_info(struct spi_nor *nor) >>> { >>> struct mtd_info *mtd = &nor->mtd; >>> struct device *dev = nor->dev; >>> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor) >>> mtd->_resume = spi_nor_resume; >>> mtd->_get_device = spi_nor_get_device; >>> mtd->_put_device = spi_nor_put_device; >>> + >>> + if (spi_nor_has_uniform_erase(nor)) >>> + return 0; >>> + >>> + return spi_nor_set_mtd_eraseregions(nor); >> >> mtd->erasesize is set somewhere else, please move it into this >> function, because it will also have a special case for the >> non_uniform flashes. Maybe we'll need our own erasesize stored >> together with the opcode. >> > Let me introduce params->erasesize which set through SFDP parse, then > > mtd->erasesize = nor->params->erasesize; > > like as other 'size' params. > I tried to implement this, but found mtd->erasesize is set in some fixup hooks in manufacturer driver and need to think carefully about changing them. Let me do this later in another series of patches. Thanks, Takahiro
On 1/19/24 06:29, Takahiro Kuwano wrote: > On 1/12/2024 4:14 PM, Takahiro Kuwano wrote: >> On 1/5/2024 9:23 PM, Michael Walle wrote: >>> Hi, >>> >>>> -static void spi_nor_set_mtd_info(struct spi_nor *nor) >>>> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor) >>>> +{ >>>> + struct spi_nor_erase_map *map = &nor->params->erase_map; >>>> + struct spi_nor_erase_region *region = map->regions; >>>> + struct mtd_info *mtd = &nor->mtd; >>>> + struct mtd_erase_region_info *mtd_region; >>>> + u32 erase_size; >>>> + u8 erase_mask; >>>> + int n_regions, i, j; >>>> + >>>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) >>>> + ; >>> >>> Please put that into a helper which returns the number of regions. >>> >> Yes, I will do it. >> >>> FWIW, I really dislike the magic around encoding all sorts of stuff >>> into the offset. It makes the code just hard to read. >>> >>> >>>> + >>>> + n_regions = i + 1; >>>> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region), >>>> + GFP_KERNEL); >>> >>> Who's the owner? mtd->dev or nor->dev? >>> >> I think it should be nor->dev. >> The mtd device is not yet registered at this point. >> >>>> + if (!mtd_region) >>>> + return -ENOMEM; >>>> + >>>> + for (i = 0; i < n_regions; i++) { >>>> + if (region[i].offset & SNOR_OVERLAID_REGION) { >>> >>> Btw. what is an overlaid region? I couldn't find any comment >>> about it. >>> >> It is the remaining part of regular sector that overlaid by 4KB sectors. >> In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on >> bottom address, 128KB is overlaid region. The erase opcode for this region is >> same as 256KB sectors. >> >>>> + erase_size = region[i].size; >>>> + } else { >>>> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK; >>>> + >>>> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) { >>>> + if (erase_mask & BIT(j)) { >>>> + erase_size = map->erase_type[j].size; >>>> + break; >>>> + } >>>> + } >>>> + } >>>> + mtd_region[i].erasesize = erase_size; >>>> + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size); >>>> + mtd_region[i].offset = region[i].offset & >>>> + ~SNOR_ERASE_FLAGS_MASK; >>>> + } >>>> + >>>> + mtd->numeraseregions = n_regions; >>>> + mtd->eraseregions = mtd_region; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int spi_nor_set_mtd_info(struct spi_nor *nor) >>>> { >>>> struct mtd_info *mtd = &nor->mtd; >>>> struct device *dev = nor->dev; >>>> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor) >>>> mtd->_resume = spi_nor_resume; >>>> mtd->_get_device = spi_nor_get_device; >>>> mtd->_put_device = spi_nor_put_device; >>>> + >>>> + if (spi_nor_has_uniform_erase(nor)) >>>> + return 0; >>>> + >>>> + return spi_nor_set_mtd_eraseregions(nor); >>> >>> mtd->erasesize is set somewhere else, please move it into this >>> function, because it will also have a special case for the >>> non_uniform flashes. Maybe we'll need our own erasesize stored >>> together with the opcode. >>> >> Let me introduce params->erasesize which set through SFDP parse, then >> >> mtd->erasesize = nor->params->erasesize; >> >> like as other 'size' params. >> > I tried to implement this, but found mtd->erasesize is set in some fixup hooks > in manufacturer driver and need to think carefully about changing them. > Let me do this later in another series of patches. > Do you mean xilinx? That's a dead weight, I haven't seen patches for it since its introduction. I'm thinking of getting rid of the xilinx s3an flashes from the SPI NOR. Michael, Pratyush? Cheers, ta
On 1/19/2024 3:55 PM, Tudor Ambarus wrote: > > > On 1/19/24 06:29, Takahiro Kuwano wrote: >> On 1/12/2024 4:14 PM, Takahiro Kuwano wrote: >>> On 1/5/2024 9:23 PM, Michael Walle wrote: >>>> Hi, >>>> >>>>> -static void spi_nor_set_mtd_info(struct spi_nor *nor) >>>>> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor) >>>>> +{ >>>>> + struct spi_nor_erase_map *map = &nor->params->erase_map; >>>>> + struct spi_nor_erase_region *region = map->regions; >>>>> + struct mtd_info *mtd = &nor->mtd; >>>>> + struct mtd_erase_region_info *mtd_region; >>>>> + u32 erase_size; >>>>> + u8 erase_mask; >>>>> + int n_regions, i, j; >>>>> + >>>>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) >>>>> + ; >>>> >>>> Please put that into a helper which returns the number of regions. >>>> >>> Yes, I will do it. >>> >>>> FWIW, I really dislike the magic around encoding all sorts of stuff >>>> into the offset. It makes the code just hard to read. >>>> >>>> >>>>> + >>>>> + n_regions = i + 1; >>>>> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region), >>>>> + GFP_KERNEL); >>>> >>>> Who's the owner? mtd->dev or nor->dev? >>>> >>> I think it should be nor->dev. >>> The mtd device is not yet registered at this point. >>> >>>>> + if (!mtd_region) >>>>> + return -ENOMEM; >>>>> + >>>>> + for (i = 0; i < n_regions; i++) { >>>>> + if (region[i].offset & SNOR_OVERLAID_REGION) { >>>> >>>> Btw. what is an overlaid region? I couldn't find any comment >>>> about it. >>>> >>> It is the remaining part of regular sector that overlaid by 4KB sectors. >>> In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on >>> bottom address, 128KB is overlaid region. The erase opcode for this region is >>> same as 256KB sectors. >>> >>>>> + erase_size = region[i].size; >>>>> + } else { >>>>> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK; >>>>> + >>>>> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) { >>>>> + if (erase_mask & BIT(j)) { >>>>> + erase_size = map->erase_type[j].size; >>>>> + break; >>>>> + } >>>>> + } >>>>> + } >>>>> + mtd_region[i].erasesize = erase_size; >>>>> + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size); >>>>> + mtd_region[i].offset = region[i].offset & >>>>> + ~SNOR_ERASE_FLAGS_MASK; >>>>> + } >>>>> + >>>>> + mtd->numeraseregions = n_regions; >>>>> + mtd->eraseregions = mtd_region; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int spi_nor_set_mtd_info(struct spi_nor *nor) >>>>> { >>>>> struct mtd_info *mtd = &nor->mtd; >>>>> struct device *dev = nor->dev; >>>>> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor) >>>>> mtd->_resume = spi_nor_resume; >>>>> mtd->_get_device = spi_nor_get_device; >>>>> mtd->_put_device = spi_nor_put_device; >>>>> + >>>>> + if (spi_nor_has_uniform_erase(nor)) >>>>> + return 0; >>>>> + >>>>> + return spi_nor_set_mtd_eraseregions(nor); >>>> >>>> mtd->erasesize is set somewhere else, please move it into this >>>> function, because it will also have a special case for the >>>> non_uniform flashes. Maybe we'll need our own erasesize stored >>>> together with the opcode. >>>> >>> Let me introduce params->erasesize which set through SFDP parse, then >>> >>> mtd->erasesize = nor->params->erasesize; >>> >>> like as other 'size' params. >>> >> I tried to implement this, but found mtd->erasesize is set in some fixup hooks >> in manufacturer driver and need to think carefully about changing them. >> Let me do this later in another series of patches. >> > > Do you mean xilinx? That's a dead weight, I haven't seen patches for it > since its introduction. I'm thinking of getting rid of the xilinx s3an > flashes from the SPI NOR. Michael, Pratyush? > spansion also. Changing it may impact to some older parts. I need time to test and debug with different Flash parts. > Cheers, > ta
Hi, > Do you mean xilinx? That's a dead weight, I haven't seen patches for it > since its introduction. I'm thinking of getting rid of the xilinx s3an > flashes from the SPI NOR. Michael, Pratyush? Yes, please! Then all these special handling with the not-power-of-2 sectors can be dropped. -michael
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 1b0c6770c14e..e512491733a8 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -3408,7 +3408,51 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, return info; } -static void spi_nor_set_mtd_info(struct spi_nor *nor) +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor) +{ + struct spi_nor_erase_map *map = &nor->params->erase_map; + struct spi_nor_erase_region *region = map->regions; + struct mtd_info *mtd = &nor->mtd; + struct mtd_erase_region_info *mtd_region; + u32 erase_size; + u8 erase_mask; + int n_regions, i, j; + + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++) + ; + + n_regions = i + 1; + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region), + GFP_KERNEL); + if (!mtd_region) + return -ENOMEM; + + for (i = 0; i < n_regions; i++) { + if (region[i].offset & SNOR_OVERLAID_REGION) { + erase_size = region[i].size; + } else { + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK; + + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) { + if (erase_mask & BIT(j)) { + erase_size = map->erase_type[j].size; + break; + } + } + } + mtd_region[i].erasesize = erase_size; + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size); + mtd_region[i].offset = region[i].offset & + ~SNOR_ERASE_FLAGS_MASK; + } + + mtd->numeraseregions = n_regions; + mtd->eraseregions = mtd_region; + + return 0; +} + +static int spi_nor_set_mtd_info(struct spi_nor *nor) { struct mtd_info *mtd = &nor->mtd; struct device *dev = nor->dev; @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor) mtd->_resume = spi_nor_resume; mtd->_get_device = spi_nor_get_device; mtd->_put_device = spi_nor_put_device; + + if (spi_nor_has_uniform_erase(nor)) + return 0; + + return spi_nor_set_mtd_eraseregions(nor); } static int spi_nor_hw_reset(struct spi_nor *nor) @@ -3531,7 +3580,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, return ret; /* No mtd_info fields should be used up to this point. */ - spi_nor_set_mtd_info(nor); + ret = spi_nor_set_mtd_info(nor); + if (ret) + return ret; dev_info(dev, "%s (%lld Kbytes)\n", info->name, (long long)mtd->size >> 10);