[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <l5xvdndysdvtil472it6ylthcfam5jp7lh3son45mezq7dh2yk@3yj557k2o5k5>
Date: Thu, 7 Nov 2024 09:41:34 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: George Stark <gnstark@...utedevices.com>
Cc: neil.armstrong@...aro.org, khilman@...libre.com, jbrunet@...libre.com,
martin.blumenstingl@...glemail.com, linux-pwm@...r.kernel.org, linux-amlogic@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, kernel@...utedevices.com
Subject: Re: [PATCH v2 2/4] pwm: meson: Support constant and polarity bits
On Wed, Nov 06, 2024 at 04:54:41PM +0300, George Stark wrote:
> On 11/4/24 12:32, Uwe Kleine-König wrote:
> > > @@ -68,6 +72,8 @@ static struct meson_pwm_channel_data {
> > > u8 clk_div_shift;
> > > u8 clk_en_shift;
> > > u32 pwm_en_mask;
> > > + u32 const_en_mask;
> > > + u32 inv_en_mask;
> > > } meson_pwm_per_channel_data[MESON_NUM_PWMS] = {
> > > {
> > > .reg_offset = REG_PWM_A,
> > > @@ -75,6 +81,8 @@ static struct meson_pwm_channel_data {
> > > .clk_div_shift = MISC_A_CLK_DIV_SHIFT,
> > > .clk_en_shift = MISC_A_CLK_EN_SHIFT,
> > > .pwm_en_mask = MISC_A_EN,
> > > + .const_en_mask = MISC_A_CONSTANT_EN,
> > > + .inv_en_mask = MISC_A_INVERT_EN,
> > > },
> > > {
> > > .reg_offset = REG_PWM_B,
> > > @@ -82,6 +90,8 @@ static struct meson_pwm_channel_data {
> > > .clk_div_shift = MISC_B_CLK_DIV_SHIFT,
> > > .clk_en_shift = MISC_B_CLK_EN_SHIFT,
> > > .pwm_en_mask = MISC_B_EN,
> > > + .const_en_mask = MISC_B_CONSTANT_EN,
> > > + .inv_en_mask = MISC_B_INVERT_EN,
> > > }
> > > };
> >
> > So the generic register description describes the const and invert bits,
> > but it doesn't apply to all IPs. Thinking about that, I wonder why this
> > struct exists at all. I would have done this as follows:
> >
> > #define MESON_PWM_REG_PWM(chan) (0 + 4 * (chan))
> >
> > #define MESON_PWM_REG_MISC (8)
> > #define MESON_PWM_REG_MISC_EN(chan) BIT(chan)
> > #define MESON_PWM_REG_MISC_CLK_SEL(chan) GENMASK(5 + 2 * (chan), 4 + 2 * (chan))
> > ....
> >
> > and then use these constants directly (with pwm->hwpwm as parameter if
> > needed) in the code. I would expect this to result in more efficient and
> > smaller code.
>
> I've been looking into this driver for more than a year and got used to
> it so much so never thought about changing the foundations :) Although it's
> an interesting thought.
>
> 1. I took meson_pwm_enable() without
> const patches and reimplemented it using only defines (e.g. w/o local
> var channel_data) and objdumped current and new versions. New version
> turned out to be one instruction longer (arm64, gcc, default -O2). So total
> difference in executable code may be not that significant although
> we can win in C-code line count.
Oh, I indeed would have expected a slight advantage size-wise. Maybe the
compiler already optimizes out the meson_pwm_per_channel_data variable.
> 2. Things like
> #define MISC_B_EN BIT(1)
> #define MISC_A_EN BIT(0)
> is more straightforward and can be matched to the datasheet easier
> comparing to (a + b * (chan)) things.
In my (subjective) view that comparison isn't hard with the
parametrised definition.
> So I'm not sure either.
>
> > > @@ -227,6 +252,15 @@ static void meson_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > value = readl(meson->base + REG_MISC_AB);
> > > value |= channel_data->pwm_en_mask;
> > > +
> > > + if (meson->data->has_constant)
> > > + meson_pwm_assign_bit(&value, channel_data->const_en_mask,
> > > + channel->constant);
> >
> > Personally I'd prefer:
> >
> > value &= ~MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
> > if (meson->data->has_constant && channel->constant)
> > value |= MESON_PWM_REG_MISC_CONST_EN(pwm->hwpwm);
> >
> > even though your variant only mentions the mask once. While it has this
> > repetition, it's clear what happens without having to know what
> > meson_pwm_assign_bit() does. Maybe that's subjective?
>
> Actually I also don't like meson_pwm_assign_bit() too match and I'm
> surprised there's no something like this in the kernel already.
> I again objdumped versions meson_pwm_assign_bit() vs double mask repetition.
> Unconditional bit clearing takes only a single instruction:
>
> // value &= ~channel_data->const_en_mask;
> 9ac: 0a250040 bic w0, w2, w5
>
> So in the current series I could drop meson_pwm_assign_bit() and use:
>
> value &= ~channel_data->const_en_mask;
> if (meson->data->has_constant && channel->constant)
> value |= channel_data->const_en_mask;
>
> If it's decided now or later to drop meson_pwm_channel_data then
> w\o meson_pwm_assign_bit() future patch will be line-to-line change.
>
> What you think?
Sounds sensible.
Best regards
Uwe
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists