* [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
@ 2022-05-09 15:49 ` Miquel Raynal
0 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2022-05-09 15:49 UTC (permalink / raw)
To: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, linux-mtd
Cc: linux-renesas-soc, Magnus Damm, Gareth Williams, Phil Edworthy,
Geert Uytterhoeven, Rob Herring, Krzysztof Kozlowski, devicetree,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger, Miquel Raynal
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>
---
.../mtd/nand/raw/renesas-nand-controller.c | 58 +++++++++----------
1 file changed, 26 insertions(+), 32 deletions(-)
diff --git a/drivers/mtd/nand/raw/renesas-nand-controller.c b/drivers/mtd/nand/raw/renesas-nand-controller.c
index 6db063b230a9..432c7c22c16c 100644
--- a/drivers/mtd/nand/raw/renesas-nand-controller.c
+++ b/drivers/mtd/nand/raw/renesas-nand-controller.c
@@ -16,6 +16,7 @@
#include <linux/mtd/rawnand.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#define COMMAND_REG 0x00
@@ -216,8 +217,7 @@ struct rnandc {
struct nand_controller controller;
struct device *dev;
void __iomem *regs;
- struct clk *hclk;
- struct clk *eclk;
+ unsigned long ext_clk_rate;
unsigned long assigned_cs;
struct list_head chips;
struct nand_chip *selected_chip;
@@ -891,7 +891,7 @@ static int rnandc_setup_interface(struct nand_chip *chip, int chipnr,
{
struct rnand_chip *rnand = to_rnand(chip);
struct rnandc *rnandc = to_rnandc(chip->controller);
- unsigned int period_ns = 1000000000 / clk_get_rate(rnandc->eclk);
+ unsigned int period_ns = 1000000000 / rnandc->ext_clk_rate;
const struct nand_sdr_timings *sdr;
unsigned int cyc, cle, ale, bef_dly, ca_to_data;
@@ -1319,6 +1319,7 @@ static int rnandc_chips_init(struct rnandc *rnandc)
static int rnandc_probe(struct platform_device *pdev)
{
struct rnandc *rnandc;
+ struct clk *eclk;
int irq, ret;
rnandc = devm_kzalloc(&pdev->dev, sizeof(*rnandc), GFP_KERNEL);
@@ -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);
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);
+ clk_put(eclk);
rnandc_clear_fifo(rnandc);
@@ -1378,14 +1375,12 @@ static int rnandc_probe(struct platform_device *pdev)
ret = rnandc_chips_init(rnandc);
if (ret)
- goto disable_eclk;
+ goto dis_runtime_pm;
return 0;
-disable_eclk:
- clk_disable_unprepare(rnandc->eclk);
-disable_hclk:
- clk_disable_unprepare(rnandc->hclk);
+dis_runtime_pm:
+ pm_runtime_put(&pdev->dev);
return ret;
}
@@ -1396,8 +1391,7 @@ static int rnandc_remove(struct platform_device *pdev)
rnandc_chips_cleanup(rnandc);
- clk_disable_unprepare(rnandc->eclk);
- clk_disable_unprepare(rnandc->hclk);
+ pm_runtime_put(&pdev->dev);
return 0;
}
--
2.27.0
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
2022-05-09 15:49 ` Miquel Raynal
@ 2022-05-10 8:52 ` Geert Uytterhoeven
-1 siblings, 0 replies; 20+ messages in thread
From: Geert Uytterhoeven @ 2022-05-10 8:52 UTC (permalink / raw)
To: Miquel Raynal
Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, MTD Maling List, Linux-Renesas,
Magnus Damm, Gareth Williams, Phil Edworthy, Rob Herring,
Krzysztof Kozlowski,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
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.
> 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@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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
@ 2022-05-10 8:52 ` Geert Uytterhoeven
0 siblings, 0 replies; 20+ messages in thread
From: Geert Uytterhoeven @ 2022-05-10 8:52 UTC (permalink / raw)
To: Miquel Raynal
Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, MTD Maling List, Linux-Renesas,
Magnus Damm, Gareth Williams, Phil Edworthy, Rob Herring,
Krzysztof Kozlowski,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
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.
> 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@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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
2022-05-10 8:52 ` Geert Uytterhoeven
@ 2022-05-10 9:02 ` Miquel Raynal
-1 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2022-05-10 9:02 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, MTD Maling List, Linux-Renesas,
Magnus Damm, Gareth Williams, Phil Edworthy, Rob Herring,
Krzysztof Kozlowski,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
@ 2022-05-10 9:02 ` Miquel Raynal
0 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2022-05-10 9:02 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, MTD Maling List, Linux-Renesas,
Magnus Damm, Gareth Williams, Phil Edworthy, Rob Herring,
Krzysztof Kozlowski,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
2022-05-10 9:02 ` Miquel Raynal
@ 2022-05-10 9:14 ` Geert Uytterhoeven
-1 siblings, 0 replies; 20+ messages in thread
From: Geert Uytterhoeven @ 2022-05-10 9:14 UTC (permalink / raw)
To: Miquel Raynal
Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, MTD Maling List, Linux-Renesas,
Magnus Damm, Gareth Williams, Phil Edworthy, Rob Herring,
Krzysztof Kozlowski,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
Hi Miquel,
On Tue, May 10, 2022 at 11:03 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> geert@linux-m68k.org wrote on Tue, 10 May 2022 10:52:41 +0200:
> > 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>
> > > @@ -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
Getting the clock (which could -EPROBE_DEFER, in theory) and its rate.
> 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.
OK.
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
@ 2022-05-10 9:14 ` Geert Uytterhoeven
0 siblings, 0 replies; 20+ messages in thread
From: Geert Uytterhoeven @ 2022-05-10 9:14 UTC (permalink / raw)
To: Miquel Raynal
Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, MTD Maling List, Linux-Renesas,
Magnus Damm, Gareth Williams, Phil Edworthy, Rob Herring,
Krzysztof Kozlowski,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
Hi Miquel,
On Tue, May 10, 2022 at 11:03 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
> geert@linux-m68k.org wrote on Tue, 10 May 2022 10:52:41 +0200:
> > 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>
> > > @@ -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
Getting the clock (which could -EPROBE_DEFER, in theory) and its rate.
> 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.
OK.
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
2022-05-09 15:49 ` Miquel Raynal
@ 2022-05-12 15:27 ` Miquel Raynal
-1 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2022-05-12 15:27 UTC (permalink / raw)
To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd
Cc: linux-renesas-soc, Magnus Damm, Gareth Williams, Phil Edworthy,
Geert Uytterhoeven, Rob Herring, Krzysztof Kozlowski, devicetree,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
On Mon, 2022-05-09 at 15:49:43 UTC, Miquel Raynal 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>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next.
Miquel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
@ 2022-05-12 15:27 ` Miquel Raynal
0 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2022-05-12 15:27 UTC (permalink / raw)
To: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd
Cc: linux-renesas-soc, Magnus Damm, Gareth Williams, Phil Edworthy,
Geert Uytterhoeven, Rob Herring, Krzysztof Kozlowski, devicetree,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
On Mon, 2022-05-09 at 15:49:43 UTC, Miquel Raynal 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>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next.
Miquel
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
2022-05-12 15:27 ` Miquel Raynal
@ 2022-05-12 15:37 ` Geert Uytterhoeven
-1 siblings, 0 replies; 20+ messages in thread
From: Geert Uytterhoeven @ 2022-05-12 15:37 UTC (permalink / raw)
To: Miquel Raynal
Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, MTD Maling List, Linux-Renesas,
Magnus Damm, Gareth Williams, Phil Edworthy, Rob Herring,
Krzysztof Kozlowski,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
Hi Miquel,
On Thu, May 12, 2022 at 5:27 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> On Mon, 2022-05-09 at 15:49:43 UTC, Miquel Raynal 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>
>
> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next.
Without moving the pm_runtime_get_sync() call
(better: pm_runtime_resume_and_get()) before the first hardware register
access?
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
@ 2022-05-12 15:37 ` Geert Uytterhoeven
0 siblings, 0 replies; 20+ messages in thread
From: Geert Uytterhoeven @ 2022-05-12 15:37 UTC (permalink / raw)
To: Miquel Raynal
Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, MTD Maling List, Linux-Renesas,
Magnus Damm, Gareth Williams, Phil Edworthy, Rob Herring,
Krzysztof Kozlowski,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
Hi Miquel,
On Thu, May 12, 2022 at 5:27 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> On Mon, 2022-05-09 at 15:49:43 UTC, Miquel Raynal 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>
>
> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next.
Without moving the pm_runtime_get_sync() call
(better: pm_runtime_resume_and_get()) before the first hardware register
access?
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
2022-05-12 15:37 ` Geert Uytterhoeven
@ 2022-05-13 10:42 ` Miquel Raynal
-1 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2022-05-13 10:42 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, MTD Maling List, Linux-Renesas,
Magnus Damm, Gareth Williams, Phil Edworthy, Rob Herring,
Krzysztof Kozlowski,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
Hi Geert,
geert@linux-m68k.org wrote on Thu, 12 May 2022 17:37:52 +0200:
> Hi Miquel,
>
> On Thu, May 12, 2022 at 5:27 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > On Mon, 2022-05-09 at 15:49:43 UTC, Miquel Raynal 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>
> >
> > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next.
>
> Without moving the pm_runtime_get_sync() call
> (better: pm_runtime_resume_and_get()) before the first hardware register
> access?
That was an accidental move, thanks for noticing.
Thanks,
Miquèl
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] mtd: rawnand: renesas: Use runtime PM instead of the raw clock API
@ 2022-05-13 10:42 ` Miquel Raynal
0 siblings, 0 replies; 20+ messages in thread
From: Miquel Raynal @ 2022-05-13 10:42 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
Pratyush Yadav, Michael Walle, MTD Maling List, Linux-Renesas,
Magnus Damm, Gareth Williams, Phil Edworthy, Rob Herring,
Krzysztof Kozlowski,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Milan Stevanovic, Jimmy Lalande, Pascal Eberhard,
Thomas Petazzoni, Herve Codina, Clement Leger
Hi Geert,
geert@linux-m68k.org wrote on Thu, 12 May 2022 17:37:52 +0200:
> Hi Miquel,
>
> On Thu, May 12, 2022 at 5:27 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > On Mon, 2022-05-09 at 15:49:43 UTC, Miquel Raynal 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>
> >
> > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next.
>
> Without moving the pm_runtime_get_sync() call
> (better: pm_runtime_resume_and_get()) before the first hardware register
> access?
That was an accidental move, thanks for noticing.
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 20+ messages in thread