[PATCH] clk: samsung: Fix PLL35XX lock time

[PATCH] clk: samsung: Fix PLL35XX lock time

[Arun Kumar K]

PLL35XX lock factor is 250 as per the manual whereas its
wrongly set as 270 now.

Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
---
 drivers/clk/samsung/clk-pll.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 529e11d..ad84be0 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -61,8 +61,8 @@ static long samsung_pll_round_rate(struct clk_hw *hw,
 /*
  * PLL35xx Clock Type
  */
-/* Maximum lock time can be 270 * PDIV cycles */
-#define PLL35XX_LOCK_FACTOR	(270)
+/* Maximum lock time can be 250 * PDIV cycles */
+#define PLL35XX_LOCK_FACTOR	(250)
 
 #define PLL35XX_MDIV_MASK       (0x3FF)
 #define PLL35XX_PDIV_MASK       (0x3F)
-- 
1.7.9.5

Re: [PATCH] clk: samsung: Fix PLL35XX lock time

[Doug Anderson]

Arun,

On Mon, Oct 7, 2013 at 11:56 PM, Arun Kumar K <arun.kk@samsung.com> wrote:
> PLL35XX lock factor is 250 as per the manual whereas its
> wrongly set as 270 now.
>
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> ---
>  drivers/clk/samsung/clk-pll.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

I don't think this is right.  I believe that it needs to be passed in
by the SoC.  On exynos5250 I see 250 in both the manual and in our
code.  On exynos5420 manual I actually see 200 now.  I'm not actually
sure where the 270 came from, now that I search for it.

-Doug

Re: [PATCH] clk: samsung: Fix PLL35XX lock time

[Arun Kumar K]

Hi Doug,

On Tue, Oct 8, 2013 at 9:39 PM, Doug Anderson <dianders@chromium.org> wrote:
> Arun,
>
> On Mon, Oct 7, 2013 at 11:56 PM, Arun Kumar K <arun.kk@samsung.com> wrote:
>> PLL35XX lock factor is 250 as per the manual whereas its
>> wrongly set as 270 now.
>>
>> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
>> ---
>>  drivers/clk/samsung/clk-pll.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> I don't think this is right.  I believe that it needs to be passed in
> by the SoC.  On exynos5250 I see 250 in both the manual and in our
> code.  On exynos5420 manual I actually see 200 now.  I'm not actually
> sure where the 270 came from, now that I search for it.
>

The manual I referred gives 250 for both 5250 and 5420 and thats why I
concluded on this fix. I will check other versions of the manual to see if
the value is changing.

Regards
Arun

Re: [PATCH] clk: samsung: Fix PLL35XX lock time

[Tomasz Figa]

Hi Arun,

On Wednesday 09 of October 2013 09:21:43 Arun Kumar K wrote:
> Hi Doug,
> 
> On Tue, Oct 8, 2013 at 9:39 PM, Doug Anderson <dianders@chromium.org> wrote:
> > Arun,
> > 
> > On Mon, Oct 7, 2013 at 11:56 PM, Arun Kumar K <arun.kk@samsung.com> wrote:
> >> PLL35XX lock factor is 250 as per the manual whereas its
> >> wrongly set as 270 now.
> >> 
> >> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> >> ---
> >> 
> >>  drivers/clk/samsung/clk-pll.c |    4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > I don't think this is right.  I believe that it needs to be passed in
> > by the SoC.  On exynos5250 I see 250 in both the manual and in our
> > code.  On exynos5420 manual I actually see 200 now.  I'm not actually
> > sure where the 270 came from, now that I search for it.
> 
> The manual I referred gives 250 for both 5250 and 5420 and thats why I
> concluded on this fix. I will check other versions of the manual to see if
> the value is changing.

270 is the value taken from Exynos 4412 User's Manual rev. 1.1.

I believe we don't have to extend this to support per SoC value, because it 
should be always safe to take the value of SoC that requires the longest 
locking period. The differences are small enough to be insignificant, so there 
should be no practical performance penalty.

Best regards,
Tomasz

Re: [PATCH] clk: samsung: Fix PLL35XX lock time

[Doug Anderson]

Tomasz,

On Wed, Oct 9, 2013 at 10:20 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> > I don't think this is right.  I believe that it needs to be passed in
>> > by the SoC.  On exynos5250 I see 250 in both the manual and in our
>> > code.  On exynos5420 manual I actually see 200 now.  I'm not actually
>> > sure where the 270 came from, now that I search for it.
>>
>> The manual I referred gives 250 for both 5250 and 5420 and thats why I
>> concluded on this fix. I will check other versions of the manual to see if
>> the value is changing.
>
> 270 is the value taken from Exynos 4412 User's Manual rev. 1.1.
>
> I believe we don't have to extend this to support per SoC value, because it
> should be always safe to take the value of SoC that requires the longest
> locking period. The differences are small enough to be insignificant, so there
> should be no practical performance penalty.

It's not _that_ small for something like APLL which can be changing
constantly in DVFS.

24MHz input clock, right?  ...and I see some P values of up to 6.

That means with 270 lock time is 67.5 us, right?
...and with 200 lock time is 50 us, right?

...so that's a 17.5 us difference by using the correct multiplier.
Overall DVFS switch time was measured recently at ~350us, so that
would give us a 5% speedup on DVFS switching.

We've had debates in-house in the past about how critical DVFS
switching time is.  In the past there have been some who have
advocated that it's very important, since it affects responsiveness
(how fast can you spin up when you suddenly have something to do).


Anyway, when it's something as simple as passing in a parameter to get
it right, it seems worth doing.

-Doug

Re: [PATCH] clk: samsung: Fix PLL35XX lock time

[Tomasz Figa]

On Friday 11 of October 2013 08:14:03 Doug Anderson wrote:
> Tomasz,
> 
> On Wed, Oct 9, 2013 at 10:20 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> >> > I don't think this is right.  I believe that it needs to be passed in
> >> > by the SoC.  On exynos5250 I see 250 in both the manual and in our
> >> > code.  On exynos5420 manual I actually see 200 now.  I'm not actually
> >> > sure where the 270 came from, now that I search for it.
> >>
> >> The manual I referred gives 250 for both 5250 and 5420 and thats why I
> >> concluded on this fix. I will check other versions of the manual to see if
> >> the value is changing.
> >
> > 270 is the value taken from Exynos 4412 User's Manual rev. 1.1.
> >
> > I believe we don't have to extend this to support per SoC value, because it
> > should be always safe to take the value of SoC that requires the longest
> > locking period. The differences are small enough to be insignificant, so there
> > should be no practical performance penalty.
> 
> It's not _that_ small for something like APLL which can be changing
> constantly in DVFS.
> 
> 24MHz input clock, right?  ...and I see some P values of up to 6.
> 
> That means with 270 lock time is 67.5 us, right?
> ...and with 200 lock time is 50 us, right?
> 
> ...so that's a 17.5 us difference by using the correct multiplier.
> Overall DVFS switch time was measured recently at ~350us, so that
> would give us a 5% speedup on DVFS switching.
>
> We've had debates in-house in the past about how critical DVFS
> switching time is.  In the past there have been some who have
> advocated that it's very important, since it affects responsiveness
> (how fast can you spin up when you suddenly have something to do).

Well, it's some kind of difference indeed. However, how often can
a frequency transition happen?

I believe that ondemand allows minimum sampling period of
100 * transition latency, so even without considering the PLL lock time,
we would have at most a single delay of ~400 us every ~40 ms, during
which remaining CPUs are operating normally, because of switching to
MPLL temporarily.

A bit more interesting case would be the Android interactive governor,
which scales up the CPU on any wake-up event, but I don't believe any
user would notice the extra 17,5 us from touching the screen to seeing
a menu animation, for example.

Please correct me if I'm missing something here.

> Anyway, when it's something as simple as passing in a parameter to get
> it right, it seems worth doing.

Sure, any patches improving things are welcome, I was just wondering
whether the improvement will be of any significance. Looking forward
to respective patches, hoping that they will not complicate the code
too much (although we already have a struct for PLL parameters, so they
should not).

Best regards,
Tomasz

Re: [PATCH] clk: samsung: Fix PLL35XX lock time

[Doug Anderson]

Tomasz,

On Fri, Oct 11, 2013 at 7:06 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Well, it's some kind of difference indeed. However, how often can
> a frequency transition happen?
>
> I believe that ondemand allows minimum sampling period of
> 100 * transition latency, so even without considering the PLL lock time,
> we would have at most a single delay of ~400 us every ~40 ms, during
> which remaining CPUs are operating normally, because of switching to
> MPLL temporarily.
>
> A bit more interesting case would be the Android interactive governor,
> which scales up the CPU on any wake-up event, but I don't believe any
> user would notice the extra 17,5 us from touching the screen to seeing
> a menu animation, for example.

OK.  You've clearly looked at this more than I.  :)  I was merely
recalling conversations that others had over a year ago and
remembering some of our engineers being concerned about latencies even
at this level, but I'm not sure if they had hard data.  I added Sonny
to see if he might remember more, though I don't think he was the one
pushing for optimizations in the past.

Note that we actually are using the Interactive governor in our
systems, and it is the interactive "time to spin up on user input"
that I'm most worried about.  ...but you're right that I think the
user perceivable values are in the 10s of milliseconds and not in the
10s of microseconds.


> Please correct me if I'm missing something here.
>
>> Anyway, when it's something as simple as passing in a parameter to get
>> it right, it seems worth doing.
>
> Sure, any patches improving things are welcome, I was just wondering
> whether the improvement will be of any significance. Looking forward
> to respective patches, hoping that they will not complicate the code
> too much (although we already have a struct for PLL parameters, so they
> should not).

I'd still love to see the change land that allows this to be right.
17.5us may not matter much, but with the PLL parameter struct it
should be pretty easy / clean.  ...and it also allows us to map the
number back the user manual, which can make it easier to understand
(there's currently nothing documenting why the number is 270, not 250
or 200).

-Doug

Re: [PATCH] clk: samsung: Fix PLL35XX lock time

[Sonny Rao]

On Mon, Oct 14, 2013 at 9:08 PM, Doug Anderson <dianders@chromium.org> wrote:
> Tomasz,
>
> On Fri, Oct 11, 2013 at 7:06 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> Well, it's some kind of difference indeed. However, how often can
>> a frequency transition happen?
>>
>> I believe that ondemand allows minimum sampling period of
>> 100 * transition latency, so even without considering the PLL lock time,
>> we would have at most a single delay of ~400 us every ~40 ms, during
>> which remaining CPUs are operating normally, because of switching to
>> MPLL temporarily.
>>
>> A bit more interesting case would be the Android interactive governor,
>> which scales up the CPU on any wake-up event, but I don't believe any
>> user would notice the extra 17,5 us from touching the screen to seeing
>> a menu animation, for example.
>
> OK.  You've clearly looked at this more than I.  :)  I was merely
> recalling conversations that others had over a year ago and
> remembering some of our engineers being concerned about latencies even
> at this level, but I'm not sure if they had hard data.  I added Sonny
> to see if he might remember more, though I don't think he was the one
> pushing for optimizations in the past.
>
>
> Note that we actually are using the Interactive governor in our
> systems, and it is the interactive "time to spin up on user input"
> that I'm most worried about.  ...but you're right that I think the
> user perceivable values are in the 10s of milliseconds and not in the
> 10s of microseconds.


Yeah, we're using interactive, and we're typically running on the
slowest clock frequency when a user uses an input device that causes a
boost to happen, and I think the initial measurements (last year) of
how long it took to complete just the voltage transition on Snow were
something like 1 millisecond mostly because of how udelay worked by
using a fixed number of loops.  That seemed pretty long but wasn't
considered terrible because after we got up to a higher speed it
wouldn't take as long to make further transitions, and I also didn't
expect us to make that transition often enough that it would be a
significant impact on CPU usage, and 1ms probably isn't quite enough
for a user to notice.

>
>
>> Please correct me if I'm missing something here.
>>
>>> Anyway, when it's something as simple as passing in a parameter to get
>>> it right, it seems worth doing.
>>
>> Sure, any patches improving things are welcome, I was just wondering
>> whether the improvement will be of any significance. Looking forward
>> to respective patches, hoping that they will not complicate the code
>> too much (although we already have a struct for PLL parameters, so they
>> should not).
>
> I'd still love to see the change land that allows this to be right.
> 17.5us may not matter much, but with the PLL parameter struct it
> should be pretty easy / clean.  ...and it also allows us to map the
> number back the user manual, which can make it easier to understand
> (there's currently nothing documenting why the number is 270, not 250
> or 200).
>
> -Doug