[PATCH 0/8] Misc timing changes

[PATCH 0/8] 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
fixes.

Cheers,
Miquèl

Miquel Raynal (8):
  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 an intermediate variable to decomplefixy
    conditions
  mtd: rawnand: onfi: Avoid doing a copy of the parameter page
  mtd: rawnand: onfi: Simplify the NAND operations during detection
  mtd: rawnand: jedec: Use an intermediate variable to decomplefixy
    conditions
  mtd: rawnand: jedec: Simplify the NAND operations during detection

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

-- 
2.20.1


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

[PATCH 1/8] 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>
---
 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..d91f914d2e9e 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
+ * @timings.mode: Timing mode as referred 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/

Re: [PATCH 1/8] mtd: rawnand: timings: Add mode information to the timings structure

[Boris Brezillon]

On Tue, 21 Apr 2020 18:46:30 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> 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>

Two nits below.

> ---
>  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..d91f914d2e9e 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

			     ^timing information

> + * @timings.mode: Timing mode as referred in the specification

				  ^ as defined?

>   * @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;
>  };
>  


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

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

[Miquel Raynal]

These values are hardcoded, there was no need to try to convert them
in picoseconds, better write the right values in picoseconds directly.

Fixes: 6a943386ee36 mtd: rawnand: add default values for dynamic timings
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.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 2/8] mtd: rawnand: timings: Fix default tR_max and tCCS_min timings

[Boris Brezillon]

On Tue, 21 Apr 2020 18:46:31 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> These values are hardcoded, there was no need to try to convert them
> in picoseconds, better write the right values in picoseconds directly.

Hm I had a hard time understanding what was wrong and whether this fix
was correct or not. Maybe you should just say that tR and tCCS are
currently wrongly expressed in femto seconds and this is restoring it
to pico seconds.

Other than that, this diff looks good.

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

> 
> Fixes: 6a943386ee36 mtd: rawnand: add default values for dynamic timings
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.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;


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

[PATCH 3/8] 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>
---
 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 3/8] mtd: rawnand: onfi: Fix redundancy detection check

[Boris Brezillon]

On Tue, 21 Apr 2020 18:46:32 +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.
> 
> 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/

[PATCH 4/8] mtd: rawnand: onfi: Use an intermediate variable to decomplefixy conditions

[Miquel Raynal]

Before reworking a little bit the ONFI detection code, let's
decomplefixy the if statements.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.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/

Re: [PATCH 4/8] mtd: rawnand: onfi: Use an intermediate variable to decomplefixy conditions

[Boris Brezillon]

On Tue, 21 Apr 2020 18:46:33 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Before reworking a little bit the ONFI detection code, let's
> decomplefixy the if statements.

It's more about coding style/formatting than complexity, but the change
improves readability indeed, so

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

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.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;
>  		}


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

[PATCH 5/8] mtd: rawnand: onfi: Avoid doing a copy of the parameter page

[Miquel Raynal]

There is not need for copying the parameter page, playing with
pointers works the same.

There is not functional change.

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

diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index 7d9a3130443a..d6124180b47b 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -141,7 +141,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];
@@ -156,8 +156,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) * 3), GFP_KERNEL);
-	if (!p)
+	pbuf = kzalloc((sizeof(*pbuf) * 3), GFP_KERNEL);
+	if (!pbuf)
 		return -ENOMEM;
 
 	ret = nand_read_param_page_op(chip, 0, NULL, 0);
@@ -167,32 +167,31 @@ int nand_onfi_detect(struct nand_chip *chip)
 	}
 
 	for (i = 0; i < 3; 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 == 3) {
-		const void *srcbufs[3] = {p, p + 1, p + 2};
-
+		const void *srcbufs[3] = {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 &&
@@ -300,7 +299,7 @@ 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;
 
-- 
2.20.1


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

Re: [PATCH 5/8] mtd: rawnand: onfi: Avoid doing a copy of the parameter page

[Boris Brezillon]

On Tue, 21 Apr 2020 18:46:34 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> There is not need for copying the parameter page, playing with

	    ^no

> pointers works the same.

	   ^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 | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> index 7d9a3130443a..d6124180b47b 100644
> --- a/drivers/mtd/nand/raw/nand_onfi.c
> +++ b/drivers/mtd/nand/raw/nand_onfi.c
> @@ -141,7 +141,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];
> @@ -156,8 +156,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) * 3), GFP_KERNEL);
> -	if (!p)
> +	pbuf = kzalloc((sizeof(*pbuf) * 3), GFP_KERNEL);
> +	if (!pbuf)
>  		return -ENOMEM;
>  
>  	ret = nand_read_param_page_op(chip, 0, NULL, 0);
> @@ -167,32 +167,31 @@ int nand_onfi_detect(struct nand_chip *chip)
>  	}
>  
>  	for (i = 0; i < 3; 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 == 3) {
> -		const void *srcbufs[3] = {p, p + 1, p + 2};
> -
> +		const void *srcbufs[3] = {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 &&
> @@ -300,7 +299,7 @@ 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;
>  


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

[PATCH 6/8] mtd: rawnand: onfi: Simplify the NAND operations during detection

[Miquel Raynal]

There is no need for separate parameter page reads, the delay penalty
is negligible so let's do read the three copies in one go.

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

diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
index d6124180b47b..b76772666b82 100644
--- a/drivers/mtd/nand/raw/nand_onfi.c
+++ b/drivers/mtd/nand/raw/nand_onfi.c
@@ -160,19 +160,13 @@ int nand_onfi_detect(struct nand_chip *chip)
 	if (!pbuf)
 		return -ENOMEM;
 
-	ret = nand_read_param_page_op(chip, 0, NULL, 0);
+	ret = nand_read_param_page_op(chip, 0, pbuf, 3 * sizeof(*pbuf));
 	if (ret) {
 		ret = 0;
 		goto free_onfi_param_page;
 	}
 
 	for (i = 0; i < 3; i++) {
-		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 *)&pbuf[i], 254);
 		if (crc == le16_to_cpu(pbuf[i].crc)) {
 			p = &pbuf[i];
-- 
2.20.1


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

Re: [PATCH 6/8] mtd: rawnand: onfi: Simplify the NAND operations during detection

[Boris Brezillon]

On Tue, 21 Apr 2020 18:46:35 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> There is no need for separate parameter page reads, the delay penalty
> is negligible so let's do read the three copies in one go.

		    ^let's read

In theory that's correct, but I fear this was done because some
controllers couldn't read 768 bytes in one go. Could we do that only if
the controller implements exec_op() and exec_op(check_only) returns true?

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_onfi.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> index d6124180b47b..b76772666b82 100644
> --- a/drivers/mtd/nand/raw/nand_onfi.c
> +++ b/drivers/mtd/nand/raw/nand_onfi.c
> @@ -160,19 +160,13 @@ int nand_onfi_detect(struct nand_chip *chip)
>  	if (!pbuf)
>  		return -ENOMEM;
>  
> -	ret = nand_read_param_page_op(chip, 0, NULL, 0);
> +	ret = nand_read_param_page_op(chip, 0, pbuf, 3 * sizeof(*pbuf));
>  	if (ret) {
>  		ret = 0;
>  		goto free_onfi_param_page;
>  	}
>  
>  	for (i = 0; i < 3; i++) {
> -		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 *)&pbuf[i], 254);
>  		if (crc == le16_to_cpu(pbuf[i].crc)) {
>  			p = &pbuf[i];


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

Re: [PATCH 6/8] mtd: rawnand: onfi: Simplify the NAND operations during detection

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 22 Apr
2020 09:00:52 +0200:

> On Tue, 21 Apr 2020 18:46:35 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > There is no need for separate parameter page reads, the delay penalty
> > is negligible so let's do read the three copies in one go.  
> 
> 		    ^let's read
> 
> In theory that's correct, but I fear this was done because some
> controllers couldn't read 768 bytes in one go. Could we do that only if
> the controller implements exec_op() and exec_op(check_only) returns true?

Thanks for reviewing all the patches. You are right that it might break
drivers so I'll find a more appropriate way to do it (same for JEDEC).

> 
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_onfi.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> > index d6124180b47b..b76772666b82 100644
> > --- a/drivers/mtd/nand/raw/nand_onfi.c
> > +++ b/drivers/mtd/nand/raw/nand_onfi.c
> > @@ -160,19 +160,13 @@ int nand_onfi_detect(struct nand_chip *chip)
> >  	if (!pbuf)
> >  		return -ENOMEM;
> >  
> > -	ret = nand_read_param_page_op(chip, 0, NULL, 0);
> > +	ret = nand_read_param_page_op(chip, 0, pbuf, 3 * sizeof(*pbuf));
> >  	if (ret) {
> >  		ret = 0;
> >  		goto free_onfi_param_page;
> >  	}
> >  
> >  	for (i = 0; i < 3; i++) {
> > -		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 *)&pbuf[i], 254);
> >  		if (crc == le16_to_cpu(pbuf[i].crc)) {
> >  			p = &pbuf[i];  
> 

Thanks,
Miquèl

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

[PATCH 7/8] mtd: rawnand: jedec: Use an intermediate variable to decomplefixy conditions

[Miquel Raynal]

Before reworking a little bit the JEDEC detection code, let's
decomplefixy an if statement.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.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 9b540e76f84f..4cc1ea512887 100644
--- a/drivers/mtd/nand/raw/nand_jedec.c
+++ b/drivers/mtd/nand/raw/nand_jedec.c
@@ -28,6 +28,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);
 
@@ -54,8 +55,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/

Re: [PATCH 7/8] mtd: rawnand: jedec: Use an intermediate variable to decomplefixy conditions

[Boris Brezillon]

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

> Before reworking a little bit the JEDEC detection code, let's
> decomplefixy an if statement.

Same comment as earlier, it's not about complexity but readability.

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

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.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 9b540e76f84f..4cc1ea512887 100644
> --- a/drivers/mtd/nand/raw/nand_jedec.c
> +++ b/drivers/mtd/nand/raw/nand_jedec.c
> @@ -28,6 +28,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);
>  
> @@ -54,8 +55,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;
>  	}
>  


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

Re: [PATCH 7/8] mtd: rawnand: jedec: Use an intermediate variable to decomplefixy conditions

[Sergei Shtylyov]

Hello!

On 21.04.2020 19:46, Miquel Raynal wrote:

> Before reworking a little bit the JEDEC detection code, let's
> decomplefixy an if statement.

    Decomplexify, maybe? :-)

> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
[...]

MBR, Sergei

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

Re: [PATCH 7/8] mtd: rawnand: jedec: Use an intermediate variable to decomplefixy conditions

[Miquel Raynal]

Hi Sergei,

Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote on Wed, 22
Apr 2020 11:49:14 +0300:

> Hello!
> 
> On 21.04.2020 19:46, Miquel Raynal wrote:
> 
> > Before reworking a little bit the JEDEC detection code, let's
> > decomplefixy an if statement.  
> 
>     Decomplexify, maybe? :-)

Oops :)

> 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> [...]
> 
> MBR, Sergei

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

[PATCH 8/8] mtd: rawnand: jedec: Simplify the NAND operations during detection

[Miquel Raynal]

There is no need for separate parameter page reads, the delay penalty
is negligible so let's do read the three copies in one go.

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

diff --git a/drivers/mtd/nand/raw/nand_jedec.c b/drivers/mtd/nand/raw/nand_jedec.c
index 4cc1ea512887..befe8c29b505 100644
--- a/drivers/mtd/nand/raw/nand_jedec.c
+++ b/drivers/mtd/nand/raw/nand_jedec.c
@@ -23,7 +23,7 @@ int nand_jedec_detect(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct nand_memory_organization *memorg;
-	struct nand_jedec_params *p;
+	struct nand_jedec_params *p = NULL, *pbuf;
 	struct jedec_ecc_info *ecc;
 	int jedec_version = 0;
 	char id[5];
@@ -38,25 +38,19 @@ int nand_jedec_detect(struct nand_chip *chip)
 		return 0;
 
 	/* JEDEC chip: allocate a buffer to hold its parameter page */
-	p = kzalloc(sizeof(*p), GFP_KERNEL);
-	if (!p)
+	pbuf = kzalloc(sizeof(*pbuf) * 3, GFP_KERNEL);
+	if (!pbuf)
 		return -ENOMEM;
 
-	ret = nand_read_param_page_op(chip, 0x40, NULL, 0);
+	ret = nand_read_param_page_op(chip, 0x40, pbuf, 3 * sizeof(*pbuf));
 	if (ret) {
 		ret = 0;
 		goto free_jedec_param_page;
 	}
 
 	for (i = 0; i < 3; i++) {
-		ret = nand_read_data_op(chip, p, sizeof(*p), true);
-		if (ret) {
-			ret = 0;
-			goto free_jedec_param_page;
-		}
-
-		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 510);
-		if (crc == le16_to_cpu(p->crc))
+		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&pbuf[i], 510);
+		if (crc == le16_to_cpu(pbuf[i].crc))
 			break;
 	}
 
@@ -65,6 +59,8 @@ int nand_jedec_detect(struct nand_chip *chip)
 		goto free_jedec_param_page;
 	}
 
+	p = pbuf;
+
 	/* Check version */
 	val = le16_to_cpu(p->revision);
 	if (val & (1 << 2))
-- 
2.20.1


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

Re: [PATCH 8/8] mtd: rawnand: jedec: Simplify the NAND operations during detection

[Boris Brezillon]

On Tue, 21 Apr 2020 18:46:37 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> There is no need for separate parameter page reads, the delay penalty
> is negligible so let's do read the three copies in one go.

Same comment as for the ONFI param page read. Let's make sure we don't
break existing users by using exec_op()'s check capability.

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_jedec.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_jedec.c b/drivers/mtd/nand/raw/nand_jedec.c
> index 4cc1ea512887..befe8c29b505 100644
> --- a/drivers/mtd/nand/raw/nand_jedec.c
> +++ b/drivers/mtd/nand/raw/nand_jedec.c
> @@ -23,7 +23,7 @@ int nand_jedec_detect(struct nand_chip *chip)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	struct nand_memory_organization *memorg;
> -	struct nand_jedec_params *p;
> +	struct nand_jedec_params *p = NULL, *pbuf;
>  	struct jedec_ecc_info *ecc;
>  	int jedec_version = 0;
>  	char id[5];
> @@ -38,25 +38,19 @@ int nand_jedec_detect(struct nand_chip *chip)
>  		return 0;
>  
>  	/* JEDEC chip: allocate a buffer to hold its parameter page */
> -	p = kzalloc(sizeof(*p), GFP_KERNEL);
> -	if (!p)
> +	pbuf = kzalloc(sizeof(*pbuf) * 3, GFP_KERNEL);
> +	if (!pbuf)
>  		return -ENOMEM;
>  
> -	ret = nand_read_param_page_op(chip, 0x40, NULL, 0);
> +	ret = nand_read_param_page_op(chip, 0x40, pbuf, 3 * sizeof(*pbuf));
>  	if (ret) {
>  		ret = 0;
>  		goto free_jedec_param_page;
>  	}
>  
>  	for (i = 0; i < 3; i++) {
> -		ret = nand_read_data_op(chip, p, sizeof(*p), true);
> -		if (ret) {
> -			ret = 0;
> -			goto free_jedec_param_page;
> -		}
> -
> -		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 510);
> -		if (crc == le16_to_cpu(p->crc))
> +		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&pbuf[i], 510);
> +		if (crc == le16_to_cpu(pbuf[i].crc))
>  			break;
>  	}
>  
> @@ -65,6 +59,8 @@ int nand_jedec_detect(struct nand_chip *chip)
>  		goto free_jedec_param_page;
>  	}
>  
> +	p = pbuf;
> +
>  	/* Check version */
>  	val = le16_to_cpu(p->revision);
>  	if (val & (1 << 2))


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