devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/5] add Amlogic A1 clock controller driver
@ 2019-12-27  9:46 Jian Hu
  2019-12-27  9:46 ` [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
  2019-12-27  9:46 ` [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops Jian Hu
  0 siblings, 2 replies; 8+ messages in thread
From: Jian Hu @ 2019-12-27  9:46 UTC (permalink / raw)
  To: Jerome Brunet, Neil Armstrong
  Cc: Jian Hu, Kevin Hilman, Rob Herring, Martin Blumenstingl,
	Michael Turquette, Stephen Boyd, Qiufang Dai, Jianxin Pan,
	Victor Wan, Chandle Zou, linux-clk, linux-amlogic,
	linux-arm-kernel, linux-kernel, devicetree

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 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

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       |   67 +
 .../bindings/clock/amlogic,a1-pll-clkc.yaml   |   54 +
 drivers/clk/meson/Kconfig                     |   18 +
 drivers/clk/meson/Makefile                    |    2 +
 drivers/clk/meson/a1-pll.c                    |  374 +++
 drivers/clk/meson/a1-pll.h                    |   56 +
 drivers/clk/meson/a1.c                        | 2263 +++++++++++++++++
 drivers/clk/meson/a1.h                        |  120 +
 drivers/clk/meson/clk-pll.c                   |   40 +-
 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, 3105 insertions(+), 5 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings
  2019-12-27  9:46 [PATCH v5 0/5] add Amlogic A1 clock controller driver Jian Hu
@ 2019-12-27  9:46 ` Jian Hu
  2020-01-03 22:29   ` Rob Herring
  2020-01-10 15:36   ` Jerome Brunet
  2019-12-27  9:46 ` [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops Jian Hu
  1 sibling, 2 replies; 8+ messages in thread
From: Jian Hu @ 2019-12-27  9:46 UTC (permalink / raw)
  To: Jerome Brunet, Neil Armstrong
  Cc: Jian Hu, Kevin Hilman, Rob Herring, Martin Blumenstingl,
	Michael Turquette, Stephen Boyd, Qiufang Dai, Jianxin Pan,
	Victor Wan, Chandle Zou, linux-clk, linux-amlogic,
	linux-arm-kernel, linux-kernel, devicetree

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   | 54 +++++++++++++++++++
 include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
 2 files changed, 70 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..7a327bb174b8
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/clock/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:
+  maxItems: 2
+  items:
+   - description: Input xtal_fixpll
+   - description: Input xtal_hifipll
+
+clock-names:
+  maxItems: 2
+  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 CLKID_XTAL_FIXPLL>,
+                         <&clkc_periphs CLKID_XTAL_HIFIPLL>;
+                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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops
  2019-12-27  9:46 [PATCH v5 0/5] add Amlogic A1 clock controller driver Jian Hu
  2019-12-27  9:46 ` [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
@ 2019-12-27  9:46 ` Jian Hu
  2019-12-27 16:53   ` Martin Blumenstingl
  1 sibling, 1 reply; 8+ messages in thread
From: Jian Hu @ 2019-12-27  9:46 UTC (permalink / raw)
  To: Jerome Brunet, Neil Armstrong
  Cc: Jian Hu, Kevin Hilman, Rob Herring, Martin Blumenstingl,
	Michael Turquette, Stephen Boyd, Qiufang Dai, Jianxin Pan,
	Victor Wan, Chandle Zou, linux-clk, linux-amlogic,
	linux-arm-kernel, linux-kernel, devicetree

Compared with the previous SoCs, self-adaption module current
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>
---
 drivers/clk/meson/clk-pll.c | 40 ++++++++++++++++++++++++++++++++-----
 drivers/clk/meson/clk-pll.h |  2 ++
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index ddb1e5634739..9eb7d465d123 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);
 	}
 }
 
@@ -294,9 +298,12 @@ 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);
+	int ret = 0;
 
-	if (meson_parm_read(clk->map, &pll->rst) ||
-	    !meson_parm_read(clk->map, &pll->en) ||
+	if (MESON_PARM_APPLICABLE(&pll->rst))
+		ret = meson_parm_read(clk->map, &pll->rst);
+
+	if (ret || !meson_parm_read(clk->map, &pll->en) ||
 	    !meson_parm_read(clk->map, &pll->l))
 		return 0;
 
@@ -321,6 +328,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
 	/* do nothing if the PLL is already enabled */
 	if (clk_hw_is_enabled(hw))
 		return 0;
+	/*
+	 * Compared with the previous SoCs, self-adaption module current
+	 * is newly added for A1, keep the new power-on sequence to enable the
+	 * PLL.
+	 */
+	if (MESON_PARM_APPLICABLE(&pll->current_en)) {
+		/* Enable the pll */
+		meson_parm_write(clk->map, &pll->en, 1);
+		udelay(10);
+		/* Enable the pll self-adaption module current */
+		meson_parm_write(clk->map, &pll->current_en, 1);
+		udelay(40);
+		/* Enable lock detect module */
+		meson_parm_write(clk->map, &pll->l_detect, 1);
+		meson_parm_write(clk->map, &pll->l_detect, 0);
+		goto out;
+	}
 
 	/* Make sure the pll is in reset */
 	meson_parm_write(clk->map, &pll->rst, 1);
@@ -331,6 +355,7 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
 	/* Take the pll out reset */
 	meson_parm_write(clk->map, &pll->rst, 0);
 
+out:
 	if (meson_clk_pll_wait_lock(hw))
 		return -EIO;
 
@@ -343,10 +368,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 module current */
+	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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops
  2019-12-27  9:46 ` [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops Jian Hu
@ 2019-12-27 16:53   ` Martin Blumenstingl
  2020-01-09  6:55     ` Jian Hu
  0 siblings, 1 reply; 8+ messages in thread
From: Martin Blumenstingl @ 2019-12-27 16:53 UTC (permalink / raw)
  To: Jian Hu
  Cc: Jerome Brunet, Neil Armstrong, Kevin Hilman, Rob Herring,
	Michael Turquette, Stephen Boyd, Qiufang Dai, Jianxin Pan,
	Victor Wan, Chandle Zou, linux-clk, linux-amlogic,
	linux-arm-kernel, linux-kernel, devicetree

Hi Jian,

On Fri, Dec 27, 2019 at 10:46 AM Jian Hu <jian.hu@amlogic.com> wrote:
[...]
> @@ -294,9 +298,12 @@ 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);
> +       int ret = 0;
>
> -       if (meson_parm_read(clk->map, &pll->rst) ||
> -           !meson_parm_read(clk->map, &pll->en) ||
> +       if (MESON_PARM_APPLICABLE(&pll->rst))
> +               ret = meson_parm_read(clk->map, &pll->rst);
> +
> +       if (ret || !meson_parm_read(clk->map, &pll->en) ||
>             !meson_parm_read(clk->map, &pll->l))
>                 return 0;
I had to read this part twice to understand what it's doing because I
misunderstood what "ret" is used for (I thought that some "return ret"
is missing)
my proposal to make it easier to read:
...
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;
...

please let me know what you think about this

> @@ -321,6 +328,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>         /* do nothing if the PLL is already enabled */
>         if (clk_hw_is_enabled(hw))
>                 return 0;
> +       /*
> +        * Compared with the previous SoCs, self-adaption module current
> +        * is newly added for A1, keep the new power-on sequence to enable the
> +        * PLL.
> +        */
> +       if (MESON_PARM_APPLICABLE(&pll->current_en)) {
> +               /* Enable the pll */
> +               meson_parm_write(clk->map, &pll->en, 1);
> +               udelay(10);
> +               /* Enable the pll self-adaption module current */
> +               meson_parm_write(clk->map, &pll->current_en, 1);
> +               udelay(40);
> +               /* Enable lock detect module */
> +               meson_parm_write(clk->map, &pll->l_detect, 1);
> +               meson_parm_write(clk->map, &pll->l_detect, 0);
> +               goto out;
> +       }
in all other functions you are skipping the pll->rst register by
checking for MESON_PARM_APPLICABLE(&pll->rst)
I like that because it's a pattern which is easy to follow

do you think we can make this part consistent with that?
I'm thinking of something like this (not compile-tested and I dropped
all comments, just so you get the idea):
...
if (MESON_PARM_APPLICABLE(&pll->rst)
  meson_parm_write(clk->map, &pll->rst, 1);

meson_parm_write(clk->map, &pll->en, 1);

if (MESON_PARM_APPLICABLE(&pll->rst))
  meson_parm_write(clk->map, &pll->rst, 0);

if (MESON_PARM_APPLICABLE(&pll->current_en))
  meson_parm_write(clk->map, &pll->current_en, 1);

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))
...

I see two (and a half) benefits here:
- if there's a PLL with neither the pll->current_en nor the pll->rst
registers then you get support for this implementation for free
- the if (MESON_PARM_APPLICABLE(...)) pattern is already used in the
driver, but only for one register (in your example when
MESON_PARM_APPLICABLE(&pll->current_en) exists you also modify the
pll->l_detect register, which I did not expect)
- only counts half: no use of "goto", which in my opinion makes it
very easy to read (just read from top to bottom, checking each "if")


Martin

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings
  2019-12-27  9:46 ` [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
@ 2020-01-03 22:29   ` Rob Herring
  2020-01-10 15:36   ` Jerome Brunet
  1 sibling, 0 replies; 8+ messages in thread
From: Rob Herring @ 2020-01-03 22:29 UTC (permalink / raw)
  To: Jian Hu
  Cc: Jerome Brunet, Neil Armstrong, Jian Hu, Kevin Hilman,
	Rob Herring, Martin Blumenstingl, Michael Turquette,
	Stephen Boyd, Qiufang Dai, Jianxin Pan, Victor Wan, Chandle Zou,
	linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel,
	devicetree

On Fri, 27 Dec 2019 17:46:02 +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   | 54 +++++++++++++++++++
>  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>  2 files changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
> 

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops
  2019-12-27 16:53   ` Martin Blumenstingl
@ 2020-01-09  6:55     ` Jian Hu
  0 siblings, 0 replies; 8+ messages in thread
From: Jian Hu @ 2020-01-09  6:55 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: Jerome Brunet, Neil Armstrong, Kevin Hilman, Rob Herring,
	Michael Turquette, Stephen Boyd, Qiufang Dai, Jianxin Pan,
	Victor Wan, Chandle Zou, linux-clk, linux-amlogic,
	linux-arm-kernel, linux-kernel, devicetree

Hi Martin

Thanks for your review

On 2019/12/28 0:53, Martin Blumenstingl wrote:
> Hi Jian,
> 
> On Fri, Dec 27, 2019 at 10:46 AM Jian Hu <jian.hu@amlogic.com> wrote:
> [...]
>> @@ -294,9 +298,12 @@ 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);
>> +       int ret = 0;
>>
>> -       if (meson_parm_read(clk->map, &pll->rst) ||
>> -           !meson_parm_read(clk->map, &pll->en) ||
>> +       if (MESON_PARM_APPLICABLE(&pll->rst))
>> +               ret = meson_parm_read(clk->map, &pll->rst);
>> +
>> +       if (ret || !meson_parm_read(clk->map, &pll->en) ||
>>              !meson_parm_read(clk->map, &pll->l))
>>                  return 0;
> I had to read this part twice to understand what it's doing because I
> misunderstood what "ret" is used for (I thought that some "return ret"
> is missing)
> my proposal to make it easier to read:
> ...
> 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;
> ...
> 
> please let me know what you think about this
I was intended to use 'ret' to store the return value of pll->rst.

If pll->rst exists, it will get it. Otherwise, the ret will be zero.

Your proposal is a good way for it. I will use it.
> 
>> @@ -321,6 +328,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>          /* do nothing if the PLL is already enabled */
>>          if (clk_hw_is_enabled(hw))
>>                  return 0;
>> +       /*
>> +        * Compared with the previous SoCs, self-adaption module current
>> +        * is newly added for A1, keep the new power-on sequence to enable the
>> +        * PLL.
>> +        */
>> +       if (MESON_PARM_APPLICABLE(&pll->current_en)) {
>> +               /* Enable the pll */
>> +               meson_parm_write(clk->map, &pll->en, 1);
>> +               udelay(10);
>> +               /* Enable the pll self-adaption module current */
>> +               meson_parm_write(clk->map, &pll->current_en, 1);
>> +               udelay(40);
>> +               /* Enable lock detect module */
>> +               meson_parm_write(clk->map, &pll->l_detect, 1);
>> +               meson_parm_write(clk->map, &pll->l_detect, 0);
>> +               goto out;
>> +       }
> in all other functions you are skipping the pll->rst register by
> checking for MESON_PARM_APPLICABLE(&pll->rst)
> I like that because it's a pattern which is easy to follow
> 
> do you think we can make this part consistent with that?
> I'm thinking of something like this (not compile-tested and I dropped
> all comments, just so you get the idea):
It is a good idea. I will test it.
> ...
> if (MESON_PARM_APPLICABLE(&pll->rst)
>    meson_parm_write(clk->map, &pll->rst, 1);
> 
> meson_parm_write(clk->map, &pll->en, 1);
> 
> if (MESON_PARM_APPLICABLE(&pll->rst))
>    meson_parm_write(clk->map, &pll->rst, 0);
> 
> if (MESON_PARM_APPLICABLE(&pll->current_en))
>    meson_parm_write(clk->map, &pll->current_en, 1);
> 
> 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))
> ...
> 
> I see two (and a half) benefits here:
> - if there's a PLL with neither the pll->current_en nor the pll->rst
> registers then you get support for this implementation for free
> - the if (MESON_PARM_APPLICABLE(...)) pattern is already used in the
> driver, but only for one register (in your example when
> MESON_PARM_APPLICABLE(&pll->current_en) exists you also modify the
> pll->l_detect register, which I did not expect)
> - only counts half: no use of "goto", which in my opinion makes it
> very easy to read (just read from top to bottom, checking each "if")
> 
I see, I will verify it.
> 
> Martin
> 
> .
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings
  2019-12-27  9:46 ` [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
  2020-01-03 22:29   ` Rob Herring
@ 2020-01-10 15:36   ` Jerome Brunet
  2020-01-16  5:50     ` Jian Hu
  1 sibling, 1 reply; 8+ messages in thread
From: Jerome Brunet @ 2020-01-10 15:36 UTC (permalink / raw)
  To: Jian Hu, Neil Armstrong
  Cc: Kevin Hilman, Rob Herring, Martin Blumenstingl,
	Michael Turquette, Stephen Boyd, Qiufang Dai, Jianxin Pan,
	Victor Wan, Chandle Zou, linux-clk, linux-amlogic,
	linux-arm-kernel, linux-kernel, devicetree


On Fri 27 Dec 2019 at 10:46, Jian Hu <jian.hu@amlogic.com> wrote:

Please read Documentation/devicetree/writing-schema.rst, run the test and
make the necessary correction.

> 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   | 54 +++++++++++++++++++
>  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>  2 files changed, 70 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..7a327bb174b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/clock/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:
> +  maxItems: 2
> +  items:
> +   - description: Input xtal_fixpll
> +   - description: Input xtal_hifipll
> +
> +clock-names:
> +  maxItems: 2
> +  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 CLKID_XTAL_FIXPLL>,
> +                         <&clkc_periphs CLKID_XTAL_HIFIPLL>;
> +                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 */


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings
  2020-01-10 15:36   ` Jerome Brunet
@ 2020-01-16  5:50     ` Jian Hu
  0 siblings, 0 replies; 8+ messages in thread
From: Jian Hu @ 2020-01-16  5:50 UTC (permalink / raw)
  To: Jerome Brunet, Neil Armstrong
  Cc: Kevin Hilman, Rob Herring, Martin Blumenstingl,
	Michael Turquette, Stephen Boyd, Qiufang Dai, Jianxin Pan,
	Victor Wan, Chandle Zou, linux-clk, linux-amlogic,
	linux-arm-kernel, linux-kernel, devicetree



On 2020/1/10 23:36, Jerome Brunet wrote:
> 
> On Fri 27 Dec 2019 at 10:46, Jian Hu <jian.hu@amlogic.com> wrote:
> 
> Please read Documentation/devicetree/writing-schema.rst, run the test and
> make the necessary correction.
> 
Yes, I had run the test before sending the V5.
>> 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   | 54 +++++++++++++++++++
>>   include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>>   2 files changed, 70 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..7a327bb174b8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> @@ -0,0 +1,54 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/clock/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:
>> +  maxItems: 2
>> +  items:
>> +   - description: Input xtal_fixpll
>> +   - description: Input xtal_hifipll
>> +
>> +clock-names:
>> +  maxItems: 2
>> +  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 CLKID_XTAL_FIXPLL>,
>> +                         <&clkc_periphs CLKID_XTAL_HIFIPLL>;
>> +                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 */
> 
> .
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-01-16  5:50 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-27  9:46 [PATCH v5 0/5] add Amlogic A1 clock controller driver Jian Hu
2019-12-27  9:46 ` [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
2020-01-03 22:29   ` Rob Herring
2020-01-10 15:36   ` Jerome Brunet
2020-01-16  5:50     ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops Jian Hu
2019-12-27 16:53   ` Martin Blumenstingl
2020-01-09  6:55     ` Jian Hu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).