| Message ID | 1515377863-20358-1-git-send-email-david@lechnology.com |
|---|---|
| Headers | show |
| Series | ARM: davinci: convert to common clock framework | expand |
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds a new driver for mach-davinci PLL clocks. This is porting the > code from arch/arm/mach-davinci/clock.c to the common clock framework. > Additionally, it adds device tree support for these clocks. > > The ifeq ($(CONFIG_COMMON_CLK), y) in the Makefile is needed to prevent > compile errors until the clock code in arch/arm/mach-davinci is removed. > > Note: although there are similar clocks for TI Keystone we are not able > to share the code for a few reasons. The keystone clocks are device tree > only and use legacy one-node-per-clock bindings. Also the register > layouts are a bit different, which would add even more if/else mess > to the keystone clocks. And the keystone PLL driver doesn't support > setting clock rates. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > MAINTAINERS | 6 + > drivers/clk/Makefile | 1 + > drivers/clk/davinci/Makefile | 5 + > drivers/clk/davinci/pll.c | 564 +++++++++++++++++++++++++++++++++++++++++++ > drivers/clk/davinci/pll.h | 61 +++++ > 5 files changed, 637 insertions(+) > create mode 100644 drivers/clk/davinci/Makefile > create mode 100644 drivers/clk/davinci/pll.c > create mode 100644 drivers/clk/davinci/pll.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index a6e86e2..1db0cf0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13554,6 +13554,12 @@ F: arch/arm/mach-davinci/ > F: drivers/i2c/busses/i2c-davinci.c > F: arch/arm/boot/dts/da850* > > +TI DAVINCI SERIES CLOCK DRIVER > +M: David Lechner <david@lechnology.com> Please also add: R: Sekhar Nori <nsekhar@ti.com> > +S: Maintained > +F: Documentation/devicetree/bindings/clock/ti/davinci/ > +F: drivers/clk/davinci/ > + > TI DAVINCI SERIES GPIO DRIVER > M: Keerthy <j-keerthy@ti.com> > L: linux-gpio@vger.kernel.org > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index f7f761b..c865fd0 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -60,6 +60,7 @@ obj-$(CONFIG_ARCH_ARTPEC) += axis/ > obj-$(CONFIG_ARC_PLAT_AXS10X) += axs10x/ > obj-y += bcm/ > obj-$(CONFIG_ARCH_BERLIN) += berlin/ > +obj-$(CONFIG_ARCH_DAVINCI) += davinci/ > obj-$(CONFIG_H8300) += h8300/ > obj-$(CONFIG_ARCH_HISI) += hisilicon/ > obj-y += imgtec/ > diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile > new file mode 100644 > index 0000000..d9673bd > --- /dev/null > +++ b/drivers/clk/davinci/Makefile > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +ifeq ($(CONFIG_COMMON_CLK), y) > +obj-y += pll.o > +endif > diff --git a/drivers/clk/davinci/pll.c b/drivers/clk/davinci/pll.c > new file mode 100644 > index 0000000..46f9c18 > --- /dev/null > +++ b/drivers/clk/davinci/pll.c > @@ -0,0 +1,564 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PLL clock driver for TI Davinci SoCs > + * > + * Copyright (C) 2017 David Lechner <david@lechnology.com> > + * > + * Based on drivers/clk/keystone/pll.c > + * Copyright (C) 2013 Texas Instruments Inc. > + * Murali Karicheri <m-karicheri2@ti.com> > + * Santosh Shilimkar <santosh.shilimkar@ti.com> > + * > + * And on arch/arm/mach-davinci/clock.c > + * Copyright (C) 2006-2007 Texas Instruments. > + * Copyright (C) 2008-2009 Deep Root Systems, LLC > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/of_address.h> > +#include <linux/of.h> > +#include <linux/slab.h> > + > +#include "pll.h" > + > +#define REVID 0x000 > +#define PLLCTL 0x100 > +#define OCSEL 0x104 > +#define PLLSECCTL 0x108 > +#define PLLM 0x110 > +#define PREDIV 0x114 > +#define PLLDIV1 0x118 > +#define PLLDIV2 0x11c > +#define PLLDIV3 0x120 > +#define OSCDIV 0x124 > +#define POSTDIV 0x128 > +#define BPDIV 0x12c > +#define PLLCMD 0x138 > +#define PLLSTAT 0x13c > +#define ALNCTL 0x140 > +#define DCHANGE 0x144 > +#define CKEN 0x148 > +#define CKSTAT 0x14c > +#define SYSTAT 0x150 > +#define PLLDIV4 0x160 > +#define PLLDIV5 0x164 > +#define PLLDIV6 0x168 > +#define PLLDIV7 0x16c > +#define PLLDIV8 0x170 > +#define PLLDIV9 0x174 > + > +#define PLLCTL_PLLEN BIT(0) > +#define PLLCTL_PLLPWRDN BIT(1) > +#define PLLCTL_PLLRST BIT(3) > +#define PLLCTL_PLLDIS BIT(4) > +#define PLLCTL_PLLENSRC BIT(5) > +#define PLLCTL_CLKMODE BIT(8) > + > +#define PLLM_MASK 0x1f > +#define PREDIV_RATIO_MASK 0x1f May be use the mode modern GENMASK()? > +#define PREDIV_PREDEN BIT(15) > +#define PLLDIV_RATIO_WIDTH 5 > +#define PLLDIV_ENABLE_SHIFT 15 > +#define OSCDIV_RATIO_WIDTH 5 > +#define POSTDIV_RATIO_MASK 0x1f > +#define POSTDIV_POSTDEN BIT(15) > +#define BPDIV_RATIO_SHIFT 0 > +#define BPDIV_RATIO_WIDTH 5 > +#define CKEN_OBSCLK_SHIFT 1 > +#define CKEN_AUXEN_SHIFT 0 > + > +/* > + * OMAP-L138 system reference guide recommends a wait for 4 OSCIN/CLKIN > + * cycles to ensure that the PLLC has switched to bypass mode. Delay of 1us > + * ensures we are good for all > 4MHz OSCIN/CLKIN inputs. Typically the input > + * is ~25MHz. Units are micro seconds. > + */ > +#define PLL_BYPASS_TIME 1 > +/* From OMAP-L138 datasheet table 6-4. Units are micro seconds */ An empty line before the comment make it easier to read. > +#define PLL_RESET_TIME 1 > +/* > + * From OMAP-L138 datasheet table 6-4; assuming prediv = 1, sqrt(pllm) = 4 > + * Units are micro seconds. > + */ > +#define PLL_LOCK_TIME 20 > + > +/** > + * struct davinci_pll_clk - Main PLL clock > + * @hw: clk_hw for the pll > + * @base: Base memory address > + * @parent_rate: Saved parent rate used by some child clocks You don't have parent_rate in the structure below. > + */ > +struct davinci_pll_clk { > + struct clk_hw hw; > + void __iomem *base; > +}; > + > +#define to_davinci_pll_clk(_hw) container_of((_hw), struct davinci_pll_clk, hw) > + > +static unsigned long davinci_pll_clk_recalc(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct davinci_pll_clk *pll = to_davinci_pll_clk(hw); > + unsigned long rate = parent_rate; > + u32 prediv, mult, postdiv; > + > + prediv = readl(pll->base + PREDIV) & PREDIV_RATIO_MASK; > + mult = readl(pll->base + PLLM) & PLLM_MASK; > + postdiv = readl(pll->base + POSTDIV) & POSTDIV_RATIO_MASK; Shouldn't we check if the pre and post dividers are enabled before using them? > + > + rate /= prediv + 1; > + rate *= mult + 1; > + rate /= postdiv + 1; > + > + return rate; > +} > + > +/** > + * davinci_pll_get_best_rate - Calculate PLL output closest to a given rate > + * @rate: The target rate > + * @parent_rate: The PLL input clock rate > + * @mult: Pointer to hold the multiplier value (optional) > + * @postdiv: Pointer to hold the postdiv value (optional) > + * > + * Returns: The closest rate less than or equal to @rate that the PLL can > + * generate. @mult and @postdiv will contain the values required to generate > + * that rate. > + */ > +static long davinci_pll_get_best_rate(u32 rate, u32 parent_rate, u32 *mult, > + u32 *postdiv) > +{ > + u32 r, m, d; > + u32 best_rate = 0; > + u32 best_mult = 0; > + u32 best_postdiv = 0; > + > + for (d = 1; d <= 4; d++) {> + for (m = min(32U, rate * d / parent_rate); m > 0; m--) { > + r = parent_rate * m / d; > + > + if (r < best_rate) > + break; > + > + if (r > best_rate && r <= rate) { > + best_rate = r; > + best_mult = m; > + best_postdiv = d; > + } > + > + if (best_rate == rate) > + goto out; > + } > + } > + > +out: > + if (mult) > + *mult = best_mult; > + if (postdiv) > + *postdiv = best_postdiv; > + > + return best_rate; > +} PLL output on DA850 must never be below 300MHz or above 600MHz (see datasheet table "Allowed PLL Operating Conditions"). Does this take care of that? Thats one of the main reasons I recall I went with some specific values of prediv, pllm and post div in arch/arm/mach-davinci/da850.c > + > +static long davinci_pll_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + return davinci_pll_get_best_rate(rate, *parent_rate, NULL, NULL); > +} > + > +/** > + * __davinci_pll_set_rate - set the output rate of a given PLL. > + * > + * Note: Currently tested to work with OMAP-L138 only. > + * > + * @pll: pll whose rate needs to be changed. > + * @prediv: The pre divider value. Passing 0 disables the pre-divider. > + * @pllm: The multiplier value. Passing 0 leads to multiply-by-one. > + * @postdiv: The post divider value. Passing 0 disables the post-divider. > + */ > +static void __davinci_pll_set_rate(struct davinci_pll_clk *pll, u32 prediv, > + u32 mult, u32 postdiv) > +{ > + u32 ctrl, locktime; > + > + /* > + * PLL lock time required per OMAP-L138 datasheet is > + * (2000 * prediv)/sqrt(pllm) OSCIN cycles. We approximate sqrt(pllm) > + * as 4 and OSCIN cycle as 25 MHz. > + */ > + if (prediv) { > + locktime = ((2000 * prediv) / 100); > + prediv = (prediv - 1) | PREDIV_PREDEN; > + } else { > + locktime = PLL_LOCK_TIME; > + } Empty line here will be nice. > + if (postdiv) > + postdiv = (postdiv - 1) | POSTDIV_POSTDEN; > + if (mult) > + mult = mult - 1; > + > + ctrl = readl(pll->base + PLLCTL); > + > + /* Switch the PLL to bypass mode */ > + ctrl &= ~(PLLCTL_PLLENSRC | PLLCTL_PLLEN); > + writel(ctrl, pll->base + PLLCTL); > + > + udelay(PLL_BYPASS_TIME); > + > + /* Reset and enable PLL */ > + ctrl &= ~(PLLCTL_PLLRST | PLLCTL_PLLDIS); > + writel(ctrl, pll->base + PLLCTL); > + > + writel(prediv, pll->base + PREDIV); > + writel(mult, pll->base + PLLM); > + writel(postdiv, pll->base + POSTDIV); > + > + udelay(PLL_RESET_TIME); > + > + /* Bring PLL out of reset */ > + ctrl |= PLLCTL_PLLRST; > + writel(ctrl, pll->base + PLLCTL); > + > + udelay(locktime); > + > + /* Remove PLL from bypass mode */ > + ctrl |= PLLCTL_PLLEN; > + writel(ctrl, pll->base + PLLCTL); > +} [...] > +/** > + * davinci_pll_obs_clk_register - Register oscillator divider clock (OBSCLK) > + * @name: The clock name > + * @parent_names: The parent clock names > + * @num_parents: The number of paren clocks > + * @base: The PLL memory region > + * @table: A table of values cooresponding to the parent clocks (see OCSEL > + * register in SRM for values) > + */ > +struct clk *davinci_pll_obs_clk_register(const char *name, > + const char * const *parent_names, > + u8 num_parents, > + void __iomem *base, > + u32 *table) > +{ > + struct clk_mux *mux; > + struct clk_gate *gate; > + struct clk_divider *divider; > + struct clk *clk; > + > + mux = kzalloc(sizeof(*mux), GFP_KERNEL); > + if (!mux) > + return ERR_PTR(-ENOMEM); > + > + mux->reg = base + OCSEL; > + mux->table = table; > + > + gate = kzalloc(sizeof(*gate), GFP_KERNEL); > + if (!gate) { > + kfree(mux); > + return ERR_PTR(-ENOMEM); > + } > + > + gate->reg = base + CKEN; > + gate->bit_idx = CKEN_OBSCLK_SHIFT; > + > + divider = kzalloc(sizeof(*divider), GFP_KERNEL); > + if (!divider) { > + kfree(gate); > + kfree(mux); > + return ERR_PTR(-ENOMEM); > + } > + > + divider->reg = base + OSCDIV; > + divider->width = OSCDIV_RATIO_WIDTH; can you write OD1EN of OSCDIV here? I guess the reset default is 1 so you didnt need to do that. But not doing exposes us to settings that bootloader left us in. > + > + clk = clk_register_composite(NULL, name, parent_names, num_parents, > + &mux->hw, &clk_mux_ops, > + ÷r->hw, &clk_divider_ops, > + &gate->hw, &clk_gate_ops, 0); > + if (IS_ERR(clk)) { > + kfree(divider); > + kfree(gate); > + kfree(mux); > + } > + > + return clk; > +} > + > +struct clk * > +davinci_pll_divclk_register(const struct davinci_pll_divclk_info *info, > + void __iomem *base) > +{ > + const struct clk_ops *divider_ops = &clk_divider_ops; setting the sysclk divider requires GOSTAT handling apart from setting the divider value. So I think .set_rate ops above wont work. Other ops can be used, I guess. So we need a private structure here. Can you port over davinci_set_sysclk_rate() too? I understand you cannot test it due to lack of cpufreq support in DT, but I can help testing there. Or leave .set_rate NULL and it can be added later. > + struct clk_gate *gate; > + struct clk_divider *divider; > + struct clk *clk; > + u32 reg; > + u32 flags = 0; > + > + /* PLLDIVn registers are not entirely consecutive */ > + if (info->id < 4) > + reg = PLLDIV1 + 4 * (info->id - 1); > + else > + reg = PLLDIV4 + 4 * (info->id - 4); > + > + gate = kzalloc(sizeof(*gate), GFP_KERNEL); > + if (!gate) > + return ERR_PTR(-ENOMEM); > + > + gate->reg = base + reg; > + gate->bit_idx = PLLDIV_ENABLE_SHIFT; > + > + divider = kzalloc(sizeof(*divider), GFP_KERNEL); > + if (!divider) { > + kfree(gate); > + return ERR_PTR(-ENOMEM); > + } > + > + divider->reg = base + reg; > + divider->width = PLLDIV_RATIO_WIDTH; > + divider->flags = 0; > + > + if (info->flags & DIVCLK_FIXED_DIV) { > + flags |= CLK_DIVIDER_READ_ONLY; > + divider_ops = &clk_divider_ro_ops; > + } > + > + /* Only the ARM clock can change the parent PLL rate */ > + if (info->flags & DIVCLK_ARM_RATE) > + flags |= CLK_SET_RATE_PARENT; > + > + if (info->flags & DIVCLK_ALWAYS_ENABLED) > + flags |= CLK_IS_CRITICAL; > + > + clk = clk_register_composite(NULL, info->name, &info->parent_name, 1, > + NULL, NULL, ÷r->hw, divider_ops, > + &gate->hw, &clk_gate_ops, flags); > + if (IS_ERR(clk)) { > + kfree(divider); > + kfree(gate); > + } > + > + return clk; > +} > + > +#ifdef CONFIG_OF > +#define MAX_NAME_SIZE 20 > + > +void of_davinci_pll_init(struct device_node *node, const char *name, > + const struct davinci_pll_divclk_info *info, > + u8 max_divclk_id) > +{ > + struct device_node *child; > + const char *parent_name; > + void __iomem *base; > + struct clk *clk; > + > + base = of_iomap(node, 0); > + if (!base) { > + pr_err("%s: ioremap failed\n", __func__); > + return; > + } > + > + parent_name = of_clk_get_parent_name(node, 0); > + > + clk = davinci_pll_clk_register(name, parent_name, base); > + if (IS_ERR(clk)) { > + pr_err("%s: failed to register %s (%ld)\n", __func__, name, > + PTR_ERR(clk)); You can probably avoid these line breaks by adding on top of the file #define pr_fmt(fmt) "%s: " fmt "\n", __func__ > + return; > + } > + > + child = of_get_child_by_name(node, "sysclk"); > + if (child && of_device_is_available(child)) { > + struct clk_onecell_data *clk_data; > + > + clk_data = clk_alloc_onecell_data(max_divclk_id + 1); > + if (!clk_data) { > + pr_err("%s: out of memory\n", __func__); > + return; > + } > + > + for (; info->name; info++) { > + clk = davinci_pll_divclk_register(info, base); > + if (IS_ERR(clk)) > + pr_warn("%s: failed to register %s (%ld)\n", > + __func__, info->name, PTR_ERR(clk)); > + else > + clk_data->clks[info->id] = clk; > + } > + of_clk_add_provider(child, of_clk_src_onecell_get, clk_data); > + } > + of_node_put(child); > + > + child = of_get_child_by_name(node, "auxclk"); > + if (child && of_device_is_available(child)) { > + char child_name[MAX_NAME_SIZE]; > + > + snprintf(child_name, MAX_NAME_SIZE, "%s_aux_clk", name); > + > + clk = davinci_pll_aux_clk_register(child_name, parent_name, base); > + if (IS_ERR(clk)) > + pr_warn("%s: failed to register %s (%ld)\n", __func__, > + child_name, PTR_ERR(clk)); > + else > + of_clk_add_provider(child, of_clk_src_simple_get, clk); > + } davinci_pll_obs_clk_register() should also be handled here? Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds platform-specific declarations for the PLL clocks on TI DA830/ > OMAP-L137/AM17XX SoCs. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/12/2018 03:21 AM, Sekhar Nori wrote: > On Monday 08 January 2018 07:47 AM, David Lechner wrote: >> This adds a new driver for mach-davinci PLL clocks. This is porting the >> code from arch/arm/mach-davinci/clock.c to the common clock framework. >> Additionally, it adds device tree support for these clocks. >> >> The ifeq ($(CONFIG_COMMON_CLK), y) in the Makefile is needed to prevent >> compile errors until the clock code in arch/arm/mach-davinci is removed. >> >> Note: although there are similar clocks for TI Keystone we are not able >> to share the code for a few reasons. The keystone clocks are device tree >> only and use legacy one-node-per-clock bindings. Also the register >> layouts are a bit different, which would add even more if/else mess >> to the keystone clocks. And the keystone PLL driver doesn't support >> setting clock rates. >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> + >> +#define PLLM_MASK 0x1f >> +#define PREDIV_RATIO_MASK 0x1f > > May be use the mode modern GENMASK()? I haven't seen that one before. Thanks. ... >> +static unsigned long davinci_pll_clk_recalc(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + struct davinci_pll_clk *pll = to_davinci_pll_clk(hw); >> + unsigned long rate = parent_rate; >> + u32 prediv, mult, postdiv; >> + >> + prediv = readl(pll->base + PREDIV) & PREDIV_RATIO_MASK; >> + mult = readl(pll->base + PLLM) & PLLM_MASK; >> + postdiv = readl(pll->base + POSTDIV) & POSTDIV_RATIO_MASK; > > Shouldn't we check if the pre and post dividers are enabled before using > them? Indeed. > >> + >> + rate /= prediv + 1; >> + rate *= mult + 1; >> + rate /= postdiv + 1; >> + >> + return rate; >> +} >> + ... > > PLL output on DA850 must never be below 300MHz or above 600MHz (see > datasheet table "Allowed PLL Operating Conditions"). Does this take care > of that? Thats one of the main reasons I recall I went with some > specific values of prediv, pllm and post div in > arch/arm/mach-davinci/da850.c Apparently, I missed this requirement. It looks like I am going to have to rework things so that there is some coordination between the PLL and the PLLDIV clocks in order to get the < 300MHz operating points. ... >> + >> + divider->reg = base + OSCDIV; >> + divider->width = OSCDIV_RATIO_WIDTH; > > can you write OD1EN of OSCDIV here? I guess the reset default is 1 so > you didnt need to do that. But not doing exposes us to settings that > bootloader left us in. > It looks like I am going to have to make a custom implementation for parts of this clock anyway, so I will probably just make new enable/disable callbacks that set both OSCDIV_OD1EN and CKEN_OBSCLK. >> + >> + clk = clk_register_composite(NULL, name, parent_names, num_parents, >> + &mux->hw, &clk_mux_ops, >> + ÷r->hw, &clk_divider_ops, >> + &gate->hw, &clk_gate_ops, 0); >> + if (IS_ERR(clk)) { >> + kfree(divider); >> + kfree(gate); >> + kfree(mux); >> + } >> + >> + return clk; >> +} >> + >> +struct clk * >> +davinci_pll_divclk_register(const struct davinci_pll_divclk_info *info, >> + void __iomem *base) >> +{ >> + const struct clk_ops *divider_ops = &clk_divider_ops; > > setting the sysclk divider requires GOSTAT handling apart from setting > the divider value. So I think .set_rate ops above wont work. Other ops > can be used, I guess. So we need a private structure here. > > Can you port over davinci_set_sysclk_rate() too? I understand you cannot > test it due to lack of cpufreq support in DT, but I can help testing there. > > Or leave .set_rate NULL and it can be added later. Yes, I noticed that I missed this after I submitted this series. And I will have to rework things to coordinate with the PLL as mentioned above. FYI, I do have cpufreq-dt working, although the LEGO EV3 has a fixed 1.2V regulator, so I am limited in what I can test. Basically, I can only switch between 300MHz and 375MHz according to the datasheets. The chip is technically the 456MHz version. What would happen if I ran it faster or slower with the wrong voltage? ... >> + >> + child = of_get_child_by_name(node, "auxclk"); >> + if (child && of_device_is_available(child)) { >> + char child_name[MAX_NAME_SIZE]; >> + >> + snprintf(child_name, MAX_NAME_SIZE, "%s_aux_clk", name); >> + >> + clk = davinci_pll_aux_clk_register(child_name, parent_name, base); >> + if (IS_ERR(clk)) >> + pr_warn("%s: failed to register %s (%ld)\n", __func__, >> + child_name, PTR_ERR(clk)); >> + else >> + of_clk_add_provider(child, of_clk_src_simple_get, clk); >> + } > > davinci_pll_obs_clk_register() should also be handled here? I omitted it because no one is using it (same reason I left it out of the device tree bindings). We can certainly add it, though. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 12, 2018 at 9:25 AM, David Lechner <david@lechnology.com> wrote: > On 01/12/2018 03:21 AM, Sekhar Nori wrote: >> >> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>> >>> This adds a new driver for mach-davinci PLL clocks. This is porting the >>> code from arch/arm/mach-davinci/clock.c to the common clock framework. >>> Additionally, it adds device tree support for these clocks. >>> >>> The ifeq ($(CONFIG_COMMON_CLK), y) in the Makefile is needed to prevent >>> compile errors until the clock code in arch/arm/mach-davinci is removed. >>> >>> Note: although there are similar clocks for TI Keystone we are not able >>> to share the code for a few reasons. The keystone clocks are device tree >>> only and use legacy one-node-per-clock bindings. Also the register >>> layouts are a bit different, which would add even more if/else mess >>> to the keystone clocks. And the keystone PLL driver doesn't support >>> setting clock rates. >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >>> --- > > >>> + >>> +#define PLLM_MASK 0x1f >>> +#define PREDIV_RATIO_MASK 0x1f >> >> >> May be use the mode modern GENMASK()? > > > I haven't seen that one before. Thanks. > > ... > >>> +static unsigned long davinci_pll_clk_recalc(struct clk_hw *hw, >>> + unsigned long parent_rate) >>> +{ >>> + struct davinci_pll_clk *pll = to_davinci_pll_clk(hw); >>> + unsigned long rate = parent_rate; >>> + u32 prediv, mult, postdiv; >>> + >>> + prediv = readl(pll->base + PREDIV) & PREDIV_RATIO_MASK; >>> + mult = readl(pll->base + PLLM) & PLLM_MASK; >>> + postdiv = readl(pll->base + POSTDIV) & POSTDIV_RATIO_MASK; >> >> >> Shouldn't we check if the pre and post dividers are enabled before using >> them? > > > Indeed. > >> >>> + >>> + rate /= prediv + 1; >>> + rate *= mult + 1; >>> + rate /= postdiv + 1; >>> + >>> + return rate; >>> +} >>> + > > > ... > >> >> PLL output on DA850 must never be below 300MHz or above 600MHz (see >> datasheet table "Allowed PLL Operating Conditions"). Does this take care >> of that? Thats one of the main reasons I recall I went with some >> specific values of prediv, pllm and post div in >> arch/arm/mach-davinci/da850.c > > > Apparently, I missed this requirement. It looks like I am going to have to > rework things so that there is some coordination between the PLL and the > PLLDIV clocks in order to get the < 300MHz operating points. > > ... > >>> + >>> + divider->reg = base + OSCDIV; >>> + divider->width = OSCDIV_RATIO_WIDTH; >> >> >> can you write OD1EN of OSCDIV here? I guess the reset default is 1 so >> you didnt need to do that. But not doing exposes us to settings that >> bootloader left us in. >> > > It looks like I am going to have to make a custom implementation for parts > of this clock anyway, so I will probably just make new enable/disable > callbacks that set both OSCDIV_OD1EN and CKEN_OBSCLK. > > >>> + >>> + clk = clk_register_composite(NULL, name, parent_names, >>> num_parents, >>> + &mux->hw, &clk_mux_ops, >>> + ÷r->hw, &clk_divider_ops, >>> + &gate->hw, &clk_gate_ops, 0); >>> + if (IS_ERR(clk)) { >>> + kfree(divider); >>> + kfree(gate); >>> + kfree(mux); >>> + } >>> + >>> + return clk; >>> +} >>> + >>> +struct clk * >>> +davinci_pll_divclk_register(const struct davinci_pll_divclk_info *info, >>> + void __iomem *base) >>> +{ >>> + const struct clk_ops *divider_ops = &clk_divider_ops; >> >> >> setting the sysclk divider requires GOSTAT handling apart from setting >> the divider value. So I think .set_rate ops above wont work. Other ops >> can be used, I guess. So we need a private structure here. >> >> Can you port over davinci_set_sysclk_rate() too? I understand you cannot >> test it due to lack of cpufreq support in DT, but I can help testing >> there. >> >> Or leave .set_rate NULL and it can be added later. > > > Yes, I noticed that I missed this after I submitted this series. And I > will have to rework things to coordinate with the PLL as mentioned above. > > FYI, I do have cpufreq-dt working, although the LEGO EV3 has a fixed 1.2V > regulator, so I am limited in what I can test. Basically, I can only switch I can test the cpufreq-dt. Are there additional patches or are they part of the same git repo I have been testing? The DA850 I have may not run all the way to highest speed, but I'll see what I can find. I think I had a few at one point, but I don't think it was part of Logic PD's standard product lineup. adam > between 300MHz and 375MHz according to the datasheets. The chip is > technically > the 456MHz version. What would happen if I ran it faster or slower with the > wrong voltage? > > ... > >>> + >>> + child = of_get_child_by_name(node, "auxclk"); >>> + if (child && of_device_is_available(child)) { >>> + char child_name[MAX_NAME_SIZE]; >>> + >>> + snprintf(child_name, MAX_NAME_SIZE, "%s_aux_clk", name); >>> + >>> + clk = davinci_pll_aux_clk_register(child_name, >>> parent_name, base); >>> + if (IS_ERR(clk)) >>> + pr_warn("%s: failed to register %s (%ld)\n", >>> __func__, >>> + child_name, PTR_ERR(clk)); >>> + else >>> + of_clk_add_provider(child, of_clk_src_simple_get, >>> clk); >>> + } >> >> >> davinci_pll_obs_clk_register() should also be handled here? > > > I omitted it because no one is using it (same reason I left it out of the > device tree bindings). We can certainly add it, though. > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/12/2018 09:30 AM, Adam Ford wrote: > On Fri, Jan 12, 2018 at 9:25 AM, David Lechner <david@lechnology.com> wrote: >> >> Yes, I noticed that I missed this after I submitted this series. And I >> will have to rework things to coordinate with the PLL as mentioned above. >> >> FYI, I do have cpufreq-dt working, although the LEGO EV3 has a fixed 1.2V >> regulator, so I am limited in what I can test. Basically, I can only switch > > I can test the cpufreq-dt. Are there additional patches or are they > part of the same git repo I have been testing? > > The DA850 I have may not run all the way to highest speed, but I'll > see what I can find. I think I had a few at one point, but I don't > think it was part of Logic PD's standard product lineup. > There are some extra patches I have not submitted yet. However, based on the other comments here, I don't have things implemented correctly, so you probably want to wait to test until I get that straightened out anyway. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 12 January 2018 08:55 PM, David Lechner wrote: >> >> PLL output on DA850 must never be below 300MHz or above 600MHz (see >> datasheet table "Allowed PLL Operating Conditions"). Does this take care >> of that? Thats one of the main reasons I recall I went with some >> specific values of prediv, pllm and post div in >> arch/arm/mach-davinci/da850.c > > Apparently, I missed this requirement. It looks like I am going to have to > rework things so that there is some coordination between the PLL and the > PLLDIV clocks in order to get the < 300MHz operating points. Just to make sure we are on the same page. The datasheet constraint is 600 >= PLLOUT >= 300. PLLOUT is output of POSTDIV. The operating points are defined in terms of ARM frequency (and voltage). The OPPs defined in kernel today are here: https://git.kernel.org/pub/scm/linux/kernel/git/nsekhar/linux-davinci.git/tree/arch/arm/mach-davinci/da850.c#n1092 Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/12/2018 10:18 AM, Sekhar Nori wrote: > On Friday 12 January 2018 08:55 PM, David Lechner wrote: >>> >>> PLL output on DA850 must never be below 300MHz or above 600MHz (see >>> datasheet table "Allowed PLL Operating Conditions"). Does this take care >>> of that? Thats one of the main reasons I recall I went with some >>> specific values of prediv, pllm and post div in >>> arch/arm/mach-davinci/da850.c >> >> Apparently, I missed this requirement. It looks like I am going to have to >> rework things so that there is some coordination between the PLL and the >> PLLDIV clocks in order to get the < 300MHz operating points. > > Just to make sure we are on the same page. The datasheet > constraint is 600 >= PLLOUT >= 300. PLLOUT is output of POSTDIV. Hmm... I am on a different page. It looks to me like PLLOUT is the output of PLLM, not POSTDIV. The datasheet says nothing at all and the TRM does not say it explicitly, but footnote 2 on the table "System PLLC Output Clocks", for example, makes it pretty clear. > > The operating points are defined in terms of ARM frequency (and > voltage). The OPPs defined in kernel today are here: > https://git.kernel.org/pub/scm/linux/kernel/git/nsekhar/linux-davinci.git/tree/arch/arm/mach-davinci/da850.c#n1092 > > Thanks, > Sekhar > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/12/2018 03:21 AM, Sekhar Nori wrote: > On Monday 08 January 2018 07:47 AM, David Lechner wrote: >> +static unsigned long davinci_pll_clk_recalc(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + struct davinci_pll_clk *pll = to_davinci_pll_clk(hw); >> + unsigned long rate = parent_rate; >> + u32 prediv, mult, postdiv; >> + >> + prediv = readl(pll->base + PREDIV) & PREDIV_RATIO_MASK; >> + mult = readl(pll->base + PLLM) & PLLM_MASK; >> + postdiv = readl(pll->base + POSTDIV) & POSTDIV_RATIO_MASK; > > Shouldn't we check if the pre and post dividers are enabled before using > them? I dug into this and the answer is no. The enable bit acts like a gate, not a bypass, so it does not affect the rate calculation. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 13 January 2018 07:43 AM, David Lechner wrote: > On 01/12/2018 03:21 AM, Sekhar Nori wrote: >> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>> +static unsigned long davinci_pll_clk_recalc(struct clk_hw *hw, >>> + unsigned long parent_rate) >>> +{ >>> + struct davinci_pll_clk *pll = to_davinci_pll_clk(hw); >>> + unsigned long rate = parent_rate; >>> + u32 prediv, mult, postdiv; >>> + >>> + prediv = readl(pll->base + PREDIV) & PREDIV_RATIO_MASK; >>> + mult = readl(pll->base + PLLM) & PLLM_MASK; >>> + postdiv = readl(pll->base + POSTDIV) & POSTDIV_RATIO_MASK; >> >> Shouldn't we check if the pre and post dividers are enabled before using >> them? > > I dug into this and the answer is no. The enable bit acts like a gate, not > a bypass, so it does not affect the rate calculation. Alright, thanks for checking this. Regards, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 13 January 2018 06:41 AM, David Lechner wrote: > On 01/12/2018 10:18 AM, Sekhar Nori wrote: >> On Friday 12 January 2018 08:55 PM, David Lechner wrote: >>>> >>>> PLL output on DA850 must never be below 300MHz or above 600MHz (see >>>> datasheet table "Allowed PLL Operating Conditions"). Does this take >>>> care >>>> of that? Thats one of the main reasons I recall I went with some >>>> specific values of prediv, pllm and post div in >>>> arch/arm/mach-davinci/da850.c >>> >>> Apparently, I missed this requirement. It looks like I am going to >>> have to >>> rework things so that there is some coordination between the PLL and the >>> PLLDIV clocks in order to get the < 300MHz operating points. >> >> Just to make sure we are on the same page. The datasheet >> constraint is 600 >= PLLOUT >= 300. PLLOUT is output of POSTDIV. > > Hmm... I am on a different page. It looks to me like PLLOUT is the output > of PLLM, not POSTDIV. The datasheet says nothing at all and the TRM does > not say it explicitly, but footnote 2 on the table "System PLLC Output > Clocks", for example, makes it pretty clear. You are right. There is also this note in "Device clock generation" of TRM which makes this clear. " The PLLOUT stage in PLLC0 and PLLC1 is capable of providing frequencies greater than what the SYSCLK dividers can handle. The POSTDIV stage should be programmed to keep the input to the SYSCLK dividers within operating limits. See the device datasheet for the maximum operating frequencies. " Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > +void __init da850_pll_clk_init(void __iomem *pll0, void __iomem *pll1) > +{ > + const struct davinci_pll_divclk_info *info; > + > + davinci_pll_clk_register("pll0", "ref_clk", pll0); I think this will be more readable with empty lines between these function calls. So here .. > + davinci_pll_aux_clk_register("pll0_aux_clk", "ref_clk", pll0); .. here > + for (info = da850_pll0_divclk_info; info->name; info++) > + davinci_pll_divclk_register(info, pll0); > + > + davinci_pll_clk_register("pll1", "ref_clk", pll1); .. and here. > + for (info = da850_pll1_divclk_info; info->name; info++) > + davinci_pll_divclk_register(info, pll1); I see that you have included empty line only when changing the PLL, but I feel its more readable with the additional spacing suggested. Same comment for other patches too with similar function implemented. With that minor comment: Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds platform-specific declarations for the PLL clocks on TI > DaVinci 355 based systems. DM355 > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds platform-specific declarations for the PLL clocks on TI > DaVinci 365 based systems. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > drivers/clk/davinci/Makefile | 1 + > drivers/clk/davinci/pll-dm365.c | 64 +++++++++++++++++++++++++++++++++++++++++ > include/linux/clk/davinci.h | 1 + > 3 files changed, 66 insertions(+) > create mode 100644 drivers/clk/davinci/pll-dm365.c > > diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile > index 6720bd0..353aa02 100644 > --- a/drivers/clk/davinci/Makefile > +++ b/drivers/clk/davinci/Makefile > @@ -5,4 +5,5 @@ obj-y += pll.o > obj-$(CONFIG_ARCH_DAVINCI_DA830) += pll-da830.o > obj-$(CONFIG_ARCH_DAVINCI_DA850) += pll-da850.o > obj-$(CONFIG_ARCH_DAVINCI_DM355) += pll-dm355.o > +obj-$(CONFIG_ARCH_DAVINCI_DM365) += pll-dm365.o > endif > diff --git a/drivers/clk/davinci/pll-dm365.c b/drivers/clk/davinci/pll-dm365.c > new file mode 100644 > index 0000000..9892b0b > --- /dev/null > +++ b/drivers/clk/davinci/pll-dm365.c > @@ -0,0 +1,64 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PLL clock descriptions for TI DM365 > + * > + * Copyright (C) 2017 David Lechner <david@lechnology.com> > + */ > + > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/types.h> > + > +#include "pll.h" > + > +static const char * const dm365_pll_obsclk_parent_names[] = { > + "ref_clk", > +}; > + > +static u32 dm365_pll_obsclk_table[] = { > + 0x10, Perhaps use a #define here. Something like OBSCLK_SEL_OSC. > +void __init dm365_pll_clk_init(void __iomem *pll1, void __iomem *pll2) > +{ > + const struct davinci_pll_divclk_info *info; > + > + davinci_pll_clk_register("pll1", "ref_clk", pll1); > + davinci_pll_aux_clk_register("pll1_aux_clk", "ref_clk", pll1); > + davinci_pll_bpdiv_clk_register("pll1_sysclkbp", "ref_clk", pll1); > + davinci_pll_obs_clk_register("clkout0", dm365_pll_obsclk_parent_names, > + ARRAY_SIZE(dm365_pll_obsclk_parent_names), > + pll1, dm365_pll_obsclk_table); > + for (info = dm365_pll1_divclk_info; info->name; info++) > + davinci_pll_divclk_register(info, pll1); > + > + davinci_pll_clk_register("pll2", "ref_clk", pll2); > + davinci_pll_aux_clk_register("clkout1", "ref_clk", pll2); > + davinci_pll_obs_clk_register("clkout1", dm365_pll_obsclk_parent_names, > + ARRAY_SIZE(dm365_pll_obsclk_parent_names), > + pll2, dm365_pll_obsclk_table); > + for (info = dm365_pll2_divclk_info; info->name; info++) > + davinci_pll_divclk_register(info, pll2); > +} With that and the empty line comment I gave earlier. Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds platform-specific declarations for the PLL clocks on TI > DaVinci 644x based systems. DM644x > Signed-off-by: David Lechner <david@lechnology.com> > + * Copyright (C) 2017 David Lechner <david@lechnology.com> 2018 now. > +void __init dm644x_pll_clk_init(void __iomem *pll1, void __iomem *pll2) > +{ > + const struct davinci_pll_divclk_info *info; > + > + davinci_pll_clk_register("pll1", "ref_clk", pll1); > + for (info = dm644x_pll1_divclk_info; info->name; info++) > + davinci_pll_divclk_register(info, pll1); > + davinci_pll_aux_clk_register("pll1_aux_clk", "ref_clk", pll1); > + davinci_pll_bpdiv_clk_register("pll1_sysclkbp", "ref_clk", pll1); > + > + davinci_pll_clk_register("pll2", "ref_clk", pll2); > + for (info = dm644x_pll2_divclk_info; info->name; info++) > + davinci_pll_divclk_register(info, pll2); > + davinci_pll_bpdiv_clk_register("pll2_sysclkbp", "ref_clk", pll2); > +} Here too, more line spacing will help. With those minor comments. Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds platform-specific declarations for the PLL clocks on TI > DaVinci 646x based systems. > > Signed-off-by: David Lechner <david@lechnology.com> Some minor comments I gave on other patches apply here too. Apart from those: Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds a new driver for mach-davinci PSC clocks. This is porting the > code from arch/arm/mach-davinci/psc.c to the common clock framework and > is converting it to use regmap to simplify the code. Additionally, it adds > device tree support for these clocks. > > Note: although there are similar clocks for TI Keystone we are not able > to share the code for a few reasons. The keystone clocks are device tree > only and use legacy one-node-per-clock bindings. Also the keystone driver > makes the assumption that there is only one PSC per SoC and uses global > variables, but here we have two controllers per SoC. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > drivers/clk/davinci/Makefile | 2 + > drivers/clk/davinci/psc.c | 282 +++++++++++++++++++++++++++++++++++++++++++ > drivers/clk/davinci/psc.h | 49 ++++++++ > 3 files changed, 333 insertions(+) > create mode 100644 drivers/clk/davinci/psc.c > create mode 100644 drivers/clk/davinci/psc.h > > diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile > index d471386..cd1bf2c 100644 > --- a/drivers/clk/davinci/Makefile > +++ b/drivers/clk/davinci/Makefile > @@ -8,4 +8,6 @@ obj-$(CONFIG_ARCH_DAVINCI_DM355) += pll-dm355.o > obj-$(CONFIG_ARCH_DAVINCI_DM365) += pll-dm365.o > obj-$(CONFIG_ARCH_DAVINCI_DM644x) += pll-dm644x.o > obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o > + > +obj-y += psc.o > endif > diff --git a/drivers/clk/davinci/psc.c b/drivers/clk/davinci/psc.c > new file mode 100644 > index 0000000..a8b5f57 > --- /dev/null > +++ b/drivers/clk/davinci/psc.c > @@ -0,0 +1,282 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Clock driver for TI Davinci PSC controllers > + * > + * Copyright (C) 2017 David Lechner <david@lechnology.com> 2018 > + * > + * Based on: drivers/clk/keystone/gate.c > + * Copyright (C) 2013 Texas Instruments. > + * Murali Karicheri <m-karicheri2@ti.com> > + * Santosh Shilimkar <santosh.shilimkar@ti.com> > + * > + * And: arch/arm/mach-davinci/psc.c > + * Copyright (C) 2006 Texas Instruments. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/clk/davinci.h> > +#include <linux/clkdev.h> > +#include <linux/err.h> > +#include <linux/of_address.h> > +#include <linux/of.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > +#include <linux/types.h> > + > +#include "psc.h" > + > +/* PSC register offsets */ > +#define EPCPR 0x070 > +#define PTCMD 0x120 > +#define PTSTAT 0x128 > +#define PDSTAT(n) (0x200 + 4 * (n)) > +#define PDCTL(n) (0x300 + 4 * (n)) > +#define MDSTAT(n) (0x800 + 4 * (n)) > +#define MDCTL(n) (0xa00 + 4 * (n)) > + > +/* PSC module states */ > +enum davinci_psc_state { > + PSC_STATE_SWRSTDISABLE = 0, > + PSC_STATE_SYNCRST = 1, > + PSC_STATE_DISABLE = 2, > + PSC_STATE_ENABLE = 3, > +}; > + > +#define MDSTAT_STATE_MASK 0x3f> +#define MDSTAT_MCKOUT BIT(12) > +#define PDSTAT_STATE_MASK 0x1f GENMASK() for masks. > +#define MDCTL_FORCE BIT(31) > +#define MDCTL_LRESET BIT(8) > +#define PDCTL_EPCGOOD BIT(8) > +#define PDCTL_NEXT BIT(0) > + > +/** > + * struct davinci_psc_clk - PSC clock structure > + * @hw: clk_hw for the psc > + * @regmap: PSC MMIO region > + * @lpsc: Local PSC number (module id) > + * @pd: Power domain > + * @flags: LPSC_* quirk flags > + */ > +struct davinci_psc_clk { > + struct clk_hw hw; > + struct regmap *regmap; > + u32 lpsc; > + u32 pd; > + u32 flags; > +}; > + > +#define to_davinci_psc_clk(_hw) container_of(_hw, struct davinci_psc_clk, hw) > + > +static void psc_config(struct davinci_psc_clk *psc, > + enum davinci_psc_state next_state) > +{ > + u32 epcpr, pdstat, mdstat, mdctl, ptstat; > + > + mdctl = next_state; > + if (psc->flags & LPSC_FORCE) > + mdctl |= MDCTL_FORCE; > + regmap_write_bits(psc->regmap, MDCTL(psc->lpsc), MDSTAT_STATE_MASK, > + mdctl); Wont this ignore the MDCTL_FORCE bit since MDSTAT_STATE_MASK does not cover that? > + > + regmap_read(psc->regmap, PDSTAT(psc->pd), &pdstat); > + if ((pdstat & PDSTAT_STATE_MASK) == 0) { > + regmap_write_bits(psc->regmap, PDSTAT(psc->pd), > + PDSTAT_STATE_MASK, PDCTL_NEXT); Shouldn't this be a write to PDCTL register? > + > + regmap_write(psc->regmap, PTCMD, BIT(psc->pd)); > + > + regmap_read_poll_timeout(psc->regmap, EPCPR, epcpr, > + epcpr & BIT(psc->pd), 0, 0); > + > + regmap_write_bits(psc->regmap, PDCTL(psc->pd), PDCTL_EPCGOOD, > + PDCTL_EPCGOOD); > + } else { > + regmap_write(psc->regmap, PTCMD, BIT(psc->pd)); > + } > + > + regmap_read_poll_timeout(psc->regmap, PTSTAT, ptstat, > + !(ptstat & BIT(psc->pd)), 0, 0); > + > + regmap_read_poll_timeout(psc->regmap, MDSTAT(psc->lpsc), mdstat, > + (mdstat & MDSTAT_STATE_MASK) == next_state, > + 0, 0); > +} > + [...] > + > +/** > + * davinci_psc_clk_register - register psc clock > + * @dev: device that is registering this clock No dev parameter below. > + * @name: name of this clock > + * @parent_name: name of clock's parent > + * @regmap: PSC MMIO region > + * @lpsc: local PSC number > + * @pd: power domain > + * @flags: LPSC_* flags > + */ > +static struct clk *davinci_psc_clk_register(const char *name, > + const char *parent_name, > + struct regmap *regmap, > + u32 lpsc, u32 pd, u32 flags) > +{ > + struct clk_init_data init; > + struct davinci_psc_clk *psc; > + struct clk *clk; > + > + psc = kzalloc(sizeof(*psc), GFP_KERNEL); > + if (!psc) > + return ERR_PTR(-ENOMEM); > + > + init.name = name; > + init.ops = &davinci_psc_clk_ops; > + init.parent_names = (parent_name ? &parent_name : NULL); > + init.num_parents = (parent_name ? 1 : 0); > + init.flags = CLK_SET_RATE_PARENT; Is this needed since PSC does not cause any rate change? > + > + if (flags & LPSC_ALWAYS_ENABLED) > + init.flags |= CLK_IS_CRITICAL; > + > + psc->regmap = regmap; > + psc->hw.init = &init; > + psc->lpsc = lpsc; > + psc->pd = pd; > + psc->flags = flags; > + > + clk = clk_register(NULL, &psc->hw); > + if (IS_ERR(clk)) > + kfree(psc); > + > + return clk; > +} > + > +/* > + * FIXME: This needs to be converted to a reset controller. But, the reset > + * framework is currently device tree only. Yeah, I see that __reset_control_get() fails with -EINVAL if there is no of_node. > + */ > + > +static int davinci_psc_clk_reset(struct davinci_psc_clk *psc, bool reset) > +{ > + u32 mdctl; > + > + if (IS_ERR_OR_NULL(psc)) > + return -EINVAL; > + > + mdctl = reset ? 0 : MDCTL_LRESET; > + regmap_write_bits(psc->regmap, MDCTL(psc->lpsc), MDCTL_LRESET, mdctl); > + > + return 0; > +} > + > +int davinci_clk_reset_assert(struct clk *clk) > +{ > + struct davinci_psc_clk *psc = to_davinci_psc_clk(__clk_get_hw(clk)); > + > + return davinci_psc_clk_reset(psc, true); > +} > +EXPORT_SYMBOL(davinci_clk_reset_assert); > + > +int davinci_clk_reset_deassert(struct clk *clk) > +{ > + struct davinci_psc_clk *psc = to_davinci_psc_clk(__clk_get_hw(clk)); > + > + return davinci_psc_clk_reset(psc, false); > +} > +EXPORT_SYMBOL(davinci_clk_reset_deassert); > + [...] > diff --git a/drivers/clk/davinci/psc.h b/drivers/clk/davinci/psc.h > new file mode 100644 > index 0000000..6022f6e > --- /dev/null > +++ b/drivers/clk/davinci/psc.h > @@ -0,0 +1,49 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Clock driver for TI Davinci PSC controllers > + * > + * Copyright (C) 2017 David Lechner <david@lechnology.com> > + */ > + > +#ifndef __CLK_DAVINCI_PSC_H__ > +#define __CLK_DAVINCI_PSC_H__ > + > +#include <linux/types.h> > + > +/* PSC quirk flags */ > +#define LPSC_ALWAYS_ENABLED BIT(1) /* never disable this clock */ > +#define LPSC_FORCE BIT(2) /* requires MDCTL FORCE bit */ > +#define LPSC_LOCAL_RESET BIT(3) /* acts as reset provider */ > + > +struct clk_onecell_data; Rather clk-provider.h should be included in this file? Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds platform-specific declarations for the PSC clocks on TI DA830/ > OMAP-L137/AM17XX SoCs. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > drivers/clk/davinci/Makefile | 1 + > drivers/clk/davinci/psc-da830.c | 96 +++++++++++++++++++++++++++++++++++++++++ > include/linux/clk/davinci.h | 2 + > 3 files changed, 99 insertions(+) > create mode 100644 drivers/clk/davinci/psc-da830.c > > diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile > index cd1bf2c..fb14c8c 100644 > --- a/drivers/clk/davinci/Makefile > +++ b/drivers/clk/davinci/Makefile > @@ -10,4 +10,5 @@ obj-$(CONFIG_ARCH_DAVINCI_DM644x) += pll-dm644x.o > obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o > > obj-y += psc.o > +obj-$(CONFIG_ARCH_DAVINCI_DA830) += psc-da830.o > endif > diff --git a/drivers/clk/davinci/psc-da830.c b/drivers/clk/davinci/psc-da830.c > new file mode 100644 > index 0000000..193b08f > --- /dev/null > +++ b/drivers/clk/davinci/psc-da830.c > @@ -0,0 +1,96 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PSC clock descriptions for TI DA830/OMAP-L137/AM17XX > + * > + * Copyright (C) 2017 David Lechner <david@lechnology.com> > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> > +#include <linux/init.h> > +#include <linux/types.h> > + > +#include "psc.h" > + > +static const struct davinci_psc_clk_info da830_psc0_info[] __initconst = { > + LPSC(0, 0, tpcc, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(3, 0, aemif, pll0_sysclk3, LPSC_ALWAYS_ENABLED), > + LPSC(4, 0, spi0, pll0_sysclk2, 0), > + LPSC(5, 0, mmcsd, pll0_sysclk2, 0), > + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), > + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(8, 0, secu_mgr, pll0_sysclk4, LPSC_ALWAYS_ENABLED), > + LPSC(9, 0, uart0, pll0_sysclk2, 0), > + LPSC(10, 0, scr0_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(11, 0, scr1_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(12, 0, scr2_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(13, 0, dmax, pll0_sysclk2, LPSC_ALWAYS_ENABLED), pruss is better (I know the name is coming from existing code). > + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), This is LPSC 15 which controls DSP too. But its missing from existing code. Not sure why. Probably a note for future. For now okay with ignoring it. > + { } > +}; Tables like these are much easier to parse if columns are spaced using a tab. > + > +static const struct davinci_psc_clk_info da830_psc1_info[] __initconst = { > + LPSC(1, 0, usb0, pll0_sysclk2, 0), > + LPSC(2, 0, usb1, pll0_sysclk4, 0), > + LPSC(3, 0, gpio, pll0_sysclk4, 0), There is LPSC 4 controlling UHPI. Again, lets ignore for now. > + LPSC(5, 0, emac, pll0_sysclk4, 0), > + LPSC(6, 0, emif3, pll0_sysclk5, LPSC_ALWAYS_ENABLED), > + LPSC(7, 0, mcasp0, pll0_sysclk2, 0), > + LPSC(8, 0, mcasp1, pll0_sysclk2, 0), > + LPSC(9, 0, mcasp2, pll0_sysclk2, 0), > + LPSC(10, 0, spi1, pll0_sysclk2, 0), > + LPSC(11, 0, i2c1, pll0_sysclk4, 0), > + LPSC(12, 0, uart1, pll0_sysclk2, 0), > + LPSC(13, 0, uart2, pll0_sysclk2, 0), > + LPSC(16, 0, lcdc, pll0_sysclk2, 0), > + LPSC(17, 0, pwm, pll0_sysclk2, 0), > + LPSC(20, 0, ecap, pll0_sysclk2, 0), > + LPSC(21, 0, eqep, pll0_sysclk2, 0), > + { } > +}; > + > +void __init da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1) > +{ > + struct clk_onecell_data *clk_data; > + > + clk_data = davinci_psc_register_clocks(psc0, da830_psc0_info, 16); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); > + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); > + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); > + clk_register_clkdev(clk_data->clks[14], "arm", NULL); > + > + clk_free_onecell_data(clk_data); > + > + clk_data = davinci_psc_register_clocks(psc1, da830_psc1_info, 32); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); > + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); > + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); > + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); This is pretty bad (and no fault of yours) - having a con_id but no device name. Can you please make a pre-series which passes NULL con_id in gpio-davinci.c? > + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); > + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); > + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); > + clk_register_clkdev(clk_data->clks[8], NULL, "davinci-mcasp.1"); > + clk_register_clkdev(clk_data->clks[9], NULL, "davinci-mcasp.2"); > + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); > + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); > + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); > + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); > + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); > + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); > + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); > + clk_register_clkdev(clk_data->clks[21], NULL, "eqep.0"); > + clk_register_clkdev(clk_data->clks[21], NULL, "eqep.1"); This is going to be very difficult to audit for mistakes. How do you feel about adding the con_id and dev_id to davinci_psc_clk_info[] so they can be initialized as part of a single static table? And then here you go over the table looking for non-NULL con_id/dev_id to call clk_register_clkdev()? I am guessing you did not take that route because the DT path does not need those. But still, I think that will be much less error prone. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) > +{ > + struct clk_onecell_data *clk_data; > + > + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); > + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); > + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); > + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); > + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); > + clk_register_clkdev(clk_data->clks[14], "arm", NULL); > + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); > + > + clk_free_onecell_data(clk_data); > + > + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); Is this con_id really needed now? Searching for "usb20_psc_clk" in your tree results in only this one hit. > + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); > + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); I guess multiple dev_id matches like these are another hurdle in moving them to davinci_psc_clk_info[] table? If its too cumbersome to keep multiple entries in the table, they can be handled as an exception at the end of processing the table? Still they are not the norm so I hope the normal case will still benefit. > + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); > + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); > + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); > + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); > + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); > + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); > + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); > + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); > + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); > + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); > + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); > + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); > + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); > + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); > + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); > + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); > + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); > + > + clk_free_onecell_data(clk_data); > +} Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds platform-specific declarations for the PSC clocks on TI > DaVinci 355 based systems. > > Signed-off-by: David Lechner <david@lechnology.com> Apart from some comments on other patches that apply to this too, I have no new comments. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds platform-specific declarations for the PSC clocks on TI > DaVinci 365 based systems. > > Signed-off-by: David Lechner <david@lechnology.com> Apart from some comments on other patches that apply to this too, I have no new comments. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/16/2018 05:03 AM, Sekhar Nori wrote: > On Monday 08 January 2018 07:47 AM, David Lechner wrote: >> This adds a new driver for mach-davinci PSC clocks. This is porting the >> code from arch/arm/mach-davinci/psc.c to the common clock framework and >> is converting it to use regmap to simplify the code. Additionally, it adds >> device tree support for these clocks. >> >> Note: although there are similar clocks for TI Keystone we are not able >> to share the code for a few reasons. The keystone clocks are device tree >> only and use legacy one-node-per-clock bindings. Also the keystone driver >> makes the assumption that there is only one PSC per SoC and uses global >> variables, but here we have two controllers per SoC. >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> +static void psc_config(struct davinci_psc_clk *psc, >> + enum davinci_psc_state next_state) >> +{ >> + u32 epcpr, pdstat, mdstat, mdctl, ptstat; >> + >> + mdctl = next_state; >> + if (psc->flags & LPSC_FORCE) >> + mdctl |= MDCTL_FORCE; >> + regmap_write_bits(psc->regmap, MDCTL(psc->lpsc), MDSTAT_STATE_MASK, >> + mdctl); > > Wont this ignore the MDCTL_FORCE bit since MDSTAT_STATE_MASK does not > cover that? > >> + >> + regmap_read(psc->regmap, PDSTAT(psc->pd), &pdstat); >> + if ((pdstat & PDSTAT_STATE_MASK) == 0) { >> + regmap_write_bits(psc->regmap, PDSTAT(psc->pd), >> + PDSTAT_STATE_MASK, PDCTL_NEXT); > > Shouldn't this be a write to PDCTL register? > Looks like I have some mistakes here. Thank you. ... >> +static struct clk *davinci_psc_clk_register(const char *name, >> + const char *parent_name, >> + struct regmap *regmap, >> + u32 lpsc, u32 pd, u32 flags) >> +{ >> + struct clk_init_data init; >> + struct davinci_psc_clk *psc; >> + struct clk *clk; >> + >> + psc = kzalloc(sizeof(*psc), GFP_KERNEL); >> + if (!psc) >> + return ERR_PTR(-ENOMEM); >> + >> + init.name = name; >> + init.ops = &davinci_psc_clk_ops; >> + init.parent_names = (parent_name ? &parent_name : NULL); >> + init.num_parents = (parent_name ? 1 : 0); >> + init.flags = CLK_SET_RATE_PARENT; > > Is this needed since PSC does not cause any rate change? Yes, because one of the PSCs is the ARM clock and for cpufreq, we need to propagate the rate change up the chain to SYSCLK6. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/16/2018 07:38 AM, Sekhar Nori wrote: > On Monday 08 January 2018 07:47 AM, David Lechner wrote: >> This adds platform-specific declarations for the PSC clocks on TI DA830/ >> OMAP-L137/AM17XX SoCs. >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> drivers/clk/davinci/Makefile | 1 + >> drivers/clk/davinci/psc-da830.c | 96 +++++++++++++++++++++++++++++++++++++++++ >> include/linux/clk/davinci.h | 2 + >> 3 files changed, 99 insertions(+) >> create mode 100644 drivers/clk/davinci/psc-da830.c >> >> diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile >> index cd1bf2c..fb14c8c 100644 >> --- a/drivers/clk/davinci/Makefile >> +++ b/drivers/clk/davinci/Makefile >> @@ -10,4 +10,5 @@ obj-$(CONFIG_ARCH_DAVINCI_DM644x) += pll-dm644x.o >> obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o >> >> obj-y += psc.o >> +obj-$(CONFIG_ARCH_DAVINCI_DA830) += psc-da830.o >> endif >> diff --git a/drivers/clk/davinci/psc-da830.c b/drivers/clk/davinci/psc-da830.c >> new file mode 100644 >> index 0000000..193b08f >> --- /dev/null >> +++ b/drivers/clk/davinci/psc-da830.c >> @@ -0,0 +1,96 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * PSC clock descriptions for TI DA830/OMAP-L137/AM17XX >> + * >> + * Copyright (C) 2017 David Lechner <david@lechnology.com> >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/clkdev.h> >> +#include <linux/init.h> >> +#include <linux/types.h> >> + >> +#include "psc.h" >> + >> +static const struct davinci_psc_clk_info da830_psc0_info[] __initconst = { >> + LPSC(0, 0, tpcc, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(3, 0, aemif, pll0_sysclk3, LPSC_ALWAYS_ENABLED), >> + LPSC(4, 0, spi0, pll0_sysclk2, 0), >> + LPSC(5, 0, mmcsd, pll0_sysclk2, 0), >> + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >> + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(8, 0, secu_mgr, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >> + LPSC(9, 0, uart0, pll0_sysclk2, 0), >> + LPSC(10, 0, scr0_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(11, 0, scr1_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(12, 0, scr2_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(13, 0, dmax, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > > pruss is better (I know the name is coming from existing code). > >> + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), > > This is LPSC 15 which controls DSP too. But its missing from existing > code. Not sure why. Probably a note for future. For now okay with > ignoring it. > >> + { } >> +}; > > Tables like these are much easier to parse if columns are spaced using a > tab. > Tabs make the lines over 80 columns. How about spaces instead? >> + >> +static const struct davinci_psc_clk_info da830_psc1_info[] __initconst = { >> + LPSC(1, 0, usb0, pll0_sysclk2, 0), >> + LPSC(2, 0, usb1, pll0_sysclk4, 0), >> + LPSC(3, 0, gpio, pll0_sysclk4, 0), > > There is LPSC 4 controlling UHPI. Again, lets ignore for now. > >> + LPSC(5, 0, emac, pll0_sysclk4, 0), >> + LPSC(6, 0, emif3, pll0_sysclk5, LPSC_ALWAYS_ENABLED), >> + LPSC(7, 0, mcasp0, pll0_sysclk2, 0), >> + LPSC(8, 0, mcasp1, pll0_sysclk2, 0), >> + LPSC(9, 0, mcasp2, pll0_sysclk2, 0), >> + LPSC(10, 0, spi1, pll0_sysclk2, 0), >> + LPSC(11, 0, i2c1, pll0_sysclk4, 0), >> + LPSC(12, 0, uart1, pll0_sysclk2, 0), >> + LPSC(13, 0, uart2, pll0_sysclk2, 0), >> + LPSC(16, 0, lcdc, pll0_sysclk2, 0), >> + LPSC(17, 0, pwm, pll0_sysclk2, 0), >> + LPSC(20, 0, ecap, pll0_sysclk2, 0), >> + LPSC(21, 0, eqep, pll0_sysclk2, 0), >> + { } >> +}; >> + >> +void __init da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >> +{ >> + struct clk_onecell_data *clk_data; >> + >> + clk_data = davinci_psc_register_clocks(psc0, da830_psc0_info, 16); >> + if (!clk_data) >> + return; >> + >> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >> + >> + clk_free_onecell_data(clk_data); >> + >> + clk_data = davinci_psc_register_clocks(psc1, da830_psc1_info, 32); >> + if (!clk_data) >> + return; >> + >> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); >> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); > > This is pretty bad (and no fault of yours) - having a con_id but no > device name. Can you please make a pre-series which passes NULL con_id > in gpio-davinci.c? I'll give it a try. This is complicated by the fact that the con_id has made it's way into the device tree bindings. However, I think we can safely deprecate clock-names = "gpio" in the device tree bindings since we can make the driver ignore that property to preserve backwards compatibility. > >> + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); >> + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); >> + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); >> + clk_register_clkdev(clk_data->clks[8], NULL, "davinci-mcasp.1"); >> + clk_register_clkdev(clk_data->clks[9], NULL, "davinci-mcasp.2"); >> + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); >> + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); >> + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); >> + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); >> + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); >> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); >> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); >> + clk_register_clkdev(clk_data->clks[21], NULL, "eqep.0"); >> + clk_register_clkdev(clk_data->clks[21], NULL, "eqep.1"); > > This is going to be very difficult to audit for mistakes. How do you > feel about adding the con_id and dev_id to davinci_psc_clk_info[] so > they can be initialized as part of a single static table? And then here > you go over the table looking for non-NULL con_id/dev_id to call > clk_register_clkdev()? > > I am guessing you did not take that route because the DT path does not > need those. But still, I think that will be much less error prone. I wasn't really happy with this either, but I haven't come up with anything better. The reason I think this is good enough is that the array index is the LPSC number, so it is not actually that hard to audit. You can use the clock declarations above (or the datasheet/ TRM) to know exactly which clock you are dealing with by matching the array index. I considered using macros instead of the numbers, but then lines are over 80 columns and it gets hard to read with wrapped lines. Any other solution will be much more verbose and require helper functions for looping through the clocks. I'm thinking that I would define a list of con_id, dev_id pairs for each clock, so that would be 15 extra static arrays here. I'll think about it a little more, but for now, I would like to push for just calling this "good enough" and leave it as-is. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/16/2018 08:00 AM, Sekhar Nori wrote: > On Monday 08 January 2018 07:47 AM, David Lechner wrote: >> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >> +{ >> + struct clk_onecell_data *clk_data; >> + >> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >> + if (!clk_data) >> + return; >> + >> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >> + >> + clk_free_onecell_data(clk_data); >> + >> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >> + if (!clk_data) >> + return; >> + >> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); > > Is this con_id really needed now? Searching for "usb20_psc_clk" in your > tree results in only this one hit. Yes, this is left over from previous attempts. > >> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); > > I guess multiple dev_id matches like these are another hurdle in moving > them to davinci_psc_clk_info[] table? If its too cumbersome to keep > multiple entries in the table, they can be handled as an exception at > the end of processing the table? Still they are not the norm so I hope > the normal case will still benefit. Right, as I mentioned in the reply to the previous patch, instead of assigning a con_id and dev_id to each clock, we would need to assign an array with a list of clocks. I think that would work better than trying to handle the extras as an exception since there, on average, about 5 per SoC. > >> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); >> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); >> + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); >> + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); >> + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); >> + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); >> + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); >> + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); >> + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); >> + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); >> + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); >> + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); >> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); >> + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); >> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); >> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); >> + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); >> + >> + clk_free_onecell_data(clk_data); >> +} > > Thanks, > Sekhar > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 January 2018 10:51 PM, David Lechner wrote: > On 01/16/2018 08:00 AM, Sekhar Nori wrote: >> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>> +{ >>> + struct clk_onecell_data *clk_data; >>> + >>> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >>> + if (!clk_data) >>> + return; >>> + >>> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >>> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >>> + >>> + clk_free_onecell_data(clk_data); >>> + >>> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >>> + if (!clk_data) >>> + return; >>> + >>> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); >> >> Is this con_id really needed now? Searching for "usb20_psc_clk" in your >> tree results in only this one hit. > > Yes, this is left over from previous attempts. > >> >>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >> >> I guess multiple dev_id matches like these are another hurdle in moving >> them to davinci_psc_clk_info[] table? If its too cumbersome to keep >> multiple entries in the table, they can be handled as an exception at >> the end of processing the table? Still they are not the norm so I hope >> the normal case will still benefit. > > Right, as I mentioned in the reply to the previous patch, instead of > assigning a con_id and dev_id to each clock, we would need to assign > an array with a list of clocks. I think that would work better than > trying to handle the extras as an exception since there, on average, > about 5 per SoC. Okay, are you going to try this to see how it looks? It looks like samsung (clk-s3c2410.c) and tegra (clk-tegra20.c) use such tables (although both use separate tables mapping just the gate number to con_id/dev_id). Others like u8540_clk.c and clk-mmp2.c have multiple calls in code to clk_register_clkdev() like you have, but they keep them right after the gate clock registration which makes it easy to see the mapping. clk-imx35.c has multiple clk_register_clkdev() calls, but uses an enum for the gates so its easy to see the mapping. This approach looks fine to me as well. So looks like there is a whole gamut of ways people have approached this. But I do think we need to change the scheme you have currently since it is difficult to review and audit (believe me on this one :)) Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 January 2018 10:46 PM, David Lechner wrote: >>> +static const struct davinci_psc_clk_info da830_psc0_info[] >>> __initconst = { >>> + LPSC(0, 0, tpcc, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(3, 0, aemif, pll0_sysclk3, LPSC_ALWAYS_ENABLED), >>> + LPSC(4, 0, spi0, pll0_sysclk2, 0), >>> + LPSC(5, 0, mmcsd, pll0_sysclk2, 0), >>> + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >>> + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(8, 0, secu_mgr, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >>> + LPSC(9, 0, uart0, pll0_sysclk2, 0), >>> + LPSC(10, 0, scr0_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(11, 0, scr1_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(12, 0, scr2_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(13, 0, dmax, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> >> pruss is better (I know the name is coming from existing code). >> >>> + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), >> >> This is LPSC 15 which controls DSP too. But its missing from existing >> code. Not sure why. Probably a note for future. For now okay with >> ignoring it. >> >>> + { } >>> +}; >> >> Tables like these are much easier to parse if columns are spaced using a >> tab. >> > > Tabs make the lines over 80 columns. How about spaces instead? That works for me. I guess checkpatch will complain, but hopefully maintainers will agree on the exception. >>> +void __init da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>> +{ >>> + struct clk_onecell_data *clk_data; >>> + >>> + clk_data = davinci_psc_register_clocks(psc0, da830_psc0_info, 16); >>> + if (!clk_data) >>> + return; >>> + >>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>> + >>> + clk_free_onecell_data(clk_data); >>> + >>> + clk_data = davinci_psc_register_clocks(psc1, da830_psc1_info, 32); >>> + if (!clk_data) >>> + return; >>> + >>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >>> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); >>> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); >> >> This is pretty bad (and no fault of yours) - having a con_id but no >> device name. Can you please make a pre-series which passes NULL con_id >> in gpio-davinci.c? > > I'll give it a try. This is complicated by the fact that the con_id has > made it's way into the device tree bindings. However, I think we can > safely deprecate clock-names = "gpio" in the device tree bindings since > we can make the driver ignore that property to preserve backwards > compatibility. I don't think this breaks DT-backward compatibility. Passing a NULL con_id in driver should find the clock for that device even if DT entry has clock-names present. As far as I can read clk_find(). The less intrusive alternate is to add the GPIO device name in the table here, while keeping the con_id and keeping the driver untouched. The advantage of that is lesser number of dependent patches for this series to go in. Later once CCF conversion has been there in the kernel for one full release and no regressions, these other clean-ups can be done. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 January 2018 10:21 PM, David Lechner wrote: >>> +static struct clk *davinci_psc_clk_register(const char *name, >>> + const char *parent_name, >>> + struct regmap *regmap, >>> + u32 lpsc, u32 pd, u32 flags) >>> +{ >>> + struct clk_init_data init; >>> + struct davinci_psc_clk *psc; >>> + struct clk *clk; >>> + >>> + psc = kzalloc(sizeof(*psc), GFP_KERNEL); >>> + if (!psc) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + init.name = name; >>> + init.ops = &davinci_psc_clk_ops; >>> + init.parent_names = (parent_name ? &parent_name : NULL); >>> + init.num_parents = (parent_name ? 1 : 0); >>> + init.flags = CLK_SET_RATE_PARENT; >> >> Is this needed since PSC does not cause any rate change? > > Yes, because one of the PSCs is the ARM clock and for cpufreq, we > need to propagate the rate change up the chain to SYSCLK6. Good point. But how about treating that as an exception with a new LPSC_ quirk flag? Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds platform-specific declarations for the PSC clocks on TI > DaVinci 644x based systems. > > Signed-off-by: David Lechner <david@lechnology.com> Looks good to me except comments I already gave for similar patches. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > +void __init dm646x_psc_clk_init(void __iomem *psc) > +{ > + struct clk_onecell_data *clk_data; > + > + clk_data = davinci_psc_register_clocks(psc, dm646x_psc_info, 41); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[0], "arm", NULL); I don't think this "arm" con_id is used any where for non-DA850 SoCs. And same with "dsp" in other files. Probably best to drop these dubious usage ones rather than carry them forward. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds a new driver for the gate and multiplexer clocks in the > CFGCHIPn syscon registers on TI DA8XX-type SoCs. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > drivers/clk/davinci/Makefile | 2 + > drivers/clk/davinci/da8xx-cfgchip.c | 203 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 205 insertions(+) > create mode 100644 drivers/clk/davinci/da8xx-cfgchip.c > > diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile > index 6c388d4..11178b7 100644 > --- a/drivers/clk/davinci/Makefile > +++ b/drivers/clk/davinci/Makefile > @@ -1,6 +1,8 @@ > # SPDX-License-Identifier: GPL-2.0 > > ifeq ($(CONFIG_COMMON_CLK), y) > +obj-$(CONFIG_ARCH_DAVINCI_DA8XX) += da8xx-cfgchip.o > + > obj-y += pll.o > obj-$(CONFIG_ARCH_DAVINCI_DA830) += pll-da830.o > obj-$(CONFIG_ARCH_DAVINCI_DA850) += pll-da850.o > diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c > new file mode 100644 > index 0000000..772e09a > --- /dev/null > +++ b/drivers/clk/davinci/da8xx-cfgchip.c > @@ -0,0 +1,203 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Clock driver for DA8xx/AM17xx/AM18xx/OMAP-L13x CFGCHIP > + * > + * Copyright (C) 2017 David Lechner <david@lechnology.com> 2018 > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/mfd/da8xx-cfgchip.h> > +#include <linux/mfd/syscon.h> > +#include <linux/of.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > + > +#ifdef CONFIG_OF Is this ifdef really needed, or included to save space for non-OF builds? I think it can be removed if not really needed. > +static void da8xx_cfgchip_gate_clk_init(struct device_node *np, u32 reg, > + u32 mask) > +{ > + struct da8xx_cfgchip_gate_clk *clk; > + struct clk_init_data init; > + const char *name = np->name; > + const char *parent_name; > + struct regmap *regmap; > + int ret; > + > + of_property_read_string(np, "clock-output-names", &name); > + parent_name = of_clk_get_parent_name(np, 0); > + > + regmap = syscon_node_to_regmap(of_get_parent(np)); > + if (IS_ERR(regmap)) { > + pr_err("%s: no regmap for syscon parent of %s (%ld)\n", > + __func__, np->full_name, PTR_ERR(regmap)); please use pr_fmt for this driver too. > +static void da8xx_cfgchip_mux_clk_init(struct device_node *np, u32 reg, > + u32 mask) > +{ > + struct da8xx_cfgchip_mux_clk *clk; > + struct clk_init_data init; > + const char *name = np->name; > + const char *parent_names[2]; > + struct regmap *regmap; > + int ret; > + > + ret = of_property_match_string(np, "clock-names", "pll0_sysclk2"); > + parent_names[0] = of_clk_get_parent_name(np, ret); > + if (!parent_names[0]) { > + pr_err("%s: missing pll0_sysclk2 clock\n", __func__); > + return; > + } > + > + ret = of_property_match_string(np, "clock-names", "pll1_sysclk2"); > + parent_names[1] = of_clk_get_parent_name(np, ret); > + if (!parent_names[1]) { > + pr_err("%s: missing pll1_sysclk2 clock\n", __func__); > + return; > + } The fact that you are looking specifically for pll0_sysclk2 and pll1_sysclk2 makes it really specific to async3 and the same function cannot be used for something like EMIFA clock source. Can this part of the function be factored out so rest of the function can still be reused for another clock? Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/17/2018 06:25 AM, Sekhar Nori wrote: > On Tuesday 16 January 2018 10:21 PM, David Lechner wrote: > >>>> +static struct clk *davinci_psc_clk_register(const char *name, >>>> + const char *parent_name, >>>> + struct regmap *regmap, >>>> + u32 lpsc, u32 pd, u32 flags) >>>> +{ >>>> + struct clk_init_data init; >>>> + struct davinci_psc_clk *psc; >>>> + struct clk *clk; >>>> + >>>> + psc = kzalloc(sizeof(*psc), GFP_KERNEL); >>>> + if (!psc) >>>> + return ERR_PTR(-ENOMEM); >>>> + >>>> + init.name = name; >>>> + init.ops = &davinci_psc_clk_ops; >>>> + init.parent_names = (parent_name ? &parent_name : NULL); >>>> + init.num_parents = (parent_name ? 1 : 0); >>>> + init.flags = CLK_SET_RATE_PARENT; >>> >>> Is this needed since PSC does not cause any rate change? >> >> Yes, because one of the PSCs is the ARM clock and for cpufreq, we >> need to propagate the rate change up the chain to SYSCLK6. > > Good point. But how about treating that as an exception with a new LPSC_ > quirk flag? Sure. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/17/2018 06:18 AM, Sekhar Nori wrote: > On Tuesday 16 January 2018 10:46 PM, David Lechner wrote: > >>>> +static const struct davinci_psc_clk_info da830_psc0_info[] >>>> __initconst = { >>>> + LPSC(0, 0, tpcc, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(3, 0, aemif, pll0_sysclk3, LPSC_ALWAYS_ENABLED), >>>> + LPSC(4, 0, spi0, pll0_sysclk2, 0), >>>> + LPSC(5, 0, mmcsd, pll0_sysclk2, 0), >>>> + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >>>> + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(8, 0, secu_mgr, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >>>> + LPSC(9, 0, uart0, pll0_sysclk2, 0), >>>> + LPSC(10, 0, scr0_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(11, 0, scr1_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(12, 0, scr2_ss, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>>> + LPSC(13, 0, dmax, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> >>> pruss is better (I know the name is coming from existing code). >>> >>>> + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), >>> >>> This is LPSC 15 which controls DSP too. But its missing from existing >>> code. Not sure why. Probably a note for future. For now okay with >>> ignoring it. >>> >>>> + { } >>>> +}; >>> >>> Tables like these are much easier to parse if columns are spaced using a >>> tab. >>> >> >> Tabs make the lines over 80 columns. How about spaces instead? > > That works for me. I guess checkpatch will complain, but hopefully > maintainers will agree on the exception. > >>>> +void __init da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>>> +{ >>>> + struct clk_onecell_data *clk_data; >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc0, da830_psc0_info, 16); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>>> + >>>> + clk_free_onecell_data(clk_data); >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc1, da830_psc1_info, 32); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >>>> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); >>>> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); >>> >>> This is pretty bad (and no fault of yours) - having a con_id but no >>> device name. Can you please make a pre-series which passes NULL con_id >>> in gpio-davinci.c? >> >> I'll give it a try. This is complicated by the fact that the con_id has >> made it's way into the device tree bindings. However, I think we can >> safely deprecate clock-names = "gpio" in the device tree bindings since >> we can make the driver ignore that property to preserve backwards >> compatibility. Agreed. > I don't think this breaks DT-backward compatibility. Passing a NULL > con_id in driver should find the clock for that device even if DT entry > has clock-names present. As far as I can read clk_find(). > > The less intrusive alternate is to add the GPIO device name in the table > here, while keeping the con_id and keeping the driver untouched. The > advantage of that is lesser number of dependent patches for this series > to go in. > > Later once CCF conversion has been there in the kernel for one full > release and no regressions, these other clean-ups can be done. I like this approach. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/17/2018 05:57 AM, Sekhar Nori wrote: > On Tuesday 16 January 2018 10:51 PM, David Lechner wrote: >> On 01/16/2018 08:00 AM, Sekhar Nori wrote: >>> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>>> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>>> +{ >>>> + struct clk_onecell_data *clk_data; >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >>>> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >>>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>>> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >>>> + >>>> + clk_free_onecell_data(clk_data); >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); >>> >>> Is this con_id really needed now? Searching for "usb20_psc_clk" in your >>> tree results in only this one hit. >> >> Yes, this is left over from previous attempts. >> >>> >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >>> >>> I guess multiple dev_id matches like these are another hurdle in moving >>> them to davinci_psc_clk_info[] table? If its too cumbersome to keep >>> multiple entries in the table, they can be handled as an exception at >>> the end of processing the table? Still they are not the norm so I hope >>> the normal case will still benefit. >> >> Right, as I mentioned in the reply to the previous patch, instead of >> assigning a con_id and dev_id to each clock, we would need to assign >> an array with a list of clocks. I think that would work better than >> trying to handle the extras as an exception since there, on average, >> about 5 per SoC. > > Okay, are you going to try this to see how it looks? It looks like > samsung (clk-s3c2410.c) and tegra (clk-tegra20.c) use such tables > (although both use separate tables mapping just the gate number to > con_id/dev_id). > > Others like u8540_clk.c and clk-mmp2.c have multiple calls in code to > clk_register_clkdev() like you have, but they keep them right after the > gate clock registration which makes it easy to see the mapping. > > clk-imx35.c has multiple clk_register_clkdev() calls, but uses an enum > for the gates so its easy to see the mapping. This approach looks fine > to me as well. > > So looks like there is a whole gamut of ways people have approached > this. But I do think we need to change the scheme you have currently > since it is difficult to review and audit (believe me on this one :)) > OK, I'll figure out something here. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/17/2018 09:31 AM, Sekhar Nori wrote: > On Monday 08 January 2018 07:47 AM, David Lechner wrote: >> This adds a new driver for the gate and multiplexer clocks in the >> CFGCHIPn syscon registers on TI DA8XX-type SoCs. >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> drivers/clk/davinci/Makefile | 2 + >> drivers/clk/davinci/da8xx-cfgchip.c | 203 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 205 insertions(+) >> create mode 100644 drivers/clk/davinci/da8xx-cfgchip.c >> >> diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile >> index 6c388d4..11178b7 100644 >> --- a/drivers/clk/davinci/Makefile >> +++ b/drivers/clk/davinci/Makefile >> @@ -1,6 +1,8 @@ >> # SPDX-License-Identifier: GPL-2.0 >> >> ifeq ($(CONFIG_COMMON_CLK), y) >> +obj-$(CONFIG_ARCH_DAVINCI_DA8XX) += da8xx-cfgchip.o >> + >> obj-y += pll.o >> obj-$(CONFIG_ARCH_DAVINCI_DA830) += pll-da830.o >> obj-$(CONFIG_ARCH_DAVINCI_DA850) += pll-da850.o >> diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c >> new file mode 100644 >> index 0000000..772e09a >> --- /dev/null >> +++ b/drivers/clk/davinci/da8xx-cfgchip.c >> @@ -0,0 +1,203 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Clock driver for DA8xx/AM17xx/AM18xx/OMAP-L13x CFGCHIP >> + * >> + * Copyright (C) 2017 David Lechner <david@lechnology.com> > > 2018 > >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/mfd/da8xx-cfgchip.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/of.h> >> +#include <linux/regmap.h> >> +#include <linux/slab.h> >> + >> +#ifdef CONFIG_OF > > Is this ifdef really needed, or included to save space for non-OF > builds? I think it can be removed if not really needed. > >> +static void da8xx_cfgchip_gate_clk_init(struct device_node *np, u32 reg, >> + u32 mask) >> +{ >> + struct da8xx_cfgchip_gate_clk *clk; >> + struct clk_init_data init; >> + const char *name = np->name; >> + const char *parent_name; >> + struct regmap *regmap; >> + int ret; >> + >> + of_property_read_string(np, "clock-output-names", &name); >> + parent_name = of_clk_get_parent_name(np, 0); >> + >> + regmap = syscon_node_to_regmap(of_get_parent(np)); >> + if (IS_ERR(regmap)) { >> + pr_err("%s: no regmap for syscon parent of %s (%ld)\n", >> + __func__, np->full_name, PTR_ERR(regmap)); > > please use pr_fmt for this driver too. > >> +static void da8xx_cfgchip_mux_clk_init(struct device_node *np, u32 reg, >> + u32 mask) >> +{ >> + struct da8xx_cfgchip_mux_clk *clk; >> + struct clk_init_data init; >> + const char *name = np->name; >> + const char *parent_names[2]; >> + struct regmap *regmap; >> + int ret; >> + >> + ret = of_property_match_string(np, "clock-names", "pll0_sysclk2"); >> + parent_names[0] = of_clk_get_parent_name(np, ret); >> + if (!parent_names[0]) { >> + pr_err("%s: missing pll0_sysclk2 clock\n", __func__); >> + return; >> + } >> + >> + ret = of_property_match_string(np, "clock-names", "pll1_sysclk2"); >> + parent_names[1] = of_clk_get_parent_name(np, ret); >> + if (!parent_names[1]) { >> + pr_err("%s: missing pll1_sysclk2 clock\n", __func__); >> + return; >> + } > > The fact that you are looking specifically for pll0_sysclk2 and > pll1_sysclk2 makes it really specific to async3 and the same function > cannot be used for something like EMIFA clock source. Can this part of > the function be factored out so rest of the function can still be reused > for another clock? > Already fixed this stuff a couple days ago. ;-) And I went ahead and added the EMIFA clocks (the mux and the DIV4.5) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/17/2018 05:57 AM, Sekhar Nori wrote: > On Tuesday 16 January 2018 10:51 PM, David Lechner wrote: >> On 01/16/2018 08:00 AM, Sekhar Nori wrote: >>> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>>> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>>> +{ >>>> + struct clk_onecell_data *clk_data; >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >>>> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >>>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>>> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >>>> + >>>> + clk_free_onecell_data(clk_data); >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); >>> >>> Is this con_id really needed now? Searching for "usb20_psc_clk" in your >>> tree results in only this one hit. >> >> Yes, this is left over from previous attempts. >> >>> >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >>> >>> I guess multiple dev_id matches like these are another hurdle in moving >>> them to davinci_psc_clk_info[] table? If its too cumbersome to keep >>> multiple entries in the table, they can be handled as an exception at >>> the end of processing the table? Still they are not the norm so I hope >>> the normal case will still benefit. >> >> Right, as I mentioned in the reply to the previous patch, instead of >> assigning a con_id and dev_id to each clock, we would need to assign >> an array with a list of clocks. I think that would work better than >> trying to handle the extras as an exception since there, on average, >> about 5 per SoC. > > Okay, are you going to try this to see how it looks? It is looking like this: static const struct davinci_psc_clkdev_info emfia_clkdev[] __initconst = { LPSC_CLKDEV(NULL, "ti-aemif"), LPSC_CLKDEV("aemif", "davinci-nand.0"), { } }; static const struct davinci_psc_clkdev_info spi0_clkdev[] __initconst = { LPSC_CLKDEV(NULL, "spi_davinci.0"), { } }; static const struct davinci_psc_clkdev_info mmcsd0_clkdev[] __initconst = { LPSC_CLKDEV(NULL, "da830-mmc.0"), { } }; static const struct davinci_psc_clkdev_info uart0_clkdev[] __initconst = { LPSC_CLKDEV(NULL, "serial8250.0"), { } }; static const struct davinci_psc_clkdev_info arm_clkdev[] __initconst = { /* * REVISIT: cpufreq-davinci should be modified to use dev_id and drop * use of con_id. */ LPSC_CLKDEV("arm", NULL), { } }; static const struct davinci_psc_clkdev_info dsp_clkdev[] __initconst = { LPSC_CLKDEV(NULL, "davinci-rproc.0"), { } }; static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { LPSC(0, 0, tpcc0, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), LPSC(1, 0, tptc0, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), LPSC(2, 0, tptc1, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), LPSC(3, 0, emifa, async1, emfia_clkdev, 0), LPSC(4, 0, spi0, pll0_sysclk2, spi0_clkdev, 0), LPSC(5, 0, mmcsd0, pll0_sysclk2, mmcsd0_clkdev, 0), LPSC(6, 0, aintc, pll0_sysclk4, NULL, LPSC_ALWAYS_ENABLED), LPSC(7, 0, arm_rom, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), LPSC(9, 0, uart0, pll0_sysclk2, uart0_clkdev, 0), LPSC(13, 0, pruss, pll0_sysclk2, NULL, 0), LPSC(14, 0, arm, pll0_sysclk6, arm_clkdev, LPSC_ALWAYS_ENABLED | LPSC_ARM_RATE), LPSC(15, 1, dsp, pll0_sysclk1, dsp_clkdev, LPSC_FORCE | LPSC_LOCAL_RESET), { } }; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 18 January 2018 12:38 AM, David Lechner wrote: > It is looking like this: > > > static const struct davinci_psc_clkdev_info emfia_clkdev[] __initconst = { > LPSC_CLKDEV(NULL, "ti-aemif"), > LPSC_CLKDEV("aemif", "davinci-nand.0"), > { } > }; > > static const struct davinci_psc_clkdev_info spi0_clkdev[] __initconst = { > LPSC_CLKDEV(NULL, "spi_davinci.0"), > { } > }; > > static const struct davinci_psc_clkdev_info mmcsd0_clkdev[] __initconst = { > LPSC_CLKDEV(NULL, "da830-mmc.0"), > { } > }; > > static const struct davinci_psc_clkdev_info uart0_clkdev[] __initconst = { > LPSC_CLKDEV(NULL, "serial8250.0"), > { } > }; > > static const struct davinci_psc_clkdev_info arm_clkdev[] __initconst = { > /* > * REVISIT: cpufreq-davinci should be modified to use dev_id and drop > * use of con_id. > */ > LPSC_CLKDEV("arm", NULL), > { } > }; > > static const struct davinci_psc_clkdev_info dsp_clkdev[] __initconst = { > LPSC_CLKDEV(NULL, "davinci-rproc.0"), > { } > }; > > static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { > LPSC(0, 0, tpcc0, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), > LPSC(1, 0, tptc0, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), > LPSC(2, 0, tptc1, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), > LPSC(3, 0, emifa, async1, emfia_clkdev, 0), > LPSC(4, 0, spi0, pll0_sysclk2, spi0_clkdev, 0), > LPSC(5, 0, mmcsd0, pll0_sysclk2, mmcsd0_clkdev, 0), > LPSC(6, 0, aintc, pll0_sysclk4, NULL, LPSC_ALWAYS_ENABLED), > LPSC(7, 0, arm_rom, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), > LPSC(9, 0, uart0, pll0_sysclk2, uart0_clkdev, 0), > LPSC(13, 0, pruss, pll0_sysclk2, NULL, 0), > LPSC(14, 0, arm, pll0_sysclk6, arm_clkdev, LPSC_ALWAYS_ENABLED | > LPSC_ARM_RATE), > LPSC(15, 1, dsp, pll0_sysclk1, dsp_clkdev, LPSC_FORCE | > LPSC_LOCAL_RESET), > { } > }; This looks good to me! Regards, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 17 January 2018 11:02 PM, David Lechner wrote: >>>>> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); >>>> >>>> This is pretty bad (and no fault of yours) - having a con_id but no >>>> device name. Can you please make a pre-series which passes NULL con_id >>>> in gpio-davinci.c? >>> >>> I'll give it a try. This is complicated by the fact that the con_id has >>> made it's way into the device tree bindings. However, I think we can >>> safely deprecate clock-names = "gpio" in the device tree bindings since >>> we can make the driver ignore that property to preserve backwards >>> compatibility. > > Agreed. > >> I don't think this breaks DT-backward compatibility. Passing a NULL >> con_id in driver should find the clock for that device even if DT entry >> has clock-names present. As far as I can read clk_find(). >> >> The less intrusive alternate is to add the GPIO device name in the table >> here, while keeping the con_id and keeping the driver untouched. The >> advantage of that is lesser number of dependent patches for this series >> to go in. >> >> Later once CCF conversion has been there in the kernel for one full >> release and no regressions, these other clean-ups can be done. > > I like this approach. One downside is that we will have to have clock-names = "gpio" in da850 device-tree too. Since its already present in keystone already, I don't think adding one more is such a big issue. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > +static int da8xx_usb1_phy_clk_set_parent(struct clk_hw *hw, u8 index) > +{ > + struct da8xx_usb1_phy_clk *clk = to_da8xx_usb1_phy_clk(hw); > + unsigned int mask, val; > + > + /* Set the USB 1.1 PHY clock mux based on the parent clock. */ > + mask = CFGCHIP2_USB1PHYCLKMUX; > + switch (index) { > + case DA8XX_USB1_PHY_CLK_PARENT_USB_REFCLKIN: > + val = CFGCHIP2_USB1PHYCLKMUX; > + break; > + case DA8XX_USB1_PHY_CLK_PARENT_USB0_PHY_PLL: > + val = 0; > + break; > + default: > + return -EINVAL; > + } > + > + regmap_write_bits(clk->regmap, CFGCHIP(2), mask, val); This function can be simplified quite a bit if you use a shift. #define CFGCHIP2_USB1PHYCLKMUX_SHIFT 12 static int da8xx_usb1_phy_clk_set_parent(struct clk_hw *hw, u8 index) { struct da8xx_usb1_phy_clk *clk = to_da8xx_usb1_phy_clk(hw); regmap_write_bits(clk->regmap, CFGCHIP(2), CFGCHIP2_USB1PHYCLKMUX, index << CFGCHIP2_USB1PHYCLKMUX_SHIFT); } Same thing for da8xx_usb0_phy_clk_set_parent() as well. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > +int __init da8xx_register_usb20_phy_clk(bool use_usb_refclkin) > +{ > + struct regmap *cfgchip; > + struct clk *usb0_psc_clk, *clk; > + struct clk_hw *parent; > + > + cfgchip = syscon_regmap_lookup_by_compatible("ti,da830-cfgchip"); Am I right in understanding that this API is only called for non-DT boot? If yes, do we really need the lookup by compatible? Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > +#ifndef CONFIG_COMMON_CLK > static int da850_set_armrate(struct clk *clk, unsigned long rate); > static int da850_round_armrate(struct clk *clk, unsigned long rate); > static int da850_set_pll0rate(struct clk *clk, unsigned long armrate); > @@ -583,6 +588,7 @@ static struct clk_lookup da850_clks[] = { > CLK("ecap.2", "fck", &ecap2_clk), > CLK(NULL, NULL, NULL), > }; > +#endif Don't like these temporary ifdefs (which I am sure is the case with you too). But don't have any other good idea for splitting these patches into review-able and build-able pieces. So lets go with this for now. > void __init da850_init_time(void) > { > +#ifdef CONFIG_COMMON_CLK > + void __iomem *pll0, *pll1, *psc0, *psc1; > + struct clk *clk; > + struct clk_hw *parent; > + > + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K); > + pll1 = ioremap(DA850_PLL1_BASE, SZ_4K); > + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K); > + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K); > + > + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA850_REF_FREQ); Overall, this and other functions like this in this series need some more line spacing. Please add a space here.. > + da850_pll_clk_init(pll0, pll1); .. and here. > + clk = clk_register_mux(NULL, "async3", > + (const char * const[]){ "pll0_sysclk2", "pll1_sysclk2" }, > + 2, 0, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG), > + ilog2(CFGCHIP3_ASYNC3_CLKSRC), 1, 0, NULL); .. here before the comment .. > + /* pll1_sysclk2 is not affected by CPU scaling, so use it for async3 */ > + parent = clk_hw_get_parent_by_index(__clk_get_hw(clk), 1); > + if (parent) > + clk_set_parent(clk, parent->clk); > + else > + pr_warn("%s: Failed to find async3 parent clock\n", __func__); .. and here. And so on. I have not taken a closer look at mach patches. But may be you should send the next version anyway and make sure everyone is happy with the driver first. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/18/2018 09:14 AM, Sekhar Nori wrote: > On Monday 08 January 2018 07:47 AM, David Lechner wrote: >> +int __init da8xx_register_usb20_phy_clk(bool use_usb_refclkin) >> +{ >> + struct regmap *cfgchip; >> + struct clk *usb0_psc_clk, *clk; >> + struct clk_hw *parent; >> + >> + cfgchip = syscon_regmap_lookup_by_compatible("ti,da830-cfgchip"); > > Am I right in understanding that this API is only called for non-DT > boot? If yes, do we really need the lookup by compatible? This code is used in DT boot until [PATCH v5 43/44] "ARM: da8xx-dt: switch to device tree clocks". So, yes it is needed temporarily to prevent breaking USB. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/18/2018 07:05 AM, Sekhar Nori wrote: > On Monday 08 January 2018 07:47 AM, David Lechner wrote: > >> +static int da8xx_usb1_phy_clk_set_parent(struct clk_hw *hw, u8 index) >> +{ >> + struct da8xx_usb1_phy_clk *clk = to_da8xx_usb1_phy_clk(hw); >> + unsigned int mask, val; >> + >> + /* Set the USB 1.1 PHY clock mux based on the parent clock. */ >> + mask = CFGCHIP2_USB1PHYCLKMUX; >> + switch (index) { >> + case DA8XX_USB1_PHY_CLK_PARENT_USB_REFCLKIN: >> + val = CFGCHIP2_USB1PHYCLKMUX; >> + break; >> + case DA8XX_USB1_PHY_CLK_PARENT_USB0_PHY_PLL: >> + val = 0; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + regmap_write_bits(clk->regmap, CFGCHIP(2), mask, val); > > This function can be simplified quite a bit if you use a shift. > > #define CFGCHIP2_USB1PHYCLKMUX_SHIFT 12 > > static int da8xx_usb1_phy_clk_set_parent(struct clk_hw *hw, u8 index) > { > struct da8xx_usb1_phy_clk *clk = to_da8xx_usb1_phy_clk(hw); > > regmap_write_bits(clk->regmap, CFGCHIP(2), > CFGCHIP2_USB1PHYCLKMUX, > index << CFGCHIP2_USB1PHYCLKMUX_SHIFT); > } > > Same thing for da8xx_usb0_phy_clk_set_parent() as well. > or to avoid defining a new macro? > regmap_write_bits(clk->regmap, CFGCHIP(2), > CFGCHIP2_USB1PHYCLKMUX, > index ? CFGCHIP2_USB1PHYCLKMUX : 0); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 19 January 2018 12:19 AM, David Lechner wrote: >> > > or to avoid defining a new macro? > > >> regmap_write_bits(clk->regmap, CFGCHIP(2), >> CFGCHIP2_USB1PHYCLKMUX, >> index ? CFGCHIP2_USB1PHYCLKMUX : 0); Looks good as well! Regards, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 19 January 2018 12:13 AM, David Lechner wrote: > On 01/18/2018 09:14 AM, Sekhar Nori wrote: >> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>> +int __init da8xx_register_usb20_phy_clk(bool use_usb_refclkin) >>> +{ >>> + struct regmap *cfgchip; >>> + struct clk *usb0_psc_clk, *clk; >>> + struct clk_hw *parent; >>> + >>> + cfgchip = syscon_regmap_lookup_by_compatible("ti,da830-cfgchip"); >> >> Am I right in understanding that this API is only called for non-DT >> boot? If yes, do we really need the lookup by compatible? > > This code is used in DT boot until [PATCH v5 43/44] "ARM: da8xx-dt: > switch to device tree clocks". So, yes it is needed temporarily to > prevent breaking USB. Alright, so this line should probably be dropped either as part of 43/44 or later. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: > This adds platform-specific declarations for the PSC clocks on TI DA850/ > OMAP-L138/AM18XX SoCs. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > drivers/clk/davinci/Makefile | 1 + > drivers/clk/davinci/psc-da850.c | 117 ++++++++++++++++++++++++++++++++++++++++ > include/linux/clk/davinci.h | 1 + > 3 files changed, 119 insertions(+) > create mode 100644 drivers/clk/davinci/psc-da850.c > > diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile > index fb14c8c..aef0390 100644 > --- a/drivers/clk/davinci/Makefile > +++ b/drivers/clk/davinci/Makefile > @@ -11,4 +11,5 @@ obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o > > obj-y += psc.o > obj-$(CONFIG_ARCH_DAVINCI_DA830) += psc-da830.o > +obj-$(CONFIG_ARCH_DAVINCI_DA850) += psc-da850.o > endif > diff --git a/drivers/clk/davinci/psc-da850.c b/drivers/clk/davinci/psc-da850.c > new file mode 100644 > index 0000000..3b4583d > --- /dev/null > +++ b/drivers/clk/davinci/psc-da850.c > @@ -0,0 +1,117 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PSC clock descriptions for TI DA850/OMAP-L138/AM18XX > + * > + * Copyright (C) 2017 David Lechner <david@lechnology.com> > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> > +#include <linux/init.h> > +#include <linux/of.h> > +#include <linux/types.h> > + > +#include "psc.h" > + > +static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { > + LPSC(0, 0, tpcc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(3, 0, aemif, pll0_sysclk3, 0), > + LPSC(4, 0, spi0, pll0_sysclk2, 0), > + LPSC(5, 0, mmcsd0, pll0_sysclk2, 0), > + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), > + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(9, 0, uart0, pll0_sysclk2, 0), > + LPSC(13, 0, pruss, pll0_sysclk2, 0), > + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), > + LPSC(15, 1, dsp, pll0_sysclk1, LPSC_FORCE | LPSC_LOCAL_RESET), > + { } > +}; > + > +static const struct davinci_psc_clk_info da850_psc1_info[] __initconst = { > + LPSC(0, 0, tpcc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(1, 0, usb0, pll0_sysclk2, 0), > + LPSC(2, 0, usb1, pll0_sysclk4, 0), > + LPSC(3, 0, gpio, pll0_sysclk4, 0), > + LPSC(5, 0, emac, pll0_sysclk4, 0), > + LPSC(6, 0, emif3, pll0_sysclk5, LPSC_ALWAYS_ENABLED), > + LPSC(7, 0, mcasp0, async3, 0), > + LPSC(8, 0, sata, pll0_sysclk2, LPSC_FORCE), > + LPSC(9, 0, vpif, pll0_sysclk2, 0), > + LPSC(10, 0, spi1, async3, 0), > + LPSC(11, 0, i2c1, pll0_sysclk4, 0), > + LPSC(12, 0, uart1, async3, 0), > + LPSC(13, 0, uart2, async3, 0), > + LPSC(14, 0, mcbsp0, async3, 0), > + LPSC(15, 0, mcbsp1, async3, 0), > + LPSC(16, 0, lcdc, pll0_sysclk2, 0), > + LPSC(17, 0, ehrpwm, async3, 0), > + LPSC(18, 0, mmcsd1, pll0_sysclk2, 0), > + LPSC(20, 0, ecap, async3, 0), > + LPSC(21, 0, tptc2, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + { } > +}; > + > +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) > +{ > + struct clk_onecell_data *clk_data; > + > + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); > + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); > + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); > + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); > + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); > + clk_register_clkdev(clk_data->clks[14], "arm", NULL); > + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); > + > + clk_free_onecell_data(clk_data); > + > + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); > + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); > + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); > + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); > + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); > + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); > + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); > + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); > + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); > + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); > + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); > + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); > + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); > + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); > + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); > + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); > + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); > + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); > + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); > + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); > + > + clk_free_onecell_data(clk_data); > +} > + > +#ifdef CONFIG_OF > +static void __init of_da850_psc0_clk_init(struct device_node *node) > +{ > + of_davinci_psc_clk_init(node, da850_psc0_info, 16); > +} > +CLK_OF_DECLARE(da850_psc0_clk, "ti,da850-psc0", of_da850_psc0_clk_init); > + > +static void __init of_da850_psc1_clk_init(struct device_node *node) > +{ > + of_davinci_psc_clk_init(node, da850_psc1_info, 32); > +} > +CLK_OF_DECLARE(da850_psc1_clk, "ti,da850-psc1", of_da850_psc1_clk_init); > +#endif > diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h > index 3ec8100..3d8bdfa 100644 > --- a/include/linux/clk/davinci.h > +++ b/include/linux/clk/davinci.h > @@ -17,5 +17,6 @@ void dm644x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); > void dm646x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); > > void da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1); > +void da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1); > > #endif /* __LINUX_CLK_DAVINCI_H__ */ > -- > 2.7.4 > Hi David, I've been working on moving the genpd code from its own driver to the psc one. I couldn't get the system to boot though and problems happened very early in the boot sequence. I struggled to figure out what's happening, but eventually I noticed that psc uses CLK_OF_DECLARE() to initialize clocks. The functions registered this way are called very early in the boot sequence, way before late_initcall() in which the genpd framework is initialized. This of course makes it impossible to register genpd at the same time we register PSCs. Mike, Stephen - it would be great if you could confirm, but I believe the clock framework now only accepts clock drivers which create real platform drivers, in which case this series would need to be modified. David: FYI I quickly converted the functions in psc-da850.c to actual probe callbacks and it worked just fine on a da850-lcdk board. Best regards, Bartosz Golaszewski -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bartosz, all, On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >> This adds platform-specific declarations for the PSC clocks on TI DA850/ >> OMAP-L138/AM18XX SoCs. >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> drivers/clk/davinci/Makefile | 1 + >> drivers/clk/davinci/psc-da850.c | 117 ++++++++++++++++++++++++++++++++++++++++ >> include/linux/clk/davinci.h | 1 + >> 3 files changed, 119 insertions(+) >> create mode 100644 drivers/clk/davinci/psc-da850.c >> >> diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile >> index fb14c8c..aef0390 100644 >> --- a/drivers/clk/davinci/Makefile >> +++ b/drivers/clk/davinci/Makefile >> @@ -11,4 +11,5 @@ obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o >> >> obj-y += psc.o >> obj-$(CONFIG_ARCH_DAVINCI_DA830) += psc-da830.o >> +obj-$(CONFIG_ARCH_DAVINCI_DA850) += psc-da850.o >> endif >> diff --git a/drivers/clk/davinci/psc-da850.c b/drivers/clk/davinci/psc-da850.c >> new file mode 100644 >> index 0000000..3b4583d >> --- /dev/null >> +++ b/drivers/clk/davinci/psc-da850.c >> @@ -0,0 +1,117 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * PSC clock descriptions for TI DA850/OMAP-L138/AM18XX >> + * >> + * Copyright (C) 2017 David Lechner <david@lechnology.com> >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/clkdev.h> >> +#include <linux/init.h> >> +#include <linux/of.h> >> +#include <linux/types.h> >> + >> +#include "psc.h" >> + >> +static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { >> + LPSC(0, 0, tpcc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(3, 0, aemif, pll0_sysclk3, 0), >> + LPSC(4, 0, spi0, pll0_sysclk2, 0), >> + LPSC(5, 0, mmcsd0, pll0_sysclk2, 0), >> + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >> + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(9, 0, uart0, pll0_sysclk2, 0), >> + LPSC(13, 0, pruss, pll0_sysclk2, 0), >> + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), >> + LPSC(15, 1, dsp, pll0_sysclk1, LPSC_FORCE | LPSC_LOCAL_RESET), >> + { } >> +}; >> + >> +static const struct davinci_psc_clk_info da850_psc1_info[] __initconst = { >> + LPSC(0, 0, tpcc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(1, 0, usb0, pll0_sysclk2, 0), >> + LPSC(2, 0, usb1, pll0_sysclk4, 0), >> + LPSC(3, 0, gpio, pll0_sysclk4, 0), >> + LPSC(5, 0, emac, pll0_sysclk4, 0), >> + LPSC(6, 0, emif3, pll0_sysclk5, LPSC_ALWAYS_ENABLED), >> + LPSC(7, 0, mcasp0, async3, 0), >> + LPSC(8, 0, sata, pll0_sysclk2, LPSC_FORCE), >> + LPSC(9, 0, vpif, pll0_sysclk2, 0), >> + LPSC(10, 0, spi1, async3, 0), >> + LPSC(11, 0, i2c1, pll0_sysclk4, 0), >> + LPSC(12, 0, uart1, async3, 0), >> + LPSC(13, 0, uart2, async3, 0), >> + LPSC(14, 0, mcbsp0, async3, 0), >> + LPSC(15, 0, mcbsp1, async3, 0), >> + LPSC(16, 0, lcdc, pll0_sysclk2, 0), >> + LPSC(17, 0, ehrpwm, async3, 0), >> + LPSC(18, 0, mmcsd1, pll0_sysclk2, 0), >> + LPSC(20, 0, ecap, async3, 0), >> + LPSC(21, 0, tptc2, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + { } >> +}; >> + >> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >> +{ >> + struct clk_onecell_data *clk_data; >> + >> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >> + if (!clk_data) >> + return; >> + >> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >> + >> + clk_free_onecell_data(clk_data); >> + >> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >> + if (!clk_data) >> + return; >> + >> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); >> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); >> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); >> + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); >> + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); >> + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); >> + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); >> + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); >> + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); >> + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); >> + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); >> + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); >> + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); >> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); >> + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); >> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); >> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); >> + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); >> + >> + clk_free_onecell_data(clk_data); >> +} >> + >> +#ifdef CONFIG_OF >> +static void __init of_da850_psc0_clk_init(struct device_node *node) >> +{ >> + of_davinci_psc_clk_init(node, da850_psc0_info, 16); >> +} >> +CLK_OF_DECLARE(da850_psc0_clk, "ti,da850-psc0", of_da850_psc0_clk_init); >> + >> +static void __init of_da850_psc1_clk_init(struct device_node *node) >> +{ >> + of_davinci_psc_clk_init(node, da850_psc1_info, 32); >> +} >> +CLK_OF_DECLARE(da850_psc1_clk, "ti,da850-psc1", of_da850_psc1_clk_init); >> +#endif >> diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h >> index 3ec8100..3d8bdfa 100644 >> --- a/include/linux/clk/davinci.h >> +++ b/include/linux/clk/davinci.h >> @@ -17,5 +17,6 @@ void dm644x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); >> void dm646x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); >> >> void da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1); >> +void da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1); >> >> #endif /* __LINUX_CLK_DAVINCI_H__ */ >> -- >> 2.7.4 >> > > Hi David, > > I've been working on moving the genpd code from its own driver to the > psc one. I couldn't get the system to boot though and problems > happened very early in the boot sequence. I struggled to figure out > what's happening, but eventually I noticed that psc uses > CLK_OF_DECLARE() to initialize clocks. The functions registered this > way are called very early in the boot sequence, way before > late_initcall() in which the genpd framework is initialized. This of > course makes it impossible to register genpd at the same time we > register PSCs. > > Mike, Stephen - it would be great if you could confirm, but I believe > the clock framework now only accepts clock drivers which create real > platform drivers, in which case this series would need to be modified. You are correct. All new uses of CLK_OF_DECLARE are met with high suspicion, and all new drivers should be platform drivers. There are cases when CLK_OF_DECLARE is necessary (sometimes temporarily while things are reworked), but it seems this is not the case for this driver based on your testing of the platform driver approach. Please use the platform driver approach instead. Thanks, Mike > > David: FYI I quickly converted the functions in psc-da850.c to actual > probe callbacks and it worked just fine on a da850-lcdk board. > > Best regards, > Bartosz Golaszewski -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/09/2018 10:48 AM, Michael Turquette wrote: > Hi Bartosz, all, > > On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >>> This adds platform-specific declarations for the PSC clocks on TI DA850/ >>> OMAP-L138/AM18XX SoCs. >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >>> --- >>> drivers/clk/davinci/Makefile | 1 + >>> drivers/clk/davinci/psc-da850.c | 117 ++++++++++++++++++++++++++++++++++++++++ >>> include/linux/clk/davinci.h | 1 + >>> 3 files changed, 119 insertions(+) >>> create mode 100644 drivers/clk/davinci/psc-da850.c >>> >>> diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile >>> index fb14c8c..aef0390 100644 >>> --- a/drivers/clk/davinci/Makefile >>> +++ b/drivers/clk/davinci/Makefile >>> @@ -11,4 +11,5 @@ obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o >>> >>> obj-y += psc.o >>> obj-$(CONFIG_ARCH_DAVINCI_DA830) += psc-da830.o >>> +obj-$(CONFIG_ARCH_DAVINCI_DA850) += psc-da850.o >>> endif >>> diff --git a/drivers/clk/davinci/psc-da850.c b/drivers/clk/davinci/psc-da850.c >>> new file mode 100644 >>> index 0000000..3b4583d >>> --- /dev/null >>> +++ b/drivers/clk/davinci/psc-da850.c >>> @@ -0,0 +1,117 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * PSC clock descriptions for TI DA850/OMAP-L138/AM18XX >>> + * >>> + * Copyright (C) 2017 David Lechner <david@lechnology.com> >>> + */ >>> + >>> +#include <linux/clk-provider.h> >>> +#include <linux/clkdev.h> >>> +#include <linux/init.h> >>> +#include <linux/of.h> >>> +#include <linux/types.h> >>> + >>> +#include "psc.h" >>> + >>> +static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { >>> + LPSC(0, 0, tpcc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(3, 0, aemif, pll0_sysclk3, 0), >>> + LPSC(4, 0, spi0, pll0_sysclk2, 0), >>> + LPSC(5, 0, mmcsd0, pll0_sysclk2, 0), >>> + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >>> + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(9, 0, uart0, pll0_sysclk2, 0), >>> + LPSC(13, 0, pruss, pll0_sysclk2, 0), >>> + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), >>> + LPSC(15, 1, dsp, pll0_sysclk1, LPSC_FORCE | LPSC_LOCAL_RESET), >>> + { } >>> +}; >>> + >>> +static const struct davinci_psc_clk_info da850_psc1_info[] __initconst = { >>> + LPSC(0, 0, tpcc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(1, 0, usb0, pll0_sysclk2, 0), >>> + LPSC(2, 0, usb1, pll0_sysclk4, 0), >>> + LPSC(3, 0, gpio, pll0_sysclk4, 0), >>> + LPSC(5, 0, emac, pll0_sysclk4, 0), >>> + LPSC(6, 0, emif3, pll0_sysclk5, LPSC_ALWAYS_ENABLED), >>> + LPSC(7, 0, mcasp0, async3, 0), >>> + LPSC(8, 0, sata, pll0_sysclk2, LPSC_FORCE), >>> + LPSC(9, 0, vpif, pll0_sysclk2, 0), >>> + LPSC(10, 0, spi1, async3, 0), >>> + LPSC(11, 0, i2c1, pll0_sysclk4, 0), >>> + LPSC(12, 0, uart1, async3, 0), >>> + LPSC(13, 0, uart2, async3, 0), >>> + LPSC(14, 0, mcbsp0, async3, 0), >>> + LPSC(15, 0, mcbsp1, async3, 0), >>> + LPSC(16, 0, lcdc, pll0_sysclk2, 0), >>> + LPSC(17, 0, ehrpwm, async3, 0), >>> + LPSC(18, 0, mmcsd1, pll0_sysclk2, 0), >>> + LPSC(20, 0, ecap, async3, 0), >>> + LPSC(21, 0, tptc2, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + { } >>> +}; >>> + >>> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>> +{ >>> + struct clk_onecell_data *clk_data; >>> + >>> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >>> + if (!clk_data) >>> + return; >>> + >>> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >>> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >>> + >>> + clk_free_onecell_data(clk_data); >>> + >>> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >>> + if (!clk_data) >>> + return; >>> + >>> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); >>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >>> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); >>> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); >>> + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); >>> + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); >>> + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); >>> + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); >>> + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); >>> + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); >>> + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); >>> + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); >>> + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); >>> + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); >>> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); >>> + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); >>> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); >>> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); >>> + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); >>> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); >>> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); >>> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); >>> + >>> + clk_free_onecell_data(clk_data); >>> +} >>> + >>> +#ifdef CONFIG_OF >>> +static void __init of_da850_psc0_clk_init(struct device_node *node) >>> +{ >>> + of_davinci_psc_clk_init(node, da850_psc0_info, 16); >>> +} >>> +CLK_OF_DECLARE(da850_psc0_clk, "ti,da850-psc0", of_da850_psc0_clk_init); >>> + >>> +static void __init of_da850_psc1_clk_init(struct device_node *node) >>> +{ >>> + of_davinci_psc_clk_init(node, da850_psc1_info, 32); >>> +} >>> +CLK_OF_DECLARE(da850_psc1_clk, "ti,da850-psc1", of_da850_psc1_clk_init); >>> +#endif >>> diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h >>> index 3ec8100..3d8bdfa 100644 >>> --- a/include/linux/clk/davinci.h >>> +++ b/include/linux/clk/davinci.h >>> @@ -17,5 +17,6 @@ void dm644x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); >>> void dm646x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); >>> >>> void da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1); >>> +void da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1); >>> >>> #endif /* __LINUX_CLK_DAVINCI_H__ */ >>> -- >>> 2.7.4 >>> >> >> Hi David, >> >> I've been working on moving the genpd code from its own driver to the >> psc one. I couldn't get the system to boot though and problems >> happened very early in the boot sequence. I struggled to figure out >> what's happening, but eventually I noticed that psc uses >> CLK_OF_DECLARE() to initialize clocks. The functions registered this >> way are called very early in the boot sequence, way before >> late_initcall() in which the genpd framework is initialized. This of >> course makes it impossible to register genpd at the same time we >> register PSCs. >> >> Mike, Stephen - it would be great if you could confirm, but I believe >> the clock framework now only accepts clock drivers which create real >> platform drivers, in which case this series would need to be modified. > > You are correct. All new uses of CLK_OF_DECLARE are met with high > suspicion, and all new drivers should be platform drivers. Oh boy. I'll try to get a v7 out this week with these changes. > > There are cases when CLK_OF_DECLARE is necessary (sometimes > temporarily while things are reworked), but it seems this is not the > case for this driver based on your testing of the platform driver > approach. Please use the platform driver approach instead. > > Thanks, > Mike > >> >> David: FYI I quickly converted the functions in psc-da850.c to actual >> probe callbacks and it worked just fine on a da850-lcdk board. >> >> Best regards, >> Bartosz Golaszewski -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bartosz, On Friday 09 February 2018 10:18 PM, Michael Turquette wrote: > On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >> Hi David, >> >> I've been working on moving the genpd code from its own driver to the >> psc one. I couldn't get the system to boot though and problems >> happened very early in the boot sequence. I struggled to figure out >> what's happening, but eventually I noticed that psc uses >> CLK_OF_DECLARE() to initialize clocks. The functions registered this >> way are called very early in the boot sequence, way before >> late_initcall() in which the genpd framework is initialized. This of late_initcall() is too late for genpd to be initialized. As you may have seen with the latest set of patches, we have problems with timer initialization. After converting to platform devices, PSC and PLL clocks get initialized post time_init(). We are working that around using fixed-clocks, which hopefully will work (I still need to test many of the affected platforms). Can you please reply with the exact issue you faced with genpd framework initialization so we do have that on record. Thanks, Sekhar >> course makes it impossible to register genpd at the same time we >> register PSCs. >> >> Mike, Stephen - it would be great if you could confirm, but I believe >> the clock framework now only accepts clock drivers which create real >> platform drivers, in which case this series would need to be modified. > > You are correct. All new uses of CLK_OF_DECLARE are met with high > suspicion, and all new drivers should be platform drivers. > > There are cases when CLK_OF_DECLARE is necessary (sometimes > temporarily while things are reworked), but it seems this is not the > case for this driver based on your testing of the platform driver > approach. Please use the platform driver approach instead. > > Thanks, > Mike > >> >> David: FYI I quickly converted the functions in psc-da850.c to actual >> probe callbacks and it worked just fine on a da850-lcdk board. >> >> Best regards, >> Bartosz Golaszewski -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-04-05 15:09 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: > Hi Bartosz, > > On Friday 09 February 2018 10:18 PM, Michael Turquette wrote: >> On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: > >>> Hi David, >>> >>> I've been working on moving the genpd code from its own driver to the >>> psc one. I couldn't get the system to boot though and problems >>> happened very early in the boot sequence. I struggled to figure out >>> what's happening, but eventually I noticed that psc uses >>> CLK_OF_DECLARE() to initialize clocks. The functions registered this >>> way are called very early in the boot sequence, way before >>> late_initcall() in which the genpd framework is initialized. This of > > late_initcall() is too late for genpd to be initialized. As you may have > seen with the latest set of patches, we have problems with timer > initialization. After converting to platform devices, PSC and PLL clocks > get initialized post time_init(). We are working that around using > fixed-clocks, which hopefully will work (I still need to test many of > the affected platforms). > > Can you please reply with the exact issue you faced with genpd framework > initialization so we do have that on record. > The exact issue manifested itself in a NULL-pointer dereference panic when I tried moving the genpd code I had initially implemented as a separate platform driver to what I believe was v6 or v7 of David's series (before the psc driver became a platform driver, when it was still using CLK_OF_DECLARE()). When I had tested a simple conversion of that version to a platform_driver, genpd worked fine. I don't have the stack traces from these panics, but I recall some debugfs functions being involved and the genpd late_initcalls are related to debugfs. Looking at it now I don't see how exactly it could fail though. Best regards, Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 05 April 2018 07:14 PM, Bartosz Golaszewski wrote: > 2018-04-05 15:09 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: >> Hi Bartosz, >> >> On Friday 09 February 2018 10:18 PM, Michael Turquette wrote: >>> On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>>> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >> >>>> Hi David, >>>> >>>> I've been working on moving the genpd code from its own driver to the >>>> psc one. I couldn't get the system to boot though and problems >>>> happened very early in the boot sequence. I struggled to figure out >>>> what's happening, but eventually I noticed that psc uses >>>> CLK_OF_DECLARE() to initialize clocks. The functions registered this >>>> way are called very early in the boot sequence, way before >>>> late_initcall() in which the genpd framework is initialized. This of >> >> late_initcall() is too late for genpd to be initialized. As you may have >> seen with the latest set of patches, we have problems with timer >> initialization. After converting to platform devices, PSC and PLL clocks >> get initialized post time_init(). We are working that around using >> fixed-clocks, which hopefully will work (I still need to test many of >> the affected platforms). >> >> Can you please reply with the exact issue you faced with genpd framework >> initialization so we do have that on record. >> > > The exact issue manifested itself in a NULL-pointer dereference panic > when I tried moving the genpd code I had initially implemented as a > separate platform driver to what I believe was v6 or v7 of David's > series (before the psc driver became a platform driver, when it was > still using CLK_OF_DECLARE()). When I had tested a simple conversion > of that version to a platform_driver, genpd worked fine. > > I don't have the stack traces from these panics, but I recall some > debugfs functions being involved and the genpd late_initcalls are > related to debugfs. Looking at it now I don't see how exactly it could > fail though. Do you have the code where you faced the problem stashed somewhere? I am not (yet) advocating going back to CLK_OF_DECLARE(). But there is a definite issue with timer being ready when not using CLK_OF_DECLARE(). So, I want to make sure there the reason why we are going down the platform device path is a amply clear. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/05/2018 09:36 AM, Sekhar Nori wrote: > On Thursday 05 April 2018 07:14 PM, Bartosz Golaszewski wrote: >> 2018-04-05 15:09 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: >>> Hi Bartosz, >>> >>> On Friday 09 February 2018 10:18 PM, Michael Turquette wrote: >>>> On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>>>> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>>>> Hi David, >>>>> >>>>> I've been working on moving the genpd code from its own driver to the >>>>> psc one. I couldn't get the system to boot though and problems >>>>> happened very early in the boot sequence. I struggled to figure out >>>>> what's happening, but eventually I noticed that psc uses >>>>> CLK_OF_DECLARE() to initialize clocks. The functions registered this >>>>> way are called very early in the boot sequence, way before >>>>> late_initcall() in which the genpd framework is initialized. This of >>> >>> late_initcall() is too late for genpd to be initialized. As you may have >>> seen with the latest set of patches, we have problems with timer >>> initialization. After converting to platform devices, PSC and PLL clocks >>> get initialized post time_init(). We are working that around using >>> fixed-clocks, which hopefully will work (I still need to test many of >>> the affected platforms). >>> >>> Can you please reply with the exact issue you faced with genpd framework >>> initialization so we do have that on record. >>> >> >> The exact issue manifested itself in a NULL-pointer dereference panic >> when I tried moving the genpd code I had initially implemented as a >> separate platform driver to what I believe was v6 or v7 of David's >> series (before the psc driver became a platform driver, when it was >> still using CLK_OF_DECLARE()). When I had tested a simple conversion >> of that version to a platform_driver, genpd worked fine. >> >> I don't have the stack traces from these panics, but I recall some >> debugfs functions being involved and the genpd late_initcalls are >> related to debugfs. Looking at it now I don't see how exactly it could >> fail though. > > Do you have the code where you faced the problem stashed somewhere? I am > not (yet) advocating going back to CLK_OF_DECLARE(). But there is a > definite issue with timer being ready when not using CLK_OF_DECLARE(). > So, I want to make sure there the reason why we are going down the > platform device path is a amply clear. > > Thanks, > Sekhar > This is the thread that led to the conversion to platform devices: https://lkml.org/lkml/2018/2/9/541 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-04-05 16:36 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: > On Thursday 05 April 2018 07:14 PM, Bartosz Golaszewski wrote: >> 2018-04-05 15:09 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: >>> Hi Bartosz, >>> >>> On Friday 09 February 2018 10:18 PM, Michael Turquette wrote: >>>> On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>>>> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>>>> Hi David, >>>>> >>>>> I've been working on moving the genpd code from its own driver to the >>>>> psc one. I couldn't get the system to boot though and problems >>>>> happened very early in the boot sequence. I struggled to figure out >>>>> what's happening, but eventually I noticed that psc uses >>>>> CLK_OF_DECLARE() to initialize clocks. The functions registered this >>>>> way are called very early in the boot sequence, way before >>>>> late_initcall() in which the genpd framework is initialized. This of >>> >>> late_initcall() is too late for genpd to be initialized. As you may have >>> seen with the latest set of patches, we have problems with timer >>> initialization. After converting to platform devices, PSC and PLL clocks >>> get initialized post time_init(). We are working that around using >>> fixed-clocks, which hopefully will work (I still need to test many of >>> the affected platforms). >>> >>> Can you please reply with the exact issue you faced with genpd framework >>> initialization so we do have that on record. >>> >> >> The exact issue manifested itself in a NULL-pointer dereference panic >> when I tried moving the genpd code I had initially implemented as a >> separate platform driver to what I believe was v6 or v7 of David's >> series (before the psc driver became a platform driver, when it was >> still using CLK_OF_DECLARE()). When I had tested a simple conversion >> of that version to a platform_driver, genpd worked fine. >> >> I don't have the stack traces from these panics, but I recall some >> debugfs functions being involved and the genpd late_initcalls are >> related to debugfs. Looking at it now I don't see how exactly it could >> fail though. > > Do you have the code where you faced the problem stashed somewhere? I am > not (yet) advocating going back to CLK_OF_DECLARE(). But there is a > definite issue with timer being ready when not using CLK_OF_DECLARE(). > So, I want to make sure there the reason why we are going down the > platform device path is a amply clear. > > Thanks, > Sekhar Yes, you can still find it on my github[1]. Bart [1] github.com:brgl/linux.git topic/davinci-genpd-final-v2 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Sekhar Nori (2018-04-06 02:37:03) > > Can you please check that and confirm there is no issue with genpd and > using CLK_OF_DECLARE() to initialize clocks? > > Unless you report an issue back, or Mike and Stephen have ideas about > how to handle the dependency between PSC/PLL derived timer clock > initialization and and timer_probe(), I think we need to move back to > using CLK_OF_DECLARE(). In such a case, please use the hybrid approach where the clks required for the clockevent and/or clocksource are registered in the early CLK_OF_DECLARE path but the rest of the clks get registered with a proper platform device and driver. There are examples of this approach on other platforms already. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/06/2018 11:46 AM, Stephen Boyd wrote: > Quoting Sekhar Nori (2018-04-06 02:37:03) >> >> Can you please check that and confirm there is no issue with genpd and >> using CLK_OF_DECLARE() to initialize clocks? >> >> Unless you report an issue back, or Mike and Stephen have ideas about >> how to handle the dependency between PSC/PLL derived timer clock >> initialization and and timer_probe(), I think we need to move back to >> using CLK_OF_DECLARE(). > > In such a case, please use the hybrid approach where the clks required > for the clockevent and/or clocksource are registered in the early > CLK_OF_DECLARE path but the rest of the clks get registered with a > proper platform device and driver. There are examples of this approach > on other platforms already. > I looked at this a bit last week, but I didn't come up with any approach that I was happy with. It seems like it would be nice to just register the absolute minimum clocks needed. On DA8XX, that would just be the PLL0 AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one LPSC clock. The AUXCLKs are easy because they are just a simple gate from the oscillator. The LPSC clocks are a bit more tricky because they have a complex sequence for turning on. Furthermore, on DM646X, we need the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit messy there. The other approach I considered was to make the PLL and PSC drivers work as platform devices or pure clock devices so that when needed, the whole IP block could be registered in very early init. However, this approach will pretty much require massive changes to the drivers to remove all of the uses of devm_, dev_err, etc. so that it works when dev == NULL. Any other bright ideas or a preference for one of these two approaches? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 23 April 2018 08:29 PM, David Lechner wrote: > On 04/06/2018 11:46 AM, Stephen Boyd wrote: >> Quoting Sekhar Nori (2018-04-06 02:37:03) >>> >>> Can you please check that and confirm there is no issue with genpd and >>> using CLK_OF_DECLARE() to initialize clocks? >>> >>> Unless you report an issue back, or Mike and Stephen have ideas about >>> how to handle the dependency between PSC/PLL derived timer clock >>> initialization and and timer_probe(), I think we need to move back to >>> using CLK_OF_DECLARE(). >> >> In such a case, please use the hybrid approach where the clks required >> for the clockevent and/or clocksource are registered in the early >> CLK_OF_DECLARE path but the rest of the clks get registered with a >> proper platform device and driver. There are examples of this approach >> on other platforms already. >> > > I looked at this a bit last week, but I didn't come up with any approach > that I was happy with. It seems like it would be nice to just register > the absolute minimum clocks needed. On DA8XX, that would just be the PLL0 > AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one > LPSC clock. The AUXCLKs are easy because they are just a simple gate > from the oscillator. The LPSC clocks are a bit more tricky because they > have a complex sequence for turning on. Furthermore, on DM646X, we need > the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit > messy there. Things might change in the context of work being done here by Bartosz for converting clocks to early platform devices. But, keeping that development aside for a moment: I think this means the PLLs and PSCs need to be CLK_OF_DECLARE(). What we can have as platform devices are clocks that are not in the path to get timer clock working (like CFGCHIP clocks). Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/24/2018 03:28 AM, Sekhar Nori wrote: > On Monday 23 April 2018 08:29 PM, David Lechner wrote: >> On 04/06/2018 11:46 AM, Stephen Boyd wrote: >>> Quoting Sekhar Nori (2018-04-06 02:37:03) >>>> >>>> Can you please check that and confirm there is no issue with genpd and >>>> using CLK_OF_DECLARE() to initialize clocks? >>>> >>>> Unless you report an issue back, or Mike and Stephen have ideas about >>>> how to handle the dependency between PSC/PLL derived timer clock >>>> initialization and and timer_probe(), I think we need to move back to >>>> using CLK_OF_DECLARE(). >>> >>> In such a case, please use the hybrid approach where the clks required >>> for the clockevent and/or clocksource are registered in the early >>> CLK_OF_DECLARE path but the rest of the clks get registered with a >>> proper platform device and driver. There are examples of this approach >>> on other platforms already. >>> >> >> I looked at this a bit last week, but I didn't come up with any approach >> that I was happy with. It seems like it would be nice to just register >> the absolute minimum clocks needed. On DA8XX, that would just be the PLL0 >> AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one >> LPSC clock. The AUXCLKs are easy because they are just a simple gate >> from the oscillator. The LPSC clocks are a bit more tricky because they >> have a complex sequence for turning on. Furthermore, on DM646X, we need >> the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit >> messy there. > > Things might change in the context of work being done here by Bartosz > for converting clocks to early platform devices. > > But, keeping that development aside for a moment: I think this means the > PLLs and PSCs need to be CLK_OF_DECLARE(). What we can have as platform > devices are clocks that are not in the path to get timer clock working > (like CFGCHIP clocks). CLK_OF_DECLARE() only matters on DA850. None of the other SoCs use device tree. And on DA850, the timer0 clock is just the PLL0 AUXCLK, so PSCs can still be platform devices there. And in fact, one of the PSC clocks on DA850 depends on async3, which is a CFGCHIP clock, so if we made the PSCs CLK_OF_DECLARE(), then we also have to make at least the async3 CFGCHIP clock CLK_OF_DECLARE(). But, like I said, I think that can be avoided by leaving the PSC clocks as a platform device on DA850 (and DA830). For everything else, we need the legacy board file equivalent of CLK_OF_DECLARE(), which is basically what I had here in v5 of the series. So, if we want this to keep moving before the if/when of Bartosz's early platform device stuff, I'm thinking that I will make the PLL and PSC drivers loadable with or without a platform device and let each SoC pick which one it should use. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 24 April 2018 09:41 PM, David Lechner wrote: > On 04/24/2018 03:28 AM, Sekhar Nori wrote: >> On Monday 23 April 2018 08:29 PM, David Lechner wrote: >>> On 04/06/2018 11:46 AM, Stephen Boyd wrote: >>>> Quoting Sekhar Nori (2018-04-06 02:37:03) >>>>> >>>>> Can you please check that and confirm there is no issue with genpd and >>>>> using CLK_OF_DECLARE() to initialize clocks? >>>>> >>>>> Unless you report an issue back, or Mike and Stephen have ideas about >>>>> how to handle the dependency between PSC/PLL derived timer clock >>>>> initialization and and timer_probe(), I think we need to move back to >>>>> using CLK_OF_DECLARE(). >>>> >>>> In such a case, please use the hybrid approach where the clks required >>>> for the clockevent and/or clocksource are registered in the early >>>> CLK_OF_DECLARE path but the rest of the clks get registered with a >>>> proper platform device and driver. There are examples of this approach >>>> on other platforms already. >>>> >>> >>> I looked at this a bit last week, but I didn't come up with any approach >>> that I was happy with. It seems like it would be nice to just register >>> the absolute minimum clocks needed. On DA8XX, that would just be the >>> PLL0 >>> AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one >>> LPSC clock. The AUXCLKs are easy because they are just a simple gate >>> from the oscillator. The LPSC clocks are a bit more tricky because they >>> have a complex sequence for turning on. Furthermore, on DM646X, we need >>> the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit >>> messy there. >> >> Things might change in the context of work being done here by Bartosz >> for converting clocks to early platform devices. >> >> But, keeping that development aside for a moment: I think this means the >> PLLs and PSCs need to be CLK_OF_DECLARE(). What we can have as platform >> devices are clocks that are not in the path to get timer clock working >> (like CFGCHIP clocks). > > CLK_OF_DECLARE() only matters on DA850. None of the other SoCs use device Thats true today, but lets not make the assumption that none of the other DaVinci SoCs will gain DT support ever. We don't want churn in CCF support once someone tries to add DT support for other platforms. I already have people using DM365 in mainline, for example. > tree. And on DA850, the timer0 clock is just the PLL0 AUXCLK, so PSCs can> still be platform devices there. And in fact, one of the PSC clocks on > DA850 depends on async3, which is a CFGCHIP clock, so if we made the PSCs > CLK_OF_DECLARE(), then we also have to make at least the async3 CFGCHIP > clock CLK_OF_DECLARE(). But, like I said, I think that can be avoided by > leaving the PSC clocks as a platform device on DA850 (and DA830). Okay, I did not realize that there is a dependency between CFGCHIP clk and PSC too. > For everything else, we need the legacy board file equivalent of > CLK_OF_DECLARE(), which is basically what I had here in v5 of the > series. Yeah. I did not realize the dependency with timer when platform devices were suggested. Sorry. > > So, if we want this to keep moving before the if/when of Bartosz's > early platform device stuff, I'm thinking that I will make the PLL After looking at feedback to Bartosz's series, and the fact that previous attempts at doing the same thing were rejected, I think there is benefit in keeping CCF support moving independent Bartosz's work. Otherwise, we have no chance of getting anything merged for v4.18. Early initialization of clocks and dependency with timer is a pretty common theme across SoCs. As and when a better way to support is found, I am sure DaVinci can be converted over along with rest of the devices. > and PSC drivers loadable with or without a platform device and let > each SoC pick which one it should use. This sounds flexible. We have to see how the patches look. But I would caution against adding a lot of dead code. If there is no way some of the clocks will be useful as platform device, I see no point of supporting them as such just in anticipation. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-04-25 8:07 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: > On Tuesday 24 April 2018 09:41 PM, David Lechner wrote: >> On 04/24/2018 03:28 AM, Sekhar Nori wrote: >>> On Monday 23 April 2018 08:29 PM, David Lechner wrote: >>>> On 04/06/2018 11:46 AM, Stephen Boyd wrote: >>>>> Quoting Sekhar Nori (2018-04-06 02:37:03) >>>>>> >>>>>> Can you please check that and confirm there is no issue with genpd and >>>>>> using CLK_OF_DECLARE() to initialize clocks? >>>>>> >>>>>> Unless you report an issue back, or Mike and Stephen have ideas about >>>>>> how to handle the dependency between PSC/PLL derived timer clock >>>>>> initialization and and timer_probe(), I think we need to move back to >>>>>> using CLK_OF_DECLARE(). >>>>> >>>>> In such a case, please use the hybrid approach where the clks required >>>>> for the clockevent and/or clocksource are registered in the early >>>>> CLK_OF_DECLARE path but the rest of the clks get registered with a >>>>> proper platform device and driver. There are examples of this approach >>>>> on other platforms already. >>>>> >>>> >>>> I looked at this a bit last week, but I didn't come up with any approach >>>> that I was happy with. It seems like it would be nice to just register >>>> the absolute minimum clocks needed. On DA8XX, that would just be the >>>> PLL0 >>>> AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one >>>> LPSC clock. The AUXCLKs are easy because they are just a simple gate >>>> from the oscillator. The LPSC clocks are a bit more tricky because they >>>> have a complex sequence for turning on. Furthermore, on DM646X, we need >>>> the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit >>>> messy there. >>> >>> Things might change in the context of work being done here by Bartosz >>> for converting clocks to early platform devices. >>> >>> But, keeping that development aside for a moment: I think this means the >>> PLLs and PSCs need to be CLK_OF_DECLARE(). What we can have as platform >>> devices are clocks that are not in the path to get timer clock working >>> (like CFGCHIP clocks). >> >> CLK_OF_DECLARE() only matters on DA850. None of the other SoCs use device > > Thats true today, but lets not make the assumption that none of the > other DaVinci SoCs will gain DT support ever. We don't want churn in CCF > support once someone tries to add DT support for other platforms. > > I already have people using DM365 in mainline, for example. > >> tree. And on DA850, the timer0 clock is just the PLL0 AUXCLK, so PSCs can> still be platform devices there. And in fact, one of the PSC clocks on >> DA850 depends on async3, which is a CFGCHIP clock, so if we made the PSCs >> CLK_OF_DECLARE(), then we also have to make at least the async3 CFGCHIP >> clock CLK_OF_DECLARE(). But, like I said, I think that can be avoided by >> leaving the PSC clocks as a platform device on DA850 (and DA830). > > Okay, I did not realize that there is a dependency between CFGCHIP clk > and PSC too. > >> For everything else, we need the legacy board file equivalent of >> CLK_OF_DECLARE(), which is basically what I had here in v5 of the >> series. > > Yeah. I did not realize the dependency with timer when platform devices > were suggested. Sorry. > >> >> So, if we want this to keep moving before the if/when of Bartosz's >> early platform device stuff, I'm thinking that I will make the PLL > > After looking at feedback to Bartosz's series, and the fact that > previous attempts at doing the same thing were rejected, I think there > is benefit in keeping CCF support moving independent Bartosz's work. > Otherwise, we have no chance of getting anything merged for v4.18. > It's true, that the feedback was not very positive, but I'm still thinking I can come up with something that would get accepted. Last time someone came up with the idea, the problem it was supposed to solve was different (dependencies between devices) and was eventually fixed with introduction of probe deferral. This time around we really want these devices probed early. The main concern was using a device tree compatible. If I can instead just early probe all devices whose drivers are registered as early without touching the device tree (as Frank Rowand suggested) then maybe the response of the community would be better. Thanks, Bart > Early initialization of clocks and dependency with timer is a pretty > common theme across SoCs. As and when a better way to support is found, > I am sure DaVinci can be converted over along with rest of the devices. > >> and PSC drivers loadable with or without a platform device and let >> each SoC pick which one it should use. > > This sounds flexible. We have to see how the patches look. But I would > caution against adding a lot of dead code. If there is no way some of > the clocks will be useful as platform device, I see no point of > supporting them as such just in anticipation. > > Thanks, > Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-04-25 12:09 GMT+02:00 Bartosz Golaszewski <brgl@bgdev.pl>: > 2018-04-25 8:07 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: >> On Tuesday 24 April 2018 09:41 PM, David Lechner wrote: >>> On 04/24/2018 03:28 AM, Sekhar Nori wrote: >>>> On Monday 23 April 2018 08:29 PM, David Lechner wrote: >>>>> On 04/06/2018 11:46 AM, Stephen Boyd wrote: >>>>>> Quoting Sekhar Nori (2018-04-06 02:37:03) >>>>>>> >>>>>>> Can you please check that and confirm there is no issue with genpd and >>>>>>> using CLK_OF_DECLARE() to initialize clocks? >>>>>>> >>>>>>> Unless you report an issue back, or Mike and Stephen have ideas about >>>>>>> how to handle the dependency between PSC/PLL derived timer clock >>>>>>> initialization and and timer_probe(), I think we need to move back to >>>>>>> using CLK_OF_DECLARE(). >>>>>> >>>>>> In such a case, please use the hybrid approach where the clks required >>>>>> for the clockevent and/or clocksource are registered in the early >>>>>> CLK_OF_DECLARE path but the rest of the clks get registered with a >>>>>> proper platform device and driver. There are examples of this approach >>>>>> on other platforms already. >>>>>> >>>>> >>>>> I looked at this a bit last week, but I didn't come up with any approach >>>>> that I was happy with. It seems like it would be nice to just register >>>>> the absolute minimum clocks needed. On DA8XX, that would just be the >>>>> PLL0 >>>>> AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one >>>>> LPSC clock. The AUXCLKs are easy because they are just a simple gate >>>>> from the oscillator. The LPSC clocks are a bit more tricky because they >>>>> have a complex sequence for turning on. Furthermore, on DM646X, we need >>>>> the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit >>>>> messy there. >>>> >>>> Things might change in the context of work being done here by Bartosz >>>> for converting clocks to early platform devices. >>>> >>>> But, keeping that development aside for a moment: I think this means the >>>> PLLs and PSCs need to be CLK_OF_DECLARE(). What we can have as platform >>>> devices are clocks that are not in the path to get timer clock working >>>> (like CFGCHIP clocks). >>> >>> CLK_OF_DECLARE() only matters on DA850. None of the other SoCs use device >> >> Thats true today, but lets not make the assumption that none of the >> other DaVinci SoCs will gain DT support ever. We don't want churn in CCF >> support once someone tries to add DT support for other platforms. >> >> I already have people using DM365 in mainline, for example. >> >>> tree. And on DA850, the timer0 clock is just the PLL0 AUXCLK, so PSCs can> still be platform devices there. And in fact, one of the PSC clocks on >>> DA850 depends on async3, which is a CFGCHIP clock, so if we made the PSCs >>> CLK_OF_DECLARE(), then we also have to make at least the async3 CFGCHIP >>> clock CLK_OF_DECLARE(). But, like I said, I think that can be avoided by >>> leaving the PSC clocks as a platform device on DA850 (and DA830). >> >> Okay, I did not realize that there is a dependency between CFGCHIP clk >> and PSC too. >> >>> For everything else, we need the legacy board file equivalent of >>> CLK_OF_DECLARE(), which is basically what I had here in v5 of the >>> series. >> >> Yeah. I did not realize the dependency with timer when platform devices >> were suggested. Sorry. >> >>> >>> So, if we want this to keep moving before the if/when of Bartosz's >>> early platform device stuff, I'm thinking that I will make the PLL >> >> After looking at feedback to Bartosz's series, and the fact that >> previous attempts at doing the same thing were rejected, I think there >> is benefit in keeping CCF support moving independent Bartosz's work. >> Otherwise, we have no chance of getting anything merged for v4.18. >> > > It's true, that the feedback was not very positive, but I'm still > thinking I can come up with something that would get accepted. Last > time someone came up with the idea, the problem it was supposed to > solve was different (dependencies between devices) and was eventually > fixed with introduction of probe deferral. > > This time around we really want these devices probed early. The main > concern was using a device tree compatible. If I can instead just > early probe all devices whose drivers are registered as early without > touching the device tree (as Frank Rowand suggested) then maybe the * It was actually Mark Rutland. > response of the community would be better. > > Thanks, > Bart > >> Early initialization of clocks and dependency with timer is a pretty >> common theme across SoCs. As and when a better way to support is found, >> I am sure DaVinci can be converted over along with rest of the devices. >> >>> and PSC drivers loadable with or without a platform device and let >>> each SoC pick which one it should use. >> >> This sounds flexible. We have to see how the patches look. But I would >> caution against adding a lot of dead code. If there is no way some of >> the clocks will be useful as platform device, I see no point of >> supporting them as such just in anticipation. >> >> Thanks, >> Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html