From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailapp01.imgtec.com ([195.59.15.196]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1aG3ex-0001tu-J1 for linux-mtd@lists.infradead.org; Mon, 04 Jan 2016 11:50:48 +0000 Subject: Re: [PATCH v10 2/3] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs To: Boris Brezillon References: <1450959615-30246-1-git-send-email-harvey.hunt@imgtec.com> <1450959615-30246-3-git-send-email-harvey.hunt@imgtec.com> <20151228092552.66c88d9b@bbrezillon> <568A484F.5060803@imgtec.com> <20160104124749.6d34aaab@bbrezillon> CC: , , , Alex Smith , "Zubair Lutfullah Kakakhel" , David Woodhouse , From: Harvey Hunt Message-ID: <568A5C80.3030301@imgtec.com> Date: Mon, 4 Jan 2016 11:50:24 +0000 MIME-Version: 1.0 In-Reply-To: <20160104124749.6d34aaab@bbrezillon> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Boris, On 04/01/16 11:47, Boris Brezillon wrote: > Hi Harvey, > > On Mon, 4 Jan 2016 10:24:15 +0000 > Harvey Hunt wrote: > > >>>> + >>>> +static void jz4780_bch_disable(struct jz4780_bch *bch) >>>> +{ >>>> + writel(readl(bch->base + BCH_BHINT), bch->base + BCH_BHINT); >>>> + writel(BCH_BHCR_BCHE, bch->base + BCH_BHCCR); >>> >>> Not sure what BCH_BHCR_BCHE means, but if BCHE stands for "BCH Enable", >>> do you really have to keep this bit set when disabling the engine? >> >> The JZ4780 has the BHCR (BCH Control Register) as well as the BHCCR >> (BCH Control Clear Register) and BHCSR (BCH Control Set Register). >> Setting the bit BCH_BHCR_BCHE in BHCCR clears the corresponding bit in >> BHCR, which disables the BCH controller. >> > > Okay, thanks for the explanation. I guess BCHE stands for BCH Engine > then. > > [...] > >>> >>>> + >>>> +static int jz4780_nand_init_ecc(struct jz4780_nand_chip *nand, struct device *dev) >>>> +{ >>>> + struct nand_chip *chip = &nand->chip; >>>> + struct mtd_info *mtd = nand_to_mtd(chip); >>>> + struct jz4780_nand_controller *nfc = to_jz4780_nand_controller(chip->controller); >>>> + struct nand_ecclayout *layout = &nand->ecclayout; >>>> + u32 start, i; >>>> + >>>> + chip->ecc.bytes = fls((1 + 8) * chip->ecc.size) * >>>> + (chip->ecc.strength / 8); >>>> + >>>> + if (nfc->bch && chip->ecc.mode == NAND_ECC_HW) { >>>> + chip->ecc.hwctl = jz4780_nand_ecc_hwctl; >>>> + chip->ecc.calculate = jz4780_nand_ecc_calculate; >>>> + chip->ecc.correct = jz4780_nand_ecc_correct; >>>> + } else if (!nfc->bch && chip->ecc.mode == NAND_ECC_HW) { >>>> + dev_err(dev, "HW BCH selected, but BCH controller not found\n"); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + if (chip->ecc.mode != NAND_ECC_NONE) >>>> + dev_info(dev, "using %s (strength %d, size %d, bytes %d)\n", >>>> + (nfc->bch) ? "hardware BCH" : "software hamming", >>> >>> As said in my previous review, '!= NAND_ECC_HW' does not necessarily >>> imply '== NAND_ECC_SOFT' (i.e. hamming ECC), so I'd suggest printing >>> something like "software ECC". >> >> Done. >> >>> >>>> + chip->ecc.strength, chip->ecc.size, chip->ecc.bytes); >>>> + else >>>> + dev_info(dev, "not using ECC\n"); >>> >>> You should probably complain about the invalid NAND_ECC_HW_SYNDROME >>> value and return -EINVAL in this case. >>> > > Don't forget that aspect ^. > I forgot to mention it in my email, but I've not forgotten it in my patchset. :-) >>>> + >>>> + /* The NAND core will generate the ECC layout. */ >>>> + if (chip->ecc.mode == NAND_ECC_SOFT || chip->ecc.mode == NAND_ECC_SOFT_BCH) >>>> + return 0; >>>> + >>>> + /* Generate ECC layout. ECC codes are right aligned in the OOB area. */ >>>> + layout->eccbytes = mtd->writesize / chip->ecc.size * chip->ecc.bytes; >>>> + >>>> + if (layout->eccbytes > mtd->oobsize - 2) { >>>> + dev_err(dev, >>>> + "invalid ECC config: required %d ECC bytes, but only %d are available", >>>> + layout->eccbytes, mtd->oobsize - 2); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + 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; >>>> + >>>> + chip->ecc.layout = layout; >>>> + return 0; >>>> +} >>>> + >>>> +static int jz4780_nand_init_chip(struct platform_device *pdev, >>>> + struct jz4780_nand_controller *nfc, >>>> + struct device_node *np, >>>> + unsigned int chipnr) >>>> +{ >>>> + struct device *dev = &pdev->dev; >>>> + struct jz4780_nand_chip *nand; >>>> + struct jz4780_nand_cs *cs; >>>> + struct resource *res; >>>> + struct nand_chip *chip; >>>> + struct mtd_info *mtd; >>>> + const __be32 *reg; >>>> + int ret = 0; >>>> + >>>> + cs = &nfc->cs[chipnr]; >>>> + >>>> + reg = of_get_property(np, "reg", NULL); >>>> + if (!reg) >>>> + return -EINVAL; >>>> + >>>> + cs->bank = be32_to_cpu(*reg); >>>> + >>>> + jz4780_nemc_set_type(nfc->dev, cs->bank, JZ4780_NEMC_BANK_NAND); >>>> + >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, chipnr); >>>> + cs->base = devm_ioremap_resource(dev, res); >>>> + if (IS_ERR(cs->base)) >>>> + return PTR_ERR(cs->base); >>>> + >>>> + nand = devm_kzalloc(dev, sizeof(*nand), GFP_KERNEL); >>>> + if (!nand) >>>> + return -ENOMEM; >>>> + >>>> + nand->busy_gpio = devm_gpiod_get_optional(dev, "rb", GPIOD_IN); >>>> + >>>> + if (IS_ERR(nand->busy_gpio)) { >>>> + ret = PTR_ERR(nand->busy_gpio); >>>> + dev_err(dev, "failed to request busy GPIO: %d\n", ret); >>>> + return ret; >>>> + } else if (nand->busy_gpio) { >>>> + nand->chip.dev_ready = jz4780_nand_dev_ready; >>>> + } >>>> + >>>> + nand->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_LOW); >>>> + >>>> + if (IS_ERR(nand->wp_gpio)) { >>>> + ret = PTR_ERR(nand->wp_gpio); >>>> + dev_err(dev, "failed to request WP GPIO: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + chip = &nand->chip; >>>> + mtd = nand_to_mtd(chip); >>>> + mtd->priv = chip; >>>> + mtd->owner = THIS_MODULE; >>>> + mtd->name = DRV_NAME; >>>> + mtd->dev.parent = dev; >>>> + >>>> + chip->IO_ADDR_R = cs->base + OFFSET_DATA; >>>> + chip->IO_ADDR_W = cs->base + OFFSET_DATA; >>>> + chip->chip_delay = RB_DELAY_US; >>>> + chip->options = NAND_NO_SUBPAGE_WRITE; >>>> + chip->select_chip = jz4780_nand_select_chip; >>>> + chip->cmd_ctrl = jz4780_nand_cmd_ctrl; >>>> + chip->ecc.mode = NAND_ECC_HW; >>>> + chip->controller = &nfc->controller; >>>> + nand_set_flash_node(chip, np); >>>> + >>>> + ret = nand_scan_ident(mtd, 1, NULL); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = jz4780_nand_init_ecc(nand, dev); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = nand_scan_tail(mtd); >>>> + if (ret) >>>> + goto err_release_bch; >>>> + >>>> + ret = mtd_device_register(mtd, NULL, 0); >>>> + if (ret) >>>> + goto err_release_nand; >>>> + >>> >>> You probably miss a list_add_tail() call here (otherwise chips are >>> registered but not unregistered when ->remove() is called): >>> >>> list_add_tail(&nand->chip_list, &nfc->chips); >> >> Thanks for spotting this - I've fixed it now, >> >>> >>>> + return 0; >>>> + >>>> +err_release_nand: >>>> + nand_release(mtd); >>>> + >>>> +err_release_bch: >>>> + if (nfc->bch) >>>> + jz4780_bch_release(nfc->bch); >>> >>> Why are you releasing the BCH engine here, isn't it the role of the >>> NAND controller to do that? BTW, it's already done in >>> jz4780_nand_remove(). >> >> I don't think jz4780_nand_remove() will get called if we fail to >> initialise the chips, so perhaps it would be better to move this into >> jz4780_nand_probe? > > No, jz4780_nand_remove() won't get called in case of failure, I was > just trying to point the asymmetry here: if it's released in > ->remove() and assigned in ->probe(), then it should be released in > ->probe() in case of failure. So yes, moving it in jz4780_nand_probe() > is the right thing to do. I'll do that now. > > Best Regards, > > Boris > Thanks, Harvey