linux-kernel - Re: [PATCH v2 2/4] pwm: meson: Support constant and polarity bits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <fbafp4qvf7xq7ob7pdtrps2bgk7n5azcysq4jpqhwxtxxhyt5g@kq763dnqkk7m>
Date: Tue, 19 Nov 2024 16:19:20 +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

Hello George,

On Tue, Nov 19, 2024 at 03:51:34PM +0300, George Stark wrote:
> On 11/7/24 11:41, Uwe Kleine-König wrote:
> > 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,
> > > > >    	}
> > > > >    };
> 
> ...
> 
> > > > 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.
> 
> While changing the patch to drop meson_pwm_assign_bit() one thing
> concerned me on the approach:
> 
> value &= ~channel_data->const_en_mask;
> if (meson->data->has_constant && channel->constant)
> 	value |= channel_data->const_en_mask;
> 
> that we reset bit in the value var even if that bit is not supported on
> the current SoC. I checked several datasheets for old SoCs and those bits
> are marked as unused (not even reserved) and I've never seen those
> bits set. Still I'd offer to use precise condition for changing those bit.
> I'll send v3 let's discuss it again if you think I bother too much.

Usually writing zeros to unused bits is a safe procedure. If the
hardware we're talking to is a newer variant of the supported stuff, the
hardware engineer did something wrong if keeping the read bits or
writing them as zero is incompatible. So either way is fine for me.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)