linux-mips.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/6] MIPS: ralink: add CPU clock detection for MT7621
@ 2019-07-24  2:23 Chuanhong Guo
  2019-07-24  2:23 ` [PATCH v2 1/6] dt-bindings: clock: add dt binding header for mt7621-pll Chuanhong Guo
                   ` (5 more replies)
  0 siblings, 6 replies; 22+ messages in thread
From: Chuanhong Guo @ 2019-07-24  2:23 UTC (permalink / raw)
  To: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM
  Cc: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland,
	Ralf Baechle, Paul Burton, James Hogan, John Crispin,
	Greg Kroah-Hartman, Weijie Gao, NeilBrown, Chuanhong Guo

This patchset ports CPU clock detection for MT7621 from OpenWrt.

Last time I sent this, I forgot to add an binding include which
caused a compile error and the patch doesn't stay in linux-next.

This patchset resent the first two commits and also added binding
documentation for mt7621-pll and used it in mt7621-dts at
drivers/staging.

Changes since v1:
1. changed commit title prefix for dt include
2. split the patch adding clock node (details in that patch body)
3. drop useless syscon in dt documentation
4. drop cpuclock node for gbpc1

Chuanhong Guo (6):
  dt-bindings: clock: add dt binding header for mt7621-pll
  MIPS: ralink: drop ralink_clk_init for mt7621
  MIPS: ralink: add clock device providing cpu/bus clock for mt7621
  dt: bindings: add mt7621-pll dt binding documentation
  staging: mt7621-dts: fix register range of memc node in mt7621.dtsi
  staging: mt7621-dts: add dt nodes for mt7621-pll

 .../bindings/clock/mediatek,mt7621-pll.txt    | 18 ++++
 arch/mips/include/asm/mach-ralink/mt7621.h    | 20 ++++
 arch/mips/ralink/mt7621.c                     | 98 +++++++++++++------
 drivers/staging/mt7621-dts/gbpc1.dts          |  5 -
 drivers/staging/mt7621-dts/mt7621.dtsi        | 17 ++--
 include/dt-bindings/clock/mt7621-clk.h        | 14 +++
 6 files changed, 126 insertions(+), 46 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt
 create mode 100644 include/dt-bindings/clock/mt7621-clk.h

-- 
2.21.0


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

* [PATCH v2 1/6] dt-bindings: clock: add dt binding header for mt7621-pll
  2019-07-24  2:23 [PATCH v2 0/6] MIPS: ralink: add CPU clock detection for MT7621 Chuanhong Guo
@ 2019-07-24  2:23 ` Chuanhong Guo
  2019-07-24  2:23 ` [PATCH v2 2/6] MIPS: ralink: drop ralink_clk_init for mt7621 Chuanhong Guo
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Chuanhong Guo @ 2019-07-24  2:23 UTC (permalink / raw)
  To: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM
  Cc: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland,
	Ralf Baechle, Paul Burton, James Hogan, John Crispin,
	Greg Kroah-Hartman, Weijie Gao, NeilBrown, Chuanhong Guo,
	Rob Herring

This patch adds dt binding header for mediatek,mt7621-pll

Signed-off-by: Weijie Gao <hackpascal@gmail.com>
Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Change since v1:
Change commit title prefix.

 include/dt-bindings/clock/mt7621-clk.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 include/dt-bindings/clock/mt7621-clk.h

diff --git a/include/dt-bindings/clock/mt7621-clk.h b/include/dt-bindings/clock/mt7621-clk.h
new file mode 100644
index 000000000000..a29e14ee2efe
--- /dev/null
+++ b/include/dt-bindings/clock/mt7621-clk.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Weijie Gao <hackpascal@gmail.com>
+ */
+
+#ifndef __DT_BINDINGS_MT7621_CLK_H
+#define __DT_BINDINGS_MT7621_CLK_H
+
+#define MT7621_CLK_CPU		0
+#define MT7621_CLK_BUS		1
+
+#define MT7621_CLK_MAX		2
+
+#endif /* __DT_BINDINGS_MT7621_CLK_H */
-- 
2.21.0


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

