* Re: [PATCH v2 1/2] clk: qcom: Add MSM8976/56 Global Clock Controller (GCC) driver
[not found] ` <20191015103221.51345-2-kholk11@gmail.com>
@ 2019-12-26 22:25 ` Stephen Boyd
0 siblings, 0 replies; 2+ messages in thread
From: Stephen Boyd @ 2019-12-26 22:25 UTC (permalink / raw)
To: kholk11, linux-arm-msm
Cc: linux-clk, devicetree, mark.rutland, robh+dt, mturquette, agross,
bjorn.andersson, marijns95, AngeloGioacchino Del Regno
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.
^ permalink raw reply [flat|nested] 2+ messages in thread