Re: [PATCH v4 01/10] clk: tegra20/30: Add custom EMC clock implementation

[Stephen Boyd]

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@xxxxxxxxx>
[...]
> 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;