Re: [PATCH v9 -next 04/12] clk: rp1: Add support for clocks provided by RP1
From: Stephen Boyd <sboyd@kernel.org>
Date: 2025-05-07 20:01:26
Also in:
linux-clk, linux-devicetree, linux-gpio, linux-pci, lkml
Quoting Andrea della Porta (2025-04-22 11:53:13)
quoted hunk ↗ jump to hunk
diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c new file mode 100644 index 000000000000..6b0b76fc6977 --- /dev/null +++ b/drivers/clk/clk-rp1.c@@ -0,0 +1,1510 @@
[...]
+static u8 rp1_clock_get_parent(struct clk_hw *hw)
+{
+ struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+ struct rp1_clockman *clockman = clock->clockman;
+ const struct rp1_clock_data *data = clock->data;
+ u32 sel, ctrl;
+ u8 parent;
+
+ /* Sel is one-hot, so find the first bit set */
+ sel = clockman_read(clockman, data->sel_reg);
+ parent = ffs(sel) - 1;
+
+ /* sel == 0 implies the parent clock is not enabled yet. */
+ if (!sel) {
+ /* Read the clock src from the CTRL register instead */
+ ctrl = clockman_read(clockman, data->ctrl_reg);
+ parent = (ctrl & data->clk_src_mask) >> CLK_CTRL_SRC_SHIFT;
+ }
+
+ if (parent >= data->num_std_parents)
+ parent = AUX_SEL;
+
+ if (parent == AUX_SEL) {
+ /*
+ * Clock parent is an auxiliary source, so get the parent from
+ * the AUXSRC register field.
+ */
+ ctrl = clockman_read(clockman, data->ctrl_reg);
+ parent = FIELD_GET(CLK_CTRL_AUXSRC_MASK, ctrl);
+ parent += data->num_std_parents;
+ }
+
+ return parent;
+}
+
+static int rp1_clock_set_parent(struct clk_hw *hw, u8 index)
+{
+ struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+ struct rp1_clockman *clockman = clock->clockman;
+ const struct rp1_clock_data *data = clock->data;
+ u32 ctrl, sel;
+
+ spin_lock(&clockman->regs_lock);
+ ctrl = clockman_read(clockman, data->ctrl_reg);
+
+ if (index >= data->num_std_parents) {
+ /* This is an aux source request */
+ if (index >= data->num_std_parents + data->num_aux_parents) {
+ spin_unlock(&clockman->regs_lock);
+ return -EINVAL;
+ }
+
+ /* Select parent from aux list */
+ ctrl &= ~CLK_CTRL_AUXSRC_MASK;
+ ctrl |= FIELD_PREP(CLK_CTRL_AUXSRC_MASK, index - data->num_std_parents);
+ /* Set src to aux list */
+ ctrl &= ~data->clk_src_mask;
+ ctrl |= (AUX_SEL << CLK_CTRL_SRC_SHIFT) & data->clk_src_mask;
+ } else {
+ ctrl &= ~data->clk_src_mask;
+ ctrl |= (index << CLK_CTRL_SRC_SHIFT) & data->clk_src_mask;
+ }
+
+ clockman_write(clockman, data->ctrl_reg, ctrl);
+ spin_unlock(&clockman->regs_lock);
+
+ sel = rp1_clock_get_parent(hw);
+ WARN_ONCE(sel != index, "(%s): Parent index req %u returned back %u\n",
+ clk_hw_get_name(hw), index, sel);Is this debug code? Why do we need to read back the parent here?
+
+ return 0;
+}
+
+static int rp1_clock_set_rate_and_parent(struct clk_hw *hw,
+ unsigned long rate,
+ unsigned long parent_rate,
+ u8 parent)
+{
+ struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+ struct rp1_clockman *clockman = clock->clockman;
+ const struct rp1_clock_data *data = clock->data;
+ u32 div = rp1_clock_choose_div(rate, parent_rate, data);
+
+ WARN_ONCE(rate > data->max_freq,
+ "(%s): Requested rate (%lu) > max rate (%lu)\n",
+ clk_hw_get_name(hw), rate, data->max_freq);If the determine_rate function is implemented properly this is impossible because we round the rate before calling this clk_op.
+ + if (WARN_ONCE(!div, + "clk divider calculated as 0! (%s, rate %lu, parent rate %lu)\n", + clk_hw_get_name(hw), rate, parent_rate)) + div = 1 << CLK_DIV_FRAC_BITS;
This one also looks weird, does it assume round_rate didn't constrain the incoming rate?
+
+ spin_lock(&clockman->regs_lock);
+
+ clockman_write(clockman, data->div_int_reg, div >> CLK_DIV_FRAC_BITS);
+ if (data->div_frac_reg)
+ clockman_write(clockman, data->div_frac_reg, div << (32 - CLK_DIV_FRAC_BITS));
+
+ spin_unlock(&clockman->regs_lock);
+
+ if (parent != 0xff)
+ rp1_clock_set_parent(hw, parent);
+
+ return 0;
+}
+
+static int rp1_clock_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ return rp1_clock_set_rate_and_parent(hw, rate, parent_rate, 0xff);
+}
+
+static void rp1_clock_choose_div_and_prate(struct clk_hw *hw,
+ int parent_idx,
+ unsigned long rate,
+ unsigned long *prate,
+ unsigned long *calc_rate)
+{
+ struct rp1_clk_desc *clock = container_of(hw, struct rp1_clk_desc, hw);
+ const struct rp1_clock_data *data = clock->data;
+ struct clk_hw *parent;
+ u32 div;
+ u64 tmp;
+
+ parent = clk_hw_get_parent_by_index(hw, parent_idx);
+
+ *prate = clk_hw_get_rate(parent);
+ div = rp1_clock_choose_div(rate, *prate, data);
+
+ if (!div) {
+ *calc_rate = 0;
+ return;
+ }
+
+ /* Recalculate to account for rounding errors */
+ tmp = (u64)*prate << CLK_DIV_FRAC_BITS;
+ tmp = div_u64(tmp, div);
+
+ /*
+ * Prevent overclocks - if all parent choices result in
+ * a downstream clock in excess of the maximum, then the
+ * call to set the clock will fail.
+ */
+ if (tmp > data->max_freq)
+ *calc_rate = 0;
+ else
+ *calc_rate = tmp;
+}
+
+static int rp1_clock_determine_rate(struct clk_hw *hw,
+ struct clk_rate_request *req)
+{
+ struct clk_hw *parent, *best_parent = NULL;
+ unsigned long best_rate = 0;
+ unsigned long best_prate = 0;
+ unsigned long best_rate_diff = ULONG_MAX;
+ unsigned long prate, calc_rate;
+ size_t i;
+
+ /*
+ * If the NO_REPARENT flag is set, try to use existing parent.
+ */
+ if ((clk_hw_get_flags(hw) & CLK_SET_RATE_NO_REPARENT)) {
+ i = rp1_clock_get_parent(hw);
+ parent = clk_hw_get_parent_by_index(hw, i);
+ if (parent) {
+ rp1_clock_choose_div_and_prate(hw, i, req->rate, &prate,
+ &calc_rate);
+ if (calc_rate > 0) {
+ req->best_parent_hw = parent;
+ req->best_parent_rate = prate;
+ req->rate = calc_rate;
+ return 0;
+ }
+ }
+ }
+
+ /*
+ * Select parent clock that results in the closest rate (lower or
+ * higher)
+ */
+ for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
+ parent = clk_hw_get_parent_by_index(hw, i);
+ if (!parent)
+ continue;
+
+ rp1_clock_choose_div_and_prate(hw, i, req->rate, &prate,
+ &calc_rate);
+
+ if (abs_diff(calc_rate, req->rate) < best_rate_diff) {
+ best_parent = parent;
+ best_prate = prate;
+ best_rate = calc_rate;
+ best_rate_diff = abs_diff(calc_rate, req->rate);
+
+ if (best_rate_diff == 0)
+ break;
+ }
+ }
+
+ if (best_rate == 0)
+ return -EINVAL;
+
+ req->best_parent_hw = best_parent;
+ req->best_parent_rate = best_prate;
+ req->rate = best_rate;
+
+ return 0;
+}
+
+static const struct clk_ops rp1_pll_core_ops = {
+ .is_prepared = rp1_pll_core_is_on,
+ .prepare = rp1_pll_core_on,
+ .unprepare = rp1_pll_core_off,
+ .set_rate = rp1_pll_core_set_rate,
+ .recalc_rate = rp1_pll_core_recalc_rate,
+ .round_rate = rp1_pll_core_round_rate,
+};
+
+static const struct clk_ops rp1_pll_ops = {
+ .set_rate = rp1_pll_set_rate,
+ .recalc_rate = rp1_pll_recalc_rate,
+ .round_rate = rp1_pll_round_rate,
+};
+
+static const struct clk_ops rp1_pll_ph_ops = {
+ .is_prepared = rp1_pll_ph_is_on,
+ .prepare = rp1_pll_ph_on,
+ .unprepare = rp1_pll_ph_off,
+ .recalc_rate = rp1_pll_ph_recalc_rate,
+ .round_rate = rp1_pll_ph_round_rate,
+};
+
+static const struct clk_ops rp1_pll_divider_ops = {
+ .is_prepared = rp1_pll_divider_is_on,
+ .prepare = rp1_pll_divider_on,
+ .unprepare = rp1_pll_divider_off,
+ .set_rate = rp1_pll_divider_set_rate,
+ .recalc_rate = rp1_pll_divider_recalc_rate,
+ .round_rate = rp1_pll_divider_round_rate,
+};
+
+static const struct clk_ops rp1_clk_ops = {
+ .is_prepared = rp1_clock_is_on,
+ .prepare = rp1_clock_on,
+ .unprepare = rp1_clock_off,
+ .recalc_rate = rp1_clock_recalc_rate,
+ .get_parent = rp1_clock_get_parent,
+ .set_parent = rp1_clock_set_parent,
+ .set_rate_and_parent = rp1_clock_set_rate_and_parent,
+ .set_rate = rp1_clock_set_rate,
+ .determine_rate = rp1_clock_determine_rate,
+};
+
+static struct clk_hw *rp1_register_pll(struct rp1_clockman *clockman,
+ struct rp1_clk_desc *desc)
+{
+ int ret;
+
+ desc->clockman = clockman;
+
+ ret = devm_clk_hw_register(clockman->dev, &desc->hw);
+Please drop this newline.
+ if (ret)
+ return ERR_PTR(ret);
+
+ return &desc->hw;
+}
+
+static struct clk_hw *rp1_register_pll_divider(struct rp1_clockman *clockman,
+ struct rp1_clk_desc *desc)
+{
+ const struct rp1_pll_data *divider_data = desc->data;
+ int ret;
+
+ desc->div.reg = clockman->regs + divider_data->ctrl_reg;
+ desc->div.shift = __ffs(PLL_SEC_DIV_MASK);
+ desc->div.width = __ffs(~(PLL_SEC_DIV_MASK >> desc->div.shift));
+ desc->div.flags = CLK_DIVIDER_ROUND_CLOSEST;
+ desc->div.lock = &clockman->regs_lock;
+ desc->div.hw.init = desc->hw.init;
+ desc->div.table = pll_sec_div_table;
+
+ desc->clockman = clockman;
+
+ ret = devm_clk_hw_register(clockman->dev, &desc->div.hw);
+Please drop this newline.
+ if (ret)
+ return ERR_PTR(ret);
+
+ return &desc->div.hw;
+}
+
+static struct clk_hw *rp1_register_clock(struct rp1_clockman *clockman,
+ struct rp1_clk_desc *desc)
+{
+ const struct rp1_clock_data *clock_data = desc->data;
+ int ret;
+
+ if (WARN_ON_ONCE(MAX_CLK_PARENTS <
+ clock_data->num_std_parents + clock_data->num_aux_parents))
+ return NULL;Return an error pointer?
+ + /* There must be a gap for the AUX selector */ + if (WARN_ON_ONCE(clock_data->num_std_parents > AUX_SEL && + desc->hw.init->parent_data[AUX_SEL].index != -1))
Why is there a gap? Can't the parents that the clk framework sees be [0, num_std_parents) + [num_std_parents, num_aux_parents + num_std_parents) without an empty parent in the middle?
+ return NULL;
Return an error pointer?
+ + desc->clockman = clockman; + + ret = devm_clk_hw_register(clockman->dev, &desc->hw); +
Drop this newline please.
+ if (ret) + return ERR_PTR(ret); + + return &desc->hw; +}
[...]
+
+static const struct clk_parent_data clk_eth_parents[] = {
+ { .hw = &pll_sys_sec_desc.div.hw },
+ { .hw = &pll_sys_desc.hw },
+};
+
+static struct rp1_clk_desc clk_eth_desc = REGISTER_CLK(
+ .hw.init = CLK_HW_INIT_PARENTS_DATA(
+ "clk_eth",
+ clk_eth_parents,
+ &rp1_clk_ops,
+ 0
+ ),
+ CLK_DATA(rp1_clock_data,
+ .num_std_parents = 0,
+ .num_aux_parents = 2,
+ .ctrl_reg = CLK_ETH_CTRL,
+ .div_int_reg = CLK_ETH_DIV_INT,
+ .sel_reg = CLK_ETH_SEL,
+ .div_int_max = DIV_INT_8BIT_MAX,
+ .max_freq = 125 * HZ_PER_MHZ,
+ .fc0_src = FC_NUM(4, 6),
+ )
+);
+
+static const struct clk_parent_data clk_sys_parents[] = {
+ { .index = 0 },
+ { .index = -1 },Why is there a gap here?
+ { .hw = &pll_sys_desc.hw },
+};
+[...]
+
+static const struct regmap_config rp1_clk_regmap_cfg = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .max_register = PLL_VIDEO_SEC,
+ .name = "rp1-clk",
+ .rd_table = &rp1_reg_table,Do you want to set the 'disable_locking' field because you're explicitly locking in this driver?
+};
+
+static int rp1_clk_probe(struct platform_device *pdev)
+{
+ const size_t asize = ARRAY_SIZE(clk_desc_array);
+ struct rp1_clk_desc *desc;
+ struct device *dev = &pdev->dev;
+ struct rp1_clockman *clockman;
+ struct clk_hw **hws;
+ unsigned int i;
+
+ clockman = devm_kzalloc(dev, struct_size(clockman, onecell.hws, asize),
+ GFP_KERNEL);
+ if (!clockman)
+ return -ENOMEM;
+
+ spin_lock_init(&clockman->regs_lock);
+ clockman->dev = dev;
+
+ clockman->regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(clockman->regs))
+ return PTR_ERR(clockman->regs);
+
+ clockman->regmap = devm_regmap_init_mmio(dev, clockman->regs,
+ &rp1_clk_regmap_cfg);
+ if (IS_ERR(clockman->regmap)) {
+ dev_err_probe(dev, PTR_ERR(clockman->regmap),
+ "could not init clock regmap\n");
+ return PTR_ERR(clockman->regmap);
+ }
+
+ clockman->onecell.num = asize;
+ hws = clockman->onecell.hws;
+
+ for (i = 0; i < asize; i++) {
+ desc = clk_desc_array[i];
+ if (desc && desc->clk_register && desc->data) {
+ hws[i] = desc->clk_register(clockman, desc);
+ if (IS_ERR_OR_NULL(hws[i]))Why is NULL a possible return value?
+ dev_err_probe(dev, PTR_ERR(hws[i]), + "Unable to register clock: %s\n", + clk_hw_get_name(hws[i]));
We pushed this into the core now so you can drop this. See commit
12a0fd23e870 ("clk: Print an error when clk registration fails").
+ } + } + + platform_set_drvdata(pdev, clockman); + + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, + &clockman->onecell); +} +