* [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
@ 2016-10-06 22:01 radek
2016-10-10 9:37 ` Daniel Thompson
0 siblings, 1 reply; 12+ messages in thread
From: radek @ 2016-10-06 22:01 UTC (permalink / raw)
To: mturquette, sboyd, mcoquelin.stm32, alexandre.torgue
Cc: linux-clk, linux-arm-kernel, linux-kernel, Radoslaw Pietrzyk
From: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
---
drivers/clk/clk-stm32f4.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
index 02d6810..1fd3eac 100644
--- a/drivers/clk/clk-stm32f4.c
+++ b/drivers/clk/clk-stm32f4.c
@@ -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);
}
/*
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
2016-10-06 22:01 [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI radek
@ 2016-10-10 9:37 ` Daniel Thompson
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Thompson @ 2016-10-10 9:37 UTC (permalink / raw)
To: radek, mturquette, sboyd, mcoquelin.stm32, alexandre.torgue
Cc: linux-clk, linux-arm-kernel, linux-kernel
On 06/10/16 23:01, radek wrote:
> From: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
>
> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
> ---
> drivers/clk/clk-stm32f4.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
> index 02d6810..1fd3eac 100644
> --- a/drivers/clk/clk-stm32f4.c
> +++ b/drivers/clk/clk-stm32f4.c
> @@ -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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
@ 2016-10-10 9:37 ` Daniel Thompson
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Thompson @ 2016-10-10 9:37 UTC (permalink / raw)
To: linux-arm-kernel
On 06/10/16 23:01, radek wrote:
> From: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
>
> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
> ---
> drivers/clk/clk-stm32f4.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
> index 02d6810..1fd3eac 100644
> --- a/drivers/clk/clk-stm32f4.c
> +++ b/drivers/clk/clk-stm32f4.c
> @@ -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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
2016-10-10 9:37 ` Daniel Thompson
(?)
@ 2016-10-10 9:56 ` Radosław Pietrzyk
2016-10-10 10:31 ` Daniel Thompson
-1 siblings, 1 reply; 12+ messages in thread
From: Radosław Pietrzyk @ 2016-10-10 9:56 UTC (permalink / raw)
To: Daniel Thompson
Cc: Michael Turquette, sboyd, Maxime Coquelin, Alexandre Torgue,
linux-clk, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]
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 <daniel.thompson@linaro.org>:
> On 06/10/16 23:01, radek wrote:
>
>> From: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
>>
>> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
>> ---
>> drivers/clk/clk-stm32f4.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
>> index 02d6810..1fd3eac 100644
>> --- a/drivers/clk/clk-stm32f4.c
>> +++ b/drivers/clk/clk-stm32f4.c
>> @@ -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.
>
[-- Attachment #2: Type: text/html, Size: 3162 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
2016-10-10 9:56 ` Radosław Pietrzyk
@ 2016-10-10 10:31 ` Daniel Thompson
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Thompson @ 2016-10-10 10:31 UTC (permalink / raw)
To: Radosław Pietrzyk
Cc: Michael Turquette, sboyd, Maxime Coquelin, Alexandre Torgue,
linux-clk, linux-arm-kernel, linux-kernel
On 10/10/16 10:56, Radosław Pietrzyk wrote:
> 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 <radoslaw.pietrzyk@gmail.com>
Please add a explanation of the problem and solution in the patch
description.
> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
> ---
> drivers/clk/clk-stm32f4.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
> index 02d6810..1fd3eac 100644
> --- a/drivers/clk/clk-stm32f4.c
> +++ b/drivers/clk/clk-stm32f4.c
> @@ -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);
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);
Why rename this clock? Multiplying is a what the vco (and its control
circuits) is *for*. Tagging it "-mul" is meaningless.
Daniel.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
@ 2016-10-10 10:31 ` Daniel Thompson
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Thompson @ 2016-10-10 10:31 UTC (permalink / raw)
To: linux-arm-kernel
On 10/10/16 10:56, Rados?aw Pietrzyk wrote:
> 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 <radoslaw.pietrzyk@gmail.com>
Please add a explanation of the problem and solution in the patch
description.
> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
> ---
> drivers/clk/clk-stm32f4.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
> index 02d6810..1fd3eac 100644
> --- a/drivers/clk/clk-stm32f4.c
> +++ b/drivers/clk/clk-stm32f4.c
> @@ -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);
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);
Why rename this clock? Multiplying is a what the vco (and its control
circuits) is *for*. Tagging it "-mul" is meaningless.
Daniel.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
2016-10-10 10:31 ` Daniel Thompson
@ 2016-10-10 11:32 ` Alexandre Torgue
-1 siblings, 0 replies; 12+ messages in thread
From: Alexandre Torgue @ 2016-10-10 11:32 UTC (permalink / raw)
To: Daniel Thompson, Radosław Pietrzyk, Gabriel FERNANDEZ
Cc: Michael Turquette, sboyd, Maxime Coquelin, linux-clk,
linux-arm-kernel, linux-kernel
Hi Radoslaw,
I add Gabriel in the discussion. Gabriel is updating PLL management for
STM32F429.
Regards
Alex
On 10/10/2016 12:31 PM, Daniel Thompson wrote:
> On 10/10/16 10:56, Radosław Pietrzyk wrote:
>> 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 <radoslaw.pietrzyk@gmail.com>
>
> Please add a explanation of the problem and solution in the patch
> description.
>
>
>> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
>> ---
>> drivers/clk/clk-stm32f4.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
>> index 02d6810..1fd3eac 100644
>> --- a/drivers/clk/clk-stm32f4.c
>> +++ b/drivers/clk/clk-stm32f4.c
>> @@ -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);
>
> 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);
>
> Why rename this clock? Multiplying is a what the vco (and its control
> circuits) is *for*. Tagging it "-mul" is meaningless.
>
>
> Daniel.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
@ 2016-10-10 11:32 ` Alexandre Torgue
0 siblings, 0 replies; 12+ messages in thread
From: Alexandre Torgue @ 2016-10-10 11:32 UTC (permalink / raw)
To: linux-arm-kernel
Hi Radoslaw,
I add Gabriel in the discussion. Gabriel is updating PLL management for
STM32F429.
Regards
Alex
On 10/10/2016 12:31 PM, Daniel Thompson wrote:
> On 10/10/16 10:56, Rados?aw Pietrzyk wrote:
>> 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 <radoslaw.pietrzyk@gmail.com>
>
> Please add a explanation of the problem and solution in the patch
> description.
>
>
>> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
>> ---
>> drivers/clk/clk-stm32f4.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
>> index 02d6810..1fd3eac 100644
>> --- a/drivers/clk/clk-stm32f4.c
>> +++ b/drivers/clk/clk-stm32f4.c
>> @@ -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);
>
> 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);
>
> Why rename this clock? Multiplying is a what the vco (and its control
> circuits) is *for*. Tagging it "-mul" is meaningless.
>
>
> Daniel.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
2016-10-10 11:32 ` Alexandre Torgue
@ 2016-10-10 12:10 ` Gabriel Fernandez
-1 siblings, 0 replies; 12+ messages in thread
From: Gabriel Fernandez @ 2016-10-10 12:10 UTC (permalink / raw)
To: Alexandre Torgue, Daniel Thompson, Radosław Pietrzyk
Cc: Michael Turquette, sboyd, Maxime Coquelin, linux-clk,
linux-arm-kernel, linux-kernel, Gabriel FERNANDEZ
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
On 10/10/2016 01:32 PM, Alexandre Torgue wrote:
> Hi Radoslaw,
>
> I add Gabriel in the discussion. Gabriel is updating PLL management
> for STM32F429.
>
> Regards
> Alex
>
> On 10/10/2016 12:31 PM, Daniel Thompson wrote:
>> On 10/10/16 10:56, Radosław Pietrzyk wrote:
>>> 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 <radoslaw.pietrzyk@gmail.com>
>>
>> Please add a explanation of the problem and solution in the patch
>> description.
>>
>>
>>> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
>>> ---
>>> drivers/clk/clk-stm32f4.c | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
>>> index 02d6810..1fd3eac 100644
>>> --- a/drivers/clk/clk-stm32f4.c
>>> +++ b/drivers/clk/clk-stm32f4.c
>>> @@ -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);
>>
>> 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);
>>
>> Why rename this clock? Multiplying is a what the vco (and its control
>> circuits) is *for*. Tagging it "-mul" is meaningless.
>>
>>
>> Daniel.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
@ 2016-10-10 12:10 ` Gabriel Fernandez
0 siblings, 0 replies; 12+ messages in thread
From: Gabriel Fernandez @ 2016-10-10 12:10 UTC (permalink / raw)
To: linux-arm-kernel
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
On 10/10/2016 01:32 PM, Alexandre Torgue wrote:
> Hi Radoslaw,
>
> I add Gabriel in the discussion. Gabriel is updating PLL management
> for STM32F429.
>
> Regards
> Alex
>
> On 10/10/2016 12:31 PM, Daniel Thompson wrote:
>> On 10/10/16 10:56, Rados?aw Pietrzyk wrote:
>>> 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 <radoslaw.pietrzyk@gmail.com>
>>
>> Please add a explanation of the problem and solution in the patch
>> description.
>>
>>
>>> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
>>> ---
>>> drivers/clk/clk-stm32f4.c | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
>>> index 02d6810..1fd3eac 100644
>>> --- a/drivers/clk/clk-stm32f4.c
>>> +++ b/drivers/clk/clk-stm32f4.c
>>> @@ -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);
>>
>> 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);
>>
>> Why rename this clock? Multiplying is a what the vco (and its control
>> circuits) is *for*. Tagging it "-mul" is meaningless.
>>
>>
>> Daniel.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
2016-10-10 11:32 ` Alexandre Torgue
(?)
(?)
@ 2016-10-10 12:11 ` Radosław Pietrzyk
-1 siblings, 0 replies; 12+ messages in thread
From: Radosław Pietrzyk @ 2016-10-10 12:11 UTC (permalink / raw)
To: Alexandre Torgue
Cc: Daniel Thompson, Gabriel FERNANDEZ, Michael Turquette, sboyd,
Maxime Coquelin, linux-clk, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2883 bytes --]
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 <alexandre.torgue@st.com>:
> Hi Radoslaw,
>
> I add Gabriel in the discussion. Gabriel is updating PLL management for
> STM32F429.
>
> Regards
> Alex
>
>
> On 10/10/2016 12:31 PM, Daniel Thompson wrote:
>
>> On 10/10/16 10:56, Radosław Pietrzyk wrote:
>>
>>> 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 <radoslaw.pietrzyk@gmail.com>
>>>
>>
>> Please add a explanation of the problem and solution in the patch
>> description.
>>
>>
>> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
>>> ---
>>> drivers/clk/clk-stm32f4.c | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
>>> index 02d6810..1fd3eac 100644
>>> --- a/drivers/clk/clk-stm32f4.c
>>> +++ b/drivers/clk/clk-stm32f4.c
>>> @@ -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);
>>>
>>
>> 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);
>>>
>>
>> Why rename this clock? Multiplying is a what the vco (and its control
>> circuits) is *for*. Tagging it "-mul" is meaningless.
>>
>>
>> Daniel.
>>
>
[-- Attachment #2: Type: text/html, Size: 4291 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI
2016-10-10 12:10 ` Gabriel Fernandez
(?)
@ 2016-10-10 12:14 ` Radosław Pietrzyk
-1 siblings, 0 replies; 12+ messages in thread
From: Radosław Pietrzyk @ 2016-10-10 12:14 UTC (permalink / raw)
To: Gabriel Fernandez
Cc: Alexandre Torgue, Daniel Thompson, Michael Turquette, sboyd,
Maxime Coquelin, linux-clk, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3258 bytes --]
Yes of course, I am happy to see this.
2016-10-10 14:10 GMT+02:00 Gabriel Fernandez <gabriel.fernandez@st.com>:
> 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
>
>
> On 10/10/2016 01:32 PM, Alexandre Torgue wrote:
>
>> Hi Radoslaw,
>>
>> I add Gabriel in the discussion. Gabriel is updating PLL management for
>> STM32F429.
>>
>> Regards
>> Alex
>>
>> On 10/10/2016 12:31 PM, Daniel Thompson wrote:
>>
>>> On 10/10/16 10:56, Radosław Pietrzyk wrote:
>>>
>>>> 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 <radoslaw.pietrzyk@gmail.com>
>>>>
>>>
>>> Please add a explanation of the problem and solution in the patch
>>> description.
>>>
>>>
>>> Signed-off-by: Radoslaw Pietrzyk <radoslaw.pietrzyk@gmail.com>
>>>> ---
>>>> drivers/clk/clk-stm32f4.c | 7 ++++---
>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
>>>> index 02d6810..1fd3eac 100644
>>>> --- a/drivers/clk/clk-stm32f4.c
>>>> +++ b/drivers/clk/clk-stm32f4.c
>>>> @@ -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);
>>>>
>>>
>>> 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);
>>>>
>>>
>>> Why rename this clock? Multiplying is a what the vco (and its control
>>> circuits) is *for*. Tagging it "-mul" is meaningless.
>>>
>>>
>>> Daniel.
>>>
>>
>
[-- Attachment #2: Type: text/html, Size: 4805 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-10-10 12:14 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-06 22:01 [PATCH] Reorganize STM32 clocks in order to prepare them for PLLI2S and PLLSAI radek
2016-10-10 9:37 ` Daniel Thompson
2016-10-10 9:37 ` Daniel Thompson
2016-10-10 9:56 ` Radosław Pietrzyk
2016-10-10 10:31 ` Daniel Thompson
2016-10-10 10:31 ` Daniel Thompson
2016-10-10 11:32 ` Alexandre Torgue
2016-10-10 11:32 ` Alexandre Torgue
2016-10-10 12:10 ` Gabriel Fernandez
2016-10-10 12:10 ` Gabriel Fernandez
2016-10-10 12:14 ` Radosław Pietrzyk
2016-10-10 12:11 ` Radosław Pietrzyk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.