Messages in this thread | | | Date | Thu, 13 Apr 2023 17:01:12 +0800 | Subject | Re: [PATCH V2] clk: meson: vid-pll-div: added meson_vid_pll_div_ops support | From | Yu Tu <> |
| |
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 >>>>>>> >>>>> >>> >
| |