[PATCH v3 0/2] Add support for the OST in Ingenic X1000.

[PATCH v3 0/2] Add support for the OST in Ingenic X1000.

[周琰杰 (Zhou Yanjie)]

v2->v3:
Fix wrong parameters in "clocks".

周琰杰 (Zhou Yanjie) (2):
  dt-bindings: timer: Add Ingenic X1000 OST bindings.
  clocksource: Ingenic: Add support for the Ingenic X1000 OST.

 .../devicetree/bindings/timer/ingenic,ost.yaml     |  61 +++
 drivers/clocksource/Kconfig                        |  19 +-
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/ingenic-sysost.c               | 509 +++++++++++++++++++++
 include/dt-bindings/clock/ingenic,ost.h            |  12 +
 5 files changed, 598 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/timer/ingenic,ost.yaml
 create mode 100755 drivers/clocksource/ingenic-sysost.c
 create mode 100644 include/dt-bindings/clock/ingenic,ost.h

-- 
2.11.0

[PATCH v3 1/2] dt-bindings: timer: Add Ingenic X1000 OST bindings.

[周琰杰 (Zhou Yanjie)]

Add the OST bindings for the X10000 SoC from Ingenic.

Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
---

Notes:
    v1->v2:
    No change.
    
    v2->v3:
    Fix wrong parameters in "clocks".

 .../devicetree/bindings/timer/ingenic,ost.yaml     | 61 ++++++++++++++++++++++
 include/dt-bindings/clock/ingenic,ost.h            | 12 +++++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/ingenic,ost.yaml
 create mode 100644 include/dt-bindings/clock/ingenic,ost.h

diff --git a/Documentation/devicetree/bindings/timer/ingenic,ost.yaml b/Documentation/devicetree/bindings/timer/ingenic,ost.yaml
new file mode 100644
index 000000000000..459dd3d161c2
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ingenic,ost.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/ingenic,ost.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bindings for SYSOST in Ingenic XBurst family SoCs
+
+maintainers:
+  - 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
+
+description:
+  The SYSOST in an Ingenic SoC provides one 64bit timer for clocksource
+  and one or more than one 32bit timers for clockevent.
+
+properties:
+  compatible:
+    oneOf:
+
+      - description: SYSOST in Ingenic XBurst family SoCs
+        enum:
+          - ingenic,x1000-ost
+          - ingenic,x2000-ost
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: ost
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - "#clock-cells"
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+
+examples:
+  - |
+    #include <dt-bindings/clock/x1000-cgu.h>
+
+    ost: timer@12000000 {
+    		compatible = "ingenic,x1000-ost";
+    		reg = <0x12000000 0x3c>;
+
+    		#clock-cells = <1>;
+
+    		clocks = <&cgu X1000_CLK_OST>;
+    		clock-names = "ost";
+
+    		interrupt-parent = <&cpuintc>;
+    		interrupts = <3>;
+    	};
+...
diff --git a/include/dt-bindings/clock/ingenic,ost.h b/include/dt-bindings/clock/ingenic,ost.h
new file mode 100644
index 000000000000..9ac88e90babf
--- /dev/null
+++ b/include/dt-bindings/clock/ingenic,ost.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This header provides clock numbers for the ingenic,tcu DT binding.
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_INGENIC_OST_H__
+#define __DT_BINDINGS_CLOCK_INGENIC_OST_H__
+
+#define OST_CLK_PERCPU_TIMER	0
+#define OST_CLK_GLOBAL_TIMER	1
+
+#endif /* __DT_BINDINGS_CLOCK_INGENIC_OST_H__ */
-- 
2.11.0

[PATCH v3 2/2] clocksource: Ingenic: Add support for the Ingenic X1000 OST.

[周琰杰 (Zhou Yanjie)]

X1000 and SoCs after X1000 (such as X1500 and X1830) had a separate
OST, it no longer belongs to TCU. This driver will register both a
clocksource and a sched_clock to the system.

Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
---

Notes:
    v1->v2:
    Fix compile warnings.
    Reported-by: kernel test robot <lkp@intel.com>
    
    v2->v3:
    No change.

 drivers/clocksource/Kconfig          |  19 +-
 drivers/clocksource/Makefile         |   1 +
 drivers/clocksource/ingenic-sysost.c | 509 +++++++++++++++++++++++++++++++++++
 3 files changed, 525 insertions(+), 4 deletions(-)
 create mode 100644 drivers/clocksource/ingenic-sysost.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 91418381fcd4..172397a00f3e 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -686,7 +686,7 @@ config MILBEAUT_TIMER
 	  Enables the support for Milbeaut timer driver.
 
 config INGENIC_TIMER
-	bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
+	bool "Clocksource/timer using the TCU in Ingenic SoCs"
 	default MACH_INGENIC
 	depends on MIPS || COMPILE_TEST
 	depends on COMMON_CLK
@@ -694,15 +694,26 @@ config INGENIC_TIMER
 	select TIMER_OF
 	select IRQ_DOMAIN
 	help
-	  Support for the timer/counter unit of the Ingenic JZ SoCs.
+	  Support for the timer/counter unit of the Ingenic SoCs.
+
+config INGENIC_SYSOST
+	bool "Clocksource/timer using the SYSOST in Ingenic SoCs"
+	default MACH_INGENIC
+	depends on MIPS || COMPILE_TEST
+	depends on COMMON_CLK
+	select MFD_SYSCON
+	select TIMER_OF
+	select IRQ_DOMAIN
+	help
+	  This option enables support for SYSOST in Ingenic SoCs .
 
 config INGENIC_OST
-	bool "Clocksource for Ingenic OS Timer"
+	bool "Clocksource for Ingenic OST"
 	depends on MIPS || COMPILE_TEST
 	depends on COMMON_CLK
 	select MFD_SYSCON
 	help
-	  Support for the Operating System Timer of the Ingenic JZ SoCs.
+	  Support for the Operating System Timer of the Ingenic SoCs.
 
 config MICROCHIP_PIT64B
 	bool "Microchip PIT64B support"
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index bdda1a2e4097..3994e221e262 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_H8300_TMR8)		+= h8300_timer8.o
 obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
 obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
 obj-$(CONFIG_INGENIC_OST)		+= ingenic-ost.o
+obj-$(CONFIG_INGENIC_SYSOST)	+= ingenic-sysost.o
 obj-$(CONFIG_INGENIC_TIMER)		+= ingenic-timer.o
 obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
diff --git a/drivers/clocksource/ingenic-sysost.c b/drivers/clocksource/ingenic-sysost.c
new file mode 100644
index 000000000000..d3e1b7582221
--- /dev/null
+++ b/drivers/clocksource/ingenic-sysost.c
@@ -0,0 +1,509 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Ingenic XBurst SoCs SYSOST clocks driver
+ * Copyright (c) 2020 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/sched_clock.h>
+#include <linux/slab.h>
+#include <linux/syscore_ops.h>
+
+#include <dt-bindings/clock/ingenic,ost.h>
+
+/* OST register offsets */
+#define OST_REG_OSTCCR			0x00
+#define OST_REG_OSTCR			0x08
+#define OST_REG_OSTFR			0x0c
+#define OST_REG_OSTMR			0x10
+#define OST_REG_OST1DFR			0x14
+#define OST_REG_OST1CNT			0x18
+#define OST_REG_OST2CNTL		0x20
+#define OST_REG_OSTCNT2HBUF		0x24
+#define OST_REG_OSTESR			0x34
+#define OST_REG_OSTECR			0x38
+
+/* bits within the OSTCCR register */
+#define OSTCCR_PRESCALE1_MASK	0x3
+#define OSTCCR_PRESCALE2_MASK	0xc
+#define OSTCCR_PRESCALE1_LSB	0
+#define OSTCCR_PRESCALE2_LSB	2
+
+/* bits within the OSTCR register */
+#define OSTCR_OST1CLR			BIT(0)
+#define OSTCR_OST2CLR			BIT(1)
+
+/* bits within the OSTFR register */
+#define OSTFR_FFLAG				BIT(0)
+
+/* bits within the OSTMR register */
+#define OSTMR_FMASK				BIT(0)
+
+/* bits within the OSTESR register */
+#define OSTESR_OST1ENS			BIT(0)
+#define OSTESR_OST2ENS			BIT(1)
+
+/* bits within the OSTECR register */
+#define OSTECR_OST1ENC			BIT(0)
+#define OSTECR_OST2ENC			BIT(1)
+
+enum ost_clk_parent {
+	OST_PARENT_EXT,
+};
+
+struct ingenic_soc_info {
+	unsigned int num_channels;
+};
+
+struct ingenic_ost_clk_info {
+	struct clk_init_data init_data;
+	u8 ostccr_reg;
+};
+
+struct ingenic_ost_clk {
+	struct clk_hw hw;
+	unsigned int idx;
+	struct ingenic_ost *ost;
+	const struct ingenic_ost_clk_info *info;
+};
+
+struct ingenic_ost {
+	void __iomem *base;
+	const struct ingenic_soc_info *soc_info;
+	struct clk *clk, *percpu_timer_clk, *global_timer_clk;
+	unsigned int percpu_timer_channel, global_timer_channel;
+	struct clock_event_device cevt;
+	struct clocksource cs;
+	char name[20];
+
+	struct clk_hw_onecell_data *clocks;
+};
+
+static struct ingenic_ost *ingenic_ost;
+
+static inline struct ingenic_ost_clk *to_ost_clk(struct clk_hw *hw)
+{
+	return container_of(hw, struct ingenic_ost_clk, hw);
+}
+
+static unsigned long ingenic_ost_recalc_rate(struct clk_hw *hw,
+		unsigned long parent_rate)
+{
+	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
+	const struct ingenic_ost_clk_info *info = ost_clk->info;
+	unsigned int prescale;
+
+	prescale = readl(ost_clk->ost->base + info->ostccr_reg);
+
+	if (ost_clk->idx == OST_CLK_PERCPU_TIMER)
+		prescale = (prescale & OSTCCR_PRESCALE1_MASK) >> OSTCCR_PRESCALE1_LSB;
+	else if (ost_clk->idx == OST_CLK_GLOBAL_TIMER)
+		prescale = (prescale & OSTCCR_PRESCALE2_MASK) >> OSTCCR_PRESCALE2_LSB;
+
+	return parent_rate >> (prescale * 2);
+}
+
+static u8 ingenic_ost_get_prescale(unsigned long rate, unsigned long req_rate)
+{
+	u8 prescale;
+
+	for (prescale = 0; prescale < 2; prescale++)
+		if ((rate >> (prescale * 2)) <= req_rate)
+			return prescale;
+
+	return 2; /* /16 divider */
+}
+
+static long ingenic_ost_round_rate(struct clk_hw *hw, unsigned long req_rate,
+		unsigned long *parent_rate)
+{
+	unsigned long rate = *parent_rate;
+	u8 prescale;
+
+	if (req_rate > rate)
+		return rate;
+
+	prescale = ingenic_ost_get_prescale(rate, req_rate);
+
+	return rate >> (prescale * 2);
+}
+
+static int ingenic_ost_set_rate(struct clk_hw *hw, unsigned long req_rate,
+		unsigned long parent_rate)
+{
+	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
+	const struct ingenic_ost_clk_info *info = ost_clk->info;
+	u8 prescale = ingenic_ost_get_prescale(parent_rate, req_rate);
+	int val;
+
+	if (ost_clk->idx == OST_CLK_PERCPU_TIMER) {
+		val = readl(ost_clk->ost->base + info->ostccr_reg);
+		val = (val & ~OSTCCR_PRESCALE1_MASK) | (prescale << OSTCCR_PRESCALE1_LSB);
+		writel(val, ost_clk->ost->base + info->ostccr_reg);
+	} else if (ost_clk->idx == OST_CLK_GLOBAL_TIMER) {
+		val = readl(ost_clk->ost->base + info->ostccr_reg);
+		val = (val & ~OSTCCR_PRESCALE2_MASK) | (prescale << OSTCCR_PRESCALE2_LSB);
+		writel(val, ost_clk->ost->base + info->ostccr_reg);
+	}
+
+	return 0;
+}
+
+static const struct clk_ops ingenic_ost_clk_ops = {
+	.recalc_rate	= ingenic_ost_recalc_rate,
+	.round_rate	= ingenic_ost_round_rate,
+	.set_rate	= ingenic_ost_set_rate,
+};
+
+static const char * const ingenic_ost_clk_parents[] = {
+	[OST_PARENT_EXT]  = "ext",
+};
+
+#define DEF_TIMER(_name, _ostccr) { \
+	.init_data = { \
+		.name = _name, \
+		.parent_names = ingenic_ost_clk_parents, \
+		.num_parents = ARRAY_SIZE(ingenic_ost_clk_parents), \
+		.ops = &ingenic_ost_clk_ops, \
+		.flags = CLK_SET_RATE_UNGATE, \
+	}, \
+	.ostccr_reg = _ostccr, \
+}
+
+static const struct ingenic_ost_clk_info ingenic_ost_clk_info[] = {
+	[OST_CLK_PERCPU_TIMER] = DEF_TIMER("percpu timer", OST_REG_OSTCCR),
+	[OST_CLK_GLOBAL_TIMER] = DEF_TIMER("global timer", OST_REG_OSTCCR),
+};
+
+#undef DEF_TIMER
+
+static u64 notrace ingenic_ost_global_timer_read_cntl(void)
+{
+	struct ingenic_ost *ost = ingenic_ost;
+	unsigned int count;
+
+	count = readl(ost->base + OST_REG_OST2CNTL);
+
+	return count;
+}
+
+static u64 notrace ingenic_ost_clocksource_read(struct clocksource *cs)
+{
+	return ingenic_ost_global_timer_read_cntl();
+}
+
+static inline struct ingenic_ost *to_ingenic_ost(struct clock_event_device *evt)
+{
+	return container_of(evt, struct ingenic_ost, cevt);
+}
+
+static int ingenic_ost_cevt_set_state_shutdown(struct clock_event_device *evt)
+{
+	struct ingenic_ost *ost = to_ingenic_ost(evt);
+
+	writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
+
+	return 0;
+}
+
+static int ingenic_ost_cevt_set_next(unsigned long next,
+				     struct clock_event_device *evt)
+{
+	struct ingenic_ost *ost = to_ingenic_ost(evt);
+
+	writel((u32)~OSTFR_FFLAG, ost->base + OST_REG_OSTFR);
+	writel(next, ost->base + OST_REG_OST1DFR);
+	writel(OSTCR_OST1CLR, ost->base + OST_REG_OSTCR);
+	writel(OSTESR_OST1ENS, ost->base + OST_REG_OSTESR);
+	writel((u32)~OSTMR_FMASK, ost->base + OST_REG_OSTMR);
+
+	return 0;
+}
+
+static irqreturn_t ingenic_ost_cevt_cb(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	struct ingenic_ost *ost = to_ingenic_ost(evt);
+
+	writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
+
+	if (evt->event_handler)
+		evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static int __init ingenic_ost_register_clock(struct ingenic_ost *ost,
+			unsigned int idx, const struct ingenic_ost_clk_info *info,
+			struct clk_hw_onecell_data *clocks)
+{
+	struct ingenic_ost_clk *ost_clk;
+	int val, err;
+
+	ost_clk = kzalloc(sizeof(*ost_clk), GFP_KERNEL);
+	if (!ost_clk)
+		return -ENOMEM;
+
+	ost_clk->hw.init = &info->init_data;
+	ost_clk->idx = idx;
+	ost_clk->info = info;
+	ost_clk->ost = ost;
+
+	/* Reset clock divider */
+	val = readl(ost->base + info->ostccr_reg);
+	val &= ~(OSTCCR_PRESCALE1_MASK | OSTCCR_PRESCALE2_MASK);
+	writel(val, ost->base + info->ostccr_reg);
+
+	err = clk_hw_register(NULL, &ost_clk->hw);
+	if (err) {
+		kfree(ost_clk);
+		return err;
+	}
+
+	clocks->hws[idx] = &ost_clk->hw;
+
+	return 0;
+}
+
+static struct clk * __init ingenic_ost_get_clock(struct device_node *np, int id)
+{
+	struct of_phandle_args args;
+
+	args.np = np;
+	args.args_count = 1;
+	args.args[0] = id;
+
+	return of_clk_get_from_provider(&args);
+}
+
+static int __init ingenic_ost_percpu_timer_init(struct device_node *np,
+					 struct ingenic_ost *ost)
+{
+	unsigned int timer_virq, channel = ost->percpu_timer_channel;
+	unsigned long rate;
+	int err;
+
+	ost->percpu_timer_clk = ingenic_ost_get_clock(np, channel);
+	if (IS_ERR(ost->percpu_timer_clk))
+		return PTR_ERR(ost->percpu_timer_clk);
+
+	err = clk_prepare_enable(ost->percpu_timer_clk);
+	if (err)
+		goto err_clk_put;
+
+	rate = clk_get_rate(ost->percpu_timer_clk);
+	if (!rate) {
+		err = -EINVAL;
+		goto err_clk_disable;
+	}
+
+	timer_virq = of_irq_get(np, 0);
+	if (!timer_virq) {
+		err = -EINVAL;
+		goto err_clk_disable;
+	}
+
+	snprintf(ost->name, sizeof(ost->name), "OST percpu timer");
+
+	err = request_irq(timer_virq, ingenic_ost_cevt_cb, IRQF_TIMER,
+			  ost->name, &ost->cevt);
+	if (err)
+		goto err_irq_dispose_mapping;
+
+	ost->cevt.cpumask = cpumask_of(smp_processor_id());
+	ost->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
+	ost->cevt.name = ost->name;
+	ost->cevt.rating = 400;
+	ost->cevt.set_state_shutdown = ingenic_ost_cevt_set_state_shutdown;
+	ost->cevt.set_next_event = ingenic_ost_cevt_set_next;
+
+	clockevents_config_and_register(&ost->cevt, rate, 4, 0xffffffff);
+
+	return 0;
+
+err_irq_dispose_mapping:
+	irq_dispose_mapping(timer_virq);
+err_clk_disable:
+	clk_disable_unprepare(ost->percpu_timer_clk);
+err_clk_put:
+	clk_put(ost->percpu_timer_clk);
+	return err;
+}
+
+static int __init ingenic_ost_global_timer_init(struct device_node *np,
+					       struct ingenic_ost *ost)
+{
+	unsigned int channel = ost->global_timer_channel;
+	struct clocksource *cs = &ost->cs;
+	unsigned long rate;
+	int err;
+
+	ost->global_timer_clk = ingenic_ost_get_clock(np, channel);
+	if (IS_ERR(ost->global_timer_clk))
+		return PTR_ERR(ost->global_timer_clk);
+
+	err = clk_prepare_enable(ost->global_timer_clk);
+	if (err)
+		goto err_clk_put;
+
+	rate = clk_get_rate(ost->global_timer_clk);
+	if (!rate) {
+		err = -EINVAL;
+		goto err_clk_disable;
+	}
+
+	/* Clear counter CNT registers */
+	writel(OSTCR_OST2CLR, ost->base + OST_REG_OSTCR);
+
+	/* Enable OST channel */
+	writel(OSTESR_OST2ENS, ost->base + OST_REG_OSTESR);
+
+	cs->name = "ingenic-ost";
+	cs->rating = 400;
+	cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+	cs->mask = CLOCKSOURCE_MASK(32);
+	cs->read = ingenic_ost_clocksource_read;
+
+	err = clocksource_register_hz(cs, rate);
+	if (err)
+		goto err_clk_disable;
+
+	return 0;
+
+err_clk_disable:
+	clk_disable_unprepare(ost->global_timer_clk);
+err_clk_put:
+	clk_put(ost->global_timer_clk);
+	return err;
+}
+
+static const struct ingenic_soc_info x1000_soc_info = {
+	.num_channels = 2,
+};
+
+static const struct of_device_id __maybe_unused ingenic_ost_of_match[] __initconst = {
+	{ .compatible = "ingenic,x1000-ost", .data = &x1000_soc_info, },
+	{ /* sentinel */ }
+};
+
+static int __init ingenic_ost_probe(struct device_node *np)
+{
+	const struct of_device_id *id = of_match_node(ingenic_ost_of_match, np);
+	struct ingenic_ost *ost;
+	unsigned int i;
+	int ret;
+
+	ost = kzalloc(sizeof(*ost), GFP_KERNEL);
+	if (!ost)
+		return -ENOMEM;
+
+	ost->base = of_iomap(np, 0);
+	if (IS_ERR(ost->base)) {
+		pr_err("%s: Failed to map OST registers\n", __func__);
+		ret = PTR_ERR(ost->base);
+		goto err_free_ost;
+	}
+
+	ost->clk = of_clk_get_by_name(np, "ost");
+	if (IS_ERR(ost->clk)) {
+		ret = PTR_ERR(ost->clk);
+		pr_crit("%s: Cannot get OST clock\n", __func__);
+		goto err_free_ost;
+	}
+
+	ret = clk_prepare_enable(ost->clk);
+	if (ret) {
+		pr_crit("%s: Unable to enable OST clock\n", __func__);
+		goto err_put_clk;
+	}
+
+	ost->soc_info = id->data;
+
+	ost->clocks = kzalloc(struct_size(ost->clocks, hws, ost->soc_info->num_channels),
+			      GFP_KERNEL);
+	if (!ost->clocks) {
+		ret = -ENOMEM;
+		goto err_clk_disable;
+	}
+
+	ost->clocks->num = ost->soc_info->num_channels;
+
+	for (i = 0; i < ost->clocks->num; i++) {
+		ret = ingenic_ost_register_clock(ost, i, &ingenic_ost_clk_info[i], ost->clocks);
+		if (ret) {
+			pr_crit("%s: Cannot register clock %d\n", __func__, i);
+			goto err_unregister_ost_clocks;
+		}
+	}
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, ost->clocks);
+	if (ret) {
+		pr_crit("%s: Cannot add OF clock provider\n", __func__);
+		goto err_unregister_ost_clocks;
+	}
+
+	ost->percpu_timer_channel = OST_CLK_PERCPU_TIMER;
+	ost->global_timer_channel = OST_CLK_GLOBAL_TIMER;
+
+	ingenic_ost = ost;
+
+	return 0;
+
+err_unregister_ost_clocks:
+	for (i = 0; i < ost->clocks->num; i++)
+		if (ost->clocks->hws[i])
+			clk_hw_unregister(ost->clocks->hws[i]);
+	kfree(ost->clocks);
+err_clk_disable:
+	clk_disable_unprepare(ost->clk);
+err_put_clk:
+	clk_put(ost->clk);
+err_free_ost:
+	kfree(ost);
+	return ret;
+}
+
+static int __init ingenic_ost_init(struct device_node *np)
+{
+	unsigned long rate;
+	int ret = ingenic_ost_probe(np);
+
+	if (ret)
+		pr_crit("%s: Failed to initialize OST clocks: %d\n", __func__, ret);
+
+	of_node_clear_flag(np, OF_POPULATED);
+
+	ret = ingenic_ost_global_timer_init(np, ingenic_ost);
+	if (ret) {
+		pr_crit("%s: Unable to init global timer: %x\n", __func__, ret);
+		goto err_free_ingenic_ost;
+	}
+
+	ret = ingenic_ost_percpu_timer_init(np, ingenic_ost);
+	if (ret)
+		goto err_ost_global_timer_cleanup;
+
+	/* Register the sched_clock at the end as there's no way to undo it */
+	rate = clk_get_rate(ingenic_ost->global_timer_clk);
+	sched_clock_register(ingenic_ost_global_timer_read_cntl, 32, rate);
+
+	return 0;
+
+err_ost_global_timer_cleanup:
+	clocksource_unregister(&ingenic_ost->cs);
+	clk_disable_unprepare(ingenic_ost->global_timer_clk);
+	clk_put(ingenic_ost->global_timer_clk);
+err_free_ingenic_ost:
+	kfree(ingenic_ost);
+	return ret;
+}
+
+TIMER_OF_DECLARE(x1000_ost,  "ingenic,x1000-ost",  ingenic_ost_init);
-- 
2.11.0

Re: [PATCH v3 1/2] dt-bindings: timer: Add Ingenic X1000 OST bindings.

[Paul Cercueil]

Hi Zhou,

Le mer. 1 juil. 2020 à 1:15, 周琰杰 (Zhou Yanjie) 
<zhouyanjie@wanyeetech.com> a écrit :
> Add the OST bindings for the X10000 SoC from Ingenic.
> 
> Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
> 
> Notes:
>     v1->v2:
>     No change.
> 
>     v2->v3:
>     Fix wrong parameters in "clocks".
> 
>  .../devicetree/bindings/timer/ingenic,ost.yaml     | 61 
> ++++++++++++++++++++++
>  include/dt-bindings/clock/ingenic,ost.h            | 12 +++++

Please name them ingenic,sysost.yaml and ingenic,sysost.h, to 
differenciate with the OST that is in the JZ SoCs.

>  2 files changed, 73 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/timer/ingenic,ost.yaml
>  create mode 100644 include/dt-bindings/clock/ingenic,ost.h
> 
> diff --git a/Documentation/devicetree/bindings/timer/ingenic,ost.yaml 
> b/Documentation/devicetree/bindings/timer/ingenic,ost.yaml
> new file mode 100644
> index 000000000000..459dd3d161c2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ingenic,ost.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/ingenic,ost.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bindings for SYSOST in Ingenic XBurst family SoCs
> +
> +maintainers:
> +  - 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> +
> +description:
> +  The SYSOST in an Ingenic SoC provides one 64bit timer for 
> clocksource
> +  and one or more than one 32bit timers for clockevent.

"and one or more than one" -> "and one or more"

> +
> +properties:
> +  compatible:
> +    oneOf:
> +
> +      - description: SYSOST in Ingenic XBurst family SoCs

XBurst is the name of the CPU, not a SoC family, so you would just 
write 'Ingenic SoCs'. But just drop the description alltogether, it 
does not add anything valuable.

> +        enum:
> +          - ingenic,x1000-ost
> +          - ingenic,x2000-ost

You have "ingenic,x2000-ost" as a compatible string, but your driver 
(in patch [2/2]) only handles the first compatible string. Either they 
are different, in this case the driver should handle both, or they work 
the same, and in the case the "ingenic,x2000-ost" string should use 
"ingenic,x1000-ost" as a fallback string.

Cheers,
-Paul

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: ost
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - "#clock-cells"
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/x1000-cgu.h>
> +
> +    ost: timer@12000000 {
> +    		compatible = "ingenic,x1000-ost";
> +    		reg = <0x12000000 0x3c>;
> +
> +    		#clock-cells = <1>;
> +
> +    		clocks = <&cgu X1000_CLK_OST>;
> +    		clock-names = "ost";
> +
> +    		interrupt-parent = <&cpuintc>;
> +    		interrupts = <3>;
> +    	};
> +...
> diff --git a/include/dt-bindings/clock/ingenic,ost.h 
> b/include/dt-bindings/clock/ingenic,ost.h
> new file mode 100644
> index 000000000000..9ac88e90babf
> --- /dev/null
> +++ b/include/dt-bindings/clock/ingenic,ost.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This header provides clock numbers for the ingenic,tcu DT binding.
> + */
> +
> +#ifndef __DT_BINDINGS_CLOCK_INGENIC_OST_H__
> +#define __DT_BINDINGS_CLOCK_INGENIC_OST_H__
> +
> +#define OST_CLK_PERCPU_TIMER	0
> +#define OST_CLK_GLOBAL_TIMER	1
> +
> +#endif /* __DT_BINDINGS_CLOCK_INGENIC_OST_H__ */
> --
> 2.11.0
> 

Re: [PATCH v3 2/2] clocksource: Ingenic: Add support for the Ingenic X1000 OST.

[Paul Cercueil]

Hi Zhou,

Le mer. 1 juil. 2020 à 1:15, 周琰杰 (Zhou Yanjie) 
<zhouyanjie@wanyeetech.com> a écrit :
> X1000 and SoCs after X1000 (such as X1500 and X1830) had a separate
> OST, it no longer belongs to TCU. This driver will register both a
> clocksource and a sched_clock to the system.
> 
> Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
> Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
> Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
> 
> Notes:
>     v1->v2:
>     Fix compile warnings.
>     Reported-by: kernel test robot <lkp@intel.com>
> 
>     v2->v3:
>     No change.
> 
>  drivers/clocksource/Kconfig          |  19 +-
>  drivers/clocksource/Makefile         |   1 +
>  drivers/clocksource/ingenic-sysost.c | 509 
> +++++++++++++++++++++++++++++++++++
>  3 files changed, 525 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/clocksource/ingenic-sysost.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 91418381fcd4..172397a00f3e 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -686,7 +686,7 @@ config MILBEAUT_TIMER
>  	  Enables the support for Milbeaut timer driver.
> 
>  config INGENIC_TIMER
> -	bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
> +	bool "Clocksource/timer using the TCU in Ingenic SoCs"

This cosmetic change is unrelated to the patch, please drop it.

>  	default MACH_INGENIC
>  	depends on MIPS || COMPILE_TEST
>  	depends on COMMON_CLK
> @@ -694,15 +694,26 @@ config INGENIC_TIMER
>  	select TIMER_OF
>  	select IRQ_DOMAIN
>  	help
> -	  Support for the timer/counter unit of the Ingenic JZ SoCs.
> +	  Support for the timer/counter unit of the Ingenic SoCs.

Same here.

> +
> +config INGENIC_SYSOST
> +	bool "Clocksource/timer using the SYSOST in Ingenic SoCs"
> +	default MACH_INGENIC
> +	depends on MIPS || COMPILE_TEST
> +	depends on COMMON_CLK
> +	select MFD_SYSCON
> +	select TIMER_OF
> +	select IRQ_DOMAIN
> +	help
> +	  This option enables support for SYSOST in Ingenic SoCs .
> 
>  config INGENIC_OST
> -	bool "Clocksource for Ingenic OS Timer"
> +	bool "Clocksource for Ingenic OST"

Same here.

>  	depends on MIPS || COMPILE_TEST
>  	depends on COMMON_CLK
>  	select MFD_SYSCON
>  	help
> -	  Support for the Operating System Timer of the Ingenic JZ SoCs.
> +	  Support for the Operating System Timer of the Ingenic SoCs.

Same here.

> 
>  config MICROCHIP_PIT64B
>  	bool "Microchip PIT64B support"
> diff --git a/drivers/clocksource/Makefile 
> b/drivers/clocksource/Makefile
> index bdda1a2e4097..3994e221e262 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_H8300_TMR8)		+= h8300_timer8.o
>  obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
>  obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
>  obj-$(CONFIG_INGENIC_OST)		+= ingenic-ost.o
> +obj-$(CONFIG_INGENIC_SYSOST)	+= ingenic-sysost.o
>  obj-$(CONFIG_INGENIC_TIMER)		+= ingenic-timer.o
>  obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>  obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
> diff --git a/drivers/clocksource/ingenic-sysost.c 
> b/drivers/clocksource/ingenic-sysost.c
> new file mode 100644
> index 000000000000..d3e1b7582221
> --- /dev/null
> +++ b/drivers/clocksource/ingenic-sysost.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Ingenic XBurst SoCs SYSOST clocks driver
> + * Copyright (c) 2020 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/sched_clock.h>
> +#include <linux/slab.h>
> +#include <linux/syscore_ops.h>
> +
> +#include <dt-bindings/clock/ingenic,ost.h>
> +
> +/* OST register offsets */
> +#define OST_REG_OSTCCR			0x00
> +#define OST_REG_OSTCR			0x08
> +#define OST_REG_OSTFR			0x0c
> +#define OST_REG_OSTMR			0x10
> +#define OST_REG_OST1DFR			0x14
> +#define OST_REG_OST1CNT			0x18
> +#define OST_REG_OST2CNTL		0x20
> +#define OST_REG_OSTCNT2HBUF		0x24
> +#define OST_REG_OSTESR			0x34
> +#define OST_REG_OSTECR			0x38
> +
> +/* bits within the OSTCCR register */
> +#define OSTCCR_PRESCALE1_MASK	0x3
> +#define OSTCCR_PRESCALE2_MASK	0xc
> +#define OSTCCR_PRESCALE1_LSB	0
> +#define OSTCCR_PRESCALE2_LSB	2
> +
> +/* bits within the OSTCR register */
> +#define OSTCR_OST1CLR			BIT(0)
> +#define OSTCR_OST2CLR			BIT(1)
> +
> +/* bits within the OSTFR register */
> +#define OSTFR_FFLAG				BIT(0)
> +
> +/* bits within the OSTMR register */
> +#define OSTMR_FMASK				BIT(0)
> +
> +/* bits within the OSTESR register */
> +#define OSTESR_OST1ENS			BIT(0)
> +#define OSTESR_OST2ENS			BIT(1)
> +
> +/* bits within the OSTECR register */
> +#define OSTECR_OST1ENC			BIT(0)
> +#define OSTECR_OST2ENC			BIT(1)
> +
> +enum ost_clk_parent {
> +	OST_PARENT_EXT,
> +};
> +
> +struct ingenic_soc_info {
> +	unsigned int num_channels;
> +};

Unless you plan to add support for other SoCs which have more than 2 
channels, I think you can drop this - just add a OST_NUM_CHANNELS macro.

> +
> +struct ingenic_ost_clk_info {
> +	struct clk_init_data init_data;
> +	u8 ostccr_reg;
> +};
> +
> +struct ingenic_ost_clk {
> +	struct clk_hw hw;
> +	unsigned int idx;
> +	struct ingenic_ost *ost;
> +	const struct ingenic_ost_clk_info *info;
> +};
> +
> +struct ingenic_ost {
> +	void __iomem *base;
> +	const struct ingenic_soc_info *soc_info;
> +	struct clk *clk, *percpu_timer_clk, *global_timer_clk;
> +	unsigned int percpu_timer_channel, global_timer_channel;
> +	struct clock_event_device cevt;
> +	struct clocksource cs;
> +	char name[20];
> +
> +	struct clk_hw_onecell_data *clocks;
> +};
> +
> +static struct ingenic_ost *ingenic_ost;
> +
> +static inline struct ingenic_ost_clk *to_ost_clk(struct clk_hw *hw)
> +{
> +	return container_of(hw, struct ingenic_ost_clk, hw);
> +}
> +
> +static unsigned long ingenic_ost_recalc_rate(struct clk_hw *hw,
> +		unsigned long parent_rate)
> +{
> +	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
> +	const struct ingenic_ost_clk_info *info = ost_clk->info;
> +	unsigned int prescale;
> +
> +	prescale = readl(ost_clk->ost->base + info->ostccr_reg);
> +
> +	if (ost_clk->idx == OST_CLK_PERCPU_TIMER)
> +		prescale = (prescale & OSTCCR_PRESCALE1_MASK) >> 
> OSTCCR_PRESCALE1_LSB;
> +	else if (ost_clk->idx == OST_CLK_GLOBAL_TIMER)
> +		prescale = (prescale & OSTCCR_PRESCALE2_MASK) >> 
> OSTCCR_PRESCALE2_LSB;

Instead of checking which clock you're manipulating here, just set a 
different .recalc_rate to the two clocks.

> +
> +	return parent_rate >> (prescale * 2);
> +}
> +
> +static u8 ingenic_ost_get_prescale(unsigned long rate, unsigned long 
> req_rate)
> +{
> +	u8 prescale;
> +
> +	for (prescale = 0; prescale < 2; prescale++)
> +		if ((rate >> (prescale * 2)) <= req_rate)
> +			return prescale;
> +
> +	return 2; /* /16 divider */
> +}
> +
> +static long ingenic_ost_round_rate(struct clk_hw *hw, unsigned long 
> req_rate,
> +		unsigned long *parent_rate)
> +{
> +	unsigned long rate = *parent_rate;
> +	u8 prescale;
> +
> +	if (req_rate > rate)
> +		return rate;
> +
> +	prescale = ingenic_ost_get_prescale(rate, req_rate);
> +
> +	return rate >> (prescale * 2);
> +}
> +
> +static int ingenic_ost_set_rate(struct clk_hw *hw, unsigned long 
> req_rate,
> +		unsigned long parent_rate)
> +{
> +	struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
> +	const struct ingenic_ost_clk_info *info = ost_clk->info;
> +	u8 prescale = ingenic_ost_get_prescale(parent_rate, req_rate);
> +	int val;
> +
> +	if (ost_clk->idx == OST_CLK_PERCPU_TIMER) {
> +		val = readl(ost_clk->ost->base + info->ostccr_reg);
> +		val = (val & ~OSTCCR_PRESCALE1_MASK) | (prescale << 
> OSTCCR_PRESCALE1_LSB);
> +		writel(val, ost_clk->ost->base + info->ostccr_reg);
> +	} else if (ost_clk->idx == OST_CLK_GLOBAL_TIMER) {
> +		val = readl(ost_clk->ost->base + info->ostccr_reg);
> +		val = (val & ~OSTCCR_PRESCALE2_MASK) | (prescale << 
> OSTCCR_PRESCALE2_LSB);
> +		writel(val, ost_clk->ost->base + info->ostccr_reg);
> +	}

Same here - set a different .set_rate() callback to the two clocks.

> +
> +	return 0;
> +}
> +
> +static const struct clk_ops ingenic_ost_clk_ops = {
> +	.recalc_rate	= ingenic_ost_recalc_rate,
> +	.round_rate	= ingenic_ost_round_rate,
> +	.set_rate	= ingenic_ost_set_rate,
> +};
> +
> +static const char * const ingenic_ost_clk_parents[] = {
> +	[OST_PARENT_EXT]  = "ext",
> +};

just { "ext" } will be fine, no need to specify the index, which is 
zero anyway. Then you can drop the ost_clock_parent enum.

> +
> +#define DEF_TIMER(_name, _ostccr) { \
> +	.init_data = { \
> +		.name = _name, \
> +		.parent_names = ingenic_ost_clk_parents, \
> +		.num_parents = ARRAY_SIZE(ingenic_ost_clk_parents), \
> +		.ops = &ingenic_ost_clk_ops, \
> +		.flags = CLK_SET_RATE_UNGATE, \
> +	}, \
> +	.ostccr_reg = _ostccr, \
> +}
> +
> +static const struct ingenic_ost_clk_info ingenic_ost_clk_info[] = {
> +	[OST_CLK_PERCPU_TIMER] = DEF_TIMER("percpu timer", OST_REG_OSTCCR),
> +	[OST_CLK_GLOBAL_TIMER] = DEF_TIMER("global timer", OST_REG_OSTCCR),
> +};
> +
> +#undef DEF_TIMER
> +
> +static u64 notrace ingenic_ost_global_timer_read_cntl(void)
> +{
> +	struct ingenic_ost *ost = ingenic_ost;
> +	unsigned int count;
> +
> +	count = readl(ost->base + OST_REG_OST2CNTL);
> +
> +	return count;
> +}
> +
> +static u64 notrace ingenic_ost_clocksource_read(struct clocksource 
> *cs)
> +{
> +	return ingenic_ost_global_timer_read_cntl();
> +}
> +
> +static inline struct ingenic_ost *to_ingenic_ost(struct 
> clock_event_device *evt)
> +{
> +	return container_of(evt, struct ingenic_ost, cevt);
> +}
> +
> +static int ingenic_ost_cevt_set_state_shutdown(struct 
> clock_event_device *evt)
> +{
> +	struct ingenic_ost *ost = to_ingenic_ost(evt);
> +
> +	writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
> +
> +	return 0;
> +}
> +
> +static int ingenic_ost_cevt_set_next(unsigned long next,
> +				     struct clock_event_device *evt)
> +{
> +	struct ingenic_ost *ost = to_ingenic_ost(evt);
> +
> +	writel((u32)~OSTFR_FFLAG, ost->base + OST_REG_OSTFR);
> +	writel(next, ost->base + OST_REG_OST1DFR);
> +	writel(OSTCR_OST1CLR, ost->base + OST_REG_OSTCR);
> +	writel(OSTESR_OST1ENS, ost->base + OST_REG_OSTESR);
> +	writel((u32)~OSTMR_FMASK, ost->base + OST_REG_OSTMR);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t ingenic_ost_cevt_cb(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = dev_id;
> +	struct ingenic_ost *ost = to_ingenic_ost(evt);
> +
> +	writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
> +
> +	if (evt->event_handler)
> +		evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __init ingenic_ost_register_clock(struct ingenic_ost *ost,
> +			unsigned int idx, const struct ingenic_ost_clk_info *info,
> +			struct clk_hw_onecell_data *clocks)
> +{
> +	struct ingenic_ost_clk *ost_clk;
> +	int val, err;
> +
> +	ost_clk = kzalloc(sizeof(*ost_clk), GFP_KERNEL);
> +	if (!ost_clk)
> +		return -ENOMEM;
> +
> +	ost_clk->hw.init = &info->init_data;
> +	ost_clk->idx = idx;
> +	ost_clk->info = info;
> +	ost_clk->ost = ost;
> +
> +	/* Reset clock divider */
> +	val = readl(ost->base + info->ostccr_reg);
> +	val &= ~(OSTCCR_PRESCALE1_MASK | OSTCCR_PRESCALE2_MASK);
> +	writel(val, ost->base + info->ostccr_reg);
> +
> +	err = clk_hw_register(NULL, &ost_clk->hw);
> +	if (err) {
> +		kfree(ost_clk);
> +		return err;
> +	}
> +
> +	clocks->hws[idx] = &ost_clk->hw;
> +
> +	return 0;
> +}
> +
> +static struct clk * __init ingenic_ost_get_clock(struct device_node 
> *np, int id)
> +{
> +	struct of_phandle_args args;
> +
> +	args.np = np;
> +	args.args_count = 1;
> +	args.args[0] = id;
> +
> +	return of_clk_get_from_provider(&args);
> +}
> +
> +static int __init ingenic_ost_percpu_timer_init(struct device_node 
> *np,
> +					 struct ingenic_ost *ost)
> +{
> +	unsigned int timer_virq, channel = ost->percpu_timer_channel;
> +	unsigned long rate;
> +	int err;
> +
> +	ost->percpu_timer_clk = ingenic_ost_get_clock(np, channel);
> +	if (IS_ERR(ost->percpu_timer_clk))
> +		return PTR_ERR(ost->percpu_timer_clk);
> +
> +	err = clk_prepare_enable(ost->percpu_timer_clk);
> +	if (err)
> +		goto err_clk_put;
> +
> +	rate = clk_get_rate(ost->percpu_timer_clk);
> +	if (!rate) {
> +		err = -EINVAL;
> +		goto err_clk_disable;
> +	}
> +
> +	timer_virq = of_irq_get(np, 0);
> +	if (!timer_virq) {
> +		err = -EINVAL;
> +		goto err_clk_disable;
> +	}
> +
> +	snprintf(ost->name, sizeof(ost->name), "OST percpu timer");
> +
> +	err = request_irq(timer_virq, ingenic_ost_cevt_cb, IRQF_TIMER,
> +			  ost->name, &ost->cevt);
> +	if (err)
> +		goto err_irq_dispose_mapping;
> +
> +	ost->cevt.cpumask = cpumask_of(smp_processor_id());
> +	ost->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
> +	ost->cevt.name = ost->name;
> +	ost->cevt.rating = 400;
> +	ost->cevt.set_state_shutdown = ingenic_ost_cevt_set_state_shutdown;
> +	ost->cevt.set_next_event = ingenic_ost_cevt_set_next;
> +
> +	clockevents_config_and_register(&ost->cevt, rate, 4, 0xffffffff);
> +
> +	return 0;
> +
> +err_irq_dispose_mapping:
> +	irq_dispose_mapping(timer_virq);
> +err_clk_disable:
> +	clk_disable_unprepare(ost->percpu_timer_clk);
> +err_clk_put:
> +	clk_put(ost->percpu_timer_clk);
> +	return err;
> +}
> +
> +static int __init ingenic_ost_global_timer_init(struct device_node 
> *np,
> +					       struct ingenic_ost *ost)
> +{
> +	unsigned int channel = ost->global_timer_channel;
> +	struct clocksource *cs = &ost->cs;
> +	unsigned long rate;
> +	int err;
> +
> +	ost->global_timer_clk = ingenic_ost_get_clock(np, channel);
> +	if (IS_ERR(ost->global_timer_clk))
> +		return PTR_ERR(ost->global_timer_clk);
> +
> +	err = clk_prepare_enable(ost->global_timer_clk);
> +	if (err)
> +		goto err_clk_put;
> +
> +	rate = clk_get_rate(ost->global_timer_clk);
> +	if (!rate) {
> +		err = -EINVAL;
> +		goto err_clk_disable;
> +	}
> +
> +	/* Clear counter CNT registers */
> +	writel(OSTCR_OST2CLR, ost->base + OST_REG_OSTCR);
> +
> +	/* Enable OST channel */
> +	writel(OSTESR_OST2ENS, ost->base + OST_REG_OSTESR);
> +
> +	cs->name = "ingenic-ost";
> +	cs->rating = 400;
> +	cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
> +	cs->mask = CLOCKSOURCE_MASK(32);
> +	cs->read = ingenic_ost_clocksource_read;
> +
> +	err = clocksource_register_hz(cs, rate);
> +	if (err)
> +		goto err_clk_disable;
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(ost->global_timer_clk);
> +err_clk_put:
> +	clk_put(ost->global_timer_clk);
> +	return err;
> +}
> +
> +static const struct ingenic_soc_info x1000_soc_info = {
> +	.num_channels = 2,
> +};
> +
> +static const struct of_device_id __maybe_unused 
> ingenic_ost_of_match[] __initconst = {
> +	{ .compatible = "ingenic,x1000-ost", .data = &x1000_soc_info, },
> +	{ /* sentinel */ }
> +};
> +
> +static int __init ingenic_ost_probe(struct device_node *np)
> +{
> +	const struct of_device_id *id = of_match_node(ingenic_ost_of_match, 
> np);
> +	struct ingenic_ost *ost;
> +	unsigned int i;
> +	int ret;
> +
> +	ost = kzalloc(sizeof(*ost), GFP_KERNEL);
> +	if (!ost)
> +		return -ENOMEM;
> +
> +	ost->base = of_iomap(np, 0);
> +	if (IS_ERR(ost->base)) {
> +		pr_err("%s: Failed to map OST registers\n", __func__);
> +		ret = PTR_ERR(ost->base);
> +		goto err_free_ost;
> +	}
> +
> +	ost->clk = of_clk_get_by_name(np, "ost");
> +	if (IS_ERR(ost->clk)) {
> +		ret = PTR_ERR(ost->clk);
> +		pr_crit("%s: Cannot get OST clock\n", __func__);
> +		goto err_free_ost;
> +	}
> +
> +	ret = clk_prepare_enable(ost->clk);
> +	if (ret) {
> +		pr_crit("%s: Unable to enable OST clock\n", __func__);
> +		goto err_put_clk;
> +	}
> +
> +	ost->soc_info = id->data;
> +
> +	ost->clocks = kzalloc(struct_size(ost->clocks, hws, 
> ost->soc_info->num_channels),
> +			      GFP_KERNEL);
> +	if (!ost->clocks) {
> +		ret = -ENOMEM;
> +		goto err_clk_disable;
> +	}
> +
> +	ost->clocks->num = ost->soc_info->num_channels;
> +
> +	for (i = 0; i < ost->clocks->num; i++) {
> +		ret = ingenic_ost_register_clock(ost, i, &ingenic_ost_clk_info[i], 
> ost->clocks);
> +		if (ret) {
> +			pr_crit("%s: Cannot register clock %d\n", __func__, i);
> +			goto err_unregister_ost_clocks;
> +		}
> +	}
> +
> +	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, 
> ost->clocks);
> +	if (ret) {
> +		pr_crit("%s: Cannot add OF clock provider\n", __func__);
> +		goto err_unregister_ost_clocks;
> +	}
> +
> +	ost->percpu_timer_channel = OST_CLK_PERCPU_TIMER;
> +	ost->global_timer_channel = OST_CLK_GLOBAL_TIMER;

If that's a constant, just use the constant where 
'ost->percpu_timer_channel' and 'ost->global_timer_channel' are used, I 
think.

> +
> +	ingenic_ost = ost;
> +
> +	return 0;
> +
> +err_unregister_ost_clocks:
> +	for (i = 0; i < ost->clocks->num; i++)
> +		if (ost->clocks->hws[i])
> +			clk_hw_unregister(ost->clocks->hws[i]);
> +	kfree(ost->clocks);
> +err_clk_disable:
> +	clk_disable_unprepare(ost->clk);
> +err_put_clk:
> +	clk_put(ost->clk);
> +err_free_ost:
> +	kfree(ost);
> +	return ret;
> +}
> +
> +static int __init ingenic_ost_init(struct device_node *np)
> +{
> +	unsigned long rate;
> +	int ret = ingenic_ost_probe(np);

Don't call functions in variable declarations - they can be overlooked 
easily.

> +
> +	if (ret)
> +		pr_crit("%s: Failed to initialize OST clocks: %d\n", __func__, 
> ret);

return ret?

> +
> +	of_node_clear_flag(np, OF_POPULATED);
> +
> +	ret = ingenic_ost_global_timer_init(np, ingenic_ost);

The 'ingenic_ost' variable is just a dirty workaround for the 
sched_clock callback, which does not take any parameter. Avoid using it 
here. Make ingenic_ost_probe() return a pointer to your ingenic_ost 
instance (and a PTR_ERR() on error), and use that pointer for the rest.

Cheers,
-Paul

> +	if (ret) {
> +		pr_crit("%s: Unable to init global timer: %x\n", __func__, ret);
> +		goto err_free_ingenic_ost;
> +	}
> +
> +	ret = ingenic_ost_percpu_timer_init(np, ingenic_ost);
> +	if (ret)
> +		goto err_ost_global_timer_cleanup;
> +
> +	/* Register the sched_clock at the end as there's no way to undo it 
> */
> +	rate = clk_get_rate(ingenic_ost->global_timer_clk);
> +	sched_clock_register(ingenic_ost_global_timer_read_cntl, 32, rate);
> +
> +	return 0;
> +
> +err_ost_global_timer_cleanup:
> +	clocksource_unregister(&ingenic_ost->cs);
> +	clk_disable_unprepare(ingenic_ost->global_timer_clk);
> +	clk_put(ingenic_ost->global_timer_clk);
> +err_free_ingenic_ost:
> +	kfree(ingenic_ost);
> +	return ret;
> +}
> +
> +TIMER_OF_DECLARE(x1000_ost,  "ingenic,x1000-ost",  ingenic_ost_init);
> --
> 2.11.0
> 

Re: [PATCH v3 1/2] dt-bindings: timer: Add Ingenic X1000 OST bindings.

[Zhou Yanjie]

Hi Paul,

在 2020/7/1 上午3:15, Paul Cercueil 写道:
> Hi Zhou,
>
> Le mer. 1 juil. 2020 à 1:15, 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com> a écrit :
>> Add the OST bindings for the X10000 SoC from Ingenic.
>>
>> Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>
>> Notes:
>>     v1->v2:
>>     No change.
>>
>>     v2->v3:
>>     Fix wrong parameters in "clocks".
>>
>>  .../devicetree/bindings/timer/ingenic,ost.yaml     | 61 
>> ++++++++++++++++++++++
>>  include/dt-bindings/clock/ingenic,ost.h            | 12 +++++
>
> Please name them ingenic,sysost.yaml and ingenic,sysost.h, to 
> differenciate with the OST that is in the JZ SoCs.
>

Sure.


>>  2 files changed, 73 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/timer/ingenic,ost.yaml
>>  create mode 100644 include/dt-bindings/clock/ingenic,ost.h
>>
>> diff --git a/Documentation/devicetree/bindings/timer/ingenic,ost.yaml 
>> b/Documentation/devicetree/bindings/timer/ingenic,ost.yaml
>> new file mode 100644
>> index 000000000000..459dd3d161c2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/timer/ingenic,ost.yaml
>> @@ -0,0 +1,61 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/timer/ingenic,ost.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Bindings for SYSOST in Ingenic XBurst family SoCs
>> +
>> +maintainers:
>> +  - 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> +
>> +description:
>> +  The SYSOST in an Ingenic SoC provides one 64bit timer for clocksource
>> +  and one or more than one 32bit timers for clockevent.
>
> "and one or more than one" -> "and one or more"
>

OK, I'll change it in the next version.


>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +
>> +      - description: SYSOST in Ingenic XBurst family SoCs
>
> XBurst is the name of the CPU, not a SoC family, so you would just 
> write 'Ingenic SoCs'. But just drop the description alltogether, it 
> does not add anything valuable.
>

Sure.


>> +        enum:
>> +          - ingenic,x1000-ost
>> +          - ingenic,x2000-ost
>
> You have "ingenic,x2000-ost" as a compatible string, but your driver 
> (in patch [2/2]) only handles the first compatible string. Either they 
> are different, in this case the driver should handle both, or they 
> work the same, and in the case the "ingenic,x2000-ost" string should 
> use "ingenic,x1000-ost" as a fallback string.
>

If SMT is not turned on, X2000 can be compatible with X1000, but if SMT 
is turned on, some additional processing is required, and now this 
compatible string is reserved for this.

Thanks and best regards!


> Cheers,
> -Paul
>
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    const: ost
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +required:
>> +  - "#clock-cells"
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +  - clock-names
>> +  - interrupts
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/x1000-cgu.h>
>> +
>> +    ost: timer@12000000 {
>> +            compatible = "ingenic,x1000-ost";
>> +            reg = <0x12000000 0x3c>;
>> +
>> +            #clock-cells = <1>;
>> +
>> +            clocks = <&cgu X1000_CLK_OST>;
>> +            clock-names = "ost";
>> +
>> +            interrupt-parent = <&cpuintc>;
>> +            interrupts = <3>;
>> +        };
>> +...
>> diff --git a/include/dt-bindings/clock/ingenic,ost.h 
>> b/include/dt-bindings/clock/ingenic,ost.h
>> new file mode 100644
>> index 000000000000..9ac88e90babf
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/ingenic,ost.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * This header provides clock numbers for the ingenic,tcu DT binding.
>> + */
>> +
>> +#ifndef __DT_BINDINGS_CLOCK_INGENIC_OST_H__
>> +#define __DT_BINDINGS_CLOCK_INGENIC_OST_H__
>> +
>> +#define OST_CLK_PERCPU_TIMER    0
>> +#define OST_CLK_GLOBAL_TIMER    1
>> +
>> +#endif /* __DT_BINDINGS_CLOCK_INGENIC_OST_H__ */
>> -- 
>> 2.11.0
>>
>

Re: [PATCH v3 2/2] clocksource: Ingenic: Add support for the Ingenic X1000 OST.

[Zhou Yanjie]

Hi Paul,

在 2020/7/1 上午3:38, Paul Cercueil 写道:
> Hi Zhou,
>
> Le mer. 1 juil. 2020 à 1:15, 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com> a écrit :
>> X1000 and SoCs after X1000 (such as X1500 and X1830) had a separate
>> OST, it no longer belongs to TCU. This driver will register both a
>> clocksource and a sched_clock to the system.
>>
>> Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com>
>> Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
>> Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>
>> Notes:
>>     v1->v2:
>>     Fix compile warnings.
>>     Reported-by: kernel test robot <lkp@intel.com>
>>
>>     v2->v3:
>>     No change.
>>
>>  drivers/clocksource/Kconfig          |  19 +-
>>  drivers/clocksource/Makefile         |   1 +
>>  drivers/clocksource/ingenic-sysost.c | 509 
>> +++++++++++++++++++++++++++++++++++
>>  3 files changed, 525 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/clocksource/ingenic-sysost.c
>>
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index 91418381fcd4..172397a00f3e 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -686,7 +686,7 @@ config MILBEAUT_TIMER
>>        Enables the support for Milbeaut timer driver.
>>
>>  config INGENIC_TIMER
>> -    bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
>> +    bool "Clocksource/timer using the TCU in Ingenic SoCs"
>
> This cosmetic change is unrelated to the patch, please drop it.
>

Sure.


>>      default MACH_INGENIC
>>      depends on MIPS || COMPILE_TEST
>>      depends on COMMON_CLK
>> @@ -694,15 +694,26 @@ config INGENIC_TIMER
>>      select TIMER_OF
>>      select IRQ_DOMAIN
>>      help
>> -      Support for the timer/counter unit of the Ingenic JZ SoCs.
>> +      Support for the timer/counter unit of the Ingenic SoCs.
>
> Same here.
>

Sure.


>> +
>> +config INGENIC_SYSOST
>> +    bool "Clocksource/timer using the SYSOST in Ingenic SoCs"
>> +    default MACH_INGENIC
>> +    depends on MIPS || COMPILE_TEST
>> +    depends on COMMON_CLK
>> +    select MFD_SYSCON
>> +    select TIMER_OF
>> +    select IRQ_DOMAIN
>> +    help
>> +      This option enables support for SYSOST in Ingenic SoCs .
>>
>>  config INGENIC_OST
>> -    bool "Clocksource for Ingenic OS Timer"
>> +    bool "Clocksource for Ingenic OST"
>
> Same here.
>

OK.


>>      depends on MIPS || COMPILE_TEST
>>      depends on COMMON_CLK
>>      select MFD_SYSCON
>>      help
>> -      Support for the Operating System Timer of the Ingenic JZ SoCs.
>> +      Support for the Operating System Timer of the Ingenic SoCs.
>
> Same here.
>

Sure.


>>
>>  config MICROCHIP_PIT64B
>>      bool "Microchip PIT64B support"
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index bdda1a2e4097..3994e221e262 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -82,6 +82,7 @@ obj-$(CONFIG_H8300_TMR8)        += h8300_timer8.o
>>  obj-$(CONFIG_H8300_TMR16)        += h8300_timer16.o
>>  obj-$(CONFIG_H8300_TPU)            += h8300_tpu.o
>>  obj-$(CONFIG_INGENIC_OST)        += ingenic-ost.o
>> +obj-$(CONFIG_INGENIC_SYSOST)    += ingenic-sysost.o
>>  obj-$(CONFIG_INGENIC_TIMER)        += ingenic-timer.o
>>  obj-$(CONFIG_CLKSRC_ST_LPC)        += clksrc_st_lpc.o
>>  obj-$(CONFIG_X86_NUMACHIP)        += numachip.o
>> diff --git a/drivers/clocksource/ingenic-sysost.c 
>> b/drivers/clocksource/ingenic-sysost.c
>> new file mode 100644
>> index 000000000000..d3e1b7582221
>> --- /dev/null
>> +++ b/drivers/clocksource/ingenic-sysost.c
>> @@ -0,0 +1,509 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Ingenic XBurst SoCs SYSOST clocks driver
>> + * Copyright (c) 2020 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/sched_clock.h>
>> +#include <linux/slab.h>
>> +#include <linux/syscore_ops.h>
>> +
>> +#include <dt-bindings/clock/ingenic,ost.h>
>> +
>> +/* OST register offsets */
>> +#define OST_REG_OSTCCR            0x00
>> +#define OST_REG_OSTCR            0x08
>> +#define OST_REG_OSTFR            0x0c
>> +#define OST_REG_OSTMR            0x10
>> +#define OST_REG_OST1DFR            0x14
>> +#define OST_REG_OST1CNT            0x18
>> +#define OST_REG_OST2CNTL        0x20
>> +#define OST_REG_OSTCNT2HBUF        0x24
>> +#define OST_REG_OSTESR            0x34
>> +#define OST_REG_OSTECR            0x38
>> +
>> +/* bits within the OSTCCR register */
>> +#define OSTCCR_PRESCALE1_MASK    0x3
>> +#define OSTCCR_PRESCALE2_MASK    0xc
>> +#define OSTCCR_PRESCALE1_LSB    0
>> +#define OSTCCR_PRESCALE2_LSB    2
>> +
>> +/* bits within the OSTCR register */
>> +#define OSTCR_OST1CLR            BIT(0)
>> +#define OSTCR_OST2CLR            BIT(1)
>> +
>> +/* bits within the OSTFR register */
>> +#define OSTFR_FFLAG                BIT(0)
>> +
>> +/* bits within the OSTMR register */
>> +#define OSTMR_FMASK                BIT(0)
>> +
>> +/* bits within the OSTESR register */
>> +#define OSTESR_OST1ENS            BIT(0)
>> +#define OSTESR_OST2ENS            BIT(1)
>> +
>> +/* bits within the OSTECR register */
>> +#define OSTECR_OST1ENC            BIT(0)
>> +#define OSTECR_OST2ENC            BIT(1)
>> +
>> +enum ost_clk_parent {
>> +    OST_PARENT_EXT,
>> +};
>> +
>> +struct ingenic_soc_info {
>> +    unsigned int num_channels;
>> +};
>
> Unless you plan to add support for other SoCs which have more than 2 
> channels, I think you can drop this - just add a OST_NUM_CHANNELS macro.
>

When X2000 turned on SMT, a total of 3 channels are required, this is 
reserved for adding support for X2000 in the future.


>> +
>> +struct ingenic_ost_clk_info {
>> +    struct clk_init_data init_data;
>> +    u8 ostccr_reg;
>> +};
>> +
>> +struct ingenic_ost_clk {
>> +    struct clk_hw hw;
>> +    unsigned int idx;
>> +    struct ingenic_ost *ost;
>> +    const struct ingenic_ost_clk_info *info;
>> +};
>> +
>> +struct ingenic_ost {
>> +    void __iomem *base;
>> +    const struct ingenic_soc_info *soc_info;
>> +    struct clk *clk, *percpu_timer_clk, *global_timer_clk;
>> +    unsigned int percpu_timer_channel, global_timer_channel;
>> +    struct clock_event_device cevt;
>> +    struct clocksource cs;
>> +    char name[20];
>> +
>> +    struct clk_hw_onecell_data *clocks;
>> +};
>> +
>> +static struct ingenic_ost *ingenic_ost;
>> +
>> +static inline struct ingenic_ost_clk *to_ost_clk(struct clk_hw *hw)
>> +{
>> +    return container_of(hw, struct ingenic_ost_clk, hw);
>> +}
>> +
>> +static unsigned long ingenic_ost_recalc_rate(struct clk_hw *hw,
>> +        unsigned long parent_rate)
>> +{
>> +    struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
>> +    const struct ingenic_ost_clk_info *info = ost_clk->info;
>> +    unsigned int prescale;
>> +
>> +    prescale = readl(ost_clk->ost->base + info->ostccr_reg);
>> +
>> +    if (ost_clk->idx == OST_CLK_PERCPU_TIMER)
>> +        prescale = (prescale & OSTCCR_PRESCALE1_MASK) >> 
>> OSTCCR_PRESCALE1_LSB;
>> +    else if (ost_clk->idx == OST_CLK_GLOBAL_TIMER)
>> +        prescale = (prescale & OSTCCR_PRESCALE2_MASK) >> 
>> OSTCCR_PRESCALE2_LSB;
>
> Instead of checking which clock you're manipulating here, just set a 
> different .recalc_rate to the two clocks.
>

Sure.


>> +
>> +    return parent_rate >> (prescale * 2);
>> +}
>> +
>> +static u8 ingenic_ost_get_prescale(unsigned long rate, unsigned long 
>> req_rate)
>> +{
>> +    u8 prescale;
>> +
>> +    for (prescale = 0; prescale < 2; prescale++)
>> +        if ((rate >> (prescale * 2)) <= req_rate)
>> +            return prescale;
>> +
>> +    return 2; /* /16 divider */
>> +}
>> +
>> +static long ingenic_ost_round_rate(struct clk_hw *hw, unsigned long 
>> req_rate,
>> +        unsigned long *parent_rate)
>> +{
>> +    unsigned long rate = *parent_rate;
>> +    u8 prescale;
>> +
>> +    if (req_rate > rate)
>> +        return rate;
>> +
>> +    prescale = ingenic_ost_get_prescale(rate, req_rate);
>> +
>> +    return rate >> (prescale * 2);
>> +}
>> +
>> +static int ingenic_ost_set_rate(struct clk_hw *hw, unsigned long 
>> req_rate,
>> +        unsigned long parent_rate)
>> +{
>> +    struct ingenic_ost_clk *ost_clk = to_ost_clk(hw);
>> +    const struct ingenic_ost_clk_info *info = ost_clk->info;
>> +    u8 prescale = ingenic_ost_get_prescale(parent_rate, req_rate);
>> +    int val;
>> +
>> +    if (ost_clk->idx == OST_CLK_PERCPU_TIMER) {
>> +        val = readl(ost_clk->ost->base + info->ostccr_reg);
>> +        val = (val & ~OSTCCR_PRESCALE1_MASK) | (prescale << 
>> OSTCCR_PRESCALE1_LSB);
>> +        writel(val, ost_clk->ost->base + info->ostccr_reg);
>> +    } else if (ost_clk->idx == OST_CLK_GLOBAL_TIMER) {
>> +        val = readl(ost_clk->ost->base + info->ostccr_reg);
>> +        val = (val & ~OSTCCR_PRESCALE2_MASK) | (prescale << 
>> OSTCCR_PRESCALE2_LSB);
>> +        writel(val, ost_clk->ost->base + info->ostccr_reg);
>> +    }
>
> Same here - set a different .set_rate() callback to the two clocks.
>

Sure.


>> +
>> +    return 0;
>> +}
>> +
>> +static const struct clk_ops ingenic_ost_clk_ops = {
>> +    .recalc_rate    = ingenic_ost_recalc_rate,
>> +    .round_rate    = ingenic_ost_round_rate,
>> +    .set_rate    = ingenic_ost_set_rate,
>> +};
>> +
>> +static const char * const ingenic_ost_clk_parents[] = {
>> +    [OST_PARENT_EXT]  = "ext",
>> +};
>
> just { "ext" } will be fine, no need to specify the index, which is 
> zero anyway. Then you can drop the ost_clock_parent enum.
>

OK.


>> +
>> +#define DEF_TIMER(_name, _ostccr) { \
>> +    .init_data = { \
>> +        .name = _name, \
>> +        .parent_names = ingenic_ost_clk_parents, \
>> +        .num_parents = ARRAY_SIZE(ingenic_ost_clk_parents), \
>> +        .ops = &ingenic_ost_clk_ops, \
>> +        .flags = CLK_SET_RATE_UNGATE, \
>> +    }, \
>> +    .ostccr_reg = _ostccr, \
>> +}
>> +
>> +static const struct ingenic_ost_clk_info ingenic_ost_clk_info[] = {
>> +    [OST_CLK_PERCPU_TIMER] = DEF_TIMER("percpu timer", OST_REG_OSTCCR),
>> +    [OST_CLK_GLOBAL_TIMER] = DEF_TIMER("global timer", OST_REG_OSTCCR),
>> +};
>> +
>> +#undef DEF_TIMER
>> +
>> +static u64 notrace ingenic_ost_global_timer_read_cntl(void)
>> +{
>> +    struct ingenic_ost *ost = ingenic_ost;
>> +    unsigned int count;
>> +
>> +    count = readl(ost->base + OST_REG_OST2CNTL);
>> +
>> +    return count;
>> +}
>> +
>> +static u64 notrace ingenic_ost_clocksource_read(struct clocksource *cs)
>> +{
>> +    return ingenic_ost_global_timer_read_cntl();
>> +}
>> +
>> +static inline struct ingenic_ost *to_ingenic_ost(struct 
>> clock_event_device *evt)
>> +{
>> +    return container_of(evt, struct ingenic_ost, cevt);
>> +}
>> +
>> +static int ingenic_ost_cevt_set_state_shutdown(struct 
>> clock_event_device *evt)
>> +{
>> +    struct ingenic_ost *ost = to_ingenic_ost(evt);
>> +
>> +    writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
>> +
>> +    return 0;
>> +}
>> +
>> +static int ingenic_ost_cevt_set_next(unsigned long next,
>> +                     struct clock_event_device *evt)
>> +{
>> +    struct ingenic_ost *ost = to_ingenic_ost(evt);
>> +
>> +    writel((u32)~OSTFR_FFLAG, ost->base + OST_REG_OSTFR);
>> +    writel(next, ost->base + OST_REG_OST1DFR);
>> +    writel(OSTCR_OST1CLR, ost->base + OST_REG_OSTCR);
>> +    writel(OSTESR_OST1ENS, ost->base + OST_REG_OSTESR);
>> +    writel((u32)~OSTMR_FMASK, ost->base + OST_REG_OSTMR);
>> +
>> +    return 0;
>> +}
>> +
>> +static irqreturn_t ingenic_ost_cevt_cb(int irq, void *dev_id)
>> +{
>> +    struct clock_event_device *evt = dev_id;
>> +    struct ingenic_ost *ost = to_ingenic_ost(evt);
>> +
>> +    writel(OSTECR_OST1ENC, ost->base + OST_REG_OSTECR);
>> +
>> +    if (evt->event_handler)
>> +        evt->event_handler(evt);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static int __init ingenic_ost_register_clock(struct ingenic_ost *ost,
>> +            unsigned int idx, const struct ingenic_ost_clk_info *info,
>> +            struct clk_hw_onecell_data *clocks)
>> +{
>> +    struct ingenic_ost_clk *ost_clk;
>> +    int val, err;
>> +
>> +    ost_clk = kzalloc(sizeof(*ost_clk), GFP_KERNEL);
>> +    if (!ost_clk)
>> +        return -ENOMEM;
>> +
>> +    ost_clk->hw.init = &info->init_data;
>> +    ost_clk->idx = idx;
>> +    ost_clk->info = info;
>> +    ost_clk->ost = ost;
>> +
>> +    /* Reset clock divider */
>> +    val = readl(ost->base + info->ostccr_reg);
>> +    val &= ~(OSTCCR_PRESCALE1_MASK | OSTCCR_PRESCALE2_MASK);
>> +    writel(val, ost->base + info->ostccr_reg);
>> +
>> +    err = clk_hw_register(NULL, &ost_clk->hw);
>> +    if (err) {
>> +        kfree(ost_clk);
>> +        return err;
>> +    }
>> +
>> +    clocks->hws[idx] = &ost_clk->hw;
>> +
>> +    return 0;
>> +}
>> +
>> +static struct clk * __init ingenic_ost_get_clock(struct device_node 
>> *np, int id)
>> +{
>> +    struct of_phandle_args args;
>> +
>> +    args.np = np;
>> +    args.args_count = 1;
>> +    args.args[0] = id;
>> +
>> +    return of_clk_get_from_provider(&args);
>> +}
>> +
>> +static int __init ingenic_ost_percpu_timer_init(struct device_node *np,
>> +                     struct ingenic_ost *ost)
>> +{
>> +    unsigned int timer_virq, channel = ost->percpu_timer_channel;
>> +    unsigned long rate;
>> +    int err;
>> +
>> +    ost->percpu_timer_clk = ingenic_ost_get_clock(np, channel);
>> +    if (IS_ERR(ost->percpu_timer_clk))
>> +        return PTR_ERR(ost->percpu_timer_clk);
>> +
>> +    err = clk_prepare_enable(ost->percpu_timer_clk);
>> +    if (err)
>> +        goto err_clk_put;
>> +
>> +    rate = clk_get_rate(ost->percpu_timer_clk);
>> +    if (!rate) {
>> +        err = -EINVAL;
>> +        goto err_clk_disable;
>> +    }
>> +
>> +    timer_virq = of_irq_get(np, 0);
>> +    if (!timer_virq) {
>> +        err = -EINVAL;
>> +        goto err_clk_disable;
>> +    }
>> +
>> +    snprintf(ost->name, sizeof(ost->name), "OST percpu timer");
>> +
>> +    err = request_irq(timer_virq, ingenic_ost_cevt_cb, IRQF_TIMER,
>> +              ost->name, &ost->cevt);
>> +    if (err)
>> +        goto err_irq_dispose_mapping;
>> +
>> +    ost->cevt.cpumask = cpumask_of(smp_processor_id());
>> +    ost->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
>> +    ost->cevt.name = ost->name;
>> +    ost->cevt.rating = 400;
>> +    ost->cevt.set_state_shutdown = ingenic_ost_cevt_set_state_shutdown;
>> +    ost->cevt.set_next_event = ingenic_ost_cevt_set_next;
>> +
>> +    clockevents_config_and_register(&ost->cevt, rate, 4, 0xffffffff);
>> +
>> +    return 0;
>> +
>> +err_irq_dispose_mapping:
>> +    irq_dispose_mapping(timer_virq);
>> +err_clk_disable:
>> +    clk_disable_unprepare(ost->percpu_timer_clk);
>> +err_clk_put:
>> +    clk_put(ost->percpu_timer_clk);
>> +    return err;
>> +}
>> +
>> +static int __init ingenic_ost_global_timer_init(struct device_node *np,
>> +                           struct ingenic_ost *ost)
>> +{
>> +    unsigned int channel = ost->global_timer_channel;
>> +    struct clocksource *cs = &ost->cs;
>> +    unsigned long rate;
>> +    int err;
>> +
>> +    ost->global_timer_clk = ingenic_ost_get_clock(np, channel);
>> +    if (IS_ERR(ost->global_timer_clk))
>> +        return PTR_ERR(ost->global_timer_clk);
>> +
>> +    err = clk_prepare_enable(ost->global_timer_clk);
>> +    if (err)
>> +        goto err_clk_put;
>> +
>> +    rate = clk_get_rate(ost->global_timer_clk);
>> +    if (!rate) {
>> +        err = -EINVAL;
>> +        goto err_clk_disable;
>> +    }
>> +
>> +    /* Clear counter CNT registers */
>> +    writel(OSTCR_OST2CLR, ost->base + OST_REG_OSTCR);
>> +
>> +    /* Enable OST channel */
>> +    writel(OSTESR_OST2ENS, ost->base + OST_REG_OSTESR);
>> +
>> +    cs->name = "ingenic-ost";
>> +    cs->rating = 400;
>> +    cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
>> +    cs->mask = CLOCKSOURCE_MASK(32);
>> +    cs->read = ingenic_ost_clocksource_read;
>> +
>> +    err = clocksource_register_hz(cs, rate);
>> +    if (err)
>> +        goto err_clk_disable;
>> +
>> +    return 0;
>> +
>> +err_clk_disable:
>> +    clk_disable_unprepare(ost->global_timer_clk);
>> +err_clk_put:
>> +    clk_put(ost->global_timer_clk);
>> +    return err;
>> +}
>> +
>> +static const struct ingenic_soc_info x1000_soc_info = {
>> +    .num_channels = 2,
>> +};
>> +
>> +static const struct of_device_id __maybe_unused 
>> ingenic_ost_of_match[] __initconst = {
>> +    { .compatible = "ingenic,x1000-ost", .data = &x1000_soc_info, },
>> +    { /* sentinel */ }
>> +};
>> +
>> +static int __init ingenic_ost_probe(struct device_node *np)
>> +{
>> +    const struct of_device_id *id = 
>> of_match_node(ingenic_ost_of_match, np);
>> +    struct ingenic_ost *ost;
>> +    unsigned int i;
>> +    int ret;
>> +
>> +    ost = kzalloc(sizeof(*ost), GFP_KERNEL);
>> +    if (!ost)
>> +        return -ENOMEM;
>> +
>> +    ost->base = of_iomap(np, 0);
>> +    if (IS_ERR(ost->base)) {
>> +        pr_err("%s: Failed to map OST registers\n", __func__);
>> +        ret = PTR_ERR(ost->base);
>> +        goto err_free_ost;
>> +    }
>> +
>> +    ost->clk = of_clk_get_by_name(np, "ost");
>> +    if (IS_ERR(ost->clk)) {
>> +        ret = PTR_ERR(ost->clk);
>> +        pr_crit("%s: Cannot get OST clock\n", __func__);
>> +        goto err_free_ost;
>> +    }
>> +
>> +    ret = clk_prepare_enable(ost->clk);
>> +    if (ret) {
>> +        pr_crit("%s: Unable to enable OST clock\n", __func__);
>> +        goto err_put_clk;
>> +    }
>> +
>> +    ost->soc_info = id->data;
>> +
>> +    ost->clocks = kzalloc(struct_size(ost->clocks, hws, 
>> ost->soc_info->num_channels),
>> +                  GFP_KERNEL);
>> +    if (!ost->clocks) {
>> +        ret = -ENOMEM;
>> +        goto err_clk_disable;
>> +    }
>> +
>> +    ost->clocks->num = ost->soc_info->num_channels;
>> +
>> +    for (i = 0; i < ost->clocks->num; i++) {
>> +        ret = ingenic_ost_register_clock(ost, i, 
>> &ingenic_ost_clk_info[i], ost->clocks);
>> +        if (ret) {
>> +            pr_crit("%s: Cannot register clock %d\n", __func__, i);
>> +            goto err_unregister_ost_clocks;
>> +        }
>> +    }
>> +
>> +    ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, 
>> ost->clocks);
>> +    if (ret) {
>> +        pr_crit("%s: Cannot add OF clock provider\n", __func__);
>> +        goto err_unregister_ost_clocks;
>> +    }
>> +
>> +    ost->percpu_timer_channel = OST_CLK_PERCPU_TIMER;
>> +    ost->global_timer_channel = OST_CLK_GLOBAL_TIMER;
>
> If that's a constant, just use the constant where 
> 'ost->percpu_timer_channel' and 'ost->global_timer_channel' are used, 
> I think.
>

Sure.


>> +
>> +    ingenic_ost = ost;
>> +
>> +    return 0;
>> +
>> +err_unregister_ost_clocks:
>> +    for (i = 0; i < ost->clocks->num; i++)
>> +        if (ost->clocks->hws[i])
>> +            clk_hw_unregister(ost->clocks->hws[i]);
>> +    kfree(ost->clocks);
>> +err_clk_disable:
>> +    clk_disable_unprepare(ost->clk);
>> +err_put_clk:
>> +    clk_put(ost->clk);
>> +err_free_ost:
>> +    kfree(ost);
>> +    return ret;
>> +}
>> +
>> +static int __init ingenic_ost_init(struct device_node *np)
>> +{
>> +    unsigned long rate;
>> +    int ret = ingenic_ost_probe(np);
>
> Don't call functions in variable declarations - they can be overlooked 
> easily.
>

OK


>> +
>> +    if (ret)
>> +        pr_crit("%s: Failed to initialize OST clocks: %d\n", 
>> __func__, ret);
>
> return ret?
>

I'll add it in the next version.


>> +
>> +    of_node_clear_flag(np, OF_POPULATED);
>> +
>> +    ret = ingenic_ost_global_timer_init(np, ingenic_ost);
>
> The 'ingenic_ost' variable is just a dirty workaround for the 
> sched_clock callback, which does not take any parameter. Avoid using 
> it here. Make ingenic_ost_probe() return a pointer to your ingenic_ost 
> instance (and a PTR_ERR() on error), and use that pointer for the rest.
>

Sure.

Thanks and best regards!


> Cheers,
> -Paul
>
>> +    if (ret) {
>> +        pr_crit("%s: Unable to init global timer: %x\n", __func__, 
>> ret);
>> +        goto err_free_ingenic_ost;
>> +    }
>> +
>> +    ret = ingenic_ost_percpu_timer_init(np, ingenic_ost);
>> +    if (ret)
>> +        goto err_ost_global_timer_cleanup;
>> +
>> +    /* Register the sched_clock at the end as there's no way to undo 
>> it */
>> +    rate = clk_get_rate(ingenic_ost->global_timer_clk);
>> +    sched_clock_register(ingenic_ost_global_timer_read_cntl, 32, rate);
>> +
>> +    return 0;
>> +
>> +err_ost_global_timer_cleanup:
>> +    clocksource_unregister(&ingenic_ost->cs);
>> +    clk_disable_unprepare(ingenic_ost->global_timer_clk);
>> +    clk_put(ingenic_ost->global_timer_clk);
>> +err_free_ingenic_ost:
>> +    kfree(ingenic_ost);
>> +    return ret;
>> +}
>> +
>> +TIMER_OF_DECLARE(x1000_ost,  "ingenic,x1000-ost", ingenic_ost_init);
>> -- 
>> 2.11.0
>>
>