| Message ID | 1387272400-4689-3-git-send-email-josephl@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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(). -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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". -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
Add External Memory Controller (EMC) clock interface for the Tegra CCF driver to support EMC scaling. Signed-off-by: Joseph Lo <josephl@nvidia.com> --- Cc: Mike Turquette <mturquette@linaro.org> --- 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