[PATCH 0/5] clk: qcom: msm8996: add support for the CBF clock

[PATCH 0/5] clk: qcom: msm8996: add support for the CBF clock

[Dmitry Baryshkov]

On MSM8996 two CPU clusters are interconnected using the Core Bus
Fabric (CBF). In order for the CPU clusters to function properly, it
should be clocked following the core's frequencies to provide adequate
bandwidth. On the other hand the CBF's clock rate can be used by other
drivers (e.g. by the pending SPDM driver to provide input on the CPU
performance).

Thus register CBF as a clock (required for CPU to boot) and add a tiny
interconnect layer on top of it to let cpufreq/opp scale the CBF clock.

Dependencies: [1], [2]

[1] https://lore.kernel.org/linux-arm-msm/20230111191453.2509468-1-dmitry.baryshkov@linaro.org/
[2] https://lore.kernel.org/linux-arm-msm/20230111191634.2509616-1-dmitry.baryshkov@linaro.org/

Dmitry Baryshkov (5):
  dt-bindings: clock: qcom,msm8996-cbf: Describe the MSM8996 CBF clock
    controller
  clk: qcom: add msm8996 Core Bus Framework (CBF) support
  clk: qcom: cbf-msm8996: scale CBF clock according to the CPUfreq
  arm64: dts: qcom: msm8996: add CBF device entry
  arm64: dts: qcom: msm8996: scale CBF clock according to the CPUfreq

 .../bindings/clock/qcom,msm8996-cbf.yaml      |  53 ++
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |  57 +++
 drivers/clk/qcom/Makefile                     |   2 +-
 drivers/clk/qcom/clk-cbf-8996.c               | 457 ++++++++++++++++++
 4 files changed, 568 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
 create mode 100644 drivers/clk/qcom/clk-cbf-8996.c

-- 
2.30.2

[PATCH 1/5] dt-bindings: clock: qcom,msm8996-cbf: Describe the MSM8996 CBF clock controller

[Dmitry Baryshkov]

MSM8996 Core Bus Fabric (CBF) clock controller clocks an interconnect
between two CPU clusters. The CBF clock should follow the CPU
frequencies to provide enough bandwidth between clusters. Thus a single
driver implements both a clock and an interconnect to set the clock
rate.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../bindings/clock/qcom,msm8996-cbf.yaml      | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml

diff --git a/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml b/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
new file mode 100644
index 000000000000..2080e1743b2e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/qcom,msm8996-cbf.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm MSM8996 Core Bus Fabric (CBF) clock controller
+
+maintainers:
+  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+
+description: >
+  The clock controller for the Qualcomm MSM8996 CBF clock, which drives the
+  interconnect between two CPU clusters.
+
+properties:
+  compatible:
+    const: qcom,msm8996-cbf
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: XO source
+      - description: SYS APCS AUX clock
+
+  '#clock-cells':
+    const: 0
+
+  '#interconnect-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - '#clock-cells'
+  - '#interconnect-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,rpmcc.h>
+    clock-controller@9a11000 {
+        compatible = "qcom,msm8996-cbf";
+        reg = <0x09a11000 0x10000>;
+        clocks = <&rpmcc RPM_SMD_BB_CLK1>, <&apcs_glb>;
+        #clock-cells = <0>;
+        #interconnect-cells = <1>;
+    };
+...
-- 
2.30.2

[PATCH 2/5] clk: qcom: add msm8996 Core Bus Framework (CBF) support

[Dmitry Baryshkov]

Add CBF clock driver as a part of MSM8996 CPU clocks. Significantly
based on AngeloGioacchino del Regno's work at [1].

The CBF is an interconnect between two CPU clusters, setting it up
properly is required for booting the MSM8996 with all four cores
enabled.

[1] https://github.com/sonyxperiadev/kernel/blob/aosp/LE.UM.2.3.2.r1.4/drivers/clk/qcom/clk-cpu-8996.c

Co-developed-by: Konrad Dybcio <konrad.dybcio@somainline.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/Makefile       |   2 +-
 drivers/clk/qcom/clk-cbf-8996.c | 318 ++++++++++++++++++++++++++++++++
 2 files changed, 319 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/qcom/clk-cbf-8996.c

diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index ca2f586edb3e..d6ecd2bf7040 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -52,7 +52,7 @@ obj-$(CONFIG_MSM_MMCC_8998) += mmcc-msm8998.o
 obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
 obj-$(CONFIG_QCOM_A7PLL) += a7-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
-obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += apcs-msm8996.o clk-cpu-8996.o
+obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += apcs-msm8996.o clk-cpu-8996.o clk-cbf-8996.o
 obj-$(CONFIG_QCOM_CLK_APCS_SDX55) += apcs-sdx55.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
 obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
