[RFC/PATCHv2,2/2] counter: introduce support for Intel QEP Encoder

Commit Message

Felipe Balbi Sept. 19, 2019, 8:03 a.m. UTC

Add support for Intel PSE Quadrature Encoder

Signed-off-by: Felipe Balbi <[e-mail]>
---

Changes since v1:
	- Many more private sysfs files converted over to counter interface


How do you want me to model this device's Capture Compare Mode (see
below)?

For the few features which I couldn't find a matching property in
counter framework, I still leave them as private sysfs files so we can
discuss how to model them in the framework.

Do you want Capture Compare to be a new function mode?

BTW, I know I'm missing a Documentation file documenting sysfs files
introduced by this driver, I'll do that once we agree how to move all
other sysfs files to the framework. No worries.


Details about the controller (do you want this in commit log?):


Controller has 2 modes of operation: QEP and Capture. Both modes are
mutually exclusive. We also have a set of maskable interrupts. Further
details about each mode below.


Quadrature Encoder Mode
=======================

Used to measure rotational speed, direction and angle of rotation of a
motor shaft. Feature list:

	- Quadrature decoder providing counter pulses with x4 count
	  resolution and count direction

	- 32-bit up/down Position Counter for position measurement

	- Two modes of position counter reset:
		> Maximum Count (ceiling) to reset the position counter
		> Index pulse to reset the position counter

	- Watchdog timer functionality for detecting ‘stall’ events

Capture Compare Mode
====================

Used to measure phase input signal Period or Duty Cycle. Feature List:

	- Capture function operates either on phase A or phase B input
	  and can be configured to operate on lo/hi or hi/lo or both hi
	  and lo transitions.

	- Free-running 32-bit counter to be configured to run at greater
          than or equal to 4x input signal frequency

	- Clock post-scaler to derive the counter clock source from the
	  peripheral clock

	- 32B wide FIFO to capture 32-bit timestamps of up to 8
	  transition events

 drivers/counter/Kconfig     |   6 +
 drivers/counter/Makefile    |   1 +
 drivers/counter/intel-qep.c | 862 ++++++++++++++++++++++++++++++++++++
 3 files changed, 869 insertions(+)
 create mode 100644 drivers/counter/intel-qep.c

Comments

William Breathitt Gray Sept. 22, 2019, 11:35 p.m. UTC | #1

On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
> Add support for Intel PSE Quadrature Encoder
> 
> Signed-off-by: Felipe Balbi <[e-mail]>
> ---
> 
> Changes since v1:
> 	- Many more private sysfs files converted over to counter interface
> 
> 
> How do you want me to model this device's Capture Compare Mode (see
> below)?

Hi Felipe,

I'm CCing Fabien and David as they may be interested in the timestamps
discussion. See below for some ideas I have on implementing this.

> For the few features which I couldn't find a matching property in
> counter framework, I still leave them as private sysfs files so we can
> discuss how to model them in the framework.
> 
> Do you want Capture Compare to be a new function mode?
> 
> BTW, I know I'm missing a Documentation file documenting sysfs files
> introduced by this driver, I'll do that once we agree how to move all
> other sysfs files to the framework. No worries.
> 
> 
> Details about the controller (do you want this in commit log?):
> 
> 
> Controller has 2 modes of operation: QEP and Capture. Both modes are
> mutually exclusive. We also have a set of maskable interrupts. Further
> details about each mode below.

I noticed your interrupt handler takes care of a number of different
scenarios. Would you be able to summarize a bit further here the
conditions for this device that cause an interrupt to be fired (watchdog
timeout, FIFO updates, etc.)?

> Quadrature Encoder Mode
> =======================
> 
> Used to measure rotational speed, direction and angle of rotation of a
> motor shaft. Feature list:
> 
> 	- Quadrature decoder providing counter pulses with x4 count
> 	  resolution and count direction
> 
> 	- 32-bit up/down Position Counter for position measurement
> 
> 	- Two modes of position counter reset:
> 		> Maximum Count (ceiling) to reset the position counter
> 		> Index pulse to reset the position counter
> 
> 	- Watchdog timer functionality for detecting ‘stall’ events
> 
> Capture Compare Mode
> ====================
> 
> Used to measure phase input signal Period or Duty Cycle. Feature List:
> 
> 	- Capture function operates either on phase A or phase B input
> 	  and can be configured to operate on lo/hi or hi/lo or both hi
> 	  and lo transitions.
> 
> 	- Free-running 32-bit counter to be configured to run at greater
>           than or equal to 4x input signal frequency

So in "Capture Compare" mode, the counter value is just increasing when
a state condition transition occurs. In that case we won't need a new
function mode to represent this behavior since one already exists:
"increase".

You can add it to your intel_qep_count_functions array like so:

        [INTEL_QEP_ENCODER_MODE_CAPTURE] =
        COUNTER_COUNT_FUNCTION_INCREASE,

The various configurations for this mode are just Synapse action modes.
If you want only Phase A, you would set the action mode for Phase A
("rising edge", "falling edge", or "both edges") and change the action
mode for Phase B to "none"; vice-versa configuration for Phase B instead
of Phase A.

One thing to keep in mind is that action_set will need to maintain valid
configurations -- so if the user tries to set the action mode for Phase
A to something other than "none", you need to automatically set Phase
B's action mode to "none" (and vice-versa).

> 	- Clock post-scaler to derive the counter clock source from the
> 	  peripheral clock

I see you already have a "prescaler" extension in your code. Is this
different from the "post-scaler" you mentioned here?

> 	- 32B wide FIFO to capture 32-bit timestamps of up to 8
> 	  transition events

You can implement this as a Count extension called "timestamps" or
similar. What we can do is have a read on this attribute return the
entire FIFO data buffer as unsigned integers, where each timestamp is
deliminated by a space.

In addition, it may be useful to have another extension called
"timestamps_layout", or something along those lines, that will report
the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.).

Would this design work for your needs?

William Breathitt Gray

>  drivers/counter/Kconfig     |   6 +
>  drivers/counter/Makefile    |   1 +
>  drivers/counter/intel-qep.c | 862 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 869 insertions(+)
>  create mode 100644 drivers/counter/intel-qep.c
> 
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 2967d0a9ff91..f280cd721350 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -59,4 +59,10 @@ config FTM_QUADDEC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ftm-quaddec.
>  
> +config INTEL_QEP
> +	tristate "Intel Quadrature Encoder"
> +	depends on PCI
> +	help
> +	  Support for Intel Quadrature Encoder Devices
> +
>  endif # COUNTER
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 40d35522937d..cf291cfd8cf0 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
>  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
>  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> +obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
> diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
> new file mode 100644
> index 000000000000..8347f9fa8e37
> --- /dev/null
> +++ b/drivers/counter/intel-qep.c
> @@ -0,0 +1,862 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel-qep.c - Intel Quadrature Encoder Driver
> + *
> + * Copyright (C) 2019 Intel Corporation - https://www.intel.com
> + *
> + * Author: Felipe Balbi <[e-mail]>
> + */
> +#include <linux/bitops.h>
> +#include <linux/counter.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/sysfs.h>
> +
> +#define INTEL_QEPCON		0x00
> +#define INTEL_QEPFLT		0x04
> +#define INTEL_QEPCOUNT		0x08
> +#define INTEL_QEPMAX		0x0c
> +#define INTEL_QEPWDT		0x10
> +#define INTEL_QEPCAPDIV		0x14
> +#define INTEL_QEPCNTR		0x18
> +#define INTEL_QEPCAPBUF		0x1c
> +#define INTEL_QEPINT_STAT	0x20
> +#define INTEL_QEPINT_MASK	0x24
> +
> +/* QEPCON */
> +#define INTEL_QEPCON_EN		BIT(0)
> +#define INTEL_QEPCON_FLT_EN	BIT(1)
> +#define INTEL_QEPCON_EDGE_A	BIT(2)
> +#define INTEL_QEPCON_EDGE_B	BIT(3)
> +#define INTEL_QEPCON_EDGE_INDX	BIT(4)
> +#define INTEL_QEPCON_SWPAB	BIT(5)
> +#define INTEL_QEPCON_OP_MODE	BIT(6)
> +#define INTEL_QEPCON_PH_ERR	BIT(7)
> +#define INTEL_QEPCON_COUNT_RST_MODE BIT(8)
> +#define INTEL_QEPCON_INDX_GATING_MASK GENMASK(10, 9)
> +#define INTEL_QEPCON_INDX_GATING(n) (((n) & 3) << 9)
> +#define INTEL_QEPCON_INDX_PAL_PBL INTEL_QEPCON_INDX_GATING(0)
> +#define INTEL_QEPCON_INDX_PAL_PBH INTEL_QEPCON_INDX_GATING(1)
> +#define INTEL_QEPCON_INDX_PAH_PBL INTEL_QEPCON_INDX_GATING(2)
> +#define INTEL_QEPCON_INDX_PAH_PBH INTEL_QEPCON_INDX_GATING(3)
> +#define INTEL_QEPCON_CAP_MODE	BIT(11)
> +#define INTEL_QEPCON_FIFO_THRE_MASK GENMASK(14, 12)
> +#define INTEL_QEPCON_FIFO_THRE(n) ((((n) - 1) & 7) << 12)
> +#define INTEL_QEPCON_FIFO_EMPTY	BIT(15)
> +
> +/* QEPFLT */
> +#define INTEL_QEPFLT_MAX_COUNT(n) ((n) & 0x1fffff)
> +
> +/* QEPINT */
> +#define INTEL_QEPINT_FIFOCRIT	BIT(5)
> +#define INTEL_QEPINT_FIFOENTRY	BIT(4)
> +#define INTEL_QEPINT_QEPDIR	BIT(3)
> +#define INTEL_QEPINT_QEPRST_UP	BIT(2)
> +#define INTEL_QEPINT_QEPRST_DOWN BIT(1)
> +#define INTEL_QEPINT_WDT	BIT(0)
> +
> +#define INTEL_QEP_DIRECTION_FORWARD 1
> +#define INTEL_QEP_DIRECTION_BACKWARD !INTEL_QEP_DIRECTION_FORWARD
> +
> +#define INTEL_QEP_COUNTER_EXT_RW(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +	.write = _name##_write, \
> +}
> +
> +#define INTEL_QEP_COUNTER_EXT_RO(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +}
> +
> +#define INTEL_QEP_COUNTER_COUNT_EXT_RW(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +	.write = _name##_write, \
> +}
> +
> +#define INTEL_QEP_COUNTER_COUNT_EXT_RO(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +}
> +
> +struct intel_qep {
> +	struct counter_device counter;
> +	struct mutex lock;
> +	struct pci_dev *pci;
> +	struct device *dev;
> +	void __iomem *regs;
> +	u32 interrupt;
> +	int direction;
> +	bool enabled;
> +};
> +
> +#define counter_to_qep(c)	(container_of((c), struct intel_qep, counter))
> +
> +static inline u32 intel_qep_readl(void __iomem *base, u32 offset)
> +{
> +	return readl(base + offset);
> +}
> +
> +static inline void intel_qep_writel(void __iomem *base, u32 offset, u32 value)
> +{
> +	writel(value, base + offset);
> +}
> +
> +static ssize_t phase_error_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			reg & INTEL_QEPCON_PH_ERR ? "error" : "okay");
> +}
> +static DEVICE_ATTR_RO(phase_error);
> +
> +static ssize_t fifo_empty_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			reg & INTEL_QEPCON_FIFO_EMPTY ? "empty" : "not empty");
> +}
> +static DEVICE_ATTR_RO(fifo_empty);
> +
> +static ssize_t operating_mode_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			reg & INTEL_QEPCON_OP_MODE ? "capture" : "quadrature");
> +}
> +
> +static ssize_t operating_mode_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	if (qep->enabled)
> +		return -EINVAL;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (sysfs_streq(buf, "capture"))
> +		reg |= INTEL_QEPCON_OP_MODE;
> +	else if (sysfs_streq(buf, "quadrature"))
> +		reg &= ~INTEL_QEPCON_OP_MODE;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(operating_mode);
> +
> +static ssize_t capture_mode_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			reg & INTEL_QEPCON_CAP_MODE ? "both" : "single");
> +}
> +
> +static ssize_t capture_mode_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	if (qep->enabled)
> +		return -EINVAL;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (sysfs_streq(buf, "both"))
> +		reg |= INTEL_QEPCON_CAP_MODE;
> +	else if (sysfs_streq(buf, "single"))
> +		reg &= ~INTEL_QEPCON_CAP_MODE;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(capture_mode);
> +
> +static const struct attribute *intel_qep_attrs[] = {
> +	&dev_attr_capture_mode.attr,
> +	&dev_attr_fifo_empty.attr,
> +	&dev_attr_operating_mode.attr,
> +	&dev_attr_phase_error.attr,
> +	NULL	/* Terminating Entry */
> +};
> +
> +static ssize_t capture_data_read(struct file *filp, struct kobject *kobj,
> +		struct bin_attribute *attr, char *buf,
> +		loff_t offset, size_t count)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +	int i;
> +
> +	mutex_lock(&qep->lock);
> +	for (i = 0; i < count; i += 4) {
> +		reg = intel_qep_readl(qep->regs, INTEL_QEPCAPBUF);
> +
> +		buf[i + 0] = reg & 0xff;
> +		buf[i + 1] = (reg >> 8) & 0xff;
> +		buf[i + 2] = (reg >> 16) & 0xff;
> +		buf[i + 3] = (reg >> 24) & 0xff;
> +	}
> +	mutex_unlock(&qep->lock);
> +
> +	return count;
> +}
> +
> +static BIN_ATTR_RO(capture_data, 4);
> +
> +static struct bin_attribute *intel_qep_bin_attrs[] = {
> +	&bin_attr_capture_data,
> +	NULL	/* Terminating Entry */
> +};
> +
> +static const struct attribute_group intel_qep_device_aattr_group = {
> +	.name = "qep",
> +	.attrs = (struct attribute **) intel_qep_attrs,
> +	.bin_attrs = intel_qep_bin_attrs,
> +};
> +
> +static const struct pci_device_id intel_qep_id_table[] = {
> +	/* EHL */
> +	{ PCI_VDEVICE(INTEL, 0x4bc3), },
> +	{ PCI_VDEVICE(INTEL, 0x4b81), },
> +	{ PCI_VDEVICE(INTEL, 0x4b82), },
> +	{ PCI_VDEVICE(INTEL, 0x4b83), },
> +	{  } /* Terminating Entry */
> +};
> +MODULE_DEVICE_TABLE(pci, intel_qep_id_table);
> +
> +static void intel_qep_init(struct intel_qep *qep, bool reset)
> +{
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	reg &= ~INTEL_QEPCON_EN;
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	/* make sure periperal is disabled by reading one more time */
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (reset) {
> +		reg &= ~(INTEL_QEPCON_OP_MODE | INTEL_QEPCON_FLT_EN);
> +		reg |= INTEL_QEPCON_EDGE_A | INTEL_QEPCON_EDGE_B |
> +			INTEL_QEPCON_EDGE_INDX | INTEL_QEPCON_COUNT_RST_MODE;
> +	}
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPWDT, 0x1000);
> +	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x0);
> +
> +	qep->direction = INTEL_QEP_DIRECTION_FORWARD;
> +}
> +
> +static irqreturn_t intel_qep_irq_thread(int irq, void *_qep)
> +{
> +	struct intel_qep	*qep = _qep;
> +	u32			stat;
> +
> +	mutex_lock(&qep->lock);
> +
> +	stat = qep->interrupt;
> +	if (stat & INTEL_QEPINT_FIFOCRIT)
> +		sysfs_notify(&qep->dev->kobj, "qep", "capture_buffer");
> +
> +	if (stat & INTEL_QEPINT_FIFOENTRY)
> +		sysfs_notify(&qep->dev->kobj, "qep", "capture_buffer");
> +
> +	if (stat & INTEL_QEPINT_QEPDIR)
> +		qep->direction = !qep->direction;
> +
> +	if (stat & INTEL_QEPINT_QEPRST_UP)
> +		qep->direction = INTEL_QEP_DIRECTION_FORWARD;
> +
> +	if (stat & INTEL_QEPINT_QEPRST_DOWN)
> +		qep->direction = INTEL_QEP_DIRECTION_BACKWARD;
> +
> +	if (stat & INTEL_QEPINT_WDT)
> +		dev_dbg(qep->dev, "Watchdog\n");
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x00);
> +	mutex_unlock(&qep->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t intel_qep_irq(int irq, void *_qep)
> +{
> +	struct intel_qep	*qep = _qep;
> +	u32			stat;
> +
> +	stat = intel_qep_readl(qep->regs, INTEL_QEPINT_STAT);
> +	if (stat) {
> +		qep->interrupt = stat;
> +		intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0xffffffff);
> +		intel_qep_writel(qep->regs, INTEL_QEPINT_STAT, stat);
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +enum intel_qep_synapse_action {
> +	INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE,
> +	INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE,
> +};
> +
> +static enum counter_synapse_action intel_qep_synapse_actions[] = {
> +	[INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE] =
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> +	
> +	[INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE] =
> +	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> +};
> +
> +enum intel_qep_count_function {
> +	INTEL_QEP_ENCODER_MODE_NORMAL,
> +	INTEL_QEP_ENCODER_MODE_SWAPPED,
> +};
> +
> +static const enum counter_count_function intel_qep_count_functions[] = {
> +	[INTEL_QEP_ENCODER_MODE_NORMAL] =
> +	COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
> +
> +	[INTEL_QEP_ENCODER_MODE_SWAPPED] =
> +	COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED,
> +};
> +
> +static int intel_qep_count_read(struct counter_device *counter,
> +		struct counter_count *count,
> +		struct counter_count_read_value *val)
> +{
> +	struct intel_qep *const qep = counter->priv;
> +	uint32_t cntval;
> +
> +	cntval = intel_qep_readl(qep, INTEL_QEPCOUNT);
> +	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cntval);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_count_write(struct counter_device *counter,
> +		struct counter_count *count,
> +		struct counter_count_write_value *val)
> +{
> +	struct intel_qep *const qep = counter->priv;
> +	u32 cnt;
> +	int err;
> +
> +	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
> +	if (err)
> +		return err;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPMAX, cnt);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_function_get(struct counter_device *counter,
> +		struct counter_count *count, size_t *function)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	if (req & INTEL_QEPCON_SWPAB)
> +		*function = INTEL_QEP_ENCODER_MODE_SWAPPED;
> +	else
> +		*function = INTEL_QEP_ENCODER_MODE_NORMAL;
> +
> +	return 0;
> +}
> +
> +static int intel_qep_function_set(struct counter_device *counter,
> +		struct counter_count *count, size_t *function)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	if (*function == INTEL_QEP_ENCODER_MODE_SWAPPED)
> +		reg |= INTEL_QEPCON_SWPAB;
> +	else
> +		reg &= ~INTEL_QEPCON_SWPAB;
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_action_get(struct counter_device *counter,
> +		struct counter_count *count, struct counter_synapse *synapse,
> +		size_t *action)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	*action = reg & synapse->signal->id ?
> +		INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE :
> +		INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE;
> +
> +	return 0;
> +}
> +
> +static int intel_qep_action_set(struct counter_device *counter,
> +		struct counter_count *count,
> +		struct counter_synapse *synapse, size_t action)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (action == INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE)
> +		reg |= synapse->signal->id;
> +	else
> +		reg &= ~synapse->signal->id;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return 0;
> +}
> +
> +static const struct counter_ops intel_qep_counter_ops = {
> +	.count_read = intel_qep_count_read,
> +	.count_write = intel_qep_count_write,
> +
> +	.function_get = intel_qep_function_get,
> +	.function_set = intel_qep_function_set,
> +
> +	.action_get = intel_qep_action_get,
> +	.action_set = intel_qep_action_set,
> +};
> +
> +static struct counter_signal intel_qep_signals[] = {
> +	{
> +		.id = INTEL_QEPCON_EDGE_A,
> +		.name = "Phase A",
> +	},
> +	{
> +		.id = INTEL_QEPCON_EDGE_B,
> +		.name = "Phase B",
> +	},
> +	{
> +		.id = INTEL_QEPCON_EDGE_INDX,
> +		.name = "Index",
> +	},
> +};
> +
> +static struct counter_synapse intel_qep_count_synapses[] = {
> +	{
> +		.actions_list = intel_qep_synapse_actions,
> +		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
> +		.signal = &intel_qep_signals[0],
> +	},
> +	{
> +		.actions_list = intel_qep_synapse_actions,
> +		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
> +		.signal = &intel_qep_signals[1],
> +	},
> +	{
> +		.actions_list = intel_qep_synapse_actions,
> +		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
> +		.signal = &intel_qep_signals[2],
> +	},
> +};
> +
> +static const char * const intel_qep_clock_prescalers[] = {
> +	"1", "2", "4", "8", "16", "32", "64", "128"
> +};
> +
> +static int intel_qep_clock_prescaler_get(struct counter_device *counter,
> +		struct counter_count *count, size_t *mode)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	
> +	*mode = intel_qep_readl(qep->regs, INTEL_QEPCAPDIV);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_clock_prescaler_set(struct counter_device *counter,
> +		struct counter_count *count, size_t mode)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCAPDIV, ffs(mode) - 1);
> +
> +	return 0;
> +}
> +
> +static struct counter_count_enum_ext intel_qep_clock_prescaler_enum = {
> +	.items = intel_qep_clock_prescalers,
> +	.num_items = ARRAY_SIZE(intel_qep_clock_prescalers),
> +	.get = intel_qep_clock_prescaler_get,
> +	.set = intel_qep_clock_prescaler_set,
> +};
> +
> +static ssize_t ceiling_read(struct counter_device *counter,
> +		struct counter_count *count, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPMAX);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", reg);
> +}
> +
> +static ssize_t ceiling_write(struct counter_device *counter,
> +		struct counter_count *count, void *priv, const char *buf,
> +		size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 max;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &max);
> +	if (ret < 0)
> +		return ret;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPMAX, max);
> +
> +	return len;
> +}
> +
> +static ssize_t enable_read(struct counter_device *counter,
> +		struct counter_count *count, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", !!(reg & INTEL_QEPCON_EN));
> +}
> +
> +static ssize_t enable_write(struct counter_device *counter,
> +		struct counter_count *count, void *priv, const char *buf,
> +		size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +	u32 val;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (val)
> +		reg |= INTEL_QEPCON_EN;
> +	else
> +		reg &= ~INTEL_QEPCON_EN;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return len;
> +}
> +
> +static ssize_t direction_read(struct counter_device *counter,
> +		struct counter_count *count, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", qep->direction ?
> +			"forward" : "backward");
> +}
> +
> +static const struct counter_count_ext intel_qep_count_ext[] = {
> +	COUNTER_COUNT_ENUM("prescaler", &intel_qep_clock_prescaler_enum),
> +	COUNTER_COUNT_ENUM_AVAILABLE("prescaler",
> +			&intel_qep_clock_prescaler_enum),
> +
> +	INTEL_QEP_COUNTER_COUNT_EXT_RW(ceiling),
> +	INTEL_QEP_COUNTER_COUNT_EXT_RW(enable),
> +	INTEL_QEP_COUNTER_COUNT_EXT_RO(direction),
> +};
> +
> +static struct counter_count intel_qep_counter_count[] = {
> +	{
> +		.id = 0,
> +		.name = "Channel 1 Count",
> +		.functions_list = intel_qep_count_functions,
> +		.num_functions = ARRAY_SIZE(intel_qep_count_functions),
> +		.synapses = intel_qep_count_synapses,
> +		.num_synapses = ARRAY_SIZE(intel_qep_count_synapses),
> +		.ext = intel_qep_count_ext,
> +		.num_ext = ARRAY_SIZE(intel_qep_count_ext),
> +	},
> +};
> +
> +static ssize_t noise_read(struct counter_device *counter, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (!(reg & INTEL_QEPCON_FLT_EN))
> +		return snprintf(buf, PAGE_SIZE, "0\n");
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPFLT);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", INTEL_QEPFLT_MAX_COUNT(reg));
> +}
> +
> +static ssize_t noise_write(struct counter_device *counter, void *priv,
> +		const char *buf, size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +	u32 max;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &max);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (max > 0x1fffff)
> +		max = 0x1ffff;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (max == 0) {
> +		reg &= ~INTEL_QEPCON_FLT_EN;
> +	} else {
> +		reg |= INTEL_QEPCON_FLT_EN;
> +		intel_qep_writel(qep->regs, INTEL_QEPFLT, max);
> +	}
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +	
> +	return len;
> +}
> +
> +static ssize_t preset_read(struct counter_device *counter, void *priv, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0\n");
> +}
> +
> +static ssize_t preset_enable_read(struct counter_device *counter, void *priv,
> +		char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			!(reg & INTEL_QEPCON_COUNT_RST_MODE));
> +}
> +
> +static ssize_t preset_enable_write(struct counter_device *counter, void *priv,
> +		const char *buf, size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +	u32 val;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (val)
> +		reg &= ~INTEL_QEPCON_COUNT_RST_MODE;
> +	else
> +		reg |= INTEL_QEPCON_COUNT_RST_MODE;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +	
> +	return len;
> +}
> +
> +static const struct counter_device_ext intel_qep_ext[] = {
> +	INTEL_QEP_COUNTER_EXT_RW(noise),
> +	INTEL_QEP_COUNTER_EXT_RO(preset),
> +	INTEL_QEP_COUNTER_EXT_RW(preset_enable)
> +};
> +
> +static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
> +{
> +	struct intel_qep	*qep;
> +	struct device		*dev = &pci->dev;
> +	void __iomem		*regs;
> +	int			ret;
> +	int			irq;
> +
> +	qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
> +	if (!qep)
> +		return -ENOMEM;
> +
> +	ret = pcim_enable_device(pci);
> +	if (ret)
> +		return ret;
> +
> +	pci_set_master(pci);
> +
> +	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
> +	if (ret)
> +		return ret;
> +
> +	regs = pcim_iomap_table(pci)[0];
> +	if (!regs)
> +		return -ENOMEM;
> +
> +	qep->pci = pci;
> +	qep->dev = dev;
> +	qep->regs = regs;
> +	mutex_init(&qep->lock);
> +
> +	intel_qep_init(qep, true);
> +	pci_set_drvdata(pci, qep);
> +
> +	qep->counter.name = pci_name(pci);
> +	qep->counter.parent = dev;
> +	qep->counter.ops = &intel_qep_counter_ops;
> +	qep->counter.counts = intel_qep_counter_count;
> +	qep->counter.num_counts = ARRAY_SIZE(intel_qep_counter_count);
> +	qep->counter.signals = intel_qep_signals;
> +	qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
> +	qep->counter.ext = intel_qep_ext;
> +	qep->counter.num_ext = ARRAY_SIZE(intel_qep_ext);
> +	qep->counter.priv = qep;
> +
> +	ret = counter_register(&qep->counter);
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
> +	if (ret < 0)
> +		goto err_irq_vectors;
> +
> +	irq = pci_irq_vector(pci, 0);
> +	ret = devm_request_threaded_irq(&pci->dev, irq, intel_qep_irq,
> +			intel_qep_irq_thread, IRQF_SHARED | IRQF_TRIGGER_RISING,
> +			"intel-qep", qep);
> +	if (ret)
> +		goto err_irq;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_allow(dev);
> +
> +	return 0;
> +
> +err_irq:
> +	pci_free_irq_vectors(pci);
> +
> +err_irq_vectors:
> +	counter_unregister(&qep->counter);
> +
> +	return ret;
> +}
> +
> +static void intel_qep_remove(struct pci_dev *pci)
> +{
> +	struct intel_qep	*qep = pci_get_drvdata(pci);
> +	struct device		*dev = &pci->dev;
> +
> +	pm_runtime_forbid(dev);
> +	pm_runtime_get_noresume(dev);
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, 0);
> +	pci_free_irq_vectors(pci);
> +	counter_unregister(&qep->counter);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int intel_qep_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int intel_qep_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct intel_qep *qep = pci_get_drvdata(pdev);
> +
> +	intel_qep_init(qep, false);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_runtime_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int intel_qep_runtime_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct intel_qep *qep = pci_get_drvdata(pdev);
> +
> +	intel_qep_init(qep, false);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops intel_qep_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(intel_qep_suspend,
> +				intel_qep_resume)
> +	SET_RUNTIME_PM_OPS(intel_qep_runtime_suspend, intel_qep_runtime_resume,
> +				NULL)
> +};
> +
> +static struct pci_driver intel_qep_driver = {
> +	.name		= "intel-qep",
> +	.id_table	= intel_qep_id_table,
> +	.probe		= intel_qep_probe,
> +	.remove		= intel_qep_remove,
> +	.driver = {
> +		.pm = &intel_qep_pm_ops,
> +	}
> +};
> +
> +module_pci_driver(intel_qep_driver);
> +
> +MODULE_AUTHOR("Felipe Balbi <[e-mail]>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Intel Quadrature Encoder Driver");
> -- 
> 2.23.0
>

Felipe Balbi Sept. 24, 2019, 9:46 a.m. UTC | #2

Hi,

William Breathitt Gray <[e-mail]> writes:
> On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
>> Add support for Intel PSE Quadrature Encoder
>> 
>> Signed-off-by: Felipe Balbi <[e-mail]>
>> ---
>> 
>> Changes since v1:
>> 	- Many more private sysfs files converted over to counter interface
>> 
>> 
>> How do you want me to model this device's Capture Compare Mode (see
>> below)?
>
> Hi Felipe,
>
> I'm CCing Fabien and David as they may be interested in the timestamps
> discussion. See below for some ideas I have on implementing this.
>
>> For the few features which I couldn't find a matching property in
>> counter framework, I still leave them as private sysfs files so we can
>> discuss how to model them in the framework.
>> 
>> Do you want Capture Compare to be a new function mode?
>> 
>> BTW, I know I'm missing a Documentation file documenting sysfs files
>> introduced by this driver, I'll do that once we agree how to move all
>> other sysfs files to the framework. No worries.
>> 
>> 
>> Details about the controller (do you want this in commit log?):
>> 
>> 
>> Controller has 2 modes of operation: QEP and Capture. Both modes are
>> mutually exclusive. We also have a set of maskable interrupts. Further
>> details about each mode below.
>
> I noticed your interrupt handler takes care of a number of different
> scenarios. Would you be able to summarize a bit further here the
> conditions for this device that cause an interrupt to be fired (watchdog
> timeout, FIFO updates, etc.)?
>
>> Quadrature Encoder Mode
>> =======================
>> 
>> Used to measure rotational speed, direction and angle of rotation of a
>> motor shaft. Feature list:
>> 
>> 	- Quadrature decoder providing counter pulses with x4 count
>> 	  resolution and count direction
>> 
>> 	- 32-bit up/down Position Counter for position measurement
>> 
>> 	- Two modes of position counter reset:
>> 		> Maximum Count (ceiling) to reset the position counter
>> 		> Index pulse to reset the position counter
>> 
>> 	- Watchdog timer functionality for detecting ‘stall’ events
>> 
>> Capture Compare Mode
>> ====================
>> 
>> Used to measure phase input signal Period or Duty Cycle. Feature List:
>> 
>> 	- Capture function operates either on phase A or phase B input
>> 	  and can be configured to operate on lo/hi or hi/lo or both hi
>> 	  and lo transitions.
>> 
>> 	- Free-running 32-bit counter to be configured to run at greater
>>           than or equal to 4x input signal frequency
>
> So in "Capture Compare" mode, the counter value is just increasing when
> a state condition transition occurs. In that case we won't need a new
> function mode to represent this behavior since one already exists:
> "increase".
>
> You can add it to your intel_qep_count_functions array like so:
>
>         [INTEL_QEP_ENCODER_MODE_CAPTURE] =
>         COUNTER_COUNT_FUNCTION_INCREASE,
>
> The various configurations for this mode are just Synapse action modes.
> If you want only Phase A, you would set the action mode for Phase A
> ("rising edge", "falling edge", or "both edges") and change the action
> mode for Phase B to "none"; vice-versa configuration for Phase B instead
> of Phase A.
>
> One thing to keep in mind is that action_set will need to maintain valid
> configurations -- so if the user tries to set the action mode for Phase
> A to something other than "none", you need to automatically set Phase
> B's action mode to "none" (and vice-versa).

interesting, thanks

>> 	- Clock post-scaler to derive the counter clock source from the
>> 	  peripheral clock
>
> I see you already have a "prescaler" extension in your code. Is this
> different from the "post-scaler" you mentioned here?

This was probably a brain-fart on my side. It should be post-scaler, but
that's only valid for capture compare mode.

>> 	- 32B wide FIFO to capture 32-bit timestamps of up to 8
>> 	  transition events
>
> You can implement this as a Count extension called "timestamps" or
> similar. What we can do is have a read on this attribute return the
> entire FIFO data buffer as unsigned integers, where each timestamp is
> deliminated by a space.
>
> In addition, it may be useful to have another extension called
> "timestamps_layout", or something along those lines, that will report
> the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.).
>
> Would this design work for your needs?

Perhaps it would be best to have a pollable binary sysfs file (meaning,
we need to be able to call sysfs_notify() at will) and userspace just
receives POLLIN whenever there's data read. Then a read returns an array
of e.g. struct counter_event where struct counter_event could be defined
as:

	struct counter_event {
        	uint32_t	event_type;
		struct timespec64 timestamp;
                uint8_t		reserved[32];
        };

Userspace would do something along the lines of:

	fd = open("/sys/bus/counter/foo/capture/timestamps",...);
	pollfd[0].fd = fd;
        pollfd[0].events = POLLIN;
        poll(pollfd, 1, -1);

	if (pollfd[0].revents & POLLIN) {
        	ret = read(fd, events, sizeof(struct counter_event) * 8);

		for (i = 0; i < ret / sizeof(struct counter_event); i++)
			process_event(events[i]);
        }
        
Or something like that.

I could, also, remove this part from the driver for now, so we can
discuss how the capture-compare buffer read would look like. At least we
could get QDP feature merged while we come to a consensus about capture
compare.

What do you think?

William Breathitt Gray Sept. 24, 2019, 11:32 a.m. UTC | #3

On Tue, Sep 24, 2019 at 12:46:39PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> William Breathitt Gray <[e-mail]> writes:
> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
> >> Add support for Intel PSE Quadrature Encoder
> >> 
> >> Signed-off-by: Felipe Balbi <[e-mail]>
> >> ---
> >> 
> >> Changes since v1:
> >> 	- Many more private sysfs files converted over to counter interface
> >> 
> >> 
> >> How do you want me to model this device's Capture Compare Mode (see
> >> below)?
> >
> > Hi Felipe,
> >
> > I'm CCing Fabien and David as they may be interested in the timestamps
> > discussion. See below for some ideas I have on implementing this.
> >
> >> For the few features which I couldn't find a matching property in
> >> counter framework, I still leave them as private sysfs files so we can
> >> discuss how to model them in the framework.
> >> 
> >> Do you want Capture Compare to be a new function mode?
> >> 
> >> BTW, I know I'm missing a Documentation file documenting sysfs files
> >> introduced by this driver, I'll do that once we agree how to move all
> >> other sysfs files to the framework. No worries.
> >> 
> >> 
> >> Details about the controller (do you want this in commit log?):
> >> 
> >> 
> >> Controller has 2 modes of operation: QEP and Capture. Both modes are
> >> mutually exclusive. We also have a set of maskable interrupts. Further
> >> details about each mode below.
> >
> > I noticed your interrupt handler takes care of a number of different
> > scenarios. Would you be able to summarize a bit further here the
> > conditions for this device that cause an interrupt to be fired (watchdog
> > timeout, FIFO updates, etc.)?
> >
> >> Quadrature Encoder Mode
> >> =======================
> >> 
> >> Used to measure rotational speed, direction and angle of rotation of a
> >> motor shaft. Feature list:
> >> 
> >> 	- Quadrature decoder providing counter pulses with x4 count
> >> 	  resolution and count direction
> >> 
> >> 	- 32-bit up/down Position Counter for position measurement
> >> 
> >> 	- Two modes of position counter reset:
> >> 		> Maximum Count (ceiling) to reset the position counter
> >> 		> Index pulse to reset the position counter
> >> 
> >> 	- Watchdog timer functionality for detecting ‘stall’ events
> >> 
> >> Capture Compare Mode
> >> ====================
> >> 
> >> Used to measure phase input signal Period or Duty Cycle. Feature List:
> >> 
> >> 	- Capture function operates either on phase A or phase B input
> >> 	  and can be configured to operate on lo/hi or hi/lo or both hi
> >> 	  and lo transitions.
> >> 
> >> 	- Free-running 32-bit counter to be configured to run at greater
> >>           than or equal to 4x input signal frequency
> >
> > So in "Capture Compare" mode, the counter value is just increasing when
> > a state condition transition occurs. In that case we won't need a new
> > function mode to represent this behavior since one already exists:
> > "increase".
> >
> > You can add it to your intel_qep_count_functions array like so:
> >
> >         [INTEL_QEP_ENCODER_MODE_CAPTURE] =
> >         COUNTER_COUNT_FUNCTION_INCREASE,
> >
> > The various configurations for this mode are just Synapse action modes.
> > If you want only Phase A, you would set the action mode for Phase A
> > ("rising edge", "falling edge", or "both edges") and change the action
> > mode for Phase B to "none"; vice-versa configuration for Phase B instead
> > of Phase A.
> >
> > One thing to keep in mind is that action_set will need to maintain valid
> > configurations -- so if the user tries to set the action mode for Phase
> > A to something other than "none", you need to automatically set Phase
> > B's action mode to "none" (and vice-versa).
> 
> interesting, thanks
> 
> >> 	- Clock post-scaler to derive the counter clock source from the
> >> 	  peripheral clock
> >
> > I see you already have a "prescaler" extension in your code. Is this
> > different from the "post-scaler" you mentioned here?
> 
> This was probably a brain-fart on my side. It should be post-scaler, but
> that's only valid for capture compare mode.

In that case you're implementation seems fine for this; perhaps a more
generic name for that extension might be better such as "scale", but
I'll decide later.
 
> >> 	- 32B wide FIFO to capture 32-bit timestamps of up to 8
> >> 	  transition events
> >
> > You can implement this as a Count extension called "timestamps" or
> > similar. What we can do is have a read on this attribute return the
> > entire FIFO data buffer as unsigned integers, where each timestamp is
> > deliminated by a space.
> >
> > In addition, it may be useful to have another extension called
> > "timestamps_layout", or something along those lines, that will report
> > the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.).
> >
> > Would this design work for your needs?
> 
> Perhaps it would be best to have a pollable binary sysfs file (meaning,
> we need to be able to call sysfs_notify() at will) and userspace just
> receives POLLIN whenever there's data read. Then a read returns an array
> of e.g. struct counter_event where struct counter_event could be defined
> as:
> 
> 	struct counter_event {
>         	uint32_t	event_type;
> 		struct timespec64 timestamp;
>                 uint8_t		reserved[32];
>         };
> 
> Userspace would do something along the lines of:
> 
> 	fd = open("/sys/bus/counter/foo/capture/timestamps",...);
> 	pollfd[0].fd = fd;
>         pollfd[0].events = POLLIN;
>         poll(pollfd, 1, -1);
> 
> 	if (pollfd[0].revents & POLLIN) {
>         	ret = read(fd, events, sizeof(struct counter_event) * 8);
> 
> 		for (i = 0; i < ret / sizeof(struct counter_event); i++)
> 			process_event(events[i]);
>         }
>         
> Or something like that.

One concern is that returning binary data like that isn't friendly for
human interaction. However, alternatively printing a human-readable
array would add latency for userspace software that has to interpret it,
so that would a problem as well. This is something we'll have to think
more about then.

> I could, also, remove this part from the driver for now, so we can
> discuss how the capture-compare buffer read would look like. At least we
> could get QDP feature merged while we come to a consensus about capture
> compare.
> 
> What do you think?
> 
> -- 
> balbi

That sounds like a good idea. Most of this driver can be implemented
using the existing Counter subsystem components, so we can do as you
suggest and focus on just getting this driver merged in with the
functionality it can for now.

After it's accepted and merged, we can turn our attention to the new
extension features such as the timestamps buffer return. This will make
it easier for us to discuss ideas since we won't have to worry about
merging in nonrelated functionality.

William Breathitt Gray

Felipe Balbi Sept. 24, 2019, 11:35 a.m. UTC | #4

Hi,

