Re: [PATCH v3 7/9] firmware: arm_scmi: Add ACPI PCC transport

On Thu, 13 Aug 2026 12:33:02 +0100
Sudeep Holla <[email protected]> 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 <[email protected]>
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 <[email protected]>");
> +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 {
>