Re: [PATCH][v2] mtd: rawnand: fsl_ifc: Fix nand waitfunc return value

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Tue, 20 Mar 2018 10:16:04 +0000
Jagdish Gediya <jagdish.gediya@nxp.com> wrote:

> Hi Boris,
> 
> > -----Original Message-----
> > From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> > Sent: Tuesday, March 20, 2018 1:51 PM
> > To: Jagdish Gediya <jagdish.gediya@nxp.com>
> > Cc: linux-mtd@lists.infradead.org; boris.brezillon@free-electrons.com;
> > computersforpeace@gmail.com; oss@buserror.net; Leo Li
> > <leoyang.li@nxp.com>; stable@vger.kernel.org
> > Subject: Re: [PATCH][v2] mtd: rawnand: fsl_ifc: Fix nand waitfunc return value
> > 
> > On Tue, 20 Mar 2018 22:13:38 +0530
> > Jagdish Gediya <jagdish.gediya@nxp.com> wrote:
> >   
> > > As per the IFC hardware manual, Most significant 2 bytes in nand_fsr
> > > register are the outcome of NAND READ STATUS command.
> > >
> > > So status value need to be shifted and aligned as per the nand
> > > framework requirement.
> > >
> > > Fixes: 82771882d960 ("NAND Machine support for Integrated Flash
> > > Controller")
> > > Cc: stable@vger.kernel.org # v3.18+
> > > Signed-off-by: Jagdish Gediya <jagdish.gediya@nxp.com>
> > > Reviewed-by: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>
> > > ---
> > > Changes for v2: Incorporated comments from Boris Brezillon
> > >         - Added fixes tag
> > >
> > >  drivers/mtd/nand/fsl_ifc_nand.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c
> > > b/drivers/mtd/nand/fsl_ifc_nand.c index 9390cbd..0aa03ba 100644
> > > --- a/drivers/mtd/nand/fsl_ifc_nand.c
> > > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> > > @@ -643,12 +643,13 @@ static int fsl_ifc_wait(struct mtd_info *mtd, struct  
> > nand_chip *chip)  
> > >  	fsl_ifc_run_command(mtd);
> > >
> > >  	nand_fsr = ifc_in32(&ifc->ifc_nand.nand_fsr);
> > > -
> > > +	nand_fsr >>= 16;
> > > +	nand_fsr = (nand_fsr >> 8) | (nand_fsr << 8);  
> > 
> > Why are you swapping the upper and lower bytes? If the NAND status is stored
> > in the upper byte of the fsr reg (bits 24 to 31), then shift the value by 24
> > directly. Note that (nand_fsr << 8) is useless here since you're masking
> > nand_fsr with 0xff before returning it.
> >   
> nand_fsr register is,
>  31				   0
>  ---------------------------------------------
> | RS0 | RS1 | reserved | reserved |
>  ---------------------------------------------
> I intended to make it,
> 31		           0
>  -----------------------------
> | 00 | 00 | RS1 | RS0 |
>  -----------------------------
> RS0 gets the nand read status. I am not dropping RS1 value for possible future use.

Then store the nand status in a different var and keep nand_fsr
untouched:

	status = nand_fsr >> 24;

Note that I don't see the point of keeping RS1 until you really need it.

> > >  	/*
> > >  	 * The chip always seems to report that it is
> > >  	 * write-protected, even when it is not.
> > >  	 */
> > > -	return nand_fsr | NAND_STATUS_WP;
> > > +	return (nand_fsr & 0xff) | NAND_STATUS_WP;  
> It should be 0xffff instead of 0xff.

No it should not. The ->waitfunc() semantic is enforced by the
framework and it expects a NAND status *byte*.


-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com