| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 18 Jun 2019 18:14:00 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Dmitry Osipenko <digetx@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Joseph Lo <josephl@...dia.com>,
Michael Turquette <mturquette@...libre.com>,
Peter De Schrijver <pdeschrijver@...dia.com>,
Prashant Gaikwad <pgaikwad@...dia.com>,
Rob Herring <robh+dt@...nel.org>,
Thierry Reding <thierry.reding@...il.com>
Cc: devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 01/10] clk: tegra20/30: Add custom EMC clock implementation
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@...il.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;
Powered by blists - more mailing lists