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

[Nikunj Kela]

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,

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