[PATCH v2 03/14] clk: mtmips: add clock driver for MediaTek MT7621 SoC
Sean Anderson
seanga2 at gmail.com
Wed Dec 15 17:11:56 CET 2021
Hi Weijie,
(sorry for the delayed response)
On 12/3/21 5:06 AM, Weijie Gao wrote:
> 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.gao at 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.gao at 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.
see below
>>
>>> + 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.gao at 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.
OK, but above your check is for clock IDs less than 31. So shouldn't it
be something like
if (clk->id > CLK_SDXC)
return -EINVAL;
>>
>>> +/* 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.
I think aligning the defines with the names in the datasheet is a good
idea. Can you send a patch to Linux to update the names of the defines?
>> 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.
It is fine to implement only the necessary functionality, but it should
be done in a way which is easy to extend in the future, and which won't
cause us compatibility problems.
Generally, I would like to preserve both source and binary compatibility
with Linux where possible. I am not sure whether they are hard
requirements, so I made a post regarding that question [1]. For now,
addressing my above comments will be fine.
--Sean
[1] https://lore.kernel.org/u-boot/c670a4cc-b234-03d4-adfb-e6a8560c2d86@gmail.com/T/#u
More information about the U-Boot
mailing list