Re: [PATCH 2/4] clk: qcom: Add global clock controller driver for MSM8917

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

On 21.02.2023 18:49, Otto Pflüger wrote:
> This driver provides clocks, resets and power domains needed for various
> components of the MSM8917 SoC and the very similar QM215 SoC.
> 
> According to [1] in the downstream kernel, the GPU clock has a different
> source mapping on QM215 (gcc_gfx3d_map vs gcc_gfx3d_map_qm215).
> 
> [1]: https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LF.UM.8.6.2-28000-89xx.0/include/dt-bindings/clock/msm-clocks-hwio-8952.h#L298
> 
> Signed-off-by: Otto Pflüger <otto.pflueger@abscue.de>
> ---
Thanks for working on reviving old and affordable devices!

>  drivers/clk/qcom/Kconfig       |    8 +
>  drivers/clk/qcom/Makefile      |    1 +
>  drivers/clk/qcom/gcc-msm8917.c | 3283 ++++++++++++++++++++++++++++++++
>  3 files changed, 3292 insertions(+)
>  create mode 100644 drivers/clk/qcom/gcc-msm8917.c
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 70d43f0a8919..3ef58b09385a 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -196,6 +196,14 @@ config MSM_GCC_8916
>  	  Say Y if you want to use devices such as UART, SPI i2c, USB,
>  	  SD/eMMC, display, graphics, camera etc.
>  
> +config MSM_GCC_8917
> +	tristate "MSM8917 Global Clock Controller"
Please also mention QM215

> +	select QCOM_GDSC
> +	help
> +	  Support for the global clock controller on msm8917 devices.
> +	  Say Y if you want to use devices such as UART, SPI i2c, USB,
> +	  SD/eMMC, display, graphics, camera etc.
> +
>  config MSM_GCC_8939
>  	tristate "MSM8939 Global Clock Controller"
>  	select QCOM_GDSC
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index f18c446a97ea..ff5f2c4127a9 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_MDM_LCC_9615) += lcc-mdm9615.o
>  obj-$(CONFIG_MSM_GCC_8660) += gcc-msm8660.o
>  obj-$(CONFIG_MSM_GCC_8909) += gcc-msm8909.o
>  obj-$(CONFIG_MSM_GCC_8916) += gcc-msm8916.o
> +obj-$(CONFIG_MSM_GCC_8917) += gcc-msm8917.o
>  obj-$(CONFIG_MSM_GCC_8939) += gcc-msm8939.o
>  obj-$(CONFIG_MSM_GCC_8953) += gcc-msm8953.o
>  obj-$(CONFIG_MSM_GCC_8960) += gcc-msm8960.o
> diff --git a/drivers/clk/qcom/gcc-msm8917.c b/drivers/clk/qcom/gcc-msm8917.c
> new file mode 100644
> index 000000000000..a2f4ffca18dc
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-msm8917.c
> @@ -0,0 +1,3283 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Otto Pflüger
> + *
> + * Based on gcc-msm8953.c:
> + *   Copyright 2021, The Linux Foundation. All rights reserved.
> + * with parts taken from gcc-qcs404.c:
> + *   Copyright 2018, The Linux Foundation. All rights reserved.
> + * and gcc-msm8939.c:
> + *   Copyright 2020 Linaro Limited
> + * adapted with data from clock-gcc-8952.c in Qualcomm's msm-4.9 release:
> + *   Copyright (c) 2014-2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
Please sort these
> +
> +#include <dt-bindings/clock/qcom,gcc-msm8917.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-pll.h"
> +#include "clk-rcg.h"
> +#include "common.h"
> +#include "gdsc.h"
> +#include "reset.h"

> +
> +enum {
> +	P_XO,
> +	P_SLEEP_CLK,
> +	P_GPLL0,
> +	P_GPLL3,
> +	P_GPLL4,
> +	P_GPLL6,
> +	P_DSI0PLL,
> +	P_DSI0PLL_BYTE,
> +};
> +
> +static struct clk_alpha_pll gpll0_sleep_clk_src = {
> +	.offset = 0x21000,
> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
> +	.clkr = {
> +		.enable_reg = 0x45008,
> +		.enable_mask = BIT(23),
> +		.enable_is_inverted = true,
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpll0_sleep_clk_src",
> +			.parent_data = &(const struct clk_parent_data) {
> +				.fw_name = "xo",
Please switch to .index (check sm8550 drivers for reference)

> +			},
> +			.num_parents = 1,
> +			.ops = &clk_alpha_pll_ops,
> +		},
> +	},
> +};
[...]

> +
> +static struct clk_pll gpll6_early = {
> +	.l_reg = 0x37004,
> +	.m_reg = 0x37008,
> +	.n_reg = 0x3700C,
> +	.config_reg = 0x37014,
> +	.mode_reg = 0x37000,
> +	.status_reg = 0x3701C,
Please use lowercase hex everywhere except for preprocessor
defines.

> +	.status_bit = 17,
> +	.clkr.hw.init = &(struct clk_init_data){
> +		.name = "gpll6_early",
> +		.parent_data = &(const struct clk_parent_data) {
> +			.fw_name = "xo",
> +		},
> +		.num_parents = 1,
> +		.ops = &clk_pll_ops,
> +	},
> +};
> +
[...]

> +
> +static const struct clk_parent_data gcc_pclk_data[] = {
> +	{ .fw_name = "xo" },
> +	{ .fw_name = "dsi0pll", .name = "dsi0pll" },
Since this is a new driver with no legacy leftovers, one
lookup method is enough.

(and .index would be preferred)


Konrad