On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
clk_periph_fixed_disable just disables clock only without deasserting the corresponding peripheral.02.08.2019 2:49, Sowjanya Komatineni пишет:On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:On 8/1/19 1:54 PM, Dmitry Osipenko wrote:01.08.2019 23:31, Sowjanya Komatineni пишет:On 8/1/19 1:17 PM, Dmitry Osipenko wrote:01.08.2019 22:42, Sowjanya Komatineni пишет:On 8/1/19 12:00 PM, Dmitry Osipenko wrote:01.08.2019 20:58, Sowjanya Komatineni пишет:On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:On 7/31/19 3:44 AM, Dmitry Osipenko wrote:31.07.2019 12:50, Dmitry Osipenko пишет:31.07.2019 3:20, Sowjanya Komatineni пишет:This patch implements save and restore context for peripheral fixed clock ops, peripheral gate clock ops, sdmmc mux clock ops, and peripheral clock ops. During system suspend, core power goes off and looses the settings of the Tegra CAR controller registers. So during suspend entry clock and reset state of peripherals is saved and on resume they are restored to have clocks back to same rate and state as before suspend. Acked-by: Thierry Reding <treding@xxxxxxxxxx> Signed-off-by: Sowjanya Komatineni <skomatineni@xxxxxxxxxx> --- drivers/clk/tegra/clk-periph-fixed.c | 33 ++++++++++++++++++++++++++++++++ drivers/clk/tegra/clk-periph-gate.c | 34 +++++++++++++++++++++++++++++++++ drivers/clk/tegra/clk-periph.c | 37 ++++++++++++++++++++++++++++++++++++ drivers/clk/tegra/clk-sdmmc-mux.c | 28 +++++++++++++++++++++++++++ drivers/clk/tegra/clk.h | 6 ++++++ 5 files changed, 138 insertions(+) diff --git a/drivers/clk/tegra/clk-periph-fixed.c b/drivers/clk/tegra/clk-periph-fixed.c index c088e7a280df..21b24530fa00 100644 --- a/drivers/clk/tegra/clk-periph-fixed.c +++ b/drivers/clk/tegra/clk-periph-fixed.c @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct clk_hw *hw, return (unsigned long)rate; } +static int tegra_clk_periph_fixed_save_context(struct clk_hw *hw) +{ + struct tegra_clk_periph_fixed *fixed = to_tegra_clk_periph_fixed(hw); + u32 mask = 1 << (fixed->num % 32); + + fixed->enb_ctx = readl_relaxed(fixed->base + fixed->regs->enb_reg) & + mask; + fixed->rst_ctx = readl_relaxed(fixed->base + fixed->regs->rst_reg) & + mask; + + return 0; +} + +static void tegra_clk_periph_fixed_restore_context(struct clk_hw *hw) +{ + struct tegra_clk_periph_fixed *fixed = to_tegra_clk_periph_fixed(hw); + u32 mask = 1 << (fixed->num % 32); + + if (fixed->enb_ctx) + writel_relaxed(mask, fixed->base + fixed->regs->enb_set_reg); + else + writel_relaxed(mask, fixed->base + fixed->regs->enb_clr_reg); + + udelay(2); + + if (!fixed->rst_ctx) { + udelay(5); /* reset propogation delay */ + writel_relaxed(mask, fixed->base + fixed->regs->rst_reg); + } +} + static const struct clk_ops tegra_clk_periph_fixed_ops = { .is_enabled = tegra_clk_periph_fixed_is_enabled, .enable = tegra_clk_periph_fixed_enable, .disable = tegra_clk_periph_fixed_disable, .recalc_rate = tegra_clk_periph_fixed_recalc_rate, + .save_context = tegra_clk_periph_fixed_save_context, + .restore_context = tegra_clk_periph_fixed_restore_context, }; struct clk *tegra_clk_register_periph_fixed(const char *name, diff --git a/drivers/clk/tegra/clk-periph-gate.c b/drivers/clk/tegra/clk-periph-gate.c index 4b31beefc9fc..6ba5b08e0787 100644 --- a/drivers/clk/tegra/clk-periph-gate.c +++ b/drivers/clk/tegra/clk-periph-gate.c @@ -25,6 +25,8 @@ static DEFINE_SPINLOCK(periph_ref_lock); #define read_rst(gate) \ readl_relaxed(gate->clk_base + (gate->regs->rst_reg)) +#define write_rst_set(val, gate) \ + writel_relaxed(val, gate->clk_base + (gate->regs->rst_set_reg)) #define write_rst_clr(val, gate) \ writel_relaxed(val, gate->clk_base + (gate->regs->rst_clr_reg)) @@ -110,10 +112,42 @@ static void clk_periph_disable(struct clk_hw *hw) spin_unlock_irqrestore(&periph_ref_lock, flags); } +static int clk_periph_gate_save_context(struct clk_hw *hw) +{ + struct tegra_clk_periph_gate *gate = to_clk_periph_gate(hw); + + gate->clk_state_ctx = read_enb(gate) & periph_clk_to_bit(gate); + gate->rst_state_ctx = read_rst(gate) & periph_clk_to_bit(gate); + + return 0; +} + +static void clk_periph_gate_restore_context(struct clk_hw *hw) +{ + struct tegra_clk_periph_gate *gate = to_clk_periph_gate(hw); + + if (gate->clk_state_ctx) + write_enb_set(periph_clk_to_bit(gate), gate); + else + write_enb_clr(periph_clk_to_bit(gate), gate); + + udelay(5); + + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) && + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) { + if (gate->rst_state_ctx) + write_rst_set(periph_clk_to_bit(gate), gate); + else + write_rst_clr(periph_clk_to_bit(gate), gate); + } +} + const struct clk_ops tegra_clk_periph_gate_ops = { .is_enabled = clk_periph_is_enabled, .enable = clk_periph_enable, .disable = clk_periph_disable, + .save_context = clk_periph_gate_save_context, + .restore_context = clk_periph_gate_restore_context, }; struct clk *tegra_clk_register_periph_gate(const char *name, diff --git a/drivers/clk/tegra/clk-periph.c b/drivers/clk/tegra/clk-periph.c index 58437da25156..06fb62955768 100644 --- a/drivers/clk/tegra/clk-periph.c +++ b/drivers/clk/tegra/clk-periph.c @@ -99,6 +99,37 @@ static void clk_periph_disable(struct clk_hw *hw) gate_ops->disable(gate_hw); } +static int clk_periph_save_context(struct clk_hw *hw) +{ + struct tegra_clk_periph *periph = to_clk_periph(hw); + const struct clk_ops *gate_ops = periph->gate_ops; + struct clk_hw *gate_hw = &periph->gate.hw; + + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE)) + gate_ops->save_context(gate_hw); + + periph->parent_ctx = clk_periph_get_parent(hw); + + return 0; +} + +static void clk_periph_restore_context(struct clk_hw *hw) +{ + struct tegra_clk_periph *periph = to_clk_periph(hw); + const struct clk_ops *gate_ops = periph->gate_ops; + struct clk_hw *gate_hw = &periph->gate.hw; + const struct clk_ops *div_ops = periph->div_ops; + struct clk_hw *div_hw = &periph->divider.hw; + + clk_periph_set_parent(hw, periph->parent_ctx); + + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV)) + div_ops->restore_context(div_hw);Could you please point to where the divider's save_context() happens? Because I can't see it.Ah, I now see that there is no need to save the dividers context because clk itself has enough info that is needed for the context's restoring (like I pointed in the review to v6). Looks like you could also implement a new clk_hw_get_parent_index() generic helper to get the index instead of storing it manually.clk_periph_get_parent basically invokes existing clk_mux_ops get_parent() which is then saved in tegra_clk_periph. All existing drivers are using directly get_parent() from clk_mux which actually gets index from the register read. To have this more generic w.r.t save/restore context point of view, probably instead of implementing new get_parent_index helper, I think its better to implement save_context and restore_context to clk_mux_ops along with creating parent_index field into clk_mux to cache index during set_parent. So we just need to invoke mux_ops save_context and restore_context.I hope its ok to add save/restore context to clk_mux_ops to be more generic w.r.t save/restore context rather than get_parent_index API. Please confirm if you agree.Sounds like a good idea. I see that there is a 'restoring' helper for the generic clk_gate, seems something similar could be done for the clk_mux. And looks like anyway you'll need to associate the parent clock with the hw index in order to restore the muxing.by 'restoring' helper for generic clk_gate, are you referring to clk_gate_restore_context API?Yes.clk_gate_restore_context is API that's any clk drivers can use for clk_gate operation restore for custom gate clk_ops. But clk-periph is directly using generic clk_mux ops from clk_mux so I think we should add .restore_context to clk_mux_ops and then during clk-periph restore need to invoke mux_ops->restore_context.I'm not sure whether it will be good for every driver that uses generic clk_mux ops. Should be more flexible to have a generic helper function that any driver could use in order to restore the clock's parent. The clk-periph restoring also includes case of combining divider and parent restoring, so generic helper could be useful in that case as well. It also looks like you could actually use the clk_gate_restore_context() instead of manually saving the clock's enable-state, couldn't you?ok for clk_mux, can add generic clk_mux_restore_context API rather than using restore_context in clk_ops and will invoke that during clk_periph restore.digging thru looks like for clk_periph source restore instead of clk_mux_restore_context, i can directly do clk_hw_get_parent and clk_set_parent with mux_hw as they invoke mux_ops get/set parent anyway. Will do this for periph clk muxReg clk_gate, looks like we cant use generic clk_gate_restore_context for clk-periph as it calls enable/disable callbacks and clk_periph_enable/disable in clk-periph-gate also updated refcnt and depending on that actual enable/disable is set. During suspend, peripherals that are already enabled have their refcnt > 1, so they dont go thru enable/disable on restore if we use same enable/disable callback.Looks like you could just decrement the gate's enable_refcnt on save_context, wouldn't that work?gate->enable_refcnt is within clk-periph-gate which gets updated when enable/disable callbacks get execute thru clk_core_enable/disable. But actual enable_count used in clk_gate_restore_context is the one which gets updated with in the clk core enable/disable functions which invokes these callbacks. Depending on this enable_count in clk core it invokes enable/disable. So, this will cause mismatch if we handle refcnt during save/restore of tegra_clk_periph_gate_ops and also enable/disable thru this clk_gate_restore_context is based on enable_count from clk core.Also to align exact reset state along with CLK (like for case where CLK is enabled but peripheral might be in reset state), implemented save/restore in tegra specific tegra_clk_periph_gate_opsI'm wondering whether instead of saving/restoring reset-state of every clock, you could simply save/restore the whole RST_DEV_x_SET register. Couldn't you?Thats what I was doing in first version of patch. But later as we moved to use clk_save_context and clk_restore_context, peripheral clk_hw RST & CLK enables happen thru its corresponding save/restore after source restoreAlso, to align both CLK & RST to the exact state of register, doing save/restore in tegra_clk_periph_gate_ops and invoking this after source restore for peripheral clock, seems cleaner to avoid any misconfiguration b/w rst & clk settings.It looks to me that it is very wasteful to store/restore each individual gate and reset state, also given that some of them are shared. I think that the gates and resets should be restored separately for the peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
corresponding peripheral drivers can also issue reset assert/deassert thru reset_control_assert/deassert.
So, we will not get the actual state of clk and rst unless we read and save state of reset and clock separately during save_context.
Currently patch is already using custom tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops. Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV register settings instead of individual peripheral bits?
