Maybe this is CCF bug

Maybe this is CCF bug

[Kuninori Morimoto]

Hi Laurent, Magnus, and All

Now, I'm working for sound DT support,
and I noticed common clock setting's strange behavior.
I guess this is bug, but 50% my misunderstanding.

Now, we have clock index on ${LINUX}/include/dt-bindings/clock/r8a7790-clock.h
For example, r8a7790's MSTP9 case, like this

	/* MSTP9 */
	#define R8A7790_CLK_GPIO5		7
	#define R8A7790_CLK_GPIO4		8
	#define R8A7790_CLK_GPIO3		9
	#define R8A7790_CLK_GPIO2		10
	#define R8A7790_CLK_GPIO1		11
	#define R8A7790_CLK_GPIO0		12
	#define R8A7790_CLK_RCAN1		15
	#define R8A7790_CLK_RCAN0		16
	#define R8A7790_CLK_QSPI_MOD		17
	#define R8A7790_CLK_IICDVFS		26
	#define R8A7790_CLK_I2C3		28
	#define R8A7790_CLK_I2C2		29
	#define R8A7790_CLK_I2C1		30
	#define R8A7790_CLK_I2C0		31

and MSTP9 is like this

	mstp9_clks: mstp9_clks@e6150994 {
		compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks";
		reg = <0 0xe6150994 0 4>, <0 0xe61509a4 0 4>;
		clocks = <&p_clk>, <&p_clk>, <&cpg_clocks R8A7790_CLK_QSPI>,
			 <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>;
		#clock-cells = <1>;
		renesas,clock-indices = <
			R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0 R8A7790_CLK_QSPI_MOD
			R8A7790_CLK_I2C3 R8A7790_CLK_I2C2 R8A7790_CLK_I2C1
			R8A7790_CLK_I2C0
		>;
		clock-output-names 			"rcan1", "rcan0", "qspi_mod", "i2c3", "i2c2", "i2c1", "i2c0";
	};

And, now, spi parent is MSTP9 QSPI MOD

	spi: spi@e6b10000 {
		...
		clocks = <&mstp9_clks R8A7790_CLK_QSPI_MOD>;
		...
	};

This SPI would like to use MSTP9's 17th (= R8A7790_CLK_QSPI_MOD) clock as its parent.
But, mstp9_clks has 7 clocks only.
R8A7790_CLK_xxx means "bit shift", not "index" on DT clock.

On ${LINUX}/drivers/clk/shmobile/clk-mstp.c,
it try to get parent name by

	parent_name = of_clk_get_parent_name(np, i);

and it returns "mstp9_clks" in this case.
Maybe SPI would like to get "qspi_mod" ?

What do you think ?
Is this cause of pm_runtime issue ?

Best regards
---
Kuninori Morimoto

Re: Maybe this is CCF bug

[Geert Uytterhoeven]

Hi Morimoto-san,

On Fri, Feb 21, 2014 at 9:58 AM, Kuninori Morimoto
<kuninori.morimoto.gx@gmail.com> wrote:
> Now, I'm working for sound DT support,
> and I noticed common clock setting's strange behavior.
> I guess this is bug, but 50% my misunderstanding.
>
> Now, we have clock index on ${LINUX}/include/dt-bindings/clock/r8a7790-clock.h
> For example, r8a7790's MSTP9 case, like this
>
>         /* MSTP9 */
>         #define R8A7790_CLK_GPIO5               7
>         #define R8A7790_CLK_GPIO4               8
>         #define R8A7790_CLK_GPIO3               9
>         #define R8A7790_CLK_GPIO2               10
>         #define R8A7790_CLK_GPIO1               11
>         #define R8A7790_CLK_GPIO0               12
>         #define R8A7790_CLK_RCAN1               15
>         #define R8A7790_CLK_RCAN0               16
>         #define R8A7790_CLK_QSPI_MOD            17
>         #define R8A7790_CLK_IICDVFS             26
>         #define R8A7790_CLK_I2C3                28
>         #define R8A7790_CLK_I2C2                29
>         #define R8A7790_CLK_I2C1                30
>         #define R8A7790_CLK_I2C0                31
>
> and MSTP9 is like this
>
>         mstp9_clks: mstp9_clks@e6150994 {
>                 compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks";
>                 reg = <0 0xe6150994 0 4>, <0 0xe61509a4 0 4>;
>                 clocks = <&p_clk>, <&p_clk>, <&cpg_clocks R8A7790_CLK_QSPI>,
>                          <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>;
>                 #clock-cells = <1>;
>                 renesas,clock-indices = <
>                         R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0 R8A7790_CLK_QSPI_MOD
>                         R8A7790_CLK_I2C3 R8A7790_CLK_I2C2 R8A7790_CLK_I2C1
>                         R8A7790_CLK_I2C0
>                 >;
>                 clock-output-names >                         "rcan1", "rcan0", "qspi_mod", "i2c3", "i2c2", "i2c1", "i2c0";
>         };
>
> And, now, spi parent is MSTP9 QSPI MOD
>
>         spi: spi@e6b10000 {
>                 ...
>                 clocks = <&mstp9_clks R8A7790_CLK_QSPI_MOD>;
>                 ...
>         };
>
> This SPI would like to use MSTP9's 17th (= R8A7790_CLK_QSPI_MOD) clock as its parent.
> But, mstp9_clks has 7 clocks only.
> R8A7790_CLK_xxx means "bit shift", not "index" on DT clock.

Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt says:

    The MSTP groups are sparsely populated. Unimplemented
    gate clocks must not be declared.

> On ${LINUX}/drivers/clk/shmobile/clk-mstp.c,
> it try to get parent name by
>
>         parent_name = of_clk_get_parent_name(np, i);
>
> and it returns "mstp9_clks" in this case.
> Maybe SPI would like to get "qspi_mod" ?

I don't know about of_clk_get_parent_name(), but in the end the QSPI driver
does seem to get the right clock, cfr. this output on Lager reference:

clk qspi_mod at 97500000 Hz
clk qspi at 97500000 Hz
clk pll1_div2 at 780000000 Hz
clk pll1 at 1560000000 Hz
clk main at 20000000 Hz
clk extal at 20000000 Hz

with the (whitespace-damaged) patch at the end of this email.

BTW, you do have these 2 applied?

    clk: shmobile: rcar-gen2: Fix clock parent all non-PLL clocks
    clk: shmobile: rcar-gen2: Fix qspi divisor

They are not in Simon's repository, but in
http://git.linaro.org/people/mike.turquette/linux.git/shortlog/refs/heads/clk-fixes

diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 239bd4206fc6..5ae0cdf96231 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -22,6 +22,11 @@
  *
  */

+#ifdef CONFIG_COMMON_CLK
+#include <linux/clk-private.h>
+#else
+#include <linux/sh_clk.h>
+#endif
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
@@ -1311,6 +1316,18 @@ static int rspi_probe(struct platform_device *pdev)
                goto error1;
        }

+{
+    struct clk *clk = rspi->clk;
+
+    while (clk) {
+#ifdef CONFIG_COMMON_CLK
+       printk("clk %s at %lu Hz\n", clk->name, clk_get_rate(clk));
+#else
+       printk("clk at %lu Hz\n", clk_get_rate(clk));
+#endif
+       clk = clk->parent;
+    }
+}
        init_waitqueue_head(&rspi->wait);

        master->bus_num = pdev->id;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Maybe this is CCF bug

[Laurent Pinchart]

Hi Morimoto-san,

(CC'ing Mike and Ben)

On Friday 21 February 2014 11:20:06 Geert Uytterhoeven wrote:
> On Fri, Feb 21, 2014 at 9:58 AM, Kuninori Morimoto wrote:
> > Now, I'm working for sound DT support,
> > and I noticed common clock setting's strange behavior.
> > I guess this is bug, but 50% my misunderstanding.
> > 
> > Now, we have clock index on
> > ${LINUX}/include/dt-bindings/clock/r8a7790-clock.h For example, r8a7790's
> > MSTP9 case, like this
> > 
> >         /* MSTP9 */
> >         #define R8A7790_CLK_GPIO5               7
> >         #define R8A7790_CLK_GPIO4               8
> >         #define R8A7790_CLK_GPIO3               9
> >         #define R8A7790_CLK_GPIO2               10
> >         #define R8A7790_CLK_GPIO1               11
> >         #define R8A7790_CLK_GPIO0               12
> >         #define R8A7790_CLK_RCAN1               15
> >         #define R8A7790_CLK_RCAN0               16
> >         #define R8A7790_CLK_QSPI_MOD            17
> >         #define R8A7790_CLK_IICDVFS             26
> >         #define R8A7790_CLK_I2C3                28
> >         #define R8A7790_CLK_I2C2                29
> >         #define R8A7790_CLK_I2C1                30
> >         #define R8A7790_CLK_I2C0                31
> > 
> > and MSTP9 is like this
> > 
> >         mstp9_clks: mstp9_clks@e6150994 {
> >                 compatible = "renesas,r8a7790-mstp-clocks",
> >                 "renesas,cpg-mstp-clocks";
> >                 reg = <0 0xe6150994 0 4>, <0 0xe61509a4 0 4>;
> >                 clocks = <&p_clk>, <&p_clk>, <&cpg_clocks
> >                 R8A7790_CLK_QSPI>,
> >                          <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>;
> >                 
> >                 #clock-cells = <1>;
> >                 renesas,clock-indices = <
> >                         R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0
> >                         R8A7790_CLK_QSPI_MOD
> >                         R8A7790_CLK_I2C3 R8A7790_CLK_I2C2 R8A7790_CLK_I2C1
> >                         R8A7790_CLK_I2C0
> >                 >;
> >                 
> >                 clock-output-names > >                         "rcan1", "rcan0", "qspi_mod", "i2c3", "i2c2",
> >                         "i2c1", "i2c0";
> >         };
> > 
> > And, now, spi parent is MSTP9 QSPI MOD
> > 
> >         spi: spi@e6b10000 {
> >                 ...
> >                 clocks = <&mstp9_clks R8A7790_CLK_QSPI_MOD>;
> >                 ...
> >         };
> > 
> > This SPI would like to use MSTP9's 17th (= R8A7790_CLK_QSPI_MOD) clock as
> > its parent. But, mstp9_clks has 7 clocks only.
> > R8A7790_CLK_xxx means "bit shift", not "index" on DT clock.
> 
> Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt says:
> 
>     The MSTP groups are sparsely populated. Unimplemented
>     gate clocks must not be declared.
> 
> > On ${LINUX}/drivers/clk/shmobile/clk-mstp.c,
> > it try to get parent name by
> > 
> >         parent_name = of_clk_get_parent_name(np, i);
> > 
> > and it returns "mstp9_clks" in this case.
> > Maybe SPI would like to get "qspi_mod" ?

I wouldn't call that a bug in CCF, but it's definitely a non-intuitive 
behaviour. CCF lets OF clock providers implement their own method to translate 
clock specifiers into clocks (see the second and third arguments passed to 
of_clk_add_provider). In practice the vast majority (if not all) of the 
drivers implementing support for multiple clocks use an index as their first 
clock cell. That index can be a direct index into the list of clocks provided 
by the CCF device, but doesn't have to be. In the case of the clk-mstp driver 
the first clock cell represents the clock hardware index, which is different 
than the index into the software list of clocks as clocks are sparsely 
populated.

The of_clk_get_parent_name() function behaves differently. It first looks up 
the parent clock node in DT and parses the clock specifier cells, and then 
uses the first cell as a direct index into the clock-names property of the 
parent clock node. This bypasses the parent clock driver clock lookup 
mechanism, and thus leads to confusion as the meaning of the clock specifier 
in DT will depend on whether you're referencing a clock from an end-user 
driver (which will in that case use clk_get() and go through the clock 
provider driver for clock lookup), or from another clock provider (which will 
in that case call of_clk_get_parent_name() and use direct index lookup). This 
has bitten me several weeks ago when I tried to add SSI clocks to DT. With the 
MSTP indices defined as

/* MSTP10 */
#define R8A7790_CLK_SSI                5
#define R8A7790_CLK_SSI9               6
#define R8A7790_CLK_SSI8               7
#define R8A7790_CLK_SSI7               8
#define R8A7790_CLK_SSI6               9
#define R8A7790_CLK_SSI5               10
#define R8A7790_CLK_SSI4               11
#define R8A7790_CLK_SSI3               12
#define R8A7790_CLK_SSI2               13
#define R8A7790_CLK_SSI1               14
#define R8A7790_CLK_SSI0               15

my naive approach was

mstp10_clks: mstp10_clks@e6150998 {
        compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks";
        reg = <0 0xe6150998 0 4>, <0 0xe61509a8 0 4>;
        clocks = <&p_clk>, <&mstp10_clks R8A7790_CLK_SSI>,
                 <&mstp10_clks R8A7790_CLK_SSI>,
                 <&mstp10_clks R8A7790_CLK_SSI>,
                 <&mstp10_clks R8A7790_CLK_SSI>,
                 <&mstp10_clks R8A7790_CLK_SSI>,
                 <&mstp10_clks R8A7790_CLK_SSI>,
                 <&mstp10_clks R8A7790_CLK_SSI>,
                 <&mstp10_clks R8A7790_CLK_SSI>,
                 <&mstp10_clks R8A7790_CLK_SSI>,
                 <&mstp10_clks R8A7790_CLK_SSI>;
        #clock-cells = <1>;
        renesas,clock-indices = <
                R8A7790_CLK_SSI R8A7790_CLK_SSI9 R8A7790_CLK_SSI8
                R8A7790_CLK_SSI7 R8A7790_CLK_SSI6 R8A7790_CLK_SSI5
                R8A7790_CLK_SSI4 R8A7790_CLK_SSI3 R8A7790_CLK_SSI2
                R8A7790_CLK_SSI1 R8A7790_CLK_SSI0
        >;
        clock-output-names                 "ssi", "ssi9", "ssi8", "ssi7", "ssi6", "ssi5",
                "ssi4", "ssi3", "ssi2", "ssi1", "ssi0";
}

However, this resulted in all SSI clocks but the master one referencing "ssi5" 
as their parent instead of "ssi".

The simple fix was to change the parent clocks to

        clocks = <&p_clk>, <&mstp10_clks 0>, <&mstp10_clks 0>,
                 <&mstp10_clks 0>, <&mstp10_clks 0>, <&mstp10_clks 0>,
                 <&mstp10_clks 0>, <&mstp10_clks 0>, <&mstp10_clks 0>,
                 <&mstp10_clks 0>, <&mstp10_clks 0>;

I understand this difference in behaviour is necessary, as the parent clock 
device might not have been probed yet when a child clock driver looks up the 
parent clock name. We thus can't rely on the parent clock driver to perform 
the clock specifier to clock translation.

Another approach to fix the problem was proposed by Ben Dooks in his "[PATCH 
1/3] clk: add clock-indices support" patch series was to standardize the 
reneses,clock-indices property into a clock-indices property, usable by 
of_clk_get_parent_name() without requiring the parent clock driver to have 
probed the device yet. That's more complex but would have the added benefit of 
making the clock specifier translation consistent, at least in this case. I'm 
not sure whether we'll always be able to achieve consistency as some exotic 
clock providers might require a really weird translation mechanism.

