Re: [RFC PATCH v2 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller

Yixun Lan
2018-08-02T14:05:19Z

Hi Boris

On 08/02/2018 05:50 AM, Boris Brezillon wrote:
> Hi Yixun,
>
> On Thu, 19 Jul 2018 17:46:12 +0800
> Yixun Lan <yixun.lan@amlogic.com> wrote:
>
> I haven't finished reviewing the driver yet (I'll try to do that later
> this week), but I already pointed a few things to fix/improve.
>

thanks for the fully review, we really appreciate your time ;-)

I will comment on a few general items first, then clarify others after
talking to the NAND/ASIC team

>> +
>> +static int meson_nfc_exec_op(struct nand_chip *chip,
>> + const struct nand_operation *op, bool check_only)
>> +{
>> +
>> +static int meson_nfc_buffer_init(struct mtd_info *mtd)
>> +{
>> + struct nand_chip *nand = mtd_to_nand(mtd);
>> + struct meson_nfc *nfc = nand_get_controller_data(nand);
>> + int info_bytes, page_bytes;
>> + int nsectors;
>> +
>> + nsectors = mtd->writesize / nand->ecc.size;
>> + info_bytes = nsectors * PER_INFO_BYTE;
>> + page_bytes = mtd->writesize + mtd->oobsize;
>> +
>> + if (nfc->data_buf && nfc->info_buf)
>> + return 0;
>> +
>> + nfc->data_buf = kmalloc(page_bytes, GFP_KERNEL);
>
> I'm pretty sure that does not work if you have several chips. Either
> you have one buffer tied to the NFC, and it has to be large enough to
> handle the NAND with the largest page, or you have one buffer per chip.
>
em, we will fix this in next version,

>> + if (!nfc->data_buf)
>> + return -ENOMEM;
>> +
>> + nfc->info_buf = kmalloc(info_bytes, GFP_KERNEL);
>> + if (!nfc->info_buf) {
>> + kfree(nfc->data_buf);
>> + return -ENOMEM;
>> + }
>> +
>
> Those buffers are not removed in the cleanup/error path.
>
indeed, thanks for pointing out.
we actually realized this error after sent out this patch ..
>> + return 0;
>> +}
>> +
>> +static int meson_nfc_calc_set_timing(struct meson_nfc *nfc,
>> + int rc_min, int rea_max, int rhoh_min)
..

>> +
>> +static int
>> +meson_nfc_nand_chip_init(struct device *dev,
>> + struct meson_nfc *nfc, struct device_node *np)
>> +{
>> + struct meson_nfc_nand_chip *chip;
>> + struct nand_chip *nand;
>> + struct mtd_info *mtd;
>> + int ret, nsels, i, len = 0;
>> + char cs_id[16];
>> + u32 tmp;
>> +
>> + if (!of_get_property(np, "reg", &nsels))
>> + return -EINVAL;
>> +
>> + nsels /= sizeof(u32);
>> + if (!nsels || nsels > MAX_CE_NUM) {
>> + dev_err(dev, "invalid reg property size\n");
>> + return -EINVAL;
>> + }
>> +
>> + chip = devm_kzalloc(dev, sizeof(*chip) + (nsels * sizeof(u8)),
>> + GFP_KERNEL);
>> + if (!chip)
>> + return -ENOMEM;
>> +
>> + chip->nsels = nsels;
>> +
>> + for (i = 0; i < nsels; i++) {
>> + ret = of_property_read_u32_index(np, "reg", i, &tmp);
>> + if (ret) {
>> + dev_err(dev, "could not retrieve reg property: %d\n",
>> + ret);
>> + return ret;
>> + }
>> + chip->sels[i] = tmp;
>
> You should probably keep track of all the already assigned CS lines, to
> prevent situations where the same controller-CS is used twice
> (copy&paste error when writing the DT).
>

will do in next version, we would consider to use a bitmap for tracking
this ..
>> + len += snprintf(cs_id + len, 16, "%s%d", i ? "-" : ".", tmp);
>
> Hm, do we really need to be that accurate? I'd suggest using the first
> CS only.
>
ok, this would much simple..
thanks for the suggestion and the detail sample code in the following
section ;-)
>> + }
>> +
>> + chip->is_scramble =
>> + of_property_read_bool(np, "amlogic,nand-enable-scrambler");
>
> I think I already complained about that :P. If you think this is still
> needed (meaning that the autodetection + NAND_NEED_SCRAMBLING flag are
> not enough), I'll need a detailed explanation ;-).
>

yes, we saw this kind comment in DT patch already, we will try to fix this..
>> +
>> + nand = &chip->nand;
>> + nand_set_flash_node(nand, np);
>> + nand_set_controller_data(nand, nfc);
>> +
>> + nand->options |= NAND_USE_BOUNCE_BUFFER;
>> + nand->select_chip = meson_nfc_select_chip;
>> + nand->exec_op = meson_nfc_exec_op;
>> + nand->setup_data_interface = meson_nfc_setup_data_interface;
>> +
>> + nand->ecc.mode = NAND_ECC_HW;
>> +
>> + nand->ecc.write_page_raw = meson_nfc_write_page_raw;
>> + nand->ecc.write_page = meson_nfc_write_page_hwecc;
>> + nand->ecc.write_oob_raw = nand_write_oob_std;
>> + nand->ecc.write_oob = nand_write_oob_std;
>> +
>> + nand->ecc.read_page_raw = meson_nfc_read_page_raw;
>> + nand->ecc.read_page = meson_nfc_read_page_hwecc;
>> + nand->ecc.read_oob_raw = meson_nfc_read_oob_raw;
>> + nand->ecc.read_oob = meson_nfc_read_oob;
>
> We usually setup the ECC fields after the identification phase. This
> way, if you ever want/need to support SW ECC, the code is already
> properly placed.
>
>> +
>> + mtd = nand_to_mtd(nand);
>> + mtd->owner = THIS_MODULE;
>> + mtd->dev.parent = dev;
>> +
>> + ret = nand_scan_ident(mtd, 1, NULL);
>> + if (ret) {
>> + dev_err(dev, "failed to can ident\n");
>> + return -ENODEV;
>> + }
>> +
>> + mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL, "%s:nand%s",
>> + dev_name(dev), cs_id);
>
> So, if you follow my suggestion, it should be:
>
>
> You should make that conditional, because the core might have retrieved
> a user-friendly from the DT (label prop defined to the NAND chip node).
>
> So, if you follow my suggestion to just use the first CS for the nand
> id, that gives:
>
> if (!mtd->name) {
> mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL,
> "%s:nand%d",
> dev_name(dev),
> chip->sels[i]);
> if (!mtd->name)
> return -ENOMEM;
> }
sure
>> +
>> + /* store bbt magic in page, cause OOB is not protected */
>> + if (nand->bbt_options & NAND_BBT_USE_FLASH)
>> + nand->bbt_options |= NAND_BBT_NO_OOB;
>> +
>> + nand->options |= NAND_NO_SUBPAGE_WRITE;
>> +
>> + ret = meson_nfc_ecc_init(dev, mtd);
>> + if (ret) {
>> + dev_err(dev, "failed to ecc init\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (nand->options & NAND_BUSWIDTH_16) {
>> + dev_err(dev, "16bits buswidth not supported");
>> + return -EINVAL;
>> + }
>> +
>> + ret = meson_nfc_buffer_init(mtd);
>> + if (ret)
>> + return -ENOMEM;
>> +
>> + ret = nand_scan_tail(mtd);
>
> Miquel has reworked the nand_scan() infrastructure recently. Now you
> have to use nand_scan() and define ->attach_chip() hook to do all the
> init that happens between nand_scan_ident() and nand_scan_tail() in
> your code. And of course, all resources allocated in the
> ->attach_chip() hook should be freed in ->detach_chip().
>
thanks, will look into this

>> + if (ret)
>> + return -ENODEV;
>> +
>> + ret = mtd_device_register(mtd, NULL, 0);
>> + if (ret) {
>> + dev_err(dev, "failed to register mtd device: %d\n", ret);
>> + nand_cleanup(nand);
>> + return ret;
>> + }
>> +
>> + list_add_tail(&chip->node, &nfc->chips);
>> +
>> + return 0;
>> +}
>> +
>> +static int meson_nfc_nand_chip_cleanup(struct meson_nfc *nfc)
>
> ^ chips
>
sure

>> +{
>> + struct meson_nfc_nand_chip *chip;
>> + struct mtd_info *mtd;
>> + int ret;
>> +
>> + while (!list_empty(&nfc->chips)) {
>> + chip = list_first_entry(&nfc->chips, struct meson_nfc_nand_chip,
>> + node);
>> + mtd = nand_to_mtd(&chip->nand);
>> + ret = mtd_device_unregister(mtd);
>> + if (ret)
>> + return ret;
>> +
>> + nand_cleanup(&chip->nand);
>> + list_del(&chip->node);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int meson_nfc_nand_chips_init(struct device *dev, struct meson_nfc *nfc)
>> +{
>> + struct device_node *np = dev->of_node;
>> + struct device_node *nand_np;
>> + int ret;
>> +
>> + for_each_child_of_node(np, nand_np) {
>> + ret = meson_nfc_nand_chip_init(dev, nfc, nand_np);
>> + if (ret) {
>> + meson_nfc_nand_chip_cleanup(nfc);
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t meson_nfc_irq(int irq, void *id)
>> +{
>> + struct meson_nfc *nfc = id;
>> + u32 cfg;
>> +
>> + cfg = readl(nfc->reg_base + NFC_REG_CFG);
>> + cfg |= (1 << 21);
>> + writel(cfg, nfc->reg_base + NFC_REG_CFG);
>> +
>> + complete(&nfc->completion);
>> + return IRQ_HANDLED;
>
> Can you check if at least one event has been generated, and if not
> return IRQ_NONE?
>
sure, will address this in next version
>> +}
>> +
>> +static const struct meson_nfc_data meson_gxl_data = {
>> + .short_bch = NFC_ECC_BCH60_1K,
>> + .ecc = meson_gxl_ecc,
>> + .ecc_num = ARRAY_SIZE(meson_gxl_ecc),
>> +};
>> +
>> +static const struct meson_nfc_data meson_axg_data = {
>> + .short_bch = NFC_ECC_BCH8_1K,
>> + .ecc = meson_axg_ecc,
>> + .ecc_num = ARRAY_SIZE(meson_axg_ecc),
>> +};
>> +
>> +static const struct of_device_id meson_nfc_id_table[] = {
>> + {
>> + .compatible = "amlogic,meson-gxl-nfc",
>> + .data = &meson_gxl_data,
>> + }, {
>> + .compatible = "amlogic,meson-axg-nfc",
>> + .data = &meson_axg_data,
>> + },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, meson_nfc_id_table);
>> +
>> +static int meson_nfc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct meson_nfc *nfc;
>> + struct resource *res;
>> + int ret, irq;
>> +
>> + nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
>> + if (!nfc)
>> + return -ENOMEM;
>> +
>> + nfc->data =
>> + (struct meson_nfc_data *)of_device_get_match_data(&pdev->dev);
>
> You don't need the cast if you declare nfc->data as const in the struct
> def.
>
ok

>> + if (!nfc->data)
>> + return -ENODEV;
>> +
>> + spin_lock_init(&nfc->controller.lock);
>> + init_waitqueue_head(&nfc->controller.wq);
>
> There's a helper for that (nand_controller_init()).
>
ok
>> + INIT_LIST_HEAD(&nfc->chips);
>> +
>> + nfc->dev = dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_err(dev, "Failed to nfc reg resource\n");
>> + return -EINVAL;
>> + }
>
> No need to do this check, just pass res to devm_ioremap_resource() and
> it will do the check for you.
>
>> +
>> + nfc->reg_base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(nfc->reg_base)) {
>> + dev_err(dev, "Failed to lookup nfi reg base\n");
>
> This error message is not needed, devm_ioremap_resource() complains
> already.
>
> Just do:
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> nfc->reg_base = devm_ioremap_resource(dev, res);
> if (IS_ERR(nfc->reg_base))
> return PTR_ERR(nfc->reg_base);
>

em.. indeed, thanks

>> + return PTR_ERR(nfc->reg_base);
>> + }
>> +
>> + nfc->reg_clk =
>> + syscon_regmap_lookup_by_phandle(dev->of_node,
>> + "amlogic,mmc-syscon");
>> + if (IS_ERR(nfc->reg_clk)) {
>> + dev_err(dev, "Failed to lookup clock base\n");
>> + return PTR_ERR(nfc->reg_clk);
>> + }
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0) {
>> + dev_err(dev, "no nfi irq resource\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = meson_nfc_clk_init(nfc);
>> + if (ret) {
>> + dev_err(dev, "failed to initialize nand clk\n");
>> + goto err_clk;
>> + }
>> +
>> + ret = devm_request_irq(dev, irq, meson_nfc_irq, 0, dev_name(dev), nfc);
>
> You should make sure all irqs are masked/cleared before setting up your
> irq handler.
>
ok, will do
>> + if (ret) {
>> + dev_err(dev, "failed to request nfi irq\n");
>> + ret = -EINVAL;
>> + goto err_clk;
>> + }
>> +
>> + ret = dma_set_mask(dev, DMA_BIT_MASK(32));
>> + if (ret) {
>> + dev_err(dev, "failed to set dma mask\n");
>> + goto err_clk;
>> + }
>> +
>> + platform_set_drvdata(pdev, nfc);
>> +
>> + ret = meson_nfc_nand_chips_init(dev, nfc);
>> + if (ret) {
>> + dev_err(dev, "failed to init nand chips\n");
>> + goto err_clk;
>> + }
>> +
>> + return 0;
>> +
>> +err_clk:
>> + meson_nfc_disable_clk(nfc);
>> + return ret;
>> +}
>
> Regards,
>
> Boris
>
> .
>

Yixun