linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC
@ 2019-08-20 15:19 megous
  2019-08-20 15:19 ` [PATCH v2 1/3] dt-bindings: Add compatible for H6 RTC megous
                   ` (3 more replies)
  0 siblings, 4 replies; 21+ messages in thread
From: megous @ 2019-08-20 15:19 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring, Mark Rutland,
	Maxime Ripard, Chen-Yu Tsai
  Cc: Ondrej Jirman, linux-rtc, devicetree, linux-arm-kernel,
	linux-kernel, linux-sunxi

From: Ondrej Jirman <megous@megous.com>

I went through the datasheets for H6 and H5, and compared the differences.
RTCs are largely similar, but not entirely compatible. Incompatibilities
are in details not yet implemented by the rtc driver though.

I also corrected the clock tree in H6 DTSI.

This patchset is necessary for implementing the WiFi/Bluetooth support
on boards using H6 SoC.

There was some discussion previously of describing HOSC, DCXO and XO
oscillators and clocks as part of RTC in DT, but I decided against it
because it's not necessary, becuse information that would be provided
as a part of DT can already be determined at runtime from RTC registers,
so this woudn't add any value and would only introduce complications
to the driver. See: https://patchwork.kernel.org/cover/10898083/

Please take a look.


Thank you and regards,
  Ondrej Jirman


Changes in v2:
- bindings converted to yaml
- added reviewed by tags

Ondrej Jirman (3):
  dt-bindings: Add compatible for H6 RTC
  rtc: sun6i: Add support for H6 RTC
  arm64: dts: sun50i-h6: Add support for RTC and fix the clock tree

 .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml | 13 ++++++
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  | 30 +++++++-------
 drivers/rtc/rtc-sun6i.c                       | 40 ++++++++++++++++++-
 3 files changed, 67 insertions(+), 16 deletions(-)

-- 
2.22.1


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

* [PATCH v2 1/3] dt-bindings: Add compatible for H6 RTC
  2019-08-20 15:19 [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC megous
@ 2019-08-20 15:19 ` megous
  2019-08-22 21:15   ` Alexandre Belloni
  2019-08-20 15:19 ` [PATCH v2 2/3] rtc: sun6i: Add support " megous
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 21+ messages in thread
From: megous @ 2019-08-20 15:19 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring, Mark Rutland,
	Maxime Ripard, Chen-Yu Tsai
  Cc: Ondrej Jirman, linux-rtc, devicetree, linux-arm-kernel,
	linux-kernel, linux-sunxi

From: Ondrej Jirman <megous@megous.com>

RTC on H6 is similar to the one on H5 SoC, but incompatible in small
details. See the driver for description of differences. For example
H6 RTC needs to enable the external low speed oscillator. Add new
compatible for this RTC.

Signed-off-by: Ondrej Jirman <megous@megous.com>
---
 .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml       | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
index 924622f39c44..d7a57ec4a640 100644
--- a/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/allwinner,sun6i-a31-rtc.yaml
@@ -25,6 +25,7 @@ properties:
       - items:
           - const: allwinner,sun50i-a64-rtc
           - const: allwinner,sun8i-h3-rtc
+      - const: allwinner,sun50i-h6-rtc
 
   reg:
     maxItems: 1
@@ -92,6 +93,18 @@ allOf:
           minItems: 3
           maxItems: 3
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: allwinner,sun50i-h6-rtc
+
+    then:
+      properties:
+        clock-output-names:
+          minItems: 3
+          maxItems: 3
+
   - if:
       properties:
         compatible:
-- 
2.22.1


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

* [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-20 15:19 [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC megous
  2019-08-20 15:19 ` [PATCH v2 1/3] dt-bindings: Add compatible for H6 RTC megous
@ 2019-08-20 15:19 ` megous
  2019-08-22 21:16   ` Alexandre Belloni
  2019-08-24 12:32   ` [linux-sunxi] " Jernej Škrabec
  2019-08-20 15:19 ` [PATCH v2 3/3] arm64: dts: sun50i-h6: Add support for RTC and fix the clock tree megous
  2019-08-24  8:04 ` [linux-sunxi] [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC Jernej Škrabec
  3 siblings, 2 replies; 21+ messages in thread
From: megous @ 2019-08-20 15:19 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring, Mark Rutland,
	Maxime Ripard, Chen-Yu Tsai
  Cc: Ondrej Jirman, linux-rtc, devicetree, linux-arm-kernel,
	linux-kernel, linux-sunxi

From: Ondrej Jirman <megous@megous.com>

RTC on H6 is mostly the same as on H5 and H3. It has slight differences
mostly in features that are not yet supported by this driver.

Some differences are already stated in the comments in existing code.
One other difference is that H6 has extra bit in LOSC_CTRL_REG, called
EXT_LOSC_EN to enable/disable external low speed crystal oscillator.

It also has bit EXT_LOSC_STA in LOSC_AUTO_SWT_STA_REG, to check whether
external low speed oscillator is working correctly.

This patch adds support for enabling LOSC when necessary:

- during reparenting
- when probing the clock

H6 also has capacbility to automatically reparent RTC clock from
external crystal oscillator, to internal RC oscillator, if external
oscillator fails. This is enabled by default. Disable it during
probe.

Signed-off-by: Ondrej Jirman <megous@megous.com>
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/rtc/rtc-sun6i.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index d50ee023b559..b0c3752bed3f 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -32,9 +32,11 @@
 /* Control register */
 #define SUN6I_LOSC_CTRL				0x0000
 #define SUN6I_LOSC_CTRL_KEY			(0x16aa << 16)
+#define SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS		BIT(15)
 #define SUN6I_LOSC_CTRL_ALM_DHMS_ACC		BIT(9)
 #define SUN6I_LOSC_CTRL_RTC_HMS_ACC		BIT(8)
 #define SUN6I_LOSC_CTRL_RTC_YMD_ACC		BIT(7)
+#define SUN6I_LOSC_CTRL_EXT_LOSC_EN		BIT(4)
 #define SUN6I_LOSC_CTRL_EXT_OSC			BIT(0)
 #define SUN6I_LOSC_CTRL_ACC_MASK		GENMASK(9, 7)
 
@@ -128,6 +130,8 @@ struct sun6i_rtc_clk_data {
 	unsigned int has_prescaler : 1;
 	unsigned int has_out_clk : 1;
 	unsigned int export_iosc : 1;
+	unsigned int has_losc_en : 1;
+	unsigned int has_auto_swt : 1;
 };
 
 struct sun6i_rtc_dev {
@@ -190,6 +194,10 @@ static int sun6i_rtc_osc_set_parent(struct clk_hw *hw, u8 index)
 	val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
 	val |= SUN6I_LOSC_CTRL_KEY;
 	val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
+	if (rtc->data->has_losc_en) {
+		val &= ~SUN6I_LOSC_CTRL_EXT_LOSC_EN;
+		val |= index ? SUN6I_LOSC_CTRL_EXT_LOSC_EN : 0;
+	}
 	writel(val, rtc->base + SUN6I_LOSC_CTRL);
 	spin_unlock_irqrestore(&rtc->lock, flags);
 
@@ -215,6 +223,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node,
 	const char *iosc_name = "rtc-int-osc";
 	const char *clkout_name = "osc32k-out";
 	const char *parents[2];
+	u32 reg;
 
 	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
 	if (!rtc)
@@ -235,9 +244,18 @@ static void __init sun6i_rtc_clk_init(struct device_node *node,
 		goto err;
 	}
 
+	reg = SUN6I_LOSC_CTRL_KEY;
+	if (rtc->data->has_auto_swt) {
+		/* Bypass auto-switch to int osc, on ext losc failure */
+		reg |= SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS;
+		writel(reg, rtc->base + SUN6I_LOSC_CTRL);
+	}
+
 	/* Switch to the external, more precise, oscillator */
-	writel(SUN6I_LOSC_CTRL_KEY | SUN6I_LOSC_CTRL_EXT_OSC,
-	       rtc->base + SUN6I_LOSC_CTRL);
+	reg |= SUN6I_LOSC_CTRL_EXT_OSC;
+	if (rtc->data->has_losc_en)
+		reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN;
+	writel(reg, rtc->base + SUN6I_LOSC_CTRL);
 
 	/* Yes, I know, this is ugly. */
 	sun6i_rtc = rtc;
@@ -345,6 +363,23 @@ 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);
+
 static const struct sun6i_rtc_clk_data sun8i_v3_rtc_data = {
 	.rc_osc_rate = 32000,
 	.has_out_clk = 1,
@@ -675,6 +710,7 @@ static const struct of_device_id sun6i_rtc_dt_ids[] = {
 	{ .compatible = "allwinner,sun8i-r40-rtc" },
 	{ .compatible = "allwinner,sun8i-v3-rtc" },
 	{ .compatible = "allwinner,sun50i-h5-rtc" },
+	{ .compatible = "allwinner,sun50i-h6-rtc" },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
-- 
2.22.1


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

* [PATCH v2 3/3] arm64: dts: sun50i-h6: Add support for RTC and fix the clock tree
  2019-08-20 15:19 [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC megous
  2019-08-20 15:19 ` [PATCH v2 1/3] dt-bindings: Add compatible for H6 RTC megous
  2019-08-20 15:19 ` [PATCH v2 2/3] rtc: sun6i: Add support " megous
@ 2019-08-20 15:19 ` megous
  2019-08-23  8:19   ` Maxime Ripard
  2019-08-24  8:04 ` [linux-sunxi] [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC Jernej Škrabec
  3 siblings, 1 reply; 21+ messages in thread
From: megous @ 2019-08-20 15:19 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, Rob Herring, Mark Rutland,
	Maxime Ripard, Chen-Yu Tsai
  Cc: Ondrej Jirman, linux-rtc, devicetree, linux-arm-kernel,
	linux-kernel, linux-sunxi

From: Ondrej Jirman <megous@megous.com>

This patch adds RTC node and fixes the clock properties and nodes
to reflect the real clock tree.

The device nodes for the internal oscillator and osc32k are removed,
as these clocks are now provided by the RTC device. Clock references
are fixed accordingly, too.

Signed-off-by: Ondrej Jirman <megous@megous.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 30 +++++++++++---------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 67b732e34091..67f920e0fc33 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -56,14 +56,6 @@
 		status = "disabled";
 	};
 
-	iosc: internal-osc-clk {
-		#clock-cells = <0>;
-		compatible = "fixed-clock";
-		clock-frequency = <16000000>;
-		clock-accuracy = <300000000>;
-		clock-output-names = "iosc";
-	};
-
 	osc24M: osc24M_clk {
 		#clock-cells = <0>;
 		compatible = "fixed-clock";
@@ -71,11 +63,11 @@
 		clock-output-names = "osc24M";
 	};
 
-	osc32k: osc32k_clk {
+	ext_osc32k: ext_osc32k_clk {
 		#clock-cells = <0>;
 		compatible = "fixed-clock";
 		clock-frequency = <32768>;
-		clock-output-names = "osc32k";
+		clock-output-names = "ext_osc32k";
 	};
 
 	psci {
@@ -197,7 +189,7 @@
 		ccu: clock@3001000 {
 			compatible = "allwinner,sun50i-h6-ccu";
 			reg = <0x03001000 0x1000>;
-			clocks = <&osc24M>, <&osc32k>, <&iosc>;
+			clocks = <&osc24M>, <&rtc 0>, <&rtc 2>;
 			clock-names = "hosc", "losc", "iosc";
 			#clock-cells = <1>;
 			#reset-cells = <1>;
@@ -236,7 +228,7 @@
 				     <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&ccu CLK_APB1>, <&osc24M>, <&osc32k>;
+			clocks = <&ccu CLK_APB1>, <&osc24M>, <&rtc 0>;
 			clock-names = "apb", "hosc", "losc";
 			gpio-controller;
 			#gpio-cells = <3>;
@@ -710,10 +702,20 @@
 			};
 		};
 
+		rtc: rtc@7000000 {
+			compatible = "allwinner,sun50i-h6-rtc";
+			reg = <0x07000000 0x400>;
+			interrupts = <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
+			clock-output-names = "osc32k", "osc32k-out", "iosc";
+			clocks = <&ext_osc32k>;
+			#clock-cells = <1>;
+		};
+
 		r_ccu: clock@7010000 {
 			compatible = "allwinner,sun50i-h6-r-ccu";
 			reg = <0x07010000 0x400>;
-			clocks = <&osc24M>, <&osc32k>, <&iosc>,
+			clocks = <&osc24M>, <&rtc 0>, <&rtc 2>,
 				 <&ccu CLK_PLL_PERIPH0>;
 			clock-names = "hosc", "losc", "iosc", "pll-periph";
 			#clock-cells = <1>;
@@ -741,7 +743,7 @@
 			reg = <0x07022000 0x400>;
 			interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
-			clocks = <&r_ccu CLK_R_APB1>, <&osc24M>, <&osc32k>;
+			clocks = <&r_ccu CLK_R_APB1>, <&osc24M>, <&rtc 0>;
 			clock-names = "apb", "hosc", "losc";
 			gpio-controller;
 			#gpio-cells = <3>;
-- 
2.22.1


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

* Re: [PATCH v2 1/3] dt-bindings: Add compatible for H6 RTC
  2019-08-20 15:19 ` [PATCH v2 1/3] dt-bindings: Add compatible for H6 RTC megous
@ 2019-08-22 21:15   ` Alexandre Belloni
  0 siblings, 0 replies; 21+ messages in thread
From: Alexandre Belloni @ 2019-08-22 21:15 UTC (permalink / raw)
  To: megous
  Cc: Alessandro Zummo, Rob Herring, Mark Rutland, Maxime Ripard,
	Chen-Yu Tsai, linux-rtc, devicetree, linux-arm-kernel,
	linux-kernel, linux-sunxi

On 20/08/2019 17:19:32+0200, megous@megous.com wrote:
> From: Ondrej Jirman <megous@megous.com>
> 
> RTC on H6 is similar to the one on H5 SoC, but incompatible in small
> details. See the driver for description of differences. For example
> H6 RTC needs to enable the external low speed oscillator. Add new
> compatible for this RTC.
> 
> Signed-off-by: Ondrej Jirman <megous@megous.com>
> ---
>  .../bindings/rtc/allwinner,sun6i-a31-rtc.yaml       | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

* Re: [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-20 15:19 ` [PATCH v2 2/3] rtc: sun6i: Add support " megous
@ 2019-08-22 21:16   ` Alexandre Belloni
  2019-08-24 12:32   ` [linux-sunxi] " Jernej Škrabec
  1 sibling, 0 replies; 21+ messages in thread
From: Alexandre Belloni @ 2019-08-22 21:16 UTC (permalink / raw)
  To: megous
  Cc: Alessandro Zummo, Rob Herring, Mark Rutland, Maxime Ripard,
	Chen-Yu Tsai, linux-rtc, devicetree, linux-arm-kernel,
	linux-kernel, linux-sunxi

On 20/08/2019 17:19:33+0200, megous@megous.com wrote:
> From: Ondrej Jirman <megous@megous.com>
> 
> RTC on H6 is mostly the same as on H5 and H3. It has slight differences
> mostly in features that are not yet supported by this driver.
> 
> Some differences are already stated in the comments in existing code.
> One other difference is that H6 has extra bit in LOSC_CTRL_REG, called
> EXT_LOSC_EN to enable/disable external low speed crystal oscillator.
> 
> It also has bit EXT_LOSC_STA in LOSC_AUTO_SWT_STA_REG, to check whether
> external low speed oscillator is working correctly.
> 
> This patch adds support for enabling LOSC when necessary:
> 
> - during reparenting
> - when probing the clock
> 
> H6 also has capacbility to automatically reparent RTC clock from
> external crystal oscillator, to internal RC oscillator, if external
> oscillator fails. This is enabled by default. Disable it during
> probe.
> 
> Signed-off-by: Ondrej Jirman <megous@megous.com>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/rtc/rtc-sun6i.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

* Re: [PATCH v2 3/3] arm64: dts: sun50i-h6: Add support for RTC and fix the clock tree
  2019-08-20 15:19 ` [PATCH v2 3/3] arm64: dts: sun50i-h6: Add support for RTC and fix the clock tree megous
@ 2019-08-23  8:19   ` Maxime Ripard
  0 siblings, 0 replies; 21+ messages in thread
From: Maxime Ripard @ 2019-08-23  8:19 UTC (permalink / raw)
  To: megous
  Cc: Alessandro Zummo, Alexandre Belloni, Rob Herring, Mark Rutland,
	Chen-Yu Tsai, linux-rtc, devicetree, linux-arm-kernel,
	linux-kernel, linux-sunxi

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

On Tue, Aug 20, 2019 at 05:19:34PM +0200, megous@megous.com wrote:
> From: Ondrej Jirman <megous@megous.com>
>
> This patch adds RTC node and fixes the clock properties and nodes
> to reflect the real clock tree.
>
> The device nodes for the internal oscillator and osc32k are removed,
> as these clocks are now provided by the RTC device. Clock references
> are fixed accordingly, too.
>
> Signed-off-by: Ondrej Jirman <megous@megous.com>

The prefix should be "arm64: dts: allwinner: h6:"

I've fixed it up and applied it.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

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

* Re: [linux-sunxi] [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC
  2019-08-20 15:19 [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC megous
                   ` (2 preceding siblings ...)
  2019-08-20 15:19 ` [PATCH v2 3/3] arm64: dts: sun50i-h6: Add support for RTC and fix the clock tree megous
@ 2019-08-24  8:04 ` Jernej Škrabec
  2019-08-24  8:06   ` Jernej Škrabec
  3 siblings, 1 reply; 21+ messages in thread
From: Jernej Škrabec @ 2019-08-24  8:04 UTC (permalink / raw)
  To: linux-sunxi, megous
  Cc: Alessandro Zummo, Alexandre Belloni, Rob Herring, Mark Rutland,
	Maxime Ripard, Chen-Yu Tsai, linux-rtc, devicetree,
	linux-arm-kernel, linux-kernel

Hi!

Dne torek, 20. avgust 2019 ob 17:19:31 CEST je megous@megous.com napisal(a):
> From: Ondrej Jirman <megous@megous.com>
> 
> I went through the datasheets for H6 and H5, and compared the differences.
> RTCs are largely similar, but not entirely compatible. Incompatibilities
> are in details not yet implemented by the rtc driver though.
> 
> I also corrected the clock tree in H6 DTSI.
> 
> This patchset is necessary for implementing the WiFi/Bluetooth support
> on boards using H6 SoC.
> 
> There was some discussion previously of describing HOSC, DCXO and XO
> oscillators and clocks as part of RTC in DT, but I decided against it
> because it's not necessary, becuse information that would be provided
> as a part of DT can already be determined at runtime from RTC registers,
> so this woudn't add any value and would only introduce complications
> to the driver. See: https://patchwork.kernel.org/cover/10898083/
> 
> Please take a look.
> 
> 
> Thank you and regards,
>   Ondrej Jirman

Sorry for a bit late test, but with your patches on Tanix TX6 box I get this 
in dmesg:

[   17.431742] sun6i-rtc 7000000.rtc: Failed to set rtc time.
[   20.439742] sun6i-rtc 7000000.rtc: rtc is still busy.
[   21.435744] sun6i-rtc 7000000.rtc: rtc is still busy.
[   24.055741] sun6i-rtc 7000000.rtc: rtc is still busy.
[   24.439752] sun6i-rtc 7000000.rtc: rtc is still busy.

Last line is repeated non-stop.

Any idea what could be wrong?

Best regards,
Jernej




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

* Re: [linux-sunxi] [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC
  2019-08-24  8:04 ` [linux-sunxi] [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC Jernej Škrabec
@ 2019-08-24  8:06   ` Jernej Škrabec
  2019-08-24 12:56     ` Ondřej Jirman
  0 siblings, 1 reply; 21+ messages in thread
From: Jernej Škrabec @ 2019-08-24  8:06 UTC (permalink / raw)
  To: linux-sunxi
  Cc: megous, Alessandro Zummo, Alexandre Belloni, Rob Herring,
	Mark Rutland, Maxime Ripard, Chen-Yu Tsai, linux-rtc, devicetree,
	linux-arm-kernel, linux-kernel

Dne sobota, 24. avgust 2019 ob 10:04:24 CEST je Jernej Škrabec napisal(a):
> Hi!
> 
> Dne torek, 20. avgust 2019 ob 17:19:31 CEST je megous@megous.com napisal(a):
> > From: Ondrej Jirman <megous@megous.com>
> > 
> > I went through the datasheets for H6 and H5, and compared the differences.
> > RTCs are largely similar, but not entirely compatible. Incompatibilities
> > are in details not yet implemented by the rtc driver though.
> > 
> > I also corrected the clock tree in H6 DTSI.
> > 
> > This patchset is necessary for implementing the WiFi/Bluetooth support
> > on boards using H6 SoC.
> > 
> > There was some discussion previously of describing HOSC, DCXO and XO
> > oscillators and clocks as part of RTC in DT, but I decided against it
> > because it's not necessary, becuse information that would be provided
> > as a part of DT can already be determined at runtime from RTC registers,
> > so this woudn't add any value and would only introduce complications
> > to the driver. See: https://patchwork.kernel.org/cover/10898083/
> > 
> > Please take a look.
> > 
> > 
> > Thank you and regards,
> > 
> >   Ondrej Jirman
> 
> Sorry for a bit late test, but with your patches on Tanix TX6 box I get this
> in dmesg:
> 
> [   17.431742] sun6i-rtc 7000000.rtc: Failed to set rtc time.
> [   20.439742] sun6i-rtc 7000000.rtc: rtc is still busy.
> [   21.435744] sun6i-rtc 7000000.rtc: rtc is still busy.
> [   24.055741] sun6i-rtc 7000000.rtc: rtc is still busy.
> [   24.439752] sun6i-rtc 7000000.rtc: rtc is still busy.
> 
> Last line is repeated non-stop.
> 
> Any idea what could be wrong?

Additional info - this is on kernel 5.2.6 with your patches applied.
 
Best regards,
Jernej





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

* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-20 15:19 ` [PATCH v2 2/3] rtc: sun6i: Add support " megous
  2019-08-22 21:16   ` Alexandre Belloni
@ 2019-08-24 12:32   ` Jernej Škrabec
  2019-08-24 12:46     ` Ondřej Jirman
  1 sibling, 1 reply; 21+ messages in thread
From: Jernej Škrabec @ 2019-08-24 12:32 UTC (permalink / raw)
  To: linux-sunxi, megous
  Cc: Alessandro Zummo, Alexandre Belloni, Rob Herring, Mark Rutland,
	Maxime Ripard, Chen-Yu Tsai, linux-rtc, devicetree,
	linux-arm-kernel, linux-kernel

Hi!

Dne torek, 20. avgust 2019 ob 17:19:33 CEST je megous@megous.com napisal(a):
> From: Ondrej Jirman <megous@megous.com>
> 
> RTC on H6 is mostly the same as on H5 and H3. It has slight differences
> mostly in features that are not yet supported by this driver.
> 
> Some differences are already stated in the comments in existing code.
> One other difference is that H6 has extra bit in LOSC_CTRL_REG, called
> EXT_LOSC_EN to enable/disable external low speed crystal oscillator.
> 
> It also has bit EXT_LOSC_STA in LOSC_AUTO_SWT_STA_REG, to check whether
> external low speed oscillator is working correctly.
> 
> This patch adds support for enabling LOSC when necessary:
> 
> - during reparenting
> - when probing the clock
> 
> H6 also has capacbility to automatically reparent RTC clock from
> external crystal oscillator, to internal RC oscillator, if external
> oscillator fails. This is enabled by default. Disable it during
> probe.
> 
> Signed-off-by: Ondrej Jirman <megous@megous.com>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/rtc/rtc-sun6i.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index d50ee023b559..b0c3752bed3f 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -32,9 +32,11 @@
>  /* Control register */
>  #define SUN6I_LOSC_CTRL				0x0000
>  #define SUN6I_LOSC_CTRL_KEY			(0x16aa << 16)
> +#define SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS		BIT(15)

User manual says that above field is bit 14.

Best regards,
Jernej

>  #define SUN6I_LOSC_CTRL_ALM_DHMS_ACC		BIT(9)
>  #define SUN6I_LOSC_CTRL_RTC_HMS_ACC		BIT(8)
>  #define SUN6I_LOSC_CTRL_RTC_YMD_ACC		BIT(7)
> +#define SUN6I_LOSC_CTRL_EXT_LOSC_EN		BIT(4)
>  #define SUN6I_LOSC_CTRL_EXT_OSC			BIT(0)
>  #define SUN6I_LOSC_CTRL_ACC_MASK		GENMASK(9, 7)
> 
> @@ -128,6 +130,8 @@ struct sun6i_rtc_clk_data {
>  	unsigned int has_prescaler : 1;
>  	unsigned int has_out_clk : 1;
>  	unsigned int export_iosc : 1;
> +	unsigned int has_losc_en : 1;
> +	unsigned int has_auto_swt : 1;
>  };
> 
>  struct sun6i_rtc_dev {
> @@ -190,6 +194,10 @@ static int sun6i_rtc_osc_set_parent(struct clk_hw *hw,
> u8 index) val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
>  	val |= SUN6I_LOSC_CTRL_KEY;
>  	val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
> +	if (rtc->data->has_losc_en) {
> +		val &= ~SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> +		val |= index ? SUN6I_LOSC_CTRL_EXT_LOSC_EN : 0;
> +	}
>  	writel(val, rtc->base + SUN6I_LOSC_CTRL);
>  	spin_unlock_irqrestore(&rtc->lock, flags);
> 
> @@ -215,6 +223,7 @@ static void __init sun6i_rtc_clk_init(struct device_node
> *node, const char *iosc_name = "rtc-int-osc";
>  	const char *clkout_name = "osc32k-out";
>  	const char *parents[2];
> +	u32 reg;
> 
>  	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
>  	if (!rtc)
> @@ -235,9 +244,18 @@ static void __init sun6i_rtc_clk_init(struct
> device_node *node, goto err;
>  	}
> 
> +	reg = SUN6I_LOSC_CTRL_KEY;
> +	if (rtc->data->has_auto_swt) {
> +		/* Bypass auto-switch to int osc, on ext losc failure */
> +		reg |= SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS;
> +		writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> +	}
> +
>  	/* Switch to the external, more precise, oscillator */
> -	writel(SUN6I_LOSC_CTRL_KEY | SUN6I_LOSC_CTRL_EXT_OSC,
> -	       rtc->base + SUN6I_LOSC_CTRL);
> +	reg |= SUN6I_LOSC_CTRL_EXT_OSC;
> +	if (rtc->data->has_losc_en)
> +		reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> +	writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> 
>  	/* Yes, I know, this is ugly. */
>  	sun6i_rtc = rtc;
> @@ -345,6 +363,23 @@ 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);
> +
>  static const struct sun6i_rtc_clk_data sun8i_v3_rtc_data = {
>  	.rc_osc_rate = 32000,
>  	.has_out_clk = 1,
> @@ -675,6 +710,7 @@ static const struct of_device_id sun6i_rtc_dt_ids[] = {
>  	{ .compatible = "allwinner,sun8i-r40-rtc" },
>  	{ .compatible = "allwinner,sun8i-v3-rtc" },
>  	{ .compatible = "allwinner,sun50i-h5-rtc" },
> +	{ .compatible = "allwinner,sun50i-h6-rtc" },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);





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

* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-24 12:32   ` [linux-sunxi] " Jernej Škrabec
@ 2019-08-24 12:46     ` Ondřej Jirman
  2019-08-24 12:51       ` Jernej Škrabec
  0 siblings, 1 reply; 21+ messages in thread
From: Ondřej Jirman @ 2019-08-24 12:46 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: linux-sunxi, Mark Rutland, Alessandro Zummo, Alexandre Belloni,
	devicetree, Maxime Ripard, linux-kernel, Chen-Yu Tsai,
	Rob Herring, linux-arm-kernel, linux-rtc

Hi,

On Sat, Aug 24, 2019 at 02:32:32PM +0200, Jernej Škrabec wrote:
> Hi!
> 
> Dne torek, 20. avgust 2019 ob 17:19:33 CEST je megous@megous.com napisal(a):
> > From: Ondrej Jirman <megous@megous.com>
> > 
> > RTC on H6 is mostly the same as on H5 and H3. It has slight differences
> > mostly in features that are not yet supported by this driver.
> > 
> > Some differences are already stated in the comments in existing code.
> > One other difference is that H6 has extra bit in LOSC_CTRL_REG, called
> > EXT_LOSC_EN to enable/disable external low speed crystal oscillator.
> > 
> > It also has bit EXT_LOSC_STA in LOSC_AUTO_SWT_STA_REG, to check whether
> > external low speed oscillator is working correctly.
> > 
> > This patch adds support for enabling LOSC when necessary:
> > 
> > - during reparenting
> > - when probing the clock
> > 
> > H6 also has capacbility to automatically reparent RTC clock from
> > external crystal oscillator, to internal RC oscillator, if external
> > oscillator fails. This is enabled by default. Disable it during
> > probe.
> > 
> > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > ---
> >  drivers/rtc/rtc-sun6i.c | 40 ++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 38 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > index d50ee023b559..b0c3752bed3f 100644
> > --- a/drivers/rtc/rtc-sun6i.c
> > +++ b/drivers/rtc/rtc-sun6i.c
> > @@ -32,9 +32,11 @@
> >  /* Control register */
> >  #define SUN6I_LOSC_CTRL				0x0000
> >  #define SUN6I_LOSC_CTRL_KEY			(0x16aa << 16)
> > +#define SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS		BIT(15)
> 
> User manual says that above field is bit 14.

See the previous discussion, this is from BSP.

regards,
	o.

> Best regards,
> Jernej
> 
> >  #define SUN6I_LOSC_CTRL_ALM_DHMS_ACC		BIT(9)
> >  #define SUN6I_LOSC_CTRL_RTC_HMS_ACC		BIT(8)
> >  #define SUN6I_LOSC_CTRL_RTC_YMD_ACC		BIT(7)
> > +#define SUN6I_LOSC_CTRL_EXT_LOSC_EN		BIT(4)
> >  #define SUN6I_LOSC_CTRL_EXT_OSC			BIT(0)
> >  #define SUN6I_LOSC_CTRL_ACC_MASK		GENMASK(9, 7)
> > 
> > @@ -128,6 +130,8 @@ struct sun6i_rtc_clk_data {
> >  	unsigned int has_prescaler : 1;
> >  	unsigned int has_out_clk : 1;
> >  	unsigned int export_iosc : 1;
> > +	unsigned int has_losc_en : 1;
> > +	unsigned int has_auto_swt : 1;
> >  };
> > 
> >  struct sun6i_rtc_dev {
> > @@ -190,6 +194,10 @@ static int sun6i_rtc_osc_set_parent(struct clk_hw *hw,
> > u8 index) val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
> >  	val |= SUN6I_LOSC_CTRL_KEY;
> >  	val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
> > +	if (rtc->data->has_losc_en) {
> > +		val &= ~SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> > +		val |= index ? SUN6I_LOSC_CTRL_EXT_LOSC_EN : 0;
> > +	}
> >  	writel(val, rtc->base + SUN6I_LOSC_CTRL);
> >  	spin_unlock_irqrestore(&rtc->lock, flags);
> > 
> > @@ -215,6 +223,7 @@ static void __init sun6i_rtc_clk_init(struct device_node
> > *node, const char *iosc_name = "rtc-int-osc";
> >  	const char *clkout_name = "osc32k-out";
> >  	const char *parents[2];
> > +	u32 reg;
> > 
> >  	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
> >  	if (!rtc)
> > @@ -235,9 +244,18 @@ static void __init sun6i_rtc_clk_init(struct
> > device_node *node, goto err;
> >  	}
> > 
> > +	reg = SUN6I_LOSC_CTRL_KEY;
> > +	if (rtc->data->has_auto_swt) {
> > +		/* Bypass auto-switch to int osc, on ext losc failure */
> > +		reg |= SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS;
> > +		writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > +	}
> > +
> >  	/* Switch to the external, more precise, oscillator */
> > -	writel(SUN6I_LOSC_CTRL_KEY | SUN6I_LOSC_CTRL_EXT_OSC,
> > -	       rtc->base + SUN6I_LOSC_CTRL);
> > +	reg |= SUN6I_LOSC_CTRL_EXT_OSC;
> > +	if (rtc->data->has_losc_en)
> > +		reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> > +	writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > 
> >  	/* Yes, I know, this is ugly. */
> >  	sun6i_rtc = rtc;
> > @@ -345,6 +363,23 @@ 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);
> > +
> >  static const struct sun6i_rtc_clk_data sun8i_v3_rtc_data = {
> >  	.rc_osc_rate = 32000,
> >  	.has_out_clk = 1,
> > @@ -675,6 +710,7 @@ static const struct of_device_id sun6i_rtc_dt_ids[] = {
> >  	{ .compatible = "allwinner,sun8i-r40-rtc" },
> >  	{ .compatible = "allwinner,sun8i-v3-rtc" },
> >  	{ .compatible = "allwinner,sun50i-h5-rtc" },
> > +	{ .compatible = "allwinner,sun50i-h6-rtc" },
> >  	{ /* sentinel */ },
> >  };
> >  MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
> 
> 
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-24 12:46     ` Ondřej Jirman
@ 2019-08-24 12:51       ` Jernej Škrabec
  2019-08-24 13:05         ` Ondřej Jirman
  0 siblings, 1 reply; 21+ messages in thread
From: Jernej Škrabec @ 2019-08-24 12:51 UTC (permalink / raw)
  To: linux-sunxi, megous
  Cc: Mark Rutland, Alessandro Zummo, Alexandre Belloni, devicetree,
	Maxime Ripard, linux-kernel, Chen-Yu Tsai, Rob Herring,
	linux-arm-kernel, linux-rtc

Dne sobota, 24. avgust 2019 ob 14:46:54 CEST je Ondřej Jirman napisal(a):
> Hi,
> 
> On Sat, Aug 24, 2019 at 02:32:32PM +0200, Jernej Škrabec wrote:
> > Hi!
> > 
> > Dne torek, 20. avgust 2019 ob 17:19:33 CEST je megous@megous.com 
napisal(a):
> > > From: Ondrej Jirman <megous@megous.com>
> > > 
> > > RTC on H6 is mostly the same as on H5 and H3. It has slight differences
> > > mostly in features that are not yet supported by this driver.
> > > 
> > > Some differences are already stated in the comments in existing code.
> > > One other difference is that H6 has extra bit in LOSC_CTRL_REG, called
> > > EXT_LOSC_EN to enable/disable external low speed crystal oscillator.
> > > 
> > > It also has bit EXT_LOSC_STA in LOSC_AUTO_SWT_STA_REG, to check whether
> > > external low speed oscillator is working correctly.
> > > 
> > > This patch adds support for enabling LOSC when necessary:
> > > 
> > > - during reparenting
> > > - when probing the clock
> > > 
> > > H6 also has capacbility to automatically reparent RTC clock from
> > > external crystal oscillator, to internal RC oscillator, if external
> > > oscillator fails. This is enabled by default. Disable it during
> > > probe.
> > > 
> > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > > ---
> > > 
> > >  drivers/rtc/rtc-sun6i.c | 40 ++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 38 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > > index d50ee023b559..b0c3752bed3f 100644
> > > --- a/drivers/rtc/rtc-sun6i.c
> > > +++ b/drivers/rtc/rtc-sun6i.c
> > > @@ -32,9 +32,11 @@
> > > 
> > >  /* Control register */
> > >  #define SUN6I_LOSC_CTRL				0x0000
> > >  #define SUN6I_LOSC_CTRL_KEY			(0x16aa << 16)
> > > 
> > > +#define SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS		BIT(15)
> > 
> > User manual says that above field is bit 14.
> 
> See the previous discussion, this is from BSP.

I have two versions of BSP (don't ask me which) which have this set as bit 14 
and changing this to 14 actually solves all my problems with LOSC (no more 
issues with setting RTC and HDMI-CEC works now - it uses LOSC as parent) on 
Tanix TX6 box.

Best regards,
Jernej

> 
> regards,
> 	o.
> 
> > Best regards,
> > Jernej
> > 
> > >  #define SUN6I_LOSC_CTRL_ALM_DHMS_ACC		BIT(9)
> > >  #define SUN6I_LOSC_CTRL_RTC_HMS_ACC		BIT(8)
> > >  #define SUN6I_LOSC_CTRL_RTC_YMD_ACC		BIT(7)
> > > 
> > > +#define SUN6I_LOSC_CTRL_EXT_LOSC_EN		BIT(4)
> > > 
> > >  #define SUN6I_LOSC_CTRL_EXT_OSC			BIT(0)
> > >  #define SUN6I_LOSC_CTRL_ACC_MASK		GENMASK(9, 7)
> > > 
> > > @@ -128,6 +130,8 @@ struct sun6i_rtc_clk_data {
> > > 
> > >  	unsigned int has_prescaler : 1;
> > >  	unsigned int has_out_clk : 1;
> > >  	unsigned int export_iosc : 1;
> > > 
> > > +	unsigned int has_losc_en : 1;
> > > +	unsigned int has_auto_swt : 1;
> > > 
> > >  };
> > >  
> > >  struct sun6i_rtc_dev {
> > > 
> > > @@ -190,6 +194,10 @@ static int sun6i_rtc_osc_set_parent(struct clk_hw
> > > *hw,
> > > u8 index) val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
> > > 
> > >  	val |= SUN6I_LOSC_CTRL_KEY;
> > >  	val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
> > > 
> > > +	if (rtc->data->has_losc_en) {
> > > +		val &= ~SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> > > +		val |= index ? SUN6I_LOSC_CTRL_EXT_LOSC_EN : 0;
> > > +	}
> > > 
> > >  	writel(val, rtc->base + SUN6I_LOSC_CTRL);
> > >  	spin_unlock_irqrestore(&rtc->lock, flags);
> > > 
> > > @@ -215,6 +223,7 @@ static void __init sun6i_rtc_clk_init(struct
> > > device_node *node, const char *iosc_name = "rtc-int-osc";
> > > 
> > >  	const char *clkout_name = "osc32k-out";
> > >  	const char *parents[2];
> > > 
> > > +	u32 reg;
> > > 
> > >  	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
> > >  	if (!rtc)
> > > 
> > > @@ -235,9 +244,18 @@ static void __init sun6i_rtc_clk_init(struct
> > > device_node *node, goto err;
> > > 
> > >  	}
> > > 
> > > +	reg = SUN6I_LOSC_CTRL_KEY;
> > > +	if (rtc->data->has_auto_swt) {
> > > +		/* Bypass auto-switch to int osc, on ext losc failure 
*/
> > > +		reg |= SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS;
> > > +		writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > > +	}
> > > +
> > > 
> > >  	/* Switch to the external, more precise, oscillator */
> > > 
> > > -	writel(SUN6I_LOSC_CTRL_KEY | SUN6I_LOSC_CTRL_EXT_OSC,
> > > -	       rtc->base + SUN6I_LOSC_CTRL);
> > > +	reg |= SUN6I_LOSC_CTRL_EXT_OSC;
> > > +	if (rtc->data->has_losc_en)
> > > +		reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> > > +	writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > > 
> > >  	/* Yes, I know, this is ugly. */
> > >  	sun6i_rtc = rtc;
> > > 
> > > @@ -345,6 +363,23 @@ 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);
> > > +
> > > 
> > >  static const struct sun6i_rtc_clk_data sun8i_v3_rtc_data = {
> > >  
> > >  	.rc_osc_rate = 32000,
> > >  	.has_out_clk = 1,
> > > 
> > > @@ -675,6 +710,7 @@ static const struct of_device_id sun6i_rtc_dt_ids[]
> > > = {
> > > 
> > >  	{ .compatible = "allwinner,sun8i-r40-rtc" },
> > >  	{ .compatible = "allwinner,sun8i-v3-rtc" },
> > >  	{ .compatible = "allwinner,sun50i-h5-rtc" },
> > > 
> > > +	{ .compatible = "allwinner,sun50i-h6-rtc" },
> > > 
> > >  	{ /* sentinel */ },
> > >  
> > >  };
> > >  MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel





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

* Re: [linux-sunxi] [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC
  2019-08-24  8:06   ` Jernej Škrabec
@ 2019-08-24 12:56     ` Ondřej Jirman
  2019-08-24 12:58       ` Jernej Škrabec
  0 siblings, 1 reply; 21+ messages in thread
From: Ondřej Jirman @ 2019-08-24 12:56 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: linux-sunxi, Alessandro Zummo, Alexandre Belloni, devicetree,
	Maxime Ripard, linux-kernel, Chen-Yu Tsai, Rob Herring,
	Mark Rutland, linux-arm-kernel, linux-rtc

Hi,

On Sat, Aug 24, 2019 at 10:06:14AM +0200, Jernej Škrabec wrote:
> Dne sobota, 24. avgust 2019 ob 10:04:24 CEST je Jernej Škrabec napisal(a):
> > Hi!
> > 
> > Dne torek, 20. avgust 2019 ob 17:19:31 CEST je megous@megous.com napisal(a):
> > > From: Ondrej Jirman <megous@megous.com>
> > > 
> > > I went through the datasheets for H6 and H5, and compared the differences.
> > > RTCs are largely similar, but not entirely compatible. Incompatibilities
> > > are in details not yet implemented by the rtc driver though.
> > > 
> > > I also corrected the clock tree in H6 DTSI.
> > > 
> > > This patchset is necessary for implementing the WiFi/Bluetooth support
> > > on boards using H6 SoC.
> > > 
> > > There was some discussion previously of describing HOSC, DCXO and XO
> > > oscillators and clocks as part of RTC in DT, but I decided against it
> > > because it's not necessary, becuse information that would be provided
> > > as a part of DT can already be determined at runtime from RTC registers,
> > > so this woudn't add any value and would only introduce complications
> > > to the driver. See: https://patchwork.kernel.org/cover/10898083/
> > > 
> > > Please take a look.
> > > 
> > > 
> > > Thank you and regards,
> > > 
> > >   Ondrej Jirman
> > 
> > Sorry for a bit late test, but with your patches on Tanix TX6 box I get this
> > in dmesg:
> > 
> > [   17.431742] sun6i-rtc 7000000.rtc: Failed to set rtc time.
> > [   20.439742] sun6i-rtc 7000000.rtc: rtc is still busy.
> > [   21.435744] sun6i-rtc 7000000.rtc: rtc is still busy.
> > [   24.055741] sun6i-rtc 7000000.rtc: rtc is still busy.
> > [   24.439752] sun6i-rtc 7000000.rtc: rtc is still busy.
> > 
> > Last line is repeated non-stop.
> > 
> > Any idea what could be wrong?
> 
> Additional info - this is on kernel 5.2.6 with your patches applied.

Do you have schematics, or a FEX file for the board or any other info on how the
RTC is set up on that board?

Can you dump the RTC register range?

regards,
	o.

> Best regards,
> Jernej
> 
> 
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [linux-sunxi] [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC
  2019-08-24 12:56     ` Ondřej Jirman
@ 2019-08-24 12:58       ` Jernej Škrabec
  0 siblings, 0 replies; 21+ messages in thread
From: Jernej Škrabec @ 2019-08-24 12:58 UTC (permalink / raw)
  To: linux-sunxi, megous
  Cc: Alessandro Zummo, Alexandre Belloni, devicetree, Maxime Ripard,
	linux-kernel, Chen-Yu Tsai, Rob Herring, Mark Rutland,
	linux-arm-kernel, linux-rtc

Dne sobota, 24. avgust 2019 ob 14:56:12 CEST je Ondřej Jirman napisal(a):
> Hi,
> 
> On Sat, Aug 24, 2019 at 10:06:14AM +0200, Jernej Škrabec wrote:
> > Dne sobota, 24. avgust 2019 ob 10:04:24 CEST je Jernej Škrabec napisal(a):
> > > Hi!
> > > 
> > > Dne torek, 20. avgust 2019 ob 17:19:31 CEST je megous@megous.com 
napisal(a):
> > > > From: Ondrej Jirman <megous@megous.com>
> > > > 
> > > > I went through the datasheets for H6 and H5, and compared the
> > > > differences.
> > > > RTCs are largely similar, but not entirely compatible.
> > > > Incompatibilities
> > > > are in details not yet implemented by the rtc driver though.
> > > > 
> > > > I also corrected the clock tree in H6 DTSI.
> > > > 
> > > > This patchset is necessary for implementing the WiFi/Bluetooth support
> > > > on boards using H6 SoC.
> > > > 
> > > > There was some discussion previously of describing HOSC, DCXO and XO
> > > > oscillators and clocks as part of RTC in DT, but I decided against it
> > > > because it's not necessary, becuse information that would be provided
> > > > as a part of DT can already be determined at runtime from RTC
> > > > registers,
> > > > so this woudn't add any value and would only introduce complications
> > > > to the driver. See: https://patchwork.kernel.org/cover/10898083/
> > > > 
> > > > Please take a look.
> > > > 
> > > > 
> > > > Thank you and regards,
> > > > 
> > > >   Ondrej Jirman
> > > 
> > > Sorry for a bit late test, but with your patches on Tanix TX6 box I get
> > > this in dmesg:
> > > 
> > > [   17.431742] sun6i-rtc 7000000.rtc: Failed to set rtc time.
> > > [   20.439742] sun6i-rtc 7000000.rtc: rtc is still busy.
> > > [   21.435744] sun6i-rtc 7000000.rtc: rtc is still busy.
> > > [   24.055741] sun6i-rtc 7000000.rtc: rtc is still busy.
> > > [   24.439752] sun6i-rtc 7000000.rtc: rtc is still busy.
> > > 
> > > Last line is repeated non-stop.
> > > 
> > > Any idea what could be wrong?
> > 
> > Additional info - this is on kernel 5.2.6 with your patches applied.
> 
> Do you have schematics, or a FEX file for the board or any other info on how
> the RTC is set up on that board?
> 
> Can you dump the RTC register range?

I have only Android DT, but as I already said in latest reply to patch 2, 
changing switch bypass to bit 14 instead of 15 solved all issues.

Best regards,
Jernej

> 
> regards,
> 	o.
> 
> > Best regards,
> > Jernej
> > 
> > 
> > 
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel





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

* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-24 12:51       ` Jernej Škrabec
@ 2019-08-24 13:05         ` Ondřej Jirman
  2019-08-24 13:16           ` Jernej Škrabec
  0 siblings, 1 reply; 21+ messages in thread
From: Ondřej Jirman @ 2019-08-24 13:05 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: linux-sunxi, Mark Rutland, Alessandro Zummo, Alexandre Belloni,
	devicetree, Maxime Ripard, linux-kernel, Chen-Yu Tsai,
	Rob Herring, linux-arm-kernel, linux-rtc

On Sat, Aug 24, 2019 at 02:51:54PM +0200, Jernej Škrabec wrote:
> Dne sobota, 24. avgust 2019 ob 14:46:54 CEST je Ondřej Jirman napisal(a):
> > Hi,
> > 
> > On Sat, Aug 24, 2019 at 02:32:32PM +0200, Jernej Škrabec wrote:
> > > Hi!
> > > 
> > > Dne torek, 20. avgust 2019 ob 17:19:33 CEST je megous@megous.com 
> napisal(a):
> > > > From: Ondrej Jirman <megous@megous.com>
> > > > 
> > > > RTC on H6 is mostly the same as on H5 and H3. It has slight differences
> > > > mostly in features that are not yet supported by this driver.
> > > > 
> > > > Some differences are already stated in the comments in existing code.
> > > > One other difference is that H6 has extra bit in LOSC_CTRL_REG, called
> > > > EXT_LOSC_EN to enable/disable external low speed crystal oscillator.
> > > > 
> > > > It also has bit EXT_LOSC_STA in LOSC_AUTO_SWT_STA_REG, to check whether
> > > > external low speed oscillator is working correctly.
> > > > 
> > > > This patch adds support for enabling LOSC when necessary:
> > > > 
> > > > - during reparenting
> > > > - when probing the clock
> > > > 
> > > > H6 also has capacbility to automatically reparent RTC clock from
> > > > external crystal oscillator, to internal RC oscillator, if external
> > > > oscillator fails. This is enabled by default. Disable it during
> > > > probe.
> > > > 
> > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > > > ---
> > > > 
> > > >  drivers/rtc/rtc-sun6i.c | 40 ++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 38 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > > > index d50ee023b559..b0c3752bed3f 100644
> > > > --- a/drivers/rtc/rtc-sun6i.c
> > > > +++ b/drivers/rtc/rtc-sun6i.c
> > > > @@ -32,9 +32,11 @@
> > > > 
> > > >  /* Control register */
> > > >  #define SUN6I_LOSC_CTRL				0x0000
> > > >  #define SUN6I_LOSC_CTRL_KEY			(0x16aa << 16)
> > > > 
> > > > +#define SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS		BIT(15)
> > > 
> > > User manual says that above field is bit 14.
> > 
> > See the previous discussion, this is from BSP.
> 
> I have two versions of BSP (don't ask me which) which have this set as bit 14 
> and changing this to 14 actually solves all my problems with LOSC (no more 
> issues with setting RTC and HDMI-CEC works now - it uses LOSC as parent) on 
> Tanix TX6 box.

Interesting. Is LOSC fed externally generated clock, or is it setup as a crystal
oscillator on your board?

Anyway, see here:

https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.h?h=h6-4.9-bsp#n649
https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n652

It would be nice to know what's really happening.

My output is:

[    0.832252] sun6i-rtc 7000000.rtc: registered as rtc0
[    0.832257] sun6i-rtc 7000000.rtc: RTC enabled
[    1.728968] sun6i-rtc 7000000.rtc: setting system clock to 1970-01-01T00:00:07 UTC (7)

I think, you may have just enabled the auto switch feature, and running the
clock from low precision RC oscillator with your patch.

The real issue probably is that the mainline driver is missing this:

https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n650

regards,
	o.

> Best regards,
> Jernej
> 
> > 
> > regards,
> > 	o.
> > 
> > > Best regards,
> > > Jernej
> > > 
> > > >  #define SUN6I_LOSC_CTRL_ALM_DHMS_ACC		BIT(9)
> > > >  #define SUN6I_LOSC_CTRL_RTC_HMS_ACC		BIT(8)
> > > >  #define SUN6I_LOSC_CTRL_RTC_YMD_ACC		BIT(7)
> > > > 
> > > > +#define SUN6I_LOSC_CTRL_EXT_LOSC_EN		BIT(4)
> > > > 
> > > >  #define SUN6I_LOSC_CTRL_EXT_OSC			BIT(0)
> > > >  #define SUN6I_LOSC_CTRL_ACC_MASK		GENMASK(9, 7)
> > > > 
> > > > @@ -128,6 +130,8 @@ struct sun6i_rtc_clk_data {
> > > > 
> > > >  	unsigned int has_prescaler : 1;
> > > >  	unsigned int has_out_clk : 1;
> > > >  	unsigned int export_iosc : 1;
> > > > 
> > > > +	unsigned int has_losc_en : 1;
> > > > +	unsigned int has_auto_swt : 1;
> > > > 
> > > >  };
> > > >  
> > > >  struct sun6i_rtc_dev {
> > > > 
> > > > @@ -190,6 +194,10 @@ static int sun6i_rtc_osc_set_parent(struct clk_hw
> > > > *hw,
> > > > u8 index) val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
> > > > 
> > > >  	val |= SUN6I_LOSC_CTRL_KEY;
> > > >  	val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
> > > > 
> > > > +	if (rtc->data->has_losc_en) {
> > > > +		val &= ~SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> > > > +		val |= index ? SUN6I_LOSC_CTRL_EXT_LOSC_EN : 0;
> > > > +	}
> > > > 
> > > >  	writel(val, rtc->base + SUN6I_LOSC_CTRL);
> > > >  	spin_unlock_irqrestore(&rtc->lock, flags);
> > > > 
> > > > @@ -215,6 +223,7 @@ static void __init sun6i_rtc_clk_init(struct
> > > > device_node *node, const char *iosc_name = "rtc-int-osc";
> > > > 
> > > >  	const char *clkout_name = "osc32k-out";
> > > >  	const char *parents[2];
> > > > 
> > > > +	u32 reg;
> > > > 
> > > >  	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
> > > >  	if (!rtc)
> > > > 
> > > > @@ -235,9 +244,18 @@ static void __init sun6i_rtc_clk_init(struct
> > > > device_node *node, goto err;
> > > > 
> > > >  	}
> > > > 
> > > > +	reg = SUN6I_LOSC_CTRL_KEY;
> > > > +	if (rtc->data->has_auto_swt) {
> > > > +		/* Bypass auto-switch to int osc, on ext losc failure 
> */
> > > > +		reg |= SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS;
> > > > +		writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > > > +	}
> > > > +
> > > > 
> > > >  	/* Switch to the external, more precise, oscillator */
> > > > 
> > > > -	writel(SUN6I_LOSC_CTRL_KEY | SUN6I_LOSC_CTRL_EXT_OSC,
> > > > -	       rtc->base + SUN6I_LOSC_CTRL);
> > > > +	reg |= SUN6I_LOSC_CTRL_EXT_OSC;
> > > > +	if (rtc->data->has_losc_en)
> > > > +		reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> > > > +	writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > > > 
> > > >  	/* Yes, I know, this is ugly. */
> > > >  	sun6i_rtc = rtc;
> > > > 
> > > > @@ -345,6 +363,23 @@ 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);
> > > > +
> > > > 
> > > >  static const struct sun6i_rtc_clk_data sun8i_v3_rtc_data = {
> > > >  
> > > >  	.rc_osc_rate = 32000,
> > > >  	.has_out_clk = 1,
> > > > 
> > > > @@ -675,6 +710,7 @@ static const struct of_device_id sun6i_rtc_dt_ids[]
> > > > = {
> > > > 
> > > >  	{ .compatible = "allwinner,sun8i-r40-rtc" },
> > > >  	{ .compatible = "allwinner,sun8i-v3-rtc" },
> > > >  	{ .compatible = "allwinner,sun50i-h5-rtc" },
> > > > 
> > > > +	{ .compatible = "allwinner,sun50i-h6-rtc" },
> > > > 
> > > >  	{ /* sentinel */ },
> > > >  
> > > >  };
> > > >  MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-24 13:05         ` Ondřej Jirman
@ 2019-08-24 13:16           ` Jernej Škrabec
  2019-08-24 13:30             ` Ondřej Jirman
  0 siblings, 1 reply; 21+ messages in thread
From: Jernej Škrabec @ 2019-08-24 13:16 UTC (permalink / raw)
  To: linux-sunxi, megous
  Cc: Mark Rutland, Alessandro Zummo, Alexandre Belloni, devicetree,
	Maxime Ripard, linux-kernel, Chen-Yu Tsai, Rob Herring,
	linux-arm-kernel, linux-rtc

Dne sobota, 24. avgust 2019 ob 15:05:44 CEST je Ondřej Jirman napisal(a):
> On Sat, Aug 24, 2019 at 02:51:54PM +0200, Jernej Škrabec wrote:
> > Dne sobota, 24. avgust 2019 ob 14:46:54 CEST je Ondřej Jirman napisal(a):
> > > Hi,
> > > 
> > > On Sat, Aug 24, 2019 at 02:32:32PM +0200, Jernej Škrabec wrote:
> > > > Hi!
> > > > 
> > > > Dne torek, 20. avgust 2019 ob 17:19:33 CEST je megous@megous.com
> > 
> > napisal(a):
> > > > > From: Ondrej Jirman <megous@megous.com>
> > > > > 
> > > > > RTC on H6 is mostly the same as on H5 and H3. It has slight
> > > > > differences
> > > > > mostly in features that are not yet supported by this driver.
> > > > > 
> > > > > Some differences are already stated in the comments in existing
> > > > > code.
> > > > > One other difference is that H6 has extra bit in LOSC_CTRL_REG,
> > > > > called
> > > > > EXT_LOSC_EN to enable/disable external low speed crystal oscillator.
> > > > > 
> > > > > It also has bit EXT_LOSC_STA in LOSC_AUTO_SWT_STA_REG, to check
> > > > > whether
> > > > > external low speed oscillator is working correctly.
> > > > > 
> > > > > This patch adds support for enabling LOSC when necessary:
> > > > > 
> > > > > - during reparenting
> > > > > - when probing the clock
> > > > > 
> > > > > H6 also has capacbility to automatically reparent RTC clock from
> > > > > external crystal oscillator, to internal RC oscillator, if external
> > > > > oscillator fails. This is enabled by default. Disable it during
> > > > > probe.
> > > > > 
> > > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > > > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > > > > ---
> > > > > 
> > > > >  drivers/rtc/rtc-sun6i.c | 40
> > > > >  ++++++++++++++++++++++++++++++++++++++--
> > > > >  1 file changed, 38 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > > > > index d50ee023b559..b0c3752bed3f 100644
> > > > > --- a/drivers/rtc/rtc-sun6i.c
> > > > > +++ b/drivers/rtc/rtc-sun6i.c
> > > > > @@ -32,9 +32,11 @@
> > > > > 
> > > > >  /* Control register */
> > > > >  #define SUN6I_LOSC_CTRL				0x0000
> > > > >  #define SUN6I_LOSC_CTRL_KEY			(0x16aa 
<< 16)
> > > > > 
> > > > > +#define SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS		BIT(15)
> > > > 
> > > > User manual says that above field is bit 14.
> > > 
> > > See the previous discussion, this is from BSP.
> > 
> > I have two versions of BSP (don't ask me which) which have this set as bit
> > 14 and changing this to 14 actually solves all my problems with LOSC (no
> > more issues with setting RTC and HDMI-CEC works now - it uses LOSC as
> > parent) on Tanix TX6 box.
> 
> Interesting. Is LOSC fed externally generated clock, or is it setup as a
> crystal oscillator on your board?

I really don't know, but here is DT: http://ix.io/1ThI

> 
> Anyway, see here:
> 
> https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.h?h=h6-4.9-bsp#n649
> https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n652

Interesting, 4.9 BSP has additional bit definition, which is not documented in 
manual and 3.10 BSP to which I refer.

I was referring to 3.10 BSP, which uses only bit 14. I thought that you named 
it differently.

> 
> It would be nice to know what's really happening.
> 
> My output is:
> 
> [    0.832252] sun6i-rtc 7000000.rtc: registered as rtc0
> [    0.832257] sun6i-rtc 7000000.rtc: RTC enabled
> [    1.728968] sun6i-rtc 7000000.rtc: setting system clock to
> 1970-01-01T00:00:07 UTC (7)

With change, I get same output.

> 
> I think, you may have just enabled the auto switch feature, and running the
> clock from low precision RC oscillator with your patch.

True, now I think there is no external crystal, but I'm still not sure how to 
confirm that.

> 
> The real issue probably is that the mainline driver is missing this:
> 
> https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n650
> 

Not sure what you mean by that. ext vs. int source selection?

Best regards,
Jernej

> regards,
> 	o.
> 
> > Best regards,
> > Jernej
> > 
> > > regards,
> > > 
> > > 	o.
> > > 	
> > > > Best regards,
> > > > Jernej
> > > > 
> > > > >  #define SUN6I_LOSC_CTRL_ALM_DHMS_ACC		BIT(9)
> > > > >  #define SUN6I_LOSC_CTRL_RTC_HMS_ACC		BIT(8)
> > > > >  #define SUN6I_LOSC_CTRL_RTC_YMD_ACC		BIT(7)
> > > > > 
> > > > > +#define SUN6I_LOSC_CTRL_EXT_LOSC_EN		BIT(4)
> > > > > 
> > > > >  #define SUN6I_LOSC_CTRL_EXT_OSC			BIT(0)
> > > > >  #define SUN6I_LOSC_CTRL_ACC_MASK		GENMASK(9, 7)
> > > > > 
> > > > > @@ -128,6 +130,8 @@ struct sun6i_rtc_clk_data {
> > > > > 
> > > > >  	unsigned int has_prescaler : 1;
> > > > >  	unsigned int has_out_clk : 1;
> > > > >  	unsigned int export_iosc : 1;
> > > > > 
> > > > > +	unsigned int has_losc_en : 1;
> > > > > +	unsigned int has_auto_swt : 1;
> > > > > 
> > > > >  };
> > > > >  
> > > > >  struct sun6i_rtc_dev {
> > > > > 
> > > > > @@ -190,6 +194,10 @@ static int sun6i_rtc_osc_set_parent(struct
> > > > > clk_hw
> > > > > *hw,
> > > > > u8 index) val &= ~SUN6I_LOSC_CTRL_EXT_OSC;
> > > > > 
> > > > >  	val |= SUN6I_LOSC_CTRL_KEY;
> > > > >  	val |= index ? SUN6I_LOSC_CTRL_EXT_OSC : 0;
> > > > > 
> > > > > +	if (rtc->data->has_losc_en) {
> > > > > +		val &= ~SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> > > > > +		val |= index ? SUN6I_LOSC_CTRL_EXT_LOSC_EN : 0;
> > > > > +	}
> > > > > 
> > > > >  	writel(val, rtc->base + SUN6I_LOSC_CTRL);
> > > > >  	spin_unlock_irqrestore(&rtc->lock, flags);
> > > > > 
> > > > > @@ -215,6 +223,7 @@ static void __init sun6i_rtc_clk_init(struct
> > > > > device_node *node, const char *iosc_name = "rtc-int-osc";
> > > > > 
> > > > >  	const char *clkout_name = "osc32k-out";
> > > > >  	const char *parents[2];
> > > > > 
> > > > > +	u32 reg;
> > > > > 
> > > > >  	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
> > > > >  	if (!rtc)
> > > > > 
> > > > > @@ -235,9 +244,18 @@ static void __init sun6i_rtc_clk_init(struct
> > > > > device_node *node, goto err;
> > > > > 
> > > > >  	}
> > > > > 
> > > > > +	reg = SUN6I_LOSC_CTRL_KEY;
> > > > > +	if (rtc->data->has_auto_swt) {
> > > > > +		/* Bypass auto-switch to int osc, on ext losc failure
> > 
> > */
> > 
> > > > > +		reg |= SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS;
> > > > > +		writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > > > > +	}
> > > > > +
> > > > > 
> > > > >  	/* Switch to the external, more precise, oscillator */
> > > > > 
> > > > > -	writel(SUN6I_LOSC_CTRL_KEY | SUN6I_LOSC_CTRL_EXT_OSC,
> > > > > -	       rtc->base + SUN6I_LOSC_CTRL);
> > > > > +	reg |= SUN6I_LOSC_CTRL_EXT_OSC;
> > > > > +	if (rtc->data->has_losc_en)
> > > > > +		reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN;
> > > > > +	writel(reg, rtc->base + SUN6I_LOSC_CTRL);
> > > > > 
> > > > >  	/* Yes, I know, this is ugly. */
> > > > >  	sun6i_rtc = rtc;
> > > > > 
> > > > > @@ -345,6 +363,23 @@ 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);
> > > > > +
> > > > > 
> > > > >  static const struct sun6i_rtc_clk_data sun8i_v3_rtc_data = {
> > > > >  
> > > > >  	.rc_osc_rate = 32000,
> > > > >  	.has_out_clk = 1,
> > > > > 
> > > > > @@ -675,6 +710,7 @@ static const struct of_device_id
> > > > > sun6i_rtc_dt_ids[]
> > > > > = {
> > > > > 
> > > > >  	{ .compatible = "allwinner,sun8i-r40-rtc" },
> > > > >  	{ .compatible = "allwinner,sun8i-v3-rtc" },
> > > > >  	{ .compatible = "allwinner,sun50i-h5-rtc" },
> > > > > 
> > > > > +	{ .compatible = "allwinner,sun50i-h6-rtc" },
> > > > > 
> > > > >  	{ /* sentinel */ },
> > > > >  
> > > > >  };
> > > > >  MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
> > > > 
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel





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

* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-24 13:16           ` Jernej Škrabec
@ 2019-08-24 13:30             ` Ondřej Jirman
  2019-08-24 21:09               ` Jernej Škrabec
  0 siblings, 1 reply; 21+ messages in thread
From: Ondřej Jirman @ 2019-08-24 13:30 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: linux-sunxi, Mark Rutland, Alessandro Zummo, Alexandre Belloni,
	devicetree, Maxime Ripard, linux-kernel, Chen-Yu Tsai,
	Rob Herring, linux-arm-kernel, linux-rtc

On Sat, Aug 24, 2019 at 03:16:41PM +0200, Jernej Škrabec wrote:
> Dne sobota, 24. avgust 2019 ob 15:05:44 CEST je Ondřej Jirman napisal(a):
> > On Sat, Aug 24, 2019 at 02:51:54PM +0200, Jernej Škrabec wrote:
> > > Dne sobota, 24. avgust 2019 ob 14:46:54 CEST je Ondřej Jirman napisal(a):
> > > > Hi,
> > > > 
> > > > On Sat, Aug 24, 2019 at 02:32:32PM +0200, Jernej Škrabec wrote:
> > > > > Hi!
> > > > > 
> > > > > Dne torek, 20. avgust 2019 ob 17:19:33 CEST je megous@megous.com
> > > 
> > > napisal(a):
> > > > > > From: Ondrej Jirman <megous@megous.com>
> > > > > > 
> > > > > > RTC on H6 is mostly the same as on H5 and H3. It has slight
> > > > > > differences
> > > > > > mostly in features that are not yet supported by this driver.
> > > > > > 
> > > > > > Some differences are already stated in the comments in existing
> > > > > > code.
> > > > > > One other difference is that H6 has extra bit in LOSC_CTRL_REG,
> > > > > > called
> > > > > > EXT_LOSC_EN to enable/disable external low speed crystal oscillator.
> > > > > > 
> > > > > > It also has bit EXT_LOSC_STA in LOSC_AUTO_SWT_STA_REG, to check
> > > > > > whether
> > > > > > external low speed oscillator is working correctly.
> > > > > > 
> > > > > > This patch adds support for enabling LOSC when necessary:
> > > > > > 
> > > > > > - during reparenting
> > > > > > - when probing the clock
> > > > > > 
> > > > > > H6 also has capacbility to automatically reparent RTC clock from
> > > > > > external crystal oscillator, to internal RC oscillator, if external
> > > > > > oscillator fails. This is enabled by default. Disable it during
> > > > > > probe.
> > > > > > 
> > > > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > > > > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/rtc/rtc-sun6i.c | 40
> > > > > >  ++++++++++++++++++++++++++++++++++++++--
> > > > > >  1 file changed, 38 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > > > > > index d50ee023b559..b0c3752bed3f 100644
> > > > > > --- a/drivers/rtc/rtc-sun6i.c
> > > > > > +++ b/drivers/rtc/rtc-sun6i.c
> > > > > > @@ -32,9 +32,11 @@
> > > > > > 
> > > > > >  /* Control register */
> > > > > >  #define SUN6I_LOSC_CTRL				0x0000
> > > > > >  #define SUN6I_LOSC_CTRL_KEY			(0x16aa 
> << 16)
> > > > > > 
> > > > > > +#define SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS		BIT(15)
> > > > > 
> > > > > User manual says that above field is bit 14.
> > > > 
> > > > See the previous discussion, this is from BSP.
> > > 
> > > I have two versions of BSP (don't ask me which) which have this set as bit
> > > 14 and changing this to 14 actually solves all my problems with LOSC (no
> > > more issues with setting RTC and HDMI-CEC works now - it uses LOSC as
> > > parent) on Tanix TX6 box.
> > 
> > Interesting. Is LOSC fed externally generated clock, or is it setup as a
> > crystal oscillator on your board?
> 
> I really don't know, but here is DT: http://ix.io/1ThI
> 
> > 
> > Anyway, see here:
> > 
> > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.h?h=h6-4.9-bsp#n649
> > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n652
> 
> Interesting, 4.9 BSP has additional bit definition, which is not documented in 
> manual and 3.10 BSP to which I refer.
> 
> I was referring to 3.10 BSP, which uses only bit 14. I thought that you named 
> it differently.
> 
> > 
> > It would be nice to know what's really happening.
> > 
> > My output is:
> > 
> > [    0.832252] sun6i-rtc 7000000.rtc: registered as rtc0
> > [    0.832257] sun6i-rtc 7000000.rtc: RTC enabled
> > [    1.728968] sun6i-rtc 7000000.rtc: setting system clock to
> > 1970-01-01T00:00:07 UTC (7)
> 
> With change, I get same output.
> 
> > 
> > I think, you may have just enabled the auto switch feature, and running the
> > clock from low precision RC oscillator with your patch.
> 
> True, now I think there is no external crystal, but I'm still not sure how to 
> confirm that.

Visually?

That would explain why it doesn't work for you. The mainline RTC driver
disables auto-switch feature, and if your board doesn't have a crystal for LOSC,
RTC will not generate a clock for the RTC.

H6's dtsi describes by default a situatiuon with external 32k crystal
oscillator. See ext_osc32k node. That's incorrect for your board if it doesn't
have the crystal. You need to fix this in the DTS for your board instead of
patching the driver.

The driver has parent clock selection logic in case the LOSC crystal is not
used.

Your patch enables automatic detection of LOSC failure and RTC changes clock
to LOSC automatically, despite what's described in the DTS. That may fix the
issue, but is not the correct solution.

Registers on my board look like this (external 32k osc is used) for reference:

LOSC_CTRL_REG[7000000]: 8011
	KEY_FIELD                      ???                  (0)
	LOSC_AUTO_SWT_BYPASS           EN                   (1)
	LOSC_AUTO_SWT_EN               DIS                  (0)
	EXT_LOSC_EN                    EN                   (1)
	EXT_LOSC_GSM                   LOW                  (0)
	BATTERY_DIR                    DISCHARGE            (0)
	LOSC_SRC_SEL                   EXT32k               (1)

LOSC_AUTO_SWT_STA_REG[7000004]: 1
	EXT_LOSC_STA                   OK                   (0)
	LOSC_AUTO_SWT_PEND             NOEFF                (0)
	LOSC_SRC_SEL_STA               EXT32K               (1)

regards,
	o.

> > 
> > The real issue probably is that the mainline driver is missing this:
> > 
> > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n650
> > 
> 
> Not sure what you mean by that. ext vs. int source selection?



> Best regards,
> Jernej
> 
> > regards,
> > 	o.

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

* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-24 13:30             ` Ondřej Jirman
@ 2019-08-24 21:09               ` Jernej Škrabec
  2019-08-24 21:27                 ` Ondřej Jirman
  0 siblings, 1 reply; 21+ messages in thread
From: Jernej Škrabec @ 2019-08-24 21:09 UTC (permalink / raw)
  To: linux-sunxi, megous
  Cc: Mark Rutland, Alessandro Zummo, Alexandre Belloni, devicetree,
	Maxime Ripard, linux-kernel, Chen-Yu Tsai, Rob Herring,
	linux-arm-kernel, linux-rtc

Dne sobota, 24. avgust 2019 ob 15:30:57 CEST je Ondřej Jirman napisal(a):
> On Sat, Aug 24, 2019 at 03:16:41PM +0200, Jernej Škrabec wrote:
> > Dne sobota, 24. avgust 2019 ob 15:05:44 CEST je Ondřej Jirman napisal(a):
> > > On Sat, Aug 24, 2019 at 02:51:54PM +0200, Jernej Škrabec wrote:
> > > > Dne sobota, 24. avgust 2019 ob 14:46:54 CEST je Ondřej Jirman 
napisal(a):
> > > > > Hi,
> > > > > 
> > > > > On Sat, Aug 24, 2019 at 02:32:32PM +0200, Jernej Škrabec wrote:
> > > > > > Hi!
> > > > > > 
> > > > > > Dne torek, 20. avgust 2019 ob 17:19:33 CEST je megous@megous.com
> > > > 
> > > > napisal(a):
> > > > > > > From: Ondrej Jirman <megous@megous.com>
> > > > > > > 
> > > > > > > RTC on H6 is mostly the same as on H5 and H3. It has slight
> > > > > > > differences
> > > > > > > mostly in features that are not yet supported by this driver.
> > > > > > > 
> > > > > > > Some differences are already stated in the comments in existing
> > > > > > > code.
> > > > > > > One other difference is that H6 has extra bit in LOSC_CTRL_REG,
> > > > > > > called
> > > > > > > EXT_LOSC_EN to enable/disable external low speed crystal
> > > > > > > oscillator.
> > > > > > > 
> > > > > > > It also has bit EXT_LOSC_STA in LOSC_AUTO_SWT_STA_REG, to check
> > > > > > > whether
> > > > > > > external low speed oscillator is working correctly.
> > > > > > > 
> > > > > > > This patch adds support for enabling LOSC when necessary:
> > > > > > > 
> > > > > > > - during reparenting
> > > > > > > - when probing the clock
> > > > > > > 
> > > > > > > H6 also has capacbility to automatically reparent RTC clock from
> > > > > > > external crystal oscillator, to internal RC oscillator, if
> > > > > > > external
> > > > > > > oscillator fails. This is enabled by default. Disable it during
> > > > > > > probe.
> > > > > > > 
> > > > > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > > > > > Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > > > > > > ---
> > > > > > > 
> > > > > > >  drivers/rtc/rtc-sun6i.c | 40
> > > > > > >  ++++++++++++++++++++++++++++++++++++++--
> > > > > > >  1 file changed, 38 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> > > > > > > index d50ee023b559..b0c3752bed3f 100644
> > > > > > > --- a/drivers/rtc/rtc-sun6i.c
> > > > > > > +++ b/drivers/rtc/rtc-sun6i.c
> > > > > > > @@ -32,9 +32,11 @@
> > > > > > > 
> > > > > > >  /* Control register */
> > > > > > >  #define SUN6I_LOSC_CTRL				
0x0000
> > > > > > >  #define SUN6I_LOSC_CTRL_KEY			(0x16aa
> > 
> > << 16)
> > 
> > > > > > > +#define SUN6I_LOSC_CTRL_AUTO_SWT_BYPASS		BIT(15)
> > > > > > 
> > > > > > User manual says that above field is bit 14.
> > > > > 
> > > > > See the previous discussion, this is from BSP.
> > > > 
> > > > I have two versions of BSP (don't ask me which) which have this set as
> > > > bit
> > > > 14 and changing this to 14 actually solves all my problems with LOSC
> > > > (no
> > > > more issues with setting RTC and HDMI-CEC works now - it uses LOSC as
> > > > parent) on Tanix TX6 box.
> > > 
> > > Interesting. Is LOSC fed externally generated clock, or is it setup as a
> > > crystal oscillator on your board?
> > 
> > I really don't know, but here is DT: http://ix.io/1ThI
> > 
> > > Anyway, see here:
> > > 
> > > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.h?h=h6-4.9-bsp#n
> > > 649
> > > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n
> > > 652
> > 
> > Interesting, 4.9 BSP has additional bit definition, which is not
> > documented in manual and 3.10 BSP to which I refer.
> > 
> > I was referring to 3.10 BSP, which uses only bit 14. I thought that you
> > named it differently.
> > 
> > > It would be nice to know what's really happening.
> > > 
> > > My output is:
> > > 
> > > [    0.832252] sun6i-rtc 7000000.rtc: registered as rtc0
> > > [    0.832257] sun6i-rtc 7000000.rtc: RTC enabled
> > > [    1.728968] sun6i-rtc 7000000.rtc: setting system clock to
> > > 1970-01-01T00:00:07 UTC (7)
> > 
> > With change, I get same output.
> > 
> > > I think, you may have just enabled the auto switch feature, and running
> > > the
> > > clock from low precision RC oscillator with your patch.
> > 
> > True, now I think there is no external crystal, but I'm still not sure how
> > to confirm that.
> 
> Visually?
> 
> That would explain why it doesn't work for you. The mainline RTC driver
> disables auto-switch feature, and if your board doesn't have a crystal for
> LOSC, RTC will not generate a clock for the RTC.
> 
> H6's dtsi describes by default a situatiuon with external 32k crystal
> oscillator. See ext_osc32k node. That's incorrect for your board if it
> doesn't have the crystal. You need to fix this in the DTS for your board
> instead of patching the driver.

I see that reparenting is supported, but I'm not sure how to fix that in DT. 
Any suggestion?

> 
> The driver has parent clock selection logic in case the LOSC crystal is not
> used.
> 
> Your patch enables automatic detection of LOSC failure and RTC changes clock
> to LOSC automatically, despite what's described in the DTS. That may fix
> the issue, but is not the correct solution.
> 
> Registers on my board look like this (external 32k osc is used) for
> reference:
> 
> LOSC_CTRL_REG[7000000]: 8011
> 	KEY_FIELD                      ???                  (0)
> 	LOSC_AUTO_SWT_BYPASS           EN                   (1)
> 	LOSC_AUTO_SWT_EN               DIS                  (0)
> 	EXT_LOSC_EN                    EN                   (1)
> 	EXT_LOSC_GSM                   LOW                  (0)
> 	BATTERY_DIR                    DISCHARGE            (0)
> 	LOSC_SRC_SEL                   EXT32k               (1)
> 
> LOSC_AUTO_SWT_STA_REG[7000004]: 1
> 	EXT_LOSC_STA                   OK                   (0)
> 	LOSC_AUTO_SWT_PEND             NOEFF                (0)
> 	LOSC_SRC_SEL_STA               EXT32K               (1)
> 

In my case LOSC_CTRL_REG has value 0x4010 and LOSC_AUTO_SWT_STA_REG
has value 0x4, so there is issue with external crystal (it's missing) and RTC 
switched to internal one.

BTW, what's wrong with automatic switching? Why is it disabled?

Best regards,
Jernej

> regards,
> 	o.
> 
> > > The real issue probably is that the mainline driver is missing this:
> > > 
> > > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n
> > > 650
> > 
> > Not sure what you mean by that. ext vs. int source selection?
> > 
> > 
> > 
> > Best regards,
> > Jernej
> > 
> > > regards,
> > > 
> > > 	o.





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

* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-24 21:09               ` Jernej Škrabec
@ 2019-08-24 21:27                 ` Ondřej Jirman
  2019-08-24 21:36                   ` Jernej Škrabec
  0 siblings, 1 reply; 21+ messages in thread
From: Ondřej Jirman @ 2019-08-24 21:27 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: linux-sunxi, Mark Rutland, Alessandro Zummo, Alexandre Belloni,
	devicetree, Maxime Ripard, linux-kernel, Chen-Yu Tsai,
	Rob Herring, linux-arm-kernel, linux-rtc

Hello Jernej,

On Sat, Aug 24, 2019 at 11:09:49PM +0200, Jernej Škrabec wrote:
> > Visually?
> > 
> > That would explain why it doesn't work for you. The mainline RTC driver
> > disables auto-switch feature, and if your board doesn't have a crystal for
> > LOSC, RTC will not generate a clock for the RTC.
> > 
> > H6's dtsi describes by default a situatiuon with external 32k crystal
> > oscillator. See ext_osc32k node. That's incorrect for your board if it
> > doesn't have the crystal. You need to fix this in the DTS for your board
> > instead of patching the driver.
> 
> I see that reparenting is supported, but I'm not sure how to fix that in DT. 
> Any suggestion?

You may try removing the clocks property from rtc node.

> > 
> > The driver has parent clock selection logic in case the LOSC crystal is not
> > used.
> > 
> > Your patch enables automatic detection of LOSC failure and RTC changes clock
> > to LOSC automatically, despite what's described in the DTS. That may fix
> > the issue, but is not the correct solution.
> > 
> > Registers on my board look like this (external 32k osc is used) for
> > reference:
> > 
> > LOSC_CTRL_REG[7000000]: 8011
> > 	KEY_FIELD                      ???                  (0)
> > 	LOSC_AUTO_SWT_BYPASS           EN                   (1)
> > 	LOSC_AUTO_SWT_EN               DIS                  (0)
> > 	EXT_LOSC_EN                    EN                   (1)
> > 	EXT_LOSC_GSM                   LOW                  (0)
> > 	BATTERY_DIR                    DISCHARGE            (0)
> > 	LOSC_SRC_SEL                   EXT32k               (1)
> > 
> > LOSC_AUTO_SWT_STA_REG[7000004]: 1
> > 	EXT_LOSC_STA                   OK                   (0)
> > 	LOSC_AUTO_SWT_PEND             NOEFF                (0)
> > 	LOSC_SRC_SEL_STA               EXT32K               (1)
> > 
> 
> In my case LOSC_CTRL_REG has value 0x4010 and LOSC_AUTO_SWT_STA_REG
> has value 0x4, so there is issue with external crystal (it's missing) and RTC 
> switched to internal one.
> 
> BTW, what's wrong with automatic switching? Why is it disabled?

It always was disabled on mainline (bit 14 was set to 0 even before my patch).
H6 just probably has another extra undocummented bit, that's needed to disables
it properly.

You probably don't want a glitch to switch your RTC from high-precision
clock to a low precision one possibly without any indication in the userspace
or a kernel log.

Regardless of all this, DTS needs to have a correct description of the HW,
which means if RTC module is not connected to the 32.757kHz crystal/clock,
clocks property should be empty.

regards,
	o.

> Best regards,
> Jernej
> 
> > regards,
> > 	o.
> > 
> > > > The real issue probably is that the mainline driver is missing this:
> > > > 
> > > > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n
> > > > 650
> > > 
> > > Not sure what you mean by that. ext vs. int source selection?
> > > 
> > > 
> > > 
> > > Best regards,
> > > Jernej
> > > 
> > > > regards,
> > > > 
> > > > 	o.
> 
> 
> 
> 

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

* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-24 21:27                 ` Ondřej Jirman
@ 2019-08-24 21:36                   ` Jernej Škrabec
  2019-08-24 22:16                     ` Ondřej Jirman
  0 siblings, 1 reply; 21+ messages in thread
From: Jernej Škrabec @ 2019-08-24 21:36 UTC (permalink / raw)
  To: linux-sunxi, megous
  Cc: Mark Rutland, Alessandro Zummo, Alexandre Belloni, devicetree,
	Maxime Ripard, linux-kernel, Chen-Yu Tsai, Rob Herring,
	linux-arm-kernel, linux-rtc

Dne sobota, 24. avgust 2019 ob 23:27:46 CEST je Ondřej Jirman napisal(a):
> Hello Jernej,
> 
> On Sat, Aug 24, 2019 at 11:09:49PM +0200, Jernej Škrabec wrote:
> > > Visually?
> > > 
> > > That would explain why it doesn't work for you. The mainline RTC driver
> > > disables auto-switch feature, and if your board doesn't have a crystal
> > > for
> > > LOSC, RTC will not generate a clock for the RTC.
> > > 
> > > H6's dtsi describes by default a situatiuon with external 32k crystal
> > > oscillator. See ext_osc32k node. That's incorrect for your board if it
> > > doesn't have the crystal. You need to fix this in the DTS for your board
> > > instead of patching the driver.
> > 
> > I see that reparenting is supported, but I'm not sure how to fix that in
> > DT. Any suggestion?
> 
> You may try removing the clocks property from rtc node.

I don't think this would work:
https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-sun6i.c#L246

> 
> > > The driver has parent clock selection logic in case the LOSC crystal is
> > > not
> > > used.
> > > 
> > > Your patch enables automatic detection of LOSC failure and RTC changes
> > > clock to LOSC automatically, despite what's described in the DTS. That
> > > may fix the issue, but is not the correct solution.
> > > 
> > > Registers on my board look like this (external 32k osc is used) for
> > > reference:
> > > 
> > > LOSC_CTRL_REG[7000000]: 8011
> > > 
> > > 	KEY_FIELD                      ???                  (0)
> > > 	LOSC_AUTO_SWT_BYPASS           EN                   (1)
> > > 	LOSC_AUTO_SWT_EN               DIS                  (0)
> > > 	EXT_LOSC_EN                    EN                   (1)
> > > 	EXT_LOSC_GSM                   LOW                  (0)
> > > 	BATTERY_DIR                    DISCHARGE            (0)
> > > 	LOSC_SRC_SEL                   EXT32k               (1)
> > > 
> > > LOSC_AUTO_SWT_STA_REG[7000004]: 1
> > > 
> > > 	EXT_LOSC_STA                   OK                   (0)
> > > 	LOSC_AUTO_SWT_PEND             NOEFF                (0)
> > > 	LOSC_SRC_SEL_STA               EXT32K               (1)
> > 
> > In my case LOSC_CTRL_REG has value 0x4010 and LOSC_AUTO_SWT_STA_REG
> > has value 0x4, so there is issue with external crystal (it's missing) and
> > RTC switched to internal one.
> > 
> > BTW, what's wrong with automatic switching? Why is it disabled?
> 
> It always was disabled on mainline (bit 14 was set to 0 even before my
> patch). H6 just probably has another extra undocummented bit, that's needed
> to disables it properly.
> 
> You probably don't want a glitch to switch your RTC from high-precision
> clock to a low precision one possibly without any indication in the
> userspace or a kernel log.
> 
> Regardless of all this, DTS needs to have a correct description of the HW,
> which means if RTC module is not connected to the 32.757kHz crystal/clock,
> clocks property should be empty.

If we are talking about correct HW description, then clock property should 
actually have possibility that two clocks are defined - one for internal RC 
(always present) and one external crystal (optional). In such case I could 
really just omit external clock and be done with it. But I'm not sure if such 
solution is acceptable at this point.

Best regards,
Jernej

> 
> regards,
> 	o.
> 
> > Best regards,
> > Jernej
> > 
> > > regards,
> > > 
> > > 	o.
> > > 	
> > > > > The real issue probably is that the mainline driver is missing this:
> > > > > 
> > > > > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-b
> > > > > sp#n
> > > > > 650
> > > > 
> > > > Not sure what you mean by that. ext vs. int source selection?
> > > > 
> > > > 
> > > > 
> > > > Best regards,
> > > > Jernej
> > > > 
> > > > > regards,
> > > > > 
> > > > > 	o.





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

* Re: [linux-sunxi] [PATCH v2 2/3] rtc: sun6i: Add support for H6 RTC
  2019-08-24 21:36                   ` Jernej Škrabec
@ 2019-08-24 22:16                     ` Ondřej Jirman
  0 siblings, 0 replies; 21+ messages in thread
From: Ondřej Jirman @ 2019-08-24 22:16 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: linux-sunxi, Mark Rutland, Alessandro Zummo, Alexandre Belloni,
	devicetree, Maxime Ripard, linux-kernel, Chen-Yu Tsai,
	Rob Herring, linux-arm-kernel, linux-rtc

On Sat, Aug 24, 2019 at 11:36:26PM +0200, Jernej Škrabec wrote:
> Dne sobota, 24. avgust 2019 ob 23:27:46 CEST je Ondřej Jirman napisal(a):
> > Hello Jernej,
> > 
> > On Sat, Aug 24, 2019 at 11:09:49PM +0200, Jernej Škrabec wrote:
> > > > Visually?
> > > > 
> > > > That would explain why it doesn't work for you. The mainline RTC driver
> > > > disables auto-switch feature, and if your board doesn't have a crystal
> > > > for
> > > > LOSC, RTC will not generate a clock for the RTC.
> > > > 
> > > > H6's dtsi describes by default a situatiuon with external 32k crystal
> > > > oscillator. See ext_osc32k node. That's incorrect for your board if it
> > > > doesn't have the crystal. You need to fix this in the DTS for your board
> > > > instead of patching the driver.
> > > 
> > > I see that reparenting is supported, but I'm not sure how to fix that in
> > > DT. Any suggestion?
> > 
> > You may try removing the clocks property from rtc node.
> 
> I don't think this would work:
> https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-sun6i.c#L246

Well, I don't know. There has to be some way to make it work, since the code
deals with it here:

https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-sun6i.c#L270

Number of parents for LOSC is calculated from the DT somehow. Maybne something
to do with the #clock-cells property?

Sorry I can't be of more help here.

> > 
> > > > The driver has parent clock selection logic in case the LOSC crystal is
> > > > not
> > > > used.
> > > > 
> > > > Your patch enables automatic detection of LOSC failure and RTC changes
> > > > clock to LOSC automatically, despite what's described in the DTS. That
> > > > may fix the issue, but is not the correct solution.
> > > > 
> > > > Registers on my board look like this (external 32k osc is used) for
> > > > reference:
> > > > 
> > > > LOSC_CTRL_REG[7000000]: 8011
> > > > 
> > > > 	KEY_FIELD                      ???                  (0)
> > > > 	LOSC_AUTO_SWT_BYPASS           EN                   (1)
> > > > 	LOSC_AUTO_SWT_EN               DIS                  (0)
> > > > 	EXT_LOSC_EN                    EN                   (1)
> > > > 	EXT_LOSC_GSM                   LOW                  (0)
> > > > 	BATTERY_DIR                    DISCHARGE            (0)
> > > > 	LOSC_SRC_SEL                   EXT32k               (1)
> > > > 
> > > > LOSC_AUTO_SWT_STA_REG[7000004]: 1
> > > > 
> > > > 	EXT_LOSC_STA                   OK                   (0)
> > > > 	LOSC_AUTO_SWT_PEND             NOEFF                (0)
> > > > 	LOSC_SRC_SEL_STA               EXT32K               (1)
> > > 
> > > In my case LOSC_CTRL_REG has value 0x4010 and LOSC_AUTO_SWT_STA_REG
> > > has value 0x4, so there is issue with external crystal (it's missing) and
> > > RTC switched to internal one.
> > > 
> > > BTW, what's wrong with automatic switching? Why is it disabled?
> > 
> > It always was disabled on mainline (bit 14 was set to 0 even before my
> > patch). H6 just probably has another extra undocummented bit, that's needed
> > to disables it properly.
> > 
> > You probably don't want a glitch to switch your RTC from high-precision
> > clock to a low precision one possibly without any indication in the
> > userspace or a kernel log.
> > 
> > Regardless of all this, DTS needs to have a correct description of the HW,
> > which means if RTC module is not connected to the 32.757kHz crystal/clock,
> > clocks property should be empty.
> 
> If we are talking about correct HW description, then clock property should 
> actually have possibility that two clocks are defined - one for internal RC 
> (always present) and one external crystal (optional). In such case I could 
> really just omit external clock and be done with it. But I'm not sure if such 

Internal RC is thought to be part of the RTC module, so it's not defined as an
input clock to the RTC module.

regards,
	Ondrej

> 
> Best regards,
> Jernej
> 
> > 
> > regards,
> > 	o.
> > 
> > > Best regards,
> > > Jernej
> > > 
> > > > regards,
> > > > 
> > > > 	o.
> > > > 	
> > > > > > The real issue probably is that the mainline driver is missing this:
> > > > > > 
> > > > > > https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-b
> > > > > > sp#n
> > > > > > 650
> > > > > 
> > > > > Not sure what you mean by that. ext vs. int source selection?
> > > > > 
> > > > > 
> > > > > 
> > > > > Best regards,
> > > > > Jernej
> > > > > 
> > > > > > regards,
> > > > > > 
> > > > > > 	o.
> 
> 
> 
> 

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

end of thread, other threads:[~2019-08-24 22:16 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-20 15:19 [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC megous
2019-08-20 15:19 ` [PATCH v2 1/3] dt-bindings: Add compatible for H6 RTC megous
2019-08-22 21:15   ` Alexandre Belloni
2019-08-20 15:19 ` [PATCH v2 2/3] rtc: sun6i: Add support " megous
2019-08-22 21:16   ` Alexandre Belloni
2019-08-24 12:32   ` [linux-sunxi] " Jernej Škrabec
2019-08-24 12:46     ` Ondřej Jirman
2019-08-24 12:51       ` Jernej Škrabec
2019-08-24 13:05         ` Ondřej Jirman
2019-08-24 13:16           ` Jernej Škrabec
2019-08-24 13:30             ` Ondřej Jirman
2019-08-24 21:09               ` Jernej Škrabec
2019-08-24 21:27                 ` Ondřej Jirman
2019-08-24 21:36                   ` Jernej Škrabec
2019-08-24 22:16                     ` Ondřej Jirman
2019-08-20 15:19 ` [PATCH v2 3/3] arm64: dts: sun50i-h6: Add support for RTC and fix the clock tree megous
2019-08-23  8:19   ` Maxime Ripard
2019-08-24  8:04 ` [linux-sunxi] [PATCH v2 0/3] Add basic support for RTC on Allwinner H6 SoC Jernej Škrabec
2019-08-24  8:06   ` Jernej Škrabec
2019-08-24 12:56     ` Ondřej Jirman
2019-08-24 12:58       ` Jernej Škrabec

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).