On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote: > Hello Gabriel, > > On 07/19/2017 05:25 PM, gabriel.fernandez@xxxxxx wrote: >> From: Gabriel Fernandez <gabriel.fernandez@xxxxxx> >> >> This patch enables clocks for STM32H743 boards. >> >> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@xxxxxx> >> >> for MFD changes: >> Acked-by: Lee Jones <lee.jones@xxxxxxxxxx> >> >> for DT-Bindings >> Acked-by: Rob Herring <robh@xxxxxxxxxx> >> --- >> .../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 ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f