Re: [PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver

From: Cristian Marussi
Date: Tue Jul 09 2024 - 06:33:59 EST

Next message: kernel test robot: "Re: [PATCH v2 2/6] Bluetooth: hci_qca: schedule a devm action for disabling the clock"
Previous message: Borislav Petkov: "Re: [PATCH 0/3] Resolve problems with kexec identity mapping"
In reply to: Nikunj Kela: "Re: [PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver"
Next in thread: kernel test robot: "Re: [PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Jul 08, 2024 at 10:59:33AM -0700, Nikunj Kela wrote:
>
> On 7/8/2024 8:47 AM, Cristian Marussi wrote:
> > On Mon, Jul 08, 2024 at 08:23:56AM -0700, Nikunj Kela wrote:
> >> 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@xxxxxxx>
> >>>>> CC: Nikunj Kela <quic_nkela@xxxxxxxxxxx>
> >>>>> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> >>>>> ---
> >>>>> 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,
> >>
> > Hi,
> >
> >> 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
> > :P
> >
> >> 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!
> >>
> > Interesting, because there is also this thread flying around from Peng:
> >
> > https://lore.kernel.org/linux-arm-kernel/20240703031715.379815-1-peng.fan@xxxxxxxxxxx/
> Thanks Cristian for the link. I was under the impression that DT binding
> that don't describe HW are not acceptable to DT maintainer. But if this

...I was under the same impression...not sure if this can be considered a
sort of HW characteristic that actually depends on how the platform (and the SCMI
server fw) is designed...

> patch goes through, I am fine using it. I would also like to have
> max_msg_size(and maybe max_msg) configurable.

...I am indeed monitoring the situation :D...in general I think there
are some transport-related characteritic that could made sense to fine
tune at compile time (in whatever way), while others can only be
menaninngfully discovered at runtime...like the get_msg_size...

Thanks,
Cristian

Next message: kernel test robot: "Re: [PATCH v2 2/6] Bluetooth: hci_qca: schedule a devm action for disabling the clock"
Previous message: Borislav Petkov: "Re: [PATCH 0/3] Resolve problems with kexec identity mapping"
In reply to: Nikunj Kela: "Re: [PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver"
Next in thread: kernel test robot: "Re: [PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]