From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel RAYNAL Subject: Re: [PATCH 5/5] mtd: nand: add ->exec_op() implementation Date: Fri, 1 Dec 2017 10:57:37 +0100 Message-ID: <20171201105737.4cea4ed9@xps13> References: <20171130170132.27522-1-miquel.raynal@free-electrons.com> <20171130170132.27522-6-miquel.raynal@free-electrons.com> <20171130215025.67f74437@bbrezillon> <20171130232538.140f17b1@xps13> <20171201105053.25af2267@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20171201105053.25af2267@bbrezillon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: Boris Brezillon Cc: Hanna Hawa , Stefan Agner , Nadav Haklai , Masahiro Yamada , Alexandre Belloni , Gregory Clement , devel@driverdev.osuosl.org, Maxim Levitsky , Kamal Dasu , Richard Weinberger , Marek Vasut , Chen-Yu Tsai , bcm-kernel-feedback-list@broadcom.com, Ezequiel Garcia , Sylvain Lemieux , Marc Gonzalez , Vladimir Zapolskiy , linux-mediatek@lists.infradead.org, Matthias Brugger , Han Xu , Ofer Heifetz , linux-arm-kernel@lists.infradead.orgGreg List-Id: linux-mediatek@lists.infradead.org SGkgQm9yaXMsCgpPbiBGcmksIDEgRGVjIDIwMTcgMTA6NTA6NTMgKzAxMDAKQm9yaXMgQnJlemls bG9uIDxib3Jpcy5icmV6aWxsb25AZnJlZS1lbGVjdHJvbnMuY29tPiB3cm90ZToKCj4gSGkgTWlx dWVsLAo+IAo+IE9uIFRodSwgMzAgTm92IDIwMTcgMjM6MjU6MzggKzAxMDAKPiBNaXF1ZWwgUkFZ TkFMIDxtaXF1ZWwucmF5bmFsQGZyZWUtZWxlY3Ryb25zLmNvbT4gd3JvdGU6Cj4gCj4gPiA+ID4g ZGlmZiAtLWdpdCBhL2RyaXZlcnMvbXRkL25hbmQvbmFuZF9iYXNlLmMKPiA+ID4gPiBiL2RyaXZl cnMvbXRkL25hbmQvbmFuZF9iYXNlLmMgaW5kZXggNTI5NjVhOGFlYjJjLi40NmJmMzFhZmY5MDkK PiA+ID4gPiAxMDA2NDQgLS0tIGEvZHJpdmVycy9tdGQvbmFuZC9uYW5kX2Jhc2UuYwo+ID4gPiA+ ICsrKyBiL2RyaXZlcnMvbXRkL25hbmQvbmFuZF9iYXNlLmMKPiA+ID4gPiBAQCAtNjg5LDYgKzY4 OSw1OSBAQCBzdGF0aWMgdm9pZCBuYW5kX3dhaXRfc3RhdHVzX3JlYWR5KHN0cnVjdAo+ID4gPiA+ IG10ZF9pbmZvICptdGQsIHVuc2lnbmVkIGxvbmcgdGltZW8pIH07Cj4gPiA+ID4gIAo+ID4gPiA+ ICAvKioKPiA+ID4gPiArICogbmFuZF9zb2Z0X3dhaXRyZHkgLSBSZWFkIHRoZSBzdGF0dXMgd2Fp dGluZyBmb3IgaXQgdG8gYmUKPiA+ID4gPiByZWFkeQo+ID4gPiA+ICsgKiBAY2hpcDogTkFORCBj aGlwIHN0cnVjdHVyZQo+ID4gPiA+ICsgKiBAdGltZW91dF9tczogVGltZW91dCBpbiBtcwo+ID4g PiA+ICsgKgo+ID4gPiA+ICsgKiBQb2xsIHRoZSBzdGF0dXMgdXNpbmcgLT5leGVjX29wKCkgdW50 aWwgaXQgaXMgcmVhZHkgdW5sZXNzCj4gPiA+ID4gaXQgdGFrZXMgdG9vCj4gPiA+ID4gKyAqIG11 Y2ggdGltZS4KPiA+ID4gPiArICoKPiA+ID4gPiArICogVGhpcyBoZWxwZXIgaXMgaW50ZW5kZWQg dG8gYmUgdXNlZCBieSBkcml2ZXJzIHdpdGhvdXQgUi9CCj4gPiA+ID4gcGluIGF2YWlsYWJsZSB0 bwo+ID4gPiA+ICsgKiBwb2xsIGZvciB0aGUgY2hpcCBzdGF0dXMgdW50aWwgcmVhZHkgYW5kIG1h eSBiZSBjYWxsZWQgYXQKPiA+ID4gPiBhbnkgdGltZSBpbiB0aGUKPiA+ID4gPiArICogbWlkZGxl IG9mIGFueSBzZXQgb2YgaW5zdHJ1Y3Rpb24uIFRoZSBSRUFEX1NUQVRVUyBqdXN0IG5lZWQKPiA+ ID4gPiB0byBhc2sgYSBzaW5nbGUKPiA+ID4gPiArICogdGltZSBmb3IgaXQgYW5kIHRoZW4gYW55 IHJlYWQgd2lsbCByZXR1cm4gdGhlIHN0YXR1cy4gT25jZQo+ID4gPiA+IHRoZSBSRUFEX1NUQVRV Uwo+ID4gPiA+ICsgKiBjeWNsZXMgYXJlIGRvbmUsIHRoZSBmdW5jdGlvbiB3aWxsIHNlbmQgYSBS RUFEMCBjb21tYW5kIHRvCj4gPiA+ID4gY2FuY2VsIHRoZQo+ID4gPiA+ICsgKiAiUkVBRF9TVEFU VVMgc3RhdGUiIGFuZCBsZXQgdGhlIG5vcm1hbCBmbG93IG9mIG9wZXJhdGlvbiB0bwo+ID4gPiA+ IGNvbnRpbnVlLgo+ID4gPiA+ICsgKgo+ID4gPiA+ICsgKiBUaGlzIGhlbHBlciAqY2Fubm90KiBz ZW5kIGEgV0FJVFJEWSBjb21tYW5kIG9yIC0+ZXhlY19vcCgpCj4gPiA+ID4gaW1wbGVtZW50YXRp b25zICAgICAgCj4gPiA+IAo+ID4gPiAJCQkJCSAgXiBpbnN0cnVjdGlvbgo+ID4gPiAgICAgCj4g PiA+ID4gKyAqIHVzaW5nIGl0IHdpbGwgZW50ZXIgYW4gaW5maW5pdGUgbG9vcC4gICAgICAKPiA+ ID4gCj4gPiA+IEhtLCBub3Qgc3VyZSB3aHkgdGhpcyB3b3VsZCBiZSB0aGUgY2FzZSwgYnV0IG9r YXkuIE1heWJlIHlvdQo+ID4gPiBzaG91bGQgbW92ZSB0aGlzIGNvbW1lbnQgb3V0c2lkZSB0aGUg a2VybmVsIGRvYyBoZWFkZXIsIHNpbmNlCj4gPiA+IHRoaXMgaXMgYW4gaW1wbGVtZW50YXRpb24g ZGV0YWlsLCBub3Qgc29tZXRoaW5nIHRoZSBjYWxsZXIvdXNlcgo+ID4gPiBzaG91bGQgYmUgYXdh cmUgb2YuICAgIAo+ID4gCj4gPiBSaWdodC4KPiA+ICAgCj4gPiA+IAo+ID4gPiBUaGVyZSdzIGFu b3RoZXIgaW1wb3J0YW50IGFzcGVjdCB0byBtZW50aW9uIGhlcmU6IHRoaXMgZnVuY3Rpb24KPiA+ ID4gY2FuIG9ubHkgYmUgY2FsbGVkIGZyb20gYW4gLT5leGVjX29wKCkgaW1wbGVtZW50YXRpb24g aWYgdGhpcwo+ID4gPiBpbXBsZW1lbnRhdGlvbiBpcyByZS1lbnRyYW50LiAgICAKPiA+IAo+ID4g SSBkbyBub3QgYWdyZWUgd2l0aCB0aGlzIHN0YXRlbWVudDogdGhpcyBmdW5jdGlvbiBjYW4gYmUg Y2FsbGVkCj4gPiBmcm9tIGFuIC0+ZXhlY19vcCgpIGltcGxlbWVudGF0aW9uIGV2ZW4gaWYgaXQg aXMgbm90IHJlZW50cmFudCBhcwo+ID4gbG9uZyBhcyBpdCBkb2VzIG5vdCBzZW5kIGEgV0FJVFJE WSBpbnN0cnVjdGlvbiBpdHNlbGYuIE5vPyAgCj4gCj4gSWYgdGhlIC0+ZXhlY19vcCgpIGltcGxl bWVudGF0aW9uIGlzIG5vdCByZS1lbnRyYW50LCBubywKPiBuYW5kX3NvZnRfd2FpdHJkeSgpIGNh bid0IGJlIGNhbGxlZCBmcm9tIC0+ZXhlY19vcCgpLCBiZWNhdXNlIHRoZW4KPiB5b3Ugd2lsbCBy ZS1lbnRlciAtPmV4ZWNfb3AoKSB0byBleGVjdXRlIHRoZSByZWFkX3N0YXR1c19vcCgpLCBhbmQK PiBCT09NIQo+IAo+ID4gCj4gPiBPciBtYXliZSB5b3Ugd2FudGVkIHRvIHBvaW50IHRoYXQgdGhl IGVudGlyZSAtPmV4ZWNfb3AoKQo+ID4gaW1wbGVtZW50YXRpb24gbXVzdCBiZSByZWVudHJhbnQg aW4gb3JkZXIgdG8gdXNlIHRoaXMgZnVuY3Rpb24gaW4KPiA+IGl0PyAgCj4gCj4gWWVzLCB3aGF0 IGRpZCB5b3UgdW5kZXJzdGFuZD8KCk9rLCBJIHRoaW5rIEkgbWlzdW5kZXJzdG9vZCB0aGUgImlm IHRoaXMgaW1wbGVtZW50YXRpb24gaXMgcmUtZW50cmFudCIuClRoZSBpbXBsZW1lbnRhdGlvbiB5 b3Ugd2VyZSByZWZlcnJpbmcgdG8gd2FzIC0+ZXhlY19vcCgpJ3MKaW1wbGVtZW50YXRpb24sIG5v dCBuYW5kX3NvZnRfd2FpdHJkeSgpJ3MuCgo+IAo+ID4gICAKPiA+ID4gICAgIAo+ID4gPiA+ICsg Kgo+ID4gPiA+ICsgKiBSZXR1cm4gMCBpZiB0aGUgTkFORCBjaGlwIGlzIHJlYWR5LCBhIG5lZ2F0 aXZlIGVycm9yCj4gPiA+ID4gb3RoZXJ3aXNlLgo+ID4gPiA+ICsgKi8KPiA+ID4gPiAraW50IG5h bmRfc29mdF93YWl0cmR5KHN0cnVjdCBuYW5kX2NoaXAgKmNoaXAsIHVuc2lnbmVkIGxvbmcKPiA+ ID4gPiB0aW1lb3V0X21zKSArewo+ID4gPiA+ICsJdTggc3RhdHVzID0gMDsKPiA+ID4gPiArCWlu dCByZXQ7Cj4gPiA+ID4gKwo+ID4gPiA+ICsJaWYgKCFjaGlwLT5leGVjX29wKQo+ID4gPiA+ICsJ CXJldHVybiAtRU5PVFNVUFA7Cj4gPiA+ID4gKwo+ID4gPiA+ICsJcmV0ID0gbmFuZF9zdGF0dXNf b3AoY2hpcCwgTlVMTCk7Cj4gPiA+ID4gKwlpZiAocmV0KQo+ID4gPiA+ICsJCXJldHVybiByZXQ7 Cj4gPiA+ID4gKwo+ID4gPiA+ICsJdGltZW91dF9tcyA9IGppZmZpZXMgKyBtc2Vjc190b19qaWZm aWVzKHRpbWVvdXRfbXMpOwo+ID4gPiA+ICsJZG8gewo+ID4gPiA+ICsJCXJldCA9IG5hbmRfcmVh ZF9kYXRhX29wKGNoaXAsICZzdGF0dXMsCj4gPiA+ID4gc2l6ZW9mKHN0YXR1cyksIHRydWUpOwo+ ID4gPiA+ICsJCWlmIChyZXQpCj4gPiA+ID4gKwkJCWJyZWFrOwo+ID4gPiA+ICsKPiA+ID4gPiAr CQlpZiAoc3RhdHVzICYgTkFORF9TVEFUVVNfUkVBRFkpCj4gPiA+ID4gKwkJCWJyZWFrOwo+ID4g PiA+ICsKPiA+ID4gPiArCQl1ZGVsYXkoMTAwKTsgICAgICAKPiA+ID4gCj4gPiA+IFNvdW5kcyBh IGJpdCBoaWdoLCBlc3BlY2lhbGx5IGZvciBhIHJlYWQgcGFnZSB3aGljaCB0YWtlcyBhcm91bmQK PiA+ID4gMjB1cy4gICAgCj4gPiAKPiA+IFdlbGwsIHRoaXMgdmFsdWUgaXMgYXJiaXRyYXJ5IGJ1 dCBncmVwaW5nIGZvciBOQU5EX09QX1dBSVRfUkRZCj4gPiB0ZWxscyB1cyB0aGUgZGlmZmVyZW50 IHRpbWVvdXRzIHdpdGggd2hpY2ggdGhpcyBmdW5jdGlvbiBpcyB1c3VhbGx5Cj4gPiBjYWxsZWQs IHRvIGdldCBhbiBpZGVhIG9mIHRoZSBwb3NzaWJsZSB3YWl0IHBlcmlvZHM6IHRSLCB0QkVSUywK PiA+IHRGRUFULCB0UFJPRywgdFJTVC4KPiA+IAo+ID4gV2hpbGUgYSB0Ul9tYXggaXMgMjAwdXMs IGEgdFJTVF9tYXggaXMgMjUwMDAwdXMuIFRoYXQgaXMgd2h5IEkKPiA+IGNob29zZSAxMDB1cyBh cyBwZXJpb2QsIHdoaWNoIEkgZm91bmQgc29tZWhvdyB3ZWxsIHR1bmVkIGZvciBldmVyeQo+ID4g dGltZW91dC4gIAo+IAo+IEEgdGltZW91dCBpcyBkaWZmZXJlbnQgZnJvbSBhIHR5cGljYWwgZXhl Y3V0aW9uIHRpbWUuIFRoZSB0aW1lb3V0IGlzCj4gaGVyZSBhcyBhIGJvdW5kYXJ5IHRvIGRldGVj dCB3aGVuIHRoZSBkZXZpY2UvY29udHJvbGxlciBpcyBub3QKPiByZXNwb25kaW5nLCBzbyBpZiB5 b3UgcG9sbCB0aGUgc3RhdHVzIGF0IHRoZSBwZXJpb2RpY2l0eSBvZiB0aGUKPiB0aW1lb3V0LCB5 b3UncmUgbGlrZWx5IHRvIHdhaXQgbXVjaCBtb3JlIHRoYW4geW91IHNob3VsZCBoYXZlLgo+IAo+ ID4gQnV0Cj4gPiBpZiB5b3UgdGhpbmsgbW9zdCBvZiB0aGUgdGltZSB0aGUgZGVsYXkgd2lsbCBi ZSBzbWFsbGVyLCBJIHdpbGwKPiA+IHVwZGF0ZSB0aGUgdmFsdWUgdG8gcmVwZWF0IHRoZSBvcGVy YXRpb24gZXZlcnkgMjB1cy4gIAo+IAo+IFdlbGwsIGVpdGhlciB5b3UgZG8gc29tZXRoaW5nIHNt YXJ0IHRoYXQgY2FsY3VsYXRlcyBhIHBvbGxpbmcgcGVyaW9kCj4gYmFzZWQgb24gdGhlIHRpbWVv dXQgdmFsICh0aW1lb3V0IC8gcmF0aW8pLCBvciB5b3UgcGljayBzb21ldGhpbmcKPiBjbG9zZSB0 byB0aGUgbG93ZXN0IHR5cGljYWwgdmFsdWUuIFNvLCBpbiBvdXIgY2FzZSwgc29tZXRoaW5nIGxp a2UKPiAxMHVzLCB3aGljaCBzaG91bGQgbm90IGJlIGZhciBmcm9tIHRoZSB0eXBpY2FsIHRSIHZh bHVlIG9uIG1vc3QgTkFORHMuCgpGb3IgdGhlIHNha2Ugb2Ygc2ltcGxpY2l0eSwgSSB3aWxsIHRo ZW4gdXNlIDEwdXMgcG9sbGluZyBwZXJpb2QgaGVyZS4KClRoYW5rcywKTWlxdcOobApfX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkZXZlbCBtYWlsaW5nIGxp c3QKZGV2ZWxAbGludXhkcml2ZXJwcm9qZWN0Lm9yZwpodHRwOi8vZHJpdmVyZGV2LmxpbnV4ZHJp dmVycHJvamVjdC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcml2ZXJkZXYtZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Fri, 1 Dec 2017 10:57:37 +0100 From: Miquel RAYNAL To: Boris Brezillon Cc: Hanna Hawa , linux-arm-kernel@lists.infradead.org, bcm-kernel-feedback-list@broadcom.com, linux-mediatek@lists.infradead.org, devel@driverdev.osuosl.org, Richard Weinberger , David Woodhouse , Brian Norris , Marek Vasut , Cyrille Pitchen , linux-mtd@lists.infradead.org, Wenyou Yang , Nicolas Ferre , Alexandre Belloni , Kamal Dasu , Masahiro Yamada , Han Xu , Vladimir Zapolskiy , Sylvain Lemieux , Matthias Brugger , Ezequiel Garcia , Maxim Levitsky , Maxime Ripard , Chen-Yu Tsai , Marc Gonzalez , Stefan Agner , Greg Kroah-Hartman , Thomas Petazzoni , Gregory Clement , Antoine Tenart , Nadav Haklai , Ofer Heifetz Subject: Re: [PATCH 5/5] mtd: nand: add ->exec_op() implementation Message-ID: <20171201105737.4cea4ed9@xps13> In-Reply-To: <20171201105053.25af2267@bbrezillon> References: <20171130170132.27522-1-miquel.raynal@free-electrons.com> <20171130170132.27522-6-miquel.raynal@free-electrons.com> <20171130215025.67f74437@bbrezillon> <20171130232538.140f17b1@xps13> <20171201105053.25af2267@bbrezillon> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Boris, On Fri, 1 Dec 2017 10:50:53 +0100 Boris Brezillon wrote: > Hi Miquel, >=20 > On Thu, 30 Nov 2017 23:25:38 +0100 > Miquel RAYNAL wrote: >=20 > > > > diff --git a/drivers/mtd/nand/nand_base.c > > > > b/drivers/mtd/nand/nand_base.c index 52965a8aeb2c..46bf31aff909 > > > > 100644 --- a/drivers/mtd/nand/nand_base.c > > > > +++ b/drivers/mtd/nand/nand_base.c > > > > @@ -689,6 +689,59 @@ static void nand_wait_status_ready(struct > > > > mtd_info *mtd, unsigned long timeo) }; > > > > =20 > > > > /** > > > > + * nand_soft_waitrdy - Read the status waiting for it to be > > > > ready > > > > + * @chip: NAND chip structure > > > > + * @timeout_ms: Timeout in ms > > > > + * > > > > + * Poll the status using ->exec_op() until it is ready unless > > > > it takes too > > > > + * much time. > > > > + * > > > > + * This helper is intended to be used by drivers without R/B > > > > pin available to > > > > + * poll for the chip status until ready and may be called at > > > > any time in the > > > > + * middle of any set of instruction. The READ_STATUS just need > > > > to ask a single > > > > + * time for it and then any read will return the status. Once > > > > the READ_STATUS > > > > + * cycles are done, the function will send a READ0 command to > > > > cancel the > > > > + * "READ_STATUS state" and let the normal flow of operation to > > > > continue. > > > > + * > > > > + * This helper *cannot* send a WAITRDY command or ->exec_op() > > > > implementations =20 > > >=20 > > > ^ instruction > > > =20 > > > > + * using it will enter an infinite loop. =20 > > >=20 > > > Hm, not sure why this would be the case, but okay. Maybe you > > > should move this comment outside the kernel doc header, since > > > this is an implementation detail, not something the caller/user > > > should be aware of. =20 > >=20 > > Right. > > =20 > > >=20 > > > There's another important aspect to mention here: this function > > > can only be called from an ->exec_op() implementation if this > > > implementation is re-entrant. =20 > >=20 > > I do not agree with this statement: this function can be called > > from an ->exec_op() implementation even if it is not reentrant as > > long as it does not send a WAITRDY instruction itself. No? =20 >=20 > If the ->exec_op() implementation is not re-entrant, no, > nand_soft_waitrdy() can't be called from ->exec_op(), because then > you will re-enter ->exec_op() to execute the read_status_op(), and > BOOM! >=20 > >=20 > > Or maybe you wanted to point that the entire ->exec_op() > > implementation must be reentrant in order to use this function in > > it? =20 >=20 > Yes, what did you understand? Ok, I think I misunderstood the "if this implementation is re-entrant". The implementation you were referring to was ->exec_op()'s implementation, not nand_soft_waitrdy()'s. >=20 > > =20 > > > =20 > > > > + * > > > > + * Return 0 if the NAND chip is ready, a negative error > > > > otherwise. > > > > + */ > > > > +int nand_soft_waitrdy(struct nand_chip *chip, unsigned long > > > > timeout_ms) +{ > > > > + u8 status =3D 0; > > > > + int ret; > > > > + > > > > + if (!chip->exec_op) > > > > + return -ENOTSUPP; > > > > + > > > > + ret =3D nand_status_op(chip, NULL); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + timeout_ms =3D jiffies + msecs_to_jiffies(timeout_ms); > > > > + do { > > > > + ret =3D nand_read_data_op(chip, &status, > > > > sizeof(status), true); > > > > + if (ret) > > > > + break; > > > > + > > > > + if (status & NAND_STATUS_READY) > > > > + break; > > > > + > > > > + udelay(100); =20 > > >=20 > > > Sounds a bit high, especially for a read page which takes around > > > 20us. =20 > >=20 > > Well, this value is arbitrary but greping for NAND_OP_WAIT_RDY > > tells us the different timeouts with which this function is usually > > called, to get an idea of the possible wait periods: tR, tBERS, > > tFEAT, tPROG, tRST. > >=20 > > While a tR_max is 200us, a tRST_max is 250000us. That is why I > > choose 100us as period, which I found somehow well tuned for every > > timeout. =20 >=20 > A timeout is different from a typical execution time. The timeout is > here as a boundary to detect when the device/controller is not > responding, so if you poll the status at the periodicity of the > timeout, you're likely to wait much more than you should have. >=20 > > But > > if you think most of the time the delay will be smaller, I will > > update the value to repeat the operation every 20us. =20 >=20 > Well, either you do something smart that calculates a polling period > based on the timeout val (timeout / ratio), or you pick something > close to the lowest typical value. So, in our case, something like > 10us, which should not be far from the typical tR value on most NANDs. For the sake of simplicity, I will then use 10us polling period here. Thanks, Miqu=C3=A8l From mboxrd@z Thu Jan 1 00:00:00 1970 From: miquel.raynal@free-electrons.com (Miquel RAYNAL) Date: Fri, 1 Dec 2017 10:57:37 +0100 Subject: [PATCH 5/5] mtd: nand: add ->exec_op() implementation In-Reply-To: <20171201105053.25af2267@bbrezillon> References: <20171130170132.27522-1-miquel.raynal@free-electrons.com> <20171130170132.27522-6-miquel.raynal@free-electrons.com> <20171130215025.67f74437@bbrezillon> <20171130232538.140f17b1@xps13> <20171201105053.25af2267@bbrezillon> Message-ID: <20171201105737.4cea4ed9@xps13> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Boris, On Fri, 1 Dec 2017 10:50:53 +0100 Boris Brezillon wrote: > Hi Miquel, > > On Thu, 30 Nov 2017 23:25:38 +0100 > Miquel RAYNAL wrote: > > > > > diff --git a/drivers/mtd/nand/nand_base.c > > > > b/drivers/mtd/nand/nand_base.c index 52965a8aeb2c..46bf31aff909 > > > > 100644 --- a/drivers/mtd/nand/nand_base.c > > > > +++ b/drivers/mtd/nand/nand_base.c > > > > @@ -689,6 +689,59 @@ static void nand_wait_status_ready(struct > > > > mtd_info *mtd, unsigned long timeo) }; > > > > > > > > /** > > > > + * nand_soft_waitrdy - Read the status waiting for it to be > > > > ready > > > > + * @chip: NAND chip structure > > > > + * @timeout_ms: Timeout in ms > > > > + * > > > > + * Poll the status using ->exec_op() until it is ready unless > > > > it takes too > > > > + * much time. > > > > + * > > > > + * This helper is intended to be used by drivers without R/B > > > > pin available to > > > > + * poll for the chip status until ready and may be called at > > > > any time in the > > > > + * middle of any set of instruction. The READ_STATUS just need > > > > to ask a single > > > > + * time for it and then any read will return the status. Once > > > > the READ_STATUS > > > > + * cycles are done, the function will send a READ0 command to > > > > cancel the > > > > + * "READ_STATUS state" and let the normal flow of operation to > > > > continue. > > > > + * > > > > + * This helper *cannot* send a WAITRDY command or ->exec_op() > > > > implementations > > > > > > ^ instruction > > > > > > > + * using it will enter an infinite loop. > > > > > > Hm, not sure why this would be the case, but okay. Maybe you > > > should move this comment outside the kernel doc header, since > > > this is an implementation detail, not something the caller/user > > > should be aware of. > > > > Right. > > > > > > > > There's another important aspect to mention here: this function > > > can only be called from an ->exec_op() implementation if this > > > implementation is re-entrant. > > > > I do not agree with this statement: this function can be called > > from an ->exec_op() implementation even if it is not reentrant as > > long as it does not send a WAITRDY instruction itself. No? > > If the ->exec_op() implementation is not re-entrant, no, > nand_soft_waitrdy() can't be called from ->exec_op(), because then > you will re-enter ->exec_op() to execute the read_status_op(), and > BOOM! > > > > > Or maybe you wanted to point that the entire ->exec_op() > > implementation must be reentrant in order to use this function in > > it? > > Yes, what did you understand? Ok, I think I misunderstood the "if this implementation is re-entrant". The implementation you were referring to was ->exec_op()'s implementation, not nand_soft_waitrdy()'s. > > > > > > > > > > + * > > > > + * Return 0 if the NAND chip is ready, a negative error > > > > otherwise. > > > > + */ > > > > +int nand_soft_waitrdy(struct nand_chip *chip, unsigned long > > > > timeout_ms) +{ > > > > + u8 status = 0; > > > > + int ret; > > > > + > > > > + if (!chip->exec_op) > > > > + return -ENOTSUPP; > > > > + > > > > + ret = nand_status_op(chip, NULL); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + timeout_ms = jiffies + msecs_to_jiffies(timeout_ms); > > > > + do { > > > > + ret = nand_read_data_op(chip, &status, > > > > sizeof(status), true); > > > > + if (ret) > > > > + break; > > > > + > > > > + if (status & NAND_STATUS_READY) > > > > + break; > > > > + > > > > + udelay(100); > > > > > > Sounds a bit high, especially for a read page which takes around > > > 20us. > > > > Well, this value is arbitrary but greping for NAND_OP_WAIT_RDY > > tells us the different timeouts with which this function is usually > > called, to get an idea of the possible wait periods: tR, tBERS, > > tFEAT, tPROG, tRST. > > > > While a tR_max is 200us, a tRST_max is 250000us. That is why I > > choose 100us as period, which I found somehow well tuned for every > > timeout. > > A timeout is different from a typical execution time. The timeout is > here as a boundary to detect when the device/controller is not > responding, so if you poll the status at the periodicity of the > timeout, you're likely to wait much more than you should have. > > > But > > if you think most of the time the delay will be smaller, I will > > update the value to repeat the operation every 20us. > > Well, either you do something smart that calculates a polling period > based on the timeout val (timeout / ratio), or you pick something > close to the lowest typical value. So, in our case, something like > 10us, which should not be far from the typical tR value on most NANDs. For the sake of simplicity, I will then use 10us polling period here. Thanks, Miqu?l