[PATCH 0/2] Broadcom STB wake-timer support

[PATCH 0/2] Broadcom STB wake-timer support

[Florian Fainelli]

Hi Alexandre,

This patch series adds support for the Broadcom STB wake-timer. This is a 27Mhz
timer which allows the system to exit S2/S3/S5 sleep states when configured.

The wake-timer has has a long history within our internal/downstream tree, and
Brian authored it in the first place, then Markus added the rtc bits, and then I
cleaned it up for upstream.

Thanks!

Brian Norris (1):
  rtc: brcmstb-waketimer: Add Broadcom STB wake-timer

Florian Fainelli (1):
  dt-bindings: Document the Broadcom STB wake-up timer node

 .../bindings/rtc/brcm,brcmstb-waketimer.txt        |  22 ++
 drivers/rtc/Kconfig                                |  11 +
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-brcmstb-waketimer.c                | 345 +++++++++++++++++++++
 4 files changed, 379 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/brcm,brcmstb-waketimer.txt
 create mode 100644 drivers/rtc/rtc-brcmstb-waketimer.c

-- 
2.9.3

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH 1/2] dt-bindings: Document the Broadcom STB wake-up timer node

[Florian Fainelli]

Document the binding for the Broadcom STB SoCs wake-up timer node
allowing the system to generate alarms and exit low power states.

Signed-off-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../bindings/rtc/brcm,brcmstb-waketimer.txt        | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/brcm,brcmstb-waketimer.txt

diff --git a/Documentation/devicetree/bindings/rtc/brcm,brcmstb-waketimer.txt b/Documentation/devicetree/bindings/rtc/brcm,brcmstb-waketimer.txt
new file mode 100644
index 000000000000..1d990bcc0baf
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/brcm,brcmstb-waketimer.txt
@@ -0,0 +1,22 @@
+Broadcom STB wake-up Timer
+
+The Broadcom STB wake-up timer provides a 27Mhz resolution timer, with the
+ability to wake up the system from low-power suspend/standby modes.
+
+Required properties:
+- compatible     : should contain "brcm,brcmstb-waketimer"
+- reg            : the register start and length for the WKTMR block
+- interrupts     : The TIMER interrupt
+- interrupt-parent: The phandle to the Always-On (AON) Power Management (PM) L2
+                    interrupt controller node
+- clocks	 : The phandle to the UPG fixed clock (27Mhz domain)
+
+Example:
+
+waketimer@f0411580 {
+	compatible = "brcm,brcmstb-waketimer";
+	reg = <0xf0411580 0x14>;
+	interrupts = <0x3>;
+	interrupt-parent = <&aon_pm_l2_intc>;
+	clocks = <&upg_fixed>;
+};
-- 
2.9.3

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/2] dt-bindings: Document the Broadcom STB wake-up timer node

[Rob Herring]

On Thu, Jun 15, 2017 at 12:59:03PM -0700, Florian Fainelli wrote:
> Document the binding for the Broadcom STB SoCs wake-up timer node
> allowing the system to generate alarms and exit low power states.
> 
> Signed-off-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../bindings/rtc/brcm,brcmstb-waketimer.txt        | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/brcm,brcmstb-waketimer.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH 2/2] rtc: brcmstb-waketimer: Add Broadcom STB wake-timer

[Florian Fainelli]

From: Brian Norris <computersforpeace@gmail.com>

This adds support for the Broadcom STB wake-timer which is a timer in
the chip's 27Mhz clock domain that offers the ability to wake the system
(wake-up source) from suspend states (S2, S3, S5). It is supported using
the rtc framework allowing us to configure alarms for system wake-up.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Signed-off-by: Markus Mayer <mmayer@broadcom.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/rtc/Kconfig                 |  11 ++
 drivers/rtc/Makefile                |   1 +
 drivers/rtc/rtc-brcmstb-waketimer.c | 345 ++++++++++++++++++++++++++++++++++++
 3 files changed, 357 insertions(+)
 create mode 100644 drivers/rtc/rtc-brcmstb-waketimer.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 8d3b95728326..8e1aa67fe533 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -197,6 +197,17 @@ config RTC_DRV_AC100
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-ac100.
 
