Re: [U-Boot] [PATCH 3/6] mtd: nand: Add the sunxi NAND controller driver

2016-06-08 Thread Scott Wood
On Tue, 2016-06-07 at 07:41 +0200, Boris Brezillon wrote:
> On Mon, 06 Jun 2016 18:54:03 -0500
> Scott Wood  wrote:
> 
> > Of course the driver model
> > is probably the long-term solution.
> 
> Definitely, and talking about things that need to be reworked, do you
> know why u-boot is using its own MTD partition infrastructure instead
> of relying on mtdpart.c?

U-Boot's partition code predates the importation of the MTD code.

> That's really a pain when one wants to add a new feature (like
> definitions of partitions in the DT, or SLC mode on MLC NANDs) because
> he has to do it twice.

Defining partitions in the DT isn't such a great idea, at least on reference
boards, as it's configuration that users are likely to want to change.

> And that's not the only inconsistent part in the MTD/NAND layer IMO.
> MTD is providing a generic abstraction for all flashes, but nand_util
> is still directly accessing the NAND layer instead of going through the
> MTD abstraction.

As with partitions, that code predates the existence of the MTD abstraction in
U-Boot.

> By using the MTD abstraction everywhere (I mean for all flash devices),
> we could provide generic utils (flash erase, flash write), even if
> specific tools might be needed in a few cases.

There are a lot of special NANDisms being handled in that code (bad block
skipping, JFFS2 OOB cleanmarkers, etc), so I wonder what a generic version
would look like.

> Anyway, good to hear that you plan to switch to the driver model.

I don't plan to do much of anything with the NAND code -- I'm still acting as
custodian because nobody stepped up when I asked for volunteers to take it
over a couple years back, but it's pretty low on my priority list regarding
active development[1].  But if someone else wants to DM-ize a NAND driver I
have no problem with that. :-)

-Scott

[1] Linux syncs are an exception, as they're easier to do than to review,
especially since a patch only shows the end result rather than the process to
produce it.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/6] mtd: nand: Add the sunxi NAND controller driver

2016-06-06 Thread Boris Brezillon
On Mon, 06 Jun 2016 18:54:03 -0500
Scott Wood  wrote:

> On Mon, 2016-06-06 at 20:31 +0200, Boris Brezillon wrote:
> > On Mon, 06 Jun 2016 12:56:48 -0500
> > Scott Wood  wrote:
> >   
> > > On Mon, 2016-06-06 at 18:22 +0200, Boris Brezillon wrote:  
> > > > On Mon, 6 Jun 2016 17:36:10 +0200
> > > > Hans de Goede  wrote:
> > > > 
> > > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > > +void sunxi_nand_init(void);
> > > > > > +#endif
> > > > > > +  
> > > > > 
> > > > > Can we have this in a header somewhere please, and without
> > > > > the #ifdef around it, that is not necessary for prototypes.
> > > > 
> > > > Hehe, I was expecting this one :-). Do you know where I should put this
> > > > prototype definition? A board.h file in board/sunxi/?
> > > 
> > > It's defined in drivers/mtd/nand and called from board/sunxi so the
> > > prototype
> > > needs to go somewhere under include/.  It can go in include/configs/sunxi
> > > -common.h with #ifndef __ASSEMBLY__ around it -- or as long as it's
> > > limited to
> > > one init func per driver, maybe we could just put it in include/nand.h.  
> > 
> > Hm, none of these solutions seem ideal.
> > 
> > Maybe we could define a generic void nand_controller_init(void)
> > prototype so that we don't need to add new xxx_nand_init() functions for
> > platforms needing this 2 steps initialization (platform specific pinmux
> > + clocks config before NAND controller initialization).
> > 
> > Otherwise, I think I'll go for the 2nd solution (defining
> > sunxi_nand_init() in include/nand.h).  
> 
> I'd prefer not having a generic nand_controller_init() because some platforms
> may want to pass arguments, plus I don't want to rule out the possibility of
> two different NAND controller types being supported at once.
> 
> include/nand.h is fine with me, as long as any driver than wants more than an
> initfunc moves its stuff into a dedicated header.

