| Message ID | 20220228111712.111737-5-tudor.ambarus@microchip.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mtd: spi-nor: Rework Octal DTR methods | expand |
On 28/02/22 01:17PM, Tudor Ambarus wrote: > RDID is used in the core to auto detect the flash, but also by some > manufacturer drivers that contain flashes that support Octal DTR mode, > so that they can read the flash ID after the switch to Octal DTR was made > to test if the switch was successful. Introduce a core method for RDID op > to avoid code duplication. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------ > drivers/mtd/spi-nor/core.h | 9 ++++++ > 2 files changed, 49 insertions(+), 18 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index b1d6fa65417d..281e3d25f74c 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor) > return ret; > } > > +/** > + * spi_nor_read_id() - Read the JEDEC ID. > + * @nor: pointer to 'struct spi_nor'. > + * @naddr: number of address bytes to send. Can be zero if the operation > + * does not need to send an address. > + * @ndummy: number of dummy bytes to send after an opcode or address. Can > + * be zero if the operation does not require dummy bytes. > + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID > + * will be written. > + * @reg_proto: the SPI protocol for register operation. > + * > + * Return: 0 on success, -errno otherwise. > + */ > +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > + enum spi_nor_protocol reg_proto) Nitpick: Could just call it 'proto'. > +{ > + int ret; > + > + if (nor->spimem) { > + struct spi_mem_op op = > + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN); > + > + spi_nor_spimem_setup_op(nor, &op, reg_proto); > + ret = spi_mem_exec_op(nor->spimem, &op); > + } else { > + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, > + SPI_NOR_MAX_ID_LEN); > + } > + > + if (ret) > + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); I think this message should be in spi_nor_detect(). Let octal DTR enable methods print their own, more specific error messages. > + > + return ret; > +} > + > /** > * spi_nor_read_sr() - Read the Status Register. > * @nor: pointer to 'struct spi_nor'. > @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor, > return NULL; > } > > -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) > +static const struct flash_info *spi_nor_detect(struct spi_nor *nor) > { > const struct flash_info *info; > u8 *id = nor->bouncebuf; > int ret; > > - if (nor->spimem) { > - struct spi_mem_op op = > - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), > - SPI_MEM_OP_NO_ADDR, > - SPI_MEM_OP_NO_DUMMY, > - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1)); > - > - ret = spi_mem_exec_op(nor->spimem, &op); > - } else { > - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, > - SPI_NOR_MAX_ID_LEN); > - } > - if (ret) { > - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); > + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto); Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so clearly signify that this is intended to use 1S-1S-1S only. What do you think? > + if (ret) > return ERR_PTR(ret); > - } > > info = spi_nor_match_id(nor, id); > if (!info) { > @@ -2900,7 +2922,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > info = spi_nor_match_name(nor, name); > /* Try to auto-detect if chip name wasn't specified or not found */ > if (!info) { > - detected_info = spi_nor_read_id(nor); > + detected_info = spi_nor_detect(nor); > info = detected_info; > } > if (IS_ERR_OR_NULL(info)) > @@ -2913,7 +2935,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, > if (name && !detected_info && info->id_len) { > const struct flash_info *jinfo; > > - jinfo = spi_nor_read_id(nor); > + jinfo = spi_nor_detect(nor); > if (IS_ERR(jinfo)) { > return jinfo; > } else if (jinfo != info) { > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index b7fd760e3b47..f952061d5c24 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -11,6 +11,13 @@ > > #define SPI_NOR_MAX_ID_LEN 6 > > +/* Standard SPI NOR flash operations. */ > +#define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \ > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0), \ > + SPI_MEM_OP_ADDR(naddr, 0, 0), \ > + SPI_MEM_OP_DUMMY(ndummy, 0), \ > + SPI_MEM_OP_DATA_IN(len, buf, 0)) > + > enum spi_nor_option_flags { > SNOR_F_HAS_SR_TB = BIT(0), > SNOR_F_NO_OP_CHIP_ERASE = BIT(1), > @@ -534,6 +541,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor); > int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor); > int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor); > int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor); > +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > + enum spi_nor_protocol reg_proto); > int spi_nor_read_sr(struct spi_nor *nor, u8 *sr); > int spi_nor_sr_ready(struct spi_nor *nor); > int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
On 3/21/22 14:21, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 28/02/22 01:17PM, Tudor Ambarus wrote: >> RDID is used in the core to auto detect the flash, but also by some >> manufacturer drivers that contain flashes that support Octal DTR mode, >> so that they can read the flash ID after the switch to Octal DTR was made >> to test if the switch was successful. Introduce a core method for RDID op >> to avoid code duplication. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------ >> drivers/mtd/spi-nor/core.h | 9 ++++++ >> 2 files changed, 49 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index b1d6fa65417d..281e3d25f74c 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor) >> return ret; >> } >> >> +/** >> + * spi_nor_read_id() - Read the JEDEC ID. >> + * @nor: pointer to 'struct spi_nor'. >> + * @naddr: number of address bytes to send. Can be zero if the operation >> + * does not need to send an address. >> + * @ndummy: number of dummy bytes to send after an opcode or address. Can >> + * be zero if the operation does not require dummy bytes. >> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID >> + * will be written. >> + * @reg_proto: the SPI protocol for register operation. >> + * >> + * Return: 0 on success, -errno otherwise. >> + */ >> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, >> + enum spi_nor_protocol reg_proto) > > Nitpick: Could just call it 'proto'. sure, will update > >> +{ >> + int ret; >> + >> + if (nor->spimem) { >> + struct spi_mem_op op = >> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN); >> + >> + spi_nor_spimem_setup_op(nor, &op, reg_proto); >> + ret = spi_mem_exec_op(nor->spimem, &op); >> + } else { >> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, >> + SPI_NOR_MAX_ID_LEN); >> + } >> + >> + if (ret) >> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); > > I think this message should be in spi_nor_detect(). Let octal DTR enable As of now every SPI NOR operation that return an error also prints a dbg message. I like this because it offers a smaller granularity on the error cause. > methods print their own, more specific error messages. How about duplicating the error in the octal dtr enable methods if you feel it is worth it? > >> + >> + return ret; >> +} >> + >> /** >> * spi_nor_read_sr() - Read the Status Register. >> * @nor: pointer to 'struct spi_nor'. >> @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor, >> return NULL; >> } >> >> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) >> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor) >> { >> const struct flash_info *info; >> u8 *id = nor->bouncebuf; >> int ret; >> >> - if (nor->spimem) { >> - struct spi_mem_op op = >> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), >> - SPI_MEM_OP_NO_ADDR, >> - SPI_MEM_OP_NO_DUMMY, >> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1)); >> - >> - ret = spi_mem_exec_op(nor->spimem, &op); >> - } else { >> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, >> - SPI_NOR_MAX_ID_LEN); >> - } >> - if (ret) { >> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); >> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto); > > Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so > clearly signify that this is intended to use 1S-1S-1S only. What do you > think? I would keep it as it is for now, because it offers flexibility. If we ever gonna determine the protocol at runtime this will come in handy because it will work without touching the code. JESD216 suggests an algorithm that tries to determine the mode depending on the SFDP signature. Cheers, ta
On 21/03/22 01:18PM, Tudor.Ambarus@microchip.com wrote: > On 3/21/22 14:21, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On 28/02/22 01:17PM, Tudor Ambarus wrote: > >> RDID is used in the core to auto detect the flash, but also by some > >> manufacturer drivers that contain flashes that support Octal DTR mode, > >> so that they can read the flash ID after the switch to Octal DTR was made > >> to test if the switch was successful. Introduce a core method for RDID op > >> to avoid code duplication. > >> > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > >> --- > >> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------ > >> drivers/mtd/spi-nor/core.h | 9 ++++++ > >> 2 files changed, 49 insertions(+), 18 deletions(-) > >> > >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > >> index b1d6fa65417d..281e3d25f74c 100644 > >> --- a/drivers/mtd/spi-nor/core.c > >> +++ b/drivers/mtd/spi-nor/core.c > >> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor) > >> return ret; > >> } > >> > >> +/** > >> + * spi_nor_read_id() - Read the JEDEC ID. > >> + * @nor: pointer to 'struct spi_nor'. > >> + * @naddr: number of address bytes to send. Can be zero if the operation > >> + * does not need to send an address. > >> + * @ndummy: number of dummy bytes to send after an opcode or address. Can > >> + * be zero if the operation does not require dummy bytes. > >> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID > >> + * will be written. > >> + * @reg_proto: the SPI protocol for register operation. > >> + * > >> + * Return: 0 on success, -errno otherwise. > >> + */ > >> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > >> + enum spi_nor_protocol reg_proto) > > > > Nitpick: Could just call it 'proto'. > > sure, will update > > > > >> +{ > >> + int ret; > >> + > >> + if (nor->spimem) { > >> + struct spi_mem_op op = > >> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN); > >> + > >> + spi_nor_spimem_setup_op(nor, &op, reg_proto); > >> + ret = spi_mem_exec_op(nor->spimem, &op); > >> + } else { > >> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, > >> + SPI_NOR_MAX_ID_LEN); > >> + } > >> + > >> + if (ret) > >> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); > > > > I think this message should be in spi_nor_detect(). Let octal DTR enable > > As of now every SPI NOR operation that return an error also prints a dbg > message. I like this because it offers a smaller granularity on the error > cause. Yes, but I think this message would be misleading. If someone sees "error reading JEDEC ID", they would think flash detection itself has failed, not that we failed to switch to Octal DTR mode. > > > methods print their own, more specific error messages. > > How about duplicating the error in the octal dtr enable methods if you > feel it is worth it? They should at the very least explain that reading ID failed _after_ attempting to switch to Octal DTR. But I think it would just be simpler if this is not printed here and the caller has the flexibility to explain the error. > > > > >> + > >> + return ret; > >> +} > >> + > >> /** > >> * spi_nor_read_sr() - Read the Status Register. > >> * @nor: pointer to 'struct spi_nor'. > >> @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor, > >> return NULL; > >> } > >> > >> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) > >> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor) > >> { > >> const struct flash_info *info; > >> u8 *id = nor->bouncebuf; > >> int ret; > >> > >> - if (nor->spimem) { > >> - struct spi_mem_op op = > >> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), > >> - SPI_MEM_OP_NO_ADDR, > >> - SPI_MEM_OP_NO_DUMMY, > >> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1)); > >> - > >> - ret = spi_mem_exec_op(nor->spimem, &op); > >> - } else { > >> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, > >> - SPI_NOR_MAX_ID_LEN); > >> - } > >> - if (ret) { > >> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); > >> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto); > > > > Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so > > clearly signify that this is intended to use 1S-1S-1S only. What do you > > think? > > I would keep it as it is for now, because it offers flexibility. > If we ever gonna determine the protocol at runtime this will come in handy > because it will work without touching the code. JESD216 suggests an algorithm > that tries to determine the mode depending on the SFDP signature. I was thinking exactly this but came to the opposite conclusion ;-). I think this would imply that other protocols can be used to detect the flash which is not true. But I have no strong preferences here. Either is fine by me.
Am 2022-02-28 12:17, schrieb Tudor Ambarus: > RDID is used in the core to auto detect the flash, but also by some > manufacturer drivers that contain flashes that support Octal DTR mode, > so that they can read the flash ID after the switch to Octal DTR was > made > to test if the switch was successful. Introduce a core method for RDID > op > to avoid code duplication. Some or all? Is that specific to the flash or can we just check that readid works in spi_nor_octal_dtr_enable()? That way we could also just get rid of the proto parameter for the read_id because it can be called after we set the reg_proto. -michael > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------ > drivers/mtd/spi-nor/core.h | 9 ++++++ > 2 files changed, 49 insertions(+), 18 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index b1d6fa65417d..281e3d25f74c 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor) > return ret; > } > > +/** > + * spi_nor_read_id() - Read the JEDEC ID. > + * @nor: pointer to 'struct spi_nor'. > + * @naddr: number of address bytes to send. Can be zero if the > operation > + * does not need to send an address. > + * @ndummy: number of dummy bytes to send after an opcode or address. > Can > + * be zero if the operation does not require dummy bytes. > + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID > + * will be written. > + * @reg_proto: the SPI protocol for register operation. > + * > + * Return: 0 on success, -errno otherwise. > + */ > +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > + enum spi_nor_protocol reg_proto) > +{ > + int ret; > + > + if (nor->spimem) { > + struct spi_mem_op op = > + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN); > + > + spi_nor_spimem_setup_op(nor, &op, reg_proto); > + ret = spi_mem_exec_op(nor->spimem, &op); > + } else { > + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, > + SPI_NOR_MAX_ID_LEN); > + } > + > + if (ret) > + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); > + > + return ret; > +} > + > /** > * spi_nor_read_sr() - Read the Status Register. > * @nor: pointer to 'struct spi_nor'. > @@ -1649,28 +1684,15 @@ static const struct flash_info > *spi_nor_match_id(struct spi_nor *nor, > return NULL; > } > > -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) > +static const struct flash_info *spi_nor_detect(struct spi_nor *nor) > { > const struct flash_info *info; > u8 *id = nor->bouncebuf; > int ret; > > - if (nor->spimem) { > - struct spi_mem_op op = > - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), > - SPI_MEM_OP_NO_ADDR, > - SPI_MEM_OP_NO_DUMMY, > - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1)); > - > - ret = spi_mem_exec_op(nor->spimem, &op); > - } else { > - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, > - SPI_NOR_MAX_ID_LEN); > - } > - if (ret) { > - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); > + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto); > + if (ret) > return ERR_PTR(ret); > - } > > info = spi_nor_match_id(nor, id); > if (!info) { > @@ -2900,7 +2922,7 @@ static const struct flash_info > *spi_nor_get_flash_info(struct spi_nor *nor, > info = spi_nor_match_name(nor, name); > /* Try to auto-detect if chip name wasn't specified or not found */ > if (!info) { > - detected_info = spi_nor_read_id(nor); > + detected_info = spi_nor_detect(nor); > info = detected_info; > } > if (IS_ERR_OR_NULL(info)) > @@ -2913,7 +2935,7 @@ static const struct flash_info > *spi_nor_get_flash_info(struct spi_nor *nor, > if (name && !detected_info && info->id_len) { > const struct flash_info *jinfo; > > - jinfo = spi_nor_read_id(nor); > + jinfo = spi_nor_detect(nor); > if (IS_ERR(jinfo)) { > return jinfo; > } else if (jinfo != info) { > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > index b7fd760e3b47..f952061d5c24 100644 > --- a/drivers/mtd/spi-nor/core.h > +++ b/drivers/mtd/spi-nor/core.h > @@ -11,6 +11,13 @@ > > #define SPI_NOR_MAX_ID_LEN 6 > > +/* Standard SPI NOR flash operations. */ > +#define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \ > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0), \ > + SPI_MEM_OP_ADDR(naddr, 0, 0), \ > + SPI_MEM_OP_DUMMY(ndummy, 0), \ > + SPI_MEM_OP_DATA_IN(len, buf, 0)) > + > enum spi_nor_option_flags { > SNOR_F_HAS_SR_TB = BIT(0), > SNOR_F_NO_OP_CHIP_ERASE = BIT(1), > @@ -534,6 +541,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor > *nor); > int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor); > int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor); > int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor); > +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > + enum spi_nor_protocol reg_proto); > int spi_nor_read_sr(struct spi_nor *nor, u8 *sr); > int spi_nor_sr_ready(struct spi_nor *nor); > int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
On 21/03/22 11:56PM, Michael Walle wrote: > Am 2022-02-28 12:17, schrieb Tudor Ambarus: > > RDID is used in the core to auto detect the flash, but also by some > > manufacturer drivers that contain flashes that support Octal DTR mode, > > so that they can read the flash ID after the switch to Octal DTR was > > made > > to test if the switch was successful. Introduce a core method for RDID > > op > > to avoid code duplication. > > Some or all? Is that specific to the flash or can we just check that > readid works in spi_nor_octal_dtr_enable()? That way we could also > just get rid of the proto parameter for the read_id because it can > be called after we set the reg_proto. It is specific to the flash. Not all flashes support RDID in 8D mode. And the RDID command is also different in 8D mode for various flashes. For example, Micron MT35XU512ABA flash expects 8 dummy cycles and 0 address cycles. Cypress S28HS512T expects 4 address cycles and 3 dummy cycles. The octal_dtr_enable hook would know what parameters to use but it is harder for the core to know since this information is not discoverable via SFDP. > > -michael > > > > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > > --- > > drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------ > > drivers/mtd/spi-nor/core.h | 9 ++++++ > > 2 files changed, 49 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > index b1d6fa65417d..281e3d25f74c 100644 > > --- a/drivers/mtd/spi-nor/core.c > > +++ b/drivers/mtd/spi-nor/core.c > > @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor) > > return ret; > > } > > > > +/** > > + * spi_nor_read_id() - Read the JEDEC ID. > > + * @nor: pointer to 'struct spi_nor'. > > + * @naddr: number of address bytes to send. Can be zero if the > > operation > > + * does not need to send an address. > > + * @ndummy: number of dummy bytes to send after an opcode or address. > > Can > > + * be zero if the operation does not require dummy bytes. > > + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID > > + * will be written. > > + * @reg_proto: the SPI protocol for register operation. > > + * > > + * Return: 0 on success, -errno otherwise. > > + */ > > +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > > + enum spi_nor_protocol reg_proto) > > +{ > > + int ret; > > + > > + if (nor->spimem) { > > + struct spi_mem_op op = > > + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN); > > + > > + spi_nor_spimem_setup_op(nor, &op, reg_proto); > > + ret = spi_mem_exec_op(nor->spimem, &op); > > + } else { > > + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, > > + SPI_NOR_MAX_ID_LEN); > > + } > > + > > + if (ret) > > + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); > > + > > + return ret; > > +} > > + > > /** > > * spi_nor_read_sr() - Read the Status Register. > > * @nor: pointer to 'struct spi_nor'. > > @@ -1649,28 +1684,15 @@ static const struct flash_info > > *spi_nor_match_id(struct spi_nor *nor, > > return NULL; > > } > > > > -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) > > +static const struct flash_info *spi_nor_detect(struct spi_nor *nor) > > { > > const struct flash_info *info; > > u8 *id = nor->bouncebuf; > > int ret; > > > > - if (nor->spimem) { > > - struct spi_mem_op op = > > - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), > > - SPI_MEM_OP_NO_ADDR, > > - SPI_MEM_OP_NO_DUMMY, > > - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1)); > > - > > - ret = spi_mem_exec_op(nor->spimem, &op); > > - } else { > > - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, > > - SPI_NOR_MAX_ID_LEN); > > - } > > - if (ret) { > > - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); > > + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto); > > + if (ret) > > return ERR_PTR(ret); > > - } > > > > info = spi_nor_match_id(nor, id); > > if (!info) { > > @@ -2900,7 +2922,7 @@ static const struct flash_info > > *spi_nor_get_flash_info(struct spi_nor *nor, > > info = spi_nor_match_name(nor, name); > > /* Try to auto-detect if chip name wasn't specified or not found */ > > if (!info) { > > - detected_info = spi_nor_read_id(nor); > > + detected_info = spi_nor_detect(nor); > > info = detected_info; > > } > > if (IS_ERR_OR_NULL(info)) > > @@ -2913,7 +2935,7 @@ static const struct flash_info > > *spi_nor_get_flash_info(struct spi_nor *nor, > > if (name && !detected_info && info->id_len) { > > const struct flash_info *jinfo; > > > > - jinfo = spi_nor_read_id(nor); > > + jinfo = spi_nor_detect(nor); > > if (IS_ERR(jinfo)) { > > return jinfo; > > } else if (jinfo != info) { > > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h > > index b7fd760e3b47..f952061d5c24 100644 > > --- a/drivers/mtd/spi-nor/core.h > > +++ b/drivers/mtd/spi-nor/core.h > > @@ -11,6 +11,13 @@ > > > > #define SPI_NOR_MAX_ID_LEN 6 > > > > +/* Standard SPI NOR flash operations. */ > > +#define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \ > > + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0), \ > > + SPI_MEM_OP_ADDR(naddr, 0, 0), \ > > + SPI_MEM_OP_DUMMY(ndummy, 0), \ > > + SPI_MEM_OP_DATA_IN(len, buf, 0)) > > + > > enum spi_nor_option_flags { > > SNOR_F_HAS_SR_TB = BIT(0), > > SNOR_F_NO_OP_CHIP_ERASE = BIT(1), > > @@ -534,6 +541,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor); > > int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor); > > int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor); > > int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor); > > +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > > + enum spi_nor_protocol reg_proto); > > int spi_nor_read_sr(struct spi_nor *nor, u8 *sr); > > int spi_nor_sr_ready(struct spi_nor *nor); > > int spi_nor_read_cr(struct spi_nor *nor, u8 *cr); > > -- > -michael
Am 2022-03-22 08:32, schrieb Pratyush Yadav: > On 21/03/22 11:56PM, Michael Walle wrote: >> Am 2022-02-28 12:17, schrieb Tudor Ambarus: >> > RDID is used in the core to auto detect the flash, but also by some >> > manufacturer drivers that contain flashes that support Octal DTR mode, >> > so that they can read the flash ID after the switch to Octal DTR was >> > made >> > to test if the switch was successful. Introduce a core method for RDID >> > op >> > to avoid code duplication. >> >> Some or all? Is that specific to the flash or can we just check that >> readid works in spi_nor_octal_dtr_enable()? That way we could also >> just get rid of the proto parameter for the read_id because it can >> be called after we set the reg_proto. > > It is specific to the flash. Not all flashes support RDID in 8D mode. > And the RDID command is also different in 8D mode for various flashes. > For example, Micron MT35XU512ABA flash expects 8 dummy cycles and 0 > address cycles. Cypress S28HS512T expects 4 address cycles and 3 dummy > cycles. > > The octal_dtr_enable hook would know what parameters to use but it is > harder for the core to know since this information is not discoverable > via SFDP. Ah, I see, thanks for clarification. -michael
On 3/21/22 19:39, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 21/03/22 01:18PM, Tudor.Ambarus@microchip.com wrote: >> On 3/21/22 14:21, Pratyush Yadav wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On 28/02/22 01:17PM, Tudor Ambarus wrote: >>>> RDID is used in the core to auto detect the flash, but also by some >>>> manufacturer drivers that contain flashes that support Octal DTR mode, >>>> so that they can read the flash ID after the switch to Octal DTR was made >>>> to test if the switch was successful. Introduce a core method for RDID op >>>> to avoid code duplication. >>>> >>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >>>> --- >>>> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------ >>>> drivers/mtd/spi-nor/core.h | 9 ++++++ >>>> 2 files changed, 49 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>>> index b1d6fa65417d..281e3d25f74c 100644 >>>> --- a/drivers/mtd/spi-nor/core.c >>>> +++ b/drivers/mtd/spi-nor/core.c >>>> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor) >>>> return ret; >>>> } >>>> >>>> +/** >>>> + * spi_nor_read_id() - Read the JEDEC ID. >>>> + * @nor: pointer to 'struct spi_nor'. >>>> + * @naddr: number of address bytes to send. Can be zero if the operation >>>> + * does not need to send an address. >>>> + * @ndummy: number of dummy bytes to send after an opcode or address. Can >>>> + * be zero if the operation does not require dummy bytes. >>>> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID >>>> + * will be written. >>>> + * @reg_proto: the SPI protocol for register operation. >>>> + * >>>> + * Return: 0 on success, -errno otherwise. >>>> + */ >>>> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, >>>> + enum spi_nor_protocol reg_proto) >>> >>> Nitpick: Could just call it 'proto'. >> >> sure, will update >> >>> >>>> +{ >>>> + int ret; >>>> + >>>> + if (nor->spimem) { >>>> + struct spi_mem_op op = >>>> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN); >>>> + >>>> + spi_nor_spimem_setup_op(nor, &op, reg_proto); >>>> + ret = spi_mem_exec_op(nor->spimem, &op); >>>> + } else { >>>> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, >>>> + SPI_NOR_MAX_ID_LEN); >>>> + } >>>> + >>>> + if (ret) >>>> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); >>> >>> I think this message should be in spi_nor_detect(). Let octal DTR enable >> >> As of now every SPI NOR operation that return an error also prints a dbg >> message. I like this because it offers a smaller granularity on the error >> cause. > > Yes, but I think this message would be misleading. If someone sees > "error reading JEDEC ID", they would think flash detection itself has > failed, not that we failed to switch to Octal DTR mode. > >> >>> methods print their own, more specific error messages. >> >> How about duplicating the error in the octal dtr enable methods if you >> feel it is worth it? > > They should at the very least explain that reading ID failed _after_ > attempting to switch to Octal DTR. But I think it would just be simpler > if this is not printed here and the caller has the flexibility to > explain the error. If the first readID fails, the one that identifies the flash, then the octal dtr will not be run, thus a single error message. When octal dtr fails, 2 errors can be printed, one specifying what failed (the read ID command) and the second where it failed (at the octal dtr enable method). But I don't care too much, I'll follow your suggestion. > >> >>> >>>> + >>>> + return ret; >>>> +} >>>> + >>>> /** >>>> * spi_nor_read_sr() - Read the Status Register. >>>> * @nor: pointer to 'struct spi_nor'. >>>> @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor, >>>> return NULL; >>>> } >>>> >>>> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) >>>> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor) >>>> { >>>> const struct flash_info *info; >>>> u8 *id = nor->bouncebuf; >>>> int ret; >>>> >>>> - if (nor->spimem) { >>>> - struct spi_mem_op op = >>>> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), >>>> - SPI_MEM_OP_NO_ADDR, >>>> - SPI_MEM_OP_NO_DUMMY, >>>> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1)); >>>> - >>>> - ret = spi_mem_exec_op(nor->spimem, &op); >>>> - } else { >>>> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, >>>> - SPI_NOR_MAX_ID_LEN); >>>> - } >>>> - if (ret) { >>>> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); >>>> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto); >>> >>> Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so >>> clearly signify that this is intended to use 1S-1S-1S only. What do you >>> think? >> >> I would keep it as it is for now, because it offers flexibility. >> If we ever gonna determine the protocol at runtime this will come in handy >> because it will work without touching the code. JESD216 suggests an algorithm >> that tries to determine the mode depending on the SFDP signature. > > I was thinking exactly this but came to the opposite conclusion ;-). I > think this would imply that other protocols can be used to detect the > flash which is not true. It can become true. As you already specified 8d-8d-8d is supported by some flashes and we can implement hooks for their specific 8d-8d-8d readID command. The logic will complicate a bit as one has to adjust the hwcaps before issuing the 8d-8d-8d readID, but it's doable. Otherwise, if the bootloaders pass you the flash in octal dtr mode, you'll have to disable it, issue readID is 1-1-1 and then re-enable it. > > But I have no strong preferences here. Either is fine by me. I don't have strong preferences either, but it seems that there's room for discussion on this, so I would keep it for later. Is that fine? I can add a comment if you prefer, specifying that at this point nor->reg_proto is in 1-1-1 mode. Cheers, ta
On 30/03/22 06:53AM, Tudor.Ambarus@microchip.com wrote: > On 3/21/22 19:39, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On 21/03/22 01:18PM, Tudor.Ambarus@microchip.com wrote: > >> On 3/21/22 14:21, Pratyush Yadav wrote: > >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>> > >>> On 28/02/22 01:17PM, Tudor Ambarus wrote: > >>>> RDID is used in the core to auto detect the flash, but also by some > >>>> manufacturer drivers that contain flashes that support Octal DTR mode, > >>>> so that they can read the flash ID after the switch to Octal DTR was made > >>>> to test if the switch was successful. Introduce a core method for RDID op > >>>> to avoid code duplication. > >>>> > >>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > >>>> --- > >>>> drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------ > >>>> drivers/mtd/spi-nor/core.h | 9 ++++++ > >>>> 2 files changed, 49 insertions(+), 18 deletions(-) > >>>> > >>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > >>>> index b1d6fa65417d..281e3d25f74c 100644 > >>>> --- a/drivers/mtd/spi-nor/core.c > >>>> +++ b/drivers/mtd/spi-nor/core.c > >>>> @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor) > >>>> return ret; > >>>> } > >>>> > >>>> +/** > >>>> + * spi_nor_read_id() - Read the JEDEC ID. > >>>> + * @nor: pointer to 'struct spi_nor'. > >>>> + * @naddr: number of address bytes to send. Can be zero if the operation > >>>> + * does not need to send an address. > >>>> + * @ndummy: number of dummy bytes to send after an opcode or address. Can > >>>> + * be zero if the operation does not require dummy bytes. > >>>> + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID > >>>> + * will be written. > >>>> + * @reg_proto: the SPI protocol for register operation. > >>>> + * > >>>> + * Return: 0 on success, -errno otherwise. > >>>> + */ > >>>> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, > >>>> + enum spi_nor_protocol reg_proto) > >>> > >>> Nitpick: Could just call it 'proto'. > >> > >> sure, will update > >> > >>> > >>>> +{ > >>>> + int ret; > >>>> + > >>>> + if (nor->spimem) { > >>>> + struct spi_mem_op op = > >>>> + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN); > >>>> + > >>>> + spi_nor_spimem_setup_op(nor, &op, reg_proto); > >>>> + ret = spi_mem_exec_op(nor->spimem, &op); > >>>> + } else { > >>>> + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, > >>>> + SPI_NOR_MAX_ID_LEN); > >>>> + } > >>>> + > >>>> + if (ret) > >>>> + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); > >>> > >>> I think this message should be in spi_nor_detect(). Let octal DTR enable > >> > >> As of now every SPI NOR operation that return an error also prints a dbg > >> message. I like this because it offers a smaller granularity on the error > >> cause. > > > > Yes, but I think this message would be misleading. If someone sees > > "error reading JEDEC ID", they would think flash detection itself has > > failed, not that we failed to switch to Octal DTR mode. > > > >> > >>> methods print their own, more specific error messages. > >> > >> How about duplicating the error in the octal dtr enable methods if you > >> feel it is worth it? > > > > They should at the very least explain that reading ID failed _after_ > > attempting to switch to Octal DTR. But I think it would just be simpler > > if this is not printed here and the caller has the flexibility to > > explain the error. > > If the first readID fails, the one that identifies the flash, then the > octal dtr will not be run, thus a single error message. When octal dtr > fails, 2 errors can be printed, one specifying what failed (the read ID > command) and the second where it failed (at the octal dtr enable method). > But I don't care too much, I'll follow your suggestion. > > > > >> > >>> > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> /** > >>>> * spi_nor_read_sr() - Read the Status Register. > >>>> * @nor: pointer to 'struct spi_nor'. > >>>> @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor, > >>>> return NULL; > >>>> } > >>>> > >>>> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) > >>>> +static const struct flash_info *spi_nor_detect(struct spi_nor *nor) > >>>> { > >>>> const struct flash_info *info; > >>>> u8 *id = nor->bouncebuf; > >>>> int ret; > >>>> > >>>> - if (nor->spimem) { > >>>> - struct spi_mem_op op = > >>>> - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), > >>>> - SPI_MEM_OP_NO_ADDR, > >>>> - SPI_MEM_OP_NO_DUMMY, > >>>> - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1)); > >>>> - > >>>> - ret = spi_mem_exec_op(nor->spimem, &op); > >>>> - } else { > >>>> - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, > >>>> - SPI_NOR_MAX_ID_LEN); > >>>> - } > >>>> - if (ret) { > >>>> - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); > >>>> + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto); > >>> > >>> Hmm, I wonder if it is better to explicitly use SNOR_PROTO_1_1_1 so > >>> clearly signify that this is intended to use 1S-1S-1S only. What do you > >>> think? > >> > >> I would keep it as it is for now, because it offers flexibility. > >> If we ever gonna determine the protocol at runtime this will come in handy > >> because it will work without touching the code. JESD216 suggests an algorithm > >> that tries to determine the mode depending on the SFDP signature. > > > > I was thinking exactly this but came to the opposite conclusion ;-). I > > think this would imply that other protocols can be used to detect the > > flash which is not true. > > It can become true. As you already specified 8d-8d-8d is supported by some flashes > and we can implement hooks for their specific 8d-8d-8d readID command. The logic > will complicate a bit as one has to adjust the hwcaps before issuing the 8d-8d-8d > readID, but it's doable. Otherwise, if the bootloaders pass you the flash in octal > dtr mode, you'll have to disable it, issue readID is 1-1-1 and then re-enable it. Right. > > > > > But I have no strong preferences here. Either is fine by me. > > I don't have strong preferences either, but it seems that there's room for discussion > on this, so I would keep it for later. Is that fine? Fine by me. It should be fine without the comment. It is not too hard to see what nor->reg_proto is initialized to. > I can add a comment if you prefer, specifying that at this point nor->reg_proto is in > 1-1-1 mode.
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index b1d6fa65417d..281e3d25f74c 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -369,6 +369,41 @@ int spi_nor_write_disable(struct spi_nor *nor) return ret; } +/** + * spi_nor_read_id() - Read the JEDEC ID. + * @nor: pointer to 'struct spi_nor'. + * @naddr: number of address bytes to send. Can be zero if the operation + * does not need to send an address. + * @ndummy: number of dummy bytes to send after an opcode or address. Can + * be zero if the operation does not require dummy bytes. + * @id: pointer to a DMA-able buffer where the value of the JEDEC ID + * will be written. + * @reg_proto: the SPI protocol for register operation. + * + * Return: 0 on success, -errno otherwise. + */ +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, + enum spi_nor_protocol reg_proto) +{ + int ret; + + if (nor->spimem) { + struct spi_mem_op op = + SPI_NOR_READID_OP(naddr, ndummy, id, SPI_NOR_MAX_ID_LEN); + + spi_nor_spimem_setup_op(nor, &op, reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); + } else { + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, + SPI_NOR_MAX_ID_LEN); + } + + if (ret) + dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); + + return ret; +} + /** * spi_nor_read_sr() - Read the Status Register. * @nor: pointer to 'struct spi_nor'. @@ -1649,28 +1684,15 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor, return NULL; } -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor) +static const struct flash_info *spi_nor_detect(struct spi_nor *nor) { const struct flash_info *info; u8 *id = nor->bouncebuf; int ret; - if (nor->spimem) { - struct spi_mem_op op = - SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1), - SPI_MEM_OP_NO_ADDR, - SPI_MEM_OP_NO_DUMMY, - SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1)); - - ret = spi_mem_exec_op(nor->spimem, &op); - } else { - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDID, id, - SPI_NOR_MAX_ID_LEN); - } - if (ret) { - dev_dbg(nor->dev, "error %d reading JEDEC ID\n", ret); + ret = spi_nor_read_id(nor, 0, 0, id, nor->reg_proto); + if (ret) return ERR_PTR(ret); - } info = spi_nor_match_id(nor, id); if (!info) { @@ -2900,7 +2922,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, info = spi_nor_match_name(nor, name); /* Try to auto-detect if chip name wasn't specified or not found */ if (!info) { - detected_info = spi_nor_read_id(nor); + detected_info = spi_nor_detect(nor); info = detected_info; } if (IS_ERR_OR_NULL(info)) @@ -2913,7 +2935,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor, if (name && !detected_info && info->id_len) { const struct flash_info *jinfo; - jinfo = spi_nor_read_id(nor); + jinfo = spi_nor_detect(nor); if (IS_ERR(jinfo)) { return jinfo; } else if (jinfo != info) { diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index b7fd760e3b47..f952061d5c24 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -11,6 +11,13 @@ #define SPI_NOR_MAX_ID_LEN 6 +/* Standard SPI NOR flash operations. */ +#define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \ + SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0), \ + SPI_MEM_OP_ADDR(naddr, 0, 0), \ + SPI_MEM_OP_DUMMY(ndummy, 0), \ + SPI_MEM_OP_DATA_IN(len, buf, 0)) + enum spi_nor_option_flags { SNOR_F_HAS_SR_TB = BIT(0), SNOR_F_NO_OP_CHIP_ERASE = BIT(1), @@ -534,6 +541,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor); int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor); int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor); int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor); +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id, + enum spi_nor_protocol reg_proto); int spi_nor_read_sr(struct spi_nor *nor, u8 *sr); int spi_nor_sr_ready(struct spi_nor *nor); int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
RDID is used in the core to auto detect the flash, but also by some manufacturer drivers that contain flashes that support Octal DTR mode, so that they can read the flash ID after the switch to Octal DTR was made to test if the switch was successful. Introduce a core method for RDID op to avoid code duplication. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 58 ++++++++++++++++++++++++++------------ drivers/mtd/spi-nor/core.h | 9 ++++++ 2 files changed, 49 insertions(+), 18 deletions(-)