The exynos5 i2c clock is based on a fixed 66 MHz peripheral clock, and
therefore is completely independent of the cpu frequency.
Thus, registering for a CPU freq notifier is very wasteful.
This patch modifes the code such that, i2c bus registers to
cpu_freq_transition only for non Exynos SoCs.
This change should save a bunch of cpufreq transitions calls
which does not apply to exynos SoCs.
Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
---
drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index cab1c91..d062fa7 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -123,7 +123,7 @@ struct s3c24xx_i2c {
struct s3c2410_platform_i2c *pdata;
int gpios[2];
struct pinctrl *pctrl;
-#ifdef CONFIG_CPU_FREQ
+#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
struct notifier_block freq_transition;
#endif
};
@@ -843,7 +843,7 @@ static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c *i2c, unsigned int *got)
return 0;
}
-#ifdef CONFIG_CPU_FREQ
+#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
#define freq_to_i2c(_n) container_of(_n, struct s3c24xx_i2c, freq_transition)
--
1.7.9.5
Hi Naveen,
On Friday 11 of October 2013 16:56:54 Naveen Krishna Chatradhi wrote:
> The exynos5 i2c clock is based on a fixed 66 MHz peripheral clock, and
> therefore is completely independent of the cpu frequency.
> Thus, registering for a CPU freq notifier is very wasteful.
>
> This patch modifes the code such that, i2c bus registers to
> cpu_freq_transition only for non Exynos SoCs.
>
> This change should save a bunch of cpufreq transitions calls
> which does not apply to exynos SoCs.
The idea is fine, although...
> Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
> ---
> drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index cab1c91..d062fa7 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -123,7 +123,7 @@ struct s3c24xx_i2c {
> struct s3c2410_platform_i2c *pdata;
> int gpios[2];
> struct pinctrl *pctrl;
> -#ifdef CONFIG_CPU_FREQ
> +#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
...this is not a good coding practice, especially when already having
multiplatform kernels in sight.
The best way would be to check on which SoC we are running at runtime,
but since this might need changing a lot of code, then at least I would
change this from !defined(EXYNOS) to defined(S3C24XX), so it is not being
compiled in when S3C24XX support is not enabled and if it's enabled then
the notifier will be registered as a safe fallback that will run correctly
on all platforms.
Best regards,
Tomasz
[Fixing incorrent mail addresses and dropping the old DT ML.]
On Saturday 12 of October 2013 04:22:04 Tomasz Figa wrote:
> Hi Naveen,
>
> On Friday 11 of October 2013 16:56:54 Naveen Krishna Chatradhi wrote:
> > The exynos5 i2c clock is based on a fixed 66 MHz peripheral clock, and
> > therefore is completely independent of the cpu frequency.
> > Thus, registering for a CPU freq notifier is very wasteful.
> >
> > This patch modifes the code such that, i2c bus registers to
> > cpu_freq_transition only for non Exynos SoCs.
> >
> > This change should save a bunch of cpufreq transitions calls
> > which does not apply to exynos SoCs.
>
> The idea is fine, although...
>
> > Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> > index cab1c91..d062fa7 100644
> > --- a/drivers/i2c/busses/i2c-s3c2410.c
> > +++ b/drivers/i2c/busses/i2c-s3c2410.c
> > @@ -123,7 +123,7 @@ struct s3c24xx_i2c {
> > struct s3c2410_platform_i2c *pdata;
> > int gpios[2];
> > struct pinctrl *pctrl;
> > -#ifdef CONFIG_CPU_FREQ
> > +#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
>
> ...this is not a good coding practice, especially when already having
> multiplatform kernels in sight.
>
> The best way would be to check on which SoC we are running at runtime,
> but since this might need changing a lot of code, then at least I would
> change this from !defined(EXYNOS) to defined(S3C24XX), so it is not being
> compiled in when S3C24XX support is not enabled and if it's enabled then
> the notifier will be registered as a safe fallback that will run correctly
> on all platforms.
>
> Best regards,
> Tomasz
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 12 of October 2013 04:28:51 Tomasz Figa wrote:
> [Fixing incorrent mail addresses and dropping the old DT ML.]
>
> On Saturday 12 of October 2013 04:22:04 Tomasz Figa wrote:
> > Hi Naveen,
> >
> > On Friday 11 of October 2013 16:56:54 Naveen Krishna Chatradhi wrote:
> > > The exynos5 i2c clock is based on a fixed 66 MHz peripheral clock, and
> > > therefore is completely independent of the cpu frequency.
> > > Thus, registering for a CPU freq notifier is very wasteful.
> > >
> > > This patch modifes the code such that, i2c bus registers to
> > > cpu_freq_transition only for non Exynos SoCs.
> > >
> > > This change should save a bunch of cpufreq transitions calls
> > > which does not apply to exynos SoCs.
> >
> > The idea is fine, although...
> >
> > > Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
> > > ---
> > > drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> > > index cab1c91..d062fa7 100644
> > > --- a/drivers/i2c/busses/i2c-s3c2410.c
> > > +++ b/drivers/i2c/busses/i2c-s3c2410.c
> > > @@ -123,7 +123,7 @@ struct s3c24xx_i2c {
> > > struct s3c2410_platform_i2c *pdata;
> > > int gpios[2];
> > > struct pinctrl *pctrl;
> > > -#ifdef CONFIG_CPU_FREQ
> > > +#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
> >
> > ...this is not a good coding practice, especially when already having
> > multiplatform kernels in sight.
> >
> > The best way would be to check on which SoC we are running at runtime,
> > but since this might need changing a lot of code, then at least I would
> > change this from !defined(EXYNOS) to defined(S3C24XX), so it is not being
> > compiled in when S3C24XX support is not enabled and if it's enabled then
> > the notifier will be registered as a safe fallback that will run correctly
> > on all platforms.
Actually you can simply check for CONFIG_CPU_FREQ_S3C24XX here.
Best regards,
Tomasz
On 12 October 2013 11:12, Tomasz Figa <[email protected]> wrote:
> On Saturday 12 of October 2013 04:28:51 Tomasz Figa wrote:
>> [Fixing incorrent mail addresses and dropping the old DT ML.]
>>
>> On Saturday 12 of October 2013 04:22:04 Tomasz Figa wrote:
>> > Hi Naveen,
>> >
>> > On Friday 11 of October 2013 16:56:54 Naveen Krishna Chatradhi wrote:
>> > > The exynos5 i2c clock is based on a fixed 66 MHz peripheral clock, and
>> > > therefore is completely independent of the cpu frequency.
>> > > Thus, registering for a CPU freq notifier is very wasteful.
>> > >
>> > > This patch modifes the code such that, i2c bus registers to
>> > > cpu_freq_transition only for non Exynos SoCs.
>> > >
>> > > This change should save a bunch of cpufreq transitions calls
>> > > which does not apply to exynos SoCs.
>> >
>> > The idea is fine, although...
>> >
>> > > Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
>> > > ---
>> > > drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
>> > > 1 file changed, 2 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
>> > > index cab1c91..d062fa7 100644
>> > > --- a/drivers/i2c/busses/i2c-s3c2410.c
>> > > +++ b/drivers/i2c/busses/i2c-s3c2410.c
>> > > @@ -123,7 +123,7 @@ struct s3c24xx_i2c {
>> > > struct s3c2410_platform_i2c *pdata;
>> > > int gpios[2];
>> > > struct pinctrl *pctrl;
>> > > -#ifdef CONFIG_CPU_FREQ
>> > > +#if defined(CONFIG_CPU_FREQ) && !defined(CONFIG_ARCH_EXYNOS)
>> >
>> > ...this is not a good coding practice, especially when already having
>> > multiplatform kernels in sight.
>> >
>> > The best way would be to check on which SoC we are running at runtime,
>> > but since this might need changing a lot of code, then at least I would
>> > change this from !defined(EXYNOS) to defined(S3C24XX), so it is not being
>> > compiled in when S3C24XX support is not enabled and if it's enabled then
>> > the notifier will be registered as a safe fallback that will run correctly
>> > on all platforms.
>
> Actually you can simply check for CONFIG_CPU_FREQ_S3C24XX here.
sure, this makes it more meaningful. will make the modifications and resubmit.
>
> Best regards,
> Tomasz
>
--
Shine bright,
(: Nav :)
For Exynos4 and Exynos5 SoCs from Samsung the i2c clock is based
on a fixed 66 MHz peripheral clock, and therefore is completely
independent of the cpu frequency.
Thus, registering for a CPU freq notifier is very wasteful.
This patch modifes the code such that, i2c bus registers to
cpu_freq_transition only if CONFIG_CPU_FREQ_S3C24XX is enabled.
This change should save a bunch of cpufreq transitions calls
which does not apply to exynos SoCs.
Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
---
Changes since v1:
Use CONFIG_CPU_FREQ_S3C24XX instead of (CONFIG_CPU_FREQ & !CONFIG_EXYNOS)
As commented by Tomasz
drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index cab1c91..97f14f7 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -123,7 +123,7 @@ struct s3c24xx_i2c {
struct s3c2410_platform_i2c *pdata;
int gpios[2];
struct pinctrl *pctrl;
-#ifdef CONFIG_CPU_FREQ
+#if defined(CONFIG_CPU_FREQ_S3C24XX)
struct notifier_block freq_transition;
#endif
};
@@ -843,7 +843,7 @@ static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c *i2c, unsigned int *got)
return 0;
}
-#ifdef CONFIG_CPU_FREQ
+#if defined(CONFIG_CPU_FREQ_S3C24XX)
#define freq_to_i2c(_n) container_of(_n, struct s3c24xx_i2c, freq_transition)
--
1.7.10.4
On Tue, Oct 15, 2013 at 3:23 PM, Naveen Krishna Chatradhi
<[email protected]> wrote:
> For Exynos4 and Exynos5 SoCs from Samsung the i2c clock is based
> on a fixed 66 MHz peripheral clock, and therefore is completely
> independent of the cpu frequency.
> Thus, registering for a CPU freq notifier is very wasteful.
>
> This patch modifes the code such that, i2c bus registers to
> cpu_freq_transition only if CONFIG_CPU_FREQ_S3C24XX is enabled.
>
> This change should save a bunch of cpufreq transitions calls
> which does not apply to exynos SoCs.
>
> Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
Acked-by: Kyungmin Park <[email protected]>
> ---
> Changes since v1:
> Use CONFIG_CPU_FREQ_S3C24XX instead of (CONFIG_CPU_FREQ & !CONFIG_EXYNOS)
> As commented by Tomasz
>
> drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index cab1c91..97f14f7 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -123,7 +123,7 @@ struct s3c24xx_i2c {
> struct s3c2410_platform_i2c *pdata;
> int gpios[2];
> struct pinctrl *pctrl;
> -#ifdef CONFIG_CPU_FREQ
> +#if defined(CONFIG_CPU_FREQ_S3C24XX)
> struct notifier_block freq_transition;
> #endif
> };
> @@ -843,7 +843,7 @@ static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c *i2c, unsigned int *got)
> return 0;
> }
>
> -#ifdef CONFIG_CPU_FREQ
> +#if defined(CONFIG_CPU_FREQ_S3C24XX)
>
> #define freq_to_i2c(_n) container_of(_n, struct s3c24xx_i2c, freq_transition)
>
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Hi,
On Mon, Oct 14, 2013 at 11:56 PM, Kyungmin Park <[email protected]> wrote:
> On Tue, Oct 15, 2013 at 3:23 PM, Naveen Krishna Chatradhi
> <[email protected]> wrote:
>> For Exynos4 and Exynos5 SoCs from Samsung the i2c clock is based
>> on a fixed 66 MHz peripheral clock, and therefore is completely
>> independent of the cpu frequency.
>> Thus, registering for a CPU freq notifier is very wasteful.
>>
>> This patch modifes the code such that, i2c bus registers to
>> cpu_freq_transition only if CONFIG_CPU_FREQ_S3C24XX is enabled.
>>
>> This change should save a bunch of cpufreq transitions calls
>> which does not apply to exynos SoCs.
>>
>> Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
> Acked-by: Kyungmin Park <[email protected]>
>> ---
>> Changes since v1:
>> Use CONFIG_CPU_FREQ_S3C24XX instead of (CONFIG_CPU_FREQ & !CONFIG_EXYNOS)
>> As commented by Tomasz
>>
>> drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
Looks great to me. Thank you for the suggestions Tomasz, as always.
Reviewed-by: Doug Anderson <[email protected]>
We need to come up with a solution for the CPU_FREQ stuff in
s3c2410_wdt too. We could use a similar solution but since the
CPU_FREQ stuff in s3c2410_wdt is more than just an optimization it
means that it's not good if S3C24XX is included in a multiplatform
kernel. (For the watchdog it's more than just an optimization since
every frequency transition actually pets the watchdog, making it
useless when you transition several times per second).
-Doug