diff mbox

[RFC,1/2] clk: sunxi-ng: Add the A83T and A80 PLL clocks

Message ID bbf3cefa47323660ead5ebdafe63f382fea06f39.1464680865.git.moinejf@free.fr (mailing list archive)
State Superseded
Headers show

Commit Message

Jean-Francois Moine May 31, 2016, 7:26 a.m. UTC
The A83T and A80 SoCs have unique settings of their PLL clocks.

Signed-off-by: Jean-Francois Moine <moinejf@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

Comments

Chen-Yu Tsai June 3, 2016, 6:53 a.m. UTC | #1
Hi,

On Tue, May 31, 2016 at 3:26 PM, Jean-Francois Moine <moinejf@free.fr> wrote:
> The A83T and A80 SoCs have unique settings of their PLL clocks.
>
> Signed-off-by: Jean-Francois Moine <moinejf@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@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"
> +
> +static void ccu_ndmp_disable(struct clk_hw *hw)
> +{
> +       struct ccu_ndmp *ndmp = hw_to_ccu_ndmp(hw);
> +
> +       return ccu_gate_helper_disable(&ndmp->common, ndmp->enable);
> +}
> +
> +static int ccu_ndmp_enable(struct clk_hw *hw)
> +{
> +       struct ccu_ndmp *ndmp = hw_to_ccu_ndmp(hw);
> +
> +       return ccu_gate_helper_enable(&ndmp->common, ndmp->enable);
> +}
> +
> +static int ccu_ndmp_is_enabled(struct clk_hw *hw)
> +{
> +       struct ccu_ndmp *ndmp = hw_to_ccu_ndmp(hw);
> +
> +       return ccu_gate_helper_is_enabled(&ndmp->common, ndmp->enable);
> +}
> +
> +static unsigned long ccu_ndmp_recalc_rate(struct clk_hw *hw,
> +                                       unsigned long parent_rate)
> +{
> +       struct ccu_ndmp *ndmp = hw_to_ccu_ndmp(hw);
> +       int n, d1, d2, m, p;
> +       unsigned long rate;
> +       u32 reg;
> +
> +       reg = readl(ndmp->common.base + ndmp->common.reg);
> +
> +       rate = parent_rate;
> +
> +       if (ndmp->d1.shift) {
> +               d1 = reg >> ndmp->d1.shift;
> +               d1 &= (1 << ndmp->d1.width) - 1;
> +               rate /= (d1 + 1);
> +       }
> +
> +       n = reg >> ndmp->n.shift;
> +       n &= (1 << ndmp->n.width) - 1;
> +       if (!(ndmp->common.features & CCU_FEATURE_N0))
> +               n++;
> +       rate *= n;
> +
> +       if (ndmp->d2.shift) {
> +               d2 = reg >> ndmp->d2.shift;
> +               d2 &= (1 << ndmp->d2.width) - 1;
> +               rate /= (d2 + 1);
> +       }
> +
> +       if (ndmp->m.shift) {
> +               m = reg >> ndmp->m.shift;
> +               m &= (1 << ndmp->m.width) - 1;
> +               rate /= (m + 1);
> +       }
> +
> +       if (ndmp->p.shift) {
> +               p = reg >> ndmp->p.shift;
> +               p &= (1 << ndmp->p.width) - 1;
> +               rate >>= p;
> +       }
> +
> +       return rate;
> +}
> +
> +/* 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.

> +        *      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.

> +               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.

> +
> +       /* p implies only n, d1 and p (pll-videox) */
> +       } else if (ndmp->m.shift) {

                           ^ p?

> +               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.

> +}
> +
> +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?

> +               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?

Regards
ChenYu

> +
> +       return 0;
> +}
> +
> +const struct clk_ops ccu_ndmp_ops = {
> +       .disable        = ccu_ndmp_disable,
> +       .enable         = ccu_ndmp_enable,
> +       .is_enabled     = ccu_ndmp_is_enabled,
> +
> +       .recalc_rate    = ccu_ndmp_recalc_rate,
> +       .round_rate     = ccu_ndmp_round_rate,
> +       .set_rate       = ccu_ndmp_set_rate,
> +};
> diff --git a/drivers/clk/sunxi-ng/ccu_ndmp.h b/drivers/clk/sunxi-ng/ccu_ndmp.h
> new file mode 100644
> index 0000000..bb47127
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_ndmp.h
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright (c) 2016 Jean-Francois Moine <moinejf@free.fr>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _CCU_NDMP_H_
> +#define _CCU_NDMP_H_
> +
> +#include <linux/clk-provider.h>
> +
> +#include "ccu_factor.h"
> +#include "ccu_common.h"
> +
> +struct ccu_ndmp {
> +       u32                     enable;
> +       u32                     lock;
> +       int                     reg_lock;
> +
> +       struct ccu_factor       n;
> +       struct ccu_factor       d1;
> +       struct ccu_factor       d2;
> +       struct ccu_factor       m;
> +       struct ccu_factor       p;
> +
> +       struct ccu_common       common;
> +};
> +
> +static inline struct ccu_ndmp *hw_to_ccu_ndmp(struct clk_hw *hw)
> +{
> +       struct ccu_common *common = hw_to_ccu_common(hw);
> +
> +       return container_of(common, struct ccu_ndmp, common);
> +}
> +
> +extern const struct clk_ops ccu_ndmp_ops;
> +
> +#endif /* _CCU_NDMP_H_ */
> --
> 2.8.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean-Francois Moine June 3, 2016, 11:16 a.m. UTC | #2
Hi Wens,

Thanks for the review.

On Fri, 3 Jun 2016 14:53:24 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

> On Tue, May 31, 2016 at 3:26 PM, Jean-Francois Moine <moinejf@free.fr> wrote:
> > The A83T and A80 SoCs have unique settings of their PLL clocks.
> >
> > Signed-off-by: Jean-Francois Moine <moinejf@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@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?
Chen-Yu Tsai June 5, 2016, 10 a.m. UTC | #3
Hi,

On Fri, Jun 3, 2016 at 7:16 PM, Jean-Francois Moine <moinejf@free.fr> wrote:
> Hi Wens,
>
> Thanks for the review.
>
> On Fri, 3 Jun 2016 14:53:24 +0800
> Chen-Yu Tsai <wens@csie.org> wrote:
>
>> On Tue, May 31, 2016 at 3:26 PM, Jean-Francois Moine <moinejf@free.fr> wrote:
>> > The A83T and A80 SoCs have unique settings of their PLL clocks.
>> >
>> > Signed-off-by: Jean-Francois Moine <moinejf@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@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?

I've not reviewed that patch yet. If you say you found bugs, I'll wait
for v2 to review.

Thanks
ChenYu
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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@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"
+
+static void ccu_ndmp_disable(struct clk_hw *hw)
+{
+	struct ccu_ndmp *ndmp = hw_to_ccu_ndmp(hw);
+
+	return ccu_gate_helper_disable(&ndmp->common, ndmp->enable);
+}
+
+static int ccu_ndmp_enable(struct clk_hw *hw)
+{
+	struct ccu_ndmp *ndmp = hw_to_ccu_ndmp(hw);
+
+	return ccu_gate_helper_enable(&ndmp->common, ndmp->enable);
+}
+
+static int ccu_ndmp_is_enabled(struct clk_hw *hw)
+{
+	struct ccu_ndmp *ndmp = hw_to_ccu_ndmp(hw);
+
+	return ccu_gate_helper_is_enabled(&ndmp->common, ndmp->enable);
+}
+
+static unsigned long ccu_ndmp_recalc_rate(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	struct ccu_ndmp *ndmp = hw_to_ccu_ndmp(hw);
+	int n, d1, d2, m, p;
+	unsigned long rate;
+	u32 reg;
+
+	reg = readl(ndmp->common.base + ndmp->common.reg);
+
+	rate = parent_rate;
+
+	if (ndmp->d1.shift) {
+		d1 = reg >> ndmp->d1.shift;
+		d1 &= (1 << ndmp->d1.width) - 1;
+		rate /= (d1 + 1);
+	}
+
+	n = reg >> ndmp->n.shift;
+	n &= (1 << ndmp->n.width) - 1;
+	if (!(ndmp->common.features & CCU_FEATURE_N0))
+		n++;
+	rate *= n;
+
+	if (ndmp->d2.shift) {
+		d2 = reg >> ndmp->d2.shift;
+		d2 &= (1 << ndmp->d2.width) - 1;
+		rate /= (d2 + 1);
+	}
+
+	if (ndmp->m.shift) {
+		m = reg >> ndmp->m.shift;
+		m &= (1 << ndmp->m.width) - 1;
+		rate /= (m + 1);
+	}
+
+	if (ndmp->p.shift) {
+		p = reg >> ndmp->p.shift;
+		p &= (1 << ndmp->p.width) - 1;
+		rate >>= p;
+	}
+
+	return rate;
+}
+
+/* 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:
+	 *	22579200Hz => n = 32, m = 17
+	 *	24576000Hz => n = 43, m = 21
+	 */
+	if (ndmp->m.shift) {
+		long unsigned int lun, lum;
+
+		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) */
+	} else if (!ndmp->d1.shift) {
+		d1 = d2 = 0 + 1;
+		n = rate / prate;
+		m = 1;
+		p = 0;
+
+	/* p implies only n, d1 and p (pll-videox) */
+	} else if (ndmp->m.shift) {
+		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;
+}
+
+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))
+		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));
+
+	return 0;
+}
+
+const struct clk_ops ccu_ndmp_ops = {
+	.disable	= ccu_ndmp_disable,
+	.enable		= ccu_ndmp_enable,
+	.is_enabled	= ccu_ndmp_is_enabled,
+
+	.recalc_rate	= ccu_ndmp_recalc_rate,
+	.round_rate	= ccu_ndmp_round_rate,
+	.set_rate	= ccu_ndmp_set_rate,
+};
diff --git a/drivers/clk/sunxi-ng/ccu_ndmp.h b/drivers/clk/sunxi-ng/ccu_ndmp.h
new file mode 100644
index 0000000..bb47127
--- /dev/null
+++ b/drivers/clk/sunxi-ng/ccu_ndmp.h
@@ -0,0 +1,45 @@ 
+/*
+ * Copyright (c) 2016 Jean-Francois Moine <moinejf@free.fr>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _CCU_NDMP_H_
+#define _CCU_NDMP_H_
+
+#include <linux/clk-provider.h>
+
+#include "ccu_factor.h"
+#include "ccu_common.h"
+
+struct ccu_ndmp {
+	u32			enable;
+	u32			lock;
+	int			reg_lock;
+
+	struct ccu_factor	n;
+	struct ccu_factor	d1;
+	struct ccu_factor	d2;
+	struct ccu_factor	m;
+	struct ccu_factor	p;
+
+	struct ccu_common	common;
+};
+
+static inline struct ccu_ndmp *hw_to_ccu_ndmp(struct clk_hw *hw)
+{
+	struct ccu_common *common = hw_to_ccu_common(hw);
+
+	return container_of(common, struct ccu_ndmp, common);
+}
+
+extern const struct clk_ops ccu_ndmp_ops;
+
+#endif /* _CCU_NDMP_H_ */