[35/46] cpufreq: pxa3: move clk register access to clk driver
diff mbox series

Message ID 20191018154201.1276638-35-arnd@arndb.de
State New
Headers show
Series
  • ARM: pxa: towards multiplatform support
Related show

Commit Message

Arnd Bergmann Oct. 18, 2019, 3:41 p.m. UTC
The driver needs some low-level register access for setting
the core and bus frequencies. These registers are owned
by the clk driver, so move the low-level access into that
driver with a slightly higher-level interface and avoid
any machine header file dependencies.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-pm@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/mach-pxa/generic.h              |  1 -
 arch/arm/mach-pxa/include/mach/generic.h |  5 --
 arch/arm/mach-pxa/pxa3xx.c               |  1 +
 drivers/clk/pxa/clk-pxa3xx.c             | 16 ++++++
 drivers/cpufreq/pxa2xx-cpufreq.c         |  3 --
 drivers/cpufreq/pxa3xx-cpufreq.c         | 64 +++++++++++++-----------
 include/linux/clk/pxa.h                  |  9 ++++
 7 files changed, 62 insertions(+), 37 deletions(-)
 delete mode 100644 arch/arm/mach-pxa/include/mach/generic.h
 create mode 100644 include/linux/clk/pxa.h

Comments

Viresh Kumar Oct. 21, 2019, 2:26 a.m. UTC | #1
On 18-10-19, 17:41, Arnd Bergmann wrote:
> The driver needs some low-level register access for setting
> the core and bus frequencies. These registers are owned
> by the clk driver, so move the low-level access into that
> driver with a slightly higher-level interface and avoid
> any machine header file dependencies.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-clk@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/cpufreq/pxa2xx-cpufreq.c         |  3 --
>  drivers/cpufreq/pxa3xx-cpufreq.c         | 64 +++++++++++++-----------

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Stephen Boyd Oct. 28, 2019, 9:23 a.m. UTC | #2
Quoting Arnd Bergmann (2019-10-18 08:41:50)
> The driver needs some low-level register access for setting
> the core and bus frequencies. These registers are owned
> by the clk driver, so move the low-level access into that
> driver with a slightly higher-level interface and avoid
> any machine header file dependencies.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-clk@vger.kernel.org
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> diff --git a/drivers/clk/pxa/clk-pxa3xx.c b/drivers/clk/pxa/clk-pxa3xx.c
> index 027b78183565..60a0db4f3790 100644
> --- a/drivers/clk/pxa/clk-pxa3xx.c
> +++ b/drivers/clk/pxa/clk-pxa3xx.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/soc/pxa/cpu.h>
>  #include <mach/smemc.h>
> +#include <linux/clk/pxa.h>
>  #include <mach/pxa3xx-regs.h>
>  
>  #include <dt-bindings/clock/pxa-clock.h>
> @@ -79,6 +80,21 @@ unsigned int pxa3xx_get_clk_frequency_khz(int info)
>         return (unsigned int)clks[0] / KHz;
>  }
>  
> +void pxa3xx_clk_update_accr(u32 disable, u32 enable, u32 xclkcfg, u32 mask)
> +{
> +       u32 accr = ACCR;
> +
> +       accr &= ~disable;
> +       accr |= enable;
> +
> +       ACCR = accr;
> +       if (xclkcfg)
> +               __asm__("mcr p14, 0, %0, c6, c0, 0\n" : : "r"(xclkcfg));

It's just a coprocessor instruction though? I don't see what this has to
do with clks that we model in the kernel? Why can't it just be put in
the cpufreq driver that calls it?

> +
> +       while ((ACSR & mask) != (accr & mask))
> +               cpu_relax();
> +}
> +
>  static unsigned long clk_pxa3xx_ac97_get_rate(struct clk_hw *hw,
>                                              unsigned long parent_rate)
>  {
Robert Jarzmik Oct. 30, 2019, 8:42 p.m. UTC | #3
Stephen Boyd <sboyd@kernel.org> writes:

> Quoting Arnd Bergmann (2019-10-18 08:41:50)
>> +void pxa3xx_clk_update_accr(u32 disable, u32 enable, u32 xclkcfg, u32 mask)
>> +{
>> +       u32 accr = ACCR;
>> +
>> +       accr &= ~disable;
>> +       accr |= enable;
>> +
>> +       ACCR = accr;
>> +       if (xclkcfg)
>> +               __asm__("mcr p14, 0, %0, c6, c0, 0\n" : : "r"(xclkcfg));
>
> It's just a coprocessor instruction though? I don't see what this has to
> do with clks that we model in the kernel? Why can't it just be put in
> the cpufreq driver that calls it?
I think you forget the "ACCR = accr", which is an MMIO register write. ACCR
stands for "Application Subsystem Clock Configuration Register", so I think Arnd
is right to put it in here.

As for the broader question you ask, about the cpufreq driver, the reason is
unfortunately "historic" : pxa3xx cpufreq driver doesn't use the clock framework
to set the bus clocks, but does it by directly accessing the registers : old
platform, old ways, etc ...

Cheers.
Stephen Boyd Nov. 8, 2019, 9:59 p.m. UTC | #4
Quoting Robert Jarzmik (2019-10-30 13:42:35)
> Stephen Boyd <sboyd@kernel.org> writes:
> 
> > Quoting Arnd Bergmann (2019-10-18 08:41:50)
> >> +void pxa3xx_clk_update_accr(u32 disable, u32 enable, u32 xclkcfg, u32 mask)
> >> +{
> >> +       u32 accr = ACCR;
> >> +
> >> +       accr &= ~disable;
> >> +       accr |= enable;
> >> +
> >> +       ACCR = accr;
> >> +       if (xclkcfg)
> >> +               __asm__("mcr p14, 0, %0, c6, c0, 0\n" : : "r"(xclkcfg));
> >
> > It's just a coprocessor instruction though? I don't see what this has to
> > do with clks that we model in the kernel? Why can't it just be put in
> > the cpufreq driver that calls it?
> I think you forget the "ACCR = accr", which is an MMIO register write. ACCR
> stands for "Application Subsystem Clock Configuration Register", so I think Arnd
> is right to put it in here.
> 
> As for the broader question you ask, about the cpufreq driver, the reason is
> unfortunately "historic" : pxa3xx cpufreq driver doesn't use the clock framework
> to set the bus clocks, but does it by directly accessing the registers : old
> platform, old ways, etc ...
> 

Ah ok. I missed the writel at the end of the function.
Stephen Boyd Nov. 8, 2019, 10:02 p.m. UTC | #5
Quoting Robert Jarzmik (2019-10-30 13:42:35)
> Stephen Boyd <sboyd@kernel.org> writes:
> 
> > Quoting Arnd Bergmann (2019-10-18 08:41:50)
> >> +void pxa3xx_clk_update_accr(u32 disable, u32 enable, u32 xclkcfg, u32 mask)
> >> +{
> >> +       u32 accr = ACCR;
> >> +
> >> +       accr &= ~disable;
> >> +       accr |= enable;
> >> +
> >> +       ACCR = accr;
> >> +       if (xclkcfg)
> >> +               __asm__("mcr p14, 0, %0, c6, c0, 0\n" : : "r"(xclkcfg));
> >
> > It's just a coprocessor instruction though? I don't see what this has to
> > do with clks that we model in the kernel? Why can't it just be put in
> > the cpufreq driver that calls it?
> I think you forget the "ACCR = accr", which is an MMIO register write. ACCR
> stands for "Application Subsystem Clock Configuration Register", so I think Arnd
> is right to put it in here.
> 
> As for the broader question you ask, about the cpufreq driver, the reason is
> unfortunately "historic" : pxa3xx cpufreq driver doesn't use the clock framework
> to set the bus clocks, but does it by directly accessing the registers : old
> platform, old ways, etc ...
> 

But there aren't any locks or anything, so presumably the cpufreq driver
can poke into the register directly and not worry? It may be a "Clock
Configuration Register" but if the clk framework isn't dealing with this
register I don't quite understand the desire to put it into the clk
driver besides it looks like something that goes near this subsystem.

Patch
diff mbox series

diff --git a/arch/arm/mach-pxa/generic.h b/arch/arm/mach-pxa/generic.h
index 2f706ef97357..487eadb0fc2a 100644
--- a/arch/arm/mach-pxa/generic.h
+++ b/arch/arm/mach-pxa/generic.h
@@ -7,7 +7,6 @@ 
  */
 
 #include <linux/reboot.h>
-#include <mach/generic.h>
 
 struct irq_data;
 
diff --git a/arch/arm/mach-pxa/include/mach/generic.h b/arch/arm/mach-pxa/include/mach/generic.h
deleted file mode 100644
index 613f6a299d0d..000000000000
--- a/arch/arm/mach-pxa/include/mach/generic.h
+++ /dev/null
@@ -1,5 +0,0 @@ 
-#ifdef CONFIG_PXA3xx
-extern unsigned	pxa3xx_get_clk_frequency_khz(int);
-#else
-#define pxa3xx_get_clk_frequency_khz(x)		(0)
-#endif
diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
index 7881888107c7..f4657f4edb3b 100644
--- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -25,6 +25,7 @@ 
 #include <linux/platform_data/i2c-pxa.h>
 #include <linux/platform_data/mmp_dma.h>
 #include <linux/soc/pxa/cpu.h>
+#include <linux/clk/pxa.h>
 
 #include <asm/mach/map.h>
 #include <asm/suspend.h>
diff --git a/drivers/clk/pxa/clk-pxa3xx.c b/drivers/clk/pxa/clk-pxa3xx.c
index 027b78183565..60a0db4f3790 100644
--- a/drivers/clk/pxa/clk-pxa3xx.c
+++ b/drivers/clk/pxa/clk-pxa3xx.c
@@ -16,6 +16,7 @@ 
 #include <linux/of.h>
 #include <linux/soc/pxa/cpu.h>
 #include <mach/smemc.h>
+#include <linux/clk/pxa.h>
 #include <mach/pxa3xx-regs.h>
 
 #include <dt-bindings/clock/pxa-clock.h>
@@ -79,6 +80,21 @@  unsigned int pxa3xx_get_clk_frequency_khz(int info)
 	return (unsigned int)clks[0] / KHz;
 }
 
+void pxa3xx_clk_update_accr(u32 disable, u32 enable, u32 xclkcfg, u32 mask)
+{
+	u32 accr = ACCR;
+
+	accr &= ~disable;
+	accr |= enable;
+
+	ACCR = accr;
+	if (xclkcfg)
+		__asm__("mcr p14, 0, %0, c6, c0, 0\n" : : "r"(xclkcfg));
+
+	while ((ACSR & mask) != (accr & mask))
+		cpu_relax();
+}
+
 static unsigned long clk_pxa3xx_ac97_get_rate(struct clk_hw *hw,
 					     unsigned long parent_rate)
 {
diff --git a/drivers/cpufreq/pxa2xx-cpufreq.c b/drivers/cpufreq/pxa2xx-cpufreq.c
index e74d36d6f78d..ed1ae061a687 100644
--- a/drivers/cpufreq/pxa2xx-cpufreq.c
+++ b/drivers/cpufreq/pxa2xx-cpufreq.c
@@ -27,9 +27,6 @@ 
 #include <linux/soc/pxa/cpu.h>
 #include <linux/io.h>
 
-#include <mach/pxa2xx-regs.h>
-#include <mach/smemc.h>
-
 #ifdef DEBUG
 static unsigned int freq_debug;
 module_param(freq_debug, uint, 0);
diff --git a/drivers/cpufreq/pxa3xx-cpufreq.c b/drivers/cpufreq/pxa3xx-cpufreq.c
index d3b398b4aa6a..4afa48d172db 100644
--- a/drivers/cpufreq/pxa3xx-cpufreq.c
+++ b/drivers/cpufreq/pxa3xx-cpufreq.c
@@ -9,12 +9,10 @@ 
 #include <linux/init.h>
 #include <linux/cpufreq.h>
 #include <linux/soc/pxa/cpu.h>
+#include <linux/clk/pxa.h>
 #include <linux/slab.h>
 #include <linux/io.h>
 
-#include <mach/generic.h>
-#include <mach/pxa3xx-regs.h>
-
 #define HSS_104M	(0)
 #define HSS_156M	(1)
 #define HSS_208M	(2)
@@ -35,6 +33,28 @@ 
 #define DMCFS_26M	(0)
 #define DMCFS_260M	(3)
 
+#define ACCR_XPDIS		(1 << 31)	/* Core PLL Output Disable */
+#define ACCR_SPDIS		(1 << 30)	/* System PLL Output Disable */
+#define ACCR_D0CS		(1 << 26)	/* D0 Mode Clock Select */
+#define ACCR_PCCE		(1 << 11)	/* Power Mode Change Clock Enable */
+#define ACCR_DDR_D0CS		(1 << 7)	/* DDR SDRAM clock frequency in D0CS (PXA31x only) */
+
+#define ACCR_SMCFS_MASK		(0x7 << 23)	/* Static Memory Controller Frequency Select */
+#define ACCR_SFLFS_MASK		(0x3 << 18)	/* Frequency Select for Internal Memory Controller */
+#define ACCR_XSPCLK_MASK	(0x3 << 16)	/* Core Frequency during Frequency Change */
+#define ACCR_HSS_MASK		(0x3 << 14)	/* System Bus-Clock Frequency Select */
+#define ACCR_DMCFS_MASK		(0x3 << 12)	/* Dynamic Memory Controller Clock Frequency Select */
+#define ACCR_XN_MASK		(0x7 << 8)	/* Core PLL Turbo-Mode-to-Run-Mode Ratio */
+#define ACCR_XL_MASK		(0x1f)		/* Core PLL Run-Mode-to-Oscillator Ratio */
+
+#define ACCR_SMCFS(x)		(((x) & 0x7) << 23)
+#define ACCR_SFLFS(x)		(((x) & 0x3) << 18)
+#define ACCR_XSPCLK(x)		(((x) & 0x3) << 16)
+#define ACCR_HSS(x)		(((x) & 0x3) << 14)
+#define ACCR_DMCFS(x)		(((x) & 0x3) << 12)
+#define ACCR_XN(x)		(((x) & 0x7) << 8)
+#define ACCR_XL(x)		((x) & 0x1f)
+
 struct pxa3xx_freq_info {
 	unsigned int cpufreq_mhz;
 	unsigned int core_xl : 5;
@@ -112,41 +132,29 @@  static int setup_freqs_table(struct cpufreq_policy *policy,
 
 static void __update_core_freq(struct pxa3xx_freq_info *info)
 {
-	uint32_t mask = ACCR_XN_MASK | ACCR_XL_MASK;
-	uint32_t accr = ACCR;
-	uint32_t xclkcfg;
-
-	accr &= ~(ACCR_XN_MASK | ACCR_XL_MASK | ACCR_XSPCLK_MASK);
-	accr |= ACCR_XN(info->core_xn) | ACCR_XL(info->core_xl);
+	u32 mask, disable, enable, xclkcfg;
 
+	mask	= ACCR_XN_MASK | ACCR_XL_MASK;
+	disable = mask | ACCR_XSPCLK_MASK;
+	enable  = ACCR_XN(info->core_xn) | ACCR_XL(info->core_xl);
 	/* No clock until core PLL is re-locked */
-	accr |= ACCR_XSPCLK(XSPCLK_NONE);
-
+	enable |= ACCR_XSPCLK(XSPCLK_NONE);
 	xclkcfg = (info->core_xn == 2) ? 0x3 : 0x2;	/* turbo bit */
 
-	ACCR = accr;
-	__asm__("mcr p14, 0, %0, c6, c0, 0\n" : : "r"(xclkcfg));
-
-	while ((ACSR & mask) != (accr & mask))
-		cpu_relax();
+	pxa3xx_clk_update_accr(disable, enable, xclkcfg, mask);
 }
 
 static void __update_bus_freq(struct pxa3xx_freq_info *info)
 {
-	uint32_t mask;
-	uint32_t accr = ACCR;
-
-	mask = ACCR_SMCFS_MASK | ACCR_SFLFS_MASK | ACCR_HSS_MASK |
-		ACCR_DMCFS_MASK;
-
-	accr &= ~mask;
-	accr |= ACCR_SMCFS(info->smcfs) | ACCR_SFLFS(info->sflfs) |
-		ACCR_HSS(info->hss) | ACCR_DMCFS(info->dmcfs);
+	u32 mask, disable, enable;
 
-	ACCR = accr;
+	mask	= ACCR_SMCFS_MASK | ACCR_SFLFS_MASK | ACCR_HSS_MASK |
+		  ACCR_DMCFS_MASK;
+	disable = mask;
+	enable	= ACCR_SMCFS(info->smcfs) | ACCR_SFLFS(info->sflfs) |
+		  ACCR_HSS(info->hss) | ACCR_DMCFS(info->dmcfs);
 
-	while ((ACSR & mask) != (accr & mask))
-		cpu_relax();
+	pxa3xx_clk_update_accr(disable, enable, 0, mask);
 }
 
 static unsigned int pxa3xx_cpufreq_get(unsigned int cpu)
diff --git a/include/linux/clk/pxa.h b/include/linux/clk/pxa.h
new file mode 100644
index 000000000000..e5516c608c99
--- /dev/null
+++ b/include/linux/clk/pxa.h
@@ -0,0 +1,9 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifdef CONFIG_PXA3xx
+extern unsigned	pxa3xx_get_clk_frequency_khz(int);
+extern void pxa3xx_clk_update_accr(u32 disable, u32 enable, u32 xclkcfg, u32 mask);
+#else
+#define pxa3xx_get_clk_frequency_khz(x)		(0)
+#define pxa3xx_clk_update_accr(disable, enable, xclkcfg, mask) do { } while (0)
+#endif