On 02/05/2022 13:10, Manivannan Sadhasivam wrote:
On Sun, May 01, 2022 at 10:21:46PM +0300, Dmitry Baryshkov wrote:
On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
muxes which must be parked to the "safe" source (bi_tcxo) when
corresponding GDSC is turned off and on again. Currently this is
handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
clock. However the same code sequence should be applied in the
pcie-qcom endpoint, USB3 and UFS drivers.
Rather than copying this sequence over and over again, follow the
example of clk_rcg2_shared_ops and implement this parking in the
enable() and disable() clock operations. Suppliement the regmap-mux with
the new regmap-pipe implementation, which hides multiplexer behind
simple branch-like clock. This is possible since each of this
multiplexers has just two clock sources: working (pipe) and safe
(bi_tcxo) clock sources. If the clock is running off the external pipe
source, report it as enable and report it as disabled otherwise.
Sorry for chiming in late and providing comments that might have been addressed
before. But I have few questions below:
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
drivers/clk/qcom/Makefile | 1 +
drivers/clk/qcom/clk-regmap-pipe.c | 62 ++++++++++++++++++++++++++++++
drivers/clk/qcom/clk-regmap-pipe.h | 24 ++++++++++++
3 files changed, 87 insertions(+)
create mode 100644 drivers/clk/qcom/clk-regmap-pipe.c
create mode 100644 drivers/clk/qcom/clk-regmap-pipe.h
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 671cf5821af1..882c8ecc2e93 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -11,6 +11,7 @@ clk-qcom-y += clk-branch.o
clk-qcom-y += clk-regmap-divider.o
clk-qcom-y += clk-regmap-mux.o
clk-qcom-y += clk-regmap-mux-div.o
+clk-qcom-y += clk-regmap-pipe.o
clk-qcom-$(CONFIG_KRAIT_CLOCKS) += clk-krait.o
clk-qcom-y += clk-hfpll.o
clk-qcom-y += reset.o
diff --git a/drivers/clk/qcom/clk-regmap-pipe.c b/drivers/clk/qcom/clk-regmap-pipe.c
new file mode 100644
index 000000000000..9a7c27cc644b
--- /dev/null
+++ b/drivers/clk/qcom/clk-regmap-pipe.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>
+
+#include "clk-regmap-pipe.h"
+
+static inline struct clk_regmap_pipe *to_clk_regmap_pipe(struct clk_hw *hw)
+{
+ return container_of(to_clk_regmap(hw), struct clk_regmap_pipe, clkr);
+}
+
+static int pipe_is_enabled(struct clk_hw *hw)
+{
+ struct clk_regmap_pipe *pipe = to_clk_regmap_pipe(hw);
+ struct clk_regmap *clkr = to_clk_regmap(hw);
+ unsigned int mask = GENMASK(pipe->width + pipe->shift - 1, pipe->shift);
+ unsigned int val;
+
+ regmap_read(clkr->regmap, pipe->reg, &val);
+ val = (val & mask) >> pipe->shift;
+
+ WARN_ON(unlikely(val != pipe->enable_val && val != pipe->disable_val));
+
+ return val == pipe->enable_val;
Selecting the clk parents in the enable/disable callback seems fine to me but
the way it is implemented doesn't look right.
First this "pipe_clksrc" is a mux clk by design, since we can only select the
parent. But you are converting it to a gate clk now.
Instead of that, my proposal would be to make this clk a composite one i.e,.
gate clk + mux clk. So even though the gate clk here would be a hack, we are
not changing the definition of mux clk.
This is what I had before, in revisions 1-3. Which proved to work, but
is problematic a bit.
In the very end, it is not easily possible to make a difference between
a clock reparented to the bi_tcxo and a disabled clock. E.g. if some
user reparents the clock to the tcxo, then the driver will consider the
clock disabled, but the clock framework will think that the clock is
still enabled.
Thus we have to remove "safe" clock (bi_tcxo) from the list of parents.
In case of pipe clocks (and ufs symbol clocks) this will leave us with
just a single possible parent. Then having the mux part just doesn't
make sense. It is just a gated clock. And this simplified a lot of things.
So you can introduce a new ops like "clk_regmap_mux_gate_ops" and implement the
parent switching logic in the enable/disable callbacks. Additional benefit of
this ops is, in the future we can also support "gate + mux" clks easily.
If the need arises, we can easily resurrect the regmap_mux_safe
patchset, fix the race pointed out by Johan, remove extra src-val
mapping for safe value and use it for such clocks. I can post it
separately, if you wish. But I'm not sure that it makes sense to use it
for single-parent clocks.
Also, please don't use the "enable_val/disable_val" members. It should be
something like "mux_sel_pre/mux_sel_post".
Why? Could you please elaborate?
+}
+
+static int pipe_enable(struct clk_hw *hw)
+{
+ struct clk_regmap_pipe *pipe = to_clk_regmap_pipe(hw);
+ struct clk_regmap *clkr = to_clk_regmap(hw);
+ unsigned int mask = GENMASK(pipe->width + pipe->shift - 1, pipe->shift);
+ unsigned int val;
+
+ val = pipe->enable_val << pipe->shift;
+
+ return regmap_update_bits(clkr->regmap, pipe->reg, mask, val);
+}
+
+static void pipe_disable(struct clk_hw *hw)
+{
+ struct clk_regmap_pipe *pipe = to_clk_regmap_pipe(hw);
+ struct clk_regmap *clkr = to_clk_regmap(hw);
+ unsigned int mask = GENMASK(pipe->width + pipe->shift - 1, pipe->shift);
+ unsigned int val;
+
+ val = pipe->disable_val << pipe->shift;
+
+ regmap_update_bits(clkr->regmap, pipe->reg, mask, val);
+}
+
+const struct clk_ops clk_regmap_pipe_ops = {
+ .enable = pipe_enable,
+ .disable = pipe_disable,
+ .is_enabled = pipe_is_enabled,
+};
+EXPORT_SYMBOL_GPL(clk_regmap_pipe_ops);
diff --git a/drivers/clk/qcom/clk-regmap-pipe.h b/drivers/clk/qcom/clk-regmap-pipe.h
new file mode 100644
index 000000000000..cfaa792a029b
--- /dev/null
+++ b/drivers/clk/qcom/clk-regmap-pipe.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022, Linaru Ltd.
Linaro
ack
+ * Author: Dmitry Baryshkov
No email?
Thanks,
Mani
+ */
+
+#ifndef __QCOM_CLK_REGMAP_PIPE_H__
+#define __QCOM_CLK_REGMAP_PIPE_H__
+
+#include <linux/clk-provider.h>
+#include "clk-regmap.h"
+
+struct clk_regmap_pipe {
+ u32 reg;
+ u32 shift;
+ u32 width;
+ u32 enable_val;
+ u32 disable_val;
+ struct clk_regmap clkr;
+};
+
+extern const struct clk_ops clk_regmap_pipe_ops;
+
+#endif
--
2.35.1
--
With best wishes
Dmitry