> I don't know about of_clk_get_parent_name(), but in the end the QSPI driver
> does seem to get the right clock, cfr. this output on Lager reference:
> 
> clk qspi_mod at 97500000 Hz
> clk qspi at 97500000 Hz
> clk pll1_div2 at 780000000 Hz
> clk pll1 at 1560000000 Hz
> clk main at 20000000 Hz
> clk extal at 20000000 Hz
> 
> with the (whitespace-damaged) patch at the end of this email.
> 
> BTW, you do have these 2 applied?
> 
>     clk: shmobile: rcar-gen2: Fix clock parent all non-PLL clocks
>     clk: shmobile: rcar-gen2: Fix qspi divisor
> 
> They are not in Simon's repository, but in
> http://git.linaro.org/people/mike.turquette/linux.git/shortlog/refs/heads/cl
> k-fixes
> 
> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
> index 239bd4206fc6..5ae0cdf96231 100644
> --- a/drivers/spi/spi-rspi.c
> +++ b/drivers/spi/spi-rspi.c
> @@ -22,6 +22,11 @@
>   *
>   */
> 
> +#ifdef CONFIG_COMMON_CLK
> +#include <linux/clk-private.h>
> +#else
> +#include <linux/sh_clk.h>
> +#endif
>  #include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/sched.h>
> @@ -1311,6 +1316,18 @@ static int rspi_probe(struct platform_device *pdev)
>                 goto error1;
>         }
> 
> +{
> +    struct clk *clk = rspi->clk;
> +
> +    while (clk) {
> +#ifdef CONFIG_COMMON_CLK
> +       printk("clk %s at %lu Hz\n", clk->name, clk_get_rate(clk));
> +#else
> +       printk("clk at %lu Hz\n", clk_get_rate(clk));
> +#endif
> +       clk = clk->parent;
> +    }
> +}
>         init_waitqueue_head(&rspi->wait);
> 
>         master->bus_num = pdev->id;

-- 
Regards,

Laurent Pinchart

Re: Maybe this is CCF bug

[Ben Dooks]

