Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756112AbdDEWcm (ORCPT ); Wed, 5 Apr 2017 18:32:42 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:40568 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750825AbdDEWch (ORCPT ); Wed, 5 Apr 2017 18:32:37 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org F33D960708 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sboyd@codeaurora.org Date: Wed, 5 Apr 2017 15:32:33 -0700 From: Stephen Boyd To: gabriel.fernandez@st.com 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@st.com, olivier.bideau@st.com, amelie.delaunay@st.com Subject: Re: [PATCH] clk: stm32h7: Add stm32h743 clock driver Message-ID: <20170405223233.GJ7065@codeaurora.org> References: <1489569810-24350-1-git-send-email-gabriel.fernandez@st.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1489569810-24350-1-git-send-email-gabriel.fernandez@st.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8027 Lines: 283 On 03/15, gabriel.fernandez@st.com wrote: > From: Gabriel Fernandez > > This patch enables clocks for STM32H743 boards. Like what clocks exactly? All of them? > 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..9d4b587 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt > @@ -0,0 +1,152 @@ > +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). > + > +- pll x node: Allow to register a pll with specific parameters. > + Please see PLL section below. > + > +Example: > + > + rcc: rcc@58024400 { > + #reset-cells = <1>; > + #clock-cells = <2> > + 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>; > + > + vco1@58024430 { > + #clock-cells = <0>; > + compatible = "stm32,pll"; > + reg = <0>; reg is super confusing and doesn't match unit address. > + }; Why? Shouldn't we know this from the compatible string how many PLLs there are and where they're located? Export the PLLs through rcc node's clock-cells? > + > + vco2@58024438 { > + #clock-cells = <0>; > + compatible = "stm32,pll"; > + reg = <1>; reg is super confusing and doesn't match unit address. > + st,clock-div = <2>; > + st,clock-mult = <40>; > + st,frac-status = <0>; > + st,frac = <0>; > + st,vcosel = <1>; > + st,pllrge = <2>; Does this stuff change on a per-board basis? I hope none of these properties need to be in DT. > + }; > + }; > + > + > +STM32H7 PLL > +----------- > + [...] > + > +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 One too many slashes? > +header and can be used in device tree sources. > + > +example: > + > + timer2 { > + resets = <&rcc STM32H7_APB1L_RESET(TIM2)>; > + }; > diff --git a/drivers/clk/clk-stm32h7.c b/drivers/clk/clk-stm32h7.c > new file mode 100644 > index 0000000..c8eb729 > --- /dev/null > +++ b/drivers/clk/clk-stm32h7.c > @@ -0,0 +1,1586 @@ > +/* > + * Copyright (C) Gabriel Fernandez 2017 > + * Author: Gabriel Fernandez > + * > + * License terms: GPL V2.0. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program. If not, see . > + */ > + > +#include Is this used? > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +/* Reset Clock Control Registers */ > +#define RCC_CR 0x00 > +#define RCC_CFGR 0x10 > +#define RCC_D1CFGR 0x18 > +#define RCC_D2CFGR 0x1C > +#define RCC_D3CFGR 0x20 > +#define RCC_PLLCKSELR 0x28 > +#define RCC_PLLCFGR 0x2C > +#define RCC_PLL1DIVR 0x30 > +#define RCC_PLL1FRACR 0x34 > +#define RCC_PLL2DIVR 0x38 > +#define RCC_PLL2FRACR 0x3C > +#define RCC_PLL3DIVR 0x40 > +#define RCC_PLL3FRACR 0x44 > +#define RCC_D1CCIPR 0x4C > +#define RCC_D2CCIP1R 0x50 > +#define RCC_D2CCIP2R 0x54 > +#define RCC_D3CCIPR 0x58 > +#define RCC_BDCR 0x70 > +#define RCC_CSR 0x74 > +#define RCC_AHB3ENR 0xD4 > +#define RCC_AHB1ENR 0xD8 > +#define RCC_AHB2ENR 0xDC > +#define RCC_AHB4ENR 0xE0 > +#define RCC_APB3ENR 0xE4 > +#define RCC_APB1LENR 0xE8 > +#define RCC_APB1HENR 0xEC > +#define RCC_APB2ENR 0xF0 > +#define RCC_APB4ENR 0xF4 > + > +static DEFINE_SPINLOCK(rlock); This is super generic and will make lockdep debugging sad. Perhaps stm32rcc_lock? > + > +static void __iomem *base; > +static struct regmap *pdrm; > +static struct clk_hw **hws; > + > +/* System clock parent */ > +static const char * const sys_src[] = { > + "hsi_ck", "csi_ck", "hse_ck", "pll1_p" }; > + > +static const char * const tracein_src[] = { > + "hsi_ck", "csi_ck", "hse_ck", "pll1_r" }; [...] > + > +static unsigned long pll_fd_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct stm32_pll_obj *clk_elem = to_pll(hw); > + struct stm32_fractional_divider *fd = &clk_elem->div; > + unsigned long m, n; > + u32 val, mask; > + u64 rate, rate1 = 0; > + > + val = clk_readl(fd->mreg); Please don't use clk_readl() unless you need it for some reason. > + mask = (GENMASK(fd->mwidth - 1, 0) << fd->mshift); > + m = (val & mask) >> fd->mshift; > + > + val = clk_readl(fd->nreg); > + mask = (GENMASK(fd->nwidth - 1, 0) << fd->nshift); Useless parentheses. And isn't GENMASK supposed to take the actual bit positions? Then we avoid overflow issues? > + n = ((val & mask) >> fd->nshift) + 1; > + > + if (!n || !m) > + return parent_rate; > + > + rate = (u64)parent_rate * n; > + do_div(rate, m); > + > + if (pll_frac_is_enabled(hw)) { > + val = pll_read_frac(hw); > + rate1 = (u64) parent_rate * (u64) val; > + do_div(rate1, (m * 8191)); > + } > + > + return rate + rate1; > +} > + [...] > + > + /* Micro-controller clocks */ > + for (n = 0; n < ARRAY_SIZE(mco_clk); n++) { > + get_cfg_composite_div(&mco_clk_cfg, &mco_clk[n], &c_cfg, > + &rlock); > + > + hws[MCO_BANK + n] = clk_hw_register_composite(NULL, > + mco_clk[n].name, > + mco_clk[n].parent_name, > + mco_clk[n].num_parents, > + c_cfg.mux_hw, c_cfg.mux_ops, > + c_cfg.div_hw, c_cfg.div_ops, > + c_cfg.gate_hw, c_cfg.gate_ops, > + mco_clk[n].flags); > + } > + > + of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_data); > + > + return; > + > +err_free_clks: > + kfree(clk_data); > +} > +CLK_OF_DECLARE_DRIVER(stm32h7_rcc, "st,stm32h743-rcc", stm32h7_rcc_init); Is there another driver that uses the same register space? Nothing showing up in -next right now. Perhaps a comment should be added to indicate the other driver. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project