[PATCH 0/2] mtd: nand: raw: atmel: R/B gpio on sam9x60
Hello everyone,
this is a patch series wtih some real fixes _and_ a question or some kind of support request in the cover letter. I would be happy if anyone could read the cover letter carefully and answer to that one what might be the problem I see. O:-)
I'm currently working on the sam9x60-curiosity board and especially trying to get it booting from onboard raw NAND flash. As reported in my last series I got the flash recognized already. However interacting with it was terribly slow, because nand_wait_ready() calling atmel_nand_dev_ready() ran into a 400ms timeout in several occasions, especially when reading from the device. Reading a single block triggered that timeout two times per page, which summed up to over 50 seconds for 64 × 4096 = 256k Bytes!
(You can have U-Boot print that warning from nand_wait_ready() by increasing the console log level to at least "warn".)
Note: the dts from atmel/next seems correct to me, I double checked that again. My own debug log messages showed the GPIO is accessed, and if you add enough debug messages sometimes the timeout is not reached, so I'm sure the NAND chip eventually switches that R/B line and the code correctly sees that, that line level change however takes ages, something between 400ms and 500ms most of the times.
I vaguely remembered on SAMA5D2 the old atmel raw nand driver is used which does not support reading the R/B signal, but nevertheless works. I'm not familiar in detail with those raw NAND flash chips, but as far as I can understand, there are other ways to determine if the chip is ready or busy. So after I removed that rb-gpio parameter from 'at91-sam9x60_curiosity.dts' I ran into the bug fixed by patch 2.
With that patch applied _and_ rb-gpio still removed from dts, raw NAND access is reasonably fast on sam9x60-curiosity board. (You might want to rebase to atmel/next for testing this.) Not sure if I should send a patch for that dts change, because I suppose it's a workaround only and not addressing the actual cause?
I think the fix is correct in itself, I tested different combinations and compared with the driver in Linux, however …
Can anyone explain to me, why flash access is sooo slow if the R/B gpio is used? Especially in comparision to Linux, where I don't need to remove that thing from dts and it works reasonably fast?
The actual flash chip is a Macronix MX30LF4G28AD, 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 256.
Greets Alex
P.S.: although not returned by get_maintainer.pl I added Eugen to Cc because he is maintainer of the at91 and might have some insight if it is a general problem of the nand controller in at91 socs?
Alexander Dahl (2): mtd: nand: raw: atmel: Remove duplicate line mtd: nand: raw: atmel: Add error handling when rb-gpios missing
drivers/mtd/nand/raw/atmel/nand-controller.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
base-commit: a169438411f9277cc689c14078151aa1d1caae3c
Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/mtd/nand/raw/atmel/nand-controller.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c index 9873d11254..2b29c8def6 100644 --- a/drivers/mtd/nand/raw/atmel/nand-controller.c +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c @@ -1474,7 +1474,6 @@ static void atmel_nand_init(struct atmel_nand_controller *nc,
mtd->dev->parent = nc->dev; nand->controller = &nc->base; - nand->controller = &nc->base;
chip->cmd_ctrl = atmel_nand_cmd_ctrl; chip->read_byte = atmel_nand_read_byte;
Hi
On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl ada@thorsis.com wrote:
Signed-off-by: Alexander Dahl ada@thorsis.com
drivers/mtd/nand/raw/atmel/nand-controller.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c index 9873d11254..2b29c8def6 100644 --- a/drivers/mtd/nand/raw/atmel/nand-controller.c +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c @@ -1474,7 +1474,6 @@ static void atmel_nand_init(struct atmel_nand_controller *nc,
mtd->dev->parent = nc->dev; nand->controller = &nc->base;
nand->controller = &nc->base; chip->cmd_ctrl = atmel_nand_cmd_ctrl; chip->read_byte = atmel_nand_read_byte;-- 2.30.2
Reviewed-by: Michael Trimarchi michael@amarulasolutions.com
On 8/8/23 16:48, Michael Nazzareno Trimarchi wrote:
Hi
On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl ada@thorsis.com wrote:
Signed-off-by: Alexander Dahl ada@thorsis.com
Reviewed-by: Michael Trimarchi michael@amarulasolutions.com
Applied this one patch to u-boot-at91/next , thanks !
Adapt behaviour to Linux kernel driver.
The return value of gpio_request_by_name_nodev() was not checked before, and thus in case 'rb-gpios' was missing in DT, rb.type was set to ATMEL_NAND_GPIO_RB nevertheless, leading to output like this for example (on sam9x60-curiosity with the line removed from dts):
NAND: Could not find valid ONFI parameter page; aborting device found, Manufacturer ID: 0xc2, Chip ID: 0xdc Macronix NAND 512MiB 3,3V 8-bit 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 64 atmel-nand-controller nand-controller: NAND scan failed: -22 Failed to probe nand driver (err = -22) Failed to initialize NAND controller. (error -22) 0 MiB
Note: not having that gpio assigned in dts is fine, the driver does not override nand_chip->dev_ready() then and a generic solution is used.
Fixes: 6a8dfd57220d ("nand: atmel: Add DM based NAND driver") Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/mtd/nand/raw/atmel/nand-controller.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c index 2b29c8def6..8e745a5111 100644 --- a/drivers/mtd/nand/raw/atmel/nand-controller.c +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c @@ -1600,10 +1600,13 @@ static struct atmel_nand *atmel_nand_create(struct atmel_nand_controller *nc, nand->cs[i].rb.type = ATMEL_NAND_NATIVE_RB; nand->cs[i].rb.id = val; } else { - gpio_request_by_name_nodev(np, "rb-gpios", 0, - &nand->cs[i].rb.gpio, - GPIOD_IS_IN); - nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB; + ret = gpio_request_by_name_nodev(np, "rb-gpios", 0, + &nand->cs[i].rb.gpio, + GPIOD_IS_IN); + if (ret) + dev_err(nc->dev, "Failed to get R/B gpio (err = %d)\n", ret); + else + nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB; }
gpio_request_by_name_nodev(np, "cs-gpios", 0,
Hi
On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl ada@thorsis.com wrote:
Adapt behaviour to Linux kernel driver.
The return value of gpio_request_by_name_nodev() was not checked before, and thus in case 'rb-gpios' was missing in DT, rb.type was set to ATMEL_NAND_GPIO_RB nevertheless, leading to output like this for example (on sam9x60-curiosity with the line removed from dts):
NAND: Could not find valid ONFI parameter page; aborting device found, Manufacturer ID: 0xc2, Chip ID: 0xdc Macronix NAND 512MiB 3,3V 8-bit 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 64 atmel-nand-controller nand-controller: NAND scan failed: -22 Failed to probe nand driver (err = -22) Failed to initialize NAND controller. (error -22) 0 MiBNote: not having that gpio assigned in dts is fine, the driver does not override nand_chip->dev_ready() then and a generic solution is used.
Fixes: 6a8dfd57220d ("nand: atmel: Add DM based NAND driver") Signed-off-by: Alexander Dahl ada@thorsis.com
drivers/mtd/nand/raw/atmel/nand-controller.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c index 2b29c8def6..8e745a5111 100644 --- a/drivers/mtd/nand/raw/atmel/nand-controller.c +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c @@ -1600,10 +1600,13 @@ static struct atmel_nand *atmel_nand_create(struct atmel_nand_controller *nc, nand->cs[i].rb.type = ATMEL_NAND_NATIVE_RB; nand->cs[i].rb.id = val; } else {
gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB;
ret = gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);if (ret)dev_err(nc->dev, "Failed to get R/B gpio (err = %d)\n", ret);
Should not then an error here
Michael
elsenand->cs[i].rb.type = ATMEL_NAND_GPIO_RB; } gpio_request_by_name_nodev(np, "cs-gpios", 0,-- 2.30.2
Hello Michael,
Am Tue, Aug 08, 2023 at 03:49:45PM +0200 schrieb Michael Nazzareno Trimarchi:
Hi
On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl ada@thorsis.com wrote:
Adapt behaviour to Linux kernel driver.
The return value of gpio_request_by_name_nodev() was not checked before, and thus in case 'rb-gpios' was missing in DT, rb.type was set to ATMEL_NAND_GPIO_RB nevertheless, leading to output like this for example (on sam9x60-curiosity with the line removed from dts):
NAND: Could not find valid ONFI parameter page; aborting device found, Manufacturer ID: 0xc2, Chip ID: 0xdc Macronix NAND 512MiB 3,3V 8-bit 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 64 atmel-nand-controller nand-controller: NAND scan failed: -22 Failed to probe nand driver (err = -22) Failed to initialize NAND controller. (error -22) 0 MiBNote: not having that gpio assigned in dts is fine, the driver does not override nand_chip->dev_ready() then and a generic solution is used.
Fixes: 6a8dfd57220d ("nand: atmel: Add DM based NAND driver") Signed-off-by: Alexander Dahl ada@thorsis.com
drivers/mtd/nand/raw/atmel/nand-controller.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c index 2b29c8def6..8e745a5111 100644 --- a/drivers/mtd/nand/raw/atmel/nand-controller.c +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c @@ -1600,10 +1600,13 @@ static struct atmel_nand *atmel_nand_create(struct atmel_nand_controller *nc, nand->cs[i].rb.type = ATMEL_NAND_NATIVE_RB; nand->cs[i].rb.id = val; } else {
gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB;
ret = gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);if (ret)dev_err(nc->dev, "Failed to get R/B gpio (err = %d)\n", ret);Should not then an error here
Different log level or no message at all?
Note: Linux prints the same message with error level in that case.
Greets Alex
Michael
elsenand->cs[i].rb.type = ATMEL_NAND_GPIO_RB; } gpio_request_by_name_nodev(np, "cs-gpios", 0,-- 2.30.2
-- 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
Hi,
On 8/8/23 18:03, Alexander Dahl wrote:
Hello Michael,
Am Tue, Aug 08, 2023 at 03:49:45PM +0200 schrieb Michael Nazzareno Trimarchi:
Hi
On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl ada@thorsis.com wrote:
Adapt behaviour to Linux kernel driver.
The return value of gpio_request_by_name_nodev() was not checked before, and thus in case 'rb-gpios' was missing in DT, rb.type was set to ATMEL_NAND_GPIO_RB nevertheless, leading to output like this for example (on sam9x60-curiosity with the line removed from dts):
NAND: Could not find valid ONFI parameter page; aborting device found, Manufacturer ID: 0xc2, Chip ID: 0xdc Macronix NAND 512MiB 3,3V 8-bit 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 64 atmel-nand-controller nand-controller: NAND scan failed: -22 Failed to probe nand driver (err = -22) Failed to initialize NAND controller. (error -22) 0 MiBNote: not having that gpio assigned in dts is fine, the driver does not override nand_chip->dev_ready() then and a generic solution is used.
Fixes: 6a8dfd57220d ("nand: atmel: Add DM based NAND driver") Signed-off-by: Alexander Dahl ada@thorsis.com
drivers/mtd/nand/raw/atmel/nand-controller.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c index 2b29c8def6..8e745a5111 100644 --- a/drivers/mtd/nand/raw/atmel/nand-controller.c +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c @@ -1600,10 +1600,13 @@ static struct atmel_nand *atmel_nand_create(struct atmel_nand_controller *nc, nand->cs[i].rb.type = ATMEL_NAND_NATIVE_RB; nand->cs[i].rb.id = val; } else {
gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB;
ret = gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);if (ret)dev_err(nc->dev, "Failed to get R/B gpio (err = %d)\n", ret);Should not then an error here
Different log level or no message at all?
Note: Linux prints the same message with error level in that case.
Greets Alex
Since the rb-gpios is optional, we can continue probing without it. Throwing an error message is optional and pure informative. So I am fine with it
What I wanted to ask is what happens with nand->cs[i].rb.type , is it 0 by default ?
Other than that, I can apply this patch, Michael, do you have any more comments on it ?
Thanks, Eugen
Michael
elsenand->cs[i].rb.type = ATMEL_NAND_GPIO_RB; } gpio_request_by_name_nodev(np, "cs-gpios", 0,-- 2.30.2
-- 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
Hi
On Wed, Aug 23, 2023 at 8:28 AM Eugen Hristev eugen.hristev@collabora.com wrote:
Hi,
On 8/8/23 18:03, Alexander Dahl wrote:
Hello Michael,
Am Tue, Aug 08, 2023 at 03:49:45PM +0200 schrieb Michael Nazzareno Trimarchi:
Hi
On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl ada@thorsis.com wrote:
Adapt behaviour to Linux kernel driver.
The return value of gpio_request_by_name_nodev() was not checked before, and thus in case 'rb-gpios' was missing in DT, rb.type was set to ATMEL_NAND_GPIO_RB nevertheless, leading to output like this for example (on sam9x60-curiosity with the line removed from dts):
NAND: Could not find valid ONFI parameter page; aborting device found, Manufacturer ID: 0xc2, Chip ID: 0xdc Macronix NAND 512MiB 3,3V 8-bit 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 64 atmel-nand-controller nand-controller: NAND scan failed: -22 Failed to probe nand driver (err = -22) Failed to initialize NAND controller. (error -22) 0 MiBNote: not having that gpio assigned in dts is fine, the driver does not override nand_chip->dev_ready() then and a generic solution is used.
Fixes: 6a8dfd57220d ("nand: atmel: Add DM based NAND driver") Signed-off-by: Alexander Dahl ada@thorsis.com
drivers/mtd/nand/raw/atmel/nand-controller.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c index 2b29c8def6..8e745a5111 100644 --- a/drivers/mtd/nand/raw/atmel/nand-controller.c +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c @@ -1600,10 +1600,13 @@ static struct atmel_nand *atmel_nand_create(struct atmel_nand_controller *nc, nand->cs[i].rb.type = ATMEL_NAND_NATIVE_RB; nand->cs[i].rb.id = val; } else {
gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB;
ret = gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);if (ret)dev_err(nc->dev, "Failed to get R/B gpio (err = %d)\n", ret);Should not then an error here
Different log level or no message at all?
Note: Linux prints the same message with error level in that case.
Greets Alex
Since the rb-gpios is optional, we can continue probing without it. Throwing an error message is optional and pure informative. So I am fine with it
Yes ok, but I'm not sure linux give an error if the gpio is get as optional and condition is IS_ERR. Am I right?
For the rest is fine
Michael
What I wanted to ask is what happens with nand->cs[i].rb.type , is it 0 by default ?
Other than that, I can apply this patch, Michael, do you have any more comments on it ?
Thanks, Eugen
Michael
elsenand->cs[i].rb.type = ATMEL_NAND_GPIO_RB; } gpio_request_by_name_nodev(np, "cs-gpios", 0,-- 2.30.2
-- 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
On 8/23/23 09:54, Michael Nazzareno Trimarchi wrote:
Hi
On Wed, Aug 23, 2023 at 8:28 AM Eugen Hristev eugen.hristev@collabora.com wrote:
Hi,
On 8/8/23 18:03, Alexander Dahl wrote:
Hello Michael,
Am Tue, Aug 08, 2023 at 03:49:45PM +0200 schrieb Michael Nazzareno Trimarchi:
Hi
On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl ada@thorsis.com wrote:
Adapt behaviour to Linux kernel driver.
The return value of gpio_request_by_name_nodev() was not checked before, and thus in case 'rb-gpios' was missing in DT, rb.type was set to ATMEL_NAND_GPIO_RB nevertheless, leading to output like this for example (on sam9x60-curiosity with the line removed from dts):
NAND: Could not find valid ONFI parameter page; aborting device found, Manufacturer ID: 0xc2, Chip ID: 0xdc Macronix NAND 512MiB 3,3V 8-bit 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 64 atmel-nand-controller nand-controller: NAND scan failed: -22 Failed to probe nand driver (err = -22) Failed to initialize NAND controller. (error -22) 0 MiBNote: not having that gpio assigned in dts is fine, the driver does not override nand_chip->dev_ready() then and a generic solution is used.
Fixes: 6a8dfd57220d ("nand: atmel: Add DM based NAND driver") Signed-off-by: Alexander Dahl ada@thorsis.com
drivers/mtd/nand/raw/atmel/nand-controller.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c index 2b29c8def6..8e745a5111 100644 --- a/drivers/mtd/nand/raw/atmel/nand-controller.c +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c @@ -1600,10 +1600,13 @@ static struct atmel_nand *atmel_nand_create(struct atmel_nand_controller *nc, nand->cs[i].rb.type = ATMEL_NAND_NATIVE_RB; nand->cs[i].rb.id = val; } else {
gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB;
ret = gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);if (ret)dev_err(nc->dev, "Failed to get R/B gpio (err = %d)\n", ret);Should not then an error here
Different log level or no message at all?
Note: Linux prints the same message with error level in that case.
Greets Alex
Since the rb-gpios is optional, we can continue probing without it. Throwing an error message is optional and pure informative. So I am fine with it
Yes ok, but I'm not sure linux give an error if the gpio is get as optional and condition is IS_ERR. Am I right?
if (IS_ERR(gpio) && PTR_ERR(gpio) != -ENOENT) { dev_err(nc->dev, "Failed to get R/B gpio (err = %ld)\n", PTR_ERR(gpio)); return ERR_CAST(gpio); }
So Linux throws the message if IS_ERR . If the property is missing (ENOENT) it moves on.
Can we replicate the same behavior or this behavior does not suit us in U-boot ?
Basically I think it should be : if (ret && ret != -ENOENT) dev_err(...) if (!ret) rb.type = ATMEL_NAND_GPIO_RB;
Is this what you had in mind Michael ?
Eugen
For the rest is fine
Michael
What I wanted to ask is what happens with nand->cs[i].rb.type , is it 0 by default ?
Other than that, I can apply this patch, Michael, do you have any more comments on it ?
Thanks, Eugen
Michael
elsenand->cs[i].rb.type = ATMEL_NAND_GPIO_RB; } gpio_request_by_name_nodev(np, "cs-gpios", 0,-- 2.30.2
-- 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
Hello Eugen, hello Michael,
Am Wed, Aug 23, 2023 at 01:59:58PM +0300 schrieb Eugen Hristev:
On 8/23/23 09:54, Michael Nazzareno Trimarchi wrote:
Hi
On Wed, Aug 23, 2023 at 8:28 AM Eugen Hristev eugen.hristev@collabora.com wrote:
Hi,
On 8/8/23 18:03, Alexander Dahl wrote:
Hello Michael,
Am Tue, Aug 08, 2023 at 03:49:45PM +0200 schrieb Michael Nazzareno Trimarchi:
Hi
On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl ada@thorsis.com wrote:
Adapt behaviour to Linux kernel driver.
The return value of gpio_request_by_name_nodev() was not checked before, and thus in case 'rb-gpios' was missing in DT, rb.type was set to ATMEL_NAND_GPIO_RB nevertheless, leading to output like this for example (on sam9x60-curiosity with the line removed from dts):
NAND: Could not find valid ONFI parameter page; aborting device found, Manufacturer ID: 0xc2, Chip ID: 0xdc Macronix NAND 512MiB 3,3V 8-bit 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 64 atmel-nand-controller nand-controller: NAND scan failed: -22 Failed to probe nand driver (err = -22) Failed to initialize NAND controller. (error -22) 0 MiBNote: not having that gpio assigned in dts is fine, the driver does not override nand_chip->dev_ready() then and a generic solution is used.
Fixes: 6a8dfd57220d ("nand: atmel: Add DM based NAND driver") Signed-off-by: Alexander Dahl ada@thorsis.com
drivers/mtd/nand/raw/atmel/nand-controller.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c index 2b29c8def6..8e745a5111 100644 --- a/drivers/mtd/nand/raw/atmel/nand-controller.c +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c @@ -1600,10 +1600,13 @@ static struct atmel_nand *atmel_nand_create(struct atmel_nand_controller *nc, nand->cs[i].rb.type = ATMEL_NAND_NATIVE_RB; nand->cs[i].rb.id = val; } else {
gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);nand->cs[i].rb.type = ATMEL_NAND_GPIO_RB;
ret = gpio_request_by_name_nodev(np, "rb-gpios", 0,&nand->cs[i].rb.gpio,GPIOD_IS_IN);if (ret)dev_err(nc->dev, "Failed to get R/B gpio (err = %d)\n", ret);Should not then an error here
Different log level or no message at all?
Note: Linux prints the same message with error level in that case.
Greets Alex
Since the rb-gpios is optional, we can continue probing without it. Throwing an error message is optional and pure informative. So I am fine with it
Yes ok, but I'm not sure linux give an error if the gpio is get as optional and condition is IS_ERR. Am I right?
if (IS_ERR(gpio) && PTR_ERR(gpio) != -ENOENT) { dev_err(nc->dev, "Failed to get R/B gpio (err = %ld)\n", PTR_ERR(gpio)); return ERR_CAST(gpio); }So Linux throws the message if IS_ERR . If the property is missing (ENOENT) it moves on.
Can we replicate the same behavior or this behavior does not suit us in U-boot ?
Basically I think it should be : if (ret && ret != -ENOENT) dev_err(...) if (!ret) rb.type = ATMEL_NAND_GPIO_RB;
Is this what you had in mind Michael ?
The discussion between you guys somehow got stuck. I still have this patch on my TODO list, but I'm not sure how to proceed. Please advice in which direction to go forward, there's still an error condition (described in the initial commit message) here, which needs to be handled, otherwise the NAND flash might not be usable.
Greets Alex
Eugen
For the rest is fine
Michael
What I wanted to ask is what happens with nand->cs[i].rb.type , is it 0 by default ?
Other than that, I can apply this patch, Michael, do you have any more comments on it ?
Thanks, Eugen
Michael
elsenand->cs[i].rb.type = ATMEL_NAND_GPIO_RB; } gpio_request_by_name_nodev(np, "cs-gpios", 0,-- 2.30.2
-- 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
Hi
Il mer 20 set 2023, 08:13 Alexander Dahl ada@thorsis.com ha scritto:
Hello Eugen, hello Michael,
Am Wed, Aug 23, 2023 at 01:59:58PM +0300 schrieb Eugen Hristev:
On 8/23/23 09:54, Michael Nazzareno Trimarchi wrote:
Hi
On Wed, Aug 23, 2023 at 8:28 AM Eugen Hristev eugen.hristev@collabora.com wrote:
Hi,
On 8/8/23 18:03, Alexander Dahl wrote:
Hello Michael,
Am Tue, Aug 08, 2023 at 03:49:45PM +0200 schrieb Michael Nazzareno
Trimarchi:
Hi
On Tue, Aug 8, 2023 at 3:03 PM Alexander Dahl ada@thorsis.com
wrote:
> > Adapt behaviour to Linux kernel driver. > > The return value of gpio_request_by_name_nodev() was not
checked before,
> and thus in case 'rb-gpios' was missing in DT, rb.type was set
to
> ATMEL_NAND_GPIO_RB nevertheless, leading to output like this
for
> example (on sam9x60-curiosity with the line removed from dts): > > NAND: Could not find valid ONFI parameter page; aborting > device found, Manufacturer ID: 0xc2, Chip ID: 0xdc > Macronix NAND 512MiB 3,3V 8-bit > 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB
size: 64
> atmel-nand-controller nand-controller: NAND scan failed:
-22
> Failed to probe nand driver (err = -22) > Failed to initialize NAND controller. (error -22) > 0 MiB > > Note: not having that gpio assigned in dts is fine, the driver
does not
> override nand_chip->dev_ready() then and a generic solution is
used.
> > Fixes: 6a8dfd57220d ("nand: atmel: Add DM based NAND driver") > Signed-off-by: Alexander Dahl ada@thorsis.com > --- > drivers/mtd/nand/raw/atmel/nand-controller.c | 11
+++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c
b/drivers/mtd/nand/raw/atmel/nand-controller.c
> index 2b29c8def6..8e745a5111 100644 > --- a/drivers/mtd/nand/raw/atmel/nand-controller.c > +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c > @@ -1600,10 +1600,13 @@ static struct atmel_nand
*atmel_nand_create(struct atmel_nand_controller *nc,
> nand->cs[i].rb.type =
ATMEL_NAND_NATIVE_RB;
> nand->cs[i].rb.id = val; > } else { > - gpio_request_by_name_nodev(np,
"rb-gpios", 0,
> -
&nand->cs[i].rb.gpio,
> -
GPIOD_IS_IN);
> - nand->cs[i].rb.type =
ATMEL_NAND_GPIO_RB;
> + ret = gpio_request_by_name_nodev(np,
"rb-gpios", 0,
> +
&nand->cs[i].rb.gpio,
> +
GPIOD_IS_IN);
> + if (ret) > + dev_err(nc->dev, "Failed to
get R/B gpio (err = %d)\n", ret);
Should not then an error here
Different log level or no message at all?
Note: Linux prints the same message with error level in that case.
Greets Alex
Since the rb-gpios is optional, we can continue probing without it. Throwing an error message is optional and pure informative. So I am
fine
with it
Yes ok, but I'm not sure linux give an error if the gpio is get as optional and condition is IS_ERR. Am I right?
if (IS_ERR(gpio) && PTR_ERR(gpio) != -ENOENT) { dev_err(nc->dev, "Failed to get R/B gpio (err =%ld)\n",
PTR_ERR(gpio)); return ERR_CAST(gpio); }So Linux throws the message if IS_ERR . If the property is missing
(ENOENT)
it moves on.
Can we replicate the same behavior or this behavior does not suit us in U-boot ?
Basically I think it should be :
if (ret && ret != -ENOENT) dev_err(...) if (!ret) rb.type = ATMEL_NAND_GPIO_RB;Is this what you had in mind Michael ?
The discussion between you guys somehow got stuck. I still have this patch on my TODO list, but I'm not sure how to proceed. Please advice in which direction to go forward, there's still an error condition (described in the initial commit message) here, which needs to be handled, otherwise the NAND flash might not be usable.
Yes the suggestion above is what is the right path to follow.
Michael
Greets Alex
Eugen
For the rest is fine
Michael
What I wanted to ask is what happens with nand->cs[i].rb.type , is
it 0
by default ?
Other than that, I can apply this patch, Michael, do you have any
more
comments on it ?
Thanks, Eugen
Michael
> + else > + nand->cs[i].rb.type =
ATMEL_NAND_GPIO_RB;
> } > > gpio_request_by_name_nodev(np, "cs-gpios", 0, > -- > 2.30.2 >
-- 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
Added Mihai who tested this a lot at some point in time
Eugen
On 8/8/23 16:02, Alexander Dahl wrote:
Hello everyone,
this is a patch series wtih some real fixes _and_ a question or some kind of support request in the cover letter. I would be happy if anyone could read the cover letter carefully and answer to that one what might be the problem I see. O:-)
I'm currently working on the sam9x60-curiosity board and especially trying to get it booting from onboard raw NAND flash. As reported in my last series I got the flash recognized already. However interacting with it was terribly slow, because nand_wait_ready() calling atmel_nand_dev_ready() ran into a 400ms timeout in several occasions, especially when reading from the device. Reading a single block triggered that timeout two times per page, which summed up to over 50 seconds for 64 × 4096 = 256k Bytes!
(You can have U-Boot print that warning from nand_wait_ready() by increasing the console log level to at least "warn".)
Note: the dts from atmel/next seems correct to me, I double checked that again. My own debug log messages showed the GPIO is accessed, and if you add enough debug messages sometimes the timeout is not reached, so I'm sure the NAND chip eventually switches that R/B line and the code correctly sees that, that line level change however takes ages, something between 400ms and 500ms most of the times.
I vaguely remembered on SAMA5D2 the old atmel raw nand driver is used which does not support reading the R/B signal, but nevertheless works. I'm not familiar in detail with those raw NAND flash chips, but as far as I can understand, there are other ways to determine if the chip is ready or busy. So after I removed that rb-gpio parameter from 'at91-sam9x60_curiosity.dts' I ran into the bug fixed by patch 2.
With that patch applied _and_ rb-gpio still removed from dts, raw NAND access is reasonably fast on sam9x60-curiosity board. (You might want to rebase to atmel/next for testing this.) Not sure if I should send a patch for that dts change, because I suppose it's a workaround only and not addressing the actual cause?
I think the fix is correct in itself, I tested different combinations and compared with the driver in Linux, however …
Can anyone explain to me, why flash access is sooo slow if the R/B gpio is used? Especially in comparision to Linux, where I don't need to remove that thing from dts and it works reasonably fast?
The actual flash chip is a Macronix MX30LF4G28AD, 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 256.
Greets Alex
P.S.: although not returned by get_maintainer.pl I added Eugen to Cc because he is maintainer of the at91 and might have some insight if it is a general problem of the nand controller in at91 socs?
Alexander Dahl (2): mtd: nand: raw: atmel: Remove duplicate line mtd: nand: raw: atmel: Add error handling when rb-gpios missing
drivers/mtd/nand/raw/atmel/nand-controller.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
base-commit: a169438411f9277cc689c14078151aa1d1caae3c
Hi Alex,
Please find bellow my answer:
------------------------------
Added Mihai who tested this a lot at some point in time
Eugen
On 8/8/23 16:02, Alexander Dahl wrote:
Hello everyone,
this is a patch series wtih some real fixes _and_ a question or some kind of support request in the cover letter. I would be happy if anyone could read the cover letter carefully and answer to that one what might be the problem I see. O:-)
I'm currently working on the sam9x60-curiosity board and especially trying to get it booting from onboard raw NAND flash. As reported in my last series I got the flash recognized already. However interacting with it was terribly slow, because nand_wait_ready() calling atmel_nand_dev_ready() ran into a 400ms timeout in several occasions, especially when reading from the device. Reading a single block triggered that timeout two times per page, which summed up to over 50 seconds for 64 × 4096 = 256k Bytes!
(You can have U-Boot print that warning from nand_wait_ready() by increasing the console log level to at least "warn".)
Note: the dts from atmel/next seems correct to me, I double checked that again. My own debug log messages showed the GPIO is accessed, and if you add enough debug messages sometimes the timeout is not reached, so I'm sure the NAND chip eventually switches that R/B line and the code correctly sees that, that line level change however takes ages, something between 400ms and 500ms most of the times.
I vaguely remembered on SAMA5D2 the old atmel raw nand driver is used which does not support reading the R/B signal, but nevertheless works. I'm not familiar in detail with those raw NAND flash chips, but as far as I can understand, there are other ways to determine if the chip is ready or busy. So after I removed that rb-gpio parameter from 'at91-sam9x60_curiosity.dts' I ran into the bug fixed by patch 2.
With that patch applied _and_ rb-gpio still removed from dts, raw NAND access is reasonably fast on sam9x60-curiosity board. (You might want to rebase to atmel/next for testing this.) Not sure if I should send a patch for that dts change, because I suppose it's a workaround only and not addressing the actual cause?
I think the fix is correct in itself, I tested different combinations and compared with the driver in Linux, however …
Can anyone explain to me, why flash access is sooo slow if the R/B gpio is used? Especially in comparision to Linux, where I don't need to remove that thing from dts and it works reasonably fast?
// I'm sorry for quoting (email is sent from Outlook) # Please add in your board dts: atmel,rb = <0> # nand@3 { # reg = <0x3 0x0 0x800000>; # atmel,rb = <0>; # cs-gpios = <&pioD 4 GPIO_ACTIVE_HIGH>; # rb-gpios = <&pioD 5 GPIO_ACTIVE_HIGH>; # nand-bus-width = <8>; # nand-ecc-mode = "hw"; # nand-ecc-strength = <8>; # nand-ecc-step-size = <512>; # nand-on-flash-bbt;>
The actual flash chip is a Macronix MX30LF4G28AD, 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 256.
Greets Alex
P.S.: although not returned by get_maintainer.pl I added Eugen to Cc because he is maintainer of the at91 and might have some insight if it is a general problem of the nand controller in at91 socs?
Alexander Dahl (2): mtd: nand: raw: atmel: Remove duplicate line mtd: nand: raw: atmel: Add error handling when rb-gpios missing
drivers/mtd/nand/raw/atmel/nand-controller.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
base-commit: a169438411f9277cc689c14078151aa1d1caae3c
Hello Mihai,
Am Tue, Aug 08, 2023 at 01:40:26PM +0000 schrieb Mihai.Sain@microchip.com:
Hi Alex,
Please find bellow my answer:
Added Mihai who tested this a lot at some point in time
Eugen
On 8/8/23 16:02, Alexander Dahl wrote:
Hello everyone,
this is a patch series wtih some real fixes _and_ a question or some kind of support request in the cover letter. I would be happy if anyone could read the cover letter carefully and answer to that one what might be the problem I see. O:-)
I'm currently working on the sam9x60-curiosity board and especially trying to get it booting from onboard raw NAND flash. As reported in my last series I got the flash recognized already. However interacting with it was terribly slow, because nand_wait_ready() calling atmel_nand_dev_ready() ran into a 400ms timeout in several occasions, especially when reading from the device. Reading a single block triggered that timeout two times per page, which summed up to over 50 seconds for 64 × 4096 = 256k Bytes!
(You can have U-Boot print that warning from nand_wait_ready() by increasing the console log level to at least "warn".)
Note: the dts from atmel/next seems correct to me, I double checked that again. My own debug log messages showed the GPIO is accessed, and if you add enough debug messages sometimes the timeout is not reached, so I'm sure the NAND chip eventually switches that R/B line and the code correctly sees that, that line level change however takes ages, something between 400ms and 500ms most of the times.
I vaguely remembered on SAMA5D2 the old atmel raw nand driver is used which does not support reading the R/B signal, but nevertheless works. I'm not familiar in detail with those raw NAND flash chips, but as far as I can understand, there are other ways to determine if the chip is ready or busy. So after I removed that rb-gpio parameter from 'at91-sam9x60_curiosity.dts' I ran into the bug fixed by patch 2.
Maybe I should add: currently there are two different drivers for atmel raw nand controllers in U-Boot, the old non-DM driver used by sam9g20 or sama5 based boards for example and a new driver used by sam9x60 based boards. We are talking about sam9x60 and the new driver in 'drivers/mtd/nand/raw/atmel/nand-controller.c' here.
With that patch applied _and_ rb-gpio still removed from dts, raw NAND access is reasonably fast on sam9x60-curiosity board. (You might want to rebase to atmel/next for testing this.) Not sure if I should send a patch for that dts change, because I suppose it's a workaround only and not addressing the actual cause?
I think the fix is correct in itself, I tested different combinations and compared with the driver in Linux, however …
Can anyone explain to me, why flash access is sooo slow if the R/B gpio is used? Especially in comparision to Linux, where I don't need to remove that thing from dts and it works reasonably fast?
// I'm sorry for quoting (email is sent from Outlook) # Please add in your board dts: atmel,rb = <0>
Although the new U-Boot driver tests for that, it is not documented in U-Boot devicetree bindings. According to Linux kernel (v6.4) device tree binding documentation it is only meaningful for sama5. Those binding documentation was added to Linux back in 2019 by Boris Brezillon. And that line is not present in the dts file for sam9x60-curiosity in Linux, so access is fast on Linux without that line (this is the part I don't understand yet).
From what I saw in datasheets sam9x60 and sama5d2 have different
controllers (named 'SMC' and 'HSMC') and the U-Boot driver reflects that. That's why I did not add 'atmel,rb' to dts before, I though SMC is the older one which does not support native R/B line handling? (And because I took it from U-Boot sam9x60ek.dts which also does not have it.)
Nevertheless, I tried to add it now in U-Boot as you suggested (although without adapting pinctrl), and to my suprise it works … o.O
Trying the same in Linux leads to not finding a NAND device and thus failing of the atmel-nand-controller driver probe.
(Note: all this while booting from SD card, so at91bootstrap should not interfere, because it does not touch nand.)
What I did not test yet:
- How it behaves in U-Boot when also adapting pinctrl - How other combinations of adaptions to dts behave in Linux
Sending this mail now anyway, it's too long already and office time is up for today. Will do more systematic tests tomorrow. Any more insights welcome.
Greets Alex
# nand@3 { # reg = <0x3 0x0 0x800000>; # atmel,rb = <0>; # cs-gpios = <&pioD 4 GPIO_ACTIVE_HIGH>; # rb-gpios = <&pioD 5 GPIO_ACTIVE_HIGH>; # nand-bus-width = <8>; # nand-ecc-mode = "hw"; # nand-ecc-strength = <8>; # nand-ecc-step-size = <512>; # nand-on-flash-bbt;>
The actual flash chip is a Macronix MX30LF4G28AD, 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 256.
Greets Alex
P.S.: although not returned by get_maintainer.pl I added Eugen to Cc because he is maintainer of the at91 and might have some insight if it is a general problem of the nand controller in at91 socs?
Alexander Dahl (2): mtd: nand: raw: atmel: Remove duplicate line mtd: nand: raw: atmel: Add error handling when rb-gpios missing
drivers/mtd/nand/raw/atmel/nand-controller.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
base-commit: a169438411f9277cc689c14078151aa1d1caae3c
Hello everyone,
I had a closer look into the SAMA5D2 Series Datasheet and the SAM9X60 Data Sheet again. See below.
Am Tue, Aug 08, 2023 at 05:00:48PM +0200 schrieb Alexander Dahl:
Hello Mihai,
Am Tue, Aug 08, 2023 at 01:40:26PM +0000 schrieb Mihai.Sain@microchip.com:
Hi Alex,
Please find bellow my answer:
Added Mihai who tested this a lot at some point in time
Eugen
On 8/8/23 16:02, Alexander Dahl wrote:
Hello everyone,
this is a patch series wtih some real fixes _and_ a question or some kind of support request in the cover letter. I would be happy if anyone could read the cover letter carefully and answer to that one what might be the problem I see. O:-)
I'm currently working on the sam9x60-curiosity board and especially trying to get it booting from onboard raw NAND flash. As reported in my last series I got the flash recognized already. However interacting with it was terribly slow, because nand_wait_ready() calling atmel_nand_dev_ready() ran into a 400ms timeout in several occasions, especially when reading from the device. Reading a single block triggered that timeout two times per page, which summed up to over 50 seconds for 64 × 4096 = 256k Bytes!
(You can have U-Boot print that warning from nand_wait_ready() by increasing the console log level to at least "warn".)
Note: the dts from atmel/next seems correct to me, I double checked that again. My own debug log messages showed the GPIO is accessed, and if you add enough debug messages sometimes the timeout is not reached, so I'm sure the NAND chip eventually switches that R/B line and the code correctly sees that, that line level change however takes ages, something between 400ms and 500ms most of the times.
I vaguely remembered on SAMA5D2 the old atmel raw nand driver is used which does not support reading the R/B signal, but nevertheless works. I'm not familiar in detail with those raw NAND flash chips, but as far as I can understand, there are other ways to determine if the chip is ready or busy. So after I removed that rb-gpio parameter from 'at91-sam9x60_curiosity.dts' I ran into the bug fixed by patch 2.
Maybe I should add: currently there are two different drivers for atmel raw nand controllers in U-Boot, the old non-DM driver used by sam9g20 or sama5 based boards for example and a new driver used by sam9x60 based boards. We are talking about sam9x60 and the new driver in 'drivers/mtd/nand/raw/atmel/nand-controller.c' here.
The old one is enabled by CONFIG_NAND_ATMEL, the new one by CONFIG_DM_NAND_ATMEL instead.
With that patch applied _and_ rb-gpio still removed from dts, raw NAND access is reasonably fast on sam9x60-curiosity board. (You might want to rebase to atmel/next for testing this.) Not sure if I should send a patch for that dts change, because I suppose it's a workaround only and not addressing the actual cause?
I think the fix is correct in itself, I tested different combinations and compared with the driver in Linux, however …
Can anyone explain to me, why flash access is sooo slow if the R/B gpio is used? Especially in comparision to Linux, where I don't need to remove that thing from dts and it works reasonably fast?
// I'm sorry for quoting (email is sent from Outlook) # Please add in your board dts: atmel,rb = <0>
Although the new U-Boot driver tests for that, it is not documented in U-Boot devicetree bindings. According to Linux kernel (v6.4) device tree binding documentation it is only meaningful for sama5. Those binding documentation was added to Linux back in 2019 by Boris Brezillon. And that line is not present in the dts file for sam9x60-curiosity in Linux, so access is fast on Linux without that line (this is the part I don't understand yet).
From what I saw in datasheets sam9x60 and sama5d2 have different controllers (named 'SMC' and 'HSMC') and the U-Boot driver reflects that. That's why I did not add 'atmel,rb' to dts before, I though SMC is the older one which does not support native R/B line handling? (And because I took it from U-Boot sam9x60ek.dts which also does not have it.)
Nevertheless, I tried to add it now in U-Boot as you suggested (although without adapting pinctrl), and to my suprise it works … o.O
Trying the same in Linux leads to not finding a NAND device and thus failing of the atmel-nand-controller driver probe.
From datasheet POV this all makes sense. The HSMC on SAMA5D2 has a
native NANDRDY signal/peripheral function to handle the R/B pin of the flash and can do the low level communication by itself with an embedded Nand Flash Controller (NFC). The SMC on SAM9X60 has no such thing and the datasheet explicitly recommends to connect the R/B pin to _any_ GPIO.
The new dm-based nand-controller driver in U-Boot only checks if dts has the 'rb-gpios' property and only then overrides the chip->dev_ready function. So for U-Boot this explains why adding that line yields the same result: it's effectivly the same handling as with ATMEL_NAND_NO_RB.
And because Linux can also not use a SoC feature which simply is not there, the question remains:
If both Linux and U-Boot have the GPIO handling for R/B setup, why can Linux access the chip fast and U-Boot can not?
I suspect because they have different drivers doing different things? One would have to look in deep detail what is done differently?
Whatever it is, from my POV it makes no sense to add the 'atmel,rb' property to the sam9x60 dts. The only two possible paths I see:
1. Short workaround: Remove the 'rb-gpios' property from U-Boot dts. The status can also be fetched through a NAND command from status register bit 6 of the flash (NAND_STATUS_READY) and that is done in drivers/mtd/nand/raw/nand_base.c all over the place. (This is probably why it works in this case and also why it works for SAMA5D2 in U-Boot with other boards, where we explicitly had to undef SYS_NAND_READY_PIN.)
2. Long term solution: Find the real cause and fix the U-Boot driver(s) …
(Note: all this while booting from SD card, so at91bootstrap should not interfere, because it does not touch nand.)
What I did not test yet:
- How it behaves in U-Boot when also adapting pinctrl
- How other combinations of adaptions to dts behave in Linux
Won't pursue these tests. Makes no sense in my opinion, because of the reasons stated above.
Greets Alex
Sending this mail now anyway, it's too long already and office time is up for today. Will do more systematic tests tomorrow. Any more insights welcome.
Greets Alex
# nand@3 { # reg = <0x3 0x0 0x800000>; # atmel,rb = <0>; # cs-gpios = <&pioD 4 GPIO_ACTIVE_HIGH>; # rb-gpios = <&pioD 5 GPIO_ACTIVE_HIGH>; # nand-bus-width = <8>; # nand-ecc-mode = "hw"; # nand-ecc-strength = <8>; # nand-ecc-step-size = <512>; # nand-on-flash-bbt;>
The actual flash chip is a Macronix MX30LF4G28AD, 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 256.
Greets Alex
P.S.: although not returned by get_maintainer.pl I added Eugen to Cc because he is maintainer of the at91 and might have some insight if it is a general problem of the nand controller in at91 socs?
Alexander Dahl (2): mtd: nand: raw: atmel: Remove duplicate line mtd: nand: raw: atmel: Add error handling when rb-gpios missing
drivers/mtd/nand/raw/atmel/nand-controller.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
base-commit: a169438411f9277cc689c14078151aa1d1caae3c
Hello,
just a short supplement …
Am Wed, Aug 09, 2023 at 09:40:15AM +0200 schrieb Alexander Dahl:
Hello everyone,
I had a closer look into the SAMA5D2 Series Datasheet and the SAM9X60 Data Sheet again. See below.
Am Tue, Aug 08, 2023 at 05:00:48PM +0200 schrieb Alexander Dahl:
Hello Mihai,
Am Tue, Aug 08, 2023 at 01:40:26PM +0000 schrieb Mihai.Sain@microchip.com:
Hi Alex,
Please find bellow my answer:
Added Mihai who tested this a lot at some point in time
Eugen
On 8/8/23 16:02, Alexander Dahl wrote:
Hello everyone,
this is a patch series wtih some real fixes _and_ a question or some kind of support request in the cover letter. I would be happy if anyone could read the cover letter carefully and answer to that one what might be the problem I see. O:-)
I'm currently working on the sam9x60-curiosity board and especially trying to get it booting from onboard raw NAND flash. As reported in my last series I got the flash recognized already. However interacting with it was terribly slow, because nand_wait_ready() calling atmel_nand_dev_ready() ran into a 400ms timeout in several occasions, especially when reading from the device. Reading a single block triggered that timeout two times per page, which summed up to over 50 seconds for 64 × 4096 = 256k Bytes!
(You can have U-Boot print that warning from nand_wait_ready() by increasing the console log level to at least "warn".)
Note: the dts from atmel/next seems correct to me, I double checked that again. My own debug log messages showed the GPIO is accessed, and if you add enough debug messages sometimes the timeout is not reached, so I'm sure the NAND chip eventually switches that R/B line and the code correctly sees that, that line level change however takes ages, something between 400ms and 500ms most of the times.
I vaguely remembered on SAMA5D2 the old atmel raw nand driver is used which does not support reading the R/B signal, but nevertheless works. I'm not familiar in detail with those raw NAND flash chips, but as far as I can understand, there are other ways to determine if the chip is ready or busy. So after I removed that rb-gpio parameter from 'at91-sam9x60_curiosity.dts' I ran into the bug fixed by patch 2.
Maybe I should add: currently there are two different drivers for atmel raw nand controllers in U-Boot, the old non-DM driver used by sam9g20 or sama5 based boards for example and a new driver used by sam9x60 based boards. We are talking about sam9x60 and the new driver in 'drivers/mtd/nand/raw/atmel/nand-controller.c' here.
The old one is enabled by CONFIG_NAND_ATMEL, the new one by CONFIG_DM_NAND_ATMEL instead.
With that patch applied _and_ rb-gpio still removed from dts, raw NAND access is reasonably fast on sam9x60-curiosity board. (You might want to rebase to atmel/next for testing this.) Not sure if I should send a patch for that dts change, because I suppose it's a workaround only and not addressing the actual cause?
I think the fix is correct in itself, I tested different combinations and compared with the driver in Linux, however …
Can anyone explain to me, why flash access is sooo slow if the R/B gpio is used? Especially in comparision to Linux, where I don't need to remove that thing from dts and it works reasonably fast?
// I'm sorry for quoting (email is sent from Outlook) # Please add in your board dts: atmel,rb = <0>
Although the new U-Boot driver tests for that, it is not documented in U-Boot devicetree bindings. According to Linux kernel (v6.4) device tree binding documentation it is only meaningful for sama5. Those binding documentation was added to Linux back in 2019 by Boris Brezillon. And that line is not present in the dts file for sam9x60-curiosity in Linux, so access is fast on Linux without that line (this is the part I don't understand yet).
From what I saw in datasheets sam9x60 and sama5d2 have different controllers (named 'SMC' and 'HSMC') and the U-Boot driver reflects that. That's why I did not add 'atmel,rb' to dts before, I though SMC is the older one which does not support native R/B line handling? (And because I took it from U-Boot sam9x60ek.dts which also does not have it.)
Nevertheless, I tried to add it now in U-Boot as you suggested (although without adapting pinctrl), and to my suprise it works … o.O
Trying the same in Linux leads to not finding a NAND device and thus failing of the atmel-nand-controller driver probe.
From datasheet POV this all makes sense. The HSMC on SAMA5D2 has a native NANDRDY signal/peripheral function to handle the R/B pin of the flash and can do the low level communication by itself with an embedded Nand Flash Controller (NFC). The SMC on SAM9X60 has no such thing and the datasheet explicitly recommends to connect the R/B pin to _any_ GPIO.
The new dm-based nand-controller driver in U-Boot only checks if dts has the 'rb-gpios' property and only then overrides the chip->dev_ready function. So for U-Boot this explains why adding that line yields the same result: it's effectivly the same handling as with ATMEL_NAND_NO_RB.
And because Linux can also not use a SoC feature which simply is not there, the question remains:
If both Linux and U-Boot have the GPIO handling for R/B setup, why can Linux access the chip fast and U-Boot can not?
I suspect because they have different drivers doing different things? One would have to look in deep detail what is done differently?
The answer is maybe simpler than I thought? No driver at all in Linux (v6.4) evaluates the 'rb-gpios' devicetree property. The Linux atmel nand controller driver only considers an old binding, which is not used in recent dts for sam9x60. So I guess Linux also does "soft wait" only and does not read the R/B pin. I suggest to discuss this topic separately on linux-mtd …
Greets Alex
P.S.: according to the schematic/layout files of the sam9x60-curiosity board it's not easily possible to probe the R/B signal with an oscilloscope or logic analyzer, because it's a direct wire between a ball of the flash chip and a ball of the SoC, which is not exposed. No pads to probe, one would have to scratch away the covering solder mask …
Whatever it is, from my POV it makes no sense to add the 'atmel,rb' property to the sam9x60 dts. The only two possible paths I see:
- Short workaround: Remove the 'rb-gpios' property from U-Boot dts.
The status can also be fetched through a NAND command from status register bit 6 of the flash (NAND_STATUS_READY) and that is done in drivers/mtd/nand/raw/nand_base.c all over the place. (This is probably why it works in this case and also why it works for SAMA5D2 in U-Boot with other boards, where we explicitly had to undef SYS_NAND_READY_PIN.)
- Long term solution: Find the real cause and fix the U-Boot
driver(s) …
(Note: all this while booting from SD card, so at91bootstrap should not interfere, because it does not touch nand.)
What I did not test yet:
- How it behaves in U-Boot when also adapting pinctrl
- How other combinations of adaptions to dts behave in Linux
Won't pursue these tests. Makes no sense in my opinion, because of the reasons stated above.
Greets Alex
Sending this mail now anyway, it's too long already and office time is up for today. Will do more systematic tests tomorrow. Any more insights welcome.
Greets Alex
# nand@3 { # reg = <0x3 0x0 0x800000>; # atmel,rb = <0>; # cs-gpios = <&pioD 4 GPIO_ACTIVE_HIGH>; # rb-gpios = <&pioD 5 GPIO_ACTIVE_HIGH>; # nand-bus-width = <8>; # nand-ecc-mode = "hw"; # nand-ecc-strength = <8>; # nand-ecc-step-size = <512>; # nand-on-flash-bbt;>
The actual flash chip is a Macronix MX30LF4G28AD, 512 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 256.
Greets Alex
P.S.: although not returned by get_maintainer.pl I added Eugen to Cc because he is maintainer of the at91 and might have some insight if it is a general problem of the nand controller in at91 socs?
Alexander Dahl (2): mtd: nand: raw: atmel: Remove duplicate line mtd: nand: raw: atmel: Add error handling when rb-gpios missing
drivers/mtd/nand/raw/atmel/nand-controller.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
base-commit: a169438411f9277cc689c14078151aa1d1caae3c
participants (4)
-
Alexander Dahl -
Eugen Hristev -
Michael Nazzareno Trimarchi -
Mihai.Sain＠microchip.com