+config RTC_DRV_BRCMSTB
+	tristate "Broadcom STB wake-timer"
+	depends on ARCH_BRCMSTB || BMIPS_GENERIC || COMPILE_TEST
+	default ARCH_BRCMSTB || BMIPS_GENERIC
+	help
+	  If you say yes here you get support for the wake-timer found on
+	  Broadcom STB SoCs (BCM7xxx).
+
+	  This driver can also be built as a module. If so, the module will
+	  be called rtc-brcmstb-waketimer.
+
 config RTC_DRV_AS3722
 	tristate "ams AS3722 RTC driver"
 	depends on MFD_AS3722
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 13857d2fce09..df89cac1f9ae 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_RTC_DRV_AT91RM9200)+= rtc-at91rm9200.o
 obj-$(CONFIG_RTC_DRV_AT91SAM9)	+= rtc-at91sam9.o
 obj-$(CONFIG_RTC_DRV_AU1XXX)	+= rtc-au1xxx.o
 obj-$(CONFIG_RTC_DRV_BFIN)	+= rtc-bfin.o
+obj-$(CONFIG_RTC_DRV_BRCMSTB)	+= rtc-brcmstb-waketimer.o
 obj-$(CONFIG_RTC_DRV_BQ32K)	+= rtc-bq32k.o
 obj-$(CONFIG_RTC_DRV_BQ4802)	+= rtc-bq4802.o
 obj-$(CONFIG_RTC_DRV_CMOS)	+= rtc-cmos.o
diff --git a/drivers/rtc/rtc-brcmstb-waketimer.c b/drivers/rtc/rtc-brcmstb-waketimer.c
new file mode 100644
index 000000000000..c7f166e39f5e
--- /dev/null
+++ b/drivers/rtc/rtc-brcmstb-waketimer.c
@@ -0,0 +1,345 @@
+/*
+ * Copyright © 2014-2017 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irqreturn.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_wakeup.h>
+#include <linux/reboot.h>
+#include <linux/rtc.h>
+#include <linux/stat.h>
+#include <linux/suspend.h>
+
+struct brcmstb_waketmr {
+	struct rtc_device *rtc;
+	struct device *dev;
+	void __iomem *base;
+	int irq;
+	struct notifier_block reboot_notifier;
+	struct clk *clk;
+	u32 rate;
+};
+
+#define BRCMSTB_WKTMR_EVENT		0x00
+#define BRCMSTB_WKTMR_COUNTER		0x04
+#define BRCMSTB_WKTMR_ALARM		0x08
+#define BRCMSTB_WKTMR_PRESCALER		0x0C
+#define BRCMSTB_WKTMR_PRESCALER_VAL	0x10
+
+#define BRCMSTB_WKTMR_DEFAULT_FREQ	27000000
+
+static inline void brcmstb_waketmr_clear_alarm(struct brcmstb_waketmr *timer)
+{
+	writel_relaxed(1, timer->base + BRCMSTB_WKTMR_EVENT);
+	(void)readl_relaxed(timer->base + BRCMSTB_WKTMR_EVENT);
+}
+
+static void brcmstb_waketmr_set_alarm(struct brcmstb_waketmr *timer,
+				      unsigned int secs)
+{
+	brcmstb_waketmr_clear_alarm(timer);
+
+	writel_relaxed(secs + 1, timer->base + BRCMSTB_WKTMR_ALARM);
+}
+
+static irqreturn_t brcmstb_waketmr_irq(int irq, void *data)
+{
+	struct brcmstb_waketmr *timer = data;
+
+	pm_wakeup_event(timer->dev, 0);
+
+	return IRQ_HANDLED;
+}
+
+struct wktmr_time {
+	u32 sec;
+	u32 pre;
+};
+
+static void wktmr_read(struct brcmstb_waketmr *timer,
+		       struct wktmr_time *t)
+{
+	u32 tmp;
+
+	do {
+		t->sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_COUNTER);
+		tmp = readl_relaxed(timer->base + BRCMSTB_WKTMR_PRESCALER_VAL);
+	} while (tmp >= timer->rate);
+
+	t->pre = timer->rate - tmp;
+}
+
+static int brcmstb_waketmr_prepare_suspend(struct brcmstb_waketmr *timer)
+{
+	struct device *dev = timer->dev;
+	int ret = 0;
+
+	if (device_may_wakeup(dev)) {
+		dev_dbg(dev, "enable wake IRQ\n");
+		ret = enable_irq_wake(timer->irq);
+		if (ret) {
+			dev_err(dev, "failed to enable wake-up interrupt\n");
+			return ret;
+		}
+	}
+
+	return ret;
+}
+
+/* If enabled as a wakeup-source, arm the timer when powering off */
+static int brcmstb_waketmr_reboot(struct notifier_block *nb,
+		unsigned long action, void *data)
+{
+	struct brcmstb_waketmr *timer;
+
+	timer = container_of(nb, struct brcmstb_waketmr, reboot_notifier);
+
+	/* Set timer for cold boot */
+	if (action == SYS_POWER_OFF)
+		brcmstb_waketmr_prepare_suspend(timer);
+
+	return NOTIFY_DONE;
+}
+
+static int brcmstb_waketmr_gettime(struct device *dev,
+				   struct rtc_time *tm)
+{
+	struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
+	struct wktmr_time now;
+
+	wktmr_read(timer, &now);
+
+	rtc_time_to_tm(now.sec, tm);
+
+	return 0;
+}
+
+static int brcmstb_waketmr_settime(struct device *dev,
+				   struct rtc_time *tm)
+{
+	struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
+	unsigned long sec;
+	int ret;
+
+	ret = rtc_valid_tm(tm);
+	if (ret)
+		return ret;
+
+	rtc_tm_to_time(tm, &sec);
+
+	dev_dbg(dev, "%s: sec=%ld\n", __func__, sec);
+	writel_relaxed(sec, timer->base + BRCMSTB_WKTMR_COUNTER);
+
+	return 0;
+}
+
+static int brcmstb_waketmr_getalarm(struct device *dev,
+				    struct rtc_wkalrm *alarm)
+{
+	struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
+	unsigned long sec;
+	u32 reg;
+
+	sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_ALARM);
+	if (sec == 0) {
+		/* Alarm is disabled */
+		alarm->enabled = 0;
+		alarm->time.tm_mon = -1;
+		alarm->time.tm_mday = -1;
+		alarm->time.tm_year = -1;
+		alarm->time.tm_hour = -1;
+		alarm->time.tm_min = -1;
+		alarm->time.tm_sec = -1;
+		dev_dbg(dev, "%s: alarm is disabled\n", __func__);
+	} else {
+		/* Alarm is enabled */
+		alarm->enabled = 1;
+		rtc_time_to_tm(sec, &alarm->time);
+		dev_dbg(dev, "%s: alarm is enabled\n", __func__);
+	}
+
+	reg = readl_relaxed(timer->base + BRCMSTB_WKTMR_EVENT);
+	alarm->pending = !!(reg & 1);
+	dev_dbg(dev, "%s: alarm pending=%d\n", __func__, alarm->pending);
+
+	return 0;
+}
+
+static int brcmstb_waketmr_setalarm(struct device *dev,
+				     struct rtc_wkalrm *alarm)
+{
+	struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
+	unsigned long sec;
+
+	if (alarm->enabled)
+		rtc_tm_to_time(&alarm->time, &sec);
+	else
+		sec = 0;
+
+	dev_dbg(dev, "%s: timeout=%ld\n", __func__, sec);
+	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)
+{
+	dev_dbg(dev, "%s: enabled=%d\n", __func__, enabled);
+	return 0;
+}
+
+static const struct rtc_class_ops brcmstb_waketmr_ops = {
+	.read_time	= brcmstb_waketmr_gettime,
+	.set_time	= brcmstb_waketmr_settime,
+	.read_alarm	= brcmstb_waketmr_getalarm,
+	.set_alarm	= brcmstb_waketmr_setalarm,
+	.alarm_irq_enable = brcmstb_waketmr_alarm_enable,
+};
+
+static int brcmstb_waketmr_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct brcmstb_waketmr *timer;
+	struct resource *res;
+	int ret;
+
+	timer = devm_kzalloc(dev, sizeof(*timer), GFP_KERNEL);
+	if (!timer)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, timer);
+	timer->dev = dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	timer->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(timer->base))
+		return PTR_ERR(timer->base);
+
+	/*
+	 * 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);
+
+	timer->irq = platform_get_irq(pdev, 0);
+	if (timer->irq < 0)
+		return -ENODEV;
+
+	timer->clk = devm_clk_get(dev, NULL);
+	if (!IS_ERR(timer->clk)) {
+		ret = clk_prepare_enable(timer->clk);
+		if (ret)
+			return ret;
+		timer->rate = clk_get_rate(timer->clk);
+		if (!timer->rate)
+			timer->rate = BRCMSTB_WKTMR_DEFAULT_FREQ;
+	} else {
+		timer->rate = BRCMSTB_WKTMR_DEFAULT_FREQ;
+		timer->clk = NULL;
+	}
+
+	ret = devm_request_irq(dev, timer->irq, brcmstb_waketmr_irq, 0,
+			       "brcmstb-waketimer", timer);
+	if (ret < 0)
+		return ret;
+
+	timer->reboot_notifier.notifier_call = brcmstb_waketmr_reboot;
+	register_reboot_notifier(&timer->reboot_notifier);
+
+	timer->rtc = rtc_device_register("brcmstb-waketmr", dev,
+					 &brcmstb_waketmr_ops, THIS_MODULE);
+	if (IS_ERR(timer->rtc)) {
+		dev_err(dev, "unable to register device\n");
+		unregister_reboot_notifier(&timer->reboot_notifier);
+		return PTR_ERR(timer->rtc);
+	}
+
+	dev_info(dev, "registered, with irq %d\n", timer->irq);
+
+	return ret;
+}
+
+static int brcmstb_waketmr_remove(struct platform_device *pdev)
+{
+	struct brcmstb_waketmr *timer = dev_get_drvdata(&pdev->dev);
+
+	unregister_reboot_notifier(&timer->reboot_notifier);
+	rtc_device_unregister(timer->rtc);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int brcmstb_waketmr_suspend(struct device *dev)
+{
+	struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
+
+	return brcmstb_waketmr_prepare_suspend(timer);
+}
+
+static int brcmstb_waketmr_resume(struct device *dev)
+{
+	struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
+	int ret;
+
+	if (!device_may_wakeup(dev))
+		return 0;
+
+	ret = disable_irq_wake(timer->irq);
+
+	brcmstb_waketmr_clear_alarm(timer);
+
+	return ret;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static SIMPLE_DEV_PM_OPS(brcmstb_waketmr_pm_ops,
+			 brcmstb_waketmr_suspend, brcmstb_waketmr_resume);
+
+static const struct of_device_id brcmstb_waketmr_of_match[] = {
+	{ .compatible = "brcm,brcmstb-waketimer" },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver brcmstb_waketmr_driver = {
+	.probe			= brcmstb_waketmr_probe,
+	.remove			= brcmstb_waketmr_remove,
+	.driver = {
+		.name		= "brcmstb-waketimer",
+		.pm		= &brcmstb_waketmr_pm_ops,
+		.of_match_table	= of_match_ptr(brcmstb_waketmr_of_match),
+	}
+};
+module_platform_driver(brcmstb_waketmr_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Brian Norris");
+MODULE_AUTHOR("Markus Mayer");
+MODULE_DESCRIPTION("Wake-up timer driver for STB chips");
-- 
2.9.3

Re: [PATCH 2/2] rtc: brcmstb-waketimer: Add Broadcom STB wake-timer

[Alexandre Belloni]

Hi,

This seems mostly good.

On 15/06/2017 at 12:59:04 -0700, Florian Fainelli wrote:
> +static void wktmr_read(struct brcmstb_waketmr *timer,
> +		       struct wktmr_time *t)
> +{
> +	u32 tmp;
> +

To be sure, is this IP always 32bit, even on 64bit platforms?

> +	do {
> +		t->sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_COUNTER);
> +		tmp = readl_relaxed(timer->base + BRCMSTB_WKTMR_PRESCALER_VAL);
> +	} while (tmp >= timer->rate);
> +
> +	t->pre = timer->rate - tmp;
> +}
> +

[...]

> +static int brcmstb_waketmr_settime(struct device *dev,
> +				   struct rtc_time *tm)
> +{
> +	struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
> +	unsigned long sec;
> +	int ret;
> +
> +	ret = rtc_valid_tm(tm);
> +	if (ret)
> +		return ret;
> +

There is no way this function can be called without a valid tm. The only
caller checks before calling.

> +	rtc_tm_to_time(tm, &sec);
> +
> +	dev_dbg(dev, "%s: sec=%ld\n", __func__, sec);
> +	writel_relaxed(sec, timer->base + BRCMSTB_WKTMR_COUNTER);
> +
> +	return 0;
> +}
> +
> +static int brcmstb_waketmr_getalarm(struct device *dev,
> +				    struct rtc_wkalrm *alarm)
> +{
> +	struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
> +	unsigned long sec;
> +	u32 reg;
> +
> +	sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_ALARM);
> +	if (sec == 0) {
> +		/* Alarm is disabled */
> +		alarm->enabled = 0;
> +		alarm->time.tm_mon = -1;
> +		alarm->time.tm_mday = -1;
> +		alarm->time.tm_year = -1;
> +		alarm->time.tm_hour = -1;
> +		alarm->time.tm_min = -1;
> +		alarm->time.tm_sec = -1;

This is not needed since d68778b80dd7

> +		dev_dbg(dev, "%s: alarm is disabled\n", __func__);
> +	} else {
> +		/* Alarm is enabled */
> +		alarm->enabled = 1;
> +		rtc_time_to_tm(sec, &alarm->time);
> +		dev_dbg(dev, "%s: alarm is enabled\n", __func__);
> +	}
> +
> +	reg = readl_relaxed(timer->base + BRCMSTB_WKTMR_EVENT);
> +	alarm->pending = !!(reg & 1);
> +	dev_dbg(dev, "%s: alarm pending=%d\n", __func__, alarm->pending);
> +
> +	return 0;
> +}

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 2/2] rtc: brcmstb-waketimer: Add Broadcom STB wake-timer

[Florian Fainelli]

On 06/24/2017 11:59 AM, Alexandre Belloni wrote:
> Hi,
> 
> This seems mostly good.
> 
> On 15/06/2017 at 12:59:04 -0700, Florian Fainelli wrote:
>> +static void wktmr_read(struct brcmstb_waketmr *timer,
>> +		       struct wktmr_time *t)
>> +{
>> +	u32 tmp;
>> +
> 
> To be sure, is this IP always 32bit, even on 64bit platforms?

Correct, it's only 32-bit capable (saw the recent discussions about the
2038yr "problem"...).

> 
>> +	do {
>> +		t->sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_COUNTER);
>> +		tmp = readl_relaxed(timer->base + BRCMSTB_WKTMR_PRESCALER_VAL);
>> +	} while (tmp >= timer->rate);
>> +
>> +	t->pre = timer->rate - tmp;
>> +}
>> +
> 
> [...]
> 
>> +static int brcmstb_waketmr_settime(struct device *dev,
>> +				   struct rtc_time *tm)
>> +{
>> +	struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
>> +	unsigned long sec;
>> +	int ret;
>> +
>> +	ret = rtc_valid_tm(tm);
>> +	if (ret)
>> +		return ret;
>> +
> 
> There is no way this function can be called without a valid tm. The only
> caller checks before calling.

OK.

> 
>> +	rtc_tm_to_time(tm, &sec);
>> +
>> +	dev_dbg(dev, "%s: sec=%ld\n", __func__, sec);
>> +	writel_relaxed(sec, timer->base + BRCMSTB_WKTMR_COUNTER);
>> +
>> +	return 0;
>> +}
>> +
>> +static int brcmstb_waketmr_getalarm(struct device *dev,
>> +				    struct rtc_wkalrm *alarm)
>> +{
>> +	struct brcmstb_waketmr *timer = dev_get_drvdata(dev);
>> +	unsigned long sec;
>> +	u32 reg;
>> +
>> +	sec = readl_relaxed(timer->base + BRCMSTB_WKTMR_ALARM);
>> +	if (sec == 0) {
>> +		/* Alarm is disabled */
>> +		alarm->enabled = 0;
>> +		alarm->time.tm_mon = -1;
>> +		alarm->time.tm_mday = -1;
>> +		alarm->time.tm_year = -1;
>> +		alarm->time.tm_hour = -1;
>> +		alarm->time.tm_min = -1;
>> +		alarm->time.tm_sec = -1;
> 
> This is not needed since d68778b80dd7

Great, I will take that out.

Do you care whether some dev_dbg() prints are left in the driver or
should I remove those in v2?

> 
>> +		dev_dbg(dev, "%s: alarm is disabled\n", __func__);
>> +	} else {
>> +		/* Alarm is enabled */
>> +		alarm->enabled = 1;
>> +		rtc_time_to_tm(sec, &alarm->time);
>> +		dev_dbg(dev, "%s: alarm is enabled\n", __func__);
>> +	}
>> +
>> +	reg = readl_relaxed(timer->base + BRCMSTB_WKTMR_EVENT);
>> +	alarm->pending = !!(reg & 1);
>> +	dev_dbg(dev, "%s: alarm pending=%d\n", __func__, alarm->pending);
>> +
>> +	return 0;
>> +}
> 


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] rtc: brcmstb-waketimer: Add Broadcom STB wake-timer

[Alexandre Belloni]

On 26/06/2017 at 10:15:46 -0700, Florian Fainelli wrote:
> > This is not needed since d68778b80dd7
> 
> Great, I will take that out.
> 
> Do you care whether some dev_dbg() prints are left in the driver or
> should I remove those in v2?
> 

It could have stayed but it seems you removed it from v2.

I'll applay that but it is ok if you change your mind.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] rtc: brcmstb-waketimer: Add Broadcom STB wake-timer

[Florian Fainelli]

On 06/26/2017 02:35 PM, Alexandre Belloni wrote:
> On 26/06/2017 at 10:15:46 -0700, Florian Fainelli wrote:
>>> This is not needed since d68778b80dd7
>>
>> Great, I will take that out.
>>
>> Do you care whether some dev_dbg() prints are left in the driver or
>> should I remove those in v2?
>>
> 
> It could have stayed but it seems you removed it from v2.
> 
> I'll applay that but it is ok if you change your mind.

No, I think they don't really belong in the driver anymore, thanks!
-- 
Florian

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.