Re: [PATCH v2 2/7] clk: thead: Add support for T-Head TH1520 AP_SUBSYS clocks

[Stephen Boyd <sboyd@kernel.org>]

Quoting Drew Fustini (2024-06-23 19:12:32)
> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> new file mode 100644
> index 000000000000..982d4d40f783
> --- /dev/null
> +++ b/drivers/clk/thead/clk-th1520-ap.c
> @@ -0,0 +1,1086 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
> + *  Authors: Yangtao Li <frank.li@vivo.com>
> + */
> +
> +#include <dt-bindings/clock/thead,th1520-clk-ap.h>

Preferably include dt-bindings after linux includes.

> +#include <linux/bitfield.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define TH1520_PLL_POSTDIV2    GENMASK(26, 24)
> +#define TH1520_PLL_POSTDIV1    GENMASK(22, 20)
> +#define TH1520_PLL_FBDIV       GENMASK(19, 8)
> +#define TH1520_PLL_REFDIV      GENMASK(5, 0)
> +#define TH1520_PLL_BYPASS      BIT(30)
> +#define TH1520_PLL_DSMPD       BIT(24)
> +#define TH1520_PLL_FRAC                GENMASK(23, 0)
> +#define TH1520_PLL_FRAC_BITS    24
[...]
> +
> +static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
> +                                               unsigned long parent_rate)
> +{
> +       struct ccu_pll *pll = hw_to_ccu_pll(hw);
> +       unsigned long div, mul, frac, rate = parent_rate;
> +       unsigned int cfg0, cfg1;
> +
> +       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> +       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> +
> +       mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
> +       div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
> +       if (!(cfg1 & TH1520_PLL_DSMPD)) {
> +               mul <<= TH1520_PLL_FRAC_BITS;
> +               frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
> +               mul += frac;
> +               div <<= TH1520_PLL_FRAC_BITS;
> +       }
> +       rate = parent_rate * mul;
> +       do_div(rate, div);

'rate' is only unsigned long, so do_div() isn't needed here. Perhaps if
'parent_rate * mul' can overflow 32-bits then 'rate' should be
u64.

> +       return rate;
> +}
> +
> +static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
> +                                                   unsigned long parent_rate)
> +{
> +       struct ccu_pll *pll = hw_to_ccu_pll(hw);
> +       unsigned long rate = parent_rate;
> +       unsigned int cfg0, cfg1;
> +
> +       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> +       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> +
> +       if (cfg1 & TH1520_PLL_BYPASS)
> +               return rate;
> +
> +       do_div(rate, FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *

Same, 'rate' is unsigned long. Did you get some compilation error
without this? How big is the divisor going to be? The fields are only
3-bits wide, so the multiplication would fit into a u32 just fine. Given
that 'rate' is unsigned long though I think you can just put the
multiplication result into a local variable that's also unsigned long
and then just write the divide with unsigned longs

	div = FIELD_GET(...) * FIELD_GET(...);

	return rate / div;

> +                    FIELD_GET(TH1520_PLL_POSTDIV2, cfg0));
> +
> +       return rate;
> +}
> +
> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
> +                                        unsigned long parent_rate)
> +{
> +       unsigned long rate = parent_rate;
> +
> +       rate = th1520_pll_vco_recalc_rate(hw, rate);
> +       rate = th1520_pll_postdiv_recalc_rate(hw, rate);
> +
> +       return rate;
> +}

Please fold this in

----8<---
diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
index 982d4d40f783..0c3821d50765 100644
--- a/drivers/clk/thead/clk-th1520-ap.c
+++ b/drivers/clk/thead/clk-th1520-ap.c
@@ -411,7 +411,7 @@ static const struct clk_parent_data c910_i0_parents[] = {
 	{ .index = 0 }
 };
 
-struct ccu_mux c910_i0_clk = {
+static struct ccu_mux c910_i0_clk = {
 	.mux	= TH_CCU_ARG(1, 1),
 	.common	= {
 		.clkid		= CLK_C910_I0,
@@ -428,7 +428,7 @@ static const struct clk_parent_data c910_parents[] = {
 	{ .hw = &cpu_pll1_clk.common.hw }
 };
 
-struct ccu_mux c910_clk = {
+static struct ccu_mux c910_clk = {
 	.mux	= TH_CCU_ARG(0, 1),
 	.common	= {
 		.clkid		= CLK_C910,
@@ -845,7 +845,7 @@ static const struct clk_parent_data uart_sclk_parents[] = {
 	{ .index = 0 },
 };
 
-struct ccu_mux uart_sclk = {
+static struct ccu_mux uart_sclk = {
 	.mux	= TH_CCU_ARG(0, 1),
 	.common	= {
 		.clkid          = CLK_UART_SCLK,