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

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

[kholk11]

From: AngeloGioacchino Del Regno <kholk11@gmail.com>

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

-- 
2.21.0

[PATCH v2 2/2] dt-bindings: clock: Document MSM8976 gcc compatible

[kholk11]

From: AngeloGioacchino Del Regno <kholk11@gmail.com>

Document the Global Clock Controller driver (gcc-msm8976)
compatible string.
This driver is valid for MSM8976, MSM8956 and APQ variants.

Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
---
 Documentation/devicetree/bindings/clock/qcom,gcc.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
index d14362ad4132..565bba5df298 100644
--- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
+++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
@@ -15,6 +15,7 @@ Required properties :
 			"qcom,gcc-msm8974"
 			"qcom,gcc-msm8974pro"
 			"qcom,gcc-msm8974pro-ac"
+			"qcom,gcc-msm8976"
 			"qcom,gcc-msm8994"
 			"qcom,gcc-msm8996"
 			"qcom,gcc-msm8998"
-- 
2.21.0

Re: [PATCH v2 2/2] dt-bindings: clock: Document MSM8976 gcc compatible

[Rob Herring]

On Tue, 15 Oct 2019 12:32:21 +0200, kholk11@gmail.com wrote:
> From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> 
> Document the Global Clock Controller driver (gcc-msm8976)
> compatible string.
> This driver is valid for MSM8976, MSM8956 and APQ variants.
> 
> Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> ---
>  Documentation/devicetree/bindings/clock/qcom,gcc.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/2] dt-bindings: clock: Document MSM8976 gcc compatible

[Vladimir Oltean]

Hi Angelo,

On Tue, 15 Oct 2019 at 14:42, <kholk11@gmail.com> wrote:
>
> From: AngeloGioacchino Del Regno <kholk11@gmail.com>
>
> Document the Global Clock Controller driver (gcc-msm8976)
> compatible string.
> This driver is valid for MSM8976, MSM8956 and APQ variants.
>
> Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> ---
>  Documentation/devicetree/bindings/clock/qcom,gcc.txt | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> index d14362ad4132..565bba5df298 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
> @@ -15,6 +15,7 @@ Required properties :
>                         "qcom,gcc-msm8974"
>                         "qcom,gcc-msm8974pro"
>                         "qcom,gcc-msm8974pro-ac"
> +                       "qcom,gcc-msm8976"
>                         "qcom,gcc-msm8994"
>                         "qcom,gcc-msm8996"
>                         "qcom,gcc-msm8998"
> --
> 2.21.0
>

You should respin this 2-patch series because qcom,gcc.txt was
converted to qcom,gcc.yaml and this patch no longer applies.

Thanks,
-Vladimir

Re: [PATCH v2 1/2] clk: qcom: Add MSM8976/56 Global Clock Controller (GCC) driver

[Stephen Boyd]

Quoting kholk11@gmail.com (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 <kholk11@gmail.com>
> + *
> + * Author: AngeloGioacchino Del Regno <kholk11@gmail.com>
> + */
> +
> +#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 <kholk11@gmail.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:gcc-msm8976");

I don't think we need MODULE_ALIAS anymore.