Re: [RFC 06/11] gpiolib: Add HTE support

[Dipen Patel <dipenp@nvidia.com>]

On 7/1/21 7:24 AM, Kent Gibson wrote:
> On Fri, Jun 25, 2021 at 04:55:27PM -0700, Dipen Patel wrote:
>> Some GPIO chip can provide hardware timestamp support on its GPIO lines
>> , in order to support that additional functions needs to be added which
>> can talk to both GPIO chip and HTE (hardware timestamping engine)
>> subsystem. This patch introduces functions which gpio consumer can use
>> to request hardware assisted timestamping. Below is the list of the APIs
>> that are added in gpiolib subsystem.
>>
>> - gpiod_hw_timestamp_control - to enable/disable HTE on specified GPIO
>> line. This API will return HTE specific descriptor for the specified
>> GPIO line during the enable call, it will be stored as pointer in the
>> gpio_desc structure as hw_ts_data.
>> - gpiod_is_hw_timestamp_enabled - to query if HTE is enabled on
>> specified GPIO line.
>> - gpiod_get_hw_timestamp - to retrieve hardware timestamps.
>>
>> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
>> ---
>>  drivers/gpio/gpiolib.c        | 92 +++++++++++++++++++++++++++++++++++
>>  drivers/gpio/gpiolib.h        | 11 +++++
>>  include/linux/gpio/consumer.h | 21 +++++++-
>>  include/linux/gpio/driver.h   | 13 +++++
>>  4 files changed, 135 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 220a9d8dd4e3..335eaddfde98 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -2361,6 +2361,98 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
>>  }
>>  EXPORT_SYMBOL_GPL(gpiod_direction_output);
>>  
>> +/**
>> + * gpiod_hw_timestamp_control - set the hardware assisted timestamp control.
>> + * @desc:	GPIO to set
>> + * @enable:	Set true to enable the hardware timestamp, false otherwise.
>> + *
>> + * Certain GPIO chip can rely on hardware assisted timestamp engines which can
>> + * record timestamp at the occurance of the configured events on selected GPIO
>> + * lines. This is helper API to control such engine.
>> + *
>> + * Return 0 in case of success, else an error code.
>> + */
>> +int gpiod_hw_timestamp_control(struct gpio_desc *desc, bool enable)
>> +{
>> +	struct gpio_chip	*gc;
>> +	int			ret = 0;
>> +
>> +	VALIDATE_DESC(desc);
>> +	gc = desc->gdev->chip;
>> +
>> +	if (!gc->timestamp_control) {
>> +		gpiod_warn(desc,
>> +			   "%s: Hardware assisted ts not supported\n",
>> +			   __func__);
>> +		return -ENOTSUPP;
>> +	}
>> +
>> +	ret = gc->timestamp_control(gc, gpio_chip_hwgpio(desc),
>> +				    &desc->hdesc, enable);
>> +
>> +	if (ret) {
>> +		gpiod_warn(desc,
>> +			   "%s: ts control operation failed\n", __func__);
>> +		return ret;
>> +	}
>> +
>> +	if (!enable)
>> +		desc->hdesc = NULL;
>> +
>> +	return ret;
>> +}
> Last I checked, pointer accesses are not guaranteed atomic, so how is
> hdesc protected from concurrent access?
> Here is it modified unprotected.
> Below it is read unprotected.

The assumption I made here was, gpiod_hw_timestamp_control will be

called after client has made at least gpdio_request call. With that assumption,

how two or more client/consumers call gpiod_hw_timestamp_control API

with the same gpio_desc? I believe its not allowed as gpiod_request call will

fail for the looser if there is a race and hence there will not be any race here

in this API. Let me know your thoughts.

>
>> +EXPORT_SYMBOL_GPL(gpiod_hw_timestamp_control);
>> +
>> +/**
>> + * gpiod_is_hw_timestamp_enabled - check if hardware assisted timestamp is
>> + * enabled.
>> + * @desc:	GPIO to check
>> + *
>> + * Return true in case of success, false otherwise.
>> + */
>> +bool gpiod_is_hw_timestamp_enabled(const struct gpio_desc *desc)
>> +{
>> +	if (!desc)
>> +		return false;
>> +
>> +	return (desc->hdesc) ? true : false;
>> +}
>> +EXPORT_SYMBOL_GPL(gpiod_is_hw_timestamp_enabled);
>> +
>> +/**
>> + * gpiod_get_hw_timestamp - Get hardware timestamp in nano seconds.
>> + * @desc:	GPIO to get the timestamp.
>> + * @block:	Set true to block until data is available.
>> + *
>> + * Return non-zero on success, else 0.
>> + */
>> +u64 gpiod_get_hw_timestamp(struct gpio_desc *desc, bool block)
>> +{
>> +	struct gpio_chip	*gc;
>> +	int			ret = 0;
>> +	u64 ts;
>> +
>> +	VALIDATE_DESC(desc);
>> +	gc = desc->gdev->chip;
>> +
>> +	if (!gc->get_hw_timestamp) {
>> +		gpiod_warn(desc,
>> +			   "%s: Hardware assisted ts not supported\n",
>> +			   __func__);
>> +		return -ENOTSUPP;
>> +	}
>> +
> Can't return an error code here.  Return value is u64, so this will look
> like a valid ts.
>
> Just return 0 on error, as you do immediately below...
yes, good catch. I forgot to clean that up.
>
>> +	ret = gc->get_hw_timestamp(gc, block, desc->hdesc, &ts);
>> +	if (ret) {
>> +		gpiod_warn(desc,
>> +			   "%s: get timestamp operation failed\n", __func__);
>> +		return 0;
>> +	}
>> +
>> +	return ts;
>> +}
>> +EXPORT_SYMBOL_GPL(gpiod_get_hw_timestamp);
>> +
>>  /**
>>   * gpiod_set_config - sets @config for a GPIO
>>   * @desc: descriptor of the GPIO for which to set the configuration
>> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
>> index 30bc3f80f83e..5393e1d90f61 100644
>> --- a/drivers/gpio/gpiolib.h
>> +++ b/drivers/gpio/gpiolib.h
>> @@ -15,6 +15,7 @@
>>  #include <linux/device.h>
>>  #include <linux/module.h>
>>  #include <linux/cdev.h>
>> +#include <linux/hte.h>
>>  
>>  #define GPIOCHIP_NAME	"gpiochip"
>>  
>> @@ -117,6 +118,7 @@ struct gpio_desc {
>>  #define FLAG_EDGE_RISING     16	/* GPIO CDEV detects rising edge events */
>>  #define FLAG_EDGE_FALLING    17	/* GPIO CDEV detects falling edge events */
>>  #define FLAG_EVENT_CLOCK_REALTIME	18 /* GPIO CDEV reports REALTIME timestamps in events */
>> +#define FLAG_EVENT_CLOCK_HARDWARE	19 /* GPIO CDEV reports hardware timestamps in events */
>>  
>>  	/* Connection label */
>>  	const char		*label;
>> @@ -129,6 +131,15 @@ struct gpio_desc {
>>  	/* debounce period in microseconds */
>>  	unsigned int		debounce_period_us;
>>  #endif
>> +	/*
>> +	 * Hardware timestamp engine related internal data structure.
>> +	 * This gets set when the consumer calls gpiod_hw_timestamp_control to enable
>> +	 * hardware timestamping on the specified GPIO line. The API calls into HTE
>> +	 * subsystem, in turns HTE subsystem return the HTE descriptor for the GPIO
>> +	 * line. The hdesc will be later used with gpiod_is_hw_timestamp_enabled
>> +	 * and gpiod_get_hw_timestamp API calls.
>> +	 */
>> +	struct hte_ts_desc *hdesc;
>>  };
>>  
>>  #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
>> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
>> index c73b25bc9213..476ee04de7d0 100644
>> --- a/include/linux/gpio/consumer.h
>> +++ b/include/linux/gpio/consumer.h
>> @@ -112,6 +112,9 @@ int gpiod_get_direction(struct gpio_desc *desc);
>>  int gpiod_direction_input(struct gpio_desc *desc);
>>  int gpiod_direction_output(struct gpio_desc *desc, int value);
>>  int gpiod_direction_output_raw(struct gpio_desc *desc, int value);
>> +int gpiod_hw_timestamp_control(struct gpio_desc *desc, bool enable);
>> +bool gpiod_is_hw_timestamp_enabled(const struct gpio_desc *desc);
>> +u64 gpiod_get_hw_timestamp(struct gpio_desc *desc, bool block);
>>  
>>  /* Value get/set from non-sleeping context */
>>  int gpiod_get_value(const struct gpio_desc *desc);
>> @@ -353,8 +356,22 @@ static inline int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
>>  	WARN_ON(desc);
>>  	return -ENOSYS;
>>  }
>> -
>> -
>> +static inline int gpiod_hw_timestamp_control(struct gpio_desc *desc,
>> +					     bool enable)
>> +{
>> +	WARN_ON(desc);
>> +	return -ENOSYS;
>> +}
>> +static inline bool gpiod_is_hw_timestamp_enabled(const struct gpio_desc *desc)
>> +{
>> +	WARN_ON(desc);
>> +	return false;
>> +}
>> +static inline u64 gpiod_get_hw_timestamp(struct gpio_desc *desc, bool block)
>> +{
>> +	WARN_ON(desc);
>> +	return 0;
>> +}
>>  static inline int gpiod_get_value(const struct gpio_desc *desc)
>>  {
>>  	/* GPIO can never have been requested */
>> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
>> index 3a268781fcec..f343e8f54b08 100644
>> --- a/include/linux/gpio/driver.h
>> +++ b/include/linux/gpio/driver.h
>> @@ -10,6 +10,7 @@
>>  #include <linux/lockdep.h>
>>  #include <linux/pinctrl/pinctrl.h>
>>  #include <linux/pinctrl/pinconf-generic.h>
>> +#include <linux/hte.h> /* For hardware timestamping */
>>  
>>  struct gpio_desc;
>>  struct of_phandle_args;
>> @@ -304,6 +305,10 @@ struct gpio_irq_chip {
>>   * @add_pin_ranges: optional routine to initialize pin ranges, to be used when
>>   *	requires special mapping of the pins that provides GPIO functionality.
>>   *	It is called after adding GPIO chip and before adding IRQ chip.
>> + * @timestamp_control: Dependent on GPIO chip, an optional routine to
>> + * 	enable/disable hardware assisted timestamp.
>> + * @get_hw_timestamp: Retrieves hardware timestamp. The consumer can specify
>> + * 	block parameter if it wishes to block till timestamp is available. 
>>   * @base: identifies the first GPIO number handled by this chip;
>>   *	or, if negative during registration, requests dynamic ID allocation.
>>   *	DEPRECATION: providing anything non-negative and nailing the base
>> @@ -396,6 +401,14 @@ struct gpio_chip {
>>  
>>  	int			(*add_pin_ranges)(struct gpio_chip *gc);
>>  
>> +	int			(*timestamp_control)(struct gpio_chip *gc,
>> +						     unsigned int offset,
>> +						     struct hte_ts_desc **hdesc,
>> +						     bool enable);
>> +	int			(*get_hw_timestamp)(struct gpio_chip *gc,
>> +						    bool block,
>> +						    struct hte_ts_desc *hdesc,
>> +						    u64 *ts);
>>  	int			base;
>>  	u16			ngpio;
>>  	const char		*const *names;
>> -- 
>> 2.17.1
>>
> Cheers,
> Kent.