[Letux-kernel] LX16/LX20 cgu issue

[H. Nikolaus Schaller]

Hi Paul,

After reverting out drivers/clk/ingenic/cgu.c to mainline (plus a tiny patch)
the x2000 can boot, but the x1600 is broken.

Therefore I did set up this as a "good" branch and the one where x1600 is
working as bad. The diff was split into tiny changes and then I ran a git
bisect.

The result is that "patch1" is still good:

diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index 6bdf241650ed5..57ab3b0c6a3ce 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -4,6 +4,7 @@
  *
  * Copyright (c) 2013-2015 Imagination Technologies
  * Author: Paul Burton <paul.burton at mips.com>
+ * Copyright (c) 2023, 2024 Paul Boddie <paul at boddie.org.uk>
  */
  #include <linux/bitops.h>
@@ -76,6 +77,41 @@ ingenic_cgu_gate_set(struct ingenic_cgu *cgu,
  * PLL operations
  */
 +static unsigned
+ingenic_pll_recalc_rate_od(u32 ctl, u8 od_shift, u8 od_bits, u8 od_max,
+                          const s8 *od_encoding)
+{
+       unsigned od, od_enc = 0;
+
+       if (od_bits > 0) {
+               od_enc = ctl >> od_shift;
+               od_enc &= GENMASK(od_bits - 1, 0);
+       }
+
+       /*
+        * Use the encoding table if indicated. Otherwise, use the value
+        * directly, interpreting an encoded value of zero as a divider
+        * value of one.
+        */
+       if (od_encoding)
+       {
+               for (od = 0; od < od_max; od++)
+                       if (od_encoding[od] == od_enc)
+                               break;
+               od++;
+       }
+       else
+               od = od_enc ? od_enc : 1;
+
+       /* if od_max = 0, od_bits should be 0 and od is fixed to 1. */
+       if (od_max == 0)
+               BUG_ON(od_bits != 0);
+       else
+               BUG_ON(od == od_max);
+
+       return od;
+}
+
 static unsigned long
 ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 {


But "patch 2" is already broken. So let's try to review...

diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index 57ab3b0c6a3ce..ff9c18f4fb802 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -119,7 +119,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
        const struct ingenic_cgu_clk_info *clk_info = to_clk_info(ingenic_clk);
        struct ingenic_cgu *cgu = ingenic_clk->cgu;
        const struct ingenic_cgu_pll_info *pll_info;
-       unsigned m, n, od, od_enc = 0;

^^^ od_enc is now loacal in ingenic_pll_recalc_rate_od()

+       unsigned m, n, od, od1;
        bool bypass;
        u32 ctl;
 @@ -133,11 +133,6 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
        n = (ctl >> pll_info->n_shift) & GENMASK(pll_info->n_bits - 1, 0);
        n += pll_info->n_offset;
-       if (pll_info->od_bits > 0) {
-               od_enc = ctl >> pll_info->od_shift;
-               od_enc &= GENMASK(pll_info->od_bits - 1, 0);
-       }
-
        if (pll_info->bypass_bit >= 0) {
                ctl = readl(cgu->base + pll_info->bypass_reg);
 @@ -147,19 +142,14 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
                        return parent_rate;
        }
-       for (od = 0; od < pll_info->od_max; od++)
-               if (pll_info->od_encoding[od] == od_enc)
-                       break;

^^^ this code was moved into ingenic_pll_recalc_rate_od() but made depend on
    if (od_encoding), which is reasonable since there is an array access

+       od = ingenic_pll_recalc_rate_od(ctl, pll_info->od_shift, pll_info->od_bits,
+               pll_info->od_max, pll_info->od_encoding);
-       /* if od_max = 0, od_bits should be 0 and od is fixed to 1. */
-       if (pll_info->od_max == 0)
-               BUG_ON(pll_info->od_bits != 0);
-       else
-               BUG_ON(od == pll_info->od_max);

^^^ including this check.

-       od++;

^^^ hm. Is this missing in case that od_encoding == NULL?

+       od1 = ingenic_pll_recalc_rate_od(ctl, pll_info->od1_shift, pll_info->od1_bits,
+               pll_info->od1_max, pll_info->od_encoding);

^^^ this is new (for x1600) - but does it always return 1 for x2000? 

         return div_u64((u64)parent_rate * m * pll_info->rate_multiplier,
-               n * od);
+               n * od * od1);

^^^ ok, there are now two od components to be multiplied.

 }
  static void

This is so far my first analysis to try to understand the changes.

If I would have to guess, I would say that for some unknown reason
the second call to ingenic_pll_recalc_rate_od() does not return 1
for the x2000 case. Do the x2000 PLLs have initialized od1_shift, od1_bits
od1_max etc.?

Likely they all should bei 0 or NULL and ingenic_pll_recalc_rate_od()
should return 1.

A test approach would be to restore some (or all) of the old code with
private variables and printk the diffs between old and new on the x2000...

BR,
Nikolaus