Skip to contentSkip to conversation

Fixes for the pxa common clocks

PATCH New Robert Jarzmik 2 participants 10 messages Oct 6, 2014 – Nov 17, 2014

Subsystems (4)
Arm (32-bit) Arm sub-architectures Common clk PXA2xx/PXA3xx support
lore
Share & tools
lore

Robert Jarzmik

author maintainer
#0 lore raw

cover Fixes for the pxa common clocks

Hi Mike, Arnd, Haojian,

Here are all the fixes I had gathered so far on pxa clocks serie.

Amongst these :
- only patch 1/5 is a really required fix
- patches 2/5 and 3/5, after review, could go to the next merge window
- patch 4/5 should be carefully reviewed by pxa and arm people, but not merged until pxa25x support is in place.
- patch 5/5 should only be considered if ever Tomeu's changes get in.

I'm sending these to give us time to review ahead of time. And of course they have only been tested on a single pxa27x platform, but once my environment is ready, I'll pass a full build test on all PXA platform.

Cheers.

--
Robert

Robert Jarzmik (5):
clk: pxa: fix pxa27x CCCR bit usage
clk: pxa: declare init function and data __init
clk: pxa: keep clocks initialization separated per variant
arm: pxa: change clocks init sequence
clk: pxa: migrate to the clk_core API

 arch/arm/mach-pxa/generic.c  |  2 ++
 arch/arm/mach-pxa/generic.h  |  1 +
 drivers/clk/pxa/clk-pxa.c    | 51 +++++++++++++++++++++++++++-----------------
 drivers/clk/pxa/clk-pxa.h    | 11 +++++-----
 drivers/clk/pxa/clk-pxa27x.c | 16 +++++++++-----
 5 files changed, 51 insertions(+), 30 deletions(-)
Signature · 1 line
2.1.0
author maintainer
#1 lore raw

PATCH 1/5 clk: pxa: fix pxa27x CCCR bit usage

Trivial fix to check the A bit of CCCR for memory frequency calculations, where the shift of the bit index was missing, triggering a wrong calculation of memory frequency.

Signed-off-by Robert Jarzmik <[email protected]>
---
 drivers/clk/pxa/clk-pxa27x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/clk/pxa/clk-pxa27x.c +2 -2
