[PATCH 1/2] cpufreq: exynos: use relaxed IO accesors

[PATCH 1/2] cpufreq: exynos: use relaxed IO accesors

[Ben Dooks]

The use of __raw IO accesors is not endian safe and should be used
sparingly. The relaxed variants should be as lightweight and also
are endian safe.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm at vger.kernel.org
Cc: linux-samsung-soc at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
---
 drivers/cpufreq/exynos5440-cpufreq.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
index c0f3373..7a5707f 100644
--- a/drivers/cpufreq/exynos5440-cpufreq.c
+++ b/drivers/cpufreq/exynos5440-cpufreq.c
@@ -155,7 +155,7 @@ static int init_div_table(void)
 		tmp = (clk_div | ema_div | (volt_id << P0_7_VDD_SHIFT)
 			| ((freq / FREQ_UNIT) << P0_7_FREQ_SHIFT));
 
-		__raw_writel(tmp, dvfs_info->base + XMU_PMU_P0_7 + 4 *
+		writel_relaxed(tmp, dvfs_info->base + XMU_PMU_P0_7 + 4 *
 						(pos - freq_tbl));
 	}
 
@@ -169,17 +169,17 @@ static void exynos_enable_dvfs(unsigned int cur_frequency)
 	struct cpufreq_frequency_table *freq_table = dvfs_info->freq_table;
 	struct cpufreq_frequency_table *pos;
 	/* Disable DVFS */
-	__raw_writel(0,	dvfs_info->base + XMU_DVFS_CTRL);
+	writel_relaxed(0,	dvfs_info->base + XMU_DVFS_CTRL);
 
 	/* Enable PSTATE Change Event */
-	tmp = __raw_readl(dvfs_info->base + XMU_PMUEVTEN);
+	tmp = readl_relaxed(dvfs_info->base + XMU_PMUEVTEN);
 	tmp |= (1 << PSTATE_CHANGED_EVTEN_SHIFT);
-	 __raw_writel(tmp, dvfs_info->base + XMU_PMUEVTEN);
+	 writel_relaxed(tmp, dvfs_info->base + XMU_PMUEVTEN);
 
 	/* Enable PSTATE Change IRQ */
-	tmp = __raw_readl(dvfs_info->base + XMU_PMUIRQEN);
+	tmp = readl_relaxed(dvfs_info->base + XMU_PMUIRQEN);
 	tmp |= (1 << PSTATE_CHANGED_IRQEN_SHIFT);
-	 __raw_writel(tmp, dvfs_info->base + XMU_PMUIRQEN);
+	 writel_relaxed(tmp, dvfs_info->base + XMU_PMUIRQEN);
 
 	/* Set initial performance index */
 	cpufreq_for_each_entry(pos, freq_table)
@@ -197,14 +197,14 @@ static void exynos_enable_dvfs(unsigned int cur_frequency)
 						cur_frequency);
 
 	for (cpu = 0; cpu < CONFIG_NR_CPUS; cpu++) {
-		tmp = __raw_readl(dvfs_info->base + XMU_C0_3_PSTATE + cpu * 4);
+		tmp = readl_relaxed(dvfs_info->base + XMU_C0_3_PSTATE + cpu * 4);
 		tmp &= ~(P_VALUE_MASK << C0_3_PSTATE_NEW_SHIFT);
 		tmp |= ((pos - freq_table) << C0_3_PSTATE_NEW_SHIFT);
-		__raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + cpu * 4);
+		writel_relaxed(tmp, dvfs_info->base + XMU_C0_3_PSTATE + cpu * 4);
 	}
 
 	/* Enable DVFS */
-	__raw_writel(1 << XMU_DVFS_CTRL_EN_SHIFT,
+	writel_relaxed(1 << XMU_DVFS_CTRL_EN_SHIFT,
 				dvfs_info->base + XMU_DVFS_CTRL);
 }
 
