On Fri, 2021-11-26 at 12:44 -0500, Sean Anderson wrote:
> On 11/18/21 8:35 PM, 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....@mediatek.com>
> > ---
> > v2 changes: none
> > ---
> > drivers/clk/mtmips/Makefile | 1 +
> > drivers/clk/mtmips/clk-mt7621.c | 260
> > +++++++++++++++++++++++++
> > include/dt-bindings/clock/mt7621-clk.h | 42 ++++
> > 3 files changed, 303 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..3799d1806a
> > --- /dev/null
> > +++ b/drivers/clk/mtmips/clk-mt7621.c
> > @@ -0,0 +1,260 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2021 MediaTek Inc. All Rights Reserved.
> > + *
> > + * Author: Weijie Gao <weijie....@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 to define this:
>
> #define XTAL_MODE_SEL_M GENMASK(8, 6)
>
> and SEL_S is unnecessary, see commentary below.
>
> > +
> > +#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
> > +
> > +/* Clock sources */
> > +#define CLK_SRC_CPU -1
> > +#define CLK_SRC_CPU_D2 -2
> > +#define CLK_SRC_DDR -3
> > +#define CLK_SRC_SYS -4
> > +#define CLK_SRC_XTAL -5
> > +#define CLK_SRC_PERI -6
>
> Please use an enum. And why are these negative?
I'll rewrite this
>
> > +/* 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;
> > +};
> > +
> > +static const int mt7621_clks[] = {
> > + [CLK_SYS] = CLK_SRC_SYS,
> > + [CLK_DDR] = CLK_SRC_DDR,
> > + [CLK_CPU] = CLK_SRC_CPU,
> > + [CLK_XTAL] = CLK_SRC_XTAL,
> > + [CLK_MIPS_CNT] = CLK_SRC_CPU_D2,
> > + [CLK_UART3] = CLK_SRC_PERI,
> > + [CLK_UART2] = CLK_SRC_PERI,
> > + [CLK_UART1] = CLK_SRC_PERI,
> > + [CLK_SPI] = CLK_SRC_SYS,
> > + [CLK_I2C] = CLK_SRC_PERI,
> > + [CLK_TIMER] = CLK_SRC_PERI,
> > +};
> > +
> > +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_clks))
> > + return 0;
> > +
> > + switch (mt7621_clks[clk->id]) {
> > + case CLK_SRC_CPU:
> > + return priv->cpu_clk;
> > + case CLK_SRC_CPU_D2:
> > + return priv->cpu_clk / 2;
> > + 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;
> > + default:
> > + return 0;
>
> -ENOSYS
>
> > + }
> > +}
> > +
> > +static int mt7621_clk_enable(struct clk *clk)
> > +{
> > + struct mt7621_clk_priv *priv = dev_get_priv(clk->dev);
> > +
> > + if (clk->id > 31)
>
> Please compare with a symbol.
OK. actually the clock gate register is 32-bit, and 31 is the MSB.
>
> > + return -1;
>
> -ENOSYS
>
> > +
> > + setbits_32(priv->sysc_base + CLKCFG1_REG, BIT(clk->id));
> > +
> > + return 0;
> > +}
> > +
> > +static int mt7621_clk_disable(struct clk *clk)
> > +{
> > + struct mt7621_clk_priv *priv = dev_get_priv(clk->dev);
> > +
> > + if (clk->id > 31)
> > + return -1;
> > +
> > + clrbits_32(priv->sysc_base + CLKCFG1_REG, BIT(clk->id));
> > +
> > + return 0;
> > +}
> > +
> > +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;
>
> xtal_sel = FIELD_GET(XTAL_MODE_SEL_M, bs);
got it.
>
> > +
> > + 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) {
>
> ditto
>
> > + 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;
>
> ditto
>
> > + 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;
>
> ditto
>
> > + 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;
>
> ditto
>
> > + 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)
>
> ditto
>
> and you can just use
>
> if (!cond)
>
> > + ddr_clk *= 2;
> > +
> > + priv->cpu_clk = cpu_clk;
> > + priv->sys_clk = cpu_clk / 4;
> > + priv->ddr_clk = ddr_clk;
> > + priv->xtal_clk = xtal_clk;
>
> Please implement the above logic in get_rate(). For example:
>
> static ulong do_mt7621_clk_get_rate()
> {
> ...
> switch (clk->id) {
> case CLK_SYS:
> cpu_clk = do_mt7621_clk_get_rate(priv,
> CLK_SYS);
> return IS_ERROR_VALUE(cpu_clk) ? cpu_clk :
> cpu_clk / 4;
> ...
> }
> }
>
> static ulong mt7621_clk_get_rate()
> {
> struct mt7621_clk_priv *priv = dev_get_priv(clk->dev);
>
> return do_mt7621_clk_get_rate(priv, clk->id);
> }
ok
>
> > +}
> > +
> > +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;
> > +
> > + regmap = syscon_node_to_regmap(args.node);
>
> According to the Linux binding for this node, it is supposed to live
> under the syscon already. So you can do
>
> syscon_node_to_regmap(dev_ofnode(dev_get_parent(dev)));
>
> and skip the phandle.
I'll try this
>
> > + if (IS_ERR(regmap))
> > + return PTR_ERR(regmap);
> > +
> > + base = regmap_get_range(regmap, 0);
> > + if (!base) {
> > + dev_err(dev, "Unable to find sysc\n");
>
> dev_dbg (see doc/develop/driver-model/design.rst)
ok
>
> > + 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,
>
> should be "ralink,memctl".
ok
>
> > + &args);
> > + if (ret)
> > + return ret;
> > +
> > + regmap = syscon_node_to_regmap(args.node);
> > + if (IS_ERR(regmap))
> > + return PTR_ERR(regmap);
>
> These two steps can be compined with syscon_regmap_lookup_by_phandle.
>
> > + base = regmap_get_range(regmap, 0);
> > + if (!base) {
> > + dev_err(dev, "Unable to find memc\n");
>
> dev_dbg
>
> > + return -ENODEV;
> > + }
> > +
> > + priv->memc_base = ioremap_nocache((phys_addr_t)base,
> > MEMC_MAP_SIZE);
> > +
> > + 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..b24aef351c
> > --- /dev/null
> > +++ b/include/dt-bindings/clock/mt7621-clk.h
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2021 MediaTek Inc. All rights reserved.
> > + *
> > + * Author: Weijie Gao <weijie....@mediatek.com>
> > + */
> > +
> > +#ifndef _DT_BINDINGS_MT7621_CLK_H_
> > +#define _DT_BINDINGS_MT7621_CLK_H_
> > +
> > +/* Base clocks */
> > +#define CLK_MIPS_CNT 36
> > +#define CLK_SYS 35
> > +#define CLK_DDR 34
> > +#define CLK_CPU 33
> > +#define CLK_XTAL 32
>
> Why is there a gap?
0~31 values are bits in clock gate register, and bit 31 is unused.
values above 31 represent clock sources not defined in the gate
register.
>
> > +/* Peripheral clocks */
> > +#define CLK_SDXC 30
> > +#define CLK_CRYPTO 29
> > +#define CLK_PCIE2 26
> > +#define CLK_PCIE1 25
> > +#define CLK_PCIE0 24
> > +#define CLK_GMAC 23
> > +#define CLK_UART3 21
> > +#define CLK_UART2 20
> > +#define CLK_UART1 19
> > +#define CLK_SPI 18
> > +#define CLK_I2S 17
> > +#define CLK_I2C 16
> > +#define CLK_NFI 15
> > +#define CLK_GDMA 14
> > +#define CLK_PIO 13
> > +#define CLK_PCM 11
> > +#define CLK_MC 10
> > +#define CLK_INTC 9
> > +#define CLK_TIMER 8
> > +#define CLK_SPDIFTX 7
> > +#define CLK_FE 6
> > +#define CLK_HSDMA 5
> > +
> > +#endif /* _DT_BINDINGS_MT7621_CLK_H_ */
>
> This file looks very different from
> include/dt-bindings/clock/mt7621-clk.h in Linux. In particular, it is
> backwards, the IDs are different (HSDMA is 8 in Linux but 5 here),
5 directly represents the bit in clock gate register, which means a
mapping must be done for the include/dt-bindings/clock/mt7621-clk.h
(i.e. subtract by 3) in kernel.
btw, the file in kernel is not submitted by mediatek.
> some
> of the IDs are named differently (SP_DIVTX vs SPDIFTX), and there is
> no
> MT7621 prefix. Can you comment on these? Are they deliberate?
The name SPDIFTX comes from the MT7621 programming guide of mediatek.
Adding MT7621 seems better.
> Note that
> in general, numerical IDs should be kept the same between Linux and
> U-Boot so we can use the same device tree. If you need to map between
> logical clock ID and a position in a register, I suggest something
> like
>
> struct {
> u8 gate_bit;
> } clocks {
> [CLK_HSDMA] = { .gate_bit = 5 },
> };
>
This is a driver dedicated for u-boot, and actually only the SYS clock
is used. I believe using correct gate bit number is clearer.
> --Sean
