| Message ID | 20230320113445.17260-1-yu.tu@amlogic.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | [V2] clk: meson: vid-pll-div: added meson_vid_pll_div_ops support | expand |
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) [...] > 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 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. Best regards, Martin
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 >
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(). 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 >>
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. 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 >>> >
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. > >>> 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 >>>> >>
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? > >> >>>> 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 >>>>> >>> >
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. >> >>> >>>>> 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 >>>>>> >>>> >>
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 >>>>>>> >>>>> >>> >
diff --git a/drivers/clk/meson/vid-pll-div.c b/drivers/clk/meson/vid-pll-div.c index daff235bc763..3c9015944f24 100644 --- a/drivers/clk/meson/vid-pll-div.c +++ b/drivers/clk/meson/vid-pll-div.c @@ -89,6 +89,73 @@ static unsigned long meson_vid_pll_div_recalc_rate(struct clk_hw *hw, return DIV_ROUND_UP_ULL(parent_rate * div->multiplier, div->divider); } +static int meson_vid_pll_div_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + unsigned long parent_rate, best = 0, now = 0, rate; + unsigned long parent_rate_saved = req->best_parent_rate; + unsigned int i, best_i = 0; + + for (i = 0 ; i < VID_PLL_DIV_TABLE_SIZE; ++i) { + rate = DIV_ROUND_CLOSEST_ULL(req->rate * vid_pll_div_table[i].divider, + vid_pll_div_table[i].multiplier); + if (parent_rate_saved == rate) { + req->best_parent_rate = parent_rate_saved; + best_i = i; + break; + } + + parent_rate = clk_hw_round_rate(req->best_parent_hw, rate); + now = DIV_ROUND_CLOSEST_ULL(parent_rate * + vid_pll_div_table[i].multiplier, + vid_pll_div_table[i].divider); + if (abs(now - req->rate) < abs(best - req->rate)) { + best = now; + best_i = i; + req->best_parent_rate = parent_rate; + } + } + + req->rate = DIV_ROUND_CLOSEST_ULL(req->best_parent_rate * + vid_pll_div_table[best_i].multiplier, + vid_pll_div_table[best_i].divider); + + return 0; +} + +static int meson_vid_pll_div_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct clk_regmap *clk = to_clk_regmap(hw); + struct meson_vid_pll_div_data *pll_div = meson_vid_pll_div_data(clk); + unsigned long best = 0, now = 0; + unsigned int i, best_i = 0; + + for (i = 0 ; i < VID_PLL_DIV_TABLE_SIZE; ++i) { + now = DIV_ROUND_CLOSEST_ULL(parent_rate * vid_pll_div_table[i].multiplier, + vid_pll_div_table[i].divider); + if (now == rate) { + best_i = i; + break; + } else if (abs(now - rate) < abs(best - rate)) { + best = now; + best_i = i; + } + } + + meson_parm_write(clk->map, &pll_div->val, vid_pll_div_table[best_i].shift_val); + meson_parm_write(clk->map, &pll_div->sel, vid_pll_div_table[best_i].shift_sel); + + return 0; +} + +const struct clk_ops meson_vid_pll_div_ops = { + .recalc_rate = meson_vid_pll_div_recalc_rate, + .determine_rate = meson_vid_pll_div_determine_rate, + .set_rate = meson_vid_pll_div_set_rate, +}; +EXPORT_SYMBOL_GPL(meson_vid_pll_div_ops); + const struct clk_ops meson_vid_pll_div_ro_ops = { .recalc_rate = meson_vid_pll_div_recalc_rate, }; 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 + struct meson_vid_pll_div_data { struct parm val; struct parm sel; }; extern const struct clk_ops meson_vid_pll_div_ro_ops; +extern const struct clk_ops meson_vid_pll_div_ops; #endif /* __MESON_VID_PLL_DIV_H */
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> --- drivers/clk/meson/vid-pll-div.c | 67 +++++++++++++++++++++++++++++++++ drivers/clk/meson/vid-pll-div.h | 3 ++ 2 files changed, 70 insertions(+)