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

Commit Message

Joseph Lo Dec. 17, 2013, 9:26 a.m. UTC

Add External Memory Controller (EMC) clock interface for the Tegra CCF
driver to support EMC scaling.

Signed-off-by: Joseph Lo <[e-mail]>
---
Cc: Mike Turquette <[e-mail]>
---
 drivers/clk/tegra/Makefile              |   1 +
 drivers/clk/tegra/clk-emc.c             | 183 ++++++++++++++++++++++++++++++++
 drivers/clk/tegra/clk.h                 |  19 ++++
 include/linux/platform_data/tegra_emc.h |   7 ++
 4 files changed, 210 insertions(+)
 create mode 100644 drivers/clk/tegra/clk-emc.c

Comments

Stephen Warren Dec. 17, 2013, 10:58 p.m. UTC | #1

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()?

> +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().

Joseph Lo Dec. 18, 2013, 9:42 a.m. UTC | #2

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.

Stephen Warren Dec. 18, 2013, 6:28 p.m. UTC | #3

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...

Mike Turquette Dec. 18, 2013, 7:30 p.m. UTC | #4

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

>

Joseph Lo Dec. 19, 2013, 8:57 a.m. UTC | #5

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

Joseph Lo Dec. 19, 2013, 9:43 a.m. UTC | #6

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.

> 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...
> 
When updating EMC rate, we also need flow controller support to lock HW
to access SDRAM (self-refresh mode). Running these codes on IRAM can't
block other HW to do that.

Peter De Schrijver Dec. 19, 2013, 10:05 a.m. UTC | #7

> > 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
> 

There is hw interaction between the EMC controller and the CAR module.
Writing to the EMC clock source register in CAR will trigger a state machine
in the EMC controller to handle the change of memory clock. The details
of the state machine and how it needs to be setup are different for
each Tegra variant. This means we need to be sure of the exact sequence
of CAR and EMC register accesses for this to work. In theory this could
be done with pre and post rate change notifiers, but I'm afraid this would
be quite fragile.

> 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

We would need to be very sure clk_set_rate() doesn't access any other register
besides the EMC clock source register before returning to the EMC scaling
driver.

> 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...
> 

There is no need for assembler or running from IRAM (at least from Tegra30
onwards, I don't know about Tegra20).

Cheers,

Peter.

Lucas Stach Dec. 19, 2013, 11:43 a.m. UTC | #8

Am Donnerstag, den 19.12.2013, 12:05 +0200 schrieb Peter De Schrijver:
[...]
> 
> > 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...
> > 
> 
> There is no need for assembler or running from IRAM (at least from Tegra30
> onwards, I don't know about Tegra20).
> 
Tegra20 doesn't need any IRAM trickery, the sequence is just:
1. set up EMC shadow registers for new clock frequency
2. change EMC divider in CAR module

This was known working on Colibri T20 before the CCF change.

Regards,
Lucas

Peter De Schrijver Dec. 19, 2013, 11:46 a.m. UTC | #9

On Thu, Dec 19, 2013 at 12:43:28PM +0100, Lucas Stach wrote:
> Am Donnerstag, den 19.12.2013, 12:05 +0200 schrieb Peter De Schrijver:
> [...]
> > 
> > > 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...
> > > 
> > 
> > There is no need for assembler or running from IRAM (at least from Tegra30
> > onwards, I don't know about Tegra20).
> > 
> Tegra20 doesn't need any IRAM trickery, the sequence is just:
> 1. set up EMC shadow registers for new clock frequency
> 2. change EMC divider in CAR module
> 

Nothing needs to be done after the divider change?

Cheers,

Peter.

Stephen Warren Dec. 19, 2013, 7:41 p.m. UTC | #10

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?

Stephen Warren Dec. 19, 2013, 7:44 p.m. UTC | #11

On 12/19/2013 03:05 AM, Peter De Schrijver wrote:
>>> 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
>>
> 
> There is hw interaction between the EMC controller and the CAR module.
> Writing to the EMC clock source register in CAR will trigger a state machine
> in the EMC controller to handle the change of memory clock. The details
> of the state machine and how it needs to be setup are different for
> each Tegra variant. This means we need to be sure of the exact sequence
> of CAR and EMC register accesses for this to work. In theory this could
> be done with pre and post rate change notifiers, but I'm afraid this would
> be quite fragile.

Isn't the state machine setup something that the EMC driver does through
EMC registers (or perhaps also using flow controller registers?). If all
the clock driver does for this clock is program the requested rate,
which triggers the HW state machine, then I think we can isolate
everything else into the EMC driver.

In such a case, we don't need to use pre-/post-rate change notifiers at
all; just have the EMC driver call into the clock driver at the
appropriate step in the process where the clock registers should be written.

> 
>> 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
> 
> We would need to be very sure clk_set_rate() doesn't access any other register
> besides the EMC clock source register before returning to the EMC scaling
> driver.

Well, we can certainly make that happen. We're writing the code behind
clk_set_rate() for the EMC clock after all...

But I'm not sure why that is a requirement. If it is, we're pretty
screwed, because surely there's nothing stopping some other driver
that's running on a different CPU from coming along and changing some
completely unrelated clock, which will then touch "some other register".

Peter De Schrijver Dec. 20, 2013, 11:34 a.m. UTC | #12

On Thu, Dec 19, 2013 at 08:44:43PM +0100, Stephen Warren wrote:
> On 12/19/2013 03:05 AM, Peter De Schrijver wrote:
> >>> 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
> >>
> > 
> > There is hw interaction between the EMC controller and the CAR module.
> > Writing to the EMC clock source register in CAR will trigger a state machine
> > in the EMC controller to handle the change of memory clock. The details
> > of the state machine and how it needs to be setup are different for
> > each Tegra variant. This means we need to be sure of the exact sequence
> > of CAR and EMC register accesses for this to work. In theory this could
> > be done with pre and post rate change notifiers, but I'm afraid this would
> > be quite fragile.
> 
> Isn't the state machine setup something that the EMC driver does through
> EMC registers (or perhaps also using flow controller registers?). If all
> the clock driver does for this clock is program the requested rate,
> which triggers the HW state machine, then I think we can isolate
> everything else into the EMC driver.
> 
> In such a case, we don't need to use pre-/post-rate change notifiers at
> all; just have the EMC driver call into the clock driver at the
> appropriate step in the process where the clock registers should be written.
> 
> > 
> >> 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
> > 
> > We would need to be very sure clk_set_rate() doesn't access any other register
> > besides the EMC clock source register before returning to the EMC scaling
> > driver.
> 
> Well, we can certainly make that happen. We're writing the code behind
> clk_set_rate() for the EMC clock after all...
> 

CCF propagates clock changes if you do a set_rate. In this case, I think it
will work out, but in general clk_set_rate() does a lot more then just calling
the clock's set_rate method.

> But I'm not sure why that is a requirement. If it is, we're pretty
> screwed, because surely there's nothing stopping some other driver
> that's running on a different CPU from coming along and changing some
> completely unrelated clock, which will then touch "some other register".
> 

It's unlikely any other CPU will execute much code, because as soon as it does
an SDRAM access, it will be stuck until the EMC has finalized its rate change.
I think the requirement is more like 'do not touch any EMC register and possibly
also the EMC divider register before the EMC state machine has finished'. At
the moment this ensured by locks to prevent other CPUs from doing an EMC rate
change and by a 'barrier read' of a EMC register which will only complete once
the state machine has finished. On Tegra30 and later, some more work needs to
be done before the entire rate change is finished.

Cheers,

Peter.

Patch

diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
index f7dfb72884a4..c493ba9ad531 100644
--- a/drivers/clk/tegra/Makefile
+++ b/drivers/clk/tegra/Makefile
@@ -1,6 +1,7 @@ 
 obj-y					+= clk.o
 obj-y					+= clk-audio-sync.o
 obj-y					+= clk-divider.o
+obj-y					+= clk-emc.o
 obj-y					+= clk-periph.o
 obj-y					+= clk-periph-gate.o
 obj-y					+= clk-pll.o
diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
new file mode 100644
index 000000000000..4403696a7dc2
--- /dev/null
+++ b/drivers/clk/tegra/clk-emc.c
@@ -0,0 +1,183 @@ 
+/*
+ * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/platform_data/tegra_emc.h>
+
+#include "clk.h"
+
+static u8 clk_emc_get_parent(struct clk_hw *hw)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	const struct clk_ops *mux_ops = emc->periph->mux_ops;
+	struct clk_hw *mux_hw = &emc->periph->mux.hw;
+
+	mux_hw->clk = hw->clk;
+	return mux_ops->get_parent(mux_hw);
+}
+
+static unsigned long clk_emc_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	struct tegra_clk_periph *periph = emc->periph;
+	const struct clk_ops *div_ops = periph->div_ops;
+	struct clk_hw *div_hw = &periph->divider.hw;
+
+	return div_ops->recalc_rate(div_hw, parent_rate);
+}
+
+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;
+}
+
+static int clk_emc_set_rate(struct clk_hw *hw, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	struct tegra_clk_periph *periph = emc->periph;
+	const struct clk_ops *div_ops = periph->div_ops;
+	struct clk_hw *div_hw = &periph->divider.hw;
+	int ret = -EINVAL;
+
+	if (!emc->emc_ops)
+		goto out;
+
+	ret = emc->emc_ops->emc_set_rate(rate);
+	if (ret)
+		goto out;
+
+	div_ops->set_rate(div_hw, rate, parent_rate);
+
+out:
+	return ret;
+}
+
+static int clk_emc_is_enabled(struct clk_hw *hw)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	const struct clk_ops *gate_ops = emc->periph->gate_ops;
+	struct clk_hw *gate_hw = &emc->periph->gate.hw;
+
+	gate_hw->clk = hw->clk;
+
+	return gate_ops->is_enabled(gate_hw);
+}
+
+static int clk_emc_enable(struct clk_hw *hw)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	const struct clk_ops *gate_ops = emc->periph->gate_ops;
+	struct clk_hw *gate_hw = &emc->periph->gate.hw;
+
+	gate_hw->clk = hw->clk;
+
+	return gate_ops->enable(gate_hw);
+}
+
+static void clk_emc_disable(struct clk_hw *hw)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	const struct clk_ops *gate_ops = emc->periph->gate_ops;
+	struct clk_hw *gate_hw = &emc->periph->gate.hw;
+
+	gate_ops->disable(gate_hw);
+}
+
+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;
+	hw = __clk_get_hw(emc_clk);
+
+	emc = to_clk_emc(hw);
+	if (emc)
+		emc->emc_ops = emc_ops;
+}
+
+static const struct clk_ops tegra_clk_emc_ops = {
+	.get_parent = clk_emc_get_parent,
+	.recalc_rate = clk_emc_recalc_rate,
+	.round_rate = clk_emc_round_rate,
+	.set_rate = clk_emc_set_rate,
+	.is_enabled = clk_emc_is_enabled,
+	.enable = clk_emc_enable,
+	.disable = clk_emc_disable,
+};
+
+struct clk *tegra_clk_register_emc(const char *name, const char **parent_names,
+	int num_parents, struct tegra_clk_periph *periph,
+	void __iomem *clk_base, u32 offset, unsigned long flags)
+{
+	struct tegra_clk_emc *emc;
+	struct clk *clk;
+	struct clk_init_data init;
+	struct tegra_clk_periph_regs *bank;
+
+	emc = kzalloc(sizeof(*emc), GFP_KERNEL);
+	if (!emc) {
+		pr_err("%s: could not allocate emc clk\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	init.name = name;
+	init.ops = &tegra_clk_emc_ops;
+	init.flags = flags;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
+	bank = get_reg_bank(periph->gate.clk_num);
+	if (!bank)
+		return ERR_PTR(-EINVAL);
+
+	periph->magic = TEGRA_CLK_PERIPH_MAGIC;
+	periph->mux.reg = clk_base + offset;
+	periph->divider.reg = clk_base + offset;
+	periph->gate.clk_base = clk_base;
+	periph->gate.regs = bank;
+	periph->gate.enable_refcnt = periph_clk_enb_refcnt;
+
+	/* Data in .init is copied by clk_register(), so stack variable OK */
+	emc->hw.init = &init;
+	emc->periph = periph;
+	clk = clk_register(NULL, &emc->hw);
+	if (IS_ERR(clk)) {
+		kfree(emc);
+		return clk;
+	}
+
+	emc->periph->mux.hw.clk = clk;
+	emc->periph->divider.hw.clk = clk;
+	emc->periph->gate.hw.clk = clk;
+
+	return clk;
+}
diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
index 16ec8d6bb87f..381a9b486805 100644
--- a/drivers/clk/tegra/clk.h
+++ b/drivers/clk/tegra/clk.h
@@ -511,6 +511,25 @@  struct tegra_periph_init_data {
 			NULL, 0, NULL)
 
 /**
+ * struct clk-emc - emc clock
+ *
+ * @hw:         handle between common and hardware-specific interfaces
+ * @periph:     periph clock
+ * @emc_ops:    emc ops
+ */
+struct tegra_clk_emc {
+	struct clk_hw			hw;
+	struct tegra_clk_periph		*periph;
+	const struct emc_clk_ops	*emc_ops;
+};
+
+#define to_clk_emc(_hw) container_of(_hw, struct tegra_clk_emc, hw)
+
+struct clk *tegra_clk_register_emc(const char *name, const char **parent_names,
+		int num_parents, struct tegra_clk_periph *periph,
+		void __iomem *clk_base, u32 offset, unsigned long flags);
+
+/**
  * struct clk_super_mux - super clock
  *
  * @hw:		handle between common and hardware-specific interfaces
diff --git a/include/linux/platform_data/tegra_emc.h b/include/linux/platform_data/tegra_emc.h
index df67505e98f8..f36cb58932d2 100644
--- a/include/linux/platform_data/tegra_emc.h
+++ b/include/linux/platform_data/tegra_emc.h
@@ -31,4 +31,11 @@  struct tegra_emc_pdata {
 	struct tegra_emc_table *tables;
 };
 
+struct emc_clk_ops {
+	long		(*emc_round_rate)(unsigned long);
+	int		(*emc_set_rate)(unsigned long);
+};
+
+void tegra_register_emc_clk_ops(struct clk *emc_clk,
+				const struct emc_clk_ops *emc_ops);
 #endif