| Message ID | 1471883463-1950-5-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 08/22/2016 06:31 PM, Sylwester Nawrocki wrote: > This patch adds code instantiating the EPLL, which is used as the > audio subsystem's root clock. > The requirement to specify the external root clock in clocks property > is also added. I think the requirement was there already but explained little bit differently: 19 External clock: 20 21 There is clock that is generated outside the SoC. It 22 is expected that it is defined using standard clock bindings 23 with following clock-output-name: 24 25 - "fin_pll" - PLL input clock from XXTI so you can just combine them. Driver now will require the fin_pll in two ways: 1. Old lookup by "fin_pll" name. 2. of_clk_get of yours. > That ensures proper initialization order by explicitly > specifying dependencies in devicetree. It prevents situations when the > SoC's clock controller driver has initialized, the external oscillator > clock is not yet registered and setting clock frequencies through > assigned-clock-rates property doesn't work properly due to unknown > external oscillator frequency. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > .../devicetree/bindings/clock/exynos5410-clock.txt | 14 +++ > drivers/clk/samsung/clk-exynos5410.c | 30 +++++- > drivers/clk/samsung/clk-pll.c | 102 +++++++++++++++++++++ > drivers/clk/samsung/clk-pll.h | 1 + > 4 files changed, 145 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt > index aeab635..2aefc07 100644 > --- a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt > +++ b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt > @@ -12,6 +12,9 @@ Required Properties: > > - #clock-cells: should be 1. > > +- clocks: should contain an entry specifying the root oscillator clock > + on XXTI pin (fin_pll). Combine with "external clock". Rest looks ok: Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/30/2016 12:36 PM, Krzysztof Kozlowski wrote: > On 08/22/2016 06:31 PM, Sylwester Nawrocki wrote: >> This patch adds code instantiating the EPLL, which is used as the >> audio subsystem's root clock. >> The requirement to specify the external root clock in clocks property >> is also added. > > I think the requirement was there already but explained little bit > differently: > 19 External clock: > 20 > 21 There is clock that is generated outside the SoC. It > 22 is expected that it is defined using standard clock bindings > 23 with following clock-output-name: > 24 > 25 - "fin_pll" - PLL input clock from XXTI > > so you can just combine them. Driver now will require the fin_pll in two > ways: > 1. Old lookup by "fin_pll" name. > 2. of_clk_get of yours. I'm not sure what do you mean exactly, I'm also adding in this patch a sentence right after the text as quoted above saying that: "This clock should be listed in the clocks property of the controller node." The description of the consumer clock (the main clock controller's parent clock) was not in the binding so far, there is no "clocks" property in the list of required properties. >> That ensures proper initialization order by explicitly >> specifying dependencies in devicetree. It prevents situations when the >> SoC's clock controller driver has initialized, the external oscillator >> clock is not yet registered and setting clock frequencies through >> assigned-clock-rates property doesn't work properly due to unknown >> external oscillator frequency. >> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> >> --- >> .../devicetree/bindings/clock/exynos5410-clock.txt | 14 +++ >> drivers/clk/samsung/clk-exynos5410.c | 30 +++++- >> drivers/clk/samsung/clk-pll.c | 102 +++++++++++++++++++++ >> drivers/clk/samsung/clk-pll.h | 1 + >> 4 files changed, 145 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt >> index aeab635..2aefc07 100644 >> --- a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt >> +++ b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt >> @@ -12,6 +12,9 @@ Required Properties: >> >> - #clock-cells: should be 1. >> >> +- clocks: should contain an entry specifying the root oscillator clock >> + on XXTI pin (fin_pll). > > Combine with "external clock". Do you mean rephrasing this to something like: - clocks: should contain an entry specifying the external clock (fin_pll), i.e. the root clock on XXTI pin. ? -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 02, 2016 at 05:15:31PM +0200, Sylwester Nawrocki wrote: > On 08/30/2016 12:36 PM, Krzysztof Kozlowski wrote: > > On 08/22/2016 06:31 PM, Sylwester Nawrocki wrote: > >> This patch adds code instantiating the EPLL, which is used as the > >> audio subsystem's root clock. > >> The requirement to specify the external root clock in clocks property > >> is also added. > > > > I think the requirement was there already but explained little bit > > differently: > > 19 External clock: > > 20 > > 21 There is clock that is generated outside the SoC. It > > 22 is expected that it is defined using standard clock bindings > > 23 with following clock-output-name: > > 24 > > 25 - "fin_pll" - PLL input clock from XXTI > > > > so you can just combine them. Driver now will require the fin_pll in two > > ways: > > 1. Old lookup by "fin_pll" name. > > 2. of_clk_get of yours. > > I'm not sure what do you mean exactly, I'm also adding in this patch a sentence > right after the text as quoted above saying that: > > "This clock should be listed in the clocks property of the controller node." > > The description of the consumer clock (the main clock controller's parent clock) > was not in the binding so far, there is no "clocks" property in the list of > required properties. First of all, in commit message, you are not adding the requirement... it was present already, I think. Wasn't it? As for the code, I am saying that you are duplicating the information. There is already an paragraph for external clock. You are adding a new one before and extending it... just make it simpler - mention external clock in one place. > > >> That ensures proper initialization order by exlicitly > >> specifying dependencies in devicetree. It prevents situations when the > >> SoC's clock controller driver has initialized, the external oscillator > >> clock is not yet registered and setting clock frequencies through > >> assigned-clock-rates property doesn't work properly due to unknown > >> external oscillator frequency. > >> > >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > >> --- > >> .../devicetree/bindings/clock/exynos5410-clock.txt | 14 +++ > >> drivers/clk/samsung/clk-exynos5410.c | 30 +++++- > >> drivers/clk/samsung/clk-pll.c | 102 +++++++++++++++++++++ > >> drivers/clk/samsung/clk-pll.h | 1 + > >> 4 files changed, 145 insertions(+), 2 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt > >> index aeab635..2aefc07 100644 > >> --- a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt > >> +++ b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt > >> @@ -12,6 +12,9 @@ Required Properties: > >> > >> - #clock-cells: should be 1. > >> > >> +- clocks: should contain an entry specifying the root oscillator clock > >> + on XXTI pin (fin_pll). > > > > Combine with "external clock". > > Do you mean rephrasing this to something like: > > - clocks: should contain an entry specifying the external clock (fin_pll), > i.e. the root clock on XXTI pin. Could be, with removal of the existing paragraph. The only comment I have is about duplicating the information. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/02/2016 07:32 PM, Krzysztof Kozlowski wrote: > On Fri, Sep 02, 2016 at 05:15:31PM +0200, Sylwester Nawrocki wrote: >> > On 08/30/2016 12:36 PM, Krzysztof Kozlowski wrote: >>> > > On 08/22/2016 06:31 PM, Sylwester Nawrocki wrote: >>>> > >> This patch adds code instantiating the EPLL, which is used as the >>>> > >> audio subsystem's root clock. >>>> > >> The requirement to specify the external root clock in clocks property >>>> > >> is also added. >>> > > >>> > > I think the requirement was there already but explained little bit >>> > > differently: >>> > > 19 External clock: >>> > > 20 >>> > > 21 There is clock that is generated outside the SoC. It >>> > > 22 is expected that it is defined using standard clock bindings >>> > > 23 with following clock-output-name: >>> > > 24 >>> > > 25 - "fin_pll" - PLL input clock from XXTI >>> > > >>> > > so you can just combine them. Driver now will require the fin_pll in two >>> > > ways: >>> > > 1. Old lookup by "fin_pll" name. >>> > > 2. of_clk_get of yours. >> > >> > I'm not sure what do you mean exactly, I'm also adding in this patch a sentence >> > right after the text as quoted above saying that: >> > >> > "This clock should be listed in the clocks property of the controller node." >> > >> > The description of the consumer clock (the main clock controller's parent clock) >> > was not in the binding so far, there is no "clocks" property in the list of >> > required properties. > > First of all, in commit message, you are not adding the requirement... > it was present already, I think. Wasn't it? I guess we could say it was there, but the documentation was unclear enough so that link between the oscillator clock and the main clocks controller is missing in many dts files. > As for the code, I am saying that you are duplicating the information. > There is already an paragraph for external clock. You are adding a new > one before and extending it... just make it simpler - mention external > clock in one place. Ah, now I see, indeed it makes sense to delete the "External clock" paragraph. I'll reword then the clocks property description to: - clocks: should contain an entry specifying the root clock from external oscillator supplied through XXTI or XusbXTI pin. This clock should be defined using standard clock bindings with "fin_pll" clock-output-name. The "fin_pll" clock is being passed internally to the 9 PLLs. -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt index aeab635..2aefc07 100644 --- a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt +++ b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt @@ -12,6 +12,9 @@ Required Properties: - #clock-cells: should be 1. +- clocks: should contain an entry specifying the root oscillator clock + on XXTI pin (fin_pll). + All available clocks are defined as preprocessor macros in dt-bindings/clock/exynos5410.h header and can be used in device tree sources. @@ -24,12 +27,23 @@ with following clock-output-name: - "fin_pll" - PLL input clock from XXTI +This clock should be listed in the clocks property of the controller node. + + Example 1: An example of a clock controller node is listed below. + fin_pll: xxti { + compatible = "fixed-clock"; + clock-frequency = <24000000>; + clock-output-names = "fin_pll"; + #clock-cells = <0>; + }; + clock: clock-controller@0x10010000 { compatible = "samsung,exynos5410-clock"; reg = <0x10010000 0x30000>; #clock-cells = <1>; + clocks = <&fin_pll>; }; Example 2: UART controller node that consumes the clock generated by the clock diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c index cf6fb41..113ce7d 100644 --- a/drivers/clk/samsung/clk-exynos5410.c +++ b/drivers/clk/samsung/clk-exynos5410.c @@ -14,6 +14,7 @@ #include <linux/clk-provider.h> #include <linux/of.h> #include <linux/of_address.h> +#include <linux/clk.h> #include "clk.h" @@ -21,6 +22,8 @@ #define APLL_CON0 0x100 #define CPLL_LOCK 0x10020 #define CPLL_CON0 0x10120 +#define EPLL_LOCK 0x10040 +#define EPLL_CON0 0x10130 #define MPLL_LOCK 0x4000 #define MPLL_CON0 0x4100 #define BPLL_LOCK 0x20010 @@ -58,7 +61,7 @@ /* list of PLLs */ enum exynos5410_plls { - apll, cpll, mpll, + apll, cpll, epll, mpll, bpll, kpll, nr_plls /* number of PLLs */ }; @@ -67,6 +70,7 @@ enum exynos5410_plls { PNAME(apll_p) = { "fin_pll", "fout_apll", }; PNAME(bpll_p) = { "fin_pll", "fout_bpll", }; PNAME(cpll_p) = { "fin_pll", "fout_cpll" }; +PNAME(epll_p) = { "fin_pll", "fout_epll" }; PNAME(mpll_p) = { "fin_pll", "fout_mpll", }; PNAME(kpll_p) = { "fin_pll", "fout_kpll", }; @@ -95,6 +99,8 @@ static const struct samsung_mux_clock exynos5410_mux_clks[] __initconst = { MUX(0, "sclk_bpll", bpll_p, SRC_CDREX, 0, 1), MUX(0, "sclk_bpll_muxed", bpll_user_p, SRC_TOP2, 24, 1), + MUX(0, "sclk_epll", epll_p, SRC_TOP2, 12, 1), + MUX(0, "sclk_cpll", cpll_p, SRC_TOP2, 8, 1), MUX(0, "sclk_mpll_bpll", mpll_bpll_p, SRC_TOP1, 20, 1), @@ -219,11 +225,26 @@ static const struct samsung_gate_clock exynos5410_gate_clks[] __initconst = { GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0), }; -static const struct samsung_pll_clock exynos5410_plls[nr_plls] __initconst = { +static const struct samsung_pll_rate_table exynos5410_pll2550x_24mhz_tbl[] __initconst = { + PLL_36XX_RATE(400000000U, 200, 2, 2, 0), + PLL_36XX_RATE(333000000U, 111, 2, 2, 0), + PLL_36XX_RATE(300000000U, 100, 2, 2, 0), + PLL_36XX_RATE(266000000U, 266, 3, 3, 0), + PLL_36XX_RATE(200000000U, 200, 3, 3, 0), + PLL_36XX_RATE(192000000U, 192, 3, 3, 0), + PLL_36XX_RATE(166000000U, 166, 3, 3, 0), + PLL_36XX_RATE(133000000U, 266, 3, 4, 0), + PLL_36XX_RATE(100000000U, 200, 3, 4, 0), + PLL_36XX_RATE(66000000U, 176, 2, 5, 0), +}; + +static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = { [apll] = PLL(pll_35xx, CLK_FOUT_APLL, "fout_apll", "fin_pll", APLL_LOCK, APLL_CON0, NULL), [cpll] = PLL(pll_35xx, CLK_FOUT_CPLL, "fout_cpll", "fin_pll", CPLL_LOCK, CPLL_CON0, NULL), + [epll] = PLL(pll_2650x, CLK_FOUT_EPLL, "fout_epll", "fin_pll", EPLL_LOCK, + EPLL_CON0, NULL), [mpll] = PLL(pll_35xx, CLK_FOUT_MPLL, "fout_mpll", "fin_pll", MPLL_LOCK, MPLL_CON0, NULL), [bpll] = PLL(pll_35xx, CLK_FOUT_BPLL, "fout_bpll", "fin_pll", BPLL_LOCK, @@ -237,6 +258,7 @@ static void __init exynos5410_clk_init(struct device_node *np) { struct samsung_clk_provider *ctx; void __iomem *reg_base; + struct clk *xxti; reg_base = of_iomap(np, 0); if (!reg_base) @@ -244,6 +266,10 @@ static void __init exynos5410_clk_init(struct device_node *np) ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS); + xxti = of_clk_get(np, 0); + if (!IS_ERR(xxti) && clk_get_rate(xxti) == 24 * MHZ) + exynos5410_plls[epll].rate_table = exynos5410_pll2550x_24mhz_tbl; + samsung_clk_register_pll(ctx, exynos5410_plls, ARRAY_SIZE(exynos5410_plls), reg_base); diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c index b5ab055..d61fd80 100644 --- a/drivers/clk/samsung/clk-pll.c +++ b/drivers/clk/samsung/clk-pll.c @@ -1018,6 +1018,102 @@ static const struct clk_ops samsung_pll2550xx_clk_min_ops = { }; /* + * PLL2650x Clock Type + */ + +/* Maximum lock time can be 3000 * PDIV cycles */ +#define PLL2650X_LOCK_FACTOR 3000 + +#define PLL2650X_M_MASK 0x3FF +#define PLL2650X_P_MASK 0x3F +#define PLL2650X_S_MASK 0x7 +#define PLL2650X_K_MASK 0xFFFF +#define PLL2650X_LOCK_STAT_MASK 0x1 +#define PLL2650X_M_SHIFT 16 +#define PLL2650X_P_SHIFT 8 +#define PLL2650X_S_SHIFT 0 +#define PLL2650X_K_SHIFT 0 +#define PLL2650X_LOCK_STAT_SHIFT 29 +#define PLL2650X_PLL_ENABLE_SHIFT 31 + +static unsigned long samsung_pll2650x_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct samsung_clk_pll *pll = to_clk_pll(hw); + u64 fout = parent_rate; + u32 mdiv, pdiv, sdiv, pll_con0, pll_con1; + s16 kdiv; + + pll_con0 = readl_relaxed(pll->con_reg); + mdiv = (pll_con0 >> PLL2650X_M_SHIFT) & PLL2650X_M_MASK; + pdiv = (pll_con0 >> PLL2650X_P_SHIFT) & PLL2650X_P_MASK; + sdiv = (pll_con0 >> PLL2650X_S_SHIFT) & PLL2650X_S_MASK; + + pll_con1 = readl_relaxed(pll->con_reg + 4); + kdiv = (s16)((pll_con1 >> PLL2650X_K_SHIFT) & PLL2650X_K_MASK); + + fout *= (mdiv << 16) + kdiv; + do_div(fout, (pdiv << sdiv)); + fout >>= 16; + + return (unsigned long)fout; +} + +static int samsung_pll2650x_set_rate(struct clk_hw *hw, unsigned long drate, + unsigned long prate) +{ + struct samsung_clk_pll *pll = to_clk_pll(hw); + const struct samsung_pll_rate_table *rate; + u32 con0, con1; + + /* Get required rate settings from table */ + rate = samsung_get_pll_settings(pll, drate); + if (!rate) { + pr_err("%s: Invalid rate : %lu for pll clk %s\n", __func__, + drate, clk_hw_get_name(hw)); + return -EINVAL; + } + + con0 = readl_relaxed(pll->con_reg); + con1 = readl_relaxed(pll->con_reg + 4); + + /* Set PLL lock time. */ + writel_relaxed(rate->pdiv * PLL2650X_LOCK_FACTOR, pll->lock_reg); + + /* Change PLL PMS values */ + con0 &= ~((PLL2650X_M_MASK << PLL2650X_M_SHIFT) | + (PLL2650X_P_MASK << PLL2650X_P_SHIFT) | + (PLL2650X_S_MASK << PLL2650X_S_SHIFT)); + con0 |= (rate->mdiv << PLL2650X_M_SHIFT) | + (rate->pdiv << PLL2650X_P_SHIFT) | + (rate->sdiv << PLL2650X_S_SHIFT); + con0 |= (1 << PLL2650X_PLL_ENABLE_SHIFT); + writel_relaxed(con0, pll->con_reg); + + con1 &= ~(PLL2650X_K_MASK << PLL2650X_K_SHIFT); + con1 |= ((rate->kdiv & PLL2650X_K_MASK) << PLL2650X_K_SHIFT); + writel_relaxed(con1, pll->con_reg + 4); + + do { + cpu_relax(); + con0 = readl_relaxed(pll->con_reg); + } while (!(con0 & (PLL2650X_LOCK_STAT_MASK + << PLL2650X_LOCK_STAT_SHIFT))); + + return 0; +} + +static const struct clk_ops samsung_pll2650x_clk_ops = { + .recalc_rate = samsung_pll2650x_recalc_rate, + .round_rate = samsung_pll_round_rate, + .set_rate = samsung_pll2650x_set_rate, +}; + +static const struct clk_ops samsung_pll2650x_clk_min_ops = { + .recalc_rate = samsung_pll2650x_recalc_rate, +}; + +/* * PLL2650XX Clock Type */ @@ -1227,6 +1323,12 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx, else init.ops = &samsung_pll2550xx_clk_ops; break; + case pll_2650x: + if (!pll->rate_table) + init.ops = &samsung_pll2650x_clk_min_ops; + else + init.ops = &samsung_pll2650x_clk_ops; + break; case pll_2650xx: if (!pll->rate_table) init.ops = &samsung_pll2650xx_clk_min_ops; diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h index df4ad8a..a1ca023 100644 --- a/drivers/clk/samsung/clk-pll.h +++ b/drivers/clk/samsung/clk-pll.h @@ -33,6 +33,7 @@ enum samsung_pll_type { pll_s3c2440_mpll, pll_2550x, pll_2550xx, + pll_2650x, pll_2650xx, pll_1450x, pll_1451x,
This patch adds code instantiating the EPLL, which is used as the audio subsystem's root clock. The requirement to specify the external root clock in clocks property is also added. That ensures proper initialization order by explicitly specifying dependencies in devicetree. It prevents situations when the SoC's clock controller driver has initialized, the external oscillator clock is not yet registered and setting clock frequencies through assigned-clock-rates property doesn't work properly due to unknown external oscillator frequency. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- .../devicetree/bindings/clock/exynos5410-clock.txt | 14 +++ drivers/clk/samsung/clk-exynos5410.c | 30 +++++- drivers/clk/samsung/clk-pll.c | 102 +++++++++++++++++++++ drivers/clk/samsung/clk-pll.h | 1 + 4 files changed, 145 insertions(+), 2 deletions(-)