[PATCH 8/9] mtd: nand: stm_nand_bch: add support for ST's BCH NAND controller

First off, this patch has several checkpatch warnings, some of which
should be addressed:

Fixed.

quoted

+/* ONFI define 6 timing modes */
+#define ST_NAND_ONFI_TIMING_MODES		6

This is unused?

Removed.

[...]

quoted

+static inline void bch_load_prog_cpu(struct nandi_controller *nandi,
+				     struct bch_prog *prog)
+{
+	void __iomem *dst = nandi->base + NANDBCH_ADDRESS_REG_1;
+	uint32_t *src = (uint32_t *)prog;
+	int i;
+
+	for (i = 0; i < 16; i++) {
+		/* Skip registers marked as "reserved" */
+		if (i != 11 && i != 14)
+			writel(*src, dst);
+		src++;
+		dst += sizeof(uint32_t *);

Are you sure you want to base the increment on the pointer size? This
looks like it might break if ever used on a 64-bit architecture. You're
probably just looking for a constant 4, or maybe sizeof(u32).

Fixed

[...]

quoted

+static int check_erased_page(uint8_t *data, uint32_t page_size, int max_zeros)
+{
+	uint8_t *b = data;
+	int zeros = 0;
+	int i;
+
+	for (i = 0; i < page_size; i++) {
+		zeros += hweight8(~*b++);
+		if (zeros > max_zeros)
+			return -1;
+	}
+
+	if (zeros)
+		memset(data, 0xff, page_size);
+
+	return zeros;
+}

I pointed out some flaws in this function back in July [1]. You said
you'd look into this one [2]. I really don't want to accept yet another
custom, unsound erased-page check if at all possible.

That's not quite true.  I said that I think it doesn't matter about
not taking the OOB area into consideration, which I still believe is
the case.  This controller does not allow access into the OOB area.

quoted

+/* Returns the number of ECC errors, or '-1' for uncorrectable error */
+int bch_read_page(struct nandi_controller *nandi,
+		  loff_t offs,
+		  uint8_t *buf)
+{
+	struct nand_chip *chip = &nandi->info.chip;
+	struct bch_prog *prog = &bch_prog_read_page;
+	uint32_t page_size = nandi->info.mtd.writesize;
+	unsigned long list_phys;

Please use dma_addr_t. This is an intentionally opaque (to some degree)
type.

quoted

+	unsigned long buf_phys;

Ditto.

BTW, is your hardware actually restricted to a 32-bit address space? If
it has some expanded registers for larger physical address ranges, it'd
be good to handle the upper bits of the DMA address when mapping. Or
else, it would be good to reflect this in your driver with
dma_set_mask() I think.

Yes, it's 32bit only.  I will add a call to dma_set_mask() to reflect
this.

... or not.  See below.

quoted

+	uint32_t ecc_err;
+	int ret = 0;
+
+	dev_dbg(nandi->dev, "%s: offs = 0x%012llx\n", __func__, offs);
+
+	BUG_ON(offs & (NANDI_BCH_DMA_ALIGNMENT - 1));
+
+	emiss_nandi_select(nandi, STM_NANDI_BCH);
+
+	nandi_enable_interrupts(nandi, NANDBCH_INT_SEQNODESOVER);
+	reinit_completion(&nandi->seq_completed);
+
+	/* Reset ECC stats */
+	writel(CFG_RESET_ECC_ALL | CFG_ENABLE_AFM,
+	       nandi->base + NANDBCH_CONTROLLER_CFG);
+	writel(CFG_ENABLE_AFM, nandi->base + NANDBCH_CONTROLLER_CFG);
+
+	prog->addr = (uint32_t)((offs >> (chip->page_shift - 8)) & 0xffffff00);
+
+	buf_phys = dma_map_single(NULL, buf, page_size, DMA_FROM_DEVICE);

Why NULL for the first arg? You should use the proper device (which will
help with the 32-bit / 64-bit masking, I think.

Also, dma_map_single() can fail. It's good practice to check the return
value with dma_mapping_error(). Same in a few other places.

If you do not supply the first parameter here, it falls back to
arm_dma_ops, which is what we want.  I guess this is also why we do
not have to set the DMA mask, as we're running on ARM32, rather than
AARCH64.

quoted

+	memset(nandi->buf_list, 0x00, NANDI_BCH_BUF_LIST_SIZE);
+	nandi->buf_list[0] = buf_phys | (nandi->sectors_per_page - 1);
+
+	list_phys = dma_map_single(NULL, nandi->buf_list,
+				   NANDI_BCH_BUF_LIST_SIZE, DMA_TO_DEVICE);

Fixed.

quoted

+
+	writel(list_phys, nandi->base + NANDBCH_BUFFER_LIST_PTR);
+
+	bch_load_prog_cpu(nandi, prog);
+
+	bch_wait_seq(nandi);
+
+	nandi_disable_interrupts(nandi, NANDBCH_INT_SEQNODESOVER);
+
+	dma_unmap_single(NULL, list_phys, NANDI_BCH_BUF_LIST_SIZE,
+			 DMA_TO_DEVICE);
+	dma_unmap_single(NULL, buf_phys, page_size, DMA_FROM_DEVICE);
+
+	/* Use the maximum per-sector ECC count! */
+	ecc_err = readl(nandi->base + NANDBCH_ECC_SCORE_REG_A) & 0xff;
+	if (ecc_err == 0xff) {
+		/*
+		 * Downgrade uncorrectable ECC error for an erased page,
+		 * tolerating 'bch_ecc_strength' bits at zero.
+		 */
+		ret = check_erased_page(buf, page_size, chip->ecc.strength);

I commented in [1] that you don't want to use the full ECC strength
here.

Actually I took your first suggestion:

  "Couldn't this (more straightforwarly) just be chip->ecc.strength?"

... but I have now fixed it up to blindly /2 instead.

[...]

quoted

+static int bch_read(struct mtd_info *mtd, struct nand_chip *chip,
+			 uint8_t *buf, int oob_required, int page)
+{
+	struct nandi_controller *nandi = chip->priv;
+	uint32_t page_size = mtd->writesize;
+	loff_t offs = (loff_t)page * page_size;
+	bool bounce = false;
+	uint8_t *p;
+	int ret;
+
+	if (((unsigned int)buf & (NANDI_BCH_DMA_ALIGNMENT - 1)) ||
+	    (!virt_addr_valid(buf))) /* vmalloc'd buffer! */

If you really need this custom DMA check, can you at least make it the
same as in bch_write_page(), and put it in a common macro / static
inline function?

We absolutely do need it, as the buffers which the framework currently
provide are seldom 64 Byte aligned.  I will provide a static inline
function as requested.

[...]

quoted

+static int bch_mtd_read_oob(struct mtd_info *mtd,
+			    struct nand_chip *chip, int page)
+{
+	BUG();

Are you sure this can't be implemented, even if it's not the expected
mode of operation? That's really unforunate.

It can.  We have a 'special' function for it using the extended
flexible mode.  Trying to upstream this has been hard enough already,
without more crud to deal with.  I will hopefully be adding this
functionality as small, succinct patches subsequently.

quoted

+	return 0;
+}
+
+static int bch_mtd_write_oob(struct mtd_info *mtd,
+			     struct nand_chip *chip, int page)
+{
+	BUG();

Same question.

Same answer.

quoted

+	return 0;
+}
+
+static int bch_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+			     uint8_t *buf, int oob_required, int page)
+{
+	BUG();

Same question.

Same answer.

quoted

+	return 0;
+}
+
+static int bch_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+			      const uint8_t *buf, int oob_required)
+{
+	BUG();

Same question.

Same answer.

quoted

+	return 0;
+}

[...]

quoted

+#ifdef CONFIG_MTD_NAND_STM_BCH_BBT

This #ifdef won't work right when you build the BBT code as a module.
You may need IS_ENABLED(), and you'll have to ensure you can't make this
driver built-in while the BBT code is a module.

One option: just disallow building your BBT code as a module.

Done.

[...]

quoted

+static int remap_named_resource(struct platform_device *pdev,
+				char *name,
+				void __iomem **io_ptr)
+{
+	struct resource *res, *mem;
+	resource_size_t size;
+	void __iomem *p;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
+	if (!res)
+		return -ENXIO;
+
+	size = resource_size(res);
+
+	mem = devm_request_mem_region(&pdev->dev, res->start, size, name);
+	if (!mem)
+		return -EBUSY;
+
+	p = devm_ioremap_nocache(&pdev->dev, res->start, size);
+	if (!p)
+		return -ENOMEM;

Can you use devm_ioremap_resource() for the above several lines? So:

	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
	*io_ptr = devm_ioremap_resource(&pdev->dev, res);
	if (IS_ERR(*io_ptr))
		return PTR_ERR(*io_ptr);

I've been staring at this file so long now I'm missing the simple
stuff.  This is of course the new and improved way of doing this
stuff.  I will update.

[...]

quoted

+static int stm_nand_bch_probe(struct platform_device *pdev)
+{
+	const char *part_probes[] = { "cmdlinepart", "ofpart", NULL, };

I feel like I already mentioned this, but maybe that was a different
driver: you're just using the defaults (see default_mtd_part_types /
parse_mtd_partitions) so you don't need to supply this array. Just pass
NULL to mtd_device_parse_register().

I don't recall you mentioning this before, but I could be wrong.

Now fixed.

[...]

quoted

+	/*
+	 * Configure timing registers
+	 */
+	if (bank && bank->timing_spec) {

It looks like no one actually sets the 'timing_spec' field any more,
since you're not getting this info from the device tree any more. Should
you kill it (and this condition branch)?

Looks that way.

Removed all mention of timing_spec.

[...]

quoted

+	chip->ecc.size = NANDI_BCH_SECTOR_SIZE;
+	chip->ecc.bytes = mtd->oobsize;

While ecc.bytes is not actually used in a meaningful way for your
driver (with HWECC), this looks wrong. ecc.bytes should align with this
code from nand_base.c:

        /*
         * Set the number of read / write steps for one page depending on ECC
         * mode.
         */
        ecc->steps = mtd->writesize / ecc->size;
        if (ecc->steps * ecc->size != mtd->writesize) {
                pr_warn("Invalid ECC parameters\n");
                BUG();
        }
        ecc->total = ecc->steps * ecc->bytes;

Currently, it looks like ecc->total will be larger than oobsize, which
doesn't really make sense.

(Maybe we need a new WARN_ON(ecc->total > mtd->oobsize) in
nand_scan_tail()?)

[...]

quoted

+struct device_node *stm_of_get_partitions_node(struct device_node *np,
+					       int bank_nr)
+{
+	struct device_node *banknp, *partsnp = NULL;
+	char name[10];
+
+	sprintf(name, "bank%d", bank_nr);
+	banknp = of_get_child_by_name(np, name);
+	if (banknp)
+		return NULL;
+
+	partsnp = of_get_child_by_name(banknp, "partitions");
+	of_node_put(banknp);
+
+	return partsnp;
+}
+EXPORT_SYMBOL(stm_of_get_partitions_node);

If you follow my advice on the DT binding structure, you don't need this
helper function at all.

It's gone.

quoted

+/**
+ * stm_of_get_nand_banks - Get nand banks info from a given device node.
+ *
+ * @dev			device pointer to use for devm allocations.
+ * @np			device node of the driver.
+ * @banksptr		double pointer to banks which is allocated
+ *			and filled with bank data.
+ *
+ * Returns a count of banks found in the given device node.
+ *
+ */
+int stm_of_get_nand_banks(struct device *dev, struct device_node *np,
+			  struct stm_nand_bank_data **banksptr)
+{
+	struct stm_nand_bank_data *banks;
+	struct device_node *banknp;
+	int nr_banks = 0;
+
+	if (!np)
+		return -ENODEV;
+
+	nr_banks = of_get_child_count(np);
+	if (!nr_banks) {
+		dev_err(dev, "No NAND banks specified in DT: %s\n",
+		np->full_name);
+		return -EINVAL;
+	}
+
+	*banksptr = devm_kzalloc(dev, sizeof(*banks) * nr_banks, GFP_KERNEL);

Missing an OOM check here.

quoted

+	banks = *banksptr;
+	banknp = NULL;

Is this initialization necessary? You overwrite it with the
for_each_child_of_node() loop.

Both fixed.

quoted

+	for_each_child_of_node(np, banknp) {

If you change the DT binding to require a proper compatible property,
you'll need of_device_is_compatible() here.

I see no reason to allocate a compatible property to the bank
descriptors.  They're not being registered/actioned through
of_platform_populate(), so ...

Also, might the for_each_available_child_of_node() helper be preferable,
so you will properly ignore any "disabled" nodes, if they exist?

Right, good catch.

quoted

+		int bank = 0;
+
+		of_property_read_u32(banknp, "st,nand-csn", &banks[bank].csn);
+
+		if (of_get_nand_bus_width(banknp) == 16)
+			banks[bank].options |= NAND_BUSWIDTH_16;
+		if (of_get_nand_on_flash_bbt(banknp))
+			banks[bank].bbt_options |= NAND_BBT_USE_FLASH;
+
+		banks[bank].nr_partitions = 0;
+		banks[bank].partitions = NULL;
+
+		of_property_read_u32(banknp, "st,nand-timing-relax",
+				     &banks[bank].timing_relax);
+		bank++;
+	}
+
+	return nr_banks;
+}
+EXPORT_SYMBOL(stm_of_get_nand_banks);

diff --git a/drivers/mtd/nand/stm_nand_dt.h b/drivers/mtd/nand/stm_nand_dt.h
new file mode 100644
index 0000000..0d2b920
--- /dev/null
+++ b/drivers/mtd/nand/stm_nand_dt.h

[...]

quoted

+int stm_of_get_nand_banks(struct device *dev, struct device_node *np,
+				 struct stm_nand_bank_data **banksp);

Hmm, really only two functions? And one of those might be not be needed,
as I commented above. I don't think you need a separate *_dt.{c,h} file.
Please merge the two.

Now squashed.

[...]

quoted

+	int			cached_page;	/* page number of page in */
+						/* 'page_buf'             */

You never use this field, except to set it to '-1'. Perhaps kill it? I
doubt you actually will win much by trying to cache pages at the driver
level anyway. It's pretty easy to get wrong too.

Gone.

[...]

quoted

+extern int flex_read_raw(struct nandi_controller *nandi,
+			 uint32_t page_addr,
+			 uint32_t col_addr,
+			 uint8_t *buf, uint32_t len);
+extern uint8_t bch_write_page(struct nandi_controller *nandi,
+			      loff_t offs, const uint8_t *buf);
+extern uint8_t bch_erase_block(struct nandi_controller *nandi,
+			       loff_t offs);
+extern int bch_read_page(struct nandi_controller *nandi,
+			 loff_t offs, uint8_t *buf);

This isn't exactly what I had in mind when I suggested the BBT
implementation be separated from the NAND driver. I don't expect a
driver to export its internal functions so that the BBT code can link to
them. I expect the BBT implementation to use indirected versions.

Particularly, we already had discussion of using the mtd_*() API
helpers, although there was some conflicting opinion there. At a
minimum, I think your BBT driver can use the nand_chip function
callbacks.

Basically, I think most / all of this header could be disintegrated and
each driver be written in a more modular fashion. But anyway, if you
take the first step of removing these exported functions, I think we can
live with the rest.

No longer exported.

quoted

+#define EMISS_NAND_CONFIG_HAMMING_NOT_BCH 	(0x1 << 6)
+
+static inline void emiss_nandi_select(struct nandi_controller *nandi,
+				      enum nandi_controllers controller)
+{
+	unsigned v;
+
+	v = readl(nandi->emisscfg);
+
+	if (controller == STM_NANDI_HAMMING) {
+		if (v & EMISS_NAND_CONFIG_HAMMING_NOT_BCH)
+			return;
+		v |= EMISS_NAND_CONFIG_HAMMING_NOT_BCH;
+	} else {
+		if (!(v & EMISS_NAND_CONFIG_HAMMING_NOT_BCH))
+			return;
+		v &= ~EMISS_NAND_CONFIG_HAMMING_NOT_BCH;
+	}
+
+	writel(v, nandi->emisscfg);
+	readl(nandi->emisscfg);
+}

Any particular reason this function is in the header? It's only used in
one file.

Yes, there are potentially two other drivers (that hopefully some
other poor sap will try to upstream). :D

quoted

+#endif /* __LINUX_STM_NAND_H */

Brian

[1] http://lists.infradead.org/pipermail/linux-mtd/2014-July/054500.html
[2] http://lists.infradead.org/pipermail/linux-mtd/2014-July/054881.html

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog