[v2,0/7] Add device tree and clock drivers for SM8250 SoC

Message

Venkata Narendra Kumar Gutta Jan. 24, 2020, 10:32 p.m. UTC

This series adds device tree support and clock drivers support
for SM8250 SoC.
As part of the device tree, the sm8250 dts file has basic nodes
like CPU, PSCI, intc, timer and clock controller.

Required clock controller driver and RPMH cloks are added to
support peripherals like USB.

All this configuration is added to support SM8250 to boot up to the
serial console.

V2:
 * Addressed comments on patch 2, kept new compatible in sorted order
   and fixed the additional spaces.
 * Addressed comments on patch 7, fixed the clk-cells of "sleep_clk"
   and updated name of the scm node.
 * Added reviewed-by/Acked-by tags.

Whole series:
Tested-by: Vinod Koul <[e-mail]>
Tested-by: Bjorn Andersson <[e-mail]>
  
This patchset depends on one of the RPMH clock driver fix
https://patchwork.kernel.org/patch/11318949/

Taniya Das (6):
  dt-bindings: clock: Add RPMHCC bindings for SM8250
  clk: qcom: rpmh: Add support for RPMH clocks on SM8250
  clk: qcom: clk-alpha-pll: Refactor and cleanup trion PLL
  clk: qcom: clk-alpha-pll: Add support for controlling Lucid PLLs
  dt-bindings: clock: Add SM8250 GCC clock bindings
  clk: qcom: gcc: Add global clock controller driver for SM8250

Venkata Narendra Kumar Gutta (1):
  arm64: dts: qcom: sm8250: Add sm8250 dts file

 .../devicetree/bindings/clock/qcom,gcc.yaml        |    1 +
 .../devicetree/bindings/clock/qcom,rpmhcc.yaml     |    1 +
 arch/arm64/boot/dts/qcom/Makefile                  |    1 +
 arch/arm64/boot/dts/qcom/sm8250-mtp.dts            |   29 +
 arch/arm64/boot/dts/qcom/sm8250.dtsi               |  450 +++
 drivers/clk/qcom/Kconfig                           |    7 +
 drivers/clk/qcom/Makefile                          |    1 +
 drivers/clk/qcom/clk-alpha-pll.c                   |  259 +-
 drivers/clk/qcom/clk-alpha-pll.h                   |   12 +
 drivers/clk/qcom/clk-rpmh.c                        |   25 +-
 drivers/clk/qcom/gcc-sm8250.c                      | 3720 ++++++++++++++++++++
 include/dt-bindings/clock/qcom,gcc-sm8250.h        |  271 ++
 include/dt-bindings/clock/qcom,rpmh.h              |    4 +-
 13 files changed, 4731 insertions(+), 50 deletions(-)
 create mode 100644 arch/arm64/boot/dts/qcom/sm8250-mtp.dts
 create mode 100644 arch/arm64/boot/dts/qcom/sm8250.dtsi
 create mode 100644 drivers/clk/qcom/gcc-sm8250.c
 create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8250.h

Comments

Stephen Boyd Feb. 5, 2020, 7:40 p.m. UTC | #1

