Hi Miquel, > -----Original Message----- > From: Miquel Raynal [mailto:[email protected]] > Sent: Thursday, June 28, 2018 12:45 PM > To: Naga Sureshkumar Relli <[email protected]> > Cc: [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; > [email protected]; Michal Simek <[email protected]> > 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
