From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH v2] clk: qcom: Add MSM8916 Global Clock Controller support Date: Thu, 5 Mar 2015 11:58:38 -0800 Message-ID: <20150305195838.GA11174@codeaurora.org> References: <1424882699-32758-1-git-send-email-georgi.djakov@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:51650 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758460AbbCET6k (ORCPT ); Thu, 5 Mar 2015 14:58:40 -0500 Content-Disposition: inline In-Reply-To: <1424882699-32758-1-git-send-email-georgi.djakov@linaro.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Georgi Djakov Cc: mturquette@linaro.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org On 02/25, Georgi Djakov wrote: > diff --git a/drivers/clk/qcom/gcc-msm8916.c b/drivers/clk/qcom/gcc-msm8916.c > new file mode 100644 > index 000000000000..810c38004520 > --- /dev/null > +++ b/drivers/clk/qcom/gcc-msm8916.c > + > +#define P_XO 0 > +#define P_GPLL0 1 > +#define P_GPLL0_AUX 1 > +#define P_BIMC 2 > +#define P_GPLL1 2 > +#define P_GPLL1_AUX 2 > +#define P_GPLL2 3 > +#define P_GPLL2_AUX 3 > +#define P_SLEEP_CLK 3 > +#define P_DSI0_PHYPLL_BYTE 2 > +#define P_DSI0_PHYPLL_DSI 2 Ok... > + > +static const u8 gcc_xo_gpll0_map[] = { > + [P_XO] = 0, > + [P_GPLL0] = 1, > +}; > + > +static const char *gcc_xo_gpll0[] = { > + "xo", > + "gpll0_vote", > +}; > + > +static const u8 gcc_xo_gpll0_bimc_map[] = { > + [P_XO] = 0, > + [P_GPLL0] = 1, > + [P_BIMC] = 2, > +}; > + > +static const char *gcc_xo_gpll0_bimc[] = { > + "xo", > + "gpll0_vote", > + "bimc_pll_vote", > +}; > + > +static const u8 gcc_xo_gpll0a_gpll1_gpll2a_map[] = { > + [P_XO] = 0, > + [P_GPLL0_AUX] = 3, > + [P_GPLL1] = 1, > + [P_GPLL2_AUX] = 2, > +}; > + > +static const char *gcc_xo_gpll0a_gpll1_gpll2a[] = { > + "xo", > + "gpll0_vote", > + "gpll1_vote", > + "gpll2_vote", > +}; > + > +static const u8 gcc_xo_gpll0_gpll2_map[] = { > + [P_XO] = 0, > + [P_GPLL0] = 1, > + [P_GPLL2] = 2, > +}; > + > +static const char *gcc_xo_gpll0_gpll2[] = { > + "xo", > + "gpll0_vote", > + "gpll2_vote", > +}; > + > +static const u8 gcc_xo_gpll0a_map[] = { > + [P_XO] = 0, > + [P_GPLL0_AUX] = 2, > +}; > + > +static const char *gcc_xo_gpll0a[] = { > + "xo", > + "gpll0_vote", > +}; > + > +static const u8 gcc_xo_gpll0_gpll1a_sleep_map[] = { > + [P_XO] = 0, > + [P_GPLL0] = 1, > + [P_GPLL1_AUX] = 2, > + [P_SLEEP_CLK] = 6, > +}; > + > +static const char *gcc_xo_gpll0_gpll1a_sleep[] = { > + "xo", > + "gpll0_vote", > + "gpll1_vote", > + "sleep_clk", > +}; > + > +static const u8 gcc_xo_gpll0_gpll1a_map[] = { > + [P_XO] = 0, > + [P_GPLL0] = 1, > + [P_GPLL1_AUX] = 2, > +}; > + > +static const char *gcc_xo_gpll0_gpll1a[] = { > + "xo", > + "gpll0_vote", > + "gpll1_vote", > +}; > + > +static const u8 gcc_xo_dsibyte_map[] = { > + [P_XO] = 0, > + [P_DSI0_PHYPLL_BYTE] = 2, > +}; This table has a hole because P_XO is 0 and P_DSI0_PHYPLL_BYTE is 2. In clk_rcg2_get_parent() we'll just iterate over the number of parents and index into this map from 0 to number of parents which unfortunately won't work. Is it time to rethink that code and these index tables? It might be easier to just make the P_* defines into an enum and then iterate over a tuple of { P_* , hw mux index } instead. It would certainly make this type of problem go away. Some other map tables here have the same problem. > + > +static const char *gcc_xo_dsibyte[] = { > + "xo", > + "dsi0pllbyte", > +}; > + > +static const u8 gcc_xo_gpll0a_dsibyte_map[] = { > + [P_XO] = 0, > + [P_GPLL0_AUX] = 2, > + [P_DSI0_PHYPLL_BYTE] = 1, > +}; > + > +static const char *gcc_xo_gpll0a_dsibyte[] = { > + "xo", > + "gpll0_vote", > + "dsi0pllbyte", > +}; > + > +static const u8 gcc_xo_gpll0_dsiphy_map[] = { > + [P_XO] = 0, > + [P_GPLL0] = 1, > + [P_DSI0_PHYPLL_DSI] = 2, > +}; > + > +static const char *gcc_xo_gpll0_dsiphy[] = { > + "xo", > + "gpll0_vote", > + "dsi0pll", > +}; > + > +static const u8 gcc_xo_gpll0a_dsiphy_map[] = { > + [P_XO] = 0, > + [P_GPLL0_AUX] = 2, > + [P_DSI0_PHYPLL_DSI] = 1, > +}; > + > +static const char *gcc_xo_gpll0a_dsiphy[] = { > + "xo", > + "gpll0_vote", > + "dsi0pll", > +}; > + > +static const u8 gcc_xo_gpll0a_gpll1_gpll2_map[] = { > + [P_XO] = 0, > + [P_GPLL0_AUX] = 1, > + [P_GPLL1] = 3, > + [P_GPLL2] = 2, > +}; > + > +static const char *gcc_xo_gpll0a_gpll1_gpll2[] = { > + "xo", > + "gpll0_vote", > + "gpll1_vote", > + "gpll2_vote", > +}; > + [...] > + > +static struct clk_pll bimc_pll = { > + .l_reg = 0x23004, > + .m_reg = 0x23008, > + .n_reg = 0x2300c, > + .config_reg = 0x23014, > + .mode_reg = 0x23000, > + .status_reg = 0x2301c, > + .status_bit = 17, > + .clkr.hw.init = &(struct clk_init_data){ > + .name = "bimc_pll", > + .parent_names = (const char *[]){ "xo" }, > + .num_parents = 1, > + .ops = &clk_pll_ops, > + }, > +}; > + > +static struct clk_regmap bimc_pll_vote = { > + .enable_reg = 0x45000, > + .enable_mask = BIT(3), > + .hw.init = &(struct clk_init_data){ > + .name = "bimc_pll_vote", > + .parent_names = (const char *[]){ "bimc_pll" }, > + .num_parents = 1, > + .ops = &clk_pll_vote_ops, > + }, > +}; I guess this is ok, but it makes me uneasy. We don't do any bimc PLL voting downstream because this PLL is completely under the control of the RPM. For all we know, the RPM hasn't configured the PLL to be in FSM voting mode so this may not even work. Furthermore, if we have clk_pll_ops then we'll go and try to turn off the PLL when the last software entity on the kernel side is done using it. Unfortunately, this PLL may be used by something else that the RPM is managing and so turning it off is going to break things. We mostly need this here to get the right rate for the bus clocks (which are usually constantly changing rate anyway so modeling it in the kernel is ok but not perfect). The best solution is probably to add some read-only PLL ops (clk_pll_ro_ops?) that we can put on the bimc_pll and drop the voting thing completely. The read-only ops would just detect the rate of the PLL and not support anything else. [...] > + > +static int gcc_msm8916_probe(struct platform_device *pdev) > +{ [..] > + > + regmap = qcom_cc_map(pdev, &gcc_msm8916_desc); > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); > + > + return qcom_cc_really_probe(pdev, &gcc_msm8916_desc, regmap); We can collapse this into a single qcom_cc_probe() now? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project