Re: [PATCH] clk: samsung: Fix PLL35XX lock time
On Mon, Oct 14, 2013 at 9:08 PM, Doug Anderson diand...@chromium.org wrote: Tomasz, On Fri, Oct 11, 2013 at 7:06 PM, Tomasz Figa t.f...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clk: samsung: Fix PLL35XX lock time
Tomasz, On Fri, Oct 11, 2013 at 7:06 PM, Tomasz Figa t.f...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clk: samsung: Fix PLL35XX lock time
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.f...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clk: samsung: Fix PLL35XX lock time
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 diand...@chromium.org wrote: Arun, On Mon, Oct 7, 2013 at 11:56 PM, Arun Kumar K arun...@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...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] clk: samsung: Fix PLL35XX lock time
PLL35XX lock factor is 250 as per the manual whereas its wrongly set as 270 now. Signed-off-by: Arun Kumar K arun...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clk: samsung: Fix PLL35XX lock time
Arun, On Mon, Oct 7, 2013 at 11:56 PM, Arun Kumar K arun...@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...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clk: samsung: Fix PLL35XX lock time
Hi Doug, On Tue, Oct 8, 2013 at 9:39 PM, Doug Anderson diand...@chromium.org wrote: Arun, On Mon, Oct 7, 2013 at 11:56 PM, Arun Kumar K arun...@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...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
