| Message ID | b0d8470fdd46e89ef692e27dc2600b880c1d5dde.1440410236.git.cyrille.pitchen@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday, August 24, 2015 at 12:13:58 PM, Cyrille Pitchen wrote: > The number of dummy cycles used during Fast Read commands can be reduced > to improve transfer performances. Each manufacturer has a dedicated set of > registers to provide the memory with the exact number of dummy cycles it > should expect. Both the memory and the (Q)SPI controller must agree on > this number of dummy cycles. > > The number of dummy cycles can be found into the memory datasheet and > mostly depends on the SPI clock frequency, the Fast Read op code and the > Single/Dual Data Rate mode. > > Probing JEDEC Serial Flash Discoverable Parameters (SFDP) tables would > only provide the driver with a high enough number of dummy cycles for each > Fast Read command to be used for all clock frequencies: this solution > would not be optimized. > > Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> Hi! > drivers/mtd/spi-nor/spi-nor.c | 97 > ++++++++++++++++++++++++++++++++++--------- include/linux/mtd/spi-nor.h > | 2 + > 2 files changed, 80 insertions(+), 19 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index e2a6029dc056..869e098a6841 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -119,24 +119,6 @@ static int read_cr(struct spi_nor *nor) > } > > /* > - * Dummy Cycle calculation for different type of read. > - * It can be used to support more commands with > - * different dummy cycle requirements. > - */ > -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) > -{ > - switch (nor->flash_read) { > - case SPI_NOR_FAST: > - case SPI_NOR_DUAL: > - case SPI_NOR_QUAD: > - return 8; > - case SPI_NOR_NORMAL: > - return 0; > - } > - return 0; > -} You can probably just soup up this function so that it sets the nor->read_dummy, no ? > -/* > * Write status register 1 byte > * Returns negative if error occurred. > */ > @@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor, struct > flash_info *info) } > } > > +static int micron_set_dummy_cycles(struct spi_nor *nor) > +{ > + int ret; > + u8 val, mask; > + > + /* read the Volatile Configuration Register (VCR) */ NIT: If this is a sentence, start it with capital letter and end it with fullstop :) > + ret = nor->read_reg(nor, SPINOR_OP_RD_VCR, &val, 1); > + if (ret < 0) { > + dev_err(nor->dev, "error %d reading VCR\n", ret); > + return ret; > + } > + > + write_enable(nor); > + > + /* update the number of dummy into the VCR */ DTTO > + mask = GENMASK(7, 4); > + val &= ~mask; > + val |= (nor->read_dummy << 4) & mask; > + ret = nor->write_reg(nor, SPINOR_OP_WR_VCR, &val, 1, 0); > + if (ret < 0) { > + dev_err(nor->dev, "error while writing VCR register\n"); > + return ret; > + } > + > + ret = spi_nor_wait_till_ready(nor); > + if (ret) > + return ret; > + > + return 0; > +} > + > +/* > + * Dummy Cycle calculation for different type of read. > + * It can be used to support more commands with > + * different dummy cycle requirements. > + */ > +static int spi_nor_read_dummy_cycles(struct spi_nor *nor, > + const struct flash_info *info) > +{ > + struct device_node *np = nor->dev->of_node; > + u32 num_dummy_cycles; > + > + if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles", > + &num_dummy_cycles)) { > + nor->read_dummy = num_dummy_cycles; > + > + /* > + * This switch block might be moved after the if...then...else > + * statement but it was not tested with all Spansion or Micron > + * memories. > + * Now the "m25p,num-dummy-cycles" property needs to be > + * explicitly set in the device tree so the switch statement is > + * executed. This should avoid unwanted side effects and keep > + * backward compatibility. > + */ > + switch (JEDEC_MFR(info)) { > + case CFI_MFR_ST: > + return micron_set_dummy_cycles(nor); > + default: If you do have m25p,num-dummy-cycles set for non-micron flash, you have a problem here I believe. > + break; > + } > + } else { The solution would be to drop this else {} bit here, so that if you fail in the DT-based configuration, you fall back to this old behavior. What do you think please ? :) > + switch (nor->flash_read) { > + case SPI_NOR_FAST: > + case SPI_NOR_DUAL: > + case SPI_NOR_QUAD: > + nor->read_dummy = 8; > + case SPI_NOR_NORMAL: > + nor->read_dummy = 0; > + } > + } > + > + return 0; > +} [...]
Hi Marek, Le 24/08/2015 12:48, Marek Vasut a écrit : > On Monday, August 24, 2015 at 12:13:58 PM, Cyrille Pitchen wrote: >> The number of dummy cycles used during Fast Read commands can be reduced >> to improve transfer performances. Each manufacturer has a dedicated set of >> registers to provide the memory with the exact number of dummy cycles it >> should expect. Both the memory and the (Q)SPI controller must agree on >> this number of dummy cycles. >> >> The number of dummy cycles can be found into the memory datasheet and >> mostly depends on the SPI clock frequency, the Fast Read op code and the >> Single/Dual Data Rate mode. >> >> Probing JEDEC Serial Flash Discoverable Parameters (SFDP) tables would >> only provide the driver with a high enough number of dummy cycles for each >> Fast Read command to be used for all clock frequencies: this solution >> would not be optimized. >> >> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> > > Hi! > >> drivers/mtd/spi-nor/spi-nor.c | 97 >> ++++++++++++++++++++++++++++++++++--------- include/linux/mtd/spi-nor.h >> | 2 + >> 2 files changed, 80 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> index e2a6029dc056..869e098a6841 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -119,24 +119,6 @@ static int read_cr(struct spi_nor *nor) >> } >> >> /* >> - * Dummy Cycle calculation for different type of read. >> - * It can be used to support more commands with >> - * different dummy cycle requirements. >> - */ >> -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) >> -{ >> - switch (nor->flash_read) { >> - case SPI_NOR_FAST: >> - case SPI_NOR_DUAL: >> - case SPI_NOR_QUAD: >> - return 8; >> - case SPI_NOR_NORMAL: >> - return 0; >> - } >> - return 0; >> -} > > You can probably just soup up this function so that it sets the > nor->read_dummy, no ? > Actually, this is what the patch does: spi_nor_read_dummy_cycles() was reused and enhanced few lines below where you've pointed out the "switch (nor->flash_read)" block should be move after the else block. I think when I wrote the code I've chosen to move the definition of this function instead of adding forward declarations of functions such as read_cr() or write_sr_cr(), which are now called by micron_set_dummy_cycles(). >> -/* >> * Write status register 1 byte >> * Returns negative if error occurred. >> */ >> @@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor, struct >> flash_info *info) } >> } >> >> +static int micron_set_dummy_cycles(struct spi_nor *nor) >> +{ >> + int ret; >> + u8 val, mask; >> + >> + /* read the Volatile Configuration Register (VCR) */ > > NIT: If this is a sentence, start it with capital letter and end it with > fullstop :) > done for the next version >> + ret = nor->read_reg(nor, SPINOR_OP_RD_VCR, &val, 1); >> + if (ret < 0) { >> + dev_err(nor->dev, "error %d reading VCR\n", ret); >> + return ret; >> + } >> + >> + write_enable(nor); >> + >> + /* update the number of dummy into the VCR */ > > DTTO > done for the next version >> + mask = GENMASK(7, 4); >> + val &= ~mask; >> + val |= (nor->read_dummy << 4) & mask; >> + ret = nor->write_reg(nor, SPINOR_OP_WR_VCR, &val, 1, 0); >> + if (ret < 0) { >> + dev_err(nor->dev, "error while writing VCR register\n"); >> + return ret; >> + } >> + >> + ret = spi_nor_wait_till_ready(nor); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +/* >> + * Dummy Cycle calculation for different type of read. >> + * It can be used to support more commands with >> + * different dummy cycle requirements. >> + */ >> +static int spi_nor_read_dummy_cycles(struct spi_nor *nor, >> + const struct flash_info *info) >> +{ >> + struct device_node *np = nor->dev->of_node; >> + u32 num_dummy_cycles; >> + >> + if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles", >> + &num_dummy_cycles)) { >> + nor->read_dummy = num_dummy_cycles; >> + >> + /* >> + * This switch block might be moved after the if...then...else >> + * statement but it was not tested with all Spansion or Micron >> + * memories. >> + * Now the "m25p,num-dummy-cycles" property needs to be >> + * explicitly set in the device tree so the switch statement is >> + * executed. This should avoid unwanted side effects and keep >> + * backward compatibility. >> + */ >> + switch (JEDEC_MFR(info)) { >> + case CFI_MFR_ST: >> + return micron_set_dummy_cycles(nor); >> + default: > > If you do have m25p,num-dummy-cycles set for non-micron flash, you have a > problem here I believe. > >> + break; >> + } >> + } else { > > The solution would be to drop this else {} bit here, so that if you fail in > the DT-based configuration, you fall back to this old behavior. What do you > think please ? :) > Good idea! I also add a trace for the default case of "switch (JEDEC_MFR(info))": dev_warn(dev, "can't set the number of dummy cycles\n"); So the user is notified that the driver could not use the value of "m25p,num-dummy-cycles" from the DT before falling back to the legacy code. >> + switch (nor->flash_read) { >> + case SPI_NOR_FAST: >> + case SPI_NOR_DUAL: >> + case SPI_NOR_QUAD: >> + nor->read_dummy = 8; >> + case SPI_NOR_NORMAL: >> + nor->read_dummy = 0; >> + } >> + } >> + >> + return 0; >> +} > > [...] > thanks for the review! Best regards, Cyrille
On Monday, August 24, 2015 at 06:42:46 PM, Cyrille Pitchen wrote: > Hi Marek, Hi! [...] > >> - * Dummy Cycle calculation for different type of read. > >> - * It can be used to support more commands with > >> - * different dummy cycle requirements. > >> - */ > >> -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) > >> -{ > >> - switch (nor->flash_read) { > >> - case SPI_NOR_FAST: > >> - case SPI_NOR_DUAL: > >> - case SPI_NOR_QUAD: > >> - return 8; > >> - case SPI_NOR_NORMAL: > >> - return 0; > >> - } > >> - return 0; > >> -} > > > > You can probably just soup up this function so that it sets the > > nor->read_dummy, no ? > > Actually, this is what the patch does: spi_nor_read_dummy_cycles() was > reused and enhanced few lines below where you've pointed out the > "switch (nor->flash_read)" block should be move after the else block. You know what? I'll go get some sleep, coffee doesn't cut it anymore :) > I think when I wrote the code I've chosen to move the definition of this > function instead of adding forward declarations of functions such as > read_cr() or write_sr_cr(), which are now called by > micron_set_dummy_cycles(). Yep, that's all right, sorry for the confusion. > >> -/* > >> > >> * Write status register 1 byte > >> * Returns negative if error occurred. > >> */ > >> > >> @@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor, > >> struct flash_info *info) } > >> > >> } [...] > >> +/* > >> + * Dummy Cycle calculation for different type of read. > >> + * It can be used to support more commands with > >> + * different dummy cycle requirements. > >> + */ > >> +static int spi_nor_read_dummy_cycles(struct spi_nor *nor, > >> + const struct flash_info *info) > >> +{ > >> + struct device_node *np = nor->dev->of_node; > >> + u32 num_dummy_cycles; > >> + > >> + if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles", > >> + &num_dummy_cycles)) { > >> + nor->read_dummy = num_dummy_cycles; > >> + > >> + /* > >> + * This switch block might be moved after the if...then...else > >> + * statement but it was not tested with all Spansion or Micron > >> + * memories. > >> + * Now the "m25p,num-dummy-cycles" property needs to be > >> + * explicitly set in the device tree so the switch statement is > >> + * executed. This should avoid unwanted side effects and keep > >> + * backward compatibility. > >> + */ > >> + switch (JEDEC_MFR(info)) { > >> + case CFI_MFR_ST: > >> + return micron_set_dummy_cycles(nor); > > > >> + default: > > If you do have m25p,num-dummy-cycles set for non-micron flash, you have a > > problem here I believe. > > > >> + break; > >> + } > >> + } else { > > > > The solution would be to drop this else {} bit here, so that if you fail > > in the DT-based configuration, you fall back to this old behavior. What > > do you think please ? :) > > Good idea! > I also add a trace for the default case of "switch (JEDEC_MFR(info))": > > dev_warn(dev, "can't set the number of dummy cycles\n"); Maybe change this to "setting the number of dummy cycles not supported by chip, ignoring" or something, to be explicit about the fallback and that this is not supported by the chip. But this is just an idea, feel free to ignore it. > So the user is notified that the driver could not use the value of > "m25p,num-dummy-cycles" from the DT before falling back to the legacy > code. Yup. > >> + switch (nor->flash_read) { > >> + case SPI_NOR_FAST: > >> + case SPI_NOR_DUAL: > >> + case SPI_NOR_QUAD: > >> + nor->read_dummy = 8; > >> + case SPI_NOR_NORMAL: > >> + nor->read_dummy = 0; > >> + } > >> + } > >> + > >> + return 0; > >> +} > > > > [...] > > thanks for the review! Im glad it helped ;-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index e2a6029dc056..869e098a6841 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -119,24 +119,6 @@ static int read_cr(struct spi_nor *nor) } /* - * Dummy Cycle calculation for different type of read. - * It can be used to support more commands with - * different dummy cycle requirements. - */ -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) -{ - switch (nor->flash_read) { - case SPI_NOR_FAST: - case SPI_NOR_DUAL: - case SPI_NOR_QUAD: - return 8; - case SPI_NOR_NORMAL: - return 0; - } - return 0; -} - -/* * Write status register 1 byte * Returns negative if error occurred. */ @@ -1012,6 +994,81 @@ static int set_quad_mode(struct spi_nor *nor, struct flash_info *info) } } +static int micron_set_dummy_cycles(struct spi_nor *nor) +{ + int ret; + u8 val, mask; + + /* read the Volatile Configuration Register (VCR) */ + ret = nor->read_reg(nor, SPINOR_OP_RD_VCR, &val, 1); + if (ret < 0) { + dev_err(nor->dev, "error %d reading VCR\n", ret); + return ret; + } + + write_enable(nor); + + /* update the number of dummy into the VCR */ + mask = GENMASK(7, 4); + val &= ~mask; + val |= (nor->read_dummy << 4) & mask; + ret = nor->write_reg(nor, SPINOR_OP_WR_VCR, &val, 1, 0); + if (ret < 0) { + dev_err(nor->dev, "error while writing VCR register\n"); + return ret; + } + + ret = spi_nor_wait_till_ready(nor); + if (ret) + return ret; + + return 0; +} + +/* + * Dummy Cycle calculation for different type of read. + * It can be used to support more commands with + * different dummy cycle requirements. + */ +static int spi_nor_read_dummy_cycles(struct spi_nor *nor, + const struct flash_info *info) +{ + struct device_node *np = nor->dev->of_node; + u32 num_dummy_cycles; + + if (np && !of_property_read_u32(np, "m25p,num-dummy-cycles", + &num_dummy_cycles)) { + nor->read_dummy = num_dummy_cycles; + + /* + * This switch block might be moved after the if...then...else + * statement but it was not tested with all Spansion or Micron + * memories. + * Now the "m25p,num-dummy-cycles" property needs to be + * explicitly set in the device tree so the switch statement is + * executed. This should avoid unwanted side effects and keep + * backward compatibility. + */ + switch (JEDEC_MFR(info)) { + case CFI_MFR_ST: + return micron_set_dummy_cycles(nor); + default: + break; + } + } else { + switch (nor->flash_read) { + case SPI_NOR_FAST: + case SPI_NOR_DUAL: + case SPI_NOR_QUAD: + nor->read_dummy = 8; + case SPI_NOR_NORMAL: + nor->read_dummy = 0; + } + } + + return 0; +} + static int spi_nor_check(struct spi_nor *nor) { if (!nor->dev || !nor->read || !nor->write || @@ -1216,7 +1273,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) nor->addr_width = 3; } - nor->read_dummy = spi_nor_read_dummy_cycles(nor); + ret = spi_nor_read_dummy_cycles(nor, info); + if (ret) + return ret; dev_info(dev, "%s (%lld Kbytes)\n", id->name, (long long)mtd->size >> 10); diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 1bf6f11310ef..e03a4c4053d3 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -59,6 +59,8 @@ /* Used for Micron flashes only. */ #define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */ #define SPINOR_OP_WD_EVCR 0x61 /* Write EVCR register */ +#define SPINOR_OP_RD_VCR 0x85 /* Read VCR register */ +#define SPINOR_OP_WR_VCR 0x81 /* Write VCR register */ /* Status Register bits. */ #define SR_WIP 1 /* Write in progress */
The number of dummy cycles used during Fast Read commands can be reduced to improve transfer performances. Each manufacturer has a dedicated set of registers to provide the memory with the exact number of dummy cycles it should expect. Both the memory and the (Q)SPI controller must agree on this number of dummy cycles. The number of dummy cycles can be found into the memory datasheet and mostly depends on the SPI clock frequency, the Fast Read op code and the Single/Dual Data Rate mode. Probing JEDEC Serial Flash Discoverable Parameters (SFDP) tables would only provide the driver with a high enough number of dummy cycles for each Fast Read command to be used for all clock frequencies: this solution would not be optimized. Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> --- drivers/mtd/spi-nor/spi-nor.c | 97 ++++++++++++++++++++++++++++++++++--------- include/linux/mtd/spi-nor.h | 2 + 2 files changed, 80 insertions(+), 19 deletions(-)