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

[Vladimir Zapolskiy]

Hello Scott,

On 18.07.2015 02:12, Scott Wood wrote:
> 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...

it didn't noticeably help, but I changed the name.

>> +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).

well, wrong users are expected to be fixed out of this scope, but okay,
for sake of clarity and sanity testing and at the acceptable cost of one
more instruction let me uncomment the second condition.

>> +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.

My fault, fixed now.

>> +/*
>> + * 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...

Right, that's my intention. I have no objection to non-leading spaces.

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

Done.

I'd like to send the updated version v3 as a reply to this email,
hopefully this is fine for you, otherwise please let me know.

Thank you for momentary review.

--
With best wishes,
Vladimir