[RFC,3/7] firmware: arm_scmi: Add QCOM vendor protocol

  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
  

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:
> 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.

"QCOM protocol" sounds overly generic, especially given how many
different vendor protocols have historically been present in
QC firmware..

> 
> 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..

Konrad

[1] https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/drivers/firmware/arm_scmi/memlat_vendor.c?ref_type=tags
[2] https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/include/linux/scmi_memlat.h#L16
  

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, Jan 17, 2024 at 11:04:54PM +0530, 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>
> ---
>  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.
> +

I assume it will include all the Qualcomm specific vendor protocol
handling here. Not sure how it it implemented across different platforms
and but I already assume different platforms will use same protocol id
for different things and this implementation will abstract all those
details.

> 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

This gives me no clue what this means ?

> +#define	SCMI_MAX_TX_RX_SIZE		128
> +#define	PROTOCOL_PAYLOAD_SIZE		16
> +#define	SET_PARAM			0x10

I assume these are the actual message IDs ? Any idea why 0x0-0xF is skipped ?
I assume atleast the required 0x0-0x2 are implemented.

> +#define	GET_PARAM			0x11
> +#define	START_ACTIVITY			0x12
> +#define	STOP_ACTIVITY			0x13

In general, good to add description of these in the implementation here
or under Documentation or a pointer to the url where I can get the info.
If documenting within the kernel, please use SCMI spec format as it may
be easy to follow the same pattern even in the vendor protocols.

> +
> +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;
> +
> +	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);
> +	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;
> +
> +	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;
> +
> +	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,

As Cristian might have pointed out, this will conflict and we need better
matching to ensure each vendor and protocols with each implementation has
unique matching mechanism so that only one match occurs per protocol on
any platform.
  

On 1/18/24 01:45, Konrad Dybcio wrote:
> 
> 
> 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.
> 

Hey Konrad,

> "QCOM protocol" sounds overly generic, especially given how many
> different vendor protocols have historically been present in
> QC firmware..

Here it is specifically mentioned that way to communicate that
this is the only vendor protocol exposed by Qualcomm. It handles
all the other protocols which were usually handled separately on
older SoCs.

> 
>>
>> 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.

Thanks for bringing this up but like I already explained the only
SoC that was actually shipped with ^^ protocol was SC7180 and we
already have an alternative arrangement for memory dvfs upstreamed
on it. Further more it handles only L3 dvfs so it makes zero sense
to try to upstream the older protocol given that working dvfs solution
already exists upstream. All other SoCs don't have the 0x80 protocol
enabled for memory dvfs in production.

> 
> 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..

This is exactly that consolidation effort from Qualcomm. Here they
expose just one vendor protocol and implement all the algorithms just
through it.

> 
> 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..
> 
> Konrad
> 
> [1] 
> https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/drivers/firmware/arm_scmi/memlat_vendor.c?ref_type=tags
> [2] 
> https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/blob/KERNEL.PLATFORM.2.1.r5-05400-kernel.0/include/linux/scmi_memlat.h#L16
  

On 8.02.2024 12:44, Sibi Sankar wrote:
> 
> 
> On 1/18/24 01:45, Konrad Dybcio wrote:
>>
>>
>> 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.
>>
> 
> Hey Konrad,
> 
>> "QCOM protocol" sounds overly generic, especially given how many
>> different vendor protocols have historically been present in
>> QC firmware..
> 
> Here it is specifically mentioned that way to communicate that
> this is the only vendor protocol exposed by Qualcomm. It handles
> all the other protocols which were usually handled separately on
> older SoCs.

I'm no SCMI specialist but that's a rather.. peculiar design decision,
I guess


> 
>>
>>>
>>> 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.
> 
> Thanks for bringing this up but like I already explained the only
> SoC that was actually shipped with ^^ protocol was SC7180 and we
> already have an alternative arrangement for memory dvfs upstreamed
> on it.

Ok, that makes sense.

I took my 8550 phone, enabled some debug prints and it looks like the
only SCMI protocol exposed is 0x19 (which doesn't seem to be defined).

Not sure what other devices would spit out, but I assume what you said
is true.

For completeness, the reported rev is:

arm-scmi firmware:scmi: SCMI Protocol v2.0 'Qualcomm:' Firmware version 0x10000

> Further more it handles only L3 dvfs so it makes zero sense
> to try to upstream the older protocol given that working dvfs solution
> already exists upstream.

We don't have any sort of governor for it though, so I wouldn't go as
far as calling it working :P

> All other SoCs don't have the 0x80 protocol
> enabled for memory dvfs in production.
> 
>>
>> 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..
> 
> This is exactly that consolidation effort from Qualcomm. Here they
> expose just one vendor protocol and implement all the algorithms just
> through it.

And I'm very glad you're taking such consolidation steps.. Just a little
worried that in case this protocol's extensibility is exhausted, the next
one would need to be called.. "Qualcomm2"?

Konrad
  

On 1/18/24 00:39, 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>
>>
>> SCMI QCOM vendor protocol provides interface to communicate with SCMI
>> controller and enable vendor specific features like bus scaling capable
>> of running on it.
>>

Hey Dmitry,

Thanks for taking time to review the series!

Will get all of these done in the next 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/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 2/10/24 04:15, Konrad Dybcio wrote:
> On 8.02.2024 12:44, Sibi Sankar wrote:
>>
>>
>> On 1/18/24 01:45, Konrad Dybcio wrote:
>>>
>>>
>>> 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.
>>>
>>
>> Hey Konrad,
>>
>>> "QCOM protocol" sounds overly generic, especially given how many
>>> different vendor protocols have historically been present in
>>> QC firmware..
>>
>> Here it is specifically mentioned that way to communicate that
>> this is the only vendor protocol exposed by Qualcomm. It handles
>> all the other protocols which were usually handled separately on
>> older SoCs.
> 
> I'm no SCMI specialist but that's a rather.. peculiar design decision,
> I guess
> 
> 
>>
>>>
>>>>
>>>> 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.
>>
>> Thanks for bringing this up but like I already explained the only
>> SoC that was actually shipped with ^^ protocol was SC7180 and we
>> already have an alternative arrangement for memory dvfs upstreamed
>> on it.
> 
> Ok, that makes sense.
> 
> I took my 8550 phone, enabled some debug prints and it looks like the
> only SCMI protocol exposed is 0x19 (which doesn't seem to be defined).
> 
> Not sure what other devices would spit out, but I assume what you said
> is true.
> 
> For completeness, the reported rev is:
> 
> arm-scmi firmware:scmi: SCMI Protocol v2.0 'Qualcomm:' Firmware version 0x10000
> 
>> Further more it handles only L3 dvfs so it makes zero sense
>> to try to upstream the older protocol given that working dvfs solution
>> already exists upstream.
> 
> We don't have any sort of governor for it though, so I wouldn't go as
> far as calling it working :P

It is a working solution (it is equivalent to the compute mon mapping in
downstream implementation) but isn't feature complete ¯\_(ツ)_/¯.

> 
>> All other SoCs don't have the 0x80 protocol
>> enabled for memory dvfs in production.
>>
>>>
>>> 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..
>>
>> This is exactly that consolidation effort from Qualcomm. Here they
>> expose just one vendor protocol and implement all the algorithms just
>> through it.
> 
> And I'm very glad you're taking such consolidation steps.. Just a little
> worried that in case this protocol's extensibility is exhausted, the next
> one would need to be called.. "Qualcomm2"?

We don't see ^^ happening in the near future (meaning this doesn't apply
to just X1E). The consolidation would still be better than spinning out
n number of protocols per SoC.

-Sibi

> 
> Konrad
  

On 1/18/24 22:52, Sudeep Holla wrote:
> On Wed, Jan 17, 2024 at 11:04:54PM +0530, 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.

Hey Sudeep,

Thanks for taking time to review the series!

>>
>> 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.
>> +
> 
> I assume it will include all the Qualcomm specific vendor protocol
> handling here. Not sure how it it implemented across different platforms
> and but I already assume different platforms will use same protocol id
> for different things and this implementation will abstract all those
> details.

Yes, that's what we are going for.

> 
>> 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
> 
> This gives me no clue what this means ?
> 
>> +#define	SCMI_MAX_TX_RX_SIZE		128
>> +#define	PROTOCOL_PAYLOAD_SIZE		16
>> +#define	SET_PARAM			0x10
> 
> I assume these are the actual message IDs ? Any idea why 0x0-0xF is skipped ?
> I assume atleast the required 0x0-0x2 are implemented.

Yup 0x0-0x2 should be implemented. I'll have to get info on why the rest
were skipped. Will add comments detailing the extended msg id as well.

> 
>> +#define	GET_PARAM			0x11
>> +#define	START_ACTIVITY			0x12
>> +#define	STOP_ACTIVITY			0x13
> 
> In general, good to add description of these in the implementation here
> or under Documentation or a pointer to the url where I can get the info.
> If documenting within the kernel, please use SCMI spec format as it may
> be easy to follow the same pattern even in the vendor protocols.
> 

ack

>> +
>> +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;
>> +
>> +	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);
>> +	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;
>> +
>> +	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;
>> +
>> +	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,
> 
> As Cristian might have pointed out, this will conflict and we need better
> matching to ensure each vendor and protocols with each implementation has
> unique matching mechanism so that only one match occurs per protocol on
> any platform.

Ack.

Also as mentioned in another thread this will be the only implementation
of the 0x80 vendor protocol upstream given that no other SoC actually
shipped with it enabled (expect for sc7180 which already has an
alternative dvfs solution upstream).

-Sibi

>
  

On Wed, Jan 17, 2024 at 11:04:54PM +0530, 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.
> 

Hi Sibi,

a few comments down below.

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

I am starting to think to put vendor protocols in their own dedicated
subdir given that a bunch of those appeared recently :D

...have to discuss with Sudeep...anyway not really an issue...

any thoughts about this ?

>  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;

If you get to call this protocol operation, the protocol itself has to
have been initialized already and registered with the SCMI core, and get
assigned a protocol_handle *ph, so ph and ph->xops are definitely non-NULL
here....if they are please report as a bug :P

> +
> +	ret = ph->xops->xfer_get_init(ph, SET_PARAM, size + PROTOCOL_PAYLOAD_SIZE,
> +				      SCMI_MAX_TX_RX_SIZE, &t);

This parameter, which you set to SCMI_MAX_TX_RX_SIZE, is meant to carry the
max RX payload size for the specific message you are sending, if you known it;
if you do NOT known it you can set this to ZERO and the SCMI core will bump it
to the maximum message size for the currently configured underlying transport
AND check if the reply fits in.

Here it seems that you are trying to somehow set the max RX to the max size you
know the transport can support (which is indeed 128bytes for nmailbox/shmem), but
you dont need to (as explained), it is something that does NOT belong to
the protocol layer in fact (if you meant to use the transport layer max size),
AND you wont be able in any case to override the underlying maximum RX payload
size, since that is the size of the pre-allocated message buffers in the SCMI
xore, and it is enforced by xfer_get_init().

So, in case somehow the underlying transport was or will be configured to be
shorter than you requested here, you will fail the xfer_get_init() in
teh future.

> +	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);

..just shift as Konrad (I think) was saying...of use FIELD_GET() (probably overkill)

.moreover... if the message PROTOCOL_PAYLOAD that you use is always the
same (surely the same in size) you should just define some sort of:
(just making up names here)

	struct qc_msg {
		__le32 ext_id;
		__le32 algo_low;
		__le32 algo_high;
		__le32 param_id;
		__le32 buf[];
	}

.so that you can easily write the above as:

	msg->ext_id = cpu_to_le32(EXTENDED_MSG_ID);
	...

which is more readable and MOST importantly can be checked by static
analyzer like smatch for consistent usage of endianess macros...(that we
all love...:P)
	
> +	*msg++ = cpu_to_le32(param_id);
> +	memcpy(msg, buf, size);

..mmm...this is correct at the end since you allocate a TX len buffer
of (size + PROTOCOL_PAYLOAD_SIZE) and just move the dst_buf @msg by just
PROTOCOL_PAYLOAD_SIZE before the memcpy, BUT the memcpy @size param
should represent the maximum amount of bytes that fits into the dst_buf,
and here it represent the src_buf size and it WORKS just fine since it
is indeed the amount of space left in msg, BUT ONLY because of how you
allocate the buffer above depending on the define PROTOCOL_PAYLOAD_SIZE...

..seems to me not so much future/error proof in these regards, what
happens if by mistake the msg fields and the define get of sync ?

.what about instead something like (applying also all of the remarks
above):

	struct qc_msg *msg;

	ret = ph->xops->xfer_get_init(ph, SET_PARAM, size + sizeof(*msg), 0 , &t);

	...

	msg = t->tx.buf;
	msg->ext_id = cpu_to_le32(EXTENDED_MSG_ID);
	...
	memcpy(msg->buf, buf, t->tx.len - sizeof(*msg));


..thoughts ?

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

Ditto. ph and ph->xops checks not needed

> +	ret = ph->xops->xfer_get_init(ph, GET_PARAM, tx_size + PROTOCOL_PAYLOAD_SIZE,
> +				      SCMI_MAX_TX_RX_SIZE, &t);

Shouldn't this be simply:
				     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);

Ditto. qc_msg + above remarks

> +	ret = ph->xops->do_xfer(ph, t);
> +	if (t->rx.len > rx_size) {

..if you use above rx_size for the desired payload max size, that will be also
used as the t->rx.len by the SCMI core and the configured transport layer to
enforce that the buffer RX payload size is not overflowed....
(see drivers/firmware/arm_scmi/shmem.c as an example)

..well you'll get you buffer silently truncated if it is too big than
expected...to be honest...

..but in any case you wont need this check...maybe here you can just anyway
warn if it is too small than expected (and was truncated)...if you want

> +		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;
Ditto.
> +
> +	ret = ph->xops->xfer_get_init(ph, START_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE,
> +				      SCMI_MAX_TX_RX_SIZE, &t);
Ditto.
> +	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);
Ditto.
> +	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;
Ditto.
> +
> +	ret = ph->xops->xfer_get_init(ph, STOP_ACTIVITY, size + PROTOCOL_PAYLOAD_SIZE,
> +				      SCMI_MAX_TX_RX_SIZE, &t);
Ditto.
> +	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);
Ditto.
> +	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);

As said already, I posted an RFC, which I am gonna cleanup and repost soon
(probably within the week) in order to allow for multiple custom protocols
from multipl distinct Vendors to co-exist within the same 0x80-0xFF
protocols numbers space....in a nutshell you will have to populate here one
or more fields to this struct at compile time so as to be able to identify
this protocol as yours...so that we can then compile all vendors protocols
into defconfig but then, at run-time, load only the ones matching the
effective platform you are running in.

I understand that you now have "your one and only protocol to rule them
all (0x80)" :P...  but this does not mean that other vendors cannot choose
that same number of yours for their own protocols (..I think it is already
happening), so we need a compile/runtime mechanism to properly select...


> +
> +MODULE_DESCRIPTION("QTI SCMI vendor protocol");

As already said, it seems a bit strange to have just one protocol where
you channel all the current and future stuff...this protocol seems related to
_MEMLAT configs at the moment only...

..consider that you can reserve/dedicate a channel to a protocol (if the
underlying transport allows) for performance purposes BUT clearly if you stick
all of your machinery into one single protocol you wont have this capability...

(... and I dont charge for new protocol numbers :P .... joking ah...)

> +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);

..what is this extern ? I maybe missing something...

> +
> +/**
> + * 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);
> +};
> +


Thanks,
Cristian
  

On Mon, Feb 12, 2024 at 05:39:19PM +0000, Cristian Marussi wrote:
> > 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
> >  
> 
> I am starting to think to put vendor protocols in their own dedicated
> subdir given that a bunch of those appeared recently :D
> 

Yes I tend to agree with different subdir for each vendor. Not sure if we
need new Kconfig entry or just use ARCH_<vendor/group of SoC> to build all
subdir used by that vendor.

> ....have to discuss with Sudeep...anyway not really an issue...
> 
> any thoughts about this ?

In general, I see lot of discussions on this thread when I was away for
past 3 weeks. I will wait for newer version as that seems simpler for me
than getting lost to follow the discussions so far.
  

On Wed, Jan 17, 2024 at 11:04:54PM +0530, 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.
> 

I would expect a proper description of the protocol specification
either as part of the header file qcom_scmi_vendor.h or somewhere
in the Documentation. It helps to understand the design instead of
assuming and/or getting confused with the assumption while reviewing.
I will point out at couple of individual place why I am asking for this.

You can follow some pattern to describe the command using SCMI spec as
reference. That will act as a contract for the software instead of changing
the implementation every time someone thinks it should work in certain
way. I have seen that quite a lot with the Qcom firmware lately with zero
transparency provided for these firmware by Qcom. In short I don't trust
just code to understand these vendor protocols. I need them to be documented
and version where needed so that we can refer back and make maintenance
smooth and easy.