Re: [RFCv1] rtc: m41t80: disable clock provider support

[Sebastian Reichel <sebastian.reichel@collabora.com>]

[-- 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 --]