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

[Brian Norris <computersforpeace@gmail.com>]

On Sat, Nov 30, 2013 at 01:35:36PM -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;
>  	}

Hmm, that does seem like a hack. What command is giving you problems, by
the way? And is the NAND operational after this hack?

It looks like those two lines are there so that we can always specify
'column' in bytes, and nand_command() will do the byte/word translation
automatically. But we don't want this for certain commands, I think, so
maybe a "hack" is necessary...

> Now, IMHO, the solution of setting the defaults to x8 during the device
> detection phase, is far simpler.
> 
> BTW: this is not really related to Uwe's patch, right? It's just a complement
> to his work, as far as I can see.

Yeah, it's a complement. At first, I though Uwe's patch included this,
but I was wrong.

Brian