Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support

From: Sowjanya Komatineni
Date: Fri Aug 02 2019 - 16:37:56 EST


On 8/2/19 1:20 PM, Dmitry Osipenko wrote:
02.08.2019 21:43, Sowjanya Komatineni ÐÐÑÐÑ:
On 8/2/19 5:32 AM, Dmitry Osipenko wrote:
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);
This could be BIT(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;
The enb_ctx/rst_ctx are booleans, while you assigning an integer value
here. You're getting away here because bool is an 32bit unsigned int,
but you shouldn't rely on it and always explicitly convert to a bool.

+ÂÂÂ 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);
Will be better to read out and compare the hardware's state with the
restored one, then bail out if the state is unchanged.

Shouldn't it be fence_udelay()?

+ÂÂÂ if (!fixed->rst_ctx) {
+ÂÂÂÂÂÂÂ udelay(5); /* reset propogation delay */
Why delaying is done before the writing to the reset register?
During SC7 exit, peripheral reset state is set to POR state. So some
peripherals will already be in reset state and making sure of
propagation delay before releasing from reset.

It should be rst_clr_reg. will fix in next rev

+ÂÂÂÂÂÂÂ writel_relaxed(mask, fixed->base + fixed->regs->rst_reg);
I'm not quite sure what's going on here, this looks wrong.

1. rst_reg points to RST_DEVICES_x
2. Each bit of RST_DEVICES_x represents the reset-assertion state of
each individual device
3. By writing to rst_reg, all (!) devices are deasserted, except the one
device which corresponds to the mask
4. The reset is asserted for a single device, while !fixed->rst_ctx
means that it actually should be deasserted (?)

Apparently you should use rst_set_reg / rst_clr_reg.
Yes, It should be rst_clr_reg. will fix in next rev
+ÂÂÂ }
What about the case where rst_ctx=true?
ON SC7 exit, state of RST_DEV will be POR state where most peripherals
will already be in reset state.

Few of them which are not in reset state in POR values are those that
need to stay de-asserted across the boot states anyway.
Okay, sounds reasonable.

BTW, it would be nice if you could add a brief clarifying comment to the
code for each of the questions asked during of the review.
OK, Will add comments in code ...