On 7/1/21 7:24 AM, Kent Gibson wrote:

On Fri, Jun 25, 2021 at 04:55:29PM -0700, Dipen Patel wrote:

quoted

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.

quoted

 	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.

quoted

@@ -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.

quoted

+	/*
+	 * 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.

quoted

 	/* 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.

quoted

 			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.

quoted

 		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.

quoted

 	}
 	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.

quoted

 		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.

quoted

 	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.