Message ID | 20211209100813.61713-1-alexander.sverdlin@nokia.com |
---|---|
State | Rejected |
Delegated to: | Ambarus Tudor |
Headers | show |
Series | [1/2] mtd: spi-nor: Introduce erase_proto | expand |
Hi Alexander, On 09/12/21 11:08AM, Alexander A Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@nokia.com> > > I've been looking into non-working erase on mt25qu256a and pinpointed it to > be write_proto 1-4-4 selected from SFDP while the chip only supports 1-1-0 > erase. > > For now just introduce the separate protocol without functional change and > leave the real fix for the following patch. > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> > --- > drivers/mtd/spi-nor/core.c | 9 ++++++--- > include/linux/mtd/spi-nor.h | 4 +++- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 2e21d5a..dcd02ea 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -177,7 +177,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode, > > static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs) > { > - if (spi_nor_protocol_is_dtr(nor->write_proto)) > + if (spi_nor_protocol_is_dtr(nor->erase_proto)) > return -EOPNOTSUPP; > > return nor->controller_ops->erase(nor, offs); > @@ -1186,7 +1186,7 @@ static int spi_nor_erase_chip(struct spi_nor *nor) > SPI_MEM_OP_NO_DUMMY, > SPI_MEM_OP_NO_DATA); > > - spi_nor_spimem_setup_op(nor, &op, nor->write_proto); > + spi_nor_spimem_setup_op(nor, &op, nor->erase_proto); > > ret = spi_mem_exec_op(nor->spimem, &op); > } else { > @@ -1331,7 +1331,7 @@ int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) > SPI_MEM_OP_NO_DUMMY, > SPI_MEM_OP_NO_DATA); > > - spi_nor_spimem_setup_op(nor, &op, nor->write_proto); > + spi_nor_spimem_setup_op(nor, &op, nor->erase_proto); > > return spi_mem_exec_op(nor->spimem, &op); > } else if (nor->controller_ops->erase) { > @@ -2727,6 +2727,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor) > */ > if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops) > spi_nor_init_default_locking_ops(nor); > + > + if (!nor->erase_proto) > + nor->erase_proto = nor->write_proto; I get that you are trying to not break any existing flashes with this, but I don't quite like it. We should keep the same initialization flow with erase_proto as with write_proto, read_proto, etc. That is, initialize it to SNOR_PROTO_1_1_1 in spi_nor_scan() and then let the initialization procedure change it as needed. The problem with this is of course that it could break some flashes by selecting the wrong erase. I would expect _most_ flashes to use erase_proto as 1-1-1 but I of course haven't went and looked at every single flash to point out the exceptions. I would like to hear from others if they think it is okay to do this. > } > > /**
On 12/16/21 22:05, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Alexander, > > On 09/12/21 11:08AM, Alexander A Sverdlin wrote: >> From: Alexander Sverdlin <alexander.sverdlin@nokia.com> >> >> I've been looking into non-working erase on mt25qu256a and pinpointed it to >> be write_proto 1-4-4 selected from SFDP while the chip only supports 1-1-0 >> erase. >> >> For now just introduce the separate protocol without functional change and >> leave the real fix for the following patch. >> >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com> >> --- >> drivers/mtd/spi-nor/core.c | 9 ++++++--- >> include/linux/mtd/spi-nor.h | 4 +++- >> 2 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 2e21d5a..dcd02ea 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -177,7 +177,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode, >> >> static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs) >> { >> - if (spi_nor_protocol_is_dtr(nor->write_proto)) >> + if (spi_nor_protocol_is_dtr(nor->erase_proto)) >> return -EOPNOTSUPP; >> >> return nor->controller_ops->erase(nor, offs); >> @@ -1186,7 +1186,7 @@ static int spi_nor_erase_chip(struct spi_nor *nor) >> SPI_MEM_OP_NO_DUMMY, >> SPI_MEM_OP_NO_DATA); >> >> - spi_nor_spimem_setup_op(nor, &op, nor->write_proto); >> + spi_nor_spimem_setup_op(nor, &op, nor->erase_proto); >> >> ret = spi_mem_exec_op(nor->spimem, &op); >> } else { >> @@ -1331,7 +1331,7 @@ int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) >> SPI_MEM_OP_NO_DUMMY, >> SPI_MEM_OP_NO_DATA); >> >> - spi_nor_spimem_setup_op(nor, &op, nor->write_proto); >> + spi_nor_spimem_setup_op(nor, &op, nor->erase_proto); >> >> return spi_mem_exec_op(nor->spimem, &op); >> } else if (nor->controller_ops->erase) { >> @@ -2727,6 +2727,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor) >> */ >> if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops) >> spi_nor_init_default_locking_ops(nor); >> + >> + if (!nor->erase_proto) >> + nor->erase_proto = nor->write_proto; > > I get that you are trying to not break any existing flashes with this, > but I don't quite like it. We should keep the same initialization flow > with erase_proto as with write_proto, read_proto, etc. That is, > initialize it to SNOR_PROTO_1_1_1 in spi_nor_scan() and then let the > initialization procedure change it as needed. > > The problem with this is of course that it could break some flashes by > selecting the wrong erase. I would expect _most_ flashes to use > erase_proto as 1-1-1 but I of course haven't went and looked at every > single flash to point out the exceptions. > > I would like to hear from others if they think it is okay to do this. > Doesn't [1] solve Alexander's problem? Alexander, would you please test Patrice's patch and provide a Tested-by tag if everything is ok? Thanks, ta [1] https://patchwork.ozlabs.org/project/linux-mtd/patch/20220629133013.3382393-1-patrice.chotard@foss.st.com/
Hi Tudor! On 18/07/2022 18:50, Tudor.Ambarus@microchip.com wrote: >>> @@ -2727,6 +2727,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor) >>> */ >>> if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops) >>> spi_nor_init_default_locking_ops(nor); >>> + >>> + if (!nor->erase_proto) >>> + nor->erase_proto = nor->write_proto; >> I get that you are trying to not break any existing flashes with this, >> but I don't quite like it. We should keep the same initialization flow >> with erase_proto as with write_proto, read_proto, etc. That is, >> initialize it to SNOR_PROTO_1_1_1 in spi_nor_scan() and then let the >> initialization procedure change it as needed. >> >> The problem with this is of course that it could break some flashes by >> selecting the wrong erase. I would expect _most_ flashes to use >> erase_proto as 1-1-1 but I of course haven't went and looked at every >> single flash to point out the exceptions. >> >> I would like to hear from others if they think it is okay to do this. >> > Doesn't [1] solve Alexander's problem? Alexander, would you please test > Patrice's patch and provide a Tested-by tag if everything is ok? Yes, looks good, provided the Tested-by tag. > Thanks, > ta > > [1] https://patchwork.ozlabs.org/project/linux-mtd/patch/20220629133013.3382393-1-patrice.chotard@foss.st.com/ >
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 2e21d5a..dcd02ea 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -177,7 +177,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode, static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs) { - if (spi_nor_protocol_is_dtr(nor->write_proto)) + if (spi_nor_protocol_is_dtr(nor->erase_proto)) return -EOPNOTSUPP; return nor->controller_ops->erase(nor, offs); @@ -1186,7 +1186,7 @@ static int spi_nor_erase_chip(struct spi_nor *nor) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_NO_DATA); - spi_nor_spimem_setup_op(nor, &op, nor->write_proto); + spi_nor_spimem_setup_op(nor, &op, nor->erase_proto); ret = spi_mem_exec_op(nor->spimem, &op); } else { @@ -1331,7 +1331,7 @@ int spi_nor_erase_sector(struct spi_nor *nor, u32 addr) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_NO_DATA); - spi_nor_spimem_setup_op(nor, &op, nor->write_proto); + spi_nor_spimem_setup_op(nor, &op, nor->erase_proto); return spi_mem_exec_op(nor->spimem, &op); } else if (nor->controller_ops->erase) { @@ -2727,6 +2727,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor) */ if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops) spi_nor_init_default_locking_ops(nor); + + if (!nor->erase_proto) + nor->erase_proto = nor->write_proto; } /** diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index fc90fce..23a901b 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -381,7 +381,8 @@ struct spi_nor_flash_parameter; * @cmd_ext_type: the command opcode extension type for DTR mode. * @read_proto: the SPI protocol for read operations * @write_proto: the SPI protocol for write operations - * @reg_proto: the SPI protocol for read_reg/write_reg/erase operations + * @reg_proto: the SPI protocol for read_reg/write_reg operations + * @erase_proto: the SPI protocol for erase operations * @sfdp: the SFDP data of the flash * @controller_ops: SPI NOR controller driver specific operations. * @params: [FLASH-SPECIFIC] SPI NOR flash parameters and settings. @@ -408,6 +409,7 @@ struct spi_nor { enum spi_nor_protocol read_proto; enum spi_nor_protocol write_proto; enum spi_nor_protocol reg_proto; + enum spi_nor_protocol erase_proto; bool sst_write_second; u32 flags; enum spi_nor_cmd_ext cmd_ext_type;