[PATCH 0/6] ARM: mstar: Basic MPLL support

[PATCH 0/6] ARM: mstar: Basic MPLL support

[Daniel Palmer]

This series adds support for the MPLL block that is present in
MStar/SigmaStar ARMv7 SoCs.

This block is intended to be set and forgotten about before
Linux is running so all it actually does it read the registers
and calculate what the output frequencies should be.

We only care about this block because there are upstream dividers,
gates, muxes etc that need something between the input crystal
and themselves to calculate their own rates.

I think this needs to be applied after the GPIO series[0] that
is currently inflight.

0 - https://lore.kernel.org/linux-gpio/20201109121731.1537580-1-daniel@0x0f.com/

Daniel Palmer (6):
  dt-bindings: clk: mstar msc313 mpll binding header
  dt-bindings: clk: mstar msc313 mpll binding description
  clk: mstar: MStar/SigmaStar MPLL driver
  ARM: mstar: Select MSTAR_MSC313_MPLL
  ARM: mstar: Add the external clocks to the base dsti
  ARM: mstar: Add mpll to base dtsi

 .../bindings/clock/mstar,msc313-mpll.yaml     |  58 ++++++
 MAINTAINERS                                   |   3 +
 arch/arm/boot/dts/mstar-v7.dtsi               |  27 +++
 arch/arm/mach-mstar/Kconfig                   |   1 +
 drivers/clk/Kconfig                           |   1 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/mstar/Kconfig                     |   5 +
 drivers/clk/mstar/Makefile                    |   6 +
 drivers/clk/mstar/clk-msc313-mpll.c           | 177 ++++++++++++++++++
 include/dt-bindings/clock/mstar-msc313-mpll.h |  19 ++
 10 files changed, 298 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
 create mode 100644 drivers/clk/mstar/Kconfig
 create mode 100644 drivers/clk/mstar/Makefile
 create mode 100644 drivers/clk/mstar/clk-msc313-mpll.c
 create mode 100644 include/dt-bindings/clock/mstar-msc313-mpll.h

-- 
2.29.2

[PATCH 1/6] dt-bindings: clk: mstar msc313 mpll binding header

[Daniel Palmer]

Simple header to document the relationship between the MPLL outputs
and which divider they come from.

Output 0 is missing because it should not be consumed.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 MAINTAINERS                                   |  1 +
 include/dt-bindings/clock/mstar-msc313-mpll.h | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)
 create mode 100644 include/dt-bindings/clock/mstar-msc313-mpll.h

diff --git a/MAINTAINERS b/MAINTAINERS
index a3f0aa46d0d8..1e874fda810e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2135,6 +2135,7 @@ F:	Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
 F:	arch/arm/boot/dts/mstar-*
 F:	arch/arm/mach-mstar/
 F:	drivers/gpio/gpio-msc313.c
+F:	include/dt-bindings/clock/mstar-*
 F:	include/dt-bindings/gpio/msc313-gpio.h
 
 ARM/NEC MOBILEPRO 900/c MACHINE SUPPORT
diff --git a/include/dt-bindings/clock/mstar-msc313-mpll.h b/include/dt-bindings/clock/mstar-msc313-mpll.h
new file mode 100644
index 000000000000..1b30b02317b6
--- /dev/null
+++ b/include/dt-bindings/clock/mstar-msc313-mpll.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Output definitions for the MStar/SigmaStar MPLL
+ *
+ * Copyright (C) 2020 Daniel Palmer <daniel@thingy.jp>
+ */
+
+#ifndef _DT_BINDINGS_CLOCK_MSTAR_MSC313_MPLL_H
+#define _DT_BINDINGS_CLOCK_MSTAR_MSC313_MPLL_H
+
+#define MSTAR_MSC313_MPLL_DIV2	1
+#define MSTAR_MSC313_MPLL_DIV3	2
+#define MSTAR_MSC313_MPLL_DIV4	3
+#define MSTAR_MSC313_MPLL_DIV5	4
+#define MSTAR_MSC313_MPLL_DIV6	5
+#define MSTAR_MSC313_MPLL_DIV7	6
+#define MSTAR_MSC313_MPLL_DIV10	7
+
+#endif
-- 
2.29.2

[PATCH 2/6] dt-bindings: clk: mstar msc313 mpll binding description

[Daniel Palmer]

Add a binding description for the MStar/SigmaStar MPLL clock block.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 .../bindings/clock/mstar,msc313-mpll.yaml     | 58 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml

diff --git a/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml b/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
new file mode 100644
index 000000000000..9ddc1163b31b
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/mstar,msc313-mpll.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MStar/Sigmastar MSC313 MPLL
+
+maintainers:
+  - Daniel Palmer <daniel@thingy.jp>
+
+description: |
+  The MStar/SigmaStar MSC313 and later ARMv7 chips have an MPLL block that
+  takes the external xtal input and multiplies it to create a high
+  frequency clock and divides that down into a number of clocks that
+  peripherals use.
+
+properties:
+  compatible:
+    const: mstar,msc313-mpll
+
+  "#clock-cells":
+    const: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-output-names:
+    minItems: 8
+    maxItems: 8
+    description: |
+      This should provide a name for the internal PLL clock and then
+      a name for each of the divided outputs.
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - "#clock-cells"
+  - clocks
+  - clock-output-names
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    mpll@206000 {
+        compatible = "mstar,msc313-mpll";
+        reg = <0x206000 0x200>;
+        #clock-cells = <1>;
+        clocks = <&xtal>;
+        clock-output-names = "mpll", "mpll_div_2",
+                             "mpll_div_3", "mpll_div_4",
+                             "mpll_div_5", "mpll_div_6",
+                             "mpll_div_7", "mpll_div_10";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1e874fda810e..d1c98a6f9f24 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2131,6 +2131,7 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 W:	http://linux-chenxing.org/
 F:	Documentation/devicetree/bindings/arm/mstar/*
+F:	Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
 F:	Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
 F:	arch/arm/boot/dts/mstar-*
 F:	arch/arm/mach-mstar/
-- 
2.29.2

[PATCH 3/6] clk: mstar: MStar/SigmaStar MPLL driver

[Daniel Palmer]

This adds a basic driver for the MPLL block found in MStar/SigmaStar
ARMv7 SoCs.

Currently this driver is only good for calculating the rates of it's
outputs and the actual configuration must be done before the kernel
boots. Usually this is done even before u-boot starts.

This driver targets the MPLL block found in the MSC313/MSC313E but
there is no documentation this chip so the register descriptions for
the another MStar chip the MST786 were used as they seem to match.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 MAINTAINERS                         |   1 +
 drivers/clk/Kconfig                 |   1 +
 drivers/clk/Makefile                |   1 +
 drivers/clk/mstar/Kconfig           |   5 +
 drivers/clk/mstar/Makefile          |   6 +
 drivers/clk/mstar/clk-msc313-mpll.c | 177 ++++++++++++++++++++++++++++
 6 files changed, 191 insertions(+)
 create mode 100644 drivers/clk/mstar/Kconfig
 create mode 100644 drivers/clk/mstar/Makefile
 create mode 100644 drivers/clk/mstar/clk-msc313-mpll.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d1c98a6f9f24..1783022b4aa8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2135,6 +2135,7 @@ F:	Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
 F:	Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
 F:	arch/arm/boot/dts/mstar-*
 F:	arch/arm/mach-mstar/
+F:	drivers/clk/mstar/
 F:	drivers/gpio/gpio-msc313.c
 F:	include/dt-bindings/clock/mstar-*
 F:	include/dt-bindings/gpio/msc313-gpio.h
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index c715d4681a0b..a002f2605fa3 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -370,6 +370,7 @@ source "drivers/clk/ingenic/Kconfig"
 source "drivers/clk/keystone/Kconfig"
 source "drivers/clk/mediatek/Kconfig"
 source "drivers/clk/meson/Kconfig"
+source "drivers/clk/mstar/Kconfig"
 source "drivers/clk/mvebu/Kconfig"
 source "drivers/clk/qcom/Kconfig"
 source "drivers/clk/renesas/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index da8fcf147eb1..b758aae17ab8 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -124,3 +124,4 @@ endif
 obj-$(CONFIG_ARCH_ZX)			+= zte/
 obj-$(CONFIG_ARCH_ZYNQ)			+= zynq/
 obj-$(CONFIG_COMMON_CLK_ZYNQMP)         += zynqmp/
+obj-$(CONFIG_ARCH_MSTARV7)		+= mstar/
diff --git a/drivers/clk/mstar/Kconfig b/drivers/clk/mstar/Kconfig
new file mode 100644
index 000000000000..23765edde3af
--- /dev/null
+++ b/drivers/clk/mstar/Kconfig
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config MSTAR_MSC313_MPLL
+	bool
+	select REGMAP
+	select REGMAP_MMIO
diff --git a/drivers/clk/mstar/Makefile b/drivers/clk/mstar/Makefile
new file mode 100644
index 000000000000..f8dcd25ede1d
--- /dev/null
+++ b/drivers/clk/mstar/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for mstar specific clk
+#
+
+obj-$(CONFIG_MSTAR_MSC313_MPLL) += clk-msc313-mpll.o
diff --git a/drivers/clk/mstar/clk-msc313-mpll.c b/drivers/clk/mstar/clk-msc313-mpll.c
new file mode 100644
index 000000000000..c1e2fe0fc412
--- /dev/null
+++ b/drivers/clk/mstar/clk-msc313-mpll.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MStar MSC313 MPLL driver
+ *
+ * Copyright (C) 2020 Daniel Palmer <daniel@thingy.jp>
+ */
+
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/of_address.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define REG_CONFIG1	0x8
+#define REG_CONFIG2	0xc
+
+static const struct regmap_config msc313_mpll_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 16,
+	.reg_stride = 4,
+};
+
+static const struct reg_field config1_loop_div_first = REG_FIELD(REG_CONFIG1, 8, 9);
+static const struct reg_field config1_input_div_first = REG_FIELD(REG_CONFIG1, 4, 5);
+static const struct reg_field config2_output_div_first = REG_FIELD(REG_CONFIG2, 12, 13);
+static const struct reg_field config2_loop_div_second = REG_FIELD(REG_CONFIG2, 0, 7);
+
+static const unsigned int output_dividers[] = {
+	2, 3, 4, 5, 6, 7, 10
+};
+
+#define NUMOUTPUTS (ARRAY_SIZE(output_dividers) + 1)
+
+struct msc313_mpll {
+	struct clk_hw clk_hw;
+	struct regmap_field *input_div;
+	struct regmap_field *loop_div_first;
+	struct regmap_field *loop_div_second;
+	struct regmap_field *output_div;
+	struct clk_hw_onecell_data *clk_data;
+};
+
+#define to_mpll(_hw) container_of(_hw, struct msc313_mpll, clk_hw)
+#define to_divider_hw(_mpll, _which) _mpll->clk_data->hws[_which + 1]
+
+static unsigned long msc313_mpll_recalc_rate(struct clk_hw *hw,
+		unsigned long parent_rate)
+{
+	struct msc313_mpll *mpll = to_mpll(hw);
+	unsigned int input_div, output_div, loop_first, loop_second;
+	unsigned long output_rate;
+
+	regmap_field_read(mpll->input_div, &input_div);
+	regmap_field_read(mpll->output_div, &output_div);
+	regmap_field_read(mpll->loop_div_first, &loop_first);
+	regmap_field_read(mpll->loop_div_second, &loop_second);
+
+	output_rate = parent_rate / (1 << input_div);
+	output_rate *= (1 << loop_first) * max(loop_second, 1U);
+	output_rate /= max(output_div, 1U);
+
+	return output_rate;
+}
+
+static const struct clk_ops msc313_mpll_ops = {
+		.recalc_rate = msc313_mpll_recalc_rate,
+};
+
+static int msc313_mpll_probe(struct platform_device *pdev)
+{
+	void __iomem *base;
+	struct msc313_mpll *mpll;
+	struct clk_init_data clk_init;
+	struct device *dev = &pdev->dev;
+	struct regmap *regmap;
+	const char *parents[1], *outputnames[NUMOUTPUTS];
+	const int numparents = ARRAY_SIZE(parents);
+	int ret, i;
+
+	if (of_clk_parent_fill(dev->of_node, parents, numparents) != numparents)
+		return -EINVAL;
+
+	if (of_property_read_string_array(pdev->dev.of_node, "clock-output-names",
+			outputnames, NUMOUTPUTS) != NUMOUTPUTS)
+		return -EINVAL;
+
+	mpll = devm_kzalloc(dev, sizeof(*mpll), GFP_KERNEL);
+	if (!mpll)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	regmap = devm_regmap_init_mmio(dev, base, &msc313_mpll_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	mpll->input_div = devm_regmap_field_alloc(dev, regmap, config1_input_div_first);
+	if (IS_ERR(mpll->input_div))
+		return PTR_ERR(mpll->input_div);
+	mpll->output_div = devm_regmap_field_alloc(dev, regmap, config2_output_div_first);
+	if (IS_ERR(mpll->output_div))
+		return PTR_ERR(mpll->output_div);
+	mpll->loop_div_first = devm_regmap_field_alloc(dev, regmap, config1_loop_div_first);
+	if (IS_ERR(mpll->loop_div_first))
+		return PTR_ERR(mpll->loop_div_first);
+	mpll->loop_div_second = devm_regmap_field_alloc(dev, regmap, config2_loop_div_second);
+	if (IS_ERR(mpll->loop_div_second))
+		return PTR_ERR(mpll->loop_div_second);
+
+	mpll->clk_data = devm_kzalloc(dev, struct_size(mpll->clk_data, hws,
+			ARRAY_SIZE(output_dividers)), GFP_KERNEL);
+	if (!mpll->clk_data)
+		return -ENOMEM;
+
+	clk_init.name = outputnames[0];
+	clk_init.ops = &msc313_mpll_ops;
+	clk_init.num_parents = 1;
+	clk_init.parent_names = parents;
+	mpll->clk_hw.init = &clk_init;
+
+	ret = devm_clk_hw_register(dev, &mpll->clk_hw);
+	if (ret)
+		return ret;
+
+	mpll->clk_data->num = NUMOUTPUTS;
+	mpll->clk_data->hws[0] = &mpll->clk_hw;
+
+	for (i = 0; i < ARRAY_SIZE(output_dividers); i++) {
+		to_divider_hw(mpll, i) = clk_hw_register_fixed_factor(dev,
+				outputnames[i + 1], outputnames[0], 0, 1, output_dividers[i]);
+		if (IS_ERR(to_divider_hw(mpll, i))) {
+			ret = PTR_ERR(to_divider_hw(mpll, i));
+			goto unregister_dividers;
+		}
+	}
+
+	platform_set_drvdata(pdev, mpll);
+
+	return devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
+			mpll->clk_data);
+
+unregister_dividers:
+	for (i--; i >= 0; i--)
+		clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i));
+	return ret;
+}
+
+static int msc313_mpll_remove(struct platform_device *pdev)
+{
+	struct msc313_mpll *mpll = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(output_dividers); i++)
+		clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i));
+
+	return 0;
+}
+
+static const struct of_device_id msc313_mpll_of_match[] = {
+	{ .compatible = "mstar,msc313-mpll", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, msc313_mpll_of_match);
+
+static struct platform_driver msc313_mpll_driver = {
+	.driver = {
+		.name = "mstar-msc313-mpll",
+		.of_match_table = msc313_mpll_of_match,
+	},
+	.probe = msc313_mpll_probe,
+	.remove = msc313_mpll_remove,
+};
+builtin_platform_driver(msc313_mpll_driver);
-- 
2.29.2

[PATCH 4/6] ARM: mstar: Select MSTAR_MSC313_MPLL

[Daniel Palmer]

All of the ARCH_MSTARV7 chips have an MPLL as the source for
peripheral clocks so select MSTAR_MSC313_MPLL.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 arch/arm/mach-mstar/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-mstar/Kconfig b/arch/arm/mach-mstar/Kconfig
index 576d1ab293c8..cd300eeedc20 100644
--- a/arch/arm/mach-mstar/Kconfig
+++ b/arch/arm/mach-mstar/Kconfig
@@ -4,6 +4,7 @@ menuconfig ARCH_MSTARV7
 	select ARM_GIC
 	select ARM_HEAVY_MB
 	select MST_IRQ
+	select MSTAR_MSC313_MPLL
 	help
 	  Support for newer MStar/Sigmastar SoC families that are
 	  based on Armv7 cores like the Cortex A7 and share the same
-- 
2.29.2

[PATCH 5/6] ARM: mstar: Add the external clocks to the base dsti

[Daniel Palmer]

All of the currently known MStar/SigmaStar ARMv7 SoCs have an "xtal"
clock input that is usually 24MHz and an "RTC xtal" that is usually 32KHz.

The xtal input has to be connected to something so it's enabled by default.

The MSC313 and MSC313E do not bring the RTC clock input out to the pins
so it's impossible to connect it. The SSC8336 does bring the input
out to the pins but it's not always actually connected to something.

The RTC node needs to always be present because in the future the nodes
for the clock muxes will refer to it even if it's not usable.

The RTC node is disabled by default and should be enabled at the board
level if the RTC input is wired up.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 arch/arm/boot/dts/mstar-v7.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/mstar-v7.dtsi b/arch/arm/boot/dts/mstar-v7.dtsi
index 81369bc07f78..6749d421dbf4 100644
--- a/arch/arm/boot/dts/mstar-v7.dtsi
+++ b/arch/arm/boot/dts/mstar-v7.dtsi
@@ -46,6 +46,21 @@ pmu: pmu {
 		interrupt-affinity = <&cpu0>;
 	};
 
+	clocks: clocks {
+		xtal: xtal {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <24000000>;
+		};
+
+		rtc_xtal: rtc_xtal {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <32768>;
+			status = "disabled";
+		};
+	};
+
 	soc: soc {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.29.2

[PATCH 6/6] ARM: mstar: Add mpll to base dtsi

[Daniel Palmer]

All of the currently known MStar/SigmaStar ARMv7 SoCs have at least
one MPLL and it seems to always be at the same place so add it to
the base dtsi.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 arch/arm/boot/dts/mstar-v7.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/mstar-v7.dtsi b/arch/arm/boot/dts/mstar-v7.dtsi
index 6749d421dbf4..07fc46c7b4d4 100644
--- a/arch/arm/boot/dts/mstar-v7.dtsi
+++ b/arch/arm/boot/dts/mstar-v7.dtsi
@@ -6,6 +6,7 @@
 
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/clock/mstar-msc313-mpll.h>
 
 / {
 	#address-cells = <1>;
@@ -124,6 +125,17 @@ l3bridge: l3bridge@204400 {
 				reg = <0x204400 0x200>;
 			};
 
+			mpll: mpll@206000 {
+				compatible = "mstar,msc313-mpll";
+				#clock-cells = <1>;
+				reg = <0x206000 0x200>;
+				clocks = <&xtal>;
+				clock-output-names = "mpll", "mpll_div_2",
+						     "mpll_div_3", "mpll_div_4",
+						     "mpll_div_5", "mpll_div_6",
+						     "mpll_div_7", "mpll_div_10";
+			};
+
 			gpio: gpio@207800 {
 				#gpio-cells = <2>;
 				reg = <0x207800 0x200>;
-- 
2.29.2

Re: [PATCH 1/6] dt-bindings: clk: mstar msc313 mpll binding header

[Rob Herring]

On Sat, 14 Nov 2020 22:50:39 +0900, Daniel Palmer wrote:
> Simple header to document the relationship between the MPLL outputs
> and which divider they come from.
> 
> Output 0 is missing because it should not be consumed.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> ---
>  MAINTAINERS                                   |  1 +
>  include/dt-bindings/clock/mstar-msc313-mpll.h | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
>  create mode 100644 include/dt-bindings/clock/mstar-msc313-mpll.h
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/6] dt-bindings: clk: mstar msc313 mpll binding description

[Rob Herring]

On Sat, 14 Nov 2020 22:50:40 +0900, Daniel Palmer wrote:
> Add a binding description for the MStar/SigmaStar MPLL clock block.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> ---
>  .../bindings/clock/mstar,msc313-mpll.yaml     | 58 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/6] dt-bindings: clk: mstar msc313 mpll binding description

[Stephen Boyd]

Quoting Daniel Palmer (2020-11-14 05:50:40)
> Add a binding description for the MStar/SigmaStar MPLL clock block.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> ---
>  .../bindings/clock/mstar,msc313-mpll.yaml     | 58 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml b/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
> new file mode 100644
> index 000000000000..9ddc1163b31b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/mstar,msc313-mpll.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MStar/Sigmastar MSC313 MPLL
> +
> +maintainers:
> +  - Daniel Palmer <daniel@thingy.jp>
> +
> +description: |
> +  The MStar/SigmaStar MSC313 and later ARMv7 chips have an MPLL block that
> +  takes the external xtal input and multiplies it to create a high
> +  frequency clock and divides that down into a number of clocks that
> +  peripherals use.
> +
> +properties:
> +  compatible:
> +    const: mstar,msc313-mpll
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-output-names:
> +    minItems: 8
> +    maxItems: 8
> +    description: |
> +      This should provide a name for the internal PLL clock and then
> +      a name for each of the divided outputs.

Is this necessary?

> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - clocks
> +  - clock-output-names
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    mpll@206000 {
> +        compatible = "mstar,msc313-mpll";
> +        reg = <0x206000 0x200>;
> +        #clock-cells = <1>;
> +        clocks = <&xtal>;
> +        clock-output-names = "mpll", "mpll_div_2",
> +                             "mpll_div_3", "mpll_div_4",
> +                             "mpll_div_5", "mpll_div_6",
> +                             "mpll_div_7", "mpll_div_10";

It looks like it can be derived in the driver.

Re: [PATCH 3/6] clk: mstar: MStar/SigmaStar MPLL driver

[Stephen Boyd]

Quoting Daniel Palmer (2020-11-14 05:50:41)
>  F:     include/dt-bindings/gpio/msc313-gpio.h
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index c715d4681a0b..a002f2605fa3 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -370,6 +370,7 @@ source "drivers/clk/ingenic/Kconfig"
>  source "drivers/clk/keystone/Kconfig"
>  source "drivers/clk/mediatek/Kconfig"
>  source "drivers/clk/meson/Kconfig"
> +source "drivers/clk/mstar/Kconfig"
>  source "drivers/clk/mvebu/Kconfig"
>  source "drivers/clk/qcom/Kconfig"
>  source "drivers/clk/renesas/Kconfig"

This looks good.

> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index da8fcf147eb1..b758aae17ab8 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -124,3 +124,4 @@ endif
>  obj-$(CONFIG_ARCH_ZX)                  += zte/
>  obj-$(CONFIG_ARCH_ZYNQ)                        += zynq/
>  obj-$(CONFIG_COMMON_CLK_ZYNQMP)         += zynqmp/
> +obj-$(CONFIG_ARCH_MSTARV7)             += mstar/

This is in the wrong place. It looks to be sorted based on the path
name, so mstar/ comes much earlier in this file.

> diff --git a/drivers/clk/mstar/Kconfig b/drivers/clk/mstar/Kconfig
> new file mode 100644
> index 000000000000..23765edde3af
> --- /dev/null
> +++ b/drivers/clk/mstar/Kconfig
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config MSTAR_MSC313_MPLL
> +       bool
> +       select REGMAP
> +       select REGMAP_MMIO
> diff --git a/drivers/clk/mstar/Makefile b/drivers/clk/mstar/Makefile
> new file mode 100644
> index 000000000000..f8dcd25ede1d
> --- /dev/null
> +++ b/drivers/clk/mstar/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for mstar specific clk
> +#
> +
> +obj-$(CONFIG_MSTAR_MSC313_MPLL) += clk-msc313-mpll.o
> diff --git a/drivers/clk/mstar/clk-msc313-mpll.c b/drivers/clk/mstar/clk-msc313-mpll.c
> new file mode 100644
> index 000000000000..c1e2fe0fc412
> --- /dev/null
> +++ b/drivers/clk/mstar/clk-msc313-mpll.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MStar MSC313 MPLL driver
> + *
> + * Copyright (C) 2020 Daniel Palmer <daniel@thingy.jp>
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>

Please remove unused includes

> +#include <linux/of_address.h>
> +#include <linux/module.h>

Isn't it builtin? This include should be removed.

> +#include <linux/regmap.h>
> +
> +#define REG_CONFIG1    0x8
> +#define REG_CONFIG2    0xc
> +
> +static const struct regmap_config msc313_mpll_regmap_config = {
> +       .reg_bits = 16,
> +       .val_bits = 16,
> +       .reg_stride = 4,
> +};
> +
> +static const struct reg_field config1_loop_div_first = REG_FIELD(REG_CONFIG1, 8, 9);
> +static const struct reg_field config1_input_div_first = REG_FIELD(REG_CONFIG1, 4, 5);
> +static const struct reg_field config2_output_div_first = REG_FIELD(REG_CONFIG2, 12, 13);
> +static const struct reg_field config2_loop_div_second = REG_FIELD(REG_CONFIG2, 0, 7);
> +
> +static const unsigned int output_dividers[] = {
> +       2, 3, 4, 5, 6, 7, 10
> +};
> +
> +#define NUMOUTPUTS (ARRAY_SIZE(output_dividers) + 1)
> +
> +struct msc313_mpll {
> +       struct clk_hw clk_hw;
> +       struct regmap_field *input_div;
> +       struct regmap_field *loop_div_first;
> +       struct regmap_field *loop_div_second;
> +       struct regmap_field *output_div;
> +       struct clk_hw_onecell_data *clk_data;
> +};
> +
> +#define to_mpll(_hw) container_of(_hw, struct msc313_mpll, clk_hw)
> +#define to_divider_hw(_mpll, _which) _mpll->clk_data->hws[_which + 1]

I'd rather not have this macro. It's confusing given that to_foo()
macros are usually a container_of() invocation. Given that it's only
used in the registration/unregistration loops please inline it and use a
local variable.

> +
> +static unsigned long msc313_mpll_recalc_rate(struct clk_hw *hw,
> +               unsigned long parent_rate)
> +{
> +       struct msc313_mpll *mpll = to_mpll(hw);
> +       unsigned int input_div, output_div, loop_first, loop_second;
> +       unsigned long output_rate;
> +
> +       regmap_field_read(mpll->input_div, &input_div);
> +       regmap_field_read(mpll->output_div, &output_div);
> +       regmap_field_read(mpll->loop_div_first, &loop_first);
> +       regmap_field_read(mpll->loop_div_second, &loop_second);
> +
> +       output_rate = parent_rate / (1 << input_div);
> +       output_rate *= (1 << loop_first) * max(loop_second, 1U);
> +       output_rate /= max(output_div, 1U);
> +
> +       return output_rate;
> +}
> +
> +static const struct clk_ops msc313_mpll_ops = {
> +               .recalc_rate = msc313_mpll_recalc_rate,

Weird double indent here.

> +};
> +
> +static int msc313_mpll_probe(struct platform_device *pdev)
> +{
> +       void __iomem *base;
> +       struct msc313_mpll *mpll;
> +       struct clk_init_data clk_init;
> +       struct device *dev = &pdev->dev;
> +       struct regmap *regmap;
> +       const char *parents[1], *outputnames[NUMOUTPUTS];
> +       const int numparents = ARRAY_SIZE(parents);
> +       int ret, i;
> +
> +       if (of_clk_parent_fill(dev->of_node, parents, numparents) != numparents)
> +               return -EINVAL;
> +
> +       if (of_property_read_string_array(pdev->dev.of_node, "clock-output-names",

Hopefully this isn't required.

> +                       outputnames, NUMOUTPUTS) != NUMOUTPUTS)
> +               return -EINVAL;
> +
> +       mpll = devm_kzalloc(dev, sizeof(*mpll), GFP_KERNEL);
> +       if (!mpll)
> +               return -ENOMEM;
> +
> +       base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       regmap = devm_regmap_init_mmio(dev, base, &msc313_mpll_regmap_config);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       mpll->input_div = devm_regmap_field_alloc(dev, regmap, config1_input_div_first);
> +       if (IS_ERR(mpll->input_div))
> +               return PTR_ERR(mpll->input_div);
> +       mpll->output_div = devm_regmap_field_alloc(dev, regmap, config2_output_div_first);
> +       if (IS_ERR(mpll->output_div))
> +               return PTR_ERR(mpll->output_div);
> +       mpll->loop_div_first = devm_regmap_field_alloc(dev, regmap, config1_loop_div_first);
> +       if (IS_ERR(mpll->loop_div_first))
> +               return PTR_ERR(mpll->loop_div_first);
> +       mpll->loop_div_second = devm_regmap_field_alloc(dev, regmap, config2_loop_div_second);
> +       if (IS_ERR(mpll->loop_div_second))
> +               return PTR_ERR(mpll->loop_div_second);
> +
> +       mpll->clk_data = devm_kzalloc(dev, struct_size(mpll->clk_data, hws,
> +                       ARRAY_SIZE(output_dividers)), GFP_KERNEL);
> +       if (!mpll->clk_data)
> +               return -ENOMEM;
> +
> +       clk_init.name = outputnames[0];
> +       clk_init.ops = &msc313_mpll_ops;
> +       clk_init.num_parents = 1;
> +       clk_init.parent_names = parents;
> +       mpll->clk_hw.init = &clk_init;
> +
> +       ret = devm_clk_hw_register(dev, &mpll->clk_hw);
> +       if (ret)
> +               return ret;
> +
> +       mpll->clk_data->num = NUMOUTPUTS;
> +       mpll->clk_data->hws[0] = &mpll->clk_hw;
> +
> +       for (i = 0; i < ARRAY_SIZE(output_dividers); i++) {
> +               to_divider_hw(mpll, i) = clk_hw_register_fixed_factor(dev,
> +                               outputnames[i + 1], outputnames[0], 0, 1, output_dividers[i]);
> +               if (IS_ERR(to_divider_hw(mpll, i))) {
> +                       ret = PTR_ERR(to_divider_hw(mpll, i));
> +                       goto unregister_dividers;
> +               }
> +       }
> +
> +       platform_set_drvdata(pdev, mpll);
> +
> +       return devm_of_clk_add_hw_provider(&pdev->dev, of_clk_hw_onecell_get,
> +                       mpll->clk_data);
> +
> +unregister_dividers:
> +       for (i--; i >= 0; i--)
> +               clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i));
> +       return ret;
> +}
> +
> +static int msc313_mpll_remove(struct platform_device *pdev)
> +{
> +       struct msc313_mpll *mpll = platform_get_drvdata(pdev);
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(output_dividers); i++)
> +               clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i));

Maybe add a devm_ for this if it doesn't exist.

> +
> +       return 0;
> +}
> +
> +static const struct of_device_id msc313_mpll_of_match[] = {
> +       { .compatible = "mstar,msc313-mpll", },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, msc313_mpll_of_match);

Drop this. It isn't a module.

> +
> +static struct platform_driver msc313_mpll_driver = {
> +       .driver = {
> +               .name = "mstar-msc313-mpll",
> +               .of_match_table = msc313_mpll_of_match,
> +       },
> +       .probe = msc313_mpll_probe,
> +       .remove = msc313_mpll_remove,
> +};
> +builtin_platform_driver(msc313_mpll_driver);

Re: [PATCH 2/6] dt-bindings: clk: mstar msc313 mpll binding description

[Daniel Palmer]

Hi Stephen,

On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <sboyd@kernel.org> wrote:
> > +  clock-output-names:
> > +    minItems: 8
> > +    maxItems: 8
> > +    description: |
> > +      This should provide a name for the internal PLL clock and then
> > +      a name for each of the divided outputs.
>
> Is this necessary?

I found without the names specified in the dt probing of muxes that
depend on the outputs but appear earlier didn't work.
Also this same PLL layout seems to be used in some other places so
eventually I was thinking this driver would get used for those PLLs
with different output names.

Thanks,

Daniel

Re: [PATCH 3/6] clk: mstar: MStar/SigmaStar MPLL driver

[Daniel Palmer]

Hi Stephen,

On Sun, 20 Dec 2020 at 13:36, Stephen Boyd <sboyd@kernel.org> wrote:
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index da8fcf147eb1..b758aae17ab8 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -124,3 +124,4 @@ endif
> >  obj-$(CONFIG_ARCH_ZX)                  += zte/
> >  obj-$(CONFIG_ARCH_ZYNQ)                        += zynq/
> >  obj-$(CONFIG_COMMON_CLK_ZYNQMP)         += zynqmp/
> > +obj-$(CONFIG_ARCH_MSTARV7)             += mstar/
>
> This is in the wrong place. It looks to be sorted based on the path
> name, so mstar/ comes much earlier in this file.

Noted. I'll fix this.

> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
>
> Please remove unused includes
>
> > +#include <linux/of_address.h>
> > +#include <linux/module.h>
>
> Isn't it builtin? This include should be removed.

Yes. Sorry it was originally a module and there are some leftovers I
didn't clean up when I changed it to builtin.

> > +#define to_mpll(_hw) container_of(_hw, struct msc313_mpll, clk_hw)
> > +#define to_divider_hw(_mpll, _which) _mpll->clk_data->hws[_which + 1]
>
> I'd rather not have this macro. It's confusing given that to_foo()
> macros are usually a container_of() invocation. Given that it's only
> used in the registration/unregistration loops please inline it and use a
> local variable.
>

Ok I'll rework that.

> > +
> > +static int msc313_mpll_remove(struct platform_device *pdev)
> > +{
> > +       struct msc313_mpll *mpll = platform_get_drvdata(pdev);
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(output_dividers); i++)
> > +               clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i));
>
> Maybe add a devm_ for this if it doesn't exist.

I did think about adding this. Would I need to do that in a separate
series or would it be ok to roll it into this one?

Thanks,

Daniel

Re: [PATCH 3/6] clk: mstar: MStar/SigmaStar MPLL driver

[Stephen Boyd]

Quoting Daniel Palmer (2020-12-19 22:42:40)
> > > +
> > > +static int msc313_mpll_remove(struct platform_device *pdev)
> > > +{
> > > +       struct msc313_mpll *mpll = platform_get_drvdata(pdev);
> > > +       int i;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(output_dividers); i++)
> > > +               clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i));
> >
> > Maybe add a devm_ for this if it doesn't exist.
> 
> I did think about adding this. Would I need to do that in a separate
> series or would it be ok to roll it into this one?
> 

Can be part of the same series.

Re: [PATCH 2/6] dt-bindings: clk: mstar msc313 mpll binding description

[Stephen Boyd]

Quoting Daniel Palmer (2020-12-19 22:35:41)
> Hi Stephen,
> 
> On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <sboyd@kernel.org> wrote:
> > > +  clock-output-names:
> > > +    minItems: 8
> > > +    maxItems: 8
> > > +    description: |
> > > +      This should provide a name for the internal PLL clock and then
> > > +      a name for each of the divided outputs.
> >
> > Is this necessary?
> 
> I found without the names specified in the dt probing of muxes that
> depend on the outputs but appear earlier didn't work.
> Also this same PLL layout seems to be used in some other places so
> eventually I was thinking this driver would get used for those PLLs
> with different output names.

Still seems like it could be auto-generated based on dev_name() +
number. Now that we have a way to specify clk parents via the clocks
property in DT (without any clock-names required) we should be able to
avoid needing clock-output-names in general.

Re: [PATCH 3/6] clk: mstar: MStar/SigmaStar MPLL driver

[Daniel Palmer]

Hi Stephen,

On Mon, 21 Dec 2020 at 03:43, Stephen Boyd <sboyd@kernel.org> wrote:
> Can be part of the same series.

Ok. I have added a small patch that adds devm_clk_hw_register_fixed_factor().
I'll send that in the v2 once the clock names issue is resolved.

Thanks,

Daniel

Re: [PATCH 2/6] dt-bindings: clk: mstar msc313 mpll binding description

[Daniel Palmer]

Hi Stephen,

On Mon, 21 Dec 2020 at 03:44, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Daniel Palmer (2020-12-19 22:35:41)
> > Hi Stephen,
> >
> > On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <sboyd@kernel.org> wrote:
> > > > +  clock-output-names:
> > > > +    minItems: 8
> > > > +    maxItems: 8
> > > > +    description: |
> > > > +      This should provide a name for the internal PLL clock and then
> > > > +      a name for each of the divided outputs.
> > >
> > > Is this necessary?
> >
> > I found without the names specified in the dt probing of muxes that
> > depend on the outputs but appear earlier didn't work.
> > Also this same PLL layout seems to be used in some other places so
> > eventually I was thinking this driver would get used for those PLLs
> > with different output names.
>
> Still seems like it could be auto-generated based on dev_name() +
> number.

At one point I had something similar to that where the output names
were generated at probe.
Without the clock outputs listed in the device tree clock muxes that
source clocks from the mpll couldn't probe properly as they couldn't
look up all of their parents if they probed before the mpll.
Maybe I'm doing something wrong there? I couldn't find a way to always
resolve all of the parents or defer the probe of the muxes until the
mpll clocks are registered.

Cheers,

Daniel

Re: [PATCH 2/6] dt-bindings: clk: mstar msc313 mpll binding description

[Stephen Boyd]

Quoting Daniel Palmer (2020-12-21 00:51:56)
> Hi Stephen,
> 
> On Mon, 21 Dec 2020 at 03:44, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Daniel Palmer (2020-12-19 22:35:41)
> > > Hi Stephen,
> > >
> > > On Sun, 20 Dec 2020 at 12:39, Stephen Boyd <sboyd@kernel.org> wrote:
> > > > > +  clock-output-names:
> > > > > +    minItems: 8
> > > > > +    maxItems: 8
> > > > > +    description: |
> > > > > +      This should provide a name for the internal PLL clock and then
> > > > > +      a name for each of the divided outputs.
> > > >
> > > > Is this necessary?
> > >
> > > I found without the names specified in the dt probing of muxes that
> > > depend on the outputs but appear earlier didn't work.
> > > Also this same PLL layout seems to be used in some other places so
> > > eventually I was thinking this driver would get used for those PLLs
> > > with different output names.
> >
> > Still seems like it could be auto-generated based on dev_name() +
> > number.
> 
> At one point I had something similar to that where the output names
> were generated at probe.
> Without the clock outputs listed in the device tree clock muxes that
> source clocks from the mpll couldn't probe properly as they couldn't
> look up all of their parents if they probed before the mpll.
> Maybe I'm doing something wrong there? I couldn't find a way to always
> resolve all of the parents or defer the probe of the muxes until the
> mpll clocks are registered.
> 

The child clks should be using clk_parent_data to point to the parent
clks through DT. That way the name of the clk doesn't matter except for
debug purposes.

Re: [PATCH 2/6] dt-bindings: clk: mstar msc313 mpll binding description

[Daniel Palmer]

Hi Stephen,

On Wed, 10 Feb 2021 at 11:29, Stephen Boyd <sboyd@kernel.org> wrote:
> The child clks should be using clk_parent_data to point to the parent
> clks through DT. That way the name of the clk doesn't matter except for
> debug purposes.

I think I get it now. I was using of_clk_parent_fill() to get the
parent clocks sourced
from the mpll but I seems like I should be filling out an array of
struct clk_parent_data
with the indices of the parents and using
clk_register_composite_pdata() etc instead.

Thanks!

Daniel

Re: [PATCH 2/6] dt-bindings: clk: mstar msc313 mpll binding description

[Stephen Boyd]

Quoting Daniel Palmer (2021-02-10 18:28:40)
> Hi Stephen,
> 
> On Wed, 10 Feb 2021 at 11:29, Stephen Boyd <sboyd@kernel.org> wrote:
> > The child clks should be using clk_parent_data to point to the parent
> > clks through DT. That way the name of the clk doesn't matter except for
> > debug purposes.
> 
> I think I get it now. I was using of_clk_parent_fill() to get the
> parent clocks sourced
> from the mpll but I seems like I should be filling out an array of
> struct clk_parent_data
> with the indices of the parents and using
> clk_register_composite_pdata() etc instead.
> 

Yes that's the idea. Thanks!