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