[PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[Biju Das]

Add RZ/G2L MTU3 counter driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v3:
 * Modelled as a counter device supporting 3 counters(2 16-bit and 
   32-bit)
 * Add kernel-doc comments to document struct rz_mtu3_cnt
 * Removed mmio variable from struct rz_mtu3_cnt
 * Removed cnt local variable from rz_mtu3_count_read()
 * Replaced -EINVAL->-ERANGE for out of range error conditions.
 * Removed explicit cast from write functions.
 * Removed local variable val from rz_mtu3_count_ceiling_read()
 * Added lock for RMW for counter/ceiling updates.
 * Added different synapses for counter0 and counter{1,2}
 * Used ARRAY for assigning num_counts.
 * Added PM runtime for managing clocks.
 * Add MODULE_IMPORT_NS(COUNTER) to import the COUNTER namespace.
---
 drivers/counter/Kconfig       |   9 +
 drivers/counter/Makefile      |   1 +
 drivers/counter/rz-mtu3-cnt.c | 568 ++++++++++++++++++++++++++++++++++
 3 files changed, 578 insertions(+)
 create mode 100644 drivers/counter/rz-mtu3-cnt.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index d388bf26f4dc..531b187e4630 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -39,6 +39,15 @@ config INTERRUPT_CNT
 	  To compile this driver as a module, choose M here: the
 	  module will be called interrupt-cnt.
 
+config RZ_MTU3_CNT
+	tristate "RZ/G2L MTU3 counter driver"
+	depends on MFD_RZ_MTU3 || COMPILE_TEST
+	help
+	  Select this option to enable RZ/G2L MTU3 counter driver.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called rz-mtu3-cnt.
+
 config STM32_TIMER_CNT
 	tristate "STM32 Timer encoder counter driver"
 	depends on MFD_STM32_TIMERS || COMPILE_TEST
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index b9a369e0d4fc..933fdd50b3e4 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -8,6 +8,7 @@ counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
 
 obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_INTERRUPT_CNT)		+= interrupt-cnt.o
+obj-$(CONFIG_RZ_MTU3_CNT)	+= rz-mtu3-cnt.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
 obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c
new file mode 100644
index 000000000000..26b5ea3852f8
--- /dev/null
+++ b/drivers/counter/rz-mtu3-cnt.c
@@ -0,0 +1,568 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L MTU3a Counter driver
+ *
+ * Copyright (C) 2022 Renesas Electronics Corporation
+ */
+#include <linux/counter.h>
+#include <linux/mfd/rz-mtu3.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/types.h>
+
+#define RZ_MTU3_TSR_TCFD	BIT(7)
+#define RZ_MTU3_MAX_HW_CNTR_CHANNELS	(2)
+
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_1	(4)
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_2	(5)
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_3	(6)
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_4	(7)
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_5	(9)
+#define RZ_MTU3_TMDR1_PH_CNT_MODE_MASK	(0xf)
+
+#define RZ_MTU3_TCR_CCLR	GENMASK(7, 5)
+#define RZ_MTU3_TCR_CCLR_NONE	FIELD_PREP(RZ_MTU3_TCR_CCLR, 0)
+
+#define RZ_MTU3_TMDR3_LWA	BIT(0)
+#define RZ_MTU3_32_BIT_CH	(2)
+
+#define RZ_MTU3_TIOR_IC_BOTH	(10)
+
+/**
+ * struct rz_mtu3_cnt - MTU3 counter private data
+ *
+ * @clk: MTU3 module clock
+ * @lock: Lock to prevent concurrent access for ceiling and count
+ * @rz_mtu3_channel: HW channels for the counters
+ */
+struct rz_mtu3_cnt {
+	struct clk *clk;
+	struct mutex lock;
+	struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
+};
+
+static const enum counter_function rz_mtu3_count_functions[] = {
+	COUNTER_FUNCTION_QUADRATURE_X4,
+	COUNTER_FUNCTION_PULSE_DIRECTION,
+	COUNTER_FUNCTION_QUADRATURE_X2_B,
+};
+
+static bool rz_mtu3_is_16_bit_cnt_mode(struct rz_mtu3_cnt *const priv)
+{
+	return (priv->ch[0]->function == RZ_MTU3_16BIT_PHASE_COUNTING ||
+		priv->ch[1]->function == RZ_MTU3_16BIT_PHASE_COUNTING);
+}
+
+static bool rz_mtu3_is_32_bit_cnt_mode(struct rz_mtu3_cnt *const priv)
+{
+	return (priv->ch[0]->function == RZ_MTU3_32BIT_PHASE_COUNTING &&
+		priv->ch[1]->function == RZ_MTU3_32BIT_PHASE_COUNTING);
+}
+
+static int rz_mtu3_count_read(struct counter_device *counter,
+			      struct counter_count *count, u64 *val)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u32 id = count->id & 1;
+
+	if (count->id == RZ_MTU3_32_BIT_CH)
+		*val = rz_mtu3_32bit_ch_read(priv->ch[id], RZ_MTU3_TCNTLW);
+	else
+		*val = rz_mtu3_16bit_ch_read(priv->ch[id], RZ_MTU3_TCNT);
+
+	return 0;
+}
+
+static int rz_mtu3_count_write(struct counter_device *counter,
+			       struct counter_count *count, const u64 val)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u32 id = count->id & 1;
+	u32 ceiling;
+
+	mutex_lock(&priv->lock);
+	if (count->id == RZ_MTU3_32_BIT_CH)
+		ceiling = rz_mtu3_32bit_ch_read(priv->ch[id], RZ_MTU3_TGRALW);
+	else
+		ceiling = rz_mtu3_16bit_ch_read(priv->ch[id], RZ_MTU3_TGRA);
+
+	if (val > ceiling) {
+		mutex_unlock(&priv->lock);
+		return -ERANGE;
+	}
+
+	if (count->id == RZ_MTU3_32_BIT_CH)
+		rz_mtu3_32bit_ch_write(priv->ch[id], RZ_MTU3_TCNTLW, val);
+	else
+		rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TCNT, val);
+
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
+static int rz_mtu3_count_function_read(struct counter_device *counter,
+				       struct counter_count *count,
+				       enum counter_function *function)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u32 id = count->id & 1;
+	u8 val;
+
+	val = rz_mtu3_8bit_ch_read(priv->ch[id], RZ_MTU3_TMDR1);
+
+	switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
+	case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
+		*function = COUNTER_FUNCTION_QUADRATURE_X4;
+		break;
+	case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
+		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
+		break;
+	case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
+		*function = COUNTER_FUNCTION_QUADRATURE_X2_B;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rz_mtu3_count_function_write(struct counter_device *counter,
+					struct counter_count *count,
+					enum counter_function function)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u32 id = count->id & 1;
+	u8 mode;
+
+	switch (function) {
+	case COUNTER_FUNCTION_QUADRATURE_X4:
+		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_1;
+		break;
+	case COUNTER_FUNCTION_PULSE_DIRECTION:
+		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_2;
+		break;
+	case COUNTER_FUNCTION_QUADRATURE_X2_B:
+		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_4;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TMDR1, mode);
+
+	return 0;
+}
+
+static int rz_mtu3_count_direction_read(struct counter_device *counter,
+					struct counter_count *count,
+					enum counter_count_direction *direction)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u32 id = count->id & 1;
+	u8 cnt;
+
+	cnt = rz_mtu3_8bit_ch_read(priv->ch[id], RZ_MTU3_TSR);
+
+	if (cnt & RZ_MTU3_TSR_TCFD)
+		*direction = COUNTER_COUNT_DIRECTION_FORWARD;
+	else
+		*direction = COUNTER_COUNT_DIRECTION_BACKWARD;
+
+	return 0;
+}
+
+static int rz_mtu3_count_ceiling_read(struct counter_device *counter,
+				      struct counter_count *count,
+				      u64 *ceiling)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u32 id = count->id & 1;
+
+	if (count->id == RZ_MTU3_32_BIT_CH)
+		*ceiling = rz_mtu3_32bit_ch_read(priv->ch[id], RZ_MTU3_TGRALW);
+	else
+		*ceiling = rz_mtu3_16bit_ch_read(priv->ch[id], RZ_MTU3_TGRA);
+
+	return 0;
+}
+
+static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
+				       struct counter_count *count,
+				       u64 ceiling)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	u32 id = count->id & 1;
+
+	if (ceiling > U16_MAX && rz_mtu3_is_16_bit_cnt_mode(priv))
+		return -ERANGE;
+
+	if (ceiling > U32_MAX && rz_mtu3_is_32_bit_cnt_mode(priv))
+		return -ERANGE;
+
+	mutex_lock(&priv->lock);
+	if (ceiling == 0) {
+		rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
+				      RZ_MTU3_TCR_CCLR_NONE);
+
+	} else {
+		if (count->id == RZ_MTU3_32_BIT_CH)
+			rz_mtu3_32bit_ch_write(priv->ch[id], RZ_MTU3_TGRALW, ceiling);
+		else
+			rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TGRA, ceiling);
+
+		rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
+				      RZ_MTU3_TCR_CCLR_TGRA);
+	}
+	mutex_unlock(&priv->lock);
+
+	return 0;
+}
+
+static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter, int id)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+
+	/*
+	 * 32-bit phase counting need MTU1 and MTU2 to create 32-bit cascade
+	 * counter.
+	 */
+	priv->ch[0]->function = RZ_MTU3_32BIT_PHASE_COUNTING;
+	priv->ch[1]->function = RZ_MTU3_32BIT_PHASE_COUNTING;
+
+	rz_mtu3_shared_reg_write(priv->ch[0], RZ_MTU3_TMDR3, RZ_MTU3_TMDR3_LWA);
+
+	/* Phase counting mode 1 is used as default in initialization. */
+	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TMDR1,
+			      RZ_MTU3_TMDR1_PH_CNT_MODE_1);
+
+	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
+	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TIOR, RZ_MTU3_TIOR_IC_BOTH);
+
+	rz_mtu3_enable(priv->ch[0]);
+	rz_mtu3_enable(priv->ch[1]);
+}
+
+static void rz_mtu3_16bit_cnt_setting(struct counter_device *counter, int id)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+
+	priv->ch[id]->function = RZ_MTU3_16BIT_PHASE_COUNTING;
+
+	/* Phase counting mode 1 is used as default in initialization. */
+	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TMDR1,
+			      RZ_MTU3_TMDR1_PH_CNT_MODE_1);
+
+	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
+	rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TGRA, U16_MAX);
+
+	rz_mtu3_enable(priv->ch[id]);
+}
+
+static int rz_mtu3_initialize_counter(struct counter_device *counter, int id)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+
+	if (id == RZ_MTU3_32_BIT_CH && rz_mtu3_is_16_bit_cnt_mode(priv))
+		return -EBUSY;
+
+	if (id != RZ_MTU3_32_BIT_CH && rz_mtu3_is_32_bit_cnt_mode(priv))
+		return -EBUSY;
+
+	if (id == RZ_MTU3_32_BIT_CH)
+		rz_mtu3_32bit_cnt_setting(counter, id);
+	else
+		rz_mtu3_16bit_cnt_setting(counter, id);
+
+	return 0;
+}
+
+static void rz_mtu3_terminate_counter(struct counter_device *counter, int id)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+
+	if (id == RZ_MTU3_32_BIT_CH) {
+		priv->ch[0]->function = RZ_MTU3_NORMAL;
+		priv->ch[1]->function = RZ_MTU3_NORMAL;
+		rz_mtu3_shared_reg_write(priv->ch[0], RZ_MTU3_TMDR3, 0);
+		rz_mtu3_disable(priv->ch[1]);
+		rz_mtu3_disable(priv->ch[0]);
+	} else {
+		priv->ch[id]->function = RZ_MTU3_NORMAL;
+		rz_mtu3_disable(priv->ch[id]);
+	}
+}
+
+static int rz_mtu3_count_enable_read(struct counter_device *counter,
+				     struct counter_count *count, u8 *enable)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+
+	if (count->id == RZ_MTU3_32_BIT_CH)
+		*enable = rz_mtu3_is_enabled(priv->ch[0]) &&
+			rz_mtu3_is_enabled(priv->ch[1]);
+	else
+		*enable = rz_mtu3_is_enabled(priv->ch[count->id]);
+
+	return 0;
+}
+
+static int rz_mtu3_count_enable_write(struct counter_device *counter,
+				      struct counter_count *count, u8 enable)
+{
+	struct rz_mtu3_cnt *const priv = counter_priv(counter);
+	struct rz_mtu3_channel *ch = priv->ch[count->id & 0x1];
+	int ret = 0;
+
+	if (enable) {
+		pm_runtime_get_sync(ch->dev);
+		ret = rz_mtu3_initialize_counter(counter, count->id);
+	} else {
+		rz_mtu3_terminate_counter(counter, count->id);
+		pm_runtime_put(ch->dev);
+	}
+
+	return ret;
+}
+
+static struct counter_comp rz_mtu3_count_ext[] = {
+	COUNTER_COMP_DIRECTION(rz_mtu3_count_direction_read),
+	COUNTER_COMP_ENABLE(rz_mtu3_count_enable_read,
+			    rz_mtu3_count_enable_write),
+	COUNTER_COMP_CEILING(rz_mtu3_count_ceiling_read,
+			     rz_mtu3_count_ceiling_write),
+};
+
+static const enum counter_synapse_action rz_mtu3_synapse_actions[] = {
+	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+	COUNTER_SYNAPSE_ACTION_NONE,
+};
+
+static int rz_mtu3_action_read(struct counter_device *counter,
+			       struct counter_count *count,
+			       struct counter_synapse *synapse,
+			       enum counter_synapse_action *action)
+{
+	const size_t signal_a_id = count->synapses[0].signal->id;
+	const size_t signal_b_id = count->synapses[1].signal->id;
+	enum counter_function function;
+	int err;
+
+	err = rz_mtu3_count_function_read(counter, count, &function);
+	if (err)
+		return err;
+
+	/* Default action mode */
+	*action = COUNTER_SYNAPSE_ACTION_NONE;
+
+	switch (function) {
+	case COUNTER_FUNCTION_PULSE_DIRECTION:
+		/*
+		 * Rising edges on signal A updates the respective count.
+		 * The input level of signal B determines direction.
+		 */
+		if (synapse->signal->id == signal_a_id)
+			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
+		break;
+	case COUNTER_FUNCTION_QUADRATURE_X2_B:
+		/*
+		 * Any state transition on quadrature pair signal B updates
+		 * the respective count.
+		 */
+		if (synapse->signal->id == signal_b_id)
+			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
+		break;
+	case COUNTER_FUNCTION_QUADRATURE_X4:
+		/* counts up/down on both edges of A and B signal*/
+		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct counter_ops rz_mtu3_cnt_ops = {
+	.count_read = rz_mtu3_count_read,
+	.count_write = rz_mtu3_count_write,
+	.function_read = rz_mtu3_count_function_read,
+	.function_write = rz_mtu3_count_function_write,
+	.action_read = rz_mtu3_action_read,
+};
+
+#define RZ_MTU3_PHASE_SIGNAL(_id, _name) {		\
+	.id = (_id),				\
+	.name = (_name),			\
+}
+
+static struct counter_signal rz_mtu3_signals[] = {
+	RZ_MTU3_PHASE_SIGNAL(0, "MTU1 MTCLKA"),
+	RZ_MTU3_PHASE_SIGNAL(1, "MTU1 MTCLKB"),
+	RZ_MTU3_PHASE_SIGNAL(2, "MTU2 MTCLKC"),
+	RZ_MTU3_PHASE_SIGNAL(3, "MTU2 MTCLKD"),
+};
+
+#define RZ_MTU3_COUNT_SYNAPSES(_id) {					\
+	{								\
+		.actions_list = rz_mtu3_synapse_actions,		\
+		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),	\
+		.signal = rz_mtu3_signals + 2 * (_id)			\
+	},								\
+	{								\
+		.actions_list = rz_mtu3_synapse_actions,		\
+		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),	\
+		.signal = rz_mtu3_signals + 2 * (_id) + 1		\
+	}								\
+}
+
+static struct counter_synapse rz_mtu3_count_synapses[][2] = {
+	RZ_MTU3_COUNT_SYNAPSES(0), RZ_MTU3_COUNT_SYNAPSES(1)
+};
+
+static struct counter_count rz_mtu3_counts[] = {
+	{
+		.id = 0,
+		.name = "Channel 1 Count(16-bit)",
+		.functions_list = rz_mtu3_count_functions,
+		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
+		.synapses = rz_mtu3_count_synapses[0],
+		.num_synapses = 2,
+		.ext = rz_mtu3_count_ext,
+		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
+	},
+	{
+		.id = 1,
+		.name = "Channel 2 Count(16-bit)",
+		.functions_list = rz_mtu3_count_functions,
+		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
+		.synapses = rz_mtu3_count_synapses[0],
+		.num_synapses = 4,
+		.ext = rz_mtu3_count_ext,
+		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
+	},
+	{
+		.id = 2,
+		.name = "Channel3 Count(32-bit)",
+		.functions_list = rz_mtu3_count_functions,
+		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
+		.synapses = rz_mtu3_count_synapses[0],
+		.num_synapses = 4,
+		.ext = rz_mtu3_count_ext,
+		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
+	}
+};
+
+static int __maybe_unused rz_mtu3_cnt_pm_runtime_suspend(struct device *dev)
+{
+	struct rz_mtu3_cnt *const priv = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static int __maybe_unused rz_mtu3_cnt_pm_runtime_resume(struct device *dev)
+{
+	struct rz_mtu3_cnt *const priv = dev_get_drvdata(dev);
+
+	clk_prepare_enable(priv->clk);
+
+	return 0;
+}
+
+static const struct dev_pm_ops rz_mtu3_cnt_pm_ops = {
+	SET_RUNTIME_PM_OPS(rz_mtu3_cnt_pm_runtime_suspend, rz_mtu3_cnt_pm_runtime_resume, NULL)
+};
+
+static void rz_mtu3_cnt_pm_disable(void *data)
+{
+	struct device *dev = data;
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+}
+
+static int rz_mtu3_cnt_probe(struct platform_device *pdev)
+{
+	struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct counter_device *counter;
+	struct rz_mtu3_cnt *priv;
+	unsigned int i;
+	int ret;
+
+	counter = devm_counter_alloc(dev, sizeof(*priv));
+	if (!counter)
+		return -ENOMEM;
+
+	priv = counter_priv(counter);
+	priv->clk = ddata->clk;
+
+	for (i = 0; i < RZ_MTU3_MAX_HW_CNTR_CHANNELS; i++) {
+		priv->ch[i] = &ddata->channels[RZ_MTU1 + i];
+		priv->ch[i]->dev = dev;
+		if (priv->ch[i]->function != RZ_MTU3_NORMAL)
+			return dev_err_probe(dev, -EINVAL,
+					     "channel '%u' is already claimed\n", i);
+	}
+
+	mutex_init(&priv->lock);
+	clk_prepare_enable(priv->clk);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rz_mtu3_cnt_pm_disable,
+				       dev);
+	if (ret < 0)
+		goto disable_clock;
+
+	counter->name = dev_name(dev);
+	counter->parent = dev;
+	counter->ops = &rz_mtu3_cnt_ops;
+	counter->counts = rz_mtu3_counts;
+	counter->num_counts = ARRAY_SIZE(rz_mtu3_counts);
+	counter->signals = rz_mtu3_signals;
+	counter->num_signals = ARRAY_SIZE(rz_mtu3_signals);
+	platform_set_drvdata(pdev, priv);
+
+	/* Register Counter device */
+	ret = devm_counter_add(dev, counter);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "Failed to add counter\n");
+		goto disable_clock;
+	}
+
+	return 0;
+
+disable_clock:
+	clk_disable_unprepare(priv->clk);
+
+	return ret;
+}
+
+static const struct of_device_id rz_mtu3_cnt_of_match[] = {
+	{ .compatible = "renesas,rz-mtu3-counter", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rz_mtu3_cnt_of_match);
+
+static struct platform_driver rz_mtu3_cnt_driver = {
+	.probe = rz_mtu3_cnt_probe,
+	.driver = {
+		.name = "rz-mtu3-counter",
+		.pm = &rz_mtu3_cnt_pm_ops,
+		.of_match_table = rz_mtu3_cnt_of_match,
+	},
+};
+module_platform_driver(rz_mtu3_cnt_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_ALIAS("platform:rz-mtu3-counter");
+MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a counter driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(COUNTER);
-- 
2.25.1

Re: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 26334 bytes --]

On Thu, Oct 06, 2022 at 02:57:17PM +0100, Biju Das wrote:
> Add RZ/G2L MTU3 counter driver.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Hi Biju,

This commit message is rather brief for an introduction of a new driver.
Provide a description of the functionality provided (e.g. two 16-bit
Counts or one 32-bit Count) as well as the hardware this driver supports
for context (e.g. what does MTU3 mean; is this a SoC; etc.).

> ---
> v1->v3:
>  * Modelled as a counter device supporting 3 counters(2 16-bit and 
>    32-bit)
>  * Add kernel-doc comments to document struct rz_mtu3_cnt
>  * Removed mmio variable from struct rz_mtu3_cnt
>  * Removed cnt local variable from rz_mtu3_count_read()
>  * Replaced -EINVAL->-ERANGE for out of range error conditions.
>  * Removed explicit cast from write functions.
>  * Removed local variable val from rz_mtu3_count_ceiling_read()
>  * Added lock for RMW for counter/ceiling updates.
>  * Added different synapses for counter0 and counter{1,2}
>  * Used ARRAY for assigning num_counts.
>  * Added PM runtime for managing clocks.
>  * Add MODULE_IMPORT_NS(COUNTER) to import the COUNTER namespace.
> ---
>  drivers/counter/Kconfig       |   9 +
>  drivers/counter/Makefile      |   1 +
>  drivers/counter/rz-mtu3-cnt.c | 568 ++++++++++++++++++++++++++++++++++
>  3 files changed, 578 insertions(+)
>  create mode 100644 drivers/counter/rz-mtu3-cnt.c
> 
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index d388bf26f4dc..531b187e4630 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -39,6 +39,15 @@ config INTERRUPT_CNT
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called interrupt-cnt.
>  
> +config RZ_MTU3_CNT
> +	tristate "RZ/G2L MTU3 counter driver"
> +	depends on MFD_RZ_MTU3 || COMPILE_TEST
> +	help
> +	  Select this option to enable RZ/G2L MTU3 counter driver.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called rz-mtu3-cnt.
> +

Provide a bit more description of the hardware here; you should at least
mention this is a Renesas RZ/G2L as opposed to some other manufacturer.

>  config STM32_TIMER_CNT
>  	tristate "STM32 Timer encoder counter driver"
>  	depends on MFD_STM32_TIMERS || COMPILE_TEST
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index b9a369e0d4fc..933fdd50b3e4 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -8,6 +8,7 @@ counter-y := counter-core.o counter-sysfs.o counter-chrdev.o
>  
>  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_INTERRUPT_CNT)		+= interrupt-cnt.o
> +obj-$(CONFIG_RZ_MTU3_CNT)	+= rz-mtu3-cnt.o
>  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
>  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
>  obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
> diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c
> new file mode 100644
> index 000000000000..26b5ea3852f8
> --- /dev/null
> +++ b/drivers/counter/rz-mtu3-cnt.c
> @@ -0,0 +1,568 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/G2L MTU3a Counter driver
> + *
> + * Copyright (C) 2022 Renesas Electronics Corporation
> + */
> +#include <linux/counter.h>
> +#include <linux/mfd/rz-mtu3.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/types.h>
> +
> +#define RZ_MTU3_TSR_TCFD	BIT(7)
> +#define RZ_MTU3_MAX_HW_CNTR_CHANNELS	(2)
> +
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_1	(4)
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_2	(5)
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_3	(6)
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_4	(7)
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_5	(9)
> +#define RZ_MTU3_TMDR1_PH_CNT_MODE_MASK	(0xf)
> +
> +#define RZ_MTU3_TCR_CCLR	GENMASK(7, 5)
> +#define RZ_MTU3_TCR_CCLR_NONE	FIELD_PREP(RZ_MTU3_TCR_CCLR, 0)
> +
> +#define RZ_MTU3_TMDR3_LWA	BIT(0)
> +#define RZ_MTU3_32_BIT_CH	(2)

Providing a define to identify the 32-bit channel is a good idea.
Defines for the other two 16-bit channels would also be good.

> +
> +#define RZ_MTU3_TIOR_IC_BOTH	(10)
> +
> +/**
> + * struct rz_mtu3_cnt - MTU3 counter private data
> + *
> + * @clk: MTU3 module clock
> + * @lock: Lock to prevent concurrent access for ceiling and count
> + * @rz_mtu3_channel: HW channels for the counters
> + */
> +struct rz_mtu3_cnt {
> +	struct clk *clk;
> +	struct mutex lock;
> +	struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_CNTR_CHANNELS];

Does this need to be a pointer to an array of struct rz_mtu3_channel?
You can avoid the double dereferences in your code if you leave it as a
simple pointer to struct rz_mtu3_channel and use subscripts directly as
you would an array. Or is there something I'm missing?

> +};
> +
> +static const enum counter_function rz_mtu3_count_functions[] = {
> +	COUNTER_FUNCTION_QUADRATURE_X4,
> +	COUNTER_FUNCTION_PULSE_DIRECTION,
> +	COUNTER_FUNCTION_QUADRATURE_X2_B,
> +};
> +
> +static bool rz_mtu3_is_16_bit_cnt_mode(struct rz_mtu3_cnt *const priv)
> +{
> +	return (priv->ch[0]->function == RZ_MTU3_16BIT_PHASE_COUNTING ||
> +		priv->ch[1]->function == RZ_MTU3_16BIT_PHASE_COUNTING);

Is there ever a situation where one channel is equal to
RZ_MTU3_16BIT_PHASE_COUNTING while the other channel is equal to
RZ_MTU3_32BIT_PHASE_COUNTING?

> +}
> +
> +static bool rz_mtu3_is_32_bit_cnt_mode(struct rz_mtu3_cnt *const priv)
> +{
> +	return (priv->ch[0]->function == RZ_MTU3_32BIT_PHASE_COUNTING &&
> +		priv->ch[1]->function == RZ_MTU3_32BIT_PHASE_COUNTING);
> +}
> +
> +static int rz_mtu3_count_read(struct counter_device *counter,
> +			      struct counter_count *count, u64 *val)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u32 id = count->id & 1;

It is not immediately clear why you are ANDing the Count id here. After
looking at the rest of the code in this function I realized it's because
you want to call rz_mtu3_32bit_ch_read() with id = 0 when you have
count->id = RZ_MTU3_32_BIT_CH.

I wouldn't even bother with the local id variable in this function and
instead just hardcode priv->ch[0] in the rz_mtu3_32bit_ch_read() call
below directly.

> +
> +	if (count->id == RZ_MTU3_32_BIT_CH)
> +		*val = rz_mtu3_32bit_ch_read(priv->ch[id], RZ_MTU3_TCNTLW);
> +	else
> +		*val = rz_mtu3_16bit_ch_read(priv->ch[id], RZ_MTU3_TCNT);

Is there a risk of these read calls returning an error code?

> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_write(struct counter_device *counter,
> +			       struct counter_count *count, const u64 val)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u32 id = count->id & 1;

Same comment about local id variable as in rz_mtu3_count_read().

> +	u32 ceiling;
> +
> +	mutex_lock(&priv->lock);
> +	if (count->id == RZ_MTU3_32_BIT_CH)
> +		ceiling = rz_mtu3_32bit_ch_read(priv->ch[id], RZ_MTU3_TGRALW);
> +	else
> +		ceiling = rz_mtu3_16bit_ch_read(priv->ch[id], RZ_MTU3_TGRA);

The ceiling value isn't expected to change unless the user executes your
ceiling_write() function, right? It might make sense to cache the
current ceiling value in your rz_mtu3_cnt structure so that you don't
have to read it out from the device every time you check it.

> +
> +	if (val > ceiling) {
> +		mutex_unlock(&priv->lock);
> +		return -ERANGE;
> +	}
> +
> +	if (count->id == RZ_MTU3_32_BIT_CH)
> +		rz_mtu3_32bit_ch_write(priv->ch[id], RZ_MTU3_TCNTLW, val);
> +	else
> +		rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TCNT, val);
> +
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_function_read(struct counter_device *counter,
> +				       struct counter_count *count,
> +				       enum counter_function *function)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u32 id = count->id & 1;

As mentioned before, this AND operation obscures the intention of your
code. Instead, rename this variable and account for RZ_MTU3_32_BIT_CH
explicitly with something like this:

    const size_t ch_id = (count->id == RZ_MTU3_32_BIT_CH) ? 0 : count->id;

You could wrap this into a preprocessor macro to reuse again in your
code, but I'll leave it up to you if you want.

> +	u8 val;
> +
> +	val = rz_mtu3_8bit_ch_read(priv->ch[id], RZ_MTU3_TMDR1);
> +
> +	switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
> +	case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
> +		*function = COUNTER_FUNCTION_QUADRATURE_X4;
> +		break;
> +	case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
> +		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
> +		break;
> +	case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
> +		*function = COUNTER_FUNCTION_QUADRATURE_X2_B;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_function_write(struct counter_device *counter,
> +					struct counter_count *count,
> +					enum counter_function function)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u32 id = count->id & 1;

Same comment as in rz_mtu3_count_function_read().

> +	u8 mode;
> +
> +	switch (function) {
> +	case COUNTER_FUNCTION_QUADRATURE_X4:
> +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_1;
> +		break;
> +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_2;
> +		break;
> +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_4;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TMDR1, mode);
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_direction_read(struct counter_device *counter,
> +					struct counter_count *count,
> +					enum counter_count_direction *direction)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u32 id = count->id & 1;

Same comment as in rz_mtu3_count_function_read().

> +	u8 cnt;
> +
> +	cnt = rz_mtu3_8bit_ch_read(priv->ch[id], RZ_MTU3_TSR);

This is the timer status register, right? A variable name of 'cnt' seems
a bit strange to me; would 'tsr' be a better name here?

> +
> +	if (cnt & RZ_MTU3_TSR_TCFD)
> +		*direction = COUNTER_COUNT_DIRECTION_FORWARD;
> +	else
> +		*direction = COUNTER_COUNT_DIRECTION_BACKWARD;
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_ceiling_read(struct counter_device *counter,
> +				      struct counter_count *count,
> +				      u64 *ceiling)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u32 id = count->id & 1;

Same comment about local id variable as in rz_mtu3_count_read().

> +
> +	if (count->id == RZ_MTU3_32_BIT_CH)
> +		*ceiling = rz_mtu3_32bit_ch_read(priv->ch[id], RZ_MTU3_TGRALW);
> +	else
> +		*ceiling = rz_mtu3_16bit_ch_read(priv->ch[id], RZ_MTU3_TGRA);

Assuming you're able to cache the ceiling value, you can set it here
directly and avoid the reads out to the device.

> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
> +				       struct counter_count *count,
> +				       u64 ceiling)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	u32 id = count->id & 1;

Same comment as in rz_mtu3_count_function_read().

> +	if (ceiling > U16_MAX && rz_mtu3_is_16_bit_cnt_mode(priv))
> +		return -ERANGE;
> +
> +	if (ceiling > U32_MAX && rz_mtu3_is_32_bit_cnt_mode(priv))
> +		return -ERANGE;

You can determine which maximum to consider by checking the count->id.
Instead of those two conditional statments, try this instead:

    switch (count->id) {
    case 0:
    case 1:
            if (ceiling > U16_MAX)
                    return -ERANGE;
            break;
    case RZ_MTU3_32_BIT_CH:
            if (ceiling > U32_MAX)
                    return -ERANGE;
            break;
    }

If you need to check whether you're in 32-bit phase mode before setting
the ceiling for the RZ_MTU3_32_BIT_CH Count (and similarly for the
16-bit Counts), check that separately and return -EBUSY as appropriate.

> +	mutex_lock(&priv->lock);
> +	if (ceiling == 0) {
> +		rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
> +				      RZ_MTU3_TCR_CCLR_NONE);

Looks like something different is done when ceiling is set to 0. Would
you explain what's happening in this case and why it's different that
then else case below; in other words, what's the difference between
RZ_MTU3_TCR_CCLR_NONE and RZ_MTU3_TCR_CCLR_TGRA?

> +

You can remove this empty line.

> +	} else {
> +		if (count->id == RZ_MTU3_32_BIT_CH)
> +			rz_mtu3_32bit_ch_write(priv->ch[id], RZ_MTU3_TGRALW, ceiling);
> +		else
> +			rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TGRA, ceiling);
> +
> +		rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
> +				      RZ_MTU3_TCR_CCLR_TGRA);
> +	}
> +	mutex_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter, int id)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +
> +	/*
> +	 * 32-bit phase counting need MTU1 and MTU2 to create 32-bit cascade
> +	 * counter.
> +	 */
> +	priv->ch[0]->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> +	priv->ch[1]->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> +
> +	rz_mtu3_shared_reg_write(priv->ch[0], RZ_MTU3_TMDR3, RZ_MTU3_TMDR3_LWA);
> +
> +	/* Phase counting mode 1 is used as default in initialization. */
> +	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TMDR1,
> +			      RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> +
> +	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> +	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TIOR, RZ_MTU3_TIOR_IC_BOTH);
> +
> +	rz_mtu3_enable(priv->ch[0]);
> +	rz_mtu3_enable(priv->ch[1]);
> +}
> +
> +static void rz_mtu3_16bit_cnt_setting(struct counter_device *counter, int id)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +
> +	priv->ch[id]->function = RZ_MTU3_16BIT_PHASE_COUNTING;

If 16-bit phase counting is selected for one 16-bit counter, does the
other 16-bit counter need to be configured as well?

> +
> +	/* Phase counting mode 1 is used as default in initialization. */
> +	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TMDR1,
> +			      RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> +
> +	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> +	rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TGRA, U16_MAX);
> +
> +	rz_mtu3_enable(priv->ch[id]);
> +}
> +
> +static int rz_mtu3_initialize_counter(struct counter_device *counter, int id)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +
> +	if (id == RZ_MTU3_32_BIT_CH && rz_mtu3_is_16_bit_cnt_mode(priv))
> +		return -EBUSY;
> +
> +	if (id != RZ_MTU3_32_BIT_CH && rz_mtu3_is_32_bit_cnt_mode(priv))
> +		return -EBUSY;
> +
> +	if (id == RZ_MTU3_32_BIT_CH)
> +		rz_mtu3_32bit_cnt_setting(counter, id);
> +	else
> +		rz_mtu3_16bit_cnt_setting(counter, id);

I think this code would flow better using a switch statement like this:

    switch (id) {
    case 0:
    case 1:
            if (rz_mtu3_is_32_bit_cnt_mode(priv))
                    return -EBUSY;
            rz_mtu3_16bit_cnt_setting(counter, id);
            break;
    case RZ_MTU3_32_BIT_CH:
            if (rz_mtu3_is_16_bit_cnt_mode(priv))
                    return -EBUSY;
            rz_mtu3_32bit_cnt_setting(counter, id);
            break;
    }

You should also protect this with a lock to prevent any races while
you're accessing and modifying the settings.

> +
> +	return 0;
> +}
> +
> +static void rz_mtu3_terminate_counter(struct counter_device *counter, int id)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +
> +	if (id == RZ_MTU3_32_BIT_CH) {
> +		priv->ch[0]->function = RZ_MTU3_NORMAL;
> +		priv->ch[1]->function = RZ_MTU3_NORMAL;
> +		rz_mtu3_shared_reg_write(priv->ch[0], RZ_MTU3_TMDR3, 0);
> +		rz_mtu3_disable(priv->ch[1]);
> +		rz_mtu3_disable(priv->ch[0]);
> +	} else {
> +		priv->ch[id]->function = RZ_MTU3_NORMAL;
> +		rz_mtu3_disable(priv->ch[id]);
> +	}

You probably need a lock in this function to prevent races.

> +}
> +
> +static int rz_mtu3_count_enable_read(struct counter_device *counter,
> +				     struct counter_count *count, u8 *enable)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +
> +	if (count->id == RZ_MTU3_32_BIT_CH)
> +		*enable = rz_mtu3_is_enabled(priv->ch[0]) &&
> +			rz_mtu3_is_enabled(priv->ch[1]);

There's a race between checking for channel 0 and channel 1, so use a
lock to prevent that.

> +	else
> +		*enable = rz_mtu3_is_enabled(priv->ch[count->id]);
> +
> +	return 0;
> +}
> +
> +static int rz_mtu3_count_enable_write(struct counter_device *counter,
> +				      struct counter_count *count, u8 enable)
> +{
> +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +	struct rz_mtu3_channel *ch = priv->ch[count->id & 0x1];

Same comment about the AND operation as mentioned before.

> +	int ret = 0;
> +
> +	if (enable) {
> +		pm_runtime_get_sync(ch->dev);
> +		ret = rz_mtu3_initialize_counter(counter, count->id);

Are you using the Count's "enable" extension to switch between 16-bit
and 32-bit phase modes?

> +	} else {
> +		rz_mtu3_terminate_counter(counter, count->id);
> +		pm_runtime_put(ch->dev);
> +	}
> +
> +	return ret;
> +}
> +
> +static struct counter_comp rz_mtu3_count_ext[] = {
> +	COUNTER_COMP_DIRECTION(rz_mtu3_count_direction_read),
> +	COUNTER_COMP_ENABLE(rz_mtu3_count_enable_read,
> +			    rz_mtu3_count_enable_write),
> +	COUNTER_COMP_CEILING(rz_mtu3_count_ceiling_read,
> +			     rz_mtu3_count_ceiling_write),
> +};
> +
> +static const enum counter_synapse_action rz_mtu3_synapse_actions[] = {
> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> +	COUNTER_SYNAPSE_ACTION_NONE,
> +};
> +
> +static int rz_mtu3_action_read(struct counter_device *counter,
> +			       struct counter_count *count,
> +			       struct counter_synapse *synapse,
> +			       enum counter_synapse_action *action)
> +{
> +	const size_t signal_a_id = count->synapses[0].signal->id;
> +	const size_t signal_b_id = count->synapses[1].signal->id;

If this is "Channel 2" Count then you will have four Synapses for
four possible Signals (MTCLKA, MTCLKB, MTCLKC, MTCLKD), so you'll need
to account for those two other Signals as well.

> +	enum counter_function function;
> +	int err;
> +
> +	err = rz_mtu3_count_function_read(counter, count, &function);
> +	if (err)
> +		return err;
> +
> +	/* Default action mode */
> +	*action = COUNTER_SYNAPSE_ACTION_NONE;
> +
> +	switch (function) {
> +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> +		/*
> +		 * Rising edges on signal A updates the respective count.
> +		 * The input level of signal B determines direction.
> +		 */
> +		if (synapse->signal->id == signal_a_id)
> +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> +		break;
> +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> +		/*
> +		 * Any state transition on quadrature pair signal B updates
> +		 * the respective count.
> +		 */
> +		if (synapse->signal->id == signal_b_id)
> +			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> +		break;
> +	case COUNTER_FUNCTION_QUADRATURE_X4:
> +		/* counts up/down on both edges of A and B signal*/
> +		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct counter_ops rz_mtu3_cnt_ops = {
> +	.count_read = rz_mtu3_count_read,
> +	.count_write = rz_mtu3_count_write,
> +	.function_read = rz_mtu3_count_function_read,
> +	.function_write = rz_mtu3_count_function_write,
> +	.action_read = rz_mtu3_action_read,
> +};
> +
> +#define RZ_MTU3_PHASE_SIGNAL(_id, _name) {		\
> +	.id = (_id),				\
> +	.name = (_name),			\
> +}
> +
> +static struct counter_signal rz_mtu3_signals[] = {
> +	RZ_MTU3_PHASE_SIGNAL(0, "MTU1 MTCLKA"),
> +	RZ_MTU3_PHASE_SIGNAL(1, "MTU1 MTCLKB"),
> +	RZ_MTU3_PHASE_SIGNAL(2, "MTU2 MTCLKC"),
> +	RZ_MTU3_PHASE_SIGNAL(3, "MTU2 MTCLKD"),
> +};

These names can be in a human-readable form (e.g. "Multi-function Timer
Clock A") if you think it's easier to understand, but I'll leave it up
to you if you want to change it.

> +
> +#define RZ_MTU3_COUNT_SYNAPSES(_id) {					\
> +	{								\
> +		.actions_list = rz_mtu3_synapse_actions,		\
> +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),	\
> +		.signal = rz_mtu3_signals + 2 * (_id)			\
> +	},								\
> +	{								\
> +		.actions_list = rz_mtu3_synapse_actions,		\
> +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),	\
> +		.signal = rz_mtu3_signals + 2 * (_id) + 1		\
> +	}								\
> +}
> +
> +static struct counter_synapse rz_mtu3_count_synapses[][2] = {
> +	RZ_MTU3_COUNT_SYNAPSES(0), RZ_MTU3_COUNT_SYNAPSES(1)
> +};

I know the 104-quad-8 module also creates a multidimensional array to
represent the synapses, but I would advise against this pattern because
it obscures the intention of the code.

Instead, create a separate distinct array for each group of Synapses; I
suppose there will be two arrays in this case judging from your existing
code.

> +
> +static struct counter_count rz_mtu3_counts[] = {
> +	{
> +		.id = 0,
> +		.name = "Channel 1 Count(16-bit)",
> +		.functions_list = rz_mtu3_count_functions,
> +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> +		.synapses = rz_mtu3_count_synapses[0],
> +		.num_synapses = 2,

As mentioned in the comment above, refer to the distinct Synapse array
for the particular Count and then use ARRAY_SIZE for that array to set
num_synapses.

> +		.ext = rz_mtu3_count_ext,
> +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> +	},
> +	{
> +		.id = 1,
> +		.name = "Channel 2 Count(16-bit)",
> +		.functions_list = rz_mtu3_count_functions,
> +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> +		.synapses = rz_mtu3_count_synapses[0],
> +		.num_synapses = 4,
> +		.ext = rz_mtu3_count_ext,
> +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> +	},
> +	{
> +		.id = 2,

Set id = RZ_MTU3_32_BIT_CH to make it more obvious here.

> +		.name = "Channel3 Count(32-bit)",

We probably don't need the "(32-bit)" in the name when it's obvious
already from the channel id and ceiling value.

I wonder how this counter is described in the RZ/G2L user documentation;
is it named "Channel 3" or "Channel 1 and 2"?

> +		.functions_list = rz_mtu3_count_functions,
> +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> +		.synapses = rz_mtu3_count_synapses[0],
> +		.num_synapses = 4,
> +		.ext = rz_mtu3_count_ext,
> +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> +	}
> +};
> +
> +static int __maybe_unused rz_mtu3_cnt_pm_runtime_suspend(struct device *dev)
> +{
> +	struct rz_mtu3_cnt *const priv = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused rz_mtu3_cnt_pm_runtime_resume(struct device *dev)
> +{
> +	struct rz_mtu3_cnt *const priv = dev_get_drvdata(dev);
> +
> +	clk_prepare_enable(priv->clk);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops rz_mtu3_cnt_pm_ops = {
> +	SET_RUNTIME_PM_OPS(rz_mtu3_cnt_pm_runtime_suspend, rz_mtu3_cnt_pm_runtime_resume, NULL)
> +};
> +
> +static void rz_mtu3_cnt_pm_disable(void *data)
> +{
> +	struct device *dev = data;
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +}
> +
> +static int rz_mtu3_cnt_probe(struct platform_device *pdev)
> +{
> +	struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct counter_device *counter;
> +	struct rz_mtu3_cnt *priv;
> +	unsigned int i;
> +	int ret;
> +
> +	counter = devm_counter_alloc(dev, sizeof(*priv));
> +	if (!counter)
> +		return -ENOMEM;
> +
> +	priv = counter_priv(counter);
> +	priv->clk = ddata->clk;
> +
> +	for (i = 0; i < RZ_MTU3_MAX_HW_CNTR_CHANNELS; i++) {
> +		priv->ch[i] = &ddata->channels[RZ_MTU1 + i];
> +		priv->ch[i]->dev = dev;
> +		if (priv->ch[i]->function != RZ_MTU3_NORMAL)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "channel '%u' is already claimed\n", i);
> +	}
> +
> +	mutex_init(&priv->lock);
> +	clk_prepare_enable(priv->clk);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       rz_mtu3_cnt_pm_disable,
> +				       dev);
> +	if (ret < 0)
> +		goto disable_clock;
> +
> +	counter->name = dev_name(dev);
> +	counter->parent = dev;
> +	counter->ops = &rz_mtu3_cnt_ops;
> +	counter->counts = rz_mtu3_counts;
> +	counter->num_counts = ARRAY_SIZE(rz_mtu3_counts);
> +	counter->signals = rz_mtu3_signals;
> +	counter->num_signals = ARRAY_SIZE(rz_mtu3_signals);
> +	platform_set_drvdata(pdev, priv);

It looks like you only ever use clk in your callbacks; how about setting
your drvdata to clk instead and removing it from your rz_mtu3_cnt
structure?

William Breathitt Gray

> +
> +	/* Register Counter device */
> +	ret = devm_counter_add(dev, counter);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "Failed to add counter\n");
> +		goto disable_clock;
> +	}
> +
> +	return 0;
> +
> +disable_clock:
> +	clk_disable_unprepare(priv->clk);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id rz_mtu3_cnt_of_match[] = {
> +	{ .compatible = "renesas,rz-mtu3-counter", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rz_mtu3_cnt_of_match);
> +
> +static struct platform_driver rz_mtu3_cnt_driver = {
> +	.probe = rz_mtu3_cnt_probe,
> +	.driver = {
> +		.name = "rz-mtu3-counter",
> +		.pm = &rz_mtu3_cnt_pm_ops,
> +		.of_match_table = rz_mtu3_cnt_of_match,
> +	},
> +};
> +module_platform_driver(rz_mtu3_cnt_driver);
> +
> +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> +MODULE_ALIAS("platform:rz-mtu3-counter");
> +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a counter driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(COUNTER);
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[Biju Das]

Hi William Breathitt Gray,

Thanks for the feedback.

> Subject: Re: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver
> 
> On Thu, Oct 06, 2022 at 02:57:17PM +0100, Biju Das wrote:
> > Add RZ/G2L MTU3 counter driver.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Hi Biju,
> 
> This commit message is rather brief for an introduction of a new
> driver.
> Provide a description of the functionality provided (e.g. two 16-bit
> Counts or one 32-bit Count) as well as the hardware this driver
> supports for context (e.g. what does MTU3 mean; is this a SoC; etc.).

OK Will do. MTU3- Multi-Function Timer Pulse Unit 3 (MTU3a). It is
an on-chip module on RZ/G2L SoC

> 
> > ---
> > v1->v3:
> >  * Modelled as a counter device supporting 3 counters(2 16-bit and
> >    32-bit)
> >  * Add kernel-doc comments to document struct rz_mtu3_cnt
> >  * Removed mmio variable from struct rz_mtu3_cnt
> >  * Removed cnt local variable from rz_mtu3_count_read()
> >  * Replaced -EINVAL->-ERANGE for out of range error conditions.
> >  * Removed explicit cast from write functions.
> >  * Removed local variable val from rz_mtu3_count_ceiling_read()
> >  * Added lock for RMW for counter/ceiling updates.
> >  * Added different synapses for counter0 and counter{1,2}
> >  * Used ARRAY for assigning num_counts.
> >  * Added PM runtime for managing clocks.
> >  * Add MODULE_IMPORT_NS(COUNTER) to import the COUNTER namespace.
> > ---
> >  drivers/counter/Kconfig       |   9 +
> >  drivers/counter/Makefile      |   1 +
> >  drivers/counter/rz-mtu3-cnt.c | 568
> > ++++++++++++++++++++++++++++++++++
> >  3 files changed, 578 insertions(+)
> >  create mode 100644 drivers/counter/rz-mtu3-cnt.c
> >
> > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig index
> > d388bf26f4dc..531b187e4630 100644
> > --- a/drivers/counter/Kconfig
> > +++ b/drivers/counter/Kconfig
> > @@ -39,6 +39,15 @@ config INTERRUPT_CNT
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called interrupt-cnt.
> >
> > +config RZ_MTU3_CNT
> > +	tristate "RZ/G2L MTU3 counter driver"
> > +	depends on MFD_RZ_MTU3 || COMPILE_TEST
> > +	help
> > +	  Select this option to enable RZ/G2L MTU3 counter driver.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called rz-mtu3-cnt.
> > +
> 
> Provide a bit more description of the hardware here; you should at
> least mention this is a Renesas RZ/G2L as opposed to some other
> manufacturer.

OK.

> 
> >  config STM32_TIMER_CNT
> >  	tristate "STM32 Timer encoder counter driver"
> >  	depends on MFD_STM32_TIMERS || COMPILE_TEST diff --git
> > a/drivers/counter/Makefile b/drivers/counter/Makefile index
> > b9a369e0d4fc..933fdd50b3e4 100644
> > --- a/drivers/counter/Makefile
> > +++ b/drivers/counter/Makefile
> > @@ -8,6 +8,7 @@ counter-y := counter-core.o counter-sysfs.o
> > counter-chrdev.o
> >
> >  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
> >  obj-$(CONFIG_INTERRUPT_CNT)		+= interrupt-cnt.o
> > +obj-$(CONFIG_RZ_MTU3_CNT)	+= rz-mtu3-cnt.o
> >  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
> >  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
> >  obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
> > diff --git a/drivers/counter/rz-mtu3-cnt.c
> > b/drivers/counter/rz-mtu3-cnt.c new file mode 100644 index
> > 000000000000..26b5ea3852f8
> > --- /dev/null
> > +++ b/drivers/counter/rz-mtu3-cnt.c
> > @@ -0,0 +1,568 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L MTU3a Counter driver
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corporation  */ #include
> > +<linux/counter.h> #include <linux/mfd/rz-mtu3.h> #include
> > +<linux/module.h> #include <linux/of.h> #include
> > +<linux/platform_device.h> #include <linux/pm_runtime.h> #include
> > +<linux/types.h>
> > +
> > +#define RZ_MTU3_TSR_TCFD	BIT(7)
> > +#define RZ_MTU3_MAX_HW_CNTR_CHANNELS	(2)
> > +
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_1	(4)
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_2	(5)
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_3	(6)
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_4	(7)
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_5	(9)
> > +#define RZ_MTU3_TMDR1_PH_CNT_MODE_MASK	(0xf)
> > +
> > +#define RZ_MTU3_TCR_CCLR	GENMASK(7, 5)
> > +#define RZ_MTU3_TCR_CCLR_NONE	FIELD_PREP(RZ_MTU3_TCR_CCLR, 0)
> > +
> > +#define RZ_MTU3_TMDR3_LWA	BIT(0)
> > +#define RZ_MTU3_32_BIT_CH	(2)
> 
> Providing a define to identify the 32-bit channel is a good idea.
> Defines for the other two 16-bit channels would also be good.

OK, will do.

> 
> > +
> > +#define RZ_MTU3_TIOR_IC_BOTH	(10)
> > +
> > +/**
> > + * struct rz_mtu3_cnt - MTU3 counter private data
> > + *
> > + * @clk: MTU3 module clock
> > + * @lock: Lock to prevent concurrent access for ceiling and count
> > + * @rz_mtu3_channel: HW channels for the counters  */ struct
> > +rz_mtu3_cnt {
> > +	struct clk *clk;
> > +	struct mutex lock;
> > +	struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> 
> Does this need to be a pointer to an array of struct rz_mtu3_channel?

Yes, HW has MTU{0..8} channels and MTU{1,2} supports counters
At probe time this array is filled with *ch[0]= MTU1 and *ch[1]= MTU2

> You can avoid the double dereferences in your code if you leave it as
> a simple pointer to struct rz_mtu3_channel and use subscripts directly
> as you would an array. Or is there something I'm missing?

Please see above.

> 
> > +};
> > +
> > +static const enum counter_function rz_mtu3_count_functions[] = {
> > +	COUNTER_FUNCTION_QUADRATURE_X4,
> > +	COUNTER_FUNCTION_PULSE_DIRECTION,
> > +	COUNTER_FUNCTION_QUADRATURE_X2_B,
> > +};
> > +
> > +static bool rz_mtu3_is_16_bit_cnt_mode(struct rz_mtu3_cnt *const
> > +priv) {
> > +	return (priv->ch[0]->function == RZ_MTU3_16BIT_PHASE_COUNTING ||
> > +		priv->ch[1]->function == RZ_MTU3_16BIT_PHASE_COUNTING);
> 
> Is there ever a situation where one channel is equal to
> RZ_MTU3_16BIT_PHASE_COUNTING while the other channel is equal to
> RZ_MTU3_32BIT_PHASE_COUNTING?

No that will never happen

The check is to detect runtime conditions. For eg:- user enables ch0 and then tries
to enable Ch2 

or 

ch1 and then tries ch2.

> 
> > +}
> > +
> > +static bool rz_mtu3_is_32_bit_cnt_mode(struct rz_mtu3_cnt *const
> > +priv) {
> > +	return (priv->ch[0]->function == RZ_MTU3_32BIT_PHASE_COUNTING &&
> > +		priv->ch[1]->function == RZ_MTU3_32BIT_PHASE_COUNTING); }
> > +
> > +static int rz_mtu3_count_read(struct counter_device *counter,
> > +			      struct counter_count *count, u64 *val) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> It is not immediately clear why you are ANDing the Count id here.
> After looking at the rest of the code in this function I realized it's
> because you want to call rz_mtu3_32bit_ch_read() with id = 0 when you
> have
> count->id = RZ_MTU3_32_BIT_CH.
> 
> I wouldn't even bother with the local id variable in this function and
> instead just hardcode priv->ch[0] in the rz_mtu3_32bit_ch_read() call
> below directly.

OK. Will do.

> 
> > +
> > +	if (count->id == RZ_MTU3_32_BIT_CH)
> > +		*val = rz_mtu3_32bit_ch_read(priv->ch[id], RZ_MTU3_TCNTLW);
> > +	else
> > +		*val = rz_mtu3_16bit_ch_read(priv->ch[id], RZ_MTU3_TCNT);
> 
> Is there a risk of these read calls returning an error code?

There is no risk. as the calls with these macros{RZ_MTU3_TCNTLW,RZ_MTU3_TCNT}
It never return error.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_write(struct counter_device *counter,
> > +			       struct counter_count *count, const u64 val) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> Same comment about local id variable as in rz_mtu3_count_read().
OK. Agreed.

> 
> > +	u32 ceiling;
> > +
> > +	mutex_lock(&priv->lock);
> > +	if (count->id == RZ_MTU3_32_BIT_CH)
> > +		ceiling = rz_mtu3_32bit_ch_read(priv->ch[id],
> RZ_MTU3_TGRALW);
> > +	else
> > +		ceiling = rz_mtu3_16bit_ch_read(priv->ch[id],
> RZ_MTU3_TGRA);
> 
> The ceiling value isn't expected to change unless the user executes
> your
> ceiling_write() function, right? It might make sense to cache the
> current ceiling value in your rz_mtu3_cnt structure so that you don't
> have to read it out from the device every time you check it.

OK. will add channel specific array to cache these values.

> 
> > +
> > +	if (val > ceiling) {
> > +		mutex_unlock(&priv->lock);
> > +		return -ERANGE;
> > +	}
> > +
> > +	if (count->id == RZ_MTU3_32_BIT_CH)
> > +		rz_mtu3_32bit_ch_write(priv->ch[id], RZ_MTU3_TCNTLW, val);
> > +	else
> > +		rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TCNT, val);
> > +
> > +	mutex_unlock(&priv->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_function_read(struct counter_device
> *counter,
> > +				       struct counter_count *count,
> > +				       enum counter_function *function) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> As mentioned before, this AND operation obscures the intention of your
> code. Instead, rename this variable and account for RZ_MTU3_32_BIT_CH
> explicitly with something like this:
> 
>     const size_t ch_id = (count->id == RZ_MTU3_32_BIT_CH) ? 0 : count-
> >id;

OK.

> 
> You could wrap this into a preprocessor macro to reuse again in your
> code, but I'll leave it up to you if you want.

OK.

> 
> > +	u8 val;
> > +
> > +	val = rz_mtu3_8bit_ch_read(priv->ch[id], RZ_MTU3_TMDR1);
> > +
> > +	switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
> > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
> > +		*function = COUNTER_FUNCTION_QUADRATURE_X4;
> > +		break;
> > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
> > +		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
> > +		break;
> > +	case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
> > +		*function = COUNTER_FUNCTION_QUADRATURE_X2_B;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_function_write(struct counter_device
> *counter,
> > +					struct counter_count *count,
> > +					enum counter_function function)
> > +{
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> Same comment as in rz_mtu3_count_function_read().

OK.

> 
> > +	u8 mode;
> > +
> > +	switch (function) {
> > +	case COUNTER_FUNCTION_QUADRATURE_X4:
> > +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_1;
> > +		break;
> > +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> > +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_2;
> > +		break;
> > +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> > +		mode = RZ_MTU3_TMDR1_PH_CNT_MODE_4;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TMDR1, mode);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_direction_read(struct counter_device
> *counter,
> > +					struct counter_count *count,
> > +					enum counter_count_direction *direction)
> {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> Same comment as in rz_mtu3_count_function_read().
OK.
> 
> > +	u8 cnt;
> > +
> > +	cnt = rz_mtu3_8bit_ch_read(priv->ch[id], RZ_MTU3_TSR);
> 
> This is the timer status register, right? A variable name of 'cnt'
> seems a bit strange to me; would 'tsr' be a better name here?

Yes, it is timer status register. Will change it to tsr.

> 
> > +
> > +	if (cnt & RZ_MTU3_TSR_TCFD)
> > +		*direction = COUNTER_COUNT_DIRECTION_FORWARD;
> > +	else
> > +		*direction = COUNTER_COUNT_DIRECTION_BACKWARD;
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_ceiling_read(struct counter_device
> *counter,
> > +				      struct counter_count *count,
> > +				      u64 *ceiling)
> > +{
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> Same comment about local id variable as in rz_mtu3_count_read().

OK.
> 
> > +
> > +	if (count->id == RZ_MTU3_32_BIT_CH)
> > +		*ceiling = rz_mtu3_32bit_ch_read(priv->ch[id],
> RZ_MTU3_TGRALW);
> > +	else
> > +		*ceiling = rz_mtu3_16bit_ch_read(priv->ch[id],
> RZ_MTU3_TGRA);
> 
> Assuming you're able to cache the ceiling value, you can set it here
> directly and avoid the reads out to the device.

OK.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_ceiling_write(struct counter_device
> *counter,
> > +				       struct counter_count *count,
> > +				       u64 ceiling)
> > +{
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 id = count->id & 1;
> 
> Same comment as in rz_mtu3_count_function_read().
OK.
> 
> > +	if (ceiling > U16_MAX && rz_mtu3_is_16_bit_cnt_mode(priv))
> > +		return -ERANGE;
> > +
> > +	if (ceiling > U32_MAX && rz_mtu3_is_32_bit_cnt_mode(priv))
> > +		return -ERANGE;
> 
> You can determine which maximum to consider by checking the count->id.
> Instead of those two conditional statments, try this instead:
> 
>     switch (count->id) {
>     case 0:
>     case 1:
>             if (ceiling > U16_MAX)
>                     return -ERANGE;
>             break;
>     case RZ_MTU3_32_BIT_CH:
>             if (ceiling > U32_MAX)
>                     return -ERANGE;
>             break;
>     }
> 
OK.

> If you need to check whether you're in 32-bit phase mode before
> setting the ceiling for the RZ_MTU3_32_BIT_CH Count (and similarly for
> the 16-bit Counts), check that separately and return -EBUSY as
> appropriate.

OK.
> 
> > +	mutex_lock(&priv->lock);
> > +	if (ceiling == 0) {
> > +		rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
> > +				      RZ_MTU3_TCR_CCLR_NONE);
> 
> Looks like something different is done when ceiling is set to 0. Would
> you explain what's happening in this case and why it's different that
> then else case below; in other words, what's the difference between
> RZ_MTU3_TCR_CCLR_NONE and RZ_MTU3_TCR_CCLR_TGRA?

RZ_MTU3_TCR_CCLR_TGRA --> for triggering counter count using Z-Phase signal.
RZ_MTU3_TCR_CCLR_NONE --> No clearing.

> 
> > +
> 
> You can remove this empty line.
OK.
> 
> > +	} else {
> > +		if (count->id == RZ_MTU3_32_BIT_CH)
> > +			rz_mtu3_32bit_ch_write(priv->ch[id], RZ_MTU3_TGRALW,
> ceiling);
> > +		else
> > +			rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TGRA,
> ceiling);
> > +
> > +		rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
> > +				      RZ_MTU3_TCR_CCLR_TGRA);
> > +	}
> > +	mutex_unlock(&priv->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static void rz_mtu3_32bit_cnt_setting(struct counter_device
> *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	/*
> > +	 * 32-bit phase counting need MTU1 and MTU2 to create 32-bit
> cascade
> > +	 * counter.
> > +	 */
> > +	priv->ch[0]->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > +	priv->ch[1]->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > +
> > +	rz_mtu3_shared_reg_write(priv->ch[0], RZ_MTU3_TMDR3,
> > +RZ_MTU3_TMDR3_LWA);
> > +
> > +	/* Phase counting mode 1 is used as default in initialization. */
> > +	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TMDR1,
> > +			      RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > +
> > +	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TCR,
> RZ_MTU3_TCR_CCLR_TGRA);
> > +	rz_mtu3_8bit_ch_write(priv->ch[0], RZ_MTU3_TIOR,
> > +RZ_MTU3_TIOR_IC_BOTH);
> > +
> > +	rz_mtu3_enable(priv->ch[0]);
> > +	rz_mtu3_enable(priv->ch[1]);
> > +}
> > +
> > +static void rz_mtu3_16bit_cnt_setting(struct counter_device
> *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	priv->ch[id]->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> 
> If 16-bit phase counting is selected for one 16-bit counter, does the
> other 16-bit counter need to be configured as well?

Not required I guess, as it is run time decision.

After this, if user tries to enable 16-bit on other channel,
we will configure that channel. otherwise, we will return error,
if user tries to enable 32-bit channel.

Are you ok with this? 


> 
> > +
> > +	/* Phase counting mode 1 is used as default in initialization. */
> > +	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TMDR1,
> > +			      RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > +
> > +	rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
> RZ_MTU3_TCR_CCLR_TGRA);
> > +	rz_mtu3_16bit_ch_write(priv->ch[id], RZ_MTU3_TGRA, U16_MAX);
> > +
> > +	rz_mtu3_enable(priv->ch[id]);
> > +}
> > +
> > +static int rz_mtu3_initialize_counter(struct counter_device
> *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	if (id == RZ_MTU3_32_BIT_CH && rz_mtu3_is_16_bit_cnt_mode(priv))
> > +		return -EBUSY;
> > +
> > +	if (id != RZ_MTU3_32_BIT_CH && rz_mtu3_is_32_bit_cnt_mode(priv))
> > +		return -EBUSY;
> > +
> > +	if (id == RZ_MTU3_32_BIT_CH)
> > +		rz_mtu3_32bit_cnt_setting(counter, id);
> > +	else
> > +		rz_mtu3_16bit_cnt_setting(counter, id);
> 
> I think this code would flow better using a switch statement like
> this:
> 
>     switch (id) {
>     case 0:
>     case 1:
>             if (rz_mtu3_is_32_bit_cnt_mode(priv))
>                     return -EBUSY;
>             rz_mtu3_16bit_cnt_setting(counter, id);
>             break;
>     case RZ_MTU3_32_BIT_CH:
>             if (rz_mtu3_is_16_bit_cnt_mode(priv))
>                     return -EBUSY;
>             rz_mtu3_32bit_cnt_setting(counter, id);
>             break;
>     }
> 
> You should also protect this with a lock to prevent any races while
> you're accessing and modifying the settings.

OK.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void rz_mtu3_terminate_counter(struct counter_device
> *counter,
> > +int id) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	if (id == RZ_MTU3_32_BIT_CH) {
> > +		priv->ch[0]->function = RZ_MTU3_NORMAL;
> > +		priv->ch[1]->function = RZ_MTU3_NORMAL;
> > +		rz_mtu3_shared_reg_write(priv->ch[0], RZ_MTU3_TMDR3, 0);
> > +		rz_mtu3_disable(priv->ch[1]);
> > +		rz_mtu3_disable(priv->ch[0]);
> > +	} else {
> > +		priv->ch[id]->function = RZ_MTU3_NORMAL;
> > +		rz_mtu3_disable(priv->ch[id]);
> > +	}
> 
> You probably need a lock in this function to prevent races.

OK.

> 
> > +}
> > +
> > +static int rz_mtu3_count_enable_read(struct counter_device
> *counter,
> > +				     struct counter_count *count, u8 *enable) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	if (count->id == RZ_MTU3_32_BIT_CH)
> > +		*enable = rz_mtu3_is_enabled(priv->ch[0]) &&
> > +			rz_mtu3_is_enabled(priv->ch[1]);
> 
> There's a race between checking for channel 0 and channel 1, so use a
> lock to prevent that.

OK. Agreed.
> 
> > +	else
> > +		*enable = rz_mtu3_is_enabled(priv->ch[count->id]);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rz_mtu3_count_enable_write(struct counter_device
> *counter,
> > +				      struct counter_count *count, u8 enable) {
> > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > +	struct rz_mtu3_channel *ch = priv->ch[count->id & 0x1];
> 
> Same comment about the AND operation as mentioned before.

OK.
> 
> > +	int ret = 0;
> > +
> > +	if (enable) {
> > +		pm_runtime_get_sync(ch->dev);
> > +		ret = rz_mtu3_initialize_counter(counter, count->id);
> 
> Are you using the Count's "enable" extension to switch between 16-bit
> and 32-bit phase modes?

No. But will use that for switching on the next version.


> 
> > +	} else {
> > +		rz_mtu3_terminate_counter(counter, count->id);
> > +		pm_runtime_put(ch->dev);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static struct counter_comp rz_mtu3_count_ext[] = {
> > +	COUNTER_COMP_DIRECTION(rz_mtu3_count_direction_read),
> > +	COUNTER_COMP_ENABLE(rz_mtu3_count_enable_read,
> > +			    rz_mtu3_count_enable_write),
> > +	COUNTER_COMP_CEILING(rz_mtu3_count_ceiling_read,
> > +			     rz_mtu3_count_ceiling_write), };
> > +
> > +static const enum counter_synapse_action rz_mtu3_synapse_actions[]
> = {
> > +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> > +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> > +	COUNTER_SYNAPSE_ACTION_NONE,
> > +};
> > +
> > +static int rz_mtu3_action_read(struct counter_device *counter,
> > +			       struct counter_count *count,
> > +			       struct counter_synapse *synapse,
> > +			       enum counter_synapse_action *action) {
> > +	const size_t signal_a_id = count->synapses[0].signal->id;
> > +	const size_t signal_b_id = count->synapses[1].signal->id;
> 
> If this is "Channel 2" Count then you will have four Synapses for four
> possible Signals (MTCLKA, MTCLKB, MTCLKC, MTCLKD), so you'll need to
> account for those two other Signals as well.

OK.

> 
> > +	enum counter_function function;
> > +	int err;
> > +
> > +	err = rz_mtu3_count_function_read(counter, count, &function);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Default action mode */
> > +	*action = COUNTER_SYNAPSE_ACTION_NONE;
> > +
> > +	switch (function) {
> > +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> > +		/*
> > +		 * Rising edges on signal A updates the respective count.
> > +		 * The input level of signal B determines direction.
> > +		 */
> > +		if (synapse->signal->id == signal_a_id)
> > +			*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> > +		break;
> > +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> > +		/*
> > +		 * Any state transition on quadrature pair signal B updates
> > +		 * the respective count.
> > +		 */
> > +		if (synapse->signal->id == signal_b_id)
> > +			*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > +		break;
> > +	case COUNTER_FUNCTION_QUADRATURE_X4:
> > +		/* counts up/down on both edges of A and B signal*/
> > +		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct counter_ops rz_mtu3_cnt_ops = {
> > +	.count_read = rz_mtu3_count_read,
> > +	.count_write = rz_mtu3_count_write,
> > +	.function_read = rz_mtu3_count_function_read,
> > +	.function_write = rz_mtu3_count_function_write,
> > +	.action_read = rz_mtu3_action_read,
> > +};
> > +
> > +#define RZ_MTU3_PHASE_SIGNAL(_id, _name) {		\
> > +	.id = (_id),				\
> > +	.name = (_name),			\
> > +}
> > +
> > +static struct counter_signal rz_mtu3_signals[] = {
> > +	RZ_MTU3_PHASE_SIGNAL(0, "MTU1 MTCLKA"),
> > +	RZ_MTU3_PHASE_SIGNAL(1, "MTU1 MTCLKB"),
> > +	RZ_MTU3_PHASE_SIGNAL(2, "MTU2 MTCLKC"),
> > +	RZ_MTU3_PHASE_SIGNAL(3, "MTU2 MTCLKD"), };
> 
> These names can be in a human-readable form (e.g. "Multi-function
> Timer Clock A") if you think it's easier to understand, but I'll leave
> it up to you if you want to change it.

HW manual says MTCLK{A,B,C,D}. that is the reason.

> 
> > +
> > +#define RZ_MTU3_COUNT_SYNAPSES(_id) {					\
> > +	{								\
> > +		.actions_list = rz_mtu3_synapse_actions,		\
> > +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),	\
> > +		.signal = rz_mtu3_signals + 2 * (_id)			\
> > +	},								\
> > +	{								\
> > +		.actions_list = rz_mtu3_synapse_actions,		\
> > +		.num_actions = ARRAY_SIZE(rz_mtu3_synapse_actions),	\
> > +		.signal = rz_mtu3_signals + 2 * (_id) + 1		\
> > +	}								\
> > +}
> > +
> > +static struct counter_synapse rz_mtu3_count_synapses[][2] = {
> > +	RZ_MTU3_COUNT_SYNAPSES(0), RZ_MTU3_COUNT_SYNAPSES(1) };
> 
> I know the 104-quad-8 module also creates a multidimensional array to
> represent the synapses, but I would advise against this pattern
> because it obscures the intention of the code.
> 
> Instead, create a separate distinct array for each group of Synapses;
> I suppose there will be two arrays in this case judging from your
> existing code.

OK. Will do.

> 
> > +
> > +static struct counter_count rz_mtu3_counts[] = {
> > +	{
> > +		.id = 0,
> > +		.name = "Channel 1 Count(16-bit)",
> > +		.functions_list = rz_mtu3_count_functions,
> > +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > +		.synapses = rz_mtu3_count_synapses[0],
> > +		.num_synapses = 2,
> 
> As mentioned in the comment above, refer to the distinct Synapse array
> for the particular Count and then use ARRAY_SIZE for that array to set
> num_synapses.

OK will do.
> 
> > +		.ext = rz_mtu3_count_ext,
> > +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > +	},
> > +	{
> > +		.id = 1,
> > +		.name = "Channel 2 Count(16-bit)",
> > +		.functions_list = rz_mtu3_count_functions,
> > +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > +		.synapses = rz_mtu3_count_synapses[0],
> > +		.num_synapses = 4,
> > +		.ext = rz_mtu3_count_ext,
> > +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > +	},
> > +	{
> > +		.id = 2,
> 
> Set id = RZ_MTU3_32_BIT_CH to make it more obvious here.
> 
> > +		.name = "Channel3 Count(32-bit)",
> 
> We probably don't need the "(32-bit)" in the name when it's obvious
> already from the channel id and ceiling value.

OK will remove it.
> 
> I wonder how this counter is described in the RZ/G2L user
> documentation; is it named "Channel 3" or "Channel 1 and 2"?

It is mentioned as MTU1 and MTU2 channels.

These channels can be used for phase counting and PWM operations.

> 
> > +		.functions_list = rz_mtu3_count_functions,
> > +		.num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > +		.synapses = rz_mtu3_count_synapses[0],
> > +		.num_synapses = 4,
> > +		.ext = rz_mtu3_count_ext,
> > +		.num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > +	}
> > +};
> > +
> > +static int __maybe_unused rz_mtu3_cnt_pm_runtime_suspend(struct
> > +device *dev) {
> > +	struct rz_mtu3_cnt *const priv = dev_get_drvdata(dev);
> > +
> > +	clk_disable_unprepare(priv->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused rz_mtu3_cnt_pm_runtime_resume(struct
> device
> > +*dev) {
> > +	struct rz_mtu3_cnt *const priv = dev_get_drvdata(dev);
> > +
> > +	clk_prepare_enable(priv->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops rz_mtu3_cnt_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(rz_mtu3_cnt_pm_runtime_suspend,
> > +rz_mtu3_cnt_pm_runtime_resume, NULL) };
> > +
> > +static void rz_mtu3_cnt_pm_disable(void *data) {
> > +	struct device *dev = data;
> > +
> > +	pm_runtime_disable(dev);
> > +	pm_runtime_set_suspended(dev);
> > +}
> > +
> > +static int rz_mtu3_cnt_probe(struct platform_device *pdev) {
> > +	struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
> > +	struct device *dev = &pdev->dev;
> > +	struct counter_device *counter;
> > +	struct rz_mtu3_cnt *priv;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	counter = devm_counter_alloc(dev, sizeof(*priv));
> > +	if (!counter)
> > +		return -ENOMEM;
> > +
> > +	priv = counter_priv(counter);
> > +	priv->clk = ddata->clk;
> > +
> > +	for (i = 0; i < RZ_MTU3_MAX_HW_CNTR_CHANNELS; i++) {
> > +		priv->ch[i] = &ddata->channels[RZ_MTU1 + i];
> > +		priv->ch[i]->dev = dev;
> > +		if (priv->ch[i]->function != RZ_MTU3_NORMAL)
> > +			return dev_err_probe(dev, -EINVAL,
> > +					     "channel '%u' is already claimed\n",
> i);
> > +	}
> > +
> > +	mutex_init(&priv->lock);
> > +	clk_prepare_enable(priv->clk);
> > +	pm_runtime_set_active(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       rz_mtu3_cnt_pm_disable,
> > +				       dev);
> > +	if (ret < 0)
> > +		goto disable_clock;
> > +
> > +	counter->name = dev_name(dev);
> > +	counter->parent = dev;
> > +	counter->ops = &rz_mtu3_cnt_ops;
> > +	counter->counts = rz_mtu3_counts;
> > +	counter->num_counts = ARRAY_SIZE(rz_mtu3_counts);
> > +	counter->signals = rz_mtu3_signals;
> > +	counter->num_signals = ARRAY_SIZE(rz_mtu3_signals);
> > +	platform_set_drvdata(pdev, priv);
> 
> It looks like you only ever use clk in your callbacks; how about
> setting your drvdata to clk instead and removing it from your
> rz_mtu3_cnt structure?

OK. Will do. 

Note:
This change is based on feedback[1] on bindings.

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20221006135717.1748560-2-biju.das.jz@bp.renesas.com/

Cheers,
Biju

> 
> > +
> > +	/* Register Counter device */
> > +	ret = devm_counter_add(dev, counter);
> > +	if (ret < 0) {
> > +		dev_err_probe(dev, ret, "Failed to add counter\n");
> > +		goto disable_clock;
> > +	}
> > +
> > +	return 0;
> > +
> > +disable_clock:
> > +	clk_disable_unprepare(priv->clk);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id rz_mtu3_cnt_of_match[] = {
> > +	{ .compatible = "renesas,rz-mtu3-counter", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rz_mtu3_cnt_of_match);
> > +
> > +static struct platform_driver rz_mtu3_cnt_driver = {
> > +	.probe = rz_mtu3_cnt_probe,
> > +	.driver = {
> > +		.name = "rz-mtu3-counter",
> > +		.pm = &rz_mtu3_cnt_pm_ops,
> > +		.of_match_table = rz_mtu3_cnt_of_match,
> > +	},
> > +};
> > +module_platform_driver(rz_mtu3_cnt_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> > +MODULE_ALIAS("platform:rz-mtu3-counter");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a counter driver");
> > +MODULE_LICENSE("GPL"); MODULE_IMPORT_NS(COUNTER);
> > --
> > 2.25.1
> >

Re: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 6303 bytes --]

On Sat, Oct 08, 2022 at 09:01:21AM +0000, Biju Das wrote:
> Hi William Breathitt Gray,
> 
> Thanks for the feedback.

Hello Biju,

I see that you have already released a v4, so some of my comments may no
longer apply, but I want to respond here to continue our discussions;
I'll reiterate any relevant suggestions when I review v4 in the coming
days.

By the way, if you agree with a review comment there is no need to reply
with "OK"; just delete the parts you agree with from your response and
I'll know those are okay. Doing this will reduce the amount of text we
have to scroll through and thus allow us to focus on just the questions
we have remaining. ;-)

> > > +/**
> > > + * struct rz_mtu3_cnt - MTU3 counter private data
> > > + *
> > > + * @clk: MTU3 module clock
> > > + * @lock: Lock to prevent concurrent access for ceiling and count
> > > + * @rz_mtu3_channel: HW channels for the counters  */ struct
> > > +rz_mtu3_cnt {
> > > +	struct clk *clk;
> > > +	struct mutex lock;
> > > +	struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> > 
> > Does this need to be a pointer to an array of struct rz_mtu3_channel?
> 
> Yes, HW has MTU{0..8} channels and MTU{1,2} supports counters
> At probe time this array is filled with *ch[0]= MTU1 and *ch[1]= MTU2

In the rz_mtu3_cnt_probe() function I see the rz_mtu3_cnt.ch elements
manually set to the address of each rz_mtu3.channels element:

    for (i = 0; i < RZ_MTU3_MAX_HW_CNTR_CHANNELS; i++) {
        priv->ch[i] = &ddata->channels[RZ_MTU1 + i];
	priv->ch[i]->dev = dev;
    ...

The rz_mut3.channels member is a contiguous array of struct
rz_mtu3_channel. If you change the rz_mtu3_channel to a pointer to
struct rz_mtu3_channel, you can set it to the RZ_MTU1 offset address
outside of the for loop and thus avoid the double dereference because
these address are contiguous:

    priv->ch = &ddata->channels[RZ_MTU1];
    for (i = 0; i < RZ_MTU3_MAX_HW_CNTR_CHANNELS; i++) {
	priv->ch[i].dev = dev;
    ...

> > > +	mutex_lock(&priv->lock);
> > > +	if (ceiling == 0) {
> > > +		rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
> > > +				      RZ_MTU3_TCR_CCLR_NONE);
> > 
> > Looks like something different is done when ceiling is set to 0. Would
> > you explain what's happening in this case and why it's different that
> > then else case below; in other words, what's the difference between
> > RZ_MTU3_TCR_CCLR_NONE and RZ_MTU3_TCR_CCLR_TGRA?
> 
> RZ_MTU3_TCR_CCLR_TGRA --> for triggering counter count using Z-Phase signal.
> RZ_MTU3_TCR_CCLR_NONE --> No clearing.

Does the Z-Phase signal trigger a reset of the counter count back to the
ceiling value? Does the count loop back to 0 when it passes the ceiling
value, or does it remain at the ceiling until the direction changes?
By "no clearing" do you mean that the ceiling is disabled in this case
and the Counter count increases without limit?

In the Counter subsystem, the "ceiling" Count extension puts an upper
limit on the Count value. This means that setting "ceiling" to 0 would
put the upper limit at 0, effectively restricting the Count value to 0
until the value of "ceiling" is raised.

If the device is unable to support a ceiling value of 0, you should
return -ERANGE rather than disable the ceiling.

> > > +static void rz_mtu3_16bit_cnt_setting(struct counter_device
> > *counter,
> > > +int id) {
> > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > +
> > > +	priv->ch[id]->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> > 
> > If 16-bit phase counting is selected for one 16-bit counter, does the
> > other 16-bit counter need to be configured as well?
> 
> Not required I guess, as it is run time decision.
> 
> After this, if user tries to enable 16-bit on other channel,
> we will configure that channel. otherwise, we will return error,
> if user tries to enable 32-bit channel.
> 
> Are you ok with this? 

Because the phase mode affects how the device interprets multiple
channels rather than a specific one, maybe it's better to save this
state as an enum rz_mtu3_function member of struct rz_mtu3_cnt. Or if
this is affecting the entire device, move it to your struct rz_mut3 and
share a pointer to that for your Counter and PWM drivers.

It makes me wonder if the rz_mut3_cnt structure is necessary for this
Counter driver at all when you could pass a pointer your existing
rz_mut3 structure instead in order to access the channels.

> > > +	int ret = 0;
> > > +
> > > +	if (enable) {
> > > +		pm_runtime_get_sync(ch->dev);
> > > +		ret = rz_mtu3_initialize_counter(counter, count->id);
> > 
> > Are you using the Count's "enable" extension to switch between 16-bit
> > and 32-bit phase modes?
> 
> No. But will use that for switching on the next version.

Sorry, I wasn't clear with my question. Please do not implement the
"enable" Count extensions as a way to toggle between the 16-bit and
32-bit phase modes. The purpose of "enable" is to provide a pause/resume
mechanism for a Count: the existing count value should be preserved when
a Count is disabled, and should continue where it left off when the
Count is enabled.

To support the phase mode selection, implement a Counter device
extension for that specific purpose. You can use DEFINE_COUNTER_ENUM()
and COUNTER_COMP_DEVICE_ENUM() to create a device extension that will
allow users to toggle between "16-bit" and "32-bit" phase modes. If you
need help with these macros, just let me know.

> > > +		.name = "Channel3 Count(32-bit)",
> > 
> > We probably don't need the "(32-bit)" in the name when it's obvious
> > already from the channel id and ceiling value.
> 
> OK will remove it.
> > 
> > I wonder how this counter is described in the RZ/G2L user
> > documentation; is it named "Channel 3" or "Channel 1 and 2"?
> 
> It is mentioned as MTU1 and MTU2 channels.
> 
> These channels can be used for phase counting and PWM operations.

We should avoid calling it "Channel 3" in that case to avoid confusion.
Perhaps "Channel 1 and 2 (combined) Count" would be better; that's just
something I came up with off the top of my head, but if you can think of
a better way to phrase it then please do.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[Biju Das]

Hi William Breathitt Gray,

Thanks for the feedback.

> Subject: Re: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver
> 
> On Sat, Oct 08, 2022 at 09:01:21AM +0000, Biju Das wrote:
> > Hi William Breathitt Gray,
> >
> > Thanks for the feedback.
> 
> Hello Biju,
> 
> I see that you have already released a v4, so some of my comments may
> no longer apply, but I want to respond here to continue our
> discussions; I'll reiterate any relevant suggestions when I review v4
> in the coming days.
> 
> By the way, if you agree with a review comment there is no need to
> reply with "OK"; just delete the parts you agree with from your
> response and I'll know those are okay. Doing this will reduce the
> amount of text we have to scroll through and thus allow us to focus on
> just the questions we have remaining. ;-)

OK.

> > > Looks like something different is done when ceiling is set to 0.
> > > Would you explain what's happening in this case and why it's
> > > different that then else case below; in other words, what's the
> > > difference between RZ_MTU3_TCR_CCLR_NONE and
> RZ_MTU3_TCR_CCLR_TGRA?
> >
> > RZ_MTU3_TCR_CCLR_TGRA --> for triggering counter count using Z-Phase
> signal.
> > RZ_MTU3_TCR_CCLR_NONE --> No clearing.
> 
> Does the Z-Phase signal trigger a reset of the counter count back to
> the ceiling value? 

No, It resets to 0.

Does the count loop back to 0 when it passes the
> ceiling value,

Yes, it loopback to 0.

 or does it remain at the ceiling until the direction
> changes?

No.

> By "no clearing" do you mean that the ceiling is disabled in this case
> and the Counter count increases without limit?

Counter clear source disabled. So the counter won't reset to 0 by applying
Z-Phase signal.

> 
> In the Counter subsystem, the "ceiling" Count extension puts an upper
> limit on the Count value. This means that setting "ceiling" to 0 would
> put the upper limit at 0, effectively restricting the Count value to 0
> until the value of "ceiling" is raised.
> 
> If the device is unable to support a ceiling value of 0, you should
> return -ERANGE rather than disable the ceiling.

OK, will check this.


> 
> > > > +static void rz_mtu3_16bit_cnt_setting(struct counter_device
> > > *counter,
> > > > +int id) {
> > > > +	struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > > +
> > > > +	priv->ch[id]->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> > >
> > > If 16-bit phase counting is selected for one 16-bit counter, does
> > > the other 16-bit counter need to be configured as well?
> >
> > Not required I guess, as it is run time decision.
> >
> > After this, if user tries to enable 16-bit on other channel, we will
> > configure that channel. otherwise, we will return error, if user
> tries
> > to enable 32-bit channel.
> >
> > Are you ok with this?
> 
> Because the phase mode affects how the device interprets multiple
> channels rather than a specific one, maybe it's better to save this
> state as an enum rz_mtu3_function member of struct rz_mtu3_cnt. Or if
> this is affecting the entire device, move it to your struct rz_mut3
> and share a pointer to that for your Counter and PWM drivers.

It is the same pointer allocated by parent and shared to both Counter
and PWM drivers. At runtime any device can claim the channel by checking
RZ_MTU3_NORMAL function.

> 
> It makes me wonder if the rz_mut3_cnt structure is necessary for this
> Counter driver at all when you could pass a pointer your existing
> rz_mut3 structure instead in order to access the channels.

It is a shared pointer. For easy handling I have added 
only 2 channels that are relevant for the counter in rz_mut3_cnt.
Similar case for pwm.

> 
> > > > +	int ret = 0;
> > > > +
> > > > +	if (enable) {
> > > > +		pm_runtime_get_sync(ch->dev);
> > > > +		ret = rz_mtu3_initialize_counter(counter, count->id);
> > >
> > > Are you using the Count's "enable" extension to switch between
> > > 16-bit and 32-bit phase modes?
> >
> > No. But will use that for switching on the next version.
> 
> Sorry, I wasn't clear with my question. Please do not implement the
> "enable" Count extensions as a way to toggle between the 16-bit and
> 32-bit phase modes. The purpose of "enable" is to provide a
> pause/resume mechanism for a Count: the existing count value should be
> preserved when a Count is disabled, and should continue where it left
> off when the Count is enabled.

I use below sample program to test 16-bit and 32-bit phase mode
Counting with trigger for clear count by grounding the Z-phase pin.

16-bit phase testing
------------------
echo 1 > /sys/bus/counter/devices/counter0/count0/enable
echo 0 > /sys/bus/counter/devices/counter0/count0/count
echo 20 > /sys/bus/counter/devices/counter0/count0/ceiling
       
echo 1 > /sys/bus/counter/devices/counter0/count1/enable
echo 0 > /sys/bus/counter/devices/counter0/count1/count
echo 20 > /sys/bus/counter/devices/counter0/count1/ceiling

for i in {1..200};
do
   cat /sys/bus/counter/devices/counter0/count0/count;
   cat /sys/bus/counter/devices/counter0/count1/count;
done

echo 0 > /sys/bus/counter/devices/counter0/count1/enable
echo 0 > /sys/bus/counter/devices/counter0/count0/enable

32-bit phase count testing
--------------------------
echo 1 > /sys/bus/counter/devices/counter0/count2/enable
echo 0 > /sys/bus/counter/devices/counter0/count2/count
echo 20 > /sys/bus/counter/devices/counter0/count2/ceiling

for i in {1..200};
do
   cat /sys/bus/counter/devices/counter0/count2/count;
done

echo 0 > /sys/bus/counter/devices/counter0/count2/enable
 
> To support the phase mode selection, implement a Counter device
> extension for that specific purpose. You can use DEFINE_COUNTER_ENUM()
> and COUNTER_COMP_DEVICE_ENUM() to create a device extension that will
> allow users to toggle between "16-bit" and "32-bit" phase modes. If
> you need help with these macros, just let me know.

Yes please, that will be helpful. 

Cheers,
Biju

RE: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[Biju Das]

Hi William Breathitt Gray,

> Subject: RE: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver
> 
> Hi William Breathitt Gray,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver
> >
> > On Sat, Oct 08, 2022 at 09:01:21AM +0000, Biju Das wrote:
> > > Hi William Breathitt Gray,
> > >
> > > Thanks for the feedback.
> >
> > Hello Biju,
> >
> > I see that you have already released a v4, so some of my comments
> may
> > no longer apply, but I want to respond here to continue our
> > discussions; I'll reiterate any relevant suggestions when I review
> v4
> > in the coming days.
> >
> > By the way, if you agree with a review comment there is no need to
> > reply with "OK"; just delete the parts you agree with from your
> > response and I'll know those are okay. Doing this will reduce the
> > amount of text we have to scroll through and thus allow us to focus
> on
> > just the questions we have remaining. ;-)
> 
> OK.
> 
> > > > Looks like something different is done when ceiling is set to 0.
> > > > Would you explain what's happening in this case and why it's
> > > > different that then else case below; in other words, what's the
> > > > difference between RZ_MTU3_TCR_CCLR_NONE and
> > RZ_MTU3_TCR_CCLR_TGRA?
> > >
> > > RZ_MTU3_TCR_CCLR_TGRA --> for triggering counter count using Z-
> Phase
> > signal.
> > > RZ_MTU3_TCR_CCLR_NONE --> No clearing.
> >
> > Does the Z-Phase signal trigger a reset of the counter count back to
> > the ceiling value?
> 
> No, It resets to 0.

It reset to 0 for forward counting
and resets to U16_MAX or U32_MAX for backword counting

> 
> Does the count loop back to 0 when it passes the
> > ceiling value,
> 
> Yes, it loopback to 0.

Same as above.

> 
>  or does it remain at the ceiling until the direction
> > changes?
> 
> No.
> 
> > By "no clearing" do you mean that the ceiling is disabled in this
> case
> > and the Counter count increases without limit?
> 
> Counter clear source disabled. So the counter won't reset to 0 by
> applying Z-Phase signal.
> 
> >
> > In the Counter subsystem, the "ceiling" Count extension puts an
> upper
> > limit on the Count value. This means that setting "ceiling" to 0
> would
> > put the upper limit at 0, effectively restricting the Count value to
> 0
> > until the value of "ceiling" is raised.
> >
> > If the device is unable to support a ceiling value of 0, you should
> > return -ERANGE rather than disable the ceiling.
> 
> OK, will check this.

HW supports ceiling value of 0.

Cheers,
Biju

Re: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

On Tue, Oct 11, 2022 at 05:55:50PM +0000, Biju Das wrote:
> > > > > Looks like something different is done when ceiling is set to 0.
> > > > > Would you explain what's happening in this case and why it's
> > > > > different that then else case below; in other words, what's the
> > > > > difference between RZ_MTU3_TCR_CCLR_NONE and
> > > RZ_MTU3_TCR_CCLR_TGRA?
> > > >
> > > > RZ_MTU3_TCR_CCLR_TGRA --> for triggering counter count using Z-
> > Phase
> > > signal.
> > > > RZ_MTU3_TCR_CCLR_NONE --> No clearing.
> > >
> > > Does the Z-Phase signal trigger a reset of the counter count back to
> > > the ceiling value?
> > 
> > No, It resets to 0.
> 
> It reset to 0 for forward counting
> and resets to U16_MAX or U32_MAX for backword counting

So when counting backwards, will the value reset to the ceiling value
when it passes 0?

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

On Tue, Oct 11, 2022 at 03:50:38PM +0000, Biju Das wrote:
> > To support the phase mode selection, implement a Counter device
> > extension for that specific purpose. You can use DEFINE_COUNTER_ENUM()
> > and COUNTER_COMP_DEVICE_ENUM() to create a device extension that will
> > allow users to toggle between "16-bit" and "32-bit" phase modes. If
> > you need help with these macros, just let me know.
> 
> Yes please, that will be helpful. 
> 
> Cheers,
> Biju

It'll look something like this::

    static const char *const rz_mtu3_phase_counting_modes[] = {
            "16-bit",
            "32-bit",
    };
    
    static int rz_mtu3_phase_counting_mode_get(struct counter_device *counter,
                                               u32 *phase_counting_mode);
    static int rz_mtu3_phase_counting_mode_set(struct counter_device *counter,
                                               u32 phase_counting_mode);
    
    static DEFINE_COUNTER_ENUM(rz_mtu3_phase_counting_mode_enum,
                               rz_mtu3_phase_counting_modes);
    
    static struct counter_comp rz_mtu3_device_ext[] = {
            COUNTER_COMP_DEVICE_ENUM("phase_counting_mode",
                                     rz_mtu3_phase_counting_mode_get,
                                     rz_mtu3_phase_counting_mode_set,
                                     rz_mtu3_phase_counting_mode_enum),
    };

Using the get/set callbacks, you can get/set the index for the
respective mode of your rz_mtu3_phase_counting_modes array.

In rz_mtu3_cnt_probe() you could set the counter_device ext member to
rz_mtu3_device_ext. This will make the extensions appear in the sysfs
tree for your Counter device. You should also add a new entry in
Documentation/ABI/testing/sysfs-bus-counter to document this new sysfs
file.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver

[Biju Das]

Hi William Breathitt Gray,

Thanks for the feedback.

> Subject: Re: [PATCH v3 4/4] counter: Add RZ/G2L MTU3 counter driver
> 
> On Tue, Oct 11, 2022 at 03:50:38PM +0000, Biju Das wrote:
> > > To support the phase mode selection, implement a Counter device
> > > extension for that specific purpose. You can use
> > > DEFINE_COUNTER_ENUM() and COUNTER_COMP_DEVICE_ENUM() to create a
> > > device extension that will allow users to toggle between "16-bit"
> > > and "32-bit" phase modes. If you need help with these macros, just
> let me know.
> >
> > Yes please, that will be helpful.
> >
> > Cheers,
> > Biju
> 
> It'll look something like this::
> 
>     static const char *const rz_mtu3_phase_counting_modes[] = {
>             "16-bit",
>             "32-bit",
>     };
> 
>     static int rz_mtu3_phase_counting_mode_get(struct counter_device
> *counter,
>                                                u32
> *phase_counting_mode);
>     static int rz_mtu3_phase_counting_mode_set(struct counter_device
> *counter,
>                                                u32
> phase_counting_mode);
> 
>     static DEFINE_COUNTER_ENUM(rz_mtu3_phase_counting_mode_enum,
>                                rz_mtu3_phase_counting_modes);
> 
>     static struct counter_comp rz_mtu3_device_ext[] = {
>             COUNTER_COMP_DEVICE_ENUM("phase_counting_mode",
>                                      rz_mtu3_phase_counting_mode_get,
>                                      rz_mtu3_phase_counting_mode_set,
> 
> rz_mtu3_phase_counting_mode_enum),
>     };
> 
> Using the get/set callbacks, you can get/set the index for the
> respective mode of your rz_mtu3_phase_counting_modes array.
> 
> In rz_mtu3_cnt_probe() you could set the counter_device ext member to
> rz_mtu3_device_ext. This will make the extensions appear in the sysfs
> tree for your Counter device. You should also add a new entry in
> Documentation/ABI/testing/sysfs-bus-counter to document this new sysfs
> file.

cool, Will do it in the next version v5.

Cheers,
Biju