[PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver
Cristian Marussi
cristian.marussi at arm.com
Mon Jul 8 07:27:34 PDT 2024
On Sun, Jul 07, 2024 at 09:52:49AM -0700, Nikunj Kela wrote:
>
> On 7/6/2024 5:20 PM, Cristian Marussi wrote:
> > Make SCMI SMC transport a standalone driver that can be optionally
> > loaded as a module.
> >
> > CC: Peng Fan <peng.fan at nxp.com>
> > CC: Nikunj Kela <quic_nkela at quicinc.com>
> > Signed-off-by: Cristian Marussi <cristian.marussi at arm.com>
> > ---
> > drivers/firmware/arm_scmi/Kconfig | 4 ++-
> > drivers/firmware/arm_scmi/Makefile | 2 +-
> > drivers/firmware/arm_scmi/common.h | 3 --
> > drivers/firmware/arm_scmi/driver.c | 5 ---
> > .../arm_scmi/{smc.c => scmi_transport_smc.c} | 31 +++++++++++++++----
> > 5 files changed, 29 insertions(+), 16 deletions(-)
> > rename drivers/firmware/arm_scmi/{smc.c => scmi_transport_smc.c} (89%)
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> > index 135e34aefd70..a4d44ef8bf45 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -102,7 +102,7 @@ config ARM_SCMI_TRANSPORT_OPTEE
> > transport based on OP-TEE SCMI service, answer Y.
> >
> > config ARM_SCMI_TRANSPORT_SMC
> > - bool "SCMI transport based on SMC"
> > + tristate "SCMI transport based on SMC"
> > depends on HAVE_ARM_SMCCC_DISCOVERY
> > select ARM_SCMI_HAVE_TRANSPORT
> > select ARM_SCMI_HAVE_SHMEM
> > @@ -112,6 +112,8 @@ config ARM_SCMI_TRANSPORT_SMC
> >
> > If you want the ARM SCMI PROTOCOL stack to include support for a
> > transport based on SMC, answer Y.
> > + This driver can also be built as a module. If so, the module
> > + will be called scmi_transport_smc.
> >
> > config ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE
> > bool "Enable atomic mode support for SCMI SMC transport"
> > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > index 121612d75f0b..6868a47fa4ab 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -5,7 +5,6 @@ scmi-core-objs := $(scmi-bus-y)
> > scmi-driver-y = driver.o notify.o
> > scmi-driver-$(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT) += raw_mode.o
> > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_SHMEM) = shmem.o
> > -scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > @@ -13,6 +12,7 @@ scmi-protocols-y := base.o clock.o perf.o power.o reset.o sensors.o system.o vol
> > scmi-protocols-y += pinctrl.o
> > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
> >
> > +obj-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += scmi_transport_smc.o
> > obj-$(CONFIG_ARM_SCMI_TRANSPORT_MAILBOX) += scmi_transport_mailbox.o
> >
> > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index c03f30db92e0..b5bd27eccf24 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -286,9 +286,6 @@ int scmi_xfer_raw_inflight_register(const struct scmi_handle *handle,
> > int scmi_xfer_raw_wait_for_message_response(struct scmi_chan_info *cinfo,
> > struct scmi_xfer *xfer,
> > unsigned int timeout_ms);
> > -#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
> > -extern const struct scmi_desc scmi_smc_desc;
> > -#endif
> > #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> > extern const struct scmi_desc scmi_virtio_desc;
> > #endif
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 96cf8ab4421e..b14c5326930a 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -3254,11 +3254,6 @@ static const struct of_device_id scmi_of_match[] = {
> > #ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE
> > { .compatible = "linaro,scmi-optee", .data = &scmi_optee_desc },
> > #endif
> > -#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
> > - { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> > - { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc},
> > - { .compatible = "qcom,scmi-smc", .data = &scmi_smc_desc},
> > -#endif
> > #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
> > { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
> > #endif
> > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/scmi_transport_smc.c
> > similarity index 89%
> > rename from drivers/firmware/arm_scmi/smc.c
> > rename to drivers/firmware/arm_scmi/scmi_transport_smc.c
> > index cb26b8aee01d..44da1a8d5387 100644
> > --- a/drivers/firmware/arm_scmi/smc.c
> > +++ b/drivers/firmware/arm_scmi/scmi_transport_smc.c
> > @@ -3,7 +3,7 @@
> > * System Control and Management Interface (SCMI) Message SMC/HVC
> > * Transport driver
> > *
> > - * Copyright 2020 NXP
> > + * Copyright 2020-2024 NXP
> > */
> >
> > #include <linux/arm-smccc.h>
> > @@ -16,6 +16,7 @@
> > #include <linux/of_address.h>
> > #include <linux/of_irq.h>
> > #include <linux/limits.h>
> > +#include <linux/platform_device.h>
> > #include <linux/processor.h>
> > #include <linux/slab.h>
> >
> > @@ -69,12 +70,14 @@ struct scmi_smc {
> > unsigned long cap_id;
> > };
> >
> > +static struct scmi_transport_core_operations *core;
> > +
> > static irqreturn_t smc_msg_done_isr(int irq, void *data)
> > {
> > struct scmi_smc *scmi_info = data;
> >
> > - scmi_rx_callback(scmi_info->cinfo,
> > - scmi_shmem_ops.read_header(scmi_info->shmem), NULL);
> > + core->rx_callback(scmi_info->cinfo,
> > + core->shmem->read_header(scmi_info->shmem), NULL);
> >
> > return IRQ_HANDLED;
> > }
> > @@ -142,7 +145,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> > if (!scmi_info)
> > return -ENOMEM;
> >
> > - scmi_info->shmem = scmi_shmem_ops.setup_iomap(cinfo, dev, tx);
> > + scmi_info->shmem = core->shmem->setup_iomap(cinfo, dev, tx);
> > if (IS_ERR(scmi_info->shmem))
> > return PTR_ERR(scmi_info->shmem);
> >
> > @@ -226,7 +229,7 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> > */
> > smc_channel_lock_acquire(scmi_info, xfer);
> >
> > - scmi_shmem_ops.tx_prepare(scmi_info->shmem, xfer, cinfo);
> > + core->shmem->tx_prepare(scmi_info->shmem, xfer, cinfo);
> >
> > if (scmi_info->cap_id != ULONG_MAX)
> > arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->cap_id, 0,
> > @@ -250,7 +253,7 @@ static void smc_fetch_response(struct scmi_chan_info *cinfo,
> > {
> > struct scmi_smc *scmi_info = cinfo->transport_info;
> >
> > - scmi_shmem_ops.fetch_response(scmi_info->shmem, xfer);
> > + core->shmem->fetch_response(scmi_info->shmem, xfer);
> > }
> >
> > static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret,
> > @@ -286,3 +289,19 @@ const struct scmi_desc scmi_smc_desc = {
> > .sync_cmds_completed_on_ret = true,
> > .atomic_enabled = IS_ENABLED(CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE),
> > };
> > +
> > +static const struct of_device_id scmi_of_match[] = {
> > + { .compatible = "arm,scmi-smc" },
> > + { .compatible = "arm,scmi-smc-param" },
> > + { .compatible = "qcom,scmi-smc" },
> > + { /* Sentinel */ },
> > +};
> > +
>
> Hi Cristian,
>
Hi Nikunj,
thanks for having a look first of all !
> Would it make sense to associate scmi descriptor(scmi_smc_desc) with
> compatible as driver/platform data so we have flexibility to replicate
> it and modify parameters such as max_timeout_ms etc. for our platform?
>
Mmmm...not sure to have understood, because the scmi_smc_desc is
effecetively passed from this driver to the core via a bit of
(questionable) magic in the mega-macro
DEFINE_SCMI_TRANSPORT_DRIVER(scmi_smc, scmi_of_match, &core);
...and it will end-up being set into the dev.platform_data and then
retrieved by the core SCMI stack driver in scmi_probe...
...OR...do you mean being able to somehow define 3 different
scmi_smc_desc* and then associate them to the different compatibles
and then, depending on which compatible is matched by this isame driver
at probe time, passing the related platform-specific desc to the core...
...in this latter case I suppose I can do it by playing with the macros
defs but maybe it is also the case to start thinking about splitting out
configuration stuff from the transport descriptor...
I'll give it a go at passing the data around, and see how it plays out
if you confirm that this is what you meant...
Thanks,
Cristian
More information about the linux-arm-kernel
mailing list