| Message ID | 20250612-byeword-update-v1-1-f4afb8f6313f@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | BYEWORD_UPDATE: unifying (most) HIWORD_UPDATE macros | expand |
On Thu, 12 Jun 2025 20:56:03 +0200 Nicolas Frattaroli 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. Please limit the spread of this weirdness to a rockchip or "hiword" specific header. To a normal reader of bitfield.h these macros will be equally confusing and useless.
On Thursday, 12 June 2025 21:44:15 Central European Summer Time Jakub Kicinski wrote: > On Thu, 12 Jun 2025 20:56:03 +0200 Nicolas Frattaroli 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. > > Please limit the spread of this weirdness to a rockchip or "hiword" > specific header. To a normal reader of bitfield.h these macros will > be equally confusing and useless. > That is how this change started out, and then a different maintainer told me that this is a commonly used thing (see: the sunplus patch), and Rockchip just happens to have a lot of these with consistent naming. I believe normal readers of bitfield.h will be much more confused by the undocumented concatenating macro soup at the end, but maybe that's just me. Best regards, Nicolas Frattaroli
On Thu, Jun 12, 2025 at 09:50:12PM +0200, Nicolas Frattaroli wrote: > On Thursday, 12 June 2025 21:44:15 Central European Summer Time Jakub Kicinski wrote: > > On Thu, 12 Jun 2025 20:56:03 +0200 Nicolas Frattaroli 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. > > > > Please limit the spread of this weirdness to a rockchip or "hiword" > > specific header. To a normal reader of bitfield.h these macros will > > be equally confusing and useless. > > > > That is how this change started out, and then a different maintainer told > me that this is a commonly used thing (see: the sunplus patch), and > Rockchip just happens to have a lot of these with consistent naming. That other maintainer was me, and the macro is indeed not used by rockchip weirdness solely: $ git grep HIWORD | grep -v rockchip | wc -l 326 I don't think that that having HWORD_UPDATE() in bitfield.h is a wrong thing. Jakub, if you do, we can just create a new header for it. Thanks, Yury
On Thu, 12 Jun 2025 16:10:37 -0400 Yury Norov wrote: > I don't think that that having HWORD_UPDATE() in bitfield.h is a wrong > thing. Jakub, if you do, we can just create a new header for it. Yes, I'd prefer to contain it. This looks very much like a CSR tooling convention of Rockchip's ASIC developers. IOW this is really about how CSRs are access for a specific vendor, not a generic bitfield operator.
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? And perhaps that (and more potential users) could persuade Jakub that this is not that weird after all? BR, Jani. > + > +/** > + * 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
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 > >
On Fri, Jun 13, 2025 at 01:55:54PM +0200, Nicolas Frattaroli wrote: > 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. I suggested a wonderful name that explains everything. Didn't I? It has the only problem - it's 25 chars long. :) So yeah, let's think once more about a better _short_ name, or just stick to the existing naming scheme. > > 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. As far as I understood, he concerns not about number of drivers that opencode HIWORD_UPDATE(), but that this macro is not generic enough to live in bitfield.h. And it's a valid concern - I doubt it will be helpful somewhere in core and arch files. I think that creating a separate header like hw_bitfield.h, or hw_bits.h aimed to absorb common helpers of that sort, would help to reach the strategic goal - decreasing the level of code duplication in the driver swamp. Thanks, Yury
On 2025-06-12 7:56 pm, Nicolas Frattaroli 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. Nit: while from one angle it indeed looks similar, from another it's even more opaque and less meaningful than what we have already. Personally I cannot help but see "hword" as "halfword", so logically if we want 32+32-bit or 8+8-bit variants in future those would be WORD_UPDATE() and BYTE_UPDATE(), right? ;) It's also confounded by "update" not actually having any obvious meaning at this level without all the implicit usage context. FWIW my suggestion would be FIELD_PREP_WM_U16, such that the reader instantly sees "FIELD_PREP with some additional semantics", even if they then need to glance at the kerneldoc for clarification that WM stands for writemask (or maybe WE for write-enable if people prefer). Plus it then leaves room to easily support different sizes (and potentially even bonkers upside-down Ux_WM variants?!) without any bother if we need to. Thanks, Robin. > 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); \ > + }) > + > +/** > + * 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 >
On Fri, Jun 13, 2025 at 02:54:50PM +0100, Robin Murphy wrote: > On 2025-06-12 7:56 pm, Nicolas Frattaroli 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. > > Nit: while from one angle it indeed looks similar, from another it's even > more opaque and less meaningful than what we have already. Personally I > cannot help but see "hword" as "halfword", so logically if we want 32+32-bit > or 8+8-bit variants in future those would be WORD_UPDATE() and > BYTE_UPDATE(), right? ;) > > It's also confounded by "update" not actually having any obvious meaning at > this level without all the implicit usage context. FWIW my suggestion would > be FIELD_PREP_WM_U16, such that the reader instantly sees "FIELD_PREP with > some additional semantics", even if they then need to glance at the > kerneldoc for clarification that WM stands for writemask (or maybe WE for > write-enable if people prefer). Plus it then leaves room to easily support > different sizes (and potentially even bonkers upside-down Ux_WM variants?!) > without any bother if we need to. I like the idea. Maybe even shorter: FIELD_PREP_WM16()?
On Fri, 13 Jun 2025 08:28:40 -0400 Yury Norov wrote: > > > 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. > > As far as I understood, he concerns not about number of drivers that > opencode HIWORD_UPDATE(), but that this macro is not generic enough > to live in bitfield.h. And it's a valid concern - I doubt it will > be helpful somewhere in core and arch files. Exactly.
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); \ + }) + +/** + * 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
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(+)