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

[Georgi Djakov <georgi.djakov@linaro.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