Re: [RESENDPATCH v15 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

["Ramuthevar, Vadivel MuruganX" <vadivel.muruganx.ramuthevar@linux.intel.com>]

Hi Miquel,

On 30/10/2020 4:23 pm, Miquel Raynal wrote:
> Hello,
> 
>>>> +static const struct nand_controller_ops ebu_nand_controller_ops = {
>>>> +	.attach_chip = ebu_nand_attach_chip,
>>>> +	.setup_interface = ebu_nand_set_timings,
>>>> +	.exec_op = ebu_nand_exec_op,
>>>> +};
>>>> +
>>>> +static void ebu_dma_cleanup(struct ebu_nand_controller *ebu_host)
>>>> +{
>>>> +	if (ebu_host->dma_rx)
>>>> +		dma_release_channel(ebu_host->dma_rx);
>>>> +
>>>> +	if (ebu_host->dma_tx)
>>>> +		dma_release_channel(ebu_host->dma_tx);
>>>> +}
>>>> +
>>>> +static int ebu_nand_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct device *dev = &pdev->dev;
>>>> +	struct ebu_nand_controller *ebu_host;
>>>> +	struct nand_chip *nand;
>>>> +	struct mtd_info *mtd;
>>>> +	struct resource *res;
>>>> +	char *resname;
>>>> +	int ret, i;
>>>> +	u32 reg;
>>>> +
>>>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
>>>> +	if (!ebu_host)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	ebu_host->dev = dev;
>>>> +	nand_controller_init(&ebu_host->controller);
>>>> +
>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
>>>> +	ebu_host->ebu = devm_ioremap_resource(&pdev->dev, res);
>>>> +	if (IS_ERR(ebu_host->ebu))
>>>> +		return PTR_ERR(ebu_host->ebu);
>>>> +
>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
>>>> +	ebu_host->hsnand = devm_ioremap_resource(&pdev->dev, res);
>>>> +	if (IS_ERR(ebu_host->hsnand))
>>>> +		return PTR_ERR(ebu_host->hsnand);
>>>> +
>>>> +	ret = device_property_read_u32(dev, "nand,cs", &reg);
>>>
>>> There is no nand,cs property. Use 'reg' instead.
>> Noted.
>>>    
>>>> +	if (ret) {
>>>> +		dev_err(dev, "failed to get chip select: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +	ebu_host->cs_num = reg;
>>>
>>> The following for loop is weird, above you can only store a single cs
>>> number, while below you seem to reserve serveral memory areas. Please
>>> clarify this code.
>> This IP supports 2 chip select for 2 different memory regions so we used the below for loop, as per reviewers comment updated.
>> 		 EBU_CS0_BASE     0xE1C0_0000 (Memory-Mapped)
>> 		 EBU_CS0_IO_BASE  0x1740_0000 (FPI I/O Mapped)
>>
>>                    EBU_CS1_BASE     0xE140_0000 (Memory-Mapped)
>>                    EBU_CS1_IO_BASE  0x17C0_0000 (FPI I/O Mapped)
> 
> Please make a difference between, "there are two CS, either can be
> picked but we can use only one in this driver" or "there are two CS,
> one or both can be used". You can start with supporting a single CS (no
> matter which one is picked by the user with the reg property) but in
> this case there is no such for loop because only 1 CS is used. Or you
> can decide that both CS can be populated and in this case you must
> handle this in ->select_chip().
Sure, I will hadle the same as per your suggestion, thanks a lot!

Regards
Vadivel
> 
>>
>>>    
>>>> +
>>>> +	for (i = 0; i < MAX_CS; i++) {
>>>> +		resname = devm_kasprintf(dev, GFP_KERNEL, "nand_cs%d", i);
>>>> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>>> +						   resname);
>>>> +		ebu_host->cs[i].chipaddr = devm_ioremap_resource(dev, res);
>>>> +		ebu_host->cs[i].nand_pa = res->start;
>>>> +		if (IS_ERR(ebu_host->cs[i].chipaddr))
>>>> +			return PTR_ERR(ebu_host->cs[i].chipaddr);
>>>> +	}
>>>> +
>>>> +	ebu_host->clk = devm_clk_get(dev, NULL);
>>>> +	if (IS_ERR(ebu_host->clk))
>>>> +		return dev_err_probe(dev, PTR_ERR(ebu_host->clk),
>>>> +				     "failed to get clock\n");
>>>> +
>>>> +	ret = clk_prepare_enable(ebu_host->clk);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "failed to enable clock: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +	ebu_host->clk_rate = clk_get_rate(ebu_host->clk);
>>>> +
>>>> +	ebu_host->dma_tx = dma_request_chan(dev, "tx");
>>>> +	if (IS_ERR(ebu_host->dma_tx))
>>>> +		return dev_err_probe(dev, PTR_ERR(ebu_host->dma_tx),
>>>> +				     "failed to request DMA tx chan!.\n");
>>>> +
>>>> +	ebu_host->dma_rx = dma_request_chan(dev, "rx");
>>>> +	if (IS_ERR(ebu_host->dma_rx))
>>>> +		return dev_err_probe(dev, PTR_ERR(ebu_host->dma_rx),
>>>> +				     "failed to request DMA rx chan!.\n");
>>>> +
>>>> +	for (i = 0; i < MAX_CS; i++) {
>>>> +		resname = devm_kasprintf(dev, GFP_KERNEL, "addr_sel%d", i);
>>>> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>>> +						   resname);
>>>> +		if (!res)
>>>> +			return -EINVAL;
>>>> +		ebu_host->cs[i].addr_sel = res->start;
>>>> +		writel(ebu_host->cs[i].addr_sel | EBU_ADDR_MASK(5) |
>>>> +		       EBU_ADDR_SEL_REGEN, ebu_host->ebu + EBU_ADDR_SEL(i));
>>>> +	}
>>>> +
>>>> +	nand_set_flash_node(&ebu_host->chip, dev->of_node);
>>>> +	if (!mtd->name) {
>>>> +		dev_err(ebu_host->dev, "NAND label property is mandatory\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	mtd = nand_to_mtd(&ebu_host->chip);
>>>> +	mtd->dev.parent = dev;
>>>> +	ebu_host->dev = dev;
>>>> +
>>>> +	platform_set_drvdata(pdev, ebu_host);
>>>> +	nand_set_controller_data(&ebu_host->chip, ebu_host);
>>>> +
>>>> +	nand = &ebu_host->chip;
>>>> +	nand->controller = &ebu_host->controller;
>>>> +	nand->controller->ops = &ebu_nand_controller_ops;
>>>> +
>>>> +	/* Scan to find existence of the device */
>>>> +	ret = nand_scan(&ebu_host->chip, 1);
>>>> +	if (ret)
>>>> +		goto err_cleanup_dma;
>>>> +
>>>> +	ret = mtd_device_register(mtd, NULL, 0);
>>>> +	if (ret)
>>>> +		goto err_clean_nand;
>>>> +
>>>> +	return 0;
>>>> +
>>>> +err_clean_nand:
>>>> +	nand_cleanup(&ebu_host->chip);
>>>> +err_cleanup_dma:
>>>> +	ebu_dma_cleanup(ebu_host);
>>>> +	clk_disable_unprepare(ebu_host->clk);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int ebu_nand_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct ebu_nand_controller *ebu_host = platform_get_drvdata(pdev);
>>>> +	int ret;
>>>> +
>>>> +	ret = mtd_device_unregister(nand_to_mtd(&ebu_host->chip));
>>>> +	WARN_ON(ret);
>>>> +	nand_cleanup(&ebu_host->chip);
>>>> +	ebu_nand_disable(&ebu_host->chip);
>>>> +	ebu_dma_cleanup(ebu_host);
>>>> +	clk_disable_unprepare(ebu_host->clk);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id ebu_nand_match[] = {
>>>> +	{ .compatible = "intel,nand-controller", },
>>>
>>> No version or soc in the compatible? (not mandatory).
>> Yes, you're right, it should be "intel,lgm-ebunand", but this same driver supports 2 dfferent SOC's , that's the reason kept as generic
>> "intel,nand-controller"
> 
> In this case I guess declaring two compatibles is the way to go.
> 
> 
> Thanks,
> Miquèl
>