Re: [PATCH v4 2/2] clk: qcom: gcc-msm8939: Add MSM8939 Generic Clock Controller
From: Stephen Boyd <sboyd@kernel.org>
Date: 2020-05-14 21:31:03
Also in:
linux-arm-msm, linux-clk, lkml
Quoting Bryan O'Donoghue (2020-05-12 04:50:23)
This patch adds support for the MSM8939 GCC. The MSM8939 is based on the MSM8916. MSM8939 is compatible in several ways with MSM8916 but, has additional functional blocks added which require additional PLL sources. In some cases functional blocks from the MSM8916 have different clock sources or different supported frequencies. Cc: Andy Gross <agross@kernel.org> Cc: Bjorn Andersson <redacted> Cc: Michael Turquette <mturquette@baylibre.com> Cc: Stephen Boyd <sboyd@kernel.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Philipp Zabel <p.zabel@pengutronix.de> Cc: linux-arm-msm@vger.kernel.org Cc: linux-clk@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: devicetree@vger.kernel.org Signed-off-by: Shawn Guo <redacted>
Is this a co-developed-by tag?
quoted hunk ↗ jump to hunk
Tested-by: Vincent Knecht <redacted> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> ---diff --git a/drivers/clk/qcom/gcc-msm8939.c b/drivers/clk/qcom/gcc-msm8939.c new file mode 100644 index 000000000000..71081eb23081 --- /dev/null +++ b/drivers/clk/qcom/gcc-msm8939.c@@ -0,0 +1,3999 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2020 Linaro Limited + */ + +#include <linux/kernel.h> +#include <linux/bitops.h> +#include <linux/err.h> +#include <linux/platform_device.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/clk-provider.h> +#include <linux/regmap.h> +#include <linux/reset-controller.h> + +#include <dt-bindings/clock/qcom,gcc-msm8939.h> +#include <dt-bindings/reset/qcom,gcc-msm8939.h> + +#include "common.h" +#include "clk-regmap.h" +#include "clk-pll.h" +#include "clk-rcg.h" +#include "clk-branch.h" +#include "reset.h" +#include "gdsc.h" + +enum { + P_XO, + P_GPLL0, + P_GPLL0_AUX, + P_BIMC, + P_GPLL1, + P_GPLL1_AUX, + P_GPLL2, + P_GPLL2_AUX, + P_GPLL3, + P_GPLL3_AUX, + P_GPLL4, + P_GPLL5, + P_GPLL5_AUX, + P_GPLL5_EARLY, + P_GPLL6, + P_GPLL6_AUX, + P_SLEEP_CLK, + P_DSI0_PHYPLL_BYTE, + P_DSI0_PHYPLL_DSI, + P_EXT_PRI_I2S, + P_EXT_SEC_I2S, + P_EXT_MCLK, +}; + +static struct clk_pll gpll0 = { + .l_reg = 0x21004, + .m_reg = 0x21008, + .n_reg = 0x2100c, + .config_reg = 0x21010, + .mode_reg = 0x21000, + .status_reg = 0x2101c, + .status_bit = 17, + .clkr.hw.init = &(struct clk_init_data){ + .name = "gpll0", + .parent_data = &(const struct clk_parent_data) { + .fw_name = "xo", + .name = "xo",
Please only have .fw_name. The .name field is a fallback, but given that this is new code there shouldn't be a need for a fallback to the old way of doing things.
+ },
+ .num_parents = 1,
+ .ops = &clk_pll_ops,
+ },
+};
+
+static struct clk_regmap gpll0_vote = {
+ .enable_reg = 0x45000,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gpll0_vote",
+ .parent_data = &(const struct clk_parent_data) {
+ .hw = &gpll0.clkr.hw,
+ },
+ .num_parents = 1,
+ .ops = &clk_pll_vote_ops,
+ },
+};
+
+static struct clk_pll gpll1 = {
+ .l_reg = 0x20004,
+ .m_reg = 0x20008,
+ .n_reg = 0x2000c,
+ .config_reg = 0x20010,
+ .mode_reg = 0x20000,
+ .status_reg = 0x2001c,
+ .status_bit = 17,
+ .clkr.hw.init = &(struct clk_init_data){
+ .name = "gpll1",
+ .parent_data = &(const struct clk_parent_data) {
+ .fw_name = "xo",
+ .name = "xo",
+ },
+ .num_parents = 1,
+ .ops = &clk_pll_ops,
+ },
+};
+
+static struct clk_regmap gpll1_vote = {
+ .enable_reg = 0x45000,
+ .enable_mask = BIT(1),
+ .hw.init = &(struct clk_init_data){
+ .name = "gpll1_vote",
+ .parent_data = &(const struct clk_parent_data) {
+ .hw = &gpll1.clkr.hw,
+ },
+ .num_parents = 1,
+ .ops = &clk_pll_vote_ops,
+ },
+};
+
+static struct clk_pll gpll2 = {
+ .l_reg = 0x4a004,
+ .m_reg = 0x4a008,
+ .n_reg = 0x4a00c,
+ .config_reg = 0x4a010,
+ .mode_reg = 0x4a000,
+ .status_reg = 0x4a01c,
+ .status_bit = 17,
+ .clkr.hw.init = &(struct clk_init_data){
+ .name = "gpll2",[...]
+
+static int gcc_msm8939_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct regmap *regmap;
+
+ ret = qcom_cc_probe(pdev, &gcc_msm8939_desc);
+ if (ret)
+ return ret;
+
+ regmap = dev_get_regmap(&pdev->dev, NULL);
+ clk_pll_configure_sr_hpm_lp(&gpll3, regmap, &gpll3_config, true);
+ clk_pll_configure_sr_hpm_lp(&gpll4, regmap, &gpll4_config, true);We should probably configure these before registering the clks. Can you do the usual, map registers, configure stuff, and then qcom_cc_really_probe()?
+ + return 0;
And then this can fail so return an error in case it does.
+}
+
+static struct platform_driver gcc_msm8939_driver = {
+ .probe = gcc_msm8939_probe,
+ .driver = {
+ .name = "gcc-msm8939",
+ .of_match_table = gcc_msm8939_match_table,
+ },
+};
+
+static int __init gcc_msm8939_init(void)
+{
+ return platform_driver_register(&gcc_msm8939_driver);
+}
+core_initcall(gcc_msm8939_init);
+
+static void __exit gcc_msm8939_exit(void)
+{
+ platform_driver_unregister(&gcc_msm8939_driver);
+}
+module_exit(gcc_msm8939_exit);
+
+MODULE_DESCRIPTION("Qualcomm GCC MSM8939 Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:gcc-msm8939");The module alias isn't needed right?