Re: [PATCH] Added set feature command in FSL IFC nand controller driver for ONFI nand

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Ronak,

On Tue, 24 Apr 2018 08:51:48 -0500, Ronak Desai
<ronak.desai@rockwellcollins.com> wrote:

> On Tue, Apr 24, 2018 at 4:56 AM, Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> > Hi Ronak,
> >  
> >> >> +/**
> >> >> + * fsl_ifc_onfi_set_features- set features for ONFI nand
> >> >> + * @mtd: MTD device structure
> >> >> + * @chip: nand chip info structure
> >> >> + * @addr: feature address.
> >> >> + * @subfeature_param: the subfeature parameters, a four bytes array.
> >> >> + */
> >> >> +static int fsl_ifc_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
> >> >> +                     int addr, uint8_t *subfeature_param)
> >> >> +{
> >> >> +     int status;
> >> >> +     int i;
> >> >> +
> >> >> +#ifdef CONFIG_SYS_NAND_ONFI_DETECTION
> >> >> +     if (!chip->onfi_version ||
> >> >> +         !(le16_to_cpu(chip->onfi_params.opt_cmd)
> >> >> +           & ONFI_OPT_CMD_SET_GET_FEATURES))
> >> >> +             return -EINVAL;
> >> >> +#endif  
> >> >
> >> > No need to do that, the core will take care of it, see [1].  
> >>
> >> Agree, I will remove this.  
> >> >  
> >> >> +
> >> >> +     /* Want data from start of the buffer */
> >> >> +     set_addr(mtd, 0, 0, 0);  
> >> >
> >> > This is the only thing that differs from the core's implementation. I
> >> > see most of the time it is called from ->cmdfunc(), could you move it
> >> > there? If yes you could get rid of this entire hook and rely on the
> >> > core's function.
> >> >  
> >> NAND_CMD_SET_FEATURES command sends the sub-feature param from flash
> >> buffer on the provided address and I added this here because I wanted
> >> to make sure
> >> that the data is set from start of the buffer. I looked at the core's
> >> implementation
> >> and I see that the NAND_CMD_SET_FEATURES  command is called first and then
> >> the data is filled into the buffer which will not work in case of my
> >> implementation.
> >> So, I will have to keep this here and the function, please suggest. I
> >> will wait for your
> >> feedback before submitting V2.  Moreover, I would suggest to check
> >> sequence in core's
> >> implementation as the command should run after setting the data and not before.  
> >
> > Not sure what you mean exactly by "NAND_CMD_SET_FEATURES is called first
> > and the data is filled into the buffer", could you please point out the
> > faulty section in the core so I can have a look?
> >  
> I was pointing to the code in else part in "nand_set_features_op". FSL
> nand controller
> driver does not use "exec_op" so if I use nand_default_set_features
> from core as you suggested
> then code in the else part will be executed and as depicted below, the
> controller specific NAND_CMD_SET_FEATURES
> is called before writing the sub-feature parameters. Whereas in my
> implementation and I believe in general
> also you would want to call the controller specific "cmdfunc" only
> after writing the sub-feature parameters
> in buffer. Please correct me if I am missing anything here.
> 
> 2193     } else {
> 2194         chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, feature, -1);
> 2195         for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
> 2196             chip->write_byte(mtd, params[i]);

The logic is supposedly the same between ->exec_op/->cmdfunc()
regarding this area, the NAND protocol requires the command
NAND_CMD_SET_FEATURES to be sent on the NAND bus together with 1/ the
feature (or address) and then 2/ the parameters. I don't think this is
wrong. You can check this matches the flowchart available p242 of the
ONFI specification [1].

> 
> > Thanks,
> > Miquèl
> >  
> >> >> +
> >> >> +     for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
> >> >> +             chip->write_byte(mtd, subfeature_param[i]);
> >> >> +
> >> >> +     chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, 0);

And this implementation does the opposite :)

> >> >> +
> >> >> +     status = chip->waitfunc(mtd, chip);
> >> >> +     if (status & NAND_STATUS_FAIL)
> >> >> +             return -EIO;
> >> >> +     return 0;
> >> >> +}  
> >
> >
> > --
> > Miquel Raynal, Bootlin (formerly Free Electrons)
> > Embedded Linux and Kernel engineering
> > https://bootlin.com  
> 
> 
> 

[1] http://www.onfi.org/~/media/onfi/specs/onfi_4_0-gold.pdf?la=en

Thanks,
Miquèl

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