| Message ID | 381f7f66c68136a2de1d768b44fb6bcf9eebbebe.1686557139.git.Takahiro.Kuwano@infineon.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: spansion: Add support for Infineon S28HS02GT | expand |
On 6/12/23 11:04, tkuw584924@gmail.com wrote: > From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > > S28HS02GT is multi-chip package (MCP) device that requires Octal DTR > configuraion for each die. As preparation for MCP support, this patch > replaces cypress_nor_octal_dtr_en/dis() with cypress_nor_setup_memlat() > and cypress_nor_setup_opiddr(). And the ID check part is moved to > cypress_nor_octal_dtr_enable(). > > Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com> > --- > drivers/mtd/spi-nor/spansion.c | 118 +++++++++++++++++---------------- > 1 file changed, 62 insertions(+), 56 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c > index 7804be3a9f2a..0daa3a357ae8 100644 > --- a/drivers/mtd/spi-nor/spansion.c > +++ b/drivers/mtd/spi-nor/spansion.c > @@ -156,7 +156,7 @@ static int cypress_nor_sr_ready_and_clear(struct spi_nor *nor) > return 1; > } > > -static int cypress_nor_octal_dtr_en(struct spi_nor *nor) > +static int cypress_nor_setup_memlat(struct spi_nor *nor) cypress_nor_set_memlat? Be kind and introduce a description for the method so that we don't cross check the datasheet each time. I see that for memlat we use a hardcoded value, whereas it should have been dynamically determined based on the flash freq. Something to improve in the future if you care. > { > struct spi_mem_op op; > u8 *buf = nor->bouncebuf; > @@ -178,67 +178,37 @@ static int cypress_nor_octal_dtr_en(struct spi_nor *nor) > CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes, > SPINOR_REG_CYPRESS_CFR2V, 1, buf); > > - ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto); > - if (ret) > - return ret; > - > - nor->read_dummy = 24; > - > - /* Set the octal and DTR enable bits. */ > - buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN; > - op = (struct spi_mem_op) > - CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes, > - SPINOR_REG_CYPRESS_CFR5V, 1, buf); > - > - ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto); > - if (ret) > - return ret; > - > - /* Read flash ID to make sure the switch was successful. */ > - ret = spi_nor_read_id(nor, nor->addr_nbytes, 3, buf, > - SNOR_PROTO_8_8_8_DTR); > - if (ret) { > - dev_dbg(nor->dev, "error %d reading JEDEC ID after enabling 8D-8D-8D mode\n", ret); > - return ret; > - } > - > - if (memcmp(buf, nor->info->id, nor->info->id_len)) > - return -EINVAL; > - > - return 0; > + return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto); > } > > -static int cypress_nor_octal_dtr_dis(struct spi_nor *nor) > +static int cypress_nor_setup_opiddr(struct spi_nor *nor, bool enable) what does opiddr stand for? Let's rename it to something humans can understand. > { > struct spi_mem_op op; > u8 *buf = nor->bouncebuf; > - int ret; > - > - /* > - * The register is 1-byte wide, but 1-byte transactions are not allowed > - * in 8D-8D-8D mode. Since there is no register at the next location, > - * just initialize the value to 0 and let the transaction go on. > - */ > - buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS; > - buf[1] = 0; > - op = (struct spi_mem_op) > - CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes, > - SPINOR_REG_CYPRESS_CFR5V, 2, buf); > - ret = spi_nor_write_any_volatile_reg(nor, &op, SNOR_PROTO_8_8_8_DTR); > - if (ret) > - return ret; > > - /* Read flash ID to make sure the switch was successful. */ > - ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1); > - if (ret) { > - dev_dbg(nor->dev, "error %d reading JEDEC ID after disabling 8D-8D-8D mode\n", ret); > - return ret; > + if (enable) { > + /* Set the octal and DTR enable bits. */ > + buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN; > + op = (struct spi_mem_op) > + CYPRESS_NOR_WR_ANY_REG_OP(nor->params->addr_mode_nbytes, > + SPINOR_REG_CYPRESS_CFR5V, 1, > + buf); > + } else { > + /* > + * The register is 1-byte wide, but 1-byte transactions are not > + * allowed in 8D-8D-8D mode. Since there is no register at the > + * next location, just initialize the value to 0 and let the > + * transaction go on. > + */ > + buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS; > + buf[1] = 0; > + op = (struct spi_mem_op) > + CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes, > + SPINOR_REG_CYPRESS_CFR5V, 2, > + buf); > } > > - if (memcmp(buf, nor->info->id, nor->info->id_len)) > - return -EINVAL; > - > - return 0; > + return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto); > } > > static int cypress_nor_quad_enable_volatile_reg(struct spi_nor *nor, u64 addr) > @@ -642,8 +612,44 @@ static struct spi_nor_fixups s25hx_t_fixups = { > */ > static int cypress_nor_octal_dtr_enable(struct spi_nor *nor, bool enable) what a terrible name > { > - return enable ? cypress_nor_octal_dtr_en(nor) : > - cypress_nor_octal_dtr_dis(nor); > + int ret; > + u8 naddr, ndummy; > + enum spi_nor_protocol proto; > + > + if (enable) { so we have cypress_nor_octal_dtr_enable and now we check for enable, yuck. I know it comes from the SPI NOR core, we shall update the core, I wouldn't continue like this. > + ret = cypress_nor_setup_memlat(nor); > + if (ret) > + return ret; > + > + nor->read_dummy = 24; shouldn't this be set in cypress_nor_set_memlat? > + } > + > + ret = cypress_nor_setup_opiddr(nor, enable); > + if (ret) > + return ret; > + > + /* Read flash ID to make sure the switch was successful. */ > + if (enable) { > + naddr = nor->addr_nbytes; > + ndummy = 3; > + proto = SNOR_PROTO_8_8_8_DTR; > + } else { > + naddr = 0; > + ndummy = 0; > + proto = SNOR_PROTO_1_1_1; > + } I don't like all the if conditions in the octal_dtr_enable methods, I find the method hard to read and I feel we are butchering the code just to make it work. > + > + ret = spi_nor_read_id(nor, naddr, ndummy, nor->bouncebuf, proto); > + if (ret) { > + dev_dbg(nor->dev, "error %d reading JEDEC ID after %s 8D-8D-8D mode\n", > + ret, enable ? "enabling" : "disabling"); > + return ret; > + } > + > + if (memcmp(nor->bouncebuf, nor->info->id, nor->info->id_len)) > + return -EINVAL; > + > + return 0; > } > > static int s28hx_t_post_sfdp_fixup(struct spi_nor *nor)
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c index 7804be3a9f2a..0daa3a357ae8 100644 --- a/drivers/mtd/spi-nor/spansion.c +++ b/drivers/mtd/spi-nor/spansion.c @@ -156,7 +156,7 @@ static int cypress_nor_sr_ready_and_clear(struct spi_nor *nor) return 1; } -static int cypress_nor_octal_dtr_en(struct spi_nor *nor) +static int cypress_nor_setup_memlat(struct spi_nor *nor) { struct spi_mem_op op; u8 *buf = nor->bouncebuf; @@ -178,67 +178,37 @@ static int cypress_nor_octal_dtr_en(struct spi_nor *nor) CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes, SPINOR_REG_CYPRESS_CFR2V, 1, buf); - ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto); - if (ret) - return ret; - - nor->read_dummy = 24; - - /* Set the octal and DTR enable bits. */ - buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN; - op = (struct spi_mem_op) - CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes, - SPINOR_REG_CYPRESS_CFR5V, 1, buf); - - ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto); - if (ret) - return ret; - - /* Read flash ID to make sure the switch was successful. */ - ret = spi_nor_read_id(nor, nor->addr_nbytes, 3, buf, - SNOR_PROTO_8_8_8_DTR); - if (ret) { - dev_dbg(nor->dev, "error %d reading JEDEC ID after enabling 8D-8D-8D mode\n", ret); - return ret; - } - - if (memcmp(buf, nor->info->id, nor->info->id_len)) - return -EINVAL; - - return 0; + return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto); } -static int cypress_nor_octal_dtr_dis(struct spi_nor *nor) +static int cypress_nor_setup_opiddr(struct spi_nor *nor, bool enable) { struct spi_mem_op op; u8 *buf = nor->bouncebuf; - int ret; - - /* - * The register is 1-byte wide, but 1-byte transactions are not allowed - * in 8D-8D-8D mode. Since there is no register at the next location, - * just initialize the value to 0 and let the transaction go on. - */ - buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS; - buf[1] = 0; - op = (struct spi_mem_op) - CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes, - SPINOR_REG_CYPRESS_CFR5V, 2, buf); - ret = spi_nor_write_any_volatile_reg(nor, &op, SNOR_PROTO_8_8_8_DTR); - if (ret) - return ret; - /* Read flash ID to make sure the switch was successful. */ - ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1); - if (ret) { - dev_dbg(nor->dev, "error %d reading JEDEC ID after disabling 8D-8D-8D mode\n", ret); - return ret; + if (enable) { + /* Set the octal and DTR enable bits. */ + buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN; + op = (struct spi_mem_op) + CYPRESS_NOR_WR_ANY_REG_OP(nor->params->addr_mode_nbytes, + SPINOR_REG_CYPRESS_CFR5V, 1, + buf); + } else { + /* + * The register is 1-byte wide, but 1-byte transactions are not + * allowed in 8D-8D-8D mode. Since there is no register at the + * next location, just initialize the value to 0 and let the + * transaction go on. + */ + buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS; + buf[1] = 0; + op = (struct spi_mem_op) + CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes, + SPINOR_REG_CYPRESS_CFR5V, 2, + buf); } - if (memcmp(buf, nor->info->id, nor->info->id_len)) - return -EINVAL; - - return 0; + return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto); } static int cypress_nor_quad_enable_volatile_reg(struct spi_nor *nor, u64 addr) @@ -642,8 +612,44 @@ static struct spi_nor_fixups s25hx_t_fixups = { */ static int cypress_nor_octal_dtr_enable(struct spi_nor *nor, bool enable) { - return enable ? cypress_nor_octal_dtr_en(nor) : - cypress_nor_octal_dtr_dis(nor); + int ret; + u8 naddr, ndummy; + enum spi_nor_protocol proto; + + if (enable) { + ret = cypress_nor_setup_memlat(nor); + if (ret) + return ret; + + nor->read_dummy = 24; + } + + ret = cypress_nor_setup_opiddr(nor, enable); + if (ret) + return ret; + + /* Read flash ID to make sure the switch was successful. */ + if (enable) { + naddr = nor->addr_nbytes; + ndummy = 3; + proto = SNOR_PROTO_8_8_8_DTR; + } else { + naddr = 0; + ndummy = 0; + proto = SNOR_PROTO_1_1_1; + } + + ret = spi_nor_read_id(nor, naddr, ndummy, nor->bouncebuf, proto); + if (ret) { + dev_dbg(nor->dev, "error %d reading JEDEC ID after %s 8D-8D-8D mode\n", + ret, enable ? "enabling" : "disabling"); + return ret; + } + + if (memcmp(nor->bouncebuf, nor->info->id, nor->info->id_len)) + return -EINVAL; + + return 0; } static int s28hx_t_post_sfdp_fixup(struct spi_nor *nor)