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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 04/11/22 15:04, Krzysztof Kozlowski ha scritto:
> 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.
> 

Agreed.

>> +	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?
> 

Yeah, const is the way. Will do.

>> +	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?
> 

Cannot be platform driver. This has to initialize as early as possible, or
booting will be unstable in some cases (big cluster enabled).

Cheers!
Angelo