On 21/02/14 13:30, Laurent Pinchart wrote:
> Hi Morimoto-san,
>
> (CC'ing Mike and Ben)
>
> On Friday 21 February 2014 11:20:06 Geert Uytterhoeven wrote:
>> On Fri, Feb 21, 2014 at 9:58 AM, Kuninori Morimoto wrote:
>>> Now, I'm working for sound DT support,
>>> and I noticed common clock setting's strange behavior.
>>> I guess this is bug, but 50% my misunderstanding.
>>>
>>> Now, we have clock index on
>>> ${LINUX}/include/dt-bindings/clock/r8a7790-clock.h For example, r8a7790's
>>> MSTP9 case, like this
>>>
>>>          /* MSTP9 */
>>>          #define R8A7790_CLK_GPIO5               7
>>>          #define R8A7790_CLK_GPIO4               8
>>>          #define R8A7790_CLK_GPIO3               9
>>>          #define R8A7790_CLK_GPIO2               10
>>>          #define R8A7790_CLK_GPIO1               11
>>>          #define R8A7790_CLK_GPIO0               12
>>>          #define R8A7790_CLK_RCAN1               15
>>>          #define R8A7790_CLK_RCAN0               16
>>>          #define R8A7790_CLK_QSPI_MOD            17
>>>          #define R8A7790_CLK_IICDVFS             26
>>>          #define R8A7790_CLK_I2C3                28
>>>          #define R8A7790_CLK_I2C2                29
>>>          #define R8A7790_CLK_I2C1                30
>>>          #define R8A7790_CLK_I2C0                31
>>>
>>> and MSTP9 is like this
>>>
>>>          mstp9_clks: mstp9_clks@e6150994 {
>>>                  compatible = "renesas,r8a7790-mstp-clocks",
>>>                  "renesas,cpg-mstp-clocks";
>>>                  reg = <0 0xe6150994 0 4>, <0 0xe61509a4 0 4>;
>>>                  clocks = <&p_clk>, <&p_clk>, <&cpg_clocks
>>>                  R8A7790_CLK_QSPI>,
>>>                           <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>;
>>>
>>>                  #clock-cells = <1>;
>>>                  renesas,clock-indices = <
>>>                          R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0
>>>                          R8A7790_CLK_QSPI_MOD
>>>                          R8A7790_CLK_I2C3 R8A7790_CLK_I2C2 R8A7790_CLK_I2C1
>>>                          R8A7790_CLK_I2C0
>>>                  >;
>>>
>>>                  clock-output-names >>>                          "rcan1", "rcan0", "qspi_mod", "i2c3", "i2c2",
>>>                          "i2c1", "i2c0";
>>>          };
>>>
>>> And, now, spi parent is MSTP9 QSPI MOD
>>>
>>>          spi: spi@e6b10000 {
>>>                  ...
>>>                  clocks = <&mstp9_clks R8A7790_CLK_QSPI_MOD>;
>>>                  ...
>>>          };
>>>
>>> This SPI would like to use MSTP9's 17th (= R8A7790_CLK_QSPI_MOD) clock as
>>> its parent. But, mstp9_clks has 7 clocks only.
>>> R8A7790_CLK_xxx means "bit shift", not "index" on DT clock.
>>
>> Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt says:
>>
>>      The MSTP groups are sparsely populated. Unimplemented
>>      gate clocks must not be declared.
>>
>>> On ${LINUX}/drivers/clk/shmobile/clk-mstp.c,
>>> it try to get parent name by
>>>
>>>          parent_name = of_clk_get_parent_name(np, i);
>>>
>>> and it returns "mstp9_clks" in this case.
>>> Maybe SPI would like to get "qspi_mod" ?
>
> I wouldn't call that a bug in CCF, but it's definitely a non-intuitive
> behaviour. CCF lets OF clock providers implement their own method to translate
> clock specifiers into clocks (see the second and third arguments passed to
> of_clk_add_provider). In practice the vast majority (if not all) of the
> drivers implementing support for multiple clocks use an index as their first
> clock cell. That index can be a direct index into the list of clocks provided
> by the CCF device, but doesn't have to be. In the case of the clk-mstp driver
> the first clock cell represents the clock hardware index, which is different
> than the index into the software list of clocks as clocks are sparsely
> populated.
>
> The of_clk_get_parent_name() function behaves differently. It first looks up
> the parent clock node in DT and parses the clock specifier cells, and then
> uses the first cell as a direct index into the clock-names property of the
> parent clock node. This bypasses the parent clock driver clock lookup
> mechanism, and thus leads to confusion as the meaning of the clock specifier
> in DT will depend on whether you're referencing a clock from an end-user
> driver (which will in that case use clk_get() and go through the clock
> provider driver for clock lookup), or from another clock provider (which will
> in that case call of_clk_get_parent_name() and use direct index lookup). This
> has bitten me several weeks ago when I tried to add SSI clocks to DT. With the
> MSTP indices defined as
>
> /* MSTP10 */
> #define R8A7790_CLK_SSI                5
> #define R8A7790_CLK_SSI9               6
> #define R8A7790_CLK_SSI8               7
> #define R8A7790_CLK_SSI7               8
> #define R8A7790_CLK_SSI6               9
> #define R8A7790_CLK_SSI5               10
> #define R8A7790_CLK_SSI4               11
> #define R8A7790_CLK_SSI3               12
> #define R8A7790_CLK_SSI2               13
> #define R8A7790_CLK_SSI1               14
> #define R8A7790_CLK_SSI0               15
>
> my naive approach was
>
> mstp10_clks: mstp10_clks@e6150998 {
>          compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks";
>          reg = <0 0xe6150998 0 4>, <0 0xe61509a8 0 4>;
>          clocks = <&p_clk>, <&mstp10_clks R8A7790_CLK_SSI>,
>                   <&mstp10_clks R8A7790_CLK_SSI>,
>                   <&mstp10_clks R8A7790_CLK_SSI>,
>                   <&mstp10_clks R8A7790_CLK_SSI>,
>                   <&mstp10_clks R8A7790_CLK_SSI>,
>                   <&mstp10_clks R8A7790_CLK_SSI>,
>                   <&mstp10_clks R8A7790_CLK_SSI>,
>                   <&mstp10_clks R8A7790_CLK_SSI>,
>                   <&mstp10_clks R8A7790_CLK_SSI>,
>                   <&mstp10_clks R8A7790_CLK_SSI>;
>          #clock-cells = <1>;
>          renesas,clock-indices = <
>                  R8A7790_CLK_SSI R8A7790_CLK_SSI9 R8A7790_CLK_SSI8
>                  R8A7790_CLK_SSI7 R8A7790_CLK_SSI6 R8A7790_CLK_SSI5
>                  R8A7790_CLK_SSI4 R8A7790_CLK_SSI3 R8A7790_CLK_SSI2
>                  R8A7790_CLK_SSI1 R8A7790_CLK_SSI0
>          >;
>          clock-output-names >                  "ssi", "ssi9", "ssi8", "ssi7", "ssi6", "ssi5",
>                  "ssi4", "ssi3", "ssi2", "ssi1", "ssi0";
> }
>
> However, this resulted in all SSI clocks but the master one referencing "ssi5"
> as their parent instead of "ssi".
>
> The simple fix was to change the parent clocks to
>
>          clocks = <&p_clk>, <&mstp10_clks 0>, <&mstp10_clks 0>,
>                   <&mstp10_clks 0>, <&mstp10_clks 0>, <&mstp10_clks 0>,
>                   <&mstp10_clks 0>, <&mstp10_clks 0>, <&mstp10_clks 0>,
>                   <&mstp10_clks 0>, <&mstp10_clks 0>;
>
> I understand this difference in behaviour is necessary, as the parent clock
> device might not have been probed yet when a child clock driver looks up the
> parent clock name. We thus can't rely on the parent clock driver to perform
> the clock specifier to clock translation.

In this case the problem is worse as even if we could defer clock
lookups until the driver had been probed, we have a case where this
is self-referential.

> Another approach to fix the problem was proposed by Ben Dooks in his "[PATCH
> 1/3] clk: add clock-indices support" patch series was to standardize the
> reneses,clock-indices property into a clock-indices property, usable by
> of_clk_get_parent_name() without requiring the parent clock driver to have
> probed the device yet. That's more complex but would have the added benefit of
> making the clock specifier translation consistent, at least in this case. I'm
> not sure whether we'll always be able to achieve consistency as some exotic
> clock providers might require a really weird translation mechanism.

I think there is a couple of issues here, the first is that 
of_clk_get_parent_name() exists at-all. It would be nicer just
to be able to pass a set of 'struct clk *' to the clock registration
code. Although this may also still have an issue where we cannot
use self-referential clocks (I added a hack to allow the mstp driver
to use sub-nodes for each mstp clock group to get around that as
a first implementation)

I have not seen any comments on my clock-indices patch yet, I wonder
if anyone has had a chance to review it.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: Maybe this is CCF bug

[Ben Dooks]

On 21/02/14 08:58, Kuninori Morimoto wrote:
>
> Hi Laurent, Magnus, and All
>
> Now, I'm working for sound DT support,
> and I noticed common clock setting's strange behavior.
> I guess this is bug, but 50% my misunderstanding.

[snip]

I ran into this a couple of weeks back and reported it
to the relevant people. See here for my proposed fix:

http://www.spinics.net/lists/linux-sh/msg28537.html
http://www.spinics.net/lists/linux-sh/msg28536.html
http://www.spinics.net/lists/linux-sh/msg28535.html

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: Maybe this is CCF bug

[Laurent Pinchart]

Hi Ben,

On Friday 21 February 2014 13:42:10 Ben Dooks wrote:
> On 21/02/14 13:30, Laurent Pinchart wrote:
> > Hi Morimoto-san,
> > 
> > (CC'ing Mike and Ben)
> > 
> > On Friday 21 February 2014 11:20:06 Geert Uytterhoeven wrote:
> >> On Fri, Feb 21, 2014 at 9:58 AM, Kuninori Morimoto wrote:
> >>> Now, I'm working for sound DT support,
> >>> and I noticed common clock setting's strange behavior.
> >>> I guess this is bug, but 50% my misunderstanding.
> >>> 
> >>> Now, we have clock index on
> >>> ${LINUX}/include/dt-bindings/clock/r8a7790-clock.h For example,
> >>> r8a7790's
> >>> MSTP9 case, like this
> >>> 
> >>>          /* MSTP9 */
> >>>          #define R8A7790_CLK_GPIO5               7
> >>>          #define R8A7790_CLK_GPIO4               8
> >>>          #define R8A7790_CLK_GPIO3               9
> >>>          #define R8A7790_CLK_GPIO2               10
> >>>          #define R8A7790_CLK_GPIO1               11
> >>>          #define R8A7790_CLK_GPIO0               12
> >>>          #define R8A7790_CLK_RCAN1               15
> >>>          #define R8A7790_CLK_RCAN0               16
> >>>          #define R8A7790_CLK_QSPI_MOD            17
> >>>          #define R8A7790_CLK_IICDVFS             26
> >>>          #define R8A7790_CLK_I2C3                28
> >>>          #define R8A7790_CLK_I2C2                29
> >>>          #define R8A7790_CLK_I2C1                30
> >>>          #define R8A7790_CLK_I2C0                31
> >>> 
> >>> and MSTP9 is like this
> >>> 
> >>>          mstp9_clks: mstp9_clks@e6150994 {
> >>>          
> >>>                  compatible = "renesas,r8a7790-mstp-clocks",
> >>>                  "renesas,cpg-mstp-clocks";
> >>>                  reg = <0 0xe6150994 0 4>, <0 0xe61509a4 0 4>;
> >>>                  clocks = <&p_clk>, <&p_clk>, <&cpg_clocks
> >>>                  R8A7790_CLK_QSPI>,
> >>>                           <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>;
> >>>                  
> >>>                  #clock-cells = <1>;
> >>>                  renesas,clock-indices = <
> >>>                          R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0
> >>>                          R8A7790_CLK_QSPI_MOD
> >>>                          R8A7790_CLK_I2C3 R8A7790_CLK_I2C2
> >>>                          R8A7790_CLK_I2C1
> >>>                          R8A7790_CLK_I2C0
> >>>                  >;
> >>>                  
> >>>                  clock-output-names > >>>                          "rcan1", "rcan0", "qspi_mod", "i2c3", "i2c2",
> >>>                          "i2c1", "i2c0";
> >>>          };
> >>> 
> >>> And, now, spi parent is MSTP9 QSPI MOD
> >>> 
> >>>          spi: spi@e6b10000 {
> >>>                  ...
> >>>                  clocks = <&mstp9_clks R8A7790_CLK_QSPI_MOD>;
> >>>                  ...
> >>>          };
> >>> 
> >>> This SPI would like to use MSTP9's 17th (= R8A7790_CLK_QSPI_MOD) clock
> >>> as its parent. But, mstp9_clks has 7 clocks only.
> >>> R8A7790_CLK_xxx means "bit shift", not "index" on DT clock.
> >> 
> >> Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt says:
> >>      The MSTP groups are sparsely populated. Unimplemented
> >>      gate clocks must not be declared.
> >>> 
> >>> On ${LINUX}/drivers/clk/shmobile/clk-mstp.c,
> >>> it try to get parent name by
> >>> 
> >>>          parent_name = of_clk_get_parent_name(np, i);
> >>> 
> >>> and it returns "mstp9_clks" in this case.
> >>> Maybe SPI would like to get "qspi_mod" ?
> > 
> > I wouldn't call that a bug in CCF, but it's definitely a non-intuitive
> > behaviour. CCF lets OF clock providers implement their own method to
> > translate clock specifiers into clocks (see the second and third
> > arguments passed to of_clk_add_provider). In practice the vast majority
> > (if not all) of the drivers implementing support for multiple clocks use
> > an index as their first clock cell. That index can be a direct index into
> > the list of clocks provided by the CCF device, but doesn't have to be. In
> > the case of the clk-mstp driver the first clock cell represents the clock
> > hardware index, which is different than the index into the software list
> > of clocks as clocks are sparsely populated.
> > 
> > The of_clk_get_parent_name() function behaves differently. It first looks
> > up the parent clock node in DT and parses the clock specifier cells, and
> > then uses the first cell as a direct index into the clock-names property
> > of the parent clock node. This bypasses the parent clock driver clock
> > lookup mechanism, and thus leads to confusion as the meaning of the clock
> > specifier in DT will depend on whether you're referencing a clock from an
> > end-user driver (which will in that case use clk_get() and go through the
> > clock provider driver for clock lookup), or from another clock provider
> > (which will in that case call of_clk_get_parent_name() and use direct
> > index lookup). This has bitten me several weeks ago when I tried to add
> > SSI clocks to DT. With the MSTP indices defined as
> > 
> > /* MSTP10 */
> > #define R8A7790_CLK_SSI                5
> > #define R8A7790_CLK_SSI9               6
> > #define R8A7790_CLK_SSI8               7
> > #define R8A7790_CLK_SSI7               8
> > #define R8A7790_CLK_SSI6               9
> > #define R8A7790_CLK_SSI5               10
> > #define R8A7790_CLK_SSI4               11
> > #define R8A7790_CLK_SSI3               12
> > #define R8A7790_CLK_SSI2               13
> > #define R8A7790_CLK_SSI1               14
> > #define R8A7790_CLK_SSI0               15
> > 
> > my naive approach was
> > 
> > mstp10_clks: mstp10_clks@e6150998 {
> > 
> >          compatible = "renesas,r8a7790-mstp-clocks",
> >          "renesas,cpg-mstp-clocks";
> >          reg = <0 0xe6150998 0 4>, <0 0xe61509a8 0 4>;
> >          clocks = <&p_clk>, <&mstp10_clks R8A7790_CLK_SSI>,
> >                   <&mstp10_clks R8A7790_CLK_SSI>,
> >                   <&mstp10_clks R8A7790_CLK_SSI>,
> >                   <&mstp10_clks R8A7790_CLK_SSI>,
> >                   <&mstp10_clks R8A7790_CLK_SSI>,
> >                   <&mstp10_clks R8A7790_CLK_SSI>,
> >                   <&mstp10_clks R8A7790_CLK_SSI>,
> >                   <&mstp10_clks R8A7790_CLK_SSI>,
> >                   <&mstp10_clks R8A7790_CLK_SSI>,
> >                   <&mstp10_clks R8A7790_CLK_SSI>;
> >          #clock-cells = <1>;
> >          renesas,clock-indices = <
> >                  R8A7790_CLK_SSI R8A7790_CLK_SSI9 R8A7790_CLK_SSI8
> >                  R8A7790_CLK_SSI7 R8A7790_CLK_SSI6 R8A7790_CLK_SSI5
> >                  R8A7790_CLK_SSI4 R8A7790_CLK_SSI3 R8A7790_CLK_SSI2
> >                  R8A7790_CLK_SSI1 R8A7790_CLK_SSI0
> >          >;
> >          clock-output-names > >                  "ssi", "ssi9", "ssi8", "ssi7", "ssi6", "ssi5",
> >                  "ssi4", "ssi3", "ssi2", "ssi1", "ssi0";
> > }
> > 
> > However, this resulted in all SSI clocks but the master one referencing
> > "ssi5" as their parent instead of "ssi".
> > 
> > The simple fix was to change the parent clocks to
> > 
> >          clocks = <&p_clk>, <&mstp10_clks 0>, <&mstp10_clks 0>,
> >                   <&mstp10_clks 0>, <&mstp10_clks 0>, <&mstp10_clks 0>,
> >                   <&mstp10_clks 0>, <&mstp10_clks 0>, <&mstp10_clks 0>,
> >                   <&mstp10_clks 0>, <&mstp10_clks 0>;
> > 
> > I understand this difference in behaviour is necessary, as the parent
> > clock device might not have been probed yet when a child clock driver
> > looks up the parent clock name. We thus can't rely on the parent clock
> > driver to perform the clock specifier to clock translation.
> 
> In this case the problem is worse as even if we could defer clock
> lookups until the driver had been probed, we have a case where this
> is self-referential.
> 
> > Another approach to fix the problem was proposed by Ben Dooks in his
> > "[PATCH 1/3] clk: add clock-indices support" patch series was to
> > standardize the reneses,clock-indices property into a clock-indices
> > property, usable by of_clk_get_parent_name() without requiring the parent
> > clock driver to have probed the device yet. That's more complex but would
> > have the added benefit of making the clock specifier translation
> > consistent, at least in this case. I'm not sure whether we'll always be
> > able to achieve consistency as some exotic clock providers might require
> > a really weird translation mechanism.
>
> I think there is a couple of issues here, the first is that
> of_clk_get_parent_name() exists at-all. It would be nicer just to be able to
> pass a set of 'struct clk *' to the clock registration code. Although this
> may also still have an issue where we cannot use self-referential clocks (I
> added a hack to allow the mstp driver to use sub-nodes for each mstp clock
> group to get around that as a first implementation)
> 
> I have not seen any comments on my clock-indices patch yet, I wonder
> if anyone has had a chance to review it.

From a code point of view your proposal looks nice, what I wonder is whether 
that's the direction we want to take. It would fix the problem in this case, 
but as I've mentioned I'm not sure whether we can solve it generically for all 
providers with exotic requirements.

Mike probably has a wider view of the issue, that's why I'd like him to 
comment on this.

-- 
Regards,

Laurent Pinchart

Re: Maybe this is CCF bug

[Mike Turquette]

Quoting Laurent Pinchart (2014-02-21 06:04:20)
> Hi Ben,
> 
> On Friday 21 February 2014 13:42:10 Ben Dooks wrote:
> > On 21/02/14 13:30, Laurent Pinchart wrote:
> > > Hi Morimoto-san,
> > > 
> > > (CC'ing Mike and Ben)
> > > 
> > > On Friday 21 February 2014 11:20:06 Geert Uytterhoeven wrote:
> > >> On Fri, Feb 21, 2014 at 9:58 AM, Kuninori Morimoto wrote:
> > >>> Now, I'm working for sound DT support,
> > >>> and I noticed common clock setting's strange behavior.
> > >>> I guess this is bug, but 50% my misunderstanding.
> > >>> 
> > >>> Now, we have clock index on
> > >>> ${LINUX}/include/dt-bindings/clock/r8a7790-clock.h For example,
> > >>> r8a7790's
> > >>> MSTP9 case, like this
> > >>> 
> > >>>          /* MSTP9 */
> > >>>          #define R8A7790_CLK_GPIO5               7
> > >>>          #define R8A7790_CLK_GPIO4               8
> > >>>          #define R8A7790_CLK_GPIO3               9
> > >>>          #define R8A7790_CLK_GPIO2               10
> > >>>          #define R8A7790_CLK_GPIO1               11
> > >>>          #define R8A7790_CLK_GPIO0               12
> > >>>          #define R8A7790_CLK_RCAN1               15
> > >>>          #define R8A7790_CLK_RCAN0               16
> > >>>          #define R8A7790_CLK_QSPI_MOD            17
> > >>>          #define R8A7790_CLK_IICDVFS             26
> > >>>          #define R8A7790_CLK_I2C3                28
> > >>>          #define R8A7790_CLK_I2C2                29
> > >>>          #define R8A7790_CLK_I2C1                30
> > >>>          #define R8A7790_CLK_I2C0                31
> > >>> 
> > >>> and MSTP9 is like this
> > >>> 
> > >>>          mstp9_clks: mstp9_clks@e6150994 {
> > >>>          
> > >>>                  compatible = "renesas,r8a7790-mstp-clocks",
> > >>>                  "renesas,cpg-mstp-clocks";
> > >>>                  reg = <0 0xe6150994 0 4>, <0 0xe61509a4 0 4>;
> > >>>                  clocks = <&p_clk>, <&p_clk>, <&cpg_clocks
> > >>>                  R8A7790_CLK_QSPI>,
> > >>>                           <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>;
> > >>>                  
> > >>>                  #clock-cells = <1>;
> > >>>                  renesas,clock-indices = <
> > >>>                          R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0
> > >>>                          R8A7790_CLK_QSPI_MOD
> > >>>                          R8A7790_CLK_I2C3 R8A7790_CLK_I2C2
> > >>>                          R8A7790_CLK_I2C1
> > >>>                          R8A7790_CLK_I2C0
> > >>>                  >;
> > >>>                  
> > >>>                  clock-output-names > > >>>                          "rcan1", "rcan0", "qspi_mod", "i2c3", "i2c2",
> > >>>                          "i2c1", "i2c0";
> > >>>          };
> > >>> 
> > >>> And, now, spi parent is MSTP9 QSPI MOD
> > >>> 
> > >>>          spi: spi@e6b10000 {
> > >>>                  ...
> > >>>                  clocks = <&mstp9_clks R8A7790_CLK_QSPI_MOD>;
> > >>>                  ...
> > >>>          };
> > >>> 
> > >>> This SPI would like to use MSTP9's 17th (= R8A7790_CLK_QSPI_MOD) clock
> > >>> as its parent. But, mstp9_clks has 7 clocks only.
> > >>> R8A7790_CLK_xxx means "bit shift", not "index" on DT clock.
> > >> 
> > >> Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt says:
> > >>      The MSTP groups are sparsely populated. Unimplemented
> > >>      gate clocks must not be declared.
> > >>> 
> > >>> On ${LINUX}/drivers/clk/shmobile/clk-mstp.c,
> > >>> it try to get parent name by
> > >>> 
> > >>>          parent_name = of_clk_get_parent_name(np, i);
> > >>> 
> > >>> and it returns "mstp9_clks" in this case.
> > >>> Maybe SPI would like to get "qspi_mod" ?
> > > 
> > > I wouldn't call that a bug in CCF, but it's definitely a non-intuitive
> > > behaviour. CCF lets OF clock providers implement their own method to
> > > translate clock specifiers into clocks (see the second and third
> > > arguments passed to of_clk_add_provider). In practice the vast majority
> > > (if not all) of the drivers implementing support for multiple clocks use
> > > an index as their first clock cell. That index can be a direct index into
> > > the list of clocks provided by the CCF device, but doesn't have to be. In
> > > the case of the clk-mstp driver the first clock cell represents the clock
> > > hardware index, which is different than the index into the software list
> > > of clocks as clocks are sparsely populated.
> > > 
> > > The of_clk_get_parent_name() function behaves differently. It first looks
> > > up the parent clock node in DT and parses the clock specifier cells, and
> > > then uses the first cell as a direct index into the clock-names property
> > > of the parent clock node. This bypasses the parent clock driver clock
> > > lookup mechanism, and thus leads to confusion as the meaning of the clock
> > > specifier in DT will depend on whether you're referencing a clock from an
> > > end-user driver (which will in that case use clk_get() and go through the
> > > clock provider driver for clock lookup), or from another clock provider
> > > (which will in that case call of_clk_get_parent_name() and use direct
> > > index lookup). This has bitten me several weeks ago when I tried to add
> > > SSI clocks to DT. With the MSTP indices defined as
> > > 
> > > /* MSTP10 */
> > > #define R8A7790_CLK_SSI                5
> > > #define R8A7790_CLK_SSI9               6
> > > #define R8A7790_CLK_SSI8               7
> > > #define R8A7790_CLK_SSI7               8
> > > #define R8A7790_CLK_SSI6               9
> > > #define R8A7790_CLK_SSI5               10
> > > #define R8A7790_CLK_SSI4               11
> > > #define R8A7790_CLK_SSI3               12
> > > #define R8A7790_CLK_SSI2               13
> > > #define R8A7790_CLK_SSI1               14
> > > #define R8A7790_CLK_SSI0               15
> > > 
> > > my naive approach was
> > > 
> > > mstp10_clks: mstp10_clks@e6150998 {
> > > 
> > >          compatible = "renesas,r8a7790-mstp-clocks",
> > >          "renesas,cpg-mstp-clocks";
> > >          reg = <0 0xe6150998 0 4>, <0 0xe61509a8 0 4>;
> > >          clocks = <&p_clk>, <&mstp10_clks R8A7790_CLK_SSI>,
> > >                   <&mstp10_clks R8A7790_CLK_SSI>,
> > >                   <&mstp10_clks R8A7790_CLK_SSI>,
> > >                   <&mstp10_clks R8A7790_CLK_SSI>,
> > >                   <&mstp10_clks R8A7790_CLK_SSI>,
> > >                   <&mstp10_clks R8A7790_CLK_SSI>,
> > >                   <&mstp10_clks R8A7790_CLK_SSI>,
> > >                   <&mstp10_clks R8A7790_CLK_SSI>,
> > >                   <&mstp10_clks R8A7790_CLK_SSI>,
> > >                   <&mstp10_clks R8A7790_CLK_SSI>;
> > >          #clock-cells = <1>;
> > >          renesas,clock-indices = <
> > >                  R8A7790_CLK_SSI R8A7790_CLK_SSI9 R8A7790_CLK_SSI8
> > >                  R8A7790_CLK_SSI7 R8A7790_CLK_SSI6 R8A7790_CLK_SSI5
> > >                  R8A7790_CLK_SSI4 R8A7790_CLK_SSI3 R8A7790_CLK_SSI2
> > >                  R8A7790_CLK_SSI1 R8A7790_CLK_SSI0
> > >          >;
> > >          clock-output-names > > >                  "ssi", "ssi9", "ssi8", "ssi7", "ssi6", "ssi5",
> > >                  "ssi4", "ssi3", "ssi2", "ssi1", "ssi0";
> > > }
> > > 
> > > However, this resulted in all SSI clocks but the master one referencing
> > > "ssi5" as their parent instead of "ssi".
> > > 
> > > The simple fix was to change the parent clocks to
> > > 
> > >          clocks = <&p_clk>, <&mstp10_clks 0>, <&mstp10_clks 0>,
> > >                   <&mstp10_clks 0>, <&mstp10_clks 0>, <&mstp10_clks 0>,
> > >                   <&mstp10_clks 0>, <&mstp10_clks 0>, <&mstp10_clks 0>,
> > >                   <&mstp10_clks 0>, <&mstp10_clks 0>;
> > > 
> > > I understand this difference in behaviour is necessary, as the parent
> > > clock device might not have been probed yet when a child clock driver
> > > looks up the parent clock name. We thus can't rely on the parent clock
> > > driver to perform the clock specifier to clock translation.
> > 
> > In this case the problem is worse as even if we could defer clock
> > lookups until the driver had been probed, we have a case where this
> > is self-referential.
> > 
> > > Another approach to fix the problem was proposed by Ben Dooks in his
> > > "[PATCH 1/3] clk: add clock-indices support" patch series was to
> > > standardize the reneses,clock-indices property into a clock-indices
> > > property, usable by of_clk_get_parent_name() without requiring the parent
> > > clock driver to have probed the device yet. That's more complex but would
> > > have the added benefit of making the clock specifier translation
> > > consistent, at least in this case. I'm not sure whether we'll always be
> > > able to achieve consistency as some exotic clock providers might require
> > > a really weird translation mechanism.
> >
> > I think there is a couple of issues here, the first is that
> > of_clk_get_parent_name() exists at-all. It would be nicer just to be able to
> > pass a set of 'struct clk *' to the clock registration code. Although this
> > may also still have an issue where we cannot use self-referential clocks (I
> > added a hack to allow the mstp driver to use sub-nodes for each mstp clock
> > group to get around that as a first implementation)
> > 
> > I have not seen any comments on my clock-indices patch yet, I wonder
> > if anyone has had a chance to review it.
> 
> From a code point of view your proposal looks nice, what I wonder is whether 
> that's the direction we want to take. It would fix the problem in this case, 
> but as I've mentioned I'm not sure whether we can solve it generically for all 
> providers with exotic requirements.
> 
> Mike probably has a wider view of the issue, that's why I'd like him to 
> comment on this.

In the past we could not pass struct clk * to clock registration code
because there was no guarantee that the parent clock had been
registered. Furthermore the definition of struct clk is not exposed to
the wide world so we can't use static data to initialize it.

I have considered the ability for a clk_init_data structure to pass a
list of parents via struct clk_init_data **parents. This mimics the
linkage that devicetree gives us, at least within a clock provider. And
really we probably should be using DT to link up clock providers via
phandles.

In general the string-based lookup scheme internal to CCF (and now
leaked out into DT bindings) is showing signs of age and
short-sightedness. Perhaps a combination of the existing phandle-based
linkage between clock providers in DT and some way to express the same
for clk_init_data would be sufficient to remove the need use clock name
strings for expressing parent/child relationships?

Regards,
Mike

> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: Maybe this is CCF bug

[Kuninori Morimoto]

Hi Ben

> > Now, I'm working for sound DT support,
> > and I noticed common clock setting's strange behavior.
> > I guess this is bug, but 50% my misunderstanding.
> 
> [snip]
> 
> I ran into this a couple of weeks back and reported it
> to the relevant people. See here for my proposed fix:
> 
> http://www.spinics.net/lists/linux-sh/msg28537.html
> http://www.spinics.net/lists/linux-sh/msg28536.html
> http://www.spinics.net/lists/linux-sh/msg28535.html

Wow thank you !!

I noticed this issue on r8a7790 MSTP10.
And Laurent already knew it.

Best regards
---
Kuninori Morimoto

Re: Maybe this is CCF bug

[Laurent Pinchart]

Hi Mike,

On Sunday 23 February 2014 16:35:54 Mike Turquette wrote:
> Quoting Laurent Pinchart (2014-02-21 06:04:20)
> > On Friday 21 February 2014 13:42:10 Ben Dooks wrote:
> > > On 21/02/14 13:30, Laurent Pinchart wrote:

[snip]

> > > > Another approach to fix the problem was proposed by Ben Dooks in his
> > > > "[PATCH 1/3] clk: add clock-indices support" patch series was to
> > > > standardize the reneses,clock-indices property into a clock-indices
> > > > property, usable by of_clk_get_parent_name() without requiring the
> > > > parent clock driver to have probed the device yet. That's more complex
> > > > but would have the added benefit of making the clock specifier
> > > > translation consistent, at least in this case. I'm not sure whether
> > > > we'll always be able to achieve consistency as some exotic clock
> > > > providers might require a really weird translation mechanism.
> > > 
> > > I think there is a couple of issues here, the first is that
> > > of_clk_get_parent_name() exists at-all. It would be nicer just to be
> > > able to pass a set of 'struct clk *' to the clock registration code.
> > > Although this may also still have an issue where we cannot use
> > > self-referential clocks (I added a hack to allow the mstp driver to use
> > > sub-nodes for each mstp clock group to get around that as a first
> > > implementation)
> > > 
> > > I have not seen any comments on my clock-indices patch yet, I wonder
> > > if anyone has had a chance to review it.
> > 
> > From a code point of view your proposal looks nice, what I wonder is
> > whether that's the direction we want to take. It would fix the problem in
> > this case, but as I've mentioned I'm not sure whether we can solve it
> > generically for all providers with exotic requirements.
> > 
> > Mike probably has a wider view of the issue, that's why I'd like him to
> > comment on this.
> 
> In the past we could not pass struct clk * to clock registration code
> because there was no guarantee that the parent clock had been
> registered.

Could we have that guarantee now ? I expect this would result in a lot of 
probe deferral, which might not be good from a boot time point of view.

> Furthermore the definition of struct clk is not exposed to the wide world so
> we can't use static data to initialize it.
> 
> I have considered the ability for a clk_init_data structure to pass a list
> of parents via struct clk_init_data **parents. This mimics the linkage that
> devicetree gives us, at least within a clock provider. And really we
> probably should be using DT to link up clock providers via phandles.

On DT platforms using clock references instead of names would be a good idea I 
believe. We of course need a different solution on platforms without DT, and 
even on ARM for platforms that haven't been fully converted to DT.

> In general the string-based lookup scheme internal to CCF (and now leaked
> out into DT bindings) is showing signs of age and short-sightedness. Perhaps
> a combination of the existing phandle-based linkage between clock providers
> in DT and some way to express the same for clk_init_data would be sufficient
> to remove the need use clock name strings for expressing parent/child
> relationships?

I have boards not fully ported to DT that instantiate all their clocks through 
DT but look them up in C code with names for platform devices. We would need 
something similar, possibly replacing the clock name by the full DT path to 
its provider node.

-- 
Regards,

Laurent Pinchart