On 12/19/2013 02:43 AM, Joseph Lo wrote: > On Thu, 2013-12-19 at 02:28 +0800, Stephen Warren wrote: >> 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 >> > Roughly say, yes. But ... > > We still need an EMC clock driver. Currently there is no clock function > in Tegra clock driver can just support it. > > For example, > > * the set_rate function > We had an EMC table that includes whole the rates, EMC settings and EMC > clock settings that we should run at the rate of MC/2 or same with MC. > We can't just set the EMC clock rate without the info came from the > table. It's pre-define, pre-setup for the platform and SDRAM module. So > the operation can't be separated into PRE_RATE_CHANGE or > POST_RATE_CHANGE. > > * the round_rate function > We also need to get a supported rate for the EMC/EMC clock from the EMC > table. > > If we ignore the case above, then we need a totally new EMC table that > only supports fixed rate of MC rate and only support one mode for rate > changing. That means only Tegra20 support it currently. Isn't the EMC scaling driver the only driver that will call clk_set_rate() on the EMC clock? As such, can't we assume that any value passed to set_rate/round_rate for this one clock have already been validated by the EMC driver, and hence the clock driver should just blindly accept/apply it? -- 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