| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cf1a15c5-a16b-2d23-8f6c-b3892c141c34@linaro.org>
Date: Fri, 4 Nov 2022 10:04:18 -0400
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>, agross@...nel.org
Cc: andersson@...nel.org, konrad.dybcio@...ainline.org,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, marijn.suijten@...ainline.org,
kernel@...labora.com,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...ainline.org>
Subject: Re: [PATCH 2/2] soc: qcom: Add Qualcomm Ramp Controller driver
On 04/11/2022 09:35, AngeloGioacchino Del Regno wrote:
> From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.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@...ainline.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@...labora.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@...labora.com>");
> +MODULE_DESCRIPTION("Qualcomm Ramp Controller driver");
> +MODULE_LICENSE("GPL");
Best regards,
Krzysztof
Powered by blists - more mailing lists