Re: [PATCH v2 2/9] clk: ralink: add clock and reset driver for MTMIPS SoCs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Sergio Paracuellos (2023-03-20 22:00:27)
> diff --git a/drivers/clk/ralink/clk-mtmips.c b/drivers/clk/ralink/clk-mtmips.c
> new file mode 100644
> index 000000000000..6b4b5ae9384d
> --- /dev/null
> +++ b/drivers/clk/ralink/clk-mtmips.c
> @@ -0,0 +1,985 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MTMIPS SoCs Clock Driver
> + * Author: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>

Drop unused include.

> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +#include <linux/slab.h>
> +
[..]
> +
> +/*
> + * There are drivers for these SoCs that are older than clock driver
> + * and are not prepared for the clock. We don't want the kernel to
> + * disable anything so we add CLK_IS_CRITICAL flag here.
> + */
> +#define CLK_PERIPH(_name, _parent) {                           \
> +       .init = &(struct clk_init_data) {                       \

const?

> +               .name = _name,                                  \
> +               .ops = &(const struct clk_ops) {                \

Make this into a named variable? Otherwise I suspect the compiler will
want to duplicate it.

> +                       .recalc_rate = mtmips_pherip_clk_rate   \
> +               },                                              \
> +               .parent_data = &(const struct clk_parent_data) {\
> +                       .name = _parent,                        \
> +                       .fw_name = _parent                      \
> +               },                                              \
> +               .num_parents = 1,                               \
> +               .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL  \

Why is everything critical? Put the comment here instead of above the
macro

> +       },                                                      \
> +}
> +
[...]
> +
> +static int mtmips_register_pherip_clocks(struct device_node *np,
> +                                        struct clk_hw_onecell_data *clk_data,
> +                                        struct mtmips_clk_priv *priv)
> +{
> +       struct clk_hw **hws = clk_data->hws;
> +       struct mtmips_clk *sclk;
> +       int ret, i;
> +
> +       for (i = 0; i < priv->data->num_clk_periph; i++) {
> +               int idx = (priv->data->num_clk_base - 1) + i;
> +
> +               sclk = &priv->data->clk_periph[i];
> +               ret = of_clk_hw_register(np, &sclk->hw);
> +               if (ret) {
> +                       pr_err("Couldn't register peripheral clock %d\n", idx);
> +                       goto err_clk_unreg;
> +               }
> +
> +               hws[idx] = &sclk->hw;
> +       }
> +
> +       return 0;
> +
> +err_clk_unreg:
> +       while (--i >= 0) {
> +               sclk = &priv->data->clk_periph[i];
> +               clk_hw_unregister(&sclk->hw);
> +       }
> +       return ret;
> +}
> +
> +static inline struct mtmips_clk *to_mtmips_clk(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct mtmips_clk, hw);
> +}
> +
> +static unsigned long rt5350_xtal_recalc_rate(struct clk_hw *hw,
> +                                            unsigned long parent_rate)
> +{
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 val;
> +
> +       regmap_read(sysc, SYSC_REG_SYSTEM_CONFIG, &val);
> +       if (!(val & RT5350_CLKCFG0_XTAL_SEL))
> +               return 20000000;
> +
> +       return 40000000;
> +}
> +
> +static unsigned long rt5350_cpu_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long xtal_clk)
> +{
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 t;
> +
> +       regmap_read(sysc, SYSC_REG_SYSTEM_CONFIG, &t);
> +       t = (t >> RT5350_SYSCFG0_CPUCLK_SHIFT) & RT5350_SYSCFG0_CPUCLK_MASK;
> +
> +       switch (t) {
> +       case RT5350_SYSCFG0_CPUCLK_360:
> +               return 360000000;
> +       case RT5350_SYSCFG0_CPUCLK_320:
> +               return 320000000;
> +       case RT5350_SYSCFG0_CPUCLK_300:
> +               return 300000000;
> +       default:
> +               BUG();
> +       }
> +}
> +
> +static unsigned long rt5350_bus_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       if (parent_rate == 320000000)
> +               return parent_rate / 4;
> +
> +       return parent_rate / 3;
> +}
> +
> +static unsigned long rt3352_cpu_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long xtal_clk)
> +{
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 t;
> +
> +       regmap_read(sysc, SYSC_REG_SYSTEM_CONFIG, &t);
> +       t = (t >> RT3352_SYSCFG0_CPUCLK_SHIFT) & RT3352_SYSCFG0_CPUCLK_MASK;
> +
> +       switch (t) {
> +       case RT3352_SYSCFG0_CPUCLK_LOW:
> +               return 384000000;
> +       case RT3352_SYSCFG0_CPUCLK_HIGH:
> +               return 400000000;
> +       default:
> +               BUG();
> +       }
> +}
> +
> +static unsigned long rt3352_periph_recalc_rate(struct clk_hw *hw,
> +                                              unsigned long parent_rate)
> +{
> +       return 40000000;
> +}
> +
> +static unsigned long rt3352_bus_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       return parent_rate / 3;
> +}
> +
> +static unsigned long rt305x_xtal_recalc_rate(struct clk_hw *hw,
> +                                            unsigned long parent_rate)
> +{
> +       return 40000000;
> +}

Register fixed factor and fixed rate clks in software instead of
duplicating the code here.

> +
> +static unsigned long rt305x_cpu_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long xtal_clk)
> +{
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 t;
> +
> +       regmap_read(sysc, SYSC_REG_SYSTEM_CONFIG, &t);
> +       t = (t >> RT305X_SYSCFG_CPUCLK_SHIFT) & RT305X_SYSCFG_CPUCLK_MASK;
> +
> +       switch (t) {
> +       case RT305X_SYSCFG_CPUCLK_LOW:
> +               return 320000000;
> +       case RT305X_SYSCFG_CPUCLK_HIGH:
> +               return 384000000;
> +       default:
> +               BUG();
> +       }
> +}
> +
> +static unsigned long rt3883_cpu_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long xtal_clk)
> +{
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 t;
> +
> +       regmap_read(sysc, SYSC_REG_SYSTEM_CONFIG, &t);
> +       t = (t >> RT3883_SYSCFG0_CPUCLK_SHIFT) & RT3883_SYSCFG0_CPUCLK_MASK;
> +
> +       switch (t) {
> +       case RT3883_SYSCFG0_CPUCLK_250:
> +               return 250000000;
> +       case RT3883_SYSCFG0_CPUCLK_384:
> +               return 384000000;
> +       case RT3883_SYSCFG0_CPUCLK_480:
> +               return 480000000;
> +       case RT3883_SYSCFG0_CPUCLK_500:
> +               return 500000000;
> +       default:
> +               BUG();
> +       }
> +}
> +
> +static unsigned long rt3883_bus_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 ddr2;
> +       u32 t;
> +
> +       regmap_read(sysc, SYSC_REG_SYSTEM_CONFIG, &t);
> +       ddr2 = t & RT3883_SYSCFG0_DRAM_TYPE_DDR2;
> +
> +       switch (parent_rate) {
> +       case 250000000:
> +               return (ddr2) ? 125000000 : 83000000;
> +       case 384000000:
> +               return (ddr2) ? 128000000 : 96000000;
> +       case 480000000:
> +               return (ddr2) ? 160000000 : 120000000;
> +       case 500000000:
> +               return (ddr2) ? 166000000 : 125000000;
> +       default:
> +               BUG();

Why? Depending on clk registration order 'parent_rate' could be 0, and
then this will crash the system.

> +       }
> +}
> +
> +static unsigned long rt2880_cpu_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long xtal_clk)
> +{
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 t;
> +
> +       regmap_read(sysc, SYSC_REG_SYSTEM_CONFIG, &t);
> +       t = (t >> RT2880_CONFIG_CPUCLK_SHIFT) & RT2880_CONFIG_CPUCLK_MASK;
> +
> +       switch (t) {
> +       case RT2880_CONFIG_CPUCLK_250:
> +               return 250000000;
> +       case RT2880_CONFIG_CPUCLK_266:
> +               return 266000000;
> +       case RT2880_CONFIG_CPUCLK_280:
> +               return 280000000;
> +       case RT2880_CONFIG_CPUCLK_300:
> +               return 300000000;
> +       default:
> +               BUG();
> +       }
> +}
> +
> +static unsigned long rt2880_bus_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       return parent_rate / 2;
> +}

A fixed factor clk?

