diff mbox series

[v13,20/35] mtd: rawnand: tegra: Add runtime PM and OPP support

Message ID 20210926224058.1252-21-digetx@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show
Series NVIDIA Tegra power management patches for 5.16 | expand

Commit Message

Dmitry Osipenko Sept. 26, 2021, 10:40 p.m. UTC
The NAND on Tegra belongs to the core power domain and we're going to
enable GENPD support for the core domain. Now NAND must be resumed using
runtime PM API in order to initialize the NAND power state. Add runtime PM
and OPP support to the NAND driver.

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/mtd/nand/raw/tegra_nand.c | 55 ++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 8 deletions(-)

Comments

Ulf Hansson Oct. 1, 2021, 2:24 p.m. UTC | #1
On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> The NAND on Tegra belongs to the core power domain and we're going to
> enable GENPD support for the core domain. Now NAND must be resumed using
> runtime PM API in order to initialize the NAND power state. Add runtime PM
> and OPP support to the NAND driver.
>
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/mtd/nand/raw/tegra_nand.c | 55 ++++++++++++++++++++++++++-----
>  1 file changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
> index 32431bbe69b8..098fcc9cb9df 100644
> --- a/drivers/mtd/nand/raw/tegra_nand.c
> +++ b/drivers/mtd/nand/raw/tegra_nand.c
> @@ -17,8 +17,11 @@
>  #include <linux/mtd/rawnand.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/reset.h>
>
> +#include <soc/tegra/common.h>
> +
>  #define COMMAND                                        0x00
>  #define   COMMAND_GO                           BIT(31)
>  #define   COMMAND_CLE                          BIT(30)
> @@ -1151,6 +1154,7 @@ static int tegra_nand_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>
>         ctrl->dev = &pdev->dev;
> +       platform_set_drvdata(pdev, ctrl);
>         nand_controller_init(&ctrl->controller);
>         ctrl->controller.ops = &tegra_nand_controller_ops;
>
> @@ -1166,14 +1170,22 @@ static int tegra_nand_probe(struct platform_device *pdev)
>         if (IS_ERR(ctrl->clk))
>                 return PTR_ERR(ctrl->clk);
>
> -       err = clk_prepare_enable(ctrl->clk);
> +       err = devm_pm_runtime_enable(&pdev->dev);
> +       if (err)
> +               return err;
> +
> +       err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
> +       if (err)
> +               return err;
> +
> +       err = pm_runtime_resume_and_get(&pdev->dev);
>         if (err)
>                 return err;
>
>         err = reset_control_reset(rst);
>         if (err) {
>                 dev_err(ctrl->dev, "Failed to reset HW: %d\n", err);
> -               goto err_disable_clk;
> +               goto err_put_pm;
>         }
>
>         writel_relaxed(HWSTATUS_CMD_DEFAULT, ctrl->regs + HWSTATUS_CMD);
> @@ -1188,21 +1200,19 @@ static int tegra_nand_probe(struct platform_device *pdev)
>                                dev_name(&pdev->dev), ctrl);
>         if (err) {
>                 dev_err(ctrl->dev, "Failed to get IRQ: %d\n", err);
> -               goto err_disable_clk;
> +               goto err_put_pm;
>         }
>
>         writel_relaxed(DMA_MST_CTRL_IS_DONE, ctrl->regs + DMA_MST_CTRL);
>
>         err = tegra_nand_chips_init(ctrl->dev, ctrl);
>         if (err)
> -               goto err_disable_clk;
> -
> -       platform_set_drvdata(pdev, ctrl);
> +               goto err_put_pm;
>

There is no corresponding call pm_runtime_put() here. Is it
intentional to always leave the device runtime resumed after ->probe()
has succeeded?

I noticed you included some comments about this for some other
drivers, as those needed more tweaks. Is that also the case for this
driver?

