Hi Torsten,
On 2026-09-25T10:54:32, Torsten Duwe <[email protected]> wrote:
> clk: add support for RPi5 RP1 chip clk
>
> Add a driver for the clocks in RP1, starting with ETH_TSU.
> Derived from Linux drivers/clk/clk-rp1.c.
>
> Signed-off-by: Torsten Duwe <[email protected]>
> Co-authored-by: Oleksii Moisieiev <[email protected]>
>
> configs/rpi_arm64_defconfig | 2 +
> drivers/clk/Kconfig | 7 ++
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-rp1.c | 280
> ++++++++++++++++++++++++++++++++++++++++++
> include/dt-bindings/clk/rp1.h | 56 +++++++++
> 5 files changed, 346 insertions(+)
Please expand the commit message to explain the motivation - which
downstream driver in this series needs the ETH_TSU clock and why the
other RP1 clocks are deliberately left as stubs. A one-line summary
and a "derived from Linux" line does not really tell reviewers what to
expect.
> diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c
> @@ -0,0 +1,280 @@
> +struct rp1_pll_core_data {
> + const char *name;
> + u32 cs_reg;
> + u32 pwr_reg;
> + u32 fbdiv_int_reg;
> + u32 fbdiv_frac_reg;
> + unsigned long flags;
> + u32 fc0_src;
> +};
None of rp1_pll_core_data, rp1_pll_data, rp1_pll_ph_data,
rp1_pll_divider_data, rp1_pll_core, rp1_pll, rp1_pll_ph,
rp1_clk_change or rp1_clk_chg_tree[3] are referenced anywhere in the
driver. Please drop the dead scaffolding - a follow-up patch can add
these back when the corresponding clock support is actually
implemented.
> diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c
> @@ -0,0 +1,280 @@
> +struct rp1_clk_change {
> + struct clk *hw;
> + unsigned long new_rate;
> +};
> +
> +struct rp1_clk_change rp1_clk_chg_tree[3];
Non-static global - if this were needed it would need to be static,
but as noted above it is unused and should just be removed.
> diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c
> @@ -0,0 +1,280 @@
> +static ulong rp1_clock_set_rate(struct clk *hw, unsigned long rate)
> +{
> + struct rp1_clockman *clockman = dev_get_priv(hw->dev);
> + const struct rp1_clock_data *data = &rp1_data[hw->id];
> + u32 div = rp1_clock_choose_div(rate, 0x2faf080, data);
> +
> + if (hw->id != RP1_CLK_ETH_TSU)
> + return 0;
Two problems - &rp1_data[hw->id] happens before the id check, and
rp1_data[] only has one initialised entry (index RP1_CLK_ETH_TSU =
29), so any other id is an out-of-bounds read. Please bounds-check
first, then bail out with -ENOTSUPP for unsupported clocks rather than
silently returning success.
Also, 0x2faf080 is 50 MHz, right? Please give it a #define with a
descriptive name, and ideally read the parent rate from the clock
framework rather than hard-coding it.
> diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c
> @@ -0,0 +1,280 @@
> + if (hw->id != RP1_CLK_ETH_TSU)
> + return 0;
> +
> + WARN(rate > 4000000000ll, "rate is -ve (%d)\n", (int)rate);
Technically, rate is an unsigned long so it can never be negative -
the "rate is -ve" message and the cast to (int) are misleading. Please
fix the message or drop the WARN.
> diff --git a/drivers/clk/clk-rp1.c b/drivers/clk/clk-rp1.c
> @@ -0,0 +1,280 @@
> + 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);
> +
> + return 0;
> +}
set_rate() should return the newly set rate on success, not 0 -
otherwise callers using clk_set_rate() to check what they got will be
confused. Please return the computed rate.
Regards,
Simon
