| Message ID | 20210625235532.19575-9-dipenp@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Intro to Hardware timestamping engine | expand |
On Sat, Jun 26, 2021 at 1:48 AM Dipen Patel <dipenp@nvidia.com> wrote: > This patch adds new clock type for the GPIO controller which can > timestamp gpio lines using hardware means. To expose such > functionalities to the userspace, code has been added in this patch > where during line create call, it checks for new clock type and if > requested, calls hardware timestamp related API from gpiolib.c. > During line change event, it retrieves timestamp in nano seconds by > calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release, > it disables this functionality by calling gpiod_hw_timestamp_control. > > Signed-off-by: Dipen Patel <dipenp@nvidia.com> This looks good to me, pretty much exactly as I imagine it should be done, and it is also nice that we only implement it for the v2 UAPI. Yours, Linus Walleij
On Sat, Jun 26, 2021 at 1:48 AM Dipen Patel <dipenp@nvidia.com> wrote:
Just a quick question about this:
> + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \
Is the usage intended to be such that since hardware timestamp
can not be guaranteed we need to ask for it and fail and if that
fails maybe the software wants to fall back to the realtime or
common timestamp?
I'm thinking from the view of libgpiod or similar apps that abstract
this and they will be "I want to use hardware timestamps if and
only if it is available, otherwise I want to use this other timestamp"
or is that use case uncommon, such that either you know exactly
what you want or you should not be messing with hardware
timestamps?
Yours,
Linus Walleij
On Fri, Jun 25, 2021 at 04:55:29PM -0700, Dipen Patel wrote: > This patch adds new clock type for the GPIO controller which can > timestamp gpio lines using hardware means. To expose such > functionalities to the userspace, code has been added in this patch > where during line create call, it checks for new clock type and if > requested, calls hardware timestamp related API from gpiolib.c. > During line change event, it retrieves timestamp in nano seconds by > calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release, > it disables this functionality by calling gpiod_hw_timestamp_control. > > Signed-off-by: Dipen Patel <dipenp@nvidia.com> > --- > drivers/gpio/gpiolib-cdev.c | 65 +++++++++++++++++++++++++++++++++++-- > include/uapi/linux/gpio.h | 1 + > 2 files changed, 64 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index 1631727bf0da..9f98c727e937 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -518,6 +518,7 @@ struct linereq { > GPIO_V2_LINE_DRIVE_FLAGS | \ > GPIO_V2_LINE_EDGE_FLAGS | \ > GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \ > + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \ > GPIO_V2_LINE_BIAS_FLAGS) > > static void linereq_put_event(struct linereq *lr, > @@ -540,9 +541,20 @@ static void linereq_put_event(struct linereq *lr, > > static u64 line_event_timestamp(struct line *line) > { > + bool block; > + > if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags)) > return ktime_get_real_ns(); > > + if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &line->desc->flags)) { > + if (irq_count()) > + block = false; > + else > + block = true; > + > + return gpiod_get_hw_timestamp(line->desc, block); > + } > + Use in_task() instead of block? > return ktime_get_ns(); > } > > @@ -828,6 +840,7 @@ static int edge_detector_setup(struct line *line, > return ret; > > line->irq = irq; > + > return 0; > } > Remove gratuitous whitespace changes. If you dislike the formatting then suggest it in a separate patch. > @@ -891,7 +904,6 @@ static int gpio_v2_line_flags_validate(u64 flags) > /* Return an error if an unknown flag is set */ > if (flags & ~GPIO_V2_LINE_VALID_FLAGS) > return -EINVAL; > - > /* > * Do not allow both INPUT and OUTPUT flags to be set as they are > * contradictory. > @@ -900,6 +912,14 @@ static int gpio_v2_line_flags_validate(u64 flags) > (flags & GPIO_V2_LINE_FLAG_OUTPUT)) > return -EINVAL; > Same here. > + /* > + * Do not mix with any other clocks if hardware assisted timestamp is > + * asked. > + */ > + if ((flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME) && > + (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE)) > + return -EINVAL; > + The comment is very hw timestamp centric. It should just be something along the lines of "only allow one event clock source". > /* Edge detection requires explicit input. */ > if ((flags & GPIO_V2_LINE_EDGE_FLAGS) && > !(flags & GPIO_V2_LINE_FLAG_INPUT)) > @@ -992,6 +1012,8 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 flags, > > assign_bit(FLAG_EVENT_CLOCK_REALTIME, flagsp, > flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME); > + assign_bit(FLAG_EVENT_CLOCK_HARDWARE, flagsp, > + flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE); > } > > static long linereq_get_values(struct linereq *lr, void __user *ip) > @@ -1139,6 +1161,18 @@ static long linereq_set_config_unlocked(struct linereq *lr, > int val = gpio_v2_line_config_output_value(lc, i); > > edge_detector_stop(&lr->lines[i]); > + > + /* > + * Assuming line was input before and hardware > + * assisted timestamp only timestamps the input > + * lines. > + */ > + if (gpiod_is_hw_timestamp_enabled(desc)) { > + ret = gpiod_hw_timestamp_control(desc, false); > + if (ret) > + return ret; > + } > + So if you fail to disable the hw timestamp then you fail the set_config? Does that make sense? It should be impossible to fail, as per the preceding edge_detector_stop(), or any failure in this context is irrelevant and so can be ignored. > ret = gpiod_direction_output(desc, val); > if (ret) > return ret; > @@ -1152,6 +1186,13 @@ static long linereq_set_config_unlocked(struct linereq *lr, > polarity_change); > if (ret) > return ret; > + > + /* Check if new config sets hardware assisted clock */ > + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { > + ret = gpiod_hw_timestamp_control(desc, true); > + if (ret) > + return ret; > + } > } > The error code here can come from the pinctrl timestamp_control(), so it should be sanitised before being returned to userspace. > blocking_notifier_call_chain(&desc->gdev->notifier, > @@ -1281,8 +1322,12 @@ static void linereq_free(struct linereq *lr) > > for (i = 0; i < lr->num_lines; i++) { > edge_detector_stop(&lr->lines[i]); > - if (lr->lines[i].desc) > + if (lr->lines[i].desc) { > + if (gpiod_is_hw_timestamp_enabled(lr->lines[i].desc)) > + gpiod_hw_timestamp_control(lr->lines[i].desc, > + false); > gpiod_free(lr->lines[i].desc); > + } Potential race on gpiod_is_hw_timestamp_enabled() and the call to gpiod_hw_timestamp_control()? Why not put the gpiod_is_hw_timestamp_enabled() check inside gpiod_hw_timestamp_control()? And the gpiod_hw_timestamp_control() call should be moved inside gpiod_free(), or more correctly gpiod_free_commit(). i.e. whenever you free the gpio you release any associated hw timestamp. > } > kfifo_free(&lr->events); > kfree(lr->label); > @@ -1409,6 +1454,15 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) > flags & GPIO_V2_LINE_EDGE_FLAGS); > if (ret) > goto out_free_linereq; > + > + /* > + * Check if hardware assisted timestamp is requested > + */ > + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { > + ret = gpiod_hw_timestamp_control(desc, true); > + if (ret) > + goto out_free_linereq; > + } > } > Comment can fit on one line, and probably isn't even necessary - the code is clear enough. > blocking_notifier_call_chain(&desc->gdev->notifier, > @@ -1956,8 +2010,15 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, > if (test_bit(FLAG_EDGE_FALLING, &desc->flags)) > info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING; > > + /* > + * Practically it is possible that user will want both the real time > + * and hardware timestamps on GPIO events, for now however lets just > + * work with either clocks > + */ > if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags)) > info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME; > + else if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &desc->flags)) > + info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE; > If there is any need or intent to support multiple clock sources then avoid creeping API changes and add it now. Either way, drop the comment. > debounce_period_us = READ_ONCE(desc->debounce_period_us); > if (debounce_period_us) { > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h > index eaaea3d8e6b4..d360545b4c21 100644 > --- a/include/uapi/linux/gpio.h > +++ b/include/uapi/linux/gpio.h > @@ -80,6 +80,7 @@ enum gpio_v2_line_flag { > GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN = _BITULL(9), > GPIO_V2_LINE_FLAG_BIAS_DISABLED = _BITULL(10), > GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME = _BITULL(11), > + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE = _BITULL(12), > }; > > /** > -- > 2.17.1 > Cheers, Kent.
On 26/06/2021 00:55, Dipen Patel wrote: > This patch adds new clock type for the GPIO controller which can > timestamp gpio lines using hardware means. To expose such > functionalities to the userspace, code has been added in this patch > where during line create call, it checks for new clock type and if > requested, calls hardware timestamp related API from gpiolib.c. > During line change event, it retrieves timestamp in nano seconds by > calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release, > it disables this functionality by calling gpiod_hw_timestamp_control. > > Signed-off-by: Dipen Patel <dipenp@nvidia.com> > --- > drivers/gpio/gpiolib-cdev.c | 65 +++++++++++++++++++++++++++++++++++-- > include/uapi/linux/gpio.h | 1 + > 2 files changed, 64 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index 1631727bf0da..9f98c727e937 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -518,6 +518,7 @@ struct linereq { > GPIO_V2_LINE_DRIVE_FLAGS | \ > GPIO_V2_LINE_EDGE_FLAGS | \ > GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \ > + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \ > GPIO_V2_LINE_BIAS_FLAGS) > > static void linereq_put_event(struct linereq *lr, > @@ -540,9 +541,20 @@ static void linereq_put_event(struct linereq *lr, > > static u64 line_event_timestamp(struct line *line) > { > + bool block; > + > if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags)) > return ktime_get_real_ns(); > > + if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &line->desc->flags)) { > + if (irq_count()) > + block = false; > + else > + block = true; > + > + return gpiod_get_hw_timestamp(line->desc, block); > + } > + > return ktime_get_ns(); > } Looking at line_event_timestamp() and the callers of this function, it appears that this should always return nanoseconds. Does gpiod_get_hw_timestamp() return nanoseconds? Jon -- nvpublic
On 7/9/21 1:30 AM, Jon Hunter wrote: > On 26/06/2021 00:55, Dipen Patel wrote: >> This patch adds new clock type for the GPIO controller which can >> timestamp gpio lines using hardware means. To expose such >> functionalities to the userspace, code has been added in this patch >> where during line create call, it checks for new clock type and if >> requested, calls hardware timestamp related API from gpiolib.c. >> During line change event, it retrieves timestamp in nano seconds by >> calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release, >> it disables this functionality by calling gpiod_hw_timestamp_control. >> >> Signed-off-by: Dipen Patel <dipenp@nvidia.com> >> --- >> drivers/gpio/gpiolib-cdev.c | 65 +++++++++++++++++++++++++++++++++++-- >> include/uapi/linux/gpio.h | 1 + >> 2 files changed, 64 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c >> index 1631727bf0da..9f98c727e937 100644 >> --- a/drivers/gpio/gpiolib-cdev.c >> +++ b/drivers/gpio/gpiolib-cdev.c >> @@ -518,6 +518,7 @@ struct linereq { >> GPIO_V2_LINE_DRIVE_FLAGS | \ >> GPIO_V2_LINE_EDGE_FLAGS | \ >> GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \ >> + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \ >> GPIO_V2_LINE_BIAS_FLAGS) >> >> static void linereq_put_event(struct linereq *lr, >> @@ -540,9 +541,20 @@ static void linereq_put_event(struct linereq *lr, >> >> static u64 line_event_timestamp(struct line *line) >> { >> + bool block; >> + >> if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags)) >> return ktime_get_real_ns(); >> >> + if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &line->desc->flags)) { >> + if (irq_count()) >> + block = false; >> + else >> + block = true; >> + >> + return gpiod_get_hw_timestamp(line->desc, block); >> + } >> + >> return ktime_get_ns(); >> } > > Looking at line_event_timestamp() and the callers of this function, it > appears that this should always return nanoseconds. Does > gpiod_get_hw_timestamp() return nanoseconds? Yes, it returns in ns to align with line_event_timestamp. > > Jon >
On 7/1/21 7:24 AM, Kent Gibson wrote: > On Fri, Jun 25, 2021 at 04:55:29PM -0700, Dipen Patel wrote: >> This patch adds new clock type for the GPIO controller which can >> timestamp gpio lines using hardware means. To expose such >> functionalities to the userspace, code has been added in this patch >> where during line create call, it checks for new clock type and if >> requested, calls hardware timestamp related API from gpiolib.c. >> During line change event, it retrieves timestamp in nano seconds by >> calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release, >> it disables this functionality by calling gpiod_hw_timestamp_control. >> >> Signed-off-by: Dipen Patel <dipenp@nvidia.com> >> --- >> drivers/gpio/gpiolib-cdev.c | 65 +++++++++++++++++++++++++++++++++++-- >> include/uapi/linux/gpio.h | 1 + >> 2 files changed, 64 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c >> index 1631727bf0da..9f98c727e937 100644 >> --- a/drivers/gpio/gpiolib-cdev.c >> +++ b/drivers/gpio/gpiolib-cdev.c >> @@ -518,6 +518,7 @@ struct linereq { >> GPIO_V2_LINE_DRIVE_FLAGS | \ >> GPIO_V2_LINE_EDGE_FLAGS | \ >> GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \ >> + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \ >> GPIO_V2_LINE_BIAS_FLAGS) >> >> static void linereq_put_event(struct linereq *lr, >> @@ -540,9 +541,20 @@ static void linereq_put_event(struct linereq *lr, >> >> static u64 line_event_timestamp(struct line *line) >> { >> + bool block; >> + >> if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags)) >> return ktime_get_real_ns(); >> >> + if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &line->desc->flags)) { >> + if (irq_count()) >> + block = false; >> + else >> + block = true; >> + >> + return gpiod_get_hw_timestamp(line->desc, block); >> + } >> + > Use in_task() instead of block? yes, will change to in_task. > >> return ktime_get_ns(); >> } >> >> @@ -828,6 +840,7 @@ static int edge_detector_setup(struct line *line, >> return ret; >> >> line->irq = irq; >> + >> return 0; >> } >> > Remove gratuitous whitespace changes. > If you dislike the formatting then suggest it in a separate patch. I will remove this space. > >> @@ -891,7 +904,6 @@ static int gpio_v2_line_flags_validate(u64 flags) >> /* Return an error if an unknown flag is set */ >> if (flags & ~GPIO_V2_LINE_VALID_FLAGS) >> return -EINVAL; >> - >> /* >> * Do not allow both INPUT and OUTPUT flags to be set as they are >> * contradictory. >> @@ -900,6 +912,14 @@ static int gpio_v2_line_flags_validate(u64 flags) >> (flags & GPIO_V2_LINE_FLAG_OUTPUT)) >> return -EINVAL; >> > Same here. > >> + /* >> + * Do not mix with any other clocks if hardware assisted timestamp is >> + * asked. >> + */ >> + if ((flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME) && >> + (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE)) >> + return -EINVAL; >> + > The comment is very hw timestamp centric. It should just be something > along the lines of "only allow one event clock source". Sure, will change it. > >> /* Edge detection requires explicit input. */ >> if ((flags & GPIO_V2_LINE_EDGE_FLAGS) && >> !(flags & GPIO_V2_LINE_FLAG_INPUT)) >> @@ -992,6 +1012,8 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 flags, >> >> assign_bit(FLAG_EVENT_CLOCK_REALTIME, flagsp, >> flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME); >> + assign_bit(FLAG_EVENT_CLOCK_HARDWARE, flagsp, >> + flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE); >> } >> >> static long linereq_get_values(struct linereq *lr, void __user *ip) >> @@ -1139,6 +1161,18 @@ static long linereq_set_config_unlocked(struct linereq *lr, >> int val = gpio_v2_line_config_output_value(lc, i); >> >> edge_detector_stop(&lr->lines[i]); >> + >> + /* >> + * Assuming line was input before and hardware >> + * assisted timestamp only timestamps the input >> + * lines. >> + */ >> + if (gpiod_is_hw_timestamp_enabled(desc)) { >> + ret = gpiod_hw_timestamp_control(desc, false); >> + if (ret) >> + return ret; >> + } >> + > So if you fail to disable the hw timestamp then you fail the set_config? > Does that make sense? > It should be impossible to fail, as per the preceding edge_detector_stop(), > or any failure in this context is irrelevant and so can be ignored. I am planning to remove is_hw_timestamp* API as it is not needed. I will also remove ret check from timestamp_control API as it is not needed. > >> ret = gpiod_direction_output(desc, val); >> if (ret) >> return ret; >> @@ -1152,6 +1186,13 @@ static long linereq_set_config_unlocked(struct linereq *lr, >> polarity_change); >> if (ret) >> return ret; >> + >> + /* Check if new config sets hardware assisted clock */ >> + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { >> + ret = gpiod_hw_timestamp_control(desc, true); >> + if (ret) >> + return ret; >> + } >> } >> > The error code here can come from the pinctrl timestamp_control(), so it > should be sanitised before being returned to userspace. I do not understand what do you mean by sanitise. I just followed what gpiod_direction_output did just above which also returns ret from gpio driver code similar to timestamp_control API. > >> blocking_notifier_call_chain(&desc->gdev->notifier, >> @@ -1281,8 +1322,12 @@ static void linereq_free(struct linereq *lr) >> >> for (i = 0; i < lr->num_lines; i++) { >> edge_detector_stop(&lr->lines[i]); >> - if (lr->lines[i].desc) >> + if (lr->lines[i].desc) { >> + if (gpiod_is_hw_timestamp_enabled(lr->lines[i].desc)) >> + gpiod_hw_timestamp_control(lr->lines[i].desc, >> + false); >> gpiod_free(lr->lines[i].desc); >> + } > Potential race on gpiod_is_hw_timestamp_enabled() and the call to > gpiod_hw_timestamp_control()? > Why not put the gpiod_is_hw_timestamp_enabled() check inside > gpiod_hw_timestamp_control()? > > And the gpiod_hw_timestamp_control() call should be moved inside > gpiod_free(), or more correctly gpiod_free_commit(). > i.e. whenever you free the gpio you release any associated hw timestamp. I am planning to remove is_hw_timestamp* API, that should take care of race condition. For gpiod_free comment, I had thought about it before but then ruled out as it would mean that for all the clients who did not register with HTE, during their gpiod_free call, it has to make unncessary call into HTE, however HTE release_ts has necessary checks which will return without doing anything. Let me know if you still think to move it in gpiod_free. > >> } >> kfifo_free(&lr->events); >> kfree(lr->label); >> @@ -1409,6 +1454,15 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) >> flags & GPIO_V2_LINE_EDGE_FLAGS); >> if (ret) >> goto out_free_linereq; >> + >> + /* >> + * Check if hardware assisted timestamp is requested >> + */ >> + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { >> + ret = gpiod_hw_timestamp_control(desc, true); >> + if (ret) >> + goto out_free_linereq; >> + } >> } >> > Comment can fit on one line, and probably isn't even necessary - the > code is clear enough. I will remove comment. > >> blocking_notifier_call_chain(&desc->gdev->notifier, >> @@ -1956,8 +2010,15 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, >> if (test_bit(FLAG_EDGE_FALLING, &desc->flags)) >> info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING; >> >> + /* >> + * Practically it is possible that user will want both the real time >> + * and hardware timestamps on GPIO events, for now however lets just >> + * work with either clocks >> + */ >> if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags)) >> info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME; >> + else if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &desc->flags)) >> + info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE; >> > If there is any need or intent to support multiple clock sources then > avoid creeping API changes and add it now. > Either way, drop the comment. I will remove comment in next RFC. > >> debounce_period_us = READ_ONCE(desc->debounce_period_us); >> if (debounce_period_us) { >> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h >> index eaaea3d8e6b4..d360545b4c21 100644 >> --- a/include/uapi/linux/gpio.h >> +++ b/include/uapi/linux/gpio.h >> @@ -80,6 +80,7 @@ enum gpio_v2_line_flag { >> GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN = _BITULL(9), >> GPIO_V2_LINE_FLAG_BIAS_DISABLED = _BITULL(10), >> GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME = _BITULL(11), >> + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE = _BITULL(12), >> }; >> >> /** >> -- >> 2.17.1 >> > Cheers, > Kent.
On 6/27/21 4:49 AM, Linus Walleij wrote: > On Sat, Jun 26, 2021 at 1:48 AM Dipen Patel <dipenp@nvidia.com> wrote: > > Just a quick question about this: > >> + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \ > Is the usage intended to be such that since hardware timestamp > can not be guaranteed we need to ask for it and fail and if that > fails maybe the software wants to fall back to the realtime or > common timestamp? > > I'm thinking from the view of libgpiod or similar apps that abstract > this and they will be "I want to use hardware timestamps if and > only if it is available, otherwise I want to use this other timestamp" > or is that use case uncommon, such that either you know exactly > what you want or you should not be messing with hardware > timestamps? The way currently is implemented, if you have requested FLAG_EVENT_CLOCK_HARDWARE and it fails, control will return to userspace with an error. There is no fallback. > > Yours, > Linus Walleij
On Thu, Jul 29, 2021 at 08:07:15PM -0700, Dipen Patel wrote: > > On 7/1/21 7:24 AM, Kent Gibson wrote: <snip> > > > >> ret = gpiod_direction_output(desc, val); > >> if (ret) > >> return ret; > >> @@ -1152,6 +1186,13 @@ static long linereq_set_config_unlocked(struct linereq *lr, > >> polarity_change); > >> if (ret) > >> return ret; > >> + > >> + /* Check if new config sets hardware assisted clock */ > >> + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { > >> + ret = gpiod_hw_timestamp_control(desc, true); > >> + if (ret) > >> + return ret; > >> + } > >> } > >> > > The error code here can come from the pinctrl timestamp_control(), so it > > should be sanitised before being returned to userspace. > > I do not understand what do you mean by sanitise. I just followed what > > gpiod_direction_output did just above which also returns ret from gpio > > driver code similar to timestamp_control API. > In this context, sanitise means convert any kernel internal error codes to their userspace equivalent before returning them to userspace. Fair enough with the gpiod_direction_output() comparison. I was thinking of a patch Andy recently submitted[1] to sanitise gpiod_request(), which can sometimes return EPROBE_DEFER. But I guess we can wait until we find a case of a driver returning an internal error code and add a sanitiser then. Cheers, Kent. [1] https://www.spinics.net/lists/linux-gpio/msg60998.html
On 30/07/2021 03:33, Dipen Patel wrote: > > On 7/9/21 1:30 AM, Jon Hunter wrote: >> On 26/06/2021 00:55, Dipen Patel wrote: >>> This patch adds new clock type for the GPIO controller which can >>> timestamp gpio lines using hardware means. To expose such >>> functionalities to the userspace, code has been added in this patch >>> where during line create call, it checks for new clock type and if >>> requested, calls hardware timestamp related API from gpiolib.c. >>> During line change event, it retrieves timestamp in nano seconds by >>> calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release, >>> it disables this functionality by calling gpiod_hw_timestamp_control. >>> >>> Signed-off-by: Dipen Patel <dipenp@nvidia.com> >>> --- >>> drivers/gpio/gpiolib-cdev.c | 65 +++++++++++++++++++++++++++++++++++-- >>> include/uapi/linux/gpio.h | 1 + >>> 2 files changed, 64 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c >>> index 1631727bf0da..9f98c727e937 100644 >>> --- a/drivers/gpio/gpiolib-cdev.c >>> +++ b/drivers/gpio/gpiolib-cdev.c >>> @@ -518,6 +518,7 @@ struct linereq { >>> GPIO_V2_LINE_DRIVE_FLAGS | \ >>> GPIO_V2_LINE_EDGE_FLAGS | \ >>> GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \ >>> + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \ >>> GPIO_V2_LINE_BIAS_FLAGS) >>> >>> static void linereq_put_event(struct linereq *lr, >>> @@ -540,9 +541,20 @@ static void linereq_put_event(struct linereq *lr, >>> >>> static u64 line_event_timestamp(struct line *line) >>> { >>> + bool block; >>> + >>> if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags)) >>> return ktime_get_real_ns(); >>> >>> + if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &line->desc->flags)) { >>> + if (irq_count()) >>> + block = false; >>> + else >>> + block = true; >>> + >>> + return gpiod_get_hw_timestamp(line->desc, block); >>> + } >>> + >>> return ktime_get_ns(); >>> } >> >> Looking at line_event_timestamp() and the callers of this function, it >> appears that this should always return nanoseconds. Does >> gpiod_get_hw_timestamp() return nanoseconds? > Yes, it returns in ns to align with line_event_timestamp. It might be worth updating the function name to gpiod_get_hw_timestamp_ns() so that this is clear. Jon -- nvpublic
On Tue, Aug 03, 2021 at 03:41:56PM -0700, Dipen Patel wrote: > > On 7/30/21 11:05 PM, Kent Gibson wrote: > > On Thu, Jul 29, 2021 at 08:07:15PM -0700, Dipen Patel wrote: > >> On 7/1/21 7:24 AM, Kent Gibson wrote: > > <snip> > >>>> ret = gpiod_direction_output(desc, val); > >>>> if (ret) > >>>> return ret; > >>>> @@ -1152,6 +1186,13 @@ static long linereq_set_config_unlocked(struct linereq *lr, > >>>> polarity_change); > >>>> if (ret) > >>>> return ret; > >>>> + > >>>> + /* Check if new config sets hardware assisted clock */ > >>>> + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { > >>>> + ret = gpiod_hw_timestamp_control(desc, true); > >>>> + if (ret) > >>>> + return ret; > >>>> + } > >>>> } > >>>> > >>> The error code here can come from the pinctrl timestamp_control(), so it > >>> should be sanitised before being returned to userspace. > >> I do not understand what do you mean by sanitise. I just followed what > >> > >> gpiod_direction_output did just above which also returns ret from gpio > >> > >> driver code similar to timestamp_control API. > >> > > In this context, sanitise means convert any kernel internal error codes > > to their userspace equivalent before returning them to userspace. > > > > Fair enough with the gpiod_direction_output() comparison. I was thinking > > of a patch Andy recently submitted[1] to sanitise gpiod_request(), which > > can sometimes return EPROBE_DEFER. But I guess we can wait until we find > > a case of a driver returning an internal error code and add a sanitiser > > then. > Make sense, I will add sanity check > > But I said don't bother yet. And you need to know what errors to sanitise before you sanitise them - unless you want to run through all the possibilities that can be returned to userspace. Cheers, Kent.
On 7/30/21 11:05 PM, Kent Gibson wrote: > On Thu, Jul 29, 2021 at 08:07:15PM -0700, Dipen Patel wrote: >> On 7/1/21 7:24 AM, Kent Gibson wrote: > <snip> >>>> ret = gpiod_direction_output(desc, val); >>>> if (ret) >>>> return ret; >>>> @@ -1152,6 +1186,13 @@ static long linereq_set_config_unlocked(struct linereq *lr, >>>> polarity_change); >>>> if (ret) >>>> return ret; >>>> + >>>> + /* Check if new config sets hardware assisted clock */ >>>> + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { >>>> + ret = gpiod_hw_timestamp_control(desc, true); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> } >>>> >>> The error code here can come from the pinctrl timestamp_control(), so it >>> should be sanitised before being returned to userspace. >> I do not understand what do you mean by sanitise. I just followed what >> >> gpiod_direction_output did just above which also returns ret from gpio >> >> driver code similar to timestamp_control API. >> > In this context, sanitise means convert any kernel internal error codes > to their userspace equivalent before returning them to userspace. > > Fair enough with the gpiod_direction_output() comparison. I was thinking > of a patch Andy recently submitted[1] to sanitise gpiod_request(), which > can sometimes return EPROBE_DEFER. But I guess we can wait until we find > a case of a driver returning an internal error code and add a sanitiser > then. Make sense, I will add sanity check > > Cheers, > Kent. > > [1] https://www.spinics.net/lists/linux-gpio/msg60998.html >
On 8/3/21 9:42 AM, Jon Hunter wrote: > > On 30/07/2021 03:33, Dipen Patel wrote: >> >> On 7/9/21 1:30 AM, Jon Hunter wrote: >>> On 26/06/2021 00:55, Dipen Patel wrote: >>>> This patch adds new clock type for the GPIO controller which can >>>> timestamp gpio lines using hardware means. To expose such >>>> functionalities to the userspace, code has been added in this patch >>>> where during line create call, it checks for new clock type and if >>>> requested, calls hardware timestamp related API from gpiolib.c. >>>> During line change event, it retrieves timestamp in nano seconds by >>>> calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release, >>>> it disables this functionality by calling gpiod_hw_timestamp_control. >>>> >>>> Signed-off-by: Dipen Patel <dipenp@nvidia.com> >>>> --- >>>> drivers/gpio/gpiolib-cdev.c | 65 +++++++++++++++++++++++++++++++++++-- >>>> include/uapi/linux/gpio.h | 1 + >>>> 2 files changed, 64 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c >>>> index 1631727bf0da..9f98c727e937 100644 >>>> --- a/drivers/gpio/gpiolib-cdev.c >>>> +++ b/drivers/gpio/gpiolib-cdev.c >>>> @@ -518,6 +518,7 @@ struct linereq { >>>> GPIO_V2_LINE_DRIVE_FLAGS | \ >>>> GPIO_V2_LINE_EDGE_FLAGS | \ >>>> GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \ >>>> + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \ >>>> GPIO_V2_LINE_BIAS_FLAGS) >>>> static void linereq_put_event(struct linereq *lr, >>>> @@ -540,9 +541,20 @@ static void linereq_put_event(struct linereq *lr, >>>> static u64 line_event_timestamp(struct line *line) >>>> { >>>> + bool block; >>>> + >>>> if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags)) >>>> return ktime_get_real_ns(); >>>> + if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &line->desc->flags)) { >>>> + if (irq_count()) >>>> + block = false; >>>> + else >>>> + block = true; >>>> + >>>> + return gpiod_get_hw_timestamp(line->desc, block); >>>> + } >>>> + >>>> return ktime_get_ns(); >>>> } >>> >>> Looking at line_event_timestamp() and the callers of this function, it >>> appears that this should always return nanoseconds. Does >>> gpiod_get_hw_timestamp() return nanoseconds? >> Yes, it returns in ns to align with line_event_timestamp. > > > It might be worth updating the function name to gpiod_get_hw_timestamp_ns() so that this is clear. Wouldn't be sufficient to right into its description rather embed in API name? > > Jon > > -- > nvpublic
On Tue, Aug 03, 2021 at 03:51:31PM -0700, Dipen Patel wrote: > > On 8/3/21 9:42 AM, Jon Hunter wrote: > > > > On 30/07/2021 03:33, Dipen Patel wrote: > >> > >> On 7/9/21 1:30 AM, Jon Hunter wrote: > >>> On 26/06/2021 00:55, Dipen Patel wrote: > >>>> This patch adds new clock type for the GPIO controller which can > >>>> timestamp gpio lines using hardware means. To expose such > >>>> functionalities to the userspace, code has been added in this patch > >>>> where during line create call, it checks for new clock type and if > >>>> requested, calls hardware timestamp related API from gpiolib.c. > >>>> During line change event, it retrieves timestamp in nano seconds by > >>>> calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release, > >>>> it disables this functionality by calling gpiod_hw_timestamp_control. > >>>> > >>>> Signed-off-by: Dipen Patel <dipenp@nvidia.com> > >>>> --- > >>>> drivers/gpio/gpiolib-cdev.c | 65 +++++++++++++++++++++++++++++++++++-- > >>>> include/uapi/linux/gpio.h | 1 + > >>>> 2 files changed, 64 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > >>>> index 1631727bf0da..9f98c727e937 100644 > >>>> --- a/drivers/gpio/gpiolib-cdev.c > >>>> +++ b/drivers/gpio/gpiolib-cdev.c > >>>> @@ -518,6 +518,7 @@ struct linereq { > >>>> GPIO_V2_LINE_DRIVE_FLAGS | \ > >>>> GPIO_V2_LINE_EDGE_FLAGS | \ > >>>> GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \ > >>>> + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \ > >>>> GPIO_V2_LINE_BIAS_FLAGS) > >>>> static void linereq_put_event(struct linereq *lr, > >>>> @@ -540,9 +541,20 @@ static void linereq_put_event(struct linereq *lr, > >>>> static u64 line_event_timestamp(struct line *line) > >>>> { > >>>> + bool block; > >>>> + > >>>> if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags)) > >>>> return ktime_get_real_ns(); > >>>> + if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &line->desc->flags)) { > >>>> + if (irq_count()) > >>>> + block = false; > >>>> + else > >>>> + block = true; > >>>> + > >>>> + return gpiod_get_hw_timestamp(line->desc, block); > >>>> + } > >>>> + > >>>> return ktime_get_ns(); > >>>> } > >>> > >>> Looking at line_event_timestamp() and the callers of this function, it > >>> appears that this should always return nanoseconds. Does > >>> gpiod_get_hw_timestamp() return nanoseconds? > >> Yes, it returns in ns to align with line_event_timestamp. > > > > > > It might be worth updating the function name to gpiod_get_hw_timestamp_ns() so that this is clear. > Wouldn't be sufficient to right into its description rather embed in API name? > > Adding a suffix identifying the timestamp resolution to variable and function names is pretty standard in the kernel. It makes the code easier to read as you don't have to keep checking the documentation. Cheers, Kent.
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 1631727bf0da..9f98c727e937 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -518,6 +518,7 @@ struct linereq { GPIO_V2_LINE_DRIVE_FLAGS | \ GPIO_V2_LINE_EDGE_FLAGS | \ GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME | \ + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE | \ GPIO_V2_LINE_BIAS_FLAGS) static void linereq_put_event(struct linereq *lr, @@ -540,9 +541,20 @@ static void linereq_put_event(struct linereq *lr, static u64 line_event_timestamp(struct line *line) { + bool block; + if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &line->desc->flags)) return ktime_get_real_ns(); + if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &line->desc->flags)) { + if (irq_count()) + block = false; + else + block = true; + + return gpiod_get_hw_timestamp(line->desc, block); + } + return ktime_get_ns(); } @@ -828,6 +840,7 @@ static int edge_detector_setup(struct line *line, return ret; line->irq = irq; + return 0; } @@ -891,7 +904,6 @@ static int gpio_v2_line_flags_validate(u64 flags) /* Return an error if an unknown flag is set */ if (flags & ~GPIO_V2_LINE_VALID_FLAGS) return -EINVAL; - /* * Do not allow both INPUT and OUTPUT flags to be set as they are * contradictory. @@ -900,6 +912,14 @@ static int gpio_v2_line_flags_validate(u64 flags) (flags & GPIO_V2_LINE_FLAG_OUTPUT)) return -EINVAL; + /* + * Do not mix with any other clocks if hardware assisted timestamp is + * asked. + */ + if ((flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME) && + (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE)) + return -EINVAL; + /* Edge detection requires explicit input. */ if ((flags & GPIO_V2_LINE_EDGE_FLAGS) && !(flags & GPIO_V2_LINE_FLAG_INPUT)) @@ -992,6 +1012,8 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 flags, assign_bit(FLAG_EVENT_CLOCK_REALTIME, flagsp, flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME); + assign_bit(FLAG_EVENT_CLOCK_HARDWARE, flagsp, + flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE); } static long linereq_get_values(struct linereq *lr, void __user *ip) @@ -1139,6 +1161,18 @@ static long linereq_set_config_unlocked(struct linereq *lr, int val = gpio_v2_line_config_output_value(lc, i); edge_detector_stop(&lr->lines[i]); + + /* + * Assuming line was input before and hardware + * assisted timestamp only timestamps the input + * lines. + */ + if (gpiod_is_hw_timestamp_enabled(desc)) { + ret = gpiod_hw_timestamp_control(desc, false); + if (ret) + return ret; + } + ret = gpiod_direction_output(desc, val); if (ret) return ret; @@ -1152,6 +1186,13 @@ static long linereq_set_config_unlocked(struct linereq *lr, polarity_change); if (ret) return ret; + + /* Check if new config sets hardware assisted clock */ + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { + ret = gpiod_hw_timestamp_control(desc, true); + if (ret) + return ret; + } } blocking_notifier_call_chain(&desc->gdev->notifier, @@ -1281,8 +1322,12 @@ static void linereq_free(struct linereq *lr) for (i = 0; i < lr->num_lines; i++) { edge_detector_stop(&lr->lines[i]); - if (lr->lines[i].desc) + if (lr->lines[i].desc) { + if (gpiod_is_hw_timestamp_enabled(lr->lines[i].desc)) + gpiod_hw_timestamp_control(lr->lines[i].desc, + false); gpiod_free(lr->lines[i].desc); + } } kfifo_free(&lr->events); kfree(lr->label); @@ -1409,6 +1454,15 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) flags & GPIO_V2_LINE_EDGE_FLAGS); if (ret) goto out_free_linereq; + + /* + * Check if hardware assisted timestamp is requested + */ + if (flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE) { + ret = gpiod_hw_timestamp_control(desc, true); + if (ret) + goto out_free_linereq; + } } blocking_notifier_call_chain(&desc->gdev->notifier, @@ -1956,8 +2010,15 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc, if (test_bit(FLAG_EDGE_FALLING, &desc->flags)) info->flags |= GPIO_V2_LINE_FLAG_EDGE_FALLING; + /* + * Practically it is possible that user will want both the real time + * and hardware timestamps on GPIO events, for now however lets just + * work with either clocks + */ if (test_bit(FLAG_EVENT_CLOCK_REALTIME, &desc->flags)) info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME; + else if (test_bit(FLAG_EVENT_CLOCK_HARDWARE, &desc->flags)) + info->flags |= GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE; debounce_period_us = READ_ONCE(desc->debounce_period_us); if (debounce_period_us) { diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h index eaaea3d8e6b4..d360545b4c21 100644 --- a/include/uapi/linux/gpio.h +++ b/include/uapi/linux/gpio.h @@ -80,6 +80,7 @@ enum gpio_v2_line_flag { GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN = _BITULL(9), GPIO_V2_LINE_FLAG_BIAS_DISABLED = _BITULL(10), GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME = _BITULL(11), + GPIO_V2_LINE_FLAG_EVENT_CLOCK_HARDWARE = _BITULL(12), }; /**
This patch adds new clock type for the GPIO controller which can timestamp gpio lines using hardware means. To expose such functionalities to the userspace, code has been added in this patch where during line create call, it checks for new clock type and if requested, calls hardware timestamp related API from gpiolib.c. During line change event, it retrieves timestamp in nano seconds by calling gpiod_get_hw_timestamp API from gpiolib.c. At the line release, it disables this functionality by calling gpiod_hw_timestamp_control. Signed-off-by: Dipen Patel <dipenp@nvidia.com> --- drivers/gpio/gpiolib-cdev.c | 65 +++++++++++++++++++++++++++++++++++-- include/uapi/linux/gpio.h | 1 + 2 files changed, 64 insertions(+), 2 deletions(-)