Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Geert,

geert@linux-m68k.org wrote on Tue, 10 May 2022 10:52:41 +0200:

> Hi Miquel,
> 
> On Mon, May 9, 2022 at 5:49 PM Miquel Raynal <miquel.raynal@bootlin.com> 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@bootlin.com>  
> 
> 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.

Oh crap, I moved the pm runtime calls below to reduce the amount of
error handling but I clearly overlooked that function call here.
 
> >         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.

What do you mean by "do this"? If you are talking about getting the
rate of the clock it's only needed when initializing the timings which
happens later in the probe path and has no relationship with the
interrupts at all. Anyway, I will move all of that earlier so that we
don't have a possible situation where the device is not clocked when
accessing it.

> 
> > +       clk_put(eclk);
> >
> >         rnandc_clear_fifo(rnandc);
> >  
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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


Thanks,
Miquèl