[PATCH v2 00/11] riscv: Add Sipeed Maix support

[PATCH v2 00/11] riscv: Add Sipeed Maix support

[Sean Anderson]

This patch series adds support for Sipeed Maix boards and the
Kendryte K210 CPU. Currently, only the Maix Bit V2.0 is supported,
however other models are similar. This series depends on
<https://patchwork.ozlabs.org/patch/1215327/>
(clk: Include missing headers for linux/clk-provider.h).

To flash u-boot to a maix bit, run
kflash -tp /dev/<your tty here> -B bit_mic u-boot-dtb.bin

Boot output should look like the following:

U-Boot 2020.01-00455-gad03fd83e1 (Jan 15 2020 - 17:10:24 -0500)

DRAM:  8 MiB
MMC:   spi at 52000000:slot at 0: 0
In:    serial at 38000000
Out:   serial at 38000000
Err:   serial at 38000000
=> 

I would really appreciate feedback! Many of the changes I had to make
for this revision were fairly trivial and could have been caught by
someone more familiar with the source code than I. In particular, there
are still some questions in individual patches which have yet to be
answered.

Changes for v2:
  Many bugfixes for the device tree.
  Modify the config to build without errors.
  Add support for keeping internal PLL frequencies in-range.
  Fix several rebase-induced artifacts.

Sean Anderson (11):
  clk: Always use the supplied struct clk
  clk: Check that ops of composite clock components exist before calling
  riscv: Add headers for asm/global_data.h
  riscv: Add an option to default to RV64I
  riscv: Add option to disable writes to mcounteren
  riscv: Fix incorrect cpu frequency on RV64
  riscv: Add initial Sipeed Maix support
  riscv: Add device tree for K210
  riscv: Add K210 sysctl support
  riscv: Add K210 pll support
  riscv: Add K210 clock support

 arch/riscv/Kconfig                      |  19 +
 arch/riscv/cpu/cpu.c                    |   2 +
 arch/riscv/dts/Makefile                 |   1 +
 arch/riscv/dts/k210-maix-bit.dts        |  42 ++
 arch/riscv/dts/k210.dtsi                | 453 ++++++++++++++++++
 arch/riscv/include/asm/global_data.h    |   2 +
 arch/riscv/include/asm/k210_sysctl.h    |  43 ++
 arch/riscv/lib/Makefile                 |   1 +
 arch/riscv/lib/k210_sysctl.c            |  21 +
 board/sipeed/maix/Kconfig               |  49 ++
 board/sipeed/maix/MAINTAINERS           |  13 +
 board/sipeed/maix/Makefile              |   5 +
 board/sipeed/maix/maix.c                |   9 +
 configs/sipeed_maix_bitm_defconfig      |  95 ++++
 drivers/clk/Kconfig                     |   1 +
 drivers/clk/Makefile                    |   1 +
 drivers/clk/clk-composite.c             |  65 ++-
 drivers/clk/clk-divider.c               |   6 +-
 drivers/clk/clk-fixed-factor.c          |   3 +-
 drivers/clk/clk-gate.c                  |   6 +-
 drivers/clk/clk-mux.c                   |  12 +-
 drivers/clk/imx/clk-gate2.c             |   4 +-
 drivers/clk/kendryte/Kconfig            |   7 +
 drivers/clk/kendryte/Makefile           |   1 +
 drivers/clk/kendryte/clk.c              | 390 ++++++++++++++++
 drivers/clk/kendryte/clk.h              |  27 ++
 drivers/clk/kendryte/pll.c              | 598 ++++++++++++++++++++++++
 drivers/clk/kendryte/pll.h              |  38 ++
 drivers/cpu/riscv_cpu.c                 |   2 +
 include/configs/sipeed-maix.h           |  19 +
 include/dt-bindings/clock/k210-sysctl.h |  54 +++
 include/dt-bindings/reset/k210-sysctl.h |  38 ++
 32 files changed, 1985 insertions(+), 42 deletions(-)
 create mode 100644 arch/riscv/dts/k210-maix-bit.dts
 create mode 100644 arch/riscv/dts/k210.dtsi
 create mode 100644 arch/riscv/include/asm/k210_sysctl.h
 create mode 100644 arch/riscv/lib/k210_sysctl.c
 create mode 100644 board/sipeed/maix/Kconfig
 create mode 100644 board/sipeed/maix/MAINTAINERS
 create mode 100644 board/sipeed/maix/Makefile
 create mode 100644 board/sipeed/maix/maix.c
 create mode 100644 configs/sipeed_maix_bitm_defconfig
 create mode 100644 drivers/clk/kendryte/Kconfig
 create mode 100644 drivers/clk/kendryte/Makefile
 create mode 100644 drivers/clk/kendryte/clk.c
 create mode 100644 drivers/clk/kendryte/clk.h
 create mode 100644 drivers/clk/kendryte/pll.c
 create mode 100644 drivers/clk/kendryte/pll.h
 create mode 100644 include/configs/sipeed-maix.h
 create mode 100644 include/dt-bindings/clock/k210-sysctl.h
 create mode 100644 include/dt-bindings/reset/k210-sysctl.h

-- 
2.24.1

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Sean Anderson]

CCF clocks should always use the struct clock passed to their methods for
extracting the driver-specific clock information struct. Previously, many
functions would use the clk->dev->priv if the device was bound. This could cause
problems with composite clocks. The individual clocks in a composite clock did
not have the ->dev field filled in. This was fine, because the device-specific
clock information would be used. However, since there was no ->dev, there was no
way to get the parent clock. This caused the recalc_rate method of the CCF
divider clock to fail. One option would be to use the clk->priv field to get the
composite clock and from there get the appropriate parent device. However, this
would tie the implementation to the composite clock. In general, different
devices should not rely on the contents of ->priv from another device.

The simple solution to this problem is to just always use the supplied struct
clock. The composite clock now fills in the ->dev pointer of its child clocks.
This allows child clocks to make calls like clk_get_parent() without issue.

imx avoided the above problem by using a custom get_rate function with composite
clocks.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
 drivers/clk/clk-composite.c    |  8 ++++++++
 drivers/clk/clk-divider.c      |  6 ++----
 drivers/clk/clk-fixed-factor.c |  3 +--
 drivers/clk/clk-gate.c         |  6 ++----
 drivers/clk/clk-mux.c          | 12 ++++--------
 drivers/clk/imx/clk-gate2.c    |  4 ++--
 6 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index a5626c33d1..d0f273d47f 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -145,6 +145,14 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 		goto err;
 	}
 
+	if (composite->mux)
+		composite->mux->dev = clk->dev;
+	if (composite->rate)
+		composite->rate->dev = clk->dev;
+	if (composite->gate)
+		composite->gate->dev = clk->dev;
+
+
 	return clk;
 
 err:
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 822e09b084..bfa05f24a3 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -70,8 +70,7 @@ unsigned long divider_recalc_rate(struct clk *hw, unsigned long parent_rate,
 
 static ulong clk_divider_recalc_rate(struct clk *clk)
 {
-	struct clk_divider *divider = to_clk_divider(clk_dev_binded(clk) ?
-			dev_get_clk_ptr(clk->dev) : clk);
+	struct clk_divider *divider = to_clk_divider(clk);
 	unsigned long parent_rate = clk_get_parent_rate(clk);
 	unsigned int val;
 
@@ -150,8 +149,7 @@ int divider_get_val(unsigned long rate, unsigned long parent_rate,
 
 static ulong clk_divider_set_rate(struct clk *clk, unsigned long rate)
 {
-	struct clk_divider *divider = to_clk_divider(clk_dev_binded(clk) ?
-			dev_get_clk_ptr(clk->dev) : clk);
+	struct clk_divider *divider = to_clk_divider(clk);
 	unsigned long parent_rate = clk_get_parent_rate(clk);
 	int value;
 	u32 val;
diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 711b0588bc..d2401cf440 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -18,8 +18,7 @@
 
 static ulong clk_factor_recalc_rate(struct clk *clk)
 {
-	struct clk_fixed_factor *fix =
-		to_clk_fixed_factor(dev_get_clk_ptr(clk->dev));
+	struct clk_fixed_factor *fix = to_clk_fixed_factor(clk);
 	unsigned long parent_rate = clk_get_parent_rate(clk);
 	unsigned long long int rate;
 
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 70b8794554..b2933bc24a 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -43,8 +43,7 @@
  */
 static void clk_gate_endisable(struct clk *clk, int enable)
 {
-	struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ?
-			dev_get_clk_ptr(clk->dev) : clk);
+	struct clk_gate *gate = to_clk_gate(clk);
 	int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;
 	u32 reg;
 
@@ -86,8 +85,7 @@ static int clk_gate_disable(struct clk *clk)
 
 int clk_gate_is_enabled(struct clk *clk)
 {
-	struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ?
-			dev_get_clk_ptr(clk->dev) : clk);
+	struct clk_gate *gate = to_clk_gate(clk);
 	u32 reg;
 
 #if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF)
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 5acc0b8cbd..67b4afef28 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -35,8 +35,7 @@
 int clk_mux_val_to_index(struct clk *clk, u32 *table, unsigned int flags,
 			 unsigned int val)
 {
-	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
-			dev_get_clk_ptr(clk->dev) : clk);
+	struct clk_mux *mux = to_clk_mux(clk);
 	int num_parents = mux->num_parents;
 
 	if (table) {
@@ -79,8 +78,7 @@ unsigned int clk_mux_index_to_val(u32 *table, unsigned int flags, u8 index)
 
 u8 clk_mux_get_parent(struct clk *clk)
 {
-	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
-			dev_get_clk_ptr(clk->dev) : clk);
+	struct clk_mux *mux = to_clk_mux(clk);
 	u32 val;
 
 #if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF)
@@ -97,8 +95,7 @@ u8 clk_mux_get_parent(struct clk *clk)
 static int clk_fetch_parent_index(struct clk *clk,
 				  struct clk *parent)
 {
-	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
-			dev_get_clk_ptr(clk->dev) : clk);
+	struct clk_mux *mux = to_clk_mux(clk);
 
 	int i;
 
@@ -115,8 +112,7 @@ static int clk_fetch_parent_index(struct clk *clk,
 
 static int clk_mux_set_parent(struct clk *clk, struct clk *parent)
 {
-	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
-			dev_get_clk_ptr(clk->dev) : clk);
+	struct clk_mux *mux = to_clk_mux(clk);
 	int index;
 	u32 val;
 	u32 reg;
diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c
index 1b9db6e791..e32c0cb53e 100644
--- a/drivers/clk/imx/clk-gate2.c
+++ b/drivers/clk/imx/clk-gate2.c
@@ -37,7 +37,7 @@ struct clk_gate2 {
 
 static int clk_gate2_enable(struct clk *clk)
 {
-	struct clk_gate2 *gate = to_clk_gate2(dev_get_clk_ptr(clk->dev));
+	struct clk_gate2 *gate = to_clk_gate2(clk);
 	u32 reg;
 
 	reg = readl(gate->reg);
@@ -50,7 +50,7 @@ static int clk_gate2_enable(struct clk *clk)
 
 static int clk_gate2_disable(struct clk *clk)
 {
-	struct clk_gate2 *gate = to_clk_gate2(dev_get_clk_ptr(clk->dev));
+	struct clk_gate2 *gate = to_clk_gate2(clk);
 	u32 reg;
 
 	reg = readl(gate->reg);
-- 
2.24.1

[PATCH v2 02/11] clk: Check that ops of composite clock components, exist before calling

[Sean Anderson]

clk_composite_ops was shared between all devices in the composite clock driver.
If one clock had a feature (such as supporting set_parent) which another clock
did not, it could call a null pointer dereference.

This patch does three things
1. It adds null-pointer checks to all composite clock functions.
2. It makes clk_composite_ops const and sets its functions at compile-time.
3. It adds some basic sanity checks to num_parents.

The combined effect of these changes is that any of mux, rate, or gate can be
NULL, and composite clocks will still function normally. Previously, at least
mux had to exist, since clk_composite_get_parent was used to determine the
parent for clk_register.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
 drivers/clk/clk-composite.c | 57 +++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index d0f273d47f..ea9982fd57 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -22,7 +22,10 @@ static u8 clk_composite_get_parent(struct clk *clk)
 		(struct clk *)dev_get_clk_ptr(clk->dev) : clk);
 	struct clk *mux = composite->mux;
 
-	return clk_mux_get_parent(mux);
+	if (mux)
+		return clk_mux_get_parent(mux);
+	else
+		return 0;
 }
 
 static int clk_composite_set_parent(struct clk *clk, struct clk *parent)
@@ -32,7 +35,10 @@ static int clk_composite_set_parent(struct clk *clk, struct clk *parent)
 	const struct clk_ops *mux_ops = composite->mux_ops;
 	struct clk *mux = composite->mux;
 
-	return mux_ops->set_parent(mux, parent);
+	if (mux && mux_ops)
+		return mux_ops->set_parent(mux, parent);
+	else
+		return -ENOTSUPP;
 }
 
 static unsigned long clk_composite_recalc_rate(struct clk *clk)
@@ -42,7 +48,10 @@ static unsigned long clk_composite_recalc_rate(struct clk *clk)
 	const struct clk_ops *rate_ops = composite->rate_ops;
 	struct clk *rate = composite->rate;
 
-	return rate_ops->get_rate(rate);
+	if (rate && rate_ops)
+		return rate_ops->get_rate(rate);
+	else
+		return clk_get_parent_rate(clk);
 }
 
 static ulong clk_composite_set_rate(struct clk *clk, unsigned long rate)
@@ -52,7 +61,10 @@ static ulong clk_composite_set_rate(struct clk *clk, unsigned long rate)
 	const struct clk_ops *rate_ops = composite->rate_ops;
 	struct clk *clk_rate = composite->rate;
 
-	return rate_ops->set_rate(clk_rate, rate);
+	if (rate && rate_ops)
+		return rate_ops->set_rate(clk_rate, rate);
+	else
+		return -ENOTSUPP;
 }
 
 static int clk_composite_enable(struct clk *clk)
@@ -62,7 +74,10 @@ static int clk_composite_enable(struct clk *clk)
 	const struct clk_ops *gate_ops = composite->gate_ops;
 	struct clk *gate = composite->gate;
 
-	return gate_ops->enable(gate);
+	if (gate && gate_ops)
+		return gate_ops->enable(gate);
+	else
+		return -ENOTSUPP;
 }
 
 static int clk_composite_disable(struct clk *clk)
@@ -72,15 +87,12 @@ static int clk_composite_disable(struct clk *clk)
 	const struct clk_ops *gate_ops = composite->gate_ops;
 	struct clk *gate = composite->gate;
 
-	gate_ops->disable(gate);
-
-	return 0;
+	if (gate && gate_ops)
+		return gate_ops->disable(gate);
+	else
+		return -ENOTSUPP;
 }
 
-struct clk_ops clk_composite_ops = {
-	/* This will be set according to clk_register_composite */
-};
-
 struct clk *clk_register_composite(struct device *dev, const char *name,
 				   const char * const *parent_names,
 				   int num_parents, struct clk *mux,
@@ -94,7 +106,9 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 	struct clk *clk;
 	struct clk_composite *composite;
 	int ret;
-	struct clk_ops *composite_ops = &clk_composite_ops;
+
+	if (!num_parents || (num_parents != 1 && !mux))
+		return ERR_PTR(-EINVAL);
 
 	composite = kzalloc(sizeof(*composite), GFP_KERNEL);
 	if (!composite)
@@ -103,8 +117,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 	if (mux && mux_ops) {
 		composite->mux = mux;
 		composite->mux_ops = mux_ops;
-		if (mux_ops->set_parent)
-			composite_ops->set_parent = clk_composite_set_parent;
 		mux->data = (ulong)composite;
 	}
 
@@ -113,11 +125,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 			clk = ERR_PTR(-EINVAL);
 			goto err;
 		}
-		composite_ops->get_rate = clk_composite_recalc_rate;
-
-		/* .set_rate requires either .round_rate or .determine_rate */
-		if (rate_ops->set_rate)
-			composite_ops->set_rate = clk_composite_set_rate;
 
 		composite->rate = rate;
 		composite->rate_ops = rate_ops;
@@ -132,8 +139,6 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
 
 		composite->gate = gate;
 		composite->gate_ops = gate_ops;
-		composite_ops->enable = clk_composite_enable;
-		composite_ops->disable = clk_composite_disable;
 		gate->data = (ulong)composite;
 	}
 
@@ -160,6 +165,14 @@ err:
 	return clk;
 }
 
+static const struct clk_ops clk_composite_ops = {
+	.set_parent = clk_composite_set_parent,
+	.get_rate = clk_composite_recalc_rate,
+	.set_rate = clk_composite_set_rate,
+	.enable = clk_composite_enable,
+	.disable = clk_composite_disable,
+};
+
 U_BOOT_DRIVER(clk_composite) = {
 	.name	= UBOOT_DM_CLK_COMPOSITE,
 	.id	= UCLASS_CLK,
-- 
2.24.1

[PATCH v2 03/11] riscv: Add headers for asm/global_data.h

[Sean Anderson]

This header depended on bd_t and ulong, but did not include the appropriate
headers.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
 arch/riscv/include/asm/global_data.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
index b74bd7e738..4f0c12b402 100644
--- a/arch/riscv/include/asm/global_data.h
+++ b/arch/riscv/include/asm/global_data.h
@@ -11,6 +11,8 @@
 #define __ASM_GBL_DATA_H
 
 #include <asm/smp.h>
+#include <asm/u-boot.h>
+#include <linux/compiler.h>
 
 /* Architecture-specific global data */
 struct arch_global_data {
-- 
2.24.1

[PATCH v2 04/11] riscv: Add an option to default to RV64I

[Sean Anderson]

This allows 64-bit boards to default to the 64-bit instruction set
without changing the current default of 32-bit.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
 arch/riscv/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 85e15ebffa..9a7b0334c2 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -60,8 +60,12 @@ source "arch/riscv/cpu/generic/Kconfig"
 
 # architecture-specific options below
 
+config ARCH_DEFAULT_RV64I
+	bool
+
 choice
 	prompt "Base ISA"
+	default ARCH_RV64I if ARCH_DEFAULT_RV64I
 	default ARCH_RV32I
 
 config ARCH_RV32I
-- 
2.24.1

[PATCH v2 05/11] riscv: Add option to disable writes to mcounteren

[Sean Anderson]

On the kendryte k210, writes to mcounteren result in an illegal instruction
exception.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
Changes for v2:
 Moved forward in the patch series

 arch/riscv/Kconfig   | 3 +++
 arch/riscv/cpu/cpu.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 9a7b0334c2..4f8c62dcff 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -226,6 +226,9 @@ config XIP
 	  from a NOR flash memory without copying the code to ram.
 	  Say yes here if U-Boot boots from flash directly.
 
+config SYS_RISCV_NOCOUNTER
+	bool "Disable accesses to the mcounteren CSR"
+
 config STACK_SIZE_SHIFT
 	int
 	default 14
diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
index e457f6acbf..df9eae663c 100644
--- a/arch/riscv/cpu/cpu.c
+++ b/arch/riscv/cpu/cpu.c
@@ -89,7 +89,9 @@ int arch_cpu_init_dm(void)
 		 * Enable perf counters for cycle, time,
 		 * and instret counters only
 		 */
+#ifndef CONFIG_SYS_RISCV_NOCOUNTER
 		csr_write(CSR_MCOUNTEREN, GENMASK(2, 0));
+#endif
 
 		/* Disable paging */
 		if (supports_extension('s'))
-- 
2.24.1

[PATCH v2 06/11] riscv: Fix incorrect cpu frequency on RV64

[Sean Anderson]

The riscv_cpu_get_info function does not always zero-out cpu_freq. This can
cause spurious higher frequencies.

Signed-off-by Sean Anderson <seanga2@gmail.com>
---
Changes for v2:
  New.

 drivers/cpu/riscv_cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c
index 28ad0aa30f..1e32bb5678 100644
--- a/drivers/cpu/riscv_cpu.c
+++ b/drivers/cpu/riscv_cpu.c
@@ -29,6 +29,8 @@ static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info)
 {
 	const char *mmu;
 
+	/* Zero out the frequency, in case sizeof(ulong) != sizeof(u32) */
+	info->cpu_freq = 0;
 	dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);
 
 	mmu = dev_read_string(dev, "mmu-type");
-- 
2.24.1

[PATCH v2 07/11] riscv: Add initial Sipeed Maix support

[Sean Anderson]

The Sipeed Maix series is a collection of boards built around the RISC-V
Kendryte K210 processor. This processor contains several peripherals to
accelerate neural network processing and other "ai" tasks. This includes a "KPU"
neural network processor, an audio processor supporting beamforming reception,
and a digital video port supporting capture and output at VGA resolution. Other
peripherals include 8M of sram (accessible with and without caching);
remappable pins, including 40 GPIOs; AES, FFT, and SHA256 accelerators; a DMA
controller; and I2C, I2S, and SPI controllers. Maix peripherals vary, but
include spi flash; on-board usb-serial bridges; ports for cameras, displays, and
sd cards; and ESP32 chips. Currently, only the Sipeed Maix Bit V2.0 (bitm) is
supported, but the boards are fairly similar.

Documentation for Maix boards is located at <http://dl.sipeed.com/MAIX/HDK/>.
Documentation for the Kendryte K210 is located at
<https://kendryte.com/downloads/>. However, hardware details are rather lacking,
so most technical reference has been taken from the standalone sdk located at
<https://github.com/kendryte/kendryte-standalone-sdk>.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
Changes for v2:
  Select CONFIG_SYS_RISCV_NOCOUNTER.
  Imply CONFIG_CLK_K210.
  Remove spurious references to CONFIG_ARCH_K210.
  Remove many configs from defconfig where the defaults were fine.
  Add a few "not set" lines to suppress unneeded defaults.
  Reduce pre-reloc malloc space, now that clocks initialization happens
  later.
  
 arch/riscv/Kconfig                 |  4 ++
 board/sipeed/maix/Kconfig          | 41 +++++++++++++
 board/sipeed/maix/MAINTAINERS      | 13 +++++
 board/sipeed/maix/Makefile         |  5 ++
 board/sipeed/maix/maix.c           |  9 +++
 configs/sipeed_maix_bitm_defconfig | 93 ++++++++++++++++++++++++++++++
 include/configs/sipeed-maix.h      | 19 ++++++
 7 files changed, 184 insertions(+)
 create mode 100644 board/sipeed/maix/Kconfig
 create mode 100644 board/sipeed/maix/MAINTAINERS
 create mode 100644 board/sipeed/maix/Makefile
 create mode 100644 board/sipeed/maix/maix.c
 create mode 100644 configs/sipeed_maix_bitm_defconfig
 create mode 100644 include/configs/sipeed-maix.h

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 4f8c62dcff..4c62b8dd77 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -20,6 +20,9 @@ config TARGET_QEMU_VIRT
 config TARGET_SIFIVE_FU540
 	bool "Support SiFive FU540 Board"
 
+config TARGET_SIPEED_MAIX
+	bool "Support Sipeed Maix Board"
+
 endchoice
 
 config SYS_ICACHE_OFF
@@ -53,6 +56,7 @@ source "board/AndesTech/ax25-ae350/Kconfig"
 source "board/emulation/qemu-riscv/Kconfig"
 source "board/microchip/mpfs_icicle/Kconfig"
 source "board/sifive/fu540/Kconfig"
+source "board/sipeed/maix/Kconfig"
 
 # platform-specific options below
 source "arch/riscv/cpu/ax25/Kconfig"
diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
new file mode 100644
index 0000000000..9259eb34aa
--- /dev/null
+++ b/board/sipeed/maix/Kconfig
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+
+if TARGET_SIPEED_MAIX
+
+config SYS_BOARD
+	default "maix"
+
+config SYS_VENDOR
+	default "sipeed"
+
+config SYS_CPU
+	default "generic"
+
+config SYS_CONFIG_NAME
+	default "sipeed-maix"
+
+config SYS_TEXT_BASE
+	default 0x80000000
+
+config NR_CPUS
+	default 2
+
+config NR_DRAM_BANKS
+	default 2
+
+config BOARD_SPECIFIC_OPTIONS
+	def_bool y
+	select GENERIC_RISCV
+	select DM_SERIAL
+	select SIFIVE_SERIAL
+	select ARCH_DEFAULT_RV64I
+	select ENV_IS_NOWHERE
+	select SYS_RISCV_NOCOUNTER
+	imply SIFIVE_CLINT
+	imply SPI
+	imply DM_GPIO
+	imply CMD_GPIO
+	imply SYS_NS16550
+	imply SYS_MALLOC_F
+endif
diff --git a/board/sipeed/maix/MAINTAINERS b/board/sipeed/maix/MAINTAINERS
new file mode 100644
index 0000000000..217de45970
--- /dev/null
+++ b/board/sipeed/maix/MAINTAINERS
@@ -0,0 +1,13 @@
+Sipeed Maix BOARD
+M:	Sean Anderson <seanga2@gmail.com>
+S:	Maintained
+F:	arch/riscv/dts/k210.dtsi
+F:	arch/riscv/dts/k210-maix-bit.dts
+F:	arch/riscv/include/asm/k210_sysctl.h
+F:	arch/riscv/lib/k210_sysctl.c
+F:	board/sipeed/maix/
+F:	configs/sipeed_maix_defconfig
+F:	drivers/clk/kendryte/
+F:	include/configs/sipeed-maix.h
+F:	include/dt-bindings/clock/k210-sysctl.h
+F:	include/dt-bindings/reset/k210-sysctl.h
diff --git a/board/sipeed/maix/Makefile b/board/sipeed/maix/Makefile
new file mode 100644
index 0000000000..4acff5b31e
--- /dev/null
+++ b/board/sipeed/maix/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright (c) 2019 Western Digital Corporation or its affiliates.
+
+obj-y += maix.o
diff --git a/board/sipeed/maix/maix.c b/board/sipeed/maix/maix.c
new file mode 100644
index 0000000000..f8e773acf7
--- /dev/null
+++ b/board/sipeed/maix/maix.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+
+int board_init(void)
+{
+	return 0;
+}
diff --git a/configs/sipeed_maix_bitm_defconfig b/configs/sipeed_maix_bitm_defconfig
new file mode 100644
index 0000000000..f062cc8c58
--- /dev/null
+++ b/configs/sipeed_maix_bitm_defconfig
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+CONFIG_CREATE_ARCH_SYMLINK=y
+CONFIG_RISCV=y
+CONFIG_SYS_ARCH="riscv"
+CONFIG_SYS_CPU="generic"
+CONFIG_SYS_VENDOR="sipeed"
+CONFIG_SYS_BOARD="maix"
+CONFIG_SYS_CONFIG_NAME="sipeed-maix"
+CONFIG_SPL_LDSCRIPT="arch/riscv/cpu/u-boot-spl.lds"
+CONFIG_SYS_TEXT_BASE=0x80000000
+CONFIG_SYS_MALLOC_F_LEN=0x1000
+CONFIG_BOARD_SPECIFIC_OPTIONS=y
+CONFIG_NR_DRAM_BANKS=2
+CONFIG_BOOTSTAGE_STASH_ADDR=0
+CONFIG_64BIT=y
+CONFIG_TARGET_SIPEED_MAIX=y
+CONFIG_NR_CPUS=2
+CONFIG_GENERIC_RISCV=y
+CONFIG_ARCH_DEFAULT_RV64I=y
+CONFIG_ARCH_RV64I=y
+CONFIG_CMODEL_MEDLOW=y
+CONFIG_RISCV_MMODE=y
+CONFIG_RISCV_ISA_C=y
+CONFIG_RISCV_ISA_A=y
+CONFIG_SIFIVE_CLINT=y
+CONFIG_K210_SYSCTL=y
+CONFIG_SYS_RISCV_NOCOUNTER=y
+CONFIG_ARCH_K210=y
+CONFIG_CC_OPTIMIZE_FOR_SIZE=y
+CONFIG_SYS_MALLOC_F=y
+CONFIG_PHYS_64BIT=y
+# CONFIG_ANDROID_BOOT_IMAGE is not set
+# CONFIG_FIT is not set
+# CONFIG_LEGACY_IMAGE_FORMAT is not set
+CONFIG_LOG=y
+CONFIG_ARCH_EARLY_INIT_R=y
+CONFIG_CMDLINE=y
+CONFIG_CMD_ENV_EXISTS=y
+CONFIG_CMD_FLASH=y
+CONFIG_CMD_SF=y
+CONFIG_CMD_LOG=y
+# CONFIG_AUTOBOOT is not set
+# CONFIG_CMD_BDI is not set
+# CONFIG_CMD_CONSOLE is not set
+# CONFIG_CMD_BOOTD is not set
+# CONFIG_CMD_BOOTM is not set
+# CONFIG_CMD_BOOTZ is not set
+CONFIG_CMD_BOOTI=y
+CONFIG_BOOTM_LINUX=y
+# CONFIG_CMD_ELF is not set
+CONFIG_CMD_FDT=y
+CONFIG_SUPPORT_OF_CONTROL=y
+CONFIG_DTC=y
+CONFIG_OF_CONTROL=y
+CONFIG_OF_SEPARATE=y
+CONFIG_ENV_IS_NOWHERE=y
+# CONFIG_NET is not set
+CONFIG_DM=y
+CONFIG_REGMAP=y
+CONFIG_SYSCON=y
+CONFIG_SIMPLE_BUS=y
+CONFIG_OF_TRANSLATE=y
+CONFIG_CLK=y
+CONFIG_CLK_CCF=y
+CONFIG_CLK_COMPOSITE_CCF=y
+CONFIG_CPU=y
+CONFIG_CPU_RISCV=y
+CONFIG_MMC=y
+# CONFIG_INPUT is not set
+CONFIG_DM_MMC=y
+CONFIG_MMC_SPI=y
+CONFIG_MMC_QUIRKS=y
+CONFIG_MTD=y
+CONFIG_DM_MTD=y
+CONFIG_DM_SPI_FLASH=y
+CONFIG_SPI_FLASH=y
+CONFIG_SPI_FLASH_GIGADEVICE=y
+# CONFIG_DM_ETH is not set
+# CONFIG_PCI is not set
+CONFIG_BAUDRATE=115200
+CONFIG_SERIAL_PRESENT=y
+CONFIG_DM_SERIAL=y
+CONFIG_SIFIVE_SERIAL=y
+CONFIG_SPI=y
+CONFIG_DM_SPI=y
+CONFIG_SPI_MEM=y
+CONFIG_DESIGNWARE_SPI=y
+CONFIG_TIMER=y
+CONFIG_RISCV_TIMER=y
+CONFIG_PANIC_HANG=y
+CONFIG_OF_LIBFDT=y
+# CONFIG_EFI_LOADER is not set
diff --git a/include/configs/sipeed-maix.h b/include/configs/sipeed-maix.h
new file mode 100644
index 0000000000..598f7dfdd0
--- /dev/null
+++ b/include/configs/sipeed-maix.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+
+#ifndef CONFIGS_SIPEED_MAIX_H
+#define CONFIGS_SIPEED_MAIX_H
+
+#include <linux/sizes.h>
+
+#define CONFIG_SYS_LOAD_ADDR 0x80000000
+#define CONFIG_SYS_SDRAM_BASE CONFIG_SYS_LOAD_ADDR
+#define CONFIG_SYS_SDRAM_SIZE SZ_8M
+/* Start just below AI memory */
+#define CONFIG_SYS_INIT_SP_ADDR 0x805FFFFF
+#define CONFIG_SYS_MALLOC_LEN SZ_1K
+#define CONFIG_SYS_CACHELINE_SIZE 64
+
+#endif /* CONFIGS_SIPEED_MAIX_H */
-- 
2.24.1

[PATCH v2 00/11] riscv: Add Sipeed Maix support

[Sean Anderson]

[PATCH v2 08/11] riscv: Add device tree for K210

[Sean Anderson]

The subject for this patch should be

Subject: [PATCH v2 08/11] riscv: Add device tree for K210

[PATCH v2 09/11] riscv: Add K210 sysctl support

[Sean Anderson]

This driver does nothing but load its children for the moment. Should it be
using regmap? I'm not sure how that fits into everything.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
Changes for v2:
  Removed spurious references to mach-k210.
  Don't load pre-relocation.

 arch/riscv/Kconfig                   | 11 +++++++
 arch/riscv/include/asm/k210_sysctl.h | 43 ++++++++++++++++++++++++++++
 arch/riscv/lib/Makefile              |  1 +
 arch/riscv/lib/k210_sysctl.c         | 21 ++++++++++++++
 board/sipeed/maix/Kconfig            |  1 +
 5 files changed, 77 insertions(+)
 create mode 100644 arch/riscv/include/asm/k210_sysctl.h
 create mode 100644 arch/riscv/lib/k210_sysctl.c

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 4c62b8dd77..677cdf5f24 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -196,6 +196,14 @@ config RISCV_RDTIME
 	  standard rdtime instruction. This is the case for S-mode U-Boot, and
 	  is useful for processors that support rdtime in M-mode too.
 
+config K210_SYSCTL
+	bool
+	select SYSCON
+	select SPL_SYSCON if SPL
+	help
+	  The K210 sysctl block holds memory-mapped control and status
+	  registers associated with clocks, resets, power, and dma handshakes.
+
 config SYS_MALLOC_F_LEN
 	default 0x1000
 
@@ -240,4 +248,7 @@ config STACK_SIZE_SHIFT
 config SPL_LDSCRIPT
 	default "arch/riscv/cpu/u-boot-spl.lds"
 
+config ARCH_K210
+	bool "Support Kendryte K210 SOCs"
+
 endmenu
diff --git a/arch/riscv/include/asm/k210_sysctl.h b/arch/riscv/include/asm/k210_sysctl.h
new file mode 100644
index 0000000000..94170f4f31
--- /dev/null
+++ b/arch/riscv/include/asm/k210_sysctl.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+
+#ifndef K210_SYSCTL_H
+#define K210_SYSCTL_H
+
+#include <linux/compiler.h>
+
+/*
+ * sysctl registers
+ * Taken from kendryte-standalone-sdk/lib/drivers/include/sysctl.h
+ */
+struct k210_sysctl {
+	u32 git_id;
+	u32 clk_freq;
+	u32 pll0;
+	u32 pll1;
+	u32 pll2;
+	u32 resv5;
+	u32 pll_lock;
+	u32 rom_error;
+	u32 clk_sel[2];
+	u32 clk_en_cent;
+	u32 clk_en_peri;
+	u32 soft_reset;
+	u32 peri_reset;
+	u32 clk_thr[7];
+	u32 misc;
+	u32 peri;
+	u32 spi_sleep;
+	u32 reset_status;
+	u32 dma_sel0;
+	u32 dma_sel1;
+	u32 power_sel;
+	u32 resv28;
+	u32 resv29;
+	u32 resv30;
+	u32 resv31;
+} __packed;
+
+#endif /* K210_SYSCTL_H */
diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
index c9179a5ff8..4c31e824d9 100644
--- a/arch/riscv/lib/Makefile
+++ b/arch/riscv/lib/Makefile
@@ -14,6 +14,7 @@ ifeq ($(CONFIG_$(SPL_)RISCV_MMODE),y)
 obj-$(CONFIG_SIFIVE_CLINT) += sifive_clint.o
 obj-$(CONFIG_ANDES_PLIC) += andes_plic.o
 obj-$(CONFIG_ANDES_PLMT) += andes_plmt.o
+obj-$(CONFIG_K210_SYSCTL) += k210_sysctl.o
 else
 obj-$(CONFIG_RISCV_RDTIME) += rdtime.o
 obj-$(CONFIG_SBI_IPI) += sbi_ipi.o
diff --git a/arch/riscv/lib/k210_sysctl.c b/arch/riscv/lib/k210_sysctl.c
new file mode 100644
index 0000000000..db2e79c974
--- /dev/null
+++ b/arch/riscv/lib/k210_sysctl.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+#include <asm/k210_sysctl.h>
+
+#include <dm.h>
+
+static const struct udevice_id k210_sysctl_ids[] = {
+	{ .compatible = "kendryte,k210-sysctl", },
+	{ }
+};
+
+U_BOOT_DRIVER(k210_sysctl) = {
+	.name = "k210_sysctl",
+	.id = UCLASS_SYSCON,
+#if !CONFIG_IS_ENABLED(OF_PLATDATA)
+	.bind = dm_scan_fdt_dev,
+#endif
+	.of_match = k210_sysctl_ids,
+};
diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
index efeaddf317..9ffd3aa7aa 100644
--- a/board/sipeed/maix/Kconfig
+++ b/board/sipeed/maix/Kconfig
@@ -36,6 +36,7 @@ config BOARD_SPECIFIC_OPTIONS
 	select ENV_IS_NOWHERE
 	select SYS_RISCV_NOCOUNTER
 	imply SIFIVE_CLINT
+	imply K210_SYSCTL
 	imply SPI
 	imply DM_GPIO
 	imply CMD_GPIO
-- 
2.24.1

[PATCH v2 10/11] riscv: Add K210 pll support

[Sean Anderson]

This pll code is primarily based on the code from the kendryte standalone sdk in
lib/drivers/sysctl.c. k210_pll_calc_params is roughly analogous to the algorithm
used to set the pll frequency, but it has been completely rewritten to be
fixed-point based.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
Changes for v2.
  Rename driver to "k210_clk_pll"
  Add additional in-line documentation on algorithm and PLLs.
  Restrict the range of internal VCO and reference frequencies.
  Don't load driver before relocation.
  Remove spurious references to mach-k210.

 board/sipeed/maix/Kconfig          |   1 +
 configs/sipeed_maix_bitm_defconfig |   1 +
 drivers/clk/Kconfig                |   1 +
 drivers/clk/Makefile               |   1 +
 drivers/clk/kendryte/Kconfig       |   7 +
 drivers/clk/kendryte/Makefile      |   1 +
 drivers/clk/kendryte/pll.c         | 598 +++++++++++++++++++++++++++++
 drivers/clk/kendryte/pll.h         |  38 ++
 8 files changed, 648 insertions(+)
 create mode 100644 drivers/clk/kendryte/Kconfig
 create mode 100644 drivers/clk/kendryte/Makefile
 create mode 100644 drivers/clk/kendryte/pll.c
 create mode 100644 drivers/clk/kendryte/pll.h

diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
index 9ffd3aa7aa..10e492a425 100644
--- a/board/sipeed/maix/Kconfig
+++ b/board/sipeed/maix/Kconfig
@@ -37,6 +37,7 @@ config BOARD_SPECIFIC_OPTIONS
 	select SYS_RISCV_NOCOUNTER
 	imply SIFIVE_CLINT
 	imply K210_SYSCTL
+	imply CLK_K210
 	imply SPI
 	imply DM_GPIO
 	imply CMD_GPIO
diff --git a/configs/sipeed_maix_bitm_defconfig b/configs/sipeed_maix_bitm_defconfig
index ecd69c0873..aeab7db0c3 100644
--- a/configs/sipeed_maix_bitm_defconfig
+++ b/configs/sipeed_maix_bitm_defconfig
@@ -65,6 +65,7 @@ CONFIG_OF_TRANSLATE=y
 CONFIG_CLK=y
 CONFIG_CLK_CCF=y
 CONFIG_CLK_COMPOSITE_CCF=y
+CONFIG_CLK_K210=y
 CONFIG_CPU=y
 CONFIG_CPU_RISCV=y
 CONFIG_MMC=y
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 16d4237f89..af75c7c4cf 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -145,6 +145,7 @@ source "drivers/clk/analogbits/Kconfig"
 source "drivers/clk/at91/Kconfig"
 source "drivers/clk/exynos/Kconfig"
 source "drivers/clk/imx/Kconfig"
+source "drivers/clk/kendryte/Kconfig"
 source "drivers/clk/meson/Kconfig"
 source "drivers/clk/mvebu/Kconfig"
 source "drivers/clk/owl/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 06131edb9f..4f3893f6fc 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_CLK_BCM6345) += clk_bcm6345.o
 obj-$(CONFIG_CLK_BOSTON) += clk_boston.o
 obj-$(CONFIG_CLK_EXYNOS) += exynos/
 obj-$(CONFIG_CLK_HSDK) += clk-hsdk-cgu.o
+obj-$(CONFIG_CLK_K210) += kendryte/
 obj-$(CONFIG_CLK_MPC83XX) += mpc83xx_clk.o
 obj-$(CONFIG_CLK_OWL) += owl/
 obj-$(CONFIG_CLK_RENESAS) += renesas/
diff --git a/drivers/clk/kendryte/Kconfig b/drivers/clk/kendryte/Kconfig
new file mode 100644
index 0000000000..a178d50f5e
--- /dev/null
+++ b/drivers/clk/kendryte/Kconfig
@@ -0,0 +1,7 @@
+config CLK_K210
+	bool "Clock support for Kendryte K210"
+	depends on K210_SYSCTL
+	select CLK
+	select CLK_CCF
+	help
+	  This enables support clock driver for Kendryte K210 platforms.
diff --git a/drivers/clk/kendryte/Makefile b/drivers/clk/kendryte/Makefile
new file mode 100644
index 0000000000..c56d93ea1c
--- /dev/null
+++ b/drivers/clk/kendryte/Makefile
@@ -0,0 +1 @@
+obj-y += pll.o
diff --git a/drivers/clk/kendryte/pll.c b/drivers/clk/kendryte/pll.c
new file mode 100644
index 0000000000..cbb112f5a8
--- /dev/null
+++ b/drivers/clk/kendryte/pll.c
@@ -0,0 +1,598 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+#include "pll.h"
+
+#define LOG_CATEGORY UCLASS_CLK
+#include <asm/io.h>
+/* For DIV_ROUND_DOWN_ULL, defined in linux/kernel.h */
+#include <div64.h>
+#include <dt-bindings/clock/k210-sysctl.h>
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <log.h>
+
+#define CLK_K210_PLL "k210_clk_pll"
+#define to_k210_pll(_clk) container_of(_clk, struct k210_pll, clk)
+
+static int k210_pll_enable(struct clk *clk);
+static int k210_pll_disable(struct clk *clk);
+
+/*
+ * The logical layout of the PLL is approximately the following:
+ *
+ *    +-----------+
+ *    |input clock|
+ *    +-----------+
+ *          |
+ *          v
+ *        +--+
+ *        |/r|
+ *        +--+
+ *          |
+ *          v
+ *  +---------------+
+ *  |reference clock|
+ *  +---------------+
+ *          |
+ *          v
+ * +----------------+
+ * |phase comparator|<--+
+ * +----------------+   |
+ *          |           |
+ *          v           |
+ *        +---+         |
+ *        |VCO|         |
+ *        +---+         |
+ *          |    +--+   |
+ *          +--->|/f|---+
+ *          |    +--+
+ *          v
+ *        +---+
+ *        |/od|
+ *        +---+
+ *          |
+ *          v
+ *       +------+
+ *       |output|
+ *       +------+
+ *
+ * The k210 PLLs have three factors: r, f, and od. Because of the feedback mode,
+ * the effect of the division by f is to multiply the input frequency. The
+ * equation for the output rate is
+ * 	rate = (rate_in * f) / (r * od).
+ * Moving knowns to one side of the equation, we get
+ *	rate / rate_in = f / (r * od)
+ * Rearranging slightly,
+ *	abs_error = abs((rate / rate_in) - (f / (r * od))).
+ * To get relative, error, we divide by the expected ratio
+ *	error = abs((rate / rate_in) - (f / (r * od))) / (rate / rate_in).
+ * Simplifying,
+ *	error = abs(1 - f / (r * od)) / (rate / rate_in)
+ *	error = abs(1 - (f * rate_in) / (r * od * rate))
+ * Using the constants ratio = rate / rate_in and inv_ratio = rate_in / rate,
+ *	error = abs((f * inv_ratio) / (r * od) - 1)
+ * This is the error used in evaluating parameters.
+ *
+ * r and od are four bits each, while f is six bits. Because r and od are
+ * multiplied together, instead of the full 256 values possible if both bits
+ * were used fully, there are only 97 distinct products. Combined with f, there
+ * are 6208 theoretical settings for the PLL. However, most of these settings
+ * can be ruled out immediately because they do not have the correct ratio.
+ *
+ * In addition to the constraint of approximating the desired ratio, parameters
+ * must also keep internal pll frequencies within acceptable ranges. The
+ * reference clock's minimum and maximum frequencies have a ratio of around 128.
+ * This leaves fairly substantial room work work with, especially since the only
+ * affected parameter is r. The VCO's minimum and maximum frequency have a ratio
+ * of 5, which is considerably more restrictive.
+ *
+ * The r and od factors are stored in a table. This is to make it easy to find
+ * the next-largest product. Some products have multiple factorizations, but
+ * only when one factor has at least a 2.5x ratio to the factors of the other
+ * factorization. This is because any smaller ratio would not make a difference
+ * when ensuring the VCO's frequency is within spec.
+ *
+ * Throughout the calculation function, fixed point arithmetic is used. Because
+ * the range of rate and rate_in may be up to 1.75 GHz, or around 2^30, 64-bit
+ * 32.32 fixed-point numbers are used to represent ratios. In general, to
+ * implement division, the numerator is first multiplied by 2^32. This gives a
+ * result where the whole number part is in the upper 32 bits, and the fraction
+ * is in the lower 32 bits.
+ *
+ * In general, rounding is done to the closest integer. This helps find the best
+ * approximation for the ratio. Rounding in one direction (e.g down) could cause
+ * the function to miss a better ratio with one of the parameters increased by
+ * one.
+ */
+
+/*
+ * The factors table was generated with the following python code:
+ *
+ * def p(x, y):
+ *    return (1.0*x/y > 2.5) or (1.0*y/x > 2.5)
+ *
+ * factors = {}
+ * for i in range(1, 17):
+ *    for j in range(1, 17):
+ *       fs = factors.get(i*j) or []
+ *       if fs == [] or all([
+ *             (p(i, x) and p(i, y)) or (p(j, x) and p(j, y))
+ *             for (x, y) in fs]):
+ *          fs.append((i, j))
+ *          factors[i*j] = fs
+ *
+ * for k, l in sorted(factors.items()):
+ *    for v in l:
+ *       print("PACK(%s, %s)," % v)
+ */
+#define PACK(r, od) ((((r - 1) & 0xF) << 4) | ((od - 1) & 0xF))
+#define UNPACK_R(val) (((val >> 4) & 0xF) + 1)
+#define UNPACK_OD(val) ((val & 0xF) + 1)
+static const u8 factors[] = {
+	PACK(1, 1),
+	PACK(1, 2),
+	PACK(1, 3),
+	PACK(1, 4),
+	PACK(1, 5),
+	PACK(1, 6),
+	PACK(1, 7),
+	PACK(1, 8),
+	PACK(1, 9),
+	PACK(3, 3),
+	PACK(1, 10),
+	PACK(1, 11),
+	PACK(1, 12),
+	PACK(3, 4),
+	PACK(1, 13),
+	PACK(1, 14),
+	PACK(1, 15),
+	PACK(3, 5),
+	PACK(1, 16),
+	PACK(4, 4),
+	PACK(2, 9),
+	PACK(2, 10),
+	PACK(3, 7),
+	PACK(2, 11),
+	PACK(2, 12),
+	PACK(5, 5),
+	PACK(2, 13),
+	PACK(3, 9),
+	PACK(2, 14),
+	PACK(2, 15),
+	PACK(2, 16),
+	PACK(3, 11),
+	PACK(5, 7),
+	PACK(3, 12),
+	PACK(3, 13),
+	PACK(4, 10),
+	PACK(3, 14),
+	PACK(4, 11),
+	PACK(3, 15),
+	PACK(3, 16),
+	PACK(7, 7),
+	PACK(5, 10),
+	PACK(4, 13),
+	PACK(6, 9),
+	PACK(5, 11),
+	PACK(4, 14),
+	PACK(4, 15),
+	PACK(7, 9),
+	PACK(4, 16),
+	PACK(5, 13),
+	PACK(6, 11),
+	PACK(5, 14),
+	PACK(6, 12),
+	PACK(5, 15),
+	PACK(7, 11),
+	PACK(6, 13),
+	PACK(5, 16),
+	PACK(9, 9),
+	PACK(6, 14),
+	PACK(8, 11),
+	PACK(6, 15),
+	PACK(7, 13),
+	PACK(6, 16),
+	PACK(7, 14),
+	PACK(9, 11),
+	PACK(10, 10),
+	PACK(8, 13),
+	PACK(7, 15),
+	PACK(9, 12),
+	PACK(10, 11),
+	PACK(7, 16),
+	PACK(9, 13),
+	PACK(8, 15),
+	PACK(11, 11),
+	PACK(9, 14),
+	PACK(8, 16),
+	PACK(10, 13),
+	PACK(11, 12),
+	PACK(9, 15),
+	PACK(10, 14),
+	PACK(11, 13),
+	PACK(9, 16),
+	PACK(10, 15),
+	PACK(11, 14),
+	PACK(12, 13),
+	PACK(10, 16),
+	PACK(11, 15),
+	PACK(12, 14),
+	PACK(13, 13),
+	PACK(11, 16),
+	PACK(12, 15),
+	PACK(13, 14),
+	PACK(12, 16),
+	PACK(13, 15),
+	PACK(14, 14),
+	PACK(13, 16),
+	PACK(14, 15),
+	PACK(14, 16),
+	PACK(15, 15),
+	PACK(15, 16),
+	PACK(16, 16),
+};
+
+struct k210_pll_params {
+	u8 r;
+	u8 f;
+	u8 od;
+};
+
+static int k210_pll_calc_params(u32 rate, u32 rate_in,
+				struct k210_pll_params *best)
+{
+	int i;
+	s64 error, best_error;
+	u64 ratio, inv_ratio; /* fixed point 32.32 ratio of the rates */
+	u64 max_r;
+	u64 r, f, od;
+
+	/*
+	 * Can't go over 1.75 GHz or under 855 kHz due to limitations on the
+	 * reference clock frequency. These are not the same limits as below
+	 * because od can reduce the output frequency by 16.
+	 */
+	if (rate > 1750000000 || rate < 854493)
+		return -EINVAL;
+
+	ratio = DIV_ROUND_CLOSEST_ULL((u64)rate << 32, rate_in);
+	inv_ratio = DIV_ROUND_CLOSEST_ULL((u64)rate_in << 32, rate);
+	/* Can't increase by more than 64 or reduce by more than 256 */
+	if (rate > rate_in && ratio > (64ULL << 32))
+		return -EINVAL;
+	else if (rate <= rate_in && inv_ratio > (256ULL << 32))
+		return -EINVAL;
+
+	/*
+	 * The reference clock (rate_in / r) must stay between 1.75 GHz and 13
+	 * MHz. There is no minimum, since the only way to get a higher input
+	 * clock than 26 MHz is to use a clock generated by a PLL. Because PLLs
+	 * cannot output frequencies greater than 1.75 GHz, the minimum would
+	 * never be greater than one.
+	 */
+	max_r = DIV_ROUND_DOWN_ULL(rate_in, 13671875);
+
+	/* Variables get immediately incremented, so start at -1th iteration */ 
+	i = -1;
+	f = 0;
+	r = 0;
+	od = 0;
+	error = best_error = S64_MAX;
+	/* do-while here so we always try@least one ratio */
+	do {
+		/*
+		 * Whether we swapped r and od while enforcing frequency limits
+		 */
+		bool swapped = false;
+		u64 last_od = od;
+		u64 last_r = r;
+
+		/*
+		 * Try the next largest value for f (or r and od) and
+		 * recalculate the other parameters based on that
+		 */
+		if (rate > rate_in) {
+			/*
+			 * Skip factors of the same product if we already tried
+			 * out that product
+			 */
+			do {
+				i++;
+				r = UNPACK_R(factors[i]);
+				od = UNPACK_OD(factors[i]);
+			} while (i + 1 < ARRAY_SIZE(factors)
+				 && r * od == last_r * last_od);
+			
+			/* Round close */
+			f = (r * od * ratio + BIT(31)) >> 32;
+			if (f > 64)
+				f = 64;
+		} else {
+			u64 tmp = ++f * inv_ratio;
+			bool round_up = !!(tmp & BIT(31));
+			u32 goal = (tmp >> 32) + round_up;
+			u32 err, last_err;
+
+			/* Get the next r/od pair in factors */
+			while (r * od < goal && i + 1 < ARRAY_SIZE(factors)) {
+				i++;
+				r = UNPACK_R(factors[i]);
+				od = UNPACK_OD(factors[i]);
+			}
+
+			/*
+			 * This is a case of double rounding. If we rounded up
+			 * above, we need to round down (in cases of ties) here.
+			 * This prevents off-by-one errors resulting from
+			 * choosing X+2 over X when X.Y rounds up to X+1 and
+			 * there is no r * od = X+1. For the converse, when X.Y
+			 * is rounded down to X, we should choose X+1 over X-1.
+			 */
+			err = abs(r * od - goal);
+			last_err = abs(last_r * last_od - goal);
+			if (last_err < err || (round_up && (last_err = err))) {
+				i--;
+				r = last_r;
+				od = last_od;
+			}
+		}
+
+		/*
+		 * Enforce limits on internal clock frequencies. If we
+		 * aren't in spec, try swapping r and od. If everything is
+		 * in-spec, calculate the relative error.
+		 */
+		while (true) {
+			/*
+			 * Whether the intermediate frequencies are out-of-spec
+			 */
+			bool out_of_spec = false;
+
+			if (r > max_r) {
+				out_of_spec = true;
+			} else {
+				/*
+				 * There is no way to only divide once; we need
+				 * to examine the frequency with and without the
+				 * effect of od.
+				 */
+				u64 vco = DIV_ROUND_CLOSEST_ULL(rate_in * f, r);
+				if (vco > 1750000000 || vco < 350000000)
+					out_of_spec = true;
+			}
+
+			if (out_of_spec) {
+				if (!swapped) {
+					u64 tmp = r;
+					r = od;
+					od = tmp;
+					swapped = true;
+					continue;
+				} else {
+					/*
+					 * Try looking ahead to see if there are
+					 * additional factors for the same
+					 * product.
+					 */
+					if (i + 1 < ARRAY_SIZE(factors)) {
+						u64 new_r, new_od;
+
+						i++;
+						new_r = UNPACK_R(factors[i]);
+						new_od = UNPACK_OD(factors[i]);
+						if (r * od == new_r * new_od) {
+							r = new_r;
+							od = new_od;
+							swapped = false;
+							continue;
+						}
+						i--;
+					}
+					break;
+				}
+			}
+
+			error = DIV_ROUND_CLOSEST_ULL(f * inv_ratio, r * od);
+			/* The lower 16 bits are spurious */
+			error = abs((error - BIT(32))) >> 16;
+
+			if (error < best_error) {
+				best->r = r;
+				best->f = f;
+				best->od = od;
+				best_error = error;
+			}
+			break;
+		}
+	} while (f < 64 && i + 1 < ARRAY_SIZE(factors) && error != 0);
+
+	if (best_error == S64_MAX)
+		return -EINVAL;
+
+	log_debug("best error %lld\n", best_error);
+	return 0;
+}
+
+static ulong k210_pll_set_rate(struct clk *clk, ulong rate)
+{
+	int err;
+	long long rate_in = clk_get_parent_rate(clk);
+	struct k210_pll_params params = {};
+	struct k210_pll *pll = to_k210_pll(clk);
+	u32 reg;
+	
+	if (rate_in < 0)
+		return rate_in;
+
+	log_debug("Calculating parameters with rate=%lu and rate_in=%lld\n",
+		  rate, rate_in);
+	err = k210_pll_calc_params(rate, rate_in, &params);
+	if (err)
+		return err;
+	log_debug("Got r=%u f=%u od=%u\n", params.r, params.f, params.od);
+
+	/*
+	 * Don't use clk_disable as it might not actually disable the pll due to
+	 * refcounting
+	 */
+	err = k210_pll_disable(clk);
+	if (err)
+		return err;
+
+	reg = readl(pll->reg);
+	reg &= ~K210_PLL_CLKR
+	    & ~K210_PLL_CLKF
+	    & ~K210_PLL_CLKOD
+	    & ~K210_PLL_BWADJ;
+	reg |= FIELD_PREP(K210_PLL_CLKR, params.r - 1)
+	    | FIELD_PREP(K210_PLL_CLKF, params.f - 1)
+	    | FIELD_PREP(K210_PLL_CLKOD, params.od - 1)
+	    | FIELD_PREP(K210_PLL_BWADJ, params.f - 1);
+	writel(reg, pll->reg);
+
+	err = k210_pll_enable(clk);
+	if (err)
+		return err;
+
+	return clk_get_rate(clk);
+}
+
+static ulong k210_pll_get_rate(struct clk *clk)
+{
+
+	long long rate_in = clk_get_parent_rate(clk);
+	struct k210_pll *pll = to_k210_pll(clk);
+	u64 r, f, od;
+	u32 reg = readl(pll->reg);
+
+	if (rate_in < 0)
+		return rate_in;
+
+	if (reg & K210_PLL_BYPASS)
+		return rate_in;
+	
+	r = FIELD_GET(K210_PLL_CLKR, reg) + 1;
+	f = FIELD_GET(K210_PLL_CLKF, reg) + 1;
+	od = FIELD_GET(K210_PLL_CLKOD, reg) + 1;
+
+	return DIV_ROUND_DOWN_ULL(((u64)rate_in) * f, r * od);
+}
+
+/* Check if the PLL is locked */
+static int k210_pll_locked(struct k210_pll *pll)
+{
+	u32 reg = readl(pll->lock);
+
+	return (reg & pll->lock_mask) == pll->lock_mask;
+}
+
+/*
+ * Wait for the PLL to be locked. If the PLL is not locked, try clearing the
+ * slip before retrying
+ */
+static void k210_pll_waitfor_lock(struct k210_pll *pll)
+{
+	while (!k210_pll_locked(pll)) {
+		u32 reg = readl(pll->lock);
+
+		reg |= BIT(pll->shift + K210_PLL_CLEAR_SLIP);
+		writel(reg, pll->lock);
+		udelay(1);
+	}
+}
+
+/* Adapted from sysctl_pll_enable */
+static int k210_pll_enable(struct clk *clk)
+{
+	struct k210_pll *pll = to_k210_pll(clk);
+	u32 reg = readl(pll->reg);
+
+	reg &= ~K210_PLL_BYPASS;
+	writel(reg, pll->reg);
+
+	reg |= K210_PLL_PWRD;
+	writel(reg, pll->reg);
+
+	/* Ensure reset is low before asserting it */
+	reg &= ~K210_PLL_RESET;
+	writel(reg, pll->reg);
+	reg |= K210_PLL_RESET;
+	writel(reg, pll->reg);
+	/* FIXME: this doesn't really have to be a whole microsecond */
+	udelay(1);
+	reg &= ~K210_PLL_RESET;
+	writel(reg, pll->reg);
+
+	k210_pll_waitfor_lock(pll);
+	return 0;
+}
+
+static int k210_pll_disable(struct clk *clk)
+{
+	struct k210_pll *pll = to_k210_pll(clk);
+	u32 reg = readl(pll->reg);
+
+	/*
+	 * Bypassing before powering off is important so child clocks don't stop
+	 * working. This is especially important for pll0, the indirect parent
+	 * of the cpu clock.
+	 */
+	reg |= K210_PLL_BYPASS;
+	writel(reg, pll->reg);
+
+	reg &= ~K210_PLL_PWRD;
+	writel(reg, pll->reg);
+	return 0;
+}
+
+const struct clk_ops k210_pll_ops = {
+	.get_rate = k210_pll_get_rate,
+	.set_rate = k210_pll_set_rate,
+	.enable = k210_pll_enable,
+	.disable = k210_pll_disable,
+};
+
+struct k210_pll *k210_clk_comp_pll(void __iomem *reg, void __iomem *lock,
+				       u8 shift, u8 width)
+{
+	struct k210_pll *pll;
+
+	
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return pll;
+	pll->reg = reg;
+	pll->lock = lock;
+	pll->shift = shift;
+	pll->lock_mask = GENMASK(shift + width, shift);
+	return pll;
+}
+
+struct clk *k210_clk_pll(const char *name, const char *parent_name,
+			 void __iomem *reg, void __iomem *lock, u8 shift,
+			 u8 width)
+{
+	int err;
+	struct k210_pll *pll;
+
+	pll = k210_clk_comp_pll(reg, lock, shift, width);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	err = clk_register(&pll->clk, CLK_K210_PLL, name, parent_name);
+	if (err) {
+		kfree(pll);
+		return ERR_PTR(err);
+	}
+	return &pll->clk;
+}
+
+U_BOOT_DRIVER(k210_pll) = {
+	.name	= CLK_K210_PLL,
+	.id	= UCLASS_CLK,
+	.ops	= &k210_pll_ops,
+};
diff --git a/drivers/clk/kendryte/pll.h b/drivers/clk/kendryte/pll.h
new file mode 100644
index 0000000000..6a8734a295
--- /dev/null
+++ b/drivers/clk/kendryte/pll.h
@@ -0,0 +1,38 @@
+#ifndef K210_PLL_H
+#define K210_PLL_H
+
+#include <clk.h>
+
+#define K210_PLL_CLKR GENMASK(3, 0)
+#define K210_PLL_CLKF GENMASK(9, 4)
+#define K210_PLL_CLKOD GENMASK(13, 10)
+#define K210_PLL_BWADJ GENMASK(19, 14)
+#define K210_PLL_RESET BIT(20)
+#define K210_PLL_PWRD BIT(21)
+#define K210_PLL_INTFB BIT(22)
+#define K210_PLL_BYPASS BIT(23)
+#define K210_PLL_TEST BIT(24)
+#define K210_PLL_EN BIT(25)
+#define K210_PLL_TEST_EN BIT(26)
+
+#define K210_PLL_LOCK 0
+#define K210_PLL_CLEAR_SLIP 2
+#define K210_PLL_TEST_OUT 3
+
+struct k210_pll {
+	struct clk clk;
+	void __iomem *reg; /* Base PLL register */
+	void __iomem *lock; /* Common PLL lock register */
+	u8 shift; /* Offset of bits in lock register */
+	u8 lock_mask; /* Mask of lock bits to test against, pre-shifted */
+};
+
+extern const struct clk_ops k210_pll_ops;
+
+struct k210_pll *k210_clk_comp_pll(void __iomem *reg, void __iomem *lock,
+				   u8 shift, u8 width);
+struct clk *k210_clk_pll(const char *name, const char *parent_name,
+			 void __iomem *reg, void __iomem *lock, u8 shift,
+			 u8 width);
+
+#endif /* K210_PLL_H */
-- 
2.24.1

[PATCH v2 11/11] riscv: Add K210 clock support

[Sean Anderson]

Due to the large number of clocks, I decided to use the CCF. The overall
structure is modeled after the imx code. A common pattern is to create a
composite clock composed of several component clocks. For these component
clocks, the clk_register_* functions are not used, since they will be registered
as part of the composite clock. To create these component clocks, several helper
k210_clk_comp_* functions are used. This functionality seems like it would be
useful to other drivers also creating composite clocks, so perhaps some general
versions should be created. I am not particularly attached to the naming
convention, suggestions are welcome.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
Changes for v2:
  Add clk.o to obj-y.
  Don't probe before relocation.

 arch/riscv/Kconfig            |   3 -
 board/sipeed/maix/Kconfig     |   3 +
 drivers/clk/kendryte/Makefile |   2 +-
 drivers/clk/kendryte/clk.c    | 390 ++++++++++++++++++++++++++++++++++
 drivers/clk/kendryte/clk.h    |  27 +++
 5 files changed, 421 insertions(+), 4 deletions(-)
 create mode 100644 drivers/clk/kendryte/clk.c
 create mode 100644 drivers/clk/kendryte/clk.h

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 677cdf5f24..6b05ef72d0 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -248,7 +248,4 @@ config STACK_SIZE_SHIFT
 config SPL_LDSCRIPT
 	default "arch/riscv/cpu/u-boot-spl.lds"
 
-config ARCH_K210
-	bool "Support Kendryte K210 SOCs"
-
 endmenu
diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
index 10e492a425..c456c47fb2 100644
--- a/board/sipeed/maix/Kconfig
+++ b/board/sipeed/maix/Kconfig
@@ -27,6 +27,9 @@ config NR_CPUS
 config NR_DRAM_BANKS
 	default 2
 
+config SYS_MALLOC_F_LEN
+	default 0x8000
+
 config BOARD_SPECIFIC_OPTIONS
 	def_bool y
 	select GENERIC_RISCV
diff --git a/drivers/clk/kendryte/Makefile b/drivers/clk/kendryte/Makefile
index c56d93ea1c..d26bce954f 100644
--- a/drivers/clk/kendryte/Makefile
+++ b/drivers/clk/kendryte/Makefile
@@ -1 +1 @@
-obj-y += pll.o
+obj-y += clk.o pll.o
diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c
new file mode 100644
index 0000000000..c6405a329c
--- /dev/null
+++ b/drivers/clk/kendryte/clk.c
@@ -0,0 +1,390 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+#include "clk.h"
+
+#include <asm/io.h>
+#include <asm/k210_sysctl.h>
+#include <dt-bindings/clock/k210-sysctl.h>
+#include <dm.h>
+#include <log.h>
+#include <mapmem.h>
+
+#include "pll.h"
+
+static ulong k210_clk_get_rate(struct clk *clk)
+{
+	struct clk *c;
+	int err = clk_get_by_id(clk->id, &c);
+
+	if (err)
+		return err;
+	return clk_get_rate(c);
+}
+
+static ulong k210_clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	struct clk *c;
+	int err = clk_get_by_id(clk->id, &c);
+
+	if (err)
+		return err;
+	return clk_set_rate(c, rate);
+}
+
+static int k210_clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	struct clk *c, *p;
+	int err = clk_get_by_id(clk->id, &c);
+
+	if (err)
+		return err;
+	
+	err = clk_get_by_id(parent->id, &p);
+	if (err)
+		return err;
+
+	return clk_set_parent(c, p);
+}
+
+static int k210_clk_endisable(struct clk *clk, bool enable)
+{
+	struct clk *c;
+	int err = clk_get_by_id(clk->id, &c);
+
+	if (err)
+		return err;
+	return enable ? clk_enable(c) : clk_disable(c);
+}
+
+static int k210_clk_enable(struct clk *clk)
+{
+	return k210_clk_endisable(clk, true);
+}
+
+static int k210_clk_disable(struct clk *clk)
+{
+	return k210_clk_endisable(clk, false);
+}
+
+static const struct clk_ops k210_clk_ops = {
+	.set_rate = k210_clk_set_rate,
+	.get_rate = k210_clk_get_rate,
+	.set_parent = k210_clk_set_parent,
+	.enable = k210_clk_enable,
+	.disable = k210_clk_disable,
+};
+
+/* The first clock is in0, which is filled in by k210_clk_probe */
+static const char *generic_sels[] = { NULL, "pll0", };
+static const char *aclk_sels[] = { "in0_half", "pll0_half", };
+static const char *pll2_sels[] = { NULL, "pll0", "pll1", };
+
+static struct clk_divider *k210_clk_comp_div_flags(void __iomem *reg, u8 shift,
+						   u8 width, u8 flags)
+{
+	struct clk_divider *div;
+
+	div = kzalloc(sizeof(*div), GFP_KERNEL);
+	if (!div)
+		return div;
+	div->reg = reg;
+	div->shift = shift;
+	div->width = width;
+	div->flags = flags;
+	return div;
+}
+
+static inline struct clk_divider *k210_clk_comp_div(void __iomem *reg, u8 shift,
+						    u8 width)
+{
+	return k210_clk_comp_div_flags(reg, shift, width, 0);
+}
+
+static struct clk_gate *k210_clk_comp_gate(void __iomem *reg, u8 bit_idx)
+{
+	struct clk_gate *gate;
+
+	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
+	if (!gate)
+		return gate;
+	gate->reg = reg;
+	gate->bit_idx = bit_idx;
+	return gate;
+}
+
+static struct clk_mux *k210_clk_comp_mux(const char *parent_names[],
+				         u8 num_parents, void __iomem *reg,
+				         u8 shift, u8 width)
+{
+	struct clk_mux *mux;
+
+	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
+	if (!mux)
+		return mux;
+	mux->reg = reg;
+	mux->mask = BIT(width) - 1;
+	mux->shift = shift;
+	mux->parent_names = parent_names;
+	mux->num_parents = num_parents;
+	return mux;
+}
+
+static struct clk *k210_clk_comp_nomux(const char *name, const char *parent,
+					 struct clk_divider *div,
+					 struct clk_gate *gate)
+{
+	if (!div || !gate) {
+		kfree(div);
+		kfree(gate);
+		return ERR_PTR(-ENOMEM);
+	}
+	return clk_register_composite(NULL, name, &parent, 1,
+				      NULL, NULL,
+				      &div->clk, &clk_divider_ops,
+				      &gate->clk, &clk_gate_ops, 0);
+}
+
+static struct clk *k210_clk_comp(const char *name, struct clk_divider *div,
+				 struct clk_gate *gate, struct clk_mux *mux)
+{
+	if (!div || !gate || !mux) {
+		kfree(div);
+		kfree(gate);
+		kfree(mux);
+		return ERR_PTR(-ENOMEM);
+	}
+	return clk_register_composite(NULL, name, generic_sels,
+				      ARRAY_SIZE(generic_sels),
+				      &mux->clk, &clk_mux_ops,
+				      &div->clk, &clk_divider_ops,
+				      &gate->clk, &clk_gate_ops, 0);
+}
+
+static int k210_clk_probe(struct udevice *dev)
+{
+	int err;
+	const char *in0;
+	struct clk in0_clk;
+	struct clk_divider *div;
+	struct clk_gate *gate;
+	struct clk_mux *mux;
+	struct k210_pll *pll;
+	struct k210_sysctl *sysctl;
+
+	sysctl = dev_read_addr_ptr(dev_get_parent(dev));
+	if (!sysctl)
+		return -EINVAL;
+
+	err = clk_get_by_index(dev, 0, &in0_clk);
+	if (err)
+		goto cleanup_sysctl;
+	in0 = in0_clk.dev->name;
+	generic_sels[0] = in0;
+	pll2_sels[0] = in0;
+
+	/* PLLs */
+	clk_dm(K210_CLK_PLL0, k210_clk_pll("pll0", in0, &sysctl->pll0,
+					   &sysctl->pll_lock, 0, 2)); 
+	clk_dm(K210_CLK_PLL1, k210_clk_pll("pll1", in0, &sysctl->pll1,
+					   &sysctl->pll_lock, 8, 1));
+	/* PLL2 is muxed, so set up a composite clock */
+	mux = k210_clk_comp_mux(pll2_sels, ARRAY_SIZE(pll2_sels),
+				&sysctl->pll2, 26, 2);
+	pll = k210_clk_comp_pll(&sysctl->pll2, &sysctl->pll_lock, 16, 1);
+	if (!mux || !pll) {
+		kfree(mux);
+		kfree(pll);
+	} else {
+		clk_dm(K210_CLK_PLL0,
+		       clk_register_composite(NULL, "pll2", pll2_sels,
+					      ARRAY_SIZE(pll2_sels),
+					      &mux->clk, &clk_mux_ops,
+					      &pll->clk, &k210_pll_ops,
+					      &pll->clk, &k210_pll_ops, 0));
+	}
+
+	/* Half-frequency clocks for "even" dividers */
+	k210_clk_half("in0_half", in0);
+	k210_clk_half("pll0_half", "pll0");
+	k210_clk_half("pll2_half", "pll2");
+
+	/* Muxed clocks */
+	div = k210_clk_comp_div_flags(&sysctl->clk_sel[0], 1, 2,
+				      CLK_DIVIDER_POWER_OF_TWO);
+	/* ACLK has no gate */
+	mux = k210_clk_comp_mux(aclk_sels, ARRAY_SIZE(generic_sels),
+				&sysctl->clk_sel[0], 0, 1);
+	if (!div || !mux) {
+		kfree(div);
+		kfree(mux);
+	} else {
+		clk_dm(K210_CLK_ACLK,
+		       clk_register_composite(NULL, "aclk", aclk_sels,
+					      ARRAY_SIZE(aclk_sels),
+					      &mux->clk, &clk_mux_ops,
+					      &div->clk, &clk_divider_ops,
+					      NULL, NULL, 0));
+	}
+
+	div = k210_clk_comp_div(&sysctl->clk_sel[0], 1, 2);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 9);
+	mux = k210_clk_comp_mux(generic_sels, ARRAY_SIZE(generic_sels),
+				&sysctl->clk_sel[0], 12, 1);
+	clk_dm(K210_CLK_SPI3, k210_clk_comp("spi3", div, gate, mux));
+	
+	div = k210_clk_comp_div(&sysctl->clk_thr[2], 8, 0);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 21);
+	mux = k210_clk_comp_mux(generic_sels, ARRAY_SIZE(generic_sels),
+				&sysctl->clk_sel[0], 12, 1);
+	clk_dm(K210_CLK_TIMER0, k210_clk_comp("timer0", div, gate, mux));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[2], 8, 8);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 22);
+	mux = k210_clk_comp_mux(generic_sels, ARRAY_SIZE(generic_sels),
+				&sysctl->clk_sel[0], 12, 1);
+	clk_dm(K210_CLK_TIMER1, k210_clk_comp("timer1", div, gate, mux));
+	
+	div = k210_clk_comp_div(&sysctl->clk_thr[2], 8, 16);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 23);
+	mux = k210_clk_comp_mux(generic_sels, ARRAY_SIZE(generic_sels),
+				&sysctl->clk_sel[0], 12, 1);
+	clk_dm(K210_CLK_TIMER2, k210_clk_comp("timer2", div, gate, mux));
+
+
+	/* Dividing clocks, no mux */
+	div = k210_clk_comp_div(&sysctl->clk_thr[0], 0, 4);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_cent, 1);
+	clk_dm(K210_CLK_SRAM0, k210_clk_comp_nomux("sram0", "aclk", div, gate));
+
+	div = k210_clk_comp_div( &sysctl->clk_thr[0], 4, 4);
+	gate = k210_clk_comp_gate( &sysctl->clk_en_cent, 2);
+	clk_dm(K210_CLK_SRAM1, k210_clk_comp_nomux("sram1", "aclk", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[0], 16, 4);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 0);
+	clk_dm(K210_CLK_ROM, k210_clk_comp_nomux("rom", "aclk", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[0], 12, 4);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 3);
+	clk_dm(K210_CLK_DVP, k210_clk_comp_nomux("dvp", "aclk", div, gate));
+
+	/*
+	 * XXX: the next three clocks may be using an even divider
+	 * c.f. <https://github.com/kendryte/kendryte-standalone-sdk/issues/99>
+	 */
+	div = k210_clk_comp_div(&sysctl->clk_sel[0], 3, 3);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_cent, 3);
+	clk_dm(K210_CLK_APB0, k210_clk_comp_nomux("apb0", "aclk", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_sel[0], 6, 3);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_cent, 4);
+	clk_dm(K210_CLK_APB1, k210_clk_comp_nomux("apb1", "aclk", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_sel[0], 9, 3);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_cent, 5);
+	clk_dm(K210_CLK_APB1, k210_clk_comp_nomux("apb2", "aclk", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[0], 8, 4);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 2);
+	clk_dm(K210_CLK_APB1, k210_clk_comp_nomux("ai", "pll1", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[3], 0, 16);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 10);
+	clk_dm(K210_CLK_I2S0,
+	       k210_clk_comp_nomux("i2s0", "pll2_half", div, gate));
+	
+	div = k210_clk_comp_div(&sysctl->clk_thr[3], 16, 16);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 11);
+	clk_dm(K210_CLK_I2S1,
+	       k210_clk_comp_nomux("i2s1", "pll2_half", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[4], 0, 16);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 12);
+	clk_dm(K210_CLK_I2S2,
+	       k210_clk_comp_nomux("i2s2", "pll2_half", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[6], 0, 8);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 24);
+	clk_dm(K210_CLK_WDT0,
+	       k210_clk_comp_nomux("wdt0", "in0_half", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[6], 8, 8);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 25);
+	clk_dm(K210_CLK_WDT1,
+	       k210_clk_comp_nomux("wdt1", "in0_half", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[1], 0, 8);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 6);
+	clk_dm(K210_CLK_SPI0,
+	       k210_clk_comp_nomux("spi0", "pll0_half", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[1], 8, 8);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 7);
+	clk_dm(K210_CLK_SPI1,
+	       k210_clk_comp_nomux("spi1", "pll0_half", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[1], 16, 8);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 8);
+	clk_dm(K210_CLK_SPI2,
+	       k210_clk_comp_nomux("spi2", "pll0_half", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[5], 8, 8);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 13);
+	clk_dm(K210_CLK_SPI2,
+	       k210_clk_comp_nomux("i2c0", "pll0_half", div, gate));
+	
+	div = k210_clk_comp_div(&sysctl->clk_thr[5], 16, 8);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 14);
+	clk_dm(K210_CLK_SPI2,
+	       k210_clk_comp_nomux("i2c1", "pll0_half", div, gate));
+
+	div = k210_clk_comp_div(&sysctl->clk_thr[5], 24, 8);
+	gate = k210_clk_comp_gate(&sysctl->clk_en_peri, 15);
+	clk_dm(K210_CLK_SPI2,
+	       k210_clk_comp_nomux("i2c2", "pll0_half", div, gate));
+
+	/* Gated clocks */
+	clk_dm(K210_CLK_CPU,
+	       k210_clk_gate("cpu", "aclk", &sysctl->clk_en_cent, 0));
+	clk_dm(K210_CLK_DMA,
+	       k210_clk_gate("dma", "aclk", &sysctl->clk_en_peri, 1));
+	clk_dm(K210_CLK_FFT,
+	       k210_clk_gate("fft", "aclk", &sysctl->clk_en_peri, 4));
+	clk_dm(K210_CLK_GPIO,
+	       k210_clk_gate("gpio", "apb0", &sysctl->clk_en_peri, 5));
+	clk_dm(K210_CLK_UART1,
+	       k210_clk_gate("uart1", "apb0", &sysctl->clk_en_peri, 16));
+	clk_dm(K210_CLK_UART2,
+	       k210_clk_gate("uart2", "apb0", &sysctl->clk_en_peri, 17));
+	clk_dm(K210_CLK_UART3,
+	       k210_clk_gate("uart3", "apb0", &sysctl->clk_en_peri, 18));
+	clk_dm(K210_CLK_FPIOA,
+	       k210_clk_gate("fpioa", "apb0", &sysctl->clk_en_peri, 20));
+	clk_dm(K210_CLK_SHA,
+	       k210_clk_gate("sha", "apb0", &sysctl->clk_en_peri, 26));
+	clk_dm(K210_CLK_AES,
+	       k210_clk_gate("aes", "apb1", &sysctl->clk_en_peri, 19));
+	clk_dm(K210_CLK_OTP,
+	       k210_clk_gate("otp", "apb1", &sysctl->clk_en_peri, 27));
+	clk_dm(K210_CLK_RTC,
+	       k210_clk_gate("rtc", in0, &sysctl->clk_en_peri, 29));
+
+cleanup_sysctl:
+	unmap_sysmem(sysctl);
+	return err;
+}
+
+static const struct udevice_id k210_clk_ids[] = {
+	{ .compatible = "kendryte,k210-clk" },
+	{ },
+};
+
+U_BOOT_DRIVER(k210_clk) = {
+	.name = "k210_clk",
+	.id = UCLASS_CLK,
+	.of_match = k210_clk_ids,
+	.ops = &k210_clk_ops,
+	.probe = k210_clk_probe,
+};
diff --git a/drivers/clk/kendryte/clk.h b/drivers/clk/kendryte/clk.h
new file mode 100644
index 0000000000..17d0f5de1b
--- /dev/null
+++ b/drivers/clk/kendryte/clk.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+
+#ifndef K210_CLK_H
+#define K210_CLK_H
+
+#define LOG_CATEGORY UCLASS_CLK
+#include <linux/types.h>
+#include <linux/clk-provider.h>
+
+static inline struct clk *k210_clk_gate(const char *name,
+					const char *parent_name,
+					void __iomem *reg, u8 bit_idx)
+{
+	return clk_register_gate(NULL, name, parent_name, 0, reg, bit_idx, 0,
+				 NULL);
+}
+
+static inline struct clk *k210_clk_half(const char *name,
+				        const char *parent_name)
+{
+	return clk_register_fixed_factor(NULL, name, parent_name, 0, 1, 2);
+}
+
+#endif /* K210_CLK_H */
-- 
2.24.1

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Rick Chen]

Hi Sean

> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Sean Anderson
> Sent: Thursday, January 16, 2020 6:48 AM
> To: U-Boot Mailing List
> Subject: [PATCH v2 01/11] clk: Always use the supplied struct clk
>
> CCF clocks should always use the struct clock passed to their methods for extracting the driver-specific clock information struct. Previously, many functions would use the clk->dev->priv if the device was bound. This could cause problems with composite clocks. The individual clocks in a composite clock did not have the ->dev field filled in. This was fine, because the device-specific clock information would be used. However, since there was no ->dev, there was no way to get the parent clock. This caused the recalc_rate method of the CCF divider clock to fail. One option would be to use the clk->priv field to get the composite clock and from there get the appropriate parent device. However, this would tie the implementation to the composite clock. In general, different devices should not rely on the contents of ->priv from another device.
>
> The simple solution to this problem is to just always use the supplied struct clock. The composite clock now fills in the ->dev pointer of its child clocks.
> This allows child clocks to make calls like clk_get_parent() without issue.
>
> imx avoided the above problem by using a custom get_rate function with composite clocks.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>  drivers/clk/clk-composite.c    |  8 ++++++++
>  drivers/clk/clk-divider.c      |  6 ++----
>  drivers/clk/clk-fixed-factor.c |  3 +--
>  drivers/clk/clk-gate.c         |  6 ++----
>  drivers/clk/clk-mux.c          | 12 ++++--------
>  drivers/clk/imx/clk-gate2.c    |  4 ++--
>  6 files changed, 19 insertions(+), 20 deletions(-)

This patch is clock relative patch, if it is not a necessary patch for
Sipeed Maix support.
Please remove patch 1/11 and 2/11, and send them in another patch-sets.

Rick,
Thanks

>
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c index a5626c33d1..d0f273d47f 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -145,6 +145,14 @@ struct clk *clk_register_composite(struct device *dev, const char *name,
>                 goto err;
>         }
>
> +       if (composite->mux)
> +               composite->mux->dev = clk->dev;
> +       if (composite->rate)
> +               composite->rate->dev = clk->dev;
> +       if (composite->gate)
> +               composite->gate->dev = clk->dev;
> +
> +
>         return clk;
>
>  err:
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index 822e09b084..bfa05f24a3 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -70,8 +70,7 @@ unsigned long divider_recalc_rate(struct clk *hw, unsigned long parent_rate,
>
>  static ulong clk_divider_recalc_rate(struct clk *clk)  {
> -       struct clk_divider *divider = to_clk_divider(clk_dev_binded(clk) ?
> -                       dev_get_clk_ptr(clk->dev) : clk);
> +       struct clk_divider *divider = to_clk_divider(clk);
>         unsigned long parent_rate = clk_get_parent_rate(clk);
>         unsigned int val;
>
> @@ -150,8 +149,7 @@ int divider_get_val(unsigned long rate, unsigned long parent_rate,
>
>  static ulong clk_divider_set_rate(struct clk *clk, unsigned long rate)  {
> -       struct clk_divider *divider = to_clk_divider(clk_dev_binded(clk) ?
> -                       dev_get_clk_ptr(clk->dev) : clk);
> +       struct clk_divider *divider = to_clk_divider(clk);
>         unsigned long parent_rate = clk_get_parent_rate(clk);
>         int value;
>         u32 val;
> diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c index 711b0588bc..d2401cf440 100644
> --- a/drivers/clk/clk-fixed-factor.c
> +++ b/drivers/clk/clk-fixed-factor.c
> @@ -18,8 +18,7 @@
>
>  static ulong clk_factor_recalc_rate(struct clk *clk)  {
> -       struct clk_fixed_factor *fix =
> -               to_clk_fixed_factor(dev_get_clk_ptr(clk->dev));
> +       struct clk_fixed_factor *fix = to_clk_fixed_factor(clk);
>         unsigned long parent_rate = clk_get_parent_rate(clk);
>         unsigned long long int rate;
>
> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c index 70b8794554..b2933bc24a 100644
> --- a/drivers/clk/clk-gate.c
> +++ b/drivers/clk/clk-gate.c
> @@ -43,8 +43,7 @@
>   */
>  static void clk_gate_endisable(struct clk *clk, int enable)  {
> -       struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ?
> -                       dev_get_clk_ptr(clk->dev) : clk);
> +       struct clk_gate *gate = to_clk_gate(clk);
>         int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;
>         u32 reg;
>
> @@ -86,8 +85,7 @@ static int clk_gate_disable(struct clk *clk)
>
>  int clk_gate_is_enabled(struct clk *clk)  {
> -       struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ?
> -                       dev_get_clk_ptr(clk->dev) : clk);
> +       struct clk_gate *gate = to_clk_gate(clk);
>         u32 reg;
>
>  #if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF)
> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c index 5acc0b8cbd..67b4afef28 100644
> --- a/drivers/clk/clk-mux.c
> +++ b/drivers/clk/clk-mux.c
> @@ -35,8 +35,7 @@
>  int clk_mux_val_to_index(struct clk *clk, u32 *table, unsigned int flags,
>                          unsigned int val)
>  {
> -       struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -                       dev_get_clk_ptr(clk->dev) : clk);
> +       struct clk_mux *mux = to_clk_mux(clk);
>         int num_parents = mux->num_parents;
>
>         if (table) {
> @@ -79,8 +78,7 @@ unsigned int clk_mux_index_to_val(u32 *table, unsigned int flags, u8 index)
>
>  u8 clk_mux_get_parent(struct clk *clk)
>  {
> -       struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -                       dev_get_clk_ptr(clk->dev) : clk);
> +       struct clk_mux *mux = to_clk_mux(clk);
>         u32 val;
>
>  #if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF)
> @@ -97,8 +95,7 @@ u8 clk_mux_get_parent(struct clk *clk)  static int clk_fetch_parent_index(struct clk *clk,
>                                   struct clk *parent)
>  {
> -       struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -                       dev_get_clk_ptr(clk->dev) : clk);
> +       struct clk_mux *mux = to_clk_mux(clk);
>
>         int i;
>
> @@ -115,8 +112,7 @@ static int clk_fetch_parent_index(struct clk *clk,
>
>  static int clk_mux_set_parent(struct clk *clk, struct clk *parent)  {
> -       struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -                       dev_get_clk_ptr(clk->dev) : clk);
> +       struct clk_mux *mux = to_clk_mux(clk);
>         int index;
>         u32 val;
>         u32 reg;
> diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c index 1b9db6e791..e32c0cb53e 100644
> --- a/drivers/clk/imx/clk-gate2.c
> +++ b/drivers/clk/imx/clk-gate2.c
> @@ -37,7 +37,7 @@ struct clk_gate2 {
>
>  static int clk_gate2_enable(struct clk *clk)  {
> -       struct clk_gate2 *gate = to_clk_gate2(dev_get_clk_ptr(clk->dev));
> +       struct clk_gate2 *gate = to_clk_gate2(clk);
>         u32 reg;
>
>         reg = readl(gate->reg);
> @@ -50,7 +50,7 @@ static int clk_gate2_enable(struct clk *clk)
>
>  static int clk_gate2_disable(struct clk *clk)  {
> -       struct clk_gate2 *gate = to_clk_gate2(dev_get_clk_ptr(clk->dev));
> +       struct clk_gate2 *gate = to_clk_gate2(clk);
>         u32 reg;
>
>         reg = readl(gate->reg);
> --
> 2.24.1
>

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Sean Anderson]

> This patch is clock relative patch, if it is not a necessary patch for
> Sipeed Maix support.
> Please remove patch 1/11 and 2/11, and send them in another patch-sets.

Patches 10 and 11 rely on these first two patches. Should I just
reference the split off patches in the cover letter?

[PATCH v2 03/11] riscv: Add headers for asm/global_data.h

[Rick Chen]

Hi Sean,

> From: Sean Anderson [mailto:seanga2 at gmail.com]
> Sent: Thursday, January 16, 2020 6:51 AM
> To: U-Boot Mailing List
> Cc: Rick Jian-Zhi Chen(陳建志)
> Subject: [PATCH v2 03/11] riscv: Add headers for asm/global_data.h
>
> This header depended on bd_t and ulong, but did not include the appropriate headers.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>  arch/riscv/include/asm/global_data.h | 2 ++
>  1 file changed, 2 insertions(+)
>

I wonder why the compiling is OK without those appropriate headers.

Thanks,
Rick

> diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> index b74bd7e738..4f0c12b402 100644
> --- a/arch/riscv/include/asm/global_data.h
> +++ b/arch/riscv/include/asm/global_data.h
> @@ -11,6 +11,8 @@
>  #define __ASM_GBL_DATA_H
>
>  #include <asm/smp.h>
> +#include <asm/u-boot.h>
> +#include <linux/compiler.h>
>
>  /* Architecture-specific global data */  struct arch_global_data {
> --
> 2.24.1
>

[PATCH v2 04/11] riscv: Add an option to default to RV64I

[Rick Chen]

Hi Sean,

> From: Sean Anderson [mailto:seanga2 at gmail.com]
> Sent: Thursday, January 16, 2020 6:52 AM
> To: U-Boot Mailing List; Rick Jian-Zhi Chen(陳建志)
> Subject: [PATCH v2 04/11] riscv: Add an option to default to RV64I
>
> This allows 64-bit boards to default to the 64-bit instruction set without changing the current default of 32-bit.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>  arch/riscv/Kconfig | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 85e15ebffa..9a7b0334c2 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -60,8 +60,12 @@ source "arch/riscv/cpu/generic/Kconfig"
>
>  # architecture-specific options below
>

This is a redundant declaration.
There have CONFIG_ARCH_RV64I=y already in sipeed_maix_bitm_defconfig

Thanks,
Rick

> +config ARCH_DEFAULT_RV64I
> +       bool
> +
>  choice
>         prompt "Base ISA"
> +       default ARCH_RV64I if ARCH_DEFAULT_RV64I
>         default ARCH_RV32I
>
>  config ARCH_RV32I
> --
> 2.24.1
>

[PATCH v2 03/11] riscv: Add headers for asm/global_data.h

[Sean Anderson]

> I wonder why the compiling is OK without those appropriate headers.

It's likely that all the uses looked like

#include <this_header_includes_ulong.h>
#include <asm/global_data.h>

So it wasn't noticed until it was included first in some file.

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Rick Chen]

Hi Sean,

> > This patch is clock relative patch, if it is not a necessary patch for
> > Sipeed Maix support.
> > Please remove patch 1/11 and 2/11, and send them in another patch-sets.
>
> Patches 10 and 11 rely on these first two patches. Should I just
> reference the split off patches in the cover letter?

I am not sure the modifications of clk are ok for other people.
It will be better to send pure riscv relative patch-sets.

Thanks,
Rick

[PATCH v2 08/11] riscv: Add device tree for K210

[Rick Chen]

Hi Sean,

> From: Sean Anderson [mailto:seanga2 at gmail.com]
> Sent: Thursday, January 16, 2020 7:18 AM
> To: U-Boot Mailing List
> Cc: Rick Jian-Zhi Chen(陳建志)
> Subject: [PATCH v2 08/11] riscv: Add device tree for K210
>
> The subject for this patch should be
>
> Subject: [PATCH v2 08/11] riscv: Add device tree for K210

Lacking of contents.

Thanks,
Rick

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Sean Anderson]

> I am not sure the modifications of clk are ok for other people.
> It will be better to send pure riscv relative patch-sets.

Hm, well at the moment removing the dependency on these two patches
would probably require a substantial rewrite of patch 11, which would
primarily consist of duplicating functionality currently found in the
clk_composite driver. I actually think the way clk_composite works at
the moment is very confusing, since the imx code which uses it uses some
custom functions to smooth out clk_composite's behaviour. These two
patches are probably the ones I would like to see merged most of all,
since it would make implementing complex clock drivers like on this
board much easier. I originally submitted these patches around a month
ago [1, 2], so I was hoping that I'd have recieved feedback one way or
the other by now.

[1] https://patchwork.ozlabs.org/patch/1215327/
[2] https://patchwork.ozlabs.org/patch/1215328/

[PATCH v2 08/11] riscv: Add device tree for K210

[Sean Anderson]

There is a mirror of ram located at 0x4000000 which is un-cached. This is
probably useful for DMA, but I am unsure of how to describe it in this device
tree.

The cache-line size is undocumented. Emphirical tests suggest that it is 32
bytes, but I've used 64-bytes to be on the safe side.

Where possible, I have tried to find compatible drivers based on the layout of
registers. However, I have not tested most of this functionality, and anything
aside from clint0, uarths0, and sysctl should be considered descriptive at best.
I would appreciate if anyone could help identify possibly compatible devices,
especially for the timers, watchdogs, and rtc.

The documentation for pinconf devices indicates that #pinctrl-cells is a
required property, but I am unsure what to put for it. Should that be filled in
by a board-specific device tree?

The sysctl device has multiple different unrelated registers. For the moment, I
have split off sub-devices for each functionality. In addition to clock and
reset control, it also contains DMA handshake registers, and power registers. I
have not described those at the moment, and I would appreciate suggestions on
how to best describe them.

I'm not sure what the value of spi-max-frequency should be for external sd-card
slots. Suggestions are appreciated.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
 arch/riscv/dts/Makefile                 |   1 +
 arch/riscv/dts/k210-maix-bit.dts        |  42 +++
 arch/riscv/dts/k210.dtsi                | 453 ++++++++++++++++++++++++
 board/sipeed/maix/Kconfig               |   3 +
 configs/sipeed_maix_bitm_defconfig      |   1 +
 include/dt-bindings/clock/k210-sysctl.h |  54 +++
 include/dt-bindings/reset/k210-sysctl.h |  38 ++
 7 files changed, 592 insertions(+)
 create mode 100644 arch/riscv/dts/k210-maix-bit.dts
 create mode 100644 arch/riscv/dts/k210.dtsi
 create mode 100644 include/dt-bindings/clock/k210-sysctl.h
 create mode 100644 include/dt-bindings/reset/k210-sysctl.h

diff --git a/arch/riscv/dts/Makefile b/arch/riscv/dts/Makefile
index 4f30e6936f..3a6f96c67d 100644
--- a/arch/riscv/dts/Makefile
+++ b/arch/riscv/dts/Makefile
@@ -2,6 +2,7 @@
 
 dtb-$(CONFIG_TARGET_AX25_AE350) += ae350_32.dtb ae350_64.dtb
 dtb-$(CONFIG_TARGET_SIFIVE_FU540) += hifive-unleashed-a00.dtb
+dtb-$(CONFIG_TARGET_SIPEED_MAIX) += k210-maix-bit.dtb
 
 targets += $(dtb-y)
 
diff --git a/arch/riscv/dts/k210-maix-bit.dts b/arch/riscv/dts/k210-maix-bit.dts
new file mode 100644
index 0000000000..a25aa34ff8
--- /dev/null
+++ b/arch/riscv/dts/k210-maix-bit.dts
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+
+/dts-v1/;
+
+#include "k210.dtsi"
+
+/ {
+	model = "Sipeed Maix Bit";
+	compatible = "sipeed,maix-bit", "kendryte,k210";
+
+	chosen {
+		bootargs = "console=ttyS0,115200n8 debug loglevel=7";
+		stdout-path = "serial0";
+	};
+};
+
+&uarths0 {
+	status = "okay";
+};
+
+&spi0 {
+	status = "okay";
+	slot at 0 {
+		compatible = "mmc-spi-slot";
+		reg = <0>;
+		broken-cd;
+		disable-wp;
+	};
+};
+
+&spi3 {
+	status = "okay";
+	spi-flash at 0 {
+		compatible = "gd25lq64c", "gd25q64", "jedec,spi-nor";
+		reg = <0>;
+		spi-max-frequency = <120000000>;
+		m25p,fast-read;
+	};
+};
diff --git a/arch/riscv/dts/k210.dtsi b/arch/riscv/dts/k210.dtsi
new file mode 100644
index 0000000000..1408d9b4a9
--- /dev/null
+++ b/arch/riscv/dts/k210.dtsi
@@ -0,0 +1,453 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+
+#include <dt-bindings/clock/k210-sysctl.h>
+#include <dt-bindings/reset/k210-sysctl.h>
+
+/ {
+	/*
+	 * Although the K210 is a 64-bit CPU, the address bus is only 32-bits
+	 * wide, and the upper half of all addresses is ignored.
+	 */
+	#address-cells = <1>;
+	#size-cells = <1>;
+	compatible = "kendryte,k210";
+
+	aliases {
+		serial0 = &uarths0;
+		serial1 = &uart1;
+		serial2 = &uart2;
+		serial3 = &uart3;
+		spi0 = &spi0;
+		spi1 = &spi1;
+		spi2 = &spi2;
+		spi3 = &spi3;
+	};
+
+	clocks {
+		in0: oscillator {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <26000000>;
+		};
+	};
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		timebase-frequency = <7800000>;
+		cpu0: cpu at 0 {
+			device_type = "cpu";
+			reg = <0>;
+			compatible = "riscv";
+			riscv,isa = "rv64acdfim";
+			mmu-type = "riscv,sv32";
+			i-cache-size = <0x8000>;
+			i-cache-block-size = <64>; /* bogus */
+			d-cache-size = <0x8000>;
+			d-cache-block-size = <64>; /* bogus */
+			clocks = <&sysclk K210_CLK_CPU>;
+			clock-frequency = <390000000>;
+			cpu0_intc: interrupt-controller {
+				#interrupt-cells = <1>;
+				interrupt-controller;
+				compatible = "riscv,cpu-intc";
+			};
+		};
+		cpu1: cpu at 1 {
+			device_type = "cpu";
+			reg = <1>;
+			compatible = "riscv";
+			riscv,isa = "rv64acdfim";
+			mmu-type = "riscv,sv32";
+			i-cache-size = <0x8000>;
+			i-cache-block-size = <64>; /* bogus */
+			d-cache-size = <0x8000>;
+			d-cache-block-size = <64>; /* bogus */
+			clocks = <&sysclk K210_CLK_CPU>;
+			clock-frequency = <390000000>;
+			cpu1_intc: interrupt-controller {
+				#interrupt-cells = <1>;
+				interrupt-controller;
+				compatible = "riscv,cpu-intc";
+			};
+		};
+	};
+
+	sram0: memory at 80000000 {
+		device_type = "memory";
+		reg = <0x80000000 0x400000>;
+		clocks = <&sysclk K210_CLK_SRAM0>;
+	};
+
+	sram1: memory at 80400000 {
+		device_type = "memory";
+		reg = <0x80400000 0x400000>;
+		clocks = <&sysclk K210_CLK_SRAM1>;
+	};
+
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		ai_reserved: ai at 80600000 {
+			reg = <0x80600000 0x200000>;
+			reusable;
+		};
+	};
+
+	soc {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "kendryte,k210-soc", "simple-bus";
+		ranges;
+		interrupt-parent = <&plic0>;
+
+		clint0: interrupt-controller at 2000000 {
+			compatible = "riscv,clint0";
+			reg = <0x2000000 0xC000>;
+			interrupts-extended = <&cpu0_intc 3>,  <&cpu1_intc 3>;
+			clocks = <&sysclk K210_CLK_CPU>;
+		};
+
+		plic0: interrupt-controller at c000000 {
+			#interrupt-cells = <1>;
+			interrupt-controller;
+			compatible = "riscv,plic1";
+			reg = <0xC000000 0x03FFF008>;
+			interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>,
+			                      <&cpu1_intc 11>, <&cpu1_intc 9>;
+			riscv,ndev = <65>;
+		};
+
+		uarths0: serial at 38000000 {
+			compatible = "sifive,uart0";
+			reg = <0x38000000 0x20>;
+			interrupts = <33>;
+			clocks = <&sysclk K210_CLK_CPU>;
+			status = "disabled";
+		};
+
+		gpio0: gpio-controller at 38001000 {
+			#gpio-cells = <2>;
+			compatible = "none";
+			reg = <0x38001000 0x44>;
+			gpio-controller;
+			interrupts = <34 35 36 37 38 39 40 41
+			              42 43 44 45 46 47 48 49
+			              50 51 52 53 54 55 56 57
+				      58 59 60 61 62 63 64 65>;
+			clocks = <&sysclk K210_CLK_CPU>;
+			status = "disabled";
+		};
+
+		kpu0: kpu at 40600000 {
+			compatible = "none";
+			reg = <0x40800000 0x48>;
+			interrupts = <25>;
+			clocks = <&sysclk K210_CLK_AI>;
+			memory-region = <&ai_reserved>;
+			status = "disabled";
+		};
+
+		fft0: fft at 42000000 {
+			compatible = "none";
+			reg = <0x42000000 0x400000>;
+			interrupts = <26>;
+			clocks = <&sysclk K210_CLK_FFT>;
+			resets = <&sysrst K210_RST_FFT>;
+			status = "disabled";
+		};
+
+		dmac0: dma-controller at 50000000 {
+			compatible = "snps,axi-dma-1.01a";
+			reg = <0x50000000 0xD00>;
+			interrupts = <27 28 29 30 31 32>;
+			clocks = <&sysclk K210_CLK_DMA>, <&sysclk K210_CLK_DMA>;
+			clock-names = "core-clk", "cfgr-clk";
+			resets = <&sysrst K210_RST_DMA>;
+			dma-channels = <6>;
+			snps,dma-masters = <2>;
+			snps,data-width = <5>;
+			snps,block-size = <0x400000 0x400000 0x400000
+			                   0x400000 0x400000 0x400000>;
+			snps,axi-max-burst-len = <256>;
+			status = "disabled";
+		};
+
+		gpio1: gpio-controller at 50200000 {
+			#gpio-cells = <2>;
+			compatible = "none";
+			reg = <0x50200000 0x6C>;
+			gpio-controller;
+			interrupts = <23>;
+			clocks = <&sysclk K210_CLK_GPIO>;
+			resets = <&sysrst K210_RST_GPIO>;
+			status = "disabled";
+		};
+
+		uart1: serial at 50210000 {
+			compatible = "ns16550";
+			reg = <50210000 0xc4>;
+			interrupts = <11>;
+			clocks = <&sysclk K210_CLK_UART1>;
+			resets = <&sysrst K210_RST_UART1>;
+			fifo-size = <8>;
+			no-loopback-test;
+			status = "disabled";
+		};
+
+		uart2: serial at 50220000 {
+			compatible = "ns16550";
+			reg = <50220000 0xc4>;
+			interrupts = <12>;
+			clocks = <&sysclk K210_CLK_UART2>;
+			resets = <&sysrst K210_RST_UART2>;
+			fifo-size = <8>;
+			no-loopback-test;
+			status = "disabled";
+		};
+
+		uart3: serial at 50230000 {
+			compatible = "ns16550";
+			reg = <50230000 0xc4>;
+			interrupts = <13>;
+			clocks = <&sysclk K210_CLK_UART3>;
+			resets = <&sysrst K210_RST_UART3>;
+			fifo-size = <8>;
+			no-loopback-test;
+			status = "disabled";
+		};
+
+		spi2: spi at 50240000 {
+			compatible = "snps,dw-apb-ssi";
+			spi-slave;
+			reg = <0x50240000 0x120>;
+			interrupts = <2>;
+			clocks = <&sysclk K210_CLK_SPI2>;
+			resets = <&sysrst K210_RST_SPI2>;
+			status = "disabled";
+		};
+
+		i2s0: i2s at 50250000 {
+			compatible = "snps.designware-i2s";
+			reg = <0x50250000 0x200>;
+			interrupts = <5>;
+			clocks = <&sysclk K210_CLK_I2S0>;
+			resets = <&sysrst K210_RST_I2S0>;
+			status = "disabled";
+		};
+
+		apu0: sound at 520250200 {
+			compatible = "none";
+			reg = <0x50250200 0x138>;
+			status = "disabled";
+		};
+
+		i2s1: i2s at 50260000 {
+			compatible = "snps.designware-i2s";
+			reg = <0x50260000 0x200>;
+			interrupts = <6>;
+			clocks = <&sysclk K210_CLK_I2S1>;
+			resets = <&sysrst K210_RST_I2S1>;
+			status = "disabled";
+		};
+
+		i2s2: i2s at 50270000 {
+			compatible = "snps.designware-i2s";
+			reg = <0x50270000 0x200>;
+			interrupts = <7>;
+			clocks = <&sysclk K210_CLK_I2S2>;
+			resets = <&sysrst K210_RST_I2S2>;
+			status = "disabled";
+		};
+
+		i2c0: i2c at 50280000 {
+			compatible = "snps,designware-i2c";
+			reg = <0x50280000 0x100>;
+			interrupts = <8>;
+			clocks = <&sysclk K210_CLK_I2C0>;
+			resets = <&sysrst K210_RST_I2C0>;
+			status = "disabled";
+		};
+
+		i2c1: i2c at 50290000 {
+			compatible = "snps,designware-i2c";
+			reg = <0x50290000 0x100>;
+			interrupts = <9>;
+			clocks = <&sysclk K210_CLK_I2C1>;
+			resets = <&sysrst K210_RST_I2C1>;
+			status = "disabled";
+		};
+
+
+		i2c2: i2c at 502A0000 {
+			compatible = "snps,designware-i2c";
+			reg = <0x502A0000 0x100>;
+			interrupts = <10>;
+			clocks = <&sysclk K210_CLK_I2C2>;
+			resets = <&sysrst K210_RST_I2C2>;
+			status = "disabled";
+		};
+
+		fpioa: pinmux at 502B0000 {
+			compatible = "pinconf-single";
+			reg = <0x502B0000 0x100>;
+			//#pinctrl-cells = <???>;
+			pinctrl-single,register-width = <32>;
+			pinctrl-single,function-mask = <0x7fffff>;
+			clocks = <&sysclk K210_CLK_FPIOA>;
+			resets = <&sysrst K210_RST_FPIOA>;
+			status = "disabled";
+		};
+
+		sha256: sha256 at 502C0000 {
+			compatible = "none";
+			reg = <0x502C0000 0x38>;
+			clocks = <&sysclk K210_CLK_SHA>;
+			resets = <&sysrst K210_RST_SHA>;
+			status = "disabled";
+		};
+
+		timer0: timer at 502D0000 {
+			compatible = "none";
+			reg = <0x502D0000 0xC0>;
+			interrupts = <14 15>;
+			clocks = <&sysclk K210_CLK_TIMER0>;
+			resets = <&sysrst K210_RST_TIMER0>;
+			status = "disabled";
+		};
+
+		timer1: timer at 502E0000 {
+			compatible = "none";
+			reg = <0x502E0000 0xC0>;
+			interrupts = <16 17>;
+			clocks = <&sysclk K210_CLK_TIMER1>;
+			resets = <&sysrst K210_RST_TIMER1>;
+			status = "disabled";
+		};
+
+		timer2: timer at 502F0000 {
+			compatible = "none";
+			reg = <0x502F0000 0xC0>;
+			interrupts = <18 19>;
+			clocks = <&sysclk K210_CLK_TIMER2>;
+			resets = <&sysrst K210_RST_TIMER2>;
+			status = "disabled";
+		};
+
+		wdt0: watchdog at 50400000 {
+			compatible = "none";
+			reg = <0x50400000 0x100>;
+			interrupts = <21>;
+			clocks = <&sysclk K210_CLK_WDT0>;
+			resets = <&sysrst K210_RST_WDT0>;
+			status = "disabled";
+		};
+
+		wdt1: watchdog at 50410000 {
+			compatible = "none";
+			reg = <0x50410000 0x100>;
+			interrupts = <22>;
+			clocks = <&sysclk K210_CLK_WDT1>;
+			resets = <&sysrst K210_RST_WDT1>;
+			status = "disabled";
+		};
+
+		dvp0: camera at 50430000 {
+			compatible = "none";
+			reg = <0x50430000 0x2c>;
+			interrupts = <24>;
+			clocks = <&sysclk K210_CLK_DVP>;
+			resets = <&sysrst K210_RST_DVP>;
+			status = "disabled";
+		};
+
+		sysctl: sysctl at 50440000 {
+			compatible = "kendryte,k210-sysctl", "syscon";
+			reg = <0x50440000 0x80>;
+
+			sysclk: clock-controller {
+				compatible = "kendryte,k210-clk";
+				clocks = <&in0>;
+				#clock-cells = <1>;
+			};
+
+			sysrst: reset-controller {
+				compatible = "kendryte,k210-rst";
+				#reset-cells = <1>;
+			};
+		};
+
+		aes0: aes at 50450000 {
+			compatible = "none";
+			reg = <0x50450000 0x94>;
+			clocks = <&sysclk K210_CLK_AES>;
+			resets = <&sysrst K210_RST_AES>;
+			status = "disabled";
+		};
+
+		rtc: rts at 50460000 {
+			compatible = "none";
+			reg = <0x50460000 0x2C>;
+			clocks = <&in0>;
+			resets = <&sysrst K210_RST_RTC>;
+			interrupts = <20>;
+			status = "disabled";
+		};
+
+		spi0: spi at 52000000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-ssi";
+			reg = <0x52000000 0x120>;
+			interrupts = <1>;
+			clocks = <&sysclk K210_CLK_SPI0>;
+			clock-names = "ssi_clk";
+			resets = <&sysrst K210_RST_SPI0>;
+			num-cs = <4>;
+			reg-io-width = <4>;
+			status = "disabled";
+		};
+
+		spi1: spi at 53000000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-ssi";
+			reg = <0x53000000 0x120>;
+			interrupts = <2>;
+			clocks = <&sysclk K210_CLK_SPI1>;
+			clock-names = "ssi_clk";
+			resets = <&sysrst K210_RST_SPI1>;
+			num-cs = <4>;
+			reg-io-width = <4>;
+			status = "disabled";
+		};
+
+		spi3: spi at 54000000 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "snps,dw-apb-ssi";
+			reg = <0x54000000 0x120>;
+			interrupts = <4>;
+			clocks = <&sysclk K210_CLK_SPI3>;
+			clock-names = "ssi_clk";
+			assigned-clocks = <&sysclk K210_CLK_SPI3>;
+			assigned-clock-parents = <&sysclk K210_CLK_PLL0>;
+			resets = <&sysrst K210_RST_SPI3>;
+			num-cs = <4>;
+			reg-io-width = <4>;
+			status = "disabled";
+		};
+
+		rom0: nvmem at 88000000 {
+			reg = <0x88000000 0x4000>;
+			read-only;
+			clocks = <&sysclk K210_CLK_ROM>;
+			resets = <&sysrst K210_RST_ROM>;
+		};
+	};
+};
diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
index 9259eb34aa..efeaddf317 100644
--- a/board/sipeed/maix/Kconfig
+++ b/board/sipeed/maix/Kconfig
@@ -18,6 +18,9 @@ config SYS_CONFIG_NAME
 config SYS_TEXT_BASE
 	default 0x80000000
 
+config DEFAULT_DEVICE_TREE
+	default "k210-maix-bit"
+
 config NR_CPUS
 	default 2
 
diff --git a/configs/sipeed_maix_bitm_defconfig b/configs/sipeed_maix_bitm_defconfig
index f062cc8c58..ecd69c0873 100644
--- a/configs/sipeed_maix_bitm_defconfig
+++ b/configs/sipeed_maix_bitm_defconfig
@@ -15,6 +15,7 @@ CONFIG_NR_DRAM_BANKS=2
 CONFIG_BOOTSTAGE_STASH_ADDR=0
 CONFIG_64BIT=y
 CONFIG_TARGET_SIPEED_MAIX=y
+CONFIG_DEFAULT_DEVICE_TREE="k210-maix-bit"
 CONFIG_NR_CPUS=2
 CONFIG_GENERIC_RISCV=y
 CONFIG_ARCH_DEFAULT_RV64I=y
diff --git a/include/dt-bindings/clock/k210-sysctl.h b/include/dt-bindings/clock/k210-sysctl.h
new file mode 100644
index 0000000000..308974baf8
--- /dev/null
+++ b/include/dt-bindings/clock/k210-sysctl.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+
+#ifndef CLOCK_K210_SYSCTL_H
+#define CLOCK_K210_SYSCTL_H
+
+/* Clocks as defined by kendryte-standalone-sdk/lib/drivers/include/sysctl.h */
+#define K210_CLK_PLL0 0
+#define K210_CLK_PLL1 1
+#define K210_CLK_PLL2 2
+#define K210_CLK_CPU 3
+#define K210_CLK_SRAM0 4
+#define K210_CLK_SRAM1 5
+#define K210_CLK_APB0 6
+#define K210_CLK_APB1 7
+#define K210_CLK_APB2 8
+#define K210_CLK_ROM 9
+#define K210_CLK_DMA 10
+#define K210_CLK_AI 11
+#define K210_CLK_DVP 12
+#define K210_CLK_FFT 13
+#define K210_CLK_GPIO 14
+#define K210_CLK_SPI0 15
+#define K210_CLK_SPI1 16
+#define K210_CLK_SPI2 17
+#define K210_CLK_SPI3 18
+#define K210_CLK_I2S0 19
+#define K210_CLK_I2S1 20
+#define K210_CLK_I2S2 21
+#define K210_CLK_I2C0 22
+#define K210_CLK_I2C1 23
+#define K210_CLK_I2C2 24
+#define K210_CLK_UART1 25
+#define K210_CLK_UART2 26
+#define K210_CLK_UART3 27
+#define K210_CLK_AES 28
+#define K210_CLK_FPIOA 29
+#define K210_CLK_TIMER0 30
+#define K210_CLK_TIMER1 31
+#define K210_CLK_TIMER2 32
+#define K210_CLK_WDT0 33
+#define K210_CLK_WDT1 34
+#define K210_CLK_SHA 35
+#define K210_CLK_OTP 36
+#define K210_CLK_RTC 37
+#define K210_CLK_ACLK 40
+#define K210_CLK_MAX 40
+
+#define K210_CLK_HCLK 41 /* Unused */
+#define K210_CLK_IN0 42 /* Defined elsewhere in the device tree */
+
+#endif /* CLOCK_K210_SYSCTL_H */
diff --git a/include/dt-bindings/reset/k210-sysctl.h b/include/dt-bindings/reset/k210-sysctl.h
new file mode 100644
index 0000000000..670e74daaf
--- /dev/null
+++ b/include/dt-bindings/reset/k210-sysctl.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
+ */
+
+#ifndef RESET_K210_SYSCTL_H
+#define RESET_K210_SYSCTL_H
+
+#define K210_RST_ROM 0	
+#define K210_RST_DMA 1	
+#define K210_RST_AI 2	
+#define K210_RST_DVP 3	
+#define K210_RST_FFT 4	
+#define K210_RST_GPIO 5	
+#define K210_RST_SPI0 6	
+#define K210_RST_SPI1 7	
+#define K210_RST_SPI2 8	
+#define K210_RST_SPI3 9	
+#define K210_RST_I2S0 10	
+#define K210_RST_I2S1 11	
+#define K210_RST_I2S2 12	
+#define K210_RST_I2C0 13	
+#define K210_RST_I2C1 14	
+#define K210_RST_I2C2 15	
+#define K210_RST_UART1 16	
+#define K210_RST_UART2 17	
+#define K210_RST_UART3 18	
+#define K210_RST_AES 19	
+#define K210_RST_FPIOA 20	
+#define K210_RST_TIMER0 21	
+#define K210_RST_TIMER1 22	
+#define K210_RST_TIMER2 23	
+#define K210_RST_WDT0 24	
+#define K210_RST_WDT1 25	
+#define K210_RST_SHA 26	
+#define K210_RST_RTC 29	
+
+#endif /* RESET_K210_SYSCTL_H */
-- 
2.25.0

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Sean Anderson]

> [1] https://patchwork.ozlabs.org/patch/1215327/
> [2] https://patchwork.ozlabs.org/patch/1215328/

err, these references should be

[1] https://patchwork.ozlabs.org/patch/1215335/
[2] https://patchwork.ozlabs.org/patch/1215337/

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Lukasz Majewski]

Hi Sean,

> > I am not sure the modifications of clk are ok for other people.
> > It will be better to send pure riscv relative patch-sets.  
> 
> Hm, well at the moment removing the dependency on these two patches
> would probably require a substantial rewrite of patch 11, which would
> primarily consist of duplicating functionality currently found in the
> clk_composite driver. I actually think the way clk_composite works at
> the moment is very confusing, since the imx code which uses it uses
> some custom functions to smooth out clk_composite's behaviour. These
> two patches are probably the ones I would like to see merged most of
> all, since it would make implementing complex clock drivers like on
> this board much easier. I originally submitted these patches around a
> month ago [1, 2], so I was hoping that I'd have recieved feedback one
> way or the other by now.
> 
> [1] https://patchwork.ozlabs.org/patch/1215327/
> [2] https://patchwork.ozlabs.org/patch/1215328/

I saw your patches. Unfortunately, there was the Christmas/New year's
break and afterwards I had some more urgent tasks to do. Apologize for
that...

What I would like to see here is to reuse (or better - make the code
less confusing) the code.

Rationale - the CCF was ported from iMX6Q Linux code from the outset.

Then Peng (CC'ed) wanted to adjust it to support composite clocks from
i.MX8.

As a result the CCF drifted to be an iMX aligned, but the goal is to
have it usable for other archs as well (and reuse from Linux as much as
possible).


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200124/075ca6dc/attachment.sig>

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Sean Anderson]

On 1/24/20 9:27 AM, Lukasz Majewski wrote:
> I saw your patches. Unfortunately, there was the Christmas/New year's
> break and afterwards I had some more urgent tasks to do. Apologize for
> that...
> 
> What I would like to see here is to reuse (or better - make the code
> less confusing) the code.
> 
> Rationale - the CCF was ported from iMX6Q Linux code from the outset.
> 
> Then Peng (CC'ed) wanted to adjust it to support composite clocks from
> i.MX8.
> 
> As a result the CCF drifted to be an iMX aligned, but the goal is to
> have it usable for other archs as well (and reuse from Linux as much as
> possible).

Ok, so I believe that these patches are a step in that direction. Is
there any specific feedback you would like to give regarding them? I've
tried to infer what good default behaviour should be. However, I'm sure
there are places where someone more familiar with the CCF would be able
to comment on.

--Sean

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Lukasz Majewski]

Hi Sean,

> On 1/24/20 9:27 AM, Lukasz Majewski wrote:
> > I saw your patches. Unfortunately, there was the Christmas/New
> > year's break and afterwards I had some more urgent tasks to do.
> > Apologize for that...
> > 
> > What I would like to see here is to reuse (or better - make the code
> > less confusing) the code.
> > 
> > Rationale - the CCF was ported from iMX6Q Linux code from the
> > outset.
> > 
> > Then Peng (CC'ed) wanted to adjust it to support composite clocks
> > from i.MX8.
> > 
> > As a result the CCF drifted to be an iMX aligned, but the goal is to
> > have it usable for other archs as well (and reuse from Linux as
> > much as possible).  
> 
> Ok, so I believe that these patches are a step in that direction. Is
> there any specific feedback you would like to give regarding them?
> I've tried to infer what good default behaviour should be. However,
> I'm sure there are places where someone more familiar with the CCF
> would be able to comment on.

I will do the review by the end of the weekend.

> 
> --Sean
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200125/bce98745/attachment.sig>

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Lukasz Majewski]

Hi Sean,

> CCF clocks should always use the struct clock passed to their methods
> for extracting the driver-specific clock information struct.

This couldn't be done for i.MX8 at least. There was an issue with
composite clock on this SoC.

(I've CC'ed Peng, who did this work for i.MX8 - I was
testing/developing the framework for i.MX6Q which doesn't support
composite clocks).

For some design decisions and the "overall picture" of CCF , please see
following doc entry:
https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt


> Previously, many functions would use the clk->dev->priv if the device
> was bound. This could cause problems with composite clocks. 

The problem (in short) is with combining Linux CCF design and U-Boot's
DM (more details in the link above).

All the clocks are linked with struct udevice (one search the linked
list for proper clock).

However, with Linux the "main" clock structure is 'struct clk, which is
embedded in CLK IP block specific structure (i.e. struct clk_gate2).

Problem is that from struct clk we do have pointer to struct udevice
(*dev), but there is no direct pointer "up" from struct udevice to
struct clk (now we do use udevice's->uclass_priv).

So far so good ....

Problem started with iMX8, where we do have a composite clocks, which
would like to be seen as a single one (like struct pllX), but they
comprise of a few other "clocks".

To distinct them the clk_dev_binded() was added, which checks if the
struct udevice represents "top" composite clock (which shall be visible
with e.g. 'clk' command)



> The
> individual clocks in a composite clock did not have the ->dev field
> filled in. This was fine, because the device-specific clock
> information would be used. However, since there was no ->dev, there
> was no way to get the parent clock. 

Wasn't there any back pointer available? Or do we need to search the
clocks linked list and look for "bind" flag in top level composite
clock?

> This caused the recalc_rate
> method of the CCF divider clock to fail. One option would be to use
> the clk->priv field to get the composite clock and from there get the
> appropriate parent device. However, this would tie the implementation
> to the composite clock. In general, different devices should not rely
> on the contents of ->priv from another device.

Maybe we shall follow:
https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt#L40

> 
> The simple solution to this problem is to just always use the
> supplied struct clock. 

I think that we took this approach in the past. Unfortunately, this
caused all clocks being exposed when 'clk' command was run.

Peng - were there any other issues with i.MX8 composite clock
implementation?

> The composite clock now fills in the ->dev
> pointer of its child clocks. This allows child clocks to make calls
> like clk_get_parent() without issue.
> 
> imx avoided the above problem by using a custom get_rate function
> with composite clocks.

Do you refer to:
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/imx/clk-composite-8m.c#L30

There the clk is cast from (struct clk_composite *)clk->data

(now in U-Boot we do have 4! different implementations of struct clk).

> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>  drivers/clk/clk-composite.c    |  8 ++++++++
>  drivers/clk/clk-divider.c      |  6 ++----
>  drivers/clk/clk-fixed-factor.c |  3 +--
>  drivers/clk/clk-gate.c         |  6 ++----
>  drivers/clk/clk-mux.c          | 12 ++++--------
>  drivers/clk/imx/clk-gate2.c    |  4 ++--
>  6 files changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index a5626c33d1..d0f273d47f 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -145,6 +145,14 @@ struct clk *clk_register_composite(struct device
> *dev, const char *name, goto err;
>  	}
>  
> +	if (composite->mux)
> +		composite->mux->dev = clk->dev;
> +	if (composite->rate)
> +		composite->rate->dev = clk->dev;
> +	if (composite->gate)
> +		composite->gate->dev = clk->dev;
> +
> +
>  	return clk;
>  
>  err:
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index 822e09b084..bfa05f24a3 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -70,8 +70,7 @@ unsigned long divider_recalc_rate(struct clk *hw,
> unsigned long parent_rate, 
>  static ulong clk_divider_recalc_rate(struct clk *clk)
>  {
> -	struct clk_divider *divider =
> to_clk_divider(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_divider *divider = to_clk_divider(clk);

How do you differentiate the "top" level of composite clock from the
clocks from which the composite clock is built?

Now we do use the clk_dev_binded().

>  	unsigned long parent_rate = clk_get_parent_rate(clk);
>  	unsigned int val;
>  
> @@ -150,8 +149,7 @@ int divider_get_val(unsigned long rate, unsigned
> long parent_rate, 
>  static ulong clk_divider_set_rate(struct clk *clk, unsigned long
> rate) {
> -	struct clk_divider *divider =
> to_clk_divider(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_divider *divider = to_clk_divider(clk);
>  	unsigned long parent_rate = clk_get_parent_rate(clk);
>  	int value;
>  	u32 val;
> diff --git a/drivers/clk/clk-fixed-factor.c
> b/drivers/clk/clk-fixed-factor.c index 711b0588bc..d2401cf440 100644
> --- a/drivers/clk/clk-fixed-factor.c
> +++ b/drivers/clk/clk-fixed-factor.c
> @@ -18,8 +18,7 @@
>  
>  static ulong clk_factor_recalc_rate(struct clk *clk)
>  {
> -	struct clk_fixed_factor *fix =
> -		to_clk_fixed_factor(dev_get_clk_ptr(clk->dev));
> +	struct clk_fixed_factor *fix = to_clk_fixed_factor(clk);
>  	unsigned long parent_rate = clk_get_parent_rate(clk);
>  	unsigned long long int rate;
>  
> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
> index 70b8794554..b2933bc24a 100644
> --- a/drivers/clk/clk-gate.c
> +++ b/drivers/clk/clk-gate.c
> @@ -43,8 +43,7 @@
>   */
>  static void clk_gate_endisable(struct clk *clk, int enable)
>  {
> -	struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_gate *gate = to_clk_gate(clk);
>  	int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;
>  	u32 reg;
>  
> @@ -86,8 +85,7 @@ static int clk_gate_disable(struct clk *clk)
>  
>  int clk_gate_is_enabled(struct clk *clk)
>  {
> -	struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_gate *gate = to_clk_gate(clk);
>  	u32 reg;
>  
>  #if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF)
> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> index 5acc0b8cbd..67b4afef28 100644
> --- a/drivers/clk/clk-mux.c
> +++ b/drivers/clk/clk-mux.c
> @@ -35,8 +35,7 @@
>  int clk_mux_val_to_index(struct clk *clk, u32 *table, unsigned int
> flags, unsigned int val)
>  {
> -	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_mux *mux = to_clk_mux(clk);
>  	int num_parents = mux->num_parents;
>  
>  	if (table) {
> @@ -79,8 +78,7 @@ unsigned int clk_mux_index_to_val(u32 *table,
> unsigned int flags, u8 index) 
>  u8 clk_mux_get_parent(struct clk *clk)
>  {
> -	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_mux *mux = to_clk_mux(clk);
>  	u32 val;
>  
>  #if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF)
> @@ -97,8 +95,7 @@ u8 clk_mux_get_parent(struct clk *clk)
>  static int clk_fetch_parent_index(struct clk *clk,
>  				  struct clk *parent)
>  {
> -	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_mux *mux = to_clk_mux(clk);
>  
>  	int i;
>  
> @@ -115,8 +112,7 @@ static int clk_fetch_parent_index(struct clk *clk,
>  
>  static int clk_mux_set_parent(struct clk *clk, struct clk *parent)
>  {
> -	struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ?
> -			dev_get_clk_ptr(clk->dev) : clk);
> +	struct clk_mux *mux = to_clk_mux(clk);
>  	int index;
>  	u32 val;
>  	u32 reg;
> diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c
> index 1b9db6e791..e32c0cb53e 100644
> --- a/drivers/clk/imx/clk-gate2.c
> +++ b/drivers/clk/imx/clk-gate2.c
> @@ -37,7 +37,7 @@ struct clk_gate2 {
>  
>  static int clk_gate2_enable(struct clk *clk)
>  {
> -	struct clk_gate2 *gate =
> to_clk_gate2(dev_get_clk_ptr(clk->dev));
> +	struct clk_gate2 *gate = to_clk_gate2(clk);
>  	u32 reg;
>  
>  	reg = readl(gate->reg);
> @@ -50,7 +50,7 @@ static int clk_gate2_enable(struct clk *clk)
>  
>  static int clk_gate2_disable(struct clk *clk)
>  {
> -	struct clk_gate2 *gate =
> to_clk_gate2(dev_get_clk_ptr(clk->dev));
> +	struct clk_gate2 *gate = to_clk_gate2(clk);
>  	u32 reg;
>  
>  	reg = readl(gate->reg);




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200126/35d7b875/attachment.sig>

[PATCH v2 02/11] clk: Check that ops of composite clock components, exist before calling

[Lukasz Majewski]

Hi Sean,

> clk_composite_ops was shared between all devices in the composite
> clock driver. If one clock had a feature (such as supporting
> set_parent) which another clock did not, it could call a null pointer
> dereference.
> 
> This patch does three things
> 1. It adds null-pointer checks to all composite clock functions.
> 2. It makes clk_composite_ops const and sets its functions at
> compile-time. 3. It adds some basic sanity checks to num_parents.
> 
> The combined effect of these changes is that any of mux, rate, or
> gate can be NULL, and composite clocks will still function normally.
> Previously, at least mux had to exist, since clk_composite_get_parent
> was used to determine the parent for clk_register.
> 

Thank you for those checks - up till now this code was tuned for i.MX
(and in-fact this SoC's clock tree).

However, I would like to first wait for the consensus regarding the
shape of composite clocks (and reply to this mail):

"clk: Always use the supplied struct clk"

> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>  drivers/clk/clk-composite.c | 57
> +++++++++++++++++++++++-------------- 1 file changed, 35
> insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index d0f273d47f..ea9982fd57 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -22,7 +22,10 @@ static u8 clk_composite_get_parent(struct clk *clk)
>  		(struct clk *)dev_get_clk_ptr(clk->dev) : clk);
>  	struct clk *mux = composite->mux;
>  
> -	return clk_mux_get_parent(mux);
> +	if (mux)
> +		return clk_mux_get_parent(mux);
> +	else
> +		return 0;
>  }
>  
>  static int clk_composite_set_parent(struct clk *clk, struct clk
> *parent) @@ -32,7 +35,10 @@ static int
> clk_composite_set_parent(struct clk *clk, struct clk *parent) const
> struct clk_ops *mux_ops = composite->mux_ops; struct clk *mux =
> composite->mux; 
> -	return mux_ops->set_parent(mux, parent);
> +	if (mux && mux_ops)
> +		return mux_ops->set_parent(mux, parent);
> +	else
> +		return -ENOTSUPP;
>  }
>  
>  static unsigned long clk_composite_recalc_rate(struct clk *clk)
> @@ -42,7 +48,10 @@ static unsigned long
> clk_composite_recalc_rate(struct clk *clk) const struct clk_ops
> *rate_ops = composite->rate_ops; struct clk *rate = composite->rate;
>  
> -	return rate_ops->get_rate(rate);
> +	if (rate && rate_ops)
> +		return rate_ops->get_rate(rate);
> +	else
> +		return clk_get_parent_rate(clk);
>  }
>  
>  static ulong clk_composite_set_rate(struct clk *clk, unsigned long
> rate) @@ -52,7 +61,10 @@ static ulong clk_composite_set_rate(struct
> clk *clk, unsigned long rate) const struct clk_ops *rate_ops =
> composite->rate_ops; struct clk *clk_rate = composite->rate;
>  
> -	return rate_ops->set_rate(clk_rate, rate);
> +	if (rate && rate_ops)
> +		return rate_ops->set_rate(clk_rate, rate);
> +	else
> +		return -ENOTSUPP;
>  }
>  
>  static int clk_composite_enable(struct clk *clk)
> @@ -62,7 +74,10 @@ static int clk_composite_enable(struct clk *clk)
>  	const struct clk_ops *gate_ops = composite->gate_ops;
>  	struct clk *gate = composite->gate;
>  
> -	return gate_ops->enable(gate);
> +	if (gate && gate_ops)
> +		return gate_ops->enable(gate);
> +	else
> +		return -ENOTSUPP;
>  }
>  
>  static int clk_composite_disable(struct clk *clk)
> @@ -72,15 +87,12 @@ static int clk_composite_disable(struct clk *clk)
>  	const struct clk_ops *gate_ops = composite->gate_ops;
>  	struct clk *gate = composite->gate;
>  
> -	gate_ops->disable(gate);
> -
> -	return 0;
> +	if (gate && gate_ops)
> +		return gate_ops->disable(gate);
> +	else
> +		return -ENOTSUPP;
>  }
>  
> -struct clk_ops clk_composite_ops = {
> -	/* This will be set according to clk_register_composite */
> -};
> -
>  struct clk *clk_register_composite(struct device *dev, const char
> *name, const char * const *parent_names,
>  				   int num_parents, struct clk *mux,
> @@ -94,7 +106,9 @@ struct clk *clk_register_composite(struct device
> *dev, const char *name, struct clk *clk;
>  	struct clk_composite *composite;
>  	int ret;
> -	struct clk_ops *composite_ops = &clk_composite_ops;
> +
> +	if (!num_parents || (num_parents != 1 && !mux))
> +		return ERR_PTR(-EINVAL);
>  
>  	composite = kzalloc(sizeof(*composite), GFP_KERNEL);
>  	if (!composite)
> @@ -103,8 +117,6 @@ struct clk *clk_register_composite(struct device
> *dev, const char *name, if (mux && mux_ops) {
>  		composite->mux = mux;
>  		composite->mux_ops = mux_ops;
> -		if (mux_ops->set_parent)
> -			composite_ops->set_parent =
> clk_composite_set_parent; mux->data = (ulong)composite;
>  	}
>  
> @@ -113,11 +125,6 @@ struct clk *clk_register_composite(struct device
> *dev, const char *name, clk = ERR_PTR(-EINVAL);
>  			goto err;
>  		}
> -		composite_ops->get_rate = clk_composite_recalc_rate;
> -
> -		/* .set_rate requires either .round_rate or
> .determine_rate */
> -		if (rate_ops->set_rate)
> -			composite_ops->set_rate =
> clk_composite_set_rate; 
>  		composite->rate = rate;
>  		composite->rate_ops = rate_ops;
> @@ -132,8 +139,6 @@ struct clk *clk_register_composite(struct device
> *dev, const char *name, 
>  		composite->gate = gate;
>  		composite->gate_ops = gate_ops;
> -		composite_ops->enable = clk_composite_enable;
> -		composite_ops->disable = clk_composite_disable;
>  		gate->data = (ulong)composite;
>  	}
>  
> @@ -160,6 +165,14 @@ err:
>  	return clk;
>  }
>  
> +static const struct clk_ops clk_composite_ops = {
> +	.set_parent = clk_composite_set_parent,
> +	.get_rate = clk_composite_recalc_rate,
> +	.set_rate = clk_composite_set_rate,
> +	.enable = clk_composite_enable,
> +	.disable = clk_composite_disable,
> +};
> +
>  U_BOOT_DRIVER(clk_composite) = {
>  	.name	= UBOOT_DM_CLK_COMPOSITE,
>  	.id	= UCLASS_CLK,




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200126/807181c9/attachment.sig>

[PATCH v2 03/11] riscv: Add headers for asm/global_data.h

[Lukas Auer]

Hi Sean,


On Wed, 2020-01-15 at 17:50 -0500, Sean Anderson wrote:
> This header depended on bd_t and ulong, but did not include the appropriate
> headers.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>  arch/riscv/include/asm/global_data.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/global_data.h b/arch/riscv/include/asm/global_data.h
> index b74bd7e738..4f0c12b402 100644
> --- a/arch/riscv/include/asm/global_data.h
> +++ b/arch/riscv/include/asm/global_data.h
> @@ -11,6 +11,8 @@
>  #define __ASM_GBL_DATA_H
>  
>  #include <asm/smp.h>
> +#include <asm/u-boot.h>
> +#include <linux/compiler.h>
>  

asm/u-boot.h is usually included with common.h. ulong is defined in
linux/types.h (also included in common.h). It should be sufficient to
include common.h in your source files.

Thanks,
Lukas

[PATCH v2 06/11] riscv: Fix incorrect cpu frequency on RV64

[Lukas Auer]

On Wed, 2020-01-15 at 17:55 -0500, Sean Anderson wrote:

> The riscv_cpu_get_info function does not always zero-out cpu_freq. This can
> cause spurious higher frequencies.
> 
> Signed-off-by Sean Anderson <seanga2@gmail.com>
> ---
> Changes for v2:
>   New.
> 
>  drivers/cpu/riscv_cpu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: Lukas Auer <lukas@auer.io>

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Sean Anderson]

Hi Lukasz,

Thanks for the feedback.

On 1/26/20 4:20 PM, Lukasz Majewski wrote:
> Hi Sean,
> 
>> CCF clocks should always use the struct clock passed to their methods
>> for extracting the driver-specific clock information struct.
> 
> This couldn't be done for i.MX8 at least. There was an issue with
> composite clock on this SoC.
> 
> (I've CC'ed Peng, who did this work for i.MX8 - I was
> testing/developing the framework for i.MX6Q which doesn't support
> composite clocks).
> 
> For some design decisions and the "overall picture" of CCF , please see
> following doc entry:
> https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt

That documentation was helpful, but it tends to focus more on the "what"
than the "why." Perhaps I can help update it when I have a better grasp
on the CCF.

>> Previously, many functions would use the clk->dev->priv if the device
>> was bound. This could cause problems with composite clocks. 
> 
> The problem (in short) is with combining Linux CCF design and U-Boot's
> DM (more details in the link above).
> 
> All the clocks are linked with struct udevice (one search the linked
> list for proper clock).
> 
> However, with Linux the "main" clock structure is 'struct clk, which is
> embedded in CLK IP block specific structure (i.e. struct clk_gate2).
> 
> Problem is that from struct clk we do have pointer to struct udevice
> (*dev), but there is no direct pointer "up" from struct udevice to
> struct clk (now we do use udevice's->uclass_priv).
> 
> So far so good ....
> 
> Problem started with iMX8, where we do have a composite clocks, which
> would like to be seen as a single one (like struct pllX), but they
> comprise of a few other "clocks".
> 
> To distinct them the clk_dev_binded() was added, which checks if the
> struct udevice represents "top" composite clock (which shall be visible
> with e.g. 'clk' command)
>
>> The
>> individual clocks in a composite clock did not have the ->dev field
>> filled in. This was fine, because the device-specific clock
>> information would be used. However, since there was no ->dev, there
>> was no way to get the parent clock. 
> 
> Wasn't there any back pointer available? Or do we need to search the
> clocks linked list and look for "bind" flag in top level composite
> clock?

This is just what the clk_get_parent [1] function does.

[1] https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/clk-uclass.c#L447

> 
>> This caused the recalc_rate
>> method of the CCF divider clock to fail. One option would be to use
>> the clk->priv field to get the composite clock and from there get the
>> appropriate parent device. However, this would tie the implementation
>> to the composite clock. In general, different devices should not rely
>> on the contents of ->priv from another device.
> 
> Maybe we shall follow:
> https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt#L40

I think this might be a better option. I just didn't see any other uses of this pointer in the u-boot
code. 

>>
>> The simple solution to this problem is to just always use the
>> supplied struct clock. 
> 
> I think that we took this approach in the past. Unfortunately, this
> caused all clocks being exposed when 'clk' command was run.

This is discussed further below, but an easy option is to just not
register the component clocks.

The real problem with the current approach (IMO) is that there is a
mismatch between the clock structure and the clock function. If one
calls clk_get_rate on some clock, the get_rate function is chosen based
on clk->dev->ops. If that clock is a composite clock, the
clk_composite_get_rate will then choose the *real* get_rate function to
call, and will call it with the appropriate component clock. But then,
the get_rate function says "Aha, I know better than you what clock
should be passed to me" and calls itself with the composite clock struct
instead! This is really unintitive behaviour. If anything, this kind of
behaviour should be moved up to clk_get_rate, where it can't cause any
harm in composite clocks.

> 
> Peng - were there any other issues with i.MX8 composite clock
> implementation?
> 
>> The composite clock now fills in the ->dev
>> pointer of its child clocks. This allows child clocks to make calls
>> like clk_get_parent() without issue.
>>
>> imx avoided the above problem by using a custom get_rate function
>> with composite clocks.
> 
> Do you refer to:
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/imx/clk-composite-8m.c#L30
> 
> There the clk is cast from (struct clk_composite *)clk->data
> 
> (now in U-Boot we do have 4! different implementations of struct clk).
>

Yes. This was really surprising when I saw it the first time, since it
is effectively bypassing clk_composite_*.

>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>  drivers/clk/clk-composite.c    |  8 ++++++++
>>  drivers/clk/clk-divider.c      |  6 ++----
>>  drivers/clk/clk-fixed-factor.c |  3 +--
>>  drivers/clk/clk-gate.c         |  6 ++----
>>  drivers/clk/clk-mux.c          | 12 ++++--------
>>  drivers/clk/imx/clk-gate2.c    |  4 ++--
>>  6 files changed, 19 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
>> index a5626c33d1..d0f273d47f 100644
>> --- a/drivers/clk/clk-composite.c
>> +++ b/drivers/clk/clk-composite.c
>> @@ -145,6 +145,14 @@ struct clk *clk_register_composite(struct device
>> *dev, const char *name, goto err;
>>  	}
>>  
>> +	if (composite->mux)
>> +		composite->mux->dev = clk->dev;
>> +	if (composite->rate)
>> +		composite->rate->dev = clk->dev;
>> +	if (composite->gate)
>> +		composite->gate->dev = clk->dev;
>> +
>> +
>>  	return clk;
>>  
>>  err:
>> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
>> index 822e09b084..bfa05f24a3 100644
>> --- a/drivers/clk/clk-divider.c
>> +++ b/drivers/clk/clk-divider.c
>> @@ -70,8 +70,7 @@ unsigned long divider_recalc_rate(struct clk *hw,
>> unsigned long parent_rate, 
>>  static ulong clk_divider_recalc_rate(struct clk *clk)
>>  {
>> -	struct clk_divider *divider =
>> to_clk_divider(clk_dev_binded(clk) ?
>> -			dev_get_clk_ptr(clk->dev) : clk);
>> +	struct clk_divider *divider = to_clk_divider(clk);
> 
> How do you differentiate the "top" level of composite clock from the
> clocks from which the composite clock is built?
> 
> Now we do use the clk_dev_binded().

With how I am using it, the clocks from which the composite clock are
built are not registered with dm [2]. The only clock which gets bound is
the composite clock. This follows the example in [3]. Since all the
parameters are known at compile-time, I could even just have a big array
with all the struct clk_dividers (or other component clocks) I need.
However, I chose to model the imx code.

[2] https://github.com/Forty-Bot/u-boot/blob/maix_v3/drivers/clk/kendryte/clk.c#L84
[3] https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/imx/clk-composite-8m.c#L117

--Sean

[PATCH v2 05/11] riscv: Add option to disable writes to mcounteren

[Lukas Auer]

+ Bin, Anup, Atish


On Wed, 2020-01-15 at 17:53 -0500, Sean Anderson wrote:
> On the kendryte k210, writes to mcounteren result in an illegal instruction
> exception.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> Changes for v2:
>  Moved forward in the patch series
> 
>  arch/riscv/Kconfig   | 3 +++
>  arch/riscv/cpu/cpu.c | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 9a7b0334c2..4f8c62dcff 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -226,6 +226,9 @@ config XIP
>  	  from a NOR flash memory without copying the code to ram.
>  	  Say yes here if U-Boot boots from flash directly.
>  
> +config SYS_RISCV_NOCOUNTER
> +	bool "Disable accesses to the mcounteren CSR"
> +

Can you rename this to something like RISCV_PRIV_1_9_1?

The k210 implements version 1.9.1 of the privileged spec (if I remember
correctly). The mcounteren CSR doesn't exist in that version and
therefore triggers the illegal instruction exception. By renaming the
config entry, it is clearer why the CSR is missing and is therefore not
accessed.
I am not too familiar with the changes between the versions of the
spec. Are there other parts of the code we need to adapt?

Thanks,
Lukas

>  config STACK_SIZE_SHIFT
>  	int
>  	default 14
> diff --git a/arch/riscv/cpu/cpu.c b/arch/riscv/cpu/cpu.c
> index e457f6acbf..df9eae663c 100644
> --- a/arch/riscv/cpu/cpu.c
> +++ b/arch/riscv/cpu/cpu.c
> @@ -89,7 +89,9 @@ int arch_cpu_init_dm(void)
>  		 * Enable perf counters for cycle, time,
>  		 * and instret counters only
>  		 */
> +#ifndef CONFIG_SYS_RISCV_NOCOUNTER
>  		csr_write(CSR_MCOUNTEREN, GENMASK(2, 0));
> +#endif
>  
>  		/* Disable paging */
>  		if (supports_extension('s'))

[PATCH v2 03/11] riscv: Add headers for asm/global_data.h

[Sean Anderson]

On 1/26/20 5:04 PM, Lukas Auer wrote:
> asm/u-boot.h is usually included with common.h. ulong is defined in
> linux/types.h (also included in common.h). It should be sufficient to
> include common.h in your source files.
> 
> Thanks,
> Lukas

So shouldn't asm/u-boot.h include common.h? Or is that header implicitly
assumed to be included with every source file? Is that documented
anywhere? To me, the "default" assumption is that any header should be
able to be included anywhere and to pull in all of its own dependencies.

--Sean

[PATCH v2 07/11] riscv: Add initial Sipeed Maix support

[Lukas Auer]

Hi Sean,


On Wed, 2020-01-15 at 18:04 -0500, Sean Anderson wrote:
> The Sipeed Maix series is a collection of boards built around the RISC-V
> Kendryte K210 processor. This processor contains several peripherals to
> accelerate neural network processing and other "ai" tasks. This includes a "KPU"
> neural network processor, an audio processor supporting beamforming reception,
> and a digital video port supporting capture and output at VGA resolution. Other
> peripherals include 8M of sram (accessible with and without caching);
> remappable pins, including 40 GPIOs; AES, FFT, and SHA256 accelerators; a DMA
> controller; and I2C, I2S, and SPI controllers. Maix peripherals vary, but
> include spi flash; on-board usb-serial bridges; ports for cameras, displays, and
> sd cards; and ESP32 chips. Currently, only the Sipeed Maix Bit V2.0 (bitm) is
> supported, but the boards are fairly similar.
> 
> Documentation for Maix boards is located at <http://dl.sipeed.com/MAIX/HDK/>;.
> Documentation for the Kendryte K210 is located at
> <https://kendryte.com/downloads/>;. However, hardware details are rather lacking,
> so most technical reference has been taken from the standalone sdk located at
> <https://github.com/kendryte/kendryte-standalone-sdk>;.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>

This patch should be the last in the patch series, because it requires
all other patches in the series.

> ---
> Changes for v2:
>   Select CONFIG_SYS_RISCV_NOCOUNTER.
>   Imply CONFIG_CLK_K210.
>   Remove spurious references to CONFIG_ARCH_K210.
>   Remove many configs from defconfig where the defaults were fine.
>   Add a few "not set" lines to suppress unneeded defaults.
>   Reduce pre-reloc malloc space, now that clocks initialization happens
>   later.
>   
>  arch/riscv/Kconfig                 |  4 ++
>  board/sipeed/maix/Kconfig          | 41 +++++++++++++
>  board/sipeed/maix/MAINTAINERS      | 13 +++++
>  board/sipeed/maix/Makefile         |  5 ++
>  board/sipeed/maix/maix.c           |  9 +++
>  configs/sipeed_maix_bitm_defconfig | 93 ++++++++++++++++++++++++++++++
>  include/configs/sipeed-maix.h      | 19 ++++++
>  7 files changed, 184 insertions(+)
>  create mode 100644 board/sipeed/maix/Kconfig
>  create mode 100644 board/sipeed/maix/MAINTAINERS
>  create mode 100644 board/sipeed/maix/Makefile
>  create mode 100644 board/sipeed/maix/maix.c
>  create mode 100644 configs/sipeed_maix_bitm_defconfig
>  create mode 100644 include/configs/sipeed-maix.h
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 4f8c62dcff..4c62b8dd77 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -20,6 +20,9 @@ config TARGET_QEMU_VIRT
>  config TARGET_SIFIVE_FU540
>  	bool "Support SiFive FU540 Board"
>  
> +config TARGET_SIPEED_MAIX
> +	bool "Support Sipeed Maix Board"
> +
>  endchoice
>  
>  config SYS_ICACHE_OFF
> @@ -53,6 +56,7 @@ source "board/AndesTech/ax25-ae350/Kconfig"
>  source "board/emulation/qemu-riscv/Kconfig"
>  source "board/microchip/mpfs_icicle/Kconfig"
>  source "board/sifive/fu540/Kconfig"
> +source "board/sipeed/maix/Kconfig"
>  
>  # platform-specific options below
>  source "arch/riscv/cpu/ax25/Kconfig"
> diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
> new file mode 100644
> index 0000000000..9259eb34aa
> --- /dev/null
> +++ b/board/sipeed/maix/Kconfig
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
> +
> +if TARGET_SIPEED_MAIX
> +
> +config SYS_BOARD
> +	default "maix"
> +
> +config SYS_VENDOR
> +	default "sipeed"
> +
> +config SYS_CPU
> +	default "generic"
> +
> +config SYS_CONFIG_NAME
> +	default "sipeed-maix"
> +
> +config SYS_TEXT_BASE
> +	default 0x80000000
> +
> +config NR_CPUS
> +	default 2
> +
> +config NR_DRAM_BANKS
> +	default 2
> +
> +config BOARD_SPECIFIC_OPTIONS
> +	def_bool y
> +	select GENERIC_RISCV
> +	select DM_SERIAL
> +	select SIFIVE_SERIAL
> +	select ARCH_DEFAULT_RV64I
> +	select ENV_IS_NOWHERE

ENV_IS_NOWHERE is automatically selected if no other environment
provider is available, so no need to include it here.
Also, why are you not using the SD card to store the environment?

> +	select SYS_RISCV_NOCOUNTER
> +	imply SIFIVE_CLINT
> +	imply SPI
> +	imply DM_GPIO
> +	imply CMD_GPIO
> +	imply SYS_NS16550
> +	imply SYS_MALLOC_F
> +endif
> diff --git a/board/sipeed/maix/MAINTAINERS b/board/sipeed/maix/MAINTAINERS
> new file mode 100644
> index 0000000000..217de45970
> --- /dev/null
> +++ b/board/sipeed/maix/MAINTAINERS
> @@ -0,0 +1,13 @@
> +Sipeed Maix BOARD
> +M:	Sean Anderson <seanga2@gmail.com>
> +S:	Maintained
> +F:	arch/riscv/dts/k210.dtsi
> +F:	arch/riscv/dts/k210-maix-bit.dts
> +F:	arch/riscv/include/asm/k210_sysctl.h
> +F:	arch/riscv/lib/k210_sysctl.c
> +F:	board/sipeed/maix/
> +F:	configs/sipeed_maix_defconfig
> +F:	drivers/clk/kendryte/
> +F:	include/configs/sipeed-maix.h
> +F:	include/dt-bindings/clock/k210-sysctl.h
> +F:	include/dt-bindings/reset/k210-sysctl.h
> diff --git a/board/sipeed/maix/Makefile b/board/sipeed/maix/Makefile
> new file mode 100644
> index 0000000000..4acff5b31e
> --- /dev/null
> +++ b/board/sipeed/maix/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Copyright (c) 2019 Western Digital Corporation or its affiliates.
> +
> +obj-y += maix.o
> diff --git a/board/sipeed/maix/maix.c b/board/sipeed/maix/maix.c
> new file mode 100644
> index 0000000000..f8e773acf7
> --- /dev/null
> +++ b/board/sipeed/maix/maix.c
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
> + */
> +
> +int board_init(void)
> +{
> +	return 0;
> +}
> diff --git a/configs/sipeed_maix_bitm_defconfig b/configs/sipeed_maix_bitm_defconfig
> new file mode 100644
> index 0000000000..f062cc8c58
> --- /dev/null
> +++ b/configs/sipeed_maix_bitm_defconfig
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
> +CONFIG_CREATE_ARCH_SYMLINK=y
> +CONFIG_RISCV=y
> +CONFIG_SYS_ARCH="riscv"
> +CONFIG_SYS_CPU="generic"
> +CONFIG_SYS_VENDOR="sipeed"
> +CONFIG_SYS_BOARD="maix"
> +CONFIG_SYS_CONFIG_NAME="sipeed-maix"
> +CONFIG_SPL_LDSCRIPT="arch/riscv/cpu/u-boot-spl.lds"
> +CONFIG_SYS_TEXT_BASE=0x80000000
> +CONFIG_SYS_MALLOC_F_LEN=0x1000
> +CONFIG_BOARD_SPECIFIC_OPTIONS=y
> +CONFIG_NR_DRAM_BANKS=2
> +CONFIG_BOOTSTAGE_STASH_ADDR=0
> +CONFIG_64BIT=y
> +CONFIG_TARGET_SIPEED_MAIX=y
> +CONFIG_NR_CPUS=2
> +CONFIG_GENERIC_RISCV=y
> +CONFIG_ARCH_DEFAULT_RV64I=y
> +CONFIG_ARCH_RV64I=y
> +CONFIG_CMODEL_MEDLOW=y
> +CONFIG_RISCV_MMODE=y
> +CONFIG_RISCV_ISA_C=y
> +CONFIG_RISCV_ISA_A=y
> +CONFIG_SIFIVE_CLINT=y
> +CONFIG_K210_SYSCTL=y
> +CONFIG_SYS_RISCV_NOCOUNTER=y
> +CONFIG_ARCH_K210=y
> +CONFIG_CC_OPTIMIZE_FOR_SIZE=y
> +CONFIG_SYS_MALLOC_F=y
> +CONFIG_PHYS_64BIT=y
> +# CONFIG_ANDROID_BOOT_IMAGE is not set
> +# CONFIG_FIT is not set
> +# CONFIG_LEGACY_IMAGE_FORMAT is not set
> +CONFIG_LOG=y
> +CONFIG_ARCH_EARLY_INIT_R=y
> +CONFIG_CMDLINE=y
> +CONFIG_CMD_ENV_EXISTS=y
> +CONFIG_CMD_FLASH=y
> +CONFIG_CMD_SF=y
> +CONFIG_CMD_LOG=y
> +# CONFIG_AUTOBOOT is not set
> +# CONFIG_CMD_BDI is not set
> +# CONFIG_CMD_CONSOLE is not set
> +# CONFIG_CMD_BOOTD is not set
> +# CONFIG_CMD_BOOTM is not set
> +# CONFIG_CMD_BOOTZ is not set
> +CONFIG_CMD_BOOTI=y
> +CONFIG_BOOTM_LINUX=y
> +# CONFIG_CMD_ELF is not set
> +CONFIG_CMD_FDT=y
> +CONFIG_SUPPORT_OF_CONTROL=y
> +CONFIG_DTC=y
> +CONFIG_OF_CONTROL=y
> +CONFIG_OF_SEPARATE=y
> +CONFIG_ENV_IS_NOWHERE=y
> +# CONFIG_NET is not set
> +CONFIG_DM=y
> +CONFIG_REGMAP=y
> +CONFIG_SYSCON=y
> +CONFIG_SIMPLE_BUS=y
> +CONFIG_OF_TRANSLATE=y
> +CONFIG_CLK=y
> +CONFIG_CLK_CCF=y
> +CONFIG_CLK_COMPOSITE_CCF=y
> +CONFIG_CPU=y
> +CONFIG_CPU_RISCV=y
> +CONFIG_MMC=y
> +# CONFIG_INPUT is not set
> +CONFIG_DM_MMC=y
> +CONFIG_MMC_SPI=y
> +CONFIG_MMC_QUIRKS=y
> +CONFIG_MTD=y
> +CONFIG_DM_MTD=y
> +CONFIG_DM_SPI_FLASH=y
> +CONFIG_SPI_FLASH=y
> +CONFIG_SPI_FLASH_GIGADEVICE=y
> +# CONFIG_DM_ETH is not set
> +# CONFIG_PCI is not set
> +CONFIG_BAUDRATE=115200
> +CONFIG_SERIAL_PRESENT=y
> +CONFIG_DM_SERIAL=y
> +CONFIG_SIFIVE_SERIAL=y
> +CONFIG_SPI=y
> +CONFIG_DM_SPI=y
> +CONFIG_SPI_MEM=y
> +CONFIG_DESIGNWARE_SPI=y
> +CONFIG_TIMER=y
> +CONFIG_RISCV_TIMER=y
> +CONFIG_PANIC_HANG=y
> +CONFIG_OF_LIBFDT=y
> +# CONFIG_EFI_LOADER is not set

Please use make savedefconfig to generate the defconfig.

> diff --git a/include/configs/sipeed-maix.h b/include/configs/sipeed-maix.h
> new file mode 100644
> index 0000000000..598f7dfdd0
> --- /dev/null
> +++ b/include/configs/sipeed-maix.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
> + */
> +
> +#ifndef CONFIGS_SIPEED_MAIX_H
> +#define CONFIGS_SIPEED_MAIX_H
> +
> +#include <linux/sizes.h>
> +
> +#define CONFIG_SYS_LOAD_ADDR 0x80000000
> +#define CONFIG_SYS_SDRAM_BASE CONFIG_SYS_LOAD_ADDR
> +#define CONFIG_SYS_SDRAM_SIZE SZ_8M
> +/* Start just below AI memory */
> +#define CONFIG_SYS_INIT_SP_ADDR 0x805FFFFF
> +#define CONFIG_SYS_MALLOC_LEN SZ_1K
> +#define CONFIG_SYS_CACHELINE_SIZE 64
> +
> +#endif /* CONFIGS_SIPEED_MAIX_H */

How are you booting images? It would be great to include a default boot
command.
Please also add a short description on the board and how to use U-Boot
with it to the documentation.

Thanks,
Lukas

[PATCH v2 03/11] riscv: Add headers for asm/global_data.h

[Lukas Auer]

On Sun, 2020-01-26 at 17:12 -0500, Sean Anderson wrote:
> On 1/26/20 5:04 PM, Lukas Auer wrote:
> > asm/u-boot.h is usually included with common.h. ulong is defined in
> > linux/types.h (also included in common.h). It should be sufficient to
> > include common.h in your source files.
> > 
> > Thanks,
> > Lukas
> 
> So shouldn't asm/u-boot.h include common.h? Or is that header implicitly
> assumed to be included with every source file? Is that documented
> anywhere? To me, the "default" assumption is that any header should be
> able to be included anywhere and to pull in all of its own dependencies.
> 

You are right, it is not entirely correct like this. I think common.h
is assumed to always be included. Unfortunately, I don't know if this
is documented anywhere.

Thanks,
Lukas

[PATCH v2 05/11] riscv: Add option to disable writes to mcounteren

[Sean Anderson]

On 1/26/20 5:09 PM, Lukas Auer wrote:
> + Bin, Anup, Atish
> 
> 
> On Wed, 2020-01-15 at 17:53 -0500, Sean Anderson wrote:
>> On the kendryte k210, writes to mcounteren result in an illegal instruction
>> exception.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>> Changes for v2:
>>  Moved forward in the patch series
>>
>>  arch/riscv/Kconfig   | 3 +++
>>  arch/riscv/cpu/cpu.c | 2 ++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 9a7b0334c2..4f8c62dcff 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -226,6 +226,9 @@ config XIP
>>  	  from a NOR flash memory without copying the code to ram.
>>  	  Say yes here if U-Boot boots from flash directly.
>>  
>> +config SYS_RISCV_NOCOUNTER
>> +	bool "Disable accesses to the mcounteren CSR"
>> +
> 
> Can you rename this to something like RISCV_PRIV_1_9_1?
> 
> The k210 implements version 1.9.1 of the privileged spec (if I remember
> correctly). The mcounteren CSR doesn't exist in that version and
> therefore triggers the illegal instruction exception. By renaming the
> config entry, it is clearer why the CSR is missing and is therefore not
> accessed.

Thanks, I was not aware that the k210 was following a different spec
when I made the change. For v3 I can add this functionality back using
the old counter CSRs.

> I am not too familiar with the changes between the versions of the
> spec. Are there other parts of the code we need to adapt?

From reading the changelog, most of the changes seem related to virtual
memory, which doesn't apply to u-boot.

--Sean

[PATCH v2 07/11] riscv: Add initial Sipeed Maix support

[Sean Anderson]

On 1/26/20 5:17 PM, Lukas Auer wrote:
> Hi Sean,
> 
> 
> On Wed, 2020-01-15 at 18:04 -0500, Sean Anderson wrote:
>> The Sipeed Maix series is a collection of boards built around the RISC-V
>> Kendryte K210 processor. This processor contains several peripherals to
>> accelerate neural network processing and other "ai" tasks. This includes a "KPU"
>> neural network processor, an audio processor supporting beamforming reception,
>> and a digital video port supporting capture and output at VGA resolution. Other
>> peripherals include 8M of sram (accessible with and without caching);
>> remappable pins, including 40 GPIOs; AES, FFT, and SHA256 accelerators; a DMA
>> controller; and I2C, I2S, and SPI controllers. Maix peripherals vary, but
>> include spi flash; on-board usb-serial bridges; ports for cameras, displays, and
>> sd cards; and ESP32 chips. Currently, only the Sipeed Maix Bit V2.0 (bitm) is
>> supported, but the boards are fairly similar.
>>
>> Documentation for Maix boards is located at <http://dl.sipeed.com/MAIX/HDK/>;.
>> Documentation for the Kendryte K210 is located at
>> <https://kendryte.com/downloads/>;. However, hardware details are rather lacking,
>> so most technical reference has been taken from the standalone sdk located at
>> <https://github.com/kendryte/kendryte-standalone-sdk>;.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> 
> This patch should be the last in the patch series, because it requires
> all other patches in the series.

Ok, will reorder it for v3.

>> ---
>> Changes for v2:
>>   Select CONFIG_SYS_RISCV_NOCOUNTER.
>>   Imply CONFIG_CLK_K210.
>>   Remove spurious references to CONFIG_ARCH_K210.
>>   Remove many configs from defconfig where the defaults were fine.
>>   Add a few "not set" lines to suppress unneeded defaults.
>>   Reduce pre-reloc malloc space, now that clocks initialization happens
>>   later.
>>   
>>  arch/riscv/Kconfig                 |  4 ++
>>  board/sipeed/maix/Kconfig          | 41 +++++++++++++
>>  board/sipeed/maix/MAINTAINERS      | 13 +++++
>>  board/sipeed/maix/Makefile         |  5 ++
>>  board/sipeed/maix/maix.c           |  9 +++
>>  configs/sipeed_maix_bitm_defconfig | 93 ++++++++++++++++++++++++++++++
>>  include/configs/sipeed-maix.h      | 19 ++++++
>>  7 files changed, 184 insertions(+)
>>  create mode 100644 board/sipeed/maix/Kconfig
>>  create mode 100644 board/sipeed/maix/MAINTAINERS
>>  create mode 100644 board/sipeed/maix/Makefile
>>  create mode 100644 board/sipeed/maix/maix.c
>>  create mode 100644 configs/sipeed_maix_bitm_defconfig
>>  create mode 100644 include/configs/sipeed-maix.h
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 4f8c62dcff..4c62b8dd77 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -20,6 +20,9 @@ config TARGET_QEMU_VIRT
>>  config TARGET_SIFIVE_FU540
>>  	bool "Support SiFive FU540 Board"
>>  
>> +config TARGET_SIPEED_MAIX
>> +	bool "Support Sipeed Maix Board"
>> +
>>  endchoice
>>  
>>  config SYS_ICACHE_OFF
>> @@ -53,6 +56,7 @@ source "board/AndesTech/ax25-ae350/Kconfig"
>>  source "board/emulation/qemu-riscv/Kconfig"
>>  source "board/microchip/mpfs_icicle/Kconfig"
>>  source "board/sifive/fu540/Kconfig"
>> +source "board/sipeed/maix/Kconfig"
>>  
>>  # platform-specific options below
>>  source "arch/riscv/cpu/ax25/Kconfig"
>> diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
>> new file mode 100644
>> index 0000000000..9259eb34aa
>> --- /dev/null
>> +++ b/board/sipeed/maix/Kconfig
>> @@ -0,0 +1,41 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +# Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
>> +
>> +if TARGET_SIPEED_MAIX
>> +
>> +config SYS_BOARD
>> +	default "maix"
>> +
>> +config SYS_VENDOR
>> +	default "sipeed"
>> +
>> +config SYS_CPU
>> +	default "generic"
>> +
>> +config SYS_CONFIG_NAME
>> +	default "sipeed-maix"
>> +
>> +config SYS_TEXT_BASE
>> +	default 0x80000000
>> +
>> +config NR_CPUS
>> +	default 2
>> +
>> +config NR_DRAM_BANKS
>> +	default 2
>> +
>> +config BOARD_SPECIFIC_OPTIONS
>> +	def_bool y
>> +	select GENERIC_RISCV
>> +	select DM_SERIAL
>> +	select SIFIVE_SERIAL
>> +	select ARCH_DEFAULT_RV64I
>> +	select ENV_IS_NOWHERE
> 
> ENV_IS_NOWHERE is automatically selected if no other environment
> provider is available, so no need to include it here.

Ok, I will remove that in v3. As a general rule, what sort of things
should be in Kconfig vs in the default config? I initially thought that
all dependencies should be in Kconfig, but "imply" just seems to set the
default, which means it won't be changed if you pick another board and
then pick this one.

> Also, why are you not using the SD card to store the environment?

I haven't managed to get my SD card working yet. I think it may be too
big (it's 32G), so I am planning to find a smaller one. 

>> +	select SYS_RISCV_NOCOUNTER
>> +	imply SIFIVE_CLINT
>> +	imply SPI
>> +	imply DM_GPIO
>> +	imply CMD_GPIO
>> +	imply SYS_NS16550
>> +	imply SYS_MALLOC_F
>> +endif
>> diff --git a/board/sipeed/maix/MAINTAINERS b/board/sipeed/maix/MAINTAINERS
>> new file mode 100644
>> index 0000000000..217de45970
>> --- /dev/null
>> +++ b/board/sipeed/maix/MAINTAINERS
>> @@ -0,0 +1,13 @@
>> +Sipeed Maix BOARD
>> +M:	Sean Anderson <seanga2@gmail.com>
>> +S:	Maintained
>> +F:	arch/riscv/dts/k210.dtsi
>> +F:	arch/riscv/dts/k210-maix-bit.dts
>> +F:	arch/riscv/include/asm/k210_sysctl.h
>> +F:	arch/riscv/lib/k210_sysctl.c
>> +F:	board/sipeed/maix/
>> +F:	configs/sipeed_maix_defconfig
>> +F:	drivers/clk/kendryte/
>> +F:	include/configs/sipeed-maix.h
>> +F:	include/dt-bindings/clock/k210-sysctl.h
>> +F:	include/dt-bindings/reset/k210-sysctl.h
>> diff --git a/board/sipeed/maix/Makefile b/board/sipeed/maix/Makefile
>> new file mode 100644
>> index 0000000000..4acff5b31e
>> --- /dev/null
>> +++ b/board/sipeed/maix/Makefile
>> @@ -0,0 +1,5 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +#
>> +# Copyright (c) 2019 Western Digital Corporation or its affiliates.
>> +
>> +obj-y += maix.o
>> diff --git a/board/sipeed/maix/maix.c b/board/sipeed/maix/maix.c
>> new file mode 100644
>> index 0000000000..f8e773acf7
>> --- /dev/null
>> +++ b/board/sipeed/maix/maix.c
>> @@ -0,0 +1,9 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
>> + */
>> +
>> +int board_init(void)
>> +{
>> +	return 0;
>> +}
>> diff --git a/configs/sipeed_maix_bitm_defconfig b/configs/sipeed_maix_bitm_defconfig
>> new file mode 100644
>> index 0000000000..f062cc8c58
>> --- /dev/null
>> +++ b/configs/sipeed_maix_bitm_defconfig
>> @@ -0,0 +1,93 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +# Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
>> +CONFIG_CREATE_ARCH_SYMLINK=y
>> +CONFIG_RISCV=y
>> +CONFIG_SYS_ARCH="riscv"
>> +CONFIG_SYS_CPU="generic"
>> +CONFIG_SYS_VENDOR="sipeed"
>> +CONFIG_SYS_BOARD="maix"
>> +CONFIG_SYS_CONFIG_NAME="sipeed-maix"
>> +CONFIG_SPL_LDSCRIPT="arch/riscv/cpu/u-boot-spl.lds"
>> +CONFIG_SYS_TEXT_BASE=0x80000000
>> +CONFIG_SYS_MALLOC_F_LEN=0x1000
>> +CONFIG_BOARD_SPECIFIC_OPTIONS=y
>> +CONFIG_NR_DRAM_BANKS=2
>> +CONFIG_BOOTSTAGE_STASH_ADDR=0
>> +CONFIG_64BIT=y
>> +CONFIG_TARGET_SIPEED_MAIX=y
>> +CONFIG_NR_CPUS=2
>> +CONFIG_GENERIC_RISCV=y
>> +CONFIG_ARCH_DEFAULT_RV64I=y
>> +CONFIG_ARCH_RV64I=y
>> +CONFIG_CMODEL_MEDLOW=y
>> +CONFIG_RISCV_MMODE=y
>> +CONFIG_RISCV_ISA_C=y
>> +CONFIG_RISCV_ISA_A=y
>> +CONFIG_SIFIVE_CLINT=y
>> +CONFIG_K210_SYSCTL=y
>> +CONFIG_SYS_RISCV_NOCOUNTER=y
>> +CONFIG_ARCH_K210=y
>> +CONFIG_CC_OPTIMIZE_FOR_SIZE=y
>> +CONFIG_SYS_MALLOC_F=y
>> +CONFIG_PHYS_64BIT=y
>> +# CONFIG_ANDROID_BOOT_IMAGE is not set
>> +# CONFIG_FIT is not set
>> +# CONFIG_LEGACY_IMAGE_FORMAT is not set
>> +CONFIG_LOG=y
>> +CONFIG_ARCH_EARLY_INIT_R=y
>> +CONFIG_CMDLINE=y
>> +CONFIG_CMD_ENV_EXISTS=y
>> +CONFIG_CMD_FLASH=y
>> +CONFIG_CMD_SF=y
>> +CONFIG_CMD_LOG=y
>> +# CONFIG_AUTOBOOT is not set
>> +# CONFIG_CMD_BDI is not set
>> +# CONFIG_CMD_CONSOLE is not set
>> +# CONFIG_CMD_BOOTD is not set
>> +# CONFIG_CMD_BOOTM is not set
>> +# CONFIG_CMD_BOOTZ is not set
>> +CONFIG_CMD_BOOTI=y
>> +CONFIG_BOOTM_LINUX=y
>> +# CONFIG_CMD_ELF is not set
>> +CONFIG_CMD_FDT=y
>> +CONFIG_SUPPORT_OF_CONTROL=y
>> +CONFIG_DTC=y
>> +CONFIG_OF_CONTROL=y
>> +CONFIG_OF_SEPARATE=y
>> +CONFIG_ENV_IS_NOWHERE=y
>> +# CONFIG_NET is not set
>> +CONFIG_DM=y
>> +CONFIG_REGMAP=y
>> +CONFIG_SYSCON=y
>> +CONFIG_SIMPLE_BUS=y
>> +CONFIG_OF_TRANSLATE=y
>> +CONFIG_CLK=y
>> +CONFIG_CLK_CCF=y
>> +CONFIG_CLK_COMPOSITE_CCF=y
>> +CONFIG_CPU=y
>> +CONFIG_CPU_RISCV=y
>> +CONFIG_MMC=y
>> +# CONFIG_INPUT is not set
>> +CONFIG_DM_MMC=y
>> +CONFIG_MMC_SPI=y
>> +CONFIG_MMC_QUIRKS=y
>> +CONFIG_MTD=y
>> +CONFIG_DM_MTD=y
>> +CONFIG_DM_SPI_FLASH=y
>> +CONFIG_SPI_FLASH=y
>> +CONFIG_SPI_FLASH_GIGADEVICE=y
>> +# CONFIG_DM_ETH is not set
>> +# CONFIG_PCI is not set
>> +CONFIG_BAUDRATE=115200
>> +CONFIG_SERIAL_PRESENT=y
>> +CONFIG_DM_SERIAL=y
>> +CONFIG_SIFIVE_SERIAL=y
>> +CONFIG_SPI=y
>> +CONFIG_DM_SPI=y
>> +CONFIG_SPI_MEM=y
>> +CONFIG_DESIGNWARE_SPI=y
>> +CONFIG_TIMER=y
>> +CONFIG_RISCV_TIMER=y
>> +CONFIG_PANIC_HANG=y
>> +CONFIG_OF_LIBFDT=y
>> +# CONFIG_EFI_LOADER is not set
> 
> Please use make savedefconfig to generate the defconfig.

Ah, I wasn't aware that that was the preferred way.

>> diff --git a/include/configs/sipeed-maix.h b/include/configs/sipeed-maix.h
>> new file mode 100644
>> index 0000000000..598f7dfdd0
>> --- /dev/null
>> +++ b/include/configs/sipeed-maix.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
>> + */
>> +
>> +#ifndef CONFIGS_SIPEED_MAIX_H
>> +#define CONFIGS_SIPEED_MAIX_H
>> +
>> +#include <linux/sizes.h>
>> +
>> +#define CONFIG_SYS_LOAD_ADDR 0x80000000
>> +#define CONFIG_SYS_SDRAM_BASE CONFIG_SYS_LOAD_ADDR
>> +#define CONFIG_SYS_SDRAM_SIZE SZ_8M
>> +/* Start just below AI memory */
>> +#define CONFIG_SYS_INIT_SP_ADDR 0x805FFFFF
>> +#define CONFIG_SYS_MALLOC_LEN SZ_1K
>> +#define CONFIG_SYS_CACHELINE_SIZE 64
>> +
>> +#endif /* CONFIGS_SIPEED_MAIX_H */
> 
> How are you booting images? It would be great to include a default boot
> command.

The board I am using has some serial issues which makes it difficult to
flash any large images. I can get u-boot to flash after several
attempts, but something like the linux kernel is far too unreliable to
test regularly. I'm going to see if I can get my hands on another k210
board which hopefully doesn't have these issues.

> Please also add a short description on the board and how to use U-Boot
> with it to the documentation.

Will something like the commit message work?

> 
> Thanks,
> Lukas
> 

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Lukasz Majewski]

Hi Sean,

> Hi Lukasz,
> 
> Thanks for the feedback.
> 
> On 1/26/20 4:20 PM, Lukasz Majewski wrote:
> > Hi Sean,
> >   
> >> CCF clocks should always use the struct clock passed to their
> >> methods for extracting the driver-specific clock information
> >> struct.  
> > 
> > This couldn't be done for i.MX8 at least. There was an issue with
> > composite clock on this SoC.
> > 
> > (I've CC'ed Peng, who did this work for i.MX8 - I was
> > testing/developing the framework for i.MX6Q which doesn't support
> > composite clocks).
> > 
> > For some design decisions and the "overall picture" of CCF , please
> > see following doc entry:
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt
> >  
> 
> That documentation was helpful, but it tends to focus more on the
> "what" than the "why." Perhaps I can help update it when I have a
> better grasp on the CCF.

Update is more than welcome, as I was the only author of this document.
(It would be good to have it checked from the other "angle").

> 
> >> Previously, many functions would use the clk->dev->priv if the
> >> device was bound. This could cause problems with composite clocks.
> >>   
> > 
> > The problem (in short) is with combining Linux CCF design and
> > U-Boot's DM (more details in the link above).
> > 
> > All the clocks are linked with struct udevice (one search the linked
> > list for proper clock).
> > 
> > However, with Linux the "main" clock structure is 'struct clk,
> > which is embedded in CLK IP block specific structure (i.e. struct
> > clk_gate2).
> > 
> > Problem is that from struct clk we do have pointer to struct udevice
> > (*dev), but there is no direct pointer "up" from struct udevice to
> > struct clk (now we do use udevice's->uclass_priv).
> > 
> > So far so good ....
> > 
> > Problem started with iMX8, where we do have a composite clocks,
> > which would like to be seen as a single one (like struct pllX), but
> > they comprise of a few other "clocks".
> > 
> > To distinct them the clk_dev_binded() was added, which checks if the
> > struct udevice represents "top" composite clock (which shall be
> > visible with e.g. 'clk' command)
> >  
> >> The
> >> individual clocks in a composite clock did not have the ->dev field
> >> filled in. This was fine, because the device-specific clock
> >> information would be used. However, since there was no ->dev, there
> >> was no way to get the parent clock.   
> > 
> > Wasn't there any back pointer available? Or do we need to search the
> > clocks linked list and look for "bind" flag in top level composite
> > clock?  
> 
> This is just what the clk_get_parent [1] function does.
> 
> [1]
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/clk-uclass.c#L447

Ach. Right.

Then the struct clk is used - and as those clocks are not bound to
device (to avoid having it visible in e.g. dm tree command output) the
clk is passed instead. 

It works because the struct clk_composite (defined @ clk-provider.h)
has embedded struct clk in it and with container_of we get clk_ops....


> 
> >   
> >> This caused the recalc_rate
> >> method of the CCF divider clock to fail. One option would be to use
> >> the clk->priv field to get the composite clock and from there get
> >> the appropriate parent device. However, this would tie the
> >> implementation to the composite clock. In general, different
> >> devices should not rely on the contents of ->priv from another
> >> device.  
> > 
> > Maybe we shall follow:
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt#L40
> >  
> 
> I think this might be a better option. I just didn't see any other
> uses of this pointer in the u-boot code. 
> 
> >>
> >> The simple solution to this problem is to just always use the
> >> supplied struct clock.   
> > 
> > I think that we took this approach in the past. Unfortunately, this
> > caused all clocks being exposed when 'clk' command was run.  
> 
> This is discussed further below, but an easy option is to just not
> register the component clocks.

You probably mean to not register clocks which 'struct clk' is embedded
in struct clk_composite?

Yes, this is what we do want - as we get:

	foo (composite clock):
		----------------> pll2
				  ----------> MUX2
					      ------> gate2

And in dm tree we do want to only see the 'foo'.

> 
> The real problem with the current approach (IMO) is that there is a
> mismatch between the clock structure and the clock function. If one
> calls clk_get_rate on some clock, the get_rate function is chosen
> based on clk->dev->ops.

Yes, exactly. This seems logical to me -> the "top" structure is struct
clk, which is a device with proper ops.
(And in U-Boot the 'central' data structure with DM is struct udevice).

> If that clock is a composite clock, the
> clk_composite_get_rate 

I've found only clk_composite_set_rate @ drivers/clk/clk-composite.c

But, yes it calls its own clk_ops (*mux_ops, *rate_ops, *gate_ops).

> will then choose the *real* get_rate function
> to call, and will call it with the appropriate component clock. 

Yes, the struct clk_composite has several clocks defined.

> But
> then, the get_rate function says "Aha, I know better than you what
> clock should be passed to me" and calls itself with the composite
> clock struct instead!

Wouldn't clk_get_rate just return 0 when it discovers that clk->dev is
NULL for not bound driver (the clk which builds a composite driver)?

> This is really unintitive behaviour. If
> anything, this kind of behaviour should be moved up to clk_get_rate,
> where it can't cause any harm in composite clocks.

Even better, the composite clocks shall be handled in the same way as
non composite ones.


To achieve that:

1. The struct clk shall be passed to all clk functions (IIRC this is
now true in U-Boot):
	- then clk IP block specific structure (e.g. struct clk_gate2)
	  are accessible via container_of on clk pointer

	- There shall be clk->dev filled always. In the dev one shall
	  use dev->ops to do the necessary work (even for composite
	  clocks components)

	- The struct clk has flags field (clk->flags). New flags:
		-- CCF_CLK_COMPOSITE_REGISTERED (visible with dm tree)
		-- CCF_CLK_COMPOSITE_HIDDEN     (used internally to
		gate, mux, etc. the composite clock)

		
		-- clk->dev->flags with CCF_CLK_COMPOSITE_REGISTERED
		set puts all "servant" clocks to its child_head list
		(clk->dev->child_head).

		__OR__ 

		-- we extend the ccf core to use struct uc_clk_priv
		(as described:
		https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt#L40)
		which would have

		struct uc_clk_priv {
			struct clk *c; /* back pointer to clk */
			
			struct clk_composite *cc;
		};

		struct clk_composite {
			struct clk *mux;
			struct clk *gate;
			...
			(no ops here - they shall be in struct udevice)
		};

		The overhead is to have extra 4 bytes (or use union and
		check CCF_CLK_COMPOSITE flags).


For me the more natural seems the approach with using
clk->dev->child_head (maybe with some extra new flags defined). U-Boot
handles lists pretty well and maybe OF_PLATDATA could be used as well.


Sean, Peng, Simon, feel free to give your ideas and comments about
possible solutions.

> 
> > 
> > Peng - were there any other issues with i.MX8 composite clock
> > implementation?
> >   
> >> The composite clock now fills in the ->dev
> >> pointer of its child clocks. This allows child clocks to make calls
> >> like clk_get_parent() without issue.
> >>
> >> imx avoided the above problem by using a custom get_rate function
> >> with composite clocks.  
> > 
> > Do you refer to:
> > https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/imx/clk-composite-8m.c#L30
> > 
> > There the clk is cast from (struct clk_composite *)clk->data
> > 
> > (now in U-Boot we do have 4! different implementations of struct
> > clk). 
> 
> Yes. This was really surprising when I saw it the first time, since it
> is effectively bypassing clk_composite_*.

Unfortunately, two struct clks are from Broadcomm devices, one for some
Linux video port and the fourth supports U-Boot's driver model (DM).

> 
> >>
> >> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >> ---
> >>  drivers/clk/clk-composite.c    |  8 ++++++++
> >>  drivers/clk/clk-divider.c      |  6 ++----
> >>  drivers/clk/clk-fixed-factor.c |  3 +--
> >>  drivers/clk/clk-gate.c         |  6 ++----
> >>  drivers/clk/clk-mux.c          | 12 ++++--------
> >>  drivers/clk/imx/clk-gate2.c    |  4 ++--
> >>  6 files changed, 19 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/clk/clk-composite.c
> >> b/drivers/clk/clk-composite.c index a5626c33d1..d0f273d47f 100644
> >> --- a/drivers/clk/clk-composite.c
> >> +++ b/drivers/clk/clk-composite.c
> >> @@ -145,6 +145,14 @@ struct clk *clk_register_composite(struct
> >> device *dev, const char *name, goto err;
> >>  	}
> >>  
> >> +	if (composite->mux)
> >> +		composite->mux->dev = clk->dev;
> >> +	if (composite->rate)
> >> +		composite->rate->dev = clk->dev;
> >> +	if (composite->gate)
> >> +		composite->gate->dev = clk->dev;
> >> +
> >> +
> >>  	return clk;
> >>  
> >>  err:
> >> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> >> index 822e09b084..bfa05f24a3 100644
> >> --- a/drivers/clk/clk-divider.c
> >> +++ b/drivers/clk/clk-divider.c
> >> @@ -70,8 +70,7 @@ unsigned long divider_recalc_rate(struct clk *hw,
> >> unsigned long parent_rate, 
> >>  static ulong clk_divider_recalc_rate(struct clk *clk)
> >>  {
> >> -	struct clk_divider *divider =
> >> to_clk_divider(clk_dev_binded(clk) ?
> >> -			dev_get_clk_ptr(clk->dev) : clk);
> >> +	struct clk_divider *divider = to_clk_divider(clk);  
> > 
> > How do you differentiate the "top" level of composite clock from the
> > clocks from which the composite clock is built?
> > 
> > Now we do use the clk_dev_binded().  
> 
> With how I am using it, the clocks from which the composite clock are
> built are not registered with dm [2]. The only clock which gets bound
> is the composite clock. This follows the example in [3]. Since all the
> parameters are known at compile-time, I could even just have a big
> array with all the struct clk_dividers (or other component clocks) I
> need. However, I chose to model the imx code.
> 
> [2]
> https://github.com/Forty-Bot/u-boot/blob/maix_v3/drivers/clk/kendryte/clk.c#L84
> [3]
> https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/clk/imx/clk-composite-8m.c#L117
> 
> --Sean
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200128/8e31ea41/attachment.sig>

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Sean Anderson]

On 1/27/20 6:40 PM, Lukasz Majewski wrote:
>> The real problem with the current approach (IMO) is that there is a
>> mismatch between the clock structure and the clock function. If one
>> calls clk_get_rate on some clock, the get_rate function is chosen
>> based on clk->dev->ops.
> 
> Yes, exactly. This seems logical to me -> the "top" structure is struct
> clk, which is a device with proper ops.
> (And in U-Boot the 'central' data structure with DM is struct udevice).
> 
>> If that clock is a composite clock, the
>> clk_composite_get_rate 
> 
> I've found only clk_composite_set_rate @ drivers/clk/clk-composite.c
> 
> But, yes it calls its own clk_ops (*mux_ops, *rate_ops, *gate_ops).
> 
>> will then choose the *real* get_rate function
>> to call, and will call it with the appropriate component clock. 
> 
> Yes, the struct clk_composite has several clocks defined.
> 
>> But
>> then, the get_rate function says "Aha, I know better than you what
>> clock should be passed to me" and calls itself with the composite
>> clock struct instead!
> 
> Wouldn't clk_get_rate just return 0 when it discovers that clk->dev is
> NULL for not bound driver (the clk which builds a composite driver)?

Yes, but then clk_get_parent throws a fit, which gets called by
clk_gate_*, clk_fixed_divider_*, clk_mux_*, etc.  So this description is
really for the case where only the first section of this patch is
applied.

>> This is really unintitive behaviour. If
>> anything, this kind of behaviour should be moved up to clk_get_rate,
>> where it can't cause any harm in composite clocks.
> 
> Even better, the composite clocks shall be handled in the same way as
> non composite ones.
> 
> 
> To achieve that:
> 
> 1. The struct clk shall be passed to all clk functions (IIRC this is
> now true in U-Boot):
> 	- then clk IP block specific structure (e.g. struct clk_gate2)
> 	  are accessible via container_of on clk pointer

Ok, so I'm a bit confused about the design decisions here. It seems to
me that there are two uses for struct clk:
	- struct clk as used by drivers not using the CCF. Here, the
	  structure is effectively an extended parameter list,
	  containing all the data needed to operate on some clock.
	  clk->dev->priv contains whatever the driver wants, and doesn't
	  necessarily have a struct clk. Because these clocks are mostly
	  just parameters, they can be created with xlate and request;
	  there is no one "canonical" struct clk for any given logical
	  clock. These clocks can appear on a device tree, and be
	  acquired by clk_get_by_*.
	- struct clk as used by CCF clocks. Here the structure contains
	  specific information configuring each clock. Each struct clk
	  refers to a different logical clock. clk->dev->priv contains a
	  struct clk (though this may not be the same struct clk). These
	  clocks cannot appear in a device tree, and hence cannot be
	  acquired by clk_get_by_* (except for clk_get_by_id which
	  doesn't use the device tree). These clocks are not created by
	  xlate and request.
With this in mind, I'm not sure why one would ever need to call
dev_get_clk_ptr. In the first case, clk->dev->priv could be anything,
and probably not a clock. In the second case, one already has the
"canonical" struct clk.

> 	- There shall be clk->dev filled always. In the dev one shall
> 	  use dev->ops to do the necessary work (even for composite
> 	  clocks components)

Do you mean having a struct device for every clock, even components of a
composite clock? I think this is unnecessary, especially for a system
with lots of composite clocks. Storing the clock ops in the composite
clock's struct works fine, and is conceptually simple.

> 
> 	- The struct clk has flags field (clk->flags). New flags:
> 		-- CCF_CLK_COMPOSITE_REGISTERED (visible with dm tree)
> 		-- CCF_CLK_COMPOSITE_HIDDEN     (used internally to
> 		gate, mux, etc. the composite clock)
> 
> 		
> 		-- clk->dev->flags with CCF_CLK_COMPOSITE_REGISTERED
> 		set puts all "servant" clocks to its child_head list
> 		(clk->dev->child_head).
> 
> 		__OR__ 
> 
> 		-- we extend the ccf core to use struct uc_clk_priv
> 		(as described:
> 		https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt#L40)
> 		which would have
> 
> 		struct uc_clk_priv {
> 			struct clk *c; /* back pointer to clk */
> 			
> 			struct clk_composite *cc;
> 		};
> 
> 		struct clk_composite {
> 			struct clk *mux;
> 			struct clk *gate;
> 			...
> 			(no ops here - they shall be in struct udevice)
> 		};
> 
> 		The overhead is to have extra 4 bytes (or use union and
> 		check CCF_CLK_COMPOSITE flags).
> 
> 
> For me the more natural seems the approach with using
> clk->dev->child_head (maybe with some extra new flags defined). U-Boot
> handles lists pretty well and maybe OF_PLATDATA could be used as well.
> 

I like the private data approach, but couldn't we use the existing
clk->data field? E.g. instead of having

struct clk_foo {
	struct clk clk;
	/* ... */
};

clk_register_foo(...)
{
	struct clk_foo *foo;
	struct clk *clk;

	foo = kzalloc(sizeof(*foo));
	/* ... */

	clk = foo->clk;
	clk_register(...);
}

int clk_foo_get_rate(struct clk *clk)
{
	struct clk_foo *foo = to_clk_foo(clk);
	/* ... */
}

we do

struct clk_foo {
	/* ... */
};

clk_register_foo(...)
{
	struct clk_foo *foo;
	struct clk *clk;

	foo = kzalloc(sizeof(*foo));
	clk = kzalloc(sizeof(*clk));
	/* ... */

	clk->data = foo;
	clk_register(...);
}

int clk_foo_get_rate(struct clk *clk)
{
	struct clk_foo *foo = (struct clk_foo *)clk->data;
	/* ... */
}

Of course, now that I have written this all out, it really feels like
the container_of approach all over again...

I think the best way of approaching this may be to simply introduce a
get_parent op. CCF already does something like this with
clk_mux_get_parent. This would also allow for some clocks to have a NULL
->dev.

--Sean

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Lukasz Majewski]

Hi Sean,

> On 1/27/20 6:40 PM, Lukasz Majewski wrote:
> >> The real problem with the current approach (IMO) is that there is a
> >> mismatch between the clock structure and the clock function. If one
> >> calls clk_get_rate on some clock, the get_rate function is chosen
> >> based on clk->dev->ops.  
> > 
> > Yes, exactly. This seems logical to me -> the "top" structure is
> > struct clk, which is a device with proper ops.
> > (And in U-Boot the 'central' data structure with DM is struct
> > udevice). 
> >> If that clock is a composite clock, the
> >> clk_composite_get_rate   
> > 
> > I've found only clk_composite_set_rate @ drivers/clk/clk-composite.c
> > 
> > But, yes it calls its own clk_ops (*mux_ops, *rate_ops, *gate_ops).
> >   
> >> will then choose the *real* get_rate function
> >> to call, and will call it with the appropriate component clock.   
> > 
> > Yes, the struct clk_composite has several clocks defined.
> >   
> >> But
> >> then, the get_rate function says "Aha, I know better than you what
> >> clock should be passed to me" and calls itself with the composite
> >> clock struct instead!  
> > 
> > Wouldn't clk_get_rate just return 0 when it discovers that clk->dev
> > is NULL for not bound driver (the clk which builds a composite
> > driver)?  
> 
> Yes, but then clk_get_parent throws a fit, which gets called by

Could you explain what "throw a fit" means here? I'm a bit confused.

> clk_gate_*, clk_fixed_divider_*, clk_mux_*, etc.  So this description
> is really for the case where only the first section of this patch is
> applied.
> 
> >> This is really unintitive behaviour. If
> >> anything, this kind of behaviour should be moved up to
> >> clk_get_rate, where it can't cause any harm in composite clocks.  
> > 
> > Even better, the composite clocks shall be handled in the same way
> > as non composite ones.
> > 
> > 
> > To achieve that:
> > 
> > 1. The struct clk shall be passed to all clk functions (IIRC this is
> > now true in U-Boot):
> > 	- then clk IP block specific structure (e.g. struct
> > clk_gate2) are accessible via container_of on clk pointer  
> 
> Ok, so I'm a bit confused about the design decisions here. It seems to
> me that there are two uses for struct clk:
> 	- struct clk as used by drivers not using the CCF. Here, the
> 	  structure is effectively an extended parameter list,
> 	  containing all the data needed to operate on some clock.
> 	  clk->dev->priv contains whatever the driver wants, and
> doesn't necessarily have a struct clk. Because these clocks are mostly
> 	  just parameters, they can be created with xlate and request;
> 	  there is no one "canonical" struct clk for any given logical
> 	  clock. These clocks can appear on a device tree, and be
> 	  acquired by clk_get_by_*.

Yes.

> 	- struct clk as used by CCF clocks. Here the structure
> contains specific information configuring each clock. Each struct clk
> 	  refers to a different logical clock. clk->dev->priv
> contains a struct clk (though this may not be the same struct clk).

The struct clk pointer is now stored in the struct's udevice
uclass_priv pointer.

For CCF it looks like below:

struct clk_gate2 -> struct clk -> struct udevice *dev -> struct udevice
		       /|\				    |
			|				    |
			-------------uclass_priv<------------
						

> These clocks cannot appear in a device tree

I think they could, but I've followed the approach of Linux CCF in e.g.
i.MX6Q SoC.

>, and hence cannot be
> 	  acquired by clk_get_by_* (except for clk_get_by_id which
> 	  doesn't use the device tree). These clocks are not created
> by xlate and request.

Correct. Those clocks are instantiated in SoC specific file. For
example in ./drivers/clk/imx/clk-imx6q.c


> With this in mind, I'm not sure why one would ever need to call
> dev_get_clk_ptr. In the first case, clk->dev->priv could be anything,
> and probably not a clock. In the second case, one already has the
> "canonical" struct clk.

The problem is that clocks are linked together with struct udevice
(_NOT_ struct clk). All clocks are linked together and have the same
uclass - UCLASS_CLK.

To access clock - one needs to search this doubly linked list and you
get struct udevice from it.
(for example in ./cmd/clk.c)

And then you need a "back pointer" to struct clk to use clock
ops/functions. And this back pointer is stored in struct udevice's
uclass_priv pointer (accessed via dev_get_clk_ptr).

> 
> > 	- There shall be clk->dev filled always. In the dev one
> > shall use dev->ops to do the necessary work (even for composite
> > 	  clocks components)  
> 
> Do you mean having a struct device for every clock, even components
> of a composite clock? I think this is unnecessary, especially for a
> system with lots of composite clocks. Storing the clock ops in the
> composite clock's struct works fine, and is conceptually simple.

I agree. We shall avoid creating/instantiating unnecessary udevices.

> 
> > 
> > 	- The struct clk has flags field (clk->flags). New flags:
> > 		-- CCF_CLK_COMPOSITE_REGISTERED (visible with dm
> > tree) -- CCF_CLK_COMPOSITE_HIDDEN     (used internally to
> > 		gate, mux, etc. the composite clock)
> > 
> > 		
> > 		-- clk->dev->flags with CCF_CLK_COMPOSITE_REGISTERED
> > 		set puts all "servant" clocks to its child_head list
> > 		(clk->dev->child_head).
> > 
> > 		__OR__ 
> > 
> > 		-- we extend the ccf core to use struct uc_clk_priv
> > 		(as described:
> > 		https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt#L40)
> > 		which would have
> > 
> > 		struct uc_clk_priv {
> > 			struct clk *c; /* back pointer to clk */
> > 			
> > 			struct clk_composite *cc;
> > 		};
> > 
> > 		struct clk_composite {
> > 			struct clk *mux;
> > 			struct clk *gate;
> > 			...
> > 			(no ops here - they shall be in struct
> > udevice) };
> > 
> > 		The overhead is to have extra 4 bytes (or use union
> > and check CCF_CLK_COMPOSITE flags).
> > 
> > 
> > For me the more natural seems the approach with using
> > clk->dev->child_head (maybe with some extra new flags defined).
> > U-Boot handles lists pretty well and maybe OF_PLATDATA could be
> > used as well. 
> 
> I like the private data approach, but couldn't we use the existing
> clk->data field? E.g. instead of having
> 
> struct clk_foo {
> 	struct clk clk;

This is the approach took from the Linux kernel. This is somewhat
similar to having the struct clk_hw:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/imx/clk-gate2.c#L27

> 	/* ... */
> };
> 
> clk_register_foo(...)
> {
> 	struct clk_foo *foo;
> 	struct clk *clk;
> 
> 	foo = kzalloc(sizeof(*foo));
> 	/* ... */
> 
> 	clk = foo->clk;
> 	clk_register(...);
> }
> 
> int clk_foo_get_rate(struct clk *clk)
> {
> 	struct clk_foo *foo = to_clk_foo(clk);
> 	/* ... */
> }
> 
> we do
> 
> struct clk_foo {
> 	/* ... */
> };
> 
> clk_register_foo(...)
> {
> 	struct clk_foo *foo;
> 	struct clk *clk;
> 
> 	foo = kzalloc(sizeof(*foo));
> 	clk = kzalloc(sizeof(*clk));
> 	/* ... */
> 
> 	clk->data = foo;

According to the description of struct clk definition (@
./include/clk.h) the data field has other purposes.

Maybe we shall add a new member of struct clk?

> 	clk_register(...);
> }
> 
> int clk_foo_get_rate(struct clk *clk)
> {
> 	struct clk_foo *foo = (struct clk_foo *)clk->data;
> 	/* ... */
> }
> 
> Of course, now that I have written this all out, it really feels like
> the container_of approach all over again...

Indeed. Even the access cost is the same as the struct clk is always
placed as the first element of e.g. struct clk_gate2

> 
> I think the best way of approaching this may be to simply introduce a
> get_parent op. CCF already does something like this with
> clk_mux_get_parent. This would also allow for some clocks to have a
> NULL ->dev.

I've thought about this some time ago and wondered if struct clk
shouldn't be extended a bit. 

Maybe adding there a pointer to "get_parent" would simplify the overall
design of CCF?

Then one could set this callback pointer in e.g. clk_register_gate2 (or
clk_register_composite) and set clk->dev to NULL to indicate
"composite" clock?

So we would have:

static inline bool is_composite_clk(struct clk *clk)
{
	return !clk->dev && clk->flags == CCF_CLK_COMPOSITE;
}

I'm just wondering if "normal" clocks shall set this clk->get_parent
pointer or continue to use clk->dev->parent?

> 
> --Sean




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200130/0431a6cb/attachment.sig>

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Sean Anderson]

Hi Lukasz,

On 1/29/20 7:29 PM, Lukasz Majewski wrote:
>> Yes, but then clk_get_parent throws a fit, which gets called by
> 
> Could you explain what "throw a fit" means here? I'm a bit confused.

Ok, so imagine I have a clk_divider in a composite clock. When
clk_get_rate gets called on the composite clock, the composite clock
then calls clk_divider_get_rate on the divider clock. The first thing
that function does is call clk_get_parent_rate, which in turn calls
clk_get_parent. This last call fails because the divider clock has a
NULL ->dev. The end behaviour is that the parent rate looks like 0,
which causes the divider to in turn return 0 as its rate. So while it
doesn't throw a fit, we still end up with a bad result.

>> 	- struct clk as used by CCF clocks. Here the structure
>> contains specific information configuring each clock. Each struct clk
>> 	  refers to a different logical clock. clk->dev->priv
>> contains a struct clk (though this may not be the same struct clk).
> 
> The struct clk pointer is now stored in the struct's udevice
> uclass_priv pointer.
> 
> For CCF it looks like below:
> 
> struct clk_gate2 -> struct clk -> struct udevice *dev -> struct udevice
> 		       /|\				    |
> 			|				    |
> 			-------------uclass_priv<------------
> 					
Right, so at best doing dev_get_clk_ptr(clk->dev) in something like
clk_divider_set_rate is a no-op, and at worst it breaks something.

>> These clocks cannot appear in a device tree
> 
> I think they could, but I've followed the approach of Linux CCF in e.g.
> i.MX6Q SoC.

They could, but none of them have compatible strings at the moment.

>> , and hence cannot be
>> 	  acquired by clk_get_by_* (except for clk_get_by_id which
>> 	  doesn't use the device tree). These clocks are not created
>> by xlate and request.
> 
> Correct. Those clocks are instantiated in SoC specific file. For
> example in ./drivers/clk/imx/clk-imx6q.c
> 
> 
>> With this in mind, I'm not sure why one would ever need to call
>> dev_get_clk_ptr. In the first case, clk->dev->priv could be anything,
>> and probably not a clock. In the second case, one already has the
>> "canonical" struct clk.
> 
> The problem is that clocks are linked together with struct udevice
> (_NOT_ struct clk). All clocks are linked together and have the same
> uclass - UCLASS_CLK.
> 
> To access clock - one needs to search this doubly linked list and you
> get struct udevice from it.
> (for example in ./cmd/clk.c)
> 
> And then you need a "back pointer" to struct clk to use clock
> ops/functions. And this back pointer is stored in struct udevice's
> uclass_priv pointer (accessed via dev_get_clk_ptr).

Right, but clock implementations will never need to do this. Whatever
clock they get passed is going to be the clock they should use. This is
why I think the calls which I removed were unnecessary.

In fact, through this discussion I think the patch as-submitted is
probably the least-disruptive way to make composite clocks work in a
useful way. The only thing it does is that sometimes clk->dev->priv !=
clk, but I don't think that anyone was relying on this being the case.

>>> 	- The struct clk has flags field (clk->flags). New flags:
>>> 		-- CCF_CLK_COMPOSITE_REGISTERED (visible with dm
>>> tree) -- CCF_CLK_COMPOSITE_HIDDEN     (used internally to
>>> 		gate, mux, etc. the composite clock)
>>>
>>> 		
>>> 		-- clk->dev->flags with CCF_CLK_COMPOSITE_REGISTERED
>>> 		set puts all "servant" clocks to its child_head list
>>> 		(clk->dev->child_head).
>>>
>>> 		__OR__ 
>>>
>>> 		-- we extend the ccf core to use struct uc_clk_priv
>>> 		(as described:
>>> 		https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt#L40)
>>> 		which would have
>>>
>>> 		struct uc_clk_priv {
>>> 			struct clk *c; /* back pointer to clk */
>>> 			
>>> 			struct clk_composite *cc;
>>> 		};
>>>
>>> 		struct clk_composite {
>>> 			struct clk *mux;
>>> 			struct clk *gate;
>>> 			...
>>> 			(no ops here - they shall be in struct
>>> udevice) };
>>>
>>> 		The overhead is to have extra 4 bytes (or use union
>>> and check CCF_CLK_COMPOSITE flags).
>>>
>>>
>>> For me the more natural seems the approach with using
>>> clk->dev->child_head (maybe with some extra new flags defined).
>>> U-Boot handles lists pretty well and maybe OF_PLATDATA could be
>>> used as well. 
>>
>> I like the private data approach, but couldn't we use the existing
>> clk->data field? E.g. instead of having
>>
>> struct clk_foo {
>> 	struct clk clk;
> 
> This is the approach took from the Linux kernel. This is somewhat
> similar to having the struct clk_hw:
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/imx/clk-gate2.c#L27



>> 	/* ... */
>> };
>>
>> clk_register_foo(...)
>> {
>> 	struct clk_foo *foo;
>> 	struct clk *clk;
>>
>> 	foo = kzalloc(sizeof(*foo));
>> 	/* ... */
>>
>> 	clk = foo->clk;
>> 	clk_register(...);
>> }
>>
>> int clk_foo_get_rate(struct clk *clk)
>> {
>> 	struct clk_foo *foo = to_clk_foo(clk);
>> 	/* ... */
>> }
>>
>> we do
>>
>> struct clk_foo {
>> 	/* ... */
>> };
>>
>> clk_register_foo(...)
>> {
>> 	struct clk_foo *foo;
>> 	struct clk *clk;
>>
>> 	foo = kzalloc(sizeof(*foo));
>> 	clk = kzalloc(sizeof(*clk));
>> 	/* ... */
>>
>> 	clk->data = foo;
> 
> According to the description of struct clk definition (@
> ./include/clk.h) the data field has other purposes.
> 
> Maybe we shall add a new member of struct clk?

Well, the CCF doesn't use xlate or register, so this field is unused at
the moment.

> 
>> 	clk_register(...);
>> }
>>
>> int clk_foo_get_rate(struct clk *clk)
>> {
>> 	struct clk_foo *foo = (struct clk_foo *)clk->data;
>> 	/* ... */
>> }
>>
>> Of course, now that I have written this all out, it really feels like
>> the container_of approach all over again...
> 
> Indeed. Even the access cost is the same as the struct clk is always
> placed as the first element of e.g. struct clk_gate2
> 
>>
>> I think the best way of approaching this may be to simply introduce a
>> get_parent op. CCF already does something like this with
>> clk_mux_get_parent. This would also allow for some clocks to have a
>> NULL ->dev.
> 
> I've thought about this some time ago and wondered if struct clk
> shouldn't be extended a bit. 
> 
> Maybe adding there a pointer to "get_parent" would simplify the overall
> design of CCF?
> 
> Then one could set this callback pointer in e.g. clk_register_gate2 (or
> clk_register_composite) and set clk->dev to NULL to indicate
> "composite" clock?
> 
> So we would have:
> 
> static inline bool is_composite_clk(struct clk *clk)
> {
> 	return !clk->dev && clk->flags == CCF_CLK_COMPOSITE;
> }
 

What would use that predicate?

> I'm just wondering if "normal" clocks shall set this clk->get_parent
> pointer or continue to use clk->dev->parent?

Hm, well after thinking it over, I think with the current system having
a method for this would not do anything useful, since you still need to
get the ops from the udevice (and then you could just use the default
behaviour).

If I could make big sweeping changes clock uclass, I would
do something like:
	- Split off of_xlate, request, and free into a new
	  clk_device_ops struct, with an optional pointer to clk_ops
	- Create a new struct clk_handle containing id, data, a pointer to
	  struct udevice, and a pointer to struct clk
	- Add get_parent to clk_ops and change all ops to work on struct
	  clk_handle
	- Add a pointer to clk_ops in struct clk, and remove dev, id,
	  and data.

So for users, the api would look like

struct clk_handle clk = clk_get_by_index(dev, 0, &clk);
clk_enable(&clk);
ulong rate = clk_get_rate(&clk);

Method calls would roughly look like

ulong clk_get_rate(struct clk_handle *h)
{
	struct clk_ops *ops;

	if (h->clk)
		ops = h->clk->ops;
	else
		ops = clk_dev_ops(h->dev)->ops;
	return ops->get_rate(h);
}

Getting the parent would use h->clk->ops->get_parent if it exists, and
otherwise use h->dev to figure it out.

For non-CCF drivers, the implementation could look something like

ulong foo_get_rate(struct clk_handle *h)
{
	/* Do something with h->id and h->dev */
}

struct foo_clk_ops = {
	.get_rate = foo_get_rate,
};

struct foo_clk_driver_ops = {
	.ops = &foo_clk_ops,
};

For drivers using the CCF, the implementation could look like

struct clk_gate *foo_gate;

int foo_probe(struct udevice *dev)
{
	foo_gate = /* ... */;
}

int foo_request(struct clk_handle *h)
{
	h->clk = foo_gate->clk;
}

struct foo_clk_driver_ops = {
	.request = foo_request,
};

So why these changes?
	- Clear separation between the different duties which are both
	  currently handled by struct clk
	- Simplification for drivers using the CCF
	- Minimal changes for existing users
		- Clock consumers have almost the same api
		- Every clock driver will need to be changed, but most
		  drivers which don't use CCF never use any fields in
		  clk other than ->dev and ->id
	- No need to get the "canonical" clock, since it is pointed at
	  by clk_handle->clk
	- Reduction in memory/driver usage since CCF clocks no longer
	  need a udevice for each clock
	- Less function calls since each driver using CCF no longer
	  needs to be a proxy for clock ops

Now these are big changes, and definitely would be the subject of their
own patch series. As-is, I think the patch as it exists now is the best
way to get the most functionality from clk_composite with the least
changes to other code.

	--Sean

[PATCH v2 05/11] riscv: Add option to disable writes to mcounteren

[Lukas Auer]

On Sun, 2020-01-26 at 17:24 -0500, Sean Anderson wrote:
> On 1/26/20 5:09 PM, Lukas Auer wrote:
> > + Bin, Anup, Atish
> > 
> > 
> > On Wed, 2020-01-15 at 17:53 -0500, Sean Anderson wrote:
> > > On the kendryte k210, writes to mcounteren result in an illegal instruction
> > > exception.
> > > 
> > > Signed-off-by: Sean Anderson <seanga2@gmail.com>
> > > ---
> > > Changes for v2:
> > >  Moved forward in the patch series
> > > 
> > >  arch/riscv/Kconfig   | 3 +++
> > >  arch/riscv/cpu/cpu.c | 2 ++
> > >  2 files changed, 5 insertions(+)
> > > 
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 9a7b0334c2..4f8c62dcff 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -226,6 +226,9 @@ config XIP
> > >  	  from a NOR flash memory without copying the code to ram.
> > >  	  Say yes here if U-Boot boots from flash directly.
> > >  
> > > +config SYS_RISCV_NOCOUNTER
> > > +	bool "Disable accesses to the mcounteren CSR"
> > > +
> > 
> > Can you rename this to something like RISCV_PRIV_1_9_1?
> > 
> > The k210 implements version 1.9.1 of the privileged spec (if I remember
> > correctly). The mcounteren CSR doesn't exist in that version and
> > therefore triggers the illegal instruction exception. By renaming the
> > config entry, it is clearer why the CSR is missing and is therefore not
> > accessed.
> 
> Thanks, I was not aware that the k210 was following a different spec
> when I made the change. For v3 I can add this functionality back using
> the old counter CSRs.
> 
> > I am not too familiar with the changes between the versions of the
> > spec. Are there other parts of the code we need to adapt?
> 
> From reading the changelog, most of the changes seem related to virtual
> memory, which doesn't apply to u-boot.
> 

Ok, great. Thanks for checking!

Regards,
Lukas

[PATCH v2 07/11] riscv: Add initial Sipeed Maix support

[Lukas Auer]

On Sun, 2020-01-26 at 20:09 -0500, Sean Anderson wrote:
> On 1/26/20 5:17 PM, Lukas Auer wrote:
> > Hi Sean,
> > 
> > 
> > On Wed, 2020-01-15 at 18:04 -0500, Sean Anderson wrote:
> > > The Sipeed Maix series is a collection of boards built around the RISC-V
> > > Kendryte K210 processor. This processor contains several peripherals to
> > > accelerate neural network processing and other "ai" tasks. This includes a "KPU"
> > > neural network processor, an audio processor supporting beamforming reception,
> > > and a digital video port supporting capture and output at VGA resolution. Other
> > > peripherals include 8M of sram (accessible with and without caching);
> > > remappable pins, including 40 GPIOs; AES, FFT, and SHA256 accelerators; a DMA
> > > controller; and I2C, I2S, and SPI controllers. Maix peripherals vary, but
> > > include spi flash; on-board usb-serial bridges; ports for cameras, displays, and
> > > sd cards; and ESP32 chips. Currently, only the Sipeed Maix Bit V2.0 (bitm) is
> > > supported, but the boards are fairly similar.
> > > 
> > > Documentation for Maix boards is located at <http://dl.sipeed.com/MAIX/HDK/>;;.
> > > Documentation for the Kendryte K210 is located at
> > > <https://kendryte.com/downloads/>;;. However, hardware details are rather lacking,
> > > so most technical reference has been taken from the standalone sdk located at
> > > <https://github.com/kendryte/kendryte-standalone-sdk>;;.
> > > 
> > > Signed-off-by: Sean Anderson <seanga2@gmail.com>
> > 
> > This patch should be the last in the patch series, because it requires
> > all other patches in the series.
> 
> Ok, will reorder it for v3.
> 
> > > ---
> > > Changes for v2:
> > >   Select CONFIG_SYS_RISCV_NOCOUNTER.
> > >   Imply CONFIG_CLK_K210.
> > >   Remove spurious references to CONFIG_ARCH_K210.
> > >   Remove many configs from defconfig where the defaults were fine.
> > >   Add a few "not set" lines to suppress unneeded defaults.
> > >   Reduce pre-reloc malloc space, now that clocks initialization happens
> > >   later.
> > >   
> > >  arch/riscv/Kconfig                 |  4 ++
> > >  board/sipeed/maix/Kconfig          | 41 +++++++++++++
> > >  board/sipeed/maix/MAINTAINERS      | 13 +++++
> > >  board/sipeed/maix/Makefile         |  5 ++
> > >  board/sipeed/maix/maix.c           |  9 +++
> > >  configs/sipeed_maix_bitm_defconfig | 93 ++++++++++++++++++++++++++++++
> > >  include/configs/sipeed-maix.h      | 19 ++++++
> > >  7 files changed, 184 insertions(+)
> > >  create mode 100644 board/sipeed/maix/Kconfig
> > >  create mode 100644 board/sipeed/maix/MAINTAINERS
> > >  create mode 100644 board/sipeed/maix/Makefile
> > >  create mode 100644 board/sipeed/maix/maix.c
> > >  create mode 100644 configs/sipeed_maix_bitm_defconfig
> > >  create mode 100644 include/configs/sipeed-maix.h
> > > 
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 4f8c62dcff..4c62b8dd77 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -20,6 +20,9 @@ config TARGET_QEMU_VIRT
> > >  config TARGET_SIFIVE_FU540
> > >  	bool "Support SiFive FU540 Board"
> > >  
> > > +config TARGET_SIPEED_MAIX
> > > +	bool "Support Sipeed Maix Board"
> > > +
> > >  endchoice
> > >  
> > >  config SYS_ICACHE_OFF
> > > @@ -53,6 +56,7 @@ source "board/AndesTech/ax25-ae350/Kconfig"
> > >  source "board/emulation/qemu-riscv/Kconfig"
> > >  source "board/microchip/mpfs_icicle/Kconfig"
> > >  source "board/sifive/fu540/Kconfig"
> > > +source "board/sipeed/maix/Kconfig"
> > >  
> > >  # platform-specific options below
> > >  source "arch/riscv/cpu/ax25/Kconfig"
> > > diff --git a/board/sipeed/maix/Kconfig b/board/sipeed/maix/Kconfig
> > > new file mode 100644
> > > index 0000000000..9259eb34aa
> > > --- /dev/null
> > > +++ b/board/sipeed/maix/Kconfig
> > > @@ -0,0 +1,41 @@
> > > +# SPDX-License-Identifier: GPL-2.0+
> > > +# Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
> > > +
> > > +if TARGET_SIPEED_MAIX
> > > +
> > > +config SYS_BOARD
> > > +	default "maix"
> > > +
> > > +config SYS_VENDOR
> > > +	default "sipeed"
> > > +
> > > +config SYS_CPU
> > > +	default "generic"
> > > +
> > > +config SYS_CONFIG_NAME
> > > +	default "sipeed-maix"
> > > +
> > > +config SYS_TEXT_BASE
> > > +	default 0x80000000
> > > +
> > > +config NR_CPUS
> > > +	default 2
> > > +
> > > +config NR_DRAM_BANKS
> > > +	default 2
> > > +
> > > +config BOARD_SPECIFIC_OPTIONS
> > > +	def_bool y
> > > +	select GENERIC_RISCV
> > > +	select DM_SERIAL
> > > +	select SIFIVE_SERIAL
> > > +	select ARCH_DEFAULT_RV64I
> > > +	select ENV_IS_NOWHERE
> > 
> > ENV_IS_NOWHERE is automatically selected if no other environment
> > provider is available, so no need to include it here.
> 
> Ok, I will remove that in v3. As a general rule, what sort of things
> should be in Kconfig vs in the default config? I initially thought that
> all dependencies should be in Kconfig, but "imply" just seems to set the
> default, which means it won't be changed if you pick another board and
> then pick this one.
> 

The Kconfig forms the base configuration, which will be shared by all
defconfig variations of the board. So yes, you are correct, it should
include all its dependencies such as the peripheral drivers, but also
commands that use the peripherals. In the SiFive FU540 configuration,
for example, we imply the peripheral drivers (clock, serial, etc.),
enable filesystem related options since we have storage, and enable
relevant commands.

Not everybody will want or need all configuration options. We therefore
try to imply as many of the options as possible, so that they can be
deselected as required in the defconfig. They should all be picked up,
if you apply different boards. Perhaps you did not run make distclean?

> > Also, why are you not using the SD card to store the environment?
> 
> I haven't managed to get my SD card working yet. I think it may be too
> big (it's 32G), so I am planning to find a smaller one. 
> 

Interesting, I thought all SD cards support SPI mode.

> > > +	select SYS_RISCV_NOCOUNTER
> > > +	imply SIFIVE_CLINT
> > > +	imply SPI
> > > +	imply DM_GPIO
> > > +	imply CMD_GPIO
> > > +	imply SYS_NS16550
> > > +	imply SYS_MALLOC_F
> > > +endif
> > > diff --git a/board/sipeed/maix/MAINTAINERS b/board/sipeed/maix/MAINTAINERS
> > > new file mode 100644
> > > index 0000000000..217de45970
> > > --- /dev/null
> > > +++ b/board/sipeed/maix/MAINTAINERS
> > > @@ -0,0 +1,13 @@
> > > +Sipeed Maix BOARD
> > > +M:	Sean Anderson <seanga2@gmail.com>
> > > +S:	Maintained
> > > +F:	arch/riscv/dts/k210.dtsi
> > > +F:	arch/riscv/dts/k210-maix-bit.dts
> > > +F:	arch/riscv/include/asm/k210_sysctl.h
> > > +F:	arch/riscv/lib/k210_sysctl.c
> > > +F:	board/sipeed/maix/
> > > +F:	configs/sipeed_maix_defconfig
> > > +F:	drivers/clk/kendryte/
> > > +F:	include/configs/sipeed-maix.h
> > > +F:	include/dt-bindings/clock/k210-sysctl.h
> > > +F:	include/dt-bindings/reset/k210-sysctl.h
> > > diff --git a/board/sipeed/maix/Makefile b/board/sipeed/maix/Makefile
> > > new file mode 100644
> > > index 0000000000..4acff5b31e
> > > --- /dev/null
> > > +++ b/board/sipeed/maix/Makefile
> > > @@ -0,0 +1,5 @@
> > > +# SPDX-License-Identifier: GPL-2.0+
> > > +#
> > > +# Copyright (c) 2019 Western Digital Corporation or its affiliates.
> > > +
> > > +obj-y += maix.o
> > > diff --git a/board/sipeed/maix/maix.c b/board/sipeed/maix/maix.c
> > > new file mode 100644
> > > index 0000000000..f8e773acf7
> > > --- /dev/null
> > > +++ b/board/sipeed/maix/maix.c
> > > @@ -0,0 +1,9 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
> > > + */
> > > +
> > > +int board_init(void)
> > > +{
> > > +	return 0;
> > > +}
> > > diff --git a/configs/sipeed_maix_bitm_defconfig b/configs/sipeed_maix_bitm_defconfig
> > > new file mode 100644
> > > index 0000000000..f062cc8c58
> > > --- /dev/null
> > > +++ b/configs/sipeed_maix_bitm_defconfig
> > > @@ -0,0 +1,93 @@
> > > +# SPDX-License-Identifier: GPL-2.0+
> > > +# Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
> > > +CONFIG_CREATE_ARCH_SYMLINK=y
> > > +CONFIG_RISCV=y
> > > +CONFIG_SYS_ARCH="riscv"
> > > +CONFIG_SYS_CPU="generic"
> > > +CONFIG_SYS_VENDOR="sipeed"
> > > +CONFIG_SYS_BOARD="maix"
> > > +CONFIG_SYS_CONFIG_NAME="sipeed-maix"
> > > +CONFIG_SPL_LDSCRIPT="arch/riscv/cpu/u-boot-spl.lds"
> > > +CONFIG_SYS_TEXT_BASE=0x80000000
> > > +CONFIG_SYS_MALLOC_F_LEN=0x1000
> > > +CONFIG_BOARD_SPECIFIC_OPTIONS=y
> > > +CONFIG_NR_DRAM_BANKS=2
> > > +CONFIG_BOOTSTAGE_STASH_ADDR=0
> > > +CONFIG_64BIT=y
> > > +CONFIG_TARGET_SIPEED_MAIX=y
> > > +CONFIG_NR_CPUS=2
> > > +CONFIG_GENERIC_RISCV=y
> > > +CONFIG_ARCH_DEFAULT_RV64I=y
> > > +CONFIG_ARCH_RV64I=y
> > > +CONFIG_CMODEL_MEDLOW=y
> > > +CONFIG_RISCV_MMODE=y
> > > +CONFIG_RISCV_ISA_C=y
> > > +CONFIG_RISCV_ISA_A=y
> > > +CONFIG_SIFIVE_CLINT=y
> > > +CONFIG_K210_SYSCTL=y
> > > +CONFIG_SYS_RISCV_NOCOUNTER=y
> > > +CONFIG_ARCH_K210=y
> > > +CONFIG_CC_OPTIMIZE_FOR_SIZE=y
> > > +CONFIG_SYS_MALLOC_F=y
> > > +CONFIG_PHYS_64BIT=y
> > > +# CONFIG_ANDROID_BOOT_IMAGE is not set
> > > +# CONFIG_FIT is not set
> > > +# CONFIG_LEGACY_IMAGE_FORMAT is not set
> > > +CONFIG_LOG=y
> > > +CONFIG_ARCH_EARLY_INIT_R=y
> > > +CONFIG_CMDLINE=y
> > > +CONFIG_CMD_ENV_EXISTS=y
> > > +CONFIG_CMD_FLASH=y
> > > +CONFIG_CMD_SF=y
> > > +CONFIG_CMD_LOG=y
> > > +# CONFIG_AUTOBOOT is not set
> > > +# CONFIG_CMD_BDI is not set
> > > +# CONFIG_CMD_CONSOLE is not set
> > > +# CONFIG_CMD_BOOTD is not set
> > > +# CONFIG_CMD_BOOTM is not set
> > > +# CONFIG_CMD_BOOTZ is not set
> > > +CONFIG_CMD_BOOTI=y
> > > +CONFIG_BOOTM_LINUX=y
> > > +# CONFIG_CMD_ELF is not set
> > > +CONFIG_CMD_FDT=y
> > > +CONFIG_SUPPORT_OF_CONTROL=y
> > > +CONFIG_DTC=y
> > > +CONFIG_OF_CONTROL=y
> > > +CONFIG_OF_SEPARATE=y
> > > +CONFIG_ENV_IS_NOWHERE=y
> > > +# CONFIG_NET is not set
> > > +CONFIG_DM=y
> > > +CONFIG_REGMAP=y
> > > +CONFIG_SYSCON=y
> > > +CONFIG_SIMPLE_BUS=y
> > > +CONFIG_OF_TRANSLATE=y
> > > +CONFIG_CLK=y
> > > +CONFIG_CLK_CCF=y
> > > +CONFIG_CLK_COMPOSITE_CCF=y
> > > +CONFIG_CPU=y
> > > +CONFIG_CPU_RISCV=y
> > > +CONFIG_MMC=y
> > > +# CONFIG_INPUT is not set
> > > +CONFIG_DM_MMC=y
> > > +CONFIG_MMC_SPI=y
> > > +CONFIG_MMC_QUIRKS=y
> > > +CONFIG_MTD=y
> > > +CONFIG_DM_MTD=y
> > > +CONFIG_DM_SPI_FLASH=y
> > > +CONFIG_SPI_FLASH=y
> > > +CONFIG_SPI_FLASH_GIGADEVICE=y
> > > +# CONFIG_DM_ETH is not set
> > > +# CONFIG_PCI is not set
> > > +CONFIG_BAUDRATE=115200
> > > +CONFIG_SERIAL_PRESENT=y
> > > +CONFIG_DM_SERIAL=y
> > > +CONFIG_SIFIVE_SERIAL=y
> > > +CONFIG_SPI=y
> > > +CONFIG_DM_SPI=y
> > > +CONFIG_SPI_MEM=y
> > > +CONFIG_DESIGNWARE_SPI=y
> > > +CONFIG_TIMER=y
> > > +CONFIG_RISCV_TIMER=y
> > > +CONFIG_PANIC_HANG=y
> > > +CONFIG_OF_LIBFDT=y
> > > +# CONFIG_EFI_LOADER is not set
> > 
> > Please use make savedefconfig to generate the defconfig.
> 
> Ah, I wasn't aware that that was the preferred way.
> 

No problem, makes it much easier as well. :)

> > > diff --git a/include/configs/sipeed-maix.h b/include/configs/sipeed-maix.h
> > > new file mode 100644
> > > index 0000000000..598f7dfdd0
> > > --- /dev/null
> > > +++ b/include/configs/sipeed-maix.h
> > > @@ -0,0 +1,19 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/*
> > > + * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>
> > > + */
> > > +
> > > +#ifndef CONFIGS_SIPEED_MAIX_H
> > > +#define CONFIGS_SIPEED_MAIX_H
> > > +
> > > +#include <linux/sizes.h>
> > > +
> > > +#define CONFIG_SYS_LOAD_ADDR 0x80000000
> > > +#define CONFIG_SYS_SDRAM_BASE CONFIG_SYS_LOAD_ADDR
> > > +#define CONFIG_SYS_SDRAM_SIZE SZ_8M
> > > +/* Start just below AI memory */
> > > +#define CONFIG_SYS_INIT_SP_ADDR 0x805FFFFF
> > > +#define CONFIG_SYS_MALLOC_LEN SZ_1K
> > > +#define CONFIG_SYS_CACHELINE_SIZE 64
> > > +
> > > +#endif /* CONFIGS_SIPEED_MAIX_H */
> > 
> > How are you booting images? It would be great to include a default boot
> > command.
> 
> The board I am using has some serial issues which makes it difficult to
> flash any large images. I can get u-boot to flash after several
> attempts, but something like the linux kernel is far too unreliable to
> test regularly. I'm going to see if I can get my hands on another k210
> board which hopefully doesn't have these issues.
> 

Ah ok, that's unfortunate. Thank you for your efforts!

> > Please also add a short description on the board and how to use U-Boot
> > with it to the documentation.
> 
> Will something like the commit message work?
> 

Yes definitely. Just add a short description on how to build U-Boot and
flash it onto the board. That makes it a lot easier for new users to
get started with the board.

Thanks,
Lukas

[PATCH v2 01/11] clk: Always use the supplied struct clk

[Lukasz Majewski]

Hi Sean,

> Hi Lukasz,
> 
> On 1/29/20 7:29 PM, Lukasz Majewski wrote:
> >> Yes, but then clk_get_parent throws a fit, which gets called by  
> > 
> > Could you explain what "throw a fit" means here? I'm a bit
> > confused.  
> 
> Ok, so imagine I have a clk_divider in a composite clock. When
> clk_get_rate gets called on the composite clock, the composite clock
> then calls clk_divider_get_rate on the divider clock. The first thing
> that function does is call clk_get_parent_rate, which in turn calls
> clk_get_parent. This last call fails because the divider clock has a
> NULL ->dev. The end behaviour is that the parent rate looks like 0,
> which causes the divider to in turn return 0 as its rate. So while it
> doesn't throw a fit, we still end up with a bad result.

Thanks for the explanation. Now it is clear.

> 
> >> 	- struct clk as used by CCF clocks. Here the structure
> >> contains specific information configuring each clock. Each struct
> >> clk refers to a different logical clock. clk->dev->priv
> >> contains a struct clk (though this may not be the same struct
> >> clk).  
> > 
> > The struct clk pointer is now stored in the struct's udevice
> > uclass_priv pointer.
> > 
> > For CCF it looks like below:
> > 
> > struct clk_gate2 -> struct clk -> struct udevice *dev -> struct
> > udevice /|\				    |
> > 			|				    |
> > 			-------------uclass_priv<------------
> > 					  
> Right, so at best doing dev_get_clk_ptr(clk->dev) in something like
> clk_divider_set_rate is a no-op, and at worst it breaks something.

Yes, correct.

> 
> >> These clocks cannot appear in a device tree  
> > 
> > I think they could, but I've followed the approach of Linux CCF in
> > e.g. i.MX6Q SoC.  
> 
> They could, but none of them have compatible strings at the moment.

Ok.

Just informative - in U-Boot I do use DTS ccf node (for iMX6) to have
this driver probed with DM.

> 
> >> , and hence cannot be
> >> 	  acquired by clk_get_by_* (except for clk_get_by_id which
> >> 	  doesn't use the device tree). These clocks are not
> >> created by xlate and request.  
> > 
> > Correct. Those clocks are instantiated in SoC specific file. For
> > example in ./drivers/clk/imx/clk-imx6q.c
> > 
> >   
> >> With this in mind, I'm not sure why one would ever need to call
> >> dev_get_clk_ptr. In the first case, clk->dev->priv could be
> >> anything, and probably not a clock. In the second case, one
> >> already has the "canonical" struct clk.  
> > 
> > The problem is that clocks are linked together with struct udevice
> > (_NOT_ struct clk). All clocks are linked together and have the same
> > uclass - UCLASS_CLK.
> > 
> > To access clock - one needs to search this doubly linked list and
> > you get struct udevice from it.
> > (for example in ./cmd/clk.c)
> > 
> > And then you need a "back pointer" to struct clk to use clock
> > ops/functions. And this back pointer is stored in struct udevice's
> > uclass_priv pointer (accessed via dev_get_clk_ptr).  
> 
> Right, but clock implementations will never need to do this. Whatever
> clock they get passed is going to be the clock they should use. 

Yes. Correct.

> This
> is why I think the calls which I removed were unnecessary.

Those calls were for composite clocks. And as I've pointed out earlier
the address of e.g. struct clk_gate2 is the same as first its element -
the struct clk in this case.

> 
> In fact, through this discussion I think the patch as-submitted is
> probably the least-disruptive way to make composite clocks work in a
> useful way. The only thing it does is that sometimes clk->dev->priv !=
> clk, but I don't think that anyone was relying on this being the case.

Then - OK. Please resubmit the patch with section added to
doc/imx/clk/ccf.txt

(If possible please also review the rest of this document - more review
- the better).

> 
> >>> 	- The struct clk has flags field (clk->flags). New flags:
> >>> 		-- CCF_CLK_COMPOSITE_REGISTERED (visible with dm
> >>> tree) -- CCF_CLK_COMPOSITE_HIDDEN     (used internally to
> >>> 		gate, mux, etc. the composite clock)
> >>>
> >>> 		
> >>> 		-- clk->dev->flags with
> >>> CCF_CLK_COMPOSITE_REGISTERED set puts all "servant" clocks to its
> >>> child_head list (clk->dev->child_head).
> >>>
> >>> 		__OR__ 
> >>>
> >>> 		-- we extend the ccf core to use struct
> >>> uc_clk_priv (as described:
> >>> 		https://gitlab.denx.de/u-boot/u-boot/blob/master/doc/imx/clk/ccf.txt#L40)
> >>> 		which would have
> >>>
> >>> 		struct uc_clk_priv {
> >>> 			struct clk *c; /* back pointer to clk */
> >>> 			
> >>> 			struct clk_composite *cc;
> >>> 		};
> >>>
> >>> 		struct clk_composite {
> >>> 			struct clk *mux;
> >>> 			struct clk *gate;
> >>> 			...
> >>> 			(no ops here - they shall be in struct
> >>> udevice) };
> >>>
> >>> 		The overhead is to have extra 4 bytes (or use
> >>> union and check CCF_CLK_COMPOSITE flags).
> >>>
> >>>
> >>> For me the more natural seems the approach with using
> >>> clk->dev->child_head (maybe with some extra new flags defined).
> >>> U-Boot handles lists pretty well and maybe OF_PLATDATA could be
> >>> used as well.   
> >>
> >> I like the private data approach, but couldn't we use the existing
> >> clk->data field? E.g. instead of having
> >>
> >> struct clk_foo {
> >> 	struct clk clk;  
> > 
> > This is the approach took from the Linux kernel. This is somewhat
> > similar to having the struct clk_hw:
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/imx/clk-gate2.c#L27
> >  
> 
> 
> 
> >> 	/* ... */
> >> };
> >>
> >> clk_register_foo(...)
> >> {
> >> 	struct clk_foo *foo;
> >> 	struct clk *clk;
> >>
> >> 	foo = kzalloc(sizeof(*foo));
> >> 	/* ... */
> >>
> >> 	clk = foo->clk;
> >> 	clk_register(...);
> >> }
> >>
> >> int clk_foo_get_rate(struct clk *clk)
> >> {
> >> 	struct clk_foo *foo = to_clk_foo(clk);
> >> 	/* ... */
> >> }
> >>
> >> we do
> >>
> >> struct clk_foo {
> >> 	/* ... */
> >> };
> >>
> >> clk_register_foo(...)
> >> {
> >> 	struct clk_foo *foo;
> >> 	struct clk *clk;
> >>
> >> 	foo = kzalloc(sizeof(*foo));
> >> 	clk = kzalloc(sizeof(*clk));
> >> 	/* ... */
> >>
> >> 	clk->data = foo;  
> > 
> > According to the description of struct clk definition (@
> > ./include/clk.h) the data field has other purposes.
> > 
> > Maybe we shall add a new member of struct clk?  
> 
> Well, the CCF doesn't use xlate or register, so this field is unused
> at the moment.

I would prefer to not mix the meaning of struct clk members. The xlate
is supposed to work with DM/DTS, so we shall not add any new meaning for
it.

> 
> >   
> >> 	clk_register(...);
> >> }
> >>
> >> int clk_foo_get_rate(struct clk *clk)
> >> {
> >> 	struct clk_foo *foo = (struct clk_foo *)clk->data;
> >> 	/* ... */
> >> }
> >>
> >> Of course, now that I have written this all out, it really feels
> >> like the container_of approach all over again...  
> > 
> > Indeed. Even the access cost is the same as the struct clk is always
> > placed as the first element of e.g. struct clk_gate2
> >   
> >>
> >> I think the best way of approaching this may be to simply
> >> introduce a get_parent op. CCF already does something like this
> >> with clk_mux_get_parent. This would also allow for some clocks to
> >> have a NULL ->dev.  
> > 
> > I've thought about this some time ago and wondered if struct clk
> > shouldn't be extended a bit. 
> > 
> > Maybe adding there a pointer to "get_parent" would simplify the
> > overall design of CCF?
> > 
> > Then one could set this callback pointer in e.g. clk_register_gate2
> > (or clk_register_composite) and set clk->dev to NULL to indicate
> > "composite" clock?
> > 
> > So we would have:
> > 
> > static inline bool is_composite_clk(struct clk *clk)
> > {
> > 	return !clk->dev && clk->flags == CCF_CLK_COMPOSITE;
> > }  
>  
> 
> What would use that predicate?

This was just my proposition. There is no "solid" use case for this
function.

> 
> > I'm just wondering if "normal" clocks shall set this clk->get_parent
> > pointer or continue to use clk->dev->parent?  
> 
> Hm, well after thinking it over, I think with the current system
> having a method for this would not do anything useful, since you
> still need to get the ops from the udevice (and then you could just
> use the default behaviour).
> 
> If I could make big sweeping changes clock uclass, I would
> do something like:
> 	- Split off of_xlate, request, and free into a new
> 	  clk_device_ops struct, with an optional pointer to clk_ops
> 	- Create a new struct clk_handle containing id, data, a
> pointer to struct udevice, and a pointer to struct clk
> 	- Add get_parent to clk_ops and change all ops to work on
> struct clk_handle
> 	- Add a pointer to clk_ops in struct clk, and remove dev, id,
> 	  and data.
> 
> So for users, the api would look like
> 
> struct clk_handle clk = clk_get_by_index(dev, 0, &clk);
> clk_enable(&clk);
> ulong rate = clk_get_rate(&clk);
> 
> Method calls would roughly look like
> 
> ulong clk_get_rate(struct clk_handle *h)
> {
> 	struct clk_ops *ops;
> 
> 	if (h->clk)
> 		ops = h->clk->ops;
> 	else
> 		ops = clk_dev_ops(h->dev)->ops;
> 	return ops->get_rate(h);
> }
> 
> Getting the parent would use h->clk->ops->get_parent if it exists, and
> otherwise use h->dev to figure it out.
> 
> For non-CCF drivers, the implementation could look something like
> 
> ulong foo_get_rate(struct clk_handle *h)
> {
> 	/* Do something with h->id and h->dev */
> }
> 
> struct foo_clk_ops = {
> 	.get_rate = foo_get_rate,
> };
> 
> struct foo_clk_driver_ops = {
> 	.ops = &foo_clk_ops,
> };
> 
> For drivers using the CCF, the implementation could look like
> 
> struct clk_gate *foo_gate;
> 
> int foo_probe(struct udevice *dev)
> {
> 	foo_gate = /* ... */;
> }
> 
> int foo_request(struct clk_handle *h)
> {
> 	h->clk = foo_gate->clk;
> }
> 
> struct foo_clk_driver_ops = {
> 	.request = foo_request,
> };
> 
> So why these changes?
> 	- Clear separation between the different duties which are both
> 	  currently handled by struct clk
> 	- Simplification for drivers using the CCF
> 	- Minimal changes for existing users
> 		- Clock consumers have almost the same api
> 		- Every clock driver will need to be changed, but most
> 		  drivers which don't use CCF never use any fields in
> 		  clk other than ->dev and ->id
> 	- No need to get the "canonical" clock, since it is pointed at
> 	  by clk_handle->clk
> 	- Reduction in memory/driver usage since CCF clocks no longer
> 	  need a udevice for each clock
> 	- Less function calls since each driver using CCF no longer
> 	  needs to be a proxy for clock ops

Solid arguments.

> 
> Now these are big changes, and definitely would be the subject of
> their own patch series. As-is, I think the patch as it exists now is
> the best way to get the most functionality from clk_composite with
> the least changes to other code.

Yes. I do agree here.

Please resubmit the aforementioned patch (with ccf.txt doc extension). I
will pull it.

Then, if you like, please prepare the patch set for CCF optimization.
We could discuss it in detail until the v2020.04 merge window opens.
There shall be enough time to have an agreement and prepare those
patches for pulling.

> 
> 	--Sean


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200131/958c935b/attachment.sig>

[PATCH v2 07/11] riscv: Add initial Sipeed Maix support

[Sean Anderson]

On 1/30/20 5:21 PM, Lukas Auer wrote:
> Interesting, I thought all SD cards support SPI mode.

I think it's a different issue, but I wasn't sure.

>> The board I am using has some serial issues which makes it difficult to
>> flash any large images. I can get u-boot to flash after several
>> attempts, but something like the linux kernel is far too unreliable to
>> test regularly. I'm going to see if I can get my hands on another k210
>> board which hopefully doesn't have these issues.
> 
> Ah ok, that's unfortunate. Thank you for your efforts!

I have since found a computer where the serial works fine (which implies
that the USB hardware on my current computer has some issues).  I should
be able to flash larger binaries fine now, but the current issue is
getting the SPI to work.  I think there may be some subtle differences
between the driver and the manufacturer's sdk.

I am also having some clock problems, despite (obstensibly) performing
the same incantations as the sdk. I guess this is what happens when they
refuse to release a technical reference...

--Sean