[U-Boot] [PATCH v2 2/4] nand: lpc32xx: add SLC NAND controller support

[Scott Wood]

On Sat, 2015-07-18 at 01:47 +0300, Vladimir Zapolskiy wrote:
> +/* TAC register bits, be aware of overflows */
> +#define TAC_W_RDY(n)         (max_t(uint32_t, (n), 0xF) << 28)
> +#define TAC_W_WIDTH(n)               (max_t(uint32_t, (n), 0xF) << 24)
> +#define TAC_W_HOLD(n)                (max_t(uint32_t, (n), 0xF) << 20)
> +#define TAC_W_SETUP(n)               (max_t(uint32_t, (n), 0xF) << 16)
> +#define TAC_R_RDY(n)         (max_t(uint32_t, (n), 0xF) << 12)
> +#define TAC_R_WIDTH(n)               (max_t(uint32_t, (n), 0xF) << 8)
> +#define TAC_R_HOLD(n)                (max_t(uint32_t, (n), 0xF) << 4)
> +#define TAC_R_SETUP(n)               (max_t(uint32_t, (n), 0xF) << 0)
> +
> +static struct lpc32xx_nand_slc_registers __iomem 
> *lpc32xx_nand_slc_registers
> +     = (struct lpc32xx_nand_slc_registers __iomem *)SLC_NAND_BASE;

s/registers/regs/ might help the formatting here...

> +static void lpc32xx_nand_cmd_ctrl(struct mtd_info *mtd,
> +                               int cmd, unsigned int ctrl)
> +{
> +     debug("ctrl: 0x%08x, cmd: 0x%08x\n", ctrl, cmd);
> +
> +     if (ctrl & NAND_NCE)
> +             setbits_le32(&lpc32xx_nand_slc_registers->cfg, CFG_CE_LOW);
> +     else
> +             clrbits_le32(&lpc32xx_nand_slc_registers->cfg, CFG_CE_LOW);
> +
> +     if (cmd == NAND_CMD_NONE)
> +             return;
> +
> +     if (ctrl & NAND_CLE)
> +             writel(cmd & 0xFF, &lpc32xx_nand_slc_registers->cmd);
> +     else /* if (ctrl & NAND_ALE) */
> +             writel(cmd & 0xFF, &lpc32xx_nand_slc_registers->addr);
> +}

Why is "if (ctrl & NAND_ALE())" commented out?  I think it'd be better if 
wrong uses of this function didn't silently cause address cycles (which could 
hide a bug if ALE is what was desired).

> +static void lpc32xx_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +     while (len-- > 0)
> +             *buf++ = (uint8_t)readl(&lpc32xx_nand_slc_registers->data);
> +}
> +
> +static uint8_t lpc32xx_read_byte(struct mtd_info *mtd)
> +{
> +     return (uint8_t)readl(&lpc32xx_nand_slc_registers->data);
> +}

You've still got a couple unneeded casts here.

> +/*
> + * LPC32xx has only one SLC NAND controller, don't utilize
> + * CONFIG_SYS_NAND_SELF_INIT to be able to reuse this function
> + * both in SPL NAND and U-boot images.
> + */
> +int board_nand_init(struct nand_chip *lpc32xx_chip)
> +{
> +     lpc32xx_chip->cmd_ctrl  = lpc32xx_nand_cmd_ctrl;
> +     lpc32xx_chip->dev_ready = lpc32xx_nand_dev_ready;
> +
> +     /*
> +      * Hardware ECC calculation is not supported by the driver,
> +      * because it requires DMA support, see LPC32x0 User Manual,
> +      * note after SLC_ECC register description (UM10326, p.198)
> +      */
> +     lpc32xx_chip->ecc.mode  = NAND_ECC_SOFT;
> +
> +     /*
> +      * The implementation of these functions is quite common, but
> +      * they MUST be defined, because access to data register
> +      * is strictly 32-bit aligned.
> +      */
> +     lpc32xx_chip->read_buf  = lpc32xx_read_buf;
> +     lpc32xx_chip->read_byte = lpc32xx_read_byte;
> +     lpc32xx_chip->write_buf = lpc32xx_write_buf;
> +     lpc32xx_chip->write_byte        = lpc32xx_write_byte;
> +
> +     /*
> +      * Use default ECC layout, but these values are predefined
> +      * for both small and large page NAND flash devices.
> +      */
> +     lpc32xx_chip->ecc.size  = 256;
> +     lpc32xx_chip->ecc.bytes = 3;
> +     lpc32xx_chip->ecc.strength      = 1;

Please use a space before '=', not a tab.  This doesn't even line up right, 
if that's what you were trying to do...

> +
> +#if defined(CONFIG_SYS_NAND_USE_FLASH_BBT)
> +     lpc32xx_chip->bbt_options       |= NAND_BBT_USE_FLASH;
> +#endif

Likewise

-Scott