Re: [PATCH] ARC: clk: introduce HSDKv1 pll driver
On 08/09, Eugeniy Paltsev wrote:
> On Thu, 2017-08-03 at 18:53 -0700, Stephen Boyd wrote:
> > On 07/14, Eugeniy Paltsev wrote:
> > > + /* input divider = reg.idiv + 1 */
> > > + idiv = 1 + ((val & CGU_PLL_CTRL_IDIV_MASK) >>
> > > CGU_PLL_CTRL_IDIV_SHIFT);
> > > + /* fb divider = 2*(reg.fbdiv + 1) */
> > > + fbdiv = 2 * (1 + ((val & CGU_PLL_CTRL_FBDIV_MASK) >>
> > > CGU_PLL_CTRL_FBDIV_SHIFT));
> > > + /* output divider = 2^(reg.odiv) */
> > > + odiv = 1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >>
> > > CGU_PLL_CTRL_ODIV_SHIFT);
> >
> > Maybe just drop these comments. They're just repeating the code.
>
> Actually I would prefer to keep them, as "2^(reg.odiv)" is more
> human-readable then
> "1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >> CGU_PLL_CTRL_ODIV_SHIFT)"
Alright.
>
> > > +
> > > + rate = (u64)parent_rate * fbdiv;
> > > + do_div(rate, idiv * odiv);
> > > +
> > > + return rate;
> > > +}
> > > +
> > > +static long hsdk_pll_round_rate(struct clk_hw *hw, unsigned long
> > > rate,
> > > + unsigned long *prate)
> > > +{
> > > + int i;
> > > + long best_rate;
> > > + struct hsdk_pll_clk *clk = to_hsdk_pll_clk(hw);
> > > + const struct hsdk_pll_cfg *pll_cfg = clk->pll_cfg;
> > > +
> > > + if (pll_cfg[0].rate == 0)
> > > + return -EINVAL;
> >
> > This happens?
>
> Only if we add bad hsdk_pll_cfg table. But it is quite cold code - we
> change pll configuration quite rare, so maybe it's better to keep this
> assert?
Can it be a BUILD_BUG_ON() somehow?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] ARC: clk: introduce HSDKv1 pll driver
Hi Stephen,
thanks for respond, my comments are inlined below.
On Thu, 2017-08-03 at 18:53 -0700, Stephen Boyd wrote:
> On 07/14, Eugeniy Paltsev wrote:
> [...]
> > + dev_dbg(clk->dev, "write configurarion: 0x%x", val);
>
> Or just use %#x to add the 0x part.
Thanks, I don't know about this option.
>
> [...]
>
> > + /* input divider = reg.idiv + 1 */
> > + idiv = 1 + ((val & CGU_PLL_CTRL_IDIV_MASK) >>
> > CGU_PLL_CTRL_IDIV_SHIFT);
> > + /* fb divider = 2*(reg.fbdiv + 1) */
> > + fbdiv = 2 * (1 + ((val & CGU_PLL_CTRL_FBDIV_MASK) >>
> > CGU_PLL_CTRL_FBDIV_SHIFT));
> > + /* output divider = 2^(reg.odiv) */
> > + odiv = 1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >>
> > CGU_PLL_CTRL_ODIV_SHIFT);
>
> Maybe just drop these comments. They're just repeating the code.
Actually I would prefer to keep them, as "2^(reg.odiv)" is more
human-readable then
"1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >> CGU_PLL_CTRL_ODIV_SHIFT)"
> > +
> > + rate = (u64)parent_rate * fbdiv;
> > + do_div(rate, idiv * odiv);
> > +
> > + return rate;
> > +}
> > +
> > +static long hsdk_pll_round_rate(struct clk_hw *hw, unsigned long
> > rate,
> > + unsigned long *prate)
> > +{
> > + int i;
> > + long best_rate;
> > + struct hsdk_pll_clk *clk = to_hsdk_pll_clk(hw);
> > + const struct hsdk_pll_cfg *pll_cfg = clk->pll_cfg;
> > +
> > + if (pll_cfg[0].rate == 0)
> > + return -EINVAL;
>
> This happens?
Only if we add bad hsdk_pll_cfg table. But it is quite cold code - we
change pll configuration quite rare, so maybe it's better to keep this
assert?
> > +
> > + best_rate = pll_cfg[0].rate;
> > +
> > + for (i = 1; pll_cfg[i].rate != 0; i++) {
> > + if (abs(rate - pll_cfg[i].rate) < abs(rate -
> > best_rate))
>
> Alright, rate is unsigned long, and best_rate is long. abs() does
> the right thing here, but it makes me have to think twice if
> best_rate can be negative and then this is a larger number, not a
> smaller one. Can we make best_rate unsigned long in this
> function?
Yes, we can.
Anyway it's a bit strange what rate is unsigned long and round_rate
return value is long.
--
Eugeniy Paltsev
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] ARC: clk: introduce HSDKv1 pll driver
On 07/14, Eugeniy Paltsev wrote:
> HSDKv1 boards manages it's clocks using various PLLs. These PLL has same
s/it's/its/
s/has/have/
> dividers and corresponding control registers mapped to different addresses.
> So we add one common driver for such PLLs.
>
> Each PLL on HSDK board consist of three dividers: IDIV, FBDIV and
> ODIV. Output clock value is managed using these dividers.
>
> We add pre-defined tables with supported rate values and appropriate
> configurations of IDIV, FBDIV and ODIV for each value.
>
> As of today we add support for PLLs that generate clock for the
> HSDKv1 arc cpus, system, ddr, AXI tunnel and hdmi.
>
> By this patch we add support for several plls (arc cpus pll and others),
> so we had to use two different init types: CLK_OF_DECLARE for arc cpus pll
> and regular probing for others plls.
Ok. This should be put into comment form above the
CLK_OF_DECLARE() macro in the driver. Like "CPU PLL needed early
for XYZ thing that is used early".
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 68ca2d9..198fc14 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -219,6 +219,13 @@ config COMMON_CLK_VC5
> This driver supports the IDT VersaClock5 programmable clock
> generator.
>
> +config CLK_HSDK_V1
> + bool "PLL Driver for HSDKv1 platform"
> + depends on OF || COMPILE_TEST
> + ---help---
> + This driver supports the HSDKv1 core, system, ddr, tunnel and hdmi
> PLLs
> + control.
> +
Are we keeping this file sorted? Not really, but please put this
somewhere semi-sorted instead of at the end of the file. I'll go
and sort this Kconfig file later.
> diff --git a/drivers/clk/clk-hsdk-pll.c b/drivers/clk/clk-hsdk-pll.c
> new file mode 100644
> index 000..4e58e55
> --- /dev/null
> +++ b/drivers/clk/clk-hsdk-pll.c
> @@ -0,0 +1,346 @@
> +/*
> + * Synopsys HSDKv1 SDP Generic PLL clock driver
> + *
> + * Copyright (C) 2017 Synopsys
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include
> +#include
No need for this include.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define CGU_PLL_CTRL 0x000 /* ARC PLL control register */
> +#define CGU_PLL_STATUS 0x004 /* ARC PLL status register */
> +#define CGU_PLL_FMEAS0x008 /* ARC PLL frequency measurement register
> */
> +#define CGU_PLL_MON 0x00C /* ARC PLL monitor register */
> +
> +#define CGU_PLL_CTRL_ODIV_SHIFT 2
> +#define CGU_PLL_CTRL_IDIV_SHIFT 4
> +#define CGU_PLL_CTRL_FBDIV_SHIFT 9
> +#define CGU_PLL_CTRL_BAND_SHIFT 20
> +
> +#define CGU_PLL_CTRL_ODIV_MASK GENMASK(3,
> CGU_PLL_CTRL_ODIV_SHIFT)
> +#define CGU_PLL_CTRL_IDIV_MASK GENMASK(8,
> CGU_PLL_CTRL_IDIV_SHIFT)
> +#define CGU_PLL_CTRL_FBDIV_MASK GENMASK(15,
> CGU_PLL_CTRL_FBDIV_SHIFT)
> +
> +#define CGU_PLL_CTRL_PD BIT(0)
> +#define CGU_PLL_CTRL_BYPASS BIT(1)
> +
> +#define CGU_PLL_STATUS_LOCK BIT(0)
> +#define CGU_PLL_STATUS_ERR BIT(1)
> +
> +#define HSDK_PLL_MAX_LOCK_TIME 100 /* 100 us */
> +
> +struct hsdk_pll_cfg {
> + u32 rate;
> + u32 idiv;
> + u32 fbdiv;
> + u32 odiv;
> + u32 band;
> +};
> +
> +static const struct hsdk_pll_cfg asdt_pll_cfg[] = {
> + { 1, 0, 11, 3, 0 },
> + { 13300, 0, 15, 3, 0 },
> + { 2, 1, 47, 3, 0 },
> + { 23300, 1, 27, 2, 0 },
> + { 3, 1, 35, 2, 0 },
> + { 33300, 1, 39, 2, 0 },
> + { 4, 1, 47, 2, 0 },
> + { 5, 0, 14, 1, 0 },
> + { 6, 0, 17, 1, 0 },
> + { 7, 0, 20, 1, 0 },
> + { 8, 0, 23, 1, 0 },
> + { 9, 1, 26, 0, 0 },
> + { 10, 1, 29, 0, 0 },
> + { 11, 1, 32, 0, 0 },
> + { 12, 1, 35, 0, 0 },
> + { 13, 1, 38, 0, 0 },
> + { 14, 1, 41, 0, 0 },
> + { 15, 1, 44, 0, 0 },
> + { 16, 1, 47, 0, 0 },
> + {}
> +};
> +
> +static const struct hsdk_pll_cfg hdmi_pll_cfg[] = {
> + { 29700, 0, 21, 2, 0 },
> + { 54000, 0, 19, 1, 0 },
> + { 59400, 0, 21, 1, 0 },
> + {}
> +};
> +
> +struct hsdk_pll_clk {
> + struct clk_hw hw;
> + void __iomem *regs;
> + const struct hsdk_pll_cfg *pll_cfg;
> + struct device *dev;
> +};
> +
> +static inline void hsdk_pll_write(struct hsdk_pll_clk *clk, u32 reg, u32 val)
> +{
> + iowrite32(val, clk->regs + reg);
> +}
> +
> +static inline u32 hsdk_pll_read(struct hsdk_pll_clk *clk, u32 reg)
> +{
> + return ioread32(clk->regs + reg);
> +}
> +
> +static inline void hsdk_pll_set_cfg(struct hsdk_pll_clk *clk,
> + const struct hsdk_pll_cfg *c
Re: [PATCH] ARC: clk: introduce HSDKv1 pll driver
On 07/27, Vineet Gupta wrote: > Hi Stephen, > > On 07/14/2017 09:01 PM, Eugeniy Paltsev wrote: > >HSDKv1 boards manages it's clocks using various PLLs. These PLL has same > >dividers and corresponding control registers mapped to different addresses. > >So we add one common driver for such PLLs. > > > >Each PLL on HSDK board consist of three dividers: IDIV, FBDIV and > >ODIV. Output clock value is managed using these dividers. > > > >We add pre-defined tables with supported rate values and appropriate > >configurations of IDIV, FBDIV and ODIV for each value. > > > >As of today we add support for PLLs that generate clock for the > >HSDKv1 arc cpus, system, ddr, AXI tunnel and hdmi. > > > >By this patch we add support for several plls (arc cpus pll and others), > >so we had to use two different init types: CLK_OF_DECLARE for arc cpus pll > >and regular probing for others plls. > > > >Signed-off-by: Eugeniy Paltsev > > Gentle ping, any chance you could look at this sometime. > > Thx, Yes it's in the queue. Probably get to it tomorrow. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] ARC: clk: introduce HSDKv1 pll driver
Hi Stephen,
On 07/14/2017 09:01 PM, Eugeniy Paltsev wrote:
HSDKv1 boards manages it's clocks using various PLLs. These PLL has same
dividers and corresponding control registers mapped to different addresses.
So we add one common driver for such PLLs.
Each PLL on HSDK board consist of three dividers: IDIV, FBDIV and
ODIV. Output clock value is managed using these dividers.
We add pre-defined tables with supported rate values and appropriate
configurations of IDIV, FBDIV and ODIV for each value.
As of today we add support for PLLs that generate clock for the
HSDKv1 arc cpus, system, ddr, AXI tunnel and hdmi.
By this patch we add support for several plls (arc cpus pll and others),
so we had to use two different init types: CLK_OF_DECLARE for arc cpus pll
and regular probing for others plls.
Signed-off-by: Eugeniy Paltsev
Gentle ping, any chance you could look at this sometime.
Thx,
-Vineet
---
.../bindings/clock/snps,hsdkv1-pll-clock.txt | 28 ++
MAINTAINERS| 6 +
drivers/clk/Kconfig| 7 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-hsdk-pll.c | 346 +
5 files changed, 388 insertions(+)
create mode 100644
Documentation/devicetree/bindings/clock/snps,hsdkv1-pll-clock.txt
create mode 100644 drivers/clk/clk-hsdk-pll.c
diff --git a/Documentation/devicetree/bindings/clock/snps,hsdkv1-pll-clock.txt
b/Documentation/devicetree/bindings/clock/snps,hsdkv1-pll-clock.txt
new file mode 100644
index 000..3580aa0
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/snps,hsdkv1-pll-clock.txt
@@ -0,0 +1,28 @@
+Binding for the HSDKv1 Generic PLL clock
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible: should be "snps,hsdk--pll-clock"
+ "snps,hsdk-core-pll-clock"
+ "snps,hsdk-gp-pll-clock"
+ "snps,hsdk-hdmi-pll-clock"
+- reg : should contain base register location and length.
+- clocks: shall be the input parent clock phandle for the PLL.
+- #clock-cells: from common clock binding; Should always be set to 0.
+
+Example:
+ input_clk: input-clk {
+ clock-frequency = <>;
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ };
+
+ cpu_clk: cpu-clk@0 {
+ compatible = "snps,hsdk-core-pll-clock";
+ reg = <0x00 0x10>;
+ #clock-cells = <0>;
+ clocks = <&input_clk>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index b5ab794..96d0021 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12768,6 +12768,12 @@ F: arch/arc/plat-axs10x
F:arch/arc/boot/dts/ax*
F:Documentation/devicetree/bindings/arc/axs10*
+SYNOPSYS ARC HSDKv1 SDP pll clock driver
+M: Eugeniy Paltsev
+S: Supported
+F: drivers/clk/clk-hsdk-pll.c
+F: Documentation/devicetree/bindings/clock/snps,hsdkv1-pll-clock.txt
+
SYSTEM CONFIGURATION (SYSCON)
M:Lee Jones
M:Arnd Bergmann
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 68ca2d9..198fc14 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -219,6 +219,13 @@ config COMMON_CLK_VC5
This driver supports the IDT VersaClock5 programmable clock
generator.
+config CLK_HSDK_V1
+ bool "PLL Driver for HSDKv1 platform"
+ depends on OF || COMPILE_TEST
+ ---help---
+ This driver supports the HSDKv1 core, system, ddr, tunnel and hdmi
PLLs
+ control.
+
source "drivers/clk/bcm/Kconfig"
source "drivers/clk/hisilicon/Kconfig"
source "drivers/clk/imgtec/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index cd376b3..91e6cbd 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_COMMON_CLK_CS2000_CP)+= clk-cs2000-cp.o
obj-$(CONFIG_ARCH_EFM32) += clk-efm32gg.o
obj-$(CONFIG_COMMON_CLK_GEMINI) += clk-gemini.o
obj-$(CONFIG_ARCH_HIGHBANK) += clk-highbank.o
+obj-$(CONFIG_CLK_HSDK_V1) += clk-hsdk-pll.o
obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
obj-$(CONFIG_ARCH_MB86S7X)+= clk-mb86s7x.o
obj-$(CONFIG_ARCH_MOXART) += clk-moxart.o
diff --git a/drivers/clk/clk-hsdk-pll.c b/drivers/clk/clk-hsdk-pll.c
new file mode 100644
index 000..4e58e55
--- /dev/null
+++ b/drivers/clk/clk-hsdk-pll.c
@@ -0,0 +1,346 @@
+/*
+ * Synopsys HSDKv1 SDP Generic PLL clock driver
+ *
+ * Copyright (C) 2017 Synopsys
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define CGU_PLL_CTRL 0x000 /* ARC PLL control r
