linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFCv1] rtc: m41t80: disable clock provider support
@ 2019-11-08 17:01 Sebastian Reichel
  2019-11-08 17:53 ` Alexandre Belloni
  0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Reichel @ 2019-11-08 17:01 UTC (permalink / raw)
  To: linux-clk, linux-rtc
  Cc: Alessandro Zummo, Alexandre Belloni, Russell King,
	Michael Turquette, Stephen Boyd, linux-kernel, kernel,
	Sebastian Reichel

Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
modules SQW clock output defaults to 32768 Hz. This behaviour is
used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
the clock is disabled and all i.MX6 functionality depending on
the 32 KHz clock have undefined behaviour (e.g. the hardware watchdog
run to fast or slow).

The normal solution would be to properly describe the clock tree
in DT, but from the kernel's perspective this is a chicken-and-egg
problem: CKIL is required very early, but the clock is only provided
after the I2C RTC has been probed.

Technically everything is fine by not touching anything, so this
works around the issue by disabling the clock handling from the
RTC driver. I guess the proper solution would be to simply mark the
clock as always-enabled, but this does not seem to be supported by
the clock framework.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
Hi,

This is a downstream workaround/hack for the issue described in the
commit message. I would like to upstream a board based on Congatec's
QMX6, which requires a proper solution for this. Do you think it
would be ok to have an always-on flag for clocks similar to regulators?

-- Sebastian
---
 drivers/rtc/rtc-m41t80.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 5f46f85f814b..81743d93d03e 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -973,7 +973,7 @@ static int m41t80_probe(struct i2c_client *client,
 		}
 	}
 #endif
-#ifdef CONFIG_COMMON_CLK
+#if 0
 	if (m41t80_data->features & M41T80_FEATURE_SQ)
 		m41t80_sqw_register_clk(m41t80_data);
 #endif
-- 
2.24.0.rc1


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

* Re: [RFCv1] rtc: m41t80: disable clock provider support
  2019-11-08 17:01 [RFCv1] rtc: m41t80: disable clock provider support Sebastian Reichel
@ 2019-11-08 17:53 ` Alexandre Belloni
  2019-11-08 22:34   ` Sebastian Reichel
  0 siblings, 1 reply; 9+ messages in thread
From: Alexandre Belloni @ 2019-11-08 17:53 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: linux-clk, linux-rtc, Alessandro Zummo, Russell King,
	Michael Turquette, Stephen Boyd, linux-kernel, kernel

On 08/11/2019 18:01:35+0100, Sebastian Reichel wrote:
> Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> modules SQW clock output defaults to 32768 Hz. This behaviour is
> used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> the clock is disabled and all i.MX6 functionality depending on
> the 32 KHz clock have undefined behaviour (e.g. the hardware watchdog
> run to fast or slow).
> 
> The normal solution would be to properly describe the clock tree
> in DT, but from the kernel's perspective this is a chicken-and-egg
> problem: CKIL is required very early, but the clock is only provided
> after the I2C RTC has been probed.
> 
> Technically everything is fine by not touching anything, so this
> works around the issue by disabling the clock handling from the
> RTC driver. I guess the proper solution would be to simply mark the
> clock as always-enabled, but this does not seem to be supported by
> the clock framework.
> 

You need to have a consumer so this clock is not disabled by the CCF
after seeing nobody uses it. If you need it early, you can have a look
at rtc-sun6i.c but I would like that to not become a recurrent pattern,
especially for discrete RTCs.

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

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

* Re: [RFCv1] rtc: m41t80: disable clock provider support
  2019-11-08 17:53 ` Alexandre Belloni
@ 2019-11-08 22:34   ` Sebastian Reichel
  2019-11-09  0:24     ` Stephen Boyd
  0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Reichel @ 2019-11-08 22:34 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: linux-clk, linux-rtc, Alessandro Zummo, Russell King,
	Michael Turquette, Stephen Boyd, linux-kernel, kernel

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

Hi,

On Fri, Nov 08, 2019 at 06:53:29PM +0100, Alexandre Belloni wrote:
> On 08/11/2019 18:01:35+0100, Sebastian Reichel wrote:
> > Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> > modules SQW clock output defaults to 32768 Hz. This behaviour is
> > used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> > the clock is disabled and all i.MX6 functionality depending on
> > the 32 KHz clock have undefined behaviour (e.g. the hardware watchdog
> > run to fast or slow).
> > 
> > The normal solution would be to properly describe the clock tree
> > in DT, but from the kernel's perspective this is a chicken-and-egg
> > problem: CKIL is required very early, but the clock is only provided
> > after the I2C RTC has been probed.
> > 
> > Technically everything is fine by not touching anything, so this
> > works around the issue by disabling the clock handling from the
> > RTC driver. I guess the proper solution would be to simply mark the
> > clock as always-enabled, but this does not seem to be supported by
> > the clock framework.
> > 
> 
> You need to have a consumer so this clock is not disabled by the CCF
> after seeing nobody uses it.

That's why I was wondering if we can have something like regulator's
always-enabled for clocks.

> If you need it early, you can have a look at rtc-sun6i.c but I
> would like that to not become a recurrent pattern, especially for
> discrete RTCs.

I don't just need it early. The issue is, that CKIL is the 32khz
low frequency clock fed into the i.MX6. It is initialized by the
clock manager, so I need it before any of the SoC clocks are
registered. Without the SoC clocks, the I2C bus cannot be probed
and thus the RTC driver cannot be probed.

-- Sebastian

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

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

* Re: [RFCv1] rtc: m41t80: disable clock provider support
  2019-11-08 22:34   ` Sebastian Reichel
@ 2019-11-09  0:24     ` Stephen Boyd
  2019-11-09  1:41       ` Sebastian Reichel
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2019-11-09  0:24 UTC (permalink / raw)
  To: Alexandre Belloni, Sebastian Reichel
  Cc: linux-clk, linux-rtc, Alessandro Zummo, Russell King,
	Michael Turquette, linux-kernel, kernel

Quoting Sebastian Reichel (2019-11-08 14:34:15)
> Hi,
> 
> On Fri, Nov 08, 2019 at 06:53:29PM +0100, Alexandre Belloni wrote:
> > On 08/11/2019 18:01:35+0100, Sebastian Reichel wrote:
> > > Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> > > modules SQW clock output defaults to 32768 Hz. This behaviour is
> > > used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> > > the clock is disabled and all i.MX6 functionality depending on
> > > the 32 KHz clock have undefined behaviour (e.g. the hardware watchdog
> > > run to fast or slow).
> > > 
> > > The normal solution would be to properly describe the clock tree
> > > in DT, but from the kernel's perspective this is a chicken-and-egg
> > > problem: CKIL is required very early, but the clock is only provided
> > > after the I2C RTC has been probed.
> > > 
> > > Technically everything is fine by not touching anything, so this
> > > works around the issue by disabling the clock handling from the
> > > RTC driver. I guess the proper solution would be to simply mark the
> > > clock as always-enabled, but this does not seem to be supported by
> > > the clock framework.
> > > 
> > 
> > You need to have a consumer so this clock is not disabled by the CCF
> > after seeing nobody uses it.
> 
> That's why I was wondering if we can have something like regulator's
> always-enabled for clocks.

There's a flag CLK_IS_CRITICAL that providers can set.

> 
> > If you need it early, you can have a look at rtc-sun6i.c but I
> > would like that to not become a recurrent pattern, especially for
> > discrete RTCs.
> 
> I don't just need it early. The issue is, that CKIL is the 32khz
> low frequency clock fed into the i.MX6. It is initialized by the
> clock manager, so I need it before any of the SoC clocks are
> registered. Without the SoC clocks, the I2C bus cannot be probed
> and thus the RTC driver cannot be probed.
> 

Is this the chicken-egg scenario? I read this thread but I can't follow
along with what the problem is. Sorry.


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

* Re: [RFCv1] rtc: m41t80: disable clock provider support
  2019-11-09  0:24     ` Stephen Boyd
@ 2019-11-09  1:41       ` Sebastian Reichel
  2019-11-09  6:53         ` Stephen Boyd
  0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Reichel @ 2019-11-09  1:41 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Alexandre Belloni, linux-clk, linux-rtc, Alessandro Zummo,
	Russell King, Michael Turquette, linux-kernel, kernel

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

Hi,

On Fri, Nov 08, 2019 at 04:24:48PM -0800, Stephen Boyd wrote:
> Quoting Sebastian Reichel (2019-11-08 14:34:15)
> > On Fri, Nov 08, 2019 at 06:53:29PM +0100, Alexandre Belloni wrote:
> > > On 08/11/2019 18:01:35+0100, Sebastian Reichel wrote:
> > > > Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> > > > modules SQW clock output defaults to 32768 Hz. This behaviour is
> > > > used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> > > > the clock is disabled and all i.MX6 functionality depending on
> > > > the 32 KHz clock have undefined behaviour (e.g. the hardware watchdog
> > > > run to fast or slow).
> > > > 
> > > > The normal solution would be to properly describe the clock tree
> > > > in DT, but from the kernel's perspective this is a chicken-and-egg
> > > > problem: CKIL is required very early, but the clock is only provided
> > > > after the I2C RTC has been probed.
> > > > 
> > > > Technically everything is fine by not touching anything, so this
> > > > works around the issue by disabling the clock handling from the
> > > > RTC driver. I guess the proper solution would be to simply mark the
> > > > clock as always-enabled, but this does not seem to be supported by
> > > > the clock framework.
> > > > 
> > > 
> > > You need to have a consumer so this clock is not disabled by the CCF
> > > after seeing nobody uses it.
> > 
> > That's why I was wondering if we can have something like regulator's
> > always-enabled for clocks.
> 
> There's a flag CLK_IS_CRITICAL that providers can set.

Thanks, that is what I was looking for.
Is there a DT binding to set that flag for a clock?

> > > If you need it early, you can have a look at rtc-sun6i.c but I
> > > would like that to not become a recurrent pattern, especially for
> > > discrete RTCs.
> > 
> > I don't just need it early. The issue is, that CKIL is the 32khz
> > low frequency clock fed into the i.MX6. It is initialized by the
> > clock manager, so I need it before any of the SoC clocks are
> > registered. Without the SoC clocks, the I2C bus cannot be probed
> > and thus the RTC driver cannot be probed.
> > 
> 
> Is this the chicken-egg scenario? I read this thread but I can't follow
> along with what the problem is. Sorry.

Yes. The board has an I2C based RTC (m41t62), which provides a programmable 1
Hz to 32 kHz square wave (SQW) output defaulting to 32 kHz. The board designers
connected the RTC's SQW output to the i.MX6 CKIL clock input instead of adding
another oscillator. The i.MX6 CCM acquires that clock in imx6q_clocks_init()
(and assumes it is a fixed clock):

hws[IMX6QDL_CLK_CKIL] = imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);

Changing this to reference the RTC SQW results in the chicken-egg scenario. It
would mean, that imx6q_clocks_init() cannot complete without the RTC driver, but
the RTC cannot probe without the I2C bus driver and the I2C bus driver needs some
clocks from the i.MX6.

I think adding the clock-is-critical flag is the best solution for
this setup, but on most boards the RTC SQW clock is not critical and
should be disabled. Did I miss a DT flag, that can be added on the
specific board?

-- Sebastian

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

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

* Re: [RFCv1] rtc: m41t80: disable clock provider support
  2019-11-09  1:41       ` Sebastian Reichel
@ 2019-11-09  6:53         ` Stephen Boyd
  2019-11-12 15:15           ` Sebastian Reichel
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2019-11-09  6:53 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Alexandre Belloni, linux-clk, linux-rtc, Alessandro Zummo,
	Russell King, Michael Turquette, linux-kernel, kernel

Quoting Sebastian Reichel (2019-11-08 17:41:51)
> Hi,
> 
> On Fri, Nov 08, 2019 at 04:24:48PM -0800, Stephen Boyd wrote:
> > Quoting Sebastian Reichel (2019-11-08 14:34:15)
> > > On Fri, Nov 08, 2019 at 06:53:29PM +0100, Alexandre Belloni wrote:
> > > > On 08/11/2019 18:01:35+0100, Sebastian Reichel wrote:
> > > > > Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> > > > > modules SQW clock output defaults to 32768 Hz. This behaviour is
> > > > > used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> > > > > the clock is disabled and all i.MX6 functionality depending on
> > > > > the 32 KHz clock have undefined behaviour (e.g. the hardware watchdog
> > > > > run to fast or slow).
> > > > > 
> > > > > The normal solution would be to properly describe the clock tree
> > > > > in DT, but from the kernel's perspective this is a chicken-and-egg
> > > > > problem: CKIL is required very early, but the clock is only provided
> > > > > after the I2C RTC has been probed.
> > > > > 
> > > > > Technically everything is fine by not touching anything, so this
> > > > > works around the issue by disabling the clock handling from the
> > > > > RTC driver. I guess the proper solution would be to simply mark the
> > > > > clock as always-enabled, but this does not seem to be supported by
> > > > > the clock framework.
> > > > > 
> > > > 
> > > > You need to have a consumer so this clock is not disabled by the CCF
> > > > after seeing nobody uses it.
> > > 
> > > That's why I was wondering if we can have something like regulator's
> > > always-enabled for clocks.
> > 
> > There's a flag CLK_IS_CRITICAL that providers can set.
> 
> Thanks, that is what I was looking for.
> Is there a DT binding to set that flag for a clock?
> 

No.

> > > > If you need it early, you can have a look at rtc-sun6i.c but I
> > > > would like that to not become a recurrent pattern, especially for
> > > > discrete RTCs.
> > > 
> > > I don't just need it early. The issue is, that CKIL is the 32khz
> > > low frequency clock fed into the i.MX6. It is initialized by the
> > > clock manager, so I need it before any of the SoC clocks are
> > > registered. Without the SoC clocks, the I2C bus cannot be probed
> > > and thus the RTC driver cannot be probed.
> > > 
> > 
> > Is this the chicken-egg scenario? I read this thread but I can't follow
> > along with what the problem is. Sorry.
> 
> Yes. The board has an I2C based RTC (m41t62), which provides a programmable 1
> Hz to 32 kHz square wave (SQW) output defaulting to 32 kHz. The board designers
> connected the RTC's SQW output to the i.MX6 CKIL clock input instead of adding
> another oscillator. The i.MX6 CCM acquires that clock in imx6q_clocks_init()
> (and assumes it is a fixed clock):
> 
> hws[IMX6QDL_CLK_CKIL] = imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);

Who uses the IMX6QDL_CLK_CKIL though? Grep on kernel sources shows me
nothing.

> 
> Changing this to reference the RTC SQW results in the chicken-egg scenario. It
> would mean, that imx6q_clocks_init() cannot complete without the RTC driver, but
> the RTC cannot probe without the I2C bus driver and the I2C bus driver needs some
> clocks from the i.MX6.
> 
> I think adding the clock-is-critical flag is the best solution for
> this setup, but on most boards the RTC SQW clock is not critical and
> should be disabled. Did I miss a DT flag, that can be added on the
> specific board?
> 

The clk framework can unwind this problem for you. It lazily evaluates
parents so that clk controllers can probe without needing all their
parent clks to exist yet.

The clocks in i.MX6 can be registered first and some of those can be
left "orphaned". Then the i2c driver can probe and get the i2c clks it
needs from the i.MX6 driver and use them because their path to the root
is registered. The i2c driver can then probe the RTC which provides the
CLK_CKIL parent.

Does something go wrong, or you're just concerned that it might not
work?


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

* Re: [RFCv1] rtc: m41t80: disable clock provider support
  2019-11-09  6:53         ` Stephen Boyd
@ 2019-11-12 15:15           ` Sebastian Reichel
  2019-11-12 22:20             ` Stephen Boyd
  0 siblings, 1 reply; 9+ messages in thread
From: Sebastian Reichel @ 2019-11-12 15:15 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Alexandre Belloni, linux-clk, linux-rtc, Alessandro Zummo,
	Russell King, Michael Turquette, linux-kernel, kernel

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

Hi,

On Fri, Nov 08, 2019 at 10:53:33PM -0800, Stephen Boyd wrote:
> Quoting Sebastian Reichel (2019-11-08 17:41:51)
> > On Fri, Nov 08, 2019 at 04:24:48PM -0800, Stephen Boyd wrote:
> > > Quoting Sebastian Reichel (2019-11-08 14:34:15)
> > > > On Fri, Nov 08, 2019 at 06:53:29PM +0100, Alexandre Belloni wrote:
> > > > > On 08/11/2019 18:01:35+0100, Sebastian Reichel wrote:
> > > > > > Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> > > > > > modules SQW clock output defaults to 32768 Hz. This behaviour is
> > > > > > used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> > > > > > the clock is disabled and all i.MX6 functionality depending on
> > > > > > the 32 KHz clock have undefined behaviour (e.g. the hardware watchdog
> > > > > > run to fast or slow).
> > > > > > 
> > > > > > The normal solution would be to properly describe the clock tree
> > > > > > in DT, but from the kernel's perspective this is a chicken-and-egg
> > > > > > problem: CKIL is required very early, but the clock is only provided
> > > > > > after the I2C RTC has been probed.
> > > > > > 
> > > > > > Technically everything is fine by not touching anything, so this
> > > > > > works around the issue by disabling the clock handling from the
> > > > > > RTC driver. I guess the proper solution would be to simply mark the
> > > > > > clock as always-enabled, but this does not seem to be supported by
> > > > > > the clock framework.
> > > > > > 
> > > > > 
> > > > > You need to have a consumer so this clock is not disabled by the CCF
> > > > > after seeing nobody uses it.
> > > > 
> > > > That's why I was wondering if we can have something like regulator's
> > > > always-enabled for clocks.
> > > 
> > > There's a flag CLK_IS_CRITICAL that providers can set.
> > 
> > Thanks, that is what I was looking for.
> > Is there a DT binding to set that flag for a clock?
> > 
> 
> No.

:(

> > > > > If you need it early, you can have a look at rtc-sun6i.c but I
> > > > > would like that to not become a recurrent pattern, especially for
> > > > > discrete RTCs.
> > > > 
> > > > I don't just need it early. The issue is, that CKIL is the 32khz
> > > > low frequency clock fed into the i.MX6. It is initialized by the
> > > > clock manager, so I need it before any of the SoC clocks are
> > > > registered. Without the SoC clocks, the I2C bus cannot be probed
> > > > and thus the RTC driver cannot be probed.
> > > > 
> > > 
> > > Is this the chicken-egg scenario? I read this thread but I can't follow
> > > along with what the problem is. Sorry.
> > 
> > Yes. The board has an I2C based RTC (m41t62), which provides a programmable 1
> > Hz to 32 kHz square wave (SQW) output defaulting to 32 kHz. The board designers
> > connected the RTC's SQW output to the i.MX6 CKIL clock input instead of adding
> > another oscillator. The i.MX6 CCM acquires that clock in imx6q_clocks_init()
> > (and assumes it is a fixed clock):
> > 
> > hws[IMX6QDL_CLK_CKIL] = imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> 
> Who uses the IMX6QDL_CLK_CKIL though? Grep on kernel sources shows me
> nothing.

The manual specifies, that CKIL is synchronized with the main system
clock. The resulting clock is used by all kind of IP cores inside
the i.MX6, for example the SNVS RTC and watchdog. I couldn't find
any registers to configure the CKIL pipeline and CKIL input is
usually a fixed clock, so current implementation might be "broken"
without anyone noticing. Checking a running i.MX6 system, that
actually seems to be the case :(

$ cat /sys/kernel/debug/clk/ckil/clk_rate         
32768
$ cat /sys/kernel/debug/clk/ckil/clk_enable_count 
0
$ cat /sys/kernel/debug/clk/ckil/clk_prepare_count 
0
$ cat /sys/kernel/debug/clk/ckil/clk_flags         
CLK_IS_BASIC

I suppose an easy fix would be to mark that clock as critical and
that would also keep the parent clocks enabled?

> > Changing this to reference the RTC SQW results in the chicken-egg scenario. It
> > would mean, that imx6q_clocks_init() cannot complete without the RTC driver, but
> > the RTC cannot probe without the I2C bus driver and the I2C bus driver needs some
> > clocks from the i.MX6.
> > 
> > I think adding the clock-is-critical flag is the best solution for
> > this setup, but on most boards the RTC SQW clock is not critical and
> > should be disabled. Did I miss a DT flag, that can be added on the
> > specific board?
> > 
> 
> The clk framework can unwind this problem for you. It lazily evaluates
> parents so that clk controllers can probe without needing all their
> parent clks to exist yet.
>
> The clocks in i.MX6 can be registered first and some of those can be
> left "orphaned". Then the i2c driver can probe and get the i2c clks it
> needs from the i.MX6 driver and use them because their path to the root
> is registered. The i2c driver can then probe the RTC which provides the
> CLK_CKIL parent.

That's nice, I wasn't aware of this feature. Thanks for the
explanation.

> Does something go wrong, or you're just concerned that it might not
> work?

I did not try it after noticing the dependencies. The only thing
known to be broken is the current situation in mainline ("unused"
RTC clock is turned off), but right now there is no QMX6 based board
in mainline.

-- Sebastian

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

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

* Re: [RFCv1] rtc: m41t80: disable clock provider support
  2019-11-12 15:15           ` Sebastian Reichel
@ 2019-11-12 22:20             ` Stephen Boyd
  2019-11-13 22:27               ` Sebastian Reichel
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2019-11-12 22:20 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Alexandre Belloni, linux-clk, linux-rtc, Alessandro Zummo,
	Russell King, Michael Turquette, linux-kernel, kernel

Quoting Sebastian Reichel (2019-11-12 07:15:26)
> Hi,
> 
> On Fri, Nov 08, 2019 at 10:53:33PM -0800, Stephen Boyd wrote:
> > Quoting Sebastian Reichel (2019-11-08 17:41:51)
> > > On Fri, Nov 08, 2019 at 04:24:48PM -0800, Stephen Boyd wrote:
> > > > 
> > > > Is this the chicken-egg scenario? I read this thread but I can't follow
> > > > along with what the problem is. Sorry.
> > > 
> > > Yes. The board has an I2C based RTC (m41t62), which provides a programmable 1
> > > Hz to 32 kHz square wave (SQW) output defaulting to 32 kHz. The board designers
> > > connected the RTC's SQW output to the i.MX6 CKIL clock input instead of adding
> > > another oscillator. The i.MX6 CCM acquires that clock in imx6q_clocks_init()
> > > (and assumes it is a fixed clock):
> > > 
> > > hws[IMX6QDL_CLK_CKIL] = imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > 
> > Who uses the IMX6QDL_CLK_CKIL though? Grep on kernel sources shows me
> > nothing.
> 
> The manual specifies, that CKIL is synchronized with the main system
> clock. The resulting clock is used by all kind of IP cores inside
> the i.MX6, for example the SNVS RTC and watchdog. I couldn't find
> any registers to configure the CKIL pipeline and CKIL input is
> usually a fixed clock, so current implementation might be "broken"
> without anyone noticing. Checking a running i.MX6 system, that
> actually seems to be the case :(
> 
> $ cat /sys/kernel/debug/clk/ckil/clk_rate         
> 32768
> $ cat /sys/kernel/debug/clk/ckil/clk_enable_count 
> 0
> $ cat /sys/kernel/debug/clk/ckil/clk_prepare_count 
> 0
> $ cat /sys/kernel/debug/clk/ckil/clk_flags         
> CLK_IS_BASIC
> 
> I suppose an easy fix would be to mark that clock as critical and
> that would also keep the parent clocks enabled?

Yes. It sounds like some sort of low frequency timer clk. It probably
should always be left enabled with CLK_IS_CRITICAL then.


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

* Re: [RFCv1] rtc: m41t80: disable clock provider support
  2019-11-12 22:20             ` Stephen Boyd
@ 2019-11-13 22:27               ` Sebastian Reichel
  0 siblings, 0 replies; 9+ messages in thread
From: Sebastian Reichel @ 2019-11-13 22:27 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Alexandre Belloni, linux-clk, linux-rtc, Alessandro Zummo,
	Russell King, Michael Turquette, linux-kernel, kernel

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

Hi Stephen,

On Tue, Nov 12, 2019 at 02:20:11PM -0800, Stephen Boyd wrote:
> Quoting Sebastian Reichel (2019-11-12 07:15:26)
> > On Fri, Nov 08, 2019 at 10:53:33PM -0800, Stephen Boyd wrote:
> > > Quoting Sebastian Reichel (2019-11-08 17:41:51)
> > > > On Fri, Nov 08, 2019 at 04:24:48PM -0800, Stephen Boyd wrote:
> > > > > 
> > > > > Is this the chicken-egg scenario? I read this thread but I can't follow
> > > > > along with what the problem is. Sorry.
> > > > 
> > > > Yes. The board has an I2C based RTC (m41t62), which provides a programmable 1
> > > > Hz to 32 kHz square wave (SQW) output defaulting to 32 kHz. The board designers
> > > > connected the RTC's SQW output to the i.MX6 CKIL clock input instead of adding
> > > > another oscillator. The i.MX6 CCM acquires that clock in imx6q_clocks_init()
> > > > (and assumes it is a fixed clock):
> > > > 
> > > > hws[IMX6QDL_CLK_CKIL] = imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > > 
> > > Who uses the IMX6QDL_CLK_CKIL though? Grep on kernel sources shows me
> > > nothing.
> > 
> > The manual specifies, that CKIL is synchronized with the main system
> > clock. The resulting clock is used by all kind of IP cores inside
> > the i.MX6, for example the SNVS RTC and watchdog. I couldn't find
> > any registers to configure the CKIL pipeline and CKIL input is
> > usually a fixed clock, so current implementation might be "broken"
> > without anyone noticing. Checking a running i.MX6 system, that
> > actually seems to be the case :(
> > 
> > $ cat /sys/kernel/debug/clk/ckil/clk_rate         
> > 32768
> > $ cat /sys/kernel/debug/clk/ckil/clk_enable_count 
> > 0
> > $ cat /sys/kernel/debug/clk/ckil/clk_prepare_count 
> > 0
> > $ cat /sys/kernel/debug/clk/ckil/clk_flags         
> > CLK_IS_BASIC
> > 
> > I suppose an easy fix would be to mark that clock as critical and
> > that would also keep the parent clocks enabled?
> 
> Yes. It sounds like some sort of low frequency timer clk. It probably
> should always be left enabled with CLK_IS_CRITICAL then.

Right, system expects that clock to be always available including
low power states. This is supposed not to be turned off at all.

I gave it a try today (I defined ckil clock in DT as fixed
rate clock with divider and multiplier set to 1 and used
the RTC as parent clock) and it happened exactly what I
expected: I received -EPROBE_DEFER. This results in
the problem, that I pointed out.

Actually imx6 clock manager driver registers a fixed clock, when
the DT part fails (incl. a -EPROBE_DEFER error), so it still boots.
But then the reference to the parent clock is obviously missing,
so RTC clock is not enabled and CKIL is effectivly missing.

If the error code is handled properly the boot does not finish,
since the i2c bus driver probe defers without the clock manager's
clocks being available. Without the i2c bus driver, the RTC driver
is not probed, so the clock never appears.

The simplest fix would be to export of_clk_detect_critical()
and call it in the RTC driver. Reading the comment above the
function I suppose this is not an acceptable solution?

-- Sebastian

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

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

end of thread, other threads:[~2019-11-13 22:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 17:01 [RFCv1] rtc: m41t80: disable clock provider support Sebastian Reichel
2019-11-08 17:53 ` Alexandre Belloni
2019-11-08 22:34   ` Sebastian Reichel
2019-11-09  0:24     ` Stephen Boyd
2019-11-09  1:41       ` Sebastian Reichel
2019-11-09  6:53         ` Stephen Boyd
2019-11-12 15:15           ` Sebastian Reichel
2019-11-12 22:20             ` Stephen Boyd
2019-11-13 22:27               ` Sebastian Reichel

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