Re: [PATCH v5] mtd: nand: tango: import driver for tango chips

[Mason <slash.tmp@free.fr>]

On 19/09/2016 19:06, Boris Brezillon wrote:

> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote:
> 
>> This driver supports the NAND Flash controller embedded in recent
>> Tango chips, such as SMP8758 and SMP8759.
> 
> Please send another patch to document the DT bindings, and Cc the DT
> maintainers.

OK.


>> +struct tango_chip {
>> +	struct nand_chip chip;
>> +	void __iomem *base;
> 
> I'm still not convinced this is needed (->base can be calculated where
> needed based on the chip->cs and nfc->reg_base values), but if you want
> to keep it, let's keep it.

I thought it was bad form to duplicate the code computing
chip->base, especially if I need to update that function.


>> +static void tango_select_chip(struct mtd_info *mtd, int idx)
>> +{
>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>> +	struct tango_nfc *nfc = to_tango_nfc(nand->controller);
>> +	struct tango_chip *chip = to_tango_chip(nand);
>> +
>> +	if (idx < 0) {
>> +		chip->base = NULL;
>> +		return;
>> +	}
>> +
>> +	chip->base = nfc->pbus_base + (chip->cs * 256);
> 
> You support only one CS per chip, so this is something you can
> configure at init time.
> 
> When I asked you to remove the chip->base field, I had multi-CS chips
> in mind, but given this implementation, there's no need to set/reset
> the chip->base field each time ->select_chip() is called.

OK. I'll set chip->cs and chip->base in the probe function.
Just to be sure, I'll ask internally whether we want to
support multi-CS chips right now, or if this can be added
later on.


>> +	writel_relaxed(chip->timing1,	nfc->reg_base + NFC_TIMING1);
>> +	writel_relaxed(chip->timing2,	nfc->reg_base + NFC_TIMING2);
>> +	writel_relaxed(chip->xfer_cfg,	nfc->reg_base + NFC_XFER_CFG);
>> +	writel_relaxed(chip->pkt_0_cfg,	nfc->reg_base + NFC_PKT_0_CFG);
>> +	writel_relaxed(chip->pkt_n_cfg,	nfc->reg_base + NFC_PKT_N_CFG);
>> +	writel_relaxed(chip->bb_cfg,	nfc->reg_base + NFC_BB_CFG);
> 
> Nit: can you avoid these weird alignments. Use a single space after the
> comma.

I was trying to highlight the fact that all these writes are
within the reg_base area. I'll do as you ask, since you are
the gatekeeper.


>> +static int do_dma(struct tango_nfc *nfc, int dir, int cmd,
>> +		const void *buf, int len, int page)
>> +{
>> +	struct dma_async_tx_descriptor *desc;
>> +	struct scatterlist sg;
>> +	struct completion tx_done;
>> +	int err = -EIO;
>> +	u32 res, val;
>> +
>> +	sg_init_one(&sg, buf, len);
>> +	if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1)
>> +		goto leave;
> 
> Return -EIO directly instead of creating this 'leave' label.

OK. I wasn't sure it was good form to make the first test
special, especially if someone adds a new alloc before this
one later on. But I don't care either way.


>> +
>> +	desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT);
> 
> Put DMA_PREP_INTERRUPT on a second line to avoid >80 char lines.

I was told some maintainers don't enforce the >80 char line rule.
Am I to understand you're not one of them? :-)


>> +	if (!desc)
>> +		goto dma_unmap;
>> +
>> +	desc->callback = tango_dma_callback;
>> +	desc->callback_param = &tx_done;
>> +	init_completion(&tx_done);
>> +
>> +	writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE);
>> +
>> +	writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE);
>> +	writel_relaxed(   0, nfc->reg_base + NFC_ADDR_OFFSET);
>> +	writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD);
>> +
>> +	dmaengine_submit(desc);
>> +	dma_async_issue_pending(nfc->chan);
>> +
>> +	res = wait_for_completion_timeout(&tx_done, HZ);
>> +	if (res > 0) {
>> +		void __iomem *addr = nfc->reg_base + NFC_STATUS;
>> +		err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);
> 
> Why do you need this local variable?
> 
> 		err = readl_poll_timeout(nfc->reg_base + NFC_STATUS,
> 					 val, val & CMD_READY, 0, 1000);

I find it hard to read when function arguments are split
over multiple lines. So if there is a hard "80 char" limit,
I prefer using temp variables for "long" arguments.


>> +	}
>> +
>> +	writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE);
> 
> Add an extra blank line here.

OK.


>> +static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>> +		uint8_t *buf, int oob_required, int page)
>> +{
>> +	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
>> +	int err, res, len = mtd->writesize;
>> +
>> +	err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page);
>> +	if (err)
>> +		return err;
>> +
>> +	if (oob_required)
>> +		chip->ecc.read_oob(mtd, chip, page);
>> +
>> +	res = decode_error_report(nfc);
>> +	if (res >= 0)
>> +		return res;
>> +
>> +	chip->ecc.read_oob(mtd, chip, page);
> 
> There's no different in your case, but it should be read in raw mode.
> How about calling ecc.read_oob_raw() so that you're safe even if you
> want to differentiate the read_oob() and read_oob_raw() cases at some
> point (IIUC, the METADATA section is ECC protected).

OK.


>> +	res = nand_check_erased_ecc_chunk(buf, len, chip->oob_poi, mtd->oobsize,
>> +			NULL, 0, chip->ecc.strength);
> 
> You should check each ECC packet/chunk independently, because ECC
> strength is referring to an ECC packet not a full page.

Because of our weird physical layout, accessing data chunks is
really awkward. We are willing to sacrifice a few bad pages
by using a sub-optimal check (allowing only "strength" bitflips
over the entire page, instead of over individual packets).


>> +static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>> +		const uint8_t *buf, int oob_required, int page)
>> +{
>> +	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
>> +	int err, len = mtd->writesize;
>> +
>> +	writel_relaxed(0xffffffff, nfc->mem_base + METADATA);
>> +	err = do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, buf, len, page);
>> +	if (err)
>> +		return err;
>> +
>> +	if (oob_required)
>> +		chip->ecc.write_oob(mtd, chip, page);

I suppose we should use write_oob_raw here?


>> +enum { OP_SKIP, OP_READ, OP_WRITE };
>> +
>> +static void raw_aux(struct mtd_info *mtd, u8 **buf, int len, int op, int *lim)
> 
> Nit: please pass struct nand_chip *chip in first argument, and extract
> the mtd using nand_to_mtd(). I know it's just a detail, but I'd like to
> avoid passing mtd pointers, especially in internal functions.

OK.


>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +	int pos = mtd->writesize - *lim;
>> +
>> +	if (op == OP_SKIP)
>> +		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, pos + len, -1);
> 
> Okay, we already discussed that one, and I already showed that this was
> not working: NAND_CMD_RNDOUT is only valid for read accesses.
> 
> And I told you I'd like to avoid the const to non-const cast in write
> accessors.

Note: I don't use SKIP for writes, therefore there's no need
to implement that code.


>> +	else if (op == OP_READ)
>> +		tango_read_buf(mtd, *buf, len);
>> +	else if (op == OP_WRITE)
>> +		tango_write_buf(mtd, *buf, len);
>> +
>> +	*lim -= len;
>> +	*buf += len;
>> +}
> 
> I'm not sure factorizing this code is really important, but since you
> insist, how about doing the following instead:

If the code is not factorized, then it must be duplicated for
read/write_raw and read/write_oob, I think (i.e. 4 times).

> enum tango_raw_op { OP_READ, OP_WRITE };
> 
> union tango_raw_buffer {
> 	void *in;
> 	const void *out;
> };
> 
> struct tango_raw_access {
> 	enum tango_raw_op op;
> 	union tango_raw_buffer *buf;
> 	int pos;
> };
> 
> static void raw_aux(struct nand_chip *chip,
> 		    struct tango_raw_access *info,
> 		    union tango_raw_buffer *buf, int len)
> {
> 	struct mtd_info *mtd = nand_to_mtd(chip);
> 
> 	if (!buf) {
> 		if (info->op == OP_READ)
> 			cmd = NAND_CMD_RNDOUT;
> 		else
> 			cmd = NAND_CMD_SEQIN;
> 
> 		chip->cmdfunc(mtd, cmd, info->pos + len, -1);
> 	} else {
> 		if (info->op == OP_READ)
> 			tango_read_buf(mtd, buf->out, len);
> 		else
> 			tango_write_buf(mtd, buf->in, len);
> 
> 		buf->in += len;
> 	}
> 
> 	info->pos += len;
> }

I'll take a closer look tomorrow.


>> +static int raw_access(struct mtd_info *mtd, uint8_t *buf, uint8_t *oob, int op)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +	int pkt_size = chip->ecc.size;
>> +	int ecc_size = chip->ecc.bytes;
>> +	int buf_op = buf ? op : OP_SKIP;
>> +	int oob_op = oob ? op : OP_SKIP;
>> +	int rem, lim = mtd->writesize;
>> +	u8 *oob_orig = oob;
>> +
>> +	oob += BBM_SIZE;
>> +	raw_aux(mtd, &oob, METADATA_SIZE, oob_op, &lim);
>> +
>> +	while (lim > pkt_size)
>> +	{
>> +		raw_aux(mtd, &buf, pkt_size, buf_op, &lim);
>> +		raw_aux(mtd, &oob, ecc_size, oob_op, &lim);
>> +	}
>> +
>> +	rem = pkt_size - lim;
>> +	raw_aux(mtd, &buf, lim, buf_op, &lim);
>> +	raw_aux(mtd, &oob_orig, BBM_SIZE, oob_op, &lim);
>> +	raw_aux(mtd, &buf, rem, buf_op, &lim);
>> +	raw_aux(mtd, &oob, ecc_size, oob_op, &lim);
>> +
>> +	return 0;
>> +}
> 
> With the above changes this gives:
> 
> static int raw_access(struct nand_chip *chip,
> 		      union tango_raw_buffer *buf,
> 		      union tango_raw_buffer *oob,
> 		      int op)
> {
> 	struct mtd_info *mtd = nand_to_mtd(chip);
> 	int pkt_size = chip->ecc.size;
> 	int ecc_size = chip->ecc.bytes;
> 	int steps = 0, rem = 0;
> 	struct tango_raw_access info = {
> 		.op = op,
> 		.pos = 0,
> 	};
> 	union tango_raw_buffer oob_orig;
> 
> 	if (oob) {
> 		oob->in = chip->oob_poi + BBM_SIZE;
> 		oob_orig.in = chip->oob_poi;
> 	}
> 
> 	raw_aux(chip, &info, oob, METADATA_SIZE);
> 
> 	while (info.pos + pkt_size + ecc_size <= mtd->writesize) {
> 		raw_aux(chip, &info, buf, pkt_size);
> 		raw_aux(mtd, &info, oob, ecc_size);
> 		steps++;
> 	}
> 
> 	if (steps < chip->ecc.steps)
> 		rem = pkt_size - (mtd->writesize - info.pos);
> 		raw_aux(mtd, &info, buf, mtd->writesize - info.pos);
> 	}

I don't think we need the 'steps' variable (I didn't need one).

> 	raw_aux(mtd, &info, oob ? &oob_orig : NULL, BBM_SIZE);
> 	raw_aux(mtd, &info, buf, rem);
> 
> 	rem = mtd->writesize + mtd->oobsize - info.pos;
> 	raw_aux(mtd, &info, oob, rem);
> 
> 	return 0;
> }

I'll take a closer look tomorrow.


>> +static u32 to_ticks(int kHz, int ps)
>> +{
>> +	return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC);
>> +}
>> +
>> +static int set_timings(struct tango_chip *p, int kHz)
>> +{
>> +	u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr;
>> +	const struct nand_sdr_timings *sdr;
>> +	int mode = onfi_get_async_timing_mode(&p->chip);
>> +
>> +	if (mode & ONFI_TIMING_MODE_UNKNOWN)
>> +		return -ENODEV;
>> +
>> +	sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1);
> 
> We recently introduced a generic interface to automate nand timings
> configuration [1].
> Can you make use of it (the patches are in my nand/next branch [2], you
> can rebase your patch series on top of this branch).

I'll take a look. I still need to have this driver working
on 4.7 for the time being...


>> +
>> +	Trdy	= to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
>> +	Textw	= to_ticks(kHz, sdr->tWB_max);
>> +	Twc	= to_ticks(kHz, sdr->tWC_min);
>> +	Twpw	= to_ticks(kHz, sdr->tWC_min - sdr->tWP_min);
>> +
>> +	Tacc	= to_ticks(kHz, sdr->tREA_max);
>> +	Thold	= to_ticks(kHz, sdr->tREH_min);
>> +	Trpw	= to_ticks(kHz, sdr->tRC_min - sdr->tREH_min);
>> +	Textr	= to_ticks(kHz, sdr->tRHZ_max);
> 
> Can you please be consistent in your indentation?
> You're using spaces before '=' almost everywhere except in a few
> places (like here).

When initializing fields of a struct, most code I've seen
uses tabs to align the equal signs. I thought it should be
the same when assigning the fields of a struct, or assigning
several variables of the same nature (such as above).

Are you saying you prefer

  Trdy = to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
  Textw = to_ticks(kHz, sdr->tWB_max);
  Twc = to_ticks(kHz, sdr->tWC_min);

Thus the to_ticks calls are not aligned, and it's less obvious
that the first argument is always the same.


>> +static int chip_init(struct device *dev, struct device_node *np, int kHz)
>> +{
>> +	int err;
>> +	u32 cs, ecc_bits;
>> +	struct nand_ecc_ctrl *ecc;
>> +	struct tango_nfc *nfc = dev_get_drvdata(dev);
>> +	struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
>> +	if (!p)
>> +		return -ENOMEM;
>> +
>> +	err = of_property_read_u32_index(np, "reg", 0, &cs);
>> +	if (err)
>> +		return err;
>> +
> 
> Can you please check that you only have a single value in reg? Just to
> make sure the user doesn't try to define a multi-CS chip.

OK.


>> +	if (cs >= MAX_CS)
>> +		return -ERANGE;
>> +
>> +	p->chip.read_byte	= tango_read_byte;
>> +	p->chip.read_buf	= tango_read_buf;
>> +	p->chip.select_chip	= tango_select_chip;
>> +	p->chip.cmd_ctrl	= tango_cmd_ctrl;
>> +	p->chip.dev_ready	= tango_dev_ready;
>> +	p->chip.options		= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
>> +	p->chip.controller	= &nfc->hw;
> 
> Again, be consistent in you coding style: use spaces.

OK. I'll take another look at the sunxi driver.

It makes no sense to me that we should tab-align when using
struct initialization with named fields, but space-align
when using assignment.


>> +
>> +	ecc			= &p->chip.ecc;
>> +	ecc->mode		= NAND_ECC_HW;
>> +	ecc->algo		= NAND_ECC_BCH;
>> +	ecc->read_page_raw	= tango_read_page_raw;
>> +	ecc->write_page_raw	= tango_write_page_raw;
>> +	ecc->read_page		= tango_read_page;
>> +	ecc->write_page		= tango_write_page;
>> +	ecc->read_oob		= tango_read_oob;
>> +	ecc->write_oob		= tango_write_oob;
> 
> Can you move this part after nand_scan_ident(). I'd also suggest to
> support software ECC (it's just taking a few more lines), but that's up
> to you.

OK. What does it mean to support SW ECC?

>> +
>> +	nand_set_flash_node(&p->chip, np);
>> +	mtd_set_ooblayout(&p->chip.mtd, &tango_nand_ooblayout_ops);
>> +	err = nand_scan_ident(&p->chip.mtd, 1, NULL);
>> +	if (err)
>> +		return err;
>> +
>> +	ecc_bits = ecc->strength * FIELD_ORDER;
>> +	p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, BITS_PER_BYTE);
>> +	set_timings(p, kHz);
> 
> You won't need that one if you implement the
> chip->setup_data_interface() method.

I'll take a closer look.


>> +	p->xfer_cfg	= XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE);
>> +	p->pkt_0_cfg	= PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength);
>> +	p->pkt_n_cfg	= PKT_CFG(ecc->size, ecc->strength);
>> +	p->bb_cfg	= BB_CFG(p->chip.mtd.writesize, BBM_SIZE);
> 
> This should go before nand_scan_tail(), because, as soon as you call
> mtd_device_register(), someone might use your NAND device.
> 
>> +	p->cs		= cs;
> 
> And this one should go before nand_scan_ident().

OK to both.


>> +	for_each_child_of_node(pdev->dev.of_node, np) {
>> +		int err = chip_init(&pdev->dev, np, kHz);
>> +		if (err)
>> +			return err;
> 
> You should unregister all NAND/MTD devices before returning an error.

Good catch. I was used to devm_* wrappers for everything in
some frameworks. I'll call tango_nand_remove on error.

Thanks for the detailed review.

Regards.