Re: [PATCH v4 08/25] clk: mtmips: add clock driver for MediaTek MT7621 SoC

Hi Weijie,

I found a copy of the MT7260 programming guide, which I am using as a reference. I assume that they have not diverged too much, but presumably most discrepencies will be accounted for in the difference in part numbers.

On 5/10/22 8:18 AM, Weijie Gao wrote:

This patch adds a clock driver for MediaTek MT7621 SoC. This driver provides clock gate control as well as getting clock frequency for CPU/SYS/XTAL and some peripherals.

Signed-off-by: Weijie Gao weijie.gao@mediatek.com

v4 changes: none v3 changes: update clock definitions to match upstream kernel v2 changes: none

---

drivers/clk/mtmips/Makefile | 1 + drivers/clk/mtmips/clk-mt7621.c | 315 +++++++++++++++++++++++++ include/dt-bindings/clock/mt7621-clk.h | 46 ++++ 3 files changed, 362 insertions(+) create mode 100644 drivers/clk/mtmips/clk-mt7621.c create mode 100644 include/dt-bindings/clock/mt7621-clk.h

diff --git a/drivers/clk/mtmips/Makefile b/drivers/clk/mtmips/Makefile index 732e7f2545..ee8b5afe87 100644 --- a/drivers/clk/mtmips/Makefile +++ b/drivers/clk/mtmips/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0

obj-$(CONFIG_SOC_MT7620) += clk-mt7620.o +obj-$(CONFIG_SOC_MT7621) += clk-mt7621.o obj-$(CONFIG_SOC_MT7628) += clk-mt7628.o diff --git a/drivers/clk/mtmips/clk-mt7621.c b/drivers/clk/mtmips/clk-mt7621.c new file mode 100644 index 0000000000..e8203016a5 --- /dev/null +++ b/drivers/clk/mtmips/clk-mt7621.c @@ -0,0 +1,315 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2022 MediaTek Inc. All rights reserved.
• • Author: Weijie Gao weijie.gao@mediatek.com
• */

+#include <common.h> +#include <clk-uclass.h> +#include <dm.h> +#include <dm/device_compat.h> +#include <regmap.h> +#include <syscon.h> +#include <dt-bindings/clock/mt7621-clk.h> +#include <linux/bitops.h> +#include <linux/io.h>

+#define SYSC_MAP_SIZE 0x100 +#define MEMC_MAP_SIZE 0x1000

+/* SYSC */ +#define SYSCFG0_REG 0x10 +#define XTAL_MODE_SEL_S 6 +#define XTAL_MODE_SEL_M 0x1c0

Please use GENMASK and BIT for these defines.

+#define CLKCFG0_REG 0x2c +#define CPU_CLK_SEL_S 30 +#define CPU_CLK_SEL_M 0xc0000000 +#define PERI_CLK_SEL 0x10

+#define CLKCFG1_REG 0x30

+#define CUR_CLK_STS_REG 0x44 +#define CUR_CPU_FDIV_S 8 +#define CUR_CPU_FDIV_M 0x1f00 +#define CUR_CPU_FFRAC_S 0 +#define CUR_CPU_FFRAC_M 0x1f

+/* MEMC */ +#define MEMPLL1_REG 0x0604 +#define RG_MEPL_DIV2_SEL_S 1 +#define RG_MEPL_DIV2_SEL_M 0x06

+#define MEMPLL6_REG 0x0618 +#define MEMPLL18_REG 0x0648 +#define RG_MEPL_PREDIV_S 12 +#define RG_MEPL_PREDIV_M 0x3000 +#define RG_MEPL_FBDIV_S 4 +#define RG_MEPL_FBDIV_M 0x7f0

+/* EPLL clock */ +#define EPLL_CLK 50000000

+struct mt7621_clk_priv {

• void __iomem *sysc_base;
• void __iomem *memc_base;
• int cpu_clk;
• int ddr_clk;
• int sys_clk;
• int xtal_clk;

+};

+enum mt7621_clk_src {

• CLK_SRC_CPU,
• CLK_SRC_DDR,
• CLK_SRC_SYS,
• CLK_SRC_XTAL,
• CLK_SRC_PERI,
• CLK_SRC_125M,
• CLK_SRC_150M,
• CLK_SRC_250M,
• CLK_SRC_270M,
• __CLK_SRC_MAX

+};

+struct mt7621_clk_map {

• u32 cgbit;
• enum mt7621_clk_src clksrc;

+};

+#define CLK_MAP(_id, _cg, _src) \

• [_id] = { .cgbit = (_cg), .clksrc = (_src) }

+#define CLK_MAP_SRC(_id, _src) \

• [_id] = { .cgbit = UINT32_MAX, .clksrc = (_src) }

+static const struct mt7621_clk_map mt7621_clk_mappings[] = {

• CLK_MAP_SRC(MT7621_CLK_XTAL, CLK_SRC_XTAL),
• CLK_MAP_SRC(MT7621_CLK_CPU, CLK_SRC_CPU),
• CLK_MAP_SRC(MT7621_CLK_BUS, CLK_SRC_SYS),
• CLK_MAP_SRC(MT7621_CLK_50M, CLK_SRC_PERI),
• CLK_MAP_SRC(MT7621_CLK_125M, CLK_SRC_125M),
• CLK_MAP_SRC(MT7621_CLK_150M, CLK_SRC_150M),
• CLK_MAP_SRC(MT7621_CLK_250M, CLK_SRC_250M),
• CLK_MAP_SRC(MT7621_CLK_270M, CLK_SRC_270M),
• CLK_MAP(MT7621_CLK_HSDMA, 5, CLK_SRC_150M),
• CLK_MAP(MT7621_CLK_FE, 6, CLK_SRC_250M),
• CLK_MAP(MT7621_CLK_SP_DIVTX, 7, CLK_SRC_270M),
• CLK_MAP(MT7621_CLK_TIMER, 8, CLK_SRC_PERI),
• CLK_MAP(MT7621_CLK_PCM, 11, CLK_SRC_270M),
• CLK_MAP(MT7621_CLK_PIO, 13, CLK_SRC_PERI),
• CLK_MAP(MT7621_CLK_GDMA, 14, CLK_SRC_SYS),
• CLK_MAP(MT7621_CLK_NAND, 15, CLK_SRC_125M),
• CLK_MAP(MT7621_CLK_I2C, 16, CLK_SRC_PERI),
• CLK_MAP(MT7621_CLK_I2S, 17, CLK_SRC_270M),

Should this be PERI?

• CLK_MAP(MT7621_CLK_SPI, 18, CLK_SRC_SYS),
• CLK_MAP(MT7621_CLK_UART1, 19, CLK_SRC_PERI),
• CLK_MAP(MT7621_CLK_UART2, 20, CLK_SRC_PERI),
• CLK_MAP(MT7621_CLK_UART3, 21, CLK_SRC_PERI),
• CLK_MAP(MT7621_CLK_ETH, 23, CLK_SRC_PERI),
• CLK_MAP(MT7621_CLK_PCIE0, 24, CLK_SRC_125M),
• CLK_MAP(MT7621_CLK_PCIE1, 25, CLK_SRC_125M),
• CLK_MAP(MT7621_CLK_PCIE2, 26, CLK_SRC_125M),
• CLK_MAP(MT7621_CLK_CRYPTO, 29, CLK_SRC_250M),
• CLK_MAP(MT7621_CLK_SHXC, 30, CLK_SRC_PERI),
• CLK_MAP_SRC(MT7621_CLK_MAX, __CLK_SRC_MAX),
• CLK_MAP_SRC(MT7621_CLK_DDR, CLK_SRC_DDR),

+};

+static ulong mt7621_clk_get_rate(struct clk *clk) +{

• struct mt7621_clk_priv *priv = dev_get_priv(clk->dev);
• u32 val;
• if (clk->id >= ARRAY_SIZE(mt7621_clk_mappings))

• return 0;
  

Do this check in request().

• switch (mt7621_clk_mappings[clk->id].clksrc) {
• case CLK_SRC_CPU:

• return priv->cpu_clk;
  

• case CLK_SRC_DDR:

• return priv->ddr_clk;
  

• case CLK_SRC_SYS:

• return priv->sys_clk;
  

• case CLK_SRC_XTAL:

• return priv->xtal_clk;
  

• case CLK_SRC_PERI:

• val = readl(priv->sysc_base + CLKCFG0_REG);
  

• if (val & PERI_CLK_SEL)
  

• 	return priv->xtal_clk;
  

• else
  

• 	return EPLL_CLK;
  

Shouldn't this be a proper clock like the CLK_SRC_XXXMs? In the 7620 this is (BBP PLL)/12 (aka the parent of PCM_480). I assume that this setup is different on this SoC?

• case CLK_SRC_125M:

• return 125000000;
  

• case CLK_SRC_150M:

• return 150000000;
  

• case CLK_SRC_250M:

• return 250000000;
  

• case CLK_SRC_270M:

• return 270000000;
  

• default:

• return 0;
  

• }

+}

