2023-01-20 19:27:06

by Doug Berger

[permalink] [raw]
Subject: [PATCH 0/6] rtc: brcmstb-waketimer: add RTC alarm irq

Support is added for an interrupt that can be triggered from the
brcmstb-waketimer hardware while the system is awake.

This interrupt allows the driver to pass the rtctest selftest.

Doug Berger (6):
rtc: brcmstb-waketimer: introduce WKTMR_ALARM_EVENT flag
rtc: brcmstb-waketimer: non-functional code changes
rtc: brcmstb-waketimer: compensate for lack of wktmr disable
rtc: brcmstb-waketimer: rename irq to wake_irq
dt-bindings: rtc: brcm,brcmstb-waketimer: add alarm interrupt
rtc: brcmstb-waketimer: allow use as non-wake alarm

.../bindings/rtc/brcm,brcmstb-waketimer.yaml | 22 ++-
drivers/rtc/rtc-brcmstb-waketimer.c | 152 +++++++++++++-----
2 files changed, 130 insertions(+), 44 deletions(-)

--
2.25.1


2023-01-20 19:59:56

by Doug Berger

[permalink] [raw]
Subject: [PATCH 2/6] rtc: brcmstb-waketimer: non-functional code changes

These changes are not intended to affect functionality, but
simplify the source code. They are performed here to simplify
review and reduce confusion with other changes in this set.

Since set_alarm includes the alarm_irq_enable functionality call
it directly from that function for simplicity (even though it
does nothing at the moment). The order of the declarations is
changed to prevent the need for a prototype.

The function device_init_wakeup() is used to replace the
functions device_set_wakeup_capable() and device_wakeup_enable()
since it is equivalent.

Signed-off-by: Doug Berger <[email protected]>
---
drivers/rtc/rtc-brcmstb-waketimer.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/rtc/rtc-brcmstb-waketimer.c b/drivers/rtc/rtc-brcmstb-waketimer.c
index fbeb8be6664b..582c23793550 100644
--- a/drivers/rtc/rtc-brcmstb-waketimer.c
+++ b/drivers/rtc/rtc-brcmstb-waketimer.c
@@ -169,6 +169,16 @@ static int brcmstb_waketmr_getalarm(struct device *dev,
return 0;
}

