From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Horman Date: Fri, 14 Mar 2014 08:08:04 +0000 Subject: Re: [PATCH v4 04/19] ARM: shmobile: r8a7779: Add clocks Message-Id: <20140314080803.GB31900@verge.net.au> List-Id: References: <1394701202-25823-1-git-send-email-horms+renesas@verge.net.au> <1394701202-25823-5-git-send-email-horms+renesas@verge.net.au> <2144303.VvnA5cuI5A@avalon> In-Reply-To: <2144303.VvnA5cuI5A@avalon> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org Hi Laurent, On Thu, Mar 13, 2014 at 10:54:03AM +0100, Laurent Pinchart wrote: > Hi Simon, > > Thank you for the patch. Thanks for your review. I will fix things up as you suggest. > > On Thursday 13 March 2014 17:59:47 Simon Horman wrote: > > Declare all core and MSTP clocks currently used by r8a7779-based boards. > > > > Based on work by Laurent Pinchart for the r8a7790 and r8a7791 SoCs. > > > > Cc: Laurent Pinchart > > Signed-off-by: Simon Horman > > > > --- > > v4 > > * As suggested by Geert Uytterhoeven > > - Wrap lines at < 80columns > > - Beef-up short lines towards 80 columns > > * As suggested by Laurent Pinchart > > - Add HSCIF clocks > > - Correct many clock sources > > - Correct reg of cpg_clocks > > * Add CPG clock "b" which is now part of the binding > > > > v3 > > * As suggested by Laurent Pinchart > > - Add and use extal_clk > > - Fix bogus status register use for MSTP clocks > > - Fix bogus mstp3_cls to use its own entries rather than > > that of mstp1_clks > > > > * Update to use "main" in cpg_clocks as per updated > > binding in previous patch > > * Update for new, consolidated and renamed index macros > > - R8A7779_CLK_ETHER > > - R8A7779_CLK_HSCIF > > - R8A7779_CLK_HSPI > > - R8A7779_CLK_MMC0,1 > > - R8A7779_CLK_PCIE > > - R8A7779_CLK_USB01,2 > > --- > > arch/arm/boot/dts/r8a7779.dtsi | 151 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 151 insertions(+) > > > > diff --git a/arch/arm/boot/dts/r8a7779.dtsi b/arch/arm/boot/dts/r8a7779.dtsi > > index d0561d4..c0802aa 100644 > > --- a/arch/arm/boot/dts/r8a7779.dtsi > > +++ b/arch/arm/boot/dts/r8a7779.dtsi > > @@ -11,6 +11,7 @@ > > > > /include/ "skeleton.dtsi" > > > > +#include > > #include > > > > / { > > @@ -278,4 +279,154 @@ > > interrupts = <0 75 IRQ_TYPE_LEVEL_HIGH>; > > status = "disabled"; > > }; > > + > > + clocks { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + ranges; > > + > > + /* External root clock */ > > + extal_clk: extal_clk { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + /* This value must be overriden by the board. */ > > + clock-frequency = <0>; > > + clock-output-names = "extal"; > > + }; > > + > > + /* Special CPG clocks */ > > + cpg_clocks: cpg_clocks@0xe6150000 { > > + compatible = "renesas,r8a7779-cpg-clocks"; > > + reg = <0 0xffc80000 0 0x30>; > > + clocks = <&extal_clk>; > > + #clock-cells = <1>; > > + clock-output-names = "plla", "z", "zs", "s", > > + "s1", "p", "b", "out"; > > + }; > > + > > + /* Fixed factor clocks */ > > + i_clk: i_clk { > > + compatible = "fixed-factor-clock"; > > + clocks = <&cpg_clocks R8A7779_CLK_PLLA>; > > + #clock-cells = <0>; > > + clock-div = <2>; > > + clock-mult = <1>; > > + clock-output-names = "i"; > > + }; > > + s3_clk: s3_clk { > > + compatible = "fixed-factor-clock"; > > + clocks = <&cpg_clocks R8A7779_CLK_PLLA>; > > + #clock-cells = <0>; > > + clock-div = <8>; > > + clock-mult = <1>; > > + clock-output-names = "s3"; > > + }; > > + s4_clk: s4_clk { > > + compatible = "fixed-factor-clock"; > > + clocks = <&cpg_clocks R8A7779_CLK_PLLA>; > > + #clock-cells = <0>; > > + clock-div = <16>; > > + clock-mult = <1>; > > + clock-output-names = "s4"; > > + }; > > + g_clk: g_clk { > > + compatible = "fixed-factor-clock"; > > + clocks = <&cpg_clocks R8A7779_CLK_PLLA>; > > + #clock-cells = <0>; > > + clock-div = <24>; > > + clock-mult = <1>; > > + clock-output-names = "g"; > > + }; > > + > > + /* Gate clocks */ > > + mstp0_clks: mstp0_clks { > > + compatible = "renesas,r8a7779-mstp-clocks", > > + "renesas,cpg-mstp-clocks"; > > + reg = <0 0xffc80030 0 4>; > > + clocks = <&cpg_clocks R8A7779_CLK_S>, > > + <&cpg_clocks R8A7779_CLK_S>, > > + <&cpg_clocks R8A7779_CLK_S>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_S>, > > + <&cpg_clocks R8A7779_CLK_S>, > > + <&cpg_clocks R8A7779_CLK_S1>, > > + <&cpg_clocks R8A7779_CLK_S1>, > > + <&cpg_clocks R8A7779_CLK_S1>, > > + <&cpg_clocks R8A7779_CLK_S1>, > > + <&cpg_clocks R8A7779_CLK_S1>, > > + <&cpg_clocks R8A7779_CLK_S1>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_P>; > > + #clock-cells = <1>; > > + renesas,clock-indices = < > > + R8A7779_CLK_HSPI R8A7779_CLK_HSPI > > + R8A7779_CLK_HSPI > > I would define a single HSPI clock and reference it from the three HSPI > instances. Having three identical copies of a single clock will not work, the > first clock to be disabled will disable the other two as they would all share > the same register bit. Don't forget to remove two of the three CLK_S > references above. > > > R8A7779_CLK_TMU0 > > + R8A7779_CLK_TMU0 R8A7779_CLK_TMU0 > > Bad copy and paste ? This should be TMU2, TMU1 and TMU0. > > > + R8A7779_CLK_HSCIF0 R8A7779_CLK_HSCIF0 > > HSCIF1 and HSCIF0 ? > > > + R8A7779_CLK_SCIF5 R8A7779_CLK_SCIF4 > > + R8A7779_CLK_SCIF3 R8A7779_CLK_SCIF2 > > + R8A7779_CLK_SCIF1 R8A7779_CLK_SCIF0 > > + R8A7779_CLK_I2C3 R8A7779_CLK_I2C2 > > + R8A7779_CLK_I2C1 R8A7779_CLK_I2C0 > > + >; > > + clock-output-names > > + "hspi0", "hspi1", "hspi2", "tmu0", "tmu1", > > + "tmu2", > > Following the comments above, this should become > > "hspi", "tmu2", "tmu1", "tmu0". > > > "hscif1", "hscif0", "scif5", "scif4", > > + "scif3", "scif2", "scif1", "scif0", "i2c3", > > + "i2c2", "i2c1", "i2c0"; > > + }; > > + mstp1_clks: mstp1_clks { > > + compatible = "renesas,r8a7779-mstp-clocks", > > + "renesas,cpg-mstp-clocks"; > > + reg = <0 0xffc80034 0 4>, <0 0xffc80044 0 4>; > > + clocks = <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_S>, > > + <&cpg_clocks R8A7779_CLK_S>, > > + <&cpg_clocks R8A7779_CLK_S>, > > + <&cpg_clocks R8A7779_CLK_S>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_P>, > > + <&cpg_clocks R8A7779_CLK_S>; > > + #clock-cells = <1>; > > + renesas,clock-indices = < > > + R8A7779_CLK_USB01 R8A7779_CLK_USB01 > > Same as above, two instances of the same hardware clock won't fly. > > > + R8A7779_CLK_USB2 R8A7779_CLK_USB2 > > Same here. > > > + R8A7779_CLK_DU R8A7779_CLK_VIN2 > > + R8A7779_CLK_VIN1 R8A7779_CLK_VIN0 > > + R8A7779_CLK_ETHER R8A7779_CLK_SATA > > + R8A7779_CLK_PCIE R8A7779_CLK_VIN3 > > + >; > > + clock-output-names > > + "ehci0", "ohci0", > > + "ehci1", "ohci1", > > Following the comments above, this would become "usb01", "usb2". > > > + "du", "vin2", > > + "vin1", "vin0", > > + "ether", "sata", > > + "pcie", "vin3"; > > + }; > > + mstp3_clks: mstp3_clks { > > + compatible = "renesas,r8a7779-mstp-clocks", > > + "renesas,cpg-mstp-clocks"; > > + reg = <0 0xffc8003c 0 4>; > > + clocks = <&s4_clk>, <&s4_clk>, <&s4_clk>, <&s4_clk>, > > + <&s4_clk>, <&s4_clk>; > > + #clock-cells = <1>; > > + renesas,clock-indices = < > > + R8A7779_CLK_SDHI3 R8A7779_CLK_SDHI2 > > + R8A7779_CLK_SDHI1 R8A7779_CLK_SDHI0 > > + R8A7779_CLK_MMC1 R8A7779_CLK_MMC0 > > + >; > > + clock-output-names > > + "sdhi3", "sdhi2", "sdhi1", "sdhi0", > > + "mmc1", "mmc0"; > > + }; > > + }; > > }; > > -- > Regards, > > Laurent Pinchart >