Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Gabriel FERNANDEZ]

On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
> Hello Gabriel,
>
> On 07/19/2017 05:25 PM, gabriel.fernan...@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 000..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 = <_hse>, <_lse>, <_i2s_ckin>;
>> +
>> +st,syscfg = <>;
>> +
>> +#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@4c00 {
>> +compatible = "st,stm32-timer";
>> +reg = <0x4c00 0x400>;
>> +interrupts = <50>;
>> +clocks = < 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 -->  

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Gabriel FERNANDEZ]

On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
> Hello Gabriel,
>
> On 07/19/2017 05:25 PM, gabriel.fernan...@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 000..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 = <_hse>, <_lse>, <_i2s_ckin>;
>> +
>> +st,syscfg = <>;
>> +
>> +#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@4c00 {
>> +compatible = "st,stm32-timer";
>> +reg = <0x4c00 0x400>;
>> +interrupts = <50>;
>> +clocks = < 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 

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Gabriel FERNANDEZ]

On 07/21/2017 10:37 PM, Stephen Boyd wrote:
> On 07/20, Vladimir Zapolskiy wrote:
>> Hi Gabriel,
>>
>> On 07/20/2017 11:31 AM, Gabriel FERNANDEZ wrote:
>>> Hi Vladimir,
>>>
>>>
>>> On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
 Hello Gabriel,

 On 07/19/2017 05:25 PM, gabriel.fernan...@st.com wrote:
> From: Gabriel Fernandez 
> +
> + 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:
>>> I prefer to keep rcc node name, to be coherent with the other ST
>>> platforms (STM32F4/F7)
>> the thing is, a device node name is expected to comply with ePAPR or
>> the devicetree specification, which says
>>
>>  The name of a node should be somewhat generic, reflecting
>>  the function of the device and not its precise programming model.
>>
>> If devicetree and CCF maintainers are fine with 'rcc', I won't object,
>> my role is just to emphasize the found issue and recommend to use another
>> and more common name 'clock-controller', it is a simple and fortunately
>> backward compatible change to other ST platforms as well.
> Yes. It should be generic so clock-controller or
> clock-reset-controller is appropriate here.
>
ok i will change order... reset-clock-controller name to match with rcc.

Best Regards
Gabriel

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Gabriel FERNANDEZ]

On 07/21/2017 10:37 PM, Stephen Boyd wrote:
> On 07/20, Vladimir Zapolskiy wrote:
>> Hi Gabriel,
>>
>> On 07/20/2017 11:31 AM, Gabriel FERNANDEZ wrote:
>>> Hi Vladimir,
>>>
>>>
>>> On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
 Hello Gabriel,

 On 07/19/2017 05:25 PM, gabriel.fernan...@st.com wrote:
> From: Gabriel Fernandez 
> +
> + 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:
>>> I prefer to keep rcc node name, to be coherent with the other ST
>>> platforms (STM32F4/F7)
>> the thing is, a device node name is expected to comply with ePAPR or
>> the devicetree specification, which says
>>
>>  The name of a node should be somewhat generic, reflecting
>>  the function of the device and not its precise programming model.
>>
>> If devicetree and CCF maintainers are fine with 'rcc', I won't object,
>> my role is just to emphasize the found issue and recommend to use another
>> and more common name 'clock-controller', it is a simple and fortunately
>> backward compatible change to other ST platforms as well.
> Yes. It should be generic so clock-controller or
> clock-reset-controller is appropriate here.
>
ok i will change order... reset-clock-controller name to match with rcc.

Best Regards
Gabriel

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Stephen Boyd]

On 07/20, Vladimir Zapolskiy wrote:
> Hi Gabriel,
> 
> On 07/20/2017 11:31 AM, Gabriel FERNANDEZ wrote:
> > Hi Vladimir,
> > 
> > 
> > On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
> >> Hello Gabriel,
> >>
> >> On 07/19/2017 05:25 PM, gabriel.fernan...@st.com wrote:
> >>> From: Gabriel Fernandez 
> >>> +
> >>> + 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:
> > I prefer to keep rcc node name, to be coherent with the other ST 
> > platforms (STM32F4/F7)
> 
> the thing is, a device node name is expected to comply with ePAPR or
> the devicetree specification, which says
> 
>   The name of a node should be somewhat generic, reflecting
>   the function of the device and not its precise programming model.
> 
> If devicetree and CCF maintainers are fine with 'rcc', I won't object,
> my role is just to emphasize the found issue and recommend to use another
> and more common name 'clock-controller', it is a simple and fortunately
> backward compatible change to other ST platforms as well.

Yes. It should be generic so clock-controller or
clock-reset-controller is appropriate here.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Stephen Boyd]

On 07/20, Vladimir Zapolskiy wrote:
> Hi Gabriel,
> 
> On 07/20/2017 11:31 AM, Gabriel FERNANDEZ wrote:
> > Hi Vladimir,
> > 
> > 
> > On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
> >> Hello Gabriel,
> >>
> >> On 07/19/2017 05:25 PM, gabriel.fernan...@st.com wrote:
> >>> From: Gabriel Fernandez 
> >>> +
> >>> + 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:
> > I prefer to keep rcc node name, to be coherent with the other ST 
> > platforms (STM32F4/F7)
> 
> the thing is, a device node name is expected to comply with ePAPR or
> the devicetree specification, which says
> 
>   The name of a node should be somewhat generic, reflecting
>   the function of the device and not its precise programming model.
> 
> If devicetree and CCF maintainers are fine with 'rcc', I won't object,
> my role is just to emphasize the found issue and recommend to use another
> and more common name 'clock-controller', it is a simple and fortunately
> backward compatible change to other ST platforms as well.

Yes. It should be generic so clock-controller or
clock-reset-controller is appropriate here.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Vladimir Zapolskiy]

Hi Gabriel,

On 07/20/2017 11:31 AM, Gabriel FERNANDEZ wrote:
> Hi Vladimir,
> 
> 
> On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
>> Hello Gabriel,
>>
>> On 07/19/2017 05:25 PM, gabriel.fernan...@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 000..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:
> I prefer to keep rcc node name, to be coherent with the other ST 
> platforms (STM32F4/F7)

the thing is, a device node name is expected to comply with ePAPR or
the devicetree specification, which says

The name of a node should be somewhat generic, reflecting
the function of the device and not its precise programming model.

If devicetree and CCF maintainers are fine with 'rcc', I won't object,
my role is just to emphasize the found issue and recommend to use another
and more common name 'clock-controller', it is a simple and fortunately
backward compatible change to other ST platforms as well.

>>  rcc: clock-controller@58024400 {
>>
>>> +   #reset-cells = <1>;
>>> +   #clock-cells = <2>
>> Missing trailing semicolon   ^^^
> ok

--
With best wishes,
Vladimir

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Vladimir Zapolskiy]

Hi Gabriel,

On 07/20/2017 11:31 AM, Gabriel FERNANDEZ wrote:
> Hi Vladimir,
> 
> 
> On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
>> Hello Gabriel,
>>
>> On 07/19/2017 05:25 PM, gabriel.fernan...@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 000..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:
> I prefer to keep rcc node name, to be coherent with the other ST 
> platforms (STM32F4/F7)

the thing is, a device node name is expected to comply with ePAPR or
the devicetree specification, which says

The name of a node should be somewhat generic, reflecting
the function of the device and not its precise programming model.

If devicetree and CCF maintainers are fine with 'rcc', I won't object,
my role is just to emphasize the found issue and recommend to use another
and more common name 'clock-controller', it is a simple and fortunately
backward compatible change to other ST platforms as well.

>>  rcc: clock-controller@58024400 {
>>
>>> +   #reset-cells = <1>;
>>> +   #clock-cells = <2>
>> Missing trailing semicolon   ^^^
> ok

--
With best wishes,
Vladimir

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Gabriel FERNANDEZ]

Hi Vladimir,


On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
> Hello Gabriel,
>
> On 07/19/2017 05:25 PM, gabriel.fernan...@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 000..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:
I prefer to keep rcc node name, to be coherent with the other ST 
platforms (STM32F4/F7)
>   rcc: clock-controller@58024400 {
>
>> +#reset-cells = <1>;
>> +#clock-cells = <2>
> Missing trailing semicolon   ^^^
ok
> My recommendation is to move #reset-cells and #clock-cells properties
> down after 'reg' or 'clocks' property in the list.
ok
>
>> +compatible = "st,stm32h743-rcc", "st,stm32-rcc";
>> +reg = <0x58024400 0x400>;
>> +clocks = <_hse>, <_lse>, <_i2s_ckin>;
>> +
>> +st,syscfg = <>;
>> +
>> +#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.
ok
>> +};
>> +
>> +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@4c00 {
>> +compatible = "st,stm32-timer";
>> +reg = <0x4c00 0x400>;
>> +interrupts = <50>;
>> +clocks = < TIM5_CK>;
>> +
> Please remote the empty line above.
ok
>> +};
>> +
>> +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 

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Gabriel FERNANDEZ]

Hi Vladimir,


On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
> Hello Gabriel,
>
> On 07/19/2017 05:25 PM, gabriel.fernan...@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 000..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:
I prefer to keep rcc node name, to be coherent with the other ST 
platforms (STM32F4/F7)
>   rcc: clock-controller@58024400 {
>
>> +#reset-cells = <1>;
>> +#clock-cells = <2>
> Missing trailing semicolon   ^^^
ok
> My recommendation is to move #reset-cells and #clock-cells properties
> down after 'reg' or 'clocks' property in the list.
ok
>
>> +compatible = "st,stm32h743-rcc", "st,stm32-rcc";
>> +reg = <0x58024400 0x400>;
>> +clocks = <_hse>, <_lse>, <_i2s_ckin>;
>> +
>> +st,syscfg = <>;
>> +
>> +#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.
ok
>> +};
>> +
>> +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@4c00 {
>> +compatible = "st,stm32-timer";
>> +reg = <0x4c00 0x400>;
>> +interrupts = <50>;
>> +clocks = < TIM5_CK>;
>> +
> Please remote the empty line above.
ok
>> +};
>> +
>> +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 -->

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Vladimir Zapolskiy]

Hello Gabriel,

On 07/19/2017 05:25 PM, gabriel.fernan...@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 000..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 = <_hse>, <_lse>, <_i2s_ckin>;
> +
> + st,syscfg = <>;
> +
> + #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@4c00 {
> + compatible = "st,stm32-timer";
> + reg = <0x4c00 0x400>;
> + interrupts = <50>;
> + clocks = < 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 

Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

[Vladimir Zapolskiy]

Hello Gabriel,

On 07/19/2017 05:25 PM, gabriel.fernan...@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 000..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 = <_hse>, <_lse>, <_i2s_ckin>;
> +
> + st,syscfg = <>;
> +
> + #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@4c00 {
> + compatible = "st,stm32-timer";
> + reg = <0x4c00 0x400>;
> + interrupts = <50>;
> + clocks = < 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,