[PATCH v7 0/5] add Amlogic A1 clock controller driver

[PATCH v7 0/5] add Amlogic A1 clock controller driver

[Jian Hu]

add support for Amlogic A1 clock driver, the clock includes 
three parts: peripheral clocks, pll clocks, CPU clocks.
sys pll and CPU clocks will be sent in next patch.

Changes since v6 at [7]:
-fix 'dt_binding_check' compiling error
-add acked-by

Changes since v5 at [6]:
-fix yaml file
-add rst/current_en/l_detect parm detection
-remove 'meson_eeclkc_data' in a1.c and a1-pll.c

Changes since v4 at [5]:
- change yaml GPL
- drop meson-eeclk.c patch, add probe function in each driver
- add CLK_IS_CRITICAL for sys_clk clock, drop the flag for sys_a and sys_b
- add new parm for pll, add protection for rst parm
- drop flag for a1_fixed_pll
- remove the same comment for fclk_div, add "refer to"
- add critical flag for a1_sys_clk
- remove rtc table
- rename a1_dspa_en_dspa and a1_dspb_en_dspb
- remove useless comment

Changes since v3 at [3]:
-fix reparenting orphan failed, it depends on jerome's patch [4]
-fix changelist in v3 about reparenting orphan
-remove the dts patch 

Changes since v2 at [2]:
-add probe function for A1
-seperate the clock driver into two patch
-change some clock flags and ops
-add support for a1 PLL ops
-add A1 clock node
-fix reparenting orphan clock failed, registering xtal_fixpll
 and xtal_hifipll after the provider registration, it is not
 a best way.

Changes since v1 at [1]:
-place A1 config alphabetically
-add actual reason for RO ops, CLK_IS_CRITICAL, CLK_IGNORE_UNUSED
-separate the driver into two driver: peripheral and pll driver
-delete CLK_IGNORE_UNUSED flag for pwm b/c/d/e/f clock, dsp clock
-delete the change in Kconfig.platforms, address to Kevin alone
-remove the useless comments
-modify the meson pll driver to support A1 PLLs

[1] https://lkml.kernel.org/r/1569411888-98116-1-git-send-email-jian.hu@amlogic.com
[2] https://lkml.kernel.org/r/1571382865-41978-1-git-send-email-jian.hu@amlogic.com
[3] https://lkml.kernel.org/r/20191129144605.182774-1-jian.hu@amlogic.com
[4] https://lkml.kernel.org/r/20191203080805.104628-1-jbrunet@baylibre.com
[5] https://lkml.kernel.org/r/20191206074052.15557-1-jian.hu@amlogic.com
[6] https://lkml.kernel.org/r/20191227094606.143637-1-jian.hu@amlogic.com
[7] https://lkml.kernel.org/r/20200116080440.118679-1-jian.hu@amlogic.com

Jian Hu (5):
  dt-bindings: clock: meson: add A1 PLL clock controller bindings
  clk: meson: add support for A1 PLL clock ops
  clk: meson: a1: add support for Amlogic A1 PLL clock driver
  dt-bindings: clock: meson: add A1 peripheral clock controller bindings
  clk: meson: a1: add support for Amlogic A1 Peripheral clock driver

 .../bindings/clock/amlogic,a1-clkc.yaml       |   65 +
 .../bindings/clock/amlogic,a1-pll-clkc.yaml   |   52 +
 drivers/clk/meson/Kconfig                     |   18 +
 drivers/clk/meson/Makefile                    |    2 +
 drivers/clk/meson/a1-pll.c                    |  360 +++
 drivers/clk/meson/a1-pll.h                    |   56 +
 drivers/clk/meson/a1.c                        | 2249 +++++++++++++++++
 drivers/clk/meson/a1.h                        |  120 +
 drivers/clk/meson/clk-pll.c                   |   47 +-
 drivers/clk/meson/clk-pll.h                   |    2 +
 include/dt-bindings/clock/a1-clkc.h           |   98 +
 include/dt-bindings/clock/a1-pll-clkc.h       |   16 +
 12 files changed, 3078 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-clkc.yaml
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
 create mode 100644 drivers/clk/meson/a1-pll.c
 create mode 100644 drivers/clk/meson/a1-pll.h
 create mode 100644 drivers/clk/meson/a1.c
 create mode 100644 drivers/clk/meson/a1.h
 create mode 100644 include/dt-bindings/clock/a1-clkc.h
 create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h

-- 
2.24.0

[PATCH v7 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings

[Jian Hu]

Add the documentation to support Amlogic A1 PLL clock driver,
and add A1 PLL clock controller bindings.

Signed-off-by: Jian Hu <jian.hu@amlogic.com>
---
 .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
 include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
 2 files changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
 create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h

diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
new file mode 100644
index 000000000000..d67250fbeece
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/amlogic,a1-pll-clkc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Amlogic Meson A/C serials PLL Clock Control Unit Device Tree Bindings
+
+maintainers:
+  - Neil Armstrong <narmstrong@baylibre.com>
+  - Jerome Brunet <jbrunet@baylibre.com>
+  - Jian Hu <jian.hu@jian.hu.com>
+
+properties:
+  compatible:
+    const: amlogic,a1-pll-clkc
+
+  "#clock-cells":
+    const: 1
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+     - description: input xtal_fixpll
+     - description: input xtal_hifipll
+
+  clock-names:
+    items:
+      - const: xtal_fixpll
+      - const: xtal_hifipll
+
+required:
+  - compatible
+  - "#clock-cells"
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    clkc_pll: pll-clock-controller@7c80 {
+                compatible = "amlogic,a1-pll-clkc";
+                reg = <0 0x7c80 0 0x18c>;
+                #clock-cells = <1>;
+                clocks = <&clkc_periphs 1>,
+                         <&clkc_periphs 4>;
+                clock-names = "xtal_fixpll", "xtal_hifipll";
+    };
diff --git a/include/dt-bindings/clock/a1-pll-clkc.h b/include/dt-bindings/clock/a1-pll-clkc.h
new file mode 100644
index 000000000000..58eae237e503
--- /dev/null
+++ b/include/dt-bindings/clock/a1-pll-clkc.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ */
+
+#ifndef __A1_PLL_CLKC_H
+#define __A1_PLL_CLKC_H
+
+#define CLKID_FIXED_PLL				1
+#define CLKID_FCLK_DIV2				6
+#define CLKID_FCLK_DIV3				7
+#define CLKID_FCLK_DIV5				8
+#define CLKID_FCLK_DIV7				9
+#define CLKID_HIFI_PLL				10
+
+#endif /* __A1_PLL_CLKC_H */
-- 
2.24.0

[PATCH v7 2/5] clk: meson: add support for A1 PLL clock ops

[Jian Hu]

Compared with the previous SoCs, self-adaption current module
is newly added for A1, and there is no reset parm except the
fixed pll. In A1 PLL, the PLL enable sequence is different, using
the new power-on sequence to enable the PLL.

Signed-off-by: Jian Hu <jian.hu@amlogic.com>
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/clk-pll.c | 47 +++++++++++++++++++++++++++++++------
 drivers/clk/meson/clk-pll.h |  2 ++
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index ddb1e5634739..10926291440f 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -283,10 +283,14 @@ static void meson_clk_pll_init(struct clk_hw *hw)
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
 
 	if (pll->init_count) {
-		meson_parm_write(clk->map, &pll->rst, 1);
+		if (MESON_PARM_APPLICABLE(&pll->rst))
+			meson_parm_write(clk->map, &pll->rst, 1);
+
 		regmap_multi_reg_write(clk->map, pll->init_regs,
 				       pll->init_count);
-		meson_parm_write(clk->map, &pll->rst, 0);
+
+		if (MESON_PARM_APPLICABLE(&pll->rst))
+			meson_parm_write(clk->map, &pll->rst, 0);
 	}
 }
 
@@ -295,8 +299,11 @@ static int meson_clk_pll_is_enabled(struct clk_hw *hw)
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
 
-	if (meson_parm_read(clk->map, &pll->rst) ||
-	    !meson_parm_read(clk->map, &pll->en) ||
+	if (MESON_PARM_APPLICABLE(&pll->rst) &&
+	    meson_parm_read(clk->map, &pll->rst))
+		return 0;
+
+	if (!meson_parm_read(clk->map, &pll->en) ||
 	    !meson_parm_read(clk->map, &pll->l))
 		return 0;
 
@@ -323,13 +330,34 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
 		return 0;
 
 	/* Make sure the pll is in reset */
-	meson_parm_write(clk->map, &pll->rst, 1);
+	if (MESON_PARM_APPLICABLE(&pll->rst))
+		meson_parm_write(clk->map, &pll->rst, 1);
 
 	/* Enable the pll */
 	meson_parm_write(clk->map, &pll->en, 1);
 
 	/* Take the pll out reset */
-	meson_parm_write(clk->map, &pll->rst, 0);
+	if (MESON_PARM_APPLICABLE(&pll->rst))
+		meson_parm_write(clk->map, &pll->rst, 0);
+
+	/*
+	 * Compared with the previous SoCs, self-adaption current module
+	 * is newly added for A1, keep the new power-on sequence to enable the
+	 * PLL. The sequence is:
+	 * 1. enable the pll, delay for 10us
+	 * 2. enable the pll self-adaption current module, delay for 40us
+	 * 3. enable the lock detect module
+	 */
+	if (MESON_PARM_APPLICABLE(&pll->current_en)) {
+		udelay(10);
+		meson_parm_write(clk->map, &pll->current_en, 1);
+		udelay(40);
+	};
+
+	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
+		meson_parm_write(clk->map, &pll->l_detect, 1);
+		meson_parm_write(clk->map, &pll->l_detect, 0);
+	}
 
 	if (meson_clk_pll_wait_lock(hw))
 		return -EIO;
@@ -343,10 +371,15 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
 
 	/* Put the pll is in reset */
-	meson_parm_write(clk->map, &pll->rst, 1);
+	if (MESON_PARM_APPLICABLE(&pll->rst))
+		meson_parm_write(clk->map, &pll->rst, 1);
 
 	/* Disable the pll */
 	meson_parm_write(clk->map, &pll->en, 0);
+
+	/* Disable PLL internal self-adaption current module */
+	if (MESON_PARM_APPLICABLE(&pll->current_en))
+		meson_parm_write(clk->map, &pll->current_en, 0);
 }
 
 static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
index 367efd0f6410..a2228c0fdce5 100644
--- a/drivers/clk/meson/clk-pll.h
+++ b/drivers/clk/meson/clk-pll.h
@@ -36,6 +36,8 @@ struct meson_clk_pll_data {
 	struct parm frac;
 	struct parm l;
 	struct parm rst;
+	struct parm current_en;
+	struct parm l_detect;
 	const struct reg_sequence *init_regs;
 	unsigned int init_count;
 	const struct pll_params_table *table;
-- 
2.24.0

Re: [PATCH v7 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings

[Rob Herring]

On Mon, 20 Jan 2020 11:49:33 +0800, Jian Hu wrote:
> Add the documentation to support Amlogic A1 PLL clock driver,
> and add A1 PLL clock controller bindings.
> 
> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
> ---
>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
>  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>  2 files changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
> 

Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.

Re: [PATCH v7 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings

[Jian Hu]

On 2020/1/22 6:00, Rob Herring wrote:
> On Mon, 20 Jan 2020 11:49:33 +0800, Jian Hu wrote:
>> Add the documentation to support Amlogic A1 PLL clock driver,
>> and add A1 PLL clock controller bindings.
>>
>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>> ---
>>   .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
>>   include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>>   2 files changed, 68 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>   create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
>>
> 
> Please add Acked-by/Reviewed-by tags when posting new versions. However,
> there's no need to repost patches *only* to add the tags. The upstream
> maintainer will do that for acks received on the version they apply.
> 
> If a tag was not added on purpose, please state why and what changed.
> 
> .
> 
I had gotten your Reviewed-by tag in v5 version, And the tag is missing 
in v6, I am sorry about it. There is something wrong in v6, it can not 
be compiled by dt_binding_check. It had been fixed in v7. Should I wait 
for your tag again?

I will send v8 because the driver is changed a bit. Could I add your tag?

Re: [PATCH v7 2/5] clk: meson: add support for A1 PLL clock ops

[Jerome Brunet]

On Mon 20 Jan 2020 at 04:49, Jian Hu <jian.hu@amlogic.com> wrote:

> Compared with the previous SoCs, self-adaption current module
> is newly added for A1, and there is no reset parm except the
> fixed pll. In A1 PLL, the PLL enable sequence is different, using
> the new power-on sequence to enable the PLL.

Things are getting clearer thanks to Martin's suggestions and I can
understand what your driver is doing now

However, I still have a problem with the fact that 2 different pll types
are getting intertwined in this driver. Parameters mandatory to one is
made optional to the other. Nothing clearly shows which needs what and
the combinatorial are quickly growing.

Apparently the only real difference is in enable/disable, So I would
prefer if the a1 had dedicated function for these ops.

I suppose you'll have to submit clk_hw_enable() and clk_hw_disable()
to the framework to call the appropriate ops dependind on the SoC.

>
> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/clk-pll.c | 47 +++++++++++++++++++++++++++++++------
>  drivers/clk/meson/clk-pll.h |  2 ++
>  2 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index ddb1e5634739..10926291440f 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -283,10 +283,14 @@ static void meson_clk_pll_init(struct clk_hw *hw)
>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>  
>  	if (pll->init_count) {
> -		meson_parm_write(clk->map, &pll->rst, 1);
> +		if (MESON_PARM_APPLICABLE(&pll->rst))
> +			meson_parm_write(clk->map, &pll->rst, 1);
> +

replace by
        enabled = clk_hw_is_enabled(hw)
        if (enabled)
           clk_hw_disable(hw)

>  		regmap_multi_reg_write(clk->map, pll->init_regs,
>  				       pll->init_count);
> -		meson_parm_write(clk->map, &pll->rst, 0);
> +
> +		if (MESON_PARM_APPLICABLE(&pll->rst))
> +			meson_parm_write(clk->map, &pll->rst, 0);

       /* restore if necessary */
       if (enabled)
          clk_hw_enable(hw)

>  	}
>  }
>  
> @@ -295,8 +299,11 @@ static int meson_clk_pll_is_enabled(struct clk_hw *hw)
>  	struct clk_regmap *clk = to_clk_regmap(hw);
>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>  
> -	if (meson_parm_read(clk->map, &pll->rst) ||
> -	    !meson_parm_read(clk->map, &pll->en) ||
> +	if (MESON_PARM_APPLICABLE(&pll->rst) &&
> +	    meson_parm_read(clk->map, &pll->rst))
> +		return 0;
> +
> +	if (!meson_parm_read(clk->map, &pll->en) ||
>  	    !meson_parm_read(clk->map, &pll->l))
>  		return 0;

I suppose the pll can't be locked if it was in reset, so we could drop
the check on `rst` entirely to simplify the function

>  
> @@ -323,13 +330,34 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>  		return 0;
>  
>  	/* Make sure the pll is in reset */
> -	meson_parm_write(clk->map, &pll->rst, 1);
> +	if (MESON_PARM_APPLICABLE(&pll->rst))
> +		meson_parm_write(clk->map, &pll->rst, 1);
>  
>  	/* Enable the pll */
>  	meson_parm_write(clk->map, &pll->en, 1);
>  
>  	/* Take the pll out reset */
> -	meson_parm_write(clk->map, &pll->rst, 0);
> +	if (MESON_PARM_APPLICABLE(&pll->rst))
> +		meson_parm_write(clk->map, &pll->rst, 0);
> +
> +	/*
> +	 * Compared with the previous SoCs, self-adaption current module
> +	 * is newly added for A1, keep the new power-on sequence to enable the
> +	 * PLL. The sequence is:
> +	 * 1. enable the pll, delay for 10us
> +	 * 2. enable the pll self-adaption current module, delay for 40us
> +	 * 3. enable the lock detect module
> +	 */
> +	if (MESON_PARM_APPLICABLE(&pll->current_en)) {
> +		udelay(10);
> +		meson_parm_write(clk->map, &pll->current_en, 1);
> +		udelay(40);
> +	};
> +
> +	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
> +		meson_parm_write(clk->map, &pll->l_detect, 1);
> +		meson_parm_write(clk->map, &pll->l_detect, 0);
> +	}
>  
>  	if (meson_clk_pll_wait_lock(hw))
>  		return -EIO;
> @@ -343,10 +371,15 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>  
>  	/* Put the pll is in reset */
> -	meson_parm_write(clk->map, &pll->rst, 1);
> +	if (MESON_PARM_APPLICABLE(&pll->rst))
> +		meson_parm_write(clk->map, &pll->rst, 1);
>  
>  	/* Disable the pll */
>  	meson_parm_write(clk->map, &pll->en, 0);
> +
> +	/* Disable PLL internal self-adaption current module */
> +	if (MESON_PARM_APPLICABLE(&pll->current_en))
> +		meson_parm_write(clk->map, &pll->current_en, 0);
>  }

With the above clarified, it should be easy to properly split the
functions between the legacy type and the a1 type.

You'll need to update meson_clk_pll_set_rate() to call
 - clk_hw_is_enabled()
 - clk_hw_enable() and clk_hw_disable() (again, you'll need to add
 those in the framework first)

>  
>  static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
> index 367efd0f6410..a2228c0fdce5 100644
> --- a/drivers/clk/meson/clk-pll.h
> +++ b/drivers/clk/meson/clk-pll.h
> @@ -36,6 +36,8 @@ struct meson_clk_pll_data {
>  	struct parm frac;
>  	struct parm l;
>  	struct parm rst;
> +	struct parm current_en;
> +	struct parm l_detect;
>  	const struct reg_sequence *init_regs;
>  	unsigned int init_count;
>  	const struct pll_params_table *table;

Re: [PATCH v7 2/5] clk: meson: add support for A1 PLL clock ops

[Jian Hu]

Hi Jerome

Thanks for your suggestions.

On 2020/2/4 18:24, Jerome Brunet wrote:
> 
> On Mon 20 Jan 2020 at 04:49, Jian Hu <jian.hu@amlogic.com> wrote:
> 
>> Compared with the previous SoCs, self-adaption current module
>> is newly added for A1, and there is no reset parm except the
>> fixed pll. In A1 PLL, the PLL enable sequence is different, using
>> the new power-on sequence to enable the PLL.
> 
> Things are getting clearer thanks to Martin's suggestions and I can
> understand what your driver is doing now
> 
> However, I still have a problem with the fact that 2 different pll types
> are getting intertwined in this driver. Parameters mandatory to one is
> made optional to the other. Nothing clearly shows which needs what and
> the combinatorial are quickly growing.
> 
> Apparently the only real difference is in enable/disable, So I would
> prefer if the a1 had dedicated function for these ops.
> 
> I suppose you'll have to submit clk_hw_enable() and clk_hw_disable()
> to the framework to call the appropriate ops dependind on the SoC.
> 
I am confused here.
What does clk_hw_is_enabled/clk_hw_enable/clk_hw_disable use here?

clk_hw_is_enabled is intend to check a parm's existence? But 
clk_hw_is_enabled which is existed in CCF to check a PLL is locked or
not. Maybe I understand wrong about your suggestions.

Could you list a example for clk_hw_enable and clk_hw_disable function 
implementation?
>>
>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>   drivers/clk/meson/clk-pll.c | 47 +++++++++++++++++++++++++++++++------
>>   drivers/clk/meson/clk-pll.h |  2 ++
>>   2 files changed, 42 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index ddb1e5634739..10926291440f 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -283,10 +283,14 @@ static void meson_clk_pll_init(struct clk_hw *hw)
>>   	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>   
>>   	if (pll->init_count) {
>> -		meson_parm_write(clk->map, &pll->rst, 1);
>> +		if (MESON_PARM_APPLICABLE(&pll->rst))
>> +			meson_parm_write(clk->map, &pll->rst, 1);
>> +
> 
> replace by
>          enabled = clk_hw_is_enabled(hw)
>          if (enabled)
>             clk_hw_disable(hw)
> 
clk_hw_is_enabled here is used to check 'pll->rst'?
>>   		regmap_multi_reg_write(clk->map, pll->init_regs,
>>   				       pll->init_count);
>> -		meson_parm_write(clk->map, &pll->rst, 0);
>> +
>> +		if (MESON_PARM_APPLICABLE(&pll->rst))
>> +			meson_parm_write(clk->map, &pll->rst, 0);
> 
>         /* restore if necessary */
>         if (enabled)
>            clk_hw_enable(hw)
> 
>>   	}
>>   }
>>   
>> @@ -295,8 +299,11 @@ static int meson_clk_pll_is_enabled(struct clk_hw *hw)
>>   	struct clk_regmap *clk = to_clk_regmap(hw);
>>   	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>   
>> -	if (meson_parm_read(clk->map, &pll->rst) ||
>> -	    !meson_parm_read(clk->map, &pll->en) ||
>> +	if (MESON_PARM_APPLICABLE(&pll->rst) &&
>> +	    meson_parm_read(clk->map, &pll->rst))
>> +		return 0;
>> +
>> +	if (!meson_parm_read(clk->map, &pll->en) ||
>>   	    !meson_parm_read(clk->map, &pll->l))
>>   		return 0;
> 
> I suppose the pll can't be locked if it was in reset, so we could drop
> the check on `rst` entirely to simplify the function
> 
OK, I will drop 'rst' check.
>>   
>> @@ -323,13 +330,34 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>   		return 0;
>>   
>>   	/* Make sure the pll is in reset */
>> -	meson_parm_write(clk->map, &pll->rst, 1);
>> +	if (MESON_PARM_APPLICABLE(&pll->rst))
>> +		meson_parm_write(clk->map, &pll->rst, 1);
>>   
>>   	/* Enable the pll */
>>   	meson_parm_write(clk->map, &pll->en, 1);
>>   
>>   	/* Take the pll out reset */
>> -	meson_parm_write(clk->map, &pll->rst, 0);
>> +	if (MESON_PARM_APPLICABLE(&pll->rst))
>> +		meson_parm_write(clk->map, &pll->rst, 0);
>> +
>> +	/*
>> +	 * Compared with the previous SoCs, self-adaption current module
>> +	 * is newly added for A1, keep the new power-on sequence to enable the
>> +	 * PLL. The sequence is:
>> +	 * 1. enable the pll, delay for 10us
>> +	 * 2. enable the pll self-adaption current module, delay for 40us
>> +	 * 3. enable the lock detect module
>> +	 */
>> +	if (MESON_PARM_APPLICABLE(&pll->current_en)) {
>> +		udelay(10);
>> +		meson_parm_write(clk->map, &pll->current_en, 1);
>> +		udelay(40);
>> +	};
>> +
>> +	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
>> +		meson_parm_write(clk->map, &pll->l_detect, 1);
>> +		meson_parm_write(clk->map, &pll->l_detect, 0);
>> +	}
>>   
>>   	if (meson_clk_pll_wait_lock(hw))
>>   		return -EIO;
>> @@ -343,10 +371,15 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
>>   	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>   
>>   	/* Put the pll is in reset */
>> -	meson_parm_write(clk->map, &pll->rst, 1);
>> +	if (MESON_PARM_APPLICABLE(&pll->rst))
>> +		meson_parm_write(clk->map, &pll->rst, 1);
>>   
>>   	/* Disable the pll */
>>   	meson_parm_write(clk->map, &pll->en, 0);
>> +
>> +	/* Disable PLL internal self-adaption current module */
>> +	if (MESON_PARM_APPLICABLE(&pll->current_en))
>> +		meson_parm_write(clk->map, &pll->current_en, 0);
>>   }
> 
> With the above clarified, it should be easy to properly split the
> functions between the legacy type and the a1 type.
> 
> You'll need to update meson_clk_pll_set_rate() to call
>   - clk_hw_is_enabled()
>   - clk_hw_enable() and clk_hw_disable() (again, you'll need to add
>   those in the framework first)
> 
>>   
>>   static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
>> index 367efd0f6410..a2228c0fdce5 100644
>> --- a/drivers/clk/meson/clk-pll.h
>> +++ b/drivers/clk/meson/clk-pll.h
>> @@ -36,6 +36,8 @@ struct meson_clk_pll_data {
>>   	struct parm frac;
>>   	struct parm l;
>>   	struct parm rst;
>> +	struct parm current_en;
>> +	struct parm l_detect;
>>   	const struct reg_sequence *init_regs;
>>   	unsigned int init_count;
>>   	const struct pll_params_table *table;
> 
> .
> 

Re: [PATCH v7 2/5] clk: meson: add support for A1 PLL clock ops

[Jerome Brunet]

On Mon 10 Feb 2020 at 07:11, Jian Hu <jian.hu@amlogic.com> wrote:

> Hi Jerome
>
> Thanks for your suggestions.
>
> On 2020/2/4 18:24, Jerome Brunet wrote:
>>
>> On Mon 20 Jan 2020 at 04:49, Jian Hu <jian.hu@amlogic.com> wrote:
>>
>>> Compared with the previous SoCs, self-adaption current module
>>> is newly added for A1, and there is no reset parm except the
>>> fixed pll. In A1 PLL, the PLL enable sequence is different, using
>>> the new power-on sequence to enable the PLL.
>>
>> Things are getting clearer thanks to Martin's suggestions and I can
>> understand what your driver is doing now
>>
>> However, I still have a problem with the fact that 2 different pll types
>> are getting intertwined in this driver. Parameters mandatory to one is
>> made optional to the other. Nothing clearly shows which needs what and
>> the combinatorial are quickly growing.
>>
>> Apparently the only real difference is in enable/disable, So I would
>> prefer if the a1 had dedicated function for these ops.
>>
>> I suppose you'll have to submit clk_hw_enable() and clk_hw_disable()
>> to the framework to call the appropriate ops dependind on the SoC.
>>
> I am confused here.
> What does clk_hw_is_enabled/clk_hw_enable/clk_hw_disable use here?

I'm asking you to make different callback for .enable() and .disable().
The .set_rate() callback of this driver needs to turn off and on the
clock, so it needs to call the appropriate one.

To do so, you should go through a clk_hw_xxxxx() function.

>
> clk_hw_is_enabled is intend to check a parm's existence? But
> clk_hw_is_enabled which is existed in CCF to check a PLL is locked or
> not. Maybe I understand wrong about your suggestions.
>
> Could you list a example for clk_hw_enable and clk_hw_disable function
> implementation?

drivers/clk/clk.c:523 : clk_hw_is_enabled()

clk_hw_enable() and clk_hw_disable() so you'll need to implement these
one and submit them.

>>>
>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>>> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>> ---
>>>   drivers/clk/meson/clk-pll.c | 47 +++++++++++++++++++++++++++++++------
>>>   drivers/clk/meson/clk-pll.h |  2 ++
>>>   2 files changed, 42 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>> index ddb1e5634739..10926291440f 100644
>>> --- a/drivers/clk/meson/clk-pll.c
>>> +++ b/drivers/clk/meson/clk-pll.c
>>> @@ -283,10 +283,14 @@ static void meson_clk_pll_init(struct clk_hw *hw)
>>>   	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>>     	if (pll->init_count) {
>>> -		meson_parm_write(clk->map, &pll->rst, 1);
>>> +		if (MESON_PARM_APPLICABLE(&pll->rst))
>>> +			meson_parm_write(clk->map, &pll->rst, 1);
>>> +
>>
>> replace by
>>          enabled = clk_hw_is_enabled(hw)
>>          if (enabled)
>>             clk_hw_disable(hw)
>>
> clk_hw_is_enabled here is used to check 'pll->rst'?
>>>   		regmap_multi_reg_write(clk->map, pll->init_regs,
>>>   				       pll->init_count);
>>> -		meson_parm_write(clk->map, &pll->rst, 0);
>>> +
>>> +		if (MESON_PARM_APPLICABLE(&pll->rst))
>>> +			meson_parm_write(clk->map, &pll->rst, 0);
>>
>>         /* restore if necessary */
>>         if (enabled)
>>            clk_hw_enable(hw)
>>
>>>   	}
>>>   }
>>>   @@ -295,8 +299,11 @@ static int meson_clk_pll_is_enabled(struct clk_hw
>>> *hw)
>>>   	struct clk_regmap *clk = to_clk_regmap(hw);
>>>   	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>>   -	if (meson_parm_read(clk->map, &pll->rst) ||
>>> -	    !meson_parm_read(clk->map, &pll->en) ||
>>> +	if (MESON_PARM_APPLICABLE(&pll->rst) &&
>>> +	    meson_parm_read(clk->map, &pll->rst))
>>> +		return 0;
>>> +
>>> +	if (!meson_parm_read(clk->map, &pll->en) ||
>>>   	    !meson_parm_read(clk->map, &pll->l))
>>>   		return 0;
>>
>> I suppose the pll can't be locked if it was in reset, so we could drop
>> the check on `rst` entirely to simplify the function
>>
> OK, I will drop 'rst' check.
>>>   @@ -323,13 +330,34 @@ static int meson_clk_pll_enable(struct clk_hw
>>> *hw)
>>>   		return 0;
>>>     	/* Make sure the pll is in reset */
>>> -	meson_parm_write(clk->map, &pll->rst, 1);
>>> +	if (MESON_PARM_APPLICABLE(&pll->rst))
>>> +		meson_parm_write(clk->map, &pll->rst, 1);
>>>     	/* Enable the pll */
>>>   	meson_parm_write(clk->map, &pll->en, 1);
>>>     	/* Take the pll out reset */
>>> -	meson_parm_write(clk->map, &pll->rst, 0);
>>> +	if (MESON_PARM_APPLICABLE(&pll->rst))
>>> +		meson_parm_write(clk->map, &pll->rst, 0);
>>> +
>>> +	/*
>>> +	 * Compared with the previous SoCs, self-adaption current module
>>> +	 * is newly added for A1, keep the new power-on sequence to enable the
>>> +	 * PLL. The sequence is:
>>> +	 * 1. enable the pll, delay for 10us
>>> +	 * 2. enable the pll self-adaption current module, delay for 40us
>>> +	 * 3. enable the lock detect module
>>> +	 */
>>> +	if (MESON_PARM_APPLICABLE(&pll->current_en)) {
>>> +		udelay(10);
>>> +		meson_parm_write(clk->map, &pll->current_en, 1);
>>> +		udelay(40);
>>> +	};
>>> +
>>> +	if (MESON_PARM_APPLICABLE(&pll->l_detect)) {
>>> +		meson_parm_write(clk->map, &pll->l_detect, 1);
>>> +		meson_parm_write(clk->map, &pll->l_detect, 0);
>>> +	}
>>>     	if (meson_clk_pll_wait_lock(hw))
>>>   		return -EIO;
>>> @@ -343,10 +371,15 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
>>>   	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>>     	/* Put the pll is in reset */
>>> -	meson_parm_write(clk->map, &pll->rst, 1);
>>> +	if (MESON_PARM_APPLICABLE(&pll->rst))
>>> +		meson_parm_write(clk->map, &pll->rst, 1);
>>>     	/* Disable the pll */
>>>   	meson_parm_write(clk->map, &pll->en, 0);
>>> +
>>> +	/* Disable PLL internal self-adaption current module */
>>> +	if (MESON_PARM_APPLICABLE(&pll->current_en))
>>> +		meson_parm_write(clk->map, &pll->current_en, 0);
>>>   }
>>
>> With the above clarified, it should be easy to properly split the
>> functions between the legacy type and the a1 type.
>>
>> You'll need to update meson_clk_pll_set_rate() to call
>>   - clk_hw_is_enabled()
>>   - clk_hw_enable() and clk_hw_disable() (again, you'll need to add
>>   those in the framework first)
>>
>>>     static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long
>>> rate,
>>> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
>>> index 367efd0f6410..a2228c0fdce5 100644
>>> --- a/drivers/clk/meson/clk-pll.h
>>> +++ b/drivers/clk/meson/clk-pll.h
>>> @@ -36,6 +36,8 @@ struct meson_clk_pll_data {
>>>   	struct parm frac;
>>>   	struct parm l;
>>>   	struct parm rst;
>>> +	struct parm current_en;
>>> +	struct parm l_detect;
>>>   	const struct reg_sequence *init_regs;
>>>   	unsigned int init_count;
>>>   	const struct pll_params_table *table;
>>
>> .
>>