Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1691713 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v7 0/3] clk: stm32h7: Add stm32h743 clock driver <gabriel.fernandez@st.com> - 2017-07-19 16:30 +0200
    [PATCH v7 2/3] clk: gate: expose clk_gate_ops::is_enabled <gabriel.fernandez@st.com> - 2017-07-19 16:30 +0200
      Re: [PATCH v7 2/3] clk: gate: expose clk_gate_ops::is_enabled Vladimir Zapolskiy <vz@mleia.com> - 2017-07-19 23:10 +0200
    [PATCH v7 1/3] clk: nxp: clk-lpc32xx: rename clk_gate_is_enabled() <gabriel.fernandez@st.com> - 2017-07-19 16:30 +0200
      Re: [PATCH v7 1/3] clk: nxp: clk-lpc32xx: rename  clk_gate_is_enabled() Vladimir Zapolskiy <vz@mleia.com> - 2017-07-19 23:10 +0200
    Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver Vladimir Zapolskiy <vz@mleia.com> - 2017-07-19 23:30 +0200
      Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver Gabriel FERNANDEZ <gabriel.fernandez@st.com> - 2017-07-20 10:40 +0200
        Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver Vladimir Zapolskiy <vz@mleia.com> - 2017-07-20 14:00 +0200
          Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver Stephen Boyd <sboyd@codeaurora.org> - 2017-07-21 22:40 +0200
            Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver Gabriel FERNANDEZ <gabriel.fernandez@st.com> - 2017-07-22 14:00 +0200
      Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver Gabriel FERNANDEZ <gabriel.fernandez@st.com> - 2017-07-22 14:00 +0200

#1691713 — [PATCH v7 0/3] clk: stm32h7: Add stm32h743 clock driver

From: Gabriel Fernandez <gabriel.fernandez@st.com>

v6:
  - rename clk_gate_is_enabled() in nxp lpc32xx driver.
  - add EXPORT_SYMBOL_GPL(clk_gate_is_enabled)

v5:
  - return bool instead int for enable_power_domain_write_protection()
  - add comment to explain use of CLK_OF_DECLARE_DRIVER()
  - add comment to explain why we can't use read_poll_timeout()
  - expose clk_gate_ops::is_enabled
  - use of __clk_mux_determine_rate & clk_gate_is_enabled to avoid wrapper
	function.

v4:
  - rename lock into stm32rcc_lock
  - don't use clk_readl() 
  - remove useless parentheses with GENMASK
  - fix parents of timer_x clocks
  - suppress pll configuration from DT
  - fix kbuild warning

v3:
  - fix compatible string "stm32h7-pll" into "st,stm32h7-pll"
  - fix bad parent name for mco2 clock
  - set CLK_SET_RATE_PARENT for ltdc clock
  - set CLK_IGNORE_UNUSED for pll1
  - disable power domain write protection on disable ops if needed


v2:
  - rename compatible string "stm32,pll" into "stm32h7-pll"
  - suppress "st,pllrge" property
  - suppress "st, frac-status" property
  - change management of "st,frac"  property
	0 : enable 0 pll integer mode 
	other values : enable pll in fractional mode (value is
	the fractional factor)

Gabriel Fernandez (3):
  clk: nxp: clk-lpc32xx: rename clk_gate_is_enabled()
  clk: gate: expose clk_gate_ops::is_enabled
  clk: stm32h7: Add stm32h743 clock driver

 .../devicetree/bindings/clock/st,stm32h7-rcc.txt   |   82 ++
 drivers/clk/Makefile                               |    1 +
 drivers/clk/clk-gate.c                             |    3 +-
 drivers/clk/clk-stm32h7.c                          | 1409 ++++++++++++++++++++
 drivers/clk/nxp/clk-lpc32xx.c                      |   12 +-
 include/dt-bindings/clock/stm32h7-clks.h           |  165 +++
 include/dt-bindings/mfd/stm32h7-rcc.h              |  136 ++
 include/linux/clk-provider.h                       |    1 +
 8 files changed, 1802 insertions(+), 7 deletions(-)
 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

-- 
1.9.1

[toc] | [next] | [standalone]

#1691715 — [PATCH v7 2/3] clk: gate: expose clk_gate_ops::is_enabled

