| Message ID | 20250612-byeword-update-v1-0-f4afb8f6313f@collabora.com |
|---|---|
| Headers | show |
| Series | BYEWORD_UPDATE: unifying (most) HIWORD_UPDATE macros | expand |
Hi Nicolas, On 6/12/25 9:56 PM, Nicolas Frattaroli wrote: > The era of hand-rolled HIWORD_UPDATE macros is over, at least for those > drivers that use constant masks. > > Remove VOP2's HIWORD_UPDATE macro from the vop2 header file, and replace > all instances in rockchip_vop2_reg.c (the only user of this particular > HIWORD_UPDATE definition) with equivalent HWORD_UPDATE instances. This > gives us better error checking. > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> This LGTM and I also confirm it works as expected on my Radxa boards: ROCK 3A (RK3568) and ROCK 5B (RK3588). Hence, Reviewed-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> Tested-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
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()?
This series was spawned by [1], where I was asked to move every instance of HIWORD_UPDATE et al that I could find to a common macro in the same series that I am introducing said common macro. The first patch of the series introduces the two new macros in bitfield.h, called HWORD_UPDATE and HWORD_UPDATE_CONST. The latter can be used in initializers. This macro definition checks that the mask fits, and that the value fits in the mask. Like FIELD_PREP, it also shifts the value up to the mask, so turning off a bit does not require using the mask as a value. Masks are also required to be contiguous, like with FIELD_PREP. For each definition of such a macro, the driver(s) that used it were evaluated for three different treatments: - full conversion to the new macro, for cases where replacing the implementation of the old macro wouldn't have worked, or where the conversion was trivial. These are the most complex patches in this series, as they sometimes have to pull apart definitions of masks and values due to the new semantics, which require a contiguous mask and shift the value for us. - replacing the implementation of the old macro with an instance of the new macro, done where I felt it made the patch much easier to review because I didn't want to drop a big diff on people. - skipping conversion entirely, usually because the mask is non-constant and it's not trivial to make it constant. Sometimes an added complication is that said non-constant mask is either used in a path where runtime overhead may not be desirable, or in an initializer. Left out of conversion: - drivers/mmc/host/sdhci-of-arasan.c: mask is non-constant. - drivers/phy/rockchip/phy-rockchip-inno-csidphy.c: mask is non-constant likely by way of runtime pointer dereferencing, even if struct and members are made const. - drivers/clk/rockchip/clk.h: way too many clock drivers use non-const masks in the context of an initializer. I will not be addressing these 3 remaining users in this series, as implementing a runtime checked version on top of this and verifying that it doesn't cause undue overhead just for 3 stragglers is a bit outside the scope of wanting to get my RK3576 PWM series unblocked. Please have mercy. In total, I count 19 different occurrences of such a macro fixed out of 22 I found. The vast majority of these patches have either undergone static testing to ensure the values end up the same during development, or have been verified to not break the device the driver is for at runtime. Only a handful are just compile-tested, and the individual patches remark which ones those are. This took a lot of manual work as this wasn't really something that could be automated: code had to be refactored to ensure masks were contiguous, made sense to how the hardware actually works and to human readers, were constant, and that the code uses unshifted values. https://lore.kernel.org/all/aD8hB-qJ4Qm6IFuS@yury/ [1] Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com> --- Nicolas Frattaroli (20): bitfield: introduce HWORD_UPDATE bitfield macros mmc: dw_mmc-rockchip: switch to HWORD_UPDATE macro soc: rockchip: grf: switch to HWORD_UPDATE_CONST macro media: synopsys: hdmirx: replace macros with bitfield variants drm/rockchip: lvds: switch to HWORD_UPDATE macro phy: rockchip-emmc: switch to HWORD_UPDATE macro drm/rockchip: dsi: switch to HWORD_UPDATE* macros drm/rockchip: vop2: switch to HWORD_UPDATE macro phy: rockchip-samsung-dcphy: switch to HWORD_UPDATE macro drm/rockchip: dw_hdmi_qp: switch to HWORD_UPDATE macro drm/rockchip: inno-hdmi: switch to HWORD_UPDATE macro phy: rockchip-usb: switch to HWORD_UPDATE macro drm/rockchip: dw_hdmi: switch to HWORD_UPDATE* macros ASoC: rockchip: i2s-tdm: switch to HWORD_UPDATE_CONST macro net: stmmac: dwmac-rk: switch to HWORD_UPDATE macro PCI: rockchip: switch to HWORD_UPDATE* macros PCI: dw-rockchip: switch to HWORD_UPDATE macro PM / devfreq: rockchip-dfi: switch to HWORD_UPDATE macro clk: sp7021: switch to HWORD_UPDATE macro phy: rockchip-pcie: switch to HWORD_UPDATE macro drivers/clk/clk-sp7021.c | 21 +-- drivers/devfreq/event/rockchip-dfi.c | 26 ++-- drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 142 ++++++++++----------- drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 80 ++++++------ drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 68 +++++----- drivers/gpu/drm/rockchip/inno_hdmi.c | 11 +- drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 1 - drivers/gpu/drm/rockchip/rockchip_lvds.h | 21 +-- drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 14 +- .../media/platform/synopsys/hdmirx/snps_hdmirx.h | 5 +- drivers/mmc/host/dw_mmc-rockchip.c | 7 +- drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 3 +- drivers/pci/controller/dwc/pcie-dw-rockchip.c | 39 +++--- drivers/pci/controller/pcie-rockchip.h | 35 ++--- drivers/phy/rockchip/phy-rockchip-emmc.c | 3 +- drivers/phy/rockchip/phy-rockchip-pcie.c | 72 +++-------- drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c | 10 +- drivers/phy/rockchip/phy-rockchip-usb.c | 51 +++----- drivers/soc/rockchip/grf.c | 35 +++-- include/linux/bitfield.h | 47 +++++++ sound/soc/rockchip/rockchip_i2s_tdm.h | 4 +- 21 files changed, 342 insertions(+), 353 deletions(-) --- base-commit: d9946fe286439c2aeaa7953b8c316efe5b83d515 change-id: 20250610-byeword-update-445c0eea771d Best regards,