linux-kernel - Re: [PATCH 1/2] rtc: pcf2127: Disable Power-On Reset Override

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8bd2f360-c43d-10a4-58ef-804833de4779@kunbus.com>
Date:   Wed, 13 Jan 2021 09:18:31 +0100
From:   Philipp Rosenberger <p.rosenberger@...bus.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
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

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.

| Power-On Reset Override (PORO) facility disabled;
|  *set logic 0 for normal operation*

My tests have shown, that as long as the bit is set not interrupts are 
generated on the interrupt pin. So the POR_OVRD bit needs to be cleared 
for normal operation. At least that is my understanding.

If nobody disagrees with my assumption I would try to reword the commit 
log and the comment for a v2.

> Best regards
> Uwe
> 

Best regards,
Philipp