mbox series

[v3,0/3] Add qcom hvc/shmem transport

Message ID 20230811175719.28378-1-quic_nkela@quicinc.com
Headers show
Series Add qcom hvc/shmem transport | expand

Message

Nikunj Kela Aug. 11, 2023, 5:57 p.m. UTC
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.

---
v3 -> fix the compilation error reported by the test bot,
      add support for polling based instances

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            |   2 +
 drivers/firmware/arm_scmi/common.h            |   3 +
 drivers/firmware/arm_scmi/driver.c            |   4 +
 drivers/firmware/arm_scmi/qcom_hvc.c          | 232 ++++++++++++++++++
 6 files changed, 293 insertions(+), 28 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/qcom_hvc.c

Comments

Nikunj Kela Sept. 5, 2023, 4:06 p.m. UTC | #1
On 8/11/2023 10:57 AM, Nikunj Kela wrote:
> 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.
>
> ---
> v3 -> fix the compilation error reported by the test bot,
>        add support for polling based instances
>
> 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            |   2 +
>   drivers/firmware/arm_scmi/common.h            |   3 +
>   drivers/firmware/arm_scmi/driver.c            |   4 +
>   drivers/firmware/arm_scmi/qcom_hvc.c          | 232 ++++++++++++++++++
>   6 files changed, 293 insertions(+), 28 deletions(-)
>   create mode 100644 drivers/firmware/arm_scmi/qcom_hvc.c
Gentle Ping!
Krzysztof Kozlowski Sept. 5, 2023, 4:37 p.m. UTC | #2
On 05/09/2023 18:06, Nikunj Kela wrote:
> 
> On 8/11/2023 10:57 AM, Nikunj Kela wrote:
>> 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.
>>
>> ---
>> v3 -> fix the compilation error reported by the test bot,
>>        add support for polling based instances
>>
>> 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            |   2 +
>>   drivers/firmware/arm_scmi/common.h            |   3 +
>>   drivers/firmware/arm_scmi/driver.c            |   4 +
>>   drivers/firmware/arm_scmi/qcom_hvc.c          | 232 ++++++++++++++++++
>>   6 files changed, 293 insertions(+), 28 deletions(-)
>>   create mode 100644 drivers/firmware/arm_scmi/qcom_hvc.c
> Gentle Ping!

It's third ping these two weeks from Qualcomm. Folks, it is merge
window. What do you think will happen with your ping during this time?

Best regards,
Krzysztof
Sudeep Holla Sept. 7, 2023, 10:36 a.m. UTC | #3
On Tue, Sep 05, 2023 at 06:37:14PM +0200, Krzysztof Kozlowski wrote:
> On 05/09/2023 18:06, Nikunj Kela wrote:
> > 
> > On 8/11/2023 10:57 AM, Nikunj Kela wrote:
> >> 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.
> >>
> >> ---
> >> v3 -> fix the compilation error reported by the test bot,
> >>        add support for polling based instances
> >>
> >> 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            |   2 +
> >>   drivers/firmware/arm_scmi/common.h            |   3 +
> >>   drivers/firmware/arm_scmi/driver.c            |   4 +
> >>   drivers/firmware/arm_scmi/qcom_hvc.c          | 232 ++++++++++++++++++
> >>   6 files changed, 293 insertions(+), 28 deletions(-)
> >>   create mode 100644 drivers/firmware/arm_scmi/qcom_hvc.c
> > Gentle Ping!

Pong !

>
> It's third ping these two weeks from Qualcomm. Folks, it is merge
> window. What do you think will happen with your ping during this time?
>

+1

Okay, here is the deal with this patch set. As you are aware that a previous
merged solution was abandoned by Qcom in a single kernel release cycle. So
I decided to ignore this for one or 2 kernel release cycle to make sure
Qcom makes up their mind on the design and then we can see how to proceed.
Qcom must understand upstream kernel is not a playground to push their
design which they might decided to drop support for in such short period.
Please understand the upstream kernel supports platforms that are more than
few decades old. It is not like the mobile platforms that are hardly supported
for couple of years. And similarly, we push core support if and only if we
know for sure it will be used on some platform. I trusted Qcom with the
previous extension of SMC/HVC transport but I was proven wrong.

Also, I definitely don't like the way you have copied the whole smc.c
and changed it to Qcom's need and made it qcom_hvc.c. Just add the required
changes in smc.c.

--
Regards,
Sudeep
Nikunj Kela Sept. 7, 2023, 2:20 p.m. UTC | #4
On 9/7/2023 3:36 AM, Sudeep Holla wrote:
> On Tue, Sep 05, 2023 at 06:37:14PM +0200, Krzysztof Kozlowski wrote:
>> On 05/09/2023 18:06, Nikunj Kela wrote:
>>> On 8/11/2023 10:57 AM, Nikunj Kela wrote:
>>>> 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.
>>>>
>>>> ---
>>>> v3 -> fix the compilation error reported by the test bot,
>>>>         add support for polling based instances
>>>>
>>>> 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            |   2 +
>>>>    drivers/firmware/arm_scmi/common.h            |   3 +
>>>>    drivers/firmware/arm_scmi/driver.c            |   4 +
>>>>    drivers/firmware/arm_scmi/qcom_hvc.c          | 232 ++++++++++++++++++
>>>>    6 files changed, 293 insertions(+), 28 deletions(-)
>>>>    create mode 100644 drivers/firmware/arm_scmi/qcom_hvc.c
>>> Gentle Ping!
> Pong !
>
>> It's third ping these two weeks from Qualcomm. Folks, it is merge
>> window. What do you think will happen with your ping during this time?
>>
> +1
>
> Okay, here is the deal with this patch set. As you are aware that a previous
> merged solution was abandoned by Qcom in a single kernel release cycle. So
> I decided to ignore this for one or 2 kernel release cycle to make sure
> Qcom makes up their mind on the design and then we can see how to proceed.
> Qcom must understand upstream kernel is not a playground to push their
> design which they might decided to drop support for in such short period.
> Please understand the upstream kernel supports platforms that are more than
> few decades old. It is not like the mobile platforms that are hardly supported
> for couple of years. And similarly, we push core support if and only if we
> know for sure it will be used on some platform. I trusted Qcom with the
> previous extension of SMC/HVC transport but I was proven wrong.
>
> Also, I definitely don't like the way you have copied the whole smc.c
> and changed it to Qcom's need and made it qcom_hvc.c. Just add the required
> changes in smc.c.
>
> --
> Regards,
> Sudeep

Completely understand your concerns and extending my apologies once 
again on the patch that was abandoned. I will rework the patch to 
include changes in smc.c. Thanks so much for your response!