+/*
+ * Does not do much but keep the RTC class happy. We always support
+ * alarms.
+ */
+static int brcmstb_waketmr_alarm_enable(struct device *dev,
+ unsigned int enabled)
+{
+ return 0;
+}
+
static int brcmstb_waketmr_setalarm(struct device *dev,
struct rtc_wkalrm *alarm)
{
@@ -182,17 +192,7 @@ static int brcmstb_waketmr_setalarm(struct device *dev,

brcmstb_waketmr_set_alarm(timer, sec);

- return 0;
-}
-
-/*
- * Does not do much but keep the RTC class happy. We always support
- * alarms.
- */
-static int brcmstb_waketmr_alarm_enable(struct device *dev,
- unsigned int enabled)
-{
- return 0;
+ return brcmstb_waketmr_alarm_enable(dev, alarm->enabled);
}

static const struct rtc_class_ops brcmstb_waketmr_ops = {
@@ -228,8 +228,7 @@ static int brcmstb_waketmr_probe(struct platform_device *pdev)
* Set wakeup capability before requesting wakeup interrupt, so we can
* process boot-time "wakeups" (e.g., from S5 soft-off)
*/
- device_set_wakeup_capable(dev, true);
- device_wakeup_enable(dev);
+ device_init_wakeup(dev, true);

timer->irq = platform_get_irq(pdev, 0);
if (timer->irq < 0)
--
2.25.1

2023-01-20 20:07:16

by Doug Berger

[permalink] [raw]
Subject: [PATCH 1/6] rtc: brcmstb-waketimer: introduce WKTMR_ALARM_EVENT flag

This commit defines bit 0 as the bit of interest within the
BRCMSTB_WKTMR_EVENT register to make the implementation more
readable.

Signed-off-by: Doug Berger <[email protected]>
---
drivers/rtc/rtc-brcmstb-waketimer.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-brcmstb-waketimer.c b/drivers/rtc/rtc-brcmstb-waketimer.c
index c74130e8f496..fbeb8be6664b 100644
--- a/drivers/rtc/rtc-brcmstb-waketimer.c
+++ b/drivers/rtc/rtc-brcmstb-waketimer.c
@@ -34,6 +34,7 @@ struct brcmstb_waketmr {
};

#define BRCMSTB_WKTMR_EVENT 0x00
+#define WKTMR_ALARM_EVENT BIT(0)
#define BRCMSTB_WKTMR_COUNTER 0x04
#define BRCMSTB_WKTMR_ALARM 0x08
#define BRCMSTB_WKTMR_PRESCALER 0x0C
@@ -41,9 +42,17 @@ struct brcmstb_waketmr {

#define BRCMSTB_WKTMR_DEFAULT_FREQ 27000000

+static inline bool brcmstb_waketmr_is_pending(struct brcmstb_waketmr *timer)
+{
+ u32 reg;
+
+ reg = readl_relaxed(timer->base + BRCMSTB_WKTMR_EVENT);
+ return !!(reg & WKTMR_ALARM_EVENT);
+}
+
static inline void brcmstb_waketmr_clear_alarm(struct brcmstb_waketmr *timer)
{
- writel_relaxed(1, timer->base + BRCMSTB_WKTMR_EVENT);
+ writel_relaxed(WKTMR_ALARM_EVENT, timer->base + BRCMSTB_WKTMR_EVENT);
(void)readl_relaxed(timer->base + BRCMSTB_WKTMR_EVENT);
}

@@ -147,7 +156,6 @@ static int brcmstb_waketmr_getalarm(struct device *dev,
{
struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
time64_t sec;
- u32 reg;

sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_ALARM);
if (sec != 0) {
@@ -156,8 +164,7 @@ static int brcmstb_waketmr_getalarm(struct device *dev,
rtc_time64_to_tm(sec, &alarm->time);
}

- reg = readl_relaxed(timer->base + BRCMSTB_WKTMR_EVENT);
- alarm->pending = !!(reg & 1);
+ alarm->pending = brcmstb_waketmr_is_pending(timer);

return 0;
}
--
2.25.1

2023-01-20 20:07:17

by Doug Berger

[permalink] [raw]
Subject: [PATCH 3/6] rtc: brcmstb-waketimer: compensate for lack of wktmr disable

Since the WKTMR hardware block cannot be disabled it is necessary
for the driver to accommodate for associated timing hazards. This
commit targets the following possibilities:

A possible race between clearing a wktmr event and the alarm expiring
is made one-sided by setting the alarm to its maximum value before
clearing the event.

Programming alarm values close to the current time may not trigger
events if the counter advances while the alarm is being programmed.
After programming an alarm, a check is made to ensure that it is
either in the future or an expiration event is pending.

Signed-off-by: Doug Berger <[email protected]>
---
drivers/rtc/rtc-brcmstb-waketimer.c | 52 +++++++++++++++++++----------
1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/rtc/rtc-brcmstb-waketimer.c b/drivers/rtc/rtc-brcmstb-waketimer.c
index 582c23793550..c791e92532b8 100644
--- a/drivers/rtc/rtc-brcmstb-waketimer.c
+++ b/drivers/rtc/rtc-brcmstb-waketimer.c
@@ -31,6 +31,8 @@ struct brcmstb_waketmr {
struct notifier_block reboot_notifier;
struct clk *clk;
u32 rate;
+ unsigned long rtc_alarm;
+ bool alarm_en;
};

#define BRCMSTB_WKTMR_EVENT 0x00
@@ -52,6 +54,11 @@ static inline bool brcmstb_waketmr_is_pending(struct brcmstb_waketmr *timer)

static inline void brcmstb_waketmr_clear_alarm(struct brcmstb_waketmr *timer)
{
+ u32 reg;
+
+ timer->alarm_en = false;
+ reg = readl_relaxed(timer->base + BRCMSTB_WKTMR_COUNTER);
+ writel_relaxed(reg - 1, timer->base + BRCMSTB_WKTMR_ALARM);
writel_relaxed(WKTMR_ALARM_EVENT, timer->base + BRCMSTB_WKTMR_EVENT);
(void)readl_relaxed(timer->base + BRCMSTB_WKTMR_EVENT);
}
@@ -59,12 +66,22 @@ static inline void brcmstb_waketmr_clear_alarm(struct brcmstb_waketmr *timer)
static void brcmstb_waketmr_set_alarm(struct brcmstb_waketmr *timer,
unsigned int secs)
{
+ unsigned int now;
+
brcmstb_waketmr_clear_alarm(timer);

/* Make sure we are actually counting in seconds */
writel_relaxed(timer->rate, timer->base + BRCMSTB_WKTMR_PRESCALER);

- writel_relaxed(secs + 1, timer->base + BRCMSTB_WKTMR_ALARM);
+ writel_relaxed(secs, timer->base + BRCMSTB_WKTMR_ALARM);
+ now = readl_relaxed(timer->base + BRCMSTB_WKTMR_COUNTER);
+
+ while ((int)(secs - now) <= 0 &&
+ !brcmstb_waketmr_is_pending(timer)) {
+ secs = now + 1;
+ writel_relaxed(secs, timer->base + BRCMSTB_WKTMR_ALARM);
+ now = readl_relaxed(timer->base + BRCMSTB_WKTMR_COUNTER);
+ }
}

static irqreturn_t brcmstb_waketmr_irq(int irq, void *data)
@@ -155,27 +172,30 @@ static int brcmstb_waketmr_getalarm(struct device *dev,
struct rtc_wkalrm *alarm)
{
struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
- time64_t sec;

- sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_ALARM);
- if (sec != 0) {
- /* Alarm is enabled */
- alarm->enabled = 1;
- rtc_time64_to_tm(sec, &alarm->time);
- }
+ alarm->enabled = timer->alarm_en;
+ rtc_time64_to_tm(timer->rtc_alarm, &alarm->time);

alarm->pending = brcmstb_waketmr_is_pending(timer);

return 0;
}

-/*
- * Does not do much but keep the RTC class happy. We always support
- * alarms.
- */
static int brcmstb_waketmr_alarm_enable(struct device *dev,
unsigned int enabled)
{
+ struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
+
+ if (enabled && !timer->alarm_en) {
+ if ((int)(readl_relaxed(timer->base + BRCMSTB_WKTMR_COUNTER) -
+ readl_relaxed(timer->base + BRCMSTB_WKTMR_ALARM)) >= 0 &&
+ !brcmstb_waketmr_is_pending(timer))
+ return -EINVAL;
+ timer->alarm_en = true;
+ } else if (!enabled && timer->alarm_en) {
+ timer->alarm_en = false;
+ }
+
return 0;
}

@@ -183,14 +203,10 @@ static int brcmstb_waketmr_setalarm(struct device *dev,
struct rtc_wkalrm *alarm)
{
struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
- time64_t sec;

- if (alarm->enabled)
- sec = rtc_tm_to_time64(&alarm->time);
- else
- sec = 0;
+ timer->rtc_alarm = rtc_tm_to_time64(&alarm->time);

- brcmstb_waketmr_set_alarm(timer, sec);
+ brcmstb_waketmr_set_alarm(timer, timer->rtc_alarm);

return brcmstb_waketmr_alarm_enable(dev, alarm->enabled);
}
--
2.25.1

2023-01-23 19:20:49

by Florian Fainelli

[permalink] [raw]
Subject: Re: [PATCH 1/6] rtc: brcmstb-waketimer: introduce WKTMR_ALARM_EVENT flag

On 1/20/23 11:01, Doug Berger wrote:
> This commit defines bit 0 as the bit of interest within the
> BRCMSTB_WKTMR_EVENT register to make the implementation more
> readable.
>
> Signed-off-by: Doug Berger <[email protected]>

Acked-by: Florian Fainelli <[email protected]>
--
Florian


2023-01-23 19:21:13

by Florian Fainelli

[permalink] [raw]
Subject: Re: [PATCH 2/6] rtc: brcmstb-waketimer: non-functional code changes

On 1/20/23 11:01, Doug Berger wrote:
> These changes are not intended to affect functionality, but
> simplify the source code. They are performed here to simplify
> review and reduce confusion with other changes in this set.
>
> Since set_alarm includes the alarm_irq_enable functionality call
> it directly from that function for simplicity (even though it
> does nothing at the moment). The order of the declarations is
> changed to prevent the need for a prototype.
>
> The function device_init_wakeup() is used to replace the
> functions device_set_wakeup_capable() and device_wakeup_enable()
> since it is equivalent.
>
> Signed-off-by: Doug Berger <[email protected]>

Acked-by: Florian Fainelli <[email protected]>
--
Florian


2023-01-23 19:21:45

by Florian Fainelli

[permalink] [raw]
Subject: Re: [PATCH 3/6] rtc: brcmstb-waketimer: compensate for lack of wktmr disable

On 1/20/23 11:01, Doug Berger wrote:
> Since the WKTMR hardware block cannot be disabled it is necessary
> for the driver to accommodate for associated timing hazards. This
> commit targets the following possibilities:
>
> A possible race between clearing a wktmr event and the alarm expiring
> is made one-sided by setting the alarm to its maximum value before
> clearing the event.
>
> Programming alarm values close to the current time may not trigger
> events if the counter advances while the alarm is being programmed.
> After programming an alarm, a check is made to ensure that it is
> either in the future or an expiration event is pending.
>
> Signed-off-by: Doug Berger <[email protected]>

Acked-by: Florian Fainelli <[email protected]>
--
Florian


2023-01-23 23:08:47

by Alexandre Belloni

[permalink] [raw]
Subject: Re: (subset) [PATCH 0/6] rtc: brcmstb-waketimer: add RTC alarm irq


On Fri, 20 Jan 2023 11:01:41 -0800, Doug Berger wrote:
> Support is added for an interrupt that can be triggered from the
> brcmstb-waketimer hardware while the system is awake.
>
> This interrupt allows the driver to pass the rtctest selftest.
>
> Doug Berger (6):
> rtc: brcmstb-waketimer: introduce WKTMR_ALARM_EVENT flag
> rtc: brcmstb-waketimer: non-functional code changes
> rtc: brcmstb-waketimer: compensate for lack of wktmr disable
> rtc: brcmstb-waketimer: rename irq to wake_irq
> dt-bindings: rtc: brcm,brcmstb-waketimer: add alarm interrupt
> rtc: brcmstb-waketimer: allow use as non-wake alarm
>
> [...]

Applied, thanks!

[1/6] rtc: brcmstb-waketimer: introduce WKTMR_ALARM_EVENT flag
commit: 90226f6b17a3edcb0bddaf2f16991861c99d6a15
[2/6] rtc: brcmstb-waketimer: non-functional code changes
commit: 2cd98b22c1443d1f2921a371baee658da184868e
[3/6] rtc: brcmstb-waketimer: compensate for lack of wktmr disable
commit: 516ae02c38ff3ae867f9b19fa050f78157e2bdae
[4/6] rtc: brcmstb-waketimer: rename irq to wake_irq
commit: eae258edcb8705932c9e5c61a99f91d8235f688b

--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2023-01-24 17:42:33

by Florian Fainelli

[permalink] [raw]
Subject: Re: (subset) [PATCH 0/6] rtc: brcmstb-waketimer: add RTC alarm irq

On 1/23/23 15:08, 'Alexandre Belloni' via BCM-KERNEL-FEEDBACK-LIST,PDL
wrote:
>
> On Fri, 20 Jan 2023 11:01:41 -0800, Doug Berger wrote:
>> Support is added for an interrupt that can be triggered from the
>> brcmstb-waketimer hardware while the system is awake.
>>
>> This interrupt allows the driver to pass the rtctest selftest.
>>
>> Doug Berger (6):
>> rtc: brcmstb-waketimer: introduce WKTMR_ALARM_EVENT flag
>> rtc: brcmstb-waketimer: non-functional code changes
>> rtc: brcmstb-waketimer: compensate for lack of wktmr disable
>> rtc: brcmstb-waketimer: rename irq to wake_irq
>> dt-bindings: rtc: brcm,brcmstb-waketimer: add alarm interrupt
>> rtc: brcmstb-waketimer: allow use as non-wake alarm
>>
>> [...]
>
> Applied, thanks!
>
> [1/6] rtc: brcmstb-waketimer: introduce WKTMR_ALARM_EVENT flag
> commit: 90226f6b17a3edcb0bddaf2f16991861c99d6a15
> [2/6] rtc: brcmstb-waketimer: non-functional code changes
> commit: 2cd98b22c1443d1f2921a371baee658da184868e
> [3/6] rtc: brcmstb-waketimer: compensate for lack of wktmr disable
> commit: 516ae02c38ff3ae867f9b19fa050f78157e2bdae
> [4/6] rtc: brcmstb-waketimer: rename irq to wake_irq
> commit: eae258edcb8705932c9e5c61a99f91d8235f688b

That was quick, how about patch 6? It does not actually have a
dependency on the Device Tree binding (patch 5) and the second interrupt
is looked up by index.
--
Florian


2023-01-24 18:46:26

by Alexandre Belloni

[permalink] [raw]
Subject: Re: (subset) [PATCH 0/6] rtc: brcmstb-waketimer: add RTC alarm irq

On 24/01/2023 09:42:19-0800, Florian Fainelli wrote:
> On 1/23/23 15:08, 'Alexandre Belloni' via BCM-KERNEL-FEEDBACK-LIST,PDL
> wrote:
> >
> > On Fri, 20 Jan 2023 11:01:41 -0800, Doug Berger wrote:
> > > Support is added for an interrupt that can be triggered from the
> > > brcmstb-waketimer hardware while the system is awake.
> > >
> > > This interrupt allows the driver to pass the rtctest selftest.
> > >
> > > Doug Berger (6):
> > > rtc: brcmstb-waketimer: introduce WKTMR_ALARM_EVENT flag
> > > rtc: brcmstb-waketimer: non-functional code changes
> > > rtc: brcmstb-waketimer: compensate for lack of wktmr disable
> > > rtc: brcmstb-waketimer: rename irq to wake_irq
> > > dt-bindings: rtc: brcm,brcmstb-waketimer: add alarm interrupt
> > > rtc: brcmstb-waketimer: allow use as non-wake alarm
> > >
> > > [...]
> >
> > Applied, thanks!
> >
> > [1/6] rtc: brcmstb-waketimer: introduce WKTMR_ALARM_EVENT flag
> > commit: 90226f6b17a3edcb0bddaf2f16991861c99d6a15
> > [2/6] rtc: brcmstb-waketimer: non-functional code changes
> > commit: 2cd98b22c1443d1f2921a371baee658da184868e
> > [3/6] rtc: brcmstb-waketimer: compensate for lack of wktmr disable
> > commit: 516ae02c38ff3ae867f9b19fa050f78157e2bdae
> > [4/6] rtc: brcmstb-waketimer: rename irq to wake_irq
> > commit: eae258edcb8705932c9e5c61a99f91d8235f688b
>
> That was quick, how about patch 6? It does not actually have a dependency on
> the Device Tree binding (patch 5) and the second interrupt is looked up by
> index.

My understanding is that if I take it, then the feature will not be
documented. I keep that as an incentive to send v2 ;)


--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com