Re: [PATCH v2] clk: qcom: Add MSM8916 Global Clock Controller support

[Stephen Boyd <sboyd@codeaurora.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