From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Date: Fri, 21 Feb 2014 13:30:14 +0000 Subject: Re: Maybe this is CCF bug Message-Id: <1439633.Xj1yf6G8YG@avalon> List-Id: References: <87zjlkq1v1.wl%kuninori.morimoto.gx@gmail.com> In-Reply-To: <87zjlkq1v1.wl%kuninori.morimoto.gx@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-sh@vger.kernel.org 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 > +#else > +#include > +#endif > #include > #include > #include > @@ -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