* [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt
@ 2024-06-11 15:04 Csókás, Bence
2024-06-12 5:06 ` Richard Cochran
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Csókás, Bence @ 2024-06-11 15:04 UTC (permalink / raw)
To: linux-rtc, linux-kernel
Cc: Csókás, Bence, Szentendrei, Tamás, Richard Cochran,
Alexandre Belloni
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 <szentendrei.tamas@prolan.hu>
Cc: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt
2024-06-11 15:04 [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt Csókás, Bence
@ 2024-06-12 5:06 ` Richard Cochran
2024-06-12 7:50 ` Miroslav Lichvar
2024-06-12 6:23 ` kernel test robot
2024-06-12 10:47 ` kernel test robot
2 siblings, 1 reply; 8+ messages in thread
From: Richard Cochran @ 2024-06-12 5:06 UTC (permalink / raw)
To: Csókás, Bence
Cc: linux-rtc, linux-kernel, Szentendrei, Tamás,
Alexandre Belloni, Miroslav Lichvar
(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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt
2024-06-11 15:04 [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt Csókás, Bence
2024-06-12 5:06 ` Richard Cochran
@ 2024-06-12 6:23 ` kernel test robot
2024-06-12 10:47 ` kernel test robot
2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-06-12 6:23 UTC (permalink / raw)
To: Csókás, Bence; +Cc: oe-kbuild-all
Hi Bence,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on linus/master v6.10-rc3 next-20240611]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cs-k-s-Bence/rtc-pcf2127-Add-PPS-capability-through-Seconds-Interrupt/20240611-231226
base: https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link: https://lore.kernel.org/r/20240611150458.684349-1-csokas.bence%40prolan.hu
patch subject: [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240612/202406121341.QCPOP2oG-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240612/202406121341.QCPOP2oG-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406121341.QCPOP2oG-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/rtc/rtc-pcf2127.c: In function 'pcf2127_rtc_irq':
>> drivers/rtc/rtc-pcf2127.c:759:27: error: 'pps' undeclared (first use in this function)
759 | pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
| ^~~
drivers/rtc/rtc-pcf2127.c:759:27: note: each undeclared identifier is reported only once for each function it appears in
drivers/rtc/rtc-pcf2127.c: In function 'pcf2127_probe':
>> drivers/rtc/rtc-pcf2127.c:1198:26: error: passing argument 1 of 'snprintf' from incompatible pointer type [-Werror=incompatible-pointer-types]
1198 | snprintf(&pps_info.name, PPS_MAX_NAME_LEN - 1, "%s", dev_name(dev));
| ^~~~~~~~~~~~~~
| |
| char (*)[32]
In file included from include/linux/kernel.h:33,
from arch/arm64/include/asm/cpufeature.h:26,
from arch/arm64/include/asm/ptrace.h:11,
from arch/arm64/include/asm/irqflags.h:10,
from include/linux/irqflags.h:18,
from include/linux/spinlock.h:59,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/slab.h:16,
from include/linux/resource_ext.h:11,
from include/linux/acpi.h:13,
from include/linux/i2c.h:13,
from drivers/rtc/rtc-pcf2127.c:20:
include/linux/sprintf.h:12:35: note: expected 'char *' but argument is of type 'char (*)[32]'
12 | __printf(3, 4) int snprintf(char *buf, size_t size, const char *fmt, ...);
| ~~~~~~^~~
cc1: some warnings being treated as errors
vim +/pps +759 drivers/rtc/rtc-pcf2127.c
689
690 static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
691 {
692 struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
693 struct pps_event_time ts;
694 unsigned int ctrl2;
695 int ret = 0;
696
697 /* First of all we get the time stamp... */
698 pps_get_ts(&ts);
699
700 ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
701 if (ret)
702 return IRQ_NONE;
703
704 if (pcf2127->cfg->ts_count == 1) {
705 /* PCF2127/29 */
706 unsigned int ctrl1;
707
708 ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL1, &ctrl1);
709 if (ret)
710 return IRQ_NONE;
711
712 if (!(ctrl1 & PCF2127_CTRL1_IRQ_MASK || ctrl2 & PCF2127_CTRL2_IRQ_MASK))
713 return IRQ_NONE;
714
715 if (ctrl1 & PCF2127_BIT_CTRL1_TSF1 || ctrl2 & PCF2127_BIT_CTRL2_TSF2)
716 pcf2127_rtc_ts_snapshot(dev, 0);
717
718 if (ctrl1 & PCF2127_CTRL1_IRQ_MASK)
719 regmap_write(pcf2127->regmap, PCF2127_REG_CTRL1,
720 ctrl1 & ~PCF2127_CTRL1_IRQ_MASK);
721
722 if (ctrl2 & PCF2127_CTRL2_IRQ_MASK)
723 regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
724 ctrl2 & ~PCF2127_CTRL2_IRQ_MASK);
725 } else {
726 /* PCF2131. */
727 unsigned int ctrl4;
728
729 ret = regmap_read(pcf2127->regmap, PCF2131_REG_CTRL4, &ctrl4);
730 if (ret)
731 return IRQ_NONE;
732
733 if (!(ctrl4 & PCF2131_CTRL4_IRQ_MASK || ctrl2 & PCF2131_CTRL2_IRQ_MASK))
734 return IRQ_NONE;
735
736 if (ctrl4 & PCF2131_CTRL4_IRQ_MASK) {
737 int i;
738 int tsf_bit = PCF2131_BIT_CTRL4_TSF1; /* Start at bit 7. */
739
740 for (i = 0; i < pcf2127->cfg->ts_count; i++) {
741 if (ctrl4 & tsf_bit)
742 pcf2127_rtc_ts_snapshot(dev, i);
743
744 tsf_bit = tsf_bit >> 1;
745 }
746
747 regmap_write(pcf2127->regmap, PCF2131_REG_CTRL4,
748 ctrl4 & ~PCF2131_CTRL4_IRQ_MASK);
749 }
750
751 if (ctrl2 & PCF2131_CTRL2_IRQ_MASK)
752 regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
753 ctrl2 & ~PCF2131_CTRL2_IRQ_MASK);
754 }
755
756 if (ctrl2 & PCF2127_BIT_CTRL2_AF)
757 rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
758 else if (ctrl2 & PCF2127_BIT_CTRL2_MSF)
> 759 pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
760
761 pcf2127_wdt_active_ping(&pcf2127->wdd);
762
763 return IRQ_HANDLED;
764 }
765
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt
2024-06-12 5:06 ` Richard Cochran
@ 2024-06-12 7:50 ` Miroslav Lichvar
2024-06-12 9:16 ` Csókás Bence
0 siblings, 1 reply; 8+ messages in thread
From: Miroslav Lichvar @ 2024-06-12 7:50 UTC (permalink / raw)
To: Richard Cochran
Cc: Csókás, Bence, linux-rtc, linux-kernel,
Szentendrei, Tamás, Alexandre Belloni
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt
2024-06-12 7:50 ` Miroslav Lichvar
@ 2024-06-12 9:16 ` Csókás Bence
2024-06-12 11:01 ` Alexandre Belloni
2024-06-13 3:25 ` Richard Cochran
0 siblings, 2 replies; 8+ messages in thread
From: Csókás Bence @ 2024-06-12 9:16 UTC (permalink / raw)
To: Miroslav Lichvar, Richard Cochran
Cc: linux-rtc, linux-kernel, Szentendrei, Tamás,
Alexandre Belloni
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt
2024-06-11 15:04 [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt Csókás, Bence
2024-06-12 5:06 ` Richard Cochran
2024-06-12 6:23 ` kernel test robot
@ 2024-06-12 10:47 ` kernel test robot
2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-06-12 10:47 UTC (permalink / raw)
To: Csókás, Bence; +Cc: llvm, oe-kbuild-all
Hi Bence,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on linus/master v6.10-rc3 next-20240612]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cs-k-s-Bence/rtc-pcf2127-Add-PPS-capability-through-Seconds-Interrupt/20240611-231226
base: https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link: https://lore.kernel.org/r/20240611150458.684349-1-csokas.bence%40prolan.hu
patch subject: [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240612/202406121833.ard7iXAi-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 4403cdbaf01379de96f8d0d6ea4f51a085e37766)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240612/202406121833.ard7iXAi-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406121833.ard7iXAi-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/rtc/rtc-pcf2127.c:20:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/rtc/rtc-pcf2127.c:20:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/rtc/rtc-pcf2127.c:20:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
In file included from drivers/rtc/rtc-pcf2127.c:20:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2208:
include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/rtc/rtc-pcf2127.c:759:13: error: use of undeclared identifier 'pps'
759 | pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
| ^
>> drivers/rtc/rtc-pcf2127.c:1198:12: error: incompatible pointer types passing 'char (*)[32]' to parameter of type 'char *' [-Werror,-Wincompatible-pointer-types]
1198 | snprintf(&pps_info.name, PPS_MAX_NAME_LEN - 1, "%s", dev_name(dev));
| ^~~~~~~~~~~~~~
include/linux/sprintf.h:12:35: note: passing argument to parameter 'buf' here
12 | __printf(3, 4) int snprintf(char *buf, size_t size, const char *fmt, ...);
| ^
7 warnings and 2 errors generated.
vim +/pps +759 drivers/rtc/rtc-pcf2127.c
689
690 static irqreturn_t pcf2127_rtc_irq(int irq, void *dev)
691 {
692 struct pcf2127 *pcf2127 = dev_get_drvdata(dev);
693 struct pps_event_time ts;
694 unsigned int ctrl2;
695 int ret = 0;
696
697 /* First of all we get the time stamp... */
698 pps_get_ts(&ts);
699
700 ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL2, &ctrl2);
701 if (ret)
702 return IRQ_NONE;
703
704 if (pcf2127->cfg->ts_count == 1) {
705 /* PCF2127/29 */
706 unsigned int ctrl1;
707
708 ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL1, &ctrl1);
709 if (ret)
710 return IRQ_NONE;
711
712 if (!(ctrl1 & PCF2127_CTRL1_IRQ_MASK || ctrl2 & PCF2127_CTRL2_IRQ_MASK))
713 return IRQ_NONE;
714
715 if (ctrl1 & PCF2127_BIT_CTRL1_TSF1 || ctrl2 & PCF2127_BIT_CTRL2_TSF2)
716 pcf2127_rtc_ts_snapshot(dev, 0);
717
718 if (ctrl1 & PCF2127_CTRL1_IRQ_MASK)
719 regmap_write(pcf2127->regmap, PCF2127_REG_CTRL1,
720 ctrl1 & ~PCF2127_CTRL1_IRQ_MASK);
721
722 if (ctrl2 & PCF2127_CTRL2_IRQ_MASK)
723 regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
724 ctrl2 & ~PCF2127_CTRL2_IRQ_MASK);
725 } else {
726 /* PCF2131. */
727 unsigned int ctrl4;
728
729 ret = regmap_read(pcf2127->regmap, PCF2131_REG_CTRL4, &ctrl4);
730 if (ret)
731 return IRQ_NONE;
732
733 if (!(ctrl4 & PCF2131_CTRL4_IRQ_MASK || ctrl2 & PCF2131_CTRL2_IRQ_MASK))
734 return IRQ_NONE;
735
736 if (ctrl4 & PCF2131_CTRL4_IRQ_MASK) {
737 int i;
738 int tsf_bit = PCF2131_BIT_CTRL4_TSF1; /* Start at bit 7. */
739
740 for (i = 0; i < pcf2127->cfg->ts_count; i++) {
741 if (ctrl4 & tsf_bit)
742 pcf2127_rtc_ts_snapshot(dev, i);
743
744 tsf_bit = tsf_bit >> 1;
745 }
746
747 regmap_write(pcf2127->regmap, PCF2131_REG_CTRL4,
748 ctrl4 & ~PCF2131_CTRL4_IRQ_MASK);
749 }
750
751 if (ctrl2 & PCF2131_CTRL2_IRQ_MASK)
752 regmap_write(pcf2127->regmap, PCF2127_REG_CTRL2,
753 ctrl2 & ~PCF2131_CTRL2_IRQ_MASK);
754 }
755
756 if (ctrl2 & PCF2127_BIT_CTRL2_AF)
757 rtc_update_irq(pcf2127->rtc, 1, RTC_IRQF | RTC_AF);
758 else if (ctrl2 & PCF2127_BIT_CTRL2_MSF)
> 759 pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
760
761 pcf2127_wdt_active_ping(&pcf2127->wdd);
762
763 return IRQ_HANDLED;
764 }
765
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt
2024-06-12 9:16 ` Csókás Bence
@ 2024-06-12 11:01 ` Alexandre Belloni
2024-06-13 3:25 ` Richard Cochran
1 sibling, 0 replies; 8+ messages in thread
From: Alexandre Belloni @ 2024-06-12 11:01 UTC (permalink / raw)
To: Csókás Bence
Cc: Miroslav Lichvar, Richard Cochran, linux-rtc, linux-kernel,
Szentendrei, Tamás
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt
2024-06-12 9:16 ` Csókás Bence
2024-06-12 11:01 ` Alexandre Belloni
@ 2024-06-13 3:25 ` Richard Cochran
1 sibling, 0 replies; 8+ messages in thread
From: Richard Cochran @ 2024-06-13 3:25 UTC (permalink / raw)
To: Csókás Bence
Cc: Miroslav Lichvar, linux-rtc, linux-kernel,
Szentendrei, Tamás, Alexandre Belloni
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-06-13 3:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-11 15:04 [RFC PATCH v2] rtc: pcf2127: Add PPS capability through Seconds Interrupt Csókás, Bence
2024-06-12 5:06 ` Richard Cochran
2024-06-12 7:50 ` Miroslav Lichvar
2024-06-12 9:16 ` Csókás Bence
2024-06-12 11:01 ` Alexandre Belloni
2024-06-13 3:25 ` Richard Cochran
2024-06-12 6:23 ` kernel test robot
2024-06-12 10:47 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.