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

[Yu Tu <yu.tu@amlogic.com>]

On 2023/4/11 15:02, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> 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.

Ok, I will be ready to send S4 chip other patch with VID clock RO ops first.

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