Hi Köry,

kory.maincent@bootlin.com wrote on Tue, 1 Feb 2022 18:45:14 +0100:

Perhaps the title prefix should be "mtd: rawnand:".

I am making a number of reworking proposals below, take what you like.

With the current code if the board have a NAND ONFI compliant without

has an ONFI compliant NAND

support to the get and set features, U-boot return an ENOTSUP error and we

returns an ENOTSUP error when trying to tune the timings which prevents the probe of the device.

can not talk to the memory. Indeed onfi_set_features return ENOTSUP error

_features() returns ENOTSUP if...

if set/get features is not supported. In the case of timings we should not return ENOTSUP because we can use the default timings, the modification of the NAND timings is optional.

Actually it does not mean that setting the timings is optional but rather that the NAND is already capable of listening at its highest supported rate, so we assume in this case that it is fine to skip the operation. But as far as I remember the ONFI specification does not clearly state that this is a fully valid behavior.

Fixed it by adding an intermediate nand_onfi_set_timings function which

Fix it ()

does nothing if set/get feature is not supported.

does not error out if...

The code looks good otherwise!

Signed-off-by: Kory Maincent kory.maincent@bootlin.com

drivers/mtd/nand/raw/nand_base.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index f7616985d9..44fb33776f 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -974,6 +974,22 @@ static int nand_reset_data_interface(struct nand_chip *chip, int chipnr) return ret; }

+static int nand_onfi_set_timings(struct mtd_info *mtd, struct nand_chip *chip) +{

• if (!chip->onfi_version ||

•    !(le16_to_cpu(chip->onfi_params.opt_cmd)
  

•      & ONFI_OPT_CMD_SET_GET_FEATURES))
  

• return 0;
  

• u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {

• chip->onfi_timing_mode_default,
  

• };
• return chip->onfi_set_features(mtd, chip,

• 		       ONFI_FEATURE_ADDR_TIMING_MODE,
  

• 		       tmode_param);
  

+}

/**

• nand_setup_data_interface - Setup the best data interface and timings
• @chip: The NAND chip

@@ -999,17 +1015,9 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) * Ensure the timing mode has been changed on the chip side * before changing timings on the controller side. */

• if (chip->onfi_version) {

• u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
  

• 	chip->onfi_timing_mode_default,
  

• };
  

• ret = chip->onfi_set_features(mtd, chip,
  

• 		ONFI_FEATURE_ADDR_TIMING_MODE,
  

• 		tmode_param);
  

• if (ret)
  

• 	goto err;
  

• }

• ret = nand_onfi_set_timings(mtd, chip);

• if (ret)

• goto err;
  
  

  ret = chip->setup_data_interface(mtd, chipnr, chip->data_interface);

err:

Thanks, Miquèl