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

From: Stephen Boyd
Date: Thu Apr 13 2023 - 14:55:40 EST

Next message: Daniel Vetter: "Re: [PATCH] drm/fbdev-generic: fix potential out-of-bounds access"
Previous message: Adrien Thierry: "Re: [PATCH v6 0/8] Add multiport support for DWC3 controllers"
Next in thread: Sergio Paracuellos: "Re: [PATCH v2 2/9] clk: ralink: add clock and reset driver for MTMIPS SoCs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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;
> +}
> +

Next message: Daniel Vetter: "Re: [PATCH] drm/fbdev-generic: fix potential out-of-bounds access"
Previous message: Adrien Thierry: "Re: [PATCH v6 0/8] Add multiport support for DWC3 controllers"
Next in thread: Sergio Paracuellos: "Re: [PATCH v2 2/9] clk: ralink: add clock and reset driver for MTMIPS SoCs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]