From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752856AbdK3Ole (ORCPT ); Thu, 30 Nov 2017 09:41:34 -0500 Received: from mailout.micron.com ([137.201.242.129]:34911 "EHLO mailout.micron.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752216AbdK3Old (ORCPT ); Thu, 30 Nov 2017 09:41:33 -0500 From: "Bean Huo (beanhuo)" To: Cyrille Pitchen , "marek.vasut@gmail.com" CC: "linux-mtd@lists.infradead.org" , "computersforpeace@gmail.com" , "dwmw2@infradead.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits Thread-Topic: [PATCH V1] drivers:mtd:spi-nor:checkup FSR error bits Thread-Index: AdNp6SrmHOHec7RBT6Ce6ebaOjyvjg== Date: Thu, 30 Nov 2017 14:40:37 +0000 Message-ID: <0665b52580d94dee9d8fc07c3eebcc51@SIWEX5A.sing.micron.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.160.29.124] X-TM-AS-Product-Ver: SMEX-12.0.0.1464-8.100.1062-23500.006 X-TM-AS-Result: No--12.516400-0.000000-31 X-TM-AS-MatchedID: 150567-702020-700107-139010-851106-703788-106420-703712-1 88019-702942-703543-701177-708712-709584-700756-711432-105700-705861-706719 -704318-707027-702039-106230-700472-704980-700476-703454-702598-704496-7016 04-187067-701456-706431-702640-700648-862883-706290-700373-700970-701461-10 5250-700398-706592-863828-711109-701298-863596-700324-707788-704713-708804- 710375-148004-148046-148133-148980-20043-42000-42003-29961 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No x-mt-checkinternalsenderrule: True Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id vAUEfeJr017013 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_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_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, > >Cyrille > >> +#define FSR_E_ERR BIT(5) /* Erase operation status */ >> +#define FSR_P_ERR BIT(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 */ >>