[v1,07/30] soc/tegra: Add sync state API

Message ID 20201104234427.26477-8-digetx@gmail.com
State New
Headers show
Series
  • Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
Related show

Commit Message

Dmitry Osipenko Nov. 4, 2020, 11:44 p.m.
Introduce sync state API that will be used by Tegra device drivers. This
new API is primarily needed for syncing state of SoC devices that are left
ON after bootloader or permanently enabled. All these devices belong to a
shared CORE voltage domain, and thus, we needed to bring all the devices
into expected state before the voltage scaling could be performed.

All drivers of DVFS-critical devices shall sync theirs the state before
Tegra's voltage regulator coupler will be allowed to perform a system-wide
voltage scaling.

Tested-by: Peter Geis <pgwipeout@gmail.com>
Tested-by: Nicolas Chauvet <kwizart@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/common.c | 152 ++++++++++++++++++++++++++++++++++++-
 include/soc/tegra/common.h |  22 ++++++
 2 files changed, 170 insertions(+), 4 deletions(-)

Comments

Thierry Reding Nov. 10, 2020, 8:47 p.m. | #1
On Thu, Nov 05, 2020 at 02:44:04AM +0300, Dmitry Osipenko wrote:
> Introduce sync state API that will be used by Tegra device drivers. This
> new API is primarily needed for syncing state of SoC devices that are left
> ON after bootloader or permanently enabled. All these devices belong to a
> shared CORE voltage domain, and thus, we needed to bring all the devices
> into expected state before the voltage scaling could be performed.
> 
> All drivers of DVFS-critical devices shall sync theirs the state before
> Tegra's voltage regulator coupler will be allowed to perform a system-wide
> voltage scaling.
> 
> Tested-by: Peter Geis <pgwipeout@gmail.com>
> Tested-by: Nicolas Chauvet <kwizart@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/soc/tegra/common.c | 152 ++++++++++++++++++++++++++++++++++++-
>  include/soc/tegra/common.h |  22 ++++++
>  2 files changed, 170 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> index 3dc54f59cafe..f9b2b6f57887 100644
> --- a/drivers/soc/tegra/common.c
> +++ b/drivers/soc/tegra/common.c
> @@ -3,13 +3,52 @@
>   * Copyright (C) 2014 NVIDIA CORPORATION.  All rights reserved.
>   */
>  
> +#define dev_fmt(fmt)	"%s: " fmt, __func__
> +#define pr_fmt(fmt)	"%s: " fmt, __func__
> +
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/mutex.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  
>  #include <soc/tegra/common.h>
>  
> +#define terga_soc_for_each_device(__dev) \

tegra_soc_for_each_device

