[PATCH 0/3] Add basic support for RTC on Allwinner H6 SoC

[meg...@megous.com]

From: Ondrej Jirman <meg...@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.

There's a small detail here, that's not described absolutely correctly in
DTSI, but the difference is not really that material. ext_osc32k is
originally modelled as a fixed clock that feeds into RTC module, but in
reality it's the RTC module that implements via its registers enabling and
disabling of this oscillator/clock.

Though:
- there's no other possible user of ext_osc32k than RTC module
- there's no other possible external configuration for the crystal
circuit that would need to be handled in the dts per board

So I guess, while the description is not perfect, this patch series still
improves the current situation. Or maybe I'm misunderstanding something,
and &ext_osc32k node just describes a fact that there's a crystal on
the board. Then, everything is perhaps fine. :)

For now, the enable bit for this oscillator is toggled by the re-parenting
code automatically, as needed.

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

Please take a look.

Thank you and regards,
Ondrej Jirman

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

.../devicetree/bindings/rtc/sun6i-rtc.txt | 1 +
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 30 +++++++-------
drivers/rtc/rtc-sun6i.c | 40 ++++++++++++++++++-
3 files changed, 55 insertions(+), 16 deletions(-)

--
2.21.0

[meg...@megous.com]

From: Ondrej Jirman <meg...@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 <meg...@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 e0dc4a05c1ba..ba0f45b6f743 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>;
@@ -215,7 +207,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>;
@@ -603,10 +595,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>;
@@ -627,7 +629,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.21.0

[meg...@megous.com]

From: Ondrej Jirman <meg...@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 <meg...@megous.com>
---

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 11f56de52179..7375a530c565 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -41,9 +41,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)

@@ -137,6 +139,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 {
@@ -199,6 +203,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);

@@ -224,6 +232,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)
@@ -244,9 +253,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;
@@ -354,6 +372,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,
@@ -683,6 +718,7 @@ static const struct of_device_id sun6i_rtc_dt_ids[] = {
{ .compatible = "allwinner,sun8i-h3-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.21.0

[meg...@megous.com]

From: Ondrej Jirman <meg...@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 <meg...@megous.com>
---

Documentation/devicetree/bindings/rtc/sun6i-rtc.txt | 1 +
1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt b/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt
index 6b732c41392b..291505b84e7b 100644
--- a/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/sun6i-rtc.txt
@@ -11,6 +11,7 @@ Required properties:
- "allwinner,sun8i-v3-rtc"
- "allwinner,sun50i-a64-rtc", "allwinner,sun8i-h3-rtc"
- "allwinner,sun50i-h5-rtc"
+ - "allwinner,sun50i-h6-rtc"

Where there are two or more compatible strings, this
denotes the hardware covered by the most specific one
--
2.21.0

[Chen-Yu Tsai]

On Fri, Apr 12, 2019 at 8:07 PM megous via linux-sunxi
<linux...@googlegroups.com> wrote:
>
> From: Ondrej Jirman <meg...@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.

Please also add DCXO clock input/output and XO clock input to the bindings
and DT, and also fix up the clock tree. You can skip them in the driver for
now, but please add a TODO. As long as you don't change the clock-output-name
of osc24M, everything should work as before.

We just want the DT to describe what is actually there. For the XO input,
you could just directly reference the external crystal node. The gate for
it is likely somewhere in the PRCM block, which we don't have docs for.

> There's a small detail here, that's not described absolutely correctly in
> DTSI, but the difference is not really that material. ext_osc32k is
> originally modelled as a fixed clock that feeds into RTC module, but in
> reality it's the RTC module that implements via its registers enabling and
> disabling of this oscillator/clock.
>
> Though:
> - there's no other possible user of ext_osc32k than RTC module
> - there's no other possible external configuration for the crystal
> circuit that would need to be handled in the dts per board
>
> So I guess, while the description is not perfect, this patch series still
> improves the current situation. Or maybe I'm misunderstanding something,
> and &ext_osc32k node just describes a fact that there's a crystal on
> the board. Then, everything is perhaps fine. :)

Correct. The external clock nodes are modeling the crystal, not the internal
clock gate / distributor.

Were the vendor to not include the crystal (for whatever reasons), the DT
should be able to describe it via the absence of the clock input, and the
driver should correctly use the internal (inaccurate) oscillator. I realize
the clocks property is required, and the driver doesn't handle this case
either, so we might have to fix that if it were to appear in the wild.

> For now, the enable bit for this oscillator is toggled by the re-parenting
> code automatically, as needed.

That's fine. No need to increase the clock tree depth.

ChenYu

> This patchset is necessary for implementing the WiFi/Bluetooth support
> on boards using H6 SoC.
>
> Please take a look.
>
> Thank you and regards,
> Ondrej Jirman
>
> 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
>
> .../devicetree/bindings/rtc/sun6i-rtc.txt | 1 +
> arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 30 +++++++-------
> drivers/rtc/rtc-sun6i.c | 40 ++++++++++++++++++-
> 3 files changed, 55 insertions(+), 16 deletions(-)
>
> --
> 2.21.0
>

> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Ondřej Jirman]

Hi ChenYu,

On Mon, Apr 15, 2019 at 04:18:12PM +0800, Chen-Yu Tsai wrote:
> On Fri, Apr 12, 2019 at 8:07 PM megous via linux-sunxi
> <linux...@googlegroups.com> wrote:
> >
> > From: Ondrej Jirman <meg...@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.
>
> Please also add DCXO clock input/output and XO clock input to the bindings
> and DT, and also fix up the clock tree. You can skip them in the driver for
> now, but please add a TODO. As long as you don't change the clock-output-name
> of osc24M, everything should work as before.

That's a bit confusing. There's no clock-output-name for osc24M, nor for input
clock used in the dt-bindings or the driver. Perhaps you meant osc32k? Maybe
I'm misunderstanding something?

If you look at the datasheet page 349, it looks like RTC provides "hosc"
clock (to plls and the system) either from XO or DCXO oscillators.
The default selection depends on the voltage level on external PAD.

So based on what you wrote, I suggest these actual changes/names:

1) Add DT docs for HOSC clock provided at index 3:

- 3: HOSC, 24MHz clock that clocks the PLLs and most of the SoC (H6 only)

2) Add bindings description for "osc24M-dc", "osc24M-m" input clocks in
addition to existing support for "osc32k". Name "osc24M-m" is based on
X24MIN/MOUT pins and datasheet's "clk_24mxo" name.

3) The RTC driver would now just registers a fixed HOSC clock with a name
gathered from the clock-output-names index 3 (if enabled by the new
export_hosc flag - only enabled on H6).

The driver would ignore the "osc24M-dc", "osc24M-m" input clocks. Or perhaps
it could just support a case where only one of these are used and make it the
only parent of the HOSC clock?

HOSC default source selection is done based on external PAD setup, and
there's no need for runtime access/selection of HOSC source at the moment.

4) In the future the RTC driver would be extended to support more refined
setup/muxing/runtime selection of osc24M-dc/osc24M-m. PRCM driver would
provide the osc24M-m clock, to be able for kernel to know how to gate it.

The board's DTSI would have to link either "osc24M-dc", "osc24M-m" to nodes
describing an external crystal (or to PRCM clock in the future). It's a boards
choice on what crystals are actually used. 3 configs are possible - with one or
two crystals, connected to either one of XIN/XOUT X24MIN/X24MOUT pins or both.

Would that work?

DT would still probably need a re-work in the future, if the PRCM clock
modeling the gate would be needed.

regards,
o.

[Maxime Ripard]

On Mon, Apr 15, 2019 at 04:22:22PM +0200, Ondřej Jirman wrote:
> DT would still probably need a re-work in the future, if the PRCM clock
> modeling the gate would be needed.

Just reacting to that bit.

I know we're pretty bad at this, and the documentation doesn't make it
any easier, but if you have any idea of how it should be modelled
next, then do that.

There's no reason to push it to a later time, it makes everyone's life
harder.

Maxime

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

[Chen-Yu Tsai]

On Mon, Apr 15, 2019 at 10:22 PM 'Ondřej Jirman' via linux-sunxi
<linux...@googlegroups.com> wrote:
>
> Hi ChenYu,
>
> On Mon, Apr 15, 2019 at 04:18:12PM +0800, Chen-Yu Tsai wrote:
> > On Fri, Apr 12, 2019 at 8:07 PM megous via linux-sunxi
> > <linux...@googlegroups.com> wrote:
> > >
> > > From: Ondrej Jirman <meg...@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.
> >
> > Please also add DCXO clock input/output and XO clock input to the bindings
> > and DT, and also fix up the clock tree. You can skip them in the driver for
> > now, but please add a TODO. As long as you don't change the clock-output-name
> > of osc24M, everything should work as before.
>
> That's a bit confusing. There's no clock-output-name for osc24M, nor for input
> clock used in the dt-bindings or the driver. Perhaps you meant osc32k? Maybe
> I'm misunderstanding something?

I meant the clock-output-names in the device node of the external 24M crystal.

> If you look at the datasheet page 349, it looks like RTC provides "hosc"
> clock (to plls and the system) either from XO or DCXO oscillators.
> The default selection depends on the voltage level on external PAD.
>
> So based on what you wrote, I suggest these actual changes/names:
>
> 1) Add DT docs for HOSC clock provided at index 3:
>
> - 3: HOSC, 24MHz clock that clocks the PLLs and most of the SoC (H6 only)

Correct.

> 2) Add bindings description for "osc24M-dc", "osc24M-m" input clocks in
> addition to existing support for "osc32k". Name "osc24M-m" is based on
> X24MIN/MOUT pins and datasheet's "clk_24mxo" name.
>
> 3) The RTC driver would now just registers a fixed HOSC clock with a name
> gathered from the clock-output-names index 3 (if enabled by the new
> export_hosc flag - only enabled on H6).

You don't need to do this part yet. Since the CCU drivers are hard-wired
(suprise) to use the global clock name "osc24M" as hosc source. The DT
references are only for show ATM, so it doesn't matter if you implement
the clocks in the RTC driver.

However we want the DT to be correct, so that when we do get around to
doing it, we won't have to update the DT again.

It's up to you though. If you want to implement basic support, that's
fine by me. However you won't be able to test it without hacking the
CCU driver.

After describing this, it seems that when we get to doing the clk parent
rework, we'll be in a bad situation if we don't get rtc changes in before
the CCU changes.

> The driver would ignore the "osc24M-dc", "osc24M-m" input clocks. Or perhaps
> it could just support a case where only one of these are used and make it the
> only parent of the HOSC clock?

They should just be DCXO and XO, based on the diagram. The names are local
to the RTC, so they don't need to be globally unique. Whatever matches the
datasheet is best.

> HOSC default source selection is done based on external PAD setup, and
> there's no need for runtime access/selection of HOSC source at the moment.

Is it even possible to change it?

> 4) In the future the RTC driver would be extended to support more refined
> setup/muxing/runtime selection of osc24M-dc/osc24M-m. PRCM driver would
> provide the osc24M-m clock, to be able for kernel to know how to gate it.
>
> The board's DTSI would have to link either "osc24M-dc", "osc24M-m" to nodes
> describing an external crystal (or to PRCM clock in the future). It's a boards
> choice on what crystals are actually used. 3 configs are possible - with one or
> two crystals, connected to either one of XIN/XOUT X24MIN/X24MOUT pins or both.

AFAIK, osc24M-dc would link directly to the external crystal, while osc24M-m
would link to the external crystal first, then PRCM if it gets implemented.

> Would that work?
>
> DT would still probably need a re-work in the future, if the PRCM clock
> modeling the gate would be needed.

Yeah. We'll deal with that when we get to it.


To summarize, the goal is to get the DT right the first time.

Regards
ChenYu

[Chen-Yu Tsai]

On Mon, Apr 15, 2019 at 10:33 PM Maxime Ripard
<maxime...@bootlin.com> wrote:
>
> On Mon, Apr 15, 2019 at 04:22:22PM +0200, Ondřej Jirman wrote:
> > DT would still probably need a re-work in the future, if the PRCM clock
> > modeling the gate would be needed.
>
> Just reacting to that bit.
>
> I know we're pretty bad at this, and the documentation doesn't make it
> any easier, but if you have any idea of how it should be modelled
> next, then do that.

It seems to be just a clock gate, judging by previous (A80/A83T) SoCs.
However since we lack documents for the PRCM, we might need to poke
around to verify that it's actually there.

Or maybe we could ask Allwinner very specific questions, like:

- Does the PRCM have a gate controlling XO?
- Is it bit 2 in register 0x44 in the PRCM?

ChenYu

> There's no reason to push it to a later time, it makes everyone's life
> harder.
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>

[Ondřej Jirman]

Ah, so that's how it works. And that's what "clock parent rework" refers
to! :)

> However we want the DT to be correct, so that when we do get around to
> doing it, we won't have to update the DT again.

Ok, so I'll try to come up with a new set of patches, and we'll see if
I'll get the description right.

> It's up to you though. If you want to implement basic support, that's
> fine by me. However you won't be able to test it without hacking the
> CCU driver.
>
> After describing this, it seems that when we get to doing the clk parent
> rework, we'll be in a bad situation if we don't get rtc changes in before
> the CCU changes.

Yeah.

> > The driver would ignore the "osc24M-dc", "osc24M-m" input clocks. Or perhaps
> > it could just support a case where only one of these are used and make it the
> > only parent of the HOSC clock?
>
> They should just be DCXO and XO, based on the diagram. The names are local
> to the RTC, so they don't need to be globally unique. Whatever matches the
> datasheet is best.

Ok.

> > HOSC default source selection is done based on external PAD setup, and
> > there's no need for runtime access/selection of HOSC source at the moment.
>
> Is it even possible to change it?

Hmm. Looks like the answer is no:

DCXO_CTRL_REG:
- OSC_CLK_SRC_SEL bit:
(Pad select)
Read/Write column contains 'U' not (R/W)

> > 4) In the future the RTC driver would be extended to support more refined
> > setup/muxing/runtime selection of osc24M-dc/osc24M-m. PRCM driver would
> > provide the osc24M-m clock, to be able for kernel to know how to gate it.
> >
> > The board's DTSI would have to link either "osc24M-dc", "osc24M-m" to nodes
> > describing an external crystal (or to PRCM clock in the future). It's a boards
> > choice on what crystals are actually used. 3 configs are possible - with one or
> > two crystals, connected to either one of XIN/XOUT X24MIN/X24MOUT pins or both.
>
> AFAIK, osc24M-dc would link directly to the external crystal, while osc24M-m
> would link to the external crystal first, then PRCM if it gets implemented.

Ok.

thank you,
o.

[Chen-Yu Tsai]

On Fri, Apr 12, 2019 at 8:07 PM megous via linux-sunxi
<linux...@googlegroups.com> wrote:
>

Manual says bit 14? Or is this different from LOSC_AUTO_SWT_EN?

The rest looks ok.

ChenYu

[Chen-Yu Tsai]

On Fri, Apr 12, 2019 at 8:07 PM megous via linux-sunxi
<linux...@googlegroups.com> wrote:
>

> From: Ondrej Jirman <meg...@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 <meg...@megous.com>

Acked-by: Chen-Yu Tsai <we...@csie.org>

[Ondřej Jirman]

Yes, see H6 BSP:

drivers/rtc/rtc-sunxi.h

20 #define REG_CLK32K_AUTO_SWT_EN BIT(14)
21 #define REG_CLK32K_AUTO_SWT_BYPASS BIT(15)

regards,
Ondrej

> ChenYu

[Ondřej Jirman]

On Mon, Aug 05, 2019 at 06:16:14PM +0800, Chen-Yu Tsai wrote:

To give you more information. This is a new thing in H6 BSP, compared
to BSPs for previous SoCs (H5/H3).

20 #define REG_CLK32K_AUTO_SWT_EN BIT(14)
21 #define REG_CLK32K_AUTO_SWT_BYPASS BIT(15)

Init sequence changed in H6 BSP to:

646 /*
647 * Step1: select RTC clock source
648 */
649 tmp_data = readl(chip->base + SUNXI_LOSC_CTRL);
650 tmp_data &= (~REG_CLK32K_AUTO_SWT_EN);
651
652 /* Disable auto switch function */
653 tmp_data |= REG_CLK32K_AUTO_SWT_BYPASS;
654 writel(tmp_data, chip->base + SUNXI_LOSC_CTRL);
655
656 tmp_data = readl(chip->base + SUNXI_LOSC_CTRL);
657 tmp_data |= (RTC_SOURCE_EXTERNAL | REG_LOSCCTRL_MAGIC);
658 writel(tmp_data, chip->base + SUNXI_LOSC_CTRL);
659
660 /* We need to set GSM after change clock source */
661 udelay(10);
662 tmp_data = readl(chip->base + SUNXI_LOSC_CTRL);
663 tmp_data |= (EXT_LOSC_GSM | REG_LOSCCTRL_MAGIC);
664 writel(tmp_data, chip->base + SUNXI_LOSC_CTRL);
665

For older BSPs, the init sequence looked like this:

482 /*
483 * Step1: select RTC clock source
484 */
485 tmp_data = sunxi_rtc_read(SUNXI_LOSC_CTRL_REG);
486 tmp_data &= (~REG_CLK32K_AUTO_SWT_EN);
487 tmp_data |= (RTC_SOURCE_EXTERNAL | REG_LOSCCTRL_MAGIC);
488 tmp_data |= (EXT_LOSC_GSM);
489 sunxi_rtc_write(tmp_data, SUNXI_LOSC_CTRL_REG);
490

EXT_LOSC_GSM has values 4 values from low to high, and I guess it configures
gain for the oscillator's amplifier in the feedback loop of the circuit.

So the new code, for some reason changed from single write to sequence
of individual writes/config steps:

1) disable auto-switch and enable auto-switch bypass
2) select RTC clock source (to LOSC)
(wait)
3) configure gain on the LOSC

regards,
o.

> ChenYu

[Chen-Yu Tsai]

I don't have this in my H6 BSPs. One is H6 Lichee v1.1 downloaded from Pine64.
The link was from linux-sunxi wiki's H6 page.

The other is a 4.9 kernel tree, which I believe is from Allwinner's github:

https://github.com/Allwinner-Homlet/H6-BSP4.9-linux

> For older BSPs, the init sequence looked like this:
>
> 482 /*
> 483 * Step1: select RTC clock source
> 484 */
> 485 tmp_data = sunxi_rtc_read(SUNXI_LOSC_CTRL_REG);
> 486 tmp_data &= (~REG_CLK32K_AUTO_SWT_EN);
> 487 tmp_data |= (RTC_SOURCE_EXTERNAL | REG_LOSCCTRL_MAGIC);
> 488 tmp_data |= (EXT_LOSC_GSM);
> 489 sunxi_rtc_write(tmp_data, SUNXI_LOSC_CTRL_REG);
> 490
>
> EXT_LOSC_GSM has values 4 values from low to high, and I guess it configures
> gain for the oscillator's amplifier in the feedback loop of the circuit.
>
> So the new code, for some reason changed from single write to sequence
> of individual writes/config steps:
>
> 1) disable auto-switch and enable auto-switch bypass
> 2) select RTC clock source (to LOSC)
> (wait)

Maybe it's possible to glitch if these two are combined?

> 3) configure gain on the LOSC
>
> regards,
> o.
>
> > ChenYu
>

> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi...@googlegroups.com.

> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190805104529.z3mex3m2tss7lzlr%40core.my.home.

[Ondřej Jirman]

Interesting. :) I have the BSP I was using saved here:

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

It's based of 4.9.119

https://megous.com/git/linux/log/?h=h6-4.9-bsp

I don't remember where I found it. But I imported it fairly recently, and
the code you pointed to looks like an older version that I can found in some
beta H6 BSP, that I imported way earlier and is based on 4.9.56:

https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=linux-h6
https://megous.com/git/linux/log/?h=linux-h6

Hmm, archeology. :)

> > For older BSPs, the init sequence looked like this:
> >
> > 482 /*
> > 483 * Step1: select RTC clock source
> > 484 */
> > 485 tmp_data = sunxi_rtc_read(SUNXI_LOSC_CTRL_REG);
> > 486 tmp_data &= (~REG_CLK32K_AUTO_SWT_EN);
> > 487 tmp_data |= (RTC_SOURCE_EXTERNAL | REG_LOSCCTRL_MAGIC);
> > 488 tmp_data |= (EXT_LOSC_GSM);
> > 489 sunxi_rtc_write(tmp_data, SUNXI_LOSC_CTRL_REG);
> > 490
> >
> > EXT_LOSC_GSM has values 4 values from low to high, and I guess it configures
> > gain for the oscillator's amplifier in the feedback loop of the circuit.
> >
> > So the new code, for some reason changed from single write to sequence
> > of individual writes/config steps:
> >
> > 1) disable auto-switch and enable auto-switch bypass
> > 2) select RTC clock source (to LOSC)
> > (wait)
>
> Maybe it's possible to glitch if these two are combined?

That's what I thought too. Or the programmer thought so, and was just
programming defensively, and there's no real problem in the practice.

But that specific delay() seems like someone trying to solve a real issue. Of
course there's no knowing if it was on H6 or on some other SoC.

regards,
o.

[Chen-Yu Tsai]

That's good enough for me. I suppose if we do have any more doubts we could
ask them directly.


Reviewed-by: Chen-Yu Tsai <we...@csie.org>

> > > For older BSPs, the init sequence looked like this:
> > >
> > > 482 /*
> > > 483 * Step1: select RTC clock source
> > > 484 */
> > > 485 tmp_data = sunxi_rtc_read(SUNXI_LOSC_CTRL_REG);
> > > 486 tmp_data &= (~REG_CLK32K_AUTO_SWT_EN);
> > > 487 tmp_data |= (RTC_SOURCE_EXTERNAL | REG_LOSCCTRL_MAGIC);
> > > 488 tmp_data |= (EXT_LOSC_GSM);
> > > 489 sunxi_rtc_write(tmp_data, SUNXI_LOSC_CTRL_REG);
> > > 490
> > >
> > > EXT_LOSC_GSM has values 4 values from low to high, and I guess it configures
> > > gain for the oscillator's amplifier in the feedback loop of the circuit.
> > >
> > > So the new code, for some reason changed from single write to sequence
> > > of individual writes/config steps:
> > >
> > > 1) disable auto-switch and enable auto-switch bypass
> > > 2) select RTC clock source (to LOSC)
> > > (wait)
> >
> > Maybe it's possible to glitch if these two are combined?
>
> That's what I thought too. Or the programmer thought so, and was just
> programming defensively, and there's no real problem in the practice.
>
> But that specific delay() seems like someone trying to solve a real issue. Of
> course there's no knowing if it was on H6 or on some other SoC.

It's probably for the clock waveform to stabilize. Why they do it _after_
switching to the clock is weird though.

> regards,
> o.
>
> >
> > > 3) configure gain on the LOSC
> > >
> > > regards,
> > > o.
> > >
> > > > ChenYu
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi...@googlegroups.com.
> > > To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190805104529.z3mex3m2tss7lzlr%40core.my.home.
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi...@googlegroups.com.

> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190805111037.76vmanzcurffpbdf%40core.my.home.

[Clément Péron]

Hi,

On Mon, 5 Aug 2019 at 13:10, Ondřej Jirman <meg...@megous.com> wrote:
>
> On Mon, Aug 05, 2019 at 06:54:17PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Aug 5, 2019 at 6:45 PM Ondřej Jirman <meg...@megous.com> wrote:
> > >

[snip]

>
> Interesting. :) I have the BSP I was using saved here:
>
> https://megous.com/git/linux/tree/drivers/rtc/rtc-sunxi.c?h=h6-4.9-bsp#n649
>
> It's based of 4.9.119
>
> https://megous.com/git/linux/log/?h=h6-4.9-bsp
>
> I don't remember where I found it. But I imported it fairly recently, and
> the code you pointed to looks like an older version that I can found in some
> beta H6 BSP, that I imported way earlier and is based on 4.9.56:

The last recent that I know is from OrangePi H6 but it's based on 4.9.118.

https://github.com/orangepi-xunlong/OrangePiH6_Linux4_9

Regards,
Clément

> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190805111037.76vmanzcurffpbdf%40core.my.home.

[Ondřej Jirman]

On Mon, Apr 15, 2019 at 04:18:12PM +0800, Chen-Yu Tsai wrote:

> On Fri, Apr 12, 2019 at 8:07 PM megous via linux-sunxi
> <linux...@googlegroups.com> wrote:
> >
> > From: Ondrej Jirman <meg...@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.
>
> Please also add DCXO clock input/output and XO clock input to the bindings
> and DT, and also fix up the clock tree. You can skip them in the driver for
> now, but please add a TODO. As long as you don't change the clock-output-name
> of osc24M, everything should work as before.
>
> We just want the DT to describe what is actually there. For the XO input,
> you could just directly reference the external crystal node. The gate for
> it is likely somewhere in the PRCM block, which we don't have docs for.

So I was thinking about this for a while, and came up with this:

----------------- arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi -----------------
index 64c39f663d22..ac99ddbebe5c 100644
@@ -627,14 +635,15 @@

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-output-names = "osc32k", "osc32k-out", "iosc", "hosc";
+ clock-names = "losc", "dcxo";
+ clocks = <&ext_osc32k>, <&osc24M>;
#clock-cells = <1>;
};

r_ccu: clock@7010000 {
compatible = "allwinner,sun50i-h6-r-ccu";
reg = <0x07010000 0x400>;

I'm not completely sure how (or why?) to describe in DTSI which oscillator the
designer used (XO vs DCXO). This information is signalled by the pad voltage and
can be determined at runtime from DCXO_CTRL_REG's OSC_CLK_SRC_SEL (bit 3). It's
not possible to change at runtime.

HOSC source selection is only material to the CPUS (ARISC) firmware when it
wants to turn off all PLLs and the main crystal oscillator so that it knows
which one to turn off. I don't see any other use for it. It's just
informational. I don't think (future) crust firmware has space to be reading
DTBs, so the detection will be using OSC_CLK_SRC_SEL anyway.

Maybe whether XO or DCXO is used also matters if you want to do some fine
tunning of DCXO (control register has pletny of options), but that's probably
better done in u-boot. And there's still no need to read HOSC source from DT.
The driver can just check compatible, and if it is H6 and OSC_CLK_SRC_SEL is 1,
it can do it's DCXO tunning, or whatever. But neither OS nor bootloader will
be using this info to gate/disable the osciallator.

If we really want this in DT, maybe we can model it by having just two input
clocks to RTC described in DTSI, and the DTSI for H6 would have this by default:

clock-names = "losc", "dcxo";
clocks = <&ext_osc32k>, <&osc24M>;

And the board designer could change it from a board file, like this:

&rtc {
clock-names = "losc", "xo";
clocks = <&ext_osc32k>, <&osc24M>;
};

The driver could decide which oscillator is used by the presence of either
dcxo or xo input clock.

But in any case, the driver can also get this info from DCXO_CTRL_REG's
OSC_CLK_SRC_SEL, so it doesn't need to read this from DT at all. So it's a bit
pointless.

So I see two options:

1) skip adding dcxo/xo to input clocks of RTC completely
2) the above

What do you think?

regards,
o.

> _______________________________________________
> linux-arm-kernel mailing list
> linux-ar...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[Ondřej Jirman]

I tried option 2) and it feels pointless. It just creates this clock tree:

osc24M
hosc
plls...

from:

osc24M
plls...

and doesn't achieve anything else, other than complicating things, for no reason
because no driver will ever need or use this info from the DT.

So my preference is for keeping it simple and going with option 1).

regards,
o.

[Alexandre Belloni]

Hi,

On 06/08/2019 20:30:45+0200, Ondřej Jirman wrote:
> Maybe whether XO or DCXO is used also matters if you want to do some fine
> tunning of DCXO (control register has pletny of options), but that's probably
> better done in u-boot. And there's still no need to read HOSC source from DT.
> The driver can just check compatible, and if it is H6 and OSC_CLK_SRC_SEL is 1,
> it can do it's DCXO tunning, or whatever. But neither OS nor bootloader will
> be using this info to gate/disable the osciallator.
>

It is actually useful to be able to tweak the crystal tuning at
runtime to be able to reduce clock drift and compare with a reliable
source (e.g. NTP).
I'm curious, what kind of options does this RTC have?

--
Alexandre Belloni, Bootlin

[Chen-Yu Tsai]

On Wed, Aug 7, 2019 at 6:55 PM Alexandre Belloni
<alexandr...@bootlin.com> wrote:
>
> Hi,
>
> On 06/08/2019 20:30:45+0200, Ondřej Jirman wrote:
> > Maybe whether XO or DCXO is used also matters if you want to do some fine
> > tunning of DCXO (control register has pletny of options), but that's probably
> > better done in u-boot. And there's still no need to read HOSC source from DT.
> > The driver can just check compatible, and if it is H6 and OSC_CLK_SRC_SEL is 1,
> > it can do it's DCXO tunning, or whatever. But neither OS nor bootloader will
> > be using this info to gate/disable the osciallator.
> >
>
> It is actually useful to be able to tweak the crystal tuning at
> runtime to be able to reduce clock drift and compare with a reliable
> source (e.g. NTP).
> I'm curious, what kind of options does this RTC have?

It has options to set the current, trim cap value, band gap voltage, and also
change the mode to just accept an external clock signal, instead of driving
a crystal. The settings for the former parameters are not explained though.

See page 364 of
http://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf

ChenYu

[Ondřej Jirman]

On Wed, Aug 07, 2019 at 12:55:02PM +0200, Alexandre Belloni wrote:
> Hi,
>
> On 06/08/2019 20:30:45+0200, Ondřej Jirman wrote:
> > Maybe whether XO or DCXO is used also matters if you want to do some fine
> > tunning of DCXO (control register has pletny of options), but that's probably
> > better done in u-boot. And there's still no need to read HOSC source from DT.
> > The driver can just check compatible, and if it is H6 and OSC_CLK_SRC_SEL is 1,
> > it can do it's DCXO tunning, or whatever. But neither OS nor bootloader will
> > be using this info to gate/disable the osciallator.
> >
>
> It is actually useful to be able to tweak the crystal tuning at
> runtime to be able to reduce clock drift and compare with a reliable
> source (e.g. NTP).

I don't think there's a Linux kernel API that you can use to achieve that, so
that's a rather theoretical concern at the moment.

Also there are multiple clocks, that can drive the RTC, and you usually don't
drive it from 24MHz DCXO oscillator. The reason is that you'd have to deal with
the fact that the clock for RTC then becomes 24000000/750 (750 is fixed
divider), which is 32000.

So if you want to get 32768Hz for RTC by tuning the DCXO, it would have to have
24 576 000 Hz. And even if you could achieve that (doubtful), it would throw off
timings in the rest of the system (say UART, USB, CPU, display ctl) in a major way.

I guess you can try tuning 24MHz oscillator so that it's closer to the
real-world 24MHz via NTP reference for other reasons. But it would be
complicated, and require precise interaction with other components, like using
HW timers sourced from 24MHz HOSC clock, because you can't use CPU's timers,
because of inaccuracies introduced during DVFS, for example.

regards,
o.

> I'm curious, what kind of options does this RTC have?
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>

[Alexandre Belloni]

On 08/08/2019 14:12:37+0200, Ondřej Jirman wrote:
> On Wed, Aug 07, 2019 at 12:55:02PM +0200, Alexandre Belloni wrote:
> > Hi,
> >
> > On 06/08/2019 20:30:45+0200, Ondřej Jirman wrote:
> > > Maybe whether XO or DCXO is used also matters if you want to do some fine
> > > tunning of DCXO (control register has pletny of options), but that's probably
> > > better done in u-boot. And there's still no need to read HOSC source from DT.
> > > The driver can just check compatible, and if it is H6 and OSC_CLK_SRC_SEL is 1,
> > > it can do it's DCXO tunning, or whatever. But neither OS nor bootloader will
> > > be using this info to gate/disable the osciallator.
> > >
> >
> > It is actually useful to be able to tweak the crystal tuning at
> > runtime to be able to reduce clock drift and compare with a reliable
> > source (e.g. NTP).
>
> I don't think there's a Linux kernel API that you can use to achieve that, so
> that's a rather theoretical concern at the moment.
>

There is /sys/class/rtc/rtcX/offset which is even properly documented.

The reason I asked is that some RTCs have both analog (changing the
oscillator capacitance) and digital (changing the RTC counter) so I'm
wondering whether this interface should be extended.

[Ondřej Jirman]

On Fri, Aug 09, 2019 at 01:39:30AM +0200, Alexandre Belloni wrote:
> On 08/08/2019 14:12:37+0200, Ondřej Jirman wrote:
> > On Wed, Aug 07, 2019 at 12:55:02PM +0200, Alexandre Belloni wrote:
> > > Hi,
> > >
> > > On 06/08/2019 20:30:45+0200, Ondřej Jirman wrote:
> > > > Maybe whether XO or DCXO is used also matters if you want to do some fine
> > > > tunning of DCXO (control register has pletny of options), but that's probably
> > > > better done in u-boot. And there's still no need to read HOSC source from DT.
> > > > The driver can just check compatible, and if it is H6 and OSC_CLK_SRC_SEL is 1,
> > > > it can do it's DCXO tunning, or whatever. But neither OS nor bootloader will
> > > > be using this info to gate/disable the osciallator.
> > > >
> > >
> > > It is actually useful to be able to tweak the crystal tuning at
> > > runtime to be able to reduce clock drift and compare with a reliable
> > > source (e.g. NTP).
> >
> > I don't think there's a Linux kernel API that you can use to achieve that, so
> > that's a rather theoretical concern at the moment.
> >
>
> There is /sys/class/rtc/rtcX/offset which is even properly documented.
>
> The reason I asked is that some RTCs have both analog (changing the
> oscillator capacitance) and digital (changing the RTC counter) so I'm
> wondering whether this interface should be extended.

As I wrote below, that can't be achieved by tuning DCXO.