Re: [PATCH v3 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SC7180
From: Taniya Das
Date: Wed Sep 25 2019 - 07:20:23 EST
Hi Stephen,
Please find my comments.
On 9/25/2019 4:42 AM, Stephen Boyd wrote:
Quoting Taniya Das (2019-09-23 01:01:11)
Hi Stephen,
Thanks for your comments.
On 9/19/2019 3:09 AM, Stephen Boyd wrote:
Quoting Taniya Das (2019-09-18 02:50:18)
diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
new file mode 100644
index 000000000000..d47865d5408f
--- /dev/null
+++ b/drivers/clk/qcom/gcc-sc7180.c
@@ -0,0 +1,2515 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
include clk-provider.h
will add this header.
Currently the <drivers/clk/qcom/clk-regmap.h> already includes it.
Yes but it should be included in any clk-provider drivers too.
+
+/* Leave the clock ON for parent config_noc_clk to be kept enabled */
+static struct clk_branch gcc_disp_ahb_clk = {
+ .halt_reg = 0xb00c,
+ .halt_check = BRANCH_HALT,
+ .hwcg_reg = 0xb00c,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0xb00c,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_disp_ahb_clk",
+ .flags = CLK_IS_CRITICAL,
Does this assume the display is left enabled out of the bootloader? Why
is this critical to system operation? Maybe it is really
CLK_IGNORE_UNUSED?
This clock is not kept enabled by bootloader. But leaving this ON for
clients on config noc.
Please see below comment for the other critical clk with no parent.
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
[...]
+static struct clk_branch gcc_ufs_phy_phy_aux_clk = {
+ .halt_reg = 0x77094,
+ .halt_check = BRANCH_HALT,
+ .hwcg_reg = 0x77094,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0x77094,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_ufs_phy_phy_aux_clk",
+ .parent_data = &(const struct clk_parent_data){
+ .hw = &gcc_ufs_phy_phy_aux_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch gcc_ufs_phy_rx_symbol_0_clk = {
+ .halt_reg = 0x7701c,
+ .halt_check = BRANCH_HALT_SKIP,
Again, nobody has fixed the UFS driver to not need to do this halt skip
check for these clks? It's been over a year.
The UFS_PHY_RX/TX clocks could be left enabled due to certain HW boot
configuration and thus during the late initcall of clk_disable there
could be warnings of "clock stuck ON" in the dmesg. That is the reason
also to use the BRANCH_HALT_SKIP flag.
Oh that's bad. Why do the clks stay on when we try to turn them off?
Those could be due to the configuration selected by HW and SW cannot
override them, so traditionally we have never polled for CLK_OFF for
these clocks.
I would also check internally for the UFS driver fix you are referring here.
Sure. I keep asking but nothing is done :(
+ .clkr = {
+ .enable_reg = 0x7701c,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_ufs_phy_rx_symbol_0_clk",
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
[...]
+
+static struct clk_branch gcc_usb3_prim_phy_pipe_clk = {
+ .halt_reg = 0xf058,
+ .halt_check = BRANCH_HALT_SKIP,
Why does this need halt_skip?
This is required as the source is external PHY, so we want to not check
for HALT.
This doesn't really answer my question. If the source is an external phy
then it should be listed as a clock in the DT binding and the parent
should be specified here. Unless something doesn't work because of that?
The USB phy is managed by the USB driver and clock driver is not aware
if USB driver models the phy as a clock. Thus we do want to keep a
dependency on the parent and not poll for CLK_ENABLE.
+ .clkr = {
+ .enable_reg = 0xf058,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_usb3_prim_phy_pipe_clk",
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch gcc_usb_phy_cfg_ahb2phy_clk = {
+ .halt_reg = 0x6a004,
+ .halt_check = BRANCH_HALT,
+ .hwcg_reg = 0x6a004,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0x6a004,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_usb_phy_cfg_ahb2phy_clk",
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+/* Leave the clock ON for parent config_noc_clk to be kept enabled */
There's no parent though... So I guess this means it keeps it enabled
implicitly in hardware?
These are not left enabled, but want to leave them enabled for clients
on config NOC.
Sure. It just doesn't make sense to create clk structures and expose
them in the kernel when we just want to turn the bits on and leave them
on forever. Why not just do some register writes in probe for this
driver? Doesn't that work just as well and use less memory?
Even if I write these registers during probe, the late init check
'clk_core_is_enabled' would return true and would be turned OFF, that is
the reason for marking them CRITICAL.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--