[U-Boot] [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init

[U-Boot] [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init

[Marek Vasut]

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.0.432.g9d3f5f5b63

[U-Boot] [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver

[Marek Vasut]

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.0.432.g9d3f5f5b63

[U-Boot] [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA

[Marek Vasut]

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.0.432.g9d3f5f5b63

[U-Boot] [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init

[Masahiro Yamada]

On Thu, Nov 21, 2019 at 6:37 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. 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().

I'd like to know this in more detailed.

I assume these registers are set up by the BootROM,
and SPL is supposed to read data in the same manner as the Boot ROM.

In which situation does the data get corrupted?


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

I guess you tested this for SOCFPGA.

Please tell me the value of CONFIG_NAND_DENALI_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.0.432.g9d3f5f5b63
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



--
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA

[Masahiro Yamada]

On Thu, Nov 21, 2019 at 6:38 AM Marek Vasut <marex@denx.de> wrote:
>
> Legacy kernel versions for SoCFPGA may not implement proper reset
> handling.

What is "legacy kernel versions" ?



> 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
> +


How about removing denali_dt_remove() entirely ?


Not only SOCFPGA, but also my boards are broken.

I am really annoyed since the following commit:




commit ed784ac3822b7d7019679a41a17907296e2dadbe
Author:     Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
AuthorDate: Fri Mar 1 20:12:34 2019 +0100
Commit:     Marek Vasut <marex@denx.de>
CommitDate: Wed Apr 17 22:20:16 2019 +0200

    mtd: rawnand: denali: add reset handling

    This adds reset handling to the devicetree-enabled Denali NAND driver.

    For backwards compatibility, only a warning is printed when failing to
    get reset handles.

    Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>



>         return reset_release_bulk(&denali->resets);
>  }
>
> --
> 2.24.0.432.g9d3f5f5b63
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver

[Masahiro Yamada]

On Thu, Nov 21, 2019 at 6:38 AM 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>
> ---
>  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;

This line changes the 'clk' optional.
Do you need additional #if CONFIG_IS_ENABLED(CLK) ?



BTW, in Linux, clk_get() and clk_enable() are no-op
when CONFIG_HAVE_CLK is unset.

The U-Boot's clk_get() returns -ENOSYS, hence
drivers tend to be ugly.
I already stopped caring the U-Boot code, though.



>         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.0.432.g9d3f5f5b63
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



--
Best Regards

Masahiro Yamada

[U-Boot] [PATCH 2/3] mtd: rawnand: denali: Allow operation without clock driver

[Marek Vasut]

On 11/26/19 4:40 AM, Masahiro Yamada wrote:
> On Thu, Nov 21, 2019 at 6:38 AM Marek Vasut 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>
>> ---
>>  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;
> 
> This line changes the 'clk' optional.
> Do you need additional #if CONFIG_IS_ENABLED(CLK) ?

This whole block is in such ifdef.

> BTW, in Linux, clk_get() and clk_enable() are no-op
> when CONFIG_HAVE_CLK is unset.
> 
> The U-Boot's clk_get() returns -ENOSYS, hence
> drivers tend to be ugly.
> I already stopped caring the U-Boot code, though.

Because of the DM ?

[...]

[U-Boot] [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init

[Marek Vasut]

On 11/26/19 3:34 AM, Masahiro Yamada wrote:
> On Thu, Nov 21, 2019 at 6:37 AM Marek Vasut 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().
> 
> I'd like to know this in more detailed.
> 
> I assume these registers are set up by the BootROM,
> and SPL is supposed to read data in the same manner as the Boot ROM.
> 
> In which situation does the data get corrupted?

When I boot the board, these registers are not set up correctly. The
bootrom reads the SPL fine, but the SPL cannot read U-Boot anymore,
probably because the bootrom does something funny to the controller
settings.

>> 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);
> 
> I guess you tested this for SOCFPGA.
> 
> Please tell me the value of CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES.
2

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA

[Marek Vasut]

On 11/26/19 3:47 AM, Masahiro Yamada wrote:
> On Thu, Nov 21, 2019 at 6:38 AM Marek Vasut wrote:
>>
>> Legacy kernel versions for SoCFPGA may not implement proper reset
>> handling.
> 
> What is "legacy kernel versions" ?

Anything older than 5.x , which got proper reset handling, finally.

>> 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
>> +
> 
> 
> How about removing denali_dt_remove() entirely ?

Fine by me.

> Not only SOCFPGA, but also my boards are broken.

How come ?

> I am really annoyed since the following commit:
> 
> 
> 
> 
> commit ed784ac3822b7d7019679a41a17907296e2dadbe
> Author:     Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> AuthorDate: Fri Mar 1 20:12:34 2019 +0100
> Commit:     Marek Vasut <marex@denx.de>
> CommitDate: Wed Apr 17 22:20:16 2019 +0200
> 
>     mtd: rawnand: denali: add reset handling
> 
>     This adds reset handling to the devicetree-enabled Denali NAND driver.
> 
>     For backwards compatibility, only a warning is printed when failing to
>     get reset handles.
> 
>     Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> 
> 
> 
>>         return reset_release_bulk(&denali->resets);
>>  }

CCing Simon.

[U-Boot] [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA

[Masahiro Yamada]

On Tue, Nov 26, 2019 at 5:23 PM Marek Vasut <marex@denx.de> wrote:
>
> On 11/26/19 3:47 AM, Masahiro Yamada wrote:
> > On Thu, Nov 21, 2019 at 6:38 AM Marek Vasut wrote:
> >>
> >> Legacy kernel versions for SoCFPGA may not implement proper reset
> >> handling.
> >
> > What is "legacy kernel versions" ?
>
> Anything older than 5.x , which got proper reset handling, finally.


Really?
Could you tell the commit ID?



>
> >> 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
> >> +
> >
> >
> > How about removing denali_dt_remove() entirely ?
>
> Fine by me.
>
> > Not only SOCFPGA, but also my boards are broken.
>
> How come ?

Because the Linux kernel version I am using
does not have the reset handling for the Denali driver.




>
> > I am really annoyed since the following commit:
> >
> >
> >
> >
> > commit ed784ac3822b7d7019679a41a17907296e2dadbe
> > Author:     Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > AuthorDate: Fri Mar 1 20:12:34 2019 +0100
> > Commit:     Marek Vasut <marex@denx.de>
> > CommitDate: Wed Apr 17 22:20:16 2019 +0200
> >
> >     mtd: rawnand: denali: add reset handling
> >
> >     This adds reset handling to the devicetree-enabled Denali NAND driver.
> >
> >     For backwards compatibility, only a warning is printed when failing to
> >     get reset handles.
> >
> >     Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> >
> >
> >
> >>         return reset_release_bulk(&denali->resets);
> >>  }
>
> CCing Simon.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA

[Simon Goldschmidt]

On Tue, Nov 26, 2019 at 9:22 AM Marek Vasut <marex@denx.de> wrote:
>
> On 11/26/19 3:47 AM, Masahiro Yamada wrote:
> > On Thu, Nov 21, 2019 at 6:38 AM Marek Vasut wrote:
> >>
> >> Legacy kernel versions for SoCFPGA may not implement proper reset
> >> handling.
> >
> > What is "legacy kernel versions" ?
>
> Anything older than 5.x , which got proper reset handling, finally.
>
> >> 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
> >> +
> >
> >
> > How about removing denali_dt_remove() entirely ?
>
> Fine by me.
>
> > Not only SOCFPGA, but also my boards are broken.
>
> How come ?
>
> > I am really annoyed since the following commit:

Sorry to break anything there. I fail to see what's wrong with that commit,
could you tell me more details so we can fix this?

Regards,
Simon

> >
> >
> >
> >
> > commit ed784ac3822b7d7019679a41a17907296e2dadbe
> > Author:     Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > AuthorDate: Fri Mar 1 20:12:34 2019 +0100
> > Commit:     Marek Vasut <marex@denx.de>
> > CommitDate: Wed Apr 17 22:20:16 2019 +0200
> >
> >     mtd: rawnand: denali: add reset handling
> >
> >     This adds reset handling to the devicetree-enabled Denali NAND driver.
> >
> >     For backwards compatibility, only a warning is printed when failing to
> >     get reset handles.
> >
> >     Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> >
> >
> >
> >>         return reset_release_bulk(&denali->resets);
> >>  }
>
> CCing Simon.

[U-Boot] [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA

[Marek Vasut]

On 11/26/19 9:46 AM, Masahiro Yamada wrote:
> On Tue, Nov 26, 2019 at 5:23 PM Marek Vasut wrote:
>>
>> On 11/26/19 3:47 AM, Masahiro Yamada wrote:
>>> On Thu, Nov 21, 2019 at 6:38 AM Marek Vasut wrote:
>>>>
>>>> Legacy kernel versions for SoCFPGA may not implement proper reset
>>>> handling.
>>>
>>> What is "legacy kernel versions" ?
>>
>> Anything older than 5.x , which got proper reset handling, finally.
> 
> 
> Really?
> Could you tell the commit ID?

commit 37f7453a4b7aa3e1d1608edeb500b105257ee945
    ARM: dts: socfpga: update missing reset property peripherals

commit 1c909b2dfe6a21de2c24dc1c1405593e40e3a88c
    ARM: dts: socfpga: update more missing reset properties

[U-Boot] [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA

[Masahiro Yamada]

On Tue, Nov 26, 2019 at 6:01 PM Marek Vasut <marex@denx.de> wrote:
>
> On 11/26/19 9:46 AM, Masahiro Yamada wrote:
> > On Tue, Nov 26, 2019 at 5:23 PM Marek Vasut wrote:
> >>
> >> On 11/26/19 3:47 AM, Masahiro Yamada wrote:
> >>> On Thu, Nov 21, 2019 at 6:38 AM Marek Vasut wrote:
> >>>>
> >>>> Legacy kernel versions for SoCFPGA may not implement proper reset
> >>>> handling.
> >>>
> >>> What is "legacy kernel versions" ?
> >>
> >> Anything older than 5.x , which got proper reset handling, finally.
> >
> >
> > Really?
> > Could you tell the commit ID?
>
> commit 37f7453a4b7aa3e1d1608edeb500b105257ee945
>     ARM: dts: socfpga: update missing reset property peripherals
>
> commit 1c909b2dfe6a21de2c24dc1c1405593e40e3a88c
>     ARM: dts: socfpga: update more missing reset properties


These are just DT updates.

I do not see any proper reset handling in drivers/mtd/nand/raw/denali*.c


I am curious how the NAND reset is working in your kernel.




> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot


--
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA

[Marek Vasut]

On 11/26/19 10:07 AM, Masahiro Yamada wrote:
> On Tue, Nov 26, 2019 at 6:01 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 11/26/19 9:46 AM, Masahiro Yamada wrote:
>>> On Tue, Nov 26, 2019 at 5:23 PM Marek Vasut wrote:
>>>>
>>>> On 11/26/19 3:47 AM, Masahiro Yamada wrote:
>>>>> On Thu, Nov 21, 2019 at 6:38 AM Marek Vasut wrote:
>>>>>>
>>>>>> Legacy kernel versions for SoCFPGA may not implement proper reset
>>>>>> handling.
>>>>>
>>>>> What is "legacy kernel versions" ?
>>>>
>>>> Anything older than 5.x , which got proper reset handling, finally.
>>>
>>>
>>> Really?
>>> Could you tell the commit ID?
>>
>> commit 37f7453a4b7aa3e1d1608edeb500b105257ee945
>>     ARM: dts: socfpga: update missing reset property peripherals
>>
>> commit 1c909b2dfe6a21de2c24dc1c1405593e40e3a88c
>>     ARM: dts: socfpga: update more missing reset properties
> 
> 
> These are just DT updates.
> 
> I do not see any proper reset handling in drivers/mtd/nand/raw/denali*.c

Yes, DT updates which actually wire the reset to the IP so it can be
toggled.

> I am curious how the NAND reset is working in your kernel.

It was not working at all until those patches were in.

[U-Boot] [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA

[Masahiro Yamada]

On Tue, Nov 26, 2019 at 6:10 PM Marek Vasut <marex@denx.de> wrote:
>
> On 11/26/19 10:07 AM, Masahiro Yamada wrote:
> > On Tue, Nov 26, 2019 at 6:01 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 11/26/19 9:46 AM, Masahiro Yamada wrote:
> >>> On Tue, Nov 26, 2019 at 5:23 PM Marek Vasut wrote:
> >>>>
> >>>> On 11/26/19 3:47 AM, Masahiro Yamada wrote:
> >>>>> On Thu, Nov 21, 2019 at 6:38 AM Marek Vasut wrote:
> >>>>>>
> >>>>>> Legacy kernel versions for SoCFPGA may not implement proper reset
> >>>>>> handling.
> >>>>>
> >>>>> What is "legacy kernel versions" ?
> >>>>
> >>>> Anything older than 5.x , which got proper reset handling, finally.
> >>>
> >>>
> >>> Really?
> >>> Could you tell the commit ID?
> >>
> >> commit 37f7453a4b7aa3e1d1608edeb500b105257ee945
> >>     ARM: dts: socfpga: update missing reset property peripherals
> >>
> >> commit 1c909b2dfe6a21de2c24dc1c1405593e40e3a88c
> >>     ARM: dts: socfpga: update more missing reset properties
> >
> >
> > These are just DT updates.
> >
> > I do not see any proper reset handling in drivers/mtd/nand/raw/denali*.c
>
> Yes, DT updates which actually wire the reset to the IP so it can be
> toggled.


I am asking where the reset _consumer_ code is.

I'd like to find where
  reset_control_get*
  reset_control_deassert()
are called for the NAND reset.



> > I am curious how the NAND reset is working in your kernel.
>
> It was not working at all until those patches were in.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init

[Masahiro Yamada]

On Tue, Nov 26, 2019 at 5:25 PM Marek Vasut <marex@denx.de> wrote:
>
> On 11/26/19 3:34 AM, Masahiro Yamada wrote:
> > On Thu, Nov 21, 2019 at 6:37 AM Marek Vasut 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().
> >
> > I'd like to know this in more detailed.
> >
> > I assume these registers are set up by the BootROM,
> > and SPL is supposed to read data in the same manner as the Boot ROM.
> >
> > In which situation does the data get corrupted?
>
> When I boot the board, these registers are not set up correctly. The
> bootrom reads the SPL fine, but the SPL cannot read U-Boot anymore,
> probably because the bootrom does something funny to the controller
> settings.

Interesting.
The boot ROM can load SPL from a NAND device,
but SPL cannot load U-Boot from it in the same manner.


How did you burn SPL, U-Boot proper, respectively?
Were they written in different ways?




>
> >> 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);
> >
> > I guess you tested this for SOCFPGA.
> >
> > Please tell me the value of CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES.
> 2


-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init

[Marek Vasut]

On 11/26/19 10:24 AM, Masahiro Yamada wrote:
> On Tue, Nov 26, 2019 at 5:25 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 11/26/19 3:34 AM, Masahiro Yamada wrote:
>>> On Thu, Nov 21, 2019 at 6:37 AM Marek Vasut 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().
>>>
>>> I'd like to know this in more detailed.
>>>
>>> I assume these registers are set up by the BootROM,
>>> and SPL is supposed to read data in the same manner as the Boot ROM.
>>>
>>> In which situation does the data get corrupted?
>>
>> When I boot the board, these registers are not set up correctly. The
>> bootrom reads the SPL fine, but the SPL cannot read U-Boot anymore,
>> probably because the bootrom does something funny to the controller
>> settings.
> 
> Interesting.
> The boot ROM can load SPL from a NAND device,
> but SPL cannot load U-Boot from it in the same manner.
> 
> 
> How did you burn SPL, U-Boot proper, respectively?
> Were they written in different ways?

U-Boot proper, all of it.

[U-Boot] [PATCH 3/3] mtd: rawnand: denali: Do not reset the block on SoCFPGA

[Marek Vasut]

On 11/26/19 10:17 AM, Masahiro Yamada wrote:
> On Tue, Nov 26, 2019 at 6:10 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 11/26/19 10:07 AM, Masahiro Yamada wrote:
>>> On Tue, Nov 26, 2019 at 6:01 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 11/26/19 9:46 AM, Masahiro Yamada wrote:
>>>>> On Tue, Nov 26, 2019 at 5:23 PM Marek Vasut wrote:
>>>>>>
>>>>>> On 11/26/19 3:47 AM, Masahiro Yamada wrote:
>>>>>>> On Thu, Nov 21, 2019 at 6:38 AM Marek Vasut wrote:
>>>>>>>>
>>>>>>>> Legacy kernel versions for SoCFPGA may not implement proper reset
>>>>>>>> handling.
>>>>>>>
>>>>>>> What is "legacy kernel versions" ?
>>>>>>
>>>>>> Anything older than 5.x , which got proper reset handling, finally.
>>>>>
>>>>>
>>>>> Really?
>>>>> Could you tell the commit ID?
>>>>
>>>> commit 37f7453a4b7aa3e1d1608edeb500b105257ee945
>>>>     ARM: dts: socfpga: update missing reset property peripherals
>>>>
>>>> commit 1c909b2dfe6a21de2c24dc1c1405593e40e3a88c
>>>>     ARM: dts: socfpga: update more missing reset properties
>>>
>>>
>>> These are just DT updates.
>>>
>>> I do not see any proper reset handling in drivers/mtd/nand/raw/denali*.c
>>
>> Yes, DT updates which actually wire the reset to the IP so it can be
>> toggled.
> 
> 
> I am asking where the reset _consumer_ code is.
> 
> I'd like to find where
>   reset_control_get*
>   reset_control_deassert()
> are called for the NAND reset.

Clearly nowhere if the old kernels do not implement the reset properly.

[U-Boot] [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init

[Masahiro Yamada]

On Tue, Nov 26, 2019 at 5:25 PM Marek Vasut <marex@denx.de> wrote:
> >> 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);
> >
> > I guess you tested this for SOCFPGA.
> >
> > Please tell me the value of CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES.
> 2


CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES=2 is the only value
that works with the Boot ROM, right?

How did you find the correct value is 2 ?

Is it documented in the SOCFPGA datasheet or somewhere?
Or, did you repeat try-and-error?


-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init

[Marek Vasut]

On 11/27/19 11:20 AM, Masahiro Yamada wrote:
> On Tue, Nov 26, 2019 at 5:25 PM Marek Vasut <marex@denx.de> wrote:
>>>> 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);
>>>
>>> I guess you tested this for SOCFPGA.
>>>
>>> Please tell me the value of CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES.
>> 2
> 
> 
> CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES=2 is the only value
> that works with the Boot ROM, right?
> 
> How did you find the correct value is 2 ?
> 
> Is it documented in the SOCFPGA datasheet or somewhere?
> Or, did you repeat try-and-error?

I took it from the old vendoruboot port.