From: Gabriel Fernandez <gabriel.fernandez@st.com>

This patch exposes clk_gate_ops::is_enabled as functions
that can be directly called and assigned in places like this so
we don't need wrapper functions that do nothing besides forward
the call.

Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
Sugested by Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/clk-gate.c       | 3 ++-
 include/linux/clk-provider.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 4e0c054a..dd82485 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -86,7 +86,7 @@ static void clk_gate_disable(struct clk_hw *hw)
 	clk_gate_endisable(hw, 0);
 }
 
-static int clk_gate_is_enabled(struct clk_hw *hw)
+int clk_gate_is_enabled(struct clk_hw *hw)
 {
 	u32 reg;
 	struct clk_gate *gate = to_clk_gate(hw);
@@ -101,6 +101,7 @@ static int clk_gate_is_enabled(struct clk_hw *hw)
 
 	return reg ? 1 : 0;
 }
+EXPORT_SYMBOL_GPL(clk_gate_is_enabled);
 
 const struct clk_ops clk_gate_ops = {
 	.enable = clk_gate_enable,
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index c59c625..e9587ab 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -343,6 +343,7 @@ struct clk_hw *clk_hw_register_gate(struct device *dev, const char *name,
 		u8 clk_gate_flags, spinlock_t *lock);
 void clk_unregister_gate(struct clk *clk);
 void clk_hw_unregister_gate(struct clk_hw *hw);
+int clk_gate_is_enabled(struct clk_hw *hw);
 
 struct clk_div_table {
 	unsigned int	val;
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1692199 — Re: [PATCH v7 2/3] clk: gate: expose clk_gate_ops::is_enabled

On 07/19/2017 05:25 PM, gabriel.fernandez@st.com wrote:
> From: Gabriel Fernandez <gabriel.fernandez@st.com>
> 
> This patch exposes clk_gate_ops::is_enabled as functions
> that can be directly called and assigned in places like this so
> we don't need wrapper functions that do nothing besides forward
> the call.
> 
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> Sugested by Stephen Boyd <sboyd@codeaurora.org>

Fixing the spelling etc. I believe the last tag line should be

Suggested-by: Stephen Boyd <sboyd@codeaurora.org>

--
With best wishes,
Vladimir

[toc] | [prev] | [next] | [standalone]

#1691719 — [PATCH v7 1/3] clk: nxp: clk-lpc32xx: rename clk_gate_is_enabled()

From: Gabriel Fernandez <gabriel.fernandez@st.com>

We need to export clk_gate_is_enabled() from clk framework, then
to avoid compilation issue we have to rename clk_gate_is_enabled()
in NXP LPC32xx clock driver.
We changed all gate op with 'lpc32xx_' prefix:
lpc32xx_clk_gate_enable(),
lpc32xx_clk_gate_disable(),
lpc32xx_clk_gate_is_enabled().

Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
---
 drivers/clk/nxp/clk-lpc32xx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/nxp/clk-lpc32xx.c b/drivers/clk/nxp/clk-lpc32xx.c
index 5b98ff9..7b359af 100644
--- a/drivers/clk/nxp/clk-lpc32xx.c
+++ b/drivers/clk/nxp/clk-lpc32xx.c
@@ -885,7 +885,7 @@ static unsigned long clk_usb_i2c_recalc_rate(struct clk_hw *hw,
 	.recalc_rate = clk_usb_i2c_recalc_rate,
 };
 
-static int clk_gate_enable(struct clk_hw *hw)
+static int lpc32xx_clk_gate_enable(struct clk_hw *hw)
 {
 	struct lpc32xx_clk_gate *clk = to_lpc32xx_gate(hw);
 	u32 mask = BIT(clk->bit_idx);
@@ -894,7 +894,7 @@ static int clk_gate_enable(struct clk_hw *hw)
 	return regmap_update_bits(clk_regmap, clk->reg, mask, val);
 }
 
-static void clk_gate_disable(struct clk_hw *hw)
+static void lpc32xx_clk_gate_disable(struct clk_hw *hw)
 {
 	struct lpc32xx_clk_gate *clk = to_lpc32xx_gate(hw);
 	u32 mask = BIT(clk->bit_idx);
@@ -903,7 +903,7 @@ static void clk_gate_disable(struct clk_hw *hw)
 	regmap_update_bits(clk_regmap, clk->reg, mask, val);
 }
 
-static int clk_gate_is_enabled(struct clk_hw *hw)
+static int lpc32xx_clk_gate_is_enabled(struct clk_hw *hw)
 {
 	struct lpc32xx_clk_gate *clk = to_lpc32xx_gate(hw);
 	u32 val;
@@ -916,9 +916,9 @@ static int clk_gate_is_enabled(struct clk_hw *hw)
 }
 
 static const struct clk_ops lpc32xx_clk_gate_ops = {
-	.enable = clk_gate_enable,
-	.disable = clk_gate_disable,
-	.is_enabled = clk_gate_is_enabled,
+	.enable = lpc32xx_clk_gate_enable,
+	.disable = lpc32xx_clk_gate_disable,
+	.is_enabled = lpc32xx_clk_gate_is_enabled,
 };
 
 #define div_mask(width)	((1 << (width)) - 1)
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1692197 — Re: [PATCH v7 1/3] clk: nxp: clk-lpc32xx: rename clk_gate_is_enabled()

Hello Gabriel,

On 07/19/2017 05:25 PM, gabriel.fernandez@st.com wrote:
> From: Gabriel Fernandez <gabriel.fernandez@st.com>
> 
> We need to export clk_gate_is_enabled() from clk framework, then
> to avoid compilation issue we have to rename clk_gate_is_enabled()
> in NXP LPC32xx clock driver.
> We changed all gate op with 'lpc32xx_' prefix:
> lpc32xx_clk_gate_enable(),
> lpc32xx_clk_gate_disable(),
> lpc32xx_clk_gate_is_enabled().
> 
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>

Acked-by: Vladimir Zapolskiy <vz@mleia.com>

--
With best wishes,
Vladimir

[toc] | [prev] | [next] | [standalone]

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

Hello Gabriel,

On 07/19/2017 05:25 PM, gabriel.fernandez@st.com wrote:
> From: Gabriel Fernandez <gabriel.fernandez@st.com>
> 
> This patch enables clocks for STM32H743 boards.
> 
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> 
> for MFD changes:
> Acked-by: Lee Jones <lee.jones@linaro.org>
> 
> for DT-Bindings
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  .../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

[toc] | [prev] | [next] | [standalone]

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

Hi Vladimir,


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 <gabriel.fernandez@st.com>
>>
>> This patch enables clocks for STM32H743 boards.
>>
>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
>>
>> for MFD changes:
>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>
>> for DT-Bindings
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>   .../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:
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 = <&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.
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@40000c00 {
>> +			compatible = "st,stm32-timer";
>> +			reg = <0x40000c00 0x400>;
>> +			interrupts = <50>;
>> +			clocks = <&rcc 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 ---------->                                           ^^^^
ok
> 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:'
ok
>> +
>> +	timer2 {
>> +		resets	= <&rcc STM32H7_APB1L_RESET(TIM2)>;
>> +	};
> --
> With best wishes,
> Vladimir
Best Regards
Gabriel

[toc] | [prev] | [next] | [standalone]

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

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.fernandez@st.com wrote:
>>> From: Gabriel Fernandez <gabriel.fernandez@st.com>
>>>
>>> This patch enables clocks for STM32H743 boards.
>>>
>>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
>>>
>>> for MFD changes:
>>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>>
>>> for DT-Bindings
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> ---
>>>   .../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:
> 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

[toc] | [prev] | [next] | [standalone]

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

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.fernandez@st.com wrote:
> >>> From: Gabriel Fernandez <gabriel.fernandez@st.com>
> >>> +
> >>> +	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

[toc] | [prev] | [next] | [standalone]

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

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.fernandez@st.com wrote:
>>>>> From: Gabriel Fernandez <gabriel.fernandez@st.com>
>>>>> +
>>>>> +	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

[toc] | [prev] | [next] | [standalone]

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

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 <gabriel.fernandez@st.com>
>>
>> This patch enables clocks for STM32H743 boards.
>>
>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
>>
>> for MFD changes:
>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>
>> for DT-Bindings
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>>   .../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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web