lkml.org 
[lkml]   [2013]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC][PATCH 1/2] ARM: OMAP4: clock: Add device tree support for AUXCLKs
    * Nishanth Menon <nm@ti.com> [130410 10:44]:
    > Details in the patch below (Tony, I have added you as collaborator for
    > helping in getting this working-clk_add_alias was'nt needed in the
    > internal patch discussion we had - I have taken a bit of freedom in
    > adding your contributions to the patch below)

    OK thanks. Noticed few minor things, see below.

    > Folks, this does seem to be the best compromise we can achieve at this
    > point in time. feedback on this approach is much appreciated - if folks
    > are ok, I can post this as an formal patch series.
    >
    > From 130a41821bf57081ca45ef654029175d173135e6 Mon Sep 17 00:00:00 2001
    > From: Nishanth Menon <nm@ti.com>
    > Date: Tue, 9 Apr 2013 19:26:40 -0500
    > Subject: [RFC PATCH] clk: OMAP: introduce device tree binding to kernel clock
    > data
    >
    > OMAP clock data is located in arch/arm/mach-omap2/cclockXYZ_data.c.
    > However, this presents an obstacle for using these clock nodes in
    > Device Tree definitions. There are many possible approaches to this
    > problem as discussed in the following thread:
    > http://marc.info/?t=136370325600009&r=1&w=2

    It might be worth clarifying that this is especially for the board
    specific clocks initially. The fixed clocks are currently found via
    the clock aliases table.

    > Highlights of the options:
    > a) device specific clk_add_alias:
    > cons: driver handling required
    > b) using an generic clk node and indexing to reach the clock required.
    > This is similar in approach taken by tegra and few other platforms.
    > example clock = <&clk 5>;
    > cons: potential to have mismatches in indexed table and associated
    > dtb data. In addition, managing continued documentation in bindings
    > as clock indexing increases. Even though readability angle could be
    > improved by using preprocessing of DT using macros, indexed approach
    > is inherently risky from cases like the following:
    > clk indexes in kernel:
    > 1 - mpu_dpll
    > 2 - aux_clk1
    > 3 - core_clk
    > DT entry for peripheral x uses <&clk 2>, kernel updates to:
    > 1 - mpu_dpll
    > 2 - per_dpll
    > 3 - aux_clk1
    > 4 - core_clk
    > using the old dtb(or dts missing an update), on new kernel which has
    > updated indices will result in per_dpll now controlled for peripheral
    > X without warning or any potential error detection and warning.
    >
    > Even though we can claim this is user error, such errors are hard to
    > track down and fix.
    >
    > An alternate approach introduced here is to introduce device tree bindings
    > corresponding to the clock nodes required in DT definition for SoC which
    > automatically maps back to the definitions in cclockXYZ_data.c.
    >
    > The driver introduced here to do this mapping will eventually be the
    > place where the clock handling will migrate to. We need to consider this
    > angle as well so that the solution will be an valid transition point for
    > moving the clock data out of kernel image (into device tree or firmware load
    > etc..).
    >
    > Overall strategy introduced here is simple: an clock node described in
    > device tree blob is used to identify the exact clock provided in the SoC
    > specific data. This is then linked back using of_clk_add_provider to the
    > device node to be accessible by of_clk_get.
    >
    > Based on discussion contributions from Roger Quadros, Grygorii Strashko
    > and others.
    >
    > [tony@atomide.com: co-developed]
    > Signed-off-by: Tony Lindgren <tony@atomide.com>
    > Signed-off-by: Nishanth Menon <nm@ti.com>
    > ---
    > .../devicetree/bindings/clock/omap-clock.txt | 40 +++++++++
    > drivers/clk/Makefile | 1 +
    > drivers/clk/omap/Makefile | 1 +
    > drivers/clk/omap/clk.c | 94 ++++++++++++++++++++
    > 4 files changed, 136 insertions(+)
    > create mode 100644 Documentation/devicetree/bindings/clock/omap-clock.txt
    > create mode 100644 drivers/clk/omap/Makefile
    > create mode 100644 drivers/clk/omap/clk.c
    >
    > diff --git a/Documentation/devicetree/bindings/clock/omap-clock.txt b/Documentation/devicetree/bindings/clock/omap-clock.txt
    > new file mode 100644
    > index 0000000..07e3ff8
    > --- /dev/null
    > +++ b/Documentation/devicetree/bindings/clock/omap-clock.txt
    > @@ -0,0 +1,40 @@
    > +Device Tree Clock bindings for Texas Instrument's OMAP compatible platforms
    > +
    > +This binding is a work-in-progress, and meant to be stage #1 of transitioning
    > +OMAP clock data out of kernel image.

    As it's using the common clock binding, this will be supported in the
    long run too. So maybe replace work-in-progress with initial minimal
    binding that can be enhanced later on.

    > +This binding uses the common clock binding[1].
    > +
    > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
    > +
    > +Required properties:
    > +- compatible : shall be "ti,omap-clock"
    > +- #clock-cells : from common clock binding; shall be set to 0.
    > +NOTE:
    > +node name should map to clock database in arch/arm/mach-omap2/cclock<SoC>_data.c
    > +Since all clocks are described with _ck, the node name is optimized to drop the
    > +usage of _ck. For example, a clock called dpll1_ck will be defined as dpll1.
    > +
    > +Example #1: describing clock node for CPU on OMAP34xx platform:
    > +Ref: arch/arm/mach-omap2/cclock3xxx_data.c
    > +describes the CPU clock to be as follows
    > + CLK(NULL, "dpll1_ck", &dpll1_ck, CK_3XXX),
    > +Corresponding binding will be:
    > + dpll1: dpll1 {
    > + #clock-cells = <0>;
    > + compatible = "ti,omap-clock";
    > + };
    > +And it's usage will be:
    > + clocks = <&dpll1>;
    > +
    > +Example #2: describing clock node for auxilary clock #3 on OMAP443x platform:
    > +Ref: arch/arm/mach-omap2/cclock44xx_data.c
    > +describes the auxclk3 clock to be as follows:
    > + CLK(NULL, "auxclk3_ck", &auxclk3_ck, CK_443X),
    > +Corresponding binding will be:
    > + auxclk3: auxclk3 {
    > + #clock-cells = <1>;
    > + compatible = "ti,omap-clock";
    > + };
    > +And it's usage will be:
    > + clocks = <&auxclk3>;

    The #clock-cells in the auxclk3 example should also be 0 instead of 1
    AFAIK. We should only use #clock-cells = <1> when the same physical
    clock provides multiple outputs. I believe the auxclocks are all separate,
    but that needs to be verified.

    > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
    > index 300d477..a0209d7 100644
    > --- a/drivers/clk/Makefile
    > +++ b/drivers/clk/Makefile
    > @@ -27,6 +27,7 @@ obj-$(CONFIG_ARCH_U8500) += ux500/
    > obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
    > obj-$(CONFIG_ARCH_ZYNQ) += clk-zynq.o
    > obj-$(CONFIG_ARCH_TEGRA) += tegra/
    > +obj-$(CONFIG_ARCH_OMAP) += omap/
    >
    > obj-$(CONFIG_X86) += x86/
    >
    > diff --git a/drivers/clk/omap/Makefile b/drivers/clk/omap/Makefile
    > new file mode 100644
    > index 0000000..8195931
    > --- /dev/null
    > +++ b/drivers/clk/omap/Makefile
    > @@ -0,0 +1 @@
    > +obj-y += clk.o
    > diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c
    > new file mode 100644
    > index 0000000..63a4cce
    > --- /dev/null
    > +++ b/drivers/clk/omap/clk.c
    > @@ -0,0 +1,94 @@
    > +/*
    > + * Texas Instruments OMAP Clock driver
    > + *
    > + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
    > + * Nishanth Menon
    > + * Tony Lindgren <tony@atomide.com>
    > + *
    > + * This program is free software; you can redistribute it and/or modify
    > + * it under the terms of the GNU General Public License version 2 as
    > + * published by the Free Software Foundation.
    > + *
    > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
    > + * kind, whether express or implied; without even the implied warranty
    > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    > + * GNU General Public License for more details.
    > + */
    > +
    > +#include <linux/clkdev.h>
    > +#include <linux/clk-private.h>
    > +#include <linux/kernel.h>
    > +#include <linux/module.h>
    > +#include <linux/of_device.h>
    > +#include <linux/platform_device.h>
    > +#include <linux/string.h>
    > +
    > +static const struct of_device_id omap_clk_of_match[] = {
    > + {.compatible = "ti,omap-clock",},
    > + {},
    > +};
    > +
    > +MODULE_DEVICE_TABLE(of, omap_clk_of_match);
    > +
    > +/**
    > + * omap_clk_probe() - create link from DT definition to clock data
    > + * @pdev: device node
    > + *
    > + * REVISIT: We are now assuming the following:
    > + * 1. omap clock names end with _ck
    > + * 2. omap clocks don't get removed
    > + */
    > +static int omap_clk_probe(struct platform_device *pdev)
    > +{
    > + struct clk *clk;
    > + int res;
    > +
    > + const struct of_device_id *match;
    > + struct device_node *np = pdev->dev.of_node;
    > + char clk_name[32];
    > +
    > + match = of_match_device(omap_clk_of_match, &pdev->dev);
    > +
    > + /* Set up things so consumer can call clk_get() with name */
    > + snprintf(clk_name, 32, "%s_ck", np->name);
    > + clk = clk_get(NULL, clk_name);
    > + if (IS_ERR(clk)) {
    > + res = PTR_ERR(clk);
    > + dev_err(&pdev->dev, "could not get clock %s (%d)\n",
    > + clk_name, res);
    > + goto out;
    > + }

    It seems that at least for now we can assume the naming will stay
    that way, then if we need other rules for finding clocks, we can
    add omap_match_clock() function or something.

    > + /* This allows the driver to of_clk_get() */
    > + res = of_clk_add_provider(np, of_clk_src_simple_get, clk);
    > + if (res)
    > + dev_err(&pdev->dev, "could not add provider for %s (%d)\n",
    > + clk_name, res);
    > +
    > + clk_put(clk);
    > +out:
    > + return res;
    > +}

    We can avoid the concern of storing the struct clk * and do the
    look up lazily on consumer driver probe by setting a dummy struct
    clk * here. Then replace of_clk_src_simple_get() with a custom
    omap_clk_src_get() that does the lookup and replaces the struct
    clk * with the real one.

    > +MODULE_ALIAS("platform:omap_clk");
    > +
    > +/* We assume here that OMAP clocks will not be removed */
    > +static struct platform_driver omap_clk_driver = {
    > + .probe = omap_clk_probe,
    > + .driver = {
    > + .name = "omap_clk",
    > + .owner = THIS_MODULE,
    > + .of_match_table = of_match_ptr(omap_clk_of_match),
    > + },
    > +};
    > +
    > +static int __init omap_clk_init(void)
    > +{
    > + return platform_driver_register(&omap_clk_driver);
    > +}
    > +
    > +arch_initcall(omap_clk_init);
    >
    > +MODULE_DESCRIPTION("OMAP Clock driver");
    > +MODULE_AUTHOR("Texas Instruments Inc.");
    > +MODULE_LICENSE("GPL v2");

    Regards,

    Tony


    \
     
     \ /
      Last update: 2013-04-10 21:21    [W:5.168 / U:0.216 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site