Re: [PATCH V7 3/4] soc: tegra: pmc: Add generic PM domain support

Hi Uffe,

On 08/03/16 21:28, Ulf Hansson wrote:

On 4 March 2016 at 13:23, Jon Hunter [off-list ref] wrote:

quoted

Adds generic PM support to the PMC driver where the PM domains are
populated from device-tree and the PM domain consumer devices are
bound to their relevant PM domains via device-tree as well.

Update the tegra_powergate_sequence_power_up() API so that internally
it calls the same tegra_powergate_xxx functions that are used by the
tegra generic power domain code for consistency.

To ensure that the Tegra power domains (a.k.a powergates) cannot be
controlled via both the legacy tegra_powergate_xxx functions as well
as the generic PM domain framework, add a bit map for available
powergates that can be controlled via the legacy powergate functions.

Move the majority of the tegra_powergate_remove_clamping() function
to a sub-function, so that this can be used by both the legacy and
generic power domain code.

Currently, the power domains are not removed once added because this
is not yet supported by the PM domains framework. An error message
will be displayed if we are unable to instantiate a power domain.

This is based upon work by Thierry Reding [off-list ref]
and Vince Hsu [off-list ref].

Signed-off-by: Jon Hunter <redacted>
---
 drivers/soc/tegra/pmc.c | 485 ++++++++++++++++++++++++++++++++++++++++++------
 include/soc/tegra/pmc.h |   1 +
 2 files changed, 425 insertions(+), 61 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 08966c26d65c..bb173456bbff 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c

@@ -31,10 +31,13 @@
 #include <linux/iopoll.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/reboot.h>
 #include <linux/reset.h>
 #include <linux/seq_file.h>
+#include <linux/slab.h>
 #include <linux/spinlock.h>

 #include <soc/tegra/common.h>

@@ -102,6 +105,16 @@

 #define GPU_RG_CNTRL                   0x2d4

+struct tegra_powergate {
+       struct generic_pm_domain genpd;
+       struct tegra_pmc *pmc;
+       unsigned int id;
+       struct clk **clks;
+       unsigned int num_clks;
+       struct reset_control **resets;
+       unsigned int num_resets;
+};
+
 struct tegra_pmc_soc {
        unsigned int num_powergates;
        const char *const *powergates;

@@ -132,6 +145,7 @@ struct tegra_pmc_soc {
  * @cpu_pwr_good_en: CPU power good signal is enabled
  * @lp0_vec_phys: physical base address of the LP0 warm boot code
  * @lp0_vec_size: size of the LP0 warm boot code
+ * @powergates_available: Bitmap of available power gates
  * @powergates_lock: mutex for power gate register access
  */
 struct tegra_pmc {

@@ -156,6 +170,7 @@ struct tegra_pmc {
        bool cpu_pwr_good_en;
        u32 lp0_vec_phys;
        u32 lp0_vec_size;
+       DECLARE_BITMAP(powergates_available, TEGRA_POWERGATE_MAX);

        struct mutex powergates_lock;
 };

@@ -165,6 +180,12 @@ static struct tegra_pmc *pmc = &(struct tegra_pmc) {
        .suspend_mode = TEGRA_SUSPEND_NONE,
 };

+static inline struct tegra_powergate *
+to_powergate(struct generic_pm_domain *domain)
+{
+       return container_of(domain, struct tegra_powergate, genpd);
+}
+
 static u32 tegra_pmc_readl(unsigned long offset)
 {
        return readl(pmc->base + offset);

@@ -188,6 +209,31 @@ static inline bool tegra_powergate_is_valid(int id)
        return (pmc->soc && pmc->soc->powergates[id]);
 }

+static inline bool tegra_powergate_is_available(int id)
+{
+       return test_bit(id, pmc->powergates_available);
+}
+
+static int tegra_powergate_lookup(struct tegra_pmc *pmc, const char *name)
+{
+       unsigned int i;
+
+       if (!pmc || !pmc->soc || !name)
+               return -EINVAL;
+
+       for (i = 0; i < pmc->soc->num_powergates; i++) {
+               if (!tegra_powergate_is_valid(i))
+                       continue;
+
+               if (!strcmp(name, pmc->soc->powergates[i]))
+                       return i;

Instead of having this name based lookup, why not provide the id using
DT instead?

In other words use of_genpd_add_provider_onecell() when adding the OF
genpd provider. In that way I think you will get a bit simplier code
dealing with the error path when initialzing the genpds, won't you?

The above is simply used when creating the PM domains and not when
calling the xlate to lookup the PM domain (which I believe you are
referring to).

Originally, the DT node for the PM domain included an ID for the PM
domain, but when discussing with Thierry we thought there was no need to
have both a name and ID. So now the DT node for the PM domain look like ...

	powergates {
		pd_audio: aud {
			clocks = <&tegra_car TEGRA210_CLK_APE>,
			         <&tegra_car TEGRA210_CLK_APB2APE>;
			resets = <&tegra_car 198>;
			#power-domain-cells = <0>;
		};
	};

And the client binding ...

	adma: adma@702e2000 {
		...
		power-domains = <&pd_audio>;
		...
	};

Anyway, I guess it's more a matter of taste, so feel free to keep as is.

I prefer not to use of_genpd_add_provider_onecell() for tegra as this
appears to be used for devices with a static list of PM domains. Yes,
the Tegra PMC driver does have a static list of PM domain names (per
pmc->soc->powergates[i]), however, I prefer that all the clock and reset
information resides in the DT blob.

Cheers
Jon