* Re: [PATCH v3 1/2] dt-bindings: rtc: Add Spreadtrum SC27xx RTC documentation
From: Rob Herring @ 2017-11-15 15:32 UTC (permalink / raw)
To: Baolin Wang
Cc: a.zummo, alexandre.belloni, mark.rutland, devicetree,
linux-kernel, linux-rtc, broonie, baolin.wang
In-Reply-To: <c65b61e5e37cb43214973cb3e479ce06c4a73395.1510198341.git.baolin.wang@spreadtrum.com>
On Thu, Nov 09, 2017 at 11:34:16AM +0800, Baolin Wang wrote:
> This patch adds the binding documentation for Spreadtrum SC27xx series
> RTC device.
>
> Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com>
> ---
> Changes since v2:
> - No updates.
>
> Changes since v1:
> - Use a specific chip name as compatible string.
> - Remove useless alias.
> - Add the parent MFD node.
> ---
> .../devicetree/bindings/rtc/sprd,sc27xx-rtc.txt | 27 ++++++++++++++++++++
> 1 file changed, 27 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/rtc/sprd,sc27xx-rtc.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: rtc: Add Spreadtrum SC27xx RTC documentation
From: Alexandre Belloni @ 2017-11-15 16:01 UTC (permalink / raw)
To: Baolin Wang
Cc: a.zummo, robh+dt, mark.rutland, devicetree, linux-kernel,
linux-rtc, broonie, baolin.wang
In-Reply-To: <c65b61e5e37cb43214973cb3e479ce06c4a73395.1510198341.git.baolin.wang@spreadtrum.com>
On 09/11/2017 at 11:34:16 +0800, Baolin Wang wrote:
> This patch adds the binding documentation for Spreadtrum SC27xx series
> RTC device.
>
> Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com>
> ---
> Changes since v2:
> - No updates.
>
> Changes since v1:
> - Use a specific chip name as compatible string.
> - Remove useless alias.
> - Add the parent MFD node.
> ---
> .../devicetree/bindings/rtc/sprd,sc27xx-rtc.txt | 27 ++++++++++++++++++++
> 1 file changed, 27 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/rtc/sprd,sc27xx-rtc.txt
>
Applied, thanks.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v3 2/2] rtc: sprd: Add Spreadtrum RTC driver
From: Alexandre Belloni @ 2017-11-15 16:01 UTC (permalink / raw)
To: Baolin Wang
Cc: a.zummo, robh+dt, mark.rutland, devicetree, linux-kernel,
linux-rtc, broonie, baolin.wang
In-Reply-To: <1e1ee941cc59375627712c0e77036ff36389bb33.1510198341.git.baolin.wang@spreadtrum.com>
On 09/11/2017 at 11:34:17 +0800, Baolin Wang wrote:
> This patch adds the Spreadtrum RTC driver, which embedded in the
> Spreadtrum SC27xx series PMICs.
>
> Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com>
> ---
> Changes since v2:
> - Add checking if RTC values are valid.
>
> Changes since v1:
> - Change file name.
> - Should depend on the MFD parent.
> - Add more help documentation.
> - Remove regmap wrapper functions.
> - Modify the mothod of setting alarm.
> - Remove set_mmss64() method.
> - Remove checking RTC power-down.
> - Optimize lock/unlock alarm.
> ---
> drivers/rtc/Kconfig | 11 +
> drivers/rtc/Makefile | 1 +
> drivers/rtc/rtc-sc27xx.c | 662 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 674 insertions(+)
> create mode 100644 drivers/rtc/rtc-sc27xx.c
>
Applied, thanks.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: rtc: Add Spreadtrum SC27xx RTC documentation
From: Baolin Wang @ 2017-11-16 2:16 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Baolin Wang, Alessandro Zummo, Rob Herring, Mark Rutland,
devicetree, LKML, linux-rtc, Mark Brown
In-Reply-To: <20171115160123.gn27j4ki5mxv4v4j@piout.net>
On 16 November 2017 at 00:01, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 09/11/2017 at 11:34:16 +0800, Baolin Wang wrote:
>> This patch adds the binding documentation for Spreadtrum SC27xx series
>> RTC device.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com>
>> ---
>> Changes since v2:
>> - No updates.
>>
>> Changes since v1:
>> - Use a specific chip name as compatible string.
>> - Remove useless alias.
>> - Add the parent MFD node.
>> ---
>> .../devicetree/bindings/rtc/sprd,sc27xx-rtc.txt | 27 ++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/rtc/sprd,sc27xx-rtc.txt
>>
> Applied, thanks.
Thanks Alexandre and Rob.
--
Baolin Wang
Best Regards
^ permalink raw reply
* [PATCH v2] rtc: Add tracepoints for RTC system
From: Baolin Wang @ 2017-11-16 5:59 UTC (permalink / raw)
To: a.zummo, alexandre.belloni, rostedt, mingo
Cc: linux-rtc, linux-kernel, arnd, broonie, baolin.wang
It will be more helpful to add some tracepoints to track RTC actions when
debugging RTC driver. Below sample is that we set/read the RTC time, then
set 2 alarms, so we can see the trace logs:
set/read RTC time:
kworker/0:1-67 [000] 21.814245: rtc_set_time: 2017-11-10 08:13:00 UTC (1510301580) (0)
kworker/0:1-67 [000] 21.814312: rtc_read_time: 2017-11-10 08:13:00 UTC (1510301580) (0)
set the first alarm timer:
kworker/0:1-67 [000] 21.829238: rtc_timer_enqueue: RTC timer:(ffffffc15eb49bc8) expires:1510301700000000000 period:0
kworker/0:1-67 [000] 22.018279: rtc_set_alarm: 2017-11-10 08:15:00 UTC (1510301700) (0)
set the second alarm timer:
kworker/0:1-67 [000] 22.230284: rtc_timer_enqueue: RTC timer:(ffffff80088e6430) expires:1510301820000000000 period:0
the first alarm timer was expired:
kworker/0:1-67 [000] 145.155584: rtc_timer_dequeue: RTC timer:(ffffffc15eb49bc8) expires:1510301700000000000 period:0
kworker/0:1-67 [000] 145.155593: rtc_timer_fired: RTC timer:(ffffffc15eb49bc8) expires:1510301700000000000 period:0
kworker/0:1-67 [000] 145.172504: rtc_set_alarm: 2017-11-10 08:17:00 UTC (1510301820) (0)
the second alarm timer was expired:
kworker/0:1-67 [000] 269.102353: rtc_timer_dequeue: RTC timer:(ffffff80088e6430) expires:1510301820000000000 period:0
kworker/0:1-67 [000] 269.102360: rtc_timer_fired: RTC timer:(ffffff80088e6430) expires:1510301820000000000 period:0
disable alarm irq:
kworker/0:1-67 [000] 269.102469: rtc_alarm_irq_enable: disable RTC alarm IRQ (0)
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes since v1:
- Use unconditional tracepoints with tracing the failures.
- Simplify the rtc_timer_class.
---
drivers/rtc/interface.c | 30 ++++++
include/trace/events/rtc.h | 220 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 250 insertions(+)
create mode 100644 include/trace/events/rtc.h
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 8cec9a0..879e40d 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -17,6 +17,9 @@
#include <linux/log2.h>
#include <linux/workqueue.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/rtc.h>
+
static int rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer);
static void rtc_timer_remove(struct rtc_device *rtc, struct rtc_timer *timer);
@@ -53,6 +56,8 @@ int rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm)
err = __rtc_read_time(rtc, tm);
mutex_unlock(&rtc->ops_lock);
+
+ trace_rtc_read_time(tm, err);
return err;
}
EXPORT_SYMBOL_GPL(rtc_read_time);
@@ -87,6 +92,8 @@ int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
mutex_unlock(&rtc->ops_lock);
/* A timer might have just expired */
schedule_work(&rtc->irqwork);
+
+ trace_rtc_set_time(tm, err);
return err;
}
EXPORT_SYMBOL_GPL(rtc_set_time);
@@ -119,6 +126,8 @@ static int rtc_read_alarm_internal(struct rtc_device *rtc, struct rtc_wkalrm *al
}
mutex_unlock(&rtc->ops_lock);
+
+ trace_rtc_read_alarm(&alarm->time, err);
return err;
}
@@ -316,6 +325,7 @@ int rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
}
mutex_unlock(&rtc->ops_lock);
+ trace_rtc_read_alarm(&alarm->time, err);
return err;
}
EXPORT_SYMBOL_GPL(rtc_read_alarm);
@@ -352,6 +362,7 @@ static int __rtc_set_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
else
err = rtc->ops->set_alarm(rtc->dev.parent, alarm);
+ trace_rtc_set_alarm(&alarm->time, err);
return err;
}
@@ -406,6 +417,7 @@ int rtc_initialize_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
rtc->aie_timer.enabled = 1;
timerqueue_add(&rtc->timerqueue, &rtc->aie_timer.node);
+ trace_rtc_timer_enqueue(&rtc->aie_timer);
}
mutex_unlock(&rtc->ops_lock);
return err;
@@ -435,6 +447,8 @@ int rtc_alarm_irq_enable(struct rtc_device *rtc, unsigned int enabled)
err = rtc->ops->alarm_irq_enable(rtc->dev.parent, enabled);
mutex_unlock(&rtc->ops_lock);
+
+ trace_rtc_alarm_irq_enable(enabled, err);
return err;
}
EXPORT_SYMBOL_GPL(rtc_alarm_irq_enable);
@@ -709,6 +723,8 @@ int rtc_irq_set_state(struct rtc_device *rtc, struct rtc_task *task, int enabled
rtc->pie_enabled = enabled;
}
spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
+
+ trace_rtc_irq_set_state(enabled, err);
return err;
}
EXPORT_SYMBOL_GPL(rtc_irq_set_state);
@@ -745,6 +761,8 @@ int rtc_irq_set_freq(struct rtc_device *rtc, struct rtc_task *task, int freq)
}
}
spin_unlock_irqrestore(&rtc->irq_task_lock, flags);
+
+ trace_rtc_irq_set_freq(freq, err);
return err;
}
EXPORT_SYMBOL_GPL(rtc_irq_set_freq);
@@ -779,6 +797,7 @@ static int rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer)
}
timerqueue_add(&rtc->timerqueue, &timer->node);
+ trace_rtc_timer_enqueue(timer);
if (!next) {
struct rtc_wkalrm alarm;
int err;
@@ -790,6 +809,7 @@ static int rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer)
schedule_work(&rtc->irqwork);
} else if (err) {
timerqueue_del(&rtc->timerqueue, &timer->node);
+ trace_rtc_timer_dequeue(timer);
timer->enabled = 0;
return err;
}
@@ -803,6 +823,7 @@ static void rtc_alarm_disable(struct rtc_device *rtc)
return;
rtc->ops->alarm_irq_enable(rtc->dev.parent, false);
+ trace_rtc_alarm_irq_enable(0, 0);
}
/**
@@ -821,6 +842,7 @@ static void rtc_timer_remove(struct rtc_device *rtc, struct rtc_timer *timer)
{
struct timerqueue_node *next = timerqueue_getnext(&rtc->timerqueue);
timerqueue_del(&rtc->timerqueue, &timer->node);
+ trace_rtc_timer_dequeue(timer);
timer->enabled = 0;
if (next == &timer->node) {
struct rtc_wkalrm alarm;
@@ -871,16 +893,19 @@ void rtc_timer_do_work(struct work_struct *work)
/* expire timer */
timer = container_of(next, struct rtc_timer, node);
timerqueue_del(&rtc->timerqueue, &timer->node);
+ trace_rtc_timer_dequeue(timer);
timer->enabled = 0;
if (timer->task.func)
timer->task.func(timer->task.private_data);
+ trace_rtc_timer_fired(timer);
/* Re-add/fwd periodic timers */
if (ktime_to_ns(timer->period)) {
timer->node.expires = ktime_add(timer->node.expires,
timer->period);
timer->enabled = 1;
timerqueue_add(&rtc->timerqueue, &timer->node);
+ trace_rtc_timer_enqueue(timer);
}
}
@@ -902,6 +927,7 @@ void rtc_timer_do_work(struct work_struct *work)
timer = container_of(next, struct rtc_timer, node);
timerqueue_del(&rtc->timerqueue, &timer->node);
+ trace_rtc_timer_dequeue(timer);
timer->enabled = 0;
dev_err(&rtc->dev, "__rtc_set_alarm: err=%d\n", err);
goto again;
@@ -992,6 +1018,8 @@ int rtc_read_offset(struct rtc_device *rtc, long *offset)
mutex_lock(&rtc->ops_lock);
ret = rtc->ops->read_offset(rtc->dev.parent, offset);
mutex_unlock(&rtc->ops_lock);
+
+ trace_rtc_read_offset(*offset, ret);
return ret;
}
@@ -1021,5 +1049,7 @@ int rtc_set_offset(struct rtc_device *rtc, long offset)
mutex_lock(&rtc->ops_lock);
ret = rtc->ops->set_offset(rtc->dev.parent, offset);
mutex_unlock(&rtc->ops_lock);
+
+ trace_rtc_set_offset(offset, ret);
return ret;
}
diff --git a/include/trace/events/rtc.h b/include/trace/events/rtc.h
new file mode 100644
index 0000000..b5a4add
--- /dev/null
+++ b/include/trace/events/rtc.h
@@ -0,0 +1,220 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM rtc
+
+#if !defined(_TRACE_RTC_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_RTC_H
+
+#include <linux/rtc.h>
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(rtc_time_alarm_class,
+
+ TP_PROTO(struct rtc_time *tm, int err),
+
+ TP_ARGS(tm, err),
+
+ TP_STRUCT__entry(
+ __field(int, sec)
+ __field(int, min)
+ __field(int, hour)
+ __field(int, mday)
+ __field(int, mon)
+ __field(int, year)
+ __field(time64_t, secs)
+ __field(int, err)
+ ),
+
+ TP_fast_assign(
+ __entry->sec = tm->tm_sec;
+ __entry->min = tm->tm_min;
+ __entry->hour = tm->tm_hour;
+ __entry->mday = tm->tm_mday;
+ __entry->mon = tm->tm_mon;
+ __entry->year = tm->tm_year;
+ __entry->secs = rtc_tm_to_time64(tm);
+ __entry->err = err;
+ ),
+
+ TP_printk("%d-%02d-%02d %02d:%02d:%02d UTC (%lld) (%d)",
+ __entry->year + 1900, __entry->mon + 1, __entry->mday,
+ __entry->hour, __entry->min, __entry->sec, __entry->secs,
+ __entry->err
+ )
+);
+
+DEFINE_EVENT(rtc_time_alarm_class, rtc_set_time,
+
+ TP_PROTO(struct rtc_time *tm, int err),
+
+ TP_ARGS(tm, err)
+);
+
+DEFINE_EVENT(rtc_time_alarm_class, rtc_read_time,
+
+ TP_PROTO(struct rtc_time *tm, int err),
+
+ TP_ARGS(tm, err)
+);
+
+DEFINE_EVENT(rtc_time_alarm_class, rtc_set_alarm,
+
+ TP_PROTO(struct rtc_time *tm, int err),
+
+ TP_ARGS(tm, err)
+);
+
+DEFINE_EVENT(rtc_time_alarm_class, rtc_read_alarm,
+
+ TP_PROTO(struct rtc_time *tm, int err),
+
+ TP_ARGS(tm, err)
+);
+
+TRACE_EVENT(rtc_irq_set_freq,
+
+ TP_PROTO(int freq, int err),
+
+ TP_ARGS(freq, err),
+
+ TP_STRUCT__entry(
+ __field(int, freq)
+ __field(int, err)
+ ),
+
+ TP_fast_assign(
+ __entry->freq = freq;
+ __entry->err = err;
+ ),
+
+ TP_printk("set RTC periodic IRQ frequency:%u (%d)",
+ __entry->freq, __entry->err
+ )
+);
+
+TRACE_EVENT(rtc_irq_set_state,
+
+ TP_PROTO(int enabled, int err),
+
+ TP_ARGS(enabled, err),
+
+ TP_STRUCT__entry(
+ __field(int, enabled)
+ __field(int, err)
+ ),
+
+ TP_fast_assign(
+ __entry->enabled = enabled;
+ __entry->err = err;
+ ),
+
+ TP_printk("%s RTC 2^N Hz periodic IRQs (%d)",
+ __entry->enabled ? "enable" : "disable",
+ __entry->err
+ )
+);
+
+TRACE_EVENT(rtc_alarm_irq_enable,
+
+ TP_PROTO(unsigned int enabled, int err),
+
+ TP_ARGS(enabled, err),
+
+ TP_STRUCT__entry(
+ __field(unsigned int, enabled)
+ __field(int, err)
+ ),
+
+ TP_fast_assign(
+ __entry->enabled = enabled;
+ __entry->err = err;
+ ),
+
+ TP_printk("%s RTC alarm IRQ (%d)",
+ __entry->enabled ? "enable" : "disable",
+ __entry->err
+ )
+);
+
+DECLARE_EVENT_CLASS(rtc_offset_class,
+
+ TP_PROTO(long offset, int err),
+
+ TP_ARGS(offset, err),
+
+ TP_STRUCT__entry(
+ __field(long, offset)
+ __field(int, err)
+ ),
+
+ TP_fast_assign(
+ __entry->offset = offset;
+ __entry->err = err;
+ ),
+
+ TP_printk("RTC offset: %ld (%d)",
+ __entry->offset, __entry->err
+ )
+);
+
+DEFINE_EVENT(rtc_offset_class, rtc_set_offset,
+
+ TP_PROTO(long offset, int err),
+
+ TP_ARGS(offset, err)
+);
+
+DEFINE_EVENT(rtc_offset_class, rtc_read_offset,
+
+ TP_PROTO(long offset, int err),
+
+ TP_ARGS(offset, err)
+);
+
+DECLARE_EVENT_CLASS(rtc_timer_class,
+
+ TP_PROTO(struct rtc_timer *timer),
+
+ TP_ARGS(timer),
+
+ TP_STRUCT__entry(
+ __field(struct rtc_timer *, timer)
+ __field(ktime_t, expires)
+ __field(ktime_t, period)
+ ),
+
+ TP_fast_assign(
+ __entry->timer = timer;
+ __entry->expires = timer->node.expires;
+ __entry->period = timer->period;
+ ),
+
+ TP_printk("RTC timer:(%p) expires:%lld period:%lld",
+ __entry->timer, __entry->expires, __entry->period
+ )
+);
+
+DEFINE_EVENT(rtc_timer_class, rtc_timer_enqueue,
+
+ TP_PROTO(struct rtc_timer *timer),
+
+ TP_ARGS(timer)
+);
+
+DEFINE_EVENT(rtc_timer_class, rtc_timer_dequeue,
+
+ TP_PROTO(struct rtc_timer *timer),
+
+ TP_ARGS(timer)
+);
+
+DEFINE_EVENT(rtc_timer_class, rtc_timer_fired,
+
+ TP_PROTO(struct rtc_timer *timer),
+
+ TP_ARGS(timer)
+);
+
+#endif /* _TRACE_RTC_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
--
1.7.9.5
^ permalink raw reply related
* [PATCH] Documentation: dt: rtc-imxdi: imx53-rtc is not compatible
From: linux-kernel-dev @ 2017-11-16 9:34 UTC (permalink / raw)
To: Alessandro Zummo, Alexandre Belloni
Cc: Patrick Bruenn, Rob Herring, Mark Rutland,
open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, Fabio Estevam, Juergen Borleis, Noel Vellemans,
Shawn Guo
From: Patrick Bruenn <p.bruenn@beckhoff.com>
rtc-imxdi driver is not compatible with imx53-rtc like the example
suggested.
Only the raw timestamp register offsets match by accident, which has the
effect, that during startup/shutdown system clock syncronization with
rtc seems to work for imx53, too.
However hwclock and rtctest, will not work as expected on imx53.
To avoid future reverts like [1], we should adjust the documentation.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-November/542312.html
Signed-off-by: Patrick Bruenn <p.bruenn@beckhoff.com>
---
To: Alessandro Zummo <a.zummo@towertech.it>
To: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: linux-rtc@vger.kernel.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
Cc: devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Cc: linux-kernel@vger.kernel.org (open list)
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Juergen Borleis <jbe@pengutronix.de>
Cc: Noel Vellemans <Noel.Vellemans@visionbms.com>
Cc: Shawn Guo <shawnguo@kernel.org>
---
Documentation/devicetree/bindings/rtc/imxdi-rtc.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
index 323cf26374cb..af853b0ceac5 100644
--- a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
@@ -1,6 +1,6 @@
* i.MX25 Real Time Clock controller
-This binding supports the following chips: i.MX25, i.MX53
+This binding supports the following chips: i.MX25
Required properties:
- compatible: should be: "fsl,imx25-rtc"
@@ -14,7 +14,7 @@ Optional properties:
Example:
rtc@80056000 {
- compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
+ compatible = "fsl,imx25-rtc";
reg = <0x80056000 2000>;
interrupts = <29 56>;
};
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] Documentation: dt: rtc-imxdi: imx53-rtc is not compatible
From: Alexandre Belloni @ 2017-11-16 9:44 UTC (permalink / raw)
To: linux-kernel-dev
Cc: Alessandro Zummo, Patrick Bruenn, Rob Herring, Mark Rutland,
open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, Fabio Estevam, Juergen Borleis, Noel Vellemans,
Shawn Guo
In-Reply-To: <20171116093425.32145-1-linux-kernel-dev@beckhoff.com>
Hi,
The following patch was already sent by Fabio:
http://patchwork.ozlabs.org/patch/838173/
On 16/11/2017 at 10:34:25 +0100, linux-kernel-dev@beckhoff.com wrote:
> From: Patrick Bruenn <p.bruenn@beckhoff.com>
>
> rtc-imxdi driver is not compatible with imx53-rtc like the example
> suggested.
> Only the raw timestamp register offsets match by accident, which has the
> effect, that during startup/shutdown system clock syncronization with
> rtc seems to work for imx53, too.
> However hwclock and rtctest, will not work as expected on imx53.
> To avoid future reverts like [1], we should adjust the documentation.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-November/542312.html
>
> Signed-off-by: Patrick Bruenn <p.bruenn@beckhoff.com>
>
> ---
>
> To: Alessandro Zummo <a.zummo@towertech.it>
> To: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: linux-rtc@vger.kernel.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM)
> Cc: devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
> Cc: linux-kernel@vger.kernel.org (open list)
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Juergen Borleis <jbe@pengutronix.de>
> Cc: Noel Vellemans <Noel.Vellemans@visionbms.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> ---
> Documentation/devicetree/bindings/rtc/imxdi-rtc.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
> index 323cf26374cb..af853b0ceac5 100644
> --- a/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
> +++ b/Documentation/devicetree/bindings/rtc/imxdi-rtc.txt
> @@ -1,6 +1,6 @@
> * i.MX25 Real Time Clock controller
>
> -This binding supports the following chips: i.MX25, i.MX53
> +This binding supports the following chips: i.MX25
>
> Required properties:
> - compatible: should be: "fsl,imx25-rtc"
> @@ -14,7 +14,7 @@ Optional properties:
> Example:
>
> rtc@80056000 {
> - compatible = "fsl,imx53-rtc", "fsl,imx25-rtc";
> + compatible = "fsl,imx25-rtc";
> reg = <0x80056000 2000>;
> interrupts = <29 56>;
> };
> --
> 2.11.0
>
>
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH] rtc: brcmstb-waketimer: fix error handling in brcmstb_waketmr_probe()
From: Alexey Khoroshilov @ 2017-11-17 21:15 UTC (permalink / raw)
To: Brian Norris, Alessandro Zummo, Alexandre Belloni
Cc: Alexey Khoroshilov, bcm-kernel-feedback-list, linux-rtc,
linux-arm-kernel, linux-kernel, ldv-project
brcmstb_waketmr_probe() does not disable timer->clk on error paths.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/rtc/rtc-brcmstb-waketimer.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/rtc/rtc-brcmstb-waketimer.c b/drivers/rtc/rtc-brcmstb-waketimer.c
index 796ac792a381..6cee61201c30 100644
--- a/drivers/rtc/rtc-brcmstb-waketimer.c
+++ b/drivers/rtc/rtc-brcmstb-waketimer.c
@@ -253,7 +253,7 @@ static int brcmstb_waketmr_probe(struct platform_device *pdev)
ret = devm_request_irq(dev, timer->irq, brcmstb_waketmr_irq, 0,
"brcmstb-waketimer", timer);
if (ret < 0)
- return ret;
+ goto err_clk;
timer->reboot_notifier.notifier_call = brcmstb_waketmr_reboot;
register_reboot_notifier(&timer->reboot_notifier);
@@ -262,12 +262,21 @@ static int brcmstb_waketmr_probe(struct platform_device *pdev)
&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);
+ ret = PTR_ERR(timer->rtc);
+ goto err_notifier;
}
dev_info(dev, "registered, with irq %d\n", timer->irq);
+ return 0;
+
+err_notifier:
+ unregister_reboot_notifier(&timer->reboot_notifier);
+
+err_clk:
+ if (timer->clk)
+ clk_disable_unprepare(timer->clk);
+
return ret;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] rtc: brcmstb-waketimer: fix error handling in brcmstb_waketmr_probe()
From: Florian Fainelli @ 2017-11-17 23:51 UTC (permalink / raw)
To: Alexey Khoroshilov, Brian Norris, Alessandro Zummo,
Alexandre Belloni
Cc: bcm-kernel-feedback-list, linux-rtc, linux-arm-kernel,
linux-kernel, ldv-project
In-Reply-To: <1510953358-7043-1-git-send-email-khoroshilov@ispras.ru>
On 11/17/2017 01:15 PM, Alexey Khoroshilov wrote:
> brcmstb_waketmr_probe() does not disable timer->clk on error paths.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Fixes: c4f07ecee22e ("rtc: brcmstb-waketimer: Add Broadcom STB wake-timer")
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* [PATCH] rtc: ds1307: Remove r9701_remove function
From: Nobuhiro Iwamatsu @ 2017-11-18 2:03 UTC (permalink / raw)
To: linux-rtc; +Cc: alexandre.belloni, Nobuhiro Iwamatsu
r9701_remove function is now empty, remove it.
Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
---
drivers/rtc/rtc-r9701.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/rtc/rtc-r9701.c b/drivers/rtc/rtc-r9701.c
index 83d2bcca6a8f..b6c5eb97051c 100644
--- a/drivers/rtc/rtc-r9701.c
+++ b/drivers/rtc/rtc-r9701.c
@@ -164,17 +164,11 @@ static int r9701_probe(struct spi_device *spi)
return 0;
}
-static int r9701_remove(struct spi_device *spi)
-{
- return 0;
-}
-
static struct spi_driver r9701_driver = {
.driver = {
.name = "rtc-r9701",
},
.probe = r9701_probe,
- .remove = r9701_remove,
};
module_spi_driver(r9701_driver);
--
2.15.0
^ permalink raw reply related
* [GIT PULL] RTC for 4.15
From: Alexandre Belloni @ 2017-11-21 15:28 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-rtc, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 7008 bytes --]
Hi Linus,
Here is the pull-request for the RTC subsystem for 4.14.
The last minute change is only a documentation update to avoid further
bug reports. (And also a small commit message reword).
There is nothing scary this cycle, mostly driver fixes and updates. The
core fix has been in for a while and has been tested on multiple kernel
revisions by multiple teams.
The following changes since commit 2bd6bf03f4c1c59381d62c61d03f6cc3fe71f66e:
Linux 4.14-rc1 (2017-09-16 15:47:51 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git tags/rtc-4.15
for you to fetch changes up to 87c9fd81825363237ac5560822e2261535800597:
dt-bindings: rtc: imxdi: Improve the bindings text (2017-11-20 22:48:20 +0100)
----------------------------------------------------------------
RTC for 4.15
Subsystem:
- Fix setting the alarm to the next expiring timer
New driver:
- Mediatek MT7622 RTC
- NXP PCF85363
- Spreadtrum SC27xx PMIC RTC
Drivers:
- Use generic nvmem to expose the Non volatile ram for ds1305, ds1511,
m48t86 and omap
- abx80x: solve possible race condition at probe
- armada38x: support trimming the RTC oscillator
- at91rm9200: fix reading the alarm value at boot
- ds1511: allow waking platform
- m41t80: rework square wave output
- pcf8523: support trimming the RTC oscillator
- pcf8563: fix clock output rate
- pl031: make interrupt optional
- xgene: fix suspend/resume
----------------------------------------------------------------
Akshay Bhat (3):
rtc: rx8010: Remove duplicate define
rtc: rx8010: Specify correct address for RX8010_RESV31
rtc: rx8010: Fix for incorrect return value
Alexandre Belloni (18):
rtc: rv3029: fix vendor string
rtc: set the alarm to the next expiring timer
rtc: ds1305: switch to rtc_register_device
rtc: ds1305: use generic nvmem
rtc: m48t86: switch to rtc_register_device
rtc: m48t86: use generic nvmem
rtc: abx80x: switch to rtc_register_device
rtc: abx80x: solve race condition
rtc: ds1511: switch to rtc_register_device
rtc: ds1511: allow waking platform
rtc: ds1511: use generic nvmem
rtc: ds1390: Add OF device ID table
rtc: omap: fix error path when pinctrl_register fails
rtc: omap: switch to rtc_register_device
rtc: omap: Support scratch registers
rtc: pcf8563: don't alway enable the alarm
rtc: at91rm9200: stop calculating yday in at91_rtc_readalarm
rtc: at91rm9200: fix reading alarm value
Arnd Bergmann (1):
rtc: xgene: mark PM functions as __maybe_unused
Baolin Wang (3):
rtc: sysfs: Use time64_t variables to set time/alarm
dt-bindings: rtc: Add Spreadtrum SC27xx RTC documentation
rtc: sc27xx: Add Spreadtrum SC27xx PMIC RTC driver
Bastian Stender (1):
rtc: ds1307: add OF and ACPI entries for Epson RX8130
Dan Carpenter (1):
rtc: rv3029: Clean up error handling in rv3029_eeprom_write()
Eric Nelson (1):
rtc: add support for NXP PCF85363 real-time clock
Fabio Estevam (1):
dt-bindings: rtc: imxdi: Improve the bindings text
Heiner Kallweit (1):
rtc: ds1307: improve weekday handling
Loc Ho (1):
rtc: xgene: Fix suspend/resume
Mathieu Malaterre (2):
rtc: jz4740: remove duplicate 'write' in message
rtc: jz4740: fix loading of rtc driver
Philipp Zabel (1):
rtc: pcf8563: fix output clock rate
Russell King (7):
rtc: pl031: constify amba_ids
rtc: pl031: use devm_* for allocating memory and mapping resource
rtc: pl031: avoid exposing alarm if no interrupt
rtc: pl031: make interrupt optional
rtc: clarify the RTC offset correction
rtc: armada38x: add support for trimming the RTC
rtc: pcf8523: add support for trimming the RTC oscillator
Sean Wang (4):
dt-bindings: rtc: mediatek: add bindings for MediaTek SoC based RTC
rtc: mediatek: add driver for RTC on MT7622 SoC
rtc: mediatek: enhance the description for MediaTek PMIC based RTC
rtc: mediatek: update MAINTAINERS entry with MediaTek RTC driver
Troy Kisky (5):
rtc: m41t80: m41t80_sqw_set_rate should return 0 on success
rtc: m41t80: fix m41t80_sqw_round_rate return value
rtc: m41t80: avoid i2c read in m41t80_sqw_recalc_rate
rtc: m41t80: avoid i2c read in m41t80_sqw_is_prepared
rtc: m41t80: remove unneeded checks from m41t80_sqw_set_rate
.../devicetree/bindings/rtc/imxdi-rtc.txt | 14 +-
Documentation/devicetree/bindings/rtc/pcf85363.txt | 17 +
.../devicetree/bindings/rtc/rtc-mt7622.txt | 21 +
.../devicetree/bindings/rtc/sprd,sc27xx-rtc.txt | 27 +
.../devicetree/bindings/trivial-devices.txt | 2 +-
MAINTAINERS | 3 +
drivers/rtc/Kconfig | 42 +-
drivers/rtc/Makefile | 3 +
drivers/rtc/interface.c | 6 +-
drivers/rtc/rtc-abx80x.c | 12 +-
drivers/rtc/rtc-armada38x.c | 101 ++++
drivers/rtc/rtc-at91rm9200.c | 19 +-
drivers/rtc/rtc-ds1305.c | 70 +--
drivers/rtc/rtc-ds1307.c | 57 +-
drivers/rtc/rtc-ds1390.c | 7 +
drivers/rtc/rtc-ds1511.c | 75 +--
drivers/rtc/rtc-jz4740.c | 6 +-
drivers/rtc/rtc-m41t80.c | 84 ++-
drivers/rtc/rtc-m48t86.c | 58 +-
drivers/rtc/rtc-mt7622.c | 422 +++++++++++++
drivers/rtc/rtc-omap.c | 57 +-
drivers/rtc/rtc-pcf8523.c | 40 ++
drivers/rtc/rtc-pcf85363.c | 220 +++++++
drivers/rtc/rtc-pcf8563.c | 4 +-
drivers/rtc/rtc-pl031.c | 48 +-
drivers/rtc/rtc-rv3029c2.c | 18 +-
drivers/rtc/rtc-rx8010.c | 7 +-
drivers/rtc/rtc-sc27xx.c | 662 +++++++++++++++++++++
drivers/rtc/rtc-sysfs.c | 25 +-
drivers/rtc/rtc-xgene.c | 47 +-
30 files changed, 1891 insertions(+), 283 deletions(-)
create mode 100644 Documentation/devicetree/bindings/rtc/pcf85363.txt
create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt7622.txt
create mode 100644 Documentation/devicetree/bindings/rtc/sprd,sc27xx-rtc.txt
create mode 100644 drivers/rtc/rtc-mt7622.c
create mode 100644 drivers/rtc/rtc-pcf85363.c
create mode 100644 drivers/rtc/rtc-sc27xx.c
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] rtc: sun6i: ensure rtc is kfree'd on error
From: Colin King @ 2017-11-22 17:16 UTC (permalink / raw)
To: Alessandro Zummo, Alexandre Belloni, Maxime Ripard, Chen-Yu Tsai,
linux-rtc, linux-arm-kernel
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The error return path on clk_data allocation failure does not kfree
the allocated rtc object. Fix this with a kfree of rtc on the error
exit path.
Detected by CoverityScan, CID#1452264 ("Resource Leak")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/rtc/rtc-sun6i.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 3d2216ccd860..5bc28eed1adf 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -201,8 +201,10 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
GFP_KERNEL);
- if (!clk_data)
+ if (!clk_data) {
+ kfree(rtc);
return;
+ }
spin_lock_init(&rtc->lock);
--
2.14.1
^ permalink raw reply related
* Re: [GIT PULL] RTC for 4.15
From: Linus Torvalds @ 2017-11-23 7:05 UTC (permalink / raw)
To: Alexandre Belloni; +Cc: linux-rtc, Linux Kernel Mailing List
In-Reply-To: <20171121152823.payahwcvnnjervgf@piout.net>
On Tue, Nov 21, 2017 at 5:28 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
>
> The last minute change is only a documentation update to avoid further
> bug reports. (And also a small commit message reword).
This is _really_ annoying.
I'm checking that pull requests have been in linux-next, and that
rewording of the commit message just means that now that commit
doesn't trigger as being in commit-next and I have to go chase it down
or just say "screw this, it adds a whole new driver that wasn't in
linux-next".
Guess which one is easier for me to do?
So don't do this crap.
I ended up looking up the original commit, but dammit, I'm not happy
about this. Why wasn't the RTC stuff sent last week like I asked
people to do? Now I'm in the middle of travels, and having to figure
out what parts were in linux-next to verify that it actually has
gotten at least some real test coverage, and that I'm not pulling
random stuff.
Linus
^ permalink raw reply
* Re: [RFC v8 2/7] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices
From: Heikki Krogerus @ 2017-11-23 11:49 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy
Cc: a.zummo, x86, wim, mingo, alexandre.belloni, qipeng.zha, hpa,
dvhart, tglx, lee.jones, andy, souvik.k.chakravarty, linux-rtc,
linux-watchdog, linux-kernel, platform-driver-x86, sathyaosid,
Andy Shevchenko
In-Reply-To: <d7b0fadb708a50e6d9ccf92f106d6939314ccf0f.1509268570.git.sathyanarayanan.kuppuswamy@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 10696 bytes --]
Hi,
On Sun, Oct 29, 2017 at 02:49:55AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Currently, we have lot of repetitive code in dependent device resource
> allocation and device creation handling code. This logic can be improved if
> we use MFD framework for dependent device creation. This patch adds this
> support.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/platform/x86/intel_pmc_ipc.c | 398 ++++++++++++-----------------------
> 1 file changed, 139 insertions(+), 259 deletions(-)
>
> Changes since v7:
> * Fixed style issues.
>
> Changes since v6:
> * Fixed style issues.
> * Used Andy's modified version.
>
> Changes since v5:
> * Changed the order of patches in this patchlist.
>
> Changes since v4:
> * Changed the order of patches in this patchlist.
>
> Changes since v3:
> * Changed PLATFORM_DEVID_AUTO to PLATFORM_DEVID_NONE in mfd device creation.
> * Fixed error in resource initalization logic in ipc_create_punit_device.
> * Removed mfd cell id initialization.
>
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index e03fa314..e36144c 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -20,6 +20,7 @@
> #include <linux/errno.h>
> #include <linux/init.h>
> #include <linux/device.h>
> +#include <linux/mfd/core.h>
> #include <linux/pm.h>
> #include <linux/pci.h>
> #include <linux/platform_device.h>
> @@ -88,6 +89,7 @@
> #define PLAT_RESOURCE_ISP_IFACE_INDEX 5
> #define PLAT_RESOURCE_GTD_DATA_INDEX 6
> #define PLAT_RESOURCE_GTD_IFACE_INDEX 7
> +#define PLAT_RESOURCE_MEM_MAX_INDEX 8
> #define PLAT_RESOURCE_ACPI_IO_INDEX 0
>
> /*
> @@ -106,8 +108,6 @@
> #define TELEM_SSRAM_SIZE 240
> #define TELEM_PMC_SSRAM_OFFSET 0x1B00
> #define TELEM_PUNIT_SSRAM_OFFSET 0x1A00
> -#define TCO_PMC_OFFSET 0x8
> -#define TCO_PMC_SIZE 0x4
>
> /* PMC register bit definitions */
>
> @@ -124,26 +124,10 @@ static struct intel_pmc_ipc_dev {
> int cmd;
> struct completion cmd_complete;
>
> - /* The following PMC BARs share the same ACPI device with the IPC */
> - resource_size_t acpi_io_base;
> - int acpi_io_size;
> - struct platform_device *tco_dev;
> -
> /* gcr */
> void __iomem *gcr_mem_base;
> bool has_gcr_regs;
> spinlock_t gcr_lock;
> -
> - /* punit */
> - struct platform_device *punit_dev;
> -
> - /* Telemetry */
> - resource_size_t telem_pmc_ssram_base;
> - resource_size_t telem_punit_ssram_base;
> - int telem_pmc_ssram_size;
> - int telem_punit_ssram_size;
> - u8 telem_res_inval;
> - struct platform_device *telemetry_dev;
> } ipcdev;
>
> static char *ipc_err_sources[] = {
> @@ -508,7 +492,7 @@ static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> ret = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_pmc_ipc",
> pmc);
> if (ret) {
> - dev_err(&pdev->dev, "Failed to request irq\n");
> + dev_err(&pdev->dev, "Failed to request IRQ\n");
> return ret;
> }
>
> @@ -593,44 +577,6 @@ static const struct attribute_group intel_ipc_group = {
> .attrs = intel_ipc_attrs,
> };
>
> -static struct resource punit_res_array[] = {
> - /* Punit BIOS */
> - {
> - .flags = IORESOURCE_MEM,
> - },
> - {
> - .flags = IORESOURCE_MEM,
> - },
> - /* Punit ISP */
> - {
> - .flags = IORESOURCE_MEM,
> - },
> - {
> - .flags = IORESOURCE_MEM,
> - },
> - /* Punit GTD */
> - {
> - .flags = IORESOURCE_MEM,
> - },
> - {
> - .flags = IORESOURCE_MEM,
> - },
> -};
> -
> -#define TCO_RESOURCE_ACPI_IO 0
> -#define TCO_RESOURCE_SMI_EN_IO 1
> -#define TCO_RESOURCE_GCR_MEM 2
> -static struct resource tco_res[] = {
> - /* ACPI - TCO */
> - {
> - .flags = IORESOURCE_IO,
> - },
> - /* ACPI - SMI */
> - {
> - .flags = IORESOURCE_IO,
> - },
> -};
> -
> static struct itco_wdt_platform_data tco_info = {
> .name = "Apollo Lake SoC",
> .version = 5,
> @@ -638,234 +584,177 @@ static struct itco_wdt_platform_data tco_info = {
> .update_no_reboot_bit = update_no_reboot_bit,
> };
>
> -#define TELEMETRY_RESOURCE_PUNIT_SSRAM 0
> -#define TELEMETRY_RESOURCE_PMC_SSRAM 1
> -static struct resource telemetry_res[] = {
> - /*Telemetry*/
> - {
> - .flags = IORESOURCE_MEM,
> - },
> - {
> - .flags = IORESOURCE_MEM,
> - },
> -};
> -
> -static int ipc_create_punit_device(void)
> +static int ipc_create_punit_device(struct platform_device *pdev)
> {
> - struct platform_device *pdev;
> - const struct platform_device_info pdevinfo = {
> - .parent = ipcdev.dev,
> - .name = PUNIT_DEVICE_NAME,
> - .id = -1,
> - .res = punit_res_array,
> - .num_res = ARRAY_SIZE(punit_res_array),
> + struct resource punit_res[PLAT_RESOURCE_MEM_MAX_INDEX];
> + struct mfd_cell punit_cell;
> + struct resource *res;
That's where you have the bug I reported earlier. You would need to
introduce those structures as static struct..
But instead of fixing those, drop them and introduce the resources and
the cells out side of these functions:
static struct resource punit_resources[PLAT_RESOURCE_MEM_MAX_INDEX];
static struct resource telemetry_resources[2];
static struct resource wdt_resources[2];
static struct mfd_cell pmc_cell[] = {
{
.name = "intel_punit_ipc",
.resources = punit_resources,
.num_resources = PLAT_RESOURCE_MEM_MAX_INDEX,
.ignore_resource_conflicts = true,
},
{
.name = "intel_telemetry",
.resources = telemetry_resources,
.num_resources = 2,
.ignore_resource_conflicts = true,
},
{
.name = "iTCO_wdt",
.resources = wdt_resources,
.num_resources = 2,
.ignore_resource_conflicts = true,
.platform_data = &tco_info,
.pdata_size = sizeof(tco_info),
},
};
Note that I'm not using the definitions for the name strings on
purpose. Please get rid of those definitions while at it.
Use these functions - ipc_create_punit/wdt/telemetry_device() - to just
collect the resources. Then you call devm_mfd_add_devices() only ones
in ipc_create_pmc_devices(). That should make this driver a bit more
easier to read and understand.
> + int mindex, pindex = 0;
> +
> + for (mindex = 0; mindex <= PLAT_RESOURCE_MEM_MAX_INDEX; mindex++) {
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, mindex);
> +
> + switch (mindex) {
> + /* Get PUNIT resources */
> + case PLAT_RESOURCE_BIOS_DATA_INDEX:
> + case PLAT_RESOURCE_BIOS_IFACE_INDEX:
> + /* BIOS resources are required, so return error if not
> + * available
> + */
> + if (!res) {
> + dev_err(&pdev->dev,
> + "Failed to get PUNIT MEM resource %d\n",
> + pindex);
> + return -ENXIO;
> + }
> + case PLAT_RESOURCE_ISP_DATA_INDEX:
> + case PLAT_RESOURCE_ISP_IFACE_INDEX:
> + case PLAT_RESOURCE_GTD_DATA_INDEX:
> + case PLAT_RESOURCE_GTD_IFACE_INDEX:
> + /* if valid resource found, copy the resource to PUNIT
> + * resource
> + */
> + if (res)
> + memcpy(&punit_res[pindex], res, sizeof(*res));
> + punit_res[pindex].flags = IORESOURCE_MEM;
> + dev_dbg(&pdev->dev, "PUNIT memory res: %pR\n",
> + &punit_res[pindex]);
I don't see how is that useful information?
> + pindex++;
> + break;
> };
> + }
>
> - pdev = platform_device_register_full(&pdevinfo);
> - if (IS_ERR(pdev))
> - return PTR_ERR(pdev);
> -
> - ipcdev.punit_dev = pdev;
> + /* Create PUNIT IPC MFD cell */
> + punit_cell.name = PUNIT_DEVICE_NAME;
> + punit_cell.num_resources = ARRAY_SIZE(punit_res);
> + punit_cell.resources = punit_res;
> + punit_cell.ignore_resource_conflicts = 1;
>
> - return 0;
> + return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
> + &punit_cell, 1, NULL, 0, NULL);
> }
>
> -static int ipc_create_tco_device(void)
> +static int ipc_create_wdt_device(struct platform_device *pdev)
> {
> - struct platform_device *pdev;
> + static struct resource wdt_ipc_res[2];
> struct resource *res;
> - const struct platform_device_info pdevinfo = {
> - .parent = ipcdev.dev,
> - .name = TCO_DEVICE_NAME,
> - .id = -1,
> - .res = tco_res,
> - .num_res = ARRAY_SIZE(tco_res),
> - .data = &tco_info,
> - .size_data = sizeof(tco_info),
> - };
> + static struct mfd_cell wdt_cell;
>
> - res = tco_res + TCO_RESOURCE_ACPI_IO;
> - res->start = ipcdev.acpi_io_base + TCO_BASE_OFFSET;
> - res->end = res->start + TCO_REGS_SIZE - 1;
> + /* If we have ACPI based watchdog use that instead, othewise create
> + * a MFD cell for iTCO watchdog
> + */
> + if (acpi_has_watchdog())
> + return 0;
>
> - res = tco_res + TCO_RESOURCE_SMI_EN_IO;
> - res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET;
> - res->end = res->start + SMI_EN_SIZE - 1;
> + /* Get iTCO watchdog resources */
> + res = platform_get_resource(pdev, IORESOURCE_IO,
> + PLAT_RESOURCE_ACPI_IO_INDEX);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get WDT resource\n");
> + return -ENXIO;
> + }
>
> - pdev = platform_device_register_full(&pdevinfo);
> - if (IS_ERR(pdev))
> - return PTR_ERR(pdev);
> + wdt_ipc_res[0].start = res->start + TCO_BASE_OFFSET;
> + wdt_ipc_res[0].end = res->start +
> + TCO_BASE_OFFSET + TCO_REGS_SIZE - 1;
> + wdt_ipc_res[0].flags = IORESOURCE_IO;
> + wdt_ipc_res[1].start = res->start + SMI_EN_OFFSET;
> + wdt_ipc_res[1].end = res->start +
> + SMI_EN_OFFSET + SMI_EN_SIZE - 1;
> + wdt_ipc_res[1].flags = IORESOURCE_IO;
>
> - ipcdev.tco_dev = pdev;
> + dev_dbg(&pdev->dev, "watchdog res 0: %pR\n", &wdt_ipc_res[0]);
> + dev_dbg(&pdev->dev, "watchdog res 1: %pR\n", &wdt_ipc_res[1]);
That definitely is not useful information. Please drop all dev_dbg
calls from these patches.
> - return 0;
> + wdt_cell.name = TCO_DEVICE_NAME;
> + wdt_cell.platform_data = &tco_info;
> + wdt_cell.pdata_size = sizeof(tco_info);
> + wdt_cell.num_resources = ARRAY_SIZE(wdt_ipc_res);
> + wdt_cell.resources = wdt_ipc_res;
> + wdt_cell.ignore_resource_conflicts = 1;
> +
> + return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
> + &wdt_cell, 1, NULL, 0, NULL);
> }
>
> -static int ipc_create_telemetry_device(void)
> +static int ipc_create_telemetry_device(struct platform_device *pdev)
> {
> - struct platform_device *pdev;
> + struct resource telemetry_ipc_res[2];
> + struct mfd_cell telemetry_cell;
This is also broken. I'm attaching a diff with the changes to this
patch I used when I tested this on my Broxton board.
Thanks,
--
heikki
[-- Attachment #2: intel_pmc_ipc.diff --]
[-- Type: text/plain, Size: 6060 bytes --]
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index e36144c337cd..a51684f4bf0b 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -584,10 +584,35 @@ static struct itco_wdt_platform_data tco_info = {
.update_no_reboot_bit = update_no_reboot_bit,
};
-static int ipc_create_punit_device(struct platform_device *pdev)
+static struct resource punit_resources[PLAT_RESOURCE_MEM_MAX_INDEX];
+static struct resource telemetry_resources[2];
+static struct resource wdt_resources[2];
+
+static struct mfd_cell pmc_cell[] = {
+ {
+ .name = "intel_punit_ipc",
+ .resources = punit_resources,
+ .num_resources = PLAT_RESOURCE_MEM_MAX_INDEX,
+ .ignore_resource_conflicts = true,
+ },
+ {
+ .name = "intel_telemetry",
+ .resources = telemetry_resources,
+ .num_resources = 2,
+ .ignore_resource_conflicts = true,
+ },
+ {
+ .name = "iTCO_wdt",
+ .resources = wdt_resources,
+ .num_resources = 2,
+ .ignore_resource_conflicts = true,
+ .platform_data = &tco_info,
+ .pdata_size = sizeof(tco_info),
+ },
+};
+
+static int ipc_get_punit_resources(struct platform_device *pdev)
{
- struct resource punit_res[PLAT_RESOURCE_MEM_MAX_INDEX];
- struct mfd_cell punit_cell;
struct resource *res;
int mindex, pindex = 0;
@@ -616,30 +641,20 @@ static int ipc_create_punit_device(struct platform_device *pdev)
* resource
*/
if (res)
- memcpy(&punit_res[pindex], res, sizeof(*res));
- punit_res[pindex].flags = IORESOURCE_MEM;
- dev_dbg(&pdev->dev, "PUNIT memory res: %pR\n",
- &punit_res[pindex]);
+ memcpy(&punit_resources[pindex], res,
+ sizeof(*res));
+ punit_resources[pindex].flags = IORESOURCE_MEM;
pindex++;
break;
};
}
- /* Create PUNIT IPC MFD cell */
- punit_cell.name = PUNIT_DEVICE_NAME;
- punit_cell.num_resources = ARRAY_SIZE(punit_res);
- punit_cell.resources = punit_res;
- punit_cell.ignore_resource_conflicts = 1;
-
- return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
- &punit_cell, 1, NULL, 0, NULL);
+ return 0;
}
-static int ipc_create_wdt_device(struct platform_device *pdev)
+static int ipc_get_wdt_resources(struct platform_device *pdev)
{
- static struct resource wdt_ipc_res[2];
struct resource *res;
- static struct mfd_cell wdt_cell;
/* If we have ACPI based watchdog use that instead, othewise create
* a MFD cell for iTCO watchdog
@@ -655,33 +670,18 @@ static int ipc_create_wdt_device(struct platform_device *pdev)
return -ENXIO;
}
- wdt_ipc_res[0].start = res->start + TCO_BASE_OFFSET;
- wdt_ipc_res[0].end = res->start +
- TCO_BASE_OFFSET + TCO_REGS_SIZE - 1;
- wdt_ipc_res[0].flags = IORESOURCE_IO;
- wdt_ipc_res[1].start = res->start + SMI_EN_OFFSET;
- wdt_ipc_res[1].end = res->start +
- SMI_EN_OFFSET + SMI_EN_SIZE - 1;
- wdt_ipc_res[1].flags = IORESOURCE_IO;
-
- dev_dbg(&pdev->dev, "watchdog res 0: %pR\n", &wdt_ipc_res[0]);
- dev_dbg(&pdev->dev, "watchdog res 1: %pR\n", &wdt_ipc_res[1]);
-
- wdt_cell.name = TCO_DEVICE_NAME;
- wdt_cell.platform_data = &tco_info;
- wdt_cell.pdata_size = sizeof(tco_info);
- wdt_cell.num_resources = ARRAY_SIZE(wdt_ipc_res);
- wdt_cell.resources = wdt_ipc_res;
- wdt_cell.ignore_resource_conflicts = 1;
-
- return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
- &wdt_cell, 1, NULL, 0, NULL);
+ wdt_resources[0].start = res->start + TCO_BASE_OFFSET;
+ wdt_resources[0].end = res->start + TCO_BASE_OFFSET + TCO_REGS_SIZE - 1;
+ wdt_resources[0].flags = IORESOURCE_IO;
+ wdt_resources[1].start = res->start + SMI_EN_OFFSET;
+ wdt_resources[1].end = res->start + SMI_EN_OFFSET + SMI_EN_SIZE - 1;
+ wdt_resources[1].flags = IORESOURCE_IO;
+
+ return 0;
}
-static int ipc_create_telemetry_device(struct platform_device *pdev)
+static int ipc_get_telemetry_resources(struct platform_device *pdev)
{
- struct resource telemetry_ipc_res[2];
- struct mfd_cell telemetry_cell;
struct resource *res;
/* Get telemetry resources */
@@ -692,44 +692,36 @@ static int ipc_create_telemetry_device(struct platform_device *pdev)
return -ENXIO;
}
- telemetry_ipc_res[0].start = res->start + TELEM_PUNIT_SSRAM_OFFSET;
- telemetry_ipc_res[0].end = res->start +
- TELEM_PUNIT_SSRAM_OFFSET + TELEM_SSRAM_SIZE - 1;
- telemetry_ipc_res[0].flags = IORESOURCE_MEM;
- telemetry_ipc_res[1].start = res->start + TELEM_PMC_SSRAM_OFFSET;
- telemetry_ipc_res[1].end = res->start +
- TELEM_PMC_SSRAM_OFFSET + TELEM_SSRAM_SIZE - 1;
- telemetry_ipc_res[1].flags = IORESOURCE_MEM;
-
- dev_dbg(&pdev->dev, "Telemetry res 0: %pR\n", &telemetry_ipc_res[0]);
- dev_dbg(&pdev->dev, "Telemetry res 1: %pR\n", &telemetry_ipc_res[1]);
-
- telemetry_cell.name = TELEMETRY_DEVICE_NAME;
- telemetry_cell.num_resources = ARRAY_SIZE(telemetry_ipc_res);
- telemetry_cell.resources = telemetry_ipc_res;
- telemetry_cell.ignore_resource_conflicts = 1;
-
- return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
- &telemetry_cell, 1, NULL, 0, NULL);
+ telemetry_resources[0].start = res->start + TELEM_PUNIT_SSRAM_OFFSET;
+ telemetry_resources[0].end = res->start + TELEM_PUNIT_SSRAM_OFFSET +
+ TELEM_SSRAM_SIZE - 1;
+ telemetry_resources[0].flags = IORESOURCE_MEM;
+ telemetry_resources[1].start = res->start + TELEM_PMC_SSRAM_OFFSET;
+ telemetry_resources[1].end = res->start + TELEM_PMC_SSRAM_OFFSET +
+ TELEM_SSRAM_SIZE - 1;
+ telemetry_resources[1].flags = IORESOURCE_MEM;
+
+ return 0;
}
static int ipc_create_pmc_devices(struct platform_device *pdev)
{
int ret;
- ret = ipc_create_punit_device(pdev);
+ ret = ipc_get_wdt_resources(pdev);
if (ret < 0)
return ret;
- ret = ipc_create_wdt_device(pdev);
+ ret = ipc_get_punit_resources(pdev);
if (ret < 0)
return ret;
- ret = ipc_create_telemetry_device(pdev);
+ ret = ipc_get_telemetry_resources(pdev);
if (ret < 0)
return ret;
- return 0;
+ return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, pmc_cell,
+ ARRAY_SIZE(pmc_cell), NULL, 0, NULL);
}
static int ipc_plat_get_res(struct platform_device *pdev)
^ permalink raw reply related
* Re: [PATCH] rtc: Allow rtc drivers to specify the tv_nsec value for ntp
From: Alexandre Belloni @ 2017-11-23 12:04 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jason Gunthorpe, John Stultz, linux-arm-kernel, Thomas Gleixner,
Stephen Boyd, Alessandro Zummo, linux-rtc
In-Reply-To: <20171123112338.GN31757@n2100.armlinux.org.uk>
(Correcting Jason's email)
On 23/11/2017 at 11:23:38 +0000, Russell King - ARM Linux wrote:
> > So I'm discovering that this patch made it upstream silently. While it
> > somewhat solves the issue for some RTCs, it is not a proper solution for
> > most.
> >
> > With this patch, set_offset_nsec will be hardcoded in the driver and
> > this basically work for the mc146818 but many RTCs are connected on a
> > slow bus (let's say i2c) and that bus may have various latencies
> > depending on the platform so the value actually depends on the platform
> > rather than on the RTC itself.
> >
> > As noted by Russell in another thread, there is already a proper
> > solution: do it from userspace as hwclock will already handle most
> > issues.
>
> That's incorrect. hwclock does not have the information to know when
> it should set the time - that is hardware specific. Some RTCs require
> it set .5s before the second flips over, others require it at other
> times.
>
> hwclock has hard-coded the assumption that it's writing to a standard
> PC RTC, so it does the .5s thing, which doesn't give good results on
> various ARM platforms.
>
The 0.5s hardcoding depends on the version of hwclock you use (the
busybox one doesn't do it anymore).
I thought it was handling it better than that and I was indeed
incorrect.
What about setting the time on the RTC once, then using UIE to get the
offset between the set time and the real time then set the time on the
RTC again after accounting for the offset. That would work nicely for
most RTCs.
> Accurately reading the current time is way simpler - hwclock does this
> by watching for the RTC's seconds changing (via the update interrupt
> or emulation.) That's way easier than setting the time.
>
> > I really think that we should simply consider dropping hctosys,
> > systohc and update_persistent_clock. Most distributions have been
> > handling it from userspace for a long time. Openembedded has a
> > startup/shutdown script.
>
> Not every system does though - you have to adjust debian's
> configuration for it to happen at shutdown.
>
> However, that's not the point of the kernel updating the RTC time -
> the point of the RTC updates while synchronised is to reduce the
> disruption that a crash/reboot causes on NTP by allowing the system
> time to be restored as close as possible to the real time as possible.
> If the system has crashed with your idea, the RTC will not have been
> synchronised since who-knows-when, and the RTC could be way out,
> especially if the system has been running for more than a month.
>
But nothing prevents you from using hwclock every 11 minutes from
userspace. I really don't think this should be done from the kernel.
Even better, from userspace you can actually chose the time interval you
want. To me, this all seems to be policy encoded in the kernel.
> > Even better, systemd has a timesyncing daemon (but it doesn't yet do the
> > correct offset calculations).
>
> No thanks. systemd's timesyncing daemon replaces ntpd, so it forces
> you to use systemd if you want this facility. What if you want this
> facility but also facilities from ntpd (eg, for synchronising with
> a reference clock?)
>
> The solution that Alexander and myself have come up with is, IMHO, the
> best solution, even on buses such as I2C buses. I've a bunch of
> follow-up patches that add set_offset_nsec for pcf8583 and armada38x,
> export controls for adjusting that value, and for disabling the NTP
> update.
>
> The idea behind the patches that export those controls is to allow
> set_offset_nsec to be finely adjusted - in order to do that:
>
> 1. you need to synchronise the machine's time keeping to a stable
> reference, let ntp settle.
>
> 2. disable NTP updates of the RTC, measure the RTC drift over a long
> period (eg, a day) and trim the RTC for minimal drift.
>
> 3. enable NTP updates, wait for an update, and measure the offset
> between real time and RTC time, and use that to adjust
> set_offset_nsec.
>
> You only need to do the full procedure if you really care about
> accurate time keeping (eg, you're trying to do something that
> requires stable time.) The point is, these patches _allow_ you to
> do this if you wish. Without them, it's completely impossible to
> use Linux for accurately timestamped monitoring allocations - the
> answer is not "just run ntpd" because if the system time is out,
> it takes ntpd several *hours* to stabilise the systems timekeeping.
>
I really don't think you currently need help from the kernel to do any
of it (apart from adjusting the oscillator obviously). Nothing currently
prevents you to keep a set_offset_nsec in userspace so you can actually
set the time as close as possible to the current time.
I didn't have time to draft a PoC and that is why I didn't reply on the
patch yet.
What I think is needed is a better tool, maybe a daemon, that would
handle both keeping tabs on the needed offset and handle the oscillator
trimming as it may need to get back and forth between two close values.
I still think we need to drop the SYSTOHC and HCTOSYS stuff. I agree it
can't happen overnight as some people are currently relying on it and
they need to migrate.
But having users wondering whether they should keep hwclock or use
SYSTOHC/HCTOSYS is fucked up as SYSTOHC probably doesn't do what they
want if they don't use NTP (and so they still need to be able to set
time from userspace).
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [RFC v8 3/7] platform/x86: intel_pmc_ipc: Use regmap calls for GCR updates
From: Heikki Krogerus @ 2017-11-23 12:29 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy
Cc: a.zummo, x86, wim, mingo, alexandre.belloni, qipeng.zha, hpa,
dvhart, tglx, lee.jones, andy, souvik.k.chakravarty, linux-rtc,
linux-watchdog, linux-kernel, platform-driver-x86, sathyaosid
In-Reply-To: <d071033be09d6902884cb81aa0e5a1e1b5d5fcb0.1509268570.git.sathyanarayanan.kuppuswamy@linux.intel.com>
On Sun, Oct 29, 2017 at 02:49:56AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> This patch adds support for regmap based implementation for GCR
> read/write/update APIs.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> drivers/platform/x86/Kconfig | 1 +
> drivers/platform/x86/intel_pmc_ipc.c | 122 +++++++++++++----------------------
> 2 files changed, 46 insertions(+), 77 deletions(-)
>
> Changes since v7:
> * Fixed style issues.
>
> Changes since v6:
> * None
>
> Changes since v5:
> * None
>
> Changes since v4:
> * None
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 80b8795..45f4e79 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1054,6 +1054,7 @@ config PVPANIC
> config INTEL_PMC_IPC
> tristate "Intel PMC IPC Driver"
> depends on ACPI
> + select REGMAP_MMIO
> ---help---
> This driver provides support for PMC control on some Intel platforms.
> The PMC is an ARC processor which defines IPC commands for communication
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index e36144c..df6af1f 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -35,6 +35,8 @@
> #include <linux/acpi.h>
> #include <linux/io-64-nonatomic-lo-hi.h>
> #include <linux/spinlock.h>
> +#include <linux/mfd/core.h>
You already included that in the previous patch.
Thanks,
--
heikki
^ permalink raw reply
* Re: [PATCH] rtc: Allow rtc drivers to specify the tv_nsec value for ntp
From: Russell King - ARM Linux @ 2017-11-23 12:53 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Jason Gunthorpe, John Stultz, linux-arm-kernel, Thomas Gleixner,
Stephen Boyd, Alessandro Zummo, linux-rtc
In-Reply-To: <20171123120451.v74j3z2c5oodxlud@piout.net>
On Thu, Nov 23, 2017 at 01:04:51PM +0100, Alexandre Belloni wrote:
> (Correcting Jason's email)
>
> On 23/11/2017 at 11:23:38 +0000, Russell King - ARM Linux wrote:
> > > So I'm discovering that this patch made it upstream silently. While it
> > > somewhat solves the issue for some RTCs, it is not a proper solution for
> > > most.
> > >
> > > With this patch, set_offset_nsec will be hardcoded in the driver and
> > > this basically work for the mc146818 but many RTCs are connected on a
> > > slow bus (let's say i2c) and that bus may have various latencies
> > > depending on the platform so the value actually depends on the platform
> > > rather than on the RTC itself.
> > >
> > > As noted by Russell in another thread, there is already a proper
> > > solution: do it from userspace as hwclock will already handle most
> > > issues.
> >
> > That's incorrect. hwclock does not have the information to know when
> > it should set the time - that is hardware specific. Some RTCs require
> > it set .5s before the second flips over, others require it at other
> > times.
> >
> > hwclock has hard-coded the assumption that it's writing to a standard
> > PC RTC, so it does the .5s thing, which doesn't give good results on
> > various ARM platforms.
> >
>
> The 0.5s hardcoding depends on the version of hwclock you use (the
> busybox one doesn't do it anymore).
Right, so it'll be correct for a different set of RTCs and wrong
for others. So, forget current versions of hwclock, none of them
know how to correctly set the time on all RTCs. It fundamentally
lacks the information required, because there's no way for it to
know that information at present.
> I thought it was handling it better than that and I was indeed
> incorrect.
>
> What about setting the time on the RTC once, then using UIE to get the
> offset between the set time and the real time then set the time on the
> RTC again after accounting for the offset. That would work nicely for
> most RTCs.
That could work, but you're looking at making every hwclock based
write take maybe at least two seconds, unless you cache that
information somewhere. If you cache that, then you end up with a
problem when someone (like I do) copies the rootfs between different
platforms. Sometimes I copy SD cards.
Sometimes I even NFS boot the same export on different platforms
(but obviously not at the same time.)
> > Accurately reading the current time is way simpler - hwclock does this
> > by watching for the RTC's seconds changing (via the update interrupt
> > or emulation.) That's way easier than setting the time.
> >
> > > I really think that we should simply consider dropping hctosys,
> > > systohc and update_persistent_clock. Most distributions have been
> > > handling it from userspace for a long time. Openembedded has a
> > > startup/shutdown script.
> >
> > Not every system does though - you have to adjust debian's
> > configuration for it to happen at shutdown.
> >
> > However, that's not the point of the kernel updating the RTC time -
> > the point of the RTC updates while synchronised is to reduce the
> > disruption that a crash/reboot causes on NTP by allowing the system
> > time to be restored as close as possible to the real time as possible.
> > If the system has crashed with your idea, the RTC will not have been
> > synchronised since who-knows-when, and the RTC could be way out,
> > especially if the system has been running for more than a month.
> >
>
> But nothing prevents you from using hwclock every 11 minutes from
> userspace. I really don't think this should be done from the kernel.
It's not just about running hwclock every 11 minutes. It's about
running hwclock when NTP sync'd. If the local clock is not sync'd
you don't want to be running hwclock, especially if you've trimmed
the RTC. So merely throwing hwclock -uw into a cron job really
doesn't solve it.
A way around that would be to install adjtimex, so that the kernel's
NTP flags can be read out. However, that comes with its own set of
problems.
On Debian, installing adjtimex will disrupt the timekeeping because
of the post-install scripts debian runs. It seems Debian assumes
that if you install something, it has the right to modify the system
timekeeping parameters immediately, screwing up ntpd in the process,
if it's running. The thought that you're installing adjtimex because
you want to _inspect_ the kernel ntp parameters is not one that
Debian folk appear to have considered as being a reason for installing
the package.
> I really don't think you currently need help from the kernel to do any
> of it (apart from adjusting the oscillator obviously). Nothing currently
> prevents you to keep a set_offset_nsec in userspace so you can actually
> set the time as close as possible to the current time.
>
> I didn't have time to draft a PoC and that is why I didn't reply on the
> patch yet.
>
> What I think is needed is a better tool, maybe a daemon, that would
> handle both keeping tabs on the needed offset and handle the oscillator
> trimming as it may need to get back and forth between two close values.
>
> I still think we need to drop the SYSTOHC and HCTOSYS stuff. I agree it
> can't happen overnight as some people are currently relying on it and
> they need to migrate.
>
> But having users wondering whether they should keep hwclock or use
> SYSTOHC/HCTOSYS is fucked up as SYSTOHC probably doesn't do what they
> want if they don't use NTP (and so they still need to be able to set
> time from userspace).
Do not go there. Having run systems with no local RTC, I can tell you
that they're a right pain if system time is not properly set before
userspace starts. For example, you sometimes can't get a "ps" listing
because a procfs file contains a "btime" of zero, and it complains
and errors out on that. Other problems have been NFS XIDs which end
up always starting from the same value, so the NFS server thinks they
are being replayed by the client (despite it being an entirely different
kernel being run.) That still exists if you reboot quickly enough.
So no, removing hctosys would be a big mistake.
systohc is more questionable, and always has been. The "push it out
to userspace" approach has been tried in the past, yet we seem to
have it back in the kernel. IIRC, it was originally decided that
rtclib would not support it, and that it was going to be done in
userspace.
However, the userspace part never appeared, and instead rtclib
eventually gained support for it, because it's what people want to
see.
I'm not yet convinced by the "lets push it all into userspace" argument
because that's vapourware - and we've been there before. It's "nice"
to state but if no one steps up and does it, it's of no benefit and
just results in people carrying local hacks (eg in vendor kernels), or
working around those who state it.
I also don't particularly like the concept of trying to measure the
RTC's time-set offset. My userspace programs that measure the RTC
offset show that to get to millisecond resolution, it isn't trivial
because of system timing "noise" - you need to calibrate the reading
side first so you can get an estimation of when the RTC second flips.
You'd then need to set the RTC time, and then re-measure when the RTC
second flips, wait for the appropriate system time and then write the
RTC. You're look at between 2 and 4 seconds for that, and a window
where a perfectly good RTC could be set to an offset time.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Re: [RFC v8 4/7] platform: x86: Add generic Intel IPC driver
From: Heikki Krogerus @ 2017-11-23 13:29 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy
Cc: a.zummo, x86, wim, mingo, alexandre.belloni, qipeng.zha, hpa,
dvhart, tglx, lee.jones, andy, souvik.k.chakravarty, linux-rtc,
linux-watchdog, linux-kernel, platform-driver-x86, sathyaosid
In-Reply-To: <5c66498c110e52343d810db9c281fe72d0d299cc.1509268570.git.sathyanarayanan.kuppuswamy@linux.intel.com>
Hi,
On Sun, Oct 29, 2017 at 02:49:57AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> +/**
> + * devm_intel_ipc_dev_create() - Create IPC device
> + * @dev : IPC parent device.
> + * @devname : Name of the IPC device.
> + * @cfg : IPC device configuration.
> + * @ops : IPC device ops.
> + *
> + * Resource managed API to create IPC device with
> + * given configuration.
> + *
> + * Return : IPC device pointer or ERR_PTR(error code).
> + */
> +struct intel_ipc_dev *devm_intel_ipc_dev_create(struct device *dev,
> + const char *devname,
> + struct intel_ipc_dev_cfg *cfg,
> + struct intel_ipc_dev_ops *ops)
> +{
> + struct intel_ipc_dev **ptr, *ipc_dev;
> + int ret;
> +
> + if (!dev && !devname && !cfg)
> + return ERR_PTR(-EINVAL);
> +
> + if (intel_ipc_dev_get(devname)) {
> + dev_err(dev, "IPC device %s already exist\n", devname);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + ptr = devres_alloc(devm_intel_ipc_dev_release, sizeof(*ptr),
> + GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + ipc_dev = kzalloc(sizeof(*ipc_dev), GFP_KERNEL);
> + if (!ipc_dev) {
> + ret = -ENOMEM;
> + goto err_dev_create;
> + }
> +
> + ipc_dev->dev.class = &intel_ipc_class;
> + ipc_dev->dev.parent = dev;
> + ipc_dev->dev.groups = ipc_dev_groups;
> + ipc_dev->cfg = cfg;
> + ipc_dev->ops = ops;
> +
> + mutex_init(&ipc_dev->lock);
> + init_completion(&ipc_dev->cmd_complete);
> + dev_set_drvdata(&ipc_dev->dev, ipc_dev);
> + dev_set_name(&ipc_dev->dev, devname);
> + device_initialize(&ipc_dev->dev);
> +
> + ret = device_add(&ipc_dev->dev);
> + if (ret < 0) {
> + dev_err(&ipc_dev->dev, "%s device create failed\n",
> + __func__);
> + ret = -ENODEV;
> + goto err_dev_add;
> + }
> +
> + if (ipc_dev->cfg->mode == IPC_DEV_MODE_IRQ) {
> + if (devm_request_irq(&ipc_dev->dev,
> + ipc_dev->cfg->irq,
> + ipc_dev_irq_handler,
> + ipc_dev->cfg->irqflags,
> + dev_name(&ipc_dev->dev),
> + ipc_dev)) {
> + dev_err(&ipc_dev->dev,
> + "Failed to request irq\n");
> + goto err_irq_request;
> + }
> + }
That looks really wrong to me. This is the class driver, so why are
you handling the interrupts of the devices in it? You are clearly
making assumption that the interrupt will always be used only for
command completion, but that may not be the case. No assumptions.
Just define completion callbacks, and let the drivers handle the
actual interrupts.
> + *ptr = ipc_dev;
> +
> + devres_add(dev, ptr);
> + return ipc_dev;
> +
> +err_irq_request:
> + device_del(&ipc_dev->dev);
> +err_dev_add:
> + kfree(ipc_dev);
> +err_dev_create:
> + devres_free(ptr);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(devm_intel_ipc_dev_create);
> +
> +static int __init intel_ipc_init(void)
> +{
> + ipc_channel_lock_init();
> + return class_register(&intel_ipc_class);
> +}
> +
> +static void __exit intel_ipc_exit(void)
> +{
> + class_unregister(&intel_ipc_class);
> +}
> +subsys_initcall(intel_ipc_init);
> +module_exit(intel_ipc_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan<sathyanarayanan.kuppuswamy@linux.intel.com>");
> +MODULE_DESCRIPTION("Intel IPC device class driver");
To be honest, creating an extra logical device for the IPC hosts, and
the entire class, all feel unnecessarily complex to me.
The goal of this patch should be possible to achieve in a much simpler
and flexible way. IMO this patch needs to be redesigned.
Thanks,
--
heikki
^ permalink raw reply
* Re: [RFC v8 0/7] SCU/PMC/PUNIT Inter-Processor Communication(IPC) driver cleanup
From: Heikki Krogerus @ 2017-11-23 13:56 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy
Cc: a.zummo, x86, wim, mingo, alexandre.belloni, qipeng.zha, hpa,
dvhart, tglx, lee.jones, andy, souvik.k.chakravarty, linux-rtc,
linux-watchdog, linux-kernel, platform-driver-x86, sathyaosid
In-Reply-To: <cover.1509268570.git.sathyanarayanan.kuppuswamy@linux.intel.com>
Hi,
On Sun, Oct 29, 2017 at 02:49:53AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> Currently intel_pmc_ipc.c, intel_punit_ipc.c, intel_scu_ipc.c drivers
> implements the same IPC features. This code duplication could be avoided if we
> implement the IPC driver as a generic library and let custom device drivers
> use API provided by generic driver. This patchset mainly addresses this issue.
>
> Along with above code duplication issue, This patchset also addresses
> following issues in intel_pmc_ipc and intel_punit_ipc drivers.
>
> 1. Intel_pmc_ipc.c driver does not use any resource managed (devm_*) calls.
> 2. In Intel_pmc_ipc.c driver, dependent devices like PUNIT, Telemetry and iTCO
> are created manually and uses lot of redundant buffer code.
> 3. Global variable is used to store the IPC device structure and it is used
> across all functions in intel_pmc_ipc.c and intel_punit_ipc.c.
I think those changes are definitely welcome. On top of those, I want
to have regmap for using the IPC. It does not make any sense that the
drivers for the devices attached to for example the PMC, like the
WhiskeyCove PMIC, have to implement their own regmaps.
But a class for the IPC library does feel like overkill to me.
Thanks,
--
heikki
^ permalink raw reply
* Re: [RFC v8 2/7] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices
From: Guenter Roeck @ 2017-11-23 17:08 UTC (permalink / raw)
To: Heikki Krogerus, sathyanarayanan.kuppuswamy
Cc: a.zummo, x86, wim, mingo, alexandre.belloni, qipeng.zha, hpa,
dvhart, tglx, lee.jones, andy, souvik.k.chakravarty, linux-rtc,
linux-watchdog, linux-kernel, platform-driver-x86, sathyaosid,
Andy Shevchenko
In-Reply-To: <20171123114927.GC17418@kuha.fi.intel.com>
On 11/23/2017 03:49 AM, Heikki Krogerus wrote:
> Hi,
>
> On Sun, Oct 29, 2017 at 02:49:55AM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> Currently, we have lot of repetitive code in dependent device resource
>> allocation and device creation handling code. This logic can be improved if
>> we use MFD framework for dependent device creation. This patch adds this
>> support.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>> drivers/platform/x86/intel_pmc_ipc.c | 398 ++++++++++++-----------------------
>> 1 file changed, 139 insertions(+), 259 deletions(-)
>>
>> Changes since v7:
>> * Fixed style issues.
>>
>> Changes since v6:
>> * Fixed style issues.
>> * Used Andy's modified version.
>>
>> Changes since v5:
>> * Changed the order of patches in this patchlist.
>>
>> Changes since v4:
>> * Changed the order of patches in this patchlist.
>>
>> Changes since v3:
>> * Changed PLATFORM_DEVID_AUTO to PLATFORM_DEVID_NONE in mfd device creation.
>> * Fixed error in resource initalization logic in ipc_create_punit_device.
>> * Removed mfd cell id initialization.
>>
>> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
>> index e03fa314..e36144c 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -20,6 +20,7 @@
>> #include <linux/errno.h>
>> #include <linux/init.h>
>> #include <linux/device.h>
>> +#include <linux/mfd/core.h>
>> #include <linux/pm.h>
>> #include <linux/pci.h>
>> #include <linux/platform_device.h>
>> @@ -88,6 +89,7 @@
>> #define PLAT_RESOURCE_ISP_IFACE_INDEX 5
>> #define PLAT_RESOURCE_GTD_DATA_INDEX 6
>> #define PLAT_RESOURCE_GTD_IFACE_INDEX 7
>> +#define PLAT_RESOURCE_MEM_MAX_INDEX 8
>> #define PLAT_RESOURCE_ACPI_IO_INDEX 0
>>
>> /*
>> @@ -106,8 +108,6 @@
>> #define TELEM_SSRAM_SIZE 240
>> #define TELEM_PMC_SSRAM_OFFSET 0x1B00
>> #define TELEM_PUNIT_SSRAM_OFFSET 0x1A00
>> -#define TCO_PMC_OFFSET 0x8
>> -#define TCO_PMC_SIZE 0x4
>>
>> /* PMC register bit definitions */
>>
>> @@ -124,26 +124,10 @@ static struct intel_pmc_ipc_dev {
>> int cmd;
>> struct completion cmd_complete;
>>
>> - /* The following PMC BARs share the same ACPI device with the IPC */
>> - resource_size_t acpi_io_base;
>> - int acpi_io_size;
>> - struct platform_device *tco_dev;
>> -
>> /* gcr */
>> void __iomem *gcr_mem_base;
>> bool has_gcr_regs;
>> spinlock_t gcr_lock;
>> -
>> - /* punit */
>> - struct platform_device *punit_dev;
>> -
>> - /* Telemetry */
>> - resource_size_t telem_pmc_ssram_base;
>> - resource_size_t telem_punit_ssram_base;
>> - int telem_pmc_ssram_size;
>> - int telem_punit_ssram_size;
>> - u8 telem_res_inval;
>> - struct platform_device *telemetry_dev;
>> } ipcdev;
>>
>> static char *ipc_err_sources[] = {
>> @@ -508,7 +492,7 @@ static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> ret = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_pmc_ipc",
>> pmc);
>> if (ret) {
>> - dev_err(&pdev->dev, "Failed to request irq\n");
>> + dev_err(&pdev->dev, "Failed to request IRQ\n");
>> return ret;
>> }
>>
>> @@ -593,44 +577,6 @@ static const struct attribute_group intel_ipc_group = {
>> .attrs = intel_ipc_attrs,
>> };
>>
>> -static struct resource punit_res_array[] = {
>> - /* Punit BIOS */
>> - {
>> - .flags = IORESOURCE_MEM,
>> - },
>> - {
>> - .flags = IORESOURCE_MEM,
>> - },
>> - /* Punit ISP */
>> - {
>> - .flags = IORESOURCE_MEM,
>> - },
>> - {
>> - .flags = IORESOURCE_MEM,
>> - },
>> - /* Punit GTD */
>> - {
>> - .flags = IORESOURCE_MEM,
>> - },
>> - {
>> - .flags = IORESOURCE_MEM,
>> - },
>> -};
>> -
>> -#define TCO_RESOURCE_ACPI_IO 0
>> -#define TCO_RESOURCE_SMI_EN_IO 1
>> -#define TCO_RESOURCE_GCR_MEM 2
>> -static struct resource tco_res[] = {
>> - /* ACPI - TCO */
>> - {
>> - .flags = IORESOURCE_IO,
>> - },
>> - /* ACPI - SMI */
>> - {
>> - .flags = IORESOURCE_IO,
>> - },
>> -};
>> -
>> static struct itco_wdt_platform_data tco_info = {
>> .name = "Apollo Lake SoC",
>> .version = 5,
>> @@ -638,234 +584,177 @@ static struct itco_wdt_platform_data tco_info = {
>> .update_no_reboot_bit = update_no_reboot_bit,
>> };
>>
>> -#define TELEMETRY_RESOURCE_PUNIT_SSRAM 0
>> -#define TELEMETRY_RESOURCE_PMC_SSRAM 1
>> -static struct resource telemetry_res[] = {
>> - /*Telemetry*/
>> - {
>> - .flags = IORESOURCE_MEM,
>> - },
>> - {
>> - .flags = IORESOURCE_MEM,
>> - },
>> -};
>> -
>> -static int ipc_create_punit_device(void)
>> +static int ipc_create_punit_device(struct platform_device *pdev)
>> {
>> - struct platform_device *pdev;
>> - const struct platform_device_info pdevinfo = {
>> - .parent = ipcdev.dev,
>> - .name = PUNIT_DEVICE_NAME,
>> - .id = -1,
>> - .res = punit_res_array,
>> - .num_res = ARRAY_SIZE(punit_res_array),
>> + struct resource punit_res[PLAT_RESOURCE_MEM_MAX_INDEX];
>> + struct mfd_cell punit_cell;
>> + struct resource *res;
>
> That's where you have the bug I reported earlier. You would need to
> introduce those structures as static struct..
>
> But instead of fixing those, drop them and introduce the resources and
> the cells out side of these functions:
>
> static struct resource punit_resources[PLAT_RESOURCE_MEM_MAX_INDEX];
> static struct resource telemetry_resources[2];
> static struct resource wdt_resources[2];
>
> static struct mfd_cell pmc_cell[] = {
> {
> .name = "intel_punit_ipc",
> .resources = punit_resources,
> .num_resources = PLAT_RESOURCE_MEM_MAX_INDEX,
> .ignore_resource_conflicts = true,
> },
> {
> .name = "intel_telemetry",
> .resources = telemetry_resources,
> .num_resources = 2,
> .ignore_resource_conflicts = true,
> },
> {
> .name = "iTCO_wdt",
> .resources = wdt_resources,
> .num_resources = 2,
> .ignore_resource_conflicts = true,
> .platform_data = &tco_info,
> .pdata_size = sizeof(tco_info),
> },
> };
>
> Note that I'm not using the definitions for the name strings on
> purpose. Please get rid of those definitions while at it.
>
> Use these functions - ipc_create_punit/wdt/telemetry_device() - to just
> collect the resources. Then you call devm_mfd_add_devices() only ones
> in ipc_create_pmc_devices(). That should make this driver a bit more
> easier to read and understand.
>
>> + int mindex, pindex = 0;
>> +
>> + for (mindex = 0; mindex <= PLAT_RESOURCE_MEM_MAX_INDEX; mindex++) {
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, mindex);
>> +
>> + switch (mindex) {
>> + /* Get PUNIT resources */
>> + case PLAT_RESOURCE_BIOS_DATA_INDEX:
>> + case PLAT_RESOURCE_BIOS_IFACE_INDEX:
>> + /* BIOS resources are required, so return error if not
>> + * available
>> + */
>> + if (!res) {
>> + dev_err(&pdev->dev,
>> + "Failed to get PUNIT MEM resource %d\n",
>> + pindex);
>> + return -ENXIO;
>> + }
>> + case PLAT_RESOURCE_ISP_DATA_INDEX:
>> + case PLAT_RESOURCE_ISP_IFACE_INDEX:
>> + case PLAT_RESOURCE_GTD_DATA_INDEX:
>> + case PLAT_RESOURCE_GTD_IFACE_INDEX:
>> + /* if valid resource found, copy the resource to PUNIT
>> + * resource
>> + */
>> + if (res)
>> + memcpy(&punit_res[pindex], res, sizeof(*res));
>> + punit_res[pindex].flags = IORESOURCE_MEM;
>> + dev_dbg(&pdev->dev, "PUNIT memory res: %pR\n",
>> + &punit_res[pindex]);
>
> I don't see how is that useful information?
>
>> + pindex++;
>> + break;
>> };
>> + }
>>
>> - pdev = platform_device_register_full(&pdevinfo);
>> - if (IS_ERR(pdev))
>> - return PTR_ERR(pdev);
>> -
>> - ipcdev.punit_dev = pdev;
>> + /* Create PUNIT IPC MFD cell */
>> + punit_cell.name = PUNIT_DEVICE_NAME;
>> + punit_cell.num_resources = ARRAY_SIZE(punit_res);
>> + punit_cell.resources = punit_res;
>> + punit_cell.ignore_resource_conflicts = 1;
>>
>> - return 0;
>> + return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
>> + &punit_cell, 1, NULL, 0, NULL);
>> }
>>
>> -static int ipc_create_tco_device(void)
>> +static int ipc_create_wdt_device(struct platform_device *pdev)
>> {
>> - struct platform_device *pdev;
>> + static struct resource wdt_ipc_res[2];
>> struct resource *res;
>> - const struct platform_device_info pdevinfo = {
>> - .parent = ipcdev.dev,
>> - .name = TCO_DEVICE_NAME,
>> - .id = -1,
>> - .res = tco_res,
>> - .num_res = ARRAY_SIZE(tco_res),
>> - .data = &tco_info,
>> - .size_data = sizeof(tco_info),
>> - };
>> + static struct mfd_cell wdt_cell;
>>
>> - res = tco_res + TCO_RESOURCE_ACPI_IO;
>> - res->start = ipcdev.acpi_io_base + TCO_BASE_OFFSET;
>> - res->end = res->start + TCO_REGS_SIZE - 1;
>> + /* If we have ACPI based watchdog use that instead, othewise create
>> + * a MFD cell for iTCO watchdog
>> + */
>> + if (acpi_has_watchdog())
>> + return 0;
>>
>> - res = tco_res + TCO_RESOURCE_SMI_EN_IO;
>> - res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET;
>> - res->end = res->start + SMI_EN_SIZE - 1;
>> + /* Get iTCO watchdog resources */
>> + res = platform_get_resource(pdev, IORESOURCE_IO,
>> + PLAT_RESOURCE_ACPI_IO_INDEX);
>> + if (!res) {
>> + dev_err(&pdev->dev, "Failed to get WDT resource\n");
>> + return -ENXIO;
>> + }
>>
>> - pdev = platform_device_register_full(&pdevinfo);
>> - if (IS_ERR(pdev))
>> - return PTR_ERR(pdev);
>> + wdt_ipc_res[0].start = res->start + TCO_BASE_OFFSET;
>> + wdt_ipc_res[0].end = res->start +
>> + TCO_BASE_OFFSET + TCO_REGS_SIZE - 1;
>> + wdt_ipc_res[0].flags = IORESOURCE_IO;
>> + wdt_ipc_res[1].start = res->start + SMI_EN_OFFSET;
>> + wdt_ipc_res[1].end = res->start +
>> + SMI_EN_OFFSET + SMI_EN_SIZE - 1;
>> + wdt_ipc_res[1].flags = IORESOURCE_IO;
>>
>> - ipcdev.tco_dev = pdev;
>> + dev_dbg(&pdev->dev, "watchdog res 0: %pR\n", &wdt_ipc_res[0]);
>> + dev_dbg(&pdev->dev, "watchdog res 1: %pR\n", &wdt_ipc_res[1]);
>
> That definitely is not useful information. Please drop all dev_dbg
> calls from these patches.
>
>> - return 0;
>> + wdt_cell.name = TCO_DEVICE_NAME;
>> + wdt_cell.platform_data = &tco_info;
>> + wdt_cell.pdata_size = sizeof(tco_info);
>> + wdt_cell.num_resources = ARRAY_SIZE(wdt_ipc_res);
>> + wdt_cell.resources = wdt_ipc_res;
>> + wdt_cell.ignore_resource_conflicts = 1;
>> +
>> + return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE,
>> + &wdt_cell, 1, NULL, 0, NULL);
Whatever you do, don't tell the mfd maintainer that you are doing this.
You are not supposed to call mfd functions from outside the mfd directory.
Guenter
>> }
>>
>> -static int ipc_create_telemetry_device(void)
>> +static int ipc_create_telemetry_device(struct platform_device *pdev)
>> {
>> - struct platform_device *pdev;
>> + struct resource telemetry_ipc_res[2];
>> + struct mfd_cell telemetry_cell;
>
> This is also broken. I'm attaching a diff with the changes to this
> patch I used when I tested this on my Broxton board.
>
>
> Thanks,
>
^ permalink raw reply
* Re: [PATCH] rtc: Allow rtc drivers to specify the tv_nsec value for ntp
From: J William Piggott @ 2017-11-24 0:13 UTC (permalink / raw)
To: Russell King - ARM Linux, Alexandre Belloni
Cc: Jason Gunthorpe, John Stultz, linux-arm-kernel, Thomas Gleixner,
Stephen Boyd, Alessandro Zummo, linux-rtc
In-Reply-To: <20171123125300.GO31757@n2100.armlinux.org.uk>
On 11/23/2017 07:53 AM, Russell King - ARM Linux wrote:
> On Thu, Nov 23, 2017 at 01:04:51PM +0100, Alexandre Belloni wrote:
>>
8<
>> But nothing prevents you from using hwclock every 11 minutes from
>> userspace. I really don't think this should be done from the kernel.
>
> It's not just about running hwclock every 11 minutes. It's about
> running hwclock when NTP sync'd. If the local clock is not sync'd
> you don't want to be running hwclock, especially if you've trimmed
> the RTC. So merely throwing hwclock -uw into a cron job really
> doesn't solve it.
>
> A way around that would be to install adjtimex, so that the kernel's
> NTP flags can be read out. However, that comes with its own set of
> problems.
>
> On Debian, installing adjtimex will disrupt the timekeeping because
> of the post-install scripts debian runs. It seems Debian assumes
> that if you install something, it has the right to modify the system
> timekeeping parameters immediately, screwing up ntpd in the process,
> if it's running. The thought that you're installing adjtimex because
> you want to _inspect_ the kernel ntp parameters is not one that
> Debian folk appear to have considered as being a reason for installing
> the package.
>
IMO, adjtimex is broken anyway. Use ntptime, it should be included
in the ntp package:
$ /usr/sbin/ntptime | grep status
status 0x40 (UNSYNC),
'ntptime -f ppm' allows correcting the system clock. So adjtimex really
isn't needed anymore.
^ permalink raw reply
* [PATCH] rtc: Remove unused RTC_DEVICE_NAME_SIZE
From: Cole Robinson @ 2017-11-26 0:41 UTC (permalink / raw)
To: linux-rtc; +Cc: a.zummo, alexandre.belloni, linux-kernel, Cole Robinson
The last usage was removed in 5c82a6ae0 when rtc_device.name
was removed
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
Just noticed this when looking at commits a few months back...
include/linux/rtc.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 41319a2e409b..fc6c90b57be0 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -87,7 +87,6 @@ struct rtc_class_ops {
int (*set_offset)(struct device *, long offset);
};
-#define RTC_DEVICE_NAME_SIZE 20
typedef struct rtc_task {
void (*func)(void *private_data);
void *private_data;
--
2.14.3
^ permalink raw reply related
* [rtc-linux] [PATCH] rtc: add support for maxim dallas ds1682
From: venkat.prashanth2498 @ 2017-11-27 6:48 UTC (permalink / raw)
To: alexandre.belloni; +Cc: rtc-linux, linux-kernel, a.zummo, Venkat Prashanth B U
From: Venkat Prashanth B U <venkat.prashanth2498@gmail.com>
Add supporting driver for Dallas Semiconductor
DS1682 2 wire total elapsed time recorder.
Signed-off-by: Venkat Prashanth B U <venkat.prashanth2498@gmail.com>
---
drivers/rtc/rtc-ds1682.c | 284 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 284 insertions(+)
diff --git a/drivers/rtc/rtc-ds1682.c b/drivers/rtc/rtc-ds1682.c
index e69de29..a21c94d 100644
--- a/drivers/rtc/rtc-ds1682.c
+++ b/drivers/rtc/rtc-ds1682.c
@@ -0,0 +1,284 @@
+/* Driver for Dallas Semiconductor DS1682 2 wire total elapsed
+ * time recorder
+ *
+ * Author : Venkat Prashanth B U <venkat.prashanth2498@gmail.com>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/rtc.h>
+#include <linux/types.h>
+#include <linux/bcd.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+
+#define DS1682_READ_MEMORY_CMD 0x1D
+
+#ifndef __LINUX_DS1682_H
+#define __LINUX_DS1682_H
+
+const struct ds1682_platform_data {
+
+ unsigned int gpio_rst;
+ unsigned int gpio_clk;
+ unsigned int gpio_dq;
+};
+#endif
+
+const struct ds1682;
+
+const struct ds1682_chip_ops {
+ int (*map_io)(const struct ds1682 *chip, struct platform_device *pdev,
+ const struct ds1682_platform_data *pdata);
+ void (*unmap_io)(const struct ds1682 *chip);
+};
+
+#define DS1682_RST 0
+#define DS1682_CLK 1
+#define DS1682_DQ 2
+
+const struct ds1682_gpio {
+ const char *name;
+ unsigned int gpio;
+};
+
+const struct ds1682 {
+ const struct ds1682_gpio *gpio;
+ const struct ds1682_chip_ops *ops;
+ const struct rtc_device *rtc;
+};
+
+const struct ds1682_gpio ds1682_gpio[] = {
+ { "RTC RST", 0 },
+ { "RTC CLK", 0 },
+ { "RTC DQ", 0 },
+};
+
+int ds1682_gpio_map(const struct ds1682 *chip, struct platform_device *pdev,
+ const struct ds1682_platform_data *pdata)
+{
+ int i, err;
+
+ ds1682_gpio[DS1682_RST].gpio = pdata->gpio_rst;
+ ds1682_gpio[DS1682_CLK].gpio = pdata->gpio_clk;
+ ds1682_gpio[DS1682_DQ].gpio = pdata->gpio_dq;
+
+ for (i = 0; i < ARRAY_SIZE(ds1682_gpio); i++) {
+ err = gpio_request(ds1682_gpio[i].gpio, ds1682_gpio[i].name);
+ if (err) {
+ dev_err(&pdev->dev, "error mapping gpio %s: %d\n",
+ ds1682_gpio[i].name, err);
+ goto err_request;
+ }
+ if (i != DS1682_DQ)
+ gpio_direction_output(ds1682_gpio[i].gpio, 1);
+ }
+
+ chip->gpio = ds1682_gpio;
+ return 0;
+
+err_request:
+ while (--i >= 0)
+ gpio_free(ds1682_gpio[i].gpio);
+ return err;
+}
+
+static void ds1682_gpio_unmap(const struct ds1682 *chip)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(ds1682_gpio); i++)
+ gpio_free(ds1682_gpio[i].gpio);
+}
+
+static const struct ds1682_chip_ops ds1682_gpio_ops = {
+ .map_io = ds1682_gpio_map,
+ .unmap_io = ds1682_gpio_unmap,
+};
+
+static void ds1682_reset(const struct device *dev)
+{
+ gpio_set_value(ds1682_gpio[DS1682_RST].gpio, 0);
+ udelay(1000);
+ gpio_set_value(ds1682_gpio[DS1682_RST].gpio, 1);
+ gpio_set_value(ds1682_gpio[DS1682_CLK].gpio, 0);
+ gpio_direction_output(ds1682_gpio[DS1682_DQ].gpio, 0);
+ udelay(10);
+}
+
+static void ds1682_write_byte(const struct device *dev, u8 byte)
+{
+ int i;
+
+ gpio_direction_output(ds1682_gpio[DS1682_DQ].gpio, 1);
+ for (i = 0; i < 8; i++) {
+ gpio_set_value(ds1682_gpio[DS1682_DQ].gpio, byte & (1 << i));
+ udelay(10);
+ gpio_set_value(ds1682_gpio[DS1682_CLK].gpio, 1);
+ udelay(10);
+ gpio_set_value(ds1682_gpio[DS1682_CLK].gpio, 0);
+ udelay(10);
+ }
+}
+
+static u8 ds1682_read_byte(const struct device *dev)
+{
+ int i;
+ u8 ret = 0;
+
+ gpio_direction_input(ds1682_gpio[DS1682_DQ].gpio);
+
+ for (i = 0; i < 8; i++) {
+ gpio_set_value(ds1682_gpio[DS1682_CLK].gpio, 0);
+ udelay(10);
+ if (gpio_get_value(ds1682_gpio[DS1682_DQ].gpio))
+ ret |= 1 << i;
+ gpio_set_value(ds1682_gpio[DS1682_CLK].gpio, 1);
+ udelay(10);
+ }
+ return ret;
+}
+
+static void ds1682_read_memory(const struct device *dev, u16 offset,
+ int length, u8 *out)
+{
+ ds1682_reset(dev);
+ ds1682_write_byte(dev, DS1682_READ_MEMORY_CMD);
+ ds1682_write_byte(dev, offset & 0xff);
+ ds1682_write_byte(dev, (offset >> 8) & 0xff);
+ while (length--)
+ *out++ = ds1682_read_byte(dev);
+}
+
+static void ds1682_write_memory(const struct device *dev, u16 offset,
+ int length, u8 *out)
+{
+ int i;
+ u8 ta01, ta02, es;
+
+ ds1682_reset(dev);
+ ds1682_write_byte(dev, offset & 0xff);
+ ds1682_write_byte(dev, (offset >> 8) & 0xff);
+
+ for (i = 0; i < length; i++)
+ ds1682_write_byte(dev, out[i]);
+
+ ds1682_reset(dev);
+ ta01 = ds1682_read_byte(dev);
+ ta02 = ds1682_read_byte(dev);
+ es = ds1682_read_byte(dev);
+
+ for (i = 0; i < length; i++) {
+ if (out[i] != ds1682_read_byte(dev)) {
+ dev_err(dev, "read invalid data\n");
+ return;
+ }
+ }
+
+ ds1682_reset(dev);
+ ds1682_write_byte(dev, ta01);
+ ds1682_write_byte(dev, ta02);
+ ds1682_write_byte(dev, es);
+ gpio_direction_input(ds1682_gpio[DS1682_DQ].gpio);
+while (gpio_get_value(ds1682_gpio[DS1682_DQ].gpio))
+}
+
+static void ds1682_enable_osc(const struct device *dev)
+{
+ u8 in[1] = { 0x10 }; /* enable oscillator */
+
+ ds1682_write_memory(dev, 0x201, 1, in);
+}
+
+static int ds1682_read_time(const struct device *dev, struct rtc_time *dt)
+{
+ unsigned long time = 0;
+
+ ds1682_read_memory(dev, 0x203, 4, (u8 *)&time);
+ time = le32_to_cpu(time);
+ rtc_time_to_tm(time, dt);
+ return rtc_valid_tm(dt);
+}
+
+static int ds1682_set_mmss(const struct device *dev, unsigned long secs)
+{
+ u32 time = cpu_to_le32(secs);
+
+ ds1682_write_memory(dev, 0x203, 4, (u8 *)&time);
+ return 0;
+}
+
+static const struct rtc_class_ops ds1682_rtc_ops = {
+ .read_time = ds1682_read_time,
+ .set_mmss = ds1682_set_mmss,
+};
+
+static int rtc_probe(const struct platform_device *pdev)
+{
+const struct ds1682_platform_data *pdata = dev_get_platdata(&pdev->dev);
+ const struct ds1682 *chip;
+ int retval = -EBUSY;
+
+chip = devm_kzalloc(&pdev->dev, sizeof(const struct ds1682), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->ops = &ds1682_gpio_ops;
+
+ retval = chip->ops->map_io(chip, pdev, pdata);
+ if (retval)
+ goto err_chip;
+
+ dev_info(&pdev->dev, "using GPIOs RST:%d, CLK:%d, DQ:%d\n",
+ chip->gpio[DS1682_RST].gpio, chip->gpio[DS1682_CLK].gpio,
+ chip->gpio[DS1682_DQ].gpio);
+
+ platform_set_drvdata(pdev, chip);
+
+ chip->rtc = devm_rtc_device_register(&pdev->dev, "ds1682",
+ &ds1682_rtc_ops, THIS_MODULE);
+ if (IS_ERR(chip->rtc)) {
+ retval = PTR_ERR(chip->rtc);
+ goto err_io;
+ }
+
+ ds1682_enable_osc(&pdev->dev);
+ return 0;
+
+err_io:
+ chip->ops->unmap_io(chip);
+err_chip:
+ return retval;
+}
+
+static int rtc_remove(const struct platform_device *dev)
+{
+const struct ds1682 *chip = platform_get_drvdata(dev);
+
+ chip->ops->unmap_io(chip);
+
+ return 0;
+}
+
+const struct platform_driver rtc_device_driver = {
+ .probe = rtc_probe,
+ .remove = rtc_remove,
+ .driver = {
+ .name = "ds1682",
+ },
+};
+module_platform_driver(rtc_device_driver);
+
+MODULE_DESCRIPTION("DS1682 Total Elapsed Time Recorder");
+MODULE_AUTHOR("Venkat Prashanth B U <venkat.prashanth2498@gmail.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ds1682");
--
1.9.1
--
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@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply related
* Re: [PATCH] rtc: Allow rtc drivers to specify the tv_nsec value for ntp
From: Alexandre Belloni @ 2017-11-27 20:18 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jason Gunthorpe, John Stultz, linux-arm-kernel, Thomas Gleixner,
Stephen Boyd, Alessandro Zummo, linux-rtc
In-Reply-To: <20171123125300.GO31757@n2100.armlinux.org.uk>
On 23/11/2017 at 12:53:00 +0000, Russell King - ARM Linux wrote:
> On Thu, Nov 23, 2017 at 01:04:51PM +0100, Alexandre Belloni wrote:
> > What about setting the time on the RTC once, then using UIE to get the
> > offset between the set time and the real time then set the time on the
> > RTC again after accounting for the offset. That would work nicely for
> > most RTCs.
>
> That could work, but you're looking at making every hwclock based
> write take maybe at least two seconds, unless you cache that
> information somewhere. If you cache that, then you end up with a
> problem when someone (like I do) copies the rootfs between different
> platforms. Sometimes I copy SD cards.
>
> Sometimes I even NFS boot the same export on different platforms
> (but obviously not at the same time.)
>
I don't think it is really an issue to have setting the RTC time take a
few seconds.
[...]
> > But nothing prevents you from using hwclock every 11 minutes from
> > userspace. I really don't think this should be done from the kernel.
>
> It's not just about running hwclock every 11 minutes. It's about
> running hwclock when NTP sync'd. If the local clock is not sync'd
> you don't want to be running hwclock, especially if you've trimmed
> the RTC. So merely throwing hwclock -uw into a cron job really
> doesn't solve it.
>
Yes and I would hope userspace knows that it is using NTP to sync the
local clock...
> A way around that would be to install adjtimex, so that the kernel's
> NTP flags can be read out. However, that comes with its own set of
> problems.
>
> On Debian, installing adjtimex will disrupt the timekeeping because
> of the post-install scripts debian runs. It seems Debian assumes
> that if you install something, it has the right to modify the system
> timekeeping parameters immediately, screwing up ntpd in the process,
> if it's running. The thought that you're installing adjtimex because
> you want to _inspect_ the kernel ntp parameters is not one that
> Debian folk appear to have considered as being a reason for installing
> the package.
>
Aren't those mainly userspace/integration problems that should be fixed
in Debian?
> > I really don't think you currently need help from the kernel to do any
> > of it (apart from adjusting the oscillator obviously). Nothing currently
> > prevents you to keep a set_offset_nsec in userspace so you can actually
> > set the time as close as possible to the current time.
> >
> > I didn't have time to draft a PoC and that is why I didn't reply on the
> > patch yet.
> >
> > What I think is needed is a better tool, maybe a daemon, that would
> > handle both keeping tabs on the needed offset and handle the oscillator
> > trimming as it may need to get back and forth between two close values.
> >
> > I still think we need to drop the SYSTOHC and HCTOSYS stuff. I agree it
> > can't happen overnight as some people are currently relying on it and
> > they need to migrate.
> >
> > But having users wondering whether they should keep hwclock or use
> > SYSTOHC/HCTOSYS is fucked up as SYSTOHC probably doesn't do what they
> > want if they don't use NTP (and so they still need to be able to set
> > time from userspace).
>
> Do not go there. Having run systems with no local RTC, I can tell you
> that they're a right pain if system time is not properly set before
> userspace starts. For example, you sometimes can't get a "ps" listing
> because a procfs file contains a "btime" of zero, and it complains
> and errors out on that. Other problems have been NFS XIDs which end
> up always starting from the same value, so the NFS server thinks they
> are being replayed by the client (despite it being an entirely different
> kernel being run.) That still exists if you reboot quickly enough.
>
> So no, removing hctosys would be a big mistake.
>
Ok, that is a good point to keep hctosys.
> systohc is more questionable, and always has been. The "push it out
> to userspace" approach has been tried in the past, yet we seem to
> have it back in the kernel. IIRC, it was originally decided that
> rtclib would not support it, and that it was going to be done in
> userspace.
>
> However, the userspace part never appeared, and instead rtclib
> eventually gained support for it, because it's what people want to
> see.
>
> I'm not yet convinced by the "lets push it all into userspace" argument
> because that's vapourware - and we've been there before. It's "nice"
> to state but if no one steps up and does it, it's of no benefit and
> just results in people carrying local hacks (eg in vendor kernels), or
> working around those who state it.
>
What I see is that people that care about precisely setting the RTC are
not using systohc because of the 0.5s offset issue or because it doesn't
do what they want and instead are using their own userspace tool to set
the RTC time.
They have the right to keep those tools closed if they desire so.
The necessary ABI for userspace to do the right thing is already there
and you are going to add a new one.
Also, fixing systohc will not fix the offset for people using hwclock or
any other tools (e.g. systemd).
I pretty much prefer trying to fix existing userspace tools or if this
is not possible have a new tool to handle reading/setting the RTC time.
> I also don't particularly like the concept of trying to measure the
> RTC's time-set offset. My userspace programs that measure the RTC
> offset show that to get to millisecond resolution, it isn't trivial
> because of system timing "noise" - you need to calibrate the reading
> side first so you can get an estimation of when the RTC second flips.
> You'd then need to set the RTC time, and then re-measure when the RTC
> second flips, wait for the appropriate system time and then write the
> RTC. You're look at between 2 and 4 seconds for that, and a window
> where a perfectly good RTC could be set to an offset time.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH] rtc: Allow rtc drivers to specify the tv_nsec value for ntp
From: Jason Gunthorpe @ 2017-11-27 20:29 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Russell King - ARM Linux, John Stultz, linux-arm-kernel,
Thomas Gleixner, Stephen Boyd, Alessandro Zummo, linux-rtc
In-Reply-To: <20171127201812.GB21126@piout.net>
On Mon, Nov 27, 2017 at 09:18:12PM +0100, Alexandre Belloni wrote:
> Also, fixing systohc will not fix the offset for people using hwclock or
> any other tools (e.g. systemd).
When I was talking this over with Russel originally I imagined the
systohc fix as a first step, and we could eventually add an rtc API to
let user space do a precise write to the RTC, with the kernel taking
care of the required sleep, etc.
Jason
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox