* [PATCH v3 1/9] firmware: arm_scmi: Set fwnode for the generated SCMI platform device
2026-08-13 11:32 [PATCH v3 0/9] firmware: arm_scmi: Refactoring and enablement of ACPI PCC transport Sudeep Holla
@ 2026-08-13 11:32 ` Sudeep Holla
2026-08-24 16:51 ` Jonathan Cameron
2026-08-13 11:32 ` [PATCH v3 2/9] firmware: arm_scmi: Extend transport driver macro to support ACPI Sudeep Holla
` (7 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2026-08-13 11:32 UTC (permalink / raw)
To: arm-scmi, linux-arm-kernel, kernel-team; +Cc: Cristian Marussi, Breno Leitao
Add a call to device_set_node() in the SCMI probe helper to associate
generated SCMI platform device with the firmware node of its supplier
transport device.
This complements device_set_of_node_from_dev() and ensures that
firmware node information is propagated correctly for both Device Tree
and non-DT (e.g. ACPI) based systems.
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
drivers/firmware/arm_scmi/common.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index fe8c22cfb9f7..cc7d11c3c1f3 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -650,6 +650,7 @@ static int __tag##_probe(struct platform_device *pdev) \
} \
\
device_set_of_node_from_dev(&spdev->dev, dev); \
+ device_set_node(&spdev->dev, dev_fwnode(dev)); \
\
strans.supplier = supplier; \
memcpy(&strans.desc, &(__desc), sizeof(strans.desc)); \
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 1/9] firmware: arm_scmi: Set fwnode for the generated SCMI platform device
2026-08-13 11:32 ` [PATCH v3 1/9] firmware: arm_scmi: Set fwnode for the generated SCMI platform device Sudeep Holla
@ 2026-08-24 16:51 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2026-08-24 16:51 UTC (permalink / raw)
To: Sudeep Holla
Cc: arm-scmi, linux-arm-kernel, kernel-team, Cristian Marussi,
Breno Leitao
On Thu, 13 Aug 2026 12:32:56 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:
> Add a call to device_set_node() in the SCMI probe helper to associate
> generated SCMI platform device with the firmware node of its supplier
> transport device.
>
> This complements device_set_of_node_from_dev() and ensures that
> firmware node information is propagated correctly for both Device Tree
> and non-DT (e.g. ACPI) based systems.
Hi Sudeep,
When you say 'complement' I'd like to see a little more. The guts
of both calls includes setting dev->ofnode() adn the extra bit
in device_set_of_node_from_dev() is about ensuring we drop existing
node reference (there isn't one) and increment the reference for what
is the parent of_node.
I may be wrong, but I think that is unnecessary and if I'm wrong I'd
like to see more info here on why it is needed.
Also, if this is commonly needed why doesn't device_set_node() include
that dance?
Unless I fumbled the command, a grep suggests there are no instances
in tree of these both being called near each other.
Jonathan
>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
> ---
> drivers/firmware/arm_scmi/common.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index fe8c22cfb9f7..cc7d11c3c1f3 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -650,6 +650,7 @@ static int __tag##_probe(struct platform_device *pdev) \
> } \
> \
> device_set_of_node_from_dev(&spdev->dev, dev); \
> + device_set_node(&spdev->dev, dev_fwnode(dev)); \
> \
> strans.supplier = supplier; \
> memcpy(&strans.desc, &(__desc), sizeof(strans.desc)); \
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 2/9] firmware: arm_scmi: Extend transport driver macro to support ACPI
2026-08-13 11:32 [PATCH v3 0/9] firmware: arm_scmi: Refactoring and enablement of ACPI PCC transport Sudeep Holla
2026-08-13 11:32 ` [PATCH v3 1/9] firmware: arm_scmi: Set fwnode for the generated SCMI platform device Sudeep Holla
@ 2026-08-13 11:32 ` Sudeep Holla
2026-08-24 17:04 ` Jonathan Cameron
2026-08-13 11:32 ` [PATCH v3 3/9] firmware: arm_scmi: Convert OF-only paths to generic fwnode in SCMI core Sudeep Holla
` (6 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2026-08-13 11:32 UTC (permalink / raw)
To: arm-scmi, linux-arm-kernel, kernel-team; +Cc: Cristian Marussi, Breno Leitao
Extend the SCMI transport driver helper to support ACPI-based systems.
Introduce an internal helper macro that accepts both OF and ACPI match
tables, and expose two wrappers:
- DEFINE_SCMI_TRANSPORT_DRIVER(...) for DT/OF transports
- DEFINE_SCMI_ACPI_TRANSPORT_DRIVER(...) for ACPI transports
For ACPI, set the generated platform_driver .acpi_match_table via
ACPI_PTR(). The ACPI wrapper relies on the firmware-node propagation
provided by the preceding change so fwnode lookups on the spawned
platform device see the correct firmware description.
Keep existing DT users unchanged while allowing transports to be probed
using struct acpi_device_id tables on ACPI platforms.
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
drivers/firmware/arm_scmi/common.h | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index cc7d11c3c1f3..0b896171faa2 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -9,6 +9,7 @@
#ifndef _SCMI_COMMON_H
#define _SCMI_COMMON_H
+#include <linux/acpi.h>
#include <linux/bitfield.h>
#include <linux/completion.h>
#include <linux/device.h>
@@ -615,7 +616,8 @@ struct scmi_transport_supplier __supplier = { \
.th.supplier_put = scmi_transport_supplier_put, \
}
-#define DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, __match, __core_ops)\
+#define __DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, __of_match, \
+ __acpi_match, __core_ops) \
static void __tag##_dev_free(void *data) \
{ \
struct platform_device *spdev = data; \
@@ -679,11 +681,18 @@ err_mem: \
static struct platform_driver __drv = { \
.driver = { \
.name = #__tag "_transport", \
- .of_match_table = __match, \
+ .of_match_table = __of_match, \
+ .acpi_match_table = ACPI_PTR(__acpi_match), \
}, \
.probe = __tag##_probe, \
}
+#define DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, __match, __core_ops)\
+ __DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, __match, NULL, __core_ops)
+
+#define DEFINE_SCMI_ACPI_TRANSPORT_DRIVER(__tag, __drv, __desc, __match, __core_ops)\
+ __DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, NULL, __match, __core_ops)
+
void scmi_notification_instance_data_set(const struct scmi_handle *handle,
void *priv);
void *scmi_notification_instance_data_get(const struct scmi_handle *handle);
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/9] firmware: arm_scmi: Extend transport driver macro to support ACPI
2026-08-13 11:32 ` [PATCH v3 2/9] firmware: arm_scmi: Extend transport driver macro to support ACPI Sudeep Holla
@ 2026-08-24 17:04 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2026-08-24 17:04 UTC (permalink / raw)
To: Sudeep Holla
Cc: arm-scmi, linux-arm-kernel, kernel-team, Cristian Marussi,
Breno Leitao
On Thu, 13 Aug 2026 12:32:57 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:
> Extend the SCMI transport driver helper to support ACPI-based systems.
> Introduce an internal helper macro that accepts both OF and ACPI match
> tables, and expose two wrappers:
>
> - DEFINE_SCMI_TRANSPORT_DRIVER(...) for DT/OF transports
> - DEFINE_SCMI_ACPI_TRANSPORT_DRIVER(...) for ACPI transports
>
> For ACPI, set the generated platform_driver .acpi_match_table via
> ACPI_PTR().
ACPI_PTR() is often more trouble that it is worth because of need
to guard the tables if they turn up in code where ACPI might not be present.
Now if someone fancied doing the work to make it work like pm_ptr()
then that would nicer. That one lets the compiler both see the stuff beyond
the pointer, but also do dead code removal on it later.
Anyhow, doesn't actually matter either way here because you only use that
with a non NULL pointer in a driver that is ACPI only.
If you really want to do it, maybe pair with of_match_ptr() but
that one is even worse as maybe some will be having fun with SCMI and
PRP0001.
Anyhow, random diversion aside, it makes no difference in practice.
> The ACPI wrapper relies on the firmware-node propagation
> provided by the preceding change so fwnode lookups on the spawned
> platform device see the correct firmware description.
>
> Keep existing DT users unchanged while allowing transports to be probed
> using struct acpi_device_id tables on ACPI platforms.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> ---
> drivers/firmware/arm_scmi/common.h | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index cc7d11c3c1f3..0b896171faa2 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -9,6 +9,7 @@
> #ifndef _SCMI_COMMON_H
> #define _SCMI_COMMON_H
>
> +#include <linux/acpi.h>
> #include <linux/bitfield.h>
> #include <linux/completion.h>
> #include <linux/device.h>
> @@ -615,7 +616,8 @@ struct scmi_transport_supplier __supplier = { \
> .th.supplier_put = scmi_transport_supplier_put, \
> }
>
> -#define DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, __match, __core_ops)\
> +#define __DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, __of_match, \
> + __acpi_match, __core_ops) \
> static void __tag##_dev_free(void *data) \
> { \
> struct platform_device *spdev = data; \
> @@ -679,11 +681,18 @@ err_mem: \
> static struct platform_driver __drv = { \
> .driver = { \
> .name = #__tag "_transport", \
> - .of_match_table = __match, \
> + .of_match_table = __of_match, \
> + .acpi_match_table = ACPI_PTR(__acpi_match), \
> }, \
> .probe = __tag##_probe, \
> }
>
> +#define DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, __match, __core_ops)\
> + __DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, __match, NULL, __core_ops)
> +
> +#define DEFINE_SCMI_ACPI_TRANSPORT_DRIVER(__tag, __drv, __desc, __match, __core_ops)\
> + __DEFINE_SCMI_TRANSPORT_DRIVER(__tag, __drv, __desc, NULL, __match, __core_ops)
> +
> void scmi_notification_instance_data_set(const struct scmi_handle *handle,
> void *priv);
> void *scmi_notification_instance_data_get(const struct scmi_handle *handle);
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 3/9] firmware: arm_scmi: Convert OF-only paths to generic fwnode in SCMI core
2026-08-13 11:32 [PATCH v3 0/9] firmware: arm_scmi: Refactoring and enablement of ACPI PCC transport Sudeep Holla
2026-08-13 11:32 ` [PATCH v3 1/9] firmware: arm_scmi: Set fwnode for the generated SCMI platform device Sudeep Holla
2026-08-13 11:32 ` [PATCH v3 2/9] firmware: arm_scmi: Extend transport driver macro to support ACPI Sudeep Holla
@ 2026-08-13 11:32 ` Sudeep Holla
2026-08-24 17:39 ` Jonathan Cameron
2026-08-13 11:32 ` [PATCH v3 4/9] firmware: arm_scmi: Fall back to ACPI HID when "compatible" is absent Sudeep Holla
` (5 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2026-08-13 11:32 UTC (permalink / raw)
To: arm-scmi, linux-arm-kernel, kernel-team; +Cc: Cristian Marussi, Breno Leitao
Switch SCMI core plumbing from struct device_node * to struct
fwnode_handle * so the core can describe SCMI instances and protocols using
firmware nodes rather than OF nodes directly.
This change:
- Replaces core OF property lookups with fwnode_property_*() helpers.
- Switches child enumeration to
fwnode_for_each_available_child_node_scoped().
- Plumbs fwnode through the SCMI device creation and channel setup paths.
- Updates transport ->chan_available() callbacks to take a fwnode.
- Stores per-protocol child fwnodes in info->active_protocols so the core
can later locate the descriptor for a given protocol ID.
- Updates mailbox/optee/smc/virtio transports to accept fwnodes and map
back to OF nodes where their existing parsing remains DT-specific.
DT-only transports such as mailbox, OP-TEE and SMC still parse DT
properties by mapping the fwnode back to an OF node. On non-DT systems
these transports report no channel available.
This is a mechanical step towards firmware-node neutrality and prepares the
SCMI core for non-DT transports, such as an ACPI/PCC transport. DT users
continue to work unchanged; no non-DT transport is enabled by this patch.
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
drivers/firmware/arm_scmi/bus.c | 37 ++++-----
drivers/firmware/arm_scmi/common.h | 9 +-
drivers/firmware/arm_scmi/driver.c | 110 +++++++++++++------------
drivers/firmware/arm_scmi/transports/mailbox.c | 6 +-
drivers/firmware/arm_scmi/transports/optee.c | 6 +-
drivers/firmware/arm_scmi/transports/smc.c | 11 ++-
drivers/firmware/arm_scmi/transports/virtio.c | 2 +-
7 files changed, 98 insertions(+), 83 deletions(-)
diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index e060edbe7e83..a86fa18fc5d2 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -9,7 +9,7 @@
#include <linux/types.h>
#include <linux/module.h>
-#include <linux/of.h>
+#include <linux/property.h>
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/device.h>
@@ -429,17 +429,16 @@ static void scmi_device_release(struct device *dev)
struct scmi_device *scmi_dev = to_scmi_dev(dev);
scmi_device_release_resources(scmi_dev);
- of_node_put(dev->of_node);
+ fwnode_handle_put(dev_fwnode(dev));
kfree_const(scmi_dev->name);
kfree(scmi_dev);
}
static void __scmi_device_destroy(struct scmi_device *scmi_dev)
{
- pr_debug("(%pOF) Destroying SCMI device '%s' for protocol 0x%x (%s)\n",
- scmi_dev->dev.parent->of_node,
- dev_name(&scmi_dev->dev), scmi_dev->protocol_id,
- scmi_dev->name);
+ pr_debug("(%pfwf) Destroying SCMI device '%s' for protocol 0x%x (%s)\n",
+ dev_fwnode(&scmi_dev->dev), dev_name(&scmi_dev->dev),
+ scmi_dev->protocol_id, scmi_dev->name);
device_del(&scmi_dev->dev);
scmi_device_release_resources(scmi_dev);
@@ -447,7 +446,7 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
}
static struct scmi_device *
-__scmi_device_create(struct device_node *np, struct device *parent,
+__scmi_device_create(struct fwnode_handle *fwnode, struct device *parent,
int protocol, const char *name)
{
int id, retval;
@@ -458,7 +457,7 @@ __scmi_device_create(struct device_node *np, struct device *parent,
* If the same protocol/name device already exist under the same parent
* (i.e. SCMI instance) just return the existent device.
* This avoids any race between the SCMI driver, creating devices for
- * each DT defined protocol at probe time, and the concurrent
+ * each fwnode defined protocol at probe time, and the concurrent
* registration of SCMI drivers.
*/
scmi_dev = scmi_child_dev_find_get(parent, protocol, name);
@@ -495,7 +494,7 @@ __scmi_device_create(struct device_node *np, struct device *parent,
scmi_dev->id = id;
scmi_dev->dev.parent = parent;
- device_set_node(&scmi_dev->dev, of_fwnode_handle(of_node_get(np)));
+ device_set_node(&scmi_dev->dev, fwnode_handle_get(fwnode));
scmi_dev->dev.bus = &scmi_bus_type;
scmi_dev->dev.release = scmi_device_release;
dev_set_name(&scmi_dev->dev, "scmi_dev.%d", id);
@@ -504,8 +503,8 @@ __scmi_device_create(struct device_node *np, struct device *parent,
if (retval)
goto put_dev;
- pr_debug("(%pOF) Created SCMI device '%s' for protocol 0x%x (%s)\n",
- parent->of_node, dev_name(&scmi_dev->dev), protocol, name);
+ pr_debug("(%pfwf) Created SCMI device '%s' - protocol 0x%x (%s)\n",
+ fwnode, dev_name(&scmi_dev->dev), protocol, name);
return scmi_dev;
put_dev:
@@ -521,15 +520,15 @@ __scmi_device_create(struct device_node *np, struct device *parent,
}
static struct scmi_device *
-_scmi_device_create(struct device_node *np, struct device *parent,
+_scmi_device_create(struct fwnode_handle *fwnode, struct device *parent,
int protocol, const char *name)
{
struct scmi_device *sdev;
- sdev = __scmi_device_create(np, parent, protocol, name);
+ sdev = __scmi_device_create(fwnode, parent, protocol, name);
if (!sdev)
- pr_err("(%pOF) Failed to create device for protocol 0x%x (%s)\n",
- parent->of_node, protocol, name);
+ pr_err("(%pfwf) Failed to create device - protocol 0x%x (%s)\n",
+ fwnode, protocol, name);
return sdev;
}
@@ -537,7 +536,7 @@ _scmi_device_create(struct device_node *np, struct device *parent,
/**
* scmi_device_create - A method to create one or more SCMI devices
*
- * @np: A reference to the device node to use for the new device(s)
+ * @fwnode: A reference to the device node to use for the new device(s)
* @parent: The parent device to use identifying a specific SCMI instance
* @protocol: The SCMI protocol to be associated with this device
* @name: The requested-name of the device to be created; this is optional
@@ -557,7 +556,7 @@ _scmi_device_create(struct device_node *np, struct device *parent,
* could have been potentially created for a whole protocol, unless no
* device was found to have been requested for that specific protocol.
*/
-struct scmi_device *scmi_device_create(struct device_node *np,
+struct scmi_device *scmi_device_create(struct fwnode_handle *fwnode,
struct device *parent, int protocol,
const char *name)
{
@@ -566,7 +565,7 @@ struct scmi_device *scmi_device_create(struct device_node *np,
struct scmi_device *scmi_dev = NULL;
if (name)
- return _scmi_device_create(np, parent, protocol, name);
+ return _scmi_device_create(fwnode, parent, protocol, name);
mutex_lock(&scmi_requested_devices_mtx);
phead = idr_find(&scmi_requested_devices, protocol);
@@ -580,7 +579,7 @@ struct scmi_device *scmi_device_create(struct device_node *np,
list_for_each_entry(rdev, phead, node) {
struct scmi_device *sdev;
- sdev = _scmi_device_create(np, parent,
+ sdev = _scmi_device_create(fwnode, parent,
rdev->id_table->protocol_id,
rdev->id_table->name);
if (sdev)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 0b896171faa2..042867c28b88 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -153,7 +153,7 @@ extern const struct bus_type scmi_bus_type;
#define SCMI_BUS_NOTIFY_DEVICE_UNREQUEST 1
extern struct blocking_notifier_head scmi_requested_devices_nh;
-struct scmi_device *scmi_device_create(struct device_node *np,
+struct scmi_device *scmi_device_create(struct fwnode_handle *fwnode,
struct device *parent, int protocol,
const char *name);
void scmi_device_destroy(struct device *parent, int protocol, const char *name);
@@ -207,7 +207,7 @@ struct scmi_chan_info {
* @poll_done: Callback to poll transfer status
*/
struct scmi_transport_ops {
- bool (*chan_available)(struct device_node *of_node, int idx);
+ bool (*chan_available)(struct fwnode_handle *fwnode, int idx);
int (*chan_setup)(struct scmi_chan_info *cinfo, struct device *dev,
bool tx);
int (*chan_free)(int id, void *p, void *data);
@@ -233,7 +233,7 @@ struct scmi_transport_ops {
* be pending simultaneously in the system. May be overridden by the
* get_max_msg op.
* @max_msg_size: Maximum size of data payload per message that can be handled.
- * @atomic_threshold: Optional system wide DT-configured threshold, expressed
+ * @atomic_threshold: Optional system wide fwnode-configured threshold, expressed
* in microseconds, for atomic operations.
* Only SCMI synchronous commands reported by the platform
* to have an execution latency lesser-equal to the threshold
@@ -241,7 +241,8 @@ struct scmi_transport_ops {
* decision is finally left up to the SCMI drivers.
* @no_completion_irq: Flag to indicate that this transport has no completion
* interrupt and has to be polled. This is similar to the
- * force_polling below, except this is set via DT property.
+ * force_polling below, except this is set via fwnode
+ * property.
* @force_polling: Flag to force this whole transport to use SCMI core polling
* mechanism instead of completion interrupts even if available.
* @sync_cmds_completed_on_ret: Flag to indicate that the transport assures
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index ef29fd223287..9014723e0f7f 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -30,7 +30,7 @@
#include <linux/hashtable.h>
#include <linux/list.h>
#include <linux/module.h>
-#include <linux/of.h>
+#include <linux/property.h>
#include <linux/platform_device.h>
#include <linux/processor.h>
#include <linux/rcupdate.h>
@@ -102,7 +102,7 @@ struct scmi_xfers_info {
* initialization code to identify this instance.
*
* Each protocol is initialized independently once for each SCMI platform in
- * which is defined by DT and implemented by the SCMI server fw.
+ * which is defined by fwnode and implemented by the SCMI server fw.
*/
struct scmi_protocol_instance {
const struct scmi_handle *handle;
@@ -137,8 +137,9 @@ struct scmi_protocol_instance {
* @protocols_imp: List of protocols implemented, currently maximum of
* scmi_base_info.num_protocols elements allocated by the
* base protocol
- * @active_protocols: IDR storing device_nodes for protocols actually defined
- * in the DT and confirmed as implemented by fw.
+ * @active_protocols: IDR storing fwnodes for protocols actually defined
+ * in the firmware description and confirmed as implemented
+ * by fw.
* @notify_priv: Pointer to private data structure specific to notifications.
* @node: List head
* @users: Number of users of this instance
@@ -416,19 +417,19 @@ EXPORT_SYMBOL_GPL(scmi_protocol_unregister);
* scmi_create_protocol_devices - Create devices for all pending requests for
* this SCMI instance.
*
- * @np: The device node describing the protocol
+ * @fwnode: The firmware node describing the protocol
* @info: The SCMI instance descriptor
* @prot_id: The protocol ID
* @name: The optional name of the device to be created: if not provided this
* call will lead to the creation of all the devices currently requested
* for the specified protocol.
*/
-static void scmi_create_protocol_devices(struct device_node *np,
+static void scmi_create_protocol_devices(struct fwnode_handle *fwnode,
struct scmi_info *info,
int prot_id, const char *name)
{
mutex_lock(&info->devreq_mtx);
- scmi_device_create(np, info->dev, prot_id, name);
+ scmi_device_create(fwnode, info->dev, prot_id, name);
mutex_unlock(&info->devreq_mtx);
}
@@ -752,9 +753,10 @@ struct scmi_xfer *scmi_xfer_raw_get(const struct scmi_handle *handle)
* @protocol_id: Identifier of the protocol
*
* Note that in a regular SCMI stack, usually, a protocol has to be defined in
- * the DT to have an associated channel and be usable; but in Raw mode any
- * protocol in range is allowed, re-using the Base channel, so as to enable
- * fuzzing on any protocol without the need of a fully compiled DT.
+ * the firmware description to have an associated channel and be usable; but in
+ * Raw mode any protocol in range is allowed, re-using the Base channel, so as
+ * to enable fuzzing on any protocol without the need of a fully compiled
+ * firmware description.
*
* Return: A reference to the channel to use, or an ERR_PTR
*/
@@ -768,7 +770,7 @@ scmi_xfer_raw_channel_get(const struct scmi_handle *handle, u8 protocol_id)
if (!cinfo) {
if (protocol_id == SCMI_PROTOCOL_BASE)
return ERR_PTR(-EINVAL);
- /* Use Base channel for protocols not defined for DT */
+ /* Use Base channel for protocols not defined for fwnode */
cinfo = idr_find(&info->tx_idr, SCMI_PROTOCOL_BASE);
if (!cinfo)
return ERR_PTR(-EINVAL);
@@ -2749,7 +2751,7 @@ static int scmi_xfer_info_init(struct scmi_info *sinfo)
return ret;
}
-static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
+static int scmi_chan_setup(struct scmi_info *info, struct fwnode_handle *fwnode,
int prot_id, bool tx)
{
int ret, idx;
@@ -2765,7 +2767,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
if (idr_find(idr, prot_id))
return -EEXIST;
- if (!info->desc->ops->chan_available(of_node, idx)) {
+ if (!info->desc->ops->chan_available(fwnode, idx)) {
cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
return -EINVAL;
@@ -2785,7 +2787,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
snprintf(name, sizeof(name), SCMI_TRANSPORT_DEVNAME_PREFIX "_%s_%02X",
idx ? "rx" : "tx", prot_id);
/* Create a uniquely named, dedicated transport device for this chan */
- tdev = scmi_device_create(of_node, info->dev, prot_id, name);
+ tdev = scmi_device_create(fwnode, info->dev, prot_id, name);
if (!tdev) {
dev_err(info->dev,
"failed to create transport device (%s)\n", name);
@@ -2831,14 +2833,14 @@ static int scmi_chan_setup(struct scmi_info *info, struct device_node *of_node,
}
static inline int
-scmi_txrx_setup(struct scmi_info *info, struct device_node *of_node,
+scmi_txrx_setup(struct scmi_info *info, struct fwnode_handle *fwnode,
int prot_id)
{
- int ret = scmi_chan_setup(info, of_node, prot_id, true);
+ int ret = scmi_chan_setup(info, fwnode, prot_id, true);
if (!ret) {
/* Rx is optional, report only memory errors */
- ret = scmi_chan_setup(info, of_node, prot_id, false);
+ ret = scmi_chan_setup(info, fwnode, prot_id, false);
if (ret && ret != -ENOMEM)
ret = 0;
}
@@ -2855,15 +2857,15 @@ scmi_txrx_setup(struct scmi_info *info, struct device_node *of_node,
*
* @info: The SCMI instance descriptor.
*
- * Initialize all the channels found described in the DT against the underlying
- * configured transport using custom defined dedicated devices instead of
- * borrowing devices from the SCMI drivers; this way channels are initialized
+ * Initialize all the channels found described in the fwnode against the
+ * underlying configured transport using custom defined dedicated devices instead
+ * of borrowing devices from the SCMI drivers; this way channels are initialized
* upfront during core SCMI stack probing and are no more coupled with SCMI
* devices used by SCMI drivers.
*
* Note that, even though a pair of TX/RX channels is associated to each
- * protocol defined in the DT, a distinct freshly initialized channel is
- * created only if the DT node for the protocol at hand describes a dedicated
+ * protocol defined in the fwnode, a distinct freshly initialized channel is
+ * created only if the fwnode for the protocol at hand describes a dedicated
* channel: in all the other cases the common BASE protocol channel is reused.
*
* Return: 0 on Success
@@ -2871,17 +2873,17 @@ scmi_txrx_setup(struct scmi_info *info, struct device_node *of_node,
static int scmi_channels_setup(struct scmi_info *info)
{
int ret;
- struct device_node *top_np = info->dev->of_node;
+ struct fwnode_handle *fwnode = dev_fwnode(info->dev);
/* Initialize a common generic channel at first */
- ret = scmi_txrx_setup(info, top_np, SCMI_PROTOCOL_BASE);
+ ret = scmi_txrx_setup(info, fwnode, SCMI_PROTOCOL_BASE);
if (ret)
return ret;
- for_each_available_child_of_node_scoped(top_np, child) {
+ fwnode_for_each_available_child_node_scoped(fwnode, child) {
u32 prot_id;
- if (of_property_read_u32(child, "reg", &prot_id))
+ if (fwnode_property_read_u32(child, "reg", &prot_id))
continue;
if (!FIELD_FIT(MSG_PROTOCOL_ID_MASK, prot_id)) {
@@ -2969,14 +2971,14 @@ static int scmi_bus_notifier(struct notifier_block *nb,
static int scmi_device_request_notifier(struct notifier_block *nb,
unsigned long action, void *data)
{
- struct device_node *np;
+ struct fwnode_handle *fwnode;
struct scmi_device_id *id_table = data;
struct scmi_info *info = req_nb_to_scmi_info(nb);
rcu_read_lock();
- np = idr_find(&info->active_protocols, id_table->protocol_id);
+ fwnode = idr_find(&info->active_protocols, id_table->protocol_id);
rcu_read_unlock();
- if (!np)
+ if (!fwnode)
return NOTIFY_DONE;
dev_dbg(info->dev, "%sRequested device (%s) for protocol 0x%x\n",
@@ -2985,7 +2987,7 @@ static int scmi_device_request_notifier(struct notifier_block *nb,
switch (action) {
case SCMI_BUS_NOTIFY_DEVICE_REQUEST:
- scmi_create_protocol_devices(np, info, id_table->protocol_id,
+ scmi_create_protocol_devices(fwnode, info, id_table->protocol_id,
id_table->name);
break;
case SCMI_BUS_NOTIFY_DEVICE_UNREQUEST:
@@ -3072,13 +3074,14 @@ static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
if (!dbg)
return NULL;
- dbg->name = kstrdup(of_node_full_name(info->dev->of_node), GFP_KERNEL);
+ dbg->name = kstrdup(fwnode_get_name(dev_fwnode(info->dev)), GFP_KERNEL);
if (!dbg->name) {
devm_kfree(info->dev, dbg);
return NULL;
}
- of_property_read_string(info->dev->of_node, "compatible", &c_ptr);
+ fwnode_property_read_string(dev_fwnode(info->dev), "compatible",
+ &c_ptr);
dbg->type = kstrdup(c_ptr, GFP_KERNEL);
if (!dbg->type) {
kfree(dbg->name);
@@ -3184,23 +3187,23 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
dev_info(dev, "Using %s\n", dev_driver_string(trans->supplier));
- ret = of_property_read_u32(dev->of_node, "arm,max-rx-timeout-ms",
- &trans->desc.max_rx_timeout_ms);
+ ret = fwnode_property_read_u32(dev_fwnode(dev), "arm,max-rx-timeout-ms",
+ &trans->desc.max_rx_timeout_ms);
if (ret && ret != -EINVAL)
- dev_err(dev, "Malformed arm,max-rx-timeout-ms DT property.\n");
+ dev_err(dev, "Malformed arm,max-rx-timeout-ms property.\n");
- ret = of_property_read_u32(dev->of_node, "arm,max-msg-size",
- &trans->desc.max_msg_size);
+ ret = fwnode_property_read_u32(dev_fwnode(dev), "arm,max-msg-size",
+ &trans->desc.max_msg_size);
if (ret && ret != -EINVAL)
- dev_err(dev, "Malformed arm,max-msg-size DT property.\n");
+ dev_err(dev, "Malformed arm,max-msg-size property.\n");
- ret = of_property_read_u32(dev->of_node, "arm,max-msg",
- &trans->desc.max_msg);
+ ret = fwnode_property_read_u32(dev_fwnode(dev), "arm,max-msg",
+ &trans->desc.max_msg);
if (ret && ret != -EINVAL)
- dev_err(dev, "Malformed arm,max-msg DT property.\n");
+ dev_err(dev, "Malformed arm,max-msg property.\n");
- trans->desc.no_completion_irq = of_property_read_bool(dev->of_node,
- "arm,no-completion-irq");
+ trans->desc.no_completion_irq =
+ fwnode_property_read_bool(dev_fwnode(dev), "arm,no-completion-irq");
dev_info(dev,
"SCMI max-rx-timeout: %dms / max-msg-size: %dbytes / max-msg: %d\n",
@@ -3208,8 +3211,8 @@ static const struct scmi_desc *scmi_transport_setup(struct device *dev)
trans->desc.max_msg);
/* System wide atomic threshold for atomic ops .. if any */
- if (!of_property_read_u32(dev->of_node, "atomic-threshold-us",
- &trans->desc.atomic_threshold))
+ if (!fwnode_property_read_u32(dev_fwnode(dev), "atomic-threshold-us",
+ &trans->desc.atomic_threshold))
dev_info(dev,
"SCMI System wide atomic threshold set to %u us\n",
trans->desc.atomic_threshold);
@@ -3238,7 +3241,6 @@ static int scmi_probe(struct platform_device *pdev)
struct scmi_info *info;
bool coex = IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT_COEX);
struct device *dev = &pdev->dev;
- struct device_node *child, *np = dev->of_node;
desc = scmi_transport_setup(dev);
if (!desc) {
@@ -3277,7 +3279,7 @@ static int scmi_probe(struct platform_device *pdev)
handle->devm_protocol_put = scmi_devm_protocol_put;
handle->is_transport_atomic = scmi_is_transport_atomic;
- /* Setup all channels described in the DT at first */
+ /* Setup all channels described in the fwnode at first */
ret = scmi_channels_setup(info);
if (ret) {
err_str = "failed to setup channels\n";
@@ -3352,10 +3354,10 @@ static int scmi_probe(struct platform_device *pdev)
scmi_enable_matching_quirks(info);
- for_each_available_child_of_node(np, child) {
+ fwnode_for_each_available_child_node_scoped(dev_fwnode(dev), child) {
u32 prot_id;
- if (of_property_read_u32(child, "reg", &prot_id))
+ if (fwnode_property_read_u32(child, "reg", &prot_id))
continue;
if (!FIELD_FIT(MSG_PROTOCOL_ID_MASK, prot_id)) {
@@ -3370,8 +3372,8 @@ static int scmi_probe(struct platform_device *pdev)
}
/*
- * Save this valid DT protocol descriptor amongst
- * @active_protocols for this SCMI instance/
+ * Save this valid fwnode protocol descriptor amongst
+ * @active_protocols for this SCMI instance.
*/
ret = idr_alloc(&info->active_protocols, child,
prot_id, prot_id + 1, GFP_KERNEL);
@@ -3381,7 +3383,7 @@ static int scmi_probe(struct platform_device *pdev)
continue;
}
- of_node_get(child);
+ fwnode_handle_get(child);
scmi_create_protocol_devices(child, info, prot_id, NULL);
}
@@ -3409,7 +3411,7 @@ static void scmi_remove(struct platform_device *pdev)
{
int id;
struct scmi_info *info = platform_get_drvdata(pdev);
- struct device_node *child;
+ struct fwnode_handle *child;
if (IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT))
scmi_raw_mode_cleanup(info->raw);
@@ -3434,7 +3436,7 @@ static void scmi_remove(struct platform_device *pdev)
mutex_unlock(&info->protocols_mtx);
idr_for_each_entry(&info->active_protocols, child, id)
- of_node_put(child);
+ fwnode_handle_put(child);
idr_destroy(&info->active_protocols);
bus_unregister_notifier(&scmi_bus_type, &info->bus_nb);
diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
index 308736c3ead9..b692cae4ff25 100644
--- a/drivers/firmware/arm_scmi/transports/mailbox.c
+++ b/drivers/firmware/arm_scmi/transports/mailbox.c
@@ -77,9 +77,13 @@ static void rx_callback(struct mbox_client *cl, void *m)
core->shmem->read_header(smbox->shmem), NULL);
}
-static bool mailbox_chan_available(struct device_node *of_node, int idx)
+static bool mailbox_chan_available(struct fwnode_handle *fwnode, int idx)
{
int num_mb;
+ struct device_node *of_node = to_of_node(fwnode);
+
+ if (!of_node)
+ return false;
/*
* Just check if bidirrectional channels are involved, and check the
diff --git a/drivers/firmware/arm_scmi/transports/optee.c b/drivers/firmware/arm_scmi/transports/optee.c
index dbe32e141748..07d3affa0964 100644
--- a/drivers/firmware/arm_scmi/transports/optee.c
+++ b/drivers/firmware/arm_scmi/transports/optee.c
@@ -314,9 +314,13 @@ static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t
return 0;
}
-static bool scmi_optee_chan_available(struct device_node *of_node, int idx)
+static bool scmi_optee_chan_available(struct fwnode_handle *fwnode, int idx)
{
u32 channel_id;
+ struct device_node *of_node = to_of_node(fwnode);
+
+ if (!of_node)
+ return false;
return !of_property_read_u32_index(of_node, "linaro,optee-channel-id",
idx, &channel_id);
diff --git a/drivers/firmware/arm_scmi/transports/smc.c b/drivers/firmware/arm_scmi/transports/smc.c
index 1fce3ccdeb7f..1079cd01190b 100644
--- a/drivers/firmware/arm_scmi/transports/smc.c
+++ b/drivers/firmware/arm_scmi/transports/smc.c
@@ -84,10 +84,15 @@ static irqreturn_t smc_msg_done_isr(int irq, void *data)
return IRQ_HANDLED;
}
-static bool smc_chan_available(struct device_node *of_node, int idx)
+static bool smc_chan_available(struct fwnode_handle *fwnode, int idx)
{
- struct device_node *np __free(device_node) =
- of_parse_phandle(of_node, "shmem", 0);
+ struct device_node *of_node = to_of_node(fwnode);
+ struct device_node *np __free(device_node) = NULL;
+
+ if (!of_node)
+ return false;
+
+ np = of_parse_phandle(of_node, "shmem", 0);
if (!np)
return false;
diff --git a/drivers/firmware/arm_scmi/transports/virtio.c b/drivers/firmware/arm_scmi/transports/virtio.c
index 3282d8271839..6b060d61e0ca 100644
--- a/drivers/firmware/arm_scmi/transports/virtio.c
+++ b/drivers/firmware/arm_scmi/transports/virtio.c
@@ -375,7 +375,7 @@ static unsigned int virtio_get_max_msg(struct scmi_chan_info *base_cinfo)
return vioch->max_msg;
}
-static bool virtio_chan_available(struct device_node *of_node, int idx)
+static bool virtio_chan_available(struct fwnode_handle *fwnode, int idx)
{
struct scmi_vio_channel *channels, *vioch = NULL;
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/9] firmware: arm_scmi: Convert OF-only paths to generic fwnode in SCMI core
2026-08-13 11:32 ` [PATCH v3 3/9] firmware: arm_scmi: Convert OF-only paths to generic fwnode in SCMI core Sudeep Holla
@ 2026-08-24 17:39 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2026-08-24 17:39 UTC (permalink / raw)
To: Sudeep Holla
Cc: arm-scmi, linux-arm-kernel, kernel-team, Cristian Marussi,
Breno Leitao
On Thu, 13 Aug 2026 12:32:58 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:
> Switch SCMI core plumbing from struct device_node * to struct
> fwnode_handle * so the core can describe SCMI instances and protocols using
> firmware nodes rather than OF nodes directly.
>
> This change:
> - Replaces core OF property lookups with fwnode_property_*() helpers.
> - Switches child enumeration to
> fwnode_for_each_available_child_node_scoped().
Why the scoped version? See inline for more on this. Probably just needs
a comment here (or use the non _scoped variant)
> - Plumbs fwnode through the SCMI device creation and channel setup paths.
> - Updates transport ->chan_available() callbacks to take a fwnode.
> - Stores per-protocol child fwnodes in info->active_protocols so the core
> can later locate the descriptor for a given protocol ID.
> - Updates mailbox/optee/smc/virtio transports to accept fwnodes and map
> back to OF nodes where their existing parsing remains DT-specific.
>
> DT-only transports such as mailbox, OP-TEE and SMC still parse DT
> properties by mapping the fwnode back to an OF node. On non-DT systems
> these transports report no channel available.
>
> This is a mechanical step towards firmware-node neutrality and prepares the
> SCMI core for non-DT transports, such as an ACPI/PCC transport. DT users
> continue to work unchanged; no non-DT transport is enabled by this patch.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
A few minor things - with those cleaned up. Only the cleanup.h one really
matters.
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index ef29fd223287..9014723e0f7f 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
...
> @@ -3352,10 +3354,10 @@ static int scmi_probe(struct platform_device *pdev)
>
> scmi_enable_matching_quirks(info);
>
> - for_each_available_child_of_node(np, child) {
> + fwnode_for_each_available_child_node_scoped(dev_fwnode(dev), child) {
There are no early exits yet and I can't see any added later in the series.
So the scoped bit is irrelevant. I don't mind the change on basis of
hardening or similar but it needs a comment in the patch description to
justify that.
> u32 prot_id;
>
> - if (of_property_read_u32(child, "reg", &prot_id))
> + if (fwnode_property_read_u32(child, "reg", &prot_id))
> continue;
>
> if (!FIELD_FIT(MSG_PROTOCOL_ID_MASK, prot_id)) {
> @@ -3370,8 +3372,8 @@ static int scmi_probe(struct platform_device *pdev)
> }
>
> /*
> - * Save this valid DT protocol descriptor amongst
> - * @active_protocols for this SCMI instance/
> + * Save this valid fwnode protocol descriptor amongst
> + * @active_protocols for this SCMI instance.
> */
> ret = idr_alloc(&info->active_protocols, child,
> prot_id, prot_id + 1, GFP_KERNEL);
> @@ -3381,7 +3383,7 @@ static int scmi_probe(struct platform_device *pdev)
> continue;
> }
>
> - of_node_get(child);
> + fwnode_handle_get(child);
> scmi_create_protocol_devices(child, info, prot_id, NULL);
Give this is handing over ownership I'd make that explicit and do
scmi_create_protcol_devices(fwnode_handle_get(child), info,
prot_id, NULL);
but that is a slightly unrelated change - so I don't mind if you don't change
this one.
> }
>
...
> diff --git a/drivers/firmware/arm_scmi/transports/smc.c b/drivers/firmware/arm_scmi/transports/smc.c
> index 1fce3ccdeb7f..1079cd01190b 100644
> --- a/drivers/firmware/arm_scmi/transports/smc.c
> +++ b/drivers/firmware/arm_scmi/transports/smc.c
> @@ -84,10 +84,15 @@ static irqreturn_t smc_msg_done_isr(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> -static bool smc_chan_available(struct device_node *of_node, int idx)
> +static bool smc_chan_available(struct fwnode_handle *fwnode, int idx)
> {
> - struct device_node *np __free(device_node) =
> - of_parse_phandle(of_node, "shmem", 0);
> + struct device_node *of_node = to_of_node(fwnode);
> + struct device_node *np __free(device_node) = NULL;
See guidance in cleanup.h. Basically says constructor and destructor
must be together (that was after some grumpy replies from Linus to early
cleanup.h users!)
> +
> + if (!of_node)
> + return false;
> +
> + np = of_parse_phandle(of_node, "shmem", 0);
struct device_node *np __free(device_node) =
of_parse_phandle(of_node, "shmem", 0);
And don't worry about the inline declaration - this case is specifically
allowed.
> if (!np)
> return false;
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 4/9] firmware: arm_scmi: Fall back to ACPI HID when "compatible" is absent
2026-08-13 11:32 [PATCH v3 0/9] firmware: arm_scmi: Refactoring and enablement of ACPI PCC transport Sudeep Holla
` (2 preceding siblings ...)
2026-08-13 11:32 ` [PATCH v3 3/9] firmware: arm_scmi: Convert OF-only paths to generic fwnode in SCMI core Sudeep Holla
@ 2026-08-13 11:32 ` Sudeep Holla
2026-08-24 17:43 ` Jonathan Cameron
2026-08-13 11:33 ` [PATCH v3 5/9] firmware: arm_scmi: Pass protocol ID to chan_available() transport callback Sudeep Holla
` (4 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2026-08-13 11:32 UTC (permalink / raw)
To: arm-scmi, linux-arm-kernel, kernel-team; +Cc: Cristian Marussi, Breno Leitao
scmi_debugfs_common_setup() uses the "compatible" property to
populate the debugfs transport type string. ACPI-described SCMI
devices do not provide that DT property, so the string remains
NULL and debugfs setup falls through the allocation failure path.
Check the property lookup result and use the ACPI HID as the
fallback transport type when an ACPI companion is present.
All supported DT SCMI platforms are expected to provide "compatible",
so the non-ACPI fallback is not needed for normal DT operation. Keep
the explicit "unknown" fallback anyway to avoid passing NULL to
kstrdup() if that assumption is ever violated.
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
drivers/firmware/arm_scmi/driver.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 9014723e0f7f..9ad827c6a9ab 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -16,6 +16,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/acpi.h>
#include <linux/bitmap.h>
#include <linux/cleanup.h>
#include <linux/debugfs.h>
@@ -3063,6 +3064,15 @@ static void scmi_debugfs_common_cleanup(void *d)
kfree(dbg->type);
}
+static const char *scmi_acpi_device_hid(struct acpi_device *adev)
+{
+#ifdef CONFIG_ACPI
+ return adev ? acpi_device_hid(adev) : "unknown";
+#else
+ return "unknown";
+#endif
+}
+
static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
{
char top_dir[16];
@@ -3080,8 +3090,10 @@ static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
return NULL;
}
- fwnode_property_read_string(dev_fwnode(info->dev), "compatible",
- &c_ptr);
+ if (fwnode_property_read_string(dev_fwnode(info->dev), "compatible",
+ &c_ptr))
+ c_ptr = scmi_acpi_device_hid(ACPI_COMPANION(info->dev));
+
dbg->type = kstrdup(c_ptr, GFP_KERNEL);
if (!dbg->type) {
kfree(dbg->name);
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 4/9] firmware: arm_scmi: Fall back to ACPI HID when "compatible" is absent
2026-08-13 11:32 ` [PATCH v3 4/9] firmware: arm_scmi: Fall back to ACPI HID when "compatible" is absent Sudeep Holla
@ 2026-08-24 17:43 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2026-08-24 17:43 UTC (permalink / raw)
To: Sudeep Holla
Cc: arm-scmi, linux-arm-kernel, kernel-team, Cristian Marussi,
Breno Leitao
On Thu, 13 Aug 2026 12:32:59 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:
> scmi_debugfs_common_setup() uses the "compatible" property to
> populate the debugfs transport type string. ACPI-described SCMI
> devices do not provide that DT property, so the string remains
> NULL and debugfs setup falls through the allocation failure path.
>
> Check the property lookup result and use the ACPI HID as the
> fallback transport type when an ACPI companion is present.
>
> All supported DT SCMI platforms are expected to provide "compatible",
> so the non-ACPI fallback is not needed for normal DT operation. Keep
> the explicit "unknown" fallback anyway to avoid passing NULL to
> kstrdup() if that assumption is ever violated.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
Can drop some complexity in here and rely on the stubs in acpi.h a little more.
> ---
> drivers/firmware/arm_scmi/driver.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 9014723e0f7f..9ad827c6a9ab 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -16,6 +16,7 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> +#include <linux/acpi.h>
> #include <linux/bitmap.h>
> #include <linux/cleanup.h>
> #include <linux/debugfs.h>
> @@ -3063,6 +3064,15 @@ static void scmi_debugfs_common_cleanup(void *d)
> kfree(dbg->type);
> }
>
> +static const char *scmi_acpi_device_hid(struct acpi_device *adev)
> +{
> +#ifdef CONFIG_ACPI
Hmm. Ugly that acpi_device_id returns an empty string.
However given when !CONFIG_ACPI the stub for ACPI_COMPANION(dev) is
NULL. You don't need this dance anyway.
> + return adev ? acpi_device_hid(adev) : "unknown";
> +#else
> + return "unknown";
> +#endif
> +}
> +
> static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
> {
> char top_dir[16];
> @@ -3080,8 +3090,10 @@ static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
> return NULL;
> }
>
> - fwnode_property_read_string(dev_fwnode(info->dev), "compatible",
> - &c_ptr);
> + if (fwnode_property_read_string(dev_fwnode(info->dev), "compatible",
> + &c_ptr))
> + c_ptr = scmi_acpi_device_hid(ACPI_COMPANION(info->dev));
> +
> dbg->type = kstrdup(c_ptr, GFP_KERNEL);
> if (!dbg->type) {
> kfree(dbg->name);
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 5/9] firmware: arm_scmi: Pass protocol ID to chan_available() transport callback
2026-08-13 11:32 [PATCH v3 0/9] firmware: arm_scmi: Refactoring and enablement of ACPI PCC transport Sudeep Holla
` (3 preceding siblings ...)
2026-08-13 11:32 ` [PATCH v3 4/9] firmware: arm_scmi: Fall back to ACPI HID when "compatible" is absent Sudeep Holla
@ 2026-08-13 11:33 ` Sudeep Holla
2026-08-24 17:45 ` Jonathan Cameron
2026-08-13 11:33 ` [PATCH v3 6/9] firmware: arm_scmi: Refactor protocol device creation logic Sudeep Holla
` (3 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2026-08-13 11:33 UTC (permalink / raw)
To: arm-scmi, linux-arm-kernel, kernel-team; +Cc: Cristian Marussi, Breno Leitao
Extend the SCMI transport_ops chan_available() callback to include the
protocol ID (prot_id) as an argument. This allows transports to determine
channel availability based on the specific protocol being used, improving
flexibility in platforms that share transport channels across multiple
protocols. This will be useful when ACPI PCC transport gets added.
Updated all existing users and definitions of chan_available() in
SCMI core and transport drivers (mailbox, optee, etc.) accordingly.
No functional change.
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
drivers/firmware/arm_scmi/common.h | 3 ++-
drivers/firmware/arm_scmi/driver.c | 2 +-
drivers/firmware/arm_scmi/transports/mailbox.c | 3 ++-
drivers/firmware/arm_scmi/transports/optee.c | 3 ++-
drivers/firmware/arm_scmi/transports/smc.c | 3 ++-
drivers/firmware/arm_scmi/transports/virtio.c | 3 ++-
6 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 042867c28b88..1ab4543e0f4a 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -207,7 +207,8 @@ struct scmi_chan_info {
* @poll_done: Callback to poll transfer status
*/
struct scmi_transport_ops {
- bool (*chan_available)(struct fwnode_handle *fwnode, int idx);
+ bool (*chan_available)(struct fwnode_handle *fwnode, int prot_id,
+ int idx);
int (*chan_setup)(struct scmi_chan_info *cinfo, struct device *dev,
bool tx);
int (*chan_free)(int id, void *p, void *data);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 9ad827c6a9ab..aad678db0f6e 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2768,7 +2768,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct fwnode_handle *fwnode,
if (idr_find(idr, prot_id))
return -EEXIST;
- if (!info->desc->ops->chan_available(fwnode, idx)) {
+ if (!info->desc->ops->chan_available(fwnode, prot_id, idx)) {
cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
return -EINVAL;
diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
index b692cae4ff25..56af2cb0f424 100644
--- a/drivers/firmware/arm_scmi/transports/mailbox.c
+++ b/drivers/firmware/arm_scmi/transports/mailbox.c
@@ -77,7 +77,8 @@ static void rx_callback(struct mbox_client *cl, void *m)
core->shmem->read_header(smbox->shmem), NULL);
}
-static bool mailbox_chan_available(struct fwnode_handle *fwnode, int idx)
+static bool
+mailbox_chan_available(struct fwnode_handle *fwnode, int prot_id, int idx)
{
int num_mb;
struct device_node *of_node = to_of_node(fwnode);
diff --git a/drivers/firmware/arm_scmi/transports/optee.c b/drivers/firmware/arm_scmi/transports/optee.c
index 07d3affa0964..6022f74d28f3 100644
--- a/drivers/firmware/arm_scmi/transports/optee.c
+++ b/drivers/firmware/arm_scmi/transports/optee.c
@@ -314,7 +314,8 @@ static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t
return 0;
}
-static bool scmi_optee_chan_available(struct fwnode_handle *fwnode, int idx)
+static bool
+scmi_optee_chan_available(struct fwnode_handle *fwnode, int prot_id, int idx)
{
u32 channel_id;
struct device_node *of_node = to_of_node(fwnode);
diff --git a/drivers/firmware/arm_scmi/transports/smc.c b/drivers/firmware/arm_scmi/transports/smc.c
index 1079cd01190b..ee9e44468c1e 100644
--- a/drivers/firmware/arm_scmi/transports/smc.c
+++ b/drivers/firmware/arm_scmi/transports/smc.c
@@ -84,7 +84,8 @@ static irqreturn_t smc_msg_done_isr(int irq, void *data)
return IRQ_HANDLED;
}
-static bool smc_chan_available(struct fwnode_handle *fwnode, int idx)
+static bool
+smc_chan_available(struct fwnode_handle *fwnode, int prot_id, int idx)
{
struct device_node *of_node = to_of_node(fwnode);
struct device_node *np __free(device_node) = NULL;
diff --git a/drivers/firmware/arm_scmi/transports/virtio.c b/drivers/firmware/arm_scmi/transports/virtio.c
index 6b060d61e0ca..3d608c8c4fe8 100644
--- a/drivers/firmware/arm_scmi/transports/virtio.c
+++ b/drivers/firmware/arm_scmi/transports/virtio.c
@@ -375,7 +375,8 @@ static unsigned int virtio_get_max_msg(struct scmi_chan_info *base_cinfo)
return vioch->max_msg;
}
-static bool virtio_chan_available(struct fwnode_handle *fwnode, int idx)
+static bool
+virtio_chan_available(struct fwnode_handle *fwnode, int prot_id, int idx)
{
struct scmi_vio_channel *channels, *vioch = NULL;
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 5/9] firmware: arm_scmi: Pass protocol ID to chan_available() transport callback
2026-08-13 11:33 ` [PATCH v3 5/9] firmware: arm_scmi: Pass protocol ID to chan_available() transport callback Sudeep Holla
@ 2026-08-24 17:45 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2026-08-24 17:45 UTC (permalink / raw)
To: Sudeep Holla
Cc: arm-scmi, linux-arm-kernel, kernel-team, Cristian Marussi,
Breno Leitao
On Thu, 13 Aug 2026 12:33:00 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:
> Extend the SCMI transport_ops chan_available() callback to include the
> protocol ID (prot_id) as an argument. This allows transports to determine
> channel availability based on the specific protocol being used, improving
> flexibility in platforms that share transport channels across multiple
> protocols. This will be useful when ACPI PCC transport gets added.
>
> Updated all existing users and definitions of chan_available() in
> SCMI core and transport drivers (mailbox, optee, etc.) accordingly.
>
> No functional change.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> ---
> drivers/firmware/arm_scmi/common.h | 3 ++-
> drivers/firmware/arm_scmi/driver.c | 2 +-
> drivers/firmware/arm_scmi/transports/mailbox.c | 3 ++-
> drivers/firmware/arm_scmi/transports/optee.c | 3 ++-
> drivers/firmware/arm_scmi/transports/smc.c | 3 ++-
> drivers/firmware/arm_scmi/transports/virtio.c | 3 ++-
> 6 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 042867c28b88..1ab4543e0f4a 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -207,7 +207,8 @@ struct scmi_chan_info {
> * @poll_done: Callback to poll transfer status
> */
> struct scmi_transport_ops {
> - bool (*chan_available)(struct fwnode_handle *fwnode, int idx);
> + bool (*chan_available)(struct fwnode_handle *fwnode, int prot_id,
> + int idx);
> int (*chan_setup)(struct scmi_chan_info *cinfo, struct device *dev,
> bool tx);
> int (*chan_free)(int id, void *p, void *data);
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 9ad827c6a9ab..aad678db0f6e 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -2768,7 +2768,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct fwnode_handle *fwnode,
> if (idr_find(idr, prot_id))
> return -EEXIST;
>
> - if (!info->desc->ops->chan_available(fwnode, idx)) {
> + if (!info->desc->ops->chan_available(fwnode, prot_id, idx)) {
> cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
> if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
> return -EINVAL;
> diff --git a/drivers/firmware/arm_scmi/transports/mailbox.c b/drivers/firmware/arm_scmi/transports/mailbox.c
> index b692cae4ff25..56af2cb0f424 100644
> --- a/drivers/firmware/arm_scmi/transports/mailbox.c
> +++ b/drivers/firmware/arm_scmi/transports/mailbox.c
> @@ -77,7 +77,8 @@ static void rx_callback(struct mbox_client *cl, void *m)
> core->shmem->read_header(smbox->shmem), NULL);
> }
>
> -static bool mailbox_chan_available(struct fwnode_handle *fwnode, int idx)
> +static bool
> +mailbox_chan_available(struct fwnode_handle *fwnode, int prot_id, int idx)
> {
> int num_mb;
> struct device_node *of_node = to_of_node(fwnode);
> diff --git a/drivers/firmware/arm_scmi/transports/optee.c b/drivers/firmware/arm_scmi/transports/optee.c
> index 07d3affa0964..6022f74d28f3 100644
> --- a/drivers/firmware/arm_scmi/transports/optee.c
> +++ b/drivers/firmware/arm_scmi/transports/optee.c
> @@ -314,7 +314,8 @@ static int invoke_process_msg_channel(struct scmi_optee_channel *channel, size_t
> return 0;
> }
>
> -static bool scmi_optee_chan_available(struct fwnode_handle *fwnode, int idx)
> +static bool
> +scmi_optee_chan_available(struct fwnode_handle *fwnode, int prot_id, int idx)
> {
> u32 channel_id;
> struct device_node *of_node = to_of_node(fwnode);
> diff --git a/drivers/firmware/arm_scmi/transports/smc.c b/drivers/firmware/arm_scmi/transports/smc.c
> index 1079cd01190b..ee9e44468c1e 100644
> --- a/drivers/firmware/arm_scmi/transports/smc.c
> +++ b/drivers/firmware/arm_scmi/transports/smc.c
> @@ -84,7 +84,8 @@ static irqreturn_t smc_msg_done_isr(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> -static bool smc_chan_available(struct fwnode_handle *fwnode, int idx)
> +static bool
> +smc_chan_available(struct fwnode_handle *fwnode, int prot_id, int idx)
> {
> struct device_node *of_node = to_of_node(fwnode);
> struct device_node *np __free(device_node) = NULL;
> diff --git a/drivers/firmware/arm_scmi/transports/virtio.c b/drivers/firmware/arm_scmi/transports/virtio.c
> index 6b060d61e0ca..3d608c8c4fe8 100644
> --- a/drivers/firmware/arm_scmi/transports/virtio.c
> +++ b/drivers/firmware/arm_scmi/transports/virtio.c
> @@ -375,7 +375,8 @@ static unsigned int virtio_get_max_msg(struct scmi_chan_info *base_cinfo)
> return vioch->max_msg;
> }
>
> -static bool virtio_chan_available(struct fwnode_handle *fwnode, int idx)
> +static bool
> +virtio_chan_available(struct fwnode_handle *fwnode, int prot_id, int idx)
> {
> struct scmi_vio_channel *channels, *vioch = NULL;
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 6/9] firmware: arm_scmi: Refactor protocol device creation logic
2026-08-13 11:32 [PATCH v3 0/9] firmware: arm_scmi: Refactoring and enablement of ACPI PCC transport Sudeep Holla
` (4 preceding siblings ...)
2026-08-13 11:33 ` [PATCH v3 5/9] firmware: arm_scmi: Pass protocol ID to chan_available() transport callback Sudeep Holla
@ 2026-08-13 11:33 ` Sudeep Holla
2026-08-24 17:49 ` Jonathan Cameron
2026-08-13 11:33 ` [PATCH v3 7/9] firmware: arm_scmi: Add ACPI PCC transport Sudeep Holla
` (2 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2026-08-13 11:33 UTC (permalink / raw)
To: arm-scmi, linux-arm-kernel, kernel-team; +Cc: Cristian Marussi, Breno Leitao
Move the protocol validation and device creation logic in scmi_probe()
into a reusable scmi_device_check_create() helper.
The helper centralizes checks for the protocol ID range, implementation
availability and duplicate activation before invoking
scmi_create_protocol_devices(). This preserves the existing behavior
while allowing the logic to be reused by the ACPI path, where protocol
child fwnodes are absent.
No functional change intended.
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
drivers/firmware/arm_scmi/driver.c | 60 ++++++++++++++++++++++----------------
1 file changed, 35 insertions(+), 25 deletions(-)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index aad678db0f6e..951ba3df6ba6 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -3244,6 +3244,40 @@ static void scmi_enable_matching_quirks(struct scmi_info *info)
rev->sub_vendor_id, rev->impl_ver);
}
+static void scmi_device_check_create(struct fwnode_handle *fwnode, int prot_id,
+ struct scmi_info *info)
+{
+ int ret;
+ struct device *dev = info->dev;
+ struct scmi_handle *handle = &info->handle;
+
+ if (!FIELD_FIT(MSG_PROTOCOL_ID_MASK, prot_id)) {
+ dev_err(dev, "Out of range protocol %d\n", prot_id);
+ return;
+ }
+
+ if (!scmi_is_protocol_implemented(handle, prot_id)) {
+ dev_err(dev, "SCMI protocol %d not implemented\n",
+ prot_id);
+ return;
+ }
+
+ /*
+ * Save this valid fwnode protocol descriptor amongst
+ * @active_protocols for this SCMI instance.
+ */
+ ret = idr_alloc(&info->active_protocols, fwnode,
+ prot_id, prot_id + 1, GFP_KERNEL);
+ if (ret != prot_id) {
+ dev_err(dev, "SCMI protocol %d already activated. Skip\n",
+ prot_id);
+ return;
+ }
+
+ fwnode_handle_get(fwnode);
+ scmi_create_protocol_devices(fwnode, info, prot_id, NULL);
+}
+
static int scmi_probe(struct platform_device *pdev)
{
int ret;
@@ -3372,31 +3406,7 @@ static int scmi_probe(struct platform_device *pdev)
if (fwnode_property_read_u32(child, "reg", &prot_id))
continue;
- if (!FIELD_FIT(MSG_PROTOCOL_ID_MASK, prot_id)) {
- dev_err(dev, "Out of range protocol %d\n", prot_id);
- continue;
- }
-
- if (!scmi_is_protocol_implemented(handle, prot_id)) {
- dev_err(dev, "SCMI protocol %d not implemented\n",
- prot_id);
- continue;
- }
-
- /*
- * Save this valid fwnode protocol descriptor amongst
- * @active_protocols for this SCMI instance.
- */
- ret = idr_alloc(&info->active_protocols, child,
- prot_id, prot_id + 1, GFP_KERNEL);
- if (ret != prot_id) {
- dev_err(dev, "SCMI protocol %d already activated. Skip\n",
- prot_id);
- continue;
- }
-
- fwnode_handle_get(child);
- scmi_create_protocol_devices(child, info, prot_id, NULL);
+ scmi_device_check_create(child, prot_id, info);
}
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 6/9] firmware: arm_scmi: Refactor protocol device creation logic
2026-08-13 11:33 ` [PATCH v3 6/9] firmware: arm_scmi: Refactor protocol device creation logic Sudeep Holla
@ 2026-08-24 17:49 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2026-08-24 17:49 UTC (permalink / raw)
To: Sudeep Holla
Cc: arm-scmi, linux-arm-kernel, kernel-team, Cristian Marussi,
Breno Leitao
On Thu, 13 Aug 2026 12:33:01 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:
> Move the protocol validation and device creation logic in scmi_probe()
> into a reusable scmi_device_check_create() helper.
>
> The helper centralizes checks for the protocol ID range, implementation
> availability and duplicate activation before invoking
> scmi_create_protocol_devices(). This preserves the existing behavior
> while allowing the logic to be reused by the ACPI path, where protocol
> child fwnodes are absent.
>
> No functional change intended.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
A trivial thing and one follow on from earlier. For that just be consistent
with what you choose to do for the earlier comment.
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> ---
> drivers/firmware/arm_scmi/driver.c | 60 ++++++++++++++++++++++----------------
> 1 file changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index aad678db0f6e..951ba3df6ba6 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -3244,6 +3244,40 @@ static void scmi_enable_matching_quirks(struct scmi_info *info)
> rev->sub_vendor_id, rev->impl_ver);
> }
>
> +static void scmi_device_check_create(struct fwnode_handle *fwnode, int prot_id,
> + struct scmi_info *info)
> +{
> + int ret;
> + struct device *dev = info->dev;
> + struct scmi_handle *handle = &info->handle;
> +
> + if (!FIELD_FIT(MSG_PROTOCOL_ID_MASK, prot_id)) {
> + dev_err(dev, "Out of range protocol %d\n", prot_id);
> + return;
> + }
> +
> + if (!scmi_is_protocol_implemented(handle, prot_id)) {
> + dev_err(dev, "SCMI protocol %d not implemented\n",
> + prot_id);
Fits on one line. I didn't check if you modify this later in the series
though so if this is churn reduction, fine to leave it as it stands.
> + return;
> + }
> +
> + /*
> + * Save this valid fwnode protocol descriptor amongst
> + * @active_protocols for this SCMI instance.
> + */
> + ret = idr_alloc(&info->active_protocols, fwnode,
> + prot_id, prot_id + 1, GFP_KERNEL);
> + if (ret != prot_id) {
> + dev_err(dev, "SCMI protocol %d already activated. Skip\n",
> + prot_id);
> + return;
> + }
> +
> + fwnode_handle_get(fwnode);
> + scmi_create_protocol_devices(fwnode, info, prot_id, NULL);
If you take the suggest I made earlier to make the hand over of ownership
slightly more obvious by using the return value of fwmonde_handle_get()
then applies here too.
> +}
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 7/9] firmware: arm_scmi: Add ACPI PCC transport
2026-08-13 11:32 [PATCH v3 0/9] firmware: arm_scmi: Refactoring and enablement of ACPI PCC transport Sudeep Holla
` (5 preceding siblings ...)
2026-08-13 11:33 ` [PATCH v3 6/9] firmware: arm_scmi: Refactor protocol device creation logic Sudeep Holla
@ 2026-08-13 11:33 ` Sudeep Holla
2026-08-24 20:17 ` Jonathan Cameron
2026-08-13 11:33 ` [PATCH v3 8/9] firmware: arm_scmi: Initialise known ACPI protocol devices and channels Sudeep Holla
2026-08-13 11:33 ` [PATCH v3 9/9] firmware: arm_scmi: Validate PCC shared memory signature Sudeep Holla
8 siblings, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2026-08-13 11:33 UTC (permalink / raw)
To: arm-scmi, linux-arm-kernel, kernel-team; +Cc: Cristian Marussi, Breno Leitao
Introduce a new SCMI transport that uses ACPI PCCT (PCC) subspaces via
the Linux PCC mailbox layer. Parse ACPI _DSD data to map protocol
associations to PCC transport UIDs. Support common and
protocol-exclusive A2P channels, plus optional common or
protocol-exclusive P2A channels for notifications.
Key points:
- new CONFIG_ARM_SCMI_TRANSPORT_PCC option
- integration with SCMI core via scmi_desc and transport ops
- response and notification fetch from PCC shared memory
- ACPI device matching and registration via the ACPI transport macro
This enables SCMI to be exercised over PCC on ACPI platforms.
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
drivers/firmware/arm_scmi/common.h | 11 +
drivers/firmware/arm_scmi/transports/Kconfig | 13 +
drivers/firmware/arm_scmi/transports/Makefile | 2 +
drivers/firmware/arm_scmi/transports/pcc.c | 791 ++++++++++++++++++++++++++
include/linux/scmi_protocol.h | 1 +
5 files changed, 818 insertions(+)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 1ab4543e0f4a..3a49ea40aea5 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -468,6 +468,17 @@ struct scmi_transport_core_operations {
const struct scmi_message_operations *msg;
};
+struct scmi_dsd_info {
+ u32 protocol_id;
+ const char *const property_name;
+};
+
+static const struct scmi_dsd_info scmi_dsd_info_list[] __maybe_unused = {
+ { SCMI_PROTOCOL_BASE, "arm-arml0001-transport-pcc"},
+ { SCMI_PROTOCOL_POWERCAP, "arm-arml0001-protocol-pcap"},
+ { SCMI_PROTOCOL_TELEMETRY, "arm-arml0001-protocol-telemetry"},
+};
+
/**
* struct scmi_transport_handle - Transport instance handle
* @supplier_get: A helper to retrieve the device descriptor, identifying the
diff --git a/drivers/firmware/arm_scmi/transports/Kconfig b/drivers/firmware/arm_scmi/transports/Kconfig
index 57eccf316e26..1054165576b3 100644
--- a/drivers/firmware/arm_scmi/transports/Kconfig
+++ b/drivers/firmware/arm_scmi/transports/Kconfig
@@ -77,6 +77,19 @@ config ARM_SCMI_TRANSPORT_OPTEE
This driver can also be built as a module. If so, the module
will be called scmi_transport_optee.
+config ARM_SCMI_TRANSPORT_PCC
+ tristate "SCMI transport based on ACPI PCC"
+ depends on PCC
+ select ARM_SCMI_HAVE_TRANSPORT
+ default y
+ help
+ Enable ACPI PCC mailbox based transport for SCMI.
+
+ If you want the ARM SCMI PROTOCOL stack to include support for a
+ transport based on mailboxes, answer Y.
+ This driver can also be built as a module. If so, the module
+ will be called scmi_transport_pcc.
+
config ARM_SCMI_TRANSPORT_VIRTIO
tristate "SCMI transport based on VirtIO"
depends on VIRTIO
diff --git a/drivers/firmware/arm_scmi/transports/Makefile b/drivers/firmware/arm_scmi/transports/Makefile
index 3ba3d3bee151..e7c4b8de6251 100644
--- a/drivers/firmware/arm_scmi/transports/Makefile
+++ b/drivers/firmware/arm_scmi/transports/Makefile
@@ -7,6 +7,8 @@ scmi_transport_mailbox-objs := mailbox.o
obj-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += scmi_transport_mailbox.o
scmi_transport_optee-objs := optee.o
obj-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += scmi_transport_optee.o
+scmi_transport_pcc-objs := pcc.o
+obj-$(CONFIG_ARM_SCMI_TRANSPORT_PCC) += scmi_transport_pcc.o
scmi_transport_virtio-objs := virtio.o
obj-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += scmi_transport_virtio.o
diff --git a/drivers/firmware/arm_scmi/transports/pcc.c b/drivers/firmware/arm_scmi/transports/pcc.c
new file mode 100644
index 000000000000..337d551e3ad8
--- /dev/null
+++ b/drivers/firmware/arm_scmi/transports/pcc.c
@@ -0,0 +1,791 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Message ACPI PCC
+ * Transport Driver
+ *
+ * This transport uses ACPI PCC (PCCT Type 3/4) subspaces via the Linux
+ * PCC mailbox controller to exchange SCMI messages over the standard
+ * SCMI Shared Memory Transport (SMT) layout.
+ *
+ * PCC subspace selection is conveyed via ACPI SCMI namespace device.
+ *
+ * Copyright (C) 2026
+ */
+
+#include <linux/acpi.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/hashtable.h>
+#include <linux/io.h>
+#include <linux/limits.h>
+#include <linux/list.h>
+#include <linux/mailbox_client.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <acpi/pcc.h>
+
+#include "../common.h"
+
+#define SCMI_TRANSPORT_PACKAGE_MAX_VERSION (1)
+#define SCMI_PROTOCOL_PACKAGE_MAX_VERSION (1)
+#define SCMI_TRANSPORT_SHARED_CHANNEL BIT_ULL(0)
+#define SCMI_TRANSPORT_P2A_CHANNEL BIT_ULL(1)
+#define SCMI_TRANSPORT_FLAGS_MASK (SCMI_TRANSPORT_SHARED_CHANNEL | \
+ SCMI_TRANSPORT_P2A_CHANNEL)
+#define SCMI_PCC_SHMEM_OVERHEAD (sizeof(struct pcc_shared_mem) + \
+ sizeof(u32))
+
+/*
+ * SCMI specification requires all parameters, message headers, return
+ * arguments or any protocol data to be expressed in little endian
+ * format only.
+ */
+struct pcc_shared_mem {
+ struct acpi_pcct_ext_pcc_shared_memory header;
+ u8 msg_payload[];
+};
+
+/**
+ * struct scmi_pcc - Structure representing a SCMI mailbox transport
+ *
+ * @cl: Mailbox Client
+ * @pchan: Transmit/Receive PCC/mailbox channel
+ * @cinfo: SCMI channel info
+ */
+struct scmi_pcc {
+ struct mbox_client cl;
+ struct pcc_mbox_chan *pchan;
+ struct scmi_chan_info *cinfo;
+};
+
+struct pcc_transport {
+ u32 uid;
+ u32 pcc_ss_id;
+ u32 protocol_id;
+ u32 flags;
+ struct hlist_node hnode;
+};
+
+struct pcc_transport_map {
+ struct fwnode_handle *fwnode;
+ struct list_head node;
+ DECLARE_HASHTABLE(table, ilog2(MAX_PCC_SUBSPACES / 8));
+};
+
+#define client_to_scmi_pcc(c) container_of(c, struct scmi_pcc, cl)
+
+static struct scmi_transport_core_operations *core;
+static LIST_HEAD(pcc_transport_maps);
+static DEFINE_MUTEX(pcc_transport_maps_lock);
+
+static void acpi_scmi_clear_transport_map(struct pcc_transport_map *map)
+{
+ struct pcc_transport *p;
+ struct hlist_node *tmp;
+ int idx;
+
+ hash_for_each_safe(map->table, idx, tmp, p, hnode) {
+ hash_del(&p->hnode);
+ kfree(p);
+ }
+}
+
+static void acpi_scmi_destroy_transport_map(struct pcc_transport_map *map)
+{
+ acpi_scmi_clear_transport_map(map);
+ if (map->fwnode)
+ fwnode_handle_put(map->fwnode);
+ kfree(map);
+}
+
+static void acpi_scmi_clear_transport_maps(void)
+{
+ struct pcc_transport_map *map, *tmp;
+
+ list_for_each_entry_safe(map, tmp, &pcc_transport_maps, node) {
+ list_del(&map->node);
+ acpi_scmi_destroy_transport_map(map);
+ }
+}
+
+static const union acpi_object *
+acpi_scmi_pkg_elements(const union acpi_object *obj, unsigned int count)
+{
+ if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != count)
+ return NULL;
+
+ return obj->package.elements;
+}
+
+static int
+acpi_scmi_pkg_u64(const union acpi_object *pkg, unsigned int idx, u64 *val)
+{
+ const union acpi_object *elem = &pkg->package.elements[idx];
+
+ if (elem->type != ACPI_TYPE_INTEGER)
+ return -EINVAL;
+
+ *val = elem->integer.value;
+
+ return 0;
+}
+
+static int
+acpi_scmi_pkg_u32(const union acpi_object *pkg, unsigned int idx, u32 *val)
+{
+ u64 tmp;
+ int ret;
+
+ ret = acpi_scmi_pkg_u64(pkg, idx, &tmp);
+ if (ret)
+ return ret;
+ if (tmp > U32_MAX)
+ return -EINVAL;
+
+ *val = tmp;
+
+ return 0;
+}
+
+static int acpi_scmi_property(const union acpi_object *properties,
+ unsigned int idx, const char **name,
+ const union acpi_object **value)
+{
+ const union acpi_object *property;
+ const union acpi_object *elems;
+
+ property = &properties->package.elements[idx];
+ elems = acpi_scmi_pkg_elements(property, 2);
+ if (!elems)
+ return -EINVAL;
+ if (elems[0].type != ACPI_TYPE_STRING || !elems[0].string.pointer)
+ return -EINVAL;
+
+ *name = elems[0].string.pointer;
+ *value = &elems[1];
+
+ return 0;
+}
+
+static int
+acpi_scmi_dsd_parse_transport_package(struct pcc_transport_map *map,
+ const union acpi_object *obj)
+{
+ const union acpi_object *elems;
+ u32 revision, pkg_cnt;
+ unsigned int common_a2p = 0, common_p2a = 0;
+ int idx;
+
+ if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2 ||
+ acpi_scmi_pkg_u32(obj, 0, &revision) ||
+ acpi_scmi_pkg_u32(obj, 1, &pkg_cnt))
+ return -EINVAL;
+ if (revision != SCMI_TRANSPORT_PACKAGE_MAX_VERSION)
+ return -EINVAL;
+ if (obj->package.count != pkg_cnt + 2)
+ return -EINVAL;
+
+ for (idx = 0; idx < pkg_cnt; idx++) {
+ union acpi_object *pack = &obj->package.elements[idx + 2];
+ struct pcc_transport *p, *tmp;
+ u32 pcc_ss_id, uid;
+ u64 flags;
+
+ elems = acpi_scmi_pkg_elements(pack, 3);
+ if (!elems) {
+ pr_info("Invalid transport properties pkg %d\n", idx);
+ return -EINVAL;
+ }
+ if (acpi_scmi_pkg_u32(pack, 0, &pcc_ss_id) ||
+ acpi_scmi_pkg_u32(pack, 1, &uid) ||
+ acpi_scmi_pkg_u64(pack, 2, &flags))
+ return -EINVAL;
+ if (flags & ~SCMI_TRANSPORT_FLAGS_MASK)
+ return -EINVAL;
+
+ hash_for_each_possible(map->table, tmp, hnode, uid) {
+ if (tmp->uid == uid) {
+ pr_info("Duplicate UID %d\n", uid);
+ return -EEXIST;
+ }
+ }
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+ if (!p)
+ return -ENOMEM;
+
+ p->uid = uid;
+ p->pcc_ss_id = pcc_ss_id;
+ p->flags = flags;
+ if (p->flags & SCMI_TRANSPORT_SHARED_CHANNEL) {
+ p->protocol_id = SCMI_PROTOCOL_BASE;
+ if (p->flags & SCMI_TRANSPORT_P2A_CHANNEL)
+ common_p2a++;
+ else
+ common_a2p++;
+ }
+
+ hash_add(map->table, &p->hnode, uid);
+ }
+
+ if (common_a2p != 1 || common_p2a > 1)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int
+acpi_scmi_dsd_parse_protocol_subpackage(struct pcc_transport_map *map,
+ const union acpi_object *obj,
+ int prot_id)
+{
+ bool found, tx_found = false, rx_found = false;
+ u32 uid;
+ int idx, ret = 0;
+ struct pcc_transport *p;
+ unsigned int pkg_cnt = obj->package.count;
+
+ if (pkg_cnt > 2) {
+ pr_warn("Only 2 channels: one Tx and one Rx needed\n");
+ return -EINVAL;
+ }
+
+ for (idx = 0; idx < pkg_cnt; idx++) {
+ union acpi_object *pack = &obj->package.elements[idx];
+ u64 flags;
+
+ if (!acpi_scmi_pkg_elements(pack, 2) ||
+ acpi_scmi_pkg_u32(pack, 0, &uid) ||
+ acpi_scmi_pkg_u64(pack, 1, &flags))
+ return -EINVAL;
+ if (flags)
+ return -EINVAL;
+
+ found = false;
+ hash_for_each_possible(map->table, p, hnode, uid) {
+ if (p->uid != uid)
+ continue;
+
+ found = true;
+ if (p->flags & SCMI_TRANSPORT_SHARED_CHANNEL) {
+ pr_info("Invalid! %d channel is shared\n",
+ p->pcc_ss_id);
+ ret = -EINVAL;
+ break;
+ }
+ if (p->protocol_id && p->protocol_id != prot_id)
+ return -EINVAL;
+
+ if (p->flags & SCMI_TRANSPORT_P2A_CHANNEL) {
+ if (rx_found)
+ return -EINVAL;
+ rx_found = true;
+ } else {
+ if (tx_found)
+ return -EINVAL;
+ tx_found = true;
+ }
+ p->protocol_id = prot_id;
+ break;
+ }
+
+ if (ret)
+ return ret;
+ if (!found)
+ return -ENOENT;
+ }
+
+ return ret;
+}
+
+static int
+acpi_scmi_dsd_parse_protocol_package(struct pcc_transport_map *map,
+ const union acpi_object *obj, int prot_id)
+{
+ const union acpi_object *elems;
+ const union acpi_object *pack;
+ u32 revision;
+ int ret;
+
+ elems = acpi_scmi_pkg_elements(obj, 3);
+ if (!elems || acpi_scmi_pkg_u32(obj, 0, &revision))
+ return -EINVAL;
+
+ pack = &elems[1];
+
+ if (revision != SCMI_PROTOCOL_PACKAGE_MAX_VERSION)
+ return -EINVAL;
+
+ if (pack->type != ACPI_TYPE_PACKAGE) {
+ pr_info("Invalid protocol transport package\n");
+ return -EINVAL;
+ }
+
+ /* Empty protocol specific transport package allowed */
+ if (pack->package.count != 0) {
+ ret = acpi_scmi_dsd_parse_protocol_subpackage(map, pack, prot_id);
+ if (ret)
+ return ret;
+ }
+
+ pack = &elems[2];
+ if (pack->type != ACPI_TYPE_PACKAGE) {
+ pr_info("Invalid protocol transport association package\n");
+ return -EINVAL;
+ }
+
+ if (pack->package.count != 0) {
+ pr_info("Non-empty association package not supported\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/* ACPI SCMI _DSD UUID: "84a2d1c6-86b6-4199-8dac-9c17449d5e03" */
+static const guid_t acpi_scmi_uuid = GUID_INIT(0x84a2d1c6, 0x86b6, 0x4199,
+ 0x8d, 0xac, 0x9c, 0x17,
+ 0x44, 0x9d, 0x5e, 0x03);
+
+static int acpi_scmi_lookup_protocol_id(const char *name)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(scmi_dsd_info_list); i++) {
+ if (!strcmp(name, scmi_dsd_info_list[i].property_name))
+ return scmi_dsd_info_list[i].protocol_id;
+ }
+ return -ENOENT;
+}
+
+static int acpi_scmi_parse_properties(struct pcc_transport_map *map,
+ const union acpi_object *properties)
+{
+ bool transport_found = false;
+ int i;
+
+ if (properties->type != ACPI_TYPE_PACKAGE)
+ return -EINVAL;
+
+ for (i = 0; i < properties->package.count; i++) {
+ const union acpi_object *v;
+ const char *name;
+ int prot_id, ret;
+
+ ret = acpi_scmi_property(properties, i, &name, &v);
+ if (ret)
+ return ret;
+
+ prot_id = acpi_scmi_lookup_protocol_id(name);
+ if (prot_id < 0)
+ continue;
+ if (prot_id != SCMI_PROTOCOL_BASE)
+ continue;
+ if (v->type != ACPI_TYPE_PACKAGE)
+ return -EINVAL;
+ if (transport_found)
+ return -EEXIST;
+
+ ret = acpi_scmi_dsd_parse_transport_package(map, v);
+ if (ret)
+ return ret;
+ transport_found = true;
+ }
+
+ if (!transport_found)
+ return -ENOENT;
+
+ for (i = 0; i < properties->package.count; i++) {
+ const union acpi_object *v;
+ const char *name;
+ int prot_id, ret;
+
+ ret = acpi_scmi_property(properties, i, &name, &v);
+ if (ret)
+ return ret;
+
+ prot_id = acpi_scmi_lookup_protocol_id(name);
+ if (prot_id < 0 || prot_id == SCMI_PROTOCOL_BASE)
+ continue;
+ if (v->type != ACPI_TYPE_PACKAGE)
+ return -EINVAL;
+
+ ret = acpi_scmi_dsd_parse_protocol_package(map, v, prot_id);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int acpi_scmi_namespace_fwnode_parse(struct fwnode_handle *fwnode,
+ struct pcc_transport_map *map)
+{
+ struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
+ struct acpi_device *adev = to_acpi_device_node(fwnode);
+ union acpi_object *desc;
+ acpi_status status;
+ int i, ret = -ENOENT;
+
+ if (!adev->handle)
+ return -EINVAL;
+
+ status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL, &buf,
+ ACPI_TYPE_PACKAGE);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+
+ desc = buf.pointer;
+ if (desc->package.count % 2)
+ goto out_free_inval;
+
+ /* Look for the device properties GUID. */
+ for (i = 0; i < desc->package.count; i += 2) {
+ const union acpi_object *guid;
+ const union acpi_object *properties;
+
+ guid = &desc->package.elements[i];
+ properties = &desc->package.elements[i + 1];
+
+ /*
+ * The first element must be a GUID and the second one must be
+ * a package.
+ */
+ if (guid->type != ACPI_TYPE_BUFFER ||
+ guid->buffer.length != UUID_SIZE ||
+ properties->type != ACPI_TYPE_PACKAGE)
+ continue;
+
+ if (!guid_equal((guid_t *)guid->buffer.pointer,
+ &acpi_scmi_uuid))
+ continue;
+
+ ret = acpi_scmi_parse_properties(map, properties);
+ goto out_free;
+ }
+
+out_free:
+ ACPI_FREE(buf.pointer);
+ return ret;
+out_free_inval:
+ ret = -EINVAL;
+ goto out_free;
+}
+
+static bool
+pcc_transport_map_has_ss_id(struct pcc_transport_map *map, u32 pcc_ss_id)
+{
+ struct pcc_transport *p;
+ int idx;
+
+ hash_for_each(map->table, idx, p, hnode) {
+ if (p->pcc_ss_id == pcc_ss_id)
+ return true;
+ }
+
+ return false;
+}
+
+static int pcc_transport_map_validate(struct pcc_transport_map *map)
+{
+ struct pcc_transport_map *iter;
+ struct pcc_transport *p;
+ int idx;
+
+ list_for_each_entry(iter, &pcc_transport_maps, node) {
+ hash_for_each(map->table, idx, p, hnode) {
+ if (pcc_transport_map_has_ss_id(iter, p->pcc_ss_id))
+ return -EEXIST;
+ }
+ }
+
+ return 0;
+}
+
+static
+struct pcc_transport_map *pcc_transport_map_find(struct fwnode_handle *fwnode)
+{
+ struct pcc_transport_map *map;
+
+ list_for_each_entry(map, &pcc_transport_maps, node) {
+ if (map->fwnode == fwnode)
+ return map;
+ }
+
+ return NULL;
+}
+
+static
+struct pcc_transport_map *pcc_transport_map_get(struct fwnode_handle *fwnode)
+{
+ struct pcc_transport_map *map;
+ int ret;
+
+ map = pcc_transport_map_find(fwnode);
+ if (map)
+ return map;
+
+ map = kzalloc_obj(*map, GFP_KERNEL);
+ if (!map)
+ return ERR_PTR(-ENOMEM);
+
+ hash_init(map->table);
+ ret = acpi_scmi_namespace_fwnode_parse(fwnode, map);
+ if (ret)
+ goto err_free_map;
+
+ ret = pcc_transport_map_validate(map);
+ if (ret)
+ goto err_free_map;
+
+ map->fwnode = fwnode_handle_get(fwnode);
+ list_add_tail(&map->node, &pcc_transport_maps);
+
+ return map;
+
+err_free_map:
+ acpi_scmi_destroy_transport_map(map);
+ return ERR_PTR(ret);
+}
+
+static int pcc_lookup_ss_id(struct pcc_transport_map *map, u32 prot_id, bool tx)
+{
+ struct pcc_transport *p;
+ int idx;
+
+ hash_for_each(map->table, idx, p, hnode) {
+ if (p->protocol_id != prot_id)
+ continue;
+
+ if ((!tx && (p->flags & SCMI_TRANSPORT_P2A_CHANNEL)) ||
+ (tx && !(p->flags & SCMI_TRANSPORT_P2A_CHANNEL)))
+ return p->pcc_ss_id;
+ }
+
+ return -ENOENT;
+}
+
+static int pcc_get_ss_id(struct fwnode_handle *fwnode, u32 prot_id, bool tx)
+{
+ struct pcc_transport_map *map;
+ int ret;
+
+ if (!fwnode)
+ return -EINVAL;
+
+ mutex_lock(&pcc_transport_maps_lock);
+ map = pcc_transport_map_get(fwnode);
+ if (IS_ERR(map))
+ ret = PTR_ERR(map);
+ else
+ ret = pcc_lookup_ss_id(map, prot_id, tx);
+ mutex_unlock(&pcc_transport_maps_lock);
+
+ return ret;
+}
+
+static bool
+pcc_chan_available(struct fwnode_handle *fwnode, int prot_id, int idx)
+{
+ if (pcc_get_ss_id(fwnode, prot_id, idx ? false : true) < 0)
+ return false;
+
+ return true;
+}
+
+static void tx_prepare(struct mbox_client *cl, void *m)
+{
+ struct scmi_pcc *smbox = client_to_scmi_pcc(cl);
+ struct pcc_shared_mem __iomem *shmem = smbox->pchan->shmem;
+ struct scmi_xfer *xfer = m;
+
+ /*
+ * PCC take cares not to call tx_prepare until last transmit is done.
+ * Request platform interrupt notification if available.
+ */
+ iowrite32(smbox->pchan->mchan->mbox->txdone_irq ?
+ PCC_CMD_COMPLETION_NOTIFY : 0, &shmem->header.flags);
+ iowrite32(sizeof(shmem->header.command) + xfer->tx.len,
+ &shmem->header.length);
+ iowrite32(pack_scmi_header(&xfer->hdr), &shmem->header.command);
+ if (xfer->tx.buf)
+ memcpy_toio(shmem->msg_payload, xfer->tx.buf, xfer->tx.len);
+}
+
+static void rx_callback(struct mbox_client *cl, void *m)
+{
+ struct scmi_pcc *smbox = client_to_scmi_pcc(cl);
+ struct pcc_shared_mem __iomem *shmem = smbox->pchan->shmem;
+
+ core->rx_callback(smbox->cinfo, ioread32(&shmem->header.command), NULL);
+}
+
+static int pcc_chan_validate_shmem(struct scmi_chan_info *cinfo,
+ struct scmi_pcc *smbox)
+{
+ if (smbox->pchan->shmem_size < SCMI_PCC_SHMEM_OVERHEAD ||
+ smbox->pchan->shmem_size - SCMI_PCC_SHMEM_OVERHEAD <
+ cinfo->max_msg_size) {
+ dev_err(cinfo->dev, "misconfigured SCMI PCC shared memory\n");
+ return -ENOSPC;
+ }
+
+ return 0;
+}
+
+static int pcc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
+ bool tx)
+{
+ const char *desc = tx ? "Tx" : "Rx";
+ struct device *cdev = cinfo->dev;
+ struct scmi_pcc *smbox;
+ int ret, ss_id;
+ struct mbox_client *cl;
+
+ smbox = devm_kzalloc(dev, sizeof(*smbox), GFP_KERNEL);
+ if (!smbox)
+ return -ENOMEM;
+
+ cl = &smbox->cl;
+ cl->dev = cdev;
+ cl->tx_prepare = tx ? tx_prepare : NULL;
+ cl->rx_callback = rx_callback;
+ cl->tx_block = false;
+
+ ss_id = pcc_get_ss_id(dev_fwnode(cinfo->dev), cinfo->id, tx);
+ if (ss_id < 0)
+ return ss_id;
+
+ smbox->pchan = pcc_mbox_request_channel(cl, ss_id);
+ if (IS_ERR(smbox->pchan)) {
+ ret = PTR_ERR(smbox->pchan);
+ if (ret != -EPROBE_DEFER)
+ dev_err(cdev,
+ "failed to request SCMI %s mailbox\n", desc);
+ return ret;
+ }
+
+ ret = pcc_chan_validate_shmem(cinfo, smbox);
+ if (ret) {
+ pcc_mbox_free_channel(smbox->pchan);
+ return ret;
+ }
+
+ cinfo->transport_info = smbox;
+ smbox->cinfo = cinfo;
+
+ return 0;
+}
+
+static int pcc_chan_free(int id, void *p, void *data)
+{
+ struct scmi_chan_info *cinfo = p;
+ struct scmi_pcc *smbox = cinfo->transport_info;
+
+ if (smbox && !IS_ERR(smbox->pchan)) {
+ pcc_mbox_free_channel(smbox->pchan);
+ cinfo->transport_info = NULL;
+ smbox->pchan = NULL;
+ smbox->cinfo = NULL;
+ }
+
+ return 0;
+}
+
+static int
+pcc_send_message(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
+{
+ struct scmi_pcc *smbox = cinfo->transport_info;
+ int ret;
+
+ /*
+ * The mailbox layer has its own queue. However the mailbox queue
+ * confuses the per message SCMI timeouts since the clock starts when
+ * the message is submitted into the mailbox queue. So when multiple
+ * messages are queued up the clock starts on all messages instead of
+ * only the one inflight.
+ */
+ ret = mbox_send_message(smbox->pchan->mchan, xfer);
+ /* mbox_send_message returns non-negative value on success */
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static void pcc_fetch_response(struct scmi_chan_info *cinfo,
+ struct scmi_xfer *xfer)
+{
+ struct scmi_pcc *smbox = cinfo->transport_info;
+ struct pcc_shared_mem __iomem *shmem = smbox->pchan->shmem;
+ size_t len = ioread32(&shmem->header.length);
+
+ xfer->hdr.status = ioread32(shmem->msg_payload);
+ /* Skip the length of header and status in shmem area i.e 8 bytes */
+ xfer->rx.len = min_t(size_t, xfer->rx.len, len > 8 ? len - 8 : 0);
+
+ /* Take a copy to the rx buffer.. */
+ memcpy_fromio(xfer->rx.buf, shmem->msg_payload + 4, xfer->rx.len);
+}
+
+static void pcc_fetch_notification(struct scmi_chan_info *cinfo, size_t max_len,
+ struct scmi_xfer *xfer)
+{
+ struct scmi_pcc *smbox = cinfo->transport_info;
+ struct pcc_shared_mem __iomem *shmem = smbox->pchan->shmem;
+ size_t len = ioread32(&shmem->header.length);
+
+ /* Skip only the length of header in shmem area i.e 4 bytes */
+ xfer->rx.len = min_t(size_t, max_len, len > 4 ? len - 4 : 0);
+
+ /* Take a copy to the rx buffer.. */
+ memcpy_fromio(xfer->rx.buf, shmem->msg_payload, xfer->rx.len);
+}
+
+static const struct scmi_transport_ops scmi_pcc_ops = {
+ .chan_available = pcc_chan_available,
+ .chan_setup = pcc_chan_setup,
+ .chan_free = pcc_chan_free,
+ .send_message = pcc_send_message,
+ .fetch_response = pcc_fetch_response,
+ .fetch_notification = pcc_fetch_notification,
+};
+
+static struct scmi_desc scmi_pcc_desc = {
+ .ops = &scmi_pcc_ops,
+ .max_rx_timeout_ms = 30, /* We may increase this if required */
+ .max_msg = 20, /* Limited by MBOX_TX_QUEUE_LEN */
+ .max_msg_size = SCMI_SHMEM_MAX_PAYLOAD_SIZE - 12,
+};
+
+static const struct acpi_device_id scmi_acpi_ids[] = {
+ { "ARML0001", 0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(acpi, scmi_acpi_ids);
+
+DEFINE_SCMI_ACPI_TRANSPORT_DRIVER(scmi_pcc, scmi_pcc_driver,
+ scmi_pcc_desc, scmi_acpi_ids, core);
+
+static int __init scmi_pcc_init(void)
+{
+ return platform_driver_register(&scmi_pcc_driver);
+}
+
+static void __exit scmi_pcc_exit(void)
+{
+ platform_driver_unregister(&scmi_pcc_driver);
+
+ mutex_lock(&pcc_transport_maps_lock);
+ acpi_scmi_clear_transport_maps();
+ mutex_unlock(&pcc_transport_maps_lock);
+}
+module_init(scmi_pcc_init);
+module_exit(scmi_pcc_exit);
+
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@kernel.org>");
+MODULE_DESCRIPTION("SCMI ACPI PCC Transport driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 5ab73b1ab9aa..02cf04543151 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -930,6 +930,7 @@ enum scmi_std_protocol {
SCMI_PROTOCOL_VOLTAGE = 0x17,
SCMI_PROTOCOL_POWERCAP = 0x18,
SCMI_PROTOCOL_PINCTRL = 0x19,
+ SCMI_PROTOCOL_TELEMETRY = 0x1B,
};
enum scmi_system_events {
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 7/9] firmware: arm_scmi: Add ACPI PCC transport
2026-08-13 11:33 ` [PATCH v3 7/9] firmware: arm_scmi: Add ACPI PCC transport Sudeep Holla
@ 2026-08-24 20:17 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2026-08-24 20:17 UTC (permalink / raw)
To: Sudeep Holla
Cc: arm-scmi, linux-arm-kernel, kernel-team, Cristian Marussi,
Breno Leitao
On Thu, 13 Aug 2026 12:33:02 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:
> Introduce a new SCMI transport that uses ACPI PCCT (PCC) subspaces via
> the Linux PCC mailbox layer. Parse ACPI _DSD data to map protocol
> associations to PCC transport UIDs. Support common and
> protocol-exclusive A2P channels, plus optional common or
> protocol-exclusive P2A channels for notifications.
>
> Key points:
> - new CONFIG_ARM_SCMI_TRANSPORT_PCC option
> - integration with SCMI core via scmi_desc and transport ops
> - response and notification fetch from PCC shared memory
> - ACPI device matching and registration via the ACPI transport macro
>
> This enables SCMI to be exercised over PCC on ACPI platforms.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
Hi Sudeep
This is quite dense and ACPI parsing code is always 'interesting'
Anyhow some comments inline
Jonathan
> ---
> drivers/firmware/arm_scmi/common.h | 11 +
> drivers/firmware/arm_scmi/transports/Kconfig | 13 +
> drivers/firmware/arm_scmi/transports/Makefile | 2 +
> drivers/firmware/arm_scmi/transports/pcc.c | 791 ++++++++++++++++++++++++++
> include/linux/scmi_protocol.h | 1 +
> 5 files changed, 818 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 1ab4543e0f4a..3a49ea40aea5 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -468,6 +468,17 @@ struct scmi_transport_core_operations {
> const struct scmi_message_operations *msg;
> };
>
> +struct scmi_dsd_info {
> + u32 protocol_id;
> + const char *const property_name;
> +};
> +
> +static const struct scmi_dsd_info scmi_dsd_info_list[] __maybe_unused = {
> + { SCMI_PROTOCOL_BASE, "arm-arml0001-transport-pcc"},
For symmetry needs a space before }
> + { SCMI_PROTOCOL_POWERCAP, "arm-arml0001-protocol-pcap"},
> + { SCMI_PROTOCOL_TELEMETRY, "arm-arml0001-protocol-telemetry"},
I guess it is trivial but I'd have been tempted to add the transport first
then follow up with the new protocol as a separate patch.
> +};
> +
> /**
> * struct scmi_transport_handle - Transport instance handle
> * @supplier_get: A helper to retrieve the device descriptor, identifying the
> diff --git a/drivers/firmware/arm_scmi/transports/Kconfig b/drivers/firmware/arm_scmi/transports/Kconfig
> index 57eccf316e26..1054165576b3 100644
> --- a/drivers/firmware/arm_scmi/transports/Kconfig
> +++ b/drivers/firmware/arm_scmi/transports/Kconfig
> @@ -77,6 +77,19 @@ config ARM_SCMI_TRANSPORT_OPTEE
> This driver can also be built as a module. If so, the module
> will be called scmi_transport_optee.
>
> +config ARM_SCMI_TRANSPORT_PCC
> + tristate "SCMI transport based on ACPI PCC"
> + depends on PCC
> + select ARM_SCMI_HAVE_TRANSPORT
> + default y
We almost never do default y except when papering over new symbols for things
that were always built before. Why is it appropriate here?
> + help
> + Enable ACPI PCC mailbox based transport for SCMI.
> +
> + If you want the ARM SCMI PROTOCOL stack to include support for a
> + transport based on mailboxes, answer Y.
> + This driver can also be built as a module. If so, the module
> + will be called scmi_transport_pcc.
> diff --git a/drivers/firmware/arm_scmi/transports/pcc.c b/drivers/firmware/arm_scmi/transports/pcc.c
> new file mode 100644
> index 000000000000..337d551e3ad8
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/transports/pcc.c
> +/*
> + * SCMI specification requires all parameters, message headers, return
> + * arguments or any protocol data to be expressed in little endian
> + * format only.
> + */
> +struct pcc_shared_mem {
> + struct acpi_pcct_ext_pcc_shared_memory header;
> + u8 msg_payload[];
Can we do __counted_by header.length?
I'm not sure if that works or not.
> +};
...
> +
> +static int
> +acpi_scmi_dsd_parse_transport_package(struct pcc_transport_map *map,
> + const union acpi_object *obj)
> +{
> + const union acpi_object *elems;
> + u32 revision, pkg_cnt;
> + unsigned int common_a2p = 0, common_p2a = 0;
> + int idx;
> +
> + if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count < 2 ||
> + acpi_scmi_pkg_u32(obj, 0, &revision) ||
> + acpi_scmi_pkg_u32(obj, 1, &pkg_cnt))
> + return -EINVAL;
> + if (revision != SCMI_TRANSPORT_PACKAGE_MAX_VERSION)
> + return -EINVAL;
> + if (obj->package.count != pkg_cnt + 2)
> + return -EINVAL;
> +
> + for (idx = 0; idx < pkg_cnt; idx++) {
for (int idx = 0; ...
> + union acpi_object *pack = &obj->package.elements[idx + 2];
> + struct pcc_transport *p, *tmp;
> + u32 pcc_ss_id, uid;
> + u64 flags;
> +
> + elems = acpi_scmi_pkg_elements(pack, 3);
> + if (!elems) {
> + pr_info("Invalid transport properties pkg %d\n", idx);
> + return -EINVAL;
> + }
> + if (acpi_scmi_pkg_u32(pack, 0, &pcc_ss_id) ||
> + acpi_scmi_pkg_u32(pack, 1, &uid) ||
> + acpi_scmi_pkg_u64(pack, 2, &flags))
> + return -EINVAL;
> + if (flags & ~SCMI_TRANSPORT_FLAGS_MASK)
> + return -EINVAL;
> +
> + hash_for_each_possible(map->table, tmp, hnode, uid) {
> + if (tmp->uid == uid) {
> + pr_info("Duplicate UID %d\n", uid);
> + return -EEXIST;
> + }
> + }
> +
> + p = kzalloc(sizeof(*p), GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
> +
> + p->uid = uid;
> + p->pcc_ss_id = pcc_ss_id;
> + p->flags = flags;
> + if (p->flags & SCMI_TRANSPORT_SHARED_CHANNEL) {
> + p->protocol_id = SCMI_PROTOCOL_BASE;
> + if (p->flags & SCMI_TRANSPORT_P2A_CHANNEL)
> + common_p2a++;
> + else
> + common_a2p++;
> + }
> +
> + hash_add(map->table, &p->hnode, uid);
> + }
> +
> + if (common_a2p != 1 || common_p2a > 1)
> + return -EINVAL;
If you are just going to fail on larger counts, why not do it earlier
as you do in some of the other similar functions when a repeat is seen?
If they need to be in the hash table anyway add a comment.
> +
> + return 0;
> +}
> +
> +static int
> +acpi_scmi_dsd_parse_protocol_subpackage(struct pcc_transport_map *map,
> + const union acpi_object *obj,
> + int prot_id)
> +{
> + bool found, tx_found = false, rx_found = false;
> + u32 uid;
> + int idx, ret = 0;
> + struct pcc_transport *p;
> + unsigned int pkg_cnt = obj->package.count;
Not sure if you've standardized on an ordering I can't spot for declarations.
If not pick one for the whole file.
> +
> + if (pkg_cnt > 2) {
> + pr_warn("Only 2 channels: one Tx and one Rx needed\n");
Not sure that's helpful. "%u channels found, only 2 needed ...
> + return -EINVAL;
> + }
> +
for (u32 idx = 0; ...
> + for (idx = 0; idx < pkg_cnt; idx++) {
> + union acpi_object *pack = &obj->package.elements[idx];
> + u64 flags;
> +
> + if (!acpi_scmi_pkg_elements(pack, 2) ||
figure out how to avoid those magic 2s.
> + acpi_scmi_pkg_u32(pack, 0, &uid) ||
> + acpi_scmi_pkg_u64(pack, 1, &flags))
> + return -EINVAL;
> + if (flags)
> + return -EINVAL;
> +
> + found = false;
> + hash_for_each_possible(map->table, p, hnode, uid) {
> + if (p->uid != uid)
> + continue;
> +
> + found = true;
> + if (p->flags & SCMI_TRANSPORT_SHARED_CHANNEL) {
> + pr_info("Invalid! %d channel is shared\n",
> + p->pcc_ss_id);
> + ret = -EINVAL;
> + break;
> + }
> + if (p->protocol_id && p->protocol_id != prot_id)
> + return -EINVAL;
> +
> + if (p->flags & SCMI_TRANSPORT_P2A_CHANNEL) {
> + if (rx_found)
> + return -EINVAL;
> + rx_found = true;
> + } else {
> + if (tx_found)
> + return -EINVAL;
> + tx_found = true;
> + }
> + p->protocol_id = prot_id;
> + break;
> + }
> +
> + if (ret)
> + return ret;
Might as well return above. You do in some paths already.
> + if (!found)
> + return -ENOENT;
> + }
> +
> + return ret;
Can you get here with ret != 0?
return 0 probably as this is the normal exit path.
> +}
> +
> +static int
> +acpi_scmi_dsd_parse_protocol_package(struct pcc_transport_map *map,
> + const union acpi_object *obj, int prot_id)
> +{
> + const union acpi_object *elems;
> + const union acpi_object *pack;
> + u32 revision;
> + int ret;
> +
> + elems = acpi_scmi_pkg_elements(obj, 3);
> + if (!elems || acpi_scmi_pkg_u32(obj, 0, &revision))
> + return -EINVAL;
> +
> + pack = &elems[1];
> +
> + if (revision != SCMI_PROTOCOL_PACKAGE_MAX_VERSION)
> + return -EINVAL;
> +
> + if (pack->type != ACPI_TYPE_PACKAGE) {
> + pr_info("Invalid protocol transport package\n");
> + return -EINVAL;
> + }
> +
> + /* Empty protocol specific transport package allowed */
For a statement like that I'd kind of expect a spec reference.
> + if (pack->package.count != 0) {
> + ret = acpi_scmi_dsd_parse_protocol_subpackage(map, pack, prot_id);
> + if (ret)
> + return ret;
> + }
> +
> + pack = &elems[2];
> + if (pack->type != ACPI_TYPE_PACKAGE) {
> + pr_info("Invalid protocol transport association package\n");
> + return -EINVAL;
> + }
> +
> + if (pack->package.count != 0) {
> + pr_info("Non-empty association package not supported\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int acpi_scmi_parse_properties(struct pcc_transport_map *map,
> + const union acpi_object *properties)
> +{
> + bool transport_found = false;
> + int i;
> +
> + if (properties->type != ACPI_TYPE_PACKAGE)
> + return -EINVAL;
> +
> + for (i = 0; i < properties->package.count; i++) {
> + const union acpi_object *v;
> + const char *name;
> + int prot_id, ret;
> +
> + ret = acpi_scmi_property(properties, i, &name, &v);
> + if (ret)
> + return ret;
> +
> + prot_id = acpi_scmi_lookup_protocol_id(name);
> + if (prot_id < 0)
> + continue;
> + if (prot_id != SCMI_PROTOCOL_BASE)
> + continue;
> + if (v->type != ACPI_TYPE_PACKAGE)
> + return -EINVAL;
> + if (transport_found)
> + return -EEXIST;
> +
> + ret = acpi_scmi_dsd_parse_transport_package(map, v);
> + if (ret)
> + return ret;
> + transport_found = true;
> + }
> +
> + if (!transport_found)
> + return -ENOENT;
This double loop needs a few more comments. Why do we need to handle
the base protocol completely first?
Maybe can factor it out to a helper that takes bool unique, bool base?
then we just get 2 calls to that.
> +
> + for (i = 0; i < properties->package.count; i++) {
> + const union acpi_object *v;
> + const char *name;
> + int prot_id, ret;
> +
> + ret = acpi_scmi_property(properties, i, &name, &v);
> + if (ret)
> + return ret;
> +
> + prot_id = acpi_scmi_lookup_protocol_id(name);
> + if (prot_id < 0 || prot_id == SCMI_PROTOCOL_BASE)
> + continue;
> + if (v->type != ACPI_TYPE_PACKAGE)
> + return -EINVAL;
> +
> + ret = acpi_scmi_dsd_parse_protocol_package(map, v, prot_id);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int acpi_scmi_namespace_fwnode_parse(struct fwnode_handle *fwnode,
> + struct pcc_transport_map *map)
> +{
> + struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct acpi_device *adev = to_acpi_device_node(fwnode);
> + union acpi_object *desc;
> + acpi_status status;
> + int i, ret = -ENOENT;
ret is always overwritten I think.
> +
> + if (!adev->handle)
> + return -EINVAL;
> +
> + status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL, &buf,
> + ACPI_TYPE_PACKAGE);
> + if (ACPI_FAILURE(status))
> + return -EINVAL;
> +
> + desc = buf.pointer;
> + if (desc->package.count % 2)
ret = -EINVAL;
goto out_free;
}
> + goto out_free_inval;
> +
> + /* Look for the device properties GUID. */
> + for (i = 0; i < desc->package.count; i += 2) {
for (int i = 0; i < ...
acceptable in kernel these days and keeps scope tight.
Any way to justify that 2 as sizeof of something? If not
maybe a define is appropriate.
(applies above as well.)
> + const union acpi_object *guid;
> + const union acpi_object *properties;
> +
> + guid = &desc->package.elements[i];
> + properties = &desc->package.elements[i + 1];
> +
> + /*
> + * The first element must be a GUID and the second one must be
> + * a package.
> + */
> + if (guid->type != ACPI_TYPE_BUFFER ||
> + guid->buffer.length != UUID_SIZE ||
> + properties->type != ACPI_TYPE_PACKAGE)
> + continue;
> +
> + if (!guid_equal((guid_t *)guid->buffer.pointer,
> + &acpi_scmi_uuid))
> + continue;
> +
> + ret = acpi_scmi_parse_properties(map, properties);
> + goto out_free;
break maybe if this doesn't get more complex in later
patches.
> + }
> +
> +out_free:
> + ACPI_FREE(buf.pointer);
> + return ret;
> +out_free_inval:
> + ret = -EINVAL;
> + goto out_free;
Two different error paths and one that folds back is not a nice to
read code structure. Particularly as second one only sets a return
value. Just set that at the callers.
> +}
> +static
> +struct pcc_transport_map *pcc_transport_map_get(struct fwnode_handle *fwnode)
> +{
> + struct pcc_transport_map *map;
> + int ret;
> +
> + map = pcc_transport_map_find(fwnode);
> + if (map)
> + return map;
> +
> + map = kzalloc_obj(*map, GFP_KERNEL);
> + if (!map)
> + return ERR_PTR(-ENOMEM);
> +
> + hash_init(map->table);
> + ret = acpi_scmi_namespace_fwnode_parse(fwnode, map);
> + if (ret)
> + goto err_free_map;
> +
> + ret = pcc_transport_map_validate(map);
> + if (ret)
> + goto err_free_map;
> +
> + map->fwnode = fwnode_handle_get(fwnode);
> + list_add_tail(&map->node, &pcc_transport_maps);
> +
> + return map;
> +
> +err_free_map:
> + acpi_scmi_destroy_transport_map(map);
Personally I'd prefer seeing each step being unwound only when necessary.
So break it out here as as series of labels.
> + return ERR_PTR(ret);
> +}
> +
> +static int pcc_lookup_ss_id(struct pcc_transport_map *map, u32 prot_id, bool tx)
> +{
> + struct pcc_transport *p;
> + int idx;
> +
> + hash_for_each(map->table, idx, p, hnode) {
> + if (p->protocol_id != prot_id)
> + continue;
> +
> + if ((!tx && (p->flags & SCMI_TRANSPORT_P2A_CHANNEL)) ||
> + (tx && !(p->flags & SCMI_TRANSPORT_P2A_CHANNEL)))
> + return p->pcc_ss_id;
> + }
> +
> + return -ENOENT;
> +}
> +
> +static int pcc_get_ss_id(struct fwnode_handle *fwnode, u32 prot_id, bool tx)
> +{
> + struct pcc_transport_map *map;
> + int ret;
> +
> + if (!fwnode)
> + return -EINVAL;
> +
> + mutex_lock(&pcc_transport_maps_lock);
guard(mutex)(&pcc_transport_maps_lock);
> + map = pcc_transport_map_get(fwnode);
> + if (IS_ERR(map))
> + ret = PTR_ERR(map);
return PTR_ERR(map)
return pcc_lookup_ss_id(map, prot_id, tx);
> + else
> + ret = pcc_lookup_ss_id(map, prot_id, tx);
> + mutex_unlock(&pcc_transport_maps_lock);
> +
> + return ret;
> +}
> +
> +static int pcc_chan_free(int id, void *p, void *data)
> +{
> + struct scmi_chan_info *cinfo = p;
> + struct scmi_pcc *smbox = cinfo->transport_info;
> +
> + if (smbox && !IS_ERR(smbox->pchan)) {
Maybe an early exit is neater?
if (!smbox || IS_ERR(smbox->pchan)
return 0;
> + pcc_mbox_free_channel(smbox->pchan);
> + cinfo->transport_info = NULL;
> + smbox->pchan = NULL;
> + smbox->cinfo = NULL;
> + }
> +
> + return 0;
> +}
> +static void pcc_fetch_response(struct scmi_chan_info *cinfo,
> + struct scmi_xfer *xfer)
> +{
> + struct scmi_pcc *smbox = cinfo->transport_info;
> + struct pcc_shared_mem __iomem *shmem = smbox->pchan->shmem;
> + size_t len = ioread32(&shmem->header.length);
> +
> + xfer->hdr.status = ioread32(shmem->msg_payload);
> + /* Skip the length of header and status in shmem area i.e 8 bytes */
> + xfer->rx.len = min_t(size_t, xfer->rx.len, len > 8 ? len - 8 : 0);
> +
> + /* Take a copy to the rx buffer.. */
As below - that bit is obvious.
> + memcpy_fromio(xfer->rx.buf, shmem->msg_payload + 4, xfer->rx.len);
So you compute the length skipping 8 but then copy 4 in. That needs an explanatory
comment if correct.
> +}
> +
> +static void pcc_fetch_notification(struct scmi_chan_info *cinfo, size_t max_len,
> + struct scmi_xfer *xfer)
> +{
> + struct scmi_pcc *smbox = cinfo->transport_info;
> + struct pcc_shared_mem __iomem *shmem = smbox->pchan->shmem;
> + size_t len = ioread32(&shmem->header.length);
> +
> + /* Skip only the length of header in shmem area i.e 4 bytes */
Ideally get that header size from a define rather than magic 4.
> + xfer->rx.len = min_t(size_t, max_len, len > 4 ? len - 4 : 0);
min() preferred unless we are hitting one of the weird corner cases (don't think so)
> +
> + /* Take a copy to the rx buffer.. */
Kind of obvious - maybe say why if that is useful, or drop the comment.
> + memcpy_fromio(xfer->rx.buf, shmem->msg_payload, xfer->rx.len);
> +}
> +
> +static const struct scmi_transport_ops scmi_pcc_ops = {
> + .chan_available = pcc_chan_available,
> + .chan_setup = pcc_chan_setup,
> + .chan_free = pcc_chan_free,
> + .send_message = pcc_send_message,
> + .fetch_response = pcc_fetch_response,
> + .fetch_notification = pcc_fetch_notification,
> +};
> +
> +static struct scmi_desc scmi_pcc_desc = {
> + .ops = &scmi_pcc_ops,
> + .max_rx_timeout_ms = 30, /* We may increase this if required */
That's always true - so what does the comment bring us?
> + .max_msg = 20, /* Limited by MBOX_TX_QUEUE_LEN */
If this is relevant to this driver, why can't see see it via a suitable header?
Feels to me like this is in the wrong place or needs a query interface.
> + .max_msg_size = SCMI_SHMEM_MAX_PAYLOAD_SIZE - 12,
> +};
> +
> +static const struct acpi_device_id scmi_acpi_ids[] = {
> + { "ARML0001", 0 },
Uwe is driving an effort to make these all named initializers.
+ Don't set anything you don't use as it makes refactors a pain.
Uwe has also been deleting those throughout the kernel!
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, scmi_acpi_ids);
> +
> +DEFINE_SCMI_ACPI_TRANSPORT_DRIVER(scmi_pcc, scmi_pcc_driver,
> + scmi_pcc_desc, scmi_acpi_ids, core);
> +
> +static int __init scmi_pcc_init(void)
> +{
> + return platform_driver_register(&scmi_pcc_driver);
> +}
> +
> +static void __exit scmi_pcc_exit(void)
> +{
> + platform_driver_unregister(&scmi_pcc_driver);
> +
> + mutex_lock(&pcc_transport_maps_lock);
I'd move the locking into acpi_scmi_clear_transport_maps()
I'm not immediately understanding why, when all setup in this
driver is associated with the registered driver, this bit
of tear down can't be done as part of the driver remove.
> + acpi_scmi_clear_transport_maps();
> + mutex_unlock(&pcc_transport_maps_lock);
> +}
> +module_init(scmi_pcc_init);
> +module_exit(scmi_pcc_exit);
> +
> +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@kernel.org>");
> +MODULE_DESCRIPTION("SCMI ACPI PCC Transport driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 5ab73b1ab9aa..02cf04543151 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -930,6 +930,7 @@ enum scmi_std_protocol {
> SCMI_PROTOCOL_VOLTAGE = 0x17,
> SCMI_PROTOCOL_POWERCAP = 0x18,
> SCMI_PROTOCOL_PINCTRL = 0x19,
> + SCMI_PROTOCOL_TELEMETRY = 0x1B,
> };
>
> enum scmi_system_events {
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 8/9] firmware: arm_scmi: Initialise known ACPI protocol devices and channels
2026-08-13 11:32 [PATCH v3 0/9] firmware: arm_scmi: Refactoring and enablement of ACPI PCC transport Sudeep Holla
` (6 preceding siblings ...)
2026-08-13 11:33 ` [PATCH v3 7/9] firmware: arm_scmi: Add ACPI PCC transport Sudeep Holla
@ 2026-08-13 11:33 ` Sudeep Holla
2026-08-24 20:21 ` Jonathan Cameron
2026-08-13 11:33 ` [PATCH v3 9/9] firmware: arm_scmi: Validate PCC shared memory signature Sudeep Holla
8 siblings, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2026-08-13 11:33 UTC (permalink / raw)
To: arm-scmi, linux-arm-kernel, kernel-team; +Cc: Cristian Marussi, Breno Leitao
Unlike Device Tree, the ACPI SCMI namespace device does not provide
child fwnodes to represent each protocol. Iterate over the non-BASE
entries in scmi_dsd_info_list to initialize their protocol devices and
transport channels. The BASE channel and device are handled by the
common setup path.
Let the transport channel-availability and SCMI protocol implementation
checks decide which of the known protocols are usable on the platform.
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
drivers/firmware/arm_scmi/driver.c | 37 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 4 deletions(-)
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 951ba3df6ba6..45591c971c56 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2873,7 +2873,7 @@ scmi_txrx_setup(struct scmi_info *info, struct fwnode_handle *fwnode,
*/
static int scmi_channels_setup(struct scmi_info *info)
{
- int ret;
+ int ret, idx;
struct fwnode_handle *fwnode = dev_fwnode(info->dev);
/* Initialize a common generic channel at first */
@@ -2898,6 +2898,20 @@ static int scmi_channels_setup(struct scmi_info *info)
return ret;
}
+ if (!is_acpi_node(fwnode))
+ return 0;
+
+ for (idx = 0; idx < ARRAY_SIZE(scmi_dsd_info_list); idx++) {
+ int prot_id = scmi_dsd_info_list[idx].protocol_id;
+
+ if (prot_id == SCMI_PROTOCOL_BASE)
+ continue;
+
+ ret = scmi_txrx_setup(info, fwnode, prot_id);
+ if (ret)
+ return ret;
+ }
+
return 0;
}
@@ -3245,7 +3259,7 @@ static void scmi_enable_matching_quirks(struct scmi_info *info)
}
static void scmi_device_check_create(struct fwnode_handle *fwnode, int prot_id,
- struct scmi_info *info)
+ struct scmi_info *info, bool report_missing)
{
int ret;
struct device *dev = info->dev;
@@ -3257,6 +3271,9 @@ static void scmi_device_check_create(struct fwnode_handle *fwnode, int prot_id,
}
if (!scmi_is_protocol_implemented(handle, prot_id)) {
+ if (!report_missing)
+ return;
+
dev_err(dev, "SCMI protocol %d not implemented\n",
prot_id);
return;
@@ -3280,7 +3297,7 @@ static void scmi_device_check_create(struct fwnode_handle *fwnode, int prot_id,
static int scmi_probe(struct platform_device *pdev)
{
- int ret;
+ int ret, idx;
char *err_str = "probe failure\n";
struct scmi_handle *handle;
const struct scmi_desc *desc;
@@ -3406,7 +3423,19 @@ static int scmi_probe(struct platform_device *pdev)
if (fwnode_property_read_u32(child, "reg", &prot_id))
continue;
- scmi_device_check_create(child, prot_id, info);
+ scmi_device_check_create(child, prot_id, info, true);
+ }
+
+ if (!is_acpi_node(dev_fwnode(dev)))
+ return 0;
+
+ for (idx = 0; idx < ARRAY_SIZE(scmi_dsd_info_list); idx++) {
+ int prot_id = scmi_dsd_info_list[idx].protocol_id;
+
+ if (prot_id == SCMI_PROTOCOL_BASE)
+ continue;
+
+ scmi_device_check_create(dev_fwnode(dev), prot_id, info, false);
}
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 8/9] firmware: arm_scmi: Initialise known ACPI protocol devices and channels
2026-08-13 11:33 ` [PATCH v3 8/9] firmware: arm_scmi: Initialise known ACPI protocol devices and channels Sudeep Holla
@ 2026-08-24 20:21 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2026-08-24 20:21 UTC (permalink / raw)
To: Sudeep Holla
Cc: arm-scmi, linux-arm-kernel, kernel-team, Cristian Marussi,
Breno Leitao
On Thu, 13 Aug 2026 12:33:03 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:
> Unlike Device Tree, the ACPI SCMI namespace device does not provide
> child fwnodes to represent each protocol. Iterate over the non-BASE
> entries in scmi_dsd_info_list to initialize their protocol devices and
> transport channels. The BASE channel and device are handled by the
> common setup path.
>
> Let the transport channel-availability and SCMI protocol implementation
> checks decide which of the known protocols are usable on the platform.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 9/9] firmware: arm_scmi: Validate PCC shared memory signature
2026-08-13 11:32 [PATCH v3 0/9] firmware: arm_scmi: Refactoring and enablement of ACPI PCC transport Sudeep Holla
` (7 preceding siblings ...)
2026-08-13 11:33 ` [PATCH v3 8/9] firmware: arm_scmi: Initialise known ACPI protocol devices and channels Sudeep Holla
@ 2026-08-13 11:33 ` Sudeep Holla
2026-08-24 20:24 ` Jonathan Cameron
8 siblings, 1 reply; 19+ messages in thread
From: Sudeep Holla @ 2026-08-13 11:33 UTC (permalink / raw)
To: arm-scmi, linux-arm-kernel, kernel-team; +Cc: Cristian Marussi, Breno Leitao
Validate the PCC shared memory signature when setting up an SCMI PCC
channel.
Reject channels whose shared memory signature does not encode
PCC_SIGNATURE combined with the PCC subspace ID, so misconfigured
firmware is caught before the transport starts using the shared memory
region.
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
drivers/firmware/arm_scmi/transports/pcc.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/arm_scmi/transports/pcc.c b/drivers/firmware/arm_scmi/transports/pcc.c
index 337d551e3ad8..0ead0ec77162 100644
--- a/drivers/firmware/arm_scmi/transports/pcc.c
+++ b/drivers/firmware/arm_scmi/transports/pcc.c
@@ -623,8 +623,11 @@ static void rx_callback(struct mbox_client *cl, void *m)
}
static int pcc_chan_validate_shmem(struct scmi_chan_info *cinfo,
- struct scmi_pcc *smbox)
+ struct scmi_pcc *smbox, int ss_id)
{
+ struct pcc_shared_mem __iomem *shmem = smbox->pchan->shmem;
+ u32 valid_signature = ss_id + PCC_SIGNATURE;
+
if (smbox->pchan->shmem_size < SCMI_PCC_SHMEM_OVERHEAD ||
smbox->pchan->shmem_size - SCMI_PCC_SHMEM_OVERHEAD <
cinfo->max_msg_size) {
@@ -632,6 +635,11 @@ static int pcc_chan_validate_shmem(struct scmi_chan_info *cinfo,
return -ENOSPC;
}
+ if (ioread32(&shmem->header.signature) != valid_signature) {
+ dev_err(cinfo->dev, "invalid PCC shared memory signature\n");
+ return -EINVAL;
+ }
+
return 0;
}
@@ -667,7 +675,7 @@ static int pcc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
return ret;
}
- ret = pcc_chan_validate_shmem(cinfo, smbox);
+ ret = pcc_chan_validate_shmem(cinfo, smbox, ss_id);
if (ret) {
pcc_mbox_free_channel(smbox->pchan);
return ret;
--
2.43.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 9/9] firmware: arm_scmi: Validate PCC shared memory signature
2026-08-13 11:33 ` [PATCH v3 9/9] firmware: arm_scmi: Validate PCC shared memory signature Sudeep Holla
@ 2026-08-24 20:24 ` Jonathan Cameron
0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2026-08-24 20:24 UTC (permalink / raw)
To: Sudeep Holla
Cc: arm-scmi, linux-arm-kernel, kernel-team, Cristian Marussi,
Breno Leitao
On Thu, 13 Aug 2026 12:33:04 +0100
Sudeep Holla <sudeep.holla@kernel.org> wrote:
> Validate the PCC shared memory signature when setting up an SCMI PCC
> channel.
>
> Reject channels whose shared memory signature does not encode
> PCC_SIGNATURE combined with the PCC subspace ID, so misconfigured
> firmware is caught before the transport starts using the shared memory
> region.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
Interesting there aren't more checks on this in the various PCC users.
Question for another day perhaps.
Seems correct to me.
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> ---
> drivers/firmware/arm_scmi/transports/pcc.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/transports/pcc.c b/drivers/firmware/arm_scmi/transports/pcc.c
> index 337d551e3ad8..0ead0ec77162 100644
> --- a/drivers/firmware/arm_scmi/transports/pcc.c
> +++ b/drivers/firmware/arm_scmi/transports/pcc.c
> @@ -623,8 +623,11 @@ static void rx_callback(struct mbox_client *cl, void *m)
> }
>
> static int pcc_chan_validate_shmem(struct scmi_chan_info *cinfo,
> - struct scmi_pcc *smbox)
> + struct scmi_pcc *smbox, int ss_id)
> {
> + struct pcc_shared_mem __iomem *shmem = smbox->pchan->shmem;
> + u32 valid_signature = ss_id + PCC_SIGNATURE;
> +
> if (smbox->pchan->shmem_size < SCMI_PCC_SHMEM_OVERHEAD ||
> smbox->pchan->shmem_size - SCMI_PCC_SHMEM_OVERHEAD <
> cinfo->max_msg_size) {
> @@ -632,6 +635,11 @@ static int pcc_chan_validate_shmem(struct scmi_chan_info *cinfo,
> return -ENOSPC;
> }
>
> + if (ioread32(&shmem->header.signature) != valid_signature) {
> + dev_err(cinfo->dev, "invalid PCC shared memory signature\n");
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> @@ -667,7 +675,7 @@ static int pcc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> return ret;
> }
>
> - ret = pcc_chan_validate_shmem(cinfo, smbox);
> + ret = pcc_chan_validate_shmem(cinfo, smbox, ss_id);
> if (ret) {
> pcc_mbox_free_channel(smbox->pchan);
> return ret;
>
^ permalink raw reply [flat|nested] 19+ messages in thread