| Message ID | 20230724164419.16092-1-quic_nkela@quicinc.com |
|---|---|
| Headers | show |
| Series | Add qcom hvc/shmem transport | expand |
On Mon, Jul 24, 2023 at 09:44:19AM -0700, 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. > > 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. > > Currently, there is no usecase for the atomic support therefore this driver > doesn't include the changes for the same. > Hi Nikunj, so basically this new SCMI transport that you are introducing is just exactly like the existing SMC transport with the only difference that you introduced even another new way to configure func_id, a new cap_id param AND the fact that you use HVC instead of SMC... all of this tied to a new compatible to identify this new transport mechanism.... ..but all in all is just a lot of plain duplicated code to maintain... ...why can't you fit this other smc/hvc transport variant into the existing SMC transport by properly picking and configuring func_id/cap_id and "doorbell" method (SMC vs HVC) in the chan_setup() step ? ..I mean ... you can decide where to pick your params based on compatibles and also you can setup your invokation method (SMC vs HVC) based on those...while keeping all the other stuff exactly the same... ...including support for atomic exchanges...if not, when you'll need that too in your QC_HVC transport you'll have to duplicate also that (and my bugs too probably :P) (... well maybe in this scenario also the transport itself should be renamed from SMC to something more general...) Not sure if I am missing something, or if Sudeep will be horrified by this unifying proposal of mine, but in this series as it stands now I just see a lot of brutally duplicated stuff that just differs by naming and a very minimal change in logic that could be addressed changing and generalizing the original SMC transport code instead. Thanks, Cristian
On 7/25/2023 10:03 AM, Cristian Marussi wrote: > On Mon, Jul 24, 2023 at 09:44:19AM -0700, 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. >> >> 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. >> >> Currently, there is no usecase for the atomic support therefore this driver >> doesn't include the changes for the same. >> > Hi Nikunj, > > so basically this new SCMI transport that you are introducing is just > exactly like the existing SMC transport with the only difference that > you introduced even another new way to configure func_id, a new cap_id > param AND the fact that you use HVC instead of SMC... all of this tied > to a new compatible to identify this new transport mechanism.... > ..but all in all is just a lot of plain duplicated code to maintain... > > ...why can't you fit this other smc/hvc transport variant into the > existing SMC transport by properly picking and configuring func_id/cap_id > and "doorbell" method (SMC vs HVC) in the chan_setup() step ? > > ..I mean ... you can decide where to pick your params based on > compatibles and also you can setup your invokation method (SMC vs HVC) > based on those...while keeping all the other stuff exactly the same... > ...including support for atomic exchanges...if not, when you'll need that > too in your QC_HVC transport you'll have to duplicate also that (and my > bugs too probably :P) > > (... well maybe in this scenario also the transport itself should be > renamed from SMC to something more general...) > > Not sure if I am missing something, or if Sudeep will be horrified by > this unifying proposal of mine, but in this series as it stands now I > just see a lot of brutally duplicated stuff that just differs by naming > and a very minimal change in logic that could be addressed changing and > generalizing the original SMC transport code instead. > > Thanks, > Cristian Hi Christian, I totally agree with you and will be happy to include my changes in smc.c if Sudeep agrees with that approach. Thanks
On 7/25/2023 10:12 AM, Nikunj Kela wrote: > > On 7/25/2023 10:03 AM, Cristian Marussi wrote: >> On Mon, Jul 24, 2023 at 09:44:19AM -0700, 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. >>> >>> 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. >>> >>> Currently, there is no usecase for the atomic support therefore this >>> driver >>> doesn't include the changes for the same. >>> >> Hi Nikunj, >> >> so basically this new SCMI transport that you are introducing is just >> exactly like the existing SMC transport with the only difference that >> you introduced even another new way to configure func_id, a new cap_id >> param AND the fact that you use HVC instead of SMC... all of this tied >> to a new compatible to identify this new transport mechanism.... >> ..but all in all is just a lot of plain duplicated code to maintain... >> >> ...why can't you fit this other smc/hvc transport variant into the >> existing SMC transport by properly picking and configuring >> func_id/cap_id >> and "doorbell" method (SMC vs HVC) in the chan_setup() step ? >> >> ..I mean ... you can decide where to pick your params based on >> compatibles and also you can setup your invokation method (SMC vs HVC) >> based on those...while keeping all the other stuff exactly the same... >> ...including support for atomic exchanges...if not, when you'll need >> that >> too in your QC_HVC transport you'll have to duplicate also that (and my >> bugs too probably :P) >> >> (... well maybe in this scenario also the transport itself should be >> renamed from SMC to something more general...) >> >> Not sure if I am missing something, or if Sudeep will be horrified by >> this unifying proposal of mine, but in this series as it stands now I >> just see a lot of brutally duplicated stuff that just differs by naming >> and a very minimal change in logic that could be addressed changing and >> generalizing the original SMC transport code instead. >> >> Thanks, >> Cristian > > Hi Christian, > > I totally agree with you and will be happy to include my changes in > smc.c if Sudeep agrees with that approach. > > Thanks Hi Sudeep, Could you please provide your feedback on this? Thanks
This change introduce a new transport channel for Qualcomm virtual platforms. The transport is mechanically similar to ARM_SCMI_TRANSPORT_SMC. The difference between the two transports is that a parameter is passed in the hypervisor call to identify which doorbell to assert. This parameter is dynamically generated at runtime on the device and insuitable to pass via the devicetree. The function ID and parameter are stored by firmware in the shmem region. This has been tested on ARM64 virtual Qualcomm platform. --- v2 -> use allOf construct in dtb schema, remove wrappers from mutexes, use architecture independent channel layout v1 -> original patches Nikunj Kela (3): dt-bindings: arm: convert nested if-else construct to allOf dt-bindings: arm: Add qcom specific hvc transport for SCMI firmware: arm_scmi: Add qcom hvc/shmem transport .../bindings/firmware/arm,scmi.yaml | 67 +++--- 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 | 224 ++++++++++++++++++ 6 files changed, 284 insertions(+), 28 deletions(-) create mode 100644 drivers/firmware/arm_scmi/qcom_hvc.c