[PATCH v2 0/9] Misc timing changes

[PATCH v2 0/9] Misc timing changes

[Miquel Raynal]

Hello,

While working on the early ->exec_op() initializations, I figured the
logic might need to be updated a little bit in order to support
"problematic" IPs like Arasan which do not leave a lot of latitude to
the core. While doing these changes in the logic, I also decided to
cleanup this portion a bit, which ended with the writing of a few
actual fixes.

Cheers,
Miquèl

Changes in v2:
* Updated a bit the kernel doc as suggested by Boris.
* Updated/fixed typos in the commit logs following Boris and Sergey
  comments.
* Dropped the ONFI/JEDEC parameter page read simplification. Indeed
  they can cause troubles. I moved these two patches to another series
  which takes care about updating some of the core's operations for
  complex controllers.

Miquel Raynal (9):
  mtd: rawnand: timings: Add mode information to the timings structure
  mtd: rawnand: timings: Fix default tR_max and tCCS_min timings
  mtd: rawnand: onfi: Fix redundancy detection check
  mtd: rawnand: onfi: Use intermediate variables to improve readability
  mtd: rawnand: onfi: Define the number of parameter pages
  mtd: rawnand: onfi: Avoid doing a copy of the parameter page
  mtd: rawnand: onfi: Drop a useless parameter page read
  mtd: rawnand: jedec: Define the number of parameter pages
  mtd: rawnand: jedec: Use intermediate variables to improve readability

 drivers/mtd/nand/raw/nand_jedec.c   | 11 ++++---
 drivers/mtd/nand/raw/nand_onfi.c    | 48 ++++++++++++++---------------
 drivers/mtd/nand/raw/nand_timings.c | 11 +++++--
 include/linux/mtd/rawnand.h         | 10 ++++--
 4 files changed, 46 insertions(+), 34 deletions(-)

-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 1/9] mtd: rawnand: timings: Add mode information to the timings structure

[Miquel Raynal]

Convert the timings union into a structure containing the mode and the
actual values. The values are still a union in prevision of the
addition of the NVDDR modes.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/nand_timings.c |  6 ++++++
 include/linux/mtd/rawnand.h         | 10 +++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_timings.c b/drivers/mtd/nand/raw/nand_timings.c
index f64b06a71dfa..0061cbaf931d 100644
--- a/drivers/mtd/nand/raw/nand_timings.c
+++ b/drivers/mtd/nand/raw/nand_timings.c
@@ -16,6 +16,7 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	/* Mode 0 */
 	{
 		.type = NAND_SDR_IFACE,
+		.timings.mode = 0,
 		.timings.sdr = {
 			.tCCS_min = 500000,
 			.tR_max = 200000000,
@@ -58,6 +59,7 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	/* Mode 1 */
 	{
 		.type = NAND_SDR_IFACE,
+		.timings.mode = 1,
 		.timings.sdr = {
 			.tCCS_min = 500000,
 			.tR_max = 200000000,
@@ -100,6 +102,7 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	/* Mode 2 */
 	{
 		.type = NAND_SDR_IFACE,
+		.timings.mode = 2,
 		.timings.sdr = {
 			.tCCS_min = 500000,
 			.tR_max = 200000000,
@@ -142,6 +145,7 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	/* Mode 3 */
 	{
 		.type = NAND_SDR_IFACE,
+		.timings.mode = 3,
 		.timings.sdr = {
 			.tCCS_min = 500000,
 			.tR_max = 200000000,
@@ -184,6 +188,7 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	/* Mode 4 */
 	{
 		.type = NAND_SDR_IFACE,
+		.timings.mode = 4,
 		.timings.sdr = {
 			.tCCS_min = 500000,
 			.tR_max = 200000000,
@@ -226,6 +231,7 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
 	/* Mode 5 */
 	{
 		.type = NAND_SDR_IFACE,
+		.timings.mode = 5,
 		.timings.sdr = {
 			.tCCS_min = 500000,
 			.tR_max = 200000000,
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 1e76196f9829..21873168ba4d 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -491,13 +491,17 @@ enum nand_data_interface_type {
 /**
  * struct nand_data_interface - NAND interface timing
  * @type:	 type of the timing
- * @timings:	 The timing, type according to @type
+ * @timings:	 The timing information
+ * @timings.mode: Timing mode as defined in the specification
  * @timings.sdr: Use it when @type is %NAND_SDR_IFACE.
  */
 struct nand_data_interface {
 	enum nand_data_interface_type type;
-	union {
-		struct nand_sdr_timings sdr;
+	struct nand_timings {
+		unsigned int mode;
+		union {
+			struct nand_sdr_timings sdr;
+		};
 	} timings;
 };
 
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 2/9] mtd: rawnand: timings: Fix default tR_max and tCCS_min timings

[Miquel Raynal]

tR and TCCS are currently wrongly expressed in femto seconds, while we
expect these values to be expressed in picoseconds. Set right
hardcoded values.

Fixes: 6a943386ee36 mtd: rawnand: add default values for dynamic timings
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/nand_timings.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_timings.c b/drivers/mtd/nand/raw/nand_timings.c
index 0061cbaf931d..36d21be3dfe5 100644
--- a/drivers/mtd/nand/raw/nand_timings.c
+++ b/drivers/mtd/nand/raw/nand_timings.c
@@ -320,10 +320,9 @@ int onfi_fill_data_interface(struct nand_chip *chip,
 		/* microseconds -> picoseconds */
 		timings->tPROG_max = 1000000ULL * ONFI_DYN_TIMING_MAX;
 		timings->tBERS_max = 1000000ULL * ONFI_DYN_TIMING_MAX;
-		timings->tR_max = 1000000ULL * 200000000ULL;
 
-		/* nanoseconds -> picoseconds */
-		timings->tCCS_min = 1000UL * 500000;
+		timings->tR_max = 200000000;
+		timings->tCCS_min = 500000;
 	}
 
 	return 0;
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/9] mtd: rawnand: timings: Fix default tR_max and tCCS_min timings

[Sergei Shtylyov]

Hello!

On 24.04.2020 19:40, Miquel Raynal wrote:

> tR and TCCS are currently wrongly expressed in femto seconds, while we

    tCCS? Femtoseconds?

> expect these values to be expressed in picoseconds. Set right
> hardcoded values.
> 
> Fixes: 6a943386ee36 mtd: rawnand: add default values for dynamic timings
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
[...]

MBR, Sergei

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 2/9] mtd: rawnand: timings: Fix default tR_max and tCCS_min timings

[Miquel Raynal]

Hi Sergei,

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote on Sat, 25
Apr 2020 12:40:02 +0300:

> Hello!
> 
> On 24.04.2020 19:40, Miquel Raynal wrote:
> 
> > tR and TCCS are currently wrongly expressed in femto seconds, while we  
> 
>     tCCS? Femtoseconds?

Yup! Thanks for the correction.

> 
> > expect these values to be expressed in picoseconds. Set right
> > hardcoded values.
> > 
> > Fixes: 6a943386ee36 mtd: rawnand: add default values for dynamic timings
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>  
> [...]
> 
> MBR, Sergei

Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 3/9] mtd: rawnand: onfi: Fix redundancy detection check

[Miquel Raynal]

During ONFI detection, the CRC derived from the parameter page and the
CRC supposed to be at the end of the parameter page are compared. If
they do not match, the second then the third copies of the page are
tried.

The current implementation compares the newly derived CRC with the CRC
contained in the first page only. So if this particular CRC area has
been corrupted, then the detection will fail for a wrong reason.

Fix this issue by checking the derived CRC against the right one.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/nand_onfi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index 0b879bd0a68c..8fe8d7bdd203 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -173,7 +173,7 @@ int nand_onfi_detect(struct nand_chip *chip)
 		}
 
 		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
-				le16_to_cpu(p->crc)) {
+		    le16_to_cpu(p[i].crc)) {
 			if (i)
 				memcpy(p, &p[i], sizeof(*p));
 			break;
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 3/9] mtd: rawnand: onfi: Fix redundancy detection check

[Boris Brezillon]

On Fri, 24 Apr 2020 18:40:36 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> During ONFI detection, the CRC derived from the parameter page and the
> CRC supposed to be at the end of the parameter page are compared. If
> they do not match, the second then the third copies of the page are
> tried.
> 
> The current implementation compares the newly derived CRC with the CRC
> contained in the first page only. So if this particular CRC area has
> been corrupted, then the detection will fail for a wrong reason.
> 
> Fix this issue by checking the derived CRC against the right one.
> 

This one probably deserves Fixes and Cc-stable tags.

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/mtd/nand/raw/nand_onfi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> index 0b879bd0a68c..8fe8d7bdd203 100644
> --- a/drivers/mtd/nand/raw/nand_onfi.c
> +++ b/drivers/mtd/nand/raw/nand_onfi.c
> @@ -173,7 +173,7 @@ int nand_onfi_detect(struct nand_chip *chip)
>  		}
>  
>  		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> -				le16_to_cpu(p->crc)) {
> +		    le16_to_cpu(p[i].crc)) {
>  			if (i)
>  				memcpy(p, &p[i], sizeof(*p));
>  			break;


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 3/9] mtd: rawnand: onfi: Fix redundancy detection check

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sat, 25 Apr
2020 10:22:25 +0200:

> On Fri, 24 Apr 2020 18:40:36 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > During ONFI detection, the CRC derived from the parameter page and the
> > CRC supposed to be at the end of the parameter page are compared. If
> > they do not match, the second then the third copies of the page are
> > tried.
> > 
> > The current implementation compares the newly derived CRC with the CRC
> > contained in the first page only. So if this particular CRC area has
> > been corrupted, then the detection will fail for a wrong reason.
> > 
> > Fix this issue by checking the derived CRC against the right one.
> >   
> 

Yes, but given the fact that we moved this code out of nand_base.c
sending it to stable would not apply, I don't know what's best in this
case?

The faulty commit being
39138c1f4a31 mtd: rawnand: use bit-wise majority to recover the ONFI param page

> This one probably deserves Fixes and Cc-stable tags.
> 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/mtd/nand/raw/nand_onfi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> > index 0b879bd0a68c..8fe8d7bdd203 100644
> > --- a/drivers/mtd/nand/raw/nand_onfi.c
> > +++ b/drivers/mtd/nand/raw/nand_onfi.c
> > @@ -173,7 +173,7 @@ int nand_onfi_detect(struct nand_chip *chip)
> >  		}
> >  
> >  		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> > -				le16_to_cpu(p->crc)) {
> > +		    le16_to_cpu(p[i].crc)) {
> >  			if (i)
> >  				memcpy(p, &p[i], sizeof(*p));
> >  			break;  
> 

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 3/9] mtd: rawnand: onfi: Fix redundancy detection check

[Boris Brezillon]

On Tue, 28 Apr 2020 10:54:44 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Sat, 25 Apr
> 2020 10:22:25 +0200:
> 
> > On Fri, 24 Apr 2020 18:40:36 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > During ONFI detection, the CRC derived from the parameter page and the
> > > CRC supposed to be at the end of the parameter page are compared. If
> > > they do not match, the second then the third copies of the page are
> > > tried.
> > > 
> > > The current implementation compares the newly derived CRC with the CRC
> > > contained in the first page only. So if this particular CRC area has
> > > been corrupted, then the detection will fail for a wrong reason.
> > > 
> > > Fix this issue by checking the derived CRC against the right one.
> > >     
> >   
> 
> Yes, but given the fact that we moved this code out of nand_base.c
> sending it to stable would not apply, I don't know what's best in this
> case?

It would at least be backported to a few releases, and you can always
provide a replacement when Greg sends you the 'patch did not apply'
notice. So yes, I think it's worth adding a cc-stable tag here.

> 
> The faulty commit being
> 39138c1f4a31 mtd: rawnand: use bit-wise majority to recover the ONFI param page
> 
> > This one probably deserves Fixes and Cc-stable tags.
> >   
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  drivers/mtd/nand/raw/nand_onfi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> > > index 0b879bd0a68c..8fe8d7bdd203 100644
> > > --- a/drivers/mtd/nand/raw/nand_onfi.c
> > > +++ b/drivers/mtd/nand/raw/nand_onfi.c
> > > @@ -173,7 +173,7 @@ int nand_onfi_detect(struct nand_chip *chip)
> > >  		}
> > >  
> > >  		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> > > -				le16_to_cpu(p->crc)) {
> > > +		    le16_to_cpu(p[i].crc)) {
> > >  			if (i)
> > >  				memcpy(p, &p[i], sizeof(*p));
> > >  			break;    
> >   
> 
> Thanks,
> Miquèl


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 4/9] mtd: rawnand: onfi: Use intermediate variables to improve readability

[Miquel Raynal]

Before reworking a little bit the ONFI detection code, let's
clean the coding style of the if statements to improve readability.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/nand_onfi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index 8fe8d7bdd203..7d9a3130443a 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -146,6 +146,7 @@ int nand_onfi_detect(struct nand_chip *chip)
 	int onfi_version = 0;
 	char id[4];
 	int i, ret, val;
+	u16 crc;
 
 	memorg = nanddev_get_memorg(&chip->base);
 
@@ -172,8 +173,8 @@ int nand_onfi_detect(struct nand_chip *chip)
 			goto free_onfi_param_page;
 		}
 
-		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
-		    le16_to_cpu(p[i].crc)) {
+		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254);
+		if (crc == le16_to_cpu(p[i].crc)) {
 			if (i)
 				memcpy(p, &p[i], sizeof(*p));
 			break;
@@ -187,8 +188,8 @@ int nand_onfi_detect(struct nand_chip *chip)
 		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
 				       sizeof(*p));
 
-		if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
-				le16_to_cpu(p->crc)) {
+		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254);
+		if (crc != le16_to_cpu(p->crc)) {
 			pr_err("ONFI parameter recovery failed, aborting\n");
 			goto free_onfi_param_page;
 		}
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 5/9] mtd: rawnand: onfi: Define the number of parameter pages

[Miquel Raynal]

Use a macro to define the number of parameter page instead of
hardcoding it everywhere.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_onfi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index 7d9a3130443a..9fe39adbde4c 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -16,6 +16,8 @@
 
 #include "internals.h"
 
+#define ONFI_PARAM_PAGES 3
+
 u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
 {
 	int i;
@@ -156,7 +158,7 @@ int nand_onfi_detect(struct nand_chip *chip)
 		return 0;
 
 	/* ONFI chip: allocate a buffer to hold its parameter page */
-	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
+	p = kzalloc((sizeof(*p) * ONFI_PARAM_PAGES), GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 
@@ -166,7 +168,7 @@ int nand_onfi_detect(struct nand_chip *chip)
 		goto free_onfi_param_page;
 	}
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < ONFI_PARAM_PAGES; i++) {
 		ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
 		if (ret) {
 			ret = 0;
@@ -181,8 +183,8 @@ int nand_onfi_detect(struct nand_chip *chip)
 		}
 	}
 
-	if (i == 3) {
-		const void *srcbufs[3] = {p, p + 1, p + 2};
+	if (i == ONFI_PARAM_PAGES) {
+		const void *srcbufs[ONFI_PARAM_PAGES] = {p, p + 1, p + 2};
 
 		pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
 		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 5/9] mtd: rawnand: onfi: Define the number of parameter pages

[Boris Brezillon]

On Fri, 24 Apr 2020 18:40:38 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Use a macro to define the number of parameter page instead of
> hardcoding it everywhere.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_onfi.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> index 7d9a3130443a..9fe39adbde4c 100644
> --- a/drivers/mtd/nand/raw/nand_onfi.c
> +++ b/drivers/mtd/nand/raw/nand_onfi.c
> @@ -16,6 +16,8 @@
>  
>  #include "internals.h"
>  
> +#define ONFI_PARAM_PAGES 3
> +
>  u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
>  {
>  	int i;
> @@ -156,7 +158,7 @@ int nand_onfi_detect(struct nand_chip *chip)
>  		return 0;
>  
>  	/* ONFI chip: allocate a buffer to hold its parameter page */
> -	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> +	p = kzalloc((sizeof(*p) * ONFI_PARAM_PAGES), GFP_KERNEL);
>  	if (!p)
>  		return -ENOMEM;
>  
> @@ -166,7 +168,7 @@ int nand_onfi_detect(struct nand_chip *chip)
>  		goto free_onfi_param_page;
>  	}
>  
> -	for (i = 0; i < 3; i++) {
> +	for (i = 0; i < ONFI_PARAM_PAGES; i++) {
>  		ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
>  		if (ret) {
>  			ret = 0;
> @@ -181,8 +183,8 @@ int nand_onfi_detect(struct nand_chip *chip)
>  		}
>  	}
>  
> -	if (i == 3) {
> -		const void *srcbufs[3] = {p, p + 1, p + 2};
> +	if (i == ONFI_PARAM_PAGES) {
> +		const void *srcbufs[ONFI_PARAM_PAGES] = {p, p + 1, p + 2};
>  

Maybe initialize the srcbufs array using a for loop so you can easily
change ONFI_PARAM_PAGES without having to touch the code. Looks good
otherwise, so

Reviewed-by: Boris Breillon <boris.brezillon@collabora.com>

>  		pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
>  		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 5/9] mtd: rawnand: onfi: Define the number of parameter pages

[Boris Brezillon]

On Sat, 25 Apr 2020 10:25:19 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Fri, 24 Apr 2020 18:40:38 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Use a macro to define the number of parameter page instead of
> > hardcoding it everywhere.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_onfi.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> > index 7d9a3130443a..9fe39adbde4c 100644
> > --- a/drivers/mtd/nand/raw/nand_onfi.c
> > +++ b/drivers/mtd/nand/raw/nand_onfi.c
> > @@ -16,6 +16,8 @@
> >  
> >  #include "internals.h"
> >  
> > +#define ONFI_PARAM_PAGES 3
> > +
> >  u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
> >  {
> >  	int i;
> > @@ -156,7 +158,7 @@ int nand_onfi_detect(struct nand_chip *chip)
> >  		return 0;
> >  
> >  	/* ONFI chip: allocate a buffer to hold its parameter page */
> > -	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > +	p = kzalloc((sizeof(*p) * ONFI_PARAM_PAGES), GFP_KERNEL);
> >  	if (!p)
> >  		return -ENOMEM;
> >  
> > @@ -166,7 +168,7 @@ int nand_onfi_detect(struct nand_chip *chip)
> >  		goto free_onfi_param_page;
> >  	}
> >  
> > -	for (i = 0; i < 3; i++) {
> > +	for (i = 0; i < ONFI_PARAM_PAGES; i++) {
> >  		ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> >  		if (ret) {
> >  			ret = 0;
> > @@ -181,8 +183,8 @@ int nand_onfi_detect(struct nand_chip *chip)
> >  		}
> >  	}
> >  
> > -	if (i == 3) {
> > -		const void *srcbufs[3] = {p, p + 1, p + 2};
> > +	if (i == ONFI_PARAM_PAGES) {
> > +		const void *srcbufs[ONFI_PARAM_PAGES] = {p, p + 1, p + 2};
> >    
> 
> Maybe initialize the srcbufs array using a for loop so you can easily
> change ONFI_PARAM_PAGES without having to touch the code. Looks good
> otherwise, so
> 
> Reviewed-by: Boris Breillon <boris.brezillon@collabora.com>

Oops, there's a typo here ^ s/Breillon/Brezillon/

> 
> >  		pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
> >  		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,  
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 5/9] mtd: rawnand: onfi: Define the number of parameter pages

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sat, 25 Apr
2020 10:25:19 +0200:

> On Fri, 24 Apr 2020 18:40:38 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Use a macro to define the number of parameter page instead of
> > hardcoding it everywhere.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_onfi.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> > index 7d9a3130443a..9fe39adbde4c 100644
> > --- a/drivers/mtd/nand/raw/nand_onfi.c
> > +++ b/drivers/mtd/nand/raw/nand_onfi.c
> > @@ -16,6 +16,8 @@
> >  
> >  #include "internals.h"
> >  
> > +#define ONFI_PARAM_PAGES 3
> > +
> >  u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
> >  {
> >  	int i;
> > @@ -156,7 +158,7 @@ int nand_onfi_detect(struct nand_chip *chip)
> >  		return 0;
> >  
> >  	/* ONFI chip: allocate a buffer to hold its parameter page */
> > -	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > +	p = kzalloc((sizeof(*p) * ONFI_PARAM_PAGES), GFP_KERNEL);
> >  	if (!p)
> >  		return -ENOMEM;
> >  
> > @@ -166,7 +168,7 @@ int nand_onfi_detect(struct nand_chip *chip)
> >  		goto free_onfi_param_page;
> >  	}
> >  
> > -	for (i = 0; i < 3; i++) {
> > +	for (i = 0; i < ONFI_PARAM_PAGES; i++) {
> >  		ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> >  		if (ret) {
> >  			ret = 0;
> > @@ -181,8 +183,8 @@ int nand_onfi_detect(struct nand_chip *chip)
> >  		}
> >  	}
> >  
> > -	if (i == 3) {
> > -		const void *srcbufs[3] = {p, p + 1, p + 2};
> > +	if (i == ONFI_PARAM_PAGES) {
> > +		const void *srcbufs[ONFI_PARAM_PAGES] = {p, p + 1, p + 2};
> >    
> 
> Maybe initialize the srcbufs array using a for loop so you can easily
> change ONFI_PARAM_PAGES without having to touch the code. Looks good
> otherwise, so
> 
> Reviewed-by: Boris Breillon <boris.brezillon@collabora.com>
> 

Agreed, here is the snippet of code that I changed:

@@ -181,11 +183,15 @@ int nand_onfi_detect(struct nand_chip *chip)
                }
        }
 
-       if (i == 3) {
-               const void *srcbufs[3] = {p, p + 1, p + 2};
+       if (i == ONFI_PARAM_PAGES) {
+               const void *srcbufs[ONFI_PARAM_PAGES];
+               int j;
+
+               for (j = 0; j < ONFI_PARAM_PAGES; j++)
+                       srcbufs[j] = p + j;
 
                pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
-               nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
+               nand_bit_wise_majority(srcbufs, ONFI_PARAM_PAGES, p,
                                       sizeof(*p));
 
                crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254);



Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 5/9] mtd: rawnand: onfi: Define the number of parameter pages

[Boris Brezillon]

On Tue, 28 Apr 2020 11:36:28 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Sat, 25 Apr
> 2020 10:25:19 +0200:
> 
> > On Fri, 24 Apr 2020 18:40:38 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Use a macro to define the number of parameter page instead of
> > > hardcoding it everywhere.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/mtd/nand/raw/nand_onfi.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> > > index 7d9a3130443a..9fe39adbde4c 100644
> > > --- a/drivers/mtd/nand/raw/nand_onfi.c
> > > +++ b/drivers/mtd/nand/raw/nand_onfi.c
> > > @@ -16,6 +16,8 @@
> > >  
> > >  #include "internals.h"
> > >  
> > > +#define ONFI_PARAM_PAGES 3
> > > +
> > >  u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
> > >  {
> > >  	int i;
> > > @@ -156,7 +158,7 @@ int nand_onfi_detect(struct nand_chip *chip)
> > >  		return 0;
> > >  
> > >  	/* ONFI chip: allocate a buffer to hold its parameter page */
> > > -	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > > +	p = kzalloc((sizeof(*p) * ONFI_PARAM_PAGES), GFP_KERNEL);
> > >  	if (!p)
> > >  		return -ENOMEM;
> > >  
> > > @@ -166,7 +168,7 @@ int nand_onfi_detect(struct nand_chip *chip)
> > >  		goto free_onfi_param_page;
> > >  	}
> > >  
> > > -	for (i = 0; i < 3; i++) {
> > > +	for (i = 0; i < ONFI_PARAM_PAGES; i++) {
> > >  		ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> > >  		if (ret) {
> > >  			ret = 0;
> > > @@ -181,8 +183,8 @@ int nand_onfi_detect(struct nand_chip *chip)
> > >  		}
> > >  	}
> > >  
> > > -	if (i == 3) {
> > > -		const void *srcbufs[3] = {p, p + 1, p + 2};
> > > +	if (i == ONFI_PARAM_PAGES) {
> > > +		const void *srcbufs[ONFI_PARAM_PAGES] = {p, p + 1, p + 2};
> > >      
> > 
> > Maybe initialize the srcbufs array using a for loop so you can easily
> > change ONFI_PARAM_PAGES without having to touch the code. Looks good
> > otherwise, so
> > 
> > Reviewed-by: Boris Breillon <boris.brezillon@collabora.com>
> >   
> 
> Agreed, here is the snippet of code that I changed:
> 
> @@ -181,11 +183,15 @@ int nand_onfi_detect(struct nand_chip *chip)
>                 }
>         }
>  
> -       if (i == 3) {
> -               const void *srcbufs[3] = {p, p + 1, p + 2};
> +       if (i == ONFI_PARAM_PAGES) {
> +               const void *srcbufs[ONFI_PARAM_PAGES];
> +               int j;

		  ^unsigned int

> +
> +               for (j = 0; j < ONFI_PARAM_PAGES; j++)
> +                       srcbufs[j] = p + j;
>  
>                 pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
> -               nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> +               nand_bit_wise_majority(srcbufs, ONFI_PARAM_PAGES, p,
>                                        sizeof(*p));
>  
>                 crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254);
> 
> 
> 
> Thanks,
> Miquèl


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 5/9] mtd: rawnand: onfi: Define the number of parameter pages

[Miquel Raynal]

Boris Brezillon <boris.brezillon@collabora.com> wrote on Tue, 28 Apr
2020 11:38:15 +0200:

> On Tue, 28 Apr 2020 11:36:28 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <boris.brezillon@collabora.com> wrote on Sat, 25 Apr
> > 2020 10:25:19 +0200:
> >   
> > > On Fri, 24 Apr 2020 18:40:38 +0200
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > Use a macro to define the number of parameter page instead of
> > > > hardcoding it everywhere.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/mtd/nand/raw/nand_onfi.c | 10 ++++++----
> > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> > > > index 7d9a3130443a..9fe39adbde4c 100644
> > > > --- a/drivers/mtd/nand/raw/nand_onfi.c
> > > > +++ b/drivers/mtd/nand/raw/nand_onfi.c
> > > > @@ -16,6 +16,8 @@
> > > >  
> > > >  #include "internals.h"
> > > >  
> > > > +#define ONFI_PARAM_PAGES 3
> > > > +
> > > >  u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
> > > >  {
> > > >  	int i;
> > > > @@ -156,7 +158,7 @@ int nand_onfi_detect(struct nand_chip *chip)
> > > >  		return 0;
> > > >  
> > > >  	/* ONFI chip: allocate a buffer to hold its parameter page */
> > > > -	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > > > +	p = kzalloc((sizeof(*p) * ONFI_PARAM_PAGES), GFP_KERNEL);
> > > >  	if (!p)
> > > >  		return -ENOMEM;
> > > >  
> > > > @@ -166,7 +168,7 @@ int nand_onfi_detect(struct nand_chip *chip)
> > > >  		goto free_onfi_param_page;
> > > >  	}
> > > >  
> > > > -	for (i = 0; i < 3; i++) {
> > > > +	for (i = 0; i < ONFI_PARAM_PAGES; i++) {
> > > >  		ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> > > >  		if (ret) {
> > > >  			ret = 0;
> > > > @@ -181,8 +183,8 @@ int nand_onfi_detect(struct nand_chip *chip)
> > > >  		}
> > > >  	}
> > > >  
> > > > -	if (i == 3) {
> > > > -		const void *srcbufs[3] = {p, p + 1, p + 2};
> > > > +	if (i == ONFI_PARAM_PAGES) {
> > > > +		const void *srcbufs[ONFI_PARAM_PAGES] = {p, p + 1, p + 2};
> > > >        
> > > 
> > > Maybe initialize the srcbufs array using a for loop so you can easily
> > > change ONFI_PARAM_PAGES without having to touch the code. Looks good
> > > otherwise, so
> > > 
> > > Reviewed-by: Boris Breillon <boris.brezillon@collabora.com>
> > >     
> > 
> > Agreed, here is the snippet of code that I changed:
> > 
> > @@ -181,11 +183,15 @@ int nand_onfi_detect(struct nand_chip *chip)
> >                 }
> >         }
> >  
> > -       if (i == 3) {
> > -               const void *srcbufs[3] = {p, p + 1, p + 2};
> > +       if (i == ONFI_PARAM_PAGES) {
> > +               const void *srcbufs[ONFI_PARAM_PAGES];
> > +               int j;  
> 
> 		  ^unsigned int

Ok

> 
> > +
> > +               for (j = 0; j < ONFI_PARAM_PAGES; j++)
> > +                       srcbufs[j] = p + j;
> >  
> >                 pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
> > -               nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> > +               nand_bit_wise_majority(srcbufs, ONFI_PARAM_PAGES, p,
> >                                        sizeof(*p));
> >  
> >                 crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254);
> > 
> > 
> > 
> > Thanks,
> > Miquèl  
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 6/9] mtd: rawnand: onfi: Avoid doing a copy of the parameter page

[Miquel Raynal]

There is no need for copying the parameter page, playing with
pointers does the trick.

There is not functional change.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/nand_onfi.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index 9fe39adbde4c..6576b841bc56 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -143,7 +143,7 @@ int nand_onfi_detect(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct nand_memory_organization *memorg;
-	struct nand_onfi_params *p;
+	struct nand_onfi_params *p = NULL, *pbuf;
 	struct onfi_params *onfi;
 	int onfi_version = 0;
 	char id[4];
@@ -158,8 +158,8 @@ int nand_onfi_detect(struct nand_chip *chip)
 		return 0;
 
 	/* ONFI chip: allocate a buffer to hold its parameter page */
-	p = kzalloc((sizeof(*p) * ONFI_PARAM_PAGES), GFP_KERNEL);
-	if (!p)
+	pbuf = kzalloc((sizeof(*pbuf) * ONFI_PARAM_PAGES), GFP_KERNEL);
+	if (!pbuf)
 		return -ENOMEM;
 
 	ret = nand_read_param_page_op(chip, 0, NULL, 0);
@@ -169,32 +169,31 @@ int nand_onfi_detect(struct nand_chip *chip)
 	}
 
 	for (i = 0; i < ONFI_PARAM_PAGES; i++) {
-		ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
+		ret = nand_read_data_op(chip, &pbuf[i], sizeof(*pbuf), true);
 		if (ret) {
 			ret = 0;
 			goto free_onfi_param_page;
 		}
 
-		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254);
-		if (crc == le16_to_cpu(p[i].crc)) {
-			if (i)
-				memcpy(p, &p[i], sizeof(*p));
+		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&pbuf[i], 254);
+		if (crc == le16_to_cpu(pbuf[i].crc)) {
+			p = &pbuf[i];
 			break;
 		}
 	}
 
 	if (i == ONFI_PARAM_PAGES) {
-		const void *srcbufs[ONFI_PARAM_PAGES] = {p, p + 1, p + 2};
-
+		const void *srcbufs[ONFI_PARAM_PAGES] = {pbuf, pbuf + 1, pbuf + 2};
 		pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
-		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
-				       sizeof(*p));
+		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), pbuf,
+				       sizeof(*pbuf));
 
-		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254);
-		if (crc != le16_to_cpu(p->crc)) {
+		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)pbuf, 254);
+		if (crc != le16_to_cpu(pbuf->crc)) {
 			pr_err("ONFI parameter recovery failed, aborting\n");
 			goto free_onfi_param_page;
 		}
+		p = pbuf;
 	}
 
 	if (chip->manufacturer.desc && chip->manufacturer.desc->ops &&
@@ -302,14 +301,14 @@ int nand_onfi_detect(struct nand_chip *chip)
 	chip->parameters.onfi = onfi;
 
 	/* Identification done, free the full ONFI parameter page and exit */
-	kfree(p);
+	kfree(pbuf);
 
 	return 1;
 
 free_model:
 	kfree(chip->parameters.model);
 free_onfi_param_page:
-	kfree(p);
+	kfree(pbuf);
 
 	return ret;
 }
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 7/9] mtd: rawnand: onfi: Drop a useless parameter page read

[Miquel Raynal]

During detection the logic on the NAND bus is:

    /* Regular ONFI detection */
    1/ read the three NAND parameter pages

    /* Extended parameter page detection */
    2/ send "read the NAND parameter page" commands without reading
       actual data
    3/ move the column pointer to the extended page and read it

If fact, as long as there is nothing happening on the NAND bus between
1/ and 3/, the operation 2/ is redundant so remove it.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_onfi.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index 6576b841bc56..2fc71b7c361f 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -47,12 +47,10 @@ static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
 	if (!ep)
 		return -ENOMEM;
 
-	/* Send our own NAND_CMD_PARAM. */
-	ret = nand_read_param_page_op(chip, 0, NULL, 0);
-	if (ret)
-		goto ext_out;
-
-	/* Use the Change Read Column command to skip the ONFI param pages. */
+	/*
+	 * Use the Change Read Column command to skip the ONFI param pages and
+	 * ensure we read at the right location.
+	 */
 	ret = nand_change_read_column_op(chip,
 					 sizeof(*p) * p->num_of_param_pages,
 					 ep, len, true);
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 7/9] mtd: rawnand: onfi: Drop a useless parameter page read