William Breathitt Gray <[e-mail]> writes:
>> William Breathitt Gray <[e-mail]> writes:
>> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
>> >> Add support for Intel PSE Quadrature Encoder
>> >> 
>> >> Signed-off-by: Felipe Balbi <[e-mail]>
>> >> ---
>> >> 
>> >> Changes since v1:
>> >> 	- Many more private sysfs files converted over to counter interface
>> >> 
>> >> 
>> >> How do you want me to model this device's Capture Compare Mode (see
>> >> below)?
>> >
>> > Hi Felipe,
>> >
>> > I'm CCing Fabien and David as they may be interested in the timestamps
>> > discussion. See below for some ideas I have on implementing this.
>> >
>> >> For the few features which I couldn't find a matching property in
>> >> counter framework, I still leave them as private sysfs files so we can
>> >> discuss how to model them in the framework.
>> >> 
>> >> Do you want Capture Compare to be a new function mode?
>> >> 
>> >> BTW, I know I'm missing a Documentation file documenting sysfs files
>> >> introduced by this driver, I'll do that once we agree how to move all
>> >> other sysfs files to the framework. No worries.
>> >> 
>> >> 
>> >> Details about the controller (do you want this in commit log?):
>> >> 
>> >> 
>> >> Controller has 2 modes of operation: QEP and Capture. Both modes are
>> >> mutually exclusive. We also have a set of maskable interrupts. Further
>> >> details about each mode below.
>> >
>> > I noticed your interrupt handler takes care of a number of different
>> > scenarios. Would you be able to summarize a bit further here the
>> > conditions for this device that cause an interrupt to be fired (watchdog
>> > timeout, FIFO updates, etc.)?
>> >
>> >> Quadrature Encoder Mode
>> >> =======================
>> >> 
>> >> Used to measure rotational speed, direction and angle of rotation of a
>> >> motor shaft. Feature list:
>> >> 
>> >> 	- Quadrature decoder providing counter pulses with x4 count
>> >> 	  resolution and count direction
>> >> 
>> >> 	- 32-bit up/down Position Counter for position measurement
>> >> 
>> >> 	- Two modes of position counter reset:
>> >> 		> Maximum Count (ceiling) to reset the position counter
>> >> 		> Index pulse to reset the position counter
>> >> 
>> >> 	- Watchdog timer functionality for detecting ‘stall’ events
>> >> 
>> >> Capture Compare Mode
>> >> ====================
>> >> 
>> >> Used to measure phase input signal Period or Duty Cycle. Feature List:
>> >> 
>> >> 	- Capture function operates either on phase A or phase B input
>> >> 	  and can be configured to operate on lo/hi or hi/lo or both hi
>> >> 	  and lo transitions.
>> >> 
>> >> 	- Free-running 32-bit counter to be configured to run at greater
>> >>           than or equal to 4x input signal frequency
>> >
>> > So in "Capture Compare" mode, the counter value is just increasing when
>> > a state condition transition occurs. In that case we won't need a new
>> > function mode to represent this behavior since one already exists:
>> > "increase".
>> >
>> > You can add it to your intel_qep_count_functions array like so:
>> >
>> >         [INTEL_QEP_ENCODER_MODE_CAPTURE] =
>> >         COUNTER_COUNT_FUNCTION_INCREASE,
>> >
>> > The various configurations for this mode are just Synapse action modes.
>> > If you want only Phase A, you would set the action mode for Phase A
>> > ("rising edge", "falling edge", or "both edges") and change the action
>> > mode for Phase B to "none"; vice-versa configuration for Phase B instead
>> > of Phase A.
>> >
>> > One thing to keep in mind is that action_set will need to maintain valid
>> > configurations -- so if the user tries to set the action mode for Phase
>> > A to something other than "none", you need to automatically set Phase
>> > B's action mode to "none" (and vice-versa).
>> 
>> interesting, thanks
>> 
>> >> 	- Clock post-scaler to derive the counter clock source from the
>> >> 	  peripheral clock
>> >
>> > I see you already have a "prescaler" extension in your code. Is this
>> > different from the "post-scaler" you mentioned here?
>> 
>> This was probably a brain-fart on my side. It should be post-scaler, but
>> that's only valid for capture compare mode.
>
> In that case you're implementation seems fine for this; perhaps a more
> generic name for that extension might be better such as "scale", but
> I'll decide later.
>  
>> >> 	- 32B wide FIFO to capture 32-bit timestamps of up to 8
>> >> 	  transition events
>> >
>> > You can implement this as a Count extension called "timestamps" or
>> > similar. What we can do is have a read on this attribute return the
>> > entire FIFO data buffer as unsigned integers, where each timestamp is
>> > deliminated by a space.
>> >
>> > In addition, it may be useful to have another extension called
>> > "timestamps_layout", or something along those lines, that will report
>> > the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.).
>> >
>> > Would this design work for your needs?
>> 
>> Perhaps it would be best to have a pollable binary sysfs file (meaning,
>> we need to be able to call sysfs_notify() at will) and userspace just
>> receives POLLIN whenever there's data read. Then a read returns an array
>> of e.g. struct counter_event where struct counter_event could be defined
>> as:
>> 
>> 	struct counter_event {
>>         	uint32_t	event_type;
>> 		struct timespec64 timestamp;
>>                 uint8_t		reserved[32];
>>         };
>> 
>> Userspace would do something along the lines of:
>> 
>> 	fd = open("/sys/bus/counter/foo/capture/timestamps",...);
>> 	pollfd[0].fd = fd;
>>         pollfd[0].events = POLLIN;
>>         poll(pollfd, 1, -1);
>> 
>> 	if (pollfd[0].revents & POLLIN) {
>>         	ret = read(fd, events, sizeof(struct counter_event) * 8);
>> 
>> 		for (i = 0; i < ret / sizeof(struct counter_event); i++)
>> 			process_event(events[i]);
>>         }
>>         
>> Or something like that.
>
> One concern is that returning binary data like that isn't friendly for
> human interaction. However, alternatively printing a human-readable
> array would add latency for userspace software that has to interpret it,
> so that would a problem as well. This is something we'll have to think
> more about then.
>
>> I could, also, remove this part from the driver for now, so we can
>> discuss how the capture-compare buffer read would look like. At least we
>> could get QDP feature merged while we come to a consensus about capture
>> compare.
>> 
>> What do you think?
>> 
>> -- 
>> balbi
>
> That sounds like a good idea. Most of this driver can be implemented
> using the existing Counter subsystem components, so we can do as you
> suggest and focus on just getting this driver merged in with the
> functionality it can for now.
>
> After it's accepted and merged, we can turn our attention to the new
> extension features such as the timestamps buffer return. This will make
> it easier for us to discuss ideas since we won't have to worry about
> merging in nonrelated functionality.

That's great. I'll prepare a version removing capture compare support
and all the extra sysfs files. That could potentially be merged for v5.5
merge window.

In parallel, we discuss capture compare buffer and how to expose it.

cheers

David Lechner Sept. 24, 2019, 9:46 p.m. UTC | #5

On 9/22/19 6:35 PM, William Breathitt Gray wrote:
> On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
>> Add support for Intel PSE Quadrature Encoder
>>
>> Signed-off-by: Felipe Balbi <[e-mail]>
>> ---
>>
>> Changes since v1:
>> 	- Many more private sysfs files converted over to counter interface
>>
>>
>> How do you want me to model this device's Capture Compare Mode (see
>> below)?
> 
> Hi Felipe,
> 
> I'm CCing Fabien and David as they may be interested in the timestamps
> discussion. See below for some ideas I have on implementing this.
> 

Could be an interesting read (thread from my first counter driver):

https://lore.kernel.org/linux-iio/[e-mail]/

What would be useful to me is something like the buffer feature in iio
where a timestamp is associated with a count and stored in a buffer so that
we can look at a window of all values recorded in the last 20ms. Being able
to access this via mmap would be very helpful for performance (running on
300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
unless it is a rare event, like a watchdog timeout.

William Breathitt Gray Sept. 28, 2019, 9:33 p.m. UTC | #6

On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:
> On 9/22/19 6:35 PM, William Breathitt Gray wrote:
> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
> >> Add support for Intel PSE Quadrature Encoder
> >>
> >> Signed-off-by: Felipe Balbi <[e-mail]>
> >> ---
> >>
> >> Changes since v1:
> >> 	- Many more private sysfs files converted over to counter interface
> >>
> >>
> >> How do you want me to model this device's Capture Compare Mode (see
> >> below)?
> > 
> > Hi Felipe,
> > 
> > I'm CCing Fabien and David as they may be interested in the timestamps
> > discussion. See below for some ideas I have on implementing this.
> > 
> 
> Could be an interesting read (thread from my first counter driver):
> 
> https://lore.kernel.org/linux-iio/[e-mail]/
> 
> What would be useful to me is something like the buffer feature in iio
> where a timestamp is associated with a count and stored in a buffer so that
> we can look at a window of all values recorded in the last 20ms. Being able
> to access this via mmap would be very helpful for performance (running on
> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
> unless it is a rare event, like a watchdog timeout.

I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
timestamps and buffers. I don't want to reinvent something that is
working well, so hopefully we can reuse the IIO timestamp design for the
Counter subsystem.

I would argue that a human-readable timestamps printout is useful for
certain applications (e.g. a tally counter attached to a fault line: a
human administrator will be able to review previous fault times).
However as you point out, a low latency operation is necessary for
performance critical applications.

Although you are correct that mmap is a good low latency operation to
get access to a timestamp buffer, I'm afraid giving direct access to
memory like that will lead to many incompatible representations of
timestamp data (e.g. variations in endianness, signedness, data size,
etc.). I would like a standardized representation for this data that
userspace applications can expect to receive and interpret, especially
when time is widely represented as an unsigned integer.

Felipe suggested the creation of a counter_event structure so that users
can poll on an attribute. This kind of behavior is useful for notifying
users of interrupts and other events, but I think we should restrict the
use of the read call on these sysfs attributes to just human-readable
data. Instead, perhaps ioctl calls can be used to facilitate binary data
transfers.

For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
that returns a counter_timestamps structure with a timestamps array
populated:

        struct counter_timestamps{
                size_t num_timestamps;
        	unsigned int *timestamps;
        }

That would allow quick access to the timestamps data, while also
restricting it to a standard representation that all userspace
applications can follow and interpret. In addition, this won't interfer
with polling, so users can still wait for an interrupt and then decide
whether they want to use the slower human-readable printout (via read)
or the faster binary data access (via ioctl).

William Breathitt Gray

Felipe Balbi Sept. 30, 2019, 5:22 a.m. UTC | #7

Hi,

William Breathitt Gray <[e-mail]> writes:
> On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:
>> On 9/22/19 6:35 PM, William Breathitt Gray wrote:
>> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
>> >> Add support for Intel PSE Quadrature Encoder
>> >>
>> >> Signed-off-by: Felipe Balbi <[e-mail]>
>> >> ---
>> >>
>> >> Changes since v1:
>> >> 	- Many more private sysfs files converted over to counter interface
>> >>
>> >>
>> >> How do you want me to model this device's Capture Compare Mode (see
>> >> below)?
>> > 
>> > Hi Felipe,
>> > 
>> > I'm CCing Fabien and David as they may be interested in the timestamps
>> > discussion. See below for some ideas I have on implementing this.
>> > 
>> 
>> Could be an interesting read (thread from my first counter driver):
>> 
>> https://lore.kernel.org/linux-iio/[e-mail]/
>> 
>> What would be useful to me is something like the buffer feature in iio
>> where a timestamp is associated with a count and stored in a buffer so that
>> we can look at a window of all values recorded in the last 20ms. Being able
>> to access this via mmap would be very helpful for performance (running on
>> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
>> unless it is a rare event, like a watchdog timeout.
>
> I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
> timestamps and buffers. I don't want to reinvent something that is
> working well, so hopefully we can reuse the IIO timestamp design for the
> Counter subsystem.
>
> I would argue that a human-readable timestamps printout is useful for
> certain applications (e.g. a tally counter attached to a fault line: a
> human administrator will be able to review previous fault times).
> However as you point out, a low latency operation is necessary for
> performance critical applications.
>
> Although you are correct that mmap is a good low latency operation to
> get access to a timestamp buffer, I'm afraid giving direct access to
> memory like that will lead to many incompatible representations of
> timestamp data (e.g. variations in endianness, signedness, data size,
> etc.). I would like a standardized representation for this data that
> userspace applications can expect to receive and interpret, especially
> when time is widely represented as an unsigned integer.
>
> Felipe suggested the creation of a counter_event structure so that users
> can poll on an attribute. This kind of behavior is useful for notifying
> users of interrupts and other events, but I think we should restrict the
> use of the read call on these sysfs attributes to just human-readable
> data. Instead, perhaps ioctl calls can be used to facilitate binary data
> transfers.
>
> For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
> that returns a counter_timestamps structure with a timestamps array
> populated:
>
>         struct counter_timestamps{
>                 size_t num_timestamps;
>         	unsigned int *timestamps;
>         }
>
> That would allow quick access to the timestamps data, while also
> restricting it to a standard representation that all userspace
> applications can follow and interpret. In addition, this won't interfer
> with polling, so users can still wait for an interrupt and then decide
> whether they want to use the slower human-readable printout (via read)
> or the faster binary data access (via ioctl).

Seems like we're starting to build the need for a /dev/counter[0123...]
representation of the subsystem. If that's the case, then it may very
well be that sysfs becomes somewhat optional.

I think is makes sense to rely more on character devices specially since
I know of devices running linux with so little memory that sysfs (and a
bunch of other features) are removed from the kernel. Having a character
device representation would allow counter subsystem to be used on such
devices.

cheers

William Breathitt Gray Oct. 2, 2019, 12:34 a.m. UTC | #8

On Mon, Sep 30, 2019 at 08:22:39AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> William Breathitt Gray <[e-mail]> writes:
> > On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:
> >> On 9/22/19 6:35 PM, William Breathitt Gray wrote:
> >> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
> >> >> Add support for Intel PSE Quadrature Encoder
> >> >>
> >> >> Signed-off-by: Felipe Balbi <[e-mail]>
> >> >> ---
> >> >>
> >> >> Changes since v1:
> >> >> 	- Many more private sysfs files converted over to counter interface
> >> >>
> >> >>
> >> >> How do you want me to model this device's Capture Compare Mode (see
> >> >> below)?
> >> > 
> >> > Hi Felipe,
> >> > 
> >> > I'm CCing Fabien and David as they may be interested in the timestamps
> >> > discussion. See below for some ideas I have on implementing this.
> >> > 
> >> 
> >> Could be an interesting read (thread from my first counter driver):
> >> 
> >> https://lore.kernel.org/linux-iio/[e-mail]/
> >> 
> >> What would be useful to me is something like the buffer feature in iio
> >> where a timestamp is associated with a count and stored in a buffer so that
> >> we can look at a window of all values recorded in the last 20ms. Being able
> >> to access this via mmap would be very helpful for performance (running on
> >> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
> >> unless it is a rare event, like a watchdog timeout.
> >
> > I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
> > timestamps and buffers. I don't want to reinvent something that is
> > working well, so hopefully we can reuse the IIO timestamp design for the
> > Counter subsystem.
> >
> > I would argue that a human-readable timestamps printout is useful for
> > certain applications (e.g. a tally counter attached to a fault line: a
> > human administrator will be able to review previous fault times).
> > However as you point out, a low latency operation is necessary for
> > performance critical applications.
> >
> > Although you are correct that mmap is a good low latency operation to
> > get access to a timestamp buffer, I'm afraid giving direct access to
> > memory like that will lead to many incompatible representations of
> > timestamp data (e.g. variations in endianness, signedness, data size,
> > etc.). I would like a standardized representation for this data that
> > userspace applications can expect to receive and interpret, especially
> > when time is widely represented as an unsigned integer.
> >
> > Felipe suggested the creation of a counter_event structure so that users
> > can poll on an attribute. This kind of behavior is useful for notifying
> > users of interrupts and other events, but I think we should restrict the
> > use of the read call on these sysfs attributes to just human-readable
> > data. Instead, perhaps ioctl calls can be used to facilitate binary data
> > transfers.
> >
> > For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
> > that returns a counter_timestamps structure with a timestamps array
> > populated:
> >
> >         struct counter_timestamps{
> >                 size_t num_timestamps;
> >         	unsigned int *timestamps;
> >         }
> >
> > That would allow quick access to the timestamps data, while also
> > restricting it to a standard representation that all userspace
> > applications can follow and interpret. In addition, this won't interfer
> > with polling, so users can still wait for an interrupt and then decide
> > whether they want to use the slower human-readable printout (via read)
> > or the faster binary data access (via ioctl).
> 
> Seems like we're starting to build the need for a /dev/counter[0123...]
> representation of the subsystem. If that's the case, then it may very
> well be that sysfs becomes somewhat optional.
> 
> I think is makes sense to rely more on character devices specially since
> I know of devices running linux with so little memory that sysfs (and a
> bunch of other features) are removed from the kernel. Having a character
> device representation would allow counter subsystem to be used on such
> devices.
> 
> cheers
> 
> -- 
> balbi

A character device node for a counter might be a good idea. If a
performance critical application can't depend on parsing a sysfs
printout for timestamps, then it probably doesn't want to do so for the
other attributes either. I think you are right that certain systems
would have sysfs disabled for that very reason.

I think latency concerns are the same reason the GPIO subsystem started
providing character device nodes as well. We can do similar with the
Counter subsystem: provide character device nodes by default, and
optionally provide the human-readable sysfs interface as well. This
would allow applications with latency concerns to use a standard
interface for the Counter subsystem, while optionally providing a
simpler sysfs interface for other users.

William Breathitt Gray

Jonathan Cameron Oct. 3, 2019, 12:38 p.m. UTC | #9

On Tue, 24 Sep 2019 20:32:58 +0900
William Breathitt Gray <[e-mail]> wrote:

> On Tue, Sep 24, 2019 at 12:46:39PM +0300, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > William Breathitt Gray <[e-mail]> writes:  
> > > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:  
> > >> Add support for Intel PSE Quadrature Encoder
> > >> 
> > >> Signed-off-by: Felipe Balbi <[e-mail]>
> > >> ---
> > >> 
> > >> Changes since v1:
> > >> 	- Many more private sysfs files converted over to counter interface
> > >> 
> > >> 
> > >> How do you want me to model this device's Capture Compare Mode (see
> > >> below)?  
> > >
> > > Hi Felipe,
> > >
> > > I'm CCing Fabien and David as they may be interested in the timestamps
> > > discussion. See below for some ideas I have on implementing this.
> > >  
> > >> For the few features which I couldn't find a matching property in
> > >> counter framework, I still leave them as private sysfs files so we can
> > >> discuss how to model them in the framework.
> > >> 
> > >> Do you want Capture Compare to be a new function mode?
> > >> 
> > >> BTW, I know I'm missing a Documentation file documenting sysfs files
> > >> introduced by this driver, I'll do that once we agree how to move all
> > >> other sysfs files to the framework. No worries.
> > >> 
> > >> 
> > >> Details about the controller (do you want this in commit log?):
> > >> 
> > >> 
> > >> Controller has 2 modes of operation: QEP and Capture. Both modes are
> > >> mutually exclusive. We also have a set of maskable interrupts. Further
> > >> details about each mode below.  
> > >
> > > I noticed your interrupt handler takes care of a number of different
> > > scenarios. Would you be able to summarize a bit further here the
> > > conditions for this device that cause an interrupt to be fired (watchdog
> > > timeout, FIFO updates, etc.)?
> > >  
> > >> Quadrature Encoder Mode
> > >> =======================
> > >> 
> > >> Used to measure rotational speed, direction and angle of rotation of a
> > >> motor shaft. Feature list:
> > >> 
> > >> 	- Quadrature decoder providing counter pulses with x4 count
> > >> 	  resolution and count direction
> > >> 
> > >> 	- 32-bit up/down Position Counter for position measurement
> > >> 
> > >> 	- Two modes of position counter reset:  
> > >> 		> Maximum Count (ceiling) to reset the position counter
> > >> 		> Index pulse to reset the position counter  
> > >> 
> > >> 	- Watchdog timer functionality for detecting ‘stall’ events
> > >> 
> > >> Capture Compare Mode
> > >> ====================
> > >> 
> > >> Used to measure phase input signal Period or Duty Cycle. Feature List:
> > >> 
> > >> 	- Capture function operates either on phase A or phase B input
> > >> 	  and can be configured to operate on lo/hi or hi/lo or both hi
> > >> 	  and lo transitions.
> > >> 
> > >> 	- Free-running 32-bit counter to be configured to run at greater
> > >>           than or equal to 4x input signal frequency  
> > >
> > > So in "Capture Compare" mode, the counter value is just increasing when
> > > a state condition transition occurs. In that case we won't need a new
> > > function mode to represent this behavior since one already exists:
> > > "increase".
> > >
> > > You can add it to your intel_qep_count_functions array like so:
> > >
> > >         [INTEL_QEP_ENCODER_MODE_CAPTURE] =
> > >         COUNTER_COUNT_FUNCTION_INCREASE,
> > >
> > > The various configurations for this mode are just Synapse action modes.
> > > If you want only Phase A, you would set the action mode for Phase A
> > > ("rising edge", "falling edge", or "both edges") and change the action
> > > mode for Phase B to "none"; vice-versa configuration for Phase B instead
> > > of Phase A.
> > >
> > > One thing to keep in mind is that action_set will need to maintain valid
> > > configurations -- so if the user tries to set the action mode for Phase
> > > A to something other than "none", you need to automatically set Phase
> > > B's action mode to "none" (and vice-versa).  
> > 
> > interesting, thanks
> >   
> > >> 	- Clock post-scaler to derive the counter clock source from the
> > >> 	  peripheral clock  
> > >
> > > I see you already have a "prescaler" extension in your code. Is this
> > > different from the "post-scaler" you mentioned here?  
> > 
> > This was probably a brain-fart on my side. It should be post-scaler, but
> > that's only valid for capture compare mode.  
> 
> In that case you're implementation seems fine for this; perhaps a more
> generic name for that extension might be better such as "scale", but
> I'll decide later.
>  
> > >> 	- 32B wide FIFO to capture 32-bit timestamps of up to 8
> > >> 	  transition events  
> > >
> > > You can implement this as a Count extension called "timestamps" or
> > > similar. What we can do is have a read on this attribute return the
> > > entire FIFO data buffer as unsigned integers, where each timestamp is
> > > deliminated by a space.
> > >
> > > In addition, it may be useful to have another extension called
> > > "timestamps_layout", or something along those lines, that will report
> > > the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.).
> > >
> > > Would this design work for your needs?  
> > 
> > Perhaps it would be best to have a pollable binary sysfs file (meaning,
> > we need to be able to call sysfs_notify() at will) and userspace just
> > receives POLLIN whenever there's data read. Then a read returns an array
> > of e.g. struct counter_event where struct counter_event could be defined
> > as:
> > 
> > 	struct counter_event {
> >         	uint32_t	event_type;
> > 		struct timespec64 timestamp;
> >                 uint8_t		reserved[32];
> >         };
> > 
> > Userspace would do something along the lines of:
> > 
> > 	fd = open("/sys/bus/counter/foo/capture/timestamps",...);
> > 	pollfd[0].fd = fd;
> >         pollfd[0].events = POLLIN;
> >         poll(pollfd, 1, -1);
> > 
> > 	if (pollfd[0].revents & POLLIN) {
> >         	ret = read(fd, events, sizeof(struct counter_event) * 8);
> > 
> > 		for (i = 0; i < ret / sizeof(struct counter_event); i++)
> > 			process_event(events[i]);
> >         }
> >         
> > Or something like that.  
> 
> One concern is that returning binary data like that isn't friendly for
> human interaction. However, alternatively printing a human-readable
> array would add latency for userspace software that has to interpret it,
> so that would a problem as well. This is something we'll have to think
> more about then.

