| Message ID | 20240707002055.1835121-6-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make SCMI transport as standalone drivers | expand |
Hi Cristian,
kernel test robot noticed the following build warnings:
[auto build test WARNING on soc/for-next]
[also build test WARNING on next-20240703]
[cannot apply to linus/master v6.10-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cristian-Marussi/firmware-arm_scmi-Introduce-setup_shmem_iomap/20240707-082513
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link: https://lore.kernel.org/r/20240707002055.1835121-6-cristian.marussi%40arm.com
patch subject: [PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver
config: arm-defconfig (https://download.01.org/0day-ci/archive/20240707/202407072316.PqU8yj52-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240707/202407072316.PqU8yj52-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407072316.PqU8yj52-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/firmware/arm_scmi/scmi_transport_smc.c:157:58: warning: variable 'size' is uninitialized when used here [-Wuninitialized]
void __iomem *ptr = (void __iomem *)scmi_info->shmem + size - 8;
^~~~
drivers/firmware/arm_scmi/scmi_transport_smc.c:136:22: note: initialize the variable 'size' to silence this warning
resource_size_t size;
^
= 0
1 warning generated.
vim +/size +157 drivers/firmware/arm_scmi/scmi_transport_smc.c
0bfdca8a8661aa drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 129
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 130 static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 131 bool tx)
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 132 {
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 133 struct device *cdev = cinfo->dev;
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 134 unsigned long cap_id = ULONG_MAX;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 135 struct scmi_smc *scmi_info;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 136 resource_size_t size;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 137 struct resource res;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 138 u32 func_id;
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 139 int ret;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 140
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 141 if (!tx)
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 142 return -ENODEV;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 143
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 144 scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL);
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 145 if (!scmi_info)
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 146 return -ENOMEM;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 147
728a057b6ab114 drivers/firmware/arm_scmi/scmi_transport_smc.c Cristian Marussi 2024-07-07 148 scmi_info->shmem = core->shmem->setup_iomap(cinfo, dev, tx);
6c5d3315c40fa0 drivers/firmware/arm_scmi/smc.c Peng Fan 2024-07-07 149 if (IS_ERR(scmi_info->shmem))
6c5d3315c40fa0 drivers/firmware/arm_scmi/smc.c Peng Fan 2024-07-07 150 return PTR_ERR(scmi_info->shmem);
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 151
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 152 ret = of_property_read_u32(dev->of_node, "arm,smc-id", &func_id);
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 153 if (ret < 0)
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 154 return ret;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 155
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 156 if (of_device_is_compatible(dev->of_node, "qcom,scmi-smc")) {
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 @157 void __iomem *ptr = (void __iomem *)scmi_info->shmem + size - 8;
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 158 /* The capability-id is kept in last 8 bytes of shmem.
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 159 * +-------+ <-- 0
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 160 * | shmem |
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 161 * +-------+ <-- size - 8
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 162 * | capId |
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 163 * +-------+ <-- size
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 164 */
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 165 memcpy_fromio(&cap_id, ptr, sizeof(cap_id));
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 166 }
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 167
5f2ea10a808aef drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-05-06 168 if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) {
5f2ea10a808aef drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-05-06 169 scmi_info->param_page = SHMEM_PAGE(res.start);
5f2ea10a808aef drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-05-06 170 scmi_info->param_offset = SHMEM_OFFSET(res.start);
5f2ea10a808aef drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-05-06 171 }
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 172 /*
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 173 * If there is an interrupt named "a2p", then the service and
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 174 * completion of a message is signaled by an interrupt rather than by
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 175 * the return of the SMC call.
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 176 */
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 177 scmi_info->irq = of_irq_get_byname(cdev->of_node, "a2p");
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 178 if (scmi_info->irq > 0) {
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 179 ret = request_irq(scmi_info->irq, smc_msg_done_isr,
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 180 IRQF_NO_SUSPEND, dev_name(dev), scmi_info);
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 181 if (ret) {
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 182 dev_err(dev, "failed to setup SCMI smc irq\n");
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 183 return ret;
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 184 }
f716cbd33f038a drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 185 } else {
f716cbd33f038a drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 186 cinfo->no_completion_irq = true;
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 187 }
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 188
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 189 scmi_info->func_id = func_id;
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 190 scmi_info->cap_id = cap_id;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 191 scmi_info->cinfo = cinfo;
0bfdca8a8661aa drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 192 smc_channel_lock_init(scmi_info);
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 193 cinfo->transport_info = scmi_info;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 194
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 195 return 0;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 196 }
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 197
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@nxp.com> > CC: Nikunj Kela <quic_nkela@quicinc.com> > Signed-off-by: Cristian Marussi <cristian.marussi@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, 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? Thanks, -Nikunj > +DEFINE_SCMI_TRANSPORT_DRIVER(scmi_smc, scmi_of_match, &core); > +module_platform_driver(scmi_smc_driver); > + > +MODULE_ALIAS("scmi-transport-smc"); > +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>"); > +MODULE_AUTHOR("Nikunj Kela <quic_nkela@quicinc.com>"); > +MODULE_DESCRIPTION("SCMI SMC Transport driver"); > +MODULE_LICENSE("GPL");
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@nxp.com> > > CC: Nikunj Kela <quic_nkela@quicinc.com> > > Signed-off-by: Cristian Marussi <cristian.marussi@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... Thanks, Cristian
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@nxp.com> >>> CC: Nikunj Kela <quic_nkela@quicinc.com> >>> Signed-off-by: Cristian Marussi <cristian.marussi@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
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@nxp.com> > >>> CC: Nikunj Kela <quic_nkela@quicinc.com> > >>> Signed-off-by: Cristian Marussi <cristian.marussi@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, > 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/ ...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
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@nxp.com> >>>>> CC: Nikunj Kela <quic_nkela@quicinc.com> >>>>> Signed-off-by: Cristian Marussi <cristian.marussi@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, >> > 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 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
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@nxp.com> > >>>>> CC: Nikunj Kela <quic_nkela@quicinc.com> > >>>>> Signed-off-by: Cristian Marussi <cristian.marussi@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, > >> > > 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
Hi Cristian,
kernel test robot noticed the following build errors:
[auto build test ERROR on soc/for-next]
[also build test ERROR on next-20240709]
[cannot apply to linus/master v6.10-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cristian-Marussi/firmware-arm_scmi-Introduce-setup_shmem_iomap/20240707-082513
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link: https://lore.kernel.org/r/20240707002055.1835121-6-cristian.marussi%40arm.com
patch subject: [PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver
config: arm-randconfig-003-20240709 (https://download.01.org/0day-ci/archive/20240709/202407092025.wqTyq8Er-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project a0c6b8aef853eedaa0980f07c0a502a5a8a9740e)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240709/202407092025.wqTyq8Er-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407092025.wqTyq8Er-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/firmware/arm_scmi/scmi_transport_smc.c:157:58: warning: variable 'size' is uninitialized when used here [-Wuninitialized]
157 | void __iomem *ptr = (void __iomem *)scmi_info->shmem + size - 8;
| ^~~~
drivers/firmware/arm_scmi/scmi_transport_smc.c:136:22: note: initialize the variable 'size' to silence this warning
136 | resource_size_t size;
| ^
| = 0
>> drivers/firmware/arm_scmi/scmi_transport_smc.c:235:3: error: write to reserved register 'R7'
235 | arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->cap_id, 0,
| ^
include/linux/arm-smccc.h:570:4: note: expanded from macro 'arm_smccc_1_1_invoke'
570 | arm_smccc_1_1_smc(__VA_ARGS__); \
| ^
include/linux/arm-smccc.h:513:48: note: expanded from macro 'arm_smccc_1_1_smc'
513 | #define arm_smccc_1_1_smc(...) __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
| ^
include/linux/arm-smccc.h:400:24: note: expanded from macro 'SMCCC_SMC_INST'
400 | #define SMCCC_SMC_INST __SMC(0)
| ^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
arch/arm/include/asm/opcodes.h:215:2: note: expanded from macro '__inst_arm_thumb32'
215 | __inst_thumb32(thumb_opcode)
| ^
arch/arm/include/asm/opcodes.h:205:27: note: expanded from macro '__inst_thumb32'
205 | #define __inst_thumb32(x) ___inst_thumb32( \
| ^
arch/arm/include/asm/opcodes.h:230:2: note: expanded from macro '___inst_thumb32'
230 | ".short " __stringify(first) ", " __stringify(second) "\n\t"
| ^
>> drivers/firmware/arm_scmi/scmi_transport_smc.c:235:3: error: write to reserved register 'R7'
include/linux/arm-smccc.h:567:4: note: expanded from macro 'arm_smccc_1_1_invoke'
567 | arm_smccc_1_1_hvc(__VA_ARGS__); \
| ^
include/linux/arm-smccc.h:529:48: note: expanded from macro 'arm_smccc_1_1_hvc'
529 | #define arm_smccc_1_1_hvc(...) __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
| ^
include/linux/arm-smccc.h:401:24: note: expanded from macro 'SMCCC_HVC_INST'
401 | #define SMCCC_HVC_INST __HVC(0)
| ^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
arch/arm/include/asm/opcodes.h:215:2: note: expanded from macro '__inst_arm_thumb32'
215 | __inst_thumb32(thumb_opcode)
| ^
arch/arm/include/asm/opcodes.h:205:27: note: expanded from macro '__inst_thumb32'
205 | #define __inst_thumb32(x) ___inst_thumb32( \
| ^
arch/arm/include/asm/opcodes.h:230:2: note: expanded from macro '___inst_thumb32'
230 | ".short " __stringify(first) ", " __stringify(second) "\n\t"
| ^
>> drivers/firmware/arm_scmi/scmi_transport_smc.c:235:3: error: write to reserved register 'R7'
include/linux/arm-smccc.h:573:4: note: expanded from macro 'arm_smccc_1_1_invoke'
573 | __fail_smccc_1_1(__VA_ARGS__); \
| ^
include/linux/arm-smccc.h:540:8: note: expanded from macro '__fail_smccc_1_1'
540 | asm ("" : \
| ^
drivers/firmware/arm_scmi/scmi_transport_smc.c:238:3: error: write to reserved register 'R7'
238 | arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param_page,
| ^
include/linux/arm-smccc.h:570:4: note: expanded from macro 'arm_smccc_1_1_invoke'
570 | arm_smccc_1_1_smc(__VA_ARGS__); \
| ^
include/linux/arm-smccc.h:513:48: note: expanded from macro 'arm_smccc_1_1_smc'
513 | #define arm_smccc_1_1_smc(...) __arm_smccc_1_1(SMCCC_SMC_INST, __VA_ARGS__)
| ^
include/linux/arm-smccc.h:400:24: note: expanded from macro 'SMCCC_SMC_INST'
400 | #define SMCCC_SMC_INST __SMC(0)
| ^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
arch/arm/include/asm/opcodes.h:215:2: note: expanded from macro '__inst_arm_thumb32'
215 | __inst_thumb32(thumb_opcode)
| ^
arch/arm/include/asm/opcodes.h:205:27: note: expanded from macro '__inst_thumb32'
205 | #define __inst_thumb32(x) ___inst_thumb32( \
| ^
arch/arm/include/asm/opcodes.h:230:2: note: expanded from macro '___inst_thumb32'
230 | ".short " __stringify(first) ", " __stringify(second) "\n\t"
| ^
drivers/firmware/arm_scmi/scmi_transport_smc.c:238:3: error: write to reserved register 'R7'
include/linux/arm-smccc.h:567:4: note: expanded from macro 'arm_smccc_1_1_invoke'
567 | arm_smccc_1_1_hvc(__VA_ARGS__); \
| ^
include/linux/arm-smccc.h:529:48: note: expanded from macro 'arm_smccc_1_1_hvc'
529 | #define arm_smccc_1_1_hvc(...) __arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
| ^
include/linux/arm-smccc.h:401:24: note: expanded from macro 'SMCCC_HVC_INST'
401 | #define SMCCC_HVC_INST __HVC(0)
| ^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
arch/arm/include/asm/opcodes.h:215:2: note: expanded from macro '__inst_arm_thumb32'
215 | __inst_thumb32(thumb_opcode)
| ^
arch/arm/include/asm/opcodes.h:205:27: note: expanded from macro '__inst_thumb32'
205 | #define __inst_thumb32(x) ___inst_thumb32( \
| ^
arch/arm/include/asm/opcodes.h:230:2: note: expanded from macro '___inst_thumb32'
230 | ".short " __stringify(first) ", " __stringify(second) "\n\t"
| ^
drivers/firmware/arm_scmi/scmi_transport_smc.c:238:3: error: write to reserved register 'R7'
include/linux/arm-smccc.h:573:4: note: expanded from macro 'arm_smccc_1_1_invoke'
573 | __fail_smccc_1_1(__VA_ARGS__); \
| ^
include/linux/arm-smccc.h:540:8: note: expanded from macro '__fail_smccc_1_1'
540 | asm ("" : \
| ^
1 warning and 6 errors generated.
vim +/R7 +235 drivers/firmware/arm_scmi/scmi_transport_smc.c
0bfdca8a8661aa drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 129
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 130 static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 131 bool tx)
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 132 {
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 133 struct device *cdev = cinfo->dev;
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 134 unsigned long cap_id = ULONG_MAX;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 135 struct scmi_smc *scmi_info;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 136 resource_size_t size;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 137 struct resource res;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 138 u32 func_id;
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 139 int ret;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 140
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 141 if (!tx)
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 142 return -ENODEV;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 143
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 144 scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL);
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 145 if (!scmi_info)
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 146 return -ENOMEM;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 147
728a057b6ab114 drivers/firmware/arm_scmi/scmi_transport_smc.c Cristian Marussi 2024-07-07 148 scmi_info->shmem = core->shmem->setup_iomap(cinfo, dev, tx);
6c5d3315c40fa0 drivers/firmware/arm_scmi/smc.c Peng Fan 2024-07-07 149 if (IS_ERR(scmi_info->shmem))
6c5d3315c40fa0 drivers/firmware/arm_scmi/smc.c Peng Fan 2024-07-07 150 return PTR_ERR(scmi_info->shmem);
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 151
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 152 ret = of_property_read_u32(dev->of_node, "arm,smc-id", &func_id);
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 153 if (ret < 0)
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 154 return ret;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 155
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 156 if (of_device_is_compatible(dev->of_node, "qcom,scmi-smc")) {
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 @157 void __iomem *ptr = (void __iomem *)scmi_info->shmem + size - 8;
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 158 /* The capability-id is kept in last 8 bytes of shmem.
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 159 * +-------+ <-- 0
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 160 * | shmem |
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 161 * +-------+ <-- size - 8
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 162 * | capId |
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 163 * +-------+ <-- size
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 164 */
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 165 memcpy_fromio(&cap_id, ptr, sizeof(cap_id));
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 166 }
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 167
5f2ea10a808aef drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-05-06 168 if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) {
5f2ea10a808aef drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-05-06 169 scmi_info->param_page = SHMEM_PAGE(res.start);
5f2ea10a808aef drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-05-06 170 scmi_info->param_offset = SHMEM_OFFSET(res.start);
5f2ea10a808aef drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-05-06 171 }
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 172 /*
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 173 * If there is an interrupt named "a2p", then the service and
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 174 * completion of a message is signaled by an interrupt rather than by
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 175 * the return of the SMC call.
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 176 */
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 177 scmi_info->irq = of_irq_get_byname(cdev->of_node, "a2p");
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 178 if (scmi_info->irq > 0) {
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 179 ret = request_irq(scmi_info->irq, smc_msg_done_isr,
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 180 IRQF_NO_SUSPEND, dev_name(dev), scmi_info);
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 181 if (ret) {
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 182 dev_err(dev, "failed to setup SCMI smc irq\n");
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 183 return ret;
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 184 }
f716cbd33f038a drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 185 } else {
f716cbd33f038a drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 186 cinfo->no_completion_irq = true;
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 187 }
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 188
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 189 scmi_info->func_id = func_id;
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 190 scmi_info->cap_id = cap_id;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 191 scmi_info->cinfo = cinfo;
0bfdca8a8661aa drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 192 smc_channel_lock_init(scmi_info);
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 193 cinfo->transport_info = scmi_info;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 194
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 195 return 0;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 196 }
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 197
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 198 static int smc_chan_free(int id, void *p, void *data)
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 199 {
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 200 struct scmi_chan_info *cinfo = p;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 201 struct scmi_smc *scmi_info = cinfo->transport_info;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 202
f1d71576d2c9ec drivers/firmware/arm_scmi/smc.c Andre Przywara 2024-01-26 203 /*
f1d71576d2c9ec drivers/firmware/arm_scmi/smc.c Andre Przywara 2024-01-26 204 * Different protocols might share the same chan info, so a previous
f1d71576d2c9ec drivers/firmware/arm_scmi/smc.c Andre Przywara 2024-01-26 205 * smc_chan_free call might have already freed the structure.
f1d71576d2c9ec drivers/firmware/arm_scmi/smc.c Andre Przywara 2024-01-26 206 */
f1d71576d2c9ec drivers/firmware/arm_scmi/smc.c Andre Przywara 2024-01-26 207 if (!scmi_info)
f1d71576d2c9ec drivers/firmware/arm_scmi/smc.c Andre Przywara 2024-01-26 208 return 0;
f1d71576d2c9ec drivers/firmware/arm_scmi/smc.c Andre Przywara 2024-01-26 209
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 210 /* Ignore any possible further reception on the IRQ path */
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 211 if (scmi_info->irq > 0)
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 212 free_irq(scmi_info->irq, scmi_info);
d1ff11d7ad8704 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2023-07-19 213
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 214 cinfo->transport_info = NULL;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 215 scmi_info->cinfo = NULL;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 216
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 217 return 0;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 218 }
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 219
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 220 static int smc_send_message(struct scmi_chan_info *cinfo,
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 221 struct scmi_xfer *xfer)
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 222 {
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 223 struct scmi_smc *scmi_info = cinfo->transport_info;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 224 struct arm_smccc_res res;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 225
f716cbd33f038a drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 226 /*
0bfdca8a8661aa drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 227 * Channel will be released only once response has been
f716cbd33f038a drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 228 * surely fully retrieved, so after .mark_txdone()
f716cbd33f038a drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 229 */
0bfdca8a8661aa drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 230 smc_channel_lock_acquire(scmi_info, xfer);
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 231
728a057b6ab114 drivers/firmware/arm_scmi/scmi_transport_smc.c Cristian Marussi 2024-07-07 232 core->shmem->tx_prepare(scmi_info->shmem, xfer, cinfo);
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 233
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 234 if (scmi_info->cap_id != ULONG_MAX)
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 @235 arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->cap_id, 0,
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 236 0, 0, 0, 0, 0, &res);
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 237 else
1f17395124a53a drivers/firmware/arm_scmi/smc.c Sudeep Holla 2023-10-09 238 arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param_page,
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 239 scmi_info->param_offset, 0, 0, 0, 0, 0,
da405477e76707 drivers/firmware/arm_scmi/smc.c Nikunj Kela 2023-10-09 240 &res);
dd820ee21d5e07 drivers/firmware/arm_scmi/smc.c Jim Quinlan 2020-12-22 241
f7199cf489027a drivers/firmware/arm_scmi/smc.c Sudeep Holla 2020-04-17 242 /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
f716cbd33f038a drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 243 if (res.a0) {
0bfdca8a8661aa drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 244 smc_channel_lock_release(scmi_info);
f7199cf489027a drivers/firmware/arm_scmi/smc.c Sudeep Holla 2020-04-17 245 return -EOPNOTSUPP;
f716cbd33f038a drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 246 }
f716cbd33f038a drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 247
f7199cf489027a drivers/firmware/arm_scmi/smc.c Sudeep Holla 2020-04-17 248 return 0;
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 249 }
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 250
Hi Cristian,
kernel test robot noticed the following build warnings:
[auto build test WARNING on soc/for-next]
[also build test WARNING on next-20240709]
[cannot apply to linus/master v6.10-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Cristian-Marussi/firmware-arm_scmi-Introduce-setup_shmem_iomap/20240707-082513
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link: https://lore.kernel.org/r/20240707002055.1835121-6-cristian.marussi%40arm.com
patch subject: [PATCH 5/8] firmware: arm_scmi: Make SMC transport a standalone driver
config: arm64-randconfig-r131-20240709 (https://download.01.org/0day-ci/archive/20240710/202407101157.p2OvSFcO-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240710/202407101157.p2OvSFcO-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407101157.p2OvSFcO-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/firmware/arm_scmi/scmi_transport_smc.c:276:24: sparse: sparse: symbol 'scmi_smc_desc' was not declared. Should it be static?
vim +/scmi_smc_desc +276 drivers/firmware/arm_scmi/scmi_transport_smc.c
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 275
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 @276 const struct scmi_desc scmi_smc_desc = {
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 277 .ops = &scmi_smc_ops,
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 278 .max_rx_timeout_ms = 30,
7adb2c8aaaa6a3 drivers/firmware/arm_scmi/smc.c Etienne Carriere 2020-10-08 279 .max_msg = 20,
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 280 .max_msg_size = 128,
117542b81fe7b1 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 281 /*
117542b81fe7b1 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 282 * Setting .sync_cmds_atomic_replies to true for SMC assumes that,
117542b81fe7b1 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 283 * once the SMC instruction has completed successfully, the issued
117542b81fe7b1 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 284 * SCMI command would have been already fully processed by the SCMI
117542b81fe7b1 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 285 * platform firmware and so any possible response value expected
117542b81fe7b1 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 286 * for the issued command will be immmediately ready to be fetched
117542b81fe7b1 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 287 * from the shared memory area.
117542b81fe7b1 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 288 */
117542b81fe7b1 drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 289 .sync_cmds_completed_on_ret = true,
0bfdca8a8661aa drivers/firmware/arm_scmi/smc.c Cristian Marussi 2021-12-20 290 .atomic_enabled = IS_ENABLED(CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE),
1dc6558062dadf drivers/firmware/arm_scmi/smc.c Peng Fan 2020-03-08 291 };
728a057b6ab114 drivers/firmware/arm_scmi/scmi_transport_smc.c Cristian Marussi 2024-07-07 292
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@nxp.com> > >>>>> CC: Nikunj Kela <quic_nkela@quicinc.com> > >>>>> Signed-off-by: Cristian Marussi <cristian.marussi@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%) > >>>>> Hi Nikunj, > >>>>> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig [snip] > >>> > >>>> 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 > 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. I am in the middle of some rework around these SCMI transport characteristics, (max_msg / max_msg_size) that by itself currently are not so well defined in terms of semantic and usage in the codebase, so I am trying to remove any ambiguity and fix innaccuracies. (i.e. even before any DT transport-related binding is introduced). I was trying to understand better how you will use this new binding.... ...I mean, of course you will use it to describe new sizes for the shmem areas and the number of in-flight messages, but is this related to the RIDE platform setup where you use a high number of SCMI instances ? ...if it is, should I guess that you will tailor somehow all of these shmem areas' size to the specific/limited use of those scmi nodes ? (in terms of protocols and messages I mean)... ...or you just need more space in general so you bumped the existing shmem descriptors depending on the platform and you need to align max_msg_size accordingly to the transport needs for that platform ? ..or I am overthinking ? :D IOW what are your use-cases...so that I can try to fit them all in a general way (possibly)... Thanks, Cristian
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 */ }, +}; + +DEFINE_SCMI_TRANSPORT_DRIVER(scmi_smc, scmi_of_match, &core); +module_platform_driver(scmi_smc_driver); + +MODULE_ALIAS("scmi-transport-smc"); +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>"); +MODULE_AUTHOR("Nikunj Kela <quic_nkela@quicinc.com>"); +MODULE_DESCRIPTION("SCMI SMC Transport driver"); +MODULE_LICENSE("GPL");
Make SCMI SMC transport a standalone driver that can be optionally loaded as a module. CC: Peng Fan <peng.fan@nxp.com> CC: Nikunj Kela <quic_nkela@quicinc.com> Signed-off-by: Cristian Marussi <cristian.marussi@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%)