Date:   Tue, 25 Apr 2017 11:30:54 +0530
From:   Keerthy <j-keerthy@...com>
To:     Tero Kristo <t-kristo@...com>, <sboyd@...eaurora.org>,
        <mturquette@...libre.com>
CC:     <linux@...linux.org.uk>, <linux-clk@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-omap@...r.kernel.org>,
        Russ Dill <Russ.Dill@...com>
Subject: Re: [PATCH] clk: Add functions to save and restore clock/dpll context
 en-masse



On Tuesday 18 April 2017 08:30 PM, Tero Kristo wrote:
> On 18/04/17 08:12, Keerthy wrote:
>> From: Russ Dill <Russ.Dill@...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.
>>
>> Signed-off-by: Russ Dill <Russ.Dill@...com>
>> Signed-off-by: Keerthy <j-keerthy@...com>
> 
> I think the core support should be provided in a separate patch, and the
> TI clock driver stuff added after that. And, for each clock driver,
> provide a separate patch for the context save/restore also.

Okay.

> 
> Some additional comments provided below.
> 
>> ---
>>  drivers/clk/clk.c            |  70 ++++++++++++++++++++++++
>>  drivers/clk/ti/divider.c     |  36 +++++++++++++
>>  drivers/clk/ti/dpll.c        |   6 +++
>>  drivers/clk/ti/dpll3xxx.c    | 124
>> +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/ti/gate.c        |   3 ++
>>  drivers/clk/ti/mux.c         |  29 ++++++++++
>>  include/linux/clk-provider.h |  11 ++++
>>  include/linux/clk.h          |  25 +++++++++
>>  include/linux/clk/ti.h       |   6 +++
>>  9 files changed, 310 insertions(+)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index cddddbe..1ca87f4 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -687,6 +687,76 @@ static int clk_core_enable_lock(struct clk_core
>> *core)
>>      return ret;
>>  }
>>
>> +void clk_dflt_restore_context(struct clk_hw *hw)
>> +{
>> +    if (hw->clk->core->enable_count)
>> +        hw->clk->core->ops->enable(hw);
>> +    else
>> +        hw->clk->core->ops->disable(hw);
>> +}
>> +EXPORT_SYMBOL_GPL(clk_dflt_restore_context);
> 
> Is the above really needed? I guess it is mainly for optimization
> purposes so that we don't need to read the HW state for every
> save/restore cycle (which makes it rather important as such...?)

Just added a return at the beginning.
Yes. I tried without this and i confirm it is not necessarily needed for
rtc+ddr restore. I will do some more testing on this.

> 
>> +
>> +static int clk_save_context(struct clk_core *clk)
>> +{
>> +    struct clk_core *child;
>> +    int ret = 0;
>> +
>> +    hlist_for_each_entry(child, &clk->children, child_node) {
>> +        ret = clk_save_context(child);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>> +
>> +    if (clk->ops && clk->ops->save_context)
>> +        ret = clk->ops->save_context(clk->hw);
>> +
>> +    return ret;
>> +}
>> +
>> +static void clk_restore_context(struct clk_core *clk)
>> +{
>> +    struct clk_core *child;
>> +
>> +    if (clk->ops && clk->ops->restore_context)
>> +        clk->ops->restore_context(clk->hw);
>> +
>> +    hlist_for_each_entry(child, &clk->children, child_node)
>> +        clk_restore_context(child);
>> +}
>> +
>> +int clks_save_context(void)
>> +{
>> +    struct clk_core *clk;
>> +    int ret;
>> +
>> +    hlist_for_each_entry(clk, &clk_root_list, child_node) {
>> +        ret = clk_save_context(clk);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>> +
>> +    hlist_for_each_entry(clk, &clk_orphan_list, child_node) {
>> +        ret = clk_save_context(clk);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(clks_save_context);
>> +
>> +void clks_restore_context(void)
>> +{
>> +    struct clk_core *clk;
>> +
>> +    hlist_for_each_entry(clk, &clk_root_list, child_node)
>> +        clk_restore_context(clk);
>> +
>> +    hlist_for_each_entry(clk, &clk_orphan_list, child_node)
>> +        clk_restore_context(clk);
>> +}
>> +EXPORT_SYMBOL_GPL(clks_restore_context);
>> +
>>  /**
>>   * clk_enable - ungate a clock
>>   * @clk: the clk being ungated
>> diff --git a/drivers/clk/ti/divider.c b/drivers/clk/ti/divider.c
>> index d6dcb28..350a58a 100644
>> --- a/drivers/clk/ti/divider.c
>> +++ b/drivers/clk/ti/divider.c
>> @@ -266,10 +266,46 @@ static int ti_clk_divider_set_rate(struct clk_hw
>> *hw, unsigned long rate,
>>      return 0;
>>  }
>>
>> +/**
>> + * clk_divider_save_context - Save the divider value
>> + * @hw: pointer  struct clk_hw
>> + *
>> + * Save the divider value
>> + */
>> +static int clk_divider_save_context(struct clk_hw *hw)
>> +{
>> +    struct clk_divider *divider = to_clk_divider(hw);
>> +    u32 val;
>> +
>> +    val = ti_clk_ll_ops->clk_readl(divider->reg) >> divider->shift;
>> +    divider->context = val & div_mask(divider);
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * clk_divider_restore_context - restore the saved the divider value
>> + * @hw: pointer  struct clk_hw
>> + *
>> + * Restore the saved the divider value
>> + */
>> +static void clk_divider_restore_context(struct clk_hw *hw)
>> +{
>> +    struct clk_divider *divider = to_clk_divider(hw);
>> +    u32 val;
>> +
>> +    val = ti_clk_ll_ops->clk_readl(divider->reg);
>> +    val &= ~(div_mask(divider) << divider->shift);
>> +    val |= divider->context << divider->shift;
>> +    ti_clk_ll_ops->clk_writel(val, divider->reg);
>> +}
>> +
>>  const struct clk_ops ti_clk_divider_ops = {
>>      .recalc_rate = ti_clk_divider_recalc_rate,
>>      .round_rate = ti_clk_divider_round_rate,
>>      .set_rate = ti_clk_divider_set_rate,
>> +    .save_context = clk_divider_save_context,
>> +    .restore_context = clk_divider_restore_context,
>>  };
>>
>>  static struct clk *_register_divider(struct device *dev, const char
>> *name,
>> diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
>> index 96d8488..791dd31 100644
>> --- a/drivers/clk/ti/dpll.c
>> +++ b/drivers/clk/ti/dpll.c
>> @@ -39,6 +39,8 @@
>>      .set_rate_and_parent    = &omap3_noncore_dpll_set_rate_and_parent,
>>      .determine_rate    = &omap4_dpll_regm4xen_determine_rate,
>>      .get_parent    = &omap2_init_dpll_parent,
>> +    .save_context    = &omap3_core_dpll_save_context,
>> +    .restore_context = &omap3_core_dpll_restore_context,
> 
> Is this part ever executed? The save/restore code is only used on am43xx
> right now for TI SoCs, and this piece of code is conditionally compiled
> out unless OMAP4+ is supported.

You are right! Just now tested rtc+ddr mode without this on
am437x-gp-evm and it works fine.

> 
>>  };
>>  #else
>>  static const struct clk_ops dpll_m4xen_ck_ops = {};
>> @@ -62,6 +64,8 @@
>>      .set_rate_and_parent    = &omap3_noncore_dpll_set_rate_and_parent,
>>      .determine_rate    = &omap3_noncore_dpll_determine_rate,
>>      .get_parent    = &omap2_init_dpll_parent,
>> +    .save_context    = &omap3_noncore_dpll_save_context,
>> +    .restore_context = &omap3_noncore_dpll_restore_context,
>>  };
>>
>>  static const struct clk_ops dpll_no_gate_ck_ops = {
>> @@ -72,6 +76,8 @@
>>      .set_parent    = &omap3_noncore_dpll_set_parent,
>>      .set_rate_and_parent    = &omap3_noncore_dpll_set_rate_and_parent,
>>      .determine_rate    = &omap3_noncore_dpll_determine_rate,
>> +    .save_context    = &omap3_noncore_dpll_save_context,
>> +    .restore_context = &omap3_noncore_dpll_restore_context
>>  };
>>  #else
>>  static const struct clk_ops dpll_core_ck_ops = {};
>> diff --git a/drivers/clk/ti/dpll3xxx.c b/drivers/clk/ti/dpll3xxx.c
>> index 4534de2..44b6b64 100644
>> --- a/drivers/clk/ti/dpll3xxx.c
>> +++ b/drivers/clk/ti/dpll3xxx.c
>> @@ -782,6 +782,130 @@ unsigned long omap3_clkoutx2_recalc(struct
>> clk_hw *hw,
>>      return rate;
>>  }
>>
>> +/**
>> + * omap3_core_dpll_save_context - Save the m and n values of the divider
>> + * @hw: pointer  struct clk_hw
>> + *
>> + * Before the dpll registers are lost save the last rounded rate m and n
>> + * and the enable mask.
>> + */
>> +int omap3_core_dpll_save_context(struct clk_hw *hw)
>> +{
> 
> Do we need this and the function below at all? Can you double check if
> it is ever executed?

As stated above not needed.

> 
>> +    struct clk_hw_omap *clk = to_clk_hw_omap(hw);
>> +    struct dpll_data *dd;
>> +    u32 v;
>> +
>> +    dd = clk->dpll_data;
>> +
>> +    v = ti_clk_ll_ops->clk_readl(&dd->control_reg);
>> +    clk->context = (v & dd->enable_mask) >> __ffs(dd->enable_mask);
>> +
>> +    if (clk->context == DPLL_LOCKED) {
>> +        v = ti_clk_ll_ops->clk_readl(&dd->mult_div1_reg);
>> +        dd->last_rounded_m = (v & dd->mult_mask) >>
>> +                        __ffs(dd->mult_mask);
>> +        dd->last_rounded_n = ((v & dd->div1_mask) >>
>> +                        __ffs(dd->div1_mask)) + 1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * omap3_core_dpll_restore_context - restore the m and n values of
>> the divider
>> + * @hw: pointer  struct clk_hw
>> + *
>> + * Restore the last rounded rate m and n
>> + * and the enable mask.
>> + */
>> +void omap3_core_dpll_restore_context(struct clk_hw *hw)
>> +{
>> +    struct clk_hw_omap *clk = to_clk_hw_omap(hw);
>> +    const struct dpll_data *dd;
>> +    u32 v;
>> +
>> +    dd = clk->dpll_data;
>> +
>> +    if (clk->context == DPLL_LOCKED) {
>> +        _omap3_dpll_write_clken(clk, 0x4);
>> +        _omap3_wait_dpll_status(clk, 0);
>> +
>> +        v = ti_clk_ll_ops->clk_readl(&dd->mult_div1_reg);
>> +        v &= ~(dd->mult_mask | dd->div1_mask);
>> +        v |= dd->last_rounded_m << __ffs(dd->mult_mask);
>> +        v |= (dd->last_rounded_n - 1) << __ffs(dd->div1_mask);
>> +        ti_clk_ll_ops->clk_writel(v, &dd->mult_div1_reg);
>> +
>> +        _omap3_dpll_write_clken(clk, DPLL_LOCKED);
>> +        _omap3_wait_dpll_status(clk, 1);
>> +    } else {
>> +        _omap3_dpll_write_clken(clk, clk->context);
>> +    }
>> +}
>> +
>> +/**
>> + * omap3_non_core_dpll_save_context - Save the m and n values of the
>> divider
>> + * @hw: pointer  struct clk_hw
>> + *
>> + * Before the dpll registers are lost save the last rounded rate m and n
>> + * and the enable mask.
>> + */
>> +int omap3_noncore_dpll_save_context(struct clk_hw *hw)
>> +{
>> +    struct clk_hw_omap *clk = to_clk_hw_omap(hw);
>> +    struct dpll_data *dd;
>> +    u32 v;
>> +
>> +    dd = clk->dpll_data;
>> +
>> +    v = ti_clk_ll_ops->clk_readl(&dd->control_reg);
>> +    clk->context = (v & dd->enable_mask) >> __ffs(dd->enable_mask);
>> +
>> +    if (clk->context == DPLL_LOCKED) {
>> +        v = ti_clk_ll_ops->clk_readl(&dd->mult_div1_reg);
>> +        dd->last_rounded_m = (v & dd->mult_mask) >>
>> +                        __ffs(dd->mult_mask);
>> +        dd->last_rounded_n = ((v & dd->div1_mask) >>
>> +                        __ffs(dd->div1_mask)) + 1;
> 
> This part could potentially be optimized so that you don't need to read
> the mult_div1_reg ever here. last_rounded_m/n should typically be set to
> the latest value, unless clock framework has ever setup the rate for the
> dpll. In this case, you could just read these during the registration of
> the clock.

Okay. The very first time it saves i see that last_rounded_m/n are 0s.
So like you have suggest i will try to initialize them during
registration of clock.

> 
> -Tero
> 
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * omap3_core_dpll_restore_context - restore the m and n values of
>> the divider
>> + * @hw: pointer  struct clk_hw
>> + *
>> + * Restore the last rounded rate m and n
>> + * and the enable mask.
>> + */
>> +void omap3_noncore_dpll_restore_context(struct clk_hw *hw)
>> +{
>> +    struct clk_hw_omap *clk = to_clk_hw_omap(hw);
>> +    const struct dpll_data *dd;
>> +    u32 ctrl, mult_div1;
>> +
>> +    dd = clk->dpll_data;
>> +
>> +    ctrl = ti_clk_ll_ops->clk_readl(&dd->control_reg);
>> +    mult_div1 = ti_clk_ll_ops->clk_readl(&dd->mult_div1_reg);
>> +
>> +    if (clk->context == ((ctrl & dd->enable_mask) >>
>> +                 __ffs(dd->enable_mask)) &&
>> +        dd->last_rounded_m == ((mult_div1 & dd->mult_mask) >>
>> +                   __ffs(dd->mult_mask)) &&
>> +        dd->last_rounded_n == ((mult_div1 & dd->div1_mask) >>
>> +                   __ffs(dd->div1_mask)) + 1) {
>> +        /* nothing to be done */
>> +        return;
>> +    }
>> +
>> +    if (clk->context == DPLL_LOCKED)
>> +        omap3_noncore_dpll_program(clk, 0);
>> +    else
>> +        _omap3_dpll_write_clken(clk, clk->context);
>> +}
>> +
>>  /* OMAP3/4 non-CORE DPLL clkops */
>>  const struct clk_hw_omap_ops clkhwops_omap3_dpll = {
>>      .allow_idle    = omap3_dpll_allow_idle,
>> diff --git a/drivers/clk/ti/gate.c b/drivers/clk/ti/gate.c
>> index 7151ec3..098db66 100644
>> --- a/drivers/clk/ti/gate.c
>> +++ b/drivers/clk/ti/gate.c
>> @@ -33,6 +33,7 @@
>>      .init        = &omap2_init_clk_clkdm,
>>      .enable        = &omap2_clkops_enable_clkdm,
>>      .disable    = &omap2_clkops_disable_clkdm,
>> +    .restore_context = clk_dflt_restore_context,
>>  };
>>
>>  const struct clk_ops omap_gate_clk_ops = {
>> @@ -40,6 +41,7 @@
>>      .enable        = &omap2_dflt_clk_enable,
>>      .disable    = &omap2_dflt_clk_disable,
>>      .is_enabled    = &omap2_dflt_clk_is_enabled,
>> +    .restore_context = clk_dflt_restore_context,
>>  };
>>
>>  static const struct clk_ops omap_gate_clk_hsdiv_restore_ops = {
>> @@ -47,6 +49,7 @@
>>      .enable        = &omap36xx_gate_clk_enable_with_hsdiv_restore,
>>      .disable    = &omap2_dflt_clk_disable,
>>      .is_enabled    = &omap2_dflt_clk_is_enabled,
>> +    .restore_context = clk_dflt_restore_context,
>>  };
>>
>>  /**
>> diff --git a/drivers/clk/ti/mux.c b/drivers/clk/ti/mux.c
>> index 18c267b..e73764a 100644
>> --- a/drivers/clk/ti/mux.c
>> +++ b/drivers/clk/ti/mux.c
>> @@ -90,10 +90,39 @@ static int ti_clk_mux_set_parent(struct clk_hw
>> *hw, u8 index)
>>      return 0;
>>  }
>>
>> +/**
>> + * clk_mux_save_context - Save the parent selcted in the mux
>> + * @hw: pointer  struct clk_hw
>> + *
>> + * Save the parent mux value.
>> + */
>> +static int clk_mux_save_context(struct clk_hw *hw)
>> +{
>> +    struct clk_mux *mux = to_clk_mux(hw);
>> +
>> +    mux->saved_parent = ti_clk_mux_get_parent(hw);
>> +    return 0;
>> +}
>> +
>> +/**
>> + * clk_mux_restore_context - Restore the parent in the mux
>> + * @hw: pointer  struct clk_hw
>> + *
>> + * Restore the saved parent mux value.
>> + */
>> +static void clk_mux_restore_context(struct clk_hw *hw)
>> +{
>> +    struct clk_mux *mux = to_clk_mux(hw);
>> +
>> +    ti_clk_mux_set_parent(hw, mux->saved_parent);
>> +}
>> +
>>  const struct clk_ops ti_clk_mux_ops = {
>>      .get_parent = ti_clk_mux_get_parent,
>>      .set_parent = ti_clk_mux_set_parent,
>>      .determine_rate = __clk_mux_determine_rate,
>> +    .save_context = clk_mux_save_context,
>> +    .restore_context = clk_mux_restore_context,
>>  };
>>
>>  static struct clk *_register_mux(struct device *dev, const char *name,
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index a428aec..2a8f636 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -103,6 +103,11 @@ struct clk_rate_request {
>>   *        Called with enable_lock held.  This function must not
>>   *        sleep.
>>   *
>> + * @save_context: Save the context of the clock in prepration for
>> poweroff.
>> + *
>> + * @restore_context: Restore the context of the clock after a
>> restoration
>> + *        of power.
>> + *
>>   * @recalc_rate    Recalculate the rate of this clock, by querying
>> hardware. The
>>   *        parent rate is an input parameter.  It is up to the caller to
>>   *        ensure that the prepare_mutex is held across this call.
>> @@ -198,6 +203,8 @@ struct clk_ops {
>>      void        (*disable)(struct clk_hw *hw);
>>      int        (*is_enabled)(struct clk_hw *hw);
>>      void        (*disable_unused)(struct clk_hw *hw);
>> +    int        (*save_context)(struct clk_hw *hw);
>> +    void        (*restore_context)(struct clk_hw *hw);
>>      unsigned long    (*recalc_rate)(struct clk_hw *hw,
>>                      unsigned long parent_rate);
>>      long        (*round_rate)(struct clk_hw *hw, unsigned long rate,
>> @@ -394,6 +401,7 @@ struct clk_divider {
>>      u8        flags;
>>      const struct clk_div_table    *table;
>>      spinlock_t    *lock;
>> +    u32        context;
>>  };
>>
>>  #define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
>> @@ -471,6 +479,7 @@ struct clk_mux {
>>      u8        shift;
>>      u8        flags;
>>      spinlock_t    *lock;
>> +    u8        saved_parent;
>>  };
>>
>>  #define to_clk_mux(_hw) container_of(_hw, struct clk_mux, hw)
>> @@ -912,5 +921,7 @@ struct dentry *clk_debugfs_add_file(struct clk_hw
>> *hw, char *name, umode_t mode,
>>                  void *data, const struct file_operations *fops);
>>  #endif
>>
>> +void clk_dflt_restore_context(struct clk_hw *hw);
>> +
>>  #endif /* CONFIG_COMMON_CLK */
>>  #endif /* CLK_PROVIDER_H */
>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>> index 024cd07..d071a65 100644
>> --- a/include/linux/clk.h
>> +++ b/include/linux/clk.h
>> @@ -438,6 +438,23 @@ struct clk *devm_get_clk_from_child(struct device
>> *dev,
>>   */
>>  struct clk *clk_get_sys(const char *dev_id, const char *con_id);
>>
>> +/**
>> + * clks_save_context - save clock context for poweroff
>> + *
>> + * Saves the context of the clock register for powerstates in which the
>> + * contents of the registers will be lost. Occurs deep within the
>> suspend
>> + * code so locking is not necessary.
>> + */
>> +int clks_save_context(void);
>> +
>> +/**
>> + * clks_restore_context - restore clock context after poweroff
>> + *
>> + * This occurs with all clocks enabled. Occurs deep within the resume
>> code
>> + * so locking is not necessary.
>> + */
>> +void clks_restore_context(void);
>> +
>>  #else /* !CONFIG_HAVE_CLK */
>>
>>  static inline struct clk *clk_get(struct device *dev, const char *id)
>> @@ -501,6 +518,14 @@ static inline struct clk *clk_get_sys(const char
>> *dev_id, const char *con_id)
>>  {
>>      return NULL;
>>  }
>> +
>> +static inline int clks_save_context(void)
>> +{
>> +    return 0;
>> +}
>> +
>> +static inline void clks_restore_context(void) {}
>> +
>>  #endif
>>
>>  /* clk_prepare_enable helps cases using clk_enable in non-atomic
>> context. */
>> diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
>> index d18da83..f604936 100644
>> --- a/include/linux/clk/ti.h
>> +++ b/include/linux/clk/ti.h
>> @@ -159,6 +159,7 @@ struct clk_hw_omap {
>>      const char        *clkdm_name;
>>      struct clockdomain    *clkdm;
>>      const struct clk_hw_omap_ops    *ops;
>> +    u32            context;
>>  };
>>
>>  /*
>> @@ -290,6 +291,11 @@ struct ti_clk_features {
>>
>>  void ti_clk_setup_features(struct ti_clk_features *features);
>>  const struct ti_clk_features *ti_clk_get_features(void);
>> +int omap3_noncore_dpll_save_context(struct clk_hw *hw);
>> +void omap3_noncore_dpll_restore_context(struct clk_hw *hw);
>> +
>> +int omap3_core_dpll_save_context(struct clk_hw *hw);
>> +void omap3_core_dpll_restore_context(struct clk_hw *hw);
>>
>>  extern const struct clk_hw_omap_ops clkhwops_omap2xxx_dpll;
>>
>>
> 

Powered by blists - more mailing lists