| Message ID | 20230718160833.36397-3-quic_nkela@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add qcom hvc/shmem transport | expand |
On 18/07/2023 20:25, Nikunj Kela wrote: >>> + >>> + scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL); >>> + if (!scmi_info) >>> + return -ENOMEM; >>> + >>> + np = of_parse_phandle(cdev->of_node, "shmem", 0); >>> + if (!of_device_is_compatible(np, "arm,scmi-shmem")) >> You leak here reference. > Wouldn't the devm_* API take care of that implicitly? It is same in > smc.c as well. Thanks for bringing my attention to this. I sent a fix for smc.c. Fix your patch as well, please. >>> + return -ENXIO; >>> + >>> + ret = of_address_to_resource(np, 0, &res); >>> + of_node_put(np); >>> + if (ret) { >>> + dev_err(cdev, "failed to get SCMI Tx shared memory\n"); >>> + return ret; >>> + } >>> + >>> + size = resource_size(&res); >>> + >>> + /* let's map 2 additional ulong since >>> + * func-id & capability-id are kept after shmem. >>> + * +-------+ >>> + * | | >>> + * | shmem | >>> + * | | >>> + * | | >>> + * +-------+ <-- size >>> + * | funcId| >>> + * +-------+ <-- size + sizeof(ulong) >>> + * | capId | >>> + * +-------+ <-- size + 2*sizeof(ulong) >>> + */ >>> + >>> + scmi_info->shmem = devm_ioremap(dev, res.start, >>> + size + 2 * sizeof(unsigned long)); >>> + if (!scmi_info->shmem) { >>> + dev_err(dev, "failed to ioremap SCMI Tx shared memory\n"); >>> + return -EADDRNOTAVAIL; >>> + } >>> + >>> + func_id = readl((void *)(scmi_info->shmem) + size); >>> + >>> +#ifdef CONFIG_ARM64 >>> + cap_id = readq((void *)(scmi_info->shmem) + size + >>> + sizeof(unsigned long)); >>> +#else >>> + cap_id = readl((void *)(scmi_info->shmem) + size + >>> + sizeof(unsigned long)); >>> +#endif >>> + >>> + /* >>> + * If there is an interrupt named "a2p", then the service and >>> + * completion of a message is signaled by an interrupt rather than by >>> + * the return of the hvc call. >>> + */ >>> + irq = of_irq_get_byname(cdev->of_node, "a2p"); >>> + if (irq > 0) { >>> + ret = devm_request_irq(dev, irq, qcom_hvc_msg_done_isr, >>> + IRQF_NO_SUSPEND, >>> + dev_name(dev), scmi_info); >>> + if (ret) { >>> + dev_err(dev, "failed to setup SCMI completion irq\n"); >> return dev_err_probe, unless this is not called in probe... but then >> using devm-interface raises questions. > This is copied as is from existing smc.c I understand and I hope you understand the code you copied. If there is a bug in existing code, please do not copy it to new code (like leaking OF node reference). Best regards, Krzysztof
On 7/18/2023 12:07 PM, Bjorn Andersson wrote: > On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote: >> On 7/18/2023 11:29 AM, Bjorn Andersson wrote: >>> On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote: >>>> diff --git a/drivers/firmware/arm_scmi/qcom_hvc.c b/drivers/firmware/arm_scmi/qcom_hvc.c > [..] >>>> +static int qcom_hvc_chan_setup(struct scmi_chan_info *cinfo, >>>> + struct device *dev, bool tx) >>>> +{ > [..] >>>> + /* let's map 2 additional ulong since >>>> + * func-id & capability-id are kept after shmem. >>>> + * +-------+ >>>> + * | | >>>> + * | shmem | >>>> + * | | >>>> + * | | >>>> + * +-------+ <-- size >>>> + * | funcId| >>>> + * +-------+ <-- size + sizeof(ulong) >>>> + * | capId | >>>> + * +-------+ <-- size + 2*sizeof(ulong) >>> Relying on an undocumented convention that following the region >>> specified in DeviceTree are two architecture specifically sized integers >>> isn't good practice. >>> >>> This should be covered by the DeviceTree binding, in one way or another. >> ok. Usually, DTBs don't allow non-hw properties in the dtb. I can try adding >> a property as cap-id-width if its allowed. >> > If you remove the additional part, per the next comment, DeviceTree > would be oblivious to these properties. I'll don't know if the > DeviceTree people have any concerns/suggestions about this. ok. > >>>> + */ >>>> + >>>> + scmi_info->shmem = devm_ioremap(dev, res.start, >>>> + size + 2 * sizeof(unsigned long)); >>> I don't find any code that uses the size of the defined shm, so I don't >>> think you need to do this dance. >> Right! I can remove the addition part. >>>> + if (!scmi_info->shmem) { >>>> + dev_err(dev, "failed to ioremap SCMI Tx shared memory\n"); >>>> + return -EADDRNOTAVAIL; >>>> + } >>>> + >>>> + func_id = readl((void *)(scmi_info->shmem) + size); >>>> + >>>> +#ifdef CONFIG_ARM64 >>>> + cap_id = readq((void *)(scmi_info->shmem) + size + >>>> + sizeof(unsigned long)); >>>> +#else >>>> + cap_id = readl((void *)(scmi_info->shmem) + size + >>>> + sizeof(unsigned long)); >>>> +#endif >>> Please don't make the in-memory representation depend on architecture >>> specific data types. Quite likely you didn't compile test one of these >>> variants? >>> >>> Just define the in-memory representation as u32 + u64. >> I tested this for ARM64, I didn't test it for 32bit since Hypervisor doesn't >> support it currently. In future, it may add 32 bit support too. > I'd not be surprised if the capability id is 64 bit on a 32-bit machine > as well, it's not really a property of the architecture. on 32bit machine, you will have to use SMC32 convention. lt will mean that the parameters can only be 32 bit long. If you keep cap-id 64 bit in 32bit machines, then it has to be passed in two parameters. Are you suggesting, I make this driver dependent on ARM64 and only care about 64 bit for now? > > But regardless, always using 64 bits in your memory representation will > at worst waste a few bytes. But the result is a better defined > interface, and you can avoid the conditional code. > > Regards, > Bjorn
On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote: > > On 7/18/2023 11:29 AM, Bjorn Andersson wrote: > > On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote: > > > diff --git a/drivers/firmware/arm_scmi/qcom_hvc.c b/drivers/firmware/arm_scmi/qcom_hvc.c > > > new file mode 100644 > > > index 000000000000..3b6096d8fe67 > > > --- /dev/null > > > +++ b/drivers/firmware/arm_scmi/qcom_hvc.c > > > @@ -0,0 +1,241 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * System Control and Management Interface (SCMI) Message > > > + * Qualcomm HVC/shmem Transport driver > > > + * > > > + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. > > > + * Copyright 2020 NXP > > > + * > > > + * This is copied from drivers/firmware/arm_scmi/smc.c > > s/copied from/based on/ > ok. Hi Nikunj, > > > > > + */ > > > + > > > +#include <linux/arm-smccc.h> > > > +#include <linux/device.h> > > > +#include <linux/err.h> > > > +#include <linux/interrupt.h> > > > +#include <linux/mutex.h> > > > +#include <linux/of.h> > > > +#include <linux/of_address.h> [snip] > > > + > > > +static inline void > > > +qcom_hvc_channel_lock_acquire(struct scmi_qcom_hvc *scmi_info, > > > + struct scmi_xfer *xfer __maybe_unused) > > > +{ > > You claim above that you copied smc.c, but you don't mention that you > > dropped the support for transfers from atomic mode. Please capture this > > in the commit message, so that someone looking at this in the future > > knows why you made this choice. > > At the moment, we dont have any requirement to support atomicity. Will add a > comment in the commit message. > As said no atomic support no wrappers needed. > > > > > > + mutex_lock(&scmi_info->shmem_lock); > > > +} > > > + > > > +static inline void qcom_hvc_channel_lock_release(struct scmi_qcom_hvc > > > + *scmi_info) > > > +{ > > > + mutex_unlock(&scmi_info->shmem_lock); > > > +} > > > + > > > +static int qcom_hvc_chan_setup(struct scmi_chan_info *cinfo, > > > + struct device *dev, bool tx) > > > +{ > > > + struct device *cdev = cinfo->dev; > > > + struct scmi_qcom_hvc *scmi_info; > > > + resource_size_t size; > > > + struct resource res; > > > + struct device_node *np; > > > + unsigned long cap_id; > > > + u32 func_id; > > > + int ret, irq; > > Please declare one variable per line, and please sort them by length, in > > descending order (i.e. reverse Christmas tree). > ok > > > > > + > > > + if (!tx) > > > + return -ENODEV; > > > + > > > + scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL); > > > + if (!scmi_info) > > > + return -ENOMEM; > > > + > > > + np = of_parse_phandle(cdev->of_node, "shmem", 0); > > > + if (!of_device_is_compatible(np, "arm,scmi-shmem")) > > > + return -ENXIO; > > > + > > > + ret = of_address_to_resource(np, 0, &res); > > > + of_node_put(np); > > > + if (ret) { > > > + dev_err(cdev, "failed to get SCMI Tx shared memory\n"); > > > + return ret; > > > + } > > > + > > > + size = resource_size(&res); > > > + > > > + /* let's map 2 additional ulong since > > > + * func-id & capability-id are kept after shmem. > > > + * +-------+ > > > + * | | > > > + * | shmem | > > > + * | | > > > + * | | > > > + * +-------+ <-- size > > > + * | funcId| > > > + * +-------+ <-- size + sizeof(ulong) > > > + * | capId | > > > + * +-------+ <-- size + 2*sizeof(ulong) > > Relying on an undocumented convention that following the region > > specified in DeviceTree are two architecture specifically sized integers > > isn't good practice. > > > > This should be covered by the DeviceTree binding, in one way or another. > > ok. Usually, DTBs don't allow non-hw properties in the dtb. I can try adding > a property as cap-id-width if its allowed. > This is sort of transport configuration so maybe it could be placed on a shmem on its own, but it seems difficult that the binding can be accepted since, as you said, is not an HW description BUT indeed configuration. > > > > > > + */ > > > + > > > + scmi_info->shmem = devm_ioremap(dev, res.start, > > > + size + 2 * sizeof(unsigned long)); > > I don't find any code that uses the size of the defined shm, so I don't > > think you need to do this dance. > Right! I can remove the addition part. > > Mmm...but can you access this trailing config bytes if you dont ioremap it ? > > > + if (!scmi_info->shmem) { > > > + dev_err(dev, "failed to ioremap SCMI Tx shared memory\n"); > > > + return -EADDRNOTAVAIL; > > > + } > > > + > > > + func_id = readl((void *)(scmi_info->shmem) + size); > > > + > > > +#ifdef CONFIG_ARM64 > > > + cap_id = readq((void *)(scmi_info->shmem) + size + > > > + sizeof(unsigned long)); > > > +#else > > > + cap_id = readl((void *)(scmi_info->shmem) + size + > > > + sizeof(unsigned long)); > > > +#endif > > Please don't make the in-memory representation depend on architecture > > specific data types. Quite likely you didn't compile test one of these > > variants? > > > > Just define the in-memory representation as u32 + u64. > I tested this for ARM64, I didn't test it for 32bit since Hypervisor doesn't > support it currently. In future, it may add 32 bit support too. > > > + > > > + /* > > > + * If there is an interrupt named "a2p", then the service and > > > + * completion of a message is signaled by an interrupt rather than by > > > + * the return of the hvc call. > > > + */ > > > + irq = of_irq_get_byname(cdev->of_node, "a2p"); > > > + if (irq > 0) { > > > + ret = devm_request_irq(dev, irq, qcom_hvc_msg_done_isr, > > > + IRQF_NO_SUSPEND, > > > + dev_name(dev), scmi_info); > > > + if (ret) { > > > + dev_err(dev, "failed to setup SCMI completion irq\n"); > > > + return ret; > > > + } > > > + } else { > > > + cinfo->no_completion_irq = true; > > > + } > > > + > > > + scmi_info->func_id = func_id; > > > + scmi_info->cap_id = cap_id; > > > + scmi_info->cinfo = cinfo; > > > + qcom_hvc_channel_lock_init(scmi_info); > > > + cinfo->transport_info = scmi_info; > > > + > > > + return 0; > > > +} > > > + > > > +static int qcom_hvc_chan_free(int id, void *p, void *data) > > > +{ > > > + struct scmi_chan_info *cinfo = p; > > > + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info; > > > + > > > + cinfo->transport_info = NULL; > > > + scmi_info->cinfo = NULL; > > Your a2p interrupt is cleaned up using devres, which will happen at a > > later point. So just setting cinfo to NULL here would cause an interrupt > > to trigger qcom_hvc_msg_done_isr() which will call scmi_rx_callback() > > which happily will dereference that NULL. > Ok, will add a check in ISR. > > Other transports here takes care to block/inhibit any further possible message reception with a transport/subsystem dependent method (like masking the IRQ calling into mbox subsys or breaking the virtio device); I suppose here you could also unregister the ISR before clearing to NULL. (and I'll need to post a similar fix for SMC...) Thanks, Cristian
On 7/19/2023 3:55 AM, Cristian Marussi wrote: > On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote: >> On 7/18/2023 11:29 AM, Bjorn Andersson wrote: >>> On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote: >>>> diff --git a/drivers/firmware/arm_scmi/qcom_hvc.c b/drivers/firmware/arm_scmi/qcom_hvc.c >>>> new file mode 100644 >>>> index 000000000000..3b6096d8fe67 >>>> --- /dev/null >>>> +++ b/drivers/firmware/arm_scmi/qcom_hvc.c >>>> @@ -0,0 +1,241 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * System Control and Management Interface (SCMI) Message >>>> + * Qualcomm HVC/shmem Transport driver >>>> + * >>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. >>>> + * Copyright 2020 NXP >>>> + * >>>> + * This is copied from drivers/firmware/arm_scmi/smc.c >>> s/copied from/based on/ >> ok. > Hi Nikunj, > >>>> + */ >>>> + >>>> +#include <linux/arm-smccc.h> >>>> +#include <linux/device.h> >>>> +#include <linux/err.h> >>>> +#include <linux/interrupt.h> >>>> +#include <linux/mutex.h> >>>> +#include <linux/of.h> >>>> +#include <linux/of_address.h> > [snip] > >>>> + >>>> +static inline void >>>> +qcom_hvc_channel_lock_acquire(struct scmi_qcom_hvc *scmi_info, >>>> + struct scmi_xfer *xfer __maybe_unused) >>>> +{ >>> You claim above that you copied smc.c, but you don't mention that you >>> dropped the support for transfers from atomic mode. Please capture this >>> in the commit message, so that someone looking at this in the future >>> knows why you made this choice. >> At the moment, we dont have any requirement to support atomicity. Will add a >> comment in the commit message. >> > As said no atomic support no wrappers needed. ACK! > >>>> + mutex_lock(&scmi_info->shmem_lock); >>>> +} >>>> + >>>> +static inline void qcom_hvc_channel_lock_release(struct scmi_qcom_hvc >>>> + *scmi_info) >>>> +{ >>>> + mutex_unlock(&scmi_info->shmem_lock); >>>> +} >>>> + >>>> +static int qcom_hvc_chan_setup(struct scmi_chan_info *cinfo, >>>> + struct device *dev, bool tx) >>>> +{ >>>> + struct device *cdev = cinfo->dev; >>>> + struct scmi_qcom_hvc *scmi_info; >>>> + resource_size_t size; >>>> + struct resource res; >>>> + struct device_node *np; >>>> + unsigned long cap_id; >>>> + u32 func_id; >>>> + int ret, irq; >>> Please declare one variable per line, and please sort them by length, in >>> descending order (i.e. reverse Christmas tree). >> ok >>>> + >>>> + if (!tx) >>>> + return -ENODEV; >>>> + >>>> + scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL); >>>> + if (!scmi_info) >>>> + return -ENOMEM; >>>> + >>>> + np = of_parse_phandle(cdev->of_node, "shmem", 0); >>>> + if (!of_device_is_compatible(np, "arm,scmi-shmem")) >>>> + return -ENXIO; >>>> + >>>> + ret = of_address_to_resource(np, 0, &res); >>>> + of_node_put(np); >>>> + if (ret) { >>>> + dev_err(cdev, "failed to get SCMI Tx shared memory\n"); >>>> + return ret; >>>> + } >>>> + >>>> + size = resource_size(&res); >>>> + >>>> + /* let's map 2 additional ulong since >>>> + * func-id & capability-id are kept after shmem. >>>> + * +-------+ >>>> + * | | >>>> + * | shmem | >>>> + * | | >>>> + * | | >>>> + * +-------+ <-- size >>>> + * | funcId| >>>> + * +-------+ <-- size + sizeof(ulong) >>>> + * | capId | >>>> + * +-------+ <-- size + 2*sizeof(ulong) >>> Relying on an undocumented convention that following the region >>> specified in DeviceTree are two architecture specifically sized integers >>> isn't good practice. >>> >>> This should be covered by the DeviceTree binding, in one way or another. >> ok. Usually, DTBs don't allow non-hw properties in the dtb. I can try adding >> a property as cap-id-width if its allowed. >> > This is sort of transport configuration so maybe it could be placed on a > shmem on its own, but it seems difficult that the binding can be accepted > since, as you said, is not an HW description BUT indeed configuration. Ok. > >>>> + */ >>>> + >>>> + scmi_info->shmem = devm_ioremap(dev, res.start, >>>> + size + 2 * sizeof(unsigned long)); >>> I don't find any code that uses the size of the defined shm, so I don't >>> think you need to do this dance. >> Right! I can remove the addition part. > Mmm...but can you access this trailing config bytes if you dont ioremap it ? No, I meant, the last 16 bytes of each channel can be used so we don't need to remap 2 extra ulong. > >>>> + if (!scmi_info->shmem) { >>>> + dev_err(dev, "failed to ioremap SCMI Tx shared memory\n"); >>>> + return -EADDRNOTAVAIL; >>>> + } >>>> + >>>> + func_id = readl((void *)(scmi_info->shmem) + size); >>>> + >>>> +#ifdef CONFIG_ARM64 >>>> + cap_id = readq((void *)(scmi_info->shmem) + size + >>>> + sizeof(unsigned long)); >>>> +#else >>>> + cap_id = readl((void *)(scmi_info->shmem) + size + >>>> + sizeof(unsigned long)); >>>> +#endif >>> Please don't make the in-memory representation depend on architecture >>> specific data types. Quite likely you didn't compile test one of these >>> variants? >>> >>> Just define the in-memory representation as u32 + u64. >> I tested this for ARM64, I didn't test it for 32bit since Hypervisor doesn't >> support it currently. In future, it may add 32 bit support too. >>>> + >>>> + /* >>>> + * If there is an interrupt named "a2p", then the service and >>>> + * completion of a message is signaled by an interrupt rather than by >>>> + * the return of the hvc call. >>>> + */ >>>> + irq = of_irq_get_byname(cdev->of_node, "a2p"); >>>> + if (irq > 0) { >>>> + ret = devm_request_irq(dev, irq, qcom_hvc_msg_done_isr, >>>> + IRQF_NO_SUSPEND, >>>> + dev_name(dev), scmi_info); >>>> + if (ret) { >>>> + dev_err(dev, "failed to setup SCMI completion irq\n"); >>>> + return ret; >>>> + } >>>> + } else { >>>> + cinfo->no_completion_irq = true; >>>> + } >>>> + >>>> + scmi_info->func_id = func_id; >>>> + scmi_info->cap_id = cap_id; >>>> + scmi_info->cinfo = cinfo; >>>> + qcom_hvc_channel_lock_init(scmi_info); >>>> + cinfo->transport_info = scmi_info; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int qcom_hvc_chan_free(int id, void *p, void *data) >>>> +{ >>>> + struct scmi_chan_info *cinfo = p; >>>> + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info; >>>> + >>>> + cinfo->transport_info = NULL; >>>> + scmi_info->cinfo = NULL; >>> Your a2p interrupt is cleaned up using devres, which will happen at a >>> later point. So just setting cinfo to NULL here would cause an interrupt >>> to trigger qcom_hvc_msg_done_isr() which will call scmi_rx_callback() >>> which happily will dereference that NULL. >> Ok, will add a check in ISR. > Other transports here takes care to block/inhibit any further possible > message reception with a transport/subsystem dependent method (like masking > the IRQ calling into mbox subsys or breaking the virtio device); I suppose > here you could also unregister the ISR before clearing to NULL. > (and I'll need to post a similar fix for SMC...) Thanks, will do! > > Thanks, > Cristian
Hi Nikunj, kernel test robot noticed the following build warnings: [auto build test WARNING on robh/for-next] [also build test WARNING on linus/master v6.5-rc2 next-20230721] [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/Nikunj-Kela/dt-bindings-arm-Add-qcom-specific-hvc-transport-for-SCMI/20230719-001116 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next patch link: https://lore.kernel.org/r/20230718160833.36397-3-quic_nkela%40quicinc.com patch subject: [PATCH 2/2] firmware: arm_scmi: Add qcom hvc/shmem transport config: arm64-randconfig-r083-20230723 (https://download.01.org/0day-ci/archive/20230723/202307231034.5C5lj4Dv-lkp@intel.com/config) compiler: aarch64-linux-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230723/202307231034.5C5lj4Dv-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/202307231034.5C5lj4Dv-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/firmware/arm_scmi/qcom_hvc.c:136:26: sparse: sparse: cast removes address space '__iomem' of expression >> drivers/firmware/arm_scmi/qcom_hvc.c:136:52: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const volatile [noderef] __iomem *addr @@ got void * @@ drivers/firmware/arm_scmi/qcom_hvc.c:136:52: sparse: expected void const volatile [noderef] __iomem *addr drivers/firmware/arm_scmi/qcom_hvc.c:136:52: sparse: got void * drivers/firmware/arm_scmi/qcom_hvc.c:139:25: sparse: sparse: cast removes address space '__iomem' of expression drivers/firmware/arm_scmi/qcom_hvc.c:139:58: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const volatile [noderef] __iomem *addr @@ got void * @@ drivers/firmware/arm_scmi/qcom_hvc.c:139:58: sparse: expected void const volatile [noderef] __iomem *addr drivers/firmware/arm_scmi/qcom_hvc.c:139:58: sparse: got void * vim +/__iomem +136 drivers/firmware/arm_scmi/qcom_hvc.c 82 83 static int qcom_hvc_chan_setup(struct scmi_chan_info *cinfo, 84 struct device *dev, bool tx) 85 { 86 struct device *cdev = cinfo->dev; 87 struct scmi_qcom_hvc *scmi_info; 88 resource_size_t size; 89 struct resource res; 90 struct device_node *np; 91 unsigned long cap_id; 92 u32 func_id; 93 int ret, irq; 94 95 if (!tx) 96 return -ENODEV; 97 98 scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL); 99 if (!scmi_info) 100 return -ENOMEM; 101 102 np = of_parse_phandle(cdev->of_node, "shmem", 0); 103 if (!of_device_is_compatible(np, "arm,scmi-shmem")) 104 return -ENXIO; 105 106 ret = of_address_to_resource(np, 0, &res); 107 of_node_put(np); 108 if (ret) { 109 dev_err(cdev, "failed to get SCMI Tx shared memory\n"); 110 return ret; 111 } 112 113 size = resource_size(&res); 114 115 /* let's map 2 additional ulong since 116 * func-id & capability-id are kept after shmem. 117 * +-------+ 118 * | | 119 * | shmem | 120 * | | 121 * | | 122 * +-------+ <-- size 123 * | funcId| 124 * +-------+ <-- size + sizeof(ulong) 125 * | capId | 126 * +-------+ <-- size + 2*sizeof(ulong) 127 */ 128 129 scmi_info->shmem = devm_ioremap(dev, res.start, 130 size + 2 * sizeof(unsigned long)); 131 if (!scmi_info->shmem) { 132 dev_err(dev, "failed to ioremap SCMI Tx shared memory\n"); 133 return -EADDRNOTAVAIL; 134 } 135 > 136 func_id = readl((void *)(scmi_info->shmem) + size); 137
diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig index ea0f5083ac47..40d07329ebf7 100644 --- a/drivers/firmware/arm_scmi/Kconfig +++ b/drivers/firmware/arm_scmi/Kconfig @@ -99,6 +99,19 @@ config ARM_SCMI_TRANSPORT_OPTEE If you want the ARM SCMI PROTOCOL stack to include support for a transport based on OP-TEE SCMI service, answer Y. +config ARM_SCMI_TRANSPORT_QCOM_HVC + bool "SCMI transport based on hvc doorbell & shmem for Qualcomm SoCs" + depends on ARCH_QCOM + select ARM_SCMI_HAVE_TRANSPORT + select ARM_SCMI_HAVE_SHMEM + default y + help + Enable hvc doorbell & shmem based transport for SCMI. + + If you want the ARM SCMI PROTOCOL stack to include support for a + hvc doorbell and shmem transport on Qualcomm virtual platforms, + answer Y. + config ARM_SCMI_TRANSPORT_SMC bool "SCMI transport based on SMC" depends on HAVE_ARM_SMCCC_DISCOVERY diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile index b31d78fa66cc..ba1ff5893ec0 100644 --- a/drivers/firmware/arm_scmi/Makefile +++ b/drivers/firmware/arm_scmi/Makefile @@ -10,6 +10,7 @@ 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 +scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_QCOM_HVC) += qcom_hvc.o scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index c46dc5215af7..5c98cbb1278b 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -298,6 +298,9 @@ extern const struct scmi_desc scmi_virtio_desc; #ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE extern const struct scmi_desc scmi_optee_desc; #endif +#ifdef CONFIG_ARM_SCMI_TRANSPORT_QCOM_HVC +extern const struct scmi_desc scmi_qcom_hvc_desc; +#endif void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv); diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index b5957cc12fee..c54519596c29 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -2918,6 +2918,10 @@ static const struct of_device_id scmi_of_match[] = { #endif #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc}, +#endif +#ifdef CONFIG_ARM_SCMI_TRANSPORT_QCOM_HVC + { .compatible = "qcom,scmi-hvc-shmem", + .data = &scmi_qcom_hvc_desc }, #endif { /* Sentinel */ }, }; diff --git a/drivers/firmware/arm_scmi/qcom_hvc.c b/drivers/firmware/arm_scmi/qcom_hvc.c new file mode 100644 index 000000000000..3b6096d8fe67 --- /dev/null +++ b/drivers/firmware/arm_scmi/qcom_hvc.c @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * System Control and Management Interface (SCMI) Message + * Qualcomm HVC/shmem Transport driver + * + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved. + * Copyright 2020 NXP + * + * This is copied from drivers/firmware/arm_scmi/smc.c + */ + +#include <linux/arm-smccc.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/processor.h> +#include <linux/slab.h> + +#include "common.h" + +/** + * struct scmi_qcom_hvc - Structure representing a SCMI qcom hvc transport + * + * @cinfo: SCMI channel info + * @shmem: Transmit/Receive shared memory area + * @shmem_lock: Lock to protect access to Tx/Rx shared memory area. + * @func_id: hvc call function-id + * @cap_id: hvc doorbell's capability id + */ + +struct scmi_qcom_hvc { + struct scmi_chan_info *cinfo; + struct scmi_shared_mem __iomem *shmem; + /* Protect access to shmem area */ + struct mutex shmem_lock; + u32 func_id; + unsigned long cap_id; +}; + +static irqreturn_t qcom_hvc_msg_done_isr(int irq, void *data) +{ + struct scmi_qcom_hvc *scmi_info = data; + + scmi_rx_callback(scmi_info->cinfo, + shmem_read_header(scmi_info->shmem), NULL); + + return IRQ_HANDLED; +} + +static bool qcom_hvc_chan_available(struct device_node *of_node, int idx) +{ + struct device_node *np = of_parse_phandle(of_node, "shmem", 0); + + if (!np) + return false; + + of_node_put(np); + return true; +} + +static inline void qcom_hvc_channel_lock_init(struct scmi_qcom_hvc *scmi_info) +{ + mutex_init(&scmi_info->shmem_lock); +} + +static inline void +qcom_hvc_channel_lock_acquire(struct scmi_qcom_hvc *scmi_info, + struct scmi_xfer *xfer __maybe_unused) +{ + mutex_lock(&scmi_info->shmem_lock); +} + +static inline void qcom_hvc_channel_lock_release(struct scmi_qcom_hvc + *scmi_info) +{ + mutex_unlock(&scmi_info->shmem_lock); +} + +static int qcom_hvc_chan_setup(struct scmi_chan_info *cinfo, + struct device *dev, bool tx) +{ + struct device *cdev = cinfo->dev; + struct scmi_qcom_hvc *scmi_info; + resource_size_t size; + struct resource res; + struct device_node *np; + unsigned long cap_id; + u32 func_id; + int ret, irq; + + if (!tx) + return -ENODEV; + + scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL); + if (!scmi_info) + return -ENOMEM; + + np = of_parse_phandle(cdev->of_node, "shmem", 0); + if (!of_device_is_compatible(np, "arm,scmi-shmem")) + return -ENXIO; + + ret = of_address_to_resource(np, 0, &res); + of_node_put(np); + if (ret) { + dev_err(cdev, "failed to get SCMI Tx shared memory\n"); + return ret; + } + + size = resource_size(&res); + + /* let's map 2 additional ulong since + * func-id & capability-id are kept after shmem. + * +-------+ + * | | + * | shmem | + * | | + * | | + * +-------+ <-- size + * | funcId| + * +-------+ <-- size + sizeof(ulong) + * | capId | + * +-------+ <-- size + 2*sizeof(ulong) + */ + + scmi_info->shmem = devm_ioremap(dev, res.start, + size + 2 * sizeof(unsigned long)); + if (!scmi_info->shmem) { + dev_err(dev, "failed to ioremap SCMI Tx shared memory\n"); + return -EADDRNOTAVAIL; + } + + func_id = readl((void *)(scmi_info->shmem) + size); + +#ifdef CONFIG_ARM64 + cap_id = readq((void *)(scmi_info->shmem) + size + + sizeof(unsigned long)); +#else + cap_id = readl((void *)(scmi_info->shmem) + size + + sizeof(unsigned long)); +#endif + + /* + * If there is an interrupt named "a2p", then the service and + * completion of a message is signaled by an interrupt rather than by + * the return of the hvc call. + */ + irq = of_irq_get_byname(cdev->of_node, "a2p"); + if (irq > 0) { + ret = devm_request_irq(dev, irq, qcom_hvc_msg_done_isr, + IRQF_NO_SUSPEND, + dev_name(dev), scmi_info); + if (ret) { + dev_err(dev, "failed to setup SCMI completion irq\n"); + return ret; + } + } else { + cinfo->no_completion_irq = true; + } + + scmi_info->func_id = func_id; + scmi_info->cap_id = cap_id; + scmi_info->cinfo = cinfo; + qcom_hvc_channel_lock_init(scmi_info); + cinfo->transport_info = scmi_info; + + return 0; +} + +static int qcom_hvc_chan_free(int id, void *p, void *data) +{ + struct scmi_chan_info *cinfo = p; + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info; + + cinfo->transport_info = NULL; + scmi_info->cinfo = NULL; + + return 0; +} + +static int qcom_hvc_send_message(struct scmi_chan_info *cinfo, + struct scmi_xfer *xfer) +{ + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info; + struct arm_smccc_res res; + + /* + * Channel will be released only once response has been + * surely fully retrieved, so after .mark_txdone() + */ + qcom_hvc_channel_lock_acquire(scmi_info, xfer); + + shmem_tx_prepare(scmi_info->shmem, xfer, cinfo); + + arm_smccc_1_1_hvc(scmi_info->func_id, scmi_info->cap_id, + 0, 0, 0, 0, 0, 0, &res); + + /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */ + if (res.a0) { + qcom_hvc_channel_lock_release(scmi_info); + return -EOPNOTSUPP; + } + + return 0; +} + +static void qcom_hvc_fetch_response(struct scmi_chan_info *cinfo, + struct scmi_xfer *xfer) +{ + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info; + + shmem_fetch_response(scmi_info->shmem, xfer); +} + +static void qcom_hvc_mark_txdone(struct scmi_chan_info *cinfo, int ret, + struct scmi_xfer *__unused) +{ + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info; + + qcom_hvc_channel_lock_release(scmi_info); +} + +static const struct scmi_transport_ops scmi_qcom_hvc_ops = { + .chan_available = qcom_hvc_chan_available, + .chan_setup = qcom_hvc_chan_setup, + .chan_free = qcom_hvc_chan_free, + .send_message = qcom_hvc_send_message, + .mark_txdone = qcom_hvc_mark_txdone, + .fetch_response = qcom_hvc_fetch_response, +}; + +const struct scmi_desc scmi_qcom_hvc_desc = { + .ops = &scmi_qcom_hvc_ops, + .max_rx_timeout_ms = 30, + .max_msg = 20, + .max_msg_size = 128, + .sync_cmds_completed_on_ret = true, +};
Add a new transport channel to the SCMI firmware interface driver for SCMI message exchange on Qualcomm virtual platforms. The hypervisor associates an object-id also known as capability-id with each hvc doorbell object. The capability-id is used to identify the doorbell from the VM's capability namespace, similar to a file-descriptor. The hypervisor, in addition to the function-id, expects the capability-id to be passed in x1 register when HVC call is invoked. The qcom hvc doorbell/shared memory transport uses a statically defined shared memory region that binds with "arm,scmi-shmem" device tree node. The function-id & capability-id are allocated by the hypervisor on bootup and are stored in the shmem region by the firmware before starting Linux. Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> --- drivers/firmware/arm_scmi/Kconfig | 13 ++ drivers/firmware/arm_scmi/Makefile | 1 + drivers/firmware/arm_scmi/common.h | 3 + drivers/firmware/arm_scmi/driver.c | 4 + drivers/firmware/arm_scmi/qcom_hvc.c | 241 +++++++++++++++++++++++++++ 5 files changed, 262 insertions(+) create mode 100644 drivers/firmware/arm_scmi/qcom_hvc.c