Quoting Venkata Narendra Kumar Gutta (2020-01-24 14:32:26)
> diff --git a/drivers/clk/qcom/gcc-sm8250.c b/drivers/clk/qcom/gcc-sm8250.c
> new file mode 100644
> index 0000000..300187e
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sm8250.c
> @@ -0,0 +1,3720 @@
[...]
> +
> +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_parent_data_0[] = {
> +       { .fw_name = "bi_tcxo" },
> +       { .hw = &gpll0.clkr.hw },
> +       { .hw = &gpll0_out_even.clkr.hw },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
> +static const struct clk_parent_data gcc_parent_data_0_ao[] = {
> +       { .fw_name = "bi_tcxo_ao" },
> +       { .hw = &gpll0.clkr.hw },
> +       { .hw = &gpll0_out_even.clkr.hw },
> +       { .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_parent_data_1[] = {
> +       { .fw_name = "bi_tcxo" },
> +       { .hw = &gpll0.clkr.hw },
> +       { .fw_name = "sleep_clk", .name = "sleep_clk" },

Please drop .name

> +       { .hw = &gpll0_out_even.clkr.hw },
> +       { .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_parent_data_2[] = {
> +       { .fw_name = "bi_tcxo" },
> +       { .fw_name = "sleep_clk", .name = "sleep_clk" },

Please drop .name

> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};
> +
[...]
> +static const struct clk_parent_data gcc_parent_data_5[] = {
> +       { .fw_name = "bi_tcxo" },
> +       { .hw = &gpll0.clkr.hw },
> +       { .fw_name = "aud_ref_clk", .name = "aud_ref_clk" },

Why have .name? Pleas remove it.

> +       { .hw = &gpll0_out_even.clkr.hw },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },

Please drop these test inputs. I don't see any reason why they're listed.

> +};
> +
> +static const struct freq_tbl ftbl_gcc_cpuss_ahb_clk_src[] = {
> +       F(19200000, P_BI_TCXO, 1, 0, 0),
> +       { }
> +};
> +
> +static struct clk_branch gcc_sys_noc_cpuss_ahb_clk = {
> +       .halt_reg = 0x48198,
> +       .halt_check = BRANCH_HALT_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x52000,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_sys_noc_cpuss_ahb_clk",
> +                       .parent_data = &(const struct clk_parent_data){
> +                               .hw = &gcc_cpuss_ahb_postdiv_clk_src.clkr.hw,
> +                       },
> +                       .num_parents = 1,
> +                       .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};

Is there a need for this clk to be exposed? Why can't we just turn the
bit on in probe and ignore it after that? I'd prefer to not have
CLK_IS_CRITICAL in this driver unless necessary.

> +
> +static struct clk_branch gcc_tsif_ahb_clk = {
> +       .halt_reg = 0x36004,
> +       .halt_check = BRANCH_HALT_VOTED,
> +       .clkr = {
> +               .enable_reg = 0x36004,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gcc_tsif_ahb_clk",
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
[...]
> +
> +
> +static int gcc_sm8250_probe(struct platform_device *pdev)
> +{
> +       struct regmap *regmap;
> +       int ret;
> +
> +       regmap = qcom_cc_map(pdev, &gcc_sm8250_desc);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       /*
> +        * Disable the GPLL0 active input to NPU and GPU
> +        * via MISC registers.
> +        */
> +       regmap_update_bits(regmap, 0x4d110, 0x3, 0x3);
> +       regmap_update_bits(regmap, 0x71028, 0x3, 0x3);
> +
> +       /*
> +        * Keep the clocks always-ON
> +        * GCC_VIDEO_AHB_CLK, GCC_CAMERA_AHB_CLK, GCC_DISP_AHB_CLK,
> +        * GCC_CPUSS_DVM_BUS_CLK, GCC_GPU_CFG_AHB_CLK
> +        */
> +       regmap_update_bits(regmap, 0x0b004, BIT(0), BIT(0));
> +       regmap_update_bits(regmap, 0x0b008, BIT(0), BIT(0));
> +       regmap_update_bits(regmap, 0x0b00c, BIT(0), BIT(0));
> +       regmap_update_bits(regmap, 0x4818c, BIT(0), BIT(0));
> +       regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));

These look like the AHB clks above that we just enabled and then ignore.

> +
> +       ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
> +                                      ARRAY_SIZE(gcc_dfs_clocks));
> +       if (ret)
> +               return ret;
> +
> +       return qcom_cc_really_probe(pdev, &gcc_sm8250_desc, regmap);
> +}

Vinod Koul Feb. 14, 2020, 1:09 p.m. UTC | #2

On 05-02-20, 11:40, Stephen Boyd wrote:

> > +static const struct clk_parent_data gcc_parent_data_2[] = {
> > +       { .fw_name = "bi_tcxo" },
> > +       { .fw_name = "sleep_clk", .name = "sleep_clk" },
> 
> Please drop .name

Yup, will do

> > +static const struct clk_parent_data gcc_parent_data_5[] = {
> > +       { .fw_name = "bi_tcxo" },
> > +       { .hw = &gpll0.clkr.hw },
> > +       { .fw_name = "aud_ref_clk", .name = "aud_ref_clk" },
> 
> Why have .name? Pleas remove it.

Dropped...

> > +       { .hw = &gpll0_out_even.clkr.hw },
> > +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> 
> Please drop these test inputs. I don't see any reason why they're listed.

Dropped this and rest.

> > +static struct clk_branch gcc_sys_noc_cpuss_ahb_clk = {
> > +       .halt_reg = 0x48198,
> > +       .halt_check = BRANCH_HALT_VOTED,
> > +       .clkr = {
> > +               .enable_reg = 0x52000,
> > +               .enable_mask = BIT(0),
> > +               .hw.init = &(struct clk_init_data){
> > +                       .name = "gcc_sys_noc_cpuss_ahb_clk",
> > +                       .parent_data = &(const struct clk_parent_data){
> > +                               .hw = &gcc_cpuss_ahb_postdiv_clk_src.clkr.hw,
> > +                       },
> > +                       .num_parents = 1,
> > +                       .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,
> > +                       .ops = &clk_branch2_ops,
> > +               },
> > +       },
> > +};
> 
> Is there a need for this clk to be exposed? Why can't we just turn the
> bit on in probe and ignore it after that? I'd prefer to not have
> CLK_IS_CRITICAL in this driver unless necessary.

yeah moved it as setting a bit in probe..

> > +       /*
> > +        * Keep the clocks always-ON
> > +        * GCC_VIDEO_AHB_CLK, GCC_CAMERA_AHB_CLK, GCC_DISP_AHB_CLK,
> > +        * GCC_CPUSS_DVM_BUS_CLK, GCC_GPU_CFG_AHB_CLK
> > +        */
> > +       regmap_update_bits(regmap, 0x0b004, BIT(0), BIT(0));
> > +       regmap_update_bits(regmap, 0x0b008, BIT(0), BIT(0));
> > +       regmap_update_bits(regmap, 0x0b00c, BIT(0), BIT(0));
> > +       regmap_update_bits(regmap, 0x4818c, BIT(0), BIT(0));
> > +       regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));
> 
> These look like the AHB clks above that we just enabled and then ignore.

right, I think these are rest of the always-on clocks