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

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