On Mon, Nov 14, 2022 at 03:24:26PM +0000, Biju Das wrote:
> > > +#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.
Ah, I see what you mean: 'id' could be an expression (e.g. x++) which
would be evaluated twice if passed as the macro argument. I think
there's a compiler hint that will supress this warning, but I can't
quite remember it right now.
For now, I'd prefer this to be implemented as an inline function so that
the purpose of the code remains clear:
static inline size_t rz_mtu3_get_hw_ch(const size_t id)
{
return (id == RZ_MTU3_32_BIT_CH) ? 0 : id;
}
> > > +/**
> > > + * 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.
Be sure also to check rz_mtu3_is_counter_invalid() in
rz_mtu3_count_ceiling_read() to make sure the proper ceiling is only
returned when its valid.
> > > + 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.
Those are interesting counting modes; it looks like counting mode 5 is
pulse-direction with an inverted direction, but I'm not sure what to
call counting mode 3. I haven't come across these counting modes before
so I wonder what sort of applications they're typically used for.
Typically we would add a new COUNTER_FUNCTION_* to represent each new
counting mode. However, for the sake of keeping this patch series simple
it will be okay to leave a comment in the default block for now
describing the missing counting modes and indicating that support for
them are TODO items for the future.
> > > +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??
Hmm, I'm not sure yet how best to handle this because if the PWM driver
changes the function in the middle of the Counter driver's operation
(or vice versa) we'll obviously have problems.
Thierry, do you have any ideas here for how we can gracefully handle
transfer of control between the PWM and Counter driver. I think a simple
mutex in struct rz_mtu3 should be fine: one driver locks it and the
other driver can return -EBUSY until its unlocked.
> > > +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?
Okay, I think I understand, the count is only lost if another driver
takes control of that channel. In that case, you can leave the code as
it is; there is no need to rename the functions. Once another driver
such as PWM has control over the device channel, it's unreasonable to
expect the Counter driver to maintain the previous device state, so
don't worry about trying to resolve that.
William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature
