[PATCH v7 06/10] clk: Add Sunplus SP7021 clock driver
Stephen Boyd
sboyd at kernel.org
Wed Jan 5 16:42:48 PST 2022
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.
More information about the linux-arm-kernel
mailing list