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

[Yu Tu]

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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > ---
> > > 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. 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?

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