Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ccu_maskdiv_determine_rate ignores the CLK_SET_RATE_PARENT flag, breaking rate propagation.
- [Medium] ccu_maskdiv_set_rate fails to apply CCU_FEATURE_UPDATE_BIT and CCU_FEATURE_KEY_FIELD, causing inconsistent state latching.
--
--- Patch [1]: [PATCH v1 1/4] clk: sunxi-ng: add cycle-masking divider (maskdiv) clock type ---
commit 07eec9d5dafd1fbf248e0e56bb3c42f8da46589d
Author: Juan Manuel López Carrillo <juanmanuellopezcarrillo@gmail.com>
clk: sunxi-ng: add cycle-masking divider (maskdiv) clock type
This commit introduces a cycle-masking divider clock type for sunxi-ng.
Instead of a linear divider, it masks M pulses out of every 2^width parent
cycles, which is used for hardware like the A523/T527 GPU clock.
> diff --git a/drivers/clk/sunxi-ng/ccu_maskdiv.c b/drivers/clk/sunxi-ng/ccu_maskdiv.c
> new file mode 100644
> index 0000000000000..2d3cccf081493
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu_maskdiv.c
> @@ -0,0 +1,199 @@
[ ... ]
> +static int ccu_maskdiv_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
> + unsigned long best_rate = 0, best_parent_rate = 0;
> + struct clk_hw *best_parent = NULL;
> + unsigned int best_m = UINT_MAX;
> + unsigned int i;
> +
> + for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
> + struct clk_hw *parent = clk_hw_get_parent_by_index(hw, i);
> + unsigned long parent_rate, new_rate;
> + unsigned int m;
> +
> + if (!parent)
> + continue;
> +
> + parent_rate = clk_hw_get_rate(parent);
> + m = ccu_maskdiv_find_m(parent_rate, req->rate, cmd->width);
> + new_rate = ccu_maskdiv_calc_rate(parent_rate, m, cmd->width);
[Severity: High]
Does this implementation ignore the CLK_SET_RATE_PARENT flag?
By using clk_hw_get_rate(parent) instead of clk_hw_round_rate(), this loop
only evaluates parents at their currently locked rates. It appears this
prevents the framework from adjusting the parent's rate (such as the
underlying pll-gpu).
Could this silently break Dynamic Voltage and Frequency Scaling (DVFS) by
locking the GPU to a masked fraction of the PLL's fixed boot rate?
> +
> + if (new_rate > req->rate)
> + continue;
[ ... ]
> +static int ccu_maskdiv_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
> + unsigned int m;
> + unsigned long flags;
> + u32 reg;
> +
> + m = ccu_maskdiv_find_m(parent_rate, rate, cmd->width);
> +
> + spin_lock_irqsave(cmd->common.lock, flags);
> +
> + reg = readl(cmd->common.base + cmd->common.reg);
> + reg &= ~GENMASK(cmd->shift + cmd->width - 1, cmd->shift);
> + writel(reg | (m << cmd->shift), cmd->common.base + cmd->common.reg);
[Severity: Medium]
Will hardware register updates fail to latch here if the clock requires the
CCU_SUNXI_UPDATE_BIT or CCU_FEATURE_KEY_FIELD?
The standard ccu_common framework relies on these feature bits to
synchronize hardware changes, which ccu_mux_helper_set_parent() applies
automatically.
Since this direct write ignores cmd->common.features, could rate changes
silently fail to apply for clocks that require the update bit?
> +
> + spin_unlock_irqrestore(cmd->common.lock, flags);
> +
> + return 0;
> +}
@@ -10,6 +10,7 @@ sunxi-ccu-y += ccu_reset.o
# Base clock types
sunxi-ccu-y += ccu_div.o
sunxi-ccu-y += ccu_frac.o
+sunxi-ccu-y += ccu_maskdiv.o
sunxi-ccu-y += ccu_gate.o
sunxi-ccu-y += ccu_mux.o
sunxi-ccu-y += ccu_mult.o
new file mode 100644
@@ -0,0 +1,199 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2026 Juan Manuel López Carrillo
+ *
+ * Cycle-masking divider: the M factor masks M pulses out of every
+ * 2^width parent cycles instead of dividing the parent rate, so
+ *
+ * rate = parent * (2^width - M) / 2^width
+ *
+ * The masked output is not an even pulse train: the surviving pulses
+ * keep the parent period. Rate selection therefore prefers, among the
+ * parents that reach the requested rate, the one needing the least
+ * masking.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/math64.h>
+
+#include "ccu_gate.h"
+#include "ccu_maskdiv.h"
+
+static unsigned long ccu_maskdiv_calc_rate(unsigned long parent_rate,
+ unsigned int m, unsigned int width)
+{
+ unsigned int n = 1 << width;
+
+ return div_u64((u64)parent_rate * (n - m), n);
+}
+
+/*
+ * Smallest M (least masking) whose output does not exceed the requested
+ * rate; masking everything (M == 2^width) is never returned.
+ */
+static unsigned int ccu_maskdiv_find_m(unsigned long parent_rate,
+ unsigned long rate, unsigned int width)
+{
+ unsigned int n = 1 << width;
+ u64 kept;
+
+ if (!parent_rate || rate >= parent_rate)
+ return 0;
+
+ kept = div64_ul((u64)rate * n, parent_rate);
+ if (!kept)
+ kept = 1;
+
+ return n - (unsigned int)kept;
+}
+
+static void ccu_maskdiv_disable(struct clk_hw *hw)
+{
+ struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
+
+ return ccu_gate_helper_disable(&cmd->common, cmd->enable);
+}
+
+static int ccu_maskdiv_enable(struct clk_hw *hw)
+{
+ struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
+
+ return ccu_gate_helper_enable(&cmd->common, cmd->enable);
+}
+
+static int ccu_maskdiv_is_enabled(struct clk_hw *hw)
+{
+ struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
+
+ return ccu_gate_helper_is_enabled(&cmd->common, cmd->enable);
+}
+
+static unsigned long ccu_maskdiv_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
+ unsigned int m;
+ u32 reg;
+
+ reg = readl(cmd->common.base + cmd->common.reg);
+ m = (reg >> cmd->shift) & ((1 << cmd->width) - 1);
+
+ return ccu_maskdiv_calc_rate(parent_rate, m, cmd->width);
+}
+
+static int ccu_maskdiv_determine_rate(struct clk_hw *hw,
+ struct clk_rate_request *req)
+{
+ struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
+ unsigned long best_rate = 0, best_parent_rate = 0;
+ struct clk_hw *best_parent = NULL;
+ unsigned int best_m = UINT_MAX;
+ unsigned int i;
+
+ for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
+ struct clk_hw *parent = clk_hw_get_parent_by_index(hw, i);
+ unsigned long parent_rate, new_rate;
+ unsigned int m;
+
+ if (!parent)
+ continue;
+
+ parent_rate = clk_hw_get_rate(parent);
+ m = ccu_maskdiv_find_m(parent_rate, req->rate, cmd->width);
+ new_rate = ccu_maskdiv_calc_rate(parent_rate, m, cmd->width);
+
+ if (new_rate > req->rate)
+ continue;
+
+ /* Closest rate first; on ties, the least masking */
+ if (new_rate > best_rate ||
+ (new_rate == best_rate && m < best_m)) {
+ best_rate = new_rate;
+ best_parent_rate = parent_rate;
+ best_parent = parent;
+ best_m = m;
+ }
+ }
+
+ if (!best_parent)
+ return -EINVAL;
+
+ req->best_parent_hw = best_parent;
+ req->best_parent_rate = best_parent_rate;
+ req->rate = best_rate;
+
+ return 0;
+}
+
+static int ccu_maskdiv_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
+ unsigned int m;
+ unsigned long flags;
+ u32 reg;
+
+ m = ccu_maskdiv_find_m(parent_rate, rate, cmd->width);
+
+ spin_lock_irqsave(cmd->common.lock, flags);
+
+ reg = readl(cmd->common.base + cmd->common.reg);
+ reg &= ~GENMASK(cmd->shift + cmd->width - 1, cmd->shift);
+ writel(reg | (m << cmd->shift), cmd->common.base + cmd->common.reg);
+
+ spin_unlock_irqrestore(cmd->common.lock, flags);
+
+ return 0;
+}
+
+static u8 ccu_maskdiv_get_parent(struct clk_hw *hw)
+{
+ struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
+
+ return ccu_mux_helper_get_parent(&cmd->common, &cmd->mux);
+}
+
+static int ccu_maskdiv_set_parent(struct clk_hw *hw, u8 index)
+{
+ struct ccu_maskdiv *cmd = hw_to_ccu_maskdiv(hw);
+
+ return ccu_mux_helper_set_parent(&cmd->common, &cmd->mux, index);
+}
+
+static int ccu_maskdiv_set_rate_and_parent(struct clk_hw *hw,
+ unsigned long rate,
+ unsigned long parent_rate, u8 index)
+{
+ /*
+ * Same ordering rule as clk_composite_set_rate_and_parent(): if
+ * switching the mux with the current M would overshoot the
+ * requested rate, program the divider first, so the
+ * intermediate rate never exceeds both the old and the new
+ * rate.
+ */
+ if (ccu_maskdiv_recalc_rate(hw, parent_rate) > rate) {
+ ccu_maskdiv_set_rate(hw, rate, parent_rate);
+ ccu_maskdiv_set_parent(hw, index);
+ } else {
+ ccu_maskdiv_set_parent(hw, index);
+ ccu_maskdiv_set_rate(hw, rate, parent_rate);
+ }
+
+ return 0;
+}
+
+const struct clk_ops ccu_maskdiv_ops = {
+ .disable = ccu_maskdiv_disable,
+ .enable = ccu_maskdiv_enable,
+ .is_enabled = ccu_maskdiv_is_enabled,
+
+ .get_parent = ccu_maskdiv_get_parent,
+ .set_parent = ccu_maskdiv_set_parent,
+
+ .determine_rate = ccu_maskdiv_determine_rate,
+ .recalc_rate = ccu_maskdiv_recalc_rate,
+ .set_rate = ccu_maskdiv_set_rate,
+ .set_rate_and_parent = ccu_maskdiv_set_rate_and_parent,
+};
+EXPORT_SYMBOL_NS_GPL(ccu_maskdiv_ops, "SUNXI_CCU");
new file mode 100644
@@ -0,0 +1,71 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2026 Juan Manuel López Carrillo
+ */
+
+#ifndef _CCU_MASKDIV_H_
+#define _CCU_MASKDIV_H_
+
+#include <linux/clk-provider.h>
+
+#include "ccu_common.h"
+#include "ccu_mux.h"
+
+/*
+ * struct ccu_maskdiv - cycle-masking ("fractional") divider
+ *
+ * This divider does not divide the parent clock: it masks (swallows) M
+ * pulses out of every 2^width parent cycles, so the average output rate
+ * is
+ *
+ * rate = parent * (2^width - M) / 2^width
+ *
+ * with the remaining pulses keeping the parent period. The A523/T527
+ * GPU clock (GPU_CLK_REG, 0x670) is such a divider: "FACTOR_M: mask M
+ * cycles at 16 cycles", GPU_CLK = Clock Source * ((16-M)/16) (T527 user
+ * manual v0.92, section 2.7.6.58).
+ *
+ * @shift: shift of the M field in the register
+ * @width: width of the M field; the mask window is 2^width cycles
+ */
+struct ccu_maskdiv {
+ u32 enable;
+
+ u8 shift;
+ u8 width;
+
+ struct ccu_mux_internal mux;
+ struct ccu_common common;
+};
+
+#define SUNXI_CCU_MASKDIV_HW_WITH_MUX_TABLE_GATE(_struct, _name, \
+ _parents, _table, \
+ _reg, \
+ _mshift, _mwidth, \
+ _muxshift, _muxwidth, \
+ _gate, _flags) \
+ struct ccu_maskdiv _struct = { \
+ .enable = _gate, \
+ .shift = _mshift, \
+ .width = _mwidth, \
+ .mux = _SUNXI_CCU_MUX_TABLE(_muxshift, _muxwidth, \
+ _table), \
+ .common = { \
+ .reg = _reg, \
+ .hw.init = CLK_HW_INIT_PARENTS_HW(_name, \
+ _parents, \
+ &ccu_maskdiv_ops, \
+ _flags), \
+ }, \
+ }
+
+static inline struct ccu_maskdiv *hw_to_ccu_maskdiv(struct clk_hw *hw)
+{
+ struct ccu_common *common = hw_to_ccu_common(hw);
+
+ return container_of(common, struct ccu_maskdiv, common);
+}
+
+extern const struct clk_ops ccu_maskdiv_ops;
+
+#endif /* _CCU_MASKDIV_H_ */