Re: [PATCH v3] mtd/nand: don't use {read,write}_buf for 8-bit transfers

Ezequiel Garcia
2013-11-30T16:51:48Z

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