Quoting Masahiro Yamada (2016-01-01 17:26:05)

Hi Michael,


2015-12-31 10:35 GMT+09:00 Michael Turquette [off-list ref]:

quoted

Hello Yamada-san,

Quoting Masahiro Yamada (2015-12-28 02:20:58)

quoted

diff --git a/drivers/clk/uniphier/Kconfig b/drivers/clk/uniphier/Kconfig
new file mode 100644
index 0000000..7606f27
--- /dev/null
+++ b/drivers/clk/uniphier/Kconfig

@@ -0,0 +1,35 @@
+menuconfig CLK_UNIPHIER
+       bool "Clock drivers for UniPhier SoCs"
+       depends on ARCH_UNIPHIER

Might want to make the above line:

        depends on ARCH_UNIPHIER || COMPILE_TEST

quoted

+       depends on OF
+       default y
+       help
+         Supports clock drivers for UniPhier SoCs.

Why menuconfig? Can we make this a non-visible config option selected by
the UniPhier platform?

Yes, I can do that if you like.

Thanks, and the other questions I asked in my previous reply are
relevant here: can you make these into platform_drivers and compile them
as loadable modules? That would be ideal.

Do you prefer to making it flat, like all clocks are shown in the
"Common Clock Framework" menu?

Or, is it better to categorize SoC clocks with "menu" for each SoC family?

The precedent is to do it flat. I'm fine with a menu though. Your
choice.

quoted

quoted

+
+if CLK_UNIPHIER

Please drop the if statement here and have the below options 'depends'
on CLK_UNIPHIER.

Why is it bad?

I think surrounding all the options with "if CLK_UNIPHIER" ... "endif"
is easier than adding "depends on CLK_UNIPHIER" to each of them.

I find it more readable to clearly define the dependencies for each
config option.

quoted

quoted

+
+config CLK_UNIPHIER_PH1_SLD3
+       bool "Clock driver for UniPhier PH1-sLD3 SoC"
+       default y

Can you make these drivers into loadable modules? If so you might
consider tristate.

For some, I can.
For some, I can't (because they must provide an input clock for the timer.)

quoted

quoted

+
+static void __init ph1_ld4_clk_init(struct device_node *np)
+{
+       uniphier_clk_init(np, ph1_ld4_clk_idata);
+}
+CLK_OF_DECLARE(ph1_ld4_clk, "socionext,ph1-ld4-sysctrl", ph1_ld4_clk_init);

Can you avoid using CLK_OF_DECLARE and instead make this into a
platform_driver? For examples please see drivers/clk/qcom*.c

For clk-uniphier-peri.c and clk-uniphier-mio.c, yes.
They provide clocks for peripheral devices like USB, MMC, etc., so I
can delay them.


For clk-ph1-*.c, I am afraid I can't.
They provide an input clock for the timer (ARM global timer).
I think they should be initialized very early.


Is platform_driver better than CLK_OF_DECLARE() where it is possible?

Yes.

I just chose CLK_OF_DECLARE() for consistency
and it seems the majority.

I'm trying to change that.

quoted

quoted

+
+static void __init uniphier_clk_update_parent_name(struct device_node *np,
+                                                  const char **parent_name)
+{
+       const char *new_name;
+       int index;
+
+       if (!parent_name || !*parent_name)
+               return;
+
+       if (strncmp(*parent_name, UNIPHIER_CLK_EXT, strlen(UNIPHIER_CLK_EXT)))
+               return;
+
+       index = of_property_match_string(np, "clock-names",
+                               *parent_name + strlen(UNIPHIER_CLK_EXT));
+       new_name = of_clk_get_parent_name(np, index);
+       if (new_name)
+               *parent_name = new_name;

Where can I find the DT binding description and DTS for this clock
controller?

Any update on the above question? I wasn't able to find the DT binding
description anywhere.

I'm confused why the parent name function is necessary and

quoted

I'd like to see the data.

clk_register() needs the name strings of parent clocks,
not pointers to parent clocks.
(It looks a bit odd to me, although it allows us to register clocks in
any order.
In stead, we have to think about orphan-walking.)

Actually, I'd like to allow a clk_register() that takes pointers to
struct clk_core for the parent. This is useful for registering clocks
whose parents are inside the same clock provider/generator/driver.

We might still need the clock parent names for registering parents across
the clock provider/generator/driver boundary. Or maybe not ... Since we
can use the phandle to fetch a struct clk_core for a clock that has
already been registered. But these are ideas for the future.

So, we need to exactly know the names of parent clocks.

Let's assume the clock system that consists of some hardware blocks.


  root_clk: root_clk {
        compatible = "foo-clk";
        reg = <0x10000000 0x1000>;
        #clock-cells = <1>;
  };

  bar_clk {
        compatible = "bar-clk";
        reg = <0x20000000 0x1000>;
        #clock-cells = <1>;
        clock-names = "input"
        clocks = <&root_clk 0>;
   };

  baz_clk {
        compatible = "baz-clk";
        reg = <0x30000000 0x1000>;
        #clock-cells = <1>;
        clock-names = "input"
        clocks = <&root_clk 1>;
   };



The "bar_clk" is a clock provider for other peripherals,
and also a consumer of "root_clk".

The "bar_clk" wants to know the name of index 0 of root_clk
because it is needed when calling clk_register().


Let's say the names of clocks provided root_clk are, "for-bar123", "for-baz456".

The "bar_clk" needs the string "for-bar123", but it does not know.
(Likewise, the "baz_clk" needs the string "for-baz456", but it does not know.)

Of course, I can hard-code "for-bar123" in the driver of "bar_clk",
but I think it is annoying to keep the name synced between the
provider and the consumer.

For on-chip clock providers and consumers it is usually safe to
hard-code the names since the clock configuration of the IC/SoC is
well-defined. For making board-level connections it is less safe.

Still, many clk drivers do indeed hard-code the string names for their
parent clocks.

So, I implemented uniphier_clk_update_parent_name()
to convert the clock-names into the parent clock name.
In this case, this function takes "input" and converts it into "for-bar123".


Is there any good idea to solve this problem?

Well, using the clock-output-names DT property in conjunction with
of_clk_get_parent_name() solves this for you in a common way. However I
would like to get rid of clock-output-names in the future. In general
I'm working slowy to reduce the reliance on clock string names, but it
is a long-term side project.

How bad is it for you to hard-code the string names in your driver? Are
the foo, bar and baz clock providers all on the same chip?

Regards,
Mike

-- 
Best Regards
Masahiro Yamada