[PATCH 0/5] clk: meson: Fix GXBB and GXL/GXM GP0 PLL

[PATCH 0/5] clk: meson: Fix GXBB and GXL/GXM GP0 PLL

[Neil Armstrong]

This patchset fixes support for the Amlogic GXBB then GXL/GXM embedded GP0 PLL.

The current support is done via a very generic interface where only the
N/M/OD parameters are changed in the control registers.

But unlike the Fixed PLL, this PLL is not initialized by the bootloader or
firmware, and needs some parameters to initialize and lock correctly.

This patchset also adds the GXL variant compatible string which is already
supported by the GXL and GXM DT nodes.

Neil Armstrong (5):
  clk: meson: Add support for parameters for specific PLLs
  clk: meson-gxbb: Add GP0 PLL init parameters
  clk: meson-gxbb: Add GXL/GXM GP0 Variant
  clk: meson-gxbb: Expose GP0 dt-bindings clock id
  dt-bindings: clock: gxbb-clkc: Add GXL compatible variant

 .../bindings/clock/amlogic,gxbb-clkc.txt           |  3 +-
 drivers/clk/meson/clk-pll.c                        | 52 +++++++++++-
 drivers/clk/meson/clkc.h                           | 23 +++++
 drivers/clk/meson/gxbb.c                           | 97 +++++++++++++++++++++-
 drivers/clk/meson/gxbb.h                           |  4 +-
 include/dt-bindings/clock/gxbb-clkc.h              |  1 +
 6 files changed, 173 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH 1/5] clk: meson: Add support for parameters for specific PLLs

[Neil Armstrong]

In recent Amlogic GXBB, GXL and GXM SoCs, the GP0 PLL needs some specific
parameters in order to initialize and lock correctly.

This patch adds an optional PARAM table used to initialize the PLL to a
default value with it's parameters in order to achieve to desired frequency.

The GP0 PLL in GXBB, GXL/GXM also needs some tweaks in the initialization
steps, and these are exposed along the PARAM table.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/meson/clk-pll.c | 52 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/clk/meson/clkc.h    | 23 ++++++++++++++++++++
 2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 4adc1e8..aff223b 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -116,6 +116,29 @@ static const struct pll_rate_table *meson_clk_get_pll_settings(struct meson_clk_
 	return NULL;
 }
 
+/* Specific wait loop for GXL/GXM GP0 PLL */
+static int meson_clk_pll_wait_lock_reset(struct meson_clk_pll *pll,
+					 struct parm *p_n)
+{
+	int delay = 100;
+	u32 reg;
+
+	while (delay > 0) {
+		reg = readl(pll->base + p_n->reg_off);
+		writel(reg | MESON_PLL_RESET, pll->base + p_n->reg_off);
+		udelay(10);
+		writel(reg & ~MESON_PLL_RESET, pll->base + p_n->reg_off);
+
+		mdelay(1);
+
+		reg = readl(pll->base + p_n->reg_off);
+		if (reg & MESON_PLL_LOCK)
+			return 0;
+		delay--;
+	}
+	return -ETIMEDOUT;
+}
+
 static int meson_clk_pll_wait_lock(struct meson_clk_pll *pll,
 				   struct parm *p_n)
 {
@@ -132,6 +155,15 @@ static int meson_clk_pll_wait_lock(struct meson_clk_pll *pll,
 	return -ETIMEDOUT;
 }
 
+static void meson_clk_pll_init_params(struct meson_clk_pll *pll)
+{
+	int i;
+
+	for (i = 0 ; i < pll->params.params_count ; ++i)
+		writel(pll->params.params_table[i].value,
+		       pll->base + pll->params.params_table[i].reg_off);
+}
+
 static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 				  unsigned long parent_rate)
 {
@@ -151,10 +183,16 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (!rate_set)
 		return -EINVAL;
 
+	/* Initialize the PLL in a clean state if specified */
+	if (pll->params.params_count)
+		meson_clk_pll_init_params(pll);
+
 	/* PLL reset */
 	p = &pll->n;
 	reg = readl(pll->base + p->reg_off);
-	writel(reg | MESON_PLL_RESET, pll->base + p->reg_off);
+	/* If no_init_reset is provided, avoid resetting at this point */
+	if (!pll->params.no_init_reset)
+		writel(reg | MESON_PLL_RESET, pll->base + p->reg_off);
 
 	reg = PARM_SET(p->width, p->shift, reg, rate_set->n);
 	writel(reg, pll->base + p->reg_off);
@@ -184,7 +222,17 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 	}
 
 	p = &pll->n;
-	ret = meson_clk_pll_wait_lock(pll, p);
+	/* If unreset_for_lock is provided, remove the reset bit here */
+	if (pll->params.unreset_for_lock) {
+		reg = readl(pll->base + p->reg_off);
+		writel(reg & ~MESON_PLL_RESET, pll->base + p->reg_off);
+	}
+
+	/* If reset_lock_loop, use a special loop including resetting */
+	if (pll->params.reset_lock_loop)
+		ret = meson_clk_pll_wait_lock_reset(pll, p);
+	else
+		ret = meson_clk_pll_wait_lock(pll, p);
 	if (ret) {
 		pr_warn("%s: pll did not lock, trying to restore old rate %lu\n",
 			__func__, old_rate);
diff --git a/drivers/clk/meson/clkc.h b/drivers/clk/meson/clkc.h
index 9bb70e7..5f1c12d 100644
--- a/drivers/clk/meson/clkc.h
+++ b/drivers/clk/meson/clkc.h
@@ -62,6 +62,28 @@ struct pll_rate_table {
 		.frac		= (_frac),				\
 	}								\
 
+struct pll_params_table {
+	unsigned int reg_off;
+	unsigned int value;
+};
+
+#define PLL_PARAM(_reg, _val)						\
+	{								\
+		.reg_off	= (_reg),				\
+		.value		= (_val),				\
+	}
+
+struct pll_setup_params {
+	struct pll_params_table *params_table;
+	unsigned int params_count;
+	/* Workaround for GP0, do not reset before configuring */
+	bool no_init_reset;
+	/* Workaround for GP0, unreset right before checking for lock */
+	bool unreset_for_lock;
+	/* Workaround for GXL GP0, reset in the lock checking loop */
+	bool reset_lock_loop;
+};
+
 struct meson_clk_pll {
 	struct clk_hw hw;
 	void __iomem *base;
@@ -70,6 +92,7 @@ struct meson_clk_pll {
 	struct parm frac;
 	struct parm od;
 	struct parm od2;
+	const struct pll_setup_params params;
 	const struct pll_rate_table *rate_table;
 	unsigned int rate_count;
 	spinlock_t *lock;
-- 
1.9.1

[PATCH 2/5] clk: meson-gxbb: Add GP0 PLL init parameters

[Neil Armstrong]

Tha Amlogic GXBB SoC GP0 PLL needs some vendor provided parameters to be
initializated in the the GP0 control registers before configuring the rate
with the rate table provided parameters.

GXBB GP0 PLL tweaks are also selected to respect the vendor init procedure.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/meson/gxbb.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
index 1c1ec13..5e1a7dc4 100644
--- a/drivers/clk/meson/gxbb.c
+++ b/drivers/clk/meson/gxbb.c
@@ -352,6 +352,13 @@
 	},
 };
 
+struct pll_params_table gxbb_gp0_params_table[] = {
+	PLL_PARAM(HHI_GP0_PLL_CNTL, 0x6a000228),
+	PLL_PARAM(HHI_GP0_PLL_CNTL2, 0x69c80000),
+	PLL_PARAM(HHI_GP0_PLL_CNTL3, 0x0a5590c4),
+	PLL_PARAM(HHI_GP0_PLL_CNTL4, 0x0000500d),
+};
+
 static struct meson_clk_pll gxbb_gp0_pll = {
 	.m = {
 		.reg_off = HHI_GP0_PLL_CNTL,
@@ -368,6 +375,12 @@
 		.shift   = 16,
 		.width   = 2,
 	},
+	.params = {
+		.params_table = gxbb_gp0_params_table,
+		.params_count =	ARRAY_SIZE(gxbb_gp0_params_table),
+		.no_init_reset = true,
+		.unreset_for_lock = true,
+	},
 	.rate_table = gp0_pll_rate_table,
 	.rate_count = ARRAY_SIZE(gp0_pll_rate_table),
 	.lock = &clk_lock,
-- 
1.9.1

[PATCH 3/5] clk: meson-gxbb: Add GXL/GXM GP0 Variant

[Neil Armstrong]

The clock tree in the Amlogic GXBB and GXL/GXM SoCs is shared, but the GXL/GXM
SoCs embeds a different GP0 PLL, and needs different parameters with a vendor
provided reduced rate table.

This patch adds the GXL GP0 variant, and adds a GXL DT compatible in order
to use the GXL GP0 PLL instead of the GXBB specific one.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/meson/gxbb.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/clk/meson/gxbb.h |  2 ++
 2 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
index 5e1a7dc4..1838f22 100644
--- a/drivers/clk/meson/gxbb.c
+++ b/drivers/clk/meson/gxbb.c
@@ -120,7 +120,7 @@
 	{ /* sentinel */ },
 };
 
-static const struct pll_rate_table gp0_pll_rate_table[] = {
+static const struct pll_rate_table gxbb_gp0_pll_rate_table[] = {
 	PLL_RATE(96000000, 32, 1, 3),
 	PLL_RATE(99000000, 33, 1, 3),
 	PLL_RATE(102000000, 34, 1, 3),
@@ -248,6 +248,35 @@
 	{ /* sentinel */ },
 };
 
+static const struct pll_rate_table gxl_gp0_pll_rate_table[] = {
+	PLL_RATE(504000000, 42, 1, 1),
+	PLL_RATE(516000000, 43, 1, 1),
+	PLL_RATE(528000000, 44, 1, 1),
+	PLL_RATE(540000000, 45, 1, 1),
+	PLL_RATE(552000000, 46, 1, 1),
+	PLL_RATE(564000000, 47, 1, 1),
+	PLL_RATE(576000000, 48, 1, 1),
+	PLL_RATE(588000000, 49, 1, 1),
+	PLL_RATE(600000000, 50, 1, 1),
+	PLL_RATE(612000000, 51, 1, 1),
+	PLL_RATE(624000000, 52, 1, 1),
+	PLL_RATE(636000000, 53, 1, 1),
+	PLL_RATE(648000000, 54, 1, 1),
+	PLL_RATE(660000000, 55, 1, 1),
+	PLL_RATE(672000000, 56, 1, 1),
+	PLL_RATE(684000000, 57, 1, 1),
+	PLL_RATE(696000000, 58, 1, 1),
+	PLL_RATE(708000000, 59, 1, 1),
+	PLL_RATE(720000000, 60, 1, 1),
+	PLL_RATE(732000000, 61, 1, 1),
+	PLL_RATE(744000000, 62, 1, 1),
+	PLL_RATE(756000000, 63, 1, 1),
+	PLL_RATE(768000000, 64, 1, 1),
+	PLL_RATE(780000000, 65, 1, 1),
+	PLL_RATE(792000000, 66, 1, 1),
+	{ /* sentinel */ },
+};
+
 static const struct clk_div_table cpu_div_table[] = {
 	{ .val = 1, .div = 1 },
 	{ .val = 2, .div = 2 },
@@ -381,8 +410,51 @@ struct pll_params_table gxbb_gp0_params_table[] = {
 		.no_init_reset = true,
 		.unreset_for_lock = true,
 	},
-	.rate_table = gp0_pll_rate_table,
-	.rate_count = ARRAY_SIZE(gp0_pll_rate_table),
+	.rate_table = gxbb_gp0_pll_rate_table,
+	.rate_count = ARRAY_SIZE(gxbb_gp0_pll_rate_table),
+	.lock = &clk_lock,
+	.hw.init = &(struct clk_init_data){
+		.name = "gp0_pll",
+		.ops = &meson_clk_pll_ops,
+		.parent_names = (const char *[]){ "xtal" },
+		.num_parents = 1,
+		.flags = CLK_GET_RATE_NOCACHE,
+	},
+};
+
+struct pll_params_table gxl_gp0_params_table[] = {
+	PLL_PARAM(HHI_GP0_PLL_CNTL, 0x40010250),
+	PLL_PARAM(HHI_GP0_PLL_CNTL1, 0xc084a000),
+	PLL_PARAM(HHI_GP0_PLL_CNTL2, 0xb75020be),
+	PLL_PARAM(HHI_GP0_PLL_CNTL3, 0x0a59a288),
+	PLL_PARAM(HHI_GP0_PLL_CNTL4, 0xc000004d),
+	PLL_PARAM(HHI_GP0_PLL_CNTL5, 0x00078000),
+};
+
+static struct meson_clk_pll gxl_gp0_pll = {
+	.m = {
+		.reg_off = HHI_GP0_PLL_CNTL,
+		.shift   = 0,
+		.width   = 9,
+	},
+	.n = {
+		.reg_off = HHI_GP0_PLL_CNTL,
+		.shift   = 9,
+		.width   = 5,
+	},
+	.od = {
+		.reg_off = HHI_GP0_PLL_CNTL,
+		.shift   = 16,
+		.width   = 2,
+	},
+	.params = {
+		.params_table = gxl_gp0_params_table,
+		.params_count =	ARRAY_SIZE(gxl_gp0_params_table),
+		.no_init_reset = true,
+		.reset_lock_loop = true,
+	},
+	.rate_table = gxl_gp0_pll_rate_table,
+	.rate_count = ARRAY_SIZE(gxl_gp0_pll_rate_table),
 	.lock = &clk_lock,
 	.hw.init = &(struct clk_init_data){
 		.name = "gp0_pll",
@@ -821,6 +893,7 @@ struct pll_params_table gxbb_gp0_params_table[] = {
 	&gxbb_hdmi_pll,
 	&gxbb_sys_pll,
 	&gxbb_gp0_pll,
+	&gxl_gp0_pll,
 };
 
 static struct meson_clk_mpll *const gxbb_clk_mplls[] = {
@@ -923,6 +996,10 @@ static int gxbb_clkc_probe(struct platform_device *pdev)
 	struct clk *parent_clk;
 	struct device *dev = &pdev->dev;
 
+	/* Override GP0 clock for GXL/GXM */
+	if (of_device_is_compatible(dev->of_node, "amlogic,gxl-clkc"))
+		gxbb_hw_onecell_data.hws[CLKID_GP0_PLL] = &gxl_gp0_pll.hw;
+
 	/*  Generic clocks and PLLs */
 	clk_base = of_iomap(dev->of_node, 0);
 	if (!clk_base) {
@@ -996,6 +1073,7 @@ static int gxbb_clkc_probe(struct platform_device *pdev)
 
 static const struct of_device_id gxbb_clkc_match_table[] = {
 	{ .compatible = "amlogic,gxbb-clkc" },
+	{ .compatible = "amlogic,gxl-clkc" },
 	{ }
 };
 
diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h
index 8ee2022..7f99bf6 100644
--- a/drivers/clk/meson/gxbb.h
+++ b/drivers/clk/meson/gxbb.h
@@ -71,6 +71,8 @@
 #define HHI_GP0_PLL_CNTL2		0x44 /* 0x11 offset in data sheet */
 #define HHI_GP0_PLL_CNTL3		0x48 /* 0x12 offset in data sheet */
 #define HHI_GP0_PLL_CNTL4		0x4c /* 0x13 offset in data sheet */
+#define	HHI_GP0_PLL_CNTL5		0x50 /* 0x14 offset in data sheet */
+#define	HHI_GP0_PLL_CNTL1		0x58 /* 0x16 offset in data sheet */
 
 #define HHI_XTAL_DIVN_CNTL		0xbc /* 0x2f offset in data sheet */
 #define HHI_TIMER90K			0xec /* 0x3b offset in data sheet */
-- 
1.9.1

[PATCH 4/5] clk: meson-gxbb: Expose GP0 dt-bindings clock id

[Neil Armstrong]

This patch exposes the GP0 PLL clock id in the dt bindings.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/meson/gxbb.h              | 2 +-
 include/dt-bindings/clock/gxbb-clkc.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h
index 7f99bf6..89f1a2c 100644
--- a/drivers/clk/meson/gxbb.h
+++ b/drivers/clk/meson/gxbb.h
@@ -179,7 +179,7 @@
 /* CLKID_FCLK_DIV4 */
 #define CLKID_FCLK_DIV5		  7
 #define CLKID_FCLK_DIV7		  8
-#define CLKID_GP0_PLL		  9
+/* CLKID_GP0_PLL */
 #define CLKID_MPEG_SEL		  10
 #define CLKID_MPEG_DIV		  11
 /* CLKID_CLK81 */
diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt-bindings/clock/gxbb-clkc.h
index 692846c..febef8b 100644
--- a/include/dt-bindings/clock/gxbb-clkc.h
+++ b/include/dt-bindings/clock/gxbb-clkc.h
@@ -10,6 +10,7 @@
 #define CLKID_FCLK_DIV2		4
 #define CLKID_FCLK_DIV3		5
 #define CLKID_FCLK_DIV4		6
+#define CLKID_GP0_PLL		9
 #define CLKID_CLK81		12
 #define CLKID_MPLL2		15
 #define CLKID_SPI		34
-- 
1.9.1

[PATCH 5/5] dt-bindings: clock: gxbb-clkc: Add GXL compatible variant

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
index ce06435..a09d627 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
+++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
@@ -5,7 +5,8 @@ controllers within the SoC.
 
 Required Properties:
 
-- compatible: should be "amlogic,gxbb-clkc"
+- compatible: should be "amlogic,gxbb-clkc" for GXBB SoC,
+	      or "amlogic,gxl-clkc" for GXL and GXM SoC.
 - reg: physical base address of the clock controller and length of memory
        mapped region.
 
-- 
1.9.1

Re: [PATCH 5/5] dt-bindings: clock: gxbb-clkc: Add GXL compatible variant

[Rob Herring]

On Mon, Mar 13, 2017 at 02:26:44PM +0100, Neil Armstrong wrote:
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/5] clk: meson: Add support for parameters for specific PLLs

[Michael Turquette]

Quoting Neil Armstrong (2017-03-13 06:26:40)
> In recent Amlogic GXBB, GXL and GXM SoCs, the GP0 PLL needs some specific
> parameters in order to initialize and lock correctly.
> 
> This patch adds an optional PARAM table used to initialize the PLL to a
> default value with it's parameters in order to achieve to desired frequency.
> 
> The GP0 PLL in GXBB, GXL/GXM also needs some tweaks in the initialization
> steps, and these are exposed along the PARAM table.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/clk/meson/clk-pll.c | 52 +++++++++++++++++++++++++++++++++++++++++++--
>  drivers/clk/meson/clkc.h    | 23 ++++++++++++++++++++
>  2 files changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index 4adc1e8..aff223b 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -116,6 +116,29 @@ static const struct pll_rate_table *meson_clk_get_pll_settings(struct meson_clk_
>         return NULL;
>  }
>  
> +/* Specific wait loop for GXL/GXM GP0 PLL */
> +static int meson_clk_pll_wait_lock_reset(struct meson_clk_pll *pll,
> +                                        struct parm *p_n)
> +{
> +       int delay = 100;
> +       u32 reg;
> +
> +       while (delay > 0) {
> +               reg = readl(pll->base + p_n->reg_off);
> +               writel(reg | MESON_PLL_RESET, pll->base + p_n->reg_off);
> +               udelay(10);
> +               writel(reg & ~MESON_PLL_RESET, pll->base + p_n->reg_off);
> +
> +               mdelay(1);

Can you add a comment explaining where the delay values come from? Can
they vary from chip to chip?

> +
> +               reg = readl(pll->base + p_n->reg_off);
> +               if (reg & MESON_PLL_LOCK)
> +                       return 0;
> +               delay--;
> +       }
> +       return -ETIMEDOUT;
> +}
> +
>  static int meson_clk_pll_wait_lock(struct meson_clk_pll *pll,
>                                    struct parm *p_n)
>  {
> @@ -132,6 +155,15 @@ static int meson_clk_pll_wait_lock(struct meson_clk_pll *pll,
>         return -ETIMEDOUT;
>  }
>  
> +static void meson_clk_pll_init_params(struct meson_clk_pll *pll)
> +{
> +       int i;
> +
> +       for (i = 0 ; i < pll->params.params_count ; ++i)
> +               writel(pll->params.params_table[i].value,
> +                      pll->base + pll->params.params_table[i].reg_off);
> +}
> +
>  static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>                                   unsigned long parent_rate)
>  {
> @@ -151,10 +183,16 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>         if (!rate_set)
>                 return -EINVAL;
>  
> +       /* Initialize the PLL in a clean state if specified */
> +       if (pll->params.params_count)
> +               meson_clk_pll_init_params(pll);
> +
>         /* PLL reset */
>         p = &pll->n;
>         reg = readl(pll->base + p->reg_off);
> -       writel(reg | MESON_PLL_RESET, pll->base + p->reg_off);
> +       /* If no_init_reset is provided, avoid resetting at this point */
> +       if (!pll->params.no_init_reset)
> +               writel(reg | MESON_PLL_RESET, pll->base + p->reg_off);
>  
>         reg = PARM_SET(p->width, p->shift, reg, rate_set->n);
>         writel(reg, pll->base + p->reg_off);
> @@ -184,7 +222,17 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>         }
>  
>         p = &pll->n;
> -       ret = meson_clk_pll_wait_lock(pll, p);
> +       /* If unreset_for_lock is provided, remove the reset bit here */
> +       if (pll->params.unreset_for_lock) {

Small nitpick, but I find "unreset" to be confusing. Since 'reset' here
is a bit that can be set and unset, maybe use clear_reset_for_lock
instead?

Regards,
Mike

> +               reg = readl(pll->base + p->reg_off);
> +               writel(reg & ~MESON_PLL_RESET, pll->base + p->reg_off);
> +       }
> +
> +       /* If reset_lock_loop, use a special loop including resetting */
> +       if (pll->params.reset_lock_loop)
> +               ret = meson_clk_pll_wait_lock_reset(pll, p);
> +       else
> +               ret = meson_clk_pll_wait_lock(pll, p);
>         if (ret) {
>                 pr_warn("%s: pll did not lock, trying to restore old rate %lu\n",
>                         __func__, old_rate);
> diff --git a/drivers/clk/meson/clkc.h b/drivers/clk/meson/clkc.h
> index 9bb70e7..5f1c12d 100644
> --- a/drivers/clk/meson/clkc.h
> +++ b/drivers/clk/meson/clkc.h
> @@ -62,6 +62,28 @@ struct pll_rate_table {
>                 .frac           = (_frac),                              \
>         }                                                               \
>  
> +struct pll_params_table {
> +       unsigned int reg_off;
> +       unsigned int value;
> +};
> +
> +#define PLL_PARAM(_reg, _val)                                          \
> +       {                                                               \
> +               .reg_off        = (_reg),                               \
> +               .value          = (_val),                               \
> +       }
> +
> +struct pll_setup_params {
> +       struct pll_params_table *params_table;
> +       unsigned int params_count;
> +       /* Workaround for GP0, do not reset before configuring */
> +       bool no_init_reset;
> +       /* Workaround for GP0, unreset right before checking for lock */
> +       bool unreset_for_lock;
> +       /* Workaround for GXL GP0, reset in the lock checking loop */
> +       bool reset_lock_loop;
> +};
> +
>  struct meson_clk_pll {
>         struct clk_hw hw;
>         void __iomem *base;
> @@ -70,6 +92,7 @@ struct meson_clk_pll {
>         struct parm frac;
>         struct parm od;
>         struct parm od2;
> +       const struct pll_setup_params params;
>         const struct pll_rate_table *rate_table;
>         unsigned int rate_count;
>         spinlock_t *lock;
> -- 
> 1.9.1
> 

Re: [PATCH 3/5] clk: meson-gxbb: Add GXL/GXM GP0 Variant

[Michael Turquette]

Hi Neil,

Quoting Neil Armstrong (2017-03-13 06:26:42)
> @@ -821,6 +893,7 @@ struct pll_params_table gxbb_gp0_params_table[] = {
>         &gxbb_hdmi_pll,
>         &gxbb_sys_pll,
>         &gxbb_gp0_pll,
> +       &gxl_gp0_pll,

Is there a reason for adding the pointer to this array here? It seems to
me that the gxbb_gp0_pll and gxl_gp0_pll are mutually exclusive, so
perhaps two different tables should be used?

>  };
>  
>  static struct meson_clk_mpll *const gxbb_clk_mplls[] = {
> @@ -923,6 +996,10 @@ static int gxbb_clkc_probe(struct platform_device *pdev)
>         struct clk *parent_clk;
>         struct device *dev = &pdev->dev;
>  
> +       /* Override GP0 clock for GXL/GXM */
> +       if (of_device_is_compatible(dev->of_node, "amlogic,gxl-clkc"))
> +               gxbb_hw_onecell_data.hws[CLKID_GP0_PLL] = &gxl_gp0_pll.hw;

Similarly, this above is a little ugly compared to dedicated tables for
each variant.

Regards,
Mike

> +
>         /*  Generic clocks and PLLs */
>         clk_base = of_iomap(dev->of_node, 0);
>         if (!clk_base) {
> @@ -996,6 +1073,7 @@ static int gxbb_clkc_probe(struct platform_device *pdev)
>  
>  static const struct of_device_id gxbb_clkc_match_table[] = {
>         { .compatible = "amlogic,gxbb-clkc" },
> +       { .compatible = "amlogic,gxl-clkc" },
>         { }
>  };
>  
> diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h
> index 8ee2022..7f99bf6 100644
> --- a/drivers/clk/meson/gxbb.h
> +++ b/drivers/clk/meson/gxbb.h
> @@ -71,6 +71,8 @@
>  #define HHI_GP0_PLL_CNTL2              0x44 /* 0x11 offset in data sheet */
>  #define HHI_GP0_PLL_CNTL3              0x48 /* 0x12 offset in data sheet */
>  #define HHI_GP0_PLL_CNTL4              0x4c /* 0x13 offset in data sheet */
> +#define        HHI_GP0_PLL_CNTL5               0x50 /* 0x14 offset in data sheet */
> +#define        HHI_GP0_PLL_CNTL1               0x58 /* 0x16 offset in data sheet */
>  
>  #define HHI_XTAL_DIVN_CNTL             0xbc /* 0x2f offset in data sheet */
>  #define HHI_TIMER90K                   0xec /* 0x3b offset in data sheet */
> -- 
> 1.9.1
> 

Re: [PATCH 1/5] clk: meson: Add support for parameters for specific PLLs

[Neil Armstrong]

On 03/22/2017 12:43 AM, Michael Turquette wrote:
> Quoting Neil Armstrong (2017-03-13 06:26:40)
>> In recent Amlogic GXBB, GXL and GXM SoCs, the GP0 PLL needs some specific
>> parameters in order to initialize and lock correctly.
>>
>> This patch adds an optional PARAM table used to initialize the PLL to a
>> default value with it's parameters in order to achieve to desired frequency.
>>
>> The GP0 PLL in GXBB, GXL/GXM also needs some tweaks in the initialization
>> steps, and these are exposed along the PARAM table.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/clk/meson/clk-pll.c | 52 +++++++++++++++++++++++++++++++++++++++++++--
>>  drivers/clk/meson/clkc.h    | 23 ++++++++++++++++++++
>>  2 files changed, 73 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index 4adc1e8..aff223b 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -116,6 +116,29 @@ static const struct pll_rate_table *meson_clk_get_pll_settings(struct meson_clk_
>>         return NULL;
>>  }
>>  
>> +/* Specific wait loop for GXL/GXM GP0 PLL */
>> +static int meson_clk_pll_wait_lock_reset(struct meson_clk_pll *pll,
>> +                                        struct parm *p_n)
>> +{
>> +       int delay = 100;
>> +       u32 reg;
>> +
>> +       while (delay > 0) {
>> +               reg = readl(pll->base + p_n->reg_off);
>> +               writel(reg | MESON_PLL_RESET, pll->base + p_n->reg_off);
>> +               udelay(10);
>> +               writel(reg & ~MESON_PLL_RESET, pll->base + p_n->reg_off);
>> +
>> +               mdelay(1);
> 
> Can you add a comment explaining where the delay values come from? Can
> they vary from chip to chip?

Hi,

Ok, the code comes from the vendor tree, and seems to be generic enough to handle
other chips.
But I'm not sure about this particular delay, 1ms seems large enough for a PLL locking...

Neil

> 
>> +
>> +               reg = readl(pll->base + p_n->reg_off);
>> +               if (reg & MESON_PLL_LOCK)
>> +                       return 0;
>> +               delay--;
>> +       }
>> +       return -ETIMEDOUT;
>> +}
>> +
>>  static int meson_clk_pll_wait_lock(struct meson_clk_pll *pll,
>>                                    struct parm *p_n)
>>  {
>> @@ -132,6 +155,15 @@ static int meson_clk_pll_wait_lock(struct meson_clk_pll *pll,
>>         return -ETIMEDOUT;
>>  }
>>  
>> +static void meson_clk_pll_init_params(struct meson_clk_pll *pll)
>> +{
>> +       int i;
>> +
>> +       for (i = 0 ; i < pll->params.params_count ; ++i)
>> +               writel(pll->params.params_table[i].value,
>> +                      pll->base + pll->params.params_table[i].reg_off);
>> +}
>> +
>>  static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>                                   unsigned long parent_rate)
>>  {
>> @@ -151,10 +183,16 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>         if (!rate_set)
>>                 return -EINVAL;
>>  
>> +       /* Initialize the PLL in a clean state if specified */
>> +       if (pll->params.params_count)
>> +               meson_clk_pll_init_params(pll);
>> +
>>         /* PLL reset */
>>         p = &pll->n;
>>         reg = readl(pll->base + p->reg_off);
>> -       writel(reg | MESON_PLL_RESET, pll->base + p->reg_off);
>> +       /* If no_init_reset is provided, avoid resetting at this point */
>> +       if (!pll->params.no_init_reset)
>> +               writel(reg | MESON_PLL_RESET, pll->base + p->reg_off);
>>  
>>         reg = PARM_SET(p->width, p->shift, reg, rate_set->n);
>>         writel(reg, pll->base + p->reg_off);
>> @@ -184,7 +222,17 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>         }
>>  
>>         p = &pll->n;
>> -       ret = meson_clk_pll_wait_lock(pll, p);
>> +       /* If unreset_for_lock is provided, remove the reset bit here */
>> +       if (pll->params.unreset_for_lock) {
> 
> Small nitpick, but I find "unreset" to be confusing. Since 'reset' here
> is a bit that can be set and unset, maybe use clear_reset_for_lock
> instead?
> 
> Regards,
> Mike
> 
>> +               reg = readl(pll->base + p->reg_off);
>> +               writel(reg & ~MESON_PLL_RESET, pll->base + p->reg_off);
>> +       }
>> +
>> +       /* If reset_lock_loop, use a special loop including resetting */
>> +       if (pll->params.reset_lock_loop)
>> +               ret = meson_clk_pll_wait_lock_reset(pll, p);
>> +       else
>> +               ret = meson_clk_pll_wait_lock(pll, p);
>>         if (ret) {
>>                 pr_warn("%s: pll did not lock, trying to restore old rate %lu\n",
>>                         __func__, old_rate);
>> diff --git a/drivers/clk/meson/clkc.h b/drivers/clk/meson/clkc.h
>> index 9bb70e7..5f1c12d 100644
>> --- a/drivers/clk/meson/clkc.h
>> +++ b/drivers/clk/meson/clkc.h
>> @@ -62,6 +62,28 @@ struct pll_rate_table {
>>                 .frac           = (_frac),                              \
>>         }                                                               \
>>  
>> +struct pll_params_table {
>> +       unsigned int reg_off;
>> +       unsigned int value;
>> +};
>> +
>> +#define PLL_PARAM(_reg, _val)                                          \
>> +       {                                                               \
>> +               .reg_off        = (_reg),                               \
>> +               .value          = (_val),                               \
>> +       }
>> +
>> +struct pll_setup_params {
>> +       struct pll_params_table *params_table;
>> +       unsigned int params_count;
>> +       /* Workaround for GP0, do not reset before configuring */
>> +       bool no_init_reset;
>> +       /* Workaround for GP0, unreset right before checking for lock */
>> +       bool unreset_for_lock;
>> +       /* Workaround for GXL GP0, reset in the lock checking loop */
>> +       bool reset_lock_loop;
>> +};
>> +
>>  struct meson_clk_pll {
>>         struct clk_hw hw;
>>         void __iomem *base;
>> @@ -70,6 +92,7 @@ struct meson_clk_pll {
>>         struct parm frac;
>>         struct parm od;
>>         struct parm od2;
>> +       const struct pll_setup_params params;
>>         const struct pll_rate_table *rate_table;
>>         unsigned int rate_count;
>>         spinlock_t *lock;
>> -- 
>> 1.9.1
>>

Re: [PATCH 3/5] clk: meson-gxbb: Add GXL/GXM GP0 Variant

[Neil Armstrong]

On 03/22/2017 12:49 AM, Michael Turquette wrote:
> Hi Neil,
> 
> Quoting Neil Armstrong (2017-03-13 06:26:42)
>> @@ -821,6 +893,7 @@ struct pll_params_table gxbb_gp0_params_table[] = {
>>         &gxbb_hdmi_pll,
>>         &gxbb_sys_pll,
>>         &gxbb_gp0_pll,
>> +       &gxl_gp0_pll,

Yes, because this is the table used to change the register base, this won't harm in any way
to add SoC variant clocks, since they they are initialized using the gxbb_hw_onecell_data table.

> 
> Is there a reason for adding the pointer to this array here? It seems to
> me that the gxbb_gp0_pll and gxl_gp0_pll are mutually exclusive, so
> perhaps two different tables should be used?
> 
>>  };
>>  
>>  static struct meson_clk_mpll *const gxbb_clk_mplls[] = {
>> @@ -923,6 +996,10 @@ static int gxbb_clkc_probe(struct platform_device *pdev)
>>         struct clk *parent_clk;
>>         struct device *dev = &pdev->dev;
>>  
>> +       /* Override GP0 clock for GXL/GXM */
>> +       if (of_device_is_compatible(dev->of_node, "amlogic,gxl-clkc"))
>> +               gxbb_hw_onecell_data.hws[CLKID_GP0_PLL] = &gxl_gp0_pll.hw;
> 
> Similarly, this above is a little ugly compared to dedicated tables for
> each variant.

Here is the true uglyness, but would you like to have the exact same gxbb_hw_onecell_data
duplicated for only two different clocks ?
The gxbb_hw_onecell_data table is 105 lines, and when adding new clocks, we will need to
make sure they are still synchronized.

If you have a better idea... I can still push a v2 with such table with also a
separate gxbb_clk_plls table stored in a struct given from of_device_get_match_data()

Neil

> 
> Regards,
> Mike
> 
>> +
>>         /*  Generic clocks and PLLs */
>>         clk_base = of_iomap(dev->of_node, 0);
>>         if (!clk_base) {
>> @@ -996,6 +1073,7 @@ static int gxbb_clkc_probe(struct platform_device *pdev)
>>  
>>  static const struct of_device_id gxbb_clkc_match_table[] = {
>>         { .compatible = "amlogic,gxbb-clkc" },
>> +       { .compatible = "amlogic,gxl-clkc" },
>>         { }
>>  };
>>  
>> diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h
>> index 8ee2022..7f99bf6 100644
>> --- a/drivers/clk/meson/gxbb.h
>> +++ b/drivers/clk/meson/gxbb.h
>> @@ -71,6 +71,8 @@
>>  #define HHI_GP0_PLL_CNTL2              0x44 /* 0x11 offset in data sheet */
>>  #define HHI_GP0_PLL_CNTL3              0x48 /* 0x12 offset in data sheet */
>>  #define HHI_GP0_PLL_CNTL4              0x4c /* 0x13 offset in data sheet */
>> +#define        HHI_GP0_PLL_CNTL5               0x50 /* 0x14 offset in data sheet */
>> +#define        HHI_GP0_PLL_CNTL1               0x58 /* 0x16 offset in data sheet */
>>  
>>  #define HHI_XTAL_DIVN_CNTL             0xbc /* 0x2f offset in data sheet */
>>  #define HHI_TIMER90K                   0xec /* 0x3b offset in data sheet */
>> -- 
>> 1.9.1
>>

Re: [PATCH 1/5] clk: meson: Add support for parameters for specific PLLs

[Neil Armstrong]

On 03/22/2017 12:43 AM, Michael Turquette wrote:
> Quoting Neil Armstrong (2017-03-13 06:26:40)
[..]
>>  
>>         p = &pll->n;
>> -       ret = meson_clk_pll_wait_lock(pll, p);
>> +       /* If unreset_for_lock is provided, remove the reset bit here */
>> +       if (pll->params.unreset_for_lock) {
> 
> Small nitpick, but I find "unreset" to be confusing. Since 'reset' here
> is a bit that can be set and unset, maybe use clear_reset_for_lock
> instead?
> 
> Regards,
> Mike
> 

You are right, I'll rename it.

Neil

Re: [PATCH 3/5] clk: meson-gxbb: Add GXL/GXM GP0 Variant

[Michael Turquette]

Quoting Neil Armstrong (2017-03-22 02:22:57)
> On 03/22/2017 12:49 AM, Michael Turquette wrote:
> > Hi Neil,
> > 
> > Quoting Neil Armstrong (2017-03-13 06:26:42)
> >> @@ -821,6 +893,7 @@ struct pll_params_table gxbb_gp0_params_table[] = {
> >>         &gxbb_hdmi_pll,
> >>         &gxbb_sys_pll,
> >>         &gxbb_gp0_pll,
> >> +       &gxl_gp0_pll,
> 
> Yes, because this is the table used to change the register base, this won't harm in any way
> to add SoC variant clocks, since they they are initialized using the gxbb_hw_onecell_data table.
> 
> > 
> > Is there a reason for adding the pointer to this array here? It seems to
> > me that the gxbb_gp0_pll and gxl_gp0_pll are mutually exclusive, so
> > perhaps two different tables should be used?
> > 
> >>  };
> >>  
> >>  static struct meson_clk_mpll *const gxbb_clk_mplls[] = {
> >> @@ -923,6 +996,10 @@ static int gxbb_clkc_probe(struct platform_device *pdev)
> >>         struct clk *parent_clk;
> >>         struct device *dev = &pdev->dev;
> >>  
> >> +       /* Override GP0 clock for GXL/GXM */
> >> +       if (of_device_is_compatible(dev->of_node, "amlogic,gxl-clkc"))
> >> +               gxbb_hw_onecell_data.hws[CLKID_GP0_PLL] = &gxl_gp0_pll.hw;
> > 
> > Similarly, this above is a little ugly compared to dedicated tables for
> > each variant.
> 
> Here is the true uglyness, but would you like to have the exact same gxbb_hw_onecell_data
> duplicated for only two different clocks ?
> The gxbb_hw_onecell_data table is 105 lines, and when adding new clocks, we will need to
> make sure they are still synchronized.
> 
> If you have a better idea... I can still push a v2 with such table with also a
> separate gxbb_clk_plls table stored in a struct given from of_device_get_match_data()

I was not thinking of duplicating all of the clock data table, but
breaking out the parts with variation into separate tables. E.g. a
common table, a gxbb table and a gp0 table.

But on second look your original solution is fine, especially since
those two new tables I mentioned would only have a single element in
them, which is silly.

Ack.

Regards,
Mike

> 
> Neil
> 
> > 
> > Regards,
> > Mike
> > 
> >> +
> >>         /*  Generic clocks and PLLs */
> >>         clk_base = of_iomap(dev->of_node, 0);
> >>         if (!clk_base) {
> >> @@ -996,6 +1073,7 @@ static int gxbb_clkc_probe(struct platform_device *pdev)
> >>  
> >>  static const struct of_device_id gxbb_clkc_match_table[] = {
> >>         { .compatible = "amlogic,gxbb-clkc" },
> >> +       { .compatible = "amlogic,gxl-clkc" },
> >>         { }
> >>  };
> >>  
> >> diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h
> >> index 8ee2022..7f99bf6 100644
> >> --- a/drivers/clk/meson/gxbb.h
> >> +++ b/drivers/clk/meson/gxbb.h
> >> @@ -71,6 +71,8 @@
> >>  #define HHI_GP0_PLL_CNTL2              0x44 /* 0x11 offset in data sheet */
> >>  #define HHI_GP0_PLL_CNTL3              0x48 /* 0x12 offset in data sheet */
> >>  #define HHI_GP0_PLL_CNTL4              0x4c /* 0x13 offset in data sheet */
> >> +#define        HHI_GP0_PLL_CNTL5               0x50 /* 0x14 offset in data sheet */
> >> +#define        HHI_GP0_PLL_CNTL1               0x58 /* 0x16 offset in data sheet */
> >>  
> >>  #define HHI_XTAL_DIVN_CNTL             0xbc /* 0x2f offset in data sheet */
> >>  #define HHI_TIMER90K                   0xec /* 0x3b offset in data sheet */
> >> -- 
> >> 1.9.1
> >>
>