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

[Sean Anderson <seanga2@gmail.com>]

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