On Sat, Oct 08, 2022 at 09:01:21AM +0000, Biju Das wrote:
> Hi William Breathitt Gray,
>
> Thanks for the feedback.
Hello Biju,
I see that you have already released a v4, so some of my comments may no
longer apply, but I want to respond here to continue our discussions;
I'll reiterate any relevant suggestions when I review v4 in the coming
days.
By the way, if you agree with a review comment there is no need to reply
with "OK"; just delete the parts you agree with from your response and
I'll know those are okay. Doing this will reduce the amount of text we
have to scroll through and thus allow us to focus on just the questions
we have remaining. ;-)
> > > +/**
> > > + * struct rz_mtu3_cnt - MTU3 counter private data
> > > + *
> > > + * @clk: MTU3 module clock
> > > + * @lock: Lock to prevent concurrent access for ceiling and count
> > > + * @rz_mtu3_channel: HW channels for the counters */ struct
> > > +rz_mtu3_cnt {
> > > + struct clk *clk;
> > > + struct mutex lock;
> > > + struct rz_mtu3_channel *ch[RZ_MTU3_MAX_HW_CNTR_CHANNELS];
> >
> > Does this need to be a pointer to an array of struct rz_mtu3_channel?
>
> Yes, HW has MTU{0..8} channels and MTU{1,2} supports counters
> At probe time this array is filled with *ch[0]= MTU1 and *ch[1]= MTU2
In the rz_mtu3_cnt_probe() function I see the rz_mtu3_cnt.ch elements
manually set to the address of each rz_mtu3.channels element:
for (i = 0; i < RZ_MTU3_MAX_HW_CNTR_CHANNELS; i++) {
priv->ch[i] = &ddata->channels[RZ_MTU1 + i];
priv->ch[i]->dev = dev;
...
The rz_mut3.channels member is a contiguous array of struct
rz_mtu3_channel. If you change the rz_mtu3_channel to a pointer to
struct rz_mtu3_channel, you can set it to the RZ_MTU1 offset address
outside of the for loop and thus avoid the double dereference because
these address are contiguous:
priv->ch = &ddata->channels[RZ_MTU1];
for (i = 0; i < RZ_MTU3_MAX_HW_CNTR_CHANNELS; i++) {
priv->ch[i].dev = dev;
...
> > > + mutex_lock(&priv->lock);
> > > + if (ceiling == 0) {
> > > + rz_mtu3_8bit_ch_write(priv->ch[id], RZ_MTU3_TCR,
> > > + RZ_MTU3_TCR_CCLR_NONE);
> >
> > Looks like something different is done when ceiling is set to 0. Would
> > you explain what's happening in this case and why it's different that
> > then else case below; in other words, what's the difference between
> > RZ_MTU3_TCR_CCLR_NONE and RZ_MTU3_TCR_CCLR_TGRA?
>
> RZ_MTU3_TCR_CCLR_TGRA --> for triggering counter count using Z-Phase signal.
> RZ_MTU3_TCR_CCLR_NONE --> No clearing.
Does the Z-Phase signal trigger a reset of the counter count back to the
ceiling value? Does the count loop back to 0 when it passes the ceiling
value, or does it remain at the ceiling until the direction changes?
By "no clearing" do you mean that the ceiling is disabled in this case
and the Counter count increases without limit?
In the Counter subsystem, the "ceiling" Count extension puts an upper
limit on the Count value. This means that setting "ceiling" to 0 would
put the upper limit at 0, effectively restricting the Count value to 0
until the value of "ceiling" is raised.
If the device is unable to support a ceiling value of 0, you should
return -ERANGE rather than disable the ceiling.
> > > +static void rz_mtu3_16bit_cnt_setting(struct counter_device
> > *counter,
> > > +int id) {
> > > + struct rz_mtu3_cnt *const priv = counter_priv(counter);
> > > +
> > > + priv->ch[id]->function = RZ_MTU3_16BIT_PHASE_COUNTING;
> >
> > If 16-bit phase counting is selected for one 16-bit counter, does the
> > other 16-bit counter need to be configured as well?
>
> Not required I guess, as it is run time decision.
>
> After this, if user tries to enable 16-bit on other channel,
> we will configure that channel. otherwise, we will return error,
> if user tries to enable 32-bit channel.
>
> Are you ok with this?
Because the phase mode affects how the device interprets multiple
channels rather than a specific one, maybe it's better to save this
state as an enum rz_mtu3_function member of struct rz_mtu3_cnt. Or if
this is affecting the entire device, move it to your struct rz_mut3 and
share a pointer to that for your Counter and PWM drivers.
It makes me wonder if the rz_mut3_cnt structure is necessary for this
Counter driver at all when you could pass a pointer your existing
rz_mut3 structure instead in order to access the channels.
> > > + int ret = 0;
> > > +
> > > + if (enable) {
> > > + pm_runtime_get_sync(ch->dev);
> > > + ret = rz_mtu3_initialize_counter(counter, count->id);
> >
> > Are you using the Count's "enable" extension to switch between 16-bit
> > and 32-bit phase modes?
>
> No. But will use that for switching on the next version.
Sorry, I wasn't clear with my question. Please do not implement the
"enable" Count extensions as a way to toggle between the 16-bit and
32-bit phase modes. The purpose of "enable" is to provide a pause/resume
mechanism for a Count: the existing count value should be preserved when
a Count is disabled, and should continue where it left off when the
Count is enabled.
To support the phase mode selection, implement a Counter device
extension for that specific purpose. You can use DEFINE_COUNTER_ENUM()
and COUNTER_COMP_DEVICE_ENUM() to create a device extension that will
allow users to toggle between "16-bit" and "32-bit" phase modes. If you
need help with these macros, just let me know.
> > > + .name = "Channel3 Count(32-bit)",
> >
> > We probably don't need the "(32-bit)" in the name when it's obvious
> > already from the channel id and ceiling value.
>
> OK will remove it.
> >
> > I wonder how this counter is described in the RZ/G2L user
> > documentation; is it named "Channel 3" or "Channel 1 and 2"?
>
> It is mentioned as MTU1 and MTU2 channels.
>
> These channels can be used for phase counting and PWM operations.
We should avoid calling it "Channel 3" in that case to avoid confusion.
Perhaps "Channel 1 and 2 (combined) Count" would be better; that's just
something I came up with off the top of my head, but if you can think of
a better way to phrase it then please do.
William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature
