Re: [PATCH 5/8] clk: qcom: add Global Clock controller (GCC) driver for IPQ5424 SoC

Sricharan Ramabadhran
2024-09-16T07:51:00Z

On 9/13/2024 6:16 PM, Dmitry Baryshkov wrote:
> On Fri, Sep 13, 2024 at 05:42:47PM GMT, Sricharan R wrote:
>> From: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>>
>> Add support for the global clock controller found on IPQ5424 SoC.
>>
>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>
> Same comment regarding tags.
>
ok
>> ---
>> drivers/clk/qcom/Kconfig | 7 +
>> drivers/clk/qcom/Makefile | 1 +
>> drivers/clk/qcom/gcc-ipq5424.c | 3333 ++++++++++++++++++++++++++++++++
>> 3 files changed, 3341 insertions(+)
>> create mode 100644 drivers/clk/qcom/gcc-ipq5424.c
>>
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index a3e2a09e2105..c41e3318c2a7 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -213,6 +213,13 @@ config IPQ_GCC_5332
>> Say Y if you want to use peripheral devices such as UART, SPI,
>> i2c, USB, SD/eMMC, etc.
>>
>> +config IPQ_GCC_5424
>> + tristate "IPQ5424 Global Clock Controller"
>> + help
>> + Support for the global clock controller on ipq5424 devices.
>> + Say Y if you want to use peripheral devices such as UART, SPI,
>> + i2c, USB, SD/eMMC, etc.
>> +
>> config IPQ_GCC_6018
>> tristate "IPQ6018 Global Clock Controller"
>> help
>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index 2b378667a63f..d58ba0f9a482 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_IPQ_APSS_6018) += apss-ipq6018.o
>> obj-$(CONFIG_IPQ_GCC_4019) += gcc-ipq4019.o
>> obj-$(CONFIG_IPQ_GCC_5018) += gcc-ipq5018.o
>> obj-$(CONFIG_IPQ_GCC_5332) += gcc-ipq5332.o
>> +obj-$(CONFIG_IPQ_GCC_5424) += gcc-ipq5424.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
>> diff --git a/drivers/clk/qcom/gcc-ipq5424.c b/drivers/clk/qcom/gcc-ipq5424.c
>> new file mode 100644
>> index 000000000000..72d2c9bfa986
>> --- /dev/null
>> +++ b/drivers/clk/qcom/gcc-ipq5424.c
>> @@ -0,0 +1,3333 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018,2020 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/clock/qcom,ipq5424-gcc.h>
>> +#include <dt-bindings/reset/qcom,ipq5424-gcc.h>
>> +
>> +#include "clk-alpha-pll.h"
>> +#include "clk-branch.h"
>> +#include "clk-rcg.h"
>> +#include "clk-regmap.h"
>> +#include "clk-regmap-divider.h"
>> +#include "clk-regmap-mux.h"
>> +#include "clk-regmap-phy-mux.h"
>> +#include "common.h"
>> +#include "reset.h"
>> +
>> +enum {
>> + DT_XO,
>> + DT_SLEEP_CLK,
>> + DT_PCIE30_PHY0_PIPE_CLK,
>> + DT_PCIE30_PHY1_PIPE_CLK,
>> + DT_PCIE30_PHY2_PIPE_CLK,
>> + DT_PCIE30_PHY3_PIPE_CLK,
>> + DT_USB_PCIE_WRAPPER_PIPE_CLK,
>
> This doesn't seem to match bindings.
>
ok, will fix
>> +};
>> +
>> +enum {
>> + P_GCC_GPLL0_OUT_MAIN_DIV_CLK_SRC,
>> + P_GPLL0_OUT_AUX,
>> + P_GPLL0_OUT_MAIN,
>> + P_GPLL2_OUT_AUX,
>> + P_GPLL2_OUT_MAIN,
>> + P_GPLL4_OUT_AUX,
>> + P_GPLL4_OUT_MAIN,
>> + P_SLEEP_CLK,
>> + P_XO,
>> + P_USB3PHY_0_PIPE,
>> +};
>> +
>> +static const struct clk_parent_data gcc_parent_data_xo = { .index = DT_XO };
>> +
>> +static struct clk_alpha_pll gpll0 = {
>> + .offset = 0x20000,
>> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT_EVO],
>> + .clkr = {
>> + .enable_reg = 0xb000,
>> + .enable_mask = BIT(0),
>> + .hw.init = &(const struct clk_init_data) {
>> + .name = "gpll0",
>> + .parent_data = &gcc_parent_data_xo,
>> + .num_parents = 1,
>> + .ops = &clk_alpha_pll_ops,
>> + .flags = CLK_IS_CRITICAL,
>
> This deserves a comment
>
ok will add
>> + },
>> + },
>> +};
>> +
>> +static struct clk_fixed_factor gpll0_div2 = {
>> + .mult = 1,
>> + .div = 2,
>> + .hw.init = &(const struct clk_init_data) {
>> + .name = "gpll0_div2",
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &gpll0.clkr.hw
>> + },
>> + .num_parents = 1,
>> + .ops = &clk_fixed_factor_ops,
>> + },
>> +};
>> +
>> +static struct clk_alpha_pll gpll2 = {
>> + .offset = 0x21000,
>> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_NSS_HUAYRA],
>> + .clkr = {
>> + .enable_reg = 0xb000,
>> + .enable_mask = BIT(1),
>> + .hw.init = &(const struct clk_init_data) {
>> + .name = "gpll2",
>> + .parent_data = &gcc_parent_data_xo,
>> + .num_parents = 1,
>> + .ops = &clk_alpha_pll_ops,
>> + },
>> + },
>> +};
>> +
>> +static const struct clk_div_table post_div_table_gpll2_out_main[] = {
>> + { 0x1, 2 },
>> + { }
>> +};
>> +
>> +static struct clk_alpha_pll_postdiv gpll2_out_main = {
>> + .offset = 0x21000,
>> + .post_div_table = post_div_table_gpll2_out_main,
>> + .num_post_div = ARRAY_SIZE(post_div_table_gpll2_out_main),
>> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_NSS_HUAYRA],
>> + .clkr.hw.init = &(const struct clk_init_data) {
>> + .name = "gpll2_out_main",
>> + .parent_hws = (const struct clk_hw*[]) {
>> + &gpll2.clkr.hw,
>> + },
>> + .num_parents = 1,
>> + .ops = &clk_alpha_pll_postdiv_ro_ops,
>> + },
>> +};
>> +
>> +static struct clk_alpha_pll gpll4 = {
>> + .offset = 0x22000,
>> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT_EVO],
>> + .clkr = {
>> + .enable_reg = 0xb000,
>> + .enable_mask = BIT(2),
>> + .hw.init = &(const struct clk_init_data) {
>> + .name = "gpll4",
>> + .parent_data = &gcc_parent_data_xo,
>> + .num_parents = 1,
>> + .flags = CLK_IS_CRITICAL,
>
> Comment, please.
>
ok, will add
>> + .ops = &clk_alpha_pll_ops,
>> + },
>> + },
>> +};
>> +
>
> [skipped]
>
>> +
>> +static struct clk_rcg2 gcc_pcnoc_bfdcd_clk_src = {
>> + .cmd_rcgr = 0x31004,
>> + .mnd_width = 0,
>> + .hid_width = 5,
>> + .parent_map = gcc_parent_map_0,
>> + .freq_tbl = ftbl_gcc_pcnoc_bfdcd_clk_src,
>> + .clkr.hw.init = &(const struct clk_init_data) {
>> + .name = "gcc_pcnoc_bfdcd_clk_src",
>> + .parent_data = gcc_parent_data_0,
>> + .num_parents = ARRAY_SIZE(gcc_parent_data_0),
>> + .flags = CLK_IS_CRITICAL,
>
> Comment
>
ok
>> + .ops = &clk_rcg2_ops,
>> + },
>> +};
>> +
>
> [skipped]
>
>> +
>> +static struct clk_branch gcc_qdss_dap_clk = {
>> + .halt_reg = 0x2d058,
>> + .clkr = {
>> + .enable_reg = 0x2d058,
>> + .enable_mask = BIT(0),
>> + .hw.init = &(const struct clk_init_data) {
>> + .name = "gcc_qdss_dap_clk",
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &gcc_qdss_dap_sync_clk_src.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>
> Comment
>
ok
>> + .ops = &clk_branch2_ops,
>> + },
>> + },
>> +};
>> +
>> +static struct clk_branch gcc_qdss_at_clk = {
>> + .halt_reg = 0x2d034,
>> + .clkr = {
>> + .enable_reg = 0x2d034,
>> + .enable_mask = BIT(0),
>> + .hw.init = &(const struct clk_init_data) {
>> + .name = "gcc_qdss_at_clk",
>> + .parent_hws = (const struct clk_hw *[]) {
>> + &gcc_qdss_at_clk_src.clkr.hw
>> + },
>> + .num_parents = 1,
>> + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>
> Comment
>
ok
>> + .ops = &clk_branch2_ops,
>> + },
>> + },
>> +};
>> +
>
> [skipped]
>
>> +
>> +static int gcc_ipq5424_probe(struct platform_device *pdev)
>> +{
>> + struct regmap *regmap;
>> + struct qcom_cc_desc ipq5424_desc = gcc_ipq5424_desc;
>> + int ret;
>> +
>> + regmap = qcom_cc_map(pdev, &ipq5424_desc);
>> + if (IS_ERR(regmap))
>> + return PTR_ERR(regmap);
>> +
>> + ret = qcom_cc_really_probe(&pdev->dev, &ipq5424_desc, regmap);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to register GCC clocks ret=%d\n", ret);
>> + return ret;
>> + }
>> +
>> + dev_info(&pdev->dev, "Registered GCC clocks\n");
>> +
>> + return ret;
>
> Drop all the cruft and use qcom_cc_probe() directly.
>
ok

Regards,
Sricharan