Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754437AbdGSVUc (ORCPT ); Wed, 19 Jul 2017 17:20:32 -0400 Received: from mleia.com ([178.79.152.223]:33577 "EHLO mail.mleia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754387AbdGSVU3 (ORCPT ); Wed, 19 Jul 2017 17:20:29 -0400 Subject: Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver To: gabriel.fernandez@st.com, Rob Herring , Stephen Boyd References: <1500474344-9832-1-git-send-email-gabriel.fernandez@st.com> <1500474344-9832-4-git-send-email-gabriel.fernandez@st.com> Cc: Mark Rutland , Russell King , Maxime Coquelin , Alexandre Torgue , Michael Turquette , Nicolas Pitre , Arnd Bergmann , daniel.thompson@linaro.org, andrea.merello@gmail.com, radoslaw.pietrzyk@gmail.com, Lee Jones , Sylvain Lemieux , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, ludovic.barre@st.com, olivier.bideau@st.com, amelie.delaunay@st.com, gabriel.fernandez.st@gmail.com, Arvind Yadav From: Vladimir Zapolskiy Message-ID: Date: Thu, 20 Jul 2017 00:20:26 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.4.0 MIME-Version: 1.0 In-Reply-To: <1500474344-9832-4-git-send-email-gabriel.fernandez@st.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-49551924 X-CRM114-CacheID: sfid-20170719_222028_090142_C8B7C7DC X-CRM114-Status: GOOD ( 30.46 ) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5037 Lines: 157 Hello Gabriel, On 07/19/2017 05:25 PM, gabriel.fernandez@st.com wrote: > From: Gabriel Fernandez > > This patch enables clocks for STM32H743 boards. > > Signed-off-by: Gabriel Fernandez > > for MFD changes: > Acked-by: Lee Jones > > for DT-Bindings > Acked-by: Rob Herring > --- > .../devicetree/bindings/clock/st,stm32h7-rcc.txt | 82 ++ I'll provide some review comments about device tree bindings only. > drivers/clk/Makefile | 1 + > drivers/clk/clk-stm32h7.c | 1409 ++++++++++++++++++++ > include/dt-bindings/clock/stm32h7-clks.h | 165 +++ > include/dt-bindings/mfd/stm32h7-rcc.h | 136 ++ > 5 files changed, 1793 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt > create mode 100644 drivers/clk/clk-stm32h7.c > create mode 100644 include/dt-bindings/clock/stm32h7-clks.h > create mode 100644 include/dt-bindings/mfd/stm32h7-rcc.h > > diff --git a/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt b/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt > new file mode 100644 > index 0000000..442c50c > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt > @@ -0,0 +1,82 @@ > +STMicroelectronics STM32H7 Reset and Clock Controller > +===================================================== > + > +The RCC IP is both a reset and a clock controller. > + > +Please refer to clock-bindings.txt for common clock controller binding usage. > +Please also refer to reset.txt for common reset controller binding usage. > + > +Required properties: > +- compatible: Should be: > + "st,stm32h743-rcc" > + > +- reg: should be register base and length as documented in the > + datasheet > + > +- #reset-cells: 1, see below > + > +- #clock-cells : from common clock binding; shall be set to 1 > + > +- clocks: External oscillator clock phandle > + - high speed external clock signal (HSE) > + - low speed external clock signal (LSE) > + - external I2S clock (I2S_CKIN) > + > +Optional properties: > +- st,syscfg: phandle for pwrcfg, mandatory to disable/enable backup domain > + write protection (RTC clock). > + > +Example: > + > + rcc: rcc@58024400 { 'rcc' as a generic device node name is awkward. I believe the main function of the device is clock controller (unlikely a generic reset controller can be converted into a clock controller), the locations of the document and device driver also indicate that primarily it is a clock controller, so I suggest to replace device node name with 'clock-controller' like below: rcc: clock-controller@58024400 { > + #reset-cells = <1>; > + #clock-cells = <2> Missing trailing semicolon ^^^ My recommendation is to move #reset-cells and #clock-cells properties down after 'reg' or 'clocks' property in the list. > + compatible = "st,stm32h743-rcc", "st,stm32-rcc"; > + reg = <0x58024400 0x400>; > + clocks = <&clk_hse>, <&clk_lse>, <&clk_i2s_ckin>; > + > + st,syscfg = <&pwrcfg>; > + > + #address-cells = <1>; > + #size-cells = <0>; Please drop #address-cells and #size-cells properties completely, from the document the device node does not define any children subnodes. > + }; > + > +The peripheral clock consumer should specify the desired clock by > +having the clock ID in its "clocks" phandle cell. > + > +All available clocks are defined as preprocessor macros in > +dt-bindings/clock/stm32h7-clks.h header and can be used in device > +tree sources. > + > +Example: > + > + timer5: timer@40000c00 { > + compatible = "st,stm32-timer"; > + reg = <0x40000c00 0x400>; > + interrupts = <50>; > + clocks = <&rcc TIM5_CK>; > + Please remote the empty line above. > + }; > + > +Specifying softreset control of devices > +======================================= > + > +Device nodes should specify the reset channel required in their "resets" > +property, containing a phandle to the reset device node and an index specifying > +which channel to use. > +The index is the bit number within the RCC registers bank, starting from RCC > +base address. > +It is calculated as: index = register_offset / 4 * 32 + bit_offset. > +Where bit_offset is the bit offset within the register. > + > +For example, for CRC reset: > + crc = AHB4RSTR_offset / 4 * 32 + CRCRST_bit_offset = 0x88 / 4 * 32 + 19 = 1107 > + > +All available preprocessor macros for reset are defined dt-bindings//mfd/stm32h7-rcc.h Double slashes ----------> ^^^^ I have doubts if it is permitted to add source paths into the device tree bindings documentation, because such information is specific to the Linux source code. Rob, can you clarify please? > +header and can be used in device tree sources. > + > +example: For unification, please capitalize it: 'Example:' > + > + timer2 { > + resets = <&rcc STM32H7_APB1L_RESET(TIM2)>; > + }; -- With best wishes, Vladimir