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

[Joseph Lo]

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.

I suggest create a EMC clock driver to work closely with EMC driver.
That would help us to represent fully clock function of EMC clock.

> 
> > +void tegra_register_emc_clk_ops(struct clk *emc_clk,
> > +				const struct emc_clk_ops *emc_ops)
> > +{
> > +	struct clk_hw *hw;
> > +	struct tegra_clk_emc *emc;
> > +
> > +	if (IS_ERR_OR_NULL(emc_clk))
> > +		return;
> 
> IS_ERROR_OR_NULL() shouldn't be used. "struct clk *" is either IS_ERR()
> on error, or it's NULL on error, not both. The body of clk_get() implies
> you need to use IS_ERR().

True, will fix.