Hi Sam, Thanks for your review. I've trimmed the non relevant parts of the email a bit at Krzysztofs request. On Fri, 1 Dec 2023 at 22:40, Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote: > > On Fri, Dec 1, 2023 at 10:11 AM Peter Griffin <peter.griffin@xxxxxxxxxx> wrote: > > > > cmu_top is the top level clock management unit which contains PLLs, muxes, > > dividers and gates that feed the other clock management units. > > > > cmu_misc clocks IPs such as Watchdog and cmu_apm clocks ips part of the > > APM module. > > > > Acked-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx> > > Tested-by: Will McVicker <willmcvicker@xxxxxxxxxx> > > Signed-off-by: Peter Griffin <peter.griffin@xxxxxxxxxx> > > --- > > drivers/clk/samsung/Makefile | 1 + > > drivers/clk/samsung/clk-gs101.c | 2495 +++++++++++++++++++++++++++++++ > > 2 files changed, 2496 insertions(+) > > create mode 100644 drivers/clk/samsung/clk-gs101.c > > > > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile > > index ebbeacabe88f..3056944a5a54 100644 > > --- a/drivers/clk/samsung/Makefile > > +++ b/drivers/clk/samsung/Makefile > > @@ -21,6 +21,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos7.o > > obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos7885.o > > obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos850.o > > obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynosautov9.o > > +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-gs101.o > > obj-$(CONFIG_S3C64XX_COMMON_CLK) += clk-s3c64xx.o > > obj-$(CONFIG_S5PV210_COMMON_CLK) += clk-s5pv210.o clk-s5pv210-audss.o > > obj-$(CONFIG_TESLA_FSD_COMMON_CLK) += clk-fsd.o > > diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c > > new file mode 100644 > > index 000000000000..6bd233a7ab63 > > --- /dev/null > > +++ b/drivers/clk/samsung/clk-gs101.c > > @@ -0,0 +1,2495 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (C) 2023 Linaro Ltd. > > + * Author: Peter Griffin <peter.griffin@xxxxxxxxxx> > > + * > > + * Common Clock Framework support for GS101. > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/clk-provider.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/platform_device.h> > > + > > +#include <dt-bindings/clock/google,gs101.h> > > + > > +#include "clk.h" > > +#include "clk-exynos-arm64.h" > > + > > +/* NOTE: Must be equal to the last clock ID increased by one */ > > +#define TOP_NR_CLK (CLK_GOUT_TPU_UART + 1) > > +#define APM_NR_CLK (CLK_APM_PLL_DIV16_APM + 1) > > +#define MISC_NR_CLK (CLK_GOUT_MISC_XIU_D_MISC_IPCLKPORT_ACLK + 1) > > + > > Suggest using CLKS_NR_* naming, to follow other drivers for consistency. Will fix > > > +/* ---- CMU_TOP ------------------------------------------------------------- */ > > + > > +/* Register Offset definitions for CMU_TOP (0x1e080000) */ > > + > > +#define PLL_LOCKTIME_PLL_SHARED0 0x0000 > > +#define PLL_LOCKTIME_PLL_SHARED1 0x0004 > > +#define PLL_LOCKTIME_PLL_SHARED2 0x0008 > > +#define PLL_LOCKTIME_PLL_SHARED3 0x000c <cut> > > + DIV(CLK_DOUT_MFC_MFC, "dout_cmu_mfc_mfc", "gout_cmu_mfc_mfc", > > + CLK_CON_DIV_CLKCMU_MFC_MFC, 0, 4), > > + DIV(CLK_DOUT_MIF_BUSP, "dout_cmu_mif_busp", "gout_cmu_mif_busp", > > + CLK_CON_DIV_CLKCMU_MIF_BUSP, 0, 4), > > + DIV(CLK_DOUT_MISC_BUS, "dout_cmu_misc_bus", "gout_cmu_misc_bus", > > + CLK_CON_DIV_CLKCMU_MISC_BUS, 0, 4), > > + DIV(CLK_DOUT_MISC_SSS, "dout_cmu_misc_sss", "gout_cmu_misc_sss", > > + CLK_CON_DIV_CLKCMU_MISC_SSS, 0, 4), > > + /* CLK_CON_DIV_CLKCMU_OTP lower bits reserved*/ > > Why the above clock where bits are reserved is declared, but this one > is not? Also, above comment is marked as TODO, but this one is not. > And there is no space before */. Good spot! These two dividers with reserved lower bits should be fixed factor clocks. I've fixed in v6. > > > + DIV(CLK_DOUT_PDP_BUS, "dout_cmu_pdp_bus", "gout_cmu_pdp_bus", > > + CLK_CON_DIV_CLKCMU_PDP_BUS, 0, 4), > > + DIV(CLK_DOUT_PDP_VRA, "dout_cmu_pdp_vra", "gout_cmu_pdp_vra", > > + CLK_CON_DIV_CLKCMU_PDP_VRA, 0, 4), > > + DIV(CLK_DOUT_PERIC0_BUS, "dout_cmu_peric0_bus", "gout_cmu_peric0_bus", > > + CLK_CON_DIV_CLKCMU_PERIC0_BUS, 0, 4), <cut> > > + DIV(CLK_DOUT_SHARED1_DIV2, "dout_shared1_div2", "mout_shared1_pll", > > + CLK_CON_DIV_PLL_SHARED1_DIV2, 0, 1), > > + DIV(CLK_DOUT_SHARED1_DIV3, "dout_shared1_div3", "mout_shared1_pll", > > + CLK_CON_DIV_PLL_SHARED1_DIV3, 0, 2), > > + DIV(CLK_DOUT_SHARED1_DIV4, "dout_shared1_div4", "mout_shared1_pll", > > + CLK_CON_DIV_PLL_SHARED1_DIV4, 0, 1), > > + DIV(CLK_DOUT_SHARED2_DIV2, "dout_shared2_div2", "mout_shared2_pll", > > + CLK_CON_DIV_PLL_SHARED2_DIV2, 0, 1), > > + DIV(CLK_DOUT_SHARED3_DIV2, "dout_shared3_div2", "mout_shared3_pll", > > + CLK_CON_DIV_PLL_SHARED3_DIV2, 0, 1), > > +}; > > + > > +static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = { > > + GATE(CLK_GOUT_BUS0_BOOST, "gout_cmu_bus0_boost", "mout_cmu_boost_option1", > > Please stick to 80 characters length, here and below. Will fix. > > > + CLK_CON_GAT_CLKCMU_BUS0_BOOST, 21, 0, 0), > > + GATE(CLK_GOUT_BUS1_BOOST, "gout_cmu_bus1_boost", "mout_cmu_boost_option1", > > + CLK_CON_GAT_CLKCMU_BUS1_BOOST, 21, 0, 0), > > + GATE(CLK_GOUT_BUS2_BOOST, "gout_cmu_bus2_boost", "mout_cmu_boost_option1", > > + CLK_CON_GAT_CLKCMU_BUS2_BOOST, 21, 0, 0), > > + GATE(CLK_GOUT_CORE_BOOST, "gout_cmu_core_boost", "mout_cmu_boost_option1", > > + CLK_CON_GAT_CLKCMU_CORE_BOOST, 21, 0, 0), > > +static void __init gs101_cmu_top_init(struct device_node *np) > > +{ > > + exynos_arm64_register_cmu(NULL, np, &top_cmu_info); > > +} > > + > > +/* Register CMU_TOP early, as it's a dependency for other early domains */ > > +CLK_OF_DECLARE(gs101_cmu_top, "google,gs101-cmu-top", > > + gs101_cmu_top_init); > > + > > +/* ---- CMU_APM ------------------------------------------------------------- */ > > Suggest adding an empty line here. Will fix > > > +/* Register Offset definitions for CMU_APM (0x17400000) */ > > +#define APM_CMU_APM_CONTROLLER_OPTION 0x0800 > > +#define CLKOUT_CON_BLK_APM_CMU_APM_CLKOUT0 0x0810 > > +#define CLK_CON_MUX_MUX_CLKCMU_APM_FUNC 0x1000 > > +#define CLK_CON_MUX_MUX_CLKCMU_APM_FUNCSRC 0x1004 > > +#define CLK_CON_DIV_DIV_CLK_APM_BOOST 0x1800 > > +#define CLK_CON_DIV_DIV_CLK_APM_USI0_UART 0x1804 > > +#define CLK_CON_DIV_DIV_CLK_APM_USI0_USI 0x1808 > > +#define CLK_CON_DIV_DIV_CLK_APM_USI1_UART 0x180c > > +#define CLK_CON_GAT_CLK_BLK_APM_UID_APM_CMU_APM_IPCLKPORT_PCLK 0x2000 > > +#define CLK_CON_GAT_CLK_BUS0_BOOST_OPTION1 0x2004 > > +#define CLK_CON_GAT_CLK_CMU_BOOST_OPTION1 0x2008 > > +#define CLK_CON_GAT_CLK_CORE_BOOST_OPTION1 0x200c <cut> > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SPEEDY_APM_IPCLKPORT_PCLK 0x20a8 > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SPEEDY_SUB_APM_IPCLKPORT_PCLK 0x20ac > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SSMT_D_APM_IPCLKPORT_ACLK 0x20b0 > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SSMT_D_APM_IPCLKPORT_PCLK 0x20b4 > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SSMT_G_DBGCORE_IPCLKPORT_ACLK 0x20b8 > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SSMT_G_DBGCORE_IPCLKPORT_PCLK 0x20bc > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SS_DBGCORE_IPCLKPORT_SS_DBGCORE_IPCLKPORT_HCLK 0x20c0 > > As I understand, all those parts like IPCLKPORT, BLK, UID (RSTNSYNC > probably too) -- they don't really mean anything in the context of the > driver, just noise. And if you remove those -- there won't be any > conflicts with other names, because those bits are not the unique > parts. Following the TRM letter for letter in this case just makes > things extremely long without adding any value IMHO. For example, that > name above might be: > > CLK_CON_GAT_GOUT_APM_SS_DBGCORE_SS_DBGCORE_HCLK > > or even > > CLK_CON_GAT_GOUT_APM_SS_DBGCORE_HCLK > > would be fine. > > In clk-exynos850 driver I removed all those parts, because they make > it pretty much impossible to read both the driver and dts. And yeah, > because those names consequenty lead to very long string names, dts > will be hard to read too, which is even worse. But again, that's only > my IMHO. I would like to keep the register names unmodified, as I mentioned previously mangling them makes checking with the datasheet much harder. As the name in the header and string are already mangled a bit I'm OK with mangling that a bit more. With that in mind I've done the following mangling in v6: - Register name: CLK_CON_GAT_GOUT_BLK_APM_UID_APBIF_GPIO_ALIVE_IPCLKPORT_PCLK Replace `CLK_CON_GAT_GOUT_BLK_<blockname>_UID_` with `CLK_GOUT_<blockname>_` Replace ` _IPCLKPORT` with nothing Replace `_RSTNSYNC` with nothing So you end up with: GATE(CLK_GOUT_APM_APBIF_GPIO_ALIVE_PCLK, "gout_apm_apbif_gpio_alive_pclk", "gout_apm_func", CLK_CON_GAT_GOUT_BLK_APM_UID_APBIF_GPIO_ALIVE_IPCLKPORT_PCLK, 21, 0, 0), That still enables cross referencing with the datasheet via the unmodified register name, but the dt binding define and string are slightly shorter like you wanted. The mangling described above the vast majority of clock names look fine. There are a couple of anomalies though like `CLK_GOUT_MISC_MISC_CMU_MISC_PCLK` and `CLK_GOUT_APM_APM_CMU_APM_PCLK`. I think it is a *really* bad idea to do custom mangling on these specific names though. > > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SYSMMU_D_APM_IPCLKPORT_CLK_S2 0x20c4 > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SYSREG_APM_IPCLKPORT_PCLK 0x20cc > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_APM_IPCLKPORT_ACLK 0x20d0 > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_APM_IPCLKPORT_PCLK 0x20d4 > > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_DBGCORE_IPCLKPORT_ACLK 0x20d8 <cut> > > + GATE(CLK_GOUT_APM_APBIF_RTC_IPCLKPORT_PCLK, > > + "gout_apm_apbif_rtc_ipclkport_pclk", "gout_apm_func", > > + CLK_CON_GAT_GOUT_BLK_APM_UID_APBIF_RTC_IPCLKPORT_PCLK, 21, 0, 0), > > + GATE(CLK_GOUT_APM_APBIF_TRTC_IPCLKPORT_PCLK, > > + "gout_apm_apbif_trtc_ipclkport_pclk", "gout_apm_func", > > + CLK_CON_GAT_GOUT_BLK_APM_UID_APBIF_TRTC_IPCLKPORT_PCLK, 21, 0, 0), > > + GATE(CLK_GOUT_APM_APM_USI0_UART_IPCLKPORT_PCLK, > > + "gout_apm_apm_usi0_uart_ipclkport_pclk", "gout_apm_func", > > + CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_UART_IPCLKPORT_PCLK, > > + 21, 0, 0), > > + GATE(CLK_GOUT_APM_APM_USI0_USI_IPCLKPORT_IPCLK, > > + "gout_apm_apm_usi0_usi_ipclkport_ipclk", "dout_apm_usi0_uart", > > + CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_USI_IPCLKPORT_PCLK, > > PCLK vs IPCLK? Will fix. The order here should match the register offsets CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_UART_IPCLKPORT_IPCLK, CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_UART_IPCLKPORT_PCLK, CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_USI_IPCLKPORT_IPCLK, CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_USI_IPCLKPORT_PCLK, CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI1_UART_IPCLKPORT_IPCLK, CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI1_UART_IPCLKPORT_PCLK, > > > + 21, 0, 0), > > + GATE(CLK_GOUT_APM_APM_USI0_USI_IPCLKPORT_PCLK, > > + "gout_apm_apm_usi0_usi_ipclkport_pclk", "gout_apm_func", > > + CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI1_UART_IPCLKPORT_PCLK, 21, 0, 0), > > Should it be USI0 instead of USI1? It's the same as the definition > below. Also, please stick to 80 characters per line, here and below. I've revisited the 6 clock gates described above to ensure they are in register offset order. > > > + GATE(CLK_GOUT_APM_APM_USI1_UART_IPCLKPORT_IPCLK, > > + "gout_apm_apm_usi1_uart_ipclkport_ipclk", "dout_apm_usi1_uart", > > + CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI1_UART_IPCLKPORT_IPCLK, 21, 0, 0), > > + GATE(CLK_GOUT_APM_APM_USI1_UART_IPCLKPORT_PCLK, > > + "gout_apm_apm_usi1_uart_ipclkport_pclk", "gout_apm_func", <cut> > > +static const struct samsung_cmu_info apm_cmu_info __initconst = { > > + .mux_clks = apm_mux_clks, > > + .nr_mux_clks = ARRAY_SIZE(apm_mux_clks), > > + .div_clks = apm_div_clks, > > + .nr_div_clks = ARRAY_SIZE(apm_div_clks), > > + .gate_clks = apm_gate_clks, > > + .nr_gate_clks = ARRAY_SIZE(apm_gate_clks), > > + .fixed_clks = apm_fixed_clks, > > + .nr_fixed_clks = ARRAY_SIZE(apm_fixed_clks), > > + .nr_clk_ids = APM_NR_CLK, > > + .clk_regs = apm_clk_regs, > > + .nr_clk_regs = ARRAY_SIZE(apm_clk_regs), > > +}; > > + > > +/* ---- CMU_MISC ------------------------------------------------------------- */ > > Suggest adding an empty line here. Will fix > > > +/* Register Offset definitions for CMU_MISC (0x10010000) */ > > +#define PLL_CON0_MUX_CLKCMU_MISC_BUS_USER 0x0600 > > +#define PLL_CON1_MUX_CLKCMU_MISC_BUS_USER 0x0604 > > +#define PLL_CON0_MUX_CLKCMU_MISC_SSS_USER 0x0610 > > +#define PLL_CON1_MUX_CLKCMU_MISC_SSS_USER 0x0614 > > +#define MISC_CMU_MISC_CONTROLLER_OPTION 0x0800 > > +#define CLKOUT_CON_BLK_MISC_CMU_MISC_CLKOUT0 0x0810 > > +#define CLK_CON_MUX_MUX_CLK_MISC_GIC 0x1000 > > +#define CLK_CON_DIV_DIV_CLK_MISC_BUSP 0x1800 > > +#define CLK_CON_DIV_DIV_CLK_MISC_GIC 0x1804 > > Please align all values for this group at the same indentation level. Will fix > > > +#define CLK_CON_GAT_CLK_BLK_MISC_UID_MISC_CMU_MISC_IPCLKPORT_PCLK 0x2000 > > +#define CLK_CON_GAT_CLK_BLK_MISC_UID_OTP_CON_BIRA_IPCLKPORT_I_OSCCLK 0x2004 > > +#define CLK_CON_GAT_CLK_BLK_MISC_UID_OTP_CON_BISR_IPCLKPORT_I_OSCCLK 0x2008 > > +#define CLK_CON_GAT_CLK_BLK_MISC_UID_OTP_CON_TOP_IPCLKPORT_I_OSCCLK 0x200c > > +#define CLK_CON_GAT_CLK_BLK_MISC_UID_RSTNSYNC_CLK_MISC_OSCCLK_IPCLKPORT_CLK 0x2010 > > Just want to give you one more example for my rant above. Look how > much easier it is to understand this name (than above one): > > CLK_CON_GAT_MISC_OSCCLK_CLK > > Which also turns this (public API!): > > "gout_misc_rstnsync_clk_misc_oscclk_ipclkport_clk" > > into this: > > "gout_misc_oscclk_clk" In v6 this becomes GATE(CLK_GOUT_MISC_CLK_MISC_OSCCLK_CLK, "gout_misc_clk_misc_oscclk_clk", "dout_misc_busp", CLK_CON_GAT_CLK_BLK_MISC_UID_RSTNSYNC_CLK_MISC_OSCCLK_IPCLKPORT_CLK, 21, 0, 0), <cut> > > > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_ADM_AHB_SSS_IPCLKPORT_HCLKM 0x2014 > > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_AD_APB_DIT_IPCLKPORT_PCLKM 0x2018 > > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_AD_APB_PUF_IPCLKPORT_PCLKM 0x201c > > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_DIT_IPCLKPORT_ICLKL2A 0x2020 > > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_D_TZPC_MISC_IPCLKPORT_PCLK 0x2024 > > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_GIC_IPCLKPORT_GICCLK 0x2028 > > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_GPC_MISC_IPCLKPORT_PCLK 0x202c <cut> > > +static const struct samsung_div_clock misc_div_clks[] __initconst = { > > + DIV(CLK_DOUT_MISC_BUSP, "dout_misc_busp", "mout_misc_bus_user", > > + CLK_CON_DIV_DIV_CLK_MISC_BUSP, 0, 3), > > + DIV(CLK_DOUT_MISC_GIC, "dout_misc_gic", "mout_misc_bus_user", > > + CLK_CON_DIV_DIV_CLK_MISC_GIC, 0, 3), > > +}; > > + > > +static const struct samsung_gate_clock misc_gate_clks[] __initconst = { > > + GATE(CLK_GOUT_MISC_MISC_CMU_MISC_IPCLKPORT_PCLK, > > + "gout_misc_ipclkport_pclk", "dout_misc_busp", > > So if you want to be consistent, it should be > "gout_misc_misc_misc_....". My point is -- Samsung's naming in TRM is > insane. They wanted to stick to some very detailed and unified naming > schema (or just generated those names from some internal clock tree > data), granted. But I'm just not sure if it's the best idea to follow > it in driver's code. With the naming described above it becomes "gout_misc_misc_cmu_misc_pclk". Not ideal, and this is one of the clocks I mentioned above. The overwhelming majority of the clock names don't have such repetition in their name though. If we start doing custom mangling on specific clocks it is going to become a nightmare to track. As I mentioned previously there are thousands of clocks in the SoC. Most upstream platforms only seem to implement a small subset of clocks in each CMU which maybe makes it easier to track. Our goal though is to have full functionality upstream. > > > + CLK_CON_GAT_CLK_BLK_MISC_UID_MISC_CMU_MISC_IPCLKPORT_PCLK, > > + 21, 0, 0), > > + GATE(CLK_GOUT_MISC_OTP_CON_BIRA_IPCLKPORT_I_OSCCLK, > > + "gout_misc_otp_con_bira_ipclkport_i_oscclk", "dout_misc_busp", > > + CLK_CON_GAT_CLK_BLK_MISC_UID_OTP_CON_BIRA_IPCLKPORT_I_OSCCLK, > > + 21, 0, 0), > > + GATE(CLK_GOUT_MISC_OTP_CON_BISR_IPCLKPORT_I_OSCCLK, > > + "gout_misc_otp_con_bisr_ipclkport_i_oscclk", "dout_misc_busp", > > + CLK_CON_GAT_CLK_BLK_MISC_UID_OTP_CON_BISR_IPCLKPORT_I_OSCCLK, > > + 21, 0, 0), <snip> > > + GATE(CLK_GOUT_MISC_RTIC_IPCLKPORT_I_ACLK, > > + "gout_misc_rtic_ipclkport_i_aclk", "dout_misc_busp", > > + CLK_CON_GAT_GOUT_BLK_MISC_UID_RTIC_IPCLKPORT_I_ACLK, > > + 21, 0, 0), > > + GATE(CLK_GOUT_MISC_RTIC_IPCLKPORT_I_PCLK, "gout_misc_rtic_ipclkport_i_pclk", > > 80 characters per line please. Will fix regards, Peter