Re: [PATCH V2] clk: meson: vid-pll-div: added meson_vid_pll_div_ops support

[Jerome Brunet <jbrunet@baylibre.com>]

On Fri 07 Apr 2023 at 18:08, Yu Tu <yu.tu@amlogic.com> wrote:

> On 2023/3/22 16:41, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> On Wed 22 Mar 2023 at 15:46, Yu Tu <yu.tu@amlogic.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@amlogic.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@amlogic.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@amlogic.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.
>
> Technically you are right. I am communicating and confirming with the chip
> designer to see if the general calculation formula can be given. If not, I
> will explain why. Please give me some time.
>
> But I have to mention that the SOC, although there is this register but
> actually does not use the clock. Can we treat this as a separate patch that
> we will continue to send and explain later?
>
> This way I can continue with the other patches of S4 SOC first, and this
> clock stays the same way as the G12A first. Later, after the patch of the
> clock is corrected, it can be corrected to "ops" as required.Otherwise, we
> cannot continue other driver patches. I don't know if you agree?
>

Sure you can send your s4 series with RO ops and change to RW later on
if necessary.

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic