Re: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
Hi, Cyrille Finally, I get your comments, thanks. >Hi Bean, > >Le 11/11/2017 à 21:49, Bean Huo (beanhuo) a écrit : >> For the Micron SPI NOR, when the erase/program operation fails, >> especially, > >To be verified but I think you'd rather remove "the" words in this case: > >"For Micron SPI NOR memories, when erase/program operation fails, >especially ..." > >Maybe no comma after "especially". > >> for the failure results from intending to modify protected space, >> spi-nor upper layers still get the return which shows the operation succeeds. >> this because spi_nor_fsr_ready() only uses bit.7 to device whether ready. > >"This": missing capital letter and maybe a verb too. > >> For the most cases, even the error of erase/program occurs, SPI NOR >> device is still ready. The device ready and the error are two different >> cases. > When the program/erase failed, or there is the error during Erasing/programming, user space always gets the status of the previous operation is successful. Because current spi_nor_fsr_ready() only checks FSR ready bit. Even if failure/error happened, spi nor device still can go into the ready stage. This is what I want to say. So, the device ready and the operation failure are two different cases. >I don't really understand what you mean here. > >> This patch is to fixup this issue and adding FSR (flag status >> register) > >This patch fixes the issue by checking relevant bits in the FSR. > >> error bits checkup. >> The FSR(flag status register) is a powerful tool to investigate the >> staus > >"The FSR (flag status register)": please insert a space ' '. > >s/staus/status/ > > >> of device,checking information regarding what is actually doing the >> memory > >"of device, checking": missing space >> and detecting possible error conditions. >> > >Globally, I think you need to reword your commit message. IMHO, it is not >clear though I think I've mostly understood what you meant reading the >actual source code. > I am not a native English speaker, hopefully I can make big progress on writing. >> Signed-off-by: beanhuo>> --- >> drivers/mtd/spi-nor/spi-nor.c | 19 +-- >> include/linux/mtd/spi-nor.h | 6 +- >> 2 files changed, 22 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c >> b/drivers/mtd/spi-nor/spi-nor.c index bc266f7..200e814 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor >*nor) >> int fsr = read_fsr(nor); >> if (fsr < 0) >> return fsr; >> -else >> -return fsr & FSR_READY; >> + >> +if (fsr & (FSR_E_ERR | FSR_P_ERR)) { >> +if (fsr & FSR_E_ERR) >> +dev_err(nor->dev, "Erase operation failed.\n"); >> +else >> +dev_err(nor->dev, "Program operation failed.\n"); >> + >> +if (fsr & FSR_PT_ERR) >> +dev_err(nor->dev, >> +"The operation has attempted to modify the >protected" > >A space ' ' is missing after "protected". >Also please verify next version passes the checkpatch test because this >version doesn't. > Yes, I already re-sent this patch which merged these two line into one line. >I think you should check the verb tense consistency: >[...] operation failed. The operation attempted [...] > >Also maybe you should write "a protected sector" or "some protected sector": >"the protected sector" sounds like there is only one protected sector. > >> +"sector or the locked OPT space.\n"); > >You meant OTP (One Time Programmable), didn't you ? s/OPT/OTP/ > Yes, it is one time programmable space. >> + >> +nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0); >> +return -EIO; >> +} >> + >> +return fsr & FSR_READY; >> } >> >> static int spi_nor_ready(struct spi_nor *nor) diff --git >> a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index >> d0c66a0..46b5608 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -61,6 +61,7 @@ >> #define SPINOR_OP_RDSFDP0x5a/* Read SFDP */ >> #define SPINOR_OP_RDCR 0x35/* Read configuration register >*/ >> #define SPINOR_OP_RDFSR 0x70/* Read flag status register */ >> +#define SPINOR_OP_CLFSR 0x50/* Clear flag status register */ >> >> /* 4-byte address opcodes - used on Spansion and some Macronix flashes. >*/ >> #define SPINOR_OP_READ_4B 0x13/* Read data bytes (low >frequency) */ >> @@ -130,7 +131,10 @@ >> #define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */ >> >> /* Flag Status Register bits */ >> -#define FSR_READY BIT(7) >> +#define FSR_READY BIT(7) /* Device status, 0 = Busy,1 = Ready */ > >You may insert a space ' ' between "Busy," and "1 = " > I thought we care more on the code, rather than comment, thanks for reviewing.
Re: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
Hi, Cyrille Finally, I get your comments, thanks. >Hi Bean, > >Le 11/11/2017 à 21:49, Bean Huo (beanhuo) a écrit : >> For the Micron SPI NOR, when the erase/program operation fails, >> especially, > >To be verified but I think you'd rather remove "the" words in this case: > >"For Micron SPI NOR memories, when erase/program operation fails, >especially ..." > >Maybe no comma after "especially". > >> for the failure results from intending to modify protected space, >> spi-nor upper layers still get the return which shows the operation succeeds. >> this because spi_nor_fsr_ready() only uses bit.7 to device whether ready. > >"This": missing capital letter and maybe a verb too. > >> For the most cases, even the error of erase/program occurs, SPI NOR >> device is still ready. The device ready and the error are two different >> cases. > When the program/erase failed, or there is the error during Erasing/programming, user space always gets the status of the previous operation is successful. Because current spi_nor_fsr_ready() only checks FSR ready bit. Even if failure/error happened, spi nor device still can go into the ready stage. This is what I want to say. So, the device ready and the operation failure are two different cases. >I don't really understand what you mean here. > >> This patch is to fixup this issue and adding FSR (flag status >> register) > >This patch fixes the issue by checking relevant bits in the FSR. > >> error bits checkup. >> The FSR(flag status register) is a powerful tool to investigate the >> staus > >"The FSR (flag status register)": please insert a space ' '. > >s/staus/status/ > > >> of device,checking information regarding what is actually doing the >> memory > >"of device, checking": missing space >> and detecting possible error conditions. >> > >Globally, I think you need to reword your commit message. IMHO, it is not >clear though I think I've mostly understood what you meant reading the >actual source code. > I am not a native English speaker, hopefully I can make big progress on writing. >> Signed-off-by: beanhuo >> --- >> drivers/mtd/spi-nor/spi-nor.c | 19 +-- >> include/linux/mtd/spi-nor.h | 6 +- >> 2 files changed, 22 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c >> b/drivers/mtd/spi-nor/spi-nor.c index bc266f7..200e814 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor >*nor) >> int fsr = read_fsr(nor); >> if (fsr < 0) >> return fsr; >> -else >> -return fsr & FSR_READY; >> + >> +if (fsr & (FSR_E_ERR | FSR_P_ERR)) { >> +if (fsr & FSR_E_ERR) >> +dev_err(nor->dev, "Erase operation failed.\n"); >> +else >> +dev_err(nor->dev, "Program operation failed.\n"); >> + >> +if (fsr & FSR_PT_ERR) >> +dev_err(nor->dev, >> +"The operation has attempted to modify the >protected" > >A space ' ' is missing after "protected". >Also please verify next version passes the checkpatch test because this >version doesn't. > Yes, I already re-sent this patch which merged these two line into one line. >I think you should check the verb tense consistency: >[...] operation failed. The operation attempted [...] > >Also maybe you should write "a protected sector" or "some protected sector": >"the protected sector" sounds like there is only one protected sector. > >> +"sector or the locked OPT space.\n"); > >You meant OTP (One Time Programmable), didn't you ? s/OPT/OTP/ > Yes, it is one time programmable space. >> + >> +nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0); >> +return -EIO; >> +} >> + >> +return fsr & FSR_READY; >> } >> >> static int spi_nor_ready(struct spi_nor *nor) diff --git >> a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index >> d0c66a0..46b5608 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -61,6 +61,7 @@ >> #define SPINOR_OP_RDSFDP0x5a/* Read SFDP */ >> #define SPINOR_OP_RDCR 0x35/* Read configuration register >*/ >> #define SPINOR_OP_RDFSR 0x70/* Read flag status register */ >> +#define SPINOR_OP_CLFSR 0x50/* Clear flag status register */ >> >> /* 4-byte address opcodes - used on Spansion and some Macronix flashes. >*/ >> #define SPINOR_OP_READ_4B 0x13/* Read data bytes (low >frequency) */ >> @@ -130,7 +131,10 @@ >> #define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */ >> >> /* Flag Status Register bits */ >> -#define FSR_READY BIT(7) >> +#define FSR_READY BIT(7) /* Device status, 0 = Busy,1 = Ready */ > >You may insert a space ' ' between "Busy," and "1 = " > I thought we care more on the code, rather than comment, thanks for reviewing. >Best regards, >
Re: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
Hi Bean, Le 11/11/2017 à 21:49, Bean Huo (beanhuo) a écrit : > For the Micron SPI NOR, when the erase/program operation fails, especially, To be verified but I think you'd rather remove "the" words in this case: "For Micron SPI NOR memories, when erase/program operation fails, especially ..." Maybe no comma after "especially". > for the failure results from intending to modify protected space, spi-nor > upper layers still get the return which shows the operation succeeds. > this because spi_nor_fsr_ready() only uses bit.7 to device whether ready. "This": missing capital letter and maybe a verb too. > For the most cases, even the error of erase/program occurs, SPI NOR device > is still ready. The device ready and the error are two different cases. I don't really understand what you mean here. > This patch is to fixup this issue and adding FSR (flag status register) This patch fixes the issue by checking relevant bits in the FSR. > error bits checkup. > The FSR(flag status register) is a powerful tool to investigate the staus "The FSR (flag status register)": please insert a space ' '. s/staus/status/ > of device,checking information regarding what is actually doing the memory "of device, checking": missing space > and detecting possible error conditions. > Globally, I think you need to reword your commit message. IMHO, it is not clear though I think I've mostly understood what you meant reading the actual source code. > Signed-off-by: beanhuo> --- > drivers/mtd/spi-nor/spi-nor.c | 19 +-- > include/linux/mtd/spi-nor.h | 6 +- > 2 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index bc266f7..200e814 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor) > int fsr = read_fsr(nor); > if (fsr < 0) > return fsr; > - else > - return fsr & FSR_READY; > + > + if (fsr & (FSR_E_ERR | FSR_P_ERR)) { > + if (fsr & FSR_E_ERR) > + dev_err(nor->dev, "Erase operation failed.\n"); > + else > + dev_err(nor->dev, "Program operation failed.\n"); > + > + if (fsr & FSR_PT_ERR) > + dev_err(nor->dev, > + "The operation has attempted to modify the protected" A space ' ' is missing after "protected". Also please verify next version passes the checkpatch test because this version doesn't. I think you should check the verb tense consistency: [...] operation failed. The operation attempted [...] Also maybe you should write "a protected sector" or "some protected sector": "the protected sector" sounds like there is only one protected sector. > + "sector or the locked OPT space.\n"); You meant OTP (One Time Programmable), didn't you ? s/OPT/OTP/ > + > + nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0); > + return -EIO; > + } > + > + return fsr & FSR_READY; > } > > static int spi_nor_ready(struct spi_nor *nor) > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index d0c66a0..46b5608 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -61,6 +61,7 @@ > #define SPINOR_OP_RDSFDP 0x5a/* Read SFDP */ > #define SPINOR_OP_RDCR 0x35/* Read configuration register > */ > #define SPINOR_OP_RDFSR 0x70/* Read flag status register */ > +#define SPINOR_OP_CLFSR 0x50/* Clear flag status register */ > > /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */ > #define SPINOR_OP_READ_4B0x13/* Read data bytes (low frequency) */ > @@ -130,7 +131,10 @@ > #define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */ > > /* Flag Status Register bits */ > -#define FSR_READYBIT(7) > +#define FSR_READYBIT(7) /* Device status, 0 = Busy,1 = Ready */ You may insert a space ' ' between "Busy," and "1 = " Best regards, Cyrille > +#define FSR_E_ERRBIT(5) /* Erase operation status */ > +#define FSR_P_ERRBIT(4) /* Program operation status */ > +#define FSR_PT_ERR BIT(1) /* Protection error bit */ > > /* Configuration Register bits. */ > #define CR_QUAD_EN_SPAN BIT(1) /* Spansion Quad I/O */ >
Re: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
Hi Bean, Le 11/11/2017 à 21:49, Bean Huo (beanhuo) a écrit : > For the Micron SPI NOR, when the erase/program operation fails, especially, To be verified but I think you'd rather remove "the" words in this case: "For Micron SPI NOR memories, when erase/program operation fails, especially ..." Maybe no comma after "especially". > for the failure results from intending to modify protected space, spi-nor > upper layers still get the return which shows the operation succeeds. > this because spi_nor_fsr_ready() only uses bit.7 to device whether ready. "This": missing capital letter and maybe a verb too. > For the most cases, even the error of erase/program occurs, SPI NOR device > is still ready. The device ready and the error are two different cases. I don't really understand what you mean here. > This patch is to fixup this issue and adding FSR (flag status register) This patch fixes the issue by checking relevant bits in the FSR. > error bits checkup. > The FSR(flag status register) is a powerful tool to investigate the staus "The FSR (flag status register)": please insert a space ' '. s/staus/status/ > of device,checking information regarding what is actually doing the memory "of device, checking": missing space > and detecting possible error conditions. > Globally, I think you need to reword your commit message. IMHO, it is not clear though I think I've mostly understood what you meant reading the actual source code. > Signed-off-by: beanhuo > --- > drivers/mtd/spi-nor/spi-nor.c | 19 +-- > include/linux/mtd/spi-nor.h | 6 +- > 2 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index bc266f7..200e814 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor) > int fsr = read_fsr(nor); > if (fsr < 0) > return fsr; > - else > - return fsr & FSR_READY; > + > + if (fsr & (FSR_E_ERR | FSR_P_ERR)) { > + if (fsr & FSR_E_ERR) > + dev_err(nor->dev, "Erase operation failed.\n"); > + else > + dev_err(nor->dev, "Program operation failed.\n"); > + > + if (fsr & FSR_PT_ERR) > + dev_err(nor->dev, > + "The operation has attempted to modify the protected" A space ' ' is missing after "protected". Also please verify next version passes the checkpatch test because this version doesn't. I think you should check the verb tense consistency: [...] operation failed. The operation attempted [...] Also maybe you should write "a protected sector" or "some protected sector": "the protected sector" sounds like there is only one protected sector. > + "sector or the locked OPT space.\n"); You meant OTP (One Time Programmable), didn't you ? s/OPT/OTP/ > + > + nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0); > + return -EIO; > + } > + > + return fsr & FSR_READY; > } > > static int spi_nor_ready(struct spi_nor *nor) > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index d0c66a0..46b5608 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -61,6 +61,7 @@ > #define SPINOR_OP_RDSFDP 0x5a/* Read SFDP */ > #define SPINOR_OP_RDCR 0x35/* Read configuration register > */ > #define SPINOR_OP_RDFSR 0x70/* Read flag status register */ > +#define SPINOR_OP_CLFSR 0x50/* Clear flag status register */ > > /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */ > #define SPINOR_OP_READ_4B0x13/* Read data bytes (low frequency) */ > @@ -130,7 +131,10 @@ > #define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */ > > /* Flag Status Register bits */ > -#define FSR_READYBIT(7) > +#define FSR_READYBIT(7) /* Device status, 0 = Busy,1 = Ready */ You may insert a space ' ' between "Busy," and "1 = " Best regards, Cyrille > +#define FSR_E_ERRBIT(5) /* Erase operation status */ > +#define FSR_P_ERRBIT(4) /* Program operation status */ > +#define FSR_PT_ERR BIT(1) /* Protection error bit */ > > /* Configuration Register bits. */ > #define CR_QUAD_EN_SPAN BIT(1) /* Spansion Quad I/O */ >
RE: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
Ping SPI-NOR maintainers again > >Ping SPI-NOR maintainers > >>-Original Message- >>From: Bean Huo (beanhuo) >>Sent: Samstag, 11. November 2017 21:49 >>To: 'cyrille.pitc...@wedev4u.fr'; >>marek.va...@gmail.com >>Cc: 'dw...@infradead.org' ; >>computersforpe...@gmail.com; 'linux-...@lists.infradead.org' >m...@lists.infradead.org>; linux-kernel@vger.kernel.org >>Subject: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits >> >>For the Micron SPI NOR, when the erase/program operation fails, >>especially, for the failure results from intending to modify protected >>space, spi-nor upper layers still get the return which shows the operation >succeeds. >>this because spi_nor_fsr_ready() only uses bit.7 to device whether ready. >>For the most cases, even the error of erase/program occurs, SPI NOR >>device is still ready. The device ready and the error are two different cases. >>This patch is to fixup this issue and adding FSR (flag status register) >>error bits checkup. >>The FSR(flag status register) is a powerful tool to investigate the >>staus of device,checking information regarding what is actually doing >>the memory and detecting possible error conditions. >> >>Signed-off-by: beanhuo >>--- >> drivers/mtd/spi-nor/spi-nor.c | 19 +-- >> include/linux/mtd/spi-nor.h | 6 +- >> 2 files changed, 22 insertions(+), 3 deletions(-) >> >>diff --git a/drivers/mtd/spi-nor/spi-nor.c >>b/drivers/mtd/spi-nor/spi-nor.c index bc266f7..200e814 100644 >>--- a/drivers/mtd/spi-nor/spi-nor.c >>+++ b/drivers/mtd/spi-nor/spi-nor.c >>@@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor >*nor) >> int fsr = read_fsr(nor); >> if (fsr < 0) >> return fsr; >>- else >>- return fsr & FSR_READY; >>+ >>+ if (fsr & (FSR_E_ERR | FSR_P_ERR)) { >>+ if (fsr & FSR_E_ERR) >>+ dev_err(nor->dev, "Erase operation failed.\n"); >>+ else >>+ dev_err(nor->dev, "Program operation failed.\n"); >>+ >>+ if (fsr & FSR_PT_ERR) >>+ dev_err(nor->dev, >>+ "The operation has attempted to modify the >>protected" >>+ "sector or the locked OPT space.\n"); >>+ >>+ nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0); >>+ return -EIO; >>+ } >>+ >>+ return fsr & FSR_READY; >> } >> >> static int spi_nor_ready(struct spi_nor *nor) diff --git >>a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index >>d0c66a0..46b5608 100644 >>--- a/include/linux/mtd/spi-nor.h >>+++ b/include/linux/mtd/spi-nor.h >>@@ -61,6 +61,7 @@ >> #define SPINOR_OP_RDSFDP 0x5a/* Read SFDP */ >> #define SPINOR_OP_RDCR 0x35/* Read configuration register >>*/ >> #define SPINOR_OP_RDFSR 0x70/* Read flag status register */ >>+#define SPINOR_OP_CLFSR 0x50/* Clear flag status register */ >> >> /* 4-byte address opcodes - used on Spansion and some Macronix flashes. >*/ >> #define SPINOR_OP_READ_4B0x13/* Read data bytes (low >frequency) */ >>@@ -130,7 +131,10 @@ >> #define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */ >> >> /* Flag Status Register bits */ >>-#define FSR_READYBIT(7) >>+#define FSR_READYBIT(7) /* Device status, 0 = Busy,1 = Ready */ >>+#define FSR_E_ERRBIT(5) /* Erase operation status */ >>+#define FSR_P_ERRBIT(4) /* Program operation status */ >>+#define FSR_PT_ERR BIT(1) /* Protection error bit */ >> >> /* Configuration Register bits. */ >> #define CR_QUAD_EN_SPAN BIT(1) /* Spansion Quad I/O */ >>-- >>2.7.4 >>
RE: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
Ping SPI-NOR maintainers again > >Ping SPI-NOR maintainers > >>-Original Message- >>From: Bean Huo (beanhuo) >>Sent: Samstag, 11. November 2017 21:49 >>To: 'cyrille.pitc...@wedev4u.fr' ; >>marek.va...@gmail.com >>Cc: 'dw...@infradead.org' ; >>computersforpe...@gmail.com; 'linux-...@lists.infradead.org' >m...@lists.infradead.org>; linux-kernel@vger.kernel.org >>Subject: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits >> >>For the Micron SPI NOR, when the erase/program operation fails, >>especially, for the failure results from intending to modify protected >>space, spi-nor upper layers still get the return which shows the operation >succeeds. >>this because spi_nor_fsr_ready() only uses bit.7 to device whether ready. >>For the most cases, even the error of erase/program occurs, SPI NOR >>device is still ready. The device ready and the error are two different cases. >>This patch is to fixup this issue and adding FSR (flag status register) >>error bits checkup. >>The FSR(flag status register) is a powerful tool to investigate the >>staus of device,checking information regarding what is actually doing >>the memory and detecting possible error conditions. >> >>Signed-off-by: beanhuo >>--- >> drivers/mtd/spi-nor/spi-nor.c | 19 +-- >> include/linux/mtd/spi-nor.h | 6 +- >> 2 files changed, 22 insertions(+), 3 deletions(-) >> >>diff --git a/drivers/mtd/spi-nor/spi-nor.c >>b/drivers/mtd/spi-nor/spi-nor.c index bc266f7..200e814 100644 >>--- a/drivers/mtd/spi-nor/spi-nor.c >>+++ b/drivers/mtd/spi-nor/spi-nor.c >>@@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor >*nor) >> int fsr = read_fsr(nor); >> if (fsr < 0) >> return fsr; >>- else >>- return fsr & FSR_READY; >>+ >>+ if (fsr & (FSR_E_ERR | FSR_P_ERR)) { >>+ if (fsr & FSR_E_ERR) >>+ dev_err(nor->dev, "Erase operation failed.\n"); >>+ else >>+ dev_err(nor->dev, "Program operation failed.\n"); >>+ >>+ if (fsr & FSR_PT_ERR) >>+ dev_err(nor->dev, >>+ "The operation has attempted to modify the >>protected" >>+ "sector or the locked OPT space.\n"); >>+ >>+ nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0); >>+ return -EIO; >>+ } >>+ >>+ return fsr & FSR_READY; >> } >> >> static int spi_nor_ready(struct spi_nor *nor) diff --git >>a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index >>d0c66a0..46b5608 100644 >>--- a/include/linux/mtd/spi-nor.h >>+++ b/include/linux/mtd/spi-nor.h >>@@ -61,6 +61,7 @@ >> #define SPINOR_OP_RDSFDP 0x5a/* Read SFDP */ >> #define SPINOR_OP_RDCR 0x35/* Read configuration register >>*/ >> #define SPINOR_OP_RDFSR 0x70/* Read flag status register */ >>+#define SPINOR_OP_CLFSR 0x50/* Clear flag status register */ >> >> /* 4-byte address opcodes - used on Spansion and some Macronix flashes. >*/ >> #define SPINOR_OP_READ_4B0x13/* Read data bytes (low >frequency) */ >>@@ -130,7 +131,10 @@ >> #define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */ >> >> /* Flag Status Register bits */ >>-#define FSR_READYBIT(7) >>+#define FSR_READYBIT(7) /* Device status, 0 = Busy,1 = Ready */ >>+#define FSR_E_ERRBIT(5) /* Erase operation status */ >>+#define FSR_P_ERRBIT(4) /* Program operation status */ >>+#define FSR_PT_ERR BIT(1) /* Protection error bit */ >> >> /* Configuration Register bits. */ >> #define CR_QUAD_EN_SPAN BIT(1) /* Spansion Quad I/O */ >>-- >>2.7.4 >>
RE: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
Ping SPI-NOR maintainers >-Original Message- >From: Bean Huo (beanhuo) >Sent: Samstag, 11. November 2017 21:49 >To: 'cyrille.pitc...@wedev4u.fr'; >marek.va...@gmail.com >Cc: 'dw...@infradead.org' ; >computersforpe...@gmail.com; 'linux-...@lists.infradead.org' m...@lists.infradead.org>; linux-kernel@vger.kernel.org >Subject: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits > >For the Micron SPI NOR, when the erase/program operation fails, especially, >for the failure results from intending to modify protected space, spi-nor >upper layers still get the return which shows the operation succeeds. >this because spi_nor_fsr_ready() only uses bit.7 to device whether ready. >For the most cases, even the error of erase/program occurs, SPI NOR device is >still ready. The device ready and the error are two different cases. >This patch is to fixup this issue and adding FSR (flag status register) error >bits >checkup. >The FSR(flag status register) is a powerful tool to investigate the staus of >device,checking information regarding what is actually doing the memory and >detecting possible error conditions. > >Signed-off-by: beanhuo >--- > drivers/mtd/spi-nor/spi-nor.c | 19 +-- > include/linux/mtd/spi-nor.h | 6 +- > 2 files changed, 22 insertions(+), 3 deletions(-) > >diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >index bc266f7..200e814 100644 >--- a/drivers/mtd/spi-nor/spi-nor.c >+++ b/drivers/mtd/spi-nor/spi-nor.c >@@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor) > int fsr = read_fsr(nor); > if (fsr < 0) > return fsr; >- else >- return fsr & FSR_READY; >+ >+ if (fsr & (FSR_E_ERR | FSR_P_ERR)) { >+ if (fsr & FSR_E_ERR) >+ dev_err(nor->dev, "Erase operation failed.\n"); >+ else >+ dev_err(nor->dev, "Program operation failed.\n"); >+ >+ if (fsr & FSR_PT_ERR) >+ dev_err(nor->dev, >+ "The operation has attempted to modify the >protected" >+ "sector or the locked OPT space.\n"); >+ >+ nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0); >+ return -EIO; >+ } >+ >+ return fsr & FSR_READY; > } > > static int spi_nor_ready(struct spi_nor *nor) diff --git >a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index >d0c66a0..46b5608 100644 >--- a/include/linux/mtd/spi-nor.h >+++ b/include/linux/mtd/spi-nor.h >@@ -61,6 +61,7 @@ > #define SPINOR_OP_RDSFDP 0x5a/* Read SFDP */ > #define SPINOR_OP_RDCR0x35/* Read configuration register >*/ > #define SPINOR_OP_RDFSR 0x70/* Read flag status register */ >+#define SPINOR_OP_CLFSR 0x50/* Clear flag status register */ > > /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */ > #define SPINOR_OP_READ_4B 0x13/* Read data bytes (low frequency) */ >@@ -130,7 +131,10 @@ > #define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */ > > /* Flag Status Register bits */ >-#define FSR_READY BIT(7) >+#define FSR_READY BIT(7) /* Device status, 0 = Busy,1 = Ready */ >+#define FSR_E_ERR BIT(5) /* Erase operation status */ >+#define FSR_P_ERR BIT(4) /* Program operation status */ >+#define FSR_PT_ERRBIT(1) /* Protection error bit */ > > /* Configuration Register bits. */ > #define CR_QUAD_EN_SPAN BIT(1) /* Spansion Quad I/O */ >-- >2.7.4 >
RE: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits
Ping SPI-NOR maintainers >-Original Message- >From: Bean Huo (beanhuo) >Sent: Samstag, 11. November 2017 21:49 >To: 'cyrille.pitc...@wedev4u.fr' ; >marek.va...@gmail.com >Cc: 'dw...@infradead.org' ; >computersforpe...@gmail.com; 'linux-...@lists.infradead.org' m...@lists.infradead.org>; linux-kernel@vger.kernel.org >Subject: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits > >For the Micron SPI NOR, when the erase/program operation fails, especially, >for the failure results from intending to modify protected space, spi-nor >upper layers still get the return which shows the operation succeeds. >this because spi_nor_fsr_ready() only uses bit.7 to device whether ready. >For the most cases, even the error of erase/program occurs, SPI NOR device is >still ready. The device ready and the error are two different cases. >This patch is to fixup this issue and adding FSR (flag status register) error >bits >checkup. >The FSR(flag status register) is a powerful tool to investigate the staus of >device,checking information regarding what is actually doing the memory and >detecting possible error conditions. > >Signed-off-by: beanhuo >--- > drivers/mtd/spi-nor/spi-nor.c | 19 +-- > include/linux/mtd/spi-nor.h | 6 +- > 2 files changed, 22 insertions(+), 3 deletions(-) > >diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >index bc266f7..200e814 100644 >--- a/drivers/mtd/spi-nor/spi-nor.c >+++ b/drivers/mtd/spi-nor/spi-nor.c >@@ -330,8 +330,23 @@ static inline int spi_nor_fsr_ready(struct spi_nor *nor) > int fsr = read_fsr(nor); > if (fsr < 0) > return fsr; >- else >- return fsr & FSR_READY; >+ >+ if (fsr & (FSR_E_ERR | FSR_P_ERR)) { >+ if (fsr & FSR_E_ERR) >+ dev_err(nor->dev, "Erase operation failed.\n"); >+ else >+ dev_err(nor->dev, "Program operation failed.\n"); >+ >+ if (fsr & FSR_PT_ERR) >+ dev_err(nor->dev, >+ "The operation has attempted to modify the >protected" >+ "sector or the locked OPT space.\n"); >+ >+ nor->write_reg(nor, SPINOR_OP_CLFSR, NULL, 0); >+ return -EIO; >+ } >+ >+ return fsr & FSR_READY; > } > > static int spi_nor_ready(struct spi_nor *nor) diff --git >a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index >d0c66a0..46b5608 100644 >--- a/include/linux/mtd/spi-nor.h >+++ b/include/linux/mtd/spi-nor.h >@@ -61,6 +61,7 @@ > #define SPINOR_OP_RDSFDP 0x5a/* Read SFDP */ > #define SPINOR_OP_RDCR0x35/* Read configuration register >*/ > #define SPINOR_OP_RDFSR 0x70/* Read flag status register */ >+#define SPINOR_OP_CLFSR 0x50/* Clear flag status register */ > > /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */ > #define SPINOR_OP_READ_4B 0x13/* Read data bytes (low frequency) */ >@@ -130,7 +131,10 @@ > #define EVCR_QUAD_EN_MICRON BIT(7) /* Micron Quad I/O */ > > /* Flag Status Register bits */ >-#define FSR_READY BIT(7) >+#define FSR_READY BIT(7) /* Device status, 0 = Busy,1 = Ready */ >+#define FSR_E_ERR BIT(5) /* Erase operation status */ >+#define FSR_P_ERR BIT(4) /* Program operation status */ >+#define FSR_PT_ERRBIT(1) /* Protection error bit */ > > /* Configuration Register bits. */ > #define CR_QUAD_EN_SPAN BIT(1) /* Spansion Quad I/O */ >-- >2.7.4 >