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

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

[Daniel Thompson]

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.

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

[Daniel Thompson]

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.

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

[Alexandre Torgue]

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.

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

[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.