[Boris Brezillon]

On Fri, 24 Apr 2020 18:40:40 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> During detection the logic on the NAND bus is:
> 
>     /* Regular ONFI detection */
>     1/ read the three NAND parameter pages
> 
>     /* Extended parameter page detection */
>     2/ send "read the NAND parameter page" commands without reading
>        actual data
>     3/ move the column pointer to the extended page and read it
> 
> If fact, as long as there is nothing happening on the NAND bus between
> 1/ and 3/, the operation 2/ is redundant so remove it.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/mtd/nand/raw/nand_onfi.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> index 6576b841bc56..2fc71b7c361f 100644
> --- a/drivers/mtd/nand/raw/nand_onfi.c
> +++ b/drivers/mtd/nand/raw/nand_onfi.c
> @@ -47,12 +47,10 @@ static int nand_flash_detect_ext_param_page(struct nand_chip *chip,
>  	if (!ep)
>  		return -ENOMEM;
>  
> -	/* Send our own NAND_CMD_PARAM. */
> -	ret = nand_read_param_page_op(chip, 0, NULL, 0);
> -	if (ret)
> -		goto ext_out;
> -
> -	/* Use the Change Read Column command to skip the ONFI param pages. */
> +	/*
> +	 * Use the Change Read Column command to skip the ONFI param pages and
> +	 * ensure we read at the right location.
> +	 */
>  	ret = nand_change_read_column_op(chip,
>  					 sizeof(*p) * p->num_of_param_pages,
>  					 ep, len, true);


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 8/9] mtd: rawnand: jedec: Define the number of parameter pages

