From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from top.free-electrons.com ([176.31.233.9] helo=mail.free-electrons.com) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Vmnlj-0003Rb-Kk for linux-mtd@lists.infradead.org; Sat, 30 Nov 2013 16:51:48 +0000 Date: Sat, 30 Nov 2013 13:51:23 -0300 From: Ezequiel Garcia To: Brian Norris Subject: Re: [PATCH v3] mtd/nand: don't use {read,write}_buf for 8-bit transfers Message-ID: <20131130165122.GC2402@localhost> References: <1365164021.28127.109.camel@i7.infradead.org> <1385500515-5376-1-git-send-email-u.kleine-koenig@pengutronix.de> <20131127073512.GB13929@norris.computersforpeace.net> <20131129122018.GB2815@localhost> <20131130060428.GA29397@norris.computersforpeace.net> <20131130163535.GB2402@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20131130163535.GB2402@localhost> Cc: Huang Shijie , linux-mtd@lists.infradead.org, Pekon Gupta , kernel@pengutronix.de, Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Sat, Nov 30, 2013 at 01:35:35PM -0300, Ezequiel Garcia wrote: > On Fri, Nov 29, 2013 at 10:04:28PM -0800, Brian Norris wrote: > > > > Unfortunately, I realized that Uwe's patch doesn't go far enough, I > > don't think. It looks like it needs something like the following diff > > (only compile-tested). > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index bd39f7b67906..1ab264457d94 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -2933,7 +2933,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > int *busw) > > { > > struct nand_onfi_params *p = &chip->onfi_params; > > - int i; > > + int i, j; > > int val; > > > > /* Try ONFI for unknown chip or LP */ > > @@ -2942,18 +2942,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > > chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') > > return 0; > > > > - /* > > - * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not > > - * with NAND_BUSWIDTH_16 > > - */ > > - if (chip->options & NAND_BUSWIDTH_16) { > > - pr_err("ONFI cannot be probed in 16-bit mode; aborting\n"); > > - return 0; > > - } > > - > > chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1); > > for (i = 0; i < 3; i++) { > > - chip->read_buf(mtd, (uint8_t *)p, sizeof(*p)); > > + for (j = 0; j < sizeof(*p); j++) > > + *(uint8_t *)p = chip->read_byte(mtd); > > if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) == > > le16_to_cpu(p->crc)) { > > break; > > > > What do you think? (And more importantly, how does this test out for > > you?) > > > > Your proposal would work (fixing a minor typo for incrementing 'p'), > except the nand_command() implementation messes with the buswith. > Therefore, after a long debugging session, I could make it work using > this hack: > > @@ -542,8 +545,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command, > /* Serially input address */ > if (column != -1) { > /* Adjust columns for 16 bit buswidth */ > - if (chip->options & NAND_BUSWIDTH_16) > - column >>= 1; > +// if (chip->options & NAND_BUSWIDTH_16) > +// column >>= 1; > chip->cmd_ctrl(mtd, column, ctrl); > ctrl &= ~NAND_CTRL_CHANGE; > } > > Now, IMHO, the solution of setting the defaults to x8 during the device > detection phase, is far simpler. > And after some more debugging, I now realise the above problem is also present at my previous proposal. So it seems we would need to actually temporary "deactivate" NAND_BUSWIDTH_16 from chip->options. I wonder how ugly that could be. Comments? In any case, it seems our proposals are equivalent: * we can change the defaults to x8 (is this at all needed?) * we can use read_byte But, again, we'll need to unset NAND_BUSWIDTH_16 from chip->option. Now, it would be a bit confusing to "trick" NAND_BUSWIDTH_16 from options without also setting the defaults to x8 for the detection phase. Comments? -- Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com