Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755249AbdGVL6y (ORCPT ); Sat, 22 Jul 2017 07:58:54 -0400 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:38771 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753602AbdGVL6v (ORCPT ); Sat, 22 Jul 2017 07:58:51 -0400 From: Gabriel FERNANDEZ To: Vladimir Zapolskiy , Rob Herring , Stephen Boyd 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 , "Olivier BIDEAU" , Amelie DELAUNAY , "gabriel.fernandez.st@gmail.com" , "Arvind Yadav" Subject: Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver Thread-Topic: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver Thread-Index: AQHTAJrtnO3TSWU6nUubz30yFHqT2qJbhvAAgAQZ5gA= Date: Sat, 22 Jul 2017 11:58:10 +0000 Message-ID: References: <1500474344-9832-1-git-send-email-gabriel.fernandez@st.com> <1500474344-9832-4-git-send-email-gabriel.fernandez@st.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.49] Content-Type: text/plain; charset="utf-8" Content-ID: <912D35F0BFD06443BA4DF39E065D708B@st.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-07-22_08:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v6MBx1ot029772 Content-Length: 5321 Lines: 156 On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote: > 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. > >> Hi 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? i will suppress it, it's not a problem Many Thanks Vladimir >> +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