I've been rather offline for a little while so just catching up.
Seems the conversation has moved on from this, but to avoid us circling
back, there are distinct rules for sysfs.

1. Single value - whilst this gets stretched a bit to allow things like
   rotation matrices where they are representing one 'thing', a fifo isn't
   going to be acceptable.

2. Binary sysfs files are probably not a path for this sort of thing either.
   IIRC as a general rule they are blocked for usecases like this (and most
   others in new drivers).

Jonathan

> 
> > I could, also, remove this part from the driver for now, so we can
> > discuss how the capture-compare buffer read would look like. At least we
> > could get QDP feature merged while we come to a consensus about capture
> > compare.
> > 
> > What do you think?
> > 
> > -- 
> > balbi  
> 
> That sounds like a good idea. Most of this driver can be implemented
> using the existing Counter subsystem components, so we can do as you
> suggest and focus on just getting this driver merged in with the
> functionality it can for now.
> 
> After it's accepted and merged, we can turn our attention to the new
> extension features such as the timestamps buffer return. This will make
> it easier for us to discuss ideas since we won't have to worry about
> merging in nonrelated functionality.
> 
> William Breathitt Gray

Jonathan Cameron Oct. 3, 2019, 1:14 p.m. UTC | #10

On Tue, 1 Oct 2019 20:34:42 -0400
William Breathitt Gray <[e-mail]> wrote:

> On Mon, Sep 30, 2019 at 08:22:39AM +0300, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > William Breathitt Gray <[e-mail]> writes:  
> > > On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:  
> > >> On 9/22/19 6:35 PM, William Breathitt Gray wrote:  
> > >> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:  
> > >> >> Add support for Intel PSE Quadrature Encoder
> > >> >>
> > >> >> Signed-off-by: Felipe Balbi <[e-mail]>
> > >> >> ---
> > >> >>
> > >> >> Changes since v1:
> > >> >> 	- Many more private sysfs files converted over to counter interface
> > >> >>
> > >> >>
> > >> >> How do you want me to model this device's Capture Compare Mode (see
> > >> >> below)?  
> > >> > 
> > >> > Hi Felipe,
> > >> > 
> > >> > I'm CCing Fabien and David as they may be interested in the timestamps
> > >> > discussion. See below for some ideas I have on implementing this.
> > >> >   
> > >> 
> > >> Could be an interesting read (thread from my first counter driver):
> > >> 
> > >> https://lore.kernel.org/linux-iio/[e-mail]/
> > >> 
> > >> What would be useful to me is something like the buffer feature in iio
> > >> where a timestamp is associated with a count and stored in a buffer so that
> > >> we can look at a window of all values recorded in the last 20ms. Being able
> > >> to access this via mmap would be very helpful for performance (running on
> > >> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
> > >> unless it is a rare event, like a watchdog timeout.  
> > >
> > > I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
> > > timestamps and buffers. I don't want to reinvent something that is
> > > working well, so hopefully we can reuse the IIO timestamp design for the
> > > Counter subsystem.

Simple approach, push them both into a kfifo. Userspace then reads this
(with poll/select/blocking read + things like watermarks all available).
The description of what is there in each record is handled via sysfs as
the IIO model is that the format of each record only changes with userspace
intervention.

The complexities mostly come from allowing the hardware side of what is there
and the software side to differ, but this is only really needed if you want
to have multiple readers and need to split up the data so each thinks it has
the device to itself.  I would guess not needed for counter usecases, but who
knows down the line.

> > >
> > > I would argue that a human-readable timestamps printout is useful for
> > > certain applications (e.g. a tally counter attached to a fault line: a
> > > human administrator will be able to review previous fault times).
> > > However as you point out, a low latency operation is necessary for
> > > performance critical applications.

For this sort of case you could use a sysfs file that just returns the oldest
entry when read.  A seek back to the start and reread or a reopen of the file
can then give you the next one and an appropriate error return indicates none
left.

> > >
> > > Although you are correct that mmap is a good low latency operation to
> > > get access to a timestamp buffer, I'm afraid giving direct access to
> > > memory like that will lead to many incompatible representations of
> > > timestamp data (e.g. variations in endianness, signedness, data size,
> > > etc.). I would like a standardized representation for this data that
> > > userspace applications can expect to receive and interpret, especially
> > > when time is widely represented as an unsigned integer.

It is moderately hard to get that sort of interface right, but it can be done.
IIO does it for DMA type buffers, but not for anything where a software
fifo is involved.   You would need to guarantee a fixed format etc.

> > >
> > > Felipe suggested the creation of a counter_event structure so that users
> > > can poll on an attribute. This kind of behavior is useful for notifying
> > > users of interrupts and other events, but I think we should restrict the
> > > use of the read call on these sysfs attributes to just human-readable
> > > data. Instead, perhaps ioctl calls can be used to facilitate binary data
> > > transfers.
> > >
> > > For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
> > > that returns a counter_timestamps structure with a timestamps array
> > > populated:
> > >
> > >         struct counter_timestamps{
> > >                 size_t num_timestamps;
> > >         	unsigned int *timestamps;
> > >         }
> > >
> > > That would allow quick access to the timestamps data, while also
> > > restricting it to a standard representation that all userspace
> > > applications can follow and interpret. In addition, this won't interfer
> > > with polling, so users can still wait for an interrupt and then decide
> > > whether they want to use the slower human-readable printout (via read)
> > > or the faster binary data access (via ioctl).  
> > 
> > Seems like we're starting to build the need for a /dev/counter[0123...]
> > representation of the subsystem. If that's the case, then it may very
> > well be that sysfs becomes somewhat optional.

Agreed.  Don't map this stuff onto sysfs where a chardev is more sensible.

> > 
> > I think is makes sense to rely more on character devices specially since
> > I know of devices running linux with so little memory that sysfs (and a
> > bunch of other features) are removed from the kernel. Having a character
> > device representation would allow counter subsystem to be used on such
> > devices.
> > 
> > cheers
> > 
> > -- 
> > balbi  
> 
> A character device node for a counter might be a good idea. If a
> performance critical application can't depend on parsing a sysfs
> printout for timestamps, then it probably doesn't want to do so for the
> other attributes either. I think you are right that certain systems
> would have sysfs disabled for that very reason.
> 

I am a little curious to whether people often run sysfs free any more?
I know it used to happen a fair bit, but a lot of the kernel is now rather
dependent on it.

Of course you can add IOCTLs to do all the configuration of a device, but
you can end up with a nasty and inflexible interface + add a burden
on simple users of them having to use a library to unwind it all rather
than simply poking text files. 

> I think latency concerns are the same reason the GPIO subsystem started
> providing character device nodes as well. 

That was part of it, but a big element was also to provide the ability
to set+get multiple lines in parallel.  Kind of the same as you have
here with a timestamp matching to a count.  For low latency gpio on an
SoC people often just directly write the registers from a userspace
mapping.

> We can do similar with the
> Counter subsystem: provide character device nodes by default, and
> optionally provide the human-readable sysfs interface as well. This
> would allow applications with latency concerns to use a standard
> interface for the Counter subsystem, while optionally providing a
> simpler sysfs interface for other users.

I wouldn't necessarily go for making the sysfs interface optional.
It is extremely convenient for describing complex devices. It is
possible to do all that via complex IOCTL query interfaces (see
the mediabus framework for example), but it's much heavier weight.

The IIO approach was to use the chardev for fast path and sysfs
for slow.  Maybe we'd do it differently if starting now, but I'm
not sure we would end up different in this aspect.  Other things
would change though ;)


