| Message ID | 20190922175021.53449-5-gwendal@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cros_ec: Add sensorhub driver and FIFO processing | expand |
Hi Gwendal, Some comments below. On 22/9/19 19:50, Gwendal Grignou wrote: > To improve sensor timestamp precision, given EC and AP are in > different time domains, the AP needs to try to record the exact > moment an event was signalled to the AP by the EC as soon as > possible after it happens. > > First thing in the hard irq is the best place for this. > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> > --- > drivers/platform/chrome/cros_ec.c | 18 +++++++++++++++++- > drivers/platform/chrome/cros_ec_lpc.c | 2 ++ > include/linux/platform_data/cros_ec_proto.h | 15 +++++++++++++++ > 3 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c > index fd77e6fa74c2..f49eb1d1e3cd 100644 > --- a/drivers/platform/chrome/cros_ec.c > +++ b/drivers/platform/chrome/cros_ec.c > @@ -31,6 +31,21 @@ static struct cros_ec_platform pd_p = { > .cmd_offset = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX), > }; > > +s64 cros_ec_get_time_ns(void) > +{ > + return ktime_get_boottime_ns(); > +} > +EXPORT_SYMBOL(cros_ec_get_time_ns); > + That's a simple wrapper only to make sure all cros_ec drivers use the same code path, right? So maybe instead of doing this we can just add an inline function in the header like this /** * cros_ec_get_time_ns - Get time since boot including time spend in suspend * * Return: ktime_t format since boot. */ static inline ktime_t cros_ec_get_time_ns(void) { return ktime_get_boottime_ns(); } I'd use ktime_t instead of s64 also. Thanks, Enric > +static irqreturn_t ec_irq_handler(int irq, void *data) > +{ > + struct cros_ec_device *ec_dev = data; > + > + ec_dev->last_event_time = cros_ec_get_time_ns(); > + > + return IRQ_WAKE_THREAD; > +} > + > static irqreturn_t ec_irq_thread(int irq, void *data) > { > struct cros_ec_device *ec_dev = data; > @@ -132,7 +147,8 @@ int cros_ec_register(struct cros_ec_device *ec_dev) > } > > if (ec_dev->irq) { > - err = devm_request_threaded_irq(dev, ec_dev->irq, NULL, > + err = devm_request_threaded_irq( > + dev, ec_dev->irq, ec_irq_handler, > ec_irq_thread, IRQF_TRIGGER_LOW | IRQF_ONESHOT, > "chromeos-ec", ec_dev); > if (err) { > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c > index 7d10d909435f..3c77496e164d 100644 > --- a/drivers/platform/chrome/cros_ec_lpc.c > +++ b/drivers/platform/chrome/cros_ec_lpc.c > @@ -313,6 +313,8 @@ static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data) > { > struct cros_ec_device *ec_dev = data; > > + ec_dev->last_event_time = cros_ec_get_time_ns(); > + > if (ec_dev->mkbp_event_supported && > cros_ec_get_next_event(ec_dev, NULL) > 0) > blocking_notifier_call_chain(&ec_dev->event_notifier, 0, > diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h > index e91e3fcb0348..ab12e28f2107 100644 > --- a/include/linux/platform_data/cros_ec_proto.h > +++ b/include/linux/platform_data/cros_ec_proto.h > @@ -121,6 +121,8 @@ struct cros_ec_command { > * @event_data: Raw payload transferred with the MKBP event. > * @event_size: Size in bytes of the event data. > * @host_event_wake_mask: Mask of host events that cause wake from suspend. > + * @last_event_time: exact time from the hard irq when we got notified of > + * a new event. > * @ec: The platform_device used by the mfd driver to interface with the > * main EC. > * @pd: The platform_device used by the mfd driver to interface with the > @@ -161,6 +163,7 @@ struct cros_ec_device { > int event_size; > u32 host_event_wake_mask; > u32 last_resume_result; > + s64 last_event_time; > > /* The platform devices used by the mfd driver */ > struct platform_device *ec; > @@ -308,4 +311,16 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event); > */ > u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev); > > +/** > + * cros_ec_get_time_ns - Return time in ns. > + * > + * This is the function used to record the time for last_event_time in struct > + * cros_ec_device during the hard irq. > + * > + * This function is probably implemented using ktime_get_boot_ns(), but it's > + * exposed here to make sure all cros_ec drivers use the same code path to get > + * the time. > + */ > +s64 cros_ec_get_time_ns(void); > + > #endif /* __LINUX_CROS_EC_PROTO_H */ >
On Sun, 22 Sep 2019 10:50:12 -0700 Gwendal Grignou <gwendal@chromium.org> wrote: > To improve sensor timestamp precision, given EC and AP are in > different time domains, the AP needs to try to record the exact > moment an event was signalled to the AP by the EC as soon as > possible after it happens. > > First thing in the hard irq is the best place for this. > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org> If this is only going to be used in IIO paths, the there is a control to set which clock is used. This is really a legacy thing due to me once picking a silly default, but we may be causing confusion by having that control but it having no effect. Otherwise looks good to me. > --- > drivers/platform/chrome/cros_ec.c | 18 +++++++++++++++++- > drivers/platform/chrome/cros_ec_lpc.c | 2 ++ > include/linux/platform_data/cros_ec_proto.h | 15 +++++++++++++++ > 3 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c > index fd77e6fa74c2..f49eb1d1e3cd 100644 > --- a/drivers/platform/chrome/cros_ec.c > +++ b/drivers/platform/chrome/cros_ec.c > @@ -31,6 +31,21 @@ static struct cros_ec_platform pd_p = { > .cmd_offset = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX), > }; > > +s64 cros_ec_get_time_ns(void) > +{ > + return ktime_get_boottime_ns(); > +} > +EXPORT_SYMBOL(cros_ec_get_time_ns); > + > +static irqreturn_t ec_irq_handler(int irq, void *data) > +{ > + struct cros_ec_device *ec_dev = data; > + > + ec_dev->last_event_time = cros_ec_get_time_ns(); > + > + return IRQ_WAKE_THREAD; > +} > + > static irqreturn_t ec_irq_thread(int irq, void *data) > { > struct cros_ec_device *ec_dev = data; > @@ -132,7 +147,8 @@ int cros_ec_register(struct cros_ec_device *ec_dev) > } > > if (ec_dev->irq) { > - err = devm_request_threaded_irq(dev, ec_dev->irq, NULL, > + err = devm_request_threaded_irq( > + dev, ec_dev->irq, ec_irq_handler, > ec_irq_thread, IRQF_TRIGGER_LOW | IRQF_ONESHOT, > "chromeos-ec", ec_dev); > if (err) { > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c > index 7d10d909435f..3c77496e164d 100644 > --- a/drivers/platform/chrome/cros_ec_lpc.c > +++ b/drivers/platform/chrome/cros_ec_lpc.c > @@ -313,6 +313,8 @@ static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data) > { > struct cros_ec_device *ec_dev = data; > > + ec_dev->last_event_time = cros_ec_get_time_ns(); > + > if (ec_dev->mkbp_event_supported && > cros_ec_get_next_event(ec_dev, NULL) > 0) > blocking_notifier_call_chain(&ec_dev->event_notifier, 0, > diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h > index e91e3fcb0348..ab12e28f2107 100644 > --- a/include/linux/platform_data/cros_ec_proto.h > +++ b/include/linux/platform_data/cros_ec_proto.h > @@ -121,6 +121,8 @@ struct cros_ec_command { > * @event_data: Raw payload transferred with the MKBP event. > * @event_size: Size in bytes of the event data. > * @host_event_wake_mask: Mask of host events that cause wake from suspend. > + * @last_event_time: exact time from the hard irq when we got notified of > + * a new event. > * @ec: The platform_device used by the mfd driver to interface with the > * main EC. > * @pd: The platform_device used by the mfd driver to interface with the > @@ -161,6 +163,7 @@ struct cros_ec_device { > int event_size; > u32 host_event_wake_mask; > u32 last_resume_result; > + s64 last_event_time; > > /* The platform devices used by the mfd driver */ > struct platform_device *ec; > @@ -308,4 +311,16 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event); > */ > u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev); > > +/** > + * cros_ec_get_time_ns - Return time in ns. > + * > + * This is the function used to record the time for last_event_time in struct > + * cros_ec_device during the hard irq. > + * > + * This function is probably implemented using ktime_get_boot_ns(), but it's > + * exposed here to make sure all cros_ec drivers use the same code path to get > + * the time. > + */ > +s64 cros_ec_get_time_ns(void); > + > #endif /* __LINUX_CROS_EC_PROTO_H */
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c index fd77e6fa74c2..f49eb1d1e3cd 100644 --- a/drivers/platform/chrome/cros_ec.c +++ b/drivers/platform/chrome/cros_ec.c @@ -31,6 +31,21 @@ static struct cros_ec_platform pd_p = { .cmd_offset = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX), }; +s64 cros_ec_get_time_ns(void) +{ + return ktime_get_boottime_ns(); +} +EXPORT_SYMBOL(cros_ec_get_time_ns); + +static irqreturn_t ec_irq_handler(int irq, void *data) +{ + struct cros_ec_device *ec_dev = data; + + ec_dev->last_event_time = cros_ec_get_time_ns(); + + return IRQ_WAKE_THREAD; +} + static irqreturn_t ec_irq_thread(int irq, void *data) { struct cros_ec_device *ec_dev = data; @@ -132,7 +147,8 @@ int cros_ec_register(struct cros_ec_device *ec_dev) } if (ec_dev->irq) { - err = devm_request_threaded_irq(dev, ec_dev->irq, NULL, + err = devm_request_threaded_irq( + dev, ec_dev->irq, ec_irq_handler, ec_irq_thread, IRQF_TRIGGER_LOW | IRQF_ONESHOT, "chromeos-ec", ec_dev); if (err) { diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c index 7d10d909435f..3c77496e164d 100644 --- a/drivers/platform/chrome/cros_ec_lpc.c +++ b/drivers/platform/chrome/cros_ec_lpc.c @@ -313,6 +313,8 @@ static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data) { struct cros_ec_device *ec_dev = data; + ec_dev->last_event_time = cros_ec_get_time_ns(); + if (ec_dev->mkbp_event_supported && cros_ec_get_next_event(ec_dev, NULL) > 0) blocking_notifier_call_chain(&ec_dev->event_notifier, 0, diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h index e91e3fcb0348..ab12e28f2107 100644 --- a/include/linux/platform_data/cros_ec_proto.h +++ b/include/linux/platform_data/cros_ec_proto.h @@ -121,6 +121,8 @@ struct cros_ec_command { * @event_data: Raw payload transferred with the MKBP event. * @event_size: Size in bytes of the event data. * @host_event_wake_mask: Mask of host events that cause wake from suspend. + * @last_event_time: exact time from the hard irq when we got notified of + * a new event. * @ec: The platform_device used by the mfd driver to interface with the * main EC. * @pd: The platform_device used by the mfd driver to interface with the @@ -161,6 +163,7 @@ struct cros_ec_device { int event_size; u32 host_event_wake_mask; u32 last_resume_result; + s64 last_event_time; /* The platform devices used by the mfd driver */ struct platform_device *ec; @@ -308,4 +311,16 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event); */ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev); +/** + * cros_ec_get_time_ns - Return time in ns. + * + * This is the function used to record the time for last_event_time in struct + * cros_ec_device during the hard irq. + * + * This function is probably implemented using ktime_get_boot_ns(), but it's + * exposed here to make sure all cros_ec drivers use the same code path to get + * the time. + */ +s64 cros_ec_get_time_ns(void); + #endif /* __LINUX_CROS_EC_PROTO_H */
To improve sensor timestamp precision, given EC and AP are in different time domains, the AP needs to try to record the exact moment an event was signalled to the AP by the EC as soon as possible after it happens. First thing in the hard irq is the best place for this. Signed-off-by: Gwendal Grignou <gwendal@chromium.org> --- drivers/platform/chrome/cros_ec.c | 18 +++++++++++++++++- drivers/platform/chrome/cros_ec_lpc.c | 2 ++ include/linux/platform_data/cros_ec_proto.h | 15 +++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-)