All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
@ 2020-01-09 10:07 Marek Vasut
  2020-01-09 10:07 ` [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver Marek Vasut
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Marek Vasut @ 2020-01-09 10:07 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver
  2020-01-09 10:07 [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init Marek Vasut
@ 2020-01-09 10:07 ` Marek Vasut
  2020-01-09 11:02   ` Masahiro Yamada
  2020-01-09 10:07 ` [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA Marek Vasut
  2020-01-09 11:29 ` [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init Masahiro Yamada
  2 siblings, 1 reply; 22+ messages in thread
From: Marek Vasut @ 2020-01-09 10:07 UTC (permalink / raw)
  To: u-boot

The SoCFPGA Gen5 does not have a clock driver yet, let the NAND driver
work without a clock driver by falling back to the default frequencies.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
---
 drivers/mtd/nand/raw/denali_dt.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index 0ce81324b9..2c9e249ab6 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -62,7 +62,6 @@ 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 resource res;
 	int ret;
 
@@ -87,11 +86,14 @@ static int denali_dt_probe(struct udevice *dev)
 
 	denali->host = devm_ioremap(dev, res.start, resource_size(&res));
 
+#if CONFIG_IS_ENABLED(CLK)
+	struct clk clk, clk_x, clk_ecc;
+
 	ret = clk_get_by_name(dev, "nand", &clk);
 	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)
@@ -117,10 +119,12 @@ static int denali_dt_probe(struct udevice *dev)
 			return ret;
 	}
 
-	if (clk_x.dev) {
+	if (clk.dev && clk_x.dev) {
 		denali->clk_rate = clk_get_rate(&clk);
 		denali->clk_x_rate = clk_get_rate(&clk_x);
-	} else {
+	} else
+#endif
+	{
 		/*
 		 * Hardcode the clock rates for the backward compatibility.
 		 * This works for both SOCFPGA and UniPhier.
-- 
2.24.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA
  2020-01-09 10:07 [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init Marek Vasut
  2020-01-09 10:07 ` [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver Marek Vasut
@ 2020-01-09 10:07 ` Marek Vasut
  2020-01-09 11:04   ` Masahiro Yamada
  2020-01-09 11:29 ` [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init Masahiro Yamada
  2 siblings, 1 reply; 22+ messages in thread
From: Marek Vasut @ 2020-01-09 10:07 UTC (permalink / raw)
  To: u-boot

Legacy kernel versions for SoCFPGA may not implement proper reset
handling. Apply the same approach as SoCFPGA reset driver, check
environment variable "socfpga_legacy_reset_compat", and if it is
set, do not reset the IP before booting Linux. This way, even the
older kernel versions can be booted by up to date U-Boot.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
---
 drivers/mtd/nand/raw/denali_dt.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index 2c9e249ab6..d35f2a3543 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -148,6 +148,18 @@ static int denali_dt_remove(struct udevice *dev)
 {
 	struct denali_nand_info *denali = dev_get_priv(dev);
 
+#if CONFIG_IS_ENABLED(ARCH_SOCFPGA)
+	/*
+	 * Legacy kernel versions do not implement proper reset handling on
+	 * SoCFPGA. To let those older kernel versions work, reuse the same
+	 * approach as the SoCFPGA reset driver does -- check environment
+	 * variable socfpga_legacy_reset_compat and avoid resetting the IP
+	 * before booting the kernel if it is set to 1.
+	 */
+	if (env_get_ulong("socfpga_legacy_reset_compat", 10, 0))
+		return 0;
+#endif
+
 	return reset_release_bulk(&denali->resets);
 }
 
-- 
2.24.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver
  2020-01-09 10:07 ` [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver Marek Vasut
@ 2020-01-09 11:02   ` Masahiro Yamada
  2020-01-09 11:14     ` Marek Vasut
  0 siblings, 1 reply; 22+ messages in thread
From: Masahiro Yamada @ 2020-01-09 11:02 UTC (permalink / raw)
  To: u-boot

On Thu, Jan 9, 2020 at 7:08 PM Marek Vasut <marex@denx.de> wrote:
>
> The SoCFPGA Gen5 does not have a clock driver yet, let the NAND driver
> work without a clock driver by falling back to the default frequencies.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>


Why do you need this?

denali_dt_probe() succeeds without CONFIG_CLK, doesn't it?



> ---
>  drivers/mtd/nand/raw/denali_dt.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
> index 0ce81324b9..2c9e249ab6 100644
> --- a/drivers/mtd/nand/raw/denali_dt.c
> +++ b/drivers/mtd/nand/raw/denali_dt.c
> @@ -62,7 +62,6 @@ 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 resource res;
>         int ret;
>
> @@ -87,11 +86,14 @@ static int denali_dt_probe(struct udevice *dev)
>
>         denali->host = devm_ioremap(dev, res.start, resource_size(&res));
>
> +#if CONFIG_IS_ENABLED(CLK)
> +       struct clk clk, clk_x, clk_ecc;
> +
>         ret = clk_get_by_name(dev, "nand", &clk);
>         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)
> @@ -117,10 +119,12 @@ static int denali_dt_probe(struct udevice *dev)
>                         return ret;
>         }
>
> -       if (clk_x.dev) {
> +       if (clk.dev && clk_x.dev) {
>                 denali->clk_rate = clk_get_rate(&clk);
>                 denali->clk_x_rate = clk_get_rate(&clk_x);
> -       } else {
> +       } else
> +#endif
> +       {
>                 /*
>                  * Hardcode the clock rates for the backward compatibility.
>                  * This works for both SOCFPGA and UniPhier.
> --
> 2.24.1
>


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA
  2020-01-09 10:07 ` [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA Marek Vasut
@ 2020-01-09 11:04   ` Masahiro Yamada
  2020-01-09 11:15     ` Marek Vasut
  0 siblings, 1 reply; 22+ messages in thread
From: Masahiro Yamada @ 2020-01-09 11:04 UTC (permalink / raw)
  To: u-boot

On Thu, Jan 9, 2020 at 7:08 PM Marek Vasut <marex@denx.de> wrote:
>
> Legacy kernel versions for SoCFPGA may not implement proper reset
> handling. Apply the same approach as SoCFPGA reset driver, check
> environment variable "socfpga_legacy_reset_compat", and if it is
> set, do not reset the IP before booting Linux. This way, even the
> older kernel versions can be booted by up to date U-Boot.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>  drivers/mtd/nand/raw/denali_dt.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
> index 2c9e249ab6..d35f2a3543 100644
> --- a/drivers/mtd/nand/raw/denali_dt.c
> +++ b/drivers/mtd/nand/raw/denali_dt.c
> @@ -148,6 +148,18 @@ static int denali_dt_remove(struct udevice *dev)
>  {
>         struct denali_nand_info *denali = dev_get_priv(dev);
>
> +#if CONFIG_IS_ENABLED(ARCH_SOCFPGA)
> +       /*
> +        * Legacy kernel versions do not implement proper reset handling on
> +        * SoCFPGA. To let those older kernel versions work, reuse the same
> +        * approach as the SoCFPGA reset driver does -- check environment
> +        * variable socfpga_legacy_reset_compat and avoid resetting the IP
> +        * before booting the kernel if it is set to 1.
> +        */
> +       if (env_get_ulong("socfpga_legacy_reset_compat", 10, 0))
> +               return 0;
> +#endif
> +
>         return reset_release_bulk(&denali->resets);
>  }


Why don't you simply remove reset_release_bulk() ?

>
> --
> 2.24.1
>


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver
  2020-01-09 11:02   ` Masahiro Yamada
@ 2020-01-09 11:14     ` Marek Vasut
  2020-01-09 12:04       ` Masahiro Yamada
  0 siblings, 1 reply; 22+ messages in thread
From: Marek Vasut @ 2020-01-09 11:14 UTC (permalink / raw)
  To: u-boot

On 1/9/20 12:02 PM, Masahiro Yamada wrote:
> On Thu, Jan 9, 2020 at 7:08 PM Marek Vasut <marex@denx.de> wrote:
>>
>> The SoCFPGA Gen5 does not have a clock driver yet, let the NAND driver
>> work without a clock driver by falling back to the default frequencies.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> 
> 
> Why do you need this?
> 
> denali_dt_probe() succeeds without CONFIG_CLK, doesn't it?

No, if you don't have the NAND clock, this fails.
This could be reverted once there is DM/DT capable clock driver for
SoCFPGA Gen5, until then this is needed.

[...]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA
  2020-01-09 11:04   ` Masahiro Yamada
@ 2020-01-09 11:15     ` Marek Vasut
  2020-01-09 12:11       ` Masahiro Yamada
  0 siblings, 1 reply; 22+ messages in thread
From: Marek Vasut @ 2020-01-09 11:15 UTC (permalink / raw)
  To: u-boot

On 1/9/20 12:04 PM, Masahiro Yamada wrote:
> On Thu, Jan 9, 2020 at 7:08 PM Marek Vasut <marex@denx.de> wrote:
>>
>> Legacy kernel versions for SoCFPGA may not implement proper reset
>> handling. Apply the same approach as SoCFPGA reset driver, check
>> environment variable "socfpga_legacy_reset_compat", and if it is
>> set, do not reset the IP before booting Linux. This way, even the
>> older kernel versions can be booted by up to date U-Boot.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>  drivers/mtd/nand/raw/denali_dt.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
>> index 2c9e249ab6..d35f2a3543 100644
>> --- a/drivers/mtd/nand/raw/denali_dt.c
>> +++ b/drivers/mtd/nand/raw/denali_dt.c
>> @@ -148,6 +148,18 @@ static int denali_dt_remove(struct udevice *dev)
>>  {
>>         struct denali_nand_info *denali = dev_get_priv(dev);
>>
>> +#if CONFIG_IS_ENABLED(ARCH_SOCFPGA)
>> +       /*
>> +        * Legacy kernel versions do not implement proper reset handling on
>> +        * SoCFPGA. To let those older kernel versions work, reuse the same
>> +        * approach as the SoCFPGA reset driver does -- check environment
>> +        * variable socfpga_legacy_reset_compat and avoid resetting the IP
>> +        * before booting the kernel if it is set to 1.
>> +        */
>> +       if (env_get_ulong("socfpga_legacy_reset_compat", 10, 0))
>> +               return 0;
>> +#endif
>> +
>>         return reset_release_bulk(&denali->resets);
>>  }
> 
> 
> Why don't you simply remove reset_release_bulk() ?

Because once mainline has proper reset handling, this can be used only
for legacy kernel versions.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
  2020-01-09 10:07 [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init Marek Vasut
  2020-01-09 10:07 ` [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver Marek Vasut
  2020-01-09 10:07 ` [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA Marek Vasut
@ 2020-01-09 11:29 ` Masahiro Yamada
  2020-01-09 13:18   ` Marek Vasut
  2 siblings, 1 reply; 22+ messages in thread
From: Masahiro Yamada @ 2020-01-09 11:29 UTC (permalink / raw)
  To: u-boot

On Thu, Jan 9, 2020 at 7:08 PM 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. 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 need this because SOCFPGA SPL resets the nand controller, correct?

https://github.com/u-boot/u-boot/blob/v2020.01/arch/arm/mach-socfpga/spl_gen5.c#L89




--
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver
  2020-01-09 11:14     ` Marek Vasut
@ 2020-01-09 12:04       ` Masahiro Yamada
  2020-01-09 13:21         ` Marek Vasut
  0 siblings, 1 reply; 22+ messages in thread
From: Masahiro Yamada @ 2020-01-09 12:04 UTC (permalink / raw)
  To: u-boot

On Thu, Jan 9, 2020 at 8:16 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/9/20 12:02 PM, Masahiro Yamada wrote:
> > On Thu, Jan 9, 2020 at 7:08 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> The SoCFPGA Gen5 does not have a clock driver yet, let the NAND driver
> >> work without a clock driver by falling back to the default frequencies.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> >
> >
> > Why do you need this?
> >
> > denali_dt_probe() succeeds without CONFIG_CLK, doesn't it?
>
> No, if you don't have the NAND clock, this fails.
> This could be reverted once there is DM/DT capable clock driver for
> SoCFPGA Gen5, until then this is needed.

Oh, sorry, I read the code in a wrong way.
But, this ifdef is ugly.

How about this alternative one?
http://patchwork.ozlabs.org/patch/1220337/



--
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA
  2020-01-09 11:15     ` Marek Vasut
@ 2020-01-09 12:11       ` Masahiro Yamada
  2020-01-09 14:25         ` Marek Vasut
  0 siblings, 1 reply; 22+ messages in thread
From: Masahiro Yamada @ 2020-01-09 12:11 UTC (permalink / raw)
  To: u-boot

On Thu, Jan 9, 2020 at 8:16 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/9/20 12:04 PM, Masahiro Yamada wrote:
> > On Thu, Jan 9, 2020 at 7:08 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> Legacy kernel versions for SoCFPGA may not implement proper reset
> >> handling. Apply the same approach as SoCFPGA reset driver, check
> >> environment variable "socfpga_legacy_reset_compat", and if it is
> >> set, do not reset the IP before booting Linux. This way, even the
> >> older kernel versions can be booted by up to date U-Boot.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> ---
> >>  drivers/mtd/nand/raw/denali_dt.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
> >> index 2c9e249ab6..d35f2a3543 100644
> >> --- a/drivers/mtd/nand/raw/denali_dt.c
> >> +++ b/drivers/mtd/nand/raw/denali_dt.c
> >> @@ -148,6 +148,18 @@ static int denali_dt_remove(struct udevice *dev)
> >>  {
> >>         struct denali_nand_info *denali = dev_get_priv(dev);
> >>
> >> +#if CONFIG_IS_ENABLED(ARCH_SOCFPGA)
> >> +       /*
> >> +        * Legacy kernel versions do not implement proper reset handling on
> >> +        * SoCFPGA. To let those older kernel versions work, reuse the same
> >> +        * approach as the SoCFPGA reset driver does -- check environment
> >> +        * variable socfpga_legacy_reset_compat and avoid resetting the IP
> >> +        * before booting the kernel if it is set to 1.
> >> +        */
> >> +       if (env_get_ulong("socfpga_legacy_reset_compat", 10, 0))
> >> +               return 0;
> >> +#endif
> >> +
> >>         return reset_release_bulk(&denali->resets);
> >>  }
> >
> >
> > Why don't you simply remove reset_release_bulk() ?
>
> Because once mainline has proper reset handling, this can be used only
> for legacy kernel versions.


I am suffering for the same problem for the uniphier platform.

The reset driver support for Linux will never be back-ported.
So, it will continue to be a problem for Linux v4.19, v5.4, etc.

reset_release_bulk() should go away.


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
  2020-01-09 11:29 ` [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init Masahiro Yamada
@ 2020-01-09 13:18   ` Marek Vasut
  0 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2020-01-09 13:18 UTC (permalink / raw)
  To: u-boot

On 1/9/20 12:29 PM, Masahiro Yamada wrote:
> On Thu, Jan 9, 2020 at 7:08 PM 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. 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 need this because SOCFPGA SPL resets the nand controller, correct?
> 
> https://github.com/u-boot/u-boot/blob/v2020.01/arch/arm/mach-socfpga/spl_gen5.c#L89

Yes. The driver should init the hardware correctly and fully, which it
does not do without this patch.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver
  2020-01-09 12:04       ` Masahiro Yamada
@ 2020-01-09 13:21         ` Marek Vasut
  0 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2020-01-09 13:21 UTC (permalink / raw)
  To: u-boot

On 1/9/20 1:04 PM, Masahiro Yamada wrote:
> On Thu, Jan 9, 2020 at 8:16 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/9/20 12:02 PM, Masahiro Yamada wrote:
>>> On Thu, Jan 9, 2020 at 7:08 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> The SoCFPGA Gen5 does not have a clock driver yet, let the NAND driver
>>>> work without a clock driver by falling back to the default frequencies.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>
>>>
>>> Why do you need this?
>>>
>>> denali_dt_probe() succeeds without CONFIG_CLK, doesn't it?
>>
>> No, if you don't have the NAND clock, this fails.
>> This could be reverted once there is DM/DT capable clock driver for
>> SoCFPGA Gen5, until then this is needed.
> 
> Oh, sorry, I read the code in a wrong way.
> But, this ifdef is ugly.
> 
> How about this alternative one?
> http://patchwork.ozlabs.org/patch/1220337/

The clock code takes space for no reason in that patch, I prefer this
one to keep the size in check.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA
  2020-01-09 12:11       ` Masahiro Yamada
@ 2020-01-09 14:25         ` Marek Vasut
  2020-01-09 15:01           ` Masahiro Yamada
  0 siblings, 1 reply; 22+ messages in thread
From: Marek Vasut @ 2020-01-09 14:25 UTC (permalink / raw)
  To: u-boot

On 1/9/20 1:11 PM, Masahiro Yamada wrote:
> On Thu, Jan 9, 2020 at 8:16 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/9/20 12:04 PM, Masahiro Yamada wrote:
>>> On Thu, Jan 9, 2020 at 7:08 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> Legacy kernel versions for SoCFPGA may not implement proper reset
>>>> handling. Apply the same approach as SoCFPGA reset driver, check
>>>> environment variable "socfpga_legacy_reset_compat", and if it is
>>>> set, do not reset the IP before booting Linux. This way, even the
>>>> older kernel versions can be booted by up to date U-Boot.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>> ---
>>>>  drivers/mtd/nand/raw/denali_dt.c | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
>>>> index 2c9e249ab6..d35f2a3543 100644
>>>> --- a/drivers/mtd/nand/raw/denali_dt.c
>>>> +++ b/drivers/mtd/nand/raw/denali_dt.c
>>>> @@ -148,6 +148,18 @@ static int denali_dt_remove(struct udevice *dev)
>>>>  {
>>>>         struct denali_nand_info *denali = dev_get_priv(dev);
>>>>
>>>> +#if CONFIG_IS_ENABLED(ARCH_SOCFPGA)
>>>> +       /*
>>>> +        * Legacy kernel versions do not implement proper reset handling on
>>>> +        * SoCFPGA. To let those older kernel versions work, reuse the same
>>>> +        * approach as the SoCFPGA reset driver does -- check environment
>>>> +        * variable socfpga_legacy_reset_compat and avoid resetting the IP
>>>> +        * before booting the kernel if it is set to 1.
>>>> +        */
>>>> +       if (env_get_ulong("socfpga_legacy_reset_compat", 10, 0))
>>>> +               return 0;
>>>> +#endif
>>>> +
>>>>         return reset_release_bulk(&denali->resets);
>>>>  }
>>>
>>>
>>> Why don't you simply remove reset_release_bulk() ?
>>
>> Because once mainline has proper reset handling, this can be used only
>> for legacy kernel versions.
> 
> 
> I am suffering for the same problem for the uniphier platform.

The Denali NAND patches for Linux are currently stuck on your review too.

> The reset driver support for Linux will never be back-ported.
> So, it will continue to be a problem for Linux v4.19, v5.4, etc.
> 
> reset_release_bulk() should go away.

Once the Linux patches are upstream, we can just reset the NAND
controller without problems. And that's the right thing to do, sometimes
you don't want NAND to be available in Linux, and then it should be kept
in reset.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA
  2020-01-09 14:25         ` Marek Vasut
@ 2020-01-09 15:01           ` Masahiro Yamada
  2020-01-10  0:10             ` Marek Vasut
  0 siblings, 1 reply; 22+ messages in thread
From: Masahiro Yamada @ 2020-01-09 15:01 UTC (permalink / raw)
  To: u-boot

On Thu, Jan 9, 2020 at 11:48 PM Marek Vasut <marex@denx.de> wrote:
>
> On 1/9/20 1:11 PM, Masahiro Yamada wrote:
> > On Thu, Jan 9, 2020 at 8:16 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 1/9/20 12:04 PM, Masahiro Yamada wrote:
> >>> On Thu, Jan 9, 2020 at 7:08 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> Legacy kernel versions for SoCFPGA may not implement proper reset
> >>>> handling. Apply the same approach as SoCFPGA reset driver, check
> >>>> environment variable "socfpga_legacy_reset_compat", and if it is
> >>>> set, do not reset the IP before booting Linux. This way, even the
> >>>> older kernel versions can be booted by up to date U-Boot.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> >>>> ---
> >>>>  drivers/mtd/nand/raw/denali_dt.c | 12 ++++++++++++
> >>>>  1 file changed, 12 insertions(+)
> >>>>
> >>>> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
> >>>> index 2c9e249ab6..d35f2a3543 100644
> >>>> --- a/drivers/mtd/nand/raw/denali_dt.c
> >>>> +++ b/drivers/mtd/nand/raw/denali_dt.c
> >>>> @@ -148,6 +148,18 @@ static int denali_dt_remove(struct udevice *dev)
> >>>>  {
> >>>>         struct denali_nand_info *denali = dev_get_priv(dev);
> >>>>
> >>>> +#if CONFIG_IS_ENABLED(ARCH_SOCFPGA)
> >>>> +       /*
> >>>> +        * Legacy kernel versions do not implement proper reset handling on
> >>>> +        * SoCFPGA. To let those older kernel versions work, reuse the same
> >>>> +        * approach as the SoCFPGA reset driver does -- check environment
> >>>> +        * variable socfpga_legacy_reset_compat and avoid resetting the IP
> >>>> +        * before booting the kernel if it is set to 1.
> >>>> +        */
> >>>> +       if (env_get_ulong("socfpga_legacy_reset_compat", 10, 0))
> >>>> +               return 0;
> >>>> +#endif
> >>>> +
> >>>>         return reset_release_bulk(&denali->resets);
> >>>>  }
> >>>
> >>>
> >>> Why don't you simply remove reset_release_bulk() ?
> >>
> >> Because once mainline has proper reset handling, this can be used only
> >> for legacy kernel versions.
> >
> >
> > I am suffering for the same problem for the uniphier platform.
>
> The Denali NAND patches for Linux are currently stuck on your review too.

Huh?

Which patch in Linux is stuck on my review?



> > The reset driver support for Linux will never be back-ported.
> > So, it will continue to be a problem for Linux v4.19, v5.4, etc.
> >
> > reset_release_bulk() should go away.
>
> Once the Linux patches are upstream, we can just reset the NAND
> controller without problems. And that's the right thing to do, sometimes
> you don't want NAND to be available in Linux, and then it should be kept
> in reset.


The NAND reset in Linux will be available in the future version
(if my patch is applied).

The stable kernels are maintained in 6 years.

https://www.kernel.org/category/releases.html

The EOF of Linux 5.4 is set at 2021.12, but
following the recent rules, it will be extended
to 2026 or so.



-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA
  2020-01-09 15:01           ` Masahiro Yamada
@ 2020-01-10  0:10             ` Marek Vasut
  0 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2020-01-10  0:10 UTC (permalink / raw)
  To: u-boot

On 1/9/20 4:01 PM, Masahiro Yamada wrote:
> On Thu, Jan 9, 2020 at 11:48 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/9/20 1:11 PM, Masahiro Yamada wrote:
>>> On Thu, Jan 9, 2020 at 8:16 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 1/9/20 12:04 PM, Masahiro Yamada wrote:
>>>>> On Thu, Jan 9, 2020 at 7:08 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> Legacy kernel versions for SoCFPGA may not implement proper reset
>>>>>> handling. Apply the same approach as SoCFPGA reset driver, check
>>>>>> environment variable "socfpga_legacy_reset_compat", and if it is
>>>>>> set, do not reset the IP before booting Linux. This way, even the
>>>>>> older kernel versions can be booted by up to date U-Boot.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>>> ---
>>>>>>  drivers/mtd/nand/raw/denali_dt.c | 12 ++++++++++++
>>>>>>  1 file changed, 12 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
>>>>>> index 2c9e249ab6..d35f2a3543 100644
>>>>>> --- a/drivers/mtd/nand/raw/denali_dt.c
>>>>>> +++ b/drivers/mtd/nand/raw/denali_dt.c
>>>>>> @@ -148,6 +148,18 @@ static int denali_dt_remove(struct udevice *dev)
>>>>>>  {
>>>>>>         struct denali_nand_info *denali = dev_get_priv(dev);
>>>>>>
>>>>>> +#if CONFIG_IS_ENABLED(ARCH_SOCFPGA)
>>>>>> +       /*
>>>>>> +        * Legacy kernel versions do not implement proper reset handling on
>>>>>> +        * SoCFPGA. To let those older kernel versions work, reuse the same
>>>>>> +        * approach as the SoCFPGA reset driver does -- check environment
>>>>>> +        * variable socfpga_legacy_reset_compat and avoid resetting the IP
>>>>>> +        * before booting the kernel if it is set to 1.
>>>>>> +        */
>>>>>> +       if (env_get_ulong("socfpga_legacy_reset_compat", 10, 0))
>>>>>> +               return 0;
>>>>>> +#endif
>>>>>> +
>>>>>>         return reset_release_bulk(&denali->resets);
>>>>>>  }
>>>>>
>>>>>
>>>>> Why don't you simply remove reset_release_bulk() ?
>>>>
>>>> Because once mainline has proper reset handling, this can be used only
>>>> for legacy kernel versions.
>>>
>>>
>>> I am suffering for the same problem for the uniphier platform.
>>
>> The Denali NAND patches for Linux are currently stuck on your review too.
> 
> Huh?
> 
> Which patch in Linux is stuck on my review?

[PATCH V2] mtd: rawnand: denali_dt: Add support for configuring
SPARE_AREA_SKIP_BYTES

>>> The reset driver support for Linux will never be back-ported.
>>> So, it will continue to be a problem for Linux v4.19, v5.4, etc.
>>>
>>> reset_release_bulk() should go away.
>>
>> Once the Linux patches are upstream, we can just reset the NAND
>> controller without problems. And that's the right thing to do, sometimes
>> you don't want NAND to be available in Linux, and then it should be kept
>> in reset.
> 
> 
> The NAND reset in Linux will be available in the future version
> (if my patch is applied).

Which patch ?

> The stable kernels are maintained in 6 years.
> 
> https://www.kernel.org/category/releases.html

How is this relevant ?

> The EOF of Linux 5.4 is set at 2021.12, but
> following the recent rules, it will be extended
> to 2026 or so.

So, for all legacy kernel versions, enable this backward compatibility
thingie.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
  2020-01-14  6:55         ` Masahiro Yamada
@ 2020-01-21 18:54           ` Marek Vasut
  0 siblings, 0 replies; 22+ messages in thread
From: Marek Vasut @ 2020-01-21 18:54 UTC (permalink / raw)
  To: u-boot

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.

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
  2020-01-13 11:05       ` Marek Vasut
@ 2020-01-14  6:55         ` Masahiro Yamada
  2020-01-21 18:54           ` Marek Vasut
  0 siblings, 1 reply; 22+ messages in thread
From: Masahiro Yamada @ 2020-01-14  6:55 UTC (permalink / raw)
  To: u-boot

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_spl.c#L170
> >>
> >> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
  2020-01-10  6:26     ` Masahiro Yamada
@ 2020-01-13 11:05       ` Marek Vasut
  2020-01-14  6:55         ` Masahiro Yamada
  0 siblings, 1 reply; 22+ messages in thread
From: Marek Vasut @ 2020-01-13 11:05 UTC (permalink / raw)
  To: u-boot

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_spl.c#L170
>>
>> 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 ?

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
  2020-01-10  3:52   ` Marek Vasut
@ 2020-01-10  6:26     ` Masahiro Yamada
  2020-01-13 11:05       ` Marek Vasut
  0 siblings, 1 reply; 22+ messages in thread
From: Masahiro Yamada @ 2020-01-10  6:26 UTC (permalink / raw)
  To: u-boot

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_spl.c#L170
>
> 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
  2020-01-10  2:45 ` Masahiro Yamada
@ 2020-01-10  3:52   ` Marek Vasut
  2020-01-10  6:26     ` Masahiro Yamada
  0 siblings, 1 reply; 22+ messages in thread
From: Marek Vasut @ 2020-01-10  3:52 UTC (permalink / raw)
  To: u-boot

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_spl.c#L170

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
  2020-01-10  0:14 Marek Vasut
@ 2020-01-10  2:45 ` Masahiro Yamada
  2020-01-10  3:52   ` Marek Vasut
  0 siblings, 1 reply; 22+ messages in thread
From: Masahiro Yamada @ 2020-01-10  2:45 UTC (permalink / raw)
  To: u-boot

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_spl.c#L170

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
>


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init
@ 2020-01-10  0:14 Marek Vasut
  2020-01-10  2:45 ` Masahiro Yamada
  0 siblings, 1 reply; 22+ messages in thread
From: Marek Vasut @ 2020-01-10  0:14 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-01-21 18:54 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-09 10:07 [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init Marek Vasut
2020-01-09 10:07 ` [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver Marek Vasut
2020-01-09 11:02   ` Masahiro Yamada
2020-01-09 11:14     ` Marek Vasut
2020-01-09 12:04       ` Masahiro Yamada
2020-01-09 13:21         ` Marek Vasut
2020-01-09 10:07 ` [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA Marek Vasut
2020-01-09 11:04   ` Masahiro Yamada
2020-01-09 11:15     ` Marek Vasut
2020-01-09 12:11       ` Masahiro Yamada
2020-01-09 14:25         ` Marek Vasut
2020-01-09 15:01           ` Masahiro Yamada
2020-01-10  0:10             ` Marek Vasut
2020-01-09 11:29 ` [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init Masahiro Yamada
2020-01-09 13:18   ` Marek Vasut
2020-01-10  0:14 Marek Vasut
2020-01-10  2:45 ` Masahiro Yamada
2020-01-10  3:52   ` Marek Vasut
2020-01-10  6:26     ` Masahiro Yamada
2020-01-13 11:05       ` Marek Vasut
2020-01-14  6:55         ` Masahiro Yamada
2020-01-21 18:54           ` Marek Vasut

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.