diff mbox series

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

Message ID 1523992259-56588-1-git-send-email-ronak.desai@rockwellcollins.com
State Changes Requested
Delegated to: Boris Brezillon
Headers show
Series Added set feature command in FSL IFC nand controller driver for ONFI nand | expand

Commit Message

Ronak Desai April 17, 2018, 7:10 p.m. UTC
This patch adds set feature command (EFh) support in Freescale IFC nand
controller driver.

The SET FEATURES (EFh) command is used to modify the target's default
power-on behavior. This command uses one-byte feature address to
determine which sub-feature parameters will be modified.

Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
---
 drivers/mtd/nand/fsl_ifc_nand.c |   51 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

--
1.7.9.5

Comments

Miquel Raynal April 22, 2018, 5:07 p.m. UTC | #1
Hi Ronak,

On Tue, 17 Apr 2018 14:10:59 -0500, Ronak Desai
<ronak.desai@rockwellcollins.com> wrote:

> This patch adds set feature command (EFh) support in Freescale IFC nand
> controller driver.
> 
> The SET FEATURES (EFh) command is used to modify the target's default
> power-on behavior. This command uses one-byte feature address to
> determine which sub-feature parameters will be modified.
> 

Could you rebase on top of 4.17-rc1 at least, this file has moved and
the core changed about set/get_features() handling.

Also while you are at it, the prefix should be
'mtd: rawnand: fsl_ifc: '.

> Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
> ---
>  drivers/mtd/nand/fsl_ifc_nand.c |   51 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index af85c4b..20b97ef 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -613,6 +613,23 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
> 		fsl_ifc_run_command(mtd);
> 		return;
> 
> +	case NAND_CMD_SET_FEATURES: {
> +		ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
> +			(IFC_FIR_OP_UA << IFC_NAND_FIR0_OP1_SHIFT) |
> +			(IFC_FIR_OP_WBCD << IFC_NAND_FIR0_OP2_SHIFT),
> +			&ifc->ifc_nand.nand_fir0);
> +
> +		ifc_out32((NAND_CMD_SET_FEATURES << IFC_NAND_FCR0_CMD0_SHIFT),
> +			&ifc->ifc_nand.nand_fcr0);
> +
> +		ifc_out32(column, &ifc->ifc_nand.row3);
> +
> +		 /* Write only 4 bytes from flash buffer */
> +		ifc_out32(4, &ifc->ifc_nand.nand_fbcr);
> +		fsl_ifc_run_command(mtd);
> +		return;
> +	}
> +
> 	case NAND_CMD_RNDOUT: {
> 		__le16 Tccs = 0;
> 		chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
> @@ -905,6 +922,39 @@ static void fsl_ifc_sram_init(struct fsl_ifc_mtd *priv)
> 	ifc_out32(csor_ext, &ifc_global->csor_cs[cs].csor_ext);
>  }
> 
> +/**
> + * 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].

> +
> +	/* 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.

> +
> +	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;
> +}
>  static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
>  {
> 	struct fsl_ifc_ctrl *ctrl = priv->ctrl;
> @@ -932,6 +982,7 @@ static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
> 	chip->select_chip = fsl_ifc_select_chip;
> 	chip->cmdfunc = fsl_ifc_cmdfunc;
> 	chip->waitfunc = fsl_ifc_wait;
> +	chip->onfi_set_features = fsl_ifc_onfi_set_features;

Should be chip->set_features now. And fsl_ifc_onfi_set_features() could
become fsl_ifc_set_features().

If the driver does not support get_features, you should add a:

        chip->get_features = nand_get_set_features_notsupp;

> 
> 	chip->bbt_td = &bbt_main_descr;
> 	chip->bbt_md = &bbt_mirror_descr;
> --
> 1.7.9.5
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

[1]
https://elixir.bootlin.com/linux/v4.17-rc1/source/drivers/mtd/nand/raw/nand_base.c#L1204

Thanks,
Miquèl
Ronak Desai April 23, 2018, 7:55 p.m. UTC | #2
On Sun, Apr 22, 2018 at 12:07 PM, Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> Hi Ronak,
>
> On Tue, 17 Apr 2018 14:10:59 -0500, Ronak Desai
> <ronak.desai@rockwellcollins.com> wrote:
>
>> This patch adds set feature command (EFh) support in Freescale IFC nand
>> controller driver.
>>
>> The SET FEATURES (EFh) command is used to modify the target's default
>> power-on behavior. This command uses one-byte feature address to
>> determine which sub-feature parameters will be modified.
>>
>
> Could you rebase on top of 4.17-rc1 at least, this file has moved and
> the core changed about set/get_features() handling.
>
> Also while you are at it, the prefix should be
> 'mtd: rawnand: fsl_ifc: '.
>
Agree, I will do that.

>> Signed-off-by: Ronak Desai <ronak.desai@rockwellcollins.com>
>> ---
>>  drivers/mtd/nand/fsl_ifc_nand.c |   51 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
>> index af85c4b..20b97ef 100644
>> --- a/drivers/mtd/nand/fsl_ifc_nand.c
>> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
>> @@ -613,6 +613,23 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
>>               fsl_ifc_run_command(mtd);
>>               return;
>>
>> +     case NAND_CMD_SET_FEATURES: {
>> +             ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
>> +                     (IFC_FIR_OP_UA << IFC_NAND_FIR0_OP1_SHIFT) |
>> +                     (IFC_FIR_OP_WBCD << IFC_NAND_FIR0_OP2_SHIFT),
>> +                     &ifc->ifc_nand.nand_fir0);
>> +
>> +             ifc_out32((NAND_CMD_SET_FEATURES << IFC_NAND_FCR0_CMD0_SHIFT),
>> +                     &ifc->ifc_nand.nand_fcr0);
>> +
>> +             ifc_out32(column, &ifc->ifc_nand.row3);
>> +
>> +              /* Write only 4 bytes from flash buffer */
>> +             ifc_out32(4, &ifc->ifc_nand.nand_fbcr);
>> +             fsl_ifc_run_command(mtd);
>> +             return;
>> +     }
>> +
>>       case NAND_CMD_RNDOUT: {
>>               __le16 Tccs = 0;
>>               chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
>> @@ -905,6 +922,39 @@ static void fsl_ifc_sram_init(struct fsl_ifc_mtd *priv)
>>       ifc_out32(csor_ext, &ifc_global->csor_cs[cs].csor_ext);
>>  }
>>
>> +/**
>> + * 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.
>> +
>> +     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;
>> +}
>>  static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
>>  {
>>       struct fsl_ifc_ctrl *ctrl = priv->ctrl;
>> @@ -932,6 +982,7 @@ static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
>>       chip->select_chip = fsl_ifc_select_chip;
>>       chip->cmdfunc = fsl_ifc_cmdfunc;
>>       chip->waitfunc = fsl_ifc_wait;
>> +     chip->onfi_set_features = fsl_ifc_onfi_set_features;
>
> Should be chip->set_features now. And fsl_ifc_onfi_set_features() could
> become fsl_ifc_set_features().
>
> If the driver does not support get_features, you should add a:
>
>         chip->get_features = nand_get_set_features_notsupp;
>
Agree, I will update this.
>>
>>       chip->bbt_td = &bbt_main_descr;
>>       chip->bbt_md = &bbt_mirror_descr;
>> --
>> 1.7.9.5
>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
> [1]
> https://elixir.bootlin.com/linux/v4.17-rc1/source/drivers/mtd/nand/raw/nand_base.c#L1204
>
> Thanks,
> Miquèl
>
> --
> Miquel Raynal, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com
Miquel Raynal April 24, 2018, 9:56 a.m. UTC | #3
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?

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;
> >> +}
Ronak Desai April 24, 2018, 1:51 p.m. UTC | #4
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]);

> 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
Miquel Raynal April 24, 2018, 2 p.m. UTC | #5
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
Ronak Desai April 24, 2018, 5:10 p.m. UTC | #6
On Tue, Apr 24, 2018 at 9:00 AM, Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> 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].
>
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
Boris Brezillon April 24, 2018, 5:37 p.m. UTC | #7
Hi Ronak,

I'm jumping on this discussion to clarify a few things.

On Tue, 24 Apr 2018 12:10:07 -0500
Ronak Desai <ronak.desai@rockwellcollins.com> wrote:

> >> >> >> +
> >> >> >> +     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.

The ->cmdfunc() semantic has been abused for too long by too many
drivers, let's stop this insanity. ->cmdfunc() has been designed to
send CMD and ADDR cycles on the NAND bus, not to trigger the actual
data transfer. You might think this is a bad design (and I partially
agree that things were not perfect), but that's how it is. This being
said, we now have a clean way for drivers to get all information at
once (CMD, ADDR and DATA cycles) and it's called ->exec_op().

If you don't want to patch the driver to support ->exec_op(), I can
consider accepting a temporary solution where the whole SET_FEATURES
handling is done in fsl_ifc_onfi_set_features() (that is, moving the
code you have in the 'case NAND_CMD_SET_FEATURES' in the
fsl_ifc_onfi_set_features() function so that you don't call ->cmdfunc()
at all), but I'd really prefer to have the driver converted to
->exec_op(), and I think we (Miquel and I) can help with that.

BTW, you implement ->set_features() but ->get_features() is not
supported??!! This sounds like a bad idea to me...

Regards,

Boris
Ronak Desai April 25, 2018, 1:56 p.m. UTC | #8
Hey Borris, Miquel,

On Tue, Apr 24, 2018 at 12:37 PM, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> Hi Ronak,
>
> I'm jumping on this discussion to clarify a few things.
>
> On Tue, 24 Apr 2018 12:10:07 -0500
> Ronak Desai <ronak.desai@rockwellcollins.com> wrote:
>
>> >> >> >> +
>> >> >> >> +     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.
>
> The ->cmdfunc() semantic has been abused for too long by too many
> drivers, let's stop this insanity. ->cmdfunc() has been designed to
> send CMD and ADDR cycles on the NAND bus, not to trigger the actual
> data transfer. You might think this is a bad design (and I partially
> agree that things were not perfect), but that's how it is. This being
> said, we now have a clean way for drivers to get all information at
> once (CMD, ADDR and DATA cycles) and it's called ->exec_op().
>
> If you don't want to patch the driver to support ->exec_op(), I can
> consider accepting a temporary solution where the whole SET_FEATURES
> handling is done in fsl_ifc_onfi_set_features() (that is, moving the
> code you have in the 'case NAND_CMD_SET_FEATURES' in the
> fsl_ifc_onfi_set_features() function so that you don't call ->cmdfunc()
> at all), but I'd really prefer to have the driver converted to
> ->exec_op(), and I think we (Miquel and I) can help with that.
>
> BTW, you implement ->set_features() but ->get_features() is not
> supported??!! This sounds like a bad idea to me...
>
I understand your concern and thank you for the feedback, I am
planning to update the patch to accommodate temporary solution as
Borris suggested as converting the driver would take time but I am
also looking forward to it.
> Regards,
>
> Boris
diff mbox series

Patch

diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index af85c4b..20b97ef 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -613,6 +613,23 @@  static void fsl_ifc_cmdfunc(struct mtd_info *mtd, unsigned int command,
		fsl_ifc_run_command(mtd);
		return;

+	case NAND_CMD_SET_FEATURES: {
+		ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
+			(IFC_FIR_OP_UA << IFC_NAND_FIR0_OP1_SHIFT) |
+			(IFC_FIR_OP_WBCD << IFC_NAND_FIR0_OP2_SHIFT),
+			&ifc->ifc_nand.nand_fir0);
+
+		ifc_out32((NAND_CMD_SET_FEATURES << IFC_NAND_FCR0_CMD0_SHIFT),
+			&ifc->ifc_nand.nand_fcr0);
+
+		ifc_out32(column, &ifc->ifc_nand.row3);
+
+		 /* Write only 4 bytes from flash buffer */
+		ifc_out32(4, &ifc->ifc_nand.nand_fbcr);
+		fsl_ifc_run_command(mtd);
+		return;
+	}
+
	case NAND_CMD_RNDOUT: {
		__le16 Tccs = 0;
		chip->onfi_version ? (Tccs = chip->onfi_params.t_ccs)
@@ -905,6 +922,39 @@  static void fsl_ifc_sram_init(struct fsl_ifc_mtd *priv)
	ifc_out32(csor_ext, &ifc_global->csor_cs[cs].csor_ext);
 }

+/**
+ * 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
+
+	/* Want data from start of the buffer */
+	set_addr(mtd, 0, 0, 0);
+
+	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;
+}
 static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
 {
	struct fsl_ifc_ctrl *ctrl = priv->ctrl;
@@ -932,6 +982,7 @@  static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
	chip->select_chip = fsl_ifc_select_chip;
	chip->cmdfunc = fsl_ifc_cmdfunc;
	chip->waitfunc = fsl_ifc_wait;
+	chip->onfi_set_features = fsl_ifc_onfi_set_features;

	chip->bbt_td = &bbt_main_descr;
	chip->bbt_md = &bbt_mirror_descr;