Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752002AbdFVOVX (ORCPT ); Thu, 22 Jun 2017 10:21:23 -0400 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:34253 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751094AbdFVOVV (ORCPT ); Thu, 22 Jun 2017 10:21:21 -0400 From: Gabriel FERNANDEZ To: Stephen Boyd CC: Rob Herring , Mark Rutland , Russell King , Maxime Coquelin , Alexandre TORGUE , Michael Turquette , Nicolas Pitre , Arnd Bergmann , "daniel.thompson@linaro.org" , "andrea.merello@gmail.com" , "radoslaw.pietrzyk@gmail.com" , Lee Jones , "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" Subject: Re: [RESEND PATCH v4] clk: stm32h7: Add stm32h743 clock driver Thread-Topic: [RESEND PATCH v4] clk: stm32h7: Add stm32h743 clock driver Thread-Index: AQHS31uuzVmODfuWeEOg6i19U5EFNaIv1S6AgAEP/QA= Date: Thu, 22 Jun 2017 14:20:33 +0000 Message-ID: <18dca3a5-6e9d-fdd5-d2a3-614d652199a6@st.com> References: <1496818794-14771-1-git-send-email-gabriel.fernandez@st.com> <20170621220703.GI4493@codeaurora.org> In-Reply-To: <20170621220703.GI4493@codeaurora.org> 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.1.1 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.50] Content-Type: text/plain; charset="utf-8" Content-ID: <190DD016E189A54FAB36C3C93D9D099A@st.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-06-22_06:,, 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 mail.home.local id v5MELYDQ010046 Content-Length: 5398 Lines: 213 Hi Stephen, Thanks for reviewing. On 06/22/2017 12:07 AM, Stephen Boyd wrote: > On 06/07, 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 >> 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' > >> 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 > >> +{ >> + if (pdrm) { >> + u32 val; >> + >> + regmap_read(pdrm, 0x00, &val); >> + >> + return !(val & 0x100); >> + } >> + return -1; > Returning -1 looks odd. ok i will change it > >> +} >> + >> +/* 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' > >> + >> +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 >> + >> +/* 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 >> + >> +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; >> + > [...] >> + >> + 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