On Wed, 2021-12-15 at 11:11 -0500, Sean Anderson wrote:
> 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....@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.
>
> 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....@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.
I agreed.
But now I decide to write a simple driver which provides only the bus
clock. It seems that I don't have much time for rewrite the full clock
driver at present.
>
> --Sean
>
> [1] https://lore.kernel.org/u-boot/c670a4cc-b234-03d4-adfb-e6a8560c2d
> 8...@gmail.com/T/#u