index b345cc7..88b9fe1 100644
@@ -322,7 +322,7 @@ static unsigned long clk_pxa27x_memory_get_rate(struct clk_hw *hw,
 	unsigned long ccsr = CCSR;
 
 	osc_forced = ccsr & (1 << CCCR_CPDIS_BIT);
-	a = cccr & CCCR_A_BIT;
+	a = cccr & (1 << CCCR_A_BIT);
 	l  = ccsr & CCSR_L_MASK;
 
 	if (osc_forced || a)

@@ -341,7 +341,7 @@ static u8 clk_pxa27x_memory_get_parent(struct clk_hw *hw)
 	unsigned long ccsr = CCSR;
 
 	osc_forced = ccsr & (1 << CCCR_CPDIS_BIT);
-	a = cccr & CCCR_A_BIT;
+	a = cccr & (1 << CCCR_A_BIT);
 	if (osc_forced)
 		return PXA_MEM_13Mhz;
 	if (a)
Signature · 1 line
2.1.0
Applied
#2 lore raw

PATCH 1/5 clk: pxa: fix pxa27x CCCR bit usage

Quoting Robert Jarzmik (2014-10-06 16:07:57)

Quoted 5 lines

Trivial fix to check the A bit of CCCR for memory frequency calculations, where the shift of the bit index was missing, triggering a wrong calculation of memory frequency.

Signed-off-by Robert Jarzmik <[email protected]>

Applied to clk-fixes towards -rc6.

Regards,
Mike

Quoted 29 lines
---
 drivers/clk/pxa/clk-pxa27x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/clk/pxa/clk-pxa27x.c +2 -2
index b345cc7..88b9fe1 100644
@@ -322,7 +322,7 @@ static unsigned long clk_pxa27x_memory_get_rate(struct clk_hw *hw,
        unsigned long ccsr = CCSR;
 
        osc_forced = ccsr & (1 << CCCR_CPDIS_BIT);
-       a = cccr & CCCR_A_BIT;
+       a = cccr & (1 << CCCR_A_BIT);
        l  = ccsr & CCSR_L_MASK;
 
        if (osc_forced || a)

@@ -341,7 +341,7 @@ static u8 clk_pxa27x_memory_get_parent(struct clk_hw *hw)
        unsigned long ccsr = CCSR;
 
        osc_forced = ccsr & (1 << CCCR_CPDIS_BIT);
-       a = cccr & CCCR_A_BIT;
+       a = cccr & (1 << CCCR_A_BIT);
        if (osc_forced)
                return PXA_MEM_13Mhz;
        if (a)
Signature · 1 line
2.1.0
author maintainer
#3 lore raw

PATCH 2/5 clk: pxa: declare init function and data __init

In reply to Robert Jarzmik

As the clock descriptions are constant and only usefull at init time, mark them as such by :
- spliting clock description (desc) and clock private data (dynamic)
- mark __initdata clock descriptions

This makes all the register and descriptions of the clocks to go after kernel init phase.

Signed-off-by Robert Jarzmik <[email protected]>
---
 drivers/clk/pxa/clk-pxa.c    | 42 +++++++++++++++++++++++++++---------------
 drivers/clk/pxa/clk-pxa.h    |  8 ++++----
 drivers/clk/pxa/clk-pxa27x.c |  2 +-
 3 files changed, 32 insertions(+), 20 deletions(-)
drivers/clk/pxa/clk-pxa.c +27 -15
index ef3c053..994fd6f 100644
@@ -26,12 +26,20 @@ static struct clk_onecell_data onecell_data = {
 	.clk_num = CLK_MAX,
 };
 
-#define to_pxa_clk(_hw) container_of(_hw, struct pxa_clk_cken, hw)
+struct pxa_clk {
+	struct clk_hw hw;
+	struct clk_fixed_factor lp;
+	struct clk_fixed_factor hp;
+	struct clk_gate gate;
+	bool (*is_in_low_power)(void);
+};
+
+#define to_pxa_clk(_hw) container_of(_hw, struct pxa_clk, hw)
 
 static unsigned long cken_recalc_rate(struct clk_hw *hw,
 				      unsigned long parent_rate)
 {
-	struct pxa_clk_cken *pclk = to_pxa_clk(hw);
+	struct pxa_clk *pclk = to_pxa_clk(hw);
 	struct clk_fixed_factor *fix;
 
 	if (!pclk->is_in_low_power || pclk->is_in_low_power())

@@ -48,7 +56,7 @@ static struct clk_ops cken_rate_ops = {
 
 static u8 cken_get_parent(struct clk_hw *hw)
 {
-	struct pxa_clk_cken *pclk = to_pxa_clk(hw);
+	struct pxa_clk *pclk = to_pxa_clk(hw);
 
 	if (!pclk->is_in_low_power)
 		return 0;

@@ -69,23 +77,27 @@ void __init clkdev_pxa_register(int ckid, const char *con_id,
 		clk_register_clkdev(clk, con_id, dev_id);
 }
 
-int __init clk_pxa_cken_init(struct pxa_clk_cken *clks, int nb_clks)
+int __init clk_pxa_cken_init(const struct desc_clk_cken *clks, int nb_clks)
 {
 	int i;
-	struct pxa_clk_cken *pclk;
+	struct pxa_clk *pxa_clk;
 	struct clk *clk;
 
 	for (i = 0; i < nb_clks; i++) {
-		pclk = clks + i;
-		pclk->gate.lock = &lock;
-		clk = clk_register_composite(NULL, pclk->name,
-					     pclk->parent_names, 2,
-					     &pclk->hw, &cken_mux_ops,
-					     &pclk->hw, &cken_rate_ops,
-					     &pclk->gate.hw, &clk_gate_ops,
-					     pclk->flags);
-		clkdev_pxa_register(pclk->ckid, pclk->con_id, pclk->dev_id,
-				    clk);
+		pxa_clk = kzalloc(sizeof(*pxa_clk), GFP_KERNEL);
+		pxa_clk->is_in_low_power = clks[i].is_in_low_power;
+		pxa_clk->lp = clks[i].lp;
+		pxa_clk->hp = clks[i].hp;
+		pxa_clk->gate = clks[i].gate;
+		pxa_clk->gate.lock = &lock;
+		clk = clk_register_composite(NULL, clks[i].name,
+					     clks[i].parent_names, 2,
+					     &pxa_clk->hw, &cken_mux_ops,
+					     &pxa_clk->hw, &cken_rate_ops,
+					     &pxa_clk->gate.hw, &clk_gate_ops,
+					     clks[i].flags);
+		clkdev_pxa_register(clks[i].ckid, clks[i].con_id,
+				    clks[i].dev_id, clk);
 	}
 	return 0;
 }
drivers/clk/pxa/clk-pxa.h +4 -4
index 5fe219d..7b8d48e 100644
@@ -25,7 +25,7 @@
 	static struct clk_ops name ## _rate_ops = {		\
 		.recalc_rate = name ## _get_rate,		\
 	};							\
-	static struct clk *clk_register_ ## name(void)		\
+	static struct clk * __init clk_register_ ## name(void)	\
 	{							\
 		return clk_register_composite(NULL, clk_name,	\
 			name ## _parents,			\

@@ -40,7 +40,7 @@
 	static struct clk_ops name ## _rate_ops = {		\
 		.recalc_rate = name ## _get_rate,		\
 	};							\
-	static struct clk *clk_register_ ## name(void)		\
+	static struct clk * __init clk_register_ ## name(void)	\
 	{							\
 		return clk_register_composite(NULL, clk_name,	\
 			name ## _parents,			\

@@ -66,7 +66,7 @@
  *  |    Clock   | --- | / div_hp  |
  *  +------------+     +-----------+
  */
-struct pxa_clk_cken {
+struct desc_clk_cken {
 	struct clk_hw hw;
 	int ckid;
 	const char *name;

@@ -102,6 +102,6 @@ static int dummy_clk_set_parent(struct clk_hw *hw, u8 index)
 
 extern void clkdev_pxa_register(int ckid, const char *con_id,
 				const char *dev_id, struct clk *clk);
-extern int clk_pxa_cken_init(struct pxa_clk_cken *clks, int nb_clks);
+extern int clk_pxa_cken_init(const struct desc_clk_cken *clks, int nb_clks);
 
 #endif
drivers/clk/pxa/clk-pxa27x.c +1 -1
index 88b9fe1..32d3e02 100644
@@ -111,7 +111,7 @@ PARENTS(pxa27x_membus) = { "lcd_base", "lcd_base" };
 	PXA_CKEN_1RATE(dev_id, con_id, bit, parents,			\
 		       &CKEN, CKEN_ ## bit, CLK_IGNORE_UNUSED)
 
-static struct pxa_clk_cken pxa27x_clocks[] = {
+static struct desc_clk_cken pxa27x_clocks[] __initdata = {
 	PXA27X_PBUS_CKEN("pxa2xx-uart.0", NULL, FFUART, 2, 42, 1),
 	PXA27X_PBUS_CKEN("pxa2xx-uart.1", NULL, BTUART, 2, 42, 1),
 	PXA27X_PBUS_CKEN("pxa2xx-uart.2", NULL, STUART, 2, 42, 1),
Signature · 1 line
2.1.0
author maintainer
#4 lore raw

PATCH 3/5 clk: pxa: keep clocks initialization separated per variant

In reply to Robert Jarzmik

Have each pxa variant (pxa25x, pxa27x, pxa3xx) have its own device-tree clock initializing function, to be able to register its own specific core clocks.

Apply that change specifically to pxa27x.

Signed-off-by Robert Jarzmik <[email protected]>
---
 drivers/clk/pxa/clk-pxa.c    | 3 +--
 drivers/clk/pxa/clk-pxa.h    | 1 +
 drivers/clk/pxa/clk-pxa27x.c | 7 +++++++
 3 files changed, 9 insertions(+), 2 deletions(-)
drivers/clk/pxa/clk-pxa.c +1 -2
index 994fd6f..4e83475 100644
@@ -102,8 +102,7 @@ int __init clk_pxa_cken_init(const struct desc_clk_cken *clks, int nb_clks)
 	return 0;
 }
 
-static void __init pxa_dt_clocks_init(struct device_node *np)
+void __init clk_pxa_dt_common_init(struct device_node *np)
 {
 	of_clk_add_provider(np, of_clk_src_onecell_get, &onecell_data);
 }
-CLK_OF_DECLARE(pxa_clks, "marvell,pxa-clocks", pxa_dt_clocks_init);
drivers/clk/pxa/clk-pxa.h +1 -0
index 7b8d48e..3239654 100644
@@ -103,5 +103,6 @@ static int dummy_clk_set_parent(struct clk_hw *hw, u8 index)
 extern void clkdev_pxa_register(int ckid, const char *con_id,
 				const char *dev_id, struct clk *clk);
 extern int clk_pxa_cken_init(const struct desc_clk_cken *clks, int nb_clks);
+void clk_pxa_dt_common_init(struct device_node *np);
 
 #endif
drivers/clk/pxa/clk-pxa27x.c +7 -0
index 32d3e02..5f9b54b 100644
@@ -368,3 +368,10 @@ static int __init pxa27x_clocks_init(void)
 	return clk_pxa_cken_init(pxa27x_clocks, ARRAY_SIZE(pxa27x_clocks));
 }
 postcore_initcall(pxa27x_clocks_init);
+
+static void __init pxa27x_dt_clocks_init(struct device_node *np)
+{
+	pxa27x_clocks_init();
+	clk_pxa_dt_common_init(np);
+}
+CLK_OF_DECLARE(pxa_clks, "marvell,pxa270-clocks", pxa27x_dt_clocks_init);
Signature · 1 line
2.1.0
#5 lore raw

PATCH 3/5 clk: pxa: keep clocks initialization separated per variant

Quoting Robert Jarzmik (2014-10-06 16:07:59)

Quoted 7 lines

Have each pxa variant (pxa25x, pxa27x, pxa3xx) have its own device-tree clock initializing function, to be able to register its own specific core clocks.

Apply that change specifically to pxa27x.

Signed-off-by Robert Jarzmik <[email protected]>

Patches #2 & #3 applied to clk-next.

Regards,
Mike

Quoted 49 lines
---
 drivers/clk/pxa/clk-pxa.c    | 3 +--
 drivers/clk/pxa/clk-pxa.h    | 1 +
 drivers/clk/pxa/clk-pxa27x.c | 7 +++++++
 3 files changed, 9 insertions(+), 2 deletions(-)
drivers/clk/pxa/clk-pxa.c +1 -2
index 994fd6f..4e83475 100644
@@ -102,8 +102,7 @@ int __init clk_pxa_cken_init(const struct desc_clk_cken *clks, int nb_clks)
        return 0;
 }
 
-static void __init pxa_dt_clocks_init(struct device_node *np)
+void __init clk_pxa_dt_common_init(struct device_node *np)
 {
        of_clk_add_provider(np, of_clk_src_onecell_get, &onecell_data);
 }
-CLK_OF_DECLARE(pxa_clks, "marvell,pxa-clocks", pxa_dt_clocks_init);
drivers/clk/pxa/clk-pxa.h +1 -0
index 7b8d48e..3239654 100644
@@ -103,5 +103,6 @@ static int dummy_clk_set_parent(struct clk_hw *hw, u8 index)
 extern void clkdev_pxa_register(int ckid, const char *con_id,
                                const char *dev_id, struct clk *clk);
 extern int clk_pxa_cken_init(const struct desc_clk_cken *clks, int nb_clks);
+void clk_pxa_dt_common_init(struct device_node *np);
 
 #endif
drivers/clk/pxa/clk-pxa27x.c +7 -0
index 32d3e02..5f9b54b 100644
@@ -368,3 +368,10 @@ static int __init pxa27x_clocks_init(void)
        return clk_pxa_cken_init(pxa27x_clocks, ARRAY_SIZE(pxa27x_clocks));
 }
 postcore_initcall(pxa27x_clocks_init);
+
+static void __init pxa27x_dt_clocks_init(struct device_node *np)
+{
+       pxa27x_clocks_init();
+       clk_pxa_dt_common_init(np);
+}
+CLK_OF_DECLARE(pxa_clks, "marvell,pxa270-clocks", pxa27x_dt_clocks_init);
Signature · 1 line
2.1.0
author maintainer
#6 lore raw

PATCH 4/5 DON'T MERGE ME: arm: pxa: change clocks init sequence

In reply to Robert Jarzmik

Since pxa clocks were ported to the clock framework, an ordering issue appears between clocks and clocksource initialization. As a consequence, the pxa timer clock cannot be acquired in pxa_timer, and is disabled by clock framework because it is "unused".

The ordering issue is that in the kernel boot sequence :
start_kernel()

    ...
    time_init()
      -> pxa_timer()
        -> here the clocksource is initialized
    ...
    rest_init()
      kernel_init()
	initcalls
	  -> here the clocks are initialized

In the current sequence, the clocks are initialized way after pxa_timer, which cannot acquire the OSTIMER0 clock.

To solve this issue, the clocks initialization is moved to pxa_timer(), so that clocks are initialized before clocksource for non device-tree. For device-tree, the standard arm time_init() will take care of the ordering.

Signed-off-by Robert Jarzmik <[email protected]>
---
 arch/arm/mach-pxa/generic.c  | 2 ++
 arch/arm/mach-pxa/generic.h  | 1 +
 drivers/clk/pxa/clk-pxa27x.c | 3 +--
 3 files changed, 4 insertions(+), 2 deletions(-)
arch/arm/mach-pxa/generic.c +2 -0
index 04b013f..4ca801b 100644
@@ -63,6 +63,8 @@ EXPORT_SYMBOL(get_clock_tick_rate);
  */
 void __init pxa_timer_init(void)
 {
+	if (cpu_is_pxa27x())
+		pxa27x_clocks_init();
 	pxa_timer_nodt_init(IRQ_OST0, io_p2v(0x40a00000),
 			    get_clock_tick_rate());
 }
arch/arm/mach-pxa/generic.h +1 -0
index 8963984..7fda082 100644
@@ -14,6 +14,7 @@
 struct irq_data;
 
 extern void pxa_timer_init(void);
+extern int pxa27x_clocks_init(void);
 
 extern void __init pxa_map_io(void);
drivers/clk/pxa/clk-pxa27x.c +1 -2
index 5f9b54b..2b8343a 100644
@@ -362,12 +362,11 @@ static void __init pxa27x_base_clocks_init(void)
 	clk_register_clk_pxa27x_lcd_base();
 }
 
-static int __init pxa27x_clocks_init(void)
+int __init pxa27x_clocks_init(void)
 {
 	pxa27x_base_clocks_init();
 	return clk_pxa_cken_init(pxa27x_clocks, ARRAY_SIZE(pxa27x_clocks));
 }
-postcore_initcall(pxa27x_clocks_init);
 
 static void __init pxa27x_dt_clocks_init(struct device_node *np)
 {
Signature · 1 line
2.1.0
author maintainer
#7 lore raw

PATCH 5/5 DON'T MERGE ME: clk: pxa: migrate to the clk_core API

In reply to Robert Jarzmik

After the clock framework split structure clk and clk_core, amend the pxa clock providers for the new API.

This patch was tested on a pxa270 on top of Tomeu's patches v9.

Signed-off-by Robert Jarzmik <[email protected]>
---
 drivers/clk/pxa/clk-pxa.c | 6 +++---
 drivers/clk/pxa/clk-pxa.h | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)
drivers/clk/pxa/clk-pxa.c +3 -3
index 4e83475..8cc29a9 100644
@@ -20,7 +20,7 @@
 
 DEFINE_SPINLOCK(lock);
 
-static struct clk *pxa_clocks[CLK_MAX];
+static struct clk_core *pxa_clocks[CLK_MAX];
 static struct clk_onecell_data onecell_data = {
 	.clks = pxa_clocks,
 	.clk_num = CLK_MAX,

@@ -69,7 +69,7 @@ static struct clk_ops cken_mux_ops = {
 };
 
 void __init clkdev_pxa_register(int ckid, const char *con_id,
-				const char *dev_id, struct clk *clk)
+				const char *dev_id, struct clk_core *clk)
 {
 	if (!IS_ERR(clk) && (ckid != CLK_NONE))
 		pxa_clocks[ckid] = clk;

@@ -81,7 +81,7 @@ int __init clk_pxa_cken_init(const struct desc_clk_cken *clks, int nb_clks)
 {
 	int i;
 	struct pxa_clk *pxa_clk;
-	struct clk *clk;
+	struct clk_core *clk;
 
 	for (i = 0; i < nb_clks; i++) {
 		pxa_clk = kzalloc(sizeof(*pxa_clk), GFP_KERNEL);
drivers/clk/pxa/clk-pxa.h +4 -4
index 3239654..e2dd28d 100644
@@ -25,7 +25,7 @@
 	static struct clk_ops name ## _rate_ops = {		\
 		.recalc_rate = name ## _get_rate,		\
 	};							\
-	static struct clk * __init clk_register_ ## name(void)	\
+	static struct clk_core * __init clk_register_ ## name(void)	\
 	{							\
 		return clk_register_composite(NULL, clk_name,	\
 			name ## _parents,			\

@@ -40,7 +40,7 @@
 	static struct clk_ops name ## _rate_ops = {		\
 		.recalc_rate = name ## _get_rate,		\
 	};							\
-	static struct clk * __init clk_register_ ## name(void)	\
+	static struct clk_core * __init clk_register_ ## name(void)	\
 	{							\
 		return clk_register_composite(NULL, clk_name,	\
 			name ## _parents,			\

@@ -101,8 +101,8 @@ static int dummy_clk_set_parent(struct clk_hw *hw, u8 index)
 }
 
 extern void clkdev_pxa_register(int ckid, const char *con_id,
-				const char *dev_id, struct clk *clk);
-extern int clk_pxa_cken_init(const struct desc_clk_cken *clks, int nb_clks);
+				const char *dev_id, struct clk_core *clk);
+extern int clk_pxa_cken_init(struct desc_clk_cken *clks, int nb_clks);
 void clk_pxa_dt_common_init(struct device_node *np);
 
 #endif
Signature · 1 line
2.1.0
author maintainer
#8 lore raw

cover Fixes for the pxa common clocks

In reply to Robert Jarzmik
Quoted 11 lines · Robert Jarzmik

Hi Mike, Arnd, Haojian,

Here are all the fixes I had gathered so far on pxa clocks serie.

Amongst these :
- only patch 1/5 is a really required fix
- patches 2/5 and 3/5, after review, could go to the next merge window
- patch 4/5 should be carefully reviewed by pxa and arm people, but not merged until pxa25x support is in place.
- patch 5/5 should only be considered if ever Tomeu's changes get in.

Quoted 5 lines

I'm sending these to give us time to review ahead of time. And of course they have only been tested on a single pxa27x platform, but once my environment is ready, I'll pass a full build test on all PXA platform.

Cheers.

Hi Mike,

Did you had a chance to look at those patches (1/5 to 3/5) ?

Cheers.

--
Robert

author maintainer
#9 lore raw

cover Fixes for the pxa common clocks

Quoted 17 lines · Robert Jarzmik
Quoted 15 lines · Robert Jarzmik

Hi Mike, Arnd, Haojian,

Here are all the fixes I had gathered so far on pxa clocks serie.

Amongst these :
- only patch 1/5 is a really required fix
- patches 2/5 and 3/5, after review, could go to the next merge window
- patch 4/5 should be carefully reviewed by pxa and arm people, but not merged until pxa25x support is in place.
- patch 5/5 should only be considered if ever Tomeu's changes get in.

I'm sending these to give us time to review ahead of time. And of course they have only been tested on a single pxa27x platform, but once my environment is ready, I'll pass a full build test on all PXA platform.

Quoted 5 lines
Quoted 1 line

Cheers.

Hi Mike,

Did you had a chance to look at those patches (1/5 to 3/5) ?

Mike ?

--
Robert