Hello Akashi-san,

From: AKASHI Takahiro takahiro.akashi@linaro.org Sent: Friday, July 28, 2023 02:33 Subject: [RFC 1/3] firmware: scmi: add a check against availability of protocols

Now that we have Base protocol support, we will be able to check if a given protocol is really supported by the SCMI server (firmware).

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

Reviewed-by: Etienne Carriere etienne.carriere@foss.st.com

drivers/firmware/scmi/scmi_agent-uclass.c | 41 +++++++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c index 91cb172f3005..9494dca95bca 100644 --- a/drivers/firmware/scmi/scmi_agent-uclass.c +++ b/drivers/firmware/scmi/scmi_agent-uclass.c @@ -44,6 +44,38 @@ static const struct error_code scmi_linux_errmap[] = { */ struct udevice *scmi_agent;

+/**

• • scmi_protocol_is_supported - check availability of protocol
• • @dev: SCMI agent device
• • @proto_id: Identifier of protocol
• • check if the protocol, @proto_id, is provided by the SCMI agent,
• • @dev.
• • Return: 0 on success, error code otherwise
• */

+static bool scmi_protocol_is_supported(struct udevice *dev,

•                                  enum scmi_std_protocol proto_id)
  

+{

•   struct scmi_agent_priv *priv;
  

•   int i;
  

•   if (proto_id == SCMI_PROTOCOL_ID_BASE)
  

•           return true;
  

•   priv = dev_get_uclass_plat(dev);
  

•   if (!priv) {
  

•           dev_err(dev, "No priv data found\n");
  

•           return false;
  

•   }
  

•   for (i = 0; i < priv->num_protocols; i++)
  

•           if (priv->protocols[i] == proto_id)
  

•                   return true;
  

•   return false;
  

+}

struct udevice *scmi_get_protocol(struct udevice *dev, enum scmi_std_protocol id) { @@ -372,15 +404,18 @@ static int scmi_bind_protocols(struct udevice *dev) name = ofnode_get_name(node); switch (protocol_id) { case SCMI_PROTOCOL_ID_CLOCK:

•                   if (CONFIG_IS_ENABLED(CLK_SCMI))
  

•                   if (CONFIG_IS_ENABLED(CLK_SCMI) &&
  

•                       scmi_protocol_is_supported(dev, protocol_id))
                            drv = DM_DRIVER_GET(scmi_clock);
                    break;
            case SCMI_PROTOCOL_ID_RESET_DOMAIN:
  

•                   if (IS_ENABLED(CONFIG_RESET_SCMI))
  

•                   if (IS_ENABLED(CONFIG_RESET_SCMI) &&
  

•                       scmi_protocol_is_supported(dev, protocol_id))
                            drv = DM_DRIVER_GET(scmi_reset_domain);
                    break;
            case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
  

•                   if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)) {
  

•                   if (IS_ENABLED(CONFIG_DM_REGULATOR_SCMI) &&
  

•                       scmi_protocol_is_supported(dev, protocol_id)) {
                            node = ofnode_find_subnode(node, "regulators");
                            if (!ofnode_valid(node)) {
                                    dev_err(dev, "no regulators node\n");
  

-- 2.41.0

From: AKASHI Takahiro takahiro.akashi@linaro.org Sent: Friday, July 28, 2023 02:33 Subject: [RFC 2/3] firmware: scmi: add PROTOCOL_VERSION support for existing protocols on sandbox

In the next patch, SCMI_PROTOCOL_VERSION support is added on the existing SCMI protocols and the version check will be introduced. To finish "ut dm scmi_[clocks|resets|voltage_domains]" tests, sandbox SCMI agent should also implement/mimic this command.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

Aside the typos, inherited from previous patch series, the patch looks all good to me. Reviewed-by: Etienne Carriere etienne.carriere@foss.st.com preferrably with typos fixed.

---

drivers/firmware/scmi/sandbox-scmi_agent.c | 80 ++++++++++++++++++++++ 1 file changed, 80 insertions(+)

diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c b/drivers/firmware/scmi/sandbox-scmi_agent.c index ab8afb01de40..a99fcb0ad4a9 100644 --- a/drivers/firmware/scmi/sandbox-scmi_agent.c +++ b/drivers/firmware/scmi/sandbox-scmi_agent.c @@ -43,6 +43,10 @@ #define SANDBOX_SCMI_AGENT_NAME "OSPM" #define SANDBOX_SCMI_PLATFORM_NAME "platform"

+#define SANDBOX_SCMI_CLOCK_PROTOCOL_VERSION SCMI_CLOCK_PROTOCOL_VERSION +#define SANDBOX_SCMI_RD_PROTOCOL_VERSION SCMI_RESETDOM_PROTOCOL_VERSION +#define SANDBOX_SCMI_VOLTD_PROTOCOL_VERSION SCMI_VOLTDOM_PROTOCOL_VERSION

static u8 protocols[] = { SCMI_PROTOCOL_ID_CLOCK, SCMI_PROTOCOL_ID_RESET_DOMAIN, @@ -440,6 +444,28 @@ static int sandbox_scmi_base_reset_agent_configuration(struct udevice *dev,

/* Clock Protocol */

+/**

• • sandbox_scmi_clock_protocol_version - implement SCMI_PROTOCOL_VERSION
• • @udevice: SCMI device

Typos: s/udevice/udev/g here and for the 2 other inline description comments below.

• • @msg: SCMI message
• • Implement SCMI_PROTOCOL_VERSION command.
• */

+static int sandbox_scmi_clock_protocol_version(struct udevice *dev,

•                                          struct scmi_msg *msg)
  

+{

•   struct scmi_protocol_version_out *out = NULL;
  

•   if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
  

•           return -EINVAL;
  

•   out = (struct scmi_protocol_version_out *)msg->out_msg;
  

•   out->version = SANDBOX_SCMI_CLOCK_PROTOCOL_VERSION;
  

•   out->status = SCMI_SUCCESS;
  

•   return 0;
  

+}

static int sandbox_scmi_clock_protocol_attribs(struct udevice *dev, struct scmi_msg *msg) { @@ -577,6 +603,30 @@ static int sandbox_scmi_clock_gate(struct udevice *dev, struct scmi_msg *msg) return 0; }

+/* Reset Domain Protocol */

+/**

• • sandbox_scmi_rd_protocol_version - implement SCMI_PROTOCOL_VERSION
• • @udevice: SCMI device
• • @msg: SCMI message
• • Implement SCMI_PROTOCOL_VERSION command.
• */

+static int sandbox_scmi_rd_protocol_version(struct udevice *dev,

•                                       struct scmi_msg *msg)
  

+{

•   struct scmi_protocol_version_out *out = NULL;
  

•   if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
  

•           return -EINVAL;
  

•   out = (struct scmi_protocol_version_out *)msg->out_msg;
  

•   out->version = SANDBOX_SCMI_RD_PROTOCOL_VERSION;
  

•   out->status = SCMI_SUCCESS;
  

•   return 0;
  

+}

static int sandbox_scmi_rd_attribs(struct udevice *dev, struct scmi_msg *msg) { struct scmi_rd_attr_in *in = NULL; @@ -647,6 +697,30 @@ static int sandbox_scmi_rd_reset(struct udevice *dev, struct scmi_msg *msg) return 0; }

+/* Voltage Domain Protocol */

+/**

• • sandbox_scmi_voltd_protocol_version - implement SCMI_PROTOCOL_VERSION
• • @udevice: SCMI device
• • @msg: SCMI message
• • Implement SCMI_PROTOCOL_VERSION command.
• */

+static int sandbox_scmi_voltd_protocol_version(struct udevice *dev,

•                                          struct scmi_msg *msg)
  

+{

•   struct scmi_protocol_version_out *out = NULL;
  

•   if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
  

•           return -EINVAL;
  

•   out = (struct scmi_protocol_version_out *)msg->out_msg;
  

•   out->version = SANDBOX_SCMI_VOLTD_PROTOCOL_VERSION;
  

•   out->status = SCMI_SUCCESS;
  

•   return 0;
  

+}

static int sandbox_scmi_voltd_attribs(struct udevice *dev, struct scmi_msg *msg) { struct scmi_voltd_attr_in *in = NULL; @@ -833,6 +907,8 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev, break; case SCMI_PROTOCOL_ID_CLOCK: switch (msg->message_id) {

•           case SCMI_PROTOCOL_VERSION:
  

•                   return sandbox_scmi_clock_protocol_version(dev, msg);
            case SCMI_PROTOCOL_ATTRIBUTES:
                    return sandbox_scmi_clock_protocol_attribs(dev, msg);
            case SCMI_CLOCK_ATTRIBUTES:
  

@@ -849,6 +925,8 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev, break; case SCMI_PROTOCOL_ID_RESET_DOMAIN: switch (msg->message_id) {

•           case SCMI_PROTOCOL_VERSION:
  

•                   return sandbox_scmi_rd_protocol_version(dev, msg);
            case SCMI_RESET_DOMAIN_ATTRIBUTES:
                    return sandbox_scmi_rd_attribs(dev, msg);
            case SCMI_RESET_DOMAIN_RESET:
  

@@ -859,6 +937,8 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev, break; case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN: switch (msg->message_id) {

•           case SCMI_PROTOCOL_VERSION:
  

•                   return sandbox_scmi_voltd_protocol_version(dev, msg);
            case SCMI_VOLTAGE_DOMAIN_ATTRIBUTES:
                    return sandbox_scmi_voltd_attribs(dev, msg);
            case SCMI_VOLTAGE_DOMAIN_CONFIG_SET:
  

-- 2.41.0

From: AKASHI Takahiro takahiro.akashi@linaro.org Sent: Friday, July 28, 2023 02:33 Subject: [RFC 3/3] firmware: scmi: add a sanity check against protocol version

SCMI_PROTOCOL_VERSION is a mandatory command for all the SCMI protocols. With this patch, this command is implemented on each protocol. Then, we can assure that the feature set implemented by SCMI Server (firmware) is compatible with U-Boot, that is, each protocol's version must be equal to or higher than the one that U-Boot's corresponding driver expects.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

This change enforces some protocol versions that may not be reported by known open source scmi server implementations: scp-firmware, tf-a and op-tee.

Latest scp-firwmare/op-tee/tf-a report the following verisons numbers: * base protocol : 0x20000 all * clock protocol: 0x10000 for scp-firwmare / 0x20000 for op-tee and tf-a. * reset-dom: 0x1000 for all * voltage-dom: 0x10000 for scp-firmware / 0x30000 for op-tee / not used in tf-a. * power-dom: 0x20000 for scp-firmware / 0x21000 for tf-a / not used in op-tee.

Regarding the SCMI specification, these protocol versions evolve with the specification version: ---------- ---base -powerD --clock -resetD --voltD SCMI v1.0: 0x10000 0x10000 0x10000 ------- ------- SCMI v2.0: 0x20000 0x20000 0x10000 0x10000 ------- SCMI v3.0: 0x20000 0x21000 0x10000 0x20000 0x10000 SCMI v3.1: 0x20000 0x30000 0x30000 0x30000 0x20000 SCMI v3.2: 0x20000 0x30000 0x30000 0x30000 0x20000

This patch enforces the versions defined in SCMI v3.2. Being strict in protocol version is not ideal I think.

BR, etienne

drivers/clk/clk_scmi.c | 6 ++++++ drivers/power/regulator/scmi_regulator.c | 6 ++++++ drivers/reset/reset-scmi.c | 14 +++++++++++++- include/scmi_protocols.h | 6 ++++++ 4 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk_scmi.c b/drivers/clk/clk_scmi.c index 34a49363a51a..3591acb6d3a9 100644 --- a/drivers/clk/clk_scmi.c +++ b/drivers/clk/clk_scmi.c @@ -138,12 +138,18 @@ static int scmi_clk_probe(struct udevice *dev) { struct clk *clk; size_t num_clocks, i;

•   u32 version;
    int ret;
  
    ret = devm_scmi_of_get_channel(dev);
    if (ret)
            return ret;
  
  

•   ret = scmi_generic_protocol_version(dev, SCMI_PROTOCOL_ID_CLOCK,
  

•                                       &version);
  

•   if (ret || version < SCMI_CLOCK_PROTOCOL_VERSION)
  

•           return -EINVAL;
  

•   if (!CONFIG_IS_ENABLED(CLK_CCF))
            return 0;
  
  

diff --git a/drivers/power/regulator/scmi_regulator.c b/drivers/power/regulator/scmi_regulator.c index 08918b20872c..936768d0eeeb 100644 --- a/drivers/power/regulator/scmi_regulator.c +++ b/drivers/power/regulator/scmi_regulator.c @@ -138,12 +138,18 @@ static int scmi_regulator_probe(struct udevice *dev) .out_msg = (u8 *)&out, .out_msg_sz = sizeof(out), };

•   u32 version;
    int ret;
  
    ret = devm_scmi_of_get_channel(dev);
    if (ret)
            return ret;
  
  

•   ret = scmi_generic_protocol_version(dev, SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN,
  

•                                       &version);
  

•   if (ret || version < SCMI_VOLTDOM_PROTOCOL_VERSION)
  

•           return -EINVAL;
  

•   /* Check voltage domain is known from SCMI server */
    in.domain_id = pdata->domain_id;
  
  

diff --git a/drivers/reset/reset-scmi.c b/drivers/reset/reset-scmi.c index b76711f0a8fb..dbd98dbdbc4f 100644 --- a/drivers/reset/reset-scmi.c +++ b/drivers/reset/reset-scmi.c @@ -73,7 +73,19 @@ static const struct reset_ops scmi_reset_domain_ops = {

static int scmi_reset_probe(struct udevice *dev) {

•   return devm_scmi_of_get_channel(dev);
  

•   u32 version;
  

•   int ret;
  

•   ret = devm_scmi_of_get_channel(dev);
  

•   if (ret)
  

•           return ret;
  

•   ret = scmi_generic_protocol_version(dev, SCMI_PROTOCOL_ID_RESET_DOMAIN,
  

•                                       &version);
  

•   if (ret || version < SCMI_RESETDOM_PROTOCOL_VERSION)
  

•           return -EINVAL;
  

•   return 0;
  

}

U_BOOT_DRIVER(scmi_reset_domain) = { diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h index 64fd740472b5..6ab16efd49cc 100644 --- a/include/scmi_protocols.h +++ b/include/scmi_protocols.h @@ -398,6 +398,8 @@ int scmi_generic_protocol_version(struct udevice *dev,

• SCMI Clock Protocol

*/

+#define SCMI_CLOCK_PROTOCOL_VERSION 0x20000

TF-A and OP-TEE scmi servers report version 0x20000 for clock protocol but SCP-firmware reports version 0x10000: https://github.com/ARM-software/SCP-firmware/blob/master/module/scmi_clock/i...

enum scmi_clock_message_id { SCMI_CLOCK_ATTRIBUTES = 0x3, SCMI_CLOCK_RATE_SET = 0x5, @@ -509,6 +511,8 @@ struct scmi_clk_rate_set_out {

• SCMI Reset Domain Protocol

*/

+#define SCMI_RESETDOM_PROTOCOL_VERSION 0x30000

enum scmi_reset_domain_message_id { SCMI_RESET_DOMAIN_ATTRIBUTES = 0x3, SCMI_RESET_DOMAIN_RESET = 0x4, @@ -569,6 +573,8 @@ struct scmi_rd_reset_out {

• SCMI Voltage Domain Protocol

*/

+#define SCMI_VOLTDOM_PROTOCOL_VERSION 0x20000

enum scmi_voltage_domain_message_id { SCMI_VOLTAGE_DOMAIN_ATTRIBUTES = 0x3, SCMI_VOLTAGE_DOMAIN_CONFIG_SET = 0x5, -- 2.41.0