Re: [PATCH 1/2] rtc: sun6i: Make external 32k oscillator optional

Hi Maxime,

Dne petek, 14. februar 2020 ob 09:14:43 CET je Maxime Ripard napisal(a):

Hi Jernej,

Thanks for taking care of this

On Thu, Feb 13, 2020 at 10:14:26PM +0100, Jernej Skrabec wrote:

quoted

Some boards, like OrangePi PC2 (H5), OrangePi Plus 2E (H3) and Tanix TX6
(H6) don't have external 32kHz oscillator. Till H6, it didn't really
matter if external oscillator was enabled because HW detected error and
fall back to internal one. H6 has same functionality but it's the first
SoC which have "auto switch bypass" bit documented and always enabled in
driver. This prevents RTC to work correctly if external crystal is not
present on board. There are other side effects - all peripherals which
depends on this clock also don't work (HDMI CEC for example).

Make clocks property optional. If it is present, select external
oscillator. If not, stay on internal.

Signed-off-by: Jernej Skrabec <redacted>
---

 drivers/rtc/rtc-sun6i.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 852f5f3b3592..538cf7e19034 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c

@@ -250,19 +250,17 @@ static void __init sun6i_rtc_clk_init(struct

device_node *node,> 
 		writel(reg, rtc->base + SUN6I_LOSC_CTRL);
 	
 	}

-	/* Switch to the external, more precise, oscillator */
-	reg |= SUN6I_LOSC_CTRL_EXT_OSC;
-	if (rtc->data->has_losc_en)
-		reg |= SUN6I_LOSC_CTRL_EXT_LOSC_EN;
+	/* Switch to the external, more precise, oscillator, if present */
+	if (of_get_property(node, "clocks", NULL)) {
+		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;

-	/* Deal with old DTs */
-	if (!of_get_property(node, "clocks", NULL))
-		goto err;
-

Doesn't that prevent the parents to be properly set if there's an
external crystal?

No, why?

Check these two clk_summary:
http://ix.io/2bHY Tanix TX6 (no external crystal)
http://ix.io/2bI2 OrangePi 3 (external crystal present)

Please disregard ac200_clk in first case, it's part of another work.

Best regards,
Jernej