| Message ID | 1466587984-14105-1-git-send-email-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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.
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
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
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.
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@vger.kernel.org > Cc: linux-samsung-soc@vger.kernel.org > Cc: linux-arm-kernel@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
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); }
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@vger.kernel.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org --- drivers/cpufreq/exynos5440-cpufreq.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)