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

[Ronak Desai]

On Tue, Apr 24, 2018 at 4:56 AM, Miquel Raynal
<miquel.raynal at 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]);

> 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);
>> >> +
>> >> +     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



-- 
Ronak A Desai
Sr. Software Engineer
Airborne Information Solutions / RC Linux Platform Software
MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
Ronak.Desai at rockwellcollins.com
https://www.rockwellcollins.com/