Re: [linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Ondřej Jirman]

On 26.7.2016 08:32, Maxime Ripard wrote:
> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
> If so, then yes, trying to switch to the 24MHz oscillator before
> applying the factors, and then switching back when the PLL is stable
> would be a nice solution.
>
> I just checked, and all the SoCs we've had so far have that
> possibility, so if it works, for now, I'd like to stick to that.

 It would need to be tested. U-boot does the change only once, while the
 kernel would be doing it all the time and between various frequencies
 and PLL settings. So the issues may show up with this solution too.
>>>
>>> That would have the benefit of being quite easy to document, not be a
>>> huge amount of code and it would work on all the CPUs PLLs we have so
>>> far, so still, a pretty big win. If it doesn't, of course, we don't
>>> really have the choice.
>>
>> It's probably more code though. It has to access different register from
>> the one that is already defined in dts, which would add a lot of code
>> and require dts changes. The original patch I sent is simpler than that.
> 
> Why?

Because I don't understand internals of clk subsystem that much. :) So
my guess might be wrong.

Wens send patches implementing clock source switching in the new CCU
code, so hopefully it will work. Ultimately it's a hack. Some internal
parts of the soc may still get into out of the bounds operating state
even if they are gated off from other parts of the SoC, via clock
multiplexing, by improper factors change procedure.

So it doesn't really matter which is more concise in code. As long as
one solution is wrong and other is proper.

Anyway, I'll try with wens's patches, later and see if I trigger some
instability or not.

> You can use container_of to retrieve the parent structure of the clock
> notifier, and then you get a ccu_common structure pointer, with the
> CCU base address, the clock register, its lock, etc.
> 
> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
> 
> I don't really get why anything should be changed in the DT, or why it
> would add a lot of code. Or maybe we're not talking about the same
> thing?
> 
> Maxime
> 

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Maxime Ripard]

On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
> >>> If so, then yes, trying to switch to the 24MHz oscillator before
> >>> applying the factors, and then switching back when the PLL is stable
> >>> would be a nice solution.
> >>>
> >>> I just checked, and all the SoCs we've had so far have that
> >>> possibility, so if it works, for now, I'd like to stick to that.
> >>
> >> It would need to be tested. U-boot does the change only once, while the
> >> kernel would be doing it all the time and between various frequencies
> >> and PLL settings. So the issues may show up with this solution too.
> > 
> > That would have the benefit of being quite easy to document, not be a
> > huge amount of code and it would work on all the CPUs PLLs we have so
> > far, so still, a pretty big win. If it doesn't, of course, we don't
> > really have the choice.
> 
> It's probably more code though. It has to access different register from
> the one that is already defined in dts, which would add a lot of code
> and require dts changes. The original patch I sent is simpler than that.

Why?

You can use container_of to retrieve the parent structure of the clock
notifier, and then you get a ccu_common structure pointer, with the
CCU base address, the clock register, its lock, etc.

Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.

I don't really get why anything should be changed in the DT, or why it
would add a lot of code. Or maybe we're not talking about the same
thing?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Ondřej Jirman]

On 21.7.2016 11:48, Maxime Ripard wrote:
> On Fri, Jul 15, 2016 at 12:38:54PM +0200, Ondřej Jirman wrote:
>> On 15.7.2016 10:53, Maxime Ripard wrote:
>>> On Fri, Jul 01, 2016 at 02:50:57AM +0200, Ondřej Jirman wrote:
>>  /**
>> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
>> + * register using an algorithm that tries to reserve the PLL lock
>> + */
>> +
>> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, 
>> struct factors_request *req)
>> +{
>> +const struct clk_factors_config *config = factors->config;
>> +u32 reg;
>> +
>> +/* Fetch the register value */
>> +reg = readl(factors->reg);
>> +
>> +if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
>> +reg = FACTOR_SET(config->pshift, config->pwidth, reg, 
>> req->p);
>> +
>> +writel(reg, factors->reg);
>> +__delay(2000);
>> +}
>
> So there was some doubts about the fact that P was being used, or at
> least that it was useful.

 p is necessary to reduce frequencies below 288 MHz according to the
 datasheet.
>>>
>>> Yes, but you could reach those frequencies without P, too, and it's
>>> not part of any OPP provided by Allwinner.
>>
>> The arisc firmware for H3 contains table of factors for frequences from
>> 0 to 2GHz and, P is used below 240MHz. M is never used, BTW. (other
>> datasheets specify M as for testing use only, whatever that means - not
>> H3, but it seems it's the same PLL block)
> 
> Interesting. Which SoCs in particular?
> 
>> +if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
>> +reg = FACTOR_SET(config->mshift, config->mwidth, reg, 
>> req->m);
>> +
>> +writel(reg, factors->reg);
>> +__delay(2000);
>> +}
>> +
>> +reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
>> +reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
>> +
>> +writel(reg, factors->reg);
>> +__delay(20);
>> +
>> +while (!(readl(factors->reg) & (1 << config->lock)));
>
> So, they are applying the dividers first, and then applying the
> multipliers, and then wait for the PLL to stabilize.

 Not exactly, first we are increasing dividers if the new dividers are
 higher that that what's already set. This ensures that because
 application of dividers is immediate by the design of the PLL, the
 application of multipliers isn't. So the VCO would still run at the same
 frequency for a while gradually rising to a new value for example,
 while the dividers would be reduced immediately. Leading to crash.

 PLL
 --
 PRE DIV(f0) -> VCO(f1) -> POST DIV(f2)
P K,N   M

 Example: (we set all factors at once, reducing dividers and multipliers
 at the same time at 0ms - this should lead to no change in the output
 frequency, but...)

 -1ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz
  0ms: f0 = 24MHz, f1 = 2GHz,   f2 = 2GHz   - boom
  1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 1.5GHz
  2ms: f0 = 24MHz, f1 = 1GHz,   f2 = 1GHz

 The current code crashes exactly at boom, you don't get any more
 instructions to execute.

 See.

 So this patch first increases dividers (only if necessary), changes
 multipliers and waits for change to happen (takes around 2000 cycles),
 and then decreases dividers (only if necessary).

 So we get:

 -1ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz
  0ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz   - no boom, multiplier
  reduced
  1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 0.75GHz
 1.9ms: f0 = 24MHz, f1 = 1GHz,   f2 = 0.5GHz - we got PLL sync
  2ms: f0 = 24MHz, f1 = 1GHz,   f2 = 1GHz   - and here we reduce divider
 at last
>>>
>>> Awesome explanation, thanks!
>>>
>>> So I guess it really all boils down to the fact that the CPU is
>>> clocked way outside of it's operating frequency while the PLL
>>> stabilizes, right?
>>
>> It may be, depending on the factors before and after change. I haven't
>> tested what factors the mainline kernel calculates for each frequency.
>> The arisc never uses M, and only uses P at frequencies that would not
>> allow for this behavior.
>>
>> I created a test program for arisc that runs a loop on the main CPU
>> using msgbox to send pings to the arisc CPU, and the vary the PLL1
>> randomly from the arisc, and haven't been able to lockup the main CPU
>> yet with either method.
>>
>> There's also AXI clock, that depends on PLL1. Arisc firmware uses the
>> same method to change it while changing CPUX clock. If the clock would
>> rise above certain frequency, AXI 

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Maxime Ripard]

On Fri, Jul 15, 2016 at 03:27:56PM +0200, Jean-Francois Moine wrote:
> On Fri, 15 Jul 2016 12:38:54 +0200
> Ondřej Jirman  wrote:
> 
> > > If so, then yes, trying to switch to the 24MHz oscillator before
> > > applying the factors, and then switching back when the PLL is stable
> > > would be a nice solution.
> > > 
> > > I just checked, and all the SoCs we've had so far have that
> > > possibility, so if it works, for now, I'd like to stick to that.
> > 
> > It would need to be tested. U-boot does the change only once, while the
> > kernel would be doing it all the time and between various frequencies
> > and PLL settings. So the issues may show up with this solution too.
> 
> I don't think this is a good idea: the CPU clock may be changed at any
> time with the CPUFreq governor. I don't see the system moving from
> 1008MHz to 24MHz and then to 1200MHz when some computation is needed!

It wouldn't happen that often. The sampling rate for the governor is
1000 times the latency, so, at most, 0.1% of the time would be spent
at 24MHz.

And if you're really concerned about performances, you won't enable
cpufreq anyway.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Maxime Ripard]