> +
> +static u32 mt7620_calc_rate(u32 ref_rate, u32 mul, u32 div)
> +{
> +       u64 t;
> +
> +       t = ref_rate;
> +       t *= mul;
> +       do_div(t, div);

Do we really need to do 64-bit math? At the least use div_u64().

> +
> +       return t;
> +}
> +
> +static unsigned long mt7620_pll_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       static const u32 clk_divider[] = { 2, 3, 4, 8 };
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       unsigned long cpu_pll;
> +       u32 t;
> +       u32 mul;
> +       u32 div;
> +
> +       regmap_read(sysc, SYSC_REG_CPLL_CONFIG0, &t);
> +       if (t & CPLL_CFG0_BYPASS_REF_CLK) {
> +               cpu_pll = parent_rate;
> +       } else if ((t & CPLL_CFG0_SW_CFG) == 0) {
> +               cpu_pll = 600000000;
> +       } else {
> +               mul = (t >> CPLL_CFG0_PLL_MULT_RATIO_SHIFT) &
> +                       CPLL_CFG0_PLL_MULT_RATIO_MASK;
> +               mul += 24;
> +               if (t & CPLL_CFG0_LC_CURFCK)
> +                       mul *= 2;
> +
> +               div = (t >> CPLL_CFG0_PLL_DIV_RATIO_SHIFT) &
> +                       CPLL_CFG0_PLL_DIV_RATIO_MASK;
> +
> +               WARN_ON(div >= ARRAY_SIZE(clk_divider));

WARN_ON_ONCE() so that this doesn't spam the system.

> +
> +               cpu_pll = mt7620_calc_rate(parent_rate, mul, clk_divider[div]);
> +       }
> +
> +       regmap_read(sysc, SYSC_REG_CPLL_CONFIG1, &t);
> +       if (t & CPLL_CFG1_CPU_AUX1)
> +               return parent_rate;
> +
> +       if (t & CPLL_CFG1_CPU_AUX0)
> +               return 480000000;
> +
> +       return cpu_pll;
> +}
> +
> +static unsigned long mt7620_cpu_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 t;
> +       u32 mul;
> +       u32 div;
> +
> +       regmap_read(sysc, SYSC_REG_CPU_SYS_CLKCFG, &t);
> +       mul = t & CPU_SYS_CLKCFG_CPU_FFRAC_MASK;
> +       div = (t >> CPU_SYS_CLKCFG_CPU_FDIV_SHIFT) &
> +               CPU_SYS_CLKCFG_CPU_FDIV_MASK;
> +
> +       return mt7620_calc_rate(parent_rate, mul, div);
> +}
> +
> +static unsigned long mt7620_bus_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       static const u32 ocp_dividers[16] = {
> +               [CPU_SYS_CLKCFG_OCP_RATIO_2] = 2,
> +               [CPU_SYS_CLKCFG_OCP_RATIO_3] = 3,
> +               [CPU_SYS_CLKCFG_OCP_RATIO_4] = 4,
> +               [CPU_SYS_CLKCFG_OCP_RATIO_5] = 5,
> +               [CPU_SYS_CLKCFG_OCP_RATIO_10] = 10,
> +       };
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 t;
> +       u32 ocp_ratio;
> +       u32 div;
> +
> +       if (IS_ENABLED(CONFIG_USB)) {
> +               /*
> +               * When the CPU goes into sleep mode, the BUS
> +               * clock will be too low for USB to function properly.
> +               * Adjust the busses fractional divider to fix this
> +               */
> +               regmap_read(sysc, SYSC_REG_CPU_SYS_CLKCFG, &t);
> +               t &= ~(CLKCFG_FDIV_MASK | CLKCFG_FFRAC_MASK);
> +               t |= CLKCFG_FDIV_USB_VAL | CLKCFG_FFRAC_USB_VAL;
> +               regmap_write(sysc, SYSC_REG_CPU_SYS_CLKCFG, t);

Why can't we do this unconditionally? And recalc_rate() shouldn't be
writing registers. It should be calculating the frequency of the clk
based on 'parent_rate' and whatever the hardware is configured for.

> +       }
> +
> +       regmap_read(sysc, SYSC_REG_CPU_SYS_CLKCFG, &t);
> +       ocp_ratio = (t >> CPU_SYS_CLKCFG_OCP_RATIO_SHIFT) &
> +               CPU_SYS_CLKCFG_OCP_RATIO_MASK;
> +
> +       if (WARN_ON(ocp_ratio >= ARRAY_SIZE(ocp_dividers)))
> +               return parent_rate;
> +
> +       div = ocp_dividers[ocp_ratio];
> +       if (WARN(!div, "invalid divider for OCP ratio %u", ocp_ratio))

Missing newline.

> +               return parent_rate;
> +
> +       return parent_rate / div;
> +}
> +
> +static unsigned long mt7620_periph_recalc_rate(struct clk_hw *hw,
> +                                              unsigned long parent_rate)
> +{
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 t;
> +
> +       regmap_read(sysc, SYSC_REG_CLKCFG0, &t);
> +       if (t & CLKCFG0_PERI_CLK_SEL)
> +               return parent_rate;
> +
> +       return 40000000;
> +}
> +
> +static unsigned long mt76x8_xtal_recalc_rate(struct clk_hw *hw,
> +                                            unsigned long parent_rate)
> +{
> +       struct mtmips_clk *clk = to_mtmips_clk(hw);
> +       struct regmap *sysc = clk->priv->sysc;
> +       u32 t;
> +
> +       regmap_read(sysc, SYSC_REG_SYSTEM_CONFIG, &t);
> +       if (t & MT7620_XTAL_FREQ_SEL)
> +               return 40000000;
> +
> +       return 20000000;
> +}
> +
> +static unsigned long mt76x8_cpu_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long xtal_clk)
> +{
> +       if (xtal_clk == 40000000)
> +               return 580000000;
> +
> +       return 575000000;
> +}
> +
> +static unsigned long mt76x8_pcmi2s_recalc_rate(struct clk_hw *hw,
> +                                              unsigned long xtal_clk)
> +{
> +       return 480000000;
> +}

Use fixed rate clk.

> +
> +#define CLK_BASE(_name, _parent, _recalc) {                            \
> +       .init = &(struct clk_init_data) {                               \

const

> +               .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 void __init mtmips_clk_init(struct device_node *node)
> +{
> +       const struct of_device_id *match;
> +       const struct mtmips_clk_data *data;
> +       struct mtmips_clk_priv *priv;
> +       struct clk_hw_onecell_data *clk_data;
> +       int ret, i, count;
> +
> +       if (!of_find_property(node, "#clock-cells", NULL)) {
> +               pr_err("No '#clock-cells' property found\n");

We don't need to validate the bindings in the driver.

> +               return;
> +       }
> +
> +       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;
> +       }
> +
> +       match = of_match_node(mtmips_of_match, node);
> +       if (WARN_ON(!match))
> +               return;
> +
> +       data = match->data;
> +       priv->data = data;
> +       count = priv->data->num_clk_base + priv->data->num_clk_periph;
> +       clk_data = kzalloc(struct_size(clk_data, hws, count), GFP_KERNEL);
> +       if (!clk_data)
> +               goto free_clk_priv;
> +
> +       ret = mtmips_register_clocks(node, clk_data, priv);
> +       if (ret) {
> +               pr_err("Couldn't register top clocks\n");
> +               goto free_clk_data;
> +       }
> +
> +       ret = mtmips_register_pherip_clocks(node, clk_data, priv);
> +       if (ret) {
> +               pr_err("Couldn't register peripheral clocks\n");
> +               goto unreg_clk_top;
> +       }
> +
> +       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_periph;
> +       }
> +
> +       return;
> +
> +unreg_clk_periph:
> +       for (i = 0; i < priv->data->num_clk_periph; i++) {
> +               struct mtmips_clk *sclk = &priv->data->clk_periph[i];
> +
> +               clk_hw_unregister(&sclk->hw);
> +       }
> +
> +unreg_clk_top:
> +       for (i = 0; i < priv->data->num_clk_base; i++) {
> +               struct mtmips_clk *sclk = &priv->data->clk_base[i];
> +
> +               clk_hw_unregister(&sclk->hw);
> +       }
> +
> +free_clk_data:
> +       kfree(clk_data);
> +
> +free_clk_priv:
> +       kfree(priv);
> +}
> +CLK_OF_DECLARE_DRIVER(rt2880_clk, "ralink,rt2880-sysc", mtmips_clk_init);
> +CLK_OF_DECLARE_DRIVER(rt3050_clk, "ralink,rt3050-sysc", mtmips_clk_init);
> +CLK_OF_DECLARE_DRIVER(rt3052_clk, "ralink,rt3052-sysc", mtmips_clk_init);
> +CLK_OF_DECLARE_DRIVER(rt3352_clk, "ralink,rt3352-sysc", mtmips_clk_init);
> +CLK_OF_DECLARE_DRIVER(rt3883_clk, "ralink,rt3883-sysc", mtmips_clk_init);
> +CLK_OF_DECLARE_DRIVER(rt5350_clk, "ralink,rt5350-sysc", mtmips_clk_init);
> +CLK_OF_DECLARE_DRIVER(mt7620_clk, "ralink,mt7620-sysc", mtmips_clk_init);
> +CLK_OF_DECLARE_DRIVER(mt7620a_clk, "ralink,mt7620a-sysc", mtmips_clk_init);
> +CLK_OF_DECLARE_DRIVER(mt7628_clk, "ralink,mt7628-sysc", mtmips_clk_init);
> +CLK_OF_DECLARE_DRIVER(mt7688_clk, "ralink,mt7688-sysc", mtmips_clk_init);

Is there any reason why these can't be platform drivers?

> +
[..]
> +
> +static int mtmips_clk_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct device *dev = &pdev->dev;
> +       struct mtmips_clk_priv *priv;
> +       int ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       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");

Use dev_err_probe()?

> +               return ret;
> +       }
> +
> +       ret = mtmips_reset_init(dev, priv->sysc);
> +       if (ret) {
> +               dev_err(dev, "Could not init reset controller\n");

Use dev_err_probe()?

> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux