From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.bootlin.com ([62.4.15.54]:39022 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729021AbeKFR54 (ORCPT ); Tue, 6 Nov 2018 12:57:56 -0500 Date: Tue, 6 Nov 2018 09:33:34 +0100 From: Boris Brezillon To: IKEGAMI Tokunori Cc: "boris.brezillon@free-electrons.com" , Felix Fietkau , Hauke Mehrtens , "stable@vger.kernel.org" , Joakim Tjernlund , PACKHAM Chris , "linux-mtd@lists.infradead.org" , Koen Vandeputte , Fabio Bettoni Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good() Message-ID: <20181106093334.35864341@bbrezillon> In-Reply-To: References: <20181025163219.25788-1-ikegami@allied-telesis.co.jp> <20181025163219.25788-2-ikegami@allied-telesis.co.jp> <20181105111544.4e511ed9@bbrezillon> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: Hi IKEGAMI, On Tue, 6 Nov 2018 00:25:43 +0000 IKEGAMI Tokunori wrote: > > > Also the issue can be fixed by using chip_good() instead of chip_ready(). > > > The chip_ready() just checks the value from flash memory twice. > > > And the chip_good() checks the value with the expected value. > > > Probably the issue can be fixed as checked correctly by the chip_good(). > > > So change to use chip_good() instead of chip_ready(). > > > > Well, that's not really explaining why you think chip_good() should be > > used instead of chip_ready(). So I went on and looked at the > > chip_good(), chip_ready() and do_write_oneword() implementation, and > > also looked at users of do_write_oneword(). It seems this function is > > used to write data to the flash, and apparently the "one bit should > > toggle to reflect a busy state" does not apply when writing things to > > the memory array (it's probably working for other CFI commands, but I > > guess it takes more time to actually change the level of a NOR cell, > > hence the result of 2 identical reads does not mean that the write is > > done). > > > > Also, it seems that cmdset_0001 is not implementing chip_ready() the > > same way, and I wonder if cmdset_0002 implementation is correct to > > start with. Or maybe I don't get what chip_ready() is for. > > > > Anyway, this is the sort of clarification I'd like to have. > > I am thinking to update the commit message as below. > > mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword() > > As reported by the OpenWRT team, write requests sometimes fail on some > platforms. > Currently to check the state chip_ready() is used correctly as described by > the flash memory S29GL256P11TFI01 datasheet. I had a look at the S29GL256P datasheet here [1], and if I'm correct, it's using cmdset 0001. > Also chip_good() is used to check if the write is succeeded and it was > implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error > checking"). > But actually the write failure is caused on some platforms and also it can > be fixed by using chip_good() to check the state and retry instead. Do you know on which NOR chips this happens? Do you have access to the datasheet? > It is depended on the actual flash chip behavior so the root cause is > unknown. Yes, and that's what I'd like you to figure out, or at least have a good idea why this doesn't work on some chips but works on others. > > If any comment please let me know. > > > > > > > > > Signed-off-by: Tokunori Ikegami > > > Signed-off-by: Hauke Mehrtens > > > Signed-off-by: Koen Vandeputte > > > Signed-off-by: Fabio Bettoni > > > > Has the patch really gone through all those people? SoB is used when you > > apply a patch in your tree or when you're the original author. > > I have just checked the OpenWRT git log again and it looks that it was originally > implemented by Felix Fietkau by the patch below so I will update the Signed-off-by tag as so. > > > > > > > Co-Developed-by: Hauke Mehrtens > > > Co-Developed-by: Koen Vandeputte > > > Co-Developed-by: Fabio Bettoni > > > > Not sure we want to add new undocumented tags, but you can mention > > that all those people helped you find/debug the issue. They can also > > add their Reviewed-by/Tested-by if they like. My bad, I just noticed these are valid flags [2], so you can keep them, and according to the doc, you should also keep the SoB. Regards, Boris [1]http://www.cypress.com/file/219926/download [2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.20-rc1#n546