On Fri, Jul 15, 2016 at 12:38:54PM +0200, Ondřej Jirman wrote:
> On 15.7.2016 10:53, Maxime Ripard wrote:
> > On Fri, Jul 01, 2016 at 02:50:57AM +0200, Ondřej Jirman wrote:
>   /**
>  + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
>  + * register using an algorithm that tries to reserve the PLL lock
>  + */
>  +
>  +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, 
>  struct factors_request *req)
>  +{
>  +const struct clk_factors_config *config = factors->config;
>  +u32 reg;
>  +
>  +/* Fetch the register value */
>  +reg = readl(factors->reg);
>  +
>  +if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
>  +reg = FACTOR_SET(config->pshift, config->pwidth, reg, 
>  req->p);
>  +
>  +writel(reg, factors->reg);
>  +__delay(2000);
>  +}
> >>>
> >>> So there was some doubts about the fact that P was being used, or at
> >>> least that it was useful.
> >>
> >> p is necessary to reduce frequencies below 288 MHz according to the
> >> datasheet.
> > 
> > Yes, but you could reach those frequencies without P, too, and it's
> > not part of any OPP provided by Allwinner.
> 
> The arisc firmware for H3 contains table of factors for frequences from
> 0 to 2GHz and, P is used below 240MHz. M is never used, BTW. (other
> datasheets specify M as for testing use only, whatever that means - not
> H3, but it seems it's the same PLL block)

Interesting. Which SoCs in particular?

>  +if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
>  +reg = FACTOR_SET(config->mshift, config->mwidth, reg, 
>  req->m);
>  +
>  +writel(reg, factors->reg);
>  +__delay(2000);
>  +}
>  +
>  +reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
>  +reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
>  +
>  +writel(reg, factors->reg);
>  +__delay(20);
>  +
>  +while (!(readl(factors->reg) & (1 << config->lock)));
> >>>
> >>> So, they are applying the dividers first, and then applying the
> >>> multipliers, and then wait for the PLL to stabilize.
> >>
> >> Not exactly, first we are increasing dividers if the new dividers are
> >> higher that that what's already set. This ensures that because
> >> application of dividers is immediate by the design of the PLL, the
> >> application of multipliers isn't. So the VCO would still run at the same
> >> frequency for a while gradually rising to a new value for example,
> >> while the dividers would be reduced immediately. Leading to crash.
> >>
> >> PLL
> >> --
> >> PRE DIV(f0) -> VCO(f1) -> POST DIV(f2)
> >>P K,N   M
> >>
> >> Example: (we set all factors at once, reducing dividers and multipliers
> >> at the same time at 0ms - this should lead to no change in the output
> >> frequency, but...)
> >>
> >> -1ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz
> >>  0ms: f0 = 24MHz, f1 = 2GHz,   f2 = 2GHz   - boom
> >>  1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 1.5GHz
> >>  2ms: f0 = 24MHz, f1 = 1GHz,   f2 = 1GHz
> >>
> >> The current code crashes exactly at boom, you don't get any more
> >> instructions to execute.
> >>
> >> See.
> >>
> >> So this patch first increases dividers (only if necessary), changes
> >> multipliers and waits for change to happen (takes around 2000 cycles),
> >> and then decreases dividers (only if necessary).
> >>
> >> So we get:
> >>
> >> -1ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz
> >>  0ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz   - no boom, multiplier
> >>  reduced
> >>  1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 0.75GHz
> >> 1.9ms: f0 = 24MHz, f1 = 1GHz,   f2 = 0.5GHz - we got PLL sync
> >>  2ms: f0 = 24MHz, f1 = 1GHz,   f2 = 1GHz   - and here we reduce divider
> >> at last
> > 
> > Awesome explanation, thanks!
> > 
> > So I guess it really all boils down to the fact that the CPU is
> > clocked way outside of it's operating frequency while the PLL
> > stabilizes, right?
> 
> It may be, depending on the factors before and after change. I haven't
> tested what factors the mainline kernel calculates for each frequency.
> The arisc never uses M, and only uses P at frequencies that would not
> allow for this behavior.
> 
> I created a test program for arisc that runs a loop on the main CPU
> using msgbox to send pings to the arisc CPU, and the vary the PLL1
> randomly from the arisc, and haven't been able to lockup the main CPU
> yet with either method.
> 
> There's also AXI clock, that depends on PLL1. Arisc firmware uses the
> same method to change it while changing CPUX clock. If the clock would
> rise above certain frequency, AXI divider is increased before the PLL1
> change. If it would fall 

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Jean-Francois Moine]

On Fri, 15 Jul 2016 12:38:54 +0200
Ondřej Jirman  wrote:

> > If so, then yes, trying to switch to the 24MHz oscillator before
> > applying the factors, and then switching back when the PLL is stable
> > would be a nice solution.
> > 
> > I just checked, and all the SoCs we've had so far have that
> > possibility, so if it works, for now, I'd like to stick to that.
> 
> It would need to be tested. U-boot does the change only once, while the
> kernel would be doing it all the time and between various frequencies
> and PLL settings. So the issues may show up with this solution too.

I don't think this is a good idea: the CPU clock may be changed at any
time with the CPUFreq governor. I don't see the system moving from
1008MHz to 24MHz and then to 1200MHz when some computation is needed!

BTW, Ondřej, in my BPi M2+, I tried to change the CPU clock with your
code at kernel start time from 792MHz to 1008MHz, but the hardware
(arisc?) set an other value, and the system speed was lower than before
(the PLL-CPUx register is 0x90031521 on boot, I want to set it to
1410 and I read 0x91031f33 - sorry, I did not have a look at the
CPU SD pattern). Do you know why?

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Ondřej Jirman]

On 15.7.2016 16:22, Michal Suchanek wrote:
> Hello,
> 
> On 15 July 2016 at 15:48, Ondřej Jirman  wrote:
>>
>>
>> On 15.7.2016 15:27, Jean-Francois Moine wrote:
>>> On Fri, 15 Jul 2016 12:38:54 +0200
>>> Ondřej Jirman  wrote:
>>>
> If so, then yes, trying to switch to the 24MHz oscillator before
> applying the factors, and then switching back when the PLL is stable
> would be a nice solution.
>
> I just checked, and all the SoCs we've had so far have that
> possibility, so if it works, for now, I'd like to stick to that.

 It would need to be tested. U-boot does the change only once, while the
 kernel would be doing it all the time and between various frequencies
 and PLL settings. So the issues may show up with this solution too.
>>>
>>> I don't think this is a good idea: the CPU clock may be changed at any
>>> time with the CPUFreq governor. I don't see the system moving from
>>> 1008MHz to 24MHz and then to 1200MHz when some computation is needed!
>>
>> PLL lock time is around 10-20us, I'd guess based on the number of loops
>> in the PLL lock wait loop. So unless you'll be switching frequencies
>> many times per second, this should be barely noticeable.
>>
>> But I'd like a different solution too.
> 
> Do you have a patch to test this?
> 
> For me changing CPU frequency on Orange Pi One always locks up the
> system. I keep running it on the u-boot setup 1.08GHz at 1.1V

