[PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
While the Denali NAND is initialized by the BootROM in SPL, there are still a couple of settings which are missing. These can trigger subtle corruption of the data read out of the NAND. Fill these settings in just like they are filled in by the full Denali NAND driver in denali_hw_init().
Signed-off-by: Marek Vasut marex@denx.de Cc: Masahiro Yamada yamada.masahiro@socionext.com --- drivers/mtd/nand/raw/denali_spl.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/mtd/nand/raw/denali_spl.c b/drivers/mtd/nand/raw/denali_spl.c index dbaba3cab2..b8b29812aa 100644 --- a/drivers/mtd/nand/raw/denali_spl.c +++ b/drivers/mtd/nand/raw/denali_spl.c @@ -173,6 +173,13 @@ void nand_init(void) page_size = readl(denali_flash_reg + DEVICE_MAIN_AREA_SIZE); oob_size = readl(denali_flash_reg + DEVICE_SPARE_AREA_SIZE); pages_per_block = readl(denali_flash_reg + PAGES_PER_BLOCK); + + /* Do as denali_hw_init() does. */ + writel(CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES, + denali_flash_reg + SPARE_AREA_SKIP_BYTES); + writel(0x0F, denali_flash_reg + RB_PIN_ENABLED); + writel(CHIP_EN_DONT_CARE__FLAG, denali_flash_reg + CHIP_ENABLE_DONT_CARE); + writel(0xffff, denali_flash_reg + SPARE_AREA_MARKER); }
int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
From: Masahiro Yamada yamada.masahiro@socionext.com
The "nand_x" and "ecc" clocks are currently optional. Make the core clock optional in the same way. This will allow platforms with no clock driver support to use this driver.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Tested-by: Marek Vasut marex@denx.de # On SoCFPGA Arria V --- drivers/mtd/nand/raw/denali_dt.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c index 0ce81324b9..b1e14982c4 100644 --- a/drivers/mtd/nand/raw/denali_dt.c +++ b/drivers/mtd/nand/raw/denali_dt.c @@ -91,7 +91,7 @@ static int denali_dt_probe(struct udevice *dev) if (ret) ret = clk_get_by_index(dev, 0, &clk); if (ret) - return ret; + clk.dev = NULL;
ret = clk_get_by_name(dev, "nand_x", &clk_x); if (ret) @@ -101,9 +101,11 @@ static int denali_dt_probe(struct udevice *dev) if (ret) clk_ecc.dev = NULL;
- ret = clk_enable(&clk); - if (ret) - return ret; + if (clk.dev) { + ret = clk_enable(&clk); + if (ret) + return ret; + }
if (clk_x.dev) { ret = clk_enable(&clk_x);
The Denali NAND block loses configuration when put in reset. Specifically, RB_PIN_ENABLED, CHIP_ENABLE_DONT_CARE, SPARE_AREA_SKIP_BYTES and SPARE_AREA_MARKER are lost. Since mainline Linux depends on the configuration programmed into the Denali NAND controller by the bootloader, do not reset the controller before starting the kernel, otherwise the kernel will read bogus values and fail to use the NAND.
Fixes: ed784ac3822b ("mtd: rawnand: denali: add reset handling") Signed-off-by: Marek Vasut marex@denx.de Cc: Masahiro Yamada yamada.masahiro@socionext.com Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com --- drivers/mtd/nand/raw/denali_dt.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c index b1e14982c4..03c97dbc05 100644 --- a/drivers/mtd/nand/raw/denali_dt.c +++ b/drivers/mtd/nand/raw/denali_dt.c @@ -142,21 +142,12 @@ static int denali_dt_probe(struct udevice *dev) return denali_init(denali); }
-static int denali_dt_remove(struct udevice *dev) -{ - struct denali_nand_info *denali = dev_get_priv(dev); - - return reset_release_bulk(&denali->resets); -} - U_BOOT_DRIVER(denali_nand_dt) = { .name = "denali-nand-dt", .id = UCLASS_MISC, .of_match = denali_nand_dt_ids, .probe = denali_dt_probe, .priv_auto_alloc_size = sizeof(struct denali_nand_info), - .remove = denali_dt_remove, - .flags = DM_FLAG_OS_PREPARE, };
void board_nand_init(void)
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
The Denali NAND block loses configuration when put in reset. Specifically, RB_PIN_ENABLED, CHIP_ENABLE_DONT_CARE, SPARE_AREA_SKIP_BYTES and SPARE_AREA_MARKER are lost. Since mainline Linux depends on the configuration programmed into the Denali NAND controller by the bootloader, do not reset the controller before starting the kernel, otherwise the kernel will read bogus values and fail to use the NAND.
I think this log is not directly describing the issue. It is not a matter of reading bogus values. You cannot use the hardware under the reset state in the first place.
How about describing the root cause? For example,
The denali driver in the mainline Linux does not support the reset control yet. Do not reset the controller before starting the kernel, otherwise the kernel cannot deassert the reset of the NAND controller.
(BTW, the situation is worse on the UniPhier platform. The kernel will crash because the register access never respond.)
Fixes: ed784ac3822b ("mtd: rawnand: denali: add reset handling") Signed-off-by: Marek Vasut marex@denx.de Cc: Masahiro Yamada yamada.masahiro@socionext.com Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
drivers/mtd/nand/raw/denali_dt.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c index b1e14982c4..03c97dbc05 100644 --- a/drivers/mtd/nand/raw/denali_dt.c +++ b/drivers/mtd/nand/raw/denali_dt.c @@ -142,21 +142,12 @@ static int denali_dt_probe(struct udevice *dev) return denali_init(denali); }
-static int denali_dt_remove(struct udevice *dev) -{
struct denali_nand_info *denali = dev_get_priv(dev);return reset_release_bulk(&denali->resets);-}
U_BOOT_DRIVER(denali_nand_dt) = { .name = "denali-nand-dt", .id = UCLASS_MISC, .of_match = denali_nand_dt_ids, .probe = denali_dt_probe, .priv_auto_alloc_size = sizeof(struct denali_nand_info),
.remove = denali_dt_remove,.flags = DM_FLAG_OS_PREPARE,};
void board_nand_init(void)
2.24.1
-- Best Regards Masahiro Yamada
On 1/10/20 4:09 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
The Denali NAND block loses configuration when put in reset. Specifically, RB_PIN_ENABLED, CHIP_ENABLE_DONT_CARE, SPARE_AREA_SKIP_BYTES and SPARE_AREA_MARKER are lost. Since mainline Linux depends on the configuration programmed into the Denali NAND controller by the bootloader, do not reset the controller before starting the kernel, otherwise the kernel will read bogus values and fail to use the NAND.
I think this log is not directly describing the issue. It is not a matter of reading bogus values. You cannot use the hardware under the reset state in the first place.
How about describing the root cause? For example,
The denali driver in the mainline Linux does not support the reset control yet. Do not reset the controller before starting the kernel, otherwise the kernel cannot deassert the reset of the NAND controller.
Is this better?
The Denali NAND block loses configuration when put in reset. Specifically, RB_PIN_ENABLED, CHIP_ENABLE_DONT_CARE, SPARE_AREA_SKIP_BYTES and SPARE_AREA_MARKER are lost. Since mainline Linux depends on the configuration programmed into the Denali NAND controller by the bootloader, do not reset the controller before starting the kernel, otherwise the kernel will either read bogus values and fail to use the NAND or get stuck on register access.
(BTW, the situation is worse on the UniPhier platform. The kernel will crash because the register access never respond.)
So uniphier is even worse off.
On Fri, Jan 10, 2020 at 1:05 PM Marek Vasut marex@denx.de wrote:
On 1/10/20 4:09 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
The Denali NAND block loses configuration when put in reset. Specifically, RB_PIN_ENABLED, CHIP_ENABLE_DONT_CARE, SPARE_AREA_SKIP_BYTES and SPARE_AREA_MARKER are lost. Since mainline Linux depends on the configuration programmed into the Denali NAND controller by the bootloader, do not reset the controller before starting the kernel, otherwise the kernel will read bogus values and fail to use the NAND.
I think this log is not directly describing the issue. It is not a matter of reading bogus values. You cannot use the hardware under the reset state in the first place.
How about describing the root cause? For example,
The denali driver in the mainline Linux does not support the reset control yet. Do not reset the controller before starting the kernel, otherwise the kernel cannot deassert the reset of the NAND controller.
Is this better?
I do not think so.
Most of the description is just redundant.
Hardware under the reset state does not work in any way. The mainline kernel cannot deassert the NAND controller reset by itself. That's all.
The Denali NAND block loses configuration when put in reset. Specifically, RB_PIN_ENABLED, CHIP_ENABLE_DONT_CARE, SPARE_AREA_SKIP_BYTES and SPARE_AREA_MARKER are lost. Since mainline Linux depends on the configuration programmed into the Denali NAND controller by the bootloader, do not reset the controller before starting the kernel, otherwise the kernel will either read bogus values and fail to use the NAND or get stuck on register access.(BTW, the situation is worse on the UniPhier platform. The kernel will crash because the register access never respond.)
So uniphier is even worse off.
-- Best Regards Masahiro Yamada
On 1/10/20 6:26 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 1:05 PM Marek Vasut marex@denx.de wrote:
On 1/10/20 4:09 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
The Denali NAND block loses configuration when put in reset. Specifically, RB_PIN_ENABLED, CHIP_ENABLE_DONT_CARE, SPARE_AREA_SKIP_BYTES and SPARE_AREA_MARKER are lost. Since mainline Linux depends on the configuration programmed into the Denali NAND controller by the bootloader, do not reset the controller before starting the kernel, otherwise the kernel will read bogus values and fail to use the NAND.
I think this log is not directly describing the issue. It is not a matter of reading bogus values. You cannot use the hardware under the reset state in the first place.
How about describing the root cause? For example,
The denali driver in the mainline Linux does not support the reset control yet. Do not reset the controller before starting the kernel, otherwise the kernel cannot deassert the reset of the NAND controller.
Is this better?
I do not think so.
Most of the description is just redundant.
Hardware under the reset state does not work in any way. The mainline kernel cannot deassert the NAND controller reset by itself. That's all.
That's only applicable to current state of things.
Can you provide better description ? Note that on SoCFPGA, the behavior is exactly as documented here. Maybe it's different on Socionext ?
On Mon, Jan 13, 2020 at 8:11 PM Marek Vasut marex@denx.de wrote:
On 1/10/20 6:26 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 1:05 PM Marek Vasut marex@denx.de wrote:
On 1/10/20 4:09 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
The Denali NAND block loses configuration when put in reset. Specifically, RB_PIN_ENABLED, CHIP_ENABLE_DONT_CARE, SPARE_AREA_SKIP_BYTES and SPARE_AREA_MARKER are lost. Since mainline Linux depends on the configuration programmed into the Denali NAND controller by the bootloader, do not reset the controller before starting the kernel, otherwise the kernel will read bogus values and fail to use the NAND.
I think this log is not directly describing the issue. It is not a matter of reading bogus values. You cannot use the hardware under the reset state in the first place.
How about describing the root cause? For example,
The denali driver in the mainline Linux does not support the reset control yet. Do not reset the controller before starting the kernel, otherwise the kernel cannot deassert the reset of the NAND controller.
Is this better?
I do not think so.
Most of the description is just redundant.
Hardware under the reset state does not work in any way. The mainline kernel cannot deassert the NAND controller reset by itself. That's all.
That's only applicable to current state of things.
Yes, I said the current state of the mainline kernel (since I am not a forecaster)
Can you provide better description ?
I did already:
https://lists.denx.de/pipermail/u-boot/2020-January/395878.html
Who cares about the SPARE_AREA_SKIP_BYTES value under the situation where you cannot deassert the reset?
Note that on SoCFPGA, the behavior is exactly as documented here. Maybe it's different on Socionext ?
The behavior of the NAND controller on Socionext SoCs is this: "It does not work while its reset signal is asserted."
I guess it is the same on SOCFPGA.
I have never seen such hardware that works with its reset signal asserted.
-- Best Regards Masahiro Yamada
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
The Denali NAND block loses configuration when put in reset. Specifically, RB_PIN_ENABLED, CHIP_ENABLE_DONT_CARE, SPARE_AREA_SKIP_BYTES and SPARE_AREA_MARKER are lost. Since mainline Linux depends on the configuration programmed into the Denali NAND controller by the bootloader, do not reset the controller before starting the kernel, otherwise the kernel will read bogus values and fail to use the NAND.
Fixes: ed784ac3822b ("mtd: rawnand: denali: add reset handling") Signed-off-by: Marek Vasut marex@denx.de Cc: Masahiro Yamada yamada.masahiro@socionext.com Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
drivers/mtd/nand/raw/denali_dt.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c index b1e14982c4..03c97dbc05 100644 --- a/drivers/mtd/nand/raw/denali_dt.c +++ b/drivers/mtd/nand/raw/denali_dt.c @@ -142,21 +142,12 @@ static int denali_dt_probe(struct udevice *dev) return denali_init(denali); }
-static int denali_dt_remove(struct udevice *dev) -{
struct denali_nand_info *denali = dev_get_priv(dev);return reset_release_bulk(&denali->resets);-}
U_BOOT_DRIVER(denali_nand_dt) = { .name = "denali-nand-dt", .id = UCLASS_MISC, .of_match = denali_nand_dt_ids, .probe = denali_dt_probe, .priv_auto_alloc_size = sizeof(struct denali_nand_info),
.remove = denali_dt_remove,.flags = DM_FLAG_OS_PREPARE,};
void board_nand_init(void)
2.24.1
One more thing:
The struct reset can be a local variable.
diff --git a/drivers/mtd/nand/raw/denali.h b/drivers/mtd/nand/raw/denali.h index 63ae828768c9..019deda094e5 100644 --- a/drivers/mtd/nand/raw/denali.h +++ b/drivers/mtd/nand/raw/denali.h @@ -10,7 +10,6 @@ #include <linux/bitops.h> #include <linux/mtd/rawnand.h> #include <linux/types.h> -#include <reset.h>
#define DEVICE_RESET 0x0 #define DEVICE_RESET__BANK(bank) BIT(bank) @@ -316,7 +315,6 @@ struct denali_nand_info { void (*host_write)(struct denali_nand_info *denali, u32 addr, u32 data); void (*setup_dma)(struct denali_nand_info *denali, dma_addr_t dma_addr, int page, int write); - struct reset_ctl_bulk resets; };
#define DENALI_CAP_HW_ECC_FIXUP BIT(0) diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c index b1e14982c443..9d5a01b076e9 100644 --- a/drivers/mtd/nand/raw/denali_dt.c +++ b/drivers/mtd/nand/raw/denali_dt.c @@ -6,6 +6,7 @@
#include <clk.h> #include <dm.h> +#include <reset.h> #include <linux/io.h> #include <linux/ioport.h> #include <linux/printk.h> @@ -63,6 +64,7 @@ static int denali_dt_probe(struct udevice *dev) struct denali_nand_info *denali = dev_get_priv(dev); const struct denali_dt_data *data; struct clk clk, clk_x, clk_ecc; + struct reset_ctl_bulk resets; struct resource res; int ret;
@@ -133,11 +135,11 @@ static int denali_dt_probe(struct udevice *dev) denali->clk_x_rate = 200000000; }
- ret = reset_get_bulk(dev, &denali->resets); + ret = reset_get_bulk(dev, &resets); if (ret) dev_warn(dev, "Can't get reset: %d\n", ret); else - reset_deassert_bulk(&denali->resets); + reset_deassert_bulk(&resets);
return denali_init(denali); }
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
While the Denali NAND is initialized by the BootROM in SPL, there are still a couple of settings which are missing.
This statement is wrong.
While the Denali NAND is initialized by the BootROM, the SOCFPGA SPL calls socfpga_per_reset_all() to put most of peripherals into the reset state. So, all the register values are lost.
These can trigger subtle corruption of the data read out of the NAND. Fill these settings in just like they are filled in by the full Denali NAND driver in denali_hw_init().
Signed-off-by: Marek Vasut marex@denx.de Cc: Masahiro Yamada yamada.masahiro@socionext.com
drivers/mtd/nand/raw/denali_spl.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/mtd/nand/raw/denali_spl.c b/drivers/mtd/nand/raw/denali_spl.c index dbaba3cab2..b8b29812aa 100644 --- a/drivers/mtd/nand/raw/denali_spl.c +++ b/drivers/mtd/nand/raw/denali_spl.c @@ -173,6 +173,13 @@ void nand_init(void) page_size = readl(denali_flash_reg + DEVICE_MAIN_AREA_SIZE); oob_size = readl(denali_flash_reg + DEVICE_SPARE_AREA_SIZE); pages_per_block = readl(denali_flash_reg + PAGES_PER_BLOCK);
/* Do as denali_hw_init() does. */writel(CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES,denali_flash_reg + SPARE_AREA_SKIP_BYTES);writel(0x0F, denali_flash_reg + RB_PIN_ENABLED);writel(CHIP_EN_DONT_CARE__FLAG, denali_flash_reg + CHIP_ENABLE_DONT_CARE);writel(0xffff, denali_flash_reg + SPARE_AREA_MARKER);
You put this code just because you found it worked for your board.
If you reset the NAND controller, not only these four, but all the register values are lost. Especially, page_size, oob_size, etc. https://github.com/u-boot/u-boot/blob/v2020.01/drivers/mtd/nand/raw/denali_s...
It is working on your board because SOCFPGA enables the reset sequencer, which enumerates the nand chip by hardware. It is not necessarily true for other platforms, like the UniPhier platform.
The reliable way for full initialization is to call nand_scan_ident(), which is too big for SPL.
To sum up, this is not a proper approach. Do not reset the NAND controller in SPL. Keep the working state that has already been set up by the boot ROM.
}
int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
2.24.1
On 1/10/20 3:45 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
While the Denali NAND is initialized by the BootROM in SPL, there are still a couple of settings which are missing.
This statement is wrong.
While the Denali NAND is initialized by the BootROM, the SOCFPGA SPL calls socfpga_per_reset_all() to put most of peripherals into the reset state. So, all the register values are lost.
And that is fine, resetting hardware to put it into defined state is what must happen, otherwise we would have all kinds of obscure semi-defined states.
These can trigger subtle corruption of the data read out of the NAND. Fill these settings in just like they are filled in by the full Denali NAND driver in denali_hw_init().
Signed-off-by: Marek Vasut marex@denx.de Cc: Masahiro Yamada yamada.masahiro@socionext.com
drivers/mtd/nand/raw/denali_spl.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/mtd/nand/raw/denali_spl.c b/drivers/mtd/nand/raw/denali_spl.c index dbaba3cab2..b8b29812aa 100644 --- a/drivers/mtd/nand/raw/denali_spl.c +++ b/drivers/mtd/nand/raw/denali_spl.c @@ -173,6 +173,13 @@ void nand_init(void) page_size = readl(denali_flash_reg + DEVICE_MAIN_AREA_SIZE); oob_size = readl(denali_flash_reg + DEVICE_SPARE_AREA_SIZE); pages_per_block = readl(denali_flash_reg + PAGES_PER_BLOCK);
/* Do as denali_hw_init() does. */writel(CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES,denali_flash_reg + SPARE_AREA_SKIP_BYTES);writel(0x0F, denali_flash_reg + RB_PIN_ENABLED);writel(CHIP_EN_DONT_CARE__FLAG, denali_flash_reg + CHIP_ENABLE_DONT_CARE);writel(0xffff, denali_flash_reg + SPARE_AREA_MARKER);You put this code just because you found it worked for your board.
I'm not even sure what to respond to this. Yes, I put this code here because it initializes the NAND controller fully and yes, I tested it on actual physical SoCFPGA hardware.
If you reset the NAND controller, not only these four, but all the register values are lost. Especially, page_size, oob_size, etc. https://github.com/u-boot/u-boot/blob/v2020.01/drivers/mtd/nand/raw/denali_s...
Nope, those are correct on SoCFPGA.
It is working on your board because SOCFPGA enables the reset sequencer, which enumerates the nand chip by hardware. It is not necessarily true for other platforms, like the UniPhier platform.
So Uniphier has a different behavior, fine, shall I put ifdef around this then ? Or what is it that you want ?
The reliable way for full initialization is to call nand_scan_ident(), which is too big for SPL.
Right, so not an option.
To sum up, this is not a proper approach. Do not reset the NAND controller in SPL.
This is not a proper approach, not resetting hardware would mean you end up with hardware in undefined state.
Keep the working state that has already been set up by the boot ROM.
The state in which the controller is is NOT defined. Resetting it puts it into defined state.
I am fine with putting an ifdef(SOCFPGA) around this code to avoid triggering it on uniphier if that's fine with you.
On Fri, Jan 10, 2020 at 1:05 PM Marek Vasut marex@denx.de wrote:
On 1/10/20 3:45 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
While the Denali NAND is initialized by the BootROM in SPL, there are still a couple of settings which are missing.
This statement is wrong.
While the Denali NAND is initialized by the BootROM, the SOCFPGA SPL calls socfpga_per_reset_all() to put most of peripherals into the reset state. So, all the register values are lost.
And that is fine, resetting hardware to put it into defined state is what must happen, otherwise we would have all kinds of obscure semi-defined states.
These can trigger subtle corruption of the data read out of the NAND. Fill these settings in just like they are filled in by the full Denali NAND driver in denali_hw_init().
Signed-off-by: Marek Vasut marex@denx.de Cc: Masahiro Yamada yamada.masahiro@socionext.com
drivers/mtd/nand/raw/denali_spl.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/mtd/nand/raw/denali_spl.c b/drivers/mtd/nand/raw/denali_spl.c index dbaba3cab2..b8b29812aa 100644 --- a/drivers/mtd/nand/raw/denali_spl.c +++ b/drivers/mtd/nand/raw/denali_spl.c @@ -173,6 +173,13 @@ void nand_init(void) page_size = readl(denali_flash_reg + DEVICE_MAIN_AREA_SIZE); oob_size = readl(denali_flash_reg + DEVICE_SPARE_AREA_SIZE); pages_per_block = readl(denali_flash_reg + PAGES_PER_BLOCK);
/* Do as denali_hw_init() does. */writel(CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES,denali_flash_reg + SPARE_AREA_SKIP_BYTES);writel(0x0F, denali_flash_reg + RB_PIN_ENABLED);writel(CHIP_EN_DONT_CARE__FLAG, denali_flash_reg + CHIP_ENABLE_DONT_CARE);writel(0xffff, denali_flash_reg + SPARE_AREA_MARKER);You put this code just because you found it worked for your board.
I'm not even sure what to respond to this. Yes, I put this code here because it initializes the NAND controller fully and yes, I tested it on actual physical SoCFPGA hardware.
If you reset the NAND controller, not only these four, but all the register values are lost. Especially, page_size, oob_size, etc. https://github.com/u-boot/u-boot/blob/v2020.01/drivers/mtd/nand/raw/denali_s...
Nope, those are correct on SoCFPGA.
You are seeing values re-initialized by the controller.
Maybe, you do not know this IP.
So, here is how it works.
When you assert the reset, **all** register values are lost.
When you de-assert the reset again, the controller starts running without any software control. The controller issues a RESET command to the chip select 0, and attempts to read out the chip ID, and further more, ONFI parameters if it is an ONFI-compliant device. Then, the controller sets up the relevant registers based on the detected device parameters.
SOCFPGA is working based on this feature. That is why you thought only 4 registers were lost.
The recent uniphier SoCs tend to not use it because this hardware feature is known to be super-buggy for non-ONFi cases.
For recent uniphier SoCs, [2] is applied in the following commit description: https://github.com/torvalds/linux/commit/0615e7ad5d52
It is working on your board because SOCFPGA enables the reset sequencer, which enumerates the nand chip by hardware. It is not necessarily true for other platforms, like the UniPhier platform.
So Uniphier has a different behavior, fine, shall I put ifdef around this then ? Or what is it that you want ?
I do not want to see this patch in upstream.
Currently, CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES is only used for U-Boot proper.
I'd like to back-port http://patchwork.ozlabs.org/patch/1214018/ when it is accepted in Linux, then delete CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES entirely.
This patch is adding it to SPL.
CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES will stay if this patch gets in.
The reliable way for full initialization is to call nand_scan_ident(), which is too big for SPL.
Right, so not an option.
To sum up, this is not a proper approach. Do not reset the NAND controller in SPL.
This is not a proper approach, not resetting hardware would mean you end up with hardware in undefined state.
It's working in the state defined by the boot ROM. The boot ROM can read pages. So, SPL will be able to do it in the same manner.
Why don't you just use it as-is?
Keep the working state that has already been set up by the boot ROM.
The state in which the controller is is NOT defined. Resetting it puts it into defined state.
I am fine with putting an ifdef(SOCFPGA) around this code to avoid triggering it on uniphier if that's fine with you.
I am not fine with ugly #ifdef in drivers as you did in the previous 2/3, 3/3.
-- Best Regards Masahiro Yamada
On 1/10/20 7:26 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 1:05 PM Marek Vasut marex@denx.de wrote:
On 1/10/20 3:45 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
While the Denali NAND is initialized by the BootROM in SPL, there are still a couple of settings which are missing.
This statement is wrong.
While the Denali NAND is initialized by the BootROM, the SOCFPGA SPL calls socfpga_per_reset_all() to put most of peripherals into the reset state. So, all the register values are lost.
And that is fine, resetting hardware to put it into defined state is what must happen, otherwise we would have all kinds of obscure semi-defined states.
These can trigger subtle corruption of the data read out of the NAND. Fill these settings in just like they are filled in by the full Denali NAND driver in denali_hw_init().
Signed-off-by: Marek Vasut marex@denx.de Cc: Masahiro Yamada yamada.masahiro@socionext.com
drivers/mtd/nand/raw/denali_spl.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/mtd/nand/raw/denali_spl.c b/drivers/mtd/nand/raw/denali_spl.c index dbaba3cab2..b8b29812aa 100644 --- a/drivers/mtd/nand/raw/denali_spl.c +++ b/drivers/mtd/nand/raw/denali_spl.c @@ -173,6 +173,13 @@ void nand_init(void) page_size = readl(denali_flash_reg + DEVICE_MAIN_AREA_SIZE); oob_size = readl(denali_flash_reg + DEVICE_SPARE_AREA_SIZE); pages_per_block = readl(denali_flash_reg + PAGES_PER_BLOCK);
/* Do as denali_hw_init() does. */writel(CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES,denali_flash_reg + SPARE_AREA_SKIP_BYTES);writel(0x0F, denali_flash_reg + RB_PIN_ENABLED);writel(CHIP_EN_DONT_CARE__FLAG, denali_flash_reg + CHIP_ENABLE_DONT_CARE);writel(0xffff, denali_flash_reg + SPARE_AREA_MARKER);You put this code just because you found it worked for your board.
I'm not even sure what to respond to this. Yes, I put this code here because it initializes the NAND controller fully and yes, I tested it on actual physical SoCFPGA hardware.
If you reset the NAND controller, not only these four, but all the register values are lost. Especially, page_size, oob_size, etc. https://github.com/u-boot/u-boot/blob/v2020.01/drivers/mtd/nand/raw/denali_s...
Nope, those are correct on SoCFPGA.
You are seeing values re-initialized by the controller.
Maybe, you do not know this IP.
Sadly, the Socionext SoC documentation is not publicly available and no development kit with NAND can be obtained, hence yes, I do not know how the IP works on Socionext SoCs.
I only know what is written in the publicly available Altera documentation and how NAND works on Altera SoCs.
[...]
It is working on your board because SOCFPGA enables the reset sequencer, which enumerates the nand chip by hardware. It is not necessarily true for other platforms, like the UniPhier platform.
So Uniphier has a different behavior, fine, shall I put ifdef around this then ? Or what is it that you want ?
I do not want to see this patch in upstream.
I see, but then that does not permit my hardware to work, so we need to find some solution.
Currently, CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES is only used for U-Boot proper.
I'd like to back-port http://patchwork.ozlabs.org/patch/1214018/ when it is accepted in Linux, then delete CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES entirely.
This patch is adding it to SPL.
CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES will stay if this patch gets in.
Not necessarily, you can just program the skip bytes register with default value depending on the platform, just like Linux does.
register = is_socfpga() ? 2 : 8;
The reliable way for full initialization is to call nand_scan_ident(), which is too big for SPL.
Right, so not an option.
To sum up, this is not a proper approach. Do not reset the NAND controller in SPL.
This is not a proper approach, not resetting hardware would mean you end up with hardware in undefined state.
It's working in the state defined by the boot ROM. The boot ROM can read pages. So, SPL will be able to do it in the same manner.
This is where you are wrong.
The SoCFPGA has different reset states -- cold and warm -- warm is the default when resetting out of U-Boot, Linux, etc. Cold reset is generally not used.
In Cold reset / Power-on reset, the NAND IP gets reset, along with other IPs. The BootROM then loads the SPL into SRAM from NAND and starts it.
In Warm reset, the BootROM checks if the SPL in SRAM is valid and if so, jumps directly to it. The boot media is not even accessed. If the boot media is in some inconsistent state, then the first software which tries to access it fails in some obscure way.
Hence, not resetting the NAND in the SPL (and in fact, boot media in general) leads to all kinds of obscure failures. And so, we need to reset the boot media on SoCFPGA, not doing so is not an option.
Note that switching to Cold reset is also not an option, as that has other implications and drawbacks.
Why don't you just use it as-is?
See above.
Keep the working state that has already been set up by the boot ROM.
The state in which the controller is is NOT defined. Resetting it puts it into defined state.
I am fine with putting an ifdef(SOCFPGA) around this code to avoid triggering it on uniphier if that's fine with you.
I am not fine with ugly #ifdef in drivers as you did in the previous 2/3, 3/3.
Do you have a better option for an non-DM SPL driver ?
On Mon, Jan 13, 2020 at 8:11 PM Marek Vasut marex@denx.de wrote:
On 1/10/20 7:26 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 1:05 PM Marek Vasut marex@denx.de wrote:
On 1/10/20 3:45 AM, Masahiro Yamada wrote:
On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut marex@denx.de wrote:
While the Denali NAND is initialized by the BootROM in SPL, there are still a couple of settings which are missing.
This statement is wrong.
While the Denali NAND is initialized by the BootROM, the SOCFPGA SPL calls socfpga_per_reset_all() to put most of peripherals into the reset state. So, all the register values are lost.
And that is fine, resetting hardware to put it into defined state is what must happen, otherwise we would have all kinds of obscure semi-defined states.
These can trigger subtle corruption of the data read out of the NAND. Fill these settings in just like they are filled in by the full Denali NAND driver in denali_hw_init().
Signed-off-by: Marek Vasut marex@denx.de Cc: Masahiro Yamada yamada.masahiro@socionext.com
drivers/mtd/nand/raw/denali_spl.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/mtd/nand/raw/denali_spl.c b/drivers/mtd/nand/raw/denali_spl.c index dbaba3cab2..b8b29812aa 100644 --- a/drivers/mtd/nand/raw/denali_spl.c +++ b/drivers/mtd/nand/raw/denali_spl.c @@ -173,6 +173,13 @@ void nand_init(void) page_size = readl(denali_flash_reg + DEVICE_MAIN_AREA_SIZE); oob_size = readl(denali_flash_reg + DEVICE_SPARE_AREA_SIZE); pages_per_block = readl(denali_flash_reg + PAGES_PER_BLOCK);
/* Do as denali_hw_init() does. */writel(CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES,denali_flash_reg + SPARE_AREA_SKIP_BYTES);writel(0x0F, denali_flash_reg + RB_PIN_ENABLED);writel(CHIP_EN_DONT_CARE__FLAG, denali_flash_reg + CHIP_ENABLE_DONT_CARE);writel(0xffff, denali_flash_reg + SPARE_AREA_MARKER);You put this code just because you found it worked for your board.
I'm not even sure what to respond to this. Yes, I put this code here because it initializes the NAND controller fully and yes, I tested it on actual physical SoCFPGA hardware.
If you reset the NAND controller, not only these four, but all the register values are lost. Especially, page_size, oob_size, etc. https://github.com/u-boot/u-boot/blob/v2020.01/drivers/mtd/nand/raw/denali_s...
Nope, those are correct on SoCFPGA.
You are seeing values re-initialized by the controller.
Maybe, you do not know this IP.
Sadly, the Socionext SoC documentation is not publicly available and no development kit with NAND can be obtained, hence yes, I do not know how the IP works on Socionext SoCs.
I am not talking about the Socionext SoCs.
I am talking about the Denali IP, which was originally developed by Denali, now provided by Cadence. (since Denali was acquired by Cadence)
What I said was written in the "Denali NAND Flash Memory Controller User's Guide" which was released by Denali/Cadence.
I only know what is written in the publicly available Altera documentation and how NAND works on Altera SoCs.
Right. Altera's SOCFPGA does not describe all the features of this IP.
To understand this IP fully, you need to read the IP datasheet, which is not publicly available, unfortunately.
Socionext (and also Altera/Intel) has access to the IP datasheet.
[...]
It is working on your board because SOCFPGA enables the reset sequencer, which enumerates the nand chip by hardware. It is not necessarily true for other platforms, like the UniPhier platform.
So Uniphier has a different behavior, fine, shall I put ifdef around this then ? Or what is it that you want ?
I do not want to see this patch in upstream.
I see, but then that does not permit my hardware to work, so we need to find some solution.
OK, see below.
Currently, CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES is only used for U-Boot proper.
I'd like to back-port http://patchwork.ozlabs.org/patch/1214018/ when it is accepted in Linux, then delete CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES entirely.
This patch is adding it to SPL.
CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES will stay if this patch gets in.
Not necessarily, you can just program the skip bytes register with default value depending on the platform, just like Linux does.
register = is_socfpga() ? 2 : 8;
The reliable way for full initialization is to call nand_scan_ident(), which is too big for SPL.
Right, so not an option.
To sum up, this is not a proper approach. Do not reset the NAND controller in SPL.
This is not a proper approach, not resetting hardware would mean you end up with hardware in undefined state.
It's working in the state defined by the boot ROM. The boot ROM can read pages. So, SPL will be able to do it in the same manner.
This is where you are wrong.
The SoCFPGA has different reset states -- cold and warm -- warm is the default when resetting out of U-Boot, Linux, etc. Cold reset is generally not used.
In Cold reset / Power-on reset, the NAND IP gets reset, along with other IPs. The BootROM then loads the SPL into SRAM from NAND and starts it.
In Warm reset, the BootROM checks if the SPL in SRAM is valid and if so, jumps directly to it. The boot media is not even accessed. If the boot media is in some inconsistent state, then the first software which tries to access it fails in some obscure way.
Until I read this email, I did not notice the fact you were talking about the warm reset, which completely skips the NAND controller access.
Your commit description (both v1 and v2) starts like this:
"While the Denali NAND is initialized by the BootROM in SPL, ..." ^^^^^^^^^^^^^^^^^^^^^^^^^^
Now I finally understood what issue you want to solve.
Could you rephrase the commit description please? I could not read the warm boot stuff from the commit log.
FWIW, the following is what I understood so far. (Please modify as needed, or you can write it up from scratch.)
" Currently, denali_spl does not set up the registers because it expects the controller has been initialized by the Boot ROM.
It is true for the ARM-v7 generation SoCs of UniPhier platform, which only supports the cold boot. So, UniPhier SPL uses the NAND controller without resetting it.
For SOCFPGA, we need to support not only the cold boot, but also the warm boot, where Boot ROM may entirely skip the boot media access.
To avoid all kinds of obscure failures caused by undefined hardware state, SOCFPGA SPL puts most of peripherals into the reset state by calling socfpga_per_reset(), and then de-asserts the reset of NAND controller if the system is booting up from the NAND device.
Most of registers are self-initialized by the NAND controller because the Denali NAND IP supports the HW-controlled bootstrap. However, some registers must be explicitly set up by software. Currently, this part is missing in nand_spl. "
Hence, not resetting the NAND in the SPL (and in fact, boot media in general) leads to all kinds of obscure failures. And so, we need to reset the boot media on SoCFPGA, not doing so is not an option.
Note that switching to Cold reset is also not an option, as that has other implications and drawbacks.
Why don't you just use it as-is?
See above.
Keep the working state that has already been set up by the boot ROM.
The state in which the controller is is NOT defined. Resetting it puts it into defined state.
I am fine with putting an ifdef(SOCFPGA) around this code to avoid triggering it on uniphier if that's fine with you.
I am not fine with ugly #ifdef in drivers as you did in the previous 2/3, 3/3.
Do you have a better option for an non-DM SPL driver ?
Now I am convinced that this change is needed, but I'd like the commit log to describe the motivation of this change.
Thanks.
-- Best regards, Marek Vasut
-- Best Regards Masahiro Yamada
On 1/14/20 7:55 AM, Masahiro Yamada wrote:
Hi,
[...]
I only know what is written in the publicly available Altera documentation and how NAND works on Altera SoCs.
Right. Altera's SOCFPGA does not describe all the features of this IP.
To understand this IP fully, you need to read the IP datasheet, which is not publicly available, unfortunately.
Socionext (and also Altera/Intel) has access to the IP datasheet.
Great, that's not really helpful from the silicon vendor side :(
[...]
It is working on your board because SOCFPGA enables the reset sequencer, which enumerates the nand chip by hardware. It is not necessarily true for other platforms, like the UniPhier platform.
So Uniphier has a different behavior, fine, shall I put ifdef around this then ? Or what is it that you want ?
I do not want to see this patch in upstream.
I see, but then that does not permit my hardware to work, so we need to find some solution.
OK, see below.
OK
Currently, CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES is only used for U-Boot proper.
I'd like to back-port http://patchwork.ozlabs.org/patch/1214018/ when it is accepted in Linux, then delete CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES entirely.
This patch is adding it to SPL.
CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES will stay if this patch gets in.
Not necessarily, you can just program the skip bytes register with default value depending on the platform, just like Linux does.
register = is_socfpga() ? 2 : 8;
The reliable way for full initialization is to call nand_scan_ident(), which is too big for SPL.
Right, so not an option.
To sum up, this is not a proper approach. Do not reset the NAND controller in SPL.
This is not a proper approach, not resetting hardware would mean you end up with hardware in undefined state.
It's working in the state defined by the boot ROM. The boot ROM can read pages. So, SPL will be able to do it in the same manner.
This is where you are wrong.
The SoCFPGA has different reset states -- cold and warm -- warm is the default when resetting out of U-Boot, Linux, etc. Cold reset is generally not used.
In Cold reset / Power-on reset, the NAND IP gets reset, along with other IPs. The BootROM then loads the SPL into SRAM from NAND and starts it.
In Warm reset, the BootROM checks if the SPL in SRAM is valid and if so, jumps directly to it. The boot media is not even accessed. If the boot media is in some inconsistent state, then the first software which tries to access it fails in some obscure way.
Until I read this email, I did not notice the fact you were talking about the warm reset, which completely skips the NAND controller access.
Good, so now we're back on track.
However, note that the SPL always resets the NAND IP, both for warm reset and cold reset condition. Hence the registers must also always be reprogrammed.
Your commit description (both v1 and v2) starts like this:
"While the Denali NAND is initialized by the BootROM in SPL, ..." ^^^^^^^^^^^^^^^^^^^^^^^^^^
Now I finally understood what issue you want to solve.
Could you rephrase the commit description please? I could not read the warm boot stuff from the commit log.
FWIW, the following is what I understood so far. (Please modify as needed, or you can write it up from scratch.)
I will reword it.
" Currently, denali_spl does not set up the registers because it expects the controller has been initialized by the Boot ROM.
It is true for the ARM-v7 generation SoCs of UniPhier platform, which only supports the cold boot. So, UniPhier SPL uses the NAND controller without resetting it.
For SOCFPGA, we need to support not only the cold boot, but also the warm boot, where Boot ROM may entirely skip the boot media access.
To avoid all kinds of obscure failures caused by undefined hardware state, SOCFPGA SPL puts most of peripherals into the reset state by calling socfpga_per_reset(), and then de-asserts the reset of NAND controller if the system is booting up from the NAND device.
Most of registers are self-initialized by the NAND controller because the Denali NAND IP supports the HW-controlled bootstrap. However, some registers must be explicitly set up by software. Currently, this part is missing in nand_spl. "
Yes
Hence, not resetting the NAND in the SPL (and in fact, boot media in general) leads to all kinds of obscure failures. And so, we need to reset the boot media on SoCFPGA, not doing so is not an option.
Note that switching to Cold reset is also not an option, as that has other implications and drawbacks.
Why don't you just use it as-is?
See above.
Keep the working state that has already been set up by the boot ROM.
The state in which the controller is is NOT defined. Resetting it puts it into defined state.
I am fine with putting an ifdef(SOCFPGA) around this code to avoid triggering it on uniphier if that's fine with you.
I am not fine with ugly #ifdef in drivers as you did in the previous 2/3, 3/3.
Do you have a better option for an non-DM SPL driver ?
Now I am convinced that this change is needed, but I'd like the commit log to describe the motivation of this change.
I will do that.
participants (2)
-
Marek Vasut -
Masahiro Yamada