Chromium: arm: tegra: Add NAND support

Chromium: arm: tegra: Add NAND support

Change-Id: Ic367415623c52f1ec210de701c4c69c912e38bd9

BUG=Chrome-os-partner:2576
TEST=Tested on Seaboard and Harmony platforms with the followings.
nand info
nand device 0
nand scrub
nand markbad 10000000
nand write 500000 12000000 1000
nand read 600000 12000000 1000
nand bad
nand dump 12000000

All works well.

Signed-off-by: Jim Lin <jilin@nvidia.com>;

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=edbc32a

[JimmyZ.nvidia, 2011-03-08 18:55:57]

Anton,

The unit test conducted by Jim is mainly on Harmony and Seaboard. You may need
to add and test out this CL on other Google sponsored boards that come with NAND
storage.

Jimmy

[Tom Warren, 2011-03-08 20:43:08]

Just a few things I'd like corrected, Jim.

Thanks!

http://codereview.chromium.org/6623073/diff/1/board/tegra2/common/board.c
File board/tegra2/common/board.c (right):

http://codereview.chromium.org/6623073/diff/1/board/tegra2/common/board.c#new...
board/tegra2/common/board.c:175: #ifndef CONFIG_NAND_TEGRA2
This should be CONFIG_TEGRA2_NAND, just like _SPI, _I2C, etc.

http://codereview.chromium.org/6623073/diff/1/board/tegra2/common/nand/HY27UF...
File board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c (right):

http://codereview.chromium.org/6623073/diff/1/board/tegra2/common/nand/HY27UF...
board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c:140: for (i=0; i< 100000;
i++)
This value should be a #define somewhere, i.e. #define WAIT_FOR_GO_TIMEOUT
100000, etc.

http://codereview.chromium.org/6623073/diff/1/board/tegra2/common/nand/HY27UF...
board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c:196: /* To DO */
How about a printf here saying writesize is too small, instead of failing
silently?

http://codereview.chromium.org/6623073/diff/1/board/tegra2/common/nand/HY27UF...
board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c:244:
writel(Bit31+Bit30+Bit29+Bit28+Bit25+Bit24+Bit8+4, chip->IO_ADDR_W +
NAND_COMMAND_0);
Some of these lines look to be too long (>80 chars). Please fix globally, so I
don't have to when I push it upstream. Thanks.

http://codereview.chromium.org/6623073/diff/1/board/tegra2/common/nand/HY27UF...
File board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.h (right):

http://codereview.chromium.org/6623073/diff/1/board/tegra2/common/nand/HY27UF...
board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.h:17: #define Bit0  0x00000001
All of these are already in common/board.h. Please include that file here or in
the .c file.

http://codereview.chromium.org/6623073/diff/1/board/tegra2/generic/Makefile
File board/tegra2/generic/Makefile (right):

http://codereview.chromium.org/6623073/diff/1/board/tegra2/generic/Makefile#n...
board/tegra2/generic/Makefile:15: ifdef CONFIG_NAND_TEGRA2
Why put our driver in board/common instead of drivers/mtd/nand? And why be
specific about the NAND part (HY27UF084G2B)?

http://codereview.chromium.org/6623073/diff/1/drivers/mtd/nand/nand_base.c
File drivers/mtd/nand/nand_base.c (right):

http://codereview.chromium.org/6623073/diff/1/drivers/mtd/nand/nand_base.c#ne...
drivers/mtd/nand/nand_base.c:2950: #ifndef CONFIG_NAND_TEGRA2
Are we using NAND_ECC_NONE? If so, why, if it's not recommended?

[jilin, 2011-03-09 08:53:21]

1. "Why put our driver in board/common instead of drivers/mtd/nand?"
This is board specific function and custom-made for our tegra2.

2. "And why be specific about the NAND part (HY27UF084G2B)?"
Currently Harmony and Seaboard platforms use this NAND device.

3. "Are we using NAND_ECC_NONE? If so, why"
HW ECC is used by DMA mode. Here we use PIO mode for data transfer in order to
fit into common code (nand_base.c). So I set NAND_ECC_NONE.

Also this NAND device supports EDC (Error Detection Code) for Copy Back Program.
Which (NAND_CMD_RNDIN) will not be invoked in our nand->write_buf function.

[jilin, 2011-03-10 07:06:44]

uploading common/cmd_cros.c is a mistake. I will remove it later.

[jilin, 2011-03-10 07:20:52]

I will keep using current CONFIG_NAND_TEGRA2 definition.

[robotboy, 2011-03-10 08:41:14]

On 2011/03/10 07:20:52, jilin wrote:
> I will keep using current CONFIG_NAND_TEGRA2 definition.

Why not switch to the same format as the rest of the configs as Tom suggests? 
CONFIG_TEGRA2_NAND seems better to me as well.  Is there a good reason for not
switching?

Thanks,
    Anton

[jilin, 2011-03-10 09:28:14]

Done, on switching from CONFIG_NAND_TEGRA2 to CONFIG_TEGRA2_NAND.

[Tom Warren, 2011-03-10 17:06:23]

One more change, please, Jim.

http://codereview.chromium.org/6623073/diff/10009/board/tegra2/common/nand/HY...
File board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c (right):

http://codereview.chromium.org/6623073/diff/10009/board/tegra2/common/nand/HY...
board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c:77:
writel(Bit31+Bit28+Bit27+(3<<20)+Bit19+Bit8, chip->IO_ADDR_W +
I had this comment in Patchset 1, but I must not have pushed it.

I don't like the use of BitXX, etc. here - it has no context. Please #define
NAND_CMD_GO, _PIO, _TX, _A_VALID, etc. in the header file and use the
more-informative names throughout. That way a future maintainer/porter (me, when
I go to push this upstream) can look at the TRM and see what it is you are
doing. Thanks.

[robotboy, 2011-03-10 18:40:42]

Something strange is going on.  If I write U-Boot with this patch into NAND and
boot from there the nand commands work fine.  If I instead write U-Boot to SPI
and boot the nand commands fail to print anything and the banner prints out:

NAND:  No NAND device found!!!

Is the NAND code checking the boot select jumpers?  Or is something else going
on?

We would like to be able to use this code even if we are booting from SPI.

Thanks,
    Anton

[robotboy, 2011-03-10 19:06:09]

On 2011/03/10 18:40:42, robotboy wrote:
> Something strange is going on.  If I write U-Boot with this patch into NAND
and
> boot from there the nand commands work fine.  If I instead write U-Boot to SPI
> and boot the nand commands fail to print anything and the banner prints out:
> 
> NAND:  No NAND device found!!!
> 
> Is the NAND code checking the boot select jumpers?  Or is something else going
> on?
> 
> We would like to be able to use this code even if we are booting from SPI.
> 
> Thanks,
>     Anton

Or, it just occurred to me that the masked ROM when booting from NAND could be
leaving the NAND interface in a different configuration than when booting from
SPI and that this CL's nand code relies on that?

-Anton

[TWarren_nvidia.com, 2011-03-10 19:15:51]

It may be that when booting from NAND, the bootrom sets up some stuff that
U-Boot doesn't, so NAND works, but when booting from SPI, those 'things' aren't
initialized/setup, and NAND fails.  Just a SWAG.

Tom

> -----Original Message-----
> From: robotboy@chromium.org [mailto:robotboy@chromium.org]
> Sent: Thursday, March 10, 2011 11:41 AM
> To: Jim Lin; twarren.nvidia@gmail.com; jimmyz.nvidia@gmail.com
> Cc: chromium-os-reviews@chromium.org; dneiss@codeaurora.org;
> olofj@chromium.org; pgargey@codeaurora.org; amoljadi@codeaurora.org
> Subject: Re: Chromium: arm: tegra: Add NAND support (issue6623073)
> 
> Something strange is going on.  If I write U-Boot with this patch into NAND
> and
> boot from there the nand commands work fine.  If I instead write U-Boot to
> SPI
> and boot the nand commands fail to print anything and the banner prints out:
> 
> NAND:  No NAND device found!!!
> 
> Is the NAND code checking the boot select jumpers?  Or is something else
> going
> on?
> 
> We would like to be able to use this code even if we are booting from SPI.
> 
> Thanks,
>      Anton
> 
> 
> http://codereview.chromium.org/6623073/
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may
contain
confidential information.  Any unauthorized review, use, disclosure or
distribution
is prohibited.  If you are not the intended recipient, please contact the sender
by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

[TWarren_nvidia.com, 2011-03-10 19:18:15]

HaHa! Great minds think alike! (see my previous reply - I hadn't read this
message yet). ;)

