Message-ID: <564B55CB.5050106@imgtec.com>
Date:	Tue, 17 Nov 2015 16:28:59 +0000
From:	Harvey Hunt <harvey.hunt@...tec.com>
To:	Boris Brezillon <boris.brezillon@...e-electrons.com>
CC:	<linux-mtd@...ts.infradead.org>,
	Zubair Lutfullah Kakakhel <Zubair.Kakakhel@...tec.com>,
	<linux-kernel@...r.kernel.org>, Alex Smith <alex.smith@...tec.com>,
	Alex Smith <alex@...x-smith.me.uk>,
	Brian Norris <computersforpeace@...il.com>,
	David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH v7, 2/3] mtd: nand: jz4780: driver for NAND devices on
 JZ4780 SoCs

Hi Boris,

On 04/11/15 10:18, Boris Brezillon wrote:
> Hi Harvey,
>
> Sorry for the late review :-/.

Not a problem. :-)


>> +#define BCH_BHCR_BSEL_SHIFT		4
>> +#define BCH_BHCR_BSEL_MASK		(0x7f << BCH_BHCR_BSEL_SHIFT)
>
> You can use GENMASK for these things.

I'll change that.

>> +#define BCH_BHCR_ENCE			BIT(2)
>> +#define BCH_BHCR_INIT			BIT(1)
>> +#define BCH_BHCR_BCHE			BIT(0)
>> +
>> +#define BCH_BHCNT_PARITYSIZE_SHIFT	16
>> +#define BCH_BHCNT_PARITYSIZE_MASK	(0x7f << BCH_BHCNT_PARITYSIZE_SHIFT)
>> +#define BCH_BHCNT_BLOCKSIZE_SHIFT	0
>> +#define BCH_BHCNT_BLOCKSIZE_MASK	(0x7ff << BCH_BHCNT_BLOCKSIZE_SHIFT)
>> +
>> +#define BCH_BHERR_MASK_SHIFT		16
>> +#define BCH_BHERR_MASK_MASK		(0xffff << BCH_BHERR_MASK_SHIFT)
>> +#define BCH_BHERR_INDEX_SHIFT		0
>> +#define BCH_BHERR_INDEX_MASK		(0x7ff << BCH_BHERR_INDEX_SHIFT)
>> +
>> +#define BCH_BHINT_ERRC_SHIFT		24
>> +#define BCH_BHINT_ERRC_MASK		(0x7f << BCH_BHINT_ERRC_SHIFT)
>> +#define BCH_BHINT_TERRC_SHIFT		16
>> +#define BCH_BHINT_TERRC_MASK		(0x7f << BCH_BHINT_TERRC_SHIFT)
>> +#define BCH_BHINT_DECF			BIT(3)
>> +#define BCH_BHINT_ENCF			BIT(2)
>> +#define BCH_BHINT_UNCOR			BIT(1)
>> +#define BCH_BHINT_ERR			BIT(0)
>> +
>> +#define BCH_CLK_RATE			(200 * 1000 * 1000)
>> +
>> +/* Timeout for BCH calculation/correction in microseconds. */
>> +#define BCH_TIMEOUT			100000
>
> Suffixing the macro name with _MS would make it clearer.

Do you mean _US?

>> +
>> +struct jz4780_bch {
>> +	void __iomem *base;
>> +	struct clk *clk;
>> +};
>
> You seem to rely on the sequencing mechanism provided by the NAND
> controller struct to sequence accesses to the BCH controller.
> That's fine as long as the jz4780 NAND controller is the only user of
> this BCH engine, which is no longer true as soon as you define two nand
> chips in your system (each of these chip is exposing its own
> nand_controller, and you end up with a concurrency problem).
>
> You have two different solutions to address that:
> 1/ provide a sequencing mechanism at the BCH engine level
> 2/ rework the NAND controller driver architecture to expose a single
> NAND controller (the NAND controller can attach to several CS of the
> nemc bus), and then define NAND chips as children nodes of the NAND
> controller node.
>
> Solution #1 is more future-proof, and allows you to use the BCH engine
> for other purpose than just NAND controller.
> Solution #2 has the benefit of making the NAND controller / NAND chip
> separation clearer, which is a good thing too.
>
> So I would recommend doing both :-).

I'll do both.

> [...]
>> diff --git a/drivers/mtd/nand/jz4780_bch.h b/drivers/mtd/nand/jz4780_bch.h
>> new file mode 100644
>> index 0000000..a5dfde5
>> --- /dev/null
>> +++ b/drivers/mtd/nand/jz4780_bch.h
>> @@ -0,0 +1,42 @@
>> +/*
>> + * JZ4780 BCH controller
>> + *
>> + * Copyright (c) 2015 Imagination Technologies
>> + * Author: Alex Smith <alex.smith@...tec.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __DRIVERS_MTD_NAND_JZ4780_BCH_H__
>> +#define __DRIVERS_MTD_NAND_JZ4780_BCH_H__
>> +
>> +#include <linux/types.h>
>> +
>> +struct device;
>> +struct device_node;
>> +
>> +/**
>> + * struct jz4780_bch_params - BCH parameters
>> + * @size: data bytes per ECC step.
>> + * @bytes: ECC bytes per step.
>> + * @strength: number of correctable bits per ECC step.
>> + */
>> +struct jz4780_bch_params {
>> +	int size;
>> +	int bytes;
>> +	int strength;
>> +};
>> +
>> +extern int jz4780_bch_calculate(struct device *dev,
>> +				struct jz4780_bch_params *params,
>> +				const uint8_t *buf, uint8_t *ecc_code);
>> +extern int jz4780_bch_correct(struct device *dev,
>> +			      struct jz4780_bch_params *params, uint8_t *buf,
>> +			      uint8_t *ecc_code);
>> +
>> +extern int jz4780_bch_get(struct device_node *np, struct device **dev);
>> +extern void jz4780_bch_release(struct device *dev);
>> +
>> +#endif /* __DRIVERS_MTD_NAND_JZ4780_BCH_H__ */
>> diff --git a/drivers/mtd/nand/jz4780_nand.c b/drivers/mtd/nand/jz4780_nand.c
>> new file mode 100644
>> index 0000000..4100ddc9
>> --- /dev/null
>> +++ b/drivers/mtd/nand/jz4780_nand.c
>> @@ -0,0 +1,384 @@
>> +/*
>> + * JZ4780 NAND driver
>> + *
>> + * Copyright (c) 2015 Imagination Technologies
>> + * Author: Alex Smith <alex.smith@...tec.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_mtd.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/nand.h>
>> +#include <linux/mtd/partitions.h>
>> +
>> +#include <linux/jz4780-nemc.h>
>> +
>> +#include "jz4780_bch.h"
>> +
>> +#define DRV_NAME	"jz4780-nand"
>> +
>> +#define OFFSET_DATA	0x00000000
>> +#define OFFSET_CMD	0x00400000
>> +#define OFFSET_ADDR	0x00800000
>> +
>> +/* Command delay when there is no R/B pin (in microseconds). */
>> +#define RB_DELAY	100
>
> _US suffix?

I'll add that.

>
>> +
>> +struct jz4780_nand_chip {
>> +	unsigned int bank;
>> +	void __iomem *base;
>> +};
>> +
>> +struct jz4780_nand {
>> +	struct mtd_info mtd;
>> +	struct nand_chip chip;
>> +
>> +	struct device *dev;
>> +	struct device *bch;
>> +
>> +	struct nand_ecclayout ecclayout;
>> +
>> +	int busy_gpio;
>> +	int wp_gpio;
>
> Any reason why you're not using gpio_desc here instead of integer ids?

No reason - I'll fix it.

>> +	unsigned int busy_gpio_active_low: 1;
>> +	unsigned int wp_gpio_active_low: 1;
>> +	unsigned int reading: 1;
>> +
>> +	unsigned int num_banks;
>> +	int selected;
>> +	struct jz4780_nand_chip chips[];
>> +};
>
> As explained earlier, I would prefer to see a clean separation between
> the nand_chip and the nand_controller definition. Something like:
>
> struct jz4780_nand_cs {
> 	unsigned int bank;
> 	void __iomem *base;
> };
>
> struct jz4780_nand_controller {
> 	struct nand_hw_control controller;
> 	struct device *dev;
> 	struct device *bch;
> 	unsigned int num_banks;
> 	struct jz4780_nand_cs cs[];
> }
>
> static inline to_jz4780_nand_controller(struct nand_hw_control *ctrl)
> {
> 	return container_of(ctrl, struct jz4780_nand_controller,
> 			    controller);
> }
>
> struct jz4780_nand {
> 	struct mtd_info mtd;
> 	struct nand_chip chip;
>
> 	struct nand_ecclayout ecclayout;
>
> 	/*
> 	 * Number of CS lines attached to this chip. Each CS line
> 	 * controls a specific die in the chip.
> 	*/
> 	unsigned int num_cs;
> 	/* Should point the definitions in struct nand_controller */
> 	struct jz4780_nand_chip **cs;
>
> 	/* RB pins definition, there can be one RB pin per die. */
> 	int *busy_gpios;
>
> 	/* RB pins definition, there can be one RB pin per die. */
> 	int *wp_gpios;
>
> 	unsigned int busy_gpio_active_low: 1;
> 	unsigned int wp_gpio_active_low: 1;
> 	unsigned int reading: 1;
> };
>
> Note that the jz4780_nand_controller pointer can be retrieved from the
> nand_chip struct by doing the following:
>
> 	to_jz4780_nand_controller(chip->controller);
>
>> +
>> +static inline struct jz4780_nand *to_jz4780_nand(struct mtd_info *mtd)
>> +{
>> +	return container_of(mtd, struct jz4780_nand, mtd);
>> +}
>> +
>> +static void jz4780_nand_select_chip(struct mtd_info *mtd, int chipnr)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +	struct jz4780_nand_chip *chip;
>> +
>> +	if (chipnr == -1) {
>> +		/* Ensure the currently selected chip is deasserted. */
>> +		if (nand->selected >= 0) {
>> +			chip = &nand->chips[nand->selected];
>> +			jz4780_nemc_assert(nand->dev, chip->bank, false);
>> +		}
>> +	} else {
>> +		chip = &nand->chips[chipnr];
>> +		nand->chip.IO_ADDR_R = chip->base + OFFSET_DATA;
>> +		nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
>
> How about providing helper functions that would use the nand->selected
> + chips information to access the correct set of registers instead of
> adapting IO_ADDR_R/IO_ADDR_W values?

I'm not sure what you mean - are you suggesting something such as:

u8 *jz4780_nand_read_io_line(struct jz4780_nand *nand, unsigned int off)
{
	return readb(&nand->chips[nand->selected]->base + off);
}

Does the NAND core code not make use of IO_ADDR_{W,R}?

>
>> +	}
>> +
>> +	nand->selected = chipnr;
>> +}
>> +
>> +static void jz4780_nand_cmd_ctrl(struct mtd_info *mtd, int cmd,
>> +				 unsigned int ctrl)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +	struct jz4780_nand_chip *chip;
>> +
>> +	if (WARN_ON(nand->selected < 0))
>> +		return;
>> +
>> +	chip = &nand->chips[nand->selected];
>> +
>> +	if (ctrl & NAND_CTRL_CHANGE) {
>> +		if (ctrl & NAND_ALE)
>> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_ADDR;
>> +		else if (ctrl & NAND_CLE)
>> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_CMD;
>> +		else
>> +			nand->chip.IO_ADDR_W = chip->base + OFFSET_DATA;
>
> Ditto.
>
>> +
>> +		jz4780_nemc_assert(nand->dev, chip->bank, ctrl & NAND_NCE);
>> +	}
>> +
>> +	if (cmd != NAND_CMD_NONE)
>> +		writeb(cmd, nand->chip.IO_ADDR_W);
>> +}
>> +
>> +static int jz4780_nand_dev_ready(struct mtd_info *mtd)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +
>> +	return !(gpio_get_value(nand->busy_gpio) ^ nand->busy_gpio_active_low);
>> +}
>> +
>> +static void jz4780_nand_ecc_hwctl(struct mtd_info *mtd, int mode)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +
>> +	nand->reading = (mode == NAND_ECC_READ);
>> +}
>> +
>> +static int jz4780_nand_ecc_calculate(struct mtd_info *mtd, const uint8_t *dat,
>> +				     uint8_t *ecc_code)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +	struct jz4780_bch_params params;
>> +
>> +	/*
>> +	 * Don't need to generate the ECC when reading, BCH does it for us as
>> +	 * part of decoding/correction.
>> +	 */
>> +	if (nand->reading)
>> +		return 0;
>> +
>> +	params.size = nand->chip.ecc.size;
>> +	params.bytes = nand->chip.ecc.bytes;
>> +	params.strength = nand->chip.ecc.strength;
>> +
>> +	return jz4780_bch_calculate(nand->bch, &params, dat, ecc_code);
>> +}
>> +
>> +static int jz4780_nand_ecc_correct(struct mtd_info *mtd, uint8_t *dat,
>> +				   uint8_t *read_ecc, uint8_t *calc_ecc)
>> +{
>> +	struct jz4780_nand *nand = to_jz4780_nand(mtd);
>> +	struct jz4780_bch_params params;
>> +
>> +	params.size = nand->chip.ecc.size;
>> +	params.bytes = nand->chip.ecc.bytes;
>> +	params.strength = nand->chip.ecc.strength;
>> +
>> +	return jz4780_bch_correct(nand->bch, &params, dat, read_ecc);
>> +}
>> +
>> +static int jz4780_nand_init_ecc(struct jz4780_nand *nand, struct device *dev)
>> +{
>> +	struct mtd_info *mtd = &nand->mtd;
>> +	struct nand_chip *chip = &nand->chip;
>> +	struct nand_ecclayout *layout = &nand->ecclayout;
>> +	struct device_node *bch_np;
>> +	int ret = 0;
>> +	uint32_t start, i;
>> +
>> +	if (!chip->ecc.size)
>> +		chip->ecc.size = 1024;
>> +
>> +	if (!chip->ecc.strength)
>> +		chip->ecc.strength = 24;
>
> Is there a strong reason to define a random default ECC config. I mean,
> if this is not provided then there is a problem somewhere, or the NAND
> does not require ECC, in which case mode should be set to NAND_ECC_NONE.
>
> At least, if you want to automatically choose an ECC config when it's
> not provided, choose it according to the NAND layout (it has to fit into
> the OOB area).

Nope, I'll change it to default to NAND_ECC_NONE if nothing has been 
passed from DT.

>> +
>> +	chip->ecc.bytes = fls(1 + 8 * chip->ecc.size) * chip->ecc.strength / 8;
>> +
>> +	dev_info(dev, "using %s BCH (strength %d, size %d, bytes %d)\n",
>> +		 (nand->bch) ? "hardware" : "software", chip->ecc.strength,
>> +		 chip->ecc.size, chip->ecc.bytes);
>> +
>> +	if (chip->ecc.mode == NAND_ECC_HW) {
>> +		bch_np = of_parse_phandle(dev->of_node,
>> +					"ingenic,bch-controller", 0);
>> +		if (bch_np) {
>> +			ret = jz4780_bch_get(bch_np, &nand->bch);
>> +			of_node_put(bch_np);
>> +			if (ret)
>> +				return ret;
>> +		} else {
>> +			dev_err(dev, "no bch controller in DT\n");
>> +			return -ENODEV;
>> +		}
>> +
>> +		chip->ecc.hwctl = jz4780_nand_ecc_hwctl;
>> +		chip->ecc.calculate = jz4780_nand_ecc_calculate;
>> +		chip->ecc.correct = jz4780_nand_ecc_correct;
>
> You should probably check that
> (ecc->bytes * (mtd->writesize / ecc->size)) <= mtd->oobsize - 2
> and fail if it's not the case.

Okay, will do.

>> +	}
>> +
>> +	/* Generate ECC layout. ECC codes are right aligned in the OOB area. */
>> +	layout->eccbytes = mtd->writesize / chip->ecc.size * chip->ecc.bytes;
>> +	start = mtd->oobsize - layout->eccbytes;
>> +	for (i = 0; i < layout->eccbytes; i++)
>> +		layout->eccpos[i] = start + i;
>> +
>> +	layout->oobfree[0].offset = 2;
>> +	layout->oobfree[0].length = mtd->oobsize - layout->eccbytes - 2;
>
> Not sure you want to fill that if mode == NAND_ECC_SOFT of
> NAND_ECC_SOFT_BCH, because those fields are automatically create/filled
> for you.

Okay, I'll add a check for that.

>> +
>> +	chip->ecc.layout = layout;
>> +
>> +	return 0;
>> +}
>> +
>> +static int jz4780_nand_init_chips(struct jz4780_nand *nand,
>> +				  struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct jz4780_nand_chip *chip;
>> +	const __be32 *prop;
>> +	struct resource *res;
>> +	int i = 0;
>> +
>> +	/*
>> +	 * Iterate over each bank assigned to this device and request resources.
>> +	 * The bank numbers may not be consecutive, but nand_scan_ident()
>> +	 * expects chip numbers to be, so fill out a consecutive array of chips
>> +	 * which map chip number to actual bank number.
>> +	 */
>> +	while ((prop = of_get_address(dev->of_node, i, NULL, NULL))) {
>> +		chip = &nand->chips[i];
>> +		chip->bank = of_read_number(prop, 1);
>> +
>> +		jz4780_nemc_set_type(nand->dev, chip->bank,
>> +				     JZ4780_NEMC_BANK_NAND);
>> +
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>> +		chip->base = devm_ioremap_resource(dev, res);
>> +		if (IS_ERR(chip->base))
>> +			return PTR_ERR(chip->base);
>> +
>> +		i++;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int jz4780_nand_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	unsigned int num_banks;
>> +	struct jz4780_nand *nand;
>> +	struct mtd_info *mtd;
>> +	struct nand_chip *chip;
>> +	enum of_gpio_flags flags;
>> +	struct mtd_part_parser_data ppdata;
>> +	int ret;
>> +
>> +	num_banks = jz4780_nemc_num_banks(dev);
>> +	if (num_banks == 0) {
>> +		dev_err(dev, "no banks found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	nand = devm_kzalloc(dev,
>> +			sizeof(*nand) + (sizeof(nand->chips[0]) * num_banks),
>> +			GFP_KERNEL);
>> +	if (!nand)
>> +		return -ENOMEM;
>> +
>> +	nand->dev = dev;
>> +	nand->num_banks = num_banks;
>> +	nand->selected = -1;
>> +
>> +	mtd = &nand->mtd;
>> +	chip = &nand->chip;
>> +	mtd->priv = chip;
>> +	mtd->owner = THIS_MODULE;
>> +	mtd->name = DRV_NAME;
>> +	mtd->dev.parent = dev;
>> +
>> +	chip->dn = dev->of_node;
>
> chip->dn has recently been renamed to chip->flash_node.

Thanks, I'll change this.

> BTW, If you go for the nand_controller + nand_chip approach, you'll have
> to initialize the nand_controller struct and link it with your
> nand_chip by doing:
>
> 	spin_lock_init(&jz4780_ctrl->controller.lock);
> 	init_waitqueue_head(&jz4780_ctrl->controller.wq);
> 	chip->controller = &jz4780_ctrl->controller;

Noted - thanks.

> That's all I see for now.
>
> Best Regards,
>
> Boris
>

Thanks again for the thorough review.

Best regards,

Harvey Hunt

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists