| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <42724eab-7840-27f2-31ad-b5bc10fd89fd@linux.intel.com>
Date: Fri, 30 Oct 2020 18:06:57 +0800
From: "Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@...ux.intel.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: vigneshr@...com, tudor.ambarus@...rochip.com,
linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org,
robh+dt@...nel.org, boris.brezillon@...labora.com,
devicetree@...r.kernel.org, simon.k.r.goldschmidt@...il.com,
dinguyen@...nel.org, richard@....at, cheol.yong.kim@...el.com,
qi-ming.wu@...el.com
Subject: Re: [RESENDPATCH v15 2/2] mtd: rawnand: Add NAND controller support
on Intel LGM SoC
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", ®);
>>>
>>> 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
>
Powered by blists - more mailing lists