| Message ID | 20201118182459.18197-3-p.yadav@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: Make sure UBIFS does not do multi-pass page programming on flashes that don't support it | expand |
On 11/18/20 8:24 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Some flashes like the Cypress S28 family use ECC. Under this ECC scheme, > multi-pass writes to an ECC block is not allowed. In other words, once > data is programmed to an ECC block, it can't be programmed again without > erasing it first. > > Upper layers like file systems need to be given this information so they > do not cause error conditions on the flash by attempting multi-pass > programming. This can be done by setting 'writesize' in 'struct > mtd_info'. > > Set the default to 1 but allow flashes to modify it in fixup hooks. If > more flashes show up with this constraint in the future it might be > worth it to add it to 'struct flash_info', but for now increasing its > size is not worth it. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > > Notes: > New in v2. > > drivers/mtd/spi-nor/core.c | 4 +++- > drivers/mtd/spi-nor/core.h | 3 +++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 5bee7c8da4dc..80fbcb9c0828 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -3054,6 +3054,8 @@ static int spi_nor_init_params(struct spi_nor *nor) > if (!nor->params) > return -ENOMEM; > > + nor->params->writesize = 1; > + please do default inits in spi_nor_info_init_params(). A good place would be: --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -2885,6 +2885,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor) nor->flags |= SNOR_F_HAS_16BIT_SR; /* Set SPI NOR sizes. */ + params->writesize = 1; params->size = (u64)info->sector_size * info->n_sectors; params->page_size = info->page_size; > spi_nor_info_init_params(nor); > > spi_nor_manufacturer_init_params(nor); > @@ -3430,7 +3432,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > mtd->name = dev_name(dev); > mtd->priv = nor; > mtd->type = MTD_NORFLASH; > - mtd->writesize = 1; > + mtd->writesize = nor->params->writesize; > mtd->flags = MTD_CAP_NORFLASH; > mtd->size = nor->params->size; > mtd->_erase = spi_nor_erase; > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index 0a775a7b5606..413ea311e632 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -197,6 +197,8 @@ struct spi_nor_locking_ops { > * @rdsr_dummy: dummy cycles needed for Read Status Register command. > * @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register > * command. > + * @writesize Minimal writable flash unit size. Defaults to 1. Set to > + * ECC unit size for ECC-ed flashes. > * @hwcaps: describes the read and page program hardware > * capabilities. > * @reads: read capabilities ordered by priority: the higher index > @@ -222,6 +224,7 @@ struct spi_nor_flash_parameter { > u32 page_size; I would put writesize before or after page_size, because they are related. Also, it would probably avoid padding. With these addressed, one can add: Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> > u8 rdsr_dummy; > u8 rdsr_addr_nbytes; > + u32 writesize; > > struct spi_nor_hwcaps hwcaps; > struct spi_nor_read_command reads[SNOR_CMD_READ_MAX]; > -- > 2.28.0 >
On 28/11/20 10:59AM, Tudor.Ambarus@microchip.com wrote: > On 11/18/20 8:24 PM, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Some flashes like the Cypress S28 family use ECC. Under this ECC scheme, > > multi-pass writes to an ECC block is not allowed. In other words, once > > data is programmed to an ECC block, it can't be programmed again without > > erasing it first. > > > > Upper layers like file systems need to be given this information so they > > do not cause error conditions on the flash by attempting multi-pass > > programming. This can be done by setting 'writesize' in 'struct > > mtd_info'. > > > > Set the default to 1 but allow flashes to modify it in fixup hooks. If > > more flashes show up with this constraint in the future it might be > > worth it to add it to 'struct flash_info', but for now increasing its > > size is not worth it. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > > > Notes: > > New in v2. > > > > drivers/mtd/spi-nor/core.c | 4 +++- > > drivers/mtd/spi-nor/core.h | 3 +++ > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > index 5bee7c8da4dc..80fbcb9c0828 100644 > > --- a/drivers/mtd/spi-nor/core.c > > +++ b/drivers/mtd/spi-nor/core.c > > @@ -3054,6 +3054,8 @@ static int spi_nor_init_params(struct spi_nor *nor) > > if (!nor->params) > > return -ENOMEM; > > > > + nor->params->writesize = 1; > > + > > please do default inits in spi_nor_info_init_params(). A good place would be: > > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2885,6 +2885,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor) > nor->flags |= SNOR_F_HAS_16BIT_SR; > > /* Set SPI NOR sizes. */ > + params->writesize = 1; > params->size = (u64)info->sector_size * info->n_sectors; > params->page_size = info->page_size; Ok. > > > spi_nor_info_init_params(nor); > > > > spi_nor_manufacturer_init_params(nor); > > @@ -3430,7 +3432,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, > > mtd->name = dev_name(dev); > > mtd->priv = nor; > > mtd->type = MTD_NORFLASH; > > - mtd->writesize = 1; > > + mtd->writesize = nor->params->writesize; > > mtd->flags = MTD_CAP_NORFLASH; > > mtd->size = nor->params->size; > > mtd->_erase = spi_nor_erase; > > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > > index 0a775a7b5606..413ea311e632 100644 > > --- a/drivers/mtd/spi-nor/core.h > > +++ b/drivers/mtd/spi-nor/core.h > > @@ -197,6 +197,8 @@ struct spi_nor_locking_ops { > > * @rdsr_dummy: dummy cycles needed for Read Status Register command. > > * @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register > > * command. > > + * @writesize Minimal writable flash unit size. Defaults to 1. Set to > > + * ECC unit size for ECC-ed flashes. > > * @hwcaps: describes the read and page program hardware > > * capabilities. > > * @reads: read capabilities ordered by priority: the higher index > > @@ -222,6 +224,7 @@ struct spi_nor_flash_parameter { > > u32 page_size; > > I would put writesize before or after page_size, because they are related. > Also, it would probably avoid padding. Ok. > With these addressed, one can add: > > Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com> Thanks. > > u8 rdsr_dummy; > > u8 rdsr_addr_nbytes; > > + u32 writesize; > > > > struct spi_nor_hwcaps hwcaps; > > struct spi_nor_read_command reads[SNOR_CMD_READ_MAX]; > > -- > > 2.28.0 > > >
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 5bee7c8da4dc..80fbcb9c0828 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -3054,6 +3054,8 @@ static int spi_nor_init_params(struct spi_nor *nor) if (!nor->params) return -ENOMEM; + nor->params->writesize = 1; + spi_nor_info_init_params(nor); spi_nor_manufacturer_init_params(nor); @@ -3430,7 +3432,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, mtd->name = dev_name(dev); mtd->priv = nor; mtd->type = MTD_NORFLASH; - mtd->writesize = 1; + mtd->writesize = nor->params->writesize; mtd->flags = MTD_CAP_NORFLASH; mtd->size = nor->params->size; mtd->_erase = spi_nor_erase; diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 0a775a7b5606..413ea311e632 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -197,6 +197,8 @@ struct spi_nor_locking_ops { * @rdsr_dummy: dummy cycles needed for Read Status Register command. * @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register * command. + * @writesize Minimal writable flash unit size. Defaults to 1. Set to + * ECC unit size for ECC-ed flashes. * @hwcaps: describes the read and page program hardware * capabilities. * @reads: read capabilities ordered by priority: the higher index @@ -222,6 +224,7 @@ struct spi_nor_flash_parameter { u32 page_size; u8 rdsr_dummy; u8 rdsr_addr_nbytes; + u32 writesize; struct spi_nor_hwcaps hwcaps; struct spi_nor_read_command reads[SNOR_CMD_READ_MAX];
Some flashes like the Cypress S28 family use ECC. Under this ECC scheme, multi-pass writes to an ECC block is not allowed. In other words, once data is programmed to an ECC block, it can't be programmed again without erasing it first. Upper layers like file systems need to be given this information so they do not cause error conditions on the flash by attempting multi-pass programming. This can be done by setting 'writesize' in 'struct mtd_info'. Set the default to 1 but allow flashes to modify it in fixup hooks. If more flashes show up with this constraint in the future it might be worth it to add it to 'struct flash_info', but for now increasing its size is not worth it. Signed-off-by: Pratyush Yadav <p.yadav@ti.com> --- Notes: New in v2. drivers/mtd/spi-nor/core.c | 4 +++- drivers/mtd/spi-nor/core.h | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-)