| Message ID | 20240117173458.2312669-1-quic_sibis@quicinc.com |
|---|---|
| Headers | show |
| Series | firmware: arm_scmi: Qualcomm Vendor Protocol | expand |
On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <quic_sibis@quicinc.com> wrote: > > From: Shivnandan Kumar <quic_kshivnan@quicinc.com> > > SCMI QCOM vendor protocol provides interface to communicate with SCMI > controller and enable vendor specific features like bus scaling capable > of running on it. > > 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/firmware/arm_scmi/Kconfig | 11 ++ > drivers/firmware/arm_scmi/Makefile | 1 + > drivers/firmware/arm_scmi/qcom_scmi_vendor.c | 160 +++++++++++++++++++ > include/linux/qcom_scmi_vendor.h | 36 +++++ > 4 files changed, 208 insertions(+) > create mode 100644 drivers/firmware/arm_scmi/qcom_scmi_vendor.c > create mode 100644 include/linux/qcom_scmi_vendor.h > > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig > index aa5842be19b2..86b5d6c18ec4 100644 > --- a/drivers/firmware/arm_scmi/Kconfig > +++ b/drivers/firmware/arm_scmi/Kconfig > @@ -180,4 +180,15 @@ config ARM_SCMI_POWER_CONTROL > called scmi_power_control. Note this may needed early in boot to catch > early shutdown/reboot SCMI requests. > > +config QCOM_SCMI_VENDOR_PROTOCOL > + tristate "Qualcomm Technologies, Inc. Qcom SCMI vendor Protocol" > + depends on ARM || ARM64 || COMPILE_TEST > + depends on ARM_SCMI_PROTOCOL > + help > + The SCMI QCOM vendor protocol provides interface to communicate with SCMI > + controller and enable vendor specific features like bus scaling. > + > + This driver defines the commands or message ID's used for this > + communication and also exposes the ops used by the clients. > + > endmenu > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile > index a7bc4796519c..eaeb788b93c6 100644 > --- a/drivers/firmware/arm_scmi/Makefile > +++ b/drivers/firmware/arm_scmi/Makefile > @@ -17,6 +17,7 @@ obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o > > obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o > +obj-$(CONFIG_QCOM_SCMI_VENDOR_PROTOCOL) += qcom_scmi_vendor.o > > ifeq ($(CONFIG_THUMB2_KERNEL)$(CONFIG_CC_IS_CLANG),yy) > # The use of R7 in the SMCCC conflicts with the compiler's use of R7 as a frame > diff --git a/drivers/firmware/arm_scmi/qcom_scmi_vendor.c b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c > new file mode 100644 > index 000000000000..878b99f0d1ef > --- /dev/null > +++ b/drivers/firmware/arm_scmi/qcom_scmi_vendor.c > @@ -0,0 +1,160 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/qcom_scmi_vendor.h> > + > +#include "common.h" > + > +#define EXTENDED_MSG_ID 0 > +#define SCMI_MAX_TX_RX_SIZE 128 > +#define PROTOCOL_PAYLOAD_SIZE 16 > +#define SET_PARAM 0x10 > +#define GET_PARAM 0x11 > +#define START_ACTIVITY 0x12 > +#define STOP_ACTIVITY 0x13 > + > +static int qcom_scmi_set_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t size) > +{ > + int ret = -EINVAL; > + struct scmi_xfer *t; > + u32 *msg; > + > + if (!ph || !ph->xops) > + return ret; Drop init of ret, return -EINVAL directly here. > + > + ret = ph->xops->xfer_get_init(ph, SET_PARAM, size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); > + if (ret) > + return ret; > + > + msg = t->tx.buf; > + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); > + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); > + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); > + *msg++ = cpu_to_le32(param_id); First, this header ops looks like a generic code which can be extracted. Second, using GENMASK here in the ops doesn't make any sense. The values will be limited to u32 anyway. > + memcpy(msg, buf, size); > + ret = ph->xops->do_xfer(ph, t); > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int qcom_scmi_get_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t tx_size, size_t rx_size) > +{ > + int ret = -EINVAL; > + struct scmi_xfer *t; > + u32 *msg; > + > + if (!ph || !ph->xops || !buf) > + return ret; Drop init of ret, return -EINVAL directly here. > + > + ret = ph->xops->xfer_get_init(ph, GET_PARAM, tx_size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); > + if (ret) > + return ret; > + > + msg = t->tx.buf; > + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); > + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); > + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); > + *msg++ = cpu_to_le32(param_id); > + memcpy(msg, buf, tx_size); > + ret = ph->xops->do_xfer(ph, t); > + if (t->rx.len > rx_size) { > + pr_err("SCMI received buffer size %zu is more than expected size %zu\n", > + t->rx.len, rx_size); > + return -EMSGSIZE; > + } > + memcpy(buf, t->rx.buf, t->rx.len); > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int qcom_scmi_start_activity(const struct scmi_protocol_handle *ph, > + void *buf, u64 algo_str, u32 param_id, size_t size) > +{ > + int ret = -EINVAL; > + struct scmi_xfer *t; > + u32 *msg; > + > + if (!ph || !ph->xops) > + return ret; You can guess the comment here. > + > + ret = ph->xops->xfer_get_init(ph, START_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); > + if (ret) > + return ret; > + > + msg = t->tx.buf; > + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); > + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); > + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); > + *msg++ = cpu_to_le32(param_id); > + memcpy(msg, buf, size); > + ret = ph->xops->do_xfer(ph, t); > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static int qcom_scmi_stop_activity(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t size) > +{ > + int ret = -EINVAL; > + struct scmi_xfer *t; > + u32 *msg; > + > + if (!ph || !ph->xops) > + return ret; > + > + ret = ph->xops->xfer_get_init(ph, STOP_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE, > + SCMI_MAX_TX_RX_SIZE, &t); > + if (ret) > + return ret; > + > + msg = t->tx.buf; > + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); > + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); > + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); > + *msg++ = cpu_to_le32(param_id); > + memcpy(msg, buf, size); > + ret = ph->xops->do_xfer(ph, t); > + ph->xops->xfer_put(ph, t); > + > + return ret; > +} > + > +static struct qcom_scmi_vendor_ops qcom_proto_ops = { > + .set_param = qcom_scmi_set_param, > + .get_param = qcom_scmi_get_param, > + .start_activity = qcom_scmi_start_activity, > + .stop_activity = qcom_scmi_stop_activity, > +}; > + > +static int qcom_scmi_vendor_protocol_init(const struct scmi_protocol_handle *ph) > +{ > + u32 version; > + > + ph->xops->version_get(ph, &version); > + > + dev_info(ph->dev, "qcom scmi version %d.%d\n", > + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); > + > + return 0; > +} > + > +static const struct scmi_protocol qcom_scmi_vendor = { > + .id = QCOM_SCMI_VENDOR_PROTOCOL, > + .owner = THIS_MODULE, > + .instance_init = &qcom_scmi_vendor_protocol_init, > + .ops = &qcom_proto_ops, > +}; > +module_scmi_protocol(qcom_scmi_vendor); > + > +MODULE_DESCRIPTION("QTI SCMI vendor protocol"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/qcom_scmi_vendor.h b/include/linux/qcom_scmi_vendor.h > new file mode 100644 > index 000000000000..bde57bb18367 > --- /dev/null > +++ b/include/linux/qcom_scmi_vendor.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * QTI SCMI vendor protocol's header > + * > + * Copyright (c) 2024, The Linux Foundation. All rights reserved. > + */ > + > +#ifndef _QCOM_SCMI_VENDOR_H > +#define _QCOM_SCMI_VENDOR_H > + > +#include <linux/bitfield.h> > +#include <linux/device.h> > +#include <linux/types.h> > + > +#define QCOM_SCMI_VENDOR_PROTOCOL 0x80 > + > +struct scmi_protocol_handle; > +extern struct scmi_device *get_qcom_scmi_device(void); > + > +/** > + * struct qcom_scmi_vendor_ops - represents the various operations provided > + * by qcom scmi vendor protocol > + */ > +struct qcom_scmi_vendor_ops { > + int (*set_param)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t size); > + int (*get_param)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t tx_size, size_t rx_size); > + int (*start_activity)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t size); > + int (*stop_activity)(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t size); > +}; > + > +#endif /* _QCOM_SCMI_VENDOR_H */ > + > -- > 2.34.1 > >
On 1/17/24 18:34, Sibi Sankar wrote: > Add devicetree binding for CPUSS Control Processor (CPUCP) mailbox > controller. > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- [...] > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + > + mailbox@17430000 { > + compatible = "qcom,x1e80100-cpucp-mbox", "qcom,cpucp-mbox"; > + reg = <0x17430000 0x10000>, <0x18830000 0x300>; These reg spaces are quite far apart.. On 7280-8550, a similar mailbox exists, although it's dubbed RIMPS-mbox instead. In that case, I separated the mbox into tx (via qcom-apcs-ipc-mailbox.c) and rx (with a simple driver). Still haven't pushed or posted that anywhere, I'd need to access another machine.. On (some of) these SoCs, one of the channels (rx[1], iirc?) clearly bleeds into the CPUFREQ_HW/OSM register region, which gives an impression of misrepresenting the hardware. X1E doesn't have a node for cpufreq_hw defined, so I can't tell whether it's also the case here. Konrad
On 1/17/24 18:34, Sibi Sankar wrote: > From: Shivnandan Kumar <quic_kshivnan@quicinc.com> > > SCMI QCOM vendor protocol provides interface to communicate with SCMI > controller and enable vendor specific features like bus scaling capable > of running on it. > > 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> > --- [...] > + > +static int qcom_scmi_set_param(const struct scmi_protocol_handle *ph, void *buf, u64 algo_str, > + u32 param_id, size_t size) > +{ > + int ret = -EINVAL; > + struct scmi_xfer *t; > + u32 *msg; After you apply Dmitry's suggestions on returning -EINVAL directly, you can also sort definitions in a reverse-Christmas- tree order throughout the file. > + msg = t->tx.buf; > + *msg++ = cpu_to_le32(EXTENDED_MSG_ID); > + *msg++ = cpu_to_le32(algo_str & GENMASK(31, 0)); > + *msg++ = cpu_to_le32((algo_str & GENMASK(63, 32)) >> 32); lower/upper_32_bits()? [...] > + if (t->rx.len > rx_size) { > + pr_err("SCMI received buffer size %zu is more than expected size %zu\n", > + t->rx.len, rx_size); > + return -EMSGSIZE; No other driver seems to be checking for this, should this: a) go to common code b) be ignored ? Konrad
On Wed, Jan 17, 2024 at 09:15:40PM +0100, Konrad Dybcio wrote: > > > On 1/17/24 18:34, Sibi Sankar wrote: > > From: Shivnandan Kumar <quic_kshivnan@quicinc.com> > > Hi, a few early remarks, I am gonna look at this better next week. > > SCMI QCOM vendor protocol provides interface to communicate with SCMI > > controller and enable vendor specific features like bus scaling capable > > of running on it. > > "QCOM protocol" sounds overly generic, especially given how many > different vendor protocols have historically been present in > QC firmware.. > I was going to raise the same point :D, usually the name identifies the aim of the protocol (and the vendor also in this case) > > > > 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> > > --- > > So, this is another 0x80 protocol, different to the one that has > been shipping on devices that got released with msm-5.4, msm-5.10 > and msm-5.15 [1][2]. They're totally incompatible (judging by the > msg format), use the same protocol ID and they are (at a glance) > providing access to the same HW/FW/tunables. > > I'm not sure if this can be trusted not to change again.. Unless > we get a strong commitment that all platforms (compute, mobile, > auto, iot, whatever) stick to this one.. > > That said, the spec (DEN0056C) says that protocol IDs 0x80-0xff > are: "Reserved for vendor or platform-specific extensions to > this interface.". So if perhaps there's a will to maintain > multiple versions of this, with a way to discern between them.. > Just recently we had a discussion with some other vendor about this possible clashing of vendor protocols numbers between different vendors/platforms, especially if aiming to just push all in defconfig. The basic idea to solve this, which I am going to post shortly in the next weeks, was to add a way to define and register your protocol number associated with a vendor identifier(s) of some kind, since vendor/subvendor/firmware versions are advertised by the Platform and are retrieved via Base protocol at probe time upfront; this way it 'should' be feasible to compile in any existent vendor protocol but allow at run-time only the registration with the SCMI core of the protocols whose vendor identity matches that of the identity advertised by the running firmware.... ...still not sure which of the IDs to use vendor/subvendor and still not have really experimented with this...so any feedback welcome. This would rule out, anyway, the capability of solving number clashes within the same vendor. Thanks, Cristian
On Wed, 17 Jan 2024 at 19:37, Sibi Sankar <quic_sibis@quicinc.com> wrote: > > Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol. Could you please post DT bindings? > > Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com> > --- > arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > index 6856a206f7fc..3dc6f32fbb4c 100644 > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 { > reg = <0x13>; > #clock-cells = <1>; > }; > + > + scmi_vendor: protocol@80 { > + reg = <0x80>; > + > + memlat { This doesn't look like a generic node name. > + #address-cells = <1>; > + #size-cells = <0>; > + > + memory@0 { > + reg = <0x0>; /* Memory Type DDR */ > + freq-table-khz = <200000 4224000>; > + > + monitor-0 { > + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; > + qcom,cpufreq-memfreq-tbl = < 999000 547000 >, > + < 1440000 768000 >, > + < 1671000 1555000 >, > + < 2189000 2092000 >, > + < 2156000 3187000 >, > + < 3860000 4224000 >; These tables should be rewritten as OPP tables. > + }; > + > + monitor-1 { > + qcom,compute-mon; > + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; > + qcom,cpufreq-memfreq-tbl = < 1440000 200000 >, > + < 2189000 768000 >, > + < 2156000 1555000 >, > + < 3860000 2092000 >; > + }; > + }; > + > + memory@1 { > + reg = <0x1>; /* Memory Type LLCC */ > + freq-table-khz = <300000 1067000>; > + > + monitor-0 { > + qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>; > + qcom,cpufreq-memfreq-tbl = < 999000 300000 >, > + < 1440000 466000 >, > + < 1671000 600000 >, > + < 2189000 806000 >, > + < 2156000 933000 >, > + < 3860000 1066000 >; > + }; > + }; > + }; > + }; > }; > }; > > -- > 2.34.1 > > -- With best wishes Dmitry