Re: [PATCH v3 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845
Quoting Amit Nischal (2018-04-18 06:03:49)
> On 2018-04-17 09:21, Stephen Boyd wrote:
> > Quoting Amit Nischal (2018-04-03 06:22:41)
> >> +
> >> +static struct clk_rcg2 gcc_usb30_prim_mock_utmi_clk_src = {
> >> + .cmd_rcgr = 0xf030,
> >> + .mnd_width = 0,
> >> + .hid_width = 5,
> >> + .parent_map = gcc_parent_map_0,
> >> + .freq_tbl = ftbl_gcc_usb30_prim_mock_utmi_clk_src,
> >> + .clkr.hw.init = &(struct clk_init_data){
> >> + .name = "gcc_usb30_prim_mock_utmi_clk_src",
> >> + .parent_names = gcc_parent_names_0,
> >> + .num_parents = 4,
> >> + .ops = &clk_rcg2_shared_ops,
> >
> > Still shared? Why?
>
> We would require the shared_ops for clocks which are configured by
> bootloader.
Why? The bootloader is not active anymore.
> >
> >> +
> >> + return qcom_cc_really_probe(pdev, &gcc_sdm845_desc, regmap);
> >> +}
> >> +
> >> diff --git a/include/dt-bindings/clock/qcom,gcc-sdm845.h
> >> b/include/dt-bindings/clock/qcom,gcc-sdm845.h
> >> new file mode 100644
> >> index 000..e27d8e2
> >> --- /dev/null
> >> +++ b/include/dt-bindings/clock/qcom,gcc-sdm845.h
> >> @@ -0,0 +1,242 @@
> > [...]
> >> +#define GCC_VDDA_VS_CLK
> >> 180
> >> +#define GCC_VDDCX_VS_CLK 181
> >> +#define GCC_VDDMX_VS_CLK 182
> >> +#define GCC_VS_CTRL_AHB_CLK183
> >> +#define GCC_VS_CTRL_CLK
> >> 184
> >> +#define GCC_VS_CTRL_CLK_SRC185
> >> +#define GCC_VSENSOR_CLK_SRC186
> >> +#define GPLL4 187
> >
> > Do you have the define for the quad spi clks? And the implementation
> > for
> > it?
> >
>
> In SDM845, Quad SPI clocks are part of gcc_qupv*_wrap*_s* clock group.
Ok thanks. I must have messed up my math before because it didn't match
what bootloader was doing last time I checked.
Re: [PATCH v3 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845
Quoting Manu Gautam (2018-04-18 09:38:41) > Hi Amit, > > > On 4/18/2018 6:33 PM, Amit Nischal wrote: > >>> + /* Disable the GPLL0 active input to MMSS and GPU via MISC > >>> registers */ > >>> + regmap_update_bits(regmap, 0x09ffc, 0x3, 0x3); > >>> + regmap_update_bits(regmap, 0x71028, 0x3, 0x3); > >> > >> I think we'll have to throw in the pipe clk branch stuff in here too? > >> And then drop the pipe clks from the driver? > > > > All the USB pipe clocks would be taken care. The PCIE pipe branch > > clocks would have to be explicitly disabled so as to retain the > > memory logic. Otherwise, it would lead to memory corruption in case > > the external source is directly disabled without disabling the branch > > clock. > > PHY driver is same for both USB and PCIE and both PHYs use pipe_clk. > If there is indeed some limitation and pipe_clk cant be left enabled > always then I will suggest to not change pipe_clk handling for USB as well. > Right. This is concerning if we have a half way solution. Just to clarify my understanding, are you saying that the pcie pipe clks are also tied to the memory logic and so toggling them on/off is used to reset the memories inside the phy? Or the memories inside the controller? What is the pipe clk clocking in these cases?
Re: [PATCH v3 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845
Hi Amit, On 4/18/2018 6:33 PM, Amit Nischal wrote: >>> + /* Disable the GPLL0 active input to MMSS and GPU via MISC >>> registers */ >>> + regmap_update_bits(regmap, 0x09ffc, 0x3, 0x3); >>> + regmap_update_bits(regmap, 0x71028, 0x3, 0x3); >> >> I think we'll have to throw in the pipe clk branch stuff in here too? >> And then drop the pipe clks from the driver? > > All the USB pipe clocks would be taken care. The PCIE pipe branch > clocks would have to be explicitly disabled so as to retain the > memory logic. Otherwise, it would lead to memory corruption in case > the external source is directly disabled without disabling the branch clock. PHY driver is same for both USB and PCIE and both PHYs use pipe_clk. If there is indeed some limitation and pipe_clk cant be left enabled always then I will suggest to not change pipe_clk handling for USB as well. -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v3 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845
On 2018-04-17 09:21, Stephen Boyd wrote:
Quoting Amit Nischal (2018-04-03 06:22:41)
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index fbf4532..c961e89 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -218,6 +218,15 @@ config MSM_MMCC_8996
Say Y if you want to support multimedia devices such as
display,
graphics, video encode/decode, camera, etc.
+config SDM_GCC_845
+ tristate "SDM845 Global Clock Controller"
+ depends on COMMON_CLK_QCOM
+ help
+ Support for the global clock controller on Qualcomm
Technologies, Inc
+ sdm845 devices.
+ Say Y if you want to use peripheral devices such as UART,
SPI,
+ i2c, USB, UFS, SD/eMMC, PCIe, etc.
Is there eMMC?
Thanks for the review comments. There is no eMMC for SDM845. I will fix
the
above in next patch series.
+
config SPMI_PMIC_CLKDIV
tristate "SPMI PMIC clkdiv Support"
depends on (COMMON_CLK_QCOM && SPMI) || COMPILE_TEST
diff --git a/drivers/clk/qcom/gcc-sdm845.c
b/drivers/clk/qcom/gcc-sdm845.c
new file mode 100644
index 000..b1b7a1e
--- /dev/null
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -0,0 +1,3546 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
Is this include used?
No, it is not getting used. We will remove this in next patch series.
+#include
+#include
+#include
+
[...]
+
+static struct clk_rcg2 gcc_usb30_prim_mock_utmi_clk_src = {
+ .cmd_rcgr = 0xf030,
+ .mnd_width = 0,
+ .hid_width = 5,
+ .parent_map = gcc_parent_map_0,
+ .freq_tbl = ftbl_gcc_usb30_prim_mock_utmi_clk_src,
+ .clkr.hw.init = &(struct clk_init_data){
+ .name = "gcc_usb30_prim_mock_utmi_clk_src",
+ .parent_names = gcc_parent_names_0,
+ .num_parents = 4,
+ .ops = &clk_rcg2_shared_ops,
Still shared? Why?
We would require the shared_ops for clocks which are configured by
bootloader.
+
+static struct clk_branch gcc_video_ahb_clk = {
+ .halt_reg = 0xb004,
+ .halt_check = BRANCH_HALT,
+ .hwcg_reg = 0xb004,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0xb004,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_video_ahb_clk",
+ .flags = CLK_IS_CRITICAL,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+
Weird double space here.
We will fix this in next patch series.
+static struct clk_branch gcc_video_xo_clk = {
+ .halt_reg = 0xb028,
+ .halt_check = BRANCH_HALT,
+ .clkr = {
+ .enable_reg = 0xb028,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_video_xo_clk",
+ .flags = CLK_IS_CRITICAL,
For these "critical" clks that don't have parents can you just throw
the
enable part in the gcc driver probe and remove these clks from being
exposed? They don't seem to provide any value to expose them as clks
when they don't hook into the final clk tree.
For all of the "critical" clocks which don't have parents, we have
removed the CRITICAL flag and mandate the clients to put their vote
to enable/disable them. Other than this, some of the "critical" clock
instances we have completely removed and enabled them in the probe.
This will be fixed in the next patch series.
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch gcc_vs_ctrl_ahb_clk = {
+ .halt_reg = 0x7a014,
+ .halt_check = BRANCH_HALT,
+ .hwcg_reg = 0x7a014,
+ .hwcg_bit = 1,
+ .clkr = {
+ .enable_reg = 0x7a014,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_vs_ctrl_ahb_clk",
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+static struct clk_branch gcc_vs_ctrl_clk = {
+ .halt_reg = 0x7a010,
+ .halt_check = BRANCH_HALT,
+ .clkr = {
+ .enable_reg = 0x7a010,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "gcc_vs_ctrl_clk",
+ .parent_names = (const char *[]){
+ "gcc_vs_ctrl_clk_src",
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ .ops = &clk_branch2_ops,
+ },
+ },
+};
+
+
+static int gcc_sdm845_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct regmap *regmap;
+ int i, ret;
+
+ reg
Re: [PATCH v3 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845
Quoting Amit Nischal (2018-04-03 06:22:41)
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index fbf4532..c961e89 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -218,6 +218,15 @@ config MSM_MMCC_8996
> Say Y if you want to support multimedia devices such as display,
> graphics, video encode/decode, camera, etc.
>
> +config SDM_GCC_845
> + tristate "SDM845 Global Clock Controller"
> + depends on COMMON_CLK_QCOM
> + help
> + Support for the global clock controller on Qualcomm Technologies,
> Inc
> + sdm845 devices.
> + Say Y if you want to use peripheral devices such as UART, SPI,
> + i2c, USB, UFS, SD/eMMC, PCIe, etc.
Is there eMMC?
> +
> config SPMI_PMIC_CLKDIV
> tristate "SPMI PMIC clkdiv Support"
> depends on (COMMON_CLK_QCOM && SPMI) || COMPILE_TEST
> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
> new file mode 100644
> index 000..b1b7a1e
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sdm845.c
> @@ -0,0 +1,3546 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Is this include used?
> +#include
> +#include
> +#include
> +
[...]
> +
> +static struct clk_rcg2 gcc_usb30_prim_mock_utmi_clk_src = {
> + .cmd_rcgr = 0xf030,
> + .mnd_width = 0,
> + .hid_width = 5,
> + .parent_map = gcc_parent_map_0,
> + .freq_tbl = ftbl_gcc_usb30_prim_mock_utmi_clk_src,
> + .clkr.hw.init = &(struct clk_init_data){
> + .name = "gcc_usb30_prim_mock_utmi_clk_src",
> + .parent_names = gcc_parent_names_0,
> + .num_parents = 4,
> + .ops = &clk_rcg2_shared_ops,
Still shared? Why?
> +
> +static struct clk_branch gcc_video_ahb_clk = {
> + .halt_reg = 0xb004,
> + .halt_check = BRANCH_HALT,
> + .hwcg_reg = 0xb004,
> + .hwcg_bit = 1,
> + .clkr = {
> + .enable_reg = 0xb004,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_video_ahb_clk",
> + .flags = CLK_IS_CRITICAL,
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +
Weird double space here.
> +static struct clk_branch gcc_video_xo_clk = {
> + .halt_reg = 0xb028,
> + .halt_check = BRANCH_HALT,
> + .clkr = {
> + .enable_reg = 0xb028,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_video_xo_clk",
> + .flags = CLK_IS_CRITICAL,
For these "critical" clks that don't have parents can you just throw the
enable part in the gcc driver probe and remove these clks from being
exposed? They don't seem to provide any value to expose them as clks
when they don't hook into the final clk tree.
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch gcc_vs_ctrl_ahb_clk = {
> + .halt_reg = 0x7a014,
> + .halt_check = BRANCH_HALT,
> + .hwcg_reg = 0x7a014,
> + .hwcg_bit = 1,
> + .clkr = {
> + .enable_reg = 0x7a014,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_vs_ctrl_ahb_clk",
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch gcc_vs_ctrl_clk = {
> + .halt_reg = 0x7a010,
> + .halt_check = BRANCH_HALT,
> + .clkr = {
> + .enable_reg = 0x7a010,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_vs_ctrl_clk",
> + .parent_names = (const char *[]){
> + "gcc_vs_ctrl_clk_src",
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = &clk_branch2_ops,
> + },
> + },
> +};
> +
> +
> +static int gcc_sdm845_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct regmap *regmap;
> + int i, ret;
> +
> + regmap = qcom_cc_map(pdev, &gcc_sdm845_desc);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + for (i = 0; i < ARRAY_SIZE(gcc_sdm845_hws); i++) {
> + ret = devm_clk_hw_register(dev, gcc_sdm845_hws[i]);
> + if (ret)
> + return ret;
> + }
> +
> + /* Disable the GPLL0 active input to MMSS and GPU via MISC registers
> */
> + regmap_updat
[PATCH v3 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845
From: Taniya Das
Add support for the global clock controller found on SDM845
based devices. This should allow most non-multimedia device
drivers to probe and control their clocks.
Signed-off-by: Taniya Das
Signed-off-by: Amit Nischal
---
The patch is dependent upon the below patches related to
GDSC operation and are under review.
https://lkml.org/lkml/2018/4/2/142
.../devicetree/bindings/clock/qcom,gcc.txt |1 +
drivers/clk/qcom/Kconfig |9 +
drivers/clk/qcom/Makefile |1 +
drivers/clk/qcom/gcc-sdm845.c | 3546
include/dt-bindings/clock/qcom,gcc-sdm845.h| 242 ++
5 files changed, 3799 insertions(+)
create mode 100644 drivers/clk/qcom/gcc-sdm845.c
create mode 100644 include/dt-bindings/clock/qcom,gcc-sdm845.h
diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
index 551d03b..bf2355d 100644
--- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
+++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
@@ -18,6 +18,7 @@ Required properties :
"qcom,gcc-msm8994"
"qcom,gcc-msm8996"
"qcom,gcc-mdm9615"
+ "qcom,gcc-sdm845"
- reg : shall contain base register location and length
- #clock-cells : shall contain 1
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index fbf4532..c961e89 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -218,6 +218,15 @@ config MSM_MMCC_8996
Say Y if you want to support multimedia devices such as display,
graphics, video encode/decode, camera, etc.
+config SDM_GCC_845
+ tristate "SDM845 Global Clock Controller"
+ depends on COMMON_CLK_QCOM
+ help
+ Support for the global clock controller on Qualcomm Technologies, Inc
+ sdm845 devices.
+ Say Y if you want to use peripheral devices such as UART, SPI,
+ i2c, USB, UFS, SD/eMMC, PCIe, etc.
+
config SPMI_PMIC_CLKDIV
tristate "SPMI PMIC clkdiv Support"
depends on (COMMON_CLK_QCOM && SPMI) || COMPILE_TEST
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 230332c..1fc934dd 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -37,4 +37,5 @@ obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
+obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
new file mode 100644
index 000..b1b7a1e
--- /dev/null
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -0,0 +1,3546 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#include "common.h"
+#include "clk-regmap.h"
+#include "clk-pll.h"
+#include "clk-rcg.h"
+#include "clk-branch.h"
+#include "clk-alpha-pll.h"
+#include "gdsc.h"
+#include "reset.h"
+
+#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }
+
+enum {
+ P_BI_TCXO,
+ P_AUD_REF_CLK,
+ P_CORE_BI_PLL_TEST_SE,
+ P_GPLL0_OUT_EVEN,
+ P_GPLL0_OUT_MAIN,
+ P_GPLL4_OUT_MAIN,
+ P_SLEEP_CLK,
+};
+
+static const struct parent_map gcc_parent_map_0[] = {
+ { P_BI_TCXO, 0 },
+ { P_GPLL0_OUT_MAIN, 1 },
+ { P_GPLL0_OUT_EVEN, 6 },
+ { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_0[] = {
+ "bi_tcxo",
+ "gpll0",
+ "gpll0_out_even",
+ "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_1[] = {
+ { P_BI_TCXO, 0 },
+ { P_GPLL0_OUT_MAIN, 1 },
+ { P_SLEEP_CLK, 5 },
+ { P_GPLL0_OUT_EVEN, 6 },
+ { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_1[] = {
+ "bi_tcxo",
+ "gpll0",
+ "core_pi_sleep_clk",
+ "gpll0_out_even",
+ "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_2[] = {
+ { P_BI_TCXO, 0 },
+ { P_SLEEP_CLK, 5 },
+ { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_2[] = {
+ "bi_tcxo",
+ "core_pi_sleep_clk",
+ "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_3[] = {
+ { P_BI_TCXO, 0 },
+ { P_GPLL0_OUT_MAIN, 1 },
+ { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_3[] = {
+ "bi_tcxo",
+ "gpll0",
+ "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_4[] = {
+ { P_BI_TCXO, 0 },
+ { P_CORE_BI_PLL_TEST_SE, 7 },
