| 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
| ||
|
Message-ID: <532C355C.3090606@gmail.com>
Date: Fri, 21 Mar 2014 13:49:32 +0100
From: Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
To: Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
Mike Turquette <mturquette@...aro.org>
CC: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
Antoine Ténart
<antoine.tenart@...e-electrons.com>
Subject: Re: [PATCH 1/5] clk: berlin: add support for berlin plls
On 03/21/2014 12:43 PM, Alexandre Belloni wrote:
> This drivers allows to provide DT clocks for the cpu and system PLLs found on
> Marvell Berlin SoCs.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@...e-electrons.com>
> ---
> drivers/clk/Makefile | 1 +
> drivers/clk/berlin/Makefile | 1 +
> drivers/clk/berlin/clk.h | 35 +++++++++++++
> drivers/clk/berlin/pll-berlin2.c | 41 ++++++++++++++++
> drivers/clk/berlin/pll-berlin2q.c | 41 ++++++++++++++++
> drivers/clk/berlin/pll.c | 100 ++++++++++++++++++++++++++++++++++++++
> 6 files changed, 219 insertions(+)
> create mode 100644 drivers/clk/berlin/Makefile
> create mode 100644 drivers/clk/berlin/clk.h
> create mode 100644 drivers/clk/berlin/pll-berlin2.c
> create mode 100644 drivers/clk/berlin/pll-berlin2q.c
> create mode 100644 drivers/clk/berlin/pll.c
>
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index a367a9831717..4a2602737c27 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
> obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
> obj-$(CONFIG_COMMON_CLK_XGENE) += clk-xgene.o
> obj-$(CONFIG_COMMON_CLK_AT91) += at91/
> +obj-$(CONFIG_ARCH_BERLIN) += berlin/
> obj-$(CONFIG_ARCH_HI3xxx) += hisilicon/
> obj-$(CONFIG_COMMON_CLK_KEYSTONE) += keystone/
I'll have a look at clk/Makefile later myself, but alphabetic ordering
seems to be broken by ARCH_HI3xxx already. Anyway, we shouldn't just
add another violator.
> ifeq ($(CONFIG_COMMON_CLK), y)
> diff --git a/drivers/clk/berlin/Makefile b/drivers/clk/berlin/Makefile
> new file mode 100644
> index 000000000000..4f6580e2a3ee
> --- /dev/null
> +++ b/drivers/clk/berlin/Makefile
> @@ -0,0 +1 @@
> +obj-y += pll.o pll-berlin2.o pll-berlin2q.o
obj-y += pll.o
obj-$(CONFIG_MACH_BERLIN_BG2) += pll-berlin2.o
obj-$(CONFIG_MACH_BERLIN_BG2CD) += pll-berlin2.o
obj-$(CONFIG_MACH_BERLIN_BG2Q) += pll-berlin2q.o
Which reminds me, that we forgot to add MACH_BERLIN_BG2Q to
arch/arm/mach-berlin/Kconfig. Can you spin a patch?
> diff --git a/drivers/clk/berlin/clk.h b/drivers/clk/berlin/clk.h
I prefer to have this include named "common.h" as I guess we will
dump all common declarations in here.
> new file mode 100644
> index 000000000000..41459db31a02
> --- /dev/null
> +++ b/drivers/clk/berlin/clk.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (c) 2013 Marvell Technology Group Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef __BERLIN_CLK_H
> +#define __BERLIN_CLK_H
> +
> +#include <linux/clk-provider.h>
What is this include good for in here?
You will need struct device_node declared, but a simple
struct device_node;
should be sufficient.
> +struct berlin_pllmap {
> + const u8 *vcodiv;
> + u32 fbdiv_mask;
> + u32 rfdiv_mask;
> + u32 divsel_mask;
> + u8 fbdiv_shift;
> + u8 rfdiv_shift;
> + u8 divsel_shift;
> + u8 mult;
> +};
> +
> +extern void __init berlin_pll_setup(struct device_node *np,
> + struct berlin_pllmap *map);
> +
> +#endif /* BERLIN_CLK_H */
> diff --git a/drivers/clk/berlin/pll-berlin2.c b/drivers/clk/berlin/pll-berlin2.c
> new file mode 100644
> index 000000000000..5f5b4674f811
> --- /dev/null
> +++ b/drivers/clk/berlin/pll-berlin2.c
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (c) 2014 Marvell Technology Group Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <linux/clk-provider.h>
Please sort includes alphabetically, it will save us some pain
in the future.
> +#include "clk.h"
#include "common.h"
> +static const u8 vcodiv_berlin2[] = {10, 15, 20, 25, 30, 40, 50, 60, 80};
> +
> +static struct berlin_pllmap berlin_pll_map = {
> + .vcodiv = vcodiv_berlin2,
> + .fbdiv_mask = 0x1FF,
> + .rfdiv_mask = 0x1F,
> + .divsel_mask = 0xF,
divsel_mask allows 16 possible values, vcodiv_berlin2[] only provides
9, and common pll driver below does not know how many valid vcodiv
values are passed. That can become dangerous..
I'd rather extend vcodiv_berlin2 to full divsel range and provide
safe (=1) divisiors. This way wrong/new register values will only
break clock frequency derived.
> + .fbdiv_shift = 6,
> + .rfdiv_shift = 1,
> + .divsel_shift = 7,
Have .foo_mask and .foo_shift together?
> + .mult = 10,
> +};
> +
> +static void __init berlin2_pll_setup(struct device_node *np)
> +{
> + berlin_pll_setup(np, &berlin_pll_map);
> +}
> +CLK_OF_DECLARE(berlin2q_pll, "marvell,berlin2-pll", berlin2_pll_setup);
> +
> diff --git a/drivers/clk/berlin/pll-berlin2q.c b/drivers/clk/berlin/pll-berlin2q.c
> new file mode 100644
> index 000000000000..8ddfa69cf8c4
> --- /dev/null
> +++ b/drivers/clk/berlin/pll-berlin2q.c
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (c) 2013 Marvell Technology Group Ltd.
nit: 2014?
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/kernel.h>
> +#include <linux/clk-provider.h>
same as above.
> +#include "clk.h"
ditto.
> +static const u8 vcodiv_berlin2q[] = {1, 0, 2, 0, 3, 4, 0, 6, 8};
> +
> +static struct berlin_pllmap berlin2q_pll_map = {
> + .vcodiv = vcodiv_berlin2q,
> + .fbdiv_mask = 0x1FF,
> + .rfdiv_mask = 0x1F,
> + .divsel_mask = 0xF,
> + .fbdiv_shift = 7,
> + .rfdiv_shift = 2,
> + .divsel_shift = 9,
> + .mult = 1,
> +};
ditto.
> +static void __init berlin2q_pll_setup(struct device_node *np)
> +{
> + berlin_pll_setup(np, &berlin2q_pll_map);
> +}
> +CLK_OF_DECLARE(berlin2q_pll, "marvell,berlin2q-pll", berlin2q_pll_setup);
> +
> diff --git a/drivers/clk/berlin/pll.c b/drivers/clk/berlin/pll.c
> new file mode 100644
> index 000000000000..188b385af436
> --- /dev/null
> +++ b/drivers/clk/berlin/pll.c
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright (c) 2013 Marvell Technology Group Ltd.
nit: 2014?
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/clk-provider.h>
same as above.
> +#include "clk.h"
ditto.
> +struct berlin_pll {
> + struct clk_hw hw;
> + void __iomem *base;
> + void *data;
struct berlin2_pllmap *map instead?
> +};
> +
> +#define to_berlin_pll(hw) container_of(hw, struct berlin_pll, hw)
> +
> +#define PLL_CTRL0 0x00
> +#define PLL_CTRL1 0x04
> +
> +static inline u32 berlin_pll_read(struct berlin_pll *pll, unsigned long offset)
> +{
> + return readl_relaxed(pll->base + offset);
> +}
> +
> +static unsigned long berlin_pll_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
Maybe have a nice little comment on how pll output frequency is
calculated out of the 5 variables?
> +{
> + struct berlin_pll *pll = to_berlin_pll(hw);
> + struct berlin_pllmap *map = pll->data;
> + u32 val, fbdiv, rfdiv, vcodivsel;
> +
> + val = berlin_pll_read(pll, PLL_CTRL0);
> + fbdiv = (val >> map->fbdiv_shift) & map->fbdiv_mask;
> + rfdiv = (val >> map->rfdiv_shift) & map->rfdiv_mask;
> + if (rfdiv == 0)
> + rfdiv = 1;
> +
> + val = berlin_pll_read(pll, PLL_CTRL1);
> + vcodivsel = (val >> map->divsel_shift) & map->divsel_mask;
> +
> + return parent_rate * fbdiv * map->mult / rfdiv /
> + map->vcodiv[vcodivsel];
IMHO
parent_rate *= fbdiv * map->mult;
parent_rate /= rfdiv;
return parent_rate / map->vcodiv[vcodivsel];
makes it slightly more readable, but that is up to you.
> +}
> +
> +static const struct clk_ops berlin_pll_ops = {
> + .recalc_rate = berlin_pll_recalc_rate,
> +};
> +
> +void __init berlin_pll_setup(struct device_node *np,
> + struct berlin_pllmap *map)
> +{
> + struct clk_init_data init;
> + struct berlin_pll *pll;
> + const char *parent_name;
> + struct clk *clk;
> + int ret;
> +
> + pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> + if (WARN_ON(!pll))
> + return;
> +
> + pll->base = of_iomap(np, 0);
> + if (WARN_ON(!pll->base))
> + return;
> +
> + pll->data = map;
pll->map = map;
> +
> + init.name = np->name;
> + init.ops = &berlin_pll_ops;
> + parent_name = of_clk_get_parent_name(np, 0);
> + init.parent_names = &parent_name;
> + init.num_parents = 1;
> +
> + pll->hw.init = &init;
> +
> + clk = clk_register(NULL, &pll->hw);
> + if (WARN_ON(IS_ERR(clk)))
> + return;
> +
> + ret = of_clk_add_provider(np, of_clk_src_simple_get, clk);
> + if (WARN_ON(ret))
> + return;
> +}
> +
>
--
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