mbox series

[v2,0/7] Add Rockchip RK3576 PWM Support Through MFPWM

Message ID 20250602-rk3576-pwm-v2-0-a6434b0ce60c@collabora.com
Headers show
Series Add Rockchip RK3576 PWM Support Through MFPWM | expand

Message

Nicolas Frattaroli June 2, 2025, 4:19 p.m. UTC
This series introduces support for some of the functions of the new PWM
silicon found on Rockchip's RK3576 SoC. Due to the wide range of
functionalities offered by it, including many parts which this series'
first iteration does not attempt to implement for now, it uses multiple
drivers hanging on a platform bus on which the parent "mfpwm" driver
registers them.

AUXBUS/MFD/PLATFORM BUS DISCUSSION: if you are currently wondering why
you were Cc'd and are about to move onto the next e-mail, it may be
because we're currently trying to figure out whether this smorgasbord of
drivers should be a MFD driver, a platform bus driver, or an auxbus
driver. It is currently implemented as a platform bus driver. Any
additional insight from people who know the conceptual place that MFD
has would be appreciated.

Here's some of the features of the hardware:
- Continuous PWM output (implemented in this series)
- One-shot/Finite repetition PWM output
- PWM capture by counting high/low cycles (implemented in this series)
- Sending IR transmissions in several TV remote protocols
- Generating an interrupt based on the input being one of 16
  user-specified values ("Power key capture")
- Biphasic counter support
- Using the hardware to measure a clock signal's frequency
- Using the hardware to count a clock signal's pulses
- Generating PWM output waveforms through a user-specified lookup table

As you can tell, there's a lot. I've focused on continuous PWM output
for now as the most important one for things like controlling fans. The
PWM capture driver is an added bonus, because I needed at least two
drivers to test things. Anyone doing consumer electronic devices like
TVs based on the RK3576 may need to do the power key stuff at some
stage, as it can be used to wake up the SoC with an IR remote. The IR
transmission stuff in general may be a funny weekend project for someone
at some point; I assume it's there so TV boxes can turn on and off TVs
without needing the HDMI control stuff.

At first, I considered simply integrating support for this new IP into
the old pwm-rockchip driver, as the downstream vendor kernel did.
However, the IP is significantly different from previous iterations.
Especially if the goal is to support some of the additional
functionality that the new silicon brings, doing it all in a single pwm
driver would be untenable. Especially one that already supports other
hardware with a way different set of registers.

Hence, the mfpwm pattern: each device functionality is its own driver,
and they all get registered as platform drivers by the parent mfpwm
driver, which is the one that binds to the DT compatible. Each device
function driver then has to _acquire and _release the hardware when it
needs control of it. If some other device function is using the device
already, -EBUSY is returned, which the device function driver can then
forward to the user and everyone is happy.

The PWM output driver, pwm-rockchip-v4, uses the new waveform APIs. I
thought while writing a new driver that I might as well use the new
APIs.

The PWM capture driver, implemented as a counter driver, is somewhat
primitive, in that it doesn't make use of things like the biphasic
counter support or clock measuring, but it serves as a good way to
showcase and test the mutual exclusion that the mfpwm framework tries to
achieve. It also goes one step beyond just exposing the raw LPC/HPC
counts and actually makes them usable in a non-theoretically-racey way
by turning them into nanosecond period/duty cycle values based on the
clock's rate. Shoutouts to the counter subsystem's documentation by the
way, it is some of the best subsystem documentation I've come across so
far, and was a great help.

All instances of the PWM controller have three clocks that they can pick
and choose to derive the PWM signal from. One is the default PLL from
the CRU, one is the 24 MHz crystal oscillator (gated by the CRU), and
one is an RC oscillator (also gated by the CRU). Each PWM channel can
switch between these with a clock selection register in the PWM register
range, hence this is implemented as a clock mux.

Along the way, I also finally took the time to give us The One True
HIWORD_UPDATE macro, which aims to replace all other copies of the
HIWORD_UPDATE macro, except it's not called HIWORD_UPDATE because all of
them have slightly different semantics and I don't want to introduce
even more confusion. It does, however, do some compile-time checking of
the function-like macro parameters.

This series went through two substantial rewrites, because after I
realised it needed to be multiple drivers (first rewrite) I then first
wrote it as a couple of auxiliary bus drivers, until I became unsure
about whether this should be auxiliary bus drivers at all, so it became
a bunch of platform bus drivers instead. If anything looks like a weird
vestigial leftover in the code, then that's probably why, but I made
sure to get rid of all the ones I could find before submitting this.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Changes in v2:
- bindings: make osc required (as it's present in all instances of the
  hardware I'm aware of) and add the rc clock as well. I thought it
  wasn't present on some instances of the PWM IP due to the vendor SoC
  dtsi, but checking the CRU made me realise those clocks do exist for
  all instances. Did not include Conor's R-b as this constitutes a
  substantial enough change to necessitate a re-review
- move bitfield write-enable mask macros into bitfield.h by replacing
  the original rockchip-specific utils header patch with a bitfield.h
  patch.
- mfpwm: change all instances of WARN to be dev_warn instead, as we have
  a device pointer.
- mfpwm: replace the ad-hoc clock mux implementation that used a sysfs
  interface with a generic clk-mux.
- mfpwm: add the rc clock
- mfpwm: rename all the pwmv4_ prefixed functions to have the
  rockchip_pwm_v4_ prefix instead
- mfpwm: remove the pwmclk indirection, hand chosen_clk to pwmf
- mfpwm: move to use the new bitfield macros for the WE mask
- mfpwm: mark reg access inline functions as static to fix build errors
- pwm-rockchip-v4 pwm output: replace mult_frac with mul_u64_u64_div_u64
- pwm-rockchip-v4 pwm output: don't return error if parameters are out
  of range, just set them to the maximum
- pwm-rockchip-v4 pwm output: add rate to debug message
- pwm-rockchip-v4 pwm output: if rate is 0 and pwm is disabled, set
  waveform parameters to 0. The clock is expected to not have a rate in
  this case.
- pwm-rockchip-v4 pwm output: add pwmchip_remove in remove callback,
  which also necessitated using chip as the platdata instead of the
  driver private struct
- pwm-rockchip-v4 pwm output: rework PWMV4_CTRL_UPDATE_EN since it never
  needs to be set to 0 by the driver
- pwm-rockchip-v4 pwm output: add a limitations list
- pwm-rockchip-v4 pwm output: handle initial hardware state during
  probe, enabling the pwm clock if the PWM is on and in continuous mode
- pwm-rockchip-v4 pwm output: rename pwmv4_is_enabled to use the
  rockchip_pwm_v4_ prefix instead
- pwm-rockchip-v4 pwm output: remove pwmclk indirection, use clk API
  directly
- pwm-rockchip-v4 pwm output: no longer claim the chip as being atomic,
  as the clk_rate_exclusive_get calls may sleep.
- rockchip-pwm-capture counter: remove pwmclk indirection, use clk API
  directly
- rockchip-pwm-capture counter: replace mult_frac with
  mul_u64_u64_div_u64
- rockchip-pwm-capture counter: don't output periods/duty cycles if the
  period is longer than the chosen timeout; this works around the
  hardware cycle counter seemingly being impossible to clear
- dts: added osc and rc to every pwm node
- dts: reordered properties in pwm0 to be sorted
- Link to v1: https://lore.kernel.org/r/20250408-rk3576-pwm-v1-0-a49286c2ca8e@collabora.com

---
Nicolas Frattaroli (7):
      dt-bindings: pinctrl: rockchip: increase max amount of device functions
      dt-bindings: pwm: Add a new binding for rockchip,rk3576-pwm
      bitfield: introduce HI16_WE bitfield prep macros
      soc: rockchip: add mfpwm driver
      pwm: Add rockchip PWMv4 driver
      counter: Add rockchip-pwm-capture driver
      arm64: dts: rockchip: add PWM nodes to RK3576 SoC dtsi

 .../bindings/pinctrl/rockchip,pinctrl.yaml         |   2 +-
 .../bindings/pwm/rockchip,rk3576-pwm.yaml          |  77 ++++
 MAINTAINERS                                        |  11 +
 arch/arm64/boot/dts/rockchip/rk3576.dtsi           | 208 +++++++++
 drivers/counter/Kconfig                            |  13 +
 drivers/counter/Makefile                           |   1 +
 drivers/counter/rockchip-pwm-capture.c             | 352 +++++++++++++++
 drivers/pwm/Kconfig                                |  13 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-rockchip-v4.c                      | 372 ++++++++++++++++
 drivers/soc/rockchip/Kconfig                       |  13 +
 drivers/soc/rockchip/Makefile                      |   1 +
 drivers/soc/rockchip/mfpwm.c                       | 398 +++++++++++++++++
 include/linux/bitfield.h                           |  47 ++
 include/soc/rockchip/mfpwm.h                       | 484 +++++++++++++++++++++
 15 files changed, 1992 insertions(+), 1 deletion(-)
---
base-commit: ba2b2250bbaf005016ba85e345add6e19116a1ea
change-id: 20250407-rk3576-pwm-46761bd0deaa

Best regards,

Comments

Heiko Stuebner June 2, 2025, 7:01 p.m. UTC | #1
Am Montag, 2. Juni 2025, 18:19:14 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> 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: FIELD_PREP_HI16_WE, and FIELD_PREP_HI16_WE_CONST. The
> latter is a version that can be used in initializers, like
> FIELD_PREP_CONST. The macro names are chosen to explicitly reference the
> assumed half-register width, and its function, while not clashing with
> any potential other macros that drivers may already have implemented
> themselves.
> 
> Future drivers should use these macros instead of handrolling their own,
> and old drivers can be ported to the new macros as time and opportunity
> allows.
> 
> 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..2b3e7cb90ccb5d48f510104f61443b06748bb7eb 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))	\
>  	)
>  
> +/**
> + * FIELD_PREP_HI16_WE() - prepare a bitfield element with a write-enable mask
> + * @_mask: shifted mask defining the field's length and position
> + * @_val:  value to put in the field
> + *
> + * FIELD_PREP_HI16_WE() 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 FIELD_PREP_HI16_WE(_mask, _val)					\
> +	({								\
> +		__BF_FIELD_CHECK(_mask, ((u16) 0U), _val,		\
> +				 "FIELD_PREP_HI16_WE: ");		\
> +		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |	\

gcc is quite adamant about suggesting more parentheses here:

../include/linux/bitfield.h:163:70: warning: suggest parentheses around arithmetic in operand of ‘|’ [-Wparentheses]
  163 |                 ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |  \
      |                                                                      ^
../include/soc/rockchip/mfpwm.h:225:41: note: in expansion of macro ‘FIELD_PREP_HI16_WE’
  225 | #define PWMV4_MODE(v)                   FIELD_PREP_HI16_WE(PWMV4_MODE_MASK, (v))
      |                                         ^~~~~~~~~~~~~~~~~~
../include/soc/rockchip/mfpwm.h:230:34: note: in expansion of macro ‘PWMV4_MODE’
  230 | #define PWMV4_CTRL_CONT_FLAGS   (PWMV4_MODE(PWMV4_MODE_CONT) | \
      |                                  ^~~~~~~~~~
../drivers/pwm/pwm-rockchip-v4.c:237:57: note: in expansion of macro ‘PWMV4_CTRL_CONT_FLAGS’
  237 |         mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL, PWMV4_CTRL_CONT_FLAGS);
      |                                                         ^~~~~~~~~~~~~~~~~~~~~


Heiko
Nicolas Frattaroli June 3, 2025, 12:55 p.m. UTC | #2
On Monday, 2 June 2025 22:02:19 Central European Summer Time Yury Norov wrote:
> On Mon, Jun 02, 2025 at 06:19:14PM +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.
> 
> Can you list them all explicitly please? I grepped myself for the
> 'HIGHWORD_UPDATE' and 'FIELD_PREP_HIGWORD', and found just 4 or 5 in
> addition to the rockchip.

Most of the ones Heiko brought up[1] just appear to be the clock stuff,
I'm only aware of the drivers/mmc/host/sdhci-of-arasan.c one outside of
Rockchip. For a complete listing I'd have to do a semantic search with
e.g. Coccinelle, which I've never used before and would need to wrap
my head around first. grep is a bad fit for catching them all as some
macros are split across lines, or reverse the operators of the OR.
Weggli[2] is another possibility but it's abandoned and undocumented, and
I've ran into its limitations before fairly quickly.

>  
> > 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.
> 
> I agree. Nice catch.
> 
> > Add two macros: FIELD_PREP_HI16_WE, and FIELD_PREP_HI16_WE_CONST. The
> > latter is a version that can be used in initializers, like
> > FIELD_PREP_CONST.
> 
> I'm not sure that the name you've chosen reflects the intention. If
> you just give me the name without any background, I'd bet it updates
> the HI16 part of presumably 32-bit field. The 'WE' part here is most
> likely excessive because at this level of abstraction you can't
> guarantee that 'write-enable mask' is the only purpose for the macro.
> 
> > The macro names are chosen to explicitly reference the
> > assumed half-register width, and its function, while not clashing with
> > any potential other macros that drivers may already have implemented
> > themselves.
> >
> > Future drivers should use these macros instead of handrolling their own,
> > and old drivers can be ported to the new macros as time and opportunity
> > allows.
> 
> This is a wrong way to go. Once you introduce a macro that replaces
> functionality of few other arch or driver macros, you should consolidate
> them all in the same series. Otherwise, it will be just another flavor
> of the same, but now living in a core header. 
> 
> Can you please prepare a series that introduces the new macro and
> wires all arch duplications to it?

Okay, I will do that after I learn Coccinelle. Though I suspect the reason
why I'm the first person to address this is because it's much easier to
hide duplicated macros away in drivers than go the long route of fixing up
every single other user. I'm not too miffed about it though, it's cleanup
of technical debt that's long overdue.

>  
> > 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..2b3e7cb90ccb5d48f510104f61443b06748bb7eb 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))	\
> >  	)
> >  
> > +/**
> > + * FIELD_PREP_HI16_WE() - prepare a bitfield element with a write-enable mask
> > + * @_mask: shifted mask defining the field's length and position
> > + * @_val:  value to put in the field
> > + *
> > + * FIELD_PREP_HI16_WE() 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 FIELD_PREP_HI16_WE(_mask, _val)					\
> > +	({								\
> > +		__BF_FIELD_CHECK(_mask, ((u16) 0U), _val,		\
> > +				 "FIELD_PREP_HI16_WE: ");		\
> > +		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |	\
> > +		((_mask) << 16);					\
> > +	})
> 
> This pretty much is a duplication of the FIELD_PREP(), isn't? Why don't
> you borrow the approach from drivers/clk/clk-sp7021.c:
> 
> 	/* HIWORD_MASK FIELD_PREP */
> 	#define HWM_FIELD_PREP(mask, value)             \
> 	({                                              \
> 	        u64 _m = mask;                          \
> 	        (_m << 16) | FIELD_PREP(_m, value);     \
> 	})
> 
> If you do so, the existing FIELD_PREP() will do all the work without
> copy-pasting.

Because then the __BF_FIELD_CHECK macro will be invoked twice, once without
the proper prefix. Factoring the actual prep-no-check operation out into a
separate macro is macro definition + 1-line invocation * 2, whereas copy-
pasting the implementation that will never change is 1-line invocation*2.

> The only questionI have  to the above macro is why '_m'
> is u64? Seemingly, it should be u32?

I didn't write the HWM_FIELD_PREP macro in clk-sp7021.c, nor am I familiar
with the hardware. It's possible they were trying to prevent an overflow
wraparound here though, but they're not checking if the result ends up
greater than 32 bits so that seems suspect.

> Regarding the name... I can't invent a good one as well, so the best
> thing I can suggest is not to invent something that can mislead. The
> HWM_FIELD_PREP() is not bad because it tells almost nothing and
> encourages one to refer to the documentation. If you want something
> self-explaining, maybe MASK_HI_FIELD_LO_PREP_U16(), or something?

This seems a bit unwieldy, at 25 characters. "FIELD32_HIMASK_LOPREP"
(or FIELD16, depending on which end of the cornet to eat) would be 21
characters but I'm also not in love with it.

I think the name should include the following parts:
1. it's a field
2. the field is halved into two halves of 16 bits
3. the mask is copied into the upper 16 bits

Since we're on the subject of bit widths, I have a somewhat sacrilegious
point to raise: should this be a function-like macro at all, as opposed
to a static __pure inline function? It's not generic with regards to the
data types, as we're always assuming a u16 value and mask input and a
u32 output. The __pure inline definition should let the compiler treat it
essentially similar to what the pre-processor expanded macro does, which
is as not a function call at all but a bunch of code to constant fold away
if possible. What we get in return is type checking and less awful syntax.
Then we could call it something like `himask_field_prep_u32`, which is
also 21 characters but the ambiguity of whether the u32 refers to the mask
or the whole register width is cleared up by the types of the function
arguments.

The const version of the macro may still need to remain though because I'm
not sure C11 can do that for us. With C23 maybe there's a way with
constexpr but I've never used it before.

> 
> Thanks,
> Yury
> 

Kind Regards,
Nicolas Frattaroli

Link: https://lore.kernel.org/linux-rockchip/1895349.atdPhlSkOF@diego/ [1]
Link: https://github.com/weggli-rs/weggli [2]

> > +
> > +/**
> > + * FIELD_PREP_HI16_WE_CONST() - prepare a constant bitfield element with a
> > + *                              write-enable mask
> > + * @_mask: shifted mask defining the field's length and position
> > + * @_val:  value to put in the field
> > + *
> > + * FIELD_PREP_HI16_WE_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 FIELD_PREP_HI16_WE(), this is a constant expression and can therefore
> > + * be used in initializers. Error checking is less comfortable for this
> > + * version, and non-constant masks cannot be used.
> > + */
> > +#define FIELD_PREP_HI16_WE_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
> > 
>
Yury Norov June 3, 2025, 4:21 p.m. UTC | #3
On Tue, Jun 03, 2025 at 02:55:40PM +0200, Nicolas Frattaroli wrote:
> On Monday, 2 June 2025 22:02:19 Central European Summer Time Yury Norov wrote:
> > On Mon, Jun 02, 2025 at 06:19:14PM +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.
> > 
> > Can you list them all explicitly please? I grepped myself for the
> > 'HIGHWORD_UPDATE' and 'FIELD_PREP_HIGWORD', and found just 4 or 5 in
> > addition to the rockchip.
> 
> Most of the ones Heiko brought up[1] just appear to be the clock stuff,
> I'm only aware of the drivers/mmc/host/sdhci-of-arasan.c one outside of
> Rockchip. For a complete listing I'd have to do a semantic search with
> e.g. Coccinelle, which I've never used before and would need to wrap
> my head around first. grep is a bad fit for catching them all as some
> macros are split across lines, or reverse the operators of the OR.
> Weggli[2] is another possibility but it's abandoned and undocumented, and
> I've ran into its limitations before fairly quickly.

Going Coccinelle way is fine, but I think it's an endless route. :)

What I meant is: you caught this HIWORD_UPDATE() duplication, and it's
great. When people copy-paste a macro implementation and even a name,
their intention is clear: they need this functionality, but the core
headers lack it, so: I'll make another small copy in my small driver,
and nobody cares.

I think your consolidation should at first target the above users.

Those having different names or substantially different implementation,
may also be a target. But they are:
 1. Obviously a minority in terms of LOCs, and
 2. More likely have their reasons to have custom flavors of the same.

...

> > Can you please prepare a series that introduces the new macro and
> > wires all arch duplications to it?
> 
> Okay, I will do that after I learn Coccinelle. Though I suspect the reason
> why I'm the first person to address this is because it's much easier to
> hide duplicated macros away in drivers than go the long route of fixing up
> every single other user. I'm not too miffed about it though, it's cleanup
> of technical debt that's long overdue.
 
I just fired 

        $ git grep "define HIWORD"

and found 27 matches. The relevant 'hiwords' have the following
semantics:

 - HIWORD_UPDATE(val, mask, shift)
 - HIWORD_UPDATE(val, mask)
 - HIWORD_UPDATE(mask, val)
 - HIWORD_UPDATE(v, h, l)
 - HIWORD_UPDATE_BIT(val)
 - HIWORD_DISABLE_BIT(val)

Most of them don't bother doing any static checks at all.

If you will just consolidate the above, and wire those drivers
to generic version with all that checks - it would be a decent
consolidation by any measure.

Something like this:

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 0470d7c175f4..d5e74d555a3d 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -30,7 +30,7 @@

 #define DMC_MAX_CHANNELS       4

-#define HIWORD_UPDATE(val, mask)       ((val) | (mask) << 16)
+#define HIWORD_UPDATE(val, mask)       HWORD_UPDATE(mask, val)

 /* DDRMON_CTRL */
 #define DDRMON_CTRL    0x04

...

> > Regarding the name... I can't invent a good one as well, so the best
> > thing I can suggest is not to invent something that can mislead. The
> > HWM_FIELD_PREP() is not bad because it tells almost nothing and
> > encourages one to refer to the documentation. If you want something
> > self-explaining, maybe MASK_HI_FIELD_LO_PREP_U16(), or something?
> 
> This seems a bit unwieldy, at 25 characters. "FIELD32_HIMASK_LOPREP"
> (or FIELD16, depending on which end of the cornet to eat) would be 21
> characters but I'm also not in love with it.
> 
> I think the name should include the following parts:
> 1. it's a field
> 2. the field is halved into two halves of 16 bits
> 3. the mask is copied into the upper 16 bits

Or just keep the HIWORD_UPDATE name as it already has over 300 users.
If it's commented well, and has an implementation based on FIELD_PREP,
I don't think users will struggle to understand what is actually
happening there.
 
> Since we're on the subject of bit widths, I have a somewhat sacrilegious
> point to raise: should this be a function-like macro at all, as opposed
> to a static __pure inline function? It's not generic with regards to the
> data types, as we're always assuming a u16 value and mask input and a
> u32 output. The __pure inline definition should let the compiler treat it
> essentially similar to what the pre-processor expanded macro does, which
> is as not a function call at all but a bunch of code to constant fold away
> if possible. What we get in return is type checking and less awful syntax.
> Then we could call it something like `himask_field_prep_u32`, which is
> also 21 characters but the ambiguity of whether the u32 refers to the mask
> or the whole register width is cleared up by the types of the function
> arguments.
> 
> The const version of the macro may still need to remain though because I'm
> not sure C11 can do that for us. With C23 maybe there's a way with
> constexpr but I've never used it before.

Inline function will disable parameters compile checks, and will be
too diverged from _CONST counterpart.

Thanks,
Yury