new file mode 100644
index 000000000000..bdd4f8b48a7e
--- /dev/null
+++ b/drivers/clk/qcom/clk-cbf-8996.c
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022, Linaro Ltd.
+ */
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "clk-alpha-pll.h"
+#include "clk-regmap.h"
+
+/* Need to match the order of clocks in DT binding */
+enum {
+	DT_XO,
+	DT_APCS_AUX,
+};
+
+enum {
+	CBF_XO_INDEX,
+	CBF_PLL_INDEX,
+	CBF_DIV_INDEX,
+	CBF_APCS_AUX_INDEX,
+};
+
+#define DIV_THRESHOLD		600000000
+
+#define CBF_MUX_OFFSET		0x18
+#define CBF_MUX_PARENT_MASK		GENMASK(1, 0)
+#define CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK GENMASK(5, 4)
+#define CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL \
+	FIELD_PREP(CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK, 0x03)
+#define CBF_MUX_AUTO_CLK_SEL_BIT	BIT(6)
+
+#define CBF_PLL_OFFSET 0xf000
+
+static const u8 cbf_pll_regs[PLL_OFF_MAX_REGS] = {
+	[PLL_OFF_L_VAL] = 0x08,
+	[PLL_OFF_ALPHA_VAL] = 0x10,
+	[PLL_OFF_USER_CTL] = 0x18,
+	[PLL_OFF_CONFIG_CTL] = 0x20,
+	[PLL_OFF_CONFIG_CTL_U] = 0x24,
+	[PLL_OFF_TEST_CTL] = 0x30,
+	[PLL_OFF_TEST_CTL_U] = 0x34,
+	[PLL_OFF_STATUS] = 0x28,
+};
+
+static const struct alpha_pll_config cbfpll_config = {
+	.l = 72,
+	.config_ctl_val = 0x200d4828,
+	.config_ctl_hi_val = 0x006,
+	.test_ctl_val = 0x1c000000,
+	.test_ctl_hi_val = 0x00004000,
+	.pre_div_mask = BIT(12),
+	.post_div_mask = 0x3 << 8,
+	.post_div_val = 0x1 << 8,
+	.main_output_mask = BIT(0),
+	.early_output_mask = BIT(3),
+};
+
+static struct clk_alpha_pll cbf_pll = {
+	.offset = CBF_PLL_OFFSET,
+	.regs = cbf_pll_regs,
+	.flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE,
+	.clkr.hw.init = &(struct clk_init_data){
+		.name = "cbf_pll",
+		.parent_data = (const struct clk_parent_data[]) {
+			{ .index = DT_XO, },
+		},
+		.num_parents = 1,
+		.ops = &clk_alpha_pll_hwfsm_ops,
+	},
+};
+
+static struct clk_fixed_factor cbf_pll_postdiv = {
+	.mult = 1,
+	.div = 2,
+	.hw.init = &(struct clk_init_data){
+		.name = "cbf_pll_postdiv",
+		.parent_data = &(const struct clk_parent_data){
+			.hw = &cbf_pll.clkr.hw
+		},
+		.num_parents = 1,
+		.ops = &clk_fixed_factor_ops,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static const struct clk_parent_data cbf_mux_parent_data[] = {
+	{ .index = DT_XO },
+	{ .hw = &cbf_pll.clkr.hw },
+	{ .hw = &cbf_pll_postdiv.hw },
+	{ .index = DT_APCS_AUX },
+};
+
+struct clk_cbf_8996_mux {
+	u32 reg;
+	struct notifier_block nb;
+	struct clk_regmap clkr;
+};
+
+static struct clk_cbf_8996_mux *to_clk_cbf_8996_mux(struct clk_regmap *clkr)
+{
+	return container_of(clkr, struct clk_cbf_8996_mux, clkr);
+}
+
+static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
+			       void *data);
+
+static u8 clk_cbf_8996_mux_get_parent(struct clk_hw *hw)
+{
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	struct clk_cbf_8996_mux *mux = to_clk_cbf_8996_mux(clkr);
+	u32 val;
+
+	regmap_read(clkr->regmap, mux->reg, &val);
+
+	return FIELD_GET(CBF_MUX_PARENT_MASK, val);
+}
+
+static int clk_cbf_8996_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	struct clk_cbf_8996_mux *mux = to_clk_cbf_8996_mux(clkr);
+	u32 val;
+
+	val = FIELD_PREP(CBF_MUX_PARENT_MASK, index);
+
+	return regmap_update_bits(clkr->regmap, mux->reg, CBF_MUX_PARENT_MASK, val);
+}
+
+static int clk_cbf_8996_mux_determine_rate(struct clk_hw *hw,
+					   struct clk_rate_request *req)
+{
+	struct clk_hw *parent;
+
+	if (req->rate < (DIV_THRESHOLD / 2))
+		return -EINVAL;
+
+	if (req->rate < DIV_THRESHOLD)
+		parent = clk_hw_get_parent_by_index(hw, CBF_DIV_INDEX);
+	else
+		parent = clk_hw_get_parent_by_index(hw, CBF_PLL_INDEX);
+
+	if (!parent)
+		return -EINVAL;
+
+	req->best_parent_rate = clk_hw_round_rate(parent, req->rate);
+	req->best_parent_hw = parent;
+
+	return 0;
+}
+
+static const struct clk_ops clk_cbf_8996_mux_ops = {
+	.set_parent = clk_cbf_8996_mux_set_parent,
+	.get_parent = clk_cbf_8996_mux_get_parent,
+	.determine_rate = clk_cbf_8996_mux_determine_rate,
+};
+
+static struct clk_cbf_8996_mux cbf_mux = {
+	.reg = CBF_MUX_OFFSET,
+	.nb.notifier_call = cbf_clk_notifier_cb,
+	.clkr.hw.init = &(struct clk_init_data) {
+		.name = "cbf_mux",
+		.parent_data = cbf_mux_parent_data,
+		.num_parents = ARRAY_SIZE(cbf_mux_parent_data),
+		.ops = &clk_cbf_8996_mux_ops,
+		/* CPU clock is critical and should never be gated */
+		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
+	},
+};
+
+static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
+			       void *data)
+{
+	struct clk_notifier_data *cnd = data;
+	int ret;
+
+	switch (event) {
+	case PRE_RATE_CHANGE:
+		/*
+		 * Avoid overvolting. clk_core_set_rate_nolock() walks from top
+		 * to bottom, so it will change the rate of the PLL before
+		 * chaging the parent of PMUX. This can result in pmux getting
+		 * clocked twice the expected rate.
+		 *
+		 * Manually switch to PLL/2 here.
+		 */
+		if (cnd->old_rate > DIV_THRESHOLD &&
+		    cnd->new_rate < DIV_THRESHOLD)
+			clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_DIV_INDEX);
+		break;
+	case ABORT_RATE_CHANGE:
+		/* Revert manual change */
+		if (cnd->new_rate < DIV_THRESHOLD &&
+		    cnd->old_rate > DIV_THRESHOLD)
+			clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_PLL_INDEX);
+		break;
+	default:
+		ret = 0;
+		break;
+	}
+
+	return notifier_from_errno(ret);
+};
+
+static struct clk_hw *cbf_msm8996_hw_clks[] = {
+	&cbf_pll_postdiv.hw,
+};
+
+static struct clk_regmap *cbf_msm8996_clks[] = {
+	&cbf_pll.clkr,
+	&cbf_mux.clkr,
+};
+
+static const struct regmap_config cbf_msm8996_regmap_config = {
+	.reg_bits		= 32,
+	.reg_stride		= 4,
+	.val_bits		= 32,
+	.max_register		= 0x10000,
+	.fast_io		= true,
+	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
+};
+
+static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
+{
+	void __iomem *base;
+	struct regmap *regmap;
+	struct device *dev = &pdev->dev;
+	int i, ret;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	regmap = devm_regmap_init_mmio(dev, base, &cbf_msm8996_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	/* Select GPLL0 for 300MHz for the CBF clock */
+	regmap_write(regmap, CBF_MUX_OFFSET, 0x3);
+
+	/* Ensure write goes through before PLLs are reconfigured */
+	udelay(5);
+
+	/* Set the auto clock sel always-on source to GPLL0/2 (300MHz) */
+	regmap_update_bits(regmap, CBF_MUX_OFFSET,
+			   CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK,
+			   CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL);
+
+	clk_alpha_pll_configure(&cbf_pll, regmap, &cbfpll_config);
+
+	/* Wait for PLL(s) to lock */
+        udelay(50);
+
+	/* Enable auto clock selection for CBF */
+	regmap_update_bits(regmap, CBF_MUX_OFFSET,
+			   CBF_MUX_AUTO_CLK_SEL_BIT,
+			   CBF_MUX_AUTO_CLK_SEL_BIT);
+
+	/* Ensure write goes through before muxes are switched */
+	udelay(5);
+
+	/* Switch CBF to use the primary PLL */
+	regmap_update_bits(regmap, CBF_MUX_OFFSET,
+			   CBF_MUX_PARENT_MASK, 0x1);
+
+	for (i = 0; i < ARRAY_SIZE(cbf_msm8996_hw_clks); i++) {
+		ret = devm_clk_hw_register(dev, cbf_msm8996_hw_clks[i]);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(cbf_msm8996_clks); i++) {
+		ret = devm_clk_register_regmap(dev, cbf_msm8996_clks[i]);
+		if (ret)
+			return ret;
+	}
+
+	ret = devm_clk_notifier_register(dev, cbf_mux.clkr.hw.clk, &cbf_mux.nb);
+	if (ret)
+		return ret;
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
+}
+
+static const struct of_device_id qcom_msm8996_cbf_match_table[] = {
+	{ .compatible = "qcom,msm8996-cbf" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, qcom_msm8996_cbf_match_table);
+
+static struct platform_driver qcom_msm8996_cbf_driver = {
+	.probe = qcom_msm8996_cbf_probe,
+	.driver = {
+		.name = "qcom-msm8996-cbf",
+		.of_match_table = qcom_msm8996_cbf_match_table,
+	},
+};
+
+/* Register early enough to fix the clock to be used for other cores */
+static int __init qcom_msm8996_cbf_init(void)
+{
+	return platform_driver_register(&qcom_msm8996_cbf_driver);
+}
+postcore_initcall(qcom_msm8996_cbf_init);
+
+static void __exit qcom_msm8996_cbf_exit(void)
+{
+	platform_driver_unregister(&qcom_msm8996_cbf_driver);
+}
+module_exit(qcom_msm8996_cbf_exit);
+
+MODULE_DESCRIPTION("QCOM MSM8996 CPU Bus Fabric Clock Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2

[PATCH 3/5] clk: qcom: cbf-msm8996: scale CBF clock according to the CPUfreq

[Dmitry Baryshkov]

Turn CBF into the interconnect provider. Scale CBF frequency (bandwidth)
according to CPU frequencies.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-cbf-8996.c | 141 +++++++++++++++++++++++++++++++-
 1 file changed, 140 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
index bdd4f8b48a7e..76db623b0f92 100644
--- a/drivers/clk/qcom/clk-cbf-8996.c
+++ b/drivers/clk/qcom/clk-cbf-8996.c
@@ -5,6 +5,7 @@
 #include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/interconnect-provider.h>
 #include <linux/of.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -225,6 +226,133 @@ static const struct regmap_config cbf_msm8996_regmap_config = {
 	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
 };
 
+#ifdef CONFIG_INTERCONNECT
+struct qcom_msm8996_cbf_icc_provider {
+	struct icc_provider provider;
+	struct clk *clk;
+};
+
+#define to_qcom_cbf_provider(_provider) \
+	container_of(_provider, struct qcom_msm8996_cbf_icc_provider, provider)
+
+enum {
+	CBF_MASTER_NODE = 2000,
+	CBF_SLAVE_NODE
+};
+
+#define CBF_NUM_NODES 2
+
+static int qcom_msm8996_cbf_set(struct icc_node *src, struct icc_node *dst)
+{
+	struct qcom_msm8996_cbf_icc_provider *qp;
+
+	qp = to_qcom_cbf_provider(src->provider);
+
+	return clk_set_rate(qp->clk, icc_units_to_bps(dst->peak_bw));
+}
+
+static int qcom_msm8996_cbf_icc_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
+{
+	struct qcom_msm8996_cbf_icc_provider *qp;
+
+	qp = to_qcom_cbf_provider(node->provider);
+	*peak = clk_get_rate(qp->clk) / 1000ULL;
+
+	return 0;
+}
+
+static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct clk_hw *cbf_hw)
+{
+	struct device *dev = &pdev->dev;
+	struct qcom_msm8996_cbf_icc_provider *qp;
+	struct icc_provider *provider;
+	struct icc_onecell_data *data;
+	struct icc_node *node;
+	struct clk *clk;
+	int ret;
+
+	clk = devm_clk_hw_get_clk(dev, cbf_hw, "cbf");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	data = devm_kzalloc(dev, struct_size(data, nodes, CBF_NUM_NODES), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->num_nodes = CBF_NUM_NODES;
+
+	qp = devm_kzalloc(dev, sizeof(*qp), GFP_KERNEL);
+	if (!qp)
+		return -ENOMEM;
+
+	qp->clk = clk;
+
+	provider = &qp->provider;
+	provider->dev = dev;
+	provider->get_bw = qcom_msm8996_cbf_icc_get_bw;
+	provider->set = qcom_msm8996_cbf_set;
+	provider->aggregate = icc_std_aggregate;
+	provider->xlate = of_icc_xlate_onecell;
+	INIT_LIST_HEAD(&provider->nodes);
+	provider->data = data;
+
+	ret = icc_provider_add(provider);
+	if (ret) {
+		dev_err(dev, "error adding interconnect provider\n");
+		return ret;
+	}
+
+	node = icc_node_create(CBF_MASTER_NODE);
+	if (IS_ERR(node)) {
+		ret = PTR_ERR(node);
+		goto err;
+	}
+
+	node->name = "cbf_master";
+	icc_node_add(node, provider);
+	icc_link_create(node, CBF_SLAVE_NODE);
+	data->nodes[0] = node;
+
+	node = icc_node_create(CBF_SLAVE_NODE);
+	if (IS_ERR(node)) {
+		ret = PTR_ERR(node);
+		goto err;
+	}
+
+	node->name = "cbf_slave";
+	icc_node_add(node, provider);
+	data->nodes[1] = node;
+
+	platform_set_drvdata(pdev, provider);
+
+	return 0;
+
+err:
+	icc_nodes_remove(provider);
+	icc_provider_del(provider);
+
+	return ret;
+}
+
+static int qcom_msm8996_cbf_icc_remove(struct platform_device *pdev)
+{
+	struct icc_provider *provider = platform_get_drvdata(pdev);
+
+	icc_nodes_remove(provider);
+	icc_provider_del(provider);
+
+	return 0;
+}
+#else
+static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev)
+{
+	dev_warn(&pdev->dev, "interconnects support is disabled, CBF clock is fixed\n");
+
+	return 0;
+}
+#define qcom_msm8996_cbf_icc_remove(pdev) (0)
+#endif
+
 static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
 {
 	void __iomem *base;
@@ -284,7 +412,16 @@ static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
+	if (ret)
+		return ret;
+
+	return qcom_msm8996_cbf_icc_register(pdev, &cbf_mux.clkr.hw);
+}
+
+static int qcom_msm8996_cbf_remove(struct platform_device *pdev)
+{
+	return qcom_msm8996_cbf_icc_remove(pdev);
 }
 
 static const struct of_device_id qcom_msm8996_cbf_match_table[] = {
@@ -295,9 +432,11 @@ MODULE_DEVICE_TABLE(of, qcom_msm8996_cbf_match_table);
 
 static struct platform_driver qcom_msm8996_cbf_driver = {
 	.probe = qcom_msm8996_cbf_probe,
+	.remove = qcom_msm8996_cbf_remove,
 	.driver = {
 		.name = "qcom-msm8996-cbf",
 		.of_match_table = qcom_msm8996_cbf_match_table,
+		.sync_state = icc_sync_state,
 	},
 };
 
-- 
2.30.2

[PATCH 4/5] arm64: dts: qcom: msm8996: add CBF device entry

[Dmitry Baryshkov]

Add device tree node for the CBF clock.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 37ebd8a3b2c8..e5a638b85ffc 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -3558,6 +3558,13 @@ saw3: syscon@9a10000 {
 			reg = <0x09a10000 0x1000>;
 		};
 
+		cbf: clock-controller@9a11000 {
+			compatible = "qcom,msm8996-cbf";
+			reg = <0x09a11000 0x10000>;
+			clocks = <&rpmcc RPM_SMD_BB_CLK1>, <&apcs_glb>;
+			#clock-cells = <0>;
+		};
+
 		intc: interrupt-controller@9bc0000 {
 			compatible = "qcom,msm8996-gic-v3", "arm,gic-v3";
 			#interrupt-cells = <3>;
-- 
2.30.2

[PATCH 5/5] arm64: dts: qcom: msm8996: scale CBF clock according to the CPUfreq

[Dmitry Baryshkov]

Turn CBF into the interconnect provider. Scale CBF frequency (bandwidth)
according to CPU frequencies.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 50 +++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index e5a638b85ffc..675933e8e98a 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -49,6 +49,7 @@ CPU0: cpu@0 {
 			cpu-idle-states = <&CPU_SLEEP_0>;
 			capacity-dmips-mhz = <1024>;
 			clocks = <&kryocc 0>;
+			interconnects = <&cbf 0 &cbf 1>;
 			operating-points-v2 = <&cluster0_opp>;
 			#cooling-cells = <2>;
 			next-level-cache = <&L2_0>;
@@ -66,6 +67,7 @@ CPU1: cpu@1 {
 			cpu-idle-states = <&CPU_SLEEP_0>;
 			capacity-dmips-mhz = <1024>;
 			clocks = <&kryocc 0>;
+			interconnects = <&cbf 0 &cbf 1>;
 			operating-points-v2 = <&cluster0_opp>;
 			#cooling-cells = <2>;
 			next-level-cache = <&L2_0>;
@@ -79,6 +81,7 @@ CPU2: cpu@100 {
 			cpu-idle-states = <&CPU_SLEEP_0>;
 			capacity-dmips-mhz = <1024>;
 			clocks = <&kryocc 1>;
+			interconnects = <&cbf 0 &cbf 1>;
 			operating-points-v2 = <&cluster1_opp>;
 			#cooling-cells = <2>;
 			next-level-cache = <&L2_1>;
@@ -96,6 +99,7 @@ CPU3: cpu@101 {
 			cpu-idle-states = <&CPU_SLEEP_0>;
 			capacity-dmips-mhz = <1024>;
 			clocks = <&kryocc 1>;
+			interconnects = <&cbf 0 &cbf 1>;
 			operating-points-v2 = <&cluster1_opp>;
 			#cooling-cells = <2>;
 			next-level-cache = <&L2_1>;
@@ -147,91 +151,109 @@ opp-307200000 {
 			opp-hz = /bits/ 64 <307200000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-422400000 {
 			opp-hz = /bits/ 64 <422400000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-480000000 {
 			opp-hz = /bits/ 64 <480000000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-556800000 {
 			opp-hz = /bits/ 64 <556800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-652800000 {
 			opp-hz = /bits/ 64 <652800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <384000>;
 		};
 		opp-729600000 {
 			opp-hz = /bits/ 64 <729600000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <460800>;
 		};
 		opp-844800000 {
 			opp-hz = /bits/ 64 <844800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <537600>;
 		};
 		opp-960000000 {
 			opp-hz = /bits/ 64 <960000000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <672000>;
 		};
 		opp-1036800000 {
 			opp-hz = /bits/ 64 <1036800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <672000>;
 		};
 		opp-1113600000 {
 			opp-hz = /bits/ 64 <1113600000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <825600>;
 		};
 		opp-1190400000 {
 			opp-hz = /bits/ 64 <1190400000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <825600>;
 		};
 		opp-1228800000 {
 			opp-hz = /bits/ 64 <1228800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <902400>;
 		};
 		opp-1324800000 {
 			opp-hz = /bits/ 64 <1324800000>;
 			opp-supported-hw = <0xd>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1056000>;
 		};
 		opp-1363200000 {
 			opp-hz = /bits/ 64 <1363200000>;
 			opp-supported-hw = <0x2>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1132800>;
 		};
 		opp-1401600000 {
 			opp-hz = /bits/ 64 <1401600000>;
 			opp-supported-hw = <0xd>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1132800>;
 		};
 		opp-1478400000 {
 			opp-hz = /bits/ 64 <1478400000>;
 			opp-supported-hw = <0x9>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1190400>;
 		};
 		opp-1497600000 {
 			opp-hz = /bits/ 64 <1497600000>;
 			opp-supported-hw = <0x04>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1305600>;
 		};
 		opp-1593600000 {
 			opp-hz = /bits/ 64 <1593600000>;
 			opp-supported-hw = <0x9>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1382400>;
 		};
 	};
 
@@ -245,136 +267,163 @@ opp-307200000 {
 			opp-hz = /bits/ 64 <307200000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-403200000 {
 			opp-hz = /bits/ 64 <403200000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-480000000 {
 			opp-hz = /bits/ 64 <480000000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-556800000 {
 			opp-hz = /bits/ 64 <556800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-652800000 {
 			opp-hz = /bits/ 64 <652800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-729600000 {
 			opp-hz = /bits/ 64 <729600000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <307200>;
 		};
 		opp-806400000 {
 			opp-hz = /bits/ 64 <806400000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <384000>;
 		};
 		opp-883200000 {
 			opp-hz = /bits/ 64 <883200000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <460800>;
 		};
 		opp-940800000 {
 			opp-hz = /bits/ 64 <940800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <537600>;
 		};
 		opp-1036800000 {
 			opp-hz = /bits/ 64 <1036800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <595200>;
 		};
 		opp-1113600000 {
 			opp-hz = /bits/ 64 <1113600000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <672000>;
 		};
 		opp-1190400000 {
 			opp-hz = /bits/ 64 <1190400000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <672000>;
 		};
 		opp-1248000000 {
 			opp-hz = /bits/ 64 <1248000000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <748800>;
 		};
 		opp-1324800000 {
 			opp-hz = /bits/ 64 <1324800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <825600>;
 		};
 		opp-1401600000 {
 			opp-hz = /bits/ 64 <1401600000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <902400>;
 		};
 		opp-1478400000 {
 			opp-hz = /bits/ 64 <1478400000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <979200>;
 		};
 		opp-1555200000 {
 			opp-hz = /bits/ 64 <1555200000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1056000>;
 		};
 		opp-1632000000 {
 			opp-hz = /bits/ 64 <1632000000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1190400>;
 		};
 		opp-1708800000 {
 			opp-hz = /bits/ 64 <1708800000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1228800>;
 		};
 		opp-1785600000 {
 			opp-hz = /bits/ 64 <1785600000>;
 			opp-supported-hw = <0xf>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1305600>;
 		};
 		opp-1804800000 {
 			opp-hz = /bits/ 64 <1804800000>;
 			opp-supported-hw = <0xe>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1305600>;
 		};
 		opp-1824000000 {
 			opp-hz = /bits/ 64 <1824000000>;
 			opp-supported-hw = <0x1>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1382400>;
 		};
 		opp-1900800000 {
 			opp-hz = /bits/ 64 <1900800000>;
 			opp-supported-hw = <0x4>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1305600>;
 		};
 		opp-1920000000 {
 			opp-hz = /bits/ 64 <1920000000>;
 			opp-supported-hw = <0x1>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1459200>;
 		};
 		opp-1996800000 {
 			opp-hz = /bits/ 64 <1996800000>;
 			opp-supported-hw = <0x1>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1593600>;
 		};
 		opp-2073600000 {
 			opp-hz = /bits/ 64 <2073600000>;
 			opp-supported-hw = <0x1>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1593600>;
 		};
 		opp-2150400000 {
 			opp-hz = /bits/ 64 <2150400000>;
 			opp-supported-hw = <0x1>;
 			clock-latency-ns = <200000>;
+			opp-peak-kBps = <1593600>;
 		};
 	};
 
@@ -3563,6 +3612,7 @@ cbf: clock-controller@9a11000 {
 			reg = <0x09a11000 0x10000>;
 			clocks = <&rpmcc RPM_SMD_BB_CLK1>, <&apcs_glb>;
 			#clock-cells = <0>;
+			#interconnect-cells = <1>;
 		};
 
 		intc: interrupt-controller@9bc0000 {
-- 
2.30.2

Re: [PATCH 1/5] dt-bindings: clock: qcom,msm8996-cbf: Describe the MSM8996 CBF clock controller

[Krzysztof Kozlowski]

On 11/01/2023 20:57, Dmitry Baryshkov wrote:
> MSM8996 Core Bus Fabric (CBF) clock controller clocks an interconnect
> between two CPU clusters. The CBF clock should follow the CPU
> frequencies to provide enough bandwidth between clusters. Thus a single
> driver implements both a clock and an interconnect to set the clock
> rate.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../bindings/clock/qcom,msm8996-cbf.yaml      | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml b/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
> new file mode 100644
> index 000000000000..2080e1743b2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: GPL-2.0-only

Dual license, please.

Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: clock: qcom,msm8996-cbf: Describe the MSM8996 CBF clock controller

[Dmitry Baryshkov]

On 12/01/2023 10:40, Krzysztof Kozlowski wrote:
> On 11/01/2023 20:57, Dmitry Baryshkov wrote:
>> MSM8996 Core Bus Fabric (CBF) clock controller clocks an interconnect
>> between two CPU clusters. The CBF clock should follow the CPU
>> frequencies to provide enough bandwidth between clusters. Thus a single
>> driver implements both a clock and an interconnect to set the clock
>> rate.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   .../bindings/clock/qcom,msm8996-cbf.yaml      | 53 +++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml b/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
>> new file mode 100644
>> index 000000000000..2080e1743b2e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
>> @@ -0,0 +1,53 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
> 
> Dual license, please.

Ack, I'll fix it for v2. Do you have any other comments for the file itself?

-- 
With best wishes
Dmitry

Re: [PATCH 1/5] dt-bindings: clock: qcom,msm8996-cbf: Describe the MSM8996 CBF clock controller

[Krzysztof Kozlowski]

On 12/01/2023 10:47, Dmitry Baryshkov wrote:
> On 12/01/2023 10:40, Krzysztof Kozlowski wrote:
>> On 11/01/2023 20:57, Dmitry Baryshkov wrote:
>>> MSM8996 Core Bus Fabric (CBF) clock controller clocks an interconnect
>>> between two CPU clusters. The CBF clock should follow the CPU
>>> frequencies to provide enough bandwidth between clusters. Thus a single
>>> driver implements both a clock and an interconnect to set the clock
>>> rate.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   .../bindings/clock/qcom,msm8996-cbf.yaml      | 53 +++++++++++++++++++
>>>   1 file changed, 53 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml b/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
>>> new file mode 100644
>>> index 000000000000..2080e1743b2e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/qcom,msm8996-cbf.yaml
>>> @@ -0,0 +1,53 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>
>> Dual license, please.
> 
> Ack, I'll fix it for v2. Do you have any other comments for the file itself?

No, rest look good.

Best regards,
Krzysztof

Re: [PATCH 2/5] clk: qcom: add msm8996 Core Bus Framework (CBF) support

[Konrad Dybcio]

On 11.01.2023 20:57, Dmitry Baryshkov wrote:
> Add CBF clock driver as a part of MSM8996 CPU clocks. Significantly
> based on AngeloGioacchino del Regno's work at [1].
> 
> The CBF is an interconnect between two CPU clusters, setting it up
> properly is required for booting the MSM8996 with all four cores
> enabled.
> 
> [1] https://github.com/sonyxperiadev/kernel/blob/aosp/LE.UM.2.3.2.r1.4/drivers/clk/qcom/clk-cpu-8996.c
> 
> Co-developed-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/Makefile       |   2 +-
>  drivers/clk/qcom/clk-cbf-8996.c | 318 ++++++++++++++++++++++++++++++++
>  2 files changed, 319 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/qcom/clk-cbf-8996.c
> 
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index ca2f586edb3e..d6ecd2bf7040 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -52,7 +52,7 @@ obj-$(CONFIG_MSM_MMCC_8998) += mmcc-msm8998.o
>  obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
>  obj-$(CONFIG_QCOM_A7PLL) += a7-pll.o
>  obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
> -obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += apcs-msm8996.o clk-cpu-8996.o
> +obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += apcs-msm8996.o clk-cpu-8996.o clk-cbf-8996.o
>  obj-$(CONFIG_QCOM_CLK_APCS_SDX55) += apcs-sdx55.o
>  obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
>  obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
> diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
> new file mode 100644
> index 000000000000..bdd4f8b48a7e
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-cbf-8996.c
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022, Linaro Ltd.
Happy new year!

> + */
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-regmap.h"
> +
> +/* Need to match the order of clocks in DT binding */
> +enum {
> +	DT_XO,
No, it should be XO_A, otherwise the platform will never sleep.

+	DT_APCS_AUX,
> +};
> +
> +enum {
> +	CBF_XO_INDEX,
> +	CBF_PLL_INDEX,
> +	CBF_DIV_INDEX,
> +	CBF_APCS_AUX_INDEX,
> +};
> +
> +#define DIV_THRESHOLD		600000000
> +
> +#define CBF_MUX_OFFSET		0x18
> +#define CBF_MUX_PARENT_MASK		GENMASK(1, 0)
> +#define CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK GENMASK(5, 4)
> +#define CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL \
> +	FIELD_PREP(CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK, 0x03)
> +#define CBF_MUX_AUTO_CLK_SEL_BIT	BIT(6)
> +
> +#define CBF_PLL_OFFSET 0xf000
> +
> +static const u8 cbf_pll_regs[PLL_OFF_MAX_REGS] = {
> +	[PLL_OFF_L_VAL] = 0x08,
> +	[PLL_OFF_ALPHA_VAL] = 0x10,
> +	[PLL_OFF_USER_CTL] = 0x18,
> +	[PLL_OFF_CONFIG_CTL] = 0x20,
> +	[PLL_OFF_CONFIG_CTL_U] = 0x24,
> +	[PLL_OFF_TEST_CTL] = 0x30,
> +	[PLL_OFF_TEST_CTL_U] = 0x34,
> +	[PLL_OFF_STATUS] = 0x28,
> +};
> +
> +static const struct alpha_pll_config cbfpll_config = {
> +	.l = 72,
> +	.config_ctl_val = 0x200d4828,
> +	.config_ctl_hi_val = 0x006,
> +	.test_ctl_val = 0x1c000000,
> +	.test_ctl_hi_val = 0x00004000,
> +	.pre_div_mask = BIT(12),
> +	.post_div_mask = 0x3 << 8,
> +	.post_div_val = 0x1 << 8,
> +	.main_output_mask = BIT(0),
> +	.early_output_mask = BIT(3),
> +};
> +
> +static struct clk_alpha_pll cbf_pll = {
> +	.offset = CBF_PLL_OFFSET,
> +	.regs = cbf_pll_regs,
> +	.flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE,
> +	.clkr.hw.init = &(struct clk_init_data){
> +		.name = "cbf_pll",
> +		.parent_data = (const struct clk_parent_data[]) {
> +			{ .index = DT_XO, },
> +		},
> +		.num_parents = 1,
> +		.ops = &clk_alpha_pll_hwfsm_ops,
> +	},
> +};
> +
> +static struct clk_fixed_factor cbf_pll_postdiv = {
> +	.mult = 1,
> +	.div = 2,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "cbf_pll_postdiv",
> +		.parent_data = &(const struct clk_parent_data){
parent_hws?

> +			.hw = &cbf_pll.clkr.hw
> +		},
> +		.num_parents = 1,
> +		.ops = &clk_fixed_factor_ops,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static const struct clk_parent_data cbf_mux_parent_data[] = {
> +	{ .index = DT_XO },
> +	{ .hw = &cbf_pll.clkr.hw },
> +	{ .hw = &cbf_pll_postdiv.hw },
> +	{ .index = DT_APCS_AUX },
> +};
> +
> +struct clk_cbf_8996_mux {
> +	u32 reg;
> +	struct notifier_block nb;
> +	struct clk_regmap clkr;
> +};
> +
> +static struct clk_cbf_8996_mux *to_clk_cbf_8996_mux(struct clk_regmap *clkr)
> +{
> +	return container_of(clkr, struct clk_cbf_8996_mux, clkr);
> +}
> +
> +static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
> +			       void *data);
Please align the indentation with the opening bracket.

> +
> +static u8 clk_cbf_8996_mux_get_parent(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +	struct clk_cbf_8996_mux *mux = to_clk_cbf_8996_mux(clkr);
> +	u32 val;
> +
> +	regmap_read(clkr->regmap, mux->reg, &val);
> +
> +	return FIELD_GET(CBF_MUX_PARENT_MASK, val);
> +}
> +
> +static int clk_cbf_8996_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +	struct clk_cbf_8996_mux *mux = to_clk_cbf_8996_mux(clkr);
> +	u32 val;
> +
> +	val = FIELD_PREP(CBF_MUX_PARENT_MASK, index);
> +
> +	return regmap_update_bits(clkr->regmap, mux->reg, CBF_MUX_PARENT_MASK, val);
> +}
> +
> +static int clk_cbf_8996_mux_determine_rate(struct clk_hw *hw,
> +					   struct clk_rate_request *req)
> +{
> +	struct clk_hw *parent;
> +
> +	if (req->rate < (DIV_THRESHOLD / 2))
> +		return -EINVAL;
Shouldn't you parent it to APCS AUX here?

> +
> +	if (req->rate < DIV_THRESHOLD)
> +		parent = clk_hw_get_parent_by_index(hw, CBF_DIV_INDEX);
> +	else
> +		parent = clk_hw_get_parent_by_index(hw, CBF_PLL_INDEX);
> +
> +	if (!parent)
> +		return -EINVAL;
> +
> +	req->best_parent_rate = clk_hw_round_rate(parent, req->rate);
> +	req->best_parent_hw = parent;
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops clk_cbf_8996_mux_ops = {
> +	.set_parent = clk_cbf_8996_mux_set_parent,
> +	.get_parent = clk_cbf_8996_mux_get_parent,
> +	.determine_rate = clk_cbf_8996_mux_determine_rate,
> +};
> +
> +static struct clk_cbf_8996_mux cbf_mux = {
> +	.reg = CBF_MUX_OFFSET,
> +	.nb.notifier_call = cbf_clk_notifier_cb,
> +	.clkr.hw.init = &(struct clk_init_data) {
> +		.name = "cbf_mux",
> +		.parent_data = cbf_mux_parent_data,
> +		.num_parents = ARRAY_SIZE(cbf_mux_parent_data),
> +		.ops = &clk_cbf_8996_mux_ops,
> +		/* CPU clock is critical and should never be gated */
> +		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> +	},
> +};
> +
> +static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
> +			       void *data)
> +{
> +	struct clk_notifier_data *cnd = data;
> +	int ret;
> +
> +	switch (event) {
> +	case PRE_RATE_CHANGE:
> +		/*
> +		 * Avoid overvolting. clk_core_set_rate_nolock() walks from top
> +		 * to bottom, so it will change the rate of the PLL before
> +		 * chaging the parent of PMUX. This can result in pmux getting
> +		 * clocked twice the expected rate.
> +		 *
> +		 * Manually switch to PLL/2 here.
> +		 */
> +		if (cnd->old_rate > DIV_THRESHOLD &&
> +		    cnd->new_rate < DIV_THRESHOLD)
> +			clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_DIV_INDEX);
> +		break;
> +	case ABORT_RATE_CHANGE:
> +		/* Revert manual change */
> +		if (cnd->new_rate < DIV_THRESHOLD &&
> +		    cnd->old_rate > DIV_THRESHOLD)
> +			clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_PLL_INDEX);
> +		break;
> +	default:
> +		ret = 0;
> +		break;
> +	}
> +
> +	return notifier_from_errno(ret);
> +};
> +
> +static struct clk_hw *cbf_msm8996_hw_clks[] = {
> +	&cbf_pll_postdiv.hw,
> +};
> +
> +static struct clk_regmap *cbf_msm8996_clks[] = {
> +	&cbf_pll.clkr,
> +	&cbf_mux.clkr,
> +};
> +
> +static const struct regmap_config cbf_msm8996_regmap_config = {
> +	.reg_bits		= 32,
> +	.reg_stride		= 4,
> +	.val_bits		= 32,
> +	.max_register		= 0x10000,
> +	.fast_io		= true,
> +	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
> +};
> +
> +static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
> +{
> +	void __iomem *base;
> +	struct regmap *regmap;
> +	struct device *dev = &pdev->dev;
> +	int i, ret;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	regmap = devm_regmap_init_mmio(dev, base, &cbf_msm8996_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	/* Select GPLL0 for 300MHz for the CBF clock */
> +	regmap_write(regmap, CBF_MUX_OFFSET, 0x3);
> +
> +	/* Ensure write goes through before PLLs are reconfigured */
> +	udelay(5);
> +
> +	/* Set the auto clock sel always-on source to GPLL0/2 (300MHz) */
> +	regmap_update_bits(regmap, CBF_MUX_OFFSET,
> +			   CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK,
> +			   CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL);
> +
> +	clk_alpha_pll_configure(&cbf_pll, regmap, &cbfpll_config);
> +
> +	/* Wait for PLL(s) to lock */
> +        udelay(50);
Weird indentation?

> +
> +	/* Enable auto clock selection for CBF */
> +	regmap_update_bits(regmap, CBF_MUX_OFFSET,
> +			   CBF_MUX_AUTO_CLK_SEL_BIT,
> +			   CBF_MUX_AUTO_CLK_SEL_BIT);
> +
> +	/* Ensure write goes through before muxes are switched */
> +	udelay(5);
> +
> +	/* Switch CBF to use the primary PLL */
> +	regmap_update_bits(regmap, CBF_MUX_OFFSET,
> +			   CBF_MUX_PARENT_MASK, 0x1);
I think you can unwrap this.

> +
> +	for (i = 0; i < ARRAY_SIZE(cbf_msm8996_hw_clks); i++) {
> +		ret = devm_clk_hw_register(dev, cbf_msm8996_hw_clks[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(cbf_msm8996_clks); i++) {
> +		ret = devm_clk_register_regmap(dev, cbf_msm8996_clks[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = devm_clk_notifier_register(dev, cbf_mux.clkr.hw.clk, &cbf_mux.nb);
> +	if (ret)
> +		return ret;
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
> +}
> +
> +static const struct of_device_id qcom_msm8996_cbf_match_table[] = {
> +	{ .compatible = "qcom,msm8996-cbf" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_msm8996_cbf_match_table);
> +
> +static struct platform_driver qcom_msm8996_cbf_driver = {
> +	.probe = qcom_msm8996_cbf_probe,
> +	.driver = {
> +		.name = "qcom-msm8996-cbf",
> +		.of_match_table = qcom_msm8996_cbf_match_table,
> +	},
> +};
> +
> +/* Register early enough to fix the clock to be used for other cores */
> +static int __init qcom_msm8996_cbf_init(void)
> +{
> +	return platform_driver_register(&qcom_msm8996_cbf_driver);
> +}
> +postcore_initcall(qcom_msm8996_cbf_init);
> +
> +static void __exit qcom_msm8996_cbf_exit(void)
> +{
> +	platform_driver_unregister(&qcom_msm8996_cbf_driver);
> +}
> +module_exit(qcom_msm8996_cbf_exit);
> +
> +MODULE_DESCRIPTION("QCOM MSM8996 CPU Bus Fabric Clock Driver");
> +MODULE_LICENSE("GPL v2");
"GPL"

The rest looks good..

Konrad

Re: [PATCH 3/5] clk: qcom: cbf-msm8996: scale CBF clock according to the CPUfreq

[Konrad Dybcio]

On 11.01.2023 20:57, Dmitry Baryshkov wrote:
> Turn CBF into the interconnect provider. Scale CBF frequency (bandwidth)
> according to CPU frequencies.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
TODO: assess if we should be doing this manually or rely on SPDM..

Konrad
>  drivers/clk/qcom/clk-cbf-8996.c | 141 +++++++++++++++++++++++++++++++-
>  1 file changed, 140 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
> index bdd4f8b48a7e..76db623b0f92 100644
> --- a/drivers/clk/qcom/clk-cbf-8996.c
> +++ b/drivers/clk/qcom/clk-cbf-8996.c
> @@ -5,6 +5,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
> +#include <linux/interconnect-provider.h>
>  #include <linux/of.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> @@ -225,6 +226,133 @@ static const struct regmap_config cbf_msm8996_regmap_config = {
>  	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
>  };
>  
> +#ifdef CONFIG_INTERCONNECT
> +struct qcom_msm8996_cbf_icc_provider {
> +	struct icc_provider provider;
> +	struct clk *clk;
> +};
> +
> +#define to_qcom_cbf_provider(_provider) \
> +	container_of(_provider, struct qcom_msm8996_cbf_icc_provider, provider)
> +
> +enum {
> +	CBF_MASTER_NODE = 2000,
> +	CBF_SLAVE_NODE
> +};
> +
> +#define CBF_NUM_NODES 2
> +
> +static int qcom_msm8996_cbf_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	struct qcom_msm8996_cbf_icc_provider *qp;
> +
> +	qp = to_qcom_cbf_provider(src->provider);
> +
> +	return clk_set_rate(qp->clk, icc_units_to_bps(dst->peak_bw));
> +}
> +
> +static int qcom_msm8996_cbf_icc_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
> +{
> +	struct qcom_msm8996_cbf_icc_provider *qp;
> +
> +	qp = to_qcom_cbf_provider(node->provider);
> +	*peak = clk_get_rate(qp->clk) / 1000ULL;
> +
> +	return 0;
> +}
> +
> +static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct clk_hw *cbf_hw)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_msm8996_cbf_icc_provider *qp;
> +	struct icc_provider *provider;
> +	struct icc_onecell_data *data;
> +	struct icc_node *node;
> +	struct clk *clk;
> +	int ret;
> +
> +	clk = devm_clk_hw_get_clk(dev, cbf_hw, "cbf");
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	data = devm_kzalloc(dev, struct_size(data, nodes, CBF_NUM_NODES), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->num_nodes = CBF_NUM_NODES;
> +
> +	qp = devm_kzalloc(dev, sizeof(*qp), GFP_KERNEL);
> +	if (!qp)
> +		return -ENOMEM;
> +
> +	qp->clk = clk;
> +
> +	provider = &qp->provider;
> +	provider->dev = dev;
> +	provider->get_bw = qcom_msm8996_cbf_icc_get_bw;
> +	provider->set = qcom_msm8996_cbf_set;
> +	provider->aggregate = icc_std_aggregate;
> +	provider->xlate = of_icc_xlate_onecell;
> +	INIT_LIST_HEAD(&provider->nodes);
> +	provider->data = data;
> +
> +	ret = icc_provider_add(provider);
> +	if (ret) {
> +		dev_err(dev, "error adding interconnect provider\n");
> +		return ret;
> +	}
> +
> +	node = icc_node_create(CBF_MASTER_NODE);
> +	if (IS_ERR(node)) {
> +		ret = PTR_ERR(node);
> +		goto err;
> +	}
> +
> +	node->name = "cbf_master";
> +	icc_node_add(node, provider);
> +	icc_link_create(node, CBF_SLAVE_NODE);
> +	data->nodes[0] = node;
> +
> +	node = icc_node_create(CBF_SLAVE_NODE);
> +	if (IS_ERR(node)) {
> +		ret = PTR_ERR(node);
> +		goto err;
> +	}
> +
> +	node->name = "cbf_slave";
> +	icc_node_add(node, provider);
> +	data->nodes[1] = node;
> +
> +	platform_set_drvdata(pdev, provider);
> +
> +	return 0;
> +
> +err:
> +	icc_nodes_remove(provider);
> +	icc_provider_del(provider);
> +
> +	return ret;
> +}
> +
> +static int qcom_msm8996_cbf_icc_remove(struct platform_device *pdev)
> +{
> +	struct icc_provider *provider = platform_get_drvdata(pdev);
> +
> +	icc_nodes_remove(provider);
> +	icc_provider_del(provider);
> +
> +	return 0;
> +}
> +#else
> +static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev)
> +{
> +	dev_warn(&pdev->dev, "interconnects support is disabled, CBF clock is fixed\n");
> +
> +	return 0;
> +}
> +#define qcom_msm8996_cbf_icc_remove(pdev) (0)
> +#endif
> +
>  static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
>  {
>  	void __iomem *base;
> @@ -284,7 +412,16 @@ static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
> +	if (ret)
> +		return ret;
> +
> +	return qcom_msm8996_cbf_icc_register(pdev, &cbf_mux.clkr.hw);
> +}
> +
> +static int qcom_msm8996_cbf_remove(struct platform_device *pdev)
> +{
> +	return qcom_msm8996_cbf_icc_remove(pdev);
>  }
>  
>  static const struct of_device_id qcom_msm8996_cbf_match_table[] = {
> @@ -295,9 +432,11 @@ MODULE_DEVICE_TABLE(of, qcom_msm8996_cbf_match_table);
>  
>  static struct platform_driver qcom_msm8996_cbf_driver = {
>  	.probe = qcom_msm8996_cbf_probe,
> +	.remove = qcom_msm8996_cbf_remove,
>  	.driver = {
>  		.name = "qcom-msm8996-cbf",
>  		.of_match_table = qcom_msm8996_cbf_match_table,
> +		.sync_state = icc_sync_state,
>  	},
>  };
>  

Re: [PATCH 4/5] arm64: dts: qcom: msm8996: add CBF device entry

[Konrad Dybcio]

On 11.01.2023 20:57, Dmitry Baryshkov wrote:
> Add device tree node for the CBF clock.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8996.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> index 37ebd8a3b2c8..e5a638b85ffc 100644
> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> @@ -3558,6 +3558,13 @@ saw3: syscon@9a10000 {
>  			reg = <0x09a10000 0x1000>;
>  		};
>  
> +		cbf: clock-controller@9a11000 {
> +			compatible = "qcom,msm8996-cbf";
> +			reg = <0x09a11000 0x10000>;
> +			clocks = <&rpmcc RPM_SMD_BB_CLK1>, <&apcs_glb>;
Shouldn't this be RPM_SMD_XO_A_CLK_SRC? I don't see BB_CLK
referenced *anywhere* downstream, other than being defined
in the 8996 GCC driver..

Konrad

> +			#clock-cells = <0>;
> +		};
> +
>  		intc: interrupt-controller@9bc0000 {
>  			compatible = "qcom,msm8996-gic-v3", "arm,gic-v3";
>  			#interrupt-cells = <3>;

Re: [PATCH 3/5] clk: qcom: cbf-msm8996: scale CBF clock according to the CPUfreq

[Dmitry Baryshkov]

On 12/01/2023 17:00, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:57, Dmitry Baryshkov wrote:
>> Turn CBF into the interconnect provider. Scale CBF frequency (bandwidth)
>> according to CPU frequencies.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> TODO: assess if we should be doing this manually or rely on SPDM..

Manually, see the msm-3.18's devfreq-cpu and m4m-cpufreq maps.

SPDM uses CBF as an input not as an output.

> 
> Konrad
>>   drivers/clk/qcom/clk-cbf-8996.c | 141 +++++++++++++++++++++++++++++++-
>>   1 file changed, 140 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
>> index bdd4f8b48a7e..76db623b0f92 100644
>> --- a/drivers/clk/qcom/clk-cbf-8996.c
>> +++ b/drivers/clk/qcom/clk-cbf-8996.c
>> @@ -5,6 +5,7 @@
>>   #include <linux/bitfield.h>
>>   #include <linux/clk.h>
>>   #include <linux/clk-provider.h>
>> +#include <linux/interconnect-provider.h>
>>   #include <linux/of.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>> @@ -225,6 +226,133 @@ static const struct regmap_config cbf_msm8996_regmap_config = {
>>   	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
>>   };
>>   
>> +#ifdef CONFIG_INTERCONNECT
>> +struct qcom_msm8996_cbf_icc_provider {
>> +	struct icc_provider provider;
>> +	struct clk *clk;
>> +};
>> +
>> +#define to_qcom_cbf_provider(_provider) \
>> +	container_of(_provider, struct qcom_msm8996_cbf_icc_provider, provider)
>> +
>> +enum {
>> +	CBF_MASTER_NODE = 2000,
>> +	CBF_SLAVE_NODE
>> +};
>> +
>> +#define CBF_NUM_NODES 2
>> +
>> +static int qcom_msm8996_cbf_set(struct icc_node *src, struct icc_node *dst)
>> +{
>> +	struct qcom_msm8996_cbf_icc_provider *qp;
>> +
>> +	qp = to_qcom_cbf_provider(src->provider);
>> +
>> +	return clk_set_rate(qp->clk, icc_units_to_bps(dst->peak_bw));
>> +}
>> +
>> +static int qcom_msm8996_cbf_icc_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
>> +{
>> +	struct qcom_msm8996_cbf_icc_provider *qp;
>> +
>> +	qp = to_qcom_cbf_provider(node->provider);
>> +	*peak = clk_get_rate(qp->clk) / 1000ULL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct clk_hw *cbf_hw)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct qcom_msm8996_cbf_icc_provider *qp;
>> +	struct icc_provider *provider;
>> +	struct icc_onecell_data *data;
>> +	struct icc_node *node;
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	clk = devm_clk_hw_get_clk(dev, cbf_hw, "cbf");
>> +	if (IS_ERR(clk))
>> +		return PTR_ERR(clk);
>> +
>> +	data = devm_kzalloc(dev, struct_size(data, nodes, CBF_NUM_NODES), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->num_nodes = CBF_NUM_NODES;
>> +
>> +	qp = devm_kzalloc(dev, sizeof(*qp), GFP_KERNEL);
>> +	if (!qp)
>> +		return -ENOMEM;
>> +
>> +	qp->clk = clk;
>> +
>> +	provider = &qp->provider;
>> +	provider->dev = dev;
>> +	provider->get_bw = qcom_msm8996_cbf_icc_get_bw;
>> +	provider->set = qcom_msm8996_cbf_set;
>> +	provider->aggregate = icc_std_aggregate;
>> +	provider->xlate = of_icc_xlate_onecell;
>> +	INIT_LIST_HEAD(&provider->nodes);
>> +	provider->data = data;
>> +
>> +	ret = icc_provider_add(provider);
>> +	if (ret) {
>> +		dev_err(dev, "error adding interconnect provider\n");
>> +		return ret;
>> +	}
>> +
>> +	node = icc_node_create(CBF_MASTER_NODE);
>> +	if (IS_ERR(node)) {
>> +		ret = PTR_ERR(node);
>> +		goto err;
>> +	}
>> +
>> +	node->name = "cbf_master";
>> +	icc_node_add(node, provider);
>> +	icc_link_create(node, CBF_SLAVE_NODE);
>> +	data->nodes[0] = node;
>> +
>> +	node = icc_node_create(CBF_SLAVE_NODE);
>> +	if (IS_ERR(node)) {
>> +		ret = PTR_ERR(node);
>> +		goto err;
>> +	}
>> +
>> +	node->name = "cbf_slave";
>> +	icc_node_add(node, provider);
>> +	data->nodes[1] = node;
>> +
>> +	platform_set_drvdata(pdev, provider);
>> +
>> +	return 0;
>> +
>> +err:
>> +	icc_nodes_remove(provider);
>> +	icc_provider_del(provider);
>> +
>> +	return ret;
>> +}
>> +
>> +static int qcom_msm8996_cbf_icc_remove(struct platform_device *pdev)
>> +{
>> +	struct icc_provider *provider = platform_get_drvdata(pdev);
>> +
>> +	icc_nodes_remove(provider);
>> +	icc_provider_del(provider);
>> +
>> +	return 0;
>> +}
>> +#else
>> +static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev)
>> +{
>> +	dev_warn(&pdev->dev, "interconnects support is disabled, CBF clock is fixed\n");
>> +
>> +	return 0;
>> +}
>> +#define qcom_msm8996_cbf_icc_remove(pdev) (0)
>> +#endif
>> +
>>   static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
>>   {
>>   	void __iomem *base;
>> @@ -284,7 +412,16 @@ static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		return ret;
>>   
>> -	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
>> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return qcom_msm8996_cbf_icc_register(pdev, &cbf_mux.clkr.hw);
>> +}
>> +
>> +static int qcom_msm8996_cbf_remove(struct platform_device *pdev)
>> +{
>> +	return qcom_msm8996_cbf_icc_remove(pdev);
>>   }
>>   
>>   static const struct of_device_id qcom_msm8996_cbf_match_table[] = {
>> @@ -295,9 +432,11 @@ MODULE_DEVICE_TABLE(of, qcom_msm8996_cbf_match_table);
>>   
>>   static struct platform_driver qcom_msm8996_cbf_driver = {
>>   	.probe = qcom_msm8996_cbf_probe,
>> +	.remove = qcom_msm8996_cbf_remove,
>>   	.driver = {
>>   		.name = "qcom-msm8996-cbf",
>>   		.of_match_table = qcom_msm8996_cbf_match_table,
>> +		.sync_state = icc_sync_state,
>>   	},
>>   };
>>   

-- 
With best wishes
Dmitry

Re: [PATCH 3/5] clk: qcom: cbf-msm8996: scale CBF clock according to the CPUfreq

[Konrad Dybcio]

On 12.01.2023 16:29, Dmitry Baryshkov wrote:
> On 12/01/2023 17:00, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:57, Dmitry Baryshkov wrote:
>>> Turn CBF into the interconnect provider. Scale CBF frequency (bandwidth)
>>> according to CPU frequencies.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>> TODO: assess if we should be doing this manually or rely on SPDM..
> 
> Manually, see the msm-3.18's devfreq-cpu and m4m-cpufreq maps.
> 
> SPDM uses CBF as an input not as an output.
Thanks, that makes sense, I keep confusing it!

Konrad
> 
>>
>> Konrad
>>>   drivers/clk/qcom/clk-cbf-8996.c | 141 +++++++++++++++++++++++++++++++-
>>>   1 file changed, 140 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
>>> index bdd4f8b48a7e..76db623b0f92 100644
>>> --- a/drivers/clk/qcom/clk-cbf-8996.c
>>> +++ b/drivers/clk/qcom/clk-cbf-8996.c
>>> @@ -5,6 +5,7 @@
>>>   #include <linux/bitfield.h>
>>>   #include <linux/clk.h>
>>>   #include <linux/clk-provider.h>
>>> +#include <linux/interconnect-provider.h>
>>>   #include <linux/of.h>
>>>   #include <linux/module.h>
>>>   #include <linux/platform_device.h>
>>> @@ -225,6 +226,133 @@ static const struct regmap_config cbf_msm8996_regmap_config = {
>>>       .val_format_endian    = REGMAP_ENDIAN_LITTLE,
>>>   };
>>>   +#ifdef CONFIG_INTERCONNECT
>>> +struct qcom_msm8996_cbf_icc_provider {
>>> +    struct icc_provider provider;
>>> +    struct clk *clk;
>>> +};
>>> +
>>> +#define to_qcom_cbf_provider(_provider) \
>>> +    container_of(_provider, struct qcom_msm8996_cbf_icc_provider, provider)
>>> +
>>> +enum {
>>> +    CBF_MASTER_NODE = 2000,
>>> +    CBF_SLAVE_NODE
>>> +};
>>> +
>>> +#define CBF_NUM_NODES 2
>>> +
>>> +static int qcom_msm8996_cbf_set(struct icc_node *src, struct icc_node *dst)
>>> +{
>>> +    struct qcom_msm8996_cbf_icc_provider *qp;
>>> +
>>> +    qp = to_qcom_cbf_provider(src->provider);
>>> +
>>> +    return clk_set_rate(qp->clk, icc_units_to_bps(dst->peak_bw));
>>> +}
>>> +
>>> +static int qcom_msm8996_cbf_icc_get_bw(struct icc_node *node, u32 *avg, u32 *peak)
>>> +{
>>> +    struct qcom_msm8996_cbf_icc_provider *qp;
>>> +
>>> +    qp = to_qcom_cbf_provider(node->provider);
>>> +    *peak = clk_get_rate(qp->clk) / 1000ULL;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev, struct clk_hw *cbf_hw)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct qcom_msm8996_cbf_icc_provider *qp;
>>> +    struct icc_provider *provider;
>>> +    struct icc_onecell_data *data;
>>> +    struct icc_node *node;
>>> +    struct clk *clk;
>>> +    int ret;
>>> +
>>> +    clk = devm_clk_hw_get_clk(dev, cbf_hw, "cbf");
>>> +    if (IS_ERR(clk))
>>> +        return PTR_ERR(clk);
>>> +
>>> +    data = devm_kzalloc(dev, struct_size(data, nodes, CBF_NUM_NODES), GFP_KERNEL);
>>> +    if (!data)
>>> +        return -ENOMEM;
>>> +
>>> +    data->num_nodes = CBF_NUM_NODES;
>>> +
>>> +    qp = devm_kzalloc(dev, sizeof(*qp), GFP_KERNEL);
>>> +    if (!qp)
>>> +        return -ENOMEM;
>>> +
>>> +    qp->clk = clk;
>>> +
>>> +    provider = &qp->provider;
>>> +    provider->dev = dev;
>>> +    provider->get_bw = qcom_msm8996_cbf_icc_get_bw;
>>> +    provider->set = qcom_msm8996_cbf_set;
>>> +    provider->aggregate = icc_std_aggregate;
>>> +    provider->xlate = of_icc_xlate_onecell;
>>> +    INIT_LIST_HEAD(&provider->nodes);
>>> +    provider->data = data;
>>> +
>>> +    ret = icc_provider_add(provider);
>>> +    if (ret) {
>>> +        dev_err(dev, "error adding interconnect provider\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    node = icc_node_create(CBF_MASTER_NODE);
>>> +    if (IS_ERR(node)) {
>>> +        ret = PTR_ERR(node);
>>> +        goto err;
>>> +    }
>>> +
>>> +    node->name = "cbf_master";
>>> +    icc_node_add(node, provider);
>>> +    icc_link_create(node, CBF_SLAVE_NODE);
>>> +    data->nodes[0] = node;
>>> +
>>> +    node = icc_node_create(CBF_SLAVE_NODE);
>>> +    if (IS_ERR(node)) {
>>> +        ret = PTR_ERR(node);
>>> +        goto err;
>>> +    }
>>> +
>>> +    node->name = "cbf_slave";
>>> +    icc_node_add(node, provider);
>>> +    data->nodes[1] = node;
>>> +
>>> +    platform_set_drvdata(pdev, provider);
>>> +
>>> +    return 0;
>>> +
>>> +err:
>>> +    icc_nodes_remove(provider);
>>> +    icc_provider_del(provider);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int qcom_msm8996_cbf_icc_remove(struct platform_device *pdev)
>>> +{
>>> +    struct icc_provider *provider = platform_get_drvdata(pdev);
>>> +
>>> +    icc_nodes_remove(provider);
>>> +    icc_provider_del(provider);
>>> +
>>> +    return 0;
>>> +}
>>> +#else
>>> +static int qcom_msm8996_cbf_icc_register(struct platform_device *pdev)
>>> +{
>>> +    dev_warn(&pdev->dev, "interconnects support is disabled, CBF clock is fixed\n");
>>> +
>>> +    return 0;
>>> +}
>>> +#define qcom_msm8996_cbf_icc_remove(pdev) (0)
>>> +#endif
>>> +
>>>   static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
>>>   {
>>>       void __iomem *base;
>>> @@ -284,7 +412,16 @@ static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
>>>       if (ret)
>>>           return ret;
>>>   -    return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
>>> +    ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return qcom_msm8996_cbf_icc_register(pdev, &cbf_mux.clkr.hw);
>>> +}
>>> +
>>> +static int qcom_msm8996_cbf_remove(struct platform_device *pdev)
>>> +{
>>> +    return qcom_msm8996_cbf_icc_remove(pdev);
>>>   }
>>>     static const struct of_device_id qcom_msm8996_cbf_match_table[] = {
>>> @@ -295,9 +432,11 @@ MODULE_DEVICE_TABLE(of, qcom_msm8996_cbf_match_table);
>>>     static struct platform_driver qcom_msm8996_cbf_driver = {
>>>       .probe = qcom_msm8996_cbf_probe,
>>> +    .remove = qcom_msm8996_cbf_remove,
>>>       .driver = {
>>>           .name = "qcom-msm8996-cbf",
>>>           .of_match_table = qcom_msm8996_cbf_match_table,
>>> +        .sync_state = icc_sync_state,
>>>       },
>>>   };
>>>   
> 

Re: [PATCH 4/5] arm64: dts: qcom: msm8996: add CBF device entry

[Dmitry Baryshkov]

On 12/01/2023 17:05, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:57, Dmitry Baryshkov wrote:
>> Add device tree node for the CBF clock.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   arch/arm64/boot/dts/qcom/msm8996.dtsi | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> index 37ebd8a3b2c8..e5a638b85ffc 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> @@ -3558,6 +3558,13 @@ saw3: syscon@9a10000 {
>>   			reg = <0x09a10000 0x1000>;
>>   		};
>>   
>> +		cbf: clock-controller@9a11000 {
>> +			compatible = "qcom,msm8996-cbf";
>> +			reg = <0x09a11000 0x10000>;
>> +			clocks = <&rpmcc RPM_SMD_BB_CLK1>, <&apcs_glb>;
> Shouldn't this be RPM_SMD_XO_A_CLK_SRC? I don't see BB_CLK
> referenced *anywhere* downstream, other than being defined
> in the 8996 GCC driver..

Good question. On the schematics I have the CXO input is connected to 
the BBCLK1 PMIC pin.

> 
> Konrad
> 
>> +			#clock-cells = <0>;
>> +		};
>> +
>>   		intc: interrupt-controller@9bc0000 {
>>   			compatible = "qcom,msm8996-gic-v3", "arm,gic-v3";
>>   			#interrupt-cells = <3>;

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] arm64: dts: qcom: msm8996: add CBF device entry

[Konrad Dybcio]

On 12.01.2023 16:38, Dmitry Baryshkov wrote:
> On 12/01/2023 17:05, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:57, Dmitry Baryshkov wrote:
>>> Add device tree node for the CBF clock.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   arch/arm64/boot/dts/qcom/msm8996.dtsi | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>>> index 37ebd8a3b2c8..e5a638b85ffc 100644
>>> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>>> @@ -3558,6 +3558,13 @@ saw3: syscon@9a10000 {
>>>               reg = <0x09a10000 0x1000>;
>>>           };
>>>   +        cbf: clock-controller@9a11000 {
>>> +            compatible = "qcom,msm8996-cbf";
>>> +            reg = <0x09a11000 0x10000>;
>>> +            clocks = <&rpmcc RPM_SMD_BB_CLK1>, <&apcs_glb>;
>> Shouldn't this be RPM_SMD_XO_A_CLK_SRC? I don't see BB_CLK
>> referenced *anywhere* downstream, other than being defined
>> in the 8996 GCC driver..
> 
> Good question. On the schematics I have the CXO input is connected to the BBCLK1 PMIC pin.
I guess it's a question of how RPM resolves this internally.. I
think I was once told that you just ask RPM nicely to turn on
the clock and everything is handled for you from there on,
so perhaps we don't need to poke at BBCLK manually if it's only
used as XO parent?

Konrad
> 
>>
>> Konrad
>>
>>> +            #clock-cells = <0>;
>>> +        };
>>> +
>>>           intc: interrupt-controller@9bc0000 {
>>>               compatible = "qcom,msm8996-gic-v3", "arm,gic-v3";
>>>               #interrupt-cells = <3>;
> 

Re: [PATCH 2/5] clk: qcom: add msm8996 Core Bus Framework (CBF) support

[Dmitry Baryshkov]

On 12/01/2023 16:58, Konrad Dybcio wrote:
> 
> 
> On 11.01.2023 20:57, Dmitry Baryshkov wrote:
>> Add CBF clock driver as a part of MSM8996 CPU clocks. Significantly
>> based on AngeloGioacchino del Regno's work at [1].
>>
>> The CBF is an interconnect between two CPU clusters, setting it up
>> properly is required for booting the MSM8996 with all four cores
>> enabled.
>>
>> [1] https://github.com/sonyxperiadev/kernel/blob/aosp/LE.UM.2.3.2.r1.4/drivers/clk/qcom/clk-cpu-8996.c
>>
>> Co-developed-by: Konrad Dybcio <konrad.dybcio@somainline.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/Makefile       |   2 +-
>>   drivers/clk/qcom/clk-cbf-8996.c | 318 ++++++++++++++++++++++++++++++++
>>   2 files changed, 319 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/clk/qcom/clk-cbf-8996.c
>>
>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index ca2f586edb3e..d6ecd2bf7040 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -52,7 +52,7 @@ obj-$(CONFIG_MSM_MMCC_8998) += mmcc-msm8998.o
>>   obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
>>   obj-$(CONFIG_QCOM_A7PLL) += a7-pll.o
>>   obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
>> -obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += apcs-msm8996.o clk-cpu-8996.o
>> +obj-$(CONFIG_QCOM_CLK_APCC_MSM8996) += apcs-msm8996.o clk-cpu-8996.o clk-cbf-8996.o
>>   obj-$(CONFIG_QCOM_CLK_APCS_SDX55) += apcs-sdx55.o
>>   obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
>>   obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
>> diff --git a/drivers/clk/qcom/clk-cbf-8996.c b/drivers/clk/qcom/clk-cbf-8996.c
>> new file mode 100644
>> index 000000000000..bdd4f8b48a7e
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-cbf-8996.c
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022, Linaro Ltd.
> Happy new year!
> 
>> + */
>> +#include <linux/bitfield.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/of.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "clk-alpha-pll.h"
>> +#include "clk-regmap.h"
>> +
>> +/* Need to match the order of clocks in DT binding */
>> +enum {
>> +	DT_XO,
> No, it should be XO_A, otherwise the platform will never sleep.
> 
> +	DT_APCS_AUX,
>> +};
>> +
>> +enum {
>> +	CBF_XO_INDEX,
>> +	CBF_PLL_INDEX,
>> +	CBF_DIV_INDEX,
>> +	CBF_APCS_AUX_INDEX,
>> +};
>> +
>> +#define DIV_THRESHOLD		600000000
>> +
>> +#define CBF_MUX_OFFSET		0x18
>> +#define CBF_MUX_PARENT_MASK		GENMASK(1, 0)
>> +#define CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK GENMASK(5, 4)
>> +#define CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL \
>> +	FIELD_PREP(CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK, 0x03)
>> +#define CBF_MUX_AUTO_CLK_SEL_BIT	BIT(6)
>> +
>> +#define CBF_PLL_OFFSET 0xf000
>> +
>> +static const u8 cbf_pll_regs[PLL_OFF_MAX_REGS] = {
>> +	[PLL_OFF_L_VAL] = 0x08,
>> +	[PLL_OFF_ALPHA_VAL] = 0x10,
>> +	[PLL_OFF_USER_CTL] = 0x18,
>> +	[PLL_OFF_CONFIG_CTL] = 0x20,
>> +	[PLL_OFF_CONFIG_CTL_U] = 0x24,
>> +	[PLL_OFF_TEST_CTL] = 0x30,
>> +	[PLL_OFF_TEST_CTL_U] = 0x34,
>> +	[PLL_OFF_STATUS] = 0x28,
>> +};
>> +
>> +static const struct alpha_pll_config cbfpll_config = {
>> +	.l = 72,
>> +	.config_ctl_val = 0x200d4828,
>> +	.config_ctl_hi_val = 0x006,
>> +	.test_ctl_val = 0x1c000000,
>> +	.test_ctl_hi_val = 0x00004000,
>> +	.pre_div_mask = BIT(12),
>> +	.post_div_mask = 0x3 << 8,
>> +	.post_div_val = 0x1 << 8,
>> +	.main_output_mask = BIT(0),
>> +	.early_output_mask = BIT(3),
>> +};
>> +
>> +static struct clk_alpha_pll cbf_pll = {
>> +	.offset = CBF_PLL_OFFSET,
>> +	.regs = cbf_pll_regs,
>> +	.flags = SUPPORTS_DYNAMIC_UPDATE | SUPPORTS_FSM_MODE,
>> +	.clkr.hw.init = &(struct clk_init_data){
>> +		.name = "cbf_pll",
>> +		.parent_data = (const struct clk_parent_data[]) {
>> +			{ .index = DT_XO, },
>> +		},
>> +		.num_parents = 1,
>> +		.ops = &clk_alpha_pll_hwfsm_ops,
>> +	},
>> +};
>> +
>> +static struct clk_fixed_factor cbf_pll_postdiv = {
>> +	.mult = 1,
>> +	.div = 2,
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "cbf_pll_postdiv",
>> +		.parent_data = &(const struct clk_parent_data){
> parent_hws?

Ack

> 
>> +			.hw = &cbf_pll.clkr.hw
>> +		},
>> +		.num_parents = 1,
>> +		.ops = &clk_fixed_factor_ops,
>> +		.flags = CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
>> +static const struct clk_parent_data cbf_mux_parent_data[] = {
>> +	{ .index = DT_XO },
>> +	{ .hw = &cbf_pll.clkr.hw },
>> +	{ .hw = &cbf_pll_postdiv.hw },
>> +	{ .index = DT_APCS_AUX },
>> +};
>> +
>> +struct clk_cbf_8996_mux {
>> +	u32 reg;
>> +	struct notifier_block nb;
>> +	struct clk_regmap clkr;
>> +};
>> +
>> +static struct clk_cbf_8996_mux *to_clk_cbf_8996_mux(struct clk_regmap *clkr)
>> +{
>> +	return container_of(clkr, struct clk_cbf_8996_mux, clkr);
>> +}
>> +
>> +static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
>> +			       void *data);
> Please align the indentation with the opening bracket.
> 
>> +
>> +static u8 clk_cbf_8996_mux_get_parent(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap *clkr = to_clk_regmap(hw);
>> +	struct clk_cbf_8996_mux *mux = to_clk_cbf_8996_mux(clkr);
>> +	u32 val;
>> +
>> +	regmap_read(clkr->regmap, mux->reg, &val);
>> +
>> +	return FIELD_GET(CBF_MUX_PARENT_MASK, val);
>> +}
>> +
>> +static int clk_cbf_8996_mux_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +	struct clk_regmap *clkr = to_clk_regmap(hw);
>> +	struct clk_cbf_8996_mux *mux = to_clk_cbf_8996_mux(clkr);
>> +	u32 val;
>> +
>> +	val = FIELD_PREP(CBF_MUX_PARENT_MASK, index);
>> +
>> +	return regmap_update_bits(clkr->regmap, mux->reg, CBF_MUX_PARENT_MASK, val);
>> +}
>> +
>> +static int clk_cbf_8996_mux_determine_rate(struct clk_hw *hw,
>> +					   struct clk_rate_request *req)
>> +{
>> +	struct clk_hw *parent;
>> +
>> +	if (req->rate < (DIV_THRESHOLD / 2))
>> +		return -EINVAL;
> Shouldn't you parent it to APCS AUX here?

No, frequencies less than 300 MHz are not supported at all, they are not 
listed in the tables. I think returning an error is more appropriate 
here than setting the clock to the fixed 300 MHz.

> 
>> +
>> +	if (req->rate < DIV_THRESHOLD)
>> +		parent = clk_hw_get_parent_by_index(hw, CBF_DIV_INDEX);
>> +	else
>> +		parent = clk_hw_get_parent_by_index(hw, CBF_PLL_INDEX);
>> +
>> +	if (!parent)
>> +		return -EINVAL;
>> +
>> +	req->best_parent_rate = clk_hw_round_rate(parent, req->rate);
>> +	req->best_parent_hw = parent;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct clk_ops clk_cbf_8996_mux_ops = {
>> +	.set_parent = clk_cbf_8996_mux_set_parent,
>> +	.get_parent = clk_cbf_8996_mux_get_parent,
>> +	.determine_rate = clk_cbf_8996_mux_determine_rate,
>> +};
>> +
>> +static struct clk_cbf_8996_mux cbf_mux = {
>> +	.reg = CBF_MUX_OFFSET,
>> +	.nb.notifier_call = cbf_clk_notifier_cb,
>> +	.clkr.hw.init = &(struct clk_init_data) {
>> +		.name = "cbf_mux",
>> +		.parent_data = cbf_mux_parent_data,
>> +		.num_parents = ARRAY_SIZE(cbf_mux_parent_data),
>> +		.ops = &clk_cbf_8996_mux_ops,
>> +		/* CPU clock is critical and should never be gated */
>> +		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>> +	},
>> +};
>> +
>> +static int cbf_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
>> +			       void *data)
>> +{
>> +	struct clk_notifier_data *cnd = data;
>> +	int ret;
>> +
>> +	switch (event) {
>> +	case PRE_RATE_CHANGE:
>> +		/*
>> +		 * Avoid overvolting. clk_core_set_rate_nolock() walks from top
>> +		 * to bottom, so it will change the rate of the PLL before
>> +		 * chaging the parent of PMUX. This can result in pmux getting
>> +		 * clocked twice the expected rate.
>> +		 *
>> +		 * Manually switch to PLL/2 here.
>> +		 */
>> +		if (cnd->old_rate > DIV_THRESHOLD &&
>> +		    cnd->new_rate < DIV_THRESHOLD)
>> +			clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_DIV_INDEX);
>> +		break;
>> +	case ABORT_RATE_CHANGE:
>> +		/* Revert manual change */
>> +		if (cnd->new_rate < DIV_THRESHOLD &&
>> +		    cnd->old_rate > DIV_THRESHOLD)
>> +			clk_cbf_8996_mux_set_parent(&cbf_mux.clkr.hw, CBF_PLL_INDEX);
>> +		break;
>> +	default:
>> +		ret = 0;
>> +		break;
>> +	}
>> +
>> +	return notifier_from_errno(ret);
>> +};
>> +
>> +static struct clk_hw *cbf_msm8996_hw_clks[] = {
>> +	&cbf_pll_postdiv.hw,
>> +};
>> +
>> +static struct clk_regmap *cbf_msm8996_clks[] = {
>> +	&cbf_pll.clkr,
>> +	&cbf_mux.clkr,
>> +};
>> +
>> +static const struct regmap_config cbf_msm8996_regmap_config = {
>> +	.reg_bits		= 32,
>> +	.reg_stride		= 4,
>> +	.val_bits		= 32,
>> +	.max_register		= 0x10000,
>> +	.fast_io		= true,
>> +	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
>> +};
>> +
>> +static int qcom_msm8996_cbf_probe(struct platform_device *pdev)
>> +{
>> +	void __iomem *base;
>> +	struct regmap *regmap;
>> +	struct device *dev = &pdev->dev;
>> +	int i, ret;
>> +
>> +	base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	regmap = devm_regmap_init_mmio(dev, base, &cbf_msm8996_regmap_config);
>> +	if (IS_ERR(regmap))
>> +		return PTR_ERR(regmap);
>> +
>> +	/* Select GPLL0 for 300MHz for the CBF clock */
>> +	regmap_write(regmap, CBF_MUX_OFFSET, 0x3);
>> +
>> +	/* Ensure write goes through before PLLs are reconfigured */
>> +	udelay(5);
>> +
>> +	/* Set the auto clock sel always-on source to GPLL0/2 (300MHz) */
>> +	regmap_update_bits(regmap, CBF_MUX_OFFSET,
>> +			   CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_MASK,
>> +			   CBF_MUX_AUTO_CLK_SEL_ALWAYS_ON_GPLL0_SEL);
>> +
>> +	clk_alpha_pll_configure(&cbf_pll, regmap, &cbfpll_config);
>> +
>> +	/* Wait for PLL(s) to lock */
>> +        udelay(50);
> Weird indentation?
> 
>> +
>> +	/* Enable auto clock selection for CBF */
>> +	regmap_update_bits(regmap, CBF_MUX_OFFSET,
>> +			   CBF_MUX_AUTO_CLK_SEL_BIT,
>> +			   CBF_MUX_AUTO_CLK_SEL_BIT);
>> +
>> +	/* Ensure write goes through before muxes are switched */
>> +	udelay(5);
>> +
>> +	/* Switch CBF to use the primary PLL */
>> +	regmap_update_bits(regmap, CBF_MUX_OFFSET,
>> +			   CBF_MUX_PARENT_MASK, 0x1);
> I think you can unwrap this.
> 
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cbf_msm8996_hw_clks); i++) {
>> +		ret = devm_clk_hw_register(dev, cbf_msm8996_hw_clks[i]);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(cbf_msm8996_clks); i++) {
>> +		ret = devm_clk_register_regmap(dev, cbf_msm8996_clks[i]);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	ret = devm_clk_notifier_register(dev, cbf_mux.clkr.hw.clk, &cbf_mux.nb);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &cbf_mux.clkr.hw);
>> +}
>> +
>> +static const struct of_device_id qcom_msm8996_cbf_match_table[] = {
>> +	{ .compatible = "qcom,msm8996-cbf" },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_msm8996_cbf_match_table);
>> +
>> +static struct platform_driver qcom_msm8996_cbf_driver = {
>> +	.probe = qcom_msm8996_cbf_probe,
>> +	.driver = {
>> +		.name = "qcom-msm8996-cbf",
>> +		.of_match_table = qcom_msm8996_cbf_match_table,
>> +	},
>> +};
>> +
>> +/* Register early enough to fix the clock to be used for other cores */
>> +static int __init qcom_msm8996_cbf_init(void)
>> +{
>> +	return platform_driver_register(&qcom_msm8996_cbf_driver);
>> +}
>> +postcore_initcall(qcom_msm8996_cbf_init);
>> +
>> +static void __exit qcom_msm8996_cbf_exit(void)
>> +{
>> +	platform_driver_unregister(&qcom_msm8996_cbf_driver);
>> +}
>> +module_exit(qcom_msm8996_cbf_exit);
>> +
>> +MODULE_DESCRIPTION("QCOM MSM8996 CPU Bus Fabric Clock Driver");
>> +MODULE_LICENSE("GPL v2");
> "GPL"

Ack

> 
> The rest looks good..
> 
> Konrad

-- 
With best wishes
Dmitry