rtc: sunxi: use external oscillator

PATCH In review Stephan Roslen 3 participants 8 messages Jul 6, 2016 – Jul 7, 2016

Subsystems (2)

Arm/Allwinner sunXi SoC support Real time clock (RTC)

lore

Share & tools

lore

Stephan Roslen

author

2016-07-06 08:43 UTC #0 lore raw

PATCH 1/1 rtc: sunxi: use external oscillator

We noticed some serious drift problems in Allwinner A20 RTCs. I found out, that the oscillator source needs to be set to an external 32.768 KHz oscillator instead of the internal 32.000 KHz oscillator.

Signed-off-by Stephan Roslen <[email protected]>

---
 drivers/rtc/rtc-sunxi.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

drivers/rtc/rtc-sunxi.c +25 -0

index abada60..001256e 100644
@@ -34,9 +34,19 @@
 #include <linux/types.h>
 
 #define SUNXI_LOSC_CTRL				0x0000
+
+/* magic number required to set bit 0 */
+#define SUNXI_LOSC_CTRL_KEY			0x16AA0000
+
+#define SUNXI_LOSC_CTRL_AUTO_SWT_EN		BIT(14)
+
 #define SUNXI_LOSC_CTRL_RTC_HMS_ACC		BIT(8)
 #define SUNXI_LOSC_CTRL_RTC_YMD_ACC		BIT(7)
 
+#define SUNXI_LOSC_CTRL_EXT_GSM1		BIT(3)
+#define SUNXI_LOSC_CTRL_EXT_GSM0		BIT(2)
+#define SUNXI_LOSC_CTRL_SRC_SEL			BIT(0)
+
 #define SUNXI_RTC_YMD				0x0004
 
 #define SUNXI_RTC_HMS				0x0008

@@ -438,6 +448,8 @@ static int sunxi_rtc_probe(struct platform_device *pdev)
 	struct resource *res;
 	int ret;
 
+	uint32_t loscctrl;
+
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;

@@ -468,6 +480,19 @@ static int sunxi_rtc_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	loscctrl = readl(chip->base + SUNXI_LOSC_CTRL);
+	loscctrl &= ~(SUNXI_LOSC_CTRL_AUTO_SWT_EN);
+	loscctrl |= (SUNXI_LOSC_CTRL_SRC_SEL | SUNXI_LOSC_CTRL_KEY);
+	loscctrl |= SUNXI_LOSC_CTRL_EXT_GSM1;
+	writel(loscctrl, chip->base + SUNXI_LOSC_CTRL);
+	udelay(100);
+
+	loscctrl = readl(chip->base + SUNXI_LOSC_CTRL);
+	if (!(loscctrl & SUNXI_LOSC_CTRL_SRC_SEL)) {
+		dev_err(&pdev->dev, "Error: Set LOSC to external failed.\n");
+		dev_err(&pdev->dev, "Warning: RTC time will be wrong!\n");
+	}
+
 	/* clear the alarm count value */
 	writel(0, chip->base + SUNXI_ALRM_DHMS);

Signature · 11 lines

2.7.4

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Maxime Ripard

2016-07-06 20:20 UTC #1 lore raw

Quoted 3 lines · Stephan Roslen

We noticed some serious drift problems in Allwinner A20 RTCs. I found out, that the oscillator source needs to be set to an external 32.768 KHz oscillator instead of the internal 32.000 KHz oscillator.

You should wrap the commit description.

Quoted 40 lines

Signed-off-by Stephan Roslen <[email protected]>

---
 drivers/rtc/rtc-sunxi.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

drivers/rtc/rtc-sunxi.c +12 -0

index abada60..001256e 100644
@@ -34,9 +34,19 @@
 #include <linux/types.h>
 
 #define SUNXI_LOSC_CTRL				0x0000
+
+/* magic number required to set bit 0 */
+#define SUNXI_LOSC_CTRL_KEY			0x16AA0000
+
+#define SUNXI_LOSC_CTRL_AUTO_SWT_EN		BIT(14)
+
 #define SUNXI_LOSC_CTRL_RTC_HMS_ACC		BIT(8)
 #define SUNXI_LOSC_CTRL_RTC_YMD_ACC		BIT(7)
 
+#define SUNXI_LOSC_CTRL_EXT_GSM1		BIT(3)
+#define SUNXI_LOSC_CTRL_EXT_GSM0		BIT(2)
+#define SUNXI_LOSC_CTRL_SRC_SEL			BIT(0)
+
 #define SUNXI_RTC_YMD				0x0004
 
 #define SUNXI_RTC_HMS				0x0008

@@ -438,6 +448,8 @@ static int sunxi_rtc_probe(struct platform_device *pdev)
 	struct resource *res;
 	int ret;
 
+	uint32_t loscctrl;
+
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;

drivers/rtc/rtc-sunxi.c static int sunxi_rtc_probe(struct platform_device *pdev) · line 488

Quoted 9 lines

drivers/rtc/rtc-sunxi.c +6 -0

@@ -468,6 +480,19 @@ static int sunxi_rtc_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	loscctrl = readl(chip->base + SUNXI_LOSC_CTRL);
+	loscctrl &= ~(SUNXI_LOSC_CTRL_AUTO_SWT_EN);
+	loscctrl |= (SUNXI_LOSC_CTRL_SRC_SEL | SUNXI_LOSC_CTRL_KEY);
+	loscctrl |= SUNXI_LOSC_CTRL_EXT_GSM1;
+	writel(loscctrl, chip->base + SUNXI_LOSC_CTRL);
+	udelay(100);

Why is that udelay needed?

drivers/rtc/rtc-sunxi.c static int sunxi_rtc_probe(struct platform_device *pdev) · line 495

Quoted 7 lines

drivers/rtc/rtc-sunxi.c +7 -0

+
+	loscctrl = readl(chip->base + SUNXI_LOSC_CTRL);
+	if (!(loscctrl & SUNXI_LOSC_CTRL_SRC_SEL)) {
+		dev_err(&pdev->dev, "Error: Set LOSC to external failed.\n");
+		dev_err(&pdev->dev, "Warning: RTC time will be wrong!\n");
+	}
+

This isn't needed

The issue is actually worse than that.

That register controls the losc source for the whole clock tree, so it will affect every clock in the system.

In order to have that correctly propagated, you should register a new mux here in the clock framework, and have all the other clocks using that mux as a parent.

That's going to be tricky, because the clocks usually probe way earlier than the RTC driver. So I'm guessing you could do a clock driver that maps the registers, register its clock, and then when the RTC probes just takes over what has been setup already by the clock driver. This also means removing the ability for the RTC to be compiled as a module.

Maxime

Signature · 13 lines

Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Stephan Roslen

author

2016-07-07 04:50 UTC #2 lore raw

Quoted 5 lines · Maxime Ripard

drivers/rtc/rtc-sunxi.c static int sunxi_rtc_probe(struct platform_device *pdev) · line 488
Quoted 2 lines · Stephan Roslen
drivers/rtc/rtc-sunxi.c +2 -0
+	writel(loscctrl, chip->base + SUNXI_LOSC_CTRL);
+	udelay(100);
=20
Why is that udelay needed?

I found that studying the sunxi 3.4 kernel code and considered it a necessa= ry delay for the hardware setup and update SUNXI_LOSC_CTRL_SRC_SEL bit. My = assumption was wrong, it seems. At least a few reboots show, that it is lik= ely to work without.

Quoted 11 lines

=20
drivers/rtc/rtc-sunxi.c static int sunxi_rtc_probe(struct platform_device *pdev) · line 495
Quoted 7 lines
drivers/rtc/rtc-sunxi.c +7 -0
+
+	loscctrl =3D readl(chip->base + SUNXI_LOSC_CTRL);
+	if (!(loscctrl & SUNXI_LOSC_CTRL_SRC_SEL)) {
+		dev_err(&pdev->dev, "Error: Set LOSC to external failed.\n");
+		dev_err(&pdev->dev, "Warning: RTC time will be wrong!\n");
+	}
+
=20
This isn't needed
=20

Quoted 8 lines

The issue is actually worse than that.
=20
That register controls the losc source for the whole clock tree, so it will affect every clock in the system.
=20
In order to have that correctly propagated, you should register a new mux here in the clock framework, and have all the other clocks using that mux as a parent.

I agree. Checking the diagram in subsection 1.5.2 of the A20 manual it seem= s, that LOSC can be a source for clocks like CPU and some SoC busses. So my=
patch could indeed mess with the whole clock tree.

Quoted 7 lines

=20
That's going to be tricky, because the clocks usually probe way earlier than the RTC driver. So I'm guessing you could do a clock driver that maps the registers, register its clock, and then when the RTC probes just takes over what has been setup already by the clock driver. This also means removing the ability for the RTC to be compiled as a module.

I agree again. Though I think, the RTC should still work as a module. The C= LK driver could provide the regmap MFD style and the RTC driver may access = it. Or rather the CLK driver should use an MFD, that is provided early, too= . Eventually even a syscon? Actually the whole bunch described in subsectio= n 1.9 (including timers, arlarms, rtc and even the watchdog) would have to = rely on that MFD. Do I miss a point?

Stephan

--=20
You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "= rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e= mail to [email protected].
For more options, visit https://groups.google.com/d/optout.

Chen-Yu Tsai

2016-07-07 04:53 UTC #3 lore raw

Quoted 8 lines · Stephan Roslen

Quoted 5 lines · Maxime Ripard
drivers/rtc/rtc-sunxi.c static int sunxi_rtc_probe(struct platform_device *pdev) · line 488
Quoted 2 lines · Stephan Roslen
drivers/rtc/rtc-sunxi.c +2 -0
+    writel(loscctrl, chip->base + SUNXI_LOSC_CTRL);
+    udelay(100);
Why is that udelay needed?
I found that studying the sunxi 3.4 kernel code and considered it a neces=

sary delay for the hardware setup and update SUNXI_LOSC_CTRL_SRC_SEL bit. M= y assumption was wrong, it seems. At least a few reboots show, that it is l= ikely to work without.

Quoted 6 lines

Quoted 6 lines
drivers/rtc/rtc-sunxi.c static int sunxi_rtc_probe(struct platform_device *pdev)
Quoted 6 lines
drivers/rtc/rtc-sunxi.c +4 -0
+
+    loscctrl =3D readl(chip->base + SUNXI_LOSC_CTRL);
+    if (!(loscctrl & SUNXI_LOSC_CTRL_SRC_SEL)) {
+            dev_err(&pdev->dev, "Error: Set LOSC to external failed.\n=

");

Quoted 8 lines

Quoted 8 lines
drivers/rtc/rtc-sunxi.c static int sunxi_rtc_probe(struct platform_device *pdev) · line 495
Quoted 3 lines
drivers/rtc/rtc-sunxi.c +3 -0
+            dev_err(&pdev->dev, "Warning: RTC time will be wrong!\n");
+    }
+
This isn't needed
The issue is actually worse than that.

Quoted 8 lines

Quoted 6 lines
That register controls the losc source for the whole clock tree, so it will affect every clock in the system.
In order to have that correctly propagated, you should register a new mux here in the clock framework, and have all the other clocks using that mux as a parent.
I agree. Checking the diagram in subsection 1.5.2 of the A20 manual it se=

ems, that LOSC can be a source for clocks like CPU and some SoC busses. So = my patch could indeed mess with the whole clock tree.

(Please wrap your email)

Within the current sunxi clk structure, the LOSC is registered as a fixed 32.768 KHz clk. So this patch actually makes things right.

ChenYu

Quoted 10 lines

Quoted 8 lines
That's going to be tricky, because the clocks usually probe way earlier than the RTC driver. So I'm guessing you could do a clock driver that maps the registers, register its clock, and then when the RTC probes just takes over what has been setup already by the clock driver. This also means removing the ability for the RTC to be compiled as a module.
I agree again. Though I think, the RTC should still work as a module. The=

CLK driver could provide the regmap MFD style and the RTC driver may acces=
s it. Or rather the CLK driver should use an MFD, that is provided early, t= oo. Eventually even a syscon? Actually the whole bunch described in subsect= ion 1.9 (including timers, arlarms, rtc and even the watchdog) would have t=
o rely on that MFD. Do I miss a point?

Quoted 2 lines

Stephan

Stephan Roslen

author

2016-07-07 06:12 UTC #4 lore raw

Quoted 4 lines · Chen-Yu Tsai

Quoted 1 line · Stephan Roslen
I agree. Checking the diagram in subsection 1.5.2 of the A20 manual it seems, that LOSC can be a source for clocks like CPU and some SoC busses. So my patch could indeed mess with the whole clock tree.
(Please wrap your email)

Sorry.

Quoted 3 lines

Within the current sunxi clk structure, the LOSC is registered as a fixed 32.768 KHz clk. So this patch actually makes things right.

In this case it should still be addressed somewhere in drivers/clk/sunxi, not in drivers/rtc, I guess? Would you prefer setting the external 32.768 KHz crystal as hard coded default instead of providing the muxer? It could be considered okay, for the data sheet seems to demand presence of the external oscillator and there is absolutely no use in a 32KHz +- 20% oscillator, if a 32.768KHz crystal is available.

Stephan

Signature · 8 lines

You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Maxime Ripard

2016-07-07 07:08 UTC #5 lore raw

Quoted 9 lines · Stephan Roslen

Quoted 2 lines
Within the current sunxi clk structure, the LOSC is registered as a fixed 32.768 KHz clk. So this patch actually makes things right.
In this case it should still be addressed somewhere in drivers/clk/sunxi, not in drivers/rtc, I guess? Would you prefer setting the external 32.768 KHz crystal as hard coded default instead of providing the muxer? It could be considered okay, for the data sheet seems to demand presence of the external oscillator and there is absolutely no use in a 32KHz +- 20% oscillator, if a 32.768KHz crystal is available.

Yes, poking into the registers in drivers/clk would also work.

Maxime

Signature · 13 lines

Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Maxime Ripard

2016-07-07 06:42 UTC #6 lore raw

In reply to Chen-Yu Tsai

Quoted 11 lines · Chen-Yu Tsai

Quoted 8 lines
Quoted 3 lines
In order to have that correctly propagated, you should register a new mux here in the clock framework, and have all the other clocks using that mux as a parent.
I agree. Checking the diagram in subsection 1.5.2 of the A20 manual it seems, that LOSC can be a source for clocks like CPU and some SoC busses. So my patch could indeed mess with the whole clock tree.
Within the current sunxi clk structure, the LOSC is registered as a fixed 32.768 KHz clk. So this patch actually makes things right.

On the condition that the driver gets loaded, which is not guaranteed.

Maxime

Signature · 13 lines

Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Stephan Roslen

author

2016-07-07 05:11 UTC #7 lore raw

In reply to Stephan Roslen

Quoted 11 lines · Stephan Roslen

Quoted 8 lines · Maxime Ripard
The issue is actually worse than that.
That register controls the losc source for the whole clock tree, so it will affect every clock in the system.
In order to have that correctly propagated, you should register a new mux here in the clock framework, and have all the other clocks using that mux as a parent.
=20
I agree. Checking the diagram in subsection 1.5.2 of the A20 manual it se=

ems, that LOSC can be a source for clocks like CPU and some SoC busses. So = my patch could indeed mess with the whole clock tree.

Sorry for the addition. The mess of course is not that huge, for the mux se= lects between a 32KHz Oscillator with +- 20% tolerance, thus ranging from 2= 5.6 to 38.4 KHz and the 32.768KHz external oscillator. Hence muxing to 32.7= 68KHz should be no problem for any clk, that was fine with the internal osc= illator as clk source. Though still you're absolutely right, that this shou= ld be handled in code correctly.

Stephan