Re: [PATCH] clk: stm32h7: Add stm32h743 clock driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Stephen,


On 04/06/2017 12:32 AM, Stephen Boyd wrote:
On 03/15, gabriel.fernandez@xxxxxx wrote:
From: Gabriel Fernandez <gabriel.fernandez@xxxxxx>

This patch enables clocks for STM32H743 boards.
Like what clocks exactly? All of them?

Yes all of them, it's new IP


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.
ok i fixed it in the v2


+		};
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?

Because i need to offer the possibility to change the PLL VCO frequencies at the start-up of this driver clock. The VCO algorithm needs a division factor, a multiplication factor and a fractional factor. Lot's of solution are possible for one frequency and it's nightmare to satisfy the 3 output dividers of the PLL.


+
+		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.
These properties are optionals.
I absolute need it to custumize VCO frequencies of a pll without the boot loader..

i suppressed  "st,frac-status" and  "st,pllrge" in the v2

+		};
+	};
+
+
+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?
ok i will fix it


+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@xxxxxx>
+ *
+ * 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?
No i will suppress it


+#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?
ok


+
+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.
ok


+	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?
ok il will fix it.

+	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.
Yes the reset controler.
ok il will add a comment.


BR
Gabriel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux