[PATCH v3 7/9] firmware: arm_scmi: Add ACPI PCC transport
Jonathan Cameron
jonathan.cameron at oss.qualcomm.com
Mon Aug 24 13:17:09 PDT 2026
On Thu, 13 Aug 2026 12:33:02 +0100
Sudeep Holla <sudeep.holla at 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 at 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 at 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 {
>
More information about the linux-arm-kernel
mailing list