[PATCH v2 0/2] SCMI multiple vendor protocol support

[PATCH v2 0/2] SCMI multiple vendor protocol support

[Cristian Marussi]

Hi all,

this is meant to address the possibility of having multiple vendors
implementing distinct SCMI vendor protocols with possibly the same
overlapping protocol number without the need of crafting the Kconfig
at compile time to include only wanted protos

Basic idea is simple:

- vendor protos HAS to be 'tagged' at build time with a vendor_id
- vendor protos COULD also optionally be tagged at build time with
sub_vendor_id and implementation version

- at init all the build vendor protos are registered with the SCMI core
  using a key derived from the above tags

- at SCMI probe time the platform is identified via Base protocol as
  usual and all the required vendor protos (i.e. IDs defined in the DT
  as usual) are loaded (if required) after a lookup process based on the
  following rules:

  + protocol DB is searched using the platform/Base runtime provided tags

  	<vendor> / <sub_vendor> / <impl_ver>

    using the following search logic (keys), first match:

     1. proto_id / <vendor_id> / <sub_vendor_id> / <impl_ver>

     2. proto_id / <vendor_id> / <sub_vendor_id> / 0

     3. proto_id / <vendor_id> / NULL / 0

  IOW, closest match, depending on how much fine grained is your
  protocol tuned (tagged) for the platform.

  I am still doubtful about the usage of <impl_ver>, and tempted to drop it
  since we have anyway protocol version and NEGOTIATE_PROTOCOL_VERSION
  to deal with slight difference in fw revision...

In V2 a dedicated vendors dir/Makefile/Kconfig is added: not sure if we
want to provide also a way of grouping SCMI vendor protocol headers under
include/linux...open to any suggestion/opinion.

Based on sudeep/for-next/scmi/updates on top of

   commit dccc943c8953 ("clk: scmi: Support get/set duty_cycle operations")

Any feedback welcome

Thanks,
Cristian

---
v1 -> v2
- added a dedicated vendors/ directory and Kconfig under arm_scmi/

Cristian Marussi (2):
  firmware: arm_scmi: Add support for multiple vendors custom protocols
  firmware: arm_scmi: Add dedicated vendor protocols submenu

 drivers/firmware/arm_scmi/Kconfig          |   2 +
 drivers/firmware/arm_scmi/Makefile         |   1 +
 drivers/firmware/arm_scmi/driver.c         | 165 ++++++++++++++++++---
 drivers/firmware/arm_scmi/protocols.h      |  15 ++
 drivers/firmware/arm_scmi/vendors/Kconfig  |   4 +
 drivers/firmware/arm_scmi/vendors/Makefile |   2 +
 6 files changed, 167 insertions(+), 22 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/vendors/Kconfig
 create mode 100644 drivers/firmware/arm_scmi/vendors/Makefile

-- 
2.44.0

[PATCH v2 0/2] SCMI multiple vendor protocol support

[Cristian Marussi]

Hi all,

this is meant to address the possibility of having multiple vendors
implementing distinct SCMI vendor protocols with possibly the same
overlapping protocol number without the need of crafting the Kconfig
at compile time to include only wanted protos

Basic idea is simple:

- vendor protos HAS to be 'tagged' at build time with a vendor_id
- vendor protos COULD also optionally be tagged at build time with
sub_vendor_id and implementation version

- at init all the build vendor protos are registered with the SCMI core
  using a key derived from the above tags

- at SCMI probe time the platform is identified via Base protocol as
  usual and all the required vendor protos (i.e. IDs defined in the DT
  as usual) are loaded (if required) after a lookup process based on the
  following rules:

  + protocol DB is searched using the platform/Base runtime provided tags

  	<vendor> / <sub_vendor> / <impl_ver>

    using the following search logic (keys), first match:

     1. proto_id / <vendor_id> / <sub_vendor_id> / <impl_ver>

     2. proto_id / <vendor_id> / <sub_vendor_id> / 0

     3. proto_id / <vendor_id> / NULL / 0

  IOW, closest match, depending on how much fine grained is your
  protocol tuned (tagged) for the platform.

  I am still doubtful about the usage of <impl_ver>, and tempted to drop it
  since we have anyway protocol version and NEGOTIATE_PROTOCOL_VERSION
  to deal with slight difference in fw revision...

In V2 a dedicated vendors dir/Makefile/Kconfig is added: not sure if we
want to provide also a way of grouping SCMI vendor protocol headers under
include/linux...open to any suggestion/opinion.

Based on sudeep/for-next/scmi/updates on top of

   commit dccc943c8953 ("clk: scmi: Support get/set duty_cycle operations")

Any feedback welcome

Thanks,
Cristian

---
v1 -> v2
- added a dedicated vendors/ directory and Kconfig under arm_scmi/

Cristian Marussi (2):
  firmware: arm_scmi: Add support for multiple vendors custom protocols
  firmware: arm_scmi: Add dedicated vendor protocols submenu

 drivers/firmware/arm_scmi/Kconfig          |   2 +
 drivers/firmware/arm_scmi/Makefile         |   1 +
 drivers/firmware/arm_scmi/driver.c         | 165 ++++++++++++++++++---
 drivers/firmware/arm_scmi/protocols.h      |  15 ++
 drivers/firmware/arm_scmi/vendors/Kconfig  |   4 +
 drivers/firmware/arm_scmi/vendors/Makefile |   2 +
 6 files changed, 167 insertions(+), 22 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/vendors/Kconfig
 create mode 100644 drivers/firmware/arm_scmi/vendors/Makefile

-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] firmware: arm_scmi: Add support for multiple vendors custom protocols

[Cristian Marussi]

Add a mechanism to be able to tag vendor protocol modules at compile-time
with a vendor/sub_vendor string and an implementation version and then to
choose to load, at run-time, only those vendor protocol modules matching
as close as possible the vendor/subvendor identification advertised by
the SCMI platform server.

In this way, any in-tree existent vendor protocol module can be build and
shipped by default in a single kernel image, even when using the same
clashing protocol identification numbers, since the SCMI core will take
care at run-time to load only the ones pertinent to the running system.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c    | 165 ++++++++++++++++++++++----
 drivers/firmware/arm_scmi/protocols.h |  15 +++
 2 files changed, 158 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index d0091459a276..aa18202054a5 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -33,6 +33,7 @@
 #include <linux/processor.h>
 #include <linux/refcount.h>
 #include <linux/slab.h>
+#include <linux/xarray.h>
 
 #include "common.h"
 #include "notify.h"
@@ -44,8 +45,7 @@
 
 static DEFINE_IDA(scmi_id);
 
-static DEFINE_IDR(scmi_protocols);
-static DEFINE_SPINLOCK(protocol_lock);
+static DEFINE_XARRAY(scmi_protocols);
 
 /* List of all SCMI devices active in system */
 static LIST_HEAD(scmi_list);
@@ -194,11 +194,90 @@ struct scmi_info {
 #define bus_nb_to_scmi_info(nb)	container_of(nb, struct scmi_info, bus_nb)
 #define req_nb_to_scmi_info(nb)	container_of(nb, struct scmi_info, dev_req_nb)
 
-static const struct scmi_protocol *scmi_protocol_get(int protocol_id)
+static unsigned long
+scmi_vendor_protocol_signature(unsigned int protocol_id, char *vendor_id,
+			       char *sub_vendor_id, u32 impl_ver)
 {
-	const struct scmi_protocol *proto;
+	char *signature, *p;
+	unsigned long hash = 0;
 
-	proto = idr_find(&scmi_protocols, protocol_id);
+	/* vendor_id/sub_vendor_id guaranteed <= SCMI_SHORT_NAME_MAX_SIZE */
+	signature = kasprintf(GFP_KERNEL, "%02X|%s|%s|0x%08X", protocol_id,
+			      vendor_id ?: "", sub_vendor_id ?: "", impl_ver);
+	if (!signature)
+		return 0;
+
+	p = signature;
+	while (*p)
+		hash = partial_name_hash(tolower(*p++), hash);
+	hash = end_name_hash(hash);
+
+	kfree(signature);
+
+	return hash;
+}
+
+static unsigned long
+scmi_protocol_key_calculate(int protocol_id, char *vendor_id,
+			    char *sub_vendor_id, u32 impl_ver)
+{
+	if (protocol_id < SCMI_PROTOCOL_VENDOR)
+		return protocol_id;
+	else
+		return scmi_vendor_protocol_signature(protocol_id, vendor_id,
+						      sub_vendor_id, impl_ver);
+}
+
+static const struct scmi_protocol *
+scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
+			    char *sub_vendor_id, u32 impl_ver)
+{
+	unsigned long key;
+	struct scmi_protocol *proto = NULL;
+
+	/* Searching for closest match ...*/
+	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
+					  sub_vendor_id, impl_ver);
+	if (key)
+		proto = xa_load(&scmi_protocols, key);
+
+	if (proto)
+		return proto;
+
+	/* Any match on vendor/sub_vendor ? */
+	if (impl_ver) {
+		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
+						  sub_vendor_id, 0);
+		if (key)
+			proto = xa_load(&scmi_protocols, key);
+
+		if (proto)
+			return proto;
+	}
+
+	/* Any match on just the vendor ? */
+	if (sub_vendor_id) {
+		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
+						  NULL, 0);
+		if (key)
+			proto = xa_load(&scmi_protocols, key);
+	}
+
+	return proto;
+}
+
+static const struct scmi_protocol *
+scmi_protocol_get(int protocol_id, struct scmi_revision_info *version)
+{
+	const struct scmi_protocol *proto = NULL;
+
+	if (protocol_id < SCMI_PROTOCOL_VENDOR)
+		proto = xa_load(&scmi_protocols, protocol_id);
+	else
+		proto = scmi_vendor_protocol_lookup(protocol_id,
+						    version->vendor_id,
+						    version->sub_vendor_id,
+						    version->impl_ver);
 	if (!proto || !try_module_get(proto->owner)) {
 		pr_warn("SCMI Protocol 0x%x not found!\n", protocol_id);
 		return NULL;
@@ -206,21 +285,46 @@ static const struct scmi_protocol *scmi_protocol_get(int protocol_id)
 
 	pr_debug("Found SCMI Protocol 0x%x\n", protocol_id);
 
+	if (protocol_id >= SCMI_PROTOCOL_VENDOR)
+		pr_info("Loaded SCMI Vendor Protocol 0x%x - %s %s %X\n",
+			protocol_id, proto->vendor_id ?: "",
+			proto->sub_vendor_id ?: "", proto->impl_ver);
+
 	return proto;
 }
 
-static void scmi_protocol_put(int protocol_id)
+static void scmi_protocol_put(const struct scmi_protocol *proto)
 {
-	const struct scmi_protocol *proto;
-
-	proto = idr_find(&scmi_protocols, protocol_id);
 	if (proto)
 		module_put(proto->owner);
 }
 
+static int scmi_vendor_protocol_check(const struct scmi_protocol *proto)
+{
+	if (!proto->vendor_id) {
+		pr_err("missing vendor_id for protocol 0x%x\n", proto->id);
+		return -EINVAL;
+	}
+
+	if (strlen(proto->vendor_id) >= SCMI_SHORT_NAME_MAX_SIZE) {
+		pr_err("malformed vendor_id for protocol 0x%x\n", proto->id);
+		return -EINVAL;
+	}
+
+	if (proto->sub_vendor_id &&
+	    strlen(proto->sub_vendor_id) >= SCMI_SHORT_NAME_MAX_SIZE) {
+		pr_err("malformed sub_vendor_id for protocol 0x%x\n",
+		       proto->id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int scmi_protocol_register(const struct scmi_protocol *proto)
 {
 	int ret;
+	unsigned long key;
 
 	if (!proto) {
 		pr_err("invalid protocol\n");
@@ -232,12 +336,23 @@ int scmi_protocol_register(const struct scmi_protocol *proto)
 		return -EINVAL;
 	}
 
-	spin_lock(&protocol_lock);
-	ret = idr_alloc(&scmi_protocols, (void *)proto,
-			proto->id, proto->id + 1, GFP_ATOMIC);
-	spin_unlock(&protocol_lock);
-	if (ret != proto->id) {
-		pr_err("unable to allocate SCMI idr slot for 0x%x - err %d\n",
+	if (proto->id >= SCMI_PROTOCOL_VENDOR &&
+	    scmi_vendor_protocol_check(proto))
+		return -EINVAL;
+
+	/*
+	 * Calculate a protocol key to register this protocol with the core;
+	 * key value 0 is considered invalid.
+	 */
+	key = scmi_protocol_key_calculate(proto->id, proto->vendor_id,
+					  proto->sub_vendor_id,
+					  proto->impl_ver);
+	if (!key)
+		return -EINVAL;
+
+	ret = xa_insert(&scmi_protocols, key, (void *)proto, GFP_KERNEL);
+	if (ret) {
+		pr_err("unable to allocate SCMI protocol slot for 0x%x - err %d\n",
 		       proto->id, ret);
 		return ret;
 	}
@@ -250,9 +365,15 @@ EXPORT_SYMBOL_GPL(scmi_protocol_register);
 
 void scmi_protocol_unregister(const struct scmi_protocol *proto)
 {
-	spin_lock(&protocol_lock);
-	idr_remove(&scmi_protocols, proto->id);
-	spin_unlock(&protocol_lock);
+	unsigned long key;
+
+	key = scmi_protocol_key_calculate(proto->id, proto->vendor_id,
+					  proto->sub_vendor_id,
+					  proto->impl_ver);
+	if (!key)
+		return;
+
+	xa_erase(&scmi_protocols, key);
 
 	pr_debug("Unregistered SCMI Protocol 0x%x\n", proto->id);
 }
@@ -1940,7 +2061,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
 	/* Protocol specific devres group */
 	gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);
 	if (!gid) {
-		scmi_protocol_put(proto->id);
+		scmi_protocol_put(proto);
 		goto out;
 	}
 
@@ -2004,7 +2125,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
 
 clean:
 	/* Take care to put the protocol module's owner before releasing all */
-	scmi_protocol_put(proto->id);
+	scmi_protocol_put(proto);
 	devres_release_group(handle->dev, gid);
 out:
 	return ERR_PTR(ret);
@@ -2038,7 +2159,7 @@ scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id)
 		const struct scmi_protocol *proto;
 
 		/* Fails if protocol not registered on bus */
-		proto = scmi_protocol_get(protocol_id);
+		proto = scmi_protocol_get(protocol_id, &info->version);
 		if (proto)
 			pi = scmi_alloc_init_protocol_instance(info, proto);
 		else
@@ -2093,7 +2214,7 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
 
 		idr_remove(&info->protocols, protocol_id);
 
-		scmi_protocol_put(protocol_id);
+		scmi_protocol_put(pi->proto);
 
 		devres_release_group(handle->dev, gid);
 		dev_dbg(handle->dev, "De-Initialized protocol: 0x%X\n",
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 317d3fb32676..45ee8b35f56d 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -29,6 +29,8 @@
 #define PROTOCOL_REV_MAJOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x))))
 #define PROTOCOL_REV_MINOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x))))
 
+#define SCMI_PROTOCOL_VENDOR	0x80
+
 enum scmi_common_cmd {
 	PROTOCOL_VERSION = 0x0,
 	PROTOCOL_ATTRIBUTES = 0x1,
@@ -323,6 +325,16 @@ typedef int (*scmi_prot_init_ph_fn_t)(const struct scmi_protocol_handle *);
  *		       protocol by the agent. Each protocol implementation
  *		       in the agent is supposed to downgrade to match the
  *		       protocol version supported by the platform.
+ * @vendor_id: A firmware vendor string for vendor protocols matching.
+ *	       Ignored when @id identifies a standard protocol, cannot be NULL
+ *	       otherwise.
+ * @sub_vendor_id: A firmware sub_vendor string for vendor protocols matching.
+ *		   Ignored if NULL or when @id identifies a standard protocol.
+ * @impl_ver: A firmware implementation version for vendor protocols matching.
+ *	      Ignored if zero or if @id identifies a standard protocol.
+ *
+ * Note that vendor protocols matching at load time is performed by attempting
+ * the closest match first against the tuple (vendor, sub_vendor, impl_ver)
  */
 struct scmi_protocol {
 	const u8				id;
@@ -332,6 +344,9 @@ struct scmi_protocol {
 	const void				*ops;
 	const struct scmi_protocol_events	*events;
 	unsigned int				supported_version;
+	char					*vendor_id;
+	char					*sub_vendor_id;
+	u32					impl_ver;
 };
 
 #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(name, proto)	\
-- 
2.44.0

[PATCH v2 1/2] firmware: arm_scmi: Add support for multiple vendors custom protocols

[Cristian Marussi]

Add a mechanism to be able to tag vendor protocol modules at compile-time
with a vendor/sub_vendor string and an implementation version and then to
choose to load, at run-time, only those vendor protocol modules matching
as close as possible the vendor/subvendor identification advertised by
the SCMI platform server.

In this way, any in-tree existent vendor protocol module can be build and
shipped by default in a single kernel image, even when using the same
clashing protocol identification numbers, since the SCMI core will take
care at run-time to load only the ones pertinent to the running system.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c    | 165 ++++++++++++++++++++++----
 drivers/firmware/arm_scmi/protocols.h |  15 +++
 2 files changed, 158 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index d0091459a276..aa18202054a5 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -33,6 +33,7 @@
 #include <linux/processor.h>
 #include <linux/refcount.h>
 #include <linux/slab.h>
+#include <linux/xarray.h>
 
 #include "common.h"
 #include "notify.h"
@@ -44,8 +45,7 @@
 
 static DEFINE_IDA(scmi_id);
 
-static DEFINE_IDR(scmi_protocols);
-static DEFINE_SPINLOCK(protocol_lock);
+static DEFINE_XARRAY(scmi_protocols);
 
 /* List of all SCMI devices active in system */
 static LIST_HEAD(scmi_list);
@@ -194,11 +194,90 @@ struct scmi_info {
 #define bus_nb_to_scmi_info(nb)	container_of(nb, struct scmi_info, bus_nb)
 #define req_nb_to_scmi_info(nb)	container_of(nb, struct scmi_info, dev_req_nb)
 
-static const struct scmi_protocol *scmi_protocol_get(int protocol_id)
+static unsigned long
+scmi_vendor_protocol_signature(unsigned int protocol_id, char *vendor_id,
+			       char *sub_vendor_id, u32 impl_ver)
 {
-	const struct scmi_protocol *proto;
+	char *signature, *p;
+	unsigned long hash = 0;
 
-	proto = idr_find(&scmi_protocols, protocol_id);
+	/* vendor_id/sub_vendor_id guaranteed <= SCMI_SHORT_NAME_MAX_SIZE */
+	signature = kasprintf(GFP_KERNEL, "%02X|%s|%s|0x%08X", protocol_id,
+			      vendor_id ?: "", sub_vendor_id ?: "", impl_ver);
+	if (!signature)
+		return 0;
+
+	p = signature;
+	while (*p)
+		hash = partial_name_hash(tolower(*p++), hash);
+	hash = end_name_hash(hash);
+
+	kfree(signature);
+
+	return hash;
+}
+
+static unsigned long
+scmi_protocol_key_calculate(int protocol_id, char *vendor_id,
+			    char *sub_vendor_id, u32 impl_ver)
+{
+	if (protocol_id < SCMI_PROTOCOL_VENDOR)
+		return protocol_id;
+	else
+		return scmi_vendor_protocol_signature(protocol_id, vendor_id,
+						      sub_vendor_id, impl_ver);
+}
+
+static const struct scmi_protocol *
+scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
+			    char *sub_vendor_id, u32 impl_ver)
+{
+	unsigned long key;
+	struct scmi_protocol *proto = NULL;
+
+	/* Searching for closest match ...*/
+	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
+					  sub_vendor_id, impl_ver);
+	if (key)
+		proto = xa_load(&scmi_protocols, key);
+
+	if (proto)
+		return proto;
+
+	/* Any match on vendor/sub_vendor ? */
+	if (impl_ver) {
+		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
+						  sub_vendor_id, 0);
+		if (key)
+			proto = xa_load(&scmi_protocols, key);
+
+		if (proto)
+			return proto;
+	}
+
+	/* Any match on just the vendor ? */
+	if (sub_vendor_id) {
+		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
+						  NULL, 0);
+		if (key)
+			proto = xa_load(&scmi_protocols, key);
+	}
+
+	return proto;
+}
+
+static const struct scmi_protocol *
+scmi_protocol_get(int protocol_id, struct scmi_revision_info *version)
+{
+	const struct scmi_protocol *proto = NULL;
+
+	if (protocol_id < SCMI_PROTOCOL_VENDOR)
+		proto = xa_load(&scmi_protocols, protocol_id);
+	else
+		proto = scmi_vendor_protocol_lookup(protocol_id,
+						    version->vendor_id,
+						    version->sub_vendor_id,
+						    version->impl_ver);
 	if (!proto || !try_module_get(proto->owner)) {
 		pr_warn("SCMI Protocol 0x%x not found!\n", protocol_id);
 		return NULL;
@@ -206,21 +285,46 @@ static const struct scmi_protocol *scmi_protocol_get(int protocol_id)
 
 	pr_debug("Found SCMI Protocol 0x%x\n", protocol_id);
 
+	if (protocol_id >= SCMI_PROTOCOL_VENDOR)
+		pr_info("Loaded SCMI Vendor Protocol 0x%x - %s %s %X\n",
+			protocol_id, proto->vendor_id ?: "",
+			proto->sub_vendor_id ?: "", proto->impl_ver);
+
 	return proto;
 }
 
-static void scmi_protocol_put(int protocol_id)
+static void scmi_protocol_put(const struct scmi_protocol *proto)
 {
-	const struct scmi_protocol *proto;
-
-	proto = idr_find(&scmi_protocols, protocol_id);
 	if (proto)
 		module_put(proto->owner);
 }
 
+static int scmi_vendor_protocol_check(const struct scmi_protocol *proto)
+{
+	if (!proto->vendor_id) {
+		pr_err("missing vendor_id for protocol 0x%x\n", proto->id);
+		return -EINVAL;
+	}
+
+	if (strlen(proto->vendor_id) >= SCMI_SHORT_NAME_MAX_SIZE) {
+		pr_err("malformed vendor_id for protocol 0x%x\n", proto->id);
+		return -EINVAL;
+	}
+
+	if (proto->sub_vendor_id &&
+	    strlen(proto->sub_vendor_id) >= SCMI_SHORT_NAME_MAX_SIZE) {
+		pr_err("malformed sub_vendor_id for protocol 0x%x\n",
+		       proto->id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 int scmi_protocol_register(const struct scmi_protocol *proto)
 {
 	int ret;
+	unsigned long key;
 
 	if (!proto) {
 		pr_err("invalid protocol\n");
@@ -232,12 +336,23 @@ int scmi_protocol_register(const struct scmi_protocol *proto)
 		return -EINVAL;
 	}
 
-	spin_lock(&protocol_lock);
-	ret = idr_alloc(&scmi_protocols, (void *)proto,
-			proto->id, proto->id + 1, GFP_ATOMIC);
-	spin_unlock(&protocol_lock);
-	if (ret != proto->id) {
-		pr_err("unable to allocate SCMI idr slot for 0x%x - err %d\n",
+	if (proto->id >= SCMI_PROTOCOL_VENDOR &&
+	    scmi_vendor_protocol_check(proto))
+		return -EINVAL;
+
+	/*
+	 * Calculate a protocol key to register this protocol with the core;
+	 * key value 0 is considered invalid.
+	 */
+	key = scmi_protocol_key_calculate(proto->id, proto->vendor_id,
+					  proto->sub_vendor_id,
+					  proto->impl_ver);
+	if (!key)
+		return -EINVAL;
+
+	ret = xa_insert(&scmi_protocols, key, (void *)proto, GFP_KERNEL);
+	if (ret) {
+		pr_err("unable to allocate SCMI protocol slot for 0x%x - err %d\n",
 		       proto->id, ret);
 		return ret;
 	}
@@ -250,9 +365,15 @@ EXPORT_SYMBOL_GPL(scmi_protocol_register);
 
 void scmi_protocol_unregister(const struct scmi_protocol *proto)
 {
-	spin_lock(&protocol_lock);
-	idr_remove(&scmi_protocols, proto->id);
-	spin_unlock(&protocol_lock);
+	unsigned long key;
+
+	key = scmi_protocol_key_calculate(proto->id, proto->vendor_id,
+					  proto->sub_vendor_id,
+					  proto->impl_ver);
+	if (!key)
+		return;
+
+	xa_erase(&scmi_protocols, key);
 
 	pr_debug("Unregistered SCMI Protocol 0x%x\n", proto->id);
 }
@@ -1940,7 +2061,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
 	/* Protocol specific devres group */
 	gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);
 	if (!gid) {
-		scmi_protocol_put(proto->id);
+		scmi_protocol_put(proto);
 		goto out;
 	}
 
@@ -2004,7 +2125,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
 
 clean:
 	/* Take care to put the protocol module's owner before releasing all */
-	scmi_protocol_put(proto->id);
+	scmi_protocol_put(proto);
 	devres_release_group(handle->dev, gid);
 out:
 	return ERR_PTR(ret);
@@ -2038,7 +2159,7 @@ scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id)
 		const struct scmi_protocol *proto;
 
 		/* Fails if protocol not registered on bus */
-		proto = scmi_protocol_get(protocol_id);
+		proto = scmi_protocol_get(protocol_id, &info->version);
 		if (proto)
 			pi = scmi_alloc_init_protocol_instance(info, proto);
 		else
@@ -2093,7 +2214,7 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
 
 		idr_remove(&info->protocols, protocol_id);
 
-		scmi_protocol_put(protocol_id);
+		scmi_protocol_put(pi->proto);
 
 		devres_release_group(handle->dev, gid);
 		dev_dbg(handle->dev, "De-Initialized protocol: 0x%X\n",
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 317d3fb32676..45ee8b35f56d 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -29,6 +29,8 @@
 #define PROTOCOL_REV_MAJOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x))))
 #define PROTOCOL_REV_MINOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x))))
 
+#define SCMI_PROTOCOL_VENDOR	0x80
+
 enum scmi_common_cmd {
 	PROTOCOL_VERSION = 0x0,
 	PROTOCOL_ATTRIBUTES = 0x1,
@@ -323,6 +325,16 @@ typedef int (*scmi_prot_init_ph_fn_t)(const struct scmi_protocol_handle *);
  *		       protocol by the agent. Each protocol implementation
  *		       in the agent is supposed to downgrade to match the
  *		       protocol version supported by the platform.
+ * @vendor_id: A firmware vendor string for vendor protocols matching.
+ *	       Ignored when @id identifies a standard protocol, cannot be NULL
+ *	       otherwise.
+ * @sub_vendor_id: A firmware sub_vendor string for vendor protocols matching.
+ *		   Ignored if NULL or when @id identifies a standard protocol.
+ * @impl_ver: A firmware implementation version for vendor protocols matching.
+ *	      Ignored if zero or if @id identifies a standard protocol.
+ *
+ * Note that vendor protocols matching at load time is performed by attempting
+ * the closest match first against the tuple (vendor, sub_vendor, impl_ver)
  */
 struct scmi_protocol {
 	const u8				id;
@@ -332,6 +344,9 @@ struct scmi_protocol {
 	const void				*ops;
 	const struct scmi_protocol_events	*events;
 	unsigned int				supported_version;
+	char					*vendor_id;
+	char					*sub_vendor_id;
+	u32					impl_ver;
 };
 
 #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(name, proto)	\
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] firmware: arm_scmi: Add dedicated vendor protocols submenu

[Cristian Marussi]

Add a dedicated Kconfig submenu and directory where to collect SCMI vendor
protocols implementations.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/Kconfig          | 2 ++
 drivers/firmware/arm_scmi/Makefile         | 1 +
 drivers/firmware/arm_scmi/vendors/Kconfig  | 4 ++++
 drivers/firmware/arm_scmi/vendors/Makefile | 2 ++
 4 files changed, 9 insertions(+)
 create mode 100644 drivers/firmware/arm_scmi/vendors/Kconfig
 create mode 100644 drivers/firmware/arm_scmi/vendors/Makefile

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index aa5842be19b2..db9166e99177 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -180,4 +180,6 @@ config ARM_SCMI_POWER_CONTROL
 	  called scmi_power_control. Note this may needed early in boot to catch
 	  early shutdown/reboot SCMI requests.
 
+source "drivers/firmware/arm_scmi/vendors/Kconfig"
+
 endmenu
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index a7bc4796519c..1316892a230d 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -15,6 +15,7 @@ scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
 
 obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
 obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
+obj-$(CONFIG_ARM_SCMI_PROTOCOL) += vendors/
 
 obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
 
diff --git a/drivers/firmware/arm_scmi/vendors/Kconfig b/drivers/firmware/arm_scmi/vendors/Kconfig
new file mode 100644
index 000000000000..7c1ca7a12603
--- /dev/null
+++ b/drivers/firmware/arm_scmi/vendors/Kconfig
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0-only
+menu "ARM SCMI Vendor Protocols"
+
+endmenu
diff --git a/drivers/firmware/arm_scmi/vendors/Makefile b/drivers/firmware/arm_scmi/vendors/Makefile
new file mode 100644
index 000000000000..c6c214158dd8
--- /dev/null
+++ b/drivers/firmware/arm_scmi/vendors/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+# obj-$(CONFIG_ARM_SCMI_PROTOCOL_<your_vendor_proto>) += <your_vendor_proto>.o
-- 
2.44.0

[PATCH v2 2/2] firmware: arm_scmi: Add dedicated vendor protocols submenu

[Cristian Marussi]

Add a dedicated Kconfig submenu and directory where to collect SCMI vendor
protocols implementations.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/Kconfig          | 2 ++
 drivers/firmware/arm_scmi/Makefile         | 1 +
 drivers/firmware/arm_scmi/vendors/Kconfig  | 4 ++++
 drivers/firmware/arm_scmi/vendors/Makefile | 2 ++
 4 files changed, 9 insertions(+)
 create mode 100644 drivers/firmware/arm_scmi/vendors/Kconfig
 create mode 100644 drivers/firmware/arm_scmi/vendors/Makefile

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index aa5842be19b2..db9166e99177 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -180,4 +180,6 @@ config ARM_SCMI_POWER_CONTROL
 	  called scmi_power_control. Note this may needed early in boot to catch
 	  early shutdown/reboot SCMI requests.
 
+source "drivers/firmware/arm_scmi/vendors/Kconfig"
+
 endmenu
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index a7bc4796519c..1316892a230d 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -15,6 +15,7 @@ scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
 
 obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
 obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
+obj-$(CONFIG_ARM_SCMI_PROTOCOL) += vendors/
 
 obj-$(CONFIG_ARM_SCMI_POWER_CONTROL) += scmi_power_control.o
 
diff --git a/drivers/firmware/arm_scmi/vendors/Kconfig b/drivers/firmware/arm_scmi/vendors/Kconfig
new file mode 100644
index 000000000000..7c1ca7a12603
--- /dev/null
+++ b/drivers/firmware/arm_scmi/vendors/Kconfig
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0-only
+menu "ARM SCMI Vendor Protocols"
+
+endmenu
diff --git a/drivers/firmware/arm_scmi/vendors/Makefile b/drivers/firmware/arm_scmi/vendors/Makefile
new file mode 100644
index 000000000000..c6c214158dd8
--- /dev/null
+++ b/drivers/firmware/arm_scmi/vendors/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+# obj-$(CONFIG_ARM_SCMI_PROTOCOL_<your_vendor_proto>) += <your_vendor_proto>.o
-- 
2.44.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] firmware: arm_scmi: Add dedicated vendor protocols submenu

[Sudeep Holla]

On Mon, Apr 08, 2024 at 10:30:52AM +0100, Cristian Marussi wrote:
> Add a dedicated Kconfig submenu and directory where to collect SCMI vendor
> protocols implementations.
>

This looks fine. But I would wait until the first vendor protocol is
ready to be merged to merge this as baseline. Hope that's OK.

--
Regards,
Sudeep

Re: [PATCH v2 2/2] firmware: arm_scmi: Add dedicated vendor protocols submenu

[Sudeep Holla]

On Mon, Apr 08, 2024 at 10:30:52AM +0100, Cristian Marussi wrote:
> Add a dedicated Kconfig submenu and directory where to collect SCMI vendor
> protocols implementations.
>

This looks fine. But I would wait until the first vendor protocol is
ready to be merged to merge this as baseline. Hope that's OK.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] firmware: arm_scmi: Add support for multiple vendors custom protocols

[Sudeep Holla]

On Mon, Apr 08, 2024 at 10:30:51AM +0100, Cristian Marussi wrote:
> Add a mechanism to be able to tag vendor protocol modules at compile-time
> with a vendor/sub_vendor string and an implementation version and then to
> choose to load, at run-time, only those vendor protocol modules matching
> as close as possible the vendor/subvendor identification advertised by
> the SCMI platform server.
> 
> In this way, any in-tree existent vendor protocol module can be build and
> shipped by default in a single kernel image, even when using the same
> clashing protocol identification numbers, since the SCMI core will take
> care at run-time to load only the ones pertinent to the running system.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/driver.c    | 165 ++++++++++++++++++++++----
>  drivers/firmware/arm_scmi/protocols.h |  15 +++
>  2 files changed, 158 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index d0091459a276..aa18202054a5 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c

[...]

> +static const struct scmi_protocol *
> +scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
> +			    char *sub_vendor_id, u32 impl_ver)
> +{
> +	unsigned long key;
> +	struct scmi_protocol *proto = NULL;
> +
> +	/* Searching for closest match ...*/
> +	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> +					  sub_vendor_id, impl_ver);
> +	if (key)
> +		proto = xa_load(&scmi_protocols, key);
> +
> +	if (proto)
> +		return proto;
> +
> +	/* Any match on vendor/sub_vendor ? */
> +	if (impl_ver) {
> +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> +						  sub_vendor_id, 0);
> +		if (key)
> +			proto = xa_load(&scmi_protocols, key);
> +
> +		if (proto)
> +			return proto;
> +	}
> +
> +	/* Any match on just the vendor ? */
> +	if (sub_vendor_id) {
> +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> +						  NULL, 0);
> +		if (key)
> +			proto = xa_load(&scmi_protocols, key);
> +	}
>

I see a pattern here, can be simplify/compress by something like below ?

static const struct scmi_protocol *
__scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
			      char *sub_vendor_id, u32 impl_ver)
{
	unsigned long key;
	struct scmi_protocol *proto = NULL;

	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
					  sub_vendor_id, impl_ver);
	if (key)
		proto = xa_load(&scmi_protocols, key);

	return proto;
}

[...]

> diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
> index 317d3fb32676..45ee8b35f56d 100644
> --- a/drivers/firmware/arm_scmi/protocols.h
> +++ b/drivers/firmware/arm_scmi/protocols.h
> @@ -29,6 +29,8 @@
>  #define PROTOCOL_REV_MAJOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x))))
>  #define PROTOCOL_REV_MINOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x))))
>  
> +#define SCMI_PROTOCOL_VENDOR	0x80
> +

s/SCMI_PROTOCOL_VENDOR/SCMI_PROTOCOL_VENDOR_BASE/

as I expect 0x80 to be used by most vendors.

-- 
Regards,
Sudeep

Re: [PATCH v2 1/2] firmware: arm_scmi: Add support for multiple vendors custom protocols

[Sudeep Holla]

On Mon, Apr 08, 2024 at 10:30:51AM +0100, Cristian Marussi wrote:
> Add a mechanism to be able to tag vendor protocol modules at compile-time
> with a vendor/sub_vendor string and an implementation version and then to
> choose to load, at run-time, only those vendor protocol modules matching
> as close as possible the vendor/subvendor identification advertised by
> the SCMI platform server.
> 
> In this way, any in-tree existent vendor protocol module can be build and
> shipped by default in a single kernel image, even when using the same
> clashing protocol identification numbers, since the SCMI core will take
> care at run-time to load only the ones pertinent to the running system.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/driver.c    | 165 ++++++++++++++++++++++----
>  drivers/firmware/arm_scmi/protocols.h |  15 +++
>  2 files changed, 158 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index d0091459a276..aa18202054a5 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c

[...]

> +static const struct scmi_protocol *
> +scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
> +			    char *sub_vendor_id, u32 impl_ver)
> +{
> +	unsigned long key;
> +	struct scmi_protocol *proto = NULL;
> +
> +	/* Searching for closest match ...*/
> +	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> +					  sub_vendor_id, impl_ver);
> +	if (key)
> +		proto = xa_load(&scmi_protocols, key);
> +
> +	if (proto)
> +		return proto;
> +
> +	/* Any match on vendor/sub_vendor ? */
> +	if (impl_ver) {
> +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> +						  sub_vendor_id, 0);
> +		if (key)
> +			proto = xa_load(&scmi_protocols, key);
> +
> +		if (proto)
> +			return proto;
> +	}
> +
> +	/* Any match on just the vendor ? */
> +	if (sub_vendor_id) {
> +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> +						  NULL, 0);
> +		if (key)
> +			proto = xa_load(&scmi_protocols, key);
> +	}
>

I see a pattern here, can be simplify/compress by something like below ?

static const struct scmi_protocol *
__scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
			      char *sub_vendor_id, u32 impl_ver)
{
	unsigned long key;
	struct scmi_protocol *proto = NULL;

	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
					  sub_vendor_id, impl_ver);
	if (key)
		proto = xa_load(&scmi_protocols, key);

	return proto;
}

[...]

> diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
> index 317d3fb32676..45ee8b35f56d 100644
> --- a/drivers/firmware/arm_scmi/protocols.h
> +++ b/drivers/firmware/arm_scmi/protocols.h
> @@ -29,6 +29,8 @@
>  #define PROTOCOL_REV_MAJOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x))))
>  #define PROTOCOL_REV_MINOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x))))
>  
> +#define SCMI_PROTOCOL_VENDOR	0x80
> +

s/SCMI_PROTOCOL_VENDOR/SCMI_PROTOCOL_VENDOR_BASE/

as I expect 0x80 to be used by most vendors.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] firmware: arm_scmi: Add support for multiple vendors custom protocols

[Cristian Marussi]

On Wed, Apr 17, 2024 at 01:32:08PM +0100, Sudeep Holla wrote:
> On Mon, Apr 08, 2024 at 10:30:51AM +0100, Cristian Marussi wrote:
> > Add a mechanism to be able to tag vendor protocol modules at compile-time
> > with a vendor/sub_vendor string and an implementation version and then to
> > choose to load, at run-time, only those vendor protocol modules matching
> > as close as possible the vendor/subvendor identification advertised by
> > the SCMI platform server.
> > 
> > In this way, any in-tree existent vendor protocol module can be build and
> > shipped by default in a single kernel image, even when using the same
> > clashing protocol identification numbers, since the SCMI core will take
> > care at run-time to load only the ones pertinent to the running system.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/driver.c    | 165 ++++++++++++++++++++++----
> >  drivers/firmware/arm_scmi/protocols.h |  15 +++
> >  2 files changed, 158 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index d0091459a276..aa18202054a5 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> 
> [...]
> 
> > +static const struct scmi_protocol *
> > +scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
> > +			    char *sub_vendor_id, u32 impl_ver)
> > +{
> > +	unsigned long key;
> > +	struct scmi_protocol *proto = NULL;
> > +
> > +	/* Searching for closest match ...*/
> > +	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > +					  sub_vendor_id, impl_ver);
> > +	if (key)
> > +		proto = xa_load(&scmi_protocols, key);
> > +
> > +	if (proto)
> > +		return proto;
> > +
> > +	/* Any match on vendor/sub_vendor ? */
> > +	if (impl_ver) {
> > +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > +						  sub_vendor_id, 0);
> > +		if (key)
> > +			proto = xa_load(&scmi_protocols, key);
> > +
> > +		if (proto)
> > +			return proto;
> > +	}
> > +
> > +	/* Any match on just the vendor ? */
> > +	if (sub_vendor_id) {
> > +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > +						  NULL, 0);
> > +		if (key)
> > +			proto = xa_load(&scmi_protocols, key);
> > +	}
> >
> 
> I see a pattern here, can be simplify/compress by something like below ?
> 
> static const struct scmi_protocol *
> __scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
> 			      char *sub_vendor_id, u32 impl_ver)
> {
> 	unsigned long key;
> 	struct scmi_protocol *proto = NULL;
> 
> 	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> 					  sub_vendor_id, impl_ver);
> 	if (key)
> 		proto = xa_load(&scmi_protocols, key);
> 
> 	return proto;
> }

Sure...was not completely sure to proceed that way because only 2 lines
were saved for a each block....bit indeed is more clear...I'll d in V3
> 
> [...]
> 
> > diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
> > index 317d3fb32676..45ee8b35f56d 100644
> > --- a/drivers/firmware/arm_scmi/protocols.h
> > +++ b/drivers/firmware/arm_scmi/protocols.h
> > @@ -29,6 +29,8 @@
> >  #define PROTOCOL_REV_MAJOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x))))
> >  #define PROTOCOL_REV_MINOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x))))
> >  
> > +#define SCMI_PROTOCOL_VENDOR	0x80
> > +
> 
> s/SCMI_PROTOCOL_VENDOR/SCMI_PROTOCOL_VENDOR_BASE/
> 
> as I expect 0x80 to be used by most vendors.
> 

I'll do.

Thanks for the review.
Cristian

Re: [PATCH v2 1/2] firmware: arm_scmi: Add support for multiple vendors custom protocols

[Cristian Marussi]

On Wed, Apr 17, 2024 at 01:32:08PM +0100, Sudeep Holla wrote:
> On Mon, Apr 08, 2024 at 10:30:51AM +0100, Cristian Marussi wrote:
> > Add a mechanism to be able to tag vendor protocol modules at compile-time
> > with a vendor/sub_vendor string and an implementation version and then to
> > choose to load, at run-time, only those vendor protocol modules matching
> > as close as possible the vendor/subvendor identification advertised by
> > the SCMI platform server.
> > 
> > In this way, any in-tree existent vendor protocol module can be build and
> > shipped by default in a single kernel image, even when using the same
> > clashing protocol identification numbers, since the SCMI core will take
> > care at run-time to load only the ones pertinent to the running system.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/driver.c    | 165 ++++++++++++++++++++++----
> >  drivers/firmware/arm_scmi/protocols.h |  15 +++
> >  2 files changed, 158 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index d0091459a276..aa18202054a5 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> 
> [...]
> 
> > +static const struct scmi_protocol *
> > +scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
> > +			    char *sub_vendor_id, u32 impl_ver)
> > +{
> > +	unsigned long key;
> > +	struct scmi_protocol *proto = NULL;
> > +
> > +	/* Searching for closest match ...*/
> > +	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > +					  sub_vendor_id, impl_ver);
> > +	if (key)
> > +		proto = xa_load(&scmi_protocols, key);
> > +
> > +	if (proto)
> > +		return proto;
> > +
> > +	/* Any match on vendor/sub_vendor ? */
> > +	if (impl_ver) {
> > +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > +						  sub_vendor_id, 0);
> > +		if (key)
> > +			proto = xa_load(&scmi_protocols, key);
> > +
> > +		if (proto)
> > +			return proto;
> > +	}
> > +
> > +	/* Any match on just the vendor ? */
> > +	if (sub_vendor_id) {
> > +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > +						  NULL, 0);
> > +		if (key)
> > +			proto = xa_load(&scmi_protocols, key);
> > +	}
> >
> 
> I see a pattern here, can be simplify/compress by something like below ?
> 
> static const struct scmi_protocol *
> __scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
> 			      char *sub_vendor_id, u32 impl_ver)
> {
> 	unsigned long key;
> 	struct scmi_protocol *proto = NULL;
> 
> 	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> 					  sub_vendor_id, impl_ver);
> 	if (key)
> 		proto = xa_load(&scmi_protocols, key);
> 
> 	return proto;
> }

Sure...was not completely sure to proceed that way because only 2 lines
were saved for a each block....bit indeed is more clear...I'll d in V3
> 
> [...]
> 
> > diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
> > index 317d3fb32676..45ee8b35f56d 100644
> > --- a/drivers/firmware/arm_scmi/protocols.h
> > +++ b/drivers/firmware/arm_scmi/protocols.h
> > @@ -29,6 +29,8 @@
> >  #define PROTOCOL_REV_MAJOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x))))
> >  #define PROTOCOL_REV_MINOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x))))
> >  
> > +#define SCMI_PROTOCOL_VENDOR	0x80
> > +
> 
> s/SCMI_PROTOCOL_VENDOR/SCMI_PROTOCOL_VENDOR_BASE/
> 
> as I expect 0x80 to be used by most vendors.
> 

I'll do.

Thanks for the review.
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] firmware: arm_scmi: Add dedicated vendor protocols submenu

[Cristian Marussi]

On Wed, Apr 17, 2024 at 01:09:45PM +0100, Sudeep Holla wrote:
> On Mon, Apr 08, 2024 at 10:30:52AM +0100, Cristian Marussi wrote:
> > Add a dedicated Kconfig submenu and directory where to collect SCMI vendor
> > protocols implementations.
> >
> 
> This looks fine. But I would wait until the first vendor protocol is
> ready to be merged to merge this as baseline. Hope that's OK.
> 

Absolutely, I think some vendor protocols currently on the list (beside
their ready-to-merge status) will rebase on this once this is in -next.

What remain to discuss really, it is, as I mentioned offline, if we want
to also group the vendor protocols headers (the one containing the
public protocol ops) somewhere...I think now they are just placed on top
level include/ named like

 /vendor1_scmi_something.h
 /scmi_vendor2_something_else.h

..or (as I now remembered you mentioned offline) just leave as it is for
now and then post a patch on top to shuffle around the headers into some
common include/scmi/ top dir...not sure anyway if it worth...maybe some
header name convention is fine (but I ignore if there are rules about
polluting more or less the top level /include/ dir :D)

I assume also that this mechanism is fine for vendors at this point, since
all the feedback we've got from  them till now was on specific (and very
much welcome) code-reviewed stuff...

Thanks,
Cristian

Re: [PATCH v2 2/2] firmware: arm_scmi: Add dedicated vendor protocols submenu

[Cristian Marussi]

On Wed, Apr 17, 2024 at 01:09:45PM +0100, Sudeep Holla wrote:
> On Mon, Apr 08, 2024 at 10:30:52AM +0100, Cristian Marussi wrote:
> > Add a dedicated Kconfig submenu and directory where to collect SCMI vendor
> > protocols implementations.
> >
> 
> This looks fine. But I would wait until the first vendor protocol is
> ready to be merged to merge this as baseline. Hope that's OK.
> 

Absolutely, I think some vendor protocols currently on the list (beside
their ready-to-merge status) will rebase on this once this is in -next.

What remain to discuss really, it is, as I mentioned offline, if we want
to also group the vendor protocols headers (the one containing the
public protocol ops) somewhere...I think now they are just placed on top
level include/ named like

 /vendor1_scmi_something.h
 /scmi_vendor2_something_else.h

..or (as I now remembered you mentioned offline) just leave as it is for
now and then post a patch on top to shuffle around the headers into some
common include/scmi/ top dir...not sure anyway if it worth...maybe some
header name convention is fine (but I ignore if there are rules about
polluting more or less the top level /include/ dir :D)

I assume also that this mechanism is fine for vendors at this point, since
all the feedback we've got from  them till now was on specific (and very
much welcome) code-reviewed stuff...

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] firmware: arm_scmi: Add support for multiple vendors custom protocols

[Sudeep Holla]

On Wed, Apr 17, 2024 at 01:42:50PM +0100, Cristian Marussi wrote:
> On Wed, Apr 17, 2024 at 01:32:08PM +0100, Sudeep Holla wrote:
> > On Mon, Apr 08, 2024 at 10:30:51AM +0100, Cristian Marussi wrote:
> > > Add a mechanism to be able to tag vendor protocol modules at compile-time
> > > with a vendor/sub_vendor string and an implementation version and then to
> > > choose to load, at run-time, only those vendor protocol modules matching
> > > as close as possible the vendor/subvendor identification advertised by
> > > the SCMI platform server.
> > >
> > > In this way, any in-tree existent vendor protocol module can be build and
> > > shipped by default in a single kernel image, even when using the same
> > > clashing protocol identification numbers, since the SCMI core will take
> > > care at run-time to load only the ones pertinent to the running system.
> > >
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > ---
> > >  drivers/firmware/arm_scmi/driver.c    | 165 ++++++++++++++++++++++----
> > >  drivers/firmware/arm_scmi/protocols.h |  15 +++
> > >  2 files changed, 158 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > > index d0091459a276..aa18202054a5 100644
> > > --- a/drivers/firmware/arm_scmi/driver.c
> > > +++ b/drivers/firmware/arm_scmi/driver.c
> >
> > [...]
> >
> > > +static const struct scmi_protocol *
> > > +scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
> > > +			    char *sub_vendor_id, u32 impl_ver)
> > > +{
> > > +	unsigned long key;
> > > +	struct scmi_protocol *proto = NULL;
> > > +
> > > +	/* Searching for closest match ...*/
> > > +	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > > +					  sub_vendor_id, impl_ver);
> > > +	if (key)
> > > +		proto = xa_load(&scmi_protocols, key);
> > > +
> > > +	if (proto)
> > > +		return proto;
> > > +
> > > +	/* Any match on vendor/sub_vendor ? */
> > > +	if (impl_ver) {
> > > +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > > +						  sub_vendor_id, 0);
> > > +		if (key)
> > > +			proto = xa_load(&scmi_protocols, key);
> > > +
> > > +		if (proto)
> > > +			return proto;
> > > +	}
> > > +
> > > +	/* Any match on just the vendor ? */
> > > +	if (sub_vendor_id) {
> > > +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > > +						  NULL, 0);
> > > +		if (key)
> > > +			proto = xa_load(&scmi_protocols, key);
> > > +	}
> > >
> >
> > I see a pattern here, can be simplify/compress by something like below ?
> >
> > static const struct scmi_protocol *
> > __scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
> > 			      char *sub_vendor_id, u32 impl_ver)
> > {
> > 	unsigned long key;
> > 	struct scmi_protocol *proto = NULL;
> >
> > 	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > 					  sub_vendor_id, impl_ver);
> > 	if (key)
> > 		proto = xa_load(&scmi_protocols, key);
> >
> > 	return proto;
> > }
>
> Sure...was not completely sure to proceed that way because only 2 lines
> were saved for a each block....bit indeed is more clear...I'll d in V3

Agreed, that's why I was asking rather than requesting you to change 😉.
Even I was not sure by the time I completed writing the above one 😁.
It may make it bit easier to read.

--
Regards,
Sudeep

Re: [PATCH v2 1/2] firmware: arm_scmi: Add support for multiple vendors custom protocols

[Sudeep Holla]

On Wed, Apr 17, 2024 at 01:42:50PM +0100, Cristian Marussi wrote:
> On Wed, Apr 17, 2024 at 01:32:08PM +0100, Sudeep Holla wrote:
> > On Mon, Apr 08, 2024 at 10:30:51AM +0100, Cristian Marussi wrote:
> > > Add a mechanism to be able to tag vendor protocol modules at compile-time
> > > with a vendor/sub_vendor string and an implementation version and then to
> > > choose to load, at run-time, only those vendor protocol modules matching
> > > as close as possible the vendor/subvendor identification advertised by
> > > the SCMI platform server.
> > >
> > > In this way, any in-tree existent vendor protocol module can be build and
> > > shipped by default in a single kernel image, even when using the same
> > > clashing protocol identification numbers, since the SCMI core will take
> > > care at run-time to load only the ones pertinent to the running system.
> > >
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > ---
> > >  drivers/firmware/arm_scmi/driver.c    | 165 ++++++++++++++++++++++----
> > >  drivers/firmware/arm_scmi/protocols.h |  15 +++
> > >  2 files changed, 158 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > > index d0091459a276..aa18202054a5 100644
> > > --- a/drivers/firmware/arm_scmi/driver.c
> > > +++ b/drivers/firmware/arm_scmi/driver.c
> >
> > [...]
> >
> > > +static const struct scmi_protocol *
> > > +scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
> > > +			    char *sub_vendor_id, u32 impl_ver)
> > > +{
> > > +	unsigned long key;
> > > +	struct scmi_protocol *proto = NULL;
> > > +
> > > +	/* Searching for closest match ...*/
> > > +	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > > +					  sub_vendor_id, impl_ver);
> > > +	if (key)
> > > +		proto = xa_load(&scmi_protocols, key);
> > > +
> > > +	if (proto)
> > > +		return proto;
> > > +
> > > +	/* Any match on vendor/sub_vendor ? */
> > > +	if (impl_ver) {
> > > +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > > +						  sub_vendor_id, 0);
> > > +		if (key)
> > > +			proto = xa_load(&scmi_protocols, key);
> > > +
> > > +		if (proto)
> > > +			return proto;
> > > +	}
> > > +
> > > +	/* Any match on just the vendor ? */
> > > +	if (sub_vendor_id) {
> > > +		key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > > +						  NULL, 0);
> > > +		if (key)
> > > +			proto = xa_load(&scmi_protocols, key);
> > > +	}
> > >
> >
> > I see a pattern here, can be simplify/compress by something like below ?
> >
> > static const struct scmi_protocol *
> > __scmi_vendor_protocol_lookup(int protocol_id, char *vendor_id,
> > 			      char *sub_vendor_id, u32 impl_ver)
> > {
> > 	unsigned long key;
> > 	struct scmi_protocol *proto = NULL;
> >
> > 	key = scmi_protocol_key_calculate(protocol_id, vendor_id,
> > 					  sub_vendor_id, impl_ver);
> > 	if (key)
> > 		proto = xa_load(&scmi_protocols, key);
> >
> > 	return proto;
> > }
>
> Sure...was not completely sure to proceed that way because only 2 lines
> were saved for a each block....bit indeed is more clear...I'll d in V3

Agreed, that's why I was asking rather than requesting you to change 😉.
Even I was not sure by the time I completed writing the above one 😁.
It may make it bit easier to read.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel