Hi Miquel,
On Mon, May 9, 2022 at 5:49 PM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> This NAND controller is part of a well defined power domain handled by
> the runtime PM core. Let's keep the harmony with the other RZ/N1 drivers
> and exclusively use the runtime PM API to enable/disable the clocks.
>
> We still need to retrieve the external clock rate in order to derive the
> NAND timings, but that is not a big deal, we can still do that in the
> probe and just save this value to reuse it later.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
Thanks for your patch!
> --- a/drivers/mtd/nand/raw/renesas-nand-controller.c
> +++ b/drivers/mtd/nand/raw/renesas-nand-controller.c
> @@ -1335,29 +1336,10 @@ static int rnandc_probe(struct platform_device *pdev)
> if (IS_ERR(rnandc->regs))
> return PTR_ERR(rnandc->regs);
>
> - /* APB clock */
> - rnandc->hclk = devm_clk_get(&pdev->dev, "hclk");
> - if (IS_ERR(rnandc->hclk))
> - return PTR_ERR(rnandc->hclk);
> -
> - /* External NAND bus clock */
> - rnandc->eclk = devm_clk_get(&pdev->dev, "eclk");
> - if (IS_ERR(rnandc->eclk))
> - return PTR_ERR(rnandc->eclk);
> -
> - ret = clk_prepare_enable(rnandc->hclk);
> - if (ret)
> - return ret;
> -
> - ret = clk_prepare_enable(rnandc->eclk);
> - if (ret)
> - goto disable_hclk;
> -
> rnandc_dis_interrupts(rnandc);
This touches the hardware, so the device must be operational.
Hence please move the pm_runtime_*() calls up, to make sure the device
is accessible.
Sorry for missing that before.
> irq = platform_get_irq_optional(pdev, 0);
> if (irq == -EPROBE_DEFER) {
> - ret = irq;
> - goto disable_eclk;
> + return irq;
> } else if (irq < 0) {
> dev_info(&pdev->dev, "No IRQ found, fallback to polling\n");
> rnandc->use_polling = true;
> @@ -1365,12 +1347,27 @@ static int rnandc_probe(struct platform_device *pdev)
> ret = devm_request_irq(&pdev->dev, irq, rnandc_irq_handler, 0,
> "renesas-nand-controller", rnandc);
> if (ret < 0)
> - goto disable_eclk;
> + return ret;
> }
>
> ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> if (ret)
> - goto disable_eclk;
> + return ret;
> +
> + devm_pm_runtime_enable(&pdev->dev);
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0)
> + return ret;
> +
> + /* The external NAND bus clock rate is needed for computing timings */
> + eclk = clk_get(&pdev->dev, "eclk");
> + if (IS_ERR(eclk)) {
> + ret = PTR_ERR(eclk);
> + goto dis_runtime_pm;
> + }
> +
> + rnandc->ext_clk_rate = clk_get_rate(eclk);
Personally, I would do this before requesting the interrupt.
But I guess it's fine to do that here, too.
> + clk_put(eclk);
>
> rnandc_clear_fifo(rnandc);
>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
