diff mbox series

[RFC,5/8] counter: Add RZ/G2L MTU3 counter driver

Message ID 20220926132114.60396-6-biju.das.jz@bp.renesas.com (mailing list archive)
State RFC
Delegated to: Geert Uytterhoeven
Headers show
Series Add RZ/G2L MTU3a MFD and Counter driver | expand

Commit Message

Biju Das Sept. 26, 2022, 1:21 p.m. UTC 
Add RZ/G2L MTU3 counter driver. Currently it supports 16-bit phase
counting mode on MTU{1,2} channels.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/counter/Kconfig          |   9 +
 drivers/counter/Makefile         |   1 +
 drivers/counter/rzg2l-mtu3-cnt.c | 367 +++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/counter/rzg2l-mtu3-cnt.c

Comments

William Breathitt Gray Oct. 1, 2022, 12:22 a.m. UTC | #1 
On Mon, Sep 26, 2022 at 02:21:11PM +0100, Biju Das wrote:
> Add RZ/G2L MTU3 counter driver. Currently it supports 16-bit phase
> counting mode on MTU{1,2} channels.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Hi Biju,

This driver will likely change in your next revision, but I want to give
some feedback anyway on a few things I noticed. See the comments below.

> +struct rzg2l_mtu3_cnt {
> +	struct clk *clk;
> +	void __iomem *mmio;
> +	struct rzg2l_mtu3_channel *ch;
> +};

Add kernel-doc comments to document this structure. It seems that
neither clk nor mmio is access in the code from this structure; what's
the purpose of having them here?

> +static int rzg2l_mtu3_count_read(struct counter_device *counter,
> +				 struct counter_count *count, u64 *val)
> +{
> +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> +	u32 cnt;
> +
> +	cnt = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TCNT);
> +	*val = cnt;

The rzg2l_mtu3_16bit_ch_read() function returns a u16, so there's no
need for the cnt variable; just return the count via val directly.

> +static int rzg2l_mtu3_count_write(struct counter_device *counter,
> +				  struct counter_count *count, const u64 val)
> +{
> +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> +	u16 ceiling;
> +
> +	ceiling = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TGRA);
> +
> +	if (val > ceiling)
> +		return -EINVAL;

Return -ERANGE instead to indicate the request is outside the boundary.

> +
> +	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TCNT, (u16)val);

Remove the explicit cast to u16, it's already implicit in the call. You
probably also need some sort of lock in this function to ensure that
your ceiling value does not change before you write to the register.

> +static int rzg2l_mtu3_count_ceiling_read(struct counter_device *counter,
> +					 struct counter_count *count,
> +					 u64 *ceiling)
> +{
> +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> +	u32 val;
> +
> +	val = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TGRA);
> +	*ceiling = val;

Same comment as in rzg2l_mtu3_count_read().

> +static int rzg2l_mtu3_count_ceiling_write(struct counter_device *counter,
> +					  struct counter_count *count,
> +					  u64 ceiling)
> +{
> +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> +
> +	if (ceiling > U16_MAX)
> +		return -ERANGE;
> +
> +	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TGRA, (u16)ceiling);
> +	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TCR,
> +				 RZG2L_MTU3_TCR_CCLR_TGRA);

Same comments about cast and lock as in rzg2l_mtu3_count_write().

> +static int rzg2l_mtu3_count_enable_read(struct counter_device *counter,
> +					struct counter_count *count, u8 *enable)
> +{
> +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> +	int ch = priv->ch->index;
> +
> +	*enable = (rzg2l_mtu3_shared_reg_read(priv->ch, RZG2L_MTU3_TSTRA) &
> +		(0x1 << ch)) >> ch;

A lot of operations happening in a single line; can this be broken down
to clearer distinct steps?

> +static int rzg2l_mtu3_action_read(struct counter_device *counter,
> +				  struct counter_count *count,
> +				  struct counter_synapse *synapse,
> +				  enum counter_synapse_action *action)
> +{
> +	enum counter_function function;
> +	int err;
> +
> +	err = rzg2l_mtu3_count_function_read(counter, count, &function);
> +	if (err)
> +		return err;
> +
> +	switch (function) {
> +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> +		/*
> +		 * Rising edges on signal A updates the respective count.
> +		 * The input level of signal B determines direction.
> +		 */
> +		*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;

You need to differentiate between signal A and B here: the Synapse for
signal A will have an action mode of COUNTER_SYNAPSE_ACTION_RING_EDGE,
but the Synapse for Signal B will have an action mode of
COUNTER_SYNAPSE_ACTION_NONE because its not the trigger point for the
respective Count value update.

> +		break;
> +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> +		/*
> +		 * Any state transition on quadrature pair signal B updates
> +		 * the respective count.
> +		 */
> +		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;

Similar to above, you need to differentiate between signal A and B here
as well.

> +static struct counter_count rzg2l_mtu3_counts = {
> +	.id = 0,

The id member is an optional way for driver authors to identify their
own Counts; it can be set to anything your like, and if you don't use
it in your code then you don't need to set it at all.

> +static int rzg2l_mtu3_cnt_probe(struct platform_device *pdev)
> +{
> +	struct rzg2l_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct counter_device *counter;
> +	struct rzg2l_mtu3_cnt *priv;
> +	int ret;
> +	u32 ch;
> +
> +	if (IS_ERR_OR_NULL(ddata))
> +		return -EINVAL;

Is this actually possible? What situation would cause this, and why is
it not handled before we reach probe()?

> +
> +	counter = devm_counter_alloc(dev, sizeof(*priv));
> +	if (!counter)
> +		return -ENOMEM;
> +
> +	priv = counter_priv(counter);
> +
> +	ret = of_property_read_u32(dev->of_node, "reg", &ch);
> +	if (ret) {
> +		dev_err(dev, "%pOF: No reg property found\n", dev->of_node);
> +		return -EINVAL;
> +	}
> +
> +	if (ch != RZG2L_MTU1 && ch != RZG2L_MTU2) {
> +		dev_err(dev, "%pOF: Invalid channel '%u'\n", dev->of_node, ch);
> +		return -EINVAL;
> +	}
> +
> +	priv->clk = ddata->clk;
> +	priv->ch = &ddata->channels[ch];
> +	priv->ch->dev = dev;
> +
> +	counter->name = dev_name(dev);
> +	counter->parent = dev;
> +	counter->ops = &rzg2l_mtu3_cnt_ops;
> +	counter->counts = &rzg2l_mtu3_counts;
> +	counter->num_counts = 1;

Even though you only have one Count defined, use ARRAY_SIZE here for
consistency with the other Counter drivers as well as making the
intention of the code clear.

> +	counter->signals = rzg2l_mtu3_signals;
> +	counter->num_signals = ARRAY_SIZE(rzg2l_mtu3_signals);
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* Register Counter device */
> +	ret = devm_counter_add(dev, counter);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to add counter\n");

The Counter driver goes live with the call to devm_counter_add() so move
it to the end after your device initialization code below.

> +
> +	priv->ch->function = RZG2L_MTU3_16BIT_PHASE_COUNTING;
> +	ret = clk_prepare_enable(ddata->clk);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Phase counting mode 1 will be used as default
> +	 * when initializing counters.
> +	 */
> +	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TMDR1,
> +				 RZG2L_MTU3_TMDR1_PH_CNT_MODE_1);
> +
> +	/* Initialize 16-bit counter max value */
> +	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TCR,
> +				 RZG2L_MTU3_TCR_CCLR_TGRA);
> +	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TGRA, U16_MAX);
> +
> +	clk_disable(ddata->clk);

Should this be moved up near the clk_prepare_enable() call above?

> +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> +MODULE_ALIAS("platform:rzg2l-mtu3-counter");
> +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a counter driver");
> +MODULE_LICENSE("GPL");

Add MODULE_IMPORT_NS(COUNTER) to import the COUNTER namespace.

Make sure you're based on top of the counter-next branch. You can find
the Counter tree here: https://git.kernel.org/pub/scm/linux/kernel/git/wbg/counter.git

William Breathitt Gray
Biju Das Oct. 5, 2022, 10:29 a.m. UTC | #2 
Hi William Breathitt Gray,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 5/8] counter: Add RZ/G2L MTU3 counter driver
> 
> On Mon, Sep 26, 2022 at 02:21:11PM +0100, Biju Das wrote:
> > Add RZ/G2L MTU3 counter driver. Currently it supports 16-bit phase
> > counting mode on MTU{1,2} channels.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Hi Biju,
> 
> This driver will likely change in your next revision, but I want to
> give some feedback anyway on a few things I noticed. See the comments
> below.
> 
> > +struct rzg2l_mtu3_cnt {
> > +	struct clk *clk;
> > +	void __iomem *mmio;
> > +	struct rzg2l_mtu3_channel *ch;
> > +};
> 
> Add kernel-doc comments to document this structure. It seems that
> neither clk nor mmio is access in the code from this structure; what's
> the purpose of having them here?

OK, will do. mmio is not required. But clk is needed.

> 
> > +static int rzg2l_mtu3_count_read(struct counter_device *counter,
> > +				 struct counter_count *count, u64 *val) {
> > +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 cnt;
> > +
> > +	cnt = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TCNT);
> > +	*val = cnt;
> 
> The rzg2l_mtu3_16bit_ch_read() function returns a u16, so there's no
> need for the cnt variable; just return the count via val directly.

OK. Agreed.

> 
> > +static int rzg2l_mtu3_count_write(struct counter_device *counter,
> > +				  struct counter_count *count, const u64 val) {
> > +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> > +	u16 ceiling;
> > +
> > +	ceiling = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TGRA);
> > +
> > +	if (val > ceiling)
> > +		return -EINVAL;
> 
> Return -ERANGE instead to indicate the request is outside the
> boundary.

Ok. Agreed.

> 
> > +
> > +	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TCNT, (u16)val);
> 
> Remove the explicit cast to u16, it's already implicit in the call.
> You probably also need some sort of lock in this function to ensure
> that your ceiling value does not change before you write to the
> register.

OK agreed.

> 
> > +static int rzg2l_mtu3_count_ceiling_read(struct counter_device
> *counter,
> > +					 struct counter_count *count,
> > +					 u64 *ceiling)
> > +{
> > +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> > +	u32 val;
> > +
> > +	val = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TGRA);
> > +	*ceiling = val;
> 
> Same comment as in rzg2l_mtu3_count_read().

OK agreed.
> 
> > +static int rzg2l_mtu3_count_ceiling_write(struct counter_device
> *counter,
> > +					  struct counter_count *count,
> > +					  u64 ceiling)
> > +{
> > +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> > +
> > +	if (ceiling > U16_MAX)
> > +		return -ERANGE;
> > +
> > +	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TGRA,
> (u16)ceiling);
> > +	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TCR,
> > +				 RZG2L_MTU3_TCR_CCLR_TGRA);
> 
> Same comments about cast and lock as in rzg2l_mtu3_count_write().

OK agreed.

> 
> > +static int rzg2l_mtu3_count_enable_read(struct counter_device
> *counter,
> > +					struct counter_count *count, u8 *enable)
> {
> > +	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
> > +	int ch = priv->ch->index;
> > +
> > +	*enable = (rzg2l_mtu3_shared_reg_read(priv->ch, RZG2L_MTU3_TSTRA)
> &
> > +		(0x1 << ch)) >> ch;
> 
> A lot of operations happening in a single line; can this be broken
> down to clearer distinct steps?

OK agreed.

> 
> > +static int rzg2l_mtu3_action_read(struct counter_device *counter,
> > +				  struct counter_count *count,
> > +				  struct counter_synapse *synapse,
> > +				  enum counter_synapse_action *action) {
> > +	enum counter_function function;
> > +	int err;
> > +
> > +	err = rzg2l_mtu3_count_function_read(counter, count, &function);
> > +	if (err)
> > +		return err;
> > +
> > +	switch (function) {
> > +	case COUNTER_FUNCTION_PULSE_DIRECTION:
> > +		/*
> > +		 * Rising edges on signal A updates the respective count.
> > +		 * The input level of signal B determines direction.
> > +		 */
> > +		*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
> 
> You need to differentiate between signal A and B here: the Synapse for
> signal A will have an action mode of COUNTER_SYNAPSE_ACTION_RING_EDGE,
> but the Synapse for Signal B will have an action mode of
> COUNTER_SYNAPSE_ACTION_NONE because its not the trigger point for the
> respective Count value update.

OK, Will do.

> 
> > +		break;
> > +	case COUNTER_FUNCTION_QUADRATURE_X2_B:
> > +		/*
> > +		 * Any state transition on quadrature pair signal B updates
> > +		 * the respective count.
> > +		 */
> > +		*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> 
> Similar to above, you need to differentiate between signal A and B
> here as well.

OK, will do.

> 
> > +static struct counter_count rzg2l_mtu3_counts = {
> > +	.id = 0,
> 
> The id member is an optional way for driver authors to identify their
> own Counts; it can be set to anything your like, and if you don't use
> it in your code then you don't need to set it at all.

It is being used in the next version.

> 
> > +static int rzg2l_mtu3_cnt_probe(struct platform_device *pdev) {
> > +	struct rzg2l_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
> > +	struct device *dev = &pdev->dev;
> > +	struct counter_device *counter;
> > +	struct rzg2l_mtu3_cnt *priv;
> > +	int ret;
> > +	u32 ch;
> > +
> > +	if (IS_ERR_OR_NULL(ddata))
> > +		return -EINVAL;
> 
> Is this actually possible? What situation would cause this, and why is
> it not handled before we reach probe()?

Theoretically not required as parent device populates child devices.
I will remove it from here.

> 
> > +
> > +	counter = devm_counter_alloc(dev, sizeof(*priv));
> > +	if (!counter)
> > +		return -ENOMEM;
> > +
> > +	priv = counter_priv(counter);
> > +
> > +	ret = of_property_read_u32(dev->of_node, "reg", &ch);
> > +	if (ret) {
> > +		dev_err(dev, "%pOF: No reg property found\n", dev-
> >of_node);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ch != RZG2L_MTU1 && ch != RZG2L_MTU2) {
> > +		dev_err(dev, "%pOF: Invalid channel '%u'\n", dev->of_node,
> ch);
> > +		return -EINVAL;
> > +	}
> > +
> > +	priv->clk = ddata->clk;
> > +	priv->ch = &ddata->channels[ch];
> > +	priv->ch->dev = dev;
> > +
> > +	counter->name = dev_name(dev);
> > +	counter->parent = dev;
> > +	counter->ops = &rzg2l_mtu3_cnt_ops;
> > +	counter->counts = &rzg2l_mtu3_counts;
> > +	counter->num_counts = 1;
> 
> Even though you only have one Count defined, use ARRAY_SIZE here for
> consistency with the other Counter drivers as well as making the
> intention of the code clear.

OK will use array.

> 
> > +	counter->signals = rzg2l_mtu3_signals;
> > +	counter->num_signals = ARRAY_SIZE(rzg2l_mtu3_signals);
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	/* Register Counter device */
> > +	ret = devm_counter_add(dev, counter);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to add counter\n");
> 
> The Counter driver goes live with the call to devm_counter_add() so
> move it to the end after your device initialization code below.

OK. Agreed.

> 
> > +
> > +	priv->ch->function = RZG2L_MTU3_16BIT_PHASE_COUNTING;
> > +	ret = clk_prepare_enable(ddata->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Phase counting mode 1 will be used as default
> > +	 * when initializing counters.
> > +	 */
> > +	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TMDR1,
> > +				 RZG2L_MTU3_TMDR1_PH_CNT_MODE_1);
> > +
> > +	/* Initialize 16-bit counter max value */
> > +	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TCR,
> > +				 RZG2L_MTU3_TCR_CCLR_TGRA);
> > +	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TGRA, U16_MAX);
> > +
> > +	clk_disable(ddata->clk);
> 
> Should this be moved up near the clk_prepare_enable() call above?

OK.

> 
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
> > +MODULE_ALIAS("platform:rzg2l-mtu3-counter");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a counter driver");
> > +MODULE_LICENSE("GPL");
> 
> Add MODULE_IMPORT_NS(COUNTER) to import the COUNTER namespace.

OK.

> 
> Make sure you're based on top of the counter-next branch. You can find
> the Counter tree here:
> https://git.kernel.org/pub/scm/linux/kernel/git/wbg/counter.git

Agreed.


Cheers,
Biju
diff mbox series

Patch

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 5edd155f1911..6bdc0756f9c4 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 RZG2L_MTU3_CNT
+	tristate "RZ/G2L MTU3 counter driver"
+	depends on MFD_RZG2L_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 rzg2l-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 8fde6c100ebc..f9138f3e14f7 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_RZG2L_MTU3_CNT)	+= rzg2l-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/rzg2l-mtu3-cnt.c b/drivers/counter/rzg2l-mtu3-cnt.c
new file mode 100644
index 000000000000..c324cd831f1d
--- /dev/null
+++ b/drivers/counter/rzg2l-mtu3-cnt.c
@@ -0,0 +1,367 @@ 
+// 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/rzg2l-mtu3.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#define RZG2L_MTU3_TSR_TCFD	BIT(7)
+
+#define RZG2L_MTU3_TMDR1_PH_CNT_MODE_1	(4)
+#define RZG2L_MTU3_TMDR1_PH_CNT_MODE_2	(5)
+#define RZG2L_MTU3_TMDR1_PH_CNT_MODE_3	(6)
+#define RZG2L_MTU3_TMDR1_PH_CNT_MODE_4	(7)
+#define RZG2L_MTU3_TMDR1_PH_CNT_MODE_5	(9)
+#define RZG2L_MTU3_TMDR1_PH_CNT_MODE_MASK	(0xf)
+
+struct rzg2l_mtu3_cnt {
+	struct clk *clk;
+	void __iomem *mmio;
+	struct rzg2l_mtu3_channel *ch;
+};
+
+static const enum counter_function rzg2l_mtu3_count_functions[] = {
+	COUNTER_FUNCTION_QUADRATURE_X4,
+	COUNTER_FUNCTION_PULSE_DIRECTION,
+	COUNTER_FUNCTION_QUADRATURE_X2_B,
+};
+
+static int rzg2l_mtu3_count_read(struct counter_device *counter,
+				 struct counter_count *count, u64 *val)
+{
+	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
+	u32 cnt;
+
+	cnt = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TCNT);
+	*val = cnt;
+
+	return 0;
+}
+
+static int rzg2l_mtu3_count_write(struct counter_device *counter,
+				  struct counter_count *count, const u64 val)
+{
+	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
+	u16 ceiling;
+
+	ceiling = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TGRA);
+
+	if (val > ceiling)
+		return -EINVAL;
+
+	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TCNT, (u16)val);
+
+	return 0;
+}
+
+static int rzg2l_mtu3_count_function_read(struct counter_device *counter,
+					  struct counter_count *count,
+					  enum counter_function *function)
+{
+	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
+	u8 val;
+
+	val = rzg2l_mtu3_8bit_ch_read(priv->ch, RZG2L_MTU3_TMDR1);
+
+	switch (val & RZG2L_MTU3_TMDR1_PH_CNT_MODE_MASK) {
+	case RZG2L_MTU3_TMDR1_PH_CNT_MODE_1:
+		*function = COUNTER_FUNCTION_QUADRATURE_X4;
+		break;
+	case RZG2L_MTU3_TMDR1_PH_CNT_MODE_2:
+		*function = COUNTER_FUNCTION_PULSE_DIRECTION;
+		break;
+	case RZG2L_MTU3_TMDR1_PH_CNT_MODE_4:
+		*function = COUNTER_FUNCTION_QUADRATURE_X2_B;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rzg2l_mtu3_count_function_write(struct counter_device *counter,
+					   struct counter_count *count,
+					   enum counter_function function)
+{
+	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
+	u8 mode;
+
+	switch (function) {
+	case COUNTER_FUNCTION_QUADRATURE_X4:
+		mode = RZG2L_MTU3_TMDR1_PH_CNT_MODE_1;
+		break;
+	case COUNTER_FUNCTION_PULSE_DIRECTION:
+		mode = RZG2L_MTU3_TMDR1_PH_CNT_MODE_2;
+		break;
+	case COUNTER_FUNCTION_QUADRATURE_X2_B:
+		mode = RZG2L_MTU3_TMDR1_PH_CNT_MODE_4;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TMDR1, mode);
+
+	return 0;
+}
+
+static int rzg2l_mtu3_count_direction_read(struct counter_device *counter,
+					   struct counter_count *count,
+					   enum counter_count_direction *direction)
+{
+	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
+	u8 cnt;
+
+	cnt = rzg2l_mtu3_8bit_ch_read(priv->ch, RZG2L_MTU3_TSR);
+
+	if (cnt & RZG2L_MTU3_TSR_TCFD)
+		*direction = COUNTER_COUNT_DIRECTION_FORWARD;
+	else
+		*direction = COUNTER_COUNT_DIRECTION_BACKWARD;
+
+	return 0;
+}
+
+static int rzg2l_mtu3_count_ceiling_read(struct counter_device *counter,
+					 struct counter_count *count,
+					 u64 *ceiling)
+{
+	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
+	u32 val;
+
+	val = rzg2l_mtu3_16bit_ch_read(priv->ch, RZG2L_MTU3_TGRA);
+	*ceiling = val;
+
+	return 0;
+}
+
+static int rzg2l_mtu3_count_ceiling_write(struct counter_device *counter,
+					  struct counter_count *count,
+					  u64 ceiling)
+{
+	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
+
+	if (ceiling > U16_MAX)
+		return -ERANGE;
+
+	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TGRA, (u16)ceiling);
+	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TCR,
+				 RZG2L_MTU3_TCR_CCLR_TGRA);
+
+	return 0;
+}
+
+static int rzg2l_mtu3_count_enable_read(struct counter_device *counter,
+					struct counter_count *count, u8 *enable)
+{
+	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
+	int ch = priv->ch->index;
+
+	*enable = (rzg2l_mtu3_shared_reg_read(priv->ch, RZG2L_MTU3_TSTRA) &
+		(0x1 << ch)) >> ch;
+
+	return 0;
+}
+
+static int rzg2l_mtu3_count_enable_write(struct counter_device *counter,
+					 struct counter_count *count, u8 enable)
+{
+	struct rzg2l_mtu3_cnt *const priv = counter_priv(counter);
+
+	if (enable)
+		rzg2l_mtu3_enable(priv->ch);
+	else
+		rzg2l_mtu3_disable(priv->ch);
+
+	return 0;
+}
+
+static struct counter_comp rzg2l_mtu3_count_ext[] = {
+	COUNTER_COMP_DIRECTION(rzg2l_mtu3_count_direction_read),
+	COUNTER_COMP_ENABLE(rzg2l_mtu3_count_enable_read,
+			    rzg2l_mtu3_count_enable_write),
+	COUNTER_COMP_CEILING(rzg2l_mtu3_count_ceiling_read,
+			     rzg2l_mtu3_count_ceiling_write),
+};
+
+static const enum counter_synapse_action rzg2l_mtu3_synapse_actions[] = {
+	COUNTER_SYNAPSE_ACTION_NONE,
+	COUNTER_SYNAPSE_ACTION_BOTH_EDGES
+};
+
+static int rzg2l_mtu3_action_read(struct counter_device *counter,
+				  struct counter_count *count,
+				  struct counter_synapse *synapse,
+				  enum counter_synapse_action *action)
+{
+	enum counter_function function;
+	int err;
+
+	err = rzg2l_mtu3_count_function_read(counter, count, &function);
+	if (err)
+		return err;
+
+	switch (function) {
+	case COUNTER_FUNCTION_PULSE_DIRECTION:
+		/*
+		 * Rising edges on signal A updates the respective count.
+		 * The input level of signal B determines direction.
+		 */
+		*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.
+		 */
+		*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 rzg2l_mtu3_cnt_ops = {
+	.count_read = rzg2l_mtu3_count_read,
+	.count_write = rzg2l_mtu3_count_write,
+	.function_read = rzg2l_mtu3_count_function_read,
+	.function_write = rzg2l_mtu3_count_function_write,
+	.action_read = rzg2l_mtu3_action_read,
+};
+
+static struct counter_signal rzg2l_mtu3_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 1 Quadrature A"
+	},
+	{
+		.id = 1,
+		.name = "Channel 1 Quadrature B"
+	}
+};
+
+static struct counter_synapse rzg2l_mtu3_count_synapses[] = {
+	{
+		.actions_list = rzg2l_mtu3_synapse_actions,
+		.num_actions = ARRAY_SIZE(rzg2l_mtu3_synapse_actions),
+		.signal = &rzg2l_mtu3_signals[0]
+	},
+	{
+		.actions_list = rzg2l_mtu3_synapse_actions,
+		.num_actions = ARRAY_SIZE(rzg2l_mtu3_synapse_actions),
+		.signal = &rzg2l_mtu3_signals[1]
+	}
+};
+
+static struct counter_count rzg2l_mtu3_counts = {
+	.id = 0,
+	.name = "Channel 1 Count",
+	.functions_list = rzg2l_mtu3_count_functions,
+	.num_functions = ARRAY_SIZE(rzg2l_mtu3_count_functions),
+	.synapses = rzg2l_mtu3_count_synapses,
+	.num_synapses = ARRAY_SIZE(rzg2l_mtu3_count_synapses),
+	.ext = rzg2l_mtu3_count_ext,
+	.num_ext = ARRAY_SIZE(rzg2l_mtu3_count_ext)
+};
+
+static int rzg2l_mtu3_cnt_probe(struct platform_device *pdev)
+{
+	struct rzg2l_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct counter_device *counter;
+	struct rzg2l_mtu3_cnt *priv;
+	int ret;
+	u32 ch;
+
+	if (IS_ERR_OR_NULL(ddata))
+		return -EINVAL;
+
+	counter = devm_counter_alloc(dev, sizeof(*priv));
+	if (!counter)
+		return -ENOMEM;
+
+	priv = counter_priv(counter);
+
+	ret = of_property_read_u32(dev->of_node, "reg", &ch);
+	if (ret) {
+		dev_err(dev, "%pOF: No reg property found\n", dev->of_node);
+		return -EINVAL;
+	}
+
+	if (ch != RZG2L_MTU1 && ch != RZG2L_MTU2) {
+		dev_err(dev, "%pOF: Invalid channel '%u'\n", dev->of_node, ch);
+		return -EINVAL;
+	}
+
+	priv->clk = ddata->clk;
+	priv->ch = &ddata->channels[ch];
+	priv->ch->dev = dev;
+
+	counter->name = dev_name(dev);
+	counter->parent = dev;
+	counter->ops = &rzg2l_mtu3_cnt_ops;
+	counter->counts = &rzg2l_mtu3_counts;
+	counter->num_counts = 1;
+	counter->signals = rzg2l_mtu3_signals;
+	counter->num_signals = ARRAY_SIZE(rzg2l_mtu3_signals);
+	platform_set_drvdata(pdev, priv);
+
+	/* Register Counter device */
+	ret = devm_counter_add(dev, counter);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to add counter\n");
+
+	priv->ch->function = RZG2L_MTU3_16BIT_PHASE_COUNTING;
+	ret = clk_prepare_enable(ddata->clk);
+	if (ret)
+		return ret;
+
+	/*
+	 * Phase counting mode 1 will be used as default
+	 * when initializing counters.
+	 */
+	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TMDR1,
+				 RZG2L_MTU3_TMDR1_PH_CNT_MODE_1);
+
+	/* Initialize 16-bit counter max value */
+	rzg2l_mtu3_8bit_ch_write(priv->ch, RZG2L_MTU3_TCR,
+				 RZG2L_MTU3_TCR_CCLR_TGRA);
+	rzg2l_mtu3_16bit_ch_write(priv->ch, RZG2L_MTU3_TGRA, U16_MAX);
+
+	clk_disable(ddata->clk);
+
+	return 0;
+}
+
+static const struct of_device_id rzg2l_mtu3_cnt_of_match[] = {
+	{ .compatible = "renesas,rzg2l-mtu3-counter", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzg2l_mtu3_cnt_of_match);
+
+static struct platform_driver rzg2l_mtu3_cnt_driver = {
+	.probe = rzg2l_mtu3_cnt_probe,
+	.driver = {
+		.name = "rzg2l-mtu3-counter",
+		.of_match_table = rzg2l_mtu3_cnt_of_match,
+	},
+};
+module_platform_driver(rzg2l_mtu3_cnt_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_ALIAS("platform:rzg2l-mtu3-counter");
+MODULE_DESCRIPTION("Renesas RZ/G2L MTU3a counter driver");
+MODULE_LICENSE("GPL");