[v2,0/2] Global Clock Controller driver for MSM8976/56

Message

AngeloGioacchino Del Regno Oct. 15, 2019, 10:32 a.m. UTC

From: AngeloGioacchino Del Regno <[e-mail]>

This is the Global Clock Controller (GCC) driver for MSM8956,
MSM8976 and APQ variants and it has been tested on two Sony phones
featuring the Qualcomm MSM8956 SoC.

This driver is responsible for providing clocks support for also the
MDSS and GFX3D, as these clocks are located in the GCC space here.

The personal aim is to upstream the MSM8956 SoC as much as possible
and, at the end, to add support for the Xperia X, X Compact and if
feasible also the Xperia Touch projector (APQ8056).

Changes in v2:
- Rebased onto linux-next 20191015
- Fixed platform driver name (qcom,gcc-8976 => gcc-msm8976)
- Splitted changes to dt-bindings to a separate commit

AngeloGioacchino Del Regno (2):
  clk: qcom: Add MSM8976/56 Global Clock Controller (GCC) driver
  dt-bindings: clock: Document MSM8976 gcc compatible

 .../devicetree/bindings/clock/qcom,gcc.txt    |    1 +
 drivers/clk/qcom/Kconfig                      |    8 +
 drivers/clk/qcom/Makefile                     |    1 +
 drivers/clk/qcom/gcc-msm8976.c                | 4215 +++++++++++++++++
 include/dt-bindings/clock/qcom,gcc-msm8976.h  |  293 ++
 5 files changed, 4518 insertions(+)
 create mode 100644 drivers/clk/qcom/gcc-msm8976.c
 create mode 100644 include/dt-bindings/clock/qcom,gcc-msm8976.h

Comments

Stephen Boyd Dec. 26, 2019, 10:25 p.m. UTC | #1

Quoting [e-mail] (2019-10-15 03:32:20)
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 32dbb4f09492..f2a7743c53a1 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -188,6 +188,14 @@ config MSM_MMCC_8974
>           Say Y if you want to support multimedia devices such as display,
>           graphics, video encode/decode, camera, etc.
>  
> +config MSM_GCC_8976
> +       tristate "MSM8956/76 Global Clock Controller"
> +       select QCOM_GDSC
> +       help
> +         Support for the global clock controller on msm8956/76 devices.
> +         Say Y if you want to use peripheral devices such as UART, SPI,
> +         i2c, USB, SD/eMMC, display, graphics, camera etc.

Does it have display and graphics and camera?

> +
>  config MSM_GCC_8994
>         tristate "MSM8994 Global Clock Controller"
>         help
> diff --git a/drivers/clk/qcom/gcc-msm8976.c b/drivers/clk/qcom/gcc-msm8976.c
> new file mode 100644
> index 000000000000..c670b53f0b5f
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-msm8976.c
> @@ -0,0 +1,4215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm Global Clock Controller driver for MSM8956/76
> + *
> + * Copyright (C) 2016, The Linux Foundation. All rights reserved.
> + * Copyright (C) 2018, AngeloGioacchino Del Regno <[e-mail]>
> + *
> + * Author: AngeloGioacchino Del Regno <[e-mail]>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/clk.h>

Is this include used?

> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/bitops.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>

Is this include used?

> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>

Is this include used?

> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +#include <dt-bindings/clock/qcom,gcc-msm8976.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "common.h"
> +#include "clk-pll.h"
> +#include "clk-regmap.h"
> +#include "clk-rcg.h"
> +#include "reset.h"
> +#include "gdsc.h"
[...]
> +
> +static struct clk_branch gcc_usb_fs_system_clk = {
> +       .halt_reg = 0x3F004,

Please use lowercase hex throughout the code.

> +       .clkr = {
> +               .enable_reg = 0x3F004,
> +               .enable_mask = BIT(0),
> +               .hw.init = &(struct clk_init_data) {
> +                       .name = "gcc_usb_fs_system_clk",
> +                       .parent_names = (const char*[]) {
> +                               "usb_fs_system_clk_src",
> +                       },
> +                       .num_parents = 1,
> +                       .flags = CLK_SET_RATE_PARENT,
> +                       .ops = &clk_branch2_ops,
> +               },
> +       },
> +};
> +
[...]
> +
> +static const struct of_device_id msm_clock_gcc_match_table[] = {

This name is super generic. Can you name it gcc_msm8976_match_table?

> +       { .compatible = "qcom,gcc-msm8976" },
> +       {},

Please drop the comma. This is the sentinel

> +};
> +
> +static int gcc_8976_probe(struct platform_device *pdev)
> +{
> +       struct regmap *regmap;
> +       int i, ret;
> +       u32 val;
> +
> +       regmap = qcom_cc_map(pdev, &gcc_msm8976_desc);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       /* Vote for GPLL0 to turn on. Needed by acpuclock. */
> +       regmap_update_bits(regmap, 0x45000, BIT(0), BIT(0));
> +
> +       /* Register the hws */

Please drop this useless comment.

> +       for (i = 0; i < ARRAY_SIZE(gcc_msm8976_hws); i++) {
> +               ret = devm_clk_hw_register(&pdev->dev, gcc_msm8976_hws[i]);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       ret = qcom_cc_really_probe(pdev, &gcc_msm8976_desc, regmap);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to register GCC clocks\n");

Please remove error print.

> +               return ret;
> +       }
> +
> +       clk_set_rate(apss_ahb_clk_src.clkr.hw.clk, 19200000);

Why? Isn't it 19.2MHz by default?

> +       clk_prepare_enable(apss_ahb_clk_src.clkr.hw.clk);

Looks like the clk should be marked CLK_IS_CRITICAL. Also, document why
clks are marked CLK_IS_CRITICAL with a comment.

> +
> +       /* Configure Sleep and Wakeup cycles for GMEM clock */
> +       regmap_read(regmap, 0x59024, &val);
> +       val ^= 0xFF0;
> +       val |= (0 << 8);
> +       val |= (0 << 4);
> +       regmap_write(regmap, 0x59024, val);

This is regmap_update_bits()?

> +
> +       clk_pll_configure_sr_hpm_lp(&gpll3, regmap,
> +                                       &gpll3_config, true);
> +
> +       clk_set_rate(gpll3.clkr.hw.clk, 1100000000);

Don't think this is required. The PLL configuration should do this.

> +
> +       /* Enable AUX2 clock for APSS */
> +       regmap_update_bits(regmap, 0x60000, BIT(2), BIT(2));
> +
> +       /* Oxili Ocmem in GX rail: OXILI_GMEM_CLAMP_IO */
> +       regmap_update_bits(regmap, 0x5B00C, BIT(0), 0);
> +
> +       /* Configure Sleep and Wakeup cycles for OXILI clock */
> +       val = regmap_read(regmap, 0x59020, &val);
> +       val &= ~0xF0;
> +       val |= (0 << 4);
> +       regmap_write(regmap, 0x59020, val);

regmap_update_bits()?

> +
> +       dev_dbg(&pdev->dev, "Registered GCC-8976 clocks\n");

Please remove this. It's not useful.

> +
> +       return 0;
> +}
> +
> +static struct platform_driver gcc_8976_driver = {
> +       .probe = gcc_8976_probe,
> +       .driver = {
> +               .name = "gcc-msm8976",
> +               .of_match_table = msm_clock_gcc_match_table,
> +       },
> +};
> +
> +static int __init gcc_8976_init(void)
> +{
> +       return platform_driver_register(&gcc_8976_driver);
> +}
> +core_initcall_sync(gcc_8976_init);
> +
> +static void __exit gcc_8976_exit(void)
> +{
> +       platform_driver_unregister(&gcc_8976_driver);
> +}
> +module_exit(gcc_8976_exit);
> +
> +MODULE_AUTHOR("AngeloGioacchino Del Regno <[e-mail]>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:gcc-msm8976");

I don't think we need MODULE_ALIAS anymore.