Re: [PATCH v6 00/18] Consolidate and improve NVIDIA Tegra CPUIDLE driver(s)

[Peter De Schrijver]

On Tue, Oct 15, 2019 at 07:59:57PM +0300, Dmitry Osipenko wrote:
> Hello,
> 
> This series does the following:
> 
>   1. Unifies Tegra20/30/114 drivers into a single driver and moves it out
>  into common drivers/cpuidle/ directory.
> 
>   2. Enables CPU cluster power-down idling state on Tegra30.
> 
> In the end there is a quite nice clean up of the Tegra CPUIDLE drivers
> and of the Tegra's arch code in general. Please review, thanks!
> 
> Changelog:
> 
> v6: - Addressed request from Thierry Reding to change the way patches are
>   organized by making changes in a more incremental manner.
> 
> - tegra_sleep_cpu() now checks for the secondary CPUs to be offline
>   in the "Make outer_disable() open-coded" patch.
> 
> v5: - Rebased on a recent linux-next, fixed one minor conflict in Kconfig.
> 
> - Improved commit's message of the "Support CPU cluster power-down state
>   on Tegra30" patch.
> 
> - The "Support CPU cluster power-down state on Tegra30" patch is also
>   got split and now there is additional "Make outer_disable() open-coded"
>   patch.
> 
> - Made minor cosmetic changes to the "Introduce unified driver for
>   NVIDIA Tegra SoCs" patch by improving error message and renaming
>   one variable.
> 
> v4: - Fixed compilation with !CONFIG_CACHE_L2X0 (and tested that it still
>   works).
> 
> - Replaced ktime_compare() with ktime_before() in the new driver,
>   for consistency.
> 
> v3: - Addressed review comments that were made by Jon Hunter to v2 by
>   splitting patches into smaller (and simpler) chunks, better
>   documenting changes in the commit messages and using proper error
>   codes in the code.
> 
>   Warnings are replaced with a useful error messages in the code of
>   "Introduce unified driver for NVIDIA Tegra SoCs" patch.
> 
>   Secondary CPUs parking timeout increased to 100ms because I found
>   that it actually may happen to take more than 1ms if CPU is running
>   on a *very* low frequency.
> 
>   Added diagnostic messages that are reporting Flow Controller state
>   when CPU parking fails.
> 
>   Further polished cpuidle driver's code.
> 
>   The coupled state entering is now aborted if there is a pending SGI
>   (Software Generated Interrupt) because it will be lost after GIC's
>   power-cycling. Like it was done by the old Tegra20 CPUIDLE driver.
> 
> v2: - Added patches to enable the new cpuidle driver in the defconfigs:
> 
> ARM: multi_v7_defconfig: Enable Tegra cpuidle driver
> ARM: tegra: Enable Tegra cpuidle driver in tegra_defconfig
> 
> - Dropped patches that removed CPUIDLE_FLAG_TIMER_STOP from the idling
>   states because that flag actually doesn't have any negative effects,
>   but still is correct for the case of a local CPU timer on older Tegra
>   SoCs:
> 
> cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from Tegra114/124 
> idle-state
> cpuidle: tegra: Remove CPUIDLE_FLAG_TIMER_STOP from all states
> 
> - The "Add unified driver for NVIDIA Tegra SoCs" patch got more polish.
>   Tegra30 and Terga114 states are now squashed into a single common C7
>   state (following Parker TRM terminology, see 17.2.2.2 Power Management
>   States), more comments added, etc minor changes.

It would be useful to switch the power state terminology to the one used
for later chips:

LP0 becomes SC7
LP1 becomes C1
LP2 becomes CC7

Meaning of these states is as follows

C is a core state:

C1 clock gating
C2 not defined
C3 not defined
C4 not defined
C5 not defined
C6 not defined for ARM cores
C7 power-gating

CC is a CPU cluster C state:

CC1 cluster clock gated
CC2 not defined
CC3 fmax@Vmin: not used prior to Tegra186
CC4: cluster retention: no longer supported
CC5: not defined
CC6: cluster power gating
CC7: cluster rail gating

SC is a System C state:

SC1: not defined
SC2: not defined
SC3: not defined
SC4: not defined
SC5: not defined
SC6: not defined
SC7: VDD_SOC off

Cheers,

Peter.

Re: [PATCH v2 3/3] soc/tegra: regulators: Add regulators coupler for Tegra30

[Peter De Schrijver]

On Mon, Aug 05, 2019 at 02:03:29PM +0300, Dmitry Osipenko wrote:
> 05.08.2019 11:33, Peter De Schrijver пишет:
> > On Fri, Aug 02, 2019 at 05:39:23PM +0300, Dmitry Osipenko wrote:
> >> 02.08.2019 17:05, Peter De Schrijver пишет:
> >>> On Thu, Jul 25, 2019 at 06:18:32PM +0300, Dmitry Osipenko wrote:
> >>>> Add regulators coupler for Tegra30 SoCs that performs voltage balancing
> >>>> of a coupled regulators and thus provides voltage scaling functionality.
> >>>>
> >>>> There are 2 coupled regulators on all Tegra30 SoCs: CORE and CPU. The
> >>>> coupled regulator voltages shall be in a range of 300mV from each other
> >>>> and CORE voltage shall be higher than the CPU by N mV, where N depends
> >>>> on the CPU voltage.
> >>>>
> >>>> Signed-off-by: Dmitry Osipenko 
> >>>> ---
> >>>>  drivers/soc/tegra/Kconfig  |   4 +
> >>>>  drivers/soc/tegra/Makefile |   1 +
> >>>>  drivers/soc/tegra/regulators-tegra30.c | 316 +
> >>>>  3 files changed, 321 insertions(+)
> >>>>  create mode 100644 drivers/soc/tegra/regulators-tegra30.c
> >>>>
> >>> ...
> >>>
> >>>> +
> >>>> +static int tegra30_core_cpu_limit(int cpu_uV)
> >>>> +{
> >>>> +if (cpu_uV < 80)
> >>>> +return 95;
> >>>> +
> >>>> +if (cpu_uV < 90)
> >>>> +return 100;
> >>>> +
> >>>> +if (cpu_uV < 100)
> >>>> +return 110;
> >>>> +
> >>>> +if (cpu_uV < 110)
> >>>> +return 120;
> >>>> +
> >>>> +if (cpu_uV < 125) {
> >>>> +switch (tegra_sku_info.cpu_speedo_id) {
> >>>> +case 0 ... 1:
> >>> Aren't we supposed to add /* fall through */ here now?
> >>
> >> There is no compiler warning if there is nothing in-between of the
> >> case-switches, so annotation isn't really necessary here. Of course it
> >> is possible to add an explicit annotation just to make clear the
> >> fall-through intention.
> >>
> > 
> > Ah. Ok. Whatever you want then :)
> 
> I'll add the comments if there will be a need to re-spin this series.
> 
> >>>> +case 4:
> >>>> +case 7 ... 8:
> >>>> +return 120;
> >>>> +
> >>>> +default:
> >>>> +return 130;
> >>>> +}
> >>>> +}
> >>>> +
> >>>
> >>> Other than that, this looks ok to me.
> >>
> >> Awesome, thank you very much! Explicit ACK will be appreciated as well.
> > 
> > Acked-By: Peter De Schrijver 

All of them.

Peter.

Re: [PATCH v2 3/3] soc/tegra: regulators: Add regulators coupler for Tegra30

[Peter De Schrijver]

On Fri, Aug 02, 2019 at 05:39:23PM +0300, Dmitry Osipenko wrote:
> 02.08.2019 17:05, Peter De Schrijver пишет:
> > On Thu, Jul 25, 2019 at 06:18:32PM +0300, Dmitry Osipenko wrote:
> >> Add regulators coupler for Tegra30 SoCs that performs voltage balancing
> >> of a coupled regulators and thus provides voltage scaling functionality.
> >>
> >> There are 2 coupled regulators on all Tegra30 SoCs: CORE and CPU. The
> >> coupled regulator voltages shall be in a range of 300mV from each other
> >> and CORE voltage shall be higher than the CPU by N mV, where N depends
> >> on the CPU voltage.
> >>
> >> Signed-off-by: Dmitry Osipenko 
> >> ---
> >>  drivers/soc/tegra/Kconfig  |   4 +
> >>  drivers/soc/tegra/Makefile |   1 +
> >>  drivers/soc/tegra/regulators-tegra30.c | 316 +
> >>  3 files changed, 321 insertions(+)
> >>  create mode 100644 drivers/soc/tegra/regulators-tegra30.c
> >>
> > ...
> > 
> >> +
> >> +static int tegra30_core_cpu_limit(int cpu_uV)
> >> +{
> >> +  if (cpu_uV < 80)
> >> +  return 95;
> >> +
> >> +  if (cpu_uV < 90)
> >> +  return 100;
> >> +
> >> +  if (cpu_uV < 100)
> >> +  return 110;
> >> +
> >> +  if (cpu_uV < 110)
> >> +  return 120;
> >> +
> >> +  if (cpu_uV < 125) {
> >> +  switch (tegra_sku_info.cpu_speedo_id) {
> >> +  case 0 ... 1:
> > Aren't we supposed to add /* fall through */ here now?
> 
> There is no compiler warning if there is nothing in-between of the
> case-switches, so annotation isn't really necessary here. Of course it
> is possible to add an explicit annotation just to make clear the
> fall-through intention.
> 

Ah. Ok. Whatever you want then :)

> >> +  case 4:
> >> +  case 7 ... 8:
> >> +  return 120;
> >> +
> >> +  default:
> >> +  return 130;
> >> +  }
> >> +  }
> >> +
> > 
> > Other than that, this looks ok to me.
> 
> Awesome, thank you very much! Explicit ACK will be appreciated as well.

Acked-By: Peter De Schrijver 

Re: [PATCH v2 3/3] soc/tegra: regulators: Add regulators coupler for Tegra30

[Peter De Schrijver]

On Thu, Jul 25, 2019 at 06:18:32PM +0300, Dmitry Osipenko wrote:
> Add regulators coupler for Tegra30 SoCs that performs voltage balancing
> of a coupled regulators and thus provides voltage scaling functionality.
> 
> There are 2 coupled regulators on all Tegra30 SoCs: CORE and CPU. The
> coupled regulator voltages shall be in a range of 300mV from each other
> and CORE voltage shall be higher than the CPU by N mV, where N depends
> on the CPU voltage.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/soc/tegra/Kconfig  |   4 +
>  drivers/soc/tegra/Makefile |   1 +
>  drivers/soc/tegra/regulators-tegra30.c | 316 +
>  3 files changed, 321 insertions(+)
>  create mode 100644 drivers/soc/tegra/regulators-tegra30.c
> 
...

> +
> +static int tegra30_core_cpu_limit(int cpu_uV)
> +{
> + if (cpu_uV < 80)
> + return 95;
> +
> + if (cpu_uV < 90)
> + return 100;
> +
> + if (cpu_uV < 100)
> + return 110;
> +
> + if (cpu_uV < 110)
> + return 120;
> +
> + if (cpu_uV < 125) {
> + switch (tegra_sku_info.cpu_speedo_id) {
> + case 0 ... 1:
Aren't we supposed to add /* fall through */ here now?
> + case 4:
> + case 7 ... 8:
> + return 120;
> +
> + default:
> + return 130;
> + }
> + }
> +

Other than that, this looks ok to me.

Peter.

Re: [PATCH v3 1/2] soc/tegra: pmc: Query PCLK clock rate at probe time

[Peter De Schrijver]

On Tue, Jul 30, 2019 at 08:40:19PM +0300, Dmitry Osipenko wrote:
> The PCLK clock is running off SCLK, which is a critical clock that is
> very unlikely to randomly change its rate. It is possible to get a
> lockup if kernel decides to enter LP2 cpuidle from a clk-notifier, which
> happens occasionally in a case of Tegra30 EMC driver that waits for the
> clk-change event in the clk-notify handler, because CCF's 'prepare' mutex
> in kept locked and thus clk_get_rate() wants to sleep with interrupts
> being disabled.
> 

I don't think this is the right solution. Eventually we will want to
scale sclk and pclk because the clock tree power of those is not
insignificant. Maybe register a notifier which updates the PMC timer
values when pclk changes?

Peter.

Re: [PATCH V6 01/21] irqchip: tegra: Do not disable COP IRQ during suspend

[Peter De Schrijver]

On Thu, Jul 25, 2019 at 01:59:09PM +0300, Dmitry Osipenko wrote:
> 25.07.2019 13:38, Peter De Schrijver пишет:
> > On Thu, Jul 25, 2019 at 01:33:48PM +0300, Peter De Schrijver wrote:
> >> On Thu, Jul 25, 2019 at 01:05:13PM +0300, Dmitry Osipenko wrote:
> >>> 25.07.2019 12:55, Peter De Schrijver пишет:
> >>>> On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
> >>>>>
> >>>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> >>>>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> >>>>> better here.
> >>>>>
> >>>>>> +  writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> >>>>>
> >>>>> Secondly, I'm also not sure why COP interrupts need to be disabled for
> >>>>> pre-T210 at all, since COP is unused. This looks to me like it was
> >>>>> cut-n-pasted from downstream kernel without a good reason and could be
> >>>>> simply removed.
> >>>>
> >>>> I don't think we can rely on the fact that COP is unused. People can
> >>>> write their own code to run on COP.
> >>>
> >>> 1. Not upstream - doesn't matter.
> >>>
> >>
> >> The code is not part of the kernel, so obviously it's not upstream?
> >>
> >>> 2. That's not very good if something unknown is running on COP and then
> >>> kernel suddenly intervenes, don't you think so?
> >>
> >> Unless the code was written with this in mind.
> >>
> 
> In that case, please see 1. ;)
> 

In general the kernel should not touch the COP interrupts I think.

> > 
> > Looking at this again, I don't think we need to enable the IRQ at all.
> 
> Could you please clarify? The code only saves/restores COP's interrupts
> context across suspend-resume.

The sc7 entry firmware doesn't use interrupts.

Peter.

Re: [PATCH V6 01/21] irqchip: tegra: Do not disable COP IRQ during suspend

[Peter De Schrijver]

On Thu, Jul 25, 2019 at 01:33:48PM +0300, Peter De Schrijver wrote:
> On Thu, Jul 25, 2019 at 01:05:13PM +0300, Dmitry Osipenko wrote:
> > 25.07.2019 12:55, Peter De Schrijver пишет:
> > > On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
> > >>
> > >> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> > >> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> > >> better here.
> > >>
> > >>> +   writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> > >>
> > >> Secondly, I'm also not sure why COP interrupts need to be disabled for
> > >> pre-T210 at all, since COP is unused. This looks to me like it was
> > >> cut-n-pasted from downstream kernel without a good reason and could be
> > >> simply removed.
> > > 
> > > I don't think we can rely on the fact that COP is unused. People can
> > > write their own code to run on COP.
> > 
> > 1. Not upstream - doesn't matter.
> > 
> 
> The code is not part of the kernel, so obviously it's not upstream?
> 
> > 2. That's not very good if something unknown is running on COP and then
> > kernel suddenly intervenes, don't you think so?
> 
> Unless the code was written with this in mind.
> 

Looking at this again, I don't think we need to enable the IRQ at all.

Peter.

Re: [PATCH V6 01/21] irqchip: tegra: Do not disable COP IRQ during suspend

[Peter De Schrijver]

On Thu, Jul 25, 2019 at 01:05:13PM +0300, Dmitry Osipenko wrote:
> 25.07.2019 12:55, Peter De Schrijver пишет:
> > On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
> >>
> >> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> >> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> >> better here.
> >>
> >>> + writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> >>
> >> Secondly, I'm also not sure why COP interrupts need to be disabled for
> >> pre-T210 at all, since COP is unused. This looks to me like it was
> >> cut-n-pasted from downstream kernel without a good reason and could be
> >> simply removed.
> > 
> > I don't think we can rely on the fact that COP is unused. People can
> > write their own code to run on COP.
> 
> 1. Not upstream - doesn't matter.
> 

The code is not part of the kernel, so obviously it's not upstream?

> 2. That's not very good if something unknown is running on COP and then
> kernel suddenly intervenes, don't you think so?

Unless the code was written with this in mind.

Peter.

Re: [PATCH V6 01/21] irqchip: tegra: Do not disable COP IRQ during suspend

[Peter De Schrijver]

On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
> 
> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> better here.
> 
> > +   writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> 
> Secondly, I'm also not sure why COP interrupts need to be disabled for
> pre-T210 at all, since COP is unused. This looks to me like it was
> cut-n-pasted from downstream kernel without a good reason and could be
> simply removed.

I don't think we can rely on the fact that COP is unused. People can
write their own code to run on COP.

Peter.

Re: [PATCH v1] soc/tegra: pmc: Query PCLK clock rate at probe time

[Peter De Schrijver]

On Thu, Jul 18, 2019 at 10:45:31AM +0100, Jon Hunter wrote:
> 
> On 08/07/2019 00:08, Dmitry Osipenko wrote:
> > The PCLK clock is running off SCLK, which is a critical clock that is
> > very unlikely to randomly change its rate. It's also a bit clumsy (and
> > apparently incorrect) to query the clock's rate with interrupts being
> > disabled because clk_get_rate() takes a mutex and that's the case during
> > suspend/cpuidle entering. Lastly, it's better to always fully reprogram
> > PMC state because it's not obvious whether it could be changed after SC7.
> 
> I agree with the first part, but I would drop the last sentence because
> I see no evidence of this. Maybe Peter can confirm.
> 

SCLK and PCLK certainly can change rate at runtime, although the changes
for this haven't made it upstream yet. It's indeed not very obvious, but
certainly doable.

Peter.

Re: [PATCH v1 0/2] Tegra30+ CPU suspend-resume bug-fixes

[Peter De Schrijver]

On Tue, Jul 23, 2019 at 04:28:35AM +0300, Dmitry Osipenko wrote:
> 08.07.2019 2:03, Dmitry Osipenko пишет:
> > Hello,
> > 
> > This small series addresses two suspend-resume bugs: one affects Tegra30+
> > due to a typo in the code, other fixes CPU hang on Tegra30 specifically.
> > 
> > Please review and apply, thanks!
> > 
> > Dmitry Osipenko (2):
> >   ARM: tegra: Fix FLOW_CTLR_HALT register clobbering by tegra_resume()
> >   ARM: tegra: Use WFE for power-gating on Tegra30
> > 
> >  arch/arm/mach-tegra/reset-handler.S |  6 +++---
> >  arch/arm/mach-tegra/sleep-tegra30.S |  4 +++-
> >  drivers/soc/tegra/flowctrl.c| 19 +--
> >  3 files changed, 23 insertions(+), 6 deletions(-)
> > 
> 
> Hello Peter,
> 
> We were talking about these fixes on the IRC not so long time ago, does
> it look good to you? Care to give an ACK?

Acked-By Peter De Schrijver 

Re: [PATCH v2 1/2] soc/tegra: pmc: Query PCLK clock rate at probe time

[Peter De Schrijver]

On Tue, Jul 23, 2019 at 05:35:10AM +0300, Dmitry Osipenko wrote:
> The PCLK clock is running off SCLK, which is a critical clock that is
> very unlikely to randomly change its rate. It's also a bit clumsy (and
> apparently incorrect) to query the clock's rate with interrupts being
> disabled because clk_get_rate() takes a mutex and that's the case during
> suspend/cpuidle entering.
> 

SCLK and PCLK certainly can change rate at runtime, although the code to
handle this hasn't reached upstream yet.

Peter.

> Signed-off-by: Dmitry Osipenko 
> ---
> 
> Changelog:
> 
> v2: Addressed review comments that were made by Jon Hunter to v1 by
> not moving the memory barrier, replacing one missed clk_get_rate()
> with pmc->rate, handling possible clk_get_rate() error on probe and
> slightly adjusting the commits message.
> 
>  drivers/soc/tegra/pmc.c | 34 --
>  1 file changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 9f9c1c677cf4..aba3396b2e73 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -1192,7 +1192,7 @@ static int tegra_io_pad_prepare(struct tegra_pmc *pmc, 
> enum tegra_io_pad id,
>   return err;
>  
>   if (pmc->clk) {
> - rate = clk_get_rate(pmc->clk);
> + rate = pmc->rate;
>   if (!rate) {
>   dev_err(pmc->dev, "failed to get clock rate\n");
>   return -ENODEV;
> @@ -1433,6 +1433,7 @@ void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode 
> mode)
>  void tegra_pmc_enter_suspend_mode(enum tegra_suspend_mode mode)
>  {
>   unsigned long long rate = 0;
> + u64 ticks;
>   u32 value;
>  
>   switch (mode) {
> @@ -1441,31 +1442,22 @@ void tegra_pmc_enter_suspend_mode(enum 
> tegra_suspend_mode mode)
>   break;
>  
>   case TEGRA_SUSPEND_LP2:
> - rate = clk_get_rate(pmc->clk);
> + rate = pmc->rate;
>   break;
>  
>   default:
>   break;
>   }
>  
> - if (WARN_ON_ONCE(rate == 0))
> - rate = 1;
> + ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
> + do_div(ticks, USEC_PER_SEC);
> + tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
>  
> - if (rate != pmc->rate) {
> - u64 ticks;
> + ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
> + do_div(ticks, USEC_PER_SEC);
> + tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
>  
> - ticks = pmc->cpu_good_time * rate + USEC_PER_SEC - 1;
> - do_div(ticks, USEC_PER_SEC);
> - tegra_pmc_writel(pmc, ticks, PMC_CPUPWRGOOD_TIMER);
> -
> - ticks = pmc->cpu_off_time * rate + USEC_PER_SEC - 1;
> - do_div(ticks, USEC_PER_SEC);
> - tegra_pmc_writel(pmc, ticks, PMC_CPUPWROFF_TIMER);
> -
> - wmb();
> -
> - pmc->rate = rate;
> - }
> + wmb();
>  
>   value = tegra_pmc_readl(pmc, PMC_CNTRL);
>   value &= ~PMC_CNTRL_SIDE_EFFECT_LP0;
> @@ -2082,8 +2074,14 @@ static int tegra_pmc_probe(struct platform_device 
> *pdev)
>   pmc->clk = NULL;
>   }
>  
> + pmc->rate = clk_get_rate(pmc->clk);
>   pmc->dev = >dev;
>  
> + if (!pmc->rate) {
> + dev_err(>dev, "failed to get pclk rate\n");
> + pmc->rate = 1;
> + }
> +
>   tegra_pmc_init(pmc);
>  
>   tegra_pmc_init_tsense_reset(pmc);
> -- 
> 2.22.0
> 

Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks

[Peter De Schrijver]

On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:
> > 
> > dependencies I am referring are dfll_ref, dfll_soc, and DVFS peripheral
> > clocks which need to be restored prior to DFLL reinit.
> 
> Okay, but that shouldn't be a problem if clock dependencies are set up
> properly.
> 
> >>> reverse list order during restore might not work as all other clocks are
> >>> in proper order no with any ref clocks for plls getting restored prior
> >>> to their clients
> >> Why? The ref clocks should be registered first and be the roots for PLLs
> >> and the rest. If it's not currently the case, then this need to be
> >> fixed. You need to ensure that each clock is modeled properly. If some
> >> child clock really depends on multiple parents, then the parents need to
> >> in the correct order or CCF need to be taught about such
> >> multi-dependencies.
> >>
> >> If some required feature is missed, then you have to implement it
> >> properly and for all, that's how things are done in upstream. Sometimes
> >> it's quite a lot of extra work that everyone are benefiting from in
> >> the end.
> >>
> >> [snip]
> > 
> > Yes, we should register ref/parents before their clients.
> > 
> > cclk_g clk is registered last after all pll and peripheral clocks are
> > registers during clock init.
> > 
> > dfllCPU_out clk is registered later during dfll-fcpu driver probe and
> > gets added to the clock list.
> > 
> > Probably the issue seems to be not linking dfll_ref and dfll_soc
> > dependencies for dfllCPU_out thru clock list.
> > 
> > clk-dfll driver during dfll_init_clks gets ref_clk and soc_clk reference
> > thru DT.

The dfll does not have any parents. It has some clocks which are needed
for the logic part of the dfll to function, but there's no parent clock
as such unlike for peripheral clocks or PLLs where the parent is at
least used as a reference. The I2C controller of the DFLL shares the
lines with a normal I2C controller using some arbitration logic. That
logic only works if the clock for the normal I2C controller is enabled.
So you need probably 3 clocks enabled to initialize the dfll in that
case. I don't think it makes sense to add complicated logic to the clock
core to deal with this rather strange case. To me it makes more sense to
use pmops and open code the sequence there.

Peter.

Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks

[Peter De Schrijver]

On Tue, Jul 16, 2019 at 09:43:16PM +0300, Dmitry Osipenko wrote:
> > CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher rate
> > so switching to PLL_P during CPUFreq probe prior to dfll clock enable
> > should be safe.
> 
> AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
> which CCLKG supports, the PLLP_OUT4.
> 
> Probably, realistically, CPU is always running off a fast PLLX during
> boot, but I'm wondering what may happen on KEXEC. I guess ideally
> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
> on a reboot, but likely that there are other clock-related problems as
> well that may break KEXEC and thus it is not very important at the moment.
> 

If you turn off the DFLL, you have to be aware that the voltage margins
for DFLL use are lower than for PLL use. So you either need to be sure
to switch to a frequency below fmax @ Vmin or you program the boot
voltage and then you can use PLLX as setup by the bootloader. For OVR
regulators you can't program a voltage without the DFLL, so you have to
tristate the PWM output which will give you a hardwired boot voltage.

Peter.

Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks

[Peter De Schrijver]

On Tue, Jul 16, 2019 at 09:25:43PM +0300, Dmitry Osipenko wrote:
> 16.07.2019 21:19, Sowjanya Komatineni пишет:
> > 
> > On 7/16/19 9:50 AM, Sowjanya Komatineni wrote:
> >>
> >> On 7/16/19 8:00 AM, Dmitry Osipenko wrote:
> >>> 16.07.2019 11:06, Peter De Schrijver пишет:
> >>>> On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
> >>>>>> OK, Will add to CPUFreq driver...
> >>>>>>> The other thing that also need attention is that T124 CPUFreq driver
> >>>>>>> implicitly relies on DFLL driver to be probed first, which is icky.
> >>>>>>>
> >>>>>> Should I add check for successful dfll clk register explicitly in
> >>>>>> CPUFreq driver probe and defer till dfll clk registers?
> >>> Probably you should use the "device links". See [1][2] for the example.
> >>>
> >>> [1]
> >>> https://elixir.bootlin.com/linux/v5.2.1/source/drivers/gpu/drm/tegra/dc.c#L2383
> >>>
> >>>
> >>> [2] https://www.kernel.org/doc/html/latest/driver-api/device_link.html
> >>>
> >>> Return EPROBE_DEFER instead of EINVAL if device_link_add() fails. And
> >>> use of_find_device_by_node() to get the DFLL's device, see [3].
> >>>
> >>> [3]
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra20-devfreq.c#n100
> >>>
> >> Will go thru and add...
> 
> Looks like I initially confused this case with getting orphaned clock.
> I'm now seeing that the DFLL driver registers the clock and then
> clk_get(dfll) should be returning EPROBE_DEFER until DFLL driver is
> probed, hence everything should be fine as-is and there is no real need
> for the 'device link'. Sorry for the confusion!
> 
> >>>>> Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
> >>>>>
> >>>>> As you know it, the DFLL clock is one of the CPU clock sources and
> >>>>> integrated with DVFS control logic with the regulator. We will not
> >>>>> switch
> >>>>> CPU to other clock sources once we switched to DFLL. Because the
> >>>>> CPU has
> >>>>> been regulated by the DFLL HW with the DVFS table (CVB or OPP table
> >>>>> you see
> >>>>> in the driver.). We shouldn't reparent it to other sources with unknew
> >>>>> freq/volt pair. That's not guaranteed to work. We allow switching to
> >>>>> open-loop mode but different sources.
> >>> Okay, then the CPUFreq driver will have to enforce DFLL freq to PLLP's
> >>> rate before switching to PLLP in order to have a proper CPU voltage.
> >>
> >> PLLP freq is safe to work for any CPU voltage. So no need to enforce
> >> DFLL freq to PLLP rate before changing CCLK_G source to PLLP during
> >> suspend
> >>
> > Sorry, please ignore my above comment. During suspend, need to change
> > CCLK_G source to PLLP when dfll is in closed loop mode first and then
> > dfll need to be set to open loop.
> 
> Okay.
> 
> >>>>> And I don't exactly understand why we need to switch to PLLP in CPU
> >>>>> idle
> >>>>> driver. Just keep it on CL-DVFS mode all the time.
> >>>>>
> >>>>> In SC7 entry, the dfll suspend function moves it the open-loop
> >>>>> mode. That's
> >>>>> all. The sc7-entryfirmware will handle the rest of the sequence to
> >>>>> turn off
> >>>>> the CPU power.
> >>>>>
> >>>>> In SC7 resume, the warmboot code will handle the sequence to turn on
> >>>>> regulator and power up the CPU cluster. And leave it on PLL_P. After
> >>>>> resuming to the kernel, we re-init DFLL, restore the CPU clock
> >>>>> policy (CPU
> >>>>> runs on DFLL open-loop mode) and then moving to close-loop mode.
> >>> The DFLL is re-inited after switching CCLK to DFLL parent during of the
> >>> early clocks-state restoring by CaR driver. Hence instead of having odd
> >>> hacks in the CaR driver, it is much nicer to have a proper
> >>> suspend-resume sequencing of the device drivers. In this case CPUFreq
> >>> driver is the driver that enables DFLL and switches CPU to that clock
> >>> source, which means that this driver is also should be responsible for
> >>> managemen

Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks

[Peter De Schrijver]

On Tue, Jul 16, 2019 at 03:24:26PM +0800, Joseph Lo wrote:
> > OK, Will add to CPUFreq driver...
> > > 
> > > The other thing that also need attention is that T124 CPUFreq driver
> > > implicitly relies on DFLL driver to be probed first, which is icky.
> > > 
> > Should I add check for successful dfll clk register explicitly in
> > CPUFreq driver probe and defer till dfll clk registers?
> 
> Sorry, I didn't follow the mail thread. Just regarding the DFLL part.
> 
> As you know it, the DFLL clock is one of the CPU clock sources and
> integrated with DVFS control logic with the regulator. We will not switch
> CPU to other clock sources once we switched to DFLL. Because the CPU has
> been regulated by the DFLL HW with the DVFS table (CVB or OPP table you see
> in the driver.). We shouldn't reparent it to other sources with unknew
> freq/volt pair. That's not guaranteed to work. We allow switching to
> open-loop mode but different sources.
> 
> And I don't exactly understand why we need to switch to PLLP in CPU idle
> driver. Just keep it on CL-DVFS mode all the time.
> 
> In SC7 entry, the dfll suspend function moves it the open-loop mode. That's
> all. The sc7-entryfirmware will handle the rest of the sequence to turn off
> the CPU power.
> 
> In SC7 resume, the warmboot code will handle the sequence to turn on
> regulator and power up the CPU cluster. And leave it on PLL_P. After
> resuming to the kernel, we re-init DFLL, restore the CPU clock policy (CPU
> runs on DFLL open-loop mode) and then moving to close-loop mode.
> 
> The DFLL part looks good to me. BTW, change the patch subject to "Add
> suspend-resume support" seems more appropriate to me.
> 

To clarify this, the sequences for DFLL use are as follows (assuming all
required DFLL hw configuration has been done)

Switch to DFLL:
0) Save current parent and frequency
1) Program DFLL to open loop mode
2) Enable DFLL
3) Change cclk_g parent to DFLL
For OVR regulator:
4) Change PWM output pin from tristate to output
5) Enable DFLL PWM output
For I2C regulator:
4) Enable DFLL I2C output
6) Program DFLL to closed loop mode

Switch away from DFLL:
0) Change cclk_g parent to PLLP so the CPU frequency is ok for any vdd_cpu 
voltage
1) Program DFLL to open loop mode

For OVR regulator:
2) Change PWM output pin from output to tristate: vdd_cpu will go back
   to hardwired boot voltage.
3) Disable DFLL PWM output

For I2C regulator:
2) Program vdd_cpu regulator voltage to the boot voltage
3) Disable DFLL I2C output

4) Reprogram parent saved in step 0 of 'Switch to DFLL' to the saved
   frequency
5) Change cclk_g parent to saved parent
6) Disable DFLL

Peter.

Re: [PATCH v4 00/10] memory: tegra: Introduce Tegra30 EMC driver

[Peter De Schrijver]

On Mon, Jun 17, 2019 at 02:35:41AM +0300, Dmitry Osipenko wrote:
> Hello,
> 
> This series introduces driver for the External Memory Controller (EMC)
> found on Tegra30 chips, it controls the external DRAM on the board. The
> purpose of this driver is to program memory timing for external memory on
> the EMC clock rate change. The driver was tested using the ACTMON devfreq
> driver that performs memory frequency scaling based on memory-usage load.

Acked-By: Peter De Schrijver 

Re: [PATCH v3 1/8] clk: tegra20/30: Add custom EMC clock implementation

[Peter De Schrijver]

On Fri, May 24, 2019 at 08:23:46PM +0300, Dmitry Osipenko wrote:
> A proper External Memory Controller clock rounding and parent selection
> functionality is required by the EMC drivers. It is not available using
> the generic clock implementation, hence add a custom one. The clock rate
> rounding shall be done by the EMC drivers because they have information
> about available memory timings, so the drivers will have to register a
> callback that will round the requested rate. EMC clock users won't be able
> to request EMC clock by getting -EPROBE_DEFER until EMC driver is probed
> and the callback is set up. The functionality is somewhat similar to the
> clk-emc.c which serves Tegra124+ SoC's, the later HW generations support
> more parent clock sources and the HW configuration and integration with
> the EMC drivers differs a tad from the older gens, hence it's not really
> worth to try to squash everything into a single source file.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/clk/tegra/Makefile  |   2 +
>  drivers/clk/tegra/clk-tegra20-emc.c | 299 
>  drivers/clk/tegra/clk-tegra20.c |  55 ++---
>  drivers/clk/tegra/clk-tegra30.c |  38 +++-
>  drivers/clk/tegra/clk.h |   6 +
>  include/linux/clk/tegra.h   |  14 ++
>  6 files changed, 362 insertions(+), 52 deletions(-)
>  create mode 100644 drivers/clk/tegra/clk-tegra20-emc.c
> 
> diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> index 4812e45c2214..df966ca06788 100644
> --- a/drivers/clk/tegra/Makefile
> +++ b/drivers/clk/tegra/Makefile
> @@ -17,7 +17,9 @@ obj-y   += 
> clk-tegra-fixed.o
>  obj-y+= clk-tegra-super-gen4.o
>  obj-$(CONFIG_TEGRA_CLK_EMC)  += clk-emc.o
>  obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20.o
> +obj-$(CONFIG_ARCH_TEGRA_2x_SOC)  += clk-tegra20-emc.o
>  obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra30.o
> +obj-$(CONFIG_ARCH_TEGRA_3x_SOC)  += clk-tegra20-emc.o
>  obj-$(CONFIG_ARCH_TEGRA_114_SOC) += clk-tegra114.o
>  obj-$(CONFIG_ARCH_TEGRA_124_SOC) += clk-tegra124.o
>  obj-$(CONFIG_TEGRA_CLK_DFLL) += clk-tegra124-dfll-fcpu.o
> diff --git a/drivers/clk/tegra/clk-tegra20-emc.c 
> b/drivers/clk/tegra/clk-tegra20-emc.c
> new file mode 100644
> index ..d971b5425ce3
> --- /dev/null
> +++ b/drivers/clk/tegra/clk-tegra20-emc.c
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "clk.h"
> +
> +#define CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK   GENMASK(7, 0)
> +#define CLK_SOURCE_EMC_2X_CLK_SRC_MASK   GENMASK(31, 30)
> +#define CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT  30
> +
> +#define MC_EMC_SAME_FREQ BIT(16)
> +#define USE_PLLM_UD  BIT(29)
> +
> +#define EMC_SRC_PLL_M0
> +#define EMC_SRC_PLL_C1
> +#define EMC_SRC_PLL_P2
> +#define EMC_SRC_CLK_M3
> +
> +static const char * const emc_parent_clk_names[] = {
> + "pll_m", "pll_c", "pll_p", "clk_m",
> +};
> +
> +struct tegra_clk_emc {
> + struct clk_hw hw;
> + void __iomem *reg;
> + bool mc_same_freq;
> + bool want_low_jitter;
> +
> + tegra20_clk_emc_round_cb *round_cb;
> + void *cb_arg;
> +};
> +
> +static inline struct tegra_clk_emc *to_tegra_clk_emc(struct clk_hw *hw)
> +{
> + return container_of(hw, struct tegra_clk_emc, hw);
> +}
> +
> +static unsigned long emc_recalc_rate(struct clk_hw *hw,
> +  unsigned long parent_rate)
> +{
> + struct tegra_clk_emc *emc = to_tegra_clk_emc(hw);
> + u32 val, div;
> +
> + val = readl_relaxed(emc->reg);
> + div = val & CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK;
> +
> + return DIV_ROUND_UP(parent_rate * 2, div + 2);
> +}
> +
> +static u8 emc_get_parent(struct clk_hw *hw)
> +{
> + struct tegra_clk_emc *emc = to_tegra_clk_emc(hw);
> +
> + return readl_relaxed(emc->reg) >> CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT;
> +}
> +
> +static int emc_set_parent(struct clk_hw *hw, u8 index)
> +{
> + struct tegra_clk_emc *emc = to_tegra_clk_emc(hw);
> + u32 val, div;
> +
> + val = readl_relaxed(emc->reg);
> + val &= ~CLK_SOURCE_EMC_2X_CLK_SRC_MASK;
> + val |= index << CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT;
> +
> + div = val & CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK;
> +
> + if (index == EMC_SRC_PLL_M && div == 0 && emc->want_low_jitter)
> + val |= USE_PLLM_UD;
> + else
> + val &= ~USE_PLLM_UD;
> +
> + if (emc->mc_same_freq)
> + val |= MC_EMC_SAME_FREQ;
> + else
> + val &= ~MC_EMC_SAME_FREQ;
> +
> + writel_relaxed(val, emc->reg);
> +

I think technically you need a fence_udelay(1) here, but in practice
this is probably not needed because you will poll some EMC register for
the 

Re: [PATCH V2 00/12] LP0 entry and exit support for Tegra210

[Peter De Schrijver]

On Tue, May 28, 2019 at 04:08:44PM -0700, Sowjanya Komatineni wrote:
> This patch series includes Tegra210 deepsleep/LP0 support with
> deep sleep exit through RTC alarm wake and power button wake events.
> 

We have been calling this SC7 (system C-state 7) for quite a while now.

Peter.

> Note: Wake on power button is through gpio-keys node in device tree.
> 
> This series also includes save and restore of PLLs, clocks, OSC contexts
> for basic LP0 exit.
> 
> This patch series doesn't support 100% suspend/resume to allow fully
> functional state upon resume and we are working on some more drivers suspend
> and resume implementations.
> 
> [V2] : V1 feedback fixes
>   Patch 0002: This version still using syscore. Thierry suggest not to
>   use syscore and waiting on suggestion from Linux Walleij for any better
>   way of storing current state of pins before suspend entry and restoring
>   them on resume at very early stage. So left this the same way as V1 and
>   will address once I get more feedback on this.
>   Also need to findout and implement proper way of forcing resume order
>   between pinctrl and gpio driver.
> 
> 
> Sowjanya Komatineni (12):
>   irqchip: tegra: do not disable COP IRQ during suspend
>   pinctrl: tegra: add suspend and resume support
>   clk: tegra: save and restore PLLs state for system
>   clk: tegra: add support for peripheral clock suspend and resume
>   clk: tegra: add support for OSC clock resume
>   clk: tegra: add suspend resume support for DFLL clock
>   clk: tegra: support for Tegra210 clocks suspend-resume
>   soc/tegra: pmc: allow support for more tegra wake models
>   soc/tegra: pmc: add pmc wake support for tegra210
>   gpio: tegra: implement wake event support for Tegra210 and prior GPIO
>   arm64: tegra: enable wake from deep sleep on RTC alarm.
>   soc/tegra: pmc: configure tegra deep sleep control settings
> 
>  arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi |   7 +
>  arch/arm64/boot/dts/nvidia/tegra210.dtsi   |   5 +-
>  drivers/clk/tegra/clk-dfll.c   |  82 ++
>  drivers/clk/tegra/clk-dfll.h   |   2 +
>  drivers/clk/tegra/clk-divider.c|  19 ++
>  drivers/clk/tegra/clk-pll-out.c|  25 ++
>  drivers/clk/tegra/clk-pll.c|  99 +--
>  drivers/clk/tegra/clk-tegra-fixed.c|  16 ++
>  drivers/clk/tegra/clk-tegra210.c   | 382 
> +
>  drivers/clk/tegra/clk.c|  74 -
>  drivers/clk/tegra/clk.h|  13 +
>  drivers/gpio/gpio-tegra.c  | 116 +++-
>  drivers/irqchip/irq-tegra.c|  22 +-
>  drivers/pinctrl/tegra/pinctrl-tegra.c  |  68 -
>  drivers/pinctrl/tegra/pinctrl-tegra.h  |   3 +
>  drivers/pinctrl/tegra/pinctrl-tegra114.c   |   1 +
>  drivers/pinctrl/tegra/pinctrl-tegra124.c   |   1 +
>  drivers/pinctrl/tegra/pinctrl-tegra20.c|   1 +
>  drivers/pinctrl/tegra/pinctrl-tegra210.c   |   1 +
>  drivers/pinctrl/tegra/pinctrl-tegra30.c|   1 +
>  drivers/soc/tegra/pmc.c| 150 +-
>  21 files changed, 1053 insertions(+), 35 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH v4 00/10] NVIDIA Tegra clocksource driver improvements

[Peter De Schrijver]

On Mon, Jun 03, 2019 at 09:59:38PM +0300, Dmitry Osipenko wrote:
> Hello,
> 
> This series primarily unifies the driver code across all Tegra SoC
> generations. In a result the clocksources are allocated per-CPU on
> older Tegra's and have a higher rating than the arch-timer where
> appropriate, the newer Tegra210 is getting support for microsecond
> clocksource and the driver's code is getting much cleaner.
> 
> The series was extensively tested on Tegra20 and Tegra30.
> 
> Changelog:
> 
> v4: In the comment to v3 Peter De Schrijver pointed out that arch-timer
> isn't affected by DVFS changes and thus it is preferred over tegra-timer
> on [T114, T210). Added new patch to address that: "Lower clocksource
> rating for some Tegra's".
> 
> Daniel Lezcano suggested that it will be worthwhile to rename driver's
> source file as driver now covers more SoC generations than it initially
> did. Hence the new "Rename timer-tegra20.c to timer-tegra.c" patch.
> 
> v3: Fixed compilation on ARM64. Turned out that it doesn't have the
> delay-timer, thanks to Nicolas Chauvet for the report.
> 
> Added new "Support COMPILE_TEST universally" patch for better
> compile-test coverage.
> 
> v2: Rebased on recent linux-next. Now all of #ifdef's are removed from the
> code due to the recent patch that generalized persistent clocksource.
> 
> Couple other minor cosmetic changes.

Series Acked-By: Peter De Schrijver 

Re: [PATCH V2 06/12] clk: tegra: add suspend resume support for DFLL clock

[Peter De Schrijver]

On Tue, May 28, 2019 at 04:08:50PM -0700, Sowjanya Komatineni wrote:
> This patch adds support for suspend and resume for DFLL clock.
> 
> Signed-off-by: Sowjanya Komatineni 
> ---
>  drivers/clk/tegra/clk-dfll.c | 82 
> 
>  drivers/clk/tegra/clk-dfll.h |  2 ++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> index 1fc71baae13b..d92a5a05fbbc 100644
> --- a/drivers/clk/tegra/clk-dfll.c
> +++ b/drivers/clk/tegra/clk-dfll.c
> @@ -286,6 +286,7 @@ struct tegra_dfll {
>   unsigned long   dvco_rate_min;
>  
>   enum dfll_ctrl_mode mode;
> + enum dfll_ctrl_mode resume_mode;
>   enum dfll_tune_rangetune_range;
>   struct dentry   *debugfs_dir;
>   struct clk_hw   dfll_clk_hw;
> @@ -1873,6 +1874,87 @@ static int dfll_fetch_common_params(struct tegra_dfll 
> *td)
>  }
>  
>  /*
> + * tegra_dfll_suspend
> + * @pdev: DFLL instance
> + *
> + * dfll controls clock/voltage to other devices, including CPU. Therefore,
> + * dfll driver pm suspend callback does not stop cl-dvfs operations. It is
> + * only used to enforce cold voltage limit, since SoC may cool down during
> + * suspend without waking up. The correct temperature zone after suspend will
> + * be updated via dfll cooling device interface during resume of temperature
> + * sensor.

Temperature dependent cl-dvfs is not yet implemented in upstream, so
leave out the part about cold voltage limits and temperature zones.

Peter.

Re: [PATCH v3 0/8] NVIDIA Tegra clocksource driver improvements

[Peter De Schrijver]

On Fri, May 31, 2019 at 03:33:41PM +0300, Dmitry Osipenko wrote:
> 31.05.2019 11:26, Peter De Schrijver пишет:
> > On Fri, May 24, 2019 at 06:32:45PM +0300, Dmitry Osipenko wrote:
> >> Hello,
> >>
> >> This series primarily unifies the driver code across all Tegra SoC
> >> generations. In a result the clocksources are allocated per-CPU on
> >> older Tegra's and have a higher rating than the arch-timer, the newer
> >> Tegra210 is getting support for microsecond clocksource and the driver's
> >> code is getting much cleaner. Note that arch-timer usage is discouraged on
> >> all Tegra's due to the time jitter caused by the CPU frequency scaling.
> > 
> > I think the limitations are more as follows:
> > 
> > Chiptimer   suffers cpu dvfs jitter can wakeup from 
> > cc7
> > T20 us-timerNo  Yes
> > T20 twd timer   Yes No?
> > T30 us-timerNo  Yes
> > T30 twd timer   Yes No?
> > T114us-timerNo  Yes
> > T114arch timer  No  Yes
> > T124us-timerNo  Yes
> > T124arch timer  No  Yes
> > T210us-timerNo  Yes
> > T210arch timer  No  No
> > T210clk_m timer No  Yes
> > 
> > right?
> 
> Doesn't arch timer run off the CPU clock? If yes (that's what I
> assumed), then it should be affected by the DVFS. Otherwise I'll lower
> the clocksource's rating for T114/124/132.
> 

No. It doesn't. This is the big change between A9 and later CPUs. 

Peter.

> TWD can't wake CPU from the power-down state, so it's a solid "No" for
> TWD in the "can wakeup from cc7" column.

Re: [PATCH v3 0/8] NVIDIA Tegra clocksource driver improvements

[Peter De Schrijver]

On Fri, May 24, 2019 at 06:32:45PM +0300, Dmitry Osipenko wrote:
> Hello,
> 
> This series primarily unifies the driver code across all Tegra SoC
> generations. In a result the clocksources are allocated per-CPU on
> older Tegra's and have a higher rating than the arch-timer, the newer
> Tegra210 is getting support for microsecond clocksource and the driver's
> code is getting much cleaner. Note that arch-timer usage is discouraged on
> all Tegra's due to the time jitter caused by the CPU frequency scaling.

I think the limitations are more as follows:

Chiptimer   suffers cpu dvfs jitter can wakeup from cc7
T20 us-timerNo  Yes
T20 twd timer   Yes No?
T30 us-timerNo  Yes
T30 twd timer   Yes No?
T114us-timerNo  Yes
T114arch timer  No  Yes
T124us-timerNo  Yes
T124arch timer  No  Yes
T210us-timerNo  Yes
T210arch timer  No  No
T210clk_m timer No  Yes

right?

Peter.

Re: [PATCH v1 0/2] clk: Tegra124 PLLM fixes

[Peter De Schrijver]

On Fri, Apr 19, 2019 at 02:50:10PM +0300, Dmitry Osipenko wrote:
> 12.04.2019 0:48, Dmitry Osipenko пишет:
> > Hello, here are two trivial patches that are correcting PLLM on Tegra124.
> > First fixes system lockup due to a bad hardware configuration, second
> > removes usage of a non-existent register bit.
> > 
> > Dmitry Osipenko (2):
> >   clk: tegra: Fix PLLM programming on Tegra124+ when PMC overrides
> > divider
> >   clk: tegra124: Remove lock-enable bit from PLLM
> > 
> >  drivers/clk/tegra/clk-pll.c  | 4 ++--
> >  drivers/clk/tegra/clk-tegra124.c | 3 +--
> >  2 files changed, 3 insertions(+), 4 deletions(-)
> > 
> 
> Hello Peter,
> 
> The patches in this series are trivial and fixing the longstanding bug on 
> Tegra124. Could you please take a look and and ACK the series if it looks 
> good to you?

Acked-By: Peter De Schrijver   



Peter.

Re: [PATCH v1 3/3] ARM: dts: tegra30: Add External Memory Controller node

[Peter De Schrijver]

On Fri, Apr 12, 2019 at 02:02:21AM +0300, Dmitry Osipenko wrote:
> Add Add External Memory Controller node to the device-tree.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
..
> diff --git a/drivers/memory/tegra/tegra30-emc.c 
> b/drivers/memory/tegra/tegra30-emc.c
> index 38ebdb076ccd..defdb38bde54 100644
> --- a/drivers/memory/tegra/tegra30-emc.c
> +++ b/drivers/memory/tegra/tegra30-emc.c
> @@ -980,7 +980,8 @@ static long emc_round_rate(unsigned long rate,
>   }
>  
>   if (!timing) {
> - dev_err(emc->dev, "no timing for rate %lu\n", rate);
> + dev_err(emc->dev, "no timing for rate %lu min %lu max %lu\n",
> + rate, min_rate, max_rate);
>   return -EINVAL;
>   }

This doesn't seem to belong to this patch?

Peter.

>  
> -- 
> 2.21.0
> 

Re: [PATCH v1 3/3] ARM: dts: tegra30: Add External Memory Controller node

[Peter De Schrijver]

On Fri, Apr 12, 2019 at 02:02:21AM +0300, Dmitry Osipenko wrote:
> Add Add External Memory Controller node to the device-tree.
> 

One 'Add' is enough I think :)

Peter.

> Signed-off-by: Dmitry Osipenko 
> ---
>  arch/arm/boot/dts/tegra30.dtsi | 11 +++
>  drivers/memory/tegra/tegra30-emc.c |  3 ++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
> index e074258d4518..92c4aeafab29 100644
> --- a/arch/arm/boot/dts/tegra30.dtsi
> +++ b/arch/arm/boot/dts/tegra30.dtsi
> @@ -732,6 +732,17 @@
>   #reset-cells = <1>;
>   };
>  
> + memory-controller@7000f400 {
> + compatible = "nvidia,tegra30-emc";
> + reg = <0x7000f400 0x400>;
> + interrupts = ;
> + clocks = <_car TEGRA30_CLK_EMC>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + nvidia,memory-controller = <>;
> + };
> +
>   fuse@7000f800 {
>   compatible = "nvidia,tegra30-efuse";
>   reg = <0x7000f800 0x400>;
> diff --git a/drivers/memory/tegra/tegra30-emc.c 
> b/drivers/memory/tegra/tegra30-emc.c
> index 38ebdb076ccd..defdb38bde54 100644
> --- a/drivers/memory/tegra/tegra30-emc.c
> +++ b/drivers/memory/tegra/tegra30-emc.c
> @@ -980,7 +980,8 @@ static long emc_round_rate(unsigned long rate,
>   }
>  
>   if (!timing) {
> - dev_err(emc->dev, "no timing for rate %lu\n", rate);
> + dev_err(emc->dev, "no timing for rate %lu min %lu max %lu\n",
> + rate, min_rate, max_rate);
>   return -EINVAL;
>   }
>  
> -- 
> 2.21.0
> 

Re: [PATCH v1] clk: tegra20/30: Add custom EMC clock implementation

[Peter De Schrijver]

On Fri, Apr 12, 2019 at 01:47:08AM +0300, Dmitry Osipenko wrote:
> A proper External Memory Controller clock rounding and parent selection
> functionality is required by the EMC drivers. It is not available using
> the generic clock implementation, hence add a custom one. The clock rate
> rounding shall be done by the EMC drivers because they have information
> about available memory timings, so the drivers will have to register a
> callback that will round the requested rate. EMC clock users won't be able
> to request EMC clock by getting -EPROBE_DEFER until EMC driver is probed
> and the callback is set up. The functionality is somewhat similar to the
> clk-emc.c which serves Tegra124+ SoC's, the later HW generations support
> more parent clock sources and the HW configuration and integration with
> the EMC drivers differs a tad from the older gens, hence it's not really
> worth to try to squash everything into a single source file.
> 
..

> + val = readl_relaxed(emc->ioaddr);
> + val &= ~CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK;
> + val |= div;
> +
> + parent = val >> CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT;
> +
> + if (parent == EMC_SRC_PLL_M && div == 0 && emc->want_low_jitter)
> + val |= USE_PLLM_UD;
> + else
> + val &= ~USE_PLLM_UD;
> +

Note that low jitter means the divider is bypassed, so you can only use
this when div == 1.

> + writel_relaxed(val, emc->ioaddr);
> +
> + return 0;
> +}
> +
> +static int emc_set_rate_and_parent(struct clk_hw *hw,
> +unsigned long rate,
> +unsigned long parent_rate,
> +u8 index)
> +{
> + struct tegra_clk_emc *emc = to_tegra_clk_emc(hw);
> + u32 val, div;
> +
> + div = div_frac_get(rate, parent_rate, 8, 1, 0);
> +
> + val = readl_relaxed(emc->ioaddr);
> +
> + val &= ~CLK_SOURCE_EMC_2X_CLK_SRC_MASK;
> + val |= index << CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT;
> +
> + val &= ~CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK;
> + val |= div;
> +
> + if (index == EMC_SRC_PLL_M && div == 0 && emc->want_low_jitter)
> + val |= USE_PLLM_UD;
> + else
> + val &= ~USE_PLLM_UD;
> +

Same here ofcourse.

Peter.

Re: [PATCH RE-SEND] clk: tegra: Don't enable already enabled PLLs

[Peter De Schrijver]

On Wed, Feb 27, 2019 at 06:15:04PM +0300, Dmitry Osipenko wrote:
> 24.02.2019 18:32, Dmitry Osipenko пишет:
> > Initially Common Clock Framework isn't aware of the clock-enable status,
> > this results in enabling of clocks that were enabled by bootloader. This
> > is not a big deal for a regular clock-gates, but for PLL's it may have
> > some unpleasant consequences. Thus re-enabling PLLX (the main CPU parent
> > clock) may result in extra long period of PLL re-locking.
> > 
> > Signed-off-by: Dmitry Osipenko 
> > ---
> 
> Peter (and everyone else), yours r-b and ACK will be appreciated since you 
> kinda already took a look at this patch in the past and were okay with it 
> (IIRC, we had some brief discussion on the #tegra IRC a few months ago), 
> thanks. 
> 

Acked-By: Peter De Schrijver 

Re: [PATCH V2 2/6] clocksource: tegra: add Tegra210 timer driver

[Peter De Schrijver]

On Wed, Jan 30, 2019 at 10:40:06AM +0800, Joseph Lo wrote:
> On 1/29/19 6:29 PM, Thierry Reding wrote:
> > On Tue, Jan 29, 2019 at 10:41:55AM +0200, Peter De Schrijver wrote:
> > > On Mon, Jan 28, 2019 at 04:09:08PM +0100, Thierry Reding wrote:
> > > 
> > > ...
> > > 
> > > > 
> > > > Up to here this is a duplicate of timer-tegra20.c. And a lot of
> > > > tegra210_timer_init() is the same as tegra20_timer_init() as well. Can't
> > > > we unify the two drivers instead?
> > > > 
> > > > The power cycle restrictions of the architected timer, do they not apply
> > > > to chips earlier than Tegra210 either? So don't we need all of these
> > > > additional features on the timer-tegra20.c driver as well? If so that
> > > 
> > > No. Chips prior to Tegra114 do not have an arch timer and the arch timer
> > > does work correctly on Cortex-A15 so Tegra114 and Tegra124 can use it.
> > > It's broken on Cortex-A57 though, so we can't use it as a wakeup source
> > > on Tegra210.
> > 
> > If chips prior to Tegra114 don't have an architected timer, then we
> > can't remove the timer-tegra20 driver, because we still need it on
> > Tegra20 and Tegra30, right?
> > 
> 
> For Tegra20/30, it's Cortext-A9 with TWD timer. (arch/arm/kernel/smp_twd.c)
> 
> Originally, I thought the functionality of timer-tegra20 would be fully
> replaced by TWD timer driver. But from the log in the kernelci test
> farm[1][2], it looks to me the timer-tegra20 driver still works as
> clocksource driver for Tegra20/30. I cannot confirm if the clock event
> device has been replaced by TWD timer in the log. It could be replaced in
> the background. And by looking into the driver, it should be.

The TWD timer runs from the CPU clock so its frequency changes with
CPU DVFS. That makes it difficult to use.

Peter.

Re: [PATCH V2 2/6] clocksource: tegra: add Tegra210 timer driver

[Peter De Schrijver]

On Mon, Jan 28, 2019 at 04:09:08PM +0100, Thierry Reding wrote:

...

> 
> Up to here this is a duplicate of timer-tegra20.c. And a lot of
> tegra210_timer_init() is the same as tegra20_timer_init() as well. Can't
> we unify the two drivers instead?
> 
> The power cycle restrictions of the architected timer, do they not apply
> to chips earlier than Tegra210 either? So don't we need all of these
> additional features on the timer-tegra20.c driver as well? If so that

No. Chips prior to Tegra114 do not have an arch timer and the arch timer
does work correctly on Cortex-A15 so Tegra114 and Tegra124 can use it.
It's broken on Cortex-A57 though, so we can't use it as a wakeup source
on Tegra210.

Peter.

Re: [PATCH v1 2/3] clk: tegra: ignore unused vfir clock shared with uartb

[Peter De Schrijver]

On Thu, Nov 01, 2018 at 02:52:29AM +0100, Marcel Ziswiler wrote:
> From: Marcel Ziswiler 
> 
> As UARTB and VFIR share their clock enable bit it is rather unwise for
> the kernel to turn off the VFIR one should that be unused (and
> potentially vice versa but so far there anyway is no VFIR driver).
> 
> Without this patch trying to use UARTB with the regular 8250 driver
> will freeze as soon as ttyS1 is accessed after boot. Luckily, using the
> high-speed Tegra serial driver won't exhibit the issue as clocks are
> dynamically enabled/disabled on every access.
> 
> This has been reproduced both on Apalis T30 as well as Apalis TK1 but
> may be an issue on all Tegra UARTB's which share the clock enable with
> VFIR.
> 

Ah.. the correct fix for this is to initialize the enable_refcnt based on the
hw state. This is done in 9619dba8325fce098bbc9ee2911d1b0150fec0c9 for
periph gate clocks, but obviously also applies to normal periph clocks.

Peter.

> Reported-by: Kory Swain 
> Signed-off-by: Marcel Ziswiler 
> 
> ---
> 
>  drivers/clk/tegra/clk-tegra-periph.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra-periph.c 
> b/drivers/clk/tegra/clk-tegra-periph.c
> index cc5275ec2c01..116c74340fb7 100644
> --- a/drivers/clk/tegra/clk-tegra-periph.c
> +++ b/drivers/clk/tegra/clk-tegra-periph.c
> @@ -668,7 +668,7 @@ static struct tegra_periph_init_data periph_clks[] = {
>   MUX("hda", mux_pllp_pllc_clkm, CLK_SOURCE_HDA, 125, 
> TEGRA_PERIPH_ON_APB, tegra_clk_hda_8),
>   MUX("hda2codec_2x", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_HDA2CODEC_2X, 
> 111, TEGRA_PERIPH_ON_APB, tegra_clk_hda2codec_2x),
>   MUX8("hda2codec_2x", mux_pllp_pllc_plla_clkm, CLK_SOURCE_HDA2CODEC_2X, 
> 111, TEGRA_PERIPH_ON_APB, tegra_clk_hda2codec_2x_8),
> - MUX("vfir", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VFIR, 7, 
> TEGRA_PERIPH_ON_APB, tegra_clk_vfir),
> + MUX_FLAGS("vfir", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VFIR, 7, 
> TEGRA_PERIPH_ON_APB, tegra_clk_vfir, CLK_IGNORE_UNUSED),
>   MUX("sdmmc1", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_SDMMC1, 14, 
> TEGRA_PERIPH_ON_APB, tegra_clk_sdmmc1),
>   MUX("sdmmc2", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_SDMMC2, 9, 
> TEGRA_PERIPH_ON_APB, tegra_clk_sdmmc2),
>   MUX("sdmmc3", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_SDMMC3, 69, 
> TEGRA_PERIPH_ON_APB, tegra_clk_sdmmc3),
> -- 
> 2.14.5
> 

Re: [PATCH v1 2/3] clk: tegra: ignore unused vfir clock shared with uartb

[Peter De Schrijver]

On Thu, Nov 01, 2018 at 02:52:29AM +0100, Marcel Ziswiler wrote:
> From: Marcel Ziswiler 
> 
> As UARTB and VFIR share their clock enable bit it is rather unwise for
> the kernel to turn off the VFIR one should that be unused (and
> potentially vice versa but so far there anyway is no VFIR driver).
> 
> Without this patch trying to use UARTB with the regular 8250 driver
> will freeze as soon as ttyS1 is accessed after boot. Luckily, using the
> high-speed Tegra serial driver won't exhibit the issue as clocks are
> dynamically enabled/disabled on every access.
> 
> This has been reproduced both on Apalis T30 as well as Apalis TK1 but
> may be an issue on all Tegra UARTB's which share the clock enable with
> VFIR.
> 

Ah.. the correct fix for this is to initialize the enable_refcnt based on the
hw state. This is done in 9619dba8325fce098bbc9ee2911d1b0150fec0c9 for
periph gate clocks, but obviously also applies to normal periph clocks.

Peter.

> Reported-by: Kory Swain 
> Signed-off-by: Marcel Ziswiler 
> 
> ---
> 
>  drivers/clk/tegra/clk-tegra-periph.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra-periph.c 
> b/drivers/clk/tegra/clk-tegra-periph.c
> index cc5275ec2c01..116c74340fb7 100644
> --- a/drivers/clk/tegra/clk-tegra-periph.c
> +++ b/drivers/clk/tegra/clk-tegra-periph.c
> @@ -668,7 +668,7 @@ static struct tegra_periph_init_data periph_clks[] = {
>   MUX("hda", mux_pllp_pllc_clkm, CLK_SOURCE_HDA, 125, 
> TEGRA_PERIPH_ON_APB, tegra_clk_hda_8),
>   MUX("hda2codec_2x", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_HDA2CODEC_2X, 
> 111, TEGRA_PERIPH_ON_APB, tegra_clk_hda2codec_2x),
>   MUX8("hda2codec_2x", mux_pllp_pllc_plla_clkm, CLK_SOURCE_HDA2CODEC_2X, 
> 111, TEGRA_PERIPH_ON_APB, tegra_clk_hda2codec_2x_8),
> - MUX("vfir", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VFIR, 7, 
> TEGRA_PERIPH_ON_APB, tegra_clk_vfir),
> + MUX_FLAGS("vfir", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VFIR, 7, 
> TEGRA_PERIPH_ON_APB, tegra_clk_vfir, CLK_IGNORE_UNUSED),
>   MUX("sdmmc1", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_SDMMC1, 14, 
> TEGRA_PERIPH_ON_APB, tegra_clk_sdmmc1),
>   MUX("sdmmc2", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_SDMMC2, 9, 
> TEGRA_PERIPH_ON_APB, tegra_clk_sdmmc2),
>   MUX("sdmmc3", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_SDMMC3, 69, 
> TEGRA_PERIPH_ON_APB, tegra_clk_sdmmc3),
> -- 
> 2.14.5
> 

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[Peter De Schrijver]

On Mon, Sep 24, 2018 at 03:18:04PM -0400, r yang wrote:
> On Mon, Sep 24, 2018 at 11:08:03AM +0300, Peter De Schrijver wrote:
> > On Fri, Sep 21, 2018 at 06:01:49PM -0400, ryang wrote:
> > > The current behavior is that clk_round_rate would return the same clock
> > > rate passed to it for valid PLL configurations. This change will return
> > > the exact rate the PLL will provide in accordance with clk API.
> > > 
> > > Signed-off-by: ryang 
> > > ---
> > >  drivers/clk/tegra/clk-pll.c | 7 ++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> > > index 17a058c3bbc1..36014a6ec42e 100644
> > > --- a/drivers/clk/tegra/clk-pll.c
> > > +++ b/drivers/clk/tegra/clk-pll.c
> > > @@ -595,7 +595,12 @@ static int _calc_rate(struct clk_hw *hw, struct 
> > > tegra_clk_pll_freq_table *cfg,
> > >   return -EINVAL;
> > >   }
> > >  
> > > - cfg->output_rate >>= p_div;
> > > + if (cfg->m == 0) {
> > > + cfg->output_rate = 0;
> > 
> > I think a WARN_ON() is appropriate here. the input divider should never be 
> > 0.
> > 
> > Peter.
> > 
> 
> Should it return -EINVAL (or some error) too? _calc_rate is also in the
> clk_set_rate code path. I think we want to avoid programming the
> register to 0 input divider all together?
> 

Yes. writing 0 to the input divider is usually not allowed. In some cases it's
equivalent to writing 1, but better not count on that.

Peter.

> > > + } else {
> > > + cfg->output_rate = cfg->n * DIV_ROUND_UP(parent_rate, cfg->m);
> > > + cfg->output_rate >>= p_div;
> > > + }
> > >  
> > >   if (pll->params->pdiv_tohw) {
> > >   ret = _p_div_to_hw(hw, 1 << p_div);
> > > -- 
> > > 2.17.1
> > > 

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[Peter De Schrijver]

On Mon, Sep 24, 2018 at 03:18:04PM -0400, r yang wrote:
> On Mon, Sep 24, 2018 at 11:08:03AM +0300, Peter De Schrijver wrote:
> > On Fri, Sep 21, 2018 at 06:01:49PM -0400, ryang wrote:
> > > The current behavior is that clk_round_rate would return the same clock
> > > rate passed to it for valid PLL configurations. This change will return
> > > the exact rate the PLL will provide in accordance with clk API.
> > > 
> > > Signed-off-by: ryang 
> > > ---
> > >  drivers/clk/tegra/clk-pll.c | 7 ++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> > > index 17a058c3bbc1..36014a6ec42e 100644
> > > --- a/drivers/clk/tegra/clk-pll.c
> > > +++ b/drivers/clk/tegra/clk-pll.c
> > > @@ -595,7 +595,12 @@ static int _calc_rate(struct clk_hw *hw, struct 
> > > tegra_clk_pll_freq_table *cfg,
> > >   return -EINVAL;
> > >   }
> > >  
> > > - cfg->output_rate >>= p_div;
> > > + if (cfg->m == 0) {
> > > + cfg->output_rate = 0;
> > 
> > I think a WARN_ON() is appropriate here. the input divider should never be 
> > 0.
> > 
> > Peter.
> > 
> 
> Should it return -EINVAL (or some error) too? _calc_rate is also in the
> clk_set_rate code path. I think we want to avoid programming the
> register to 0 input divider all together?
> 

Yes. writing 0 to the input divider is usually not allowed. In some cases it's
equivalent to writing 1, but better not count on that.

Peter.

> > > + } else {
> > > + cfg->output_rate = cfg->n * DIV_ROUND_UP(parent_rate, cfg->m);
> > > + cfg->output_rate >>= p_div;
> > > + }
> > >  
> > >   if (pll->params->pdiv_tohw) {
> > >   ret = _p_div_to_hw(hw, 1 << p_div);
> > > -- 
> > > 2.17.1
> > > 

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[Peter De Schrijver]

On Fri, Sep 21, 2018 at 06:01:49PM -0400, ryang wrote:
> The current behavior is that clk_round_rate would return the same clock
> rate passed to it for valid PLL configurations. This change will return
> the exact rate the PLL will provide in accordance with clk API.
> 
> Signed-off-by: ryang 
> ---
>  drivers/clk/tegra/clk-pll.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 17a058c3bbc1..36014a6ec42e 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -595,7 +595,12 @@ static int _calc_rate(struct clk_hw *hw, struct 
> tegra_clk_pll_freq_table *cfg,
>   return -EINVAL;
>   }
>  
> - cfg->output_rate >>= p_div;
> + if (cfg->m == 0) {
> + cfg->output_rate = 0;

I think a WARN_ON() is appropriate here. the input divider should never be 0.

Peter.

> + } else {
> + cfg->output_rate = cfg->n * DIV_ROUND_UP(parent_rate, cfg->m);
> + cfg->output_rate >>= p_div;
> + }
>  
>   if (pll->params->pdiv_tohw) {
>   ret = _p_div_to_hw(hw, 1 << p_div);
> -- 
> 2.17.1
> 

Re: [PATCH] clk: tegra: Return the exact clock rate from clk_round_rate

[Peter De Schrijver]

On Fri, Sep 21, 2018 at 06:01:49PM -0400, ryang wrote:
> The current behavior is that clk_round_rate would return the same clock
> rate passed to it for valid PLL configurations. This change will return
> the exact rate the PLL will provide in accordance with clk API.
> 
> Signed-off-by: ryang 
> ---
>  drivers/clk/tegra/clk-pll.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 17a058c3bbc1..36014a6ec42e 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -595,7 +595,12 @@ static int _calc_rate(struct clk_hw *hw, struct 
> tegra_clk_pll_freq_table *cfg,
>   return -EINVAL;
>   }
>  
> - cfg->output_rate >>= p_div;
> + if (cfg->m == 0) {
> + cfg->output_rate = 0;

I think a WARN_ON() is appropriate here. the input divider should never be 0.

Peter.

> + } else {
> + cfg->output_rate = cfg->n * DIV_ROUND_UP(parent_rate, cfg->m);
> + cfg->output_rate >>= p_div;
> + }
>  
>   if (pll->params->pdiv_tohw) {
>   ret = _p_div_to_hw(hw, 1 << p_div);
> -- 
> 2.17.1
> 

Re: [PATCH] clk: tegra: Fix an infinite loop when clock rate is zero

[Peter De Schrijver]

On Fri, Sep 21, 2018 at 06:00:37PM -0400, ryang wrote:
> Calling clk_set_rate or clk_round_rate will lock up the kernel when the
> rate is zero. This avoids the infinite loop and uses a slightly more
> optimized p divider calculation.
> 

Acked-By: Peter De Schrijver 

At some point we should also limit pdiv to its maximum possible value, but
that's not so obvious as we need to take into account PLLs where pdiv is
non-linear.

Peter.

> Signed-off-by: ryang 
> ---
>  drivers/clk/tegra/clk-pll.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 830d1c87fa7c..17a058c3bbc1 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -582,9 +582,8 @@ static int _calc_rate(struct clk_hw *hw, struct 
> tegra_clk_pll_freq_table *cfg,
>   }
>  
>   /* Raise VCO to guarantee 0.5% accuracy */
> - for (cfg->output_rate = rate; cfg->output_rate < 200 * cfreq;
> -  cfg->output_rate <<= 1)
> - p_div++;
> + p_div = rate ? fls((200 * cfreq) / rate) : 0;
> + cfg->output_rate = rate << p_div;
>  
>   cfg->m = parent_rate / cfreq;
>   cfg->n = cfg->output_rate / cfreq;
> -- 
> 2.17.1
> 

