Message-ID: <Zo0SAjZ2H-sBn-LH@pluto>
Date: Tue, 9 Jul 2024 11:33:38 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Nikunj Kela <quic_nkela@...cinc.com>
Cc: Cristian Marussi <cristian.marussi@....com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	arm-scmi@...r.kernel.org, sudeep.holla@....com,
	james.quinlan@...adcom.com, f.fainelli@...il.com,
	vincent.guittot@...aro.org, etienne.carriere@...s.st.com,
	peng.fan@....nxp.com, michal.simek@....com, quic_sibis@...cinc.com,
	ptosi@...gle.com, dan.carpenter@...aro.org,
	souvik.chakravarty@....com, Peng Fan <peng.fan@....com>
Subject: Re: [PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone
 driver

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@....com>
> >>>>> CC: Nikunj Kela <quic_nkela@...cinc.com>
> >>>>> Signed-off-by: Cristian Marussi <cristian.marussi@....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,
> >>
> > 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@oss.nxp.com/
> 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

Powered by blists - more mailing lists