> +	for ((__dev) = tegra_soc_devices; (__dev) && (__dev)->compatible; \
> +	     (__dev)++)
> +
> +struct tegra_soc_device {
> +	const char *compatible;
> +	const bool dvfs_critical;
> +	unsigned int sync_count;
> +};
> +
> +static DEFINE_MUTEX(tegra_soc_lock);
> +static struct tegra_soc_device *tegra_soc_devices;
> +
> +/*
> + * DVFS-critical devices are either active at a boot time or permanently
> + * active, like EMC for example.  System-wide DVFS should be deferred until
> + * drivers of the critical devices synced theirs state.
> + */
> +
> +static struct tegra_soc_device tegra20_soc_devices[] = {
> +	{ .compatible = "nvidia,tegra20-dc", .dvfs_critical = true, },
> +	{ .compatible = "nvidia,tegra20-emc", .dvfs_critical = true, },
> +	{ }
> +};
> +
> +static struct tegra_soc_device tegra30_soc_devices[] = {
> +	{ .compatible = "nvidia,tegra30-dc", .dvfs_critical = true, },
> +	{ .compatible = "nvidia,tegra30-emc", .dvfs_critical = true, },
> +	{ .compatible = "nvidia,tegra30-pwm", .dvfs_critical = true, },
> +	{ }
> +};
> +
>  static const struct of_device_id tegra_machine_match[] = {
> -	{ .compatible = "nvidia,tegra20", },
> -	{ .compatible = "nvidia,tegra30", },
> +	{ .compatible = "nvidia,tegra20", .data = tegra20_soc_devices, },
> +	{ .compatible = "nvidia,tegra30", .data = tegra30_soc_devices, },
>  	{ .compatible = "nvidia,tegra114", },
>  	{ .compatible = "nvidia,tegra124", },
>  	{ .compatible = "nvidia,tegra132", },
> @@ -17,7 +56,7 @@ static const struct of_device_id tegra_machine_match[] = {
>  	{ }
>  };
>  
> -bool soc_is_tegra(void)
> +static const struct of_device_id *tegra_soc_of_match(void)
>  {
>  	const struct of_device_id *match;
>  	struct device_node *root;
> @@ -29,5 +68,110 @@ bool soc_is_tegra(void)
>  	match = of_match_node(tegra_machine_match, root);
>  	of_node_put(root);
>  
> -	return match != NULL;
> +	return match;
> +}
> +
> +bool soc_is_tegra(void)
> +{
> +	return tegra_soc_of_match() != NULL;
> +}
> +
> +void tegra_soc_device_sync_state(struct device *dev)
> +{
> +	struct tegra_soc_device *soc_dev;
> +
> +	mutex_lock(&tegra_soc_lock);
> +	terga_soc_for_each_device(soc_dev) {

tegra_soc_for_each_device

> +		if (!of_device_is_compatible(dev->of_node, soc_dev->compatible))
> +			continue;
> +
> +		if (!soc_dev->sync_count) {
> +			dev_err(dev, "already synced\n");
> +			break;
> +		}
> +
> +		/*
> +		 * All DVFS-capable devices should have the CORE regulator
> +		 * phandle.  Older device-trees don't have it, hence state
> +		 * won't be synced for the older DTBs, allowing them to work
> +		 * properly.
> +		 */
> +		if (soc_dev->dvfs_critical &&
> +		    !device_property_present(dev, "core-supply")) {
> +			dev_dbg(dev, "doesn't have core supply\n");
> +			break;
> +		}
> +
> +		soc_dev->sync_count--;
> +		dev_dbg(dev, "sync_count=%u\n", soc_dev->sync_count);
> +		break;
> +	}
> +	mutex_unlock(&tegra_soc_lock);
> +}
> +EXPORT_SYMBOL_GPL(tegra_soc_device_sync_state);
> +
> +bool tegra_soc_dvfs_state_synced(void)
> +{
> +	struct tegra_soc_device *soc_dev;
> +	bool synced_state = true;
> +
> +	/*
> +	 * CORE voltage scaling is limited until drivers of the critical
> +	 * devices synced theirs state.
> +	 */
> +	mutex_lock(&tegra_soc_lock);
> +	terga_soc_for_each_device(soc_dev) {

tegra_soc_for_each_device

I wonder if you copy/pasted this or if you got really lucky to mistype
this all three times.

> +		if (!soc_dev->sync_count || !soc_dev->dvfs_critical)
> +			continue;
> +
> +		pr_debug_ratelimited("%s: sync_count=%u\n",
> +				     soc_dev->compatible, soc_dev->sync_count);
> +
> +		synced_state = false;
> +		break;
> +	}
> +	mutex_unlock(&tegra_soc_lock);
> +
> +	return synced_state;
> +}
> +
> +static int __init tegra_soc_devices_init(void)
> +{
> +	struct device_node *np, *prev_np = NULL;
> +	struct tegra_soc_device *soc_dev;
> +	const struct of_device_id *match;
> +
> +	if (!soc_is_tegra())
> +		return 0;
> +
> +	match = tegra_soc_of_match();
> +	tegra_soc_devices = (void *)match->data;
> +
> +	/*
> +	 * If device node is disabled in a device-tree, then we shouldn't
> +	 * care about this device. Even if device is active during boot,
> +	 * its clock will be disabled by CCF as unused.
> +	 */
> +	terga_soc_for_each_device(soc_dev) {
> +		do {
> +			/*
> +			 * Devices like display controller have multiple
> +			 * instances with the same compatible. Hence we need
> +			 * to walk up the whole tree in order to account those
> +			 * multiple instances.
> +			 */
> +			np = of_find_compatible_node(prev_np, NULL,
> +						     soc_dev->compatible);
> +			of_node_put(prev_np);
> +			prev_np = np;
> +
> +			if (of_device_is_available(np)) {
> +				pr_debug("added %s\n", soc_dev->compatible);
> +				soc_dev->sync_count++;
> +			}
> +		} while (np);

Maybe use for_each_compatible_node() for that inside loop?

> +	}
> +
> +	return 0;
>  }
> +postcore_initcall_sync(tegra_soc_devices_init);

This is unfortunate. I recall having this discussion multiple times and
one idea that has been floating around for a while was to let a driver
bind against the top-level "bus" node. That has the advantage that it
both anchors the code somewhere, so we don't have to play this game of
checking for the SoC with soc_is_tegra(), and it properly orders this
with respect to the child devices, so we wouldn't have to make this a
postcore_initcall.

Might be worth looking at that again, but for now this seems okay.

Thierry
Dmitry Osipenko Nov. 10, 2020, 9:22 p.m. | #2
10.11.2020 23:47, Thierry Reding пишет:
...
> tegra_soc_for_each_device

> 

> I wonder if you copy/pasted this or if you got really lucky to mistype

> this all three times.


Copied of course :)

I added a special spell checking rule for this typo, but it does help
reliably.

...
>> +	terga_soc_for_each_device(soc_dev) {

>> +		do {

>> +			/*

>> +			 * Devices like display controller have multiple

>> +			 * instances with the same compatible. Hence we need

>> +			 * to walk up the whole tree in order to account those

>> +			 * multiple instances.

>> +			 */

>> +			np = of_find_compatible_node(prev_np, NULL,

>> +						     soc_dev->compatible);

>> +			of_node_put(prev_np);

>> +			prev_np = np;

>> +

>> +			if (of_device_is_available(np)) {

>> +				pr_debug("added %s\n", soc_dev->compatible);

>> +				soc_dev->sync_count++;

>> +			}

>> +		} while (np);

> 

> Maybe use for_each_compatible_node() for that inside loop?


Good point! I think there is actually an of_node_put() bug in current
variant, which for_each_compatible_node() would safe from.

>> +	}

>> +

>> +	return 0;

>>  }

>> +postcore_initcall_sync(tegra_soc_devices_init);

> 

> This is unfortunate. I recall having this discussion multiple times and

> one idea that has been floating around for a while was to let a driver

> bind against the top-level "bus" node. That has the advantage that it

> both anchors the code somewhere, so we don't have to play this game of

> checking for the SoC with soc_is_tegra(), and it properly orders this

> with respect to the child devices, so we wouldn't have to make this a

> postcore_initcall.

> 

> Might be worth looking at that again, but for now this seems okay.


Thanks
Dmitry Osipenko Nov. 10, 2020, 9:32 p.m. | #3
11.11.2020 00:22, Dmitry Osipenko пишет:
> I added a special spell checking rule for this typo, but it does help
> reliably.

does *not*

Patch

diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
index 3dc54f59cafe..f9b2b6f57887 100644
--- a/drivers/soc/tegra/common.c
+++ b/drivers/soc/tegra/common.c
@@ -3,13 +3,52 @@ 
  * Copyright (C) 2014 NVIDIA CORPORATION.  All rights reserved.
  */
 
+#define dev_fmt(fmt)	"%s: " fmt, __func__
+#define pr_fmt(fmt)	"%s: " fmt, __func__
+
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <soc/tegra/common.h>
 
+#define terga_soc_for_each_device(__dev) \
+	for ((__dev) = tegra_soc_devices; (__dev) && (__dev)->compatible; \
+	     (__dev)++)
+
+struct tegra_soc_device {
+	const char *compatible;
+	const bool dvfs_critical;
+	unsigned int sync_count;
+};
+
+static DEFINE_MUTEX(tegra_soc_lock);
+static struct tegra_soc_device *tegra_soc_devices;
+
+/*
+ * DVFS-critical devices are either active at a boot time or permanently
+ * active, like EMC for example.  System-wide DVFS should be deferred until
+ * drivers of the critical devices synced theirs state.
+ */
+
+static struct tegra_soc_device tegra20_soc_devices[] = {
+	{ .compatible = "nvidia,tegra20-dc", .dvfs_critical = true, },
+	{ .compatible = "nvidia,tegra20-emc", .dvfs_critical = true, },
+	{ }
+};
+
+static struct tegra_soc_device tegra30_soc_devices[] = {
+	{ .compatible = "nvidia,tegra30-dc", .dvfs_critical = true, },
+	{ .compatible = "nvidia,tegra30-emc", .dvfs_critical = true, },
+	{ .compatible = "nvidia,tegra30-pwm", .dvfs_critical = true, },
+	{ }
+};
+
 static const struct of_device_id tegra_machine_match[] = {
-	{ .compatible = "nvidia,tegra20", },
-	{ .compatible = "nvidia,tegra30", },
+	{ .compatible = "nvidia,tegra20", .data = tegra20_soc_devices, },
+	{ .compatible = "nvidia,tegra30", .data = tegra30_soc_devices, },
 	{ .compatible = "nvidia,tegra114", },
 	{ .compatible = "nvidia,tegra124", },
 	{ .compatible = "nvidia,tegra132", },
@@ -17,7 +56,7 @@  static const struct of_device_id tegra_machine_match[] = {
 	{ }
 };
 
-bool soc_is_tegra(void)
+static const struct of_device_id *tegra_soc_of_match(void)
 {
 	const struct of_device_id *match;
 	struct device_node *root;
@@ -29,5 +68,110 @@  bool soc_is_tegra(void)
 	match = of_match_node(tegra_machine_match, root);
 	of_node_put(root);
 
-	return match != NULL;
+	return match;
+}
+
+bool soc_is_tegra(void)
+{
+	return tegra_soc_of_match() != NULL;
+}
+
+void tegra_soc_device_sync_state(struct device *dev)
+{
+	struct tegra_soc_device *soc_dev;
+
+	mutex_lock(&tegra_soc_lock);
+	terga_soc_for_each_device(soc_dev) {
+		if (!of_device_is_compatible(dev->of_node, soc_dev->compatible))
+			continue;
+
+		if (!soc_dev->sync_count) {
+			dev_err(dev, "already synced\n");
+			break;
+		}
+
+		/*
+		 * All DVFS-capable devices should have the CORE regulator
+		 * phandle.  Older device-trees don't have it, hence state
+		 * won't be synced for the older DTBs, allowing them to work
+		 * properly.
+		 */
+		if (soc_dev->dvfs_critical &&
+		    !device_property_present(dev, "core-supply")) {
+			dev_dbg(dev, "doesn't have core supply\n");
+			break;
+		}
+
+		soc_dev->sync_count--;
+		dev_dbg(dev, "sync_count=%u\n", soc_dev->sync_count);
+		break;
+	}
+	mutex_unlock(&tegra_soc_lock);
+}
+EXPORT_SYMBOL_GPL(tegra_soc_device_sync_state);
+
+bool tegra_soc_dvfs_state_synced(void)
+{
+	struct tegra_soc_device *soc_dev;
+	bool synced_state = true;
+
+	/*
+	 * CORE voltage scaling is limited until drivers of the critical
+	 * devices synced theirs state.
+	 */
+	mutex_lock(&tegra_soc_lock);
+	terga_soc_for_each_device(soc_dev) {
+		if (!soc_dev->sync_count || !soc_dev->dvfs_critical)
+			continue;
+
+		pr_debug_ratelimited("%s: sync_count=%u\n",
+				     soc_dev->compatible, soc_dev->sync_count);
+
+		synced_state = false;
+		break;
+	}
+	mutex_unlock(&tegra_soc_lock);
+
+	return synced_state;
+}
+
+static int __init tegra_soc_devices_init(void)
+{
+	struct device_node *np, *prev_np = NULL;
+	struct tegra_soc_device *soc_dev;
+	const struct of_device_id *match;
+
+	if (!soc_is_tegra())
+		return 0;
+
+	match = tegra_soc_of_match();
+	tegra_soc_devices = (void *)match->data;
+
+	/*
+	 * If device node is disabled in a device-tree, then we shouldn't
+	 * care about this device. Even if device is active during boot,
+	 * its clock will be disabled by CCF as unused.
+	 */
+	terga_soc_for_each_device(soc_dev) {
+		do {
+			/*
+			 * Devices like display controller have multiple
+			 * instances with the same compatible. Hence we need
+			 * to walk up the whole tree in order to account those
+			 * multiple instances.
+			 */
+			np = of_find_compatible_node(prev_np, NULL,
+						     soc_dev->compatible);
+			of_node_put(prev_np);
+			prev_np = np;
+
+			if (of_device_is_available(np)) {
+				pr_debug("added %s\n", soc_dev->compatible);
+				soc_dev->sync_count++;
+			}
+		} while (np);
+	}
+
+	return 0;
 }
+postcore_initcall_sync(tegra_soc_devices_init);
diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h
index 98027a76ce3d..d3ddb96d0fe2 100644
--- a/include/soc/tegra/common.h
+++ b/include/soc/tegra/common.h
@@ -6,6 +6,28 @@ 
 #ifndef __SOC_TEGRA_COMMON_H__
 #define __SOC_TEGRA_COMMON_H__
 
+#include <linux/types.h>
+
+struct device;
+
+#ifdef CONFIG_ARCH_TEGRA
 bool soc_is_tegra(void);
+void tegra_soc_device_sync_state(struct device *dev);
+bool tegra_soc_dvfs_state_synced(void);
+#else
+static inline bool soc_is_tegra(void)
+{
+	return false;
+}
+
+static inline void tegra_soc_device_sync_state(struct device *dev)
+{
+}
+
+static inline tegra_soc_dvfs_state_synced(void)
+{
+	return false;
+}
+#endif
 
 #endif /* __SOC_TEGRA_COMMON_H__ */