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/[email protected]/
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
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 <[email protected]>
---
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 <[email protected]>
+ */
+
+#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
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 <[email protected]>
---
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 <[email protected]>
+ */
+
+#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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> + */
> +
> +#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);
Hi Stephen,
On Sun, 20 Dec 2020 at 13:36, Stephen Boyd <[email protected]> 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
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.
Hi Stephen,
On Mon, 21 Dec 2020 at 03:43, Stephen Boyd <[email protected]> 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