On 03/04/2013 04:40 AM, Joseph Lo wrote:
> Adding suspend to ram support for Tegra platform. There are three suspend
nit: s/ram/RAM/
> mode for Tegra. The difference were below.
Does this patch work for all 3 upstream Tegras, or just Tegra20 (or 20+30?).
> diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
> + tegra_init_suspend();
That call isn't ifdef'd on anything ...
> diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
> +static void tegra_suspend_enter_lp2(void)
> +{
> + tegra_set_cpu_in_lp2(0);
> +}
> +
> +static void tegra_suspend_exit_lp2(void)
> +{
> + tegra_clear_cpu_in_lp2(0);
> +}
Are those two functions going to get larger? If not, why not just call
tegra_{set,clear}_cpu_in_lp2() from tegra_suspend_enter()?
Note: I realized that the LP0/LP1 functions might be larger, and feel
free to make them separate functions when they're implemented, but if
those two functions specifically are always going to be a single line,
then there's little point in separating them out, is there?
> +void __init tegra_init_suspend(void)
...
> #endif
... but this function is only implemented inside an ifdef ...
> diff --git a/arch/arm/mach-tegra/pm.h b/arch/arm/mach-tegra/pm.h
> +void tegra_init_suspend(void);
... and no dummy inline is provided if the ifdef isn't defined.
So, turning off whatever that ifdef is would cause a build error.
> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> +unsigned long tegra_pmc_get_cpu_timer(void)
...
> +unsigned long tegra_pmc_get_cpu_off_timer(void)
I think you want to make those return u32, since the values they return
should be u32 to match DT cell type.
> +void tegra_pmc_pm_set(enum tegra_suspend_mode mode)
> +{
> + u32 reg;
> +
> + reg = tegra_pmc_readl(PMC_CTRL);
> + reg |= TEGRA_POWER_CPU_PWRREQ_OE;
> + reg &= ~TEGRA_POWER_EFFECT_LP0;
> +
> + tegra_pmc_writel(reg, PMC_CTRL);
> +}
The "mode" parameter doesn't seem to be used here.
One overall comment: tegra_suspend_enter() only supports LP2 so far, but
I don't see a check anywhere (e.g. tegra_init_suspend() or
tegra_pmc_parse_dt()) which prevents suspend from being initialized if
the user requested some other type, or on SoCs where even LP2 isn't
supported yet (e.g. Tegra114).
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
