From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm0-x229.google.com ([2a00:1450:400c:c09::229]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1coBlm-0006EC-7v for linux-mtd@lists.infradead.org; Wed, 15 Mar 2017 16:27:32 +0000 Received: by mail-wm0-x229.google.com with SMTP id t189so27356825wmt.1 for ; Wed, 15 Mar 2017 09:27:05 -0700 (PDT) Subject: Re: [PATCH V6, 1/1] mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program To: Boris Brezillon , Marek Vasut References: <1488575813-12660-1-git-send-email-kdasu.kdev@gmail.com> <1488575813-12660-2-git-send-email-kdasu.kdev@gmail.com> <25484b94-341f-300d-474e-a272f249f0a2@gmail.com> <20170315150147.337ddd04@bbrezillon> Cc: Kamal Dasu , linux-mtd@lists.infradead.org, f.fainelli@gmail.com, bcm-kernel-feedback-list@broadcom.com, dwmw2@infradead.org, computersforpeace@gmail.com, richard@nod.at, cyrille.pitchen@atmel.com From: Florian Fainelli Message-ID: Date: Wed, 15 Mar 2017 09:26:57 -0700 MIME-Version: 1.0 In-Reply-To: <20170315150147.337ddd04@bbrezillon> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 03/15/2017 07:01 AM, Boris Brezillon wrote: > On Fri, 10 Mar 2017 14:22:39 +0100 > Marek Vasut wrote: > >> On 03/03/2017 10:16 PM, Kamal Dasu wrote: >>> On brcmnand controller v6.x and v7.x, the #WP pin is controlled through >>> the NAND_WP bit in CS_SELECT register. >>> >>> The driver currently assumes that toggling the #WP pin is >>> instantaneously enabling/disabling write-protection, but it actually >>> takes some time to propagate the new state to the internal NAND chip >>> logic. This behavior is sometime causing data corruptions when an >>> erase/program operation is executed before write-protection has really >>> been disabled. >>> >>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller") >>> Signed-off-by: Kamal Dasu >>> --- >>> drivers/mtd/nand/brcmnand/brcmnand.c | 61 ++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 58 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c >>> index 42ebd73..7419c5c 100644 >>> --- a/drivers/mtd/nand/brcmnand/brcmnand.c >>> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c >>> @@ -101,6 +101,9 @@ struct brcm_nand_dma_desc { >>> #define BRCMNAND_MIN_BLOCKSIZE (8 * 1024) >>> #define BRCMNAND_MIN_DEVSIZE (4ULL * 1024 * 1024) >>> >>> +#define NAND_CTRL_RDY (INTFC_CTLR_READY | INTFC_FLASH_READY) >>> +#define NAND_POLL_STATUS_TIMEOUT_MS 100 >>> + >>> /* Controller feature flags */ >>> enum { >>> BRCMNAND_HAS_1K_SECTORS = BIT(0), >>> @@ -765,6 +768,31 @@ enum { >>> CS_SELECT_AUTO_DEVICE_ID_CFG = BIT(30), >>> }; >>> >>> +static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl, >>> + u32 mask, u32 expected_val, >>> + unsigned long timeout_ms) >>> +{ >>> + unsigned long limit; >>> + u32 val; >>> + >>> + if (!timeout_ms) >>> + timeout_ms = NAND_POLL_STATUS_TIMEOUT_MS; >>> + >>> + limit = jiffies + msecs_to_jiffies(timeout_ms); >>> + do { >>> + val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS); >>> + if ((val & mask) == expected_val) >>> + return 0; >>> + >>> + cpu_relax(); >>> + } while (time_after(limit, jiffies)); >>> + >>> + dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n", >>> + expected_val, val & mask); >>> + >>> + return -ETIMEDOUT; >>> +} >>> + >>> static inline void brcmnand_set_wp(struct brcmnand_controller *ctrl, bool en) >>> { >>> u32 val = en ? CS_SELECT_NAND_WP : 0; >>> @@ -1024,12 +1052,39 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp) >>> >>> if ((ctrl->features & BRCMNAND_HAS_WP) && wp_on == 1) { >>> static int old_wp = -1; >> >> Unrelated to this patch, but this static variable should be moved to >> driver's private data instead. >> >>> + int ret; >>> >>> if (old_wp != wp) { >>> dev_dbg(ctrl->dev, "WP %s\n", wp ? "on" : "off"); >>> old_wp = wp; >>> } >>> + >>> + /* >>> + * make sure ctrl/flash ready before and after >>> + * changing state of #WP pin >>> + */ >> >> Shouldn't the brcmnand_set_wp() do this ? > > Hm, AFAIU, brcmnand_set_wp() is only controlling the WP pin from the > controller side, so maybe we should rename the function > brcmnand_ctrl_set_wp() to clarify that. While I don't object that this change should be made, we are already at version 6 here, and this is a bugfix, so what else needs to be done to get this included? -- Florian