Message-ID: <20170405223233.GJ7065@codeaurora.org>
Date:   Wed, 5 Apr 2017 15:32:33 -0700
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     gabriel.fernandez@...com
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Michael Turquette <mturquette@...libre.com>,
        Nicolas Pitre <nico@...aro.org>, Arnd Bergmann <arnd@...db.de>,
        daniel.thompson@...aro.org, andrea.merello@...il.com,
        radoslaw.pietrzyk@...il.com, Lee Jones <lee.jones@...aro.org>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
        ludovic.barre@...com, olivier.bideau@...com, amelie.delaunay@...com
Subject: Re: [PATCH] clk: stm32h7: Add stm32h743 clock driver

On 03/15, gabriel.fernandez@...com wrote:
> From: Gabriel Fernandez <gabriel.fernandez@...com>
> 
> 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@...24400 {
> +		#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@...24430 {
> +			#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@...24438 {
> +			#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 <gabriel.fernandez@...com>
> + *
> + * 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 <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clk.h>

Is this used?

> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/stm32h7-clks.h>
> +
> +/* 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

Powered by blists - more mailing lists