[02/16] clk: sunxi-ng: Add common infrastructure

Commit Message

Maxime Ripard May 8, 2016, 8:01 p.m. UTC

Start our new clock infrastructure by adding the registration code, common
structure and common code.

Signed-off-by: Maxime Ripard <[e-mail]>
---
 drivers/clk/Makefile              |   1 +
 drivers/clk/sunxi-ng/Makefile     |   2 +
 drivers/clk/sunxi-ng/ccu_common.c | 108 ++++++++++++++++++++++++++++++++++++++
 drivers/clk/sunxi-ng/ccu_common.h |  74 ++++++++++++++++++++++++++
 drivers/clk/sunxi-ng/ccu_factor.h |  15 ++++++
 drivers/clk/sunxi-ng/ccu_mux.h    |  20 +++++++
 drivers/clk/sunxi-ng/ccu_reset.c  |  55 +++++++++++++++++++
 drivers/clk/sunxi-ng/ccu_reset.h  |  40 ++++++++++++++
 8 files changed, 315 insertions(+)
 create mode 100644 drivers/clk/sunxi-ng/Makefile
 create mode 100644 drivers/clk/sunxi-ng/ccu_common.c
 create mode 100644 drivers/clk/sunxi-ng/ccu_common.h
 create mode 100644 drivers/clk/sunxi-ng/ccu_factor.h
 create mode 100644 drivers/clk/sunxi-ng/ccu_mux.h
 create mode 100644 drivers/clk/sunxi-ng/ccu_reset.c
 create mode 100644 drivers/clk/sunxi-ng/ccu_reset.h

Comments

Chen-Yu Tsai May 9, 2016, 10:01 a.m. UTC | #1

On Mon, May 9, 2016 at 4:01 AM, Maxime Ripard
<[e-mail]> wrote:
> Start our new clock infrastructure by adding the registration code, common
> structure and common code.
>
> Signed-off-by: Maxime Ripard <[e-mail]>
> ---
>  drivers/clk/Makefile              |   1 +
>  drivers/clk/sunxi-ng/Makefile     |   2 +
>  drivers/clk/sunxi-ng/ccu_common.c | 108 ++++++++++++++++++++++++++++++++++++++
>  drivers/clk/sunxi-ng/ccu_common.h |  74 ++++++++++++++++++++++++++
>  drivers/clk/sunxi-ng/ccu_factor.h |  15 ++++++
>  drivers/clk/sunxi-ng/ccu_mux.h    |  20 +++++++
>  drivers/clk/sunxi-ng/ccu_reset.c  |  55 +++++++++++++++++++
>  drivers/clk/sunxi-ng/ccu_reset.h  |  40 ++++++++++++++
>  8 files changed, 315 insertions(+)
>  create mode 100644 drivers/clk/sunxi-ng/Makefile
>  create mode 100644 drivers/clk/sunxi-ng/ccu_common.c
>  create mode 100644 drivers/clk/sunxi-ng/ccu_common.h
>  create mode 100644 drivers/clk/sunxi-ng/ccu_factor.h
>  create mode 100644 drivers/clk/sunxi-ng/ccu_mux.h
>  create mode 100644 drivers/clk/sunxi-ng/ccu_reset.c
>  create mode 100644 drivers/clk/sunxi-ng/ccu_reset.h
>
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 4ef71a13ab37..83a93cd9e21d 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -78,6 +78,7 @@ obj-$(CONFIG_ARCH_SOCFPGA)            += socfpga/
>  obj-$(CONFIG_PLAT_SPEAR)               += spear/
>  obj-$(CONFIG_ARCH_STI)                 += st/
>  obj-$(CONFIG_ARCH_SUNXI)               += sunxi/
> +obj-$(CONFIG_ARCH_SUNXI)               += sunxi-ng/
>  obj-$(CONFIG_ARCH_TEGRA)               += tegra/
>  obj-y                                  += ti/
>  obj-$(CONFIG_ARCH_U8500)               += ux500/
> diff --git a/drivers/clk/sunxi-ng/Makefile b/drivers/clk/sunxi-ng/Makefile
> new file mode 100644
> index 000000000000..bd3461b0f38c
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/Makefile
> @@ -0,0 +1,2 @@
> +obj-y += ccu_common.o
> +obj-y += ccu_reset.o
> diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
> new file mode 100644
> index 000000000000..1d9242566fbd
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_common.c
> @@ -0,0 +1,108 @@
> +/*
> + * Copyright 2016 Maxime Ripard
> + *
> + * Maxime Ripard <[e-mail]>
> + *
> + * 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.
> + *
> + * 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.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/iopoll.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#include "ccu_common.h"
> +#include "ccu_reset.h"
> +
> +static DEFINE_SPINLOCK(ccu_lock);
> +
> +void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock)
> +{
> +       u32 reg;
> +
> +       if (!(common->features & CCU_FEATURE_LOCK))
> +               return;
> +
> +       WARN_ON(readl_relaxed_poll_timeout(common->base + common->reg, reg,
> +                                          !(reg & lock), 0, 500));

                                    no delay between reads? ^

> +}
> +
> +int sunxi_ccu_probe(struct device_node *node,
> +                   const struct sunxi_ccu_desc *desc)
> +{
> +       struct ccu_common **cclks = desc->clks;
> +       size_t num_clks = desc->num_clks;
> +       struct clk_onecell_data *data;
> +       struct ccu_reset *reset;
> +       struct clk **clks;
> +       void __iomem *reg;
> +       int i, ret;
> +
> +       reg = of_iomap(node, 0);

Why not of_io_request_and_map?

> +       if (IS_ERR(reg)) {

And of_iomap returns NULL on error. This is for of_io_request_and_map.

> +               pr_err("%s: Could not map the clock registers\n",
> +                      of_node_full_name(node));
> +               return PTR_ERR(reg);
> +       }
> +
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       clks = kcalloc(num_clks, sizeof(struct clk *), GFP_KERNEL);
> +       if (!clks)
> +               return -ENOMEM;
> +
> +       data->clks = clks;
> +       data->clk_num = num_clks;
> +
> +       for (i = 0; i < num_clks; i++) {
> +               struct ccu_common *cclk = cclks[i];
> +               struct clk *clk;
> +
> +               if (!cclk) {
> +                       cclk = ERR_PTR(-ENOENT);

This seems redundant, unless you intended to use it elsewhere?

> +                       continue;
> +               }
> +
> +               cclk->base = reg;
> +               cclk->lock = &ccu_lock;
> +
> +               clk = clk_register(NULL, &cclk->hw);
> +               if (IS_ERR(clk))
> +                       continue;
> +
> +               clks[i] = clk;
> +       }
> +
> +       ret = of_clk_add_provider(node, of_clk_src_onecell_get, data);
> +       if (ret)
> +               goto err_clk_unreg;
> +
> +       reset = kzalloc(sizeof(*reset), GFP_KERNEL);
> +       reset->rcdev.of_node = node;
> +       reset->rcdev.ops = &ccu_reset_ops;
> +       reset->rcdev.owner = THIS_MODULE;
> +       reset->rcdev.nr_resets = desc->num_resets;
> +       reset->base = reg;
> +       reset->lock = &ccu_lock;
> +       reset->reset_map = desc->resets;
> +
> +       ret = reset_controller_register(&reset->rcdev);
> +       if (ret)
> +               goto err_of_clk_unreg;
> +
> +       return 0;
> +
> +err_of_clk_unreg:
> +err_clk_unreg:
> +       return ret;
> +}
> diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
> new file mode 100644
> index 000000000000..e8b477fcd320
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_common.h
> @@ -0,0 +1,74 @@
> +/*
> + * Copyright (c) 2016 Maxime Ripard. All rights reserved.
> + *
> + * 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 _COMMON_H_
> +#define _COMMON_H_
> +
> +#include <linux/compiler.h>
> +#include <linux/clk-provider.h>
> +
> +#define CCU_FEATURE_GATE               BIT(0)
> +#define CCU_FEATURE_LOCK               BIT(1)

*_PLL_LOCK would be clearer that this implements a PLL lock indicator.
Or maybe a comment.

> +#define CCU_FEATURE_FRACTIONAL         BIT(2)
> +#define CCU_FEATURE_VARIABLE_PREDIV    BIT(3)
> +#define CCU_FEATURE_FIXED_PREDIV       BIT(4)
> +#define CCU_FEATURE_FIXED_POSTDIV      BIT(5)
> +
> +struct device_node;
> +
> +#define SUNXI_HW_INIT(_name, _parent, _ops, _flags)                    \
> +       &(struct clk_init_data) {                                       \
> +               .flags          = _flags,                               \
> +               .name           = _name,                                \
> +               .parent_names   = (const char *[]) { _parent },         \
> +               .num_parents    = 1,                                    \
> +               .ops            = _ops,                                 \
> +       }
> +
> +#define SUNXI_HW_INIT_PARENTS(_name, _parents, _ops, _flags)           \
> +       &(struct clk_init_data) {                                       \
> +               .flags          = _flags,                               \
> +               .name           = _name,                                \
> +               .parent_names   = _parents,                             \
> +               .num_parents    = ARRAY_SIZE(_parents),                 \
> +               .ops            = _ops,                                 \
> +       }
> +
> +struct ccu_common {
> +       void __iomem    *base;
> +       unsigned long   reg;

This seems quite large, considering the address space of the CCU,
and you using u16 or u32 for the same thing on the reset control side.

> +
> +       unsigned long   features;
> +       spinlock_t      *lock;
> +       struct clk_hw   hw;
> +};
> +
> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct ccu_common, hw);
> +}
> +
> +struct sunxi_ccu_desc {
> +       struct ccu_common       **clks;
> +       unsigned long           num_clks;
> +
> +       struct ccu_reset_map    *resets;
> +       unsigned long           num_resets;
> +};
> +
> +void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock);
> +
> +int sunxi_ccu_probe(struct device_node *node,
> +                   const struct sunxi_ccu_desc *desc);
> +
> +#endif /* _COMMON_H_ */
> diff --git a/drivers/clk/sunxi-ng/ccu_factor.h b/drivers/clk/sunxi-ng/ccu_factor.h
> new file mode 100644
> index 000000000000..e7cc564aaea0
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_factor.h
> @@ -0,0 +1,15 @@
> +#ifndef _CLK_FACTOR_H_
> +#define _CLK_FACTOR_H_
> +
> +struct ccu_factor {
> +       u8      shift;
> +       u8      width;
> +};
> +
> +#define SUNXI_CLK_FACTOR(_shift, _width)       \
> +       {                                       \
> +               .shift  = _shift,               \
> +               .width  = _width,               \
> +       }
> +
> +#endif /* _CLK_FACTOR_H_ */
> diff --git a/drivers/clk/sunxi-ng/ccu_mux.h b/drivers/clk/sunxi-ng/ccu_mux.h
> new file mode 100644
> index 000000000000..17cedad4e433
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_mux.h

As far as I can tell there are no users of this file within this patch or the
following patches before the mux clock support one. It'd be easier to understand
if this part was moved to the mux clock patch.

> @@ -0,0 +1,20 @@
> +#ifndef _CCU_MUX_H_
> +#define _CCU_MUX_H_
> +
> +#include "common.h"
> +
> +struct ccu_mux_internal {
> +       u8      shift;
> +       u8      width;
> +
> +       u8      *map;

I assume map is a table?

> +};
> +
> +#define SUNXI_CLK_MUX(_shift, _width, _map)    \
> +       {                                       \
> +               .map    = _map,                 \
> +               .shift  = _shift,               \
> +               .width  = _width,               \
> +       }
> +
> +#endif /* _CCU_MUX_H_ */
> diff --git a/drivers/clk/sunxi-ng/ccu_reset.c b/drivers/clk/sunxi-ng/ccu_reset.c
> new file mode 100644
> index 000000000000..6c31d48783a7
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_reset.c
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright (C) 2016 Maxime Ripard
> + * Maxime Ripard <[e-mail]>
> + *
> + * 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.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/reset-controller.h>
> +
> +#include "ccu_reset.h"
> +
> +static int ccu_reset_assert(struct reset_controller_dev *rcdev,
> +                           unsigned long id)
> +{
> +       struct ccu_reset *ccu = rcdev_to_ccu_reset(rcdev);
> +       const struct ccu_reset_map *map = &ccu->reset_map[id];
> +       unsigned long flags;
> +       u32 reg;
> +
> +       spin_lock_irqsave(ccu->lock, flags);
> +
> +       reg = readl(ccu->base + map->reg);
> +       writel(reg & ~map->bit, ccu->base + map->reg);
> +
> +       spin_unlock_irqrestore(ccu->lock, flags);
> +
> +       return 0;
> +}
> +
> +static int ccu_reset_deassert(struct reset_controller_dev *rcdev,
> +                             unsigned long id)
> +{
> +       struct ccu_reset *ccu = rcdev_to_ccu_reset(rcdev);
> +       const struct ccu_reset_map *map = &ccu->reset_map[id];
> +       unsigned long flags;
> +       u32 reg;
> +
> +       spin_lock_irqsave(ccu->lock, flags);
> +
> +       reg = readl(ccu->base + map->reg);
> +       writel(reg | map->bit, ccu->base + map->reg);
> +
> +       spin_unlock_irqrestore(ccu->lock, flags);
> +
> +       return 0;
> +}
> +
> +const struct reset_control_ops ccu_reset_ops = {
> +       .assert         = ccu_reset_assert,
> +       .deassert       = ccu_reset_deassert,
> +};
> diff --git a/drivers/clk/sunxi-ng/ccu_reset.h b/drivers/clk/sunxi-ng/ccu_reset.h
> new file mode 100644
> index 000000000000..36a4679210bd
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_reset.h
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright (c) 2016 Maxime Ripard. All rights reserved.
> + *
> + * 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_RESET_H_
> +#define _CCU_RESET_H_
> +
> +#include <linux/reset-controller.h>
> +
> +struct ccu_reset_map {
> +       u16     reg;
> +       u32     bit;
> +};
> +
> +
> +struct ccu_reset {
> +       void __iomem                    *base;
> +       struct ccu_reset_map            *reset_map;
> +       spinlock_t                      *lock;
> +
> +       struct reset_controller_dev     rcdev;
> +};
> +
> +static inline struct ccu_reset *rcdev_to_ccu_reset(struct reset_controller_dev *rcdev)
> +{
> +       return container_of(rcdev, struct ccu_reset, rcdev);
> +}
> +
> +extern const struct reset_control_ops ccu_reset_ops;
> +
> +#endif /* _CCU_RESET_H_ */

The reset control code looks good.


Regards
ChenYu

> --
> 2.8.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Maxime Ripard May 15, 2016, 6:31 p.m. UTC | #2

Hi,

On Mon, May 09, 2016 at 06:01:45PM +0800, Chen-Yu Tsai wrote:
> On Mon, May 9, 2016 at 4:01 AM, Maxime Ripard
> <[e-mail]> wrote:
> > Start our new clock infrastructure by adding the registration code, common
> > structure and common code.
> >
> > Signed-off-by: Maxime Ripard <[e-mail]>
> > ---
> >  drivers/clk/Makefile              |   1 +
> >  drivers/clk/sunxi-ng/Makefile     |   2 +
> >  drivers/clk/sunxi-ng/ccu_common.c | 108 ++++++++++++++++++++++++++++++++++++++
> >  drivers/clk/sunxi-ng/ccu_common.h |  74 ++++++++++++++++++++++++++
> >  drivers/clk/sunxi-ng/ccu_factor.h |  15 ++++++
> >  drivers/clk/sunxi-ng/ccu_mux.h    |  20 +++++++
> >  drivers/clk/sunxi-ng/ccu_reset.c  |  55 +++++++++++++++++++
> >  drivers/clk/sunxi-ng/ccu_reset.h  |  40 ++++++++++++++
> >  8 files changed, 315 insertions(+)
> >  create mode 100644 drivers/clk/sunxi-ng/Makefile
> >  create mode 100644 drivers/clk/sunxi-ng/ccu_common.c
> >  create mode 100644 drivers/clk/sunxi-ng/ccu_common.h
> >  create mode 100644 drivers/clk/sunxi-ng/ccu_factor.h
> >  create mode 100644 drivers/clk/sunxi-ng/ccu_mux.h
> >  create mode 100644 drivers/clk/sunxi-ng/ccu_reset.c
> >  create mode 100644 drivers/clk/sunxi-ng/ccu_reset.h
> >
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index 4ef71a13ab37..83a93cd9e21d 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -78,6 +78,7 @@ obj-$(CONFIG_ARCH_SOCFPGA)            += socfpga/
> >  obj-$(CONFIG_PLAT_SPEAR)               += spear/
> >  obj-$(CONFIG_ARCH_STI)                 += st/
> >  obj-$(CONFIG_ARCH_SUNXI)               += sunxi/
> > +obj-$(CONFIG_ARCH_SUNXI)               += sunxi-ng/
> >  obj-$(CONFIG_ARCH_TEGRA)               += tegra/
> >  obj-y                                  += ti/
> >  obj-$(CONFIG_ARCH_U8500)               += ux500/
> > diff --git a/drivers/clk/sunxi-ng/Makefile b/drivers/clk/sunxi-ng/Makefile
> > new file mode 100644
> > index 000000000000..bd3461b0f38c
> > --- /dev/null
> > +++ b/drivers/clk/sunxi-ng/Makefile
> > @@ -0,0 +1,2 @@
> > +obj-y += ccu_common.o
> > +obj-y += ccu_reset.o
> > diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
> > new file mode 100644
> > index 000000000000..1d9242566fbd
> > --- /dev/null
> > +++ b/drivers/clk/sunxi-ng/ccu_common.c
> > @@ -0,0 +1,108 @@
> > +/*
> > + * Copyright 2016 Maxime Ripard
> > + *
> > + * Maxime Ripard <[e-mail]>
> > + *
> > + * 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.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/of_address.h>
> > +#include <linux/slab.h>
> > +
> > +#include "ccu_common.h"
> > +#include "ccu_reset.h"
> > +
> > +static DEFINE_SPINLOCK(ccu_lock);
> > +
> > +void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock)
> > +{
> > +       u32 reg;
> > +
> > +       if (!(common->features & CCU_FEATURE_LOCK))
> > +               return;
> > +
> > +       WARN_ON(readl_relaxed_poll_timeout(common->base + common->reg, reg,
> > +                                          !(reg & lock), 0, 500));
> 
>                                     no delay between reads? ^

Yes, I intended it to be a simple busy waiting loop since I don't
expect it to be very long. Do yu have any more data on how much time
it usually takes?

> 
> > +int sunxi_ccu_probe(struct device_node *node,
> > +                   const struct sunxi_ccu_desc *desc)
> > +{
> > +       struct ccu_common **cclks = desc->clks;
> > +       size_t num_clks = desc->num_clks;
> > +       struct clk_onecell_data *data;
> > +       struct ccu_reset *reset;
> > +       struct clk **clks;
> > +       void __iomem *reg;
> > +       int i, ret;
> > +
> > +       reg = of_iomap(node, 0);
> 
> Why not of_io_request_and_map?

Because initially I still had some old clocks that were probing, which
was leading to some issues. This is obviously not the case anymore,
I'll switch to it.

> 
> > +       if (IS_ERR(reg)) {
> 
> And of_iomap returns NULL on error. This is for of_io_request_and_map.
> 
> > +               pr_err("%s: Could not map the clock registers\n",
> > +                      of_node_full_name(node));
> > +               return PTR_ERR(reg);
> > +       }
> > +
> > +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return -ENOMEM;
> > +
> > +       clks = kcalloc(num_clks, sizeof(struct clk *), GFP_KERNEL);
> > +       if (!clks)
> > +               return -ENOMEM;
> > +
> > +       data->clks = clks;
> > +       data->clk_num = num_clks;
> > +
> > +       for (i = 0; i < num_clks; i++) {
> > +               struct ccu_common *cclk = cclks[i];
> > +               struct clk *clk;
> > +
> > +               if (!cclk) {
> > +                       cclk = ERR_PTR(-ENOENT);
> 
> This seems redundant, unless you intended to use it elsewhere?

Yeah, that was supposed to be clks[i] = ERR_PTR(..);

I'll fix it.

> 
> > +                       continue;
> > +               }
> > +
> > +               cclk->base = reg;
> > +               cclk->lock = &ccu_lock;
> > +
> > +               clk = clk_register(NULL, &cclk->hw);
> > +               if (IS_ERR(clk))
> > +                       continue;
> > +
> > +               clks[i] = clk;
> > +       }
> > +
> > +       ret = of_clk_add_provider(node, of_clk_src_onecell_get, data);
> > +       if (ret)
> > +               goto err_clk_unreg;
> > +
> > +       reset = kzalloc(sizeof(*reset), GFP_KERNEL);
> > +       reset->rcdev.of_node = node;
> > +       reset->rcdev.ops = &ccu_reset_ops;
> > +       reset->rcdev.owner = THIS_MODULE;
> > +       reset->rcdev.nr_resets = desc->num_resets;
> > +       reset->base = reg;
> > +       reset->lock = &ccu_lock;
> > +       reset->reset_map = desc->resets;
> > +
> > +       ret = reset_controller_register(&reset->rcdev);
> > +       if (ret)
> > +               goto err_of_clk_unreg;
> > +
> > +       return 0;
> > +
> > +err_of_clk_unreg:
> > +err_clk_unreg:
> > +       return ret;
> > +}
> > diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
> > new file mode 100644
> > index 000000000000..e8b477fcd320
> > --- /dev/null
> > +++ b/drivers/clk/sunxi-ng/ccu_common.h
> > @@ -0,0 +1,74 @@
> > +/*
> > + * Copyright (c) 2016 Maxime Ripard. All rights reserved.
> > + *
> > + * 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 _COMMON_H_
> > +#define _COMMON_H_
> > +
> > +#include <linux/compiler.h>
> > +#include <linux/clk-provider.h>
> > +
> > +#define CCU_FEATURE_GATE               BIT(0)
> > +#define CCU_FEATURE_LOCK               BIT(1)
> 
> *_PLL_LOCK would be clearer that this implements a PLL lock indicator.
> Or maybe a comment.

I'll change it for PLL_LOCK

> 
> > +#define CCU_FEATURE_FRACTIONAL         BIT(2)
> > +#define CCU_FEATURE_VARIABLE_PREDIV    BIT(3)
> > +#define CCU_FEATURE_FIXED_PREDIV       BIT(4)
> > +#define CCU_FEATURE_FIXED_POSTDIV      BIT(5)
> > +
> > +struct device_node;
> > +
> > +#define SUNXI_HW_INIT(_name, _parent, _ops, _flags)                    \
> > +       &(struct clk_init_data) {                                       \
> > +               .flags          = _flags,                               \
> > +               .name           = _name,                                \
> > +               .parent_names   = (const char *[]) { _parent },         \
> > +               .num_parents    = 1,                                    \
> > +               .ops            = _ops,                                 \
> > +       }
> > +
> > +#define SUNXI_HW_INIT_PARENTS(_name, _parents, _ops, _flags)           \
> > +       &(struct clk_init_data) {                                       \
> > +               .flags          = _flags,                               \
> > +               .name           = _name,                                \
> > +               .parent_names   = _parents,                             \
> > +               .num_parents    = ARRAY_SIZE(_parents),                 \
> > +               .ops            = _ops,                                 \
> > +       }
> > +
> > +struct ccu_common {
> > +       void __iomem    *base;
> > +       unsigned long   reg;
> 
> This seems quite large, considering the address space of the CCU,
> and you using u16 or u32 for the same thing on the reset control side.

Indeed, what about u16?

> 
> > +
> > +       unsigned long   features;
> > +       spinlock_t      *lock;
> > +       struct clk_hw   hw;
> > +};
> > +
> > +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
> > +{
> > +       return container_of(hw, struct ccu_common, hw);
> > +}
> > +
> > +struct sunxi_ccu_desc {
> > +       struct ccu_common       **clks;
> > +       unsigned long           num_clks;
> > +
> > +       struct ccu_reset_map    *resets;
> > +       unsigned long           num_resets;
> > +};
> > +
> > +void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock);
> > +
> > +int sunxi_ccu_probe(struct device_node *node,
> > +                   const struct sunxi_ccu_desc *desc);
> > +
> > +#endif /* _COMMON_H_ */
> > diff --git a/drivers/clk/sunxi-ng/ccu_factor.h b/drivers/clk/sunxi-ng/ccu_factor.h
> > new file mode 100644
> > index 000000000000..e7cc564aaea0
> > --- /dev/null
> > +++ b/drivers/clk/sunxi-ng/ccu_factor.h
> > @@ -0,0 +1,15 @@
> > +#ifndef _CLK_FACTOR_H_
> > +#define _CLK_FACTOR_H_
> > +
> > +struct ccu_factor {
> > +       u8      shift;
> > +       u8      width;
> > +};
> > +
> > +#define SUNXI_CLK_FACTOR(_shift, _width)       \
> > +       {                                       \
> > +               .shift  = _shift,               \
> > +               .width  = _width,               \
> > +       }
> > +
> > +#endif /* _CLK_FACTOR_H_ */
> > diff --git a/drivers/clk/sunxi-ng/ccu_mux.h b/drivers/clk/sunxi-ng/ccu_mux.h
> > new file mode 100644
> > index 000000000000..17cedad4e433
> > --- /dev/null
> > +++ b/drivers/clk/sunxi-ng/ccu_mux.h
> 
> As far as I can tell there are no users of this file within this patch or the
> following patches before the mux clock support one. It'd be easier to understand
> if this part was moved to the mux clock patch.

Will do.

> 
> > @@ -0,0 +1,20 @@
> > +#ifndef _CCU_MUX_H_
> > +#define _CCU_MUX_H_
> > +
> > +#include "common.h"
> > +
> > +struct ccu_mux_internal {
> > +       u8      shift;
> > +       u8      width;
> > +
> > +       u8      *map;
> 
> I assume map is a table?
> 
> > +};
> > +
> > +#define SUNXI_CLK_MUX(_shift, _width, _map)    \
> > +       {                                       \
> > +               .map    = _map,                 \
> > +               .shift  = _shift,               \
> > +               .width  = _width,               \
> > +       }
> > +
> > +#endif /* _CCU_MUX_H_ */
> > diff --git a/drivers/clk/sunxi-ng/ccu_reset.c b/drivers/clk/sunxi-ng/ccu_reset.c
> > new file mode 100644
> > index 000000000000..6c31d48783a7
> > --- /dev/null
> > +++ b/drivers/clk/sunxi-ng/ccu_reset.c
> > @@ -0,0 +1,55 @@
> > +/*
> > + * Copyright (C) 2016 Maxime Ripard
> > + * Maxime Ripard <[e-mail]>
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/reset-controller.h>
> > +
> > +#include "ccu_reset.h"
> > +
> > +static int ccu_reset_assert(struct reset_controller_dev *rcdev,
> > +                           unsigned long id)
> > +{
> > +       struct ccu_reset *ccu = rcdev_to_ccu_reset(rcdev);
> > +       const struct ccu_reset_map *map = &ccu->reset_map[id];
> > +       unsigned long flags;
> > +       u32 reg;
> > +
> > +       spin_lock_irqsave(ccu->lock, flags);
> > +
> > +       reg = readl(ccu->base + map->reg);
> > +       writel(reg & ~map->bit, ccu->base + map->reg);
> > +
> > +       spin_unlock_irqrestore(ccu->lock, flags);
> > +
> > +       return 0;
> > +}
> > +
> > +static int ccu_reset_deassert(struct reset_controller_dev *rcdev,
> > +                             unsigned long id)
> > +{
> > +       struct ccu_reset *ccu = rcdev_to_ccu_reset(rcdev);
> > +       const struct ccu_reset_map *map = &ccu->reset_map[id];
> > +       unsigned long flags;
> > +       u32 reg;
> > +
> > +       spin_lock_irqsave(ccu->lock, flags);
> > +
> > +       reg = readl(ccu->base + map->reg);
> > +       writel(reg | map->bit, ccu->base + map->reg);
> > +
> > +       spin_unlock_irqrestore(ccu->lock, flags);
> > +
> > +       return 0;
> > +}
> > +
> > +const struct reset_control_ops ccu_reset_ops = {
> > +       .assert         = ccu_reset_assert,
> > +       .deassert       = ccu_reset_deassert,
> > +};
> > diff --git a/drivers/clk/sunxi-ng/ccu_reset.h b/drivers/clk/sunxi-ng/ccu_reset.h
> > new file mode 100644
> > index 000000000000..36a4679210bd
> > --- /dev/null
> > +++ b/drivers/clk/sunxi-ng/ccu_reset.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * Copyright (c) 2016 Maxime Ripard. All rights reserved.
> > + *
> > + * 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_RESET_H_
> > +#define _CCU_RESET_H_
> > +
> > +#include <linux/reset-controller.h>
> > +
> > +struct ccu_reset_map {
> > +       u16     reg;
> > +       u32     bit;
> > +};
> > +
> > +
> > +struct ccu_reset {
> > +       void __iomem                    *base;
> > +       struct ccu_reset_map            *reset_map;
> > +       spinlock_t                      *lock;
> > +
> > +       struct reset_controller_dev     rcdev;
> > +};
> > +
> > +static inline struct ccu_reset *rcdev_to_ccu_reset(struct reset_controller_dev *rcdev)
> > +{
> > +       return container_of(rcdev, struct ccu_reset, rcdev);
> > +}
> > +
> > +extern const struct reset_control_ops ccu_reset_ops;
> > +
> > +#endif /* _CCU_RESET_H_ */
> 
> The reset control code looks good.

Thanks!
Maxime

Chen-Yu Tsai May 16, 2016, 7:02 a.m. UTC | #3

On Mon, May 16, 2016 at 2:31 AM, Maxime Ripard
<[e-mail]> wrote:
> Hi,
>
> On Mon, May 09, 2016 at 06:01:45PM +0800, Chen-Yu Tsai wrote:
>> On Mon, May 9, 2016 at 4:01 AM, Maxime Ripard
>> <[e-mail]> wrote:
>> > Start our new clock infrastructure by adding the registration code, common
>> > structure and common code.
>> >
>> > Signed-off-by: Maxime Ripard <[e-mail]>
>> > ---
>> >  drivers/clk/Makefile              |   1 +
>> >  drivers/clk/sunxi-ng/Makefile     |   2 +
>> >  drivers/clk/sunxi-ng/ccu_common.c | 108 ++++++++++++++++++++++++++++++++++++++
>> >  drivers/clk/sunxi-ng/ccu_common.h |  74 ++++++++++++++++++++++++++
>> >  drivers/clk/sunxi-ng/ccu_factor.h |  15 ++++++
>> >  drivers/clk/sunxi-ng/ccu_mux.h    |  20 +++++++
>> >  drivers/clk/sunxi-ng/ccu_reset.c  |  55 +++++++++++++++++++
>> >  drivers/clk/sunxi-ng/ccu_reset.h  |  40 ++++++++++++++
>> >  8 files changed, 315 insertions(+)
>> >  create mode 100644 drivers/clk/sunxi-ng/Makefile
>> >  create mode 100644 drivers/clk/sunxi-ng/ccu_common.c
>> >  create mode 100644 drivers/clk/sunxi-ng/ccu_common.h
>> >  create mode 100644 drivers/clk/sunxi-ng/ccu_factor.h
>> >  create mode 100644 drivers/clk/sunxi-ng/ccu_mux.h
>> >  create mode 100644 drivers/clk/sunxi-ng/ccu_reset.c
>> >  create mode 100644 drivers/clk/sunxi-ng/ccu_reset.h
>> >
>> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> > index 4ef71a13ab37..83a93cd9e21d 100644
>> > --- a/drivers/clk/Makefile
>> > +++ b/drivers/clk/Makefile
>> > @@ -78,6 +78,7 @@ obj-$(CONFIG_ARCH_SOCFPGA)            += socfpga/
>> >  obj-$(CONFIG_PLAT_SPEAR)               += spear/
>> >  obj-$(CONFIG_ARCH_STI)                 += st/
>> >  obj-$(CONFIG_ARCH_SUNXI)               += sunxi/
>> > +obj-$(CONFIG_ARCH_SUNXI)               += sunxi-ng/
>> >  obj-$(CONFIG_ARCH_TEGRA)               += tegra/
>> >  obj-y                                  += ti/
>> >  obj-$(CONFIG_ARCH_U8500)               += ux500/
>> > diff --git a/drivers/clk/sunxi-ng/Makefile b/drivers/clk/sunxi-ng/Makefile
>> > new file mode 100644
>> > index 000000000000..bd3461b0f38c
>> > --- /dev/null
>> > +++ b/drivers/clk/sunxi-ng/Makefile
>> > @@ -0,0 +1,2 @@
>> > +obj-y += ccu_common.o
>> > +obj-y += ccu_reset.o
>> > diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
>> > new file mode 100644
>> > index 000000000000..1d9242566fbd
>> > --- /dev/null
>> > +++ b/drivers/clk/sunxi-ng/ccu_common.c
>> > @@ -0,0 +1,108 @@
>> > +/*
>> > + * Copyright 2016 Maxime Ripard
>> > + *
>> > + * Maxime Ripard <[e-mail]>
>> > + *
>> > + * 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.
>> > + *
>> > + * 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.
>> > + */
>> > +
>> > +#include <linux/clk-provider.h>
>> > +#include <linux/iopoll.h>
>> > +#include <linux/of_address.h>
>> > +#include <linux/slab.h>
>> > +
>> > +#include "ccu_common.h"
>> > +#include "ccu_reset.h"
>> > +
>> > +static DEFINE_SPINLOCK(ccu_lock);
>> > +
>> > +void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock)
>> > +{
>> > +       u32 reg;
>> > +
>> > +       if (!(common->features & CCU_FEATURE_LOCK))
>> > +               return;
>> > +
>> > +       WARN_ON(readl_relaxed_poll_timeout(common->base + common->reg, reg,
>> > +                                          !(reg & lock), 0, 500));
>>
>>                                     no delay between reads? ^
>
> Yes, I intended it to be a simple busy waiting loop since I don't
> expect it to be very long. Do yu have any more data on how much time
> it usually takes?
>
>>
>> > +int sunxi_ccu_probe(struct device_node *node,
>> > +                   const struct sunxi_ccu_desc *desc)
>> > +{
>> > +       struct ccu_common **cclks = desc->clks;
>> > +       size_t num_clks = desc->num_clks;
>> > +       struct clk_onecell_data *data;
>> > +       struct ccu_reset *reset;
>> > +       struct clk **clks;
>> > +       void __iomem *reg;
>> > +       int i, ret;
>> > +
>> > +       reg = of_iomap(node, 0);
>>
>> Why not of_io_request_and_map?
>
> Because initially I still had some old clocks that were probing, which
> was leading to some issues. This is obviously not the case anymore,
> I'll switch to it.
>
>>
>> > +       if (IS_ERR(reg)) {
>>
>> And of_iomap returns NULL on error. This is for of_io_request_and_map.
>>
>> > +               pr_err("%s: Could not map the clock registers\n",
>> > +                      of_node_full_name(node));
>> > +               return PTR_ERR(reg);
>> > +       }
>> > +
>> > +       data = kzalloc(sizeof(*data), GFP_KERNEL);
>> > +       if (!data)
>> > +               return -ENOMEM;
>> > +
>> > +       clks = kcalloc(num_clks, sizeof(struct clk *), GFP_KERNEL);
>> > +       if (!clks)
>> > +               return -ENOMEM;
>> > +
>> > +       data->clks = clks;
>> > +       data->clk_num = num_clks;
>> > +
>> > +       for (i = 0; i < num_clks; i++) {
>> > +               struct ccu_common *cclk = cclks[i];
>> > +               struct clk *clk;
>> > +
>> > +               if (!cclk) {
>> > +                       cclk = ERR_PTR(-ENOENT);
>>
>> This seems redundant, unless you intended to use it elsewhere?
>
> Yeah, that was supposed to be clks[i] = ERR_PTR(..);
>
> I'll fix it.
>
>>
>> > +                       continue;
>> > +               }
>> > +
>> > +               cclk->base = reg;
>> > +               cclk->lock = &ccu_lock;
>> > +
>> > +               clk = clk_register(NULL, &cclk->hw);
>> > +               if (IS_ERR(clk))
>> > +                       continue;
>> > +
>> > +               clks[i] = clk;
>> > +       }
>> > +
>> > +       ret = of_clk_add_provider(node, of_clk_src_onecell_get, data);
>> > +       if (ret)
>> > +               goto err_clk_unreg;
>> > +
>> > +       reset = kzalloc(sizeof(*reset), GFP_KERNEL);
>> > +       reset->rcdev.of_node = node;
>> > +       reset->rcdev.ops = &ccu_reset_ops;
>> > +       reset->rcdev.owner = THIS_MODULE;
>> > +       reset->rcdev.nr_resets = desc->num_resets;
>> > +       reset->base = reg;
>> > +       reset->lock = &ccu_lock;
>> > +       reset->reset_map = desc->resets;
>> > +
>> > +       ret = reset_controller_register(&reset->rcdev);
>> > +       if (ret)
>> > +               goto err_of_clk_unreg;
>> > +
>> > +       return 0;
>> > +
>> > +err_of_clk_unreg:
>> > +err_clk_unreg:
>> > +       return ret;
>> > +}
>> > diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
>> > new file mode 100644
>> > index 000000000000..e8b477fcd320
>> > --- /dev/null
>> > +++ b/drivers/clk/sunxi-ng/ccu_common.h
>> > @@ -0,0 +1,74 @@
>> > +/*
>> > + * Copyright (c) 2016 Maxime Ripard. All rights reserved.
>> > + *
>> > + * 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 _COMMON_H_
>> > +#define _COMMON_H_
>> > +
>> > +#include <linux/compiler.h>
>> > +#include <linux/clk-provider.h>
>> > +
>> > +#define CCU_FEATURE_GATE               BIT(0)
>> > +#define CCU_FEATURE_LOCK               BIT(1)
>>
>> *_PLL_LOCK would be clearer that this implements a PLL lock indicator.
>> Or maybe a comment.
>
> I'll change it for PLL_LOCK
>
>>
>> > +#define CCU_FEATURE_FRACTIONAL         BIT(2)
>> > +#define CCU_FEATURE_VARIABLE_PREDIV    BIT(3)
>> > +#define CCU_FEATURE_FIXED_PREDIV       BIT(4)
>> > +#define CCU_FEATURE_FIXED_POSTDIV      BIT(5)
>> > +
>> > +struct device_node;
>> > +
>> > +#define SUNXI_HW_INIT(_name, _parent, _ops, _flags)                    \
>> > +       &(struct clk_init_data) {                                       \
>> > +               .flags          = _flags,                               \
>> > +               .name           = _name,                                \
>> > +               .parent_names   = (const char *[]) { _parent },         \
>> > +               .num_parents    = 1,                                    \
>> > +               .ops            = _ops,                                 \
>> > +       }
>> > +
>> > +#define SUNXI_HW_INIT_PARENTS(_name, _parents, _ops, _flags)           \
>> > +       &(struct clk_init_data) {                                       \
>> > +               .flags          = _flags,                               \
>> > +               .name           = _name,                                \
>> > +               .parent_names   = _parents,                             \
>> > +               .num_parents    = ARRAY_SIZE(_parents),                 \
>> > +               .ops            = _ops,                                 \
>> > +       }
>> > +
>> > +struct ccu_common {
>> > +       void __iomem    *base;
>> > +       unsigned long   reg;
>>
>> This seems quite large, considering the address space of the CCU,
>> and you using u16 or u32 for the same thing on the reset control side.
>
> Indeed, what about u16?

Either u16 or u32 will do. I suspect u16 will get bumped up to u32 due to
alignment issues of the struct.

ChenYu

>>
>> > +
>> > +       unsigned long   features;
>> > +       spinlock_t      *lock;
>> > +       struct clk_hw   hw;
>> > +};
>> > +
>> > +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>> > +{
>> > +       return container_of(hw, struct ccu_common, hw);
>> > +}
>> > +
>> > +struct sunxi_ccu_desc {
>> > +       struct ccu_common       **clks;
>> > +       unsigned long           num_clks;
>> > +
>> > +       struct ccu_reset_map    *resets;
>> > +       unsigned long           num_resets;
>> > +};
>> > +
>> > +void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock);
>> > +
>> > +int sunxi_ccu_probe(struct device_node *node,
>> > +                   const struct sunxi_ccu_desc *desc);
>> > +
>> > +#endif /* _COMMON_H_ */
>> > diff --git a/drivers/clk/sunxi-ng/ccu_factor.h b/drivers/clk/sunxi-ng/ccu_factor.h
>> > new file mode 100644
>> > index 000000000000..e7cc564aaea0
>> > --- /dev/null
>> > +++ b/drivers/clk/sunxi-ng/ccu_factor.h
>> > @@ -0,0 +1,15 @@
>> > +#ifndef _CLK_FACTOR_H_
>> > +#define _CLK_FACTOR_H_
>> > +
>> > +struct ccu_factor {
>> > +       u8      shift;
>> > +       u8      width;
>> > +};
>> > +
>> > +#define SUNXI_CLK_FACTOR(_shift, _width)       \
>> > +       {                                       \
>> > +               .shift  = _shift,               \
>> > +               .width  = _width,               \
>> > +       }
>> > +
>> > +#endif /* _CLK_FACTOR_H_ */
>> > diff --git a/drivers/clk/sunxi-ng/ccu_mux.h b/drivers/clk/sunxi-ng/ccu_mux.h
>> > new file mode 100644
>> > index 000000000000..17cedad4e433
>> > --- /dev/null
>> > +++ b/drivers/clk/sunxi-ng/ccu_mux.h
>>
>> As far as I can tell there are no users of this file within this patch or the
>> following patches before the mux clock support one. It'd be easier to understand
>> if this part was moved to the mux clock patch.
>
> Will do.
>
>>
>> > @@ -0,0 +1,20 @@
>> > +#ifndef _CCU_MUX_H_
>> > +#define _CCU_MUX_H_
>> > +
>> > +#include "common.h"
>> > +
>> > +struct ccu_mux_internal {
>> > +       u8      shift;
>> > +       u8      width;
>> > +
>> > +       u8      *map;
>>
>> I assume map is a table?
>>
>> > +};
>> > +
>> > +#define SUNXI_CLK_MUX(_shift, _width, _map)    \
>> > +       {                                       \
>> > +               .map    = _map,                 \
>> > +               .shift  = _shift,               \
>> > +               .width  = _width,               \
>> > +       }
>> > +
>> > +#endif /* _CCU_MUX_H_ */
>> > diff --git a/drivers/clk/sunxi-ng/ccu_reset.c b/drivers/clk/sunxi-ng/ccu_reset.c
>> > new file mode 100644
>> > index 000000000000..6c31d48783a7
>> > --- /dev/null
>> > +++ b/drivers/clk/sunxi-ng/ccu_reset.c
>> > @@ -0,0 +1,55 @@
>> > +/*
>> > + * Copyright (C) 2016 Maxime Ripard
>> > + * Maxime Ripard <[e-mail]>
>> > + *
>> > + * 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.
>> > + */
>> > +
>> > +#include <linux/io.h>
>> > +#include <linux/reset-controller.h>
>> > +
>> > +#include "ccu_reset.h"
>> > +
>> > +static int ccu_reset_assert(struct reset_controller_dev *rcdev,
>> > +                           unsigned long id)
>> > +{
>> > +       struct ccu_reset *ccu = rcdev_to_ccu_reset(rcdev);
>> > +       const struct ccu_reset_map *map = &ccu->reset_map[id];
>> > +       unsigned long flags;
>> > +       u32 reg;
>> > +
>> > +       spin_lock_irqsave(ccu->lock, flags);
>> > +
>> > +       reg = readl(ccu->base + map->reg);
>> > +       writel(reg & ~map->bit, ccu->base + map->reg);
>> > +
>> > +       spin_unlock_irqrestore(ccu->lock, flags);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int ccu_reset_deassert(struct reset_controller_dev *rcdev,
>> > +                             unsigned long id)
>> > +{
>> > +       struct ccu_reset *ccu = rcdev_to_ccu_reset(rcdev);
>> > +       const struct ccu_reset_map *map = &ccu->reset_map[id];
>> > +       unsigned long flags;
>> > +       u32 reg;
>> > +
>> > +       spin_lock_irqsave(ccu->lock, flags);
>> > +
>> > +       reg = readl(ccu->base + map->reg);
>> > +       writel(reg | map->bit, ccu->base + map->reg);
>> > +
>> > +       spin_unlock_irqrestore(ccu->lock, flags);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +const struct reset_control_ops ccu_reset_ops = {
>> > +       .assert         = ccu_reset_assert,
>> > +       .deassert       = ccu_reset_deassert,
>> > +};
>> > diff --git a/drivers/clk/sunxi-ng/ccu_reset.h b/drivers/clk/sunxi-ng/ccu_reset.h
>> > new file mode 100644
>> > index 000000000000..36a4679210bd
>> > --- /dev/null
>> > +++ b/drivers/clk/sunxi-ng/ccu_reset.h
>> > @@ -0,0 +1,40 @@
>> > +/*
>> > + * Copyright (c) 2016 Maxime Ripard. All rights reserved.
>> > + *
>> > + * 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_RESET_H_
>> > +#define _CCU_RESET_H_
>> > +
>> > +#include <linux/reset-controller.h>
>> > +
>> > +struct ccu_reset_map {
>> > +       u16     reg;
>> > +       u32     bit;
>> > +};
>> > +
>> > +
>> > +struct ccu_reset {
>> > +       void __iomem                    *base;
>> > +       struct ccu_reset_map            *reset_map;
>> > +       spinlock_t                      *lock;
>> > +
>> > +       struct reset_controller_dev     rcdev;
>> > +};
>> > +
>> > +static inline struct ccu_reset *rcdev_to_ccu_reset(struct reset_controller_dev *rcdev)
>> > +{
>> > +       return container_of(rcdev, struct ccu_reset, rcdev);
>> > +}
>> > +
>> > +extern const struct reset_control_ops ccu_reset_ops;
>> > +
>> > +#endif /* _CCU_RESET_H_ */
>>
>> The reset control code looks good.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jean-Francois Moine May 16, 2016, 8:02 a.m. UTC | #4

On Sun, 15 May 2016 20:31:22 +0200
Maxime Ripard <[e-mail]> wrote:

> > > +void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock)
> > > +{
> > > +       u32 reg;
> > > +
> > > +       if (!(common->features & CCU_FEATURE_LOCK))
> > > +               return;
> > > +
> > > +       WARN_ON(readl_relaxed_poll_timeout(common->base + common->reg, reg,
> > > +                                          !(reg & lock), 0, 500));
> > 
> >                                     no delay between reads? ^
> 
> Yes, I intended it to be a simple busy waiting loop since I don't
> expect it to be very long. Do yu have any more data on how much time
> it usually takes?

I have a Soc in which the rate of the audio clock is stable after a
good second.

Maxime Ripard May 16, 2016, 8:15 p.m. UTC | #5

Hi Jean-Francois,

On Mon, May 16, 2016 at 10:02:39AM +0200, Jean-Francois Moine wrote:
> On Sun, 15 May 2016 20:31:22 +0200
> Maxime Ripard <[e-mail]> wrote:
> 
> > > > +void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock)
> > > > +{
> > > > +       u32 reg;
> > > > +
> > > > +       if (!(common->features & CCU_FEATURE_LOCK))
> > > > +               return;
> > > > +
> > > > +       WARN_ON(readl_relaxed_poll_timeout(common->base + common->reg, reg,
> > > > +                                          !(reg & lock), 0, 500));
> > > 
> > >                                     no delay between reads? ^
> > 
> > Yes, I intended it to be a simple busy waiting loop since I don't
> > expect it to be very long. Do yu have any more data on how much time
> > it usually takes?
> 
> I have a Soc in which the rate of the audio clock is stable after a
> good second.

You mean before the clock is actually stable, or before the lock bit
is cleared?

Which SoC is it? As far as I've seen, only the H3 allows to configure
the stable time, and while by default it will take 16us, you can
configure as high as 66ms (which is still way higher than the current
limit).

Thanks!
Maxime

Jean-Francois Moine May 17, 2016, 6:54 a.m. UTC | #6

On Mon, 16 May 2016 22:15:09 +0200
Maxime Ripard <[e-mail]> wrote:

> > > Yes, I intended it to be a simple busy waiting loop since I don't
> > > expect it to be very long. Do yu have any more data on how much time
> > > it usually takes?
> > 
> > I have a Soc in which the rate of the audio clock is stable after a
> > good second.
> 
> You mean before the clock is actually stable, or before the lock bit
> is cleared?
> 
> Which SoC is it? As far as I've seen, only the H3 allows to configure
> the stable time, and while by default it will take 16us, you can
> configure as high as 66ms (which is still way higher than the current
> limit).

Hi Maxime,

Well, it is not a SoC, but an external chip, the SI5351 (in the Dove
Cubox). There is no lock bit, but as it is used for audio, and as I did
not set any delay at streaming start time, I can hear when the clock is
stable.

Maxime Ripard May 18, 2016, 7:59 p.m. UTC | #7

On Tue, May 17, 2016 at 08:54:27AM +0200, Jean-Francois Moine wrote:
> On Mon, 16 May 2016 22:15:09 +0200
> Maxime Ripard <[e-mail]> wrote:
> 
> > > > Yes, I intended it to be a simple busy waiting loop since I don't
> > > > expect it to be very long. Do yu have any more data on how much time
> > > > it usually takes?
> > > 
> > > I have a Soc in which the rate of the audio clock is stable after a
> > > good second.
> > 
> > You mean before the clock is actually stable, or before the lock bit
> > is cleared?
> > 
> > Which SoC is it? As far as I've seen, only the H3 allows to configure
> > the stable time, and while by default it will take 16us, you can
> > configure as high as 66ms (which is still way higher than the current
> > limit).
> 
> Hi Maxime,
> 
> Well, it is not a SoC, but an external chip, the SI5351 (in the Dove
> Cubox). There is no lock bit, but as it is used for audio, and as I did
> not set any delay at streaming start time, I can hear when the clock is
> stable.

Ok. So I guess we set it high enough to cover the maximum we know for
know (65ms), and we can always rise that up later if needed.

Thanks!
Maxime

Patch

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 4ef71a13ab37..83a93cd9e21d 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -78,6 +78,7 @@  obj-$(CONFIG_ARCH_SOCFPGA)		+= socfpga/
 obj-$(CONFIG_PLAT_SPEAR)		+= spear/
 obj-$(CONFIG_ARCH_STI)			+= st/
 obj-$(CONFIG_ARCH_SUNXI)		+= sunxi/
+obj-$(CONFIG_ARCH_SUNXI)		+= sunxi-ng/
 obj-$(CONFIG_ARCH_TEGRA)		+= tegra/
 obj-y					+= ti/
 obj-$(CONFIG_ARCH_U8500)		+= ux500/
diff --git a/drivers/clk/sunxi-ng/Makefile b/drivers/clk/sunxi-ng/Makefile
new file mode 100644
index 000000000000..bd3461b0f38c
--- /dev/null
+++ b/drivers/clk/sunxi-ng/Makefile
@@ -0,0 +1,2 @@ 
+obj-y += ccu_common.o
+obj-y += ccu_reset.o
diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
new file mode 100644
index 000000000000..1d9242566fbd
--- /dev/null
+++ b/drivers/clk/sunxi-ng/ccu_common.c
@@ -0,0 +1,108 @@ 
+/*
+ * Copyright 2016 Maxime Ripard
+ *
+ * Maxime Ripard <[e-mail]>
+ *
+ * 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.
+ *
+ * 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.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/iopoll.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+#include "ccu_common.h"
+#include "ccu_reset.h"
+
+static DEFINE_SPINLOCK(ccu_lock);
+
+void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock)
+{
+	u32 reg;
+
+	if (!(common->features & CCU_FEATURE_LOCK))
+		return;
+
+	WARN_ON(readl_relaxed_poll_timeout(common->base + common->reg, reg,
+					   !(reg & lock), 0, 500));
+}
+
+int sunxi_ccu_probe(struct device_node *node,
+		    const struct sunxi_ccu_desc *desc)
+{
+	struct ccu_common **cclks = desc->clks;
+	size_t num_clks = desc->num_clks;
+	struct clk_onecell_data *data;
+	struct ccu_reset *reset;
+	struct clk **clks;
+	void __iomem *reg;
+	int i, ret;
+	
+	reg = of_iomap(node, 0);
+	if (IS_ERR(reg)) {
+		pr_err("%s: Could not map the clock registers\n",
+		       of_node_full_name(node));
+		return PTR_ERR(reg);
+	}
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	clks = kcalloc(num_clks, sizeof(struct clk *), GFP_KERNEL);
+	if (!clks)
+		return -ENOMEM;
+
+	data->clks = clks;
+	data->clk_num = num_clks;
+
+	for (i = 0; i < num_clks; i++) {
+		struct ccu_common *cclk = cclks[i];
+		struct clk *clk;
+
+		if (!cclk) {
+			cclk = ERR_PTR(-ENOENT);
+			continue;
+		}
+
+		cclk->base = reg;
+		cclk->lock = &ccu_lock;
+
+		clk = clk_register(NULL, &cclk->hw);
+		if (IS_ERR(clk))
+			continue;
+
+		clks[i] = clk;
+	}
+
+	ret = of_clk_add_provider(node, of_clk_src_onecell_get, data);
+	if (ret)
+		goto err_clk_unreg;
+
+	reset = kzalloc(sizeof(*reset), GFP_KERNEL);
+	reset->rcdev.of_node = node;
+	reset->rcdev.ops = &ccu_reset_ops;
+	reset->rcdev.owner = THIS_MODULE;
+	reset->rcdev.nr_resets = desc->num_resets;
+	reset->base = reg;
+	reset->lock = &ccu_lock;
+	reset->reset_map = desc->resets;
+
+	ret = reset_controller_register(&reset->rcdev);
+	if (ret)
+		goto err_of_clk_unreg;
+
+	return 0;
+
+err_of_clk_unreg:
+err_clk_unreg:
+	return ret;
+}
diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
new file mode 100644
index 000000000000..e8b477fcd320
--- /dev/null
+++ b/drivers/clk/sunxi-ng/ccu_common.h
@@ -0,0 +1,74 @@ 
+/*
+ * Copyright (c) 2016 Maxime Ripard. All rights reserved.
+ *
+ * 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 _COMMON_H_
+#define _COMMON_H_
+
+#include <linux/compiler.h>
+#include <linux/clk-provider.h>
+
+#define CCU_FEATURE_GATE		BIT(0)
+#define CCU_FEATURE_LOCK		BIT(1)
+#define CCU_FEATURE_FRACTIONAL		BIT(2)
+#define CCU_FEATURE_VARIABLE_PREDIV	BIT(3)
+#define CCU_FEATURE_FIXED_PREDIV	BIT(4)
+#define CCU_FEATURE_FIXED_POSTDIV	BIT(5)
+
+struct device_node;
+
+#define SUNXI_HW_INIT(_name, _parent, _ops, _flags)			\
+	&(struct clk_init_data) {					\
+		.flags		= _flags,				\
+		.name		= _name,				\
+		.parent_names	= (const char *[]) { _parent },		\
+		.num_parents	= 1,					\
+		.ops 		= _ops,					\
+	}
+
+#define SUNXI_HW_INIT_PARENTS(_name, _parents, _ops, _flags)		\
+	&(struct clk_init_data) {					\
+		.flags		= _flags,				\
+		.name		= _name,				\
+		.parent_names	= _parents,				\
+		.num_parents	= ARRAY_SIZE(_parents),			\
+		.ops 		= _ops,					\
+	}
+
+struct ccu_common {
+	void __iomem	*base;
+	unsigned long	reg;
+
+	unsigned long	features;
+	spinlock_t	*lock;
+	struct clk_hw	hw;
+};
+
+static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
+{
+	return container_of(hw, struct ccu_common, hw);
+}
+
+struct sunxi_ccu_desc {
+	struct ccu_common	**clks;
+	unsigned long		num_clks;
+
+	struct ccu_reset_map	*resets;
+	unsigned long		num_resets;
+};
+
+void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock);
+
+int sunxi_ccu_probe(struct device_node *node,
+		    const struct sunxi_ccu_desc *desc);
+
+#endif /* _COMMON_H_ */
diff --git a/drivers/clk/sunxi-ng/ccu_factor.h b/drivers/clk/sunxi-ng/ccu_factor.h
new file mode 100644
index 000000000000..e7cc564aaea0
--- /dev/null
+++ b/drivers/clk/sunxi-ng/ccu_factor.h
@@ -0,0 +1,15 @@ 
+#ifndef _CLK_FACTOR_H_
+#define _CLK_FACTOR_H_
+
+struct ccu_factor {
+	u8	shift;
+	u8	width;
+};
+
+#define SUNXI_CLK_FACTOR(_shift, _width)	\
+	{					\
+		.shift	= _shift,		\
+		.width	= _width,		\
+	}
+
+#endif /* _CLK_FACTOR_H_ */
diff --git a/drivers/clk/sunxi-ng/ccu_mux.h b/drivers/clk/sunxi-ng/ccu_mux.h
new file mode 100644
index 000000000000..17cedad4e433
--- /dev/null
+++ b/drivers/clk/sunxi-ng/ccu_mux.h
@@ -0,0 +1,20 @@ 
+#ifndef _CCU_MUX_H_
+#define _CCU_MUX_H_
+
+#include "common.h"
+
+struct ccu_mux_internal {
+	u8	shift;
+	u8	width;
+
+	u8	*map;
+};
+
+#define SUNXI_CLK_MUX(_shift, _width, _map)	\
+	{					\
+		.map	= _map,			\
+		.shift	= _shift,		\
+		.width	= _width,		\
+	}
+
+#endif /* _CCU_MUX_H_ */
diff --git a/drivers/clk/sunxi-ng/ccu_reset.c b/drivers/clk/sunxi-ng/ccu_reset.c
new file mode 100644
index 000000000000..6c31d48783a7
--- /dev/null
+++ b/drivers/clk/sunxi-ng/ccu_reset.c
@@ -0,0 +1,55 @@ 
+/*
+ * Copyright (C) 2016 Maxime Ripard
+ * Maxime Ripard <[e-mail]>
+ *
+ * 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.
+ */
+
+#include <linux/io.h>
+#include <linux/reset-controller.h>
+
+#include "ccu_reset.h"
+
+static int ccu_reset_assert(struct reset_controller_dev *rcdev,
+			    unsigned long id)
+{
+	struct ccu_reset *ccu = rcdev_to_ccu_reset(rcdev);
+	const struct ccu_reset_map *map = &ccu->reset_map[id];
+	unsigned long flags;
+	u32 reg;
+
+	spin_lock_irqsave(ccu->lock, flags);
+
+	reg = readl(ccu->base + map->reg);
+	writel(reg & ~map->bit, ccu->base + map->reg);
+
+	spin_unlock_irqrestore(ccu->lock, flags);
+
+	return 0;
+}
+
+static int ccu_reset_deassert(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct ccu_reset *ccu = rcdev_to_ccu_reset(rcdev);
+	const struct ccu_reset_map *map = &ccu->reset_map[id];
+	unsigned long flags;
+	u32 reg;
+
+	spin_lock_irqsave(ccu->lock, flags);
+
+	reg = readl(ccu->base + map->reg);
+	writel(reg | map->bit, ccu->base + map->reg);
+
+	spin_unlock_irqrestore(ccu->lock, flags);
+
+	return 0;
+}
+
+const struct reset_control_ops ccu_reset_ops = {
+	.assert		= ccu_reset_assert,
+	.deassert	= ccu_reset_deassert,
+};
diff --git a/drivers/clk/sunxi-ng/ccu_reset.h b/drivers/clk/sunxi-ng/ccu_reset.h
new file mode 100644
index 000000000000..36a4679210bd
--- /dev/null
+++ b/drivers/clk/sunxi-ng/ccu_reset.h
@@ -0,0 +1,40 @@ 
+/*
+ * Copyright (c) 2016 Maxime Ripard. All rights reserved.
+ *
+ * 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_RESET_H_
+#define _CCU_RESET_H_
+
+#include <linux/reset-controller.h>
+
+struct ccu_reset_map {
+	u16	reg;
+	u32	bit;
+};
+
+
+struct ccu_reset {
+	void __iomem			*base;
+	struct ccu_reset_map		*reset_map;
+	spinlock_t			*lock;
+
+	struct reset_controller_dev	rcdev;
+};
+
+static inline struct ccu_reset *rcdev_to_ccu_reset(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct ccu_reset, rcdev);
+}
+
+extern const struct reset_control_ops ccu_reset_ops;
+
+#endif /* _CCU_RESET_H_ */