From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 63D26C433F5 for ; Tue, 1 Feb 2022 17:59:38 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 420F7801EA; Tue, 1 Feb 2022 18:59:36 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=bootlin.com header.i=@bootlin.com header.b="PgkGJIMU"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 9EC738069C; Tue, 1 Feb 2022 18:59:34 +0100 (CET) Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id C19F7801B2 for ; Tue, 1 Feb 2022 18:59:29 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=miquel.raynal@bootlin.com Received: (Authenticated sender: miquel.raynal@bootlin.com) by mail.gandi.net (Postfix) with ESMTPSA id 6A3E3100008; Tue, 1 Feb 2022 17:59:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1643738369; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=jrNp1KN9bAdvGRTcsF7rGvlfo3P2O/NhJZfBPhSUG7E=; b=PgkGJIMUZKZl2WSV6jhGY0WFn+exqjp+7LllMO6xhLwAxQbP34TqWvVlFazU0A7ZY9Z9gv asQvau/TdnzVYzy+E4s+L/UoWlUMm3bj2LBpOddzVbL+T6NlqaAa2dRuSN42nPEciMqch2 gubdWSmw64+LEvo/SAK4R6rte17HTXzTws632wo5PwEvdAecb9YQG2vtsOGi5zh6KfkUIR RMLvbhoth6heCKymyw4c7CH48Ah/lBMWxBWdPGNtMkfgZNGR9riv4fa1FVbTZyF8az4lkM DcCY5NQbefbipKBp+4bSSHwyot6kfRGu7KR6bzjwQJ0cWW4tjMgzW4YLRjJYWw== Date: Tue, 1 Feb 2022 18:59:26 +0100 From: Miquel Raynal To: Kory Maincent Cc: u-boot@lists.denx.de, thomas.petazzoni@bootlin.com, Simon Glass , Wolfgang Denk , Patrice Chotard , Marek =?UTF-8?B?QmVow7pu?= Subject: Re: [PATCH] mtd: nand: Add support to dedicated function to set timings Message-ID: <20220201185926.583da64b@xps13> In-Reply-To: <20220201174514.409429-1-kory.maincent@bootlin.com> References: <20220201174514.409429-1-kory.maincent@bootlin.com> Organization: Bootlin X-Mailer: Claws Mail 3.17.7 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Hi K=C3=B6ry, 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 i= f...=20 > 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... =20 The code looks good otherwise! > Signed-off-by: Kory Maincent > --- > drivers/mtd/nand/raw/nand_base.c | 30 +++++++++++++++++++----------- > 1 file changed, 19 insertions(+), 11 deletions(-) >=20 > 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_chi= p *chip, int chipnr) > return ret; > } > =20 > +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] =3D { > + 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_ch= ip *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] =3D { > - chip->onfi_timing_mode_default, > - }; > - > - ret =3D chip->onfi_set_features(mtd, chip, > - ONFI_FEATURE_ADDR_TIMING_MODE, > - tmode_param); > - if (ret) > - goto err; > - } > + ret =3D nand_onfi_set_timings(mtd, chip); > + if (ret) > + goto err; > =20 > ret =3D chip->setup_data_interface(mtd, chipnr, chip->data_interface); > err: Thanks, Miqu=C3=A8l