Sure.

> Of course the driver model
> is probably the long-term solution.

Definitely, and talking about things that need to be reworked, do you
know why u-boot is using its own MTD partition infrastructure instead
of relying on mtdpart.c?
That's really a pain when one wants to add a new feature (like
definitions of partitions in the DT, or SLC mode on MLC NANDs) because
he has to do it twice.

And that's not the only inconsistent part in the MTD/NAND layer IMO.
MTD is providing a generic abstraction for all flashes, but nand_util
is still directly accessing the NAND layer instead of going through the
MTD abstraction.
By using the MTD abstraction everywhere (I mean for all flash devices),
we could provide generic utils (flash erase, flash write), even if
specific tools might be needed in a few cases.

Anyway, good to hear that you plan to switch to the driver model.

Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/6] mtd: nand: Add the sunxi NAND controller driver

2016-06-06 Thread Scott Wood
On Mon, 2016-06-06 at 20:31 +0200, Boris Brezillon wrote:
> On Mon, 06 Jun 2016 12:56:48 -0500
> Scott Wood  wrote:
> 
> > On Mon, 2016-06-06 at 18:22 +0200, Boris Brezillon wrote:
> > > On Mon, 6 Jun 2016 17:36:10 +0200
> > > Hans de Goede  wrote:
> > >   
> > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > +void sunxi_nand_init(void);
> > > > > +#endif
> > > > > +
> > > > 
> > > > Can we have this in a header somewhere please, and without
> > > > the #ifdef around it, that is not necessary for prototypes.  
> > > 
> > > Hehe, I was expecting this one :-). Do you know where I should put this
> > > prototype definition? A board.h file in board/sunxi/?  
> > 
> > It's defined in drivers/mtd/nand and called from board/sunxi so the
> > prototype
> > needs to go somewhere under include/.  It can go in include/configs/sunxi
> > -common.h with #ifndef __ASSEMBLY__ around it -- or as long as it's
> > limited to
> > one init func per driver, maybe we could just put it in include/nand.h.
> 
> Hm, none of these solutions seem ideal.
> 
> Maybe we could define a generic void nand_controller_init(void)
> prototype so that we don't need to add new xxx_nand_init() functions for
> platforms needing this 2 steps initialization (platform specific pinmux
> + clocks config before NAND controller initialization).
> 
> Otherwise, I think I'll go for the 2nd solution (defining
> sunxi_nand_init() in include/nand.h).

I'd prefer not having a generic nand_controller_init() because some platforms
may want to pass arguments, plus I don't want to rule out the possibility of
two different NAND controller types being supported at once.

include/nand.h is fine with me, as long as any driver than wants more than an
initfunc moves its stuff into a dedicated header.  Of course the driver model
is probably the long-term solution.

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/6] mtd: nand: Add the sunxi NAND controller driver

2016-06-06 Thread Boris Brezillon
On Mon, 06 Jun 2016 12:56:48 -0500
Scott Wood  wrote:

> On Mon, 2016-06-06 at 18:22 +0200, Boris Brezillon wrote:
> > On Mon, 6 Jun 2016 17:36:10 +0200
> > Hans de Goede  wrote:
> >   
> > > > +#ifndef CONFIG_SPL_BUILD
> > > > +void sunxi_nand_init(void);
> > > > +#endif
> > > > +
> > > 
> > > Can we have this in a header somewhere please, and without
> > > the #ifdef around it, that is not necessary for prototypes.  
> > 
> > Hehe, I was expecting this one :-). Do you know where I should put this
> > prototype definition? A board.h file in board/sunxi/?  
> 
> It's defined in drivers/mtd/nand and called from board/sunxi so the prototype
> needs to go somewhere under include/.  It can go in include/configs/sunxi
> -common.h with #ifndef __ASSEMBLY__ around it -- or as long as it's limited to
> one init func per driver, maybe we could just put it in include/nand.h.

Hm, none of these solutions seem ideal.

Maybe we could define a generic void nand_controller_init(void)
prototype so that we don't need to add new xxx_nand_init() functions for
platforms needing this 2 steps initialization (platform specific pinmux
+ clocks config before NAND controller initialization).

Otherwise, I think I'll go for the 2nd solution (defining
sunxi_nand_init() in include/nand.h).

Let me know what you prefer.

Thanks,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/6] mtd: nand: Add the sunxi NAND controller driver

2016-06-06 Thread Scott Wood
On Mon, 2016-06-06 at 18:22 +0200, Boris Brezillon wrote:
> On Mon, 6 Jun 2016 17:36:10 +0200
> Hans de Goede  wrote:
> 
> > > +#ifndef CONFIG_SPL_BUILD
> > > +void sunxi_nand_init(void);
> > > +#endif
> > > +  
> > 
> > Can we have this in a header somewhere please, and without
> > the #ifdef around it, that is not necessary for prototypes.
> 
> Hehe, I was expecting this one :-). Do you know where I should put this
> prototype definition? A board.h file in board/sunxi/?

It's defined in drivers/mtd/nand and called from board/sunxi so the prototype
needs to go somewhere under include/.  It can go in include/configs/sunxi
-common.h with #ifndef __ASSEMBLY__ around it -- or as long as it's limited to
one init func per driver, maybe we could just put it in include/nand.h.

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/6] mtd: nand: Add the sunxi NAND controller driver

2016-06-06 Thread Boris Brezillon
On Mon, 6 Jun 2016 17:36:10 +0200
Hans de Goede  wrote:

> > +#ifndef CONFIG_SPL_BUILD
> > +void sunxi_nand_init(void);
> > +#endif
> > +  
> 
> Can we have this in a header somewhere please, and without
> the #ifdef around it, that is not necessary for prototypes.

Hehe, I was expecting this one :-). Do you know where I should put this
prototype definition? A board.h file in board/sunxi/?

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH 3/6] mtd: nand: Add the sunxi NAND controller driver

2016-06-06 Thread Hans de Goede

Hi,

One small remark inline.

On 06-06-16 17:21, Boris Brezillon wrote:

We already have an SPL driver for the sunxi NAND controller, now add
the normal/standard one.

The source has been copied from Linux 4.6 with a few changes to make
it work in u-boot.

Signed-off-by: Boris Brezillon 
---
 board/sunxi/board.c|9 +-
 drivers/mtd/nand/Kconfig   |8 +-
 drivers/mtd/nand/Makefile  |1 +
 drivers/mtd/nand/sunxi_nand.c  | 1845 
 include/configs/sunxi-common.h |2 +
 include/fdtdec.h   |1 +
 lib/fdtdec.c   |1 +
 7 files changed, 1863 insertions(+), 4 deletions(-)
 create mode 100644 drivers/mtd/nand/sunxi_nand.c

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 3cf3614..e0b3fb0 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -142,7 +142,7 @@ void dram_init_banksize(void)
 }
 #endif

-#if defined(CONFIG_NAND_SUNXI) && defined(CONFIG_SPL_BUILD)
+#if defined(CONFIG_NAND_SUNXI)
 static void nand_pinmux_setup(void)
 {
unsigned int pin;
@@ -175,10 +175,17 @@ static void nand_clock_setup(void)
setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
 }

+#ifndef CONFIG_SPL_BUILD
+void sunxi_nand_init(void);
+#endif
+


Can we have this in a header somewhere please, and without
the #ifdef around it, that is not necessary for prototypes.

Otherwise the entire series looks good to me.

Regards,

Hans


 void board_nand_init(void)
 {
nand_pinmux_setup();
nand_clock_setup();
+#ifndef CONFIG_SPL_BUILD
+   sunxi_nand_init();
+#endif
 }
 #endif

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 8c46a2f..5ce7d6d 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -64,12 +64,14 @@ config NAND_PXA3XX
  PXA3xx processors (NFCv1) and also on Armada 370/XP (NFCv2).

 config NAND_SUNXI
-   bool "Support for NAND on Allwinner SoCs in SPL"
+   bool "Support for NAND on Allwinner SoCs"
depends on MACH_SUN4I || MACH_SUN5I || MACH_SUN7I
select SYS_NAND_SELF_INIT
---help---
-   Enable support for NAND. This option allows SPL to read from
-   sunxi NAND using DMA transfers.
+   Enable support for NAND. This option enables the standard and
+   SPL drivers.
+   The SPL driver only supports reading from the NAND using DMA
+   transfers.

 config NAND_ARASAN
bool "Configure Arasan Nand"
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 837d397..1df9273 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_TEGRA_NAND) += tegra_nand.o
 obj-$(CONFIG_NAND_OMAP_GPMC) += omap_gpmc.o
 obj-$(CONFIG_NAND_OMAP_ELM) += omap_elm.o
 obj-$(CONFIG_NAND_PLAT) += nand_plat.o
+obj-$(CONFIG_NAND_SUNXI) += sunxi_nand.o

 else  # minimal SPL drivers

diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
new file mode 100644
index 000..c4e2cd7
--- /dev/null
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -0,0 +1,1845 @@
+/*
+ * Copyright (C) 2013 Boris BREZILLON 
+ * Copyright (C) 2015 Roy Spliet 
+ *
+ * Derived from:
+ * https://github.com/yuq/sunxi-nfc-mtd
+ * Copyright (C) 2013 Qiang Yu 
+ *
+ * https://github.com/hno/Allwinner-Info
+ * Copyright (C) 2013 Henrik Nordström 
+ *
+ * Copyright (C) 2013 Dmitriy B. 
+ * Copyright (C) 2013 Sergey Lapin 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define NFC_REG_CTL0x
+#define NFC_REG_ST 0x0004
+#define NFC_REG_INT0x0008
+#define NFC_REG_TIMING_CTL 0x000C
+#define NFC_REG_TIMING_CFG 0x0010
+#define NFC_REG_ADDR_LOW   0x0014
+#define NFC_REG_ADDR_HIGH  0x0018
+#define NFC_REG_SECTOR_NUM 0x001C
+#define NFC_REG_CNT0x0020
+#define NFC_REG_CMD0x0024
+#define NFC_REG_RCMD_SET   0x0028
+#define NFC_REG_WCMD_SET   0x002C
+#define NFC_REG_IO_DATA0x0030
+#define NFC_REG_ECC_CTL0x0034
+#define NFC_REG_ECC_ST 0x0038
+#define NFC_REG_DEBUG  0x003C
+#define NFC_REG_ECC_ERR_CNT(x) ((0x0040 + (x)) & ~0x3)
+#define NFC_REG_USER_DATA(x)   (0x0050 + ((x) * 4))
+#define NFC_REG_SPARE_AREA 0x00A0
+#define NFC_REG_PAT_ID 0

[U-Boot] [PATCH 3/6] mtd: nand: Add the sunxi NAND controller driver

2016-06-06 Thread Boris Brezillon
We already have an SPL driver for the sunxi NAND controller, now add
the normal/standard one.

The source has been copied from Linux 4.6 with a few changes to make
it work in u-boot.

Signed-off-by: Boris Brezillon 
---
 board/sunxi/board.c|9 +-
 drivers/mtd/nand/Kconfig   |8 +-
 drivers/mtd/nand/Makefile  |1 +
 drivers/mtd/nand/sunxi_nand.c  | 1845 
 include/configs/sunxi-common.h |2 +
 include/fdtdec.h   |1 +
 lib/fdtdec.c   |1 +
 7 files changed, 1863 insertions(+), 4 deletions(-)
 create mode 100644 drivers/mtd/nand/sunxi_nand.c

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 3cf3614..e0b3fb0 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -142,7 +142,7 @@ void dram_init_banksize(void)
 }
 #endif
 
-#if defined(CONFIG_NAND_SUNXI) && defined(CONFIG_SPL_BUILD)
+#if defined(CONFIG_NAND_SUNXI)
 static void nand_pinmux_setup(void)
 {
unsigned int pin;
@@ -175,10 +175,17 @@ static void nand_clock_setup(void)
setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
 }
 
+#ifndef CONFIG_SPL_BUILD
+void sunxi_nand_init(void);
+#endif
+
 void board_nand_init(void)
 {
nand_pinmux_setup();
nand_clock_setup();
+#ifndef CONFIG_SPL_BUILD
+   sunxi_nand_init();
+#endif
 }
 #endif
 
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 8c46a2f..5ce7d6d 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -64,12 +64,14 @@ config NAND_PXA3XX
  PXA3xx processors (NFCv1) and also on Armada 370/XP (NFCv2).
 
 config NAND_SUNXI
-   bool "Support for NAND on Allwinner SoCs in SPL"
+   bool "Support for NAND on Allwinner SoCs"
depends on MACH_SUN4I || MACH_SUN5I || MACH_SUN7I
select SYS_NAND_SELF_INIT
---help---
-   Enable support for NAND. This option allows SPL to read from
-   sunxi NAND using DMA transfers.
+   Enable support for NAND. This option enables the standard and
+   SPL drivers.
+   The SPL driver only supports reading from the NAND using DMA
+   transfers.
 
 config NAND_ARASAN
bool "Configure Arasan Nand"
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 837d397..1df9273 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_TEGRA_NAND) += tegra_nand.o
 obj-$(CONFIG_NAND_OMAP_GPMC) += omap_gpmc.o
 obj-$(CONFIG_NAND_OMAP_ELM) += omap_elm.o
 obj-$(CONFIG_NAND_PLAT) += nand_plat.o
+obj-$(CONFIG_NAND_SUNXI) += sunxi_nand.o
 
 else  # minimal SPL drivers
 
diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
new file mode 100644
index 000..c4e2cd7
--- /dev/null
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -0,0 +1,1845 @@
+/*
+ * Copyright (C) 2013 Boris BREZILLON 
+ * Copyright (C) 2015 Roy Spliet 
+ *
+ * Derived from:
+ * https://github.com/yuq/sunxi-nfc-mtd
+ * Copyright (C) 2013 Qiang Yu 
+ *
+ * https://github.com/hno/Allwinner-Info
+ * Copyright (C) 2013 Henrik Nordström 
+ *
+ * Copyright (C) 2013 Dmitriy B. 
+ * Copyright (C) 2013 Sergey Lapin 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define NFC_REG_CTL0x
+#define NFC_REG_ST 0x0004
+#define NFC_REG_INT0x0008
+#define NFC_REG_TIMING_CTL 0x000C
+#define NFC_REG_TIMING_CFG 0x0010
+#define NFC_REG_ADDR_LOW   0x0014
+#define NFC_REG_ADDR_HIGH  0x0018
+#define NFC_REG_SECTOR_NUM 0x001C
+#define NFC_REG_CNT0x0020
+#define NFC_REG_CMD0x0024
+#define NFC_REG_RCMD_SET   0x0028
+#define NFC_REG_WCMD_SET   0x002C
+#define NFC_REG_IO_DATA0x0030
+#define NFC_REG_ECC_CTL0x0034
+#define NFC_REG_ECC_ST 0x0038
+#define NFC_REG_DEBUG  0x003C
+#define NFC_REG_ECC_ERR_CNT(x) ((0x0040 + (x)) & ~0x3)
+#define NFC_REG_USER_DATA(x)   (0x0050 + ((x) * 4))
+#define NFC_REG_SPARE_AREA 0x00A0
+#define NFC_REG_PAT_ID 0x00A4
+#define NFC_RAM0_BASE  0x0400
+#define NFC_RAM1_BASE  0x0800
+
+/* define bit use in NFC_CTL */
+#define NFC_EN BIT(0)
+#define NFC_RESET  BIT(1)
+#define NFC_BUS_WIDTH_MSK  BIT(2)
+#define NFC_BUS_W