References: <20230320113445.17260-1-yu.tu@amlogic.com>
 <CAFBinCAE-ihq9oeXc=GqUEHVKUYM+n_e+2_5+gDMTGQcEEhRtg@mail.gmail.com>
 <b5e647e2-4561-e6c1-016f-2c3b260916bb@amlogic.com>
User-agent: mu4e 1.8.13; emacs 28.2
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Yu Tu <yu.tu@amlogic.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc:     linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>, kelvin.zhang@amlogic.com,
        qi.duan@amlogic.com
Subject: Re: [PATCH V2] clk: meson: vid-pll-div: added meson_vid_pll_div_ops
 support
Date:   Tue, 21 Mar 2023 10:41:33 +0100
In-reply-to: <b5e647e2-4561-e6c1-016f-2c3b260916bb@amlogic.com>
Message-ID: <1jsfdy77n8.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On Tue 21 Mar 2023 at 10:29, Yu Tu <yu.tu@amlogic.com> wrote:

> Hi Martin=EF=BC=8C
> 	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=E2=80=AFPM 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-pl=
l-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().

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.

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