Re: [PATCH 2/2] soc: qcom: Add Qualcomm Ramp Controller driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 04/11/2022 09:35, AngeloGioacchino Del Regno wrote:
> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> 
> The Ramp Controller is used to program the sequence ID for pulse
> swallowing, enable sequence and linking sequence IDs for the CPU
> cores on some Qualcomm SoCs.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  drivers/soc/qcom/Kconfig           |   9 +
>  drivers/soc/qcom/Makefile          |   1 +
>  drivers/soc/qcom/ramp_controller.c | 330 +++++++++++++++++++++++++++++
>  3 files changed, 340 insertions(+)
>  create mode 100644 drivers/soc/qcom/ramp_controller.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 024e420f1bb7..1e681f98bad4 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -95,6 +95,15 @@ config QCOM_QMI_HELPERS
>  	tristate
>  	depends on NET
>  
> +config QCOM_RAMP_CTRL
> +	tristate "Qualcomm Ramp Controller driver"
> +	depends on ARCH_QCOM

I propose:
depends on ARCH_QCOM && ARM || COMPILE_TEST

I don't think it is used on ARM64 SoCs, so let's make life of distros
easier.

> +	help
> +	  The Ramp Controller is used to program the sequence ID for pulse
> +	  swallowing, enable sequence and linking sequence IDs for the
> +	  CPU cores on some Qualcomm SoCs.
> +	  Say y here to enable support for the ramp controller.
> +
>  config QCOM_RMTFS_MEM
>  	tristate "Qualcomm Remote Filesystem memory driver"
>  	depends on ARCH_QCOM
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index d66604aff2b0..6e02333c4080 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o
>  obj-$(CONFIG_QCOM_PDR_HELPERS)	+= pdr_interface.o
>  obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
>  qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
> +obj-$(CONFIG_QCOM_RAMP_CTRL)	+= ramp_controller.o
>  obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
>  obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
>  qcom_rpmh-y			+= rpmh-rsc.o
> diff --git a/drivers/soc/qcom/ramp_controller.c b/drivers/soc/qcom/ramp_controller.c
> new file mode 100644
> index 000000000000..e28679b545d1
> --- /dev/null
> +++ b/drivers/soc/qcom/ramp_controller.c
> @@ -0,0 +1,330 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm Ramp Controller driver
> + * Copyright (c) 2022, AngeloGioacchino Del Regno
> + *                     <angelogioacchino.delregno@collabora.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +#define RC_UPDATE_EN		BIT(0)
> +#define RC_ROOT_EN		BIT(1)
> +
> +#define RC_REG_CFG_UPDATE	0x60
> + #define RC_CFG_UPDATE_EN	BIT(8)
> + #define RC_CFG_ACK		GENMASK(31, 16)

Drop spaces before #define

> +
> +#define RC_DCVS_CFG_SID		2
> +#define RC_LINK_SID		3
> +#define RC_LMH_SID		6
> +#define RC_DFS_SID		14
> +
> +#define RC_UPDATE_TIMEOUT_US	500
> +
> +/**
> + * struct qcom_ramp_controller_desc - SoC specific parameters
> + * @cfg_dfs_sid:      Dynamic Frequency Scaling SID configuration
> + * @cfg_link_sid:     Link SID configuration
> + * @cfg_lmh_sid:      Limits Management hardware SID configuration
> + * @cfg_ramp_pre_en:  Ramp Controller pre-enable sequence
> + * @cfg_ramp_en:      Ramp Controller enable sequence
> + * @cfg_ramp_post_en: Ramp Controller post-enable sequence
> + * @cfg_ramp_dis:     Ramp Controller disable sequence
> + * @cmd_reg:          Command register offset
> + * @num_dfs_sids:     Number of DFS SIDs (max 8)
> + * @num_link_sids:    Number of Link SIDs (max 3)
> + * @num_lmh_sids:     Number of LMh SIDs (max 8)
> + */
> +struct qcom_ramp_controller_desc {
> +	struct reg_sequence *cfg_dfs_sid;

I didn't check much, but can these be pointers to const?

> +	struct reg_sequence *cfg_link_sid;
> +	struct reg_sequence *cfg_lmh_sid;
> +	struct reg_sequence *cfg_ramp_pre_en;
> +	struct reg_sequence *cfg_ramp_en;
> +	struct reg_sequence *cfg_ramp_post_en;
> +	struct reg_sequence *cfg_ramp_dis;
> +	u8 cmd_reg;
> +	u8 num_dfs_sids;
> +	u8 num_link_sids;
> +	u8 num_lmh_sids;
> +};
> +

(...)

> +
> +static struct platform_driver qcom_ramp_controller_driver = {
> +	.driver = {
> +		.name = "qcom-ramp-controller",
> +		.of_match_table = qcom_ramp_controller_match_table,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe  = qcom_ramp_controller_probe,
> +	.remove = qcom_ramp_controller_remove,
> +};
> +
> +static int __init qcom_ramp_controller_init(void)
> +{
> +	return platform_driver_register(&qcom_ramp_controller_driver);
> +}
> +arch_initcall(qcom_ramp_controller_init);

Does it really have to be arch initcall? Cannot be module platform driver?

> +
> +MODULE_AUTHOR("AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>");
> +MODULE_DESCRIPTION("Qualcomm Ramp Controller driver");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof