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

[Yu Tu]

On 2023/3/22 16:41, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
>
> On Wed 22 Mar 2023 at 15:46, Yu Tu <yu.tu@xxxxxxxxxxx> 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@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.
>
> 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?

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