No subject
not just check for that and error (not silently continue) if it's anything else?
Quoted 17 lines
(quoted diff) +17
-0+} + +/** + * 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) +{
Review
Quoted 10 lines
(quoted diff) +10
-0+ 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)
Review
Quoted 1 line
(quoted diff) +1
-0+ printf("buf 0x%X has to be 4-byte aligned\n", (u32) buf);
%p
Review
Quoted 1 line
(quoted diff) +1
-0+ 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)?
Quoted 35 lines
(quoted diff) +35
-0+ /* 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);
Review
Quoted 10 lines
(quoted diff) +10
-0+ + 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...
Quoted 21 lines
(quoted diff) +21
-0+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,
Review
Quoted 10 lines
(quoted diff) +10
-0+ "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").
Quoted 1 line
+/* Oh dear I suspect no one will like these Bit values? */
Indeed.
Review
Quoted 4 lines
(quoted diff) +4
-0+enum { + Bit0 = 1 << 0, + Bit1 = 1 << 1, + Bit2 = 1 << 2,
Please just open code this in the functional definitions.
Review
Quoted 11 lines
(quoted diff) +11
-0+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