[Miquel Raynal]

Use a macro to define the number of parameter page instead of
hardcoding it everywhere.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_jedec.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_jedec.c b/drivers/mtd/nand/raw/nand_jedec.c
index 9b540e76f84f..0cd322a8be24 100644
--- a/drivers/mtd/nand/raw/nand_jedec.c
+++ b/drivers/mtd/nand/raw/nand_jedec.c
@@ -16,6 +16,8 @@
 
 #include "internals.h"
 
+#define JEDEC_PARAM_PAGES 3
+
 /*
  * Check if the NAND chip is JEDEC compliant, returns 1 if it is, 0 otherwise.
  */
@@ -47,7 +49,7 @@ int nand_jedec_detect(struct nand_chip *chip)
 		goto free_jedec_param_page;
 	}
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < JEDEC_PARAM_PAGES; i++) {
 		ret = nand_read_data_op(chip, p, sizeof(*p), true);
 		if (ret) {
 			ret = 0;
@@ -59,7 +61,7 @@ int nand_jedec_detect(struct nand_chip *chip)
 			break;
 	}
 
-	if (i == 3) {
+	if (i == JEDEC_PARAM_PAGES) {
 		pr_err("Could not find valid JEDEC parameter page; aborting\n");
 		goto free_jedec_param_page;
 	}
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 8/9] mtd: rawnand: jedec: Define the number of parameter pages

[Boris Brezillon]

On Fri, 24 Apr 2020 18:40:41 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Use a macro to define the number of parameter page instead of
> hardcoding it everywhere.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/mtd/nand/raw/nand_jedec.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_jedec.c b/drivers/mtd/nand/raw/nand_jedec.c
> index 9b540e76f84f..0cd322a8be24 100644
> --- a/drivers/mtd/nand/raw/nand_jedec.c
> +++ b/drivers/mtd/nand/raw/nand_jedec.c
> @@ -16,6 +16,8 @@
>  
>  #include "internals.h"
>  
> +#define JEDEC_PARAM_PAGES 3
> +
>  /*
>   * Check if the NAND chip is JEDEC compliant, returns 1 if it is, 0 otherwise.
>   */
> @@ -47,7 +49,7 @@ int nand_jedec_detect(struct nand_chip *chip)
>  		goto free_jedec_param_page;
>  	}
>  
> -	for (i = 0; i < 3; i++) {
> +	for (i = 0; i < JEDEC_PARAM_PAGES; i++) {
>  		ret = nand_read_data_op(chip, p, sizeof(*p), true);
>  		if (ret) {
>  			ret = 0;
> @@ -59,7 +61,7 @@ int nand_jedec_detect(struct nand_chip *chip)
>  			break;
>  	}
>  
> -	if (i == 3) {
> +	if (i == JEDEC_PARAM_PAGES) {
>  		pr_err("Could not find valid JEDEC parameter page; aborting\n");
>  		goto free_jedec_param_page;
>  	}


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 9/9] mtd: rawnand: jedec: Use intermediate variables to improve readability

[Miquel Raynal]

Before reworking a little bit the JEDEC detection code, let's
clean the coding style of an if statement to improve readability.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/nand_jedec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_jedec.c b/drivers/mtd/nand/raw/nand_jedec.c
index 0cd322a8be24..15937e02c64f 100644
--- a/drivers/mtd/nand/raw/nand_jedec.c
+++ b/drivers/mtd/nand/raw/nand_jedec.c
@@ -30,6 +30,7 @@ int nand_jedec_detect(struct nand_chip *chip)
 	int jedec_version = 0;
 	char id[5];
 	int i, val, ret;
+	u16 crc;
 
 	memorg = nanddev_get_memorg(&chip->base);
 
@@ -56,8 +57,8 @@ int nand_jedec_detect(struct nand_chip *chip)
 			goto free_jedec_param_page;
 		}
 
-		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
-				le16_to_cpu(p->crc))
+		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 510);
+		if (crc == le16_to_cpu(p->crc))
 			break;
 	}
 
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/