Hi Pete, On Thu, 2024-04-04 at 13:25 +0100, Peter Griffin wrote: > CMU_HSI2 is the clock management unit used for the hsi2 block. > HSI stands for High Speed Interface and as such it generates > clocks for PCIe, UFS and MMC card. > > This patch adds support for the muxes, dividers, and gates in > cmu_hsi2. > > CLK_GOUT_HSI2_HSI2_CMU_HSI2_PCLK is marked as CLK_IS_CRITICAL > as disabling it leads to an immediate system hang. > > CLK_GOUT_HSI2_SYSREG_HSI2_PCLK is also marked CLK_IS_CRITICAL. > A hang is not observed with fine grained clock control, but > UFS IP does not function with syscon controlling this clock > just around hsi2_sysreg register accesses. Would it make sense to add this clock to the &ufs_0 node in the DTS instead? Seems more natural than a clock that's constantly enabled? > [...] > > Updated regex for clock name mangling > sed \ > -e 's|^PLL_LOCKTIME_PLL_\([^_]\+\)|fout_\L\1_pll|' \ > \ > -e 's|^PLL_CON0_MUX_CLKCMU_\([^_]\+\)_\(.*\)|mout_\L\1_\2|' \ > -e 's|^PLL_CON0_PLL_\(.*\)|mout_pll_\L\1|' \ > -e 's|^CLK_CON_MUX_MUX_CLK_\(.*\)|mout_\L\1|' \ > -e '/^PLL_CON[1-4]_[^_]\+_/d' \ > -e '/^[^_]\+_CMU_[^_]\+_CONTROLLER_OPTION/d' \ > -e '/^CLKOUT_CON_BLK_[^_]\+_CMU_[^_]\+_CLKOUT0/d' \ > \ > -e 's|_IPCLKPORT||' \ > -e 's|_RSTNSYNC||' \ > -e 's|_G4X2_DWC_PCIE_CTL||' \ > -e 's|_G4X1_DWC_PCIE_CTL||' \ > -e 's|_PCIE_SUB_CTRL||' \ > -e 's|_INST_0||g' \ > -e 's|_LN05LPE||' \ > -e 's|_TM_WRAPPER||' \ > -e 's|_SF||' \ > \ > -e 's|^CLK_CON_DIV_DIV_CLK_\([^_]\+\)_\(.*\)|dout_\L\1_\2|' \ > \ > -e 's|^CLK_CON_BUF_CLKBUF_\([^_]\+\)_\(.*\)|gout_\L\1_\2|' \ > -e 's|^CLK_CON_GAT_CLK_BLK_\([^_]\+\)_UID_\(.*\)|gout_\L\1_\2|' \ > -e 's|^gout_[^_]\+_[^_]\+_cmu_\([^_]\+\)_pclk$|gout_\1_\1_pclk|' \ > -e 's|^CLK_CON_GAT_GOUT_BLK_\([^_]\+\)_UID_\(.*\)|gout_\L\1_\2|' \ > -e 's|^CLK_CON_GAT_CLK_\([^_]\+\)_\(.*\)|gout_\L\1_clk_\L\1_\2|' \ > \ > -e '/^\(DMYQCH\|PCH\|QCH\|QUEUE\)_/d' Thank you for the updated regex. > --- > drivers/clk/samsung/clk-gs101.c | 558 +++++++++++++++++++++++ > include/dt-bindings/clock/google,gs101.h | 63 +++ > 2 files changed, 621 insertions(+) > > diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c > index d065e343a85d..b9f84c7d5c22 100644 > --- a/drivers/clk/samsung/clk-gs101.c > +++ b/drivers/clk/samsung/clk-gs101.c > @@ -22,6 +22,7 @@ > #define CLKS_NR_MISC (CLK_GOUT_MISC_XIU_D_MISC_ACLK + 1) > #define CLKS_NR_PERIC0 (CLK_GOUT_PERIC0_SYSREG_PERIC0_PCLK + 1) > #define CLKS_NR_PERIC1 (CLK_GOUT_PERIC1_SYSREG_PERIC1_PCLK + 1) > +#define CLKS_NR_HSI2 (CLK_GOUT_HSI2_XIU_P_HSI2_ACLK + 1) > > /* ---- CMU_TOP ------------------------------------------------------------- */ > > @@ -3409,6 +3410,560 @@ static const struct samsung_cmu_info peric1_cmu_info __initconst = { > .clk_name = "bus", > }; > > +/* ---- CMU_HSI2 ---------------------------------------------------------- */ This comment is shorter that all the other similar comments in this file. > [...] > + > +PNAME(mout_hsi2_bus_user_p) = { "oscclk", "dout_cmu_hsi2_bus" }; > +PNAME(mout_hsi2_pcie_user_p) = { "oscclk", "dout_cmu_hsi2_pcie" }; > +PNAME(mout_hsi2_ufs_embd_user_p) = { "oscclk", "dout_cmu_hsi2_ufs_embd" }; > +PNAME(mout_hsi2_mmc_card_user_p) = { "oscclk", "dout_cmu_hsi2_mmc_card" }; Can you make these alphabetical, too, please, which would also match their usage below: > + > +static const struct samsung_mux_clock hsi2_mux_clks[] __initconst = { > + MUX(CLK_MOUT_HSI2_BUS_USER, "mout_hsi2_bus_user", mout_hsi2_bus_user_p, > + PLL_CON0_MUX_CLKCMU_HSI2_BUS_USER, 4, 1), > + MUX(CLK_MOUT_HSI2_MMC_CARD_USER, "mout_hsi2_mmc_card_user", > + mout_hsi2_mmc_card_user_p, PLL_CON0_MUX_CLKCMU_HSI2_MMC_CARD_USER, > + 4, 1), > + MUX(CLK_MOUT_HSI2_PCIE_USER, "mout_hsi2_pcie_user", > + mout_hsi2_pcie_user_p, PLL_CON0_MUX_CLKCMU_HSI2_PCIE_USER, > + 4, 1), > + MUX(CLK_MOUT_HSI2_UFS_EMBD_USER, "mout_hsi2_ufs_embd_user", > + mout_hsi2_ufs_embd_user_p, PLL_CON0_MUX_CLKCMU_HSI2_UFS_EMBD_USER, > + 4, 1), > +}; > + > +static const struct samsung_gate_clock hsi2_gate_clks[] __initconst = { > + Here and below: all these extra empty lines are not needed. > + GATE(CLK_GOUT_HSI2_PCIE_GEN4_1_PCIE_003_PHY_REFCLK_IN, > + "gout_hsi2_pcie_gen4_1_pcie_003_phy_refclk_in", > + "mout_hsi2_pcie_user", > + CLK_CON_GAT_CLK_BLK_HSI2_UID_PCIE_GEN4_1_IPCLKPORT_PCIE_003_PCIE_SUB_CTRL_INST_0_PHY_REFCLK_IN, > + 21, 0, 0), > + > + GATE(CLK_GOUT_HSI2_PCIE_GEN4_1_PCIE_004_PHY_REFCLK_IN, > + "gout_hsi2_pcie_gen4_1_pcie_004_phy_refclk_in", > + "mout_hsi2_pcie_user", > + CLK_CON_GAT_CLK_BLK_HSI2_UID_PCIE_GEN4_1_IPCLKPORT_PCIE_004_PCIE_SUB_CTRL_INST_0_PHY_REFCLK_IN, > + 21, 0, 0), > + > + GATE(CLK_GOUT_HSI2_SSMT_PCIE_IA_GEN4A_1_ACLK, > + "gout_hsi2_ssmt_pcie_ia_gen4a_1_aclk", > + "mout_hsi2_bus_user", The two strings fit on the same line. > + CLK_CON_GAT_CLK_BLK_HSI2_UID_SSMT_PCIE_IA_GEN4A_1_IPCLKPORT_ACLK, > + 21, 0, 0), > + > + GATE(CLK_GOUT_HSI2_SSMT_PCIE_IA_GEN4A_1_PCLK, > + "gout_hsi2_ssmt_pcie_ia_gen4a_1_pclk", > + "mout_hsi2_bus_user", dito. > [...] > + /* Disabling this clock makes the system hang. Mark the clock as critical. */ > + GATE(CLK_GOUT_HSI2_HSI2_CMU_HSI2_PCLK, > + "gout_hsi2_hsi2_cmu_hsi2_pclk", "mout_hsi2_bus_user", > + CLK_CON_GAT_GOUT_BLK_HSI2_UID_HSI2_CMU_HSI2_IPCLKPORT_PCLK, > + 21, CLK_IS_CRITICAL, 0), I have a similar clock in USB, which also causes a hang if off, I wonder what we could do better here. Cheers, Andre'