Skip to contentSkip to conversation

Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI

PATCH In review radek 4 participants 8 messages Oct 6, 2016 – Oct 10, 2016

Subsystems (2)
Common clk Arm STM32
lore
Share & tools
lore

radek

author
#0 lore raw

PATCH 1/1 Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI

From: Radoslaw Pietrzyk <[email protected]>

Signed-off-by Radoslaw Pietrzyk <[email protected]>
---
 drivers/clk/clk-stm32f4.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/clk/clk-stm32f4.c +4 -3
index 02d6810..1fd3eac 100644
@@ -245,9 +245,10 @@ static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk)
 	const char   *pllsrc = pllcfgr & BIT(22) ? hse_clk : hsi_clk;
 	unsigned long pllq   = (pllcfgr >> 24) & 0xf;
 
-	clk_register_fixed_factor(NULL, "vco", pllsrc, 0, plln, pllm);
-	clk_register_fixed_factor(NULL, "pll", "vco", 0, 1, pllp);
-	clk_register_fixed_factor(NULL, "pll48", "vco", 0, 1, pllq);
+	clk_register_fixed_factor(NULL, "vco-div", pllsrc, 0, 1, pllm);
+	clk_register_fixed_factor(NULL, "vco-mul", "vco-div", 0, plln, 1);
+	clk_register_fixed_factor(NULL, "pll", "vco-mul", 0, 1, pllp);
+	clk_register_fixed_factor(NULL, "pll48", "vco-mul", 0, 1, pllq);
 }
 
 /*
Signature · 1 line
1.9.1
#1 lore raw
Quoted 12 lines · radek

From: Radoslaw Pietrzyk <[email protected]>

Signed-off-by Radoslaw Pietrzyk <[email protected]>
---
 drivers/clk/clk-stm32f4.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/clk/clk-stm32f4.c +0 -0
index 02d6810..1fd3eac 100644

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk) · line 251

Quoted 10 lines
drivers/clk/clk-stm32f4.c +4 -3
@@ -245,9 +245,10 @@ static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk)
 	const char   *pllsrc = pllcfgr & BIT(22) ? hse_clk : hsi_clk;
 	unsigned long pllq   = (pllcfgr >> 24) & 0xf;
 
-	clk_register_fixed_factor(NULL, "vco", pllsrc, 0, plln, pllm);
-	clk_register_fixed_factor(NULL, "pll", "vco", 0, 1, pllp);
-	clk_register_fixed_factor(NULL, "pll48", "vco", 0, 1, pllq);
+	clk_register_fixed_factor(NULL, "vco-div", pllsrc, 0, 1, pllm);
+	clk_register_fixed_factor(NULL, "vco-mul", "vco-div", 0, plln, 1);
+	clk_register_fixed_factor(NULL, "pll", "vco-mul", 0, 1, pllp);
+	clk_register_fixed_factor(NULL, "pll48", "vco-mul", 0, 1, pllq);

I'm struggling to marry this up to the clock tree diagram for the F4-series (and there's no patch description to help me).

I can see the value of naming the "/M" pre-division separately (and agree that its hard to find it a good name for this clock in the datasheet). However I am struggling to work out why we'd want to rename the vco output.

For me the names for the multiplies clock within each pll emerges fairly cleanly from the datasheet (PLL -> vco, PLLI2S -> vcoi2s, PLLSAI -> vcosai). What does the '-mul' add?

Daniel.

author
#2 lore raw

Hi,
all plls have the same clock parent which is after a main divider. Currently the divider and multiplier are connected together within vco clock and therefore there is no chance to reuse the divider and clearly state where the conncetion "really" is. We can arrange all of them separately but than the divider will be hidden for all of them separately.

2016-10-10 11:37 GMT+02:00 Daniel Thompson <[email protected]>:

Quoted 42 lines · radek
Quoted 16 lines

From: Radoslaw Pietrzyk <[email protected]>

Signed-off-by Radoslaw Pietrzyk <[email protected]>
---
 drivers/clk/clk-stm32f4.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/clk/clk-stm32f4.c +0 -0
index 02d6810..1fd3eac 100644
@@ -245,9 +245,10 @@ static void stm32f4_rcc_register_pll(const char
 *hse_clk, const char *hsi_clk)
        const char   *pllsrc = pllcfgr & BIT(22) ? hse_clk : hsi_clk;
        unsigned long pllq   = (pllcfgr >> 24) & 0xf;

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char · line 254

Quoted 10 lines
drivers/clk/clk-stm32f4.c +4 -3
 
-       clk_register_fixed_factor(NULL, "vco", pllsrc, 0, plln, pllm);
-       clk_register_fixed_factor(NULL, "pll", "vco", 0, 1, pllp);
-       clk_register_fixed_factor(NULL, "pll48", "vco", 0, 1, pllq);
+       clk_register_fixed_factor(NULL, "vco-div", pllsrc, 0, 1, pllm);
+       clk_register_fixed_factor(NULL, "vco-mul", "vco-div", 0, plln,
 1);
+       clk_register_fixed_factor(NULL, "pll", "vco-mul", 0, 1, pllp);
+       clk_register_fixed_factor(NULL, "pll48", "vco-mul", 0, 1, pllq);

I'm struggling to marry this up to the clock tree diagram for the F4-series (and there's no patch description to help me).

I can see the value of naming the "/M" pre-division separately (and agree that its hard to find it a good name for this clock in the datasheet). However I am struggling to work out why we'd want to rename the vco output.

For me the names for the multiplies clock within each pll emerges fairly cleanly from the datasheet (PLL -> vco, PLLI2S -> vcoi2s, PLLSAI -> vcosai). What does the '-mul' add?

Daniel.

#3 lore raw
Quoted 6 lines · Rados?aw Pietrzyk

Hi,
all plls have the same clock parent which is after a main divider. Currently the divider and multiplier are connected together within vco clock and therefore there is no chance to reuse the divider and clearly state where the conncetion "really" is. We can arrange all of them separately but than the divider will be hidden for all of them separately.

Quoting my last mail "I can see the value of naming the "/M" pre-division separately". In other words I agree with the idea of the patch.

To more explicitly state my review comments...

Quoted 1 line

From: Radoslaw Pietrzyk <[email protected]>

Please add a explanation of the problem and solution in the patch description.

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char

Quoted 10 lines
Signed-off-by Radoslaw Pietrzyk <[email protected]>
---
 drivers/clk/clk-stm32f4.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/clk/clk-stm32f4.c +0 -0
index 02d6810..1fd3eac 100644
@@ -245,9 +245,10 @@ static void stm32f4_rcc_register_pll(const char

*hse_clk, const char *hsi_clk)

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk) · line 248

Quoted 7 lines
drivers/clk/clk-stm32f4.c +1 -3
 	const char   *pllsrc = pllcfgr & BIT(22) ? hse_clk : hsi_clk;
 	unsigned long pllq   = (pllcfgr >> 24) & 0xf;
 
-	clk_register_fixed_factor(NULL, "vco", pllsrc, 0, plln, pllm);
-	clk_register_fixed_factor(NULL, "pll", "vco", 0, 1, pllp);
-	clk_register_fixed_factor(NULL, "pll48", "vco", 0, 1, pllq);
+	clk_register_fixed_factor(NULL, "vco-div", pllsrc, 0, 1, pllm);

This strikes me as a bad name for a clock that is shared by all three PLLs (the vco being an internal component of the PLL) however since the clock is not named in the datasheet we are forced to invent a name [I suspect that's why I gave up trying to name it when I wrote the driver originally ;-) ].

Perhaps "pllin-prediv"?

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk) · line 249

Quoted 1 line
drivers/clk/clk-stm32f4.c +1 -0
+	clk_register_fixed_factor(NULL, "vco-mul", "vco-div", 0, plln, 1);

Why rename this clock? Multiplying is a what the vco (and its control circuits) is *for*. Tagging it "-mul" is meaningless.

Daniel.

#4 lore raw

Hi Radoslaw,

I add Gabriel in the discussion. Gabriel is updating PLL management for STM32F429.

Regards
Alex

Quoted 56 lines · Daniel Thompson
Quoted 7 lines · Rados?aw Pietrzyk

Hi,
all plls have the same clock parent which is after a main divider. Currently the divider and multiplier are connected together within vco clock and therefore there is no chance to reuse the divider and clearly state where the conncetion "really" is. We can arrange all of them separately but than the divider will be hidden for all of them separately.

Quoting my last mail "I can see the value of naming the "/M" pre-division separately". In other words I agree with the idea of the patch.

To more explicitly state my review comments...

Quoted 1 line

From: Radoslaw Pietrzyk <[email protected]>

Please add a explanation of the problem and solution in the patch description.

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char

Quoted 10 lines
Signed-off-by Radoslaw Pietrzyk <[email protected]>
---
 drivers/clk/clk-stm32f4.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/clk/clk-stm32f4.c +0 -0
index 02d6810..1fd3eac 100644
@@ -245,9 +245,10 @@ static void stm32f4_rcc_register_pll(const char

*hse_clk, const char *hsi_clk)

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk) · line 248

Quoted 7 lines
drivers/clk/clk-stm32f4.c +1 -3
     const char   *pllsrc = pllcfgr & BIT(22) ? hse_clk : hsi_clk;
     unsigned long pllq   = (pllcfgr >> 24) & 0xf;
 
-    clk_register_fixed_factor(NULL, "vco", pllsrc, 0, plln, pllm);
-    clk_register_fixed_factor(NULL, "pll", "vco", 0, 1, pllp);
-    clk_register_fixed_factor(NULL, "pll48", "vco", 0, 1, pllq);
+    clk_register_fixed_factor(NULL, "vco-div", pllsrc, 0, 1, pllm);

This strikes me as a bad name for a clock that is shared by all three PLLs (the vco being an internal component of the PLL) however since the clock is not named in the datasheet we are forced to invent a name [I suspect that's why I gave up trying to name it when I wrote the driver originally ;-) ].

Perhaps "pllin-prediv"?

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk) · line 249

Quoted 1 line
drivers/clk/clk-stm32f4.c +1 -0
+    clk_register_fixed_factor(NULL, "vco-mul", "vco-div", 0, plln, 1);

Why rename this clock? Multiplying is a what the vco (and its control circuits) is *for*. Tagging it "-mul" is meaningless.

Daniel.

#5 lore raw

Hi Rados?aw,

Yes i m nearly ready to push a patch-set to manage LCD-TFT clock.

In my patch-set i introduced PLLI2S and PLLSAI in generic way, and offer the possibility to change the vco frequency (in order to cover all frequencies for any LCD).

And then, the vco is no longer a fixed factor.

This patch is just a fix or do you planned to upstream PLLI2S and PLLSAI ?

If you are ok I can send my patch-set ?

Best Regards

Gabriel

Quoted 65 lines · Alexandre Torgue

Hi Radoslaw,

I add Gabriel in the discussion. Gabriel is updating PLL management for STM32F429.

Regards
Alex

Quoted 56 lines · Daniel Thompson
Quoted 7 lines · Rados?aw Pietrzyk

Hi,
all plls have the same clock parent which is after a main divider. Currently the divider and multiplier are connected together within vco clock and therefore there is no chance to reuse the divider and clearly state where the conncetion "really" is. We can arrange all of them separately but than the divider will be hidden for all of them separately.

Quoting my last mail "I can see the value of naming the "/M" pre-division separately". In other words I agree with the idea of the patch.

To more explicitly state my review comments...

Quoted 1 line

From: Radoslaw Pietrzyk <[email protected]>

Please add a explanation of the problem and solution in the patch description.

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char

Quoted 10 lines
Signed-off-by Radoslaw Pietrzyk <[email protected]>
---
 drivers/clk/clk-stm32f4.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/clk/clk-stm32f4.c +0 -0
index 02d6810..1fd3eac 100644
@@ -245,9 +245,10 @@ static void stm32f4_rcc_register_pll(const char

*hse_clk, const char *hsi_clk)

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk) · line 248

Quoted 7 lines
drivers/clk/clk-stm32f4.c +1 -3
     const char   *pllsrc = pllcfgr & BIT(22) ? hse_clk : hsi_clk;
     unsigned long pllq   = (pllcfgr >> 24) & 0xf;
 
-    clk_register_fixed_factor(NULL, "vco", pllsrc, 0, plln, pllm);
-    clk_register_fixed_factor(NULL, "pll", "vco", 0, 1, pllp);
-    clk_register_fixed_factor(NULL, "pll48", "vco", 0, 1, pllq);
+    clk_register_fixed_factor(NULL, "vco-div", pllsrc, 0, 1, pllm);

This strikes me as a bad name for a clock that is shared by all three PLLs (the vco being an internal component of the PLL) however since the clock is not named in the datasheet we are forced to invent a name [I suspect that's why I gave up trying to name it when I wrote the driver originally ;-) ].

Perhaps "pllin-prediv"?

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk) · line 249

Quoted 1 line
drivers/clk/clk-stm32f4.c +1 -0
+    clk_register_fixed_factor(NULL, "vco-mul", "vco-div", 0, plln, 1);

Why rename this clock? Multiplying is a what the vco (and its control circuits) is *for*. Tagging it "-mul" is meaningless.

Daniel.

author
#6 lore raw

Yes of course, I am happy to see this.

2016-10-10 14:10 GMT+02:00 Gabriel Fernandez <[email protected]>:

Quoted 97 lines

Hi Radosław,

Yes i m nearly ready to push a patch-set to manage LCD-TFT clock.

In my patch-set i introduced PLLI2S and PLLSAI in generic way, and offer the possibility to change the vco frequency (in order to cover all frequencies for any LCD).

And then, the vco is no longer a fixed factor.

This patch is just a fix or do you planned to upstream PLLI2S and PLLSAI ?

If you are ok I can send my patch-set ?

Best Regards

Gabriel

Quoted 75 lines · Alexandre Torgue

Hi Radoslaw,

I add Gabriel in the discussion. Gabriel is updating PLL management for STM32F429.

Regards
Alex

Quoted 64 lines · Daniel Thompson
Quoted 8 lines · Radosław Pietrzyk

Hi,
all plls have the same clock parent which is after a main divider. Currently the divider and multiplier are connected together within vco clock and therefore there is no chance to reuse the divider and clearly state where the conncetion "really" is. We can arrange all of them separately but than the divider will be hidden for all of them separately.

Quoting my last mail "I can see the value of naming the "/M" pre-division separately". In other words I agree with the idea of the patch.

To more explicitly state my review comments...

From: Radoslaw Pietrzyk <[email protected]>

Quoted 1 line

Please add a explanation of the problem and solution in the patch description.

Signed-off-by Radoslaw Pietrzyk <[email protected]>

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char · line 245

Quoted 10 lines
---
 drivers/clk/clk-stm32f4.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/clk/clk-stm32f4.c +0 -0
index 02d6810..1fd3eac 100644
@@ -245,9 +245,10 @@ static void stm32f4_rcc_register_pll(const char

*hse_clk, const char *hsi_clk)

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk) · line 253

Quoted 8 lines
drivers/clk/clk-stm32f4.c +1 -3
     const char   *pllsrc = pllcfgr & BIT(22) ? hse_clk : hsi_clk;
     unsigned long pllq   = (pllcfgr >> 24) & 0xf;
 
-    clk_register_fixed_factor(NULL, "vco", pllsrc, 0, plln, pllm);
-    clk_register_fixed_factor(NULL, "pll", "vco", 0, 1, pllp);
-    clk_register_fixed_factor(NULL, "pll48", "vco", 0, 1, pllq);
+    clk_register_fixed_factor(NULL, "vco-div", pllsrc, 0, 1, pllm);

This strikes me as a bad name for a clock that is shared by all three PLLs (the vco being an internal component of the PLL) however since the clock is not named in the datasheet we are forced to invent a name [I suspect that's why I gave up trying to name it when I wrote the driver originally ;-) ].

Perhaps "pllin-prediv"?

+ clk_register_fixed_factor(NULL, "vco-mul", "vco-div", 0, plln, 1);

Quoted 1 line

Why rename this clock? Multiplying is a what the vco (and its control circuits) is *for*. Tagging it "-mul" is meaningless.

Daniel.

author
#7 lore raw
In reply to Alexandre Torgue

I agree that the name is not suitable at all but I just wanted to leave it as it is to make things clear but generally this divider should be combined with the HSI/HSE muxer to create one general clock for all PLLs. If Gabriel is doing this than this patch should be discarded.

2016-10-10 13:32 GMT+02:00 Alexandre Torgue <[email protected]>:

Quoted 76 lines

Hi Radoslaw,

I add Gabriel in the discussion. Gabriel is updating PLL management for STM32F429.

Regards
Alex

Quoted 64 lines · Daniel Thompson
Quoted 8 lines · Radosław Pietrzyk

Hi,
all plls have the same clock parent which is after a main divider. Currently the divider and multiplier are connected together within vco clock and therefore there is no chance to reuse the divider and clearly state where the conncetion "really" is. We can arrange all of them separately but than the divider will be hidden for all of them separately.

Quoting my last mail "I can see the value of naming the "/M" pre-division separately". In other words I agree with the idea of the patch.

To more explicitly state my review comments...

From: Radoslaw Pietrzyk <[email protected]>

Quoted 1 line

Please add a explanation of the problem and solution in the patch description.

Signed-off-by Radoslaw Pietrzyk <[email protected]>

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char · line 245

Quoted 10 lines
---
 drivers/clk/clk-stm32f4.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/clk/clk-stm32f4.c +0 -0
index 02d6810..1fd3eac 100644
@@ -245,9 +245,10 @@ static void stm32f4_rcc_register_pll(const char

*hse_clk, const char *hsi_clk)

drivers/clk/clk-stm32f4.c static void stm32f4_rcc_register_pll(const char *hse_clk, const char *hsi_clk) · line 253

Quoted 8 lines
drivers/clk/clk-stm32f4.c +1 -3
     const char   *pllsrc = pllcfgr & BIT(22) ? hse_clk : hsi_clk;
     unsigned long pllq   = (pllcfgr >> 24) & 0xf;
 
-    clk_register_fixed_factor(NULL, "vco", pllsrc, 0, plln, pllm);
-    clk_register_fixed_factor(NULL, "pll", "vco", 0, 1, pllp);
-    clk_register_fixed_factor(NULL, "pll48", "vco", 0, 1, pllq);
+    clk_register_fixed_factor(NULL, "vco-div", pllsrc, 0, 1, pllm);

This strikes me as a bad name for a clock that is shared by all three PLLs (the vco being an internal component of the PLL) however since the clock is not named in the datasheet we are forced to invent a name [I suspect that's why I gave up trying to name it when I wrote the driver originally ;-) ].

Perhaps "pllin-prediv"?

+ clk_register_fixed_factor(NULL, "vco-mul", "vco-div", 0, plln, 1);

Quoted 1 line

Why rename this clock? Multiplying is a what the vco (and its control circuits) is *for*. Tagging it "-mul" is meaningless.

Daniel.