[PATCH v2] mtd: zynq: nand: Move board_nand_init() function to board.c

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] mtd: zynq: nand: Move board_nand_init() function to board.c

Wilson Lee
Putting board_nand_init() function inside NAND driver was not appropriate
due to it doesn't allow board vendor to customise their NAND
initialization code such as adding NAND lock/unlock code.

This commit was to move the board_nand_init() function from NAND driver
to board.c file. This allow customization of board_nand_init() function.

Signed-off-by: Wilson Lee <[hidden email]>
Cc: Joe Hershberger <[hidden email]>
Cc: Keng Soon Cheah <[hidden email]>
Cc: Chen Yee Chew <[hidden email]>
Cc: Albert Aribaud <[hidden email]>
Cc: Michal Simek <[hidden email]>
Cc: Siva Durga Prasad Paladugu <[hidden email]>
Cc: Scott Wood <[hidden email]>
---
 arch/arm/mach-zynq/include/mach/nand.h | 9 +++++++++
 drivers/mtd/nand/zynq_nand.c           | 6 ++++--
 2 files changed, 13 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/mach-zynq/include/mach/nand.h

diff --git a/arch/arm/mach-zynq/include/mach/nand.h b/arch/arm/mach-zynq/include/mach/nand.h
new file mode 100644
index 0000000..61ef45f
--- /dev/null
+++ b/arch/arm/mach-zynq/include/mach/nand.h
@@ -0,0 +1,9 @@
+/*
+ * Copyright (C) 2017 National Instruments Corp.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <nand.h>
+
+void zynq_nand_init(void);
diff --git a/drivers/mtd/nand/zynq_nand.c b/drivers/mtd/nand/zynq_nand.c
index dec2c41..076b878 100644
--- a/drivers/mtd/nand/zynq_nand.c
+++ b/drivers/mtd/nand/zynq_nand.c
@@ -1006,7 +1006,7 @@ static int zynq_nand_device_ready(struct mtd_info *mtd)
  return 0;
 }
 
-static int zynq_nand_init(struct nand_chip *nand_chip, int devnum)
+int zynq_nand_init(struct nand_chip *nand_chip, int devnum)
 {
  struct zynq_nand_info *xnand;
  struct mtd_info *mtd;
@@ -1192,12 +1192,14 @@ fail:
  return err;
 }
 
+#ifdef CONFIG_SYS_NAND_SELF_INIT
 static struct nand_chip nand_chip[CONFIG_SYS_MAX_NAND_DEVICE];
 
-void board_nand_init(void)
+void __weak board_nand_init(void)
 {
  struct nand_chip *nand = &nand_chip[0];
 
  if (zynq_nand_init(nand, 0))
  puts("ZYNQ NAND init failed\n");
 }
+#endif
--
2.7.4

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] mtd: zynq: nand: Move board_nand_init() function to board.c

Michal Simek-3
On 15.11.2017 10:14, Wilson Lee wrote:

> Putting board_nand_init() function inside NAND driver was not appropriate
> due to it doesn't allow board vendor to customise their NAND
> initialization code such as adding NAND lock/unlock code.
>
> This commit was to move the board_nand_init() function from NAND driver
> to board.c file. This allow customization of board_nand_init() function.
>
> Signed-off-by: Wilson Lee <[hidden email]>
> Cc: Joe Hershberger <[hidden email]>
> Cc: Keng Soon Cheah <[hidden email]>
> Cc: Chen Yee Chew <[hidden email]>
> Cc: Albert Aribaud <[hidden email]>
> Cc: Michal Simek <[hidden email]>
> Cc: Siva Durga Prasad Paladugu <[hidden email]>
> Cc: Scott Wood <[hidden email]>
> ---
>  arch/arm/mach-zynq/include/mach/nand.h | 9 +++++++++
>  drivers/mtd/nand/zynq_nand.c           | 6 ++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/mach-zynq/include/mach/nand.h
>
> diff --git a/arch/arm/mach-zynq/include/mach/nand.h b/arch/arm/mach-zynq/include/mach/nand.h
> new file mode 100644
> index 0000000..61ef45f
> --- /dev/null
> +++ b/arch/arm/mach-zynq/include/mach/nand.h
> @@ -0,0 +1,9 @@
> +/*
> + * Copyright (C) 2017 National Instruments Corp.
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <nand.h>
> +
> +void zynq_nand_init(void);
> diff --git a/drivers/mtd/nand/zynq_nand.c b/drivers/mtd/nand/zynq_nand.c
> index dec2c41..076b878 100644
> --- a/drivers/mtd/nand/zynq_nand.c
> +++ b/drivers/mtd/nand/zynq_nand.c
> @@ -1006,7 +1006,7 @@ static int zynq_nand_device_ready(struct mtd_info *mtd)
>   return 0;
>  }
>  
> -static int zynq_nand_init(struct nand_chip *nand_chip, int devnum)
> +int zynq_nand_init(struct nand_chip *nand_chip, int devnum)
>  {
>   struct zynq_nand_info *xnand;
>   struct mtd_info *mtd;
> @@ -1192,12 +1192,14 @@ fail:
>   return err;
>  }
>  
> +#ifdef CONFIG_SYS_NAND_SELF_INIT
>  static struct nand_chip nand_chip[CONFIG_SYS_MAX_NAND_DEVICE];
>  
> -void board_nand_init(void)
> +void __weak board_nand_init(void)
>  {
>   struct nand_chip *nand = &nand_chip[0];
>  
>   if (zynq_nand_init(nand, 0))
>   puts("ZYNQ NAND init failed\n");
>  }
> +#endif
>

Applied.
Michal
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] mtd: zynq: nand: Move board_nand_init() function to board.c

Ezequiel Garcia-2
In reply to this post by Wilson Lee
On 15 November 2017 at 06:14, Wilson Lee <[hidden email]> wrote:

> Putting board_nand_init() function inside NAND driver was not appropriate
> due to it doesn't allow board vendor to customise their NAND
> initialization code such as adding NAND lock/unlock code.
>
> This commit was to move the board_nand_init() function from NAND driver
> to board.c file. This allow customization of board_nand_init() function.
>
> Signed-off-by: Wilson Lee <[hidden email]>
> Cc: Joe Hershberger <[hidden email]>
> Cc: Keng Soon Cheah <[hidden email]>
> Cc: Chen Yee Chew <[hidden email]>
> Cc: Albert Aribaud <[hidden email]>
> Cc: Michal Simek <[hidden email]>
> Cc: Siva Durga Prasad Paladugu <[hidden email]>
> Cc: Scott Wood <[hidden email]>
> ---
>  arch/arm/mach-zynq/include/mach/nand.h | 9 +++++++++
>  drivers/mtd/nand/zynq_nand.c           | 6 ++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/mach-zynq/include/mach/nand.h
>
> diff --git a/arch/arm/mach-zynq/include/mach/nand.h b/arch/arm/mach-zynq/include/mach/nand.h
> new file mode 100644
> index 0000000..61ef45f
> --- /dev/null
> +++ b/arch/arm/mach-zynq/include/mach/nand.h
> @@ -0,0 +1,9 @@
> +/*
> + * Copyright (C) 2017 National Instruments Corp.
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <nand.h>
> +
> +void zynq_nand_init(void);
> diff --git a/drivers/mtd/nand/zynq_nand.c b/drivers/mtd/nand/zynq_nand.c
> index dec2c41..076b878 100644
> --- a/drivers/mtd/nand/zynq_nand.c
> +++ b/drivers/mtd/nand/zynq_nand.c
> @@ -1006,7 +1006,7 @@ static int zynq_nand_device_ready(struct mtd_info *mtd)
>         return 0;
>  }
>
> -static int zynq_nand_init(struct nand_chip *nand_chip, int devnum)
> +int zynq_nand_init(struct nand_chip *nand_chip, int devnum)
>  {
>         struct zynq_nand_info *xnand;
>         struct mtd_info *mtd;
> @@ -1192,12 +1192,14 @@ fail:
>         return err;
>  }
>
> +#ifdef CONFIG_SYS_NAND_SELF_INIT

Just found this patch while debugging a NAND problem
using current U-Boot upstream, on a custom board. In fact,
I wrote a patch which is an exact revert of this one,
until I noticed the board_nand_init is exported on purpose.

A few observations:

1) The driver selects CONFIG_SYS_NAND_SELF_INIT,
which makes this ifdef a no-op. I'm guessing vendors are using
the driver without the self-init mode. Is that the case?

2) This driver looks broken in upstream, refusing to initalize properly.
The reason is that get_nand_dev_by_index() was being called before
nand_register(), thus returning a pointer into uninitialized memory.
In other words, the struct mtd_info used by the driver is total junk.

The fix is simple, I cooked a patch for it:

diff --git a/drivers/mtd/nand/zynq_nand.c b/drivers/mtd/nand/zynq_nand.c
index 6494196049f1..9f6ff3d045c2 100644
--- a/drivers/mtd/nand/zynq_nand.c
+++ b/drivers/mtd/nand/zynq_nand.c
@@ -1025,7 +1025,7 @@ int zynq_nand_init(struct nand_chip *nand_chip,
int devnum)
        }

        xnand->nand_base = (void __iomem *)ZYNQ_NAND_BASEADDR;
-       mtd = get_nand_dev_by_index(0);
+       mtd = nand_to_mtd(nand_chip);

However, I am not sure about sending this patch upstream, because
it assumes the driver is used on self-init mode only.

Any hints on how this should be fixed properly? Perhaps we should
find a cleaner path into a lock/unlock hook in upstream.

>  static struct nand_chip nand_chip[CONFIG_SYS_MAX_NAND_DEVICE];
>
> -void board_nand_init(void)
> +void __weak board_nand_init(void)
>  {
>         struct nand_chip *nand = &nand_chip[0];
>
>         if (zynq_nand_init(nand, 0))
>                 puts("ZYNQ NAND init failed\n");
>  }
> +#endif
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> [hidden email]
> https://lists.denx.de/listinfo/u-boot



--
Ezequiel GarcĂ­a, VanguardiaSur
www.vanguardiasur.com.ar
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot