From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5C83BC433EF for ; Fri, 26 Nov 2021 17:44:59 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C78F1830A7; Fri, 26 Nov 2021 18:44:56 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="S4wvupyW"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 9193483681; Fri, 26 Nov 2021 18:44:54 +0100 (CET) Received: from mail-qt1-x833.google.com (mail-qt1-x833.google.com [IPv6:2607:f8b0:4864:20::833]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id CC3EA80960 for ; Fri, 26 Nov 2021 18:44:48 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=seanga2@gmail.com Received: by mail-qt1-x833.google.com with SMTP id v22so9649953qtx.8 for ; Fri, 26 Nov 2021 09:44:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:subject:to:cc:references:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=yCtfYcdYmCZs7BL/aFeI04n9Z65FjCfRgfl9DKFBHR0=; b=S4wvupyWnTBYKOViljv5JgT6QTIpId10o7CzQDUCdrcC9D0Cx1ktBNp56Le2QCcx+P ZSn/bxR+ZxRsLMGdWHdkF9gRmLDqAKdkmSMEFgJ78HeCcjH0q5grispDMbXA7U7LtzEV I9mMLHB8B4RUP8GpSlUfIhdezxamc3dAOuLFoAa6N1mf5635qHmlwlR4aFSkchbvfIC0 RApIFRbbvANYGZ9piuUBVbRQJckRkIguDEbMb+n14s8JvwKsm8d29bDAiqVil6VHxX4j Y7czQjUuu4OxT36xpmOvGvfSVkvxkL2P+EQ+IqyoAPMEkxFOIOKB8kAA5ArcHlJ5oGQW nn/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=yCtfYcdYmCZs7BL/aFeI04n9Z65FjCfRgfl9DKFBHR0=; b=2WOzhsWuXQOIkTUa1nD7vFGyTz51SXOKWE3PjdumyL3MJtESjeHn9wC75szM7sW63r LYsAF4dWt5WGAf/j5KkWUqo+kP5bdBJvXZewR349Li4Xiq1stRkB1mbPrOFtqHWEPPN9 O/NZhP0gWuICL/K59Qg48j15Y4t+yrj2eqmQvUJv3+NReM51eho/5pxnXZikx49HpZTC ZSdRGMKHURhz5RIg9xEPxQtBI0Z7lRJLyYk8Bl6ZwGj+4gLaqVYtv59uZiNGb1GIHhsT hmflSa2uzovjMjD//mS+E/LfPtT2RHrgBsHMX0+KQrwY6cWpcupPU6aBeCcVf1F0piTC 7HOA== X-Gm-Message-State: AOAM530FTuvgE6Q/yB1mlFpud6+S7nQ5onaDq42PyCO5vhrJdNDDKmJR pKIbx/bjFEx9jXvfZQA2MWUI0nKM13M= X-Google-Smtp-Source: ABdhPJyuFDW+qqEq8Grmhnou6+ncwqWRJd4k/CIp1RNgDDr1PpSYDfKvvGcoTM3hqMXYJ5DWK/69cQ== X-Received: by 2002:ac8:5b88:: with SMTP id a8mr17090581qta.494.1637948687556; Fri, 26 Nov 2021 09:44:47 -0800 (PST) Received: from [192.168.1.201] (pool-108-18-207-184.washdc.fios.verizon.net. [108.18.207.184]) by smtp.googlemail.com with ESMTPSA id u18sm3693712qki.69.2021.11.26.09.44.47 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 26 Nov 2021 09:44:47 -0800 (PST) From: Sean Anderson Subject: Re: [PATCH v2 03/14] clk: mtmips: add clock driver for MediaTek MT7621 SoC To: Weijie Gao , u-boot@lists.denx.de Cc: GSS_MTK_Uboot_upstream , Lukasz Majewski References: Message-ID: Date: Fri, 26 Nov 2021 12:44:46 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.37 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean 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 > --- > 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 > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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 > + */ > + > +#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