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

[Stephen Boyd <sboyd@kernel.org>]

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.