Re: [PATCH 3/7] clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018
Quoting Varadarajan Narayanan (2020-09-27 22:15:36)
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 0583273..d1a2504 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -155,6 +155,14 @@ config IPQ_GCC_8074
> i2c, USB, SD/eMMC, etc. Select this for the root clock
> of ipq8074.
>
> +config IPQ_GCC_5018
> + tristate "IPQ5018 Global Clock Controller"
> + help
> +Support for global clock controller on ipq5018 devices.
> +Say Y if you want to use peripheral devices such as UART, SPI,
> +i2c, USB, SD/eMMC, etc. Select this for the root clock
> +of ipq5018.
What is the root clock of ipq5018? Please drop that last sentence.
> +
> config MSM_GCC_8660
> tristate "MSM8660 Global Clock Controller"
> help
> diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
> new file mode 100644
> index ..9056386
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-ipq5018.c
> @@ -0,0 +1,3833 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
Why is this attached to dt-bindings? Please remove that newline above
and move this away from dt-bindings below.
> +#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 "clk-regmap-divider.h"
> +#include "clk-regmap-mux.h"
> +#include "reset.h"
> +
> +#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }
This is in clk-rcg.h already.
> +
> +static const char * const gcc_usb3phy_0_cc_pipe_clk_xo[] = {
> + "usb3phy_0_cc_pipe_clk",
> + "xo",
> +};
All these names structures need to change, see next comment.
> +
> +static struct clk_rcg2 apss_ahb_clk_src = {
> + .cmd_rcgr = 0x46000,
> + .mnd_width = 0,
> + .hid_width = 5,
> + .freq_tbl = ftbl_apss_ahb_clk_src,
> + .parent_map = gcc_xo_gpll0_gpll0_out_main_div2_map,
> + .clkr.hw.init = &(struct clk_init_data){
> + .name = "apss_ahb_clk_src",
> + .parent_names = gcc_xo_gpll0_gpll0_out_main_div2,
> + .num_parents = 3,
Please migrate to the new way of specifying clks with
clk_init_data::clk_parent_data
> + .ops = _rcg2_ops,
> + .flags = CLK_IS_CRITICAL | CLK_IGNORE_UNUSED,
Why is it critical and ignore unused? Do you need this clk to be here at
all? Can we just enable it when this driver probes with a register write
and then ignore it from there on out?
> + },
> +};
> +
> +static struct clk_regmap_div apss_ahb_postdiv_clk_src = {
> + .reg = 0x46018,
> + .shift = 4,
> + .width = 4,
> + .clkr = {
> + .hw.init = &(struct clk_init_data){
> + .name = "apss_ahb_postdiv_clk_src",
> + .parent_names = (const char *[]){
> + "apss_ahb_clk_src"
> + },
> + .num_parents = 1,
> + .ops = _regmap_div_ops,
> + },
> + },
> +};
> +
[...]
> +
> +static struct clk_branch gcc_qdss_dap_clk = {
> + .halt_reg = 0x29084,
> + .clkr = {
> + .enable_reg = 0x29084,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_qdss_dap_clk",
> + .parent_names = (const char *[]){
> + "qdss_tsctr_clk_src"
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
Whenever CLK_IS_CRITICAL is there please document why it is needed. And
if possible remove the clk structure and hit the clk on in driver probe
so we don't waste memory modeling something that never matters.
Typically that can only be done if nothing references this clk as a
parent or if we're willing to break the clk tree and ignore describing
parents. In this case it's a branch so probably nothing else is under it
so we can just turn it on during probe and stop caring.
> + .ops = _branch2_ops,
> + },
> + },
> +};
> +
> +static struct clk_branch gcc_qdss_cfg_ahb_clk = {
> + .halt_reg = 0x29008,
> + .clkr = {
> + .enable_reg = 0x29008,
> + .enable_mask = BIT(0),
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_qdss_cfg_ahb_clk",
> + .parent_names = (const char *[]){
> + "pcnoc_clk_src"
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + .ops = _branch2_ops,
> +
[PATCH 3/7] clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018
Add support for the global clock controller found on IPQ5018
based devices.
Signed-off-by: Varadarajan Narayanan
---
drivers/clk/qcom/Kconfig |8 +
drivers/clk/qcom/Makefile |1 +
drivers/clk/qcom/gcc-ipq5018.c | 3833
include/linux/clk-provider.h |4 +-
4 files changed, 3844 insertions(+), 2 deletions(-)
create mode 100644 drivers/clk/qcom/gcc-ipq5018.c
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 0583273..d1a2504 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -155,6 +155,14 @@ config IPQ_GCC_8074
i2c, USB, SD/eMMC, etc. Select this for the root clock
of ipq8074.
+config IPQ_GCC_5018
+ tristate "IPQ5018 Global Clock Controller"
+ help
+Support for global clock controller on ipq5018 devices.
+Say Y if you want to use peripheral devices such as UART, SPI,
+i2c, USB, SD/eMMC, etc. Select this for the root clock
+of ipq5018.
+
config MSM_GCC_8660
tristate "MSM8660 Global Clock Controller"
help
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 9677e76..1283f70 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_IPQ_GCC_4019) += gcc-ipq4019.o
obj-$(CONFIG_IPQ_GCC_6018) += gcc-ipq6018.o
obj-$(CONFIG_IPQ_GCC_806X) += gcc-ipq806x.o
obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o
+obj-$(CONFIG_IPQ_GCC_5018) += gcc-ipq5018.o
obj-$(CONFIG_IPQ_LCC_806X) += lcc-ipq806x.o
obj-$(CONFIG_MDM_GCC_9615) += gcc-mdm9615.o
obj-$(CONFIG_MDM_LCC_9615) += lcc-mdm9615.o
diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
new file mode 100644
index ..9056386
--- /dev/null
+++ b/drivers/clk/qcom/gcc-ipq5018.c
@@ -0,0 +1,3833 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#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 "clk-regmap-divider.h"
+#include "clk-regmap-mux.h"
+#include "reset.h"
+
+#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }
+
+enum {
+ P_XO,
+ P_GPLL0,
+ P_GPLL0_DIV2,
+ P_GPLL2,
+ P_GPLL4,
+ P_UBI32_PLL,
+ P_GEPHY_RX,
+ P_GEPHY_TX,
+ P_UNIPHY_RX,
+ P_UNIPHY_TX,
+ P_CORE_PI_SLEEP_CLK,
+ P_PCIE20_PHY0_PIPE,
+ P_PCIE20_PHY1_PIPE,
+ P_USB3PHY_0_PIPE,
+};
+
+static const char * const gcc_xo_gpll0_gpll0_out_main_div2[] = {
+ "xo",
+ "gpll0",
+ "gpll0_out_main_div2",
+};
+
+static const struct parent_map gcc_xo_gpll0_gpll0_out_main_div2_map[] = {
+ { P_XO, 0 },
+ { P_GPLL0, 1 },
+ { P_GPLL0_DIV2, 4 },
+};
+
+static const char * const gcc_xo_gpll0[] = {
+ "xo",
+ "gpll0",
+};
+
+static const struct parent_map gcc_xo_gpll0_map[] = {
+ { P_XO, 0 },
+ { P_GPLL0, 1 },
+};
+
+static const char * const gcc_xo_gpll0_out_main_div2_gpll0[] = {
+ "xo",
+ "gpll0_out_main_div2",
+ "gpll0",
+};
+
+static const struct parent_map gcc_xo_gpll0_out_main_div2_gpll0_map[] = {
+ { P_XO, 0 },
+ { P_GPLL0_DIV2, 2 },
+ { P_GPLL0, 1 },
+};
+
+static const char * const gcc_xo_ubi32_gpll0[] = {
+ "xo",
+ "ubi32_pll",
+ "gpll0",
+};
+
+static const struct parent_map gcc_xo_ubi32_gpll0_map[] = {
+ { P_XO, 0 },
+ { P_UBI32_PLL, 1 },
+ { P_GPLL0, 2 },
+};
+
+static const char * const gcc_xo_gpll0_gpll2[] = {
+ "xo",
+ "gpll0",
+ "gpll2",
+};
+
+static const struct parent_map gcc_xo_gpll0_gpll2_map[] = {
+ { P_XO, 0 },
+ { P_GPLL0, 1 },
+ { P_GPLL2, 2 },
+};
+
+static const char * const gcc_xo_gpll0_gpll2_gpll4[] = {
+ "xo",
+ "gpll0",
+ "gpll2",
+ "gpll4",
+};
+
+static const struct parent_map gcc_xo_gpll0_gpll2_gpll4_map[] = {
+ { P_XO, 0 },
+ { P_GPLL0, 1 },
+ { P_GPLL2, 2 },
+ { P_GPLL4, 3 },
+};
+
+static const char * const gcc_xo_gpll0_out_main_div2[] = {
+ "xo",
+ "gpll0_out_main_div2",
+};
+
+static const struct parent_map gcc_xo_gpll0_out_main_div2_map[] = {
+ { P_XO, 0 },
+ { P_GPLL0_DIV2, 1 },
+};
+
+static const char * const gcc_xo_gpll0_gpll4[] = {
+ "xo",
+ "gpll0",
+ "gpll4",
+};
+
+static const struct parent_map gcc_xo_gpll0_gpll4_map[] = {
+ { P_XO, 0 },
+ { P_GPLL0, 1 },
+ { P_GPLL4, 2 },
+};
+
+static const char * const gcc_xo_gpll0_core_pi_sleep_clk[] = {
+ "xo",
+ "gpll0",
+ "sleep_clk",
+};
+
+static const struct parent_map gcc_xo_gpll0_core_pi_sleep_clk_map[] = {
+ { P_XO, 0 },
+ { P_GPLL0, 2 },
+ { P_CORE_PI_SLEEP_CLK, 6 },
+};
+
+static const char * const gcc_xo_gpll0_gpll0_out_main_div2_sleep_clk[]