Jonathan

> 
> William Breathitt Gray

William Breathitt Gray Oct. 16, 2019, 8:20 p.m. UTC | #11

On Thu, Oct 03, 2019 at 02:14:43PM +0100, Jonathan Cameron wrote:
> On Tue, 1 Oct 2019 20:34:42 -0400
> William Breathitt Gray <[e-mail]> wrote:
> 
> > On Mon, Sep 30, 2019 at 08:22:39AM +0300, Felipe Balbi wrote:
> > > 
> > > Hi,
> > > 
> > > William Breathitt Gray <[e-mail]> writes:  
> > > > On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:  
> > > >> On 9/22/19 6:35 PM, William Breathitt Gray wrote:  
> > > >> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:  
> > > >> >> Add support for Intel PSE Quadrature Encoder
> > > >> >>
> > > >> >> Signed-off-by: Felipe Balbi <[e-mail]>
> > > >> >> ---
> > > >> >>
> > > >> >> Changes since v1:
> > > >> >> 	- Many more private sysfs files converted over to counter interface
> > > >> >>
> > > >> >>
> > > >> >> How do you want me to model this device's Capture Compare Mode (see
> > > >> >> below)?  
> > > >> > 
> > > >> > Hi Felipe,
> > > >> > 
> > > >> > I'm CCing Fabien and David as they may be interested in the timestamps
> > > >> > discussion. See below for some ideas I have on implementing this.
> > > >> >   
> > > >> 
> > > >> Could be an interesting read (thread from my first counter driver):
> > > >> 
> > > >> https://lore.kernel.org/linux-iio/[e-mail]/
> > > >> 
> > > >> What would be useful to me is something like the buffer feature in iio
> > > >> where a timestamp is associated with a count and stored in a buffer so that
> > > >> we can look at a window of all values recorded in the last 20ms. Being able
> > > >> to access this via mmap would be very helpful for performance (running on
> > > >> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
> > > >> unless it is a rare event, like a watchdog timeout.  
> > > >
> > > > I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
> > > > timestamps and buffers. I don't want to reinvent something that is
> > > > working well, so hopefully we can reuse the IIO timestamp design for the
> > > > Counter subsystem.
> 
> Simple approach, push them both into a kfifo. Userspace then reads this
> (with poll/select/blocking read + things like watermarks all available).
> The description of what is there in each record is handled via sysfs as
> the IIO model is that the format of each record only changes with userspace
> intervention.
> 
> The complexities mostly come from allowing the hardware side of what is there
> and the software side to differ, but this is only really needed if you want
> to have multiple readers and need to split up the data so each thinks it has
> the device to itself.  I would guess not needed for counter usecases, but who
> knows down the line.

Is the kfifo updating only when a user interacts with the timestamp
sysfs attributes? If so, why store the timestamps in a kfifo instead of
serving them directly on demand; or is the kfifo there to allow multiple
users to access the same timestamp fifo?

> > > >
> > > > I would argue that a human-readable timestamps printout is useful for
> > > > certain applications (e.g. a tally counter attached to a fault line: a
> > > > human administrator will be able to review previous fault times).
> > > > However as you point out, a low latency operation is necessary for
> > > > performance critical applications.
> 
> For this sort of case you could use a sysfs file that just returns the oldest
> entry when read.  A seek back to the start and reread or a reopen of the file
> can then give you the next one and an appropriate error return indicates none
> left.

The downside to this interface is that it's volatile, in the sense that
once the fifo is read you can't reread it unless you saved those
timestamps yourself. However, as you've pointed out in another email,
sysfs has a "single value" rule so returning the latest timestamp is
likely the best solution given the restriction that multiple values
cannot be returned via a single attribute.

> > > >
> > > > Although you are correct that mmap is a good low latency operation to
> > > > get access to a timestamp buffer, I'm afraid giving direct access to
> > > > memory like that will lead to many incompatible representations of
> > > > timestamp data (e.g. variations in endianness, signedness, data size,
> > > > etc.). I would like a standardized representation for this data that
> > > > userspace applications can expect to receive and interpret, especially
> > > > when time is widely represented as an unsigned integer.
> 
> It is moderately hard to get that sort of interface right, but it can be done.
> IIO does it for DMA type buffers, but not for anything where a software
> fifo is involved.   You would need to guarantee a fixed format etc.
> 
> > > >
> > > > Felipe suggested the creation of a counter_event structure so that users
> > > > can poll on an attribute. This kind of behavior is useful for notifying
> > > > users of interrupts and other events, but I think we should restrict the
> > > > use of the read call on these sysfs attributes to just human-readable
> > > > data. Instead, perhaps ioctl calls can be used to facilitate binary data
> > > > transfers.
> > > >
> > > > For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
> > > > that returns a counter_timestamps structure with a timestamps array
> > > > populated:
> > > >
> > > >         struct counter_timestamps{
> > > >                 size_t num_timestamps;
> > > >         	unsigned int *timestamps;
> > > >         }
> > > >
> > > > That would allow quick access to the timestamps data, while also
> > > > restricting it to a standard representation that all userspace
> > > > applications can follow and interpret. In addition, this won't interfer
> > > > with polling, so users can still wait for an interrupt and then decide
> > > > whether they want to use the slower human-readable printout (via read)
> > > > or the faster binary data access (via ioctl).  
> > > 
> > > Seems like we're starting to build the need for a /dev/counter[0123...]
> > > representation of the subsystem. If that's the case, then it may very
> > > well be that sysfs becomes somewhat optional.
> 
> Agreed.  Don't map this stuff onto sysfs where a chardev is more sensible.
> 
> > > 
> > > I think is makes sense to rely more on character devices specially since
> > > I know of devices running linux with so little memory that sysfs (and a
> > > bunch of other features) are removed from the kernel. Having a character
> > > device representation would allow counter subsystem to be used on such
> > > devices.
> > > 
> > > cheers
> > > 
> > > -- 
> > > balbi  
> > 
> > A character device node for a counter might be a good idea. If a
> > performance critical application can't depend on parsing a sysfs
> > printout for timestamps, then it probably doesn't want to do so for the
> > other attributes either. I think you are right that certain systems
> > would have sysfs disabled for that very reason.
> > 
> 
> I am a little curious to whether people often run sysfs free any more?
> I know it used to happen a fair bit, but a lot of the kernel is now rather
> dependent on it.
> 
> Of course you can add IOCTLs to do all the configuration of a device, but
> you can end up with a nasty and inflexible interface + add a burden
> on simple users of them having to use a library to unwind it all rather
> than simply poking text files. 
> 
> > I think latency concerns are the same reason the GPIO subsystem started
> > providing character device nodes as well. 
> 
> That was part of it, but a big element was also to provide the ability
> to set+get multiple lines in parallel.  Kind of the same as you have
> here with a timestamp matching to a count.  For low latency gpio on an
> SoC people often just directly write the registers from a userspace
> mapping.
> 
> > We can do similar with the
> > Counter subsystem: provide character device nodes by default, and
> > optionally provide the human-readable sysfs interface as well. This
> > would allow applications with latency concerns to use a standard
> > interface for the Counter subsystem, while optionally providing a
> > simpler sysfs interface for other users.
> 
> I wouldn't necessarily go for making the sysfs interface optional.
> It is extremely convenient for describing complex devices. It is
> possible to do all that via complex IOCTL query interfaces (see
> the mediabus framework for example), but it's much heavier weight.
> 
> The IIO approach was to use the chardev for fast path and sysfs
> for slow.  Maybe we'd do it differently if starting now, but I'm
> not sure we would end up different in this aspect.  Other things
> would change though ;)
> 
> 
> Jonathan
> 
> > 
> > William Breathitt Gray

I agree about the utility and convenience of sysfs; it's an interface
that is simple enough for a human to understand and provides information
in a universally parsable format: text. We can leave the sysfs interface
available by default since disabling it can be left to the power users
that have a need to do so (and flipping a Kconfig option shouldn't be
that difficult to accomplish).

Regardless, I do see the benefits of providing a chardev interface --
not just for timestamps evaluation but in other counter applications as
well, such as two-dimensional positioning tables where multiple counter
values would need to be read at relatively the same time (a situation
where multiple sysfs reads would impede precision).

My concern with this is similar: a growing list of IOCTLs that are added
with new drivers trying to expose their device-specific extensions. I
think exposing every single extension with a respective dedicated IOCTL
is not the way to go; this would quickly lead to an unmaintainable list
of IOCTLs that most users would never need

A more maintainable approach is to dedicate specialized IOCTLs only for
those features that are used by multiple counter drivers (i.e. listed in
Documentation/ABI/testing/sysfs-bus-counter); this will help curb
feature creep. The other driver-specific extensions can be accessed via
a few IOCTLs made for such interaction: perhaps one to poll for a list
of available extensions and another to read/write.

The only change required to the Counter driver API is to allow for a
more opaque extension structure. This will allow us to supply dedicated
extension callbacks to handle binary data rather than text (though in an
opaque way such as the existing counter_count_direction enum); however,
driver-specific extensions can still be handled by text for simplicity.

The main change would be in the drivers/counter/counter.c file. This can
be split into three file: counter-core, counter-sysfs, counter-chardev.
The counter-core file would provide the core Counter functionality and
handle the driver API; the counter-sysfs and counter-chardev files can
expose the Counter subsystem to userspace in their respective formats.

William Breathitt Gray

Jonathan Cameron Oct. 17, 2019, 12:29 p.m. UTC | #12

On Wed, 16 Oct 2019 16:20:35 -0400
William Breathitt Gray <[e-mail]> wrote:

> On Thu, Oct 03, 2019 at 02:14:43PM +0100, Jonathan Cameron wrote:
> > On Tue, 1 Oct 2019 20:34:42 -0400
> > William Breathitt Gray <[e-mail]> wrote:
> >   
> > > On Mon, Sep 30, 2019 at 08:22:39AM +0300, Felipe Balbi wrote:  
> > > > 
> > > > Hi,
> > > > 
> > > > William Breathitt Gray <[e-mail]> writes:    
> > > > > On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:    
> > > > >> On 9/22/19 6:35 PM, William Breathitt Gray wrote:    
> > > > >> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:    
> > > > >> >> Add support for Intel PSE Quadrature Encoder
> > > > >> >>
> > > > >> >> Signed-off-by: Felipe Balbi <[e-mail]>
> > > > >> >> ---
> > > > >> >>
> > > > >> >> Changes since v1:
> > > > >> >> 	- Many more private sysfs files converted over to counter interface
> > > > >> >>
> > > > >> >>
> > > > >> >> How do you want me to model this device's Capture Compare Mode (see
> > > > >> >> below)?    
> > > > >> > 
> > > > >> > Hi Felipe,
> > > > >> > 
> > > > >> > I'm CCing Fabien and David as they may be interested in the timestamps
> > > > >> > discussion. See below for some ideas I have on implementing this.
> > > > >> >     
> > > > >> 
> > > > >> Could be an interesting read (thread from my first counter driver):
> > > > >> 
> > > > >> https://lore.kernel.org/linux-iio/[e-mail]/
> > > > >> 
> > > > >> What would be useful to me is something like the buffer feature in iio
> > > > >> where a timestamp is associated with a count and stored in a buffer so that
> > > > >> we can look at a window of all values recorded in the last 20ms. Being able
> > > > >> to access this via mmap would be very helpful for performance (running on
> > > > >> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
> > > > >> unless it is a rare event, like a watchdog timeout.    
> > > > >
> > > > > I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
> > > > > timestamps and buffers. I don't want to reinvent something that is
> > > > > working well, so hopefully we can reuse the IIO timestamp design for the
> > > > > Counter subsystem.  
> > 
> > Simple approach, push them both into a kfifo. Userspace then reads this
> > (with poll/select/blocking read + things like watermarks all available).
> > The description of what is there in each record is handled via sysfs as
> > the IIO model is that the format of each record only changes with userspace
> > intervention.
> > 
> > The complexities mostly come from allowing the hardware side of what is there
> > and the software side to differ, but this is only really needed if you want
> > to have multiple readers and need to split up the data so each thinks it has
> > the device to itself.  I would guess not needed for counter usecases, but who
> > knows down the line.  
> 
> Is the kfifo updating only when a user interacts with the timestamp
> sysfs attributes? If so, why store the timestamps in a kfifo instead of
> serving them directly on demand; or is the kfifo there to allow multiple
> users to access the same timestamp fifo?

I'm confused here.  We have to update 'what' is stored in the kfifo when
a change is made to what userspace wants us to store there.

The kfifo is because userspace doesn't want to have to constantly read
data to avoid loosing it.  A count + timestamp is grabbed based either on
the something from the hardware, or another in kernel source of "grab data now".
The kernel pushes that into the kfifo.  Userspace then reads out when it
is ready.

Multiple users would require multiple kfifos. Which is what evdev does IIRC.

The logic to fill those multiple kfifos gets complex if different users have
asked for different things to be stored.  Eg. one wants timestamps and position,
the other just the timestamps (odd case, but seems people do want to do that!).
Then you have to split the data up appropriately before pushing it to the buffers.

> 
> > > > >
> > > > > I would argue that a human-readable timestamps printout is useful for
> > > > > certain applications (e.g. a tally counter attached to a fault line: a
> > > > > human administrator will be able to review previous fault times).
> > > > > However as you point out, a low latency operation is necessary for
> > > > > performance critical applications.  
> > 
> > For this sort of case you could use a sysfs file that just returns the oldest
> > entry when read.  A seek back to the start and reread or a reopen of the file
> > can then give you the next one and an appropriate error return indicates none
> > left.  
> 
> The downside to this interface is that it's volatile, in the sense that
> once the fifo is read you can't reread it unless you saved those
> timestamps yourself. However, as you've pointed out in another email,
> sysfs has a "single value" rule so returning the latest timestamp is
> likely the best solution given the restriction that multiple values
> cannot be returned via a single attribute.

Even a sysfs interface would need to be effectively volatile.  Might require
an explicit 'clear' signal, but those are racy to deal with.  If not volatile
it would rapidly become very large.

> 
> > > > >
> > > > > Although you are correct that mmap is a good low latency operation to
> > > > > get access to a timestamp buffer, I'm afraid giving direct access to
> > > > > memory like that will lead to many incompatible representations of
> > > > > timestamp data (e.g. variations in endianness, signedness, data size,
> > > > > etc.). I would like a standardized representation for this data that
> > > > > userspace applications can expect to receive and interpret, especially
> > > > > when time is widely represented as an unsigned integer.  
> > 
> > It is moderately hard to get that sort of interface right, but it can be done.
> > IIO does it for DMA type buffers, but not for anything where a software
> > fifo is involved.   You would need to guarantee a fixed format etc.
> >   
> > > > >
> > > > > Felipe suggested the creation of a counter_event structure so that users
> > > > > can poll on an attribute. This kind of behavior is useful for notifying
> > > > > users of interrupts and other events, but I think we should restrict the
> > > > > use of the read call on these sysfs attributes to just human-readable
> > > > > data. Instead, perhaps ioctl calls can be used to facilitate binary data
> > > > > transfers.
> > > > >
> > > > > For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
> > > > > that returns a counter_timestamps structure with a timestamps array
> > > > > populated:
> > > > >
> > > > >         struct counter_timestamps{
> > > > >                 size_t num_timestamps;
> > > > >         	unsigned int *timestamps;
> > > > >         }
> > > > >
> > > > > That would allow quick access to the timestamps data, while also
> > > > > restricting it to a standard representation that all userspace
> > > > > applications can follow and interpret. In addition, this won't interfer
> > > > > with polling, so users can still wait for an interrupt and then decide
> > > > > whether they want to use the slower human-readable printout (via read)
> > > > > or the faster binary data access (via ioctl).    
> > > > 
> > > > Seems like we're starting to build the need for a /dev/counter[0123...]
> > > > representation of the subsystem. If that's the case, then it may very
> > > > well be that sysfs becomes somewhat optional.  
> > 
> > Agreed.  Don't map this stuff onto sysfs where a chardev is more sensible.
> >   
> > > > 
> > > > I think is makes sense to rely more on character devices specially since
> > > > I know of devices running linux with so little memory that sysfs (and a
> > > > bunch of other features) are removed from the kernel. Having a character
> > > > device representation would allow counter subsystem to be used on such
> > > > devices.
> > > > 
> > > > cheers
> > > > 
> > > > -- 
> > > > balbi    
> > > 
> > > A character device node for a counter might be a good idea. If a
> > > performance critical application can't depend on parsing a sysfs
> > > printout for timestamps, then it probably doesn't want to do so for the
> > > other attributes either. I think you are right that certain systems
> > > would have sysfs disabled for that very reason.
> > >   
> > 
> > I am a little curious to whether people often run sysfs free any more?
> > I know it used to happen a fair bit, but a lot of the kernel is now rather
> > dependent on it.
> > 
> > Of course you can add IOCTLs to do all the configuration of a device, but
> > you can end up with a nasty and inflexible interface + add a burden
> > on simple users of them having to use a library to unwind it all rather
> > than simply poking text files. 
> >   
> > > I think latency concerns are the same reason the GPIO subsystem started
> > > providing character device nodes as well.   
> > 
> > That was part of it, but a big element was also to provide the ability
> > to set+get multiple lines in parallel.  Kind of the same as you have
> > here with a timestamp matching to a count.  For low latency gpio on an
> > SoC people often just directly write the registers from a userspace
> > mapping.
> >   
> > > We can do similar with the
> > > Counter subsystem: provide character device nodes by default, and
> > > optionally provide the human-readable sysfs interface as well. This
> > > would allow applications with latency concerns to use a standard
> > > interface for the Counter subsystem, while optionally providing a
> > > simpler sysfs interface for other users.  
> > 
> > I wouldn't necessarily go for making the sysfs interface optional.
> > It is extremely convenient for describing complex devices. It is
> > possible to do all that via complex IOCTL query interfaces (see
> > the mediabus framework for example), but it's much heavier weight.
> > 
> > The IIO approach was to use the chardev for fast path and sysfs
> > for slow.  Maybe we'd do it differently if starting now, but I'm
> > not sure we would end up different in this aspect.  Other things
> > would change though ;)
> > 
> > 
> > Jonathan
> >   
> > > 
> > > William Breathitt Gray  
> 
> I agree about the utility and convenience of sysfs; it's an interface
> that is simple enough for a human to understand and provides information
> in a universally parsable format: text. We can leave the sysfs interface
> available by default since disabling it can be left to the power users
> that have a need to do so (and flipping a Kconfig option shouldn't be
> that difficult to accomplish).
> 
> Regardless, I do see the benefits of providing a chardev interface --
> not just for timestamps evaluation but in other counter applications as
> well, such as two-dimensional positioning tables where multiple counter
> values would need to be read at relatively the same time (a situation
> where multiple sysfs reads would impede precision).

Thats one of the main reasons we have this in IIO as well.

> 
> My concern with this is similar: a growing list of IOCTLs that are added
> with new drivers trying to expose their device-specific extensions. I
> think exposing every single extension with a respective dedicated IOCTL
> is not the way to go; this would quickly lead to an unmaintainable list
> of IOCTLs that most users would never need

Yes. If you are going to do that, it needs to be very carefully controlled.
Assumption is that a new driver almost never adds a new IOCTL.

> 
> A more maintainable approach is to dedicate specialized IOCTLs only for
> those features that are used by multiple counter drivers (i.e. listed in
> Documentation/ABI/testing/sysfs-bus-counter); this will help curb
> feature creep. The other driver-specific extensions can be accessed via
> a few IOCTLs made for such interaction: perhaps one to poll for a list
> of available extensions and another to read/write.
> 
> The only change required to the Counter driver API is to allow for a
> more opaque extension structure. This will allow us to supply dedicated
> extension callbacks to handle binary data rather than text (though in an
> opaque way such as the existing counter_count_direction enum); however,
> driver-specific extensions can still be handled by text for simplicity.

I'd push back as much as possible on any opaque structures.  They don't
work for generic userspace.   This isn't an area where things are normally
complex enough that we need a userspace driver component for most devices.

> 
> The main change would be in the drivers/counter/counter.c file. This can
> be split into three file: counter-core, counter-sysfs, counter-chardev.
> The counter-core file would provide the core Counter functionality and
> handle the driver API; the counter-sysfs and counter-chardev files can
> expose the Counter subsystem to userspace in their respective formats.

That would be easy enough.  The chardev control interface will need a lot
of careful thought though.  It is very easy to end up with something
inconsistent or impossible to extend.

Good luck ;)

Jonathan


> 
> William Breathitt Gray

Patch

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2967d0a9ff91..f280cd721350 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -59,4 +59,10 @@  config FTM_QUADDEC
 	  To compile this driver as a module, choose M here: the
 	  module will be called ftm-quaddec.
 
+config INTEL_QEP
+	tristate "Intel Quadrature Encoder"
+	depends on PCI
+	help
+	  Support for Intel Quadrature Encoder Devices
+
 endif # COUNTER
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 40d35522937d..cf291cfd8cf0 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -9,3 +9,4 @@  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
 obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
+obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
new file mode 100644
index 000000000000..8347f9fa8e37
--- /dev/null
+++ b/drivers/counter/intel-qep.c
@@ -0,0 +1,862 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * intel-qep.c - Intel Quadrature Encoder Driver
+ *
+ * Copyright (C) 2019 Intel Corporation - https://www.intel.com
+ *
+ * Author: Felipe Balbi <[e-mail]>
+ */
+#include <linux/bitops.h>
+#include <linux/counter.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/sysfs.h>
+
+#define INTEL_QEPCON		0x00
+#define INTEL_QEPFLT		0x04
+#define INTEL_QEPCOUNT		0x08
+#define INTEL_QEPMAX		0x0c
+#define INTEL_QEPWDT		0x10
+#define INTEL_QEPCAPDIV		0x14
+#define INTEL_QEPCNTR		0x18
+#define INTEL_QEPCAPBUF		0x1c
+#define INTEL_QEPINT_STAT	0x20
+#define INTEL_QEPINT_MASK	0x24
+
+/* QEPCON */
+#define INTEL_QEPCON_EN		BIT(0)
+#define INTEL_QEPCON_FLT_EN	BIT(1)
+#define INTEL_QEPCON_EDGE_A	BIT(2)
+#define INTEL_QEPCON_EDGE_B	BIT(3)
+#define INTEL_QEPCON_EDGE_INDX	BIT(4)
+#define INTEL_QEPCON_SWPAB	BIT(5)
+#define INTEL_QEPCON_OP_MODE	BIT(6)
+#define INTEL_QEPCON_PH_ERR	BIT(7)
+#define INTEL_QEPCON_COUNT_RST_MODE BIT(8)
+#define INTEL_QEPCON_INDX_GATING_MASK GENMASK(10, 9)
+#define INTEL_QEPCON_INDX_GATING(n) (((n) & 3) << 9)
+#define INTEL_QEPCON_INDX_PAL_PBL INTEL_QEPCON_INDX_GATING(0)
+#define INTEL_QEPCON_INDX_PAL_PBH INTEL_QEPCON_INDX_GATING(1)
+#define INTEL_QEPCON_INDX_PAH_PBL INTEL_QEPCON_INDX_GATING(2)
+#define INTEL_QEPCON_INDX_PAH_PBH INTEL_QEPCON_INDX_GATING(3)
+#define INTEL_QEPCON_CAP_MODE	BIT(11)
+#define INTEL_QEPCON_FIFO_THRE_MASK GENMASK(14, 12)
+#define INTEL_QEPCON_FIFO_THRE(n) ((((n) - 1) & 7) << 12)
+#define INTEL_QEPCON_FIFO_EMPTY	BIT(15)
+
+/* QEPFLT */
+#define INTEL_QEPFLT_MAX_COUNT(n) ((n) & 0x1fffff)
+
+/* QEPINT */
+#define INTEL_QEPINT_FIFOCRIT	BIT(5)
+#define INTEL_QEPINT_FIFOENTRY	BIT(4)
+#define INTEL_QEPINT_QEPDIR	BIT(3)
+#define INTEL_QEPINT_QEPRST_UP	BIT(2)
+#define INTEL_QEPINT_QEPRST_DOWN BIT(1)
+#define INTEL_QEPINT_WDT	BIT(0)
+
+#define INTEL_QEP_DIRECTION_FORWARD 1
+#define INTEL_QEP_DIRECTION_BACKWARD !INTEL_QEP_DIRECTION_FORWARD
+
+#define INTEL_QEP_COUNTER_EXT_RW(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+	.write = _name##_write, \
+}
+
+#define INTEL_QEP_COUNTER_EXT_RO(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+}
+
+#define INTEL_QEP_COUNTER_COUNT_EXT_RW(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+	.write = _name##_write, \
+}
+
+#define INTEL_QEP_COUNTER_COUNT_EXT_RO(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+}
+
+struct intel_qep {
+	struct counter_device counter;
+	struct mutex lock;
+	struct pci_dev *pci;
+	struct device *dev;
+	void __iomem *regs;
+	u32 interrupt;
+	int direction;
+	bool enabled;
+};
+
+#define counter_to_qep(c)	(container_of((c), struct intel_qep, counter))
+
+static inline u32 intel_qep_readl(void __iomem *base, u32 offset)
+{
+	return readl(base + offset);
+}
+
+static inline void intel_qep_writel(void __iomem *base, u32 offset, u32 value)
+{
+	writel(value, base + offset);
+}
+
+static ssize_t phase_error_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_PH_ERR ? "error" : "okay");
+}
+static DEVICE_ATTR_RO(phase_error);
+
+static ssize_t fifo_empty_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_FIFO_EMPTY ? "empty" : "not empty");
+}
+static DEVICE_ATTR_RO(fifo_empty);
+
+static ssize_t operating_mode_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_OP_MODE ? "capture" : "quadrature");
+}
+
+static ssize_t operating_mode_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (sysfs_streq(buf, "capture"))
+		reg |= INTEL_QEPCON_OP_MODE;
+	else if (sysfs_streq(buf, "quadrature"))
+		reg &= ~INTEL_QEPCON_OP_MODE;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return count;
+}
+static DEVICE_ATTR_RW(operating_mode);
+
+static ssize_t capture_mode_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_CAP_MODE ? "both" : "single");
+}
+
+static ssize_t capture_mode_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (sysfs_streq(buf, "both"))
+		reg |= INTEL_QEPCON_CAP_MODE;
+	else if (sysfs_streq(buf, "single"))
+		reg &= ~INTEL_QEPCON_CAP_MODE;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return count;
+}
+static DEVICE_ATTR_RW(capture_mode);
+
+static const struct attribute *intel_qep_attrs[] = {
+	&dev_attr_capture_mode.attr,
+	&dev_attr_fifo_empty.attr,
+	&dev_attr_operating_mode.attr,
+	&dev_attr_phase_error.attr,
+	NULL	/* Terminating Entry */
+};
+
+static ssize_t capture_data_read(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *attr, char *buf,
+		loff_t offset, size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+	int i;
+
+	mutex_lock(&qep->lock);
+	for (i = 0; i < count; i += 4) {
+		reg = intel_qep_readl(qep->regs, INTEL_QEPCAPBUF);
+
+		buf[i + 0] = reg & 0xff;
+		buf[i + 1] = (reg >> 8) & 0xff;
+		buf[i + 2] = (reg >> 16) & 0xff;
+		buf[i + 3] = (reg >> 24) & 0xff;
+	}
+	mutex_unlock(&qep->lock);
+
+	return count;
+}
+
+static BIN_ATTR_RO(capture_data, 4);
+
+static struct bin_attribute *intel_qep_bin_attrs[] = {
+	&bin_attr_capture_data,
+	NULL	/* Terminating Entry */
+};
+
+static const struct attribute_group intel_qep_device_aattr_group = {
+	.name = "qep",
+	.attrs = (struct attribute **) intel_qep_attrs,
+	.bin_attrs = intel_qep_bin_attrs,
+};
+
+static const struct pci_device_id intel_qep_id_table[] = {
+	/* EHL */
+	{ PCI_VDEVICE(INTEL, 0x4bc3), },
+	{ PCI_VDEVICE(INTEL, 0x4b81), },
+	{ PCI_VDEVICE(INTEL, 0x4b82), },
+	{ PCI_VDEVICE(INTEL, 0x4b83), },
+	{  } /* Terminating Entry */
+};
+MODULE_DEVICE_TABLE(pci, intel_qep_id_table);
+
+static void intel_qep_init(struct intel_qep *qep, bool reset)
+{
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	reg &= ~INTEL_QEPCON_EN;
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	/* make sure periperal is disabled by reading one more time */
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (reset) {
+		reg &= ~(INTEL_QEPCON_OP_MODE | INTEL_QEPCON_FLT_EN);
+		reg |= INTEL_QEPCON_EDGE_A | INTEL_QEPCON_EDGE_B |
+			INTEL_QEPCON_EDGE_INDX | INTEL_QEPCON_COUNT_RST_MODE;
+	}
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	intel_qep_writel(qep->regs, INTEL_QEPWDT, 0x1000);
+	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x0);
+
+	qep->direction = INTEL_QEP_DIRECTION_FORWARD;
+}
+
+static irqreturn_t intel_qep_irq_thread(int irq, void *_qep)
+{
+	struct intel_qep	*qep = _qep;
+	u32			stat;
+
+	mutex_lock(&qep->lock);
+
+	stat = qep->interrupt;
+	if (stat & INTEL_QEPINT_FIFOCRIT)
+		sysfs_notify(&qep->dev->kobj, "qep", "capture_buffer");
+
+	if (stat & INTEL_QEPINT_FIFOENTRY)
+		sysfs_notify(&qep->dev->kobj, "qep", "capture_buffer");
+
+	if (stat & INTEL_QEPINT_QEPDIR)
+		qep->direction = !qep->direction;
+
+	if (stat & INTEL_QEPINT_QEPRST_UP)
+		qep->direction = INTEL_QEP_DIRECTION_FORWARD;
+
+	if (stat & INTEL_QEPINT_QEPRST_DOWN)
+		qep->direction = INTEL_QEP_DIRECTION_BACKWARD;
+
+	if (stat & INTEL_QEPINT_WDT)
+		dev_dbg(qep->dev, "Watchdog\n");
+
+	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x00);
+	mutex_unlock(&qep->lock);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t intel_qep_irq(int irq, void *_qep)
+{
+	struct intel_qep	*qep = _qep;
+	u32			stat;
+
+	stat = intel_qep_readl(qep->regs, INTEL_QEPINT_STAT);
+	if (stat) {
+		qep->interrupt = stat;
+		intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0xffffffff);
+		intel_qep_writel(qep->regs, INTEL_QEPINT_STAT, stat);
+		return IRQ_WAKE_THREAD;
+	}
+
+	return IRQ_HANDLED;
+}
+
+enum intel_qep_synapse_action {
+	INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE,
+	INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE,
+};
+
+static enum counter_synapse_action intel_qep_synapse_actions[] = {
+	[INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE] =
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+	
+	[INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE] =
+	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
+};
+
+enum intel_qep_count_function {
+	INTEL_QEP_ENCODER_MODE_NORMAL,
+	INTEL_QEP_ENCODER_MODE_SWAPPED,
+};
+
+static const enum counter_count_function intel_qep_count_functions[] = {
+	[INTEL_QEP_ENCODER_MODE_NORMAL] =
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
+
+	[INTEL_QEP_ENCODER_MODE_SWAPPED] =
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED,
+};
+
+static int intel_qep_count_read(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_count_read_value *val)
+{
+	struct intel_qep *const qep = counter->priv;
+	uint32_t cntval;
+
+	cntval = intel_qep_readl(qep, INTEL_QEPCOUNT);
+	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cntval);
+
+	return 0;
+}
+
+static int intel_qep_count_write(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_count_write_value *val)
+{
+	struct intel_qep *const qep = counter->priv;
+	u32 cnt;
+	int err;
+
+	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
+	if (err)
+		return err;
+
+	intel_qep_writel(qep->regs, INTEL_QEPMAX, cnt);
+
+	return 0;
+}
+
+static int intel_qep_function_get(struct counter_device *counter,
+		struct counter_count *count, size_t *function)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	if (req & INTEL_QEPCON_SWPAB)
+		*function = INTEL_QEP_ENCODER_MODE_SWAPPED;
+	else
+		*function = INTEL_QEP_ENCODER_MODE_NORMAL;
+
+	return 0;
+}
+
+static int intel_qep_function_set(struct counter_device *counter,
+		struct counter_count *count, size_t *function)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	if (*function == INTEL_QEP_ENCODER_MODE_SWAPPED)
+		reg |= INTEL_QEPCON_SWPAB;
+	else
+		reg &= ~INTEL_QEPCON_SWPAB;
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return 0;
+}
+
+static int intel_qep_action_get(struct counter_device *counter,
+		struct counter_count *count, struct counter_synapse *synapse,
+		size_t *action)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	*action = reg & synapse->signal->id ?
+		INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE :
+		INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE;
+
+	return 0;
+}
+
+static int intel_qep_action_set(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_synapse *synapse, size_t action)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (action == INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE)
+		reg |= synapse->signal->id;
+	else
+		reg &= ~synapse->signal->id;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return 0;
+}
+
+static const struct counter_ops intel_qep_counter_ops = {
+	.count_read = intel_qep_count_read,
+	.count_write = intel_qep_count_write,
+
+	.function_get = intel_qep_function_get,
+	.function_set = intel_qep_function_set,
+
+	.action_get = intel_qep_action_get,
+	.action_set = intel_qep_action_set,
+};
+
+static struct counter_signal intel_qep_signals[] = {
+	{
+		.id = INTEL_QEPCON_EDGE_A,
+		.name = "Phase A",
+	},
+	{
+		.id = INTEL_QEPCON_EDGE_B,
+		.name = "Phase B",
+	},
+	{
+		.id = INTEL_QEPCON_EDGE_INDX,
+		.name = "Index",
+	},
+};
+
+static struct counter_synapse intel_qep_count_synapses[] = {
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[0],
+	},
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[1],
+	},
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[2],
+	},
+};
+
+static const char * const intel_qep_clock_prescalers[] = {
+	"1", "2", "4", "8", "16", "32", "64", "128"
+};
+
+static int intel_qep_clock_prescaler_get(struct counter_device *counter,
+		struct counter_count *count, size_t *mode)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	
+	*mode = intel_qep_readl(qep->regs, INTEL_QEPCAPDIV);
+
+	return 0;
+}
+
+static int intel_qep_clock_prescaler_set(struct counter_device *counter,
+		struct counter_count *count, size_t mode)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+
+	intel_qep_writel(qep->regs, INTEL_QEPCAPDIV, ffs(mode) - 1);
+
+	return 0;
+}
+
+static struct counter_count_enum_ext intel_qep_clock_prescaler_enum = {
+	.items = intel_qep_clock_prescalers,
+	.num_items = ARRAY_SIZE(intel_qep_clock_prescalers),
+	.get = intel_qep_clock_prescaler_get,
+	.set = intel_qep_clock_prescaler_set,
+};
+
+static ssize_t ceiling_read(struct counter_device *counter,
+		struct counter_count *count, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPMAX);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", reg);
+}
+
+static ssize_t ceiling_write(struct counter_device *counter,
+		struct counter_count *count, void *priv, const char *buf,
+		size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 max;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &max);
+	if (ret < 0)
+		return ret;
+
+	intel_qep_writel(qep->regs, INTEL_QEPMAX, max);
+
+	return len;
+}
+
+static ssize_t enable_read(struct counter_device *counter,
+		struct counter_count *count, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", !!(reg & INTEL_QEPCON_EN));
+}
+
+static ssize_t enable_write(struct counter_device *counter,
+		struct counter_count *count, void *priv, const char *buf,
+		size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+	u32 val;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (val)
+		reg |= INTEL_QEPCON_EN;
+	else
+		reg &= ~INTEL_QEPCON_EN;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return len;
+}
+
+static ssize_t direction_read(struct counter_device *counter,
+		struct counter_count *count, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", qep->direction ?
+			"forward" : "backward");
+}
+
+static const struct counter_count_ext intel_qep_count_ext[] = {
+	COUNTER_COUNT_ENUM("prescaler", &intel_qep_clock_prescaler_enum),
+	COUNTER_COUNT_ENUM_AVAILABLE("prescaler",
+			&intel_qep_clock_prescaler_enum),
+
+	INTEL_QEP_COUNTER_COUNT_EXT_RW(ceiling),
+	INTEL_QEP_COUNTER_COUNT_EXT_RW(enable),
+	INTEL_QEP_COUNTER_COUNT_EXT_RO(direction),
+};
+
+static struct counter_count intel_qep_counter_count[] = {
+	{
+		.id = 0,
+		.name = "Channel 1 Count",
+		.functions_list = intel_qep_count_functions,
+		.num_functions = ARRAY_SIZE(intel_qep_count_functions),
+		.synapses = intel_qep_count_synapses,
+		.num_synapses = ARRAY_SIZE(intel_qep_count_synapses),
+		.ext = intel_qep_count_ext,
+		.num_ext = ARRAY_SIZE(intel_qep_count_ext),
+	},
+};
+
+static ssize_t noise_read(struct counter_device *counter, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (!(reg & INTEL_QEPCON_FLT_EN))
+		return snprintf(buf, PAGE_SIZE, "0\n");
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPFLT);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", INTEL_QEPFLT_MAX_COUNT(reg));
+}
+
+static ssize_t noise_write(struct counter_device *counter, void *priv,
+		const char *buf, size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+	u32 max;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &max);
+	if (ret < 0)
+		return ret;
+
+	if (max > 0x1fffff)
+		max = 0x1ffff;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (max == 0) {
+		reg &= ~INTEL_QEPCON_FLT_EN;
+	} else {
+		reg |= INTEL_QEPCON_FLT_EN;
+		intel_qep_writel(qep->regs, INTEL_QEPFLT, max);
+	}
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+	
+	return len;
+}
+
+static ssize_t preset_read(struct counter_device *counter, void *priv, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0\n");
+}
+
+static ssize_t preset_enable_read(struct counter_device *counter, void *priv,
+		char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			!(reg & INTEL_QEPCON_COUNT_RST_MODE));
+}
+
+static ssize_t preset_enable_write(struct counter_device *counter, void *priv,
+		const char *buf, size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+	u32 val;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (val)
+		reg &= ~INTEL_QEPCON_COUNT_RST_MODE;
+	else
+		reg |= INTEL_QEPCON_COUNT_RST_MODE;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+	
+	return len;
+}
+
+static const struct counter_device_ext intel_qep_ext[] = {
+	INTEL_QEP_COUNTER_EXT_RW(noise),
+	INTEL_QEP_COUNTER_EXT_RO(preset),
+	INTEL_QEP_COUNTER_EXT_RW(preset_enable)
+};
+
+static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	struct intel_qep	*qep;
+	struct device		*dev = &pci->dev;
+	void __iomem		*regs;
+	int			ret;
+	int			irq;
+
+	qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
+	if (!qep)
+		return -ENOMEM;
+
+	ret = pcim_enable_device(pci);
+	if (ret)
+		return ret;
+
+	pci_set_master(pci);
+
+	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
+	if (ret)
+		return ret;
+
+	regs = pcim_iomap_table(pci)[0];
+	if (!regs)
+		return -ENOMEM;
+
+	qep->pci = pci;
+	qep->dev = dev;
+	qep->regs = regs;
+	mutex_init(&qep->lock);
+
+	intel_qep_init(qep, true);
+	pci_set_drvdata(pci, qep);
+
+	qep->counter.name = pci_name(pci);
+	qep->counter.parent = dev;
+	qep->counter.ops = &intel_qep_counter_ops;
+	qep->counter.counts = intel_qep_counter_count;
+	qep->counter.num_counts = ARRAY_SIZE(intel_qep_counter_count);
+	qep->counter.signals = intel_qep_signals;
+	qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
+	qep->counter.ext = intel_qep_ext;
+	qep->counter.num_ext = ARRAY_SIZE(intel_qep_ext);
+	qep->counter.priv = qep;
+
+	ret = counter_register(&qep->counter);
+	if (ret)
+		return ret;
+
+	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		goto err_irq_vectors;
+
+	irq = pci_irq_vector(pci, 0);
+	ret = devm_request_threaded_irq(&pci->dev, irq, intel_qep_irq,
+			intel_qep_irq_thread, IRQF_SHARED | IRQF_TRIGGER_RISING,
+			"intel-qep", qep);
+	if (ret)
+		goto err_irq;
+
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_allow(dev);
+
+	return 0;
+
+err_irq:
+	pci_free_irq_vectors(pci);
+
+err_irq_vectors:
+	counter_unregister(&qep->counter);
+
+	return ret;
+}
+
+static void intel_qep_remove(struct pci_dev *pci)
+{
+	struct intel_qep	*qep = pci_get_drvdata(pci);
+	struct device		*dev = &pci->dev;
+
+	pm_runtime_forbid(dev);
+	pm_runtime_get_noresume(dev);
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, 0);
+	pci_free_irq_vectors(pci);
+	counter_unregister(&qep->counter);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int intel_qep_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int intel_qep_resume(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct intel_qep *qep = pci_get_drvdata(pdev);
+
+	intel_qep_init(qep, false);
+
+	return 0;
+}
+
+static int intel_qep_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int intel_qep_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct intel_qep *qep = pci_get_drvdata(pdev);
+
+	intel_qep_init(qep, false);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops intel_qep_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(intel_qep_suspend,
+				intel_qep_resume)
+	SET_RUNTIME_PM_OPS(intel_qep_runtime_suspend, intel_qep_runtime_resume,
+				NULL)
+};
+
+static struct pci_driver intel_qep_driver = {
+	.name		= "intel-qep",
+	.id_table	= intel_qep_id_table,
+	.probe		= intel_qep_probe,
+	.remove		= intel_qep_remove,
+	.driver = {
+		.pm = &intel_qep_pm_ops,
+	}
+};
+
+module_pci_driver(intel_qep_driver);
+
+MODULE_AUTHOR("Felipe Balbi <[e-mail]>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel Quadrature Encoder Driver");