Re: [PATCH v11 3/6] clk: ralink: add clock driver for mt7621 SoC

[Stephen Boyd]

Quoting Sergio Paracuellos (2021-03-08 21:22:23)
> diff --git a/drivers/clk/ralink/Kconfig b/drivers/clk/ralink/Kconfig
> new file mode 100644
> index 000000000000..3e3f5cb9ad88
> --- /dev/null
> +++ b/drivers/clk/ralink/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# MediaTek Mt7621 Clock Driver
> +#
> +menu "Clock driver for Mediatek mt7621 SoC"
> +       depends on SOC_MT7621 || COMPILE_TEST

Do we need a menu and a config that says the same thing? Maybe the menu
can be dropped?

> +
> +config CLK_MT7621
> +       bool "Clock driver for MediaTek MT7621"
> +       depends on SOC_MT7621 || COMPILE_TEST
> +       default SOC_MT7621
> +       select MFD_SYSCON
> +       help
> +         This driver supports MediaTek MT7621 basic clocks.
> +endmenu
> diff --git a/drivers/clk/ralink/Makefile b/drivers/clk/ralink/Makefile
> new file mode 100644
> index 000000000000..cf6f9216379d
> --- /dev/null
> +++ b/drivers/clk/ralink/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_CLK_MT7621) += clk-mt7621.o
> diff --git a/drivers/clk/ralink/clk-mt7621.c b/drivers/clk/ralink/clk-mt7621.c
> new file mode 100644
> index 000000000000..6aea5accd51c
> --- /dev/null
> +++ b/drivers/clk/ralink/clk-mt7621.c
> @@ -0,0 +1,528 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Mediatek MT7621 Clock Driver
> + * Author: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <dt-bindings/clock/mt7621-clk.h>
> +
> +/* Configuration registers */
> +#define SYSC_REG_SYSTEM_CONFIG0         0x10
> +#define SYSC_REG_SYSTEM_CONFIG1         0x14
> +#define SYSC_REG_CLKCFG0               0x2c
> +#define SYSC_REG_CLKCFG1               0x30
> +#define SYSC_REG_CUR_CLK_STS           0x44
> +#define MEMC_REG_CPU_PLL               0x648
> +
> +#define XTAL_MODE_SEL_MASK             GENMASK(8, 6)
> +#define CPU_CLK_SEL_MASK               GENMASK(31, 30)
> +#define CUR_CPU_FDIV_MASK              GENMASK(12, 8)
> +#define CUR_CPU_FFRAC_MASK             GENMASK(4, 0)
> +#define CPU_PLL_PREDIV_MASK            GENMASK(13, 12)
> +#define CPU_PLL_FBDIV_MASK             GENMASK(10, 4)
> +
> +struct mt7621_clk_priv {
> +       struct regmap *sysc;
> +       struct regmap *memc;
> +};
> +
> +struct mt7621_clk {
> +       struct clk_hw hw;
> +       struct mt7621_clk_priv *priv;
> +};
> +
> +struct mt7621_fixed_clk {
> +       u8 idx;
> +       const char *name;
> +       const char *parent_name;
> +       unsigned long rate;
> +       struct clk_hw *hw;
> +};
> +
> +struct mt7621_gate {
> +       u8 idx;
> +       const char *name;
> +       const char *parent_name;
> +       struct mt7621_clk_priv *priv;
> +       u32 bit_idx;
> +       struct clk_hw hw;
> +};
> +
> +#define GATE(_id, _name, _pname, _shift)       \
> +       {                                       \
> +               .idx            = _id,          \
> +               .name           = _name,        \
> +               .parent_name    = _pname,       \
> +               .bit_idx        = _shift        \
> +       }
> +
> +static struct mt7621_gate mt7621_gates[] = {
> +       GATE(MT7621_CLK_HSDMA, "hsdma", "150m", BIT(5)),
> +       GATE(MT7621_CLK_FE, "fe", "250m", BIT(6)),
> +       GATE(MT7621_CLK_SP_DIVTX, "sp_divtx", "270m", BIT(7)),
> +       GATE(MT7621_CLK_TIMER, "timer", "50m", BIT(8)),
> +       GATE(MT7621_CLK_PCM, "pcm", "270m", BIT(11)),
> +       GATE(MT7621_CLK_PIO, "pio", "50m", BIT(13)),
> +       GATE(MT7621_CLK_GDMA, "gdma", "bus", BIT(14)),
> +       GATE(MT7621_CLK_NAND, "nand", "125m", BIT(15)),
> +       GATE(MT7621_CLK_I2C, "i2c", "50m", BIT(16)),
> +       GATE(MT7621_CLK_I2S, "i2s", "270m", BIT(17)),
> +       GATE(MT7621_CLK_SPI, "spi", "bus", BIT(18)),
> +       GATE(MT7621_CLK_UART1, "uart1", "50m", BIT(19)),
> +       GATE(MT7621_CLK_UART2, "uart2", "50m", BIT(20)),
> +       GATE(MT7621_CLK_UART3, "uart3", "50m", BIT(21)),
> +       GATE(MT7621_CLK_ETH, "eth", "50m", BIT(23)),
> +       GATE(MT7621_CLK_PCIE0, "pcie0", "125m", BIT(24)),
> +       GATE(MT7621_CLK_PCIE1, "pcie1", "125m", BIT(25)),
> +       GATE(MT7621_CLK_PCIE2, "pcie2", "125m", BIT(26)),
> +       GATE(MT7621_CLK_CRYPTO, "crypto", "250m", BIT(29)),
> +       GATE(MT7621_CLK_SHXC, "shxc", "50m", BIT(30))
> +};
> +
> +static inline struct mt7621_gate *to_mt7621_gate(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct mt7621_gate, hw);
> +}
> +
> +static int mt7621_gate_enable(struct clk_hw *hw)
> +{
> +       struct mt7621_gate *clk_gate = to_mt7621_gate(hw);
> +       struct regmap *sysc = clk_gate->priv->sysc;
> +
> +       return regmap_update_bits(sysc, SYSC_REG_CLKCFG1,
> +                                 clk_gate->bit_idx, clk_gate->bit_idx);
> +}
> +
> +static void mt7621_gate_disable(struct clk_hw *hw)
> +{
> +       struct mt7621_gate *clk_gate = to_mt7621_gate(hw);
> +       struct regmap *sysc = clk_gate->priv->sysc;
> +
> +       regmap_update_bits(sysc, SYSC_REG_CLKCFG1, clk_gate->bit_idx, 0);
> +}
> +
> +static int mt7621_gate_is_enabled(struct clk_hw *hw)
> +{
> +       struct mt7621_gate *clk_gate = to_mt7621_gate(hw);
> +       struct regmap *sysc = clk_gate->priv->sysc;
> +       u32 val;
> +
> +       if (regmap_read(sysc, SYSC_REG_CLKCFG1, &val))
> +               return 0;
> +
> +       return val & BIT(clk_gate->bit_idx);
> +}
> +
> +static const struct clk_ops mt7621_gate_ops = {
> +       .enable = mt7621_gate_enable,
> +       .disable = mt7621_gate_disable,
> +       .is_enabled = mt7621_gate_is_enabled,
> +};
> +
> +static int mt7621_gate_ops_init(struct device *dev,
> +                               struct mt7621_gate *sclk)
> +{
> +       struct clk_init_data init = {
> +               .flags = CLK_SET_RATE_PARENT,
> +               .num_parents = 1,
> +               .parent_names = &sclk->parent_name,
> +               .ops = &mt7621_gate_ops,
> +               .name = sclk->name,
> +       };
> +
> +       sclk->hw.init = &init;
> +       return devm_clk_hw_register(dev, &sclk->hw);
> +}
> +
> +static int mt7621_register_gates(struct device *dev,
> +                                struct clk_hw_onecell_data *clk_data,
> +                                struct mt7621_clk_priv *priv)
> +{
> +       struct clk_hw **hws = clk_data->hws;
> +       int ret, i;
> +
> +       for (i = 0; i < ARRAY_SIZE(mt7621_gates); i++) {
> +               struct mt7621_gate *sclk = &mt7621_gates[i];
> +
> +               sclk->priv = priv;
> +               ret = mt7621_gate_ops_init(dev, sclk);
> +               if (ret) {
> +                       dev_err(dev, "Couldn't register clock %s\n", sclk->name);
> +                       goto err_clk_unreg;
> +               }
> +
> +               hws[sclk->idx] = &sclk->hw;
> +       }
> +
> +       return 0;
> +
> +err_clk_unreg:
> +       while (--i >= 0) {
> +               struct mt7621_gate *sclk = &mt7621_gates[i];
> +
> +               clk_hw_unregister(&sclk->hw);
> +       }
> +       return ret;
> +}
> +
> +#define FIXED(_id, _name, _rate)               \
> +       {                                       \
> +               .idx            = _id,          \
> +               .name           = _name,        \
> +               .parent_name    = "xtal",       \
> +               .rate           = _rate         \
> +       }
> +
> +static struct mt7621_fixed_clk mt7621_fixed_clks[] = {
> +       FIXED(MT7621_CLK_50M, "50m", 50000000),
> +       FIXED(MT7621_CLK_125M, "125m", 125000000),
> +       FIXED(MT7621_CLK_150M, "150m", 150000000),
> +       FIXED(MT7621_CLK_250M, "250m", 250000000),
> +       FIXED(MT7621_CLK_270M, "270m", 270000000),
> +};
> +
> +static int mt7621_register_fixed_clocks(struct device *dev,
> +                                       struct clk_hw_onecell_data *clk_data)
> +{
> +       struct clk_hw **hws = clk_data->hws;
> +       int ret, i;
> +
> +       for (i = 0; i < ARRAY_SIZE(mt7621_fixed_clks); i++) {
> +               struct mt7621_fixed_clk *sclk = &mt7621_fixed_clks[i];
> +
> +               sclk->hw = clk_hw_register_fixed_rate(dev, sclk->name,
> +                                                     sclk->parent_name, 0,
> +                                                     sclk->rate);
> +               if (IS_ERR(sclk->hw)) {
> +                       dev_err(dev, "Couldn't register clock %s\n", sclk->name);
> +                       ret = PTR_ERR(sclk->hw);
> +                       goto err_clk_unreg;
> +               }
> +
> +               hws[sclk->idx] = sclk->hw;
> +       }
> +
> +       return 0;
> +
> +err_clk_unreg:
> +       while (--i >= 0) {
> +               struct mt7621_fixed_clk *sclk = &mt7621_fixed_clks[i];
> +
> +               clk_hw_unregister_fixed_rate(sclk->hw);
> +       }
> +       return ret;
> +}
> +
> +static inline struct mt7621_clk *to_mt7621_clk(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct mt7621_clk, hw);
> +}
> +
> +static unsigned long mt7621_xtal_recalc_rate(struct clk_hw *hw,
> +                                            unsigned long parent_rate)
> +{
> +       struct mt7621_clk *clk = to_mt7621_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 val;
> +
> +       regmap_read(sysc, SYSC_REG_SYSTEM_CONFIG0, &val);
> +       val = FIELD_GET(XTAL_MODE_SEL_MASK, val);
> +
> +       if (val <= 2)
> +               return 20000000;
> +       if (val <= 5)
> +               return 40000000;
> +
> +       return 25000000;
> +}
> +
> +static unsigned long mt7621_cpu_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long xtal_clk)
> +{
> +       static const u32 prediv_tbl[] = { 0, 1, 2, 2 };
> +       struct mt7621_clk *clk = to_mt7621_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       struct regmap *memc = clk->priv->memc;
> +       u32 clkcfg, clk_sel, curclk, ffiv, ffrac;
> +       u32 pll, prediv, fbdiv;
> +       unsigned long cpu_clk;
> +
> +       regmap_read(sysc, SYSC_REG_CLKCFG0, &clkcfg);
> +       clk_sel = FIELD_GET(CPU_CLK_SEL_MASK, clkcfg);
> +
> +       regmap_read(sysc, SYSC_REG_CUR_CLK_STS, &curclk);
> +       ffiv = FIELD_GET(CUR_CPU_FDIV_MASK, curclk);
> +       ffrac = FIELD_GET(CUR_CPU_FFRAC_MASK, curclk);
> +
> +       switch (clk_sel) {
> +       case 0:
> +               cpu_clk = 500000000;
> +               break;
> +       case 1:
> +               regmap_read(memc, MEMC_REG_CPU_PLL, &pll);
> +               fbdiv = FIELD_GET(CPU_PLL_FBDIV_MASK, pll);
> +               prediv = FIELD_GET(CPU_PLL_PREDIV_MASK, pll);
> +               cpu_clk = ((fbdiv + 1) * xtal_clk) >> prediv_tbl[prediv];
> +               break;
> +       default:
> +               cpu_clk = xtal_clk;
> +       }
> +
> +       return cpu_clk / ffiv * ffrac;
> +}
> +
> +static unsigned long mt7621_bus_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       return parent_rate / 4;
> +}
> +
> +#define CLK_BASE(_name, _parent, _recalc) {                            \
> +       .init = &(struct clk_init_data) {                               \
> +               .name = _name,                                          \
> +               .ops = &(const struct clk_ops) {                        \
> +                       .recalc_rate = _recalc,                         \
> +               },                                                      \
> +               .parent_data = &(const struct clk_parent_data) {        \
> +                       .name = _parent,                                \
> +                       .fw_name = _parent                              \
> +               },                                                      \
> +               .num_parents = _parent ? 1 : 0                          \
> +       },                                                              \
> +}
> +
> +static struct mt7621_clk mt7621_clks_base[] = {
> +       { CLK_BASE("xtal", NULL, mt7621_xtal_recalc_rate) },
> +       { CLK_BASE("cpu", "xtal", mt7621_cpu_recalc_rate) },
> +       { CLK_BASE("bus", "cpu", mt7621_bus_recalc_rate) },
> +};
> +
> +static struct clk_hw *mt7621_clk_early[MT7621_CLK_MAX];
> +
> +static int mt7621_register_early_clocks(struct device_node *np,
> +                                       struct clk_hw_onecell_data *clk_data,
> +                                       struct mt7621_clk_priv *priv)
> +{
> +       struct clk_hw **hws = clk_data->hws;
> +       int ret, i, j;
> +
> +       for (i = 0; i < ARRAY_SIZE(mt7621_clks_base); i++) {
> +               struct mt7621_clk *sclk = &mt7621_clks_base[i];
> +
> +               sclk->priv = priv;
> +               ret = of_clk_hw_register(np, &sclk->hw);
> +               if (ret) {
> +                       pr_err("Couldn't register top clock %i\n", i);
> +                       goto err_clk_unreg;
> +               }
> +
> +               hws[i] = &sclk->hw;
> +               mt7621_clk_early[i] = &sclk->hw;
> +       }
> +
> +       for (j = i; j < MT7621_CLK_MAX; j++)
> +               mt7621_clk_early[j] = ERR_PTR(-EPROBE_DEFER);
> +
> +       return 0;
> +
> +err_clk_unreg:
> +       while (--i >= 0) {
> +               struct mt7621_clk *sclk = &mt7621_clks_base[i];

Please move sclk to the toplevel of this function instead of having it
twice.

> +
> +               clk_hw_unregister(&sclk->hw);
> +       }
> +       return ret;
> +}
> +
> +static int mt7621_prepare_enable_clocks(struct clk_hw_onecell_data *clk_data)
> +{
> +       int ret, i;
> +
> +       for (i = 0; i < MT7621_CLK_MAX; i++) {
> +               ret = clk_prepare_enable(clk_data->hws[i]->clk);

Are these critical clks? Why not use the CLK_IS_CRITICAL flag?

> +               if (ret) {
> +                       pr_err("failed to enable clk: %d\n", ret);
> +                       goto err_clk_disable;
> +               }
> +       }
> +
> +       return 0;
> +
> +err_clk_disable:
> +       while (--i >= 0)
> +               clk_disable_unprepare(clk_data->hws[i]->clk);
> +       return ret;
> +}
> +
> +static void __init mt7621_clk_init(struct device_node *node)
> +{
> +       struct mt7621_clk_priv *priv;
> +       struct clk_hw_onecell_data *clk_data;
> +       int ret, i, count;
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return;
> +
> +       priv->sysc = syscon_node_to_regmap(node);
> +       if (IS_ERR(priv->sysc)) {
> +               pr_err("Could not get sysc syscon regmap\n");
> +               goto free_clk_priv;
> +       }
> +
> +       priv->memc = syscon_regmap_lookup_by_phandle(node, "ralink,memctl");
> +       if (IS_ERR(priv->memc)) {
> +               pr_err("Could not get memc syscon regmap\n");
> +               goto free_clk_priv;
> +       }
> +
> +       count = ARRAY_SIZE(mt7621_clks_base) +
> +               ARRAY_SIZE(mt7621_fixed_clks) + ARRAY_SIZE(mt7621_gates);
> +       clk_data = kzalloc(struct_size(clk_data, hws, count), GFP_KERNEL);
> +       if (!clk_data)
> +               goto free_clk_priv;
> +
> +       ret = mt7621_register_early_clocks(node, clk_data, priv);
> +       if (ret) {
> +               pr_err("Couldn't register top clocks\n");
> +               goto free_clk_data;
> +       }
> +
> +       clk_data->num = count;
> +
> +       ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
> +       if (ret) {
> +               pr_err("Couldn't add clk hw provider\n");
> +               goto unreg_clk_top;
> +       }
> +
> +       return;
> +
> +unreg_clk_top:
> +       for (i = 0; i < ARRAY_SIZE(mt7621_clks_base); i++) {
> +               struct mt7621_clk *sclk = &mt7621_clks_base[i];
> +
> +               clk_hw_unregister(&sclk->hw);
> +       }
> +
> +free_clk_data:
> +       kfree(clk_data);
> +
> +free_clk_priv:
> +       kfree(priv);
> +}
> +CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init);
> +
> +static int mt7621_clk_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct clk_hw_onecell_data *clk_data;
> +       struct device *dev = &pdev->dev;
> +       struct mt7621_clk_priv *priv;
> +       int ret, i, count;
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);

Can we use devm_* APIs here?

> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->sysc = syscon_node_to_regmap(np);
> +       if (IS_ERR(priv->sysc)) {
> +               ret = PTR_ERR(priv->sysc);
> +               dev_err(dev, "Could not get sysc syscon regmap\n");
> +               goto free_clk_priv;
> +       }
> +
> +       priv->memc = syscon_regmap_lookup_by_phandle(np, "ralink,memctl");
> +       if (IS_ERR(priv->memc)) {
> +               ret = PTR_ERR(priv->memc);
> +               dev_err(dev, "Could not get memc syscon regmap\n");
> +               goto free_clk_priv;
> +       }
> +
> +       count = ARRAY_SIZE(mt7621_clks_base) +
> +               ARRAY_SIZE(mt7621_fixed_clks) + ARRAY_SIZE(mt7621_gates);
> +       clk_data = kzalloc(struct_size(clk_data, hws, count), GFP_KERNEL);
> +       if (!clk_data) {
> +               ret = -ENOMEM;
> +               goto free_clk_priv;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(mt7621_clks_base); i++)
> +               clk_data->hws[i] = mt7621_clk_early[i];
> +
> +       ret = mt7621_register_fixed_clocks(dev, clk_data);
> +       if (ret) {
> +               dev_err(dev, "Couldn't register fixed clocks\n");
> +               goto free_clk_data;
> +       }
> +
> +       ret = mt7621_register_gates(dev, clk_data, priv);
> +       if (ret) {
> +               dev_err(dev, "Couldn't register fixed clock gates\n");
> +               goto unreg_clk_fixed;
> +       }
> +
> +       clk_data->num = count;
> +
> +       ret = mt7621_prepare_enable_clocks(clk_data);
> +       if (ret) {
> +               dev_err(dev, "Couldn't register fixed clock gates\n");

This isn't registering fixed clock gates though?

> +               goto unreg_clk_gates;
> +       }
> +
> +       ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> +       if (ret) {
> +               dev_err(dev, "Couldn't add clk hw provider\n");
> +               goto disable_clks;
> +       }
> +
> +       return 0;
> +
> +disable_clks:
> +       for (i = 0; i < MT7621_CLK_MAX; i++)
> +               clk_disable_unprepare(clk_data->hws[i]->clk);
> +
> +unreg_clk_gates:
> +       for (i = 0; i < ARRAY_SIZE(mt7621_gates); i++) {
> +               struct mt7621_gate *sclk = &mt7621_gates[i];
> +
> +               clk_hw_unregister(&sclk->hw);
> +       }
> +
> +unreg_clk_fixed:
> +       for (i = 0; i < ARRAY_SIZE(mt7621_fixed_clks); i++) {
> +               struct mt7621_fixed_clk *sclk = &mt7621_fixed_clks[i];
> +
> +               clk_hw_unregister_fixed_rate(sclk->hw);
> +       }
> +
> +free_clk_data:
> +       kfree(clk_data);
> +
> +free_clk_priv:
> +       kfree(priv);
> +
> +       return ret;
> +}
> +
> +static const struct of_device_id mt7621_clk_of_match[] = {
> +       { .compatible = "mediatek,mt7621-sysc" },
> +       {},

Nitpick: Drop the comma on the sentinel so it's a compile error if
another element is added after.

> +};
> +
> +static struct platform_driver mt7621_clk_driver = {
> +       .probe = mt7621_clk_probe,
> +       .driver = {
> +               .name = "mt7621-clk",
> +               .of_match_table = mt7621_clk_of_match,
> +       },
> +};
> +builtin_platform_driver(mt7621_clk_driver);