Message-ID: <20201110204727.GG2375022@ulmo>
Date:   Tue, 10 Nov 2020 21:47:27 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Alan Stern <stern@...land.harvard.edu>,
        Peter Chen <Peter.Chen@....com>,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Lee Jones <lee.jones@...aro.org>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Peter Geis <pgwipeout@...il.com>,
        Nicolas Chauvet <kwizart@...il.com>,
        linux-samsung-soc@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-usb@...r.kernel.org, linux-pwm@...r.kernel.org,
        linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-media@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v1 07/30] soc/tegra: Add sync state API

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@...il.com>
> Tested-by: Nicolas Chauvet <kwizart@...il.com>
> Signed-off-by: Dmitry Osipenko <digetx@...il.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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists