mbox series

[v3,0/3] clk: qcom: Add support for SM8150 GCC

Message ID 20190625063140.17106-1-vkoul@kernel.org (mailing list archive)
Headers show
Series clk: qcom: Add support for SM8150 GCC | expand

Message

Vinod Koul June 25, 2019, 6:31 a.m. UTC 
This series adds suport for SM8150 GCC which requires Trion PLLs and adds
that as well.

Also fixed some minor nits seen in clk-alpha-pll code and module alias for
qcs404 gcc driver.

Changes in v3:
	- Drop the cast for DIV_ROUND_UP_ULL as that need s afix in macro,
	  so patch it up once core change in merged
	- Drop module alias patch and drop it from sm8150 driver
	- Add review tag by Bjorn

Deepak Katragadda (2):
  clk: qcom: clk-alpha-pll: Add support for Trion PLLs
  clk: qcom: gcc: Add global clock controller driver for SM8150

Vinod Koul (1):
  clk: qcom: clk-alpha-pll: Remove post_div_table checks

 .../devicetree/bindings/clock/qcom,gcc.txt    |    1 +
 drivers/clk/qcom/Kconfig                      |    7 +
 drivers/clk/qcom/Makefile                     |    1 +
 drivers/clk/qcom/clk-alpha-pll.c              |  243 +-
 drivers/clk/qcom/clk-alpha-pll.h              |    7 +
 drivers/clk/qcom/gcc-sm8150.c                 | 3719 +++++++++++++++++
 include/dt-bindings/clock/qcom,gcc-sm8150.h   |  243 ++
 7 files changed, 4206 insertions(+), 15 deletions(-)
 create mode 100644 drivers/clk/qcom/gcc-sm8150.c
 create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8150.h

Comments

Stephen Boyd June 27, 2019, 9:31 p.m. UTC | #1 
Quoting Vinod Koul (2019-06-24 23:31:40)
> From: Deepak Katragadda <dkatraga@codeaurora.org>
> 
> Add the clocks supported in global clock controller which clock the
> peripherals like BLSPs, SDCC, USB, MDSS etc. Register all the clocks
> to the clock framework for the clients to be able to request for them.
> 
> Signed-off-by: Deepak Katragadda <dkatraga@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> [vkoul: port to upstream and tidy-up]

This can say "changed to parent_data scheme"?

> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../devicetree/bindings/clock/qcom,gcc.txt    |    1 +
>  drivers/clk/qcom/Kconfig                      |    7 +
>  drivers/clk/qcom/Makefile                     |    1 +
>  drivers/clk/qcom/gcc-sm8150.c                 | 3719 +++++++++++++++++
>  include/dt-bindings/clock/qcom,gcc-sm8150.h   |  243 ++
>  5 files changed, 3971 insertions(+)
>  create mode 100644 drivers/clk/qcom/gcc-sm8150.c
>  create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8150.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> index 8661c3cd3ccf..1b39d949e63d 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> @@ -23,6 +23,7 @@ Required properties :
>                         "qcom,gcc-sdm630"
>                         "qcom,gcc-sdm660"
>                         "qcom,gcc-sdm845"
> +                       "qcom,gcc-sm8150"
>  
>  - reg : shall contain base register location and length
>  - #clock-cells : shall contain 1

I'd expect the binding to get a larger update indicating which clks come
from outside the clk controller and need to be specified as 'clocks' and
'clock-names'.

> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 18bdf34d5e64..076872d195fd 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -291,6 +291,13 @@ config SDM_LPASSCC_845
>           Say Y if you want to use the LPASS branch clocks of the LPASS clock
>           controller to reset the LPASS subsystem.
>  
> +config SM_GCC_8150
> +       tristate "SM8150 Global Clock Controller"
> +       help
> +         Support for the global clock controller on SM8150 devices.
> +         Say Y if you want to use peripheral devices such as UART,
> +         SPI, I2C, USB, SD/eMMC, PCIe etc.

Is there eMMC support?

> +
>  config SPMI_PMIC_CLKDIV
>         tristate "SPMI PMIC clkdiv Support"
>         depends on SPMI || COMPILE_TEST
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index f0768fb1f037..4a813b4055d0 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
>  obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
>  obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
>  obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
> +obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o
>  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
>  obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
>  obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
> diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> new file mode 100644
> index 000000000000..11cd9e19f18d
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sm8150.c
> @@ -0,0 +1,3719 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2017-19, The Linux Foundation. All rights reserved.

Is it really 2017-19 and not 2017-2019?

> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/clk.h>

Is this include used?

> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +#include <dt-bindings/clock/qcom,gcc-sm8150.h>
> +
> +#include "common.h"
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-pll.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "reset.h"
> +
> +#define GCC_NPU_MISC                           0x4d110
> +#define GCC_GPU_MISC                           0x71028

Drop these defines please and inline the values.

> +
> +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_GPLL7_OUT_MAIN,
> +       P_GPLL9_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 struct clk_parent_data gcc_parents_0[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct clk_parent_data gcc_parents_0_ao[] = {
> +       { .fw_name = "bi_tcxo_ao", .name = "bi_tcxo_ao" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "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 struct clk_parent_data gcc_parents_1[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "sleep_clk", .name = "sleep_clk" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "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 struct clk_parent_data gcc_parents_2[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "sleep_clk", .name = "sleep_clk" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "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 struct clk_parent_data gcc_parents_3[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "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 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_4[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct parent_map gcc_parent_map_5[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_GPLL0_OUT_MAIN, 1 },
> +       { P_GPLL7_OUT_MAIN, 3 },
> +       { P_GPLL0_OUT_EVEN, 6 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_5[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "gpll7", .name = "gpll7" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },

Aren't these gplls all created in this file? They shouldn't be listed in
DT so I'm confused why we have .fw_name for them.

> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },

If there isn't a legacy string name for core_bi_pll_test_se we should
just leave it out and not have a .name field, just the .fw_name one.

> +};
> +
> +static const struct parent_map gcc_parent_map_6[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_GPLL0_OUT_MAIN, 1 },
> +       { P_GPLL9_OUT_MAIN, 2 },
> +       { P_GPLL4_OUT_MAIN, 5 },
> +       { P_GPLL0_OUT_EVEN, 6 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_6[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "gpll9", .name = "gpll9" },
> +       { .fw_name = "gpll4", .name = "gpll4" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct parent_map gcc_parent_map_7[] = {
> +       { P_BI_TCXO, 0 },
> +       { P_GPLL0_OUT_MAIN, 1 },
> +       { P_AUD_REF_CLK, 2 },
> +       { P_GPLL0_OUT_EVEN, 6 },
> +       { P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const struct clk_parent_data gcc_parents_7[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .fw_name = "gpll0", .name = "gpll0" },
> +       { .fw_name = "aud_ref_clk", .name = "aud_ref_clk" },
> +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static struct pll_vco trion_vco[] = {

const?

> +       { 249600000, 2000000000, 0 },
> +};
> +
> +static struct clk_alpha_pll gpll0 = {
> +       .offset = 0x0,
> +       .vco_table = trion_vco,
> +       .num_vco = ARRAY_SIZE(trion_vco),
> +       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_TRION],
> +       .clkr = {
> +               .enable_reg = 0x52000,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gpll0",
> +                       .parent_data = &(const struct clk_parent_data){
> +                               .fw_name = "bi_tcxo",
> +                               .name = "bi_tcxo",
> +                       },
> +                       .num_parents = 1,
> +                       .ops = &clk_trion_fixed_pll_ops,
> +               },
> +       },
> +};
> +
> +static const struct clk_div_table post_div_table_trion_even[] = {
> +       { 0x0, 1 },
> +       { 0x1, 2 },
> +       { 0x3, 4 },
> +       { 0x7, 8 },
> +       { }
> +};
> +
> +static struct clk_alpha_pll_postdiv gpll0_out_even = {
> +       .offset = 0x0,
> +       .post_div_shift = 8,
> +       .post_div_table = post_div_table_trion_even,
> +       .num_post_div = ARRAY_SIZE(post_div_table_trion_even),
> +       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_TRION],
> +       .width = 4,
> +       .clkr.hw.init = &(struct clk_init_data){
> +               .name = "gpll0_out_even",
> +               .parent_data = &(const struct clk_parent_data){
> +                       .fw_name = "gpll0",
> +                       .name = "gpll0",

Use a direct pointer? This applies in many places in the code.

> +               },
> +               .num_parents = 1,
> +               .ops = &clk_trion_pll_postdiv_ops,
> +       },
> +};
> +
[...]
> +
> +static struct clk_branch gcc_video_ahb_clk = {
> +       .halt_reg = 0xb004,
> +       .halt_check = BRANCH_HALT_DELAY,
> +       .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,

Can you please add a comment around all CLK_IS_CRITICAL flags indicating
why these clks can't be turned off?

> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_video_axi0_clk = {
> +       .halt_reg = 0xb024,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0xb024,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_video_axi0_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_video_axi1_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_axi1_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_video_axic_clk = {
> +       .halt_reg = 0xb02c,
> +       .halt_check = BRANCH_HALT,
> +       .clkr = {
> +               .enable_reg = 0xb02c,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_video_axic_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
> +static struct clk_branch gcc_video_xo_clk = {
> +       .halt_reg = 0xb040,
> +       .halt_check = BRANCH_HALT_DELAY,

Why doesn't the halt bit work?

> +       .clkr = {
> +               .enable_reg = 0xb040,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_video_xo_clk",
> +                       .flags = CLK_IS_CRITICAL,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
Vinod Koul June 29, 2019, 1:51 p.m. UTC | #2 
On 27-06-19, 14:31, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-24 23:31:40)
> > From: Deepak Katragadda <dkatraga@codeaurora.org>
> > 
> > Add the clocks supported in global clock controller which clock the
> > peripherals like BLSPs, SDCC, USB, MDSS etc. Register all the clocks
> > to the clock framework for the clients to be able to request for them.
> > 
> > Signed-off-by: Deepak Katragadda <dkatraga@codeaurora.org>
> > Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > [vkoul: port to upstream and tidy-up]
> 
> This can say "changed to parent_data scheme"?
> 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  .../devicetree/bindings/clock/qcom,gcc.txt    |    1 +
> >  drivers/clk/qcom/Kconfig                      |    7 +
> >  drivers/clk/qcom/Makefile                     |    1 +
> >  drivers/clk/qcom/gcc-sm8150.c                 | 3719 +++++++++++++++++
> >  include/dt-bindings/clock/qcom,gcc-sm8150.h   |  243 ++
> >  5 files changed, 3971 insertions(+)
> >  create mode 100644 drivers/clk/qcom/gcc-sm8150.c
> >  create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8150.h
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > index 8661c3cd3ccf..1b39d949e63d 100644
> > --- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> > @@ -23,6 +23,7 @@ Required properties :
> >                         "qcom,gcc-sdm630"
> >                         "qcom,gcc-sdm660"
> >                         "qcom,gcc-sdm845"
> > +                       "qcom,gcc-sm8150"
> >  
> >  - reg : shall contain base register location and length
> >  - #clock-cells : shall contain 1
> 
> I'd expect the binding to get a larger update indicating which clks come
> from outside the clk controller and need to be specified as 'clocks' and
> 'clock-names'.
> 
> > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > index 18bdf34d5e64..076872d195fd 100644
> > --- a/drivers/clk/qcom/Kconfig
> > +++ b/drivers/clk/qcom/Kconfig
> > @@ -291,6 +291,13 @@ config SDM_LPASSCC_845
> >           Say Y if you want to use the LPASS branch clocks of the LPASS clock
> >           controller to reset the LPASS subsystem.
> >  
> > +config SM_GCC_8150
> > +       tristate "SM8150 Global Clock Controller"
> > +       help
> > +         Support for the global clock controller on SM8150 devices.
> > +         Say Y if you want to use peripheral devices such as UART,
> > +         SPI, I2C, USB, SD/eMMC, PCIe etc.
> 
> Is there eMMC support?
> 
> > +
> >  config SPMI_PMIC_CLKDIV
> >         tristate "SPMI PMIC clkdiv Support"
> >         depends on SPMI || COMPILE_TEST
> > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > index f0768fb1f037..4a813b4055d0 100644
> > --- a/drivers/clk/qcom/Makefile
> > +++ b/drivers/clk/qcom/Makefile
> > @@ -50,6 +50,7 @@ obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
> >  obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
> >  obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
> >  obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
> > +obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o
> >  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> >  obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
> >  obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
> > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> > new file mode 100644
> > index 000000000000..11cd9e19f18d
> > --- /dev/null
> > +++ b/drivers/clk/qcom/gcc-sm8150.c
> > @@ -0,0 +1,3719 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2017-19, The Linux Foundation. All rights reserved.
> 
> Is it really 2017-19 and not 2017-2019?

will update

> 
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/bitops.h>
> > +#include <linux/err.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/clk.h>
> 
> Is this include used?

will check and update

> 
> > +#include <linux/clk-provider.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset-controller.h>
> > +
> > +#include <dt-bindings/clock/qcom,gcc-sm8150.h>
> > +
> > +#include "common.h"
> > +#include "clk-alpha-pll.h"
> > +#include "clk-branch.h"
> > +#include "clk-pll.h"
> > +#include "clk-rcg.h"
> > +#include "clk-regmap.h"
> > +#include "reset.h"
> > +
> > +#define GCC_NPU_MISC                           0x4d110
> > +#define GCC_GPU_MISC                           0x71028
> 
> Drop these defines please and inline the values.

Sure

> > +static const struct clk_parent_data gcc_parents_5[] = {
> > +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> > +       { .fw_name = "gpll0", .name = "gpll0" },
> > +       { .fw_name = "gpll7", .name = "gpll7" },
> > +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> 
> Aren't these gplls all created in this file? They shouldn't be listed in
> DT so I'm confused why we have .fw_name for them.

Yes they are and the DT doesnt provide these clock. From what I
understood from the name conversion to new schema was we should add it
like above, let me know if I missed something

> > +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> 
> If there isn't a legacy string name for core_bi_pll_test_se we should
> just leave it out and not have a .name field, just the .fw_name one.

Okay

> > +static const struct clk_parent_data gcc_parents_7[] = {
> > +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> > +       { .fw_name = "gpll0", .name = "gpll0" },
> > +       { .fw_name = "aud_ref_clk", .name = "aud_ref_clk" },
> > +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> > +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> > +};
> > +
> > +static struct pll_vco trion_vco[] = {
> 
> const?

will add

> > +static struct clk_alpha_pll_postdiv gpll0_out_even = {
> > +       .offset = 0x0,
> > +       .post_div_shift = 8,
> > +       .post_div_table = post_div_table_trion_even,
> > +       .num_post_div = ARRAY_SIZE(post_div_table_trion_even),
> > +       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_TRION],
> > +       .width = 4,
> > +       .clkr.hw.init = &(struct clk_init_data){
> > +               .name = "gpll0_out_even",
> > +               .parent_data = &(const struct clk_parent_data){
> > +                       .fw_name = "gpll0",
> > +                       .name = "gpll0",
> 
> Use a direct pointer? This applies in many places in the code.

okay will check here and other places

> > +static struct clk_branch gcc_video_ahb_clk = {
> > +       .halt_reg = 0xb004,
> > +       .halt_check = BRANCH_HALT_DELAY,
> > +       .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,
> 
> Can you please add a comment around all CLK_IS_CRITICAL flags indicating
> why these clks can't be turned off?

okay will do

> > +static struct clk_branch gcc_video_axic_clk = {
> > +       .halt_reg = 0xb02c,
> > +       .halt_check = BRANCH_HALT,
> > +       .clkr = {
> > +               .enable_reg = 0xb02c,
> > +               .enable_mask = BIT(0),
> > +               .hw.init = &(struct clk_init_data){
> > +                       .name = "gcc_video_axic_clk",
> > +                       .ops = &clk_branch2_ops,
> > +               },
> > +       },
> > +};
> > +
> > +static struct clk_branch gcc_video_xo_clk = {
> > +       .halt_reg = 0xb040,
> > +       .halt_check = BRANCH_HALT_DELAY,
> 
> Why doesn't the halt bit work?

Not sure, will check and update
Stephen Boyd July 15, 2019, 11:01 p.m. UTC | #3 
Quoting Vinod Koul (2019-06-29 06:51:19)
> On 27-06-19, 14:31, Stephen Boyd wrote:
> > Quoting Vinod Koul (2019-06-24 23:31:40)
> > > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > > index 18bdf34d5e64..076872d195fd 100644
> > > --- a/drivers/clk/qcom/Kconfig
> > > +++ b/drivers/clk/qcom/Kconfig
> > > @@ -291,6 +291,13 @@ config SDM_LPASSCC_845
> > >           Say Y if you want to use the LPASS branch clocks of the LPASS clock
> > >           controller to reset the LPASS subsystem.
> > >  
> > > +config SM_GCC_8150
> > > +       tristate "SM8150 Global Clock Controller"
> > > +       help
> > > +         Support for the global clock controller on SM8150 devices.
> > > +         Say Y if you want to use peripheral devices such as UART,
> > > +         SPI, I2C, USB, SD/eMMC, PCIe etc.
> > 
> > Is there eMMC support?
> > 

I guess no?

> > > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > > index f0768fb1f037..4a813b4055d0 100644
> > > --- a/drivers/clk/qcom/Makefile
> > > +++ b/drivers/clk/qcom/Makefile
> > > @@ -50,6 +50,7 @@ obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
> > >  obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
> > >  obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
> > >  obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
> > > +obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o
> > >  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> > >  obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
> > >  obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
> > > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> > > new file mode 100644
> > > index 000000000000..11cd9e19f18d
> > > --- /dev/null
> > > +++ b/drivers/clk/qcom/gcc-sm8150.c
> > > +static const struct clk_parent_data gcc_parents_5[] = {
> > > +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> > > +       { .fw_name = "gpll0", .name = "gpll0" },
> > > +       { .fw_name = "gpll7", .name = "gpll7" },
> > > +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> > 
> > Aren't these gplls all created in this file? They shouldn't be listed in
> > DT so I'm confused why we have .fw_name for them.
> 
> Yes they are and the DT doesnt provide these clock. From what I
> understood from the name conversion to new schema was we should add it
> like above, let me know if I missed something
> 

Yes, you should use the direct clk_hw pointer part of the
clk_parent_data structure instead of having a .fw_name or .name member
in these rows. Any clk inside of the clk controller shouldn't be exposed
into DT just so that we can find it again through string comparisons.
Vinod Koul July 16, 2019, 4:07 a.m. UTC | #4 
On 15-07-19, 16:01, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-29 06:51:19)
> > On 27-06-19, 14:31, Stephen Boyd wrote:
> > > Quoting Vinod Koul (2019-06-24 23:31:40)
> > > > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> > > > index 18bdf34d5e64..076872d195fd 100644
> > > > --- a/drivers/clk/qcom/Kconfig
> > > > +++ b/drivers/clk/qcom/Kconfig
> > > > @@ -291,6 +291,13 @@ config SDM_LPASSCC_845
> > > >           Say Y if you want to use the LPASS branch clocks of the LPASS clock
> > > >           controller to reset the LPASS subsystem.
> > > >  
> > > > +config SM_GCC_8150
> > > > +       tristate "SM8150 Global Clock Controller"
> > > > +       help
> > > > +         Support for the global clock controller on SM8150 devices.
> > > > +         Say Y if you want to use peripheral devices such as UART,
> > > > +         SPI, I2C, USB, SD/eMMC, PCIe etc.
> > > 
> > > Is there eMMC support?
> 
> I guess no?

Nope, copy paste error, have fixed it up :)

> 
> > > > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> > > > index f0768fb1f037..4a813b4055d0 100644
> > > > --- a/drivers/clk/qcom/Makefile
> > > > +++ b/drivers/clk/qcom/Makefile
> > > > @@ -50,6 +50,7 @@ obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
> > > >  obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
> > > >  obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
> > > >  obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
> > > > +obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o
> > > >  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> > > >  obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o
> > > >  obj-$(CONFIG_QCOM_HFPLL) += hfpll.o
> > > > diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> > > > new file mode 100644
> > > > index 000000000000..11cd9e19f18d
> > > > --- /dev/null
> > > > +++ b/drivers/clk/qcom/gcc-sm8150.c
> > > > +static const struct clk_parent_data gcc_parents_5[] = {
> > > > +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> > > > +       { .fw_name = "gpll0", .name = "gpll0" },
> > > > +       { .fw_name = "gpll7", .name = "gpll7" },
> > > > +       { .fw_name = "gpll0_out_even", .name = "gpll0_out_even" },
> > > 
> > > Aren't these gplls all created in this file? They shouldn't be listed in
> > > DT so I'm confused why we have .fw_name for them.
> > 
> > Yes they are and the DT doesnt provide these clock. From what I
> > understood from the name conversion to new schema was we should add it
> > like above, let me know if I missed something
> > 
> 
> Yes, you should use the direct clk_hw pointer part of the
> clk_parent_data structure instead of having a .fw_name or .name member
> in these rows. Any clk inside of the clk controller shouldn't be exposed
> into DT just so that we can find it again through string comparisons.

Thanks, I think I have fixed it up. The parent clk in DT would be cxo
and sleep_clk and creating bi_tcxo,  gpll0, gpll7 etc in driver

I have updated the patchset and will post shortly

Thanks