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

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