| Message ID | 20200110001417.82917-3-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [1/3] mtd: rawnand: denali-spl: Add missing hardware init | expand |
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 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 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
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)
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(-)