Re: [PATCH v7 06/10] clk: Add Sunplus SP7021 clock driver

[Stephen Boyd <sboyd@kernel.org>]

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.