From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Mon, 1 Jun 2015 17:14:19 -0500
Subject: [U-Boot] [PATCH 2/2] nand: sunxi: Add support for booting from
 internal NAND memory
In-Reply-To: <1432216765-8421-3-git-send-email-r.spliet@ultimaker.com>
References: <1432216765-8421-1-git-send-email-r.spliet@ultimaker.com>
 <1432216765-8421-3-git-send-email-r.spliet@ultimaker.com>
Message-ID: <1433196859.4089.193.camel@freescale.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Thu, 2015-05-21 at 15:59 +0200, Roy Spliet wrote:
> +#ifdef CONFIG_NAND_SUNXI
> +#include <nand.h>
> +#endif

Why do you need the ifdef?

+#include <common.h>
> +#include <config.h>
> +#include <asm/io.h>
> +#include <nand.h>
> +
> +/* DMAC */
> +#define DMAC_BASE                    0x01c02000
> +#define DMAC_REG(a)                  (DMAC_BASE + a)
> +
> +#define DMAC_INT                     DMAC_REG(0x000)
> +#define DMAC_DDMA_CFG                        DMAC_REG(0x300)
> +#define DMAC_DDMA_SRC                        DMAC_REG(0x304)
> +#define DMAC_DDMA_DST                        DMAC_REG(0x308)
> +#define DMAC_DDMA_BYTE_COUNT         DMAC_REG(0x30C)
> +#define DMAC_DDMA_PARAM                      DMAC_REG(0x318)
> +
> +/* NAND controller */
> +#define NANDFLASHC_BASE                      0x01c03000
> +#define NREG(a)                              (0x01c03000 + a)
> +
> +#define NANDFLASHC_CTL                       NREG(0x00)
> +#define NANDFLASHC_CTL_EN            0x00000001
> +#define NANDFLASHC_CTL_RST           0x00000002
> +#define NANDFLASHC_CTL_RAM_METHOD    0x00004000
> +
> +#define NANDFLASHC_ST                        NREG(0x004)
> +#define NANDFLASHC_INT                       NREG(0x008)
> +#define NANDFLASHC_TIMING_CTL                NREG(0x00C)
> +#define NANDFLASHC_TIMING_CFG                NREG(0x010)
> +#define NANDFLASHC_ADDR_LOW          NREG(0x014)
> +#define NANDFLASHC_ADDR_HIGH         NREG(0x018)
> +#define NANDFLASHC_SECTOR_NUM                NREG(0x01C)
> +#define NANDFLASHC_CNT                       NREG(0x020)
> +
> +#define NANDFLASHC_CMD                       NREG(0x024)
> +#define NANDFLASHC_SEND_CMD1         (1 << 22)
> +#define NANDFLASHC_WAIT_FLAG         (1 << 23)
> +
> +#define NANDFLASHC_RCMD_SET          NREG(0x028)
> +#define NANDFLASHC_WCMD_SET          NREG(0x02C)
> +#define NANDFLASHC_IO_DATA           NREG(0x030)
> +#define NANDFLASHC_ECC_CTL           NREG(0x034)
> +#define NANDFLASHC_ECC_ST            NREG(0x038)
> +#define NANDFLASHC_DEBUG             NREG(0x03c)
> +#define NANDFLASHC_ECC_CNT0          NREG(0x040)
> +#define NANDFLASHC_ECC_CNT1          NREG(0x044)
> +#define NANDFLASHC_ECC_CNT2          NREG(0x048)
> +#define NANDFLASHC_ECC_CNT3          NREG(0x04c)
> +#define NANDFLASHC_USER_DATA_BASE    NREG(0x050)
> +#define NANDFLASHC_EFNAND_STATUS     NREG(0x090)
> +#define NANDFLASHC_SPARE_AREA                NREG(0x0A0)
> +#define NANDFLASHC_PATTERN_ID                NREG(0x0A4)
> +#define NANDFLASHC_RAM0_BASE         NREG(0x400)
> +#define NANDFLASHC_RAM1_BASE         NREG(0x800)

Shouldn't these be in a header file so they can be shared with a non-
SPL driver?

> +void
> +nand_init(void)
> +{

Don't put a newline after the return type.

> +     uint32_t val;
> +
> +     board_nand_init();
> +     val = readl(NANDFLASHC_CTL);
> +     val |= NANDFLASHC_CTL_RST;
> +     writel(val, NANDFLASHC_CTL);
> +
> +     /* Wait until reset pin is deasserted */
> +     do {
> +             val = readl(NANDFLASHC_CTL);
> +             if (!(val & NANDFLASHC_CTL_RST))
> +                     break;
> +     } while (1);

Add a timeout to delay loops.

> +
> +     /** \todo Chip select, currently kind of static */
> +     val = readl(NANDFLASHC_CTL);
> +     val &= 0xf0fff0f2;

Don't put magic numbers in the code -- use symbolic constants taht 
describe what the fields mean.

+/* random seed */
> +static const uint16_t random_seed[128] = {
> +     0x2b75, 0x0bd0, 0x5ca3, 0x62d1, 0x1c93, 0x07e9, 0x2162, 0x3a72,
> +     0x0d67, 0x67f9, 0x1be7, 0x077d, 0x032f, 0x0dac, 0x2716, 0x2436,
> +     0x7922, 0x1510, 0x3860, 0x5287, 0x480f, 0x4252, 0x1789, 0x5a2d,
> +     0x2a49, 0x5e10, 0x437f, 0x4b4e, 0x2f45, 0x216e, 0x5cb7, 0x7130,
> +     0x2a3f, 0x60e4, 0x4dc9, 0x0ef0, 0x0f52, 0x1bb9, 0x6211, 0x7a56,
> +     0x226d, 0x4ea7, 0x6f36, 0x3692, 0x38bf, 0x0c62, 0x05eb, 0x4c55,
> +     0x60f4, 0x728c, 0x3b6f, 0x2037, 0x7f69, 0x0936, 0x651a, 0x4ceb,
> +     0x6218, 0x79f3, 0x383f, 0x18d9, 0x4f05, 0x5c82, 0x2912, 0x6f17,
> +     0x6856, 0x5938, 0x1007, 0x61ab, 0x3e7f, 0x57c2, 0x542f, 0x4f62,
> +     0x7454, 0x2eac, 0x7739, 0x42d4, 0x2f90, 0x435a, 0x2e52, 0x2064,
> +     0x637c, 0x66ad, 0x2c90, 0x0bad, 0x759c, 0x0029, 0x0986, 0x7126,
> +     0x1ca7, 0x1605, 0x386a, 0x27f5, 0x1380, 0x6d75, 0x24c3, 0x0f8e,
> +     0x2b7a, 0x1418, 0x1fd1, 0x7dc1, 0x2d8e, 0x43af, 0x2267, 0x7da3,
> +     0x4e3d, 0x1338, 0x50db, 0x454d, 0x764d, 0x40a3, 0x42e6, 0x262b,
> +     0x2d2e, 0x1aea, 0x2e17, 0x173d, 0x3a6e, 0x71bf, 0x25f9, 0x0a5d,
> +     0x7c57, 0x0fbe, 0x46ce, 0x4939, 0x6b17, 0x37bb, 0x3e91, 0x76db,
> +};

Why is randomness needed?

> +uint32_t ecc_errors = 0;

Why is this global?

> +int
> +nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
> +{
> +     dma_addr_t dst_block;
> +     dma_addr_t dst_end;
> +     phys_addr_t addr = offs;
> +
> +     dst_end = ((dma_addr_t) dest) + size;
> +
> +     memset((void *)dest, 0x0, size);

Unnecessary cast.

-Scott