Hi William Breathitt Gray,
Thanks for the feedback.
> -----Original Message-----
> From: William Breathitt Gray <william.gray@xxxxxxxxxx>
> Sent: 14 November 2022 03:47
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>;
> Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v6 4/5] counter: Add Renesas RZ/G2L MTU3a counter driver
>
> On Sun, Nov 13, 2022 at 05:15:44PM +0000, Biju Das wrote:
>
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > +#define RZ_MTU3_GET_HW_CH(id) \
> > +({ \
> > + size_t _id = (id); _id = (_id == RZ_MTU3_32_BIT_CH) ? 0 : _id; \
> > +})
>
> I probably missed a discussion about this change in a previous thread; what
> is the purpose of using a local size_t variable here? Is this due to the
> "possible side-effects" mentioned in the patch changes note?
Check patch is complaining
"CHECK: Macro argument reuse 'id' - possible side-effects?"
By using local size_t variable, it fixed the check patch warning.
>
> > +
> > +#define SIGNAL_A_ID (0)
> > +#define SIGNAL_B_ID (1)
> > +#define SIGNAL_C_ID (2)
> > +#define SIGNAL_D_ID (3)
> > +
> > +/**
> > + * struct rz_mtu3_cnt - MTU3 counter private data
> > + *
> > + * @clk: MTU3 module clock
> > + * @lock: Lock to prevent concurrent access for ceiling and count
> > + * @ch: HW channels for the counters
> > + * @mtu_16bit_max: Cache for 16-bit counters
> > + * @mtu_32bit_max: Cache for 32-bit counters */ struct rz_mtu3_cnt {
> > + struct clk *clk;
> > + struct mutex lock;
> > + struct rz_mtu3_channel *ch;
> > + u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> > + u32 mtu_32bit_max;
>
> Does the ceiling set on the device get clobbered when you change between 16-
> bit and 32-bit phase modes (i.e. writing to TGRALW vs TGRA)? You have a
> separate cache for the 32-bit ceiling value here, but if it is getting
> clobbered then as a small optimization you may reimplement this cache as a
> union such as:
>
> union {
> u16 mtu_16bit_max[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> u32 mtu_32bit_max;
> }
Yes, it gets clobbered when we change between 16-bit and 32-bit mode.
For eg: 0xbe1352 value
Split up into mtu1.TGRA=0xbe and mtu2.TGRA=0x1352.
OK will use the union.
>
> > +};
> > +
>
> > +
> > + switch (val & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
> > + case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
> > + *function = COUNTER_FUNCTION_QUADRATURE_X4;
> > + break;
> > + case RZ_MTU3_TMDR1_PH_CNT_MODE_2:
> > + *function = COUNTER_FUNCTION_PULSE_DIRECTION;
> > + break;
> > + case RZ_MTU3_TMDR1_PH_CNT_MODE_4:
> > + *function = COUNTER_FUNCTION_QUADRATURE_X2_B;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
>
> Sorry if I asked this before: what are counting modes 3 and 5, and are they
> not supported by this device? If they are not supported, please include a
> comment stating so in the default case block so that it is clear for future
> reviewers as well.
Our hardware supports 5 phase counting modes. From that list, I match up some of the functions
supported by the counter driver.
counting modes 3 and 5 are supported by the Devices, but currently counter driver is not supported this.
Please see the attached counting modes 3 and 5.
https://ibb.co/3YJByG1
OK, I will add a comment for the details for modes not supported by the current driver in the default block.
> > +
> > +static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter,
> > +int id) {
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > + struct rz_mtu3_channel *ch1 = priv->ch;
> > + struct rz_mtu3_channel *ch2 = ch1 + 1;
> > +
> > + /*
> > + * 32-bit phase counting need MTU1 and MTU2 to create 32-bit cascade
> > + * counter.
> > + */
> > + ch1->function = RZ_MTU3_32BIT_PHASE_COUNTING;
> > + ch2->function = RZ_MTU3_32BIT_PHASE_COUNTING;
>
> Can these "function" members be modified from outside this driver? If so, you
> could have a race condition here.
OK will add channel specific locks to avoid the races.
Do you prefer mutex or spin lock here? As channel selection is based on runtime decision
For both PWM and counter??
>
> > +
> > + /* Phase counting mode 1 is used as default in initialization. */
> > + rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TMDR1,
> > +RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > +
> > + rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> > + rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TIOR, RZ_MTU3_TIOR_IC_BOTH);
> > +
> > + rz_mtu3_enable(ch1);
> > + rz_mtu3_enable(ch2);
> > +}
> > +
> > +static void rz_mtu3_16bit_cnt_setting(struct counter_device *counter,
> > +int id) {
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > + struct rz_mtu3_channel *ch = priv->ch + id;
> > +
> > + ch->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> > +
> > + /* Phase counting mode 1 is used as default in initialization. */
> > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1,
> > +RZ_MTU3_TMDR1_PH_CNT_MODE_1);
> > +
> > + rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
> > + rz_mtu3_enable(ch);
> > +}
> > +
> > +static int rz_mtu3_initialize_counter(struct counter_device *counter,
> > +int id) {
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > + struct rz_mtu3_channel *ch1 = priv->ch;
> > + struct rz_mtu3_channel *ch2 = ch1 + 1;
>
> No need to complicate this, just use priv->ch[0], priv->ch[1], and
> priv->ch[id]. Same advice applies to the other functions as well.
I get below error when I use array susbscripts. "*ch1 = priv->ch[0];"
drivers/counter/rz-mtu3-cnt.c:291:32: error: incompatible types when initialising type 'struct rz_mtu3_channel *' using type 'struct rz_mtu3_channel'
291 | struct rz_mtu3_channel *ch1 = priv->ch[0];
>
> > +
> > + mutex_lock(&priv->lock);
> > + rz_mtu3_8bit_ch_write(ch1, RZ_MTU3_TIOR, RZ_MTU3_TIOR_NO_OUTPUT);
> > + switch (id) {
> > + case RZ_MTU3_16_BIT_MTU1_CH:
> > + case RZ_MTU3_16_BIT_MTU2_CH:
> > + if ((priv->ch + id)->function != RZ_MTU3_NORMAL) {
> > + mutex_unlock(&priv->lock);
> > + return -EBUSY;
> > + }
> > +
> > + rz_mtu3_16bit_cnt_setting(counter, id);
> > + break;
> > + case RZ_MTU3_32_BIT_CH:
> > + if (ch1->function != RZ_MTU3_NORMAL ||
> > + ch2->function != RZ_MTU3_NORMAL) {
> > + mutex_unlock(&priv->lock);
> > + return -EBUSY;
> > + }
> > + rz_mtu3_32bit_cnt_setting(counter, id);
> > + break;
> > + }
> > + mutex_unlock(&priv->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static void rz_mtu3_terminate_counter(struct counter_device *counter,
> > +int id) {
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > + struct rz_mtu3_channel *ch1 = priv->ch;
> > + struct rz_mtu3_channel *ch2 = ch1 + 1;
> > +
> > + mutex_lock(&priv->lock);
> > + if (id == RZ_MTU3_32_BIT_CH) {
> > + ch1->function = RZ_MTU3_NORMAL;
> > + ch2->function = RZ_MTU3_NORMAL;
> > + rz_mtu3_disable(ch2);
> > + rz_mtu3_disable(ch1);
> > + } else {
> > + (priv->ch + id)->function = RZ_MTU3_NORMAL;
> > + rz_mtu3_disable(priv->ch + id);
> > + }
> > + mutex_unlock(&priv->lock);
> > +}
> > +
> > +static int rz_mtu3_count_enable_read(struct counter_device *counter,
> > + struct counter_count *count, u8 *enable) {
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > + struct rz_mtu3_channel *ch1 = priv->ch;
> > + struct rz_mtu3_channel *ch2 = ch1 + 1;
> > +
> > + if (count->id == RZ_MTU3_32_BIT_CH) {
> > + mutex_lock(&priv->lock);
> > + *enable = rz_mtu3_is_enabled(ch1) &&
> > + rz_mtu3_is_enabled(ch2);
> > + mutex_unlock(&priv->lock);
> > + } else {
> > + *enable = rz_mtu3_is_enabled(priv->ch + count->id);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rz_mtu3_count_enable_write(struct counter_device *counter,
> > + struct counter_count *count, u8 enable) {
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > + 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);
>
> The "enable" Count component serves to pause/resume counting operation; that
> means the existing count should not be lost when a Count is disabled. The
> rz_mtu3_initialize_counter() function will clear the current Count, so you'll
> need to restore it before returning.
Yes, it is doing pause/resume operation only. It is using clock gating and PM operations.
During enable, Channel is enabled, clk is on.
During disable, Channel is disabled, clk is off.
Here we are not losing the count when it is disabled and then enable particular count.
But we will loss the count, after disable, if it is used by other devices such as PWM
Or we are switching to 16-bit and 32-bit and vice versa.
Maybe Will rename it to "rz_mtu3_{resume,pause}_counter" to make it clear.
Compared to PWM framework we are missing export/unexport calls here in counter subsystem.
For PWM, we have an export/unexport calls for creating runtime pwm devices such as pwm0, pwm1 for pwmdevice.
Here, count0, count1 and count2 are created during probe.
My current test sequence is,
1) Set phase clk
2) Set cascade_enable
3) Set enable(Since we don't have export/unexport, I am using disable calls for freeing Channels for other subsystem)
4) Set count
5) Set ceiling
>
> Alternatively, the "enable" Count component is optional so you can remove it
> if you don't want to implement it; just initialize the counter at probe time
> instead.
Let me know your opinion based on the above?
>
> > + } else {
> > + rz_mtu3_terminate_counter(counter, count->id);
> > + pm_runtime_put(ch->dev);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int rz_mtu3_long_word_access_ctrl_mode_get(struct counter_device
> *counter,
> > + u32 *lwa_ctrl_mode)
> > +{
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > + u16 val;
> > +
> > + pm_runtime_get_sync(priv->ch->dev);
> > + val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> > + *lwa_ctrl_mode = val & RZ_MTU3_TMDR3_LWA;
> > + pm_runtime_put(priv->ch->dev);
> > +
> > + return 0;
> > +}
> > +
> > +static int rz_mtu3_long_word_access_ctrl_mode_set(struct counter_device
> *counter,
> > + u32 lwa_ctrl_mode)
> > +{
> > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > + u16 val;
> > +
> > + pm_runtime_get_sync(priv->ch->dev);
> > + val = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
> > + if (lwa_ctrl_mode)
> > + val |= RZ_MTU3_TMDR3_LWA;
> > + else
> > + val &= ~RZ_MTU3_TMDR3_LWA;
> > +
> > + rz_mtu3_shared_reg_write(priv->ch, RZ_MTU3_TMDR3, val);
> > + pm_runtime_put(priv->ch->dev);
>
> When you want to assign a bit to a buffer, you can use __assign_bit() to
> simplify your code:
OK will use and remove BIT macro for both RZ_MTU3_TMDR3_LWA & RZ_MTU3_TMDR3_PHCKSEL
And use bit apis.
> > +static struct counter_count rz_mtu3_counts[] = {
> > + {
> > + .id = RZ_MTU3_16_BIT_MTU1_CH,
> > + .name = "Channel 1 Count",
> > + .functions_list = rz_mtu3_count_functions,
> > + .num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > + .synapses = rz_mtu3_mtu1_count_synapses,
> > + .num_synapses = ARRAY_SIZE(rz_mtu3_mtu1_count_synapses),
> > + .ext = rz_mtu3_count_ext,
> > + .num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > + },
> > + {
> > + .id = RZ_MTU3_16_BIT_MTU2_CH,
> > + .name = "Channel 2 Count",
> > + .functions_list = rz_mtu3_count_functions,
> > + .num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > + .synapses = rz_mtu3_mtu2_count_synapses,
> > + .num_synapses = ARRAY_SIZE(rz_mtu3_mtu2_count_synapses),
> > + .ext = rz_mtu3_count_ext,
> > + .num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > + },
> > + {
> > + .id = RZ_MTU3_32_BIT_CH,
> > + .name = "Channel 1 and 2 (combined) Count",
> > + .functions_list = rz_mtu3_count_functions,
> > + .num_functions = ARRAY_SIZE(rz_mtu3_count_functions),
> > + .synapses = rz_mtu3_mtu2_count_synapses,
>
> I'm just checking again for my benefit of understanding: in 32-bit phase
> counting mode, if ext_input_phase_clock_select is "MTCLKC-MTCLKD", is signal
> C and D used instead of signal A and B? In other words, is the
> ext_input_phase_clock_select only valid for 16-bit phase counting mode, or
> does it also apply for 32-bit phase counting mode?
MTU1(16-bit mode) :- "MTCLKA-MTCLKB",
MTU2(16-bit mode) :- "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
32-bit mode :- "MTCLKA-MTCLKB" or "MTCLKC-MTCLKD"
>
> > + .num_synapses = ARRAY_SIZE(rz_mtu3_mtu2_count_synapses),
> > + .ext = rz_mtu3_count_ext,
> > + .num_ext = ARRAY_SIZE(rz_mtu3_count_ext),
> > + }
> > +};
> > +
> > +static const char *const rz_mtu3_long_word_access_ctrl_modes[] = {
> > + "16-bit",
> > + "32-bit",
> > +};
> > +
> > +static DEFINE_COUNTER_ENUM(rz_mtu3_long_word_access_ctrl_mode_enum,
> > + rz_mtu3_long_word_access_ctrl_modes);
> > +
> > +static const char *const rz_mtu3_ext_input_phase_clock_select[] = {
> > + "MTCLKA-MTCLKB",
> > + "MTCLKC-MTCLKD",
> > +};
> > +
> > +static DEFINE_COUNTER_ENUM(rz_mtu3_ext_input_phase_clock_select_enum,
> > + rz_mtu3_ext_input_phase_clock_select);
> > +
> > +static struct counter_comp rz_mtu3_device_ext[] = {
> > + COUNTER_COMP_DEVICE_ENUM("long_word_access_ctrl_mode",
>
> Cascading Counts seems like a feature that other counter devices may also
> want to expose so it'll be prudent to rename this to something more general
> that other drivers can expose. Perhaps reimplement this as a
> COUNTER_COMP_DEVICE_BOOL and call it "cascade_enable", or something similar.
OK.Will do.
>
> > + rz_mtu3_long_word_access_ctrl_mode_get,
> > + rz_mtu3_long_word_access_ctrl_mode_set,
> > + rz_mtu3_long_word_access_ctrl_mode_enum),
> > + COUNTER_COMP_DEVICE_ENUM("external_input_phase_clock_select",
> > + rz_mtu3_ext_input_phase_clock_select_get,
> > + rz_mtu3_ext_input_phase_clock_select_set,
> > + rz_mtu3_ext_input_phase_clock_select_enum),
> > +};
> > +
> > +static int rz_mtu3_cnt_pm_runtime_suspend(struct device *dev) {
> > + struct clk *const clk = dev_get_drvdata(dev);
> > +
> > + clk_disable_unprepare(clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int rz_mtu3_cnt_pm_runtime_resume(struct device *dev) {
> > + struct clk *const clk = dev_get_drvdata(dev);
> > +
> > + clk_prepare_enable(clk);
> > +
> > + return 0;
> > +}
>
> Do the current counts need to be saved and restored when the pm
> suspend/resume ops occur?
For runtime PM it is not required. But it is required for PM Sleep.
Currently we don't have any PM sleep functionality.
Cheers,
Biju
