[PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[Biju Das]

The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded in
the Renesas RZ/G2L family SoC's. It consists of eight 16-bit timer
channels and one 32-bit timer channel. It supports the following
functions
 - Counter
 - Timer
 - PWM

This patch series aim to add MFD and counter driver for MTU3a.
Subsequent patch seies will add TImer and PWM driver support
also enhancements to counter driver.

The 8/16/32 bit registers are mixed in each channel. The HW
specifications of the IP is described in patch#2.

Current patch set is tested for 16-bit phase counting mode on
MTU1 channel.

Please share your valuable comments on this patch series.

Biju Das (8):
  clk: renesas: r9a07g044: Add MTU3a clock and reset entry
  dt-bindings: mfd: Document RZ/G2L MTU3a bindings
  mfd: Add RZ/G2L MTU3 driver
  dt-bindings: mfd: rzg2l-mtu3: Document RZ/G2UL MTU3 counter
  counter: Add RZ/G2L MTU3 counter driver
  arm64: dts: renesas: r9a07g044: Add MTU3a node
  arm64: dts: renesas: r9a07g054: Add MTU3a node
  arm64: dts: renesas: rzg2l-smarc: [HACK] Enable MTU for 16-bit phase
    count testing

 .../bindings/mfd/renesas,rzg2l-mtu3.yaml      | 310 ++++++++++++++
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi    |  83 ++++
 .../boot/dts/renesas/r9a07g044l2-smarc.dts    |   2 -
 arch/arm64/boot/dts/renesas/r9a07g054.dtsi    |  83 ++++
 .../dts/renesas/rzg2l-smarc-pinfunction.dtsi  |  11 +
 arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi  |  25 +-
 drivers/clk/renesas/r9a07g044-cpg.c           |   5 +-
 drivers/counter/Kconfig                       |   9 +
 drivers/counter/Makefile                      |   1 +
 drivers/counter/rzg2l-mtu3-cnt.c              | 367 +++++++++++++++++
 drivers/mfd/Kconfig                           |   9 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/rzg2l-mtu3.c                      | 377 ++++++++++++++++++
 include/linux/mfd/rzg2l-mtu3.h                | 124 ++++++
 14 files changed, 1403 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/renesas,rzg2l-mtu3.yaml
 create mode 100644 drivers/counter/rzg2l-mtu3-cnt.c
 create mode 100644 drivers/mfd/rzg2l-mtu3.c
 create mode 100644 include/linux/mfd/rzg2l-mtu3.h

-- 
2.25.1

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

[Biju Das]

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

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");
-- 
2.25.1

Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[William Breathitt Gray]

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

On Mon, Sep 26, 2022 at 02:21:06PM +0100, Biju Das wrote:
> The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded in
> the Renesas RZ/G2L family SoC's. It consists of eight 16-bit timer
> channels and one 32-bit timer channel. It supports the following
> functions
>  - Counter
>  - Timer
>  - PWM
> 
> This patch series aim to add MFD and counter driver for MTU3a.
> Subsequent patch seies will add TImer and PWM driver support
> also enhancements to counter driver.

Hello Biju,

I see this device consists of several channels, but only one Count is
defined in the counter patch ("Channel 1 Count"). Do all channels
support counting, or is it limited to just one channel?

Thanks,

William Breathitt Gray

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

RE: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[Biju Das]

Hi William Breathitt Gray,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver
> 
> On Mon, Sep 26, 2022 at 02:21:06PM +0100, Biju Das wrote:
> > The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded in
> > the Renesas RZ/G2L family SoC's. It consists of eight 16-bit timer
> > channels and one 32-bit timer channel. It supports the following
> > functions
> >  - Counter
> >  - Timer
> >  - PWM
> >
> > This patch series aim to add MFD and counter driver for MTU3a.
> > Subsequent patch seies will add TImer and PWM driver support also
> > enhancements to counter driver.
> 
> Hello Biju,
> 
> I see this device consists of several channels, but only one Count is
> defined in the counter patch ("Channel 1 Count"). Do all channels
> support counting, or is it limited to just one channel?

It is like this
MTU1 channel :- 1 16-bit phase counter
MTU2-Channel :- 1 16-bit phase counter
MTU1 + MTU2 channel combined:- 1 32-bit phase counter
Other channels are not supporting phase counting.

Each counter device will have 1 channel. Currently it supports
16-bit phase counting.

Please see my test program. Am I missing something here?

My test program:-

echo 1 > /sys/bus/counter/devices/counter0/count0/enable
echo 50 > /sys/bus/counter/devices/counter0/count0/ceiling
devmem2 0x10001391 b 0x00 # Enable phase clock selection A for MTU2.
echo 1 > /sys/bus/counter/devices/counter1/count0/enable
echo 50 > /sys/bus/counter/devices/counter1/count0/ceiling

for i in {1..5};
do cat /sys/bus/counter/devices/counter0/count0/count ;
cat /sys/bus/counter/devices/counter0/count0/direction;
cat /sys/bus/counter/devices/counter1/count0/count;
cat /sys/bus/counter/devices/counter1/count0/direction;
done

Cheers,
Biju

Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[William Breathitt Gray]

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

On Wed, Sep 28, 2022 at 06:14:57AM +0000, Biju Das wrote:
> Hi William Breathitt Gray,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver
> > 
> > On Mon, Sep 26, 2022 at 02:21:06PM +0100, Biju Das wrote:
> > > The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded in
> > > the Renesas RZ/G2L family SoC's. It consists of eight 16-bit timer
> > > channels and one 32-bit timer channel. It supports the following
> > > functions
> > >  - Counter
> > >  - Timer
> > >  - PWM
> > >
> > > This patch series aim to add MFD and counter driver for MTU3a.
> > > Subsequent patch seies will add TImer and PWM driver support also
> > > enhancements to counter driver.
> > 
> > Hello Biju,
> > 
> > I see this device consists of several channels, but only one Count is
> > defined in the counter patch ("Channel 1 Count"). Do all channels
> > support counting, or is it limited to just one channel?
> 
> It is like this
> MTU1 channel :- 1 16-bit phase counter
> MTU2-Channel :- 1 16-bit phase counter
> MTU1 + MTU2 channel combined:- 1 32-bit phase counter
> Other channels are not supporting phase counting.
> 
> Each counter device will have 1 channel. Currently it supports
> 16-bit phase counting.
> 
> Please see my test program. Am I missing something here?
> 
> My test program:-
> 
> echo 1 > /sys/bus/counter/devices/counter0/count0/enable
> echo 50 > /sys/bus/counter/devices/counter0/count0/ceiling
> devmem2 0x10001391 b 0x00 # Enable phase clock selection A for MTU2.
> echo 1 > /sys/bus/counter/devices/counter1/count0/enable
> echo 50 > /sys/bus/counter/devices/counter1/count0/ceiling
> 
> for i in {1..5};
> do cat /sys/bus/counter/devices/counter0/count0/count ;
> cat /sys/bus/counter/devices/counter0/count0/direction;
> cat /sys/bus/counter/devices/counter1/count0/count;
> cat /sys/bus/counter/devices/counter1/count0/direction;
> done
> 
> Cheers,
> Biju

I'm not familiar with this hardware, but it looks like MTU1 and MTU2 are
on the same device. I think a more natural way to expose this
functionality in the Counter subsystem would be to define a Count for
each count value you can support; so something like this (all under
/sys/bus/counter/devices/counter0):

* count0 :- MTU1
* count1 :- MTU2
* count3 :- MTU1 + MTU2

You can then control the phase selection using a top-level Counter
device extension (e.g. /sys/bus/counter/devices/counter0/phase) that
configures whether you're in 16-bit phase or 32-phase counting mode.

William Breathitt Gray

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

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

[William Breathitt Gray]

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

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

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

RE: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[Biju Das]

Hi William Breathitt Gray,

Thanks for the feedback.

> Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver
> 
> On Wed, Sep 28, 2022 at 06:14:57AM +0000, Biju Das wrote:
> > Hi William Breathitt Gray,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter
> driver
> > >
> > > On Mon, Sep 26, 2022 at 02:21:06PM +0100, Biju Das wrote:
> > > > The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded
> > > > in the Renesas RZ/G2L family SoC's. It consists of eight 16-bit
> > > > timer channels and one 32-bit timer channel. It supports the
> > > > following functions
> > > >  - Counter
> > > >  - Timer
> > > >  - PWM
> > > >
> > > > This patch series aim to add MFD and counter driver for MTU3a.
> > > > Subsequent patch seies will add TImer and PWM driver support
> also
> > > > enhancements to counter driver.
> > >
> > > Hello Biju,
> > >
> > > I see this device consists of several channels, but only one Count
> > > is defined in the counter patch ("Channel 1 Count"). Do all
> channels
> > > support counting, or is it limited to just one channel?
> >
> > It is like this
> > MTU1 channel :- 1 16-bit phase counter MTU2-Channel :- 1 16-bit
> phase
> > counter
> > MTU1 + MTU2 channel combined:- 1 32-bit phase counter Other channels
> > are not supporting phase counting.
> >
> > Each counter device will have 1 channel. Currently it supports 16-
> bit
> > phase counting.
> >
> > Please see my test program. Am I missing something here?
> >
> > My test program:-
> >
> > echo 1 > /sys/bus/counter/devices/counter0/count0/enable
> > echo 50 > /sys/bus/counter/devices/counter0/count0/ceiling
> > devmem2 0x10001391 b 0x00 # Enable phase clock selection A for MTU2.
> > echo 1 > /sys/bus/counter/devices/counter1/count0/enable
> > echo 50 > /sys/bus/counter/devices/counter1/count0/ceiling
> >
> > for i in {1..5};
> > do cat /sys/bus/counter/devices/counter0/count0/count ; cat
> > /sys/bus/counter/devices/counter0/count0/direction;
> > cat /sys/bus/counter/devices/counter1/count0/count;
> > cat /sys/bus/counter/devices/counter1/count0/direction;
> > done
> >
> > Cheers,
> > Biju
> 
> I'm not familiar with this hardware, but it looks like MTU1 and MTU2
> are on the same device. I think a more natural way to expose this
> functionality in the Counter subsystem would be to define a Count for
> each count value you can support; so something like this (all under
> /sys/bus/counter/devices/counter0):
> 
> * count0 :- MTU1
> * count1 :- MTU2
> * count3 :- MTU1 + MTU2

OK, sounds good. so count3 :- 32 bit phase counting
count 0 or count1 or both then 16 bit phase counting

> 
> You can then control the phase selection using a top-level Counter
> device extension (e.g. /sys/bus/counter/devices/counter0/phase) that
> configures whether you're in 16-bit phase or 32-phase counting mode.

So I need to introduce a new sysfs called phase. Use that one for
Selecting the external clock pin for phase counting mode.
Please correct me if I am wrong??

Hardware supports 4 pins for phase counting mode,

MTCLKA Input External clock A input pin (MTU1/MTU2 phase counting mode A phase input)
MTCLKB Input External clock B input pin (MTU1/MTU2 phase counting mode B phase input)
MTCLKC Input External clock C input pin (MTU2 phase counting mode A phase input)
MTCLKD Input External clock D input pin (MTU2 phase counting mode B phase input)

For MTU1, it is fixed MTCLKA and MTCLKB.
But for MTU2, it can be either 0-{ MTCLKA, MTCLKB} or 1 - { MTCLKC , MTCLKD}
On reset it is set to { MTCLKC , MTCLKD}.

Cheers,
Biju

Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[William Breathitt Gray]

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

On Sat, Oct 01, 2022 at 04:45:55PM +0000, Biju Das wrote:
> Hi William Breathitt Gray,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver
> > 
> > On Wed, Sep 28, 2022 at 06:14:57AM +0000, Biju Das wrote:
> > > Hi William Breathitt Gray,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter
> > driver
> > > >
> > > > On Mon, Sep 26, 2022 at 02:21:06PM +0100, Biju Das wrote:
> > > > > The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is embedded
> > > > > in the Renesas RZ/G2L family SoC's. It consists of eight 16-bit
> > > > > timer channels and one 32-bit timer channel. It supports the
> > > > > following functions
> > > > >  - Counter
> > > > >  - Timer
> > > > >  - PWM
> > > > >
> > > > > This patch series aim to add MFD and counter driver for MTU3a.
> > > > > Subsequent patch seies will add TImer and PWM driver support
> > also
> > > > > enhancements to counter driver.
> > > >
> > > > Hello Biju,
> > > >
> > > > I see this device consists of several channels, but only one Count
> > > > is defined in the counter patch ("Channel 1 Count"). Do all
> > channels
> > > > support counting, or is it limited to just one channel?
> > >
> > > It is like this
> > > MTU1 channel :- 1 16-bit phase counter MTU2-Channel :- 1 16-bit
> > phase
> > > counter
> > > MTU1 + MTU2 channel combined:- 1 32-bit phase counter Other channels
> > > are not supporting phase counting.
> > >
> > > Each counter device will have 1 channel. Currently it supports 16-
> > bit
> > > phase counting.
> > >
> > > Please see my test program. Am I missing something here?
> > >
> > > My test program:-
> > >
> > > echo 1 > /sys/bus/counter/devices/counter0/count0/enable
> > > echo 50 > /sys/bus/counter/devices/counter0/count0/ceiling
> > > devmem2 0x10001391 b 0x00 # Enable phase clock selection A for MTU2.
> > > echo 1 > /sys/bus/counter/devices/counter1/count0/enable
> > > echo 50 > /sys/bus/counter/devices/counter1/count0/ceiling
> > >
> > > for i in {1..5};
> > > do cat /sys/bus/counter/devices/counter0/count0/count ; cat
> > > /sys/bus/counter/devices/counter0/count0/direction;
> > > cat /sys/bus/counter/devices/counter1/count0/count;
> > > cat /sys/bus/counter/devices/counter1/count0/direction;
> > > done
> > >
> > > Cheers,
> > > Biju
> > 
> > I'm not familiar with this hardware, but it looks like MTU1 and MTU2
> > are on the same device. I think a more natural way to expose this
> > functionality in the Counter subsystem would be to define a Count for
> > each count value you can support; so something like this (all under
> > /sys/bus/counter/devices/counter0):
> > 
> > * count0 :- MTU1
> > * count1 :- MTU2
> > * count3 :- MTU1 + MTU2
> 
> OK, sounds good. so count3 :- 32 bit phase counting
> count 0 or count1 or both then 16 bit phase counting

That "count3" should be "count2" (sorry for the typo), but yes all three
Counts should be defined; if a particular Count can't be read/written
due to the current phase counting mode selected, you can return -EBUSY
or -EINVAL as appropriate.

To clarify one more time, do you have two 16-bit registers holding count
values (one for MTU1 and one for MTU2), and when configured for 32-bit
phase counting mode you combine both registers to give you a 32-bit
count value?

> > 
> > You can then control the phase selection using a top-level Counter
> > device extension (e.g. /sys/bus/counter/devices/counter0/phase) that
> > configures whether you're in 16-bit phase or 32-phase counting mode.
> 
> So I need to introduce a new sysfs called phase. Use that one for
> Selecting the external clock pin for phase counting mode.
> Please correct me if I am wrong??
> 
> Hardware supports 4 pins for phase counting mode,
> 
> MTCLKA Input External clock A input pin (MTU1/MTU2 phase counting mode A phase input)
> MTCLKB Input External clock B input pin (MTU1/MTU2 phase counting mode B phase input)
> MTCLKC Input External clock C input pin (MTU2 phase counting mode A phase input)
> MTCLKD Input External clock D input pin (MTU2 phase counting mode B phase input)
> 
> For MTU1, it is fixed MTCLKA and MTCLKB.
> But for MTU2, it can be either 0-{ MTCLKA, MTCLKB} or 1 - { MTCLKC , MTCLKD}
> On reset it is set to { MTCLKC , MTCLKD}.
> 
> Cheers,
> Biju

It doesn't need to be named "phase" specifically, but it seems like a
new sysfs file will be necessary in order to select the proper phase
counting mode.

Are these MTCLK signals the quadrature A and B Signals you defined in
the counter driver?

William Breathitt Gray

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

RE: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[Biju Das]

> Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver
> 
> On Sat, Oct 01, 2022 at 04:45:55PM +0000, Biju Das wrote:
> > Hi William Breathitt Gray,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter
> driver
> > >
> > > On Wed, Sep 28, 2022 at 06:14:57AM +0000, Biju Das wrote:
> > > > Hi William Breathitt Gray,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter
> > > driver
> > > > >
> > > > > On Mon, Sep 26, 2022 at 02:21:06PM +0100, Biju Das wrote:
> > > > > > The RZ/G2L multi-function timer pulse unit 3 (MTU3a) is
> > > > > > embedded in the Renesas RZ/G2L family SoC's. It consists of
> > > > > > eight 16-bit timer channels and one 32-bit timer channel. It
> > > > > > supports the following functions
> > > > > >  - Counter
> > > > > >  - Timer
> > > > > >  - PWM
> > > > > >
> > > > > > This patch series aim to add MFD and counter driver for
> MTU3a.
> > > > > > Subsequent patch seies will add TImer and PWM driver support
> > > also
> > > > > > enhancements to counter driver.
> > > > >
> > > > > Hello Biju,
> > > > >
> > > > > I see this device consists of several channels, but only one
> > > > > Count is defined in the counter patch ("Channel 1 Count"). Do
> > > > > all
> > > channels
> > > > > support counting, or is it limited to just one channel?
> > > >
> > > > It is like this
> > > > MTU1 channel :- 1 16-bit phase counter MTU2-Channel :- 1 16-bit
> > > phase
> > > > counter
> > > > MTU1 + MTU2 channel combined:- 1 32-bit phase counter Other
> > > > channels are not supporting phase counting.
> > > >
> > > > Each counter device will have 1 channel. Currently it supports
> 16-
> > > bit
> > > > phase counting.
> > > >
> > > > Please see my test program. Am I missing something here?
> > > >
> > > > My test program:-
> > > >
> > > > echo 1 > /sys/bus/counter/devices/counter0/count0/enable
> > > > echo 50 > /sys/bus/counter/devices/counter0/count0/ceiling
> > > > devmem2 0x10001391 b 0x00 # Enable phase clock selection A for
> MTU2.
> > > > echo 1 > /sys/bus/counter/devices/counter1/count0/enable
> > > > echo 50 > /sys/bus/counter/devices/counter1/count0/ceiling
> > > >
> > > > for i in {1..5};
> > > > do cat /sys/bus/counter/devices/counter0/count0/count ; cat
> > > > /sys/bus/counter/devices/counter0/count0/direction;
> > > > cat /sys/bus/counter/devices/counter1/count0/count;
> > > > cat /sys/bus/counter/devices/counter1/count0/direction;
> > > > done
> > > >
> > > > Cheers,
> > > > Biju
> > >
> > > I'm not familiar with this hardware, but it looks like MTU1 and
> MTU2
> > > are on the same device. I think a more natural way to expose this
> > > functionality in the Counter subsystem would be to define a Count
> > > for each count value you can support; so something like this (all
> > > under
> > > /sys/bus/counter/devices/counter0):
> > >
> > > * count0 :- MTU1
> > > * count1 :- MTU2
> > > * count3 :- MTU1 + MTU2
> >
> > OK, sounds good. so count3 :- 32 bit phase counting count 0 or
> count1
> > or both then 16 bit phase counting
> 
> That "count3" should be "count2" (sorry for the typo), but yes all
> three Counts should be defined; if a particular Count can't be
> read/written due to the current phase counting mode selected, you can
> return -EBUSY or -EINVAL as appropriate.
> 

OK.

> To clarify one more time, do you have two 16-bit registers holding
> count values (one for MTU1 and one for MTU2), and when configured for
> 32-bit phase counting mode you combine both registers to give you a
> 32-bit count value?

Yes, that is correct.

> 
> > >
> > > You can then control the phase selection using a top-level Counter
> > > device extension (e.g. /sys/bus/counter/devices/counter0/phase)
> that
> > > configures whether you're in 16-bit phase or 32-phase counting
> mode.
> >
> > So I need to introduce a new sysfs called phase. Use that one for
> > Selecting the external clock pin for phase counting mode.
> > Please correct me if I am wrong??
> >
> > Hardware supports 4 pins for phase counting mode,
> >
> > MTCLKA Input External clock A input pin (MTU1/MTU2 phase counting
> mode
> > A phase input) MTCLKB Input External clock B input pin (MTU1/MTU2
> > phase counting mode B phase input) MTCLKC Input External clock C
> input
> > pin (MTU2 phase counting mode A phase input) MTCLKD Input External
> > clock D input pin (MTU2 phase counting mode B phase input)
> >
> > For MTU1, it is fixed MTCLKA and MTCLKB.
> > But for MTU2, it can be either 0-{ MTCLKA, MTCLKB} or 1 - { MTCLKC ,
> > MTCLKD} On reset it is set to { MTCLKC , MTCLKD}.
> >
> > Cheers,
> > Biju
> 
> It doesn't need to be named "phase" specifically, but it seems like a
> new sysfs file will be necessary in order to select the proper phase
> counting mode.
> 
> Are these MTCLK signals the quadrature A and B Signals you defined in
> the counter driver?

Yes, that is correct.

Cheers,
Biju

Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[William Breathitt Gray]

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

On Sat, Oct 01, 2022 at 05:12:56PM +0000, Biju Das wrote:
> > > > You can then control the phase selection using a top-level Counter
> > > > device extension (e.g. /sys/bus/counter/devices/counter0/phase)
> > that
> > > > configures whether you're in 16-bit phase or 32-phase counting
> > mode.
> > >
> > > So I need to introduce a new sysfs called phase. Use that one for
> > > Selecting the external clock pin for phase counting mode.
> > > Please correct me if I am wrong??
> > >
> > > Hardware supports 4 pins for phase counting mode,
> > >
> > > MTCLKA Input External clock A input pin (MTU1/MTU2 phase counting
> > mode
> > > A phase input) MTCLKB Input External clock B input pin (MTU1/MTU2
> > > phase counting mode B phase input) MTCLKC Input External clock C
> > input
> > > pin (MTU2 phase counting mode A phase input) MTCLKD Input External
> > > clock D input pin (MTU2 phase counting mode B phase input)
> > >
> > > For MTU1, it is fixed MTCLKA and MTCLKB.
> > > But for MTU2, it can be either 0-{ MTCLKA, MTCLKB} or 1 - { MTCLKC ,
> > > MTCLKD} On reset it is set to { MTCLKC , MTCLKD}.
> > >
> > > Cheers,
> > > Biju
> > 
> > It doesn't need to be named "phase" specifically, but it seems like a
> > new sysfs file will be necessary in order to select the proper phase
> > counting mode.
> > 
> > Are these MTCLK signals the quadrature A and B Signals you defined in
> > the counter driver?
> 
> Yes, that is correct.
> 
> Cheers,
> Biju

You should define a Signal then for each of the four MTCLK inputs.
Create synapse arrays for each Count respectively; e.g. Count 0 will
have Synapses for MTCLKA and MTCLKB, but Count 1 will probably need
Synapses for all four Signals (the action mode for two of them will be
COUNTER_SYNAPSE_ACTION_NONE depending on the configuration set).

What is the configuration when 32-bit phase counting mode is selected?
Does MTCLKA and MTCLKB serve as the counting signals in this case, with
overflows on the MTU1 register incrementing the MTU2 register?

William Breathitt Gray

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

RE: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[Biju Das]

> -----Original Message-----
> From: William Breathitt Gray <william.gray@linaro.org>
> Sent: 01 October 2022 18:44
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: William Breathitt Gray <wbg@kernel.org>; Rob Herring
> <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Philipp Zabel
> <p.zabel@pengutronix.de>; Michael Turquette <mturquette@baylibre.com>;
> Stephen Boyd <sboyd@kernel.org>; Geert Uytterhoeven
> <geert+renesas@glider.be>; Lee Jones <lee@kernel.org>; Uwe Kleine-
> König <u.kleine-koenig@pengutronix.de>; linux-pwm@vger.kernel.org;
> linux-iio@vger.kernel.org; linux-clk@vger.kernel.org;
> devicetree@vger.kernel.org; Chris Paterson
> <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com>;
> Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>;
> linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver
> 
> On Sat, Oct 01, 2022 at 05:12:56PM +0000, Biju Das wrote:
> > > > > You can then control the phase selection using a top-level
> > > > > Counter device extension (e.g.
> > > > > /sys/bus/counter/devices/counter0/phase)
> > > that
> > > > > configures whether you're in 16-bit phase or 32-phase counting
> > > mode.
> > > >
> > > > So I need to introduce a new sysfs called phase. Use that one
> for
> > > > Selecting the external clock pin for phase counting mode.
> > > > Please correct me if I am wrong??
> > > >
> > > > Hardware supports 4 pins for phase counting mode,
> > > >
> > > > MTCLKA Input External clock A input pin (MTU1/MTU2 phase
> counting
> > > mode
> > > > A phase input) MTCLKB Input External clock B input pin
> (MTU1/MTU2
> > > > phase counting mode B phase input) MTCLKC Input External clock C
> > > input
> > > > pin (MTU2 phase counting mode A phase input) MTCLKD Input
> External
> > > > clock D input pin (MTU2 phase counting mode B phase input)
> > > >
> > > > For MTU1, it is fixed MTCLKA and MTCLKB.
> > > > But for MTU2, it can be either 0-{ MTCLKA, MTCLKB} or 1 - {
> MTCLKC
> > > > , MTCLKD} On reset it is set to { MTCLKC , MTCLKD}.
> > > >
> > > > Cheers,
> > > > Biju
> > >
> > > It doesn't need to be named "phase" specifically, but it seems
> like
> > > a new sysfs file will be necessary in order to select the proper
> > > phase counting mode.
> > >
> > > Are these MTCLK signals the quadrature A and B Signals you defined
> > > in the counter driver?
> >
> > Yes, that is correct.
> >
> > Cheers,
> > Biju
> 
> You should define a Signal then for each of the four MTCLK inputs.
> Create synapse arrays for each Count respectively; e.g. Count 0 will
> have Synapses for MTCLKA and MTCLKB, but Count 1 will probably need
> Synapses for all four Signals (the action mode for two of them will be
> COUNTER_SYNAPSE_ACTION_NONE depending on the configuration set).

OK.

> 
> What is the configuration when 32-bit phase counting mode is selected?

LWA Bit (MTU1/MTU2 Combination Longword Access Control) needs to set for 32-bit phase counting mode.

b0 LWA 0 R/W MTU1/MTU2 Combination Longword Access Control
0: 16-bit access is enabled.
1: 32-bit access is enabled.

> Does MTCLKA and MTCLKB serve as the counting signals in this case,

For 16-bit and 32-bit counting signals same. We can set 

1) MTU 1 and MTU2 signals as MTCLKA and MTCLKB

Or 

2) MTU 1 signal as MTCLKA and MTCLKB and MTU2 signals as MTCLKC and MTCLKD


b1 PHCKSEL 1 R/W External Input Phase Clock Select
Selects the external clock pin for phase counting mode.
0: MTCLKA and MTCLKB are selected for the external phase clock.
1: MTCLKC and MTCLKD are selected for the external phase clock

> with overflows on the MTU1 register incrementing the MTU2 register?

No. that won't happen as we need to use different register for Long word access

These are the regiters used
16-bit:- TCNT{MTU1,MTU2}, TGRA{MTU1,MTU2},  and TGRB{MTU1,MTU2},
32-bit:- MTU1.TCNT_1_LW, MTU1.TGRA_1_LW and MTU1.TGRB_1_LW

Counter in MTU1   MTU1.TCNT Word MTU1.TCNT_1_LW Longword
Counter in MTU2   MTU2.TCNT Word

General register A in MTU1 MTU1.TGRA Word MTU1.TGRA_1_LW Longword
General register A in MTU2 MTU2.TGRA Word

General register B in MTU1 MTU1.TGRB Word MTU1.TGRB_1_LW Longword
General register B in MTU2 MTU2.TGRB Word

Cheers,
Biju

Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[William Breathitt Gray]

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

On Sat, Oct 01, 2022 at 06:03:37PM +0000, Biju Das wrote:
> > What is the configuration when 32-bit phase counting mode is selected?
> 
> LWA Bit (MTU1/MTU2 Combination Longword Access Control) needs to set for 32-bit phase counting mode.
> 
> b0 LWA 0 R/W MTU1/MTU2 Combination Longword Access Control
> 0: 16-bit access is enabled.
> 1: 32-bit access is enabled.
> 
> > Does MTCLKA and MTCLKB serve as the counting signals in this case,
> 
> For 16-bit and 32-bit counting signals same. We can set 
> 
> 1) MTU 1 and MTU2 signals as MTCLKA and MTCLKB
> 
> Or 
> 
> 2) MTU 1 signal as MTCLKA and MTCLKB and MTU2 signals as MTCLKC and MTCLKD

I'm having trouble understanding this case. If 32-bit access is enabled
by setting the LWA bit, and the MTU1 signals are configured as MTCLKA
and MTCLKB while at the same time the MTU2 signals are configured as
MTCLKC and MTCLKD, how is the 32-bit count value determined -- wouldn't
MTU1 and MTU2 be counting independently if they each had separate input
clocks fed to them?

William Breathitt Gray

> 
> 
> b1 PHCKSEL 1 R/W External Input Phase Clock Select
> Selects the external clock pin for phase counting mode.
> 0: MTCLKA and MTCLKB are selected for the external phase clock.
> 1: MTCLKC and MTCLKD are selected for the external phase clock
> 
> > with overflows on the MTU1 register incrementing the MTU2 register?
> 
> No. that won't happen as we need to use different register for Long word access
> 
> These are the regiters used
> 16-bit:- TCNT{MTU1,MTU2}, TGRA{MTU1,MTU2},  and TGRB{MTU1,MTU2},
> 32-bit:- MTU1.TCNT_1_LW, MTU1.TGRA_1_LW and MTU1.TGRB_1_LW
> 
> Counter in MTU1   MTU1.TCNT Word MTU1.TCNT_1_LW Longword
> Counter in MTU2   MTU2.TCNT Word
> 
> General register A in MTU1 MTU1.TGRA Word MTU1.TGRA_1_LW Longword
> General register A in MTU2 MTU2.TGRA Word
> 
> General register B in MTU1 MTU1.TGRB Word MTU1.TGRB_1_LW Longword
> General register B in MTU2 MTU2.TGRB Word
> 
> Cheers,
> Biju

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

RE: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[Biju Das]

> Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver
> 
> On Sat, Oct 01, 2022 at 06:03:37PM +0000, Biju Das wrote:
> > > What is the configuration when 32-bit phase counting mode is
> selected?
> >
> > LWA Bit (MTU1/MTU2 Combination Longword Access Control) needs to set
> for 32-bit phase counting mode.
> >
> > b0 LWA 0 R/W MTU1/MTU2 Combination Longword Access Control
> > 0: 16-bit access is enabled.
> > 1: 32-bit access is enabled.
> >
> > > Does MTCLKA and MTCLKB serve as the counting signals in this case,
> >
> > For 16-bit and 32-bit counting signals same. We can set
> >
> > 1) MTU 1 and MTU2 signals as MTCLKA and MTCLKB
> >
> > Or
> >
> > 2) MTU 1 signal as MTCLKA and MTCLKB and MTU2 signals as MTCLKC and
> > MTCLKD
> 
> I'm having trouble understanding this case. If 32-bit access is
> enabled by setting the LWA bit, and the MTU1 signals are configured as
> MTCLKA and MTCLKB while at the same time the MTU2 signals are
> configured as MTCLKC and MTCLKD, how is the 32-bit count value
> determined -- wouldn't
> MTU1 and MTU2 be counting independently if they each had separate
> input clocks fed to them?

It is taken care by the HW. We just configure the register as mentioned below
and hardware provide counter values once feeding the signals to 
either
{MTCLKA and MTCLKB} for both MTU1 and  MTU2 

or 

MTU1{MTCLKA and MTCLKB} and MTU2{MTCLKC and MTCLKD}

The signal feeding is same for 16-bit and 32-bit phase modes.

Note:- I haven't tested 32-bit mode yet. 

Cheers,
Biju

> 
> >
> >
> > b1 PHCKSEL 1 R/W External Input Phase Clock Select Selects the
> > external clock pin for phase counting mode.
> > 0: MTCLKA and MTCLKB are selected for the external phase clock.
> > 1: MTCLKC and MTCLKD are selected for the external phase clock
> >
> > > with overflows on the MTU1 register incrementing the MTU2
> register?
> >
> > No. that won't happen as we need to use different register for Long
> > word access
> >
> > These are the regiters used
> > 16-bit:- TCNT{MTU1,MTU2}, TGRA{MTU1,MTU2},  and TGRB{MTU1,MTU2},
> > 32-bit:- MTU1.TCNT_1_LW, MTU1.TGRA_1_LW and MTU1.TGRB_1_LW
> >
> > Counter in MTU1   MTU1.TCNT Word MTU1.TCNT_1_LW Longword
> > Counter in MTU2   MTU2.TCNT Word
> >
> > General register A in MTU1 MTU1.TGRA Word MTU1.TGRA_1_LW Longword
> > General register A in MTU2 MTU2.TGRA Word
> >
> > General register B in MTU1 MTU1.TGRB Word MTU1.TGRB_1_LW Longword
> > General register B in MTU2 MTU2.TGRB Word
> >
> > Cheers,
> > Biju

Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[William Breathitt Gray]

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

On Sat, Oct 01, 2022 at 06:51:48PM +0000, Biju Das wrote:
> > On Sat, Oct 01, 2022 at 06:03:37PM +0000, Biju Das wrote:
> > > > What is the configuration when 32-bit phase counting mode is
> > selected?
> > >
> > > LWA Bit (MTU1/MTU2 Combination Longword Access Control) needs to set
> > for 32-bit phase counting mode.
> > >
> > > b0 LWA 0 R/W MTU1/MTU2 Combination Longword Access Control
> > > 0: 16-bit access is enabled.
> > > 1: 32-bit access is enabled.
> > >
> > > > Does MTCLKA and MTCLKB serve as the counting signals in this case,
> > >
> > > For 16-bit and 32-bit counting signals same. We can set
> > >
> > > 1) MTU 1 and MTU2 signals as MTCLKA and MTCLKB
> > >
> > > Or
> > >
> > > 2) MTU 1 signal as MTCLKA and MTCLKB and MTU2 signals as MTCLKC and
> > > MTCLKD
> > 
> > I'm having trouble understanding this case. If 32-bit access is
> > enabled by setting the LWA bit, and the MTU1 signals are configured as
> > MTCLKA and MTCLKB while at the same time the MTU2 signals are
> > configured as MTCLKC and MTCLKD, how is the 32-bit count value
> > determined -- wouldn't
> > MTU1 and MTU2 be counting independently if they each had separate
> > input clocks fed to them?
> 
> It is taken care by the HW. We just configure the register as mentioned below
> and hardware provide counter values once feeding the signals to 
> either
> {MTCLKA and MTCLKB} for both MTU1 and  MTU2 
> 
> or 
> 
> MTU1{MTCLKA and MTCLKB} and MTU2{MTCLKC and MTCLKD}
> 
> The signal feeding is same for 16-bit and 32-bit phase modes.
> 
> Note:- I haven't tested 32-bit mode yet. 
> 
> Cheers,
> Biju

I'm not quite grokking it yet, but I'll trust that you're right for now.
I suspect it'll make more sense to me once your next revision is
submitted and I've had time to evaluate the code more closely.

Thanks,

William Breathitt Gray

> 
> > 
> > >
> > >
> > > b1 PHCKSEL 1 R/W External Input Phase Clock Select Selects the
> > > external clock pin for phase counting mode.
> > > 0: MTCLKA and MTCLKB are selected for the external phase clock.
> > > 1: MTCLKC and MTCLKD are selected for the external phase clock
> > >
> > > > with overflows on the MTU1 register incrementing the MTU2
> > register?
> > >
> > > No. that won't happen as we need to use different register for Long
> > > word access
> > >
> > > These are the regiters used
> > > 16-bit:- TCNT{MTU1,MTU2}, TGRA{MTU1,MTU2},  and TGRB{MTU1,MTU2},
> > > 32-bit:- MTU1.TCNT_1_LW, MTU1.TGRA_1_LW and MTU1.TGRB_1_LW
> > >
> > > Counter in MTU1   MTU1.TCNT Word MTU1.TCNT_1_LW Longword
> > > Counter in MTU2   MTU2.TCNT Word
> > >
> > > General register A in MTU1 MTU1.TGRA Word MTU1.TGRA_1_LW Longword
> > > General register A in MTU2 MTU2.TGRA Word
> > >
> > > General register B in MTU1 MTU1.TGRB Word MTU1.TGRB_1_LW Longword
> > > General register B in MTU2 MTU2.TGRB Word
> > >
> > > Cheers,
> > > Biju

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

RE: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver

[Biju Das]

> Subject: Re: [PATCH RFC 0/8] Add RZ/G2L MTU3a MFD and Counter driver
> 
> On Sat, Oct 01, 2022 at 06:51:48PM +0000, Biju Das wrote:
> > > On Sat, Oct 01, 2022 at 06:03:37PM +0000, Biju Das wrote:
> > > > > What is the configuration when 32-bit phase counting mode is
> > > selected?
> > > >
> > > > LWA Bit (MTU1/MTU2 Combination Longword Access Control) needs to
> > > > set
> > > for 32-bit phase counting mode.
> > > >
> > > > b0 LWA 0 R/W MTU1/MTU2 Combination Longword Access Control
> > > > 0: 16-bit access is enabled.
> > > > 1: 32-bit access is enabled.
> > > >
> > > > > Does MTCLKA and MTCLKB serve as the counting signals in this
> > > > > case,
> > > >
> > > > For 16-bit and 32-bit counting signals same. We can set
> > > >
> > > > 1) MTU 1 and MTU2 signals as MTCLKA and MTCLKB
> > > >
> > > > Or
> > > >
> > > > 2) MTU 1 signal as MTCLKA and MTCLKB and MTU2 signals as MTCLKC
> > > > and MTCLKD
> > >
> > > I'm having trouble understanding this case. If 32-bit access is
> > > enabled by setting the LWA bit, and the MTU1 signals are
> configured
> > > as MTCLKA and MTCLKB while at the same time the MTU2 signals are
> > > configured as MTCLKC and MTCLKD, how is the 32-bit count value
> > > determined -- wouldn't
> > > MTU1 and MTU2 be counting independently if they each had separate
> > > input clocks fed to them?
> >
> > It is taken care by the HW. We just configure the register as
> > mentioned below and hardware provide counter values once feeding the
> > signals to either {MTCLKA and MTCLKB} for both MTU1 and  MTU2
> >
> > or
> >
> > MTU1{MTCLKA and MTCLKB} and MTU2{MTCLKC and MTCLKD}
> >
> > The signal feeding is same for 16-bit and 32-bit phase modes.
> >
> > Note:- I haven't tested 32-bit mode yet.
> >
> > Cheers,
> > Biju
> 
> I'm not quite grokking it yet, but I'll trust that you're right for
> now.
> I suspect it'll make more sense to me once your next revision is
> submitted and I've had time to evaluate the code more closely.

OK, softwarewise we don't need to anything for 16-bit and 32-bit 
as HW provide separate registers for accessing 32-bit counter values
eventhoug internally, it is formed by combining 2 16-bit counters on
MTU1 and MTU2.

OK, we will have more idea when I submit patches for 32-bit mode.

Cheers,
Biju


> 
> Thanks,
> 
> William Breathitt Gray
> 
> >
> > >
> > > >
> > > >
> > > > b1 PHCKSEL 1 R/W External Input Phase Clock Select Selects the
> > > > external clock pin for phase counting mode.
> > > > 0: MTCLKA and MTCLKB are selected for the external phase clock.
> > > > 1: MTCLKC and MTCLKD are selected for the external phase clock
> > > >
> > > > > with overflows on the MTU1 register incrementing the MTU2
> > > register?
> > > >
> > > > No. that won't happen as we need to use different register for
> > > > Long word access
> > > >
> > > > These are the regiters used
> > > > 16-bit:- TCNT{MTU1,MTU2}, TGRA{MTU1,MTU2},  and TGRB{MTU1,MTU2},
> > > > 32-bit:- MTU1.TCNT_1_LW, MTU1.TGRA_1_LW and MTU1.TGRB_1_LW
> > > >
> > > > Counter in MTU1   MTU1.TCNT Word MTU1.TCNT_1_LW Longword
> > > > Counter in MTU2   MTU2.TCNT Word
> > > >
> > > > General register A in MTU1 MTU1.TGRA Word MTU1.TGRA_1_LW
> Longword
> > > > General register A in MTU2 MTU2.TGRA Word
> > > >
> > > > General register B in MTU1 MTU1.TGRB Word MTU1.TGRB_1_LW
> Longword
> > > > General register B in MTU2 MTU2.TGRB Word
> > > >
> > > > Cheers,
> > > > Biju

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

[Biju Das]

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