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

Groups > linux.kernel > #1668508 > unrolled thread

[PATCH V1 0/9] add clock driver for Spreadtrum platforms

Started byChunyan Zhang <chunyan.zhang@spreadtrum.com>
First post2017-06-18 04:10 +0200
Last post2017-06-30 03:50 +0200
Articles 4 on this page of 24 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH V1 0/9] add clock driver for Spreadtrum platforms Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-06-18 04:10 +0200
    [PATCH V1 2/9] clk: sprd: Add common infrastructure Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-06-18 04:10 +0200
      Re: [PATCH V1 2/9] clk: sprd: Add common infrastructure Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:30 +0200
        Re: [PATCH V1 2/9] clk: sprd: Add common infrastructure Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:20 +0200
    [PATCH V1 9/9] arm64: dts: add ccu for SC9860 Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-06-18 04:10 +0200
      Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:30 +0200
        Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:30 +0200
          Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 03:00 +0200
            Re: [PATCH V1 9/9] arm64: dts: add ccu for SC9860 Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-30 09:40 +0200
    [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <chunyan.zhang@spreadtrum.com> - 2017-06-18 04:10 +0200
      Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:40 +0200
        Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:20 +0200
          Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Arnd Bergmann <arnd@arndb.de> - 2017-06-22 13:20 +0200
            Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 14:10 +0200
          Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 03:50 +0200
            Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-30 10:00 +0200
              Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 21:30 +0200
                Re: [PATCH V1 7/9] clk: sprd: add adjustable pll support Chunyan Zhang <zhang.lyra@gmail.com> - 2017-07-03 09:50 +0200
    Re: [PATCH V1 0/9] add clock driver for Spreadtrum platforms Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 03:30 +0200
      Re: [PATCH V1 0/9] add clock driver for Spreadtrum platforms Chunyan Zhang <zhang.lyra@gmail.com> - 2017-06-22 12:10 +0200
        Re: [PATCH V1 0/9] add clock driver for Spreadtrum platforms Stephen Boyd <sboyd@codeaurora.org> - 2017-06-30 02:50 +0200
    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

Page 2 of 2 — ← Prev page 1 [2]

#1678348

FromStephen Boyd <sboyd@codeaurora.org>
Date2017-06-30 02:50 +0200
Message-ID<tXSie-6BV-25@gated-at.bofh.it>
In reply to#1672486
On 06/22, Chunyan Zhang wrote:
> Hi Stephen,
> 
> On 20 June 2017 at 09:25, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/18, Chunyan Zhang wrote:
> >> In the last cycle, the patches support Whale2 sc9860 mobile chip have been
> >> merged. This patchset adds clock driver which is used on almost all
> >> Spreadtrum SoCs.
> >>
> >> This is a rewrite of Spreadtrum's original clock driver[1] according to the
> >> comments[2] from Stephen Boyd.
> >>
> >> This series also adds Spreadtrum clock binding documentation and devicetree
> >> data.
> >>
> >> Any comments would be greatly appreciated.
> >
> > Overall it seems to copy quite a bit of code from sunxi-ng, which
> > is OK, but if that's just copy/paste + replace some names then
> > perhaps we should consolidate the two implementations into one
> > that both SoCs can use.
> >
> 
> OK, will try.

Ok. Please don't spend too much time on it though. 

> 
> > Also, is there any reason why we can't use a platform device
> > driver for this instead of the DT probing mechanism? That is more
> > preferred method of probing clk controllers.
> 
> From what I have known on ARM platforms, device drivers cannot
> recognize out which SoC the driver is running on, assume that the
> device on different SoC has some differences.  To make one only kernel
> Image can be used on all SoCs of Spreadtrum, we selected the way of
> loading different dtb for each SoC.

Device drivers can figure out what device the driver is bound to
based on the compatible string of the node. Typically, the clk
driver binds to a device node with a compatible indicating the
clock controller it is, like spd,soc-name-clk-controller-name.
Then that can be used to determine what sort of associated data
there is.

> 
> Actually, I haven't understood the merits of moving more clk things to
> driver from DT, could you please introduce more about that?
> 
> 

Some mailing list digging may be helpful, but I admit I need to
have some sort of canned response here that I can just repeat
each time this comes up. Here it goes.

We really only need CLK_OF_DECLARE() if a clk needs to be
available for timers or interrupt controllers. Otherwise, its
possible to put the rest of the clk tree registration in the
normal device driver path.

Reasons (in no particular order):

  1. We get a dev pointer to use with clk_hw_register()

  2. We can handle probe defer if some resource is not available

  3. Using device model gets us a hook into power management frameworks
     like runtime PM and system PM for things like suspend and hibernate

  4. It encourages a single DT node clk controller style binding
     instead of a single node per clk style binding

  5. We can use non-DT specific functions like devm_ioremap_resource() to map
     registers and acquire other resources, leading to more portable and
     generic code

  6. We may be able to make the device driver a module, which will
     make distros happy if we don't have to compile in all
     these clk drivers to the resulting vmlinux

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

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

#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>
In reply to#1668508
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] | [prev] | [next] | [standalone]

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

FromChunyan Zhang <zhang.lyra@gmail.com>
Date2017-06-22 12:30 +0200
SubjectRe: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860
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 — Re: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860

FromStephen Boyd <sboyd@codeaurora.org>
Date2017-06-30 03:50 +0200
SubjectRe: [PATCH V1 8/9] clk: sprd: add clocks support for SC9860
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]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web