On Tue, 20 Mar 2018 10:16:04 +0000 Jagdish Gediya <jagdish.gediya@xxxxxxx> wrote: > Hi Boris, > > > -----Original Message----- > > From: Boris Brezillon [mailto:boris.brezillon@xxxxxxxxxxx] > > Sent: Tuesday, March 20, 2018 1:51 PM > > To: Jagdish Gediya <jagdish.gediya@xxxxxxx> > > Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx; boris.brezillon@xxxxxxxxxxxxxxxxxx; > > computersforpeace@xxxxxxxxx; oss@xxxxxxxxxxxx; Leo Li > > <leoyang.li@xxxxxxx>; stable@xxxxxxxxxxxxxxx > > 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@xxxxxxx> 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@xxxxxxxxxxxxxxx # v3.18+ > > > Signed-off-by: Jagdish Gediya <jagdish.gediya@xxxxxxx> > > > Reviewed-by: Prabhakar Kushwaha <prabhakar.kushwaha@xxxxxxx> > > > --- > > > 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