Re: [PATCH 3/4] clk: qcom: Add clock driver for SM8450

[Konrad Dybcio <konrad.dybcio@somainline.org>]

On 01.12.2021 08:23, Vinod Koul wrote:
> This adds Global Clock controller (GCC) driver for SM8450 SoC including
> the gcc resets and gdsc.
>
> This patch is based on initial code downstream by Vivek Aknurwar
> <viveka@codeaurora.org>
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/clk/qcom/Kconfig      |    8 +
>  drivers/clk/qcom/Makefile     |    1 +
>  drivers/clk/qcom/gcc-sm8450.c | 3314 +++++++++++++++++++++++++++++++++
>  3 files changed, 3323 insertions(+)
>  create mode 100644 drivers/clk/qcom/gcc-sm8450.c
>
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 74efc82127e1..3a568e7ad886 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -618,6 +618,14 @@ config SM_GCC_8350
>  	  Say Y if you want to use peripheral devices such as UART,
>  	  SPI, I2C, USB, SD/UFS, PCIe etc.
>  
> +config SM_GCC_8450
> +	tristate "SM8450 Global Clock Controller"
> +	select QCOM_GDSC
> +	help
> +	  Support for the global clock controller on SM8450 devices.
> +	  Say Y if you want to use peripheral devices such as UART,
> +	  SPI, I2C, USB, SD/UFS, PCIe etc.
> +
>  config SM_GPUCC_8150
>  	tristate "SM8150 Graphics Clock Controller"
>  	select SM_GCC_8150
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 1718c34d3551..efb0837ea199 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -90,6 +90,7 @@ obj-$(CONFIG_SM_GCC_6350) += gcc-sm6350.o
>  obj-$(CONFIG_SM_GCC_8150) += gcc-sm8150.o
>  obj-$(CONFIG_SM_GCC_8250) += gcc-sm8250.o
>  obj-$(CONFIG_SM_GCC_8350) += gcc-sm8350.o
> +obj-$(CONFIG_SM_GCC_8450) += gcc-sm8450.o
>  obj-$(CONFIG_SM_GPUCC_8150) += gpucc-sm8150.o
>  obj-$(CONFIG_SM_GPUCC_8250) += gpucc-sm8250.o
>  obj-$(CONFIG_SM_VIDEOCC_8150) += videocc-sm8150.o
> diff --git a/drivers/clk/qcom/gcc-sm8450.c b/drivers/clk/qcom/gcc-sm8450.c
> new file mode 100644
> index 000000000000..175c18a1cdbe
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sm8450.c
> @@ -0,0 +1,3314 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2021, Linaro Limited
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,gcc-sm8450.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "clk-regmap-divider.h"
> +#include "clk-regmap-mux.h"
> +#include "gdsc.h"
> +#include "reset.h"
> +
> +enum {
> +	P_BI_TCXO,
> +	P_GCC_GPLL0_OUT_EVEN,
> +	P_GCC_GPLL0_OUT_MAIN,
> +	P_GCC_GPLL4_OUT_MAIN,
> +	P_GCC_GPLL9_OUT_MAIN,
> +	P_PCIE_0_PIPE_CLK,
> +	P_PCIE_1_PHY_AUX_CLK,
> +	P_PCIE_1_PIPE_CLK,
> +	P_SLEEP_CLK,
> +	P_UFS_PHY_RX_SYMBOL_0_CLK,
> +	P_UFS_PHY_RX_SYMBOL_1_CLK,
> +	P_UFS_PHY_TX_SYMBOL_0_CLK,
> +	P_USB3_PHY_WRAPPER_GCC_USB30_PIPE_CLK,
> +};
> +
> +static struct clk_alpha_pll gcc_gpll0 = {
> +	.offset = 0x0,
> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID_EVO],
> +	.clkr = {
> +		.enable_reg = 0x62018,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gcc_gpll0",
> +			.parent_data = &(const struct clk_parent_data){
> +				.fw_name = "bi_tcxo",
> +				.name = "bi_tcxo",

I don't think we want .name for new drivers, as we do things cleanly

from the start and don't have DT incompatibility problems.

[...]


> +
> +static int gcc_sm8450_probe(struct platform_device *pdev)
> +{
> +	struct regmap *regmap;
> +	int ret;
> +
> +	regmap = qcom_cc_map(pdev, &gcc_sm8450_desc);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks,
> +				       ARRAY_SIZE(gcc_dfs_clocks));
> +	if (ret)
> +		return ret;
> +
> +	/* FORCE_MEM_CORE_ON for ufs phy ice core clocks */
> +	regmap_update_bits(regmap, gcc_ufs_phy_ice_core_clk.halt_reg, BIT(14), BIT(14));
> +
> +	/* Keep the critical clock always-On

I think the general style for multiline comments is to start with /* and then add a newline



> +	 * gcc_camera_ahb_clk, gcc_camera_xo_clk, gcc_disp_ahb_clk,
> +	 * gcc_disp_xo_clk, gcc_gpu_cfg_ahb_clk, gcc_video_ahb_clk,
> +	 * gcc_video_xo_clk
> +	 */
> +	regmap_update_bits(regmap, 0x36004, BIT(0), BIT(0));
> +	regmap_update_bits(regmap, 0x36020, BIT(0), BIT(0));
> +	regmap_update_bits(regmap, 0x37004, BIT(0), BIT(0));
> +	regmap_update_bits(regmap, 0x3701c, BIT(0), BIT(0));
> +	regmap_update_bits(regmap, 0x81004, BIT(0), BIT(0));
> +	regmap_update_bits(regmap, 0x42004, BIT(0), BIT(0));
> +	regmap_update_bits(regmap, 0x42028, BIT(0), BIT(0));
> +
> +	ret = qcom_cc_really_probe(pdev, &gcc_sm8450_desc, regmap);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register GCC clocks\n");

That's a bad downstream leftover that we don't want..



> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "Registered GCC clocks\n");

And so is this.



> +
> +	return ret;

You can simply return qcom_cc_really_probe(pdev, &gcc_sm8450_desc, regmap);


Konrad