Re: [PATCH V4 13/16] soc: tegra: pmc: Add generic PM domain support

[Lucas Stach]

Am Freitag, den 15.01.2016, 09:42 +0000 schrieb Jon Hunter:
> On 14/01/16 14:39, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Fri, Dec 04, 2015 at 02:57:14PM +0000, Jon Hunter wrote:
> > [...]
> >> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> > [...]
> >> +static int tegra_powergate_power_down(struct tegra_powergate *pg,
> >> +				      bool enable_clocks)
> >> +{
> >> +	int err;
> >> +
> >> +	if (enable_clocks) {
> >> +		err = tegra_powergate_enable_clocks(pg);
> >> +		if (err)
> >> +			return err;
> >> +
> >> +		usleep_range(10, 20);
> >> +	}
> >> +
> >> +	err = tegra_powergate_reset_assert(pg);
> >> +	if (err)
> >> +		goto err_reset;
> >> +
> >> +	usleep_range(10, 20);
> >> +
> >> +	tegra_powergate_disable_clocks(pg);
> > 
> > There's no guarantee that all clocks are actually disabled at this
> > point. Will the power down and subsequent power up sequences still
> > work properly in such cases? If not perhaps we should add some way
> > of checking for that case here (WARN_ON?) to make sure we can fix
> > up all drivers to release their clock enable references.
> 
> The problem is that there is no easy way to check the status of a clock
> and whether it is enabled. Yes clk-provider.h does provide a
> __clk_is_enabled() API but I don't think that this is meant to be used
> here. May be we need a clk API for disabling a clock that will WARN if
> the clock is not disabled?
> 
I can't find any hint in the TRM that disabling the clocks is required
for clamping/powergating.

All occurrences of those clock requirements are in regard to a
synchronous reset propagation. So there is no requirement that the
clocks need to be _disabled_ at a certain time, but actually they just
need to be _enabled_ whenever the PMC is trying to drive the resets in
any way.

Obviously the power savings might be higher when the peripheral drivers
properly disable any unused clocks in suspend, but that can be solved on
a case by case basis I think.

Regards,
Lucas

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |