Messages in this thread | | | Date | Wed, 19 Jul 2023 07:02:24 -0700 | Subject | Re: [PATCH 2/2] firmware: arm_scmi: Add qcom hvc/shmem transport | From | Nikunj Kela <> |
| |
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
| |