From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Sibi Sankar <quic_sibis@quicinc.com>
Cc: sudeep.holla@arm.com, cristian.marussi@arm.com,
andersson@kernel.org, konrad.dybcio@linaro.org,
jassisinghbrar@gmail.com, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
quic_rgottimu@quicinc.com, quic_kshivnan@quicinc.com,
conor+dt@kernel.org, Amir Vajid <avajid@quicinc.com>
Subject: Re: [RFC 4/7] soc: qcom: Utilize qcom scmi vendor protocol for bus dvfs
Date: Mon, 12 Feb 2024 15:22:49 +0200 [thread overview]
Message-ID: <CAA8EJpqjqCVs6DEaUQowk2d3yS66cSC8vnVQzv+MxhvobeNvEg@mail.gmail.com> (raw)
In-Reply-To: <0adaa065-3883-ebfe-8259-05ebdbd821eb@quicinc.com>
On Mon, 12 Feb 2024 at 12:24, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>
>
>
> On 1/18/24 02:11, Dmitry Baryshkov wrote:
> > On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <quic_sibis@quicinc.com> wrote:
> >>
> >> From: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> >>
> >> This patch introduces a client driver that interacts with the SCMI QCOM
> >
> > git grep This.patch Documentation/process/
> >
> >> vendor protocol and passes on the required tuneables to start various
> >> features running on the SCMI controller.
> >
> > Is there any word about the 'memlat'? No. Unless one reads into the
> > patch, one can not come up with the idea of what is being introduced.
>
> ack, will fix it in the re-spin.
>
> >
> >>
> >> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> >> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com>
> >> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com>
> >> Co-developed-by: Amir Vajid <avajid@quicinc.com>
> >> Signed-off-by: Amir Vajid <avajid@quicinc.com>
> >> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com>
> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> >> ---
> >> drivers/soc/qcom/Kconfig | 10 +
> >> drivers/soc/qcom/Makefile | 1 +
> >> drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
> >
> > Should it go to drivers/firmware/arm_scmi instead? Or maybe to drivers/devfreq?
>
> I don't think it should go into arm_scmi unless Sudeep wants it there.
> As for drivers/devfreq, I would have moved it there if this driver
> benfitted being classified as a devfreq device. We can't use any of
> the available governors on it and the tuneables appear way too custom.
> These are the reasons why I placed it in drivers/soc/qcom instead.
>
> >
> >> 3 files changed, 497 insertions(+)
> >> create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
> >>
> >> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> >> index c6ca4de42586..1530558aebfb 100644
> >> --- a/drivers/soc/qcom/Kconfig
> >> +++ b/drivers/soc/qcom/Kconfig
> >> @@ -264,6 +264,16 @@ config QCOM_ICC_BWMON
> >> the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
> >> memory throughput even with lower CPU frequencies.
> >>
> >> +config QCOM_SCMI_CLIENT
> >> + tristate "Qualcomm Technologies Inc. SCMI client driver"
> >> + depends on QCOM_SCMI_VENDOR_PROTOCOL || COMPILE_TEST
> >> + default n
> >> + help
> >> + SCMI client driver registers for SCMI QCOM vendor protocol.
> >> +
> >> + This driver interacts with the vendor protocol and passes on the required
> >> + tuneables to start various features running on the SCMI controller.
> >> +
> >> config QCOM_INLINE_CRYPTO_ENGINE
> >> tristate
> >> select QCOM_SCM
> >> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> >> index 05b3d54e8dc9..c2a51293c886 100644
> >> --- a/drivers/soc/qcom/Makefile
> >> +++ b/drivers/soc/qcom/Makefile
> >> @@ -32,5 +32,6 @@ obj-$(CONFIG_QCOM_APR) += apr.o
> >> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> >> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> >> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> >> +obj-$(CONFIG_QCOM_SCMI_CLIENT) += qcom_scmi_client.o
> >> qcom_ice-objs += ice.o
> >> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
> >> diff --git a/drivers/soc/qcom/qcom_scmi_client.c b/drivers/soc/qcom/qcom_scmi_client.c
> >> new file mode 100644
> >> index 000000000000..418aa7900496
> >> --- /dev/null
> >> +++ b/drivers/soc/qcom/qcom_scmi_client.c
> >> @@ -0,0 +1,486 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#include <linux/cpu.h>
> >> +#include <linux/err.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/init.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/qcom_scmi_vendor.h>
> >> +#include <linux/scmi_protocol.h>
> >> +
> >> +#define MAX_MEMORY_TYPES 3
> >> +#define MEMLAT_ALGO_STR 0x74616C6D656D /* "memlat" */
> >> +#define INVALID_IDX 0xFF
> >> +#define MAX_NAME_LEN 20
> >> +#define MAX_MAP_ENTRIES 6
> >> +#define MAX_MONITOR_CNT 4
> >> +#define SCMI_VENDOR_MSG_START 3
> >> +#define SCMI_VENDOR_MSG_MODULE_START 16
> >> +
> >> +enum scmi_memlat_protocol_cmd {
> >> + MEMLAT_SET_LOG_LEVEL = SCMI_VENDOR_MSG_START,
> >> + MEMLAT_FLUSH_LOGBUF,
> >> + MEMLAT_SET_MEM_GROUP = SCMI_VENDOR_MSG_MODULE_START,
> >> + MEMLAT_SET_MONITOR,
> >> + MEMLAT_SET_COMMON_EV_MAP,
> >> + MEMLAT_SET_GRP_EV_MAP,
> >> + MEMLAT_ADAPTIVE_LOW_FREQ,
> >> + MEMLAT_ADAPTIVE_HIGH_FREQ,
> >> + MEMLAT_GET_ADAPTIVE_CUR_FREQ,
> >> + MEMLAT_IPM_CEIL,
> >> + MEMLAT_FE_STALL_FLOOR,
> >> + MEMLAT_BE_STALL_FLOOR,
> >> + MEMLAT_WB_PCT,
> >> + MEMLAT_IPM_FILTER,
> >> + MEMLAT_FREQ_SCALE_PCT,
> >> + MEMLAT_FREQ_SCALE_CEIL_MHZ,
> >> + MEMLAT_FREQ_SCALE_FLOOR_MHZ,
> >> + MEMLAT_SAMPLE_MS,
> >> + MEMLAT_MON_FREQ_MAP,
> >> + MEMLAT_SET_MIN_FREQ,
> >> + MEMLAT_SET_MAX_FREQ,
> >> + MEMLAT_GET_CUR_FREQ,
> >> + MEMLAT_START_TIMER,
> >> + MEMLAT_STOP_TIMER,
> >> + MEMLAT_GET_TIMESTAMP,
> >> + MEMLAT_MAX_MSG
> >> +};
> >> +
> >> +struct map_table {
> >> + u16 v1;
> >> + u16 v2;
> >> +};
> >> +
> >
> > Any documentation for these structures? It won't bite you, but it will
> > help reviewers and other developers.
>
> ack
>
> >
> >> +struct map_param_msg {
> >> + u32 hw_type;
> >> + u32 mon_idx;
> >> + u32 nr_rows;
> >> + struct map_table tbl[MAX_MAP_ENTRIES];
> >> +} __packed;
> >> +
> >> +struct node_msg {
> >> + u32 cpumask;
> >> + u32 hw_type;
> >> + u32 mon_type;
> >> + u32 mon_idx;
> >> + char mon_name[MAX_NAME_LEN];
> >> +};
> >> +
> >> +struct scalar_param_msg {
> >> + u32 hw_type;
> >> + u32 mon_idx;
> >> + u32 val;
> >> +};
> >> +
> >> +enum common_ev_idx {
> >> + INST_IDX,
> >> + CYC_IDX,
> >> + FE_STALL_IDX,
> >> + BE_STALL_IDX,
> >> + NUM_COMMON_EVS
> >> +};
> >> +
> >> +enum grp_ev_idx {
> >> + MISS_IDX,
> >> + WB_IDX,
> >> + ACC_IDX,
> >> + NUM_GRP_EVS
> >> +};
> >> +
> >> +#define EV_CPU_CYCLES 0
> >> +#define EV_INST_RETIRED 2
> >> +#define EV_L2_D_RFILL 5
> >> +
> >> +struct ev_map_msg {
> >> + u32 num_evs;
> >> + u32 hw_type;
> >> + u32 cid[NUM_COMMON_EVS];
> >> +};
> >> +
> >> +struct cpufreq_memfreq_map {
> >> + unsigned int cpufreq_mhz;
> >> + unsigned int memfreq_khz;
> >> +};
> >> +
> >> +struct scmi_monitor_info {
> >> + struct cpufreq_memfreq_map *freq_map;
> >> + char mon_name[MAX_NAME_LEN];
> >> + u32 mon_idx;
> >> + u32 mon_type;
> >> + u32 ipm_ceil;
> >> + u32 mask;
> >> + u32 freq_map_len;
> >> +};
> >> +
> >> +struct scmi_memory_info {
> >> + struct scmi_monitor_info *monitor[MAX_MONITOR_CNT];
> >> + u32 hw_type;
> >> + int monitor_cnt;
> >> + u32 min_freq;
> >> + u32 max_freq;
> >> +};
> >> +
> >> +struct scmi_memlat_info {
> >> + struct scmi_protocol_handle *ph;
> >> + const struct qcom_scmi_vendor_ops *ops;
> >> + struct scmi_memory_info *memory[MAX_MEMORY_TYPES];
> >> + int memory_cnt;
> >> +};
> >> +
> >> +static int get_mask(struct device_node *np, u32 *mask)
> >> +{
> >> + struct device_node *dev_phandle;
> >> + struct device *cpu_dev;
> >> + int cpu, i = 0;
> >> + int ret = -ENODEV;
> >> +
> >> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> >> + while (dev_phandle) {
> >> + for_each_possible_cpu(cpu) {
> >> + cpu_dev = get_cpu_device(cpu);
> >> + if (cpu_dev && cpu_dev->of_node == dev_phandle) {
> >> + *mask |= BIT(cpu);
> >> + ret = 0;
> >> + break;
> >> + }
> >> + }
> >
> > There is of_cpu_node_to_id(). No need to reinvent it.
>
> ack
>
> >
> >> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static struct cpufreq_memfreq_map *init_cpufreq_memfreq_map(struct device *dev,
> >> + struct device_node *of_node,
> >> + u32 *cnt)
> >> +{
> >> + int len, nf, i, j;
> >> + u32 data;
> >> + struct cpufreq_memfreq_map *tbl;
> >> + int ret;
> >> +
> >> + if (!of_find_property(of_node, "qcom,cpufreq-memfreq-tbl", &len))
> >> + return NULL;
> >> + len /= sizeof(data);
> >> +
> >> + if (len % 2 || len == 0)
> >> + return NULL;
> >> + nf = len / 2;
> >> +
> >> + tbl = devm_kzalloc(dev, (nf + 1) * sizeof(struct cpufreq_memfreq_map),
> >> + GFP_KERNEL);
> >> + if (!tbl)
> >> + return NULL;
> >> +
> >> + for (i = 0, j = 0; i < nf; i++, j += 2) {
> >> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
> >> + j, &data);
> >> + if (ret < 0)
> >> + return NULL;
> >> + tbl[i].cpufreq_mhz = data / 1000;
> >> +
> >> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
> >> + j + 1, &data);
> >> + if (ret < 0)
> >> + return NULL;
> >> +
> >> + tbl[i].memfreq_khz = data;
> >> + pr_debug("Entry%d CPU:%u, Mem:%u\n", i, tbl[i].cpufreq_mhz,
> >> + tbl[i].memfreq_khz);
> >> + }
> >> + *cnt = nf;
> >> + tbl[i].cpufreq_mhz = 0;
> >
> > This looks like a lame version of the OPP table.
>
> I didn't know listing multiple frequencies in a opp was allowed. We can
> probably get away with it here since we just parse the data here and not
> populate data in the opp core.
You are describing driver behaviour. DT describes hardware. So, the
proper way to describe this kind of data is the OPP table.
--
With best wishes
Dmitry
next prev parent reply other threads:[~2024-02-12 13:23 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-17 17:34 [RFC 0/7] firmware: arm_scmi: Qualcomm Vendor Protocol Sibi Sankar
2024-01-17 17:34 ` [RFC 1/7] dt-bindings: mailbox: qcom: Add CPUCP mailbox controller bindings Sibi Sankar
2024-01-17 19:53 ` Konrad Dybcio
2024-02-08 10:22 ` Sibi Sankar
2024-02-08 23:14 ` Konrad Dybcio
2024-02-12 5:48 ` Sibi Sankar
2024-01-30 17:12 ` Rob Herring
2024-02-08 10:28 ` Sibi Sankar
2024-02-08 15:58 ` Krzysztof Kozlowski
2024-02-28 17:37 ` Konrad Dybcio
2024-01-17 17:34 ` [RFC 2/7] mailbox: Add support for QTI CPUCP mailbox controller Sibi Sankar
2024-01-17 19:03 ` Dmitry Baryshkov
2024-01-17 17:34 ` [RFC 3/7] firmware: arm_scmi: Add QCOM vendor protocol Sibi Sankar
2024-01-17 19:09 ` Dmitry Baryshkov
2024-02-12 8:31 ` Sibi Sankar
2024-01-17 20:15 ` Konrad Dybcio
2024-01-17 20:31 ` Cristian Marussi
2024-02-08 11:44 ` Sibi Sankar
2024-02-09 22:45 ` Konrad Dybcio
2024-02-12 8:56 ` Sibi Sankar
2024-01-17 20:15 ` Konrad Dybcio
2024-01-18 17:22 ` Sudeep Holla
2024-02-12 9:14 ` Sibi Sankar
2024-02-12 17:39 ` Cristian Marussi
2024-02-29 14:16 ` Sudeep Holla
2024-02-29 14:24 ` Sudeep Holla
2024-01-17 17:34 ` [RFC 4/7] soc: qcom: Utilize qcom scmi vendor protocol for bus dvfs Sibi Sankar
2024-01-17 20:28 ` Konrad Dybcio
2024-02-12 10:33 ` Sibi Sankar
2024-01-17 20:41 ` Dmitry Baryshkov
2024-02-12 10:24 ` Sibi Sankar
2024-02-12 13:22 ` Dmitry Baryshkov [this message]
2024-02-20 15:07 ` Cristian Marussi
2024-02-28 17:31 ` Sibi Sankar
2024-02-29 14:27 ` Sudeep Holla
2024-02-20 16:19 ` Cristian Marussi
2024-02-29 14:41 ` Sudeep Holla
2024-01-17 17:34 ` [RFC 5/7] arm64: dts: qcom: x1e80100: Add cpucp mailbox and sram nodes Sibi Sankar
2024-01-17 17:34 ` [RFC 6/7] arm64: dts: qcom: x1e80100: Enable cpufreq Sibi Sankar
2024-01-18 15:25 ` Sudeep Holla
2024-02-12 9:28 ` Sibi Sankar
2024-01-17 17:34 ` [RFC 7/7] arm64: dts: qcom: x1e80100: Enable LLCC/DDR dvfs Sibi Sankar
2024-01-17 20:38 ` Konrad Dybcio
2024-02-12 10:05 ` Sibi Sankar
2024-01-17 20:47 ` Dmitry Baryshkov
2024-02-12 9:47 ` Sibi Sankar
2024-02-12 18:11 ` [RFC 0/7] firmware: arm_scmi: Qualcomm Vendor Protocol Cristian Marussi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAA8EJpqjqCVs6DEaUQowk2d3yS66cSC8vnVQzv+MxhvobeNvEg@mail.gmail.com \
--to=dmitry.baryshkov@linaro.org \
--cc=andersson@kernel.org \
--cc=avajid@quicinc.com \
--cc=conor+dt@kernel.org \
--cc=cristian.marussi@arm.com \
--cc=devicetree@vger.kernel.org \
--cc=jassisinghbrar@gmail.com \
--cc=konrad.dybcio@linaro.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=quic_kshivnan@quicinc.com \
--cc=quic_rgottimu@quicinc.com \
--cc=quic_sibis@quicinc.com \
--cc=robh+dt@kernel.org \
--cc=sudeep.holla@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).