Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1668508 > unrolled thread
| Started by | Chunyan Zhang <chunyan.zhang@spreadtrum.com> |
|---|---|
| First post | 2017-06-18 04:10 +0200 |
| Last post | 2017-06-30 03:50 +0200 |
| Articles | 20 on this page of 24 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH V1 0/9] add clock driver for Spreadtrum platforms Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-06-18 04:10 +0200
[PATCH V1 2/9] clk: sprd: Add common infrastructure Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-06-18 04:10 +0200
Re: [PATCH V1 2/9] clk: sprd: Add common infrastructure Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:30 +0200
Re: [PATCH V1 2/9] clk: sprd: Add common infrastructure Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:20 +0200
[PATCH V1 9/9] arm64: dts: add ccu for SC9860 Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-06-18 04:10 +0200
Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:30 +0200
Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:30 +0200
Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 03:00 +0200
Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-30 09:40 +0200
[PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-06-18 04:10 +0200
Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:40 +0200
Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:20 +0200
Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Arnd Bergmann <arnd@arndb.de> - 2017-06-22 13:20 +0200
Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 14:10 +0200
Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 03:50 +0200
Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-30 10:00 +0200
Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 21:30 +0200
Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-07-03 09:50 +0200
Re: [PATCH V1 0/9] add clock driver for Spreadtrum platforms Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:30 +0200
Re: [PATCH V1 0/9] add clock driver for Spreadtrum platforms Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:10 +0200
Re: [PATCH V1 0/9] add clock driver for Spreadtrum platforms Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 02:50 +0200
Re: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860 Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:50 +0200
Re: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860 Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:30 +0200
Re: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860 Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 03:50 +0200
Page 1 of 2 [1] 2 Next page →
| From | Chunyan Zhang <chunyan.zhang@spreadtrum.com> |
|---|---|
| Date | 2017-06-18 04:10 +0200 |
| Subject | [PATCH V1 0/9] add clock driver for Spreadtrum platforms |
| Message-ID | <tTxP3-7p2-3@gated-at.bofh.it> |
In the last cycle, the patches support Whale2 sc9860 mobile chip have been merged. This patchset adds clock driver which is used on almost all Spreadtrum SoCs. This is a rewrite of Spreadtrum's original clock driver[1] according to the comments[2] from Stephen Boyd. This series also adds Spreadtrum clock binding documentation and devicetree data. Any comments would be greatly appreciated. Thanks, Chunyan [1] https://lwn.net/Articles/722739/ [2] https://www.spinics.net/lists/arm-kernel/msg582017.html Chunyan Zhang (9): dt-bindings: Add Spreadtrum CCU binding documentation clk: sprd: Add common infrastructure clk: sprd: add gate clock support clk: sprd: add mux clock support clk: sprd: add divider clock support clk: sprd: add composite clock support clk: sprd: add adjustable pll support clk: sprd: add clock support for SC9860 arm64: dts: add ccu for SC9860 .../devicetree/bindings/clock/sprd-ccu.txt | 46 + arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi | 67 + arch/arm64/boot/dts/sprd/sc9860.dtsi | 2 + arch/arm64/boot/dts/sprd/whale2.dtsi | 8 - drivers/clk/Makefile | 1 + drivers/clk/sprd/Makefile | 4 + drivers/clk/sprd/ccu-sc9860.c | 1742 ++++++++++++++++++++ drivers/clk/sprd/ccu-sc9860.h | 379 +++++ drivers/clk/sprd/ccu_common.c | 78 + drivers/clk/sprd/ccu_common.h | 90 + drivers/clk/sprd/ccu_composite.c | 62 + drivers/clk/sprd/ccu_composite.h | 47 + drivers/clk/sprd/ccu_div.c | 93 ++ drivers/clk/sprd/ccu_div.h | 77 + drivers/clk/sprd/ccu_gate.c | 102 ++ drivers/clk/sprd/ccu_gate.h | 73 + drivers/clk/sprd/ccu_mux.c | 82 + drivers/clk/sprd/ccu_mux.h | 63 + drivers/clk/sprd/ccu_pll.c | 241 +++ drivers/clk/sprd/ccu_pll.h | 123 ++ include/dt-bindings/clock/sc9860-ccu.h | 19 + 21 files changed, 3391 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/sprd-ccu.txt create mode 100644 arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi create mode 100644 drivers/clk/sprd/Makefile create mode 100644 drivers/clk/sprd/ccu-sc9860.c create mode 100644 drivers/clk/sprd/ccu-sc9860.h create mode 100644 drivers/clk/sprd/ccu_common.c create mode 100644 drivers/clk/sprd/ccu_common.h create mode 100644 drivers/clk/sprd/ccu_composite.c create mode 100644 drivers/clk/sprd/ccu_composite.h create mode 100644 drivers/clk/sprd/ccu_div.c create mode 100644 drivers/clk/sprd/ccu_div.h create mode 100644 drivers/clk/sprd/ccu_gate.c create mode 100644 drivers/clk/sprd/ccu_gate.h create mode 100644 drivers/clk/sprd/ccu_mux.c create mode 100644 drivers/clk/sprd/ccu_mux.h create mode 100644 drivers/clk/sprd/ccu_pll.c create mode 100644 drivers/clk/sprd/ccu_pll.h create mode 100644 include/dt-bindings/clock/sc9860-ccu.h -- 2.7.4
[toc] | [next] | [standalone]
| From | Chunyan Zhang <chunyan.zhang@spreadtrum.com> |
|---|---|
| Date | 2017-06-18 04:10 +0200 |
| Subject | [PATCH V1 2/9] clk: sprd: Add common infrastructure |
| Message-ID | <tTxP4-7p2-17@gated-at.bofh.it> |
| In reply to | #1668508 |
Added Spreadtrum's clock driver common structure and registration code.
Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
---
drivers/clk/Makefile | 1 +
drivers/clk/sprd/Makefile | 3 ++
drivers/clk/sprd/ccu_common.c | 78 +++++++++++++++++++++++++++++++++++++
drivers/clk/sprd/ccu_common.h | 90 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 172 insertions(+)
create mode 100644 drivers/clk/sprd/Makefile
create mode 100644 drivers/clk/sprd/ccu_common.c
create mode 100644 drivers/clk/sprd/ccu_common.h
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index c19983a..1d62721 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_COMMON_CLK_SAMSUNG) += samsung/
obj-$(CONFIG_ARCH_SIRF) += sirf/
obj-$(CONFIG_ARCH_SOCFPGA) += socfpga/
obj-$(CONFIG_PLAT_SPEAR) += spear/
+obj-$(CONFIG_ARCH_SPRD) += sprd/
obj-$(CONFIG_ARCH_STI) += st/
obj-$(CONFIG_ARCH_SUNXI) += sunxi/
obj-$(CONFIG_ARCH_SUNXI) += sunxi-ng/
diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
new file mode 100644
index 0000000..8f802b2
--- /dev/null
+++ b/drivers/clk/sprd/Makefile
@@ -0,0 +1,3 @@
+ifneq ($(CONFIG_OF),)
+obj-y += ccu_common.o
+endif
diff --git a/drivers/clk/sprd/ccu_common.c b/drivers/clk/sprd/ccu_common.c
new file mode 100644
index 0000000..911f4ba
--- /dev/null
+++ b/drivers/clk/sprd/ccu_common.c
@@ -0,0 +1,78 @@
+/*
+ * Spreadtrum clock infrastructure
+ *
+ * Copyright (C) 2017 Spreadtrum, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include "ccu_common.h"
+
+static inline void __iomem *ccu_find_base(struct ccu_addr_map *maps,
+ unsigned int num, unsigned int reg)
+{
+ int i;
+
+ for (i = 0; i < num; i++)
+ if ((reg & 0xffff0000) == maps[i].phy)
+ return maps[i].virt;
+
+ return 0;
+}
+
+int sprd_ccu_probe(struct device_node *node, struct ccu_addr_map *maps,
+ unsigned int count, const struct sprd_ccu_desc *desc)
+{
+ int i, ret = 0;
+ struct ccu_common *cclk;
+ struct clk_hw *hw;
+
+ for (i = 0; i < desc->num_ccu_clks; i++) {
+ cclk = desc->ccu_clks[i];
+ if (!cclk)
+ continue;
+
+ cclk->base = ccu_find_base(maps, count, cclk->reg);
+ if (!cclk->base) {
+ pr_err("%s: No mapped address found for clock(0x%x)\n",
+ __func__, cclk->reg);
+ return -EINVAL;
+ }
+ cclk->reg = cclk->reg & 0xffff;
+ }
+
+ for (i = 0; i < desc->hw_clks->num; i++) {
+
+ hw = desc->hw_clks->hws[i];
+
+ if (!hw)
+ continue;
+
+ ret = clk_hw_register(NULL, hw);
+ if (ret) {
+ pr_err("Couldn't register clock %d - %s\n",
+ i, hw->init->name);
+ goto err_clk_unreg;
+ }
+ }
+
+ ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
+ desc->hw_clks);
+ if (ret) {
+ pr_err("Failed to add clock provider.\n");
+ goto err_clk_unreg;
+ }
+
+ return 0;
+
+err_clk_unreg:
+ while (--i >= 0) {
+ hw = desc->hw_clks->hws[i];
+ if (!hw)
+ continue;
+
+ clk_hw_unregister(hw);
+ }
+
+ return ret;
+}
diff --git a/drivers/clk/sprd/ccu_common.h b/drivers/clk/sprd/ccu_common.h
new file mode 100644
index 0000000..ff07772
--- /dev/null
+++ b/drivers/clk/sprd/ccu_common.h
@@ -0,0 +1,90 @@
+/*
+ * Spreadtrum clock infrastructure
+ *
+ * Copyright (C) 2017 Spreadtrum, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#ifndef _CCU_COMMON_H_
+#define _CCU_COMMON_H_
+
+#include <linux/clk-provider.h>
+
+struct device_node;
+
+#define CLK_HW_INIT_NO_PARENT(_name, _ops, _flags) \
+ (&(struct clk_init_data) { \
+ .flags = _flags, \
+ .name = _name, \
+ .parent_names = NULL, \
+ .num_parents = 0, \
+ .ops = _ops, \
+ })
+
+#define CLK_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 CLK_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, \
+ })
+
+#define CLK_FIXED_FACTOR(_struct, _name, _parent, \
+ _div, _mult, _flags) \
+ struct clk_fixed_factor _struct = { \
+ .div = _div, \
+ .mult = _mult, \
+ .hw.init = CLK_HW_INIT(_name, \
+ _parent, \
+ &clk_fixed_factor_ops, \
+ _flags), \
+ }
+
+struct ccu_common {
+ void __iomem *base;
+ u32 reg;
+ spinlock_t *lock;
+ struct clk_hw hw;
+};
+
+struct ccu_addr_map {
+ phys_addr_t phy;
+ void __iomem *virt;
+};
+
+static inline u32 ccu_readl(struct ccu_common *common)
+{
+ return readl(common->base + common->reg);
+}
+
+static inline void ccu_writel(u32 val, struct ccu_common *common)
+{
+ writel(val, common->base + common->reg);
+}
+
+static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
+{
+ return container_of(hw, struct ccu_common, hw);
+}
+
+struct sprd_ccu_desc {
+ struct ccu_common **ccu_clks;
+ unsigned long num_ccu_clks;
+ struct clk_hw_onecell_data *hw_clks;
+};
+
+int sprd_ccu_probe(struct device_node *node, struct ccu_addr_map *maps,
+ unsigned int count, const struct sprd_ccu_desc *desc);
+
+#endif /* _CCU_COMMON_H_ */
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2017-06-20 03:30 +0200 |
| Subject | Re: [PATCH V1 2/9] clk: sprd: Add common infrastructure |
| Message-ID | <tUg9r-1X0-17@gated-at.bofh.it> |
| In reply to | #1668509 |
On 06/18, Chunyan Zhang wrote:
> Added Spreadtrum's clock driver common structure and registration code.
>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> ---
> drivers/clk/Makefile | 1 +
> drivers/clk/sprd/Makefile | 3 ++
> drivers/clk/sprd/ccu_common.c | 78 +++++++++++++++++++++++++++++++++++++
> drivers/clk/sprd/ccu_common.h | 90 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 172 insertions(+)
> create mode 100644 drivers/clk/sprd/Makefile
> create mode 100644 drivers/clk/sprd/ccu_common.c
> create mode 100644 drivers/clk/sprd/ccu_common.h
>
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index c19983a..1d62721 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_COMMON_CLK_SAMSUNG) += samsung/
> obj-$(CONFIG_ARCH_SIRF) += sirf/
> obj-$(CONFIG_ARCH_SOCFPGA) += socfpga/
> obj-$(CONFIG_PLAT_SPEAR) += spear/
> +obj-$(CONFIG_ARCH_SPRD) += sprd/
> obj-$(CONFIG_ARCH_STI) += st/
> obj-$(CONFIG_ARCH_SUNXI) += sunxi/
> obj-$(CONFIG_ARCH_SUNXI) += sunxi-ng/
> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
> new file mode 100644
> index 0000000..8f802b2
> --- /dev/null
> +++ b/drivers/clk/sprd/Makefile
> @@ -0,0 +1,3 @@
> +ifneq ($(CONFIG_OF),)
> +obj-y += ccu_common.o
> +endif
I'd prefer a Kconfig for SPRD clk drivers instead of this
CONFIG_OF check. Then we can compile test the sprd code in
configurations that don't have CONFIG_ARCH_SPRD set too.
> diff --git a/drivers/clk/sprd/ccu_common.c b/drivers/clk/sprd/ccu_common.c
> new file mode 100644
> index 0000000..911f4ba
> --- /dev/null
> +++ b/drivers/clk/sprd/ccu_common.c
> @@ -0,0 +1,78 @@
> +/*
> + * Spreadtrum clock infrastructure
> + *
> + * Copyright (C) 2017 Spreadtrum, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include "ccu_common.h"
> +
> +static inline void __iomem *ccu_find_base(struct ccu_addr_map *maps,
> + unsigned int num, unsigned int reg)
> +{
> + int i;
> +
> + for (i = 0; i < num; i++)
> + if ((reg & 0xffff0000) == maps[i].phy)
What is this?
> + return maps[i].virt;
> +
> + return 0;
> +}
> +
> +int sprd_ccu_probe(struct device_node *node, struct ccu_addr_map *maps,
> + unsigned int count, const struct sprd_ccu_desc *desc)
> +{
> + int i, ret = 0;
> + struct ccu_common *cclk;
> + struct clk_hw *hw;
> +
> + for (i = 0; i < desc->num_ccu_clks; i++) {
> + cclk = desc->ccu_clks[i];
> + if (!cclk)
> + continue;
> +
> + cclk->base = ccu_find_base(maps, count, cclk->reg);
> + if (!cclk->base) {
> + pr_err("%s: No mapped address found for clock(0x%x)\n",
> + __func__, cclk->reg);
> + return -EINVAL;
> + }
> + cclk->reg = cclk->reg & 0xffff;
> + }
> +
> + for (i = 0; i < desc->hw_clks->num; i++) {
> +
> + hw = desc->hw_clks->hws[i];
> +
> + if (!hw)
> + continue;
> +
> + ret = clk_hw_register(NULL, hw);
> + if (ret) {
> + pr_err("Couldn't register clock %d - %s\n",
> + i, hw->init->name);
> + goto err_clk_unreg;
> + }
> + }
> +
> + ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
> + desc->hw_clks);
> + if (ret) {
> + pr_err("Failed to add clock provider.\n");
> + goto err_clk_unreg;
> + }
> +
> + return 0;
> +
> +err_clk_unreg:
> + while (--i >= 0) {
> + hw = desc->hw_clks->hws[i];
> + if (!hw)
> + continue;
> +
> + clk_hw_unregister(hw);
> + }
> +
> + return ret;
> +}
> diff --git a/drivers/clk/sprd/ccu_common.h b/drivers/clk/sprd/ccu_common.h
> new file mode 100644
> index 0000000..ff07772
> --- /dev/null
> +++ b/drivers/clk/sprd/ccu_common.h
> @@ -0,0 +1,90 @@
> +/*
> + * Spreadtrum clock infrastructure
> + *
> + * Copyright (C) 2017 Spreadtrum, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#ifndef _CCU_COMMON_H_
> +#define _CCU_COMMON_H_
> +
> +#include <linux/clk-provider.h>
> +
> +struct device_node;
> +
> +#define CLK_HW_INIT_NO_PARENT(_name, _ops, _flags) \
> + (&(struct clk_init_data) { \
> + .flags = _flags, \
> + .name = _name, \
> + .parent_names = NULL, \
> + .num_parents = 0, \
> + .ops = _ops, \
> + })
> +
> +#define CLK_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 CLK_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, \
> + })
> +
> +#define CLK_FIXED_FACTOR(_struct, _name, _parent, \
> + _div, _mult, _flags) \
> + struct clk_fixed_factor _struct = { \
> + .div = _div, \
> + .mult = _mult, \
> + .hw.init = CLK_HW_INIT(_name, \
> + _parent, \
> + &clk_fixed_factor_ops, \
> + _flags), \
> + }
> +
> +struct ccu_common {
> + void __iomem *base;
> + u32 reg;
> + spinlock_t *lock;
> + struct clk_hw hw;
> +};
> +
> +struct ccu_addr_map {
> + phys_addr_t phy;
> + void __iomem *virt;
> +};
> +
> +static inline u32 ccu_readl(struct ccu_common *common)
> +{
> + return readl(common->base + common->reg);
> +}
> +
> +static inline void ccu_writel(u32 val, struct ccu_common *common)
> +{
> + writel(val, common->base + common->reg);
> +}
> +
> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
> +{
> + return container_of(hw, struct ccu_common, hw);
> +}
> +
> +struct sprd_ccu_desc {
> + struct ccu_common **ccu_clks;
> + unsigned long num_ccu_clks;
> + struct clk_hw_onecell_data *hw_clks;
> +};
> +
> +int sprd_ccu_probe(struct device_node *node, struct ccu_addr_map *maps,
> + unsigned int count, const struct sprd_ccu_desc *desc);
> +
> +#endif /* _CCU_COMMON_H_ */
Do you call them CCUs internally? I thought CCU was a sunxi
thing, so it may make more sense to call it whatever you call the
clock controller on your hardware.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Chunyan Zhang <zhang.lyra@gmail.com> |
|---|---|
| Date | 2017-06-22 12:20 +0200 |
| Subject | Re: [PATCH V1 2/9] clk: sprd: Add common infrastructure |
| Message-ID | <tV7ns-3hH-29@gated-at.bofh.it> |
| In reply to | #1670153 |
On 20 June 2017 at 09:29, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/18, Chunyan Zhang wrote:
>> Added Spreadtrum's clock driver common structure and registration code.
>>
>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
>> ---
>> drivers/clk/Makefile | 1 +
>> drivers/clk/sprd/Makefile | 3 ++
>> drivers/clk/sprd/ccu_common.c | 78 +++++++++++++++++++++++++++++++++++++
>> drivers/clk/sprd/ccu_common.h | 90 +++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 172 insertions(+)
>> create mode 100644 drivers/clk/sprd/Makefile
>> create mode 100644 drivers/clk/sprd/ccu_common.c
>> create mode 100644 drivers/clk/sprd/ccu_common.h
>>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index c19983a..1d62721 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -81,6 +81,7 @@ obj-$(CONFIG_COMMON_CLK_SAMSUNG) += samsung/
>> obj-$(CONFIG_ARCH_SIRF) += sirf/
>> obj-$(CONFIG_ARCH_SOCFPGA) += socfpga/
>> obj-$(CONFIG_PLAT_SPEAR) += spear/
>> +obj-$(CONFIG_ARCH_SPRD) += sprd/
>> obj-$(CONFIG_ARCH_STI) += st/
>> obj-$(CONFIG_ARCH_SUNXI) += sunxi/
>> obj-$(CONFIG_ARCH_SUNXI) += sunxi-ng/
>> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
>> new file mode 100644
>> index 0000000..8f802b2
>> --- /dev/null
>> +++ b/drivers/clk/sprd/Makefile
>> @@ -0,0 +1,3 @@
>> +ifneq ($(CONFIG_OF),)
>> +obj-y += ccu_common.o
>> +endif
>
> I'd prefer a Kconfig for SPRD clk drivers instead of this
> CONFIG_OF check. Then we can compile test the sprd code in
> configurations that don't have CONFIG_ARCH_SPRD set too.
Ok, make sense, will address this in the next version.
>
>> diff --git a/drivers/clk/sprd/ccu_common.c b/drivers/clk/sprd/ccu_common.c
>> new file mode 100644
>> index 0000000..911f4ba
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu_common.c
>> @@ -0,0 +1,78 @@
>> +/*
>> + * Spreadtrum clock infrastructure
>> + *
>> + * Copyright (C) 2017 Spreadtrum, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#include "ccu_common.h"
>> +
>> +static inline void __iomem *ccu_find_base(struct ccu_addr_map *maps,
>> + unsigned int num, unsigned int reg)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < num; i++)
>> + if ((reg & 0xffff0000) == maps[i].phy)
>
> What is this?
You can look at ccu-sc9860.c, different from sunxi-ng, we specify the
whole register address rather than register offset when initializing
Spreadtrum clocks, the high 16 bits is the base address which is
configured in DT, and is mapped in the board clock files like
ccu-sc9860.c, I will write more in commit message to explain this when
cooking next version of the patchset.
>
>> + return maps[i].virt;
>> +
>> + return 0;
>> +}
>> +
>> +int sprd_ccu_probe(struct device_node *node, struct ccu_addr_map *maps,
>> + unsigned int count, const struct sprd_ccu_desc *desc)
>> +{
>> + int i, ret = 0;
>> + struct ccu_common *cclk;
>> + struct clk_hw *hw;
>> +
>> + for (i = 0; i < desc->num_ccu_clks; i++) {
>> + cclk = desc->ccu_clks[i];
>> + if (!cclk)
>> + continue;
>> +
>> + cclk->base = ccu_find_base(maps, count, cclk->reg);
>> + if (!cclk->base) {
>> + pr_err("%s: No mapped address found for clock(0x%x)\n",
>> + __func__, cclk->reg);
>> + return -EINVAL;
>> + }
>> + cclk->reg = cclk->reg & 0xffff;
>> + }
>> +
>> + for (i = 0; i < desc->hw_clks->num; i++) {
>> +
>> + hw = desc->hw_clks->hws[i];
>> +
>> + if (!hw)
>> + continue;
>> +
>> + ret = clk_hw_register(NULL, hw);
>> + if (ret) {
>> + pr_err("Couldn't register clock %d - %s\n",
>> + i, hw->init->name);
>> + goto err_clk_unreg;
>> + }
>> + }
>> +
>> + ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
>> + desc->hw_clks);
>> + if (ret) {
>> + pr_err("Failed to add clock provider.\n");
>> + goto err_clk_unreg;
>> + }
>> +
>> + return 0;
>> +
>> +err_clk_unreg:
>> + while (--i >= 0) {
>> + hw = desc->hw_clks->hws[i];
>> + if (!hw)
>> + continue;
>> +
>> + clk_hw_unregister(hw);
>> + }
>> +
>> + return ret;
>> +}
>> diff --git a/drivers/clk/sprd/ccu_common.h b/drivers/clk/sprd/ccu_common.h
>> new file mode 100644
>> index 0000000..ff07772
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu_common.h
>> @@ -0,0 +1,90 @@
>> +/*
>> + * Spreadtrum clock infrastructure
>> + *
>> + * Copyright (C) 2017 Spreadtrum, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#ifndef _CCU_COMMON_H_
>> +#define _CCU_COMMON_H_
>> +
>> +#include <linux/clk-provider.h>
>> +
>> +struct device_node;
>> +
>> +#define CLK_HW_INIT_NO_PARENT(_name, _ops, _flags) \
>> + (&(struct clk_init_data) { \
>> + .flags = _flags, \
>> + .name = _name, \
>> + .parent_names = NULL, \
>> + .num_parents = 0, \
>> + .ops = _ops, \
>> + })
>> +
>> +#define CLK_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 CLK_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, \
>> + })
>> +
>> +#define CLK_FIXED_FACTOR(_struct, _name, _parent, \
>> + _div, _mult, _flags) \
>> + struct clk_fixed_factor _struct = { \
>> + .div = _div, \
>> + .mult = _mult, \
>> + .hw.init = CLK_HW_INIT(_name, \
>> + _parent, \
>> + &clk_fixed_factor_ops, \
>> + _flags), \
>> + }
>> +
>> +struct ccu_common {
>> + void __iomem *base;
>> + u32 reg;
>> + spinlock_t *lock;
>> + struct clk_hw hw;
>> +};
>> +
>> +struct ccu_addr_map {
>> + phys_addr_t phy;
>> + void __iomem *virt;
>> +};
>> +
>> +static inline u32 ccu_readl(struct ccu_common *common)
>> +{
>> + return readl(common->base + common->reg);
>> +}
>> +
>> +static inline void ccu_writel(u32 val, struct ccu_common *common)
>> +{
>> + writel(val, common->base + common->reg);
>> +}
>> +
>> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>> +{
>> + return container_of(hw, struct ccu_common, hw);
>> +}
>> +
>> +struct sprd_ccu_desc {
>> + struct ccu_common **ccu_clks;
>> + unsigned long num_ccu_clks;
>> + struct clk_hw_onecell_data *hw_clks;
>> +};
>> +
>> +int sprd_ccu_probe(struct device_node *node, struct ccu_addr_map *maps,
>> + unsigned int count, const struct sprd_ccu_desc *desc);
>> +
>> +#endif /* _CCU_COMMON_H_ */
>
> Do you call them CCUs internally? I thought CCU was a sunxi
> thing, so it may make more sense to call it whatever you call the
> clock controller on your hardware.
OK, will address that.
Thanks,
Chunyan
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Chunyan Zhang <chunyan.zhang@spreadtrum.com> |
|---|---|
| Date | 2017-06-18 04:10 +0200 |
| Subject | [PATCH V1 9/9] arm64: dts: add ccu for SC9860 |
| Message-ID | <tTxP4-7p2-21@gated-at.bofh.it> |
| In reply to | #1668508 |
Now we have clock driver, so add clock dt for SC9860 platform.
This patch also removed "ext-26m" from whale2.dtsi since it
is described in sc9860-ccu.dtsi.
Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
---
arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi | 67 ++++++++++++++++++++++++++++++++
arch/arm64/boot/dts/sprd/sc9860.dtsi | 2 +
arch/arm64/boot/dts/sprd/whale2.dtsi | 8 ----
3 files changed, 69 insertions(+), 8 deletions(-)
create mode 100644 arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi
diff --git a/arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi b/arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi
new file mode 100644
index 0000000..e15bf2d
--- /dev/null
+++ b/arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi
@@ -0,0 +1,67 @@
+/*
+ * Spreadtrum SC9860 SoC CCU
+ *
+ * Copyright (C) 2017, Spreadtrum Communications Inc.
+ *
+ * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+ */
+
+&soc {
+ ext_26m: ext-26m {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <26000000>;
+ clock-output-names = "ext-26m";
+ };
+
+ ext_32m_sine0: ext-32m-sine0 {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <32000000>;
+ clock-output-names = "ext-32m-sine0";
+ };
+
+ ext_32m_sine1: ext-32m-sine1 {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <32000000>;
+ clock-output-names = "ext-32m-sine1";
+ };
+
+ ext_rco_100m: ext-rco-100m {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <100000000>;
+ clock-output-names = "ext-rco-100m";
+ };
+
+ ext_32k: ext-32k {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <32768>;
+ clock-output-names = "ext-32k";
+ };
+
+ ccu: clk {
+ compatible = "sprd,sc9860-ccu";
+ #clock-cells = <1>;
+ reg = <0 0x20000000 0 0x400>,
+ <0 0x20210000 0 0x3000>,
+ <0 0x402b0000 0 0x4000>,
+ <0 0x402d0000 0 0x400>,
+ <0 0x402e0000 0 0x4000>,
+ <0 0x40400000 0 0x400>,
+ <0 0x40880000 0 0x400>,
+ <0 0x415e0000 0 0x400>,
+ <0 0x60200000 0 0x400>,
+ <0 0x61000000 0 0x400>,
+ <0 0x61100000 0 0x3000>,
+ <0 0x62000000 0 0x4000>,
+ <0 0x62100000 0 0x4000>,
+ <0 0x63000000 0 0x400>,
+ <0 0x63100000 0 0x3000>,
+ <0 0x70b00000 0 0x3000>;
+ clocks = <&ext_26m>, <&ext_rco_100m>, <&ext_32k>;
+ clock-names = "ext-26m", "ext-rco-100m", "ext-32k";
+ };
+};
diff --git a/arch/arm64/boot/dts/sprd/sc9860.dtsi b/arch/arm64/boot/dts/sprd/sc9860.dtsi
index 7b7d8ce..10ff7c6 100644
--- a/arch/arm64/boot/dts/sprd/sc9860.dtsi
+++ b/arch/arm64/boot/dts/sprd/sc9860.dtsi
@@ -7,7 +7,9 @@
*/
#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/clock/sc9860-ccu.h>
#include "whale2.dtsi"
+#include "sc9860-ccu.dtsi"
/ {
cpus {
diff --git a/arch/arm64/boot/dts/sprd/whale2.dtsi b/arch/arm64/boot/dts/sprd/whale2.dtsi
index 7c217c5..9d69b84 100644
--- a/arch/arm64/boot/dts/sprd/whale2.dtsi
+++ b/arch/arm64/boot/dts/sprd/whale2.dtsi
@@ -59,13 +59,5 @@
status = "disabled";
};
};
-
- };
-
- ext_26m: ext-26m {
- compatible = "fixed-clock";
- #clock-cells = <0>;
- clock-frequency = <26000000>;
- clock-output-names = "ext_26m";
};
};
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2017-06-20 03:30 +0200 |
| Subject | Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 |
| Message-ID | <tUg9r-1X0-9@gated-at.bofh.it> |
| In reply to | #1668510 |
On 06/18, Chunyan Zhang wrote:
> diff --git a/arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi b/arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi
> new file mode 100644
> index 0000000..e15bf2d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi
> @@ -0,0 +1,67 @@
> +/*
> + * Spreadtrum SC9860 SoC CCU
> + *
> + * Copyright (C) 2017, Spreadtrum Communications Inc.
> + *
> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> + */
> +
> +&soc {
> + ext_26m: ext-26m {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <26000000>;
> + clock-output-names = "ext-26m";
> + };
> +
> + ext_32m_sine0: ext-32m-sine0 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <32000000>;
> + clock-output-names = "ext-32m-sine0";
> + };
> +
> + ext_32m_sine1: ext-32m-sine1 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <32000000>;
> + clock-output-names = "ext-32m-sine1";
> + };
> +
> + ext_rco_100m: ext-rco-100m {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <100000000>;
> + clock-output-names = "ext-rco-100m";
> + };
> +
> + ext_32k: ext-32k {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <32768>;
> + clock-output-names = "ext-32k";
> + };
These should all be outside of the soc node as they're probably
on the board and not the SoC? The hint is that they don't have a
reg property.
> +
> + ccu: clk {
clock-controller is a more standard node name.
> + compatible = "sprd,sc9860-ccu";
> + #clock-cells = <1>;
> + reg = <0 0x20000000 0 0x400>,
> + <0 0x20210000 0 0x3000>,
> + <0 0x402b0000 0 0x4000>,
> + <0 0x402d0000 0 0x400>,
> + <0 0x402e0000 0 0x4000>,
> + <0 0x40400000 0 0x400>,
> + <0 0x40880000 0 0x400>,
> + <0 0x415e0000 0 0x400>,
> + <0 0x60200000 0 0x400>,
> + <0 0x61000000 0 0x400>,
> + <0 0x61100000 0 0x3000>,
> + <0 0x62000000 0 0x4000>,
> + <0 0x62100000 0 0x4000>,
> + <0 0x63000000 0 0x400>,
> + <0 0x63100000 0 0x3000>,
> + <0 0x70b00000 0 0x3000>;
There are a lot of reg properties here. Perhaps there needs to be
different nodes for the different clock controllers in this SoC?
> + clocks = <&ext_26m>, <&ext_rco_100m>, <&ext_32k>;
> + clock-names = "ext-26m", "ext-rco-100m", "ext-32k";
> + };
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Chunyan Zhang <zhang.lyra@gmail.com> |
|---|---|
| Date | 2017-06-22 12:30 +0200 |
| Subject | Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 |
| Message-ID | <tV7x8-3ll-11@gated-at.bofh.it> |
| In reply to | #1670149 |
Hi Stephen,
On 20 June 2017 at 09:24, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/18, Chunyan Zhang wrote:
>> diff --git a/arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi b/arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi
>> new file mode 100644
>> index 0000000..e15bf2d
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/sprd/sc9860-ccu.dtsi
>> @@ -0,0 +1,67 @@
>> +/*
>> + * Spreadtrum SC9860 SoC CCU
>> + *
>> + * Copyright (C) 2017, Spreadtrum Communications Inc.
>> + *
>> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> + */
>> +
>> +&soc {
>> + ext_26m: ext-26m {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <26000000>;
>> + clock-output-names = "ext-26m";
>> + };
>> +
>> + ext_32m_sine0: ext-32m-sine0 {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <32000000>;
>> + clock-output-names = "ext-32m-sine0";
>> + };
>> +
>> + ext_32m_sine1: ext-32m-sine1 {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <32000000>;
>> + clock-output-names = "ext-32m-sine1";
>> + };
>> +
>> + ext_rco_100m: ext-rco-100m {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <100000000>;
>> + clock-output-names = "ext-rco-100m";
>> + };
>> +
>> + ext_32k: ext-32k {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <32768>;
>> + clock-output-names = "ext-32k";
>> + };
>
> These should all be outside of the soc node as they're probably
> on the board and not the SoC? The hint is that they don't have a
> reg property.
>
>> +
>> + ccu: clk {
>
> clock-controller is a more standard node name.
OK, will address.
>
>> + compatible = "sprd,sc9860-ccu";
>> + #clock-cells = <1>;
>> + reg = <0 0x20000000 0 0x400>,
>> + <0 0x20210000 0 0x3000>,
>> + <0 0x402b0000 0 0x4000>,
>> + <0 0x402d0000 0 0x400>,
>> + <0 0x402e0000 0 0x4000>,
>> + <0 0x40400000 0 0x400>,
>> + <0 0x40880000 0 0x400>,
>> + <0 0x415e0000 0 0x400>,
>> + <0 0x60200000 0 0x400>,
>> + <0 0x61000000 0 0x400>,
>> + <0 0x61100000 0 0x3000>,
>> + <0 0x62000000 0 0x4000>,
>> + <0 0x62100000 0 0x4000>,
>> + <0 0x63000000 0 0x400>,
>> + <0 0x63100000 0 0x3000>,
>> + <0 0x70b00000 0 0x3000>;
>
> There are a lot of reg properties here. Perhaps there needs to be
> different nodes for the different clock controllers in this SoC?
>
On Spreadtrum's platform, clocks are basically located in a few
address areas due to some hardware design issue, that says there're
more than one kinds of clocks in one address range, and one kind of
clocks have more than one physical address bases, except ccu_pll and
ccu_div in this patchset.
We're planning to map the whole device area at one time before
initializing each of them, once that has been done and upstreamed, I
will remove these lists of addressed.
Thanks for your review,
Chunyan
>> + clocks = <&ext_26m>, <&ext_rco_100m>, <&ext_32k>;
>> + clock-names = "ext-26m", "ext-rco-100m", "ext-32k";
>> + };
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2017-06-30 03:00 +0200 |
| Subject | Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 |
| Message-ID | <tXSrT-6F8-5@gated-at.bofh.it> |
| In reply to | #1672509 |
On 06/22, Chunyan Zhang wrote: > Hi Stephen, > > On 20 June 2017 at 09:24, Stephen Boyd <sboyd@codeaurora.org> wrote: > > On 06/18, Chunyan Zhang wrote: > > > > >> + compatible = "sprd,sc9860-ccu"; > >> + #clock-cells = <1>; > >> + reg = <0 0x20000000 0 0x400>, > >> + <0 0x20210000 0 0x3000>, > >> + <0 0x402b0000 0 0x4000>, > >> + <0 0x402d0000 0 0x400>, > >> + <0 0x402e0000 0 0x4000>, > >> + <0 0x40400000 0 0x400>, > >> + <0 0x40880000 0 0x400>, > >> + <0 0x415e0000 0 0x400>, > >> + <0 0x60200000 0 0x400>, > >> + <0 0x61000000 0 0x400>, > >> + <0 0x61100000 0 0x3000>, > >> + <0 0x62000000 0 0x4000>, > >> + <0 0x62100000 0 0x4000>, > >> + <0 0x63000000 0 0x400>, > >> + <0 0x63100000 0 0x3000>, > >> + <0 0x70b00000 0 0x3000>; > > > > There are a lot of reg properties here. Perhaps there needs to be > > different nodes for the different clock controllers in this SoC? > > > > On Spreadtrum's platform, clocks are basically located in a few > address areas due to some hardware design issue, that says there're > more than one kinds of clocks in one address range, and one kind of > clocks have more than one physical address bases, except ccu_pll and > ccu_div in this patchset. > > We're planning to map the whole device area at one time before > initializing each of them, once that has been done and upstreamed, I > will remove these lists of addressed. Ok. Does this mean we need to wait for those patches to be sent out for review? Is it more like certain clks are embedded inside other devices like display controllers, i2c controllers, etc? Is there any more information I can get on this SoC? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Chunyan Zhang <zhang.lyra@gmail.com> |
|---|---|
| Date | 2017-06-30 09:40 +0200 |
| Subject | Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 |
| Message-ID | <tXYH1-2wp-29@gated-at.bofh.it> |
| In reply to | #1678360 |
Hi Stephen, Thanks for your every so clear and detailed answer, thank you. On 30 June 2017 at 08:57, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 06/22, Chunyan Zhang wrote: >> Hi Stephen, >> >> On 20 June 2017 at 09:24, Stephen Boyd <sboyd@codeaurora.org> wrote: >> > On 06/18, Chunyan Zhang wrote: >> >> > >> >> + compatible = "sprd,sc9860-ccu"; >> >> + #clock-cells = <1>; >> >> + reg = <0 0x20000000 0 0x400>, >> >> + <0 0x20210000 0 0x3000>, >> >> + <0 0x402b0000 0 0x4000>, >> >> + <0 0x402d0000 0 0x400>, >> >> + <0 0x402e0000 0 0x4000>, >> >> + <0 0x40400000 0 0x400>, >> >> + <0 0x40880000 0 0x400>, >> >> + <0 0x415e0000 0 0x400>, >> >> + <0 0x60200000 0 0x400>, >> >> + <0 0x61000000 0 0x400>, >> >> + <0 0x61100000 0 0x3000>, >> >> + <0 0x62000000 0 0x4000>, >> >> + <0 0x62100000 0 0x4000>, >> >> + <0 0x63000000 0 0x400>, >> >> + <0 0x63100000 0 0x3000>, >> >> + <0 0x70b00000 0 0x3000>; >> > >> > There are a lot of reg properties here. Perhaps there needs to be >> > different nodes for the different clock controllers in this SoC? >> > >> >> On Spreadtrum's platform, clocks are basically located in a few >> address areas due to some hardware design issue, that says there're >> more than one kinds of clocks in one address range, and one kind of >> clocks have more than one physical address bases, except ccu_pll and >> ccu_div in this patchset. >> >> We're planning to map the whole device area at one time before >> initializing each of them, once that has been done and upstreamed, I >> will remove these lists of addressed. > > Ok. Does this mean we need to wait for those patches to be sent I don't think that would come out for review in the near future, so I have to keep these ranges of the address listed here for the time being. > out for review? Is it more like certain clks are embedded inside > other devices like display controllers, i2c controllers, etc? Is From what I understand, that's just something like you said. > there any more information I can get on this SoC? I think you may get more information from our dts files [1], if you would like to :) Thanks again, Chunyan [1] https://github.com/sprdlinux/kernel/blob/sp9860g-1h10/arch/arm64/boot/dts/sprd/whale.dtsi > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Chunyan Zhang <chunyan.zhang@spreadtrum.com> |
|---|---|
| Date | 2017-06-18 04:10 +0200 |
| Subject | [PATCH V1 7/9] clk: sprd: add adjustable pll support |
| Message-ID | <tTxP4-7p2-19@gated-at.bofh.it> |
| In reply to | #1668508 |
Introduced a common adjustable pll clock driver for Spreadtrum SoCs.
Original-by: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
drivers/clk/sprd/Makefile | 2 +-
drivers/clk/sprd/ccu_pll.c | 241 +++++++++++++++++++++++++++++++++++++++++++++
drivers/clk/sprd/ccu_pll.h | 123 +++++++++++++++++++++++
3 files changed, 365 insertions(+), 1 deletion(-)
create mode 100644 drivers/clk/sprd/ccu_pll.c
create mode 100644 drivers/clk/sprd/ccu_pll.h
diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
index 83232e5..c593a93 100644
--- a/drivers/clk/sprd/Makefile
+++ b/drivers/clk/sprd/Makefile
@@ -1,3 +1,3 @@
ifneq ($(CONFIG_OF),)
-obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o
+obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o
endif
diff --git a/drivers/clk/sprd/ccu_pll.c b/drivers/clk/sprd/ccu_pll.c
new file mode 100644
index 0000000..6c908e4
--- /dev/null
+++ b/drivers/clk/sprd/ccu_pll.c
@@ -0,0 +1,241 @@
+/*
+ * Spreadtrum pll clock driver
+ *
+ * Copyright (C) 2015~2017 Spreadtrum, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <linux/delay.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+
+#include "ccu_pll.h"
+
+#define CCU_PLL_1M 1000000
+#define CCU_PLL_10M (CCU_PLL_1M * 10)
+
+#define pindex(pll, member) \
+ (pll->factors[member].shift / (8 * sizeof(pll->regs[0])))
+
+#define pshift(pll, member) \
+ (pll->factors[member].shift % (8 * sizeof(pll->regs[0])))
+
+#define pwidth(pll, member) \
+ pll->factors[member].width
+
+#define pmask(pll, member) \
+ ((pwidth(pll, member)) ? \
+ GENMASK(pwidth(pll, member) + pshift(pll, member) - 1, \
+ pshift(pll, member)) : 0)
+
+#define pinternal(pll, cfg, member) \
+ (cfg[pindex(pll, member)] & pmask(pll, member))
+
+#define pinternal_val(pll, cfg, member) \
+ (pinternal(pll, cfg, member) >> pshift(pll, member))
+
+static unsigned long pll_get_refin_rate(struct ccu_pll *pll)
+{
+ u8 shift, index, refin_id = 3;
+ u32 mask;
+ const unsigned long refin[4] = { 2, 4, 13, 26 };
+
+ if (pwidth(pll, PLL_REFIN)) {
+ index = pindex(pll, PLL_REFIN);
+ shift = pshift(pll, PLL_REFIN);
+ mask = pmask(pll, PLL_REFIN);
+ refin_id = (ccu_pll_readl(pll, index) & mask) >> shift;
+ if (refin_id > 3)
+ refin_id = 3;
+ }
+
+ return refin[refin_id];
+}
+
+static u8 pll_get_ibias(unsigned long rate, const u64 *table)
+{
+ u64 i;
+ u8 num = table[0];
+
+ for (i = 0; i < num; i++)
+ if (rate <= table[i + 1])
+ break;
+
+ return i == num ? num - 1 : i;
+}
+
+static unsigned long ccu_pll_helper_recalc_rate(struct ccu_pll *pll,
+ unsigned long parent_rate)
+{
+ unsigned long rate, refin, k1, k2;
+ unsigned long kint = 0, nint;
+ u32 reg_num = pll->regs[0];
+ u32 *cfg;
+ u32 i;
+ u32 mask;
+
+ cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
+ if (!cfg)
+ return -ENOMEM;
+
+ for (i = 0; i < reg_num; i++)
+ cfg[i] = ccu_pll_readl(pll, i);
+
+ refin = pll_get_refin_rate(pll);
+
+ if (pinternal(pll, cfg, PLL_PREDIV))
+ refin = refin * 2;
+
+ if (pwidth(pll, PLL_POSTDIV) &&
+ ((pll->fflag == 1 && pinternal(pll, cfg, PLL_POSTDIV)) ||
+ (!pll->fflag && !pinternal(pll, cfg, PLL_POSTDIV))))
+ refin = refin / 2;
+
+ if (!pinternal(pll, cfg, PLL_DIV_S))
+ rate = refin * pinternal_val(pll, cfg, PLL_N) * CCU_PLL_10M;
+ else {
+ nint = pinternal_val(pll, cfg, PLL_NINT);
+ if (pinternal(pll, cfg, PLL_SDM_EN))
+ kint = pinternal_val(pll, cfg, PLL_KINT);
+
+ mask = pmask(pll, PLL_KINT);
+#ifdef CONFIG_64BIT
+ k1 = 1000;
+ k2 = 1000;
+ rate = DIV_ROUND_CLOSEST(refin * kint * k1,
+ ((mask >> __ffs(mask)) + 1)) *
+ k2 + refin * nint * CCU_PLL_1M;
+#else
+ k1 = 100;
+ k2 = 10000;
+ i = pwidth(pll, PLL_KINT);
+ i = i < 21 ? 0 : i - 21;
+ rate = DIV_ROUND_CLOSEST(refin * (kint >> i) * k1,
+ ((mask >> (__ffs(mask) + i)) + 1)) *
+ k2 + refin * nint * CCU_PLL_1M;
+#endif
+ }
+
+ return rate;
+}
+
+static int ccu_pll_helper_set_rate(struct ccu_pll *pll,
+ unsigned long rate,
+ unsigned long parent_rate)
+{
+ u32 mask, shift, width, ibias_val, index, kint, nint;
+ u32 reg_num = pll->regs[0], i = 0;
+ unsigned long refin, fvco = rate;
+ struct reg_cfg *cfg;
+
+ cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
+ if (!cfg)
+ return -ENOMEM;
+
+ refin = pll_get_refin_rate(pll);
+
+ mask = pmask(pll, PLL_PREDIV);
+ index = pindex(pll, PLL_PREDIV);
+ width = pwidth(pll, PLL_PREDIV);
+ if (width && (ccu_pll_readl(pll, index) & mask))
+ refin = refin * 2;
+
+ mask = pmask(pll, PLL_POSTDIV);
+ index = pindex(pll, PLL_POSTDIV);
+ width = pwidth(pll, PLL_POSTDIV);
+ cfg[index].msk = mask;
+ if (width && ((pll->fflag == 1 && fvco <= pll->fvco) ||
+ (pll->fflag == 0 && fvco > pll->fvco)))
+ cfg[index].val |= mask;
+
+ if (width && fvco <= pll->fvco)
+ fvco = fvco * 2;
+
+ mask = pmask(pll, PLL_DIV_S);
+ index = pindex(pll, PLL_DIV_S);
+ cfg[index].val |= mask;
+ cfg[index].msk |= mask;
+
+ mask = pmask(pll, PLL_SDM_EN);
+ index = pindex(pll, PLL_SDM_EN);
+ cfg[index].val |= mask;
+ cfg[index].msk |= mask;
+
+ nint = fvco/(refin * CCU_PLL_1M);
+
+ mask = pmask(pll, PLL_NINT);
+ index = pindex(pll, PLL_NINT);
+ shift = pshift(pll, PLL_NINT);
+ cfg[index].val |= (nint << shift) & mask;
+ cfg[index].msk |= mask;
+
+ mask = pmask(pll, PLL_KINT);
+ index = pindex(pll, PLL_KINT);
+ width = pwidth(pll, PLL_KINT);
+ shift = pshift(pll, PLL_KINT);
+#ifndef CONFIG_64BIT
+ i = width < 21 ? 0 : i - 21;
+#endif
+ kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) *
+ ((mask >> (shift + i)) + 1), refin * 100) << i;
+ cfg[index].val |= (kint << shift) & mask;
+ cfg[index].msk |= mask;
+
+ ibias_val = pll_get_ibias(fvco, pll->itable);
+
+ mask = pmask(pll, PLL_IBIAS);
+ index = pindex(pll, PLL_IBIAS);
+ shift = pshift(pll, PLL_IBIAS);
+ cfg[index].val |= ibias_val << shift & mask;
+ cfg[index].msk |= mask;
+
+ for (i = 0; i < reg_num; i++) {
+ if (cfg[i].msk)
+ ccu_pll_writel(pll, i, cfg[i].val, cfg[i].msk);
+ }
+
+ udelay(pll->udelay);
+
+ return 0;
+}
+
+static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct ccu_pll *pll = hw_to_ccu_pll(hw);
+
+ return ccu_pll_helper_recalc_rate(pll, parent_rate);
+}
+
+static int ccu_pll_set_rate(struct clk_hw *hw,
+ unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct ccu_pll *pll = hw_to_ccu_pll(hw);
+
+ return ccu_pll_helper_set_rate(pll, rate, parent_rate);
+}
+
+static int ccu_pll_clk_prepare(struct clk_hw *hw)
+{
+ struct ccu_pll *pll = hw_to_ccu_pll(hw);
+
+ udelay(pll->udelay);
+
+ return 0;
+}
+
+static long ccu_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *prate)
+{
+ return rate;
+}
+
+const struct clk_ops ccu_pll_ops = {
+ .prepare = ccu_pll_clk_prepare,
+ .recalc_rate = ccu_pll_recalc_rate,
+ .round_rate = ccu_pll_round_rate,
+ .set_rate = ccu_pll_set_rate,
+};
diff --git a/drivers/clk/sprd/ccu_pll.h b/drivers/clk/sprd/ccu_pll.h
new file mode 100644
index 0000000..66fe5d1
--- /dev/null
+++ b/drivers/clk/sprd/ccu_pll.h
@@ -0,0 +1,123 @@
+/*
+ * Spreadtrum clock pll configurations
+ *
+ * Copyright (C) 2015~2017 Spreadtrum, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#ifndef _CCU_PLL_H_
+#define _CCU_PLL_H_
+
+#include "ccu_common.h"
+
+struct reg_cfg {
+ u32 val;
+ u32 msk;
+};
+
+struct ccu_bit_field {
+ u8 shift;
+ u8 width;
+};
+
+enum {
+ PLL_LOCK_DONE = 0,
+ PLL_DIV_S,
+ PLL_MOD_EN,
+ PLL_SDM_EN,
+ PLL_REFIN,
+ PLL_IBIAS,
+ PLL_N,
+ PLL_NINT,
+ PLL_KINT,
+ PLL_PREDIV,
+ PLL_POSTDIV,
+
+ PLL_FACT_MAX
+};
+
+/*
+ * struct ccu_pll - defination of adjustable pll clock
+ *
+ * @reg: registers used to set the configuration of pll clock,
+ * reg[0] shows how many registers this pll clock uses.
+ * @itable: pll ibias table, itable[0] means how many items this
+ * table includes
+ * @udelay delay time after setting rate
+ * @factors used to calculate the pll clock rate
+ * @fvco: fvco threshold rate
+ * @fflag: fvco flag
+ */
+struct ccu_pll {
+ const u32 *regs;
+ const u64 *itable;
+ u16 udelay;
+ const struct ccu_bit_field *factors;
+ u64 fvco;
+ u16 fflag;
+
+ struct ccu_common common;
+};
+
+#define SPRD_CCU_PLL_WITH_ITABLE_FVCO(_struct, _name, _parent, _reg, \
+ _regs, _itable, _udelay, \
+ _factors, _fvco, _fflag) \
+ struct ccu_pll _struct = { \
+ .regs = _regs, \
+ .itable = _itable, \
+ .udelay = _udelay, \
+ .factors = _factors, \
+ .fvco = _fvco, \
+ .fflag = _fflag, \
+ .common = { \
+ .reg = _reg, \
+ .hw.init = CLK_HW_INIT(_name, \
+ _parent, \
+ &ccu_pll_ops, \
+ CLK_IGNORE_UNUSED), \
+ }, \
+ }
+
+#define SPRD_CCU_PLL_WITH_ITABLE(_struct, _name, _parent, _reg, \
+ _regs, _itable, _udelay, \
+ _factors) \
+ SPRD_CCU_PLL_WITH_ITABLE_FVCO(_struct, _name, _parent, _reg, \
+ _regs, _itable, _udelay, \
+ _factors, 0, 0)
+
+static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw)
+{
+ struct ccu_common *common = hw_to_ccu_common(hw);
+
+ return container_of(common, struct ccu_pll, common);
+}
+
+static inline u32 ccu_pll_readl(struct ccu_pll *pll, u8 index)
+{
+ struct ccu_common *common = &pll->common;
+
+ if (WARN_ON(index >= pll->regs[0]))
+ return 0;
+
+ return readl(common->base + pll->regs[index + 1]);
+}
+
+static inline void ccu_pll_writel(struct ccu_pll *pll, u8 index,
+ u32 val, u32 msk)
+{
+ struct ccu_common *common = &pll->common;
+ void __iomem *addr;
+ u32 reg;
+
+ if (WARN_ON(index >= pll->regs[0]))
+ return;
+
+ addr = common->base + pll->regs[index + 1];
+ reg = readl(addr);
+ writel((reg & ~msk) | val, addr);
+}
+
+extern const struct clk_ops ccu_pll_ops;
+
+#endif /* _CCU_PLL_H_ */
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2017-06-20 03:40 +0200 |
| Subject | Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support |
| Message-ID | <tUgj8-20K-19@gated-at.bofh.it> |
| In reply to | #1668511 |
On 06/18, Chunyan Zhang wrote:
> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
> index 83232e5..c593a93 100644
> --- a/drivers/clk/sprd/Makefile
> +++ b/drivers/clk/sprd/Makefile
> @@ -1,3 +1,3 @@
> ifneq ($(CONFIG_OF),)
> -obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o
> +obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o
> endif
> diff --git a/drivers/clk/sprd/ccu_pll.c b/drivers/clk/sprd/ccu_pll.c
> new file mode 100644
> index 0000000..6c908e4
> --- /dev/null
> +++ b/drivers/clk/sprd/ccu_pll.c
> @@ -0,0 +1,241 @@
> +/*
> + * Spreadtrum pll clock driver
> + *
> + * Copyright (C) 2015~2017 Spreadtrum, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/clk.h>
Is this include used? Should be clk-provider?
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +
> +#include "ccu_pll.h"
> +
> +#define CCU_PLL_1M 1000000
> +#define CCU_PLL_10M (CCU_PLL_1M * 10)
> +
> +#define pindex(pll, member) \
> + (pll->factors[member].shift / (8 * sizeof(pll->regs[0])))
> +
> +#define pshift(pll, member) \
> + (pll->factors[member].shift % (8 * sizeof(pll->regs[0])))
> +
> +#define pwidth(pll, member) \
> + pll->factors[member].width
> +
> +#define pmask(pll, member) \
> + ((pwidth(pll, member)) ? \
> + GENMASK(pwidth(pll, member) + pshift(pll, member) - 1, \
> + pshift(pll, member)) : 0)
> +
> +#define pinternal(pll, cfg, member) \
> + (cfg[pindex(pll, member)] & pmask(pll, member))
> +
> +#define pinternal_val(pll, cfg, member) \
> + (pinternal(pll, cfg, member) >> pshift(pll, member))
> +
> +static unsigned long pll_get_refin_rate(struct ccu_pll *pll)
pll could be const?
> +{
> + u8 shift, index, refin_id = 3;
> + u32 mask;
> + const unsigned long refin[4] = { 2, 4, 13, 26 };
> +
> + if (pwidth(pll, PLL_REFIN)) {
> + index = pindex(pll, PLL_REFIN);
> + shift = pshift(pll, PLL_REFIN);
> + mask = pmask(pll, PLL_REFIN);
> + refin_id = (ccu_pll_readl(pll, index) & mask) >> shift;
> + if (refin_id > 3)
> + refin_id = 3;
> + }
> +
> + return refin[refin_id];
> +}
> +
> +static u8 pll_get_ibias(unsigned long rate, const u64 *table)
> +{
> + u64 i;
> + u8 num = table[0];
> +
> + for (i = 0; i < num; i++)
> + if (rate <= table[i + 1])
> + break;
> +
> + return i == num ? num - 1 : i;
> +}
> +
> +static unsigned long ccu_pll_helper_recalc_rate(struct ccu_pll *pll,
> + unsigned long parent_rate)
> +{
> + unsigned long rate, refin, k1, k2;
> + unsigned long kint = 0, nint;
> + u32 reg_num = pll->regs[0];
> + u32 *cfg;
> + u32 i;
> + u32 mask;
> +
> + cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
> + if (!cfg)
> + return -ENOMEM;
> +
> + for (i = 0; i < reg_num; i++)
> + cfg[i] = ccu_pll_readl(pll, i);
> +
> + refin = pll_get_refin_rate(pll);
> +
> + if (pinternal(pll, cfg, PLL_PREDIV))
> + refin = refin * 2;
> +
> + if (pwidth(pll, PLL_POSTDIV) &&
> + ((pll->fflag == 1 && pinternal(pll, cfg, PLL_POSTDIV)) ||
> + (!pll->fflag && !pinternal(pll, cfg, PLL_POSTDIV))))
> + refin = refin / 2;
> +
> + if (!pinternal(pll, cfg, PLL_DIV_S))
> + rate = refin * pinternal_val(pll, cfg, PLL_N) * CCU_PLL_10M;
> + else {
Please include braces on the if as well when another branch has them.
> + nint = pinternal_val(pll, cfg, PLL_NINT);
> + if (pinternal(pll, cfg, PLL_SDM_EN))
> + kint = pinternal_val(pll, cfg, PLL_KINT);
> +
> + mask = pmask(pll, PLL_KINT);
> +#ifdef CONFIG_64BIT
> + k1 = 1000;
> + k2 = 1000;
> + rate = DIV_ROUND_CLOSEST(refin * kint * k1,
> + ((mask >> __ffs(mask)) + 1)) *
> + k2 + refin * nint * CCU_PLL_1M;
> +#else
> + k1 = 100;
> + k2 = 10000;
> + i = pwidth(pll, PLL_KINT);
> + i = i < 21 ? 0 : i - 21;
> + rate = DIV_ROUND_CLOSEST(refin * (kint >> i) * k1,
> + ((mask >> (__ffs(mask) + i)) + 1)) *
> + k2 + refin * nint * CCU_PLL_1M;
> +#endif
> + }
> +
> + return rate;
> +}
> +
> +static int ccu_pll_helper_set_rate(struct ccu_pll *pll,
> + unsigned long rate,
> + unsigned long parent_rate)
> +{
> + u32 mask, shift, width, ibias_val, index, kint, nint;
> + u32 reg_num = pll->regs[0], i = 0;
> + unsigned long refin, fvco = rate;
> + struct reg_cfg *cfg;
> +
> + cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
> + if (!cfg)
> + return -ENOMEM;
> +
> + refin = pll_get_refin_rate(pll);
> +
> + mask = pmask(pll, PLL_PREDIV);
> + index = pindex(pll, PLL_PREDIV);
> + width = pwidth(pll, PLL_PREDIV);
> + if (width && (ccu_pll_readl(pll, index) & mask))
> + refin = refin * 2;
> +
> + mask = pmask(pll, PLL_POSTDIV);
> + index = pindex(pll, PLL_POSTDIV);
> + width = pwidth(pll, PLL_POSTDIV);
> + cfg[index].msk = mask;
> + if (width && ((pll->fflag == 1 && fvco <= pll->fvco) ||
> + (pll->fflag == 0 && fvco > pll->fvco)))
> + cfg[index].val |= mask;
> +
> + if (width && fvco <= pll->fvco)
> + fvco = fvco * 2;
> +
> + mask = pmask(pll, PLL_DIV_S);
> + index = pindex(pll, PLL_DIV_S);
> + cfg[index].val |= mask;
> + cfg[index].msk |= mask;
> +
> + mask = pmask(pll, PLL_SDM_EN);
> + index = pindex(pll, PLL_SDM_EN);
> + cfg[index].val |= mask;
> + cfg[index].msk |= mask;
> +
> + nint = fvco/(refin * CCU_PLL_1M);
> +
> + mask = pmask(pll, PLL_NINT);
> + index = pindex(pll, PLL_NINT);
> + shift = pshift(pll, PLL_NINT);
> + cfg[index].val |= (nint << shift) & mask;
> + cfg[index].msk |= mask;
> +
> + mask = pmask(pll, PLL_KINT);
> + index = pindex(pll, PLL_KINT);
> + width = pwidth(pll, PLL_KINT);
> + shift = pshift(pll, PLL_KINT);
> +#ifndef CONFIG_64BIT
> + i = width < 21 ? 0 : i - 21;
> +#endif
What's this? Why do we depend on CONFIG_64BIT?
> + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) *
> + ((mask >> (shift + i)) + 1), refin * 100) << i;
> + cfg[index].val |= (kint << shift) & mask;
> + cfg[index].msk |= mask;
> +
> + ibias_val = pll_get_ibias(fvco, pll->itable);
> +
> + mask = pmask(pll, PLL_IBIAS);
> + index = pindex(pll, PLL_IBIAS);
> + shift = pshift(pll, PLL_IBIAS);
> + cfg[index].val |= ibias_val << shift & mask;
> + cfg[index].msk |= mask;
> +
> + for (i = 0; i < reg_num; i++) {
> + if (cfg[i].msk)
> + ccu_pll_writel(pll, i, cfg[i].val, cfg[i].msk);
> + }
> +
Are we waiting for the writel() to go through above? If so we
need a readl() of the same register to make sure the write has
completed before delaying.
> + udelay(pll->udelay);
> +
> + return 0;
> +}
> +
> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct ccu_pll *pll = hw_to_ccu_pll(hw);
> +
> + return ccu_pll_helper_recalc_rate(pll, parent_rate);
> +}
> +
> +static int ccu_pll_set_rate(struct clk_hw *hw,
> + unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct ccu_pll *pll = hw_to_ccu_pll(hw);
> +
> + return ccu_pll_helper_set_rate(pll, rate, parent_rate);
> +}
> +
> +static int ccu_pll_clk_prepare(struct clk_hw *hw)
> +{
> + struct ccu_pll *pll = hw_to_ccu_pll(hw);
> +
> + udelay(pll->udelay);
> +
> + return 0;
> +}
> +
> +static long ccu_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *prate)
> +{
> + return rate;
> +}
> +
> +const struct clk_ops ccu_pll_ops = {
> + .prepare = ccu_pll_clk_prepare,
> + .recalc_rate = ccu_pll_recalc_rate,
> + .round_rate = ccu_pll_round_rate,
> + .set_rate = ccu_pll_set_rate,
> +};
> diff --git a/drivers/clk/sprd/ccu_pll.h b/drivers/clk/sprd/ccu_pll.h
> new file mode 100644
> index 0000000..66fe5d1
> --- /dev/null
> +++ b/drivers/clk/sprd/ccu_pll.h
> @@ -0,0 +1,123 @@
> +/*
> + * Spreadtrum clock pll configurations
> + *
> + * Copyright (C) 2015~2017 Spreadtrum, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#ifndef _CCU_PLL_H_
> +#define _CCU_PLL_H_
> +
> +#include "ccu_common.h"
> +
> +struct reg_cfg {
> + u32 val;
> + u32 msk;
> +};
> +
> +struct ccu_bit_field {
> + u8 shift;
> + u8 width;
> +};
> +
> +enum {
> + PLL_LOCK_DONE = 0,
Drop the = 0 please unless it's needed for something?
> + PLL_DIV_S,
> + PLL_MOD_EN,
> + PLL_SDM_EN,
> + PLL_REFIN,
> + PLL_IBIAS,
> + PLL_N,
> + PLL_NINT,
> + PLL_KINT,
> + PLL_PREDIV,
> + PLL_POSTDIV,
> +
> + PLL_FACT_MAX
> +};
> +
> +/*
> + * struct ccu_pll - defination of adjustable pll clock
s/defination/definition/
> + *
> + * @reg: registers used to set the configuration of pll clock,
> + * reg[0] shows how many registers this pll clock uses.
> + * @itable: pll ibias table, itable[0] means how many items this
> + * table includes
> + * @udelay delay time after setting rate
> + * @factors used to calculate the pll clock rate
> + * @fvco: fvco threshold rate
> + * @fflag: fvco flag
> + */
> +struct ccu_pll {
> + const u32 *regs;
> + const u64 *itable;
> + u16 udelay;
> + const struct ccu_bit_field *factors;
Does this change across the different PLLs? Would be nice to not
need the bit field thing.
> + u64 fvco;
> + u16 fflag;
> +
> + struct ccu_common common;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Chunyan Zhang <zhang.lyra@gmail.com> |
|---|---|
| Date | 2017-06-22 12:20 +0200 |
| Subject | Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support |
| Message-ID | <tV7ns-3hH-23@gated-at.bofh.it> |
| In reply to | #1670161 |
On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/18, Chunyan Zhang wrote:
>> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
>> index 83232e5..c593a93 100644
>> --- a/drivers/clk/sprd/Makefile
>> +++ b/drivers/clk/sprd/Makefile
>> @@ -1,3 +1,3 @@
>> ifneq ($(CONFIG_OF),)
>> -obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o
>> +obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o
>> endif
>> diff --git a/drivers/clk/sprd/ccu_pll.c b/drivers/clk/sprd/ccu_pll.c
>> new file mode 100644
>> index 0000000..6c908e4
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu_pll.c
>> @@ -0,0 +1,241 @@
>> +/*
>> + * Spreadtrum pll clock driver
>> + *
>> + * Copyright (C) 2015~2017 Spreadtrum, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/clk.h>
>
> Is this include used? Should be clk-provider?
Right, will remove it.
>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +
>> +#include "ccu_pll.h"
>> +
>> +#define CCU_PLL_1M 1000000
>> +#define CCU_PLL_10M (CCU_PLL_1M * 10)
>> +
>> +#define pindex(pll, member) \
>> + (pll->factors[member].shift / (8 * sizeof(pll->regs[0])))
>> +
>> +#define pshift(pll, member) \
>> + (pll->factors[member].shift % (8 * sizeof(pll->regs[0])))
>> +
>> +#define pwidth(pll, member) \
>> + pll->factors[member].width
>> +
>> +#define pmask(pll, member) \
>> + ((pwidth(pll, member)) ? \
>> + GENMASK(pwidth(pll, member) + pshift(pll, member) - 1, \
>> + pshift(pll, member)) : 0)
>> +
>> +#define pinternal(pll, cfg, member) \
>> + (cfg[pindex(pll, member)] & pmask(pll, member))
>> +
>> +#define pinternal_val(pll, cfg, member) \
>> + (pinternal(pll, cfg, member) >> pshift(pll, member))
>> +
>> +static unsigned long pll_get_refin_rate(struct ccu_pll *pll)
>
> pll could be const?
What this function returns is a factor used to calculate the pll rate
later, I will rename this function in the next iterator.
>
>> +{
>> + u8 shift, index, refin_id = 3;
>> + u32 mask;
>> + const unsigned long refin[4] = { 2, 4, 13, 26 };
>> +
>> + if (pwidth(pll, PLL_REFIN)) {
>> + index = pindex(pll, PLL_REFIN);
>> + shift = pshift(pll, PLL_REFIN);
>> + mask = pmask(pll, PLL_REFIN);
>> + refin_id = (ccu_pll_readl(pll, index) & mask) >> shift;
>> + if (refin_id > 3)
>> + refin_id = 3;
>> + }
>> +
>> + return refin[refin_id];
>> +}
>> +
>> +static u8 pll_get_ibias(unsigned long rate, const u64 *table)
>> +{
>> + u64 i;
>> + u8 num = table[0];
>> +
>> + for (i = 0; i < num; i++)
>> + if (rate <= table[i + 1])
>> + break;
>> +
>> + return i == num ? num - 1 : i;
>> +}
>> +
>> +static unsigned long ccu_pll_helper_recalc_rate(struct ccu_pll *pll,
>> + unsigned long parent_rate)
>> +{
>> + unsigned long rate, refin, k1, k2;
>> + unsigned long kint = 0, nint;
>> + u32 reg_num = pll->regs[0];
>> + u32 *cfg;
>> + u32 i;
>> + u32 mask;
>> +
>> + cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
>> + if (!cfg)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < reg_num; i++)
>> + cfg[i] = ccu_pll_readl(pll, i);
>> +
>> + refin = pll_get_refin_rate(pll);
>> +
>> + if (pinternal(pll, cfg, PLL_PREDIV))
>> + refin = refin * 2;
>> +
>> + if (pwidth(pll, PLL_POSTDIV) &&
>> + ((pll->fflag == 1 && pinternal(pll, cfg, PLL_POSTDIV)) ||
>> + (!pll->fflag && !pinternal(pll, cfg, PLL_POSTDIV))))
>> + refin = refin / 2;
>> +
>> + if (!pinternal(pll, cfg, PLL_DIV_S))
>> + rate = refin * pinternal_val(pll, cfg, PLL_N) * CCU_PLL_10M;
>> + else {
>
> Please include braces on the if as well when another branch has them.
Sure.
>
>> + nint = pinternal_val(pll, cfg, PLL_NINT);
>> + if (pinternal(pll, cfg, PLL_SDM_EN))
>> + kint = pinternal_val(pll, cfg, PLL_KINT);
>> +
>> + mask = pmask(pll, PLL_KINT);
>> +#ifdef CONFIG_64BIT
>> + k1 = 1000;
>> + k2 = 1000;
>> + rate = DIV_ROUND_CLOSEST(refin * kint * k1,
>> + ((mask >> __ffs(mask)) + 1)) *
>> + k2 + refin * nint * CCU_PLL_1M;
>> +#else
>> + k1 = 100;
>> + k2 = 10000;
>> + i = pwidth(pll, PLL_KINT);
>> + i = i < 21 ? 0 : i - 21;
>> + rate = DIV_ROUND_CLOSEST(refin * (kint >> i) * k1,
>> + ((mask >> (__ffs(mask) + i)) + 1)) *
>> + k2 + refin * nint * CCU_PLL_1M;
>> +#endif
>> + }
>> +
>> + return rate;
>> +}
>> +
>> +static int ccu_pll_helper_set_rate(struct ccu_pll *pll,
>> + unsigned long rate,
>> + unsigned long parent_rate)
>> +{
>> + u32 mask, shift, width, ibias_val, index, kint, nint;
>> + u32 reg_num = pll->regs[0], i = 0;
>> + unsigned long refin, fvco = rate;
>> + struct reg_cfg *cfg;
>> +
>> + cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
>> + if (!cfg)
>> + return -ENOMEM;
>> +
>> + refin = pll_get_refin_rate(pll);
>> +
>> + mask = pmask(pll, PLL_PREDIV);
>> + index = pindex(pll, PLL_PREDIV);
>> + width = pwidth(pll, PLL_PREDIV);
>> + if (width && (ccu_pll_readl(pll, index) & mask))
>> + refin = refin * 2;
>> +
>> + mask = pmask(pll, PLL_POSTDIV);
>> + index = pindex(pll, PLL_POSTDIV);
>> + width = pwidth(pll, PLL_POSTDIV);
>> + cfg[index].msk = mask;
>> + if (width && ((pll->fflag == 1 && fvco <= pll->fvco) ||
>> + (pll->fflag == 0 && fvco > pll->fvco)))
>> + cfg[index].val |= mask;
>> +
>> + if (width && fvco <= pll->fvco)
>> + fvco = fvco * 2;
>> +
>> + mask = pmask(pll, PLL_DIV_S);
>> + index = pindex(pll, PLL_DIV_S);
>> + cfg[index].val |= mask;
>> + cfg[index].msk |= mask;
>> +
>> + mask = pmask(pll, PLL_SDM_EN);
>> + index = pindex(pll, PLL_SDM_EN);
>> + cfg[index].val |= mask;
>> + cfg[index].msk |= mask;
>> +
>> + nint = fvco/(refin * CCU_PLL_1M);
>> +
>> + mask = pmask(pll, PLL_NINT);
>> + index = pindex(pll, PLL_NINT);
>> + shift = pshift(pll, PLL_NINT);
>> + cfg[index].val |= (nint << shift) & mask;
>> + cfg[index].msk |= mask;
>> +
>> + mask = pmask(pll, PLL_KINT);
>> + index = pindex(pll, PLL_KINT);
>> + width = pwidth(pll, PLL_KINT);
>> + shift = pshift(pll, PLL_KINT);
>> +#ifndef CONFIG_64BIT
>> + i = width < 21 ? 0 : i - 21;
>> +#endif
>
> What's this? Why do we depend on CONFIG_64BIT?
On 32-bit SoCs, the largest width we can support is limited due to the
limitation of calculation precision.
>
>> + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) *
>> + ((mask >> (shift + i)) + 1), refin * 100) << i;
>> + cfg[index].val |= (kint << shift) & mask;
>> + cfg[index].msk |= mask;
>> +
>> + ibias_val = pll_get_ibias(fvco, pll->itable);
>> +
>> + mask = pmask(pll, PLL_IBIAS);
>> + index = pindex(pll, PLL_IBIAS);
>> + shift = pshift(pll, PLL_IBIAS);
>> + cfg[index].val |= ibias_val << shift & mask;
>> + cfg[index].msk |= mask;
>> +
>> + for (i = 0; i < reg_num; i++) {
>> + if (cfg[i].msk)
>> + ccu_pll_writel(pll, i, cfg[i].val, cfg[i].msk);
>> + }
>> +
>
> Are we waiting for the writel() to go through above? If so we
> need a readl() of the same register to make sure the write has
> completed before delaying.
After writing these configuration registers, we have to wait a certain
time to make sure the pll has worked as we configured. This depends
on other circuit part, so we use udelay rather than reading the same
register.
>
>> + udelay(pll->udelay);
>> +
>> + return 0;
>> +}
>> +
>> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> +
>> + return ccu_pll_helper_recalc_rate(pll, parent_rate);
>> +}
>> +
>> +static int ccu_pll_set_rate(struct clk_hw *hw,
>> + unsigned long rate,
>> + unsigned long parent_rate)
>> +{
>> + struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> +
>> + return ccu_pll_helper_set_rate(pll, rate, parent_rate);
>> +}
>> +
>> +static int ccu_pll_clk_prepare(struct clk_hw *hw)
>> +{
>> + struct ccu_pll *pll = hw_to_ccu_pll(hw);
>> +
>> + udelay(pll->udelay);
>> +
>> + return 0;
>> +}
>> +
>> +static long ccu_pll_round_rate(struct clk_hw *hw, unsigned long rate,
>> + unsigned long *prate)
>> +{
>> + return rate;
>> +}
>> +
>> +const struct clk_ops ccu_pll_ops = {
>> + .prepare = ccu_pll_clk_prepare,
>> + .recalc_rate = ccu_pll_recalc_rate,
>> + .round_rate = ccu_pll_round_rate,
>> + .set_rate = ccu_pll_set_rate,
>> +};
>> diff --git a/drivers/clk/sprd/ccu_pll.h b/drivers/clk/sprd/ccu_pll.h
>> new file mode 100644
>> index 0000000..66fe5d1
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu_pll.h
>> @@ -0,0 +1,123 @@
>> +/*
>> + * Spreadtrum clock pll configurations
>> + *
>> + * Copyright (C) 2015~2017 Spreadtrum, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#ifndef _CCU_PLL_H_
>> +#define _CCU_PLL_H_
>> +
>> +#include "ccu_common.h"
>> +
>> +struct reg_cfg {
>> + u32 val;
>> + u32 msk;
>> +};
>> +
>> +struct ccu_bit_field {
>> + u8 shift;
>> + u8 width;
>> +};
>> +
>> +enum {
>> + PLL_LOCK_DONE = 0,
>
> Drop the = 0 please unless it's needed for something?
OK.
>
>> + PLL_DIV_S,
>> + PLL_MOD_EN,
>> + PLL_SDM_EN,
>> + PLL_REFIN,
>> + PLL_IBIAS,
>> + PLL_N,
>> + PLL_NINT,
>> + PLL_KINT,
>> + PLL_PREDIV,
>> + PLL_POSTDIV,
>> +
>> + PLL_FACT_MAX
>> +};
>> +
>> +/*
>> + * struct ccu_pll - defination of adjustable pll clock
>
> s/defination/definition/
>
>> + *
>> + * @reg: registers used to set the configuration of pll clock,
>> + * reg[0] shows how many registers this pll clock uses.
>> + * @itable: pll ibias table, itable[0] means how many items this
>> + * table includes
>> + * @udelay delay time after setting rate
>> + * @factors used to calculate the pll clock rate
>> + * @fvco: fvco threshold rate
>> + * @fflag: fvco flag
>> + */
>> +struct ccu_pll {
>> + const u32 *regs;
>> + const u64 *itable;
>> + u16 udelay;
>> + const struct ccu_bit_field *factors;
>
> Does this change across the different PLLs? Would be nice to not
Yes, different PLL has diferent bit field arragement, even on the same SoC.
Thanks for your review,
Chunyan
> need the bit field thing.
>
>> + u64 fvco;
>> + u16 fflag;
>> +
>> + struct ccu_common common;
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-06-22 13:20 +0200 |
| Subject | Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support |
| Message-ID | <tV8jw-3Vn-19@gated-at.bofh.it> |
| In reply to | #1672500 |
On Thu, Jun 22, 2017 at 12:17 PM, Chunyan Zhang <zhang.lyra@gmail.com> wrote:
> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 06/18, Chunyan Zhang wrote:
>>> + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) *
>>> + ((mask >> (shift + i)) + 1), refin * 100) << i;
>>> + cfg[index].val |= (kint << shift) & mask;
>>> + cfg[index].msk |= mask;
>>> +
>>> + ibias_val = pll_get_ibias(fvco, pll->itable);
>>> +
>>> + mask = pmask(pll, PLL_IBIAS);
>>> + index = pindex(pll, PLL_IBIAS);
>>> + shift = pshift(pll, PLL_IBIAS);
>>> + cfg[index].val |= ibias_val << shift & mask;
>>> + cfg[index].msk |= mask;
>>> +
>>> + for (i = 0; i < reg_num; i++) {
>>> + if (cfg[i].msk)
>>> + ccu_pll_writel(pll, i, cfg[i].val, cfg[i].msk);
>>> + }
>>> +
>>
>> Are we waiting for the writel() to go through above? If so we
>> need a readl() of the same register to make sure the write has
>> completed before delaying.
>
> After writing these configuration registers, we have to wait a certain
> time to make sure the pll has worked as we configured. This depends
> on other circuit part, so we use udelay rather than reading the same
> register.
I think you have to do both: normally the writel() is not guaranteed
to arrive at the device until you read back from an address in the
same device, so the delay must happen after the readl(), or you won't
know how long to wait for.
Arnd
[toc] | [prev] | [next] | [standalone]
| From | Chunyan Zhang <zhang.lyra@gmail.com> |
|---|---|
| Date | 2017-06-22 14:10 +0200 |
| Subject | Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support |
| Message-ID | <tV95U-4vV-25@gated-at.bofh.it> |
| In reply to | #1672547 |
Hi Arnd,
On 22 June 2017 at 19:15, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Jun 22, 2017 at 12:17 PM, Chunyan Zhang <zhang.lyra@gmail.com> wrote:
>> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>> On 06/18, Chunyan Zhang wrote:
>
>>>> + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) *
>>>> + ((mask >> (shift + i)) + 1), refin * 100) << i;
>>>> + cfg[index].val |= (kint << shift) & mask;
>>>> + cfg[index].msk |= mask;
>>>> +
>>>> + ibias_val = pll_get_ibias(fvco, pll->itable);
>>>> +
>>>> + mask = pmask(pll, PLL_IBIAS);
>>>> + index = pindex(pll, PLL_IBIAS);
>>>> + shift = pshift(pll, PLL_IBIAS);
>>>> + cfg[index].val |= ibias_val << shift & mask;
>>>> + cfg[index].msk |= mask;
>>>> +
>>>> + for (i = 0; i < reg_num; i++) {
>>>> + if (cfg[i].msk)
>>>> + ccu_pll_writel(pll, i, cfg[i].val, cfg[i].msk);
>>>> + }
>>>> +
>>>
>>> Are we waiting for the writel() to go through above? If so we
>>> need a readl() of the same register to make sure the write has
>>> completed before delaying.
>>
>> After writing these configuration registers, we have to wait a certain
>> time to make sure the pll has worked as we configured. This depends
>> on other circuit part, so we use udelay rather than reading the same
>> register.
>
> I think you have to do both: normally the writel() is not guaranteed
> to arrive at the device until you read back from an address in the
> same device, so the delay must happen after the readl(), or you won't
> know how long to wait for.
I got it, will add the readl() in the next iterator.
Thanks,
Chunyan
>
> Arnd
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2017-06-30 03:50 +0200 |
| Subject | Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support |
| Message-ID | <tXTeh-7bI-11@gated-at.bofh.it> |
| In reply to | #1672500 |
On 06/22, Chunyan Zhang wrote:
> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/18, Chunyan Zhang wrote:
> >> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
> >> index 83232e5..c593a93 100644
> >> --- a/drivers/clk/sprd/Makefile
> >> +++ b/drivers/clk/sprd/Makefile
> >> @@ -1,3 +1,3 @@
> >> ifneq ($(CONFIG_OF),)
> >> -obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o
> >> +obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o
> >> endif
> >> diff --git a/drivers/clk/sprd/ccu_pll.c b/drivers/clk/sprd/ccu_pll.c
> >> new file mode 100644
> >> index 0000000..6c908e4
> >> --- /dev/null
> >> +++ b/drivers/clk/sprd/ccu_pll.c
> >> @@ -0,0 +1,241 @@
> >> +/*
> >> + * Spreadtrum pll clock driver
> >> + *
> >> + * Copyright (C) 2015~2017 Spreadtrum, Inc.
> >> + *
> >> + * SPDX-License-Identifier: GPL-2.0
> >> + */
> >> +
> >> +#include <linux/delay.h>
> >> +#include <linux/clk.h>
> >
> > Is this include used? Should be clk-provider?
>
> Right, will remove it.
>
> >
> >> +#include <linux/err.h>
> >> +#include <linux/slab.h>
> >> +
> >> +#include "ccu_pll.h"
> >> +
> >> +#define CCU_PLL_1M 1000000
> >> +#define CCU_PLL_10M (CCU_PLL_1M * 10)
> >> +
> >> +#define pindex(pll, member) \
> >> + (pll->factors[member].shift / (8 * sizeof(pll->regs[0])))
> >> +
> >> +#define pshift(pll, member) \
> >> + (pll->factors[member].shift % (8 * sizeof(pll->regs[0])))
> >> +
> >> +#define pwidth(pll, member) \
> >> + pll->factors[member].width
> >> +
> >> +#define pmask(pll, member) \
> >> + ((pwidth(pll, member)) ? \
> >> + GENMASK(pwidth(pll, member) + pshift(pll, member) - 1, \
> >> + pshift(pll, member)) : 0)
> >> +
> >> +#define pinternal(pll, cfg, member) \
> >> + (cfg[pindex(pll, member)] & pmask(pll, member))
> >> +
> >> +#define pinternal_val(pll, cfg, member) \
> >> + (pinternal(pll, cfg, member) >> pshift(pll, member))
> >> +
> >> +static unsigned long pll_get_refin_rate(struct ccu_pll *pll)
> >
> > pll could be const?
>
> What this function returns is a factor used to calculate the pll rate
> later, I will rename this function in the next iterator.
>
Rename is fine, but pll can still be marked const?
>
> >
> >> + nint = pinternal_val(pll, cfg, PLL_NINT);
> >> + if (pinternal(pll, cfg, PLL_SDM_EN))
> >> + kint = pinternal_val(pll, cfg, PLL_KINT);
> >> +
> >> + mask = pmask(pll, PLL_KINT);
> >> +#ifdef CONFIG_64BIT
> >> + k1 = 1000;
> >> + k2 = 1000;
> >> + rate = DIV_ROUND_CLOSEST(refin * kint * k1,
> >> + ((mask >> __ffs(mask)) + 1)) *
> >> + k2 + refin * nint * CCU_PLL_1M;
> >> +#else
> >> + k1 = 100;
> >> + k2 = 10000;
> >> + i = pwidth(pll, PLL_KINT);
> >> + i = i < 21 ? 0 : i - 21;
> >> + rate = DIV_ROUND_CLOSEST(refin * (kint >> i) * k1,
> >> + ((mask >> (__ffs(mask) + i)) + 1)) *
> >> + k2 + refin * nint * CCU_PLL_1M;
> >> +#endif
> >> + }
> >> +
> >> + return rate;
> >> +}
> >> +
> >> +static int ccu_pll_helper_set_rate(struct ccu_pll *pll,
> >> + unsigned long rate,
> >> + unsigned long parent_rate)
> >> +{
> >> + u32 mask, shift, width, ibias_val, index, kint, nint;
> >> + u32 reg_num = pll->regs[0], i = 0;
> >> + unsigned long refin, fvco = rate;
> >> + struct reg_cfg *cfg;
> >> +
> >> + cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
> >> + if (!cfg)
> >> + return -ENOMEM;
> >> +
> >> + refin = pll_get_refin_rate(pll);
> >> +
> >> + mask = pmask(pll, PLL_PREDIV);
> >> + index = pindex(pll, PLL_PREDIV);
> >> + width = pwidth(pll, PLL_PREDIV);
> >> + if (width && (ccu_pll_readl(pll, index) & mask))
> >> + refin = refin * 2;
> >> +
> >> + mask = pmask(pll, PLL_POSTDIV);
> >> + index = pindex(pll, PLL_POSTDIV);
> >> + width = pwidth(pll, PLL_POSTDIV);
> >> + cfg[index].msk = mask;
> >> + if (width && ((pll->fflag == 1 && fvco <= pll->fvco) ||
> >> + (pll->fflag == 0 && fvco > pll->fvco)))
> >> + cfg[index].val |= mask;
> >> +
> >> + if (width && fvco <= pll->fvco)
> >> + fvco = fvco * 2;
> >> +
> >> + mask = pmask(pll, PLL_DIV_S);
> >> + index = pindex(pll, PLL_DIV_S);
> >> + cfg[index].val |= mask;
> >> + cfg[index].msk |= mask;
> >> +
> >> + mask = pmask(pll, PLL_SDM_EN);
> >> + index = pindex(pll, PLL_SDM_EN);
> >> + cfg[index].val |= mask;
> >> + cfg[index].msk |= mask;
> >> +
> >> + nint = fvco/(refin * CCU_PLL_1M);
> >> +
> >> + mask = pmask(pll, PLL_NINT);
> >> + index = pindex(pll, PLL_NINT);
> >> + shift = pshift(pll, PLL_NINT);
> >> + cfg[index].val |= (nint << shift) & mask;
> >> + cfg[index].msk |= mask;
> >> +
> >> + mask = pmask(pll, PLL_KINT);
> >> + index = pindex(pll, PLL_KINT);
> >> + width = pwidth(pll, PLL_KINT);
> >> + shift = pshift(pll, PLL_KINT);
> >> +#ifndef CONFIG_64BIT
> >> + i = width < 21 ? 0 : i - 21;
> >> +#endif
> >
> > What's this? Why do we depend on CONFIG_64BIT?
>
> On 32-bit SoCs, the largest width we can support is limited due to the
> limitation of calculation precision.
Does the hardware width change? Still not clear to me what's
going on here.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Chunyan Zhang <zhang.lyra@gmail.com> |
|---|---|
| Date | 2017-06-30 10:00 +0200 |
| Subject | Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support |
| Message-ID | <tXZ0m-2EL-25@gated-at.bofh.it> |
| In reply to | #1678385 |
Hi Stephen,
On 30 June 2017 at 09:44, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/22, Chunyan Zhang wrote:
>> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 06/18, Chunyan Zhang wrote:
>> >> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
>> >> index 83232e5..c593a93 100644
>> >> --- a/drivers/clk/sprd/Makefile
>> >> +++ b/drivers/clk/sprd/Makefile
>> >> @@ -1,3 +1,3 @@
>> >> ifneq ($(CONFIG_OF),)
>> >> -obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o
>> >> +obj-y += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o
>> >> endif
>> >> diff --git a/drivers/clk/sprd/ccu_pll.c b/drivers/clk/sprd/ccu_pll.c
>> >> new file mode 100644
>> >> index 0000000..6c908e4
>> >> --- /dev/null
>> >> +++ b/drivers/clk/sprd/ccu_pll.c
>> >> @@ -0,0 +1,241 @@
>> >> +/*
>> >> + * Spreadtrum pll clock driver
>> >> + *
>> >> + * Copyright (C) 2015~2017 Spreadtrum, Inc.
>> >> + *
>> >> + * SPDX-License-Identifier: GPL-2.0
>> >> + */
>> >> +
>> >> +#include <linux/delay.h>
>> >> +#include <linux/clk.h>
>> >
>> > Is this include used? Should be clk-provider?
>>
>> Right, will remove it.
>>
>> >
>> >> +#include <linux/err.h>
>> >> +#include <linux/slab.h>
>> >> +
>> >> +#include "ccu_pll.h"
>> >> +
>> >> +#define CCU_PLL_1M 1000000
>> >> +#define CCU_PLL_10M (CCU_PLL_1M * 10)
>> >> +
>> >> +#define pindex(pll, member) \
>> >> + (pll->factors[member].shift / (8 * sizeof(pll->regs[0])))
>> >> +
>> >> +#define pshift(pll, member) \
>> >> + (pll->factors[member].shift % (8 * sizeof(pll->regs[0])))
>> >> +
>> >> +#define pwidth(pll, member) \
>> >> + pll->factors[member].width
>> >> +
>> >> +#define pmask(pll, member) \
>> >> + ((pwidth(pll, member)) ? \
>> >> + GENMASK(pwidth(pll, member) + pshift(pll, member) - 1, \
>> >> + pshift(pll, member)) : 0)
>> >> +
>> >> +#define pinternal(pll, cfg, member) \
>> >> + (cfg[pindex(pll, member)] & pmask(pll, member))
>> >> +
>> >> +#define pinternal_val(pll, cfg, member) \
>> >> + (pinternal(pll, cfg, member) >> pshift(pll, member))
>> >> +
>> >> +static unsigned long pll_get_refin_rate(struct ccu_pll *pll)
>> >
>> > pll could be const?
>>
>> What this function returns is a factor used to calculate the pll rate
>> later, I will rename this function in the next iterator.
>>
>
> Rename is fine, but pll can still be marked const?
Oh, sorry I misunderstood :)
You mean mark the input parameter "pll" const, right?
>>
>> >
>> >> + nint = pinternal_val(pll, cfg, PLL_NINT);
>> >> + if (pinternal(pll, cfg, PLL_SDM_EN))
>> >> + kint = pinternal_val(pll, cfg, PLL_KINT);
>> >> +
>> >> + mask = pmask(pll, PLL_KINT);
>> >> +#ifdef CONFIG_64BIT
>> >> + k1 = 1000;
>> >> + k2 = 1000;
>> >> + rate = DIV_ROUND_CLOSEST(refin * kint * k1,
>> >> + ((mask >> __ffs(mask)) + 1)) *
>> >> + k2 + refin * nint * CCU_PLL_1M;
>> >> +#else
>> >> + k1 = 100;
>> >> + k2 = 10000;
>> >> + i = pwidth(pll, PLL_KINT);
>> >> + i = i < 21 ? 0 : i - 21;
>> >> + rate = DIV_ROUND_CLOSEST(refin * (kint >> i) * k1,
>> >> + ((mask >> (__ffs(mask) + i)) + 1)) *
>> >> + k2 + refin * nint * CCU_PLL_1M;
>> >> +#endif
>> >> + }
>> >> +
>> >> + return rate;
>> >> +}
>> >> +
>> >> +static int ccu_pll_helper_set_rate(struct ccu_pll *pll,
>> >> + unsigned long rate,
>> >> + unsigned long parent_rate)
>> >> +{
>> >> + u32 mask, shift, width, ibias_val, index, kint, nint;
>> >> + u32 reg_num = pll->regs[0], i = 0;
>> >> + unsigned long refin, fvco = rate;
>> >> + struct reg_cfg *cfg;
>> >> +
>> >> + cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
>> >> + if (!cfg)
>> >> + return -ENOMEM;
>> >> +
>> >> + refin = pll_get_refin_rate(pll);
>> >> +
>> >> + mask = pmask(pll, PLL_PREDIV);
>> >> + index = pindex(pll, PLL_PREDIV);
>> >> + width = pwidth(pll, PLL_PREDIV);
>> >> + if (width && (ccu_pll_readl(pll, index) & mask))
>> >> + refin = refin * 2;
>> >> +
>> >> + mask = pmask(pll, PLL_POSTDIV);
>> >> + index = pindex(pll, PLL_POSTDIV);
>> >> + width = pwidth(pll, PLL_POSTDIV);
>> >> + cfg[index].msk = mask;
>> >> + if (width && ((pll->fflag == 1 && fvco <= pll->fvco) ||
>> >> + (pll->fflag == 0 && fvco > pll->fvco)))
>> >> + cfg[index].val |= mask;
>> >> +
>> >> + if (width && fvco <= pll->fvco)
>> >> + fvco = fvco * 2;
>> >> +
>> >> + mask = pmask(pll, PLL_DIV_S);
>> >> + index = pindex(pll, PLL_DIV_S);
>> >> + cfg[index].val |= mask;
>> >> + cfg[index].msk |= mask;
>> >> +
>> >> + mask = pmask(pll, PLL_SDM_EN);
>> >> + index = pindex(pll, PLL_SDM_EN);
>> >> + cfg[index].val |= mask;
>> >> + cfg[index].msk |= mask;
>> >> +
>> >> + nint = fvco/(refin * CCU_PLL_1M);
>> >> +
>> >> + mask = pmask(pll, PLL_NINT);
>> >> + index = pindex(pll, PLL_NINT);
>> >> + shift = pshift(pll, PLL_NINT);
>> >> + cfg[index].val |= (nint << shift) & mask;
>> >> + cfg[index].msk |= mask;
>> >> +
>> >> + mask = pmask(pll, PLL_KINT);
>> >> + index = pindex(pll, PLL_KINT);
>> >> + width = pwidth(pll, PLL_KINT);
>> >> + shift = pshift(pll, PLL_KINT);
>> >> +#ifndef CONFIG_64BIT
>> >> + i = width < 21 ? 0 : i - 21;
>> >> +#endif
>> >
>> > What's this? Why do we depend on CONFIG_64BIT?
>>
>> On 32-bit SoCs, the largest width we can support is limited due to the
>> limitation of calculation precision.
>
> Does the hardware width change? Still not clear to me what's
> going on here.
I heard from my colleague, that because the calculation precision on
Spreadtrum's 32-bit SoCs is different from on 64-bit SoCs, when the
width of the value of PLL_KINT is larger than 21, the value is too
large to be multiplied on 32-bit Spreadtrum's SoCs.
i = width < 21 ? 0 : i - 21;
Here ' i ' is used to adjust 'shift' rather than 'width' like below (
wrote the code back for convenience of understanding)
+ kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) *
+ ((mask >> (shift + i)) + 1), refin * 100) << i;
Thanks for your review,
Chunyan
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2017-06-30 21:30 +0200 |
| Subject | Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support |
| Message-ID | <tY9M7-13z-35@gated-at.bofh.it> |
| In reply to | #1678576 |
On 06/30, Chunyan Zhang wrote:
> Hi Stephen,
>
> On 30 June 2017 at 09:44, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/22, Chunyan Zhang wrote:
> >> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >> > On 06/18, Chunyan Zhang wrote:
> >> >> + pll->factors[member].width
> >> >> +
> >> >> +#define pmask(pll, member) \
> >> >> + ((pwidth(pll, member)) ? \
> >> >> + GENMASK(pwidth(pll, member) + pshift(pll, member) - 1, \
> >> >> + pshift(pll, member)) : 0)
> >> >> +
> >> >> +#define pinternal(pll, cfg, member) \
> >> >> + (cfg[pindex(pll, member)] & pmask(pll, member))
> >> >> +
> >> >> +#define pinternal_val(pll, cfg, member) \
> >> >> + (pinternal(pll, cfg, member) >> pshift(pll, member))
> >> >> +
> >> >> +static unsigned long pll_get_refin_rate(struct ccu_pll *pll)
> >> >
> >> > pll could be const?
> >>
> >> What this function returns is a factor used to calculate the pll rate
> >> later, I will rename this function in the next iterator.
> >>
> >
> > Rename is fine, but pll can still be marked const?
>
> Oh, sorry I misunderstood :)
> You mean mark the input parameter "pll" const, right?
Yes.
> >> >> +
> >> >> +static int ccu_pll_helper_set_rate(struct ccu_pll *pll,
> >> >> + unsigned long rate,
> >> >> + unsigned long parent_rate)
> >> >> +{
> >> >> + u32 mask, shift, width, ibias_val, index, kint, nint;
> >> >> + u32 reg_num = pll->regs[0], i = 0;
> >> >> + unsigned long refin, fvco = rate;
> >> >> + struct reg_cfg *cfg;
> >> >> +
> >> >> + cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
> >> >> + if (!cfg)
> >> >> + return -ENOMEM;
> >> >> +
> >> >> + refin = pll_get_refin_rate(pll);
> >> >> +
> >> >> + mask = pmask(pll, PLL_PREDIV);
> >> >> + index = pindex(pll, PLL_PREDIV);
> >> >> + width = pwidth(pll, PLL_PREDIV);
> >> >> + if (width && (ccu_pll_readl(pll, index) & mask))
> >> >> + refin = refin * 2;
> >> >> +
> >> >> + mask = pmask(pll, PLL_POSTDIV);
> >> >> + index = pindex(pll, PLL_POSTDIV);
> >> >> + width = pwidth(pll, PLL_POSTDIV);
> >> >> + cfg[index].msk = mask;
> >> >> + if (width && ((pll->fflag == 1 && fvco <= pll->fvco) ||
> >> >> + (pll->fflag == 0 && fvco > pll->fvco)))
> >> >> + cfg[index].val |= mask;
> >> >> +
> >> >> + if (width && fvco <= pll->fvco)
> >> >> + fvco = fvco * 2;
> >> >> +
> >> >> + mask = pmask(pll, PLL_DIV_S);
> >> >> + index = pindex(pll, PLL_DIV_S);
> >> >> + cfg[index].val |= mask;
> >> >> + cfg[index].msk |= mask;
> >> >> +
> >> >> + mask = pmask(pll, PLL_SDM_EN);
> >> >> + index = pindex(pll, PLL_SDM_EN);
> >> >> + cfg[index].val |= mask;
> >> >> + cfg[index].msk |= mask;
> >> >> +
> >> >> + nint = fvco/(refin * CCU_PLL_1M);
> >> >> +
> >> >> + mask = pmask(pll, PLL_NINT);
> >> >> + index = pindex(pll, PLL_NINT);
> >> >> + shift = pshift(pll, PLL_NINT);
> >> >> + cfg[index].val |= (nint << shift) & mask;
> >> >> + cfg[index].msk |= mask;
> >> >> +
> >> >> + mask = pmask(pll, PLL_KINT);
> >> >> + index = pindex(pll, PLL_KINT);
> >> >> + width = pwidth(pll, PLL_KINT);
> >> >> + shift = pshift(pll, PLL_KINT);
> >> >> +#ifndef CONFIG_64BIT
> >> >> + i = width < 21 ? 0 : i - 21;
> >> >> +#endif
> >> >
> >> > What's this? Why do we depend on CONFIG_64BIT?
> >>
> >> On 32-bit SoCs, the largest width we can support is limited due to the
> >> limitation of calculation precision.
> >
> > Does the hardware width change? Still not clear to me what's
> > going on here.
>
> I heard from my colleague, that because the calculation precision on
> Spreadtrum's 32-bit SoCs is different from on 64-bit SoCs, when the
> width of the value of PLL_KINT is larger than 21, the value is too
> large to be multiplied on 32-bit Spreadtrum's SoCs.
It sounds like you're saying that the clk hardware is not
changing, but the sizeof(long) is different on 64-bit and 32-bit
CPUs so you've added this ifndef here for that.
>
> i = width < 21 ? 0 : i - 21;
>
> Here ' i ' is used to adjust 'shift' rather than 'width' like below (
> wrote the code back for convenience of understanding)
>
> + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) *
> + ((mask >> (shift + i)) + 1), refin * 100) << i;
>
Having the types for all these variables would also be helpful.
u32 mask, shift, width, kint, nint;
unsigned long refin, fvco;
Why don't we do 64-bit math here instead of 32-bit math? And use
DIV_ROUND_CLOSEST_ULL?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Chunyan Zhang <zhang.lyra@gmail.com> |
|---|---|
| Date | 2017-07-03 09:50 +0200 |
| Subject | Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support |
| Message-ID | <tZ4hj-6qv-1@gated-at.bofh.it> |
| In reply to | #1679077 |
On 1 July 2017 at 03:22, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/30, Chunyan Zhang wrote:
>> Hi Stephen,
>>
>> On 30 June 2017 at 09:44, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 06/22, Chunyan Zhang wrote:
>> >> On 20 June 2017 at 09:37, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> >> > On 06/18, Chunyan Zhang wrote:
>> >> >> + pll->factors[member].width
>> >> >> +
>> >> >> +#define pmask(pll, member) \
>> >> >> + ((pwidth(pll, member)) ? \
>> >> >> + GENMASK(pwidth(pll, member) + pshift(pll, member) - 1, \
>> >> >> + pshift(pll, member)) : 0)
>> >> >> +
>> >> >> +#define pinternal(pll, cfg, member) \
>> >> >> + (cfg[pindex(pll, member)] & pmask(pll, member))
>> >> >> +
>> >> >> +#define pinternal_val(pll, cfg, member) \
>> >> >> + (pinternal(pll, cfg, member) >> pshift(pll, member))
>> >> >> +
>> >> >> +static unsigned long pll_get_refin_rate(struct ccu_pll *pll)
>> >> >
>> >> > pll could be const?
>> >>
>> >> What this function returns is a factor used to calculate the pll rate
>> >> later, I will rename this function in the next iterator.
>> >>
>> >
>> > Rename is fine, but pll can still be marked const?
>>
>> Oh, sorry I misunderstood :)
>> You mean mark the input parameter "pll" const, right?
>
> Yes.
>
>> >> >> +
>> >> >> +static int ccu_pll_helper_set_rate(struct ccu_pll *pll,
>> >> >> + unsigned long rate,
>> >> >> + unsigned long parent_rate)
>> >> >> +{
>> >> >> + u32 mask, shift, width, ibias_val, index, kint, nint;
>> >> >> + u32 reg_num = pll->regs[0], i = 0;
>> >> >> + unsigned long refin, fvco = rate;
>> >> >> + struct reg_cfg *cfg;
>> >> >> +
>> >> >> + cfg = kcalloc(reg_num, sizeof(*cfg), GFP_KERNEL);
>> >> >> + if (!cfg)
>> >> >> + return -ENOMEM;
>> >> >> +
>> >> >> + refin = pll_get_refin_rate(pll);
>> >> >> +
>> >> >> + mask = pmask(pll, PLL_PREDIV);
>> >> >> + index = pindex(pll, PLL_PREDIV);
>> >> >> + width = pwidth(pll, PLL_PREDIV);
>> >> >> + if (width && (ccu_pll_readl(pll, index) & mask))
>> >> >> + refin = refin * 2;
>> >> >> +
>> >> >> + mask = pmask(pll, PLL_POSTDIV);
>> >> >> + index = pindex(pll, PLL_POSTDIV);
>> >> >> + width = pwidth(pll, PLL_POSTDIV);
>> >> >> + cfg[index].msk = mask;
>> >> >> + if (width && ((pll->fflag == 1 && fvco <= pll->fvco) ||
>> >> >> + (pll->fflag == 0 && fvco > pll->fvco)))
>> >> >> + cfg[index].val |= mask;
>> >> >> +
>> >> >> + if (width && fvco <= pll->fvco)
>> >> >> + fvco = fvco * 2;
>> >> >> +
>> >> >> + mask = pmask(pll, PLL_DIV_S);
>> >> >> + index = pindex(pll, PLL_DIV_S);
>> >> >> + cfg[index].val |= mask;
>> >> >> + cfg[index].msk |= mask;
>> >> >> +
>> >> >> + mask = pmask(pll, PLL_SDM_EN);
>> >> >> + index = pindex(pll, PLL_SDM_EN);
>> >> >> + cfg[index].val |= mask;
>> >> >> + cfg[index].msk |= mask;
>> >> >> +
>> >> >> + nint = fvco/(refin * CCU_PLL_1M);
>> >> >> +
>> >> >> + mask = pmask(pll, PLL_NINT);
>> >> >> + index = pindex(pll, PLL_NINT);
>> >> >> + shift = pshift(pll, PLL_NINT);
>> >> >> + cfg[index].val |= (nint << shift) & mask;
>> >> >> + cfg[index].msk |= mask;
>> >> >> +
>> >> >> + mask = pmask(pll, PLL_KINT);
>> >> >> + index = pindex(pll, PLL_KINT);
>> >> >> + width = pwidth(pll, PLL_KINT);
>> >> >> + shift = pshift(pll, PLL_KINT);
>> >> >> +#ifndef CONFIG_64BIT
>> >> >> + i = width < 21 ? 0 : i - 21;
>> >> >> +#endif
>> >> >
>> >> > What's this? Why do we depend on CONFIG_64BIT?
>> >>
>> >> On 32-bit SoCs, the largest width we can support is limited due to the
>> >> limitation of calculation precision.
>> >
>> > Does the hardware width change? Still not clear to me what's
>> > going on here.
>>
>> I heard from my colleague, that because the calculation precision on
>> Spreadtrum's 32-bit SoCs is different from on 64-bit SoCs, when the
>> width of the value of PLL_KINT is larger than 21, the value is too
>> large to be multiplied on 32-bit Spreadtrum's SoCs.
>
> It sounds like you're saying that the clk hardware is not
> changing, but the sizeof(long) is different on 64-bit and 32-bit
> CPUs so you've added this ifndef here for that.
>
I finally figure out that this #ifndef is indeed not needed, thanks to
your querying :)
They told me that on Spreadtrum's 32-bit SoCs, only 20-bit of PLL_KINT
is used and 3-bit is reserved in order to keep in line with the PLLs
on 64-bit SoC.
And we can get the same effect only if we define PLL with the specific
'width' and 'shift' for PLL_KINT.
I'll remove this statement and 'i' from here and
sprd_pll_helper_recalc_rate() function.
>>
>> i = width < 21 ? 0 : i - 20;
>>
>> Here ' i ' is used to adjust 'shift' rather than 'width' like below (
>> wrote the code back for convenience of understanding)
>>
>> + kint = DIV_ROUND_CLOSEST(((fvco - refin * nint * CCU_PLL_1M)/10000) *
>> + ((mask >> (shift + i)) + 1), refin * 100) << i;
>>
>
> Having the types for all these variables would also be helpful.
>
> u32 mask, shift, width, kint, nint;
> unsigned long refin, fvco;
>
> Why don't we do 64-bit math here instead of 32-bit math? And use
> DIV_ROUND_CLOSEST_ULL?
Agree, we should use 64-bit math, will address this.
Thanks,
Chunyan
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Stephen Boyd <sboyd@codeaurora.org> |
|---|---|
| Date | 2017-06-20 03:30 +0200 |
| Message-ID | <tUg9r-1X0-13@gated-at.bofh.it> |
| In reply to | #1668508 |
On 06/18, Chunyan Zhang wrote: > In the last cycle, the patches support Whale2 sc9860 mobile chip have been > merged. This patchset adds clock driver which is used on almost all > Spreadtrum SoCs. > > This is a rewrite of Spreadtrum's original clock driver[1] according to the > comments[2] from Stephen Boyd. > > This series also adds Spreadtrum clock binding documentation and devicetree > data. > > Any comments would be greatly appreciated. Overall it seems to copy quite a bit of code from sunxi-ng, which is OK, but if that's just copy/paste + replace some names then perhaps we should consolidate the two implementations into one that both SoCs can use. Also, is there any reason why we can't use a platform device driver for this instead of the DT probing mechanism? That is more preferred method of probing clk controllers. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
| From | Chunyan Zhang <zhang.lyra@gmail.com> |
|---|---|
| Date | 2017-06-22 12:10 +0200 |
| Message-ID | <tV7dM-3dT-21@gated-at.bofh.it> |
| In reply to | #1670151 |
Hi Stephen, On 20 June 2017 at 09:25, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 06/18, Chunyan Zhang wrote: >> In the last cycle, the patches support Whale2 sc9860 mobile chip have been >> merged. This patchset adds clock driver which is used on almost all >> Spreadtrum SoCs. >> >> This is a rewrite of Spreadtrum's original clock driver[1] according to the >> comments[2] from Stephen Boyd. >> >> This series also adds Spreadtrum clock binding documentation and devicetree >> data. >> >> Any comments would be greatly appreciated. > > Overall it seems to copy quite a bit of code from sunxi-ng, which > is OK, but if that's just copy/paste + replace some names then > perhaps we should consolidate the two implementations into one > that both SoCs can use. > OK, will try. > Also, is there any reason why we can't use a platform device > driver for this instead of the DT probing mechanism? That is more > preferred method of probing clk controllers. From what I have known on ARM platforms, device drivers cannot recognize out which SoC the driver is running on, assume that the device on different SoC has some differences. To make one only kernel Image can be used on all SoCs of Spreadtrum, we selected the way of loading different dtb for each SoC. Actually, I haven't understood the merits of moving more clk things to driver from DT, could you please introduce more about that? Many appreciated, Chunyan > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web