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

[Ronak Desai]

On Tue, Apr 24, 2018 at 9:00 AM, Miquel Raynal
<miquel.raynal at bootlin.com> wrote:
> Hi Ronak,
>
> On Tue, 24 Apr 2018 08:51:48 -0500, Ronak Desai
> <ronak.desai at rockwellcollins.com> wrote:
>
>> 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]);
>
> 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].
>
I think it depends on the implementation of the NAND controller's
command function. In FSL driver, the command is executed when the
cmdfunc is called so flash controller data buffer needs to be filled
with the data that needs to be send in that command beforehand. Here
you are assuming that the cmndfunc is just sending the command and
then write_byte will transfer data on the bus. So, I think I will keep
my driver specific implementation of set_feature and will not use
core's function.
>>
>> > 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 :)
No, this is not opposite, I am just copying the data in the nand
controller data buffer and not sending on the bus and when the
NAND_CMD_SET_FEATURES is called, in that I tell nand controller to
transfer 4 bytes from data buffer after sending the set feature
command. So, on the bus the sequence will be same as depicted in the
ONFI specification. I have tested this on two different target
hardwares with processors having this nand controller.
>
>> >> >> +
>> >> >> +     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



-- 
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/