On Mon, Oct 10, 2022 at 03:52:21PM +0100, Biju Das wrote:
> Add RZ/G2L MTU3 counter driver. This IP supports the following
> phase counting modes on MTU1 and MTU2 channels
>
> 1) 16-bit phase counting modes on MTU1 and MTU2 channels.
> 2) 32-bit phase counting mode by cascading MTU1 and MTU2.
>
> This patch adds 3 counters by creating 3 logical channels
> counter0: 16-bit phase counter on MTU1 channel
> counter1: 16-bit phase counter on MTU2 channel
> counter2: 32-bit phase counter by cascading MTU1 and MTU2
> channels.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
Hello Biju,
We discussed some changes already for v5, but I have some additional
comments and questions below.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 7329971a3bdf..fa88056224c9 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1986,6 +1986,14 @@ config MFD_RZ_MTU3
> To compile this driver as a module, choose M here: the
> module will be called rz-mtu3.
>
> +config MFD_RZ_MTU3_CNT
> + tristate "RZ/G2L MTU3 counter driver"
This is a nitpick, but include the manufacturer name in the tristate
string for the sake of clarity: "Renesas RZ/G2L MTU3 counter driver".
> + depends on MFD_RZ_MTU3 || COMPILE_TEST
I noticed you include <linux/of.h> in the rz-mtu3-cnt.c file; do you
need to depend on OF here in the Kconfig as well?
> +static int rz_mtu3_count_read(struct counter_device *counter,
> + struct counter_count *count, u64 *val)
> +{
> + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> +
> + if (count->id == RZ_MTU3_32_BIT_CH)
> + *val = rz_mtu3_32bit_ch_read(priv->ch[0], RZ_MTU3_TCNTLW);
> + else
> + *val = rz_mtu3_16bit_ch_read(priv->ch[count->id], RZ_MTU3_TCNT);
After considering this again, I think it'll be better to match the
structure of the rest of the functions in this driver for consistency.
Rather than hardcoding priv->ch[0], determine the ch_id first and pass
that instead::
const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
if (count->id == RZ_MTU3_32_BIT_CH)
*val = rz_mtu3_32bit_ch_read(priv->ch[ch_id], RZ_MTU3_TCNTLW);
else
*val = rz_mtu3_16bit_ch_read(priv->ch[ch_id], RZ_MTU3_TCNT);
> +static int rz_mtu3_count_write(struct counter_device *counter,
> + struct counter_count *count, const u64 val)
> +{
> + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> + const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> + u32 ceiling;
> +
> + mutex_lock(&priv->lock);
> + if (count->id == RZ_MTU3_32_BIT_CH)
> + ceiling = priv->mtu_32bit_max;
> + else
> + ceiling = priv->mtu_16bit_max[ch_id];
> +
> + if (val > ceiling) {
> + mutex_unlock(&priv->lock);
> + return -ERANGE;
> + }
> +
> + if (count->id == RZ_MTU3_32_BIT_CH)
> + rz_mtu3_32bit_ch_write(priv->ch[0], RZ_MTU3_TCNTLW, val);
Like in count_read(), use ch_id here instead of 0 for the sake of
consistency.
> +static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
> + struct counter_count *count,
> + u64 ceiling)
> +{
> + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> + const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> +
> + switch (count->id) {
> + case RZ_MTU3_16_BIT_MTU1_CH:
> + case RZ_MTU3_16_BIT_MTU2_CH:
> + if (ceiling > U16_MAX)
> + return -ERANGE;
> + priv->mtu_16bit_max[ch_id] = ceiling;
> + break;
> + case RZ_MTU3_32_BIT_CH:
> + if (ceiling > U32_MAX)
> + return -ERANGE;
> + priv->mtu_32bit_max = ceiling;
> + break;
> + }
> +
> + mutex_lock(&priv->lock);
> + if (ceiling == 0) {
> + /* Disable counter clear source */
> + rz_mtu3_8bit_ch_write(priv->ch[ch_id], RZ_MTU3_TCR,
> + RZ_MTU3_TCR_CCLR_NONE);
Refer to our discussions in the v3 review thread regarding ceiling set
to 0; in particular, don't disable the count value ceiling but rather
limit it to a maximum value of 0.
> +static int rz_mtu3_count_enable_write(struct counter_device *counter,
> + struct counter_count *count, u8 enable)
> +{
> + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> + const size_t ch_id = RZ_MTU3_GET_HW_CH(count->id);
> + struct rz_mtu3_channel *ch = priv->ch[ch_id];
> + int ret = 0;
> +
> + if (enable) {
> + pm_runtime_get_sync(ch->dev);
> + ret = rz_mtu3_initialize_counter(counter, count->id);
> + } else {
> + rz_mtu3_terminate_counter(counter, count->id);
> + pm_runtime_put(ch->dev);
> + }
Refer to our discussions in the v3 review thread regarding the "enable"
Count extension; in particular, "enable" pauses/unpauses counting.
> +static int rz_mtu3_action_read(struct counter_device *counter,
> + struct counter_count *count,
> + struct counter_synapse *synapse,
> + enum counter_synapse_action *action)
> +{
> + const size_t signal_a_id = count->synapses[0].signal->id;
> + const size_t signal_b_id = count->synapses[1].signal->id;
> + size_t signal_c_id;
> + size_t signal_d_id;
> + enum counter_function function;
> + int err;
> +
> + if (count->id != RZ_MTU3_16_BIT_MTU1_CH) {
> + signal_c_id = count->synapses[2].signal->id;
> + signal_d_id = count->synapses[3].signal->id;
> + }
The Signal ids are constants so you remove them from this function and
use preprocessor defines instead to represent SIGNAL_A_ID, SIGNAL_B_ID,
SIGNAL_C_ID, and SIGNAL_D_ID. Remember to set the Signal ids in the
rz_mtu3_signals[] array accordingly.
> +static struct counter_signal rz_mtu3_signals[] = {
> + RZ_MTU3_PHASE_SIGNAL(0, "MTU1 MTCLKA"),
> + RZ_MTU3_PHASE_SIGNAL(1, "MTU1 MTCLKB"),
> + RZ_MTU3_PHASE_SIGNAL(2, "MTU2 MTCLKC"),
> + RZ_MTU3_PHASE_SIGNAL(3, "MTU2 MTCLKD"),
> +};
The relationship of these Signals still has me somewhat confused so I'm
hoping you can help me properly ironed out my understanding. This is how
I currently understand it, so please point out any mistakes I have:
MTU1 (Channel 1):
* Pulse-Direction mode:
- MTCLKA updates count
- MTCLKB determines direction
* Quadrature x2 B:
- MTCLKA is quadrature phase A
- MTCLKB is quadrature phase B
- Any state transition on MTCLKB updates count
* Quadrature x4:
- MTCLKA is quadrature phase A
- MTCLKB is quadrature phase B
- Any state transition on either MTCLKA or MTCLKB updates count
MTU2 (Channel 2):
- Same as MTU1, but optionally can select MTCLKC and MTCLKD instead of
MTCLKA and MTCLKB respectively
* Pulse-Direction mode:
- MTCLKA(C) updates count
- MTCLKB(D) determines direction
* Quadrature x2 B:
- MTCLKA(C) is quadrature phase A
- MTCLKB(D) is quadrature phase B
- Any state transition on MTCLKB updates count
* Quadrature x4:
- MTCLKA(C) is quadrature phase A
- MTCLKB(D) is quadrature phase B
- Any state transition on either MTCLKA(C) or MTCLKB(D) updates count
MTU3 (Channel 1 and 2 cascading):
- Same as MTU2 (but count is now 32-bit)
* Pulse-Direction mode:
- MTCLKA(C) updates count
- MTCLKB(D) determines direction
* Quadrature x2 B:
- MTCLKA(C) is quadrature phase A
- MTCLKB(D) is quadrature phase B
- Any state transition on MTCLKB updates count
* Quadrature x4:
- MTCLKA(C) is quadrature phase A
- MTCLKB(D) is quadrature phase B
- Any state transition on either MTCLKA(C) or MTCLKB(D) updates count
Is my understanding correct here? Is the selection between MTCLKA/MTCLKB
and MTCLKC/MTCLKD done in software, and should we expose it in sysfs?
William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature
