Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] rtc: sun6i: Make external oscillator optional
@ 2020-02-13 21:14 Jernej Skrabec
  2020-02-13 21:14 ` [PATCH 1/2] rtc: sun6i: Make external 32k " Jernej Skrabec
  2020-02-13 21:14 ` [PATCH 2/2] arm64: dts: allwinner: h6: Move ext. oscillator to board DTs Jernej Skrabec
  0 siblings, 2 replies; 7+ messages in thread
From: Jernej Skrabec @ 2020-02-13 21:14 UTC (permalink / raw)
  To: mripard, wens
  Cc: mark.rutland, a.zummo, alexandre.belloni, devicetree,
	jernej.skrabec, linux-kernel, robh+dt, linux-arm-kernel,
	linux-rtc

This is implementation of idea discussed here:
https://lore.kernel.org/linux-arm-kernel/20200117183901.lkieha3hu6nz2hoj@gilmour.lan/T/

Part of first patch commit message:

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).

In this series I fixed only H6 based boards since improper settings have
real impact due to explicitly forbidden fallback to internal oscillator.
Since most boards actually contain external oscillator, I wonder if it's
better to leave external oscillator in common H6 dtsi and just delete
clocks property in rtc node and ext. oscillator node in board dts file?

What do you think?

Best regards,
Jernej

Jernej Skrabec (2):
  rtc: sun6i: Make external 32k oscillator optional
  arm64: dts: allwinner: h6: Move ext. oscillator to board DTs

 .../boot/dts/allwinner/sun50i-h6-beelink-gs1.dts   | 11 +++++++++++
 .../boot/dts/allwinner/sun50i-h6-orangepi-3.dts    | 11 +++++++++++
 .../boot/dts/allwinner/sun50i-h6-orangepi.dtsi     | 11 +++++++++++
 .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 11 +++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi       |  8 --------
 drivers/rtc/rtc-sun6i.c                            | 14 ++++++--------
 6 files changed, 50 insertions(+), 16 deletions(-)

-- 
2.25.0


_______________________________________________
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] 7+ messages in thread

* [PATCH 1/2] rtc: sun6i: Make external 32k oscillator optional
  2020-02-13 21:14 [PATCH 0/2] rtc: sun6i: Make external oscillator optional Jernej Skrabec
@ 2020-02-13 21:14 ` " Jernej Skrabec
  2020-02-14  8:14   ` Maxime Ripard
  2020-02-13 21:14 ` [PATCH 2/2] arm64: dts: allwinner: h6: Move ext. oscillator to board DTs Jernej Skrabec
  1 sibling, 1 reply; 7+ messages in thread
From: Jernej Skrabec @ 2020-02-13 21:14 UTC (permalink / raw)
  To: mripard, wens
  Cc: mark.rutland, a.zummo, alexandre.belloni, devicetree,
	jernej.skrabec, linux-kernel, robh+dt, linux-arm-kernel,
	linux-rtc

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 <jernej.skrabec@siol.net>
---
 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;
-
 	/* Only read IOSC name from device tree if it is exported */
 	if (rtc->data->export_iosc)
 		of_property_read_string_index(node, "clock-output-names", 2,
-- 
2.25.0


_______________________________________________
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] 7+ messages in thread

* [PATCH 2/2] arm64: dts: allwinner: h6: Move ext. oscillator to board DTs
  2020-02-13 21:14 [PATCH 0/2] rtc: sun6i: Make external oscillator optional Jernej Skrabec
  2020-02-13 21:14 ` [PATCH 1/2] rtc: sun6i: Make external 32k " Jernej Skrabec
@ 2020-02-13 21:14 ` Jernej Skrabec
  1 sibling, 0 replies; 7+ messages in thread
From: Jernej Skrabec @ 2020-02-13 21:14 UTC (permalink / raw)
  To: mripard, wens
  Cc: mark.rutland, a.zummo, alexandre.belloni, devicetree,
	jernej.skrabec, linux-kernel, robh+dt, linux-arm-kernel,
	linux-rtc

It turns out that not all H6 boards have external 32kHz oscillator.
Currently the only one known such H6 board is Tanix TX6.

Move external oscillator node from common H6 dtsi to board specific dts
files where present.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 .../boot/dts/allwinner/sun50i-h6-beelink-gs1.dts      | 11 +++++++++++
 .../arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts | 11 +++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi | 11 +++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts  | 11 +++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi          |  8 --------
 5 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
index df6d872c34e2..8f09d209359b 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
@@ -32,6 +32,13 @@ hdmi_con_in: endpoint {
 		};
 	};
 
+	ext_osc32k: ext_osc32k_clk {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <32768>;
+		clock-output-names = "ext_osc32k";
+	};
+
 	leds {
 		compatible = "gpio-leds";
 
@@ -275,6 +282,10 @@ &r_pio {
 	vcc-pm-supply = <&reg_aldo1>;
 };
 
+&rtc {
+	clocks = <&ext_osc32k>;
+};
+
 &spdif {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
index c311eee52a35..ca11e19c2d17 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
@@ -32,6 +32,13 @@ hdmi_con_in: endpoint {
 		};
 	};
 
+	ext_osc32k: ext_osc32k_clk {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <32768>;
+		clock-output-names = "ext_osc32k";
+	};
+
 	leds {
 		compatible = "gpio-leds";
 
@@ -276,6 +283,10 @@ &r_ir {
 	status = "okay";
 };
 
+&rtc {
+	clocks = <&ext_osc32k>;
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_ph_pins>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
index 37f4c57597d4..37fc3f3697f7 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
@@ -20,6 +20,13 @@ chosen {
 		stdout-path = "serial0:115200n8";
 	};
 
+	ext_osc32k: ext_osc32k_clk {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <32768>;
+		clock-output-names = "ext_osc32k";
+	};
+
 	leds {
 		compatible = "gpio-leds";
 
@@ -197,6 +204,10 @@ &r_ir {
 	status = "okay";
 };
 
+&rtc {
+	clocks = <&ext_osc32k>;
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_ph_pins>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
index 3c9dd0d69754..b0642d841933 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
@@ -21,6 +21,13 @@ chosen {
 		stdout-path = "serial0:115200n8";
 	};
 
+	ext_osc32k: ext_osc32k_clk {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <32768>;
+		clock-output-names = "ext_osc32k";
+	};
+
 	hdmi_connector: connector {
 		compatible = "hdmi-connector";
 		type = "a";
@@ -279,6 +286,10 @@ &r_pio {
 	vcc-pm-supply = <&reg_aldo1>;
 };
 
+&rtc {
+	clocks = <&ext_osc32k>;
+};
+
 /*
  * The CS pin is shared with the MMC2 CMD pin, so we cannot have the SPI
  * flash and eMMC at the same time, as one of them would fail probing.
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 41b58ffa8596..3ad7c78e9c43 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -62,13 +62,6 @@ osc24M: osc24M_clk {
 		clock-output-names = "osc24M";
 	};
 
-	ext_osc32k: ext_osc32k_clk {
-		#clock-cells = <0>;
-		compatible = "fixed-clock";
-		clock-frequency = <32768>;
-		clock-output-names = "ext_osc32k";
-	};
-
 	pmu {
 		compatible = "arm,cortex-a53-pmu",
 			     "arm,armv8-pmuv3";
@@ -855,7 +848,6 @@ rtc: rtc@7000000 {
 			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>;
 		};
 
-- 
2.25.0


_______________________________________________
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] 7+ messages in thread

* Re: [PATCH 1/2] rtc: sun6i: Make external 32k oscillator optional
  2020-02-13 21:14 ` [PATCH 1/2] rtc: sun6i: Make external 32k " Jernej Skrabec
@ 2020-02-14  8:14   ` Maxime Ripard
  2020-02-14 16:42     ` Jernej Škrabec
  0 siblings, 1 reply; 7+ messages in thread
From: Maxime Ripard @ 2020-02-14  8:14 UTC (permalink / raw)
  To: Jernej Skrabec
  Cc: mark.rutland, a.zummo, alexandre.belloni, devicetree,
	linux-kernel, wens, robh+dt, linux-arm-kernel, linux-rtc

[-- Attachment #1.1: Type: text/plain, Size: 1978 bytes --]

Hi Jernej,

Thanks for taking care of this

On Thu, Feb 13, 2020 at 10:14:26PM +0100, Jernej Skrabec wrote:
> 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 <jernej.skrabec@siol.net>
> ---
>  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?

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
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] 7+ messages in thread

* Re: [PATCH 1/2] rtc: sun6i: Make external 32k oscillator optional
  2020-02-14  8:14   ` Maxime Ripard
@ 2020-02-14 16:42     ` Jernej Škrabec
  2020-02-20 17:47       ` Maxime Ripard
  0 siblings, 1 reply; 7+ messages in thread
From: Jernej Škrabec @ 2020-02-14 16:42 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: mark.rutland, a.zummo, alexandre.belloni, devicetree,
	linux-kernel, wens, robh+dt, linux-arm-kernel, linux-rtc

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:
> > 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 <jernej.skrabec@siol.net>
> > ---
> > 
> >  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




_______________________________________________
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] 7+ messages in thread

* Re: [PATCH 1/2] rtc: sun6i: Make external 32k oscillator optional
  2020-02-14 16:42     ` Jernej Škrabec
@ 2020-02-20 17:47       ` Maxime Ripard
  2020-02-20 17:59         ` Jernej Škrabec
  0 siblings, 1 reply; 7+ messages in thread
From: Maxime Ripard @ 2020-02-20 17:47 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: mark.rutland, a.zummo, alexandre.belloni, devicetree,
	linux-kernel, wens, robh+dt, linux-arm-kernel, linux-rtc

[-- Attachment #1.1: Type: text/plain, Size: 3387 bytes --]

On Fri, Feb 14, 2020 at 05:42:13PM +0100, Jernej Škrabec wrote:
> 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:
> > > 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 <jernej.skrabec@siol.net>
> > > ---
> > >
> > >  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)

I was concerned about the "other" parent. In the case where you don't
have a clocks property (so the check that you are removing), the code
then registers a clock with two parents: the one that we create (the
internal oscillator) and the one coming from the clocks property.

clk_summary only shows the current parent, which is going to be right
with your patch, but in the case where you have no clocks property,
you still (attempts to) register two parents, the second one being
non-functional.

Further looking at it, we might be good because we allocate an array
of two clocks, but only register of_clk_get_parent_count(node) + 1
clocks, so 1 if clocks is missing.

Still, I think this should be more obvious, through a comment or
shuffling a bit the parent registration maybe?

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
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] 7+ messages in thread

* Re: [PATCH 1/2] rtc: sun6i: Make external 32k oscillator optional
  2020-02-20 17:47       ` Maxime Ripard
@ 2020-02-20 17:59         ` Jernej Škrabec
  0 siblings, 0 replies; 7+ messages in thread
From: Jernej Škrabec @ 2020-02-20 17:59 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: mark.rutland, a.zummo, alexandre.belloni, devicetree,
	linux-kernel, wens, robh+dt, linux-arm-kernel, linux-rtc

Dne četrtek, 20. februar 2020 ob 18:47:49 CET je Maxime Ripard napisal(a):
> On Fri, Feb 14, 2020 at 05:42:13PM +0100, Jernej Škrabec wrote:
> > 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:
> > > > 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 <jernej.skrabec@siol.net>
> > > > ---
> > > > 
> > > >  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)
> 
> I was concerned about the "other" parent. In the case where you don't
> have a clocks property (so the check that you are removing), the code
> then registers a clock with two parents: the one that we create (the
> internal oscillator) and the one coming from the clocks property.
> 
> clk_summary only shows the current parent, which is going to be right
> with your patch, but in the case where you have no clocks property,
> you still (attempts to) register two parents, the second one being
> non-functional.
> 
> Further looking at it, we might be good because we allocate an array
> of two clocks, but only register of_clk_get_parent_count(node) + 1
> clocks, so 1 if clocks is missing.

Yes, my patch rely on "of_clk_get_parent_count(node) + 1". If there is no 
property, it will return 1 thus only first parent (internal RC oscilator) will 
be registered.

Anyway, following line:
parents[1] = of_clk_get_parent_name(node, 0);
should evaluate to null. I didn't research further what clk framework does 
with null parent because number of parents will be set to 1 and this null 
value will be ignored anyway.

> 
> Still, I think this should be more obvious, through a comment or
> shuffling a bit the parent registration maybe?

I think code is in correct order, just maybe a bit more explanation in form of 
comment(s) to make it more obvious how it works for either case.

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] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13 21:14 [PATCH 0/2] rtc: sun6i: Make external oscillator optional Jernej Skrabec
2020-02-13 21:14 ` [PATCH 1/2] rtc: sun6i: Make external 32k " Jernej Skrabec
2020-02-14  8:14   ` Maxime Ripard
2020-02-14 16:42     ` Jernej Škrabec
2020-02-20 17:47       ` Maxime Ripard
2020-02-20 17:59         ` Jernej Škrabec
2020-02-13 21:14 ` [PATCH 2/2] arm64: dts: allwinner: h6: Move ext. oscillator to board DTs Jernej Skrabec

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git