On 18/05/2022 20:58, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2022-05-13 10:53:36)
diff --git a/drivers/clk/qcom/clk-regmap-phy-mux.c b/drivers/clk/qcom/clk-regmap-phy-mux.c
new file mode 100644
index 000000000000..d7a45f7fa1aa
--- /dev/null
+++ b/drivers/clk/qcom/clk-regmap-phy-mux.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022, Linaro Ltd.
+ */
+
+#include <linux/kernel.h>
+#include <linux/bitops.h>
+#include <linux/regmap.h>
+#include <linux/export.h>
clk-provider.h for clk_hw/clk_ops usage. It helps with grep to identify
clk providers.
+
+#include "clk-regmap-phy-mux.h"
Same for clk-regmap.h, avoid include hell.
I couldn't catch this comment. I think we need clk-regmap.h in
clk-regmap-phy-mux.h as clk_regmap is a part of defined structure.
+
+static inline struct clk_regmap_phy_mux *to_clk_regmap_phy_mux(struct clk_hw *hw)
+{
+ return container_of(to_clk_regmap(hw), struct clk_regmap_phy_mux, clkr);
+}
+
+static int phy_mux_is_enabled(struct clk_hw *hw)
+{
+ struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
+ struct clk_regmap *clkr = to_clk_regmap(hw);
+ unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
+ unsigned int val;
+
+ regmap_read(clkr->regmap, phy_mux->reg, &val);
+ val = (val & mask) >> phy_mux->shift;
Can this use FIELD_GET?
+
+ WARN_ON(val != phy_mux->phy_src_val && val != phy_mux->ref_src_val);
+
+ return val == phy_mux->phy_src_val;
+}
+
+static int phy_mux_enable(struct clk_hw *hw)
+{
+ struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
+ struct clk_regmap *clkr = to_clk_regmap(hw);
+ unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
+ unsigned int val;
+
+ val = phy_mux->phy_src_val << phy_mux->shift;
Can this use FIELD_PREP?
+
+ return regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
+}
+
+static void phy_mux_disable(struct clk_hw *hw)
+{
+ struct clk_regmap_phy_mux *phy_mux = to_clk_regmap_phy_mux(hw);
+ struct clk_regmap *clkr = to_clk_regmap(hw);
+ unsigned int mask = GENMASK(phy_mux->width + phy_mux->shift - 1, phy_mux->shift);
+ unsigned int val;
+
+ val = phy_mux->ref_src_val << phy_mux->shift;
+
+ regmap_update_bits(clkr->regmap, phy_mux->reg, mask, val);
+}
+
+const struct clk_ops clk_regmap_phy_mux_ops = {
+ .enable = phy_mux_enable,
+ .disable = phy_mux_disable,
+ .is_enabled = phy_mux_is_enabled,
+};
+EXPORT_SYMBOL_GPL(clk_regmap_phy_mux_ops);
diff --git a/drivers/clk/qcom/clk-regmap-phy-mux.h b/drivers/clk/qcom/clk-regmap-phy-mux.h
new file mode 100644
index 000000000000..6260912191c5
--- /dev/null
+++ b/drivers/clk/qcom/clk-regmap-phy-mux.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022, Linaro Ltd.
+ * Author: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
+ */
+
+#ifndef __QCOM_CLK_REGMAP_PHY_MUX_H__
+#define __QCOM_CLK_REGMAP_PHY_MUX_H__
+
+#include <linux/clk-provider.h>
+#include "clk-regmap.h"
+
+/*
+ * A special clock implementation for PHY pipe and symbols clock sources.
Remove "special" please. Everything is special :)
+ *
+ * If the clock is running off the from-PHY source, report it as enabled.
from-PHY is @phy_src_val? Maybe add that information like "from-PHY
source (@phy_src_val)"
+ * Report it as disabled otherwise (if it uses reference source).
Same for @ref_src_val
+ *
+ * This way the PHY will disable the pipe clock before turning off the GDSC,
+ * which in turn would lead to disabling corresponding pipe_clk_src (and thus
+ * it being parked to a safe, reference clock source). And vice versa, after
+ * enabling the GDSC the PHY will enable the pipe clock, which would cause
+ * pipe_clk_src to be switched from a safe source to the working one.
Might as well make it into real kernel-doc at the same time.
+ */
+
+struct clk_regmap_phy_mux {
+ u32 reg;
+ u32 shift;
+ u32 width;
Technically neither of these need to be u32 and could be u8 to save a
byte or two. The other thing is that possibly the width and shift never
changes? The RCG layout is pretty well fixed. Does hardcoding it work?
+ u32 phy_src_val;
+ u32 ref_src_val;
I feel like "_val" is redundant. Just "ref_src" and "phy_src"? Shorter
is nice.
+ struct clk_regmap clkr;
+};
+
+extern const struct clk_ops clk_regmap_phy_mux_ops;
--
With best wishes
Dmitry
