From mboxrd@z Thu Jan 1 00:00:00 1970 From: Albert ARIBAUD Date: Thu, 16 Jul 2015 14:43:32 +0200 Subject: [U-Boot] [PATCH 1/4] spl: nand: simple: replace readb() with chip specific read_buf() In-Reply-To: <55A7960E.6000809@mleia.com> References: <1437003228-14746-1-git-send-email-vz@mleia.com> <1437003228-14746-2-git-send-email-vz@mleia.com> <20150716100231.677f8d16@lilith> <55A7960E.6000809@mleia.com> Message-ID: <20150716144332.64c4edf2@lilith> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Vladimir, On Thu, 16 Jul 2015 14:31:26 +0300, Vladimir Zapolskiy wrote: > Hello Albert, > > On 16.07.2015 11:02, Albert ARIBAUD wrote: > > Hello Vladimir, > > > > On Thu, 16 Jul 2015 02:33:45 +0300, Vladimir Zapolskiy > > wrote: > >> Some NAND controllers define custom functions to read data out, > >> respect this in order to correctly support bad block handling in > >> simple SPL NAND framework. > >> > >> NAND controller specific read_buf() is used even to read 1 byte in > >> case of connected 8-bit NAND device, it turns out that read_byte() > >> may become outdated. > >> > >> Signed-off-by: Vladimir Zapolskiy > >> --- > >> drivers/mtd/nand/nand_spl_simple.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c > >> index 700ca32..e69f662 100644 > >> --- a/drivers/mtd/nand/nand_spl_simple.c > >> +++ b/drivers/mtd/nand/nand_spl_simple.c > >> @@ -115,6 +115,7 @@ static int nand_command(int block, int page, uint32_t offs, > >> static int nand_is_bad_block(int block) > >> { > >> struct nand_chip *this = mtd.priv; > >> + u_char bb_data[2]; > >> > >> nand_command(block, 0, CONFIG_SYS_NAND_BAD_BLOCK_POS, > >> NAND_CMD_READOOB); > >> @@ -123,10 +124,12 @@ static int nand_is_bad_block(int block) > >> * Read one byte (or two if it's a 16 bit chip). > >> */ > >> if (this->options & NAND_BUSWIDTH_16) { > >> - if (readw(this->IO_ADDR_R) != 0xffff) > >> + this->read_buf(&mtd, bb_data, 2); > >> + if (bb_data[0] != 0xff || bb_data[1] != 0xff) > >> return 1; > >> } else { > >> - if (readb(this->IO_ADDR_R) != 0xff) > >> + this->read_buf(&mtd, bb_data, 1); > >> + if (bb_data[0] != 0xff) > >> return 1; > >> } > >> > >> -- > >> 2.1.4 > >> > > > > The way you describe this patch, it looks like a bug that should have > > bitten way more boards than lpc32xx. Did you have a look at some other > > boards to see why this did not affect them? > > Yes, it is a bug IMHO. If it is, then it has hit no board which defines CONFIG_SPL_NAND_SIMPLE and we should understand why. > Grepping for CONFIG_SPL_NAND_SIMPLE I see that TI and Tegra boards may > be impacted (positively or negatively): > > * CONFIG_NAND_OMAP_GPMC --- own .read_buf(), default .read_byte() > * CONFIG_NAND_DAVINCI --- own .read_buf(), default .read_byte() > * CONFIG_TEGRA_NAND --- own .read_byte(), own .read_buf() They may be impacted by your change, but they are working now -- they are not obscure models. Let's dig a bit... > > Conversively, what is the actual failure scenario that led you to > > writing this patch? > > The failure scenario is quite simple, the ARM core gets stuck on first > attempt to access SLC NAND data register -- traced with JTAG. > > The same happens, if I remove custom .read_byte()/.read_buf() from SLC > NAND driver. The only difference between custom .read_byte() and shared > read_byte() is in readb()/readl() access to the data register, it is > essential to have only 32-bit wide access to SLC NAND registers. README describes CONFIG_SPL_NAND_SIMPLE as "Support for NAND boot using simple NAND drivers that expose the cmd_ctrl() interface". The cmd_ctrl interface actually contains the cmd_ctrl() function *and* the IO_ADDR_[RW] fields. IOW, a simple NAND driver provides byte or word access to the NAND's I/O lines on which command, address and data are passed. If the NAND is 8 bits, then there are 8 lines; if it is 16 bit, then there are 16 lines. I reckon that the OMAP/DAVINCI and TEGRA simple drivers just do that: set IO_ADDR_[RW] to the IP register through which direct access to the NAND's I/O lines can be performed, byte or word depending on the chip width. As such, there is no bug in the way nand_simple.c uses IO_ADDR_[RW]. OTOH, the SLC IP does not provide direct access to the NAND I/O lines through a general register; what it provides is a set of three specialized registers one for commands, one for addresses and one for data. Plus, even though only 8 bit NANDs are supported, the data register does not physically support 8-bit wide accesses (NXP: I am still struggling to understand what you were trying to achieve there). All this basically makes the SLC controller a 'not simple NAND IP', and its driver a 'not simple NAND driver' incompatible with nand_simple.c. Your solution to this incompatibility is to change nand_simple.c to support other types of drivers; but by doing that, you're changing the API between nand_simple.c and all simple drivers, possibly changing the established behavior. I personally don't think this is the right way; nand_simple.c should be left unchanged and the board should simply not use it since it does not have a simple NAND controller, and instead it should provide its own nand_spl_load_image(). But hey, I'm not then NAND custodian. Scott: your call. :) > -- > With best wishes, > Vladimir Amicalement, -- Albert.