@@ -223,11 +223,11 @@ static int exynos_target(struct cpufreq_policy *policy, unsigned int index)
 
 	/* Set the target frequency in all C0_3_PSTATE register */
 	for_each_cpu(i, policy->cpus) {
-		tmp = __raw_readl(dvfs_info->base + XMU_C0_3_PSTATE + i * 4);
+		tmp = readl_relaxed(dvfs_info->base + XMU_C0_3_PSTATE + i * 4);
 		tmp &= ~(P_VALUE_MASK << C0_3_PSTATE_NEW_SHIFT);
 		tmp |= (index << C0_3_PSTATE_NEW_SHIFT);
 
-		__raw_writel(tmp, dvfs_info->base + XMU_C0_3_PSTATE + i * 4);
+		writel_relaxed(tmp, dvfs_info->base + XMU_C0_3_PSTATE + i * 4);
 	}
 	mutex_unlock(&cpufreq_lock);
 	return 0;
@@ -246,7 +246,7 @@ static void exynos_cpufreq_work(struct work_struct *work)
 	mutex_lock(&cpufreq_lock);
 	freqs.old = policy->cur;
 
-	cur_pstate = __raw_readl(dvfs_info->base + XMU_P_STATUS);
+	cur_pstate = readl_relaxed(dvfs_info->base + XMU_P_STATUS);
 	if (cur_pstate >> C0_3_PSTATE_VALID_SHIFT & 0x1)
 		index = (cur_pstate >> C0_3_PSTATE_CURR_SHIFT) & P_VALUE_MASK;
 	else
@@ -270,9 +270,9 @@ static irqreturn_t exynos_cpufreq_irq(int irq, void *id)
 {
 	unsigned int tmp;
 
-	tmp = __raw_readl(dvfs_info->base + XMU_PMUIRQ);
+	tmp = readl_relaxed(dvfs_info->base + XMU_PMUIRQ);
 	if (tmp >> PSTATE_CHANGED_SHIFT & 0x1) {
-		__raw_writel(tmp, dvfs_info->base + XMU_PMUIRQ);
+		writel_relaxed(tmp, dvfs_info->base + XMU_PMUIRQ);
 		disable_irq_nosync(irq);
 		schedule_work(&dvfs_info->irq_work);
 	}
-- 
2.8.1

[PATCH 1/2] cpufreq: exynos: use relaxed IO accesors

[Viresh Kumar]

On 22-06-16, 10:33, Ben Dooks wrote:
> The use of __raw IO accesors is not endian safe and should be used
> sparingly. The relaxed variants should be as lightweight and also
> are endian safe.

I am not sure if exynos's code is required to be endian safe at all or
not, but this change wouldn't harm as well.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 1/2] cpufreq: exynos: use relaxed IO accesors

[Ben Dooks]

On 22/06/16 10:35, Viresh Kumar wrote:
> On 22-06-16, 10:33, Ben Dooks wrote:
>> The use of __raw IO accesors is not endian safe and should be used
>> sparingly. The relaxed variants should be as lightweight and also
>> are endian safe.
> 
> I am not sure if exynos's code is required to be endian safe at all or
> not, but this change wouldn't harm as well.
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

It is better for any of the ARMv7 and later to do this. We've been
fixing some of the exynos and other code to be endian safe.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 1/2] cpufreq: exynos: use relaxed IO accesors

[Arnd Bergmann]

On Wednesday, June 22, 2016 10:33:03 AM CEST Ben Dooks wrote:
> The use of __raw IO accesors is not endian safe and should be used
> sparingly. The relaxed variants should be as lightweight and also
> are endian safe.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> 

Why not use the normal readl/writel() here instead of the relaxed version?

Either one should work here, but in general I'd recommend using the
non-relaxed version unless code is particularly performance sensitive.

The main argument for that is to not let people get used to using
_relaxed() all the time because it causes some very hard to debug
problems in the cases where you actually need the barriers.

	Arnd

[PATCH 1/2] cpufreq: exynos: use relaxed IO accesors

[Krzysztof Kozlowski]

On Wed, Jun 22, 2016 at 11:53 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, June 22, 2016 10:33:03 AM CEST Ben Dooks wrote:
>> The use of __raw IO accesors is not endian safe and should be used
>> sparingly. The relaxed variants should be as lightweight and also
>> are endian safe.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>
>
> Why not use the normal readl/writel() here instead of the relaxed version?
>
> Either one should work here, but in general I'd recommend using the
> non-relaxed version unless code is particularly performance sensitive.
>
> The main argument for that is to not let people get used to using
> _relaxed() all the time because it causes some very hard to debug
> problems in the cases where you actually need the barriers.

I think that would be actually different patch, not endian related.
The concurrent operations here are excluded by mutexes so this looks
safe.

Viresh,
I saw your ack. Do you prefer me to take the set through samsung-soc?

Best regards,
Krzysztof

[PATCH 1/2] cpufreq: exynos: use relaxed IO accesors

[Viresh Kumar]

On 22-06-16, 12:36, Krzysztof Kozlowski wrote:
> On Wed, Jun 22, 2016 at 11:53 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday, June 22, 2016 10:33:03 AM CEST Ben Dooks wrote:
> >> The use of __raw IO accesors is not endian safe and should be used
> >> sparingly. The relaxed variants should be as lightweight and also
> >> are endian safe.
> >>
> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> >>
> >
> > Why not use the normal readl/writel() here instead of the relaxed version?
> >
> > Either one should work here, but in general I'd recommend using the
> > non-relaxed version unless code is particularly performance sensitive.
> >
> > The main argument for that is to not let people get used to using
> > _relaxed() all the time because it causes some very hard to debug
> > problems in the cases where you actually need the barriers.
> 
> I think that would be actually different patch, not endian related.
> The concurrent operations here are excluded by mutexes so this looks
> safe.
> 
> Viresh,
> I saw your ack. Do you prefer me to take the set through samsung-soc?

Sure.

-- 
viresh

[PATCH 1/2] cpufreq: exynos: use relaxed IO accesors

[Krzysztof Kozlowski]

On Wed, Jun 22, 2016 at 11:33 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> The use of __raw IO accesors is not endian safe and should be used
> sparingly. The relaxed variants should be as lightweight and also
> are endian safe.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-pm at vger.kernel.org
> Cc: linux-samsung-soc at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> ---
>  drivers/cpufreq/exynos5440-cpufreq.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/cpufreq/exynos5440-cpufreq.c b/drivers/cpufreq/exynos5440-cpufreq.c
> index c0f3373..7a5707f 100644
> --- a/drivers/cpufreq/exynos5440-cpufreq.c
> +++ b/drivers/cpufreq/exynos5440-cpufreq.c
> @@ -155,7 +155,7 @@ static int init_div_table(void)
>                 tmp = (clk_div | ema_div | (volt_id << P0_7_VDD_SHIFT)
>                         | ((freq / FREQ_UNIT) << P0_7_FREQ_SHIFT));
>
> -               __raw_writel(tmp, dvfs_info->base + XMU_PMU_P0_7 + 4 *
> +               writel_relaxed(tmp, dvfs_info->base + XMU_PMU_P0_7 + 4 *
>                                                 (pos - freq_tbl));
>         }
>
> @@ -169,17 +169,17 @@ static void exynos_enable_dvfs(unsigned int cur_frequency)
>         struct cpufreq_frequency_table *freq_table = dvfs_info->freq_table;
>         struct cpufreq_frequency_table *pos;
>         /* Disable DVFS */
> -       __raw_writel(0, dvfs_info->base + XMU_DVFS_CTRL);
> +       writel_relaxed(0,       dvfs_info->base + XMU_DVFS_CTRL);

Please fix the whitespace.

Rest looks good but this patch appeared twice. I suspect they are the same?

Best regards,
Krzysztof