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

From: [email-masked] (Maxime Ripard)
Date: Sun, 15 May 2016 20:31:22 +0200

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
> <[email-masked]> wrote:
> > Start our new clock infrastructure by adding the registration code, common
> > structure and common code.
> >
> > Signed-off-by: Maxime Ripard <[email-masked]>
> > ---
> >  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 <[email-masked]>
> > + *
> > + * 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 <[email-masked]>
> > + *
> > + * 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160515/f96ad50b/attachment.sig>