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