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

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

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