[PATCH v3 00/11] riscv: k210: Enable use of AI ram bank

[PATCH v3 00/11] riscv: k210: Enable use of AI ram bank

[Sean Anderson]

This ram bank was previously unusable because PLL1 was not started correctly.
This series fixes that bug, and allows U-Boot to relocate into the AI ram. This
provides an extra 2M of space in which to load payloads.  I've also added some
minor patches to bring the device tree and clock driver closer to what Linux has
(or will have).

The bypass clock turned out to be still necessary after all; I had neglected to
test with CLK_K210_SET_RATE enabled, and I didn't notice that the clock rate
wasn't actually getting set (oops). I've added a warning which will hopefully
make it more difficult to make such mistakes in the future.

Changes in v3:
- Add a warning if we can't assign rates
- Split off patchs for fdtdec_setup_mem_size_base_* into another series
- Eliminated separate RAM driver
- Rebased onto u-boot/next

Changes in v2:
- Don't re-enable the PLL
- Simplify PLL instantiation
- Modify clock tree so clint is a child of aclk
- Sync memory dts node with Linux
- Use correct aisram clock

Sean Anderson (11):
  clk: Warn on failure to assign rate
  clk: k210: Fix PLLs not being enabled
  clk: k210: Fix PLL enable always getting taken
  clk: k210: Remove k210_register_pll
  clk: k210: Move the clint clock to under aclk
  clk: Add support for the k210 clock driver pre-relocation
  riscv: Enable some devices pre-relocation
  riscv: Enable AI ram on K210
  riscv: k210: Rename airam to aisram
  riscv: k210: Use AI as the parent clock of aisram, not PLL1
  riscv: Don't reserve AI ram in k210 dts

 arch/riscv/dts/k210.dtsi           | 22 +++++++---------------
 board/sipeed/maix/maix.c           | 14 ++++++++++++--
 configs/sipeed_maix_bitm_defconfig |  2 ++
 drivers/clk/clk-uclass.c           | 11 +++++++----
 drivers/clk/kendryte/clk.c         | 26 ++++++++++++++------------
 drivers/clk/kendryte/pll.c         | 26 ++++----------------------
 include/configs/sipeed-maix.h      |  3 +--
 include/kendryte/pll.h             |  4 ----
 8 files changed, 47 insertions(+), 61 deletions(-)

-- 
2.31.0

[PATCH v3 01/11] clk: Warn on failure to assign rate

[Sean Anderson]

If the user/dev explicitly requests a clock be assigned a certain rate,
then we should warn them if we can't do it. This makes it clear if the
clock is running at the default rate.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v3:
- New

 drivers/clk/clk-uclass.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 4ab3c402ed..53e7be764d 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -14,6 +14,7 @@
 #include <errno.h>
 #include <log.h>
 #include <malloc.h>
+#include <dm/device_compat.h>
 #include <dm/device-internal.h>
 #include <dm/devres.h>
 #include <dm/read.h>
@@ -309,8 +310,9 @@ static int clk_set_default_rates(struct udevice *dev, int stage)
 		ret = clk_get_by_indexed_prop(dev, "assigned-clocks",
 					      index, &clk);
 		if (ret) {
-			debug("%s: could not get assigned clock %d for %s\n",
-			      __func__, index, dev_read_name(dev));
+			dev_dbg(dev,
+				"could not get assigned clock %d (err = %d)\n",
+				index, ret);
 			continue;
 		}
 
@@ -332,8 +334,9 @@ static int clk_set_default_rates(struct udevice *dev, int stage)
 		ret = clk_set_rate(c, rates[index]);
 
 		if (ret < 0) {
-			debug("%s: failed to set rate on clock index %d (%ld) for %s\n",
-			      __func__, index, clk.id, dev_read_name(dev));
+			dev_warn(dev,
+				 "failed to set rate on clock index %d (%ld) (error = %d)\n",
+				 index, clk.id, ret);
 			break;
 		}
 	}
-- 
2.31.0

[PATCH v3 02/11] clk: k210: Fix PLLs not being enabled

[Sean Anderson]

After starting or setting the rate of a PLL, the enable bit must be set.

This fixes a bug where the AI ram would not be accessible, because it
requires PLL1 to be running.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/clk/kendryte/pll.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/kendryte/pll.c b/drivers/clk/kendryte/pll.c
index ab6d75d585..f198920113 100644
--- a/drivers/clk/kendryte/pll.c
+++ b/drivers/clk/kendryte/pll.c
@@ -531,6 +531,7 @@ static int k210_pll_enable(struct clk *clk)
 	k210_pll_waitfor_lock(pll);
 
 	reg &= ~K210_PLL_BYPASS;
+	reg |= K210_PLL_EN;
 	writel(reg, pll->reg);
 
 	return 0;
@@ -550,6 +551,7 @@ static int k210_pll_disable(struct clk *clk)
 	writel(reg, pll->reg);
 
 	reg &= ~K210_PLL_PWRD;
+	reg &= ~K210_PLL_EN;
 	writel(reg, pll->reg);
 	return 0;
 }
-- 
2.31.0

[PATCH v3 03/11] clk: k210: Fix PLL enable always getting taken

[Sean Anderson]

This conditional always evaluated as false, regardless of the value of reg.
Fix it so that it properly tests the bits in the PLL register. Also test
PLL_EN, now that we set it.

Reported-by: Damien Le Moal <Damien.LeMoal@wdc.com>
Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- New

 drivers/clk/kendryte/pll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/kendryte/pll.c b/drivers/clk/kendryte/pll.c
index f198920113..d46fd0ebbf 100644
--- a/drivers/clk/kendryte/pll.c
+++ b/drivers/clk/kendryte/pll.c
@@ -512,7 +512,8 @@ static int k210_pll_enable(struct clk *clk)
 	struct k210_pll *pll = to_k210_pll(clk);
 	u32 reg = readl(pll->reg);
 
-	if ((reg | K210_PLL_PWRD) && !(reg | K210_PLL_RESET))
+	if ((reg & K210_PLL_PWRD) && (reg & K210_PLL_EN) &&
+	    !(reg & K210_PLL_RESET))
 		return 0;
 
 	reg |= K210_PLL_PWRD;
-- 
2.31.0

[PATCH v3 04/11] clk: k210: Remove k210_register_pll

[Sean Anderson]

This simplifies the PLL creation process, since we don't have to pass all
the parameters individually.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- New

 drivers/clk/kendryte/clk.c | 10 +++-------
 drivers/clk/kendryte/pll.c | 21 ---------------------
 include/kendryte/pll.h     |  4 ----
 3 files changed, 3 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
index 3b674a998e..ab86533bb4 100644
--- a/drivers/clk/kendryte/clk.c
+++ b/drivers/clk/kendryte/clk.c
@@ -528,14 +528,10 @@ static int k210_clk_probe(struct udevice *dev)
 		return -ENOMEM;
 	}
 
-	{
-		const struct k210_pll_params *params = &k210_plls[1];
-
+	pll = k210_create_pll(&k210_plls[1], base);
+	if (pll)
 		clk_dm(K210_CLK_PLL1,
-		       k210_register_pll("pll1", in0, base + params->off,
-					 base + params->lock_off, params->shift,
-					 params->width));
-	}
+		       k210_register_pll_struct("pll1", in0, pll));
 
 	/* PLL2 is muxed, so set up a composite clock */
 	mux = k210_create_mux(&k210_muxes[MUXIFY(K210_CLK_PLL2)], base);
diff --git a/drivers/clk/kendryte/pll.c b/drivers/clk/kendryte/pll.c
index d46fd0ebbf..184f37aaf2 100644
--- a/drivers/clk/kendryte/pll.c
+++ b/drivers/clk/kendryte/pll.c
@@ -578,27 +578,6 @@ struct clk *k210_register_pll_struct(const char *name, const char *parent_name,
 	return clk;
 }
 
-struct clk *k210_register_pll(const char *name, const char *parent_name,
-			      void __iomem *reg, void __iomem *lock, u8 shift,
-			      u8 width)
-{
-	struct clk *clk;
-	struct k210_pll *pll;
-
-	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
-	if (!pll)
-		return ERR_PTR(-ENOMEM);
-	pll->reg = reg;
-	pll->lock = lock;
-	pll->shift = shift;
-	pll->width = width;
-
-	clk = k210_register_pll_struct(name, parent_name, pll);
-	if (IS_ERR(clk))
-		kfree(pll);
-	return clk;
-}
-
 U_BOOT_DRIVER(k210_pll) = {
 	.name	= CLK_K210_PLL,
 	.id	= UCLASS_CLK,
diff --git a/include/kendryte/pll.h b/include/kendryte/pll.h
index 55a40b9c97..95b8494f40 100644
--- a/include/kendryte/pll.h
+++ b/include/kendryte/pll.h
@@ -55,8 +55,4 @@ extern const struct clk_ops k210_pll_ops;
 
 struct clk *k210_register_pll_struct(const char *name, const char *parent_name,
 				     struct k210_pll *pll);
-struct clk *k210_register_pll(const char *name, const char *parent_name,
-			      void __iomem *reg, void __iomem *lock, u8 shift,
-			      u8 width);
-
 #endif /* K210_PLL_H */
-- 
2.31.0

[PATCH v3 05/11] clk: k210: Move the clint clock to under aclk

[Sean Anderson]

No other (real) clocks have the cpu clock as their parent; instead they are
children of aclk. Move the clint clock under aclk to match them.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- New

 drivers/clk/kendryte/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
index ab86533bb4..5091f07eb0 100644
--- a/drivers/clk/kendryte/clk.c
+++ b/drivers/clk/kendryte/clk.c
@@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
 
 	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
 	clk_dm(K210_CLK_CLINT,
-	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
+	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
 
 	return 0;
 }
-- 
2.31.0

[PATCH v3 06/11] clk: Add support for the k210 clock driver pre-relocation

[Sean Anderson]

Variables which had previously been stored in .bss are moved to .data. In
addition, probed needs to be reset when the clock driver is re-bound
post-relocation.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/clk/kendryte/clk.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
index 5091f07eb0..2d6ac03693 100644
--- a/drivers/clk/kendryte/clk.c
+++ b/drivers/clk/kendryte/clk.c
@@ -347,9 +347,7 @@ static const struct k210_comp_params k210_comps[] = {
 #undef COMP_NOMUX_ID
 #undef COMP_LIST
 
-static struct clk *k210_bypass_children = {
-	NULL,
-};
+static struct clk *k210_bypass_children __section(.data);
 
 /* Helper functions to create sub-clocks */
 static struct clk_mux *k210_create_mux(const struct k210_mux_params *params,
@@ -475,7 +473,14 @@ cleanup_mux:
 	return comp;
 }
 
-static bool probed;
+static bool __section(.data) probed;
+
+/* reset probed so we will probe again post-relocation */
+static int k210_clk_bind(struct udevice *dev)
+{
+	probed = false;
+	return 0;
+}
 
 static int k210_clk_probe(struct udevice *dev)
 {
@@ -658,5 +663,6 @@ U_BOOT_DRIVER(k210_clk) = {
 	.id = UCLASS_CLK,
 	.of_match = k210_clk_ids,
 	.ops = &k210_clk_ops,
+	.bind = k210_clk_bind,
 	.probe = k210_clk_probe,
 };
-- 
2.31.0

[PATCH v3 07/11] riscv: Enable some devices pre-relocation

[Sean Anderson]

These devices are necessary for the clock driver, which is required by the
sram driver, to run pre-relocation.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 arch/riscv/dts/k210.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/dts/k210.dtsi b/arch/riscv/dts/k210.dtsi
index 0b79a29600..a735bebf2c 100644
--- a/arch/riscv/dts/k210.dtsi
+++ b/arch/riscv/dts/k210.dtsi
@@ -91,6 +91,7 @@
 			 <&sysclk K210_CLK_SRAM1>,
 			 <&sysclk K210_CLK_PLL1>;
 		clock-names = "sram0", "sram1", "airam";
+		u-boot,dm-pre-reloc;
 	};
 
 	reserved-memory {
@@ -109,6 +110,7 @@
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
 			clock-frequency = <26000000>;
+			u-boot,dm-pre-reloc;
 		};
 	};
 
@@ -505,11 +507,13 @@
 					     "syscon", "simple-mfd";
 				reg = <0x50440000 0x100>;
 				reg-io-width = <4>;
+				u-boot,dm-pre-reloc;
 
 				sysclk: clock-controller {
 					#clock-cells = <1>;
 					compatible = "kendryte,k210-clk";
 					clocks = <&in0>;
+					u-boot,dm-pre-reloc;
 				};
 
 				sysrst: reset-controller {
-- 
2.31.0

[PATCH v3 08/11] riscv: Enable AI ram on K210

[Sean Anderson]

We just need to initialize all the clocks pre-reloc. The clock driver
creates a bunch of devices, so we need to increase the pre-reloc malloc
arena.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 board/sipeed/maix/maix.c           | 12 +++++++++++-
 configs/sipeed_maix_bitm_defconfig |  2 ++
 include/configs/sipeed-maix.h      |  3 +--
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/board/sipeed/maix/maix.c b/board/sipeed/maix/maix.c
index cbcb23cf5c..6e582911f8 100644
--- a/board/sipeed/maix/maix.c
+++ b/board/sipeed/maix/maix.c
@@ -14,7 +14,7 @@ phys_size_t get_effective_memsize(void)
 	return CONFIG_SYS_SDRAM_SIZE;
 }
 
-int board_init(void)
+static int sram_init(void)
 {
 	int ret, i;
 	const char * const banks[] = { "sram0", "sram1", "airam" };
@@ -39,3 +39,13 @@ int board_init(void)
 
 	return 0;
 }
+
+int board_early_init_f(void)
+{
+	return sram_init();
+}
+
+int board_init(void)
+{
+	return 0;
+}
diff --git a/configs/sipeed_maix_bitm_defconfig b/configs/sipeed_maix_bitm_defconfig
index 210848cccf..bd877cd055 100644
--- a/configs/sipeed_maix_bitm_defconfig
+++ b/configs/sipeed_maix_bitm_defconfig
@@ -1,4 +1,5 @@
 CONFIG_RISCV=y
+CONFIG_SYS_MALLOC_F_LEN=0x10000
 CONFIG_ENV_SIZE=0x1000
 CONFIG_ENV_OFFSET=0xfff000
 CONFIG_ENV_SECT_SIZE=0x1000
@@ -7,6 +8,7 @@ CONFIG_ARCH_RV64I=y
 CONFIG_STACK_SIZE=0x100000
 CONFIG_USE_BOOTCOMMAND=y
 CONFIG_BOOTCOMMAND="run k210_bootcmd"
+CONFIG_BOARD_EARLY_INIT_F=y
 CONFIG_HUSH_PARSER=y
 CONFIG_MTDIDS_DEFAULT="nor0=spi3:0"
 CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000 at 0xfff000(env)"
diff --git a/include/configs/sipeed-maix.h b/include/configs/sipeed-maix.h
index 4c1ff98ec6..0fbe8a5905 100644
--- a/include/configs/sipeed-maix.h
+++ b/include/configs/sipeed-maix.h
@@ -15,8 +15,7 @@
 #define CONFIG_SYS_CACHELINE_SIZE 64
 
 #define CONFIG_SYS_SDRAM_BASE 0x80000000
-/* Don't relocate into AI ram since it isn't set up yet */
-#define CONFIG_SYS_SDRAM_SIZE (SZ_4M + SZ_2M)
+#define CONFIG_SYS_SDRAM_SIZE SZ_8M
 
 #ifndef CONFIG_EXTRA_ENV_SETTINGS
 #define CONFIG_EXTRA_ENV_SETTINGS \
-- 
2.31.0

[PATCH v3 09/11] riscv: k210: Rename airam to aisram

[Sean Anderson]

This is more consistent with the naming of other ram banks, and matches
what Linux is doing.

Reported-by: Damien Le Moal <Damien.LeMoal@wdc.com>
Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- New

 arch/riscv/dts/k210.dtsi | 4 ++--
 board/sipeed/maix/maix.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/dts/k210.dtsi b/arch/riscv/dts/k210.dtsi
index a735bebf2c..2032f1e5c2 100644
--- a/arch/riscv/dts/k210.dtsi
+++ b/arch/riscv/dts/k210.dtsi
@@ -86,11 +86,11 @@
 		reg = <0x80000000 0x400000>,
 		      <0x80400000 0x200000>,
 		      <0x80600000 0x200000>;
-		reg-names = "sram0", "sram1", "airam";
+		reg-names = "sram0", "sram1", "aisram";
 		clocks = <&sysclk K210_CLK_SRAM0>,
 			 <&sysclk K210_CLK_SRAM1>,
 			 <&sysclk K210_CLK_PLL1>;
-		clock-names = "sram0", "sram1", "airam";
+		clock-names = "sram0", "sram1", "aisram";
 		u-boot,dm-pre-reloc;
 	};
 
diff --git a/board/sipeed/maix/maix.c b/board/sipeed/maix/maix.c
index 6e582911f8..52e4fee2f0 100644
--- a/board/sipeed/maix/maix.c
+++ b/board/sipeed/maix/maix.c
@@ -17,7 +17,7 @@ phys_size_t get_effective_memsize(void)
 static int sram_init(void)
 {
 	int ret, i;
-	const char * const banks[] = { "sram0", "sram1", "airam" };
+	const char * const banks[] = { "sram0", "sram1", "aisram" };
 	ofnode memory;
 	struct clk clk;
 
-- 
2.31.0

[PATCH v3 10/11] riscv: k210: Use AI as the parent clock of aisram, not PLL1

[Sean Anderson]

Testing showed that disabling AI while leaving PLL1 enabled disabled the
aisram. This suggests that AI is a more appropriate clock for that ram
bank.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- New

 arch/riscv/dts/k210.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/dts/k210.dtsi b/arch/riscv/dts/k210.dtsi
index 2032f1e5c2..75e101530b 100644
--- a/arch/riscv/dts/k210.dtsi
+++ b/arch/riscv/dts/k210.dtsi
@@ -89,7 +89,7 @@
 		reg-names = "sram0", "sram1", "aisram";
 		clocks = <&sysclk K210_CLK_SRAM0>,
 			 <&sysclk K210_CLK_SRAM1>,
-			 <&sysclk K210_CLK_PLL1>;
+			 <&sysclk K210_CLK_AI>;
 		clock-names = "sram0", "sram1", "aisram";
 		u-boot,dm-pre-reloc;
 	};
-- 
2.31.0

[PATCH v3 11/11] riscv: Don't reserve AI ram in k210 dts

[Sean Anderson]

It is no longer necessary to disallow ai ram, since it is enabled by the
sram driver.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 arch/riscv/dts/k210.dtsi | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/arch/riscv/dts/k210.dtsi b/arch/riscv/dts/k210.dtsi
index 75e101530b..2492af8038 100644
--- a/arch/riscv/dts/k210.dtsi
+++ b/arch/riscv/dts/k210.dtsi
@@ -94,17 +94,6 @@
 		u-boot,dm-pre-reloc;
 	};
 
-	reserved-memory {
-		#address-cells = <1>;
-		#size-cells = <1>;
-		ranges;
-
-		ai_reserved: ai at 80600000 {
-			reg = <0x80600000 0x200000>;
-			reusable;
-		};
-	};
-
 	clocks {
 		in0: osc {
 			compatible = "fixed-clock";
@@ -179,7 +168,6 @@
 			reg = <0x40800000 0xc00000>;
 			interrupts = <25>;
 			clocks = <&sysclk K210_CLK_AI>;
-			memory-region = <&ai_reserved>;
 			status = "disabled";
 		};
 
-- 
2.31.0

[PATCH v3 02/11] clk: k210: Fix PLLs not being enabled

[Damien Le Moal]

On 2021/04/09 11:13, Sean Anderson wrote:
> After starting or setting the rate of a PLL, the enable bit must be set.
> 
> This fixes a bug where the AI ram would not be accessible, because it
> requires PLL1 to be running.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> 
> (no changes since v1)
> 
>  drivers/clk/kendryte/pll.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/kendryte/pll.c b/drivers/clk/kendryte/pll.c
> index ab6d75d585..f198920113 100644
> --- a/drivers/clk/kendryte/pll.c
> +++ b/drivers/clk/kendryte/pll.c
> @@ -531,6 +531,7 @@ static int k210_pll_enable(struct clk *clk)
>  	k210_pll_waitfor_lock(pll);
>  
>  	reg &= ~K210_PLL_BYPASS;
> +	reg |= K210_PLL_EN;
>  	writel(reg, pll->reg);
>  
>  	return 0;
> @@ -550,6 +551,7 @@ static int k210_pll_disable(struct clk *clk)
>  	writel(reg, pll->reg);
>  
>  	reg &= ~K210_PLL_PWRD;
> +	reg &= ~K210_PLL_EN;
>  	writel(reg, pll->reg);
>  	return 0;
>  }
> 

Looks good. That matches what the linux driver is doing.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>


-- 
Damien Le Moal
Western Digital Research

[PATCH v3 05/11] clk: k210: Move the clint clock to under aclk

[Damien Le Moal]

On 2021/04/09 11:13, Sean Anderson wrote:
> No other (real) clocks have the cpu clock as their parent; instead they are
> children of aclk. Move the clint clock under aclk to match them.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - New
> 
>  drivers/clk/kendryte/clk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
> index ab86533bb4..5091f07eb0 100644
> --- a/drivers/clk/kendryte/clk.c
> +++ b/drivers/clk/kendryte/clk.c
> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
>  
>  	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
>  	clk_dm(K210_CLK_CLINT,
> -	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
> +	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
>  
>  	return 0;
>  }
> 

Looks good, but representing this clock is in fact useless, at least in Linux,
since the clint driver does not use it directly and derives its rate from
riscv_timebase which is set from the timebase-frequency DT property.

Not sure how u-boot handles that though. Since your code allows changing the
PLLs frequency, the timebase-frequency property may end up being buggy if it is
not changed too.

-- 
Damien Le Moal
Western Digital Research

[PATCH v3 05/11] clk: k210: Move the clint clock to under aclk

[Sean Anderson]

On 4/8/21 10:54 PM, Damien Le Moal wrote:
> On 2021/04/09 11:13, Sean Anderson wrote:
>> No other (real) clocks have the cpu clock as their parent; instead they are
>> children of aclk. Move the clint clock under aclk to match them.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> (no changes since v2)
>>
>> Changes in v2:
>> - New
>>
>>   drivers/clk/kendryte/clk.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
>> index ab86533bb4..5091f07eb0 100644
>> --- a/drivers/clk/kendryte/clk.c
>> +++ b/drivers/clk/kendryte/clk.c
>> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
>>   
>>   	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
>>   	clk_dm(K210_CLK_CLINT,
>> -	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
>> +	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
>>   
>>   	return 0;
>>   }
>>
> 
> Looks good, but representing this clock is in fact useless, at least in Linux,
> since the clint driver does not use it directly and derives its rate from
> riscv_timebase which is set from the timebase-frequency DT property.
> 
> Not sure how u-boot handles that though. Since your code allows changing the
> PLLs frequency, the timebase-frequency property may end up being buggy if it is
> not changed too.
> 

U-Boot uses the clocks property since 47d7e3b5eb ("riscv: Move timer
portions of SiFive CLINT to drivers/timer") :)

--Sean

[PATCH v3 05/11] clk: k210: Move the clint clock to under aclk

[Damien Le Moal]

On 2021/04/09 11:54, Damien Le Moal wrote:
> On 2021/04/09 11:13, Sean Anderson wrote:
>> No other (real) clocks have the cpu clock as their parent; instead they are
>> children of aclk. Move the clint clock under aclk to match them.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> (no changes since v2)
>>
>> Changes in v2:
>> - New
>>
>>  drivers/clk/kendryte/clk.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
>> index ab86533bb4..5091f07eb0 100644
>> --- a/drivers/clk/kendryte/clk.c
>> +++ b/drivers/clk/kendryte/clk.c
>> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
>>  
>>  	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
>>  	clk_dm(K210_CLK_CLINT,
>> -	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
>> +	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
>>  
>>  	return 0;
>>  }
>>
> 
> Looks good, but representing this clock is in fact useless, at least in Linux,
> since the clint driver does not use it directly and derives its rate from
> riscv_timebase which is set from the timebase-frequency DT property.
> 
> Not sure how u-boot handles that though. Since your code allows changing the
> PLLs frequency, the timebase-frequency property may end up being buggy if it is
> not changed too.

Actually, thinking about this some more, the clint DT node should have an
optional clock entry and use that clock rate to set riscv_timebase if the clock
entry is present. riscv_timebase can be set using timebase-frequency DT property
if the clock is not set.

> 


-- 
Damien Le Moal
Western Digital Research

[PATCH v3 05/11] clk: k210: Move the clint clock to under aclk

[Damien Le Moal]

On 2021/04/09 11:58, Sean Anderson wrote:
> 
> On 4/8/21 10:54 PM, Damien Le Moal wrote:
>> On 2021/04/09 11:13, Sean Anderson wrote:
>>> No other (real) clocks have the cpu clock as their parent; instead they are
>>> children of aclk. Move the clint clock under aclk to match them.
>>>
>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>> ---
>>>
>>> (no changes since v2)
>>>
>>> Changes in v2:
>>> - New
>>>
>>>   drivers/clk/kendryte/clk.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
>>> index ab86533bb4..5091f07eb0 100644
>>> --- a/drivers/clk/kendryte/clk.c
>>> +++ b/drivers/clk/kendryte/clk.c
>>> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
>>>   
>>>   	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
>>>   	clk_dm(K210_CLK_CLINT,
>>> -	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
>>> +	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
>>>   
>>>   	return 0;
>>>   }
>>>
>>
>> Looks good, but representing this clock is in fact useless, at least in Linux,
>> since the clint driver does not use it directly and derives its rate from
>> riscv_timebase which is set from the timebase-frequency DT property.
>>
>> Not sure how u-boot handles that though. Since your code allows changing the
>> PLLs frequency, the timebase-frequency property may end up being buggy if it is
>> not changed too.
>>
> 
> U-Boot uses the clocks property since 47d7e3b5eb ("riscv: Move timer
> portions of SiFive CLINT to drivers/timer") :)

I remember that adding the clock property to Linux DT gave warning with make
dtb_check. Hence I removed it. driver/clocksoure/timer-riscv.c also make the
timebase-frequency DT property mandatory. Time for some more patching on Linux
side I guess :)


> 
> --Sean
> 


-- 
Damien Le Moal
Western Digital Research

[PATCH v3 05/11] clk: k210: Move the clint clock to under aclk

[Sean Anderson]

On 4/8/21 10:58 PM, Damien Le Moal wrote:
> On 2021/04/09 11:54, Damien Le Moal wrote:
>> On 2021/04/09 11:13, Sean Anderson wrote:
>>> No other (real) clocks have the cpu clock as their parent; instead they are
>>> children of aclk. Move the clint clock under aclk to match them.
>>>
>>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>>> ---
>>>
>>> (no changes since v2)
>>>
>>> Changes in v2:
>>> - New
>>>
>>>   drivers/clk/kendryte/clk.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
>>> index ab86533bb4..5091f07eb0 100644
>>> --- a/drivers/clk/kendryte/clk.c
>>> +++ b/drivers/clk/kendryte/clk.c
>>> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev)
>>>   
>>>   	/* The MTIME register in CLINT runs at one 50th the CPU clock speed */
>>>   	clk_dm(K210_CLK_CLINT,
>>> -	       clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50));
>>> +	       clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50));
>>>   
>>>   	return 0;
>>>   }
>>>
>>
>> Looks good, but representing this clock is in fact useless, at least in Linux,
>> since the clint driver does not use it directly and derives its rate from
>> riscv_timebase which is set from the timebase-frequency DT property.
>>
>> Not sure how u-boot handles that though. Since your code allows changing the
>> PLLs frequency, the timebase-frequency property may end up being buggy if it is
>> not changed too.
> 
> Actually, thinking about this some more, the clint DT node should have an
> optional clock entry and use that clock rate to set riscv_timebase if the clock
> entry is present. riscv_timebase can be set using timebase-frequency DT property
> if the clock is not set.

This is how it is done in U-Boot. See timer_pre_probe and
timer_timebase_fallback for the mechanism.

--Sean