On Thu, 2013-12-19 at 03:30 +0800, Mike Turquette wrote: > 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. > Thanks. Joseph -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html