LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Kent Gibson <warthog618@gmail.com>
To: Dipen Patel <dipenp@nvidia.com>
Cc: thierry.reding@gmail.com, jonathanh@nvidia.com,
	linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-gpio@vger.kernel.org, linus.walleij@linaro.org,
	bgolaszewski@baylibre.com, devicetree@vger.kernel.org,
	linux-doc@vger.kernel.org, robh+dt@kernel.org
Subject: Re: [RFC 06/11] gpiolib: Add HTE support
Date: Thu, 1 Jul 2021 22:24:14 +0800	[thread overview]
Message-ID: <20210701142414.GB34285@sol> (raw)
In-Reply-To: <20210625235532.19575-7-dipenp@nvidia.com>

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.

  parent reply	other threads:[~2021-07-01 14:24 UTC|newest]

Thread overview: 75+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-25 23:55 [RFC 00/11] Intro to Hardware timestamping engine Dipen Patel
2021-06-25 23:55 ` [RFC 01/11] Documentation: Add HTE subsystem guide Dipen Patel
2021-07-04 18:55   ` Jonathan Cameron
2021-07-27 23:44     ` Dipen Patel
2021-08-01 15:24       ` Jonathan Cameron
2021-06-25 23:55 ` [RFC 02/11] drivers: Add HTE subsystem Dipen Patel
2021-06-27 17:40   ` Randy Dunlap
2021-07-04 20:15   ` Jonathan Cameron
2021-07-04 20:45     ` Jonathan Cameron
2021-07-28  5:12       ` Dipen Patel
2021-08-01 16:48         ` Jonathan Cameron
2021-07-28  4:38     ` Dipen Patel
2021-08-01 16:13       ` Jonathan Cameron
2021-09-14  5:43         ` Dipen Patel
2021-09-26 15:42           ` Jonathan Cameron
2021-07-05  7:30   ` Greg KH
2021-07-28  0:34     ` Dipen Patel
2021-06-25 23:55 ` [RFC 03/11] hte: Add tegra194 HTE kernel provider Dipen Patel
2021-07-01 14:21   ` Kent Gibson
2021-07-28 23:59     ` Dipen Patel
2021-07-30  7:01       ` Dipen Patel
2021-07-31 15:43       ` Kent Gibson
2021-08-03 22:40         ` Dipen Patel
2021-08-03 23:02           ` Kent Gibson
2021-08-07  2:41         ` Dipen Patel
2021-08-07  3:07           ` Kent Gibson
2021-08-07  4:52             ` Dipen Patel
2021-08-07  4:51               ` Kent Gibson
2021-08-07  5:35                 ` Dipen Patel
2021-08-07  5:42                   ` Kent Gibson
2021-08-07  5:47                   ` Dipen Patel
2021-07-04 20:27   ` Jonathan Cameron
2021-07-29  2:42     ` Dipen Patel
2021-07-08 23:33   ` Michał Mirosław
2021-07-29  2:43     ` Dipen Patel
2021-06-25 23:55 ` [RFC 04/11] dt-bindings: Add HTE bindings Dipen Patel
2021-06-27 10:56   ` Linus Walleij
2021-07-30  1:32     ` Dipen Patel
2021-07-01 14:02   ` Rob Herring
2021-07-30  1:56     ` Dipen Patel
2021-07-01 15:54   ` Rob Herring
2021-07-30  1:58     ` Dipen Patel
2021-06-25 23:55 ` [RFC 05/11] hte: Add Tegra194 IRQ HTE test driver Dipen Patel
2021-06-27 17:42   ` Randy Dunlap
2021-06-25 23:55 ` [RFC 06/11] gpiolib: Add HTE support Dipen Patel
2021-06-27 11:41   ` Linus Walleij
2021-07-01 14:24   ` Kent Gibson [this message]
2021-07-30  2:25     ` Dipen Patel
2021-07-31  5:13       ` Kent Gibson
2021-06-25 23:55 ` [RFC 07/11] gpio: tegra186: Add HTE in gpio-tegra186 driver Dipen Patel
2021-06-25 23:55 ` [RFC 08/11] gpiolib: cdev: Add hardware timestamp clock type Dipen Patel
2021-06-27 11:38   ` Linus Walleij
2021-06-27 11:49   ` Linus Walleij
2021-07-30  3:16     ` Dipen Patel
2021-07-01 14:24   ` Kent Gibson
2021-07-30  3:07     ` Dipen Patel
2021-07-31  6:05       ` Kent Gibson
2021-08-03 22:41         ` Dipen Patel
2021-08-03 22:38           ` Kent Gibson
2021-07-09  8:30   ` Jon Hunter
2021-07-30  2:33     ` Dipen Patel
2021-08-03 16:42       ` Jon Hunter
2021-08-03 22:51         ` Dipen Patel
2021-08-03 23:09           ` Kent Gibson
2021-06-25 23:55 ` [RFC 09/11] tools: gpio: Add new hardware " Dipen Patel
2021-06-27 11:36   ` Linus Walleij
2021-07-30  3:17     ` Dipen Patel
2021-07-31  6:16       ` Kent Gibson
2021-08-11  9:11         ` Linus Walleij
2021-06-25 23:55 ` [RFC 10/11] hte: Add tegra GPIO HTE test driver Dipen Patel
2021-06-27 17:43   ` Randy Dunlap
2021-06-25 23:55 ` [RFC 11/11] MAINTAINERS: Added HTE Subsystem Dipen Patel
2021-06-27 13:07 ` [RFC 00/11] Intro to Hardware timestamping engine Andy Shevchenko
2021-06-27 14:40   ` Linus Walleij
2021-06-28 12:02     ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210701142414.GB34285@sol \
    --to=warthog618@gmail.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dipenp@nvidia.com \
    --cc=jonathanh@nvidia.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    --subject='Re: [RFC 06/11] gpiolib: Add HTE support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).