Not anymore. But it's quite simple to hack it into the
drivers/clk/sunxi/clk-factors.c:clk_factors_set_rate()

You can look at u-boot arch/arm/mach-sunxi/clock_sun6i.c:clock_set_pll1
for how to change the CPU clock source.

> Thanks
> 
> Michal
> 

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

Re: [linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Michal Suchanek]

Hello,

On 15 July 2016 at 15:48, Ondřej Jirman  wrote:
>
>
> On 15.7.2016 15:27, Jean-Francois Moine wrote:
>> On Fri, 15 Jul 2016 12:38:54 +0200
>> Ondřej Jirman  wrote:
>>
 If so, then yes, trying to switch to the 24MHz oscillator before
 applying the factors, and then switching back when the PLL is stable
 would be a nice solution.

 I just checked, and all the SoCs we've had so far have that
 possibility, so if it works, for now, I'd like to stick to that.
>>>
>>> It would need to be tested. U-boot does the change only once, while the
>>> kernel would be doing it all the time and between various frequencies
>>> and PLL settings. So the issues may show up with this solution too.
>>
>> I don't think this is a good idea: the CPU clock may be changed at any
>> time with the CPUFreq governor. I don't see the system moving from
>> 1008MHz to 24MHz and then to 1200MHz when some computation is needed!
>
> PLL lock time is around 10-20us, I'd guess based on the number of loops
> in the PLL lock wait loop. So unless you'll be switching frequencies
> many times per second, this should be barely noticeable.
>
> But I'd like a different solution too.

Do you have a patch to test this?

For me changing CPU frequency on Orange Pi One always locks up the
system. I keep running it on the u-boot setup 1.08GHz at 1.1V

Thanks

Michal

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Ondřej Jirman]

On 15.7.2016 15:27, Jean-Francois Moine wrote:
> On Fri, 15 Jul 2016 12:38:54 +0200
> Ondřej Jirman  wrote:
> 
>>> If so, then yes, trying to switch to the 24MHz oscillator before
>>> applying the factors, and then switching back when the PLL is stable
>>> would be a nice solution.
>>>
>>> I just checked, and all the SoCs we've had so far have that
>>> possibility, so if it works, for now, I'd like to stick to that.
>>
>> It would need to be tested. U-boot does the change only once, while the
>> kernel would be doing it all the time and between various frequencies
>> and PLL settings. So the issues may show up with this solution too.
> 
> I don't think this is a good idea: the CPU clock may be changed at any
> time with the CPUFreq governor. I don't see the system moving from
> 1008MHz to 24MHz and then to 1200MHz when some computation is needed!

PLL lock time is around 10-20us, I'd guess based on the number of loops
in the PLL lock wait loop. So unless you'll be switching frequencies
many times per second, this should be barely noticeable.

But I'd like a different solution too.

> BTW, Ondřej, in my BPi M2+, I tried to change the CPU clock with your
> code at kernel start time from 792MHz to 1008MHz, but the hardware
> (arisc?) set an other value, and the system speed was lower than before
> (the PLL-CPUx register is 0x90031521 on boot, I want to set it to
> 1410 and I read 0x91031f33 - sorry, I did not have a look at the
> CPU SD pattern). Do you know why?

No idea. Arisc shouldn't do anything, unless you load some firmware into
it, and release its reset line.

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Ondřej Jirman]

On 15.7.2016 10:53, Maxime Ripard wrote:
> On Fri, Jul 01, 2016 at 02:50:57AM +0200, Ondřej Jirman wrote:
  /**
 + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
 + * register using an algorithm that tries to reserve the PLL lock
 + */
 +
 +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, 
 struct factors_request *req)
 +{
 +  const struct clk_factors_config *config = factors->config;
 +  u32 reg;
 +
 +  /* Fetch the register value */
 +  reg = readl(factors->reg);
 +
 +  if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
 +  reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
 +
 +  writel(reg, factors->reg);
 +  __delay(2000);
 +  }
>>>
>>> So there was some doubts about the fact that P was being used, or at
>>> least that it was useful.
>>
>> p is necessary to reduce frequencies below 288 MHz according to the
>> datasheet.
> 
> Yes, but you could reach those frequencies without P, too, and it's
> not part of any OPP provided by Allwinner.

