linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] clk: meson: g12a: Add CPU Clock support
@ 2019-03-01 10:21 Neil Armstrong
  2019-03-01 10:21 ` [PATCH 1/2] clk: meson-g12a: add cpu clock bindings Neil Armstrong
  2019-03-01 10:21 ` [PATCH 2/2] clk: meson: g12a: add cpu clocks Neil Armstrong
  0 siblings, 2 replies; 9+ messages in thread
From: Neil Armstrong @ 2019-03-01 10:21 UTC (permalink / raw)
  To: jbrunet
  Cc: Neil Armstrong, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel

This patchset adds support for the clock tree feeding the 4xCortex A53
cpu cluster.

This patchet does not handle clock changing, this will be added in a
secondary patchset.

The CPU clock can either use the SYS_PLL for > 1GHz frequencies or
use a couple of div+mux from 1GHz/667MHz/24MHz source with 2 non-glitch
muxes.

The CPU clock must be switched to a safe clock while changing the clocks
before the non-glitch muxes. Proper support will be added later.

In this patchset, clocks are set read-only.

Neil Armstrong (2):
  clk: meson-g12a: add cpu clock bindings
  clk: meson: g12a: add cpu clocks

 drivers/clk/meson/g12a.c              | 348 ++++++++++++++++++++++++++
 drivers/clk/meson/g12a.h              |  25 +-
 include/dt-bindings/clock/g12a-clkc.h |   1 +
 3 files changed, 373 insertions(+), 1 deletion(-)

-- 
2.20.1


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

* [PATCH 1/2] clk: meson-g12a: add cpu clock bindings
  2019-03-01 10:21 [PATCH 0/2] clk: meson: g12a: Add CPU Clock support Neil Armstrong
@ 2019-03-01 10:21 ` Neil Armstrong
  2019-03-01 15:26   ` Martin Blumenstingl
  2019-03-01 10:21 ` [PATCH 2/2] clk: meson: g12a: add cpu clocks Neil Armstrong
  1 sibling, 1 reply; 9+ messages in thread
From: Neil Armstrong @ 2019-03-01 10:21 UTC (permalink / raw)
  To: jbrunet, devicetree
  Cc: Neil Armstrong, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel

Add Amlogic G12A Family CPU clocks bindings, only export CPU_CLK since
it should be the only ID used.

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

diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h
index f399dfe1401c..4854750df902 100644
--- a/drivers/clk/meson/g12a.h
+++ b/drivers/clk/meson/g12a.h
@@ -166,8 +166,30 @@
 #define CLKID_MALI_0_DIV			170
 #define CLKID_MALI_1_DIV			173
 #define CLKID_MPLL_5OM_DIV			176
+#define CLKID_PCIE_PLL_DCO			178
+#define CLKID_PCIE_PLL_DCO_DIV2			179
+#define CLKID_PCIE_PLL_OD			180
+#define CLKID_SYS_PLL_DIV16_EN			181
+#define CLKID_SYS_PLL_DIV16			182
+#define CLKID_CPU_CLK_DYN0_SEL			183
+#define CLKID_CPU_CLK_DYN0_DIV			184
+#define CLKID_CPU_CLK_DYN0			185
+#define CLKID_CPU_CLK_DYN1_SEL			186
+#define CLKID_CPU_CLK_DYN1_DIV			187
+#define CLKID_CPU_CLK_DYN1			188
+#define CLKID_CPU_CLK_DYN			189
+#define CLKID_CPU_CLK_DIV16_EN			191
+#define CLKID_CPU_CLK_DIV16			192
+#define CLKID_CPU_CLK_APB_DIV			193
+#define CLKID_CPU_CLK_APB			194
+#define CLKID_CPU_CLK_ATB_DIV			195
+#define CLKID_CPU_CLK_ATB			196
+#define CLKID_CPU_CLK_AXI_DIV			197
+#define CLKID_CPU_CLK_AXI			198
+#define CLKID_CPU_CLK_TRACE_DIV			299
+#define CLKID_CPU_CLK_TRACE			200
 
-#define NR_CLKS					178
+#define NR_CLKS					201
 
 /* include the CLKIDs that have been made part of the DT binding */
 #include <dt-bindings/clock/g12a-clkc.h>
diff --git a/include/dt-bindings/clock/g12a-clkc.h b/include/dt-bindings/clock/g12a-clkc.h
index 83b657038d1e..33aba232282c 100644
--- a/include/dt-bindings/clock/g12a-clkc.h
+++ b/include/dt-bindings/clock/g12a-clkc.h
@@ -131,5 +131,6 @@
 #define CLKID_MALI_1				174
 #define CLKID_MALI				175
 #define CLKID_MPLL_5OM				177
+#define CLKID_CPU_CLK				190
 
 #endif /* __G12A_CLKC_H */
-- 
2.20.1


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

* [PATCH 2/2] clk: meson: g12a: add cpu clocks
  2019-03-01 10:21 [PATCH 0/2] clk: meson: g12a: Add CPU Clock support Neil Armstrong
  2019-03-01 10:21 ` [PATCH 1/2] clk: meson-g12a: add cpu clock bindings Neil Armstrong
@ 2019-03-01 10:21 ` Neil Armstrong
  2019-03-01 15:21   ` Martin Blumenstingl
  1 sibling, 1 reply; 9+ messages in thread
From: Neil Armstrong @ 2019-03-01 10:21 UTC (permalink / raw)
  To: jbrunet
  Cc: Neil Armstrong, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel

Add the Amlogic G12A Family CPU Clock tree in read/only for now.

The CPU clock can either use the SYS_PLL for > 1GHz frequencies or
use a couple of div+mux from 1GHz/667MHz/24MHz source with 2 non-glitch
muxes.

Proper DVFS support will come in a second time.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/meson/g12a.c | 348 +++++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/g12a.h |   1 +
 2 files changed, 349 insertions(+)

diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
index 0e1ce8c03259..4c938f1b8421 100644
--- a/drivers/clk/meson/g12a.c
+++ b/drivers/clk/meson/g12a.c
@@ -150,6 +150,316 @@ static struct clk_regmap g12a_sys_pll = {
 	},
 };
 
+static struct clk_regmap g12a_sys_pll_div16_en = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL1,
+		.bit_idx = 24,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "sys_pll_div16_en",
+		.ops = &clk_regmap_gate_ops,
+		.parent_names = (const char *[]){ "sys_pll" },
+		.num_parents = 1,
+		/*
+		 * This clock is used to debug the sys_pll range
+		 * Linux should not change it at runtime
+		 */
+		.flags = CLK_IGNORE_UNUSED,
+	},
+};
+
+static struct clk_fixed_factor g12a_sys_pll_div16 = {
+	.mult = 1,
+	.div = 16,
+	.hw.init = &(struct clk_init_data){
+		.name = "sys_pll_div16",
+		.ops = &clk_fixed_factor_ops,
+		.parent_names = (const char *[]){ "sys_pll_div16_en" },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_dyn0_sel = {
+	.data = &(struct clk_regmap_mux_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL0,
+		.mask = 0x3,
+		.shift = 0,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_dyn0_sel",
+		.ops = &clk_regmap_mux_ro_ops,
+		.parent_names = (const char *[]){ IN_PREFIX "xtal",
+						  "fclk_div2",
+						  "fclk_div3" },
+		.num_parents = 3,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_dyn0_div = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL0,
+		.shift = 4,
+		.width = 6,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_dyn0_div",
+		.ops = &clk_regmap_divider_ro_ops,
+		.parent_names = (const char *[]){ "cpu_clk_dyn0_sel" },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_dyn0 = {
+	.data = &(struct clk_regmap_mux_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL0,
+		.mask = 0x1,
+		.shift = 2,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_dyn0",
+		.ops = &clk_regmap_mux_ro_ops,
+		.parent_names = (const char *[]){ "cpu_clk_dyn0_sel",
+						  "cpu_clk_dyn0_div" },
+		.num_parents = 2,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_dyn1_sel = {
+	.data = &(struct clk_regmap_mux_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL0,
+		.mask = 0x3,
+		.shift = 16,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_dyn1_sel",
+		.ops = &clk_regmap_mux_ro_ops,
+		.parent_names = (const char *[]){ IN_PREFIX "xtal",
+						  "fclk_div2",
+						  "fclk_div3" },
+		.num_parents = 3,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_dyn1_div = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL0,
+		.shift = 20,
+		.width = 6,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_dyn1_div",
+		.ops = &clk_regmap_divider_ro_ops,
+		.parent_names = (const char *[]){ "cpu_clk_dyn1_sel" },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_dyn1 = {
+	.data = &(struct clk_regmap_mux_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL0,
+		.mask = 0x1,
+		.shift = 18,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_dyn1",
+		.ops = &clk_regmap_mux_ro_ops,
+		.parent_names = (const char *[]){ "cpu_clk_dyn1_sel",
+						  "cpu_clk_dyn1_div" },
+		.num_parents = 2,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_dyn = {
+	.data = &(struct clk_regmap_mux_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL0,
+		.mask = 0x1,
+		.shift = 10,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_dyn",
+		.ops = &clk_regmap_mux_ro_ops,
+		.parent_names = (const char *[]){ "cpu_clk_dyn0",
+						  "cpu_clk_dyn1" },
+		.num_parents = 2,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk = {
+	.data = &(struct clk_regmap_mux_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL0,
+		.mask = 0x1,
+		.shift = 11,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk",
+		.ops = &clk_regmap_mux_ro_ops,
+		.parent_names = (const char *[]){ "cpu_clk_dyn",
+						  "sys_pll" },
+		.num_parents = 2,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_div16_en = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL1,
+		.bit_idx = 1,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_clk_div16_en",
+		.ops = &clk_regmap_gate_ops,
+		.parent_names = (const char *[]){ "cpu_clk" },
+		.num_parents = 1,
+		/*
+		 * This clock is used to debug the cpu_clk range
+		 * Linux should not change it at runtime
+		 */
+		.flags = CLK_IGNORE_UNUSED,
+	},
+};
+
+static struct clk_fixed_factor g12a_cpu_clk_div16 = {
+	.mult = 1,
+	.div = 16,
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_div16",
+		.ops = &clk_fixed_factor_ops,
+		.parent_names = (const char *[]){ "cpu_clk_div16_en" },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_apb_div = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL1,
+		.shift = 3,
+		.width = 3,
+		.flags = CLK_DIVIDER_POWER_OF_TWO,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_apb_div",
+		.ops = &clk_regmap_divider_ro_ops,
+		.parent_names = (const char *[]){ "cpu_clk" },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_apb = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL1,
+		.bit_idx = 1,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_clk_apb",
+		.ops = &clk_regmap_gate_ops,
+		.parent_names = (const char *[]){ "cpu_clk_apb_div" },
+		.num_parents = 1,
+		/*
+		 * This clock is set by the ROM monitor code,
+		 * Linux should not change it at runtime
+		 */
+		.flags = CLK_IGNORE_UNUSED,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_atb_div = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL1,
+		.shift = 6,
+		.width = 3,
+		.flags = CLK_DIVIDER_POWER_OF_TWO,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_atb_div",
+		.ops = &clk_regmap_divider_ro_ops,
+		.parent_names = (const char *[]){ "cpu_clk" },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_atb = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL1,
+		.bit_idx = 17,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_clk_atb",
+		.ops = &clk_regmap_gate_ops,
+		.parent_names = (const char *[]){ "cpu_clk_atb_div" },
+		.num_parents = 1,
+		/*
+		 * This clock is set by the ROM monitor code,
+		 * Linux should not change it at runtime
+		 */
+		.flags = CLK_IGNORE_UNUSED,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_axi_div = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL1,
+		.shift = 9,
+		.width = 3,
+		.flags = CLK_DIVIDER_POWER_OF_TWO,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_axi_div",
+		.ops = &clk_regmap_divider_ro_ops,
+		.parent_names = (const char *[]){ "cpu_clk" },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_axi = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL1,
+		.bit_idx = 18,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_clk_axi",
+		.ops = &clk_regmap_gate_ops,
+		.parent_names = (const char *[]){ "cpu_clk_axi_div" },
+		.num_parents = 1,
+		/*
+		 * This clock is set by the ROM monitor code,
+		 * Linux should not change it at runtime
+		 */
+		.flags = CLK_IGNORE_UNUSED,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_trace_div = {
+	.data = &(struct clk_regmap_div_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL1,
+		.shift = 20,
+		.width = 3,
+		.flags = CLK_DIVIDER_POWER_OF_TWO,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "cpu_clk_trace_div",
+		.ops = &clk_regmap_divider_ro_ops,
+		.parent_names = (const char *[]){ "cpu_clk" },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap g12a_cpu_clk_trace = {
+	.data = &(struct clk_regmap_gate_data){
+		.offset = HHI_SYS_CPU_CLK_CNTL1,
+		.bit_idx = 23,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_clk_trace",
+		.ops = &clk_regmap_gate_ops,
+		.parent_names = (const char *[]){ "cpu_clk_trace_div" },
+		.num_parents = 1,
+		/*
+		 * This clock is set by the ROM monitor code,
+		 * Linux should not change it at runtime
+		 */
+		.flags = CLK_IGNORE_UNUSED,
+	},
+};
+
 static const struct pll_mult_range g12a_gp0_pll_mult_range = {
 	.min = 55,
 	.max = 255,
@@ -2167,6 +2477,26 @@ static struct clk_hw_onecell_data g12a_hw_onecell_data = {
 		[CLKID_MALI]			= &g12a_mali.hw,
 		[CLKID_MPLL_5OM_DIV]		= &g12a_mpll_50m_div.hw,
 		[CLKID_MPLL_5OM]		= &g12a_mpll_50m.hw,
+		[CLKID_SYS_PLL_DIV16_EN]	= &g12a_sys_pll_div16_en.hw,
+		[CLKID_SYS_PLL_DIV16]		= &g12a_sys_pll_div16.hw,
+		[CLKID_CPU_CLK_DYN0_SEL]	= &g12a_cpu_clk_dyn0_sel.hw,
+		[CLKID_CPU_CLK_DYN0_DIV]	= &g12a_cpu_clk_dyn0_div.hw,
+		[CLKID_CPU_CLK_DYN0]		= &g12a_cpu_clk_dyn0.hw,
+		[CLKID_CPU_CLK_DYN1_SEL]	= &g12a_cpu_clk_dyn1_sel.hw,
+		[CLKID_CPU_CLK_DYN1_DIV]	= &g12a_cpu_clk_dyn1_div.hw,
+		[CLKID_CPU_CLK_DYN1]		= &g12a_cpu_clk_dyn1.hw,
+		[CLKID_CPU_CLK_DYN]		= &g12a_cpu_clk_dyn.hw,
+		[CLKID_CPU_CLK]			= &g12a_cpu_clk.hw,
+		[CLKID_CPU_CLK_DIV16_EN]	= &g12a_cpu_clk_div16_en.hw,
+		[CLKID_CPU_CLK_DIV16]		= &g12a_cpu_clk_div16.hw,
+		[CLKID_CPU_CLK_APB_DIV]		= &g12a_cpu_clk_apb_div.hw,
+		[CLKID_CPU_CLK_APB]		= &g12a_cpu_clk_apb.hw,
+		[CLKID_CPU_CLK_ATB_DIV]		= &g12a_cpu_clk_atb_div.hw,
+		[CLKID_CPU_CLK_ATB]		= &g12a_cpu_clk_atb.hw,
+		[CLKID_CPU_CLK_AXI_DIV]		= &g12a_cpu_clk_axi_div.hw,
+		[CLKID_CPU_CLK_AXI]		= &g12a_cpu_clk_axi.hw,
+		[CLKID_CPU_CLK_TRACE_DIV]	= &g12a_cpu_clk_trace_div.hw,
+		[CLKID_CPU_CLK_TRACE]		= &g12a_cpu_clk_trace.hw,
 		[NR_CLKS]			= NULL,
 	},
 	.num = NR_CLKS,
@@ -2335,6 +2665,24 @@ static struct clk_regmap *const g12a_clk_regmaps[] = {
 	&g12a_mali_1,
 	&g12a_mali,
 	&g12a_mpll_50m,
+	&g12a_sys_pll_div16_en,
+	&g12a_cpu_clk_dyn0_sel,
+	&g12a_cpu_clk_dyn0_div,
+	&g12a_cpu_clk_dyn0,
+	&g12a_cpu_clk_dyn1_sel,
+	&g12a_cpu_clk_dyn1_div,
+	&g12a_cpu_clk_dyn1,
+	&g12a_cpu_clk_dyn,
+	&g12a_cpu_clk,
+	&g12a_cpu_clk_div16_en,
+	&g12a_cpu_clk_apb_div,
+	&g12a_cpu_clk_apb,
+	&g12a_cpu_clk_atb_div,
+	&g12a_cpu_clk_atb,
+	&g12a_cpu_clk_axi_div,
+	&g12a_cpu_clk_axi,
+	&g12a_cpu_clk_trace_div,
+	&g12a_cpu_clk_trace,
 };
 
 static const struct meson_eeclkc_data g12a_clkc_data = {
diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h
index 4854750df902..0ba3bfe4d46d 100644
--- a/drivers/clk/meson/g12a.h
+++ b/drivers/clk/meson/g12a.h
@@ -50,6 +50,7 @@
 #define HHI_GCLK_MPEG2			0x148
 #define HHI_GCLK_OTHER			0x150
 #define HHI_GCLK_OTHER2			0x154
+#define HHI_SYS_CPU_CLK_CNTL1		0x15c
 #define HHI_VID_CLK_DIV			0x164
 #define HHI_MPEG_CLK_CNTL		0x174
 #define HHI_AUD_CLK_CNTL		0x178
-- 
2.20.1


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

* Re: [PATCH 2/2] clk: meson: g12a: add cpu clocks
  2019-03-01 10:21 ` [PATCH 2/2] clk: meson: g12a: add cpu clocks Neil Armstrong
@ 2019-03-01 15:21   ` Martin Blumenstingl
  2019-03-01 16:41     ` Neil Armstrong
  0 siblings, 1 reply; 9+ messages in thread
From: Martin Blumenstingl @ 2019-03-01 15:21 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: jbrunet, linux-amlogic, linux-kernel, linux-clk, linux-arm-kernel

Hi Neil,

it's great to see the progress on G12A!

On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Add the Amlogic G12A Family CPU Clock tree in read/only for now.
>
> The CPU clock can either use the SYS_PLL for > 1GHz frequencies or
> use a couple of div+mux from 1GHz/667MHz/24MHz source with 2 non-glitch
> muxes.
>
> Proper DVFS support will come in a second time.
can you please also mention that this adds various CPU clock
post-dividers (APB, ATB, AXI and CPU trace)?
I don't mind them being int his patchset

disclaimer for my code-review:
- I don't have access to the datasheet so I can't verify if the clock
tree from this patch is correct
- the latest buildroot code with G12A support
(buildroot_openlinux_kernel_4.9_fbdev_20180706) doesn't have proper
names for all clocks
- based on my experience with Meson8* this looks good overall, some
questions and comments below

> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>
> ---
>  drivers/clk/meson/g12a.c | 348 +++++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/g12a.h |   1 +
>  2 files changed, 349 insertions(+)
>
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index 0e1ce8c03259..4c938f1b8421 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -150,6 +150,316 @@ static struct clk_regmap g12a_sys_pll = {
>         },
>  };
>
> +static struct clk_regmap g12a_sys_pll_div16_en = {
> +       .data = &(struct clk_regmap_gate_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> +               .bit_idx = 24,
> +       },
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "sys_pll_div16_en",
> +               .ops = &clk_regmap_gate_ops,
> +               .parent_names = (const char *[]){ "sys_pll" },
> +               .num_parents = 1,
> +               /*
> +                * This clock is used to debug the sys_pll range
> +                * Linux should not change it at runtime
> +                */
if we're not supposed to touch this the enable bit, can you switch to
clk_regmap_gate_ro_ops ?

> +               .flags = CLK_IGNORE_UNUSED,
> +       },
> +};
> +
> +static struct clk_fixed_factor g12a_sys_pll_div16 = {
> +       .mult = 1,
> +       .div = 16,
> +       .hw.init = &(struct clk_init_data){
> +               .name = "sys_pll_div16",
> +               .ops = &clk_fixed_factor_ops,
> +               .parent_names = (const char *[]){ "sys_pll_div16_en" },
> +               .num_parents = 1,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_dyn0_sel = {
> +       .data = &(struct clk_regmap_mux_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
> +               .mask = 0x3,
> +               .shift = 0,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_dyn0_sel",
the buildroot code has a variable with the name "p_premux"
I'm not sure what the datasheet states, but maybe this should be
cpu_clk_dyn0_pre_sel
same applies to the corresponding dyn1 clock below

> +               .ops = &clk_regmap_mux_ro_ops,
> +               .parent_names = (const char *[]){ IN_PREFIX "xtal",
> +                                                 "fclk_div2",
> +                                                 "fclk_div3" },
> +               .num_parents = 3,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_dyn0_div = {
> +       .data = &(struct clk_regmap_div_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
> +               .shift = 4,
> +               .width = 6,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_dyn0_div",
> +               .ops = &clk_regmap_divider_ro_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_dyn0_sel" },
> +               .num_parents = 1,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_dyn0 = {
> +       .data = &(struct clk_regmap_mux_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
> +               .mask = 0x1,
> +               .shift = 2,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_dyn0",
the buildroot code has a variable with the name "p_postmux". in this
case I would leave the name "cpu_clk_dyn0" because it's the "output"
of this specific "clock tree/branch".
same applies to the corresponding dyn1 clock below

> +               .ops = &clk_regmap_mux_ro_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_dyn0_sel",
> +                                                 "cpu_clk_dyn0_div" },
> +               .num_parents = 2,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_dyn1_sel = {
> +       .data = &(struct clk_regmap_mux_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
> +               .mask = 0x3,
> +               .shift = 16,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_dyn1_sel",
> +               .ops = &clk_regmap_mux_ro_ops,
> +               .parent_names = (const char *[]){ IN_PREFIX "xtal",
> +                                                 "fclk_div2",
> +                                                 "fclk_div3" },
> +               .num_parents = 3,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_dyn1_div = {
> +       .data = &(struct clk_regmap_div_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
> +               .shift = 20,
> +               .width = 6,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_dyn1_div",
> +               .ops = &clk_regmap_divider_ro_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_dyn1_sel" },
> +               .num_parents = 1,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_dyn1 = {
> +       .data = &(struct clk_regmap_mux_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
> +               .mask = 0x1,
> +               .shift = 18,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_dyn1",
> +               .ops = &clk_regmap_mux_ro_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_dyn1_sel",
> +                                                 "cpu_clk_dyn1_div" },
> +               .num_parents = 2,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_dyn = {
> +       .data = &(struct clk_regmap_mux_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
> +               .mask = 0x1,
> +               .shift = 10,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_dyn",
> +               .ops = &clk_regmap_mux_ro_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_dyn0",
> +                                                 "cpu_clk_dyn1" },
> +               .num_parents = 2,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk = {
> +       .data = &(struct clk_regmap_mux_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
> +               .mask = 0x1,
> +               .shift = 11,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk",
> +               .ops = &clk_regmap_mux_ro_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_dyn",
> +                                                 "sys_pll" },
> +               .num_parents = 2,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_div16_en = {
> +       .data = &(struct clk_regmap_gate_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> +               .bit_idx = 1,
> +       },
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "cpu_clk_div16_en",
> +               .ops = &clk_regmap_gate_ops,
> +               .parent_names = (const char *[]){ "cpu_clk" },
> +               .num_parents = 1,
> +               /*
> +                * This clock is used to debug the cpu_clk range
> +                * Linux should not change it at runtime
> +                */
same as above: if we're not supposed to touch this the enable bit, can
you switch to clk_regmap_gate_ro_ops ?

> +               .flags = CLK_IGNORE_UNUSED,
> +       },
> +};
> +
> +static struct clk_fixed_factor g12a_cpu_clk_div16 = {
> +       .mult = 1,
> +       .div = 16,
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_div16",
> +               .ops = &clk_fixed_factor_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_div16_en" },
> +               .num_parents = 1,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_apb_div = {
> +       .data = &(struct clk_regmap_div_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> +               .shift = 3,
> +               .width = 3,
> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_apb_div",
> +               .ops = &clk_regmap_divider_ro_ops,
> +               .parent_names = (const char *[]){ "cpu_clk" },
> +               .num_parents = 1,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_apb = {
> +       .data = &(struct clk_regmap_gate_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> +               .bit_idx = 1,
> +       },
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "cpu_clk_apb",
> +               .ops = &clk_regmap_gate_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_apb_div" },
> +               .num_parents = 1,
> +               /*
> +                * This clock is set by the ROM monitor code,
> +                * Linux should not change it at runtime
> +                */
same as above: if we're not supposed to touch this the enable bit, can
you switch to clk_regmap_gate_ro_ops ?

> +               .flags = CLK_IGNORE_UNUSED,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_atb_div = {
> +       .data = &(struct clk_regmap_div_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> +               .shift = 6,
> +               .width = 3,
> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_atb_div",
> +               .ops = &clk_regmap_divider_ro_ops,
> +               .parent_names = (const char *[]){ "cpu_clk" },
> +               .num_parents = 1,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_atb = {
> +       .data = &(struct clk_regmap_gate_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> +               .bit_idx = 17,
> +       },
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "cpu_clk_atb",
> +               .ops = &clk_regmap_gate_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_atb_div" },
> +               .num_parents = 1,
> +               /*
> +                * This clock is set by the ROM monitor code,
> +                * Linux should not change it at runtime
> +                */
same as above: if we're not supposed to touch this the enable bit, can
you switch to clk_regmap_gate_ro_ops ?

> +               .flags = CLK_IGNORE_UNUSED,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_axi_div = {
> +       .data = &(struct clk_regmap_div_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> +               .shift = 9,
> +               .width = 3,
> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_axi_div",
> +               .ops = &clk_regmap_divider_ro_ops,
out of curiosity (this applies to all CPU clock post-dividers):
did you check whether CLK_DIVIDER_POWER_OF_TWO is correct on G12A?
I'm asking because on Meson8b the post-dividers select between one of:
"cpu_clk divided by 2, 3, 4, 5, 6, 7 or 8". also some of the
post-dividers use register value 0 for cpu_clk_div2 while others use
register value 1 for cpu_clk_div2.

> +               .parent_names = (const char *[]){ "cpu_clk" },
> +               .num_parents = 1,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_axi = {
> +       .data = &(struct clk_regmap_gate_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> +               .bit_idx = 18,
> +       },
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "cpu_clk_axi",
> +               .ops = &clk_regmap_gate_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_axi_div" },
> +               .num_parents = 1,
> +               /*
> +                * This clock is set by the ROM monitor code,
> +                * Linux should not change it at runtime
> +                */
same as above: if we're not supposed to touch this the enable bit, can
you switch to clk_regmap_gate_ro_ops ?

> +               .flags = CLK_IGNORE_UNUSED,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_trace_div = {
> +       .data = &(struct clk_regmap_div_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> +               .shift = 20,
> +               .width = 3,
> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
> +       },
> +       .hw.init = &(struct clk_init_data){
> +               .name = "cpu_clk_trace_div",
> +               .ops = &clk_regmap_divider_ro_ops,
> +               .parent_names = (const char *[]){ "cpu_clk" },
> +               .num_parents = 1,
> +       },
> +};
> +
> +static struct clk_regmap g12a_cpu_clk_trace = {
> +       .data = &(struct clk_regmap_gate_data){
> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> +               .bit_idx = 23,
> +       },
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "cpu_clk_trace",
> +               .ops = &clk_regmap_gate_ops,
> +               .parent_names = (const char *[]){ "cpu_clk_trace_div" },
> +               .num_parents = 1,
> +               /*
> +                * This clock is set by the ROM monitor code,
> +                * Linux should not change it at runtime
> +                */
same as above: if we're not supposed to touch this the enable bit, can
you switch to clk_regmap_gate_ro_ops ?

> +               .flags = CLK_IGNORE_UNUSED,
> +       },
> +};
> +
>  static const struct pll_mult_range g12a_gp0_pll_mult_range = {
>         .min = 55,
>         .max = 255,
> @@ -2167,6 +2477,26 @@ static struct clk_hw_onecell_data g12a_hw_onecell_data = {
>                 [CLKID_MALI]                    = &g12a_mali.hw,
>                 [CLKID_MPLL_5OM_DIV]            = &g12a_mpll_50m_div.hw,
>                 [CLKID_MPLL_5OM]                = &g12a_mpll_50m.hw,
> +               [CLKID_SYS_PLL_DIV16_EN]        = &g12a_sys_pll_div16_en.hw,
> +               [CLKID_SYS_PLL_DIV16]           = &g12a_sys_pll_div16.hw,
> +               [CLKID_CPU_CLK_DYN0_SEL]        = &g12a_cpu_clk_dyn0_sel.hw,
> +               [CLKID_CPU_CLK_DYN0_DIV]        = &g12a_cpu_clk_dyn0_div.hw,
> +               [CLKID_CPU_CLK_DYN0]            = &g12a_cpu_clk_dyn0.hw,
> +               [CLKID_CPU_CLK_DYN1_SEL]        = &g12a_cpu_clk_dyn1_sel.hw,
> +               [CLKID_CPU_CLK_DYN1_DIV]        = &g12a_cpu_clk_dyn1_div.hw,
> +               [CLKID_CPU_CLK_DYN1]            = &g12a_cpu_clk_dyn1.hw,
> +               [CLKID_CPU_CLK_DYN]             = &g12a_cpu_clk_dyn.hw,
> +               [CLKID_CPU_CLK]                 = &g12a_cpu_clk.hw,
> +               [CLKID_CPU_CLK_DIV16_EN]        = &g12a_cpu_clk_div16_en.hw,
> +               [CLKID_CPU_CLK_DIV16]           = &g12a_cpu_clk_div16.hw,
> +               [CLKID_CPU_CLK_APB_DIV]         = &g12a_cpu_clk_apb_div.hw,
> +               [CLKID_CPU_CLK_APB]             = &g12a_cpu_clk_apb.hw,
> +               [CLKID_CPU_CLK_ATB_DIV]         = &g12a_cpu_clk_atb_div.hw,
> +               [CLKID_CPU_CLK_ATB]             = &g12a_cpu_clk_atb.hw,
> +               [CLKID_CPU_CLK_AXI_DIV]         = &g12a_cpu_clk_axi_div.hw,
> +               [CLKID_CPU_CLK_AXI]             = &g12a_cpu_clk_axi.hw,
> +               [CLKID_CPU_CLK_TRACE_DIV]       = &g12a_cpu_clk_trace_div.hw,
> +               [CLKID_CPU_CLK_TRACE]           = &g12a_cpu_clk_trace.hw,
>                 [NR_CLKS]                       = NULL,
>         },
>         .num = NR_CLKS,
> @@ -2335,6 +2665,24 @@ static struct clk_regmap *const g12a_clk_regmaps[] = {
>         &g12a_mali_1,
>         &g12a_mali,
>         &g12a_mpll_50m,
> +       &g12a_sys_pll_div16_en,
> +       &g12a_cpu_clk_dyn0_sel,
> +       &g12a_cpu_clk_dyn0_div,
> +       &g12a_cpu_clk_dyn0,
> +       &g12a_cpu_clk_dyn1_sel,
> +       &g12a_cpu_clk_dyn1_div,
> +       &g12a_cpu_clk_dyn1,
> +       &g12a_cpu_clk_dyn,
> +       &g12a_cpu_clk,
> +       &g12a_cpu_clk_div16_en,
> +       &g12a_cpu_clk_apb_div,
> +       &g12a_cpu_clk_apb,
> +       &g12a_cpu_clk_atb_div,
> +       &g12a_cpu_clk_atb,
> +       &g12a_cpu_clk_axi_div,
> +       &g12a_cpu_clk_axi,
> +       &g12a_cpu_clk_trace_div,
> +       &g12a_cpu_clk_trace,
>  };
>
>  static const struct meson_eeclkc_data g12a_clkc_data = {
> diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h
> index 4854750df902..0ba3bfe4d46d 100644
> --- a/drivers/clk/meson/g12a.h
> +++ b/drivers/clk/meson/g12a.h
> @@ -50,6 +50,7 @@
>  #define HHI_GCLK_MPEG2                 0x148
>  #define HHI_GCLK_OTHER                 0x150
>  #define HHI_GCLK_OTHER2                        0x154
> +#define HHI_SYS_CPU_CLK_CNTL1          0x15c
>  #define HHI_VID_CLK_DIV                        0x164
>  #define HHI_MPEG_CLK_CNTL              0x174
>  #define HHI_AUD_CLK_CNTL               0x178
> --
> 2.20.1
>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

* Re: [PATCH 1/2] clk: meson-g12a: add cpu clock bindings
  2019-03-01 10:21 ` [PATCH 1/2] clk: meson-g12a: add cpu clock bindings Neil Armstrong
@ 2019-03-01 15:26   ` Martin Blumenstingl
  2019-03-01 16:43     ` Neil Armstrong
  0 siblings, 1 reply; 9+ messages in thread
From: Martin Blumenstingl @ 2019-03-01 15:26 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: jbrunet, devicetree, linux-amlogic, linux-kernel, linux-clk,
	linux-arm-kernel

Hi Neil,

On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Add Amlogic G12A Family CPU clocks bindings, only export CPU_CLK since
> it should be the only ID used.
is this also true for the CPU post-dividers (APB, ATB, AXI, CPU CLK TRACE)?

> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/clk/meson/g12a.h              | 24 +++++++++++++++++++++++-
>  include/dt-bindings/clock/g12a-clkc.h |  1 +
>  2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h
> index f399dfe1401c..4854750df902 100644
> --- a/drivers/clk/meson/g12a.h
> +++ b/drivers/clk/meson/g12a.h
> @@ -166,8 +166,30 @@
>  #define CLKID_MALI_0_DIV                       170
>  #define CLKID_MALI_1_DIV                       173
>  #define CLKID_MPLL_5OM_DIV                     176
> +#define CLKID_PCIE_PLL_DCO                     178
> +#define CLKID_PCIE_PLL_DCO_DIV2                        179
> +#define CLKID_PCIE_PLL_OD                      180
how are these PCIe clock related to the CPU clocks?

> +#define CLKID_SYS_PLL_DIV16_EN                 181
> +#define CLKID_SYS_PLL_DIV16                    182
> +#define CLKID_CPU_CLK_DYN0_SEL                 183
> +#define CLKID_CPU_CLK_DYN0_DIV                 184
> +#define CLKID_CPU_CLK_DYN0                     185
> +#define CLKID_CPU_CLK_DYN1_SEL                 186
> +#define CLKID_CPU_CLK_DYN1_DIV                 187
> +#define CLKID_CPU_CLK_DYN1                     188
> +#define CLKID_CPU_CLK_DYN                      189
> +#define CLKID_CPU_CLK_DIV16_EN                 191
> +#define CLKID_CPU_CLK_DIV16                    192
> +#define CLKID_CPU_CLK_APB_DIV                  193
> +#define CLKID_CPU_CLK_APB                      194
> +#define CLKID_CPU_CLK_ATB_DIV                  195
> +#define CLKID_CPU_CLK_ATB                      196
> +#define CLKID_CPU_CLK_AXI_DIV                  197
> +#define CLKID_CPU_CLK_AXI                      198
> +#define CLKID_CPU_CLK_TRACE_DIV                        299
> +#define CLKID_CPU_CLK_TRACE                    200
>
> -#define NR_CLKS                                        178
> +#define NR_CLKS                                        201
shouldn't all changes to this file (drivers/clk/meson/g12a.h) be part
of the patch which adds the actual clocks so the dt-bindings patch is
independent of the clock driver patches?
in this case the subject should also be updated to "dt-bindings:
clock: g12a: ..."


Regards
Martin

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

* Re: [PATCH 2/2] clk: meson: g12a: add cpu clocks
  2019-03-01 15:21   ` Martin Blumenstingl
@ 2019-03-01 16:41     ` Neil Armstrong
  2019-03-01 16:52       ` Martin Blumenstingl
  0 siblings, 1 reply; 9+ messages in thread
From: Neil Armstrong @ 2019-03-01 16:41 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: jbrunet, linux-amlogic, linux-kernel, linux-clk, linux-arm-kernel

Hi Martin,

On 01/03/2019 16:21, Martin Blumenstingl wrote:
> Hi Neil,
> 
> it's great to see the progress on G12A!
> 
> On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> Add the Amlogic G12A Family CPU Clock tree in read/only for now.
>>
>> The CPU clock can either use the SYS_PLL for > 1GHz frequencies or
>> use a couple of div+mux from 1GHz/667MHz/24MHz source with 2 non-glitch
>> muxes.
>>
>> Proper DVFS support will come in a second time.
> can you please also mention that this adds various CPU clock
> post-dividers (APB, ATB, AXI and CPU trace)?
> I don't mind them being int his patchset

indeed, I forgot this !

> 
> disclaimer for my code-review:
> - I don't have access to the datasheet so I can't verify if the clock
> tree from this patch is correct
> - the latest buildroot code with G12A support
> (buildroot_openlinux_kernel_4.9_fbdev_20180706) doesn't have proper
> names for all clocks
> - based on my experience with Meson8* this looks good overall, some
> questions and comments below
> 
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>
>> ---
>>  drivers/clk/meson/g12a.c | 348 +++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/meson/g12a.h |   1 +
>>  2 files changed, 349 insertions(+)
>>
>> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
>> index 0e1ce8c03259..4c938f1b8421 100644
>> --- a/drivers/clk/meson/g12a.c
>> +++ b/drivers/clk/meson/g12a.c
>> @@ -150,6 +150,316 @@ static struct clk_regmap g12a_sys_pll = {
>>         },
>>  };
>>
>> +static struct clk_regmap g12a_sys_pll_div16_en = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 24,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "sys_pll_div16_en",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "sys_pll" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is used to debug the sys_pll range
>> +                * Linux should not change it at runtime
>> +                */
> if we're not supposed to touch this the enable bit, can you switch to
> clk_regmap_gate_ro_ops ?

exact

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>> +static struct clk_fixed_factor g12a_sys_pll_div16 = {
>> +       .mult = 1,
>> +       .div = 16,
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "sys_pll_div16",
>> +               .ops = &clk_fixed_factor_ops,
>> +               .parent_names = (const char *[]){ "sys_pll_div16_en" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_dyn0_sel = {
>> +       .data = &(struct clk_regmap_mux_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
>> +               .mask = 0x3,
>> +               .shift = 0,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_dyn0_sel",
> the buildroot code has a variable with the name "p_premux"
> I'm not sure what the datasheet states, but maybe this should be
> cpu_clk_dyn0_pre_sel
> same applies to the corresponding dyn1 clock below

these bit are named "premux1", and cpu_clk_dyn0 names "postmux1",
which has no sense because there is no mux in between.

clk_dyn0_sel is the actual source selector of the dyn0 tree,
clkdyn0 is the top of the dyn0 tree, this is why i did not add "sel"
in it.

> 
>> +               .ops = &clk_regmap_mux_ro_ops,
>> +               .parent_names = (const char *[]){ IN_PREFIX "xtal",
>> +                                                 "fclk_div2",
>> +                                                 "fclk_div3" },
>> +               .num_parents = 3,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_dyn0_div = {
>> +       .data = &(struct clk_regmap_div_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
>> +               .shift = 4,
>> +               .width = 6,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_dyn0_div",
>> +               .ops = &clk_regmap_divider_ro_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_dyn0_sel" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_dyn0 = {
>> +       .data = &(struct clk_regmap_mux_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
>> +               .mask = 0x1,
>> +               .shift = 2,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_dyn0",
> the buildroot code has a variable with the name "p_postmux". in this
> case I would leave the name "cpu_clk_dyn0" because it's the "output"
> of this specific "clock tree/branch".
> same applies to the corresponding dyn1 clock below
> 
>> +               .ops = &clk_regmap_mux_ro_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_dyn0_sel",
>> +                                                 "cpu_clk_dyn0_div" },
>> +               .num_parents = 2,
>> +       },
>> +};
>> +

[...]

>> +
>> +static struct clk_regmap g12a_cpu_clk_div16_en = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 1,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cpu_clk_div16_en",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is used to debug the cpu_clk range
>> +                * Linux should not change it at runtime
>> +                */
> same as above: if we're not supposed to touch this the enable bit, can
> you switch to clk_regmap_gate_ro_ops ?

Yep

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>> +static struct clk_fixed_factor g12a_cpu_clk_div16 = {
>> +       .mult = 1,
>> +       .div = 16,
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_div16",
>> +               .ops = &clk_fixed_factor_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_div16_en" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_apb_div = {
>> +       .data = &(struct clk_regmap_div_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .shift = 3,
>> +               .width = 3,
>> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_apb_div",
>> +               .ops = &clk_regmap_divider_ro_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_apb = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 1,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cpu_clk_apb",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_apb_div" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is set by the ROM monitor code,
>> +                * Linux should not change it at runtime
>> +                */
> same as above: if we're not supposed to touch this the enable bit, can
> you switch to clk_regmap_gate_ro_ops ?

Yep

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_atb_div = {
>> +       .data = &(struct clk_regmap_div_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .shift = 6,
>> +               .width = 3,
>> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_atb_div",
>> +               .ops = &clk_regmap_divider_ro_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_atb = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 17,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cpu_clk_atb",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_atb_div" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is set by the ROM monitor code,
>> +                * Linux should not change it at runtime
>> +                */
> same as above: if we're not supposed to touch this the enable bit, can
> you switch to clk_regmap_gate_ro_ops ?

Yep

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_axi_div = {
>> +       .data = &(struct clk_regmap_div_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .shift = 9,
>> +               .width = 3,
>> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_axi_div",
>> +               .ops = &clk_regmap_divider_ro_ops,
> out of curiosity (this applies to all CPU clock post-dividers):
> did you check whether CLK_DIVIDER_POWER_OF_TWO is correct on G12A?
> I'm asking because on Meson8b the post-dividers select between one of:
> "cpu_clk divided by 2, 3, 4, 5, 6, 7 or 8". also some of the
> post-dividers use register value 0 for cpu_clk_div2 while others use
> register value 1 for cpu_clk_div2.

It's correct !

> 
>> +               .parent_names = (const char *[]){ "cpu_clk" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_axi = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 18,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cpu_clk_axi",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_axi_div" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is set by the ROM monitor code,
>> +                * Linux should not change it at runtime
>> +                */
> same as above: if we're not supposed to touch this the enable bit, can
> you switch to clk_regmap_gate_ro_ops ?

Yep

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_trace_div = {
>> +       .data = &(struct clk_regmap_div_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .shift = 20,
>> +               .width = 3,
>> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
>> +       },
>> +       .hw.init = &(struct clk_init_data){
>> +               .name = "cpu_clk_trace_div",
>> +               .ops = &clk_regmap_divider_ro_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk" },
>> +               .num_parents = 1,
>> +       },
>> +};
>> +
>> +static struct clk_regmap g12a_cpu_clk_trace = {
>> +       .data = &(struct clk_regmap_gate_data){
>> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
>> +               .bit_idx = 23,
>> +       },
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cpu_clk_trace",
>> +               .ops = &clk_regmap_gate_ops,
>> +               .parent_names = (const char *[]){ "cpu_clk_trace_div" },
>> +               .num_parents = 1,
>> +               /*
>> +                * This clock is set by the ROM monitor code,
>> +                * Linux should not change it at runtime
>> +                */
> same as above: if we're not supposed to touch this the enable bit, can
> you switch to clk_regmap_gate_ro_ops ?

Yep

> 
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>>  static const struct pll_mult_range g12a_gp0_pll_mult_range = {
>>         .min = 55,
>>         .max = 255,

Thanks !

Neil


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

* Re: [PATCH 1/2] clk: meson-g12a: add cpu clock bindings
  2019-03-01 15:26   ` Martin Blumenstingl
@ 2019-03-01 16:43     ` Neil Armstrong
  2019-03-01 17:05       ` Martin Blumenstingl
  0 siblings, 1 reply; 9+ messages in thread
From: Neil Armstrong @ 2019-03-01 16:43 UTC (permalink / raw)
  To: Martin Blumenstingl
  Cc: jbrunet, devicetree, linux-amlogic, linux-kernel, linux-clk,
	linux-arm-kernel

Hi Martin,

On 01/03/2019 16:26, Martin Blumenstingl wrote:
> Hi Neil,
> 
> On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> Add Amlogic G12A Family CPU clocks bindings, only export CPU_CLK since
>> it should be the only ID used.
> is this also true for the CPU post-dividers (APB, ATB, AXI, CPU CLK TRACE)?

Do we need these to be exported ?

> 
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/clk/meson/g12a.h              | 24 +++++++++++++++++++++++-
>>  include/dt-bindings/clock/g12a-clkc.h |  1 +
>>  2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h
>> index f399dfe1401c..4854750df902 100644
>> --- a/drivers/clk/meson/g12a.h
>> +++ b/drivers/clk/meson/g12a.h
>> @@ -166,8 +166,30 @@
>>  #define CLKID_MALI_0_DIV                       170
>>  #define CLKID_MALI_1_DIV                       173
>>  #define CLKID_MPLL_5OM_DIV                     176
>> +#define CLKID_PCIE_PLL_DCO                     178
>> +#define CLKID_PCIE_PLL_DCO_DIV2                        179
>> +#define CLKID_PCIE_PLL_OD                      180
> how are these PCIe clock related to the CPU clocks?

Oops, bad merge...

> 
>> +#define CLKID_SYS_PLL_DIV16_EN                 181
>> +#define CLKID_SYS_PLL_DIV16                    182
>> +#define CLKID_CPU_CLK_DYN0_SEL                 183
>> +#define CLKID_CPU_CLK_DYN0_DIV                 184
>> +#define CLKID_CPU_CLK_DYN0                     185
>> +#define CLKID_CPU_CLK_DYN1_SEL                 186
>> +#define CLKID_CPU_CLK_DYN1_DIV                 187
>> +#define CLKID_CPU_CLK_DYN1                     188
>> +#define CLKID_CPU_CLK_DYN                      189
>> +#define CLKID_CPU_CLK_DIV16_EN                 191
>> +#define CLKID_CPU_CLK_DIV16                    192
>> +#define CLKID_CPU_CLK_APB_DIV                  193
>> +#define CLKID_CPU_CLK_APB                      194
>> +#define CLKID_CPU_CLK_ATB_DIV                  195
>> +#define CLKID_CPU_CLK_ATB                      196
>> +#define CLKID_CPU_CLK_AXI_DIV                  197
>> +#define CLKID_CPU_CLK_AXI                      198
>> +#define CLKID_CPU_CLK_TRACE_DIV                        299
>> +#define CLKID_CPU_CLK_TRACE                    200
>>
>> -#define NR_CLKS                                        178
>> +#define NR_CLKS                                        201
> shouldn't all changes to this file (drivers/clk/meson/g12a.h) be part
> of the patch which adds the actual clocks so the dt-bindings patch is
> independent of the clock driver patches?
> in this case the subject should also be updated to "dt-bindings:
> clock: g12a: ..."

I don't have any opinion, Jerome ?

> 
> 
> Regards
> Martin
> 

Thanks !

Neil

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

* Re: [PATCH 2/2] clk: meson: g12a: add cpu clocks
  2019-03-01 16:41     ` Neil Armstrong
@ 2019-03-01 16:52       ` Martin Blumenstingl
  0 siblings, 0 replies; 9+ messages in thread
From: Martin Blumenstingl @ 2019-03-01 16:52 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: jbrunet, linux-amlogic, linux-kernel, linux-clk, linux-arm-kernel

Hi Neil,

On Fri, Mar 1, 2019 at 5:42 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
[...]
> >> +static struct clk_regmap g12a_cpu_clk_dyn0_sel = {
> >> +       .data = &(struct clk_regmap_mux_data){
> >> +               .offset = HHI_SYS_CPU_CLK_CNTL0,
> >> +               .mask = 0x3,
> >> +               .shift = 0,
> >> +       },
> >> +       .hw.init = &(struct clk_init_data){
> >> +               .name = "cpu_clk_dyn0_sel",
> > the buildroot code has a variable with the name "p_premux"
> > I'm not sure what the datasheet states, but maybe this should be
> > cpu_clk_dyn0_pre_sel
> > same applies to the corresponding dyn1 clock below
>
> these bit are named "premux1", and cpu_clk_dyn0 names "postmux1",
> which has no sense because there is no mux in between.
>
> clk_dyn0_sel is the actual source selector of the dyn0 tree,
> clkdyn0 is the top of the dyn0 tree, this is why i did not add "sel"
> in it.
OK, thank you for the explanation.
can you please add a comment with the name from the datasheet in that
case? that'll make it easier (at least for me) to compare the
datasheet (and also buildroot kernel, since similar names are used
there) with the mainline drivers

[...]
> >
> >> +               .flags = CLK_IGNORE_UNUSED,
> >> +       },
> >> +};
> >> +
> >> +static struct clk_regmap g12a_cpu_clk_axi_div = {
> >> +       .data = &(struct clk_regmap_div_data){
> >> +               .offset = HHI_SYS_CPU_CLK_CNTL1,
> >> +               .shift = 9,
> >> +               .width = 3,
> >> +               .flags = CLK_DIVIDER_POWER_OF_TWO,
> >> +       },
> >> +       .hw.init = &(struct clk_init_data){
> >> +               .name = "cpu_clk_axi_div",
> >> +               .ops = &clk_regmap_divider_ro_ops,
> > out of curiosity (this applies to all CPU clock post-dividers):
> > did you check whether CLK_DIVIDER_POWER_OF_TWO is correct on G12A?
> > I'm asking because on Meson8b the post-dividers select between one of:
> > "cpu_clk divided by 2, 3, 4, 5, 6, 7 or 8". also some of the
> > post-dividers use register value 0 for cpu_clk_div2 while others use
> > register value 1 for cpu_clk_div2.
>
> It's correct !
thanks for looking this up again and double-checking!


Martin

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

* Re: [PATCH 1/2] clk: meson-g12a: add cpu clock bindings
  2019-03-01 16:43     ` Neil Armstrong
@ 2019-03-01 17:05       ` Martin Blumenstingl
  0 siblings, 0 replies; 9+ messages in thread
From: Martin Blumenstingl @ 2019-03-01 17:05 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: jbrunet, devicetree, linux-amlogic, linux-kernel, linux-clk,
	linux-arm-kernel

Hi Neil,

On Fri, Mar 1, 2019 at 5:43 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Hi Martin,
>
> On 01/03/2019 16:26, Martin Blumenstingl wrote:
> > Hi Neil,
> >
> > On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> >>
> >> Add Amlogic G12A Family CPU clocks bindings, only export CPU_CLK since
> >> it should be the only ID used.
> > is this also true for the CPU post-dividers (APB, ATB, AXI, CPU CLK TRACE)?
>
> Do we need these to be exported ?
I'm not sure as I couldn't find more details about APB, ATB and AXI on G12A:
- APB and ATB may be needed by the CoreSight bindings
(Documentation/devicetree/bindings/arm/coresight.txt)
- AXI may be needed by the VPU driver (the S912 datasheet mentions in
OSD1_AFBCD_ENABLE: "id_fifo_thrd : unsigned , default = 64, axi id
fifo threshold")

if you don't know either then I'm fine with skipping them for now, we
can still export them later.


Martin

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

end of thread, other threads:[~2019-03-01 17:05 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-01 10:21 [PATCH 0/2] clk: meson: g12a: Add CPU Clock support Neil Armstrong
2019-03-01 10:21 ` [PATCH 1/2] clk: meson-g12a: add cpu clock bindings Neil Armstrong
2019-03-01 15:26   ` Martin Blumenstingl
2019-03-01 16:43     ` Neil Armstrong
2019-03-01 17:05       ` Martin Blumenstingl
2019-03-01 10:21 ` [PATCH 2/2] clk: meson: g12a: add cpu clocks Neil Armstrong
2019-03-01 15:21   ` Martin Blumenstingl
2019-03-01 16:41     ` Neil Armstrong
2019-03-01 16:52       ` Martin Blumenstingl

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