From mboxrd@z Thu Jan  1 00:00:00 1970
From: Georgi Djakov <georgi.djakov@linaro.org>
Subject: Re: [PATCH v2] clk: qcom: Add MSM8916 Global Clock Controller support
Date: Mon, 09 Mar 2015 19:26:25 +0200
Message-ID: <54FDD7C1.6090704@linaro.org>
References: <1424882699-32758-1-git-send-email-georgi.djakov@linaro.org> <20150305195838.GA11174@codeaurora.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Return-path: <linux-arm-msm-owner@vger.kernel.org>
Received: from mail-wi0-f175.google.com ([209.85.212.175]:41152 "EHLO
	mail-wi0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751702AbbCIR0G (ORCPT
	<rfc822;linux-arm-msm@vger.kernel.org>);
	Mon, 9 Mar 2015 13:26:06 -0400
Received: by wiwl15 with SMTP id l15so23355864wiw.0
        for <linux-arm-msm@vger.kernel.org>; Mon, 09 Mar 2015 10:26:05 -0700 (PDT)
In-Reply-To: <20150305195838.GA11174@codeaurora.org>
Sender: linux-arm-msm-owner@vger.kernel.org
List-Id: linux-arm-msm@vger.kernel.org
To: Stephen Boyd <sboyd@codeaurora.org>
Cc: mturquette@linaro.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org

Hi Stephen,
Thanks for looking into this.

On 03/05/2015 09:58 PM, Stephen Boyd wrote:
> 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.
> 

I am not sure that i understand your suggestion. It seems that
changing clk_rcg2_get_parent() and also the above map structs is
the best way to resolve this. Other solution requiring minimal
changes is to fill the holes with a magic value, that we will skip
when iterating, but this is not elegant at all.

BR,
Georgi