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 326B1C433EF for ; Wed, 15 Dec 2021 16:12:09 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C5B6C83047; Wed, 15 Dec 2021 17:12:06 +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="GtCCRmEG"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 9F162830A8; Wed, 15 Dec 2021 17:12:04 +0100 (CET) Received: from mail-qt1-x829.google.com (mail-qt1-x829.google.com [IPv6:2607:f8b0:4864:20::829]) (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 04C3580FD2 for ; Wed, 15 Dec 2021 17:11:59 +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-x829.google.com with SMTP id a1so1413548qtx.11 for ; Wed, 15 Dec 2021 08:11:58 -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=ruwZEOhAZ46mADUkFpbvuWMDpXZYTuMcFQ9I9XHM/oo=; b=GtCCRmEGkM/9r3Bj4kXX4EifZDeiRnpUt/Q4/ViQUe0Lonozb9G01LGSzTcRtUsMAe PE05Uy00QbxNjawGi0ySli9Y88IJRTctToVKa2y0QU6fu87pg8tILVGoKI1OtteHXfpd xzsl4wiznK57aXxDdwGoSjdA4RY/8sqDsNGp3lVqmBckf5Rds/Nq5h8T0x37ES0hZ9dt eFScdt8zYwJ5YpDd46iWpSJzQnd5giLlbQLU0IcKPVkuKI1O9BeknlDjkZthpnaTJ+hW NdfelHOIJwMCM2w5OBrP64+A6QOzTv1ErbwofDGTnTwxaCV7RgquxL/62NMlIb+hH1FA eQSQ== 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=ruwZEOhAZ46mADUkFpbvuWMDpXZYTuMcFQ9I9XHM/oo=; b=j6BJ2I6IVVkbuR/6U6wYIuKK5ONZqELaOYL0N5USv6MA2qz0/i7gN+0T4kA7GmTADo 1a+nv9GJ9nVqaIwqvL6W3rDhmXng9DE/in2GhsuWBwFviyqIuenLIgPMFuaYGNqYswKY f8F1nDG6TNU7ZJMQU4HcbTZSunajcixI2hO8koIqcF82PhN8glzCdrnlC9eQuJqe440W 7l6kVH1/bz0RfUR7FMEMzmS3UNGcjZeKY4sGYmuN7Z1Px+JUxhyyC4lSDEXIVN8AdXHK LhmtwBjRk98MecjbyX1W8WVLeCWQBpDgvqN2kXAJg9vJyw1YEsmppztcdKEwudVRLUUg 4S2A== X-Gm-Message-State: AOAM531qQKTGqMAf3tZnz//7LVVFmzxdFIg4cerGMx7aBAqFjOoUNUPl z1dXnMN6wU2xMVi4tNpDMPoIcSC2DBw= X-Google-Smtp-Source: ABdhPJxVvu7+HI3c3RAr04Bb37cNPn3EXDdQm223zTT9/3Gf1PNg4BEgscCu2y2Eu4sG7U/0jpzlhA== X-Received: by 2002:a05:622a:344:: with SMTP id r4mr12221450qtw.590.1639584717598; Wed, 15 Dec 2021 08:11:57 -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 y20sm1304439qkj.24.2021.12.15.08.11.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 15 Dec 2021 08:11:57 -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: <0a795ee34bbeb7d8e58cf8f60c35d5048f9bda95.camel@mediatek.com> Message-ID: Date: Wed, 15 Dec 2021 11:11:56 -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: <0a795ee34bbeb7d8e58cf8f60c35d5048f9bda95.camel@mediatek.com> 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.38 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 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 >>> --- >>> 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? > > 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 >>> + */ >>> + >>> +#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