| Message ID | A765B125120D1346A63912DDE6D8B6315DCED6@NTXXIAMBX02.xacn.micron.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sunday, September 28, 2014 at 03:59:42 AM, bpqw wrote: > For Micron spi norflash,you can enable > Quad spi transfer by clear EVCR(Enhanced > Volatile Configuration Register) Quad I/O > protocol bit. OK, this information is nice and all, but what does this patch do? I can't learn this information from the commit message as it is, can I ? And , the purpose of the commit message is exactly to summarize the change the patch implements. > Signed-off-by: bean huo <beanhuo@micron.com> > --- > v1-v2:modified to that capture wait_till_ready() > return value,if error,directly return its > the value. > > drivers/mtd/spi-nor/spi-nor.c | 46 > +++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | > 6 ++++++ > 2 files changed, 52 insertions(+) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index b5ad6be..0c3b4fd 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -878,6 +878,45 @@ static int spansion_quad_enable(struct spi_nor *nor) > return 0; > } > > +static int micron_quad_enable(struct spi_nor *nor) > +{ > + int ret, val; > + > + ret = nor->read_reg(nor, SPINOR_OP_RD_EVCR, &val, 1); > + if (ret < 0) { > + dev_err(nor->dev, "error %d reading EVCR\n", ret); > + return -EINVAL; > + } > + > + write_enable(nor); > + > + /* set EVCR ,enable quad I/O */ > + nor->cmd_buf[0] = val & ~EVCR_QUAD_EN_MICRON; > + ret = nor->write_reg(nor, SPINOR_OP_WD_EVCR, nor->cmd_buf, 1, 0); > + if (ret < 0) { > + dev_err(nor->dev, > + "error while writing EVCR register\n"); > + return -EINVAL; Why not just "return ret;" ? [...]
>> For Micron spi norflash,you can enable Quad spi transfer by clear >> EVCR(Enhanced Volatile Configuration Register) Quad I/O protocol bit. > >OK, this information is nice and all, but what does this patch do? I can't learn this information from the commit message as it is, can I ? >And , the purpose of the commit message is exactly to summarize the change the patch implements. you don't understand what purpose of this patch! just as subject and commit message described, it is for enable Micron Quad spi transfer mode.do you read the spi-nor.c file? please pay attention to the set_quad_mode() function.by the way,I can add more commit message for it,but I think it is redundant,don't need. >> +static int micron_quad_enable(struct spi_nor *nor) { >> + int ret, val; >> + >> + ret = nor->read_reg(nor, SPINOR_OP_RD_EVCR, &val, 1); >> + if (ret < 0) { >> + dev_err(nor->dev, "error %d reading EVCR\n", ret); >> + return -EINVAL; >> + } >> + >> + write_enable(nor); >> + >> + /* set EVCR ,enable quad I/O */ >> + nor->cmd_buf[0] = val & ~EVCR_QUAD_EN_MICRON; >> + ret = nor->write_reg(nor, SPINOR_OP_WD_EVCR, nor->cmd_buf, 1, 0); >> + if (ret < 0) { >> + dev_err(nor->dev, >> + "error while writing EVCR register\n"); >> + return -EINVAL; >Why not just "return ret;" ? >[...] Ok,this is good,I will modify it in the next patch version.thanks.
On Monday, September 29, 2014 at 02:30:04 AM, bpqw wrote: > >> For Micron spi norflash,you can enable Quad spi transfer by clear > >> EVCR(Enhanced Volatile Configuration Register) Quad I/O protocol bit. > > > >OK, this information is nice and all, but what does this patch do? I can't > >learn this information from the commit message as it is, can I ? And , > >the purpose of the commit message is exactly to summarize the change the > >patch implements. > > you don't understand what purpose of this patch! Well, I dare to say, reacting to feedback like you just did won't make you many allies around here. > just as subject and commit > message described, it is for enable Micron Quad spi transfer mode. I understand the subject part. The commit message, on the other hand, just states that it is possible to frob with a certain register to achieve a certain effect ; the commit message does not state what this patch does or how is the patch useful. Does this patch enable the bit or does it disable the bit ? I cannot tell without looking into the code , I really have no clue just by reading the subject and the commit message. > do you > read the spi-nor.c file? No, I didn't even look at the code. > please pay attention to the set_quad_mode() > function. No, what set_quad_mode_function() are you talking about ... > by the way,I can add more commit message for it,but I think it is > redundant,don't need. The commit message shall state what the patch does in the first place, what the hardware can do is ortogonal to that. The commit message can be as short as: The hardware supports 4-bit I/O when bit FOO is set in register BAR. This patch adds function that sets bit FOO in register BAR to enable 4-bit I/O if condition BAZ and QUUX are met. Then I do not even have to look at the code if I want to just get the high-level overview of what the patch does. If I want to know the details, I will look into the code. Do you know what I'm getting at ? [...] Best regards, Marek Vasut
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index b5ad6be..0c3b4fd 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -878,6 +878,45 @@ static int spansion_quad_enable(struct spi_nor *nor) return 0; } +static int micron_quad_enable(struct spi_nor *nor) +{ + int ret, val; + + ret = nor->read_reg(nor, SPINOR_OP_RD_EVCR, &val, 1); + if (ret < 0) { + dev_err(nor->dev, "error %d reading EVCR\n", ret); + return -EINVAL; + } + + write_enable(nor); + + /* set EVCR ,enable quad I/O */ + nor->cmd_buf[0] = val & ~EVCR_QUAD_EN_MICRON; + ret = nor->write_reg(nor, SPINOR_OP_WD_EVCR, nor->cmd_buf, 1, 0); + if (ret < 0) { + dev_err(nor->dev, + "error while writing EVCR register\n"); + return -EINVAL; + } + + ret = wait_till_ready(nor); + if (ret) + return ret; + + /* read EVCR and check it */ + ret = nor->read_reg(nor, SPINOR_OP_RD_EVCR, &val, 1); + if (ret < 0) { + dev_err(nor->dev, "error %d reading EVCR\n", ret); + return -EINVAL; + } + if (val & EVCR_QUAD_EN_MICRON) { + dev_err(nor->dev, "Micron EVCR Quad bit not clear\n"); + return -EINVAL; + } + + return 0; +} + static int set_quad_mode(struct spi_nor *nor, u32 jedec_id) { int status; @@ -890,6 +929,13 @@ static int set_quad_mode(struct spi_nor *nor, u32 jedec_id) return -EINVAL; } return status; + case CFI_MFR_ST: + status = micron_quad_enable(nor); + if (status) { + dev_err(nor->dev, "Micron quad-read not enabled\n"); + return -EINVAL; + } + return status; default: status = spansion_quad_enable(nor); if (status) { diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 9e6294f..d71b659 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -56,6 +56,10 @@ /* Used for Spansion flashes only. */ #define SPINOR_OP_BRWR 0x17 /* Bank register write */ +/* Used for Micron flashes only. */ +#define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */ +#define SPINOR_OP_WD_EVCR 0x61 /* Write EVCR register */ + /* Status Register bits. */ #define SR_WIP 1 /* Write in progress */ #define SR_WEL 2 /* Write enable latch */ @@ -67,6 +71,8 @@ #define SR_QUAD_EN_MX 0x40 /* Macronix Quad I/O */ +#define EVCR_QUAD_EN_MICRON 0x80 /* Micron Quad I/O */ + /* Flag Status Register bits */ #define FSR_READY 0x80
For Micron spi norflash,you can enable Quad spi transfer by clear EVCR(Enhanced Volatile Configuration Register) Quad I/O protocol bit. Signed-off-by: bean huo <beanhuo@micron.com> --- v1-v2:modified to that capture wait_till_ready() return value,if error,directly return its the value. drivers/mtd/spi-nor/spi-nor.c | 46 +++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 6 ++++++ 2 files changed, 52 insertions(+)