Re: [PATCH] clk: tegra: Fix an infinite loop when clock rate is zero

[Peter De Schrijver]

On Fri, Sep 21, 2018 at 06:00:37PM -0400, ryang wrote:
> Calling clk_set_rate or clk_round_rate will lock up the kernel when the
> rate is zero. This avoids the infinite loop and uses a slightly more
> optimized p divider calculation.
> 

Acked-By: Peter De Schrijver 

At some point we should also limit pdiv to its maximum possible value, but
that's not so obvious as we need to take into account PLLs where pdiv is
non-linear.

Peter.

> Signed-off-by: ryang 
> ---
>  drivers/clk/tegra/clk-pll.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> index 830d1c87fa7c..17a058c3bbc1 100644
> --- a/drivers/clk/tegra/clk-pll.c
> +++ b/drivers/clk/tegra/clk-pll.c
> @@ -582,9 +582,8 @@ static int _calc_rate(struct clk_hw *hw, struct 
> tegra_clk_pll_freq_table *cfg,
>   }
>  
>   /* Raise VCO to guarantee 0.5% accuracy */
> - for (cfg->output_rate = rate; cfg->output_rate < 200 * cfreq;
> -  cfg->output_rate <<= 1)
> - p_div++;
> + p_div = rate ? fls((200 * cfreq) / rate) : 0;
> + cfg->output_rate = rate << p_div;
>  
>   cfg->m = parent_rate / cfreq;
>   cfg->n = cfg->output_rate / cfreq;
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

[Peter De Schrijver]

On Fri, Aug 31, 2018 at 12:45:17PM +0300, Dmitry Osipenko wrote:
> On 8/31/18 12:29 PM, Peter De Schrijver wrote:
> > On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
> >> Currently all PLL's on Tegra20 use a hardcoded delay despite of having
> >> a lock-status bit. The lock-status polling was disabled ~7 years ago
> >> because PLLE was failing to lock and was a suspicion that other PLLs
> >> might be faulty too. Other PLLs are okay, hence enable the lock-status
> >> polling for them. This reduces delay of any operation that require PLL
> >> to lock.
> >>
> >> Signed-off-by: Dmitry Osipenko 
> >> ---
> >>
> >> Changelog:
> >>
> >> v2:Don't enable polling for PLLE as it known to not being able to 
> >> lock.
> >>
> > 
> > This isn't correct. The lock bit of PLLE can declare lock too early, but the
> > PLL itself does lock.
> 
> Indeed, it locks but can't be polled for the lock-status as it doesn't have 
> the
> lock-status bit.
> 
> Do you want me to adjust the commit description or it is fine as is?
> 

I think it's better to adjust it.

> It is also a bit odd that PLLE has "lock_delay = 0", is it correct?

That seems odd yes..

Peter.

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

[Peter De Schrijver]

On Fri, Aug 31, 2018 at 12:45:17PM +0300, Dmitry Osipenko wrote:
> On 8/31/18 12:29 PM, Peter De Schrijver wrote:
> > On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
> >> Currently all PLL's on Tegra20 use a hardcoded delay despite of having
> >> a lock-status bit. The lock-status polling was disabled ~7 years ago
> >> because PLLE was failing to lock and was a suspicion that other PLLs
> >> might be faulty too. Other PLLs are okay, hence enable the lock-status
> >> polling for them. This reduces delay of any operation that require PLL
> >> to lock.
> >>
> >> Signed-off-by: Dmitry Osipenko 
> >> ---
> >>
> >> Changelog:
> >>
> >> v2:Don't enable polling for PLLE as it known to not being able to 
> >> lock.
> >>
> > 
> > This isn't correct. The lock bit of PLLE can declare lock too early, but the
> > PLL itself does lock.
> 
> Indeed, it locks but can't be polled for the lock-status as it doesn't have 
> the
> lock-status bit.
> 
> Do you want me to adjust the commit description or it is fine as is?
> 

I think it's better to adjust it.

> It is also a bit odd that PLLE has "lock_delay = 0", is it correct?

That seems odd yes..

Peter.

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

[Peter De Schrijver]

On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
> Currently all PLL's on Tegra20 use a hardcoded delay despite of having
> a lock-status bit. The lock-status polling was disabled ~7 years ago
> because PLLE was failing to lock and was a suspicion that other PLLs
> might be faulty too. Other PLLs are okay, hence enable the lock-status
> polling for them. This reduces delay of any operation that require PLL
> to lock.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
> 
> Changelog:
> 
> v2:   Don't enable polling for PLLE as it known to not being able to lock.
> 

This isn't correct. The lock bit of PLLE can declare lock too early, but the
PLL itself does lock.

>  drivers/clk/tegra/clk-tegra20.c | 20 +---
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index cc857d4d4a86..cfde3745a0db 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -298,7 +298,8 @@ static struct tegra_clk_pll_params pll_c_params = {
>   .lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
>   .lock_delay = 300,
>   .freq_table = pll_c_freq_table,
> - .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static struct tegra_clk_pll_params pll_m_params = {
> @@ -314,7 +315,8 @@ static struct tegra_clk_pll_params pll_m_params = {
>   .lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
>   .lock_delay = 300,
>   .freq_table = pll_m_freq_table,
> - .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static struct tegra_clk_pll_params pll_p_params = {
> @@ -331,7 +333,7 @@ static struct tegra_clk_pll_params pll_p_params = {
>   .lock_delay = 300,
>   .freq_table = pll_p_freq_table,
>   .flags = TEGRA_PLL_FIXED | TEGRA_PLL_HAS_CPCON |
> -  TEGRA_PLL_HAS_LOCK_ENABLE,
> +  TEGRA_PLL_HAS_LOCK_ENABLE | TEGRA_PLL_USE_LOCK,
>   .fixed_rate =  21600,
>  };
>  
> @@ -348,7 +350,8 @@ static struct tegra_clk_pll_params pll_a_params = {
>   .lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
>   .lock_delay = 300,
>   .freq_table = pll_a_freq_table,
> - .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static struct tegra_clk_pll_params pll_d_params = {
> @@ -364,7 +367,8 @@ static struct tegra_clk_pll_params pll_d_params = {
>   .lock_enable_bit_idx = PLLDU_MISC_LOCK_ENABLE,
>   .lock_delay = 1000,
>   .freq_table = pll_d_freq_table,
> - .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static const struct pdiv_map pllu_p[] = {
> @@ -387,7 +391,8 @@ static struct tegra_clk_pll_params pll_u_params = {
>   .lock_delay = 1000,
>   .pdiv_tohw = pllu_p,
>   .freq_table = pll_u_freq_table,
> - .flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static struct tegra_clk_pll_params pll_x_params = {
> @@ -403,7 +408,8 @@ static struct tegra_clk_pll_params pll_x_params = {
>   .lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
>   .lock_delay = 300,
>   .freq_table = pll_x_freq_table,
> - .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static struct tegra_clk_pll_params pll_e_params = {
> -- 
> 2.18.0
> 

Re: [PATCH v2 2/2] clk: tegra20: Enable lock-status polling for PLLs

[Peter De Schrijver]

On Thu, Aug 30, 2018 at 09:42:10PM +0300, Dmitry Osipenko wrote:
> Currently all PLL's on Tegra20 use a hardcoded delay despite of having
> a lock-status bit. The lock-status polling was disabled ~7 years ago
> because PLLE was failing to lock and was a suspicion that other PLLs
> might be faulty too. Other PLLs are okay, hence enable the lock-status
> polling for them. This reduces delay of any operation that require PLL
> to lock.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
> 
> Changelog:
> 
> v2:   Don't enable polling for PLLE as it known to not being able to lock.
> 

This isn't correct. The lock bit of PLLE can declare lock too early, but the
PLL itself does lock.

>  drivers/clk/tegra/clk-tegra20.c | 20 +---
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index cc857d4d4a86..cfde3745a0db 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -298,7 +298,8 @@ static struct tegra_clk_pll_params pll_c_params = {
>   .lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
>   .lock_delay = 300,
>   .freq_table = pll_c_freq_table,
> - .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static struct tegra_clk_pll_params pll_m_params = {
> @@ -314,7 +315,8 @@ static struct tegra_clk_pll_params pll_m_params = {
>   .lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
>   .lock_delay = 300,
>   .freq_table = pll_m_freq_table,
> - .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static struct tegra_clk_pll_params pll_p_params = {
> @@ -331,7 +333,7 @@ static struct tegra_clk_pll_params pll_p_params = {
>   .lock_delay = 300,
>   .freq_table = pll_p_freq_table,
>   .flags = TEGRA_PLL_FIXED | TEGRA_PLL_HAS_CPCON |
> -  TEGRA_PLL_HAS_LOCK_ENABLE,
> +  TEGRA_PLL_HAS_LOCK_ENABLE | TEGRA_PLL_USE_LOCK,
>   .fixed_rate =  21600,
>  };
>  
> @@ -348,7 +350,8 @@ static struct tegra_clk_pll_params pll_a_params = {
>   .lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
>   .lock_delay = 300,
>   .freq_table = pll_a_freq_table,
> - .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static struct tegra_clk_pll_params pll_d_params = {
> @@ -364,7 +367,8 @@ static struct tegra_clk_pll_params pll_d_params = {
>   .lock_enable_bit_idx = PLLDU_MISC_LOCK_ENABLE,
>   .lock_delay = 1000,
>   .freq_table = pll_d_freq_table,
> - .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static const struct pdiv_map pllu_p[] = {
> @@ -387,7 +391,8 @@ static struct tegra_clk_pll_params pll_u_params = {
>   .lock_delay = 1000,
>   .pdiv_tohw = pllu_p,
>   .freq_table = pll_u_freq_table,
> - .flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLLU | TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static struct tegra_clk_pll_params pll_x_params = {
> @@ -403,7 +408,8 @@ static struct tegra_clk_pll_params pll_x_params = {
>   .lock_enable_bit_idx = PLL_MISC_LOCK_ENABLE,
>   .lock_delay = 300,
>   .freq_table = pll_x_freq_table,
> - .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE,
> + .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_HAS_LOCK_ENABLE |
> +  TEGRA_PLL_USE_LOCK,
>  };
>  
>  static struct tegra_clk_pll_params pll_e_params = {
> -- 
> 2.18.0
> 

Re: [PATCH 2/8] clk: tegra: host1x has fractional divider

[Peter De Schrijver]

On Mon, Jul 23, 2018 at 10:32:58AM +0100, Ben Dooks wrote:
> 
> 
> On 2018-07-23 09:50, Peter De Schrijver wrote:
> >On Fri, Jul 20, 2018 at 02:45:26PM +0100, Ben Dooks wrote:
> >>The host1x clock according to both tegra2 and tegra3 manuals is
> >>an 8bit divider with lsb being fractional. This is running into
> >>an issue where the host1x is being set on a tegra20a system to
> >>266.4MHz but ends up at 222MHz instead.
> >>
> >
> >The fact the hw has a fractional divider, does not mean we're
> >allowed to use
> >it. Due to the non 50% duty cycle of fractional divided clocks,
> >they are not
> >allowed for certain peripherals. Do you have information
> >indicating this is
> >ok for the host1x clock?
> 
> Only that's what was setup for the systems we're using.
> We couldn't match the 2.6 working system without these changes.
> 

On Tegra20 or Tegra30?

Peter.

Re: [PATCH 2/8] clk: tegra: host1x has fractional divider

[Peter De Schrijver]

On Mon, Jul 23, 2018 at 10:32:58AM +0100, Ben Dooks wrote:
> 
> 
> On 2018-07-23 09:50, Peter De Schrijver wrote:
> >On Fri, Jul 20, 2018 at 02:45:26PM +0100, Ben Dooks wrote:
> >>The host1x clock according to both tegra2 and tegra3 manuals is
> >>an 8bit divider with lsb being fractional. This is running into
> >>an issue where the host1x is being set on a tegra20a system to
> >>266.4MHz but ends up at 222MHz instead.
> >>
> >
> >The fact the hw has a fractional divider, does not mean we're
> >allowed to use
> >it. Due to the non 50% duty cycle of fractional divided clocks,
> >they are not
> >allowed for certain peripherals. Do you have information
> >indicating this is
> >ok for the host1x clock?
> 
> Only that's what was setup for the systems we're using.
> We couldn't match the 2.6 working system without these changes.
> 

On Tegra20 or Tegra30?

Peter.

Re: [PATCH 3/8] clk: tegra: fix fractional clocks for VDI, VI, EPP, MPE, 2D and 3D

[Peter De Schrijver]

On Fri, Jul 20, 2018 at 02:45:27PM +0100, Ben Dooks wrote:
> The clocks vde, vi, epp, mpe, 2d and 3d are all fractional
> divisors, and not integer divisors as setup in the current
> kernel. This seems to be the same for tegra2 and tegra3.
> 

Same comment as the host1x clock patch.

> Signed-off-by: Ben Dooks 
> ---
>  drivers/clk/tegra/clk-tegra-periph.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra-periph.c 
> b/drivers/clk/tegra/clk-tegra-periph.c
> index 8fa1cecf18a0..ed70419f4ff9 100644
> --- a/drivers/clk/tegra/clk-tegra-periph.c
> +++ b/drivers/clk/tegra/clk-tegra-periph.c
> @@ -641,13 +641,13 @@ static struct tegra_periph_init_data periph_clks[] = {
>   I2C("i2c4", mux_pllp_clkm, CLK_SOURCE_I2C4, 103, tegra_clk_i2c4),
>   I2C("i2c5", mux_pllp_clkm, CLK_SOURCE_I2C5, 47, tegra_clk_i2c5),
>   I2C("i2c6", mux_pllp_clkm, CLK_SOURCE_I2C6, 166, tegra_clk_i2c6),
> - INT("vde", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VDE, 61, 0, 
> tegra_clk_vde),
> - INT("vi", mux_pllm_pllc_pllp_plla, CLK_SOURCE_VI, 20, 0, tegra_clk_vi),
> - INT("epp", mux_pllm_pllc_pllp_plla, CLK_SOURCE_EPP, 19, 0, 
> tegra_clk_epp),
> + MUX("vde", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VDE, 61, 0, 
> tegra_clk_vde),
> + MUX("vi", mux_pllm_pllc_pllp_plla, CLK_SOURCE_VI, 20, 0, tegra_clk_vi),
> + MUX("epp", mux_pllm_pllc_pllp_plla, CLK_SOURCE_EPP, 19, 0, 
> tegra_clk_epp),
>   MUX("host1x", mux_pllm_pllc_pllp_plla, CLK_SOURCE_HOST1X, 28, 0, 
> tegra_clk_host1x),
> - INT("mpe", mux_pllm_pllc_pllp_plla, CLK_SOURCE_MPE, 60, 0, 
> tegra_clk_mpe),
> - INT("2d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_2D, 21, 0, 
> tegra_clk_gr2d),
> - INT("3d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_3D, 24, 0, 
> tegra_clk_gr3d),
> + MUX("mpe", mux_pllm_pllc_pllp_plla, CLK_SOURCE_MPE, 60, 0, 
> tegra_clk_mpe),
> + MUX("2d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_2D, 21, 0, 
> tegra_clk_gr2d),
> + MUX("3d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_3D, 24, 0, 
> tegra_clk_gr3d),
>   INT8("vde", mux_pllp_pllc2_c_c3_pllm_clkm, CLK_SOURCE_VDE, 61, 0, 
> tegra_clk_vde_8),
>   INT8("vi", mux_pllm_pllc2_c_c3_pllp_plla, CLK_SOURCE_VI, 20, 0, 
> tegra_clk_vi_8),
>   INT8("vi", mux_pllm_pllc2_c_c3_pllp_plla_pllc4, CLK_SOURCE_VI, 20, 0, 
> tegra_clk_vi_9),
> -- 
> 2.18.0
> 

Re: [PATCH 3/8] clk: tegra: fix fractional clocks for VDI, VI, EPP, MPE, 2D and 3D

[Peter De Schrijver]

On Fri, Jul 20, 2018 at 02:45:27PM +0100, Ben Dooks wrote:
> The clocks vde, vi, epp, mpe, 2d and 3d are all fractional
> divisors, and not integer divisors as setup in the current
> kernel. This seems to be the same for tegra2 and tegra3.
> 

Same comment as the host1x clock patch.

> Signed-off-by: Ben Dooks 
> ---
>  drivers/clk/tegra/clk-tegra-periph.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra-periph.c 
> b/drivers/clk/tegra/clk-tegra-periph.c
> index 8fa1cecf18a0..ed70419f4ff9 100644
> --- a/drivers/clk/tegra/clk-tegra-periph.c
> +++ b/drivers/clk/tegra/clk-tegra-periph.c
> @@ -641,13 +641,13 @@ static struct tegra_periph_init_data periph_clks[] = {
>   I2C("i2c4", mux_pllp_clkm, CLK_SOURCE_I2C4, 103, tegra_clk_i2c4),
>   I2C("i2c5", mux_pllp_clkm, CLK_SOURCE_I2C5, 47, tegra_clk_i2c5),
>   I2C("i2c6", mux_pllp_clkm, CLK_SOURCE_I2C6, 166, tegra_clk_i2c6),
> - INT("vde", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VDE, 61, 0, 
> tegra_clk_vde),
> - INT("vi", mux_pllm_pllc_pllp_plla, CLK_SOURCE_VI, 20, 0, tegra_clk_vi),
> - INT("epp", mux_pllm_pllc_pllp_plla, CLK_SOURCE_EPP, 19, 0, 
> tegra_clk_epp),
> + MUX("vde", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VDE, 61, 0, 
> tegra_clk_vde),
> + MUX("vi", mux_pllm_pllc_pllp_plla, CLK_SOURCE_VI, 20, 0, tegra_clk_vi),
> + MUX("epp", mux_pllm_pllc_pllp_plla, CLK_SOURCE_EPP, 19, 0, 
> tegra_clk_epp),
>   MUX("host1x", mux_pllm_pllc_pllp_plla, CLK_SOURCE_HOST1X, 28, 0, 
> tegra_clk_host1x),
> - INT("mpe", mux_pllm_pllc_pllp_plla, CLK_SOURCE_MPE, 60, 0, 
> tegra_clk_mpe),
> - INT("2d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_2D, 21, 0, 
> tegra_clk_gr2d),
> - INT("3d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_3D, 24, 0, 
> tegra_clk_gr3d),
> + MUX("mpe", mux_pllm_pllc_pllp_plla, CLK_SOURCE_MPE, 60, 0, 
> tegra_clk_mpe),
> + MUX("2d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_2D, 21, 0, 
> tegra_clk_gr2d),
> + MUX("3d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_3D, 24, 0, 
> tegra_clk_gr3d),
>   INT8("vde", mux_pllp_pllc2_c_c3_pllm_clkm, CLK_SOURCE_VDE, 61, 0, 
> tegra_clk_vde_8),
>   INT8("vi", mux_pllm_pllc2_c_c3_pllp_plla, CLK_SOURCE_VI, 20, 0, 
> tegra_clk_vi_8),
>   INT8("vi", mux_pllm_pllc2_c_c3_pllp_plla_pllc4, CLK_SOURCE_VI, 20, 0, 
> tegra_clk_vi_9),
> -- 
> 2.18.0
> 

Re: [PATCH 2/8] clk: tegra: host1x has fractional divider

[Peter De Schrijver]

On Fri, Jul 20, 2018 at 02:45:26PM +0100, Ben Dooks wrote:
> The host1x clock according to both tegra2 and tegra3 manuals is
> an 8bit divider with lsb being fractional. This is running into
> an issue where the host1x is being set on a tegra20a system to
> 266.4MHz but ends up at 222MHz instead.
> 

The fact the hw has a fractional divider, does not mean we're allowed to use
it. Due to the non 50% duty cycle of fractional divided clocks, they are not
allowed for certain peripherals. Do you have information indicating this is
ok for the host1x clock?

Peter.


> Signed-off-by: Ben Dooks 
> ---
>  drivers/clk/tegra/clk-tegra-periph.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra-periph.c 
> b/drivers/clk/tegra/clk-tegra-periph.c
> index 2acba2986bc6..8fa1cecf18a0 100644
> --- a/drivers/clk/tegra/clk-tegra-periph.c
> +++ b/drivers/clk/tegra/clk-tegra-periph.c
> @@ -644,7 +644,7 @@ static struct tegra_periph_init_data periph_clks[] = {
>   INT("vde", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VDE, 61, 0, 
> tegra_clk_vde),
>   INT("vi", mux_pllm_pllc_pllp_plla, CLK_SOURCE_VI, 20, 0, tegra_clk_vi),
>   INT("epp", mux_pllm_pllc_pllp_plla, CLK_SOURCE_EPP, 19, 0, 
> tegra_clk_epp),
> - INT("host1x", mux_pllm_pllc_pllp_plla, CLK_SOURCE_HOST1X, 28, 0, 
> tegra_clk_host1x),
> + MUX("host1x", mux_pllm_pllc_pllp_plla, CLK_SOURCE_HOST1X, 28, 0, 
> tegra_clk_host1x),
>   INT("mpe", mux_pllm_pllc_pllp_plla, CLK_SOURCE_MPE, 60, 0, 
> tegra_clk_mpe),
>   INT("2d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_2D, 21, 0, 
> tegra_clk_gr2d),
>   INT("3d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_3D, 24, 0, 
> tegra_clk_gr3d),
> -- 
> 2.18.0
> 

Re: [PATCH 2/8] clk: tegra: host1x has fractional divider

[Peter De Schrijver]

On Fri, Jul 20, 2018 at 02:45:26PM +0100, Ben Dooks wrote:
> The host1x clock according to both tegra2 and tegra3 manuals is
> an 8bit divider with lsb being fractional. This is running into
> an issue where the host1x is being set on a tegra20a system to
> 266.4MHz but ends up at 222MHz instead.
> 

The fact the hw has a fractional divider, does not mean we're allowed to use
it. Due to the non 50% duty cycle of fractional divided clocks, they are not
allowed for certain peripherals. Do you have information indicating this is
ok for the host1x clock?

Peter.


> Signed-off-by: Ben Dooks 
> ---
>  drivers/clk/tegra/clk-tegra-periph.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra-periph.c 
> b/drivers/clk/tegra/clk-tegra-periph.c
> index 2acba2986bc6..8fa1cecf18a0 100644
> --- a/drivers/clk/tegra/clk-tegra-periph.c
> +++ b/drivers/clk/tegra/clk-tegra-periph.c
> @@ -644,7 +644,7 @@ static struct tegra_periph_init_data periph_clks[] = {
>   INT("vde", mux_pllp_pllc_pllm_clkm, CLK_SOURCE_VDE, 61, 0, 
> tegra_clk_vde),
>   INT("vi", mux_pllm_pllc_pllp_plla, CLK_SOURCE_VI, 20, 0, tegra_clk_vi),
>   INT("epp", mux_pllm_pllc_pllp_plla, CLK_SOURCE_EPP, 19, 0, 
> tegra_clk_epp),
> - INT("host1x", mux_pllm_pllc_pllp_plla, CLK_SOURCE_HOST1X, 28, 0, 
> tegra_clk_host1x),
> + MUX("host1x", mux_pllm_pllc_pllp_plla, CLK_SOURCE_HOST1X, 28, 0, 
> tegra_clk_host1x),
>   INT("mpe", mux_pllm_pllc_pllp_plla, CLK_SOURCE_MPE, 60, 0, 
> tegra_clk_mpe),
>   INT("2d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_2D, 21, 0, 
> tegra_clk_gr2d),
>   INT("3d", mux_pllm_pllc_pllp_plla, CLK_SOURCE_3D, 24, 0, 
> tegra_clk_gr3d),
> -- 
> 2.18.0
> 

Re: [PATCH v5 3/4] clk: tegra: Add sdmmc mux divider clock

[Peter De Schrijver]

On Thu, Jul 12, 2018 at 11:52:31AM +0100, Jon Hunter wrote:
> 
> On 11/07/18 15:39, Aapo Vienamo wrote:
> > From: Peter De-Schrijver 
> > 
> > Add a clock type to model the sdmmc switch divider clocks which have paths
> > to source clocks bypassing the divider (Low Jitter paths). These
> > are handled by selecting the lj path when the divider is 1 (ie the
> > rate is the parent rate), otherwise the normal path with divider
> > will be selected. Otherwise this clock behaves as a normal peripheral
> > clock.
> > 
> > Signed-off-by: Peter De-Schrijver 
> > Signed-off-by: Aapo Vienamo 
> > Acked-by: Peter De Schrijver 
> > ---
> >  drivers/clk/tegra/Makefile|   1 +
> >  drivers/clk/tegra/clk-sdmmc-mux.c | 249 
> > ++
> >  drivers/clk/tegra/clk.h   |  26 
> >  3 files changed, 276 insertions(+)
> >  create mode 100644 drivers/clk/tegra/clk-sdmmc-mux.c
> > 
> > diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> > index c79319d..8975f88 100644
> > --- a/drivers/clk/tegra/Makefile
> > +++ b/drivers/clk/tegra/Makefile
> > @@ -8,6 +8,7 @@ obj-y   += 
> > clk-periph-fixed.o
> >  obj-y  += clk-periph-gate.o
> >  obj-y  += clk-pll.o
> >  obj-y  += clk-pll-out.o
> > +obj-y  += clk-sdmmc-mux.o
> >  obj-y  += clk-super.o
> >  obj-y  += clk-tegra-audio.o
> >  obj-y  += clk-tegra-periph.o
> > diff --git a/drivers/clk/tegra/clk-sdmmc-mux.c 
> > b/drivers/clk/tegra/clk-sdmmc-mux.c
> > new file mode 100644
> > index 000..9566754
> > --- /dev/null
> > +++ b/drivers/clk/tegra/clk-sdmmc-mux.c
> > @@ -0,0 +1,249 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 NVIDIA CORPORATION.  All rights reserved.
> > + *
> > + * based on clk-mux.c
> > + *
> > + * Copyright (C) 2011 Sascha Hauer, Pengutronix 
> > + * Copyright (C) 2011 Richard Zhao, Linaro 
> > + * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd 
> > 
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "clk.h"
> > +
> > +#define DIV_MASK GENMASK(7, 0)
> > +#define MUX_SHIFT 29
> > +#define MUX_MASK GENMASK(MUX_SHIFT + 2, MUX_SHIFT)
> > +
> > +#define get_max_div(d) DIV_MASK
> > +#define get_div_field(val) ((val) & DIV_MASK)
> > +#define get_mux_field(val) (((val) & MUX_MASK) >> MUX_SHIFT)
> > +
> > +static const char * const mux_sdmmc_parents[] = {
> > +   "pll_p", "pll_c4_out2", "pll_c4_out0", "pll_c4_out1", "clk_m"
> > +};
> > +
> > +static const u8 mux_lj_idx[] = {
> > +   [0] = 0, [1] = 1, [2] = 2, [3] = 5, [4] = 6
> > +};
> > +
> > +static const u8 mux_non_lj_idx[] = {
> > +   [0] = 0, [1] = 3, [2] = 7, [3] = 4, [4] = 6
> > +};
> > +
> > +static u8 clk_sdmmc_mux_get_parent(struct clk_hw *hw)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   int num_parents, i;
> > +   u32 src, val;
> > +   const u8 *mux_idx;
> > +
> > +   num_parents = clk_hw_get_num_parents(hw);
> > +
> > +   val = readl_relaxed(sdmmc_mux->reg);
> > +   src = get_mux_field(val);
> > +   if (get_div_field(val))
> > +   mux_idx = mux_non_lj_idx;
> > +   else
> > +   mux_idx = mux_lj_idx;
> > +
> > +   for (i = 0; i < num_parents; i++) {
> > +   if (mux_idx[i] == src)
> > +   return i;
> > +   }
> > +
> > +   WARN(1, "Unknown parent selector %d\n", src);
> > +
> > +   return 0;
> > +}
> > +
> > +static int clk_sdmmc_mux_set_parent(struct clk_hw *hw, u8 index)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   u32 val;
> > +
> > +
> > +   val = readl_relaxed(sdmmc_mux->reg);
> > +   if (get_div_field(val))
> > +   index = mux_non_lj_idx[index];
> > +   else
> > +   index = mux_lj_idx[index];
> > +
> > +   val &= ~MUX_MASK;
> > +   val |= index << MUX_SHIFT;
> > +
> > +   writel(val, sdmmc_mux->reg);
> > +
> > +   return 0;
>

Re: [PATCH v5 3/4] clk: tegra: Add sdmmc mux divider clock

[Peter De Schrijver]

On Thu, Jul 12, 2018 at 11:52:31AM +0100, Jon Hunter wrote:
> 
> On 11/07/18 15:39, Aapo Vienamo wrote:
> > From: Peter De-Schrijver 
> > 
> > Add a clock type to model the sdmmc switch divider clocks which have paths
> > to source clocks bypassing the divider (Low Jitter paths). These
> > are handled by selecting the lj path when the divider is 1 (ie the
> > rate is the parent rate), otherwise the normal path with divider
> > will be selected. Otherwise this clock behaves as a normal peripheral
> > clock.
> > 
> > Signed-off-by: Peter De-Schrijver 
> > Signed-off-by: Aapo Vienamo 
> > Acked-by: Peter De Schrijver 
> > ---
> >  drivers/clk/tegra/Makefile|   1 +
> >  drivers/clk/tegra/clk-sdmmc-mux.c | 249 
> > ++
> >  drivers/clk/tegra/clk.h   |  26 
> >  3 files changed, 276 insertions(+)
> >  create mode 100644 drivers/clk/tegra/clk-sdmmc-mux.c
> > 
> > diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> > index c79319d..8975f88 100644
> > --- a/drivers/clk/tegra/Makefile
> > +++ b/drivers/clk/tegra/Makefile
> > @@ -8,6 +8,7 @@ obj-y   += 
> > clk-periph-fixed.o
> >  obj-y  += clk-periph-gate.o
> >  obj-y  += clk-pll.o
> >  obj-y  += clk-pll-out.o
> > +obj-y  += clk-sdmmc-mux.o
> >  obj-y  += clk-super.o
> >  obj-y  += clk-tegra-audio.o
> >  obj-y  += clk-tegra-periph.o
> > diff --git a/drivers/clk/tegra/clk-sdmmc-mux.c 
> > b/drivers/clk/tegra/clk-sdmmc-mux.c
> > new file mode 100644
> > index 000..9566754
> > --- /dev/null
> > +++ b/drivers/clk/tegra/clk-sdmmc-mux.c
> > @@ -0,0 +1,249 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 NVIDIA CORPORATION.  All rights reserved.
> > + *
> > + * based on clk-mux.c
> > + *
> > + * Copyright (C) 2011 Sascha Hauer, Pengutronix 
> > + * Copyright (C) 2011 Richard Zhao, Linaro 
> > + * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd 
> > 
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "clk.h"
> > +
> > +#define DIV_MASK GENMASK(7, 0)
> > +#define MUX_SHIFT 29
> > +#define MUX_MASK GENMASK(MUX_SHIFT + 2, MUX_SHIFT)
> > +
> > +#define get_max_div(d) DIV_MASK
> > +#define get_div_field(val) ((val) & DIV_MASK)
> > +#define get_mux_field(val) (((val) & MUX_MASK) >> MUX_SHIFT)
> > +
> > +static const char * const mux_sdmmc_parents[] = {
> > +   "pll_p", "pll_c4_out2", "pll_c4_out0", "pll_c4_out1", "clk_m"
> > +};
> > +
> > +static const u8 mux_lj_idx[] = {
> > +   [0] = 0, [1] = 1, [2] = 2, [3] = 5, [4] = 6
> > +};
> > +
> > +static const u8 mux_non_lj_idx[] = {
> > +   [0] = 0, [1] = 3, [2] = 7, [3] = 4, [4] = 6
> > +};
> > +
> > +static u8 clk_sdmmc_mux_get_parent(struct clk_hw *hw)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   int num_parents, i;
> > +   u32 src, val;
> > +   const u8 *mux_idx;
> > +
> > +   num_parents = clk_hw_get_num_parents(hw);
> > +
> > +   val = readl_relaxed(sdmmc_mux->reg);
> > +   src = get_mux_field(val);
> > +   if (get_div_field(val))
> > +   mux_idx = mux_non_lj_idx;
> > +   else
> > +   mux_idx = mux_lj_idx;
> > +
> > +   for (i = 0; i < num_parents; i++) {
> > +   if (mux_idx[i] == src)
> > +   return i;
> > +   }
> > +
> > +   WARN(1, "Unknown parent selector %d\n", src);
> > +
> > +   return 0;
> > +}
> > +
> > +static int clk_sdmmc_mux_set_parent(struct clk_hw *hw, u8 index)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   u32 val;
> > +
> > +
> > +   val = readl_relaxed(sdmmc_mux->reg);
> > +   if (get_div_field(val))
> > +   index = mux_non_lj_idx[index];
> > +   else
> > +   index = mux_lj_idx[index];
> > +
> > +   val &= ~MUX_MASK;
> > +   val |= index << MUX_SHIFT;
> > +
> > +   writel(val, sdmmc_mux->reg);
> > +
> > +   return 0;
>

Re: [PATCH v4 2/4] clk: tegra: refactor 7.1 div calculation

[Peter De Schrijver]

On Wed, Jul 11, 2018 at 09:42:20AM +0100, Jon Hunter wrote:
> 
> On 11/07/18 09:00, Peter De Schrijver wrote:
> > On Tue, Jul 10, 2018 at 05:17:05PM +0100, Jon Hunter wrote:
> >>
> >> On 09/07/18 17:38, Aapo Vienamo wrote:
> >>> From: Peter De Schrijver 
> >>>
> >>> Move this to a separate file so it can be used to calculate the sdmmc
> >>> clock dividers.
> >>
> >> Sorry for not commenting sooner, but what is the motivation for moving
> >> this to its own file? I don't see why we need to do this in order to use
> >> elsewhere. Furthermore, the original file is quite aptly named 
> >> 'clk-divider.c'
> >> and now we have a div71.c which seems quite specific.
> > 
> > How else would you do it?
> 
> Keep it in the same file?
> 

That seems odd. clk-divider.c is meant to implement a clock type, not
utility functions we happen to need in several types.

Peter.

Re: [PATCH v4 2/4] clk: tegra: refactor 7.1 div calculation

[Peter De Schrijver]

On Wed, Jul 11, 2018 at 09:42:20AM +0100, Jon Hunter wrote:
> 
> On 11/07/18 09:00, Peter De Schrijver wrote:
> > On Tue, Jul 10, 2018 at 05:17:05PM +0100, Jon Hunter wrote:
> >>
> >> On 09/07/18 17:38, Aapo Vienamo wrote:
> >>> From: Peter De Schrijver 
> >>>
> >>> Move this to a separate file so it can be used to calculate the sdmmc
> >>> clock dividers.
> >>
> >> Sorry for not commenting sooner, but what is the motivation for moving
> >> this to its own file? I don't see why we need to do this in order to use
> >> elsewhere. Furthermore, the original file is quite aptly named 
> >> 'clk-divider.c'
> >> and now we have a div71.c which seems quite specific.
> > 
> > How else would you do it?
> 
> Keep it in the same file?
> 

That seems odd. clk-divider.c is meant to implement a clock type, not
utility functions we happen to need in several types.

Peter.

Re: [PATCH v4 2/4] clk: tegra: refactor 7.1 div calculation

[Peter De Schrijver]

On Tue, Jul 10, 2018 at 05:17:05PM +0100, Jon Hunter wrote:
> 
> On 09/07/18 17:38, Aapo Vienamo wrote:
> > From: Peter De Schrijver 
> > 
> > Move this to a separate file so it can be used to calculate the sdmmc
> > clock dividers.
> 
> Sorry for not commenting sooner, but what is the motivation for moving
> this to its own file? I don't see why we need to do this in order to use
> elsewhere. Furthermore, the original file is quite aptly named 'clk-divider.c'
> and now we have a div71.c which seems quite specific.

How else would you do it?

Peter.

> > Signed-off-by: Peter De-Schrijver 
> > Signed-off-by: Aapo Vienamo 
> > Acked-by: Peter De Schrijver 
> > ---
> >  drivers/clk/tegra/Makefile  |  1 +
> >  drivers/clk/tegra/clk-divider.c | 30 +---
> >  drivers/clk/tegra/clk.h |  3 +++
> >  drivers/clk/tegra/div71.c   | 43 
> > +
> >  4 files changed, 52 insertions(+), 25 deletions(-)
> >  create mode 100644 drivers/clk/tegra/div71.c
> > 
> > diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> > index b716923..6d4f563 100644
> > --- a/drivers/clk/tegra/Makefile
> > +++ b/drivers/clk/tegra/Makefile
> > @@ -24,3 +24,4 @@ obj-$(CONFIG_ARCH_TEGRA_132_SOC)  += clk-tegra124.o
> >  obj-y  += cvb.o
> >  obj-$(CONFIG_ARCH_TEGRA_210_SOC)   += clk-tegra210.o
> >  obj-$(CONFIG_CLK_TEGRA_BPMP)   += clk-bpmp.o
> > +obj-y  += div71.o
> > diff --git a/drivers/clk/tegra/clk-divider.c 
> > b/drivers/clk/tegra/clk-divider.c
> > index 16e0aee..ad87858 100644
> > --- a/drivers/clk/tegra/clk-divider.c
> > +++ b/drivers/clk/tegra/clk-divider.c
> > @@ -32,35 +32,15 @@
> >  static int get_div(struct tegra_clk_frac_div *divider, unsigned long rate,
> >unsigned long parent_rate)
> >  {
> > -   u64 divider_ux1 = parent_rate;
> > -   u8 flags = divider->flags;
> > -   int mul;
> > -
> > -   if (!rate)
> > -   return 0;
> > -
> > -   mul = get_mul(divider);
> > -
> > -   if (!(flags & TEGRA_DIVIDER_INT))
> > -   divider_ux1 *= mul;
> > -
> > -   if (flags & TEGRA_DIVIDER_ROUND_UP)
> > -   divider_ux1 += rate - 1;
> > -
> > -   do_div(divider_ux1, rate);
> > -
> > -   if (flags & TEGRA_DIVIDER_INT)
> > -   divider_ux1 *= mul;
> > +   int div;
> >  
> > -   divider_ux1 -= mul;
> > +   div = div71_get(rate, parent_rate, divider->width, divider->frac_width,
> > +   divider->flags);
> >  
> > -   if ((s64)divider_ux1 < 0)
> > +   if (div < 0)
> > return 0;
> >  
> > -   if (divider_ux1 > get_max_div(divider))
> > -   return get_max_div(divider);
> > -
> > -   return divider_ux1;
> > +   return div;
> >  }
> >  
> >  static unsigned long clk_frac_div_recalc_rate(struct clk_hw *hw,
> > diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> > index e3b9c22..149cc70 100644
> > --- a/drivers/clk/tegra/clk.h
> > +++ b/drivers/clk/tegra/clk.h
> > @@ -812,6 +812,9 @@ extern tegra_clk_apply_init_table_func 
> > tegra_clk_apply_init_table;
> >  int tegra_pll_wait_for_lock(struct tegra_clk_pll *pll);
> >  u16 tegra_pll_get_fixed_mdiv(struct clk_hw *hw, unsigned long input_rate);
> >  int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
> > +int div71_get(unsigned long rate, unsigned parent_rate, u8 width,
> > + u8 frac_width, u8 flags);
> > +
> >  
> >  /* Combined read fence with delay */
> >  #define fence_udelay(delay, reg)   \
> > diff --git a/drivers/clk/tegra/div71.c b/drivers/clk/tegra/div71.c
> > new file mode 100644
> > index 000..1eecc84
> > --- /dev/null
> > +++ b/drivers/clk/tegra/div71.c
> > @@ -0,0 +1,43 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018, NVIDIA CORPORATION.  All rights reserved.
> > + */
> > +
> > +#include 
> > +
> > +#include "clk.h"
> > +
> > +#define div_mask(w) ((1 << (w)) - 1)
> > +
> > +int div71_get(unsigned long rate, unsigned parent_rate, u8 width,
> > + u8 frac_width, u8 flags)
> > +{
> > +   u64 divider_ux1 = parent_rate;
> > +   int mul;
> > +
> > +   if (!rate)
> > +   return 0;
> > +
> > +   mul = 1 << frac_width;
> > +
&g

Re: [PATCH v4 2/4] clk: tegra: refactor 7.1 div calculation

[Peter De Schrijver]

On Tue, Jul 10, 2018 at 05:17:05PM +0100, Jon Hunter wrote:
> 
> On 09/07/18 17:38, Aapo Vienamo wrote:
> > From: Peter De Schrijver 
> > 
> > Move this to a separate file so it can be used to calculate the sdmmc
> > clock dividers.
> 
> Sorry for not commenting sooner, but what is the motivation for moving
> this to its own file? I don't see why we need to do this in order to use
> elsewhere. Furthermore, the original file is quite aptly named 'clk-divider.c'
> and now we have a div71.c which seems quite specific.

How else would you do it?

Peter.

> > Signed-off-by: Peter De-Schrijver 
> > Signed-off-by: Aapo Vienamo 
> > Acked-by: Peter De Schrijver 
> > ---
> >  drivers/clk/tegra/Makefile  |  1 +
> >  drivers/clk/tegra/clk-divider.c | 30 +---
> >  drivers/clk/tegra/clk.h |  3 +++
> >  drivers/clk/tegra/div71.c   | 43 
> > +
> >  4 files changed, 52 insertions(+), 25 deletions(-)
> >  create mode 100644 drivers/clk/tegra/div71.c
> > 
> > diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> > index b716923..6d4f563 100644
> > --- a/drivers/clk/tegra/Makefile
> > +++ b/drivers/clk/tegra/Makefile
> > @@ -24,3 +24,4 @@ obj-$(CONFIG_ARCH_TEGRA_132_SOC)  += clk-tegra124.o
> >  obj-y  += cvb.o
> >  obj-$(CONFIG_ARCH_TEGRA_210_SOC)   += clk-tegra210.o
> >  obj-$(CONFIG_CLK_TEGRA_BPMP)   += clk-bpmp.o
> > +obj-y  += div71.o
> > diff --git a/drivers/clk/tegra/clk-divider.c 
> > b/drivers/clk/tegra/clk-divider.c
> > index 16e0aee..ad87858 100644
> > --- a/drivers/clk/tegra/clk-divider.c
> > +++ b/drivers/clk/tegra/clk-divider.c
> > @@ -32,35 +32,15 @@
> >  static int get_div(struct tegra_clk_frac_div *divider, unsigned long rate,
> >unsigned long parent_rate)
> >  {
> > -   u64 divider_ux1 = parent_rate;
> > -   u8 flags = divider->flags;
> > -   int mul;
> > -
> > -   if (!rate)
> > -   return 0;
> > -
> > -   mul = get_mul(divider);
> > -
> > -   if (!(flags & TEGRA_DIVIDER_INT))
> > -   divider_ux1 *= mul;
> > -
> > -   if (flags & TEGRA_DIVIDER_ROUND_UP)
> > -   divider_ux1 += rate - 1;
> > -
> > -   do_div(divider_ux1, rate);
> > -
> > -   if (flags & TEGRA_DIVIDER_INT)
> > -   divider_ux1 *= mul;
> > +   int div;
> >  
> > -   divider_ux1 -= mul;
> > +   div = div71_get(rate, parent_rate, divider->width, divider->frac_width,
> > +   divider->flags);
> >  
> > -   if ((s64)divider_ux1 < 0)
> > +   if (div < 0)
> > return 0;
> >  
> > -   if (divider_ux1 > get_max_div(divider))
> > -   return get_max_div(divider);
> > -
> > -   return divider_ux1;
> > +   return div;
> >  }
> >  
> >  static unsigned long clk_frac_div_recalc_rate(struct clk_hw *hw,
> > diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> > index e3b9c22..149cc70 100644
> > --- a/drivers/clk/tegra/clk.h
> > +++ b/drivers/clk/tegra/clk.h
> > @@ -812,6 +812,9 @@ extern tegra_clk_apply_init_table_func 
> > tegra_clk_apply_init_table;
> >  int tegra_pll_wait_for_lock(struct tegra_clk_pll *pll);
> >  u16 tegra_pll_get_fixed_mdiv(struct clk_hw *hw, unsigned long input_rate);
> >  int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
> > +int div71_get(unsigned long rate, unsigned parent_rate, u8 width,
> > + u8 frac_width, u8 flags);
> > +
> >  
> >  /* Combined read fence with delay */
> >  #define fence_udelay(delay, reg)   \
> > diff --git a/drivers/clk/tegra/div71.c b/drivers/clk/tegra/div71.c
> > new file mode 100644
> > index 000..1eecc84
> > --- /dev/null
> > +++ b/drivers/clk/tegra/div71.c
> > @@ -0,0 +1,43 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018, NVIDIA CORPORATION.  All rights reserved.
> > + */
> > +
> > +#include 
> > +
> > +#include "clk.h"
> > +
> > +#define div_mask(w) ((1 << (w)) - 1)
> > +
> > +int div71_get(unsigned long rate, unsigned parent_rate, u8 width,
> > + u8 frac_width, u8 flags)
> > +{
> > +   u64 divider_ux1 = parent_rate;
> > +   int mul;
> > +
> > +   if (!rate)
> > +   return 0;
> > +
> > +   mul = 1 << frac_width;
> > +
&g

Re: [PATCH v4 0/4] Multiplex sdmmc low jitter clock path

[Peter De Schrijver]

Series Acked-By: Peter De Schrijver 

Peter.

On Mon, Jul 09, 2018 at 07:38:54PM +0300, Aapo Vienamo wrote:
> The SDMMC clocks have a Low Jitter (LJ) clock path which bypasses a
> divider to achieve better jitter performance with high speed signaling
> modes. The clock path with the divider is needed by some of the slower
> signaling modes. This series automatically multiplexes the LJ and
> non-LJ clock paths based on the requested frequency.
> 
> Changelog:
> v4:
>   - Add a changelog
> 
> v3:
>   - Use  include instead of  for
> do_div()
>   - Use SPDX tags for new files
>   - Make mux_lj_idx[] and mux_non_lj_idx[] const
>   - Make tegra_clk_sdmmc_mux_ops static
>   - Fix the includes for fence_udelay() in a separate patch
> 
> v2:
>   - Fix the type compatibility error on do_div
> 
> Aapo Vienamo (1):
>   clk: tegra: Fix includes required by fence_udelay()
> 
> Peter De Schrijver (1):
>   clk: tegra: refactor 7.1 div calculation
> 
> Peter De-Schrijver (2):
>   clk: tegra: Add sdmmc mux divider clock
>   clk: tegra: make sdmmc2 and sdmmc4 as sdmmc clocks
> 
>  drivers/clk/tegra/Makefile   |   2 +
>  drivers/clk/tegra/clk-divider.c  |  30 +
>  drivers/clk/tegra/clk-id.h   |   2 -
>  drivers/clk/tegra/clk-sdmmc-mux.c| 250 
> +++
>  drivers/clk/tegra/clk-tegra-periph.c |  11 --
>  drivers/clk/tegra/clk-tegra210.c |  14 +-
>  drivers/clk/tegra/clk.h  |  30 +
>  drivers/clk/tegra/div71.c|  43 ++
>  8 files changed, 342 insertions(+), 40 deletions(-)
>  create mode 100644 drivers/clk/tegra/clk-sdmmc-mux.c
>  create mode 100644 drivers/clk/tegra/div71.c
> 
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/4] Multiplex sdmmc low jitter clock path

[Peter De Schrijver]

Series Acked-By: Peter De Schrijver 

Peter.

On Mon, Jul 09, 2018 at 07:38:54PM +0300, Aapo Vienamo wrote:
> The SDMMC clocks have a Low Jitter (LJ) clock path which bypasses a
> divider to achieve better jitter performance with high speed signaling
> modes. The clock path with the divider is needed by some of the slower
> signaling modes. This series automatically multiplexes the LJ and
> non-LJ clock paths based on the requested frequency.
> 
> Changelog:
> v4:
>   - Add a changelog
> 
> v3:
>   - Use  include instead of  for
> do_div()
>   - Use SPDX tags for new files
>   - Make mux_lj_idx[] and mux_non_lj_idx[] const
>   - Make tegra_clk_sdmmc_mux_ops static
>   - Fix the includes for fence_udelay() in a separate patch
> 
> v2:
>   - Fix the type compatibility error on do_div
> 
> Aapo Vienamo (1):
>   clk: tegra: Fix includes required by fence_udelay()
> 
> Peter De Schrijver (1):
>   clk: tegra: refactor 7.1 div calculation
> 
> Peter De-Schrijver (2):
>   clk: tegra: Add sdmmc mux divider clock
>   clk: tegra: make sdmmc2 and sdmmc4 as sdmmc clocks
> 
>  drivers/clk/tegra/Makefile   |   2 +
>  drivers/clk/tegra/clk-divider.c  |  30 +
>  drivers/clk/tegra/clk-id.h   |   2 -
>  drivers/clk/tegra/clk-sdmmc-mux.c| 250 
> +++
>  drivers/clk/tegra/clk-tegra-periph.c |  11 --
>  drivers/clk/tegra/clk-tegra210.c |  14 +-
>  drivers/clk/tegra/clk.h  |  30 +
>  drivers/clk/tegra/div71.c|  43 ++
>  8 files changed, 342 insertions(+), 40 deletions(-)
>  create mode 100644 drivers/clk/tegra/clk-sdmmc-mux.c
>  create mode 100644 drivers/clk/tegra/div71.c
> 
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] clk: tegra: Add sdmmc mux divider clock

[Peter De Schrijver]

On Fri, Jul 06, 2018 at 11:18:30AM -0700, Stephen Boyd wrote:
> Quoting Aapo Vienamo (2018-07-04 03:17:34)
> > diff --git a/drivers/clk/tegra/clk-sdmmc-mux.c 
> > b/drivers/clk/tegra/clk-sdmmc-mux.c
> > new file mode 100644
> > index 000..8e19cb3
> > --- /dev/null
> > +++ b/drivers/clk/tegra/clk-sdmmc-mux.c
> > @@ -0,0 +1,254 @@
> > +/*
> > + * Copyright (c) 2018 NVIDIA CORPORATION.  All rights reserved.
> > + *
> > + * based on clk-mux.c
> > +
> > + * Copyright (C) 2011 Sascha Hauer, Pengutronix 
> > + * Copyright (C) 2011 Richard Zhao, Linaro 
> > + * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd 
> > 
> > + *
> > + * 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.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see .
> 
> Any chance we can get SPDX tags here instead of all the boiler plate?
> 
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "clk.h"
> > +
> > +#define DIV_MASK GENMASK(7, 0)
> > +#define MUX_SHIFT 29
> > +#define MUX_MASK GENMASK(MUX_SHIFT + 2, MUX_SHIFT)
> > +
> > +#define get_max_div(d) DIV_MASK
> > +#define get_div_field(val) ((val) & DIV_MASK)
> > +#define get_mux_field(val) (((val) & MUX_MASK) >> MUX_SHIFT)
> > +
> > +static const char * const mux_sdmmc_parents[] = { "pll_p", "pll_c4_out2",
> > + "pll_c4_out0", 
> > "pll_c4_out1",
> > +  "clk_m" };
> > +static u8 mux_lj_idx[] = { [0] = 0, [1] = 1, [2] = 2, [3] = 5, [4] = 6 };
> > +static u8 mux_non_lj_idx[] = { [0] = 0, [1] = 3, [2] = 7, [3] = 4, [4] = 6 
> > };
> 
> These can be const?
> 
> > +
> > +static u8 clk_sdmmc_mux_get_parent(struct clk_hw *hw)
> > +{
> [...]
> > +static int clk_sdmmc_mux_set_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   int div;
> > +   unsigned long flags = 0;
> > +   u32 val;
> > +   u8 src;
> > +
> > +   div = div71_get(rate, parent_rate, 8, 1, sdmmc_mux->div_flags);
> > +   if (div < 0)
> > +   return div;
> > +
> > +   if (sdmmc_mux->lock)
> > +   spin_lock_irqsave(sdmmc_mux->lock, flags);
> > +
> > +   src = clk_sdmmc_mux_get_parent(hw);
> > +   if (div)
> > +   src = mux_non_lj_idx[src];
> > +   else
> > +   src = mux_lj_idx[src];
> > +
> > +   val = src << MUX_SHIFT;
> > +   val |= div;
> > +   writel(val, sdmmc_mux->reg);
> > +   fence_udelay(2, sdmmc_mux->reg);
> > +
> > +   if (sdmmc_mux->lock)
> > +   spin_unlock_irqrestore(sdmmc_mux->lock, flags);
> 
> This conditional locking will give sparse a headache. O well.
> 
> > +
> > +   return 0;
> > +}
> > +
> > +static int clk_sdmmc_mux_is_enabled(struct clk_hw *hw)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   const struct clk_ops *gate_ops = sdmmc_mux->gate_ops;
> > +   struct clk_hw *gate_hw = _mux->gate.hw;
> > +
> > +   __clk_hw_set_clk(gate_hw, hw);
> > +
> > +   return gate_ops->is_enabled(gate_hw);
> > +}
> > +
> > +static int clk_sdmmc_mux_enable(struct clk_hw *hw)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   const struct clk_ops *gate_ops = sdmmc_mux->gate_ops;
> > +   struct clk_hw *gate_hw = _mux->gate.hw;
> > +
> > +   __clk_hw_set_clk(gate_hw, hw);
> > +
> > +   return  gate_ops->enable(gate_hw);
> > +}
> > +
> > +static void clk_sdmmc_mux_disable(struct clk_hw *hw)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   const struct clk_ops *gate_ops = sdmmc_mux->gate_ops;
> > +   struct clk_hw *gate_hw = _mux->gate.hw;
> > +
> > +   gate_ops->disable(gate_hw);
> > +}
> > +
> > +const struct clk_ops tegra_clk_sdmmc_mux_ops = {
> 
> static?
> 
> > +   .get_parent = clk_sdmmc_mux_get_parent,
> > +   .set_parent = clk_sdmmc_mux_set_parent,
> > +   .determine_rate = clk_sdmmc_mux_determine_rate,
> > +   .recalc_rate = clk_sdmmc_mux_recalc_rate,
> > +   .set_rate = clk_sdmmc_mux_set_rate,
> > +   .is_enabled = clk_sdmmc_mux_is_enabled,
> > +   .enable = clk_sdmmc_mux_enable,
> > +   .disable = clk_sdmmc_mux_disable,
> > +};
> > +
> > diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> > index f14e136..4c3d0f5 

Re: [PATCH v2 2/3] clk: tegra: Add sdmmc mux divider clock

[Peter De Schrijver]

On Fri, Jul 06, 2018 at 11:18:30AM -0700, Stephen Boyd wrote:
> Quoting Aapo Vienamo (2018-07-04 03:17:34)
> > diff --git a/drivers/clk/tegra/clk-sdmmc-mux.c 
> > b/drivers/clk/tegra/clk-sdmmc-mux.c
> > new file mode 100644
> > index 000..8e19cb3
> > --- /dev/null
> > +++ b/drivers/clk/tegra/clk-sdmmc-mux.c
> > @@ -0,0 +1,254 @@
> > +/*
> > + * Copyright (c) 2018 NVIDIA CORPORATION.  All rights reserved.
> > + *
> > + * based on clk-mux.c
> > +
> > + * Copyright (C) 2011 Sascha Hauer, Pengutronix 
> > + * Copyright (C) 2011 Richard Zhao, Linaro 
> > + * Copyright (C) 2011-2012 Mike Turquette, Linaro Ltd 
> > 
> > + *
> > + * 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.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see .
> 
> Any chance we can get SPDX tags here instead of all the boiler plate?
> 
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "clk.h"
> > +
> > +#define DIV_MASK GENMASK(7, 0)
> > +#define MUX_SHIFT 29
> > +#define MUX_MASK GENMASK(MUX_SHIFT + 2, MUX_SHIFT)
> > +
> > +#define get_max_div(d) DIV_MASK
> > +#define get_div_field(val) ((val) & DIV_MASK)
> > +#define get_mux_field(val) (((val) & MUX_MASK) >> MUX_SHIFT)
> > +
> > +static const char * const mux_sdmmc_parents[] = { "pll_p", "pll_c4_out2",
> > + "pll_c4_out0", 
> > "pll_c4_out1",
> > +  "clk_m" };
> > +static u8 mux_lj_idx[] = { [0] = 0, [1] = 1, [2] = 2, [3] = 5, [4] = 6 };
> > +static u8 mux_non_lj_idx[] = { [0] = 0, [1] = 3, [2] = 7, [3] = 4, [4] = 6 
> > };
> 
> These can be const?
> 
> > +
> > +static u8 clk_sdmmc_mux_get_parent(struct clk_hw *hw)
> > +{
> [...]
> > +static int clk_sdmmc_mux_set_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   int div;
> > +   unsigned long flags = 0;
> > +   u32 val;
> > +   u8 src;
> > +
> > +   div = div71_get(rate, parent_rate, 8, 1, sdmmc_mux->div_flags);
> > +   if (div < 0)
> > +   return div;
> > +
> > +   if (sdmmc_mux->lock)
> > +   spin_lock_irqsave(sdmmc_mux->lock, flags);
> > +
> > +   src = clk_sdmmc_mux_get_parent(hw);
> > +   if (div)
> > +   src = mux_non_lj_idx[src];
> > +   else
> > +   src = mux_lj_idx[src];
> > +
> > +   val = src << MUX_SHIFT;
> > +   val |= div;
> > +   writel(val, sdmmc_mux->reg);
> > +   fence_udelay(2, sdmmc_mux->reg);
> > +
> > +   if (sdmmc_mux->lock)
> > +   spin_unlock_irqrestore(sdmmc_mux->lock, flags);
> 
> This conditional locking will give sparse a headache. O well.
> 
> > +
> > +   return 0;
> > +}
> > +
> > +static int clk_sdmmc_mux_is_enabled(struct clk_hw *hw)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   const struct clk_ops *gate_ops = sdmmc_mux->gate_ops;
> > +   struct clk_hw *gate_hw = _mux->gate.hw;
> > +
> > +   __clk_hw_set_clk(gate_hw, hw);
> > +
> > +   return gate_ops->is_enabled(gate_hw);
> > +}
> > +
> > +static int clk_sdmmc_mux_enable(struct clk_hw *hw)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   const struct clk_ops *gate_ops = sdmmc_mux->gate_ops;
> > +   struct clk_hw *gate_hw = _mux->gate.hw;
> > +
> > +   __clk_hw_set_clk(gate_hw, hw);
> > +
> > +   return  gate_ops->enable(gate_hw);
> > +}
> > +
> > +static void clk_sdmmc_mux_disable(struct clk_hw *hw)
> > +{
> > +   struct tegra_sdmmc_mux *sdmmc_mux = to_clk_sdmmc_mux(hw);
> > +   const struct clk_ops *gate_ops = sdmmc_mux->gate_ops;
> > +   struct clk_hw *gate_hw = _mux->gate.hw;
> > +
> > +   gate_ops->disable(gate_hw);
> > +}
> > +
> > +const struct clk_ops tegra_clk_sdmmc_mux_ops = {
> 
> static?
> 
> > +   .get_parent = clk_sdmmc_mux_get_parent,
> > +   .set_parent = clk_sdmmc_mux_set_parent,
> > +   .determine_rate = clk_sdmmc_mux_determine_rate,
> > +   .recalc_rate = clk_sdmmc_mux_recalc_rate,
> > +   .set_rate = clk_sdmmc_mux_set_rate,
> > +   .is_enabled = clk_sdmmc_mux_is_enabled,
> > +   .enable = clk_sdmmc_mux_enable,
> > +   .disable = clk_sdmmc_mux_disable,
> > +};
> > +
> > diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> > index f14e136..4c3d0f5 

Re: [PATCH v2 1/3] clk: tegra: refactor 7.1 div calculation

[Peter De Schrijver]

On Wed, Jul 04, 2018 at 01:17:33PM +0300, Aapo Vienamo wrote:
> From: Peter De Schrijver 
> 
> Move this to a separate file so it can be used to calculate the sdmmc
> clock dividers.
> 

Series Acked-By: Peter De Schrijver 

> Signed-off-by: Peter De-Schrijver 
> Signed-off-by: Aapo Vienamo 
> ---
>  drivers/clk/tegra/Makefile  |  1 +
>  drivers/clk/tegra/clk-divider.c | 30 ---
>  drivers/clk/tegra/clk.h |  3 +++
>  drivers/clk/tegra/div71.c   | 54 
> +
>  4 files changed, 63 insertions(+), 25 deletions(-)
>  create mode 100644 drivers/clk/tegra/div71.c
> 
> diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> index b716923..6d4f563 100644
> --- a/drivers/clk/tegra/Makefile
> +++ b/drivers/clk/tegra/Makefile
> @@ -24,3 +24,4 @@ obj-$(CONFIG_ARCH_TEGRA_132_SOC)+= clk-tegra124.o
>  obj-y+= cvb.o
>  obj-$(CONFIG_ARCH_TEGRA_210_SOC) += clk-tegra210.o
>  obj-$(CONFIG_CLK_TEGRA_BPMP) += clk-bpmp.o
> +obj-y+= div71.o
> diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
> index 16e0aee..ad87858 100644
> --- a/drivers/clk/tegra/clk-divider.c
> +++ b/drivers/clk/tegra/clk-divider.c
> @@ -32,35 +32,15 @@
>  static int get_div(struct tegra_clk_frac_div *divider, unsigned long rate,
>  unsigned long parent_rate)
>  {
> - u64 divider_ux1 = parent_rate;
> - u8 flags = divider->flags;
> - int mul;
> -
> - if (!rate)
> - return 0;
> -
> - mul = get_mul(divider);
> -
> - if (!(flags & TEGRA_DIVIDER_INT))
> - divider_ux1 *= mul;
> -
> - if (flags & TEGRA_DIVIDER_ROUND_UP)
> - divider_ux1 += rate - 1;
> -
> - do_div(divider_ux1, rate);
> -
> - if (flags & TEGRA_DIVIDER_INT)
> - divider_ux1 *= mul;
> + int div;
>  
> - divider_ux1 -= mul;
> + div = div71_get(rate, parent_rate, divider->width, divider->frac_width,
> + divider->flags);
>  
> - if ((s64)divider_ux1 < 0)
> + if (div < 0)
>   return 0;
>  
> - if (divider_ux1 > get_max_div(divider))
> - return get_max_div(divider);
> -
> - return divider_ux1;
> + return div;
>  }
>  
>  static unsigned long clk_frac_div_recalc_rate(struct clk_hw *hw,
> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> index e1f8846..f14e136 100644
> --- a/drivers/clk/tegra/clk.h
> +++ b/drivers/clk/tegra/clk.h
> @@ -811,6 +811,9 @@ extern tegra_clk_apply_init_table_func 
> tegra_clk_apply_init_table;
>  int tegra_pll_wait_for_lock(struct tegra_clk_pll *pll);
>  u16 tegra_pll_get_fixed_mdiv(struct clk_hw *hw, unsigned long input_rate);
>  int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
> +int div71_get(unsigned long rate, unsigned parent_rate, u8 width,
> +   u8 frac_width, u8 flags);
> +
>  
>  /* Combined read fence with delay */
>  #define fence_udelay(delay, reg) \
> diff --git a/drivers/clk/tegra/div71.c b/drivers/clk/tegra/div71.c
> new file mode 100644
> index 000..1a5e04c
> --- /dev/null
> +++ b/drivers/clk/tegra/div71.c
> @@ -0,0 +1,54 @@
> +/*
> + * Copyright (c) 2018, 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include 
> +
> +#include "clk.h"
> +
> +#define div_mask(w) ((1 << (w)) - 1)
> +
> +int div71_get(unsigned long rate, unsigned parent_rate, u8 width,
> +   u8 frac_width, u8 flags)
> +{
> + u64 divider_ux1 = parent_rate;
> + int mul;
> +
> + if (!rate)
> + return 0;
> +
> + mul = 1 << frac_width;
> +
> + if (!(flags & TEGRA_DIVIDER_INT))
> + divider_ux1 *= mul;
> +
> + if (flags & TEGRA_DIVIDER_ROUND_UP)
> + divider_ux1 += rate - 1;
> +
> + do_div(divider_ux1, rate);
> +
> + if (flags & TEGRA_DIVIDER_INT)
> + divider_ux1 *= mul;
> +
> + if (divider_ux1 < mul)
> + return 0;
> +
> + divider_ux1 -= mul;
> +
> + if (divider_ux1 > div_mask(width))
> + return div_mask(width);
> +
> + return divider_ux1;
> +}
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/3] clk: tegra: refactor 7.1 div calculation

[Peter De Schrijver]

On Wed, Jul 04, 2018 at 01:17:33PM +0300, Aapo Vienamo wrote:
> From: Peter De Schrijver 
> 
> Move this to a separate file so it can be used to calculate the sdmmc
> clock dividers.
> 

Series Acked-By: Peter De Schrijver 

> Signed-off-by: Peter De-Schrijver 
> Signed-off-by: Aapo Vienamo 
> ---
>  drivers/clk/tegra/Makefile  |  1 +
>  drivers/clk/tegra/clk-divider.c | 30 ---
>  drivers/clk/tegra/clk.h |  3 +++
>  drivers/clk/tegra/div71.c   | 54 
> +
>  4 files changed, 63 insertions(+), 25 deletions(-)
>  create mode 100644 drivers/clk/tegra/div71.c
> 
> diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> index b716923..6d4f563 100644
> --- a/drivers/clk/tegra/Makefile
> +++ b/drivers/clk/tegra/Makefile
> @@ -24,3 +24,4 @@ obj-$(CONFIG_ARCH_TEGRA_132_SOC)+= clk-tegra124.o
>  obj-y+= cvb.o
>  obj-$(CONFIG_ARCH_TEGRA_210_SOC) += clk-tegra210.o
>  obj-$(CONFIG_CLK_TEGRA_BPMP) += clk-bpmp.o
> +obj-y+= div71.o
> diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
> index 16e0aee..ad87858 100644
> --- a/drivers/clk/tegra/clk-divider.c
> +++ b/drivers/clk/tegra/clk-divider.c
> @@ -32,35 +32,15 @@
>  static int get_div(struct tegra_clk_frac_div *divider, unsigned long rate,
>  unsigned long parent_rate)
>  {
> - u64 divider_ux1 = parent_rate;
> - u8 flags = divider->flags;
> - int mul;
> -
> - if (!rate)
> - return 0;
> -
> - mul = get_mul(divider);
> -
> - if (!(flags & TEGRA_DIVIDER_INT))
> - divider_ux1 *= mul;
> -
> - if (flags & TEGRA_DIVIDER_ROUND_UP)
> - divider_ux1 += rate - 1;
> -
> - do_div(divider_ux1, rate);
> -
> - if (flags & TEGRA_DIVIDER_INT)
> - divider_ux1 *= mul;
> + int div;
>  
> - divider_ux1 -= mul;
> + div = div71_get(rate, parent_rate, divider->width, divider->frac_width,
> + divider->flags);
>  
> - if ((s64)divider_ux1 < 0)
> + if (div < 0)
>   return 0;
>  
> - if (divider_ux1 > get_max_div(divider))
> - return get_max_div(divider);
> -
> - return divider_ux1;
> + return div;
>  }
>  
>  static unsigned long clk_frac_div_recalc_rate(struct clk_hw *hw,
> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> index e1f8846..f14e136 100644
> --- a/drivers/clk/tegra/clk.h
> +++ b/drivers/clk/tegra/clk.h
> @@ -811,6 +811,9 @@ extern tegra_clk_apply_init_table_func 
> tegra_clk_apply_init_table;
>  int tegra_pll_wait_for_lock(struct tegra_clk_pll *pll);
>  u16 tegra_pll_get_fixed_mdiv(struct clk_hw *hw, unsigned long input_rate);
>  int tegra_pll_p_div_to_hw(struct tegra_clk_pll *pll, u8 p_div);
> +int div71_get(unsigned long rate, unsigned parent_rate, u8 width,
> +   u8 frac_width, u8 flags);
> +
>  
>  /* Combined read fence with delay */
>  #define fence_udelay(delay, reg) \
> diff --git a/drivers/clk/tegra/div71.c b/drivers/clk/tegra/div71.c
> new file mode 100644
> index 000..1a5e04c
> --- /dev/null
> +++ b/drivers/clk/tegra/div71.c
> @@ -0,0 +1,54 @@
> +/*
> + * Copyright (c) 2018, 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include 
> +
> +#include "clk.h"
> +
> +#define div_mask(w) ((1 << (w)) - 1)
> +
> +int div71_get(unsigned long rate, unsigned parent_rate, u8 width,
> +   u8 frac_width, u8 flags)
> +{
> + u64 divider_ux1 = parent_rate;
> + int mul;
> +
> + if (!rate)
> + return 0;
> +
> + mul = 1 << frac_width;
> +
> + if (!(flags & TEGRA_DIVIDER_INT))
> + divider_ux1 *= mul;
> +
> + if (flags & TEGRA_DIVIDER_ROUND_UP)
> + divider_ux1 += rate - 1;
> +
> + do_div(divider_ux1, rate);
> +
> + if (flags & TEGRA_DIVIDER_INT)
> + divider_ux1 *= mul;
> +
> + if (divider_ux1 < mul)
> + return 0;
> +
> + divider_ux1 -= mul;
> +
> + if (divider_ux1 > div_mask(width))
> + return div_mask(width);
> +
> + return divider_ux1;
> +}
> -- 
> 2.7.4
> 

Re: [PATCH v1] clk: tegra: Mark Memory Controller clock as critical

[Peter De Schrijver]

On Mon, Jun 04, 2018 at 01:48:05AM +0300, Dmitry Osipenko wrote:
> Memory Controller should be always-on. Currently the sibling EMC clock is
> marked as critical, let's mark MC clock too for consistency.
> 
> Signed-off-by: Dmitry Osipenko 

Acked-By: Peter De Schrijver 

> ---
>  drivers/clk/tegra/clk-divider.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
> index 16e0aee14773..58874c1bbf5e 100644
> --- a/drivers/clk/tegra/clk-divider.c
> +++ b/drivers/clk/tegra/clk-divider.c
> @@ -194,6 +194,7 @@ static const struct clk_div_table mc_div_table[] = {
>  struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
> void __iomem *reg, spinlock_t *lock)
>  {
> - return clk_register_divider_table(NULL, name, parent_name, 0, reg,
> -   16, 1, 0, mc_div_table, lock);
> + return clk_register_divider_table(NULL, name, parent_name,
> +   CLK_IS_CRITICAL, reg, 16, 1, 0,
> +   mc_div_table, lock);
>  }
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] clk: tegra: Mark Memory Controller clock as critical

[Peter De Schrijver]

On Mon, Jun 04, 2018 at 01:48:05AM +0300, Dmitry Osipenko wrote:
> Memory Controller should be always-on. Currently the sibling EMC clock is
> marked as critical, let's mark MC clock too for consistency.
> 
> Signed-off-by: Dmitry Osipenko 

Acked-By: Peter De Schrijver 

> ---
>  drivers/clk/tegra/clk-divider.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
> index 16e0aee14773..58874c1bbf5e 100644
> --- a/drivers/clk/tegra/clk-divider.c
> +++ b/drivers/clk/tegra/clk-divider.c
> @@ -194,6 +194,7 @@ static const struct clk_div_table mc_div_table[] = {
>  struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
> void __iomem *reg, spinlock_t *lock)
>  {
> - return clk_register_divider_table(NULL, name, parent_name, 0, reg,
> -   16, 1, 0, mc_div_table, lock);
> + return clk_register_divider_table(NULL, name, parent_name,
> +   CLK_IS_CRITICAL, reg, 16, 1, 0,
> +   mc_div_table, lock);
>  }
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/5] Tegra20 External Memory Controller driver

[Peter De Schrijver]

On Mon, Jun 04, 2018 at 01:36:49AM +0300, Dmitry Osipenko wrote:
> Hello,
> 
> Couple years ago the Tegra20 EMC driver was removed from the kernel
> due to incompatible changes in the Tegra's clock driver. This patchset
> introduces a modernized EMC driver. Currently the sole purpose of the
> driver is to initialize DRAM frequency to maximum rate during of the
> kernels boot-up. Later we may consider implementing dynamic memory
> frequency scaling, utilizing functionality provided by this driver.
> 
> Changelog:
> 
> v2:
>   - Minor code cleanups like consistent use of writel_relaxed instead
> of non-relaxed version, reworded error messages, etc.
> 
>   - Factored out use_pllm_ud bit checking into a standalone patch for
> consistency.
> 
> Dmitry Osipenko (5):
>   dt: bindings: tegra20-emc: Document interrupt property
>   ARM: dts: tegra20: Add interrupt to External Memory Controller
>   clk: tegra20: Turn EMC clock gate into divider
>   clk: tegra20: Check whether direct PLLM sourcing is turned off for EMC
>   memory: tegra: Introduce Tegra20 EMC driver
> 

Series Acked-By: Peter De Schrijver 

Re: [PATCH v2 0/5] Tegra20 External Memory Controller driver

[Peter De Schrijver]

On Mon, Jun 04, 2018 at 01:36:49AM +0300, Dmitry Osipenko wrote:
> Hello,
> 
> Couple years ago the Tegra20 EMC driver was removed from the kernel
> due to incompatible changes in the Tegra's clock driver. This patchset
> introduces a modernized EMC driver. Currently the sole purpose of the
> driver is to initialize DRAM frequency to maximum rate during of the
> kernels boot-up. Later we may consider implementing dynamic memory
> frequency scaling, utilizing functionality provided by this driver.
> 
> Changelog:
> 
> v2:
>   - Minor code cleanups like consistent use of writel_relaxed instead
> of non-relaxed version, reworded error messages, etc.
> 
>   - Factored out use_pllm_ud bit checking into a standalone patch for
> consistency.
> 
> Dmitry Osipenko (5):
>   dt: bindings: tegra20-emc: Document interrupt property
>   ARM: dts: tegra20: Add interrupt to External Memory Controller
>   clk: tegra20: Turn EMC clock gate into divider
>   clk: tegra20: Check whether direct PLLM sourcing is turned off for EMC
>   memory: tegra: Introduce Tegra20 EMC driver
> 

Series Acked-By: Peter De Schrijver 

Re: [PATCH v2 4/6] clk: tegra20: init NDFLASH clock to sensible rate

[Peter De Schrijver]

On Tue, May 29, 2018 at 03:19:47PM +0300, Dmitry Osipenko wrote:
> On 29.05.2018 15:12, Stefan Agner wrote:
> > On 29.05.2018 09:48, Peter De Schrijver wrote:
> >> On Mon, May 28, 2018 at 05:53:08PM +0200, Stefan Agner wrote:
> >>> On 28.05.2018 09:55, Peter De Schrijver wrote:
> >>>> On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
> >>>>> From: Lucas Stach 
> >>>>>
> >>>>> Set up the NAND Flash controller clock to run at 150MHz
> >>>>> instead of the rate set by the bootloader. This is a
> >>>>> conservative rate which also yields good performance.
> >>>>>
> >>>>> Signed-off-by: Lucas Stach 
> >>>>> Signed-off-by: Stefan Agner 
> >>>>> ---
> >>>>>  drivers/clk/tegra/clk-tegra20.c | 1 +
> >>>>>  1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/drivers/clk/tegra/clk-tegra20.c 
> >>>>> b/drivers/clk/tegra/clk-tegra20.c
> >>>>> index 0ee56dd04cec..dff8c425cd28 100644
> >>>>> --- a/drivers/clk/tegra/clk-tegra20.c
> >>>>> +++ b/drivers/clk/tegra/clk-tegra20.c
> >>>>> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] 
> >>>>> __initdata = {
> >>>>> { TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 3, 0 },
> >>>>> { TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 3, 0 },
> >>>>> { TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 3, 0 },
> >>>>> +   { TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 15000, 0 },
> >>>>> /* must be the last entry */
> >>>>> { TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
> >>>>>  };
> >>>>> --
> >>>>> 2.17.0
> >>>>>
> >>>>
> >>>> Maybe better to specify this in the Tegra20 dtsi? See
> >>>> "Assigned clock parents and rates" in
> >>>> Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>
> >>> assigned-clocks indeed works just fine for this case. Thanks for
> >>> bringing this up, will drop this patch and add the device tree
> >>> properties in v3.
> >>>
> >>> Hm, interesting that none of the Tegra device tree make use of the
> >>> feature so far. I guess there would be other cases where this would be
> >>> useful as well (the one just above, VDE?).
> >>>
> >>
> >> Yes, historically this feature wasn't available, so we used these init 
> >> tables.
> >> Unfortunately it's not easy to get rid of them for parent and rate
> >> configuration, because new kernels should also work with existing DTBs, so 
> >> we
> >> can't just add assigned-clock properties and remove the existing table
> >> entries. What we could do is use the CLK_IS_CRITICAL flag for all clocks 
> >> which
> >> are only enabled by the init table. For not yet merged blocks, this is
> >> ofcourse not a concern.
> > 
> > Sure I understand.
> > 
> > Was just somewhat surprised that it isn't used at all yet (grep -r -e
> > assigned-clock arch/arm/boot/dts/tegra* returns nothing). After all,
> > assigned clocks bindings have been merged in 2014 :-)
> > 
> > At least "clk: tegra: Specify VDE clock rate" merged earlier this year
> > would have been a candidate already.
> 
> I wasn't even aware of existence of the assigned-clock properties, probably 
> just
> like others.

This feature seems to be little used indeed. Not sure why.

Peter.

Re: [PATCH v2 4/6] clk: tegra20: init NDFLASH clock to sensible rate

[Peter De Schrijver]

On Tue, May 29, 2018 at 03:19:47PM +0300, Dmitry Osipenko wrote:
> On 29.05.2018 15:12, Stefan Agner wrote:
> > On 29.05.2018 09:48, Peter De Schrijver wrote:
> >> On Mon, May 28, 2018 at 05:53:08PM +0200, Stefan Agner wrote:
> >>> On 28.05.2018 09:55, Peter De Schrijver wrote:
> >>>> On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
> >>>>> From: Lucas Stach 
> >>>>>
> >>>>> Set up the NAND Flash controller clock to run at 150MHz
> >>>>> instead of the rate set by the bootloader. This is a
> >>>>> conservative rate which also yields good performance.
> >>>>>
> >>>>> Signed-off-by: Lucas Stach 
> >>>>> Signed-off-by: Stefan Agner 
> >>>>> ---
> >>>>>  drivers/clk/tegra/clk-tegra20.c | 1 +
> >>>>>  1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/drivers/clk/tegra/clk-tegra20.c 
> >>>>> b/drivers/clk/tegra/clk-tegra20.c
> >>>>> index 0ee56dd04cec..dff8c425cd28 100644
> >>>>> --- a/drivers/clk/tegra/clk-tegra20.c
> >>>>> +++ b/drivers/clk/tegra/clk-tegra20.c
> >>>>> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] 
> >>>>> __initdata = {
> >>>>> { TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 3, 0 },
> >>>>> { TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 3, 0 },
> >>>>> { TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 3, 0 },
> >>>>> +   { TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 15000, 0 },
> >>>>> /* must be the last entry */
> >>>>> { TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
> >>>>>  };
> >>>>> --
> >>>>> 2.17.0
> >>>>>
> >>>>
> >>>> Maybe better to specify this in the Tegra20 dtsi? See
> >>>> "Assigned clock parents and rates" in
> >>>> Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>
> >>> assigned-clocks indeed works just fine for this case. Thanks for
> >>> bringing this up, will drop this patch and add the device tree
> >>> properties in v3.
> >>>
> >>> Hm, interesting that none of the Tegra device tree make use of the
> >>> feature so far. I guess there would be other cases where this would be
> >>> useful as well (the one just above, VDE?).
> >>>
> >>
> >> Yes, historically this feature wasn't available, so we used these init 
> >> tables.
> >> Unfortunately it's not easy to get rid of them for parent and rate
> >> configuration, because new kernels should also work with existing DTBs, so 
> >> we
> >> can't just add assigned-clock properties and remove the existing table
> >> entries. What we could do is use the CLK_IS_CRITICAL flag for all clocks 
> >> which
> >> are only enabled by the init table. For not yet merged blocks, this is
> >> ofcourse not a concern.
> > 
> > Sure I understand.
> > 
> > Was just somewhat surprised that it isn't used at all yet (grep -r -e
> > assigned-clock arch/arm/boot/dts/tegra* returns nothing). After all,
> > assigned clocks bindings have been merged in 2014 :-)
> > 
> > At least "clk: tegra: Specify VDE clock rate" merged earlier this year
> > would have been a candidate already.
> 
> I wasn't even aware of existence of the assigned-clock properties, probably 
> just
> like others.

This feature seems to be little used indeed. Not sure why.

Peter.

Re: [PATCH v2 4/6] clk: tegra20: init NDFLASH clock to sensible rate

[Peter De Schrijver]

On Mon, May 28, 2018 at 05:53:08PM +0200, Stefan Agner wrote:
> On 28.05.2018 09:55, Peter De Schrijver wrote:
> > On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
> >> From: Lucas Stach 
> >>
> >> Set up the NAND Flash controller clock to run at 150MHz
> >> instead of the rate set by the bootloader. This is a
> >> conservative rate which also yields good performance.
> >>
> >> Signed-off-by: Lucas Stach 
> >> Signed-off-by: Stefan Agner 
> >> ---
> >>  drivers/clk/tegra/clk-tegra20.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/clk/tegra/clk-tegra20.c 
> >> b/drivers/clk/tegra/clk-tegra20.c
> >> index 0ee56dd04cec..dff8c425cd28 100644
> >> --- a/drivers/clk/tegra/clk-tegra20.c
> >> +++ b/drivers/clk/tegra/clk-tegra20.c
> >> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] 
> >> __initdata = {
> >>{ TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 3, 0 },
> >>{ TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 3, 0 },
> >>{ TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 3, 0 },
> >> +  { TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 15000, 0 },
> >>/* must be the last entry */
> >>{ TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
> >>  };
> >> --
> >> 2.17.0
> >>
> > 
> > Maybe better to specify this in the Tegra20 dtsi? See 
> > "Assigned clock parents and rates" in
> > Documentation/devicetree/bindings/clock/clock-bindings.txt
> 
> assigned-clocks indeed works just fine for this case. Thanks for
> bringing this up, will drop this patch and add the device tree
> properties in v3.
> 
> Hm, interesting that none of the Tegra device tree make use of the
> feature so far. I guess there would be other cases where this would be
> useful as well (the one just above, VDE?).
> 

Yes, historically this feature wasn't available, so we used these init tables.
Unfortunately it's not easy to get rid of them for parent and rate
configuration, because new kernels should also work with existing DTBs, so we
can't just add assigned-clock properties and remove the existing table
entries. What we could do is use the CLK_IS_CRITICAL flag for all clocks which
are only enabled by the init table. For not yet merged blocks, this is
ofcourse not a concern.

Cheers,

Peter.

Re: [PATCH v2 4/6] clk: tegra20: init NDFLASH clock to sensible rate

[Peter De Schrijver]

On Mon, May 28, 2018 at 05:53:08PM +0200, Stefan Agner wrote:
> On 28.05.2018 09:55, Peter De Schrijver wrote:
> > On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
> >> From: Lucas Stach 
> >>
> >> Set up the NAND Flash controller clock to run at 150MHz
> >> instead of the rate set by the bootloader. This is a
> >> conservative rate which also yields good performance.
> >>
> >> Signed-off-by: Lucas Stach 
> >> Signed-off-by: Stefan Agner 
> >> ---
> >>  drivers/clk/tegra/clk-tegra20.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/clk/tegra/clk-tegra20.c 
> >> b/drivers/clk/tegra/clk-tegra20.c
> >> index 0ee56dd04cec..dff8c425cd28 100644
> >> --- a/drivers/clk/tegra/clk-tegra20.c
> >> +++ b/drivers/clk/tegra/clk-tegra20.c
> >> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] 
> >> __initdata = {
> >>{ TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 3, 0 },
> >>{ TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 3, 0 },
> >>{ TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 3, 0 },
> >> +  { TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 15000, 0 },
> >>/* must be the last entry */
> >>{ TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
> >>  };
> >> --
> >> 2.17.0
> >>
> > 
> > Maybe better to specify this in the Tegra20 dtsi? See 
> > "Assigned clock parents and rates" in
> > Documentation/devicetree/bindings/clock/clock-bindings.txt
> 
> assigned-clocks indeed works just fine for this case. Thanks for
> bringing this up, will drop this patch and add the device tree
> properties in v3.
> 
> Hm, interesting that none of the Tegra device tree make use of the
> feature so far. I guess there would be other cases where this would be
> useful as well (the one just above, VDE?).
> 

Yes, historically this feature wasn't available, so we used these init tables.
Unfortunately it's not easy to get rid of them for parent and rate
configuration, because new kernels should also work with existing DTBs, so we
can't just add assigned-clock properties and remove the existing table
entries. What we could do is use the CLK_IS_CRITICAL flag for all clocks which
are only enabled by the init table. For not yet merged blocks, this is
ofcourse not a concern.

Cheers,

Peter.

Re: [PATCH v2 4/6] clk: tegra20: init NDFLASH clock to sensible rate

[Peter De Schrijver]

On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
> From: Lucas Stach 
> 
> Set up the NAND Flash controller clock to run at 150MHz
> instead of the rate set by the bootloader. This is a
> conservative rate which also yields good performance.
> 
> Signed-off-by: Lucas Stach 
> Signed-off-by: Stefan Agner 
> ---
>  drivers/clk/tegra/clk-tegra20.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 0ee56dd04cec..dff8c425cd28 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] 
> __initdata = {
>   { TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 3, 0 },
>   { TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 3, 0 },
>   { TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 3, 0 },
> + { TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 15000, 0 },
>   /* must be the last entry */
>   { TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
>  };
> -- 
> 2.17.0
> 

Maybe better to specify this in the Tegra20 dtsi? See 
"Assigned clock parents and rates" in 
Documentation/devicetree/bindings/clock/clock-bindings.txt

Peter.

Re: [PATCH v2 4/6] clk: tegra20: init NDFLASH clock to sensible rate

[Peter De Schrijver]

On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
> From: Lucas Stach 
> 
> Set up the NAND Flash controller clock to run at 150MHz
> instead of the rate set by the bootloader. This is a
> conservative rate which also yields good performance.
> 
> Signed-off-by: Lucas Stach 
> Signed-off-by: Stefan Agner 
> ---
>  drivers/clk/tegra/clk-tegra20.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 0ee56dd04cec..dff8c425cd28 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] 
> __initdata = {
>   { TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 3, 0 },
>   { TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 3, 0 },
>   { TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 3, 0 },
> + { TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 15000, 0 },
>   /* must be the last entry */
>   { TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
>  };
> -- 
> 2.17.0
> 

Maybe better to specify this in the Tegra20 dtsi? See 
"Assigned clock parents and rates" in 
Documentation/devicetree/bindings/clock/clock-bindings.txt

Peter.

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Peter De Schrijver]

On Thu, May 24, 2018 at 03:49:22PM +0300, Dmitry Osipenko wrote:
> On 24.05.2018 13:04, Peter De Schrijver wrote:
> > On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
> >> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> >> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> >> running on that PLL. Let's use PLL_C as intermediate clock source, making
> >> CPU snappier a tad during of the frequency transition.
> >>
> > 
> > pll_c isn't necessarily 600Mhz when used as a source for the second display
> > head.
> 
> Hmm, indeed.
> 
> Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't
> it? That's likely to be good enough.
> 

Yes. I think it can be always higher than pll_p, but that assumes the display
driver will always program the highest possible rate for pll_c for a given mode
and then program the display divider to divide it down to the required rate.

> Do you know if any of the available CCLK parents has a glitch-less rate
> switching? I.e. CPU won't hang on the rate switch.
> 

Tegra20 doesn't have dynamic ramp PLLs no. So you always have to switch to a
backup clock source before changing the rate of pll_x.

> There is other possible 600MHz source, the PLL_M. Can we use it? This one also
> may become dynamic if we'll consider implementing the memory scaling, but the
> memory frequency probably will fit the transition role pretty well.

I think this should work, but as you mention it may very well be lower than
pll_p if Tegra20 EMC scaling is re-introduced. I think that's why historically
this was never done. 

Peter.

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Peter De Schrijver]

On Thu, May 24, 2018 at 03:49:22PM +0300, Dmitry Osipenko wrote:
> On 24.05.2018 13:04, Peter De Schrijver wrote:
> > On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
> >> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> >> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> >> running on that PLL. Let's use PLL_C as intermediate clock source, making
> >> CPU snappier a tad during of the frequency transition.
> >>
> > 
> > pll_c isn't necessarily 600Mhz when used as a source for the second display
> > head.
> 
> Hmm, indeed.
> 
> Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't
> it? That's likely to be good enough.
> 

Yes. I think it can be always higher than pll_p, but that assumes the display
driver will always program the highest possible rate for pll_c for a given mode
and then program the display divider to divide it down to the required rate.

> Do you know if any of the available CCLK parents has a glitch-less rate
> switching? I.e. CPU won't hang on the rate switch.
> 

Tegra20 doesn't have dynamic ramp PLLs no. So you always have to switch to a
backup clock source before changing the rate of pll_x.

> There is other possible 600MHz source, the PLL_M. Can we use it? This one also
> may become dynamic if we'll consider implementing the memory scaling, but the
> memory frequency probably will fit the transition role pretty well.

I think this should work, but as you mention it may very well be lower than
pll_p if Tegra20 EMC scaling is re-introduced. I think that's why historically
this was never done. 

Peter.

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Peter De Schrijver]

On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> running on that PLL. Let's use PLL_C as intermediate clock source, making
> CPU snappier a tad during of the frequency transition.
> 

pll_c isn't necessarily 600Mhz when used as a source for the second display
head.

Peter.

> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c 
> b/drivers/cpufreq/tegra20-cpufreq.c
> index 3ad6bded6efc..4bf5ba7da40b 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -25,12 +25,13 @@
>  #include 
>  
>  #define PLL_P_FREQ   216000
> +#define PLL_C_FREQ   60
>  
>  static struct cpufreq_frequency_table freq_table[] = {
>   { .frequency = 216000 },
>   { .frequency = 312000 },
>   { .frequency = 456000 },
> - { .frequency = 608000 },
> + { .frequency = 60 },
>   { .frequency = 76 },
>   { .frequency = 816000 },
>   { .frequency = 912000 },
> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>   struct clk *cpu_clk;
>   struct clk *pll_x_clk;
>   struct clk *pll_p_clk;
> + struct clk *pll_c_clk;
>   bool pll_x_prepared;
>  };
>  
> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct 
> cpufreq_policy *policy,
>   if (index == 0 || policy->cur == PLL_P_FREQ)
>   return 0;
>  
> - return PLL_P_FREQ;
> + if (index == 3 || policy->cur == PLL_C_FREQ)
> + return 0;
> +
> + return PLL_C_FREQ;
>  }
>  
>  static int tegra_target_intermediate(struct cpufreq_policy *policy,
> @@ -79,7 +84,7 @@ static int tegra_target_intermediate(struct cpufreq_policy 
> *policy,
>*/
>   clk_prepare_enable(cpufreq->pll_x_clk);
>  
> - ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
> + ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
>   if (ret)
>   clk_disable_unprepare(cpufreq->pll_x_clk);
>   else
> @@ -101,6 +106,9 @@ static int tegra_target(struct cpufreq_policy *policy, 
> unsigned int index)
>   if (index == 0)
>   return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
>  
> + if (index == 3)
> + return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
> +
>   ret = clk_set_rate(cpufreq->pll_x_clk, rate * 1000);
>   /* Restore to earlier frequency on error, i.e. pll_x */
>   if (ret)
> @@ -174,6 +182,12 @@ static int tegra20_cpufreq_probe(struct platform_device 
> *pdev)
>   goto put_pll_x;
>   }
>  
> + cpufreq->pll_c_clk = clk_get_sys(NULL, "pll_c");
> + if (IS_ERR(cpufreq->pll_c_clk)) {
> + err = PTR_ERR(cpufreq->pll_c_clk);
> + goto put_pll_p;
> + }
> +
>   cpufreq->dev = >dev;
>   cpufreq->driver.get = cpufreq_generic_get;
>   cpufreq->driver.attr = cpufreq_generic_attr;
> @@ -190,12 +204,14 @@ static int tegra20_cpufreq_probe(struct platform_device 
> *pdev)
>  
>   err = cpufreq_register_driver(>driver);
>   if (err)
> - goto put_pll_p;
> + goto put_pll_c;
>  
>   platform_set_drvdata(pdev, cpufreq);
>  
>   return 0;
>  
> +put_pll_c:
> + clk_put(cpufreq->pll_c_clk);
>  put_pll_p:
>   clk_put(cpufreq->pll_p_clk);
>  put_pll_x:
> @@ -212,6 +228,7 @@ static int tegra20_cpufreq_remove(struct platform_device 
> *pdev)
>  
>   cpufreq_unregister_driver(>driver);
>  
> + clk_put(cpufreq->pll_c_clk);
>   clk_put(cpufreq->pll_p_clk);
>   clk_put(cpufreq->pll_x_clk);
>   clk_put(cpufreq->cpu_clk);
> -- 
> 2.17.0
> 

Re: [PATCH v1 2/2] cpufreq: tegra20: Use PLL_C as intermediate clock source

[Peter De Schrijver]

On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote:
> PLL_C is running at 600MHz which is significantly higher than the 216MHz
> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is
> running on that PLL. Let's use PLL_C as intermediate clock source, making
> CPU snappier a tad during of the frequency transition.
> 

pll_c isn't necessarily 600Mhz when used as a source for the second display
head.

Peter.

> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/cpufreq/tegra20-cpufreq.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/tegra20-cpufreq.c 
> b/drivers/cpufreq/tegra20-cpufreq.c
> index 3ad6bded6efc..4bf5ba7da40b 100644
> --- a/drivers/cpufreq/tegra20-cpufreq.c
> +++ b/drivers/cpufreq/tegra20-cpufreq.c
> @@ -25,12 +25,13 @@
>  #include 
>  
>  #define PLL_P_FREQ   216000
> +#define PLL_C_FREQ   60
>  
>  static struct cpufreq_frequency_table freq_table[] = {
>   { .frequency = 216000 },
>   { .frequency = 312000 },
>   { .frequency = 456000 },
> - { .frequency = 608000 },
> + { .frequency = 60 },
>   { .frequency = 76 },
>   { .frequency = 816000 },
>   { .frequency = 912000 },
> @@ -44,6 +45,7 @@ struct tegra20_cpufreq {
>   struct clk *cpu_clk;
>   struct clk *pll_x_clk;
>   struct clk *pll_p_clk;
> + struct clk *pll_c_clk;
>   bool pll_x_prepared;
>  };
>  
> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct 
> cpufreq_policy *policy,
>   if (index == 0 || policy->cur == PLL_P_FREQ)
>   return 0;
>  
> - return PLL_P_FREQ;
> + if (index == 3 || policy->cur == PLL_C_FREQ)
> + return 0;
> +
> + return PLL_C_FREQ;
>  }
>  
>  static int tegra_target_intermediate(struct cpufreq_policy *policy,
> @@ -79,7 +84,7 @@ static int tegra_target_intermediate(struct cpufreq_policy 
> *policy,
>*/
>   clk_prepare_enable(cpufreq->pll_x_clk);
>  
> - ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
> + ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
>   if (ret)
>   clk_disable_unprepare(cpufreq->pll_x_clk);
>   else
> @@ -101,6 +106,9 @@ static int tegra_target(struct cpufreq_policy *policy, 
> unsigned int index)
>   if (index == 0)
>   return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk);
>  
> + if (index == 3)
> + return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk);
> +
>   ret = clk_set_rate(cpufreq->pll_x_clk, rate * 1000);
>   /* Restore to earlier frequency on error, i.e. pll_x */
>   if (ret)
> @@ -174,6 +182,12 @@ static int tegra20_cpufreq_probe(struct platform_device 
> *pdev)
>   goto put_pll_x;
>   }
>  
> + cpufreq->pll_c_clk = clk_get_sys(NULL, "pll_c");
> + if (IS_ERR(cpufreq->pll_c_clk)) {
> + err = PTR_ERR(cpufreq->pll_c_clk);
> + goto put_pll_p;
> + }
> +
>   cpufreq->dev = >dev;
>   cpufreq->driver.get = cpufreq_generic_get;
>   cpufreq->driver.attr = cpufreq_generic_attr;
> @@ -190,12 +204,14 @@ static int tegra20_cpufreq_probe(struct platform_device 
> *pdev)
>  
>   err = cpufreq_register_driver(>driver);
>   if (err)
> - goto put_pll_p;
> + goto put_pll_c;
>  
>   platform_set_drvdata(pdev, cpufreq);
>  
>   return 0;
>  
> +put_pll_c:
> + clk_put(cpufreq->pll_c_clk);
>  put_pll_p:
>   clk_put(cpufreq->pll_p_clk);
>  put_pll_x:
> @@ -212,6 +228,7 @@ static int tegra20_cpufreq_remove(struct platform_device 
> *pdev)
>  
>   cpufreq_unregister_driver(>driver);
>  
> + clk_put(cpufreq->pll_c_clk);
>   clk_put(cpufreq->pll_p_clk);
>   clk_put(cpufreq->pll_x_clk);
>   clk_put(cpufreq->cpu_clk);
> -- 
> 2.17.0
> 

Re: [PATCH v3 3/4] clk: tegra: Add quirk for getting CDEV1/2 clocks on Tegra20

[Peter De Schrijver]

On Tue, May 08, 2018 at 07:26:06PM +0300, Dmitry Osipenko wrote:
> CDEV1 and CDEV2 clocks are a bit special case, their parent clock is
> created by the pinctrl driver. It should be possible for clk user to
> request these clocks before pinctrl driver got probed and hence user will
> get an orphaned clock. That might be undesirable because user may expect
> parent clock to be enabled by the child, so let's return -EPROBE_DEFER
> till parent clock appears.
> 
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>

Acked-by: Peter De Schrijver <pdeschrij...@nvidia.com>  
   


> ---
>  drivers/clk/tegra/clk-tegra114.c |  2 +-
>  drivers/clk/tegra/clk-tegra124.c |  2 +-
>  drivers/clk/tegra/clk-tegra20.c  | 32 +++-
>  drivers/clk/tegra/clk-tegra210.c |  2 +-
>  drivers/clk/tegra/clk-tegra30.c  |  2 +-
>  drivers/clk/tegra/clk.c  |  5 +++--
>  drivers/clk/tegra/clk.h  |  2 +-
>  7 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra114.c 
> b/drivers/clk/tegra/clk-tegra114.c
> index 5d5a22d529f5..1824f014202b 100644
> --- a/drivers/clk/tegra/clk-tegra114.c
> +++ b/drivers/clk/tegra/clk-tegra114.c
> @@ -1367,7 +1367,7 @@ static void __init tegra114_clock_init(struct 
> device_node *np)
>   tegra_super_clk_gen4_init(clk_base, pmc_base, tegra114_clks,
>   _x_params);
>  
> - tegra_add_of_provider(np);
> + tegra_add_of_provider(np, of_clk_src_onecell_get);
>   tegra_register_devclks(devclks, ARRAY_SIZE(devclks));
>  
>   tegra_clk_apply_init_table = tegra114_clock_apply_init_table;
> diff --git a/drivers/clk/tegra/clk-tegra124.c 
> b/drivers/clk/tegra/clk-tegra124.c
> index 50088e976611..0c69c7970950 100644
> --- a/drivers/clk/tegra/clk-tegra124.c
> +++ b/drivers/clk/tegra/clk-tegra124.c
> @@ -1479,7 +1479,7 @@ static void __init tegra124_132_clock_init_post(struct 
> device_node *np)
> _x_params);
>   tegra_init_special_resets(1, tegra124_reset_assert,
> tegra124_reset_deassert);
> - tegra_add_of_provider(np);
> + tegra_add_of_provider(np, of_clk_src_onecell_get);
>  
>   clks[TEGRA124_CLK_EMC] = tegra_clk_register_emc(clk_base, np,
>   _lock);
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 636500a98561..cc857d4d4a86 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -1089,6 +1089,36 @@ static const struct of_device_id pmc_match[] 
> __initconst = {
>   { },
>  };
>  
> +static struct clk *tegra20_clk_src_onecell_get(struct of_phandle_args 
> *clkspec,
> +void *data)
> +{
> + struct clk_hw *parent_hw;
> + struct clk_hw *hw;
> + struct clk *clk;
> +
> + clk = of_clk_src_onecell_get(clkspec, data);
> + if (IS_ERR(clk))
> + return clk;
> +
> + /*
> +  * Tegra20 CDEV1 and CDEV2 clocks are a bit special case, their parent
> +  * clock is created by the pinctrl driver. It is possible for clk user
> +  * to request these clocks before pinctrl driver got probed and hence
> +  * user will get an orphaned clock. That might be undesirable because
> +  * user may expect parent clock to be enabled by the child.
> +  */
> + if (clkspec->args[0] == TEGRA20_CLK_CDEV1 ||
> + clkspec->args[0] == TEGRA20_CLK_CDEV2) {
> + hw = __clk_get_hw(clk);
> +
> + parent_hw = clk_hw_get_parent(hw);
> + if (!parent_hw)
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + return clk;
> +}
> +
>  static void __init tegra20_clock_init(struct device_node *np)
>  {
>   struct device_node *node;
> @@ -1127,7 +1157,7 @@ static void __init tegra20_clock_init(struct 
> device_node *np)
>  
>   tegra_init_dup_clks(tegra_clk_duplicates, clks, TEGRA20_CLK_CLK_MAX);
>  
> - tegra_add_of_provider(np);
> + tegra_add_of_provider(np, tegra20_clk_src_onecell_get);
>   tegra_register_devclks(devclks, ARRAY_SIZE(devclks));
>  
>   tegra_clk_apply_init_table = tegra20_clock_apply_init_table;
> diff --git a/drivers/clk/tegra/clk-tegra210.c 
> b/drivers/clk/tegra/clk-tegra210.c
> index 9fb5d51ccce4..5435d01c636a 100644
> --- a/drivers/clk/tegra/clk-tegra210.c
> +++ b/drivers/clk/tegra/clk-tegra210.c
> @@ -3567,7 +3567,7 @@ static void __init tegra210_clock_init(struct 
> device_node *np)

Re: [PATCH v3 3/4] clk: tegra: Add quirk for getting CDEV1/2 clocks on Tegra20

[Peter De Schrijver]

On Tue, May 08, 2018 at 07:26:06PM +0300, Dmitry Osipenko wrote:
> CDEV1 and CDEV2 clocks are a bit special case, their parent clock is
> created by the pinctrl driver. It should be possible for clk user to
> request these clocks before pinctrl driver got probed and hence user will
> get an orphaned clock. That might be undesirable because user may expect
> parent clock to be enabled by the child, so let's return -EPROBE_DEFER
> till parent clock appears.
> 
> Signed-off-by: Dmitry Osipenko 

Acked-by: Peter De Schrijver   
   


> ---
>  drivers/clk/tegra/clk-tegra114.c |  2 +-
>  drivers/clk/tegra/clk-tegra124.c |  2 +-
>  drivers/clk/tegra/clk-tegra20.c  | 32 +++-
>  drivers/clk/tegra/clk-tegra210.c |  2 +-
>  drivers/clk/tegra/clk-tegra30.c  |  2 +-
>  drivers/clk/tegra/clk.c  |  5 +++--
>  drivers/clk/tegra/clk.h  |  2 +-
>  7 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra114.c 
> b/drivers/clk/tegra/clk-tegra114.c
> index 5d5a22d529f5..1824f014202b 100644
> --- a/drivers/clk/tegra/clk-tegra114.c
> +++ b/drivers/clk/tegra/clk-tegra114.c
> @@ -1367,7 +1367,7 @@ static void __init tegra114_clock_init(struct 
> device_node *np)
>   tegra_super_clk_gen4_init(clk_base, pmc_base, tegra114_clks,
>   _x_params);
>  
> - tegra_add_of_provider(np);
> + tegra_add_of_provider(np, of_clk_src_onecell_get);
>   tegra_register_devclks(devclks, ARRAY_SIZE(devclks));
>  
>   tegra_clk_apply_init_table = tegra114_clock_apply_init_table;
> diff --git a/drivers/clk/tegra/clk-tegra124.c 
> b/drivers/clk/tegra/clk-tegra124.c
> index 50088e976611..0c69c7970950 100644
> --- a/drivers/clk/tegra/clk-tegra124.c
> +++ b/drivers/clk/tegra/clk-tegra124.c
> @@ -1479,7 +1479,7 @@ static void __init tegra124_132_clock_init_post(struct 
> device_node *np)
> _x_params);
>   tegra_init_special_resets(1, tegra124_reset_assert,
> tegra124_reset_deassert);
> - tegra_add_of_provider(np);
> + tegra_add_of_provider(np, of_clk_src_onecell_get);
>  
>   clks[TEGRA124_CLK_EMC] = tegra_clk_register_emc(clk_base, np,
>   _lock);
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 636500a98561..cc857d4d4a86 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -1089,6 +1089,36 @@ static const struct of_device_id pmc_match[] 
> __initconst = {
>   { },
>  };
>  
> +static struct clk *tegra20_clk_src_onecell_get(struct of_phandle_args 
> *clkspec,
> +void *data)
> +{
> + struct clk_hw *parent_hw;
> + struct clk_hw *hw;
> + struct clk *clk;
> +
> + clk = of_clk_src_onecell_get(clkspec, data);
> + if (IS_ERR(clk))
> + return clk;
> +
> + /*
> +  * Tegra20 CDEV1 and CDEV2 clocks are a bit special case, their parent
> +  * clock is created by the pinctrl driver. It is possible for clk user
> +  * to request these clocks before pinctrl driver got probed and hence
> +  * user will get an orphaned clock. That might be undesirable because
> +  * user may expect parent clock to be enabled by the child.
> +  */
> + if (clkspec->args[0] == TEGRA20_CLK_CDEV1 ||
> + clkspec->args[0] == TEGRA20_CLK_CDEV2) {
> + hw = __clk_get_hw(clk);
> +
> + parent_hw = clk_hw_get_parent(hw);
> + if (!parent_hw)
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + return clk;
> +}
> +
>  static void __init tegra20_clock_init(struct device_node *np)
>  {
>   struct device_node *node;
> @@ -1127,7 +1157,7 @@ static void __init tegra20_clock_init(struct 
> device_node *np)
>  
>   tegra_init_dup_clks(tegra_clk_duplicates, clks, TEGRA20_CLK_CLK_MAX);
>  
> - tegra_add_of_provider(np);
> + tegra_add_of_provider(np, tegra20_clk_src_onecell_get);
>   tegra_register_devclks(devclks, ARRAY_SIZE(devclks));
>  
>   tegra_clk_apply_init_table = tegra20_clock_apply_init_table;
> diff --git a/drivers/clk/tegra/clk-tegra210.c 
> b/drivers/clk/tegra/clk-tegra210.c
> index 9fb5d51ccce4..5435d01c636a 100644
> --- a/drivers/clk/tegra/clk-tegra210.c
> +++ b/drivers/clk/tegra/clk-tegra210.c
> @@ -3567,7 +3567,7 @@ static void __init tegra210_clock_init(struct 
> device_node *np)
>   tegra_init_special_resets(2, tegra210_reset_

Re: [PATCH v2 3/4] clk: tegra: Add quirk for getting CDEV1/2 clocks

[Peter De Schrijver]

On Fri, May 04, 2018 at 01:55:36AM +0300, Dmitry Osipenko wrote:
> CDEV1 and CDEV2 clocks are a bit special case, their parent clock is
> created by the pinctrl driver. It should be possible for clk user to
> request these clocks before pinctrl driver got probed and hence user will
> get an orphaned clock. That might be undesirable because user expects
> parent clock to be enabled by the child, so let's return EPROBE_DEFER
> until parent clock appear.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/clk/tegra/clk.c | 34 +-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
> index ba923f0d5953..04cbe7e9eff3 100644
> --- a/drivers/clk/tegra/clk.c
> +++ b/drivers/clk/tegra/clk.c
> @@ -298,6 +298,38 @@ static struct reset_controller_dev rst_ctlr = {
>   .of_reset_n_cells = 1,
>  };
>  
> +static struct clk *tegra_of_clk_src_onecell_get(struct of_phandle_args 
> *clkspec,
> + void *data)
> +{
> + struct clk_hw *parent_hw;
> + struct clk_hw *hw;
> + struct clk *clk;
> + const char *name;
> +
> + clk = of_clk_src_onecell_get(clkspec, data);
> + if (IS_ERR(clk))
> + return clk;
> +
> + name = __clk_get_name(clk);
> +
> + /*
> +  * Tegra20 CDEV1 and CDEV2 clocks are a bit special case, their parent
> +  * clock is created by the pinctrl driver. It is possible for clk user
> +  * to request these clocks before pinctrl driver got probed and hence
> +  * user will get an orphaned clock. That might be undesirable because
> +  * user may expect parent clock to be enabled by the child.
> +  */
> + if (!strcmp(name, "cdev1") || !strcmp(name, "cdev2")) {
> + hw = __clk_get_hw(clk);
> +
> + parent_hw = clk_hw_get_parent(hw);
> + if (!parent_hw)
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + return clk;
> +}
> +

Rather than retrieving the struct clk * and comparing the names, you can use
the DT ID in  clkspec->args[0] and compare against TEGRA20_CLK_CDEV1 and
TEGRA20_CLK_CDEV2.

Peter.

>  void __init tegra_add_of_provider(struct device_node *np)
>  {
>   int i;
> @@ -314,7 +346,7 @@ void __init tegra_add_of_provider(struct device_node *np)
>  
>   clk_data.clks = clks;
>   clk_data.clk_num = clk_num;
> - of_clk_add_provider(np, of_clk_src_onecell_get, _data);
> + of_clk_add_provider(np, tegra_of_clk_src_onecell_get, _data);
>  
>   rst_ctlr.of_node = np;
>   rst_ctlr.nr_resets = periph_banks * 32 + num_special_reset;
> -- 
> 2.17.0
> 

Re: [PATCH v2 3/4] clk: tegra: Add quirk for getting CDEV1/2 clocks

[Peter De Schrijver]

On Fri, May 04, 2018 at 01:55:36AM +0300, Dmitry Osipenko wrote:
> CDEV1 and CDEV2 clocks are a bit special case, their parent clock is
> created by the pinctrl driver. It should be possible for clk user to
> request these clocks before pinctrl driver got probed and hence user will
> get an orphaned clock. That might be undesirable because user expects
> parent clock to be enabled by the child, so let's return EPROBE_DEFER
> until parent clock appear.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/clk/tegra/clk.c | 34 +-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
> index ba923f0d5953..04cbe7e9eff3 100644
> --- a/drivers/clk/tegra/clk.c
> +++ b/drivers/clk/tegra/clk.c
> @@ -298,6 +298,38 @@ static struct reset_controller_dev rst_ctlr = {
>   .of_reset_n_cells = 1,
>  };
>  
> +static struct clk *tegra_of_clk_src_onecell_get(struct of_phandle_args 
> *clkspec,
> + void *data)
> +{
> + struct clk_hw *parent_hw;
> + struct clk_hw *hw;
> + struct clk *clk;
> + const char *name;
> +
> + clk = of_clk_src_onecell_get(clkspec, data);
> + if (IS_ERR(clk))
> + return clk;
> +
> + name = __clk_get_name(clk);
> +
> + /*
> +  * Tegra20 CDEV1 and CDEV2 clocks are a bit special case, their parent
> +  * clock is created by the pinctrl driver. It is possible for clk user
> +  * to request these clocks before pinctrl driver got probed and hence
> +  * user will get an orphaned clock. That might be undesirable because
> +  * user may expect parent clock to be enabled by the child.
> +  */
> + if (!strcmp(name, "cdev1") || !strcmp(name, "cdev2")) {
> + hw = __clk_get_hw(clk);
> +
> + parent_hw = clk_hw_get_parent(hw);
> + if (!parent_hw)
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + return clk;
> +}
> +

Rather than retrieving the struct clk * and comparing the names, you can use
the DT ID in  clkspec->args[0] and compare against TEGRA20_CLK_CDEV1 and
TEGRA20_CLK_CDEV2.

Peter.

>  void __init tegra_add_of_provider(struct device_node *np)
>  {
>   int i;
> @@ -314,7 +346,7 @@ void __init tegra_add_of_provider(struct device_node *np)
>  
>   clk_data.clks = clks;
>   clk_data.clk_num = clk_num;
> - of_clk_add_provider(np, of_clk_src_onecell_get, _data);
> + of_clk_add_provider(np, tegra_of_clk_src_onecell_get, _data);
>  
>   rst_ctlr.of_node = np;
>   rst_ctlr.nr_resets = periph_banks * 32 + num_special_reset;
> -- 
> 2.17.0
> 

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:15AM +0300, Dmitry Osipenko wrote:
> CDEV1/CDEV2 clocks could have corresponding oscillator clock divider as
> a parent. Add these dividers in order to be able to provide that parent
> option.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/clk/tegra/clk-tegra20.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 0ee56dd04cec..16cf4108f2ff 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -26,6 +26,8 @@
>  #include "clk.h"
>  #include "clk-id.h"
>  
> +#define MISC_CLK_ENB 0x48
> +
>  #define OSC_CTRL 0x50
>  #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
>  #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
> @@ -831,6 +833,16 @@ static void __init tegra20_periph_clk_init(void)
>   periph_clk_enb_refcnt);
>   clks[TEGRA20_CLK_PEX] = clk;
>  
> + /* cdev1 OSC divider */
> + clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
> +  0, clk_base + MISC_CLK_ENB, 20, 2,
> +  CLK_DIVIDER_POWER_OF_TWO, NULL);
> +

I don't know if this divider can be changed glitchlessly so to be safe,
I would mark this readonly, so add the CLK_DIVIDER_READ_ONLY flag.

> + /* cdev2 OSC divider */
> + clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
> +  0, clk_base + MISC_CLK_ENB, 22, 2,
> +  CLK_DIVIDER_POWER_OF_TWO, NULL);
> +
>   /* cdev1 */
>   clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0, 2600);
>   clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed", 0,
> -- 
> 2.17.0
> 

Peter.

Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:15AM +0300, Dmitry Osipenko wrote:
> CDEV1/CDEV2 clocks could have corresponding oscillator clock divider as
> a parent. Add these dividers in order to be able to provide that parent
> option.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/clk/tegra/clk-tegra20.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 0ee56dd04cec..16cf4108f2ff 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -26,6 +26,8 @@
>  #include "clk.h"
>  #include "clk-id.h"
>  
> +#define MISC_CLK_ENB 0x48
> +
>  #define OSC_CTRL 0x50
>  #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
>  #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
> @@ -831,6 +833,16 @@ static void __init tegra20_periph_clk_init(void)
>   periph_clk_enb_refcnt);
>   clks[TEGRA20_CLK_PEX] = clk;
>  
> + /* cdev1 OSC divider */
> + clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
> +  0, clk_base + MISC_CLK_ENB, 20, 2,
> +  CLK_DIVIDER_POWER_OF_TWO, NULL);
> +

I don't know if this divider can be changed glitchlessly so to be safe,
I would mark this readonly, so add the CLK_DIVIDER_READ_ONLY flag.

> + /* cdev2 OSC divider */
> + clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
> +  0, clk_base + MISC_CLK_ENB, 22, 2,
> +  CLK_DIVIDER_POWER_OF_TWO, NULL);
> +
>   /* cdev1 */
>   clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0, 2600);
>   clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed", 0,
> -- 
> 2.17.0
> 

Peter.

Re: [PATCH v1 3/4] clk: tegra20: Set correct parents for CDEV1/2 clocks

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:17AM +0300, Dmitry Osipenko wrote:
> Parents of CDEV1/2 clocks are determined by muxing of the corresponding
> pins. Pinctrl driver now provides the CDEV1/2 clock muxes and hence
> CDEV1/2 clocks could have correct parents. Set CDEV1/2 parents to the
> corresponding muxes to fix the parents.
> 
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
Acked-By:  Peter De Schrijver <pdeschrij...@nvidia.com>

> ---
>  drivers/clk/tegra/clk-tegra20.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 16cf4108f2ff..7e8b6de86d89 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -844,14 +844,12 @@ static void __init tegra20_periph_clk_init(void)
>CLK_DIVIDER_POWER_OF_TWO, NULL);
>  
>   /* cdev1 */
> - clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0, 2600);
> - clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed", 0,
> + clk = tegra_clk_register_periph_gate("cdev1", "cdev1_mux", 0,
>   clk_base, 0, 94, periph_clk_enb_refcnt);
>   clks[TEGRA20_CLK_CDEV1] = clk;
>  
>   /* cdev2 */
> - clk = clk_register_fixed_rate(NULL, "cdev2_fixed", NULL, 0, 2600);
> - clk = tegra_clk_register_periph_gate("cdev2", "cdev2_fixed", 0,
> + clk = tegra_clk_register_periph_gate("cdev2", "cdev2_mux", 0,
>   clk_base, 0, 93, periph_clk_enb_refcnt);
>   clks[TEGRA20_CLK_CDEV2] = clk;
>  
> -- 
> 2.17.0
> 

Re: [PATCH v1 3/4] clk: tegra20: Set correct parents for CDEV1/2 clocks

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:17AM +0300, Dmitry Osipenko wrote:
> Parents of CDEV1/2 clocks are determined by muxing of the corresponding
> pins. Pinctrl driver now provides the CDEV1/2 clock muxes and hence
> CDEV1/2 clocks could have correct parents. Set CDEV1/2 parents to the
> corresponding muxes to fix the parents.
> 
> Signed-off-by: Dmitry Osipenko 
Acked-By:  Peter De Schrijver 

> ---
>  drivers/clk/tegra/clk-tegra20.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 16cf4108f2ff..7e8b6de86d89 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -844,14 +844,12 @@ static void __init tegra20_periph_clk_init(void)
>CLK_DIVIDER_POWER_OF_TWO, NULL);
>  
>   /* cdev1 */
> - clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL, 0, 2600);
> - clk = tegra_clk_register_periph_gate("cdev1", "cdev1_fixed", 0,
> + clk = tegra_clk_register_periph_gate("cdev1", "cdev1_mux", 0,
>   clk_base, 0, 94, periph_clk_enb_refcnt);
>   clks[TEGRA20_CLK_CDEV1] = clk;
>  
>   /* cdev2 */
> - clk = clk_register_fixed_rate(NULL, "cdev2_fixed", NULL, 0, 2600);
> - clk = tegra_clk_register_periph_gate("cdev2", "cdev2_fixed", 0,
> + clk = tegra_clk_register_periph_gate("cdev2", "cdev2_mux", 0,
>   clk_base, 0, 93, periph_clk_enb_refcnt);
>   clks[TEGRA20_CLK_CDEV2] = clk;
>  
> -- 
> 2.17.0
> 

Re: [PATCH v1 2/4] pinctrl: tegra20: Provide CDEV1/2 clock muxes

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:16AM +0300, Dmitry Osipenko wrote:
> Muxing of pins MCLK1/2 determine the muxing of the corresponding clocks.
> Make pinctrl driver to provide clock muxes for the CDEV1/2 pingroups, so
> that main clk-controller driver could get an actual parent clock for the
> CDEV1/2 clocks.
> 
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> ---
>  drivers/pinctrl/tegra/pinctrl-tegra.c   | 11 -
>  drivers/pinctrl/tegra/pinctrl-tegra.h   | 11 +
>  drivers/pinctrl/tegra/pinctrl-tegra20.c | 30 -
>  3 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c 
> b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 72c718e66ebb..49c7c1499bc3 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -33,17 +33,6 @@
>  #include "../pinctrl-utils.h"
>  #include "pinctrl-tegra.h"
>  
> -struct tegra_pmx {
> - struct device *dev;
> - struct pinctrl_dev *pctl;
> -
> - const struct tegra_pinctrl_soc_data *soc;
> - const char **group_pins;
> -
> - int nbanks;
> - void __iomem **regs;
> -};
> -
>  static inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>  {
>   return readl(pmx->regs[bank] + reg);
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h 
> b/drivers/pinctrl/tegra/pinctrl-tegra.h
> index 33b17cb1471e..aa33c20766c4 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.h
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
> @@ -16,6 +16,17 @@
>  #ifndef __PINMUX_TEGRA_H__
>  #define __PINMUX_TEGRA_H__
>  
> +struct tegra_pmx {
> + struct device *dev;
> + struct pinctrl_dev *pctl;
> +
> + const struct tegra_pinctrl_soc_data *soc;
> + const char **group_pins;
> +
> + int nbanks;
> + void __iomem **regs;
> +};
> +
>  enum tegra_pinconf_param {
>   /* argument: tegra_pinconf_pull */
>   TEGRA_PINCONF_PARAM_PULL,
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra20.c 
> b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> index 7e38ee9bae78..f31e39d797f9 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra20.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> @@ -19,6 +19,7 @@
>   * more details.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2231,9 +2232,36 @@ static const struct tegra_pinctrl_soc_data 
> tegra20_pinctrl = {
>   .drvtype_in_mux = false,
>  };
>  
> +static const char *cdev1_parents[4] = {

No need to have 4 here, just cdev1_parents[] will do. 

> + "cdev1_osc_div", "pll_a_out0", "pll_m_out1", "audio",
> +};
> +
> +static const char *cdev2_parents[4] = {
> + "cdev2_osc_div", "hclk", "pclk", "pll_p_out4",
> +};
> +

Same here.

> +static void tegra20_pinctrl_register_clock_muxes(struct platform_device 
> *pdev)
> +{
> + struct tegra_pmx *pmx = platform_get_drvdata(pdev);
> +
> + clk_register_mux(NULL, "cdev1_mux", cdev1_parents, 4, 0,
> +  pmx->regs[1] + 0x8, 2, 2, CLK_MUX_READ_ONLY, NULL);
> +
> + clk_register_mux(NULL, "cdev2_mux", cdev2_parents, 4, 0,
> +  pmx->regs[1] + 0x8, 4, 2, CLK_MUX_READ_ONLY, NULL);
> +}
> +
>  static int tegra20_pinctrl_probe(struct platform_device *pdev)
>  {
> - return tegra_pinctrl_probe(pdev, _pinctrl);
> + int err;
> +
> + err = tegra_pinctrl_probe(pdev, _pinctrl);
> + if (err)
> + return err;
> +
> + tegra20_pinctrl_register_clock_muxes(pdev);
> +
> + return 0;
>  }
>  
>  static const struct of_device_id tegra20_pinctrl_of_match[] = {

Apart from these nitpicks:

Acked-By:  Peter De Schrijver <pdeschrij...@nvidia.com>

Re: [PATCH v1 2/4] pinctrl: tegra20: Provide CDEV1/2 clock muxes

[Peter De Schrijver]

On Fri, Apr 27, 2018 at 02:58:16AM +0300, Dmitry Osipenko wrote:
> Muxing of pins MCLK1/2 determine the muxing of the corresponding clocks.
> Make pinctrl driver to provide clock muxes for the CDEV1/2 pingroups, so
> that main clk-controller driver could get an actual parent clock for the
> CDEV1/2 clocks.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/pinctrl/tegra/pinctrl-tegra.c   | 11 -
>  drivers/pinctrl/tegra/pinctrl-tegra.h   | 11 +
>  drivers/pinctrl/tegra/pinctrl-tegra20.c | 30 -
>  3 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c 
> b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 72c718e66ebb..49c7c1499bc3 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -33,17 +33,6 @@
>  #include "../pinctrl-utils.h"
>  #include "pinctrl-tegra.h"
>  
> -struct tegra_pmx {
> - struct device *dev;
> - struct pinctrl_dev *pctl;
> -
> - const struct tegra_pinctrl_soc_data *soc;
> - const char **group_pins;
> -
> - int nbanks;
> - void __iomem **regs;
> -};
> -
>  static inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>  {
>   return readl(pmx->regs[bank] + reg);
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h 
> b/drivers/pinctrl/tegra/pinctrl-tegra.h
> index 33b17cb1471e..aa33c20766c4 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.h
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
> @@ -16,6 +16,17 @@
>  #ifndef __PINMUX_TEGRA_H__
>  #define __PINMUX_TEGRA_H__
>  
> +struct tegra_pmx {
> + struct device *dev;
> + struct pinctrl_dev *pctl;
> +
> + const struct tegra_pinctrl_soc_data *soc;
> + const char **group_pins;
> +
> + int nbanks;
> + void __iomem **regs;
> +};
> +
>  enum tegra_pinconf_param {
>   /* argument: tegra_pinconf_pull */
>   TEGRA_PINCONF_PARAM_PULL,
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra20.c 
> b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> index 7e38ee9bae78..f31e39d797f9 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra20.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> @@ -19,6 +19,7 @@
>   * more details.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2231,9 +2232,36 @@ static const struct tegra_pinctrl_soc_data 
> tegra20_pinctrl = {
>   .drvtype_in_mux = false,
>  };
>  
> +static const char *cdev1_parents[4] = {

No need to have 4 here, just cdev1_parents[] will do. 

> + "cdev1_osc_div", "pll_a_out0", "pll_m_out1", "audio",
> +};
> +
> +static const char *cdev2_parents[4] = {
> + "cdev2_osc_div", "hclk", "pclk", "pll_p_out4",
> +};
> +

Same here.

> +static void tegra20_pinctrl_register_clock_muxes(struct platform_device 
> *pdev)
> +{
> + struct tegra_pmx *pmx = platform_get_drvdata(pdev);
> +
> + clk_register_mux(NULL, "cdev1_mux", cdev1_parents, 4, 0,
> +  pmx->regs[1] + 0x8, 2, 2, CLK_MUX_READ_ONLY, NULL);
> +
> + clk_register_mux(NULL, "cdev2_mux", cdev2_parents, 4, 0,
> +  pmx->regs[1] + 0x8, 4, 2, CLK_MUX_READ_ONLY, NULL);
> +}
> +
>  static int tegra20_pinctrl_probe(struct platform_device *pdev)
>  {
> - return tegra_pinctrl_probe(pdev, _pinctrl);
> + int err;
> +
> + err = tegra_pinctrl_probe(pdev, _pinctrl);
> + if (err)
> + return err;
> +
> + tegra20_pinctrl_register_clock_muxes(pdev);
> +
> + return 0;
>  }
>  
>  static const struct of_device_id tegra20_pinctrl_of_match[] = {

Apart from these nitpicks:

Acked-By:  Peter De Schrijver 

Re: [PATCH] ARM: tegra: fix ulpi regression on tegra20

[Peter De Schrijver]

On Tue, Apr 24, 2018 at 05:38:41PM +0300, Dmitry Osipenko wrote:
> Hi Marcel,
> 
> On 24.04.2018 01:05, Marcel Ziswiler wrote:
> > Hi Dmitry
> > 
> > On Fri, 2018-04-20 at 13:50 +0300, Dmitry Osipenko wrote:
> >> ...
> >> I managed to find CDEV clocks in TRM this time.
> > 
> > And where exactly in which TRM? In all my attempts at finding anything
> > CDEV2 is just a pad group which includes the DAP_MCLK2 pad in question. 
> 
> That's the DEV2 clock you're talking about below.
> 
> >> Seems assigning CDEV2 clock to
> >> "ulpi-link" was correct
> > 
> > Sorry, but I do really not see how you can get to any such conclusion.
> > 
> > Whatever that CDEV2 clock exactly is. Its offset 93 points towards the
> > CLK_RST_CONTROLLER_CLK_OUT_ENB_U_0 register which has CLK_ENB_DEV2_OUT
> > at bit position 29 reading "Enable clock to DEV2 pad". While the TRM
> > misses further documenting what exactly that DEV2 pad should be I guess
> > it may point towards our suspected DAP_MCLK2. So for some reason
> > besides PLL_P_OUT4 which is what that pad is actually muxed to also
> > some additional DEV2 pad clock needs enabling. In addition there would
> > be a CLK_RST_CONTROLLER_MISC_CLK_ENB_0 register where one could also
> > specify a DEV2_OSC_DIV_SEL but I would assume that one only applies if
> > the pad in question being muxed to OSC which is not the case at least
> > in all device trees I have looked at.
> > 
> >> and both CDEV2 and PLL_P_OUT4 should be enabled,
> > 
> > Agreed, basically the DAP_MCLK2 pad seems gated by an additional enable
> > called CLK_ENB_DEV2_OUT.
> > 
> >> CDEV2
> >> should gate the PLL_P_OUT4 that feeds USB HW and PLL_P_OUT4 should be
> >> always-enabled because it is enabled by init_table, but apparently it
> >> is getting
> >> disabled erroneously.
> > 
> > At least that was the case back when I had trouble getting ULPI to work
> > on T20. Strangely latest -next right now does no longer seem to suffer
> > from that same issue even if my patch is reverted but as mentioned
> > before nobody stops the required PLL_P_OUT4 which is what is actually
> > driving DAP_MCLK2 to not be changed behind the scenes breaking the
> > whole thing again.
> > 
> >> Marcel, could you please revert your patch, add
> >> "trace_event=clk_enable,clk_disable,clk_set_parent tp_printk" to
> >> kernels cmdline
> >> and post the log?
> > 
> > Yes, the only difference in those traces is whether or not that non-
> > existent CDEV2 clock is enabled or not:
> > 
> > [1.897521] clk_enable: cdev2_fixed
> > [1.901008] clk_enable: cdev2
> > 
> > I also do have an explanation why it kept working in my case. Probably
> > the boot ROM or U-Boot is already setting that bit.
> > 
> >> It looks like there is some clk framework bug,
> > 
> > My conclusion is that there should be a DAP_MCLK2 clock which is gated
> > by that CLK_ENB_DEV2_OUT but really outputs a T20 internal clock
> > according to its pin muxing which if set to OSC may be further divided
> > down by DEV2_OSC_DIV_SEL.
> 
> Could be that DEV2_OSC_DIV_SEL is only relevant when DAP_MCLK2 is mux'ed to 
> OSC.
> Maybe Peter could clarify everything.
> 

Yes. This looks likely. Problem here is that the source selection for cdev1
and cdev2 is done via pinmuxing rather than via a regular mux control in CAR.
So probably the "OSC" input is not really OSC but the output of the divider
configured in CLK_RST_CONTROLLER_MISC_CLK_ENB_0.

Possible solutions I see are:

1) make the tegra20 pinctrl driver a clock provider for the cdev1 and cdev2
   clocks. We would then need to introduce 2 new clocks cdev1_mux and
   cdev2_mux which represent pinmuxes. cdev1 and cdev2 would use those as
   a parent. cdev1_fixed and cdev2_fixed would need to be renamed to refelect
   the settings in CLK_RST_CONTROLLER_MISC_CLK_ENB_0 and can be inputs for
   cdev1_mux and cdev2_mux

2) read the pinmux configuration for cdev1 and cdev2 at boot. this would
   require mapping the apbmisc aperture in the clock driver, but we already
   map the PMC aperture, so there's a precedent for mappings outside CAR.
   Then define the cdev1 and cdev2 parent accordingly. Also cdev1_fixed and
   cdev2_fixed could be defined as the divided version of OSC according to
   the settings in CLK_RST_CONTROLLER_MISC_CLK_ENB_0. Limitation of this
   approach is that we assume these settings are never changed at runtime.
   This seems acceptable to me though. Or do we know of a use case where the
   setting is changed at runtime?

Solution 1 is nicer, but would require backwards-incompatible changes to the
DT bindings.

Peter.

Re: [PATCH] ARM: tegra: fix ulpi regression on tegra20

[Peter De Schrijver]

On Tue, Apr 24, 2018 at 05:38:41PM +0300, Dmitry Osipenko wrote:
> Hi Marcel,
> 
> On 24.04.2018 01:05, Marcel Ziswiler wrote:
> > Hi Dmitry
> > 
> > On Fri, 2018-04-20 at 13:50 +0300, Dmitry Osipenko wrote:
> >> ...
> >> I managed to find CDEV clocks in TRM this time.
> > 
> > And where exactly in which TRM? In all my attempts at finding anything
> > CDEV2 is just a pad group which includes the DAP_MCLK2 pad in question. 
> 
> That's the DEV2 clock you're talking about below.
> 
> >> Seems assigning CDEV2 clock to
> >> "ulpi-link" was correct
> > 
> > Sorry, but I do really not see how you can get to any such conclusion.
> > 
> > Whatever that CDEV2 clock exactly is. Its offset 93 points towards the
> > CLK_RST_CONTROLLER_CLK_OUT_ENB_U_0 register which has CLK_ENB_DEV2_OUT
> > at bit position 29 reading "Enable clock to DEV2 pad". While the TRM
> > misses further documenting what exactly that DEV2 pad should be I guess
> > it may point towards our suspected DAP_MCLK2. So for some reason
> > besides PLL_P_OUT4 which is what that pad is actually muxed to also
> > some additional DEV2 pad clock needs enabling. In addition there would
> > be a CLK_RST_CONTROLLER_MISC_CLK_ENB_0 register where one could also
> > specify a DEV2_OSC_DIV_SEL but I would assume that one only applies if
> > the pad in question being muxed to OSC which is not the case at least
> > in all device trees I have looked at.
> > 
> >> and both CDEV2 and PLL_P_OUT4 should be enabled,
> > 
> > Agreed, basically the DAP_MCLK2 pad seems gated by an additional enable
> > called CLK_ENB_DEV2_OUT.
> > 
> >> CDEV2
> >> should gate the PLL_P_OUT4 that feeds USB HW and PLL_P_OUT4 should be
> >> always-enabled because it is enabled by init_table, but apparently it
> >> is getting
> >> disabled erroneously.
> > 
> > At least that was the case back when I had trouble getting ULPI to work
> > on T20. Strangely latest -next right now does no longer seem to suffer
> > from that same issue even if my patch is reverted but as mentioned
> > before nobody stops the required PLL_P_OUT4 which is what is actually
> > driving DAP_MCLK2 to not be changed behind the scenes breaking the
> > whole thing again.
> > 
> >> Marcel, could you please revert your patch, add
> >> "trace_event=clk_enable,clk_disable,clk_set_parent tp_printk" to
> >> kernels cmdline
> >> and post the log?
> > 
> > Yes, the only difference in those traces is whether or not that non-
> > existent CDEV2 clock is enabled or not:
> > 
> > [1.897521] clk_enable: cdev2_fixed
> > [1.901008] clk_enable: cdev2
> > 
> > I also do have an explanation why it kept working in my case. Probably
> > the boot ROM or U-Boot is already setting that bit.
> > 
> >> It looks like there is some clk framework bug,
> > 
> > My conclusion is that there should be a DAP_MCLK2 clock which is gated
> > by that CLK_ENB_DEV2_OUT but really outputs a T20 internal clock
> > according to its pin muxing which if set to OSC may be further divided
> > down by DEV2_OSC_DIV_SEL.
> 
> Could be that DEV2_OSC_DIV_SEL is only relevant when DAP_MCLK2 is mux'ed to 
> OSC.
> Maybe Peter could clarify everything.
> 

Yes. This looks likely. Problem here is that the source selection for cdev1
and cdev2 is done via pinmuxing rather than via a regular mux control in CAR.
So probably the "OSC" input is not really OSC but the output of the divider
configured in CLK_RST_CONTROLLER_MISC_CLK_ENB_0.

Possible solutions I see are:

1) make the tegra20 pinctrl driver a clock provider for the cdev1 and cdev2
   clocks. We would then need to introduce 2 new clocks cdev1_mux and
   cdev2_mux which represent pinmuxes. cdev1 and cdev2 would use those as
   a parent. cdev1_fixed and cdev2_fixed would need to be renamed to refelect
   the settings in CLK_RST_CONTROLLER_MISC_CLK_ENB_0 and can be inputs for
   cdev1_mux and cdev2_mux

2) read the pinmux configuration for cdev1 and cdev2 at boot. this would
   require mapping the apbmisc aperture in the clock driver, but we already
   map the PMC aperture, so there's a precedent for mappings outside CAR.
   Then define the cdev1 and cdev2 parent accordingly. Also cdev1_fixed and
   cdev2_fixed could be defined as the divided version of OSC according to
   the settings in CLK_RST_CONTROLLER_MISC_CLK_ENB_0. Limitation of this
   approach is that we assume these settings are never changed at runtime.
   This seems acceptable to me though. Or do we know of a use case where the
   setting is changed at runtime?

Solution 1 is nicer, but would require backwards-incompatible changes to the
DT bindings.

Peter.

Re: [PATCH v3 09/11] cpufreq: tegra124-cpufreq: extend to support Tegra210

[Peter De Schrijver]

On Mon, Mar 12, 2018 at 12:15:22PM +, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> > support in this driver. Also allow for the case where the CPU voltage is
> > controlled directly by the DFLL rather than by a separate regulator object.
> > 
> > Signed-off-by: Peter De Schrijver <pdeschrij...@nvidia.com>
> > ---
> >  drivers/cpufreq/tegra124-cpufreq.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/tegra124-cpufreq.c 
> > b/drivers/cpufreq/tegra124-cpufreq.c
> > index 4353025..f8e01a8 100644
> > --- a/drivers/cpufreq/tegra124-cpufreq.c
> > +++ b/drivers/cpufreq/tegra124-cpufreq.c
> > @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct 
> > tegra124_cpufreq_priv *priv)
> >  {
> > clk_set_parent(priv->cpu_clk, priv->pllp_clk);
> > clk_disable_unprepare(priv->dfll_clk);
> > -   regulator_sync_voltage(priv->vdd_cpu_reg);
> > +   if (priv->vdd_cpu_reg)
> > +   regulator_sync_voltage(priv->vdd_cpu_reg);
> > clk_set_parent(priv->cpu_clk, priv->pllx_clk);
> >  }
> 
> OK, so this bit does not make sense to me. In the above we are switching
> from the DFLL to the PLL (ie. disabling the DFLL) and so to ensure we
> are operating at the correct voltage after disabling the DFLL we need to
> sync the voltage. Seems we would need to do this for all devices, no?
> How is the different between Tegra124 and Tegra210?

Yes. So in case of i2c the regulator framework will reapply the voltage it
knows which in our case is the boot voltage for VDD_CPU because noone else
from a regulator framework pov has ever changed the voltage. In case of PWM
putting the PWM output pad in tri state will cause the OVR regulator to output
a hardware defined voltage. This is done as part of the dfll_clk_disable()
function. To summarize:

switch from pll_x to DFLL for i2c regulator:

entry: voltage is boot voltage set bootloader
1) set dfll rate to pll_x rate
2) set parent to pll_p so we run at 408MHz which is below Fmax@Vmin when
   running from PLL
3) enable DFLL this will switch to closed loop mode and start controlling
   the voltage via i2c, however because we are below Fmax@Vmin there's no
   V/f curve violation.
4) change parent from pll_x to DFLL

switch from DFLL to pll_x for i2c regulator:

entry: voltage is whatever the DFLL has programmed but not lower than Vmin
1) set parent to pll_p so we run at 408MHz (below Fmax@Vmin)
2) disable DFLL, this will cause the voltage to be the last value programmed
   by the DFLL.
3) go back to the voltage as programmed by the boot loader using
   regulator_sync_voltage().
4) change parent from DFLL to pll_x. Because pll_x is still programmed at
   the bootloader frequency, we're within the V/f curve.

switch from pll_x to DFLL for PWM regulator:

entry: voltage is boot voltage set by hardware because PWM pin is in tristate
1) set dfll rate to pll_x rate
2) set parent to pll_p so we run at 408MHz which is below Fmax@Vmin when
   running from PLL
3) enable DFLL this will set the PWM pad to output, switch to closed loop mode
   and start controlling the voltage via PWM, however because we are below
   Fmax@Vmin there's no V/f curve violation.
4) change parent from pll_x to DFLL

switch from DFLL to pll_x for PWM regulator:

entry: voltage is whatever the DFLL has programmed but not lower than Vmin
1) set parent to pll_p so we run at 408MHz (below Fmax@Vmin)
2) disable DFLL, this will cause the PWM pad to be programmed to tristate
   and the voltage to change back to the hardware defined voltage.
3) change parent from DFLL to pll_x. Because pll_x is still programmed at
   the bootloader frequency, we're within the V/f curve.

Peter.

Re: [PATCH v3 09/11] cpufreq: tegra124-cpufreq: extend to support Tegra210

[Peter De Schrijver]

On Mon, Mar 12, 2018 at 12:15:22PM +, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> > support in this driver. Also allow for the case where the CPU voltage is
> > controlled directly by the DFLL rather than by a separate regulator object.
> > 
> > Signed-off-by: Peter De Schrijver 
> > ---
> >  drivers/cpufreq/tegra124-cpufreq.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/tegra124-cpufreq.c 
> > b/drivers/cpufreq/tegra124-cpufreq.c
> > index 4353025..f8e01a8 100644
> > --- a/drivers/cpufreq/tegra124-cpufreq.c
> > +++ b/drivers/cpufreq/tegra124-cpufreq.c
> > @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct 
> > tegra124_cpufreq_priv *priv)
> >  {
> > clk_set_parent(priv->cpu_clk, priv->pllp_clk);
> > clk_disable_unprepare(priv->dfll_clk);
> > -   regulator_sync_voltage(priv->vdd_cpu_reg);
> > +   if (priv->vdd_cpu_reg)
> > +   regulator_sync_voltage(priv->vdd_cpu_reg);
> > clk_set_parent(priv->cpu_clk, priv->pllx_clk);
> >  }
> 
> OK, so this bit does not make sense to me. In the above we are switching
> from the DFLL to the PLL (ie. disabling the DFLL) and so to ensure we
> are operating at the correct voltage after disabling the DFLL we need to
> sync the voltage. Seems we would need to do this for all devices, no?
> How is the different between Tegra124 and Tegra210?

Yes. So in case of i2c the regulator framework will reapply the voltage it
knows which in our case is the boot voltage for VDD_CPU because noone else
from a regulator framework pov has ever changed the voltage. In case of PWM
putting the PWM output pad in tri state will cause the OVR regulator to output
a hardware defined voltage. This is done as part of the dfll_clk_disable()
function. To summarize:

switch from pll_x to DFLL for i2c regulator:

entry: voltage is boot voltage set bootloader
1) set dfll rate to pll_x rate
2) set parent to pll_p so we run at 408MHz which is below Fmax@Vmin when
   running from PLL
3) enable DFLL this will switch to closed loop mode and start controlling
   the voltage via i2c, however because we are below Fmax@Vmin there's no
   V/f curve violation.
4) change parent from pll_x to DFLL

switch from DFLL to pll_x for i2c regulator:

entry: voltage is whatever the DFLL has programmed but not lower than Vmin
1) set parent to pll_p so we run at 408MHz (below Fmax@Vmin)
2) disable DFLL, this will cause the voltage to be the last value programmed
   by the DFLL.
3) go back to the voltage as programmed by the boot loader using
   regulator_sync_voltage().
4) change parent from DFLL to pll_x. Because pll_x is still programmed at
   the bootloader frequency, we're within the V/f curve.

switch from pll_x to DFLL for PWM regulator:

entry: voltage is boot voltage set by hardware because PWM pin is in tristate
1) set dfll rate to pll_x rate
2) set parent to pll_p so we run at 408MHz which is below Fmax@Vmin when
   running from PLL
3) enable DFLL this will set the PWM pad to output, switch to closed loop mode
   and start controlling the voltage via PWM, however because we are below
   Fmax@Vmin there's no V/f curve violation.
4) change parent from pll_x to DFLL

switch from DFLL to pll_x for PWM regulator:

entry: voltage is whatever the DFLL has programmed but not lower than Vmin
1) set parent to pll_p so we run at 408MHz (below Fmax@Vmin)
2) disable DFLL, this will cause the PWM pad to be programmed to tristate
   and the voltage to change back to the hardware defined voltage.
3) change parent from DFLL to pll_x. Because pll_x is still programmed at
   the bootloader frequency, we're within the V/f curve.

Peter.

Re: [PATCH v3 09/11] cpufreq: tegra124-cpufreq: extend to support Tegra210

[Peter De Schrijver]

On Mon, Mar 12, 2018 at 10:14:17AM +, Jon Hunter wrote:
> 
> On 09/03/18 08:14, Peter De Schrijver wrote:
> > On Thu, Mar 08, 2018 at 11:25:04PM +, Jon Hunter wrote:
> >>
> >> On 06/02/18 16:34, Peter De Schrijver wrote:
> >>> Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> >>> support in this driver. Also allow for the case where the CPU voltage is
> >>> controlled directly by the DFLL rather than by a separate regulator 
> >>> object.
> >>>
> >>> Signed-off-by: Peter De Schrijver <pdeschrij...@nvidia.com>
> >>> ---
> >>>  drivers/cpufreq/tegra124-cpufreq.c | 15 ---
> >>>  1 file changed, 8 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/cpufreq/tegra124-cpufreq.c 
> >>> b/drivers/cpufreq/tegra124-cpufreq.c
> >>> index 4353025..f8e01a8 100644
> >>> --- a/drivers/cpufreq/tegra124-cpufreq.c
> >>> +++ b/drivers/cpufreq/tegra124-cpufreq.c
> >>> @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct 
> >>> tegra124_cpufreq_priv *priv)
> >>>  {
> >>>   clk_set_parent(priv->cpu_clk, priv->pllp_clk);
> >>>   clk_disable_unprepare(priv->dfll_clk);
> >>> - regulator_sync_voltage(priv->vdd_cpu_reg);
> >>> + if (priv->vdd_cpu_reg)
> >>> + regulator_sync_voltage(priv->vdd_cpu_reg);
> >>>   clk_set_parent(priv->cpu_clk, priv->pllx_clk);
> >>>  }
> >>>  
> >>> @@ -89,10 +90,10 @@ static int tegra124_cpufreq_probe(struct 
> >>> platform_device *pdev)
> >>>   return -ENODEV;
> >>>  
> >>>   priv->vdd_cpu_reg = regulator_get(cpu_dev, "vdd-cpu");
> >>> - if (IS_ERR(priv->vdd_cpu_reg)) {
> >>> - ret = PTR_ERR(priv->vdd_cpu_reg);
> >>> - goto out_put_np;
> >>> - }
> >>> + if (IS_ERR(priv->vdd_cpu_reg) != -EPROBE_DEFER)
> >>> + priv->vdd_cpu_reg = NULL;
> >>> + else
> >>> + return -EPROBE_DEFER;
> >>
> >> I am still not sure that we should rely on the fact that the regulator
> >> is not present in DT to imply that we do not need it. I think that we
> >> should be checking if we are using I2C mode here.
> >>
> > 
> > The cpufreq driver doesn't know this however. Also the current approach of
> > setting the same voltage when switching to pll_x is incorrect. The CVB
> > tables when using pll_x include more margin than when using the DFLL.
> 
> Ah yes I see now. However, we are going to need to update the DT doc,
> because 'vdd-cpu-supply' is listed as required.
> 

