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