Re: [PATCH 01/20] bitfield: introduce HWORD_UPDATE bitfield macros

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

Hello,

On Friday, 13 June 2025 10:51:15 Central European Summer Time Jani Nikula wrote:
> On Thu, 12 Jun 2025, Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:
> > Hardware of various vendors, but very notably Rockchip, often uses
> > 32-bit registers where the upper 16-bit half of the register is a
> > write-enable mask for the lower half.
> >
> > This type of hardware setup allows for more granular concurrent register
> > write access.
> >
> > Over the years, many drivers have hand-rolled their own version of this
> > macro, usually without any checks, often called something like
> > HIWORD_UPDATE or FIELD_PREP_HIWORD, commonly with slightly different
> > semantics between them.
> >
> > Clearly there is a demand for such a macro, and thus the demand should
> > be satisfied in a common header file.
> >
> > Add two macros: HWORD_UPDATE, and HWORD_UPDATE_CONST. The latter is a
> > version that can be used in initializers, like FIELD_PREP_CONST. The
> > macro names are chosen to not clash with any potential other macros that
> > drivers may already have implemented themselves, while retaining a
> > familiar name.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  include/linux/bitfield.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> >
> > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > index 6d9a53db54b66c0833973c880444bd289d9667b1..b90d88db7405f95b78cdd6f3426263086bab5aa6 100644
> > --- a/include/linux/bitfield.h
> > +++ b/include/linux/bitfield.h
> > @@ -8,6 +8,7 @@
> >  #define _LINUX_BITFIELD_H
> >  
> >  #include <linux/build_bug.h>
> > +#include <linux/limits.h>
> >  #include <linux/typecheck.h>
> >  #include <asm/byteorder.h>
> >  
> > @@ -142,6 +143,52 @@
> >  		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
> >  	)
> >  
> > +/**
> > + * HWORD_UPDATE() - prepare a bitfield element with a mask in the upper half
> > + * @_mask: shifted mask defining the field's length and position
> > + * @_val:  value to put in the field
> > + *
> > + * HWORD_UPDATE() masks and shifts up the value, as well as bitwise ORs the
> > + * result with the mask shifted up by 16.
> > + *
> > + * This is useful for a common design of hardware registers where the upper
> > + * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
> > + * register, a bit in the lower half is only updated if the corresponding bit
> > + * in the upper half is high.
> > + */
> > +#define HWORD_UPDATE(_mask, _val)					 \
> > +	({								 \
> > +		__BF_FIELD_CHECK(_mask, ((u16) 0U), _val,		 \
> > +				 "HWORD_UPDATE: ");			 \
> > +		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)) | \
> > +		((_mask) << 16);					 \
> > +	})
> 
> i915 uses something like this for a few registers too, with the name
> _MASKED_FIELD(). I think we could use it.
> 
> I do think this is clearly an extension of FIELD_PREP(), though, and
> should be be named similarly, instead of the completely deviating
> HWORD_UPDATE().
> 
> Also, we recently got GENMASK() versions with sizes, GENMASK_U16()
> etc. so I find it inconsistent to denote size here with HWORD.
> 
> FIELD_PREP_MASKED_U16? MASKED_FIELD_PREP_U16? Something along those
> lines?

Yeah, I agree the name could be better. I used HWORD_UPDATE as Yury and
I couldn't come up with a name we liked either, and Yury suggested not
breaking from what's already there too much. I do think making the name
more field-adjacent would be good though, as well as somehow indicating
that it is 16 bits of data.

> 
> And perhaps that (and more potential users) could persuade Jakub that
> this is not that weird after all?

I will operate under the assumption that Jakub's opinion will not change
as he ignored the commit message that talks about multiple vendors,
ignored the cover letter that talks about multiple vendors, and ignored
my e-mail where I once again made it clear to him that it's multiple
vendors, and still claims it's a Rockchip specific convention.

> 
> 
> BR,
> Jani.
> 

Best Regards,
Nicolas Frattaroli

> 
> 
> 
> > +
> > +/**
> > + * HWORD_UPDATE_CONST() - prepare a constant bitfield element with a mask in
> > + *                        the upper half
> > + * @_mask: shifted mask defining the field's length and position
> > + * @_val:  value to put in the field
> > + *
> > + * HWORD_UPDATE_CONST() masks and shifts up the value, as well as bitwise ORs
> > + * the result with the mask shifted up by 16.
> > + *
> > + * This is useful for a common design of hardware registers where the upper
> > + * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
> > + * register, a bit in the lower half is only updated if the corresponding bit
> > + * in the upper half is high.
> > + *
> > + * Unlike HWORD_UPDATE(), this is a constant expression and can therefore
> > + * be used in initializers. Error checking is less comfortable for this
> > + * version.
> > + */
> > +#define HWORD_UPDATE_CONST(_mask, _val)					  \
> > +	(								  \
> > +		FIELD_PREP_CONST(_mask, _val) |				  \
> > +		(BUILD_BUG_ON_ZERO(const_true((u64) (_mask) > U16_MAX)) + \
> > +		 ((_mask) << 16))					  \
> > +	)
> > +
> >  /**
> >   * FIELD_GET() - extract a bitfield element
> >   * @_mask: shifted mask defining the field's length and position
> 
>