mbox series

[RFC,v3,00/12] Intro to Hardware timestamping engine

Message ID 20211123193039.25154-1-dipenp@nvidia.com
Headers show
Series Intro to Hardware timestamping engine | expand

Message

Dipen Patel Nov. 23, 2021, 7:30 p.m. UTC
This patch series introduces new subsystem called hardware timestamping
engine (HTE). It offers functionality such as timestamping through hardware
means in realtime. The HTE subsystem centralizes HTE provider and consumers
where providers can register themselves and the consumers can request
interested entity which could be lines, GPIO, signals or buses. The
HTE subsystem provides timestamp in nano seconds, having said that the provider
need to convert the timestamp if its not in that unit. There was upstream
discussion about the HTE at
https://lore.kernel.org/lkml/4c46726d-fa35-1a95-4295-bca37c8b6fe3@nvidia.com/

To summarize upstream discussion:
- It was heavily favoured by Linus and Kent to extend GPIOLIB and supporting
GPIO drivers to add HTE functionality and I agreed to experiment with it.
This patch series implements and extends GPIOLIB, GPIOLIB-CDEV and GPIO tegra
driver.
- Discussed possibility to add HTE provider as irqchip instead which
was argued against as HTE devices are not necessarily event emitting
devices. This RFC version 2 however tries to emulate threaded irq style
implementation.
- Discussed other possibility if HTE device can be added as posix clock
type like PTP clocks. That was also argues against since HTE devices
are not necessarily tightly coupled with hardware clock.

Typical HTE provider does following:
- Register itself with HTE subsystem
- Provide request, release, enable, disable timestamp and
get_clk_src_info callbacks to HTE subsystem.
- Provide optional xlate callback to the subsystem which can translate
consumer provided logical ids into actual ids of the entity, where entity here
is the provider dependent and could be GPIO, in chip lines or signals, buses
etc...This converted id will be used between HTE subsystem and the provider for
below bullet point.
- Push timestamps to the subsystem. This happens when HTE provider has
timestamp data available and willing to push it to HTE subsystem.
- Unregister itself on exit.

Typical HTE consumer does following:
- Request interested entity it wishes to timestamp in realtime to the
subsystem.
- The subsystem does necessary communications with the provider to
complete the request, which includes translating logical id of the entity to
provider dependent physical/actual id and enabling hardware timestamping on
requested id.
- The request includes callbacks, it will be used to push timestamps.
Optionally, the consumer can provided threaded callback, if specified, the HTE
subsystem will create kernel thread responsible executing the threaded callback.
- Release entity and its resources.

HTE and GPIOLIB:
- For the HTE provider which can timestamp GPIO lines.
- For the GPIO consumers, either in kernel or userspace, The GPIOLIB and its
CDEV framework are extended as frontend to the HTE by introducing new APIs.
- Tegra194 AON GPIO controller has HTE support known as GTE
(Generic Timestamping Engine). The tegra gpio driver is modified to accommodate
HTE functionality.

Changes in V2:
- Removed buffer management and related APIs from the HTE core.
- Removed timestamp retrieve APIs from the HTE core.
- Modified request API with two callbacks, second callback is invoked in thread
context and is optional, while first callback is mandatory and used to push
timestamp data to consumers.
- Replaced hte with hardware-timestamping in DT bindings as hte appeared too
short according to review comments.

Changes in V3:
- Corrected grammatical errors in HTE documentation and its bindings documents
- Removed multi-plural words in the HTE DT bindings
- Reflected changes done in DT bindings in the respective source codes
- Separated previous patch 07 into two patches in this series as 07 and 08
- Corrections in MAINTAINERS file

Dipen Patel (12):
  Documentation: Add HTE subsystem guide
  drivers: Add hardware timestamp engine (HTE)
  hte: Add tegra194 HTE kernel provider
  dt-bindings: Add HTE bindings
  hte: Add Tegra194 IRQ HTE test driver
  gpiolib: Add HTE support
  dt-bindings: gpio: Add hardware-timestamp-engine property
  gpio: tegra186: Add HTE in gpio-tegra186 driver
  gpiolib: cdev: Add hardware timestamp clock type
  tools: gpio: Add new hardware clock type
  hte: Add tegra GPIO HTE test driver
  MAINTAINERS: Added HTE Subsystem

 .../devicetree/bindings/gpio/gpio.txt         |   8 +
 .../bindings/gpio/nvidia,tegra186-gpio.txt    |   7 +
 .../hte/hardware-timestamps-common.yaml       |  29 +
 .../devicetree/bindings/hte/hte-consumer.yaml |  48 +
 .../bindings/hte/nvidia,tegra194-hte.yaml     |  80 ++
 Documentation/hte/hte.rst                     |  84 ++
 Documentation/hte/index.rst                   |  22 +
 Documentation/hte/tegra194-hte.rst            |  57 ++
 Documentation/index.rst                       |   1 +
 MAINTAINERS                                   |   8 +
 drivers/Kconfig                               |   2 +
 drivers/Makefile                              |   1 +
 drivers/gpio/gpio-tegra186.c                  |  89 ++
 drivers/gpio/gpiolib-cdev.c                   | 161 +++-
 drivers/gpio/gpiolib.c                        |  73 ++
 drivers/gpio/gpiolib.h                        |  12 +
 drivers/hte/Kconfig                           |  50 +
 drivers/hte/Makefile                          |   5 +
 drivers/hte/hte-tegra194-gpio-test.c          | 252 +++++
 drivers/hte/hte-tegra194-irq-test.c           | 169 ++++
 drivers/hte/hte-tegra194.c                    | 545 +++++++++++
 drivers/hte/hte.c                             | 907 ++++++++++++++++++
 include/linux/gpio/consumer.h                 |  19 +-
 include/linux/gpio/driver.h                   |  14 +
 include/linux/hte.h                           | 248 +++++
 include/uapi/linux/gpio.h                     |   1 +
 tools/gpio/gpio-event-mon.c                   |   6 +-
 27 files changed, 2886 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hte/hardware-timestamps-common.yaml
 create mode 100644 Documentation/devicetree/bindings/hte/hte-consumer.yaml
 create mode 100644 Documentation/devicetree/bindings/hte/nvidia,tegra194-hte.yaml
 create mode 100644 Documentation/hte/hte.rst
 create mode 100644 Documentation/hte/index.rst
 create mode 100644 Documentation/hte/tegra194-hte.rst
 create mode 100644 drivers/hte/Kconfig
 create mode 100644 drivers/hte/Makefile
 create mode 100644 drivers/hte/hte-tegra194-gpio-test.c
 create mode 100644 drivers/hte/hte-tegra194-irq-test.c
 create mode 100644 drivers/hte/hte-tegra194.c
 create mode 100644 drivers/hte/hte.c
 create mode 100644 include/linux/hte.h


base-commit: 5191249f880367a4cd675825cd721a8d78f26a45

Comments

Kent Gibson Nov. 26, 2021, 1:30 a.m. UTC | #1
On Tue, Nov 23, 2021 at 11:30:29AM -0800, Dipen Patel wrote:
> Some devices can timestamp system lines/signals/Buses in real-time
> using the hardware counter or other hardware means which can give
> finer granularity and help avoid jitter introduced by software
> timestamping. To utilize such functionality, this patchset creates
> HTE subsystem where devices can register themselves as providers so
> that the consumers devices can request specific line from the
> providers.
> 
> It provides below APIs for the provider:
> - devm_hte_register_chip -- To register the HTE provider.
> - hte_push_ts_ns() -- To push timestamp data into HTE subsystem.
> 
> It provides below APIs for the consumer:
> - devm_of_hte_request_ts() -- Request timestamp functionality.
> - hte_req_ts_by_hte_name() -- To request timestamp functionality by
> using HTE provider dt node.
> - hte_enable_ts() -- To disable timestamp functionality.
> - hte_disable_ts() -- To enable timestamp functionality.
> - hte_release_ts() -- To release timestamp functionality and its
> associated resources.
> - hte_get_clk_src_info() -- To query clock source information from
> the provider
> 
> The detail about parameters and API usage are described in each
> functions definitions in drivers/hte/hte.c file.
> 
> The patch adds compilation support in Makefile and menu options in
> Kconfig.
> 
> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
> Changes in v2:
> - Removed buffer abstraction layer as well related APIs, HTE now will not store
> any data, instead will pass to consumer as soon as it is available.
> - Removed unnecessary dynamical allocations
> - Removed timestamp retrieve API
> - Removed release, unregister related APIs as their counterpart are resource
> managed.
> - Added kernel thread implementation if consumer indicates threaded callback
> during request API time.
> - Changed hte_req_ts_by_dt_node API to remove device node exposure from the
> interface, instead consumer will their device node with property name that
> indicates the provider it wants to use.
> 
> Changes in v3:
> - Addressed grammatical/spelling errors.
> 
>  drivers/Kconfig      |   2 +
>  drivers/Makefile     |   1 +
>  drivers/hte/Kconfig  |  22 ++
>  drivers/hte/Makefile |   2 +
>  drivers/hte/hte.c    | 907 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/hte.h  | 248 ++++++++++++
>  6 files changed, 1182 insertions(+)
>  create mode 100644 drivers/hte/Kconfig
>  create mode 100644 drivers/hte/Makefile
>  create mode 100644 drivers/hte/hte.c
>  create mode 100644 include/linux/hte.h
> 
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 0d399ddaa185..b3c8332bf5c7 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -236,4 +236,6 @@ source "drivers/interconnect/Kconfig"
>  source "drivers/counter/Kconfig"
>  
>  source "drivers/most/Kconfig"
> +
> +source "drivers/hte/Kconfig"
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index be5d40ae1488..087c08e846e6 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -188,3 +188,4 @@ obj-$(CONFIG_GNSS)		+= gnss/
>  obj-$(CONFIG_INTERCONNECT)	+= interconnect/
>  obj-$(CONFIG_COUNTER)		+= counter/
>  obj-$(CONFIG_MOST)		+= most/
> +obj-$(CONFIG_HTE)		+= hte/
> diff --git a/drivers/hte/Kconfig b/drivers/hte/Kconfig
> new file mode 100644
> index 000000000000..1fcfe17cf28a
> --- /dev/null
> +++ b/drivers/hte/Kconfig
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +menuconfig HTE
> +	bool "Hardware Timestamping Engine (HTE) Support"
> +	help
> +	  Hardware Timestamping Engine (HTE) Support.
> +
> +	  Some devices provide a hardware timestamping engine which can
> +	  timestamp certain device lines/signals in realtime. This provides a
> +	  hardware-assisted timestamp to generic signals like GPIOs or IRQs
> +	  lines. It comes with a benefit for applications like autonomous
> +	  machines needing accurate timestamping event with less jitter.
> +
> +	  This framework provides a generic interface to such HTE devices
> +	  within the Linux kernel. It provides an API to register and
> +	  unregister a HTE provider chip, configurable software buffer to
> +	  store the timestamps, push the timestamp from the HTE providers and
> +	  retrieve timestamps for the consumers. It also provides means for the
> +	  consumers to request signals it wishes to hardware timestamp and
> +	  release them if not required.
> +
> +	  If unsure, say no.
> +
> diff --git a/drivers/hte/Makefile b/drivers/hte/Makefile
> new file mode 100644
> index 000000000000..fc03bdf44427
> --- /dev/null
> +++ b/drivers/hte/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_HTE)		+= hte.o
> +
> diff --git a/drivers/hte/hte.c b/drivers/hte/hte.c
> new file mode 100644
> index 000000000000..ca91d48f48b9
> --- /dev/null
> +++ b/drivers/hte/hte.c
> @@ -0,0 +1,907 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021 NVIDIA Corporation
> + *
> + * Author: Dipen Patel <dipenp@nvidia.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/mutex.h>
> +#include <linux/sched.h>
> +#include <linux/uaccess.h>
> +#include <linux/hte.h>
> +#include <linux/delay.h>
> +#include <linux/debugfs.h>
> +#include <linux/kthread.h>
> +
> +#define HTE_TS_NAME_LEN		10
> +
> +/* Global list of the HTE devices */
> +static DEFINE_SPINLOCK(hte_lock);
> +static LIST_HEAD(hte_devices);
> +
> +enum {
> +	HTE_TS_REGISTERED,
> +	HTE_TS_DISABLE,
> +};
> +
> +enum {
> +	HTE_CB_RUN_THREAD,
> +	HTE_CB_NUM,
> +};
> +
> +/**
> + * struct hte_ts_info - Information related to requested timestamp.
> + *
> + * @xlated_id: Timestamp ID as understood between HTE subsys and HTE provider,
> + * See xlate callback API.
> + * @flags: Flags holding state informations.
> + * @hte_cb_flags: Callback related flags.
> + * @seq: Timestamp sequence counter.
> + * @hte_name: Indicates if HTE core has set name for this timestamp entity.
> + * @cb: Callback function provided by clients.
> + * @tcb: Threaded callback function provided by clients.
> + * @dropped_ts: Dropped timestamps.
> + * @slock: Spin lock.
> + * @thread: Thread task when tcb is provided.
> + * @req_mlock: Lock during timestamp request/release APIs.
> + * @ts_dbg_root: Root for the debug fs.
> + * @gdev: HTE abstract device that this timestamp belongs to.
> + * @cl_data: Client specific data.
> + */
> +struct hte_ts_info {
> +	u32 xlated_id;
> +	unsigned long flags;
> +	unsigned long hte_cb_flags;
> +	u64 seq;
> +	bool hte_name;
> +	hte_ts_cb_t cb;
> +	hte_ts_threaded_cb_t tcb;
> +	atomic_t dropped_ts;
> +	spinlock_t slock;
> +	struct task_struct *thread;
> +	struct mutex req_mlock;
> +	struct dentry *ts_dbg_root;
> +	struct hte_device *gdev;
> +	void *cl_data;
> +};
> +
> +/**
> + * struct hte_device - HTE abstract device
> + * @nlines: Number of entities this device supports.
> + * @ts_req: Total number of entities requested.
> + * @sdev: Device used at various debug prints.
> + * @dbg_root: Root directory for debug fs.
> + * @list: List node to store hte_device for each provider.
> + * @chip: HTE chip providing this HTE device.
> + * @owner: helps prevent removal of modules when in use.
> + * @ei: Timestamp information.
> + */
> +struct hte_device {
> +	u32 nlines;
> +	atomic_t ts_req;
> +	struct device *sdev;
> +	struct dentry *dbg_root;
> +	struct list_head list;
> +	struct hte_chip *chip;
> +	struct module *owner;
> +	struct hte_ts_info ei[];
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static struct dentry *hte_root;
> +
> +static int __init hte_subsys_dbgfs_init(void)
> +{
> +	/* creates /sys/kernel/debug/hte/ */
> +	hte_root = debugfs_create_dir("hte", NULL);
> +
> +	return 0;
> +}
> +subsys_initcall(hte_subsys_dbgfs_init);
> +
> +static void hte_chip_dbgfs_init(struct hte_device *gdev)
> +{
> +	const struct hte_chip *chip = gdev->chip;
> +	const char *name = chip->name ? chip->name : dev_name(chip->dev);
> +
> +	gdev->dbg_root = debugfs_create_dir(name, hte_root);
> +
> +	debugfs_create_atomic_t("ts_requested", 0444, gdev->dbg_root,
> +				&gdev->ts_req);
> +	debugfs_create_u32("total_ts", 0444, gdev->dbg_root,
> +			   &gdev->nlines);
> +}
> +
> +static void hte_ts_dbgfs_init(const char *name, struct hte_ts_info *ei)
> +{
> +	if (!ei->gdev->dbg_root || !name)
> +		return;
> +
> +	ei->ts_dbg_root = debugfs_create_dir(name, ei->gdev->dbg_root);
> +
> +	debugfs_create_atomic_t("dropped_timestamps", 0444, ei->ts_dbg_root,
> +				&ei->dropped_ts);
> +}
> +
> +#else
> +
> +static void hte_chip_dbgfs_init(struct hte_device *gdev)
> +{
> +}
> +
> +static void hte_ts_dbgfs_init(const char *name, struct hte_ts_info *ei)
> +{
> +}
> +
> +#endif
> +
> +/**
> + * hte_release_ts() - Consumer calls this API to release the entity, where
> + * entity could be anything providers support, like lines, signals, buses,
> + * etc...
> + *
> + * @desc: timestamp descriptor, this is the same as returned by the request API.
> + *
> + * Context: debugfs_remove_recursive() function call may use sleeping locks,
> + *	    not suitable from atomic context.
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int hte_release_ts(struct hte_ts_desc *desc)
> +{
> +	u32 id;
> +	int ret = 0;
> +	unsigned long flag;
> +	struct hte_device *gdev;
> +	struct hte_ts_info *ei;
> +
> +	if (!desc)
> +		return -EINVAL;
> +
> +	ei = desc->hte_data;
> +
> +	if (!ei || !ei->gdev)
> +		return -EINVAL;
> +
> +	gdev = ei->gdev;
> +	id = desc->con_id;
> +
> +	mutex_lock(&ei->req_mlock);
> +
> +	if (!test_bit(HTE_TS_REGISTERED, &ei->flags)) {
> +		dev_info(gdev->sdev, "id:%d is not registered", id);
> +		ret = -EUSERS;
> +		goto unlock;
> +	}
> +
> +	ret = gdev->chip->ops->release(gdev->chip, ei->xlated_id);
> +	if (ret) {
> +		dev_err(gdev->sdev, "id: %d free failed\n", id);
> +		goto unlock;
> +	}
> +
> +	if (ei->hte_name)
> +		kfree(desc->name);
> +
> +	debugfs_remove_recursive(ei->ts_dbg_root);
> +
> +	spin_lock_irqsave(&ei->slock, flag);
> +
> +	atomic_dec(&gdev->ts_req);
> +	atomic_set(&ei->dropped_ts, 0);
> +
> +	ei->seq = 0;
> +	desc->hte_data = NULL;
> +
> +	clear_bit(HTE_TS_REGISTERED, &ei->flags);
> +
> +	spin_unlock_irqrestore(&ei->slock, flag);
> +
> +	if (ei->tcb) {
> +		kthread_stop(ei->thread);
> +		put_task_struct(ei->thread);
> +	}
> +
> +	ei->cb = NULL;
> +	ei->tcb = NULL;
> +	ei->thread = NULL;
> +	ei->cl_data = NULL;
> +
> +	module_put(gdev->owner);
> +unlock:
> +	mutex_unlock(&ei->req_mlock);
> +	dev_dbg(gdev->sdev, "release id: %d\n", id);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(hte_release_ts);
> +
> +static int hte_ts_dis_en_common(struct hte_ts_desc *desc, bool en)
> +{
> +	u32 ts_id;
> +	struct hte_device *gdev;
> +	struct hte_ts_info *ei;
> +	int ret;
> +	unsigned long flag;
> +
> +	if (!desc)
> +		return -EINVAL;
> +
> +	ei = desc->hte_data;
> +
> +	if (!ei || !ei->gdev)
> +		return -EINVAL;
> +
> +	gdev = ei->gdev;
> +	ts_id = desc->con_id;
> +
> +	mutex_lock(&ei->req_mlock);
> +
> +	if (!test_bit(HTE_TS_REGISTERED, &ei->flags)) {
> +		dev_dbg(gdev->sdev, "id:%d is not registered", ts_id);
> +		ret = -EUSERS;
> +		goto out;
> +	}
> +
> +	spin_lock_irqsave(&ei->slock, flag);
> +
> +	if (en) {
> +		if (!test_bit(HTE_TS_DISABLE, &ei->flags)) {
> +			ret = 0;
> +			goto out_unlock;
> +		}
> +
> +		spin_unlock_irqrestore(&ei->slock, flag);
> +		ret = gdev->chip->ops->enable(gdev->chip, ei->xlated_id);
> +		if (ret) {
> +			dev_warn(gdev->sdev, "id: %d enable failed\n",
> +				 ts_id);
> +			goto out;
> +		}
> +
> +		spin_lock_irqsave(&ei->slock, flag);
> +		clear_bit(HTE_TS_DISABLE, &ei->flags);
> +	} else {
> +		if (test_bit(HTE_TS_DISABLE, &ei->flags)) {
> +			ret = 0;
> +			goto out_unlock;
> +		}
> +
> +		spin_unlock_irqrestore(&ei->slock, flag);
> +		ret = gdev->chip->ops->disable(gdev->chip, ei->xlated_id);
> +		if (ret) {
> +			dev_warn(gdev->sdev, "id: %d disable failed\n",
> +				 ts_id);
> +			goto out;
> +		}
> +
> +		spin_lock_irqsave(&ei->slock, flag);
> +		set_bit(HTE_TS_DISABLE, &ei->flags);
> +	}
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&ei->slock, flag);
> +out:
> +	mutex_unlock(&ei->req_mlock);
> +	return ret;
> +}
> +
> +/**
> + * hte_disable_ts() - Disable timestamp on given descriptor.
> + *
> + * The API does not release any resources associated with desc.
> + *
> + * @desc: ts descriptor, this is the same as returned by the request API.
> + *
> + * Context: Holds mutex lock, not suitable from atomic context.
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int hte_disable_ts(struct hte_ts_desc *desc)
> +{
> +	return hte_ts_dis_en_common(desc, false);
> +}
> +EXPORT_SYMBOL_GPL(hte_disable_ts);
> +
> +/**
> + * hte_enable_ts() - Enable timestamp on given descriptor.
> + *
> + * @desc: ts descriptor, this is the same as returned by the request API.
> + *
> + * Context: Holds mutex lock, not suitable from atomic context.
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int hte_enable_ts(struct hte_ts_desc *desc)
> +{
> +	return hte_ts_dis_en_common(desc, true);
> +}
> +EXPORT_SYMBOL_GPL(hte_enable_ts);
> +
> +static int hte_simple_xlate(struct hte_chip *gc,
> +			    const struct of_phandle_args *args,
> +			    struct hte_ts_desc *desc,
> +			    u32 *id)
> +{
> +	if (!id || !desc || !gc)
> +		return -EINVAL;
> +
> +	/*
> +	 * For the providers which do not have any internal mappings between
> +	 * logically exposed ids and actual ids, will set both
> +	 * the same.
> +	 *
> +	 * In case there is a internal mapping needed, providers will need to
> +	 * provide its own xlate function where con_id will be sent as
> +	 * args[0] and it will return xlated id. Later xlated id will be
> +	 * used for any future exchanges between provider and subsystems.
> +	 */
> +
> +	if (args) {
> +		if (gc->of_hte_n_cells < 1)
> +			return -EINVAL;
> +
> +		if (args->args_count != gc->of_hte_n_cells)
> +			return -EINVAL;
> +
> +		*id = args->args[0];
> +		desc->con_id = *id;
> +	} else {
> +		*id = desc->con_id;
> +	}
> +
> +	if (desc->con_id > gc->nlines)
> +		return -EINVAL;
> +
> +	desc->hte_data = NULL;
> +
> +	return 0;
> +}
> +
> +static int _hte_wait_for_ts_data(struct hte_ts_info *ei)
> +{
> +	for (;;) {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +
> +		if (kthread_should_stop()) {
> +			if (test_and_clear_bit(HTE_CB_RUN_THREAD,
> +			    &ei->hte_cb_flags)) {
> +				__set_current_state(TASK_RUNNING);
> +				return 0;
> +			}
> +			__set_current_state(TASK_RUNNING);
> +			return -1;
> +		}
> +
> +		if (test_and_clear_bit(HTE_CB_RUN_THREAD,
> +				       &ei->hte_cb_flags)) {
> +			__set_current_state(TASK_RUNNING);
> +			return 0;
> +		}
> +		schedule();
> +	}
> +}
> +
> +static int _hte_threadfn(void *data)
> +{
> +	struct hte_ts_info *ei = data;
> +
> +	while (!_hte_wait_for_ts_data(ei))
> +		ei->tcb(ei->cl_data);
> +
> +	return 0;
> +}
> +
> +static int _hte_setup_thread(struct hte_ts_info *ei, u32 id)
> +{
> +	struct task_struct *t;
> +
> +	t = kthread_create(_hte_threadfn, ei, "hte-%u", id);
> +	if (IS_ERR(t))
> +		return PTR_ERR(t);
> +
> +	ei->thread = get_task_struct(t);
> +
> +	return 0;
> +}
> +
> +static int ___hte_req_ts(struct hte_device *gdev, struct hte_ts_desc *desc,
> +			 u32 xlated_id, hte_ts_cb_t cb,
> +			 hte_ts_threaded_cb_t tcb, void *data)
> +{
> +	struct hte_ts_info *ei;
> +	int ret;
> +	u32 con_id = desc->con_id;
> +
> +	if (!try_module_get(gdev->owner))
> +		return -ENODEV;
> +
> +	ei = &gdev->ei[xlated_id];
> +	ei->xlated_id = xlated_id;
> +
> +	/*
> +	 * There is a chance that multiple consumers requesting same entity,
> +	 * lock here.
> +	 */
> +	mutex_lock(&ei->req_mlock);
> +
> +	if (test_bit(HTE_TS_REGISTERED, &ei->flags)) {
> +		dev_dbg(gdev->chip->dev, "id:%u is already registered",
> +			xlated_id);
> +		ret = -EUSERS;
> +		goto unlock;
> +	}
> +
> +	ei->cb = cb;
> +	ei->tcb = tcb;
> +	if (tcb) {
> +		ret = _hte_setup_thread(ei, xlated_id);
> +		if (ret < 0) {
> +			dev_err(gdev->chip->dev, "setting thread failed\n");
> +			goto unlock;
> +		}
> +	}
> +
> +	ret = gdev->chip->ops->request(gdev->chip, xlated_id);
> +	if (ret < 0) {
> +		dev_err(gdev->chip->dev, "ts request failed\n");
> +		goto unlock;
> +	}
> +
> +	desc->hte_data = ei;
> +	ei->cl_data = data;
> +
> +	atomic_inc(&gdev->ts_req);
> +
> +	ei->hte_name = false;
> +	if (!desc->name) {
> +		desc->name = kzalloc(HTE_TS_NAME_LEN, GFP_KERNEL);
> +		if (desc->name) {
> +			scnprintf(desc->name, HTE_TS_NAME_LEN, "ts_%u",
> +				  con_id);
> +			ei->hte_name = true;
> +		}
> +	}
> +
> +	hte_ts_dbgfs_init(desc->name, ei);
> +	set_bit(HTE_TS_REGISTERED, &ei->flags);
> +
> +	mutex_unlock(&ei->req_mlock);
> +
> +	dev_dbg(gdev->chip->dev, "id: %u, xlated id:%u", con_id, xlated_id);
> +
> +	return 0;
> +
> +unlock:
> +	module_put(gdev->owner);
> +	mutex_unlock(&ei->req_mlock);
> +
> +	return ret;
> +}
> +
> +static struct hte_device *of_node_to_htedevice(struct device_node *np)
> +{
> +	struct hte_device *gdev;
> +
> +	spin_lock(&hte_lock);
> +
> +	list_for_each_entry(gdev, &hte_devices, list)
> +		if (gdev->chip && gdev->chip->dev &&
> +		    gdev->chip->dev->of_node == np) {
> +			spin_unlock(&hte_lock);
> +			return gdev;
> +		}
> +
> +	spin_unlock(&hte_lock);
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static struct hte_device *of_hte_dev_get(struct device *dev,
> +					 struct device_node *np,
> +					 const char *label,
> +					 struct of_phandle_args *args)
> +{
> +	struct hte_device *gdev = NULL;
> +	int index = 0;
> +	int err;
> +
> +	if (label) {
> +		index = of_property_match_string(np,
> +						 "hardware-timestamp-names",
> +						 label);
> +		if (index < 0)
> +			return ERR_PTR(index);
> +	}
> +
> +	err = of_parse_phandle_with_args(np, "hardware-timestamps",
> +					 "#hardware-timestamp-cells", index,
> +					 args);
> +	if (err) {
> +		pr_err("%s(): can't parse \"hardware-timestamps\" property\n",
> +		       __func__);
> +		return ERR_PTR(err);
> +	}
> +
> +	gdev = of_node_to_htedevice(args->np);
> +	if (IS_ERR(gdev)) {
> +		pr_err("%s(): HTE chip not found\n", __func__);
> +		of_node_put(args->np);
> +		return gdev;
> +	}
> +
> +	return gdev;
> +}
> +
> +static int __hte_req_ts(struct device *dev, struct hte_ts_desc *desc,
> +			hte_ts_cb_t cb, hte_ts_threaded_cb_t tcb, void *data)
> +{
> +	struct hte_device *gdev = NULL;
> +	struct of_phandle_args args;
> +	int ret;
> +	u32 xlated_id;
> +
> +	gdev = of_hte_dev_get(dev, dev->of_node, desc->name, &args);
> +	if (IS_ERR(gdev))
> +		return PTR_ERR(gdev);
> +
> +	if (!gdev->chip) {
> +		pr_debug("requested id does not have provider\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = gdev->chip->xlate(gdev->chip, &args, desc, &xlated_id);
> +	if (ret < 0)
> +		goto put;
> +
> +	ret = ___hte_req_ts(gdev, desc, xlated_id, cb, tcb, data);
> +	if (ret < 0)
> +		goto put;
> +
> +	return 0;
> +
> +put:
> +	of_node_put(args.np);
> +
> +	return ret;
> +}
> +
> +static void __devm_hte_release_ts(void *res)
> +{
> +	hte_release_ts(res);
> +}
> +
> +/**
> + * devm_of_hte_request_ts() - Resource managed API to request the HTE facility
> + * on the specified entity, where entity is provider specific for example,
> + * GPIO lines, signals, buses etc...
> + *
> + * The API allocates necessary resources and enables the timestamp. So calling
> + * hte_enable_ts is not needed. The consumer does not need to call
> + * hte_release_ts since it will be called upon consumer exit.
> + *
> + * @dev: HTE consumer/client device.
> + * @desc: Pre-allocated timestamp descriptor. HTE core will fill out necessary
> + * details. Optionally the consumer can set name field of desc, if not
> + * specified HTE core will set it as ts_con_id. It will be the consumer's
> + * job to free any allocation related to this structure as well name field
> + * in case it has set that field.
> + * @cb: Callback to push the timestamp data to consumer.
> + * @tcb: Optional callback. If its provided, subsystem will create
> + * thread. This will be called when cb returns HTE_RUN_THREADED_CB.
> + * @data: Client data, will be sent back during cb and tcb callbacks.
> + *
> + * Context: Holds mutex lock.
> + * Returns: Returns 0 on success or negative error code on failure.
> + */
> +int devm_of_hte_request_ts(struct device *dev, struct hte_ts_desc *desc,
> +			   hte_ts_cb_t cb, hte_ts_threaded_cb_t tcb,
> +			   void *data)
> +{
> +	int err;
> +
> +	if (!dev || !dev->of_node || !desc || !cb)
> +		return -EINVAL;
> +
> +	err = __hte_req_ts(dev, desc, cb, tcb, data);
> +	if (err)
> +		return err;
> +
> +	err = devm_add_action_or_reset(dev, __devm_hte_release_ts, desc);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_hte_request_ts);
> +
> +/**
> + * hte_req_ts_by_hte_name() - Request entity to timestamp realtime by passing
> + * property name that contains HTE provider phandle, meaning of the entity
> + * is HTE provider specific, for example lines, signals, GPIOs, buses etc...
> + *
> + * This API is designed to address below uses cases:
> + *
> + * 1) For the consumer device which acts as a central device for secondary
> + * consumers. For example, GPIO controller driver acts as a primary consumer
> + * on behalf of in kernel and userspace GPIO HTE consumers. The GPIO controller
> + * driver specifies HTE provider that it supports/wants and it becomes opaque
> + * for the secondary consumers requesting GPIO and hardware timestamp through
> + * that GPIO controller.
> + *
> + * 2) For the providers which are dependent on other hardware modules. In that
> + * case it forces consumers to go through other subsystem or driver making them
> + * secondary consumers. Same example as above applies here as well.
> + *
> + * The API allocates necessary resources and enables the timestamp. So calling
> + * hte_enable_ts is not needed.
> + *
> + * @dev: HTE consumer/client device.
> + * @propname: Name of property holding a HTE provider phandle value
> + * @desc: Pre-allocated timestamp descriptor with con_id set by the consumer.
> + * HTE core will fill out the rest. Optionally the consumer can set name
> + * field of desc, if not specified HTE core will set it as ts_con_id. It will
> + * be the consumer's job to free any allocation related to this structure as
> + * well name field in case it has set that field.
> + * @cb: Callback to push the timestamp data to consumer.
> + * @tcb: Optional callback. If its provided, subsystem will create
> + * thread. This will be called when cb returns HTE_RUN_THREADED_CB.
> + * @data: Client data, will be sent back during cb and tcb callbacks.
> + *
> + * Context: Holds mutex lock, can not be called from atomic context. The mutex
> + * lock is used to serialize multiple consumers.
> + * Returns: returns 0 on success or negative error code on failure.
> + */
> +int hte_req_ts_by_hte_name(struct device *dev, const char *propname,
> +			   struct hte_ts_desc *desc, hte_ts_cb_t cb,
> +			   hte_ts_threaded_cb_t tcb, void *data)
> +{
> +	struct hte_device *gdev;
> +	struct device_node *np = NULL;
> +	int ret;
> +	u32 xlated_id;
> +
> +	if (!dev->of_node || !propname || !desc)
> +		return -EINVAL;
> +
> +	np = of_parse_phandle(dev->of_node, propname, 0);
> +	if (!np)
> +		return -ENODEV;
> +
> +	of_node_put(np);
> +
> +	gdev = of_node_to_htedevice(np);
> +	if (IS_ERR(gdev))
> +		return -ENOTSUPP;
> +
> +	if (!gdev->chip || !gdev->chip->ops)
> +		return -ENOTSUPP;
> +
> +	ret = gdev->chip->xlate(gdev->chip, NULL, desc, &xlated_id);
> +	if (ret < 0) {
> +		dev_err(gdev->chip->dev,
> +			"failed to xlate id: %d\n", desc->con_id);
> +		return ret;
> +	}
> +
> +	ret = ___hte_req_ts(gdev, desc, xlated_id, cb, tcb, data);
> +	if (ret < 0) {
> +		dev_err(gdev->chip->dev,
> +			"failed to request id: %d\n", desc->con_id);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(hte_req_ts_by_hte_name);
> +
> +/**
> + * hte_get_clk_src_info() - Consumer calls this API to query clock source
> + * information of the desc.
> + *

                               ^^^^^^^^^^
Get the clock source information for a ts descriptor.

> + * @desc: ts descriptor, same as returned from request API.
> + * @ci: The API fills this structure with the clock information data.
> + *
> + * Context: Any context.
> + * Returns: 0 on success else negative error code on failure.
> + */
> +int hte_get_clk_src_info(const struct hte_ts_desc *desc,
> +			 struct hte_clk_info *ci)
> +{
> +	struct hte_chip *chip;
> +	struct hte_ts_info *ei;
> +
> +	if (!desc || !desc->hte_data || !ci) {
> +		pr_debug("%s:%d\n", __func__, __LINE__);
> +		return -EINVAL;
> +	}
> +
> +	ei = desc->hte_data;
> +	if (!ei || !ei->gdev || !ei->gdev->chip)
> +		return -EINVAL;
> +
> +	chip = ei->gdev->chip;
> +	if (!chip->ops->get_clk_src_info)
> +		return -ENOTSUPP;
> +
> +	return chip->ops->get_clk_src_info(chip, ci);
> +}
> +EXPORT_SYMBOL_GPL(hte_get_clk_src_info);
> +
> +/**
> + * hte_push_ts_ns() - Used by the provider to push timestamp in nano
> + * seconds i.e data->tsc will be in ns.
> + *
                         ^^^^^^^^
Push timestamp in nanoseconds.

> + * @chip: The HTE chip, used during the registration.
> + * @xlated_id: entity id understood by both subsystem and provider, usually this
> + * is obtained from xlate callback during request API.
> + * @data: timestamp data.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int hte_push_ts_ns(const struct hte_chip *chip, u32 xlated_id,
> +		   struct hte_ts_data *data)
> +{
> +	hte_return_t ret;
> +	int st = 0;
> +	struct hte_ts_info *ei;
> +	unsigned long flag;
> +
> +	if (!chip || !data || !chip->gdev)
> +		return -EINVAL;
> +
> +	if (xlated_id > chip->nlines)
> +		return -EINVAL;
> +
> +	ei = &chip->gdev->ei[xlated_id];
> +
> +	spin_lock_irqsave(&ei->slock, flag);
> +
> +	/* timestamp sequence counter */
> +	data->seq = ei->seq++;
> +
> +	if (!test_bit(HTE_TS_REGISTERED, &ei->flags) ||
> +	    test_bit(HTE_TS_DISABLE, &ei->flags)) {
> +		dev_dbg(chip->dev, "Unknown timestamp push\n");
> +		st = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	ret = ei->cb(data, ei->cl_data);
> +	if (ret == HTE_RUN_THREADED_CB && ei->thread) {
> +		if (test_and_set_bit(HTE_CB_RUN_THREAD, &ei->hte_cb_flags))
> +			goto unlock;
> +		else
> +			wake_up_process(ei->thread);
> +	} else if (ret == HTE_CB_TS_DROPPED) {
> +		atomic_inc(&ei->dropped_ts);
> +	} else if (ret == HTE_CB_ERROR) {
> +		dev_dbg(chip->dev, "cb error\n");
> +	}
> +
> +unlock:
> +	spin_unlock_irqrestore(&ei->slock, flag);
> +
> +	return st;
> +}
> +EXPORT_SYMBOL_GPL(hte_push_ts_ns);
> +
> +static int hte_register_chip(struct hte_chip *chip)
> +{
> +	struct hte_device *gdev;
> +	u32 i;
> +
> +	if (!chip || !chip->dev || !chip->dev->of_node)
> +		return -EINVAL;
> +
> +	if (!chip->ops || !chip->ops->request || !chip->ops->release) {
> +		dev_err(chip->dev, "Driver needs to provide ops\n");
> +		return -EINVAL;
> +	}
> +
> +	gdev = kzalloc(struct_size(gdev, ei, chip->nlines), GFP_KERNEL);
> +	if (!gdev)
> +		return -ENOMEM;
> +
> +	gdev->chip = chip;
> +	chip->gdev = gdev;
> +	gdev->nlines = chip->nlines;
> +	gdev->sdev = chip->dev;
> +
> +	for (i = 0; i < chip->nlines; i++) {
> +		gdev->ei[i].gdev = gdev;
> +		mutex_init(&gdev->ei[i].req_mlock);
> +		spin_lock_init(&gdev->ei[i].slock);
> +	}
> +
> +	if (chip->dev->driver)
> +		gdev->owner = chip->dev->driver->owner;
> +	else
> +		gdev->owner = THIS_MODULE;
> +
> +	if (!chip->xlate) {
> +		chip->xlate = hte_simple_xlate;
> +		/* Just a id number to monitor */
> +		chip->of_hte_n_cells = 1;
> +	}
> +
> +	of_node_get(chip->dev->of_node);
> +
> +	INIT_LIST_HEAD(&gdev->list);
> +
> +	spin_lock(&hte_lock);
> +	list_add_tail(&gdev->list, &hte_devices);
> +	spin_unlock(&hte_lock);
> +
> +	hte_chip_dbgfs_init(gdev);
> +
> +	dev_dbg(chip->dev, "Added hte chip\n");
> +
> +	return 0;
> +}
> +
> +/**
> + * hte_unregister_chip() - Used by the provider to remove a HTE chip.
> + * @chip: the HTE chip to remove.
> + *
> + * Context: Can not be called from atomic context.
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +static int hte_unregister_chip(struct hte_chip *chip)
> +{
> +	struct hte_device *gdev;
> +
> +	if (!chip)
> +		return -EINVAL;
> +
> +	gdev = chip->gdev;
> +
> +	spin_lock(&hte_lock);
> +	list_del(&gdev->list);
> +	spin_unlock(&hte_lock);
> +
> +	gdev->chip = NULL;
> +
> +	of_node_put(chip->dev->of_node);
> +	debugfs_remove_recursive(gdev->dbg_root);
> +	kfree(gdev);
> +
> +	dev_dbg(chip->dev, "Removed hte chip\n");
> +
> +	return 0;
> +}
> +
> +static void _hte_devm_unregister_chip(void *chip)
> +{
> +	hte_unregister_chip(chip);
> +}
> +
> +/**
> + * devm_hte_register_chip() - Used by provider to register a HTE chip.
> + * @chip: the HTE chip to add to subsystem.
> + *
> + * The API is resource managed and  _hte_devm_unregister_chip will be called
> + * automatically when the provider exits.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int devm_hte_register_chip(struct hte_chip *chip)
> +{
> +	int err;
> +
> +	err = hte_register_chip(chip);
> +	if (err)
> +		return err;
> +
> +	err = devm_add_action_or_reset(chip->dev, _hte_devm_unregister_chip,
> +				       chip);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_hte_register_chip);
> diff --git a/include/linux/hte.h b/include/linux/hte.h
> new file mode 100644
> index 000000000000..f4dd5415a493
> --- /dev/null
> +++ b/include/linux/hte.h
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021 NVIDIA Corporation
> + *
> + * Author: Dipen Patel <dipenp@nvidia.com>
> + */
> +
> +#ifndef __LINUX_HTE_H
> +#define __LINUX_HTE_H
> +
> +#include <linux/errno.h>
> +
> +struct hte_chip;
> +struct hte_device;
> +struct of_phandle_args;
> +struct device_node;
> +
> +/**
> + * enum hte_dir - HTE edge timestamp direction.
> + *
> + * @HTE_RISING_EDGE_TS: Timestamps is for rising edge.
> + * @HTE_FALLING_EDGE_TS: Timestamps is for falling edge.
> + * @HTE_DIR_NOSUPP: Direction is not supported.
> + */
> +enum hte_dir {
> +	HTE_RISING_EDGE_TS,
> +	HTE_FALLING_EDGE_TS,
> +	HTE_DIR_NOSUPP,
> +};
> +

Your tegra194 driver doesn't support direction - it always reports
HTE_DIR_NOSUPP.  Do you have an example of hardware that does support
direction?

This might be better as a raw line level rather than an edge direction.
See my related comments on patch 09.

And the third option should be "unknown", rather than "not supported" as
this is a reading being returned, not a feature being requested.
It may be unknown because the feature is not supported, but that is a
separate issue.

> +/**
> + * struct hte_ts_data - HTE timestamp data.
> + * The provider uses and fills timestamp related details during push_timestamp
> + * API call. The consumer uses during retrieve_timestamp API call.
> + *

What push_timestamp() and retrieve_timestamp() calls?

General comment:
Documenting how a struct is used in an API, rather than what it is for or
what it represents, is a maintenance nightmare.  If it is subsequently
used elsewhere or names are changed then the user is now obliged to update
your documentation.
And they will forget to, or overlook it, as you yourself have done here.

Descriptions of what the API calls do should be in the documentation of
the API calls themselves, not the structs they use.

> + * @tsc: Timestamp value.
> + * @seq: Sequence counter of the timestamps.
> + * @dir: Direction of the event at the time of timestamp.
> + */
> +struct hte_ts_data {
> +	u64 tsc;
> +	u64 seq;
> +	enum hte_dir dir;
> +};
> +
> +/**
> + * struct hte_clk_info - Clock source info that HTE provider uses to timestamp
> + * The provider uses hardware clock as a source to timestamp real time. This
> + * structure presents the clock information to consumers during
> + * hte_get_clk_src_info call.
> + *
> + * @hz: Supported clock rate in HZ, for example 1KHz clock = 1000.
> + * @type: Supported clock type. CLOCK_* types.
> + */
> +struct hte_clk_info {
> +	u64 hz;
> +	clockid_t type;
> +};
> +
> +/**
> + * enum hte_return- HTE subsystem return values used during callback.
> + *
> + * @HTE_CB_HANDLED: The consumer handled the data successfully.
> + * @HTE_RUN_THREADED_CB: The consumer needs further processing, in that case HTE
> + * subsystem will invoke kernel thread and call secondary callback provided by
> + * the consumer during devm_of_hte_request_ts and hte_req_ts_by_dt_node call.
> + * @HTE_CB_TS_DROPPED: The client returns when it can not store ts data.
> + * @HTE_CB_ERROR: The client returns error if anything goes wrong.
> + */
> +enum hte_return {
> +	HTE_CB_HANDLED,
> +	HTE_RUN_THREADED_CB,
> +	HTE_CB_TS_DROPPED,
> +	HTE_CB_ERROR,
> +};
> +typedef enum hte_return hte_return_t;
> +

Wrt HTE_CB_TS_DROPPED, why is the client dropping data any of hte's
business?  It is also confusing in that I would expect the dropped_ts
gauge, that you increment when this code is returned, to indicate the
events dropped by the hardware, not the client.  But then you have no
indication of events dropped by hardware at all, though you could
determine that from gaps in the sequence numbers.
Anyway, the client can do the math in both cases if they care to, so not
sure what its purpose is here.

Similarly HTE_CB_ERROR, what can hte do here other than log it - and the
client can do that themselves if they have the need.

> +/**
> + * typedef hte_ts_cb_t - Callback provided during devm_of_hte_request_ts and
> + * hte_req_ts_by_dt_node APIs call.
> + *
> + * The callback is used to push timestamp data to client.
> + * @ts: HW timestamp data.
> + * @data: Client supplied data.
> + */
> +typedef hte_return_t (*hte_ts_cb_t)(struct hte_ts_data *ts, void *data);
> +
> +/**
> + * typedef hte_ts_threaded_cb_t - Threaded callback provided during
> + * devm_of_hte_request_ts and hte_req_ts_by_dt_node APIs call.
> + *
> + * @data: Client supplied data.
> + *
> + * It will be called when client return HTE_RUN_THREADED_CB from hte_ts_cb_t.
> + * The callback will be called from thread context.
> + *
> + */
> +typedef hte_return_t (*hte_ts_threaded_cb_t)(void *data);
> +
> +/**
> + * struct hte_ts_desc - HTE timestamp descriptor, this structure will be
> + * communication token between consumers to subsystem and subsystem to
> + * providers.
> + *
> + * @con_id: This is the same id sent in request APIs.
> + * @name: Descriptive name of the entity that is being monitored for the
> + * realtime timestamping. The consumer can set any name it likes. If null
> + * HTE core will construct name as ts_con_id. It will be the consumer's
> + * job to free any allocation if name is set by the consumer.
> + * @hte_data: Subsystem's private data relate to requested con_id.
> + */
> +struct hte_ts_desc {
> +	u32 con_id;
> +	char *name;
> +	void *hte_data;
> +};
> +
> +/**
> + * struct hte_ops - HTE operations set by providers.
> + *
> + * @request: Hook for requesting a HTE timestamp. Returns 0 on success,
> + * non-zero for failures.
> + * @release: Hook for releasing a HTE timestamp. Returns 0 on success,
> + * non-zero for failures.
> + * @enable: Hook to enable the specified timestamp. Returns 0 on success,
> + * non-zero for failures.
> + * @disable: Hook to disable specified timestamp. Returns 0 on success,
> + * non-zero for failures.
> + * @get_clk_src_info: Hook to get the clock information the provider uses
> + * to timestamp. Returns 0 for success and negative error code for failure. On
> + * success HTE subsystem fills up provided struct hte_clk_info.
> + *
> + * xlated_id parameter is used to communicate between HTE subsystem and the
> + * providers. It is the same id returned during xlate API call and translated
> + * by the provider. This may be helpful as both subsystem and provider locate
> + * the requested entity in constant time, where entity could be anything from
> + * lines, signals, events, buses etc.. that providers support.
> + */
> +struct hte_ops {
> +	int (*request)(struct hte_chip *chip, u32 xlated_id);
> +	int (*release)(struct hte_chip *chip, u32 xlated_id);
> +	int (*enable)(struct hte_chip *chip, u32 xlated_id);
> +	int (*disable)(struct hte_chip *chip, u32 xlated_id);
> +	int (*get_clk_src_info)(struct hte_chip *chip,
> +				struct hte_clk_info *ci);
> +};
> +
> +/**
> + * struct hte_chip - Abstract HTE chip structure.
> + * @name: functional name of the HTE IP block.
> + * @dev: device providing the HTE.
> + * @ops: callbacks for this HTE.
> + * @nlines: number of lines/signals supported by this chip.
> + * @xlate: Callback which translates consumer supplied logical ids to
> + * physical ids, return from 0 for the success and negative for the
> + * failures. It stores (0 to @nlines) in xlated_id parameter for the success.
> + * @of_hte_n_cells: Number of cells used to form the HTE specifier.
> + * @gdev: HTE subsystem abstract device, internal to the HTE subsystem.
> + * @data: chip specific private data.
> + */
> +struct hte_chip {
> +	const char *name;
> +	struct device *dev;
> +	const struct hte_ops *ops;
> +	u32 nlines;
> +	int (*xlate)(struct hte_chip *gc,
> +		     const struct of_phandle_args *args,
> +		     struct hte_ts_desc *desc, u32 *xlated_id);
> +	u8 of_hte_n_cells;
> +
> +	struct hte_device *gdev;
> +	void *data;
> +};
> +
> +#if IS_ENABLED(CONFIG_HTE)
> +/* HTE APIs for the providers */
> +int devm_hte_register_chip(struct hte_chip *chip);
> +int hte_push_ts_ns(const struct hte_chip *chip, u32 xlated_id,
> +		   struct hte_ts_data *data);
> +
> +/* HTE APIs for the consumers */
> +
> +int hte_release_ts(struct hte_ts_desc *desc);
> +int devm_of_hte_request_ts(struct device *dev, struct hte_ts_desc *desc,
> +			   hte_ts_cb_t cb, hte_ts_threaded_cb_t tcb,
> +			   void *data);
> +int hte_req_ts_by_hte_name(struct device *dev, const char *propname,
> +			   struct hte_ts_desc *desc, hte_ts_cb_t cb,
> +			   hte_ts_threaded_cb_t tcb, void *data);
> +int hte_enable_ts(struct hte_ts_desc *desc);
> +int hte_disable_ts(struct hte_ts_desc *desc);
> +int hte_get_clk_src_info(const struct hte_ts_desc *desc,
> +			 struct hte_clk_info *ci);
> +
> +#else /* !CONFIG_HTE */
> +static inline int devm_hte_register_chip(struct hte_chip *chip)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline int hte_push_ts_ns(const struct hte_chip *chip,
> +				 u32 xlated_id,
> +				 const struct hte_ts_data *data)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline int hte_release_ts(struct hte_ts_desc *desc)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static inline int devm_of_hte_request_ts(struct device *dev,
> +					 struct hte_ts_desc *desc,
> +					 hte_ts_cb_t cb,
> +					 hte_ts_threaded_cb_t threaded_cb,
> +					 void *data)
> +{
> +	return -ENOTSUPP;
> +}
> +

Rename threaded_cb to tcb to be consistent with other functions.

> +static inline int hte_req_ts_by_hte_name(struct device *dev,
> +					 const char *propname,
> +					 struct hte_ts_desc *desc,
> +					 hte_ts_cb_t cb,
> +					 hte_ts_threaded_cb_t tcb, void *data)
> +{
> +	return -ENOTSUPP;
> +}
> +

Make line wrapping of hte_req_ts_by_hte_name() consistent with
devm_of_hte_request_ts().

Cheers,
Kent.
Rob Herring Nov. 30, 2021, 10:32 p.m. UTC | #2
On Tue, Nov 23, 2021 at 11:30:34AM -0800, Dipen Patel wrote:
> Some GPIO controllers can timestamp GPIO lines in real time using
> hardware timestamp engine. The optional property is added to facilitate
> this feature.
> 
> The nvidia GPIO controller has hardware timestamp engine as a backend
> to timestamp its GPIO lines in a real time. This change set adds
> hardware-timestamp-engine property to reflect that fact.
> 
> Signed-off-by: Dipen Patel <dipenp@nvidia.com>
> ---
>  Documentation/devicetree/bindings/gpio/gpio.txt           | 8 ++++++++
>  .../devicetree/bindings/gpio/nvidia,tegra186-gpio.txt     | 7 +++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> index a8895d339bfe..c6ae2eaf5762 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -167,6 +167,13 @@ left to right from the passed array. An incomplete array (where the number
>  of passed named are less than ngpios) will still be used up until the last
>  provided valid line index.
>  
> +Optionally, a GPIO controller may have a "hardware-timestamp-engine" property.
> +This specifies the timestamp engine node. The property is only useful if the
> +hardware timestamp engine (HTE) serves as a back-end to support hardware
> +timestamp GPIO lines in a real time and GPIO controller has dependency on the
> +engine. It will be up to GPIO controller and HTE provider drivers to figure out
> +the mapping between GPIO controller and HTE namespaces of a given GPIO line.

This needs to be added to schema instead (in dtschema).

I find the 'hardware-' part to be redundant. What other type of 
timestamp engine are we going to have in the h/w description?

> +
>  Example:
>  
>  gpio-controller@00000000 {
> @@ -180,6 +187,7 @@ gpio-controller@00000000 {
>  		"LED G", "LED B", "Col A", "Col B", "Col C", "Col D",
>  		"Row A", "Row B", "Row C", "Row D", "NMI button",
>  		"poweroff", "reset";
> +	hardware-timestamp-engine = <&hadrware_ts_engine_node>;
>  }
>  
>  The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> diff --git a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> index adff16c71d21..20f6c9e69839 100644
> --- a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> @@ -127,6 +127,12 @@ Required properties:
>              - 8: Active low level-sensitive.
>              Valid combinations are 1, 2, 3, 4, 8.
>  
> +Optional properties:
> +- hardware-timestamp-engine
> +    AON GPIO controller has timestamp engine which can hardware timestamp
> +    GPIO configured as input and IRQ. This property specifies hardware
> +    timestamp engine (HTE) device-tree node.
> +
>  Example:
>  
>  #include <dt-bindings/interrupt-controller/irq.h>
> @@ -162,4 +168,5 @@ gpio@c2f0000 {
>  	#gpio-cells = <2>;
>  	interrupt-controller;
>  	#interrupt-cells = <2>;
> +	hardware-timestamp-engine = <&tegra_hte_aon>;
>  };
> -- 
> 2.17.1
> 
>
Dipen Patel Dec. 8, 2021, 1:59 a.m. UTC | #3
On 12/7/21 5:21 PM, Kent Gibson wrote:
> On Tue, Dec 07, 2021 at 04:36:35PM -0800, Dipen Patel wrote:
>> Hi,
>>
> [snip]
>
>>>> +/**
>>>> + * enum hte_return- HTE subsystem return values used during callback.
>>>> + *
>>>> + * @HTE_CB_HANDLED: The consumer handled the data successfully.
>>>> + * @HTE_RUN_THREADED_CB: The consumer needs further processing, in that case HTE
>>>> + * subsystem will invoke kernel thread and call secondary callback provided by
>>>> + * the consumer during devm_of_hte_request_ts and hte_req_ts_by_dt_node call.
>>>> + * @HTE_CB_TS_DROPPED: The client returns when it can not store ts data.
>>>> + * @HTE_CB_ERROR: The client returns error if anything goes wrong.
>>>> + */
>>>> +enum hte_return {
>>>> +	HTE_CB_HANDLED,
>>>> +	HTE_RUN_THREADED_CB,
>>>> +	HTE_CB_TS_DROPPED,
>>>> +	HTE_CB_ERROR,
>>>> +};
>>>> +typedef enum hte_return hte_return_t;
>>>> +
>>> Wrt HTE_CB_TS_DROPPED, why is the client dropping data any of hte's
>>> business?  It is also confusing in that I would expect the dropped_ts
>>> gauge, that you increment when this code is returned, to indicate the
>>> events dropped by the hardware, not the client.  But then you have no
>>> indication of events dropped by hardware at all, though you could
>>> determine that from gaps in the sequence numbers.
>>> Anyway, the client can do the math in both cases if they care to, so not
>>> sure what its purpose is here.
>> It is used for statistical purpose and hte being subsytem it can provide
>>
>> standard interface in debugfs (so that clients do not have to) to anyone interested.
>>
>> The dropped_ts could represent total dropped ts by both hardware and
>>
>> client. I can add debugfs interface to break it down further if it helps in statistics.
>>
> Updating stats is not what the return code here is for.
>
> And what if the client discards the event AFTER returning from the
> handler, say in the threaded cb?
>
> If you want stats fedback then provide a function for the client to call
> to update stats, rather than piggy-backing it on the callback return.
I agree, will work that in v4.
> I'm unconvinced that stats are a worthwhile addition, and you certainly
> don't need to bake it into your core api.

Wouldn't it help in debugging i.e. quickly check using this interface

if there are drops for given application setup?

>
> Cheers,
> Kent.