Quoting Qin Jian (2021-12-21 23:06:02)
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index c5b3dc973..4a186ccf6 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -334,6 +334,15 @@ config COMMON_CLK_VC5
> This driver supports the IDT VersaClock 5 and VersaClock 6
> programmable clock generators.
>
> +config COMMON_CLK_SP7021
> + bool "Clock driver for Sunplus SP7021 SoC"
> + default SOC_SP7021
> + help
> + This driver supports the Sunplus SP7021 SoC clocks.
> + It implements SP7021 PLLs/gate.
> + Not all features of the PLL are currently supported
> + by the driver.
> +
> config COMMON_CLK_STM32MP157
> def_bool COMMON_CLK && MACH_STM32MP157
> help
> @@ -366,7 +375,6 @@ config COMMON_CLK_MMP2
>
> config COMMON_CLK_MMP2_AUDIO
> tristate "Clock driver for MMP2 Audio subsystem"
> - depends on COMMON_CLK_MMP2 || COMPILE_TEST
> help
> This driver supports clocks for Audio subsystem on MMP2 SoC.
>
What's the relevance of this hunk to this patch? Can you drop this part?
> diff --git a/drivers/clk/clk-sp7021.c b/drivers/clk/clk-sp7021.c
> new file mode 100644
> index 000000000..6df87f0a6
> --- /dev/null
> +++ b/drivers/clk/clk-sp7021.c
> @@ -0,0 +1,705 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Copyright (C) Sunplus Technology Co., Ltd.
> + * All rights reserved.
> + */
> +//#define DEBUG
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/bitfield.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <dt-bindings/clock/sp-sp7021.h>
> +
> +#define REG(i) (pll_regs + (i) * 4)
> +
[....]
> +
> +static long plltv_div(struct sp_pll *clk, unsigned long freq)
> +{
> + if (freq % 100)
> + return plltv_fractional_div(clk, freq);
> + else
> + return plltv_integer_div(clk, freq);
Drop else please
> +}
> +
> +static void plltv_set_rate(struct sp_pll *clk)
> +{
> + u32 reg;
> +
> + reg = HWM_FIELD_PREP(MASK_SEL_FRA, clk->p[SEL_FRA]);
> + reg |= HWM_FIELD_PREP(MASK_SDM_MOD, clk->p[SDM_MOD]);
> + reg |= HWM_FIELD_PREP(MASK_PH_SEL, clk->p[PH_SEL]);
> + reg |= HWM_FIELD_PREP(MASK_NFRA, clk->p[NFRA]);
> + writel(reg, clk->reg);
> +
> + reg = HWM_FIELD_PREP(MASK_DIVR, clk->p[DIVR]);
[...]
> + .set_rate = sp_pll_set_rate
> +};
> +
> +static const struct clk_ops sp_pll_sub_ops = {
> + .enable = sp_pll_enable,
> + .disable = sp_pll_disable,
> + .is_enabled = sp_pll_is_enabled,
> + .recalc_rate = sp_pll_recalc_rate,
> +};
> +
> +struct clk_hw *sp_pll_register(const char *name, const char *parent,
> + void __iomem *reg, int pd_bit, int bp_bit,
> + unsigned long brate, int shift, int width,
> + spinlock_t *lock)
> +{
> + struct sp_pll *pll;
> + struct clk_hw *hw;
> + struct clk_init_data initd = {
> + .name = name,
> + .parent_names = &parent,
Any chance clk_parent_data can be used instead of string names?
> + .ops = (bp_bit >= 0) ? &sp_pll_ops : &sp_pll_sub_ops,
> + .num_parents = 1,
> + };
> + int ret;
> +
> + pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> + if (!pll)
> + return ERR_PTR(-ENOMEM);
> +
> + if (reg == PLLSYS_CTL) /* system clock, should not be closed */
s/closed/disabled/
> + initd.flags |= CLK_IS_CRITICAL;
> +
> + pll->hw.init = &initd;
> + pll->reg = reg;
> + pll->pd_bit = pd_bit;
> + pll->bp_bit = bp_bit;
> + pll->brate = brate;
> + pll->div_shift = shift;
> + pll->div_width = width;
> + pll->lock = lock;
> +
> + hw = &pll->hw;
> + ret = clk_hw_register(NULL, hw);
> + if (ret) {
> + kfree(pll);
> + hw = ERR_PTR(ret);
> + } else {
> + pr_info("%-20s%lu\n", name, clk_hw_get_rate(hw));
Drop this print or make it pr_debug()
> + clk_hw_register_clkdev(hw, NULL, name);
> + }
> +
> + return hw;
> +}
> +
> +static void __init sp_clk_setup(struct device_node *np)
> +{
> + int i, j;
> + struct clk_hw **hws;
> +
> + sp_clk_base = of_iomap(np, 0);
> + if (WARN_ON(!sp_clk_base))
> + return; /* -ENXIO */
> +
> + sp_clk_data = kzalloc(struct_size(sp_clk_data, hws, CLK_MAX), GFP_KERNEL);
> + if (!sp_clk_data)
> + return; /* -ENOMEM */
> +
> + hws = sp_clk_data->hws;
> +
> + /* PLL_A */
> + hws[PLL_A] = sp_pll_register("plla", EXTCLK, PLLA_CTL, 11, 12,
> + 27000000, 0, DIV_A, &plla_lock);
> +
> + /* PLL_E */
> + hws[PLL_E] = sp_pll_register("plle", EXTCLK, PLLE_CTL, 6, 2,
> + 50000000, 0, 0, &plle_lock);
> + hws[PLL_E_2P5] = sp_pll_register("plle_2p5", "plle", PLLE_CTL, 13, -1,
> + 2500000, 0, 0, &plle_lock);
> + hws[PLL_E_25] = sp_pll_register("plle_25", "plle", PLLE_CTL, 12, -1,
> + 25000000, 0, 0, &plle_lock);
> + hws[PLL_E_112P5] = sp_pll_register("plle_112p5", "plle", PLLE_CTL, 11, -1,
> + 112500000, 0, 0, &plle_lock);
> +
> + /* PLL_F */
> + hws[PLL_F] = sp_pll_register("pllf", EXTCLK, PLLF_CTL, 0, 10,
> + 13500000, 1, 4, &pllf_lock);
> +
> + /* PLL_TV */
> + hws[PLL_TV] = sp_pll_register("plltv", EXTCLK, PLLTV_CTL, 0, 15,
> + 27000000, 0, DIV_TV, &plltv_lock);
> + hws[PLL_TV_A] = clk_hw_register_divider(NULL, "plltv_a", "plltv", 0,
> + PLLTV_CTL + 4, 5, 1,
> + CLK_DIVIDER_POWER_OF_TWO,
> + &plltv_lock);
> + clk_hw_register_clkdev(hws[PLL_TV_A], NULL, "plltv_a");
> +
> + /* PLL_SYS */
> + hws[PLL_SYS] = sp_pll_register("pllsys", EXTCLK, PLLSYS_CTL, 10, 9,
> + 13500000, 0, 4, &pllsys_lock);
> +
> + /* gates */
> + for (i = 0; i < ARRAY_SIZE(gates); i++) {
> + char s[10];
> +
> + j = gates[i] & 0xffff;
> + sprintf(s, "clken%02x", j);
> + hws[j] = clk_hw_register_gate(NULL, s,
> + parents[gates[i] >> 16].fw_name,
> + CLK_IGNORE_UNUSED,
Why CLK_IGNORE_UNUSED? There should be a comment or it should be
replaced with CLK_IS_CRITICAL.
> + clk_regs + (j >> 4) * 4, j & 0x0f,
> + CLK_GATE_HIWORD_MASK, NULL);
> + }
> +
> + sp_clk_data->num = CLK_MAX;
> + of_clk_add_hw_provider(np, of_clk_hw_onecell_get, sp_clk_data);
> +}
> +
> +CLK_OF_DECLARE_DRIVER(sp_clkc, "sunplus,sp7021-clkc", sp_clk_setup);
Why CLK_OF_DECLARE_DRIVER? There should be a comment but better would be
to make a platform driver instead. If the platform driver can't be used,
we need to know what other device driver is probing based on this clkc
compatible string.
