No subject

[<bogus@does.not.exist.com>]

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, &reg_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