[PATCH v2 0/5] clk: Add Aspeed clock driver

[PATCH v2 0/5] clk: Add Aspeed clock driver

[Joel Stanley]

This driver supports the ast2500, ast2400 (and derivative) BMC SoCs from
Aspeed.

This is version two of the series. Version one contained two patches; an update
to the binding document and a single patch for the driver. Lee has merged the
bindings change, so that is dropped from this series, and I split the driver
out into a series of patches to make them easier to review.

All of the important clocks are supported, with most non-essential ones
also implemented where information is available. I am working with
Aspeed to clear up some of the missing information, including the
missing parent-sibling relationships.

We need to know the rate of the apb clock in order to correctly program
the clocksource driver, so the apb and it's parents are created in the
CLK_OF_DECLARE_DRIVER callback.

The rest of the clocks are created at normal driver probe time. I
followed the Gemini driver's lead with using the regmap where I could,
but also having a pointer to the base address for use with the common
clock callbacks.

The driver borrows from the clk_gate common clock infra spruce, but modifies
it in order to support the clock gate and reset pair that most of the clocks
have. This pair must be reset-ungated-released, with appropriate delays,
according to the datasheet.

The first patch introduces the core clock registration parts, and describes
the clocks. The second creates the core clocks, giving the system enough to
boot (but without uart). Next come the non-core clocks, and finally the reset
controller that is used for the few cocks that don't have a gate to go with their
reset pair.

Please review!

Cheers,

Joel


Joel Stanley (5):
  clk: Add clock driver for ASPEED BMC SoCs
  clk: aspeed: Register core clocks
  clk: aspeed: Add platform driver and register PLLs
  clk: aspeed: Register gated clocks
  clk: aspeed: Add reset controller

 drivers/clk/Kconfig                      |  12 +
 drivers/clk/Makefile                     |   1 +
 drivers/clk/clk-aspeed.c                 | 645 +++++++++++++++++++++++++++++++
 include/dt-bindings/clock/aspeed-clock.h |  52 +++
 4 files changed, 710 insertions(+)
 create mode 100644 drivers/clk/clk-aspeed.c
 create mode 100644 include/dt-bindings/clock/aspeed-clock.h

-- 
2.14.1

[PATCH v2 0/5] clk: Add Aspeed clock driver

[Joel Stanley]

This driver supports the ast2500, ast2400 (and derivative) BMC SoCs from
Aspeed.

This is version two of the series. Version one contained two patches; an update
to the binding document and a single patch for the driver. Lee has merged the
bindings change, so that is dropped from this series, and I split the driver
out into a series of patches to make them easier to review.

All of the important clocks are supported, with most non-essential ones
also implemented where information is available. I am working with
Aspeed to clear up some of the missing information, including the
missing parent-sibling relationships.

We need to know the rate of the apb clock in order to correctly program
the clocksource driver, so the apb and it's parents are created in the
CLK_OF_DECLARE_DRIVER callback.

The rest of the clocks are created at normal driver probe time. I
followed the Gemini driver's lead with using the regmap where I could,
but also having a pointer to the base address for use with the common
clock callbacks.

The driver borrows from the clk_gate common clock infra spruce, but modifies
it in order to support the clock gate and reset pair that most of the clocks
have. This pair must be reset-ungated-released, with appropriate delays,
according to the datasheet.

The first patch introduces the core clock registration parts, and describes
the clocks. The second creates the core clocks, giving the system enough to
boot (but without uart). Next come the non-core clocks, and finally the reset
controller that is used for the few cocks that don't have a gate to go with their
reset pair.

Please review!

Cheers,

Joel


Joel Stanley (5):
  clk: Add clock driver for ASPEED BMC SoCs
  clk: aspeed: Register core clocks
  clk: aspeed: Add platform driver and register PLLs
  clk: aspeed: Register gated clocks
  clk: aspeed: Add reset controller

 drivers/clk/Kconfig                      |  12 +
 drivers/clk/Makefile                     |   1 +
 drivers/clk/clk-aspeed.c                 | 645 +++++++++++++++++++++++++++++++
 include/dt-bindings/clock/aspeed-clock.h |  52 +++
 4 files changed, 710 insertions(+)
 create mode 100644 drivers/clk/clk-aspeed.c
 create mode 100644 include/dt-bindings/clock/aspeed-clock.h

-- 
2.14.1

[PATCH v2 1/5] clk: Add clock driver for ASPEED BMC SoCs

[Joel Stanley]

This adds the stub of a driver for the ASPEED SoCs. The clocks are
defined and the static registration is set up.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/Kconfig                      |  12 +++
 drivers/clk/Makefile                     |   1 +
 drivers/clk/clk-aspeed.c                 | 162 +++++++++++++++++++++++++++++++
 include/dt-bindings/clock/aspeed-clock.h |  43 ++++++++
 4 files changed, 218 insertions(+)
 create mode 100644 drivers/clk/clk-aspeed.c
 create mode 100644 include/dt-bindings/clock/aspeed-clock.h

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 1c4e1aa6767e..9abe063ef8d2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -142,6 +142,18 @@ config COMMON_CLK_GEMINI
 	  This driver supports the SoC clocks on the Cortina Systems Gemini
 	  platform, also known as SL3516 or CS3516.
 
+config COMMON_CLK_ASPEED
+	bool "Clock driver for Aspeed BMC SoCs"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	default ARCH_ASPEED
+	select MFD_SYSCON
+	select RESET_CONTROLLER
+	---help---
+	  This driver supports the SoC clocks on the Aspeed BMC platforms.
+
+	  The G4 and G5 series, including the ast2400 and ast2500, are supported
+	  by this driver.
+
 config COMMON_CLK_S2MPS11
 	tristate "Clock driver for S2MPS1X/S5M8767 MFD"
 	depends on MFD_SEC_CORE || COMPILE_TEST
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index c99f363826f0..575c68919d9b 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
 obj-$(CONFIG_COMMON_CLK_CS2000_CP)	+= clk-cs2000-cp.o
 obj-$(CONFIG_ARCH_EFM32)		+= clk-efm32gg.o
 obj-$(CONFIG_COMMON_CLK_GEMINI)		+= clk-gemini.o
+obj-$(CONFIG_COMMON_CLK_ASPEED)		+= clk-aspeed.o
 obj-$(CONFIG_ARCH_HIGHBANK)		+= clk-highbank.o
 obj-$(CONFIG_CLK_HSDK)			+= clk-hsdk-pll.o
 obj-$(CONFIG_COMMON_CLK_MAX77686)	+= clk-max77686.o
diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
new file mode 100644
index 000000000000..824c54767009
--- /dev/null
+++ b/drivers/clk/clk-aspeed.c
@@ -0,0 +1,162 @@
+/*
+ * Copyright 2017 IBM Corporation
+ *
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#define pr_fmt(fmt) "clk-aspeed: " fmt
+
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/clk-provider.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+#include <linux/mfd/syscon.h>
+
+#include <dt-bindings/clock/aspeed-clock.h>
+
+#define ASPEED_RESET_CTRL	0x04
+#define ASPEED_CLK_SELECTION	0x08
+#define ASPEED_CLK_STOP_CTRL	0x0c
+#define ASPEED_MPLL_PARAM	0x20
+#define ASPEED_HPLL_PARAM	0x24
+#define ASPEED_MISC_CTRL	0x2c
+#define ASPEED_STRAP		0x70
+
+/* Keeps track of all clocks */
+static struct clk_hw_onecell_data *aspeed_clk_data;
+
+static void __iomem *scu_base;
+
+/**
+ * struct aspeed_gate_data - Aspeed gated clocks
+ * @clock_idx: bit used to gate this clock in the clock register
+ * @reset_idx: bit used to reset this IP in the reset register. -1 if no
+ *             reset is required when enabling the clock
+ * @name: the clock name
+ * @parent_name: the name of the parent clock
+ * @flags: standard clock framework flags
+ */
+struct aspeed_gate_data {
+	u8		clock_idx;
+	s8		reset_idx;
+	const char	*name;
+	const char	*parent_name;
+	unsigned long	flags;
+};
+
+/**
+ * struct aspeed_clk_gate - Aspeed specific clk_gate structure
+ * @hw:		handle between common and hardware-specific interfaces
+ * @reg:	register controlling gate
+ * @clock_idx:	bit used to gate this clock in the clock register
+ * @reset_idx:	bit used to reset this IP in the reset register. -1 if no
+ *		reset is required when enabling the clock
+ * @flags:	hardware-specific flags
+ * @lock:	register lock
+ *
+ * Some of the clocks in the Aspeed SoC must be put in reset before enabling.
+ * This modified version of clk_gate allows an optional reset bit to be
+ * specified.
+ */
+struct aspeed_clk_gate {
+	struct clk_hw	hw;
+	struct regmap	*map;
+	u8		clock_idx;
+	s8		reset_idx;
+	u8		flags;
+	spinlock_t	*lock;
+};
+
+#define to_aspeed_clk_gate(_hw) container_of(_hw, struct aspeed_clk_gate, hw)
+
+/* TODO: ask Aspeed about the actual parent data */
+static const struct aspeed_gate_data aspeed_gates[] __initconst = {
+/*	 clk rst   name			parent		flags	*/
+	{  0, -1, "eclk-gate",		"eclk",		0 }, /* Video Engine */
+	{  1,  7, "gclk-gate",		NULL,		0 }, /* 2D engine */
+	{  2, -1, "mclk-gate",		"mpll",		CLK_IS_CRITICAL }, /* SDRAM */
+	{  3,  6, "vclk-gate",		NULL,		0 }, /* Video Capture */
+	{  4, 10, "bclk-gate",		"bclk",		0 }, /* PCIe/PCI */
+	{  5, -1, "dclk-gate",		NULL,		0 }, /* DAC */
+	{  6, -1, "refclk-gate",	"clkin",	CLK_IS_CRITICAL },
+	{  7,  3, "usb-port2-gate",	NULL,		0 }, /* USB2.0 Host port 2 */
+	{  8,  5, "lclk-gate",		NULL,		0 }, /* LPC */
+	{  9, 15, "usb-uhci-gate",	NULL,		0 }, /* USB1.1 (requires port 2 enabled) */
+	{ 10, 13, "d1clk-gate",		NULL,		0 }, /* GFX CRT */
+	/* 11: reserved */
+	/* 12: reserved */
+	{ 13, 4,  "yclk-gate",		NULL,		0 }, /* HAC */
+	{ 14, 14, "usb-port1-gate",	NULL,		0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
+	{ 15, -1, "uart1clk-gate",	"uart",		0 }, /* UART1 */
+	{ 16, -1, "uart2clk-gate",	"uart",		0 }, /* UART2 */
+	{ 17, -1, "uart5clk-gate",	"uart",		0 }, /* UART5 */
+	/* 18: reserved */
+	{ 19, -1, "espiclk-gate",	NULL,		0 }, /* eSPI */
+	{ 20, 11, "mac1clk-gate",	"clkin",	0 }, /* MAC1 */
+	{ 21, 12, "mac2clk-gate",	"clkin",	0 }, /* MAC2 */
+	/* 22: reserved */
+	/* 23: reserved */
+	{ 24, -1, "rsaclk-gate",	NULL,		0 }, /* RSA */
+	{ 25, -1, "uart3clk-gate",	"uart",		0 }, /* UART3 */
+	{ 26, -1, "uart4clk-gate",	"uart",		0 }, /* UART4 */
+	{ 27, 16, "sdclk-gate",		NULL,		0 }, /* SDIO/SD */
+	{ 28, -1, "lhclk-gate",		"lhclk",	0 }, /* LPC master/LPC+ */
+	/* 29: reserved */
+	/* 30: reserved */
+	/* 31: reserved */
+};
+
+static void __init aspeed_cc_init(struct device_node *np)
+{
+	struct regmap *map;
+	u32 val;
+	int ret;
+	int i;
+
+	scu_base = of_iomap(np, 0);
+	if (IS_ERR(scu_base))
+		return;
+
+	aspeed_clk_data = kzalloc(sizeof(*aspeed_clk_data) +
+			sizeof(*aspeed_clk_data->hws) * ASPEED_NUM_CLKS,
+			GFP_KERNEL);
+	if (!aspeed_clk_data)
+		return;
+
+	/*
+	 * This way all clock fetched before the platform device probes,
+	 * except those we assign here for early use, will be deferred.
+	 */
+	for (i = 0; i < ASPEED_NUM_CLKS; i++)
+		aspeed_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+	map = syscon_node_to_regmap(np);
+	if (IS_ERR(map)) {
+		pr_err("no syscon regmap\n");
+		return;
+	}
+	/*
+	 * We check that the regmap works on this very first access,
+	 * but as this is an MMIO-backed regmap, subsequent regmap
+	 * access is not going to fail and we skip error checks from
+	 * this point.
+	 */
+	ret = regmap_read(map, ASPEED_STRAP, &val);
+	if (ret) {
+		pr_err("failed to read strapping register\n");
+		return;
+	}
+
+	aspeed_clk_data->num = ASPEED_NUM_CLKS;
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
+	if (ret)
+		pr_err("failed to add DT provider: %d\n", ret);
+};
+CLK_OF_DECLARE_DRIVER(aspeed_cc_g5, "aspeed,ast2500-scu", aspeed_cc_init);
+CLK_OF_DECLARE_DRIVER(aspeed_cc_g4, "aspeed,ast2400-scu", aspeed_cc_init);
diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
new file mode 100644
index 000000000000..afe06b77562d
--- /dev/null
+++ b/include/dt-bindings/clock/aspeed-clock.h
@@ -0,0 +1,43 @@
+#ifndef DT_BINDINGS_ASPEED_CLOCK_H
+#define DT_BINDINGS_ASPEED_CLOCK_H
+
+#define ASPEED_NUM_CLKS	34
+
+#define ASPEED_CLK_HPLL			0
+#define ASPEED_CLK_AHB			1
+#define ASPEED_CLK_APB			2
+#define ASPEED_CLK_UART			3
+#define ASPEED_CLK_SDIO			4
+#define ASPEED_CLK_ECLK			5
+#define ASPEED_CLK_ECLK_MUX		6
+#define ASPEED_CLK_LHCLK		7
+#define ASPEED_CLK_MAC			8
+#define ASPEED_CLK_BCLK			9
+#define ASPEED_CLK_MPLL			10
+#define ASPEED_CLK_GATES		11
+#define ASPEED_CLK_GATE_ECLK		(0 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_GCLK		(1 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_MCLK		(2 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_VCLK		(3 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_BCLK		(4 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_DCLK		(5 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_REFCLK		(6 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_USBPORT2CLK	(7 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_LCLK		(8 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_USBUHCICLK	(9 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_D1CLK		(10 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_YCLK		(11 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_USBPORT1CLK	(12 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_UART1CLK	(13 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_UART2CLK	(14 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_UART5CLK	(15 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_ESPICLK		(16 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_MAC1CLK		(17 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_MAC2CLK		(18 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_RSACLK		(19 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_UART3CLK	(20 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_UART4CLK	(21 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_SDCLKCLK	(22 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_LHCCLK		(23 + ASPEED_CLK_GATES)
+
+#endif
-- 
2.14.1

[PATCH v2 1/5] clk: Add clock driver for ASPEED BMC SoCs

[Joel Stanley]

This adds the stub of a driver for the ASPEED SoCs. The clocks are
defined and the static registration is set up.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/Kconfig                      |  12 +++
 drivers/clk/Makefile                     |   1 +
 drivers/clk/clk-aspeed.c                 | 162 +++++++++++++++++++++++++++++++
 include/dt-bindings/clock/aspeed-clock.h |  43 ++++++++
 4 files changed, 218 insertions(+)
 create mode 100644 drivers/clk/clk-aspeed.c
 create mode 100644 include/dt-bindings/clock/aspeed-clock.h

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 1c4e1aa6767e..9abe063ef8d2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -142,6 +142,18 @@ config COMMON_CLK_GEMINI
 	  This driver supports the SoC clocks on the Cortina Systems Gemini
 	  platform, also known as SL3516 or CS3516.
 
+config COMMON_CLK_ASPEED
+	bool "Clock driver for Aspeed BMC SoCs"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	default ARCH_ASPEED
+	select MFD_SYSCON
+	select RESET_CONTROLLER
+	---help---
+	  This driver supports the SoC clocks on the Aspeed BMC platforms.
+
+	  The G4 and G5 series, including the ast2400 and ast2500, are supported
+	  by this driver.
+
 config COMMON_CLK_S2MPS11
 	tristate "Clock driver for S2MPS1X/S5M8767 MFD"
 	depends on MFD_SEC_CORE || COMPILE_TEST
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index c99f363826f0..575c68919d9b 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
 obj-$(CONFIG_COMMON_CLK_CS2000_CP)	+= clk-cs2000-cp.o
 obj-$(CONFIG_ARCH_EFM32)		+= clk-efm32gg.o
 obj-$(CONFIG_COMMON_CLK_GEMINI)		+= clk-gemini.o
+obj-$(CONFIG_COMMON_CLK_ASPEED)		+= clk-aspeed.o
 obj-$(CONFIG_ARCH_HIGHBANK)		+= clk-highbank.o
 obj-$(CONFIG_CLK_HSDK)			+= clk-hsdk-pll.o
 obj-$(CONFIG_COMMON_CLK_MAX77686)	+= clk-max77686.o
diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
new file mode 100644
index 000000000000..824c54767009
--- /dev/null
+++ b/drivers/clk/clk-aspeed.c
@@ -0,0 +1,162 @@
+/*
+ * Copyright 2017 IBM Corporation
+ *
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#define pr_fmt(fmt) "clk-aspeed: " fmt
+
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/clk-provider.h>
+#include <linux/regmap.h>
+#include <linux/spinlock.h>
+#include <linux/mfd/syscon.h>
+
+#include <dt-bindings/clock/aspeed-clock.h>
+
+#define ASPEED_RESET_CTRL	0x04
+#define ASPEED_CLK_SELECTION	0x08
+#define ASPEED_CLK_STOP_CTRL	0x0c
+#define ASPEED_MPLL_PARAM	0x20
+#define ASPEED_HPLL_PARAM	0x24
+#define ASPEED_MISC_CTRL	0x2c
+#define ASPEED_STRAP		0x70
+
+/* Keeps track of all clocks */
+static struct clk_hw_onecell_data *aspeed_clk_data;
+
+static void __iomem *scu_base;
+
+/**
+ * struct aspeed_gate_data - Aspeed gated clocks
+ * @clock_idx: bit used to gate this clock in the clock register
+ * @reset_idx: bit used to reset this IP in the reset register. -1 if no
+ *             reset is required when enabling the clock
+ * @name: the clock name
+ * @parent_name: the name of the parent clock
+ * @flags: standard clock framework flags
+ */
+struct aspeed_gate_data {
+	u8		clock_idx;
+	s8		reset_idx;
+	const char	*name;
+	const char	*parent_name;
+	unsigned long	flags;
+};
+
+/**
+ * struct aspeed_clk_gate - Aspeed specific clk_gate structure
+ * @hw:		handle between common and hardware-specific interfaces
+ * @reg:	register controlling gate
+ * @clock_idx:	bit used to gate this clock in the clock register
+ * @reset_idx:	bit used to reset this IP in the reset register. -1 if no
+ *		reset is required when enabling the clock
+ * @flags:	hardware-specific flags
+ * @lock:	register lock
+ *
+ * Some of the clocks in the Aspeed SoC must be put in reset before enabling.
+ * This modified version of clk_gate allows an optional reset bit to be
+ * specified.
+ */
+struct aspeed_clk_gate {
+	struct clk_hw	hw;
+	struct regmap	*map;
+	u8		clock_idx;
+	s8		reset_idx;
+	u8		flags;
+	spinlock_t	*lock;
+};
+
+#define to_aspeed_clk_gate(_hw) container_of(_hw, struct aspeed_clk_gate, hw)
+
+/* TODO: ask Aspeed about the actual parent data */
+static const struct aspeed_gate_data aspeed_gates[] __initconst = {
+/*	 clk rst   name			parent		flags	*/
+	{  0, -1, "eclk-gate",		"eclk",		0 }, /* Video Engine */
+	{  1,  7, "gclk-gate",		NULL,		0 }, /* 2D engine */
+	{  2, -1, "mclk-gate",		"mpll",		CLK_IS_CRITICAL }, /* SDRAM */
+	{  3,  6, "vclk-gate",		NULL,		0 }, /* Video Capture */
+	{  4, 10, "bclk-gate",		"bclk",		0 }, /* PCIe/PCI */
+	{  5, -1, "dclk-gate",		NULL,		0 }, /* DAC */
+	{  6, -1, "refclk-gate",	"clkin",	CLK_IS_CRITICAL },
+	{  7,  3, "usb-port2-gate",	NULL,		0 }, /* USB2.0 Host port 2 */
+	{  8,  5, "lclk-gate",		NULL,		0 }, /* LPC */
+	{  9, 15, "usb-uhci-gate",	NULL,		0 }, /* USB1.1 (requires port 2 enabled) */
+	{ 10, 13, "d1clk-gate",		NULL,		0 }, /* GFX CRT */
+	/* 11: reserved */
+	/* 12: reserved */
+	{ 13, 4,  "yclk-gate",		NULL,		0 }, /* HAC */
+	{ 14, 14, "usb-port1-gate",	NULL,		0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
+	{ 15, -1, "uart1clk-gate",	"uart",		0 }, /* UART1 */
+	{ 16, -1, "uart2clk-gate",	"uart",		0 }, /* UART2 */
+	{ 17, -1, "uart5clk-gate",	"uart",		0 }, /* UART5 */
+	/* 18: reserved */
+	{ 19, -1, "espiclk-gate",	NULL,		0 }, /* eSPI */
+	{ 20, 11, "mac1clk-gate",	"clkin",	0 }, /* MAC1 */
+	{ 21, 12, "mac2clk-gate",	"clkin",	0 }, /* MAC2 */
+	/* 22: reserved */
+	/* 23: reserved */
+	{ 24, -1, "rsaclk-gate",	NULL,		0 }, /* RSA */
+	{ 25, -1, "uart3clk-gate",	"uart",		0 }, /* UART3 */
+	{ 26, -1, "uart4clk-gate",	"uart",		0 }, /* UART4 */
+	{ 27, 16, "sdclk-gate",		NULL,		0 }, /* SDIO/SD */
+	{ 28, -1, "lhclk-gate",		"lhclk",	0 }, /* LPC master/LPC+ */
+	/* 29: reserved */
+	/* 30: reserved */
+	/* 31: reserved */
+};
+
+static void __init aspeed_cc_init(struct device_node *np)
+{
+	struct regmap *map;
+	u32 val;
+	int ret;
+	int i;
+
+	scu_base = of_iomap(np, 0);
+	if (IS_ERR(scu_base))
+		return;
+
+	aspeed_clk_data = kzalloc(sizeof(*aspeed_clk_data) +
+			sizeof(*aspeed_clk_data->hws) * ASPEED_NUM_CLKS,
+			GFP_KERNEL);
+	if (!aspeed_clk_data)
+		return;
+
+	/*
+	 * This way all clock fetched before the platform device probes,
+	 * except those we assign here for early use, will be deferred.
+	 */
+	for (i = 0; i < ASPEED_NUM_CLKS; i++)
+		aspeed_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+	map = syscon_node_to_regmap(np);
+	if (IS_ERR(map)) {
+		pr_err("no syscon regmap\n");
+		return;
+	}
+	/*
+	 * We check that the regmap works on this very first access,
+	 * but as this is an MMIO-backed regmap, subsequent regmap
+	 * access is not going to fail and we skip error checks from
+	 * this point.
+	 */
+	ret = regmap_read(map, ASPEED_STRAP, &val);
+	if (ret) {
+		pr_err("failed to read strapping register\n");
+		return;
+	}
+
+	aspeed_clk_data->num = ASPEED_NUM_CLKS;
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
+	if (ret)
+		pr_err("failed to add DT provider: %d\n", ret);
+};
+CLK_OF_DECLARE_DRIVER(aspeed_cc_g5, "aspeed,ast2500-scu", aspeed_cc_init);
+CLK_OF_DECLARE_DRIVER(aspeed_cc_g4, "aspeed,ast2400-scu", aspeed_cc_init);
diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
new file mode 100644
index 000000000000..afe06b77562d
--- /dev/null
+++ b/include/dt-bindings/clock/aspeed-clock.h
@@ -0,0 +1,43 @@
+#ifndef DT_BINDINGS_ASPEED_CLOCK_H
+#define DT_BINDINGS_ASPEED_CLOCK_H
+
+#define ASPEED_NUM_CLKS	34
+
+#define ASPEED_CLK_HPLL			0
+#define ASPEED_CLK_AHB			1
+#define ASPEED_CLK_APB			2
+#define ASPEED_CLK_UART			3
+#define ASPEED_CLK_SDIO			4
+#define ASPEED_CLK_ECLK			5
+#define ASPEED_CLK_ECLK_MUX		6
+#define ASPEED_CLK_LHCLK		7
+#define ASPEED_CLK_MAC			8
+#define ASPEED_CLK_BCLK			9
+#define ASPEED_CLK_MPLL			10
+#define ASPEED_CLK_GATES		11
+#define ASPEED_CLK_GATE_ECLK		(0 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_GCLK		(1 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_MCLK		(2 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_VCLK		(3 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_BCLK		(4 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_DCLK		(5 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_REFCLK		(6 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_USBPORT2CLK	(7 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_LCLK		(8 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_USBUHCICLK	(9 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_D1CLK		(10 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_YCLK		(11 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_USBPORT1CLK	(12 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_UART1CLK	(13 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_UART2CLK	(14 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_UART5CLK	(15 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_ESPICLK		(16 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_MAC1CLK		(17 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_MAC2CLK		(18 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_RSACLK		(19 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_UART3CLK	(20 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_UART4CLK	(21 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_SDCLKCLK	(22 + ASPEED_CLK_GATES)
+#define ASPEED_CLK_GATE_LHCCLK		(23 + ASPEED_CLK_GATES)
+
+#endif
-- 
2.14.1

[PATCH v2 2/5] clk: aspeed: Register core clocks

[Joel Stanley]

This registers the core clocks; those which are required to calculate
the rate of the timer periperhal so the system can load a clocksource
driver.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/clk-aspeed.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 149 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 824c54767009..e614c61b82d2 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -11,12 +11,12 @@
 
 #define pr_fmt(fmt) "clk-aspeed: " fmt
 
-#include <linux/slab.h>
-#include <linux/of_address.h>
 #include <linux/clk-provider.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
 #include <linux/regmap.h>
+#include <linux/slab.h>
 #include <linux/spinlock.h>
-#include <linux/mfd/syscon.h>
 
 #include <dt-bindings/clock/aspeed-clock.h>
 
@@ -28,6 +28,9 @@
 #define ASPEED_MISC_CTRL	0x2c
 #define ASPEED_STRAP		0x70
 
+/* Globally visible clocks */
+static DEFINE_SPINLOCK(aspeed_clk_lock);
+
 /* Keeps track of all clocks */
 static struct clk_hw_onecell_data *aspeed_clk_data;
 
@@ -112,9 +115,137 @@ static const struct aspeed_gate_data aspeed_gates[] __initconst = {
 	/* 31: reserved */
 };
 
+static const struct clk_div_table ast2400_div_table[] = {
+	{ 0x0, 2 },
+	{ 0x1, 4 },
+	{ 0x2, 6 },
+	{ 0x3, 8 },
+	{ 0x4, 10 },
+	{ 0x5, 12 },
+	{ 0x6, 14 },
+	{ 0x7, 16 },
+	{ 0 }
+};
+
+static const struct clk_div_table ast2500_div_table[] = {
+	{ 0x0, 4 },
+	{ 0x1, 8 },
+	{ 0x2, 12 },
+	{ 0x3, 16 },
+	{ 0x4, 20 },
+	{ 0x5, 24 },
+	{ 0x6, 28 },
+	{ 0x7, 32 },
+	{ 0 }
+};
+
+static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
+{
+	unsigned int mult, div;
+
+	if (val & BIT(20)) {
+		/* Pass through mode */
+		mult = div = 1;
+	} else {
+		/* F = clkin * [(M+1) / (N+1)] / (P + 1) */
+		u32 p = (val >> 13) & 0x3f;
+		u32 m = (val >> 5) & 0xff;
+		u32 n = val & 0x1f;
+
+		mult = (m + 1) / (n + 1);
+		div = p + 1;
+	}
+
+	return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
+			mult, div);
+}
+
+static void __init aspeed_ast2400_cc(struct regmap *map)
+{
+	struct clk_hw *hw;
+	u32 val, div, mult;
+
+	/*
+	 * High-speed PLL clock derived from the crystal. This the CPU clock,
+	 * and we assume that it is enabled
+	 */
+	regmap_read(map, ASPEED_HPLL_PARAM, &val);
+	WARN(val & BIT(18), "clock is strapped not configured");
+	if (val & BIT(17)) {
+		/* Pass through mode */
+		mult = div = 1;
+	} else {
+		/* F = 24Mhz * (2-OD) * [(N + 2) / (D + 1)] */
+		u32 n = (val >> 5) & 0x3f;
+		u32 od = (val >> 4) & 0x1;
+		u32 d = val & 0xf;
+
+		mult = (2 - od) * (n + 2);
+		div = d + 1;
+	}
+	hw = clk_hw_register_fixed_factor(NULL, "hpll", "clkin", 0, mult, div);
+	aspeed_clk_data->hws[ASPEED_CLK_HPLL] = hw;
+
+	/*
+	 * Strap bits 11:10 define the CPU/AHB clock frequency ratio (aka HCLK)
+	 *   00: Select CPU:AHB = 1:1
+	 *   01: Select CPU:AHB = 2:1
+	 *   10: Select CPU:AHB = 4:1
+	 *   11: Select CPU:AHB = 3:1
+	 */
+	regmap_read(map, ASPEED_STRAP, &val);
+	val = (val >> 10) & 0x3;
+	div = val + 1;
+	if (div == 2)
+		div = 3;
+	else if (div == 3)
+		div = 2;
+	hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
+	aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
+
+	/* APB clock clock selection register SCU08 (aka PCLK) */
+	hw = clk_hw_register_divider_table(NULL, "apb", "hpll", 0,
+			scu_base + ASPEED_CLK_SELECTION, 23, 3, 0,
+			ast2400_div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
+}
+
+static void __init aspeed_ast2500_cc(struct regmap *map)
+{
+	struct clk_hw *hw;
+	u32 val, div;
+
+	/*
+	 * High-speed PLL clock derived from the crystal. This the CPU clock,
+	 * and we assume that it is enabled
+	 */
+	regmap_read(map, ASPEED_HPLL_PARAM, &val);
+	aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_calc_pll("hpll", val);
+
+	/* Strap bits 11:9 define the AXI/AHB clock frequency ratio (aka HCLK)*/
+	regmap_read(map, ASPEED_STRAP, &val);
+	val = (val >> 9) & 0x7;
+	WARN_ON(val == 0);
+	div = 2 * (val + 1);
+	hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
+	aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
+
+	/* APB clock clock selection register SCU08 (aka PCLK) */
+	/* TODO: this is wrong! */
+	regmap_read(map, ASPEED_CLK_SELECTION, &val);
+	val = (val >> 23) & 0x7;
+	div = 4 * (val + 1);
+	hw = clk_hw_register_fixed_factor(NULL, "apb", "hpll", 0, 1, div);
+	aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
+};
+
+
 static void __init aspeed_cc_init(struct device_node *np)
 {
 	struct regmap *map;
+	unsigned long freq;
+	struct clk_hw *hw;
 	u32 val;
 	int ret;
 	int i;
@@ -153,6 +284,21 @@ static void __init aspeed_cc_init(struct device_node *np)
 		return;
 	}
 
+	/* CLKIN is the crystal oscillator, 24 or 25MHz selected by strapping */
+	if (val & BIT(23))
+		freq = 25000000;
+	else
+		freq = 24000000;
+	hw = clk_hw_register_fixed_rate(NULL, "clkin", NULL, 0, freq);
+	pr_debug("clkin @%lu MHz\n", freq / 1000000);
+
+	if (of_device_is_compatible(np, "aspeed,ast2400-scu"))
+		aspeed_ast2400_cc(map);
+	else if (of_device_is_compatible(np, "aspeed,ast2500-scu"))
+		aspeed_ast2500_cc(map);
+	else
+		pr_err("unknown platform, failed to add clocks\n");
+
 	aspeed_clk_data->num = ASPEED_NUM_CLKS;
 	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
 	if (ret)
-- 
2.14.1

[PATCH v2 2/5] clk: aspeed: Register core clocks

[Joel Stanley]

This registers the core clocks; those which are required to calculate
the rate of the timer periperhal so the system can load a clocksource
driver.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/clk-aspeed.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 149 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 824c54767009..e614c61b82d2 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -11,12 +11,12 @@
 
 #define pr_fmt(fmt) "clk-aspeed: " fmt
 
-#include <linux/slab.h>
-#include <linux/of_address.h>
 #include <linux/clk-provider.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
 #include <linux/regmap.h>
+#include <linux/slab.h>
 #include <linux/spinlock.h>
-#include <linux/mfd/syscon.h>
 
 #include <dt-bindings/clock/aspeed-clock.h>
 
@@ -28,6 +28,9 @@
 #define ASPEED_MISC_CTRL	0x2c
 #define ASPEED_STRAP		0x70
 
+/* Globally visible clocks */
+static DEFINE_SPINLOCK(aspeed_clk_lock);
+
 /* Keeps track of all clocks */
 static struct clk_hw_onecell_data *aspeed_clk_data;
 
@@ -112,9 +115,137 @@ static const struct aspeed_gate_data aspeed_gates[] __initconst = {
 	/* 31: reserved */
 };
 
+static const struct clk_div_table ast2400_div_table[] = {
+	{ 0x0, 2 },
+	{ 0x1, 4 },
+	{ 0x2, 6 },
+	{ 0x3, 8 },
+	{ 0x4, 10 },
+	{ 0x5, 12 },
+	{ 0x6, 14 },
+	{ 0x7, 16 },
+	{ 0 }
+};
+
+static const struct clk_div_table ast2500_div_table[] = {
+	{ 0x0, 4 },
+	{ 0x1, 8 },
+	{ 0x2, 12 },
+	{ 0x3, 16 },
+	{ 0x4, 20 },
+	{ 0x5, 24 },
+	{ 0x6, 28 },
+	{ 0x7, 32 },
+	{ 0 }
+};
+
+static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
+{
+	unsigned int mult, div;
+
+	if (val & BIT(20)) {
+		/* Pass through mode */
+		mult = div = 1;
+	} else {
+		/* F = clkin * [(M+1) / (N+1)] / (P + 1) */
+		u32 p = (val >> 13) & 0x3f;
+		u32 m = (val >> 5) & 0xff;
+		u32 n = val & 0x1f;
+
+		mult = (m + 1) / (n + 1);
+		div = p + 1;
+	}
+
+	return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
+			mult, div);
+}
+
+static void __init aspeed_ast2400_cc(struct regmap *map)
+{
+	struct clk_hw *hw;
+	u32 val, div, mult;
+
+	/*
+	 * High-speed PLL clock derived from the crystal. This the CPU clock,
+	 * and we assume that it is enabled
+	 */
+	regmap_read(map, ASPEED_HPLL_PARAM, &val);
+	WARN(val & BIT(18), "clock is strapped not configured");
+	if (val & BIT(17)) {
+		/* Pass through mode */
+		mult = div = 1;
+	} else {
+		/* F = 24Mhz * (2-OD) * [(N + 2) / (D + 1)] */
+		u32 n = (val >> 5) & 0x3f;
+		u32 od = (val >> 4) & 0x1;
+		u32 d = val & 0xf;
+
+		mult = (2 - od) * (n + 2);
+		div = d + 1;
+	}
+	hw = clk_hw_register_fixed_factor(NULL, "hpll", "clkin", 0, mult, div);
+	aspeed_clk_data->hws[ASPEED_CLK_HPLL] = hw;
+
+	/*
+	 * Strap bits 11:10 define the CPU/AHB clock frequency ratio (aka HCLK)
+	 *   00: Select CPU:AHB = 1:1
+	 *   01: Select CPU:AHB = 2:1
+	 *   10: Select CPU:AHB = 4:1
+	 *   11: Select CPU:AHB = 3:1
+	 */
+	regmap_read(map, ASPEED_STRAP, &val);
+	val = (val >> 10) & 0x3;
+	div = val + 1;
+	if (div == 2)
+		div = 3;
+	else if (div == 3)
+		div = 2;
+	hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
+	aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
+
+	/* APB clock clock selection register SCU08 (aka PCLK) */
+	hw = clk_hw_register_divider_table(NULL, "apb", "hpll", 0,
+			scu_base + ASPEED_CLK_SELECTION, 23, 3, 0,
+			ast2400_div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
+}
+
+static void __init aspeed_ast2500_cc(struct regmap *map)
+{
+	struct clk_hw *hw;
+	u32 val, div;
+
+	/*
+	 * High-speed PLL clock derived from the crystal. This the CPU clock,
+	 * and we assume that it is enabled
+	 */
+	regmap_read(map, ASPEED_HPLL_PARAM, &val);
+	aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_calc_pll("hpll", val);
+
+	/* Strap bits 11:9 define the AXI/AHB clock frequency ratio (aka HCLK)*/
+	regmap_read(map, ASPEED_STRAP, &val);
+	val = (val >> 9) & 0x7;
+	WARN_ON(val == 0);
+	div = 2 * (val + 1);
+	hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
+	aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
+
+	/* APB clock clock selection register SCU08 (aka PCLK) */
+	/* TODO: this is wrong! */
+	regmap_read(map, ASPEED_CLK_SELECTION, &val);
+	val = (val >> 23) & 0x7;
+	div = 4 * (val + 1);
+	hw = clk_hw_register_fixed_factor(NULL, "apb", "hpll", 0, 1, div);
+	aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
+};
+
+
 static void __init aspeed_cc_init(struct device_node *np)
 {
 	struct regmap *map;
+	unsigned long freq;
+	struct clk_hw *hw;
 	u32 val;
 	int ret;
 	int i;
@@ -153,6 +284,21 @@ static void __init aspeed_cc_init(struct device_node *np)
 		return;
 	}
 
+	/* CLKIN is the crystal oscillator, 24 or 25MHz selected by strapping */
+	if (val & BIT(23))
+		freq = 25000000;
+	else
+		freq = 24000000;
+	hw = clk_hw_register_fixed_rate(NULL, "clkin", NULL, 0, freq);
+	pr_debug("clkin @%lu MHz\n", freq / 1000000);
+
+	if (of_device_is_compatible(np, "aspeed,ast2400-scu"))
+		aspeed_ast2400_cc(map);
+	else if (of_device_is_compatible(np, "aspeed,ast2500-scu"))
+		aspeed_ast2500_cc(map);
+	else
+		pr_err("unknown platform, failed to add clocks\n");
+
 	aspeed_clk_data->num = ASPEED_NUM_CLKS;
 	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
 	if (ret)
-- 
2.14.1

[PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Joel Stanley]

This registers a platform driver to set up all of the non-core clocks.

The clocks that have configurable rates are now registered.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/clk-aspeed.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index e614c61b82d2..19531798e040 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -14,6 +14,8 @@
 #include <linux/clk-provider.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -115,6 +117,20 @@ static const struct aspeed_gate_data aspeed_gates[] __initconst = {
 	/* 31: reserved */
 };
 
+static const char * const eclk_parents[] = {"d1pll", "hpll", "mpll"};
+
+static const struct clk_div_table ast2500_mac_div_table[] = {
+	{ 0x0, 4 }, /* Yep, really. Aspeed confirmed this is correct */
+	{ 0x1, 4 },
+	{ 0x2, 6 },
+	{ 0x3, 8 },
+	{ 0x4, 10 },
+	{ 0x5, 12 },
+	{ 0x6, 14 },
+	{ 0x7, 16 },
+	{ 0 }
+};
+
 static const struct clk_div_table ast2400_div_table[] = {
 	{ 0x0, 2 },
 	{ 0x1, 4 },
@@ -139,6 +155,21 @@ static const struct clk_div_table ast2500_div_table[] = {
 	{ 0 }
 };
 
+struct aspeed_clk_soc_data {
+	const struct clk_div_table *div_table;
+	const struct clk_div_table *mac_div_table;
+};
+
+static const struct aspeed_clk_soc_data ast2500_data = {
+	.div_table = ast2500_div_table,
+	.mac_div_table = ast2500_mac_div_table,
+};
+
+static const struct aspeed_clk_soc_data ast2400_data = {
+	.div_table = ast2400_div_table,
+	.mac_div_table = ast2400_div_table,
+};
+
 static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
 {
 	unsigned int mult, div;
@@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
 			mult, div);
 }
 
+static int __init aspeed_clk_probe(struct platform_device *pdev)
+{
+	const struct aspeed_clk_soc_data *soc_data;
+	const struct clk_div_table *mac_div_table;
+	const struct clk_div_table *div_table;
+	struct device *dev = &pdev->dev;
+	struct regmap *map;
+	struct clk_hw *hw;
+	u32 val, rate;
+
+	map = syscon_node_to_regmap(dev->of_node);
+	if (IS_ERR(map)) {
+		dev_err(dev, "no syscon regmap\n");
+		return PTR_ERR(map);
+	}
+
+	/* SoC generations share common layouts but have different divisors */
+	soc_data = of_device_get_match_data(&pdev->dev);
+	div_table = soc_data->div_table;
+	mac_div_table = soc_data->mac_div_table;
+
+	/* UART clock div13 setting */
+	regmap_read(map, ASPEED_MISC_CTRL, &val);
+	if (val & BIT(12))
+		rate = 24000000 / 13;
+	else
+		rate = 24000000;
+	/* TODO: Find the parent data for the uart clock */
+	hw = clk_hw_register_fixed_rate(NULL, "uart", NULL, 0, rate);
+	aspeed_clk_data->hws[ASPEED_CLK_UART] = hw;
+
+	/*
+	 * Memory controller (M-PLL) PLL. This clock is configured by the
+	 * bootloader, and is exposed to Linux as a read-only clock rate.
+	 */
+	regmap_read(map, ASPEED_MPLL_PARAM, &val);
+	aspeed_clk_data->hws[ASPEED_CLK_MPLL] =	aspeed_calc_pll("mpll", val);
+
+	/* SD/SDIO clock divider (TODO: There's a gate too) */
+	hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
+			scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
+			div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_SDIO] = hw;
+
+	/* MAC AHB bus clock divider */
+	hw = clk_hw_register_divider_table(NULL, "mac", "hpll", 0,
+			scu_base + ASPEED_CLK_SELECTION, 16, 3, 0,
+			mac_div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_MAC] = hw;
+
+	/* LPC Host (LHCLK) clock divider */
+	hw = clk_hw_register_divider_table(NULL, "lhclk", "hpll", 0,
+			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
+			div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_LHCLK] = hw;
+
+	/* Video Engine (ECLK) mux and clock divider */
+	hw = clk_hw_register_mux(NULL, "eclk_mux",
+			eclk_parents, ARRAY_SIZE(eclk_parents), 0,
+			scu_base + ASPEED_CLK_SELECTION, 2, 2,
+			0, &aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
+	hw = clk_hw_register_divider_table(NULL, "eclk", "eclk_mux", 0,
+			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
+			div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
+
+	/* P-Bus (BCLK) clock divider */
+	hw = clk_hw_register_divider_table(NULL, "bclk", "hpll", 0,
+			scu_base + ASPEED_CLK_SELECTION, 0, 2, 0,
+			div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
+
+	return 0;
+};
+
+static const struct of_device_id aspeed_clk_dt_ids[] = {
+	{ .compatible = "aspeed,ast2400-scu", .data = &ast2400_data },
+	{ .compatible = "aspeed,ast2500-scu", .data = &ast2500_data },
+	{ },
+};
+
+static struct platform_driver aspeed_clk_driver = {
+	.probe  = aspeed_clk_probe,
+	.driver = {
+		.name = "aspeed-clk",
+		.of_match_table = aspeed_clk_dt_ids,
+		.suppress_bind_attrs = true,
+	},
+};
+builtin_platform_driver(aspeed_clk_driver);
+
+
 static void __init aspeed_ast2400_cc(struct regmap *map)
 {
 	struct clk_hw *hw;
-- 
2.14.1

[PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Joel Stanley]

This registers a platform driver to set up all of the non-core clocks.

The clocks that have configurable rates are now registered.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/clk-aspeed.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index e614c61b82d2..19531798e040 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -14,6 +14,8 @@
 #include <linux/clk-provider.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -115,6 +117,20 @@ static const struct aspeed_gate_data aspeed_gates[] __initconst = {
 	/* 31: reserved */
 };
 
+static const char * const eclk_parents[] = {"d1pll", "hpll", "mpll"};
+
+static const struct clk_div_table ast2500_mac_div_table[] = {
+	{ 0x0, 4 }, /* Yep, really. Aspeed confirmed this is correct */
+	{ 0x1, 4 },
+	{ 0x2, 6 },
+	{ 0x3, 8 },
+	{ 0x4, 10 },
+	{ 0x5, 12 },
+	{ 0x6, 14 },
+	{ 0x7, 16 },
+	{ 0 }
+};
+
 static const struct clk_div_table ast2400_div_table[] = {
 	{ 0x0, 2 },
 	{ 0x1, 4 },
@@ -139,6 +155,21 @@ static const struct clk_div_table ast2500_div_table[] = {
 	{ 0 }
 };
 
+struct aspeed_clk_soc_data {
+	const struct clk_div_table *div_table;
+	const struct clk_div_table *mac_div_table;
+};
+
+static const struct aspeed_clk_soc_data ast2500_data = {
+	.div_table = ast2500_div_table,
+	.mac_div_table = ast2500_mac_div_table,
+};
+
+static const struct aspeed_clk_soc_data ast2400_data = {
+	.div_table = ast2400_div_table,
+	.mac_div_table = ast2400_div_table,
+};
+
 static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
 {
 	unsigned int mult, div;
@@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
 			mult, div);
 }
 
+static int __init aspeed_clk_probe(struct platform_device *pdev)
+{
+	const struct aspeed_clk_soc_data *soc_data;
+	const struct clk_div_table *mac_div_table;
+	const struct clk_div_table *div_table;
+	struct device *dev = &pdev->dev;
+	struct regmap *map;
+	struct clk_hw *hw;
+	u32 val, rate;
+
+	map = syscon_node_to_regmap(dev->of_node);
+	if (IS_ERR(map)) {
+		dev_err(dev, "no syscon regmap\n");
+		return PTR_ERR(map);
+	}
+
+	/* SoC generations share common layouts but have different divisors */
+	soc_data = of_device_get_match_data(&pdev->dev);
+	div_table = soc_data->div_table;
+	mac_div_table = soc_data->mac_div_table;
+
+	/* UART clock div13 setting */
+	regmap_read(map, ASPEED_MISC_CTRL, &val);
+	if (val & BIT(12))
+		rate = 24000000 / 13;
+	else
+		rate = 24000000;
+	/* TODO: Find the parent data for the uart clock */
+	hw = clk_hw_register_fixed_rate(NULL, "uart", NULL, 0, rate);
+	aspeed_clk_data->hws[ASPEED_CLK_UART] = hw;
+
+	/*
+	 * Memory controller (M-PLL) PLL. This clock is configured by the
+	 * bootloader, and is exposed to Linux as a read-only clock rate.
+	 */
+	regmap_read(map, ASPEED_MPLL_PARAM, &val);
+	aspeed_clk_data->hws[ASPEED_CLK_MPLL] =	aspeed_calc_pll("mpll", val);
+
+	/* SD/SDIO clock divider (TODO: There's a gate too) */
+	hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
+			scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
+			div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_SDIO] = hw;
+
+	/* MAC AHB bus clock divider */
+	hw = clk_hw_register_divider_table(NULL, "mac", "hpll", 0,
+			scu_base + ASPEED_CLK_SELECTION, 16, 3, 0,
+			mac_div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_MAC] = hw;
+
+	/* LPC Host (LHCLK) clock divider */
+	hw = clk_hw_register_divider_table(NULL, "lhclk", "hpll", 0,
+			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
+			div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_LHCLK] = hw;
+
+	/* Video Engine (ECLK) mux and clock divider */
+	hw = clk_hw_register_mux(NULL, "eclk_mux",
+			eclk_parents, ARRAY_SIZE(eclk_parents), 0,
+			scu_base + ASPEED_CLK_SELECTION, 2, 2,
+			0, &aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
+	hw = clk_hw_register_divider_table(NULL, "eclk", "eclk_mux", 0,
+			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
+			div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
+
+	/* P-Bus (BCLK) clock divider */
+	hw = clk_hw_register_divider_table(NULL, "bclk", "hpll", 0,
+			scu_base + ASPEED_CLK_SELECTION, 0, 2, 0,
+			div_table,
+			&aspeed_clk_lock);
+	aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
+
+	return 0;
+};
+
+static const struct of_device_id aspeed_clk_dt_ids[] = {
+	{ .compatible = "aspeed,ast2400-scu", .data = &ast2400_data },
+	{ .compatible = "aspeed,ast2500-scu", .data = &ast2500_data },
+	{ },
+};
+
+static struct platform_driver aspeed_clk_driver = {
+	.probe  = aspeed_clk_probe,
+	.driver = {
+		.name = "aspeed-clk",
+		.of_match_table = aspeed_clk_dt_ids,
+		.suppress_bind_attrs = true,
+	},
+};
+builtin_platform_driver(aspeed_clk_driver);
+
+
 static void __init aspeed_ast2400_cc(struct regmap *map)
 {
 	struct clk_hw *hw;
-- 
2.14.1

[PATCH v2 4/5] clk: aspeed: Register gated clocks

[Joel Stanley]

The majority of the clocks in the system are gates paired with a reset
controller that holds the IP in reset.

This borrows from clk_hw_register_gate, but registers two 'gates', one
to control the clock enable register and the other to control the reset
IP. This allows us to enforce the ordering:

 1. Place IP in reset
 2. Enable clock
 3. Delay
 4. Release reset

There are some gates that do not have an associated reset; these are
handled by using -1 as the index for the reset.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/clk-aspeed.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 19531798e040..dec9db4ec47b 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -191,6 +191,108 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
 			mult, div);
 }
 
+static int aspeed_clk_enable(struct clk_hw *hw)
+{
+	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
+	unsigned long flags;
+	u32 clk = BIT(gate->clock_idx);
+	u32 rst = BIT(gate->reset_idx);
+
+	spin_lock_irqsave(gate->lock, flags);
+
+	if (gate->reset_idx >= 0) {
+		/* Put IP in reset */
+		regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, rst);
+
+		/* Delay 100us */
+		udelay(100);
+	}
+
+	/* Enable clock */
+	regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, 0);
+
+	if (gate->reset_idx >= 0) {
+		/* Delay 10ms */
+		/* TODO: can we sleep here? */
+		msleep(10);
+
+		/* Take IP out of reset */
+		regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, 0);
+	}
+
+	spin_unlock_irqrestore(gate->lock, flags);
+
+	return 0;
+}
+
+static void aspeed_clk_disable(struct clk_hw *hw)
+{
+	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
+	unsigned long flags;
+	u32 clk = BIT(gate->clock_idx);
+
+	spin_lock_irqsave(gate->lock, flags);
+
+	/* Disable clock */
+	regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, clk);
+
+	spin_unlock_irqrestore(gate->lock, flags);
+}
+
+static int aspeed_clk_is_enabled(struct clk_hw *hw)
+{
+	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
+	u32 clk = BIT(gate->clock_idx);
+	u32 reg;
+
+	regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
+
+	return (reg & clk) ? 0 : 1;
+}
+
+static const struct clk_ops aspeed_clk_gate_ops = {
+	.enable = aspeed_clk_enable,
+	.disable = aspeed_clk_disable,
+	.is_enabled = aspeed_clk_is_enabled,
+};
+
+static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		struct regmap *map, u8 clock_idx, u8 reset_idx,
+		u8 clk_gate_flags, spinlock_t *lock)
+{
+	struct aspeed_clk_gate *gate;
+	struct clk_init_data init;
+	struct clk_hw *hw;
+	int ret;
+
+	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
+	if (!gate)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &aspeed_clk_gate_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = parent_name ? &parent_name : NULL;
+	init.num_parents = parent_name ? 1 : 0;
+
+	gate->map = map;
+	gate->clock_idx = clock_idx;
+	gate->reset_idx = reset_idx;
+	gate->flags = clk_gate_flags;
+	gate->lock = lock;
+	gate->hw.init = &init;
+
+	hw = &gate->hw;
+	ret = clk_hw_register(dev, hw);
+	if (ret) {
+		kfree(gate);
+		hw = ERR_PTR(ret);
+	}
+
+	return hw;
+}
+
 static int __init aspeed_clk_probe(struct platform_device *pdev)
 {
 	const struct aspeed_clk_soc_data *soc_data;
@@ -200,6 +302,7 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
 	struct regmap *map;
 	struct clk_hw *hw;
 	u32 val, rate;
+	int i;
 
 	map = syscon_node_to_regmap(dev->of_node);
 	if (IS_ERR(map)) {
@@ -269,6 +372,31 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
 			&aspeed_clk_lock);
 	aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
 
+	/* There are a number of clocks that not included in this driver as
+	 * more information is required:
+	 *   D2-PLL
+	 *   D-PLL
+	 *   YCLK
+	 *   RGMII
+	 *   RMII
+	 *   UART[1..5] clock source mux
+	 */
+
+	for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) {
+		const struct aspeed_gate_data *gd;
+
+		gd = &aspeed_gates[i];
+		aspeed_clk_data->hws[ASPEED_CLK_GATES + i] =
+			aspeed_clk_hw_register_gate(NULL, gd->name,
+					     gd->parent_name,
+					     gd->flags,
+					     map,
+					     gd->clock_idx,
+					     gd->reset_idx,
+					     CLK_GATE_SET_TO_DISABLE,
+					     &aspeed_clk_lock);
+	}
+
 	return 0;
 };
 
-- 
2.14.1

[PATCH v2 4/5] clk: aspeed: Register gated clocks

[Joel Stanley]

The majority of the clocks in the system are gates paired with a reset
controller that holds the IP in reset.

This borrows from clk_hw_register_gate, but registers two 'gates', one
to control the clock enable register and the other to control the reset
IP. This allows us to enforce the ordering:

 1. Place IP in reset
 2. Enable clock
 3. Delay
 4. Release reset

There are some gates that do not have an associated reset; these are
handled by using -1 as the index for the reset.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/clk-aspeed.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 19531798e040..dec9db4ec47b 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -191,6 +191,108 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
 			mult, div);
 }
 
+static int aspeed_clk_enable(struct clk_hw *hw)
+{
+	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
+	unsigned long flags;
+	u32 clk = BIT(gate->clock_idx);
+	u32 rst = BIT(gate->reset_idx);
+
+	spin_lock_irqsave(gate->lock, flags);
+
+	if (gate->reset_idx >= 0) {
+		/* Put IP in reset */
+		regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, rst);
+
+		/* Delay 100us */
+		udelay(100);
+	}
+
+	/* Enable clock */
+	regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, 0);
+
+	if (gate->reset_idx >= 0) {
+		/* Delay 10ms */
+		/* TODO: can we sleep here? */
+		msleep(10);
+
+		/* Take IP out of reset */
+		regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, 0);
+	}
+
+	spin_unlock_irqrestore(gate->lock, flags);
+
+	return 0;
+}
+
+static void aspeed_clk_disable(struct clk_hw *hw)
+{
+	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
+	unsigned long flags;
+	u32 clk = BIT(gate->clock_idx);
+
+	spin_lock_irqsave(gate->lock, flags);
+
+	/* Disable clock */
+	regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, clk);
+
+	spin_unlock_irqrestore(gate->lock, flags);
+}
+
+static int aspeed_clk_is_enabled(struct clk_hw *hw)
+{
+	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
+	u32 clk = BIT(gate->clock_idx);
+	u32 reg;
+
+	regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
+
+	return (reg & clk) ? 0 : 1;
+}
+
+static const struct clk_ops aspeed_clk_gate_ops = {
+	.enable = aspeed_clk_enable,
+	.disable = aspeed_clk_disable,
+	.is_enabled = aspeed_clk_is_enabled,
+};
+
+static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		struct regmap *map, u8 clock_idx, u8 reset_idx,
+		u8 clk_gate_flags, spinlock_t *lock)
+{
+	struct aspeed_clk_gate *gate;
+	struct clk_init_data init;
+	struct clk_hw *hw;
+	int ret;
+
+	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
+	if (!gate)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &aspeed_clk_gate_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = parent_name ? &parent_name : NULL;
+	init.num_parents = parent_name ? 1 : 0;
+
+	gate->map = map;
+	gate->clock_idx = clock_idx;
+	gate->reset_idx = reset_idx;
+	gate->flags = clk_gate_flags;
+	gate->lock = lock;
+	gate->hw.init = &init;
+
+	hw = &gate->hw;
+	ret = clk_hw_register(dev, hw);
+	if (ret) {
+		kfree(gate);
+		hw = ERR_PTR(ret);
+	}
+
+	return hw;
+}
+
 static int __init aspeed_clk_probe(struct platform_device *pdev)
 {
 	const struct aspeed_clk_soc_data *soc_data;
@@ -200,6 +302,7 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
 	struct regmap *map;
 	struct clk_hw *hw;
 	u32 val, rate;
+	int i;
 
 	map = syscon_node_to_regmap(dev->of_node);
 	if (IS_ERR(map)) {
@@ -269,6 +372,31 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
 			&aspeed_clk_lock);
 	aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
 
+	/* There are a number of clocks that not included in this driver as
+	 * more information is required:
+	 *   D2-PLL
+	 *   D-PLL
+	 *   YCLK
+	 *   RGMII
+	 *   RMII
+	 *   UART[1..5] clock source mux
+	 */
+
+	for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) {
+		const struct aspeed_gate_data *gd;
+
+		gd = &aspeed_gates[i];
+		aspeed_clk_data->hws[ASPEED_CLK_GATES + i] =
+			aspeed_clk_hw_register_gate(NULL, gd->name,
+					     gd->parent_name,
+					     gd->flags,
+					     map,
+					     gd->clock_idx,
+					     gd->reset_idx,
+					     CLK_GATE_SET_TO_DISABLE,
+					     &aspeed_clk_lock);
+	}
+
 	return 0;
 };
 
-- 
2.14.1

[PATCH v2 5/5] clk: aspeed: Add reset controller

[Joel Stanley]

There are some resets that are not associated with gates. These are
represented by a reset controller.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/clk-aspeed.c                 | 82 +++++++++++++++++++++++++++++++-
 include/dt-bindings/clock/aspeed-clock.h |  9 ++++
 2 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index dec9db4ec47b..db97c0f9f99e 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -17,6 +17,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/reset-controller.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
@@ -256,6 +257,68 @@ static const struct clk_ops aspeed_clk_gate_ops = {
 	.is_enabled = aspeed_clk_is_enabled,
 };
 
+/**
+ * struct aspeed_reset - Aspeed reset controller
+ * @map: regmap to access the containing system controller
+ * @rcdev: reset controller device
+ */
+struct aspeed_reset {
+	struct regmap			*map;
+	struct reset_controller_dev	rcdev;
+};
+
+#define to_aspeed_reset(p) container_of((p), struct aspeed_reset, rcdev)
+
+static const u8 aspeed_resets[] = {
+	25, /* x-dma */
+	24, /* mctp */
+	23, /* adc */
+	22, /* jtag-master */
+	18, /* mic */
+	 9, /* pwm */
+	 8, /* pci-vga */
+	 2, /* i2c */
+	 1, /* ahb */
+};
+
+static int aspeed_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
+	u32 rst = BIT(aspeed_resets[id]);
+
+	return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, 0);
+}
+
+static int aspeed_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
+	u32 rst = BIT(aspeed_resets[id]);
+
+	return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, rst);
+}
+
+static int aspeed_reset_status(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
+	u32 val, rst = BIT(aspeed_resets[id]);
+	int ret;
+
+	ret = regmap_read(ar->map, ASPEED_RESET_CTRL, &val);
+	if (ret)
+		return ret;
+
+	return !!(val & rst);
+}
+
+static const struct reset_control_ops aspeed_reset_ops = {
+	.assert = aspeed_reset_assert,
+	.deassert = aspeed_reset_deassert,
+	.status = aspeed_reset_status,
+};
+
 static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		struct regmap *map, u8 clock_idx, u8 reset_idx,
@@ -299,10 +362,11 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
 	const struct clk_div_table *mac_div_table;
 	const struct clk_div_table *div_table;
 	struct device *dev = &pdev->dev;
+	struct aspeed_reset *ar;
 	struct regmap *map;
 	struct clk_hw *hw;
 	u32 val, rate;
-	int i;
+	int i, ret;
 
 	map = syscon_node_to_regmap(dev->of_node);
 	if (IS_ERR(map)) {
@@ -310,6 +374,22 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
 		return PTR_ERR(map);
 	}
 
+	ar = devm_kzalloc(dev, sizeof(*ar), GFP_KERNEL);
+	if (!ar)
+		return -ENOMEM;
+
+	ar->map = map;
+	ar->rcdev.owner = THIS_MODULE;
+	ar->rcdev.nr_resets = ARRAY_SIZE(aspeed_resets);
+	ar->rcdev.ops = &aspeed_reset_ops;
+	ar->rcdev.of_node = dev->of_node;
+
+	ret = devm_reset_controller_register(dev, &ar->rcdev);
+	if (ret) {
+		dev_err(dev, "could not register reset controller\n");
+		return ret;
+	}
+
 	/* SoC generations share common layouts but have different divisors */
 	soc_data = of_device_get_match_data(&pdev->dev);
 	div_table = soc_data->div_table;
diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
index afe06b77562d..a9d552b6bbd2 100644
--- a/include/dt-bindings/clock/aspeed-clock.h
+++ b/include/dt-bindings/clock/aspeed-clock.h
@@ -40,4 +40,13 @@
 #define ASPEED_CLK_GATE_SDCLKCLK	(22 + ASPEED_CLK_GATES)
 #define ASPEED_CLK_GATE_LHCCLK		(23 + ASPEED_CLK_GATES)
 
+#define ASPEED_RESET_XDMA		0
+#define ASPEED_RESET_MCTP		1
+#define ASPEED_RESET_JTAG_MASTER	2
+#define ASPEED_RESET_MIC		3
+#define ASPEED_RESET_PWM		4
+#define ASPEED_RESET_PCIVGA		5
+#define ASPEED_RESET_I2C		6
+#define ASPEED_RESET_AHB		7
+
 #endif
-- 
2.14.1

[PATCH v2 5/5] clk: aspeed: Add reset controller

[Joel Stanley]

There are some resets that are not associated with gates. These are
represented by a reset controller.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clk/clk-aspeed.c                 | 82 +++++++++++++++++++++++++++++++-
 include/dt-bindings/clock/aspeed-clock.h |  9 ++++
 2 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index dec9db4ec47b..db97c0f9f99e 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -17,6 +17,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/reset-controller.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
@@ -256,6 +257,68 @@ static const struct clk_ops aspeed_clk_gate_ops = {
 	.is_enabled = aspeed_clk_is_enabled,
 };
 
+/**
+ * struct aspeed_reset - Aspeed reset controller
+ * @map: regmap to access the containing system controller
+ * @rcdev: reset controller device
+ */
+struct aspeed_reset {
+	struct regmap			*map;
+	struct reset_controller_dev	rcdev;
+};
+
+#define to_aspeed_reset(p) container_of((p), struct aspeed_reset, rcdev)
+
+static const u8 aspeed_resets[] = {
+	25, /* x-dma */
+	24, /* mctp */
+	23, /* adc */
+	22, /* jtag-master */
+	18, /* mic */
+	 9, /* pwm */
+	 8, /* pci-vga */
+	 2, /* i2c */
+	 1, /* ahb */
+};
+
+static int aspeed_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
+	u32 rst = BIT(aspeed_resets[id]);
+
+	return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, 0);
+}
+
+static int aspeed_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
+	u32 rst = BIT(aspeed_resets[id]);
+
+	return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, rst);
+}
+
+static int aspeed_reset_status(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
+	u32 val, rst = BIT(aspeed_resets[id]);
+	int ret;
+
+	ret = regmap_read(ar->map, ASPEED_RESET_CTRL, &val);
+	if (ret)
+		return ret;
+
+	return !!(val & rst);
+}
+
+static const struct reset_control_ops aspeed_reset_ops = {
+	.assert = aspeed_reset_assert,
+	.deassert = aspeed_reset_deassert,
+	.status = aspeed_reset_status,
+};
+
 static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
 		const char *name, const char *parent_name, unsigned long flags,
 		struct regmap *map, u8 clock_idx, u8 reset_idx,
@@ -299,10 +362,11 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
 	const struct clk_div_table *mac_div_table;
 	const struct clk_div_table *div_table;
 	struct device *dev = &pdev->dev;
+	struct aspeed_reset *ar;
 	struct regmap *map;
 	struct clk_hw *hw;
 	u32 val, rate;
-	int i;
+	int i, ret;
 
 	map = syscon_node_to_regmap(dev->of_node);
 	if (IS_ERR(map)) {
@@ -310,6 +374,22 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
 		return PTR_ERR(map);
 	}
 
+	ar = devm_kzalloc(dev, sizeof(*ar), GFP_KERNEL);
+	if (!ar)
+		return -ENOMEM;
+
+	ar->map = map;
+	ar->rcdev.owner = THIS_MODULE;
+	ar->rcdev.nr_resets = ARRAY_SIZE(aspeed_resets);
+	ar->rcdev.ops = &aspeed_reset_ops;
+	ar->rcdev.of_node = dev->of_node;
+
+	ret = devm_reset_controller_register(dev, &ar->rcdev);
+	if (ret) {
+		dev_err(dev, "could not register reset controller\n");
+		return ret;
+	}
+
 	/* SoC generations share common layouts but have different divisors */
 	soc_data = of_device_get_match_data(&pdev->dev);
 	div_table = soc_data->div_table;
diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
index afe06b77562d..a9d552b6bbd2 100644
--- a/include/dt-bindings/clock/aspeed-clock.h
+++ b/include/dt-bindings/clock/aspeed-clock.h
@@ -40,4 +40,13 @@
 #define ASPEED_CLK_GATE_SDCLKCLK	(22 + ASPEED_CLK_GATES)
 #define ASPEED_CLK_GATE_LHCCLK		(23 + ASPEED_CLK_GATES)
 
+#define ASPEED_RESET_XDMA		0
+#define ASPEED_RESET_MCTP		1
+#define ASPEED_RESET_JTAG_MASTER	2
+#define ASPEED_RESET_MIC		3
+#define ASPEED_RESET_PWM		4
+#define ASPEED_RESET_PCIVGA		5
+#define ASPEED_RESET_I2C		6
+#define ASPEED_RESET_AHB		7
+
 #endif
-- 
2.14.1

Re: [PATCH v2 1/5] clk: Add clock driver for ASPEED BMC SoCs

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 11937 bytes --]

On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:
> This adds the stub of a driver for the ASPEED SoCs. The clocks are
> defined and the static registration is set up.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/clk/Kconfig                      |  12 +++
>  drivers/clk/Makefile                     |   1 +
>  drivers/clk/clk-aspeed.c                 | 162 +++++++++++++++++++++++++++++++
>  include/dt-bindings/clock/aspeed-clock.h |  43 ++++++++
>  4 files changed, 218 insertions(+)
>  create mode 100644 drivers/clk/clk-aspeed.c
>  create mode 100644 include/dt-bindings/clock/aspeed-clock.h
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 1c4e1aa6767e..9abe063ef8d2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -142,6 +142,18 @@ config COMMON_CLK_GEMINI
>  	  This driver supports the SoC clocks on the Cortina Systems Gemini
>  	  platform, also known as SL3516 or CS3516.
>  
> +config COMMON_CLK_ASPEED
> +	bool "Clock driver for Aspeed BMC SoCs"
> +	depends on ARCH_ASPEED || COMPILE_TEST
> +	default ARCH_ASPEED
> +	select MFD_SYSCON
> +	select RESET_CONTROLLER
> +	---help---
> +	  This driver supports the SoC clocks on the Aspeed BMC platforms.
>
> +	  The G4 and G5 series, including the ast2400 and ast2500, are supported
> +	  by this driver.
> +
>  config COMMON_CLK_S2MPS11
>  	tristate "Clock driver for S2MPS1X/S5M8767 MFD"
>  	depends on MFD_SEC_CORE || COMPILE_TEST
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index c99f363826f0..575c68919d9b 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
>  obj-$(CONFIG_COMMON_CLK_CS2000_CP)	+= clk-cs2000-cp.o
>  obj-$(CONFIG_ARCH_EFM32)		+= clk-efm32gg.o
>  obj-$(CONFIG_COMMON_CLK_GEMINI)		+= clk-gemini.o
> +obj-$(CONFIG_COMMON_CLK_ASPEED)		+= clk-aspeed.o
>  obj-$(CONFIG_ARCH_HIGHBANK)		+= clk-highbank.o
>  obj-$(CONFIG_CLK_HSDK)			+= clk-hsdk-pll.o
>  obj-$(CONFIG_COMMON_CLK_MAX77686)	+= clk-max77686.o
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> new file mode 100644
> index 000000000000..824c54767009
> --- /dev/null
> +++ b/drivers/clk/clk-aspeed.c
> @@ -0,0 +1,162 @@
> +/*
> + * Copyright 2017 IBM Corporation
> + *
> + * Joel Stanley <joel@jms.id.au>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#define pr_fmt(fmt) "clk-aspeed: " fmt
> +
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/spinlock.h>
> +#include <linux/mfd/syscon.h>
> +
> +#include <dt-bindings/clock/aspeed-clock.h>
> +
> +#define ASPEED_RESET_CTRL	0x04
> +#define ASPEED_CLK_SELECTION	0x08
> +#define ASPEED_CLK_STOP_CTRL	0x0c
> +#define ASPEED_MPLL_PARAM	0x20
> +#define ASPEED_HPLL_PARAM	0x24
> +#define ASPEED_MISC_CTRL	0x2c
> +#define ASPEED_STRAP		0x70
> +
> +/* Keeps track of all clocks */
> +static struct clk_hw_onecell_data *aspeed_clk_data;
> +
> +static void __iomem *scu_base;
> +
> +/**
> + * struct aspeed_gate_data - Aspeed gated clocks
> + * @clock_idx: bit used to gate this clock in the clock register
> + * @reset_idx: bit used to reset this IP in the reset register. -1 if no
> + *             reset is required when enabling the clock
> + * @name: the clock name
> + * @parent_name: the name of the parent clock
> + * @flags: standard clock framework flags
> + */
> +struct aspeed_gate_data {
> +	u8		clock_idx;
> +	s8		reset_idx;
> +	const char	*name;
> +	const char	*parent_name;
> +	unsigned long	flags;
> +};
> +
> +/**
> + * struct aspeed_clk_gate - Aspeed specific clk_gate structure
> + * @hw:		handle between common and hardware-specific interfaces
> + * @reg:	register controlling gate
> + * @clock_idx:	bit used to gate this clock in the clock register
> + * @reset_idx:	bit used to reset this IP in the reset register. -1 if no
> + *		reset is required when enabling the clock
> + * @flags:	hardware-specific flags
> + * @lock:	register lock
> + *
> + * Some of the clocks in the Aspeed SoC must be put in reset before enabling.
> + * This modified version of clk_gate allows an optional reset bit to be
> + * specified.
> + */
> +struct aspeed_clk_gate {
> +	struct clk_hw	hw;
> +	struct regmap	*map;
> +	u8		clock_idx;
> +	s8		reset_idx;
> +	u8		flags;
> +	spinlock_t	*lock;
> +};

It feels like the two structures could be unified, but the result turns into a
bit of a mess with a union of structs to limit the space impact, so perhaps we
shouldn't go there?

> +
> +#define to_aspeed_clk_gate(_hw) container_of(_hw, struct aspeed_clk_gate, hw)
> +
> +/* TODO: ask Aspeed about the actual parent data */
> +static const struct aspeed_gate_data aspeed_gates[] __initconst = {
> +/*	 clk rst   name			parent		flags	*/
> +	{  0, -1, "eclk-gate",		"eclk",		0 }, /* Video Engine */
> +	{  1,  7, "gclk-gate",		NULL,		0 }, /* 2D engine */
> +	{  2, -1, "mclk-gate",		"mpll",		CLK_IS_CRITICAL }, /* SDRAM */
> +	{  3,  6, "vclk-gate",		NULL,		0 }, /* Video Capture */
> +	{  4, 10, "bclk-gate",		"bclk",		0 }, /* PCIe/PCI */
> +	{  5, -1, "dclk-gate",		NULL,		0 }, /* DAC */
> +	{  6, -1, "refclk-gate",	"clkin",	CLK_IS_CRITICAL },
> +	{  7,  3, "usb-port2-gate",	NULL,		0 }, /* USB2.0 Host port 2 */
> +	{  8,  5, "lclk-gate",		NULL,		0 }, /* LPC */
> +	{  9, 15, "usb-uhci-gate",	NULL,		0 }, /* USB1.1 (requires port 2 enabled) */
> +	{ 10, 13, "d1clk-gate",		NULL,		0 }, /* GFX CRT */

Is there a typo in the AST2400 datasheet? It claims bit 10 is "D2CLK".

> +	/* 11: reserved */
> +	/* 12: reserved */
> +	{ 13, 4,  "yclk-gate",		NULL,		0 }, /* HAC */
> +	{ 14, 14, "usb-port1-gate",	NULL,		0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
> +	{ 15, -1, "uart1clk-gate",	"uart",		0 }, /* UART1 */
> +	{ 16, -1, "uart2clk-gate",	"uart",		0 }, /* UART2 */
> +	{ 17, -1, "uart5clk-gate",	"uart",		0 }, /* UART5 */
> +	/* 18: reserved */
> +	{ 19, -1, "espiclk-gate",	NULL,		0 }, /* eSPI */

19 is reserved on the AST2400, and must be kept at '1'.

> +	{ 20, 11, "mac1clk-gate",	"clkin",	0 }, /* MAC1 */
> +	{ 21, 12, "mac2clk-gate",	"clkin",	0 }, /* MAC2 */
> +	/* 22: reserved */
> +	/* 23: reserved */
> +	{ 24, -1, "rsaclk-gate",	NULL,		0 }, /* RSA */
> +	{ 25, -1, "uart3clk-gate",	"uart",		0 }, /* UART3 */
> +	{ 26, -1, "uart4clk-gate",	"uart",		0 }, /* UART4 */
> +	{ 27, 16, "sdclk-gate",		NULL,		0 }, /* SDIO/SD */
> +	{ 28, -1, "lhclk-gate",		"lhclk",	0 }, /* LPC master/LPC+ */
> +	/* 29: reserved */
> +	/* 30: reserved */
> +	/* 31: reserved */

Interestingly bits 29-30 are reserved in both the AST2400 and AST2500
datasheets, but are reserved-clear in the AST2400 datasheet and reserved-set in
the AST2500 datasheet. This may affect how we write register.

Separately, at one point I mistook the clk column for the index the entry
should appear at, which isn't the case. Do you think designated intializers
might help?

> +};
> +
> +static void __init aspeed_cc_init(struct device_node *np)
> +{
> +	struct regmap *map;
> +	u32 val;
> +	int ret;
> +	int i;
>
> +	scu_base = of_iomap(np, 0);
> +	if (IS_ERR(scu_base))
> +		return;
> +
> +	aspeed_clk_data = kzalloc(sizeof(*aspeed_clk_data) +
> +			sizeof(*aspeed_clk_data->hws) * ASPEED_NUM_CLKS,
> +			GFP_KERNEL);
> +	if (!aspeed_clk_data)
> +		return;
> +
> +	/*
> +	 * This way all clock fetched before the platform device probes,

Typo: "clocks"

> +	 * except those we assign here for early use, will be deferred.
> +	 */
> +	for (i = 0; i < ASPEED_NUM_CLKS; i++)
> +		aspeed_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> +
> +	map = syscon_node_to_regmap(np);
> +	if (IS_ERR(map)) {
> +		pr_err("no syscon regmap\n");
> +		return;
> +	}
> +	/*
> +	 * We check that the regmap works on this very first access,
> +	 * but as this is an MMIO-backed regmap, subsequent regmap
> +	 * access is not going to fail and we skip error checks from
> +	 * this point.
> +	 */
> +	ret = regmap_read(map, ASPEED_STRAP, &val);
> +	if (ret) {
> +		pr_err("failed to read strapping register\n");
> +		return;
> +	}
> +
> +	aspeed_clk_data->num = ASPEED_NUM_CLKS;
> +	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
> +	if (ret)
> +		pr_err("failed to add DT provider: %d\n", ret);
> +};
> +CLK_OF_DECLARE_DRIVER(aspeed_cc_g5, "aspeed,ast2500-scu", aspeed_cc_init);
> +CLK_OF_DECLARE_DRIVER(aspeed_cc_g4, "aspeed,ast2400-scu", aspeed_cc_init);
> diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
> new file mode 100644
> index 000000000000..afe06b77562d
> --- /dev/null
> +++ b/include/dt-bindings/clock/aspeed-clock.h
> @@ -0,0 +1,43 @@
> +#ifndef DT_BINDINGS_ASPEED_CLOCK_H
> +#define DT_BINDINGS_ASPEED_CLOCK_H
> +
> +#define ASPEED_NUM_CLKS	34

This is a bit of a nit pick: Do the constants below represent all the
clocks in the SoC(s)? Maybe if not it's better to define
ASPEED_NUM_CLKS in terms of (n + ASPEED_CLK_GATES) at the bottom - that
way when a clock or gate is added ASPEED_NUM_CLKS has better locality and
better visual progression in context.

As a note, and as a clk noob but as someone with access to the Aspeed
datasheet, I was wondering whether the clocks and gates need to be represented
in the same number space. However it seems to be so by how it's used with
respect to struct clk_hw_onecell_data.

> +
> +#define ASPEED_CLK_HPLL			0
> +#define ASPEED_CLK_AHB			1
> +#define ASPEED_CLK_APB			2
> +#define ASPEED_CLK_UART			3
> +#define ASPEED_CLK_SDIO			4
> +#define ASPEED_CLK_ECLK			5
> +#define ASPEED_CLK_ECLK_MUX		6
> +#define ASPEED_CLK_LHCLK		7
> +#define ASPEED_CLK_MAC			8
> +#define ASPEED_CLK_BCLK			9
> +#define ASPEED_CLK_MPLL			10
> +#define ASPEED_CLK_GATES		11
> +#define ASPEED_CLK_GATE_ECLK		(0 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_GCLK		(1 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_MCLK		(2 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_VCLK		(3 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_BCLK		(4 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_DCLK		(5 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_REFCLK		(6 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_USBPORT2CLK	(7 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_LCLK		(8 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_USBUHCICLK	(9 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_D1CLK		(10 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_YCLK		(11 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_USBPORT1CLK	(12 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_UART1CLK	(13 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_UART2CLK	(14 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_UART5CLK	(15 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_ESPICLK		(16 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_MAC1CLK		(17 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_MAC2CLK		(18 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_RSACLK		(19 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_UART3CLK	(20 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_UART4CLK	(21 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_SDCLKCLK	(22 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_LHCCLK		(23 + ASPEED_CLK_GATES)

Looks like there's an off-by-one error here: ASPEED_NUM_CLKS should be (23 +
ASPEED_CLK_GATES + 1) = 35.

Cheers,

Andrew

> +
> +#endif

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v2 1/5] clk: Add clock driver for ASPEED BMC SoCs

[Andrew Jeffery]

On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:
> This adds the stub of a driver for the ASPEED SoCs. The clocks are
> defined and the static registration is set up.
>?
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> ?drivers/clk/Kconfig??????????????????????|??12 +++
> ?drivers/clk/Makefile?????????????????????|???1 +
> ?drivers/clk/clk-aspeed.c?????????????????| 162 +++++++++++++++++++++++++++++++
> ?include/dt-bindings/clock/aspeed-clock.h |??43 ++++++++
> ?4 files changed, 218 insertions(+)
> ?create mode 100644 drivers/clk/clk-aspeed.c
> ?create mode 100644 include/dt-bindings/clock/aspeed-clock.h
>?
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 1c4e1aa6767e..9abe063ef8d2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -142,6 +142,18 @@ config COMMON_CLK_GEMINI
> ?	??This driver supports the SoC clocks on the Cortina Systems Gemini
> ?	??platform, also known as SL3516 or CS3516.
> ?
> +config COMMON_CLK_ASPEED
> +	bool "Clock driver for Aspeed BMC SoCs"
> +	depends on ARCH_ASPEED || COMPILE_TEST
> +	default ARCH_ASPEED
> +	select MFD_SYSCON
> +	select RESET_CONTROLLER
> +	---help---
> +	??This driver supports the SoC clocks on the Aspeed BMC platforms.
>
> +	??The G4 and G5 series, including the ast2400 and ast2500, are supported
> +	??by this driver.
> +
> ?config COMMON_CLK_S2MPS11
> ?	tristate "Clock driver for S2MPS1X/S5M8767 MFD"
> ?	depends on MFD_SEC_CORE || COMPILE_TEST
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index c99f363826f0..575c68919d9b 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
> ?obj-$(CONFIG_COMMON_CLK_CS2000_CP)	+= clk-cs2000-cp.o
> ?obj-$(CONFIG_ARCH_EFM32)		+= clk-efm32gg.o
> ?obj-$(CONFIG_COMMON_CLK_GEMINI)		+= clk-gemini.o
> +obj-$(CONFIG_COMMON_CLK_ASPEED)		+= clk-aspeed.o
> ?obj-$(CONFIG_ARCH_HIGHBANK)		+= clk-highbank.o
> ?obj-$(CONFIG_CLK_HSDK)			+= clk-hsdk-pll.o
> ?obj-$(CONFIG_COMMON_CLK_MAX77686)	+= clk-max77686.o
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> new file mode 100644
> index 000000000000..824c54767009
> --- /dev/null
> +++ b/drivers/clk/clk-aspeed.c
> @@ -0,0 +1,162 @@
> +/*
> + * Copyright 2017 IBM Corporation
> + *
> + * Joel Stanley <joel@jms.id.au>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#define pr_fmt(fmt) "clk-aspeed: " fmt
> +
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/spinlock.h>
> +#include <linux/mfd/syscon.h>
> +
> +#include <dt-bindings/clock/aspeed-clock.h>
> +
> +#define ASPEED_RESET_CTRL	0x04
> +#define ASPEED_CLK_SELECTION	0x08
> +#define ASPEED_CLK_STOP_CTRL	0x0c
> +#define ASPEED_MPLL_PARAM	0x20
> +#define ASPEED_HPLL_PARAM	0x24
> +#define ASPEED_MISC_CTRL	0x2c
> +#define ASPEED_STRAP		0x70
> +
> +/* Keeps track of all clocks */
> +static struct clk_hw_onecell_data *aspeed_clk_data;
> +
> +static void __iomem *scu_base;
> +
> +/**
> + * struct aspeed_gate_data - Aspeed gated clocks
> + * @clock_idx: bit used to gate this clock in the clock register
> + * @reset_idx: bit used to reset this IP in the reset register. -1 if no
> + *?????????????reset is required when enabling the clock
> + * @name: the clock name
> + * @parent_name: the name of the parent clock
> + * @flags: standard clock framework flags
> + */
> +struct aspeed_gate_data {
> +	u8		clock_idx;
> +	s8		reset_idx;
> +	const char	*name;
> +	const char	*parent_name;
> +	unsigned long	flags;
> +};
> +
> +/**
> + * struct aspeed_clk_gate - Aspeed specific clk_gate structure
> + * @hw:		handle between common and hardware-specific interfaces
> + * @reg:	register controlling gate
> + * @clock_idx:	bit used to gate this clock in the clock register
> + * @reset_idx:	bit used to reset this IP in the reset register. -1 if no
> + *		reset is required when enabling the clock
> + * @flags:	hardware-specific flags
> + * @lock:	register lock
> + *
> + * Some of the clocks in the Aspeed SoC must be put in reset before enabling.
> + * This modified version of clk_gate allows an optional reset bit to be
> + * specified.
> + */
> +struct aspeed_clk_gate {
> +	struct clk_hw	hw;
> +	struct regmap	*map;
> +	u8		clock_idx;
> +	s8		reset_idx;
> +	u8		flags;
> +	spinlock_t	*lock;
> +};

It feels like the two structures could be unified, but the result turns into a
bit of a mess with a union of structs to limit the space impact, so perhaps we
shouldn't go there?

> +
> +#define to_aspeed_clk_gate(_hw) container_of(_hw, struct aspeed_clk_gate, hw)
> +
> +/* TODO: ask Aspeed about the actual parent data */
> +static const struct aspeed_gate_data aspeed_gates[] __initconst = {
> +/*	?clk rst???name			parent		flags	*/
> +	{??0, -1, "eclk-gate",		"eclk",		0 }, /* Video Engine */
> +	{??1,??7, "gclk-gate",		NULL,		0 }, /* 2D engine */
> +	{??2, -1, "mclk-gate",		"mpll",		CLK_IS_CRITICAL }, /* SDRAM */
> +	{??3,??6, "vclk-gate",		NULL,		0 }, /* Video Capture */
> +	{??4, 10, "bclk-gate",		"bclk",		0 }, /* PCIe/PCI */
> +	{??5, -1, "dclk-gate",		NULL,		0 }, /* DAC */
> +	{??6, -1, "refclk-gate",	"clkin",	CLK_IS_CRITICAL },
> +	{??7,??3, "usb-port2-gate",	NULL,		0 }, /* USB2.0 Host port 2 */
> +	{??8,??5, "lclk-gate",		NULL,		0 }, /* LPC */
> +	{??9, 15, "usb-uhci-gate",	NULL,		0 }, /* USB1.1 (requires port 2 enabled) */
> +	{ 10, 13, "d1clk-gate",		NULL,		0 }, /* GFX CRT */

Is there a typo in the AST2400 datasheet? It claims bit 10 is "D2CLK".

> +	/* 11: reserved */
> +	/* 12: reserved */
> +	{ 13, 4,??"yclk-gate",		NULL,		0 }, /* HAC */
> +	{ 14, 14, "usb-port1-gate",	NULL,		0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
> +	{ 15, -1, "uart1clk-gate",	"uart",		0 }, /* UART1 */
> +	{ 16, -1, "uart2clk-gate",	"uart",		0 }, /* UART2 */
> +	{ 17, -1, "uart5clk-gate",	"uart",		0 }, /* UART5 */
> +	/* 18: reserved */
> +	{ 19, -1, "espiclk-gate",	NULL,		0 }, /* eSPI */

19 is reserved on the AST2400, and must be kept at '1'.

> +	{ 20, 11, "mac1clk-gate",	"clkin",	0 }, /* MAC1 */
> +	{ 21, 12, "mac2clk-gate",	"clkin",	0 }, /* MAC2 */
> +	/* 22: reserved */
> +	/* 23: reserved */
> +	{ 24, -1, "rsaclk-gate",	NULL,		0 }, /* RSA */
> +	{ 25, -1, "uart3clk-gate",	"uart",		0 }, /* UART3 */
> +	{ 26, -1, "uart4clk-gate",	"uart",		0 }, /* UART4 */
> +	{ 27, 16, "sdclk-gate",		NULL,		0 }, /* SDIO/SD */
> +	{ 28, -1, "lhclk-gate",		"lhclk",	0 }, /* LPC master/LPC+ */
> +	/* 29: reserved */
> +	/* 30: reserved */
> +	/* 31: reserved */

Interestingly bits 29-30 are reserved in both the AST2400 and AST2500
datasheets, but are reserved-clear in the AST2400 datasheet and reserved-set in
the AST2500 datasheet. This may affect how we write register.

Separately, at one point I mistook the clk column for the index the entry
should appear at, which isn't the case. Do you think designated intializers
might help?

> +};
> +
> +static void __init aspeed_cc_init(struct device_node *np)
> +{
> +	struct regmap *map;
> +	u32 val;
> +	int ret;
> +	int i;
>
> +	scu_base = of_iomap(np, 0);
> +	if (IS_ERR(scu_base))
> +		return;
> +
> +	aspeed_clk_data = kzalloc(sizeof(*aspeed_clk_data) +
> +			sizeof(*aspeed_clk_data->hws) * ASPEED_NUM_CLKS,
> +			GFP_KERNEL);
> +	if (!aspeed_clk_data)
> +		return;
> +
> +	/*
> +	?* This way all clock fetched before the platform device probes,

Typo: "clocks"

> +	?* except those we assign here for early use, will be deferred.
> +	?*/
> +	for (i = 0; i < ASPEED_NUM_CLKS; i++)
> +		aspeed_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> +
> +	map = syscon_node_to_regmap(np);
> +	if (IS_ERR(map)) {
> +		pr_err("no syscon regmap\n");
> +		return;
> +	}
> +	/*
> +	?* We check that the regmap works on this very first access,
> +	?* but as this is an MMIO-backed regmap, subsequent regmap
> +	?* access is not going to fail and we skip error checks from
> +	?* this point.
> +	?*/
> +	ret = regmap_read(map, ASPEED_STRAP, &val);
> +	if (ret) {
> +		pr_err("failed to read strapping register\n");
> +		return;
> +	}
> +
> +	aspeed_clk_data->num = ASPEED_NUM_CLKS;
> +	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
> +	if (ret)
> +		pr_err("failed to add DT provider: %d\n", ret);
> +};
> +CLK_OF_DECLARE_DRIVER(aspeed_cc_g5, "aspeed,ast2500-scu", aspeed_cc_init);
> +CLK_OF_DECLARE_DRIVER(aspeed_cc_g4, "aspeed,ast2400-scu", aspeed_cc_init);
> diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
> new file mode 100644
> index 000000000000..afe06b77562d
> --- /dev/null
> +++ b/include/dt-bindings/clock/aspeed-clock.h
> @@ -0,0 +1,43 @@
> +#ifndef DT_BINDINGS_ASPEED_CLOCK_H
> +#define DT_BINDINGS_ASPEED_CLOCK_H
> +
> +#define ASPEED_NUM_CLKS	34

This is a bit of a nit pick: Do the constants below represent all the
clocks in the SoC(s)? Maybe if not it's better to define
ASPEED_NUM_CLKS in terms of (n + ASPEED_CLK_GATES) at the bottom - that
way when a clock or gate is added ASPEED_NUM_CLKS has better locality and
better visual progression in context.

As a note, and as a clk noob but as someone with access to the Aspeed
datasheet, I was wondering whether the clocks and gates need to be represented
in the same number space. However it seems to be so by how it's used with
respect to struct clk_hw_onecell_data.

> +
> +#define ASPEED_CLK_HPLL			0
> +#define ASPEED_CLK_AHB			1
> +#define ASPEED_CLK_APB			2
> +#define ASPEED_CLK_UART			3
> +#define ASPEED_CLK_SDIO			4
> +#define ASPEED_CLK_ECLK			5
> +#define ASPEED_CLK_ECLK_MUX		6
> +#define ASPEED_CLK_LHCLK		7
> +#define ASPEED_CLK_MAC			8
> +#define ASPEED_CLK_BCLK			9
> +#define ASPEED_CLK_MPLL			10
> +#define ASPEED_CLK_GATES		11
> +#define ASPEED_CLK_GATE_ECLK		(0 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_GCLK		(1 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_MCLK		(2 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_VCLK		(3 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_BCLK		(4 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_DCLK		(5 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_REFCLK		(6 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_USBPORT2CLK	(7 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_LCLK		(8 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_USBUHCICLK	(9 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_D1CLK		(10 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_YCLK		(11 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_USBPORT1CLK	(12 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_UART1CLK	(13 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_UART2CLK	(14 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_UART5CLK	(15 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_ESPICLK		(16 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_MAC1CLK		(17 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_MAC2CLK		(18 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_RSACLK		(19 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_UART3CLK	(20 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_UART4CLK	(21 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_SDCLKCLK	(22 + ASPEED_CLK_GATES)
> +#define ASPEED_CLK_GATE_LHCCLK		(23 + ASPEED_CLK_GATES)

Looks like there's an off-by-one error here: ASPEED_NUM_CLKS should be (23 +
ASPEED_CLK_GATES + 1) = 35.

Cheers,

Andrew

> +
> +#endif
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170925/92e3efd1/attachment.sig>

Re: [PATCH v2 2/5] clk: aspeed: Register core clocks

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 7354 bytes --]

On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:
> This registers the core clocks; those which are required to calculate
> the rate of the timer periperhal so the system can load a clocksource
> driver.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/clk/clk-aspeed.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 149 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 824c54767009..e614c61b82d2 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -11,12 +11,12 @@
>  
>  #define pr_fmt(fmt) "clk-aspeed: " fmt
>  
> -#include <linux/slab.h>
> -#include <linux/of_address.h>
>  #include <linux/clk-provider.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
>  #include <linux/regmap.h>
> +#include <linux/slab.h>
>  #include <linux/spinlock.h>
> -#include <linux/mfd/syscon.h>
>  
>  #include <dt-bindings/clock/aspeed-clock.h>
>  
> @@ -28,6 +28,9 @@
>  #define ASPEED_MISC_CTRL	0x2c
>  #define ASPEED_STRAP		0x70
>  
> +/* Globally visible clocks */
> +static DEFINE_SPINLOCK(aspeed_clk_lock);
> +
>  /* Keeps track of all clocks */
>  static struct clk_hw_onecell_data *aspeed_clk_data;
>  
> @@ -112,9 +115,137 @@ static const struct aspeed_gate_data aspeed_gates[] __initconst = {
>  	/* 31: reserved */
>  };
>  
> +static const struct clk_div_table ast2400_div_table[] = {
> +	{ 0x0, 2 },
> +	{ 0x1, 4 },
> +	{ 0x2, 6 },
> +	{ 0x3, 8 },
> +	{ 0x4, 10 },
> +	{ 0x5, 12 },
> +	{ 0x6, 14 },
> +	{ 0x7, 16 },
> +	{ 0 }
> +};
> +
> +static const struct clk_div_table ast2500_div_table[] = {
> +	{ 0x0, 4 },
> +	{ 0x1, 8 },
> +	{ 0x2, 12 },
> +	{ 0x3, 16 },
> +	{ 0x4, 20 },
> +	{ 0x5, 24 },
> +	{ 0x6, 28 },
> +	{ 0x7, 32 },
> +	{ 0 }
> +};

ast2500_div_table is unused?

> +
> +static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
> +{
> +	unsigned int mult, div;
> +
> +	if (val & BIT(20)) {
> +		/* Pass through mode */
> +		mult = div = 1;
> +	} else {
> +		/* F = clkin * [(M+1) / (N+1)] / (P + 1) */
> +		u32 p = (val >> 13) & 0x3f;
> +		u32 m = (val >> 5) & 0xff;
> +		u32 n = val & 0x1f;
> +
> +		mult = (m + 1) / (n + 1);
> +		div = p + 1;
> +	}
> +
> +	return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
> +			mult, div);
> +}
> +
> +static void __init aspeed_ast2400_cc(struct regmap *map)
> +{
> +	struct clk_hw *hw;
> +	u32 val, div, mult;
> +
> +	/*
> +	 * High-speed PLL clock derived from the crystal. This the CPU clock,
> +	 * and we assume that it is enabled
> +	 */
> +	regmap_read(map, ASPEED_HPLL_PARAM, &val);
> +	WARN(val & BIT(18), "clock is strapped not configured");

I don't quite understand the intent of the warning - are we just trying to say
that the defaults have been assumed and they may not match reality? It's
probably not a great idea to not strap the board properly, but you could get
away with it for a given configuration (Numerator = 20, H-PLL Output Divider
set, H-PLL denominator = 1)

> +	if (val & BIT(17)) {
> +		/* Pass through mode */
> +		mult = div = 1;
> +	} else {
> +		/* F = 24Mhz * (2-OD) * [(N + 2) / (D + 1)] */
> +		u32 n = (val >> 5) & 0x3f;
> +		u32 od = (val >> 4) & 0x1;
> +		u32 d = val & 0xf;
> +
> +		mult = (2 - od) * (n + 2);
> +		div = d + 1;
> +	}
> +	hw = clk_hw_register_fixed_factor(NULL, "hpll", "clkin", 0, mult, div);
> +	aspeed_clk_data->hws[ASPEED_CLK_HPLL] = hw;
> +
> +	/*
> +	 * Strap bits 11:10 define the CPU/AHB clock frequency ratio (aka HCLK)
> +	 *   00: Select CPU:AHB = 1:1
> +	 *   01: Select CPU:AHB = 2:1
> +	 *   10: Select CPU:AHB = 4:1
> +	 *   11: Select CPU:AHB = 3:1
> +	 */
> +	regmap_read(map, ASPEED_STRAP, &val);
> +	val = (val >> 10) & 0x3;

How do the calculations below line up with the comment above? I can't see the
connection and I feel like I've missed something.

> +	div = val + 1;

This is only a valid mapping for 0b00 and 0b01?

> +	if (div == 2)
> +		div = 3;

If (div == 2) after adding 1, then the original value was 0b01, and so div
should remain 2?

> +	else if (div == 3)
> +		div = 2;

If (div == 3) after adding 1, then the original value was 0b10, and so div
should be 4?

> +	hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
> +	aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
> +
> +	/* APB clock clock selection register SCU08 (aka PCLK) */
> +	hw = clk_hw_register_divider_table(NULL, "apb", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 23, 3, 0,
> +			ast2400_div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
> +}
> +
> +static void __init aspeed_ast2500_cc(struct regmap *map)
> +{
> +	struct clk_hw *hw;
> +	u32 val, div;
> +
> +	/*
> +	 * High-speed PLL clock derived from the crystal. This the CPU clock,
> +	 * and we assume that it is enabled
> +	 */
> +	regmap_read(map, ASPEED_HPLL_PARAM, &val);
> +	aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_calc_pll("hpll", val);

Why did you extract the aspeed_calc_pll() implementation for the ast2500 but
not the ast2400?

> +
> +	/* Strap bits 11:9 define the AXI/AHB clock frequency ratio (aka HCLK)*/
> +	regmap_read(map, ASPEED_STRAP, &val);
> +	val = (val >> 9) & 0x7;
> +	WARN_ON(val == 0);

Maybe WARN() would be better here to explain what's going wrong (zero is an
undefined value)?

> +	div = 2 * (val + 1);
> +	hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
> +	aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
> +
> +	/* APB clock clock selection register SCU08 (aka PCLK) */
> +	/* TODO: this is wrong! */

Why is this wrong?

> +	regmap_read(map, ASPEED_CLK_SELECTION, &val);
> +	val = (val >> 23) & 0x7;
> +	div = 4 * (val + 1);
> +	hw = clk_hw_register_fixed_factor(NULL, "apb", "hpll", 0, 1, div);
> +	aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
> +};
> +
> +
>  static void __init aspeed_cc_init(struct device_node *np)
>  {
>  	struct regmap *map;
> +	unsigned long freq;
> +	struct clk_hw *hw;
>  	u32 val;
>  	int ret;
>  	int i;
> @@ -153,6 +284,21 @@ static void __init aspeed_cc_init(struct device_node *np)
>  		return;
>  	}
>  
> +	/* CLKIN is the crystal oscillator, 24 or 25MHz selected by strapping */
> +	if (val & BIT(23))
> +		freq = 25000000;
> +	else
> +		freq = 24000000;

This isn't true on the AST2400, where CLKIN could be 24 OR 48MHz when BIT(23) is
clear. You need to test BIT(18) as well on the AST2400 to disambiguate.

> +	hw = clk_hw_register_fixed_rate(NULL, "clkin", NULL, 0, freq);
> +	pr_debug("clkin @%lu MHz\n", freq / 1000000);
> +
> +	if (of_device_is_compatible(np, "aspeed,ast2400-scu"))
> +		aspeed_ast2400_cc(map);
> +	else if (of_device_is_compatible(np, "aspeed,ast2500-scu"))
> +		aspeed_ast2500_cc(map);
> +	else
> +		pr_err("unknown platform, failed to add clocks\n");

I'm not confident that "borrowing" the SCU compatible like this is entirely in
the spirit of its MFD nature, but maybe others can chime in.

Cheers,

Andrew

> +
>  	aspeed_clk_data->num = ASPEED_NUM_CLKS;
>  	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
>  	if (ret)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v2 2/5] clk: aspeed: Register core clocks

[Andrew Jeffery]

On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:
> This registers the core clocks; those which are required to calculate
> the rate of the timer periperhal so the system can load a clocksource
> driver.
>?
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> ?drivers/clk/clk-aspeed.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++-
> ?1 file changed, 149 insertions(+), 3 deletions(-)
>?
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 824c54767009..e614c61b82d2 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -11,12 +11,12 @@
> ?
> ?#define pr_fmt(fmt) "clk-aspeed: " fmt
> ?
> -#include <linux/slab.h>
> -#include <linux/of_address.h>
> ?#include <linux/clk-provider.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
> ?#include <linux/regmap.h>
> +#include <linux/slab.h>
> ?#include <linux/spinlock.h>
> -#include <linux/mfd/syscon.h>
> ?
> ?#include <dt-bindings/clock/aspeed-clock.h>
> ?
> @@ -28,6 +28,9 @@
> ?#define ASPEED_MISC_CTRL	0x2c
> ?#define ASPEED_STRAP		0x70
> ?
> +/* Globally visible clocks */
> +static DEFINE_SPINLOCK(aspeed_clk_lock);
> +
> ?/* Keeps track of all clocks */
> ?static struct clk_hw_onecell_data *aspeed_clk_data;
> ?
> @@ -112,9 +115,137 @@ static const struct aspeed_gate_data aspeed_gates[] __initconst = {
> ?	/* 31: reserved */
> ?};
> ?
> +static const struct clk_div_table ast2400_div_table[] = {
> +	{ 0x0, 2 },
> +	{ 0x1, 4 },
> +	{ 0x2, 6 },
> +	{ 0x3, 8 },
> +	{ 0x4, 10 },
> +	{ 0x5, 12 },
> +	{ 0x6, 14 },
> +	{ 0x7, 16 },
> +	{ 0 }
> +};
> +
> +static const struct clk_div_table ast2500_div_table[] = {
> +	{ 0x0, 4 },
> +	{ 0x1, 8 },
> +	{ 0x2, 12 },
> +	{ 0x3, 16 },
> +	{ 0x4, 20 },
> +	{ 0x5, 24 },
> +	{ 0x6, 28 },
> +	{ 0x7, 32 },
> +	{ 0 }
> +};

ast2500_div_table is unused?

> +
> +static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
> +{
> +	unsigned int mult, div;
> +
> +	if (val & BIT(20)) {
> +		/* Pass through mode */
> +		mult = div = 1;
> +	} else {
> +		/* F = clkin * [(M+1) / (N+1)] / (P + 1) */
> +		u32 p = (val >> 13) & 0x3f;
> +		u32 m = (val >> 5) & 0xff;
> +		u32 n = val & 0x1f;
> +
> +		mult = (m + 1) / (n + 1);
> +		div = p + 1;
> +	}
> +
> +	return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
> +			mult, div);
> +}
> +
> +static void __init aspeed_ast2400_cc(struct regmap *map)
> +{
> +	struct clk_hw *hw;
> +	u32 val, div, mult;
> +
> +	/*
> +	?* High-speed PLL clock derived from the crystal. This the CPU clock,
> +	?* and we assume that it is enabled
> +	?*/
> +	regmap_read(map, ASPEED_HPLL_PARAM, &val);
> +	WARN(val & BIT(18), "clock is strapped not configured");

I don't quite understand the intent of the warning - are we just trying to say
that the defaults have been assumed and they may not match reality? It's
probably not a great idea to not strap the board properly, but you could get
away with it for a given configuration (Numerator = 20, H-PLL Output Divider
set, H-PLL denominator = 1)

> +	if (val & BIT(17)) {
> +		/* Pass through mode */
> +		mult = div = 1;
> +	} else {
> +		/* F = 24Mhz * (2-OD) * [(N + 2) / (D + 1)] */
> +		u32 n = (val >> 5) & 0x3f;
> +		u32 od = (val >> 4) & 0x1;
> +		u32 d = val & 0xf;
> +
> +		mult = (2 - od) * (n + 2);
> +		div = d + 1;
> +	}
> +	hw = clk_hw_register_fixed_factor(NULL, "hpll", "clkin", 0, mult, div);
> +	aspeed_clk_data->hws[ASPEED_CLK_HPLL] = hw;
> +
> +	/*
> +	?* Strap bits 11:10 define the CPU/AHB clock frequency ratio (aka HCLK)
> +	?*???00: Select CPU:AHB = 1:1
> +	?*???01: Select CPU:AHB = 2:1
> +	?*???10: Select CPU:AHB = 4:1
> +	?*???11: Select CPU:AHB = 3:1
> +	?*/
> +	regmap_read(map, ASPEED_STRAP, &val);
> +	val = (val >> 10) & 0x3;

How do the calculations below line up with the comment above? I can't see the
connection and I feel like I've missed something.

> +	div = val + 1;

This is only a valid mapping for 0b00 and 0b01?

> +	if (div == 2)
> +		div = 3;

If (div == 2) after adding 1, then the original value was 0b01, and so div
should remain 2?

> +	else if (div == 3)
> +		div = 2;

If (div == 3) after adding 1, then the original value was 0b10, and so div
should be 4?

> +	hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
> +	aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
> +
> +	/* APB clock clock selection register SCU08 (aka PCLK) */
> +	hw = clk_hw_register_divider_table(NULL, "apb", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 23, 3, 0,
> +			ast2400_div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
> +}
> +
> +static void __init aspeed_ast2500_cc(struct regmap *map)
> +{
> +	struct clk_hw *hw;
> +	u32 val, div;
> +
> +	/*
> +	?* High-speed PLL clock derived from the crystal. This the CPU clock,
> +	?* and we assume that it is enabled
> +	?*/
> +	regmap_read(map, ASPEED_HPLL_PARAM, &val);
> +	aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_calc_pll("hpll", val);

Why did you extract the aspeed_calc_pll() implementation for the ast2500 but
not the ast2400?

> +
> +	/* Strap bits 11:9 define the AXI/AHB clock frequency ratio (aka HCLK)*/
> +	regmap_read(map, ASPEED_STRAP, &val);
> +	val = (val >> 9) & 0x7;
> +	WARN_ON(val == 0);

Maybe WARN() would be better here to explain what's going wrong (zero is an
undefined value)?

> +	div = 2 * (val + 1);
> +	hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
> +	aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
> +
> +	/* APB clock clock selection register SCU08 (aka PCLK) */
> +	/* TODO: this is wrong! */

Why is this wrong?

> +	regmap_read(map, ASPEED_CLK_SELECTION, &val);
> +	val = (val >> 23) & 0x7;
> +	div = 4 * (val + 1);
> +	hw = clk_hw_register_fixed_factor(NULL, "apb", "hpll", 0, 1, div);
> +	aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
> +};
> +
> +
> ?static void __init aspeed_cc_init(struct device_node *np)
> ?{
> ?	struct regmap *map;
> +	unsigned long freq;
> +	struct clk_hw *hw;
> ?	u32 val;
> ?	int ret;
> ?	int i;
> @@ -153,6 +284,21 @@ static void __init aspeed_cc_init(struct device_node *np)
> ?		return;
> ?	}
> ?
> +	/* CLKIN is the crystal oscillator, 24 or 25MHz selected by strapping */
> +	if (val & BIT(23))
> +		freq = 25000000;
> +	else
> +		freq = 24000000;

This isn't true on the AST2400, where CLKIN could be 24 OR 48MHz when BIT(23) is
clear. You need to test BIT(18) as well on the AST2400 to disambiguate.

> +	hw = clk_hw_register_fixed_rate(NULL, "clkin", NULL, 0, freq);
> +	pr_debug("clkin @%lu MHz\n", freq / 1000000);
> +
> +	if (of_device_is_compatible(np, "aspeed,ast2400-scu"))
> +		aspeed_ast2400_cc(map);
> +	else if (of_device_is_compatible(np, "aspeed,ast2500-scu"))
> +		aspeed_ast2500_cc(map);
> +	else
> +		pr_err("unknown platform, failed to add clocks\n");

I'm not confident that "borrowing" the SCU compatible like this is entirely in
the spirit of its MFD nature, but maybe others can chime in.

Cheers,

Andrew

> +
> ?	aspeed_clk_data->num = ASPEED_NUM_CLKS;
> ?	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
> ?	if (ret)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170925/c4f24243/attachment-0001.sig>

Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 6544 bytes --]

On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:
> This registers a platform driver to set up all of the non-core clocks.
> 
> The clocks that have configurable rates are now registered.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/clk/clk-aspeed.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index e614c61b82d2..19531798e040 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -14,6 +14,8 @@
>  #include <linux/clk-provider.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> @@ -115,6 +117,20 @@ static const struct aspeed_gate_data aspeed_gates[] __initconst = {
>  	/* 31: reserved */
>  };
>  
> +static const char * const eclk_parents[] = {"d1pll", "hpll", "mpll"};
> +
> +static const struct clk_div_table ast2500_mac_div_table[] = {
> +	{ 0x0, 4 }, /* Yep, really. Aspeed confirmed this is correct */
> +	{ 0x1, 4 },
> +	{ 0x2, 6 },
> +	{ 0x3, 8 },
> +	{ 0x4, 10 },
> +	{ 0x5, 12 },
> +	{ 0x6, 14 },
> +	{ 0x7, 16 },
> +	{ 0 }
> +};
> +
>  static const struct clk_div_table ast2400_div_table[] = {
>  	{ 0x0, 2 },
>  	{ 0x1, 4 },
> @@ -139,6 +155,21 @@ static const struct clk_div_table ast2500_div_table[] = {
>  	{ 0 }
>  };
>  
> +struct aspeed_clk_soc_data {
> +	const struct clk_div_table *div_table;
> +	const struct clk_div_table *mac_div_table;
> +};
> +
> +static const struct aspeed_clk_soc_data ast2500_data = {
> +	.div_table = ast2500_div_table,
> +	.mac_div_table = ast2500_mac_div_table,
> +};
> +
> +static const struct aspeed_clk_soc_data ast2400_data = {
> +	.div_table = ast2400_div_table,
> +	.mac_div_table = ast2400_div_table,
> +};
> +
>  static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>  {
>  	unsigned int mult, div;
> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>  			mult, div);
>  }
>  
> +static int __init aspeed_clk_probe(struct platform_device *pdev)
> +{
> +	const struct aspeed_clk_soc_data *soc_data;
> +	const struct clk_div_table *mac_div_table;
> +	const struct clk_div_table *div_table;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *map;
> +	struct clk_hw *hw;
> +	u32 val, rate;
> +
> +	map = syscon_node_to_regmap(dev->of_node);
> +	if (IS_ERR(map)) {
> +		dev_err(dev, "no syscon regmap\n");
> +		return PTR_ERR(map);
> +	}
> +
> +	/* SoC generations share common layouts but have different divisors */
> +	soc_data = of_device_get_match_data(&pdev->dev);
> +	div_table = soc_data->div_table;
> +	mac_div_table = soc_data->mac_div_table;
> +
> +	/* UART clock div13 setting */
> +	regmap_read(map, ASPEED_MISC_CTRL, &val);
> +	if (val & BIT(12))
> +		rate = 24000000 / 13;
> +	else
> +		rate = 24000000;
> +	/* TODO: Find the parent data for the uart clock */
> +	hw = clk_hw_register_fixed_rate(NULL, "uart", NULL, 0, rate);
> +	aspeed_clk_data->hws[ASPEED_CLK_UART] = hw;
> +
> +	/*
> +	 * Memory controller (M-PLL) PLL. This clock is configured by the
> +	 * bootloader, and is exposed to Linux as a read-only clock rate.
> +	 */
> +	regmap_read(map, ASPEED_MPLL_PARAM, &val);
> +	aspeed_clk_data->hws[ASPEED_CLK_MPLL] =	aspeed_calc_pll("mpll", val);

IIRC the calculation in aspeed_calc_pll() is appropriate for the AST2500, but
not the AST2400.

> +
> +	/* SD/SDIO clock divider (TODO: There's a gate too) */
> +	hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_SDIO] = hw;
> +
> +	/* MAC AHB bus clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "mac", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 16, 3, 0,
> +			mac_div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_MAC] = hw;
> +
> +	/* LPC Host (LHCLK) clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "lhclk", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_LHCLK] = hw;
> +
> +	/* Video Engine (ECLK) mux and clock divider */
> +	hw = clk_hw_register_mux(NULL, "eclk_mux",
> +			eclk_parents, ARRAY_SIZE(eclk_parents), 0,
> +			scu_base + ASPEED_CLK_SELECTION, 2, 2,
> +			0, &aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
> +	hw = clk_hw_register_divider_table(NULL, "eclk", "eclk_mux", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,

On the AST2500 it looks like this should start at bit 28 for 3 bits, not bit
20. Separately, I'm not sure how to interpret the AST2400 datasheet here -
maybe it's similar but with different wording ("clock slow down" rather than
"divisor"?).

> +			div_table,

This doesn't seem to be correct. There's the problem of 0b000 and 0b001 mapping
the same value of 2 for the AST2500, whose table then increments in steps of 1.
The AST2400 mapping on the otherhand is multiples of 2 starting at 2, with no
inconsistency for 0b000 vs 0b001.

> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
> +
> +	/* P-Bus (BCLK) clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "bclk", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 0, 2, 0,

Bit 0 in SCU08 is a 1-bit field "CPU/AHB clock dynamic slow down enable". BCLK
is actually in SCU*D*8, but (perhaps confusingly) documented immediately below
SCU*0*8.

Cheers,

Andrew

> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
> +
> +	return 0;
> +};
> +
> +static const struct of_device_id aspeed_clk_dt_ids[] = {
> +	{ .compatible = "aspeed,ast2400-scu", .data = &ast2400_data },
> +	{ .compatible = "aspeed,ast2500-scu", .data = &ast2500_data },
> +	{ },
> +};
> +
> +static struct platform_driver aspeed_clk_driver = {
> +	.probe  = aspeed_clk_probe,
> +	.driver = {
> +		.name = "aspeed-clk",
> +		.of_match_table = aspeed_clk_dt_ids,
> +		.suppress_bind_attrs = true,
> +	},
> +};
> +builtin_platform_driver(aspeed_clk_driver);
> +
> +
>  static void __init aspeed_ast2400_cc(struct regmap *map)
>  {
>  	struct clk_hw *hw;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Andrew Jeffery]

On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:
> This registers a platform driver to set up all of the non-core clocks.
>?
> The clocks that have configurable rates are now registered.
>?
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> ?drivers/clk/clk-aspeed.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++
> ?1 file changed, 129 insertions(+)
>?
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index e614c61b82d2..19531798e040 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -14,6 +14,8 @@
> ?#include <linux/clk-provider.h>
> ?#include <linux/mfd/syscon.h>
> ?#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> ?#include <linux/regmap.h>
> ?#include <linux/slab.h>
> ?#include <linux/spinlock.h>
> @@ -115,6 +117,20 @@ static const struct aspeed_gate_data aspeed_gates[] __initconst = {
> ?	/* 31: reserved */
> ?};
> ?
> +static const char * const eclk_parents[] = {"d1pll", "hpll", "mpll"};
> +
> +static const struct clk_div_table ast2500_mac_div_table[] = {
> +	{ 0x0, 4 }, /* Yep, really. Aspeed confirmed this is correct */
> +	{ 0x1, 4 },
> +	{ 0x2, 6 },
> +	{ 0x3, 8 },
> +	{ 0x4, 10 },
> +	{ 0x5, 12 },
> +	{ 0x6, 14 },
> +	{ 0x7, 16 },
> +	{ 0 }
> +};
> +
> ?static const struct clk_div_table ast2400_div_table[] = {
> ?	{ 0x0, 2 },
> ?	{ 0x1, 4 },
> @@ -139,6 +155,21 @@ static const struct clk_div_table ast2500_div_table[] = {
> ?	{ 0 }
> ?};
> ?
> +struct aspeed_clk_soc_data {
> +	const struct clk_div_table *div_table;
> +	const struct clk_div_table *mac_div_table;
> +};
> +
> +static const struct aspeed_clk_soc_data ast2500_data = {
> +	.div_table = ast2500_div_table,
> +	.mac_div_table = ast2500_mac_div_table,
> +};
> +
> +static const struct aspeed_clk_soc_data ast2400_data = {
> +	.div_table = ast2400_div_table,
> +	.mac_div_table = ast2400_div_table,
> +};
> +
> ?static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
> ?{
> ?	unsigned int mult, div;
> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
> ?			mult, div);
> ?}
> ?
> +static int __init aspeed_clk_probe(struct platform_device *pdev)
> +{
> +	const struct aspeed_clk_soc_data *soc_data;
> +	const struct clk_div_table *mac_div_table;
> +	const struct clk_div_table *div_table;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *map;
> +	struct clk_hw *hw;
> +	u32 val, rate;
> +
> +	map = syscon_node_to_regmap(dev->of_node);
> +	if (IS_ERR(map)) {
> +		dev_err(dev, "no syscon regmap\n");
> +		return PTR_ERR(map);
> +	}
> +
> +	/* SoC generations share common layouts but have different divisors */
> +	soc_data = of_device_get_match_data(&pdev->dev);
> +	div_table = soc_data->div_table;
> +	mac_div_table = soc_data->mac_div_table;
> +
> +	/* UART clock div13 setting */
> +	regmap_read(map, ASPEED_MISC_CTRL, &val);
> +	if (val & BIT(12))
> +		rate = 24000000 / 13;
> +	else
> +		rate = 24000000;
> +	/* TODO: Find the parent data for the uart clock */
> +	hw = clk_hw_register_fixed_rate(NULL, "uart", NULL, 0, rate);
> +	aspeed_clk_data->hws[ASPEED_CLK_UART] = hw;
> +
> +	/*
> +	?* Memory controller (M-PLL) PLL. This clock is configured by the
> +	?* bootloader, and is exposed to Linux as a read-only clock rate.
> +	?*/
> +	regmap_read(map, ASPEED_MPLL_PARAM, &val);
> +	aspeed_clk_data->hws[ASPEED_CLK_MPLL] =	aspeed_calc_pll("mpll", val);

IIRC the calculation in aspeed_calc_pll() is appropriate for the AST2500, but
not the AST2400.

> +
> +	/* SD/SDIO clock divider (TODO: There's a gate too) */
> +	hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_SDIO] = hw;
> +
> +	/* MAC AHB bus clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "mac", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 16, 3, 0,
> +			mac_div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_MAC] = hw;
> +
> +	/* LPC Host (LHCLK) clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "lhclk", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_LHCLK] = hw;
> +
> +	/* Video Engine (ECLK) mux and clock divider */
> +	hw = clk_hw_register_mux(NULL, "eclk_mux",
> +			eclk_parents, ARRAY_SIZE(eclk_parents), 0,
> +			scu_base + ASPEED_CLK_SELECTION, 2, 2,
> +			0, &aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
> +	hw = clk_hw_register_divider_table(NULL, "eclk", "eclk_mux", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,

On the AST2500 it looks like this should start at bit 28 for 3 bits, not bit
20. Separately, I'm not sure how to interpret the AST2400 datasheet here -
maybe it's similar but with different wording ("clock slow down" rather than
"divisor"?).

> +			div_table,

This doesn't seem to be correct. There's the problem of 0b000 and 0b001 mapping
the same value of 2 for the AST2500, whose table then increments in steps of 1.
The AST2400 mapping on the otherhand is multiples of 2 starting at 2, with no
inconsistency for 0b000 vs 0b001.

> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
> +
> +	/* P-Bus (BCLK) clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "bclk", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 0, 2, 0,

Bit 0 in SCU08 is a 1-bit field "CPU/AHB clock dynamic slow down enable". BCLK
is actually in SCU*D*8, but (perhaps confusingly) documented immediately below
SCU*0*8.

Cheers,

Andrew

> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
> +
> +	return 0;
> +};
> +
> +static const struct of_device_id aspeed_clk_dt_ids[] = {
> +	{ .compatible = "aspeed,ast2400-scu", .data = &ast2400_data },
> +	{ .compatible = "aspeed,ast2500-scu", .data = &ast2500_data },
> +	{ },
> +};
> +
> +static struct platform_driver aspeed_clk_driver = {
> +	.probe??= aspeed_clk_probe,
> +	.driver = {
> +		.name = "aspeed-clk",
> +		.of_match_table = aspeed_clk_dt_ids,
> +		.suppress_bind_attrs = true,
> +	},
> +};
> +builtin_platform_driver(aspeed_clk_driver);
> +
> +
> ?static void __init aspeed_ast2400_cc(struct regmap *map)
> ?{
> ?	struct clk_hw *hw;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170925/dc5ae955/attachment.sig>

Re: [PATCH v2 5/5] clk: aspeed: Add reset controller

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4916 bytes --]

On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:
> There are some resets that are not associated with gates. These are
> represented by a reset controller.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/clk/clk-aspeed.c                 | 82 +++++++++++++++++++++++++++++++-
>  include/dt-bindings/clock/aspeed-clock.h |  9 ++++
>  2 files changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index dec9db4ec47b..db97c0f9f99e 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -17,6 +17,7 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <linux/reset-controller.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  
> @@ -256,6 +257,68 @@ static const struct clk_ops aspeed_clk_gate_ops = {
>  	.is_enabled = aspeed_clk_is_enabled,
>  };
>  
> +/**
> + * struct aspeed_reset - Aspeed reset controller
> + * @map: regmap to access the containing system controller
> + * @rcdev: reset controller device
> + */
> +struct aspeed_reset {
> +	struct regmap			*map;
> +	struct reset_controller_dev	rcdev;
> +};
> +
> +#define to_aspeed_reset(p) container_of((p), struct aspeed_reset, rcdev)
> +
> +static const u8 aspeed_resets[] = {
> +	25, /* x-dma */
> +	24, /* mctp */
> +	23, /* adc */
> +	22, /* jtag-master */
> +	18, /* mic */
> +	 9, /* pwm */
> +	 8, /* pci-vga */
> +	 2, /* i2c */
> +	 1, /* ahb */

Bit of a nit, but given you define macros for the indices, maybe use designated
initialisers and drop the comments.

> +};
> +
> +static int aspeed_reset_deassert(struct reset_controller_dev *rcdev,
> +				 unsigned long id)
> +{
> +	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> +	u32 rst = BIT(aspeed_resets[id]);
> +
> +	return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, 0);
> +}
> +
> +static int aspeed_reset_assert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> +	u32 rst = BIT(aspeed_resets[id]);
> +
> +	return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, rst);
> +}
> +
> +static int aspeed_reset_status(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> +	u32 val, rst = BIT(aspeed_resets[id]);
> +	int ret;
> +
> +	ret = regmap_read(ar->map, ASPEED_RESET_CTRL, &val);
> +	if (ret)
> +		return ret;
> +
> +	return !!(val & rst);
> +}
> +
> +static const struct reset_control_ops aspeed_reset_ops = {
> +	.assert = aspeed_reset_assert,
> +	.deassert = aspeed_reset_deassert,
> +	.status = aspeed_reset_status,
> +};
> +
>  static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
>  		const char *name, const char *parent_name, unsigned long flags,
>  		struct regmap *map, u8 clock_idx, u8 reset_idx,
> @@ -299,10 +362,11 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
>  	const struct clk_div_table *mac_div_table;
>  	const struct clk_div_table *div_table;
>  	struct device *dev = &pdev->dev;
> +	struct aspeed_reset *ar;
>  	struct regmap *map;
>  	struct clk_hw *hw;
>  	u32 val, rate;
> -	int i;
> +	int i, ret;
>  
>  	map = syscon_node_to_regmap(dev->of_node);
>  	if (IS_ERR(map)) {
> @@ -310,6 +374,22 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
>  		return PTR_ERR(map);
>  	}
>  
> +	ar = devm_kzalloc(dev, sizeof(*ar), GFP_KERNEL);
> +	if (!ar)
> +		return -ENOMEM;
> +
> +	ar->map = map;
> +	ar->rcdev.owner = THIS_MODULE;
> +	ar->rcdev.nr_resets = ARRAY_SIZE(aspeed_resets);
> +	ar->rcdev.ops = &aspeed_reset_ops;
> +	ar->rcdev.of_node = dev->of_node;
> +
> +	ret = devm_reset_controller_register(dev, &ar->rcdev);
> +	if (ret) {
> +		dev_err(dev, "could not register reset controller\n");
> +		return ret;
> +	}
> +
>  	/* SoC generations share common layouts but have different divisors */
>  	soc_data = of_device_get_match_data(&pdev->dev);
>  	div_table = soc_data->div_table;
> diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
> index afe06b77562d..a9d552b6bbd2 100644
> --- a/include/dt-bindings/clock/aspeed-clock.h
> +++ b/include/dt-bindings/clock/aspeed-clock.h
> @@ -40,4 +40,13 @@
>  #define ASPEED_CLK_GATE_SDCLKCLK	(22 + ASPEED_CLK_GATES)
>  #define ASPEED_CLK_GATE_LHCCLK		(23 + ASPEED_CLK_GATES)
>  
> +#define ASPEED_RESET_XDMA		0
> +#define ASPEED_RESET_MCTP		1
> +#define ASPEED_RESET_JTAG_MASTER	2
> +#define ASPEED_RESET_MIC		3
> +#define ASPEED_RESET_PWM		4
> +#define ASPEED_RESET_PCIVGA		5
> +#define ASPEED_RESET_I2C		6
> +#define ASPEED_RESET_AHB		7
> +
>  #endif

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v2 5/5] clk: aspeed: Add reset controller

[Andrew Jeffery]

On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:
> There are some resets that are not associated with gates. These are
> represented by a reset controller.
>?
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> ?drivers/clk/clk-aspeed.c?????????????????| 82 +++++++++++++++++++++++++++++++-
> ?include/dt-bindings/clock/aspeed-clock.h |??9 ++++
> ?2 files changed, 90 insertions(+), 1 deletion(-)
>?
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index dec9db4ec47b..db97c0f9f99e 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -17,6 +17,7 @@
> ?#include <linux/of_device.h>
> ?#include <linux/platform_device.h>
> ?#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> ?#include <linux/slab.h>
> ?#include <linux/spinlock.h>
> ?
> @@ -256,6 +257,68 @@ static const struct clk_ops aspeed_clk_gate_ops = {
> ?	.is_enabled = aspeed_clk_is_enabled,
> ?};
> ?
> +/**
> + * struct aspeed_reset - Aspeed reset controller
> + * @map: regmap to access the containing system controller
> + * @rcdev: reset controller device
> + */
> +struct aspeed_reset {
> +	struct regmap			*map;
> +	struct reset_controller_dev	rcdev;
> +};
> +
> +#define to_aspeed_reset(p) container_of((p), struct aspeed_reset, rcdev)
> +
> +static const u8 aspeed_resets[] = {
> +	25, /* x-dma */
> +	24, /* mctp */
> +	23, /* adc */
> +	22, /* jtag-master */
> +	18, /* mic */
> +	?9, /* pwm */
> +	?8, /* pci-vga */
> +	?2, /* i2c */
> +	?1, /* ahb */

Bit of a nit, but given you define macros for the indices, maybe use designated
initialisers and drop the comments.

> +};
> +
> +static int aspeed_reset_deassert(struct reset_controller_dev *rcdev,
> +				?unsigned long id)
> +{
> +	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> +	u32 rst = BIT(aspeed_resets[id]);
> +
> +	return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, 0);
> +}
> +
> +static int aspeed_reset_assert(struct reset_controller_dev *rcdev,
> +			???????unsigned long id)
> +{
> +	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> +	u32 rst = BIT(aspeed_resets[id]);
> +
> +	return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, rst);
> +}
> +
> +static int aspeed_reset_status(struct reset_controller_dev *rcdev,
> +			???????unsigned long id)
> +{
> +	struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> +	u32 val, rst = BIT(aspeed_resets[id]);
> +	int ret;
> +
> +	ret = regmap_read(ar->map, ASPEED_RESET_CTRL, &val);
> +	if (ret)
> +		return ret;
> +
> +	return !!(val & rst);
> +}
> +
> +static const struct reset_control_ops aspeed_reset_ops = {
> +	.assert = aspeed_reset_assert,
> +	.deassert = aspeed_reset_deassert,
> +	.status = aspeed_reset_status,
> +};
> +
> ?static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
> ?		const char *name, const char *parent_name, unsigned long flags,
> ?		struct regmap *map, u8 clock_idx, u8 reset_idx,
> @@ -299,10 +362,11 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
> ?	const struct clk_div_table *mac_div_table;
> ?	const struct clk_div_table *div_table;
> ?	struct device *dev = &pdev->dev;
> +	struct aspeed_reset *ar;
> ?	struct regmap *map;
> ?	struct clk_hw *hw;
> ?	u32 val, rate;
> -	int i;
> +	int i, ret;
> ?
> ?	map = syscon_node_to_regmap(dev->of_node);
> ?	if (IS_ERR(map)) {
> @@ -310,6 +374,22 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
> ?		return PTR_ERR(map);
> ?	}
> ?
> +	ar = devm_kzalloc(dev, sizeof(*ar), GFP_KERNEL);
> +	if (!ar)
> +		return -ENOMEM;
> +
> +	ar->map = map;
> +	ar->rcdev.owner = THIS_MODULE;
> +	ar->rcdev.nr_resets = ARRAY_SIZE(aspeed_resets);
> +	ar->rcdev.ops = &aspeed_reset_ops;
> +	ar->rcdev.of_node = dev->of_node;
> +
> +	ret = devm_reset_controller_register(dev, &ar->rcdev);
> +	if (ret) {
> +		dev_err(dev, "could not register reset controller\n");
> +		return ret;
> +	}
> +
> ?	/* SoC generations share common layouts but have different divisors */
> ?	soc_data = of_device_get_match_data(&pdev->dev);
> ?	div_table = soc_data->div_table;
> diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
> index afe06b77562d..a9d552b6bbd2 100644
> --- a/include/dt-bindings/clock/aspeed-clock.h
> +++ b/include/dt-bindings/clock/aspeed-clock.h
> @@ -40,4 +40,13 @@
> ?#define ASPEED_CLK_GATE_SDCLKCLK	(22 + ASPEED_CLK_GATES)
> ?#define ASPEED_CLK_GATE_LHCCLK		(23 + ASPEED_CLK_GATES)
> ?
> +#define ASPEED_RESET_XDMA		0
> +#define ASPEED_RESET_MCTP		1
> +#define ASPEED_RESET_JTAG_MASTER	2
> +#define ASPEED_RESET_MIC		3
> +#define ASPEED_RESET_PWM		4
> +#define ASPEED_RESET_PCIVGA		5
> +#define ASPEED_RESET_I2C		6
> +#define ASPEED_RESET_AHB		7
> +
> ?#endif
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170925/645f6bb8/attachment.sig>

Re: [PATCH v2 1/5] clk: Add clock driver for ASPEED BMC SoCs

[Joel Stanley]

On Mon, Sep 25, 2017 at 1:40 PM, Andrew Jeffery <andrew@aj.id.au> wrote:

>> +struct aspeed_clk_gate {
>> +     struct clk_hw   hw;
>> +     struct regmap   *map;
>> +     u8              clock_idx;
>> +     s8              reset_idx;
>> +     u8              flags;
>> +     spinlock_t      *lock;
>> +};
>
> It feels like the two structures could be unified, but the result turns into a
> bit of a mess with a union of structs to limit the space impact, so perhaps we
> shouldn't go there?

I agree; it's not going to make it any easier to read so lets leave it as is.

>
>> +
>> +#define to_aspeed_clk_gate(_hw) container_of(_hw, struct aspeed_clk_gate, hw)
>> +
>> +/* TODO: ask Aspeed about the actual parent data */
>> +static const struct aspeed_gate_data aspeed_gates[] __initconst = {
>> +/*    clk rst   name                 parent          flags   */
>> +     {  0, -1, "eclk-gate",          "eclk",         0 }, /* Video Engine */
>> +     {  1,  7, "gclk-gate",          NULL,           0 }, /* 2D engine */
>> +     {  2, -1, "mclk-gate",          "mpll",         CLK_IS_CRITICAL }, /* SDRAM */
>> +     {  3,  6, "vclk-gate",          NULL,           0 }, /* Video Capture */
>> +     {  4, 10, "bclk-gate",          "bclk",         0 }, /* PCIe/PCI */
>> +     {  5, -1, "dclk-gate",          NULL,           0 }, /* DAC */
>> +     {  6, -1, "refclk-gate",        "clkin",        CLK_IS_CRITICAL },
>> +     {  7,  3, "usb-port2-gate",     NULL,           0 }, /* USB2.0 Host port 2 */
>> +     {  8,  5, "lclk-gate",          NULL,           0 }, /* LPC */
>> +     {  9, 15, "usb-uhci-gate",      NULL,           0 }, /* USB1.1 (requires port 2 enabled) */
>> +     { 10, 13, "d1clk-gate",         NULL,           0 }, /* GFX CRT */
>
> Is there a typo in the AST2400 datasheet? It claims bit 10 is "D2CLK".

The ast2500 says d1clk, and the ast2400 says d2clk. Both call it the
GFX CRT clock. Normally changes between the two are in a different
colour, but we don't have that here :(

I'll ask Ryan if he knows what's going on.

>
>> +     /* 11: reserved */
>> +     /* 12: reserved */
>> +     { 13, 4,  "yclk-gate",          NULL,           0 }, /* HAC */
>> +     { 14, 14, "usb-port1-gate",     NULL,           0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
>> +     { 15, -1, "uart1clk-gate",      "uart",         0 }, /* UART1 */
>> +     { 16, -1, "uart2clk-gate",      "uart",         0 }, /* UART2 */
>> +     { 17, -1, "uart5clk-gate",      "uart",         0 }, /* UART5 */
>> +     /* 18: reserved */
>> +     { 19, -1, "espiclk-gate",       NULL,           0 }, /* eSPI */
>
> 19 is reserved on the AST2400, and must be kept at '1'.

Yeah. This is another difference between the SoCs that isn't called
out by the "this has changed" colouring of the datasheet.

I'd suggest it's not worth adding the code to make the driver return
an error when requesting this one. If anyone feels strongly about it
we could skip the registration of this one at probe time on the
ast2400.

>
>> +     { 20, 11, "mac1clk-gate",       "clkin",        0 }, /* MAC1 */
>> +     { 21, 12, "mac2clk-gate",       "clkin",        0 }, /* MAC2 */
>> +     /* 22: reserved */
>> +     /* 23: reserved */
>> +     { 24, -1, "rsaclk-gate",        NULL,           0 }, /* RSA */
>> +     { 25, -1, "uart3clk-gate",      "uart",         0 }, /* UART3 */
>> +     { 26, -1, "uart4clk-gate",      "uart",         0 }, /* UART4 */
>> +     { 27, 16, "sdclk-gate",         NULL,           0 }, /* SDIO/SD */
>> +     { 28, -1, "lhclk-gate",         "lhclk",        0 }, /* LPC master/LPC+ */
>> +     /* 29: reserved */
>> +     /* 30: reserved */
>> +     /* 31: reserved */
>
> Interestingly bits 29-30 are reserved in both the AST2400 and AST2500
> datasheets, but are reserved-clear in the AST2400 datasheet and reserved-set in
> the AST2500 datasheet. This may affect how we write register.

The driver doesn't change the value, so I think we're safe.

> Separately, at one point I mistook the clk column for the index the entry
> should appear at, which isn't the case. Do you think designated intializers
> might help?

Will make the table a bit awkward, but it's a good suggestion. I'll
give it a go for v3.

>> +     /*
>> +      * This way all clock fetched before the platform device probes,
>
> Typo: "clocks"

Nice catch. It's a typo in the Gemini driver as well ;)

>
>> +      * except those we assign here for early use, will be deferred.
>> +      */
>> +     for (i = 0; i < ASPEED_NUM_CLKS; i++)
>> +             aspeed_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);

>> +++ b/include/dt-bindings/clock/aspeed-clock.h
>> @@ -0,0 +1,43 @@
>> +#ifndef DT_BINDINGS_ASPEED_CLOCK_H
>> +#define DT_BINDINGS_ASPEED_CLOCK_H
>> +
>> +#define ASPEED_NUM_CLKS      34
>
> This is a bit of a nit pick: Do the constants below represent all the
> clocks in the SoC(s)? Maybe if not it's better to define
> ASPEED_NUM_CLKS in terms of (n + ASPEED_CLK_GATES) at the bottom - that
> way when a clock or gate is added ASPEED_NUM_CLKS has better locality and
> better visual progression in context.

Okay, I put it at the bottom.

>> +#define ASPEED_CLK_GATE_LHCCLK               (23 + ASPEED_CLK_GATES)
>
> Looks like there's an off-by-one error here: ASPEED_NUM_CLKS should be (23 +
> ASPEED_CLK_GATES + 1) = 35.

Thanks, fixed.

[PATCH v2 1/5] clk: Add clock driver for ASPEED BMC SoCs

[Joel Stanley]

On Mon, Sep 25, 2017 at 1:40 PM, Andrew Jeffery <andrew@aj.id.au> wrote:

>> +struct aspeed_clk_gate {
>> +     struct clk_hw   hw;
>> +     struct regmap   *map;
>> +     u8              clock_idx;
>> +     s8              reset_idx;
>> +     u8              flags;
>> +     spinlock_t      *lock;
>> +};
>
> It feels like the two structures could be unified, but the result turns into a
> bit of a mess with a union of structs to limit the space impact, so perhaps we
> shouldn't go there?

I agree; it's not going to make it any easier to read so lets leave it as is.

>
>> +
>> +#define to_aspeed_clk_gate(_hw) container_of(_hw, struct aspeed_clk_gate, hw)
>> +
>> +/* TODO: ask Aspeed about the actual parent data */
>> +static const struct aspeed_gate_data aspeed_gates[] __initconst = {
>> +/*    clk rst   name                 parent          flags   */
>> +     {  0, -1, "eclk-gate",          "eclk",         0 }, /* Video Engine */
>> +     {  1,  7, "gclk-gate",          NULL,           0 }, /* 2D engine */
>> +     {  2, -1, "mclk-gate",          "mpll",         CLK_IS_CRITICAL }, /* SDRAM */
>> +     {  3,  6, "vclk-gate",          NULL,           0 }, /* Video Capture */
>> +     {  4, 10, "bclk-gate",          "bclk",         0 }, /* PCIe/PCI */
>> +     {  5, -1, "dclk-gate",          NULL,           0 }, /* DAC */
>> +     {  6, -1, "refclk-gate",        "clkin",        CLK_IS_CRITICAL },
>> +     {  7,  3, "usb-port2-gate",     NULL,           0 }, /* USB2.0 Host port 2 */
>> +     {  8,  5, "lclk-gate",          NULL,           0 }, /* LPC */
>> +     {  9, 15, "usb-uhci-gate",      NULL,           0 }, /* USB1.1 (requires port 2 enabled) */
>> +     { 10, 13, "d1clk-gate",         NULL,           0 }, /* GFX CRT */
>
> Is there a typo in the AST2400 datasheet? It claims bit 10 is "D2CLK".

The ast2500 says d1clk, and the ast2400 says d2clk. Both call it the
GFX CRT clock. Normally changes between the two are in a different
colour, but we don't have that here :(

I'll ask Ryan if he knows what's going on.

>
>> +     /* 11: reserved */
>> +     /* 12: reserved */
>> +     { 13, 4,  "yclk-gate",          NULL,           0 }, /* HAC */
>> +     { 14, 14, "usb-port1-gate",     NULL,           0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
>> +     { 15, -1, "uart1clk-gate",      "uart",         0 }, /* UART1 */
>> +     { 16, -1, "uart2clk-gate",      "uart",         0 }, /* UART2 */
>> +     { 17, -1, "uart5clk-gate",      "uart",         0 }, /* UART5 */
>> +     /* 18: reserved */
>> +     { 19, -1, "espiclk-gate",       NULL,           0 }, /* eSPI */
>
> 19 is reserved on the AST2400, and must be kept at '1'.

Yeah. This is another difference between the SoCs that isn't called
out by the "this has changed" colouring of the datasheet.

I'd suggest it's not worth adding the code to make the driver return
an error when requesting this one. If anyone feels strongly about it
we could skip the registration of this one at probe time on the
ast2400.

>
>> +     { 20, 11, "mac1clk-gate",       "clkin",        0 }, /* MAC1 */
>> +     { 21, 12, "mac2clk-gate",       "clkin",        0 }, /* MAC2 */
>> +     /* 22: reserved */
>> +     /* 23: reserved */
>> +     { 24, -1, "rsaclk-gate",        NULL,           0 }, /* RSA */
>> +     { 25, -1, "uart3clk-gate",      "uart",         0 }, /* UART3 */
>> +     { 26, -1, "uart4clk-gate",      "uart",         0 }, /* UART4 */
>> +     { 27, 16, "sdclk-gate",         NULL,           0 }, /* SDIO/SD */
>> +     { 28, -1, "lhclk-gate",         "lhclk",        0 }, /* LPC master/LPC+ */
>> +     /* 29: reserved */
>> +     /* 30: reserved */
>> +     /* 31: reserved */
>
> Interestingly bits 29-30 are reserved in both the AST2400 and AST2500
> datasheets, but are reserved-clear in the AST2400 datasheet and reserved-set in
> the AST2500 datasheet. This may affect how we write register.

The driver doesn't change the value, so I think we're safe.

> Separately, at one point I mistook the clk column for the index the entry
> should appear at, which isn't the case. Do you think designated intializers
> might help?

Will make the table a bit awkward, but it's a good suggestion. I'll
give it a go for v3.

>> +     /*
>> +      * This way all clock fetched before the platform device probes,
>
> Typo: "clocks"

Nice catch. It's a typo in the Gemini driver as well ;)

>
>> +      * except those we assign here for early use, will be deferred.
>> +      */
>> +     for (i = 0; i < ASPEED_NUM_CLKS; i++)
>> +             aspeed_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);

>> +++ b/include/dt-bindings/clock/aspeed-clock.h
>> @@ -0,0 +1,43 @@
>> +#ifndef DT_BINDINGS_ASPEED_CLOCK_H
>> +#define DT_BINDINGS_ASPEED_CLOCK_H
>> +
>> +#define ASPEED_NUM_CLKS      34
>
> This is a bit of a nit pick: Do the constants below represent all the
> clocks in the SoC(s)? Maybe if not it's better to define
> ASPEED_NUM_CLKS in terms of (n + ASPEED_CLK_GATES) at the bottom - that
> way when a clock or gate is added ASPEED_NUM_CLKS has better locality and
> better visual progression in context.

Okay, I put it at the bottom.

>> +#define ASPEED_CLK_GATE_LHCCLK               (23 + ASPEED_CLK_GATES)
>
> Looks like there's an off-by-one error here: ASPEED_NUM_CLKS should be (23 +
> ASPEED_CLK_GATES + 1) = 35.

Thanks, fixed.

Re: [PATCH v2 2/5] clk: aspeed: Register core clocks

[Joel Stanley]

On Mon, Sep 25, 2017 at 10:02 PM, Andrew Jeffery <andrew@aj.id.au> wrote:

>> +static const struct clk_div_table ast2400_div_table[] = {
>> +     { 0x0, 2 },
>> +     { 0x1, 4 },
>> +     { 0x2, 6 },
>> +     { 0x3, 8 },
>> +     { 0x4, 10 },
>> +     { 0x5, 12 },
>> +     { 0x6, 14 },
>> +     { 0x7, 16 },
>> +     { 0 }
>> +};
>> +
>> +static const struct clk_div_table ast2500_div_table[] = {
>> +     { 0x0, 4 },
>> +     { 0x1, 8 },
>> +     { 0x2, 12 },
>> +     { 0x3, 16 },
>> +     { 0x4, 20 },
>> +     { 0x5, 24 },
>> +     { 0x6, 28 },
>> +     { 0x7, 32 },
>> +     { 0 }
>> +};
>
> ast2500_div_table is unused?

In this patch, yeah. It gets used in the next one. I defined it next
to the ast2400_div_table so that it is clear why we need the separate
ones.

>> +static void __init aspeed_ast2400_cc(struct regmap *map)
>> +{
>> +     struct clk_hw *hw;
>> +     u32 val, div, mult;
>> +
>> +     /*
>> +      * High-speed PLL clock derived from the crystal. This the CPU clock,
>> +      * and we assume that it is enabled
>> +      */
>> +     regmap_read(map, ASPEED_HPLL_PARAM, &val);
>> +     WARN(val & BIT(18), "clock is strapped not configured");
>
> I don't quite understand the intent of the warning - are we just trying to say
> that the defaults have been assumed and they may not match reality? It's
> probably not a great idea to not strap the board properly, but you could get
> away with it for a given configuration (Numerator = 20, H-PLL Output Divider
> set, H-PLL denominator = 1)

The vlaue of the PLL can be configured in two different ways; one is
via the strapping registers and the other is configured via the
ASPEED_HPLL_PARAM.

I added this warning as the driver does not support reading the value
of hpll from the strapping.

>
>> +     if (val & BIT(17)) {
>> +             /* Pass through mode */
>> +             mult = div = 1;
>> +     } else {
>> +             /* F = 24Mhz * (2-OD) * [(N + 2) / (D + 1)] */
>> +             u32 n = (val >> 5) & 0x3f;
>> +             u32 od = (val >> 4) & 0x1;
>> +             u32 d = val & 0xf;
>> +
>> +             mult = (2 - od) * (n + 2);
>> +             div = d + 1;
>> +     }
>> +     hw = clk_hw_register_fixed_factor(NULL, "hpll", "clkin", 0, mult, div);
>> +     aspeed_clk_data->hws[ASPEED_CLK_HPLL] = hw;
>> +
>> +     /*
>> +      * Strap bits 11:10 define the CPU/AHB clock frequency ratio (aka HCLK)
>> +      *   00: Select CPU:AHB = 1:1
>> +      *   01: Select CPU:AHB = 2:1
>> +      *   10: Select CPU:AHB = 4:1
>> +      *   11: Select CPU:AHB = 3:1
>> +      */
>> +     regmap_read(map, ASPEED_STRAP, &val);
>> +     val = (val >> 10) & 0x3;
>
> How do the calculations below line up with the comment above? I can't see the
> connection and I feel like I've missed something.
>

The code should read like this:

if (div == 3)
  div = 4;
else if (div == 3)
  div = 4;

Fixed in v3.

>
>> +     hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
>> +     aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
>> +
>> +     /* APB clock clock selection register SCU08 (aka PCLK) */
>> +     hw = clk_hw_register_divider_table(NULL, "apb", "hpll", 0,
>> +                     scu_base + ASPEED_CLK_SELECTION, 23, 3, 0,
>> +                     ast2400_div_table,
>> +                     &aspeed_clk_lock);
>> +     aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
>> +}
>> +
>> +static void __init aspeed_ast2500_cc(struct regmap *map)
>> +{
>> +     struct clk_hw *hw;
>> +     u32 val, div;
>> +
>> +     /*
>> +      * High-speed PLL clock derived from the crystal. This the CPU clock,
>> +      * and we assume that it is enabled
>> +      */
>> +     regmap_read(map, ASPEED_HPLL_PARAM, &val);
>> +     aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_calc_pll("hpll", val);
>
> Why did you extract the aspeed_calc_pll() implementation for the ast2500 but
> not the ast2400?

It was used more than once for the ast2500, but only once for the
ast2400. That's changed it v3 so I broke it out again.

>> +
>> +     /* Strap bits 11:9 define the AXI/AHB clock frequency ratio (aka HCLK)*/
>> +     regmap_read(map, ASPEED_STRAP, &val);
>> +     val = (val >> 9) & 0x7;
>> +     WARN_ON(val == 0);
>
> Maybe WARN() would be better here to explain what's going wrong (zero is an
> undefined value)?

Done.

>
>> +     div = 2 * (val + 1);
>> +     hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
>> +     aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
>> +
>> +     /* APB clock clock selection register SCU08 (aka PCLK) */
>> +     /* TODO: this is wrong! */
>
> Why is this wrong?

I think I must have been looking at the ast2400 data sheet and gotten
confused. It looks good to me. I removed the comment in v3.

>
>> +     regmap_read(map, ASPEED_CLK_SELECTION, &val);
>> +     val = (val >> 23) & 0x7;
>> +     div = 4 * (val + 1);
>> +     hw = clk_hw_register_fixed_factor(NULL, "apb", "hpll", 0, 1, div);
>> +     aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
>> +};
>> +
>> +
>>  static void __init aspeed_cc_init(struct device_node *np)
>>  {
>>       struct regmap *map;
>> +     unsigned long freq;
>> +     struct clk_hw *hw;
>>       u32 val;
>>       int ret;
>>       int i;
>> @@ -153,6 +284,21 @@ static void __init aspeed_cc_init(struct device_node *np)
>>               return;
>>       }
>>
>> +     /* CLKIN is the crystal oscillator, 24 or 25MHz selected by strapping */
>> +     if (val & BIT(23))
>> +             freq = 25000000;
>> +     else
>> +             freq = 24000000;
>
> This isn't true on the AST2400, where CLKIN could be 24 OR 48MHz when BIT(23) is
> clear. You need to test BIT(18) as well on the AST2400 to disambiguate.

Fixed. I moved the clkin calcs into the soc specific functions.

>
>> +     hw = clk_hw_register_fixed_rate(NULL, "clkin", NULL, 0, freq);
>> +     pr_debug("clkin @%lu MHz\n", freq / 1000000);
>> +
>> +     if (of_device_is_compatible(np, "aspeed,ast2400-scu"))
>> +             aspeed_ast2400_cc(map);
>> +     else if (of_device_is_compatible(np, "aspeed,ast2500-scu"))
>> +             aspeed_ast2500_cc(map);
>> +     else
>> +             pr_err("unknown platform, failed to add clocks\n");
>
> I'm not confident that "borrowing" the SCU compatible like this is entirely in
> the spirit of its MFD nature, but maybe others can chime in.

[PATCH v2 2/5] clk: aspeed: Register core clocks

[Joel Stanley]

On Mon, Sep 25, 2017 at 10:02 PM, Andrew Jeffery <andrew@aj.id.au> wrote:

>> +static const struct clk_div_table ast2400_div_table[] = {
>> +     { 0x0, 2 },
>> +     { 0x1, 4 },
>> +     { 0x2, 6 },
>> +     { 0x3, 8 },
>> +     { 0x4, 10 },
>> +     { 0x5, 12 },
>> +     { 0x6, 14 },
>> +     { 0x7, 16 },
>> +     { 0 }
>> +};
>> +
>> +static const struct clk_div_table ast2500_div_table[] = {
>> +     { 0x0, 4 },
>> +     { 0x1, 8 },
>> +     { 0x2, 12 },
>> +     { 0x3, 16 },
>> +     { 0x4, 20 },
>> +     { 0x5, 24 },
>> +     { 0x6, 28 },
>> +     { 0x7, 32 },
>> +     { 0 }
>> +};
>
> ast2500_div_table is unused?

In this patch, yeah. It gets used in the next one. I defined it next
to the ast2400_div_table so that it is clear why we need the separate
ones.

>> +static void __init aspeed_ast2400_cc(struct regmap *map)
>> +{
>> +     struct clk_hw *hw;
>> +     u32 val, div, mult;
>> +
>> +     /*
>> +      * High-speed PLL clock derived from the crystal. This the CPU clock,
>> +      * and we assume that it is enabled
>> +      */
>> +     regmap_read(map, ASPEED_HPLL_PARAM, &val);
>> +     WARN(val & BIT(18), "clock is strapped not configured");
>
> I don't quite understand the intent of the warning - are we just trying to say
> that the defaults have been assumed and they may not match reality? It's
> probably not a great idea to not strap the board properly, but you could get
> away with it for a given configuration (Numerator = 20, H-PLL Output Divider
> set, H-PLL denominator = 1)

The vlaue of the PLL can be configured in two different ways; one is
via the strapping registers and the other is configured via the
ASPEED_HPLL_PARAM.

I added this warning as the driver does not support reading the value
of hpll from the strapping.

>
>> +     if (val & BIT(17)) {
>> +             /* Pass through mode */
>> +             mult = div = 1;
>> +     } else {
>> +             /* F = 24Mhz * (2-OD) * [(N + 2) / (D + 1)] */
>> +             u32 n = (val >> 5) & 0x3f;
>> +             u32 od = (val >> 4) & 0x1;
>> +             u32 d = val & 0xf;
>> +
>> +             mult = (2 - od) * (n + 2);
>> +             div = d + 1;
>> +     }
>> +     hw = clk_hw_register_fixed_factor(NULL, "hpll", "clkin", 0, mult, div);
>> +     aspeed_clk_data->hws[ASPEED_CLK_HPLL] = hw;
>> +
>> +     /*
>> +      * Strap bits 11:10 define the CPU/AHB clock frequency ratio (aka HCLK)
>> +      *   00: Select CPU:AHB = 1:1
>> +      *   01: Select CPU:AHB = 2:1
>> +      *   10: Select CPU:AHB = 4:1
>> +      *   11: Select CPU:AHB = 3:1
>> +      */
>> +     regmap_read(map, ASPEED_STRAP, &val);
>> +     val = (val >> 10) & 0x3;
>
> How do the calculations below line up with the comment above? I can't see the
> connection and I feel like I've missed something.
>

The code should read like this:

if (div == 3)
  div = 4;
else if (div == 3)
  div = 4;

Fixed in v3.

>
>> +     hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
>> +     aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
>> +
>> +     /* APB clock clock selection register SCU08 (aka PCLK) */
>> +     hw = clk_hw_register_divider_table(NULL, "apb", "hpll", 0,
>> +                     scu_base + ASPEED_CLK_SELECTION, 23, 3, 0,
>> +                     ast2400_div_table,
>> +                     &aspeed_clk_lock);
>> +     aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
>> +}
>> +
>> +static void __init aspeed_ast2500_cc(struct regmap *map)
>> +{
>> +     struct clk_hw *hw;
>> +     u32 val, div;
>> +
>> +     /*
>> +      * High-speed PLL clock derived from the crystal. This the CPU clock,
>> +      * and we assume that it is enabled
>> +      */
>> +     regmap_read(map, ASPEED_HPLL_PARAM, &val);
>> +     aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_calc_pll("hpll", val);
>
> Why did you extract the aspeed_calc_pll() implementation for the ast2500 but
> not the ast2400?

It was used more than once for the ast2500, but only once for the
ast2400. That's changed it v3 so I broke it out again.

>> +
>> +     /* Strap bits 11:9 define the AXI/AHB clock frequency ratio (aka HCLK)*/
>> +     regmap_read(map, ASPEED_STRAP, &val);
>> +     val = (val >> 9) & 0x7;
>> +     WARN_ON(val == 0);
>
> Maybe WARN() would be better here to explain what's going wrong (zero is an
> undefined value)?

Done.

>
>> +     div = 2 * (val + 1);
>> +     hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
>> +     aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
>> +
>> +     /* APB clock clock selection register SCU08 (aka PCLK) */
>> +     /* TODO: this is wrong! */
>
> Why is this wrong?

I think I must have been looking at the ast2400 data sheet and gotten
confused. It looks good to me. I removed the comment in v3.

>
>> +     regmap_read(map, ASPEED_CLK_SELECTION, &val);
>> +     val = (val >> 23) & 0x7;
>> +     div = 4 * (val + 1);
>> +     hw = clk_hw_register_fixed_factor(NULL, "apb", "hpll", 0, 1, div);
>> +     aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
>> +};
>> +
>> +
>>  static void __init aspeed_cc_init(struct device_node *np)
>>  {
>>       struct regmap *map;
>> +     unsigned long freq;
>> +     struct clk_hw *hw;
>>       u32 val;
>>       int ret;
>>       int i;
>> @@ -153,6 +284,21 @@ static void __init aspeed_cc_init(struct device_node *np)
>>               return;
>>       }
>>
>> +     /* CLKIN is the crystal oscillator, 24 or 25MHz selected by strapping */
>> +     if (val & BIT(23))
>> +             freq = 25000000;
>> +     else
>> +             freq = 24000000;
>
> This isn't true on the AST2400, where CLKIN could be 24 OR 48MHz when BIT(23) is
> clear. You need to test BIT(18) as well on the AST2400 to disambiguate.

Fixed. I moved the clkin calcs into the soc specific functions.

>
>> +     hw = clk_hw_register_fixed_rate(NULL, "clkin", NULL, 0, freq);
>> +     pr_debug("clkin @%lu MHz\n", freq / 1000000);
>> +
>> +     if (of_device_is_compatible(np, "aspeed,ast2400-scu"))
>> +             aspeed_ast2400_cc(map);
>> +     else if (of_device_is_compatible(np, "aspeed,ast2500-scu"))
>> +             aspeed_ast2500_cc(map);
>> +     else
>> +             pr_err("unknown platform, failed to add clocks\n");
>
> I'm not confident that "borrowing" the SCU compatible like this is entirely in
> the spirit of its MFD nature, but maybe others can chime in.

Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Joel Stanley]

On Mon, Sep 25, 2017 at 10:40 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:

>> +     /*
>> +      * Memory controller (M-PLL) PLL. This clock is configured by the
>> +      * bootloader, and is exposed to Linux as a read-only clock rate.
>> +      */
>> +     regmap_read(map, ASPEED_MPLL_PARAM, &val);
>> +     aspeed_clk_data->hws[ASPEED_CLK_MPLL] = aspeed_calc_pll("mpll", val);
>
> IIRC the calculation in aspeed_calc_pll() is appropriate for the AST2500, but
> not the AST2400.

Ah ha. I knew there was a reason why I created an ast2400_calc_pll. I
will add it back in in the previous patch so we can use it here.

>> +     /* Video Engine (ECLK) mux and clock divider */
>> +     hw = clk_hw_register_mux(NULL, "eclk_mux",
>> +                     eclk_parents, ARRAY_SIZE(eclk_parents), 0,
>> +                     scu_base + ASPEED_CLK_SELECTION, 2, 2,
>> +                     0, &aspeed_clk_lock);
>> +     aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
>> +     hw = clk_hw_register_divider_table(NULL, "eclk", "eclk_mux", 0,
>> +                     scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
>
> On the AST2500 it looks like this should start at bit 28 for 3 bits, not bit

Good catch. Fixed in v3.

> 20. Separately, I'm not sure how to interpret the AST2400 datasheet here -
> maybe it's similar but with different wording ("clock slow down" rather than
> "divisor"?).
>
>> +                     div_table,
>
> This doesn't seem to be correct. There's the problem of 0b000 and 0b001 mapping
> the same value of 2 for the AST2500, whose table then increments in steps of 1.
> The AST2400 mapping on the otherhand is multiples of 2 starting at 2, with no
> inconsistency for 0b000 vs 0b001.

Yep, we do use a different table for ast2400 vs ast2500. See
ast2400_div_table vs ast2500_div_table.

>
>> +                     &aspeed_clk_lock);
>> +     aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
>> +
>> +     /* P-Bus (BCLK) clock divider */
>> +     hw = clk_hw_register_divider_table(NULL, "bclk", "hpll", 0,
>> +                     scu_base + ASPEED_CLK_SELECTION, 0, 2, 0,
>
> Bit 0 in SCU08 is a 1-bit field "CPU/AHB clock dynamic slow down enable". BCLK
> is actually in SCU*D*8, but (perhaps confusingly) documented immediately below
> SCU*0*8.

Good catch. Fixed in v3.

Cheers,

Joel

[PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Joel Stanley]

On Mon, Sep 25, 2017 at 10:40 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:

>> +     /*
>> +      * Memory controller (M-PLL) PLL. This clock is configured by the
>> +      * bootloader, and is exposed to Linux as a read-only clock rate.
>> +      */
>> +     regmap_read(map, ASPEED_MPLL_PARAM, &val);
>> +     aspeed_clk_data->hws[ASPEED_CLK_MPLL] = aspeed_calc_pll("mpll", val);
>
> IIRC the calculation in aspeed_calc_pll() is appropriate for the AST2500, but
> not the AST2400.

Ah ha. I knew there was a reason why I created an ast2400_calc_pll. I
will add it back in in the previous patch so we can use it here.

>> +     /* Video Engine (ECLK) mux and clock divider */
>> +     hw = clk_hw_register_mux(NULL, "eclk_mux",
>> +                     eclk_parents, ARRAY_SIZE(eclk_parents), 0,
>> +                     scu_base + ASPEED_CLK_SELECTION, 2, 2,
>> +                     0, &aspeed_clk_lock);
>> +     aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
>> +     hw = clk_hw_register_divider_table(NULL, "eclk", "eclk_mux", 0,
>> +                     scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
>
> On the AST2500 it looks like this should start at bit 28 for 3 bits, not bit

Good catch. Fixed in v3.

> 20. Separately, I'm not sure how to interpret the AST2400 datasheet here -
> maybe it's similar but with different wording ("clock slow down" rather than
> "divisor"?).
>
>> +                     div_table,
>
> This doesn't seem to be correct. There's the problem of 0b000 and 0b001 mapping
> the same value of 2 for the AST2500, whose table then increments in steps of 1.
> The AST2400 mapping on the otherhand is multiples of 2 starting at 2, with no
> inconsistency for 0b000 vs 0b001.

Yep, we do use a different table for ast2400 vs ast2500. See
ast2400_div_table vs ast2500_div_table.

>
>> +                     &aspeed_clk_lock);
>> +     aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
>> +
>> +     /* P-Bus (BCLK) clock divider */
>> +     hw = clk_hw_register_divider_table(NULL, "bclk", "hpll", 0,
>> +                     scu_base + ASPEED_CLK_SELECTION, 0, 2, 0,
>
> Bit 0 in SCU08 is a 1-bit field "CPU/AHB clock dynamic slow down enable". BCLK
> is actually in SCU*D*8, but (perhaps confusingly) documented immediately below
> SCU*0*8.

Good catch. Fixed in v3.

Cheers,

Joel

Re: [PATCH v2 5/5] clk: aspeed: Add reset controller

[Joel Stanley]

On Mon, Sep 25, 2017 at 10:54 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:
>> +static const u8 aspeed_resets[] = {
>> +     25, /* x-dma */
>> +     24, /* mctp */
>> +     23, /* adc */
>> +     22, /* jtag-master */
>> +     18, /* mic */
>> +      9, /* pwm */
>> +      8, /* pci-vga */
>> +      2, /* i2c */
>> +      1, /* ahb */
>
> Bit of a nit, but given you define macros for the indices, maybe use designated
> initialisers and drop the comments.

Done. And that way I'd notice that I missed the define for the ADC :)

Cheers,

Joel

[PATCH v2 5/5] clk: aspeed: Add reset controller

[Joel Stanley]

On Mon, Sep 25, 2017 at 10:54 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Thu, 2017-09-21 at 13:56 +0930, Joel Stanley wrote:
>> +static const u8 aspeed_resets[] = {
>> +     25, /* x-dma */
>> +     24, /* mctp */
>> +     23, /* adc */
>> +     22, /* jtag-master */
>> +     18, /* mic */
>> +      9, /* pwm */
>> +      8, /* pci-vga */
>> +      2, /* i2c */
>> +      1, /* ahb */
>
> Bit of a nit, but given you define macros for the indices, maybe use designated
> initialisers and drop the comments.

Done. And that way I'd notice that I missed the define for the ADC :)

Cheers,

Joel

Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On Wed, 2017-09-27 at 16:13 +1000, Joel Stanley wrote:
> > > +                     div_table,
> > 
> > This doesn't seem to be correct. There's the problem of 0b000 and 0b001 mapping
> > the same value of 2 for the AST2500, whose table then increments in steps of 1.
> > The AST2400 mapping on the otherhand is multiples of 2 starting at 2, with no
> > inconsistency for 0b000 vs 0b001.
> 
> Yep, we do use a different table for ast2400 vs ast2500. See
> ast2400_div_table vs ast2500_div_table.

Yep, but for the AST2500 this is a different table again to what you've
already defined (for the AST2500). However, for the AST2400 the table
looks the same as the other AST2400 tables.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Andrew Jeffery]

On Wed, 2017-09-27 at 16:13 +1000, Joel Stanley wrote:
> > > +???????????????????? div_table,
> > 
> > This doesn't seem to be correct. There's the problem of 0b000 and 0b001 mapping
> > the same value of 2 for the AST2500, whose table then increments in steps of 1.
> > The AST2400 mapping on the otherhand is multiples of 2 starting at 2, with no
> > inconsistency for 0b000 vs 0b001.
> 
> Yep, we do use a different table for ast2400 vs ast2500. See
> ast2400_div_table vs ast2500_div_table.

Yep, but for the AST2500 this is a different table again to what you've
already defined (for the AST2500). However, for the AST2400 the table
looks the same as the other AST2400 tables.

Cheers,

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170927/f60dc2e6/attachment.sig>

Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Joel Stanley]

On Wed, Sep 27, 2017 at 5:34 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Wed, 2017-09-27 at 16:13 +1000, Joel Stanley wrote:
>> > > +                     div_table,
>> >
>> > This doesn't seem to be correct. There's the problem of 0b000 and 0b001 mapping
>> > the same value of 2 for the AST2500, whose table then increments in steps of 1.
>> > The AST2400 mapping on the otherhand is multiples of 2 starting at 2, with no
>> > inconsistency for 0b000 vs 0b001.
>>
>> Yep, we do use a different table for ast2400 vs ast2500. See
>> ast2400_div_table vs ast2500_div_table.
>
> Yep, but for the AST2500 this is a different table again to what you've
> already defined (for the AST2500). However, for the AST2400 the table
> looks the same as the other AST2400 tables.

You're right. I didn't realise you were pointing out something strange
about eclk.

I added another table for eclk, and the correct one is selected by the
platform data.

I'll send out v4 today if no more reviews come in.

[PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Joel Stanley]

On Wed, Sep 27, 2017 at 5:34 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Wed, 2017-09-27 at 16:13 +1000, Joel Stanley wrote:
>> > > +                     div_table,
>> >
>> > This doesn't seem to be correct. There's the problem of 0b000 and 0b001 mapping
>> > the same value of 2 for the AST2500, whose table then increments in steps of 1.
>> > The AST2400 mapping on the otherhand is multiples of 2 starting at 2, with no
>> > inconsistency for 0b000 vs 0b001.
>>
>> Yep, we do use a different table for ast2400 vs ast2500. See
>> ast2400_div_table vs ast2500_div_table.
>
> Yep, but for the AST2500 this is a different table again to what you've
> already defined (for the AST2500). However, for the AST2400 the table
> looks the same as the other AST2400 tables.

You're right. I didn't realise you were pointing out something strange
about eclk.

I added another table for eclk, and the correct one is selected by the
platform data.

I'll send out v4 today if no more reviews come in.

Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Stephen Boyd]

On 09/21, Joel Stanley wrote:
> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>  			mult, div);
>  }
>  
> +static int __init aspeed_clk_probe(struct platform_device *pdev)

Drop __init? Should be a section mismatch with __init here.

> +{
> +	const struct aspeed_clk_soc_data *soc_data;
> +	const struct clk_div_table *mac_div_table;
> +	const struct clk_div_table *div_table;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *map;
> +	struct clk_hw *hw;
> +	u32 val, rate;
> +
> +	map = syscon_node_to_regmap(dev->of_node);
> +	if (IS_ERR(map)) {
> +		dev_err(dev, "no syscon regmap\n");
> +		return PTR_ERR(map);
> +	}
> +
> +	/* SoC generations share common layouts but have different divisors */
> +	soc_data = of_device_get_match_data(&pdev->dev);

Check for soc_data being NULL.

> +	div_table = soc_data->div_table;
> +	mac_div_table = soc_data->mac_div_table;
> +
> +	/* UART clock div13 setting */
> +	regmap_read(map, ASPEED_MISC_CTRL, &val);
> +	if (val & BIT(12))

What does BIT(12) mean? #define or comment please.

> +		rate = 24000000 / 13;
> +	else
> +		rate = 24000000;
> +	/* TODO: Find the parent data for the uart clock */
> +	hw = clk_hw_register_fixed_rate(NULL, "uart", NULL, 0, rate);
> +	aspeed_clk_data->hws[ASPEED_CLK_UART] = hw;
> +
> +	/*
> +	 * Memory controller (M-PLL) PLL. This clock is configured by the
> +	 * bootloader, and is exposed to Linux as a read-only clock rate.
> +	 */
> +	regmap_read(map, ASPEED_MPLL_PARAM, &val);
> +	aspeed_clk_data->hws[ASPEED_CLK_MPLL] =	aspeed_calc_pll("mpll", val);
> +
> +	/* SD/SDIO clock divider (TODO: There's a gate too) */
> +	hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,

Please pass your dev pointer here from the platform device.

> +			scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
> +			div_table,
> +			&aspeed_clk_lock);

And check for errors? Perhaps use devm_clk_hw_regsiter() APIs and
construct the dividers and muxes directly instead of using the
basic type registration APIs.

> +	aspeed_clk_data->hws[ASPEED_CLK_SDIO] = hw;
> +
> +	/* MAC AHB bus clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "mac", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 16, 3, 0,
> +			mac_div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_MAC] = hw;
> +
> +	/* LPC Host (LHCLK) clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "lhclk", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_LHCLK] = hw;
> +
> +	/* Video Engine (ECLK) mux and clock divider */
> +	hw = clk_hw_register_mux(NULL, "eclk_mux",
> +			eclk_parents, ARRAY_SIZE(eclk_parents), 0,
> +			scu_base + ASPEED_CLK_SELECTION, 2, 2,
> +			0, &aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
> +	hw = clk_hw_register_divider_table(NULL, "eclk", "eclk_mux", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
> +
> +	/* P-Bus (BCLK) clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "bclk", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 0, 2, 0,
> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
> +
> +	return 0;
> +};
> +
> +static const struct of_device_id aspeed_clk_dt_ids[] = {
> +	{ .compatible = "aspeed,ast2400-scu", .data = &ast2400_data },
> +	{ .compatible = "aspeed,ast2500-scu", .data = &ast2500_data },
> +	{ },

           ^
Nitpick, drop the comma

> +};
> +
> +static struct platform_driver aspeed_clk_driver = {
> +	.probe  = aspeed_clk_probe,
> +	.driver = {
> +		.name = "aspeed-clk",
> +		.of_match_table = aspeed_clk_dt_ids,
> +		.suppress_bind_attrs = true,
> +	},
> +};
> +builtin_platform_driver(aspeed_clk_driver);
> +
> +

Kill a newline, save the internet.

>  static void __init aspeed_ast2400_cc(struct regmap *map)
>  {
>  	struct clk_hw *hw;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Stephen Boyd]

On 09/21, Joel Stanley wrote:
> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>  			mult, div);
>  }
>  
> +static int __init aspeed_clk_probe(struct platform_device *pdev)

Drop __init? Should be a section mismatch with __init here.

> +{
> +	const struct aspeed_clk_soc_data *soc_data;
> +	const struct clk_div_table *mac_div_table;
> +	const struct clk_div_table *div_table;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *map;
> +	struct clk_hw *hw;
> +	u32 val, rate;
> +
> +	map = syscon_node_to_regmap(dev->of_node);
> +	if (IS_ERR(map)) {
> +		dev_err(dev, "no syscon regmap\n");
> +		return PTR_ERR(map);
> +	}
> +
> +	/* SoC generations share common layouts but have different divisors */
> +	soc_data = of_device_get_match_data(&pdev->dev);

Check for soc_data being NULL.

> +	div_table = soc_data->div_table;
> +	mac_div_table = soc_data->mac_div_table;
> +
> +	/* UART clock div13 setting */
> +	regmap_read(map, ASPEED_MISC_CTRL, &val);
> +	if (val & BIT(12))

What does BIT(12) mean? #define or comment please.

> +		rate = 24000000 / 13;
> +	else
> +		rate = 24000000;
> +	/* TODO: Find the parent data for the uart clock */
> +	hw = clk_hw_register_fixed_rate(NULL, "uart", NULL, 0, rate);
> +	aspeed_clk_data->hws[ASPEED_CLK_UART] = hw;
> +
> +	/*
> +	 * Memory controller (M-PLL) PLL. This clock is configured by the
> +	 * bootloader, and is exposed to Linux as a read-only clock rate.
> +	 */
> +	regmap_read(map, ASPEED_MPLL_PARAM, &val);
> +	aspeed_clk_data->hws[ASPEED_CLK_MPLL] =	aspeed_calc_pll("mpll", val);
> +
> +	/* SD/SDIO clock divider (TODO: There's a gate too) */
> +	hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,

Please pass your dev pointer here from the platform device.

> +			scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
> +			div_table,
> +			&aspeed_clk_lock);

And check for errors? Perhaps use devm_clk_hw_regsiter() APIs and
construct the dividers and muxes directly instead of using the
basic type registration APIs.

> +	aspeed_clk_data->hws[ASPEED_CLK_SDIO] = hw;
> +
> +	/* MAC AHB bus clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "mac", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 16, 3, 0,
> +			mac_div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_MAC] = hw;
> +
> +	/* LPC Host (LHCLK) clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "lhclk", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_LHCLK] = hw;
> +
> +	/* Video Engine (ECLK) mux and clock divider */
> +	hw = clk_hw_register_mux(NULL, "eclk_mux",
> +			eclk_parents, ARRAY_SIZE(eclk_parents), 0,
> +			scu_base + ASPEED_CLK_SELECTION, 2, 2,
> +			0, &aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
> +	hw = clk_hw_register_divider_table(NULL, "eclk", "eclk_mux", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
> +
> +	/* P-Bus (BCLK) clock divider */
> +	hw = clk_hw_register_divider_table(NULL, "bclk", "hpll", 0,
> +			scu_base + ASPEED_CLK_SELECTION, 0, 2, 0,
> +			div_table,
> +			&aspeed_clk_lock);
> +	aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
> +
> +	return 0;
> +};
> +
> +static const struct of_device_id aspeed_clk_dt_ids[] = {
> +	{ .compatible = "aspeed,ast2400-scu", .data = &ast2400_data },
> +	{ .compatible = "aspeed,ast2500-scu", .data = &ast2500_data },
> +	{ },

           ^
Nitpick, drop the comma

> +};
> +
> +static struct platform_driver aspeed_clk_driver = {
> +	.probe  = aspeed_clk_probe,
> +	.driver = {
> +		.name = "aspeed-clk",
> +		.of_match_table = aspeed_clk_dt_ids,
> +		.suppress_bind_attrs = true,
> +	},
> +};
> +builtin_platform_driver(aspeed_clk_driver);
> +
> +

Kill a newline, save the internet.

>  static void __init aspeed_ast2400_cc(struct regmap *map)
>  {
>  	struct clk_hw *hw;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 4/5] clk: aspeed: Register gated clocks

[Stephen Boyd]

On 09/21, Joel Stanley wrote:
> The majority of the clocks in the system are gates paired with a reset
> controller that holds the IP in reset.
> 
> This borrows from clk_hw_register_gate, but registers two 'gates', one
> to control the clock enable register and the other to control the reset
> IP. This allows us to enforce the ordering:
> 
>  1. Place IP in reset
>  2. Enable clock
>  3. Delay
>  4. Release reset
> 
> There are some gates that do not have an associated reset; these are
> handled by using -1 as the index for the reset.

Half of these aren't clks, but reset bits? Perhaps you should
register power domains for these things and then have the power
domain assert the reset, enable the clock, delay, and then
release the reset in the poweron callback. Then device drivers
don't have to be aware of the ordering, and you can keep
enforcing the ordering in one place, but we don't have to make
fake gates and shoehorn the sequence into the clk framework.

> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/clk/clk-aspeed.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 128 insertions(+)
> 
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 19531798e040..dec9db4ec47b 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -191,6 +191,108 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>  			mult, div);
>  }
>  
> +static int aspeed_clk_enable(struct clk_hw *hw)
> +{
> +	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> +	unsigned long flags;
> +	u32 clk = BIT(gate->clock_idx);
> +	u32 rst = BIT(gate->reset_idx);
> +
> +	spin_lock_irqsave(gate->lock, flags);
> +
> +	if (gate->reset_idx >= 0) {
> +		/* Put IP in reset */
> +		regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, rst);
> +
> +		/* Delay 100us */
> +		udelay(100);
> +	}
> +
> +	/* Enable clock */
> +	regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, 0);
> +
> +	if (gate->reset_idx >= 0) {
> +		/* Delay 10ms */
> +		/* TODO: can we sleep here? */
> +		msleep(10);
> +
> +		/* Take IP out of reset */
> +		regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, 0);
> +	}
> +
> +	spin_unlock_irqrestore(gate->lock, flags);
> +
> +	return 0;
> +}
> +
> +static void aspeed_clk_disable(struct clk_hw *hw)
> +{
> +	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> +	unsigned long flags;
> +	u32 clk = BIT(gate->clock_idx);
> +
> +	spin_lock_irqsave(gate->lock, flags);
> +
> +	/* Disable clock */

Drop useless comment please.

> +	regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, clk);
> +
> +	spin_unlock_irqrestore(gate->lock, flags);
> +}
> +
> +static int aspeed_clk_is_enabled(struct clk_hw *hw)
> +{
> +	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> +	u32 clk = BIT(gate->clock_idx);
> +	u32 reg;
> +
> +	regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
> +
> +	return (reg & clk) ? 0 : 1;
> +}
> +
> +static const struct clk_ops aspeed_clk_gate_ops = {
> +	.enable = aspeed_clk_enable,
> +	.disable = aspeed_clk_disable,
> +	.is_enabled = aspeed_clk_is_enabled,
> +};
> +
> +static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
> +		const char *name, const char *parent_name, unsigned long flags,
> +		struct regmap *map, u8 clock_idx, u8 reset_idx,
> +		u8 clk_gate_flags, spinlock_t *lock)
> +{
> +	struct aspeed_clk_gate *gate;
> +	struct clk_init_data init;
> +	struct clk_hw *hw;
> +	int ret;
> +
> +	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> +	if (!gate)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init.name = name;
> +	init.ops = &aspeed_clk_gate_ops;
> +	init.flags = flags | CLK_IS_BASIC;

Please drop CLK_IS_BASIC.

> +	init.parent_names = parent_name ? &parent_name : NULL;
> +	init.num_parents = parent_name ? 1 : 0;
> +
> +	gate->map = map;
> +	gate->clock_idx = clock_idx;
> +	gate->reset_idx = reset_idx;
> +	gate->flags = clk_gate_flags;
> +	gate->lock = lock;
> +	gate->hw.init = &init;
> +
> +	hw = &gate->hw;
> +	ret = clk_hw_register(dev, hw);
> +	if (ret) {
> +		kfree(gate);
> +		hw = ERR_PTR(ret);
> +	}
> +
> +	return hw;
> +}
> +
>  static int __init aspeed_clk_probe(struct platform_device *pdev)
>  {
>  	const struct aspeed_clk_soc_data *soc_data;
> @@ -200,6 +302,7 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
>  	struct regmap *map;
>  	struct clk_hw *hw;
>  	u32 val, rate;
> +	int i;
>  
>  	map = syscon_node_to_regmap(dev->of_node);
>  	if (IS_ERR(map)) {
> @@ -269,6 +372,31 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
>  			&aspeed_clk_lock);
>  	aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
>  
> +	/* There are a number of clocks that not included in this driver as

Needs a /* before comment text starts. Also, TODO?

> +	 * more information is required:
> +	 *   D2-PLL
> +	 *   D-PLL
> +	 *   YCLK
> +	 *   RGMII
> +	 *   RMII
> +	 *   UART[1..5] clock source mux
> +	 */
> +
> +	for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) {
> +		const struct aspeed_gate_data *gd;
> +
> +		gd = &aspeed_gates[i];
> +		aspeed_clk_data->hws[ASPEED_CLK_GATES + i] =
> +			aspeed_clk_hw_register_gate(NULL, gd->name,

Pass device?

> +					     gd->parent_name,
> +					     gd->flags,
> +					     map,
> +					     gd->clock_idx,
> +					     gd->reset_idx,
> +					     CLK_GATE_SET_TO_DISABLE,
> +					     &aspeed_clk_lock);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 4/5] clk: aspeed: Register gated clocks

[Stephen Boyd]

On 09/21, Joel Stanley wrote:
> The majority of the clocks in the system are gates paired with a reset
> controller that holds the IP in reset.
> 
> This borrows from clk_hw_register_gate, but registers two 'gates', one
> to control the clock enable register and the other to control the reset
> IP. This allows us to enforce the ordering:
> 
>  1. Place IP in reset
>  2. Enable clock
>  3. Delay
>  4. Release reset
> 
> There are some gates that do not have an associated reset; these are
> handled by using -1 as the index for the reset.

Half of these aren't clks, but reset bits? Perhaps you should
register power domains for these things and then have the power
domain assert the reset, enable the clock, delay, and then
release the reset in the poweron callback. Then device drivers
don't have to be aware of the ordering, and you can keep
enforcing the ordering in one place, but we don't have to make
fake gates and shoehorn the sequence into the clk framework.

> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/clk/clk-aspeed.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 128 insertions(+)
> 
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 19531798e040..dec9db4ec47b 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -191,6 +191,108 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>  			mult, div);
>  }
>  
> +static int aspeed_clk_enable(struct clk_hw *hw)
> +{
> +	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> +	unsigned long flags;
> +	u32 clk = BIT(gate->clock_idx);
> +	u32 rst = BIT(gate->reset_idx);
> +
> +	spin_lock_irqsave(gate->lock, flags);
> +
> +	if (gate->reset_idx >= 0) {
> +		/* Put IP in reset */
> +		regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, rst);
> +
> +		/* Delay 100us */
> +		udelay(100);
> +	}
> +
> +	/* Enable clock */
> +	regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, 0);
> +
> +	if (gate->reset_idx >= 0) {
> +		/* Delay 10ms */
> +		/* TODO: can we sleep here? */
> +		msleep(10);
> +
> +		/* Take IP out of reset */
> +		regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, 0);
> +	}
> +
> +	spin_unlock_irqrestore(gate->lock, flags);
> +
> +	return 0;
> +}
> +
> +static void aspeed_clk_disable(struct clk_hw *hw)
> +{
> +	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> +	unsigned long flags;
> +	u32 clk = BIT(gate->clock_idx);
> +
> +	spin_lock_irqsave(gate->lock, flags);
> +
> +	/* Disable clock */

Drop useless comment please.

> +	regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, clk);
> +
> +	spin_unlock_irqrestore(gate->lock, flags);
> +}
> +
> +static int aspeed_clk_is_enabled(struct clk_hw *hw)
> +{
> +	struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> +	u32 clk = BIT(gate->clock_idx);
> +	u32 reg;
> +
> +	regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, &reg);
> +
> +	return (reg & clk) ? 0 : 1;
> +}
> +
> +static const struct clk_ops aspeed_clk_gate_ops = {
> +	.enable = aspeed_clk_enable,
> +	.disable = aspeed_clk_disable,
> +	.is_enabled = aspeed_clk_is_enabled,
> +};
> +
> +static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
> +		const char *name, const char *parent_name, unsigned long flags,
> +		struct regmap *map, u8 clock_idx, u8 reset_idx,
> +		u8 clk_gate_flags, spinlock_t *lock)
> +{
> +	struct aspeed_clk_gate *gate;
> +	struct clk_init_data init;
> +	struct clk_hw *hw;
> +	int ret;
> +
> +	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> +	if (!gate)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init.name = name;
> +	init.ops = &aspeed_clk_gate_ops;
> +	init.flags = flags | CLK_IS_BASIC;

Please drop CLK_IS_BASIC.

> +	init.parent_names = parent_name ? &parent_name : NULL;
> +	init.num_parents = parent_name ? 1 : 0;
> +
> +	gate->map = map;
> +	gate->clock_idx = clock_idx;
> +	gate->reset_idx = reset_idx;
> +	gate->flags = clk_gate_flags;
> +	gate->lock = lock;
> +	gate->hw.init = &init;
> +
> +	hw = &gate->hw;
> +	ret = clk_hw_register(dev, hw);
> +	if (ret) {
> +		kfree(gate);
> +		hw = ERR_PTR(ret);
> +	}
> +
> +	return hw;
> +}
> +
>  static int __init aspeed_clk_probe(struct platform_device *pdev)
>  {
>  	const struct aspeed_clk_soc_data *soc_data;
> @@ -200,6 +302,7 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
>  	struct regmap *map;
>  	struct clk_hw *hw;
>  	u32 val, rate;
> +	int i;
>  
>  	map = syscon_node_to_regmap(dev->of_node);
>  	if (IS_ERR(map)) {
> @@ -269,6 +372,31 @@ static int __init aspeed_clk_probe(struct platform_device *pdev)
>  			&aspeed_clk_lock);
>  	aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
>  
> +	/* There are a number of clocks that not included in this driver as

Needs a /* before comment text starts. Also, TODO?

> +	 * more information is required:
> +	 *   D2-PLL
> +	 *   D-PLL
> +	 *   YCLK
> +	 *   RGMII
> +	 *   RMII
> +	 *   UART[1..5] clock source mux
> +	 */
> +
> +	for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) {
> +		const struct aspeed_gate_data *gd;
> +
> +		gd = &aspeed_gates[i];
> +		aspeed_clk_data->hws[ASPEED_CLK_GATES + i] =
> +			aspeed_clk_hw_register_gate(NULL, gd->name,

Pass device?

> +					     gd->parent_name,
> +					     gd->flags,
> +					     map,
> +					     gd->clock_idx,
> +					     gd->reset_idx,
> +					     CLK_GATE_SET_TO_DISABLE,
> +					     &aspeed_clk_lock);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/5] clk: aspeed: Register core clocks

[Stephen Boyd]

On 09/21, Joel Stanley wrote:
> @@ -112,9 +115,137 @@ static const struct aspeed_gate_data aspeed_gates[] __initconst = {
>  	/* 31: reserved */
>  };
>  
> +static const struct clk_div_table ast2400_div_table[] = {
> +	{ 0x0, 2 },
> +	{ 0x1, 4 },
> +	{ 0x2, 6 },
> +	{ 0x3, 8 },
> +	{ 0x4, 10 },
> +	{ 0x5, 12 },
> +	{ 0x6, 14 },
> +	{ 0x7, 16 },
> +	{ 0 }
> +};
> +
> +static const struct clk_div_table ast2500_div_table[] = {
> +	{ 0x0, 4 },
> +	{ 0x1, 8 },
> +	{ 0x2, 12 },
> +	{ 0x3, 16 },
> +	{ 0x4, 20 },
> +	{ 0x5, 24 },
> +	{ 0x6, 28 },
> +	{ 0x7, 32 },
> +	{ 0 }
> +};
> +
> +static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
> +{
> +	unsigned int mult, div;
> +
> +	if (val & BIT(20)) {

#define for these BIT() things please. Or a comment, but #define
is probably better. Just improves readability.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/5] clk: aspeed: Register core clocks

[Stephen Boyd]

On 09/21, Joel Stanley wrote:
> @@ -112,9 +115,137 @@ static const struct aspeed_gate_data aspeed_gates[] __initconst = {
>  	/* 31: reserved */
>  };
>  
> +static const struct clk_div_table ast2400_div_table[] = {
> +	{ 0x0, 2 },
> +	{ 0x1, 4 },
> +	{ 0x2, 6 },
> +	{ 0x3, 8 },
> +	{ 0x4, 10 },
> +	{ 0x5, 12 },
> +	{ 0x6, 14 },
> +	{ 0x7, 16 },
> +	{ 0 }
> +};
> +
> +static const struct clk_div_table ast2500_div_table[] = {
> +	{ 0x0, 4 },
> +	{ 0x1, 8 },
> +	{ 0x2, 12 },
> +	{ 0x3, 16 },
> +	{ 0x4, 20 },
> +	{ 0x5, 24 },
> +	{ 0x6, 28 },
> +	{ 0x7, 32 },
> +	{ 0 }
> +};
> +
> +static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
> +{
> +	unsigned int mult, div;
> +
> +	if (val & BIT(20)) {

#define for these BIT() things please. Or a comment, but #define
is probably better. Just improves readability.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 4/5] clk: aspeed: Register gated clocks

[Joel Stanley]

On Tue, Oct 3, 2017 at 7:07 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/21, Joel Stanley wrote:
>> The majority of the clocks in the system are gates paired with a reset
>> controller that holds the IP in reset.
>>
>> This borrows from clk_hw_register_gate, but registers two 'gates', one
>> to control the clock enable register and the other to control the reset
>> IP. This allows us to enforce the ordering:
>>
>>  1. Place IP in reset
>>  2. Enable clock
>>  3. Delay
>>  4. Release reset
>>
>> There are some gates that do not have an associated reset; these are
>> handled by using -1 as the index for the reset.
>
> Half of these aren't clks, but reset bits? Perhaps you should
> register power domains for these things and then have the power
> domain assert the reset, enable the clock, delay, and then
> release the reset in the poweron callback. Then device drivers
> don't have to be aware of the ordering, and you can keep
> enforcing the ordering in one place, but we don't have to make
> fake gates and shoehorn the sequence into the clk framework.

I had a look. We've got 24 gates being registered, and half are just
gate+reset pairs, while the other half are both. Some of them have
clocks with divisors upstream of the gate, so in those cases we do
care what the rate is. For the others they are simply bringing the IP
online.

Note that we don't have drivers for all of the peripherals, so things
may change once those drivers are written.

I hadn't looked into the power domain stuff - when I brought this up
on the list recently Philip suggested that this should be contained in
a clk driver, so that's the direction we went:

 http://www.spinics.net/lists/linux-clk/msg18733.html

Cheers,

Joel

[PATCH v2 4/5] clk: aspeed: Register gated clocks

[Joel Stanley]

On Tue, Oct 3, 2017 at 7:07 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/21, Joel Stanley wrote:
>> The majority of the clocks in the system are gates paired with a reset
>> controller that holds the IP in reset.
>>
>> This borrows from clk_hw_register_gate, but registers two 'gates', one
>> to control the clock enable register and the other to control the reset
>> IP. This allows us to enforce the ordering:
>>
>>  1. Place IP in reset
>>  2. Enable clock
>>  3. Delay
>>  4. Release reset
>>
>> There are some gates that do not have an associated reset; these are
>> handled by using -1 as the index for the reset.
>
> Half of these aren't clks, but reset bits? Perhaps you should
> register power domains for these things and then have the power
> domain assert the reset, enable the clock, delay, and then
> release the reset in the poweron callback. Then device drivers
> don't have to be aware of the ordering, and you can keep
> enforcing the ordering in one place, but we don't have to make
> fake gates and shoehorn the sequence into the clk framework.

I had a look. We've got 24 gates being registered, and half are just
gate+reset pairs, while the other half are both. Some of them have
clocks with divisors upstream of the gate, so in those cases we do
care what the rate is. For the others they are simply bringing the IP
online.

Note that we don't have drivers for all of the peripherals, so things
may change once those drivers are written.

I hadn't looked into the power domain stuff - when I brought this up
on the list recently Philip suggested that this should be contained in
a clk driver, so that's the direction we went:

 http://www.spinics.net/lists/linux-clk/msg18733.html

Cheers,

Joel

Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Joel Stanley]

On Tue, Oct 3, 2017 at 6:54 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/21, Joel Stanley wrote:
>> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>> +     /*
>> +      * Memory controller (M-PLL) PLL. This clock is configured by the
>> +      * bootloader, and is exposed to Linux as a read-only clock rate.
>> +      */
>> +     regmap_read(map, ASPEED_MPLL_PARAM, &val);
>> +     aspeed_clk_data->hws[ASPEED_CLK_MPLL] = aspeed_calc_pll("mpll", val);
>> +
>> +     /* SD/SDIO clock divider (TODO: There's a gate too) */
>> +     hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
>
> Please pass your dev pointer here from the platform device.
>
>> +                     scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
>> +                     div_table,
>> +                     &aspeed_clk_lock);
>
> And check for errors? Perhaps use devm_clk_hw_regsiter() APIs and
> construct the dividers and muxes directly instead of using the
> basic type registration APIs.

Do you think that devm_ is overkill, given we will never unload this driver?

Can you explain why you suggest to construct the structures directly
instead of using the APIs?

I had a read of the basic type registration functions, and the
relevant failure paths are memory allocation failures. If we're out of
memory that early in boot then things have gone pretty bad.

I can add checks for null and bail out; I don't think there's value in
freeing the allocated memory: if a system can't load it's clock driver
then it's super hosed.

Thanks for the review. I fixed all of the other things you mentioned.

Cheers,

Joel

[PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Joel Stanley]

On Tue, Oct 3, 2017 at 6:54 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/21, Joel Stanley wrote:
>> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>> +     /*
>> +      * Memory controller (M-PLL) PLL. This clock is configured by the
>> +      * bootloader, and is exposed to Linux as a read-only clock rate.
>> +      */
>> +     regmap_read(map, ASPEED_MPLL_PARAM, &val);
>> +     aspeed_clk_data->hws[ASPEED_CLK_MPLL] = aspeed_calc_pll("mpll", val);
>> +
>> +     /* SD/SDIO clock divider (TODO: There's a gate too) */
>> +     hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
>
> Please pass your dev pointer here from the platform device.
>
>> +                     scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
>> +                     div_table,
>> +                     &aspeed_clk_lock);
>
> And check for errors? Perhaps use devm_clk_hw_regsiter() APIs and
> construct the dividers and muxes directly instead of using the
> basic type registration APIs.

Do you think that devm_ is overkill, given we will never unload this driver?

Can you explain why you suggest to construct the structures directly
instead of using the APIs?

I had a read of the basic type registration functions, and the
relevant failure paths are memory allocation failures. If we're out of
memory that early in boot then things have gone pretty bad.

I can add checks for null and bail out; I don't think there's value in
freeing the allocated memory: if a system can't load it's clock driver
then it's super hosed.

Thanks for the review. I fixed all of the other things you mentioned.

Cheers,

Joel

Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Stephen Boyd]

On 10/03, Joel Stanley wrote:
> On Tue, Oct 3, 2017 at 6:54 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 09/21, Joel Stanley wrote:
> >> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
> >> +     /*
> >> +      * Memory controller (M-PLL) PLL. This clock is configured by the
> >> +      * bootloader, and is exposed to Linux as a read-only clock rate.
> >> +      */
> >> +     regmap_read(map, ASPEED_MPLL_PARAM, &val);
> >> +     aspeed_clk_data->hws[ASPEED_CLK_MPLL] = aspeed_calc_pll("mpll", val);
> >> +
> >> +     /* SD/SDIO clock divider (TODO: There's a gate too) */
> >> +     hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
> >
> > Please pass your dev pointer here from the platform device.
> >
> >> +                     scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
> >> +                     div_table,
> >> +                     &aspeed_clk_lock);
> >
> > And check for errors? Perhaps use devm_clk_hw_regsiter() APIs and
> > construct the dividers and muxes directly instead of using the
> > basic type registration APIs.
> 
> Do you think that devm_ is overkill, given we will never unload this driver?

Is probe defer going to happen? Even if unload can't happen,
probe defer is a concern unless that is also ruled out.

> 
> Can you explain why you suggest to construct the structures directly
> instead of using the APIs?

There aren't devm APIs for some of these basic clk type
registration functions.

> 
> I had a read of the basic type registration functions, and the
> relevant failure paths are memory allocation failures. If we're out of
> memory that early in boot then things have gone pretty bad.
> 
> I can add checks for null and bail out; I don't think there's value in
> freeing the allocated memory: if a system can't load it's clock driver
> then it's super hosed.

If we can't proceed without this driver because it's super hosed,
then perhaps we need to panic the system on errors here. Should
be simple enough to add some error checks and goto panic("Things
are super hosed").

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Stephen Boyd]

On 10/03, Joel Stanley wrote:
> On Tue, Oct 3, 2017 at 6:54 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 09/21, Joel Stanley wrote:
> >> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
> >> +     /*
> >> +      * Memory controller (M-PLL) PLL. This clock is configured by the
> >> +      * bootloader, and is exposed to Linux as a read-only clock rate.
> >> +      */
> >> +     regmap_read(map, ASPEED_MPLL_PARAM, &val);
> >> +     aspeed_clk_data->hws[ASPEED_CLK_MPLL] = aspeed_calc_pll("mpll", val);
> >> +
> >> +     /* SD/SDIO clock divider (TODO: There's a gate too) */
> >> +     hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
> >
> > Please pass your dev pointer here from the platform device.
> >
> >> +                     scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
> >> +                     div_table,
> >> +                     &aspeed_clk_lock);
> >
> > And check for errors? Perhaps use devm_clk_hw_regsiter() APIs and
> > construct the dividers and muxes directly instead of using the
> > basic type registration APIs.
> 
> Do you think that devm_ is overkill, given we will never unload this driver?

Is probe defer going to happen? Even if unload can't happen,
probe defer is a concern unless that is also ruled out.

> 
> Can you explain why you suggest to construct the structures directly
> instead of using the APIs?

There aren't devm APIs for some of these basic clk type
registration functions.

> 
> I had a read of the basic type registration functions, and the
> relevant failure paths are memory allocation failures. If we're out of
> memory that early in boot then things have gone pretty bad.
> 
> I can add checks for null and bail out; I don't think there's value in
> freeing the allocated memory: if a system can't load it's clock driver
> then it's super hosed.

If we can't proceed without this driver because it's super hosed,
then perhaps we need to panic the system on errors here. Should
be simple enough to add some error checks and goto panic("Things
are super hosed").

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Joel Stanley]

On Thu, Oct 5, 2017 at 6:48 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 10/03, Joel Stanley wrote:
>> On Tue, Oct 3, 2017 at 6:54 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 09/21, Joel Stanley wrote:
>> >> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>> >> +     /*
>> >> +      * Memory controller (M-PLL) PLL. This clock is configured by the
>> >> +      * bootloader, and is exposed to Linux as a read-only clock rate.
>> >> +      */
>> >> +     regmap_read(map, ASPEED_MPLL_PARAM, &val);
>> >> +     aspeed_clk_data->hws[ASPEED_CLK_MPLL] = aspeed_calc_pll("mpll", val);
>> >> +
>> >> +     /* SD/SDIO clock divider (TODO: There's a gate too) */
>> >> +     hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
>> >
>> > Please pass your dev pointer here from the platform device.
>> >
>> >> +                     scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
>> >> +                     div_table,
>> >> +                     &aspeed_clk_lock);
>> >
>> > And check for errors? Perhaps use devm_clk_hw_regsiter() APIs and
>> > construct the dividers and muxes directly instead of using the
>> > basic type registration APIs.
>>
>> Do you think that devm_ is overkill, given we will never unload this driver?
>
> Is probe defer going to happen? Even if unload can't happen,
> probe defer is a concern unless that is also ruled out.

As I understand it, I will only get an EPROBE_DEFER if I'm requesting
something from another driver?

If that's the case then we can rule that out.

>>
>> Can you explain why you suggest to construct the structures directly
>> instead of using the APIs?
>
> There aren't devm APIs for some of these basic clk type
> registration functions.
>
>>
>> I had a read of the basic type registration functions, and the
>> relevant failure paths are memory allocation failures. If we're out of
>> memory that early in boot then things have gone pretty bad.
>>
>> I can add checks for null and bail out; I don't think there's value in
>> freeing the allocated memory: if a system can't load it's clock driver
>> then it's super hosed.
>
> If we can't proceed without this driver because it's super hosed,
> then perhaps we need to panic the system on errors here. Should
> be simple enough to add some error checks and goto panic("Things
> are super hosed").

Okay. I added checks in v4. Does that look ok you you?

Cheers,

Joel

[PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

[Joel Stanley]

On Thu, Oct 5, 2017 at 6:48 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 10/03, Joel Stanley wrote:
>> On Tue, Oct 3, 2017 at 6:54 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 09/21, Joel Stanley wrote:
>> >> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>> >> +     /*
>> >> +      * Memory controller (M-PLL) PLL. This clock is configured by the
>> >> +      * bootloader, and is exposed to Linux as a read-only clock rate.
>> >> +      */
>> >> +     regmap_read(map, ASPEED_MPLL_PARAM, &val);
>> >> +     aspeed_clk_data->hws[ASPEED_CLK_MPLL] = aspeed_calc_pll("mpll", val);
>> >> +
>> >> +     /* SD/SDIO clock divider (TODO: There's a gate too) */
>> >> +     hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
>> >
>> > Please pass your dev pointer here from the platform device.
>> >
>> >> +                     scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
>> >> +                     div_table,
>> >> +                     &aspeed_clk_lock);
>> >
>> > And check for errors? Perhaps use devm_clk_hw_regsiter() APIs and
>> > construct the dividers and muxes directly instead of using the
>> > basic type registration APIs.
>>
>> Do you think that devm_ is overkill, given we will never unload this driver?
>
> Is probe defer going to happen? Even if unload can't happen,
> probe defer is a concern unless that is also ruled out.

As I understand it, I will only get an EPROBE_DEFER if I'm requesting
something from another driver?

If that's the case then we can rule that out.

>>
>> Can you explain why you suggest to construct the structures directly
>> instead of using the APIs?
>
> There aren't devm APIs for some of these basic clk type
> registration functions.
>
>>
>> I had a read of the basic type registration functions, and the
>> relevant failure paths are memory allocation failures. If we're out of
>> memory that early in boot then things have gone pretty bad.
>>
>> I can add checks for null and bail out; I don't think there's value in
>> freeing the allocated memory: if a system can't load it's clock driver
>> then it's super hosed.
>
> If we can't proceed without this driver because it's super hosed,
> then perhaps we need to panic the system on errors here. Should
> be simple enough to add some error checks and goto panic("Things
> are super hosed").

Okay. I added checks in v4. Does that look ok you you?

Cheers,

Joel