Message-ID: <20210113083553.vsrc5xksssmpdw2f@pengutronix.de>
Date:   Wed, 13 Jan 2021 09:35:53 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Philipp Rosenberger <p.rosenberger@...bus.com>
Cc:     linux-rtc@...r.kernel.org, dan.carpenter@...cle.com,
        biwen.li@....com, lvb@...hos.com, bruno.thomsen@...il.com,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] rtc: pcf2127: Disable Power-On Reset Override

On Wed, Jan 13, 2021 at 09:18:31AM +0100, Philipp Rosenberger wrote:
> Hi Uwe,
> 
> On 12.01.21 20:26, Uwe Kleine-König wrote:
> > 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).
> 
> Yes I guess the commit log and the comment are not good enough. I took me a
> long time to find what was wrong with my setup until I realized the the PORO
> was the problem. I find the description in the manual not very clear. But
> from my tests and from the description in Table 7 Bit 3 it is pretty clear
> that the PORO bit should not be set during normal operation.

Ah, I misunderstood your intention. I though you want to enable this
bit, after rereading the commit log and patch I don't know why though. I
think the part "If the PCF2127/2129 has lost all power and is then
powered again it goes into "Power-On Reset Override" mode." is wrong in
general. So there is still something to improve (which we seem to agree
on). So I look forward to your v2.

Best regards
Uwe

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists