[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver
Simon Glass
sjg at chromium.org
Fri Apr 13 19:42:51 CEST 2012
Hi Scott,
On Fri, Jan 20, 2012 at 2:55 PM, Scott Wood <scottwood at freescale.com> wrote:
> On 01/13/2012 05:10 PM, Simon Glass wrote:
>> +/* Information about an attached NAND chip */
>> +struct fdt_nand {
>> + struct nand_ctlr *reg;
>> + int enabled; /* 1 to enable, 0 to disable */
>> + struct fdt_gpio_state wp_gpio; /* write-protect GPIO */
>> + int width; /* bit width, normally 8 */
>> + int tag_ecc_bytes; /* ECC bytes to be generated for tag bytes */
>> + int tag_bytes; /* Tag bytes in spare area */
>> + int data_ecc_bytes; /* ECC bytes for data area */
>> + int skipped_spare_bytes;
>> + /*
>> + * How many bytes in spare area:
>> + * spare area = skipped bytes + ECC bytes of data area
>> + * + tag bytes + ECC bytes of tag bytes
>> + */
>> + int page_spare_bytes;
>> + int page_data_bytes; /* Bytes in data area */
>> + unsigned timing[FDT_NAND_TIMING_COUNT];
>
> s/unsigned/u32/g
Done
>
> Likewise, any of these other fields that correspond to device tree
> fields should be u32 or s32.
Done
>
> ...and even when you do mean "unsigned int", please spell it out.
>
>> +struct nand_info {
>
> Please do not define "struct nand_info" when there is already a
> different "nand_info_t".
Done
>
>> +struct nand_info nand_ctrl;
>
> static (or better, dynamic).
Done, what is dynamic?
>
>> +/**
>> + * Wait for command completion
>> + *
>> + * @param reg nand_ctlr structure
>> + * @return
>> + * 1 - Command completed
>> + * 0 - Timeout
>> + */
>> +static int nand_waitfor_cmd_completion(struct nand_ctlr *reg)
>> +{
>> + int i;
>> + u32 reg_val;
>> +
>> + for (i = 0; i < NAND_CMD_TIMEOUT_MS * 1000; i++) {
>> + if ((readl(®->command) & CMD_GO) ||
>> + !(readl(®->status) &
>> + STATUS_RBSY0) ||
>> + !(readl(®->isr) &
>> + ISR_IS_CMD_DONE)) {
>> + udelay(1);
>> + continue;
>> + }
>> + reg_val = readl(®->dma_mst_ctrl);
>> + /*
>> + * If DMA_MST_CTRL_EN_A_ENABLE or
>> + * DMA_MST_CTRL_EN_B_ENABLE is set,
>> + * that means DMA engine is running, then we
>> + * have to wait until
>> + * DMA_MST_CTRL_IS_DMA_DONE
>> + * is cleared for DMA transfer completion.
>> + */
>> + if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE |
>> + DMA_MST_CTRL_EN_B_ENABLE)) {
>> + if (reg_val & DMA_MST_CTRL_IS_DMA_DONE)
>> + return 1;
>
> Please don't line up continuation lines with the if-body.
>
> E.g. either line up DMA_MST_CTRL_EN_B_ENABLE with
> DMA_MST_CTRL_EN_A_ENABLE (my preference), or just add another tab (if
> you want to be a tabs-only purist).
Yes, ick, have fixed that.
>
>> + } else
>> + return 1;
>
> If braces are used on one side of the if/else, use on both sides.
Done
>
>> + udelay(1);
>> + }
>> + return 0;
>> +}
>> +
>> +/**
>> + * [DEFAULT] Read one byte from the chip
>> + *
>> + * @param mtd MTD device structure
>> + * @return data byte
>> + *
>> + * Default read function for 8bit bus-width
>> + */
>> +static uint8_t read_byte(struct mtd_info *mtd)
>> +{
>> + struct nand_chip *chip = mtd->priv;
>> + int dword_read;
>
> s/int/u32/
done
>
>> + struct nand_info *info;
>> +
>> + info = (struct nand_info *) chip->priv;
>> +
>> + dword_read = readl(&info->reg->resp);
>> + dword_read = dword_read >> (8 * info->pio_byte_index);
>> + info->pio_byte_index++;
>> + return (uint8_t) dword_read;
>
> No space after casts.
I think I have fixed that globally.
>
> What happens when pio_byte_index > 3? Please don't assume that upper
> bits will be ignored, even if that happens to be true on this chip.
I added an assert() - there is no way to return an error here. I'm not
sure what you mean by upper bits - there is a uint8_t cast.
>
> How does info->reg->resp work? You don't seem to be choosing the dword
> to read based on pio_byte_index, which suggests that the act of reading
> resp changes what will be read the next time. But, you read it on each
> byte, which would advance resp four times too fast if it's simply
> returning a new dword each time.
>
> If the hardware is really auto-advancing resp only on every fourth
> access, that needs a comment.
I was just looking at that. Have added a comment.
>
>> +/* Hardware specific access to control-lines */
>> +static void nand_hwcontrol(struct mtd_info *mtd, int cmd,
>> + unsigned int ctrl)
>> +{
>> +}
>
> Don't implement this at all if it doesn't make sense for this hardware.
It isn't implemented (the function is empty), but if it is not defined
then the NAND layer will die when it calls a null function. What
should I do here?
>
>> +
>> +/**
>> + * Clear all interrupt status bits
>> + *
>> + * @param reg nand_ctlr structure
>> + */
>> +static void nand_clear_interrupt_status(struct nand_ctlr *reg)
>> +{
>> + u32 reg_val;
>> +
>> + /* Clear interrupt status */
>> + reg_val = readl(®->isr);
>> + writel(reg_val, ®->isr);
>> +}
>> +
>> +/**
>> + * [DEFAULT] Send command to NAND device
>> + *
>> + * @param mtd MTD device structure
>> + * @param command the command to be sent
>> + * @param column the column address for this command, -1 if none
>> + * @param page_addr the page address for this command, -1 if none
>> + */
>> +static void nand_command(struct mtd_info *mtd, unsigned int command,
>> + int column, int page_addr)
>> +{
>> + register struct nand_chip *chip = mtd->priv;
>
> Are you really seeing any difference by using the register keyword?
I doubt it, will remove globally.
>
>> + struct nand_info *info;
>> +
>> + info = (struct nand_info *) chip->priv;
>> +
>> + /*
>> + * Write out the command to the device.
>> + */
>> + if (mtd->writesize < 2048) {
>> + /*
>> + * Only command NAND_CMD_RESET or NAND_CMD_READID will come
>> + * here before mtd->writesize is initialized, we don't have
>> + * any action here because page size of NAND HY27UF084G2B
>> + * is 2048 bytes and mtd->writesize will be 2048 after
>> + * initialized.
>> + */
>
> Why are you assuming a particular NAND chip here?
>
> If you want to not support certain page sizes in this driver, fine, but
> document that somewhere more prominent, and I don't see why this test is
> needed.
Yes I agree, will punt it.
>
>> + } else {
>> + /* Emulate NAND_CMD_READOOB */
>> + if (command == NAND_CMD_READOOB) {
>> + column += mtd->writesize;
>> + command = NAND_CMD_READ0;
>> + }
>> +
>> + /* Adjust columns for 16 bit bus-width */
>> + if (column != -1 && (chip->options & NAND_BUSWIDTH_16))
>> + column >>= 1;
>> + }
>
> Why does the treatment of column for NAND_CMD_READID depend on whether
> you've yet filled in mtd->writesize?
Not sure, punted.
>
>> + nand_clear_interrupt_status(info->reg);
>> +
>> + /* Stop DMA engine, clear DMA completion status */
>> + writel(DMA_MST_CTRL_EN_A_DISABLE
>> + | DMA_MST_CTRL_EN_B_DISABLE
>> + | DMA_MST_CTRL_IS_DMA_DONE,
>> + &info->reg->dma_mst_ctrl);
>> +
>> + /*
>> + * Program and erase have their own busy handlers
>> + * status and sequential in needs no delay
>> + */
>> + switch (command) {
>> + case NAND_CMD_READID:
>> + writel(NAND_CMD_READID, &info->reg->cmd_reg1);
>> + writel(CMD_GO | CMD_CLE | CMD_ALE | CMD_PIO
>> + | CMD_RX |
>> + (CMD_TRANS_SIZE_BYTES4 << CMD_TRANS_SIZE_SHIFT)
>> + | CMD_CE0,
>> + &info->reg->command);
>> + info->pio_byte_index = 0;
>> + break;
>
> Looks like you don't use column at all for READID -- no ONFI support, I
> guess.
Yes.
>
>> + case NAND_CMD_READ0:
>> + writel(NAND_CMD_READ0, &info->reg->cmd_reg1);
>> + writel(NAND_CMD_READSTART, &info->reg->cmd_reg2);
>> + writel((page_addr << 16) | (column & 0xFFFF),
>> + &info->reg->addr_reg1);
>> + writel(page_addr >> 16, &info->reg->addr_reg2);
>> + return;
>> + case NAND_CMD_SEQIN:
>> + writel(NAND_CMD_SEQIN, &info->reg->cmd_reg1);
>> + writel(NAND_CMD_PAGEPROG, &info->reg->cmd_reg2);
>> + writel((page_addr << 16) | (column & 0xFFFF),
>> + &info->reg->addr_reg1);
>> + writel(page_addr >> 16,
>> + &info->reg->addr_reg2);
>> + return;
>> + case NAND_CMD_PAGEPROG:
>> + return;
>> + case NAND_CMD_ERASE1:
>> + writel(NAND_CMD_ERASE1, &info->reg->cmd_reg1);
>> + writel(NAND_CMD_ERASE2, &info->reg->cmd_reg2);
>> + writel(page_addr, &info->reg->addr_reg1);
>> + writel(CMD_GO | CMD_CLE | CMD_ALE |
>> + CMD_SEC_CMD | CMD_CE0 | CMD_ALE_BYTES3,
>> + &info->reg->command);
>> + break;
>> + case NAND_CMD_RNDOUT:
>> + return;
>
> Don't silently ignore RNDOUT -- if you don't support it and it happens,
> complain loudly.
OK, I added a printf(), that should be loud enough. Would prefer a
debug() though.
>
>> + case NAND_CMD_ERASE2:
>> + return;
>> + case NAND_CMD_STATUS:
>> + writel(NAND_CMD_STATUS, &info->reg->cmd_reg1);
>> + writel(CMD_GO | CMD_CLE | CMD_PIO | CMD_RX
>> + | (CMD_TRANS_SIZE_BYTES1 <<
>> + CMD_TRANS_SIZE_SHIFT)
>> + | CMD_CE0,
>> + &info->reg->command);
>> + info->pio_byte_index = 0;
>> + break;
>> + case NAND_CMD_RESET:
>> + writel(NAND_CMD_RESET, &info->reg->cmd_reg1);
>> + writel(CMD_GO | CMD_CLE | CMD_CE0,
>> + &info->reg->command);
>> + break;
>> + default:
>> + return;
>
> Likewise, complain if you get an unrecognized command.
Done - I wonder why we can't just return an error?
>
>> +/**
>> + * Set up NAND bus width and page size
>> + *
>> + * @param info nand_info structure
>> + * @param *reg_val address of reg_val
>> + * @return none - value is set in reg_val
>> + */
>> +static void set_bus_width_page_size(struct fdt_nand *config,
>> + u32 *reg_val)
>> +{
>> + if (config->width == 8)
>> + *reg_val = CFG_BUS_WIDTH_8BIT;
>> + else
>> + *reg_val = CFG_BUS_WIDTH_16BIT;
>> +
>> + if (config->page_data_bytes == 256)
>> + *reg_val |= CFG_PAGE_SIZE_256;
>> + else if (config->page_data_bytes == 512)
>> + *reg_val |= CFG_PAGE_SIZE_512;
>> + else if (config->page_data_bytes == 1024)
>> + *reg_val |= CFG_PAGE_SIZE_1024;
>> + else if (config->page_data_bytes == 2048)
>> + *reg_val |= CFG_PAGE_SIZE_2048;
>
> Really, page sizes of 256 and 1024 bytes?
>
> From elsewhere in the driver you only support 2048 byte pages, so why
> not just check for that and error (not silently continue) if it's
> anything else?
I don't think it is that bad - I believe the driver should all the
options, although I have not tested it or gone into it carefully. Have
added error checking.
>
>> +}
>> +
>> +/**
>> + * Page read/write function
>> + *
>> + * @param mtd mtd info structure
>> + * @param chip nand chip info structure
>> + * @param buf data buffer
>> + * @param page page number
>> + * @param with_ecc 1 to enable ECC, 0 to disable ECC
>> + * @param is_writing 0 for read, 1 for write
>> + * @return 0 when successfully completed
>> + * -EIO when command timeout
>> + */
>> +static int nand_rw_page(struct mtd_info *mtd, struct nand_chip *chip,
>> + uint8_t *buf, int page, int with_ecc, int is_writing)
>> +{
>> + u32 reg_val;
>> + int tag_size;
>> + struct nand_oobfree *free = chip->ecc.layout->oobfree;
>> + /* 128 is larger than the value that our HW can support. */
>> + u32 tag_buf[128];
>> + char *tag_ptr;
>> + struct nand_info *info;
>> + struct fdt_nand *config;
>> +
>> + if (((int) buf) & 0x03) {
>
> s/int/uintptr_t/ (or unsigned long)
done
>
>> + printf("buf 0x%X has to be 4-byte aligned\n", (u32) buf);
>
> %p
done
>
>> + set_bus_width_page_size(&info->config, ®_val);
>
> You need to set up the bus width and page size on every page transfer?
> Why not figure out the value to write in the register at init time (thus
> making it a good place to reject unsupported configurations)?
Done, added a check to the init.
>
>> + /* Set ECC selection, configure ECC settings */
>> + if (with_ecc) {
>> + tag_size = config->tag_bytes + config->tag_ecc_bytes;
>> + reg_val |= (CFG_SKIP_SPARE_SEL_4
>> + | CFG_SKIP_SPARE_ENABLE
>> + | CFG_HW_ECC_CORRECTION_ENABLE
>> + | CFG_ECC_EN_TAG_DISABLE
>> + | CFG_HW_ECC_SEL_RS
>> + | CFG_HW_ECC_ENABLE
>> + | CFG_TVAL4
>> + | (tag_size - 1));
>> +
>> + if (!is_writing) {
>> + tag_size += config->skipped_spare_bytes;
>> + invalidate_dcache_range((unsigned long) tag_ptr,
>> + ((unsigned long) tag_ptr) + tag_size);
>> + } else
>> + flush_dcache_range((unsigned long) tag_ptr,
>> + ((unsigned long) tag_ptr) + tag_size);
>> + } else {
>> + tag_size = mtd->oobsize;
>> + reg_val |= (CFG_SKIP_SPARE_DISABLE
>> + | CFG_HW_ECC_CORRECTION_DISABLE
>> + | CFG_ECC_EN_TAG_DISABLE
>> + | CFG_HW_ECC_DISABLE
>> + | (tag_size - 1));
>> + if (!is_writing) {
>> + invalidate_dcache_range((unsigned long) chip->oob_poi,
>> + ((unsigned long) chip->oob_poi) + tag_size);
>> + } else {
>> + flush_dcache_range((unsigned long) chip->oob_poi,
>> + ((unsigned long) chip->oob_poi) + tag_size);
>> + }
>> + }
>> + writel(reg_val, &info->reg->config);
>> +
>> + if (!is_writing) {
>> + invalidate_dcache_range((unsigned long) buf,
>> + ((unsigned long) buf) +
>> + (1 << chip->page_shift));
>> + } else {
>> + flush_dcache_range((unsigned long) buf,
>> + ((unsigned long) buf) +
>> + (1 << chip->page_shift));
>> + }
>
> Please factor out all this cache stuff into a dma prepare function that
> takes start, length, and is_writing. Is it even really needed to
> invalidate rather than flush in the read case? It doesn't look like
> these buffers are even going to be cacheline-aligned...
Done
I am not keen on adding cache alignment into every driver - IMO this
should happen at the level above as with MMC, USB, etc. A fairly
trivial fix to nand_base caches most of the problems. I will include a
patch in my next series. Anyway for now, Tegra has cache off because
of all these warnings.
>
>> +int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)
>> +{
>> + int err;
>> +
>> + config->reg = (struct nand_ctlr *)fdtdec_get_addr(blob, node, "reg");
>> + config->enabled = fdtdec_get_is_enabled(blob, node);
>> + config->width = fdtdec_get_int(blob, node, "width", 8);
>> + err = fdtdec_decode_gpio(blob, node, "wp-gpios", &config->wp_gpio);
>> + if (err)
>> + return err;
>> + err = fdtdec_get_int_array(blob, node, "nvidia,timing",
>> + config->timing, FDT_NAND_TIMING_COUNT);
>> + if (err < 0)
>> + return err;
>> +
>> + /* Now look up the controller and decode that */
>> + node = fdt_next_node(blob, node, NULL);
>> + if (node < 0)
>> + return node;
>> +
>> + config->page_data_bytes = fdtdec_get_int(blob, node,
>> + "page-data-bytes", -1);
>> + config->tag_ecc_bytes = fdtdec_get_int(blob, node,
>> + "tag-ecc-bytes", -1);
>> + config->tag_bytes = fdtdec_get_int(blob, node, "tag-bytes", -1);
>> + config->data_ecc_bytes = fdtdec_get_int(blob, node,
>> + "data-ecc-bytes", -1);
>> + config->skipped_spare_bytes = fdtdec_get_int(blob, node,
>> + "skipped-spare-bytes", -1);
>> + config->page_spare_bytes = fdtdec_get_int(blob, node,
>> + "page-spare-bytes", -1);
>
> Has this binding been accepted into Linux's documentation or another
> canonical source?
No
>
> Usually vendor prefixes are asked for on such properties (similar to
> "nvidia,timing").
Done
>
>> +/* Oh dear I suspect no one will like these Bit values? */
>
> Indeed.
>
>> +enum {
>> + Bit0 = 1 << 0,
>> + Bit1 = 1 << 1,
>> + Bit2 = 1 << 2,
>
> Please just open code this in the functional definitions.
Done
>
>> +enum {
>> + CMD_TRANS_SIZE_BYTES1 = 0,
>> + CMD_TRANS_SIZE_BYTES2,
>> + CMD_TRANS_SIZE_BYTES3,
>> + CMD_TRANS_SIZE_BYTES4,
>> + CMD_TRANS_SIZE_BYTES5,
>> + CMD_TRANS_SIZE_BYTES6,
>> + CMD_TRANS_SIZE_BYTES7,
>> + CMD_TRANS_SIZE_BYTES8,
>> + CMD_TRANS_SIZE_BYTES_PAGE_SIZE_SEL
>> +};
>
> Why not just use the number of bytes directly, minus one?
Done, except for the last one which makes sense as a define I think.
>
> -Scott
>
Regards,
Simon
More information about the U-Boot
mailing list