lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <d367c8c3-50ce-585f-9caa-573f42b76064@starfivetech.com> Date: Fri, 25 Nov 2022 10:33:47 +0800 From: Hal Feng <hal.feng@...rfivetech.com> To: Emil Renner Berthing <emil.renner.berthing@...onical.com> CC: "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>, Conor Dooley <conor@...nel.org>, Palmer Dabbelt <palmer@...belt.com>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Stephen Boyd <sboyd@...nel.org>, "Michael Turquette" <mturquette@...libre.com>, Philipp Zabel <p.zabel@...gutronix.de>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v2 11/14] clk: starfive: Add StarFive JH7110 system clock driver On Sat, 19 Nov 2022 01:03:54 +0800, Emil Renner Berthing wrote: > On Fri, 18 Nov 2022 at 02:06, Hal Feng <hal.feng@...rfivetech.com> wrote: >> diff --git a/drivers/clk/starfive/clk-starfive-jh7110-sys.c b/drivers/clk/starfive/clk-starfive-jh7110-sys.c >> new file mode 100644 >> index 000000000000..3c1afd691210 >> --- /dev/null >> +++ b/drivers/clk/starfive/clk-starfive-jh7110-sys.c >> @@ -0,0 +1,650 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * StarFive JH7110 System Clock Driver >> + * >> + * Copyright (C) 2022 Emil Renner Berthing <kernel@...il.dk> >> + * Copyright (C) 2022 StarFive Technology Co., Ltd. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/clk-provider.h> >> +#include <linux/init.h> >> +#include <linux/io.h> >> +#include <linux/platform_device.h> >> + >> +#include <dt-bindings/clock/starfive-jh7110.h> >> + >> +#include "clk-starfive-jh71x0.h" >> + >> +/* external clocks */ >> +#define JH7110_SYSCLK_OSC (JH7110_SYSCLK_END + 0) >> +#define JH7110_SYSCLK_GMAC1_RMII_REFIN (JH7110_SYSCLK_END + 1) >> +#define JH7110_SYSCLK_GMAC1_RGMII_RXIN (JH7110_SYSCLK_END + 2) >> +#define JH7110_SYSCLK_I2STX_BCLK_EXT (JH7110_SYSCLK_END + 3) >> +#define JH7110_SYSCLK_I2STX_LRCK_EXT (JH7110_SYSCLK_END + 4) >> +#define JH7110_SYSCLK_I2SRX_BCLK_EXT (JH7110_SYSCLK_END + 5) >> +#define JH7110_SYSCLK_I2SRX_LRCK_EXT (JH7110_SYSCLK_END + 6) >> +#define JH7110_SYSCLK_TDM_EXT (JH7110_SYSCLK_END + 7) >> +#define JH7110_SYSCLK_MCLK_EXT (JH7110_SYSCLK_END + 8) >> + >> +static const struct jh71x0_clk_data jh7110_sysclk_data[] __initconst = { >> + /* root */ >> + JH71X0__MUX(JH7110_SYSCLK_CPU_ROOT, "cpu_root", 2, >> + JH7110_SYSCLK_OSC, >> + JH7110_SYSCLK_PLL0_OUT), >> + JH71X0__DIV(JH7110_SYSCLK_CPU_CORE, "cpu_core", 7, >> + JH7110_SYSCLK_CPU_ROOT), >> + JH71X0__DIV(JH7110_SYSCLK_CPU_BUS, "cpu_bus", 2, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0__MUX(JH7110_SYSCLK_GPU_ROOT, "gpu_root", 2, >> + JH7110_SYSCLK_PLL2_OUT, >> + JH7110_SYSCLK_PLL1_OUT), >> + JH71X0_MDIV(JH7110_SYSCLK_PERH_ROOT, "perh_root", 2, 2, >> + JH7110_SYSCLK_PLL0_OUT, >> + JH7110_SYSCLK_PLL2_OUT), >> + JH71X0__MUX(JH7110_SYSCLK_BUS_ROOT, "bus_root", 2, >> + JH7110_SYSCLK_OSC, >> + JH7110_SYSCLK_PLL2_OUT), >> + JH71X0__DIV(JH7110_SYSCLK_NOCSTG_BUS, "nocstg_bus", 3, >> + JH7110_SYSCLK_BUS_ROOT), >> + JH71X0__DIV(JH7110_SYSCLK_AXI_CFG0, "axi_cfg0", 3, >> + JH7110_SYSCLK_BUS_ROOT), >> + JH71X0__DIV(JH7110_SYSCLK_STG_AXIAHB, "stg_axiahb", 2, >> + JH7110_SYSCLK_AXI_CFG0), >> + JH71X0_GATE(JH7110_SYSCLK_AHB0, "ahb0", CLK_IS_CRITICAL, >> + JH7110_SYSCLK_STG_AXIAHB), >> + JH71X0_GATE(JH7110_SYSCLK_AHB1, "ahb1", CLK_IS_CRITICAL, >> + JH7110_SYSCLK_STG_AXIAHB), >> + JH71X0__DIV(JH7110_SYSCLK_APB_BUS_FUNC, "apb_bus_func", 8, >> + JH7110_SYSCLK_STG_AXIAHB), >> + JH71X0_GATE(JH7110_SYSCLK_APB0, "apb0", CLK_IS_CRITICAL, >> + JH7110_SYSCLK_APB_BUS), >> + JH71X0__DIV(JH7110_SYSCLK_PLL0_DIV2, "pll0_div2", 2, >> + JH7110_SYSCLK_PLL0_OUT), >> + JH71X0__DIV(JH7110_SYSCLK_PLL1_DIV2, "pll1_div2", 2, >> + JH7110_SYSCLK_PLL1_OUT), >> + JH71X0__DIV(JH7110_SYSCLK_PLL2_DIV2, "pll2_div2", 2, >> + JH7110_SYSCLK_PLL2_OUT), >> + JH71X0__DIV(JH7110_SYSCLK_AUDIO_ROOT, "audio_root", 8, >> + JH7110_SYSCLK_PLL2_OUT), >> + JH71X0__DIV(JH7110_SYSCLK_MCLK_INNER, "mclk_inner", 64, >> + JH7110_SYSCLK_AUDIO_ROOT), >> + JH71X0__MUX(JH7110_SYSCLK_MCLK, "mclk", 2, >> + JH7110_SYSCLK_MCLK_INNER, >> + JH7110_SYSCLK_MCLK_EXT), >> + JH71X0_GATE(JH7110_SYSCLK_MCLK_OUT, "mclk_out", 0, >> + JH7110_SYSCLK_MCLK_INNER), >> + JH71X0_MDIV(JH7110_SYSCLK_ISP_2X, "isp_2x", 8, 2, >> + JH7110_SYSCLK_PLL2_OUT, >> + JH7110_SYSCLK_PLL1_OUT), >> + JH71X0__DIV(JH7110_SYSCLK_ISP_AXI, "isp_axi", 4, >> + JH7110_SYSCLK_ISP_2X), >> + JH71X0_GDIV(JH7110_SYSCLK_GCLK0, "gclk0", 0, 62, >> + JH7110_SYSCLK_PLL0_DIV2), >> + JH71X0_GDIV(JH7110_SYSCLK_GCLK1, "gclk1", 0, 62, >> + JH7110_SYSCLK_PLL1_DIV2), >> + JH71X0_GDIV(JH7110_SYSCLK_GCLK2, "gclk2", 0, 62, >> + JH7110_SYSCLK_PLL2_DIV2), >> + /* cores */ >> + JH71X0_GATE(JH7110_SYSCLK_CORE, "core_clk", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0_GATE(JH7110_SYSCLK_CORE1, "core_clk1", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0_GATE(JH7110_SYSCLK_CORE2, "core_clk2", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0_GATE(JH7110_SYSCLK_CORE3, "core_clk3", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0_GATE(JH7110_SYSCLK_CORE4, "core_clk4", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0_GATE(JH7110_SYSCLK_DEBUG, "debug_clk", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_BUS), >> + JH71X0__DIV(JH7110_SYSCLK_RTC_TOGGLE, "rtc_toggle", 6, >> + JH7110_SYSCLK_OSC), >> + JH71X0_GATE(JH7110_SYSCLK_TRACE0, "trace_clk0", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0_GATE(JH7110_SYSCLK_TRACE1, "trace_clk1", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0_GATE(JH7110_SYSCLK_TRACE2, "trace_clk2", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0_GATE(JH7110_SYSCLK_TRACE3, "trace_clk3", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0_GATE(JH7110_SYSCLK_TRACE4, "trace_clk4", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_CORE), >> + JH71X0_GATE(JH7110_SYSCLK_TRACE_COM, "trace_com", CLK_IGNORE_UNUSED, >> + JH7110_SYSCLK_CPU_BUS), > > Why should these clocks not be turned off if they're not critical like > the clocks above? > > My guess would be that the clocks for core 1-4 are critical, but maybe > not the trace and debug clocks. I synchronized these flags before with StarFive JH7110 SDK which is not released yet. But you're right, the core clocks should be enabled and the debug/trace clocks could be disabled during system booting. After some discussion, we think it's more appropriate to remove the flags of trace/debug clocks and set the flags of core clocks as CLK_IS_CRITICAL. > [...] >> +}; >> + >> +static const struct { >> + const char *name; >> + const char *parent; >> + unsigned int mul; >> + unsigned int div; >> +} jh7110_fixed_factor_clocks[JH7110_SYSCLK_END - JH7110_SYSCLK_PLL0_OUT] __initconst = { >> + [JH7110_SYSCLK_PLL0_OUT - JH7110_SYSCLK_PLL0_OUT] = { >> + "pll0_out", "osc", 625, 12 /* 24MHz -> 1250.0MHz */ >> + }, >> + [JH7110_SYSCLK_PLL1_OUT - JH7110_SYSCLK_PLL0_OUT] = { >> + "pll1_out", "osc", 533, 12 /* 24MHz -> 1066.0MHz */ >> + }, >> + [JH7110_SYSCLK_PLL2_OUT - JH7110_SYSCLK_PLL0_OUT] = { >> + "pll2_out", "osc", 99, 2 /* 24MHz -> 1188.0MHz */ >> + }, >> + [JH7110_SYSCLK_PCLK2_MUX_FUNC - JH7110_SYSCLK_PLL0_OUT] = { >> + "pclk2_mux_func", "apb_bus_func", 1, 1 >> + }, >> + [JH7110_SYSCLK_PCLK2_MUX - JH7110_SYSCLK_PLL0_OUT] = { >> + "pclk2_mux", "pclk2_mux_func", 1, 1 >> + }, >> + [JH7110_SYSCLK_APB_BUS - JH7110_SYSCLK_PLL0_OUT] = { >> + "apb_bus", "pclk2_mux", 1, 1 >> + }, >> + [JH7110_SYSCLK_AXI_CFG1 - JH7110_SYSCLK_PLL0_OUT] = { >> + "axi_cfg1", "isp_axi", 1, 1 >> + }, >> + [JH7110_SYSCLK_APB12 - JH7110_SYSCLK_PLL0_OUT] = { >> + "apb12", "apb_bus", 1, 1 >> + }, >> + [JH7110_SYSCLK_VOUT_ROOT - JH7110_SYSCLK_PLL0_OUT] = { >> + "vout_root", "pll2_out", 1, 1 >> + }, >> + [JH7110_SYSCLK_VENC_ROOT - JH7110_SYSCLK_PLL0_OUT] = { >> + "venc_root", "pll2_out", 1, 1 >> + }, >> + [JH7110_SYSCLK_VDEC_ROOT - JH7110_SYSCLK_PLL0_OUT] = { >> + "vdec_root", "pll0_out", 1, 1 >> + }, >> + [JH7110_SYSCLK_GMACUSB_ROOT - JH7110_SYSCLK_PLL0_OUT] = { >> + "gmacusb_root", "pll0_out", 1, 1 >> + }, >> +}; > > Having these extra 1-1 clocks that can't be controlled anyway seems unnecessary. > Consider sync'ing with my latest jh7110 branch at > https://github.com/esmil/linux/tree/jh7110 Thanks. I'll have a look. >> diff --git a/drivers/clk/starfive/clk-starfive-jh71x0.h b/drivers/clk/starfive/clk-starfive-jh71x0.h >> index baf4b5cb4b8a..7a52e46eb6db 100644 >> --- a/drivers/clk/starfive/clk-starfive-jh71x0.h >> +++ b/drivers/clk/starfive/clk-starfive-jh71x0.h >> @@ -105,10 +105,18 @@ struct jh71x0_clk_priv { >> spinlock_t rmw_lock; >> struct device *dev; >> void __iomem *base; >> - struct clk_hw *pll[3]; >> + struct clk_hw *pll[12]; >> struct jh71x0_clk reg[]; >> }; >> >> const struct clk_ops *starfive_jh71x0_clk_ops(u32 max); >> >> +#if IS_ENABLED(CONFIG_CLK_STARFIVE_JH7110_SYS) > > The function below is used both in the syscrg driver and the aoncrg > driver, so why guard it like this? Because the aoncrg driver depends on the syscrg driver, which is set in Kconfig. Best regards, Hal > >> +int jh7110_reset_controller_register(struct jh71x0_clk_priv *priv, >> + const char *adev_name, >> + u32 adev_id); >> + >> +#endif >> + >> #endif
Powered by blists - more mailing lists