RE: [[LINUX PATCH v10] 4/4] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Naga Sureshkumar Relli]

Hi Miquel,

> -----Original Message-----
> From: Miquel Raynal [mailto:miquel.raynal@xxxxxxxxxxx]
> Sent: Thursday, June 28, 2018 12:45 PM
> To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> Cc: boris.brezillon@xxxxxxxxxxx; richard@xxxxxx; dwmw2@xxxxxxxxxxxxx;
> computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx; f.fainelli@xxxxxxxxx;
> mmayer@xxxxxxxxxxxx; rogerq@xxxxxx; ladis@xxxxxxxxxxxxxx; ada@xxxxxxxxxxx;
> honghui.zhang@xxxxxxxxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> nagasureshkumarrelli@xxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>
> 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).
Sure. 
> 
> [...]
> 
> > > > +
> > > > +	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.
Ok.
> 
> > 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
Ok.
> 
> > > > +	ret = pl353_nand_ecc_init(mtd, &chip->ecc, chip->ecc.mode);
> 
> ret should be checked
Ok.
> 
> > > > +	if (chip->options & NAND_BUSWIDTH_16)
> > > > +		pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_16);
> 
> Space
Ok.
> 
> > > > +	/* 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().
Ok, I will update it.
> 
> > >
> > > > +
> > > > +	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

Thanks,
Naga Sureshkumar Relli