On Fri, Dec 08, 2023 at 09:14:32AM +0800, Chen Wang wrote:Yes, I will improve this.
+#define div_mask(width) ((1 << (width)) - 1)Looks like this should be genmask.
Would like to listen why it should be a function instead of a macro? Any experiences you can share with me?
+#define ENCODE_PLL_CTRL(fbdiv, p1, p2, refdiv) \IMO this should be a function not a macro.
+ (((fbdiv & 0xfff) << 16) | ((p2 & 0x7) << 12) | ((p1 & 0x7) << 8) | (refdiv & 0x3f))
+Why the __ prefixing of function names btw?
+static inline int __sg2042_pll_enable(struct sg2042_pll_clock *pll, bool en)
It looks like this, will check more.+{Can't you just use read_poll_timeout()?
+ unsigned int value = 0;
+ unsigned long enter;
+ struct regmap *map = pll->map;
+
+ if (en) {
+ /* wait pll lock */
+ enter = jiffies;
+ regmap_read(map, pll->offset_status, &value);
+ while (!((value >> pll->shift_status_lock) & 0x1)) {
+ regmap_read(map, pll->shift_status_lock, &value);
+ if (time_after(jiffies, enter + HZ / 10))
+ pr_warn("%s not locked\n", pll->name);
+ }
https://elixir.bootlin.com/linux/latest/source/include/linux/iopoll.h#L16
Will double check.+ /* wait pll updating */Are these not effectively the same as clk_divider's ops?
+ enter = jiffies;
+ regmap_read(map, pll->shift_status_updating, &value);
+ while (((value >> pll->shift_status_updating) & 0x1)) {
+ regmap_read(map, pll->shift_status_updating, &value);
+ if (time_after(jiffies, enter + HZ / 10))
+ pr_warn("%s still updating\n", pll->name);
+ }
+ /* enable pll */
+ regmap_read(map, pll->offset_enable, &value);
+ regmap_write(map, pll->offset_enable, value | (1 << pll->shift_enable));
+ } else {
+ /* disable pll */
+ regmap_read(map, pll->offset_enable, &value);
+ regmap_write(map, pll->offset_enable, value & (~(1 << pll->shift_enable)));
+ }
+
+ return 0;
+}
+ +static unsigned int __sg2042_get_table_div(
+ const struct clk_div_table *table,
+ unsigned int val)
+{
+ const struct clk_div_table *clkt;
+
+ for (clkt = table; clkt->div; clkt++)
+ if (clkt->val == val)
+ return clkt->div;
+ return 0;
+}
+
+static unsigned int __sg2042_get_div(
+ const struct clk_div_table *table,
+ unsigned int val,
+ unsigned long flags, u8 width)
+{
+ if (flags & CLK_DIVIDER_ONE_BASED)
+ return val;
+ if (flags & CLK_DIVIDER_POWER_OF_TWO)
+ return 1 << val;
+ if (flags & CLK_DIVIDER_MAX_AT_ZERO)
+ return val ? val : div_mask(width) + 1;
+ if (table)
+ return __sg2042_get_table_div(table, val);
+ return val + 1;
+}
Looks like this, will double check.
+IMO all of these are opportunities for GENMASK and defines.
+/*
+ * @reg_value: current register value
+ * @parent_rate: parent frequency
+ *
+ * This function is used to calculate below "rate" in equation
+ * rate = (parent_rate/REFDIV) x FBDIV/POSTDIV1/POSTDIV2
+ * = (parent_rate x FBDIV) / (REFDIV x POSTDIV1 x POSTDIV2)
+ */
+static unsigned long __sg2042_pll_recalc_rate(
+ unsigned int reg_value,
+ unsigned long parent_rate)
+{
+ unsigned int fbdiv, refdiv;
+ unsigned int postdiv1, postdiv2;
+ u64 rate, numerator, denominator;
+
+ fbdiv = (reg_value >> 16) & 0xfff;
+ refdiv = reg_value & 0x3f;
+ postdiv1 = (reg_value >> 8) & 0x7;
+ postdiv2 = (reg_value >> 12) & 0x7;
Agree, will fix this.+This is not the coding style btw.
+ numerator = parent_rate * fbdiv;
+ denominator = refdiv * postdiv1 * postdiv2;
+ do_div(numerator, denominator);
+ rate = numerator;
+
+ return rate;
+}
+
+/*
+ * Below array is the total combination lists of POSTDIV1 and POSTDIV2
+ * for example:
+ * postdiv1_2[0] = {2, 4, 8}
+ * ==> div1 = 2, div2 = 4 , div1 * div2 = 8
+ * And POSTDIV_RESULT_INDEX point to 3rd element in the array
+ */
+#define POSTDIV_RESULT_INDEX 2
+static int postdiv1_2[][3] = {
+ {2, 4, 8}, {3, 3, 9}, {2, 5, 10}, {2, 6, 12},
+ {2, 7, 14}, {3, 5, 15}, {4, 4, 16}, {3, 6, 18},
+ {4, 5, 20}, {3, 7, 21}, {4, 6, 24}, {5, 5, 25},
+ {4, 7, 28}, {5, 6, 30}, {5, 7, 35}, {6, 6, 36},
+ {6, 7, 42}, {7, 7, 49}
+};
+
+/*
+ * 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)
+{Reading this function it makes me wonder if (and I am far from the best
+ 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;
+ } else {
+ /* (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;
+ }
+ }
+ if (index < ARRAY_SIZE(postdiv1_2)) {
+ *postdiv1 = postdiv1_2[index][1];
+ *postdiv2 = postdiv1_2[index][0];
+ } else {
+ pr_debug("%s can not find in postdiv array!\n", __func__);
+ ret = -EINVAL;
+ }
+ }
+
+ return ret;
+}
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.
Agree, I will double check and try to optimize the code in next revision.
There's quite a lot in the driver and I will admit that I have not read
it all my any means (I skimmed from here onwards), but in general my
advice would be to try and reuse the generic code as much as possible.
......
Thanks,
Conor.