The arisc firmware for H3 contains table of factors for frequences from
0 to 2GHz and, P is used below 240MHz. M is never used, BTW. (other
datasheets specify M as for testing use only, whatever that means - not
H3, but it seems it's the same PLL block)


... snip ...
{ .n = 9, .k = 0, .m = 0, .p = 2 }, // 60 => 60 MHz
{ .n = 10, .k = 0, .m = 0, .p = 2 }, // 66 => 66 MHz
{ .n = 11, .k = 0, .m = 0, .p = 2 }, // 72 => 72 MHz
{ .n = 12, .k = 0, .m = 0, .p = 2 }, // 78 => 78 MHz
{ .n = 13, .k = 0, .m = 0, .p = 2 }, // 84 => 84 MHz
{ .n = 14, .k = 0, .m = 0, .p = 2 }, // 90 => 90 MHz
{ .n = 15, .k = 0, .m = 0, .p = 2 }, // 96 => 96 MHz
{ .n = 16, .k = 0, .m = 0, .p = 2 }, // 102 => 102 MHz
{ .n = 17, .k = 0, .m = 0, .p = 2 }, // 108 => 108 MHz
{ .n = 18, .k = 0, .m = 0, .p = 2 }, // 114 => 114 MHz
{ .n = 9, .k = 0, .m = 0, .p = 1 }, // 120 => 120 MHz
{ .n = 10, .k = 0, .m = 0, .p = 1 }, // 126 => 132 MHz
{ .n = 10, .k = 0, .m = 0, .p = 1 }, // 132 => 132 MHz
{ .n = 11, .k = 0, .m = 0, .p = 1 }, // 138 => 144 MHz
{ .n = 11, .k = 0, .m = 0, .p = 1 }, // 144 => 144 MHz
{ .n = 12, .k = 0, .m = 0, .p = 1 }, // 150 => 156 MHz
{ .n = 12, .k = 0, .m = 0, .p = 1 }, // 156 => 156 MHz
{ .n = 13, .k = 0, .m = 0, .p = 1 }, // 162 => 168 MHz
{ .n = 13, .k = 0, .m = 0, .p = 1 }, // 168 => 168 MHz
{ .n = 14, .k = 0, .m = 0, .p = 1 }, // 174 => 180 MHz
{ .n = 14, .k = 0, .m = 0, .p = 1 }, // 180 => 180 MHz
{ .n = 15, .k = 0, .m = 0, .p = 1 }, // 186 => 192 MHz
{ .n = 15, .k = 0, .m = 0, .p = 1 }, // 192 => 192 MHz
{ .n = 16, .k = 0, .m = 0, .p = 1 }, // 198 => 204 MHz
{ .n = 16, .k = 0, .m = 0, .p = 1 }, // 204 => 204 MHz
{ .n = 17, .k = 0, .m = 0, .p = 1 }, // 210 => 216 MHz
{ .n = 17, .k = 0, .m = 0, .p = 1 }, // 216 => 216 MHz
{ .n = 18, .k = 0, .m = 0, .p = 1 }, // 222 => 228 MHz
{ .n = 18, .k = 0, .m = 0, .p = 1 }, // 228 => 228 MHz
{ .n = 9, .k = 0, .m = 0, .p = 0 }, // 234 => 240 MHz
{ .n = 9, .k = 0, .m = 0, .p = 0 }, // 240 => 240 MHz
{ .n = 10, .k = 0, .m = 0, .p = 0 }, // 246 => 264 MHz
{ .n = 10, .k = 0, .m = 0, .p = 0 }, // 252 => 264 MHz
{ .n = 10, .k = 0, .m = 0, .p = 0 }, // 258 => 264 MHz
{ .n = 10, .k = 0, .m = 0, .p = 0 }, // 264 => 264 MHz
{ .n = 11, .k = 0, .m = 0, .p = 0 }, // 270 => 288 MHz
{ .n = 11, .k = 0, .m = 0, .p = 0 }, // 276 => 288 MHz
... snip ...


 +  if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
 +  reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
 +
 +  writel(reg, factors->reg);
 +  __delay(2000);
 +  }
 +
 +  reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
 +  reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
 +
 +  writel(reg, factors->reg);
 +  __delay(20);
 +
 +  while (!(readl(factors->reg) & (1 << config->lock)));
>>>
>>> So, they are applying the dividers first, and then applying the
>>> multipliers, and then wait for the PLL to stabilize.
>>
>> Not exactly, first we are increasing dividers if the new dividers are
>> higher that that what's already set. This ensures that because
>> application of dividers is immediate by the design of the PLL, the
>> application of multipliers isn't. So the VCO would still run at the same
>> frequency for a while gradually rising to a new value for example,
>> while the dividers would be reduced immediately. Leading to crash.
>>
>> PLL
>> --
>> PRE DIV(f0) -> VCO(f1) -> POST DIV(f2)
>>P K,N   M
>>
>> Example: (we set all factors at once, reducing dividers and multipliers

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Maxime Ripard]

On Fri, Jul 01, 2016 at 02:53:52AM +0200, Ondřej Jirman wrote:
> On 30.6.2016 22:40, Maxime Ripard wrote:
> > Hi,
> > 
> > On Sat, Jun 25, 2016 at 05:45:03AM +0200, meg...@megous.com wrote:
> >> From: Ondrej Jirman 
> >>
> >> PLL1 on H3 requires special factors application algorithm,
> >> when the rate is changed. This algorithm was extracted
> >> from the arisc code that handles frequency scaling
> >> in the BSP kernel.
> >>
> >> This commit adds optional apply function to
> >> struct factors_data, that can implement non-trivial
> >> factors application method, when necessary.
> >>
> >> Also struct clk_factors_config is extended with position
> >> of the PLL lock flag.
> > 
> > Have you tested the current implementation, and found that it was not
> > working, or did you duplicate the arisc code directly?
> 
> Also of note is that similar code probably doesn't crash in u-boot,
> because there, before changing the PLL1 clock, the cpu is switched to
> 24MHz osc, so it is not overclocked, even if factors align in such a way
> that you'd get the behavior I described in the other email.

That's also something that we can do.

See Meson's clk-cpu clock notifiers for example.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Maxime Ripard]

On Fri, Jul 01, 2016 at 02:50:57AM +0200, Ondřej Jirman wrote:
> >>  /**
> >> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
> >> + * register using an algorithm that tries to reserve the PLL lock
> >> + */
> >> +
> >> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, 
> >> struct factors_request *req)
> >> +{
> >> +  const struct clk_factors_config *config = factors->config;
> >> +  u32 reg;
> >> +
> >> +  /* Fetch the register value */
> >> +  reg = readl(factors->reg);
> >> +
> >> +  if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
> >> +  reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
> >> +
> >> +  writel(reg, factors->reg);
> >> +  __delay(2000);
> >> +  }
> > 
> > So there was some doubts about the fact that P was being used, or at
> > least that it was useful.
> 
> p is necessary to reduce frequencies below 288 MHz according to the
> datasheet.

Yes, but you could reach those frequencies without P, too, and it's
not part of any OPP provided by Allwinner.

> >> +  if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
> >> +  reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
> >> +
> >> +  writel(reg, factors->reg);
> >> +  __delay(2000);
> >> +  }
> >> +
> >> +  reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
> >> +  reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
> >> +
> >> +  writel(reg, factors->reg);
> >> +  __delay(20);
> >> +
> >> +  while (!(readl(factors->reg) & (1 << config->lock)));
> > 
> > So, they are applying the dividers first, and then applying the
> > multipliers, and then wait for the PLL to stabilize.
> 
> Not exactly, first we are increasing dividers if the new dividers are
> higher that that what's already set. This ensures that because
> application of dividers is immediate by the design of the PLL, the
> application of multipliers isn't. So the VCO would still run at the same
> frequency for a while gradually rising to a new value for example,
> while the dividers would be reduced immediately. Leading to crash.
> 
> PLL
> --
> PRE DIV(f0) -> VCO(f1) -> POST DIV(f2)
>P K,N   M
> 
> Example: (we set all factors at once, reducing dividers and multipliers
> at the same time at 0ms - this should lead to no change in the output
> frequency, but...)
> 
> -1ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz
>  0ms: f0 = 24MHz, f1 = 2GHz,   f2 = 2GHz   - boom
>  1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 1.5GHz
>  2ms: f0 = 24MHz, f1 = 1GHz,   f2 = 1GHz
> 
> The current code crashes exactly at boom, you don't get any more
> instructions to execute.
> 
> See.
> 
> So this patch first increases dividers (only if necessary), changes
> multipliers and waits for change to happen (takes around 2000 cycles),
> and then decreases dividers (only if necessary).
> 
> So we get:
> 
> -1ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz
>  0ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz   - no boom, multiplier
>  reduced
>  1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 0.75GHz
> 1.9ms: f0 = 24MHz, f1 = 1GHz,   f2 = 0.5GHz - we got PLL sync
>  2ms: f0 = 24MHz, f1 = 1GHz,   f2 = 1GHz   - and here we reduce divider
> at last

Awesome explanation, thanks!

So I guess it really all boils down to the fact that the CPU is
clocked way outside of it's operating frequency while the PLL
stabilizes, right?

If so, then yes, trying to switch to the 24MHz oscillator before
applying the factors, and then switching back when the PLL is stable
would be a nice solution.

I just checked, and all the SoCs we've had so far have that
possibility, so if it works, for now, I'd like to stick to that.

> >> +
> >> +  if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) {
> >> +  reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
> >> +
> >> +  writel(reg, factors->reg);
> >> +  __delay(2000);
> >> +  }
> >> +
> >> +  if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) {
> >> +  reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
> >> +
> >> +  writel(reg, factors->reg);
> >> +  __delay(2000);
> >> +  }
> > 
> > However, this is kind of weird, why would you need to re-apply the
> > dividers? Nothing really changes. Have you tried without that part?
> 
> See above, we either increase before PLL change, or reduce dividers
> after the change. Nothing is re-applied.
> 
> > Since this is really specific, I guess you could simply make the
> > clk_ops for the nkmp clocks public, and just re-implement set_rate
> > using that logic.
> 
> I would argue that this may be necessary for other PLL clocks too, if
> you can get out of bounds output frequency, by changing the dividers too
> early or too late. So perhaps this code should be generalized for other
> PLL clocks too, instead.

We can scale down the problem a bit. The only 

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Jean-Francois Moine]

On Fri, 1 Jul 2016 02:50:57 +0200
Ondřej Jirman  wrote:

> > Since this is really specific, I guess you could simply make the
> > clk_ops for the nkmp clocks public, and just re-implement set_rate
> > using that logic.
> 
> I would argue that this may be necessary for other PLL clocks too, if
> you can get out of bounds output frequency, by changing the dividers too
> early or too late. So perhaps this code should be generalized for other
> PLL clocks too, instead.

The documentation says that only the CPU and DDR PLLs can be dynamically
changed after boot.

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Jean-Francois Moine]

On Fri, 1 Jul 2016 08:34:21 +0200
Ondřej Jirman  wrote:

> > The documentation says that only the CPU and DDR PLLs can be dynamically
> > changed after boot.
> 
> The question is what exactly is meant by after boot. :) Anyway, if the
> kernel has no business changing some other PLLs, if there's code for
> changing them, should it be dropped?

No, because all the other PLLs may not be initialized by the U-boot
(audio, video, gpu...), and also, their rate may be changed safely by
stopping them (gate).

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Ondřej Jirman]

On 1.7.2016 07:37, Jean-Francois Moine wrote:
> On Fri, 1 Jul 2016 02:50:57 +0200
> Ondřej Jirman  wrote:
> 
>>> Since this is really specific, I guess you could simply make the
>>> clk_ops for the nkmp clocks public, and just re-implement set_rate
>>> using that logic.
>>
>> I would argue that this may be necessary for other PLL clocks too, if
>> you can get out of bounds output frequency, by changing the dividers too
>> early or too late. So perhaps this code should be generalized for other
>> PLL clocks too, instead.
> 
> The documentation says that only the CPU and DDR PLLs can be dynamically
> changed after boot.

The question is what exactly is meant by after boot. :) Anyway, if the
kernel has no business changing some other PLLs, if there's code for
changing them, should it be dropped?

regards,
  Ondrej

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Ondřej Jirman]

On 30.6.2016 22:40, Maxime Ripard wrote:
> Hi,
> 
> On Sat, Jun 25, 2016 at 05:45:03AM +0200, meg...@megous.com wrote:
>> From: Ondrej Jirman 
>>
>> PLL1 on H3 requires special factors application algorithm,
>> when the rate is changed. This algorithm was extracted
>> from the arisc code that handles frequency scaling
>> in the BSP kernel.
>>
>> This commit adds optional apply function to
>> struct factors_data, that can implement non-trivial
>> factors application method, when necessary.
>>
>> Also struct clk_factors_config is extended with position
>> of the PLL lock flag.
> 
> Have you tested the current implementation, and found that it was not
> working, or did you duplicate the arisc code directly?

Also of note is that similar code probably doesn't crash in u-boot,
because there, before changing the PLL1 clock, the cpu is switched to
24MHz osc, so it is not overclocked, even if factors align in such a way
that you'd get the behavior I described in the other email.

>>  /**
>> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
>> + * register using an algorithm that tries to reserve the PLL lock
>> + */
>> +
>> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct 
>> factors_request *req)
>> +{
>> +const struct clk_factors_config *config = factors->config;
>> +u32 reg;
>> +
>> +/* Fetch the register value */
>> +reg = readl(factors->reg);
>> +
>> +if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
>> +reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
>> +
>> +writel(reg, factors->reg);
>> +__delay(2000);
>> +}
> 
> So there was some doubts about the fact that P was being used, or at
> least that it was useful.
> 
>> +if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
>> +reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
>> +
>> +writel(reg, factors->reg);
>> +__delay(2000);
>> +}
>> +
>> +reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
>> +reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
>> +
>> +writel(reg, factors->reg);
>> +__delay(20);
>> +
>> +while (!(readl(factors->reg) & (1 << config->lock)));
> 
> So, they are applying the dividers first, and then applying the
> multipliers, and then wait for the PLL to stabilize.
> 
>> +
>> +if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) {
>> +reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
>> +
>> +writel(reg, factors->reg);
>> +__delay(2000);
>> +}
>> +
>> +if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) {
>> +reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
>> +
>> +writel(reg, factors->reg);
>> +__delay(2000);
>> +}
> 
> However, this is kind of weird, why would you need to re-apply the
> dividers? Nothing really changes. Have you tried without that part?
> 
> Since this is really specific, I guess you could simply make the
> clk_ops for the nkmp clocks public, and just re-implement set_rate
> using that logic.
> 
> You might also need to set an upper limit on P, since the last value
> (4) is not a valid one.
> 
> I guess you could do that by adding a max field in the __ccu_div
> structure.
> 
> Maxime
> 

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Ondřej Jirman]

Hi,

On 30.6.2016 22:40, Maxime Ripard wrote:
> Hi,
> 
> On Sat, Jun 25, 2016 at 05:45:03AM +0200, meg...@megous.com wrote:
>> From: Ondrej Jirman 
>>
>> PLL1 on H3 requires special factors application algorithm,
>> when the rate is changed. This algorithm was extracted
>> from the arisc code that handles frequency scaling
>> in the BSP kernel.
>>
>> This commit adds optional apply function to
>> struct factors_data, that can implement non-trivial
>> factors application method, when necessary.
>>
>> Also struct clk_factors_config is extended with position
>> of the PLL lock flag.
> 
> Have you tested the current implementation, and found that it was not
> working, or did you duplicate the arisc code directly?

I have tested the current implementation, and it was not working. It
depended on some other factors, like the initial setup done by u-boot.
It didn't work reliably.

Then I reverse engineered arisc, in an effort to see what's the
difference, between mainline and BSP code.

