| Message ID | 20230718160833.36397-3-quic_nkela@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add qcom hvc/shmem transport | expand |
On 18/07/2023 18:08, Nikunj Kela wrote: > 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. > ... > + > +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) Why do you pass unused arguments? > +{ > + mutex_lock(&scmi_info->shmem_lock); Why do you need these wrappers? Why not using mutexes directly? If they are needed, then add lock __acquires annotation. > +} > + > +static inline void qcom_hvc_channel_lock_release(struct scmi_qcom_hvc > + *scmi_info) Same comments. > +{ > + 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")) You leak here reference. > + 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. Best regards, Krzysztof
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/ > + */ > + > +#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> This is here because the original code uses spin_until_cond(). > +#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; One architecture-independent and one architecture-dependent parameter, see below. > +}; > + > +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); 80-char is a nice guideline, but this would be easier to read if not line-wrapped. > + > + 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); Just inline these three lock-related methods, saves the reader from wondering what is actually locked, what a "channel" is (is a "channel" the same thing as a "shm region"?) etc. > +} > + > +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. > + 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). > + > + 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. > + */ > + > + 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. > + 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. > + > + /* > + * 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. > + > + 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 */ Does this hold for your implementation as well? > + 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, I'm confused about the purpose of this number, it serves both as a limit of the number of clients that are currently allowed to prepare messages, and as a limit for how many outstanding transfers waiting for response. Making the prior limit too low will result in the client running into an -ENOMEM which it likely won't recover from - the user experience after getting -ENOMEM on regulator_enable() will be suboptimal... The latter limit would relate to resource limitations on the firmware side. Please make sure that you have accounted for 2.5kB of firmware memory, per agent, in your design. > + .max_msg_size = 128, Regards, Bjorn
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 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. > > > > > > + */ > > > + > > > + 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. 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 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 7/18/2023 12:30 PM, Bjorn Andersson wrote: > On Tue, Jul 18, 2023 at 12:10:16PM -0700, Nikunj Kela wrote: >> 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 >>> [..] > [..] >>>>>> +#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? >> > I'm suggesting that the calling convention is one thing and the > in-memory format for passing the information to the driver is a > different thing. > > Keep the arguments passed in memory architecture-independent (i.e. make > it u64). > > If you're saying that the calling convention would be different on a > 32-bit system, then you are also saying that your driver _is_ 64-bit > specific. Please confirm what the size of your capability id would be in > such a system and make sure the Kconfig and/or the code, reflects > reality. > > Thanks, > Bjorn It is confirmed that cap-id will be 32 bit long on 32bit machines. That said, I will make changes to use last 16bytes irrespective of the architecture.
On 18/07/2023 23:16, Nikunj Kela wrote: > > On 7/18/2023 11:42 AM, Krzysztof Kozlowski wrote: >> 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. > Thanks, I thought you were referring to kzalloc cleanup. Will include > this fix. BTW, you may need to fix mailbox.c as well. Indeed, thanks. Best regards, Krzysztof
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
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