[v2,03/14] clk: mtmips: add clock driver for MediaTek MT7621 SoC

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>
---
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

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@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@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?

> +/* 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.

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

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

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

> +	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)

> +		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".

> +					 &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@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?

> +/* 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), 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? 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 },
};

--Sean

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@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@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.gao@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

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@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@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@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

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.gao@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@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@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
> 86@gmail.com/T/#u

On 12/27/21 3:06 AM, Weijie Gao wrote:
> On Wed, 2021-12-15 at 11:11 -0500, Sean Anderson wrote:

>> 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.

Well, all you have to do is something like

switch (clk->id) {
case MT7621_CLK_TIMER:
     mask = CLKCFG1_TIMER;
     break;
/* etc */
}

or use an array if you like that style better.

--Sean