PCF2127/29/31 is capable of generating an interrupt on every
second (SI) or minute (MI) change. It signals this through
the Minute/Second Flag (MSF) as well, which needs to be cleared.
Cc: Szentendrei, Tamás <[email protected]>
Cc: Richard Cochran <[email protected]>
Signed-off-by: Csókás, Bence <[email protected]>
---
drivers/rtc/rtc-pcf2127.c | 49 +++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 9c04c4e1a49c..352ea12c15b9 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -27,10 +27,15 @@
#include <linux/of_irq.h>
#include <linux/of_device.h>
#include <linux/regmap.h>
+#include <linux/pps_kernel.h>
#include <linux/watchdog.h>
/* Control register 1 */
#define PCF2127_REG_CTRL1 0x00
+/* Change interrupt. 1=seconds change, 2=minutes */
+#define PCF2127_CTRL1_MSI_MASK GENMASK(1, 0)
+#define PCF2127_BIT_CTRL1_SI BIT(0)
+#define PCF2127_BIT_CTRL1_MI BIT(1)
#define PCF2127_BIT_CTRL1_POR_OVRD BIT(3)
#define PCF2127_BIT_CTRL1_TSF1 BIT(4)
#define PCF2127_BIT_CTRL1_STOP BIT(5)
@@ -41,6 +46,7 @@
#define PCF2127_BIT_CTRL2_AF BIT(4)
#define PCF2127_BIT_CTRL2_TSF2 BIT(5)
#define PCF2127_BIT_CTRL2_WDTF BIT(6)
+#define PCF2127_BIT_CTRL2_MSF BIT(7)
/* Control register 3 */
#define PCF2127_REG_CTRL3 0x02
#define PCF2127_BIT_CTRL3_BLIE BIT(0)
@@ -92,6 +98,7 @@
/* Mask for currently enabled interrupts */
#define PCF2127_CTRL1_IRQ_MASK (PCF2127_BIT_CTRL1_TSF1)
#define PCF2127_CTRL2_IRQ_MASK ( \
+ PCF2127_CTRL1_MSI_MASK | \
PCF2127_BIT_CTRL2_AF | \
PCF2127_BIT_CTRL2_WDTF | \
PCF2127_BIT_CTRL2_TSF2)
@@ -143,6 +150,7 @@
#define PCF2131_BIT_INT_SI BIT(4)
#define PCF2131_BIT_INT_MI BIT(5)
#define PCF2131_CTRL2_IRQ_MASK ( \
+ PCF2127_CTRL1_MSI_MASK | \
PCF2127_BIT_CTRL2_AF | \
PCF2127_BIT_CTRL2_WDTF)
#define PCF2131_CTRL4_IRQ_MASK ( \
@@ -207,6 +215,7 @@ struct pcf2127 {
bool irq_enabled;
time64_t ts[PCF2127_MAX_TS_SUPPORTED]; /* Timestamp values. */
bool ts_valid[PCF2127_MAX_TS_SUPPORTED]; /* Timestamp valid indication. */
+ struct pps_device *pps;
};
/*
@@ -604,6 +613,20 @@ static int pcf2127_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
return pcf2127_rtc_alarm_irq_enable(dev, alrm->enabled);
}
+static int pcf2127_rtc_pps_irq_enable(struct device *dev, u32 enable)
+{
+ struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
+ int ret;
+
+ ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
+ PCF2127_CTRL1_MSI_MASK,
+ enable ? PCF2127_BIT_CTRL1_SI : 0);
+ if (ret)
+ return ret;
+
+ return pcf2127_wdt_active_ping(&pcf2127->wdd);
+}
+
/*
* This function reads one timestamp function data, caller is responsible for
* calling pcf2127_wdt_active_ping()
@@ -667,9 +690,13 @@ static void pcf2127_rtc_ts_snapshot(struct device *dev, int ts_id)
static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
{
struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
+ struct pps_event_time ts;
unsigned int ctrl2;
int ret = 0;
+ /* First of all we get the time stamp... */
+ pps_get_ts(&ts);
+
ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
if (ret)
return IRQ_NONE;
@@ -728,6 +755,8 @@ static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
if (ctrl2 & PCF2127_BIT_CTRL2_AF)
rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
+ else if (ctrl2 & PCF2127_BIT_CTRL2_MSF)
+ pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
pcf2127_wdt_active_ping(&pcf2127->wdd);
@@ -1159,6 +1188,26 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
set_bit(RTC_FEATURE_ALARM, pcf2127->rtc->features);
}
+ if (alarm_irq > 0 && device_property_read_bool(dev, "pps-source")) {
+ struct pps_source_info pps_info = {
+ .mode = PPS_CAPTUREASSERT | PPS_OFFSETASSERT |
+ PPS_ECHOASSERT | PPS_CANWAIT | PPS_TSFMT_TSPEC,
+ .owner = THIS_MODULE,
+ };
+
+ snprintf(&pps_info.name, PPS_MAX_NAME_LEN - 1, "%s", dev_name(dev));
+
+ pcf2127->pps = pps_register_source(&pps_info, PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
+ if (IS_ERR(pcf2127->pps)) {
+ dev_err(dev, "failed to register PPS source\n");
+ return PTR_ERR(pcf2127->pps);
+ }
+
+ ret = pcf2127_rtc_pps_irq_enable(dev, TRUE);
+ if (ret)
+ return ret;
+ }
+
if (pcf2127->cfg->has_int_a_b) {
/* Configure int A/B pins, independently of alarm_irq. */
ret = pcf2127_configure_interrupt_pins(dev);
--
2.34.1
(adding Miroslav onto CC)
On Tue, Jun 11, 2024 at 05:04:57PM +0200, Cs?k?s, Bence wrote:
> PCF2127/29/31 is capable of generating an interrupt on every
> second (SI) or minute (MI) change. It signals this through
> the Minute/Second Flag (MSF) as well, which needs to be cleared.
This is a RFC, and my comment is that a PPS from an RTC is not useful
to the Linux kernel.
The kernel only uses the RTC to boot strap the wall clock to some
approximate phase.
After that, Linux either continues with a free running clock, or it
synchronizes to a global time source via NTP. In the latter case,
Linux will write the NTP time back into the RTC.
So I can't see how the RTC's PPS provides any benefit.
Thanks,
Richard
On Tue, Jun 11, 2024 at 10:06:39PM -0700, Richard Cochran wrote:
> On Tue, Jun 11, 2024 at 05:04:57PM +0200, Cs?k?s, Bence wrote:
>
> > PCF2127/29/31 is capable of generating an interrupt on every
> > second (SI) or minute (MI) change. It signals this through
> > the Minute/Second Flag (MSF) as well, which needs to be cleared.
>
> This is a RFC, and my comment is that a PPS from an RTC is not useful
> to the Linux kernel.
I think a TCXO-based RTC can be useful to user space to improve
holdover performance with NTP/PTP. There already is the RTC_UIE_ON
ioctl to enable interrupts and receive them in user space.
The advantage of the PPS device over the ioctl would be more accurate
timestamping (kernel vs user-space). Should PPS be supported, it would
be nice if it worked generally with all drivers that support RTC_UIE_ON.
--
Miroslav Lichvar
On 6/12/24 09:50, Miroslav Lichvar wrote:
> On Tue, Jun 11, 2024 at 10:06:39PM -0700, Richard Cochran wrote:
>> On Tue, Jun 11, 2024 at 05:04:57PM +0200, Csókás, Bence wrote:
>>
>>> PCF2127/29/31 is capable of generating an interrupt on every
>>> second (SI) or minute (MI) change. It signals this through
>>> the Minute/Second Flag (MSF) as well, which needs to be cleared.
>>
>> This is a RFC, and my comment is that a PPS from an RTC is not useful
>> to the Linux kernel.
>
> I think a TCXO-based RTC can be useful to user space to improve
> holdover performance with NTP/PTP.
Exactly.
> There already is the RTC_UIE_ON
> ioctl to enable interrupts and receive them in user space.
>
> The advantage of the PPS device over the ioctl would be more accurate
> timestamping (kernel vs user-space). Should PPS be supported, it would
> be nice if it worked generally with all drivers that support RTC_UIE_ON.
As we've discussed in v1, UIE hardware support is being removed from the
RTC subsystem, which I tried to optionally re-introduce. Since there was
no response since then, I assumed that there is no willingness to do
that, so I chose the next best option, the PPS subsystem.
On 5/28/24 19:56, Alexandre Belloni wrote:
> This has been removed from the kernel 13 years ago. What is your use
> case to reintroduce it?
I also agree that multiple RTCs would benefit from this feature.
However, we should only add it to those which *have* hardware support
for a "one second has elapsed" signal. UIE is currently implemented by
setting an alarm to the next second, which didn't work well with the
PCF2129.
Bence
On 12/06/2024 11:16:02+0200, Cs?k?s Bence wrote:
> On 6/12/24 09:50, Miroslav Lichvar wrote:
> > On Tue, Jun 11, 2024 at 10:06:39PM -0700, Richard Cochran wrote:
> > > On Tue, Jun 11, 2024 at 05:04:57PM +0200, Cs?k?s, Bence wrote:
> > >
> > > > PCF2127/29/31 is capable of generating an interrupt on every
> > > > second (SI) or minute (MI) change. It signals this through
> > > > the Minute/Second Flag (MSF) as well, which needs to be cleared.
> > >
> > > This is a RFC, and my comment is that a PPS from an RTC is not useful
> > > to the Linux kernel.
> >
> > I think a TCXO-based RTC can be useful to user space to improve
> > holdover performance with NTP/PTP.
>
> Exactly.
>
> > There already is the RTC_UIE_ON
> > ioctl to enable interrupts and receive them in user space.
> >
> > The advantage of the PPS device over the ioctl would be more accurate
> > timestamping (kernel vs user-space). Should PPS be supported, it would
> > be nice if it worked generally with all drivers that support RTC_UIE_ON.
>
> As we've discussed in v1, UIE hardware support is being removed from the RTC
> subsystem, which I tried to optionally re-introduce. Since there was no
> response since then, I assumed that there is no willingness to do that, so I
> chose the next best option, the PPS subsystem.
I won't reintroduce UIE but I'm going to fix the issue you see with the
pcf2129.
>
> On 5/28/24 19:56, Alexandre Belloni wrote:
> > This has been removed from the kernel 13 years ago. What is your use
> > case to reintroduce it?
>
> I also agree that multiple RTCs would benefit from this feature. However, we
> should only add it to those which *have* hardware support for a "one second
> has elapsed" signal. UIE is currently implemented by setting an alarm to the
> next second, which didn't work well with the PCF2129.
I agree with Miroslav that if done, this should be subsystem wise and
not just for individual drivers.
>
> Bence
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Wed, Jun 12, 2024 at 11:16:02AM +0200, Cs?k?s Bence wrote:
> On 6/12/24 09:50, Miroslav Lichvar wrote:
> > I think a TCXO-based RTC can be useful to user space to improve
> > holdover performance with NTP/PTP.
>
> Exactly.
Oh, the fact that the RTC has a TCXO is useful context to explain your
motivation and justify the need for this change.
It wouldn't hurt to include that tidbit in the commit message.
Thanks,
Richard