Re: [PATCH 1/2] rtc: pcf2127: Disable Power-On Reset Override - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Philipp Rosenberger <p.rosenberger@kunbus.com>
Cc: linux-rtc@vger.kernel.org, dan.carpenter@oracle.com,
	biwen.li@nxp.com, lvb@xiphos.com, bruno.thomsen@gmail.com,
	Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] rtc: pcf2127: Disable Power-On Reset Override
Date: Tue, 12 Jan 2021 20:26:33 +0100	[thread overview]
Message-ID: <20210112192633.yl7nx474r6njfynd@pengutronix.de> (raw)
In-Reply-To: <20210104161910.9144-2-p.rosenberger@kunbus.com>

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

Hello,

On Mon, Jan 04, 2021 at 05:19:09PM +0100, Philipp Rosenberger wrote:
> If the PCF2127/2129 has lost all power and is then powered again it goes
> into "Power-On Reset Override" mode. In this mode the RTC seems to work
> fine. Also the watchdog can be configured. The watchdog timer counts as
> expected and the WDTF (watchdog timer flag) gets set. But no interrupt
> is generated on the INT pin. The same applies to the alarm function.
> 
> The POR_OVRD bit on the Control_1 register must be cleared first. In
> some cases the bootloader or firmware might have done this already. But
> we clear the bit nevertheless to guarantee correct behavior the
> watchdog and alarm function.

I don't understand this. The reference manual tells us about this bit:

| The POR duration is directly related to the crystal oscillator
| start-up time. Due to the long start-up times experienced by these
| types of circuits, a mechanism has been built in to disable the POR
| and therefore speed up the on-board test of the device.
| The setting of the PORO mode requires that POR_OVRD in register
| Control_1 is set logic 1 and that the signals at the interface pins
| SDA/CE and SCL are toggled as illustrated in Figure 18.

So this means that with the bit set (i.e. with this patch added) after a
power-on the oscillator isn't properly reset. This means the chip might
not work correctly, right? Does "speed up the on-board test" suggest,
this is a feature that is to be used while testing the chip and not for
production use? You missed to ensure that the requested toggling is
done. Did you test how much time this actually saves? I doubt it is
worth to trade correct operation for quicker startup time is the thing
we want here.

If you still think this is a good idea I guess you need a much better
commit log (and code comment).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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