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

From: Yu Tu <yu.tu@amlogic.com>
To: 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>,
	Jerome Brunet <jbrunet@baylibre.com>,
	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:29:59 +0800	[thread overview]
Message-ID: <b5e647e2-4561-e6c1-016f-2c3b260916bb@amlogic.com> (raw)
In-Reply-To: <CAFBinCAE-ihq9oeXc=GqUEHVKUYM+n_e+2_5+gDMTGQcEEhRtg@mail.gmail.com>

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.

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

In fact, pll and mpll are also fixed register writes corresponding 
values. 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