Re: [PATCH 3/7] soc: rockchip: add utils header for things shared across drivers

["Heiko Stübner" <heiko@sntech.de>]

Hi,

Am Dienstag, 8. April 2025, 14:32:15 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> Rockchip hardware has some functionality that is shared across many
> hardware IPs, and therefore many drivers for them.
> 
> Most notably is "HIWORD_UPDATE", a macro with slightly different
> semantics replicated across many a rockchip driver. It currently can be
> found defined in 19 files, of which 18 are Rockchip drivers.
> 
> Instead of continuing this tradition with yet another version of it in
> my new drivers, add a rockchip soc header for utility macros and such.
> In this header, we define a new set of macros: REG_UPDATE_WE and its
> little brother REG_UPDATE_BIT_WE. These are purposefully named something
> other than "HIWORD_UPDATE", to reduce the likelihood of macro
> redefinitions and also reduce the potential to mislead any adopter into
> thinking this HIWORD_UPDATE is just like their HIWORD_UPDATE.
> 
> Old drivers can be moved over to the new macros over the next while if
> their maintainers think it makes sense for them, which it probably does.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

when you're doing these fancy nice new macros, I think they might want to
be even more centrally located for _everyone_ :-) .


Because while true, Rockchip seems to be the biggest user of hiword-mask-
registers, they're not the only one.

Just simply grepping for HIWORD in kernel drivers revealed
- Sunplus sp7021 clock and reset drivers [0]
- a number of hisilicon clock drivers [1]
- some other clock drivers

and as the naming is not really standarized, I guess there will be more
of the same thing under different names in other places.

Similarly, we already have a FIELD_PREP_HIWORD in [2], so all in all
I think all of this wants to move in with the other bitfield stuff like
FIELD_PREP.


Heiko


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-sp7021.c#n42
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/reset-sunplus.c
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/hisilicon/clk-hi3620.c
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/hisilicon/clk-hi3660.c
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/hisilicon/clk-hi3670.c
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/hisilicon/clk-hi6220.c
[2] https://elixir.bootlin.com/linux/v6.15/source/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c#L23

> ---
>  include/soc/rockchip/utils.h | 76 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/include/soc/rockchip/utils.h b/include/soc/rockchip/utils.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..3349069e75ff51ebd7a22089af796feafd227ffb
> --- /dev/null
> +++ b/include/soc/rockchip/utils.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (c) 2025 Collabora Ltd.
> + *
> + * Utility types, inline functions, and macros that are used across several
> + * Rockchip-specific drivers.
> + *
> + * Authors:
> + *     Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> + */
> +
> +#ifndef __SOC_ROCKCHIP_UTILS_H__
> +#define __SOC_ROCKCHIP_UTILS_H__
> +
> +#include <linux/bits.h>
> +#include <linux/build_bug.h>
> +#include <linux/limits.h>
> +
> +/*
> + * Incoming macro basilisks, stare directly at them at your own peril.
> + * As a gentle reminder to help with code comprehension: BUILD_BUG_ON_ZERO
> + * is confusingly named; it's a version of BUILD_BUG_ON that evaluates to zero
> + * if it does not trigger, i.e. the assertion within the macro still checks
> + * for a truthy value, not zero.
> + */
> +
> +/**
> + * REG_UPDATE_WE - generate a register write value with a write-enable mask
> + * @_val: unshifted value we wish to update between @_low and @_high
> + * @_low: index of the low bit of the bit range we want to update
> + * @_high: index of the high bit of the bit range we want to update
> + *
> + * This macro statically generates a value consisting of @_val shifted to the
> + * left by @_low, and a write-enable mask in the upper 16 bits of the value
> + * that sets bit ``i << 16`` to ``1`` if bit ``i`` is within the @_low to @_high
> + * range. Only up to bit (@_high - @_low) of @_val is used for safety, i.e.
> + * trying to write a value that doesn't fit in the specified range will simply
> + * truncate it.
> + *
> + * This is useful for some hardware, like some of Rockchip's registers, where
> + * a 32-bit width register is divided into a value low half, and a write enable
> + * high half. Bits in the low half are only update if the corresponding bit in
> + * the high half is ``1``, allowing for lock-free atomic updates of a register.
> + *
> + * This macro replaces the venerable ``HIWORD_UPDATE``, which is copied and
> + * pasted in slightly different forms across many different Rockchip drivers.
> + * Before switching drivers to use it, familiarise yourself with the semantics
> + * of your specific ``HIWORD_UPDATE`` compared to this function-like macro's
> + * semantics.
> + *
> + * Return: the value, shifted into place, with the required write-enable bits
> + */
> +#define REG_UPDATE_WE(_val, _low, _high) ( \
> +	BUILD_BUG_ON_ZERO(const_true((_low) > (_high))) + \
> +	BUILD_BUG_ON_ZERO(const_true((_high) > 15)) + \
> +	BUILD_BUG_ON_ZERO(const_true((_low) < 0)) + \
> +	BUILD_BUG_ON_ZERO(const_true((u64) (_val) > U16_MAX)) + \
> +	((_val & GENMASK((_high) - (_low), 0)) << (_low) | \
> +	(GENMASK((_high), (_low)) << 16)))
> +
> +/**
> + * REG_UPDATE_BIT_WE - update a bit with a write-enable mask
> + * @__val: new value of the bit, either ``0`` 0r ``1``
> + * @__bit: bit index to modify, 0 <= @__bit < 16.
> + *
> + * This is like REG_UPDATE_WE() but only modifies a single bit, thereby making
> + * invocation easier by avoiding having to pass a repeated value.
> + *
> + * Return: a value with bit @__bit set to @__val and @__bit << 16 set to ``1``
> + */
> +#define REG_UPDATE_BIT_WE(__val, __bit) ( \
> +	BUILD_BUG_ON_ZERO(const_true((__val) > 1)) + \
> +	BUILD_BUG_ON_ZERO(const_true((__val) < 0)) + \
> +	REG_UPDATE_WE((__val), (__bit), (__bit)))
> +
> +#endif /* __SOC_ROCKCHIP_UTILS_H__ */
> 
>