From: Chen-Yu Tsai <wens@csie.org>
To: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Chen-Yu Tsai <wens@csie.org>,
Mike Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@codeaurora.org>,
linux-clk <linux-clk@vger.kernel.org>,
Hans de Goede <hdegoede@redhat.com>,
Andre Przywara <andre.przywara@arm.com>,
Rob Herring <robh+dt@kernel.org>,
Vishnu Patekar <vishnupatekar0510@gmail.com>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
Boris Brezillon <boris.brezillon@free-electrons.com>
Subject: Re: [PATCH 02/16] clk: sunxi-ng: Add common infrastructure
Date: Mon, 16 May 2016 15:02:13 +0800 [thread overview]
Message-ID: <CAGb2v64kgPOw3_fQDpZyBxW34s3QF6M4sfbgpMTduojb9FPhFQ@mail.gmail.com> (raw)
In-Reply-To: <20160515183122.GA27618@lukather>
On Mon, May 16, 2016 at 2:31 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> 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
>> <maxime.ripard@free-electrons.com> wrote:
>> > Start our new clock infrastructure by adding the registration code, common
>> > structure and common code.
>> >
>> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > ---
>> > 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 <maxime.ripard@free-electrons.com>
>> > + *
>> > + * 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 <maxime.ripard@free-electrons.com>
>> > + *
>> > + * 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
WARNING: multiple messages have this Message-ID (diff)
From: wens@csie.org (Chen-Yu Tsai)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 02/16] clk: sunxi-ng: Add common infrastructure
Date: Mon, 16 May 2016 15:02:13 +0800 [thread overview]
Message-ID: <CAGb2v64kgPOw3_fQDpZyBxW34s3QF6M4sfbgpMTduojb9FPhFQ@mail.gmail.com> (raw)
In-Reply-To: <20160515183122.GA27618@lukather>
On Mon, May 16, 2016 at 2:31 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> 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
>> <maxime.ripard@free-electrons.com> wrote:
>> > Start our new clock infrastructure by adding the registration code, common
>> > structure and common code.
>> >
>> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > ---
>> > 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 <maxime.ripard@free-electrons.com>
>> > + *
>> > + * 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 <maxime.ripard@free-electrons.com>
>> > + *
>> > + * 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 prev parent reply other threads:[~2016-05-16 7:02 UTC|newest]
Thread overview: 128+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-08 20:01 [PATCH 00/16] clk: sunxi: introduce "modern" clock support Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 01/16] clk: fix critical clock locking Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-09 22:11 ` Stephen Boyd
2016-05-09 22:11 ` Stephen Boyd
2016-05-13 7:50 ` Maxime Ripard
2016-05-13 7:50 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 02/16] clk: sunxi-ng: Add common infrastructure Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-09 10:01 ` Chen-Yu Tsai
2016-05-09 10:01 ` Chen-Yu Tsai
2016-05-15 18:31 ` Maxime Ripard
2016-05-15 18:31 ` Maxime Ripard
2016-05-16 7:02 ` Chen-Yu Tsai [this message]
2016-05-16 7:02 ` Chen-Yu Tsai
2016-05-16 8:02 ` Jean-Francois Moine
2016-05-16 8:02 ` Jean-Francois Moine
2016-05-16 20:15 ` Maxime Ripard
2016-05-16 20:15 ` Maxime Ripard
2016-05-17 6:54 ` Jean-Francois Moine
2016-05-17 6:54 ` Jean-Francois Moine
2016-05-18 19:59 ` Maxime Ripard
2016-05-18 19:59 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 03/16] clk: sunxi-ng: Add fixed factor clock support Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-09 10:05 ` Chen-Yu Tsai
2016-05-09 10:05 ` Chen-Yu Tsai
2016-05-16 13:15 ` Jean-Francois Moine
2016-05-16 13:15 ` Jean-Francois Moine
2016-05-16 21:08 ` Maxime Ripard
2016-05-16 21:08 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 04/16] clk: sunxi-ng: Add gate " Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 05/16] clk: sunxi-ng: Add mux " Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-21 16:18 ` Chen-Yu Tsai
2016-05-21 16:18 ` Chen-Yu Tsai
2016-05-22 19:20 ` Maxime Ripard
2016-05-22 19:20 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 06/16] clk: sunxi-ng: Add divider table clock Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-21 16:30 ` Chen-Yu Tsai
2016-05-21 16:30 ` Chen-Yu Tsai
2016-05-08 20:01 ` [PATCH 07/16] clk: sunxi-ng: Add phase clock support Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-21 16:43 ` Chen-Yu Tsai
2016-05-21 16:43 ` Chen-Yu Tsai
2016-05-23 17:01 ` Maxime Ripard
2016-05-23 17:01 ` Maxime Ripard
2016-05-24 9:01 ` Chen-Yu Tsai
2016-05-24 9:01 ` Chen-Yu Tsai
2016-05-08 20:01 ` [PATCH 08/16] clk: sunxi-ng: Add M-factor " Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-11 6:46 ` Jean-Francois Moine
2016-05-11 6:46 ` Jean-Francois Moine
2016-05-15 18:51 ` Maxime Ripard
2016-05-15 18:51 ` Maxime Ripard
2016-05-21 17:09 ` Chen-Yu Tsai
2016-05-21 17:09 ` Chen-Yu Tsai
2016-05-22 19:22 ` Maxime Ripard
2016-05-22 19:22 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 09/16] clk: sunxi-ng: Add P-factor " Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 10/16] clk: sunxi-ng: Add M-P factor " Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-23 13:45 ` Chen-Yu Tsai
2016-05-23 13:45 ` Chen-Yu Tsai
2016-05-23 17:18 ` Maxime Ripard
2016-05-23 17:18 ` Maxime Ripard
2016-05-24 4:14 ` Chen-Yu Tsai
2016-05-24 4:14 ` Chen-Yu Tsai
2016-05-24 21:07 ` Maxime Ripard
2016-05-24 21:07 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 11/16] clk: sunxi-ng: Add N-K-factor " Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-23 13:58 ` Chen-Yu Tsai
2016-05-23 13:58 ` Chen-Yu Tsai
2016-05-08 20:01 ` [PATCH 12/16] clk: sunxi-ng: Add N-M-factor " Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-09 7:24 ` Jean-Francois Moine
2016-05-09 7:24 ` Jean-Francois Moine
2016-05-15 19:04 ` Maxime Ripard
2016-05-15 19:04 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 13/16] clk: sunxi-ng: Add N-K-M Factor clock Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-11 8:45 ` Jean-Francois Moine
2016-05-11 8:45 ` Jean-Francois Moine
2016-05-15 19:08 ` Maxime Ripard
2016-05-15 19:08 ` Maxime Ripard
2016-05-23 14:10 ` Chen-Yu Tsai
2016-05-23 14:10 ` Chen-Yu Tsai
2016-05-08 20:01 ` [PATCH 14/16] clk: sunxi-ng: Add N-K-M-P factor clock Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-11 8:49 ` Jean-Francois Moine
2016-05-11 8:49 ` Jean-Francois Moine
2016-05-23 14:36 ` Chen-Yu Tsai
2016-05-23 14:36 ` Chen-Yu Tsai
2016-05-30 7:57 ` Maxime Ripard
2016-05-30 7:57 ` Maxime Ripard
2016-05-08 20:01 ` [PATCH 15/16] clk: sunxi-ng: Add H3 clocks Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
2016-05-09 7:39 ` Jean-Francois Moine
2016-05-09 7:39 ` Jean-Francois Moine
2016-05-15 19:18 ` Maxime Ripard
2016-05-15 19:18 ` Maxime Ripard
2016-05-13 9:45 ` Jean-Francois Moine
2016-05-13 9:45 ` Jean-Francois Moine
2016-05-18 14:02 ` Maxime Ripard
2016-05-18 14:02 ` Maxime Ripard
2016-05-18 16:23 ` Jean-Francois Moine
2016-05-18 16:23 ` Jean-Francois Moine
2016-05-18 16:27 ` Jean-Francois Moine
2016-05-18 16:27 ` Jean-Francois Moine
2016-05-16 13:47 ` Jean-Francois Moine
2016-05-16 13:47 ` Jean-Francois Moine
2016-05-18 21:20 ` Maxime Ripard
2016-05-18 21:20 ` Maxime Ripard
2016-05-30 16:15 ` Chen-Yu Tsai
2016-05-30 16:15 ` Chen-Yu Tsai
2016-06-01 19:19 ` Maxime Ripard
2016-06-01 19:19 ` Maxime Ripard
2016-06-03 6:42 ` Chen-Yu Tsai
2016-06-03 6:42 ` Chen-Yu Tsai
2016-06-03 6:55 ` Chen-Yu Tsai
2016-06-03 6:55 ` Chen-Yu Tsai
2016-05-08 20:01 ` [PATCH 16/16] ARM: dt: sun8i: switch the H3 to the new CCU driver Maxime Ripard
2016-05-08 20:01 ` Maxime Ripard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGb2v64kgPOw3_fQDpZyBxW34s3QF6M4sfbgpMTduojb9FPhFQ@mail.gmail.com \
--to=wens@csie.org \
--cc=andre.przywara@arm.com \
--cc=boris.brezillon@free-electrons.com \
--cc=hdegoede@redhat.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=maxime.ripard@free-electrons.com \
--cc=mturquette@baylibre.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@codeaurora.org \
--cc=vishnupatekar0510@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.