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

[Yixun Lan]

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@xxxxxxxxxxx> 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