| Message ID | 20200623183030.26591-6-p.yadav@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mtd: spi-nor: add xSPI Octal DTR support | expand |
Hi, Pratyush, On 6/23/20 9:30 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Double Transfer Rate (DTR) is SPI protocol in which data is transferred > on each clock edge as opposed to on each clock cycle. Make > framework-level changes to allow supporting flashes in DTR mode. > > Right now, mixed DTR modes are not supported. So, for example a mode > like 4S-4D-4D will not work. All phases need to be either DTR or STR. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/mtd/spi-nor/core.c | 305 ++++++++++++++++++++++++++++-------- > drivers/mtd/spi-nor/core.h | 6 + > drivers/mtd/spi-nor/sfdp.c | 9 +- > include/linux/mtd/spi-nor.h | 51 ++++-- > 4 files changed, 295 insertions(+), 76 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 0369d98b2d12..22a3832b83a6 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -40,6 +40,76 @@ > > #define SPI_NOR_MAX_ADDR_WIDTH 4 > > +/** > + * spi_nor_get_cmd_ext() - Get the command opcode extension based on the > + * extension type. > + * @nor: pointer to a 'struct spi_nor' > + * @op: pointer to the 'struct spi_mem_op' whose properties > + * need to be initialized. > + * > + * Right now, only "repeat" and "invert" are supported. > + * > + * Return: The opcode extension. > + */ > +static u8 spi_nor_get_cmd_ext(const struct spi_nor *nor, > + const struct spi_mem_op *op) > +{ > + switch (nor->cmd_ext_type) { > + case SPI_NOR_EXT_INVERT: > + return ~op->cmd.opcode; > + > + case SPI_NOR_EXT_REPEAT: > + return op->cmd.opcode; > + > + default: > + dev_err(nor->dev, "Unknown command extension type\n"); > + return 0; > + } > +} > + > +/** > + * spi_nor_spimem_setup_op() - Set up common properties of a spi-mem op. > + * @nor: pointer to a 'struct spi_nor' > + * @op: pointer to the 'struct spi_mem_op' whose properties > + * need to be initialized. > + * @proto: the protocol from which the properties need to be set. > + */ > +void spi_nor_spimem_setup_op(const struct spi_nor *nor, > + struct spi_mem_op *op, > + const enum spi_nor_protocol proto) There's not much to set for the REG operations. > +{ > + u8 ext; > + > + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(proto); > + > + if (op->addr.nbytes) > + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto); > + > + if (op->dummy.nbytes) > + op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto); > + > + if (op->data.nbytes) > + op->data.buswidth = spi_nor_get_protocol_data_nbits(proto); How about getting rid of the above and > + > + if (spi_nor_protocol_is_dtr(proto)) { introduce a spi_nor_spimem_setup_dtr_op() just for the body of this if? > + /* > + * spi-mem supports mixed DTR modes, but right now we can only > + * have all phases either DTR or STR. IOW, spi-mem can have nit: SPIMEM > + * something like 4S-4D-4D, but spi-nor can't. So, set all 4 nit: SPI NOR > + * phases to either DTR or STR. > + */ > + op->cmd.dtr = op->addr.dtr = op->dummy.dtr > + = op->data.dtr = true; > + > + /* 2 bytes per clock cycle in DTR mode. */ > + op->dummy.nbytes *= 2; > + > + ext = spi_nor_get_cmd_ext(nor, op); > + op->cmd.opcode = (op->cmd.opcode << 8) | ext; > + op->cmd.nbytes = 2; > + } > +} > + > /** > * spi_nor_spimem_bounce() - check if a bounce buffer is needed for the data > * transfer > @@ -104,14 +174,12 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from, > ssize_t nbytes; > int error; > > - /* get transfer protocols. */ > - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); > - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); > - op.dummy.buswidth = op.addr.buswidth; > - op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); > + spi_nor_spimem_setup_op(nor, &op, nor->read_proto); Here we would keep the code as it were. > > /* convert the dummy cycles to the number of bytes */ > op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; > + if (spi_nor_protocol_is_dtr(nor->read_proto)) > + op.dummy.nbytes *= 2; And replace these 2 lines with: if (spi_nor_protocol_is_dtr(nor->read_proto)) spi_nor_spimem_setup_dtr_op(nor, &op, nor->read_proto) > > usebouncebuf = spi_nor_spimem_bounce(nor, &op); > > @@ -169,13 +237,11 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to, > ssize_t nbytes; > int error; > > - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); > - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); > - op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); > - > if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second) > op.addr.nbytes = 0; > > + spi_nor_spimem_setup_op(nor, &op, nor->write_proto); > + > if (spi_nor_spimem_bounce(nor, &op)) > memcpy(nor->bouncebuf, buf, op.data.nbytes); > > @@ -227,10 +293,16 @@ int spi_nor_write_enable(struct spi_nor *nor) > SPI_MEM_OP_NO_DUMMY, > SPI_MEM_OP_NO_DATA); > > + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); For the reg operation we can get rid of the extra checks that were in spi_nor_spimem_setup_op and simply do: if (spi_nor_protocol_is_dtr(proto)) spi_nor_spimem_setup_dtr_op() > + > ret = spi_mem_exec_op(nor->spimem, &op); > } else { > - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREN, > - NULL, 0); > + if (spi_nor_protocol_is_dtr(nor->reg_proto)) > + ret = -ENOTSUPP; > + else > + ret = nor->controller_ops->write_reg(nor, > + SPINOR_OP_WREN, > + NULL, 0); Would you introduce helpers for the controller ops, like Boris did in the following patch? https://patchwork.ozlabs.org/project/linux-mtd/patch/20181012084825.23697-10-boris.brezillon@bootlin.com/ How about spi_nor_controller_ops_read_reg() and spi_nor_controller_ops_write_reg() instead? cut > @@ -1144,7 +1291,11 @@ static 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); > + > return spi_mem_exec_op(nor->spimem, &op); > + } else if (spi_nor_protocol_is_dtr(nor->write_proto)) { > + return -ENOTSUPP; > } else if (nor->controller_ops->erase) { > return nor->controller_ops->erase(nor, addr); > } here you would need a helper: spi_nor_controller_ops_erase() cut > @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps) > struct spi_nor_flash_parameter *params = nor->params; > unsigned int cap; > > - /* DTR modes are not supported yet, mask them all. */ > - *hwcaps &= ~SNOR_HWCAPS_DTR; > - > /* X-X-X modes are not supported yet, mask them all. */ > *hwcaps &= ~SNOR_HWCAPS_X_X_X; > > + /* > + * If the reset line is broken, we do not want to enter a stateful > + * mode. > + */ > + if (nor->flags & SNOR_F_BROKEN_RESET) > + *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR); A dedicated reset line is not enough for flashes that keep their state in non-volatile bits. Since we can't protect from unexpected crashes in the non volatile state case, we should enter these modes only with an explicit request, i.e. an optional DT property: "update-nonvolatile-state", or something similar. For the volatile state case, we can parse the SFDP SCCR map, save if we can enter stateful modes in a volatile way, and if yes allow the entering. Do the flashes that you played with define the SFDP SCCR map? > + > for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) { > int rdidx, ppidx; > > @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor, > * controller directly implements the spi_nor interface. > * Yet another reason to switch to spi-mem. > */ > - ignored_mask = SNOR_HWCAPS_X_X_X; > + ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR; > if (shared_mask & ignored_mask) { > dev_dbg(nor->dev, > "SPI n-n-n protocols are not supported.\n"); > @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor) > SNOR_PROTO_1_1_8); > } > > + if (info->flags & SPI_NOR_OCTAL_DTR_READ) { Why do we need this flag? Can't we determine if the flash supports octal DTR by parsing SFDP? > + params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR; > + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_8_8_8_DTR], > + 0, 20, SPINOR_OP_READ_FAST, > + SNOR_PROTO_8_8_8_DTR); > + } > + > /* Page Program settings. */ > params->hwcaps.mask |= SNOR_HWCAPS_PP; > spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], > SPINOR_OP_PP, SNOR_PROTO_1_1_1); > > + /* > + * Since xSPI Page Program opcode is backward compatible with > + * Legacy SPI, use Legacy SPI opcode there as well. > + */ > + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP_8_8_8_DTR], > + SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR); > + This looks fishy. You haven't updated the hwcaps.mask, these pp settings never get selected? > /* > * Sector Erase settings. Sort Erase Types in ascending order, with the > * smallest erase size starting at BIT(0). > @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor) > > spi_nor_manufacturer_init_params(nor); > > - if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) && > + if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > + SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && > !(nor->info->flags & SPI_NOR_SKIP_SFDP)) > spi_nor_sfdp_init_params(nor); > > @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor) > return err; > } > > - if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) { > + if (nor->addr_width == 4 && > + !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) && Why is the Octal DTR read exempted? > + !(nor->flags & SNOR_F_4B_OPCODES)) { > /* > * If the RESET# pin isn't hooked up properly, or the system > * otherwise doesn't perform a reset command in the boot > @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor) > { > if (nor->addr_width) { > /* already configured from SFDP */ > + } else if (spi_nor_protocol_is_dtr(nor->read_proto)) { > + /* Always use 4-byte addresses in DTR mode. */ > + nor->addr_width = 4; Why? DTR with 3 byte addr width should be possible too. > } else if (nor->info->addr_width) { > nor->addr_width = nor->info->addr_width; > } else if (nor->mtd.size > 0x1000000) { Cheers, ta
Hi Tudor, On 07/07/20 05:37PM, Tudor.Ambarus@microchip.com wrote: > Hi, Pratyush, > > On 6/23/20 9:30 PM, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Double Transfer Rate (DTR) is SPI protocol in which data is transferred > > on each clock edge as opposed to on each clock cycle. Make > > framework-level changes to allow supporting flashes in DTR mode. > > > > Right now, mixed DTR modes are not supported. So, for example a mode > > like 4S-4D-4D will not work. All phases need to be either DTR or STR. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/mtd/spi-nor/core.c | 305 ++++++++++++++++++++++++++++-------- > > drivers/mtd/spi-nor/core.h | 6 + > > drivers/mtd/spi-nor/sfdp.c | 9 +- > > include/linux/mtd/spi-nor.h | 51 ++++-- > > 4 files changed, 295 insertions(+), 76 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > index 0369d98b2d12..22a3832b83a6 100644 > > --- a/drivers/mtd/spi-nor/core.c > > +++ b/drivers/mtd/spi-nor/core.c > > @@ -40,6 +40,76 @@ > > > > #define SPI_NOR_MAX_ADDR_WIDTH 4 > > > > +/** > > + * spi_nor_get_cmd_ext() - Get the command opcode extension based on the > > + * extension type. > > + * @nor: pointer to a 'struct spi_nor' > > + * @op: pointer to the 'struct spi_mem_op' whose properties > > + * need to be initialized. > > + * > > + * Right now, only "repeat" and "invert" are supported. > > + * > > + * Return: The opcode extension. > > + */ > > +static u8 spi_nor_get_cmd_ext(const struct spi_nor *nor, > > + const struct spi_mem_op *op) > > +{ > > + switch (nor->cmd_ext_type) { > > + case SPI_NOR_EXT_INVERT: > > + return ~op->cmd.opcode; > > + > > + case SPI_NOR_EXT_REPEAT: > > + return op->cmd.opcode; > > + > > + default: > > + dev_err(nor->dev, "Unknown command extension type\n"); > > + return 0; > > + } > > +} > > + > > +/** > > + * spi_nor_spimem_setup_op() - Set up common properties of a spi-mem op. > > + * @nor: pointer to a 'struct spi_nor' > > + * @op: pointer to the 'struct spi_mem_op' whose properties > > + * need to be initialized. > > + * @proto: the protocol from which the properties need to be set. > > + */ > > +void spi_nor_spimem_setup_op(const struct spi_nor *nor, > > + struct spi_mem_op *op, > > + const enum spi_nor_protocol proto) > > There's not much to set for the REG operations. > > > +{ > > + u8 ext; > > + > > + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(proto); > > + > > + if (op->addr.nbytes) > > + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto); > > + > > + if (op->dummy.nbytes) > > + op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto); > > + > > + if (op->data.nbytes) > > + op->data.buswidth = spi_nor_get_protocol_data_nbits(proto); > > How about getting rid of the above and > > > + > > + if (spi_nor_protocol_is_dtr(proto)) { > > introduce a spi_nor_spimem_setup_dtr_op() just for the body of this if? What benefit do we get with that other than skipping a couple of if() checks? The downside is that we would have to then replicate all this code to assign buswidth everywhere, including in spi_nor_read_sr() and spi_nor_read_fsr() adding to more boilerplate. If we change anything about spi-mem ops in the future we would then again have to hunt and peck all places where we create spi-mem ops and update them. For example, I was recently experimenting with a mechanism to tell controllers the maximum supported frequency for an op (xSPI says read SFDP should support at least 50MHz operation so we want to make sure controllers don't exceed that speed). A max speed of 0 would mean controllers can go as fast as they wish (how it is done currently). Having a central function to set up ops made it a 1 line change to set the speed to 0 for all ops, and then we can set it to 50MHz for read SFDP. The same thing without it would have me copying that line in 10-15 places. So unless there are any significant reasons to avoid having this, I think it is a good idea to keep it. > > + /* > > + * spi-mem supports mixed DTR modes, but right now we can only > > + * have all phases either DTR or STR. IOW, spi-mem can have > nit: SPIMEM > > + * something like 4S-4D-4D, but spi-nor can't. So, set all 4 > nit: SPI NOR > > + * phases to either DTR or STR. > > + */ > > + op->cmd.dtr = op->addr.dtr = op->dummy.dtr > > + = op->data.dtr = true; > > + > > + /* 2 bytes per clock cycle in DTR mode. */ > > + op->dummy.nbytes *= 2; > > + > > + ext = spi_nor_get_cmd_ext(nor, op); > > + op->cmd.opcode = (op->cmd.opcode << 8) | ext; > > + op->cmd.nbytes = 2; > > + } > > +} > > + > > /** > > * spi_nor_spimem_bounce() - check if a bounce buffer is needed for the data > > * transfer > > @@ -104,14 +174,12 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from, > > ssize_t nbytes; > > int error; > > > > - /* get transfer protocols. */ > > - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); > > - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); > > - op.dummy.buswidth = op.addr.buswidth; > > - op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); > > + spi_nor_spimem_setup_op(nor, &op, nor->read_proto); > > Here we would keep the code as it were. > > > > /* convert the dummy cycles to the number of bytes */ > > op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; > > + if (spi_nor_protocol_is_dtr(nor->read_proto)) > > + op.dummy.nbytes *= 2; > > And replace these 2 lines with: > if (spi_nor_protocol_is_dtr(nor->read_proto)) > spi_nor_spimem_setup_dtr_op(nor, &op, nor->read_proto) > > > > usebouncebuf = spi_nor_spimem_bounce(nor, &op); > > > > @@ -169,13 +237,11 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to, > > ssize_t nbytes; > > int error; > > > > - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); > > - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); > > - op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); > > - > > if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second) > > op.addr.nbytes = 0; > > > > + spi_nor_spimem_setup_op(nor, &op, nor->write_proto); > > + > > if (spi_nor_spimem_bounce(nor, &op)) > > memcpy(nor->bouncebuf, buf, op.data.nbytes); > > > > @@ -227,10 +293,16 @@ int spi_nor_write_enable(struct spi_nor *nor) > > SPI_MEM_OP_NO_DUMMY, > > SPI_MEM_OP_NO_DATA); > > > > + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); > > For the reg operation we can get rid of the extra checks that were in > spi_nor_spimem_setup_op and simply do: > > if (spi_nor_protocol_is_dtr(proto)) > spi_nor_spimem_setup_dtr_op() > > > + > > ret = spi_mem_exec_op(nor->spimem, &op); > > } else { > > - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREN, > > - NULL, 0); > > + if (spi_nor_protocol_is_dtr(nor->reg_proto)) > > + ret = -ENOTSUPP; > > + else > > + ret = nor->controller_ops->write_reg(nor, > > + SPINOR_OP_WREN, > > + NULL, 0); > > Would you introduce helpers for the controller ops, like Boris > did in the following patch? > https://patchwork.ozlabs.org/project/linux-mtd/patch/20181012084825.23697-10-boris.brezillon@bootlin.com/ > > How about spi_nor_controller_ops_read_reg() > and spi_nor_controller_ops_write_reg() instead? It would get rid of the boilerplate so I think it is a good idea. > cut > > > @@ -1144,7 +1291,11 @@ static 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); > > + > > return spi_mem_exec_op(nor->spimem, &op); > > + } else if (spi_nor_protocol_is_dtr(nor->write_proto)) { > > + return -ENOTSUPP; > > } else if (nor->controller_ops->erase) { > > return nor->controller_ops->erase(nor, addr); > > } > > here you would need a helper: spi_nor_controller_ops_erase() Ok. > cut > > > @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps) > > struct spi_nor_flash_parameter *params = nor->params; > > unsigned int cap; > > > > - /* DTR modes are not supported yet, mask them all. */ > > - *hwcaps &= ~SNOR_HWCAPS_DTR; > > - > > /* X-X-X modes are not supported yet, mask them all. */ > > *hwcaps &= ~SNOR_HWCAPS_X_X_X; > > > > + /* > > + * If the reset line is broken, we do not want to enter a stateful > > + * mode. > > + */ > > + if (nor->flags & SNOR_F_BROKEN_RESET) > > + *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR); > > A dedicated reset line is not enough for flashes that keep their state > in non-volatile bits. Since we can't protect from unexpected crashes in > the non volatile state case, we should enter these modes only with an > explicit request, i.e. an optional DT property: "update-nonvolatile-state", > or something similar. I wrote this patch with the assumption that we won't be supporting non-volatile configuration as of now. In the previous discussions we came to the conclusion that it is not easy to detect the flash if it boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile state, the flash would be useless after a reboot because we won't be able to detect it in 8D mode. It doesn't matter if the reset line is connected or not because it will reset the flash to the non-volatile state, and we can't detect it from the non-volatile state. > For the volatile state case, we can parse the SFDP SCCR map, save if we > can enter stateful modes in a volatile way, and if yes allow the entering. If we are not support volatile configurations, the reset line is enough to take care of unexpected resets, no? I don't see any need to parse SCCR map just for this. > Do the flashes that you played with define the SFDP SCCR map? FWIW, the Cypress S28HS512T flash does but the Micron MT35XU512ABA does not. > > + > > for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) { > > int rdidx, ppidx; > > > > @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor, > > * controller directly implements the spi_nor interface. > > * Yet another reason to switch to spi-mem. > > */ > > - ignored_mask = SNOR_HWCAPS_X_X_X; > > + ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR; > > if (shared_mask & ignored_mask) { > > dev_dbg(nor->dev, > > "SPI n-n-n protocols are not supported.\n"); > > @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor) > > SNOR_PROTO_1_1_8); > > } > > > > + if (info->flags & SPI_NOR_OCTAL_DTR_READ) { > > Why do we need this flag? Can't we determine if the flash supports > octal DTR by parsing SFDP? For Cypress S28HS512T, we can since it is xSPI compliant. We can't do that for Micron MT35XU512ABA since it is not xSPI compliant. > > + params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR; > > + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_8_8_8_DTR], > > + 0, 20, SPINOR_OP_READ_FAST, > > + SNOR_PROTO_8_8_8_DTR); > > + } > > + > > /* Page Program settings. */ > > params->hwcaps.mask |= SNOR_HWCAPS_PP; > > spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], > > SPINOR_OP_PP, SNOR_PROTO_1_1_1); > > > > + /* > > + * Since xSPI Page Program opcode is backward compatible with > > + * Legacy SPI, use Legacy SPI opcode there as well. > > + */ > > + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP_8_8_8_DTR], > > + SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR); > > + > > This looks fishy. You haven't updated the hwcaps.mask, these pp settings never > get selected? The problem here is that I don't see any field/table in SFDP that can tell us {if,which} 8D-8D-8D program commands are supported. The xSPI spec says that "The program commands provide SPI backward compatible commands for programming data...". So we populate the 8D page program opcodes here (and in 4bait parsing) using the 1S opcodes. The flashes have to enable the hwcap in fixup hooks. As an alternative, maybe we can introduce the SPI_NOR_OCTAL_DTR_PP flag that can enable the hwcap here? Thoughts? > > /* > > * Sector Erase settings. Sort Erase Types in ascending order, with the > > * smallest erase size starting at BIT(0). > > @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor) > > > > spi_nor_manufacturer_init_params(nor); > > > > - if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) && > > + if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > > + SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && > > !(nor->info->flags & SPI_NOR_SKIP_SFDP)) > > spi_nor_sfdp_init_params(nor); > > > > @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor) > > return err; > > } > > > > - if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) { > > + if (nor->addr_width == 4 && > > + !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) && > > Why is the Octal DTR read exempted? It is based on the assumption explained below that 8D mode will always use 4-byte addresses so we don't need to explicitly enable 8D mode. Although I think maybe we should exempt all flashes that support DTR mode? > > + !(nor->flags & SNOR_F_4B_OPCODES)) { > > /* > > * If the RESET# pin isn't hooked up properly, or the system > > * otherwise doesn't perform a reset command in the boot > > @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor) > > { > > if (nor->addr_width) { > > /* already configured from SFDP */ > > + } else if (spi_nor_protocol_is_dtr(nor->read_proto)) { > > + /* Always use 4-byte addresses in DTR mode. */ > > + nor->addr_width = 4; > > Why? DTR with 3 byte addr width should be possible too. Should it be? What would happen to the half cycle left over? Do we then start the dummy phase in the middle of the cycle? We would also have to start the data phase in the middle of a cycle as well and end the transaction with half a cycle left over. AFAIK, the controller I tested with (Cadence QSPI) does not support this. Similarly, the two flashes this series adds support for, Cypress S28HS512T and Micron MT35XU512ABA, don't support 3-byte address in 8D mode. I'm not sure if there are any flashes or controllers that do. > > } else if (nor->info->addr_width) { > > nor->addr_width = nor->info->addr_width; > > } else if (nor->mtd.size > 0x1000000) { > > Cheers, > ta [0] https://lore.kernel.org/linux-mtd/20200228093658.zc3uifqg4zruokq3@ti.com/ -- Regards, Pratyush Yadav
Hi, Pratyush, I'm replying to v10 so that we continue the discussion, but this applies to v13 as well. On 7/21/20 2:29 PM, Pratyush Yadav wrote: >>> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps) >>> struct spi_nor_flash_parameter *params = nor->params; >>> unsigned int cap; >>> >>> - /* DTR modes are not supported yet, mask them all. */ >>> - *hwcaps &= ~SNOR_HWCAPS_DTR; >>> - >>> /* X-X-X modes are not supported yet, mask them all. */ >>> *hwcaps &= ~SNOR_HWCAPS_X_X_X; >>> >>> + /* >>> + * If the reset line is broken, we do not want to enter a stateful >>> + * mode. >>> + */ >>> + if (nor->flags & SNOR_F_BROKEN_RESET) >>> + *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR); >> >> A dedicated reset line is not enough for flashes that keep their state >> in non-volatile bits. Since we can't protect from unexpected crashes in >> the non volatile state case, we should enter these modes only with an >> explicit request, i.e. an optional DT property: "update-nonvolatile-state", >> or something similar. > > I wrote this patch with the assumption that we won't be supporting> non-volatile configuration as of now. In the previous discussions we I think we have to take care of the stateful flashes now, otherwise we'll risk to end up with users that let their flashes in a mode from which they can't recover. I made some small RFC patches in reply to your v13, let me know what you think. > came to the conclusion that it is not easy to detect the flash if it > boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile > state, the flash would be useless after a reboot because we won't be > able to detect it in 8D mode. It doesn't matter if the reset line is > connected or not because it will reset the flash to the non-volatile > state, and we can't detect it from the non-volatile state. correct, so a reset line for stateful modes doesn't help and the comment from the code should be updated. s/stateful/stateless > >> For the volatile state case, we can parse the SFDP SCCR map, save if we >> can enter stateful modes in a volatile way, and if yes allow the entering. > > If we are not support volatile configurations, the reset line is enough > to take care of unexpected resets, no? I don't see any need to parse the reset line is excellent for the stateless flashes, it guarantees that the volatile bits will return to their default state. Disabling the clock, waiting for a period and re-enabling it again should do the trick too, but probably a dedicated reset line is safer. > SCCR map just for this. This fits the RFC that I sent to your v13. We need to parse as much as we can from the SFDP tables so that we don't abuse the flash info flags. > >> Do the flashes that you played with define the SFDP SCCR map? > > FWIW, the Cypress S28HS512T flash does but the Micron MT35XU512ABA does > not. > >>> + >>> for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) { >>> int rdidx, ppidx; >>> >>> @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor, >>> * controller directly implements the spi_nor interface. >>> * Yet another reason to switch to spi-mem. >>> */ >>> - ignored_mask = SNOR_HWCAPS_X_X_X; >>> + ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR; >>> if (shared_mask & ignored_mask) { >>> dev_dbg(nor->dev, >>> "SPI n-n-n protocols are not supported.\n"); >>> @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor) >>> SNOR_PROTO_1_1_8); >>> } >>> >>> + if (info->flags & SPI_NOR_OCTAL_DTR_READ) { >> >> Why do we need this flag? Can't we determine if the flash supports >> octal DTR by parsing SFDP? > > For Cypress S28HS512T, we can since it is xSPI compliant. We can't do > that for Micron MT35XU512ABA since it is not xSPI compliant. Ok > >>> + params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR; >>> + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_8_8_8_DTR], >>> + 0, 20, SPINOR_OP_READ_FAST, >>> + SNOR_PROTO_8_8_8_DTR); >>> + } >>> + >>> /* Page Program settings. */ >>> params->hwcaps.mask |= SNOR_HWCAPS_PP; >>> spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], >>> SPINOR_OP_PP, SNOR_PROTO_1_1_1); >>> >>> + /* >>> + * Since xSPI Page Program opcode is backward compatible with >>> + * Legacy SPI, use Legacy SPI opcode there as well. >>> + */ >>> + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP_8_8_8_DTR], >>> + SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR); >>> + >> >> This looks fishy. You haven't updated the hwcaps.mask, these pp settings never >> get selected? > > The problem here is that I don't see any field/table in SFDP that can > tell us {if,which} 8D-8D-8D program commands are supported. The xSPI > spec says that "The program commands provide SPI backward compatible > commands for programming data...". > > So we populate the 8D page program opcodes here (and in 4bait parsing) > using the 1S opcodes. The flashes have to enable the hwcap in fixup > hooks. I see. Would be good if you write this description as a comment, or in the commit message. > > As an alternative, maybe we can introduce the SPI_NOR_OCTAL_DTR_PP flag > that can enable the hwcap here? Thoughts? Should be fine the way that you did. We can change this later on if needed. > >>> /* >>> * Sector Erase settings. Sort Erase Types in ascending order, with the >>> * smallest erase size starting at BIT(0). >>> @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor) >>> >>> spi_nor_manufacturer_init_params(nor); >>> >>> - if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) && >>> + if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | >>> + SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && >>> !(nor->info->flags & SPI_NOR_SKIP_SFDP)) >>> spi_nor_sfdp_init_params(nor); >>> >>> @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor) >>> return err; >>> } >>> >>> - if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) { >>> + if (nor->addr_width == 4 && >>> + !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) && >> >> Why is the Octal DTR read exempted? > > It is based on the assumption explained below that 8D mode will always > use 4-byte addresses so we don't need to explicitly enable 8D mode. > Although I think maybe we should exempt all flashes that support DTR > mode? 4-4-4-dtr can work with 3-byte addresses, check MX25L25673G. 2-2-2-dtr should work too. > >>> + !(nor->flags & SNOR_F_4B_OPCODES)) { >>> /* >>> * If the RESET# pin isn't hooked up properly, or the system >>> * otherwise doesn't perform a reset command in the boot >>> @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor) >>> { >>> if (nor->addr_width) { >>> /* already configured from SFDP */ >>> + } else if (spi_nor_protocol_is_dtr(nor->read_proto)) { >>> + /* Always use 4-byte addresses in DTR mode. */ >>> + nor->addr_width = 4; >> >> Why? DTR with 3 byte addr width should be possible too. > > Should it be? What would happen to the half cycle left over? Do we then > start the dummy phase in the middle of the cycle? We would also have to > start the data phase in the middle of a cycle as well and end the > transaction with half a cycle left over. > > AFAIK, the controller I tested with (Cadence QSPI) does not support > this. Similarly, the two flashes this series adds support for, Cypress > S28HS512T and Micron MT35XU512ABA, don't support 3-byte address in 8D > mode. I'm not sure if there are any flashes or controllers that do. how about conditioning this only for 8-8-8-dtr? I'll now jump to v13 and continue the review there.
On 29/09/20 03:42PM, Tudor.Ambarus@microchip.com wrote: > Hi, Pratyush, > > I'm replying to v10 so that we continue the discussion, but this applies to v13 as well. > > On 7/21/20 2:29 PM, Pratyush Yadav wrote: > > >>> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps) > >>> struct spi_nor_flash_parameter *params = nor->params; > >>> unsigned int cap; > >>> > >>> - /* DTR modes are not supported yet, mask them all. */ > >>> - *hwcaps &= ~SNOR_HWCAPS_DTR; > >>> - > >>> /* X-X-X modes are not supported yet, mask them all. */ > >>> *hwcaps &= ~SNOR_HWCAPS_X_X_X; > >>> > >>> + /* > >>> + * If the reset line is broken, we do not want to enter a stateful > >>> + * mode. > >>> + */ > >>> + if (nor->flags & SNOR_F_BROKEN_RESET) > >>> + *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR); > >> > >> A dedicated reset line is not enough for flashes that keep their state > >> in non-volatile bits. Since we can't protect from unexpected crashes in > >> the non volatile state case, we should enter these modes only with an > >> explicit request, i.e. an optional DT property: "update-nonvolatile-state", > >> or something similar. > > > > I wrote this patch with the assumption that we won't be supporting> non-volatile configuration as of now. In the previous discussions we > > I think we have to take care of the stateful flashes now, otherwise we'll risk > to end up with users that let their flashes in a mode from which they can't recover. > I made some small RFC patches in reply to your v13, let me know what you think. I haven't gone through them yet. Will check tomorrow. > > came to the conclusion that it is not easy to detect the flash if it > > boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile > > state, the flash would be useless after a reboot because we won't be > > able to detect it in 8D mode. It doesn't matter if the reset line is > > connected or not because it will reset the flash to the non-volatile > > state, and we can't detect it from the non-volatile state. > > correct, so a reset line for stateful modes doesn't help and the comment from the > code should be updated. s/stateful/stateless We are talking about two different kinds of "state" here. The state you are talking about is the persistent state of the flash configured via non-volatile registers. Yes, a reset line doesn't help in that case at all. The other state is the non-persistent state we set on the flash. Using 1S-1S-8D mode is stateless in the sense that we didn't change any state on the flash to be able to use this mode, and only had to use the correct opcode. If we execute a 1S-1S-1S command next it will also work because the flash is still interpreting opcodes in 1S mode. Using 8D-8D-8D or 4S-4S-4S mode is stateful because we did have to configure some state on the flash (which can very well be volatile). Once 8D-8D-8D or 4S-4S-4S mode is entered, we cannot execute 1S-1S-1S commands until we reset the flash because now the flash is interpreting commands in 4S or 8D mode. This means we introduced some state on the flash. Having a reset line will not help against the former but will help against the latter. If the flash is in a stateful mode like 8D-8D-8D without a reset line, an unexpected reset could leave bootloader unable to boot because it issues the commands in 1S-1S-1S mode that the flash cannot interpret. So even if the state we set is volatile, we still want to avoid doing it if there is no reset line. So I think the code and comment should stay as they are. > > > >> For the volatile state case, we can parse the SFDP SCCR map, save if we > >> can enter stateful modes in a volatile way, and if yes allow the entering. > > > > If we are not support volatile configurations, the reset line is enough > > to take care of unexpected resets, no? I don't see any need to parse > > the reset line is excellent for the stateless flashes, it guarantees that the > volatile bits will return to their default state. Disabling the clock, waiting > for a period and re-enabling it again should do the trick too, but probably > a dedicated reset line is safer. > > > SCCR map just for this. > > This fits the RFC that I sent to your v13. We need to parse as much as we can > from the SFDP tables so that we don't abuse the flash info flags. > > > > >> Do the flashes that you played with define the SFDP SCCR map? > > > > FWIW, the Cypress S28HS512T flash does but the Micron MT35XU512ABA does > > not. > > > >>> + > >>> for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) { > >>> int rdidx, ppidx; > >>> > >>> @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor, > >>> * controller directly implements the spi_nor interface. > >>> * Yet another reason to switch to spi-mem. > >>> */ > >>> - ignored_mask = SNOR_HWCAPS_X_X_X; > >>> + ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR; > >>> if (shared_mask & ignored_mask) { > >>> dev_dbg(nor->dev, > >>> "SPI n-n-n protocols are not supported.\n"); > >>> @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor) > >>> SNOR_PROTO_1_1_8); > >>> } > >>> > >>> + if (info->flags & SPI_NOR_OCTAL_DTR_READ) { > >> > >> Why do we need this flag? Can't we determine if the flash supports > >> octal DTR by parsing SFDP? > > > > For Cypress S28HS512T, we can since it is xSPI compliant. We can't do > > that for Micron MT35XU512ABA since it is not xSPI compliant. > > Ok > > > > >>> + params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR; > >>> + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_8_8_8_DTR], > >>> + 0, 20, SPINOR_OP_READ_FAST, > >>> + SNOR_PROTO_8_8_8_DTR); > >>> + } > >>> + > >>> /* Page Program settings. */ > >>> params->hwcaps.mask |= SNOR_HWCAPS_PP; > >>> spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], > >>> SPINOR_OP_PP, SNOR_PROTO_1_1_1); > >>> > >>> + /* > >>> + * Since xSPI Page Program opcode is backward compatible with > >>> + * Legacy SPI, use Legacy SPI opcode there as well. > >>> + */ > >>> + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP_8_8_8_DTR], > >>> + SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR); > >>> + > >> > >> This looks fishy. You haven't updated the hwcaps.mask, these pp settings never > >> get selected? > > > > The problem here is that I don't see any field/table in SFDP that can > > tell us {if,which} 8D-8D-8D program commands are supported. The xSPI > > spec says that "The program commands provide SPI backward compatible > > commands for programming data...". > > > > So we populate the 8D page program opcodes here (and in 4bait parsing) > > using the 1S opcodes. The flashes have to enable the hwcap in fixup > > hooks. > > I see. Would be good if you write this description as a comment, or in the commit > message. > > > > > As an alternative, maybe we can introduce the SPI_NOR_OCTAL_DTR_PP flag > > that can enable the hwcap here? Thoughts? > > Should be fine the way that you did. We can change this later on if needed. I have already added it in v11 and later. Since it is already there I suppose it can stay. > > > >>> /* > >>> * Sector Erase settings. Sort Erase Types in ascending order, with the > >>> * smallest erase size starting at BIT(0). > >>> @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor) > >>> > >>> spi_nor_manufacturer_init_params(nor); > >>> > >>> - if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) && > >>> + if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > >>> + SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && > >>> !(nor->info->flags & SPI_NOR_SKIP_SFDP)) > >>> spi_nor_sfdp_init_params(nor); > >>> > >>> @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor) > >>> return err; > >>> } > >>> > >>> - if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) { > >>> + if (nor->addr_width == 4 && > >>> + !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) && > >> > >> Why is the Octal DTR read exempted? > > > > It is based on the assumption explained below that 8D mode will always > > use 4-byte addresses so we don't need to explicitly enable 8D mode. > > Although I think maybe we should exempt all flashes that support DTR > > mode? > > 4-4-4-dtr can work with 3-byte addresses, check MX25L25673G. 2-2-2-dtr should work too. Yes, I didn't catch this before. > > > > >>> + !(nor->flags & SNOR_F_4B_OPCODES)) { > >>> /* > >>> * If the RESET# pin isn't hooked up properly, or the system > >>> * otherwise doesn't perform a reset command in the boot > >>> @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor) > >>> { > >>> if (nor->addr_width) { > >>> /* already configured from SFDP */ > >>> + } else if (spi_nor_protocol_is_dtr(nor->read_proto)) { > >>> + /* Always use 4-byte addresses in DTR mode. */ > >>> + nor->addr_width = 4; > >> > >> Why? DTR with 3 byte addr width should be possible too. > > > > Should it be? What would happen to the half cycle left over? Do we then > > start the dummy phase in the middle of the cycle? We would also have to > > start the data phase in the middle of a cycle as well and end the > > transaction with half a cycle left over. > > > > AFAIK, the controller I tested with (Cadence QSPI) does not support > > this. Similarly, the two flashes this series adds support for, Cypress > > S28HS512T and Micron MT35XU512ABA, don't support 3-byte address in 8D > > mode. I'm not sure if there are any flashes or controllers that do. > > how about conditioning this only for 8-8-8-dtr? Yes, it should only apply for 8D-8D-8D. Will fix. > I'll now jump to v13 and continue the review there. -- Regards, Pratyush Yadav Texas Instruments India
On 9/29/20 9:12 PM, Tudor.Ambarus@microchip.com wrote: > Hi, Pratyush, > > I'm replying to v10 so that we continue the discussion, but this applies to v13 as well. > > On 7/21/20 2:29 PM, Pratyush Yadav wrote: > >>>> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps) >>>> struct spi_nor_flash_parameter *params = nor->params; >>>> unsigned int cap; >>>> >>>> - /* DTR modes are not supported yet, mask them all. */ >>>> - *hwcaps &= ~SNOR_HWCAPS_DTR; >>>> - >>>> /* X-X-X modes are not supported yet, mask them all. */ >>>> *hwcaps &= ~SNOR_HWCAPS_X_X_X; >>>> >>>> + /* >>>> + * If the reset line is broken, we do not want to enter a stateful >>>> + * mode. >>>> + */ >>>> + if (nor->flags & SNOR_F_BROKEN_RESET) >>>> + *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR); >>> >>> A dedicated reset line is not enough for flashes that keep their state >>> in non-volatile bits. Since we can't protect from unexpected crashes in >>> the non volatile state case, we should enter these modes only with an >>> explicit request, i.e. an optional DT property: "update-nonvolatile-state", >>> or something similar. >> >> I wrote this patch with the assumption that we won't be supporting> non-volatile configuration as of now. In the previous discussions we > > I think we have to take care of the stateful flashes now, otherwise we'll risk > to end up with users that let their flashes in a mode from which they can't recover. > I made some small RFC patches in reply to your v13, let me know what you think. > >> came to the conclusion that it is not easy to detect the flash if it >> boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile >> state, the flash would be useless after a reboot because we won't be >> able to detect it in 8D mode. It doesn't matter if the reset line is >> connected or not because it will reset the flash to the non-volatile >> state, and we can't detect it from the non-volatile state. > > correct, so a reset line for stateful modes doesn't help and the comment from the > code should be updated. s/stateful/stateless Entering an IO mode even using volatile bits (which gets cleared on SW or HW reset) creates a "state" that SW needs to keep track of which is why "stateful" term is used. I think that's what is implied here AFAIU, Boris's original RFC[1] introducing X-X-X mode also used "stateful mode" term in the same context . >> >>> For the volatile state case, we can parse the SFDP SCCR map, save if we >>> can enter stateful modes in a volatile way, and if yes allow the entering. >> >> If we are not support volatile configurations, the reset line is enough >> to take care of unexpected resets, no? I don't see any need to parse > > the reset line is excellent for the stateless flashes, it guarantees that the > volatile bits will return to their default state. Disabling the clock, waiting > for a period and re-enabling it again should do the trick too, but probably > a dedicated reset line is safer. > I don't think disable-wait-enable sequence of clock input to flash will trigger a reset... You have to take down the power and thus force flash to go through power-down/power-up sequence or use HW or SW reset sequences [1] https://patchwork.ozlabs.org/project/linux-mtd/cover/20181012084825.23697-1-boris.brezillon@bootlin.com/ Regards Vignesh
On 9/29/20 7:29 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 29/09/20 03:42PM, Tudor.Ambarus@microchip.com wrote: >> Hi, Pratyush, >> >> I'm replying to v10 so that we continue the discussion, but this applies to v13 as well. >> >> On 7/21/20 2:29 PM, Pratyush Yadav wrote: >> >>>>> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps) >>>>> struct spi_nor_flash_parameter *params = nor->params; >>>>> unsigned int cap; >>>>> >>>>> - /* DTR modes are not supported yet, mask them all. */ >>>>> - *hwcaps &= ~SNOR_HWCAPS_DTR; >>>>> - >>>>> /* X-X-X modes are not supported yet, mask them all. */ >>>>> *hwcaps &= ~SNOR_HWCAPS_X_X_X; >>>>> >>>>> + /* >>>>> + * If the reset line is broken, we do not want to enter a stateful >>>>> + * mode. >>>>> + */ >>>>> + if (nor->flags & SNOR_F_BROKEN_RESET) >>>>> + *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR); >>>> >>>> A dedicated reset line is not enough for flashes that keep their state >>>> in non-volatile bits. Since we can't protect from unexpected crashes in >>>> the non volatile state case, we should enter these modes only with an >>>> explicit request, i.e. an optional DT property: "update-nonvolatile-state", >>>> or something similar. >>> >>> I wrote this patch with the assumption that we won't be supporting> non-volatile configuration as of now. In the previous discussions we >> >> I think we have to take care of the stateful flashes now, otherwise we'll risk >> to end up with users that let their flashes in a mode from which they can't recover. >> I made some small RFC patches in reply to your v13, let me know what you think. > > I haven't gone through them yet. Will check tomorrow. > >>> came to the conclusion that it is not easy to detect the flash if it >>> boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile >>> state, the flash would be useless after a reboot because we won't be >>> able to detect it in 8D mode. It doesn't matter if the reset line is >>> connected or not because it will reset the flash to the non-volatile >>> state, and we can't detect it from the non-volatile state. >> >> correct, so a reset line for stateful modes doesn't help and the comment from the >> code should be updated. s/stateful/stateless > > We are talking about two different kinds of "state" here. The state you Right, I used 'stateful' for flashes that enter in a X-X-X mode by setting a non-volatile bit and 'stateless' for those that enter in a X-X-X mode via volatile bits. > are talking about is the persistent state of the flash configured via > non-volatile registers. Yes, a reset line doesn't help in that case at > all. > > The other state is the non-persistent state we set on the flash. Using > 1S-1S-8D mode is stateless in the sense that we didn't change any state > on the flash to be able to use this mode, and only had to use the > correct opcode. If we execute a 1S-1S-1S command next it will also work > because the flash is still interpreting opcodes in 1S mode. Using > 8D-8D-8D or 4S-4S-4S mode is stateful because we did have to configure > some state on the flash (which can very well be volatile). Once 8D-8D-8D > or 4S-4S-4S mode is entered, we cannot execute 1S-1S-1S commands until > we reset the flash because now the flash is interpreting commands in 4S > or 8D mode. This means we introduced some state on the flash. > > Having a reset line will not help against the former but will help > against the latter. If the flash is in a stateful mode like 8D-8D-8D > without a reset line, an unexpected reset could leave bootloader unable > to boot because it issues the commands in 1S-1S-1S mode that the flash > cannot interpret. So even if the state we set is volatile, we still want > to avoid doing it if there is no reset line. > > So I think the code and comment should stay as they are. Ok. Cheers, ta
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 0369d98b2d12..22a3832b83a6 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -40,6 +40,76 @@ #define SPI_NOR_MAX_ADDR_WIDTH 4 +/** + * spi_nor_get_cmd_ext() - Get the command opcode extension based on the + * extension type. + * @nor: pointer to a 'struct spi_nor' + * @op: pointer to the 'struct spi_mem_op' whose properties + * need to be initialized. + * + * Right now, only "repeat" and "invert" are supported. + * + * Return: The opcode extension. + */ +static u8 spi_nor_get_cmd_ext(const struct spi_nor *nor, + const struct spi_mem_op *op) +{ + switch (nor->cmd_ext_type) { + case SPI_NOR_EXT_INVERT: + return ~op->cmd.opcode; + + case SPI_NOR_EXT_REPEAT: + return op->cmd.opcode; + + default: + dev_err(nor->dev, "Unknown command extension type\n"); + return 0; + } +} + +/** + * spi_nor_spimem_setup_op() - Set up common properties of a spi-mem op. + * @nor: pointer to a 'struct spi_nor' + * @op: pointer to the 'struct spi_mem_op' whose properties + * need to be initialized. + * @proto: the protocol from which the properties need to be set. + */ +void spi_nor_spimem_setup_op(const struct spi_nor *nor, + struct spi_mem_op *op, + const enum spi_nor_protocol proto) +{ + u8 ext; + + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(proto); + + if (op->addr.nbytes) + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto); + + if (op->dummy.nbytes) + op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto); + + if (op->data.nbytes) + op->data.buswidth = spi_nor_get_protocol_data_nbits(proto); + + if (spi_nor_protocol_is_dtr(proto)) { + /* + * spi-mem supports mixed DTR modes, but right now we can only + * have all phases either DTR or STR. IOW, spi-mem can have + * something like 4S-4D-4D, but spi-nor can't. So, set all 4 + * phases to either DTR or STR. + */ + op->cmd.dtr = op->addr.dtr = op->dummy.dtr + = op->data.dtr = true; + + /* 2 bytes per clock cycle in DTR mode. */ + op->dummy.nbytes *= 2; + + ext = spi_nor_get_cmd_ext(nor, op); + op->cmd.opcode = (op->cmd.opcode << 8) | ext; + op->cmd.nbytes = 2; + } +} + /** * spi_nor_spimem_bounce() - check if a bounce buffer is needed for the data * transfer @@ -104,14 +174,12 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from, ssize_t nbytes; int error; - /* get transfer protocols. */ - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); - op.dummy.buswidth = op.addr.buswidth; - op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); + spi_nor_spimem_setup_op(nor, &op, nor->read_proto); /* convert the dummy cycles to the number of bytes */ op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8; + if (spi_nor_protocol_is_dtr(nor->read_proto)) + op.dummy.nbytes *= 2; usebouncebuf = spi_nor_spimem_bounce(nor, &op); @@ -169,13 +237,11 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to, ssize_t nbytes; int error; - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); - op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); - if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second) op.addr.nbytes = 0; + spi_nor_spimem_setup_op(nor, &op, nor->write_proto); + if (spi_nor_spimem_bounce(nor, &op)) memcpy(nor->bouncebuf, buf, op.data.nbytes); @@ -227,10 +293,16 @@ int spi_nor_write_enable(struct spi_nor *nor) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_NO_DATA); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREN, - NULL, 0); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->write_reg(nor, + SPINOR_OP_WREN, + NULL, 0); } if (ret) @@ -256,10 +328,16 @@ int spi_nor_write_disable(struct spi_nor *nor) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_NO_DATA); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WRDI, - NULL, 0); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->write_reg(nor, + SPINOR_OP_WRDI, + NULL, 0); } if (ret) @@ -318,10 +396,15 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_DATA_IN(1, fsr, 1)); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDFSR, - fsr, 1); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDFSR, + fsr, 1); } if (ret) @@ -350,9 +433,15 @@ static int spi_nor_read_cr(struct spi_nor *nor, u8 *cr) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_DATA_IN(1, cr, 1)); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDCR, cr, 1); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDCR, + cr, 1); } if (ret) @@ -383,12 +472,17 @@ int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_NO_DATA); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->write_reg(nor, - enable ? SPINOR_OP_EN4B : - SPINOR_OP_EX4B, - NULL, 0); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->write_reg(nor, + enable ? SPINOR_OP_EN4B : + SPINOR_OP_EX4B, + NULL, 0); } if (ret) @@ -419,10 +513,15 @@ static int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_DATA_OUT(1, nor->bouncebuf, 1)); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_BRWR, - nor->bouncebuf, 1); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->write_reg(nor, SPINOR_OP_BRWR, + nor->bouncebuf, 1); } if (ret) @@ -451,10 +550,16 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_DATA_OUT(1, nor->bouncebuf, 1)); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREAR, - nor->bouncebuf, 1); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->write_reg(nor, + SPINOR_OP_WREAR, + nor->bouncebuf, 1); } if (ret) @@ -482,10 +587,16 @@ int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_DATA_IN(1, sr, 1)); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_XRDSR, - sr, 1); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->read_reg(nor, + SPINOR_OP_XRDSR, + sr, 1); } if (ret) @@ -527,10 +638,16 @@ static void spi_nor_clear_sr(struct spi_nor *nor) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_NO_DATA); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_CLSR, - NULL, 0); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->write_reg(nor, + SPINOR_OP_CLSR, + NULL, 0); } if (ret) @@ -591,10 +708,16 @@ static void spi_nor_clear_fsr(struct spi_nor *nor) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_NO_DATA); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_CLFSR, - NULL, 0); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->write_reg(nor, + SPINOR_OP_CLFSR, + NULL, 0); } if (ret) @@ -735,10 +858,16 @@ static int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_DATA_OUT(len, sr, 1)); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WRSR, - sr, len); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->write_reg(nor, + SPINOR_OP_WRSR, + sr, len); } if (ret) { @@ -937,10 +1066,16 @@ static int spi_nor_write_sr2(struct spi_nor *nor, const u8 *sr2) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_DATA_OUT(1, sr2, 1)); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WRSR2, - sr2, 1); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->write_reg(nor, + SPINOR_OP_WRSR2, + sr2, 1); } if (ret) { @@ -971,10 +1106,16 @@ static int spi_nor_read_sr2(struct spi_nor *nor, u8 *sr2) SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_DATA_IN(1, sr2, 1)); + spi_nor_spimem_setup_op(nor, &op, nor->reg_proto); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR2, - sr2, 1); + if (spi_nor_protocol_is_dtr(nor->reg_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->read_reg(nor, + SPINOR_OP_RDSR2, + sr2, 1); } if (ret) @@ -1002,10 +1143,16 @@ 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); + ret = spi_mem_exec_op(nor->spimem, &op); } else { - ret = nor->controller_ops->write_reg(nor, SPINOR_OP_CHIP_ERASE, - NULL, 0); + if (spi_nor_protocol_is_dtr(nor->write_proto)) + ret = -ENOTSUPP; + else + ret = nor->controller_ops->write_reg(nor, + SPINOR_OP_CHIP_ERASE, + NULL, 0); } if (ret) @@ -1144,7 +1291,11 @@ static 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); + return spi_mem_exec_op(nor->spimem, &op); + } else if (spi_nor_protocol_is_dtr(nor->write_proto)) { + return -ENOTSUPP; } else if (nor->controller_ops->erase) { return nor->controller_ops->erase(nor, addr); } @@ -2253,6 +2404,7 @@ int spi_nor_hwcaps_read2cmd(u32 hwcaps) { SNOR_HWCAPS_READ_1_8_8, SNOR_CMD_READ_1_8_8 }, { SNOR_HWCAPS_READ_8_8_8, SNOR_CMD_READ_8_8_8 }, { SNOR_HWCAPS_READ_1_8_8_DTR, SNOR_CMD_READ_1_8_8_DTR }, + { SNOR_HWCAPS_READ_8_8_8_DTR, SNOR_CMD_READ_8_8_8_DTR }, }; return spi_nor_hwcaps2cmd(hwcaps, hwcaps_read2cmd, @@ -2269,6 +2421,7 @@ static int spi_nor_hwcaps_pp2cmd(u32 hwcaps) { SNOR_HWCAPS_PP_1_1_8, SNOR_CMD_PP_1_1_8 }, { SNOR_HWCAPS_PP_1_8_8, SNOR_CMD_PP_1_8_8 }, { SNOR_HWCAPS_PP_8_8_8, SNOR_CMD_PP_8_8_8 }, + { SNOR_HWCAPS_PP_8_8_8_DTR, SNOR_CMD_PP_8_8_8_DTR }, }; return spi_nor_hwcaps2cmd(hwcaps, hwcaps_pp2cmd, @@ -2322,13 +2475,11 @@ static int spi_nor_spimem_check_readop(struct spi_nor *nor, SPI_MEM_OP_DUMMY(0, 1), SPI_MEM_OP_DATA_IN(0, NULL, 1)); - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(read->proto); - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(read->proto); - op.data.buswidth = spi_nor_get_protocol_data_nbits(read->proto); - op.dummy.buswidth = op.addr.buswidth; op.dummy.nbytes = (read->num_mode_clocks + read->num_wait_states) * op.dummy.buswidth / 8; + spi_nor_spimem_setup_op(nor, &op, read->proto); + return spi_nor_spimem_check_op(nor, &op); } @@ -2348,9 +2499,7 @@ static int spi_nor_spimem_check_pp(struct spi_nor *nor, SPI_MEM_OP_NO_DUMMY, SPI_MEM_OP_DATA_OUT(0, NULL, 1)); - op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(pp->proto); - op.addr.buswidth = spi_nor_get_protocol_addr_nbits(pp->proto); - op.data.buswidth = spi_nor_get_protocol_data_nbits(pp->proto); + spi_nor_spimem_setup_op(nor, &op, pp->proto); return spi_nor_spimem_check_op(nor, &op); } @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps) struct spi_nor_flash_parameter *params = nor->params; unsigned int cap; - /* DTR modes are not supported yet, mask them all. */ - *hwcaps &= ~SNOR_HWCAPS_DTR; - /* X-X-X modes are not supported yet, mask them all. */ *hwcaps &= ~SNOR_HWCAPS_X_X_X; + /* + * If the reset line is broken, we do not want to enter a stateful + * mode. + */ + if (nor->flags & SNOR_F_BROKEN_RESET) + *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR); + for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) { int rdidx, ppidx; @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor, * controller directly implements the spi_nor interface. * Yet another reason to switch to spi-mem. */ - ignored_mask = SNOR_HWCAPS_X_X_X; + ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR; if (shared_mask & ignored_mask) { dev_dbg(nor->dev, "SPI n-n-n protocols are not supported.\n"); @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor) SNOR_PROTO_1_1_8); } + if (info->flags & SPI_NOR_OCTAL_DTR_READ) { + params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR; + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_8_8_8_DTR], + 0, 20, SPINOR_OP_READ_FAST, + SNOR_PROTO_8_8_8_DTR); + } + /* Page Program settings. */ params->hwcaps.mask |= SNOR_HWCAPS_PP; spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP], SPINOR_OP_PP, SNOR_PROTO_1_1_1); + /* + * Since xSPI Page Program opcode is backward compatible with + * Legacy SPI, use Legacy SPI opcode there as well. + */ + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP_8_8_8_DTR], + SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR); + /* * Sector Erase settings. Sort Erase Types in ascending order, with the * smallest erase size starting at BIT(0). @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor) spi_nor_manufacturer_init_params(nor); - if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) && + if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && !(nor->info->flags & SPI_NOR_SKIP_SFDP)) spi_nor_sfdp_init_params(nor); @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor) return err; } - if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) { + if (nor->addr_width == 4 && + !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) && + !(nor->flags & SNOR_F_4B_OPCODES)) { /* * If the RESET# pin isn't hooked up properly, or the system * otherwise doesn't perform a reset command in the boot @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor) { if (nor->addr_width) { /* already configured from SFDP */ + } else if (spi_nor_protocol_is_dtr(nor->read_proto)) { + /* Always use 4-byte addresses in DTR mode. */ + nor->addr_width = 4; } else if (nor->info->addr_width) { nor->addr_width = nor->info->addr_width; } else if (nor->mtd.size > 0x1000000) { @@ -3244,14 +3417,19 @@ static int spi_nor_create_read_dirmap(struct spi_nor *nor) }; struct spi_mem_op *op = &info.op_tmpl; - /* get transfer protocols. */ - op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto); - op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto); - op->dummy.buswidth = op->addr.buswidth; - op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); + spi_nor_spimem_setup_op(nor, op, nor->read_proto); /* convert the dummy cycles to the number of bytes */ op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8; + if (spi_nor_protocol_is_dtr(nor->read_proto)) + op->dummy.nbytes *= 2; + + /* + * Since spi_nor_spimem_setup_op() only sets buswidth when the number + * of data bytes is non-zero, the data buswidth won't be set here. So, + * do it explicitly. + */ + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto); nor->dirmap.rdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem, &info); @@ -3270,15 +3448,18 @@ static int spi_nor_create_write_dirmap(struct spi_nor *nor) }; struct spi_mem_op *op = &info.op_tmpl; - /* get transfer protocols. */ - op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto); - op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto); - op->dummy.buswidth = op->addr.buswidth; - op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); - if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second) op->addr.nbytes = 0; + spi_nor_spimem_setup_op(nor, op, nor->write_proto); + + /* + * Since spi_nor_spimem_setup_op() only sets buswidth when the number + * of data bytes is non-zero, the data buswidth won't be set here. So, + * do it explicitly. + */ + op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto); + nor->dirmap.wdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem, &info); return PTR_ERR_OR_ZERO(nor->dirmap.wdesc); diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h index 6f2f6b27173f..de1e3917889f 100644 --- a/drivers/mtd/spi-nor/core.h +++ b/drivers/mtd/spi-nor/core.h @@ -62,6 +62,7 @@ enum spi_nor_read_command_index { SNOR_CMD_READ_1_8_8, SNOR_CMD_READ_8_8_8, SNOR_CMD_READ_1_8_8_DTR, + SNOR_CMD_READ_8_8_8_DTR, SNOR_CMD_READ_MAX }; @@ -78,6 +79,7 @@ enum spi_nor_pp_command_index { SNOR_CMD_PP_1_1_8, SNOR_CMD_PP_1_8_8, SNOR_CMD_PP_8_8_8, + SNOR_CMD_PP_8_8_8_DTR, SNOR_CMD_PP_MAX }; @@ -311,6 +313,7 @@ struct flash_info { * BP3 is bit 6 of status register. * Must be used with SPI_NOR_4BIT_BP. */ +#define SPI_NOR_OCTAL_DTR_READ BIT(19) /* Flash supports octal DTR Read. */ /* Part specific fixup hooks. */ const struct spi_nor_fixups *fixups; @@ -399,6 +402,9 @@ extern const struct spi_nor_manufacturer spi_nor_winbond; extern const struct spi_nor_manufacturer spi_nor_xilinx; extern const struct spi_nor_manufacturer spi_nor_xmc; +void spi_nor_spimem_setup_op(const struct spi_nor *nor, + struct spi_mem_op *op, + const enum spi_nor_protocol proto); int spi_nor_write_enable(struct spi_nor *nor); int spi_nor_write_disable(struct spi_nor *nor); int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable); diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c index 55c0c508464b..cb6e93a3560a 100644 --- a/drivers/mtd/spi-nor/sfdp.c +++ b/drivers/mtd/spi-nor/sfdp.c @@ -1046,9 +1046,16 @@ static int spi_nor_parse_4bait(struct spi_nor *nor, } /* 4BAIT is the only SFDP table that indicates page program support. */ - if (pp_hwcaps & SNOR_HWCAPS_PP) + if (pp_hwcaps & SNOR_HWCAPS_PP) { spi_nor_set_pp_settings(¶ms_pp[SNOR_CMD_PP], SPINOR_OP_PP_4B, SNOR_PROTO_1_1_1); + /* + * Since xSPI Page Program opcode is backward compatible with + * Legacy SPI, use Legacy SPI opcode there as well. + */ + spi_nor_set_pp_settings(¶ms_pp[SNOR_CMD_PP_8_8_8_DTR], + SPINOR_OP_PP_4B, SNOR_PROTO_8_8_8_DTR); + } if (pp_hwcaps & SNOR_HWCAPS_PP_1_1_4) spi_nor_set_pp_settings(¶ms_pp[SNOR_CMD_PP_1_1_4], SPINOR_OP_PP_1_1_4_4B, diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 60bac2c0ec45..cd549042c53d 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -182,6 +182,7 @@ enum spi_nor_protocol { SNOR_PROTO_1_2_2_DTR = SNOR_PROTO_DTR(1, 2, 2), SNOR_PROTO_1_4_4_DTR = SNOR_PROTO_DTR(1, 4, 4), SNOR_PROTO_1_8_8_DTR = SNOR_PROTO_DTR(1, 8, 8), + SNOR_PROTO_8_8_8_DTR = SNOR_PROTO_DTR(8, 8, 8), }; static inline bool spi_nor_protocol_is_dtr(enum spi_nor_protocol proto) @@ -228,7 +229,7 @@ struct spi_nor_hwcaps { * then Quad SPI protocols before Dual SPI protocols, Fast Read and lastly * (Slow) Read. */ -#define SNOR_HWCAPS_READ_MASK GENMASK(14, 0) +#define SNOR_HWCAPS_READ_MASK GENMASK(15, 0) #define SNOR_HWCAPS_READ BIT(0) #define SNOR_HWCAPS_READ_FAST BIT(1) #define SNOR_HWCAPS_READ_1_1_1_DTR BIT(2) @@ -245,11 +246,12 @@ struct spi_nor_hwcaps { #define SNOR_HWCAPS_READ_4_4_4 BIT(9) #define SNOR_HWCAPS_READ_1_4_4_DTR BIT(10) -#define SNOR_HWCAPS_READ_OCTAL GENMASK(14, 11) +#define SNOR_HWCAPS_READ_OCTAL GENMASK(15, 11) #define SNOR_HWCAPS_READ_1_1_8 BIT(11) #define SNOR_HWCAPS_READ_1_8_8 BIT(12) #define SNOR_HWCAPS_READ_8_8_8 BIT(13) #define SNOR_HWCAPS_READ_1_8_8_DTR BIT(14) +#define SNOR_HWCAPS_READ_8_8_8_DTR BIT(15) /* * Page Program capabilities. @@ -260,18 +262,19 @@ struct spi_nor_hwcaps { * JEDEC/SFDP standard to define them. Also at this moment no SPI flash memory * implements such commands. */ -#define SNOR_HWCAPS_PP_MASK GENMASK(22, 16) -#define SNOR_HWCAPS_PP BIT(16) +#define SNOR_HWCAPS_PP_MASK GENMASK(23, 16) +#define SNOR_HWCAPS_PP BIT(16) -#define SNOR_HWCAPS_PP_QUAD GENMASK(19, 17) -#define SNOR_HWCAPS_PP_1_1_4 BIT(17) -#define SNOR_HWCAPS_PP_1_4_4 BIT(18) -#define SNOR_HWCAPS_PP_4_4_4 BIT(19) +#define SNOR_HWCAPS_PP_QUAD GENMASK(19, 17) +#define SNOR_HWCAPS_PP_1_1_4 BIT(17) +#define SNOR_HWCAPS_PP_1_4_4 BIT(18) +#define SNOR_HWCAPS_PP_4_4_4 BIT(19) -#define SNOR_HWCAPS_PP_OCTAL GENMASK(22, 20) -#define SNOR_HWCAPS_PP_1_1_8 BIT(20) -#define SNOR_HWCAPS_PP_1_8_8 BIT(21) -#define SNOR_HWCAPS_PP_8_8_8 BIT(22) +#define SNOR_HWCAPS_PP_OCTAL GENMASK(23, 20) +#define SNOR_HWCAPS_PP_1_1_8 BIT(20) +#define SNOR_HWCAPS_PP_1_8_8 BIT(21) +#define SNOR_HWCAPS_PP_8_8_8 BIT(22) +#define SNOR_HWCAPS_PP_8_8_8_DTR BIT(23) #define SNOR_HWCAPS_X_X_X (SNOR_HWCAPS_READ_2_2_2 | \ SNOR_HWCAPS_READ_4_4_4 | \ @@ -279,10 +282,14 @@ struct spi_nor_hwcaps { SNOR_HWCAPS_PP_4_4_4 | \ SNOR_HWCAPS_PP_8_8_8) +#define SNOR_HWCAPS_X_X_X_DTR (SNOR_HWCAPS_READ_8_8_8_DTR | \ + SNOR_HWCAPS_PP_8_8_8_DTR) + #define SNOR_HWCAPS_DTR (SNOR_HWCAPS_READ_1_1_1_DTR | \ SNOR_HWCAPS_READ_1_2_2_DTR | \ SNOR_HWCAPS_READ_1_4_4_DTR | \ - SNOR_HWCAPS_READ_1_8_8_DTR) + SNOR_HWCAPS_READ_1_8_8_DTR | \ + SNOR_HWCAPS_READ_8_8_8_DTR) #define SNOR_HWCAPS_ALL (SNOR_HWCAPS_READ_MASK | \ SNOR_HWCAPS_PP_MASK) @@ -318,6 +325,22 @@ struct spi_nor_controller_ops { int (*erase)(struct spi_nor *nor, loff_t offs); }; +/** + * enum spi_nor_cmd_ext - describes the command opcode extension in DTR mode + * @SPI_NOR_EXT_NONE: no extension. This is the default, and is used in Legacy + * SPI mode + * @SPI_NOR_EXT_REPEAT: the extension is same as the opcode + * @SPI_NOR_EXT_INVERT: the extension is the bitwise inverse of the opcode + * @SPI_NOR_EXT_HEX: the extension is any hex value. The command and opcode + * combine to form a 16-bit opcode. + */ +enum spi_nor_cmd_ext { + SPI_NOR_EXT_NONE = 0, + SPI_NOR_EXT_REPEAT, + SPI_NOR_EXT_INVERT, + SPI_NOR_EXT_HEX, +}; + /* * Forward declarations that are used internally by the core and manufacturer * drivers. @@ -345,6 +368,7 @@ struct spi_nor_flash_parameter; * @program_opcode: the program opcode * @sst_write_second: used by the SST write operation * @flags: flag options for the current SPI NOR (SNOR_F_*) + * @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 @@ -376,6 +400,7 @@ struct spi_nor { enum spi_nor_protocol reg_proto; bool sst_write_second; u32 flags; + enum spi_nor_cmd_ext cmd_ext_type; const struct spi_nor_controller_ops *controller_ops;
Double Transfer Rate (DTR) is SPI protocol in which data is transferred on each clock edge as opposed to on each clock cycle. Make framework-level changes to allow supporting flashes in DTR mode. Right now, mixed DTR modes are not supported. So, for example a mode like 4S-4D-4D will not work. All phases need to be either DTR or STR. Signed-off-by: Pratyush Yadav <p.yadav@ti.com> --- drivers/mtd/spi-nor/core.c | 305 ++++++++++++++++++++++++++++-------- drivers/mtd/spi-nor/core.h | 6 + drivers/mtd/spi-nor/sfdp.c | 9 +- include/linux/mtd/spi-nor.h | 51 ++++-- 4 files changed, 295 insertions(+), 76 deletions(-)