> -----Original Message-----
> From: robotboy@chromium.org [mailto:robotboy@chromium.org]
> Sent: Thursday, March 10, 2011 12:06 PM
> To: Jim Lin; twarren.nvidia@gmail.com; jimmyz.nvidia@gmail.com
> Cc: chromium-os-reviews@chromium.org; dneiss@codeaurora.org;
> olofj@chromium.org; pgargey@codeaurora.org; amoljadi@codeaurora.org
> Subject: Re: Chromium: arm: tegra: Add NAND support (issue6623073)
> 
> On 2011/03/10 18:40:42, robotboy wrote:
> > Something strange is going on.  If I write U-Boot with this patch into
> > NAND
> and
> > boot from there the nand commands work fine.  If I instead write U-Boot
> > to SPI
> > and boot the nand commands fail to print anything and the banner prints
> > out:
> 
> > NAND:  No NAND device found!!!
> 
> > Is the NAND code checking the boot select jumpers?  Or is something else
> > going
> > on?
> 
> > We would like to be able to use this code even if we are booting from SPI.
> 
> > Thanks,
> >      Anton
> 
> Or, it just occurred to me that the masked ROM when booting from NAND could
> be
> leaving the NAND interface in a different configuration than when booting
> from
> SPI and that this CL's nand code relies on that?
> 
> -Anton
> 
> http://codereview.chromium.org/6623073/
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may
contain
confidential information.  Any unauthorized review, use, disclosure or
distribution
is prohibited.  If you are not the intended recipient, please contact the sender
by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

[Tom Warren, 2011-03-11 15:45:11]

Much better, thanks, Jim!

LGTM.

[robotboy, 2011-03-11 22:45:33]

Tom, does this work for you when booting from SPI now?  Or were you LGTMing the
NAND_CMD_xx work?

Thanks,
    Anton

[Tom Warren, 2011-03-11 23:17:37]

Sorry, just the NAND_CMD stuff. I'm on T30 right now, so I'm not doing
any building/testing on Seaboard.

I think Jim's still looking into the NAND init stuff - possibly
PLLs/clocks, DMA, AHB/APB setup, etc. that differs between SPI and
NAND bootrom init. None of that was fixed in his latest patchset,
AFAICT.

On Fri, Mar 11, 2011 at 3:45 PM,  <robotboy@chromium.org> wrote:
> Tom, does this work for you when booting from SPI now?  Or were you LGTMing
> the
> NAND_CMD_xx work?
>
> Thanks,
>    Anton
>
> http://codereview.chromium.org/6623073/
>

[robotboy, 2011-03-11 23:26:47]

OK, thanks.  That's what I was assuming, just wanted to make sure.

Thanks again,
    Anton

On Fri, Mar 11, 2011 at 3:17 PM, Tom Warren <twarren.nvidia@gmail.com>wrote:

> Sorry, just the NAND_CMD stuff. I'm on T30 right now, so I'm not doing
> any building/testing on Seaboard.
>
> I think Jim's still looking into the NAND init stuff - possibly
> PLLs/clocks, DMA, AHB/APB setup, etc. that differs between SPI and
> NAND bootrom init. None of that was fixed in his latest patchset,
> AFAICT.
>
> On Fri, Mar 11, 2011 at 3:45 PM,  <robotboy@chromium.org> wrote:
> > Tom, does this work for you when booting from SPI now?  Or were you
> LGTMing
> > the
> > NAND_CMD_xx work?
> >
> > Thanks,
> >    Anton
> >
> > http://codereview.chromium.org/6623073/
> >
>

[jimmzhang, 2011-03-16 17:32:03]

I believe when boot selectors are set to NAND, the NAND controller init, ie
clock, pin mux init, is done by BootROM. That is why previous code has problem
to access NAND.

[Tom Warren, 2011-03-16 18:04:35]

A couple of questions about NAND_TIMING. Note that the Hynix part is used on
Harmony, but a Toshiba NAND is used on Seaboard (another reason I don't like
having the NAND part# hard-coded in the directory name). Are you sure the 2
parts are identical in timing/params/clocks, etc.? I'm trying to get the 2
datasheets from DeanS.

http://codereview.chromium.org/6623073/diff/27001/board/tegra2/common/nand/HY...
File board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c (right):

http://codereview.chromium.org/6623073/diff/27001/board/tegra2/common/nand/HY...
board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c:421: 
That doesn't sound right. PLLP is set to 216MHz in the UART init code in U-Boot,
so PLLP_OUT0 should be 216MHz. /4 and you get 54MHz.1.5MHz seems awful slow. I
found a NAND BCT that shows a 108MHz clock (432 BootROM PLLP/4).

http://codereview.chromium.org/6623073/diff/27001/board/tegra2/common/nand/HY...
board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c:436: 
So NAND_TIMING_0 and NAND_TIMING2_0 are being set to 0 here? Is that correct?
That just seems odd that _all_ Hynix timing values should result in 0's here.
Again, in the NAND BCT I found the timing values are 0xA and 0x3b269213 (for a
108MHz clock).

If it's correct, why not just have a comment saying 'TRP/TWB/TCR/etc. timings
are all 0 for Hynix Hy27UF4G2B' and just do a RegVal = 0?

[robotboy, 2011-03-16 22:51:34]

I agree with Tom, it seems strange to have this driver have a NAND specific name
(since Seaboard and Harmony use different parts).

Could we move the NAND part specific values to config values (#defined in the
config files) and make the driver more generic.

Thanks,
    Anton

http://codereview.chromium.org/6623073/diff/27001/board/tegra2/common/nand/HY...
File board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c (right):

http://codereview.chromium.org/6623073/diff/27001/board/tegra2/common/nand/HY...
board/tegra2/common/nand/HY27UF084G2B/tegra2_nand.c:436: 
On 2011/03/16 18:04:35, Tom Warren wrote:
> So NAND_TIMING_0 and NAND_TIMING2_0 are being set to 0 here? Is that correct?
> That just seems odd that _all_ Hynix timing values should result in 0's here.
> Again, in the NAND BCT I found the timing values are 0xA and 0x3b269213 (for a
> 108MHz clock).
> 
> If it's correct, why not just have a comment saying 'TRP/TWB/TCR/etc. timings
> are all 0 for Hynix Hy27UF4G2B' and just do a RegVal = 0?

I agree, this seems wrong.  These values should probably be defines that are set
in per-board config files.

That way we could copy the values from the BCT config files to the U-Boot config
files.

[jimmzhang, 2011-03-17 00:40:25]

On 2011/03/16 17:32:03, jimmzhang wrote:
typo. It should be "previous code has no problem to access NAND".

[Tom Warren, 2011-03-17 16:28:49]

Looking better, Jim. Thanks.

Just one more change (see my comments in tegra2_seaboard.h) to clean this up and
make it future-proof and easier to push upstream later.

Tom

http://codereview.chromium.org/6623073/diff/20002/include/configs/tegra2_seab...
File include/configs/tegra2_seaboard.h (right):

http://codereview.chromium.org/6623073/diff/20002/include/configs/tegra2_seab...
include/configs/tegra2_seaboard.h:168: /* For HYNIX HY27UF4G2B
The NAND BCT CFG files for Seaboard that I've been able to find all run at
108MHz. Don't we want max speed out of NAND?
This will change the timings here, so the divisor should probably be declared as
a CONFIG option, too.

I'd also like to see these comments/NAND_TIMING #defines in the tegra2_nand.h
header (under an #ifdef CONFIG_TEGRA2_NAND_HYNIX), and something like
CONFIG_TEGRA2_NAND_HYNIX defined here to enable it. Later, if another variant is
built with a different NAND chip (Toshiba, etc.), we just need to define
CONFIG_TEGRA2_NAND_TOSHIBA here, and provide the timings in the NAND driver
header as per the (Toshiba) data sheet.

http://codereview.chromium.org/6623073/diff/20002/include/configs/tegra2_seab...
include/configs/tegra2_seaboard.h:184: #define CONFIG_TEGRA2_NAND_tR_US	25
Where is this used?

[robotboy, 2011-03-17 17:50:21]

Looking good, there are a couple of things that need to be done to make this
match TOT U-Boot.  Since this review started the board/tegra2/common directory
has merged with the board/tegra2/generic directory.  Also, the
board/tegra2/harmony directory is no more.  Please rebase to TOT.

Thanks,
    Anton

[jilin, 2011-03-18 10:42:03]

"include/configs/tegra2_seaboard.h:184: #define CONFIG_TEGRA2_NAND_tR_US	25
Where is this used?"

Tom,
I have removed this fixed delay after sending READ1 command.

[robotboy, 2011-03-18 18:17:32]

We're getting there.  :)  A few comments below.  Thanks for your patience.

-Anton

http://codereview.chromium.org/6623073/diff/31003/board/tegra2/common/nand/te...
File board/tegra2/common/nand/tegra2_nand.h (right):

http://codereview.chromium.org/6623073/diff/31003/board/tegra2/common/nand/te...
board/tegra2/common/nand/tegra2_nand.h:140: #define
CONFIG_TEGRA2_NAND_TIMING2	0x01
Actually, what I was hoping for was that the values of
CONFIG_TEGRA2_NAND_TIMING(2) be set in the config files, not here.  There is no
need to put the HYNIX values in this generic header file.

http://codereview.chromium.org/6623073/diff/31003/board/tegra2/generic/Makefile
File board/tegra2/generic/Makefile (right):

http://codereview.chromium.org/6623073/diff/31003/board/tegra2/generic/Makefi...
board/tegra2/generic/Makefile:47: endif
This looks like a rebase error.  Please take a look later in this file for the
COBJS-$(CONFIG_XXX) method of conditionally including an object and use that for
the NAND driver.

http://codereview.chromium.org/6623073/diff/31003/include/configs/tegra2_harm...
File include/configs/tegra2_harmony.h (left):

http://codereview.chromium.org/6623073/diff/31003/include/configs/tegra2_harm...
include/configs/tegra2_harmony.h:159: 
Why remove this line?

[robotboy, 2011-03-21 16:50:12]

Please remove the board/tegra2/common files (this directory is no more).  Also,
please remove the changes to the files in include/configs/chromeos/*.  I will
make the required changes there based on the changes in the
include/configs/tegra2*.h files in your CL.  The changes in the chromeos
specific configs are a little trickier because I don't want to turn on NAND
support for all configs.

Thanks,
    Anton

http://codereview.chromium.org/6623073/diff/36031/board/tegra2/common/nand/te...
File board/tegra2/common/nand/tegra2_nand.c (right):

http://codereview.chromium.org/6623073/diff/36031/board/tegra2/common/nand/te...
board/tegra2/common/nand/tegra2_nand.c:1: /*
And please remove this file as well.

http://codereview.chromium.org/6623073/diff/36031/board/tegra2/common/nand/te...
File board/tegra2/common/nand/tegra2_nand.h (right):

http://codereview.chromium.org/6623073/diff/36031/board/tegra2/common/nand/te...
board/tegra2/common/nand/tegra2_nand.h:1: /* register offset */
Please remove this file.

[robotboy, 2011-03-23 17:48:18]

LGTM, I'll push this minus the changes to the chromeos config files.  I'll make
the appropriate changes to those files in a separate CL.

Thanks,
    Anton