Re: [PATCH v4 01/10] clk: tegra20/30: Add custom EMC clock implementation
From: Stephen Boyd <sboyd@kernel.org>
Date: 2019-06-19 01:14:05
Also in:
linux-clk, linux-tegra, lkml
Quoting Dmitry Osipenko (2019-06-16 16:35:42)
A proper External Memory Controller clock rounding and parent selection functionality is required by the EMC drivers. It is not available using the generic clock implementation, hence add a custom one.
Why isn't it available? Please add this information to the commit text.
The clock rate rounding shall be done by the EMC drivers because they have information about available memory timings, so the drivers will have to register a callback that will round the requested rate. EMC clock users won't be able to request EMC clock by getting -EPROBE_DEFER until EMC driver is probed and the callback is set up. The functionality is somewhat similar to the clk-emc.c which serves Tegra124+ SoC's, the later HW generations support more parent clock sources and the HW configuration and integration with the EMC drivers differs a tad from the older gens, hence it's not really worth to try to squash everything into a single source file. Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
[...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/clk/tegra/clk-tegra20-emc.c b/drivers/clk/tegra/clk-tegra20-emc.c new file mode 100644 index 000000000000..b7f64ad5c04c --- /dev/null +++ b/drivers/clk/tegra/clk-tegra20-emc.c@@ -0,0 +1,305 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bits.h> +#include <linux/clk-provider.h> +#include <linux/clk/tegra.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/slab.h> + +#include "clk.h" + +#define CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK GENMASK(7, 0) +#define CLK_SOURCE_EMC_2X_CLK_SRC_MASK GENMASK(31, 30) +#define CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT 30 + +#define MC_EMC_SAME_FREQ BIT(16) +#define USE_PLLM_UD BIT(29) + +#define EMC_SRC_PLL_M 0 +#define EMC_SRC_PLL_C 1 +#define EMC_SRC_PLL_P 2 +#define EMC_SRC_CLK_M 3 +
[...]
+void tegra20_clk_set_emc_round_callback(tegra20_clk_emc_round_cb *round_cb,
+ void *cb_arg)
+{
+ struct clk *clk = __clk_lookup("emc");
+ struct tegra_clk_emc *emc;
+ struct clk_hw *hw;
+
+ if (clk) {
+ hw = __clk_get_hw(clk);
+ emc = to_tegra_clk_emc(hw);
+
+ emc->round_cb = round_cb;
+ emc->cb_arg = cb_arg;
+ }
+}
+
+bool tegra20_clk_emc_driver_available(struct clk_hw *emc_hw)
+{
+ return to_tegra_clk_emc(emc_hw)->round_cb != NULL;
+}
+
+struct clk *tegra20_clk_register_emc(void __iomem *ioaddr)Is this used outside this file?
+{
+ struct tegra_clk_emc *emc;
+ struct clk_init_data init;
+ struct clk *clk;
+
+ emc = kzalloc(sizeof(*emc), GFP_KERNEL);
+ if (!emc)
+ return NULL;
+
+ init.name = "emc";
+ init.ops = &tegra_clk_emc_ops;
+ init.flags = CLK_IS_CRITICAL;Can you please add a comment in the code why this clk is critical?
+ init.parent_names = emc_parent_clk_names;
+ init.num_parents = ARRAY_SIZE(emc_parent_clk_names);
+
+ emc->reg = ioaddr;
+ emc->hw.init = &init;
+
+ clk = clk_register(NULL, &emc->hw);
+ if (IS_ERR(clk)) {
+ kfree(emc);
+ return NULL;
+ }
+
+ return clk;
+}
+
+void tegra30_clk_set_emc_round_callback(tegra30_clk_emc_round_cb *round_cb,
+ void *cb_arg)
+{
+ tegra20_clk_set_emc_round_callback(round_cb, cb_arg);
+}
+
+bool tegra30_clk_emc_driver_available(struct clk_hw *emc_hw)
+{
+ return tegra20_clk_emc_driver_available(emc_hw);
+}
+
+struct clk *tegra30_clk_register_emc(void __iomem *ioaddr)
+{
+ struct tegra_clk_emc *emc;
+ struct clk_hw *hw;
+ struct clk *clk;
+
+ clk = tegra20_clk_register_emc(ioaddr);
+ if (!clk)
+ return NULL;
+
+ hw = __clk_get_hw(clk);It would be nicer to not use __clk_get_hw() and have the above function return the clk_hw pointer instead. Then some driver can return the clk pointer from there, if it's even needed for anything?
quoted hunk ↗ jump to hunk
+ emc = to_tegra_clk_emc(hw); + emc->want_low_jitter = true; + + return clk; +} + +int tegra30_clk_prepare_emc_mc_same_freq(struct clk *emc_clk, bool same) +{ + struct tegra_clk_emc *emc; + struct clk_hw *hw; + + if (emc_clk) { + hw = __clk_get_hw(emc_clk); + emc = to_tegra_clk_emc(hw); + emc->mc_same_freq = same; + + return 0; + } + + return -EINVAL; +}diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c index bcd871134f45..f937a0f35afb 100644 --- a/drivers/clk/tegra/clk-tegra20.c +++ b/drivers/clk/tegra/clk-tegra20.c@@ -1115,6 +1083,8 @@ static struct clk *tegra20_clk_src_onecell_get(struct of_phandle_args *clkspec, if (IS_ERR(clk)) return clk; + hw = __clk_get_hw(clk); + /* * Tegra20 CDEV1 and CDEV2 clocks are a bit special case, their parent * clock is created by the pinctrl driver. It is possible for clk user@@ -1124,13 +1094,16 @@ static struct clk *tegra20_clk_src_onecell_get(struct of_phandle_args *clkspec, */ if (clkspec->args[0] == TEGRA20_CLK_CDEV1 || clkspec->args[0] == TEGRA20_CLK_CDEV2) { - hw = __clk_get_hw(clk); - parent_hw = clk_hw_get_parent(hw); if (!parent_hw) return ERR_PTR(-EPROBE_DEFER); } + if (clkspec->args[0] == TEGRA20_CLK_EMC) { + if (!tegra20_clk_emc_driver_available(hw)) + return ERR_PTR(-EPROBE_DEFER); + } + return clk; }diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c index 7b4c6a488527..fab075808c20 100644 --- a/drivers/clk/tegra/clk-tegra30.c +++ b/drivers/clk/tegra/clk-tegra30.c@@ -1302,6 +1298,26 @@ static struct tegra_audio_clk_info tegra30_audio_plls[] = { { "pll_a", &pll_a_params, tegra_clk_pll_a, "pll_p_out1" }, }; +static struct clk *tegra30_clk_src_onecell_get(struct of_phandle_args *clkspec, + void *data) +{ + struct clk_hw *hw; + struct clk *clk; + + clk = of_clk_src_onecell_get(clkspec, data); + if (IS_ERR(clk)) + return clk; + + hw = __clk_get_hw(clk); + + if (clkspec->args[0] == TEGRA30_CLK_EMC) { + if (!tegra30_clk_emc_driver_available(hw)) + return ERR_PTR(-EPROBE_DEFER); + } + + return clk; +}
This above function makes me uneasy because it looks like a clk_get() on top of a clk_get()?
+
static void __init tegra30_clock_init(struct device_node *np)
{
struct device_node *node;