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

Jonathan Cameron
2025-10-20T17:37:36Z

On Fri, 17 Oct 2025 14:23:50 +0100
Sudeep Holla <sudeep.holla@arm.com> wrote:

> Introduce a new SCMI transport that uses ACPI PCCT (PCC) subspaces via
> the Linux PCC mailbox layer. The driver parses ACPI _DSD data to map
> protocols to PCC subspace UIDs, supports shared TX/RX channels, and
> optionally sets up a P2A channel for notifications.
>
> Key points:
> - new CONFIG_ARM_SCMI_TRANSPORT_PCC option
> - integration with SCMI core via scmi_desc and transport ops
> - response/notification fetch from PCC shared memory header/payload
> - 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@arm.com>
Hi Sudeep,

Just a very quick look in what is definitely a drive by style review
A few things below.

> diff --git a/drivers/firmware/arm_scmi/transports/pcc.c b/drivers/firmware/arm_scmi/transports/pcc.c
> new file mode 100644
> index 000000000000..39ef83e2dfd4
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/transports/pcc.c
> @@ -0,0 +1,390 @@

> +
> +/**
> + * struct scmi_pcc - Structure representing a SCMI mailbox transport
> + *
> + * @cl: Mailbox Client
> + * @pchan: Transmit/Receive PCC/mailbox channel
> + * @cinfo: SCMI channel info
> + * @shmem: Transmit/Receive shared memory area
run kernel-doc over the file (shmem doesn't exist).

> + */
> +struct scmi_pcc {
> + struct mbox_client cl;
> + struct pcc_mbox_chan *pchan;
> + struct scmi_chan_info *cinfo;
> +};

> +
> +static int acpi_scmi_dsd_parse_protocol_subpackage(const union acpi_object *obj,
> + int prot_id)
> +{
> + 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];
> +
> + /* Flags(pack->package.elements[1]) must be always 0 for now */
> + uid = pack->package.elements[0].integer.value;
> + hash_for_each_possible(pcc_id_hash, p, hnode, uid) {
> + if (p->flags & SCMI_TRANSPORT_SHARED_CHANNEL) {
> + pr_info("Invalid! %d channel is shared\n",
> + p->pcc_ss_id);
> + ret = -EINVAL;

This breaks out of the hash_for_each_... but the outer loop might continue
and it's just possible pass. however we leave ret set. Why not bail out
on first error? E.g. return -EINVAL; here.

> + break;
> + }
> + p->protocol_id = prot_id;
> + }
> + }
> +
> + return ret;
> +}

> +#ifdef CONFIG_ACPI

This is the bit I'd avoid by not use ACPI_PTR() in the earlier patch.

> +static const struct acpi_device_id scmi_acpi_ids[] = {
> + {"ARML0001", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, scmi_acpi_ids);
> +#endif
> +
> +DEFINE_SCMI_ACPI_TRANSPORT_DRIVER(scmi_pcc, scmi_pcc_driver,
> + scmi_pcc_desc, scmi_acpi_ids, core);
> +module_platform_driver(scmi_pcc_driver);
> +
> +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
> +MODULE_DESCRIPTION("SCMI ACPI PCC Transport driver");
> +MODULE_LICENSE("GPL");
>