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

[Nikunj Kela]

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
patch goes through, I am fine using it. I would also like to have
max_msg_size(and maybe max_msg) configurable.
>
> ...which I was planning in embedding in this series; Peng's proposal is
> limited to timeout and based on DT (and title is a bit misleading...the
> series is not mailbox-only related)....
>
> ...now I am NOT saying that we should dump all configs into the DT, but
> just that this issue about configurability of transports is already sort
> of a known-problem, and it is a while that it floats in the back of my mind...
> i.e. the fact that the transport runtime configurations should be *somehow*
> independent of the transport descriptor and *somehow* configurable....so now
> only remains to *somehow* figure out how to do it :D
>
> ... I'll have a though and maybe cannibalize your ideas and Peng's one and then
> we could have a discussion around this on the list to address eveybody's needs...
>
> ...and maybe, in the meantime, you could also post your proposed series about
> this even though based on the old code, but as an RFC, just to make a point and
> detail the needs...but yeah only if it does not require a bunch of extra work from
> you given that it is only to be used as a basis for discussion ....
>
> ...up to you what do you prefer.
>
> Thanks,
> Cristian