Hi Wens, Thanks for the review.
On Fri, 3 Jun 2016 14:53:24 +0800 Chen-Yu Tsai <w...@csie.org> wrote: > On Tue, May 31, 2016 at 3:26 PM, Jean-Francois Moine <moin...@free.fr> wrote: > > The A83T and A80 SoCs have unique settings of their PLL clocks. > > > > Signed-off-by: Jean-Francois Moine <moin...@free.fr> > > --- > > drivers/clk/sunxi-ng/ccu_ndmp.c | 247 > > ++++++++++++++++++++++++++++++++++++++++ > > drivers/clk/sunxi-ng/ccu_ndmp.h | 45 ++++++++ > > 2 files changed, 292 insertions(+) > > create mode 100644 drivers/clk/sunxi-ng/ccu_ndmp.c > > create mode 100644 drivers/clk/sunxi-ng/ccu_ndmp.h > > > > diff --git a/drivers/clk/sunxi-ng/ccu_ndmp.c > > b/drivers/clk/sunxi-ng/ccu_ndmp.c > > new file mode 100644 > > index 0000000..079b155 > > --- /dev/null > > +++ b/drivers/clk/sunxi-ng/ccu_ndmp.c > > @@ -0,0 +1,247 @@ > > +/* > > + * PLL clocks of sun8iw6 (A83T) and sun9iw1 (A80) > > + * > > + * Copyright (c) 2016 Jean-Francois Moine <moin...@free.fr> > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation; either version 2 of > > + * the License, or (at your option) any later version. > > + * > > + * The clock rates are computed as: > > + * rate = parent_rate / d1 * n / d2 / m >> p > > + */ > > + > > +#include <linux/clk-provider.h> > > +#include <linux/rational.h> > > +#include <linux/iopoll.h> > > + > > +#include "ccu_gate.h" > > +#include "ccu_ndmp.h" [snip] > > +/* d1 and d2 may be only 1 or 2 */ > > +static int ccu_ndmp_get_fact(struct ccu_ndmp *ndmp, > > + unsigned long rate, unsigned long prate, > > + int *p_n, int *p_d1, int *p_d2, int *p_m, int *p_p) > > +{ > > + int n, d1, d2, m, p, d; > > + unsigned long t; > > + > > + /* m implies only n, d1, d2 and m (pll-audio) */ > > + /* Setting d1=1 and d2=2 keeps n and m small enough > > + * with error < 5/10000 */ > > + /* As only 2 rates are used, this could be simplified: > > Best not simplify generic code to specific use cases. Well, the Allwinner's audio PLL clocks always ask for these 2 rates only. Anyway, this is just a comment. > > + * 22579200Hz => n = 32, m = 17 > > + * 24576000Hz => n = 43, m = 21 > > + */ > > + if (ndmp->m.shift) { > > shift could be 0. Testing against width is better. > Same for the other functions. Yes. I fixed this already, with some other bugs. > > + long unsigned int lun, lum; > > unsigned long, to match other places. > > > + > > + d1 = 0 + 1; > > + d2 = 1 + 1; > > + t = prate / 2; > > + rational_best_approximation(rate, t, > > + 1 << ndmp->n.width, > > + 1 << ndmp->m.width, > > + &lun, &lum); > > + if (lum == 0) > > + return -EINVAL; > > + n = lun; > > + m = lum; > > + p = 0; > > + > > + /* no d1 implies n alone (pll-cxcpux) */ > > Pretending these don't have a p factor does not make it disappear. > > > + } else if (!ndmp->d1.shift) { > > + d1 = d2 = 0 + 1; > > If you say they aren't there, why do you still need to set them. > > > + n = rate / prate; > > + m = 1; > > + p = 0; > > A note about why p isn't used would be nice. Like: > > P should only be used for rates under 288 MHz. > > from the manual. Yes. > > + > > + /* p implies only n, d1 and p (pll-videox) */ > > + } else if (ndmp->m.shift) { > > ^ p? Yes. Already fixed. > > + d2 = 0 + 1; > > + d = 2 + ndmp->p.width; > > + n = rate / (prate / (1 << d)); > > + if (n < 12) { > > + n *= 2; > > + d++; > > + } > > + while (n >= 12 * 2 && !(n & 1)) { > > + n /= 2; > > + if (--d == 0) > > + break; > > + } > > + if (d <= 1) { > > + d1 = d + 1; > > + p = 0; > > + } else { > > + d1 = 1 + 1; > > + p = d - 1; > > + } > > + m = 1; > > + > > + /* only n, d1 and d2 (other plls) */ > > + } else { > > + t = prate / 4; > > + n = rate / t; > > + if (n < 12) { > > + n *= 4; > > + d1 = d2 = 0 + 1; > > + } else if (n >= 12 * 2 && !(n & 1)) { > > + if (n >= 12 * 4 && !(n % 4)) { > > + n /= 4; > > + d1 = d2 = 0 + 1; > > + } else { > > + n /= 2; > > + d1 = 0 + 1; > > + d2 = 1 + 1; > > + } > > + } else { > > + d1 = d2 = 1 + 1; > > + } > > + if (n > (1 << ndmp->n.width)) > > + return -EINVAL; > > + m = 1; > > + p = 0; > > + } > > + > > + if (n < 12 || n > (1 << ndmp->n.width)) > > + return -EINVAL; > > + > > + *p_n = n; > > + *p_d1 = d1; > > + *p_d2 = d2; > > + *p_m = m; > > + *p_p = p; > > + > > + return 0; > > +} > > + > > +static long ccu_ndmp_round_rate(struct clk_hw *hw, unsigned long rate, > > + unsigned long *parent_rate) > > +{ > > + struct ccu_ndmp *ndmp = hw_to_ccu_ndmp(hw); > > + int n, d1, d2, m, p, ret; > > + > > + ret = ccu_ndmp_get_fact(ndmp, rate, *parent_rate, > > + &n, &d1, &d2, &m, &p); > > + if (!ret) > > + return 0; > > + > > + return *parent_rate / d1 * n / d2 / m >> p; > > A warning should be put at the top of ccu_ndmp_get_fact stating the code > should not lazily skip initializing factors it doesn't use. Or just > initialize them in this function beforehand. The contract between these > 2 functions could be made clearer. You are right. > > +} > > + > > +static int ccu_ndmp_set_rate(struct clk_hw *hw, unsigned long rate, > > + unsigned long parent_rate) > > +{ > > + struct ccu_ndmp *ndmp = hw_to_ccu_ndmp(hw); > > + unsigned long flags; > > + int n, d1, d2, m, p, ret; > > + u32 reg; > > + > > + ret = ccu_ndmp_get_fact(ndmp, rate, parent_rate, > > + &n, &d1, &d2, &m, &p); > > + if (!ret) > > + return ret; > > + if (!(ndmp->common.features & CCU_FEATURE_N0)) > > I do not remember seeing this in Maxime's original code. Did I > miss something? There are a lot of bugs to be fixed in Maxime's code, and he did not yet submit a new version. As we don't know how he will introduce the 'n' shift (start from 0 or 1 - in the A80/A83T PLL clocks, only the pll-ddr starts from 1), I added this feature flag. > > + n--; > > + > > + spin_lock_irqsave(ndmp->common.lock, flags); > > + > > + reg = readl(ndmp->common.base + ndmp->common.reg) & > > + ~((((1 << ndmp->n.width) - 1) << ndmp->n.shift) | > > + (((1 << ndmp->d1.width) - 1) << ndmp->d1.shift) | > > + (((1 << ndmp->d2.width) - 1) << ndmp->d2.shift) | > > + (((1 << ndmp->m.width) - 1) << ndmp->m.shift) | > > + (((1 << ndmp->p.width) - 1) << ndmp->p.shift)); > > + > > + writel(reg | (n << ndmp->n.shift) | > > + ((d1 - 1) << ndmp->d1.shift) | > > + ((d2 - 1) << ndmp->d2.shift) | > > + ((m - 1) << ndmp->m.shift) | > > + (p << ndmp->p.shift), > > + ndmp->common.base + ndmp->common.reg); > > + > > + spin_unlock_irqrestore(ndmp->common.lock, flags); > > + > > + WARN_ON(readl_relaxed_poll_timeout(ndmp->common.base + > > ndmp->reg_lock, > > + reg, !(reg & ndmp->lock), 50, > > 500)); > > Maybe a feature flag to test for this separate PLL lock register? All PLLs have a lock bit. But, if some clocks would have no lock, a feature flag would not be needed: testing the lock bit (ndmp->lock) would do the job (and that is the case for all the feature flags in Maxime's original code). > Regards > ChenYu > > > + > > + return 0; > > +} [snip] BTW, I also found some bugs in the A83T clocks. Do you want I submit a new version? -- Ken ar c'hentaƱ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.