Re: [PATCH 5/6] clk: rockchip: rk3506: Use FIELD_PREP_WM16 macro

Wed, 12 Aug 2026 06:02:01 -0700 

Hi Jonas,

On 8/3/26 9:09 PM, Jonas Karlman wrote:

Change to use writel() together with the FIELD_PREP_WM16() macro
instead of using the rk_clrsetreg() macro to avoid having to define the
mask as a parameter to both rk_clrsetreg() and FIELD_PREP(). Also change
to use u32 variables consistently.

No change in behavior is expected due to this code style change.

Signed-off-by: Jonas Karlman <[email protected]>
---
  drivers/clk/rockchip/clk_rk3506.c | 195 ++++++++++++++----------------
  1 file changed, 90 insertions(+), 105 deletions(-)

diff --git a/drivers/clk/rockchip/clk_rk3506.c 
b/drivers/clk/rockchip/clk_rk3506.c
index e156bf19a6b4..7ccab313e6ec 100644
--- a/drivers/clk/rockchip/clk_rk3506.c
+++ b/drivers/clk/rockchip/clk_rk3506.c
@@ -9,10 +9,11 @@
  #include <clk-uclass.h>
  #include <asm/arch-rockchip/clock.h>
  #include <asm/arch-rockchip/cru_rk3506.h>
-#include <asm/arch-rockchip/hardware.h>
+#include <asm/io.h>
  #include <dm/device-internal.h>
  #include <dm/lists.h>
  #include <dt-bindings/clock/rockchip,rk3506-cru.h>
+#include <linux/hw_bitfield.h>
#define DIV_TO_RATE(input_rate, div) ((input_rate) / ((div) + 1)) @@ -122,10 +123,10 @@ static int rk3506_armclk_set_rate(struct rk3506_clk_priv *priv, ulong new_rate) 
         */
        old_rate = rk3506_armclk_get_rate(priv);
        if (new_rate >= old_rate) {
-               rk_clrsetreg(RK3506_CLKSEL_CON(15), ACLK_CORE_DIV_MASK,
-                            FIELD_PREP(ACLK_CORE_DIV_MASK, rate->aclk_div));
-               rk_clrsetreg(RK3506_CLKSEL_CON(16), PCLK_CORE_DIV_MASK,
-                            FIELD_PREP(PCLK_CORE_DIV_MASK, rate->pclk_div));
+               writel(FIELD_PREP_WM16(ACLK_CORE_DIV_MASK, rate->aclk_div),
+                      RK3506_CLKSEL_CON(15));
+               writel(FIELD_PREP_WM16(PCLK_CORE_DIV_MASK, rate->pclk_div),
+                      RK3506_CLKSEL_CON(16));
        }
if (new_rate == 589824000 || new_rate == 1179648000) { 
@@ -146,22 +147,22 @@ static int rk3506_armclk_set_rate(struct rk3506_clk_priv 
*priv, ulong new_rate)
        con = readl(RK3506_CLKSEL_CON(15));
        old_div = FIELD_GET(CLK_CORE_SRC_DIV_MASK, con);
        if (DIV_TO_RATE(prate, old_div) > new_rate) {
-               rk_clrsetreg(RK3506_CLKSEL_CON(15), CLK_CORE_SRC_DIV_MASK,
-                            FIELD_PREP(CLK_CORE_SRC_DIV_MASK, div - 1));
-               rk_clrsetreg(RK3506_CLKSEL_CON(15), CLK_CORE_SRC_SEL_MASK,
-                            FIELD_PREP(CLK_CORE_SRC_SEL_MASK, sel));
+               writel(FIELD_PREP_WM16(CLK_CORE_SRC_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(15));
+               writel(FIELD_PREP_WM16(CLK_CORE_SRC_SEL_MASK, sel),
+                      RK3506_CLKSEL_CON(15));
        } else {
-               rk_clrsetreg(RK3506_CLKSEL_CON(15), CLK_CORE_SRC_SEL_MASK,
-                            FIELD_PREP(CLK_CORE_SRC_SEL_MASK, sel));
-               rk_clrsetreg(RK3506_CLKSEL_CON(15), CLK_CORE_SRC_DIV_MASK,
-                            FIELD_PREP(CLK_CORE_SRC_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_CORE_SRC_SEL_MASK, sel),
+                      RK3506_CLKSEL_CON(15));
+               writel(FIELD_PREP_WM16(CLK_CORE_SRC_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(15));
        }
if (new_rate < old_rate) { 
-               rk_clrsetreg(RK3506_CLKSEL_CON(15), ACLK_CORE_DIV_MASK,
-                            FIELD_PREP(ACLK_CORE_DIV_MASK, rate->aclk_div));
-               rk_clrsetreg(RK3506_CLKSEL_CON(16), PCLK_CORE_DIV_MASK,
-                            FIELD_PREP(PCLK_CORE_DIV_MASK, rate->pclk_div));
+               writel(FIELD_PREP_WM16(ACLK_CORE_DIV_MASK, rate->aclk_div),
+                      RK3506_CLKSEL_CON(15));
+               writel(FIELD_PREP_WM16(PCLK_CORE_DIV_MASK, rate->pclk_div),
+                      RK3506_CLKSEL_CON(16));
        }
return rk3506_armclk_get_rate(priv); 
@@ -209,26 +210,26 @@ static ulong rk3506_pll_div_set_rate(struct 
rk3506_clk_priv *priv, ulong clk_id,
        case CLK_GPLL_DIV:
                div = DIV_ROUND_UP(priv->gpll_hz, rate);
                assert(div - 1 <= 15);
-               rk_clrsetreg(RK3506_CLKSEL_CON(0), CLK_GPLL_DIV_MASK,
-                            FIELD_PREP(CLK_GPLL_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_GPLL_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(0));
                break;
        case CLK_GPLL_DIV_100M:
                div = DIV_ROUND_UP(priv->gpll_div_hz, rate);
                assert(div - 1 <= 15);
-               rk_clrsetreg(RK3506_CLKSEL_CON(0), CLK_GPLL_DIV_100M_MASK,
-                            FIELD_PREP(CLK_GPLL_DIV_100M_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_GPLL_DIV_100M_MASK, div - 1),
+                      RK3506_CLKSEL_CON(0));
                break;
        case CLK_V0PLL_DIV:
                div = DIV_ROUND_UP(priv->v0pll_hz, rate);
                assert(div - 1 <= 15);
-               rk_clrsetreg(RK3506_CLKSEL_CON(1), CLK_V0PLL_DIV_MASK,
-                            FIELD_PREP(CLK_V0PLL_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_V0PLL_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(1));
                break;
        case CLK_V1PLL_DIV:
                div = DIV_ROUND_UP(priv->v1pll_hz, rate);
                assert(div - 1 <= 15);
-               rk_clrsetreg(RK3506_CLKSEL_CON(1), CLK_V1PLL_DIV_MASK,
-                            FIELD_PREP(CLK_V1PLL_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_V1PLL_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(1));
                break;
        default:
                return -ENOENT;
@@ -293,22 +294,19 @@ static ulong rk3506_bus_set_rate(struct rk3506_clk_priv 
*priv, ulong clk_id,
switch (clk_id) { 
        case ACLK_BUS_ROOT:
-               rk_clrsetreg(RK3506_CLKSEL_CON(21),
-                            ACLK_BUS_SEL_MASK | ACLK_BUS_DIV_MASK,
-                            FIELD_PREP(ACLK_BUS_SEL_MASK, sel) |
-                            FIELD_PREP(ACLK_BUS_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(ACLK_BUS_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(ACLK_BUS_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(21));
                break;
        case HCLK_BUS_ROOT:
-               rk_clrsetreg(RK3506_CLKSEL_CON(21),
-                            HCLK_BUS_SEL_MASK | HCLK_BUS_DIV_MASK,
-                            FIELD_PREP(HCLK_BUS_SEL_MASK, sel) |
-                            FIELD_PREP(HCLK_BUS_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(HCLK_BUS_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(HCLK_BUS_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(21));
                break;
        case PCLK_BUS_ROOT:
-               rk_clrsetreg(RK3506_CLKSEL_CON(22),
-                            PCLK_BUS_SEL_MASK | PCLK_BUS_DIV_MASK,
-                            FIELD_PREP(PCLK_BUS_SEL_MASK, sel) |
-                            FIELD_PREP(PCLK_BUS_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(PCLK_BUS_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(PCLK_BUS_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(22));
                break;
        default:
                return -ENOENT;
@@ -368,16 +366,14 @@ static ulong rk3506_peri_set_rate(struct rk3506_clk_priv 
*priv, ulong clk_id,
switch (clk_id) { 
        case ACLK_HSPERI_ROOT:
-               rk_clrsetreg(RK3506_CLKSEL_CON(49),
-                            ACLK_HSPERI_SEL_MASK | ACLK_HSPERI_DIV_MASK,
-                            FIELD_PREP(ACLK_HSPERI_SEL_MASK, sel) |
-                            FIELD_PREP(ACLK_HSPERI_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(ACLK_HSPERI_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(ACLK_HSPERI_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(49));
                break;
        case HCLK_LSPERI_ROOT:
-               rk_clrsetreg(RK3506_CLKSEL_CON(29),
-                            HCLK_LSPERI_SEL_MASK | HCLK_LSPERI_DIV_MASK,
-                            FIELD_PREP(HCLK_LSPERI_SEL_MASK, sel) |
-                            FIELD_PREP(HCLK_LSPERI_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(HCLK_LSPERI_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(HCLK_LSPERI_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(29));
                break;
        default:
                return -ENOENT;
@@ -429,10 +425,9 @@ static ulong rk3506_sdmmc_set_rate(struct rk3506_clk_priv 
*priv, ulong clk_id,
        }
        assert(div - 1 <= 63);
- rk_clrsetreg(RK3506_CLKSEL_CON(49), 
-                    CCLK_SDMMC_SEL_MASK | CCLK_SDMMC_DIV_MASK,
-                    FIELD_PREP(CCLK_SDMMC_SEL_MASK, sel) |
-                    FIELD_PREP(CCLK_SDMMC_DIV_MASK, div - 1));
+       writel(FIELD_PREP_WM16(CCLK_SDMMC_SEL_MASK, sel) |
+              FIELD_PREP_WM16(CCLK_SDMMC_DIV_MASK, div - 1),
+              RK3506_CLKSEL_CON(49));
return rk3506_sdmmc_get_rate(priv, clk_id); 
  }
@@ -472,10 +467,9 @@ static ulong rk3506_saradc_set_rate(struct rk3506_clk_priv 
*priv, ulong clk_id,
        }
        assert(div - 1 <= 15);
- rk_clrsetreg(RK3506_CLKSEL_CON(54), 
-                    CLK_SARADC_SEL_MASK | CLK_SARADC_DIV_MASK,
-                    FIELD_PREP(CLK_SARADC_SEL_MASK, sel) |
-                    FIELD_PREP(CLK_SARADC_DIV_MASK, div - 1));
+       writel(FIELD_PREP_WM16(CLK_SARADC_SEL_MASK, sel) |
+              FIELD_PREP_WM16(CLK_SARADC_DIV_MASK, div - 1),
+              RK3506_CLKSEL_CON(54));
return rk3506_saradc_get_rate(priv, clk_id); 
  }
@@ -485,6 +479,7 @@ static ulong rk3506_tsadc_get_rate(struct rk3506_clk_priv 
*priv, ulong clk_id)
        u32 con, div;
con = readl(RK3506_CLKSEL_CON(61)); 
+
        switch (clk_id) {
        case CLK_TSADC_TSEN:
                div = FIELD_GET(CLK_TSADC_TSEN_DIV_MASK, con);
@@ -508,14 +503,14 @@ static ulong rk3506_tsadc_set_rate(struct rk3506_clk_priv 
*priv, ulong clk_id,
        case CLK_TSADC_TSEN:
                div = DIV_ROUND_UP(OSC_HZ, rate);
                assert(div - 1 <= 7);
-               rk_clrsetreg(RK3506_CLKSEL_CON(61), CLK_TSADC_TSEN_DIV_MASK,
-                            FIELD_PREP(CLK_TSADC_TSEN_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_TSADC_TSEN_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(61));
                break;
        case CLK_TSADC:
                div = DIV_ROUND_UP(OSC_HZ, rate);
                assert(div - 1 <= 255);
-               rk_clrsetreg(RK3506_CLKSEL_CON(61), CLK_TSADC_DIV_MASK,
-                            FIELD_PREP(CLK_TSADC_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_TSADC_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(61));
                break;
        default:
                return -ENOENT;
@@ -580,22 +575,19 @@ static ulong rk3506_i2c_set_rate(struct rk3506_clk_priv 
*priv, ulong clk_id,
switch (clk_id) { 
        case CLK_I2C0:
-               rk_clrsetreg(RK3506_CLKSEL_CON(32),
-                            CLK_I2C0_SEL_MASK | CLK_I2C0_DIV_MASK,
-                            FIELD_PREP(CLK_I2C0_SEL_MASK, sel) |
-                            FIELD_PREP(CLK_I2C0_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_I2C0_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(CLK_I2C0_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(32));
                break;
        case CLK_I2C1:
-               rk_clrsetreg(RK3506_CLKSEL_CON(32),
-                            CLK_I2C1_SEL_MASK | CLK_I2C1_DIV_MASK,
-                            FIELD_PREP(CLK_I2C1_SEL_MASK, sel) |
-                            FIELD_PREP(CLK_I2C1_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_I2C1_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(CLK_I2C1_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(32));
                break;
        case CLK_I2C2:
-               rk_clrsetreg(RK3506_CLKSEL_CON(33),
-                            CLK_I2C2_SEL_MASK | CLK_I2C2_DIV_MASK,
-                            FIELD_PREP(CLK_I2C2_SEL_MASK, sel) |
-                            FIELD_PREP(CLK_I2C2_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_I2C2_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(CLK_I2C2_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(33));
                break;
        default:
                return -ENOENT;
@@ -644,8 +636,8 @@ static ulong rk3506_pwm_set_rate(struct rk3506_clk_priv 
*priv, ulong clk_id,
        case CLK_PWM0:
                div = DIV_ROUND_UP(priv->gpll_div_100mhz, rate);
                assert(div - 1 <= 15);
-               rk_clrsetreg(RK3506_PMU_CLKSEL_CON(0), CLK_PWM0_DIV_MASK,
-                            FIELD_PREP(CLK_PWM0_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_PWM0_DIV_MASK, div - 1),
+                      RK3506_PMU_CLKSEL_CON(0));
                break;
        case CLK_PWM1:
                if (priv->v0pll_hz % rate == 0) {
@@ -659,10 +651,9 @@ static ulong rk3506_pwm_set_rate(struct rk3506_clk_priv 
*priv, ulong clk_id,
                        div = DIV_ROUND_UP(priv->gpll_div_hz, rate);
                }
                assert(div - 1 <= 15);
-               rk_clrsetreg(RK3506_CLKSEL_CON(33),
-                            CLK_PWM1_SEL_MASK | CLK_PWM1_DIV_MASK,
-                            FIELD_PREP(CLK_PWM1_SEL_MASK, sel) |
-                            FIELD_PREP(CLK_PWM1_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_PWM1_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(CLK_PWM1_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(33));
                break;
        default:
                return -ENOENT;
@@ -727,16 +718,14 @@ static ulong rk3506_spi_set_rate(struct rk3506_clk_priv 
*priv, ulong clk_id,
switch (clk_id) { 
        case CLK_SPI0:
-               rk_clrsetreg(RK3506_CLKSEL_CON(34),
-                            CLK_SPI0_SEL_MASK | CLK_SPI0_DIV_MASK,
-                            FIELD_PREP(CLK_SPI0_SEL_MASK, sel) |
-                            FIELD_PREP(CLK_SPI0_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_SPI0_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(CLK_SPI0_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(34));
                break;
        case CLK_SPI1:
-               rk_clrsetreg(RK3506_CLKSEL_CON(34),
-                            CLK_SPI1_SEL_MASK | CLK_SPI1_DIV_MASK,
-                            FIELD_PREP(CLK_SPI1_SEL_MASK, sel) |
-                            FIELD_PREP(CLK_SPI1_DIV_MASK, div - 1));
+               writel(FIELD_PREP_WM16(CLK_SPI1_SEL_MASK, sel) |
+                      FIELD_PREP_WM16(CLK_SPI1_DIV_MASK, div - 1),
+                      RK3506_CLKSEL_CON(34));
                break;
        default:
                return -ENOENT;
@@ -770,7 +759,7 @@ static ulong rk3506_fspi_get_rate(struct rk3506_clk_priv 
*priv)
static ulong rk3506_fspi_set_rate(struct rk3506_clk_priv *priv, ulong rate) 
  {
-       int div, sel;
+       u32 div, sel;
if (OSC_HZ % rate == 0) { 
                sel = SCLK_FSPI_SEL_24M;
@@ -787,10 +776,9 @@ static ulong rk3506_fspi_set_rate(struct rk3506_clk_priv 
*priv, ulong rate)
        }
        assert(div - 1 <= 31);
If somehow div = 0, then this will result in an underflow. Because it's an unsigned type, it'll wrap around and start at U32_MAX so assert() should catch it. However, assert() is a noop when DEBUG constant isn't defined, so this actually does nothing.
We should switch those assert to something that actually does something otherwise we could still very well attempt to write a divider that's too big and have weird behaviors instead of outright fail when we know we cannot do something that's requested.
This here doesn't change the logic anyway so this can be done in a follow-up patch, thus: 

Reviewed-by: Quentin Schulz <[email protected]>

Thanks!
Quentin

Reply via email to