Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670165 > unrolled thread

Re: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860

Started byStephen Boyd <sboyd@codeaurora.org>
First post2017-06-20 03:50 +0200
Last post2017-06-30 03:50 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860 Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:50 +0200
    Re: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860 Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:30 +0200
      Re: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860 Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 03:50 +0200

#1670165 — Re: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860

FromStephen Boyd <sboyd@codeaurora.org>
Date2017-06-20 03:50 +0200
SubjectRe: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860
Message-ID<tUgsO-247-13@gated-at.bofh.it>
On 06/18, Chunyan Zhang wrote:
> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
> index c593a93..0d90b40 100644
> --- a/drivers/clk/sprd/Makefile
> +++ b/drivers/clk/sprd/Makefile
> @@ -1,3 +1,4 @@
>  ifneq ($(CONFIG_OF),)
>  obj-y	+= ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o
> +obj-y	+= ccu-sc9860.o

And a Kconfig for this SoC specific driver.

>  endif
> diff --git a/drivers/clk/sprd/ccu-sc9860.c b/drivers/clk/sprd/ccu-sc9860.c
> new file mode 100644
> index 0000000..6cd4dc5
> --- /dev/null
> +++ b/drivers/clk/sprd/ccu-sc9860.c
> +
> +static CLK_FIXED_FACTOR(fac_4m,		"fac-4m",	"ext-26m",
> +			6, 1, CLK_IS_BASIC);
> +static CLK_FIXED_FACTOR(fac_2m,		"fac-2m",	"ext-26m",
> +			13, 1, CLK_IS_BASIC);
> +static CLK_FIXED_FACTOR(fac_1m,		"fac-1m",	"ext-26m",
> +			26, 1, CLK_IS_BASIC);
> +static CLK_FIXED_FACTOR(fac_250k,	"fac-250k",	"ext-26m",
> +			104, 1, CLK_IS_BASIC);
> +static CLK_FIXED_FACTOR(fac_rpll0_26m,	"rpll0-26m",	"ext-26m",
> +			1, 1, CLK_IS_BASIC);
> +static CLK_FIXED_FACTOR(fac_rpll1_26m,	"rpll1-26m",	"ext-26m",
> +			1, 1, CLK_IS_BASIC);
> +static CLK_FIXED_FACTOR(fac_rco_25m,	"rco-25m",	"ext-rc0-100m",
> +			4, 1, CLK_IS_BASIC);
> +static CLK_FIXED_FACTOR(fac_rco_4m,	"rco-4m",	"ext-rc0-100m",
> +			25, 1, CLK_IS_BASIC);
> +static CLK_FIXED_FACTOR(fac_rco_2m,	"rco-2m",	"ext-rc0-100m",
> +			50, 1, CLK_IS_BASIC);
> +static CLK_FIXED_FACTOR(fac_3k2,	"fac-3k2",	"ext-32k",
> +			10, 1, CLK_IS_BASIC);
> +static CLK_FIXED_FACTOR(fac_1k,		"fac-1k",	"ext-32k",
> +			32, 1, CLK_IS_BASIC);
> +
> +#define SC9860_GATE_FLAGS (CLK_IGNORE_UNUSED | CLK_IS_BASIC)

No CLK_IS_BASIC. Why is everything marked as CLK_IGNORE_UNUSED?

> +static SPRD_CCU_GATE(rpll0_gate,	"rpll0-gate",	"ext-26m", 0x402b016c,
> +		     0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE(rpll1_gate,	"rpll1-gate",	"ext-26m", 0x402b016c,
> +		     0x1000, BIT(18), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE(mpll0_gate,	"mpll0-gate",	"ext-26m", 0x402b00b0,
> +		     0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE(mpll1_gate,	"mpll1-gate",	"ext-26m", 0x402b00b0,
> +		     0x1000, BIT(18), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE(dpll0_gate,	"dpll0-gate",	"ext-26m", 0x402b00b4,
> +		     0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE(dpll1_gate,	"dpll1-gate",	"ext-26m", 0x402b00b4,
> +		     0x1000, BIT(18), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE(gpll_gate,		"gpll-gate",	"ext-26m", 0x402b032c,
> +		     0x1000, BIT(0), SC9860_GATE_FLAGS,
> +		     CLK_GATE_SET_TO_DISABLE);
> +static SPRD_CCU_GATE(cppll_gate,	"cppll-gate",	"ext-26m", 0x402b02b4,
> +		     0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE(ltepll0_gate,	"ltepll0-gate",	"ext-26m", 0x402b00b8,
> +		     0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE(ltepll1_gate,	"ltepll1-gate",	"ext-26m", 0x402b010c,
> +		     0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE(twpll_gate,	"twpll-gate",	"ext-26m", 0x402b00bc,
> +		     0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE_NO_PARENT(sdio0_2x_en,	"sdio0-2x-en",	0x402e013c,
> +			       0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE_NO_PARENT(sdio0_1x_en,	"sdio0-1x-en",	0x402e013c,
> +			       0x1000, BIT(3), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE_NO_PARENT(sdio1_2x_en,	"sdio1-2x-en",	0x402e013c,
> +			       0x1000, BIT(4), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE_NO_PARENT(sdio1_1x_en,	"sdio1-1x-en",	0x402e013c,
> +			       0x1000, BIT(5), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE_NO_PARENT(sdio2_2x_en,	"sdio2-2x-en",	0x402e013c,
> +			       0x1000, BIT(6), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE_NO_PARENT(sdio2_1x_en,	"sdio2-1x-en",	0x402e013c,
> +			       0x1000, BIT(7), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE_NO_PARENT(emmc_1x_en,	"emmc-1x-en",	0x402e013c,
> +			       0x1000, BIT(8), SC9860_GATE_FLAGS, 0);
> +static SPRD_CCU_GATE_NO_PARENT(emmc_2x_en,	"emmc-2x-en",	0x402e013c,
> +			       0x1000, BIT(9), SC9860_GATE_FLAGS, 0);
> +
> +/* GPLL/LPLL/DPLL/RPLL/CPLL */
> +static const u64 const itable1[4] = {3, 780000000, 988000000, 1196000000};
> +
> +/* TWPLL/MPLL0/MPLL1 */
> +static const u64 itable2[4] = {3, 1638000000, 2080000000, 2600000000UL};
> +
> +static const struct ccu_bit_field const f_rpll[PLL_FACT_MAX] = {
> +	{ .shift = 0,	.width = 1 },	/* lock_done	*/
> +	{ .shift = 3,	.width = 1 },	/* div_s	*/
> +	{ .shift = 80,	.width = 1 },	/* mod_en	*/

Are they even shifts? Or offsets from some base? I have to go
back and read the other patch.

> +	{ .shift = 81,	.width = 1 },	/* sdm_en	*/
> +	{ .shift = 0,	.width = 0 },	/* refin	*/
> +	{ .shift = 14,	.width = 2 },	/* ibias	*/
> +	{ .shift = 16,	.width = 7 },	/* n		*/
> +	{ .shift = 4,	.width = 7 },	/* nint		*/
> +	{ .shift = 32,	.width = 23},	/* kint		*/
> +	{ .shift = 0,	.width = 0 },	/* prediv	*/
> +	{ .shift = 0,	.width = 0 },	/* postdiv	*/
> +};
> +static const u32 const regs_rpll0[4] = { 3, 0x44, 0x48, 0x4c };
> +static SPRD_CCU_PLL_WITH_ITABLE(rpll0_clk, "rpll0", "rpll0-gate", 0x40400044,
> +				regs_rpll0, itable1, 200, f_rpll);
> +
> +static const u32 const regs_rpll1[4] = { 3, 0x50, 0x54, 0x58 };
> +static SPRD_CCU_PLL_WITH_ITABLE(rpll1_clk, "rpll1", "rpll1-gate", 0x40400050,
> +				regs_rpll1, itable1, 200, f_rpll);
> +
> +static const struct ccu_bit_field const f_mpll0[PLL_FACT_MAX] = {
[...]
> diff --git a/include/dt-bindings/clock/sc9860-ccu.h b/include/dt-bindings/clock/sc9860-ccu.h
> new file mode 100644
> index 0000000..dd7ccf9
> --- /dev/null
> +++ b/include/dt-bindings/clock/sc9860-ccu.h
> @@ -0,0 +1,19 @@
> +/*
> + * Spreadtrum SC9860 platform clocks
> + *
> + * Copyright (C) 2017, Spreadtrum Communications Inc.
> + *
> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> + */
> +
> +#ifndef _DT_BINDINGS_CLK_SC9860_CCU_H_
> +#define _DT_BINDINGS_CLK_SC9860_CCU_H_
> +
> +#define CLK_FAC_1M	2
> +#define CLK_EMMC_2X_EN	29
> +#define CLK_L0_409M6	60
> +#define CLK_EMMC_2X	88
> +#define CLK_EMMC_EB	158

Why are only a handful exposed in the header file? Not exposing
everything is mostly a maintenance nightmare right now.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1672511

FromChunyan Zhang <zhang.lyra@gmail.com>
Date2017-06-22 12:30 +0200
Message-ID<tV7x8-3ll-19@gated-at.bofh.it>
In reply to#1670165
Hi Stephen,

On 20 June 2017 at 09:41, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/18, Chunyan Zhang wrote:
>> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
>> index c593a93..0d90b40 100644
>> --- a/drivers/clk/sprd/Makefile
>> +++ b/drivers/clk/sprd/Makefile
>> @@ -1,3 +1,4 @@
>>  ifneq ($(CONFIG_OF),)
>>  obj-y        += ccu_common.o ccu_gate.o ccu_mux.o ccu_div.o ccu_composite.o ccu_pll.o
>> +obj-y        += ccu-sc9860.o
>
> And a Kconfig for this SoC specific driver.

Ok.

>
>>  endif
>> diff --git a/drivers/clk/sprd/ccu-sc9860.c b/drivers/clk/sprd/ccu-sc9860.c
>> new file mode 100644
>> index 0000000..6cd4dc5
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu-sc9860.c
>> +
>> +static CLK_FIXED_FACTOR(fac_4m,              "fac-4m",       "ext-26m",
>> +                     6, 1, CLK_IS_BASIC);
>> +static CLK_FIXED_FACTOR(fac_2m,              "fac-2m",       "ext-26m",
>> +                     13, 1, CLK_IS_BASIC);
>> +static CLK_FIXED_FACTOR(fac_1m,              "fac-1m",       "ext-26m",
>> +                     26, 1, CLK_IS_BASIC);
>> +static CLK_FIXED_FACTOR(fac_250k,    "fac-250k",     "ext-26m",
>> +                     104, 1, CLK_IS_BASIC);
>> +static CLK_FIXED_FACTOR(fac_rpll0_26m,       "rpll0-26m",    "ext-26m",
>> +                     1, 1, CLK_IS_BASIC);
>> +static CLK_FIXED_FACTOR(fac_rpll1_26m,       "rpll1-26m",    "ext-26m",
>> +                     1, 1, CLK_IS_BASIC);
>> +static CLK_FIXED_FACTOR(fac_rco_25m, "rco-25m",      "ext-rc0-100m",
>> +                     4, 1, CLK_IS_BASIC);
>> +static CLK_FIXED_FACTOR(fac_rco_4m,  "rco-4m",       "ext-rc0-100m",
>> +                     25, 1, CLK_IS_BASIC);
>> +static CLK_FIXED_FACTOR(fac_rco_2m,  "rco-2m",       "ext-rc0-100m",
>> +                     50, 1, CLK_IS_BASIC);
>> +static CLK_FIXED_FACTOR(fac_3k2,     "fac-3k2",      "ext-32k",
>> +                     10, 1, CLK_IS_BASIC);
>> +static CLK_FIXED_FACTOR(fac_1k,              "fac-1k",       "ext-32k",
>> +                     32, 1, CLK_IS_BASIC);
>> +
>> +#define SC9860_GATE_FLAGS (CLK_IGNORE_UNUSED | CLK_IS_BASIC)
>
> No CLK_IS_BASIC. Why is everything marked as CLK_IGNORE_UNUSED?

Copied from the original implementation :)
But I will do a double check, there'are indeed some clocks which
shouldn't be marked as CLK_IGNORE_UNUSED.

>
>> +static SPRD_CCU_GATE(rpll0_gate,     "rpll0-gate",   "ext-26m", 0x402b016c,
>> +                  0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE(rpll1_gate,     "rpll1-gate",   "ext-26m", 0x402b016c,
>> +                  0x1000, BIT(18), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE(mpll0_gate,     "mpll0-gate",   "ext-26m", 0x402b00b0,
>> +                  0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE(mpll1_gate,     "mpll1-gate",   "ext-26m", 0x402b00b0,
>> +                  0x1000, BIT(18), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE(dpll0_gate,     "dpll0-gate",   "ext-26m", 0x402b00b4,
>> +                  0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE(dpll1_gate,     "dpll1-gate",   "ext-26m", 0x402b00b4,
>> +                  0x1000, BIT(18), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE(gpll_gate,              "gpll-gate",    "ext-26m", 0x402b032c,
>> +                  0x1000, BIT(0), SC9860_GATE_FLAGS,
>> +                  CLK_GATE_SET_TO_DISABLE);
>> +static SPRD_CCU_GATE(cppll_gate,     "cppll-gate",   "ext-26m", 0x402b02b4,
>> +                  0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE(ltepll0_gate,   "ltepll0-gate", "ext-26m", 0x402b00b8,
>> +                  0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE(ltepll1_gate,   "ltepll1-gate", "ext-26m", 0x402b010c,
>> +                  0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE(twpll_gate,     "twpll-gate",   "ext-26m", 0x402b00bc,
>> +                  0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE_NO_PARENT(sdio0_2x_en,  "sdio0-2x-en",  0x402e013c,
>> +                            0x1000, BIT(2), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE_NO_PARENT(sdio0_1x_en,  "sdio0-1x-en",  0x402e013c,
>> +                            0x1000, BIT(3), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE_NO_PARENT(sdio1_2x_en,  "sdio1-2x-en",  0x402e013c,
>> +                            0x1000, BIT(4), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE_NO_PARENT(sdio1_1x_en,  "sdio1-1x-en",  0x402e013c,
>> +                            0x1000, BIT(5), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE_NO_PARENT(sdio2_2x_en,  "sdio2-2x-en",  0x402e013c,
>> +                            0x1000, BIT(6), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE_NO_PARENT(sdio2_1x_en,  "sdio2-1x-en",  0x402e013c,
>> +                            0x1000, BIT(7), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE_NO_PARENT(emmc_1x_en,   "emmc-1x-en",   0x402e013c,
>> +                            0x1000, BIT(8), SC9860_GATE_FLAGS, 0);
>> +static SPRD_CCU_GATE_NO_PARENT(emmc_2x_en,   "emmc-2x-en",   0x402e013c,
>> +                            0x1000, BIT(9), SC9860_GATE_FLAGS, 0);
>> +
>> +/* GPLL/LPLL/DPLL/RPLL/CPLL */
>> +static const u64 const itable1[4] = {3, 780000000, 988000000, 1196000000};
>> +
>> +/* TWPLL/MPLL0/MPLL1 */
>> +static const u64 itable2[4] = {3, 1638000000, 2080000000, 2600000000UL};
>> +
>> +static const struct ccu_bit_field const f_rpll[PLL_FACT_MAX] = {
>> +     { .shift = 0,   .width = 1 },   /* lock_done    */
>> +     { .shift = 3,   .width = 1 },   /* div_s        */
>> +     { .shift = 80,  .width = 1 },   /* mod_en       */
>
> Are they even shifts? Or offsets from some base? I have to go
> back and read the other patch.

You must've noticed that each PLL has a structure of "ccu_bit_field
*factors", each pair of shift - width represents a bit field in
registers, each bit field stores a factor used to calculate PLL clock
rate, but different PLL clock has different bit fields arrangement due
to hardware design.

>
>> +     { .shift = 81,  .width = 1 },   /* sdm_en       */
>> +     { .shift = 0,   .width = 0 },   /* refin        */
>> +     { .shift = 14,  .width = 2 },   /* ibias        */
>> +     { .shift = 16,  .width = 7 },   /* n            */
>> +     { .shift = 4,   .width = 7 },   /* nint         */
>> +     { .shift = 32,  .width = 23},   /* kint         */
>> +     { .shift = 0,   .width = 0 },   /* prediv       */
>> +     { .shift = 0,   .width = 0 },   /* postdiv      */
>> +};
>> +static const u32 const regs_rpll0[4] = { 3, 0x44, 0x48, 0x4c };
>> +static SPRD_CCU_PLL_WITH_ITABLE(rpll0_clk, "rpll0", "rpll0-gate", 0x40400044,
>> +                             regs_rpll0, itable1, 200, f_rpll);
>> +
>> +static const u32 const regs_rpll1[4] = { 3, 0x50, 0x54, 0x58 };
>> +static SPRD_CCU_PLL_WITH_ITABLE(rpll1_clk, "rpll1", "rpll1-gate", 0x40400050,
>> +                             regs_rpll1, itable1, 200, f_rpll);
>> +
>> +static const struct ccu_bit_field const f_mpll0[PLL_FACT_MAX] = {
> [...]
>> diff --git a/include/dt-bindings/clock/sc9860-ccu.h b/include/dt-bindings/clock/sc9860-ccu.h
>> new file mode 100644
>> index 0000000..dd7ccf9
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/sc9860-ccu.h
>> @@ -0,0 +1,19 @@
>> +/*
>> + * Spreadtrum SC9860 platform clocks
>> + *
>> + * Copyright (C) 2017, Spreadtrum Communications Inc.
>> + *
>> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> + */
>> +
>> +#ifndef _DT_BINDINGS_CLK_SC9860_CCU_H_
>> +#define _DT_BINDINGS_CLK_SC9860_CCU_H_
>> +
>> +#define CLK_FAC_1M   2
>> +#define CLK_EMMC_2X_EN       29
>> +#define CLK_L0_409M6 60
>> +#define CLK_EMMC_2X  88
>> +#define CLK_EMMC_EB  158
>
> Why are only a handful exposed in the header file? Not exposing
> everything is mostly a maintenance nightmare right now.

No special reason here, my thought simply was that there's no much
Spreadtrum's device driver in mainline at present, so most of the
clocks wouldn't be needed for now, I planned to expose only those when
the device driver they provide clock to, that's saying when we add a
new device driver we'll expose the clocks this device needs.

Another reason is, I'm not sure if there are still some clocks I
haven't listed for SC9860 , so if we need to add a clock in the
feature, the value of these macros defined in
"include/dt-bindings/clock/sc9860-ccu.h" may be changed, that means I
need to change the index of all clocks following the clock inserted
into.

But why will not exposing everything bring trouble to maintenance? :)


Thank you for your review,
Chunyan

>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1678383

FromStephen Boyd <sboyd@codeaurora.org>
Date2017-06-30 03:50 +0200
Message-ID<tXTeh-7bI-5@gated-at.bofh.it>
In reply to#1672511
On 06/22, Chunyan Zhang wrote:
> Hi Stephen,
> 
> On 20 June 2017 at 09:41, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/18, Chunyan Zhang wrote:
> >> +++ b/include/dt-bindings/clock/sc9860-ccu.h
> >> @@ -0,0 +1,19 @@
> >> +/*
> >> + * Spreadtrum SC9860 platform clocks
> >> + *
> >> + * Copyright (C) 2017, Spreadtrum Communications Inc.
> >> + *
> >> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> + */
> >> +
> >> +#ifndef _DT_BINDINGS_CLK_SC9860_CCU_H_
> >> +#define _DT_BINDINGS_CLK_SC9860_CCU_H_
> >> +
> >> +#define CLK_FAC_1M   2
> >> +#define CLK_EMMC_2X_EN       29
> >> +#define CLK_L0_409M6 60
> >> +#define CLK_EMMC_2X  88
> >> +#define CLK_EMMC_EB  158
> >
> > Why are only a handful exposed in the header file? Not exposing
> > everything is mostly a maintenance nightmare right now.
> 
> No special reason here, my thought simply was that there's no much
> Spreadtrum's device driver in mainline at present, so most of the
> clocks wouldn't be needed for now, I planned to expose only those when
> the device driver they provide clock to, that's saying when we add a
> new device driver we'll expose the clocks this device needs.

Please no. I don't want to get the one-off patch in my inbox to
add more and more ids here "just because" nobody was using the
numbers before. I really don't care that someone has added the
driver for some random device on their SoC now and so they need
to expose the number into a header file. I know some people are
doing this, and it's really not meaningful. I'm much happier if
we're exposing every single clk number to DT and reducing churn
in the include/dt-bindings/ directory. And really, the raw number
could be used in dt, so the enforcement of any sort of ABI needs
to be in the clk driver anyway.

Now if a clk was left out of the clk driver implementation, then
the number could be left out of the header file, but even then it
doesn't really need to be. The clk driver could be implemented in
a way to return an error if the number doesn't map to a clk_hw
structure in the driver. In the future, the clk could be added to
the driver, and then DT wouldn't need to change and consumer
drivers would start to work when various branches are merged
together.

Also, sometimes clk driver authors don't know about all the clks
that they have on their hardware (I doubt this includes you
because you work at the company making the SoC here). In this
case, it's OK to leave out the ids that aren't known to the
binding author because we can't expect more from these people.
And if you know for a fact that a certain clk will never need to
be exposed in the binding, like some random internal clk that
nobody will care to use, then it's also OK to leave that out from
the binding.

> 
> Another reason is, I'm not sure if there are still some clocks I
> haven't listed for SC9860 , so if we need to add a clock in the
> feature, the value of these macros defined in
> "include/dt-bindings/clock/sc9860-ccu.h" may be changed, that means I
> need to change the index of all clocks following the clock inserted
> into.

When modifying a dt-bindings header file to add more clk ids you
should _never_ modify existing numbers. That would be a backwards
incompatible change of the DT binding. If anything, just keep
adding more numbers to the end of the number space. If something
needs to be removed, make that number map to an error or some
no-op clk_hw structure in the clk driver.

> 
> But why will not exposing everything bring trouble to maintenance? :)
> 

Mostly it's because I spend too much time worrying about these
include/dt-bindings files and how they're going to land in Linus'
tree and not enough time reviewing driver and core framework
patches. I'm trying to reduce the time spent worrying about these
header files to a manageable amount.

Hopefully that's helpful. Sorry for the long email.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web