Ok.

Peter.

Re: [PATCH v3 09/11] cpufreq: tegra124-cpufreq: extend to support Tegra210

[Peter De Schrijver]

On Mon, Mar 12, 2018 at 10:14:17AM +, Jon Hunter wrote:
> 
> On 09/03/18 08:14, Peter De Schrijver wrote:
> > On Thu, Mar 08, 2018 at 11:25:04PM +, Jon Hunter wrote:
> >>
> >> On 06/02/18 16:34, Peter De Schrijver wrote:
> >>> Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> >>> support in this driver. Also allow for the case where the CPU voltage is
> >>> controlled directly by the DFLL rather than by a separate regulator 
> >>> object.
> >>>
> >>> Signed-off-by: Peter De Schrijver 
> >>> ---
> >>>  drivers/cpufreq/tegra124-cpufreq.c | 15 ---
> >>>  1 file changed, 8 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/cpufreq/tegra124-cpufreq.c 
> >>> b/drivers/cpufreq/tegra124-cpufreq.c
> >>> index 4353025..f8e01a8 100644
> >>> --- a/drivers/cpufreq/tegra124-cpufreq.c
> >>> +++ b/drivers/cpufreq/tegra124-cpufreq.c
> >>> @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct 
> >>> tegra124_cpufreq_priv *priv)
> >>>  {
> >>>   clk_set_parent(priv->cpu_clk, priv->pllp_clk);
> >>>   clk_disable_unprepare(priv->dfll_clk);
> >>> - regulator_sync_voltage(priv->vdd_cpu_reg);
> >>> + if (priv->vdd_cpu_reg)
> >>> + regulator_sync_voltage(priv->vdd_cpu_reg);
> >>>   clk_set_parent(priv->cpu_clk, priv->pllx_clk);
> >>>  }
> >>>  
> >>> @@ -89,10 +90,10 @@ static int tegra124_cpufreq_probe(struct 
> >>> platform_device *pdev)
> >>>   return -ENODEV;
> >>>  
> >>>   priv->vdd_cpu_reg = regulator_get(cpu_dev, "vdd-cpu");
> >>> - if (IS_ERR(priv->vdd_cpu_reg)) {
> >>> - ret = PTR_ERR(priv->vdd_cpu_reg);
> >>> - goto out_put_np;
> >>> - }
> >>> + if (IS_ERR(priv->vdd_cpu_reg) != -EPROBE_DEFER)
> >>> + priv->vdd_cpu_reg = NULL;
> >>> + else
> >>> + return -EPROBE_DEFER;
> >>
> >> I am still not sure that we should rely on the fact that the regulator
> >> is not present in DT to imply that we do not need it. I think that we
> >> should be checking if we are using I2C mode here.
> >>
> > 
> > The cpufreq driver doesn't know this however. Also the current approach of
> > setting the same voltage when switching to pll_x is incorrect. The CVB
> > tables when using pll_x include more margin than when using the DFLL.
> 
> Ah yes I see now. However, we are going to need to update the DT doc,
> because 'vdd-cpu-supply' is listed as required.
> 

Ok.

Peter.

Re: [PATCH v3 05/11] clk: tegra: prepare dfll driver for PWM regulator

[Peter De Schrijver]

On Mon, Mar 12, 2018 at 11:08:51AM +, Jon Hunter wrote:
> 
> On 12/03/18 09:14, Peter De Schrijver wrote:
> > On Thu, Mar 08, 2018 at 10:50:06PM +, Jon Hunter wrote:
> >>
> >> On 06/02/18 16:34, Peter De Schrijver wrote:
> >>> This patch prepares the dfll driver to work with PWM regulators.
> >>> To do this we introduce a new array lut_uv which gives the voltage for
> >>> a given index generated by the dfll logic. This index will then be
> >>> translated to a PMIC voltage ID in case of I2C using the i2c_lut. In case
> >>> of a PWM regulator, it will be used to determine the PWM duty cycle.
> >>> We also introduce lut_bottom which holds the lowest voltage we will ever
> >>> need. In case of I2C this can be set to zero because the i2c_lut will be
> >>> initialized such that entry 0 will be the lowest voltage we will ever
> >>> need. In case of PWM, the lowest voltage is determined by the regulator
> >>> hardware so we need this software limit. Note that this is different
> >>> from lut_min which gives the lowest voltage we allow taking temperature
> >>> into account. In a future patchset we will update lut_vmin dynamically.
> >>> Similarly lut_max will be the highest voltage allowed taking temperature
> >>> into accouint. Also this will be updated dynamically once temperature
> >>> dependence will be introduced.
> >>>
> >>> Signed-off-by: Peter De Schrijver <pdeschrij...@nvidia.com>
> >>> ---
> >>>  drivers/clk/tegra/clk-dfll.c | 62 
> >>> ++--
> >>>  1 file changed, 37 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> >>> index 0a7deee..fa97763 100644
> >>> --- a/drivers/clk/tegra/clk-dfll.c
> >>> +++ b/drivers/clk/tegra/clk-dfll.c
> >>> @@ -301,9 +301,10 @@ struct tegra_dfll {
> >>>   u32 i2c_slave_addr;
> >>>  
> >>>   /* i2c_lut array entries are regulator framework selectors */
> >>> - unsignedi2c_lut[MAX_DFLL_VOLTAGES];
> >>> - int i2c_lut_size;
> >>> - u8  lut_min, lut_max, lut_safe;
> >>> + unsigned inti2c_lut[MAX_DFLL_VOLTAGES];
> >>> + unsigned intlut_uv[MAX_DFLL_VOLTAGES];
> >>> + int lut_size;
> >>> + u8  lut_bottom, lut_min, lut_max, lut_safe;
> >>>  };
> >>>  
> >>>  #define clk_hw_to_dfll(_hw) container_of(_hw, struct tegra_dfll, 
> >>> dfll_clk_hw)
> >>> @@ -531,10 +532,10 @@ static void dfll_load_i2c_lut(struct tegra_dfll *td)
> >>>   u32 val;
> >>>  
> >>>   for (i = 0; i < MAX_DFLL_VOLTAGES; i++) {
> >>> - if (i < td->lut_min)
> >>> - lut_index = td->lut_min;
> >>> - else if (i > td->lut_max)
> >>> - lut_index = td->lut_max;
> >>> + if (i < td->lut_bottom)
> >>> + lut_index = td->lut_bottom;
> >>> + else if (i > td->lut_size - 1)
> >>> + lut_index = td->lut_size - 1;
> >>>   else
> >>>   lut_index = i;
> >>>  
> >>> @@ -594,9 +595,9 @@ static void dfll_init_out_if(struct tegra_dfll *td)
> >>>  {
> >>>   u32 val;
> >>>  
> >>> - td->lut_min = 0;
> >>> - td->lut_max = td->i2c_lut_size - 1;
> >>> - td->lut_safe = td->lut_min + 1;
> >>> + td->lut_min = td->lut_bottom;
> >>> + td->lut_max = td->lut_size - 1;
> >>> + td->lut_safe = td->lut_min + (td->lut_min < td->lut_max ? 1 : 0);
> >>>  
> >>>   dfll_i2c_writel(td, 0, DFLL_OUTPUT_CFG);
> >>>   val = (td->lut_safe << DFLL_OUTPUT_CFG_SAFE_SHIFT) |
> >>> @@ -619,11 +620,11 @@ static void dfll_init_out_if(struct tegra_dfll *td)
> >>>   */
> >>>  
> >>>  /**
> >>> - * find_lut_index_for_rate - determine I2C LUT index for given DFLL rate
> >>> + * find_lut_index_for_rate - determine LUT index for given DFLL rate
> >>>   * @td: DFLL instance
> >>>   * @rate: clock rate
> >>&g

Re: [PATCH v3 05/11] clk: tegra: prepare dfll driver for PWM regulator

[Peter De Schrijver]

On Mon, Mar 12, 2018 at 11:08:51AM +, Jon Hunter wrote:
> 
> On 12/03/18 09:14, Peter De Schrijver wrote:
> > On Thu, Mar 08, 2018 at 10:50:06PM +, Jon Hunter wrote:
> >>
> >> On 06/02/18 16:34, Peter De Schrijver wrote:
> >>> This patch prepares the dfll driver to work with PWM regulators.
> >>> To do this we introduce a new array lut_uv which gives the voltage for
> >>> a given index generated by the dfll logic. This index will then be
> >>> translated to a PMIC voltage ID in case of I2C using the i2c_lut. In case
> >>> of a PWM regulator, it will be used to determine the PWM duty cycle.
> >>> We also introduce lut_bottom which holds the lowest voltage we will ever
> >>> need. In case of I2C this can be set to zero because the i2c_lut will be
> >>> initialized such that entry 0 will be the lowest voltage we will ever
> >>> need. In case of PWM, the lowest voltage is determined by the regulator
> >>> hardware so we need this software limit. Note that this is different
> >>> from lut_min which gives the lowest voltage we allow taking temperature
> >>> into account. In a future patchset we will update lut_vmin dynamically.
> >>> Similarly lut_max will be the highest voltage allowed taking temperature
> >>> into accouint. Also this will be updated dynamically once temperature
> >>> dependence will be introduced.
> >>>
> >>> Signed-off-by: Peter De Schrijver 
> >>> ---
> >>>  drivers/clk/tegra/clk-dfll.c | 62 
> >>> ++--
> >>>  1 file changed, 37 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> >>> index 0a7deee..fa97763 100644
> >>> --- a/drivers/clk/tegra/clk-dfll.c
> >>> +++ b/drivers/clk/tegra/clk-dfll.c
> >>> @@ -301,9 +301,10 @@ struct tegra_dfll {
> >>>   u32 i2c_slave_addr;
> >>>  
> >>>   /* i2c_lut array entries are regulator framework selectors */
> >>> - unsignedi2c_lut[MAX_DFLL_VOLTAGES];
> >>> - int i2c_lut_size;
> >>> - u8  lut_min, lut_max, lut_safe;
> >>> + unsigned inti2c_lut[MAX_DFLL_VOLTAGES];
> >>> + unsigned intlut_uv[MAX_DFLL_VOLTAGES];
> >>> + int lut_size;
> >>> + u8  lut_bottom, lut_min, lut_max, lut_safe;
> >>>  };
> >>>  
> >>>  #define clk_hw_to_dfll(_hw) container_of(_hw, struct tegra_dfll, 
> >>> dfll_clk_hw)
> >>> @@ -531,10 +532,10 @@ static void dfll_load_i2c_lut(struct tegra_dfll *td)
> >>>   u32 val;
> >>>  
> >>>   for (i = 0; i < MAX_DFLL_VOLTAGES; i++) {
> >>> - if (i < td->lut_min)
> >>> - lut_index = td->lut_min;
> >>> - else if (i > td->lut_max)
> >>> - lut_index = td->lut_max;
> >>> + if (i < td->lut_bottom)
> >>> + lut_index = td->lut_bottom;
> >>> + else if (i > td->lut_size - 1)
> >>> + lut_index = td->lut_size - 1;
> >>>   else
> >>>   lut_index = i;
> >>>  
> >>> @@ -594,9 +595,9 @@ static void dfll_init_out_if(struct tegra_dfll *td)
> >>>  {
> >>>   u32 val;
> >>>  
> >>> - td->lut_min = 0;
> >>> - td->lut_max = td->i2c_lut_size - 1;
> >>> - td->lut_safe = td->lut_min + 1;
> >>> + td->lut_min = td->lut_bottom;
> >>> + td->lut_max = td->lut_size - 1;
> >>> + td->lut_safe = td->lut_min + (td->lut_min < td->lut_max ? 1 : 0);
> >>>  
> >>>   dfll_i2c_writel(td, 0, DFLL_OUTPUT_CFG);
> >>>   val = (td->lut_safe << DFLL_OUTPUT_CFG_SAFE_SHIFT) |
> >>> @@ -619,11 +620,11 @@ static void dfll_init_out_if(struct tegra_dfll *td)
> >>>   */
> >>>  
> >>>  /**
> >>> - * find_lut_index_for_rate - determine I2C LUT index for given DFLL rate
> >>> + * find_lut_index_for_rate - determine LUT index for given DFLL rate
> >>>   * @td: DFLL instance
> >>>   * @rate: clock rate
> >>>   *
> >>> - *

Re: [PATCH v3 05/11] clk: tegra: prepare dfll driver for PWM regulator

[Peter De Schrijver]

On Thu, Mar 08, 2018 at 10:50:06PM +, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > This patch prepares the dfll driver to work with PWM regulators.
> > To do this we introduce a new array lut_uv which gives the voltage for
> > a given index generated by the dfll logic. This index will then be
> > translated to a PMIC voltage ID in case of I2C using the i2c_lut. In case
> > of a PWM regulator, it will be used to determine the PWM duty cycle.
> > We also introduce lut_bottom which holds the lowest voltage we will ever
> > need. In case of I2C this can be set to zero because the i2c_lut will be
> > initialized such that entry 0 will be the lowest voltage we will ever
> > need. In case of PWM, the lowest voltage is determined by the regulator
> > hardware so we need this software limit. Note that this is different
> > from lut_min which gives the lowest voltage we allow taking temperature
> > into account. In a future patchset we will update lut_vmin dynamically.
> > Similarly lut_max will be the highest voltage allowed taking temperature
> > into accouint. Also this will be updated dynamically once temperature
> > dependence will be introduced.
> > 
> > Signed-off-by: Peter De Schrijver <pdeschrij...@nvidia.com>
> > ---
> >  drivers/clk/tegra/clk-dfll.c | 62 
> > ++--
> >  1 file changed, 37 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> > index 0a7deee..fa97763 100644
> > --- a/drivers/clk/tegra/clk-dfll.c
> > +++ b/drivers/clk/tegra/clk-dfll.c
> > @@ -301,9 +301,10 @@ struct tegra_dfll {
> > u32 i2c_slave_addr;
> >  
> > /* i2c_lut array entries are regulator framework selectors */
> > -   unsignedi2c_lut[MAX_DFLL_VOLTAGES];
> > -   int i2c_lut_size;
> > -   u8  lut_min, lut_max, lut_safe;
> > +   unsigned inti2c_lut[MAX_DFLL_VOLTAGES];
> > +   unsigned intlut_uv[MAX_DFLL_VOLTAGES];
> > +   int lut_size;
> > +   u8  lut_bottom, lut_min, lut_max, lut_safe;
> >  };
> >  
> >  #define clk_hw_to_dfll(_hw) container_of(_hw, struct tegra_dfll, 
> > dfll_clk_hw)
> > @@ -531,10 +532,10 @@ static void dfll_load_i2c_lut(struct tegra_dfll *td)
> > u32 val;
> >  
> > for (i = 0; i < MAX_DFLL_VOLTAGES; i++) {
> > -   if (i < td->lut_min)
> > -   lut_index = td->lut_min;
> > -   else if (i > td->lut_max)
> > -   lut_index = td->lut_max;
> > +   if (i < td->lut_bottom)
> > +   lut_index = td->lut_bottom;
> > +   else if (i > td->lut_size - 1)
> > +   lut_index = td->lut_size - 1;
> > else
> > lut_index = i;
> >  
> > @@ -594,9 +595,9 @@ static void dfll_init_out_if(struct tegra_dfll *td)
> >  {
> > u32 val;
> >  
> > -   td->lut_min = 0;
> > -   td->lut_max = td->i2c_lut_size - 1;
> > -   td->lut_safe = td->lut_min + 1;
> > +   td->lut_min = td->lut_bottom;
> > +   td->lut_max = td->lut_size - 1;
> > +   td->lut_safe = td->lut_min + (td->lut_min < td->lut_max ? 1 : 0);
> >  
> > dfll_i2c_writel(td, 0, DFLL_OUTPUT_CFG);
> > val = (td->lut_safe << DFLL_OUTPUT_CFG_SAFE_SHIFT) |
> > @@ -619,11 +620,11 @@ static void dfll_init_out_if(struct tegra_dfll *td)
> >   */
> >  
> >  /**
> > - * find_lut_index_for_rate - determine I2C LUT index for given DFLL rate
> > + * find_lut_index_for_rate - determine LUT index for given DFLL rate
> >   * @td: DFLL instance
> >   * @rate: clock rate
> >   *
> > - * Determines the index of a I2C LUT entry for a voltage that approximately
> > + * Determines the index of a LUT entry for a voltage that approximately
> >   * produces the given DFLL clock rate. This is used when forcing a value
> >   * to the integrator during rate changes. Returns -ENOENT if a suitable
> >   * LUT index is not found.
> > @@ -637,11 +638,11 @@ static int find_lut_index_for_rate(struct tegra_dfll 
> > *td, unsigned long rate)
> > if (IS_ERR(opp))
> > return PTR_ERR(opp);
> >  
> > -   uv = dev_pm_opp_get_voltage(opp);
> > +   uv = dev_pm_opp_get_voltage(opp) / td->soc-&

Re: [PATCH v3 05/11] clk: tegra: prepare dfll driver for PWM regulator

[Peter De Schrijver]

On Thu, Mar 08, 2018 at 10:50:06PM +, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > This patch prepares the dfll driver to work with PWM regulators.
> > To do this we introduce a new array lut_uv which gives the voltage for
> > a given index generated by the dfll logic. This index will then be
> > translated to a PMIC voltage ID in case of I2C using the i2c_lut. In case
> > of a PWM regulator, it will be used to determine the PWM duty cycle.
> > We also introduce lut_bottom which holds the lowest voltage we will ever
> > need. In case of I2C this can be set to zero because the i2c_lut will be
> > initialized such that entry 0 will be the lowest voltage we will ever
> > need. In case of PWM, the lowest voltage is determined by the regulator
> > hardware so we need this software limit. Note that this is different
> > from lut_min which gives the lowest voltage we allow taking temperature
> > into account. In a future patchset we will update lut_vmin dynamically.
> > Similarly lut_max will be the highest voltage allowed taking temperature
> > into accouint. Also this will be updated dynamically once temperature
> > dependence will be introduced.
> > 
> > Signed-off-by: Peter De Schrijver 
> > ---
> >  drivers/clk/tegra/clk-dfll.c | 62 
> > ++--
> >  1 file changed, 37 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> > index 0a7deee..fa97763 100644
> > --- a/drivers/clk/tegra/clk-dfll.c
> > +++ b/drivers/clk/tegra/clk-dfll.c
> > @@ -301,9 +301,10 @@ struct tegra_dfll {
> > u32 i2c_slave_addr;
> >  
> > /* i2c_lut array entries are regulator framework selectors */
> > -   unsignedi2c_lut[MAX_DFLL_VOLTAGES];
> > -   int i2c_lut_size;
> > -   u8  lut_min, lut_max, lut_safe;
> > +   unsigned inti2c_lut[MAX_DFLL_VOLTAGES];
> > +   unsigned intlut_uv[MAX_DFLL_VOLTAGES];
> > +   int lut_size;
> > +   u8  lut_bottom, lut_min, lut_max, lut_safe;
> >  };
> >  
> >  #define clk_hw_to_dfll(_hw) container_of(_hw, struct tegra_dfll, 
> > dfll_clk_hw)
> > @@ -531,10 +532,10 @@ static void dfll_load_i2c_lut(struct tegra_dfll *td)
> > u32 val;
> >  
> > for (i = 0; i < MAX_DFLL_VOLTAGES; i++) {
> > -   if (i < td->lut_min)
> > -   lut_index = td->lut_min;
> > -   else if (i > td->lut_max)
> > -   lut_index = td->lut_max;
> > +   if (i < td->lut_bottom)
> > +   lut_index = td->lut_bottom;
> > +   else if (i > td->lut_size - 1)
> > +   lut_index = td->lut_size - 1;
> > else
> > lut_index = i;
> >  
> > @@ -594,9 +595,9 @@ static void dfll_init_out_if(struct tegra_dfll *td)
> >  {
> > u32 val;
> >  
> > -   td->lut_min = 0;
> > -   td->lut_max = td->i2c_lut_size - 1;
> > -   td->lut_safe = td->lut_min + 1;
> > +   td->lut_min = td->lut_bottom;
> > +   td->lut_max = td->lut_size - 1;
> > +   td->lut_safe = td->lut_min + (td->lut_min < td->lut_max ? 1 : 0);
> >  
> > dfll_i2c_writel(td, 0, DFLL_OUTPUT_CFG);
> > val = (td->lut_safe << DFLL_OUTPUT_CFG_SAFE_SHIFT) |
> > @@ -619,11 +620,11 @@ static void dfll_init_out_if(struct tegra_dfll *td)
> >   */
> >  
> >  /**
> > - * find_lut_index_for_rate - determine I2C LUT index for given DFLL rate
> > + * find_lut_index_for_rate - determine LUT index for given DFLL rate
> >   * @td: DFLL instance
> >   * @rate: clock rate
> >   *
> > - * Determines the index of a I2C LUT entry for a voltage that approximately
> > + * Determines the index of a LUT entry for a voltage that approximately
> >   * produces the given DFLL clock rate. This is used when forcing a value
> >   * to the integrator during rate changes. Returns -ENOENT if a suitable
> >   * LUT index is not found.
> > @@ -637,11 +638,11 @@ static int find_lut_index_for_rate(struct tegra_dfll 
> > *td, unsigned long rate)
> > if (IS_ERR(opp))
> > return PTR_ERR(opp);
> >  
> > -   uv = dev_pm_opp_get_voltage(opp);
> > +   uv = dev_pm_opp_get_voltage(opp) / td->soc->alignment.step_uv;
>

Re: [PATCH v3 07/11] dt-bindings: tegra: Update DFLL binding for PWM regulator

[Peter De Schrijver]

On Thu, Mar 08, 2018 at 11:21:40PM +, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > Add new properties to configure the DFLL PWM regulator support. Also
> > add an example and make the I2C clock only required when I2C support is
> > used.
> > 
> > Signed-off-by: Peter De Schrijver <pdeschrij...@nvidia.com>
> > ---
> >  .../bindings/clock/nvidia,tegra124-dfll.txt| 76 
> > +-
> >  1 file changed, 74 insertions(+), 2 deletions(-)
> > 
> > diff --git 
> > a/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt 
> > b/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
> > index dff236f..a4903f7 100644
> > --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
> > +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
> > @@ -23,7 +23,8 @@ Required properties:
> >  - clock-names: Must include the following entries:
> >- soc: Clock source for the DFLL control logic.
> >- ref: The closed loop reference clock
> > -  - i2c: Clock source for the integrated I2C master.
> > +  - i2c: Clock source for the integrated I2C master (only required when
> > +using I2C mode).
> >  - resets: Must contain an entry for each entry in reset-names.
> >See ../reset/reset.txt for details.
> >  - reset-names: Must include the following entries:
> > @@ -45,10 +46,28 @@ Required properties for the control loop parameters:
> >  Optional properties for the control loop parameters:
> >  - nvidia,cg-scale: Boolean value, see the field DFLL_PARAMS_CG_SCALE in 
> > the TRM.
> >  
> > +Optional properties for mode selection:
> > +- nvidia,pwm-to-pmic: Use PWM to control regulator rather then I2C.
> > +
> 
> Do we need this property? Seems that we should be able to detect if it
> is I2C or PWM based upon the other properties.
> 

I guess we could look for nvidia,pwm-period and switch to PWM mode if this is
found, but I think it's more clear to have explicit property.

Peter.

Re: [PATCH v3 07/11] dt-bindings: tegra: Update DFLL binding for PWM regulator

[Peter De Schrijver]

On Thu, Mar 08, 2018 at 11:21:40PM +, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > Add new properties to configure the DFLL PWM regulator support. Also
> > add an example and make the I2C clock only required when I2C support is
> > used.
> > 
> > Signed-off-by: Peter De Schrijver 
> > ---
> >  .../bindings/clock/nvidia,tegra124-dfll.txt| 76 
> > +-
> >  1 file changed, 74 insertions(+), 2 deletions(-)
> > 
> > diff --git 
> > a/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt 
> > b/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
> > index dff236f..a4903f7 100644
> > --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
> > +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
> > @@ -23,7 +23,8 @@ Required properties:
> >  - clock-names: Must include the following entries:
> >- soc: Clock source for the DFLL control logic.
> >- ref: The closed loop reference clock
> > -  - i2c: Clock source for the integrated I2C master.
> > +  - i2c: Clock source for the integrated I2C master (only required when
> > +using I2C mode).
> >  - resets: Must contain an entry for each entry in reset-names.
> >See ../reset/reset.txt for details.
> >  - reset-names: Must include the following entries:
> > @@ -45,10 +46,28 @@ Required properties for the control loop parameters:
> >  Optional properties for the control loop parameters:
> >  - nvidia,cg-scale: Boolean value, see the field DFLL_PARAMS_CG_SCALE in 
> > the TRM.
> >  
> > +Optional properties for mode selection:
> > +- nvidia,pwm-to-pmic: Use PWM to control regulator rather then I2C.
> > +
> 
> Do we need this property? Seems that we should be able to detect if it
> is I2C or PWM based upon the other properties.
> 

I guess we could look for nvidia,pwm-period and switch to PWM mode if this is
found, but I think it's more clear to have explicit property.

Peter.

Re: [PATCH v3 09/11] cpufreq: tegra124-cpufreq: extend to support Tegra210

[Peter De Schrijver]

On Thu, Mar 08, 2018 at 11:25:04PM +, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> > support in this driver. Also allow for the case where the CPU voltage is
> > controlled directly by the DFLL rather than by a separate regulator object.
> > 
> > Signed-off-by: Peter De Schrijver <pdeschrij...@nvidia.com>
> > ---
> >  drivers/cpufreq/tegra124-cpufreq.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/tegra124-cpufreq.c 
> > b/drivers/cpufreq/tegra124-cpufreq.c
> > index 4353025..f8e01a8 100644
> > --- a/drivers/cpufreq/tegra124-cpufreq.c
> > +++ b/drivers/cpufreq/tegra124-cpufreq.c
> > @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct 
> > tegra124_cpufreq_priv *priv)
> >  {
> > clk_set_parent(priv->cpu_clk, priv->pllp_clk);
> > clk_disable_unprepare(priv->dfll_clk);
> > -   regulator_sync_voltage(priv->vdd_cpu_reg);
> > +   if (priv->vdd_cpu_reg)
> > +   regulator_sync_voltage(priv->vdd_cpu_reg);
> > clk_set_parent(priv->cpu_clk, priv->pllx_clk);
> >  }
> >  
> > @@ -89,10 +90,10 @@ static int tegra124_cpufreq_probe(struct 
> > platform_device *pdev)
> > return -ENODEV;
> >  
> > priv->vdd_cpu_reg = regulator_get(cpu_dev, "vdd-cpu");
> > -   if (IS_ERR(priv->vdd_cpu_reg)) {
> > -   ret = PTR_ERR(priv->vdd_cpu_reg);
> > -   goto out_put_np;
> > -   }
> > +   if (IS_ERR(priv->vdd_cpu_reg) != -EPROBE_DEFER)
> > +   priv->vdd_cpu_reg = NULL;
> > +   else
> > +   return -EPROBE_DEFER;
> 
> I am still not sure that we should rely on the fact that the regulator
> is not present in DT to imply that we do not need it. I think that we
> should be checking if we are using I2C mode here.
> 

The cpufreq driver doesn't know this however. Also the current approach of
setting the same voltage when switching to pll_x is incorrect. The CVB
tables when using pll_x include more margin than when using the DFLL.

Peter.

Re: [PATCH v3 09/11] cpufreq: tegra124-cpufreq: extend to support Tegra210

[Peter De Schrijver]

On Thu, Mar 08, 2018 at 11:25:04PM +, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > Tegra210 has a very similar CPU clocking scheme than Tegra124. So add
> > support in this driver. Also allow for the case where the CPU voltage is
> > controlled directly by the DFLL rather than by a separate regulator object.
> > 
> > Signed-off-by: Peter De Schrijver 
> > ---
> >  drivers/cpufreq/tegra124-cpufreq.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/tegra124-cpufreq.c 
> > b/drivers/cpufreq/tegra124-cpufreq.c
> > index 4353025..f8e01a8 100644
> > --- a/drivers/cpufreq/tegra124-cpufreq.c
> > +++ b/drivers/cpufreq/tegra124-cpufreq.c
> > @@ -64,7 +64,8 @@ static void tegra124_cpu_switch_to_pllx(struct 
> > tegra124_cpufreq_priv *priv)
> >  {
> > clk_set_parent(priv->cpu_clk, priv->pllp_clk);
> > clk_disable_unprepare(priv->dfll_clk);
> > -   regulator_sync_voltage(priv->vdd_cpu_reg);
> > +   if (priv->vdd_cpu_reg)
> > +   regulator_sync_voltage(priv->vdd_cpu_reg);
> > clk_set_parent(priv->cpu_clk, priv->pllx_clk);
> >  }
> >  
> > @@ -89,10 +90,10 @@ static int tegra124_cpufreq_probe(struct 
> > platform_device *pdev)
> > return -ENODEV;
> >  
> > priv->vdd_cpu_reg = regulator_get(cpu_dev, "vdd-cpu");
> > -   if (IS_ERR(priv->vdd_cpu_reg)) {
> > -   ret = PTR_ERR(priv->vdd_cpu_reg);
> > -   goto out_put_np;
> > -   }
> > +   if (IS_ERR(priv->vdd_cpu_reg) != -EPROBE_DEFER)
> > +   priv->vdd_cpu_reg = NULL;
> > +   else
> > +   return -EPROBE_DEFER;
> 
> I am still not sure that we should rely on the fact that the regulator
> is not present in DT to imply that we do not need it. I think that we
> should be checking if we are using I2C mode here.
> 

The cpufreq driver doesn't know this however. Also the current approach of
setting the same voltage when switching to pll_x is incorrect. The CVB
tables when using pll_x include more margin than when using the DFLL.

Peter.

Re: [PATCH v3 06/11] clk: tegra: dfll: support PWM regulator control

[Peter De Schrijver]

On Thu, Mar 08, 2018 at 11:15:17PM +, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > The DFLL can directly generate a PWM signal to control the regulator IC
> > rather then sending i2c messages. This patch adds support for this mode.
> > In this mode the hardware LUT is not used and also there is no regulator
> > object involved because there is no way to control the regulator voltage
> > without also changing the DFLL output frequency. Also the register debugfs
> > file is slightly reworked to only show the i2c registers when i2c mode is
> > in use. As there is no regulator object for the PWM regulator, its step and
> > offset values are retrieved from DT instead.
> 
> It is unclear to me why we bother creating the LUT for PWM if it is not
> used? Is this for debug to get an approximation? Why do we do this?
> 

lut_uv certainly is used. This makes it easier to abstract PWM vs i2c. It
would also be possible to replace every reference to lut_uv with a function
which calculates the corresponding voltage by either querying the regulator
framework in case of i2c or doing alignment.offset_uv + x * alignment.step_uv
in case of PWM. Doing this once seems a bit easier to me.

> > Signed-off-by: Peter De Schrijver <pdeschrij...@nvidia.com>
> > ---
> >  drivers/clk/tegra/clk-dfll.c   | 398 
> > ++---
> >  drivers/clk/tegra/clk-tegra124-dfll-fcpu.c |  23 +-
> >  2 files changed, 382 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> > index fa97763..228edb4 100644
> > --- a/drivers/clk/tegra/clk-dfll.c
> > +++ b/drivers/clk/tegra/clk-dfll.c
> > @@ -243,6 +243,12 @@ enum dfll_tune_range {
> > DFLL_TUNE_LOW = 1,
> >  };
> >  
> > +
> > +enum tegra_dfll_pmu_if {
> > +   TEGRA_DFLL_PMU_I2C = 0,
> > +   TEGRA_DFLL_PMU_PWM = 1,
> > +};
> > +
> >  /**
> >   * struct dfll_rate_req - target DFLL rate request data
> >   * @rate: target frequency, after the postscaling
> > @@ -294,17 +300,25 @@ struct tegra_dfll {
> > u32 ci;
> > u32 cg;
> > boolcg_scale;
> > +   u32 reg_init_uV;
> >  
> > /* I2C interface parameters */
> > u32 i2c_fs_rate;
> > u32 i2c_reg;
> > u32 i2c_slave_addr;
> >  
> > -   /* i2c_lut array entries are regulator framework selectors */
> > +   /* lut array entries are regulator framework selectors or PWM values*/
> > unsigned inti2c_lut[MAX_DFLL_VOLTAGES];
> > unsigned intlut_uv[MAX_DFLL_VOLTAGES];
> > int lut_size;
> > u8  lut_bottom, lut_min, lut_max, lut_safe;
> > +
> > +   /* PWM interface */
> > +   enum tegra_dfll_pmu_if  pmu_if;
> > +   unsigned long   pwm_rate;
> > +   struct pinctrl  *pwm_pin;
> > +   struct pinctrl_state*pwm_enable_state;
> > +   struct pinctrl_state*pwm_disable_state;
> >  };
> >  
> >  #define clk_hw_to_dfll(_hw) container_of(_hw, struct tegra_dfll, 
> > dfll_clk_hw)
> > @@ -491,6 +505,36 @@ static void dfll_set_mode(struct tegra_dfll *td,
> >  }
> >  
> >  /*
> > + * DVCO rate control
> > + */
> > +
> > +static unsigned long get_dvco_rate_below(struct tegra_dfll *td, u8 out_min)
> > +{
> > +   struct dev_pm_opp *opp;
> > +   unsigned long rate, prev_rate;
> > +   int uv, min_uv;
> > +
> > +   min_uv = td->lut_uv[out_min];
> > +   for (rate = 0, prev_rate = 0; ; rate++) {
> > +   rcu_read_lock();
> > +   opp = dev_pm_opp_find_freq_ceil(td->soc->dev, );
> > +   if (IS_ERR(opp)) {
> > +   rcu_read_unlock();
> > +   break;
> > +   }
> > +   uv = dev_pm_opp_get_voltage(opp);
> > +   rcu_read_unlock();
> > +
> > +   if (uv && uv > min_uv)
> > +   return prev_rate;
> > +
> > +   prev_rate = rate;
> > +   }
> > +
> > +   return prev_rate;
> > +}
> > +
> > +/*
> >   * DFLL-to-I2C controller interface
> >   */
> >  
> > @@ -519,6 +563,119 @@ static int dfll_i2c_set_output_enabled(struct 
> > tegra_dfll *

Re: [PATCH v3 06/11] clk: tegra: dfll: support PWM regulator control

[Peter De Schrijver]

On Thu, Mar 08, 2018 at 11:15:17PM +, Jon Hunter wrote:
> 
> On 06/02/18 16:34, Peter De Schrijver wrote:
> > The DFLL can directly generate a PWM signal to control the regulator IC
> > rather then sending i2c messages. This patch adds support for this mode.
> > In this mode the hardware LUT is not used and also there is no regulator
> > object involved because there is no way to control the regulator voltage
> > without also changing the DFLL output frequency. Also the register debugfs
> > file is slightly reworked to only show the i2c registers when i2c mode is
> > in use. As there is no regulator object for the PWM regulator, its step and
> > offset values are retrieved from DT instead.
> 
> It is unclear to me why we bother creating the LUT for PWM if it is not
> used? Is this for debug to get an approximation? Why do we do this?
> 

lut_uv certainly is used. This makes it easier to abstract PWM vs i2c. It
would also be possible to replace every reference to lut_uv with a function
which calculates the corresponding voltage by either querying the regulator
framework in case of i2c or doing alignment.offset_uv + x * alignment.step_uv
in case of PWM. Doing this once seems a bit easier to me.

> > Signed-off-by: Peter De Schrijver 
> > ---
> >  drivers/clk/tegra/clk-dfll.c   | 398 
> > ++---
> >  drivers/clk/tegra/clk-tegra124-dfll-fcpu.c |  23 +-
> >  2 files changed, 382 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/clk/tegra/clk-dfll.c b/drivers/clk/tegra/clk-dfll.c
> > index fa97763..228edb4 100644
> > --- a/drivers/clk/tegra/clk-dfll.c
> > +++ b/drivers/clk/tegra/clk-dfll.c
> > @@ -243,6 +243,12 @@ enum dfll_tune_range {
> > DFLL_TUNE_LOW = 1,
> >  };
> >  
> > +
> > +enum tegra_dfll_pmu_if {
> > +   TEGRA_DFLL_PMU_I2C = 0,
> > +   TEGRA_DFLL_PMU_PWM = 1,
> > +};
> > +
> >  /**
> >   * struct dfll_rate_req - target DFLL rate request data
> >   * @rate: target frequency, after the postscaling
> > @@ -294,17 +300,25 @@ struct tegra_dfll {
> > u32 ci;
> > u32 cg;
> > boolcg_scale;
> > +   u32 reg_init_uV;
> >  
> > /* I2C interface parameters */
> > u32 i2c_fs_rate;
> > u32 i2c_reg;
> > u32 i2c_slave_addr;
> >  
> > -   /* i2c_lut array entries are regulator framework selectors */
> > +   /* lut array entries are regulator framework selectors or PWM values*/
> > unsigned inti2c_lut[MAX_DFLL_VOLTAGES];
> > unsigned intlut_uv[MAX_DFLL_VOLTAGES];
> > int lut_size;
> > u8  lut_bottom, lut_min, lut_max, lut_safe;
> > +
> > +   /* PWM interface */
> > +   enum tegra_dfll_pmu_if  pmu_if;
> > +   unsigned long   pwm_rate;
> > +   struct pinctrl  *pwm_pin;
> > +   struct pinctrl_state*pwm_enable_state;
> > +   struct pinctrl_state*pwm_disable_state;
> >  };
> >  
> >  #define clk_hw_to_dfll(_hw) container_of(_hw, struct tegra_dfll, 
> > dfll_clk_hw)
> > @@ -491,6 +505,36 @@ static void dfll_set_mode(struct tegra_dfll *td,
> >  }
> >  
> >  /*
> > + * DVCO rate control
> > + */
> > +
> > +static unsigned long get_dvco_rate_below(struct tegra_dfll *td, u8 out_min)
> > +{
> > +   struct dev_pm_opp *opp;
> > +   unsigned long rate, prev_rate;
> > +   int uv, min_uv;
> > +
> > +   min_uv = td->lut_uv[out_min];
> > +   for (rate = 0, prev_rate = 0; ; rate++) {
> > +   rcu_read_lock();
> > +   opp = dev_pm_opp_find_freq_ceil(td->soc->dev, );
> > +   if (IS_ERR(opp)) {
> > +   rcu_read_unlock();
> > +   break;
> > +   }
> > +   uv = dev_pm_opp_get_voltage(opp);
> > +   rcu_read_unlock();
> > +
> > +   if (uv && uv > min_uv)
> > +   return prev_rate;
> > +
> > +   prev_rate = rate;
> > +   }
> > +
> > +   return prev_rate;
> > +}
> > +
> > +/*
> >   * DFLL-to-I2C controller interface
> >   */
> >  
> > @@ -519,6 +563,119 @@ static int dfll_i2c_set_output_enabled(struct 
> > tegra_dfll *td, bool enable)
> > return 0;
> >  }
&