Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni
Date: Fri Aug 02 2019 - 19:52:24 EST
On 8/2/19 2:15 PM, Dmitry Osipenko wrote:
02.08.2019 23:32, Sowjanya Komatineni ÐÐÑÐÑ:
On 8/2/19 1:17 PM, Dmitry Osipenko wrote:
02.08.2019 23:13, Dmitry Osipenko ÐÐÑÐÑ:
02.08.2019 21:33, Sowjanya Komatineni ÐÐÑÐÑ:
On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
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 mux
Just to be clear, clk_mux don't have cached parent. get_parent is by
register read. So, cant directly use get_parent and then set during restore.
So will create both clk_mux_save/restore_context in generic clk driver
and will invoke them during tegra peripheral clock suspend/resume.
Reg 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_ops
I'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 restore
Also, 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().
clk_periph_fixed_disable just disables clock only without deasserting
the corresponding peripheral.
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?
Yes, I'm suggesting to do a complete ungate/reset handling of the
devices in a separate function. All enabling/deassertion will be done in
a single hop, hence using 7us delay and four u32 words, which is much
nicer IMHO.
Actually six words, three for CLKs and three for RSTs.
OK, So with separate function doing complete register save/restore for
clk & rst, we can't do this thru clk_ops save/restore as clk_ops
save_restore happens per peripheral wise. So if we decide to do this,
then this should be invoked in clk-tegra210 driver suspend/resume.
Yes, per-clock save/restore should be used for setting rate and parent.
The ungating and resetting could be done separately to keep things cleaner.