* [PATCH v2 2/6] MIPS: ralink: drop ralink_clk_init for mt7621
  2019-07-24  2:23 [PATCH v2 0/6] MIPS: ralink: add CPU clock detection for MT7621 Chuanhong Guo
  2019-07-24  2:23 ` [PATCH v2 1/6] dt-bindings: clock: add dt binding header for mt7621-pll Chuanhong Guo
@ 2019-07-24  2:23 ` Chuanhong Guo
  2019-07-24  2:23 ` [PATCH v2 3/6] MIPS: ralink: add clock device providing cpu/bus clock " Chuanhong Guo
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Chuanhong Guo @ 2019-07-24  2:23 UTC (permalink / raw)
  To: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM
  Cc: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland,
	Ralf Baechle, Paul Burton, James Hogan, John Crispin,
	Greg Kroah-Hartman, Weijie Gao, NeilBrown, Chuanhong Guo

This function isn't called anywhere. just drop it.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
Change since v1:
New patch. Split from:
  "MIPS: ralink: fix cpu clock of mt7621 and add dt clk devices"

 arch/mips/ralink/mt7621.c | 43 ---------------------------------------
 1 file changed, 43 deletions(-)

diff --git a/arch/mips/ralink/mt7621.c b/arch/mips/ralink/mt7621.c
index 9415be0d57b8..ba39f3f3a7c7 100644
--- a/arch/mips/ralink/mt7621.c
+++ b/arch/mips/ralink/mt7621.c
@@ -18,11 +18,6 @@
 
 #include "common.h"
 
-#define SYSC_REG_SYSCFG		0x10
-#define SYSC_REG_CPLL_CLKCFG0	0x2c
-#define SYSC_REG_CUR_CLK_STS	0x44
-#define CPU_CLK_SEL		(BIT(30) | BIT(31))
-
 #define MT7621_GPIO_MODE_UART1		1
 #define MT7621_GPIO_MODE_I2C		2
 #define MT7621_GPIO_MODE_UART3_MASK	0x3
@@ -113,44 +108,6 @@ phys_addr_t mips_cpc_default_phys_base(void)
 	panic("Cannot detect cpc address");
 }
 
-void __init ralink_clk_init(void)
-{
-	int cpu_fdiv = 0;
-	int cpu_ffrac = 0;
-	int fbdiv = 0;
-	u32 clk_sts, syscfg;
-	u8 clk_sel = 0, xtal_mode;
-	u32 cpu_clk;
-
-	if ((rt_sysc_r32(SYSC_REG_CPLL_CLKCFG0) & CPU_CLK_SEL) != 0)
-		clk_sel = 1;
-
-	switch (clk_sel) {
-	case 0:
-		clk_sts = rt_sysc_r32(SYSC_REG_CUR_CLK_STS);
-		cpu_fdiv = ((clk_sts >> 8) & 0x1F);
-		cpu_ffrac = (clk_sts & 0x1F);
-		cpu_clk = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000;
-		break;
-
-	case 1:
-		fbdiv = ((rt_sysc_r32(0x648) >> 4) & 0x7F) + 1;
-		syscfg = rt_sysc_r32(SYSC_REG_SYSCFG);
-		xtal_mode = (syscfg >> 6) & 0x7;
-		if (xtal_mode >= 6) {
-			/* 25Mhz Xtal */
-			cpu_clk = 25 * fbdiv * 1000 * 1000;
-		} else if (xtal_mode >= 3) {
-			/* 40Mhz Xtal */
-			cpu_clk = 40 * fbdiv * 1000 * 1000;
-		} else {
-			/* 20Mhz Xtal */
-			cpu_clk = 20 * fbdiv * 1000 * 1000;
-		}
-		break;
-	}
-}
-
 void __init ralink_of_remap(void)
 {
 	rt_sysc_membase = plat_of_remap_node("mtk,mt7621-sysc");
-- 
2.21.0


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

* [PATCH v2 3/6] MIPS: ralink: add clock device providing cpu/bus clock for mt7621
  2019-07-24  2:23 [PATCH v2 0/6] MIPS: ralink: add CPU clock detection for MT7621 Chuanhong Guo
  2019-07-24  2:23 ` [PATCH v2 1/6] dt-bindings: clock: add dt binding header for mt7621-pll Chuanhong Guo
  2019-07-24  2:23 ` [PATCH v2 2/6] MIPS: ralink: drop ralink_clk_init for mt7621 Chuanhong Guo
@ 2019-07-24  2:23 ` Chuanhong Guo
  2019-07-24  2:23 ` [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation Chuanhong Guo
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Chuanhong Guo @ 2019-07-24  2:23 UTC (permalink / raw)
  To: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM
  Cc: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland,
	Ralf Baechle, Paul Burton, James Hogan, John Crispin,
	Greg Kroah-Hartman, Weijie Gao, NeilBrown, Chuanhong Guo

For a long time the mt7621 uses a fixed cpu clock which causes a problem
if the cpu frequency is not 880MHz.
This patch adds cpu/bus clock calculation code and binds clocks to
mt7621-pll node.

Ported from OpenWrt:
c7ca224299 ramips: fix cpu clock of mt7621 and add dt clk devices

Signed-off-by: Weijie Gao <hackpascal@gmail.com>
Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---

Changes since v1:
1. split patch.
2. calculate clocks using the function called by CLK_OF_DECLARE
   drop direct function call in timer-gic.c of ralink_clk_init
3. drop assignment of mips-hpt-frequency

 arch/mips/include/asm/mach-ralink/mt7621.h | 20 ++++++
 arch/mips/ralink/mt7621.c                  | 77 ++++++++++++++++++++++
 2 files changed, 97 insertions(+)

diff --git a/arch/mips/include/asm/mach-ralink/mt7621.h b/arch/mips/include/asm/mach-ralink/mt7621.h
index 65483a4681ab..51a6e51aef3f 100644
--- a/arch/mips/include/asm/mach-ralink/mt7621.h
+++ b/arch/mips/include/asm/mach-ralink/mt7621.h
@@ -17,6 +17,10 @@
 #define SYSC_REG_CHIP_REV		0x0c
 #define SYSC_REG_SYSTEM_CONFIG0		0x10
 #define SYSC_REG_SYSTEM_CONFIG1		0x14
+#define SYSC_REG_CLKCFG0		0x2c
+#define SYSC_REG_CUR_CLK_STS		0x44
+
+#define MEMC_REG_CPU_PLL		0x648
 
 #define CHIP_REV_PKG_MASK		0x1
 #define CHIP_REV_PKG_SHIFT		16
@@ -24,6 +28,22 @@
 #define CHIP_REV_VER_SHIFT		8
 #define CHIP_REV_ECO_MASK		0xf
 
+#define XTAL_MODE_SEL_MASK		0x7
+#define XTAL_MODE_SEL_SHIFT		6
+
+#define CPU_CLK_SEL_MASK		0x3
+#define CPU_CLK_SEL_SHIFT		30
+
+#define CUR_CPU_FDIV_MASK		0x1f
+#define CUR_CPU_FDIV_SHIFT		8
+#define CUR_CPU_FFRAC_MASK		0x1f
+#define CUR_CPU_FFRAC_SHIFT		0
+
+#define CPU_PLL_PREDIV_MASK		0x3
+#define CPU_PLL_PREDIV_SHIFT		12
+#define CPU_PLL_FBDIV_MASK		0x7f
+#define CPU_PLL_FBDIV_SHIFT		4
+
 #define MT7621_DRAM_BASE                0x0
 #define MT7621_DDR2_SIZE_MIN		32
 #define MT7621_DDR2_SIZE_MAX		256
diff --git a/arch/mips/ralink/mt7621.c b/arch/mips/ralink/mt7621.c
index ba39f3f3a7c7..baf9033a67b4 100644
--- a/arch/mips/ralink/mt7621.c
+++ b/arch/mips/ralink/mt7621.c
@@ -7,12 +7,16 @@
 
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <dt-bindings/clock/mt7621-clk.h>
 
 #include <asm/mipsregs.h>
 #include <asm/smp-ops.h>
 #include <asm/mips-cps.h>
 #include <asm/mach-ralink/ralink_regs.h>
 #include <asm/mach-ralink/mt7621.h>
+#include <asm/time.h>
 
 #include <pinmux.h>
 
@@ -103,11 +107,84 @@ static struct rt2880_pmx_group mt7621_pinmux_data[] = {
 	{ 0 }
 };
 
+static struct clk *clks[MT7621_CLK_MAX];
+static struct clk_onecell_data clk_data = {
+	.clks = clks,
+	.clk_num = ARRAY_SIZE(clks),
+};
+
 phys_addr_t mips_cpc_default_phys_base(void)
 {
 	panic("Cannot detect cpc address");
 }
 
+static struct clk *__init mt7621_add_sys_clkdev(
+	const char *id, unsigned long rate)
+{
+	struct clk *clk;
+	int err;
+
+	clk = clk_register_fixed_rate(NULL, id, NULL, 0, rate);
+	if (IS_ERR(clk))
+		panic("failed to allocate %s clock structure", id);
+
+	err = clk_register_clkdev(clk, id, NULL);
+	if (err)
+		panic("unable to register %s clock device", id);
+
+	return clk;
+}
+
+static void __init mt7621_clocks_init(struct device_node *np)
+{
+	const static u32 prediv_tbl[] = {0, 1, 2, 2};
+	u32 syscfg, xtal_sel, clkcfg, clk_sel, curclk, ffiv, ffrac;
+	u32 pll, prediv, fbdiv;
+	u32 xtal_clk, cpu_clk, bus_clk;
+
+	syscfg = rt_sysc_r32(SYSC_REG_SYSTEM_CONFIG0);
+	xtal_sel = (syscfg >> XTAL_MODE_SEL_SHIFT) & XTAL_MODE_SEL_MASK;
+
+	clkcfg = rt_sysc_r32(SYSC_REG_CLKCFG0);
+	clk_sel = (clkcfg >> CPU_CLK_SEL_SHIFT) & CPU_CLK_SEL_MASK;
+
+	curclk = rt_sysc_r32(SYSC_REG_CUR_CLK_STS);
+	ffiv = (curclk >> CUR_CPU_FDIV_SHIFT) & CUR_CPU_FDIV_MASK;
+	ffrac = (curclk >> CUR_CPU_FFRAC_SHIFT) & CUR_CPU_FFRAC_MASK;
+
+	if (xtal_sel <= 2)
+		xtal_clk = 20 * 1000 * 1000;
+	else if (xtal_sel <= 5)
+		xtal_clk = 40 * 1000 * 1000;
+	else
+		xtal_clk = 25 * 1000 * 1000;
+
+	switch (clk_sel) {
+	case 0:
+		cpu_clk = 500 * 1000 * 1000;
+		break;
+	case 1:
+		pll = rt_memc_r32(MEMC_REG_CPU_PLL);
+		fbdiv = (pll >> CPU_PLL_FBDIV_SHIFT) & CPU_PLL_FBDIV_MASK;
+		prediv = (pll >> CPU_PLL_PREDIV_SHIFT) & CPU_PLL_PREDIV_MASK;
+		cpu_clk = ((fbdiv + 1) * xtal_clk) >> prediv_tbl[prediv];
+		break;
+	default:
+		cpu_clk = xtal_clk;
+	}
+
+	cpu_clk = cpu_clk / ffiv * ffrac;
+	bus_clk = cpu_clk / 4;
+
+	clks[MT7621_CLK_CPU] = mt7621_add_sys_clkdev("cpu", cpu_clk);
+	clks[MT7621_CLK_BUS] = mt7621_add_sys_clkdev("bus", bus_clk);
+
+	pr_info("CPU Clock: %dMHz\n", cpu_clk / 1000000);
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
+}
+CLK_OF_DECLARE(mt7621_clk, "mediatek,mt7621-pll", mt7621_clocks_init);
+
 void __init ralink_of_remap(void)
 {
 	rt_sysc_membase = plat_of_remap_node("mtk,mt7621-sysc");
-- 
2.21.0


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

* [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-07-24  2:23 [PATCH v2 0/6] MIPS: ralink: add CPU clock detection for MT7621 Chuanhong Guo
                   ` (2 preceding siblings ...)
  2019-07-24  2:23 ` [PATCH v2 3/6] MIPS: ralink: add clock device providing cpu/bus clock " Chuanhong Guo
@ 2019-07-24  2:23 ` Chuanhong Guo
  2019-07-29 17:33   ` Paul Burton
  2019-08-13 15:51   ` Rob Herring
  2019-07-24  2:23 ` [PATCH v2 5/6] staging: mt7621-dts: fix register range of memc node in mt7621.dtsi Chuanhong Guo
  2019-07-24  2:23 ` [PATCH v2 6/6] staging: mt7621-dts: add dt nodes for mt7621-pll Chuanhong Guo
  5 siblings, 2 replies; 22+ messages in thread
From: Chuanhong Guo @ 2019-07-24  2:23 UTC (permalink / raw)
  To: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM
  Cc: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland,
	Ralf Baechle, Paul Burton, James Hogan, John Crispin,
	Greg Kroah-Hartman, Weijie Gao, NeilBrown, Chuanhong Guo

This commit adds device tree binding documentation for MT7621
PLL controller.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---

Change since v1:
drop useless syscon in compatible string

 .../bindings/clock/mediatek,mt7621-pll.txt     | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt

diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt
new file mode 100644
index 000000000000..7dcfbd5283e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt
@@ -0,0 +1,18 @@
+Binding for Mediatek MT7621 PLL controller
+
+The PLL controller provides the 2 main clocks of the SoC: CPU and BUS.
+
+Required Properties:
+- compatible: has to be "mediatek,mt7621-pll"
+- #clock-cells: has to be one
+
+Optional properties:
+- clock-output-names: should be "cpu", "bus"
+
+Example:
+	pll {
+		compatible = "mediatek,mt7621-pll";
+
+		#clock-cells = <1>;
+		clock-output-names = "cpu", "bus";
+	};
-- 
2.21.0


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

* [PATCH v2 5/6] staging: mt7621-dts: fix register range of memc node in mt7621.dtsi
  2019-07-24  2:23 [PATCH v2 0/6] MIPS: ralink: add CPU clock detection for MT7621 Chuanhong Guo
                   ` (3 preceding siblings ...)
  2019-07-24  2:23 ` [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation Chuanhong Guo
@ 2019-07-24  2:23 ` Chuanhong Guo
  2019-07-24  2:23 ` [PATCH v2 6/6] staging: mt7621-dts: add dt nodes for mt7621-pll Chuanhong Guo
  5 siblings, 0 replies; 22+ messages in thread
From: Chuanhong Guo @ 2019-07-24  2:23 UTC (permalink / raw)
  To: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM
  Cc: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland,
	Ralf Baechle, Paul Burton, James Hogan, John Crispin,
	Greg Kroah-Hartman, Weijie Gao, NeilBrown, Chuanhong Guo

The memc node from mt7621.dtsi has incorrect register resource.
Fix it according to the programming guide.

Signed-off-by: Weijie Gao <hackpascal@gmail.com>
Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---

Change since v1: None.

 drivers/staging/mt7621-dts/mt7621.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index a4c08110094b..d89d68ffa7bc 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -138,7 +138,7 @@
 
 		memc: memc@5000 {
 			compatible = "mtk,mt7621-memc";
-			reg = <0x300 0x100>;
+			reg = <0x5000 0x1000>;
 		};
 
 		cpc: cpc@1fbf0000 {
-- 
2.21.0


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

* [PATCH v2 6/6] staging: mt7621-dts: add dt nodes for mt7621-pll
  2019-07-24  2:23 [PATCH v2 0/6] MIPS: ralink: add CPU clock detection for MT7621 Chuanhong Guo
                   ` (4 preceding siblings ...)
  2019-07-24  2:23 ` [PATCH v2 5/6] staging: mt7621-dts: fix register range of memc node in mt7621.dtsi Chuanhong Guo
@ 2019-07-24  2:23 ` Chuanhong Guo
  5 siblings, 0 replies; 22+ messages in thread
From: Chuanhong Guo @ 2019-07-24  2:23 UTC (permalink / raw)
  To: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM
  Cc: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland,
	Ralf Baechle, Paul Burton, James Hogan, John Crispin,
	Greg Kroah-Hartman, Weijie Gao, NeilBrown, Chuanhong Guo

This commit adds device-tree node for mt7621-pll and use its clocks
accordingly.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---

Changes since v1:
1. drop cpuclock node in gbpc1.dts
2. drop syscon in mt7621-pll node

 drivers/staging/mt7621-dts/gbpc1.dts   |  5 -----
 drivers/staging/mt7621-dts/mt7621.dtsi | 15 +++++++--------
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/mt7621-dts/gbpc1.dts b/drivers/staging/mt7621-dts/gbpc1.dts
index 1fb560ff059c..d94b73243268 100644
--- a/drivers/staging/mt7621-dts/gbpc1.dts
+++ b/drivers/staging/mt7621-dts/gbpc1.dts
@@ -106,11 +106,6 @@
 			clock-frequency = <225000000>;
 };
 
-&cpuclock {
-			compatible = "fixed-clock";
-			clock-frequency = <900000000>;
-};
-
 &pcie {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_pins>;
diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index d89d68ffa7bc..7b82f7f70404 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -1,4 +1,5 @@
 #include <dt-bindings/interrupt-controller/mips-gic.h>
+#include <dt-bindings/clock/mt7621-clk.h>
 #include <dt-bindings/gpio/gpio.h>
 
 / {
@@ -27,12 +28,11 @@
 		serial0 = &uartlite;
 	};
 
-	cpuclock: cpuclock@0 {
-		#clock-cells = <0>;
-		compatible = "fixed-clock";
+	pll: pll {
+		compatible = "mediatek,mt7621-pll";
 
-		/* FIXME: there should be way to detect this */
-		clock-frequency = <880000000>;
+		#clock-cells = <1>;
+		clock-output-names = "cpu", "bus";
 	};
 
 	sysclock: sysclock@0 {
@@ -155,7 +155,6 @@
 			compatible = "ns16550a";
 			reg = <0xc00 0x100>;
 
-			clocks = <&sysclock>;
 			clock-frequency = <50000000>;
 
 			interrupt-parent = <&gic>;
@@ -172,7 +171,7 @@
 			compatible = "ralink,mt7621-spi";
 			reg = <0xb00 0x100>;
 
-			clocks = <&sysclock>;
+			clocks = <&pll MT7621_CLK_BUS>;
 
 			resets = <&rstctrl 18>;
 			reset-names = "spi";
@@ -372,7 +371,7 @@
 		timer {
 			compatible = "mti,gic-timer";
 			interrupts = <GIC_LOCAL 1 IRQ_TYPE_NONE>;
-			clocks = <&cpuclock>;
+			clocks = <&pll MT7621_CLK_CPU>;
 		};
 	};
 
-- 
2.21.0


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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-07-24  2:23 ` [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation Chuanhong Guo
@ 2019-07-29 17:33   ` Paul Burton
  2019-08-13 15:51   ` Rob Herring
  1 sibling, 0 replies; 22+ messages in thread
From: Paul Burton @ 2019-07-29 17:33 UTC (permalink / raw)
  To: Chuanhong Guo
  Cc: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland,
	Ralf Baechle, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown

Hi Chuanhong,

On Wed, Jul 24, 2019 at 10:23:08AM +0800, Chuanhong Guo wrote:
> This commit adds device tree binding documentation for MT7621
> PLL controller.
> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> ---
> 
> Change since v1:
> drop useless syscon in compatible string
> 
>  .../bindings/clock/mediatek,mt7621-pll.txt     | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt

This binding needs review from DT maintainers before I apply it, but as
a general note it's typical to add the binding *before* its use in the
series. That is, this patch should come before patch 3.

Personally I'd squash it with patch 1 so the binding & the header file
needed to use the binding are added in one patch, then a later patch
actually makes use of them.

Thanks,
    Paul

> diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt
> new file mode 100644
> index 000000000000..7dcfbd5283e3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt
> @@ -0,0 +1,18 @@
> +Binding for Mediatek MT7621 PLL controller
> +
> +The PLL controller provides the 2 main clocks of the SoC: CPU and BUS.
> +
> +Required Properties:
> +- compatible: has to be "mediatek,mt7621-pll"
> +- #clock-cells: has to be one
> +
> +Optional properties:
> +- clock-output-names: should be "cpu", "bus"
> +
> +Example:
> +	pll {
> +		compatible = "mediatek,mt7621-pll";
> +
> +		#clock-cells = <1>;
> +		clock-output-names = "cpu", "bus";
> +	};
> -- 
> 2.21.0
> 

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-07-24  2:23 ` [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation Chuanhong Guo
  2019-07-29 17:33   ` Paul Burton
@ 2019-08-13 15:51   ` Rob Herring
  2019-08-17 14:42     ` Chuanhong Guo
  1 sibling, 1 reply; 22+ messages in thread
From: Rob Herring @ 2019-08-13 15:51 UTC (permalink / raw)
  To: Chuanhong Guo
  Cc: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown

On Wed, Jul 24, 2019 at 10:23:08AM +0800, Chuanhong Guo wrote:
> This commit adds device tree binding documentation for MT7621
> PLL controller.
> 
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> ---
> 
> Change since v1:
> drop useless syscon in compatible string
> 
>  .../bindings/clock/mediatek,mt7621-pll.txt     | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt
> new file mode 100644
> index 000000000000..7dcfbd5283e3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-pll.txt
> @@ -0,0 +1,18 @@
> +Binding for Mediatek MT7621 PLL controller
> +
> +The PLL controller provides the 2 main clocks of the SoC: CPU and BUS.
> +
> +Required Properties:
> +- compatible: has to be "mediatek,mt7621-pll"
> +- #clock-cells: has to be one
> +
> +Optional properties:
> +- clock-output-names: should be "cpu", "bus"
> +
> +Example:
> +	pll {
> +		compatible = "mediatek,mt7621-pll";

You didn't answer Stephen's question on v1.

Based on this binding, there is no way to control/program the PLL. Is 
this part of some IP block?

> +
> +		#clock-cells = <1>;
> +		clock-output-names = "cpu", "bus";
> +	};
> -- 
> 2.21.0
> 

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-13 15:51   ` Rob Herring
@ 2019-08-17 14:42     ` Chuanhong Guo
  2019-08-17 15:39       ` Oleksij Rempel
  2019-08-17 15:40       ` Oleksij Rempel
  0 siblings, 2 replies; 22+ messages in thread
From: Chuanhong Guo @ 2019-08-17 14:42 UTC (permalink / raw)
  To: Rob Herring
  Cc: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown

Hi!

On Tue, Aug 13, 2019 at 11:51 PM Rob Herring <robh@kernel.org> wrote:
> [...]
> > +Example:
> > +     pll {
> > +             compatible = "mediatek,mt7621-pll";
>
> You didn't answer Stephen's question on v1.

I thought he was asking why there's a syscon in compatible string. I
noticed that the syscon in my previous patch is a copy-paste error
from elsewhere and dropped it.

>
> Based on this binding, there is no way to control/program the PLL. Is
> this part of some IP block?

The entire section is called "system control" in datasheet and is
occupied in arch/mips/ralink/mt7621.c [0]
Two clocks provided here is determined by reading some read-only
registers in this part.
There's another register in this section providing clock gates for
every peripherals, but MTK doesn't provide a clock plan in their
datasheet. I can't determine corresponding clock frequencies for every
peripherals, thus unable to write a working clock driver.

>
> > +
> > +             #clock-cells = <1>;
> > +             clock-output-names = "cpu", "bus";
> > +     };
> > --
> > 2.21.0
> >

Regards,
Chuanhong Guo

[0] https://elixir.bootlin.com/linux/latest/source/arch/mips/ralink/mt7621.c#L156

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-17 14:42     ` Chuanhong Guo
@ 2019-08-17 15:39       ` Oleksij Rempel
  2019-08-17 16:22         ` Chuanhong Guo
  2019-08-17 15:40       ` Oleksij Rempel
  1 sibling, 1 reply; 22+ messages in thread
From: Oleksij Rempel @ 2019-08-17 15:39 UTC (permalink / raw)
  To: Chuanhong Guo, Rob Herring
  Cc: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown

Hi,

Am 17.08.19 um 16:42 schrieb Chuanhong Guo:
> Hi!
>
> On Tue, Aug 13, 2019 at 11:51 PM Rob Herring <robh@kernel.org> wrote:
>> [...]
>>> +Example:
>>> +     pll {
>>> +             compatible = "mediatek,mt7621-pll";
>>
>> You didn't answer Stephen's question on v1.
>
> I thought he was asking why there's a syscon in compatible string. I
> noticed that the syscon in my previous patch is a copy-paste error
> from elsewhere and dropped it.
>
>>
>> Based on this binding, there is no way to control/program the PLL. Is
>> this part of some IP block?
>
> The entire section is called "system control" in datasheet and is
> occupied in arch/mips/ralink/mt7621.c [0]
> Two clocks provided here is determined by reading some read-only
> registers in this part.
> There's another register in this section providing clock gates for
> every peripherals, but MTK doesn't provide a clock plan in their
> datasheet. I can't determine corresponding clock frequencies for every
> peripherals, thus unable to write a working clock driver.

In provided link [0] the  ralink_clk_init function is reading SYSC_REG_CPLL_CLKCFG0 R/W register.
This register is used to determine clock source,  clock freq and CPU or bus clocks.
SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to enable or disable clocks.
Jist wild assumption. All peripheral devices are suing bus clock.

IMO - this information is enough to create full blown drivers/clk/mediatek/clk-mt7621.c

>>> +
>>> +             #clock-cells = <1>;
>>> +             clock-output-names = "cpu", "bus";
>>> +     };
>>> --
>>> 2.21.0
>>>
>
> Regards,
> Chuanhong Guo
>
> [0] https://elixir.bootlin.com/linux/latest/source/arch/mips/ralink/mt7621.c#L156
>


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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-17 14:42     ` Chuanhong Guo
  2019-08-17 15:39       ` Oleksij Rempel
@ 2019-08-17 15:40       ` Oleksij Rempel
  1 sibling, 0 replies; 22+ messages in thread
From: Oleksij Rempel @ 2019-08-17 15:40 UTC (permalink / raw)
  To: Chuanhong Guo, Rob Herring
  Cc: open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown

Hi,

Am 17.08.19 um 16:42 schrieb Chuanhong Guo:
> Hi!
>
> On Tue, Aug 13, 2019 at 11:51 PM Rob Herring <robh@kernel.org> wrote:
>> [...]
>>> +Example:
>>> +     pll {
>>> +             compatible = "mediatek,mt7621-pll";
>>
>> You didn't answer Stephen's question on v1.
>
> I thought he was asking why there's a syscon in compatible string. I
> noticed that the syscon in my previous patch is a copy-paste error
> from elsewhere and dropped it.
>
>>
>> Based on this binding, there is no way to control/program the PLL. Is
>> this part of some IP block?
>
> The entire section is called "system control" in datasheet and is
> occupied in arch/mips/ralink/mt7621.c [0]
> Two clocks provided here is determined by reading some read-only
> registers in this part.
> There's another register in this section providing clock gates for
> every peripherals, but MTK doesn't provide a clock plan in their
> datasheet. I can't determine corresponding clock frequencies for every
> peripherals, thus unable to write a working clock driver.

In provided link [0] the  ralink_clk_init function is reading SYSC_REG_CPLL_CLKCFG0 R/W register.
This register is used to determine clock source,  clock freq and CPU or bus clocks.
SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to enable or disable clocks.
Jist wild assumption. All peripheral devices are suing bus clock.

IMO - this information is enough to create full blown drivers/clk/mediatek/clk-mt7621.c

>>> +
>>> +             #clock-cells = <1>;
>>> +             clock-output-names = "cpu", "bus";
>>> +     };
>>> --
>>> 2.21.0
>>>
>
> Regards,
> Chuanhong Guo
>
> [0] https://elixir.bootlin.com/linux/latest/source/arch/mips/ralink/mt7621.c#L156
>


--
Regards,
Oleksij

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-17 15:39       ` Oleksij Rempel
@ 2019-08-17 16:22         ` Chuanhong Guo
  2019-08-17 18:05           ` Oleksij Rempel
  0 siblings, 1 reply; 22+ messages in thread
From: Chuanhong Guo @ 2019-08-17 16:22 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Rob Herring, open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown

Hi!

On Sat, Aug 17, 2019 at 11:40 PM Oleksij Rempel <fishor@gmx.net> wrote:

> In provided link [0] the  ralink_clk_init function is reading SYSC_REG_CPLL_CLKCFG0 R/W register.
> This register is used to determine clock source,  clock freq and CPU or bus clocks.

This register should only be changed by bootloader, not kernel. So
it's read-only in kernel's perspective.

> SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to enable or disable clocks.
> Jist wild assumption. All peripheral devices are suing bus clock.

This assumption is incorrect. When this patchset is applied in
OpenWrt, I asked the author why there's still a fixed clock in
mt7621.dtsi, He told me that there's another clock for those unchanged
peripherals and he doesn't have time to write a clock provider for it.
I don't know how many undocumented clocks are there since this piece
of info is missing in datasheet.

>
> IMO - this information is enough to create full blown drivers/clk/mediatek/clk-mt7621.c

And this information isn't enough because the assumption above is incorrect :P

Regards,
Chuanhong Guo

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-17 16:22         ` Chuanhong Guo
@ 2019-08-17 18:05           ` Oleksij Rempel
  2019-08-18  2:29             ` Chuanhong Guo
  0 siblings, 1 reply; 22+ messages in thread
From: Oleksij Rempel @ 2019-08-17 18:05 UTC (permalink / raw)
  To: Chuanhong Guo
  Cc: Rob Herring, open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown

Am 17.08.19 um 18:22 schrieb Chuanhong Guo:
> Hi!
>
> On Sat, Aug 17, 2019 at 11:40 PM Oleksij Rempel <fishor@gmx.net> wrote:
>
>> In provided link [0] the  ralink_clk_init function is reading SYSC_REG_CPLL_CLKCFG0 R/W register.
>> This register is used to determine clock source,  clock freq and CPU or bus clocks.
>
> This register should only be changed by bootloader, not kernel. So
> it's read-only in kernel's perspective.
there is no kernel perspective, until you have some kind of privilege
separation. There is only: "i decided not to write on to writeable
register".

>> SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to enable or disable clocks.
>> Jist wild assumption. All peripheral devices are suing bus clock.
>
> This assumption is incorrect. When this patchset is applied in
> OpenWrt, I asked the author why there's still a fixed clock in
> mt7621.dtsi, He told me that there's another clock for those unchanged
> peripherals and he doesn't have time to write a clock provider for it.

Can you please provide a link to this patch or email.

> I don't know how many undocumented clocks are there since this piece
> of info is missing in datasheet.
>
>>
>> IMO - this information is enough to create full blown drivers/clk/mediatek/clk-mt7621.c
>
> And this information isn't enough because the assumption above is incorrect :P

Ok, let's assume I accept this not technical argumentation.

We have at least 2 know registers:
SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
all or some ip cores. What is probably missing is a set of dividers for
each ip core. From your words it is not document.

With this information the clk driver will provide gate functionality and
a set of hardcoded clocks. With this driver will work part of power
management and nice devicetree without fixed clocks.

--
Regards,
Oleksij

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-17 18:05           ` Oleksij Rempel
@ 2019-08-18  2:29             ` Chuanhong Guo
  2019-08-18  6:10               ` Oleksij Rempel
  0 siblings, 1 reply; 22+ messages in thread
From: Chuanhong Guo @ 2019-08-18  2:29 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Rob Herring, open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown

Hi!

On Sun, Aug 18, 2019 at 2:06 AM Oleksij Rempel <linux@rempel-privat.de> wrote:
> >> SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to enable or disable clocks.
> >> Jist wild assumption. All peripheral devices are suing bus clock.
> >
> > This assumption is incorrect. When this patchset is applied in
> > OpenWrt, I asked the author why there's still a fixed clock in
> > mt7621.dtsi, He told me that there's another clock for those unchanged
> > peripherals and he doesn't have time to write a clock provider for it.
>
> Can you please provide a link to this patch or email.

This discussion is in Chinese and using an IM software so there's no
link available.

> We have at least 2 know registers:
> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
> all or some ip cores.
> What is probably missing is a set of dividers for
> each ip core. From your words it is not document.

The specific missing part I was referring to, is parent clocks for
every gates. I'm not going to assume this with current openwrt device
tree because some peripherals doesn't have a clock binding at all or
have a dummy one there.

>
> With this information the clk driver will provide gate functionality and
> a set of hardcoded clocks. With this driver will work part of power
> management and nice devicetree without fixed clocks.

Regards,
Chuanhong Guo

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-18  2:29             ` Chuanhong Guo
@ 2019-08-18  6:10               ` Oleksij Rempel
  2019-08-18  7:19                 ` Chuanhong Guo
  0 siblings, 1 reply; 22+ messages in thread
From: Oleksij Rempel @ 2019-08-18  6:10 UTC (permalink / raw)
  To: Chuanhong Guo
  Cc: Rob Herring, open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown, Paul Fertser

Am 18.08.19 um 04:29 schrieb Chuanhong Guo:
> Hi!
>
> On Sun, Aug 18, 2019 at 2:06 AM Oleksij Rempel <linux@rempel-privat.de> wrote:
>>>> SYSC_REG_CPLL_CLKCFG1 register is a clock gate controller. It is used to enable or disable clocks.
>>>> Jist wild assumption. All peripheral devices are suing bus clock.
>>>
>>> This assumption is incorrect. When this patchset is applied in
>>> OpenWrt, I asked the author why there's still a fixed clock in
>>> mt7621.dtsi, He told me that there's another clock for those unchanged
>>> peripherals and he doesn't have time to write a clock provider for it.
>>
>> Can you please provide a link to this patch or email.
>
> This discussion is in Chinese and using an IM software so there's no
> link available.
>
>> We have at least 2 know registers:
>> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
>> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
>> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
>> all or some ip cores.
>> What is probably missing is a set of dividers for
>> each ip core. From your words it is not document.
>
> The specific missing part I was referring to, is parent clocks for
> every gates. I'm not going to assume this with current openwrt device
> tree because some peripherals doesn't have a clock binding at all or
> have a dummy one there.

Ok, then I do not understand what is the motivation to upstream
something what is not nearly ready for use. This code is currently on
prototyping phase and you have no information how to make it ready.

It means, we cannot expect that this driver will be fixed any time soon.

--
Regards,
Oleksij

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-18  6:10               ` Oleksij Rempel
@ 2019-08-18  7:19                 ` Chuanhong Guo
  2019-08-18  7:59                   ` Oleksij Rempel
  0 siblings, 1 reply; 22+ messages in thread
From: Chuanhong Guo @ 2019-08-18  7:19 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Rob Herring, open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown, Paul Fertser

Hi!

On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel <linux@rempel-privat.de> wrote:
>
> >> We have at least 2 know registers:
> >> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
> >> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
> >> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
> >> all or some ip cores.
> >> What is probably missing is a set of dividers for
> >> each ip core. From your words it is not document.
> >
> > The specific missing part I was referring to, is parent clocks for
> > every gates. I'm not going to assume this with current openwrt device
> > tree because some peripherals doesn't have a clock binding at all or
> > have a dummy one there.
>
> Ok, then I do not understand what is the motivation to upstream
> something what is not nearly ready for use.

Why isn't it "ready for use" then?
A complete mt7621-pll driver will contain two parts:
1. A clock provider which outputs several clocks
2. A clock gate with parent clocks properly configured

Two clocks provided here are just two clocks that can't be controlled
in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
Having a working CPU clock provider is better than defining a fixed
clock in dts because CPU clock can be controlled by bootloader.
(BTW description for CPU PLL register is also missing in datasheet.)
Clock gate is an unrelated part and there is no information to
properly implement it unless MTK decided to release a clock plan
somehow.

> This code is currently on prototyping phase

Code for clock calculation is done, not "prototyping".

> It means, we cannot expect that this driver will be fixed any time soon.

I think clock gating is a separated feature instead of a broken part
that has to be fixed.

Regards,
Chuanhong Guo

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-18  7:19                 ` Chuanhong Guo
@ 2019-08-18  7:59                   ` Oleksij Rempel
  2019-08-18  8:26                     ` Chuanhong Guo
  0 siblings, 1 reply; 22+ messages in thread
From: Oleksij Rempel @ 2019-08-18  7:59 UTC (permalink / raw)
  To: Chuanhong Guo
  Cc: Rob Herring, open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown, Paul Fertser

Am 18.08.19 um 09:19 schrieb Chuanhong Guo:
> Hi!
>
> On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel <linux@rempel-privat.de> wrote:
>>
>>>> We have at least 2 know registers:
>>>> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
>>>> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
>>>> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
>>>> all or some ip cores.
>>>> What is probably missing is a set of dividers for
>>>> each ip core. From your words it is not document.
>>>
>>> The specific missing part I was referring to, is parent clocks for
>>> every gates. I'm not going to assume this with current openwrt device
>>> tree because some peripherals doesn't have a clock binding at all or
>>> have a dummy one there.
>>
>> Ok, then I do not understand what is the motivation to upstream
>> something what is not nearly ready for use.
>
> Why isn't it "ready for use" then?
> A complete mt7621-pll driver will contain two parts:
> 1. A clock provider which outputs several clocks
> 2. A clock gate with parent clocks properly configured
>
> Two clocks provided here are just two clocks that can't be controlled
> in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
> Having a working CPU clock provider is better than defining a fixed
> clock in dts because CPU clock can be controlled by bootloader.
> (BTW description for CPU PLL register is also missing in datasheet.)
> Clock gate is an unrelated part and there is no information to
> properly implement it unless MTK decided to release a clock plan
> somehow.

With other words, your complete system is running with unknown clock
rates. The source clock in the clock three can be configured differently
by bootloader but you don't know how it is done how and it is not
documented.

>> This code is currently on prototyping phase
>
> Code for clock calculation is done, not "prototyping".
>
>> It means, we cannot expect that this driver will be fixed any time soon.
>
> I think clock gating is a separated feature instead of a broken part
> that has to be fixed.

Ok, i would agree with it. But from what you said, this feature will be
never implemented.

So, I repeat my question. What is the point to upstream code for a
system, which has not enough information to get proper clock rate even
for uart? or is uart running with cpu or bus clock rate?

--
Regards,
Oleksij

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-18  7:59                   ` Oleksij Rempel
@ 2019-08-18  8:26                     ` Chuanhong Guo
  2019-08-18  8:44                       ` Chuanhong Guo
  0 siblings, 1 reply; 22+ messages in thread
From: Chuanhong Guo @ 2019-08-18  8:26 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Rob Herring, open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown, Paul Fertser

Hi!

On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel <linux@rempel-privat.de> wrote:
>
> Am 18.08.19 um 09:19 schrieb Chuanhong Guo:
> > Hi!
> >
> > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel <linux@rempel-privat.de> wrote:
> >>
> >>>> We have at least 2 know registers:
> >>>> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
> >>>> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
> >>>> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
> >>>> all or some ip cores.
> >>>> What is probably missing is a set of dividers for
> >>>> each ip core. From your words it is not document.
> >>>
> >>> The specific missing part I was referring to, is parent clocks for
> >>> every gates. I'm not going to assume this with current openwrt device
> >>> tree because some peripherals doesn't have a clock binding at all or
> >>> have a dummy one there.
> >>
> >> Ok, then I do not understand what is the motivation to upstream
> >> something what is not nearly ready for use.
> >
> > Why isn't it "ready for use" then?
> > A complete mt7621-pll driver will contain two parts:
> > 1. A clock provider which outputs several clocks
> > 2. A clock gate with parent clocks properly configured
> >
> > Two clocks provided here are just two clocks that can't be controlled
> > in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
> > Having a working CPU clock provider is better than defining a fixed
> > clock in dts because CPU clock can be controlled by bootloader.
> > (BTW description for CPU PLL register is also missing in datasheet.)
> > Clock gate is an unrelated part and there is no information to
> > properly implement it unless MTK decided to release a clock plan
> > somehow.
>
> With other words, your complete system is running with unknown clock
> rates.

And without this patchset the complete system is running with unknown
clock and, even worse, we make assumptions about what clock bootloader
uses, hardcoded it in dts and hope it is the correct value.

> The source clock in the clock three can be configured differently
> by bootloader but you don't know how it is done how and it is not
> documented.

Actually, I don't know about this and I didn't wrote the original
clock calculation code. I just ported it from downstream OpenWrt
kernel. Here's a piece of code from Mediatek's SDK kernel:

case 0:
        reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x44));
        cpu_fdiv = ((reg >> 8) & 0x1F);
        cpu_ffrac = (reg & 0x1F);
mips_cpu_feq = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000;
break;
case 1: //CPU PLL
        reg = (*(volatile u32 *)(RALINK_MEMCTRL_BASE + 0x648));
        fbdiv = ((reg >> 4) & 0x7F) + 1;
        reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x10));
        reg = (reg >> 6) & 0x7;
        if(reg >= 6) { //25Mhz Xtal
            mips_cpu_feq = 25 * fbdiv * 1000 * 1000;
        } else if(reg >=3) { //40Mhz Xtal
            mips_cpu_feq = 20 * fbdiv * 1000 * 1000;
        } else { // 20Mhz Xtal
            /* TODO */
        }
        break;



>
> >> This code is currently on prototyping phase
> >
> > Code for clock calculation is done, not "prototyping".
> >
> >> It means, we cannot expect that this driver will be fixed any time soon.
> >
> > I think clock gating is a separated feature instead of a broken part
> > that has to be fixed.
>
> Ok, i would agree with it. But from what you said, this feature will be
> never implemented.
>
> So, I repeat my question. What is the point to upstream code for a
> system, which has not enough information to get proper clock rate even
> for uart? or is uart running with cpu or bus clock rate?

uart runs of a fixed 50MHz clock according to another piece of code
from MTK SDK:
(a pastebin version here for better readability. This specific
question has nothing to do with patch reviewing and doesn't need to be
preserved in mail forever.)
https://paste.ubuntu.com/p/fYmtDFW9nh/

I could ask the same question:
What is the point of upstreaming an incomplete MT7621 support in the
first place? Current MT7621 support in upstream kernel works only for
mt7621a not mt7621s and it runs of unknown clocks. These kind of code
should stay in downstream projects like OpenWrt forever isn't it?

Regards,
Chuanhong Guo

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-18  8:26                     ` Chuanhong Guo
@ 2019-08-18  8:44                       ` Chuanhong Guo
  2019-08-18  9:51                         ` Oleksij Rempel
  0 siblings, 1 reply; 22+ messages in thread
From: Chuanhong Guo @ 2019-08-18  8:44 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Rob Herring, open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown, Paul Fertser

On Sun, Aug 18, 2019 at 4:26 PM Chuanhong Guo <gch981213@gmail.com> wrote:
>
> Hi!
>
> On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel <linux@rempel-privat.de> wrote:
> >
> > Am 18.08.19 um 09:19 schrieb Chuanhong Guo:
> > > Hi!
> > >
> > > On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel <linux@rempel-privat.de> wrote:
> > >>
> > >>>> We have at least 2 know registers:
> > >>>> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
> > >>>> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
> > >>>> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
> > >>>> all or some ip cores.
> > >>>> What is probably missing is a set of dividers for
> > >>>> each ip core. From your words it is not document.
> > >>>
> > >>> The specific missing part I was referring to, is parent clocks for
> > >>> every gates. I'm not going to assume this with current openwrt device
> > >>> tree because some peripherals doesn't have a clock binding at all or
> > >>> have a dummy one there.
> > >>
> > >> Ok, then I do not understand what is the motivation to upstream
> > >> something what is not nearly ready for use.
> > >
> > > Why isn't it "ready for use" then?
> > > A complete mt7621-pll driver will contain two parts:
> > > 1. A clock provider which outputs several clocks
> > > 2. A clock gate with parent clocks properly configured
> > >
> > > Two clocks provided here are just two clocks that can't be controlled
> > > in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
> > > Having a working CPU clock provider is better than defining a fixed
> > > clock in dts because CPU clock can be controlled by bootloader.
> > > (BTW description for CPU PLL register is also missing in datasheet.)
> > > Clock gate is an unrelated part and there is no information to
> > > properly implement it unless MTK decided to release a clock plan
> > > somehow.
> >
> > With other words, your complete system is running with unknown clock
> > rates.
>
> And without this patchset the complete system is running with unknown
> clock and, even worse, we make assumptions about what clock bootloader
> uses, hardcoded it in dts and hope it is the correct value.
>
> > The source clock in the clock three can be configured differently
> > by bootloader but you don't know how it is done how and it is not
> > documented.
>
> Actually, I don't know about this and I didn't wrote the original
> clock calculation code. I just ported it from downstream OpenWrt
> kernel. Here's a piece of code from Mediatek's SDK kernel:
>
> case 0:
>         reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x44));
>         cpu_fdiv = ((reg >> 8) & 0x1F);
>         cpu_ffrac = (reg & 0x1F);
> mips_cpu_feq = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000;
> break;
> case 1: //CPU PLL
>         reg = (*(volatile u32 *)(RALINK_MEMCTRL_BASE + 0x648));
>         fbdiv = ((reg >> 4) & 0x7F) + 1;
>         reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x10));
>         reg = (reg >> 6) & 0x7;
>         if(reg >= 6) { //25Mhz Xtal
>             mips_cpu_feq = 25 * fbdiv * 1000 * 1000;
>         } else if(reg >=3) { //40Mhz Xtal
>             mips_cpu_feq = 20 * fbdiv * 1000 * 1000;
>         } else { // 20Mhz Xtal
>             /* TODO */
>         }
>         break;
>
>
>
> >
> > >> This code is currently on prototyping phase
> > >
> > > Code for clock calculation is done, not "prototyping".
> > >
> > >> It means, we cannot expect that this driver will be fixed any time soon.
> > >
> > > I think clock gating is a separated feature instead of a broken part
> > > that has to be fixed.
> >
> > Ok, i would agree with it. But from what you said, this feature will be
> > never implemented.
> >
> > So, I repeat my question. What is the point to upstream code for a
> > system, which has not enough information to get proper clock rate even
> > for uart? or is uart running with cpu or bus clock rate?
>
> uart runs of a fixed 50MHz clock according to another piece of code
> from MTK SDK:
> (a pastebin version here for better readability. This specific
> question has nothing to do with patch reviewing and doesn't need to be
> preserved in mail forever.)
> https://paste.ubuntu.com/p/fYmtDFW9nh/
>
> I could ask the same question:
> What is the point of upstreaming an incomplete MT7621 support in the
> first place? Current MT7621 support in upstream kernel works only for
> mt7621a not mt7621s and it runs of unknown clocks. These kind of code
> should stay in downstream projects like OpenWrt forever isn't it?

And in fact you've upstreamed a broken ag71xx driver anyway.

This debate goes nowhere. I've clarified the situation and made my
point. Of course I can't read through the ancient and heavily hacked
vendor kernel to figure out a clock plan myself.

Regards,
Chuanhong Guo

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-18  8:44                       ` Chuanhong Guo
@ 2019-08-18  9:51                         ` Oleksij Rempel
  2019-08-18 10:07                           ` Chuanhong Guo
  0 siblings, 1 reply; 22+ messages in thread
From: Oleksij Rempel @ 2019-08-18  9:51 UTC (permalink / raw)
  To: Chuanhong Guo
  Cc: Rob Herring, open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown, Paul Fertser

Am 18.08.19 um 10:44 schrieb Chuanhong Guo:
> On Sun, Aug 18, 2019 at 4:26 PM Chuanhong Guo <gch981213@gmail.com> wrote:
>>
>> Hi!
>>
>> On Sun, Aug 18, 2019 at 3:59 PM Oleksij Rempel <linux@rempel-privat.de> wrote:
>>>
>>> Am 18.08.19 um 09:19 schrieb Chuanhong Guo:
>>>> Hi!
>>>>
>>>> On Sun, Aug 18, 2019 at 2:10 PM Oleksij Rempel <linux@rempel-privat.de> wrote:
>>>>>
>>>>>>> We have at least 2 know registers:
>>>>>>> SYSC_REG_CPLL_CLKCFG0 - it provides some information about boostrapped
>>>>>>> refclock. PLL and dividers used for CPU and some sort of BUS (AHB?).
>>>>>>> SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable clocks for
>>>>>>> all or some ip cores.
>>>>>>> What is probably missing is a set of dividers for
>>>>>>> each ip core. From your words it is not document.
>>>>>>
>>>>>> The specific missing part I was referring to, is parent clocks for
>>>>>> every gates. I'm not going to assume this with current openwrt device
>>>>>> tree because some peripherals doesn't have a clock binding at all or
>>>>>> have a dummy one there.
>>>>>
>>>>> Ok, then I do not understand what is the motivation to upstream
>>>>> something what is not nearly ready for use.
>>>>
>>>> Why isn't it "ready for use" then?
>>>> A complete mt7621-pll driver will contain two parts:
>>>> 1. A clock provider which outputs several clocks
>>>> 2. A clock gate with parent clocks properly configured
>>>>
>>>> Two clocks provided here are just two clocks that can't be controlled
>>>> in kernel no matter where it goes (arch/mips/ralink or drivers/clk).
>>>> Having a working CPU clock provider is better than defining a fixed
>>>> clock in dts because CPU clock can be controlled by bootloader.
>>>> (BTW description for CPU PLL register is also missing in datasheet.)
>>>> Clock gate is an unrelated part and there is no information to
>>>> properly implement it unless MTK decided to release a clock plan
>>>> somehow.
>>>
>>> With other words, your complete system is running with unknown clock
>>> rates.
>>
>> And without this patchset the complete system is running with unknown
>> clock and, even worse, we make assumptions about what clock bootloader
>> uses, hardcoded it in dts and hope it is the correct value.
>>
>>> The source clock in the clock three can be configured differently
>>> by bootloader but you don't know how it is done how and it is not
>>> documented.
>>
>> Actually, I don't know about this and I didn't wrote the original
>> clock calculation code. I just ported it from downstream OpenWrt
>> kernel. Here's a piece of code from Mediatek's SDK kernel:
>>
>> case 0:
>>         reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x44));
>>         cpu_fdiv = ((reg >> 8) & 0x1F);
>>         cpu_ffrac = (reg & 0x1F);
>> mips_cpu_feq = (500 * cpu_ffrac / cpu_fdiv) * 1000 * 1000;
>> break;
>> case 1: //CPU PLL
>>         reg = (*(volatile u32 *)(RALINK_MEMCTRL_BASE + 0x648));
>>         fbdiv = ((reg >> 4) & 0x7F) + 1;
>>         reg = (*(volatile u32 *)(RALINK_SYSCTL_BASE + 0x10));
>>         reg = (reg >> 6) & 0x7;
>>         if(reg >= 6) { //25Mhz Xtal
>>             mips_cpu_feq = 25 * fbdiv * 1000 * 1000;
>>         } else if(reg >=3) { //40Mhz Xtal
>>             mips_cpu_feq = 20 * fbdiv * 1000 * 1000;
>>         } else { // 20Mhz Xtal
>>             /* TODO */
>>         }
>>         break;
>>
>>
>>
>>>
>>>>> This code is currently on prototyping phase
>>>>
>>>> Code for clock calculation is done, not "prototyping".
>>>>
>>>>> It means, we cannot expect that this driver will be fixed any time soon.
>>>>
>>>> I think clock gating is a separated feature instead of a broken part
>>>> that has to be fixed.
>>>
>>> Ok, i would agree with it. But from what you said, this feature will be
>>> never implemented.
>>>
>>> So, I repeat my question. What is the point to upstream code for a
>>> system, which has not enough information to get proper clock rate even
>>> for uart? or is uart running with cpu or bus clock rate?
>>
>> uart runs of a fixed 50MHz clock according to another piece of code
>> from MTK SDK:
>> (a pastebin version here for better readability. This specific
>> question has nothing to do with patch reviewing and doesn't need to be
>> preserved in mail forever.)
>> https://paste.ubuntu.com/p/fYmtDFW9nh/

ok.

lets see more code:
drivers/staging/mt7621-mmc/sd.c
/* clock source for host: global */
#if defined(CONFIG_SOC_MT7620)
static u32 hclks[] = {48000000}; /* +/- by chhung */
#elif defined(CONFIG_SOC_MT7621)
static u32 hclks[] = {50000000}; /* +/- by chhung */
#endif

hm.. 50Mhz again. Feels like APB clock.

./drivers/staging/mt7621-dts/mt7621.dtsi
        cpuclock: cpuclock@0 {
                #clock-cells = <0>;
                compatible = "fixed-clock";

                /* FIXME: there should be way to detect this */
                clock-frequency = <880000000>;
        };

Your driver is trying to cover cpuclock

        sysclock: sysclock@0 {
                #clock-cells = <0>;
                compatible = "fixed-clock";

                /* This is normally 1/4 of cpuclock */
                clock-frequency = <220000000>;
        };

and most probably system clock alias "bus clock", most probably AHB.

                i2c: i2c@900 {
                        compatible = "mediatek,mt7621-i2c";
                        clocks = <&sysclock>;

looks like i2c is using AHB clock.

                uartlite: uartlite@c00 {
                        compatible = "ns16550a";

                        clocks = <&sysclock>;
                        clock-frequency = <50000000>;

and uart is suing APB clock

                spi0: spi@b00 {
                        compatible = "ralink,mt7621-spi";
                        clocks = <&sysclock>;

SPI -> APB

        xhci: xhci@1E1C0000 {
                compatible = "mediatek,mt8173-xhci";
                clocks = <&sysclock>;
                clock-names = "sys_ck";
XHCI -> AHB


        ethernet: ethernet@1e100000 {
                compatible = "mediatek,mt7621-eth";
                clocks = <&sysclock>;
                clock-names = "ethif";

Ethernet -> AHB
So, all device are using two groups of clocks. System clock with seems
to depend on CPU clock.
Or 50MHz clock. Which is for some unknow reason always 50MHz.

Here we have already upstream code:
arch/mips/ralink/mt7620.c
        xtal_rate = mt7620_get_xtal_rate();

#define RFMT(label)     label ":%lu.%03luMHz "
#define RINT(x)         ((x) / 1000000)
#define RFRAC(x)        (((x) / 1000) % 1000)

        if (is_mt76x8()) {
                if (xtal_rate == MHZ(40))
                        cpu_rate = MHZ(580);
                else
                        cpu_rate = MHZ(575);
                dram_rate = sys_rate = cpu_rate / 3;
                periph_rate = MHZ(40);
                pcmi2s_rate = MHZ(480);

                ralink_clk_add("10000d00.uartlite", periph_rate);
                ralink_clk_add("10000e00.uartlite", periph_rate);


which is doing same as I requested before. Well, preferred in
drivers/clk/ manner.

>> I could ask the same question:
>> What is the point of upstreaming an incomplete MT7621 support in the
>> first place? Current MT7621 support in upstream kernel works only for
>> mt7621a not mt7621s and it runs of unknown clocks. These kind of code
>> should stay in downstream projects like OpenWrt forever isn't it?

I need to admit. I really like the idea of pushing all downstream
OpenWrt code mainline. And respect every one who is doing it.

Other question, do this platfrom SoC really worth it? Vendor seems do
not care about it. This product belong to

> And in fact you've upstreamed a broken ag71xx driver anyway.

I don't think this example is nearly comparable. We have enough
documentation to provide working driver. For mainlining was removed
everything what do not belong there:
- direct reading and writing to system wide clock registers. This should
be done over clk framework.
- switch support. It should be done in dsa framework, not inside of
ethernet driver.
- ethertool support was removed to reduce review overhead and can be add
later if need.
- debugfs support was removed removed to reduce review overhead and can
be add later if need.
- phy or board specific quirks was removed as well. This has nothing
todo with ethernet driver and belong to other frameworks or drivers.


> This debate goes nowhere. I've clarified the situation and made my
> point. Of course I can't read through the ancient and heavily hacked
> vendor kernel to figure out a clock plan myself.

Ok, I provided you some productive technical hints how it should be
done. I don't think mt7620 has better documentation then mt7621 and even
in this case it was possible to make more or less good driver.

--
Regards,
Oleksij

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

* Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation
  2019-08-18  9:51                         ` Oleksij Rempel
@ 2019-08-18 10:07                           ` Chuanhong Guo
  0 siblings, 0 replies; 22+ messages in thread
From: Chuanhong Guo @ 2019-08-18 10:07 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Rob Herring, open list:COMMON CLK FRAMEWORK,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:MIPS, open list:STAGING SUBSYSTEM,
	Michael Turquette, Stephen Boyd, Mark Rutland, Ralf Baechle,
	Paul Burton, James Hogan, John Crispin, Greg Kroah-Hartman,
	Weijie Gao, NeilBrown, Paul Fertser

Hi!

On Sun, Aug 18, 2019 at 5:51 PM Oleksij Rempel <linux@rempel-privat.de> wrote:
>
> lets see more code:
> drivers/staging/mt7621-mmc/sd.c
> /* clock source for host: global */
> #if defined(CONFIG_SOC_MT7620)
> static u32 hclks[] = {48000000}; /* +/- by chhung */
> #elif defined(CONFIG_SOC_MT7621)
> static u32 hclks[] = {50000000}; /* +/- by chhung */
> #endif
>
> hm.. 50Mhz again. Feels like APB clock.
>
> ./drivers/staging/mt7621-dts/mt7621.dtsi
>         cpuclock: cpuclock@0 {
>                 #clock-cells = <0>;
>                 compatible = "fixed-clock";
>
>                 /* FIXME: there should be way to detect this */
>                 clock-frequency = <880000000>;
>         };
>
> Your driver is trying to cover cpuclock
>
>         sysclock: sysclock@0 {
>                 #clock-cells = <0>;
>                 compatible = "fixed-clock";
>
>                 /* This is normally 1/4 of cpuclock */
>                 clock-frequency = <220000000>;
>         };
>
> and most probably system clock alias "bus clock", most probably AHB.

This sysclock was the 50MHz clock in OpenWrt. It's set as "bus clock"
upstream by an incorrect commit.
As already stated in previous reply: I'm not going to make assumption
about clock plan using OpenWrt's device tree because it already
contains several mistakes on clocks. Since the upstream device tree
comes from there, I don't trust it either. You might want to check out
patch 6/6 in this series where the original author of this commit in
openwrt fixed some clocks and I ported it here.

> [...]
> > This debate goes nowhere. I've clarified the situation and made my
> > point. Of course I can't read through the ancient and heavily hacked
> > vendor kernel to figure out a clock plan myself.
>
> Ok, I provided you some productive technical hints how it should be
> done. I don't think mt7620 has better documentation then mt7621 and even
> in this case it was possible to make more or less good driver.

It does. A clock plan for mt7620 is available in MT7620 Programming
Guide, Page 14.

Regards,
Chuanhong Guo

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

end of thread, other threads:[~2019-08-18 10:07 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-24  2:23 [PATCH v2 0/6] MIPS: ralink: add CPU clock detection for MT7621 Chuanhong Guo
2019-07-24  2:23 ` [PATCH v2 1/6] dt-bindings: clock: add dt binding header for mt7621-pll Chuanhong Guo
2019-07-24  2:23 ` [PATCH v2 2/6] MIPS: ralink: drop ralink_clk_init for mt7621 Chuanhong Guo
2019-07-24  2:23 ` [PATCH v2 3/6] MIPS: ralink: add clock device providing cpu/bus clock " Chuanhong Guo
2019-07-24  2:23 ` [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation Chuanhong Guo
2019-07-29 17:33   ` Paul Burton
2019-08-13 15:51   ` Rob Herring
2019-08-17 14:42     ` Chuanhong Guo
2019-08-17 15:39       ` Oleksij Rempel
2019-08-17 16:22         ` Chuanhong Guo
2019-08-17 18:05           ` Oleksij Rempel
2019-08-18  2:29             ` Chuanhong Guo
2019-08-18  6:10               ` Oleksij Rempel
2019-08-18  7:19                 ` Chuanhong Guo
2019-08-18  7:59                   ` Oleksij Rempel
2019-08-18  8:26                     ` Chuanhong Guo
2019-08-18  8:44                       ` Chuanhong Guo
2019-08-18  9:51                         ` Oleksij Rempel
2019-08-18 10:07                           ` Chuanhong Guo
2019-08-17 15:40       ` Oleksij Rempel
2019-07-24  2:23 ` [PATCH v2 5/6] staging: mt7621-dts: fix register range of memc node in mt7621.dtsi Chuanhong Guo
2019-07-24  2:23 ` [PATCH v2 6/6] staging: mt7621-dts: add dt nodes for mt7621-pll Chuanhong Guo

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