Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752492AbdGSNuy (ORCPT ); Wed, 19 Jul 2017 09:50:54 -0400 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:35424 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751370AbdGSNuv (ORCPT ); Wed, 19 Jul 2017 09:50:51 -0400 From: Gabriel FERNANDEZ To: Vladimir Zapolskiy CC: Rob Herring , Mark Rutland , Russell King , Maxime Coquelin , Alexandre TORGUE , Michael Turquette , Stephen Boyd , Nicolas Pitre , Arnd Bergmann , "daniel.thompson@linaro.org" , "andrea.merello@gmail.com" , "radoslaw.pietrzyk@gmail.com" , Lee Jones , Sylvain Lemieux , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "linux-clk@vger.kernel.org" , Ludovic BARRE , "Olivier BIDEAU" , Amelie DELAUNAY , "gabriel.fernandez.st@gmail.com" , "Arvind Yadav" Subject: Re: [PATCH v6 3/3] clk: stm32h7: Add stm32h743 clock driver Thread-Topic: [PATCH v6 3/3] clk: stm32h7: Add stm32h743 clock driver Thread-Index: AQHS/5r7twuQH4yT60SvWZHaaX/lf6JZ5ZoAgAElegA= Date: Wed, 19 Jul 2017 13:49:59 +0000 Message-ID: References: <1500364414-10021-1-git-send-email-gabriel.fernandez@st.com> <1500364414-10021-4-git-send-email-gabriel.fernandez@st.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.47] Content-Type: text/plain; charset="utf-8" Content-ID: <01930AD552F17C4D8C1F9A7AAA0F649B@st.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-07-19_09:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v6JDp1QZ001901 Content-Length: 6409 Lines: 217 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.fernandez@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 | 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