Hi Vladimi, Many thanks for the code review
On 07/18/2017 10:19 PM, Vladimir Zapolskiy wrote: > Hello Gabriel, > > On 07/18/2017 10:53 AM, gabriel.fernan...@st.com wrote: >> From: Gabriel Fernandez <gabriel.fernan...@st.com> >> >> This patch enables clocks for STM32H743 boards. >> >> Signed-off-by: Gabriel Fernandez <gabriel.fernan...@st.com> >> >> for MFD changes: >> Acked-by: Lee Jones <lee.jo...@linaro.org> >> >> for DT-Bindings >> Acked-by: Rob Herring <r...@kernel.org> >> --- >> .../devicetree/bindings/clock/st,stm32h7-rcc.txt | 81 ++ >> drivers/clk/Makefile | 1 + >> drivers/clk/clk-stm32h7.c | 1522 >> ++++++++++++++++++++ >> include/dt-bindings/clock/stm32h7-clks.h | 165 +++ >> include/dt-bindings/mfd/stm32h7-rcc.h | 136 ++ >> 5 files changed, 1905 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..e41e4ac >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt >> @@ -0,0 +1,81 @@ >> +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) >> + >> +- st,syscfg: phandle for pwrcfg, mandatory to disable/enable backup domain >> + write protection (RTC clock). >> + > please make a clear decision if "st,syscfg" property is mandatory or not. > From the driver code this property is optional, and the clock driver > is expected to work properly, if the property is omitted. Do I miss > anything? You right, in the driver code it's optional. I will change it in dt binding documentation. > >> diff --git a/drivers/clk/clk-stm32h7.c b/drivers/clk/clk-stm32h7.c >> new file mode 100644 >> index 0000000..2608c40 >> --- /dev/null >> +++ b/drivers/clk/clk-stm32h7.c > [snip] > >> +static const char * const ltdc_src[] = {"pll3_r"}; >> + >> +/* 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)); > (0 << 8) is zero. ok > >> +} > IMHO a version below is better: > > static inline void enable_power_domain_write_protection(bool enable) > { > if (pdrm) > regmap_update_bits(pdrm, 0x00, BIT(8), enable ? 0x0: BIT(8)); > } > >> + >> +static inline bool is_enable_power_domain_write_protection(void) > is_enabled_... ok >> +{ >> + if (pdrm) { >> + u32 val; >> + >> + regmap_read(pdrm, 0x00, &val); >> + >> + return !(val & 0x100); >> + } >> + return 0; >> +} > Please replace (1 << 8) and 0x100 all above with a macro or at least > BIT(8). ok >> + >> +/* 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 10000 >> + >> +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); >> + >> + /* We can't use readl_poll_timeout() because we can blocked if >> + * someone enables this clock before clocksource changes. >> + * Only jiffies counter is available. Jiffies are incremented by >> + * interruptions and enable op does not allow to be interrupted. >> + */ >> + do { >> + bit_status = !(readl(gate->reg) & BIT(rgate->bit_rdy)); >> + >> + if (bit_status) >> + udelay(100); >> + >> + } while (bit_status && --timeout); >> + >> + if (rgate->backup_domain && dbp_status) >> + enable_power_domain_write_protection(); >> + >> + return bit_status; >> +} > I'm not convinced that the repetitive lockless pattern > > dbp_status = is_enable_power_domain_write_protection(); > if (dbp_status) > disable_power_domain_write_protection(); > > do_something(); > > if (dbp_status) > enable_power_domain_write_protection(); > > is correct in a concurrent environment. > > You still have a risk of running do_something() *after* a call of > enable_power_domain_write_protection(). Humm, after long discussion with the hardware ip owner, he recommended to disable power domain write protection only one time at the init (don't disable/enable dynamically). Then i can suppress this code dbp_status = is_enable_power_domain_write_protection(); if (dbp_status) disable_power_domain_write_protection() ... if (dbp_status) enable_power_domain_write_protection(); Best regards Gabriel > The best option for you is either to switch to ordinary power domains > and use their API or to move the driver to use regmaps, and at least > in the latter case you don't need to wrap your code by CCF code (!), > and as a result you don't need to export truly internal to CCF function > clk_gate_is_enabled(). > > -- > With best wishes, > Vladimir