Re: [PATCH 2/4] clk: tegra: add EMC clock driver

[Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

Quoting Stephen Warren (2013-12-18 10:28:32)
> On 12/18/2013 02:42 AM, Joseph Lo wrote:
> > On Wed, 2013-12-18 at 06:58 +0800, Stephen Warren wrote:
> >> On 12/17/2013 02:26 AM, Joseph Lo wrote:
> >>> Add External Memory Controller (EMC) clock interface for the Tegra CCF
> >>> driver to support EMC scaling.
> >>
> >>> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
> >>
> >>> +static long clk_emc_round_rate(struct clk_hw *hw, unsigned long rate,
> >>> +                          unsigned long *prate)
> >>> +{
> >>> +   struct tegra_clk_emc *emc = to_clk_emc(hw);
> >>> +   struct clk *parent_clk = __clk_get_parent(hw->clk);
> >>> +   unsigned long parent_rate = __clk_get_rate(parent_clk);
> >>> +   unsigned long ret;
> >>> +
> >>> +   if (!emc->emc_ops)
> >>> +           return parent_rate;
> >>> +
> >>> +   ret = emc->emc_ops->emc_round_rate(rate);
> >>> +   if (!ret)
> >>> +           return parent_rate;
> >>> +
> >>> +   return ret;
> >>> +}
> >>
> >> Rather than implementing this custom "emc_ops" feature, isn't there a
> >> standard clock notifier feature that the EMC driver can use?
> >>
> >> Isn't the EMC driver the only thing that will be changing the EMC clock
> >> rate? If so, why not just have the EMC driver perform the appropriate
> >> pre-/post-rate-change actions before/after calling clk_set_rate()?
> > 
> > We have two HW components needs to be updated when EMC rate change. One
> > is the EMC clock for tuning the frequency, the other is the EMC for
> > updating the timing and configuration for external memory (DRAM). So
> > this question looks like to me is that can we separate the operation of
> > the two components when rate changing?
> > 
> > We have two modes that depend on what memory type (DDR2, LPDDR2,
> > DDR3/LP) we used on the platform to support EMC scaling.
> > 1. power down mode (Tegra20 used this mode only.)
> > In this mode, we update the EMC timing and configurations to EMC shadow
> > registers. Then updating the rate in the EMC clock register. The HW will
> > trigger the rate changing and timing/configurations updating for EMC.
> > 2. self-refresh mode (Tegra30/114/124 if DDR3)
> > More complicate in this mode. Putting DRAM in self-refresh, updating EMC
> > settings, updating EMC clock, then we still need auto calibration before
> > restores DRAM from the self-refresh mode. So the difference was the EMC
> > clock operation was part of EMC scaling procedures.
> > 
> > I guess using the clk_notifier may be OK for the 1st case.
> 
> In both cases, isn't the overall operation something like:
> 
> a) Do some work before changing the EMC clock
> b) Change the EMC clock
> c) Do some work after changing the EMC clock
> 
> Admittedly, the exact definition of "some work" is different for your
> cases (1) and (2) above, but the overall structure is the same. As such,
> can't the EMC scaling driver do (a), then do (b) i.e. call
> clk_set_rate(), then do (c)? Or, in your case (2), do we need to do
> funny tricks like running from IRAM since we can't access SDRAM during
> the clock change? If so, I'm not sure how having the EMC clock changing
> code is going to help your case (2) anyway, since we'll presumably have
> to code up a custom stub in assembly for the part of the code that runs
> from IRAM...

Joseph,

Just as a reference, check out how the smp_twd stuff is updated based on
a clock rate-change notifier in arch/arm/kernel/smp_twd.c, lines
103-142.

Regards,
Mike

>