Quoting Andrea della Porta (2025-04-22 11:53:13)
> 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);
> +}
> +
