lkml.org 
[lkml]   [2019]   [Jun]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v4 01/10] clk: tegra20/30: Add custom EMC clock implementation
    Date
    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>
    [...]
    > 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?

    > + 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;

    \
     
     \ /
      Last update: 2019-06-19 03:14    [W:2.379 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site