Hi Stephen,

Thanks for reviewing.

On 06/22/2017 12:07 AM, Stephen Boyd wrote:

On 06/07, gabriel.fernandez@st.com wrote:

quoted

From: Gabriel Fernandez <redacted>

This patch enables clocks for STM32H743 boards.

Signed-off-by: Gabriel Fernandez <redacted>

for MFD changes:
Acked-by: Lee Jones <redacted>

for DT-Bindings
Acked-by: Rob Herring <robh@kernel.org>
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)

Please drop the changelog from commit text.

strange, i added the changelog after 'git format-patch'

quoted

diff --git a/drivers/clk/clk-stm32h7.c b/drivers/clk/clk-stm32h7.c
new file mode 100644
index 0000000..2907c1f
--- /dev/null
+++ b/drivers/clk/clk-stm32h7.c

@@ -0,0 +1,1532 @@
+/* Power domain helper */
+static inline void disable_power_domain_write_protection(void)
+{
+	if (pdrm)
+		regmap_update_bits(pdrm, 0x00, (1 << 8), (1 << 8));
+}
+
+static inline void enable_power_domain_write_protection(void)
+{
+	if (pdrm)
+		regmap_update_bits(pdrm, 0x00, (1 << 8), (0 << 8));
+}
+
+static inline int is_enable_power_domain_write_protection(void)

Return bool, not int?

ok

quoted

+{
+	if (pdrm) {
+		u32 val;
+
+		regmap_read(pdrm, 0x00, &val);
+
+		return !(val & 0x100);
+	}
+	return -1;

Returning -1 looks odd.

ok i will change it

quoted

+}
+
+/* Gate clock with ready bit and backup domain management */
+struct stm32_ready_gate {
+	struct	clk_gate gate;
+	u8	bit_rdy;
+	u8	backup_domain;
+};
+
+#define to_ready_gate_clk(_rgate) container_of(_rgate, struct stm32_ready_gate,\
+		gate)
+
+#define RGATE_TIMEOUT 600000
+
+static int ready_gate_clk_is_enabled(struct clk_hw *hw)
+{
+	return clk_gate_ops.is_enabled(hw);
+}

Perhaps we should expose 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.

ok i will add a patch in clk.c and clk-provider.h to export 'clk_gate_is_enabled'

quoted

+
+static int ready_gate_clk_enable(struct clk_hw *hw)
+{
+	struct clk_gate *gate = to_clk_gate(hw);
+	struct stm32_ready_gate *rgate = to_ready_gate_clk(gate);
+	int dbp_status;
+	int bit_status;
+	unsigned int timeout = RGATE_TIMEOUT;
+
+	if (clk_gate_ops.is_enabled(hw))
+		return 0;
+
+	dbp_status = is_enable_power_domain_write_protection();
+
+	if (rgate->backup_domain && dbp_status)
+		disable_power_domain_write_protection();
+
+	clk_gate_ops.enable(hw);
+
+	do {
+		bit_status = !(readl(gate->reg) & BIT(rgate->bit_rdy));
+
+		if (bit_status)
+			udelay(1000);
+
+	} while (bit_status && --timeout);

readl_poll_timeout?

last time it didn't work, i will investigate again

quoted

+
+/* RTC clock */
+static u8 rtc_mux_get_parent(struct clk_hw *hw)
+{
+	return clk_mux_ops.get_parent(hw);
+}
+
+static int rtc_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	int dbp_status;
+	int err;
+
+	dbp_status = is_enable_power_domain_write_protection();
+
+	if (dbp_status)
+		disable_power_domain_write_protection();
+
+	err = clk_mux_ops.set_parent(hw, index);
+
+	if (dbp_status)
+		enable_power_domain_write_protection();
+
+	return err;
+}
+
+static int rtc_mux_determine_rate(struct clk_hw *hw,
+		struct clk_rate_request *req)
+{
+	return clk_mux_ops.determine_rate(hw, req);
+}

In this case we have that function exposed already so it could
be assigned.

ok i will use __clk_mux_determine_rate

quoted

+
+static const struct clk_ops rtc_mux_ops = {
+	.get_parent	= rtc_mux_get_parent,
+	.set_parent	= rtc_mux_set_parent,
+	.determine_rate = rtc_mux_determine_rate,
+};
+
+/* Clock gate with backup domain protection management */
+static int bd_gate_is_enabled(struct clk_hw *hw)
+{
+	return clk_gate_ops.is_enabled(hw);
+}
+
+static int bd_gate_enable(struct clk_hw *hw)
+{
+	int dbp_status;
+	int err;
+
+	if (bd_gate_is_enabled(hw))
+		return 0;
+

[...]

quoted

+
+	return;
+
+err_free_clks:
+	kfree(clk_data);
  +}
+
+CLK_OF_DECLARE_DRIVER(stm32h7_rcc, "st,stm32h743-rcc", stm32h7_rcc_init);

Can you add a comment above this why we can't do a split design
with a platform driver and a CLK_OF_DECLARE_DRIVER() routine here
and also mention the other driver that's probing against the same
compatible?

ok