* [PATCH v4 0/6] add Amlogic A1 clock controller driver
@ 2019-12-06 7:40 Jian Hu
2019-12-06 7:40 ` [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
2019-12-06 7:40 ` [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops Jian Hu
0 siblings, 2 replies; 13+ messages in thread
From: Jian Hu @ 2019-12-06 7:40 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 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
Jian Hu (6):
dt-bindings: clock: meson: add A1 PLL clock controller bindings
clk: meson: add support for A1 PLL clock ops
clk: meson: eeclk: refactor eeclk common driver to support A1
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 | 70 +
.../bindings/clock/amlogic,a1-pll-clkc.yaml | 59 +
drivers/clk/meson/Kconfig | 20 +
drivers/clk/meson/Makefile | 2 +
drivers/clk/meson/a1-pll.c | 334 +++
drivers/clk/meson/a1-pll.h | 56 +
drivers/clk/meson/a1.c | 2246 +++++++++++++++++
drivers/clk/meson/a1.h | 120 +
drivers/clk/meson/clk-pll.c | 21 +
drivers/clk/meson/clk-pll.h | 1 +
drivers/clk/meson/meson-eeclk.c | 59 +-
drivers/clk/meson/meson-eeclk.h | 1 +
drivers/clk/meson/parm.h | 1 +
include/dt-bindings/clock/a1-clkc.h | 98 +
include/dt-bindings/clock/a1-pll-clkc.h | 16 +
15 files changed, 3094 insertions(+), 10 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] 13+ messages in thread
* [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings
2019-12-06 7:40 [PATCH v4 0/6] add Amlogic A1 clock controller driver Jian Hu
@ 2019-12-06 7:40 ` Jian Hu
2019-12-12 9:57 ` Jerome Brunet
2019-12-13 10:38 ` Maxime Ripard
2019-12-06 7:40 ` [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops Jian Hu
1 sibling, 2 replies; 13+ messages in thread
From: Jian Hu @ 2019-12-06 7:40 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 | 59 +++++++++++++++++++
include/dt-bindings/clock/a1-pll-clkc.h | 16 +++++
2 files changed, 75 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..7feeef5abf1b
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ */
+%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:
+ - enum:
+ - amlogic,a1-pll-clkc
+ "#clock-cells":
+ const: 1
+
+ reg:
+ maxItems: 1
+
+clocks:
+ minItems: 2
+ maxItems: 2
+ items:
+ - description: Input xtal_fixpll
+ - description: Input xtal_hifipll
+
+clock-names:
+ minItems: 2
+ 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] 13+ messages in thread
* [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops
2019-12-06 7:40 [PATCH v4 0/6] add Amlogic A1 clock controller driver Jian Hu
2019-12-06 7:40 ` [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
@ 2019-12-06 7:40 ` Jian Hu
2019-12-12 10:16 ` Jerome Brunet
1 sibling, 1 reply; 13+ messages in thread
From: Jian Hu @ 2019-12-06 7:40 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
The A1 PLL design is different with previous SoCs. The PLL
internal analog modules Power-on sequence is different
with previous, and thus requires a strict register sequence to
enable the PLL.
Signed-off-by: Jian Hu <jian.hu@amlogic.com>
---
drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
drivers/clk/meson/clk-pll.h | 1 +
drivers/clk/meson/parm.h | 1 +
3 files changed, 23 insertions(+)
diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index ddb1e5634739..4aff31a51589 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -318,6 +318,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
struct clk_regmap *clk = to_clk_regmap(hw);
struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
+ /*
+ * The A1 design is different with previous SoCs.The PLL
+ * internal analog modules Power-on sequence is different with
+ * previous, and thus requires a strict register 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);
+ meson_parm_write(clk->map, &pll->rst, 1);
+ meson_parm_write(clk->map, &pll->rst, 0);
+ }
+
/* do nothing if the PLL is already enabled */
if (clk_hw_is_enabled(hw))
return 0;
@@ -347,6 +364,10 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
/* 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..30f039242a65 100644
--- a/drivers/clk/meson/clk-pll.h
+++ b/drivers/clk/meson/clk-pll.h
@@ -36,6 +36,7 @@ struct meson_clk_pll_data {
struct parm frac;
struct parm l;
struct parm rst;
+ struct parm current_en;
const struct reg_sequence *init_regs;
unsigned int init_count;
const struct pll_params_table *table;
diff --git a/drivers/clk/meson/parm.h b/drivers/clk/meson/parm.h
index 3c9ef1b505ce..c53fb26577e3 100644
--- a/drivers/clk/meson/parm.h
+++ b/drivers/clk/meson/parm.h
@@ -20,6 +20,7 @@
(((reg) & CLRPMASK(width, shift)) | ((val) << (shift)))
#define MESON_PARM_APPLICABLE(p) (!!((p)->width))
+#define MESON_PARM_CURRENT(p) (!!((p)->width))
struct parm {
u16 reg_off;
--
2.24.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings
2019-12-06 7:40 ` [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
@ 2019-12-12 9:57 ` Jerome Brunet
2019-12-13 9:35 ` Jian Hu
2019-12-13 10:38 ` Maxime Ripard
1 sibling, 1 reply; 13+ messages in thread
From: Jerome Brunet @ 2019-12-12 9:57 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 06 Dec 2019 at 08:40, Jian Hu <jian.hu@amlogic.com> 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 | 59 +++++++++++++++++++
> include/dt-bindings/clock/a1-pll-clkc.h | 16 +++++
> 2 files changed, 75 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..7feeef5abf1b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
Rob commented on the above in v1 and it remains unaddressed
> +/*
> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> + */
> +%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:
> + - enum:
> + - amlogic,a1-pll-clkc
> + "#clock-cells":
> + const: 1
> +
> + reg:
> + maxItems: 1
> +
> +clocks:
> + minItems: 2
> + maxItems: 2
> + items:
> + - description: Input xtal_fixpll
> + - description: Input xtal_hifipll
> +
> +clock-names:
> + minItems: 2
> + 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] 13+ messages in thread
* Re: [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops
2019-12-06 7:40 ` [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops Jian Hu
@ 2019-12-12 10:16 ` Jerome Brunet
2019-12-17 8:41 ` Jian Hu
0 siblings, 1 reply; 13+ messages in thread
From: Jerome Brunet @ 2019-12-12 10:16 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 06 Dec 2019 at 08:40, Jian Hu <jian.hu@amlogic.com> wrote:
> The A1 PLL design is different with previous SoCs. The PLL
> internal analog modules Power-on sequence is different
> with previous, and thus requires a strict register sequence to
> enable the PLL.
>
> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
> ---
> drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
> drivers/clk/meson/clk-pll.h | 1 +
> drivers/clk/meson/parm.h | 1 +
> 3 files changed, 23 insertions(+)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index ddb1e5634739..4aff31a51589 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -318,6 +318,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
> struct clk_regmap *clk = to_clk_regmap(hw);
> struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>
> + /*
> + * The A1 design is different with previous SoCs.The PLL
> + * internal analog modules Power-on sequence is different with
> + * previous, and thus requires a strict register sequence to
> + * enable the PLL.
The code does something more, not completly different. This comment is
not aligned with what the code does
> + */
> + 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);
> + meson_parm_write(clk->map, &pll->rst, 1);
> + meson_parm_write(clk->map, &pll->rst, 0);
Here you enable the PLL and self adaptation module then reset the PLL.
However:
#1 when you enter this function, the PLL should already by in reset
and disabled
#2 the code after that will reset the PLL again
So if what you submited works, inserting the following should accomplish
the same thing:
---8<---
diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 489092dde3a6..9b38df0a7682 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -330,6 +330,13 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
/* Enable the pll */
meson_parm_write(clk->map, &pll->en, 1);
+ if (MESON_PARM_APPLICABLE(&pll->current_en)) {
+ udelay(10);
+ /* Enable the pll self-adaption module current */
+ meson_parm_write(clk->map, &pll->current_en, 1);
+ udelay(40);
+ }
+
/* Take the pll out reset */
meson_parm_write(clk->map, &pll->rst, 0);
--->8---
> + }
> +
> /* do nothing if the PLL is already enabled */
> if (clk_hw_is_enabled(hw))
> return 0;
In any case, nothing should be done on the clock before this check
otherwise you might just break the clock
> @@ -347,6 +364,10 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
>
> /* 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..30f039242a65 100644
> --- a/drivers/clk/meson/clk-pll.h
> +++ b/drivers/clk/meson/clk-pll.h
> @@ -36,6 +36,7 @@ struct meson_clk_pll_data {
> struct parm frac;
> struct parm l;
> struct parm rst;
> + struct parm current_en;
> const struct reg_sequence *init_regs;
> unsigned int init_count;
> const struct pll_params_table *table;
> diff --git a/drivers/clk/meson/parm.h b/drivers/clk/meson/parm.h
> index 3c9ef1b505ce..c53fb26577e3 100644
> --- a/drivers/clk/meson/parm.h
> +++ b/drivers/clk/meson/parm.h
> @@ -20,6 +20,7 @@
> (((reg) & CLRPMASK(width, shift)) | ((val) << (shift)))
>
> #define MESON_PARM_APPLICABLE(p) (!!((p)->width))
> +#define MESON_PARM_CURRENT(p) (!!((p)->width))
Why do we need that ?
>
> struct parm {
> u16 reg_off;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings
2019-12-12 9:57 ` Jerome Brunet
@ 2019-12-13 9:35 ` Jian Hu
0 siblings, 0 replies; 13+ messages in thread
From: Jian Hu @ 2019-12-13 9:35 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 2019/12/12 17:57, Jerome Brunet wrote:
>
> On Fri 06 Dec 2019 at 08:40, Jian Hu <jian.hu@amlogic.com> 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 | 59 +++++++++++++++++++
>> include/dt-bindings/clock/a1-pll-clkc.h | 16 +++++
>> 2 files changed, 75 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..7feeef5abf1b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> @@ -0,0 +1,59 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>
> Rob commented on the above in v1 and it remains unaddressedOK, I will fix it in the next version.
>
>> +/*
>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>> + */
>> +%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:
>> + - enum:
>> + - amlogic,a1-pll-clkc
>> + "#clock-cells":
>> + const: 1
>> +
>> + reg:
>> + maxItems: 1
>> +
>> +clocks:
>> + minItems: 2
>> + maxItems: 2
>> + items:
>> + - description: Input xtal_fixpll
>> + - description: Input xtal_hifipll
>> +
>> +clock-names:
>> + minItems: 2
>> + 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] 13+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings
2019-12-06 7:40 ` [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
2019-12-12 9:57 ` Jerome Brunet
@ 2019-12-13 10:38 ` Maxime Ripard
2019-12-18 8:00 ` Jian Hu
1 sibling, 1 reply; 13+ messages in thread
From: Maxime Ripard @ 2019-12-13 10:38 UTC (permalink / raw)
To: Jian Hu
Cc: Jerome Brunet, Neil Armstrong, 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
[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]
Hi,
On Fri, Dec 06, 2019 at 03:40:47PM +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 | 59 +++++++++++++++++++
> include/dt-bindings/clock/a1-pll-clkc.h | 16 +++++
> 2 files changed, 75 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..7feeef5abf1b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> + */
> +%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:
> + - enum:
> + - amlogic,a1-pll-clkc
I'm not sure this works, compatible shouldn't contain a list.
You can write this like:
compatible:
const: amlogic,a1-pll-clkc
> + "#clock-cells":
> + const: 1
> +
> + reg:
> + maxItems: 1
> +
> +clocks:
> + minItems: 2
> + maxItems: 2
This is redundant, it will be added automatically by the tools ...
> + items:
> + - description: Input xtal_fixpll
> + - description: Input xtal_hifipll
... When you have a list of items :)
> +
> +clock-names:
> + minItems: 2
> + maxItems: 2
> + items:
> + - const: xtal_fixpll
> + - const: xtal_hifipll
Same story here
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops
2019-12-12 10:16 ` Jerome Brunet
@ 2019-12-17 8:41 ` Jian Hu
2019-12-17 9:29 ` Jerome Brunet
0 siblings, 1 reply; 13+ messages in thread
From: Jian Hu @ 2019-12-17 8:41 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 2019/12/12 18:16, Jerome Brunet wrote:
>
> On Fri 06 Dec 2019 at 08:40, Jian Hu <jian.hu@amlogic.com> wrote:
>
>> The A1 PLL design is different with previous SoCs. The PLL
>> internal analog modules Power-on sequence is different
>> with previous, and thus requires a strict register sequence to
>> enable the PLL.
>>
>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>> ---
>> drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
>> drivers/clk/meson/clk-pll.h | 1 +
>> drivers/clk/meson/parm.h | 1 +
>> 3 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index ddb1e5634739..4aff31a51589 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -318,6 +318,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>> struct clk_regmap *clk = to_clk_regmap(hw);
>> struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>
>> + /*
>> + * The A1 design is different with previous SoCs.The PLL
>> + * internal analog modules Power-on sequence is different with
>> + * previous, and thus requires a strict register sequence to
>> + * enable the PLL.
>
> The code does something more, not completly different. This comment is
> not aligned with what the code does
ok, I will correct the comment.
>
>> + */
>> + 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);
>> + meson_parm_write(clk->map, &pll->rst, 1);
>> + meson_parm_write(clk->map, &pll->rst, 0);
>
> Here you enable the PLL and self adaptation module then reset the PLL.
> However:
> #1 when you enter this function, the PLL should already by in reset
> and disabled
> #2 the code after that will reset the PLL again
For A1 PLLs, There is no reset bit, It will not reset the PLL.
And in V2, you mentioned PARM 'rst' can be used for one toggling, And
'rst' is used for BIT(6) in CTRL2.
Quote V2 the HIFI PLL init_regs definition:
+static const struct reg_sequence a1_hifi_init_regs[] = {
+ { .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
+ { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
+ { .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
+ { .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
+ { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
+ { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x11f18440, .delay_us = 10 },
+ { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x15f18440, .delay_us = 40 },
+ { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001140 },
+ { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
+};
So maybe another new PARM should be defined to avoid the ambiguity.
What do you think about it?
>
> So if what you submited works, inserting the following should accomplish
> the same thing:
>
> ---8<---
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index 489092dde3a6..9b38df0a7682 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -330,6 +330,13 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
> /* Enable the pll */
> meson_parm_write(clk->map, &pll->en, 1);
>
> + if (MESON_PARM_APPLICABLE(&pll->current_en)) {
> + udelay(10);
> + /* Enable the pll self-adaption module current */
> + meson_parm_write(clk->map, &pll->current_en, 1);
> + udelay(40);
> + }
> +
> /* Take the pll out reset */
> meson_parm_write(clk->map, &pll->rst, 0);
> --->8---
>
>
>
>
>> + }
>> +
>> /* do nothing if the PLL is already enabled */
>> if (clk_hw_is_enabled(hw))
>> return 0;
>
> In any case, nothing should be done on the clock before this check
> otherwise you might just break the clock
>
OK, I will put the enabled check ahead.
>> @@ -347,6 +364,10 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
>>
>> /* 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..30f039242a65 100644
>> --- a/drivers/clk/meson/clk-pll.h
>> +++ b/drivers/clk/meson/clk-pll.h
>> @@ -36,6 +36,7 @@ struct meson_clk_pll_data {
>> struct parm frac;
>> struct parm l;
>> struct parm rst;
>> + struct parm current_en;
>> const struct reg_sequence *init_regs;
>> unsigned int init_count;
>> const struct pll_params_table *table;
>> diff --git a/drivers/clk/meson/parm.h b/drivers/clk/meson/parm.h
>> index 3c9ef1b505ce..c53fb26577e3 100644
>> --- a/drivers/clk/meson/parm.h
>> +++ b/drivers/clk/meson/parm.h
>> @@ -20,6 +20,7 @@
>> (((reg) & CLRPMASK(width, shift)) | ((val) << (shift)))
>>
>> #define MESON_PARM_APPLICABLE(p) (!!((p)->width))
>> +#define MESON_PARM_CURRENT(p) (!!((p)->width))
>
> Why do we need that ?
OK, I will remove it ,and use 'MESON_PARM_APPLICABLE' instead
>
>>
>> struct parm {
>> u16 reg_off;
>
> .
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops
2019-12-17 8:41 ` Jian Hu
@ 2019-12-17 9:29 ` Jerome Brunet
2019-12-18 8:37 ` Jian Hu
0 siblings, 1 reply; 13+ messages in thread
From: Jerome Brunet @ 2019-12-17 9:29 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 Tue 17 Dec 2019 at 09:41, Jian Hu <jian.hu@amlogic.com> wrote:
> On 2019/12/12 18:16, Jerome Brunet wrote:
>>
>> On Fri 06 Dec 2019 at 08:40, Jian Hu <jian.hu@amlogic.com> wrote:
>>
>>> The A1 PLL design is different with previous SoCs. The PLL
>>> internal analog modules Power-on sequence is different
>>> with previous, and thus requires a strict register sequence to
>>> enable the PLL.
>>>
>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>>> ---
>>> drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
>>> drivers/clk/meson/clk-pll.h | 1 +
>>> drivers/clk/meson/parm.h | 1 +
>>> 3 files changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>> index ddb1e5634739..4aff31a51589 100644
>>> --- a/drivers/clk/meson/clk-pll.c
>>> +++ b/drivers/clk/meson/clk-pll.c
>>> @@ -318,6 +318,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>> struct clk_regmap *clk = to_clk_regmap(hw);
>>> struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>> + /*
>>> + * The A1 design is different with previous SoCs.The PLL
>>> + * internal analog modules Power-on sequence is different with
>>> + * previous, and thus requires a strict register sequence to
>>> + * enable the PLL.
>>
>> The code does something more, not completly different. This comment is
>> not aligned with what the code does
> ok, I will correct the comment.
>>
>>> + */
>>> + 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);
>>> + meson_parm_write(clk->map, &pll->rst, 1);
>>> + meson_parm_write(clk->map, &pll->rst, 0);
>>
>> Here you enable the PLL and self adaptation module then reset the PLL.
>> However:
>> #1 when you enter this function, the PLL should already by in reset
>> and disabled
>> #2 the code after that will reset the PLL again
> For A1 PLLs, There is no reset bit, It will not reset the PLL.
> And in V2, you mentioned PARM 'rst' can be used for one toggling, And 'rst'
> is used for BIT(6) in CTRL2.
>
oh my ! What is it then ? Why do you need to toggle it ? What does is do ?
> Quote V2 the HIFI PLL init_regs definition:
>
>
> +static const struct reg_sequence a1_hifi_init_regs[] = {
> + { .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
> + { .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
> + { .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x11f18440, .delay_us = 10 },
> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x15f18440, .delay_us = 40 },
> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001140 },
> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
> +};
>
> So maybe another new PARM should be defined to avoid the ambiguity.
> What do you think about it?
This is not the point of my comment Jian !
I'm assuming here that you have tested your v4 before sending and that
it work (hopefully)
The fact is that with this code, when disabled the bit behind rst
(whatever it is) is set. So when you get to enable the bit is already set.
The code you sent does the same as the snip I gave you in the reply.
Now, if your PLL is THAT different, maybe it would be best if you could
clearly explain how it works, what bit should be set and why. Then we
will be able to figure out how the driver has to be restructured.
>
>>
>> So if what you submited works, inserting the following should accomplish
>> the same thing:
>>
>> ---8<---
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index 489092dde3a6..9b38df0a7682 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -330,6 +330,13 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>> /* Enable the pll */
>> meson_parm_write(clk->map, &pll->en, 1);
>>
>> + if (MESON_PARM_APPLICABLE(&pll->current_en)) {
>> + udelay(10);
>> + /* Enable the pll self-adaption module current */
>> + meson_parm_write(clk->map, &pll->current_en, 1);
>> + udelay(40);
>> + }
>> +
>> /* Take the pll out reset */
>> meson_parm_write(clk->map, &pll->rst, 0);
>> --->8---
>>
>>
>>
>>
>>> + }
>>> +
>>> /* do nothing if the PLL is already enabled */
>>> if (clk_hw_is_enabled(hw))
>>> return 0;
>>
>> In any case, nothing should be done on the clock before this check
>> otherwise you might just break the clock
>>
> OK, I will put the enabled check ahead.
>>> @@ -347,6 +364,10 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
>>> /* 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..30f039242a65 100644
>>> --- a/drivers/clk/meson/clk-pll.h
>>> +++ b/drivers/clk/meson/clk-pll.h
>>> @@ -36,6 +36,7 @@ struct meson_clk_pll_data {
>>> struct parm frac;
>>> struct parm l;
>>> struct parm rst;
>>> + struct parm current_en;
>>> const struct reg_sequence *init_regs;
>>> unsigned int init_count;
>>> const struct pll_params_table *table;
>>> diff --git a/drivers/clk/meson/parm.h b/drivers/clk/meson/parm.h
>>> index 3c9ef1b505ce..c53fb26577e3 100644
>>> --- a/drivers/clk/meson/parm.h
>>> +++ b/drivers/clk/meson/parm.h
>>> @@ -20,6 +20,7 @@
>>> (((reg) & CLRPMASK(width, shift)) | ((val) << (shift)))
>>> #define MESON_PARM_APPLICABLE(p) (!!((p)->width))
>>> +#define MESON_PARM_CURRENT(p) (!!((p)->width))
>>
>> Why do we need that ?
> OK, I will remove it ,and use 'MESON_PARM_APPLICABLE' instead
>>
>>> struct parm {
>>> u16 reg_off;
>>
>> .
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings
2019-12-13 10:38 ` Maxime Ripard
@ 2019-12-18 8:00 ` Jian Hu
2019-12-18 12:57 ` Maxime Ripard
0 siblings, 1 reply; 13+ messages in thread
From: Jian Hu @ 2019-12-18 8:00 UTC (permalink / raw)
To: Maxime Ripard
Cc: Jerome Brunet, Neil Armstrong, 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
Hi Maxime
Thanks for your review
On 2019/12/13 18:38, Maxime Ripard wrote:
> Hi,
>
> On Fri, Dec 06, 2019 at 03:40:47PM +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 | 59 +++++++++++++++++++
>> include/dt-bindings/clock/a1-pll-clkc.h | 16 +++++
>> 2 files changed, 75 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..7feeef5abf1b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> @@ -0,0 +1,59 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
>> + */
>> +%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:
>> + - enum:
>> + - amlogic,a1-pll-clkc
>
> I'm not sure this works, compatible shouldn't contain a list.
>
I refered to
Documentation/devicetree/bindings/clock/allwinner,sun4i-a10-ccu.yaml.
I have used 'dt-doc-validate' tools to check, it will report something
wrong below.
properties:compatible: [{'enum': ['amlogic,a1-pll-clkc']}] is not of
type 'object', 'boolean'
Refer to
https://github.com/robherring/dt-schema/blob/master/example-schema.yaml
I will change it like this:
properties:
compatible:
oneOf:
- enum:
- amlogic,a1-pll-clkc
And It has been passed by 'dt-doc-validate' tools.
Is it right?
> You can write this like:
> compatible:
> const: amlogic,a1-pll-clkc
>
>> + "#clock-cells":
>> + const: 1
>> +
>> + reg:
>> + maxItems: 1
>> +
>> +clocks:
>> + minItems: 2
>> + maxItems: 2
>
> This is redundant, it will be added automatically by the tools ...
If I remove the minItems, it will pass by dt-doc-validate.
Would please tell how to use dt-schema to generate automatically it?
>
>> + items:
>> + - description: Input xtal_fixpll
>> + - description: Input xtal_hifipll
>
> ... When you have a list of items :)
>
>> +
>> +clock-names:
>> + minItems: 2
>> + maxItems: 2
>> + items:
>> + - const: xtal_fixpll
>> + - const: xtal_hifipll
>
> Same story here
OK, I will change it when I find the right way.
>
> Maxime
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops
2019-12-17 9:29 ` Jerome Brunet
@ 2019-12-18 8:37 ` Jian Hu
2019-12-20 7:03 ` Jian Hu
0 siblings, 1 reply; 13+ messages in thread
From: Jian Hu @ 2019-12-18 8:37 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 2019/12/17 17:29, Jerome Brunet wrote:
>
> On Tue 17 Dec 2019 at 09:41, Jian Hu <jian.hu@amlogic.com> wrote:
>
>> On 2019/12/12 18:16, Jerome Brunet wrote:
>>>
>>> On Fri 06 Dec 2019 at 08:40, Jian Hu <jian.hu@amlogic.com> wrote:
>>>
>>>> The A1 PLL design is different with previous SoCs. The PLL
>>>> internal analog modules Power-on sequence is different
>>>> with previous, and thus requires a strict register sequence to
>>>> enable the PLL.
>>>>
>>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>>>> ---
>>>> drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
>>>> drivers/clk/meson/clk-pll.h | 1 +
>>>> drivers/clk/meson/parm.h | 1 +
>>>> 3 files changed, 23 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>>> index ddb1e5634739..4aff31a51589 100644
>>>> --- a/drivers/clk/meson/clk-pll.c
>>>> +++ b/drivers/clk/meson/clk-pll.c
>>>> @@ -318,6 +318,23 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>>> struct clk_regmap *clk = to_clk_regmap(hw);
>>>> struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>>> + /*
>>>> + * The A1 design is different with previous SoCs.The PLL
>>>> + * internal analog modules Power-on sequence is different with
>>>> + * previous, and thus requires a strict register sequence to
>>>> + * enable the PLL.
>>>
>>> The code does something more, not completly different. This comment is
>>> not aligned with what the code does
>> ok, I will correct the comment.
>>>
>>>> + */
>>>> + 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);
>>>> + meson_parm_write(clk->map, &pll->rst, 1);
>>>> + meson_parm_write(clk->map, &pll->rst, 0);
>>>
>>> Here you enable the PLL and self adaptation module then reset the PLL.
>>> However:
>>> #1 when you enter this function, the PLL should already by in reset
>>> and disabled
>>> #2 the code after that will reset the PLL again
>> For A1 PLLs, There is no reset bit, It will not reset the PLL.
>> And in V2, you mentioned PARM 'rst' can be used for one toggling, And 'rst'
>> is used for BIT(6) in CTRL2.
>>
>
> oh my ! What is it then ? Why do you need to toggle it ? What does is do ?
>
The PLL enable flow:
step1: enable the PLL
step2: enable the self adaptation module
step3: reset the lock detect module, let the lock detect module
work,And then the PLL will work.
Toggle the bit 6 in CTRL2 can reset the lock detect module.
>> Quote V2 the HIFI PLL init_regs definition:
>>
>>
>> +static const struct reg_sequence a1_hifi_init_regs[] = {
>> + { .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x11f18440, .delay_us = 10 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x15f18440, .delay_us = 40 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001140 },
>> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
>> +};
>>
>> So maybe another new PARM should be defined to avoid the ambiguity.
>> What do you think about it?
>
> This is not the point of my comment Jian !
>
> I'm assuming here that you have tested your v4 before sending and that
> it work (hopefully)
>
Yes, it works wells. I have tested the drivers before sending every
patchset version.
> The fact is that with this code, when disabled the bit behind rst
> (whatever it is) is set. So when you get to enable the bit is already set.
> The code you sent does the same as the snip I gave you in the reply.
>
> Now, if your PLL is THAT different, maybe it would be best if you could
> clearly explain how it works, what bit should be set and why. Then we
> will be able to figure out how the driver has to be restructured.
>
the same as 'The PLL enable flow' above
>>
>>>
>>> So if what you submited works, inserting the following should accomplish
>>> the same thing:
>>>
>>> ---8<---
>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>> index 489092dde3a6..9b38df0a7682 100644
>>> --- a/drivers/clk/meson/clk-pll.c
>>> +++ b/drivers/clk/meson/clk-pll.c
>>> @@ -330,6 +330,13 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>> /* Enable the pll */
>>> meson_parm_write(clk->map, &pll->en, 1);
>>>
>>> + if (MESON_PARM_APPLICABLE(&pll->current_en)) {
>>> + udelay(10);
>>> + /* Enable the pll self-adaption module current */
>>> + meson_parm_write(clk->map, &pll->current_en, 1);
>>> + udelay(40);
>>> + }
>>> +
>>> /* Take the pll out reset */
>>> meson_parm_write(clk->map, &pll->rst, 0);
>>> --->8---
>>>
>>>
>>>
>>>
>>>> + }
>>>> +
>>>> /* do nothing if the PLL is already enabled */
>>>> if (clk_hw_is_enabled(hw))
>>>> return 0;
>>>
>>> In any case, nothing should be done on the clock before this check
>>> otherwise you might just break the clock
>>>
>> OK, I will put the enabled check ahead.
>>>> @@ -347,6 +364,10 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
>>>> /* 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..30f039242a65 100644
>>>> --- a/drivers/clk/meson/clk-pll.h
>>>> +++ b/drivers/clk/meson/clk-pll.h
>>>> @@ -36,6 +36,7 @@ struct meson_clk_pll_data {
>>>> struct parm frac;
>>>> struct parm l;
>>>> struct parm rst;
>>>> + struct parm current_en;
>>>> const struct reg_sequence *init_regs;
>>>> unsigned int init_count;
>>>> const struct pll_params_table *table;
>>>> diff --git a/drivers/clk/meson/parm.h b/drivers/clk/meson/parm.h
>>>> index 3c9ef1b505ce..c53fb26577e3 100644
>>>> --- a/drivers/clk/meson/parm.h
>>>> +++ b/drivers/clk/meson/parm.h
>>>> @@ -20,6 +20,7 @@
>>>> (((reg) & CLRPMASK(width, shift)) | ((val) << (shift)))
>>>> #define MESON_PARM_APPLICABLE(p) (!!((p)->width))
>>>> +#define MESON_PARM_CURRENT(p) (!!((p)->width))
>>>
>>> Why do we need that ?
>> OK, I will remove it ,and use 'MESON_PARM_APPLICABLE' instead
>>>
>>>> struct parm {
>>>> u16 reg_off;
>>>
>>> .
>>>
>
> .
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings
2019-12-18 8:00 ` Jian Hu
@ 2019-12-18 12:57 ` Maxime Ripard
0 siblings, 0 replies; 13+ messages in thread
From: Maxime Ripard @ 2019-12-18 12:57 UTC (permalink / raw)
To: Jian Hu
Cc: Jerome Brunet, Neil Armstrong, 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
[-- Attachment #1: Type: text/plain, Size: 2998 bytes --]
On Wed, Dec 18, 2019 at 04:00:20PM +0800, Jian Hu wrote:
> Hi Maxime
>
> Thanks for your review
>
> On 2019/12/13 18:38, Maxime Ripard wrote:
> > Hi,
> >
> > On Fri, Dec 06, 2019 at 03:40:47PM +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 | 59 +++++++++++++++++++
> > > include/dt-bindings/clock/a1-pll-clkc.h | 16 +++++
> > > 2 files changed, 75 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..7feeef5abf1b
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > > @@ -0,0 +1,59 @@
> > > +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> > > +/*
> > > + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> > > + */
> > > +%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:
> > > + - enum:
> > > + - amlogic,a1-pll-clkc
> >
> > I'm not sure this works, compatible shouldn't contain a list.
> >
> I refered to
> Documentation/devicetree/bindings/clock/allwinner,sun4i-a10-ccu.yaml.
>
> I have used 'dt-doc-validate' tools to check, it will report something wrong
> below.
>
> properties:compatible: [{'enum': ['amlogic,a1-pll-clkc']}] is not of type
> 'object', 'boolean'
>
> Refer to
> https://github.com/robherring/dt-schema/blob/master/example-schema.yaml
>
> I will change it like this:
>
> properties:
> compatible:
> oneOf:
> - enum:
> - amlogic,a1-pll-clkc
>
> And It has been passed by 'dt-doc-validate' tools.
>
> Is it right?
You can simply do
properties:
compatible:
const: amlogic,a1-pll-clkc
> > You can write this like:
> > compatible:
> > const: amlogic,a1-pll-clkc
> >
> > > + "#clock-cells":
> > > + const: 1
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > +clocks:
> > > + minItems: 2
> > > + maxItems: 2
> >
> > This is redundant, it will be added automatically by the tools ...
> If I remove the minItems, it will pass by dt-doc-validate.
>
> Would please tell how to use dt-schema to generate automatically it?
You don't have to do anything, it's just done at the tools runtime.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops
2019-12-18 8:37 ` Jian Hu
@ 2019-12-20 7:03 ` Jian Hu
0 siblings, 0 replies; 13+ messages in thread
From: Jian Hu @ 2019-12-20 7:03 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
Hi jerome
On 2019/12/18 16:37, Jian Hu wrote:
>
>
> On 2019/12/17 17:29, Jerome Brunet wrote:
>>
>> On Tue 17 Dec 2019 at 09:41, Jian Hu <jian.hu@amlogic.com> wrote:
>>
>>> On 2019/12/12 18:16, Jerome Brunet wrote:
>>>>
>>>> On Fri 06 Dec 2019 at 08:40, Jian Hu <jian.hu@amlogic.com> wrote:
>>>>
>>>>> The A1 PLL design is different with previous SoCs. The PLL
>>>>> internal analog modules Power-on sequence is different
>>>>> with previous, and thus requires a strict register sequence to
>>>>> enable the PLL.
>>>>>
>>>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>>>>> ---
>>>>> drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
>>>>> drivers/clk/meson/clk-pll.h | 1 +
>>>>> drivers/clk/meson/parm.h | 1 +
>>>>> 3 files changed, 23 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>>>> index ddb1e5634739..4aff31a51589 100644
>>>>> --- a/drivers/clk/meson/clk-pll.c
>>>>> +++ b/drivers/clk/meson/clk-pll.c
>>>>> @@ -318,6 +318,23 @@ static int meson_clk_pll_enable(struct clk_hw
>>>>> *hw)
>>>>> struct clk_regmap *clk = to_clk_regmap(hw);
>>>>> struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>>>> + /*
>>>>> + * The A1 design is different with previous SoCs.The PLL
>>>>> + * internal analog modules Power-on sequence is different with
>>>>> + * previous, and thus requires a strict register sequence to
>>>>> + * enable the PLL.
>>>>
>>>> The code does something more, not completly different. This comment is
>>>> not aligned with what the code does
>>> ok, I will correct the comment.
>>>>
>>>>> + */
>>>>> + 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);
>>>>> + meson_parm_write(clk->map, &pll->rst, 1);
>>>>> + meson_parm_write(clk->map, &pll->rst, 0);
>>>>
>>>> Here you enable the PLL and self adaptation module then reset the PLL.
>>>> However:
>>>> #1 when you enter this function, the PLL should already by in reset
>>>> and disabled
>>>> #2 the code after that will reset the PLL again
>>> For A1 PLLs, There is no reset bit, It will not reset the PLL.
>>> And in V2, you mentioned PARM 'rst' can be used for one toggling, And
>>> 'rst'
>>> is used for BIT(6) in CTRL2.
>>>
>>
>> oh my ! What is it then ? Why do you need to toggle it ? What does is
>> do ?
>>
> The PLL enable flow:
> step1: enable the PLL
> step2: enable the self adaptation module
> step3: reset the lock detect module, let the lock detect module
> work,And then the PLL will work.
>
> Toggle the bit 6 in CTRL2 can reset the lock detect module.
#1 I intend to introduce a new PARM 'l_detect', and the
meson_clk_pll_enable is blow. Please help to review it.
static int meson_clk_pll_enable(struct clk_hw *hw)
{
struct clk_regmap *clk = to_clk_regmap(hw);
struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
/* 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, 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);
/* Enable the pll */
meson_parm_write(clk->map, &pll->en, 1);
/* Take the pll out reset */
meson_parm_write(clk->map, &pll->rst, 0);
out:
if (meson_clk_pll_wait_lock(hw))
return -EIO;
return 0;
}
#2 Add check for PARM 'rst' when
writing to it, the same with frac.
if (MESON_PARM_APPLICABLE(&pll->rst))
meson_parm_write(clk->map, &pll->rst, 1);
And I have tested it for it, The periphs and hifi pll works well.
Or any good idea about it?
>>> Quote V2 the HIFI PLL init_regs definition:
>>>
>>>
>>> +static const struct reg_sequence a1_hifi_init_regs[] = {
>>> + { .reg = ANACTRL_HIFIPLL_CTRL1, .def = 0x01800000 },
>>> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
>>> + { .reg = ANACTRL_HIFIPLL_CTRL3, .def = 0x100a1100 },
>>> + { .reg = ANACTRL_HIFIPLL_CTRL4, .def = 0x00302000 },
>>> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x01f18440 },
>>> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x11f18440, .delay_us =
>>> 10 },
>>> + { .reg = ANACTRL_HIFIPLL_CTRL0, .def = 0x15f18440, .delay_us =
>>> 40 },
>>> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001140 },
>>> + { .reg = ANACTRL_HIFIPLL_CTRL2, .def = 0x00001100 },
>>> +};
>>>
>>> So maybe another new PARM should be defined to avoid the ambiguity.
>>> What do you think about it?
>>
>> This is not the point of my comment Jian !
>>
>> I'm assuming here that you have tested your v4 before sending and that
>> it work (hopefully)
>>
> Yes, it works wells. I have tested the drivers before sending every
> patchset version.
>> The fact is that with this code, when disabled the bit behind rst
>> (whatever it is) is set. So when you get to enable the bit is already
>> set.
>> The code you sent does the same as the snip I gave you in the reply.
>>
>> Now, if your PLL is THAT different, maybe it would be best if you could
>> clearly explain how it works, what bit should be set and why. Then we
>> will be able to figure out how the driver has to be restructured.
>>
> the same as 'The PLL enable flow' above
>>>
>>>>
>>>> So if what you submited works, inserting the following should
>>>> accomplish
>>>> the same thing:
>>>>
>>>> ---8<---
>>>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>>>> index 489092dde3a6..9b38df0a7682 100644
>>>> --- a/drivers/clk/meson/clk-pll.c
>>>> +++ b/drivers/clk/meson/clk-pll.c
>>>> @@ -330,6 +330,13 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
>>>> /* Enable the pll */
>>>> meson_parm_write(clk->map, &pll->en, 1);
>>>>
>>>> + if (MESON_PARM_APPLICABLE(&pll->current_en)) {
>>>> + udelay(10);
>>>> + /* Enable the pll self-adaption module current */
>>>> + meson_parm_write(clk->map, &pll->current_en, 1);
>>>> + udelay(40);
>>>> + }
>>>> +
>>>> /* Take the pll out reset */
>>>> meson_parm_write(clk->map, &pll->rst, 0);
>>>> --->8---
>>>>
>>>>
>>>>
>>>>
>>>>> + }
>>>>> +
>>>>> /* do nothing if the PLL is already enabled */
>>>>> if (clk_hw_is_enabled(hw))
>>>>> return 0;
>>>>
>>>> In any case, nothing should be done on the clock before this check
>>>> otherwise you might just break the clock
>>>>
>>> OK, I will put the enabled check ahead.
>>>>> @@ -347,6 +364,10 @@ static void meson_clk_pll_disable(struct
>>>>> clk_hw *hw)
>>>>> /* 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..30f039242a65 100644
>>>>> --- a/drivers/clk/meson/clk-pll.h
>>>>> +++ b/drivers/clk/meson/clk-pll.h
>>>>> @@ -36,6 +36,7 @@ struct meson_clk_pll_data {
>>>>> struct parm frac;
>>>>> struct parm l;
>>>>> struct parm rst;
>>>>> + struct parm current_en;
>>>>> const struct reg_sequence *init_regs;
>>>>> unsigned int init_count;
>>>>> const struct pll_params_table *table;
>>>>> diff --git a/drivers/clk/meson/parm.h b/drivers/clk/meson/parm.h
>>>>> index 3c9ef1b505ce..c53fb26577e3 100644
>>>>> --- a/drivers/clk/meson/parm.h
>>>>> +++ b/drivers/clk/meson/parm.h
>>>>> @@ -20,6 +20,7 @@
>>>>> (((reg) & CLRPMASK(width, shift)) | ((val) << (shift)))
>>>>> #define MESON_PARM_APPLICABLE(p) (!!((p)->width))
>>>>> +#define MESON_PARM_CURRENT(p) (!!((p)->width))
>>>>
>>>> Why do we need that ?
>>> OK, I will remove it ,and use 'MESON_PARM_APPLICABLE' instead
>>>>
>>>>> struct parm {
>>>>> u16 reg_off;
>>>>
>>>> .
>>>>
>>
>> .
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-12-20 7:03 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06 7:40 [PATCH v4 0/6] add Amlogic A1 clock controller driver Jian Hu
2019-12-06 7:40 ` [PATCH v4 1/6] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
2019-12-12 9:57 ` Jerome Brunet
2019-12-13 9:35 ` Jian Hu
2019-12-13 10:38 ` Maxime Ripard
2019-12-18 8:00 ` Jian Hu
2019-12-18 12:57 ` Maxime Ripard
2019-12-06 7:40 ` [PATCH v4 2/6] clk: meson: add support for A1 PLL clock ops Jian Hu
2019-12-12 10:16 ` Jerome Brunet
2019-12-17 8:41 ` Jian Hu
2019-12-17 9:29 ` Jerome Brunet
2019-12-18 8:37 ` Jian Hu
2019-12-20 7:03 ` 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).