[PATCH RFC 1/2] clk: sunxi-ng: Add the A83T and A80 PLL clocks
Jean-Francois Moine
moinejf at free.fr
Fri Jun 3 04:16:41 PDT 2016
Hi Wens,
Thanks for the review.
On Fri, 3 Jun 2016 14:53:24 +0800
Chen-Yu Tsai <wens at csie.org> wrote:
> On Tue, May 31, 2016 at 3:26 PM, Jean-Francois Moine <moinejf at free.fr> wrote:
> > The A83T and A80 SoCs have unique settings of their PLL clocks.
> >
> > Signed-off-by: Jean-Francois Moine <moinejf at 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 <moinejf at 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/
More information about the linux-arm-kernel
mailing list