Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1635216 > unrolled thread

Re: [PATCH] clk: Add functions to save and restore clock/dpll context en-masse

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] clk: Add functions to save and restore clock/dpll  context en-masse Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-05-03 21:00 +0200
    Re: [PATCH] clk: Add functions to save and restore clock/dpll context  en-masse Tero Kristo <t-kristo@ti.com> - 2017-05-04 11:00 +0200

#1635216 — Re: [PATCH] clk: Add functions to save and restore clock/dpll context en-masse

On Tue, Apr 18, 2017 at 10:42:49AM +0530, Keerthy wrote:
> From: Russ Dill <Russ.Dill@ti.com>
> 
> The clock/dpll registers are in the WKUP power domain. Under both RTC-only
> suspend and hibernation, these registers are lost. Hence save/restore
> them accordingly.

This looks like a huge hammer, and I think this will cause problems for
some systems, especially where some clocks are on remote devices which
need to be controlled via I2C buses.

The generic part walks all root clocks in the system, and all children
of those clocks.  If we have clocks on an I2C device, then we could
very well end up with a circular dependency - the I2C controller needs
its clocks restored in order for control of the clocks on the I2C device
to be accessible.

So, I think this needs more thought - clocks are not just about core SoC
clocks - and cracking this nut with such a big hammer is likely to cause
regressions all over the place.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [next] | [standalone]

#1635483 — Re: [PATCH] clk: Add functions to save and restore clock/dpll context en-masse

On 03/05/17 21:52, Russell King - ARM Linux wrote:
> On Tue, Apr 18, 2017 at 10:42:49AM +0530, Keerthy wrote:
>> From: Russ Dill <Russ.Dill@ti.com>
>>
>> The clock/dpll registers are in the WKUP power domain. Under both RTC-only
>> suspend and hibernation, these registers are lost. Hence save/restore
>> them accordingly.
>
> This looks like a huge hammer, and I think this will cause problems for
> some systems, especially where some clocks are on remote devices which
> need to be controlled via I2C buses.

Actually, if you look at the generic part of the code, it is not called 
from anywhere right now, but it just implements the support for 
save/restore. Also, for external clocks, no save/restore is implemented, 
and you would need to handle those separately anyway. Consider an 
external clock that is fed from SoC internal source; basically you need 
to disable this clock before going to the deep idle state, and enable it 
only after everything has been restored. If you keep it enabled over a 
deep idle, its source clock will potentially die causing issues.

>
> The generic part walks all root clocks in the system, and all children
> of those clocks.  If we have clocks on an I2C device, then we could
> very well end up with a circular dependency - the I2C controller needs
> its clocks restored in order for control of the clocks on the I2C device
> to be accessible.

For generic external clocks, I would not expect any save/restore is 
required at all, except for the potentially required enable/disable 
calls which would need to be handled either by some driver or SoC 
specific PM core.

> So, I think this needs more thought - clocks are not just about core SoC
> clocks - and cracking this nut with such a big hammer is likely to cause
> regressions all over the place.

This won't cause any regressions as it is not called by default from 
anywhere. A SoC needs to introduce the functionality for entering a deep 
idle state, and call the clk save/restore functionality from there.

-Tero

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web