[PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Benedikt Spranger]

Hi,

while porting an ATSAMA5D2 based board (booting from NAND flash with UBI)
I stumbled over generic, NAND- and UBI related asperity:

1. tiny-printf does not handle NULL arguments to '%s' in a proper way
2. vtbl_check() has an useless debug output due to a typo
3. NAND: An informative output fails badly, if the NAND vendor is unknown
4. the nand_atmel driver lacks a vital initialisation

OK, you might say the nand_atmel NAND driver is obsolete, but it was the
only solution to get booting from NAND running. The new DM based NAND driver
refused to do anything usefull, so I dropped it after spending a couple of
days debugging it:

<RANT>brilliant!</RANT>



regards
    Benedikt Spranger

Benedikt Spranger (5):
  tiny-printf: Handle NULL pointer argument to %s
  drivers/mtd/ubispl/ubispl.c: Fix error message
  mtd: nand: raw: Fix potential NULL pointer dereference
  mtd: nand: Update NAND manufacturer Ids
  mtd: nand: raw: atmel_nand: Add missing nand_scan_ident()

 drivers/mtd/nand/raw/atmel_nand.c |  4 ++++
 drivers/mtd/nand/raw/nand_base.c  |  8 ++++----
 drivers/mtd/nand/raw/nand_ids.c   | 22 ++++++++++++----------
 drivers/mtd/ubispl/ubispl.c       |  2 +-
 include/linux/mtd/rawnand.h       | 23 +++++++++++++----------
 lib/tiny-printf.c                 |  2 +-
 6 files changed, 35 insertions(+), 26 deletions(-)

-- 
2.45.2

[PATCH 0/5] collected fallout of porting an ATSAMA5D2 based board

[Benedikt Spranger]

Hi,

while porting an ATSAMA5D2 based board (booting from NAND flash with UBI)
I stumbled over generic, NAND- and UBI related asperity:

1. tiny-printf does not handle NULL arguments to '%s' in a proper way
2. vtbl_check() has an useless debug output due to a typo
3. NAND: An informative output fails badly, if the NAND vendor is unknown
4. the nand_atmel driver lacks a vital initialisation

OK, you might say the nand_atmel NAND driver is obsolete, but it was the
only solution to get booting from NAND running. The new DM based NAND driver
refused to do anything usefull, so I dropped it after spending a couple of
days debugging it:

<RANT>brilliant!</RANT>

regards
    Benedikt Spranger

Benedikt Spranger (5):
  tiny-printf: Handle NULL pointer argument to %s
  drivers/mtd/ubispl/ubispl.c: Fix error message
  mtd: nand: raw: Fix potential NULL pointer dereference
  mtd: nand: Update NAND manufacturer Ids
  mtd: nand: raw: atmel_nand: Add missing nand_scan_ident()

 drivers/mtd/nand/raw/atmel_nand.c |  4 ++++
 drivers/mtd/nand/raw/nand_base.c  |  8 ++++----
 drivers/mtd/nand/raw/nand_ids.c   | 22 ++++++++++++----------
 drivers/mtd/ubispl/ubispl.c       |  2 +-
 include/linux/mtd/rawnand.h       | 23 +++++++++++++----------
 lib/tiny-printf.c                 |  2 +-
 6 files changed, 35 insertions(+), 26 deletions(-)

-- 
2.45.2

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D2 based board

[Eugen Hristev]

On 10/18/24 11:30, Benedikt Spranger wrote:
> Hi,
> 
> while porting an ATSAMA5D2 based board (booting from NAND flash with UBI)
> I stumbled over generic, NAND- and UBI related asperity:
> 
> 1. tiny-printf does not handle NULL arguments to '%s' in a proper way
> 2. vtbl_check() has an useless debug output due to a typo
> 3. NAND: An informative output fails badly, if the NAND vendor is unknown
> 4. the nand_atmel driver lacks a vital initialisation
> 
> OK, you might say the nand_atmel NAND driver is obsolete, but it was the
> only solution to get booting from NAND running. The new DM based NAND driver
> refused to do anything usefull, so I dropped it after spending a couple of
> days debugging it:
> 
> <RANT>brilliant!</RANT>
> 
> regards
>     Benedikt Spranger
> 
> Benedikt Spranger (5):
>   tiny-printf: Handle NULL pointer argument to %s
>   drivers/mtd/ubispl/ubispl.c: Fix error message
>   mtd: nand: raw: Fix potential NULL pointer dereference
>   mtd: nand: Update NAND manufacturer Ids
>   mtd: nand: raw: atmel_nand: Add missing nand_scan_ident()
> 
>  drivers/mtd/nand/raw/atmel_nand.c |  4 ++++
>  drivers/mtd/nand/raw/nand_base.c  |  8 ++++----
>  drivers/mtd/nand/raw/nand_ids.c   | 22 ++++++++++++----------
>  drivers/mtd/ubispl/ubispl.c       |  2 +-
>  include/linux/mtd/rawnand.h       | 23 +++++++++++++----------
>  lib/tiny-printf.c                 |  2 +-
>  6 files changed, 35 insertions(+), 26 deletions(-)
> 

Coming back to these patches I had a look and except a typo , it looks good.
I can take these through at91 tree or maybe Michael you want to have a
look as they touch the MTD/NAND drivers ?

Thanks,
Eugen

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D2 based board

[Eugen Hristev]

On 11/12/24 15:45, Eugen Hristev wrote:
> 
> 
> On 10/18/24 11:30, Benedikt Spranger wrote:
>> Hi,
>>
>> while porting an ATSAMA5D2 based board (booting from NAND flash with UBI)
>> I stumbled over generic, NAND- and UBI related asperity:
>>
>> 1. tiny-printf does not handle NULL arguments to '%s' in a proper way
>> 2. vtbl_check() has an useless debug output due to a typo
>> 3. NAND: An informative output fails badly, if the NAND vendor is unknown
>> 4. the nand_atmel driver lacks a vital initialisation
>>
>> OK, you might say the nand_atmel NAND driver is obsolete, but it was the
>> only solution to get booting from NAND running. The new DM based NAND driver
>> refused to do anything usefull, so I dropped it after spending a couple of
>> days debugging it:
>>
>> <RANT>brilliant!</RANT>
>>
>> regards
>>     Benedikt Spranger
>>
>> Benedikt Spranger (5):
>>   tiny-printf: Handle NULL pointer argument to %s
>>   drivers/mtd/ubispl/ubispl.c: Fix error message
>>   mtd: nand: raw: Fix potential NULL pointer dereference
>>   mtd: nand: Update NAND manufacturer Ids
>>   mtd: nand: raw: atmel_nand: Add missing nand_scan_ident()
>>
>>  drivers/mtd/nand/raw/atmel_nand.c |  4 ++++
>>  drivers/mtd/nand/raw/nand_base.c  |  8 ++++----
>>  drivers/mtd/nand/raw/nand_ids.c   | 22 ++++++++++++----------
>>  drivers/mtd/ubispl/ubispl.c       |  2 +-
>>  include/linux/mtd/rawnand.h       | 23 +++++++++++++----------
>>  lib/tiny-printf.c                 |  2 +-
>>  6 files changed, 35 insertions(+), 26 deletions(-)
>>
> 
> Coming back to these patches I had a look and except a typo , it looks good.
> I can take these through at91 tree or maybe Michael you want to have a
> look as they touch the MTD/NAND drivers ?
> 
> Thanks,
> Eugen


I have applied patches 1 and 2 to u-boot-at91/next .
The other three would need a resubmit .

Eugen

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D2 based board

[Michael Nazzareno Trimarchi]

HI all

On Tue, Nov 19, 2024 at 4:46 PM Eugen Hristev <eugen.hristev@linaro.org> wrote:
>
>
>
> On 11/12/24 15:45, Eugen Hristev wrote:
> >
> >
> > On 10/18/24 11:30, Benedikt Spranger wrote:
> >> Hi,
> >>
> >> while porting an ATSAMA5D2 based board (booting from NAND flash with UBI)
> >> I stumbled over generic, NAND- and UBI related asperity:
> >>
> >> 1. tiny-printf does not handle NULL arguments to '%s' in a proper way
> >> 2. vtbl_check() has an useless debug output due to a typo
> >> 3. NAND: An informative output fails badly, if the NAND vendor is unknown
> >> 4. the nand_atmel driver lacks a vital initialisation
> >>
> >> OK, you might say the nand_atmel NAND driver is obsolete, but it was the
> >> only solution to get booting from NAND running. The new DM based NAND driver
> >> refused to do anything usefull, so I dropped it after spending a couple of
> >> days debugging it:
> >>
> >> <RANT>brilliant!</RANT>
> >>

I totally missed those patches. I'm really concerned about it. I will
review the respin

Michael

> >> regards
> >>     Benedikt Spranger
> >>
> >> Benedikt Spranger (5):
> >>   tiny-printf: Handle NULL pointer argument to %s
> >>   drivers/mtd/ubispl/ubispl.c: Fix error message
> >>   mtd: nand: raw: Fix potential NULL pointer dereference
> >>   mtd: nand: Update NAND manufacturer Ids
> >>   mtd: nand: raw: atmel_nand: Add missing nand_scan_ident()
> >>
> >>  drivers/mtd/nand/raw/atmel_nand.c |  4 ++++
> >>  drivers/mtd/nand/raw/nand_base.c  |  8 ++++----
> >>  drivers/mtd/nand/raw/nand_ids.c   | 22 ++++++++++++----------
> >>  drivers/mtd/ubispl/ubispl.c       |  2 +-
> >>  include/linux/mtd/rawnand.h       | 23 +++++++++++++----------
> >>  lib/tiny-printf.c                 |  2 +-
> >>  6 files changed, 35 insertions(+), 26 deletions(-)
> >>
> >
> > Coming back to these patches I had a look and except a typo , it looks good.
> > I can take these through at91 tree or maybe Michael you want to have a
> > look as they touch the MTD/NAND drivers ?
> >
> > Thanks,
> > Eugen
>
>
> I have applied patches 1 and 2 to u-boot-at91/next .
> The other three would need a resubmit .
>
> Eugen



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@amarulasolutions.com
www.amarulasolutions.com

[PATCH 1/5] tiny-printf: Handle NULL pointer argument to %s

[Benedikt Spranger]

A NULL pointer argument to %s causes a NULL pointer dereference in the
fixed width numerical printout code, since p is overwritten with NULL.
In case of %s width is 0. Check width before dereferencing the pointer.

Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
---
 lib/tiny-printf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index 9a70c6095b3..b2f31c4004a 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -311,7 +311,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 
 			*info->bf = 0;
 			info->bf = p;
-			while (*info->bf++ && width > 0)
+			while (width > 0 && info->bf && *info->bf++)
 				width--;
 			while (width-- > 0)
 				info->putc(info, lz ? '0' : ' ');
-- 
2.45.2

[PATCH 2/5] drivers/mtd/ubispl/ubispl.c: Fix error message

[Benedikt Spranger]

The bad CRC error message has transposed characters, which render the
output useless:

"bad CRC at record 213: #08x, not #08x" instead of
"bad CRC at record 213: #00000000, not #4be31f4d"

Fix the error message.

Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/ubispl/ubispl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubispl/ubispl.c b/drivers/mtd/ubispl/ubispl.c
index 90a7c4c6f9e..9face5fae15 100644
--- a/drivers/mtd/ubispl/ubispl.c
+++ b/drivers/mtd/ubispl/ubispl.c
@@ -113,7 +113,7 @@ static int vtbl_check(struct ubi_scan_info *ubi,
 
 		crc = crc32(UBI_CRC32_INIT, &vtbl[i], UBI_VTBL_RECORD_SIZE_CRC);
 		if (be32_to_cpu(vtbl[i].crc) != crc) {
-			ubi_err("bad CRC at record %u: %#08x, not %#08x",
+			ubi_err("bad CRC at record %u: #%08x, not #%08x",
 				i, crc, be32_to_cpu(vtbl[i].crc));
 			ubi_dump_vtbl_record(&vtbl[i], i);
 			return 1;
-- 
2.45.2

[PATCH 3/5] mtd: nand: raw: Fix potential NULL pointer dereference

[Benedikt Spranger]

A NAND manufacture ID may not be found in nand_manuf_ids[] database.
In case of an ONFI or JEDEC NAND a crutial NULL pointer check is missing
and printing out the manufacture name result in a NULL pointer
dereference. Instead of adding additional NULL pointer checks ensure
a valid nand_menufacture_desc and remove the checks entirely.

Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/nand/raw/nand_base.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 4401bdcdb90..5dbc47bfe5f 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4241,7 +4241,7 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
  * @id: manufacturer ID
  *
  * Returns a nand_manufacturer_desc object if the manufacturer is defined
- * in the NAND manufacturers database, NULL otherwise.
+ * in the NAND manufacturers database, "Unknown" entry otherwise.
  */
 static const struct nand_manufacturer *nand_get_manufacturer_desc(u8 id)
 {
@@ -4252,7 +4252,7 @@ static const struct nand_manufacturer *nand_get_manufacturer_desc(u8 id)
 			return &nand_manuf_ids[i];
 	}
 
-	return NULL;
+	return &nand_manuf_ids[i];
 }
 
 /*
@@ -4425,13 +4425,13 @@ ident_done:
 	else if (chip->jedec_version)
 		pr_info("%s %s\n", manufacturer_desc->name,
 			chip->jedec_params.model);
-	else if (manufacturer_desc)
+	else
 		pr_info("%s %s\n", manufacturer_desc->name, type->name);
 #else
 	if (chip->jedec_version)
 		pr_info("%s %s\n", manufacturer_desc->name,
 			chip->jedec_params.model);
-	else if (manufacturer_desc)
+	else
 		pr_info("%s %s\n", manufacturer_desc->name, type->name);
 #endif
 
-- 
2.45.2

[PATCH 4/5] mtd: nand: Update NAND manufacturer Ids

[Benedikt Spranger]

Align manufacturer Ids with the Id list from Linux kernel v6.11.
While at it, sort the entries in alphabetical order.

Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/nand/raw/nand_ids.c | 22 ++++++++++++----------
 include/linux/mtd/rawnand.h     | 23 +++++++++++++----------
 2 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
index 4f46378ffe1..e40facf774c 100644
--- a/drivers/mtd/nand/raw/nand_ids.c
+++ b/drivers/mtd/nand/raw/nand_ids.c
@@ -180,20 +180,22 @@ struct nand_flash_dev nand_flash_ids[] = {
 
 /* Manufacturer IDs */
 struct nand_manufacturer nand_manuf_ids[] = {
-	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
-	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
+	{NAND_MFR_AMD, "AMD/Spansion", &amd_nand_manuf_ops},
+	{NAND_MFR_ATO, "ATO"},
+	{NAND_MFR_EON, "Eon"},
+	{NAND_MFR_EON, "ESMT"},
 	{NAND_MFR_FUJITSU, "Fujitsu"},
-	{NAND_MFR_NATIONAL, "National"},
-	{NAND_MFR_RENESAS, "Renesas"},
-	{NAND_MFR_STMICRO, "ST Micro"},
 	{NAND_MFR_HYNIX, "Hynix", &hynix_nand_manuf_ops},
-	{NAND_MFR_MICRON, "Micron", &micron_nand_manuf_ops},
-	{NAND_MFR_AMD, "AMD/Spansion", &amd_nand_manuf_ops},
+	{NAND_MFR_INTEL, "Intel"},
 	{NAND_MFR_MACRONIX, "Macronix", &macronix_nand_manuf_ops},
-	{NAND_MFR_EON, "Eon"},
+	{NAND_MFR_MICRON, "Micron", &micron_nand_manuf_ops},
+	{NAND_MFR_NATIONAL, "National"},
+	{NAND_MFR_RENESAS, "Renesas"},
+	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
 	{NAND_MFR_SANDISK, "SanDisk"},
-	{NAND_MFR_INTEL, "Intel"},
-	{NAND_MFR_ATO, "ATO"},
+	{NAND_MFR_STMICRO, "ST Micro"},
+	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
+	{NAND_MFR_WINBOND, "Winbond"},
 	{0x0, "Unknown"}
 };
 
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 537c62424a1..eaa6ec84b07 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1030,20 +1030,23 @@ static inline void *nand_get_manufacturer_data(struct nand_chip *chip)
 /*
  * NAND Flash Manufacturer ID Codes
  */
-#define NAND_MFR_TOSHIBA	0x98
-#define NAND_MFR_SAMSUNG	0xec
+#define NAND_MFR_AMD		0x01
+#define NAND_MFR_ATO		0x9b
+#define NAND_MFR_EON		0x92
+#define NAND_MFR_ESMT		0xc8
 #define NAND_MFR_FUJITSU	0x04
-#define NAND_MFR_NATIONAL	0x8f
-#define NAND_MFR_RENESAS	0x07
-#define NAND_MFR_STMICRO	0x20
 #define NAND_MFR_HYNIX		0xad
-#define NAND_MFR_MICRON		0x2c
-#define NAND_MFR_AMD		0x01
+#define NAND_MFR_INTEL		0x89
 #define NAND_MFR_MACRONIX	0xc2
-#define NAND_MFR_EON		0x92
+#define NAND_MFR_MICRON		0x2c
+#define NAND_MFR_NATIONAL	0x8f
+#define NAND_MFR_RENESAS	0x07
+#define NAND_MFR_SAMSUNG	0xec
 #define NAND_MFR_SANDISK	0x45
-#define NAND_MFR_INTEL		0x89
-#define NAND_MFR_ATO		0x9b
+#define NAND_MFR_STMICRO	0x20
+/* Kioxia is new name of Toshiba memory. */
+#define NAND_MFR_TOSHIBA	0x98
+#define NAND_MFR_WINBOND	0xef
 
 /* The maximum expected count of bytes in the NAND ID sequence */
 #define NAND_MAX_ID_LEN 8
-- 
2.45.2

Re: [PATCH 4/5] mtd: nand: Update NAND manufacturer Ids

[Eugen Hristev]

Hi Benedikt ,

On 10/18/24 11:30, Benedikt Spranger wrote:
> Align manufacturer Ids with the Id list from Linux kernel v6.11.
> While at it, sort the entries in alphabetical order.
> 
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> Reviewed-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/mtd/nand/raw/nand_ids.c | 22 ++++++++++++----------
>  include/linux/mtd/rawnand.h     | 23 +++++++++++++----------
>  2 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
> index 4f46378ffe1..e40facf774c 100644
> --- a/drivers/mtd/nand/raw/nand_ids.c
> +++ b/drivers/mtd/nand/raw/nand_ids.c
> @@ -180,20 +180,22 @@ struct nand_flash_dev nand_flash_ids[] = {
>  
>  /* Manufacturer IDs */
>  struct nand_manufacturer nand_manuf_ids[] = {
> -	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
> -	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
> +	{NAND_MFR_AMD, "AMD/Spansion", &amd_nand_manuf_ops},
> +	{NAND_MFR_ATO, "ATO"},
> +	{NAND_MFR_EON, "Eon"},
> +	{NAND_MFR_EON, "ESMT"},

Did you mean NAND_MFR_ESMT here ?

>  	{NAND_MFR_FUJITSU, "Fujitsu"},
> -	{NAND_MFR_NATIONAL, "National"},
> -	{NAND_MFR_RENESAS, "Renesas"},
> -	{NAND_MFR_STMICRO, "ST Micro"},
>  	{NAND_MFR_HYNIX, "Hynix", &hynix_nand_manuf_ops},
> -	{NAND_MFR_MICRON, "Micron", &micron_nand_manuf_ops},
> -	{NAND_MFR_AMD, "AMD/Spansion", &amd_nand_manuf_ops},
> +	{NAND_MFR_INTEL, "Intel"},
>  	{NAND_MFR_MACRONIX, "Macronix", &macronix_nand_manuf_ops},
> -	{NAND_MFR_EON, "Eon"},
> +	{NAND_MFR_MICRON, "Micron", &micron_nand_manuf_ops},
> +	{NAND_MFR_NATIONAL, "National"},
> +	{NAND_MFR_RENESAS, "Renesas"},
> +	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
>  	{NAND_MFR_SANDISK, "SanDisk"},
> -	{NAND_MFR_INTEL, "Intel"},
> -	{NAND_MFR_ATO, "ATO"},
> +	{NAND_MFR_STMICRO, "ST Micro"},
> +	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
> +	{NAND_MFR_WINBOND, "Winbond"},
>  	{0x0, "Unknown"}
>  };
>  
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 537c62424a1..eaa6ec84b07 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -1030,20 +1030,23 @@ static inline void *nand_get_manufacturer_data(struct nand_chip *chip)
>  /*
>   * NAND Flash Manufacturer ID Codes
>   */
> -#define NAND_MFR_TOSHIBA	0x98
> -#define NAND_MFR_SAMSUNG	0xec
> +#define NAND_MFR_AMD		0x01
> +#define NAND_MFR_ATO		0x9b
> +#define NAND_MFR_EON		0x92
> +#define NAND_MFR_ESMT		0xc8
>  #define NAND_MFR_FUJITSU	0x04
> -#define NAND_MFR_NATIONAL	0x8f
> -#define NAND_MFR_RENESAS	0x07
> -#define NAND_MFR_STMICRO	0x20
>  #define NAND_MFR_HYNIX		0xad
> -#define NAND_MFR_MICRON		0x2c
> -#define NAND_MFR_AMD		0x01
> +#define NAND_MFR_INTEL		0x89
>  #define NAND_MFR_MACRONIX	0xc2
> -#define NAND_MFR_EON		0x92
> +#define NAND_MFR_MICRON		0x2c
> +#define NAND_MFR_NATIONAL	0x8f
> +#define NAND_MFR_RENESAS	0x07
> +#define NAND_MFR_SAMSUNG	0xec
>  #define NAND_MFR_SANDISK	0x45
> -#define NAND_MFR_INTEL		0x89
> -#define NAND_MFR_ATO		0x9b
> +#define NAND_MFR_STMICRO	0x20
> +/* Kioxia is new name of Toshiba memory. */
> +#define NAND_MFR_TOSHIBA	0x98
> +#define NAND_MFR_WINBOND	0xef
>  
>  /* The maximum expected count of bytes in the NAND ID sequence */
>  #define NAND_MAX_ID_LEN 8

Re: [PATCH 4/5] mtd: nand: Update NAND manufacturer Ids

[Benedikt Spranger]

On Tue, 12 Nov 2024 15:39:23 +0200
Eugen Hristev <eugen.hristev@linaro.org> wrote:

Hi Eugen,
> On 10/18/24 11:30, Benedikt Spranger wrote:
> > Align manufacturer Ids with the Id list from Linux kernel v6.11.
> > While at it, sort the entries in alphabetical order.
> > 
> > Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> > Reviewed-by: John Ogness <john.ogness@linutronix.de>
> > ---
> >  drivers/mtd/nand/raw/nand_ids.c | 22 ++++++++++++----------
> >  include/linux/mtd/rawnand.h     | 23 +++++++++++++----------
> >  2 files changed, 25 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
> > index 4f46378ffe1..e40facf774c 100644
> > --- a/drivers/mtd/nand/raw/nand_ids.c
> > +++ b/drivers/mtd/nand/raw/nand_ids.c
> > @@ -180,20 +180,22 @@ struct nand_flash_dev nand_flash_ids[] = {
> >  
> >  /* Manufacturer IDs */
> >  struct nand_manufacturer nand_manuf_ids[] = {
> > -	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
> > -	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
> > +	{NAND_MFR_AMD, "AMD/Spansion", &amd_nand_manuf_ops},
> > +	{NAND_MFR_ATO, "ATO"},
> > +	{NAND_MFR_EON, "Eon"},
> > +	{NAND_MFR_EON, "ESMT"},
> 
> Did you mean NAND_MFR_ESMT here ?
Yes, fatfingered that...

Regards
    Benedikt Spranger

Re: [PATCH 4/5] mtd: nand: Update NAND manufacturer Ids

[Eugen Hristev]

On 11/12/24 15:57, Benedikt Spranger wrote:
> On Tue, 12 Nov 2024 15:39:23 +0200
> Eugen Hristev <eugen.hristev@linaro.org> wrote:
> 
> Hi Eugen,
>> On 10/18/24 11:30, Benedikt Spranger wrote:
>>> Align manufacturer Ids with the Id list from Linux kernel v6.11.
>>> While at it, sort the entries in alphabetical order.
>>>
>>> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
>>> Reviewed-by: John Ogness <john.ogness@linutronix.de>
>>> ---
>>>  drivers/mtd/nand/raw/nand_ids.c | 22 ++++++++++++----------
>>>  include/linux/mtd/rawnand.h     | 23 +++++++++++++----------
>>>  2 files changed, 25 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
>>> index 4f46378ffe1..e40facf774c 100644
>>> --- a/drivers/mtd/nand/raw/nand_ids.c
>>> +++ b/drivers/mtd/nand/raw/nand_ids.c
>>> @@ -180,20 +180,22 @@ struct nand_flash_dev nand_flash_ids[] = {
>>>  
>>>  /* Manufacturer IDs */
>>>  struct nand_manufacturer nand_manuf_ids[] = {
>>> -	{NAND_MFR_TOSHIBA, "Toshiba", &toshiba_nand_manuf_ops},
>>> -	{NAND_MFR_SAMSUNG, "Samsung", &samsung_nand_manuf_ops},
>>> +	{NAND_MFR_AMD, "AMD/Spansion", &amd_nand_manuf_ops},
>>> +	{NAND_MFR_ATO, "ATO"},
>>> +	{NAND_MFR_EON, "Eon"},
>>> +	{NAND_MFR_EON, "ESMT"},
>>
>> Did you mean NAND_MFR_ESMT here ?
> Yes, fatfingered that...
> 
> Regards
>     Benedikt Spranger



Unfortunately you will need a new revision anyway.

I get this while building sama5d4ek_nandflash_defconfig


arm-none-linux-gnueabihf-ld.bfd: drivers/mtd/nand/raw/nand_base.o: in
function `nand_reset_data_interface':
..drivers/mtd/nand/raw/nand_base.c:948:(.text.nand_reset+0xc): undefined
reference to `nand_get_default_data_interface'
arm-none-linux-gnueabihf-ld.bfd: drivers/mtd/nand/raw/nand_base.o: in
function `nand_flash_detect_onfi':
..drivers/mtd/nand/raw/nand_base.c:3884:(.text.nand_detect+0x318):
undefined reference to `nand_manuf_ids'
arm-none-linux-gnueabihf-ld.bfd:
drivers/mtd/nand/raw/nand_base.c:3884:(.text.nand_detect+0x31c):
undefined reference to `nand_flash_ids'
arm-none-linux-gnueabihf-ld.bfd: drivers/mtd/nand/raw/nand_base.o: in
function `nand_init_data_interface':
drivers/mtd/nand/raw/nand_base.c:1047:(.text.nand_scan_ident+0x1ec):
undefined reference to `onfi_init_data_interface'
make[1]: *** [scripts/Makefile.xpl:542: spl/u-boot-spl] Error 1

[PATCH 5/5] mtd: nand: raw: atmel_nand: Add missing nand_scan_ident()

[Benedikt Spranger]

In board_nand_init() the used NAND flash is not evaluated. This left
vital parts of internal structures uninitialized and SPL NAND flash
access is broken.

Add the missing nand_scan_ident() to board_nand_init().

Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/mtd/nand/raw/atmel_nand.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/raw/atmel_nand.c b/drivers/mtd/nand/raw/atmel_nand.c
index 4dbf7b47135..cf7ac54ea13 100644
--- a/drivers/mtd/nand/raw/atmel_nand.c
+++ b/drivers/mtd/nand/raw/atmel_nand.c
@@ -1423,6 +1423,10 @@ int board_nand_init(struct nand_chip *nand)
 	nand->bbt_options |= NAND_BBT_USE_FLASH;
 #endif
 
+	ret = nand_scan_ident(mtd, CONFIG_SYS_NAND_MAX_CHIPS, NULL);
+	if (ret)
+		return ret;
+
 #ifdef CONFIG_ATMEL_NAND_HWECC
 #ifdef CONFIG_ATMEL_NAND_HW_PMECC
 	ret = atmel_pmecc_nand_init_params(nand, mtd);
-- 
2.45.2

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Alexander Dahl]

Hello Benedikt,

Am Fri, Oct 18, 2024 at 10:30:00AM +0200 schrieb Benedikt Spranger:
> Hi,
> 
> while porting an ATSAMA5D2 based board (booting from NAND flash with UBI)
> I stumbled over generic, NAND- and UBI related asperity:
> 
> 1. tiny-printf does not handle NULL arguments to '%s' in a proper way
> 2. vtbl_check() has an useless debug output due to a typo
> 3. NAND: An informative output fails badly, if the NAND vendor is unknown
> 4. the nand_atmel driver lacks a vital initialisation
> 
> OK, you might say the nand_atmel NAND driver is obsolete, but it was the
> only solution to get booting from NAND running. The new DM based NAND driver
> refused to do anything usefull, so I dropped it after spending a couple of
> days debugging it:

Strange.  I have at least three different boards with SAMA5D27
successfully booting from NAND flash with the new DM based driver
(unfortunately none of them upstreamed, which also won't change in the
foreseeable future, sorry).

Do you use at91bootstrap as 2nd level bootloader like me or something
else?  For me it was basically getting the dts for U-Boot correct, but
I got that running with all U-Boot releases since 2023.04 up to
2024.10.  Could you share some more detail?

> <RANT>brilliant!</RANT>

Did you talk with Microchip about it?

Greets
Alex

> 
> 
> 
> regards
>     Benedikt Spranger
> 
> Benedikt Spranger (5):
>   tiny-printf: Handle NULL pointer argument to %s
>   drivers/mtd/ubispl/ubispl.c: Fix error message
>   mtd: nand: raw: Fix potential NULL pointer dereference
>   mtd: nand: Update NAND manufacturer Ids
>   mtd: nand: raw: atmel_nand: Add missing nand_scan_ident()
> 
>  drivers/mtd/nand/raw/atmel_nand.c |  4 ++++
>  drivers/mtd/nand/raw/nand_base.c  |  8 ++++----
>  drivers/mtd/nand/raw/nand_ids.c   | 22 ++++++++++++----------
>  drivers/mtd/ubispl/ubispl.c       |  2 +-
>  include/linux/mtd/rawnand.h       | 23 +++++++++++++----------
>  lib/tiny-printf.c                 |  2 +-
>  6 files changed, 35 insertions(+), 26 deletions(-)
> 
> -- 
> 2.45.2
> 

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Benedikt Spranger]

On Fri, 18 Oct 2024 15:11:20 +0200
Alexander Dahl <ada@thorsis.com> wrote:

Hello Alexander,

> Am Fri, Oct 18, 2024 at 10:30:00AM +0200 schrieb Benedikt Spranger:
> > OK, you might say the nand_atmel NAND driver is obsolete, but it
> > was the only solution to get booting from NAND running. The new DM
> > based NAND driver refused to do anything usefull, so I dropped it
> > after spending a couple of days debugging it:
> 
> Strange.  I have at least three different boards with SAMA5D27
> successfully booting from NAND flash with the new DM based driver
> (unfortunately none of them upstreamed, which also won't change in the
> foreseeable future, sorry).
OK.
 
> Do you use at91bootstrap as 2nd level bootloader like me or something
> else?
I use the U-Boot SPL. There is no UBI support in at91boostrap. There
were some attemps, but...
No interest at the at91bootstrap side.

> For me it was basically getting the dts for U-Boot correct, but
> I got that running with all U-Boot releases since 2023.04 up to
> 2024.10.  Could you share some more detail?
I face all the trouble in SPL. And since the SPL is the essential part
here (UBI support) I gave up at one point. I simply couldn't any
usefull read data from the NAND flash, but all 0.

Regards
    Benedikt Spranger

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Alexander Dahl]

Hello Benedikt,

Am Fri, Oct 18, 2024 at 04:19:08PM +0200 schrieb Benedikt Spranger:
> On Fri, 18 Oct 2024 15:11:20 +0200
> Alexander Dahl <ada@thorsis.com> wrote:
> 
> Hello Alexander,
> 
> > Am Fri, Oct 18, 2024 at 10:30:00AM +0200 schrieb Benedikt Spranger:
> > > OK, you might say the nand_atmel NAND driver is obsolete, but it
> > > was the only solution to get booting from NAND running. The new DM
> > > based NAND driver refused to do anything usefull, so I dropped it
> > > after spending a couple of days debugging it:
> > 
> > Strange.  I have at least three different boards with SAMA5D27
> > successfully booting from NAND flash with the new DM based driver
> > (unfortunately none of them upstreamed, which also won't change in the
> > foreseeable future, sorry).
> OK.
>  
> > Do you use at91bootstrap as 2nd level bootloader like me or something
> > else?
> I use the U-Boot SPL. There is no UBI support in at91boostrap. There
> were some attemps, but...
> No interest at the at91bootstrap side.

Yeah, been there.  Wanted at91bootstrap to boot from SPI-NOR which is
_not_ Quad-SPI, and the feature request was closed with "if you wanna
do it by yourself, ask our support for help". :-/

> > For me it was basically getting the dts for U-Boot correct, but
> > I got that running with all U-Boot releases since 2023.04 up to
> > 2024.10.  Could you share some more detail?
> I face all the trouble in SPL. And since the SPL is the essential part
> here (UBI support) I gave up at one point. I simply couldn't any
> usefull read data from the NAND flash, but all 0.

You can not store bootstrap or SPL in UBI, it must sit in first sector
of raw NAND, I suppose that's how you load SPL?  Did you try
reading/writing raw NAND from there?  I'm not familiar with SPL, does
it consider dts?  I could share my dts parts if that helps you.

(What I do here: SoC loads at91bootstrap from raw NAND at offset 0,
at91bootstrap loads U-Boot from raw NAND at some offset like 0x20000,
U-Boot (proper) loads everything else from UBI.)

Greets
Alex

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Eugen Hristev]

Hello,

On 10/21/24 09:03, Alexander Dahl wrote:
> Hello Benedikt,
> 
> Am Fri, Oct 18, 2024 at 04:19:08PM +0200 schrieb Benedikt Spranger:
>> On Fri, 18 Oct 2024 15:11:20 +0200
>> Alexander Dahl <ada@thorsis.com> wrote:
>>
>> Hello Alexander,
>>
>>> Am Fri, Oct 18, 2024 at 10:30:00AM +0200 schrieb Benedikt Spranger:
>>>> OK, you might say the nand_atmel NAND driver is obsolete, but it
>>>> was the only solution to get booting from NAND running. The new DM
>>>> based NAND driver refused to do anything usefull, so I dropped it
>>>> after spending a couple of days debugging it:
>>>
>>> Strange.  I have at least three different boards with SAMA5D27
>>> successfully booting from NAND flash with the new DM based driver
>>> (unfortunately none of them upstreamed, which also won't change in the
>>> foreseeable future, sorry).
>> OK.
>>   
>>> Do you use at91bootstrap as 2nd level bootloader like me or something
>>> else?
>> I use the U-Boot SPL. There is no UBI support in at91boostrap. There
>> were some attemps, but...
>> No interest at the at91bootstrap side.

It's not about interest. There are two main reasons:
First, all open source UBI stacks are GPL. At91bootstrap is not GPL and 
it cannot take GPL restricted code (being MIT ).
Second, all UBI stacks are huge in terms of code and memory. 
At91bootstrap is a second stage that should fit in 16k , 32k or 64k 
depending on the platform. So far this restriction could not be met by 
any UBI implementation.
... and yes, doing a custom in-house small UBI implementation would be 
challenging, and costly.

> 
> Yeah, been there.  Wanted at91bootstrap to boot from SPI-NOR which is
> _not_ Quad-SPI, and the feature request was closed with "if you wanna
> do it by yourself, ask our support for help". :-/

iirc at91bootstrap can boot from raw spi-nor (yes, not UBI)
> 
>>> For me it was basically getting the dts for U-Boot correct, but
>>> I got that running with all U-Boot releases since 2023.04 up to
>>> 2024.10.  Could you share some more detail?
>> I face all the trouble in SPL. And since the SPL is the essential part
>> here (UBI support) I gave up at one point. I simply couldn't any
>> usefull read data from the NAND flash, but all 0.
> 
> You can not store bootstrap or SPL in UBI, it must sit in first sector
> of raw NAND, I suppose that's how you load SPL?  Did you try
> reading/writing raw NAND from there?  I'm not familiar with SPL, does
> it consider dts?  I could share my dts parts if that helps you.

SPL should use dts. However on at91 platforms, SPL is dangerously close 
to the SRAM limit where it should fit, and it has been challenging in 
the last years to either make it fit or remove it.

> 
> (What I do here: SoC loads at91bootstrap from raw NAND at offset 0,
> at91bootstrap loads U-Boot from raw NAND at some offset like 0x20000,
> U-Boot (proper) loads everything else from UBI.)

Either way, it is interesting to see that the U-boot proper NAND dm 
driver does not work without at91bootstrap.
There may be NAND quirks in at91bootstrap board init that may influence 
this (pins resets, timing delays, etc).

You can test NAND driver by booting at91bootstrap and Uboot from another 
medium, like sd-card. In this way, at91bootstrap will not touch NAND 
device, and you have a clean U-boot booted.

Eugen

> 
> Greets
> Alex

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Alexander Dahl]

Hello Eugen,

Am Mon, Oct 21, 2024 at 10:17:54AM +0300 schrieb Eugen Hristev:
> Hello,
> 
> On 10/21/24 09:03, Alexander Dahl wrote:
> > Hello Benedikt,
> > 
> > Am Fri, Oct 18, 2024 at 04:19:08PM +0200 schrieb Benedikt Spranger:
> > > On Fri, 18 Oct 2024 15:11:20 +0200
> > > Alexander Dahl <ada@thorsis.com> wrote:
> > > 
> > > Hello Alexander,
> > > 
> > > > Am Fri, Oct 18, 2024 at 10:30:00AM +0200 schrieb Benedikt Spranger:
> > > > > OK, you might say the nand_atmel NAND driver is obsolete, but it
> > > > > was the only solution to get booting from NAND running. The new DM
> > > > > based NAND driver refused to do anything usefull, so I dropped it
> > > > > after spending a couple of days debugging it:
> > > > 
> > > > Strange.  I have at least three different boards with SAMA5D27
> > > > successfully booting from NAND flash with the new DM based driver
> > > > (unfortunately none of them upstreamed, which also won't change in the
> > > > foreseeable future, sorry).
> > > OK.
> > > > Do you use at91bootstrap as 2nd level bootloader like me or something
> > > > else?
> > > I use the U-Boot SPL. There is no UBI support in at91boostrap. There
> > > were some attemps, but...
> > > No interest at the at91bootstrap side.
> 
> It's not about interest. There are two main reasons:
> First, all open source UBI stacks are GPL. At91bootstrap is not GPL and it
> cannot take GPL restricted code (being MIT ).
> Second, all UBI stacks are huge in terms of code and memory. At91bootstrap
> is a second stage that should fit in 16k , 32k or 64k depending on the
> platform. So far this restriction could not be met by any UBI
> implementation.
> ... and yes, doing a custom in-house small UBI implementation would be
> challenging, and costly.

I suspected technical reasons, but nice to know it's also licensing.

(Side note: To increase robustness we considered storing bootloaders
(and maybe U-Boot env) in SPI-NOR flash with longer data retention,
and use UBI on raw NAND for everything else.)

> 
> > 
> > Yeah, been there.  Wanted at91bootstrap to boot from SPI-NOR which is
> > _not_ Quad-SPI, and the feature request was closed with "if you wanna
> > do it by yourself, ask our support for help". :-/
> 
> iirc at91bootstrap can boot from raw spi-nor (yes, not UBI)

Speaking of at91bootstrap 3.x and later: It can, but only if you use
the QSPI port, it does not work with flexcom for example.  See
https://github.com/linux4sam/at91bootstrap/issues/181 for reference.

> > 
> > > > For me it was basically getting the dts for U-Boot correct, but
> > > > I got that running with all U-Boot releases since 2023.04 up to
> > > > 2024.10.  Could you share some more detail?
> > > I face all the trouble in SPL. And since the SPL is the essential part
> > > here (UBI support) I gave up at one point. I simply couldn't any
> > > usefull read data from the NAND flash, but all 0.
> > 
> > You can not store bootstrap or SPL in UBI, it must sit in first sector
> > of raw NAND, I suppose that's how you load SPL?  Did you try
> > reading/writing raw NAND from there?  I'm not familiar with SPL, does
> > it consider dts?  I could share my dts parts if that helps you.
> 
> SPL should use dts. However on at91 platforms, SPL is dangerously close to
> the SRAM limit where it should fit, and it has been challenging in the last
> years to either make it fit or remove it.
> 
> > 
> > (What I do here: SoC loads at91bootstrap from raw NAND at offset 0,
> > at91bootstrap loads U-Boot from raw NAND at some offset like 0x20000,
> > U-Boot (proper) loads everything else from UBI.)
> 
> Either way, it is interesting to see that the U-boot proper NAND dm driver
> does not work without at91bootstrap.
> There may be NAND quirks in at91bootstrap board init that may influence this
> (pins resets, timing delays, etc).

Think this is a misunderstanding here.  I have this combination
running, and from the discussions on NAND issues on at91 on this list
I think at least two developers at Microchip, too.  From my
understanding U-Boot proper completely (re)initializes everything
required to get NAND to work.  I can not speak about U-Boot SPL
though, never tried that.

> You can test NAND driver by booting at91bootstrap and Uboot from another
> medium, like sd-card. In this way, at91bootstrap will not touch NAND device,
> and you have a clean U-boot booted.

Lets hear what Benedikt has tried. ;-)

Greets
Alex

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Eugen Hristev]

>>
>>>
>>> Yeah, been there.  Wanted at91bootstrap to boot from SPI-NOR which is
>>> _not_ Quad-SPI, and the feature request was closed with "if you wanna
>>> do it by yourself, ask our support for help". :-/
>>
>> iirc at91bootstrap can boot from raw spi-nor (yes, not UBI)
> 
> Speaking of at91bootstrap 3.x and later: It can, but only if you use
> the QSPI port, it does not work with flexcom for example.  See
> https://github.com/linux4sam/at91bootstrap/issues/181 for reference.
> 

Judging by the answer, we are both right, my memory was saying it works, 
and apparently for some older SoCs it's also implemented, while not for 
sam9x60 at the moment.
I remember using a flexcom in SPI mode was fairly simple and should work 
out of the box with the existing SPI driver in at91bootstrap, just that 
the flexcom mode has to be correctly configured, pinout, etc.

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Benedikt Spranger]

On Mon, 21 Oct 2024 10:17:54 +0300
Eugen Hristev <eugen.hristev@linaro.org> wrote:

Hi,
  
> >>> Do you use at91bootstrap as 2nd level bootloader like me or
> >>> something else?
> >> I use the U-Boot SPL. There is no UBI support in at91boostrap.
> >> There were some attemps, but...
> >> No interest at the at91bootstrap side.
> 
> It's not about interest. There are two main reasons:
> First, all open source UBI stacks are GPL. At91bootstrap is not GPL
> and it cannot take GPL restricted code (being MIT ).
I do not recall any issue mentioned about a license issue, when I send
out UBI patches for the AT91boostrap over a decade ago. It was a simple
"nice, I put it in a branch, but we do not need it."
I followed the UBI topic in AT91bootstrap loosely after that.

> Second, all UBI stacks are huge in terms of code and memory. 
OK. Maybe we have a different view on huge. The board I am supporting
now is the successor of a AT91SAM9263 board. This (old) board use
AT91boootstrap with UBI support. BTW: The original requirement for UBI
support was to fit into 4k (PPC 440 bootloader).

> At91bootstrap is a second stage that should fit in 16k , 32k or 64k 
> depending on the platform. So far this restriction could not be met
> by any UBI implementation.
OK. This need some more in depth view:
A SPL needs UBI read support only. This support can be done in less
then 4k, as I mentioned before. So UBI full read/write support?
You're right. It does not fit.

Needed read support?
It definitly fit (and works fine for over a decade).

> ... and yes, doing a custom in-house small UBI implementation would
> be challenging, and costly.
If you look at ubispl, you got some clue, what's needed. The UBI
support to the AT91boostrap (for the AT91SAM9263) is about ~450 LOC.
Managable, but why should I do it, when U-Boot SPL does the job...

> > (What I do here: SoC loads at91bootstrap from raw NAND at offset 0,
> > at91bootstrap loads U-Boot from raw NAND at some offset like
> > 0x20000, U-Boot (proper) loads everything else from UBI.)
> 
> Either way, it is interesting to see that the U-boot proper NAND dm 
> driver does not work without at91bootstrap.
To be clear: The U-Boot *SPL* does not work with the NAND dm driver.

The old NAND driver does. So from a developers view I can spend some
spare cycles on that, from a overall project view: we use the old
driver, since it is working.

Regards
    Benedikt Spranger

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Eugen Hristev]

On 10/21/24 15:30, Benedikt Spranger wrote:
> On Mon, 21 Oct 2024 10:17:54 +0300
> Eugen Hristev <eugen.hristev@linaro.org> wrote:
> 
> Hi,
>    
>>>>> Do you use at91bootstrap as 2nd level bootloader like me or
>>>>> something else?
>>>> I use the U-Boot SPL. There is no UBI support in at91boostrap.
>>>> There were some attemps, but...
>>>> No interest at the at91bootstrap side.
>>
>> It's not about interest. There are two main reasons:
>> First, all open source UBI stacks are GPL. At91bootstrap is not GPL
>> and it cannot take GPL restricted code (being MIT ).
> I do not recall any issue mentioned about a license issue, when I send
> out UBI patches for the AT91boostrap over a decade ago. It was a simple
> "nice, I put it in a branch, but we do not need it."
> I followed the UBI topic in AT91bootstrap loosely after that.

The patches that I saw ( I do not recall whether they were yours or not) 
while I was maintaining the project were GPL and about 4000 LOC.
It was adding more than 12k to the size.

> 
>> Second, all UBI stacks are huge in terms of code and memory.
> OK. Maybe we have a different view on huge. The board I am supporting
> now is the successor of a AT91SAM9263 board. This (old) board use
> AT91boootstrap with UBI support. BTW: The original requirement for UBI
> support was to fit into 4k (PPC 440 bootloader).
> 
>> At91bootstrap is a second stage that should fit in 16k , 32k or 64k
>> depending on the platform. So far this restriction could not be met
>> by any UBI implementation.
> OK. This need some more in depth view:
> A SPL needs UBI read support only. This support can be done in less
> then 4k, as I mentioned before. So UBI full read/write support?
> You're right. It does not fit.
> 
> Needed read support?
> It definitly fit (and works fine for over a decade).

I am glad you see you have an implementation that works. I am no longer 
maintaining that project so I cannot say anything about the possibility 
of ever merging it.

> 
>> ... and yes, doing a custom in-house small UBI implementation would
>> be challenging, and costly.
> If you look at ubispl, you got some clue, what's needed. The UBI
> support to the AT91boostrap (for the AT91SAM9263) is about ~450 LOC.
> Managable, but why should I do it, when U-Boot SPL does the job...

I can understand that. Nobody asked you to do it.

> 
>>> (What I do here: SoC loads at91bootstrap from raw NAND at offset 0,
>>> at91bootstrap loads U-Boot from raw NAND at some offset like
>>> 0x20000, U-Boot (proper) loads everything else from UBI.)
>>
>> Either way, it is interesting to see that the U-boot proper NAND dm
>> driver does not work without at91bootstrap.
> To be clear: The U-Boot *SPL* does not work with the NAND dm driver.
> 
> The old NAND driver does. So from a developers view I can spend some
> spare cycles on that, from a overall project view: we use the old
> driver, since it is working.

Ok, so my understanding was wrong, in fact you cannot use NAND dm in the 
SPL. It may be that there are some missing components from U-boot proper 
that are not built in (or some quirk/initialization)
So this can be looked into by the people interested, and I can 
understand that you have a solution that works for you.


> 
> Regards
>      Benedikt Spranger

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Benedikt Spranger]

Am Mon, 21 Oct 2024 08:03:35 +0200
schrieb Alexander Dahl <ada@thorsis.com>:

> Hello Benedikt,
> 
> Am Fri, Oct 18, 2024 at 04:19:08PM +0200 schrieb Benedikt Spranger:
> > On Fri, 18 Oct 2024 15:11:20 +0200
> > Alexander Dahl <ada@thorsis.com> wrote:
> > 
> > Hello Alexander,
> > 
> > > Am Fri, Oct 18, 2024 at 10:30:00AM +0200 schrieb Benedikt
> > > Spranger:
> > > > OK, you might say the nand_atmel NAND driver is obsolete, but it
> > > > was the only solution to get booting from NAND running. The new
> > > > DM based NAND driver refused to do anything usefull, so I
> > > > dropped it after spending a couple of days debugging it:
> > > 
> > > Strange.  I have at least three different boards with SAMA5D27
> > > successfully booting from NAND flash with the new DM based driver
> > > (unfortunately none of them upstreamed, which also won't change
> > > in the foreseeable future, sorry).
> > OK.
> >  
> > > Do you use at91bootstrap as 2nd level bootloader like me or
> > > something else?
> > I use the U-Boot SPL. There is no UBI support in at91boostrap. There
> > were some attemps, but...
> > No interest at the at91bootstrap side.
> 
> Yeah, been there.  Wanted at91bootstrap to boot from SPI-NOR which is
> _not_ Quad-SPI, and the feature request was closed with "if you wanna
> do it by yourself, ask our support for help". :-/
> 
> > > For me it was basically getting the dts for U-Boot correct, but
> > > I got that running with all U-Boot releases since 2023.04 up to
> > > 2024.10.  Could you share some more detail?
> > I face all the trouble in SPL. And since the SPL is the essential
> > part here (UBI support) I gave up at one point. I simply couldn't
> > any usefull read data from the NAND flash, but all 0.
> 
> You can not store bootstrap or SPL in UBI, it must sit in first sector
> of raw NAND, I suppose that's how you load SPL?
Sorry for not beeing that clear.
The SPL sits in the first Page in NAND flash. It does it's magic and
then load the U-Boot payload from an UBI volume (Or directly load a
kernel). Then it "starts" the payload.

> Did you try reading/writing raw NAND from there?
I try to read from SPL. That's all what ubispl wants. No need for write
support here.

> I'm not familiar with SPL, does it consider dts?
It consider dts.

> I could share my dts parts if that helps you.
That would be great. I definitly give it a try.
So here is a quick extract (EBI is also used otherwise) of what I did.
Hopefully I did not forget anything...

--- a/arch/arm/dts/at91-sama5d27_ABC.dts
+++ b/arch/arm/dts/at91-sama5d27_ABC.dts
@@ -63,7 +63,62 @@
                                        bias-disable;
                                        bootph-all;
                                };
+
+                               pinctrl_nand0_default: nand0_default {
+                                       pinmux =
+                                       <PIN_PA0__D0>,
+                                        <PIN_PA1__D1>,
+                                        <PIN_PA2__D2>,
+                                        <PIN_PA3__D3>,
+                                        <PIN_PA4__D4>,
+                                        <PIN_PA5__D5>,
+                                        <PIN_PA6__D6>,
+                                        <PIN_PA7__D7>,
+                                       <PIN_PA8__NWE_NANDWE>,
+                                        <PIN_PA9__NCS3>,
+                                        <PIN_PA10__A21_NANDALE>,
+                                        <PIN_PA11__A22_NANDCLE>,
+                                        <PIN_PA12__NRD_NANDOE>,
+                                        <PIN_PA21__NANDRDY>;
+                                       bias-disable;
+                               };
                        };
                };
        };
 };
+
+&ebi {
+       pinctrl-names = "default";
+       pinctrl-0 = <&pinctrl_nand0_default>;
+       status = "okay";
+
+       nand_controller: nand-controller {
+               status = "okay";
+
+               nand@3 {
+                       reg = <0x3 0x0 0x800000>;
+                       //atmel,rb = <0>;
+                       nand-bus-width = <8>;
+                       nand-ecc-mode = "hw";
+                       nand-ecc-strength = <4>;
+                       nand-ecc-step-size = <512>;
+                       nand-on-flash-bbt;
+
+                       partitions {
+                               compatible = "fixed-partitions";
+                               #address-cells = <1>;
+                               #size-cells = <1>;
+
+                               spl@0 {
+                                       label = "spl";
+                                       reg = <0x0 0x40000>;
+                               };
+
+                               ubi@40000 {
+                                       label = "UBI";
+                                       reg = <0x40000 0x0>;
+                               };
+                       };
+                };
+        };
+};
 
> (What I do here: SoC loads at91bootstrap from raw NAND at offset 0,
> at91bootstrap loads U-Boot from raw NAND at some offset like 0x20000,
> U-Boot (proper) loads everything else from UBI.)
From my experience that can cause significant headache starting after
5-6 years in use. That's what I try to avoid :)

Regards
    Benedikt Spranger

Re: [PATCH 0/5] collected fallout of porting an ATSAMA5D27 based board

[Alexander Dahl]

Hello Benedikt,

Am Mon, Oct 21, 2024 at 12:20:17PM +0200 schrieb Benedikt Spranger:
> The SPL sits in the first Page in NAND flash. It does it's magic and
> then load the U-Boot payload from an UBI volume (Or directly load a
> kernel). Then it "starts" the payload.
> 
> > Did you try reading/writing raw NAND from there?
> I try to read from SPL. That's all what ubispl wants. No need for write
> support here.
> 
> > I'm not familiar with SPL, does it consider dts?
> It consider dts.
> 
> > I could share my dts parts if that helps you.
> That would be great. I definitly give it a try.
> So here is a quick extract (EBI is also used otherwise) of what I did.

Using EBI for accessing an FPGA here on one of three boards.
So I'll share that dts excerpt from my v2024.10 branch.

> Hopefully I did not forget anything...
> 
> --- a/arch/arm/dts/at91-sama5d27_ABC.dts
> +++ b/arch/arm/dts/at91-sama5d27_ABC.dts
> @@ -63,7 +63,62 @@
>                                         bias-disable;
>                                         bootph-all;
>                                 };
> +
> +                               pinctrl_nand0_default: nand0_default {
> +                                       pinmux =
> +                                       <PIN_PA0__D0>,
> +                                        <PIN_PA1__D1>,
> +                                        <PIN_PA2__D2>,
> +                                        <PIN_PA3__D3>,
> +                                        <PIN_PA4__D4>,
> +                                        <PIN_PA5__D5>,
> +                                        <PIN_PA6__D6>,
> +                                        <PIN_PA7__D7>,
> +                                       <PIN_PA8__NWE_NANDWE>,
> +                                        <PIN_PA9__NCS3>,
> +                                        <PIN_PA10__A21_NANDALE>,
> +                                        <PIN_PA11__A22_NANDCLE>,
> +                                        <PIN_PA12__NRD_NANDOE>,
> +                                        <PIN_PA21__NANDRDY>;
> +                                       bias-disable;
> +                               };
>                         };
>                 };
>         };
>  };
> +
> +&ebi {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_nand0_default>;
> +       status = "okay";
> +
> +       nand_controller: nand-controller {
> +               status = "okay";
> +
> +               nand@3 {
> +                       reg = <0x3 0x0 0x800000>;
> +                       //atmel,rb = <0>;
> +                       nand-bus-width = <8>;
> +                       nand-ecc-mode = "hw";
> +                       nand-ecc-strength = <4>;
> +                       nand-ecc-step-size = <512>;
> +                       nand-on-flash-bbt;
> +
> +                       partitions {
> +                               compatible = "fixed-partitions";
> +                               #address-cells = <1>;
> +                               #size-cells = <1>;
> +
> +                               spl@0 {
> +                                       label = "spl";
> +                                       reg = <0x0 0x40000>;
> +                               };
> +
> +                               ubi@40000 {
> +                                       label = "UBI";
> +                                       reg = <0x40000 0x0>;
> +                               };
> +                       };
> +                };
> +        };
> +};

I have no partitions defined, but that should not hurt.  Split pinctrl
settings here, but that's basically because I have more child nodes on
the EBI node in Linux which I don't use in U-Boot here.  Should not
make such a difference.

Do you have external pull-ups for the A21_NANDALE, A22_NANDCLE,
NANDRDY, and NCS3 lines?  Maybe cross check that with the board code
you probably have for the old non-DM NAND driver.

Now my example:

  &ebi {
          pinctrl-names = "default";
          pinctrl-0 = <&pinctrl_ebi_default>;
          status = "okay";
  
          nand_controller: nand-controller {
                  pinctrl-names = "default";
                  pinctrl-0 = <&pinctrl_nand_default>;
                  status = "okay";
  
                  nand@3 {
                          reg = <0x3 0x0 0x800000>;
                          atmel,rb = <0>;
  
                          nand-bus-width = <8>;
                          nand-ecc-mode = "hw";
                          nand-on-flash-bbt;
                          nand-ecc-strength = <8>;
                          nand-ecc-step-size = <512>;
                          label = "atmel_nand";
                  };
          };
  };
  
  &pioA {
          pinctrl_ebi_default: ebi_default {
                  rd_we_data {
                          pinmux = <PIN_PA22__D0>,
                                   <PIN_PA23__D1>,
                                   <PIN_PA24__D2>,
                                   <PIN_PA25__D3>,
                                   <PIN_PA26__D4>,
                                   <PIN_PA27__D5>,
                                   <PIN_PA28__D6>,
                                   <PIN_PA29__D7>,
                                   <PIN_PA30__NWE_NANDWE>,
                                   <PIN_PB2__NRD_NANDOE>;
                          bias-disable;
                          atmel,drive-strength = <ATMEL_PIO_DRVSTR_ME>;
                  };
          };
  
          pinctrl_nand_default: nand_default {
                  ale_cle_rdy_cs {
                          pinmux = <PIN_PB0__A21_NANDALE>,
                                   <PIN_PB1__A22_NANDCLE>,
                                   <PIN_PC8__NANDRDY>,
                                   <PIN_PA31__NCS3>;
                          bias-pull-up;
                  };
          };
  };

Notice mine uses IO Set 1 here, but your's should work with IO Set 2 as
well.

> > (What I do here: SoC loads at91bootstrap from raw NAND at offset 0,
> > at91bootstrap loads U-Boot from raw NAND at some offset like 0x20000,
> > U-Boot (proper) loads everything else from UBI.)
> From my experience that can cause significant headache starting after
> 5-6 years in use. That's what I try to avoid :)

I'm aware of that.  The data retention time for the NAND flash used
here is 10 years according to datasheet.  No big deal on Linux when
running ubihealthd for everything stored in UBI, but everything
outside of that is about to fail some day. :-/

Greets
Alex