[PATCH v2 00/14] NVIDIA Tegra memory and power management changes for 5.14

[PATCH v2 00/14] NVIDIA Tegra memory and power management changes for 5.14

[Dmitry Osipenko]

This series:

  1. Adds CPU/core voltage bump before system is rebooted.
  2. Adds new devm_tegra_core_dev_init_opp_table() helper and switches
     Tegra memory drivers to use it.
  3. Adds compile-testing support to the Tegra memory drivers.
  4. Adds Tegra SoC core power domain support.

Changelog:

v2: - Added more clk stubs that should fix build error reported by the
      kernel bot to v1 for the T210 memory driver.

    - Added r-b from Krzysztof Kozlowski to the memory patches.

    - Added back voltage restore on detaching of coupled regulators to
      the T20 regulator coupler which previously got missed by accident.

    - Added new patch:

        regulator: core: Detach coupled regulator before coupling count is dropped

      It fixes skipped voltage balancing on detaching of regulators which I
      happened to notice due to the recent regression of the MAX77620 driver
      that made driver to re-probe and detach coupled regulators.

v1: - Merged previous patches into this single series.

    - Added ack from Rob Herring to the core domain DT binding patch.

    - Implemented suggestions that were given by Krzysztof Kozlowski:

        - Factored out soc_is_tegra() stub into standalone patch.
        - Updated tags of the "Fix compilation warnings on 64bit platforms"
          patch, added reported-by from lkp robot and removed suggested-by
          from Nathan Chancellor.
        - Switched to use use BIT() macro.

    - Added r-b from Krzysztof Kozlowski to "Enable compile testing for
      all drivers" patch.

    - Added r-b from Nathan Chancellor.

    - Dropped voltage floor/ceiling from devm_tegra_core_dev_init_opp_table()
      since only memory drivers now need to initialize voltage vote and they
      don't need floor/ceil. This was suggested by Viresh Kumar.

Dmitry Osipenko (14):
  regulator: core: Add regulator_sync_voltage_rdev()
  regulator: core: Detach coupled regulator before coupling count is
    dropped
  soc/tegra: regulators: Bump voltages on system reboot
  soc/tegra: Add stub for soc_is_tegra()
  soc/tegra: Add devm_tegra_core_dev_init_opp_table()
  soc/tegra: fuse: Add stubs needed for compile-testing
  clk: tegra: Add stubs needed for compile-testing
  memory: tegra: Fix compilation warnings on 64bit platforms
  memory: tegra: Enable compile testing for all drivers
  memory: tegra20-emc: Use devm_tegra_core_dev_init_opp_table()
  memory: tegra30-emc: Use devm_tegra_core_dev_init_opp_table()
  dt-bindings: soc: tegra-pmc: Document core power domain
  soc/tegra: pmc: Add core power domain
  soc/tegra: regulators: Support core domain state syncing

 .../arm/tegra/nvidia,tegra20-pmc.yaml         |  35 +++++
 drivers/memory/tegra/Kconfig                  |  16 +-
 drivers/memory/tegra/tegra124-emc.c           |   4 +-
 drivers/memory/tegra/tegra20-emc.c            |  48 +-----
 drivers/memory/tegra/tegra30-emc.c            |  52 +------
 drivers/regulator/core.c                      |  37 ++++-
 drivers/soc/tegra/Kconfig                     |  14 ++
 drivers/soc/tegra/common.c                    |  97 ++++++++++++
 drivers/soc/tegra/pmc.c                       | 143 ++++++++++++++++++
 drivers/soc/tegra/regulators-tegra20.c        |  97 +++++++++++-
 drivers/soc/tegra/regulators-tegra30.c        |  96 +++++++++++-
 include/linux/clk/tegra.h                     |  82 +++++++---
 include/linux/regulator/driver.h              |   1 +
 include/soc/tegra/common.h                    |  38 +++++
 include/soc/tegra/fuse.h                      |  20 ++-
 15 files changed, 650 insertions(+), 130 deletions(-)

-- 
2.30.2

[PATCH v2 01/14] regulator: core: Add regulator_sync_voltage_rdev()

[Dmitry Osipenko]

Some NVIDIA Tegra devices use a CPU soft-reset method for the reboot and
in this case we need to restore the coupled voltages to the state that is
suitable for hardware during boot. Add new regulator_sync_voltage_rdev()
helper which is needed by regulator drivers in order to sync voltage of
a coupled regulators.

Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/regulator/core.c         | 23 +++++++++++++++++++++++
 include/linux/regulator/driver.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e20e77e4c159..aae978c0c148 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4111,6 +4111,29 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage_time_sel);
 
+int regulator_sync_voltage_rdev(struct regulator_dev *rdev)
+{
+	int ret;
+
+	regulator_lock(rdev);
+
+	if (!rdev->desc->ops->set_voltage &&
+	    !rdev->desc->ops->set_voltage_sel) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* balance only, if regulator is coupled */
+	if (rdev->coupling_desc.n_coupled > 1)
+		ret = regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+	else
+		ret = -EOPNOTSUPP;
+
+out:
+	regulator_unlock(rdev);
+	return ret;
+}
+
 /**
  * regulator_sync_voltage - re-apply last regulator output voltage
  * @regulator: regulator source
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 4ea520c248e9..35e5a611db81 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -540,6 +540,7 @@ int regulator_set_current_limit_regmap(struct regulator_dev *rdev,
 int regulator_get_current_limit_regmap(struct regulator_dev *rdev);
 void *regulator_get_init_drvdata(struct regulator_init_data *reg_init_data);
 int regulator_set_ramp_delay_regmap(struct regulator_dev *rdev, int ramp_delay);
+int regulator_sync_voltage_rdev(struct regulator_dev *rdev);
 
 /*
  * Helper functions intended to be used by regulator drivers prior registering
-- 
2.30.2

[PATCH v2 02/14] regulator: core: Detach coupled regulator before coupling count is dropped

[Dmitry Osipenko]

Detach coupled regulator before dropping coupling count in order to allow
detaching callback to balance voltage of regulators. This is needed by
NVIDIA Tegra regulator couplers in order to bring back voltage to a value
that is safe for reboot once regulators are decoupled.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/regulator/core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index aae978c0c148..83571f83af04 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5084,6 +5084,13 @@ static void regulator_remove_coupling(struct regulator_dev *rdev)
 
 	n_coupled = c_desc->n_coupled;
 
+	if (coupler && coupler->detach_regulator) {
+		err = coupler->detach_regulator(coupler, rdev);
+		if (err)
+			rdev_err(rdev, "failed to detach from coupler: %pe\n",
+				 ERR_PTR(err));
+	}
+
 	for (i = 1; i < n_coupled; i++) {
 		c_rdev = c_desc->coupled_rdevs[i];
 
@@ -5111,13 +5118,6 @@ static void regulator_remove_coupling(struct regulator_dev *rdev)
 		c_desc->n_resolved--;
 	}
 
-	if (coupler && coupler->detach_regulator) {
-		err = coupler->detach_regulator(coupler, rdev);
-		if (err)
-			rdev_err(rdev, "failed to detach from coupler: %pe\n",
-				 ERR_PTR(err));
-	}
-
 	kfree(rdev->coupling_desc.coupled_rdevs);
 	rdev->coupling_desc.coupled_rdevs = NULL;
 }
-- 
2.30.2

Re: [PATCH v2 02/14] regulator: core: Detach coupled regulator before coupling count is dropped

[Dmitry Osipenko]

24.05.2021 02:13, Dmitry Osipenko пишет:
> Detach coupled regulator before dropping coupling count in order to allow
> detaching callback to balance voltage of regulators. This is needed by
> NVIDIA Tegra regulator couplers in order to bring back voltage to a value
> that is safe for reboot once regulators are decoupled.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/regulator/core.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index aae978c0c148..83571f83af04 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5084,6 +5084,13 @@ static void regulator_remove_coupling(struct regulator_dev *rdev)
>  
>  	n_coupled = c_desc->n_coupled;
>  
> +	if (coupler && coupler->detach_regulator) {
> +		err = coupler->detach_regulator(coupler, rdev);
> +		if (err)
> +			rdev_err(rdev, "failed to detach from coupler: %pe\n",
> +				 ERR_PTR(err));
> +	}
> +
>  	for (i = 1; i < n_coupled; i++) {
>  		c_rdev = c_desc->coupled_rdevs[i];
>  
> @@ -5111,13 +5118,6 @@ static void regulator_remove_coupling(struct regulator_dev *rdev)
>  		c_desc->n_resolved--;
>  	}
>  
> -	if (coupler && coupler->detach_regulator) {
> -		err = coupler->detach_regulator(coupler, rdev);
> -		if (err)
> -			rdev_err(rdev, "failed to detach from coupler: %pe\n",
> -				 ERR_PTR(err));
> -	}
> -
>  	kfree(rdev->coupling_desc.coupled_rdevs);
>  	rdev->coupling_desc.coupled_rdevs = NULL;
>  }
> 

I now realized that this is a bit too fragile approach. I'll drop this
patch in v3, there are better options of how to manage balancing on
detaching and this is not critical feature for now anyways.

[PATCH v2 03/14] soc/tegra: regulators: Bump voltages on system reboot

[Dmitry Osipenko]

Ensure that SoC voltages are at a level suitable for a system reboot.
This is important for some devices that use CPU reset method for the
rebooting. SoC CPU and core voltages now are be restored to a level
that is suitable for rebooting. This patch fixes hang on reboot on
Asus Transformer TF101, it was also reported as fixing some of reboot
issues on Toshiba AC100.

Reported-by: Nikola Milosavljević <mnidza@outlook.com>
Tested-by: Nikola Milosavljević <mnidza@outlook.com> # TF101
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/regulators-tegra20.c | 78 +++++++++++++++++++++++++-
 drivers/soc/tegra/regulators-tegra30.c | 78 +++++++++++++++++++++++++-
 2 files changed, 154 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/tegra/regulators-tegra20.c b/drivers/soc/tegra/regulators-tegra20.c
index 367a71a3cd10..35335c6a20b8 100644
--- a/drivers/soc/tegra/regulators-tegra20.c
+++ b/drivers/soc/tegra/regulators-tegra20.c
@@ -12,6 +12,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/reboot.h>
 #include <linux/regulator/coupler.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
@@ -21,7 +22,10 @@ struct tegra_regulator_coupler {
 	struct regulator_dev *core_rdev;
 	struct regulator_dev *cpu_rdev;
 	struct regulator_dev *rtc_rdev;
-	int core_min_uV;
+	struct notifier_block reboot_notifier;
+	int core_min_uV, cpu_min_uV;
+	bool sys_reboot_mode_req;
+	bool sys_reboot_mode;
 };
 
 static inline struct tegra_regulator_coupler *
@@ -242,6 +246,10 @@ static int tegra20_cpu_voltage_update(struct tegra_regulator_coupler *tegra,
 	if (cpu_uV < 0)
 		return cpu_uV;
 
+	/* store boot voltage level */
+	if (!tegra->cpu_min_uV)
+		tegra->cpu_min_uV = cpu_uV;
+
 	/*
 	 * CPU's regulator may not have any consumers, hence the voltage
 	 * must not be changed in that case because CPU simply won't
@@ -250,6 +258,10 @@ static int tegra20_cpu_voltage_update(struct tegra_regulator_coupler *tegra,
 	if (!cpu_min_uV_consumers)
 		cpu_min_uV = cpu_uV;
 
+	/* restore boot voltage level */
+	if (tegra->sys_reboot_mode)
+		cpu_min_uV = max(cpu_min_uV, tegra->cpu_min_uV);
+
 	if (cpu_min_uV > cpu_uV) {
 		err = tegra20_core_rtc_update(tegra, core_rdev, rtc_rdev,
 					      cpu_uV, cpu_min_uV);
@@ -290,6 +302,8 @@ static int tegra20_regulator_balance_voltage(struct regulator_coupler *coupler,
 		return -EINVAL;
 	}
 
+	tegra->sys_reboot_mode = READ_ONCE(tegra->sys_reboot_mode_req);
+
 	if (rdev == cpu_rdev)
 		return tegra20_cpu_voltage_update(tegra, cpu_rdev,
 						  core_rdev, rtc_rdev);
@@ -303,6 +317,51 @@ static int tegra20_regulator_balance_voltage(struct regulator_coupler *coupler,
 	return -EPERM;
 }
 
+static int tegra20_regulator_prepare_reboot(struct tegra_regulator_coupler *tegra,
+					    bool sys_reboot_mode)
+{
+	int err;
+
+	if (!tegra->core_rdev || !tegra->rtc_rdev || !tegra->cpu_rdev)
+		return 0;
+
+	WRITE_ONCE(tegra->sys_reboot_mode_req, true);
+
+	/*
+	 * Some devices use CPU soft-reboot method and in this case we
+	 * should ensure that voltages are sane for the reboot by restoring
+	 * the minimum boot levels.
+	 */
+	err = regulator_sync_voltage_rdev(tegra->cpu_rdev);
+	if (err)
+		return err;
+
+	err = regulator_sync_voltage_rdev(tegra->core_rdev);
+	if (err)
+		return err;
+
+	WRITE_ONCE(tegra->sys_reboot_mode_req, sys_reboot_mode);
+
+	return 0;
+}
+
+static int tegra20_regulator_reboot(struct notifier_block *notifier,
+				    unsigned long event, void *cmd)
+{
+	struct tegra_regulator_coupler *tegra;
+	int ret;
+
+	if (event != SYS_RESTART)
+		return NOTIFY_DONE;
+
+	tegra = container_of(notifier, struct tegra_regulator_coupler,
+			     reboot_notifier);
+
+	ret = tegra20_regulator_prepare_reboot(tegra, true);
+
+	return notifier_from_errno(ret);
+}
+
 static int tegra20_regulator_attach(struct regulator_coupler *coupler,
 				    struct regulator_dev *rdev)
 {
@@ -335,6 +394,17 @@ static int tegra20_regulator_detach(struct regulator_coupler *coupler,
 {
 	struct tegra_regulator_coupler *tegra = to_tegra_coupler(coupler);
 
+	/*
+	 * We don't expect regulators to be decoupled during reboot,
+	 * this may race with the reboot handler and shouldn't ever
+	 * happen in practice.
+	 */
+	if (WARN_ON_ONCE(system_state > SYSTEM_RUNNING))
+		return -EPERM;
+
+	/* bring regulators to the state that is safe for reboot */
+	tegra20_regulator_prepare_reboot(tegra, false);
+
 	if (tegra->core_rdev == rdev) {
 		tegra->core_rdev = NULL;
 		return 0;
@@ -359,13 +429,19 @@ static struct tegra_regulator_coupler tegra20_coupler = {
 		.detach_regulator = tegra20_regulator_detach,
 		.balance_voltage = tegra20_regulator_balance_voltage,
 	},
+	.reboot_notifier.notifier_call = tegra20_regulator_reboot,
 };
 
 static int __init tegra_regulator_coupler_init(void)
 {
+	int err;
+
 	if (!of_machine_is_compatible("nvidia,tegra20"))
 		return 0;
 
+	err = register_reboot_notifier(&tegra20_coupler.reboot_notifier);
+	WARN_ON(err);
+
 	return regulator_coupler_register(&tegra20_coupler.coupler);
 }
 arch_initcall(tegra_regulator_coupler_init);
diff --git a/drivers/soc/tegra/regulators-tegra30.c b/drivers/soc/tegra/regulators-tegra30.c
index 0e776b20f625..6e4f3d9e7be1 100644
--- a/drivers/soc/tegra/regulators-tegra30.c
+++ b/drivers/soc/tegra/regulators-tegra30.c
@@ -12,6 +12,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/reboot.h>
 #include <linux/regulator/coupler.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
@@ -22,7 +23,10 @@ struct tegra_regulator_coupler {
 	struct regulator_coupler coupler;
 	struct regulator_dev *core_rdev;
 	struct regulator_dev *cpu_rdev;
-	int core_min_uV;
+	struct notifier_block reboot_notifier;
+	int core_min_uV, cpu_min_uV;
+	bool sys_reboot_mode_req;
+	bool sys_reboot_mode;
 };
 
 static inline struct tegra_regulator_coupler *
@@ -172,6 +176,10 @@ static int tegra30_voltage_update(struct tegra_regulator_coupler *tegra,
 	if (cpu_uV < 0)
 		return cpu_uV;
 
+	/* store boot voltage level */
+	if (!tegra->cpu_min_uV)
+		tegra->cpu_min_uV = cpu_uV;
+
 	/*
 	 * CPU's regulator may not have any consumers, hence the voltage
 	 * must not be changed in that case because CPU simply won't
@@ -195,6 +203,10 @@ static int tegra30_voltage_update(struct tegra_regulator_coupler *tegra,
 	if (err)
 		return err;
 
+	/* restore boot voltage level */
+	if (tegra->sys_reboot_mode)
+		cpu_min_uV = max(cpu_min_uV, tegra->cpu_min_uV);
+
 	if (core_min_limited_uV > core_uV) {
 		pr_err("core voltage constraint violated: %d %d %d\n",
 		       core_uV, core_min_limited_uV, cpu_uV);
@@ -263,9 +275,56 @@ static int tegra30_regulator_balance_voltage(struct regulator_coupler *coupler,
 		return -EINVAL;
 	}
 
+	tegra->sys_reboot_mode = READ_ONCE(tegra->sys_reboot_mode_req);
+
 	return tegra30_voltage_update(tegra, cpu_rdev, core_rdev);
 }
 
+static int tegra30_regulator_prepare_reboot(struct tegra_regulator_coupler *tegra,
+					    bool sys_reboot_mode)
+{
+	int err;
+
+	if (!tegra->core_rdev || !tegra->cpu_rdev)
+		return 0;
+
+	WRITE_ONCE(tegra->sys_reboot_mode_req, true);
+
+	/*
+	 * Some devices use CPU soft-reboot method and in this case we
+	 * should ensure that voltages are sane for the reboot by restoring
+	 * the minimum boot levels.
+	 */
+	err = regulator_sync_voltage_rdev(tegra->cpu_rdev);
+	if (err)
+		return err;
+
+	err = regulator_sync_voltage_rdev(tegra->core_rdev);
+	if (err)
+		return err;
+
+	WRITE_ONCE(tegra->sys_reboot_mode_req, sys_reboot_mode);
+
+	return 0;
+}
+
+static int tegra30_regulator_reboot(struct notifier_block *notifier,
+				    unsigned long event, void *cmd)
+{
+	struct tegra_regulator_coupler *tegra;
+	int ret;
+
+	if (event != SYS_RESTART)
+		return NOTIFY_DONE;
+
+	tegra = container_of(notifier, struct tegra_regulator_coupler,
+			     reboot_notifier);
+
+	ret = tegra30_regulator_prepare_reboot(tegra, true);
+
+	return notifier_from_errno(ret);
+}
+
 static int tegra30_regulator_attach(struct regulator_coupler *coupler,
 				    struct regulator_dev *rdev)
 {
@@ -292,6 +351,17 @@ static int tegra30_regulator_detach(struct regulator_coupler *coupler,
 {
 	struct tegra_regulator_coupler *tegra = to_tegra_coupler(coupler);
 
+	/*
+	 * We don't expect regulators to be decoupled during reboot,
+	 * this may race with the reboot handler and shouldn't ever
+	 * happen in practice.
+	 */
+	if (WARN_ON_ONCE(system_state > SYSTEM_RUNNING))
+		return -EPERM;
+
+	/* bring regulators to the state that is safe for reboot */
+	tegra30_regulator_prepare_reboot(tegra, false);
+
 	if (tegra->core_rdev == rdev) {
 		tegra->core_rdev = NULL;
 		return 0;
@@ -311,13 +381,19 @@ static struct tegra_regulator_coupler tegra30_coupler = {
 		.detach_regulator = tegra30_regulator_detach,
 		.balance_voltage = tegra30_regulator_balance_voltage,
 	},
+	.reboot_notifier.notifier_call = tegra30_regulator_reboot,
 };
 
 static int __init tegra_regulator_coupler_init(void)
 {
+	int err;
+
 	if (!of_machine_is_compatible("nvidia,tegra30"))
 		return 0;
 
+	err = register_reboot_notifier(&tegra30_coupler.reboot_notifier);
+	WARN_ON(err);
+
 	return regulator_coupler_register(&tegra30_coupler.coupler);
 }
 arch_initcall(tegra_regulator_coupler_init);
-- 
2.30.2

[PATCH v2 04/14] soc/tegra: Add stub for soc_is_tegra()

[Dmitry Osipenko]

Add stub required for compile-testing of drivers.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 include/soc/tegra/common.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h
index 98027a76ce3d..744280ecab5f 100644
--- a/include/soc/tegra/common.h
+++ b/include/soc/tegra/common.h
@@ -6,6 +6,15 @@
 #ifndef __SOC_TEGRA_COMMON_H__
 #define __SOC_TEGRA_COMMON_H__
 
+#include <linux/types.h>
+
+#ifdef CONFIG_ARCH_TEGRA
 bool soc_is_tegra(void);
+#else
+static inline bool soc_is_tegra(void)
+{
+	return false;
+}
+#endif
 
 #endif /* __SOC_TEGRA_COMMON_H__ */
-- 
2.30.2

[PATCH v2 05/14] soc/tegra: Add devm_tegra_core_dev_init_opp_table()

[Dmitry Osipenko]

Add common helper which initializes OPP table for Tegra SoC core devices.

Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/common.c | 97 ++++++++++++++++++++++++++++++++++++++
 include/soc/tegra/common.h | 22 +++++++++
 2 files changed, 119 insertions(+)

diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
index 3dc54f59cafe..cd33e99249c3 100644
--- a/drivers/soc/tegra/common.c
+++ b/drivers/soc/tegra/common.c
@@ -3,9 +3,16 @@
  * Copyright (C) 2014 NVIDIA CORPORATION.  All rights reserved.
  */
 
+#define dev_fmt(fmt)	"tegra-soc: " fmt
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/export.h>
 #include <linux/of.h>
+#include <linux/pm_opp.h>
 
 #include <soc/tegra/common.h>
+#include <soc/tegra/fuse.h>
 
 static const struct of_device_id tegra_machine_match[] = {
 	{ .compatible = "nvidia,tegra20", },
@@ -31,3 +38,93 @@ bool soc_is_tegra(void)
 
 	return match != NULL;
 }
+
+static int tegra_core_dev_init_opp_state(struct device *dev)
+{
+	unsigned long rate;
+	struct clk *clk;
+	int err;
+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(dev, "failed to get clk: %pe\n", clk);
+		return PTR_ERR(clk);
+	}
+
+	rate = clk_get_rate(clk);
+	if (!rate) {
+		dev_err(dev, "failed to get clk rate\n");
+		return -EINVAL;
+	}
+
+	/* first dummy rate-setting initializes voltage vote */
+	err = dev_pm_opp_set_rate(dev, rate);
+	if (err) {
+		dev_err(dev, "failed to initialize OPP clock: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+/**
+ * devm_tegra_core_dev_init_opp_table() - initialize OPP table
+ * @dev: device for which OPP table is initialized
+ * @params: pointer to the OPP table configuration
+ *
+ * This function will initialize OPP table and sync OPP state of a Tegra SoC
+ * core device.
+ *
+ * Return: 0 on success or errorno.
+ */
+int devm_tegra_core_dev_init_opp_table(struct device *dev,
+				       struct tegra_core_opp_params *params)
+{
+	u32 hw_version;
+	int err;
+
+	err = devm_pm_opp_set_clkname(dev, NULL);
+	if (err) {
+		dev_err(dev, "failed to set OPP clk: %d\n", err);
+		return err;
+	}
+
+	/* Tegra114+ doesn't support OPP yet */
+	if (!of_machine_is_compatible("nvidia,tegra20") &&
+	    !of_machine_is_compatible("nvidia,tegra30"))
+		return -ENODEV;
+
+	if (of_machine_is_compatible("nvidia,tegra20"))
+		hw_version = BIT(tegra_sku_info.soc_process_id);
+	else
+		hw_version = BIT(tegra_sku_info.soc_speedo_id);
+
+	err = devm_pm_opp_set_supported_hw(dev, &hw_version, 1);
+	if (err) {
+		dev_err(dev, "failed to set OPP supported HW: %d\n", err);
+		return err;
+	}
+
+	/*
+	 * Older device-trees have an empty OPP table, we will get
+	 * -ENODEV from devm_pm_opp_of_add_table() in this case.
+	 */
+	err = devm_pm_opp_of_add_table(dev);
+	if (err) {
+		if (err == -ENODEV)
+			dev_err_once(dev, "OPP table not found, please update device-tree\n");
+		else
+			dev_err(dev, "failed to add OPP table: %d\n", err);
+
+		return err;
+	}
+
+	if (params->init_state) {
+		err = tegra_core_dev_init_opp_state(dev);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_tegra_core_dev_init_opp_table);
diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h
index 744280ecab5f..af41ad80ec21 100644
--- a/include/soc/tegra/common.h
+++ b/include/soc/tegra/common.h
@@ -6,15 +6,37 @@
 #ifndef __SOC_TEGRA_COMMON_H__
 #define __SOC_TEGRA_COMMON_H__
 
+#include <linux/errno.h>
 #include <linux/types.h>
 
+struct device;
+
+/**
+ * Tegra SoC core device OPP table configuration
+ *
+ * @init_state: pre-initialize OPP state of a device
+ */
+struct tegra_core_opp_params {
+	bool init_state;
+};
+
 #ifdef CONFIG_ARCH_TEGRA
 bool soc_is_tegra(void);
+
+int devm_tegra_core_dev_init_opp_table(struct device *dev,
+				       struct tegra_core_opp_params *params);
 #else
 static inline bool soc_is_tegra(void)
 {
 	return false;
 }
+
+static inline int
+devm_tegra_core_dev_init_opp_table(struct device *dev,
+				   struct tegra_core_opp_params *params)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif /* __SOC_TEGRA_COMMON_H__ */
-- 
2.30.2

[PATCH v2 06/14] soc/tegra: fuse: Add stubs needed for compile-testing

[Dmitry Osipenko]

Add missing stubs that will allow Tegra memory driver to be compile-tested
by kernel build bots.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 include/soc/tegra/fuse.h | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/soc/tegra/fuse.h b/include/soc/tegra/fuse.h
index 78cbc787a4dc..990701f788bc 100644
--- a/include/soc/tegra/fuse.h
+++ b/include/soc/tegra/fuse.h
@@ -52,14 +52,28 @@ struct tegra_sku_info {
 	enum tegra_revision revision;
 };
 
+#ifdef CONFIG_ARCH_TEGRA
+extern struct tegra_sku_info tegra_sku_info;
 u32 tegra_read_straps(void);
 u32 tegra_read_ram_code(void);
 int tegra_fuse_readl(unsigned long offset, u32 *value);
-
-#ifdef CONFIG_ARCH_TEGRA
-extern struct tegra_sku_info tegra_sku_info;
 #else
 static struct tegra_sku_info tegra_sku_info __maybe_unused;
+
+static inline u32 tegra_read_straps(void)
+{
+	return 0;
+}
+
+static inline u32 tegra_read_ram_code(void)
+{
+	return 0;
+}
+
+static inline int tegra_fuse_readl(unsigned long offset, u32 *value)
+{
+	return -ENODEV;
+}
 #endif
 
 struct device *tegra_soc_device_register(void);
-- 
2.30.2

[PATCH v2 07/14] clk: tegra: Add stubs needed for compile-testing

[Dmitry Osipenko]

Add stubs needed for compile-testing of Tegra memory drivers.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 include/linux/clk/tegra.h | 82 ++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 18 deletions(-)

diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h
index f7ff722a03dd..3dab36dcb534 100644
--- a/include/linux/clk/tegra.h
+++ b/include/linux/clk/tegra.h
@@ -123,20 +123,6 @@ static inline void tegra_cpu_clock_resume(void)
 }
 #endif
 
-extern int tegra210_plle_hw_sequence_start(void);
-extern bool tegra210_plle_hw_sequence_is_enabled(void);
-extern void tegra210_xusb_pll_hw_control_enable(void);
-extern void tegra210_xusb_pll_hw_sequence_start(void);
-extern void tegra210_sata_pll_hw_control_enable(void);
-extern void tegra210_sata_pll_hw_sequence_start(void);
-extern void tegra210_set_sata_pll_seq_sw(bool state);
-extern void tegra210_put_utmipll_in_iddq(void);
-extern void tegra210_put_utmipll_out_iddq(void);
-extern int tegra210_clk_handle_mbist_war(unsigned int id);
-extern void tegra210_clk_emc_dll_enable(bool flag);
-extern void tegra210_clk_emc_dll_update_setting(u32 emc_dll_src_value);
-extern void tegra210_clk_emc_update_setting(u32 emc_src_value);
-
 struct clk;
 struct tegra_emc;
 
@@ -144,18 +130,78 @@ typedef long (tegra20_clk_emc_round_cb)(unsigned long rate,
 					unsigned long min_rate,
 					unsigned long max_rate,
 					void *arg);
+typedef int (tegra124_emc_prepare_timing_change_cb)(struct tegra_emc *emc,
+						    unsigned long rate);
+typedef void (tegra124_emc_complete_timing_change_cb)(struct tegra_emc *emc,
+						      unsigned long rate);
 
+#ifdef CONFIG_ARCH_TEGRA
 void tegra20_clk_set_emc_round_callback(tegra20_clk_emc_round_cb *round_cb,
 					void *cb_arg);
 int tegra20_clk_prepare_emc_mc_same_freq(struct clk *emc_clk, bool same);
 
-typedef int (tegra124_emc_prepare_timing_change_cb)(struct tegra_emc *emc,
-						    unsigned long rate);
-typedef void (tegra124_emc_complete_timing_change_cb)(struct tegra_emc *emc,
-						      unsigned long rate);
 void tegra124_clk_set_emc_callbacks(tegra124_emc_prepare_timing_change_cb *prep_cb,
 				    tegra124_emc_complete_timing_change_cb *complete_cb);
 
+int tegra210_plle_hw_sequence_start(void);
+bool tegra210_plle_hw_sequence_is_enabled(void);
+void tegra210_xusb_pll_hw_control_enable(void);
+void tegra210_xusb_pll_hw_sequence_start(void);
+void tegra210_sata_pll_hw_control_enable(void);
+void tegra210_sata_pll_hw_sequence_start(void);
+void tegra210_set_sata_pll_seq_sw(bool state);
+void tegra210_put_utmipll_in_iddq(void);
+void tegra210_put_utmipll_out_iddq(void);
+int tegra210_clk_handle_mbist_war(unsigned int id);
+void tegra210_clk_emc_dll_enable(bool flag);
+void tegra210_clk_emc_dll_update_setting(u32 emc_dll_src_value);
+void tegra210_clk_emc_update_setting(u32 emc_src_value);
+#else
+static inline void
+tegra20_clk_set_emc_round_callback(tegra20_clk_emc_round_cb *round_cb,
+				   void *cb_arg)
+{
+}
+
+static inline int
+tegra20_clk_prepare_emc_mc_same_freq(struct clk *emc_clk, bool same)
+{
+	return 0;
+}
+
+static inline void
+tegra124_clk_set_emc_callbacks(tegra124_emc_prepare_timing_change_cb *prep_cb,
+			       tegra124_emc_complete_timing_change_cb *complete_cb)
+{
+}
+
+static inline int tegra210_plle_hw_sequence_start(void)
+{
+	return 0;
+}
+
+static inline bool tegra210_plle_hw_sequence_is_enabled(void)
+{
+	return false;
+}
+
+static inline int tegra210_clk_handle_mbist_war(unsigned int id)
+{
+	return 0;
+}
+
+static inline void tegra210_xusb_pll_hw_control_enable(void) {}
+static inline void tegra210_xusb_pll_hw_sequence_start(void) {}
+static inline void tegra210_sata_pll_hw_control_enable(void) {}
+static inline void tegra210_sata_pll_hw_sequence_start(void) {}
+static inline void tegra210_set_sata_pll_seq_sw(bool state) {}
+static inline void tegra210_put_utmipll_in_iddq(void) {}
+static inline void tegra210_put_utmipll_out_iddq(void) {}
+static inline void tegra210_clk_emc_dll_enable(bool flag) {}
+static inline void tegra210_clk_emc_dll_update_setting(u32 emc_dll_src_value) {}
+static inline void tegra210_clk_emc_update_setting(u32 emc_src_value) {}
+#endif
+
 struct tegra210_clk_emc_config {
 	unsigned long rate;
 	bool same_freq;
-- 
2.30.2

[PATCH v2 08/14] memory: tegra: Fix compilation warnings on 64bit platforms

[Dmitry Osipenko]

Fix compilation warning on 64bit platforms caused by implicit promotion
of 32bit signed integer to a 64bit unsigned value which happens after
enabling compile-testing of the EMC drivers.

Reported-by: kernel test robot <lkp@intel.com>
Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/memory/tegra/tegra124-emc.c | 4 ++--
 drivers/memory/tegra/tegra30-emc.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
index 5699d909abc2..a21ca8e0841a 100644
--- a/drivers/memory/tegra/tegra124-emc.c
+++ b/drivers/memory/tegra/tegra124-emc.c
@@ -272,8 +272,8 @@
 #define EMC_PUTERM_ADJ				0x574
 
 #define DRAM_DEV_SEL_ALL			0
-#define DRAM_DEV_SEL_0				(2 << 30)
-#define DRAM_DEV_SEL_1				(1 << 30)
+#define DRAM_DEV_SEL_0				BIT(31)
+#define DRAM_DEV_SEL_1				BIT(30)
 
 #define EMC_CFG_POWER_FEATURES_MASK		\
 	(EMC_CFG_DYN_SREF | EMC_CFG_DRAM_ACPD | EMC_CFG_DRAM_CLKSTOP_SR | \
diff --git a/drivers/memory/tegra/tegra30-emc.c b/drivers/memory/tegra/tegra30-emc.c
index 829f6d673c96..a2f2738ccb94 100644
--- a/drivers/memory/tegra/tegra30-emc.c
+++ b/drivers/memory/tegra/tegra30-emc.c
@@ -150,8 +150,8 @@
 #define EMC_SELF_REF_CMD_ENABLED		BIT(0)
 
 #define DRAM_DEV_SEL_ALL			(0 << 30)
-#define DRAM_DEV_SEL_0				(2 << 30)
-#define DRAM_DEV_SEL_1				(1 << 30)
+#define DRAM_DEV_SEL_0				BIT(31)
+#define DRAM_DEV_SEL_1				BIT(30)
 #define DRAM_BROADCAST(num) \
 	((num) > 1 ? DRAM_DEV_SEL_ALL : DRAM_DEV_SEL_0)
 
-- 
2.30.2

[PATCH v2 09/14] memory: tegra: Enable compile testing for all drivers

[Dmitry Osipenko]

Enable compile testing for all Tegra memory drivers.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/memory/tegra/Kconfig | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
index a70967a56e52..c63ffa74ab94 100644
--- a/drivers/memory/tegra/Kconfig
+++ b/drivers/memory/tegra/Kconfig
@@ -2,16 +2,18 @@
 config TEGRA_MC
 	bool "NVIDIA Tegra Memory Controller support"
 	default y
-	depends on ARCH_TEGRA
+	depends on ARCH_TEGRA || COMPILE_TEST
 	select INTERCONNECT
 	help
 	  This driver supports the Memory Controller (MC) hardware found on
 	  NVIDIA Tegra SoCs.
 
+if TEGRA_MC
+
 config TEGRA20_EMC
 	tristate "NVIDIA Tegra20 External Memory Controller driver"
 	default y
-	depends on TEGRA_MC && ARCH_TEGRA_2x_SOC
+	depends on ARCH_TEGRA_2x_SOC || COMPILE_TEST
 	select DEVFREQ_GOV_SIMPLE_ONDEMAND
 	select PM_DEVFREQ
 	help
@@ -23,7 +25,7 @@ config TEGRA20_EMC
 config TEGRA30_EMC
 	tristate "NVIDIA Tegra30 External Memory Controller driver"
 	default y
-	depends on TEGRA_MC && ARCH_TEGRA_3x_SOC
+	depends on ARCH_TEGRA_3x_SOC || COMPILE_TEST
 	select PM_OPP
 	help
 	  This driver is for the External Memory Controller (EMC) found on
@@ -34,8 +36,8 @@ config TEGRA30_EMC
 config TEGRA124_EMC
 	tristate "NVIDIA Tegra124 External Memory Controller driver"
 	default y
-	depends on TEGRA_MC && ARCH_TEGRA_124_SOC
-	select TEGRA124_CLK_EMC
+	depends on ARCH_TEGRA_124_SOC || COMPILE_TEST
+	select TEGRA124_CLK_EMC if ARCH_TEGRA
 	select PM_OPP
 	help
 	  This driver is for the External Memory Controller (EMC) found on
@@ -49,10 +51,12 @@ config TEGRA210_EMC_TABLE
 
 config TEGRA210_EMC
 	tristate "NVIDIA Tegra210 External Memory Controller driver"
-	depends on TEGRA_MC && ARCH_TEGRA_210_SOC
+	depends on ARCH_TEGRA_210_SOC || COMPILE_TEST
 	select TEGRA210_EMC_TABLE
 	help
 	  This driver is for the External Memory Controller (EMC) found on
 	  Tegra210 chips. The EMC controls the external DRAM on the board.
 	  This driver is required to change memory timings / clock rate for
 	  external memory.
+
+endif
-- 
2.30.2

[PATCH v2 10/14] memory: tegra20-emc: Use devm_tegra_core_dev_init_opp_table()

[Dmitry Osipenko]

Use common devm_tegra_core_dev_init_opp_table() helper for the OPP table
initialization.

Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/memory/tegra/tegra20-emc.c | 48 +++---------------------------
 1 file changed, 4 insertions(+), 44 deletions(-)

diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
index da8a0da8da79..a534197a5fb2 100644
--- a/drivers/memory/tegra/tegra20-emc.c
+++ b/drivers/memory/tegra/tegra20-emc.c
@@ -908,49 +908,6 @@ static int tegra_emc_interconnect_init(struct tegra_emc *emc)
 	return err;
 }
 
-static int tegra_emc_opp_table_init(struct tegra_emc *emc)
-{
-	u32 hw_version = BIT(tegra_sku_info.soc_process_id);
-	struct opp_table *hw_opp_table;
-	int err;
-
-	hw_opp_table = dev_pm_opp_set_supported_hw(emc->dev, &hw_version, 1);
-	err = PTR_ERR_OR_ZERO(hw_opp_table);
-	if (err) {
-		dev_err(emc->dev, "failed to set OPP supported HW: %d\n", err);
-		return err;
-	}
-
-	err = dev_pm_opp_of_add_table(emc->dev);
-	if (err) {
-		if (err == -ENODEV)
-			dev_err(emc->dev, "OPP table not found, please update your device tree\n");
-		else
-			dev_err(emc->dev, "failed to add OPP table: %d\n", err);
-
-		goto put_hw_table;
-	}
-
-	dev_info_once(emc->dev, "OPP HW ver. 0x%x, current clock rate %lu MHz\n",
-		      hw_version, clk_get_rate(emc->clk) / 1000000);
-
-	/* first dummy rate-set initializes voltage state */
-	err = dev_pm_opp_set_rate(emc->dev, clk_get_rate(emc->clk));
-	if (err) {
-		dev_err(emc->dev, "failed to initialize OPP clock: %d\n", err);
-		goto remove_table;
-	}
-
-	return 0;
-
-remove_table:
-	dev_pm_opp_of_remove_table(emc->dev);
-put_hw_table:
-	dev_pm_opp_put_supported_hw(hw_opp_table);
-
-	return err;
-}
-
 static void devm_tegra_emc_unset_callback(void *data)
 {
 	tegra20_clk_set_emc_round_callback(NULL, NULL);
@@ -1077,6 +1034,7 @@ static int tegra_emc_devfreq_init(struct tegra_emc *emc)
 
 static int tegra_emc_probe(struct platform_device *pdev)
 {
+	struct tegra_core_opp_params opp_params = {};
 	struct device_node *np;
 	struct tegra_emc *emc;
 	int irq, err;
@@ -1122,7 +1080,9 @@ static int tegra_emc_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	err = tegra_emc_opp_table_init(emc);
+	opp_params.init_state = true;
+
+	err = devm_tegra_core_dev_init_opp_table(&pdev->dev, &opp_params);
 	if (err)
 		return err;
 
-- 
2.30.2

[PATCH v2 11/14] memory: tegra30-emc: Use devm_tegra_core_dev_init_opp_table()

[Dmitry Osipenko]

Use common devm_tegra_core_dev_init_opp_table() helper for the OPP table
initialization.

Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/memory/tegra/tegra30-emc.c | 48 +++---------------------------
 1 file changed, 4 insertions(+), 44 deletions(-)

diff --git a/drivers/memory/tegra/tegra30-emc.c b/drivers/memory/tegra/tegra30-emc.c
index a2f2738ccb94..63e1983f8a0d 100644
--- a/drivers/memory/tegra/tegra30-emc.c
+++ b/drivers/memory/tegra/tegra30-emc.c
@@ -1480,49 +1480,6 @@ static int tegra_emc_interconnect_init(struct tegra_emc *emc)
 	return err;
 }
 
-static int tegra_emc_opp_table_init(struct tegra_emc *emc)
-{
-	u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
-	struct opp_table *hw_opp_table;
-	int err;
-
-	hw_opp_table = dev_pm_opp_set_supported_hw(emc->dev, &hw_version, 1);
-	err = PTR_ERR_OR_ZERO(hw_opp_table);
-	if (err) {
-		dev_err(emc->dev, "failed to set OPP supported HW: %d\n", err);
-		return err;
-	}
-
-	err = dev_pm_opp_of_add_table(emc->dev);
-	if (err) {
-		if (err == -ENODEV)
-			dev_err(emc->dev, "OPP table not found, please update your device tree\n");
-		else
-			dev_err(emc->dev, "failed to add OPP table: %d\n", err);
-
-		goto put_hw_table;
-	}
-
-	dev_info_once(emc->dev, "OPP HW ver. 0x%x, current clock rate %lu MHz\n",
-		      hw_version, clk_get_rate(emc->clk) / 1000000);
-
-	/* first dummy rate-set initializes voltage state */
-	err = dev_pm_opp_set_rate(emc->dev, clk_get_rate(emc->clk));
-	if (err) {
-		dev_err(emc->dev, "failed to initialize OPP clock: %d\n", err);
-		goto remove_table;
-	}
-
-	return 0;
-
-remove_table:
-	dev_pm_opp_of_remove_table(emc->dev);
-put_hw_table:
-	dev_pm_opp_put_supported_hw(hw_opp_table);
-
-	return err;
-}
-
 static void devm_tegra_emc_unset_callback(void *data)
 {
 	tegra20_clk_set_emc_round_callback(NULL, NULL);
@@ -1568,6 +1525,7 @@ static int tegra_emc_init_clk(struct tegra_emc *emc)
 
 static int tegra_emc_probe(struct platform_device *pdev)
 {
+	struct tegra_core_opp_params opp_params = {};
 	struct device_node *np;
 	struct tegra_emc *emc;
 	int err;
@@ -1617,7 +1575,9 @@ static int tegra_emc_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	err = tegra_emc_opp_table_init(emc);
+	opp_params.init_state = true;
+
+	err = devm_tegra_core_dev_init_opp_table(&pdev->dev, &opp_params);
 	if (err)
 		return err;
 
-- 
2.30.2

[PATCH v2 12/14] dt-bindings: soc: tegra-pmc: Document core power domain

[Dmitry Osipenko]

All NVIDIA Tegra SoCs have a core power domain where majority of hardware
blocks reside. Document the new core power domain properties.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 .../arm/tegra/nvidia,tegra20-pmc.yaml         | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml
index 43fd2f8927d0..0afec83cc723 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml
@@ -301,6 +301,33 @@ patternProperties:
 
     additionalProperties: false
 
+  core-domain:
+    type: object
+    description: |
+      The vast majority of hardware blocks of Tegra SoC belong to a
+      Core power domain, which has a dedicated voltage rail that powers
+      the blocks.
+
+    properties:
+      operating-points-v2:
+        description:
+          Should contain level, voltages and opp-supported-hw property.
+          The supported-hw is a bitfield indicating SoC speedo or process
+          ID mask.
+
+      "#power-domain-cells":
+        const: 0
+
+    required:
+      - operating-points-v2
+      - "#power-domain-cells"
+
+    additionalProperties: false
+
+  core-supply:
+    description:
+      Phandle to voltage regulator connected to the SoC Core power rail.
+
 required:
   - compatible
   - reg
@@ -325,6 +352,7 @@ examples:
     tegra_pmc: pmc@7000e400 {
               compatible = "nvidia,tegra210-pmc";
               reg = <0x7000e400 0x400>;
+              core-supply = <&regulator>;
               clocks = <&tegra_car TEGRA210_CLK_PCLK>, <&clk32k_in>;
               clock-names = "pclk", "clk32k_in";
               #clock-cells = <1>;
@@ -338,17 +366,24 @@ examples:
               nvidia,core-power-req-active-high;
               nvidia,sys-clock-req-active-high;
 
+              pd_core: core-domain {
+                      operating-points-v2 = <&core_opp_table>;
+                      #power-domain-cells = <0>;
+              };
+
               powergates {
                     pd_audio: aud {
                             clocks = <&tegra_car TEGRA210_CLK_APE>,
                                      <&tegra_car TEGRA210_CLK_APB2APE>;
                             resets = <&tegra_car 198>;
+                            power-domains = <&pd_core>;
                             #power-domain-cells = <0>;
                     };
 
                     pd_xusbss: xusba {
                             clocks = <&tegra_car TEGRA210_CLK_XUSB_SS>;
                             resets = <&tegra_car TEGRA210_CLK_XUSB_SS>;
+                            power-domains = <&pd_core>;
                             #power-domain-cells = <0>;
                     };
               };
-- 
2.30.2

Re: [PATCH v2 12/14] dt-bindings: soc: tegra-pmc: Document core power domain

[Ulf Hansson]

On Mon, 24 May 2021 at 01:13, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> All NVIDIA Tegra SoCs have a core power domain where majority of hardware
> blocks reside. Document the new core power domain properties.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe


> ---
>  .../arm/tegra/nvidia,tegra20-pmc.yaml         | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml
> index 43fd2f8927d0..0afec83cc723 100644
> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-pmc.yaml
> @@ -301,6 +301,33 @@ patternProperties:
>
>      additionalProperties: false
>
> +  core-domain:
> +    type: object
> +    description: |
> +      The vast majority of hardware blocks of Tegra SoC belong to a
> +      Core power domain, which has a dedicated voltage rail that powers
> +      the blocks.
> +
> +    properties:
> +      operating-points-v2:
> +        description:
> +          Should contain level, voltages and opp-supported-hw property.
> +          The supported-hw is a bitfield indicating SoC speedo or process
> +          ID mask.
> +
> +      "#power-domain-cells":
> +        const: 0
> +
> +    required:
> +      - operating-points-v2
> +      - "#power-domain-cells"
> +
> +    additionalProperties: false
> +
> +  core-supply:
> +    description:
> +      Phandle to voltage regulator connected to the SoC Core power rail.
> +
>  required:
>    - compatible
>    - reg
> @@ -325,6 +352,7 @@ examples:
>      tegra_pmc: pmc@7000e400 {
>                compatible = "nvidia,tegra210-pmc";
>                reg = <0x7000e400 0x400>;
> +              core-supply = <&regulator>;
>                clocks = <&tegra_car TEGRA210_CLK_PCLK>, <&clk32k_in>;
>                clock-names = "pclk", "clk32k_in";
>                #clock-cells = <1>;
> @@ -338,17 +366,24 @@ examples:
>                nvidia,core-power-req-active-high;
>                nvidia,sys-clock-req-active-high;
>
> +              pd_core: core-domain {
> +                      operating-points-v2 = <&core_opp_table>;
> +                      #power-domain-cells = <0>;
> +              };
> +
>                powergates {
>                      pd_audio: aud {
>                              clocks = <&tegra_car TEGRA210_CLK_APE>,
>                                       <&tegra_car TEGRA210_CLK_APB2APE>;
>                              resets = <&tegra_car 198>;
> +                            power-domains = <&pd_core>;
>                              #power-domain-cells = <0>;
>                      };
>
>                      pd_xusbss: xusba {
>                              clocks = <&tegra_car TEGRA210_CLK_XUSB_SS>;
>                              resets = <&tegra_car TEGRA210_CLK_XUSB_SS>;
> +                            power-domains = <&pd_core>;
>                              #power-domain-cells = <0>;
>                      };
>                };
> --
> 2.30.2
>

[PATCH v2 13/14] soc/tegra: pmc: Add core power domain

[Dmitry Osipenko]

NVIDIA Tegra SoCs have multiple power domains, each domain corresponds
to an external SoC power rail. Core power domain covers vast majority of
hardware blocks within a Tegra SoC. The voltage of a power domain should
be set to a level which satisfies all devices within the power domain.
Add support for the core power domain which controls voltage state of the
domain. This allows us to support system-wide DVFS on Tegra20-210 SoCs.
The PMC powergate domains now are sub-domains of the core domain, this
requires device-tree updating, older DTBs are unaffected.

Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/Kconfig  |  14 ++++
 drivers/soc/tegra/pmc.c    | 143 +++++++++++++++++++++++++++++++++++++
 include/soc/tegra/common.h |   7 ++
 3 files changed, 164 insertions(+)

diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig
index 976dee036470..7057254604ee 100644
--- a/drivers/soc/tegra/Kconfig
+++ b/drivers/soc/tegra/Kconfig
@@ -13,6 +13,7 @@ config ARCH_TEGRA_2x_SOC
 	select PINCTRL_TEGRA20
 	select PL310_ERRATA_727915 if CACHE_L2X0
 	select PL310_ERRATA_769419 if CACHE_L2X0
+	select SOC_TEGRA_COMMON
 	select SOC_TEGRA_FLOWCTRL
 	select SOC_TEGRA_PMC
 	select SOC_TEGRA20_VOLTAGE_COUPLER
@@ -27,6 +28,7 @@ config ARCH_TEGRA_3x_SOC
 	select ARM_ERRATA_764369 if SMP
 	select PINCTRL_TEGRA30
 	select PL310_ERRATA_769419 if CACHE_L2X0
+	select SOC_TEGRA_COMMON
 	select SOC_TEGRA_FLOWCTRL
 	select SOC_TEGRA_PMC
 	select SOC_TEGRA30_VOLTAGE_COUPLER
@@ -40,6 +42,7 @@ config ARCH_TEGRA_114_SOC
 	select ARM_ERRATA_798181 if SMP
 	select HAVE_ARM_ARCH_TIMER
 	select PINCTRL_TEGRA114
+	select SOC_TEGRA_COMMON
 	select SOC_TEGRA_FLOWCTRL
 	select SOC_TEGRA_PMC
 	select TEGRA_TIMER
@@ -51,6 +54,7 @@ config ARCH_TEGRA_124_SOC
 	bool "Enable support for Tegra124 family"
 	select HAVE_ARM_ARCH_TIMER
 	select PINCTRL_TEGRA124
+	select SOC_TEGRA_COMMON
 	select SOC_TEGRA_FLOWCTRL
 	select SOC_TEGRA_PMC
 	select TEGRA_TIMER
@@ -66,6 +70,7 @@ if ARM64
 config ARCH_TEGRA_132_SOC
 	bool "NVIDIA Tegra132 SoC"
 	select PINCTRL_TEGRA124
+	select SOC_TEGRA_COMMON
 	select SOC_TEGRA_FLOWCTRL
 	select SOC_TEGRA_PMC
 	help
@@ -77,6 +82,7 @@ config ARCH_TEGRA_132_SOC
 config ARCH_TEGRA_210_SOC
 	bool "NVIDIA Tegra210 SoC"
 	select PINCTRL_TEGRA210
+	select SOC_TEGRA_COMMON
 	select SOC_TEGRA_FLOWCTRL
 	select SOC_TEGRA_PMC
 	select TEGRA_TIMER
@@ -99,6 +105,7 @@ config ARCH_TEGRA_186_SOC
 	select TEGRA_BPMP
 	select TEGRA_HSP_MBOX
 	select TEGRA_IVC
+	select SOC_TEGRA_COMMON
 	select SOC_TEGRA_PMC
 	help
 	  Enable support for the NVIDIA Tegar186 SoC. The Tegra186 features a
@@ -115,6 +122,7 @@ config ARCH_TEGRA_194_SOC
 	select TEGRA_BPMP
 	select TEGRA_HSP_MBOX
 	select TEGRA_IVC
+	select SOC_TEGRA_COMMON
 	select SOC_TEGRA_PMC
 	help
 	  Enable support for the NVIDIA Tegra194 SoC.
@@ -125,6 +133,7 @@ config ARCH_TEGRA_234_SOC
 	select TEGRA_BPMP
 	select TEGRA_HSP_MBOX
 	select TEGRA_IVC
+	select SOC_TEGRA_COMMON
 	select SOC_TEGRA_PMC
 	help
 	  Enable support for the NVIDIA Tegra234 SoC.
@@ -132,6 +141,11 @@ config ARCH_TEGRA_234_SOC
 endif
 endif
 
+config SOC_TEGRA_COMMON
+	bool
+	select PM_OPP
+	select PM_GENERIC_DOMAINS
+
 config SOC_TEGRA_FUSE
 	def_bool y
 	depends on ARCH_TEGRA
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 8e3b78bb2ac2..33b6e0885020 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -38,6 +38,7 @@
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_opp.h>
 #include <linux/reboot.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
@@ -428,6 +429,8 @@ struct tegra_pmc {
 	struct irq_chip irq;
 
 	struct notifier_block clk_nb;
+
+	bool core_domain_state_synced;
 };
 
 static struct tegra_pmc *pmc = &(struct tegra_pmc) {
@@ -1302,12 +1305,115 @@ static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
 	return err;
 }
 
+bool tegra_soc_core_domain_state_synced(void)
+{
+	return pmc->core_domain_state_synced;
+}
+
+static int
+tegra_pmc_core_pd_set_performance_state(struct generic_pm_domain *genpd,
+					unsigned int level)
+{
+	struct dev_pm_opp *opp;
+	int err;
+
+	opp = dev_pm_opp_find_level_ceil(&genpd->dev, &level);
+	if (IS_ERR(opp)) {
+		dev_err(&genpd->dev, "failed to find OPP for level %u: %pe\n",
+			level, opp);
+		return PTR_ERR(opp);
+	}
+
+	mutex_lock(&pmc->powergates_lock);
+	err = dev_pm_opp_set_opp(pmc->dev, opp);
+	mutex_unlock(&pmc->powergates_lock);
+
+	dev_pm_opp_put(opp);
+
+	if (err) {
+		dev_err(&genpd->dev, "failed to set voltage to %duV: %d\n",
+			level, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static unsigned int
+tegra_pmc_core_pd_opp_to_performance_state(struct generic_pm_domain *genpd,
+					   struct dev_pm_opp *opp)
+{
+	return dev_pm_opp_get_level(opp);
+}
+
+static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
+{
+	static struct lock_class_key tegra_core_domain_lock_class;
+	struct generic_pm_domain *genpd;
+	const char *rname = "core";
+	int err;
+
+	genpd = devm_kzalloc(pmc->dev, sizeof(*genpd), GFP_KERNEL);
+	if (!genpd)
+		return -ENOMEM;
+
+	genpd->name = np->name;
+	genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
+	genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
+
+	err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1);
+	if (err)
+		return dev_err_probe(pmc->dev, err,
+				     "failed to set core OPP regulator\n");
+
+	err = pm_genpd_init(genpd, NULL, false);
+	if (err) {
+		dev_err(pmc->dev, "failed to init core genpd: %d\n", err);
+		return err;
+	}
+
+	/*
+	 * We have a "PMC pwrgate -> Core" hierarchy of the power domains
+	 * where PMC needs to resume and change performance (voltage) of the
+	 * Core domain from the PMC GENPD on/off callbacks, hence we need
+	 * to annotate the lock in order to remove confusion from the
+	 * lockdep checker when a nested access happens.
+	 */
+	lockdep_set_class(&genpd->mlock, &tegra_core_domain_lock_class);
+
+	err = of_genpd_add_provider_simple(np, genpd);
+	if (err) {
+		dev_err(pmc->dev, "failed to add core genpd: %d\n", err);
+		goto remove_genpd;
+	}
+
+	return 0;
+
+remove_genpd:
+	pm_genpd_remove(genpd);
+
+	return err;
+}
+
 static int tegra_powergate_init(struct tegra_pmc *pmc,
 				struct device_node *parent)
 {
+	struct of_phandle_args child_args, parent_args;
 	struct device_node *np, *child;
 	int err = 0;
 
+	/*
+	 * Core power domain is the parent of powergate domains, hence it
+	 * should be registered first.
+	 */
+	np = of_get_child_by_name(parent, "core-domain");
+	if (np) {
+		err = tegra_pmc_core_pd_add(pmc, np);
+		of_node_put(np);
+		if (err)
+			return err;
+	}
+
 	np = of_get_child_by_name(parent, "powergates");
 	if (!np)
 		return 0;
@@ -1318,6 +1424,21 @@ static int tegra_powergate_init(struct tegra_pmc *pmc,
 			of_node_put(child);
 			break;
 		}
+
+		if (of_parse_phandle_with_args(child, "power-domains",
+					       "#power-domain-cells",
+					       0, &parent_args))
+			continue;
+
+		child_args.np = child;
+		child_args.args_count = 0;
+
+		err = of_genpd_add_subdomain(&parent_args, &child_args);
+		of_node_put(parent_args.np);
+		if (err) {
+			of_node_put(child);
+			break;
+		}
 	}
 
 	of_node_put(np);
@@ -1361,6 +1482,12 @@ static void tegra_powergate_remove_all(struct device_node *parent)
 	}
 
 	of_node_put(np);
+
+	np = of_get_child_by_name(parent, "core-domain");
+	if (np) {
+		of_genpd_del_provider(np);
+		of_genpd_remove_last(np);
+	}
 }
 
 static const struct tegra_io_pad_soc *
@@ -3672,6 +3799,21 @@ static const struct of_device_id tegra_pmc_match[] = {
 	{ }
 };
 
+static void tegra_pmc_sync_state(struct device *dev)
+{
+	int err;
+
+	pmc->core_domain_state_synced = true;
+
+	/* this is a no-op if core regulator isn't used */
+	mutex_lock(&pmc->powergates_lock);
+	err = dev_pm_opp_sync_regulators(dev);
+	mutex_unlock(&pmc->powergates_lock);
+
+	if (err)
+		dev_err(dev, "failed to sync regulators: %d\n", err);
+}
+
 static struct platform_driver tegra_pmc_driver = {
 	.driver = {
 		.name = "tegra-pmc",
@@ -3680,6 +3822,7 @@ static struct platform_driver tegra_pmc_driver = {
 #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM)
 		.pm = &tegra_pmc_pm_ops,
 #endif
+		.sync_state = tegra_pmc_sync_state,
 	},
 	.probe = tegra_pmc_probe,
 };
diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h
index af41ad80ec21..135a6956a18c 100644
--- a/include/soc/tegra/common.h
+++ b/include/soc/tegra/common.h
@@ -23,6 +23,8 @@ struct tegra_core_opp_params {
 #ifdef CONFIG_ARCH_TEGRA
 bool soc_is_tegra(void);
 
+bool tegra_soc_core_domain_state_synced(void);
+
 int devm_tegra_core_dev_init_opp_table(struct device *dev,
 				       struct tegra_core_opp_params *params);
 #else
@@ -31,6 +33,11 @@ static inline bool soc_is_tegra(void)
 	return false;
 }
 
+static inline bool tegra_soc_core_domain_state_synced(void)
+{
+	return false;
+}
+
 static inline int
 devm_tegra_core_dev_init_opp_table(struct device *dev,
 				   struct tegra_core_opp_params *params)
-- 
2.30.2

Re: [PATCH v2 13/14] soc/tegra: pmc: Add core power domain

[Ulf Hansson]

On Mon, 24 May 2021 at 01:13, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> NVIDIA Tegra SoCs have multiple power domains, each domain corresponds
> to an external SoC power rail. Core power domain covers vast majority of
> hardware blocks within a Tegra SoC. The voltage of a power domain should
> be set to a level which satisfies all devices within the power domain.
> Add support for the core power domain which controls voltage state of the
> domain. This allows us to support system-wide DVFS on Tegra20-210 SoCs.
> The PMC powergate domains now are sub-domains of the core domain, this
> requires device-tree updating, older DTBs are unaffected.
>
> Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
> Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
> Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
> Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

[...]

> +
> +static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
> +{
> +       static struct lock_class_key tegra_core_domain_lock_class;
> +       struct generic_pm_domain *genpd;
> +       const char *rname = "core";
> +       int err;
> +
> +       genpd = devm_kzalloc(pmc->dev, sizeof(*genpd), GFP_KERNEL);
> +       if (!genpd)
> +               return -ENOMEM;
> +
> +       genpd->name = np->name;
> +       genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
> +       genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
> +
> +       err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1);
> +       if (err)
> +               return dev_err_probe(pmc->dev, err,
> +                                    "failed to set core OPP regulator\n");
> +
> +       err = pm_genpd_init(genpd, NULL, false);
> +       if (err) {
> +               dev_err(pmc->dev, "failed to init core genpd: %d\n", err);
> +               return err;
> +       }
> +
> +       /*
> +        * We have a "PMC pwrgate -> Core" hierarchy of the power domains
> +        * where PMC needs to resume and change performance (voltage) of the
> +        * Core domain from the PMC GENPD on/off callbacks, hence we need
> +        * to annotate the lock in order to remove confusion from the
> +        * lockdep checker when a nested access happens.
> +        */

Can you elaborate a bit more on this?

Are you saying that when the child domain (PMC pwrgate) gets powered
off, you want to drop its aggregated votes it may hold for the
performance state, as otherwise it may affect the parent domain (core
domain)?

I guess this would be a valid scenario to optimize for, especially if
you have more than one child domain of the core power domain, right?

If you have only one child domain, would it be sufficient to assign
->power_on|off() callbacks for the core domain and deal with the
performance stare votes from there instead?

> +       lockdep_set_class(&genpd->mlock, &tegra_core_domain_lock_class);
> +
> +       err = of_genpd_add_provider_simple(np, genpd);
> +       if (err) {
> +               dev_err(pmc->dev, "failed to add core genpd: %d\n", err);
> +               goto remove_genpd;
> +       }
> +
> +       return 0;
> +
> +remove_genpd:
> +       pm_genpd_remove(genpd);
> +
> +       return err;
> +}

[...]

> +static void tegra_pmc_sync_state(struct device *dev)
> +{
> +       int err;
> +
> +       pmc->core_domain_state_synced = true;
> +
> +       /* this is a no-op if core regulator isn't used */
> +       mutex_lock(&pmc->powergates_lock);
> +       err = dev_pm_opp_sync_regulators(dev);
> +       mutex_unlock(&pmc->powergates_lock);
> +
> +       if (err)
> +               dev_err(dev, "failed to sync regulators: %d\n", err);
> +}
> +

Nitpick.

Would you mind splitting the "sync_state" thingy out into a separate
patch on top of $subject patch?

I think it would be nice, especially since it shares a function via
include/soc/tegra/common.h - that would make it clear to what part
that belongs to.

>  static struct platform_driver tegra_pmc_driver = {
>         .driver = {
>                 .name = "tegra-pmc",
> @@ -3680,6 +3822,7 @@ static struct platform_driver tegra_pmc_driver = {
>  #if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM)
>                 .pm = &tegra_pmc_pm_ops,
>  #endif
> +               .sync_state = tegra_pmc_sync_state,
>         },
>         .probe = tegra_pmc_probe,
>  };
> diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h
> index af41ad80ec21..135a6956a18c 100644
> --- a/include/soc/tegra/common.h
> +++ b/include/soc/tegra/common.h
> @@ -23,6 +23,8 @@ struct tegra_core_opp_params {
>  #ifdef CONFIG_ARCH_TEGRA
>  bool soc_is_tegra(void);
>
> +bool tegra_soc_core_domain_state_synced(void);
> +
>  int devm_tegra_core_dev_init_opp_table(struct device *dev,
>                                        struct tegra_core_opp_params *params);
>  #else
> @@ -31,6 +33,11 @@ static inline bool soc_is_tegra(void)
>         return false;
>  }
>
> +static inline bool tegra_soc_core_domain_state_synced(void)
> +{
> +       return false;
> +}
> +
>  static inline int
>  devm_tegra_core_dev_init_opp_table(struct device *dev,
>                                    struct tegra_core_opp_params *params)

Kind regards
Uffe

Re: [PATCH v2 13/14] soc/tegra: pmc: Add core power domain

[Dmitry Osipenko]

24.05.2021 20:04, Ulf Hansson пишет:
> On Mon, 24 May 2021 at 01:13, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> NVIDIA Tegra SoCs have multiple power domains, each domain corresponds
>> to an external SoC power rail. Core power domain covers vast majority of
>> hardware blocks within a Tegra SoC. The voltage of a power domain should
>> be set to a level which satisfies all devices within the power domain.
>> Add support for the core power domain which controls voltage state of the
>> domain. This allows us to support system-wide DVFS on Tegra20-210 SoCs.
>> The PMC powergate domains now are sub-domains of the core domain, this
>> requires device-tree updating, older DTBs are unaffected.
>>
>> Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
>> Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
>> Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
>> Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> 
> [...]
> 
>> +
>> +static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
>> +{
>> +       static struct lock_class_key tegra_core_domain_lock_class;
>> +       struct generic_pm_domain *genpd;
>> +       const char *rname = "core";
>> +       int err;
>> +
>> +       genpd = devm_kzalloc(pmc->dev, sizeof(*genpd), GFP_KERNEL);
>> +       if (!genpd)
>> +               return -ENOMEM;
>> +
>> +       genpd->name = np->name;
>> +       genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
>> +       genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
>> +
>> +       err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1);
>> +       if (err)
>> +               return dev_err_probe(pmc->dev, err,
>> +                                    "failed to set core OPP regulator\n");
>> +
>> +       err = pm_genpd_init(genpd, NULL, false);
>> +       if (err) {
>> +               dev_err(pmc->dev, "failed to init core genpd: %d\n", err);
>> +               return err;
>> +       }
>> +
>> +       /*
>> +        * We have a "PMC pwrgate -> Core" hierarchy of the power domains
>> +        * where PMC needs to resume and change performance (voltage) of the
>> +        * Core domain from the PMC GENPD on/off callbacks, hence we need
>> +        * to annotate the lock in order to remove confusion from the
>> +        * lockdep checker when a nested access happens.
>> +        */
> 
> Can you elaborate a bit more on this?
> 
> Are you saying that when the child domain (PMC pwrgate) gets powered
> off, you want to drop its aggregated votes it may hold for the
> performance state, as otherwise it may affect the parent domain (core
> domain)?

Yes, in particular we want to remove/add the performance vote when clk is disabled/enabled, see tegra_clock_runtime_resume/suspend() of the clk-runtimePM driver [1]. I'll send that clk patch separately once this series and some other tegra-clk patches will be merged, otherwise there are too many dependencies and it's too difficult to review.

[1] https://patchwork.ozlabs.org/project/linux-tegra/patch/20201217180638.22748-33-digetx@gmail.com/

Please see the example lockdep trace in the end of the email that is fixed by the mutex annotation. What we have there is the tegra-host1x device driver that resumes PMC powergate domain on Tegra30, the PMC driver enables clock from the genpd.power_on callback of the powergate domain and this propagates to the clock's RPM callback which sets the performance vote of the core domain. Hence core domain vote is set from within of the powergate domain. 

> I guess this would be a valid scenario to optimize for, especially if
> you have more than one child domain of the core power domain, right?
> 
> If you have only one child domain, would it be sufficient to assign
> ->power_on|off() callbacks for the core domain and deal with the
> performance stare votes from there instead?

The core domain is the parent domain of the PMC domains + some devices directly belong to the core domain. The GENPD core aggregates the performance votes from the children domains and from devices of the parent core, this all works great already.

It sounds to me that you're suggesting to reinvent the aggregation logic within the PMC driver and create a fake hierarchy that doesn't match hardware. It won't help the lockdep warning anyways.

I actually don't quite understand what problem you're trying to solve, could you please explain? The lockdep warning is harmless, we just need to annotate the mutex lock class.

If you don't feel comfortable with the usage of lockdep_set_class() in the driver, then maybe it should be possible to make it a part of the pm_genpd_init(). For example like we did that for tegra-host1x driver recently [2].

[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a24f98176d1efae2c37d3438c57a624d530d9c33


 LOCKDEP
 ============================================
 WARNING: possible recursive locking detected
 5.13.0-rc3-next-20210524-00202-g80a288f17147-dirty #7935 Tainted: G        W        
 --------------------------------------------
 kworker/u8:2/96 is trying to acquire lock:
 c202e494 (&genpd->mlock){+.+.}-{3:3}, at: genpd_runtime_resume+0x95/0x174
 
               but task is already holding lock:
 c35d9454 (&genpd->mlock){+.+.}-{3:3}, at: genpd_runtime_resume+0x95/0x174
 
               other info that might help us debug this:
  Possible unsafe locking scenario:

        CPU0
        ----
   lock(&genpd->mlock);
   lock(&genpd->mlock);
 
                *** DEADLOCK ***

  May be due to missing lock nesting notation

 5 locks held by kworker/u8:2/96:
  #0: c2024ea8 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x15a/0x600
  #1: c2a31f20 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x15a/0x600
  #2: c35f04d8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x29/0xdc
  #3: c35d9454 (&genpd->mlock){+.+.}-{3:3}, at: genpd_runtime_resume+0x95/0x174
  #4: c13fbbcc (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x17/0xac
 
               stack backtrace:
 CPU: 0 PID: 96 Comm: kworker/u8:2 Tainted: G        W         5.13.0-rc3-next-20210524-00202-g80a288f17147-dirty #7935
 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
 Workqueue: events_unbound deferred_probe_work_func
 [<c010d1cd>] (unwind_backtrace) from [<c0109639>] (show_stack+0x11/0x14)
 [<c0109639>] (show_stack) from [<c0ba6dab>] (dump_stack_lvl+0x97/0xb0)
 [<c0ba6dab>] (dump_stack_lvl) from [<c017b24f>] (__lock_acquire+0x7fb/0x2534)
 [<c017b24f>] (__lock_acquire) from [<c017d75b>] (lock_acquire+0xf3/0x424)
 [<c017d75b>] (lock_acquire) from [<c0bb0daf>] (__mutex_lock+0x87/0x7f4)
 [<c0bb0daf>] (__mutex_lock) from [<c0bb1535>] (mutex_lock_nested+0x19/0x20)
 [<c0bb1535>] (mutex_lock_nested) from [<c0669ced>] (genpd_runtime_resume+0x95/0x174)
 [<c0669ced>] (genpd_runtime_resume) from [<c0660167>] (__rpm_callback+0x7b/0xc8)
 [<c0660167>] (__rpm_callback) from [<c06601cd>] (rpm_callback+0x19/0x60)
 [<c06601cd>] (rpm_callback) from [<c065fde3>] (rpm_resume+0x47f/0x65c)
 [<c065fde3>] (rpm_resume) from [<c066000f>] (__pm_runtime_resume+0x4f/0x78)
 [<c066000f>] (__pm_runtime_resume) from [<c05857f7>] (clk_pm_runtime_get.part.0+0x13/0x54)
 [<c05857f7>] (clk_pm_runtime_get.part.0) from [<c05881e9>] (clk_core_set_rate_nolock+0x81/0x1cc)
 [<c05881e9>] (clk_core_set_rate_nolock) from [<c0588353>] (clk_set_rate+0x1f/0x44)
 [<c0588353>] (clk_set_rate) from [<c0597cd3>] (tegra_powergate_prepare_clocks+0x2f/0x94)
 [<c0597cd3>] (tegra_powergate_prepare_clocks) from [<c059a4d1>] (tegra_powergate_power_up+0x45/0xec)
 [<c059a4d1>] (tegra_powergate_power_up) from [<c0ba7211>] (tegra_genpd_power_on+0x2b/0x50)
 [<c0ba7211>] (tegra_genpd_power_on) from [<c0667231>] (_genpd_power_on+0x6d/0xb8)
 [<c0667231>] (_genpd_power_on) from [<c066999d>] (genpd_power_on.part.0+0x85/0xf0)
 [<c066999d>] (genpd_power_on.part.0) from [<c0669cfb>] (genpd_runtime_resume+0xa3/0x174)
 [<c0669cfb>] (genpd_runtime_resume) from [<c0660167>] (__rpm_callback+0x7b/0xc8)
 [<c0660167>] (__rpm_callback) from [<c06601cd>] (rpm_callback+0x19/0x60)
 [<c06601cd>] (rpm_callback) from [<c065fde3>] (rpm_resume+0x47f/0x65c)
 [<c065fde3>] (rpm_resume) from [<c066000f>] (__pm_runtime_resume+0x4f/0x78)
 [<c066000f>] (__pm_runtime_resume) from [<c065675f>] (__device_attach+0x83/0xdc)
 [<c065675f>] (__device_attach) from [<c0655d55>] (bus_probe_device+0x5d/0x64)
 [<c0655d55>] (bus_probe_device) from [<c06560b7>] (deferred_probe_work_func+0x63/0x88)
 [<c06560b7>] (deferred_probe_work_func) from [<c0139993>] (process_one_work+0x1eb/0x600)
 [<c0139993>] (process_one_work) from [<c0139fcf>] (worker_thread+0x227/0x3bc)
 [<c0139fcf>] (worker_thread) from [<c0140ab3>] (kthread+0x13f/0x15c)
 [<c0140ab3>] (kthread) from [<c0100159>] (ret_from_fork+0x11/0x38)
 Exception stack(0xc2a31fb0 to 0xc2a31ff8)

Re: [PATCH v2 13/14] soc/tegra: pmc: Add core power domain

[Ulf Hansson]

On Mon, 24 May 2021 at 22:23, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 24.05.2021 20:04, Ulf Hansson пишет:
> > On Mon, 24 May 2021 at 01:13, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> NVIDIA Tegra SoCs have multiple power domains, each domain corresponds
> >> to an external SoC power rail. Core power domain covers vast majority of
> >> hardware blocks within a Tegra SoC. The voltage of a power domain should
> >> be set to a level which satisfies all devices within the power domain.
> >> Add support for the core power domain which controls voltage state of the
> >> domain. This allows us to support system-wide DVFS on Tegra20-210 SoCs.
> >> The PMC powergate domains now are sub-domains of the core domain, this
> >> requires device-tree updating, older DTBs are unaffected.
> >>
> >> Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
> >> Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
> >> Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
> >> Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >
> > [...]
> >
> >> +
> >> +static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
> >> +{
> >> +       static struct lock_class_key tegra_core_domain_lock_class;
> >> +       struct generic_pm_domain *genpd;
> >> +       const char *rname = "core";
> >> +       int err;
> >> +
> >> +       genpd = devm_kzalloc(pmc->dev, sizeof(*genpd), GFP_KERNEL);
> >> +       if (!genpd)
> >> +               return -ENOMEM;
> >> +
> >> +       genpd->name = np->name;
> >> +       genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
> >> +       genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
> >> +
> >> +       err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1);
> >> +       if (err)
> >> +               return dev_err_probe(pmc->dev, err,
> >> +                                    "failed to set core OPP regulator\n");
> >> +
> >> +       err = pm_genpd_init(genpd, NULL, false);
> >> +       if (err) {
> >> +               dev_err(pmc->dev, "failed to init core genpd: %d\n", err);
> >> +               return err;
> >> +       }
> >> +
> >> +       /*
> >> +        * We have a "PMC pwrgate -> Core" hierarchy of the power domains
> >> +        * where PMC needs to resume and change performance (voltage) of the
> >> +        * Core domain from the PMC GENPD on/off callbacks, hence we need
> >> +        * to annotate the lock in order to remove confusion from the
> >> +        * lockdep checker when a nested access happens.
> >> +        */
> >
> > Can you elaborate a bit more on this?
> >
> > Are you saying that when the child domain (PMC pwrgate) gets powered
> > off, you want to drop its aggregated votes it may hold for the
> > performance state, as otherwise it may affect the parent domain (core
> > domain)?
>
> Yes, in particular we want to remove/add the performance vote when clk is disabled/enabled, see tegra_clock_runtime_resume/suspend() of the clk-runtimePM driver [1]. I'll send that clk patch separately once this series and some other tegra-clk patches will be merged, otherwise there are too many dependencies and it's too difficult to review.

You are likely correct from the merging point of view, but for
completeness I would prefer to look at the whole series. Would you
mind folding in some of these changes too?

>
> [1] https://patchwork.ozlabs.org/project/linux-tegra/patch/20201217180638.22748-33-digetx@gmail.com/

Hmm. In general, the new changes to genpd and the opp library with
"performance states" for DVFS, should help to avoid using clock rate
notifications.

Instead of updating the performance votes from the clock provider
driver, the more proper thing would be to let the clock consumer
driver (various drivers) to call dev_pm_opp_set_rate() when it needs
to move to a new OPP. This also means calling dev_pm_opp_set_rate(dev,
0) when the votes for an OPP can be dropped.

In this way, the opp library will call genpd to update the performance
state vote for the corresponding device.

>
> Please see the example lockdep trace in the end of the email that is fixed by the mutex annotation. What we have there is the tegra-host1x device driver that resumes PMC powergate domain on Tegra30, the PMC driver enables clock from the genpd.power_on callback of the powergate domain and this propagates to the clock's RPM callback which sets the performance vote of the core domain. Hence core domain vote is set from within of the powergate domain.

Right, this sounds like a fragile looking path. Are you sure it can't
lead into deadlock situations?

In any case, we designed dev_pm_opp_set_rate() (and its friends in
genpd) with these locking issues in mind.

>
> > I guess this would be a valid scenario to optimize for, especially if
> > you have more than one child domain of the core power domain, right?
> >
> > If you have only one child domain, would it be sufficient to assign
> > ->power_on|off() callbacks for the core domain and deal with the
> > performance stare votes from there instead?
>
> The core domain is the parent domain of the PMC domains + some devices directly belong to the core domain. The GENPD core aggregates the performance votes from the children domains and from devices of the parent core, this all works great already.
>
> It sounds to me that you're suggesting to reinvent the aggregation logic within the PMC driver and create a fake hierarchy that doesn't match hardware. It won't help the lockdep warning anyways.
>
> I actually don't quite understand what problem you're trying to solve, could you please explain? The lockdep warning is harmless, we just need to annotate the mutex lock class.
>
> If you don't feel comfortable with the usage of lockdep_set_class() in the driver, then maybe it should be possible to make it a part of the pm_genpd_init(). For example like we did that for tegra-host1x driver recently [2].

I was not trying to solve a problem, but was just curious and wanted
to ask about the reasons for the lockdep_set_class(), as it simply
caught my attention while reviewing.

Looks like there may be something fishy going on, but let's see, I may be wrong.

>
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a24f98176d1efae2c37d3438c57a624d530d9c33
>
>
>  LOCKDEP
>  ============================================
>  WARNING: possible recursive locking detected
>  5.13.0-rc3-next-20210524-00202-g80a288f17147-dirty #7935 Tainted: G        W
>  --------------------------------------------
>  kworker/u8:2/96 is trying to acquire lock:
>  c202e494 (&genpd->mlock){+.+.}-{3:3}, at: genpd_runtime_resume+0x95/0x174
>
>                but task is already holding lock:
>  c35d9454 (&genpd->mlock){+.+.}-{3:3}, at: genpd_runtime_resume+0x95/0x174
>
>                other info that might help us debug this:
>   Possible unsafe locking scenario:
>
>         CPU0
>         ----
>    lock(&genpd->mlock);
>    lock(&genpd->mlock);
>
>                 *** DEADLOCK ***
>
>   May be due to missing lock nesting notation
>
>  5 locks held by kworker/u8:2/96:
>   #0: c2024ea8 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x15a/0x600
>   #1: c2a31f20 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x15a/0x600
>   #2: c35f04d8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x29/0xdc
>   #3: c35d9454 (&genpd->mlock){+.+.}-{3:3}, at: genpd_runtime_resume+0x95/0x174
>   #4: c13fbbcc (prepare_lock){+.+.}-{3:3}, at: clk_prepare_lock+0x17/0xac
>
>                stack backtrace:
>  CPU: 0 PID: 96 Comm: kworker/u8:2 Tainted: G        W         5.13.0-rc3-next-20210524-00202-g80a288f17147-dirty #7935
>  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>  Workqueue: events_unbound deferred_probe_work_func
>  [<c010d1cd>] (unwind_backtrace) from [<c0109639>] (show_stack+0x11/0x14)
>  [<c0109639>] (show_stack) from [<c0ba6dab>] (dump_stack_lvl+0x97/0xb0)
>  [<c0ba6dab>] (dump_stack_lvl) from [<c017b24f>] (__lock_acquire+0x7fb/0x2534)
>  [<c017b24f>] (__lock_acquire) from [<c017d75b>] (lock_acquire+0xf3/0x424)
>  [<c017d75b>] (lock_acquire) from [<c0bb0daf>] (__mutex_lock+0x87/0x7f4)
>  [<c0bb0daf>] (__mutex_lock) from [<c0bb1535>] (mutex_lock_nested+0x19/0x20)
>  [<c0bb1535>] (mutex_lock_nested) from [<c0669ced>] (genpd_runtime_resume+0x95/0x174)
>  [<c0669ced>] (genpd_runtime_resume) from [<c0660167>] (__rpm_callback+0x7b/0xc8)
>  [<c0660167>] (__rpm_callback) from [<c06601cd>] (rpm_callback+0x19/0x60)
>  [<c06601cd>] (rpm_callback) from [<c065fde3>] (rpm_resume+0x47f/0x65c)
>  [<c065fde3>] (rpm_resume) from [<c066000f>] (__pm_runtime_resume+0x4f/0x78)
>  [<c066000f>] (__pm_runtime_resume) from [<c05857f7>] (clk_pm_runtime_get.part.0+0x13/0x54)
>  [<c05857f7>] (clk_pm_runtime_get.part.0) from [<c05881e9>] (clk_core_set_rate_nolock+0x81/0x1cc)
>  [<c05881e9>] (clk_core_set_rate_nolock) from [<c0588353>] (clk_set_rate+0x1f/0x44)
>  [<c0588353>] (clk_set_rate) from [<c0597cd3>] (tegra_powergate_prepare_clocks+0x2f/0x94)
>  [<c0597cd3>] (tegra_powergate_prepare_clocks) from [<c059a4d1>] (tegra_powergate_power_up+0x45/0xec)
>  [<c059a4d1>] (tegra_powergate_power_up) from [<c0ba7211>] (tegra_genpd_power_on+0x2b/0x50)
>  [<c0ba7211>] (tegra_genpd_power_on) from [<c0667231>] (_genpd_power_on+0x6d/0xb8)
>  [<c0667231>] (_genpd_power_on) from [<c066999d>] (genpd_power_on.part.0+0x85/0xf0)
>  [<c066999d>] (genpd_power_on.part.0) from [<c0669cfb>] (genpd_runtime_resume+0xa3/0x174)
>  [<c0669cfb>] (genpd_runtime_resume) from [<c0660167>] (__rpm_callback+0x7b/0xc8)
>  [<c0660167>] (__rpm_callback) from [<c06601cd>] (rpm_callback+0x19/0x60)
>  [<c06601cd>] (rpm_callback) from [<c065fde3>] (rpm_resume+0x47f/0x65c)
>  [<c065fde3>] (rpm_resume) from [<c066000f>] (__pm_runtime_resume+0x4f/0x78)
>  [<c066000f>] (__pm_runtime_resume) from [<c065675f>] (__device_attach+0x83/0xdc)
>  [<c065675f>] (__device_attach) from [<c0655d55>] (bus_probe_device+0x5d/0x64)
>  [<c0655d55>] (bus_probe_device) from [<c06560b7>] (deferred_probe_work_func+0x63/0x88)
>  [<c06560b7>] (deferred_probe_work_func) from [<c0139993>] (process_one_work+0x1eb/0x600)
>  [<c0139993>] (process_one_work) from [<c0139fcf>] (worker_thread+0x227/0x3bc)
>  [<c0139fcf>] (worker_thread) from [<c0140ab3>] (kthread+0x13f/0x15c)
>  [<c0140ab3>] (kthread) from [<c0100159>] (ret_from_fork+0x11/0x38)
>  Exception stack(0xc2a31fb0 to 0xc2a31ff8)

Thanks for sharing the log.

Could you perhaps point me to the corresponding DTS files. I would
like to understand more about the PM domain hierarchy and where the
clock controller may be located.

Kind regards
Uffe

Re: [PATCH v2 13/14] soc/tegra: pmc: Add core power domain

[Dmitry Osipenko]

31.05.2021 16:17, Ulf Hansson пишет:
> On Mon, 24 May 2021 at 22:23, Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 24.05.2021 20:04, Ulf Hansson пишет:
>>> On Mon, 24 May 2021 at 01:13, Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> NVIDIA Tegra SoCs have multiple power domains, each domain corresponds
>>>> to an external SoC power rail. Core power domain covers vast majority of
>>>> hardware blocks within a Tegra SoC. The voltage of a power domain should
>>>> be set to a level which satisfies all devices within the power domain.
>>>> Add support for the core power domain which controls voltage state of the
>>>> domain. This allows us to support system-wide DVFS on Tegra20-210 SoCs.
>>>> The PMC powergate domains now are sub-domains of the core domain, this
>>>> requires device-tree updating, older DTBs are unaffected.
>>>>
>>>> Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
>>>> Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
>>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
>>>> Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>
>>> [...]
>>>
>>>> +
>>>> +static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
>>>> +{
>>>> +       static struct lock_class_key tegra_core_domain_lock_class;
>>>> +       struct generic_pm_domain *genpd;
>>>> +       const char *rname = "core";
>>>> +       int err;
>>>> +
>>>> +       genpd = devm_kzalloc(pmc->dev, sizeof(*genpd), GFP_KERNEL);
>>>> +       if (!genpd)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       genpd->name = np->name;
>>>> +       genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
>>>> +       genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
>>>> +
>>>> +       err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1);
>>>> +       if (err)
>>>> +               return dev_err_probe(pmc->dev, err,
>>>> +                                    "failed to set core OPP regulator\n");
>>>> +
>>>> +       err = pm_genpd_init(genpd, NULL, false);
>>>> +       if (err) {
>>>> +               dev_err(pmc->dev, "failed to init core genpd: %d\n", err);
>>>> +               return err;
>>>> +       }
>>>> +
>>>> +       /*
>>>> +        * We have a "PMC pwrgate -> Core" hierarchy of the power domains
>>>> +        * where PMC needs to resume and change performance (voltage) of the
>>>> +        * Core domain from the PMC GENPD on/off callbacks, hence we need
>>>> +        * to annotate the lock in order to remove confusion from the
>>>> +        * lockdep checker when a nested access happens.
>>>> +        */
>>>
>>> Can you elaborate a bit more on this?
>>>
>>> Are you saying that when the child domain (PMC pwrgate) gets powered
>>> off, you want to drop its aggregated votes it may hold for the
>>> performance state, as otherwise it may affect the parent domain (core
>>> domain)?
>>
>> Yes, in particular we want to remove/add the performance vote when clk is disabled/enabled, see tegra_clock_runtime_resume/suspend() of the clk-runtimePM driver [1]. I'll send that clk patch separately once this series and some other tegra-clk patches will be merged, otherwise there are too many dependencies and it's too difficult to review.
> 
> You are likely correct from the merging point of view, but for
> completeness I would prefer to look at the whole series. Would you
> mind folding in some of these changes too?

Sure, I will send those changes once this fundamental patchset will be merged. 

>> [1] https://patchwork.ozlabs.org/project/linux-tegra/patch/20201217180638.22748-33-digetx@gmail.com/
> 
> Hmm. In general, the new changes to genpd and the opp library with
> "performance states" for DVFS, should help to avoid using clock rate
> notifications.
> 
> Instead of updating the performance votes from the clock provider
> driver, the more proper thing would be to let the clock consumer
> driver (various drivers) to call dev_pm_opp_set_rate() when it needs
> to move to a new OPP. This also means calling dev_pm_opp_set_rate(dev,
> 0) when the votes for an OPP can be dropped.
> 
> In this way, the opp library will call genpd to update the performance
> state vote for the corresponding device.

This is not sufficient for Tegra because we have individual OPP tables for the root PLLs, system clocks and device clocks. The device clocks could be muxed to a different PLLs, depending on clk requirements of a particular board.

>> Please see the example lockdep trace in the end of the email that is fixed by the mutex annotation. What we have there is the tegra-host1x device driver that resumes PMC powergate domain on Tegra30, the PMC driver enables clock from the genpd.power_on callback of the powergate domain and this propagates to the clock's RPM callback which sets the performance vote of the core domain. Hence core domain vote is set from within of the powergate domain.
> 
> Right, this sounds like a fragile looking path. Are you sure it can't
> lead into deadlock situations?

The locking was a concern, but turned out that it works nicely for the Tegra clk/power tree. This approach may not be suitable for other SoCs. 

> In any case, we designed dev_pm_opp_set_rate() (and its friends in
> genpd) with these locking issues in mind.

The device drivers don't manage the parent clocks directly and OPP core doesn't support this use-case where OPP needs to be applied to a generic/parent PLL clock. Moving the OPP management to the clk driver is the easy solution which works good in practice for Tegra, it also removes a need to switch each driver to dev_pm_opp_set_rate() usage.

>>> I guess this would be a valid scenario to optimize for, especially if
>>> you have more than one child domain of the core power domain, right?
>>>
>>> If you have only one child domain, would it be sufficient to assign
>>> ->power_on|off() callbacks for the core domain and deal with the
>>> performance stare votes from there instead?
>>
>> The core domain is the parent domain of the PMC domains + some devices directly belong to the core domain. The GENPD core aggregates the performance votes from the children domains and from devices of the parent core, this all works great already.
>>
>> It sounds to me that you're suggesting to reinvent the aggregation logic within the PMC driver and create a fake hierarchy that doesn't match hardware. It won't help the lockdep warning anyways.
>>
>> I actually don't quite understand what problem you're trying to solve, could you please explain? The lockdep warning is harmless, we just need to annotate the mutex lock class.
>>
>> If you don't feel comfortable with the usage of lockdep_set_class() in the driver, then maybe it should be possible to make it a part of the pm_genpd_init(). For example like we did that for tegra-host1x driver recently [2].
> 
> I was not trying to solve a problem, but was just curious and wanted
> to ask about the reasons for the lockdep_set_class(), as it simply
> caught my attention while reviewing.
> 
> Looks like there may be something fishy going on, but let's see, I may be wrong.
> 
>>
>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a24f98176d1efae2c37d3438c57a624d530d9c33
>>
>>
>>  LOCKDEP
>>  ============================================
...
> 
> Thanks for sharing the log.
> 
> Could you perhaps point me to the corresponding DTS files. I would
> like to understand more about the PM domain hierarchy and where the
> clock controller may be located.

https://github.com/grate-driver/linux/tree/master has all the latest versions of patches applied.

For example please see clock@60006000 and pmc@7000e400 nodes of [1].

[1] https://github.com/grate-driver/linux/blob/master/arch/arm/boot/dts/tegra30.dtsi

This is how output of pm_genpd_summary looks like on Tegra30 Nexus 7, where performance=voltage:

domain                          status          children                           performance
    /device                                             runtime status
----------------------------------------------------------------------------------------------
heg                             on                                                 1000000
    /devices/soc0/50000000.host1x                       active                     1000000
    /devices/soc0/50000000.host1x/540c0000.epp          suspended                  0
    /devices/soc0/50000000.host1x/54140000.gr2d         suspended                  0
mpe                             off-0                                              0
    /devices/soc0/50000000.host1x/54040000.mpe          suspended                  0
vdec                            off-0                                              0
    /devices/soc0/6001a000.vde                          suspended                  0
venc                            off-0                                              0
    /devices/soc0/50000000.host1x/54080000.vi           suspended                  0
    /devices/soc0/50000000.host1x/54100000.isp          suspended                  0
3d1                             off-0                                              0
    /devices/genpd:1:54180000.gr3d                      suspended                  0
3d0                             off-0                                              0
    /devices/genpd:0:54180000.gr3d                      suspended                  0
core-domain                     on                                                 1000000
                                                3d0, 3d1, venc, vdec, mpe, heg
    /devices/soc0/7000f400.memory-controller            unsupported                1000000
    /devices/platform/tegra_clk_pll_c                   active                     1000000
    /devices/platform/tegra_clk_pll_e                   suspended                  0
    /devices/platform/tegra_clk_pll_m                   active                     1000000
    /devices/platform/tegra_clk_sclk                    active                     1000000
    /devices/platform/tegra_clk_dsia                    suspended                  0
    /devices/platform/tegra_clk_dsib                    suspended                  0
    /devices/platform/tegra_clk_spdif_out               suspended                  0
    /devices/platform/tegra_clk_se                      suspended                  0
    /devices/platform/tegra_clk_hdmi                    suspended                  0
    /devices/platform/tegra_clk_pwm                     active                     1000000
    /devices/platform/tegra_clk_pcie                    suspended                  0
    /devices/platform/tegra_clk_afi                     suspended                  0
    /devices/platform/tegra_clk_vde                     suspended                  0
    /devices/platform/tegra_clk_vi                      suspended                  0
    /devices/platform/tegra_clk_epp                     suspended                  0
    /devices/platform/tegra_clk_mpe                     suspended                  0
    /devices/platform/tegra_clk_nor                     suspended                  0
    /devices/platform/tegra_clk_sdmmc1                  suspended                  0
    /devices/platform/tegra_clk_sdmmc3                  active                     950000
    /devices/platform/tegra_clk_mipi                    suspended                  0
    /devices/platform/tegra_clk_sbc1                    suspended                  0
    /devices/platform/tegra_clk_sbc2                    suspended                  0
    /devices/platform/tegra_clk_sbc3                    suspended                  0
    /devices/platform/tegra_clk_sbc4                    suspended                  0
    /devices/platform/tegra_clk_sbc5                    suspended                  0
    /devices/platform/tegra_clk_sbc6                    suspended                  0
    /devices/platform/tegra_clk_cve                     suspended                  0
    /devices/platform/tegra_clk_tvo                     suspended                  0
    /devices/platform/tegra_clk_tvdac                   suspended                  0
    /devices/platform/tegra_clk_ndflash                 suspended                  0
    /devices/platform/tegra_clk_sata_oob                suspended                  0
    /devices/platform/tegra_clk_sata                    suspended                  0
    /devices/platform/tegra_clk_fuse_burn               suspended                  0
    /devices/platform/tegra_clk_usbd                    active                     1000000
    /devices/platform/tegra_clk_usb2                    suspended                  0
    /devices/platform/tegra_clk_usb3                    suspended                  0
    /devices/soc0/50000000.host1x/54200000.dc           active                     1000000
    /devices/soc0/50000000.host1x/54240000.dc           suspended                  0

Re: [PATCH v2 13/14] soc/tegra: pmc: Add core power domain

[Ulf Hansson]

On Mon, 31 May 2021 at 22:07, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 31.05.2021 16:17, Ulf Hansson пишет:
> > On Mon, 24 May 2021 at 22:23, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> 24.05.2021 20:04, Ulf Hansson пишет:
> >>> On Mon, 24 May 2021 at 01:13, Dmitry Osipenko <digetx@gmail.com> wrote:
> >>>>
> >>>> NVIDIA Tegra SoCs have multiple power domains, each domain corresponds
> >>>> to an external SoC power rail. Core power domain covers vast majority of
> >>>> hardware blocks within a Tegra SoC. The voltage of a power domain should
> >>>> be set to a level which satisfies all devices within the power domain.
> >>>> Add support for the core power domain which controls voltage state of the
> >>>> domain. This allows us to support system-wide DVFS on Tegra20-210 SoCs.
> >>>> The PMC powergate domains now are sub-domains of the core domain, this
> >>>> requires device-tree updating, older DTBs are unaffected.
> >>>>
> >>>> Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
> >>>> Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
> >>>> Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
> >>>> Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>>
> >>> [...]
> >>>
> >>>> +
> >>>> +static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
> >>>> +{
> >>>> +       static struct lock_class_key tegra_core_domain_lock_class;
> >>>> +       struct generic_pm_domain *genpd;
> >>>> +       const char *rname = "core";
> >>>> +       int err;
> >>>> +
> >>>> +       genpd = devm_kzalloc(pmc->dev, sizeof(*genpd), GFP_KERNEL);
> >>>> +       if (!genpd)
> >>>> +               return -ENOMEM;
> >>>> +
> >>>> +       genpd->name = np->name;
> >>>> +       genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
> >>>> +       genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
> >>>> +
> >>>> +       err = devm_pm_opp_set_regulators(pmc->dev, &rname, 1);
> >>>> +       if (err)
> >>>> +               return dev_err_probe(pmc->dev, err,
> >>>> +                                    "failed to set core OPP regulator\n");
> >>>> +
> >>>> +       err = pm_genpd_init(genpd, NULL, false);
> >>>> +       if (err) {
> >>>> +               dev_err(pmc->dev, "failed to init core genpd: %d\n", err);
> >>>> +               return err;
> >>>> +       }
> >>>> +
> >>>> +       /*
> >>>> +        * We have a "PMC pwrgate -> Core" hierarchy of the power domains
> >>>> +        * where PMC needs to resume and change performance (voltage) of the
> >>>> +        * Core domain from the PMC GENPD on/off callbacks, hence we need
> >>>> +        * to annotate the lock in order to remove confusion from the
> >>>> +        * lockdep checker when a nested access happens.
> >>>> +        */
> >>>
> >>> Can you elaborate a bit more on this?
> >>>
> >>> Are you saying that when the child domain (PMC pwrgate) gets powered
> >>> off, you want to drop its aggregated votes it may hold for the
> >>> performance state, as otherwise it may affect the parent domain (core
> >>> domain)?
> >>
> >> Yes, in particular we want to remove/add the performance vote when clk is disabled/enabled, see tegra_clock_runtime_resume/suspend() of the clk-runtimePM driver [1]. I'll send that clk patch separately once this series and some other tegra-clk patches will be merged, otherwise there are too many dependencies and it's too difficult to review.
> >
> > You are likely correct from the merging point of view, but for
> > completeness I would prefer to look at the whole series. Would you
> > mind folding in some of these changes too?
>
> Sure, I will send those changes once this fundamental patchset will be merged.
>
> >> [1] https://patchwork.ozlabs.org/project/linux-tegra/patch/20201217180638.22748-33-digetx@gmail.com/
> >
> > Hmm. In general, the new changes to genpd and the opp library with
> > "performance states" for DVFS, should help to avoid using clock rate
> > notifications.
> >
> > Instead of updating the performance votes from the clock provider
> > driver, the more proper thing would be to let the clock consumer
> > driver (various drivers) to call dev_pm_opp_set_rate() when it needs
> > to move to a new OPP. This also means calling dev_pm_opp_set_rate(dev,
> > 0) when the votes for an OPP can be dropped.
> >
> > In this way, the opp library will call genpd to update the performance
> > state vote for the corresponding device.
>
> This is not sufficient for Tegra because we have individual OPP tables for the root PLLs, system clocks and device clocks. The device clocks could be muxed to a different PLLs, depending on clk requirements of a particular board.

Are you saying that the clock providers for the "root PLLs" and
"system clocks" have OPP tables themselves? If so, would you mind
posting a patch for an updated DT binding for these changes, so it can
be discussed separately?

>
> >> Please see the example lockdep trace in the end of the email that is fixed by the mutex annotation. What we have there is the tegra-host1x device driver that resumes PMC powergate domain on Tegra30, the PMC driver enables clock from the genpd.power_on callback of the powergate domain and this propagates to the clock's RPM callback which sets the performance vote of the core domain. Hence core domain vote is set from within of the powergate domain.
> >
> > Right, this sounds like a fragile looking path. Are you sure it can't
> > lead into deadlock situations?
>
> The locking was a concern, but turned out that it works nicely for the Tegra clk/power tree. This approach may not be suitable for other SoCs.
>
> > In any case, we designed dev_pm_opp_set_rate() (and its friends in
> > genpd) with these locking issues in mind.
>
> The device drivers don't manage the parent clocks directly and OPP core doesn't support this use-case where OPP needs to be applied to a generic/parent PLL clock. Moving the OPP management to the clk driver is the easy solution which works good in practice for Tegra, it also removes a need to switch each driver to dev_pm_opp_set_rate() usage.

I admit, if clock consumer drivers could avoid calling
dev_pm_opp_set_rate|opp(), that would be nice. But, as I stated, it's
a fragile path from locking point of view, to call
dev_pm_opp_set_rate|opp() from a clock provider driver. Personally, I
think it's better to avoid it.

More importantly, you also need to convince the clock subsystem
maintainers, that setting an OPP internally from the clock provider
driver is a good idea. As far as I can tell, they have said *no* to
this, since the common clock framework was invented, I believe for
good reasons.

>
> >>> I guess this would be a valid scenario to optimize for, especially if
> >>> you have more than one child domain of the core power domain, right?
> >>>
> >>> If you have only one child domain, would it be sufficient to assign
> >>> ->power_on|off() callbacks for the core domain and deal with the
> >>> performance stare votes from there instead?
> >>
> >> The core domain is the parent domain of the PMC domains + some devices directly belong to the core domain. The GENPD core aggregates the performance votes from the children domains and from devices of the parent core, this all works great already.
> >>
> >> It sounds to me that you're suggesting to reinvent the aggregation logic within the PMC driver and create a fake hierarchy that doesn't match hardware. It won't help the lockdep warning anyways.
> >>
> >> I actually don't quite understand what problem you're trying to solve, could you please explain? The lockdep warning is harmless, we just need to annotate the mutex lock class.
> >>
> >> If you don't feel comfortable with the usage of lockdep_set_class() in the driver, then maybe it should be possible to make it a part of the pm_genpd_init(). For example like we did that for tegra-host1x driver recently [2].
> >
> > I was not trying to solve a problem, but was just curious and wanted
> > to ask about the reasons for the lockdep_set_class(), as it simply
> > caught my attention while reviewing.
> >
> > Looks like there may be something fishy going on, but let's see, I may be wrong.
> >
> >>
> >> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a24f98176d1efae2c37d3438c57a624d530d9c33
> >>
> >>
> >>  LOCKDEP
> >>  ============================================
> ...
> >
> > Thanks for sharing the log.
> >
> > Could you perhaps point me to the corresponding DTS files. I would
> > like to understand more about the PM domain hierarchy and where the
> > clock controller may be located.
>
> https://github.com/grate-driver/linux/tree/master has all the latest versions of patches applied.
>
> For example please see clock@60006000 and pmc@7000e400 nodes of [1].
>
> [1] https://github.com/grate-driver/linux/blob/master/arch/arm/boot/dts/tegra30.dtsi

Thanks, that certainly helped me understand better!

I see that you want to add OPP tables to clock provider nodes. As I
said above, an updated DT binding is probably a good idea to discuss
separately.

>
> This is how output of pm_genpd_summary looks like on Tegra30 Nexus 7, where performance=voltage:
>
> domain                          status          children                           performance
>     /device                                             runtime status
> ----------------------------------------------------------------------------------------------
> heg                             on                                                 1000000
>     /devices/soc0/50000000.host1x                       active                     1000000
>     /devices/soc0/50000000.host1x/540c0000.epp          suspended                  0
>     /devices/soc0/50000000.host1x/54140000.gr2d         suspended                  0
> mpe                             off-0                                              0
>     /devices/soc0/50000000.host1x/54040000.mpe          suspended                  0
> vdec                            off-0                                              0
>     /devices/soc0/6001a000.vde                          suspended                  0
> venc                            off-0                                              0
>     /devices/soc0/50000000.host1x/54080000.vi           suspended                  0
>     /devices/soc0/50000000.host1x/54100000.isp          suspended                  0
> 3d1                             off-0                                              0
>     /devices/genpd:1:54180000.gr3d                      suspended                  0
> 3d0                             off-0                                              0
>     /devices/genpd:0:54180000.gr3d                      suspended                  0
> core-domain                     on                                                 1000000
>                                                 3d0, 3d1, venc, vdec, mpe, heg
>     /devices/soc0/7000f400.memory-controller            unsupported                1000000
>     /devices/platform/tegra_clk_pll_c                   active                     1000000
>     /devices/platform/tegra_clk_pll_e                   suspended                  0
>     /devices/platform/tegra_clk_pll_m                   active                     1000000
>     /devices/platform/tegra_clk_sclk                    active                     1000000
>     /devices/platform/tegra_clk_dsia                    suspended                  0
>     /devices/platform/tegra_clk_dsib                    suspended                  0
>     /devices/platform/tegra_clk_spdif_out               suspended                  0
>     /devices/platform/tegra_clk_se                      suspended                  0
>     /devices/platform/tegra_clk_hdmi                    suspended                  0
>     /devices/platform/tegra_clk_pwm                     active                     1000000
>     /devices/platform/tegra_clk_pcie                    suspended                  0
>     /devices/platform/tegra_clk_afi                     suspended                  0
>     /devices/platform/tegra_clk_vde                     suspended                  0
>     /devices/platform/tegra_clk_vi                      suspended                  0
>     /devices/platform/tegra_clk_epp                     suspended                  0
>     /devices/platform/tegra_clk_mpe                     suspended                  0
>     /devices/platform/tegra_clk_nor                     suspended                  0
>     /devices/platform/tegra_clk_sdmmc1                  suspended                  0
>     /devices/platform/tegra_clk_sdmmc3                  active                     950000
>     /devices/platform/tegra_clk_mipi                    suspended                  0
>     /devices/platform/tegra_clk_sbc1                    suspended                  0
>     /devices/platform/tegra_clk_sbc2                    suspended                  0
>     /devices/platform/tegra_clk_sbc3                    suspended                  0
>     /devices/platform/tegra_clk_sbc4                    suspended                  0
>     /devices/platform/tegra_clk_sbc5                    suspended                  0
>     /devices/platform/tegra_clk_sbc6                    suspended                  0
>     /devices/platform/tegra_clk_cve                     suspended                  0
>     /devices/platform/tegra_clk_tvo                     suspended                  0
>     /devices/platform/tegra_clk_tvdac                   suspended                  0
>     /devices/platform/tegra_clk_ndflash                 suspended                  0
>     /devices/platform/tegra_clk_sata_oob                suspended                  0
>     /devices/platform/tegra_clk_sata                    suspended                  0
>     /devices/platform/tegra_clk_fuse_burn               suspended                  0
>     /devices/platform/tegra_clk_usbd                    active                     1000000
>     /devices/platform/tegra_clk_usb2                    suspended                  0
>     /devices/platform/tegra_clk_usb3                    suspended                  0
>     /devices/soc0/50000000.host1x/54200000.dc           active                     1000000
>     /devices/soc0/50000000.host1x/54240000.dc           suspended                  0

Okay, to not stall things from moving forward, may I suggest that you
simply drop the call to lockdep_set_class() (and the corresponding
comment) for now.

Then you can continue to post the next parts - and if it turns out
that lockdep_set_class() becomes needed, you can always add it back
then.

Kind regards
Uffe

Re: [PATCH v2 13/14] soc/tegra: pmc: Add core power domain

[Dmitry Osipenko]

01.06.2021 13:19, Ulf Hansson пишет:
...
>> This is not sufficient for Tegra because we have individual OPP tables for the root PLLs, system clocks and device clocks. The device clocks could be muxed to a different PLLs, depending on clk requirements of a particular board.
> 
> Are you saying that the clock providers for the "root PLLs" and
> "system clocks" have OPP tables themselves? If so, would you mind
> posting a patch for an updated DT binding for these changes, so it can
> be discussed separately?

I will post all those patches soon, thank you.

...
>> The device drivers don't manage the parent clocks directly and OPP core doesn't support this use-case where OPP needs to be applied to a generic/parent PLL clock. Moving the OPP management to the clk driver is the easy solution which works good in practice for Tegra, it also removes a need to switch each driver to dev_pm_opp_set_rate() usage.
> 
> I admit, if clock consumer drivers could avoid calling
> dev_pm_opp_set_rate|opp(), that would be nice. But, as I stated, it's
> a fragile path from locking point of view, to call
> dev_pm_opp_set_rate|opp() from a clock provider driver. Personally, I
> think it's better to avoid it.
> 
> More importantly, you also need to convince the clock subsystem
> maintainers, that setting an OPP internally from the clock provider
> driver is a good idea. As far as I can tell, they have said *no* to
> this, since the common clock framework was invented, I believe for
> good reasons.

Pushing the OPP into a CCF driver is indeed not ideal. I'm open to new
ideas. I will post those patches where we could discuss this in a more
details.

...
>> For example please see clock@60006000 and pmc@7000e400 nodes of [1].
>>
>> [1] https://github.com/grate-driver/linux/blob/master/arch/arm/boot/dts/tegra30.dtsi
> 
> Thanks, that certainly helped me understand better!
> 
> I see that you want to add OPP tables to clock provider nodes. As I
> said above, an updated DT binding is probably a good idea to discuss
> separately.
...
> 
> Okay, to not stall things from moving forward, may I suggest that you
> simply drop the call to lockdep_set_class() (and the corresponding
> comment) for now.
> 
> Then you can continue to post the next parts - and if it turns out
> that lockdep_set_class() becomes needed, you can always add it back
> then.

Thank you very much for helping with reviewing this all. I'll drop the
lockdep_set_class() and post the v7 shortly. Afterwards, I'll send the
rest of clk, device-tree and etc related patches targeting 5.15.

Re: [PATCH v2 13/14] soc/tegra: pmc: Add core power domain

[Dmitry Osipenko]

24.05.2021 20:04, Ulf Hansson пишет:
>> +static void tegra_pmc_sync_state(struct device *dev)
>> +{
>> +       int err;
>> +
>> +       pmc->core_domain_state_synced = true;
>> +
>> +       /* this is a no-op if core regulator isn't used */
>> +       mutex_lock(&pmc->powergates_lock);
>> +       err = dev_pm_opp_sync_regulators(dev);
>> +       mutex_unlock(&pmc->powergates_lock);
>> +
>> +       if (err)
>> +               dev_err(dev, "failed to sync regulators: %d\n", err);
>> +}
>> +
> Nitpick.
> 
> Would you mind splitting the "sync_state" thingy out into a separate
> patch on top of $subject patch?
> 
> I think it would be nice, especially since it shares a function via
> include/soc/tegra/common.h - that would make it clear to what part
> that belongs to.
> 

I'll split it in v3. Thank you for the review.

[PATCH v2 14/14] soc/tegra: regulators: Support core domain state syncing

[Dmitry Osipenko]

The core voltage shall not drop until state of core domain is synced,
i.e. all device drivers that use core domain are loaded and ready.

Support core domain state syncing. The core domain driver invokes the
core-regulator voltage syncing once the state of domain is synced, at
this point the core voltage is allowed to go lower than the level left
after bootloader.

Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/regulators-tegra20.c | 19 ++++++++++++++++++-
 drivers/soc/tegra/regulators-tegra30.c | 18 +++++++++++++++++-
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/tegra/regulators-tegra20.c b/drivers/soc/tegra/regulators-tegra20.c
index 35335c6a20b8..f79835640005 100644
--- a/drivers/soc/tegra/regulators-tegra20.c
+++ b/drivers/soc/tegra/regulators-tegra20.c
@@ -17,6 +17,8 @@
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 
+#include <soc/tegra/common.h>
+
 struct tegra_regulator_coupler {
 	struct regulator_coupler coupler;
 	struct regulator_dev *core_rdev;
@@ -42,6 +44,21 @@ static int tegra20_core_limit(struct tegra_regulator_coupler *tegra,
 	int core_cur_uV;
 	int err;
 
+	/*
+	 * Tegra20 SoC has critical DVFS-capable devices that are
+	 * permanently-active or active at a boot time, like EMC
+	 * (DRAM controller) or Display controller for example.
+	 *
+	 * The voltage of a CORE SoC power domain shall not be dropped below
+	 * a minimum level, which is determined by device's clock rate.
+	 * This means that we can't fully allow CORE voltage scaling until
+	 * the state of all DVFS-critical CORE devices is synced.
+	 */
+	if (tegra_soc_core_domain_state_synced() && !tegra->sys_reboot_mode) {
+		pr_info_once("voltage state synced\n");
+		return 0;
+	}
+
 	if (tegra->core_min_uV > 0)
 		return tegra->core_min_uV;
 
@@ -62,7 +79,7 @@ static int tegra20_core_limit(struct tegra_regulator_coupler *tegra,
 	 */
 	tegra->core_min_uV = core_max_uV;
 
-	pr_info("core minimum voltage limited to %duV\n", tegra->core_min_uV);
+	pr_info("core voltage initialized to %duV\n", tegra->core_min_uV);
 
 	return tegra->core_min_uV;
 }
diff --git a/drivers/soc/tegra/regulators-tegra30.c b/drivers/soc/tegra/regulators-tegra30.c
index 6e4f3d9e7be1..e0203f78b396 100644
--- a/drivers/soc/tegra/regulators-tegra30.c
+++ b/drivers/soc/tegra/regulators-tegra30.c
@@ -17,6 +17,7 @@
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 
+#include <soc/tegra/common.h>
 #include <soc/tegra/fuse.h>
 
 struct tegra_regulator_coupler {
@@ -43,6 +44,21 @@ static int tegra30_core_limit(struct tegra_regulator_coupler *tegra,
 	int core_cur_uV;
 	int err;
 
+	/*
+	 * Tegra30 SoC has critical DVFS-capable devices that are
+	 * permanently-active or active at a boot time, like EMC
+	 * (DRAM controller) or Display controller for example.
+	 *
+	 * The voltage of a CORE SoC power domain shall not be dropped below
+	 * a minimum level, which is determined by device's clock rate.
+	 * This means that we can't fully allow CORE voltage scaling until
+	 * the state of all DVFS-critical CORE devices is synced.
+	 */
+	if (tegra_soc_core_domain_state_synced() && !tegra->sys_reboot_mode) {
+		pr_info_once("voltage state synced\n");
+		return 0;
+	}
+
 	if (tegra->core_min_uV > 0)
 		return tegra->core_min_uV;
 
@@ -63,7 +79,7 @@ static int tegra30_core_limit(struct tegra_regulator_coupler *tegra,
 	 */
 	tegra->core_min_uV = core_max_uV;
 
-	pr_info("core minimum voltage limited to %duV\n", tegra->core_min_uV);
+	pr_info("core voltage initialized to %duV\n", tegra->core_min_uV);
 
 	return tegra->core_min_uV;
 }
-- 
2.30.2