Re: [PATCHv1 1/6] rtc: m41t80: add support for protected clock

[Rob Herring <robh@kernel.org>]

On Mon, Mar 08, 2021 at 03:03:58PM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Sat, Mar 06, 2021 at 11:56:45AM -0800, Rob Herring wrote:
> > On Tue, Feb 23, 2021 at 02:26:57AM +0100, Sebastian Reichel wrote:
> > > On Mon, Feb 22, 2021 at 10:26:26PM +0100, Alexandre Belloni wrote:
> > > > On 22/02/2021 22:20:47+0100, Alexandre Belloni wrote:
> > > > > On 22/02/2021 18:12:42+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 has undefined behaviour. On systems using hardware
> > > > > > watchdog it seems to likely trigger a lot earlier than configured.
> > > > > > 
> > > > > > The proper solution would be to describe this dependency in DT,
> > > > > > but that will result in a deadlock. The kernel will see, that
> > > > > > i.MX6 system clock needs the RTC clock and do probe deferral.
> > > > > > But the i.MX6 I2C module never becomes usable without the i.MX6
> > > > > > CKIL clock and thus the RTC's clock will not be probed. So from
> > > > > > the kernel's perspective this is a chicken-and-egg problem.
> > > > > > 
> > > > > 
> > > > > Reading the previous paragraph, I was going to suggest describing the
> > > > > dependency and wondering whether this would cause a circular dependency.
> > > > > I guess this will keep being an issue for clocks on an I2C or SPI bus...
> > > 
> > > Yes, it is a circular dependency on this particular system on
> > > module. It only works because the RTC enables the clock by
> > > default. The i.MX6 CKIL is expected to be always enabled.
> > 
> > I think you should describe the circular clocking and then provide a way 
> > to break the dependency.
> 
> This is very much not trivial. The clock is required during early
> initialization of the i.MX. At this point we are far from probing
> I2C drivers and without the I2C driver the clock is not registered.
> The current i.MX code expects the system clocks to be fixed clocks,
> since they must be enabled before any code is executed (incl.
> bootloader) and must never be disabled. From a HW design point of
> view it does not make sense to have a SW controllable clock for it,
> since it just adds extra cost. I believe for QMX6 it is only SW
> controllable, because that avoids the need for an extra crystal.
> 
> So how is the clock framework supposed to know, that it can ignore
> the clock during registration? I see the following options:
> 
> 1. My solution is the simplest one. Keep i.MX clock code the same
>    (it assumes a fixed-clock being used for CKIL) and avoid
>    registering RTC clock. This basically means the RTC is considered
>    to be a fixed-clock on this system, which is what the HW designers
>    seemed to have in mind (vendor kernel for the QMX6 is old enough
>    (4.9.x) to not to have CLK feature in the RTC driver. Vendor
>    U-Boot also does not touch the RTC. Booting mainline kernel once
>    bricks QMX6 boards until RTC battery is removed, so one could
>    actually argue addition of the CLK feature in 1373e77b4f10 (4.13)
>    is a regression). Currently Qualcomm device uses "protected-clocks"
>    for FW controlled clocks where Linux would crash the system by
>    trying to access them. IMHO the RTC is similar, since disabling
>    or modifying its frequency on QMX6 results in undefined behaviour
>    and possibly system crash.
> 
> 2. Make i.MX clock code use the RTC as CKIL clock provider, but
>    ignore it somehow. I see three sub-options:
> 
> 2.1. Add a property 'boot-enabled' to the RTC node, so that the
>      clock framework is aware of clock being enabled. This can
>      be used to satisfy clock dependencies somehow.
> 
> 2.2. The RTC device is not probed without I2C bus, but the driver
>      could also register a fake clock purely based on DT
>      information by adding some early init hook and take over
>      the clock once the I2C part is being probed. I think this
>      is a bad idea regarding maintainability of the driver.
>      Also for systems not using the RTC clock, the early clock
>      registration is basically wrong: If the kernel disables
>      the RTC it will stay disabled across boots if the RTC has
>      a backup battery. Basically we cannot imply anything from
>      the RTC compatible value alone.
> 
> 2.3 The i.MX core code could request CKIL with some flag, that
>     it's fine to have an unresolvable clock and just expect it
>     to be boot-enabled. The rationale would be, that CKIL must
>     be always-enabled.

I think 2.1 or 2.3 is fine. It boils down to detecting a cycle and then 
either you have a property or implicitly know to ignore a dependency.

> > It's a somewhat common issue.
> 
> It is? This only works, because one can treat the RTC's clock
> output like a fixed clock by not messing around with it.

Well, it's not the first time I've heard of the issue. Audio clocks are 
another example, but a bit different in that the clocks aren't needed 
until later. It's also come up in context of fw_devlinks which I 
think has some cycle breaking logic already.

Rob