>>  /**
>> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
>> + * register using an algorithm that tries to reserve the PLL lock
>> + */
>> +
>> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct 
>> factors_request *req)
>> +{
>> +const struct clk_factors_config *config = factors->config;
>> +u32 reg;
>> +
>> +/* Fetch the register value */
>> +reg = readl(factors->reg);
>> +
>> +if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
>> +reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
>> +
>> +writel(reg, factors->reg);
>> +__delay(2000);
>> +}
> 
> So there was some doubts about the fact that P was being used, or at
> least that it was useful.

p is necessary to reduce frequencies below 288 MHz according to the
datasheet.

>> +if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
>> +reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
>> +
>> +writel(reg, factors->reg);
>> +__delay(2000);
>> +}
>> +
>> +reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
>> +reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
>> +
>> +writel(reg, factors->reg);
>> +__delay(20);
>> +
>> +while (!(readl(factors->reg) & (1 << config->lock)));
> 
> So, they are applying the dividers first, and then applying the
> multipliers, and then wait for the PLL to stabilize.

Not exactly, first we are increasing dividers if the new dividers are
higher that that what's already set. This ensures that because
application of dividers is immediate by the design of the PLL, the
application of multipliers isn't. So the VCO would still run at the same
frequency for a while gradually rising to a new value for example,
while the dividers would be reduced immediately. Leading to crash.

PLL
--
PRE DIV(f0) -> VCO(f1) -> POST DIV(f2)
   P K,N   M

Example: (we set all factors at once, reducing dividers and multipliers
at the same time at 0ms - this should lead to no change in the output
frequency, but...)

-1ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz
 0ms: f0 = 24MHz, f1 = 2GHz,   f2 = 2GHz   - boom
 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 1.5GHz
 2ms: f0 = 24MHz, f1 = 1GHz,   f2 = 1GHz

The current code crashes exactly at boom, you don't get any more
instructions to execute.

See.

So this patch first increases dividers (only if necessary), changes
multipliers and waits for change to happen (takes around 2000 cycles),
and then decreases dividers (only if necessary).

So we get:

-1ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz
 0ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz   - no boom, multiplier
 reduced
 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 0.75GHz
1.9ms: f0 = 24MHz, f1 = 1GHz,   f2 = 0.5GHz - we got PLL sync
 2ms: f0 = 24MHz, f1 = 1GHz,   f2 = 1GHz   - and here we reduce divider
at last

>> +
>> +if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) {
>> +reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
>> +
>> +writel(reg, factors->reg);
>> +__delay(2000);
>> +}
>> +
>> +if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) {
>> +reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
>> +
>> +writel(reg, factors->reg);
>> +__delay(2000);
>> +}
> 
> However, this is kind of weird, why would you need to re-apply the
> dividers? Nothing really changes. Have you tried without that part?

See above, we either increase before PLL change, or reduce dividers
after the change. Nothing is re-applied.

> Since this is really specific, I guess you could simply make the
> clk_ops for the nkmp clocks public, and just re-implement set_rate
> using that logic.

I would argue that this may be necessary for other PLL clocks too, if
you can 

[linux-sunxi] Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Maxime Ripard]

Hi,

On Sat, Jun 25, 2016 at 05:45:03AM +0200, meg...@megous.com wrote:
> From: Ondrej Jirman 
> 
> PLL1 on H3 requires special factors application algorithm,
> when the rate is changed. This algorithm was extracted
> from the arisc code that handles frequency scaling
> in the BSP kernel.
> 
> This commit adds optional apply function to
> struct factors_data, that can implement non-trivial
> factors application method, when necessary.
> 
> Also struct clk_factors_config is extended with position
> of the PLL lock flag.

Have you tested the current implementation, and found that it was not
working, or did you duplicate the arisc code directly?

>  /**
> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
> + * register using an algorithm that tries to reserve the PLL lock
> + */
> +
> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct 
> factors_request *req)
> +{
> + const struct clk_factors_config *config = factors->config;
> + u32 reg;
> +
> + /* Fetch the register value */
> + reg = readl(factors->reg);
> +
> + if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
> +
> + writel(reg, factors->reg);
> + __delay(2000);
> + }

So there was some doubts about the fact that P was being used, or at
least that it was useful.

> + if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
> +
> + writel(reg, factors->reg);
> + __delay(2000);
> + }
> +
> + reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
> + reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
> +
> + writel(reg, factors->reg);
> + __delay(20);
> +
> + while (!(readl(factors->reg) & (1 << config->lock)));

So, they are applying the dividers first, and then applying the
multipliers, and then wait for the PLL to stabilize.

> +
> + if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) {
> + reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
> +
> + writel(reg, factors->reg);
> + __delay(2000);
> + }
> +
> + if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) {
> + reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
> +
> + writel(reg, factors->reg);
> + __delay(2000);
> + }

However, this is kind of weird, why would you need to re-apply the
dividers? Nothing really changes. Have you tried without that part?

Since this is really specific, I guess you could simply make the
clk_ops for the nkmp clocks public, and just re-implement set_rate
using that logic.

You might also need to set an upper limit on P, since the last value
(4) is not a valid one.

I guess you could do that by adding a max field in the __ccu_div
structure.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature