Re: [PATCH v4 4/9] nand: spi: add basic blocks for infrastructure

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Thu, 23 Mar 2017 12:29:58 +0100
Marek Vasut <marex@denx.de> wrote:


> > +
> > +/*
> > + * spinand_read_status - get status register value
> > + * @chip: SPI NAND device structure
> > + * @status: buffer to store value
> > + * Description:
> > + *   After read, write, or erase, the NAND device is expected to set the
> > + *   busy status.
> > + *   This function is to allow reading the status of the command: read,
> > + *   write, and erase.
> > + */
> > +static int spinand_read_status(struct spinand_device *chip, uint8_t *status)
> > +{
> > +	return spinand_read_reg(chip, REG_STATUS, status);
> > +}
> > +
> > +/*
> > + * spinand_wait - wait until the command is done
> > + * @chip: SPI NAND device structure
> > + * @s: buffer to store status register value (can be NULL)
> > + */
> > +static int spinand_wait(struct spinand_device *chip, u8 *s)
> > +{
> > +	unsigned long timeo = msecs_to_jiffies(400);
> > +	u8 status;
> > +
> > +	do {
> > +		spinand_read_status(chip, &status);
> > +		if ((status & STATUS_OIP_MASK) == STATUS_READY)
> > +			goto out;
> > +	} while (time_before(jiffies, timeo));
> > +
> > +	/*
> > +	 * Extra read, just in case the STATUS_READY bit has changed
> > +	 * since our last check
> > +	 */  
> 
> Is this likely to happen ? Probably not ... so in case you reach this
> place here, timeout happened.

If the timeout is big enough, no. But it does not hurt to do one last
check.

> 
> > +	spinand_read_status(chip, &status);
> > +out:
> > +	if (s)
> > +		*s = status;
> > +
> > +	return (status & STATUS_OIP_MASK) == STATUS_READY ? 0 :	-ETIMEDOUT;
> > +}
> > +
> > +/*
> > + * spinand_read_id - send 9Fh command to get ID
> > + * @chip: SPI NAND device structure
> > + * @buf: buffer to store id
> > + * Description:
> > + *   Manufacturers' read ID method is not unique. Some need a dummy before
> > + *   reading, some's ID has three byte.
> > + *   This function send one byte opcode (9Fh) and then read
> > + *   SPINAND_MAX_ID_LEN (4 currently) bytes. Manufacturer's detect function
> > + *   need to filter out real ID from the 4 bytes.
> > + */
> > +static int spinand_read_id(struct spinand_device *chip, u8 *buf)
> > +{
> > +	struct spinand_op op;
> > +
> > +	spinand_op_init(&op);
> > +	op.cmd = SPINAND_CMD_READ_ID;
> > +	op.n_rx = SPINAND_MAX_ID_LEN;
> > +	op.rx_buf = buf;
> > +
> > +	return spinand_exec_op(chip, &op);
> > +}
> > +
> > +/*
> > + * spinand_reset - reset device by FFh command.
> > + * @chip: SPI NAND device structure
> > + */
> > +static int spinand_reset(struct spinand_device *chip)
> > +{
> > +	struct spinand_op op;
> > +	int ret;
> > +
> > +	spinand_op_init(&op);
> > +	op.cmd = SPINAND_CMD_RESET;
> > +
> > +	ret = spinand_exec_op(chip, &op);
> > +	if (ret < 0) {
> > +		pr_err("spinand reset failed!\n");  
> 
> Can we use dev_err() ? Also , previous patch had some pr_fmt which added
> "SPI NAND:" prefix, so replace "spinand" with "SPI NAND: " to be
> consistent ...

The underlying dev is not ready/initialized the first time
spinand_reset() is called.

> 
> > +		goto out;
> > +	}
> > +	ret = spinand_wait(chip, NULL);
> > +
> > +out:
> > +	return ret;
> > +}

> > +/*
> > + * spinand_manufacturer_detect - detect SPI NAND device by each manufacturer
> > + * @chip: SPI NAND device structure
> > + *
> > + * ->detect() should decode raw id in chip->id.data and initialize device
> > + * related part in spinand_device structure if it is the right device.
> > + * ->detect() can not be NULL.
> > + */
> > +static int spinand_manufacturer_detect(struct spinand_device *chip)
> > +{
> > +	int i = 0;
> > +
> > +	for (; spinand_manufacturers[i]->id != 0x0; i++) {
> > +		if (!spinand_manufacturers[i]->ops ||
> > +		    !spinand_manufacturers[i]->ops->detect) {
> > +			pr_err("%s's ops or ops->detect() is be NULL!\n",
> > +			       spinand_manufacturers[i]->name);  
> 
> Can this case happen, EVER ? I'd mandate the ->detect to be always set.

I agree. Let's assume ->detect is always != NULL. Same goes for ->ops
(assuming you use the ARRAY_SIZE() approach I suggested below).

> 
> > +			return -EINVAL;
> > +		}
> > +		if (spinand_manufacturers[i]->ops->detect(chip)) {
> > +			chip->manufacturer.manu = spinand_manufacturers[i];
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -ENODEV;
> > +}

[...]

> > +/*
> > + * spinand_alloc - [SPI NAND Interface] allocate SPI NAND device instance
> > + * @dev: pointer to device model structure
> > + */
> > +struct spinand_device *spinand_alloc(struct device *dev)
> > +{
> > +	struct spinand_device *chip;
> > +	struct spinand_ecc_engine *ecc_engine;
> > +
> > +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> > +	if (!chip)
> > +		goto err1;
> > +
> > +	ecc_engine = kzalloc(sizeof(*ecc_engine), GFP_KERNEL);
> > +	if (!ecc_engine)
> > +		goto err2;  
> 
> Just allocate a structure with both chip and ecc_engine in them, then
> you don't have to kzalloc/kfree twice .

Actually, the ECC engine allocation/initialization should not be done
here. At this point, you don't know yet which ECC engine will be used
(on-die, external engine, software ECC, ...).

Let the real ECC implementation allocate the ecc.engine struct
(and overload it if needed).

> 
> > +	ecc_engine->mode = SPINAND_ECC_ONDIE;

The engine mode (which is more a type than a mode IMO) should be
stored in chip->ecc.type (or chip->ecc.mode) and be used by the
different components (core, vendor or controller code) to determine who
is supposed to provide the ECC engine.

> > +	chip->ecc.engine = ecc_engine;
> > +	spinand_set_of_node(chip, dev->of_node);
> > +	mutex_init(&chip->lock);
> > +
> > +	return chip;
> > +
> > +err2:
> > +	kfree(chip);
> > +err1:
> > +	return ERR_PTR(-ENOMEM);
> > +}
> > +EXPORT_SYMBOL_GPL(spinand_alloc);
> > +
> > +/*
> > + * spinand_free - [SPI NAND Interface] free SPI NAND device instance
> > + * @chip: SPI NAND device structure
> > + */
> > +void spinand_free(struct spinand_device *chip)
> > +{
> > +	kfree(chip->ecc.engine);
> > +	kfree(chip);
> > +}
> > +EXPORT_SYMBOL_GPL(spinand_free);
> > +
> > +/*
> > + * spinand_register - [SPI NAND Interface] register SPI NAND device
> > + * @chip: SPI NAND device structure
> > + */
> > +int spinand_register(struct spinand_device *chip)
> > +{
> > +	int ret;
> > +
> > +	ret = spinand_detect(chip);
> > +	if (ret) {
> > +		pr_err("Detect SPI NAND failed with error %d.\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = spinand_init(chip);
> > +	if (ret)
> > +		pr_err("Init SPI NAND failed with error %d.\n", ret);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(spinand_register);
> > +
> > +/*
> > + * spinand_unregister - [SPI NAND Interface] unregister SPI NAND device
> > + * @chip: SPI NAND device structure
> > + */
> > +int spinand_unregister(struct spinand_device *chip)
> > +{
> > +	struct nand_device *nand = &chip->base;
> > +
> > +	nand_unregister(nand);
> > +	spinand_manufacturer_cleanup(chip);
> > +	kfree(chip->buf);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(spinand_unregister);
> > +
> > +MODULE_DESCRIPTION("SPI NAND framework");
> > +MODULE_AUTHOR("Peter Pan<peterpandong@micron.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/mtd/nand/spi/manufactures.c b/drivers/mtd/nand/spi/manufactures.c
> > new file mode 100644
> > index 0000000..7e0b42d
> > --- /dev/null
> > +++ b/drivers/mtd/nand/spi/manufactures.c

manufacturers.c, but I don't think I want to keep this file anyway.

> > @@ -0,0 +1,24 @@
> > +/**
> > + *
> > + * Copyright (c) 2009-2017 Micron Technology, Inc.  
> 
> 2009-2017 ? Wow :)
> 
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/mtd/spinand.h>
> > +
> > +struct spinand_manufacturer spinand_manufacturer_end = {0x0, "Unknown", NULL};
> > +
> > +struct spinand_manufacturer *spinand_manufacturers[] = {
> > +	&spinand_manufacturer_end,  
> 
> Just populate the array directly .

BTW, you don't need this sentinel. Just use ARRAY_SIZE() and put this
table directly in the core.