On 2023/12/14 8:15, Stephen Boyd wrote:
Quoting Conor Dooley (2023-12-12 00:37:39)
On Tue, Dec 12, 2023 at 10:22:28AM +0800, Chen Wang wrote:
On 2023/12/9 0:47, Conor Dooley wrote:
On Fri, Dec 08, 2023 at 09:14:32AM +0800, Chen Wang wrote:
+#define ENCODE_PLL_CTRL(fbdiv, p1, p2, refdiv) \
+ (((fbdiv & 0xfff) << 16) | ((p2 & 0x7) << 12) | ((p1 & 0x7) << 8) | (refdiv & 0x3f))
IMO this should be a function not a macro.
Would like to listen why it should be a function instead of a macro? Any
experiences you can share with me?
Readability. A function, which could be inlined allows you to break this
up and make it easier to read.
+/*
+ * Based on input rate/prate/fbdiv/refdiv, look up the postdiv1_2 table
+ * to get the closest postdiiv combination.
+ * @rate: FOUTPOSTDIV
+ * @prate: parent rate, i.e. FREF
+ * @fbdiv: FBDIV
+ * @refdiv: REFDIV
+ * @postdiv1: POSTDIV1, output
+ * @postdiv2: POSTDIV2, output
+ * See TRM:
+ * FOUTPOSTDIV = FREF * FBDIV / REFDIV / (POSTDIV1 * POSTDIV2)
+ * So we get following formula to get POSTDIV1 and POSTDIV2:
+ * POSTDIV = (prate/REFDIV) x FBDIV/rate
+ * above POSTDIV = POSTDIV1*POSTDIV2
+ */
+static int __sg2042_pll_get_postdiv_1_2(
+ unsigned long rate,
+ unsigned long prate,
+ unsigned int fbdiv,
+ unsigned int refdiv,
+ unsigned int *postdiv1,
+ unsigned int *postdiv2)
This is not the coding style btw.
Agree, will fix this.
+{
+ int index = 0;
+ int ret = 0;
+ u64 tmp0;
+
+ /* prate/REFDIV and result save to tmp0 */
+ tmp0 = prate;
+ do_div(tmp0, refdiv);
+
+ /* ((prate/REFDIV) x FBDIV) and result save to tmp0 */
+ tmp0 *= fbdiv;
+
+ /* ((prate/REFDIV) x FBDIV)/rate and result save to tmp0 */
+ do_div(tmp0, rate);
+
+ /* tmp0 is POSTDIV1*POSTDIV2, now we calculate div1 and div2 value */
+ if (tmp0 <= 7) {
+ /* (div1 * div2) <= 7, no need to use array search */
+ *postdiv1 = tmp0;
+ *postdiv2 = 1;
why not return 0 here?
+ } else {
And then de-indent this?
+ /* (div1 * div2) > 7, use array search */
+ for (index = 0; index < ARRAY_SIZE(postdiv1_2); index++) {
+ if (tmp0 > postdiv1_2[index][POSTDIV_RESULT_INDEX]) {
+ continue;
+ } else {
+ /* found it */
+ break;
This can also return?
+ }
+ }
+ if (index < ARRAY_SIZE(postdiv1_2)) {
And this condition can be removed.
+ *postdiv1 = postdiv1_2[index][1];
+ *postdiv2 = postdiv1_2[index][0];
+ } else {
This can be the default after the loop.
+ pr_debug("%s can not find in postdiv array!\n", __func__);
+ ret = -EINVAL;
/* tmp0 is POSTDIV1*POSTDIV2, now we calculate div1 and div2 value */
if (tmp0 <= 7) {
/* (div1 * div2) <= 7, no need to use array search */
*postdiv1 = tmp0;
*postdiv2 = 1;
return 0;
}
/* (div1 * div2) > 7, use array search */
for (index = 0; index < ARRAY_SIZE(postdiv1_2); index++) {
if (tmp0 > postdiv1_2[index][POSTDIV_RESULT_INDEX]) {
continue;
} else {
*postdiv1 = postdiv1_2[index][1];
*postdiv2 = postdiv1_2[index][0];
return 0;
}
}
pr_debug("%s can not find in postdiv array!\n", __func__);
return -EINVAL;
Thanks, Stephen, I will improve this.
Reading this function it makes me wonder if (and I am far from the best
person to comment, someone like Stephen is vastly more qualified) you
should model this as several "stages", each implemented by the
"standard" clocks - like clk_divider etc. The code here is quite
complicated IMO as it seems to be trying to implement several stages of
division in one go.
The objective of __sg2042_pll_get_postdiv_1_2() is straightforward: based on
the formula defined by the TRM, with input rate/prate/fbdiv/refdiv, we can
get the possiblle combination of POSTDIV1 and POSTDIV2 by looking up the
table of postdiv1_2. We will later use it to setup the clock register.
Though the codes looks a bit complicated, but accually it is calculate with
the formula : POSTDIV = (prate/REFDIV) x FBDIV/rate, I just separate it into
several steps to make it easy to understand, I have listed the formula in
the comment on top of the function.
I understand what you are doing, I did something similar myself
previously. My suggestion/question was about using the "standard" types
of clock that the core provides to represent as many of the clocks in
this driver as is feasible.
I would not twist the code to conform with the basic clk types. If
possible it would be good to use the helpers for these things, but I
wouldn't split up a clk that is a complex divider with multiple stages
of division into the basic types just to make it fit. I say this because
every clk takes more effort to maintain in the clk tree, it has a name,
pointers, etc. If you can keep that self contained and logically it is
really one clk, then go for it.
Thanks, I will double check if we can reuse "standard" types of clock as
much as possible, or just keep current way.
