On 06/05/2022 15:31, Johan Hovold 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.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
I think this is much better and it addresses most of my concerns with
the previous approach by keeping things simple and using a dedicated
implementation (i.e. separate from regmap-mux).
The purpose of the clock implementation can be documented in the source
and is reflected in the naming. It avoids the issues related to the
caching (locking and deferred muxing) which wasn't really needed in the
first place as these muxes are binary.
By implementing is_enabled() you also allow for inspecting the state
that the boot firmware left the mux in.
The only thing that comes to mind that wouldn't be possible is to
set the mux state using an assigned clock parent in devicetree to make
sure that XO is always selected before toggling the GDSC at probe.
But since that doesn't seem to work anyway when the boot firmware has
set things up (e.g. causes a modem here to reset) that would probably
need to be handled in the GDSC driver anyway (i.e. make sure the source
is XO before enabling the GDSC but only when it was actually disabled).
Taking that one step further would be to implement all this in the GDSC
driver from the start so that the PHY PLL is always muxed in while the
power domain is enabled (and only then)...
I think, if we move this to the gdsc driver, we'd loose the part of the
clock tree.
---
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);
Since pipe is so overloaded already can we call this "pipe_mux" or
"pipe_src" instead of just "pipe"?
I'd settle on the pipe_src then.
If you don't mind, I'll wait for your Tested-by and will post the rename
patchset afterwards.
And similarly for
pipe_mux_is_enabled()
struct clk_regmap_pipe_mux
struct clk_regmap_pipe_mux_ops
etc.
+ 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));
This is not a hot path and there's rarely a need for unlikely().
Ack.
+
+ return val == pipe->enable_val;
+}
Johan
--
With best wishes
Dmitry
