[RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

[RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

[Samuel Holland]

This patch series adds a CCU driver for the RTC in the H616 and R329.
The extra patches at the end of this series show how it would be
explanded to additional hardware variants.

The driver is intended to support the existing binding used for the H6,
but also an updated binding which includes all RTC input clocks. I do
not know how to best represent that binding -- that is a major reason
why this series is an RFC.

A future patch series could add functionality to the driver to manage
IOSC calibration at boot and during suspend/resume.

It may be possible to support all of these hardware variants in the
existing RTC clock driver and avoid some duplicate code, but I'm
concerned about the complexity there, without any of the CCU
abstraction.

This series is currently based on top of the other series I just sent
(clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
elsewhere.

Samuel Holland (7):
  dt-bindings: rtc: sun6i: Add H616 and R329 compatibles
  clk: sunxi-ng: div: Add macro using CLK_HW_INIT_FW_NAME
  clk: sunxi-ng: mux: Add macro using CLK_HW_INIT_PARENTS_DATA
  clk: sunxi-ng: mux: Allow muxes to have keys
  clk: sunxi-ng: Add support for the sun50i RTC clocks
  [DO NOT MERGE] clk: sunxi-ng: Add support for H6
  [DO NOT MERGE] clk: sunxi-ng: Add support for T5

 .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml |  55 ++-
 drivers/clk/sunxi-ng/Kconfig                  |   6 +
 drivers/clk/sunxi-ng/Makefile                 |   1 +
 drivers/clk/sunxi-ng/ccu_common.h             |   1 +
 drivers/clk/sunxi-ng/ccu_div.h                |  14 +
 drivers/clk/sunxi-ng/ccu_mux.c                |   7 +
 drivers/clk/sunxi-ng/ccu_mux.h                |  28 ++
 drivers/clk/sunxi-ng/sun50i-rtc-ccu.c         | 433 ++++++++++++++++++
 drivers/clk/sunxi-ng/sun50i-rtc-ccu.h         |  15 +
 drivers/rtc/rtc-sun6i.c                       |  17 -
 include/dt-bindings/clock/sun50i-rtc.h        |  12 +
 11 files changed, 566 insertions(+), 23 deletions(-)
 create mode 100644 drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
 create mode 100644 drivers/clk/sunxi-ng/sun50i-rtc-ccu.h
 create mode 100644 include/dt-bindings/clock/sun50i-rtc.h

-- 
2.31.1

[RFC PATCH 1/7] dt-bindings: rtc: sun6i: Add H616 and R329 compatibles

[Samuel Holland]

For these new SoCs, start requiring a complete list of input clocks.

For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
bus, and hosc; and optionally ext-osc32k.

I'm not sure how to best represent this in the binding...

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
 include/dt-bindings/clock/sun50i-rtc.h        | 12 ++++
 2 files changed, 61 insertions(+), 6 deletions(-)
 create mode 100644 include/dt-bindings/clock/sun50i-rtc.h

diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
index beeb90e55727..3e085db1294f 100644
--- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
@@ -26,6 +26,8 @@ properties:
           - const: allwinner,sun50i-a64-rtc
           - const: allwinner,sun8i-h3-rtc
       - const: allwinner,sun50i-h6-rtc
+      - const: allwinner,sun50i-h616-rtc
+      - const: allwinner,sun50i-r329-rtc
 
   reg:
     maxItems: 1
@@ -37,7 +39,24 @@ properties:
       - description: RTC Alarm 1
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    maxItems: 4
+
+  clock-names:
+    minItems: 1
+    maxItems: 4
+    items:
+      - anyOf:
+          - const: ahb
+            description: AHB parent for SPI bus clock
+          - const: bus
+            description: AHB/APB bus clock for register access
+          - const: ext-osc32k
+            description: External 32768 Hz oscillator input
+          - const: hosc
+            description: 24 MHz oscillator input
+          - const: pll-32k
+            description: 32 kHz clock divided from a PLL
 
   clock-output-names:
     minItems: 1
@@ -85,6 +104,9 @@ allOf:
             enum:
               - allwinner,sun8i-h3-rtc
               - allwinner,sun50i-h5-rtc
+              - allwinner,sun50i-h6-rtc
+              - allwinner,sun50i-h616-rtc
+              - allwinner,sun50i-r329-rtc
 
     then:
       properties:
@@ -96,13 +118,35 @@ allOf:
       properties:
         compatible:
           contains:
-            const: allwinner,sun50i-h6-rtc
+            enum:
+              - allwinner,sun50i-h616-rtc
+              - allwinner,sun50i-r329-rtc
 
     then:
+      clocks:
+        minItems: 3 # bus, hosc, and (pll-32k [H616] or ahb [R329])
+
+      clock-names:
+        minItems: 3
+
+      required:
+        - clock-names
+
+    else:
+      required:
+        - clock-output-names
+
+  - if:
+      properties: clock-names
+
+    then:
+      required:
+        - clocks # hosc is required
+
+    else:
       properties:
-        clock-output-names:
-          minItems: 3
-          maxItems: 3
+        clocks:
+          maxItems: 1 # only ext-osc32k is allowed
 
   - if:
       properties:
@@ -127,7 +171,6 @@ required:
   - compatible
   - reg
   - interrupts
-  - clock-output-names
 
 additionalProperties: false
 
diff --git a/include/dt-bindings/clock/sun50i-rtc.h b/include/dt-bindings/clock/sun50i-rtc.h
new file mode 100644
index 000000000000..d45e3ff4e105
--- /dev/null
+++ b/include/dt-bindings/clock/sun50i-rtc.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_
+#define _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_
+
+#define CLK_OSC32K		0
+#define CLK_OSC32K_FANOUT	1
+#define CLK_IOSC		2
+
+#define CLK_RTC_SPI		8
+
+#endif /* _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ */
-- 
2.31.1

[RFC PATCH 2/7] clk: sunxi-ng: div: Add macro using CLK_HW_INIT_FW_NAME

[Samuel Holland]

To use the external clock references from the device tree, instead of
hardcoded global names, parents should be referenced with .fw_name. Add
a variant of the SUNXI_CCU_M_WITH_GATE initializer which does this.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/clk/sunxi-ng/ccu_div.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu_div.h b/drivers/clk/sunxi-ng/ccu_div.h
index 6682fde6043c..4f8c78a4665b 100644
--- a/drivers/clk/sunxi-ng/ccu_div.h
+++ b/drivers/clk/sunxi-ng/ccu_div.h
@@ -166,6 +166,20 @@ struct ccu_div {
 	SUNXI_CCU_M_WITH_GATE(_struct, _name, _parent, _reg,		\
 			      _mshift, _mwidth, 0, _flags)
 
+#define SUNXI_CCU_M_FW_WITH_GATE(_struct, _name, _parent, _reg,		\
+				 _mshift, _mwidth, _gate, _flags)	\
+	struct ccu_div _struct = {					\
+		.enable	= _gate,					\
+		.div	= _SUNXI_CCU_DIV(_mshift, _mwidth),		\
+		.common	= {						\
+			.reg		= _reg,				\
+			.hw.init	= CLK_HW_INIT_FW_NAME(_name,	\
+							      _parent,	\
+							      &ccu_div_ops, \
+							      _flags),	\
+		},							\
+	}
+
 static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw)
 {
 	struct ccu_common *common = hw_to_ccu_common(hw);
-- 
2.31.1

[RFC PATCH 3/7] clk: sunxi-ng: mux: Add macro using CLK_HW_INIT_PARENTS_DATA

[Samuel Holland]

Some muxes need the flexibility to specify a combination of internal
parents (using .hw) and external parents (using .fw_name). Support
this with a version of the SUNXI_CCU_MUX_WITH_GATE macro that uses
CLK_HW_INIT_PARENTS_DATA to provide the parent information.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/clk/sunxi-ng/ccu_mux.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu_mux.h b/drivers/clk/sunxi-ng/ccu_mux.h
index f165395effb5..5aa5a6f49bd8 100644
--- a/drivers/clk/sunxi-ng/ccu_mux.h
+++ b/drivers/clk/sunxi-ng/ccu_mux.h
@@ -73,6 +73,20 @@ struct ccu_mux {
 	SUNXI_CCU_MUX_TABLE_WITH_GATE(_struct, _name, _parents, NULL,	\
 				      _reg, _shift, _width, 0, _flags)
 
+#define SUNXI_CCU_MUX_DATA_WITH_GATE(_struct, _name, _parents, _reg,	\
+				     _shift, _width, _gate, _flags)	\
+	struct ccu_mux _struct = {					\
+		.enable	= _gate,					\
+		.mux	= _SUNXI_CCU_MUX(_shift, _width),		\
+		.common	= {						\
+			.reg		= _reg,				\
+			.hw.init	= CLK_HW_INIT_PARENTS_DATA(_name, \
+								   _parents, \
+								   &ccu_mux_ops, \
+								   _flags), \
+		}							\
+	}
+
 static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
 {
 	struct ccu_common *common = hw_to_ccu_common(hw);
-- 
2.31.1

[RFC PATCH 4/7] clk: sunxi-ng: mux: Allow muxes to have keys

[Samuel Holland]

The muxes in the RTC can only be updated when setting a key field to a
specific value. Add a feature flag to denote muxes with this property.
Since so far the key value is always the same, it does not need to be
provided separately for each mux.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/clk/sunxi-ng/ccu_common.h |  1 +
 drivers/clk/sunxi-ng/ccu_mux.c    |  7 +++++++
 drivers/clk/sunxi-ng/ccu_mux.h    | 14 ++++++++++++++
 3 files changed, 22 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
index 98a1834b58bb..fbf16c6b896d 100644
--- a/drivers/clk/sunxi-ng/ccu_common.h
+++ b/drivers/clk/sunxi-ng/ccu_common.h
@@ -17,6 +17,7 @@
 #define CCU_FEATURE_LOCK_REG		BIT(5)
 #define CCU_FEATURE_MMC_TIMING_SWITCH	BIT(6)
 #define CCU_FEATURE_SIGMA_DELTA_MOD	BIT(7)
+#define CCU_FEATURE_KEY_FIELD		BIT(8)
 
 /* MMC timing mode switch bit */
 #define CCU_MMC_NEW_TIMING_MODE		BIT(30)
diff --git a/drivers/clk/sunxi-ng/ccu_mux.c b/drivers/clk/sunxi-ng/ccu_mux.c
index 785803cd7e51..fb93cea3a502 100644
--- a/drivers/clk/sunxi-ng/ccu_mux.c
+++ b/drivers/clk/sunxi-ng/ccu_mux.c
@@ -12,6 +12,8 @@
 #include "ccu_gate.h"
 #include "ccu_mux.h"
 
+#define CCU_MUX_KEY_VALUE		0x16aa0000
+
 static u16 ccu_mux_get_prediv(struct ccu_common *common,
 			      struct ccu_mux_internal *cm,
 			      int parent_index)
@@ -188,6 +190,11 @@ int ccu_mux_helper_set_parent(struct ccu_common *common,
 	spin_lock_irqsave(common->lock, flags);
 
 	reg = readl(common->base + common->reg);
+
+	/* The key field always reads as zero. */
+	if (common->features & CCU_FEATURE_KEY_FIELD)
+		reg |= CCU_MUX_KEY_VALUE;
+
 	reg &= ~GENMASK(cm->width + cm->shift - 1, cm->shift);
 	writel(reg | (index << cm->shift), common->base + common->reg);
 
diff --git a/drivers/clk/sunxi-ng/ccu_mux.h b/drivers/clk/sunxi-ng/ccu_mux.h
index 5aa5a6f49bd8..6ca15e43f9c8 100644
--- a/drivers/clk/sunxi-ng/ccu_mux.h
+++ b/drivers/clk/sunxi-ng/ccu_mux.h
@@ -87,6 +87,20 @@ struct ccu_mux {
 		}							\
 	}
 
+#define SUNXI_CCU_MUX_HW_WITH_KEY(_struct, _name, _parents, _reg,	\
+				  _shift, _width, _flags)		\
+	struct ccu_mux _struct = {					\
+		.mux	= _SUNXI_CCU_MUX(_shift, _width),		\
+		.common	= {						\
+			.reg		= _reg,				\
+			.features	= CCU_FEATURE_KEY_FIELD,	\
+			.hw.init	= CLK_HW_INIT_PARENTS_HW(_name,	\
+								 _parents, \
+								 &ccu_mux_ops, \
+								 _flags), \
+		}							\
+	}
+
 static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw)
 {
 	struct ccu_common *common = hw_to_ccu_common(hw);
-- 
2.31.1

[RFC PATCH 5/7] clk: sunxi-ng: Add support for the sun50i RTC clocks

[Samuel Holland]

The RTC power domain in sun50i SoCs manages the 16 MHz RC oscillator
(called "IOSC" or "osc16M") and the optional 32 kHz crystal oscillator
(called "LOSC" or "osc32k"). Starting with the H6, this power domain
handles the 24 MHz DCXO (called "HOSC", "dcxo24M", or "osc24M") as well.
The H6 also introduces a calibration circuit for IOSC.

Later SoCs introduce further variations on the design:
 - H616 adds an additional mux for the 32 kHz fanout source.
 - R329 adds an additional mux for the RTC timekeeping clock, a clock
   for the SPI bus between power domains inside the RTC, and removes the
   IOSC calibration functionality.

Take advantage of the CCU framework to handle this increased complexity.
The CCU framework provides pre-made widgets for the mux/gate/divider
combinations. And it allows plugging in different clocks for the same
specifiers based on the compatible string.

This driver is intended to be a drop-in replacement for the existing RTC
clock driver. So some runtime adjustment of the clock parents is needed,
both to handle hardware differences, and to support the old binding
which omitted some of the input clocks.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/clk/sunxi-ng/Kconfig          |   6 +
 drivers/clk/sunxi-ng/Makefile         |   1 +
 drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 344 ++++++++++++++++++++++++++
 drivers/clk/sunxi-ng/sun50i-rtc-ccu.h |  15 ++
 4 files changed, 366 insertions(+)
 create mode 100644 drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
 create mode 100644 drivers/clk/sunxi-ng/sun50i-rtc-ccu.h

diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
index ee383658ff4d..8d9d486c4673 100644
--- a/drivers/clk/sunxi-ng/Kconfig
+++ b/drivers/clk/sunxi-ng/Kconfig
@@ -42,6 +42,12 @@ config SUN50I_H6_R_CCU
 	default ARM64 && ARCH_SUNXI
 	depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
 
+config SUN50I_RTC_CCU
+	bool "Support for the Allwinner H6/H616/R329 RTC CCU"
+	default ARM64 && ARCH_SUNXI
+	depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
+	depends on SUNXI_CCU=y
+
 config SUN4I_A10_CCU
 	tristate "Support for the Allwinner A10/A20 CCU"
 	default MACH_SUN4I
diff --git a/drivers/clk/sunxi-ng/Makefile b/drivers/clk/sunxi-ng/Makefile
index 6e9eb004fca0..99554b13d150 100644
--- a/drivers/clk/sunxi-ng/Makefile
+++ b/drivers/clk/sunxi-ng/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_SUN50I_A100_R_CCU)	+= sun50i-a100-r-ccu.o
 obj-$(CONFIG_SUN50I_H6_CCU)	+= sun50i-h6-ccu.o
 obj-$(CONFIG_SUN50I_H616_CCU)	+= sun50i-h616-ccu.o
 obj-$(CONFIG_SUN50I_H6_R_CCU)	+= sun50i-h6-r-ccu.o
+obj-$(CONFIG_SUN50I_RTC_CCU)	+= sun50i-rtc-ccu.o
 obj-$(CONFIG_SUN4I_A10_CCU)	+= sun4i-a10-ccu.o
 obj-$(CONFIG_SUN5I_CCU)		+= sun5i-ccu.o
 obj-$(CONFIG_SUN6I_A31_CCU)	+= sun6i-a31-ccu.o
diff --git a/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
new file mode 100644
index 000000000000..1dfa05c2f0e9
--- /dev/null
+++ b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
@@ -0,0 +1,344 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+
+#include "ccu_common.h"
+
+#include "ccu_div.h"
+#include "ccu_gate.h"
+#include "ccu_mux.h"
+
+#include "sun50i-rtc-ccu.h"
+
+#define IOSC_ACCURACY			300000000 /* 30% */
+#define IOSC_RATE			16000000
+
+#define LOSC_RATE			32768
+#define LOSC_RATE_SHIFT			15
+
+#define LOSC_CTRL_KEY			0x16aa0000
+
+#define IOSC_32K_CLK_DIV_REG		0x8
+#define IOSC_32K_CLK_DIV		GENMASK(4, 0)
+#define IOSC_32K_PRE_DIV		32
+
+#define IOSC_CLK_CALI_REG		0xc
+#define IOSC_CLK_CALI_DIV_ONES		22
+#define IOSC_CLK_CALI_EN		BIT(1)
+#define IOSC_CLK_CALI_SRC_SEL		BIT(0)
+
+#define DCXO_CTRL_REG			0x160
+#define DCXO_CTRL_CLK16M_RC_EN		BIT(0)
+
+static bool have_iosc_calib;
+
+static int ccu_iosc_enable(struct clk_hw *hw)
+{
+	struct ccu_common *cm = hw_to_ccu_common(hw);
+
+	return ccu_gate_helper_enable(cm, DCXO_CTRL_CLK16M_RC_EN);
+}
+
+static void ccu_iosc_disable(struct clk_hw *hw)
+{
+	struct ccu_common *cm = hw_to_ccu_common(hw);
+
+	return ccu_gate_helper_disable(cm, DCXO_CTRL_CLK16M_RC_EN);
+}
+
+static int ccu_iosc_is_enabled(struct clk_hw *hw)
+{
+	struct ccu_common *cm = hw_to_ccu_common(hw);
+
+	return ccu_gate_helper_is_enabled(cm, DCXO_CTRL_CLK16M_RC_EN);
+}
+
+static unsigned long ccu_iosc_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct ccu_common *cm = hw_to_ccu_common(hw);
+
+	if (have_iosc_calib) {
+		u32 reg = readl(cm->base + IOSC_CLK_CALI_REG);
+
+		/*
+		 * Recover the IOSC frequency by shifting the ones place of
+		 * (fixed-point divider * 32768) into bit zero.
+		 */
+		if (reg & IOSC_CLK_CALI_EN)
+			return reg >> (IOSC_CLK_CALI_DIV_ONES - LOSC_RATE_SHIFT);
+	}
+
+	return IOSC_RATE;
+}
+
+static unsigned long ccu_iosc_recalc_accuracy(struct clk_hw *hw,
+					      unsigned long parent_accuracy)
+{
+	return IOSC_ACCURACY;
+}
+
+static const struct clk_ops ccu_iosc_ops = {
+	.enable			= ccu_iosc_enable,
+	.disable		= ccu_iosc_disable,
+	.is_enabled		= ccu_iosc_is_enabled,
+	.recalc_rate		= ccu_iosc_recalc_rate,
+	.recalc_accuracy	= ccu_iosc_recalc_accuracy,
+};
+
+static struct ccu_common iosc_clk = {
+	.reg		= DCXO_CTRL_REG,
+	.hw.init	= CLK_HW_INIT_NO_PARENT("iosc", &ccu_iosc_ops,
+						CLK_GET_RATE_NOCACHE),
+};
+
+static int ccu_iosc_32k_enable(struct clk_hw *hw)
+{
+	struct ccu_common *cm = hw_to_ccu_common(hw);
+	unsigned long flags;
+	u32 reg;
+
+	if (!have_iosc_calib)
+		return 0;
+
+	spin_lock_irqsave(cm->lock, flags);
+
+	reg = readl(cm->base + IOSC_CLK_CALI_REG);
+	writel(reg | IOSC_CLK_CALI_EN | IOSC_CLK_CALI_SRC_SEL,
+	       cm->base + IOSC_CLK_CALI_REG);
+
+	spin_unlock_irqrestore(cm->lock, flags);
+
+	return 0;
+}
+
+static void ccu_iosc_32k_disable(struct clk_hw *hw)
+{
+	struct ccu_common *cm = hw_to_ccu_common(hw);
+	unsigned long flags;
+	u32 reg;
+
+	if (!have_iosc_calib)
+		return;
+
+	spin_lock_irqsave(cm->lock, flags);
+
+	reg = readl(cm->base + IOSC_CLK_CALI_REG);
+	writel(reg & ~(IOSC_CLK_CALI_EN | IOSC_CLK_CALI_SRC_SEL),
+	       cm->base + IOSC_CLK_CALI_REG);
+
+	spin_unlock_irqrestore(cm->lock, flags);
+}
+
+static unsigned long ccu_iosc_32k_recalc_rate(struct clk_hw *hw,
+					      unsigned long parent_rate)
+{
+	struct ccu_common *cm = hw_to_ccu_common(hw);
+	u32 reg;
+
+	if (have_iosc_calib) {
+		reg = readl(cm->base + IOSC_CLK_CALI_REG);
+
+		/* Assume the calibrated 32k clock is accurate. */
+		if (reg & IOSC_CLK_CALI_SRC_SEL)
+			return LOSC_RATE;
+	}
+
+	reg = readl(cm->base + IOSC_32K_CLK_DIV_REG) & IOSC_32K_CLK_DIV;
+
+	return parent_rate / IOSC_32K_PRE_DIV / (reg + 1);
+}
+
+static unsigned long ccu_iosc_32k_recalc_accuracy(struct clk_hw *hw,
+						  unsigned long parent_accuracy)
+{
+	struct ccu_common *cm = hw_to_ccu_common(hw);
+	u32 reg;
+
+	if (have_iosc_calib) {
+		reg = readl(cm->base + IOSC_CLK_CALI_REG);
+
+		/* Assume the calibrated 32k clock is accurate. */
+		if (reg & IOSC_CLK_CALI_SRC_SEL)
+			return 0;
+	}
+
+	return parent_accuracy;
+}
+
+static const struct clk_ops ccu_iosc_32k_ops = {
+	.enable			= ccu_iosc_32k_enable,
+	.disable		= ccu_iosc_32k_disable,
+	.recalc_rate		= ccu_iosc_32k_recalc_rate,
+	.recalc_accuracy	= ccu_iosc_32k_recalc_accuracy,
+};
+
+static struct ccu_common iosc_32k_clk = {
+	.hw.init	= CLK_HW_INIT_HW("iosc-32k", &iosc_clk.hw,
+					 &ccu_iosc_32k_ops, 0),
+};
+
+/* The old binding did not use clock-names, so fw_name may get cleared out. */
+static struct clk_parent_data ext_osc32k[] = {
+	{ .fw_name = "ext-osc32k", .index = 0 }
+};
+static SUNXI_CCU_GATE_DATA(ext_osc32k_gate_clk, "ext-osc32k-gate",
+			   ext_osc32k, 0x0, BIT(4), 0);
+
+static const struct clk_hw *osc32k_parents[] = { &iosc_32k_clk.hw,
+						 &ext_osc32k_gate_clk.common.hw };
+static SUNXI_CCU_MUX_HW_WITH_KEY(osc32k_clk, "osc32k", osc32k_parents,
+				 0x0, 0, 1, 0);
+
+/* Fall back to the global name for RTC nodes without an osc24M reference. */
+static struct clk_parent_data osc24M[] = {
+	{ .fw_name = "hosc", .name = "osc24M" }
+};
+static struct ccu_gate osc24M_32k_clk = {
+	.enable	= BIT(16),
+	.common	= {
+		.reg		= 0x60,
+		.prediv		= 750,
+		.features	= CCU_FEATURE_ALL_PREDIV,
+		.hw.init	= CLK_HW_INIT_PARENTS_DATA("osc24M-32k", osc24M,
+							   &ccu_gate_ops, 0),
+	},
+};
+
+static CLK_FIXED_FACTOR_HW(rtc_32k_fixed_clk, "rtc-32k",
+			   &osc32k_clk.common.hw, 1, 1, 0);
+
+static const struct clk_hw *rtc_32k_parents[] = { &osc32k_clk.common.hw,
+						  &osc24M_32k_clk.common.hw };
+static SUNXI_CCU_MUX_HW_WITH_KEY(rtc_32k_mux_clk, "rtc-32k", rtc_32k_parents,
+				 0x0, 1, 1, 0);
+
+static struct clk_parent_data osc32k_fanout_parents[] = {
+	{ .hw = &osc32k_clk.common.hw },
+	/* Parent is modified depending on the hardware variant. */
+	{ .fw_name = "pll-32k" },
+	{ .hw = &osc24M_32k_clk.common.hw },
+};
+static SUNXI_CCU_MUX_DATA_WITH_GATE(osc32k_fanout_clk, "rtc-32k-fanout",
+				    osc32k_fanout_parents,
+				    0x60, 1, 2, BIT(0), 0);
+
+static SUNXI_CCU_M_FW_WITH_GATE(rtc_spi_clk, "rtc-spi", "ahb",
+				0x310, 0, 5, BIT(31), 0);
+
+static struct ccu_common *sun50i_h616_rtc_ccu_clks[] = {
+	&iosc_clk,
+	&iosc_32k_clk,
+	&osc32k_clk.common,
+	&osc24M_32k_clk.common,
+	&osc32k_fanout_clk.common,
+};
+
+static struct ccu_common *sun50i_r329_rtc_ccu_clks[] = {
+	&iosc_clk,
+	&iosc_32k_clk,
+	&ext_osc32k_gate_clk.common,
+	&osc32k_clk.common,
+	&osc24M_32k_clk.common,
+	&rtc_32k_mux_clk.common,
+	&osc32k_fanout_clk.common,
+	&rtc_spi_clk.common,
+};
+
+static struct clk_hw_onecell_data sun50i_h616_rtc_ccu_hw_clks = {
+	.num = CLK_NUMBER,
+	.hws = {
+		[CLK_OSC32K]		= &osc32k_clk.common.hw,
+		[CLK_OSC32K_FANOUT]	= &osc32k_fanout_clk.common.hw,
+		[CLK_IOSC]		= &iosc_clk.hw,
+
+		[CLK_IOSC_32K]		= &iosc_32k_clk.hw,
+		[CLK_EXT_OSC32K_GATE]	= NULL,
+		[CLK_OSC24M_32K]	= &osc24M_32k_clk.common.hw,
+		[CLK_RTC_32K]		= &rtc_32k_fixed_clk.hw,
+		[CLK_RTC_SPI]		= NULL,
+	},
+};
+
+static struct clk_hw_onecell_data sun50i_r329_rtc_ccu_hw_clks = {
+	.num = CLK_NUMBER,
+	.hws = {
+		[CLK_OSC32K]		= &osc32k_clk.common.hw,
+		[CLK_OSC32K_FANOUT]	= &osc32k_fanout_clk.common.hw,
+		[CLK_IOSC]		= &iosc_clk.hw,
+
+		[CLK_IOSC_32K]		= &iosc_32k_clk.hw,
+		[CLK_EXT_OSC32K_GATE]	= &ext_osc32k_gate_clk.common.hw,
+		[CLK_OSC24M_32K]	= &osc24M_32k_clk.common.hw,
+		[CLK_RTC_32K]		= &rtc_32k_mux_clk.common.hw,
+		[CLK_RTC_SPI]		= &rtc_spi_clk.common.hw,
+	},
+};
+
+static const struct sunxi_ccu_desc sun50i_h616_rtc_ccu_desc = {
+	.ccu_clks	= sun50i_h616_rtc_ccu_clks,
+	.num_ccu_clks	= ARRAY_SIZE(sun50i_h616_rtc_ccu_clks),
+
+	.hw_clks	= &sun50i_h616_rtc_ccu_hw_clks,
+};
+
+static const struct sunxi_ccu_desc sun50i_r329_rtc_ccu_desc = {
+	.ccu_clks	= sun50i_r329_rtc_ccu_clks,
+	.num_ccu_clks	= ARRAY_SIZE(sun50i_r329_rtc_ccu_clks),
+
+	.hw_clks	= &sun50i_r329_rtc_ccu_hw_clks,
+};
+
+static void __init sunxi_rtc_ccu_init(struct device_node *node,
+				      const struct sunxi_ccu_desc *desc)
+{
+	void __iomem *reg;
+	int i;
+
+	reg = of_iomap(node, 0);
+	if (IS_ERR(reg)) {
+		pr_err("%pOF: Failed to map registers\n", node);
+		return;
+	}
+
+	/* ext-osc32k was the only input clock in the old binding. */
+	if (!of_property_read_bool(node, "clock-names"))
+		ext_osc32k[0].fw_name = NULL;
+
+	/* Rename the first 3 clocks to respect clock-output-names. */
+	for (i = 0; i < 3; ++i) {
+		struct clk_init_data *init = (struct clk_init_data *)
+			desc->hw_clks->hws[i]->init;
+
+		of_property_read_string_index(node, "clock-output-names", i,
+					      &init->name);
+	}
+
+	of_sunxi_ccu_probe(node, reg, desc);
+}
+
+static void __init sun50i_h616_rtc_ccu_setup(struct device_node *node)
+{
+	have_iosc_calib = 1;
+
+	/* Remove the second parent as external osc32k is not supported. */
+	osc32k_parents[1] = &iosc_32k_clk.hw;
+
+	sunxi_rtc_ccu_init(node, &sun50i_h616_rtc_ccu_desc);
+}
+CLK_OF_DECLARE_DRIVER(sun50i_h616_rtc_ccu, "allwinner,sun50i-h616-rtc",
+		      sun50i_h616_rtc_ccu_setup);
+
+static void __init sun50i_r329_rtc_ccu_setup(struct device_node *node)
+{
+	have_iosc_calib = 0;
+
+	osc32k_fanout_parents[1] = (struct clk_parent_data) {
+		.hw = &ext_osc32k_gate_clk.common.hw
+	};
+
+	sunxi_rtc_ccu_init(node, &sun50i_r329_rtc_ccu_desc);
+}
+CLK_OF_DECLARE_DRIVER(sun50i_r329_rtc_ccu, "allwinner,sun50i-r329-rtc",
+		      sun50i_r329_rtc_ccu_setup);
diff --git a/drivers/clk/sunxi-ng/sun50i-rtc-ccu.h b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.h
new file mode 100644
index 000000000000..7bd4d8700612
--- /dev/null
+++ b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _CCU_SUN50I_RTC_H
+#define _CCU_SUN50I_RTC_H
+
+#include <dt-bindings/clock/sun50i-rtc.h>
+
+#define CLK_IOSC_32K		4
+#define CLK_EXT_OSC32K_GATE	5
+#define CLK_OSC24M_32K		6
+#define CLK_RTC_32K		7
+
+#define CLK_NUMBER		(CLK_RTC_SPI + 1)
+
+#endif /* _CCU_SUN50I_RTC_H */
-- 
2.31.1

[RFC PATCH 6/7] [DO NOT MERGE] clk: sunxi-ng: Add support for H6

[Samuel Holland]

H6 has IOSC calibration and an ext-osc32k input.

H6 has the osc32k mux and the rtc-32k mux, but no fanout mux.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 49 +++++++++++++++++++++++++++
 drivers/rtc/rtc-sun6i.c               | 17 ----------
 2 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
index 1dfa05c2f0e9..9603dc0d3d7b 100644
--- a/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
+++ b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
@@ -227,6 +227,16 @@ static SUNXI_CCU_MUX_DATA_WITH_GATE(osc32k_fanout_clk, "rtc-32k-fanout",
 static SUNXI_CCU_M_FW_WITH_GATE(rtc_spi_clk, "rtc-spi", "ahb",
 				0x310, 0, 5, BIT(31), 0);
 
+static struct ccu_common *sun50i_h6_rtc_ccu_clks[] = {
+	&iosc_clk,
+	&iosc_32k_clk,
+	&ext_osc32k_gate_clk.common,
+	&osc32k_clk.common,
+	&osc24M_32k_clk.common,
+	&rtc_32k_mux_clk.common,
+	&osc32k_fanout_clk.common,
+};
+
 static struct ccu_common *sun50i_h616_rtc_ccu_clks[] = {
 	&iosc_clk,
 	&iosc_32k_clk,
@@ -246,6 +256,21 @@ static struct ccu_common *sun50i_r329_rtc_ccu_clks[] = {
 	&rtc_spi_clk.common,
 };
 
+static struct clk_hw_onecell_data sun50i_h6_rtc_ccu_hw_clks = {
+	.num = CLK_NUMBER,
+	.hws = {
+		[CLK_OSC32K]		= &osc32k_clk.common.hw,
+		[CLK_OSC32K_FANOUT]	= &osc32k_fanout_clk.common.hw,
+		[CLK_IOSC]		= &iosc_clk.hw,
+
+		[CLK_IOSC_32K]		= &iosc_32k_clk.hw,
+		[CLK_EXT_OSC32K_GATE]	= &ext_osc32k_gate_clk.common.hw,
+		[CLK_OSC24M_32K]	= &osc24M_32k_clk.common.hw,
+		[CLK_RTC_32K]		= &rtc_32k_mux_clk.common.hw,
+		[CLK_RTC_SPI]		= NULL,
+	},
+};
+
 static struct clk_hw_onecell_data sun50i_h616_rtc_ccu_hw_clks = {
 	.num = CLK_NUMBER,
 	.hws = {
@@ -276,6 +301,13 @@ static struct clk_hw_onecell_data sun50i_r329_rtc_ccu_hw_clks = {
 	},
 };
 
+static const struct sunxi_ccu_desc sun50i_h6_rtc_ccu_desc = {
+	.ccu_clks	= sun50i_h6_rtc_ccu_clks,
+	.num_ccu_clks	= ARRAY_SIZE(sun50i_h6_rtc_ccu_clks),
+
+	.hw_clks	= &sun50i_h6_rtc_ccu_hw_clks,
+};
+
 static const struct sunxi_ccu_desc sun50i_h616_rtc_ccu_desc = {
 	.ccu_clks	= sun50i_h616_rtc_ccu_clks,
 	.num_ccu_clks	= ARRAY_SIZE(sun50i_h616_rtc_ccu_clks),
@@ -318,6 +350,23 @@ static void __init sunxi_rtc_ccu_init(struct device_node *node,
 	of_sunxi_ccu_probe(node, reg, desc);
 }
 
+static void __init sun50i_h6_rtc_ccu_setup(struct device_node *node)
+{
+	struct clk_init_data *init;
+
+	have_iosc_calib = 1;
+
+	/* Casting away the const from a pointer to a non-const anonymous object... */
+	init = (struct clk_init_data *)osc32k_fanout_clk.common.hw.init;
+
+	/* Fanout only has one parent: osc32k. */
+	init->num_parents = 1;
+
+	sunxi_rtc_ccu_init(node, &sun50i_h6_rtc_ccu_desc);
+}
+CLK_OF_DECLARE_DRIVER(sun50i_h6_rtc_ccu, "allwinner,sun50i-h6-rtc",
+		      sun50i_h6_rtc_ccu_setup);
+
 static void __init sun50i_h616_rtc_ccu_setup(struct device_node *node)
 {
 	have_iosc_calib = 1;
diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index adec1b14a8de..f7cbbf57f112 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -363,23 +363,6 @@ CLK_OF_DECLARE_DRIVER(sun8i_h3_rtc_clk, "allwinner,sun8i-h3-rtc",
 CLK_OF_DECLARE_DRIVER(sun50i_h5_rtc_clk, "allwinner,sun50i-h5-rtc",
 		      sun8i_h3_rtc_clk_init);
 
-static const struct sun6i_rtc_clk_data sun50i_h6_rtc_data = {
-	.rc_osc_rate = 16000000,
-	.fixed_prescaler = 32,
-	.has_prescaler = 1,
-	.has_out_clk = 1,
-	.export_iosc = 1,
-	.has_losc_en = 1,
-	.has_auto_swt = 1,
-};
-
-static void __init sun50i_h6_rtc_clk_init(struct device_node *node)
-{
-	sun6i_rtc_clk_init(node, &sun50i_h6_rtc_data);
-}
-CLK_OF_DECLARE_DRIVER(sun50i_h6_rtc_clk, "allwinner,sun50i-h6-rtc",
-		      sun50i_h6_rtc_clk_init);
-
 /*
  * The R40 user manual is self-conflicting on whether the prescaler is
  * fixed or configurable. The clock diagram shows it as fixed, but there
-- 
2.31.1

[RFC PATCH 7/7] [DO NOT MERGE] clk: sunxi-ng: Add support for T5

[Samuel Holland]

The T5 RTC is similar to the H616 RTC (no rtc-32k mux, pll-32k as the
second fanout input), except that it adds the ext-osc32k input.

It also isn't a "sun50i" SoC, so it creates a bit of a naming problem...

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 40 +++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
index 9603dc0d3d7b..fe6b21a24193 100644
--- a/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
+++ b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
@@ -227,6 +227,15 @@ static SUNXI_CCU_MUX_DATA_WITH_GATE(osc32k_fanout_clk, "rtc-32k-fanout",
 static SUNXI_CCU_M_FW_WITH_GATE(rtc_spi_clk, "rtc-spi", "ahb",
 				0x310, 0, 5, BIT(31), 0);
 
+static struct ccu_common *sun8i_t5_rtc_ccu_clks[] = {
+	&iosc_clk,
+	&iosc_32k_clk,
+	&ext_osc32k_gate_clk.common,
+	&osc32k_clk.common,
+	&osc24M_32k_clk.common,
+	&osc32k_fanout_clk.common,
+};
+
 static struct ccu_common *sun50i_h6_rtc_ccu_clks[] = {
 	&iosc_clk,
 	&iosc_32k_clk,
@@ -256,6 +265,21 @@ static struct ccu_common *sun50i_r329_rtc_ccu_clks[] = {
 	&rtc_spi_clk.common,
 };
 
+static struct clk_hw_onecell_data sun8i_t5_rtc_ccu_hw_clks = {
+	.num = CLK_NUMBER,
+	.hws = {
+		[CLK_OSC32K]		= &osc32k_clk.common.hw,
+		[CLK_OSC32K_FANOUT]	= &osc32k_fanout_clk.common.hw,
+		[CLK_IOSC]		= &iosc_clk.hw,
+
+		[CLK_IOSC_32K]		= &iosc_32k_clk.hw,
+		[CLK_EXT_OSC32K_GATE]	= &ext_osc32k_gate_clk.common.hw,
+		[CLK_OSC24M_32K]	= &osc24M_32k_clk.common.hw,
+		[CLK_RTC_32K]		= &rtc_32k_fixed_clk.hw,
+		[CLK_RTC_SPI]		= NULL,
+	},
+};
+
 static struct clk_hw_onecell_data sun50i_h6_rtc_ccu_hw_clks = {
 	.num = CLK_NUMBER,
 	.hws = {
@@ -301,6 +325,13 @@ static struct clk_hw_onecell_data sun50i_r329_rtc_ccu_hw_clks = {
 	},
 };
 
+static const struct sunxi_ccu_desc sun8i_t5_rtc_ccu_desc = {
+	.ccu_clks	= sun8i_t5_rtc_ccu_clks,
+	.num_ccu_clks	= ARRAY_SIZE(sun8i_t5_rtc_ccu_clks),
+
+	.hw_clks	= &sun8i_t5_rtc_ccu_hw_clks,
+};
+
 static const struct sunxi_ccu_desc sun50i_h6_rtc_ccu_desc = {
 	.ccu_clks	= sun50i_h6_rtc_ccu_clks,
 	.num_ccu_clks	= ARRAY_SIZE(sun50i_h6_rtc_ccu_clks),
@@ -350,6 +381,15 @@ static void __init sunxi_rtc_ccu_init(struct device_node *node,
 	of_sunxi_ccu_probe(node, reg, desc);
 }
 
+static void __init sun8i_t5_rtc_ccu_setup(struct device_node *node)
+{
+	have_iosc_calib = 1;
+
+	sunxi_rtc_ccu_init(node, &sun8i_t5_rtc_ccu_desc);
+}
+CLK_OF_DECLARE_DRIVER(sun8i_t5_rtc_ccu, "allwinner,sun8i-t5-rtc",
+		      sun8i_t5_rtc_ccu_setup);
+
 static void __init sun50i_h6_rtc_ccu_setup(struct device_node *node)
 {
 	struct clk_init_data *init;
-- 
2.31.1

Re: [RFC PATCH 1/7] dt-bindings: rtc: sun6i: Add H616 and R329 compatibles

[Rob Herring]

On Wed, 01 Sep 2021 00:39:45 -0500, Samuel Holland wrote:
> For these new SoCs, start requiring a complete list of input clocks.
> 
> For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
> bus, and hosc; and optionally ext-osc32k.
> 
> I'm not sure how to best represent this in the binding...
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
>  include/dt-bindings/clock/sun50i-rtc.h        | 12 ++++
>  2 files changed, 61 insertions(+), 6 deletions(-)
>  create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Traceback (most recent call last):
  File "/usr/local/bin/dt-doc-validate", line 67, in <module>
    ret = check_doc(f)
  File "/usr/local/bin/dt-doc-validate", line 33, in check_doc
    for error in sorted(dtschema.DTValidator.iter_schema_errors(testtree), key=lambda e: e.linecol):
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 723, in iter_schema_errors
    cls(meta_schema).annotate_error(error, meta_schema, error.schema_path)
  File "/usr/local/lib/python3.8/dist-packages/dtschema/lib.py", line 706, in annotate_error
    schema = schema[p]
KeyError: 'properties'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml: ignoring, error in schema: allOf: 5: if: properties
warning: no schema found in file: ./Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.example.dt.yaml:0:0: /example-0/rtc@1f00000: failed to match any schema with compatible: ['allwinner,sun6i-a31-rtc']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1522863

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [RFC PATCH 1/7] dt-bindings: rtc: sun6i: Add H616 and R329 compatibles

[Rob Herring]

On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote:
> For these new SoCs, start requiring a complete list of input clocks.
> 
> For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
> bus, and hosc; and optionally ext-osc32k.
> 
> I'm not sure how to best represent this in the binding...
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
>  include/dt-bindings/clock/sun50i-rtc.h        | 12 ++++
>  2 files changed, 61 insertions(+), 6 deletions(-)
>  create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
> 
> diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> index beeb90e55727..3e085db1294f 100644
> --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> @@ -26,6 +26,8 @@ properties:
>            - const: allwinner,sun50i-a64-rtc
>            - const: allwinner,sun8i-h3-rtc
>        - const: allwinner,sun50i-h6-rtc
> +      - const: allwinner,sun50i-h616-rtc
> +      - const: allwinner,sun50i-r329-rtc

Can you please make all the single entry cases a single 'enum'.

>  
>    reg:
>      maxItems: 1
> @@ -37,7 +39,24 @@ properties:
>        - description: RTC Alarm 1
>  
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 4
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 4
> +    items:
> +      - anyOf:

This says the first entry is any of these. What about the rest of them?

> +          - const: ahb
> +            description: AHB parent for SPI bus clock

The description should go in 'clocks'. The order should be defined as 
well with the first clock being the one that existed previously.

> +          - const: bus
> +            description: AHB/APB bus clock for register access
> +          - const: ext-osc32k
> +            description: External 32768 Hz oscillator input
> +          - const: hosc
> +            description: 24 MHz oscillator input
> +          - const: pll-32k
> +            description: 32 kHz clock divided from a PLL
>  
>    clock-output-names:
>      minItems: 1
> @@ -85,6 +104,9 @@ allOf:
>              enum:
>                - allwinner,sun8i-h3-rtc
>                - allwinner,sun50i-h5-rtc
> +              - allwinner,sun50i-h6-rtc
> +              - allwinner,sun50i-h616-rtc
> +              - allwinner,sun50i-r329-rtc
>  
>      then:
>        properties:
> @@ -96,13 +118,35 @@ allOf:
>        properties:
>          compatible:
>            contains:
> -            const: allwinner,sun50i-h6-rtc
> +            enum:
> +              - allwinner,sun50i-h616-rtc
> +              - allwinner,sun50i-r329-rtc
>  
>      then:
> +      clocks:
> +        minItems: 3 # bus, hosc, and (pll-32k [H616] or ahb [R329])
> +
> +      clock-names:
> +        minItems: 3
> +
> +      required:
> +        - clock-names
> +
> +    else:
> +      required:
> +        - clock-output-names
> +
> +  - if:
> +      properties: clock-names
> +
> +    then:
> +      required:
> +        - clocks # hosc is required
> +
> +    else:
>        properties:
> -        clock-output-names:
> -          minItems: 3
> -          maxItems: 3
> +        clocks:
> +          maxItems: 1 # only ext-osc32k is allowed
>  
>    - if:
>        properties:
> @@ -127,7 +171,6 @@ required:
>    - compatible
>    - reg
>    - interrupts
> -  - clock-output-names
>  
>  additionalProperties: false
>  
> diff --git a/include/dt-bindings/clock/sun50i-rtc.h b/include/dt-bindings/clock/sun50i-rtc.h
> new file mode 100644
> index 000000000000..d45e3ff4e105
> --- /dev/null
> +++ b/include/dt-bindings/clock/sun50i-rtc.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Dual license please.

> +
> +#ifndef _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_
> +#define _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_
> +
> +#define CLK_OSC32K		0
> +#define CLK_OSC32K_FANOUT	1
> +#define CLK_IOSC		2
> +
> +#define CLK_RTC_SPI		8
> +
> +#endif /* _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ */
> -- 
> 2.31.1
> 
> 

Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1849 bytes --]

Hi,

On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
> This patch series adds a CCU driver for the RTC in the H616 and R329.
> The extra patches at the end of this series show how it would be
> explanded to additional hardware variants.
> 
> The driver is intended to support the existing binding used for the H6,
> but also an updated binding which includes all RTC input clocks. I do
> not know how to best represent that binding -- that is a major reason
> why this series is an RFC.
> 
> A future patch series could add functionality to the driver to manage
> IOSC calibration at boot and during suspend/resume.
> 
> It may be possible to support all of these hardware variants in the
> existing RTC clock driver and avoid some duplicate code, but I'm
> concerned about the complexity there, without any of the CCU
> abstraction.
> 
> This series is currently based on top of the other series I just sent
> (clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
> elsewhere.

I'm generally ok with this, it makes sense to move it to sunxi-ng,
especially with that other series of yours.

My main concern about this is the split driver approach. We used to have
that before in the RTC too, but it was mostly due to the early clock
requirements. With your previous work, that requirement is not there
anymore and we can just register it as a device just like the other
clock providers.

And since we can register all those clocks at device probe time, we
don't really need to split the driver in two (and especially in two
different places). The only obstacle to this after your previous series
is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
functions public, but that can easily be fixed by moving their
definition to include/linux/clk/sunxi-ng.h

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 6/7] [DO NOT MERGE] clk: sunxi-ng: Add support for H6

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3204 bytes --]

On Wed, Sep 01, 2021 at 12:39:50AM -0500, Samuel Holland wrote:
> H6 has IOSC calibration and an ext-osc32k input.
> 
> H6 has the osc32k mux and the rtc-32k mux, but no fanout mux.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 49 +++++++++++++++++++++++++++
>  drivers/rtc/rtc-sun6i.c               | 17 ----------
>  2 files changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
> index 1dfa05c2f0e9..9603dc0d3d7b 100644
> --- a/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
> +++ b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
> @@ -227,6 +227,16 @@ static SUNXI_CCU_MUX_DATA_WITH_GATE(osc32k_fanout_clk, "rtc-32k-fanout",
>  static SUNXI_CCU_M_FW_WITH_GATE(rtc_spi_clk, "rtc-spi", "ahb",
>  				0x310, 0, 5, BIT(31), 0);
>  
> +static struct ccu_common *sun50i_h6_rtc_ccu_clks[] = {
> +	&iosc_clk,
> +	&iosc_32k_clk,
> +	&ext_osc32k_gate_clk.common,
> +	&osc32k_clk.common,
> +	&osc24M_32k_clk.common,
> +	&rtc_32k_mux_clk.common,
> +	&osc32k_fanout_clk.common,
> +};
> +
>  static struct ccu_common *sun50i_h616_rtc_ccu_clks[] = {
>  	&iosc_clk,
>  	&iosc_32k_clk,
> @@ -246,6 +256,21 @@ static struct ccu_common *sun50i_r329_rtc_ccu_clks[] = {
>  	&rtc_spi_clk.common,
>  };
>  
> +static struct clk_hw_onecell_data sun50i_h6_rtc_ccu_hw_clks = {
> +	.num = CLK_NUMBER,
> +	.hws = {
> +		[CLK_OSC32K]		= &osc32k_clk.common.hw,
> +		[CLK_OSC32K_FANOUT]	= &osc32k_fanout_clk.common.hw,
> +		[CLK_IOSC]		= &iosc_clk.hw,
> +
> +		[CLK_IOSC_32K]		= &iosc_32k_clk.hw,
> +		[CLK_EXT_OSC32K_GATE]	= &ext_osc32k_gate_clk.common.hw,
> +		[CLK_OSC24M_32K]	= &osc24M_32k_clk.common.hw,
> +		[CLK_RTC_32K]		= &rtc_32k_mux_clk.common.hw,
> +		[CLK_RTC_SPI]		= NULL,
> +	},
> +};
> +
>  static struct clk_hw_onecell_data sun50i_h616_rtc_ccu_hw_clks = {
>  	.num = CLK_NUMBER,
>  	.hws = {
> @@ -276,6 +301,13 @@ static struct clk_hw_onecell_data sun50i_r329_rtc_ccu_hw_clks = {
>  	},
>  };
>  
> +static const struct sunxi_ccu_desc sun50i_h6_rtc_ccu_desc = {
> +	.ccu_clks	= sun50i_h6_rtc_ccu_clks,
> +	.num_ccu_clks	= ARRAY_SIZE(sun50i_h6_rtc_ccu_clks),
> +
> +	.hw_clks	= &sun50i_h6_rtc_ccu_hw_clks,
> +};
> +
>  static const struct sunxi_ccu_desc sun50i_h616_rtc_ccu_desc = {
>  	.ccu_clks	= sun50i_h616_rtc_ccu_clks,
>  	.num_ccu_clks	= ARRAY_SIZE(sun50i_h616_rtc_ccu_clks),
> @@ -318,6 +350,23 @@ static void __init sunxi_rtc_ccu_init(struct device_node *node,
>  	of_sunxi_ccu_probe(node, reg, desc);
>  }
>  
> +static void __init sun50i_h6_rtc_ccu_setup(struct device_node *node)
> +{
> +	struct clk_init_data *init;
> +
> +	have_iosc_calib = 1;
> +
> +	/* Casting away the const from a pointer to a non-const anonymous object... */
> +	init = (struct clk_init_data *)osc32k_fanout_clk.common.hw.init;
> +
> +	/* Fanout only has one parent: osc32k. */
> +	init->num_parents = 1;
> +
> +	sunxi_rtc_ccu_init(node, &sun50i_h6_rtc_ccu_desc);
> +}

Indeed, that's not great.

Maybe we should just duplicate the sun50i_h6_rtc_ccu_desc (and
osc32k_fanout_clk) to cover both cases?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 6/7] [DO NOT MERGE] clk: sunxi-ng: Add support for H6

[Samuel Holland]

On 9/3/21 9:51 AM, Maxime Ripard wrote:
> On Wed, Sep 01, 2021 at 12:39:50AM -0500, Samuel Holland wrote:
>> H6 has IOSC calibration and an ext-osc32k input.
>>
>> H6 has the osc32k mux and the rtc-32k mux, but no fanout mux.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  drivers/clk/sunxi-ng/sun50i-rtc-ccu.c | 49 +++++++++++++++++++++++++++
>>  drivers/rtc/rtc-sun6i.c               | 17 ----------
>>  2 files changed, 49 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
>> index 1dfa05c2f0e9..9603dc0d3d7b 100644
>> --- a/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
>> +++ b/drivers/clk/sunxi-ng/sun50i-rtc-ccu.c
>> @@ -227,6 +227,16 @@ static SUNXI_CCU_MUX_DATA_WITH_GATE(osc32k_fanout_clk, "rtc-32k-fanout",
>>  static SUNXI_CCU_M_FW_WITH_GATE(rtc_spi_clk, "rtc-spi", "ahb",
>>  				0x310, 0, 5, BIT(31), 0);
>>  
>> +static struct ccu_common *sun50i_h6_rtc_ccu_clks[] = {
>> +	&iosc_clk,
>> +	&iosc_32k_clk,
>> +	&ext_osc32k_gate_clk.common,
>> +	&osc32k_clk.common,
>> +	&osc24M_32k_clk.common,
>> +	&rtc_32k_mux_clk.common,
>> +	&osc32k_fanout_clk.common,
>> +};
>> +
>>  static struct ccu_common *sun50i_h616_rtc_ccu_clks[] = {
>>  	&iosc_clk,
>>  	&iosc_32k_clk,
>> @@ -246,6 +256,21 @@ static struct ccu_common *sun50i_r329_rtc_ccu_clks[] = {
>>  	&rtc_spi_clk.common,
>>  };
>>  
>> +static struct clk_hw_onecell_data sun50i_h6_rtc_ccu_hw_clks = {
>> +	.num = CLK_NUMBER,
>> +	.hws = {
>> +		[CLK_OSC32K]		= &osc32k_clk.common.hw,
>> +		[CLK_OSC32K_FANOUT]	= &osc32k_fanout_clk.common.hw,
>> +		[CLK_IOSC]		= &iosc_clk.hw,
>> +
>> +		[CLK_IOSC_32K]		= &iosc_32k_clk.hw,
>> +		[CLK_EXT_OSC32K_GATE]	= &ext_osc32k_gate_clk.common.hw,
>> +		[CLK_OSC24M_32K]	= &osc24M_32k_clk.common.hw,
>> +		[CLK_RTC_32K]		= &rtc_32k_mux_clk.common.hw,
>> +		[CLK_RTC_SPI]		= NULL,
>> +	},
>> +};
>> +
>>  static struct clk_hw_onecell_data sun50i_h616_rtc_ccu_hw_clks = {
>>  	.num = CLK_NUMBER,
>>  	.hws = {
>> @@ -276,6 +301,13 @@ static struct clk_hw_onecell_data sun50i_r329_rtc_ccu_hw_clks = {
>>  	},
>>  };
>>  
>> +static const struct sunxi_ccu_desc sun50i_h6_rtc_ccu_desc = {
>> +	.ccu_clks	= sun50i_h6_rtc_ccu_clks,
>> +	.num_ccu_clks	= ARRAY_SIZE(sun50i_h6_rtc_ccu_clks),
>> +
>> +	.hw_clks	= &sun50i_h6_rtc_ccu_hw_clks,
>> +};
>> +
>>  static const struct sunxi_ccu_desc sun50i_h616_rtc_ccu_desc = {
>>  	.ccu_clks	= sun50i_h616_rtc_ccu_clks,
>>  	.num_ccu_clks	= ARRAY_SIZE(sun50i_h616_rtc_ccu_clks),
>> @@ -318,6 +350,23 @@ static void __init sunxi_rtc_ccu_init(struct device_node *node,
>>  	of_sunxi_ccu_probe(node, reg, desc);
>>  }
>>  
>> +static void __init sun50i_h6_rtc_ccu_setup(struct device_node *node)
>> +{
>> +	struct clk_init_data *init;
>> +
>> +	have_iosc_calib = 1;
>> +
>> +	/* Casting away the const from a pointer to a non-const anonymous object... */
>> +	init = (struct clk_init_data *)osc32k_fanout_clk.common.hw.init;
>> +
>> +	/* Fanout only has one parent: osc32k. */
>> +	init->num_parents = 1;
>> +
>> +	sunxi_rtc_ccu_init(node, &sun50i_h6_rtc_ccu_desc);
>> +}
> 
> Indeed, that's not great.
> 
> Maybe we should just duplicate the sun50i_h6_rtc_ccu_desc (and
> osc32k_fanout_clk) to cover both cases?

Right, I could split osc32k_fanout_clk like with rtc_32k_fixed_clk and
rtc_32k_mux_clk. Then H6 would use osc32k_fanout_fixed_clk.

Regards,
Samuel

Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

[Samuel Holland]

On 9/3/21 9:50 AM, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
>> This patch series adds a CCU driver for the RTC in the H616 and R329.
>> The extra patches at the end of this series show how it would be
>> explanded to additional hardware variants.
>>
>> The driver is intended to support the existing binding used for the H6,
>> but also an updated binding which includes all RTC input clocks. I do
>> not know how to best represent that binding -- that is a major reason
>> why this series is an RFC.
>>
>> A future patch series could add functionality to the driver to manage
>> IOSC calibration at boot and during suspend/resume.
>>
>> It may be possible to support all of these hardware variants in the
>> existing RTC clock driver and avoid some duplicate code, but I'm
>> concerned about the complexity there, without any of the CCU
>> abstraction.
>>
>> This series is currently based on top of the other series I just sent
>> (clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
>> elsewhere.
> 
> I'm generally ok with this, it makes sense to move it to sunxi-ng,
> especially with that other series of yours.
> 
> My main concern about this is the split driver approach. We used to have
> that before in the RTC too, but it was mostly due to the early clock
> requirements. With your previous work, that requirement is not there
> anymore and we can just register it as a device just like the other
> clock providers.

That's a good point. Originally, I had this RTC CCU providing osc24M, so
it did need to be an early provider. But with the current version, we
could have the RTC platform driver call devm_sunxi_ccu_probe. That does
seem cleaner.

(Since it wasn't immediately obvious to me why this works: the only
early provider remaining is the sun5i CCU, and it doesn't use the sun6i
RTC driver.)

> And since we can register all those clocks at device probe time, we
> don't really need to split the driver in two (and especially in two
> different places). The only obstacle to this after your previous series
> is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
> functions public, but that can easily be fixed by moving their
> definition to include/linux/clk/sunxi-ng.h

Where are you thinking the clock definitions would go? We don't export
any of those structures (ccu_mux, ccu_common) or macros
(SUNXI_CCU_GATE_DATA) in a public header either.

Would you want to export those? That seems like a lot of churn. Or would
we put the CCU descriptions in drivers/clk/sunxi-ng and export a
function that the RTC driver can call? (Or some other idea?)

Regards,
Samuel

Re: [RFC PATCH 1/7] dt-bindings: rtc: sun6i: Add H616 and R329 compatibles

[Samuel Holland]

On 9/2/21 10:27 AM, Rob Herring wrote:
> On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote:
>> For these new SoCs, start requiring a complete list of input clocks.
>>
>> For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
>> bus, and hosc; and optionally ext-osc32k.
>>
>> I'm not sure how to best represent this in the binding...
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
>>  include/dt-bindings/clock/sun50i-rtc.h        | 12 ++++
>>  2 files changed, 61 insertions(+), 6 deletions(-)
>>  create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
>> index beeb90e55727..3e085db1294f 100644
>> --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
>> +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
>> @@ -26,6 +26,8 @@ properties:
>>            - const: allwinner,sun50i-a64-rtc
>>            - const: allwinner,sun8i-h3-rtc
>>        - const: allwinner,sun50i-h6-rtc
>> +      - const: allwinner,sun50i-h616-rtc
>> +      - const: allwinner,sun50i-r329-rtc
> 
> Can you please make all the single entry cases a single 'enum'.
> 
>>  
>>    reg:
>>      maxItems: 1
>> @@ -37,7 +39,24 @@ properties:
>>        - description: RTC Alarm 1
>>  
>>    clocks:
>> -    maxItems: 1
>> +    minItems: 1
>> +    maxItems: 4
>> +
>> +  clock-names:
>> +    minItems: 1
>> +    maxItems: 4
>> +    items:
>> +      - anyOf:
> 
> This says the first entry is any of these. What about the rest of them?

Oh, right. The list below is the list of all possible clocks.

>> +          - const: ahb
>> +            description: AHB parent for SPI bus clock
> 
> The description should go in 'clocks'.

Will do for v2.

> The order should be defined as well with the first clock being the
> one that existed previously.

The only way I know how to further refine the list is with
minItems/maxItems. My problem is that 1) some clocks are only valid for
certain SoCs, and 2) some clocks are optional, depending on how the
board is wired. So there is no single order where the "valid"
combinations are prefixes of the "possible" combinations of clocks.

Or in other words, how can I say "clocks #1 and #2 from this list are
required, and #4 is optional, but #3 is not allowed"?

Some concrete examples, with the always-required clocks moved to the
beginning:

H6:
 - bus: required
 - hosc: required
 - ahb: not allowed
 - ext-osc32k: optional
 - pll-32k: not allowed

H616:
 - bus: required
 - hosc: required
 - ahb: not allowed
 - ext-osc32k: not allowed
 - pll-32k: required

R329:
 - bus: required
 - hosc: required
 - ahb: required
 - ext-osc32k: optional
 - pll-32k: not allowed

Should I just move the entire clocks/clock-items properties to if/then
blocks based on the compatible?

>> +          - const: bus
>> +            description: AHB/APB bus clock for register access
>> +          - const: ext-osc32k
>> +            description: External 32768 Hz oscillator input
>> +          - const: hosc
>> +            description: 24 MHz oscillator input
>> +          - const: pll-32k
>> +            description: 32 kHz clock divided from a PLL
>>  
>>    clock-output-names:
>>      minItems: 1
>> @@ -85,6 +104,9 @@ allOf:
>>              enum:
>>                - allwinner,sun8i-h3-rtc
>>                - allwinner,sun50i-h5-rtc
>> +              - allwinner,sun50i-h6-rtc
>> +              - allwinner,sun50i-h616-rtc
>> +              - allwinner,sun50i-r329-rtc
>>  
>>      then:
>>        properties:
>> @@ -96,13 +118,35 @@ allOf:
>>        properties:
>>          compatible:
>>            contains:
>> -            const: allwinner,sun50i-h6-rtc
>> +            enum:
>> +              - allwinner,sun50i-h616-rtc
>> +              - allwinner,sun50i-r329-rtc
>>  
>>      then:
>> +      clocks:
>> +        minItems: 3 # bus, hosc, and (pll-32k [H616] or ahb [R329])
>> +
>> +      clock-names:
>> +        minItems: 3
>> +
>> +      required:
>> +        - clock-names
>> +
>> +    else:
>> +      required:
>> +        - clock-output-names
>> +
>> +  - if:
>> +      properties: clock-names
>> +
>> +    then:
>> +      required:
>> +        - clocks # hosc is required
>> +
>> +    else:
>>        properties:
>> -        clock-output-names:
>> -          minItems: 3
>> -          maxItems: 3
>> +        clocks:
>> +          maxItems: 1 # only ext-osc32k is allowed
>>  
>>    - if:
>>        properties:
>> @@ -127,7 +171,6 @@ required:
>>    - compatible
>>    - reg
>>    - interrupts
>> -  - clock-output-names
>>  
>>  additionalProperties: false
>>  
>> diff --git a/include/dt-bindings/clock/sun50i-rtc.h b/include/dt-bindings/clock/sun50i-rtc.h
>> new file mode 100644
>> index 000000000000..d45e3ff4e105
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/sun50i-rtc.h
>> @@ -0,0 +1,12 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> Dual license please.

Will do for v2.

Regards,
Samuel

>> +
>> +#ifndef _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_
>> +#define _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_
>> +
>> +#define CLK_OSC32K		0
>> +#define CLK_OSC32K_FANOUT	1
>> +#define CLK_IOSC		2
>> +
>> +#define CLK_RTC_SPI		8
>> +
>> +#endif /* _DT_BINDINGS_CLK_SUN50I_RTC_CCU_H_ */
>> -- 
>> 2.31.1
>>
>>

Re: [RFC PATCH 1/7] dt-bindings: rtc: sun6i: Add H616 and R329 compatibles

[Rob Herring]

On Fri, Sep 3, 2021 at 10:36 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 9/2/21 10:27 AM, Rob Herring wrote:
> > On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote:
> >> For these new SoCs, start requiring a complete list of input clocks.
> >>
> >> For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
> >> bus, and hosc; and optionally ext-osc32k.
> >>
> >> I'm not sure how to best represent this in the binding...
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>  .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
> >>  include/dt-bindings/clock/sun50i-rtc.h        | 12 ++++
> >>  2 files changed, 61 insertions(+), 6 deletions(-)
> >>  create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> >> index beeb90e55727..3e085db1294f 100644
> >> --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> >> +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
> >> @@ -26,6 +26,8 @@ properties:
> >>            - const: allwinner,sun50i-a64-rtc
> >>            - const: allwinner,sun8i-h3-rtc
> >>        - const: allwinner,sun50i-h6-rtc
> >> +      - const: allwinner,sun50i-h616-rtc
> >> +      - const: allwinner,sun50i-r329-rtc
> >
> > Can you please make all the single entry cases a single 'enum'.
> >
> >>
> >>    reg:
> >>      maxItems: 1
> >> @@ -37,7 +39,24 @@ properties:
> >>        - description: RTC Alarm 1
> >>
> >>    clocks:
> >> -    maxItems: 1
> >> +    minItems: 1
> >> +    maxItems: 4
> >> +
> >> +  clock-names:
> >> +    minItems: 1
> >> +    maxItems: 4
> >> +    items:
> >> +      - anyOf:
> >
> > This says the first entry is any of these. What about the rest of them?
>
> Oh, right. The list below is the list of all possible clocks.
>
> >> +          - const: ahb
> >> +            description: AHB parent for SPI bus clock
> >
> > The description should go in 'clocks'.
>
> Will do for v2.
>
> > The order should be defined as well with the first clock being the
> > one that existed previously.
>
> The only way I know how to further refine the list is with
> minItems/maxItems. My problem is that 1) some clocks are only valid for
> certain SoCs, and 2) some clocks are optional, depending on how the
> board is wired. So there is no single order where the "valid"
> combinations are prefixes of the "possible" combinations of clocks.
>
> Or in other words, how can I say "clocks #1 and #2 from this list are
> required, and #4 is optional, but #3 is not allowed"?

This says you have up to 4 clocks, but only defines the 1st 2:

maxItems: 4
items:
  - description: 1st clock
  - description: 2nd clock

But I think you will be better off with just defining the range
(minItems/maxItems) at the top level and then use if/then schemas.

>
> Some concrete examples, with the always-required clocks moved to the
> beginning:
>
> H6:
>  - bus: required
>  - hosc: required
>  - ahb: not allowed
>  - ext-osc32k: optional
>  - pll-32k: not allowed

Is this really 2 different 32k clock inputs to the h/w block? Doesn't
seem like it given both are never valid.

>
> H616:
>  - bus: required
>  - hosc: required
>  - ahb: not allowed
>  - ext-osc32k: not allowed
>  - pll-32k: required
>
> R329:
>  - bus: required
>  - hosc: required
>  - ahb: required
>  - ext-osc32k: optional
>  - pll-32k: not allowed
>
> Should I just move the entire clocks/clock-items properties to if/then
> blocks based on the compatible?

Probably so.

Rob

Re: [RFC PATCH 1/7] dt-bindings: rtc: sun6i: Add H616 and R329 compatibles

[Samuel Holland]

On 9/7/21 9:44 AM, Rob Herring wrote:
> On Fri, Sep 3, 2021 at 10:36 AM Samuel Holland <samuel@sholland.org> wrote:
>>
>> On 9/2/21 10:27 AM, Rob Herring wrote:
>>> On Wed, Sep 01, 2021 at 12:39:45AM -0500, Samuel Holland wrote:
>>>> For these new SoCs, start requiring a complete list of input clocks.
>>>>
>>>> For H616, this means bus, hosc, and pll-32k. For R329, this means ahb,
>>>> bus, and hosc; and optionally ext-osc32k.
>>>>
>>>> I'm not sure how to best represent this in the binding...
>>>>
>>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>>> ---
>>>>  .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 55 +++++++++++++++++--
>>>>  include/dt-bindings/clock/sun50i-rtc.h        | 12 ++++
>>>>  2 files changed, 61 insertions(+), 6 deletions(-)
>>>>  create mode 100644 include/dt-bindings/clock/sun50i-rtc.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
>>>> index beeb90e55727..3e085db1294f 100644
>>>> --- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
>>>> +++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
>>>> @@ -26,6 +26,8 @@ properties:
>>>>            - const: allwinner,sun50i-a64-rtc
>>>>            - const: allwinner,sun8i-h3-rtc
>>>>        - const: allwinner,sun50i-h6-rtc
>>>> +      - const: allwinner,sun50i-h616-rtc
>>>> +      - const: allwinner,sun50i-r329-rtc
>>>
>>> Can you please make all the single entry cases a single 'enum'.
>>>
>>>>
>>>>    reg:
>>>>      maxItems: 1
>>>> @@ -37,7 +39,24 @@ properties:
>>>>        - description: RTC Alarm 1
>>>>
>>>>    clocks:
>>>> -    maxItems: 1
>>>> +    minItems: 1
>>>> +    maxItems: 4
>>>> +
>>>> +  clock-names:
>>>> +    minItems: 1
>>>> +    maxItems: 4
>>>> +    items:
>>>> +      - anyOf:
>>>
>>> This says the first entry is any of these. What about the rest of them?
>>
>> Oh, right. The list below is the list of all possible clocks.
>>
>>>> +          - const: ahb
>>>> +            description: AHB parent for SPI bus clock
>>>
>>> The description should go in 'clocks'.
>>
>> Will do for v2.
>>
>>> The order should be defined as well with the first clock being the
>>> one that existed previously.
>>
>> The only way I know how to further refine the list is with
>> minItems/maxItems. My problem is that 1) some clocks are only valid for
>> certain SoCs, and 2) some clocks are optional, depending on how the
>> board is wired. So there is no single order where the "valid"
>> combinations are prefixes of the "possible" combinations of clocks.
>>
>> Or in other words, how can I say "clocks #1 and #2 from this list are
>> required, and #4 is optional, but #3 is not allowed"?
> 
> This says you have up to 4 clocks, but only defines the 1st 2:
> 
> maxItems: 4
> items:
>   - description: 1st clock
>   - description: 2nd clock
> 
> But I think you will be better off with just defining the range
> (minItems/maxItems) at the top level and then use if/then schemas.

Ah, thanks for the suggestions.

>>
>> Some concrete examples, with the always-required clocks moved to the
>> beginning:
>>
>> H6:
>>  - bus: required
>>  - hosc: required
>>  - ahb: not allowed
>>  - ext-osc32k: optional
>>  - pll-32k: not allowed
> 
> Is this really 2 different 32k clock inputs to the h/w block? Doesn't
> seem like it given both are never valid.

Yes, there are two separate 32k inputs. Both are valid at the same time
on some SoCs like T5 (patch 7), but not on any of those I listed here.

Regards,
Samuel

>>
>> H616:
>>  - bus: required
>>  - hosc: required
>>  - ahb: not allowed
>>  - ext-osc32k: not allowed
>>  - pll-32k: required
>>
>> R329:
>>  - bus: required
>>  - hosc: required
>>  - ahb: required
>>  - ext-osc32k: optional
>>  - pll-32k: not allowed
>>
>> Should I just move the entire clocks/clock-items properties to if/then
>> blocks based on the compatible?
> 
> Probably so.
> 
> Rob
> 

Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3631 bytes --]

On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
> On 9/3/21 9:50 AM, Maxime Ripard wrote:
> > Hi,
> > 
> > On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
> >> This patch series adds a CCU driver for the RTC in the H616 and R329.
> >> The extra patches at the end of this series show how it would be
> >> explanded to additional hardware variants.
> >>
> >> The driver is intended to support the existing binding used for the H6,
> >> but also an updated binding which includes all RTC input clocks. I do
> >> not know how to best represent that binding -- that is a major reason
> >> why this series is an RFC.
> >>
> >> A future patch series could add functionality to the driver to manage
> >> IOSC calibration at boot and during suspend/resume.
> >>
> >> It may be possible to support all of these hardware variants in the
> >> existing RTC clock driver and avoid some duplicate code, but I'm
> >> concerned about the complexity there, without any of the CCU
> >> abstraction.
> >>
> >> This series is currently based on top of the other series I just sent
> >> (clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
> >> elsewhere.
> > 
> > I'm generally ok with this, it makes sense to move it to sunxi-ng,
> > especially with that other series of yours.
> > 
> > My main concern about this is the split driver approach. We used to have
> > that before in the RTC too, but it was mostly due to the early clock
> > requirements. With your previous work, that requirement is not there
> > anymore and we can just register it as a device just like the other
> > clock providers.
> 
> That's a good point. Originally, I had this RTC CCU providing osc24M, so
> it did need to be an early provider. But with the current version, we
> could have the RTC platform driver call devm_sunxi_ccu_probe. That does
> seem cleaner.
> 
> (Since it wasn't immediately obvious to me why this works: the only
> early provider remaining is the sun5i CCU, and it doesn't use the sun6i
> RTC driver.)
> 
> > And since we can register all those clocks at device probe time, we
> > don't really need to split the driver in two (and especially in two
> > different places). The only obstacle to this after your previous series
> > is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
> > functions public, but that can easily be fixed by moving their
> > definition to include/linux/clk/sunxi-ng.h
> 
> Where are you thinking the clock definitions would go? We don't export
> any of those structures (ccu_mux, ccu_common) or macros
> (SUNXI_CCU_GATE_DATA) in a public header either.

Ah, right...

> Would you want to export those? That seems like a lot of churn. Or would
> we put the CCU descriptions in drivers/clk/sunxi-ng and export a
> function that the RTC driver can call? (Or some other idea?)

I guess we could export it. There's some fairly big headers in
include/linux/clk already (tegra and ti), it's not uAPI and we do have
reasons to do so, so I guess it's fine.

I'd like to avoid having two drivers for the same device if possible,
especially in two separate places. This creates some confusion since the
general expectation is that there's only one driver per device. There's
also the fact that this could lead to subtle bugs since the probe order
is the link order (or module loading).

And synchronizing access to registers between those two drivers will be
hard, while we could just share the same spin lock between the RTC and
clock drivers if they are instanciated in the same place.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

[Samuel Holland]

On 9/9/21 3:45 AM, Maxime Ripard wrote:
> On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
>> On 9/3/21 9:50 AM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
>>>> This patch series adds a CCU driver for the RTC in the H616 and R329.
>>>> The extra patches at the end of this series show how it would be
>>>> explanded to additional hardware variants.
>>>>
>>>> The driver is intended to support the existing binding used for the H6,
>>>> but also an updated binding which includes all RTC input clocks. I do
>>>> not know how to best represent that binding -- that is a major reason
>>>> why this series is an RFC.
>>>>
>>>> A future patch series could add functionality to the driver to manage
>>>> IOSC calibration at boot and during suspend/resume.
>>>>
>>>> It may be possible to support all of these hardware variants in the
>>>> existing RTC clock driver and avoid some duplicate code, but I'm
>>>> concerned about the complexity there, without any of the CCU
>>>> abstraction.
>>>>
>>>> This series is currently based on top of the other series I just sent
>>>> (clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
>>>> elsewhere.
>>>
>>> I'm generally ok with this, it makes sense to move it to sunxi-ng,
>>> especially with that other series of yours.
>>>
>>> My main concern about this is the split driver approach. We used to have
>>> that before in the RTC too, but it was mostly due to the early clock
>>> requirements. With your previous work, that requirement is not there
>>> anymore and we can just register it as a device just like the other
>>> clock providers.
>>
>> That's a good point. Originally, I had this RTC CCU providing osc24M, so
>> it did need to be an early provider. But with the current version, we
>> could have the RTC platform driver call devm_sunxi_ccu_probe. That does
>> seem cleaner.
>>
>> (Since it wasn't immediately obvious to me why this works: the only
>> early provider remaining is the sun5i CCU, and it doesn't use the sun6i
>> RTC driver.)
>>
>>> And since we can register all those clocks at device probe time, we
>>> don't really need to split the driver in two (and especially in two
>>> different places). The only obstacle to this after your previous series
>>> is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
>>> functions public, but that can easily be fixed by moving their
>>> definition to include/linux/clk/sunxi-ng.h
>>
>> Where are you thinking the clock definitions would go? We don't export
>> any of those structures (ccu_mux, ccu_common) or macros
>> (SUNXI_CCU_GATE_DATA) in a public header either.
> 
> Ah, right...
> 
>> Would you want to export those? That seems like a lot of churn. Or would
>> we put the CCU descriptions in drivers/clk/sunxi-ng and export a
>> function that the RTC driver can call? (Or some other idea?)
> 
> I guess we could export it. There's some fairly big headers in
> include/linux/clk already (tegra and ti), it's not uAPI and we do have
> reasons to do so, so I guess it's fine.
> 
> I'd like to avoid having two drivers for the same device if possible,
> especially in two separate places. This creates some confusion since the
> general expectation is that there's only one driver per device. There's
> also the fact that this could lead to subtle bugs since the probe order
> is the link order (or module loading).

I don't think there can be two "struct device"s for a single OF node. So
if the CCU part is in drivers/clk/sunxi-ng, the CCU "probe" function
would have to be called from the RTC driver. Since there has to be
cooperation anyway, I don't think there would be any ordering problems.

> And synchronizing access to registers between those two drivers will be
> hard, while we could just share the same spin lock between the RTC and
> clock drivers if they are instanciated in the same place.

While the RTC driver currently shares a spinlock between the clock part
and the RTC part, there isn't actually any overlap in register usage
between the two. So there doesn't need to be any synchronization.

Regards,
Samuel

Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

[Maxime Ripard]

Hi,

On Tue, Sep 28, 2021 at 02:46:39AM -0500, Samuel Holland wrote:
> On 9/9/21 3:45 AM, Maxime Ripard wrote:
> > On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
> >> On 9/3/21 9:50 AM, Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote:
> >>>> This patch series adds a CCU driver for the RTC in the H616 and R329.
> >>>> The extra patches at the end of this series show how it would be
> >>>> explanded to additional hardware variants.
> >>>>
> >>>> The driver is intended to support the existing binding used for the H6,
> >>>> but also an updated binding which includes all RTC input clocks. I do
> >>>> not know how to best represent that binding -- that is a major reason
> >>>> why this series is an RFC.
> >>>>
> >>>> A future patch series could add functionality to the driver to manage
> >>>> IOSC calibration at boot and during suspend/resume.
> >>>>
> >>>> It may be possible to support all of these hardware variants in the
> >>>> existing RTC clock driver and avoid some duplicate code, but I'm
> >>>> concerned about the complexity there, without any of the CCU
> >>>> abstraction.
> >>>>
> >>>> This series is currently based on top of the other series I just sent
> >>>> (clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it
> >>>> elsewhere.
> >>>
> >>> I'm generally ok with this, it makes sense to move it to sunxi-ng,
> >>> especially with that other series of yours.
> >>>
> >>> My main concern about this is the split driver approach. We used to have
> >>> that before in the RTC too, but it was mostly due to the early clock
> >>> requirements. With your previous work, that requirement is not there
> >>> anymore and we can just register it as a device just like the other
> >>> clock providers.
> >>
> >> That's a good point. Originally, I had this RTC CCU providing osc24M, so
> >> it did need to be an early provider. But with the current version, we
> >> could have the RTC platform driver call devm_sunxi_ccu_probe. That does
> >> seem cleaner.
> >>
> >> (Since it wasn't immediately obvious to me why this works: the only
> >> early provider remaining is the sun5i CCU, and it doesn't use the sun6i
> >> RTC driver.)
> >>
> >>> And since we can register all those clocks at device probe time, we
> >>> don't really need to split the driver in two (and especially in two
> >>> different places). The only obstacle to this after your previous series
> >>> is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
> >>> functions public, but that can easily be fixed by moving their
> >>> definition to include/linux/clk/sunxi-ng.h
> >>
> >> Where are you thinking the clock definitions would go? We don't export
> >> any of those structures (ccu_mux, ccu_common) or macros
> >> (SUNXI_CCU_GATE_DATA) in a public header either.
> > 
> > Ah, right...
> > 
> >> Would you want to export those? That seems like a lot of churn. Or would
> >> we put the CCU descriptions in drivers/clk/sunxi-ng and export a
> >> function that the RTC driver can call? (Or some other idea?)
> > 
> > I guess we could export it. There's some fairly big headers in
> > include/linux/clk already (tegra and ti), it's not uAPI and we do have
> > reasons to do so, so I guess it's fine.
> > 
> > I'd like to avoid having two drivers for the same device if possible,
> > especially in two separate places. This creates some confusion since the
> > general expectation is that there's only one driver per device. There's
> > also the fact that this could lead to subtle bugs since the probe order
> > is the link order (or module loading).
> 
> I don't think there can be two "struct device"s for a single OF node.

That's not what I meant, there's indeed a single of_node for a single
struct device. If we dig a bit into the core framework, the most likely
scenario is that we would register both the RTC and clock driver at
module_init, and with the device already created with its of_node set
during the initial DT parsing.

We register our platform driver using module_platform_driver, which
expands to calling driver_register() at module_init(), setting the
driver bus to the platform_bus in the process (in
__platform_driver_register()).

After some sanity check, driver_register() calls bus_add_driver(), which
will call driver_attach() if drivers_autoprobe is set (which is the
default, set into bus_register()).

driver_attach() will, for each device on the platform bus, call
__driver_attach(). If there's a match between that device and our driver
(which is evaluated by platform_match() in our case), we'll call our
driver probe with that device through driver_probe_device(),
__driver_probe_device() and finally really_probe().

However, at no point in time there's any check about whether that device
has already been bound to a driver, nor does it create a new device for
each driver. So this means that, if you have two drivers that match the
same device (like our clock and RTC drivers), you'll have both probe
being called with the same device, and the probe order will be defined
by the link order. Worse, they would share the same driver_data, with
each driver not being aware of the other. This is incredibly fragile,
and hard to notice since it goes against the usual expectations.

> So if the CCU part is in drivers/clk/sunxi-ng, the CCU "probe"
> function would have to be called from the RTC driver.

No, it would be called by the core directly if there's a compatible to
match.

> Since there has to be cooperation anyway, I don't think there would be
> any ordering problems.

My initial point was that, with a direct function call, it's both
deterministic and obvious.

> > And synchronizing access to registers between those two drivers will be
> > hard, while we could just share the same spin lock between the RTC and
> > clock drivers if they are instanciated in the same place.
> 
> While the RTC driver currently shares a spinlock between the clock part
> and the RTC part, there isn't actually any overlap in register usage
> between the two. So there doesn't need to be any synchronization.

I know, but this was more of a social problem than a technical one. Each
contributor and reviewer in the future will have to know or remember
that it's there, and make sure that it's still the case after any change
they make or review.

This is again a fairly fragile assumption.

Maxime

Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

[Samuel Holland]

Hi Maxime,

Thanks for your reply.

On 9/28/21 4:06 AM, Maxime Ripard wrote:
> On Tue, Sep 28, 2021 at 02:46:39AM -0500, Samuel Holland wrote:
>> On 9/9/21 3:45 AM, Maxime Ripard wrote:
>>> On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
>>>> On 9/3/21 9:50 AM, Maxime Ripard wrote:
>>>>> And since we can register all those clocks at device probe time, we
>>>>> don't really need to split the driver in two (and especially in two
>>>>> different places). The only obstacle to this after your previous series
>>>>> is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
>>>>> functions public, but that can easily be fixed by moving their
>>>>> definition to include/linux/clk/sunxi-ng.h
>>>>
>>>> Where are you thinking the clock definitions would go? We don't export
>>>> any of those structures (ccu_mux, ccu_common) or macros
>>>> (SUNXI_CCU_GATE_DATA) in a public header either.
>>>
>>> Ah, right...
>>>
>>>> Would you want to export those? That seems like a lot of churn. Or would
>>>> we put the CCU descriptions in drivers/clk/sunxi-ng and export a
>>>> function that the RTC driver can call? (Or some other idea?)
>>>
>>> I guess we could export it. There's some fairly big headers in
>>> include/linux/clk already (tegra and ti), it's not uAPI and we do have
>>> reasons to do so, so I guess it's fine.
>>>
>>> I'd like to avoid having two drivers for the same device if possible,
>>> especially in two separate places. This creates some confusion since the
>>> general expectation is that there's only one driver per device. There's
>>> also the fact that this could lead to subtle bugs since the probe order
>>> is the link order (or module loading).
>>
>> I don't think there can be two "struct device"s for a single OF node.
> 
> That's not what I meant, there's indeed a single of_node for a single
> struct device. If we dig a bit into the core framework, the most likely
> scenario is that we would register both the RTC and clock driver at
> module_init, and with the device already created with its of_node set
> during the initial DT parsing.
> 
> We register our platform driver using module_platform_driver, which
> expands to calling driver_register() at module_init(), setting the
> driver bus to the platform_bus in the process (in
> __platform_driver_register()).
> 
> After some sanity check, driver_register() calls bus_add_driver(), which
> will call driver_attach() if drivers_autoprobe is set (which is the
> default, set into bus_register()).
> 
> driver_attach() will, for each device on the platform bus, call
> __driver_attach(). If there's a match between that device and our driver
> (which is evaluated by platform_match() in our case), we'll call our
> driver probe with that device through driver_probe_device(),
> __driver_probe_device() and finally really_probe().
> 
> However, at no point in time there's any check about whether that device
> has already been bound to a driver, nor does it create a new device for
> each driver.

I would expect this to hit the:

	if (dev->driver)
		return -EBUSY;

in __driver_probe_device(), or fail the "if (!dev->driver)" check in
__driver_attach() for the async case, once the first driver is bound.

> So this means that, if you have two drivers that match the
> same device (like our clock and RTC drivers), you'll have both probe
> being called with the same device, and the probe order will be defined
> by the link order. Worse, they would share the same driver_data, with
> each driver not being aware of the other. This is incredibly fragile,
> and hard to notice since it goes against the usual expectations.
> 
>> So if the CCU part is in drivers/clk/sunxi-ng, the CCU "probe"
>> function would have to be called from the RTC driver.
> 
> No, it would be called by the core directly if there's a compatible to
> match.
> 
>> Since there has to be cooperation anyway, I don't think there would be
>> any ordering problems.
> 
> My initial point was that, with a direct function call, it's both
> deterministic and obvious.

I believe I did what you are suggesting for v2. From patch 7:

--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -683,6 +684,10 @@ static int sun6i_rtc_probe(struct platform_device
*pdev)
 		chip->base = devm_platform_ioremap_resource(pdev, 0);
 		if (IS_ERR(chip->base))
 			return PTR_ERR(chip->base);
+
+		ret = sun6i_rtc_ccu_probe(&pdev->dev, chip->base);
+		if (ret)
+			return ret;
 	}

 	platform_set_drvdata(pdev, chip);

>>> And synchronizing access to registers between those two drivers will be
>>> hard, while we could just share the same spin lock between the RTC and
>>> clock drivers if they are instanciated in the same place.
>>
>> While the RTC driver currently shares a spinlock between the clock part
>> and the RTC part, there isn't actually any overlap in register usage
>> between the two. So there doesn't need to be any synchronization.
> 
> I know, but this was more of a social problem than a technical one. Each
> contributor and reviewer in the future will have to know or remember
> that it's there, and make sure that it's still the case after any change
> they make or review.
> 
> This is again a fairly fragile assumption.

Yeah, I agree that having a lock that is only sometimes safe to use with
certain registers is quite fragile.

Would splitting the spinlock in rtc-sun6i.c into "losc_lock" (for the
clock provider) and "alarm_lock" (for the RTC driver) make this
distinction clear enough?

Eventually, I want to split up the struct between the clock provider and
RTC driver so it's clear which members belong to whom, and there's no
ugly global pointer use. Maybe I should do this first?

Regards,
Samuel

Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6496 bytes --]

On Tue, Sep 28, 2021 at 10:54:26PM -0500, Samuel Holland wrote:
> Hi Maxime,
> 
> Thanks for your reply.
> 
> On 9/28/21 4:06 AM, Maxime Ripard wrote:
> > On Tue, Sep 28, 2021 at 02:46:39AM -0500, Samuel Holland wrote:
> >> On 9/9/21 3:45 AM, Maxime Ripard wrote:
> >>> On Fri, Sep 03, 2021 at 10:21:13AM -0500, Samuel Holland wrote:
> >>>> On 9/3/21 9:50 AM, Maxime Ripard wrote:
> >>>>> And since we can register all those clocks at device probe time, we
> >>>>> don't really need to split the driver in two (and especially in two
> >>>>> different places). The only obstacle to this after your previous series
> >>>>> is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe
> >>>>> functions public, but that can easily be fixed by moving their
> >>>>> definition to include/linux/clk/sunxi-ng.h
> >>>>
> >>>> Where are you thinking the clock definitions would go? We don't export
> >>>> any of those structures (ccu_mux, ccu_common) or macros
> >>>> (SUNXI_CCU_GATE_DATA) in a public header either.
> >>>
> >>> Ah, right...
> >>>
> >>>> Would you want to export those? That seems like a lot of churn. Or would
> >>>> we put the CCU descriptions in drivers/clk/sunxi-ng and export a
> >>>> function that the RTC driver can call? (Or some other idea?)
> >>>
> >>> I guess we could export it. There's some fairly big headers in
> >>> include/linux/clk already (tegra and ti), it's not uAPI and we do have
> >>> reasons to do so, so I guess it's fine.
> >>>
> >>> I'd like to avoid having two drivers for the same device if possible,
> >>> especially in two separate places. This creates some confusion since the
> >>> general expectation is that there's only one driver per device. There's
> >>> also the fact that this could lead to subtle bugs since the probe order
> >>> is the link order (or module loading).
> >>
> >> I don't think there can be two "struct device"s for a single OF node.
> > 
> > That's not what I meant, there's indeed a single of_node for a single
> > struct device. If we dig a bit into the core framework, the most likely
> > scenario is that we would register both the RTC and clock driver at
> > module_init, and with the device already created with its of_node set
> > during the initial DT parsing.
> > 
> > We register our platform driver using module_platform_driver, which
> > expands to calling driver_register() at module_init(), setting the
> > driver bus to the platform_bus in the process (in
> > __platform_driver_register()).
> > 
> > After some sanity check, driver_register() calls bus_add_driver(), which
> > will call driver_attach() if drivers_autoprobe is set (which is the
> > default, set into bus_register()).
> > 
> > driver_attach() will, for each device on the platform bus, call
> > __driver_attach(). If there's a match between that device and our driver
> > (which is evaluated by platform_match() in our case), we'll call our
> > driver probe with that device through driver_probe_device(),
> > __driver_probe_device() and finally really_probe().
> > 
> > However, at no point in time there's any check about whether that device
> > has already been bound to a driver, nor does it create a new device for
> > each driver.
> 
> I would expect this to hit the:
> 
> 	if (dev->driver)
> 		return -EBUSY;
> 
> in __driver_probe_device(), or fail the "if (!dev->driver)" check in
> __driver_attach() for the async case, once the first driver is bound.

Hmmm, it might. I know we "leveraged" this some time ago for another
platform, but it might not be working anymore indeed.

> > So this means that, if you have two drivers that match the
> > same device (like our clock and RTC drivers), you'll have both probe
> > being called with the same device, and the probe order will be defined
> > by the link order. Worse, they would share the same driver_data, with
> > each driver not being aware of the other. This is incredibly fragile,
> > and hard to notice since it goes against the usual expectations.
> > 
> >> So if the CCU part is in drivers/clk/sunxi-ng, the CCU "probe"
> >> function would have to be called from the RTC driver.
> > 
> > No, it would be called by the core directly if there's a compatible to
> > match.
> > 
> >> Since there has to be cooperation anyway, I don't think there would be
> >> any ordering problems.
> > 
> > My initial point was that, with a direct function call, it's both
> > deterministic and obvious.
> 
> I believe I did what you are suggesting for v2. From patch 7:
> 
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -683,6 +684,10 @@ static int sun6i_rtc_probe(struct platform_device
> *pdev)
>  		chip->base = devm_platform_ioremap_resource(pdev, 0);
>  		if (IS_ERR(chip->base))
>  			return PTR_ERR(chip->base);
> +
> +		ret = sun6i_rtc_ccu_probe(&pdev->dev, chip->base);
> +		if (ret)
> +			return ret;
>  	}

Ah, sorry, I entirely missed it. Yes, that totally fine by me then. I'd
prefer to have the spinlock passed as an argument as well, but it can be
done in a follow-up patch.

>  	platform_set_drvdata(pdev, chip);
> 
> >>> And synchronizing access to registers between those two drivers will be
> >>> hard, while we could just share the same spin lock between the RTC and
> >>> clock drivers if they are instanciated in the same place.
> >>
> >> While the RTC driver currently shares a spinlock between the clock part
> >> and the RTC part, there isn't actually any overlap in register usage
> >> between the two. So there doesn't need to be any synchronization.
> > 
> > I know, but this was more of a social problem than a technical one. Each
> > contributor and reviewer in the future will have to know or remember
> > that it's there, and make sure that it's still the case after any change
> > they make or review.
> > 
> > This is again a fairly fragile assumption.
> 
> Yeah, I agree that having a lock that is only sometimes safe to use with
> certain registers is quite fragile.
> 
> Would splitting the spinlock in rtc-sun6i.c into "losc_lock" (for the
> clock provider) and "alarm_lock" (for the RTC driver) make this
> distinction clear enough?
> 
> Eventually, I want to split up the struct between the clock provider and
> RTC driver so it's clear which members belong to whom, and there's no
> ugly global pointer use. Maybe I should do this first?

Yeah, it sounds like a good plan

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]