+static int mt7621_clk_enable(struct clk *clk) +{

• struct mt7621_clk_priv *priv = dev_get_priv(clk->dev);
• u32 cgbit;
• if (clk->id >= ARRAY_SIZE(mt7621_clk_mappings))

• return -1;
  

Please use a real error code (e.g. ENOENT)

• cgbit = mt7621_clk_mappings[clk->id].cgbit;
• if (cgbit == UINT32_MAX)

• return -1;
  

return -ENOSYS

• setbits_32(priv->sysc_base + CLKCFG1_REG, BIT(cgbit));
• return 0;

+}

+static int mt7621_clk_disable(struct clk *clk) +{

• struct mt7621_clk_priv *priv = dev_get_priv(clk->dev);
• u32 cgbit;
• if (clk->id >= ARRAY_SIZE(mt7621_clk_mappings))

• return -1;
  

• cgbit = mt7621_clk_mappings[clk->id].cgbit;
• if (cgbit == UINT32_MAX)

• return -1;
  

• clrbits_32(priv->sysc_base + CLKCFG1_REG, BIT(cgbit));
• return 0;

+}

ditto

+const struct clk_ops mt7621_clk_ops = {

• .enable = mt7621_clk_enable,
• .disable = mt7621_clk_disable,
• .get_rate = mt7621_clk_get_rate,

+};

+static void mt7621_get_clocks(struct mt7621_clk_priv *priv) +{

• u32 bs, xtal_sel, clkcfg0, cur_clk, mempll, dividx, fb;
• u32 xtal_clk, xtal_div, ffiv, ffrac, cpu_clk, ddr_clk;
• static const u32 xtal_div_tbl[] = {0, 1, 2, 2};
• bs = readl(priv->sysc_base + SYSCFG0_REG);
• clkcfg0 = readl(priv->sysc_base + CLKCFG0_REG);
• cur_clk = readl(priv->sysc_base + CUR_CLK_STS_REG);
• xtal_sel = (bs & XTAL_MODE_SEL_M) >> XTAL_MODE_SEL_S;
• if (xtal_sel <= 2)

• xtal_clk = 20 * 1000 * 1000;
  

• else if (xtal_sel <= 5)

• xtal_clk = 40 * 1000 * 1000;
  

• else

• xtal_clk = 25 * 1000 * 1000;
  

• switch ((clkcfg0 & CPU_CLK_SEL_M) >> CPU_CLK_SEL_S) {
• case 0:

• cpu_clk = 500 * 1000 * 1000;
  

• break;
  

• case 1:

• mempll = readl(priv->memc_base + MEMPLL18_REG);
  

• dividx = (mempll & RG_MEPL_PREDIV_M) >> RG_MEPL_PREDIV_S;
  

• fb = (mempll & RG_MEPL_FBDIV_M) >> RG_MEPL_FBDIV_S;
  

• xtal_div = 1 << xtal_div_tbl[dividx];
  

• cpu_clk = (fb + 1) * xtal_clk / xtal_div;
  

• break;
  

• default:

• cpu_clk = xtal_clk;
  

• }
• ffiv = (cur_clk & CUR_CPU_FDIV_M) >> CUR_CPU_FDIV_S;
• ffrac = (cur_clk & CUR_CPU_FFRAC_M) >> CUR_CPU_FFRAC_S;
• cpu_clk = cpu_clk / ffiv * ffrac;
• mempll = readl(priv->memc_base + MEMPLL6_REG);
• dividx = (mempll & RG_MEPL_PREDIV_M) >> RG_MEPL_PREDIV_S;
• fb = (mempll & RG_MEPL_FBDIV_M) >> RG_MEPL_FBDIV_S;
• xtal_div = 1 << xtal_div_tbl[dividx];
• ddr_clk = fb * xtal_clk / xtal_div;
• bs = readl(priv->memc_base + MEMPLL1_REG);
• if (((bs & RG_MEPL_DIV2_SEL_M) >> RG_MEPL_DIV2_SEL_S) == 0)

• ddr_clk *= 2;
  

• priv->cpu_clk = cpu_clk;
• priv->sys_clk = cpu_clk / 4;
• priv->ddr_clk = ddr_clk;
• priv->xtal_clk = xtal_clk;

+}

I would generally like to see these calculations done in get_rate (to allow for set_rate), but this style is fine .

+static int mt7621_clk_probe(struct udevice *dev) +{

• struct mt7621_clk_priv *priv = dev_get_priv(dev);
• struct ofnode_phandle_args args;
• struct regmap *regmap;
• void __iomem *base;
• int ret;
• /* get corresponding sysc phandle */
• ret = dev_read_phandle_with_args(dev, "mediatek,sysc", NULL, 0, 0,

• 			 &args);
  

• if (ret)

• return ret;
  

I think it might be better to have this node be a child of the sysc. Like

sysc: sysc@1e000000 { compatible = "mediatek,mt7621-sysc", "syscon"; reg = <0x1e000000 0x100>; u-boot,dm-pre-reloc;

clkctrl: clock-controller { compatible = "mediatek,mt7621-clk"; mediatek,memc = <&memc>; #clock-cells = <1>; };

rstctrl: reset-controller { compatible = "mediatek,mtmips-reset"; #reset-cells = <1>; };

reboot { compatible = "resetctl-reboot";

resets = <&rstctrl RST_SYS>; reset-names = "sysreset"; }; };

And then do something like

base = dev_read_addr_ptr(dev_get_parent(dev)); if (!priv->base) return -EINVAL;

• regmap = syscon_node_to_regmap(args.node);
• if (IS_ERR(regmap))

• return PTR_ERR(regmap);
  

• base = regmap_get_range(regmap, 0);
• if (!base) {

• dev_err(dev, "Unable to find sysc\n");
  

• return -ENODEV;
  

• }
• priv->sysc_base = ioremap_nocache((phys_addr_t)base, SYSC_MAP_SIZE);
• /* get corresponding memc phandle */
• ret = dev_read_phandle_with_args(dev, "mediatek,memc", NULL, 0, 0,

• 			 &args);
  

• if (ret)

• return ret;
  

• regmap = syscon_node_to_regmap(args.node);
• if (IS_ERR(regmap))

• return PTR_ERR(regmap);
  

• base = regmap_get_range(regmap, 0);
• if (!base) {

• dev_err(dev, "Unable to find memc\n");
  

• return -ENODEV;
  

• }
• priv->memc_base = ioremap_nocache((phys_addr_t)base, MEMC_MAP_SIZE);

Why not just use the regmap api?

• mt7621_get_clocks(priv);
• return 0;

+}

+static const struct udevice_id mt7621_clk_ids[] = {

• { .compatible = "mediatek,mt7621-clk" },
• { }

+};

+U_BOOT_DRIVER(mt7621_clk) = {

• .name = "mt7621-clk",
• .id = UCLASS_CLK,
• .of_match = mt7621_clk_ids,
• .probe = mt7621_clk_probe,
• .priv_auto = sizeof(struct mt7621_clk_priv),
• .ops = &mt7621_clk_ops,

+}; diff --git a/include/dt-bindings/clock/mt7621-clk.h b/include/dt-bindings/clock/mt7621-clk.h new file mode 100644 index 0000000000..978c67951b --- /dev/null +++ b/include/dt-bindings/clock/mt7621-clk.h @@ -0,0 +1,46 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2022 MediaTek Inc. All rights reserved.
• • Author: Weijie Gao weijie.gao@mediatek.com
• */

+#ifndef _DT_BINDINGS_MT7621_CLK_H_ +#define _DT_BINDINGS_MT7621_CLK_H_

+#define MT7621_CLK_XTAL 0 +#define MT7621_CLK_CPU 1 +#define MT7621_CLK_BUS 2 +#define MT7621_CLK_50M 3 +#define MT7621_CLK_125M 4 +#define MT7621_CLK_150M 5 +#define MT7621_CLK_250M 6 +#define MT7621_CLK_270M 7

+#define MT7621_CLK_HSDMA 8 +#define MT7621_CLK_FE 9 +#define MT7621_CLK_SP_DIVTX 10 +#define MT7621_CLK_TIMER 11 +#define MT7621_CLK_PCM 12 +#define MT7621_CLK_PIO 13 +#define MT7621_CLK_GDMA 14 +#define MT7621_CLK_NAND 15 +#define MT7621_CLK_I2C 16 +#define MT7621_CLK_I2S 17 +#define MT7621_CLK_SPI 18 +#define MT7621_CLK_UART1 19 +#define MT7621_CLK_UART2 20 +#define MT7621_CLK_UART3 21 +#define MT7621_CLK_ETH 22 +#define MT7621_CLK_PCIE0 23 +#define MT7621_CLK_PCIE1 24 +#define MT7621_CLK_PCIE2 25 +#define MT7621_CLK_CRYPTO 26 +#define MT7621_CLK_SHXC 27

+#define MT7621_CLK_MAX 28

+/* for u-boot only */ +#define MT7621_CLK_DDR 29

+#endif /* _DT_BINDINGS_MT7621_CLK_H_ */

--Sean