Message-ID: <20180628091450.25bd54e5@xps13>
Date:   Thu, 28 Jun 2018 09:14:50 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Naga Sureshkumar Relli <nagasure@...inx.com>
Cc:     "boris.brezillon@...tlin.com" <boris.brezillon@...tlin.com>,
        "richard@....at" <richard@....at>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "computersforpeace@...il.com" <computersforpeace@...il.com>,
        "marek.vasut@...il.com" <marek.vasut@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "mmayer@...adcom.com" <mmayer@...adcom.com>,
        "rogerq@...com" <rogerq@...com>,
        "ladis@...ux-mips.org" <ladis@...ux-mips.org>,
        "ada@...rsis.com" <ada@...rsis.com>,
        "honghui.zhang@...iatek.com" <honghui.zhang@...iatek.com>,
        "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "nagasureshkumarrelli@...il.com" <nagasureshkumarrelli@...il.com>,
        Michal Simek <michals@...inx.com>
Subject: Re: [[LINUX PATCH v10] 4/4] mtd: rawnand: pl353: Add basic driver
 for arm pl353 smc nand interface

Hi Naga,

> > > +/**
> > > + * pl353_nand_read_page_hwecc - Hardware ECC based page read function
> > > + * @mtd:		Pointer to the mtd info structure
> > > + * @chip:		Pointer to the NAND chip info structure
> > > + * @buf:		Pointer to the buffer to store read data
> > > + * @oob_required:	Caller requires OOB data read to chip->oob_poi
> > > + * @page:		Page number to read
> > > + *
> > > + * This functions reads data and checks the data integrity by
> > > +comparing hardware
> > > + * generated ECC values and read ECC values from spare area.
> > > + *
> > > + * Return:	0 always and updates ECC operation status in to MTD structure
> > > + */
> > > +static int pl353_nand_read_page_hwecc(struct mtd_info *mtd,
> > > +				      struct nand_chip *chip,
> > > +				      u8 *buf, int oob_required, int page) {
> > > +	int i, stat, eccsize = chip->ecc.size;
> > > +	int eccbytes = chip->ecc.bytes;
> > > +	int eccsteps = chip->ecc.steps;
> > > +	u8 *p = buf;
> > > +	u8 *ecc_calc = chip->ecc.calc_buf;
> > > +	u8 *ecc = chip->ecc.code_buf;
> > > +	unsigned int max_bitflips = 0;
> > > +	u8 *oob_ptr;
> > > +	u32 ret;
> > > +	unsigned long data_phase_addr;
> > > +	struct pl353_nand_info *xnfc =
> > > +		container_of(chip, struct pl353_nand_info, chip);
> > > +	unsigned long nand_offset = (unsigned long __force)xnfc->nand_base;
> > > +
> > > +	pl353_prepare_cmd(mtd, chip, page, 0, NAND_CMD_READ0,
> > > +			  NAND_CMD_READSTART, 1);
> > > +	ndelay(100);  
> > 
> > What is this delay for?  
> We have seen failures with out this delay, with older code.
> But i will check this by removing this delay, in this new driver.

Please check all of them. We should get rid of random delays like that.
Either there is something to poll, or there is a specific value to use
(you can get them from the SDR interface structure).

[...]

> > > +
> > > +	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
> > > +		return -EINVAL;  
> > 
> > Why?  
> It is similar to 
> if (chipnr < 0)
> 	return 0;

Mmmmmh, no?

(return 0) != (return -EINVAL)

The core is asking you to check if the controller driver support
particular timings (usually ONFI modes 1-5). Returning an error means
"I only support the slowest timings" which, I suppose, is wrong. Please
fix this and compare the speeds.

> hence written like that.
> Also if I didn't do that, then probe is failing.
> Am I missing some thing?
> >   

[...]

> > > +/**
> > > + * pl353_nand_probe - Probe method for the NAND driver
> > > + * @pdev:	Pointer to the platform_device structure
> > > + *
> > > + * This function initializes the driver data structures and the hardware.
> > > + *
> > > + * Return:	0 on success or error value on failure
> > > + */
> > > +static int pl353_nand_probe(struct platform_device *pdev) {
> > > +	struct pl353_nand_info *xnfc;
> > > +	struct mtd_info *mtd;
> > > +	struct nand_chip *chip;
> > > +	struct resource *res;
> > > +	struct device_node *np;
> > > +	u32 ret;
> > > +
> > > +	xnfc = devm_kzalloc(&pdev->dev, sizeof(*xnfc), GFP_KERNEL);
> > > +	if (!xnfc)
> > > +		return -ENOMEM;
> > > +	xnfc->dev = &pdev->dev;
> > > +	/* Map physical address of NAND flash */
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	xnfc->nand_base = devm_ioremap_resource(xnfc->dev, res);
> > > +	if (IS_ERR(xnfc->nand_base))
> > > +		return PTR_ERR(xnfc->nand_base);
> > > +
> > > +	chip = &xnfc->chip;
> > > +	mtd = nand_to_mtd(chip);
> > > +	chip->exec_op = pl353_nfc_exec_op;
> > > +	nand_set_controller_data(chip, xnfc);
> > > +	mtd->priv = chip;
> > > +	mtd->owner = THIS_MODULE;
> > > +	if (!mtd->name) {
> > > +		/*
> > > +		 * If the new bindings are used and the bootloader has not been
> > > +		 * updated to pass a new mtdparts parameter on the cmdline, you
> > > +		 * should define the following property in your NAND node, ie:
> > > +		 *
> > > +		 *	label = "pl353-nand";
> > > +		 *
> > > +		 * This way, mtd->name will be set by the core when
> > > +		 * nand_set_flash_node() is called.
> > > +		 */
> > > +		mtd->name = devm_kasprintf(xnfc->dev, GFP_KERNEL,
> > > +					   "%s", PL353_NAND_DRIVER_NAME);
> > > +		if (!mtd->name) {
> > > +			dev_err(xnfc->dev, "Failed to allocate mtd->name\n");
> > > +			return -ENOMEM;
> > > +		}
> > > +	}
> > > +	nand_set_flash_node(chip, xnfc->dev->of_node);
> > > +
> > > +	/* Set address of NAND IO lines */
> > > +	chip->IO_ADDR_R = xnfc->nand_base;
> > > +	chip->IO_ADDR_W = xnfc->nand_base;
> > > +	/* Set the driver entry points for MTD */
> > > +	chip->dev_ready = pl353_nand_device_ready;
> > > +	chip->select_chip = pl353_nand_select_chip;
> > > +	/* If we don't set this delay driver sets 20us by default */
> > > +	np = of_get_next_parent(xnfc->dev->of_node);
> > > +	xnfc->mclk = of_clk_get(np, 0);
> > > +	if (IS_ERR(xnfc->mclk)) {
> > > +		dev_err(xnfc->dev, "Failed to retrieve MCK clk\n");
> > > +		return PTR_ERR(xnfc->mclk);
> > > +	}
> > > +	chip->chip_delay = 30;
> > > +	/* Set the device option and flash width */
> > > +	chip->options = NAND_BUSWIDTH_AUTO;
> > > +	chip->bbt_options = NAND_BBT_USE_FLASH;
> > > +	platform_set_drvdata(pdev, xnfc);
> > > +	chip->setup_data_interface = pl353_setup_data_interface;
> > > +	/* first scan to find the device and get the page size */
> > > +	if (nand_scan_ident(mtd, 1, NULL)) {
> > > +		dev_err(xnfc->dev, "nand_scan_ident for NAND failed\n");
> > > +		return -ENXIO;
> > > +	}

Space here

> > > +	ret = pl353_nand_ecc_init(mtd, &chip->ecc, chip->ecc.mode);

ret should be checked

> > > +	if (chip->options & NAND_BUSWIDTH_16)
> > > +		pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_16);

Space

> > > +	/* second phase scan */
> > > +	if (nand_scan_tail(mtd)) {
> > > +		dev_err(xnfc->dev, "nand_scan_tail for NAND failed\n");
> > > +		return -ENXIO;
> > > +	}
> > > +
> > > +	mtd_device_register(mtd, NULL, 0);  
> > 
> > Check the returned code  
> Ok.

And if it returns an error, please call nand_cleanup().

> >   
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * pl353_nand_remove - Remove method for the NAND driver
> > > + * @pdev:	Pointer to the platform_device structure
> > > + *
> > > + * This function is called if the driver module is being unloaded. It
> > > +frees all
> > > + * resources allocated to the device.
> > > + *
> > > + * Return:	0 on success or error value on failure
> > > + */
> > > +static int pl353_nand_remove(struct platform_device *pdev) {
> > > +	struct pl353_nand_info *xnfc = platform_get_drvdata(pdev);
> > > +	struct mtd_info *mtd = nand_to_mtd(&xnfc->chip);
> > > +
> > > +	/* Release resources, unregister device */
> > > +	nand_release(mtd);  
> > 
> > What about MTD core deregistration?  
> nand_release(), it self will do that.

My bad.

> >   
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/* Match table for device tree binding */ static const struct
> > > +of_device_id pl353_nand_of_match[] = {
> > > +	{ .compatible = "arm,pl353-nand-r2p1" },
> > > +	{},
> > > +};

Thanks,
Miquèl

Powered by blists - more mailing lists