diff mbox series

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

Message ID 20210222171247.97609-2-sebastian.reichel@collabora.com
State Not Applicable
Headers show
Series Support for GE B1x5v2 | expand

Commit Message

Sebastian Reichel Feb. 22, 2021, 5:12 p.m. UTC
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.

Technically everything is fine by not touching anything, since
the RTC clock correctly enables the clock on reset (i.e. on
battery backup power loss) and also the bootloader enables it
in case a kernel without this support has been booted.

The 'protected-clocks' property is already in use for some clocks
that may not be touched because of firmware limitations and is
described in Documentation/devicetree/bindings/clock/clock-bindings.txt.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 Documentation/devicetree/bindings/rtc/rtc-m41t80.txt | 1 +
 drivers/rtc/rtc-m41t80.c                             | 3 +++
 2 files changed, 4 insertions(+)

Comments

Alexandre Belloni Feb. 22, 2021, 9:20 p.m. UTC | #1
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...

> Technically everything is fine by not touching anything, since
> the RTC clock correctly enables the clock on reset (i.e. on
> battery backup power loss) and also the bootloader enables it
> in case a kernel without this support has been booted.
> 
> The 'protected-clocks' property is already in use for some clocks
> that may not be touched because of firmware limitations and is
> described in Documentation/devicetree/bindings/clock/clock-bindings.txt.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  Documentation/devicetree/bindings/rtc/rtc-m41t80.txt | 1 +
>  drivers/rtc/rtc-m41t80.c                             | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt b/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
> index c746cb221210..ea4bbf5c4282 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
> +++ b/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
> @@ -19,6 +19,7 @@ Optional properties:
>  - interrupts: rtc alarm interrupt.
>  - clock-output-names: From common clock binding to override the default output
>                        clock name
> +- protected-clocks: Bool, if set operating system should not handle clock.
>  - wakeup-source: Enables wake up of host system on alarm
>  
>  Example:
> diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
> index 160dcf68e64e..3296583853a8 100644
> --- a/drivers/rtc/rtc-m41t80.c
> +++ b/drivers/rtc/rtc-m41t80.c
> @@ -546,6 +546,9 @@ static struct clk *m41t80_sqw_register_clk(struct m41t80_data *m41t80)
>  	struct clk_init_data init;
>  	int ret;
>  
> +	if (of_property_read_bool(node, "protected-clocks"))
> +		return 0;
> +
>  	/* First disable the clock */
>  	ret = i2c_smbus_read_byte_data(client, M41T80_REG_ALARM_MON);
>  	if (ret < 0)
> -- 
> 2.30.0
>
Alexandre Belloni Feb. 22, 2021, 9:26 p.m. UTC | #2
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...
> 
> > Technically everything is fine by not touching anything, since
> > the RTC clock correctly enables the clock on reset (i.e. on
> > battery backup power loss) and also the bootloader enables it
> > in case a kernel without this support has been booted.
> > 
> > The 'protected-clocks' property is already in use for some clocks
> > that may not be touched because of firmware limitations and is
> > described in Documentation/devicetree/bindings/clock/clock-bindings.txt.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Or maybe you expected me to apply the patch, how are the following
patches dependent on this one?
Sebastian Reichel Feb. 23, 2021, 1:26 a.m. UTC | #3
Hi,

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.

> > > Technically everything is fine by not touching anything, since
> > > the RTC clock correctly enables the clock on reset (i.e. on
> > > battery backup power loss) and also the bootloader enables it
> > > in case a kernel without this support has been booted.
> > > 
> > > The 'protected-clocks' property is already in use for some clocks
> > > that may not be touched because of firmware limitations and is
> > > described in Documentation/devicetree/bindings/clock/clock-bindings.txt.
> > > 
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> 
> Or maybe you expected me to apply the patch, how are the following
> patches dependent on this one?

The last patch, which introduces a new board has a runtime
dependency on this patch. Without this feature the board
goes into a reboot loop because its bootloader enables the
i.MX6 watchdog and without the CKIL its timing is messed up.

But it's a pure runtime dependency for a new board, so it should
be fine to merge this via your tree. It basically means the board
is only working once your tree and arm tree have been merged,
which seems ok from my POV.

-- Sebastian
Rob Herring March 6, 2021, 7:56 p.m. UTC | #4
On Tue, Feb 23, 2021 at 02:26:57AM +0100, Sebastian Reichel wrote:
> Hi,
> 
> 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. It's a somewhat common issue.

Rob
Sebastian Reichel March 8, 2021, 2:03 p.m. UTC | #5
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.

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

-- Sebastian
Rob Herring March 16, 2021, 9:51 p.m. UTC | #6
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
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt b/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
index c746cb221210..ea4bbf5c4282 100644
--- a/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
+++ b/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
@@ -19,6 +19,7 @@  Optional properties:
 - interrupts: rtc alarm interrupt.
 - clock-output-names: From common clock binding to override the default output
                       clock name
+- protected-clocks: Bool, if set operating system should not handle clock.
 - wakeup-source: Enables wake up of host system on alarm
 
 Example:
diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 160dcf68e64e..3296583853a8 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -546,6 +546,9 @@  static struct clk *m41t80_sqw_register_clk(struct m41t80_data *m41t80)
 	struct clk_init_data init;
 	int ret;
 
+	if (of_property_read_bool(node, "protected-clocks"))
+		return 0;
+
 	/* First disable the clock */
 	ret = i2c_smbus_read_byte_data(client, M41T80_REG_ALARM_MON);
 	if (ret < 0)