Hi Magnus,
(CC'ing Grant Likely)
On Wednesday 06 November 2013 17:19:48 Magnus Damm wrote:
> On Wed, Nov 6, 2013 at 8:47 AM, Laurent Pinchart wrote:
> > On Tuesday 05 November 2013 16:56:40 Magnus Damm wrote:
> >> On Tue, Oct 29, 2013 at 11:55 PM, Laurent Pinchart wrote:
> >> > The R8A7790 has several clocks that are too custom to be supported in a
> >> > generic driver. Those clocks can be divided in two categories:
> >> >
> >> > - Fixed rate clocks with multiplier and divisor set according to boot
> >> > mode configuration
> >> >
> >> > - Custom divider clocks with SoC-specific divider values
> >> >
> >> > This driver supports both.
> >> >
> >> > Signed-off-by: Laurent Pinchart
> >> > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> >> > ---
> >> >
> >> > +void __init r8a7790_clocks_init(u32 mode)
> >> > +{
> >> > + cpg_mode = mode;
> >> > +
> >> > + of_clk_init(NULL);
> >> > +}
> >>
> >> Hi Laurent,
> >>
> >> Thanks a lot for your efforts on this. In general I think it all looks
> >> good,
> >
> > Thank you.
> >
> >> I have however one question regarding the "probe" interface between the
> >> SoC code in arch/arm/mach-shmobile and drivers/clk. The code above in
> >> r8a7790_clocks_init() looks a bit spaghetti-style to me, perhaps it is
> >> possible to make it cleaner somehow?
> >
> > Isn't it just a loop ? :-) The clocks handled by this driver are
> > "special", they need to be initialized manually.
>
> Spaghetti-O's? =) I agree that some special handling is needed.
>
> >> I realize that you need some way to read out the mode pin setting, and
> >> that is currently provided by the SoC code in arch/arm/mach-shmobile/.
> >> Today it seems that you instead of letting the drivers/clk/ code call SoC
> >> code to get the mode pin setting (and add a SoC link dependency) you
> >> simply feed the settings to the clk code from the SoC code using the
> >> parameter to r8a7790_clocks_init(). This direction seems good to me. I'm
> >> however not too keen on the current symbol dependency in the "other"
> >> direction.
> >>
> >> If possible I'd like to keep the interface between the SoC code and the
> >> driver code to "standard" driver model functions. For instance, using
> >> of_clk_init(NULL) from the SoC code seems like a pretty good standard
> >> interface - without any link time dependencies. So with the current code,
> >> the r8a7790_clocks_init() function somehow goes against this no-symbol-
> >> dependency preference that I happen to have. =)
> >>
> >> Would it for instance be possible let the SoC code read out the mode pin
> >> setting, and then pass the current setting using the argument of
> >> of_clk_init() to select different dividers? That way the symbol
> >> dependency goes away. Or maybe it becomes too verbose?
> >
> > The of_clk_init() argument is an array of struct of_device_id that is used
> > by the clock core to match device tree nodes. I don't really see how you
> > would like to use it to pass the boot mode pins state to the driver.
>
> Yeah, it seems that interface isn't such a good match. Thanks.
>
> > There is currently no dedicated API (at least to my knowledge) to pass
> > clock configuration information between arch/arm and drivers/clk. Here's
> > a couple of methods that can be used.
> >
> > - Call a drivers/clk function from platform code with the clock
> > configuration information and store that information in a driver global
> > variable for later use (this is the method currently used by this patch).
> >
> > - Call a platform code function from driver code to read the
> > configuration. This is pretty similar to the above, with a dependency in
> > the other direction.
> >
> > - Read the value directly from the hardware in the clock driver. This
> > doesn't feel really right, as that's not the job of the clock driver. In
> > a more general case this solution might not always be possible, as
> > reading the configuration might require access to resources not available
> > to drivers.
> >
> > - Create a standard API for this purpose. It might be overengineering.
> >
> > - Use AUXDATA filled by platform code.
> >
> > There might be other solutions I haven't thought of. I went for the first
> > method as there's already 8 other clock drivers that expose an
> > initialization function to platform code. I might just have copied a
> > mistake though.
>
> Thanks for listing these. I have come across another option:
>
> Use of_update_property() and of_property_read_u32() to pass using a
> property, see below.
>
> > Mike, do you have an opinion on this ? In a nutshell, the code clocks are
> > fixed factor but their factor depend on a boot mode configuration. The
> > configuration value is thus needed when instantiating the clocks. It can
> > be read from a hardware register, outside of the clocks IP core.
>
> Yeah, I too would be interested to hear what Mike thinks about this matter.
>
> Please see below for a prototype patch on top of your code that shows
> the driver side of my DT property proposal. The omitted SoC side
> becomes slightly more complicated but that code can be shared between
> multiple R-Car SoCs so I think it should be acceptable size-wise.
>
> With this patch applied the interface between the driver and the SoC
> code is a DT property (that needs documentation) and
> of_clk_init(NULL). No more evil link time symbol dependencies between
> arch/ and drivers/...
This feels a bit like a DT abuse to me. I'm not strongly opposed to this
approach, but I would need an ack from Mike and Grant before implementing it.
> drivers/clk/shmobile/clk-r8a7790.c | 15 ++++++---------
> include/linux/clk/shmobile.h | 19 -------------------
> 2 files changed, 6 insertions(+), 28 deletions(-)
>
> --- 0018/drivers/clk/shmobile/clk-r8a7790.c
> +++ work/drivers/clk/shmobile/clk-r8a7790.c 2013-11-06
> 16:59:59.000000000 +0 900
> @@ -12,7 +12,6 @@
>
> #include <linux/clk-provider.h>
> #include <linux/clkdev.h>
> -#include <linux/clk/shmobile.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/of.h>
> @@ -70,8 +69,6 @@ static const struct clk_div_table cpg_sd
> { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 },
> };
>
> -static u32 cpg_mode __initdata;
> -
> #define CPG_SDCKCR 0x00000074
>
> static void __init r8a7790_cpg_clocks_init(struct device_node *np)
> @ -80,6 +77,12 @@ static void __init r8a7790_cpg_clocks_in
> struct r8a7790_cpg *cpg;
> struct clk **clks;
> unsigned int i;
> + u32 cpg_mode;
> +
> + if (of_property_read_u32(node, "cpg-mode", &cpg_mode)) {
> + pr_err("%s: failed to determine CPG mode\n", __func__);
> + return;
> + }
>
> cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> @ -168,9 +171,3 @@ static void __init r8a7790_cpg_clocks_in
> CLK_OF_DECLARE(r8a7790_cpg_clks, "renesas,r8a7790-cpg-clocks",
> r8a7790_cpg_clocks_init);
>
> -void __init r8a7790_clocks_init(u32 mode)
> -{
> - cpg_mode = mode;
> -
> - of_clk_init(NULL);
> -}
> --- 0018/include/linux/clk/shmobile.h
> +++ /dev/null 2013-06-03 21:41:10.638032047 +0900
> @@ -1,19 +0,0 @@
> -/*
> - * Copyright 2013 Ideas On Board SPRL
> - *
> - * Contact: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - */
> -
> -#ifndef __LINUX_CLK_SHMOBILE_H_
> -#define __LINUX_CLK_SHMOBILE_H_
> -
> -#include <linux/types.h>
> -
> -void r8a7790_clocks_init(u32 mode);
> -
> -#endif
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
