Re: [U-Boot] [PATCH v2] mtd: zynq: nand: Move board_nand_init() function to board.c
On 15 November 2017 at 06:14, Wilson Leewrote: > 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 > Cc: Joe Hershberger > Cc: Keng Soon Cheah > Cc: Chen Yee Chew > Cc: Albert Aribaud > Cc: Michal Simek > Cc: Siva Durga Prasad Paladugu > Cc: Scott Wood > --- > 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 000..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 > + > +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 = _chip[0]; > > if (zynq_nand_init(nand, 0)) > puts("ZYNQ NAND init failed\n"); > } > +#endif > -- > 2.7.4 > > ___ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot -- Ezequiel GarcĂa, VanguardiaSur www.vanguardiasur.com.ar ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] mtd: zynq: nand: Move board_nand_init() function to board.c
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> Cc: Joe Hershberger > Cc: Keng Soon Cheah > Cc: Chen Yee Chew > Cc: Albert Aribaud > Cc: Michal Simek > Cc: Siva Durga Prasad Paladugu > Cc: Scott Wood > --- > 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 000..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 > + > +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 = _chip[0]; > > if (zynq_nand_init(nand, 0)) > puts("ZYNQ NAND init failed\n"); > } > +#endif > Applied. Michal ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v2] mtd: zynq: nand: Move board_nand_init() function to board.c
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 LeeCc: Joe Hershberger Cc: Keng Soon Cheah Cc: Chen Yee Chew Cc: Albert Aribaud Cc: Michal Simek Cc: Siva Durga Prasad Paladugu Cc: Scott Wood --- 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 000..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 + +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 = _chip[0]; if (zynq_nand_init(nand, 0)) puts("ZYNQ NAND init failed\n"); } +#endif -- 2.7.4 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot