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
Likewise, any of these other fields that correspond to device tree
fields should be u32 or s32.
...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".
> +struct nand_info nand_ctrl;
static (or better, 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).
> + } else
> + return 1;
If braces are used on one side of the if/else, use on both sides.
> + 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/
> + 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.
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.
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.
> +/* 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.
> +
> +/**
> + * 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?
> + 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.
> + } 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?
> + 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.
> + 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.
> + 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.
> +/**
> + * 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?
> +}
> +
> +/**
> + * 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)
> + printf("buf 0x%X has to be 4-byte aligned\n", (u32) buf);
%p
> + 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)?
> + /* 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...
> +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?
Usually vendor prefixes are asked for on such properties (similar to
"nvidia,timing").
> +/* 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.
> +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?
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
