Re: [PATCH v2 1/4] clk: move core flags into a new enum for kernel docs

[Brian Masney <bmasney@redhat.com>]

Hi Stephen,

On Wed, Mar 25, 2026 at 07:52:10PM -0400, Brian Masney wrote:
> Let's move all of the existing clk flags into a new enum so that all of
> the flags can be easily referenced in the kernel documentation. Note
> that I went with name clk_core_flags for the enum since the name
> clk_flags is already in use in clk.c for the debugfs interface.
> 
> Note: The comment about "Please update clk_flags..." is included as a
> separate comment so it doesn't show up in the generated documents.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  include/linux/clk-provider.h | 55 +++++++++++++++++++++++++++-----------------
>  1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 630705a47129453c241f1b1755f2c2f2a7ed8f77..cb167c17c4f79cf438a26bb113b4968d0f223468 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -9,29 +9,42 @@
>  #include <linux/of.h>
>  #include <linux/of_clk.h>
>  
> -/*
> - * flags used across common struct clk.  these flags should only affect the
> - * top-level framework.  custom flags for dealing with hardware specifics
> - * belong in struct clk_foo
> +/* Please update clk_flags[] in drivers/clk/clk.c when making changes here! */
> +/**
> + * enum clk_core_flags - framework-level clock flags
>   *
> - * Please update clk_flags[] in drivers/clk/clk.c when making changes here!
> + * These flags should only affect the top-level framework. Custom flags for
> + * dealing with hardware specifics belong in struct clk_foo.
> + *
> + * @CLK_SET_RATE_GATE: must be gated across rate change
> + * @CLK_SET_PARENT_GATE: must be gated across re-parent
> + * @CLK_SET_RATE_PARENT: propagate rate change up one level
> + * @CLK_IGNORE_UNUSED: do not gate even if unused
> + * @CLK_GET_RATE_NOCACHE: do not use the cached clk rate
> + * @CLK_SET_RATE_NO_REPARENT: don't re-parent on rate change
> + * @CLK_GET_ACCURACY_NOCACHE: do not use the cached clk accuracy
> + * @CLK_RECALC_NEW_RATES: recalc rates after notifications
> + * @CLK_SET_RATE_UNGATE: clock needs to run to set rate
> + * @CLK_IS_CRITICAL: do not gate, ever
> + * @CLK_OPS_PARENT_ENABLE: parents need enable during gate/ungate, set rate and re-parent
> + * @CLK_DUTY_CYCLE_PARENT: duty cycle call may be forwarded to the parent clock
>   */
> -#define CLK_SET_RATE_GATE	BIT(0) /* must be gated across rate change */
> -#define CLK_SET_PARENT_GATE	BIT(1) /* must be gated across re-parent */
> -#define CLK_SET_RATE_PARENT	BIT(2) /* propagate rate change up one level */
> -#define CLK_IGNORE_UNUSED	BIT(3) /* do not gate even if unused */
> -				/* unused */
> -				/* unused */
> -#define CLK_GET_RATE_NOCACHE	BIT(6) /* do not use the cached clk rate */
> -#define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */
> -#define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
> -#define CLK_RECALC_NEW_RATES	BIT(9) /* recalc rates after notifications */
> -#define CLK_SET_RATE_UNGATE	BIT(10) /* clock needs to run to set rate */
> -#define CLK_IS_CRITICAL		BIT(11) /* do not gate, ever */
> -/* parents need enable during gate/ungate, set rate and re-parent */
> -#define CLK_OPS_PARENT_ENABLE	BIT(12)
> -/* duty cycle call may be forwarded to the parent clock */
> -#define CLK_DUTY_CYCLE_PARENT	BIT(13)
> +enum clk_core_flags {
> +	CLK_SET_RATE_GATE		= BIT(0),
> +	CLK_SET_PARENT_GATE		= BIT(1),
> +	CLK_SET_RATE_PARENT		= BIT(2),
> +	CLK_IGNORE_UNUSED		= BIT(3),
> +	/* unused */
> +	/* unused */
> +	CLK_GET_RATE_NOCACHE		= BIT(6),
> +	CLK_SET_RATE_NO_REPARENT	= BIT(7),
> +	CLK_GET_ACCURACY_NOCACHE	= BIT(8),
> +	CLK_RECALC_NEW_RATES		= BIT(9),
> +	CLK_SET_RATE_UNGATE		= BIT(10),
> +	CLK_IS_CRITICAL			= BIT(11),
> +	CLK_OPS_PARENT_ENABLE		= BIT(12),
> +	CLK_DUTY_CYCLE_PARENT		= BIT(13),
> +};

I just checked Sashiko [1] for this series and it has this comment:

    Could converting these unsigned long bitmasks to an enum create a silent
    type-safety trap if flags ever reach BIT(31)?
    
    The flags fields in the clock framework are explicitly designed to be 64-bit
    on 64-bit architectures (unsigned long). The BIT() macro evaluates to an
    unsigned long. When defined as macros, operations like clearing flags
    produce a 64-bit inverted mask, perfectly preserving the upper 32 bits.
    
    By moving these flags into an enum, their types are implicitly downgraded to
    int, since the current maximum flag BIT(13) fits in a 32-bit signed integer.
    If the flags ever grow to include BIT(31), the enumerator value will overflow
    a signed 32-bit int, causing the compiler to type it as an unsigned int.
    
    Applying a bitwise NOT to this unsigned int will produce an unsigned int.
    When this is bitwise ANDed with the 64-bit unsigned long flags variable,
    the unsigned mask will zero-extend to 64 bits, silently clearing all upper
    32 bits (bits 32-63) of the flags field.
    
    Would it be safer to keep them as #define macros and use a DOC: block
    to properly document the flags using kernel-doc without breaking type safety?

It's up to you how we should proceed with this.

[1] https://sashiko.dev/#/patchset/20260325-clk-docs-v2-0-bcf660e1ceb5%40redhat.com

Brian