[PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver
Nikunj Kela
quic_nkela at quicinc.com
Mon Jul 8 08:23:56 PDT 2024
On 7/8/2024 7:27 AM, Cristian Marussi wrote:
> 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...
Hi Cristian,
I wanted to send a patch for review(with older driver code) that will
allow us to override transport parameters(e.g. max_timeout_ms,
max_msg_size etc.) on Qualcomm platform. There could be multiple
approaches- 1) add callbacks (similar to get_msg_size) in transport_ops
and override the default or 2) replicate the descriptors for different
compatible and change those values as needed. I was going with the
second option but then I saw your patch and thought of throwing this at
you ;) I don't want you to hold your patch series for this but if you
have a better way to achieve or a preferred way between the two
mentioned before, please let me know. If you do want to add this feature
in this patch series, that would be great!
Thanks,
-Nikunj
>
> Thanks,
> Cristian
More information about the linux-arm-kernel
mailing list