>         return 0;
>
> -err_disable_clk:
> -       clk_disable_unprepare(ctrl->clk);
> +err_put_pm:
> +       pm_runtime_put(ctrl->dev);
>         return err;
>  }
>

[...]

Kind regards
Uffe
Dmitry Osipenko Oct. 1, 2021, 2:35 p.m. UTC | #2
01.10.2021 17:24, Ulf Hansson пишет:
> On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> The NAND on Tegra belongs to the core power domain and we're going to
>> enable GENPD support for the core domain. Now NAND must be resumed using
>> runtime PM API in order to initialize the NAND power state. Add runtime PM
>> and OPP support to the NAND driver.
>>
>> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/mtd/nand/raw/tegra_nand.c | 55 ++++++++++++++++++++++++++-----
>>  1 file changed, 47 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
>> index 32431bbe69b8..098fcc9cb9df 100644
>> --- a/drivers/mtd/nand/raw/tegra_nand.c
>> +++ b/drivers/mtd/nand/raw/tegra_nand.c
>> @@ -17,8 +17,11 @@
>>  #include <linux/mtd/rawnand.h>
>>  #include <linux/of.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/reset.h>
>>
>> +#include <soc/tegra/common.h>
>> +
>>  #define COMMAND                                        0x00
>>  #define   COMMAND_GO                           BIT(31)
>>  #define   COMMAND_CLE                          BIT(30)
>> @@ -1151,6 +1154,7 @@ static int tegra_nand_probe(struct platform_device *pdev)
>>                 return -ENOMEM;
>>
>>         ctrl->dev = &pdev->dev;
>> +       platform_set_drvdata(pdev, ctrl);
>>         nand_controller_init(&ctrl->controller);
>>         ctrl->controller.ops = &tegra_nand_controller_ops;
>>
>> @@ -1166,14 +1170,22 @@ static int tegra_nand_probe(struct platform_device *pdev)
>>         if (IS_ERR(ctrl->clk))
>>                 return PTR_ERR(ctrl->clk);
>>
>> -       err = clk_prepare_enable(ctrl->clk);
>> +       err = devm_pm_runtime_enable(&pdev->dev);
>> +       if (err)
>> +               return err;
>> +
>> +       err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
>> +       if (err)
>> +               return err;
>> +
>> +       err = pm_runtime_resume_and_get(&pdev->dev);
>>         if (err)
>>                 return err;
>>
>>         err = reset_control_reset(rst);
>>         if (err) {
>>                 dev_err(ctrl->dev, "Failed to reset HW: %d\n", err);
>> -               goto err_disable_clk;
>> +               goto err_put_pm;
>>         }
>>
>>         writel_relaxed(HWSTATUS_CMD_DEFAULT, ctrl->regs + HWSTATUS_CMD);
>> @@ -1188,21 +1200,19 @@ static int tegra_nand_probe(struct platform_device *pdev)
>>                                dev_name(&pdev->dev), ctrl);
>>         if (err) {
>>                 dev_err(ctrl->dev, "Failed to get IRQ: %d\n", err);
>> -               goto err_disable_clk;
>> +               goto err_put_pm;
>>         }
>>
>>         writel_relaxed(DMA_MST_CTRL_IS_DONE, ctrl->regs + DMA_MST_CTRL);
>>
>>         err = tegra_nand_chips_init(ctrl->dev, ctrl);
>>         if (err)
>> -               goto err_disable_clk;
>> -
>> -       platform_set_drvdata(pdev, ctrl);
>> +               goto err_put_pm;
>>
> 
> There is no corresponding call pm_runtime_put() here. Is it
> intentional to always leave the device runtime resumed after ->probe()
> has succeeded?
> 
> I noticed you included some comments about this for some other
> drivers, as those needed more tweaks. Is that also the case for this
> driver?

Could you please clarify? There is pm_runtime_put() in both probe-error
and remove() code paths here.

I assume you're meaning pm_runtime_disable(), but this patch uses
resource-managed devm_pm_runtime_enable(), and thus, explicit disable
isn't needed.

>>         return 0;
>>
>> -err_disable_clk:
>> -       clk_disable_unprepare(ctrl->clk);
>> +err_put_pm:
>> +       pm_runtime_put(ctrl->dev);
>>         return err;
>>  }
>>
> 
> [...]
> 
> Kind regards
> Uffe
>
Ulf Hansson Oct. 1, 2021, 3:01 p.m. UTC | #3
On Fri, 1 Oct 2021 at 16:35, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 01.10.2021 17:24, Ulf Hansson пишет:
> > On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> The NAND on Tegra belongs to the core power domain and we're going to
> >> enable GENPD support for the core domain. Now NAND must be resumed using
> >> runtime PM API in order to initialize the NAND power state. Add runtime PM
> >> and OPP support to the NAND driver.
> >>
> >> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/mtd/nand/raw/tegra_nand.c | 55 ++++++++++++++++++++++++++-----
> >>  1 file changed, 47 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
> >> index 32431bbe69b8..098fcc9cb9df 100644
> >> --- a/drivers/mtd/nand/raw/tegra_nand.c
> >> +++ b/drivers/mtd/nand/raw/tegra_nand.c
> >> @@ -17,8 +17,11 @@
> >>  #include <linux/mtd/rawnand.h>
> >>  #include <linux/of.h>
> >>  #include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >>  #include <linux/reset.h>
> >>
> >> +#include <soc/tegra/common.h>
> >> +
> >>  #define COMMAND                                        0x00
> >>  #define   COMMAND_GO                           BIT(31)
> >>  #define   COMMAND_CLE                          BIT(30)
> >> @@ -1151,6 +1154,7 @@ static int tegra_nand_probe(struct platform_device *pdev)
> >>                 return -ENOMEM;
> >>
> >>         ctrl->dev = &pdev->dev;
> >> +       platform_set_drvdata(pdev, ctrl);
> >>         nand_controller_init(&ctrl->controller);
> >>         ctrl->controller.ops = &tegra_nand_controller_ops;
> >>
> >> @@ -1166,14 +1170,22 @@ static int tegra_nand_probe(struct platform_device *pdev)
> >>         if (IS_ERR(ctrl->clk))
> >>                 return PTR_ERR(ctrl->clk);
> >>
> >> -       err = clk_prepare_enable(ctrl->clk);
> >> +       err = devm_pm_runtime_enable(&pdev->dev);
> >> +       if (err)
> >> +               return err;
> >> +
> >> +       err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
> >> +       if (err)
> >> +               return err;
> >> +
> >> +       err = pm_runtime_resume_and_get(&pdev->dev);
> >>         if (err)
> >>                 return err;
> >>
> >>         err = reset_control_reset(rst);
> >>         if (err) {
> >>                 dev_err(ctrl->dev, "Failed to reset HW: %d\n", err);
> >> -               goto err_disable_clk;
> >> +               goto err_put_pm;
> >>         }
> >>
> >>         writel_relaxed(HWSTATUS_CMD_DEFAULT, ctrl->regs + HWSTATUS_CMD);
> >> @@ -1188,21 +1200,19 @@ static int tegra_nand_probe(struct platform_device *pdev)
> >>                                dev_name(&pdev->dev), ctrl);
> >>         if (err) {
> >>                 dev_err(ctrl->dev, "Failed to get IRQ: %d\n", err);
> >> -               goto err_disable_clk;
> >> +               goto err_put_pm;
> >>         }
> >>
> >>         writel_relaxed(DMA_MST_CTRL_IS_DONE, ctrl->regs + DMA_MST_CTRL);
> >>
> >>         err = tegra_nand_chips_init(ctrl->dev, ctrl);
> >>         if (err)
> >> -               goto err_disable_clk;
> >> -
> >> -       platform_set_drvdata(pdev, ctrl);
> >> +               goto err_put_pm;
> >>
> >
> > There is no corresponding call pm_runtime_put() here. Is it
> > intentional to always leave the device runtime resumed after ->probe()
> > has succeeded?
> >
> > I noticed you included some comments about this for some other
> > drivers, as those needed more tweaks. Is that also the case for this
> > driver?
>
> Could you please clarify? There is pm_runtime_put() in both probe-error
> and remove() code paths here.

I was not considering the error path of ->probe() (or ->remove()), but
was rather thinking about when ->probe() completes successfully. Then
you keep the device runtime resumed, because you have called
pm_runtime_resume_and_get() for it.

Shouldn't you have a corresponding pm_runtime_put() in ->probe(),
allowing it to be runtime suspended, until the device is really needed
later on. No?

>
> I assume you're meaning pm_runtime_disable(), but this patch uses
> resource-managed devm_pm_runtime_enable(), and thus, explicit disable
> isn't needed.
>
> >>         return 0;
> >>
> >> -err_disable_clk:
> >> -       clk_disable_unprepare(ctrl->clk);
> >> +err_put_pm:
> >> +       pm_runtime_put(ctrl->dev);
> >>         return err;
> >>  }
> >>

[...]

Kind regards
Uffe
Dmitry Osipenko Oct. 17, 2021, 8:38 a.m. UTC | #4
01.10.2021 18:01, Ulf Hansson пишет:
> On Fri, 1 Oct 2021 at 16:35, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 01.10.2021 17:24, Ulf Hansson пишет:
>>> On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> The NAND on Tegra belongs to the core power domain and we're going to
>>>> enable GENPD support for the core domain. Now NAND must be resumed using
>>>> runtime PM API in order to initialize the NAND power state. Add runtime PM
>>>> and OPP support to the NAND driver.
>>>>
>>>> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/mtd/nand/raw/tegra_nand.c | 55 ++++++++++++++++++++++++++-----
>>>>  1 file changed, 47 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
>>>> index 32431bbe69b8..098fcc9cb9df 100644
>>>> --- a/drivers/mtd/nand/raw/tegra_nand.c
>>>> +++ b/drivers/mtd/nand/raw/tegra_nand.c
>>>> @@ -17,8 +17,11 @@
>>>>  #include <linux/mtd/rawnand.h>
>>>>  #include <linux/of.h>
>>>>  #include <linux/platform_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>>  #include <linux/reset.h>
>>>>
>>>> +#include <soc/tegra/common.h>
>>>> +
>>>>  #define COMMAND                                        0x00
>>>>  #define   COMMAND_GO                           BIT(31)
>>>>  #define   COMMAND_CLE                          BIT(30)
>>>> @@ -1151,6 +1154,7 @@ static int tegra_nand_probe(struct platform_device *pdev)
>>>>                 return -ENOMEM;
>>>>
>>>>         ctrl->dev = &pdev->dev;
>>>> +       platform_set_drvdata(pdev, ctrl);
>>>>         nand_controller_init(&ctrl->controller);
>>>>         ctrl->controller.ops = &tegra_nand_controller_ops;
>>>>
>>>> @@ -1166,14 +1170,22 @@ static int tegra_nand_probe(struct platform_device *pdev)
>>>>         if (IS_ERR(ctrl->clk))
>>>>                 return PTR_ERR(ctrl->clk);
>>>>
>>>> -       err = clk_prepare_enable(ctrl->clk);
>>>> +       err = devm_pm_runtime_enable(&pdev->dev);
>>>> +       if (err)
>>>> +               return err;
>>>> +
>>>> +       err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
>>>> +       if (err)
>>>> +               return err;
>>>> +
>>>> +       err = pm_runtime_resume_and_get(&pdev->dev);
>>>>         if (err)
>>>>                 return err;
>>>>
>>>>         err = reset_control_reset(rst);
>>>>         if (err) {
>>>>                 dev_err(ctrl->dev, "Failed to reset HW: %d\n", err);
>>>> -               goto err_disable_clk;
>>>> +               goto err_put_pm;
>>>>         }
>>>>
>>>>         writel_relaxed(HWSTATUS_CMD_DEFAULT, ctrl->regs + HWSTATUS_CMD);
>>>> @@ -1188,21 +1200,19 @@ static int tegra_nand_probe(struct platform_device *pdev)
>>>>                                dev_name(&pdev->dev), ctrl);
>>>>         if (err) {
>>>>                 dev_err(ctrl->dev, "Failed to get IRQ: %d\n", err);
>>>> -               goto err_disable_clk;
>>>> +               goto err_put_pm;
>>>>         }
>>>>
>>>>         writel_relaxed(DMA_MST_CTRL_IS_DONE, ctrl->regs + DMA_MST_CTRL);
>>>>
>>>>         err = tegra_nand_chips_init(ctrl->dev, ctrl);
>>>>         if (err)
>>>> -               goto err_disable_clk;
>>>> -
>>>> -       platform_set_drvdata(pdev, ctrl);
>>>> +               goto err_put_pm;
>>>>
>>>
>>> There is no corresponding call pm_runtime_put() here. Is it
>>> intentional to always leave the device runtime resumed after ->probe()
>>> has succeeded?
>>>
>>> I noticed you included some comments about this for some other
>>> drivers, as those needed more tweaks. Is that also the case for this
>>> driver?
>>
>> Could you please clarify? There is pm_runtime_put() in both probe-error
>> and remove() code paths here.
> 
> I was not considering the error path of ->probe() (or ->remove()), but
> was rather thinking about when ->probe() completes successfully. Then
> you keep the device runtime resumed, because you have called
> pm_runtime_resume_and_get() for it.
> 
> Shouldn't you have a corresponding pm_runtime_put() in ->probe(),
> allowing it to be runtime suspended, until the device is really needed
> later on. No?

This driver doesn't support active power management. I don't have Tegra
hardware that uses NAND storage for testing, so it's up to somebody else
to implement dynamic power management. NAND doesn't require high
voltages, so it's fine to keep the old driver behaviour by keeping
hardware resumed since the probe time.
Ulf Hansson Oct. 19, 2021, 11:40 a.m. UTC | #5
On Sun, 17 Oct 2021 at 10:38, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 01.10.2021 18:01, Ulf Hansson пишет:
> > On Fri, 1 Oct 2021 at 16:35, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> 01.10.2021 17:24, Ulf Hansson пишет:
> >>> On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>>>
> >>>> The NAND on Tegra belongs to the core power domain and we're going to
> >>>> enable GENPD support for the core domain. Now NAND must be resumed using
> >>>> runtime PM API in order to initialize the NAND power state. Add runtime PM
> >>>> and OPP support to the NAND driver.
> >>>>
> >>>> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>>> ---
> >>>>  drivers/mtd/nand/raw/tegra_nand.c | 55 ++++++++++++++++++++++++++-----
> >>>>  1 file changed, 47 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
> >>>> index 32431bbe69b8..098fcc9cb9df 100644
> >>>> --- a/drivers/mtd/nand/raw/tegra_nand.c
> >>>> +++ b/drivers/mtd/nand/raw/tegra_nand.c
> >>>> @@ -17,8 +17,11 @@
> >>>>  #include <linux/mtd/rawnand.h>
> >>>>  #include <linux/of.h>
> >>>>  #include <linux/platform_device.h>
> >>>> +#include <linux/pm_runtime.h>
> >>>>  #include <linux/reset.h>
> >>>>
> >>>> +#include <soc/tegra/common.h>
> >>>> +
> >>>>  #define COMMAND                                        0x00
> >>>>  #define   COMMAND_GO                           BIT(31)
> >>>>  #define   COMMAND_CLE                          BIT(30)
> >>>> @@ -1151,6 +1154,7 @@ static int tegra_nand_probe(struct platform_device *pdev)
> >>>>                 return -ENOMEM;
> >>>>
> >>>>         ctrl->dev = &pdev->dev;
> >>>> +       platform_set_drvdata(pdev, ctrl);
> >>>>         nand_controller_init(&ctrl->controller);
> >>>>         ctrl->controller.ops = &tegra_nand_controller_ops;
> >>>>
> >>>> @@ -1166,14 +1170,22 @@ static int tegra_nand_probe(struct platform_device *pdev)
> >>>>         if (IS_ERR(ctrl->clk))
> >>>>                 return PTR_ERR(ctrl->clk);
> >>>>
> >>>> -       err = clk_prepare_enable(ctrl->clk);
> >>>> +       err = devm_pm_runtime_enable(&pdev->dev);
> >>>> +       if (err)
> >>>> +               return err;
> >>>> +
> >>>> +       err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
> >>>> +       if (err)
> >>>> +               return err;
> >>>> +
> >>>> +       err = pm_runtime_resume_and_get(&pdev->dev);
> >>>>         if (err)
> >>>>                 return err;
> >>>>
> >>>>         err = reset_control_reset(rst);
> >>>>         if (err) {
> >>>>                 dev_err(ctrl->dev, "Failed to reset HW: %d\n", err);
> >>>> -               goto err_disable_clk;
> >>>> +               goto err_put_pm;
> >>>>         }
> >>>>
> >>>>         writel_relaxed(HWSTATUS_CMD_DEFAULT, ctrl->regs + HWSTATUS_CMD);
> >>>> @@ -1188,21 +1200,19 @@ static int tegra_nand_probe(struct platform_device *pdev)
> >>>>                                dev_name(&pdev->dev), ctrl);
> >>>>         if (err) {
> >>>>                 dev_err(ctrl->dev, "Failed to get IRQ: %d\n", err);
> >>>> -               goto err_disable_clk;
> >>>> +               goto err_put_pm;
> >>>>         }
> >>>>
> >>>>         writel_relaxed(DMA_MST_CTRL_IS_DONE, ctrl->regs + DMA_MST_CTRL);
> >>>>
> >>>>         err = tegra_nand_chips_init(ctrl->dev, ctrl);
> >>>>         if (err)
> >>>> -               goto err_disable_clk;
> >>>> -
> >>>> -       platform_set_drvdata(pdev, ctrl);
> >>>> +               goto err_put_pm;
> >>>>
> >>>
> >>> There is no corresponding call pm_runtime_put() here. Is it
> >>> intentional to always leave the device runtime resumed after ->probe()
> >>> has succeeded?
> >>>
> >>> I noticed you included some comments about this for some other
> >>> drivers, as those needed more tweaks. Is that also the case for this
> >>> driver?
> >>
> >> Could you please clarify? There is pm_runtime_put() in both probe-error
> >> and remove() code paths here.
> >
> > I was not considering the error path of ->probe() (or ->remove()), but
> > was rather thinking about when ->probe() completes successfully. Then
> > you keep the device runtime resumed, because you have called
> > pm_runtime_resume_and_get() for it.
> >
> > Shouldn't you have a corresponding pm_runtime_put() in ->probe(),
> > allowing it to be runtime suspended, until the device is really needed
> > later on. No?
>
> This driver doesn't support active power management. I don't have Tegra
> hardware that uses NAND storage for testing, so it's up to somebody else
> to implement dynamic power management. NAND doesn't require high
> voltages, so it's fine to keep the old driver behaviour by keeping
> hardware resumed since the probe time.

Alright, fair enough and thanks for clarifying!

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
index 32431bbe69b8..098fcc9cb9df 100644
--- a/drivers/mtd/nand/raw/tegra_nand.c
+++ b/drivers/mtd/nand/raw/tegra_nand.c
@@ -17,8 +17,11 @@ 
 #include <linux/mtd/rawnand.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/reset.h>
 
+#include <soc/tegra/common.h>
+
 #define COMMAND					0x00
 #define   COMMAND_GO				BIT(31)
 #define   COMMAND_CLE				BIT(30)
@@ -1151,6 +1154,7 @@  static int tegra_nand_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ctrl->dev = &pdev->dev;
+	platform_set_drvdata(pdev, ctrl);
 	nand_controller_init(&ctrl->controller);
 	ctrl->controller.ops = &tegra_nand_controller_ops;
 
@@ -1166,14 +1170,22 @@  static int tegra_nand_probe(struct platform_device *pdev)
 	if (IS_ERR(ctrl->clk))
 		return PTR_ERR(ctrl->clk);
 
-	err = clk_prepare_enable(ctrl->clk);
+	err = devm_pm_runtime_enable(&pdev->dev);
+	if (err)
+		return err;
+
+	err = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
+	if (err)
+		return err;
+
+	err = pm_runtime_resume_and_get(&pdev->dev);
 	if (err)
 		return err;
 
 	err = reset_control_reset(rst);
 	if (err) {
 		dev_err(ctrl->dev, "Failed to reset HW: %d\n", err);
-		goto err_disable_clk;
+		goto err_put_pm;
 	}
 
 	writel_relaxed(HWSTATUS_CMD_DEFAULT, ctrl->regs + HWSTATUS_CMD);
@@ -1188,21 +1200,19 @@  static int tegra_nand_probe(struct platform_device *pdev)
 			       dev_name(&pdev->dev), ctrl);
 	if (err) {
 		dev_err(ctrl->dev, "Failed to get IRQ: %d\n", err);
-		goto err_disable_clk;
+		goto err_put_pm;
 	}
 
 	writel_relaxed(DMA_MST_CTRL_IS_DONE, ctrl->regs + DMA_MST_CTRL);
 
 	err = tegra_nand_chips_init(ctrl->dev, ctrl);
 	if (err)
-		goto err_disable_clk;
-
-	platform_set_drvdata(pdev, ctrl);
+		goto err_put_pm;
 
 	return 0;
 
-err_disable_clk:
-	clk_disable_unprepare(ctrl->clk);
+err_put_pm:
+	pm_runtime_put(ctrl->dev);
 	return err;
 }
 
@@ -1219,11 +1229,39 @@  static int tegra_nand_remove(struct platform_device *pdev)
 
 	nand_cleanup(chip);
 
+	pm_runtime_put(ctrl->dev);
+
+	return 0;
+}
+
+static int __maybe_unused tegra_nand_runtime_resume(struct device *dev)
+{
+	struct tegra_nand_controller *ctrl = dev_get_drvdata(dev);
+	int err;
+
+	err = clk_prepare_enable(ctrl->clk);
+	if (err) {
+		dev_err(dev, "Failed to enable clock: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int __maybe_unused tegra_nand_runtime_suspend(struct device *dev)
+{
+	struct tegra_nand_controller *ctrl = dev_get_drvdata(dev);
+
 	clk_disable_unprepare(ctrl->clk);
 
 	return 0;
 }
 
+static const struct dev_pm_ops tegra_nand_pm = {
+	SET_RUNTIME_PM_OPS(tegra_nand_runtime_suspend, tegra_nand_runtime_resume,
+			   NULL)
+};
+
 static const struct of_device_id tegra_nand_of_match[] = {
 	{ .compatible = "nvidia,tegra20-nand" },
 	{ /* sentinel */ }
@@ -1234,6 +1272,7 @@  static struct platform_driver tegra_nand_driver = {
 	.driver = {
 		.name = "tegra-nand",
 		.of_match_table = tegra_nand_of_match,
+		.pm = &tegra_nand_pm,
 	},
 	.probe = tegra_nand_probe,
 	.remove = tegra_nand_remove,