Message-ID: <1j8rfp6u0h.fsf@starbuckisacylon.baylibre.com>
Date:   Wed, 22 Mar 2023 09:41:39 +0100
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Yu Tu <yu.tu@...ogic.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc:     linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Kevin Hilman <khilman@...libre.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, kelvin.zhang@...ogic.com,
        qi.duan@...ogic.com
Subject: Re: [PATCH V2] clk: meson: vid-pll-div: added meson_vid_pll_div_ops
 support


On Wed 22 Mar 2023 at 15:46, Yu Tu <yu.tu@...ogic.com> wrote:

> On 2023/3/21 17:41, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
> Hi Jerome,
> 	Thank you for your reply.
>> On Tue 21 Mar 2023 at 10:29, Yu Tu <yu.tu@...ogic.com> wrote:
>> 
>>> Hi Martin，
>>> 	First of all, thank you for your reply.
>>>
>>> On 2023/3/20 23:35, Martin Blumenstingl wrote:
>>>> [ EXTERNAL EMAIL ]
>>>> Hello Yu Tu,
>>>> On Mon, Mar 20, 2023 at 12:35 PM Yu Tu <yu.tu@...ogic.com> wrote:
>>>>>
>>>>> Since the previous code only provides "ro_ops" for the vid_pll_div
>>>>> clock. In fact, the clock can be set. So add "ops" that can set the
>>>>> clock, especially for later chips like S4 SOC and so on.
>>>>>
>>>>> Signed-off-by: Yu Tu <yu.tu@...ogic.com>
>>>>> ---
>>>> please describe the changes you did compared to the previous version(s)
>>>
>>> I'll add it in the next version.
>>>
>>>> [...]
>>>>> diff --git a/drivers/clk/meson/vid-pll-div.h b/drivers/clk/meson/vid-pll-div.h
>>>>> index c0128e33ccf9..bbccab340910 100644
>>>>> --- a/drivers/clk/meson/vid-pll-div.h
>>>>> +++ b/drivers/clk/meson/vid-pll-div.h
>>>>> @@ -10,11 +10,14 @@
>>>>>    #include <linux/clk-provider.h>
>>>>>    #include "parm.h"
>>>>>
>>>>> +#define VID_PLL_DIV_TABLE_SIZE         14
>>>> In v1 you used ARRAY_SIZE(vid_pll_div_table) wherever this new macro
>>>> is used instead.
>>>> I think using ARRAY_SIZE is the better approach because it means the
>>>> references will update automatically if an entry is added/removed from
>>>> vid_pll_div_table
>>>
>>> I agree with you. Perhaps the key is to understand what Jerome said.
>> I asked you to describe how this divider actually works. Not remove
>> ARRAY_SIZE().
>
> OKay! I misunderstood your meaning.
>
>> This divider uses tables only because the parameters are "magic".
>> I'd like the driver to be able come up with "computed" values instead.
>> What I requested is some explanation about how this HW clock works
>> because the documentation is not very clear when it comes to this. These
>> values must come from somewhere, I'd like to understand "how".
>> This is the same as the PLL driver which can take a range and come up
>> with the different parameters, instead of using big pre-computed tables.
>> 
>>>
>>>> Also I think there's a different understanding about what Jerome
>>>> previously wrote:
>>>>> It would be nice to actually describe how this vid pll work so we can
>>>>> stop using precompute "magic" values and actually use the IP to its full
>>>>> capacity.
>>>>   From what I understand is that you interpreted this as "let's change
>>>> ARRAY_SIZE(vid_pll_div_table) to a new macro called
>>>> VID_PLL_DIV_TABLE_SIZE".
>>>> But I think what Jerome meant is: "let's get rid of vid_pll_div_table
>>>> and implement how to actually calculate the clock rate - without
>>>> hard-coding 14 possible clock settings in vid_pll_div_table". Look at
>>>> clk-mpll.c and/or clk-pll.c which allow calculating arbitrary rates
>>>> without any hard-coded tables.
>>>
>> exactly ... or at least an explanation about how it works and
>> why it is too complicated to compute the values at runtime.
>> 
>>> In fact, pll and mpll are also fixed register writes corresponding
>>> values.
>> That is not true. The pll and mpll drivers are able to compute their
>> values at runtime. Please have a look at the drivers.
>> 
>
> After consulting the engineer of the chip design, the clock is a digital
> frequency divider, and the frequency divider is verified by the sequence
> generator, which is bit0-bi15. bit16-bit17 confirms the size of the
> frequency division.

That, we already know. This is what the datasheet already give us.
It is still a bit light.

You don't set the bit randomly and check the output, do you ?

The question is how setting this bit impact the relation between
the input and output rate? IOW, from these 17bits, how do you come up
with the multiplier and divider values (and the other way around) ? 

> Whereas other PLLS and MPLLS are analog dividers so
> there are fixed formulas to calculate.
>
> So Neil had no problem implementing it this way. So now I want to know your
> advice what should I do next?

1) Neil did what he could to get compute the rate (RO) which the little
information he had. You are trying to extend the driver, keeping an
dummy approach. It is only fair that I ask you to make this a real
driver.

2) Because something has been done once, it not necessarily appropriate
to continue ... this type of argument hardly a valid reason.

I don't want to keep adding table based driver unless necessary.
So far, you have not proved this approach is really required, nor
provided the necessary information to make the calculation.

>
>>> But every SOC is different, so it makes more sense to set it
>>> outside. The VID PLL is a fixed value for all current SoCs.
>>>
>>>> Best regards,
>>>> Martin
>>>>
>> 

Powered by blists - more mailing lists