diff mbox series

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

Message ID 20210625235532.19575-7-dipenp@nvidia.com
State New
Headers show
Series Intro to Hardware timestamping engine | expand

Commit Message

Dipen Patel June 25, 2021, 11:55 p.m. UTC
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(-)

Comments

Linus Walleij June 27, 2021, 11:41 a.m. UTC | #1
On Sat, Jun 26, 2021 at 1:48 AM Dipen Patel <dipenp@nvidia.com> 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>

This looks good to me.

The chip driver can look up and provide a timestamp provider for a
certain line, which is proper since the GPIO hardware will be tightly
coupled with the timestamp hardware so we need to ask the hardware
about this directly and delegate it to the GPIO driver.

Yours,
Linus Walleij
Kent Gibson July 1, 2021, 2:24 p.m. UTC | #2
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.

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

> +	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.
Dipen Patel July 30, 2021, 2:25 a.m. UTC | #3
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.
Kent Gibson July 31, 2021, 5:13 a.m. UTC | #4
On Thu, Jul 29, 2021 at 07:25:36PM -0700, Dipen Patel wrote:
> 
> 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.
> 

My assumptions are that the userspace client is multi-threaded and that
there is nothing preventing concurrent uAPI calls, including closing the
line request fd.

The specific case I had in mind is one thread closing the req fd while
another is using set_config to switch to the hardware event clock.
In that race, the close be calling linereq_free() at the same time the
linereq_set_config_unlocked() is being called.  Both of those functions
make calls to the functions here that read and write the hdesc.

There may be others, e.g. line_event_timestamp() running in the
irq_thread at the same time a set_config call switches the event clock
away from the hardware clock.

So assume concurrent access unless you can prove otherwise.

Cheers,
Kent.
diff mbox series

Patch

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;
+}
+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;
+	}
+
+	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;