| 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: Thu, 6 Nov 2014 17:04:25 +0900
From: Alexandre Courbot <acourbot@...dia.com>
To: Tomeu Vizoso <tomeu.vizoso@...labora.com>,
<linux-tegra@...r.kernel.org>
CC: Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
"Mikko Perttunen" <mperttunen@...dia.com>,
Peter De Schrijver <pdeschrijver@...dia.com>,
Prashant Gaikwad <pgaikwad@...dia.com>,
"Mike Turquette" <mturquette@...aro.org>,
Stephen Warren <swarren@...dotorg.org>,
Thierry Reding <thierry.reding@...il.com>,
Alexandre Courbot <gnurou@...il.com>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 11/13] clk: tegra: Add EMC clock driver
On 10/30/2014 01:22 AM, Tomeu Vizoso wrote:
> From: Mikko Perttunen <mperttunen@...dia.com>
>
> The driver is currently only tested on Tegra124 Jetson TK1, but should
> work with other Tegra124 boards, provided that correct EMC tables are
> provided through the device tree. Older chip models have differing
> timing change sequences, so they are not currently supported.
>
> Signed-off-by: Mikko Perttunen <mperttunen@...dia.com>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@...labora.com>
>
> ---
>
> v3: * Add some locking to protect the registers that are shared with the MC
> clock
>
> v2: * Make sure that the clock is properly registered
> * Bail out early from attempts to set the same rate
> ---
> drivers/clk/tegra/Makefile | 2 +-
> drivers/clk/tegra/clk-emc.c | 470 +++++++++++++++++++++++++++++++++++++++
> drivers/clk/tegra/clk-tegra124.c | 4 +-
> drivers/clk/tegra/clk.h | 2 +
> 4 files changed, 476 insertions(+), 2 deletions(-)
> create mode 100644 drivers/clk/tegra/clk-emc.c
>
> diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> index f7dfb72..240e5b4 100644
> --- a/drivers/clk/tegra/Makefile
> +++ b/drivers/clk/tegra/Makefile
> @@ -14,4 +14,4 @@ obj-y += clk-tegra-super-gen4.o
> obj-$(CONFIG_ARCH_TEGRA_2x_SOC) += clk-tegra20.o
> obj-$(CONFIG_ARCH_TEGRA_3x_SOC) += clk-tegra30.o
> obj-$(CONFIG_ARCH_TEGRA_114_SOC) += clk-tegra114.o
> -obj-$(CONFIG_ARCH_TEGRA_124_SOC) += clk-tegra124.o
> +obj-$(CONFIG_ARCH_TEGRA_124_SOC) += clk-tegra124.o clk-emc.o
> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
> new file mode 100644
> index 0000000..182a059
> --- /dev/null
> +++ b/drivers/clk/tegra/clk-emc.c
> @@ -0,0 +1,470 @@
> +/*
> + * drivers/clk/tegra/clk-emc.c
> + *
> + * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved.
> + *
> + * Author:
> + * Mikko Perttunen <mperttunen@...dia.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/sort.h>
> +#include <linux/string.h>
> +
> +#include <soc/tegra/fuse.h>
> +#include <soc/tegra/memory.h>
> +
> +#define CLK_SOURCE_EMC 0x19c
> +
> +#define CLK_SOURCE_EMC_EMC_2X_CLK_DIVISOR_SHIFT 0
> +#define CLK_SOURCE_EMC_EMC_2X_CLK_DIVISOR_MASK 0xff
> +#define CLK_SOURCE_EMC_EMC_2X_CLK_DIVISOR(x) (((x) & CLK_SOURCE_EMC_EMC_2X_CLK_DIVISOR_MASK) << \
> + CLK_SOURCE_EMC_EMC_2X_CLK_DIVISOR_SHIFT)
> +
> +#define CLK_SOURCE_EMC_EMC_2X_CLK_SRC_SHIFT 29
> +#define CLK_SOURCE_EMC_EMC_2X_CLK_SRC_MASK 0x7
> +#define CLK_SOURCE_EMC_EMC_2X_CLK_SRC(x) (((x) & CLK_SOURCE_EMC_EMC_2X_CLK_SRC_MASK) << \
> + CLK_SOURCE_EMC_EMC_2X_CLK_SRC_SHIFT)
> +
> +const char *emc_parent_clk_names[] = {
> + "pll_m", "pll_c", "pll_p", "clk_m", "pll_m_ud",
> + "pll_c2", "pll_c3", "pll_c_ud"
> +};
> +
> +/* List of clock sources for various parents the EMC clock can have.
> + * When we change the timing to a timing with a parent that has the same
> + * clock source as the current parent, we must first change to a backup
> + * timing that has a different clock source.
> + */
Nit: comment style throughout this file.
> +
> +#define EMC_SRC_PLL_M 0
> +#define EMC_SRC_PLL_C 1
> +#define EMC_SRC_PLL_P 2
> +#define EMC_SRC_CLK_M 3
> +#define EMC_SRC_PLL_C2 4
> +#define EMC_SRC_PLL_C3 5
> +const char emc_parent_clk_sources[] = {
> + EMC_SRC_PLL_M, EMC_SRC_PLL_C, EMC_SRC_PLL_P, EMC_SRC_CLK_M,
> + EMC_SRC_PLL_M, EMC_SRC_PLL_C2, EMC_SRC_PLL_C3, EMC_SRC_PLL_C
> +};
> +
> +struct emc_timing {
> + unsigned long rate, parent_rate;
> + u8 parent_index;
> + struct clk *parent;
> + u32 ram_code;
> +};
> +
> +struct tegra_emc {
> + struct clk_hw hw;
> + void __iomem *clk_regs;
> + struct clk *prev_parent;
> + bool changing_timing;
> +
> + int num_timings;
> + struct emc_timing *timings;
> + spinlock_t *lock;
> +};
> +
> +/* * * * * * * * * * * * * * * * * * * * * * * * * *
> + * Common clock framework callback implementations *
> + * * * * * * * * * * * * * * * * * * * * * * * * * */
> +
> +unsigned long emc_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
Shouldn't this function be static? Also applies to other functions in
this file.
> +{
> + struct tegra_emc *tegra = container_of(hw, struct tegra_emc, hw);
> + u32 val, div;
> +
> + /* CCF wrongly assumes that the parent won't change during set_rate,
> + * so get the parent rate explicitly.
> + */
> + parent_rate = __clk_get_rate(__clk_get_parent(hw->clk));
> +
> + val = readl(tegra->clk_regs + CLK_SOURCE_EMC);
> + div = val & CLK_SOURCE_EMC_EMC_2X_CLK_DIVISOR_MASK;
> +
> + return parent_rate / (div + 2) * 2;
> +}
> +
> +/* Rounds up unless no higher rate exists, in which case down. This way is
> + * safer since things have EMC rate floors. Also don't touch parent_rate
> + * since we don't want the CCF to play with our parent clocks.
> + */
> +long emc_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
> +{
> + struct tegra_emc *tegra = container_of(hw, struct tegra_emc, hw);
> + u8 ram_code = tegra_read_ram_code();
> + struct emc_timing *timing;
> + int i;
> +
> + /* Returning the original rate will lead to a more sensible error
> + * message when emc_set_rate fails.
> + */
> + if (tegra->num_timings == 0)
> + return rate;
> +
> + for (i = 0; i < tegra->num_timings; ++i) {
> + timing = tegra->timings + i;
> + if (timing->ram_code != ram_code)
> + continue;
> +
> + if (timing->rate >= rate)
> + return timing->rate;
> + }
> +
> + return tegra->timings[tegra->num_timings - 1].rate;
> +}
> +
> +u8 emc_get_parent(struct clk_hw *hw)
> +{
> + struct tegra_emc *tegra = container_of(hw, struct tegra_emc, hw);
> + u32 val;
> +
> + val = readl(tegra->clk_regs + CLK_SOURCE_EMC);
> +
> + return (val >> CLK_SOURCE_EMC_EMC_2X_CLK_SRC_SHIFT)
> + & CLK_SOURCE_EMC_EMC_2X_CLK_SRC_MASK;
> +}
> +
> +static int emc_set_timing(struct tegra_emc *tegra, struct emc_timing *timing)
> +{
> + int err;
> + u8 div;
> + u32 car_value;
> + unsigned long flags = 0;
> +
> + pr_debug("going to rate %ld prate %ld p %s\n",
> + timing->rate, timing->parent_rate,
> + __clk_get_name(timing->parent));
> +
> + if (emc_get_parent(&tegra->hw) == timing->parent_index &&
> + clk_get_rate(timing->parent) != timing->parent_rate) {
> + BUG();
> + return -EINVAL;
> + }
> +
> + tegra->changing_timing = true;
> +
> + err = clk_set_rate(timing->parent, timing->parent_rate);
> + if (err) {
> + pr_err("cannot change parent %s rate to %ld: %d\n",
> + __clk_get_name(timing->parent),
> + timing->parent_rate, err);
> +
> + return err;
> + }
> +
> + err = clk_prepare_enable(timing->parent);
> + if (err) {
> + pr_err("cannot enable parent clock: %d\n", err);
> + return err;
> + }
> +
> + div = timing->parent_rate / (timing->rate / 2) - 2;
> +
> + tegra_emc_prepare_timing_change(timing->rate);
Since there is no locking whatsoever in the EMC driver, I wonder if this
function should be called after the spinlock is acquired to ensure it
cannot be called twice?
> +
> + spin_lock_irqsave(tegra->lock, flags);
> +
> + car_value = readl(tegra->clk_regs + CLK_SOURCE_EMC);
> +
> + car_value &= ~CLK_SOURCE_EMC_EMC_2X_CLK_SRC(~0);
> + car_value |= CLK_SOURCE_EMC_EMC_2X_CLK_SRC(timing->parent_index);
> +
> + car_value &= ~CLK_SOURCE_EMC_EMC_2X_CLK_DIVISOR(~0);
> + car_value |= CLK_SOURCE_EMC_EMC_2X_CLK_DIVISOR(div);
> +
> + writel(car_value, tegra->clk_regs + CLK_SOURCE_EMC);
> +
> + spin_unlock_irqrestore(tegra->lock, flags);
> +
> + tegra_emc_complete_timing_change(timing->rate);
Same for this one.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists