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

Ulf Hansson
2016-03-08T21:28:54Z

On 4 March 2016 at 13:23, Jon Hunter <jonathanh@nvidia.com> wrote:
> 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 <treding@nvidia.com>
> and Vince Hsu <vinceh@nvidia.com>.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> 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?

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

[...]

Kind regards
Uffe