[U-Boot] [PATCH 2/2] nand: sunxi: Add support for booting from internal NAND memory

[Roy Spliet]

Dear Scott,

Thank you for taking your time to feedback. However, it seems to be 
about a week and two versions of the patchset too late. Most of your 
issues have been addressed in the meanwhile, as you can see in Hans de 
Goede's sunxi branch.
Yours,

Roy

Op 02-06-15 om 00:14 schreef Scott Wood:
> 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
>


-- 


IMAGINE IT >> MAKE IT

Meet us online at Twitter <http://twitter.com/ultimaker>, Facebook 
<http://facebook.com/ultimaker>, Google+ <http://google.com/+Ultimaker>

www.ultimaker.com