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

[Dmitry Osipenko <digetx@gmail.com>]

19.06.2019 4:14, 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.

Ok! It's not available because only the EMC driver has information about available memory
timings and thus about the available rates (and parents consequently).

>> 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?

Yes, it is. It is getting used even in this patch, you just snipped it off in the reply.

>> +{
>> +       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?

Okay!

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

This is solely for internal use by the tegra-clk driver itself, this function isn't publicly
exposed. Again, you snipped off a lot in the reply, please take a look at the original patch.

Technically I could return clk_hw from here, but it doesn't make much sense in the context
of the tegra-clk driver. Please see how these functions are getting used in the original patch.

In short, tegra-clk driver operates with clk struct and not clk_hw. You already was asking
the same question in v3 and I replied that converting the whole driver for clk_hw will take
a lot of effort. I suppose it will be somewhat similar to what was done for the IMX driver
recently [1].

[1] https://lkml.org/lkml/2019/5/2/170

>> +       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()? 

Yes, this way we're intercepting clk_get() within the driver, which is a very nice feature.
We're already doing that in the clk-tegra20.c, can't see any problems with that.