RE: [PATCH v4 3/4] mfd: Add RZ/G2L MTU3 counter driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi William Breathitt Gray,

> Subject: Re: [PATCH v4 3/4] mfd: Add RZ/G2L MTU3 counter driver
> 
> 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.
OK.

> 
> > 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?

I will take out "of.h", as there is no compatible.

> 
> > +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::

OK.

> 
> 
>     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?

Yes, we need to expose this to sysfs. Is it same as phase mode?
Can you please provide an example, if possible how to handle this?

Cheers,
Biju





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux