[RFC 0/3] Enable firmware-managed USB resources on Qcom targets

[RFC 0/3] Enable firmware-managed USB resources on Qcom targets

[Sriram Dash]

Some target systems allow multiple resources to be managed by firmware.
On these targets, tasks related to clocks, regulators, resets, and
interconnects can be delegated to the firmware, while the remaining
responsibilities are handled by Linux.

To support the management of partial resources in Linux and leave the rest
to firmware, multiple power domains are introduced. Each power domain can
manage one or more resources, depending on the specific use case.

These power domains handle SCMI calls to the firmware, enabling the
activation and deactivation of firmware-managed resources.

The driver is responsible for managing multiple power domains and
linking them to consumers as needed. Incase there is only single
power domain, it is considered to be a standard GDSC hooked on to
the qcom dt node which is read and assigned to device structure
(by genpd framework) before the driver probe even begins.

fw-managed dt property allows the driver to determine whether
device resources are managed by Linux or firmware, ensuring
backward compatibility.

Establish the channel and domain mapping for the power domains to connect
with firmware, enabling the firmware to handle the assigned resources.
Since these delegated resources will remain invisible to the operating
system, ensure that any references to them are removed.

Sriram Dash (3):
  dt-bindings: usb: qcom,dwc3: Add support for multiple power-domains
  USB: dwc3: qcom: Add support for firmware managed resources
  arm64: dts: qcom: sa8775p-ride: Enable support for firmware managed
    resources

 .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        |  74 ++++--
 .../bindings/phy/qcom,usb-snps-femto-v2.yaml       |  49 +++-
 .../devicetree/bindings/usb/qcom,dwc3.yaml         |  37 ++-
 arch/arm64/boot/dts/qcom/sa8775p-ride.dts          |  96 +++++--
 drivers/phy/qualcomm/phy-qcom-qmp-usb.c            | 290 ++++++++++++++++-----
 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c      | 213 ++++++++++++---
 drivers/usb/dwc3/dwc3-qcom.c                       | 259 +++++++++++++-----
 7 files changed, 801 insertions(+), 217 deletions(-)

-- 
2.7.4

[RFC 1/3] dt-bindings: usb: qcom,dwc3: Add support for multiple power-domains

[Sriram Dash]

Some target systems allow multiple resources to be managed by firmware.
On these targets, tasks related to clocks, regulators, resets, and
interconnects can be delegated to the firmware, while the remaining
responsibilities are handled by Linux.

To support the management of partial resources in Linux and leave the rest
to firmware, multiple power domains are introduced. Each power domain can
manage one or more resources, depending on the specific use case.

These power domains handle SCMI calls to the firmware, enabling the
activation and deactivation of firmware-managed resources.

Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
---
 .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 74 ++++++++++++++++------
 .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 49 ++++++++++++--
 .../devicetree/bindings/usb/qcom,dwc3.yaml         | 37 ++++++++++-
 3 files changed, 130 insertions(+), 30 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
index 1e2d4dd..53b9ba9 100644
--- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
@@ -44,7 +44,32 @@ properties:
     maxItems: 5
 
   power-domains:
-    maxItems: 1
+    description: specifies a phandle to PM domain provider node
+    minItems: 1
+    maxItems: 2
+
+  power-domain-names:
+    description:
+      A list of power domain name strings sorted in the same order as the
+      power-domains property.
+
+      For platforms where some resource are firmware managed, the name
+      corresponding to the index of an SCMI domain provider can be
+      "usb_core" or "usb_transfer".
+    items:
+      - const: usb_core
+      - const: usb_transfer
+
+  qmp,fw-managed:
+    description:
+      Some targets allow multiple resources to be managed by firmware.
+      On these targets, tasks related to clocks, regulators, resets, and
+      interconnects can be delegated to the firmware, while the remaining
+      responsibilities are handled by Linux.
+
+      Decide if the target resources will be managed by firmware or High level
+      OS.
+    type: boolean
 
   resets:
     maxItems: 2
@@ -70,14 +95,6 @@ properties:
 required:
   - compatible
   - reg
-  - clocks
-  - clock-names
-  - resets
-  - reset-names
-  - vdda-phy-supply
-  - vdda-pll-supply
-  - "#clock-cells"
-  - clock-output-names
   - "#phy-cells"
 
 allOf:
@@ -86,6 +103,33 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,sa8775p-qmp-usb3-uni-phy
+              - qcom,sc8280xp-qmp-usb3-uni-phy
+              - qcom,x1e80100-qmp-usb3-uni-phy
+    then:
+      required:
+        - power-domains
+
+  - if:
+      not:
+        required:
+          - qmp,fw-managed
+    then:
+      required:
+        - clocks
+        - clock-names
+        - resets
+        - reset-names
+        - vdda-phy-supply
+        - vdda-pll-supply
+        - clock-output-names
+        - "#clock-cells"
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
               - qcom,ipq6018-qmp-usb3-phy
               - qcom,ipq8074-qmp-usb3-phy
               - qcom,ipq9574-qmp-usb3-phy
@@ -144,18 +188,6 @@ allOf:
             - const: com_aux
             - const: pipe
 
-  - if:
-      properties:
-        compatible:
-          contains:
-            enum:
-              - qcom,sa8775p-qmp-usb3-uni-phy
-              - qcom,sc8280xp-qmp-usb3-uni-phy
-              - qcom,x1e80100-qmp-usb3-uni-phy
-    then:
-      required:
-        - power-domains
-
 additionalProperties: false
 
 examples:
diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
index 0f200e3..ad2f08f 100644
--- a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
@@ -49,6 +49,34 @@ properties:
     items:
       - const: ref
 
+  power-domains:
+    description: specifies a phandle to PM domain provider node
+    minItems: 1
+    maxItems: 2
+
+  power-domain-names:
+    description:
+      A list of power domain name strings sorted in the same order as the
+      power-domains property.
+
+      For platforms where some resource are firmware managed, the name
+      corresponding to the index of an SCMI domain provider can be
+      "usb_core" or "usb_transfer".
+    items:
+      - const: usb_core
+      - const: usb_transfer
+
+  hsphy,fw-managed:
+    description:
+      Some targets allow multiple resources to be managed by firmware.
+      On these targets, tasks related to clocks, regulators, resets, and
+      interconnects can be delegated to the firmware, while the remaining
+      responsibilities are handled by Linux.
+
+      Decide if the target resources will be managed by firmware or High level
+      OS.
+    type: boolean
+
   resets:
     items:
       - description: PHY core reset
@@ -154,12 +182,21 @@ required:
   - compatible
   - reg
   - "#phy-cells"
-  - clocks
-  - clock-names
-  - resets
-  - vdda-pll-supply
-  - vdda18-supply
-  - vdda33-supply
+
+
+allOf:
+  - if:
+      not:
+        required:
+          - hsphy,fw-managed
+    then:
+      required:
+        - clocks
+        - clock-names
+        - resets
+        - vdda-pll-supply
+        - vdda18-supply
+        - vdda33-supply
 
 additionalProperties: false
 
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index 63d150b..5bf3a29 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -64,7 +64,31 @@ properties:
 
   power-domains:
     description: specifies a phandle to PM domain provider node
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
+
+  power-domain-names:
+    description:
+      A list of power domain name strings sorted in the same order as the
+      power-domains property.
+
+      For platforms where some resource are firmware managed, the name
+      corresponding to the index of an SCMI domain provider can be
+      "usb_core" or "usb_transfer".
+    items:
+      - const: usb_core
+      - const: usb_transfer
+
+  qcom,fw-managed:
+    description:
+      Some targets allow multiple resources to be managed by firmware.
+      On these targets, tasks related to clocks, regulators, resets, and
+      interconnects can be delegated to the firmware, while the remaining
+      responsibilities are handled by Linux.
+
+      Decide if the target resources will be managed by firmware or High level
+      OS.
+    type: boolean
 
   required-opps:
     maxItems: 1
@@ -148,13 +172,20 @@ required:
   - "#address-cells"
   - "#size-cells"
   - ranges
-  - clocks
-  - clock-names
   - interrupts
   - interrupt-names
 
 allOf:
   - if:
+      not:
+        required:
+          - qcom,fw-managed
+    then:
+      required:
+        - clocks
+        - clock-names
+
+  - if:
       properties:
         compatible:
           contains:
-- 
2.7.4

Re: [RFC 1/3] dt-bindings: usb: qcom,dwc3: Add support for multiple power-domains

[Krzysztof Kozlowski]

On 05/03/2024 17:57, Sriram Dash wrote:
> Some target systems allow multiple resources to be managed by firmware.
> On these targets, tasks related to clocks, regulators, resets, and
> interconnects can be delegated to the firmware, while the remaining
> responsibilities are handled by Linux.
> 
> To support the management of partial resources in Linux and leave the rest
> to firmware, multiple power domains are introduced. Each power domain can
> manage one or more resources, depending on the specific use case.
> 
> These power domains handle SCMI calls to the firmware, enabling the
> activation and deactivation of firmware-managed resources.
> 
> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
> ---
>  .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 74 ++++++++++++++++------
>  .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 49 ++++++++++++--
>  .../devicetree/bindings/usb/qcom,dwc3.yaml         | 37 ++++++++++-
>  3 files changed, 130 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> index 1e2d4dd..53b9ba9 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> @@ -44,7 +44,32 @@ properties:
>      maxItems: 5
>  
>    power-domains:
> -    maxItems: 1
> +    description: specifies a phandle to PM domain provider node

Please drop all redundant descriptions. Adding them is not even related
to this patch.

> +    minItems: 1
> +    maxItems: 2
> +
> +  power-domain-names:
> +    description:
> +      A list of power domain name strings sorted in the same order as the
> +      power-domains property.
> +
> +      For platforms where some resource are firmware managed, the name
> +      corresponding to the index of an SCMI domain provider can be
> +      "usb_core" or "usb_transfer".
> +    items:
> +      - const: usb_core
> +      - const: usb_transfer

How is this related to fw-managed? I fail to see it. Don't mix
independent problems in one patch.

> +
> +  qmp,fw-managed:

Please do not upstream vendor code directly, but perform basic
adjustment to upstream Linux kernel. There is no such company as gmp.

Run this first through your internal review process.

> +    description:
> +      Some targets allow multiple resources to be managed by firmware.

You miss clear mapping between compatibles and this property - allOf
restricting it to specific SoCs.

Is this different property than qcom,controlled-remotely?

Best regards,
Krzysztof

Re: [RFC 1/3] dt-bindings: usb: qcom,dwc3: Add support for multiple power-domains

[Sriram Dash]

On 3/5/2024 10:37 PM, Krzysztof Kozlowski wrote:
> On 05/03/2024 17:57, Sriram Dash wrote:
>> Some target systems allow multiple resources to be managed by firmware.
>> On these targets, tasks related to clocks, regulators, resets, and
>> interconnects can be delegated to the firmware, while the remaining
>> responsibilities are handled by Linux.
>>
>> To support the management of partial resources in Linux and leave the rest
>> to firmware, multiple power domains are introduced. Each power domain can
>> manage one or more resources, depending on the specific use case.
>>
>> These power domains handle SCMI calls to the firmware, enabling the
>> activation and deactivation of firmware-managed resources.
>>
>> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
>> ---
>>   .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 74 ++++++++++++++++------
>>   .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 49 ++++++++++++--
>>   .../devicetree/bindings/usb/qcom,dwc3.yaml         | 37 ++++++++++-
>>   3 files changed, 130 insertions(+), 30 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>> index 1e2d4dd..53b9ba9 100644
>> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>> @@ -44,7 +44,32 @@ properties:
>>       maxItems: 5
>>   
>>     power-domains:
>> -    maxItems: 1
>> +    description: specifies a phandle to PM domain provider node
> 
> Please drop all redundant descriptions. Adding them is not even related
> to this patch.
> 

Thanks Krzysztof for the super quick response !
Sure. will drop the description for power-domain
and power-doamin-names.

>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  power-domain-names:
>> +    description:
>> +      A list of power domain name strings sorted in the same order as the
>> +      power-domains property.
>> +
>> +      For platforms where some resource are firmware managed, the name
>> +      corresponding to the index of an SCMI domain provider can be
>> +      "usb_core" or "usb_transfer".
>> +    items:
>> +      - const: usb_core
>> +      - const: usb_transfer
> 
> How is this related to fw-managed? I fail to see it. Don't mix
> independent problems in one patch.
> 

Some of the the resources like clocks, regulators, etc will be 
controlled by the firmware instead of OS. However, some resources
still will be controlled by OS (interrupts for example).

So, to support the management of partial resources in Linux, and
offload the other resource management to firmware, multiple power 
domains are introduced. They will be corresponding to different
hw blocks.

Do you suggest splitting the addition of power-domain-names and
addition of fw-managed property in separate patches for bindings!

>> +
>> +  qmp,fw-managed:
> 
> Please do not upstream vendor code directly, but perform basic
> adjustment to upstream Linux kernel. There is no such company as gmp.
> 
> Run this first through your internal review process.
> 

This property is newly introduced for the qmp superspeed phy and
similar dt properties are introduced for hsphy and dwc3 qcom
controller glue layer driver. It will govern the suspend/ resume
of the resources (by firmware or OS) as required.

>> +    description:
>> +      Some targets allow multiple resources to be managed by firmware.
> 
> You miss clear mapping between compatibles and this property - allOf
> restricting it to specific SoCs.
> 
> Is this different property than qcom,controlled-remotely?
> 

No. unlike "qcom,controlled-remotely", which lets the OS know only to
consume the resources, "qmp,fw-managed" property will decide the
resource management and trigger the suspend/ resume from OS, which
will be handled by the firmware.

Can we reuse the property "qcom,controlled-remotely" here? If so,
we can replace the aformentioned property with qmp phy, hsphy, and
controller glue layer property. Please suggest.

> Best regards,
> Krzysztof
> 

Re: [RFC 1/3] dt-bindings: usb: qcom,dwc3: Add support for multiple power-domains

[Krzysztof Kozlowski]

On 05/03/2024 19:03, Sriram Dash wrote:
> On 3/5/2024 10:37 PM, Krzysztof Kozlowski wrote:
>> On 05/03/2024 17:57, Sriram Dash wrote:
>>> Some target systems allow multiple resources to be managed by firmware.
>>> On these targets, tasks related to clocks, regulators, resets, and
>>> interconnects can be delegated to the firmware, while the remaining
>>> responsibilities are handled by Linux.
>>>
>>> To support the management of partial resources in Linux and leave the rest
>>> to firmware, multiple power domains are introduced. Each power domain can
>>> manage one or more resources, depending on the specific use case.
>>>
>>> These power domains handle SCMI calls to the firmware, enabling the
>>> activation and deactivation of firmware-managed resources.
>>>
>>> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
>>> ---
>>>   .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 74 ++++++++++++++++------
>>>   .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 49 ++++++++++++--
>>>   .../devicetree/bindings/usb/qcom,dwc3.yaml         | 37 ++++++++++-
>>>   3 files changed, 130 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>> index 1e2d4dd..53b9ba9 100644
>>> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>> @@ -44,7 +44,32 @@ properties:
>>>       maxItems: 5
>>>   
>>>     power-domains:
>>> -    maxItems: 1
>>> +    description: specifies a phandle to PM domain provider node
>>
>> Please drop all redundant descriptions. Adding them is not even related
>> to this patch.
>>
> 
> Thanks Krzysztof for the super quick response !
> Sure. will drop the description for power-domain
> and power-doamin-names.
> 
>>> +    minItems: 1
>>> +    maxItems: 2
>>> +
>>> +  power-domain-names:
>>> +    description:
>>> +      A list of power domain name strings sorted in the same order as the
>>> +      power-domains property.
>>> +
>>> +      For platforms where some resource are firmware managed, the name
>>> +      corresponding to the index of an SCMI domain provider can be
>>> +      "usb_core" or "usb_transfer".
>>> +    items:
>>> +      - const: usb_core
>>> +      - const: usb_transfer
>>
>> How is this related to fw-managed? I fail to see it. Don't mix
>> independent problems in one patch.
>>
> 
> Some of the the resources like clocks, regulators, etc will be 
> controlled by the firmware instead of OS. However, some resources
> still will be controlled by OS (interrupts for example).
> 
> So, to support the management of partial resources in Linux, and
> offload the other resource management to firmware, multiple power 
> domains are introduced. They will be corresponding to different
> hw blocks.
> 
> Do you suggest splitting the addition of power-domain-names and
> addition of fw-managed property in separate patches for bindings!
> 
>>> +
>>> +  qmp,fw-managed:
>>
>> Please do not upstream vendor code directly, but perform basic
>> adjustment to upstream Linux kernel. There is no such company as gmp.
>>
>> Run this first through your internal review process.
>>
> 
> This property is newly introduced for the qmp superspeed phy and
> similar dt properties are introduced for hsphy and dwc3 qcom
> controller glue layer driver. It will govern the suspend/ resume
> of the resources (by firmware or OS) as required.

So that's your answer to "there is no such company as gmp"? It's not
relevant at all.

Please run it through internal review first.


Best regards,
Krzysztof

Re: [RFC 1/3] dt-bindings: usb: qcom,dwc3: Add support for multiple power-domains

[Krishna Kurapati PSSNV]

On 3/6/2024 12:33 PM, Krzysztof Kozlowski wrote:
> On 05/03/2024 19:03, Sriram Dash wrote:
>> On 3/5/2024 10:37 PM, Krzysztof Kozlowski wrote:
>>> On 05/03/2024 17:57, Sriram Dash wrote:
>>>> Some target systems allow multiple resources to be managed by firmware.
>>>> On these targets, tasks related to clocks, regulators, resets, and
>>>> interconnects can be delegated to the firmware, while the remaining
>>>> responsibilities are handled by Linux.
>>>>
>>>> To support the management of partial resources in Linux and leave the rest
>>>> to firmware, multiple power domains are introduced. Each power domain can
>>>> manage one or more resources, depending on the specific use case.
>>>>
>>>> These power domains handle SCMI calls to the firmware, enabling the
>>>> activation and deactivation of firmware-managed resources.
>>>>
>>>> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
>>>> ---
>>>>    .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 74 ++++++++++++++++------
>>>>    .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 49 ++++++++++++--
>>>>    .../devicetree/bindings/usb/qcom,dwc3.yaml         | 37 ++++++++++-
>>>>    3 files changed, 130 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>>> index 1e2d4dd..53b9ba9 100644
>>>> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>>> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>>> @@ -44,7 +44,32 @@ properties:
>>>>        maxItems: 5
>>>>    
>>>>      power-domains:
>>>> -    maxItems: 1
>>>> +    description: specifies a phandle to PM domain provider node
>>>
>>> Please drop all redundant descriptions. Adding them is not even related
>>> to this patch.
>>>
>>
>> Thanks Krzysztof for the super quick response !
>> Sure. will drop the description for power-domain
>> and power-doamin-names.
>>
>>>> +    minItems: 1
>>>> +    maxItems: 2
>>>> +
>>>> +  power-domain-names:
>>>> +    description:
>>>> +      A list of power domain name strings sorted in the same order as the
>>>> +      power-domains property.
>>>> +
>>>> +      For platforms where some resource are firmware managed, the name
>>>> +      corresponding to the index of an SCMI domain provider can be
>>>> +      "usb_core" or "usb_transfer".
>>>> +    items:
>>>> +      - const: usb_core
>>>> +      - const: usb_transfer
>>>
>>> How is this related to fw-managed? I fail to see it. Don't mix
>>> independent problems in one patch.
>>>
>>
>> Some of the the resources like clocks, regulators, etc will be
>> controlled by the firmware instead of OS. However, some resources
>> still will be controlled by OS (interrupts for example).
>>
>> So, to support the management of partial resources in Linux, and
>> offload the other resource management to firmware, multiple power
>> domains are introduced. They will be corresponding to different
>> hw blocks.
>>
>> Do you suggest splitting the addition of power-domain-names and
>> addition of fw-managed property in separate patches for bindings!
>>
>>>> +
>>>> +  qmp,fw-managed:
>>>
>>> Please do not upstream vendor code directly, but perform basic
>>> adjustment to upstream Linux kernel. There is no such company as gmp.
>>>
>>> Run this first through your internal review process.
>>>
>>
>> This property is newly introduced for the qmp superspeed phy and
>> similar dt properties are introduced for hsphy and dwc3 qcom
>> controller glue layer driver. It will govern the suspend/ resume
>> of the resources (by firmware or OS) as required.
> 
> So that's your answer to "there is no such company as gmp"? It's not
> relevant at all.
> 
> Please run it through internal review first.
> 
> 
Hi Sriram,

  QMP indicates the driver name "QMP Phy" (for super speed mode of 
operation), not the company name. If this property is for identification 
of fw management to be done / (or not to be done) in QMP Phy, then 
please rename is appropriately. Check how other properties are defined 
in the QMP bindings and add "qcom" at beginning preferably. For 
reference as to how properties were added to femto phy driver: [1].

[1]: 
https://patchwork.kernel.org/project/linux-arm-msm/patch/1662480933-12326-2-git-send-email-quic_kriskura@quicinc.com/

Regards,
Krishna,

Re: [RFC 1/3] dt-bindings: usb: qcom,dwc3: Add support for multiple power-domains

[Sriram Dash]

On 3/6/2024 1:03 PM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 3/6/2024 12:33 PM, Krzysztof Kozlowski wrote:
>> On 05/03/2024 19:03, Sriram Dash wrote:
>>> On 3/5/2024 10:37 PM, Krzysztof Kozlowski wrote:
>>>> On 05/03/2024 17:57, Sriram Dash wrote:
>>>>> Some target systems allow multiple resources to be managed by 
>>>>> firmware.
>>>>> On these targets, tasks related to clocks, regulators, resets, and
>>>>> interconnects can be delegated to the firmware, while the remaining
>>>>> responsibilities are handled by Linux.
>>>>>
>>>>> To support the management of partial resources in Linux and leave 
>>>>> the rest
>>>>> to firmware, multiple power domains are introduced. Each power 
>>>>> domain can
>>>>> manage one or more resources, depending on the specific use case.
>>>>>
>>>>> These power domains handle SCMI calls to the firmware, enabling the
>>>>> activation and deactivation of firmware-managed resources.
>>>>>
>>>>> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
>>>>> ---
>>>>>    .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 74 
>>>>> ++++++++++++++++------
>>>>>    .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 49 
>>>>> ++++++++++++--
>>>>>    .../devicetree/bindings/usb/qcom,dwc3.yaml         | 37 ++++++++++-
>>>>>    3 files changed, 130 insertions(+), 30 deletions(-)
>>>>>
>>>>> diff --git 
>>>>> a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>>>> index 1e2d4dd..53b9ba9 100644
>>>>> --- 
>>>>> a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>>>> +++ 
>>>>> b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>>>> @@ -44,7 +44,32 @@ properties:
>>>>>        maxItems: 5
>>>>>      power-domains:
>>>>> -    maxItems: 1
>>>>> +    description: specifies a phandle to PM domain provider node
>>>>
>>>> Please drop all redundant descriptions. Adding them is not even related
>>>> to this patch.
>>>>
>>>
>>> Thanks Krzysztof for the super quick response !
>>> Sure. will drop the description for power-domain
>>> and power-doamin-names.
>>>
>>>>> +    minItems: 1
>>>>> +    maxItems: 2
>>>>> +
>>>>> +  power-domain-names:
>>>>> +    description:
>>>>> +      A list of power domain name strings sorted in the same order 
>>>>> as the
>>>>> +      power-domains property.
>>>>> +
>>>>> +      For platforms where some resource are firmware managed, the 
>>>>> name
>>>>> +      corresponding to the index of an SCMI domain provider can be
>>>>> +      "usb_core" or "usb_transfer".
>>>>> +    items:
>>>>> +      - const: usb_core
>>>>> +      - const: usb_transfer
>>>>
>>>> How is this related to fw-managed? I fail to see it. Don't mix
>>>> independent problems in one patch.
>>>>
>>>
>>> Some of the the resources like clocks, regulators, etc will be
>>> controlled by the firmware instead of OS. However, some resources
>>> still will be controlled by OS (interrupts for example).
>>>
>>> So, to support the management of partial resources in Linux, and
>>> offload the other resource management to firmware, multiple power
>>> domains are introduced. They will be corresponding to different
>>> hw blocks.
>>>
>>> Do you suggest splitting the addition of power-domain-names and
>>> addition of fw-managed property in separate patches for bindings!
>>>
>>>>> +
>>>>> +  qmp,fw-managed:
>>>>
>>>> Please do not upstream vendor code directly, but perform basic
>>>> adjustment to upstream Linux kernel. There is no such company as gmp.
>>>>
>>>> Run this first through your internal review process.
>>>>
>>>
>>> This property is newly introduced for the qmp superspeed phy and
>>> similar dt properties are introduced for hsphy and dwc3 qcom
>>> controller glue layer driver. It will govern the suspend/ resume
>>> of the resources (by firmware or OS) as required.
>>
>> So that's your answer to "there is no such company as gmp"? It's not
>> relevant at all.
>>
>> Please run it through internal review first.
>>
>>
> Hi Sriram,
> 
>   QMP indicates the driver name "QMP Phy" (for super speed mode of 
> operation), not the company name. If this property is for identification 
> of fw management to be done / (or not to be done) in QMP Phy, then 
> please rename is appropriately. Check how other properties are defined 
> in the QMP bindings and add "qcom" at beginning preferably. For 
> reference as to how properties were added to femto phy driver: [1].
> 
> [1]: 
> https://patchwork.kernel.org/project/linux-arm-msm/patch/1662480933-12326-2-git-send-email-quic_kriskura@quicinc.com/
> 

Thanks for the references Krzysztof/Krishna.
I got suggestion from Dmitry on the use of the
property too. Indeed we will be the scmi based
clock/ PD management on SA8775. So, we will
introduce a new compatible string and take the
decision in driver accordingly.


> Regards,
> Krishna,

Re: [RFC 1/3] dt-bindings: usb: qcom,dwc3: Add support for multiple power-domains

[Dmitry Baryshkov]

On Tue, 5 Mar 2024 at 18:58, Sriram Dash <quic_sriramd@quicinc.com> wrote:
>
> Some target systems allow multiple resources to be managed by firmware.
> On these targets, tasks related to clocks, regulators, resets, and
> interconnects can be delegated to the firmware, while the remaining
> responsibilities are handled by Linux.
>
> To support the management of partial resources in Linux and leave the rest
> to firmware, multiple power domains are introduced. Each power domain can
> manage one or more resources, depending on the specific use case.
>
> These power domains handle SCMI calls to the firmware, enabling the
> activation and deactivation of firmware-managed resources.
>
> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
> ---
>  .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 74 ++++++++++++++++------
>  .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 49 ++++++++++++--
>  .../devicetree/bindings/usb/qcom,dwc3.yaml         | 37 ++++++++++-
>  3 files changed, 130 insertions(+), 30 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> index 1e2d4dd..53b9ba9 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> @@ -44,7 +44,32 @@ properties:
>      maxItems: 5
>
>    power-domains:
> -    maxItems: 1
> +    description: specifies a phandle to PM domain provider node
> +    minItems: 1
> +    maxItems: 2
> +
> +  power-domain-names:
> +    description:
> +      A list of power domain name strings sorted in the same order as the
> +      power-domains property.
> +
> +      For platforms where some resource are firmware managed, the name
> +      corresponding to the index of an SCMI domain provider can be
> +      "usb_core" or "usb_transfer".
> +    items:
> +      - const: usb_core
> +      - const: usb_transfer
> +
> +  qmp,fw-managed:
> +    description:
> +      Some targets allow multiple resources to be managed by firmware.
> +      On these targets, tasks related to clocks, regulators, resets, and
> +      interconnects can be delegated to the firmware, while the remaining
> +      responsibilities are handled by Linux.
> +
> +      Decide if the target resources will be managed by firmware or High level
> +      OS.
> +    type: boolean
>
>    resets:
>      maxItems: 2
> @@ -70,14 +95,6 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - clocks
> -  - clock-names
> -  - resets
> -  - reset-names
> -  - vdda-phy-supply
> -  - vdda-pll-supply
> -  - "#clock-cells"
> -  - clock-output-names
>    - "#phy-cells"
>
>  allOf:
> @@ -86,6 +103,33 @@ allOf:
>          compatible:
>            contains:
>              enum:
> +              - qcom,sa8775p-qmp-usb3-uni-phy
> +              - qcom,sc8280xp-qmp-usb3-uni-phy
> +              - qcom,x1e80100-qmp-usb3-uni-phy
> +    then:
> +      required:
> +        - power-domains
> +
> +  - if:
> +      not:
> +        required:
> +          - qmp,fw-managed
> +    then:
> +      required:
> +        - clocks
> +        - clock-names
> +        - resets
> +        - reset-names
> +        - vdda-phy-supply
> +        - vdda-pll-supply
> +        - clock-output-names
> +        - "#clock-cells"
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
>                - qcom,ipq6018-qmp-usb3-phy
>                - qcom,ipq8074-qmp-usb3-phy
>                - qcom,ipq9574-qmp-usb3-phy
> @@ -144,18 +188,6 @@ allOf:
>              - const: com_aux
>              - const: pipe
>
> -  - if:
> -      properties:
> -        compatible:
> -          contains:
> -            enum:
> -              - qcom,sa8775p-qmp-usb3-uni-phy
> -              - qcom,sc8280xp-qmp-usb3-uni-phy
> -              - qcom,x1e80100-qmp-usb3-uni-phy
> -    then:
> -      required:
> -        - power-domains
> -
>  additionalProperties: false
>
>  examples:
> diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
> index 0f200e3..ad2f08f 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
> @@ -49,6 +49,34 @@ properties:
>      items:
>        - const: ref
>
> +  power-domains:
> +    description: specifies a phandle to PM domain provider node
> +    minItems: 1
> +    maxItems: 2
> +
> +  power-domain-names:
> +    description:
> +      A list of power domain name strings sorted in the same order as the
> +      power-domains property.
> +
> +      For platforms where some resource are firmware managed, the name
> +      corresponding to the index of an SCMI domain provider can be
> +      "usb_core" or "usb_transfer".
> +    items:
> +      - const: usb_core
> +      - const: usb_transfer
> +
> +  hsphy,fw-managed:
> +    description:
> +      Some targets allow multiple resources to be managed by firmware.
> +      On these targets, tasks related to clocks, regulators, resets, and
> +      interconnects can be delegated to the firmware, while the remaining
> +      responsibilities are handled by Linux.
> +
> +      Decide if the target resources will be managed by firmware or High level
> +      OS.
> +    type: boolean
> +
>    resets:
>      items:
>        - description: PHY core reset
> @@ -154,12 +182,21 @@ required:
>    - compatible
>    - reg
>    - "#phy-cells"
> -  - clocks
> -  - clock-names
> -  - resets
> -  - vdda-pll-supply
> -  - vdda18-supply
> -  - vdda33-supply
> +
> +
> +allOf:
> +  - if:
> +      not:
> +        required:
> +          - hsphy,fw-managed
> +    then:
> +      required:
> +        - clocks
> +        - clock-names
> +        - resets
> +        - vdda-pll-supply
> +        - vdda18-supply
> +        - vdda33-supply
>
>  additionalProperties: false
>
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index 63d150b..5bf3a29 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -64,7 +64,31 @@ properties:
>
>    power-domains:
>      description: specifies a phandle to PM domain provider node
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
> +
> +  power-domain-names:
> +    description:
> +      A list of power domain name strings sorted in the same order as the
> +      power-domains property.
> +
> +      For platforms where some resource are firmware managed, the name
> +      corresponding to the index of an SCMI domain provider can be
> +      "usb_core" or "usb_transfer".
> +    items:
> +      - const: usb_core
> +      - const: usb_transfer
> +
> +  qcom,fw-managed:
> +    description:
> +      Some targets allow multiple resources to be managed by firmware.
> +      On these targets, tasks related to clocks, regulators, resets, and
> +      interconnects can be delegated to the firmware, while the remaining
> +      responsibilities are handled by Linux.
> +
> +      Decide if the target resources will be managed by firmware or High level
> +      OS.
> +    type: boolean

I think this is an overkill. You know that SA8775 is going to use
SCMI-based clocks / PD management. Thus I'd suggest adding new
bindings file targeting qcom,sa8775-dwc3. Also you can drop the
qcom,fw-managed property at all, let the driver decide basing on the
compat string.


>
>    required-opps:
>      maxItems: 1
> @@ -148,13 +172,20 @@ required:
>    - "#address-cells"
>    - "#size-cells"
>    - ranges
> -  - clocks
> -  - clock-names
>    - interrupts
>    - interrupt-names
>
>  allOf:
>    - if:
> +      not:
> +        required:
> +          - qcom,fw-managed
> +    then:
> +      required:
> +        - clocks
> +        - clock-names
> +
> +  - if:
>        properties:
>          compatible:
>            contains:
> --
> 2.7.4
>
>


--
With best wishes
Dmitry

Re: [RFC 1/3] dt-bindings: usb: qcom,dwc3: Add support for multiple power-domains

[Sriram Dash]

On 3/6/2024 12:33 AM, Dmitry Baryshkov wrote:
> On Tue, 5 Mar 2024 at 18:58, Sriram Dash <quic_sriramd@quicinc.com> wrote:
>>
>> Some target systems allow multiple resources to be managed by firmware.
>> On these targets, tasks related to clocks, regulators, resets, and
>> interconnects can be delegated to the firmware, while the remaining
>> responsibilities are handled by Linux.
>>
>> To support the management of partial resources in Linux and leave the rest
>> to firmware, multiple power domains are introduced. Each power domain can
>> manage one or more resources, depending on the specific use case.
>>
>> These power domains handle SCMI calls to the firmware, enabling the
>> activation and deactivation of firmware-managed resources.
>>
>> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
>> ---
>>   .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 74 ++++++++++++++++------
>>   .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 49 ++++++++++++--
>>   .../devicetree/bindings/usb/qcom,dwc3.yaml         | 37 ++++++++++-
>>   3 files changed, 130 insertions(+), 30 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>> index 1e2d4dd..53b9ba9 100644
>> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>> @@ -44,7 +44,32 @@ properties:
>>       maxItems: 5
>>
>>     power-domains:
>> -    maxItems: 1
>> +    description: specifies a phandle to PM domain provider node
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  power-domain-names:
>> +    description:
>> +      A list of power domain name strings sorted in the same order as the
>> +      power-domains property.
>> +
>> +      For platforms where some resource are firmware managed, the name
>> +      corresponding to the index of an SCMI domain provider can be
>> +      "usb_core" or "usb_transfer".
>> +    items:
>> +      - const: usb_core
>> +      - const: usb_transfer
>> +
>> +  qmp,fw-managed:
>> +    description:
>> +      Some targets allow multiple resources to be managed by firmware.
>> +      On these targets, tasks related to clocks, regulators, resets, and
>> +      interconnects can be delegated to the firmware, while the remaining
>> +      responsibilities are handled by Linux.
>> +
>> +      Decide if the target resources will be managed by firmware or High level
>> +      OS.
>> +    type: boolean
>>
>>     resets:
>>       maxItems: 2
>> @@ -70,14 +95,6 @@ properties:
>>   required:
>>     - compatible
>>     - reg
>> -  - clocks
>> -  - clock-names
>> -  - resets
>> -  - reset-names
>> -  - vdda-phy-supply
>> -  - vdda-pll-supply
>> -  - "#clock-cells"
>> -  - clock-output-names
>>     - "#phy-cells"
>>
>>   allOf:
>> @@ -86,6 +103,33 @@ allOf:
>>           compatible:
>>             contains:
>>               enum:
>> +              - qcom,sa8775p-qmp-usb3-uni-phy
>> +              - qcom,sc8280xp-qmp-usb3-uni-phy
>> +              - qcom,x1e80100-qmp-usb3-uni-phy
>> +    then:
>> +      required:
>> +        - power-domains
>> +
>> +  - if:
>> +      not:
>> +        required:
>> +          - qmp,fw-managed
>> +    then:
>> +      required:
>> +        - clocks
>> +        - clock-names
>> +        - resets
>> +        - reset-names
>> +        - vdda-phy-supply
>> +        - vdda-pll-supply
>> +        - clock-output-names
>> +        - "#clock-cells"
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>>                 - qcom,ipq6018-qmp-usb3-phy
>>                 - qcom,ipq8074-qmp-usb3-phy
>>                 - qcom,ipq9574-qmp-usb3-phy
>> @@ -144,18 +188,6 @@ allOf:
>>               - const: com_aux
>>               - const: pipe
>>
>> -  - if:
>> -      properties:
>> -        compatible:
>> -          contains:
>> -            enum:
>> -              - qcom,sa8775p-qmp-usb3-uni-phy
>> -              - qcom,sc8280xp-qmp-usb3-uni-phy
>> -              - qcom,x1e80100-qmp-usb3-uni-phy
>> -    then:
>> -      required:
>> -        - power-domains
>> -
>>   additionalProperties: false
>>
>>   examples:
>> diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
>> index 0f200e3..ad2f08f 100644
>> --- a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
>> +++ b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
>> @@ -49,6 +49,34 @@ properties:
>>       items:
>>         - const: ref
>>
>> +  power-domains:
>> +    description: specifies a phandle to PM domain provider node
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  power-domain-names:
>> +    description:
>> +      A list of power domain name strings sorted in the same order as the
>> +      power-domains property.
>> +
>> +      For platforms where some resource are firmware managed, the name
>> +      corresponding to the index of an SCMI domain provider can be
>> +      "usb_core" or "usb_transfer".
>> +    items:
>> +      - const: usb_core
>> +      - const: usb_transfer
>> +
>> +  hsphy,fw-managed:
>> +    description:
>> +      Some targets allow multiple resources to be managed by firmware.
>> +      On these targets, tasks related to clocks, regulators, resets, and
>> +      interconnects can be delegated to the firmware, while the remaining
>> +      responsibilities are handled by Linux.
>> +
>> +      Decide if the target resources will be managed by firmware or High level
>> +      OS.
>> +    type: boolean
>> +
>>     resets:
>>       items:
>>         - description: PHY core reset
>> @@ -154,12 +182,21 @@ required:
>>     - compatible
>>     - reg
>>     - "#phy-cells"
>> -  - clocks
>> -  - clock-names
>> -  - resets
>> -  - vdda-pll-supply
>> -  - vdda18-supply
>> -  - vdda33-supply
>> +
>> +
>> +allOf:
>> +  - if:
>> +      not:
>> +        required:
>> +          - hsphy,fw-managed
>> +    then:
>> +      required:
>> +        - clocks
>> +        - clock-names
>> +        - resets
>> +        - vdda-pll-supply
>> +        - vdda18-supply
>> +        - vdda33-supply
>>
>>   additionalProperties: false
>>
>> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> index 63d150b..5bf3a29 100644
>> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> @@ -64,7 +64,31 @@ properties:
>>
>>     power-domains:
>>       description: specifies a phandle to PM domain provider node
>> -    maxItems: 1
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  power-domain-names:
>> +    description:
>> +      A list of power domain name strings sorted in the same order as the
>> +      power-domains property.
>> +
>> +      For platforms where some resource are firmware managed, the name
>> +      corresponding to the index of an SCMI domain provider can be
>> +      "usb_core" or "usb_transfer".
>> +    items:
>> +      - const: usb_core
>> +      - const: usb_transfer
>> +
>> +  qcom,fw-managed:
>> +    description:
>> +      Some targets allow multiple resources to be managed by firmware.
>> +      On these targets, tasks related to clocks, regulators, resets, and
>> +      interconnects can be delegated to the firmware, while the remaining
>> +      responsibilities are handled by Linux.
>> +
>> +      Decide if the target resources will be managed by firmware or High level
>> +      OS.
>> +    type: boolean
> 
> I think this is an overkill. You know that SA8775 is going to use
> SCMI-based clocks / PD management. Thus I'd suggest adding new
> bindings file targeting qcom,sa8775-dwc3. Also you can drop the
> qcom,fw-managed property at all, let the driver decide basing on the
> compat string.
> 
> 

Thank you for the suggestion Dmitry. I will include
new compat string for SA8775 which will decide whether
to use scmi based clock/ PD.

>>
>>     required-opps:
>>       maxItems: 1
>> @@ -148,13 +172,20 @@ required:
>>     - "#address-cells"
>>     - "#size-cells"
>>     - ranges
>> -  - clocks
>> -  - clock-names
>>     - interrupts
>>     - interrupt-names
>>
>>   allOf:
>>     - if:
>> +      not:
>> +        required:
>> +          - qcom,fw-managed
>> +    then:
>> +      required:
>> +        - clocks
>> +        - clock-names
>> +
>> +  - if:
>>         properties:
>>           compatible:
>>             contains:
>> --
>> 2.7.4
>>
>>
> 
> 
> --
> With best wishes
> Dmitry

Re: [RFC 1/3] dt-bindings: usb: qcom,dwc3: Add support for multiple power-domains

[Dmitry Baryshkov]

On Wed, 6 Mar 2024 at 17:52, Sriram Dash <quic_sriramd@quicinc.com> wrote:
>
> On 3/6/2024 12:33 AM, Dmitry Baryshkov wrote:
> > On Tue, 5 Mar 2024 at 18:58, Sriram Dash <quic_sriramd@quicinc.com> wrote:
> >>
> >> Some target systems allow multiple resources to be managed by firmware.
> >> On these targets, tasks related to clocks, regulators, resets, and
> >> interconnects can be delegated to the firmware, while the remaining
> >> responsibilities are handled by Linux.
> >>
> >> To support the management of partial resources in Linux and leave the rest
> >> to firmware, multiple power domains are introduced. Each power domain can
> >> manage one or more resources, depending on the specific use case.
> >>
> >> These power domains handle SCMI calls to the firmware, enabling the
> >> activation and deactivation of firmware-managed resources.
> >>
> >> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
> >> ---
> >>   .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 74 ++++++++++++++++------
> >>   .../bindings/phy/qcom,usb-snps-femto-v2.yaml       | 49 ++++++++++++--
> >>   .../devicetree/bindings/usb/qcom,dwc3.yaml         | 37 ++++++++++-
> >>   3 files changed, 130 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >> index 1e2d4dd..53b9ba9 100644
> >> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >> @@ -44,7 +44,32 @@ properties:
> >>       maxItems: 5
> >>
> >>     power-domains:
> >> -    maxItems: 1
> >> +    description: specifies a phandle to PM domain provider node
> >> +    minItems: 1
> >> +    maxItems: 2
> >> +
> >> +  power-domain-names:
> >> +    description:
> >> +      A list of power domain name strings sorted in the same order as the
> >> +      power-domains property.
> >> +
> >> +      For platforms where some resource are firmware managed, the name
> >> +      corresponding to the index of an SCMI domain provider can be
> >> +      "usb_core" or "usb_transfer".
> >> +    items:
> >> +      - const: usb_core
> >> +      - const: usb_transfer
> >> +
> >> +  qmp,fw-managed:
> >> +    description:
> >> +      Some targets allow multiple resources to be managed by firmware.
> >> +      On these targets, tasks related to clocks, regulators, resets, and
> >> +      interconnects can be delegated to the firmware, while the remaining
> >> +      responsibilities are handled by Linux.
> >> +
> >> +      Decide if the target resources will be managed by firmware or High level
> >> +      OS.
> >> +    type: boolean
> >>
> >>     resets:
> >>       maxItems: 2
> >> @@ -70,14 +95,6 @@ properties:
> >>   required:
> >>     - compatible
> >>     - reg
> >> -  - clocks
> >> -  - clock-names
> >> -  - resets
> >> -  - reset-names
> >> -  - vdda-phy-supply
> >> -  - vdda-pll-supply
> >> -  - "#clock-cells"
> >> -  - clock-output-names
> >>     - "#phy-cells"
> >>
> >>   allOf:
> >> @@ -86,6 +103,33 @@ allOf:
> >>           compatible:
> >>             contains:
> >>               enum:
> >> +              - qcom,sa8775p-qmp-usb3-uni-phy
> >> +              - qcom,sc8280xp-qmp-usb3-uni-phy
> >> +              - qcom,x1e80100-qmp-usb3-uni-phy
> >> +    then:
> >> +      required:
> >> +        - power-domains
> >> +
> >> +  - if:
> >> +      not:
> >> +        required:
> >> +          - qmp,fw-managed
> >> +    then:
> >> +      required:
> >> +        - clocks
> >> +        - clock-names
> >> +        - resets
> >> +        - reset-names
> >> +        - vdda-phy-supply
> >> +        - vdda-pll-supply
> >> +        - clock-output-names
> >> +        - "#clock-cells"
> >> +
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            enum:
> >>                 - qcom,ipq6018-qmp-usb3-phy
> >>                 - qcom,ipq8074-qmp-usb3-phy
> >>                 - qcom,ipq9574-qmp-usb3-phy
> >> @@ -144,18 +188,6 @@ allOf:
> >>               - const: com_aux
> >>               - const: pipe
> >>
> >> -  - if:
> >> -      properties:
> >> -        compatible:
> >> -          contains:
> >> -            enum:
> >> -              - qcom,sa8775p-qmp-usb3-uni-phy
> >> -              - qcom,sc8280xp-qmp-usb3-uni-phy
> >> -              - qcom,x1e80100-qmp-usb3-uni-phy
> >> -    then:
> >> -      required:
> >> -        - power-domains
> >> -
> >>   additionalProperties: false
> >>
> >>   examples:
> >> diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
> >> index 0f200e3..ad2f08f 100644
> >> --- a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
> >> +++ b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml
> >> @@ -49,6 +49,34 @@ properties:
> >>       items:
> >>         - const: ref
> >>
> >> +  power-domains:
> >> +    description: specifies a phandle to PM domain provider node
> >> +    minItems: 1
> >> +    maxItems: 2
> >> +
> >> +  power-domain-names:
> >> +    description:
> >> +      A list of power domain name strings sorted in the same order as the
> >> +      power-domains property.
> >> +
> >> +      For platforms where some resource are firmware managed, the name
> >> +      corresponding to the index of an SCMI domain provider can be
> >> +      "usb_core" or "usb_transfer".
> >> +    items:
> >> +      - const: usb_core
> >> +      - const: usb_transfer
> >> +
> >> +  hsphy,fw-managed:
> >> +    description:
> >> +      Some targets allow multiple resources to be managed by firmware.
> >> +      On these targets, tasks related to clocks, regulators, resets, and
> >> +      interconnects can be delegated to the firmware, while the remaining
> >> +      responsibilities are handled by Linux.
> >> +
> >> +      Decide if the target resources will be managed by firmware or High level
> >> +      OS.
> >> +    type: boolean
> >> +
> >>     resets:
> >>       items:
> >>         - description: PHY core reset
> >> @@ -154,12 +182,21 @@ required:
> >>     - compatible
> >>     - reg
> >>     - "#phy-cells"
> >> -  - clocks
> >> -  - clock-names
> >> -  - resets
> >> -  - vdda-pll-supply
> >> -  - vdda18-supply
> >> -  - vdda33-supply
> >> +
> >> +
> >> +allOf:
> >> +  - if:
> >> +      not:
> >> +        required:
> >> +          - hsphy,fw-managed
> >> +    then:
> >> +      required:
> >> +        - clocks
> >> +        - clock-names
> >> +        - resets
> >> +        - vdda-pll-supply
> >> +        - vdda18-supply
> >> +        - vdda33-supply
> >>
> >>   additionalProperties: false
> >>
> >> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> >> index 63d150b..5bf3a29 100644
> >> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> >> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> >> @@ -64,7 +64,31 @@ properties:
> >>
> >>     power-domains:
> >>       description: specifies a phandle to PM domain provider node
> >> -    maxItems: 1
> >> +    minItems: 1
> >> +    maxItems: 2
> >> +
> >> +  power-domain-names:
> >> +    description:
> >> +      A list of power domain name strings sorted in the same order as the
> >> +      power-domains property.
> >> +
> >> +      For platforms where some resource are firmware managed, the name
> >> +      corresponding to the index of an SCMI domain provider can be
> >> +      "usb_core" or "usb_transfer".
> >> +    items:
> >> +      - const: usb_core
> >> +      - const: usb_transfer
> >> +
> >> +  qcom,fw-managed:
> >> +    description:
> >> +      Some targets allow multiple resources to be managed by firmware.
> >> +      On these targets, tasks related to clocks, regulators, resets, and
> >> +      interconnects can be delegated to the firmware, while the remaining
> >> +      responsibilities are handled by Linux.
> >> +
> >> +      Decide if the target resources will be managed by firmware or High level
> >> +      OS.
> >> +    type: boolean
> >
> > I think this is an overkill. You know that SA8775 is going to use
> > SCMI-based clocks / PD management. Thus I'd suggest adding new
> > bindings file targeting qcom,sa8775-dwc3. Also you can drop the
> > qcom,fw-managed property at all, let the driver decide basing on the
> > compat string.
> >
> >
>
> Thank you for the suggestion Dmitry. I will include
> new compat string for SA8775 which will decide whether
> to use scmi based clock/ PD.

As a reminder:
- same hardware = same compatible string
- existing DT better to continue to work. Or ask for explicit
permission from Bjorn to break the ABI.

>
> >>
> >>     required-opps:
> >>       maxItems: 1
> >> @@ -148,13 +172,20 @@ required:
> >>     - "#address-cells"
> >>     - "#size-cells"
> >>     - ranges
> >> -  - clocks
> >> -  - clock-names
> >>     - interrupts
> >>     - interrupt-names
> >>
> >>   allOf:
> >>     - if:
> >> +      not:
> >> +        required:
> >> +          - qcom,fw-managed
> >> +    then:
> >> +      required:
> >> +        - clocks
> >> +        - clock-names
> >> +
> >> +  - if:
> >>         properties:
> >>           compatible:
> >>             contains:
> >> --
> >> 2.7.4
> >>
> >>
> >
> >
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry

[RFC 2/3] USB: dwc3: qcom: Add support for firmware managed resources

[Sriram Dash]

Some target systems allow multiple resources to be managed by firmware.
On these targets, tasks related to clocks, regulators, resets, and
interconnects can be delegated to the firmware, while the remaining
responsibilities are handled by Linux.

The driver is responsible for managing multiple power domains and
linking them to consumers as needed. Incase there is only single
power domain, it is considered to be a standard GDSC hooked on to
the qcom dt node which is read and assigned to device structure
(by genpd framework) before the driver probe even begins.

This differentiation logic allows the driver to determine whether
device resources are managed by Linux or firmware, ensuring
backward compatibility.

Furthermore, minor cleanup is performed for the private data of
the SNPS Femto PHY. However, ACPI handling is omitted due to the
absence of clients on the ACPI side.

Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-usb.c       | 290 ++++++++++++++++++++------
 drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 213 +++++++++++++++----
 drivers/usb/dwc3/dwc3-qcom.c                  | 259 +++++++++++++++++------
 3 files changed, 594 insertions(+), 168 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
index 8525393..1ac1b50 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
@@ -21,6 +21,9 @@
 
 #include "phy-qcom-qmp-common.h"
 
+#include <linux/pm_opp.h>
+#include <linux/pm_domain.h>
+
 #include "phy-qcom-qmp.h"
 #include "phy-qcom-qmp-pcs-misc-v3.h"
 #include "phy-qcom-qmp-pcs-misc-v4.h"
@@ -1212,6 +1215,9 @@ struct qmp_phy_cfg {
 	unsigned int pcs_usb_offset;
 };
 
+#define DOMAIN_GENPD_TRANSFER			0
+#define DOMAIN_GENPD_CORE			1
+
 struct qmp_usb {
 	struct device *dev;
 
@@ -1236,6 +1242,19 @@ struct qmp_usb {
 	struct phy *phy;
 
 	struct clk_fixed_rate pipe_clk_fixed;
+
+	struct dev_pm_domain_list *pd_list;
+	struct device *genpd_core;
+	struct device *genpd_transfer;
+
+	bool fw_managed;
+	/* separate resource management for fw_managed vs locally managed devices */
+	struct qmp_usb_device_ops {
+		int (*bus_resume_resource)(struct qmp_usb *qmp);
+		int (*runtime_resume_resource)(struct qmp_usb *qmp);
+		int (*bus_suspend_resource)(struct qmp_usb *qmp);
+		int (*runtime_suspend_resource)(struct qmp_usb *qmp);
+	} qmp_usb_device_ops;
 };
 
 static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
@@ -1598,6 +1617,41 @@ static const struct qmp_phy_cfg x1e80100_usb3_uniphy_cfg = {
 	.regs			= qmp_v7_usb3phy_regs_layout,
 };
 
+static void qmp_fw_managed_domain_remove(struct qmp_usb *qmp)
+{
+	dev_pm_domain_detach_list(qmp->pd_list);
+}
+
+static int qmp_fw_managed_domain_init(struct qmp_usb *qmp)
+{
+	struct device *dev = qmp->dev;
+	struct dev_pm_domain_attach_data pd_data = {
+		.pd_flags	= PD_FLAG_NO_DEV_LINK,
+		.pd_names	= (const char*[]) { "usb_transfer", "usb_core" },
+		.num_pd_names	= 2,
+	};
+	int ret = 0;
+
+	if (!dev->pm_domain) {
+		ret = dev_pm_domain_attach_list(dev, &pd_data, &qmp->pd_list);
+		if (ret < 0) {
+			dev_err(dev, "domain attach failed %d)\n", ret);
+			return ret;
+		}
+
+		qmp->genpd_transfer = qmp->pd_list->pd_devs[DOMAIN_GENPD_TRANSFER];
+		qmp->genpd_core = qmp->pd_list->pd_devs[DOMAIN_GENPD_CORE];
+
+
+		dev_dbg(dev, "domains attached successfully\n");
+	} else {
+		dev_dbg(dev, "domain attach fail\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int qmp_usb_serdes_init(struct qmp_usb *qmp)
 {
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
@@ -1610,11 +1664,20 @@ static int qmp_usb_serdes_init(struct qmp_usb *qmp)
 	return 0;
 }
 
-static int qmp_usb_init(struct phy *phy)
+static int fw_managed_ssphy_bus_init(struct qmp_usb *qmp)
+{
+	int ret = 0;
+
+	ret = pm_runtime_get_sync(qmp->genpd_core);
+	if (ret < 0)
+		dev_err(qmp->dev, "Failed to enable fw managed resources");
+
+	return ret;
+}
+
+static int locally_managed_ssphy_bus_init(struct qmp_usb *qmp)
 {
-	struct qmp_usb *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
-	void __iomem *pcs = qmp->pcs;
 	int ret;
 
 	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
@@ -1639,8 +1702,11 @@ static int qmp_usb_init(struct phy *phy)
 	if (ret)
 		goto err_assert_reset;
 
-	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
-
+	ret = clk_prepare_enable(qmp->pipe_clk);
+	if (ret) {
+		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
+		return ret;
+	}
 	return 0;
 
 err_assert_reset:
@@ -1651,11 +1717,22 @@ static int qmp_usb_init(struct phy *phy)
 	return ret;
 }
 
-static int qmp_usb_exit(struct phy *phy)
+static int fw_managed_ssphy_bus_exit(struct qmp_usb *qmp)
+{
+	int ret = 0;
+
+	ret = pm_runtime_put_sync(qmp->genpd_core);
+	if (ret < 0)
+		dev_err(qmp->dev, "Failed to disable fw managed resources");
+
+	return 0;
+}
+
+static int locally_managed_ssphy_bus_exit(struct qmp_usb *qmp)
 {
-	struct qmp_usb *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 
+	clk_disable_unprepare(qmp->pipe_clk);
 	reset_control_bulk_assert(qmp->num_resets, qmp->resets);
 
 	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
@@ -1677,13 +1754,9 @@ static int qmp_usb_power_on(struct phy *phy)
 	unsigned int val;
 	int ret;
 
-	qmp_usb_serdes_init(qmp);
+	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
 
-	ret = clk_prepare_enable(qmp->pipe_clk);
-	if (ret) {
-		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
-		return ret;
-	}
+	qmp_usb_serdes_init(qmp);
 
 	/* Tx, Rx, and PCS configurations */
 	qmp_configure_lane(tx, cfg->tx_tbl, cfg->tx_tbl_num, 1);
@@ -1708,15 +1781,10 @@ static int qmp_usb_power_on(struct phy *phy)
 				 PHY_INIT_COMPLETE_TIMEOUT);
 	if (ret) {
 		dev_err(qmp->dev, "phy initialization timed-out\n");
-		goto err_disable_pipe_clk;
+		return ret;
 	}
 
 	return 0;
-
-err_disable_pipe_clk:
-	clk_disable_unprepare(qmp->pipe_clk);
-
-	return ret;
 }
 
 static int qmp_usb_power_off(struct phy *phy)
@@ -1724,8 +1792,6 @@ static int qmp_usb_power_off(struct phy *phy)
 	struct qmp_usb *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 
-	clk_disable_unprepare(qmp->pipe_clk);
-
 	/* PHY reset */
 	qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
 
@@ -1742,27 +1808,30 @@ static int qmp_usb_power_off(struct phy *phy)
 
 static int qmp_usb_enable(struct phy *phy)
 {
+	struct qmp_usb *qmp = phy_get_drvdata(phy);
 	int ret;
 
-	ret = qmp_usb_init(phy);
+	ret = qmp->qmp_usb_device_ops.bus_resume_resource(qmp);
 	if (ret)
 		return ret;
 
 	ret = qmp_usb_power_on(phy);
 	if (ret)
-		qmp_usb_exit(phy);
+		qmp->qmp_usb_device_ops.bus_suspend_resource(qmp);
 
 	return ret;
 }
 
 static int qmp_usb_disable(struct phy *phy)
 {
+	struct qmp_usb *qmp = phy_get_drvdata(phy);
 	int ret;
 
 	ret = qmp_usb_power_off(phy);
 	if (ret)
 		return ret;
-	return qmp_usb_exit(phy);
+
+	return qmp->qmp_usb_device_ops.bus_suspend_resource(qmp);
 }
 
 static int qmp_usb_set_mode(struct phy *phy, enum phy_mode mode, int submode)
@@ -1828,6 +1897,25 @@ static void qmp_usb_disable_autonomous_mode(struct qmp_usb *qmp)
 	qphy_clrbits(pcs_usb, cfg->regs[QPHY_PCS_LFPS_RXTERM_IRQ_CLEAR], IRQ_CLEAR);
 }
 
+static int locally_managed_ssphy_runtime_exit(struct qmp_usb *qmp)
+{
+	clk_disable_unprepare(qmp->pipe_clk);
+	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
+
+	return 0;
+}
+
+static int fw_managed_ssphy_runtime_exit(struct qmp_usb *qmp)
+{
+	int ret = 0;
+
+	ret = pm_runtime_put_sync(qmp->genpd_transfer);
+	if (ret < 0)
+		dev_err(qmp->dev, "Failed to disable fw managed resources");
+
+	return 0;
+}
+
 static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev)
 {
 	struct qmp_usb *qmp = dev_get_drvdata(dev);
@@ -1841,16 +1929,44 @@ static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev)
 
 	qmp_usb_enable_autonomous_mode(qmp);
 
-	clk_disable_unprepare(qmp->pipe_clk);
-	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
+	qmp->qmp_usb_device_ops.runtime_suspend_resource(qmp);
 
 	return 0;
 }
 
+static int locally_managed_ssphy_runtime_init(struct qmp_usb *qmp)
+{
+	int ret = 0;
+
+	ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(qmp->pipe_clk);
+	if (ret) {
+		dev_err(qmp->dev, "pipe_clk enable failed, err=%d\n", ret);
+		clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int fw_managed_ssphy_runtime_init(struct qmp_usb *qmp)
+{
+	int ret = 0;
+
+	ret = pm_runtime_get_sync(qmp->genpd_transfer);
+	if (ret < 0)
+		dev_err(qmp->dev, "Failed to enable fw managed resources");
+
+	return ret;
+}
+
 static int __maybe_unused qmp_usb_runtime_resume(struct device *dev)
 {
+	int ret;
 	struct qmp_usb *qmp = dev_get_drvdata(dev);
-	int ret = 0;
 
 	dev_vdbg(dev, "Resuming QMP phy, mode:%d\n", qmp->mode);
 
@@ -1859,17 +1975,10 @@ static int __maybe_unused qmp_usb_runtime_resume(struct device *dev)
 		return 0;
 	}
 
-	ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
+	ret = qmp->qmp_usb_device_ops.runtime_resume_resource(qmp);
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(qmp->pipe_clk);
-	if (ret) {
-		dev_err(dev, "pipe_clk enable failed, err=%d\n", ret);
-		clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
-		return ret;
-	}
-
 	qmp_usb_disable_autonomous_mode(qmp);
 
 	return 0;
@@ -2059,22 +2168,24 @@ static int qmp_usb_parse_dt_legacy(struct qmp_usb *qmp, struct device_node *np)
 		qmp->pcs_misc = NULL;
 	}
 
-	qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL);
-	if (IS_ERR(qmp->pipe_clk)) {
-		return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
-				     "failed to get pipe clock\n");
-	}
+	if (!qmp->fw_managed) {
+		qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL);
+		if (IS_ERR(qmp->pipe_clk)) {
+			return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
+					     "failed to get pipe clock\n");
+		}
 
-	ret = devm_clk_bulk_get_all(qmp->dev, &qmp->clks);
-	if (ret < 0)
-		return ret;
+		ret = devm_clk_bulk_get_all(qmp->dev, &qmp->clks);
+		if (ret < 0)
+			return ret;
 
-	qmp->num_clks = ret;
+		qmp->num_clks = ret;
 
-	ret = qmp_usb_reset_init(qmp, usb3phy_legacy_reset_l,
-				 ARRAY_SIZE(usb3phy_legacy_reset_l));
-	if (ret)
-		return ret;
+		ret = qmp_usb_reset_init(qmp, usb3phy_legacy_reset_l,
+					 ARRAY_SIZE(usb3phy_legacy_reset_l));
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -2104,21 +2215,23 @@ static int qmp_usb_parse_dt(struct qmp_usb *qmp)
 	qmp->tx = base + offs->tx;
 	qmp->rx = base + offs->rx;
 
-	ret = qmp_usb_clk_init(qmp);
-	if (ret)
-		return ret;
-
-	qmp->pipe_clk = devm_clk_get(dev, "pipe");
-	if (IS_ERR(qmp->pipe_clk)) {
-		return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
-				     "failed to get pipe clock\n");
+	if (!qmp->fw_managed) {
+		ret = qmp_usb_clk_init(qmp);
+		if (ret)
+			return ret;
+
+		qmp->pipe_clk = devm_clk_get(dev, "pipe");
+		if (IS_ERR(qmp->pipe_clk)) {
+			return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
+					     "failed to get pipe clock\n");
+		}
+
+		ret = qmp_usb_reset_init(qmp, usb3phy_reset_l,
+					 ARRAY_SIZE(usb3phy_reset_l));
+		if (ret)
+			return ret;
 	}
 
-	ret = qmp_usb_reset_init(qmp, usb3phy_reset_l,
-				 ARRAY_SIZE(usb3phy_reset_l));
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
@@ -2140,9 +2253,36 @@ static int qmp_usb_probe(struct platform_device *pdev)
 	if (!qmp->cfg)
 		return -EINVAL;
 
-	ret = qmp_usb_vreg_init(qmp);
-	if (ret)
-		return ret;
+	qmp->fw_managed  = device_property_read_bool(dev, "qmp,fw-managed");
+	if (qmp->fw_managed) {
+		ret = qmp_fw_managed_domain_init(qmp);
+		if (ret) {
+			dev_err(dev, "Failed to init domains. Bail out");
+			return ret;
+		}
+
+		qmp->qmp_usb_device_ops.bus_resume_resource =
+						fw_managed_ssphy_bus_init;
+		qmp->qmp_usb_device_ops.runtime_resume_resource =
+						fw_managed_ssphy_runtime_init;
+		qmp->qmp_usb_device_ops.bus_suspend_resource =
+						fw_managed_ssphy_bus_exit;
+		qmp->qmp_usb_device_ops.runtime_suspend_resource =
+						fw_managed_ssphy_runtime_exit;
+	} else {
+		ret = qmp_usb_vreg_init(qmp);
+		if (ret)
+			return ret;
+
+		qmp->qmp_usb_device_ops.bus_resume_resource =
+						locally_managed_ssphy_bus_init;
+		qmp->qmp_usb_device_ops.runtime_resume_resource =
+						locally_managed_ssphy_runtime_init;
+		qmp->qmp_usb_device_ops.bus_suspend_resource =
+						locally_managed_ssphy_bus_exit;
+		qmp->qmp_usb_device_ops.runtime_suspend_resource =
+						locally_managed_ssphy_runtime_exit;
+	}
 
 	/* Check for legacy binding with child node. */
 	np = of_get_next_available_child(dev->of_node, NULL);
@@ -2165,9 +2305,11 @@ static int qmp_usb_probe(struct platform_device *pdev)
 	 */
 	pm_runtime_forbid(dev);
 
-	ret = phy_pipe_clk_register(qmp, np);
-	if (ret)
-		goto err_node_put;
+	if (!qmp->fw_managed) {
+		ret = phy_pipe_clk_register(qmp, np);
+		if (ret)
+			goto err_node_put;
+	}
 
 	qmp->phy = devm_phy_create(dev, np, &qmp_usb_phy_ops);
 	if (IS_ERR(qmp->phy)) {
@@ -2186,9 +2328,22 @@ static int qmp_usb_probe(struct platform_device *pdev)
 
 err_node_put:
 	of_node_put(np);
+	if (qmp->fw_managed)
+		qmp_fw_managed_domain_remove(qmp);
 	return ret;
 }
 
+static void qmp_usb_remove(struct platform_device *pdev)
+{
+	struct qmp_usb *qmp = platform_get_drvdata(pdev);
+
+	if (qmp->fw_managed) {
+		pm_runtime_put_sync(qmp->genpd_core);
+		qmp_fw_managed_domain_remove(qmp);
+	}
+}
+
+
 static const struct of_device_id qmp_usb_of_match_table[] = {
 	{
 		.compatible = "qcom,ipq6018-qmp-usb3-phy",
@@ -2239,6 +2394,7 @@ MODULE_DEVICE_TABLE(of, qmp_usb_of_match_table);
 
 static struct platform_driver qmp_usb_driver = {
 	.probe		= qmp_usb_probe,
+	.remove_new	= qmp_usb_remove,
 	.driver = {
 		.name	= "qcom-qmp-usb-phy",
 		.pm	= &qmp_usb_pm_ops,
diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
index eb0b0f6..9a1377f 100644
--- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
+++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
@@ -17,6 +17,9 @@
 #include <linux/reset.h>
 #include <linux/slab.h>
 
+#include <linux/pm_opp.h>
+#include <linux/pm_domain.h>
+
 #define USB2_PHY_USB_PHY_UTMI_CTRL0		(0x3c)
 #define SLEEPM					BIT(0)
 #define OPMODE_MASK				GENMASK(4, 3)
@@ -89,7 +92,7 @@ struct override_param {
 	u8	reg_val;
 };
 
-struct override_param_map {
+struct snps_femto_v2_priv_data {
 	const char *prop_name;
 	const struct override_param *param_table;
 	u8 table_size;
@@ -106,6 +109,9 @@ struct phy_override_seq {
 
 #define NUM_HSPHY_TUNING_PARAMS	(9)
 
+#define DOMAIN_GENPD_TRANSFER			0
+#define DOMAIN_GENPD_CORE			1
+
 /**
  * struct qcom_snps_hsphy - snps hs phy attributes
  *
@@ -136,8 +142,54 @@ struct qcom_snps_hsphy {
 	bool phy_initialized;
 	enum phy_mode mode;
 	struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS];
+
+	struct dev_pm_domain_list *pd_list;
+	struct device *genpd_core;
+	struct device *genpd_transfer;
+
+	bool fw_managed;
+	/* separate resource management for fw_managed vs locally managed devices */
+	struct snps_femto_v2_device_ops {
+		int (*resume_resource)(struct qcom_snps_hsphy *hsphy);
+		int (*suspend_resource)(struct qcom_snps_hsphy *hsphy);
+	} snps_femto_v2_device_ops;
+
 };
 
+static void hsphy_fw_managed_domain_remove(struct qcom_snps_hsphy *hsphy)
+{
+	dev_pm_domain_detach_list(hsphy->pd_list);
+}
+
+static int hsphy_fw_managed_domain_init(struct qcom_snps_hsphy *hsphy)
+{
+	struct device *dev = hsphy->dev;
+	struct dev_pm_domain_attach_data pd_data = {
+		.pd_flags	= PD_FLAG_NO_DEV_LINK,
+		.pd_names	= (const char*[]) { "usb_transfer", "usb_core" },
+		.num_pd_names	= 2,
+	};
+	int ret = 0;
+
+	if (!dev->pm_domain) {
+		ret = dev_pm_domain_attach_list(dev, &pd_data, &hsphy->pd_list);
+		if (ret < 0) {
+			dev_err(dev, "domain attach failed %d)\n", ret);
+			return ret;
+		}
+
+		hsphy->genpd_transfer = hsphy->pd_list->pd_devs[DOMAIN_GENPD_TRANSFER];
+		hsphy->genpd_core = hsphy->pd_list->pd_devs[DOMAIN_GENPD_CORE];
+
+		dev_dbg(dev, "domains attached successfully\n");
+	} else {
+		dev_dbg(dev, "domain attach fail\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy)
 {
 	struct device *dev = hsphy->dev;
@@ -316,27 +368,27 @@ static const struct override_param ls_fs_output_impedance_sc7280[] = {
 	{ 1310, 0 },
 };
 
-static const struct override_param_map sc7280_snps_7nm_phy[] = {
+static const struct snps_femto_v2_priv_data sc7280_snps_7nm_phy[] = {
 	{
 		"qcom,hs-disconnect-bp",
 		hs_disconnect_sc7280,
 		ARRAY_SIZE(hs_disconnect_sc7280),
 		USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
-		HS_DISCONNECT_MASK
+		HS_DISCONNECT_MASK,
 	},
 	{
 		"qcom,squelch-detector-bp",
 		squelch_det_threshold_sc7280,
 		ARRAY_SIZE(squelch_det_threshold_sc7280),
 		USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
-		SQUELCH_DETECTOR_MASK
+		SQUELCH_DETECTOR_MASK,
 	},
 	{
 		"qcom,hs-amplitude-bp",
 		hs_amplitude_sc7280,
 		ARRAY_SIZE(hs_amplitude_sc7280),
 		USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
-		HS_AMPLITUDE_MASK
+		HS_AMPLITUDE_MASK,
 	},
 	{
 		"qcom,pre-emphasis-duration-bp",
@@ -357,14 +409,14 @@ static const struct override_param_map sc7280_snps_7nm_phy[] = {
 		hs_rise_fall_time_sc7280,
 		ARRAY_SIZE(hs_rise_fall_time_sc7280),
 		USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
-		HS_RISE_FALL_MASK
+		HS_RISE_FALL_MASK,
 	},
 	{
 		"qcom,hs-crossover-voltage-microvolt",
 		hs_crossover_voltage_sc7280,
 		ARRAY_SIZE(hs_crossover_voltage_sc7280),
 		USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
-		HS_CROSSOVER_VOLTAGE_MASK
+		HS_CROSSOVER_VOLTAGE_MASK,
 	},
 	{
 		"qcom,hs-output-impedance-micro-ohms",
@@ -383,12 +435,31 @@ static const struct override_param_map sc7280_snps_7nm_phy[] = {
 	{},
 };
 
-static int qcom_snps_hsphy_init(struct phy *phy)
+static int fw_managed_hsphy_init(struct qcom_snps_hsphy *hsphy)
 {
-	struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
-	int ret, i;
+	int ret = 0;
 
-	dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
+	ret = pm_runtime_get_sync(hsphy->genpd_core);
+	if (ret < 0)
+		dev_err(hsphy->dev, "Failed to enable fw managed resources");
+
+	return ret;
+}
+
+static int fw_managed_hsphy_exit(struct qcom_snps_hsphy *hsphy)
+{
+	int ret = 0;
+
+	ret = pm_runtime_put_sync(hsphy->genpd_core);
+	if (ret < 0)
+		dev_err(hsphy->dev, "Failed to disable fw managed resources");
+
+	return 0;
+}
+
+static int locally_managed_hsphy_init(struct qcom_snps_hsphy *hsphy)
+{
+	int ret;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
 	if (ret)
@@ -396,13 +467,13 @@ static int qcom_snps_hsphy_init(struct phy *phy)
 
 	ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks);
 	if (ret) {
-		dev_err(&phy->dev, "failed to enable clocks, %d\n", ret);
+		dev_err(hsphy->dev, "failed to enable clocks, %d\n", ret);
 		goto poweroff_phy;
 	}
 
 	ret = reset_control_assert(hsphy->phy_reset);
 	if (ret) {
-		dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret);
+		dev_err(hsphy->dev, "failed to assert phy_reset, %d\n", ret);
 		goto disable_clks;
 	}
 
@@ -410,10 +481,42 @@ static int qcom_snps_hsphy_init(struct phy *phy)
 
 	ret = reset_control_deassert(hsphy->phy_reset);
 	if (ret) {
-		dev_err(&phy->dev, "failed to de-assert phy_reset, %d\n", ret);
+		dev_err(hsphy->dev, "failed to de-assert phy_reset, %d\n", ret);
 		goto disable_clks;
 	}
 
+	return 0;
+
+disable_clks:
+	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
+poweroff_phy:
+	regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
+
+	return ret;
+}
+
+static int locally_managed_hsphy_exit(struct qcom_snps_hsphy *hsphy)
+{
+	reset_control_assert(hsphy->phy_reset);
+	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
+	regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
+
+	return 0;
+}
+
+static int qcom_snps_hsphy_init(struct phy *phy)
+{
+	struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
+	int ret, i;
+
+	dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
+
+	ret = hsphy->snps_femto_v2_device_ops.resume_resource(hsphy);
+	if (ret) {
+		dev_err(&phy->dev, "Error resumeing hsphy resources %d\n", ret);
+		return ret;
+	}
+
 	qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_CFG0,
 					UTMI_PHY_CMN_CTRL_OVERRIDE_EN,
 					UTMI_PHY_CMN_CTRL_OVERRIDE_EN);
@@ -467,22 +570,19 @@ static int qcom_snps_hsphy_init(struct phy *phy)
 	hsphy->phy_initialized = true;
 
 	return 0;
-
-disable_clks:
-	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
-poweroff_phy:
-	regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
-
-	return ret;
 }
 
 static int qcom_snps_hsphy_exit(struct phy *phy)
 {
 	struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
+	int ret = 0;
+
+	ret = hsphy->snps_femto_v2_device_ops.suspend_resource(hsphy);
+	if (ret) {
+		dev_err(&phy->dev, "Error suspending hsphy resources %d\n", ret);
+		return ret;
+	}
 
-	reset_control_assert(hsphy->phy_reset);
-	clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
-	regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
 	hsphy->phy_initialized = false;
 
 	return 0;
@@ -497,7 +597,8 @@ static const struct phy_ops qcom_snps_hsphy_gen_ops = {
 
 static const struct of_device_id qcom_snps_hsphy_of_match_table[] = {
 	{ .compatible	= "qcom,sm8150-usb-hs-phy", },
-	{ .compatible	= "qcom,usb-snps-hs-5nm-phy", },
+	{ .compatible	= "qcom,sa8775p-usb-hs-phy", },
+	{ .compatible	= "qcom,sc8280xp-usb-hs-phy", },
 	{
 		.compatible	= "qcom,usb-snps-hs-7nm-phy",
 		.data		= &sc7280_snps_7nm_phy,
@@ -513,7 +614,7 @@ static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = {
 };
 
 static void qcom_snps_hsphy_override_param_update_val(
-			const struct override_param_map map,
+			const struct snps_femto_v2_priv_data map,
 			s32 dt_val, struct phy_override_seq *seq_entry)
 {
 	int i;
@@ -541,7 +642,7 @@ static void qcom_snps_hsphy_read_override_param_seq(struct device *dev)
 	s32 val;
 	int ret, i;
 	struct qcom_snps_hsphy *hsphy;
-	const struct override_param_map *cfg = of_device_get_match_data(dev);
+	const struct snps_femto_v2_priv_data *cfg = of_device_get_match_data(dev);
 
 	if (!cfg)
 		return;
@@ -580,24 +681,39 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
 	if (IS_ERR(hsphy->base))
 		return PTR_ERR(hsphy->base);
 
-	ret = qcom_snps_hsphy_clk_init(hsphy);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to initialize clocks\n");
 
-	hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
-	if (IS_ERR(hsphy->phy_reset)) {
-		dev_err(dev, "failed to get phy core reset\n");
-		return PTR_ERR(hsphy->phy_reset);
-	}
+	hsphy->fw_managed  = device_property_read_bool(dev, "hsphy,fw-managed");
+	if (hsphy->fw_managed) {
+		ret = hsphy_fw_managed_domain_init(hsphy);
+		if (ret) {
+			dev_err(dev, "Failed to init domains. Bail out");
+			return ret;
+		}
 
-	num = ARRAY_SIZE(hsphy->vregs);
-	for (i = 0; i < num; i++)
-		hsphy->vregs[i].supply = qcom_snps_hsphy_vreg_names[i];
+		hsphy->snps_femto_v2_device_ops.resume_resource = fw_managed_hsphy_init;
+		hsphy->snps_femto_v2_device_ops.suspend_resource = fw_managed_hsphy_exit;
+	} else {
+		ret = qcom_snps_hsphy_clk_init(hsphy);
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to initialize clocks\n");
 
-	ret = devm_regulator_bulk_get(dev, num, hsphy->vregs);
-	if (ret)
-		return dev_err_probe(dev, ret,
-				     "failed to get regulator supplies\n");
+		hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+		if (IS_ERR(hsphy->phy_reset)) {
+			dev_err(dev, "failed to get phy core reset\n");
+			return PTR_ERR(hsphy->phy_reset);
+		}
+
+		num = ARRAY_SIZE(hsphy->vregs);
+		for (i = 0; i < num; i++)
+			hsphy->vregs[i].supply = qcom_snps_hsphy_vreg_names[i];
+
+		ret = devm_regulator_bulk_get(dev, num, hsphy->vregs);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to get regulator supplies\n");
+		hsphy->snps_femto_v2_device_ops.resume_resource = locally_managed_hsphy_init;
+		hsphy->snps_femto_v2_device_ops.suspend_resource = locally_managed_hsphy_exit;
+	}
 
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -609,6 +725,8 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
 
 	generic_phy = devm_phy_create(dev, NULL, &qcom_snps_hsphy_gen_ops);
 	if (IS_ERR(generic_phy)) {
+		if (hsphy->fw_managed)
+			hsphy_fw_managed_domain_remove(hsphy);
 		ret = PTR_ERR(generic_phy);
 		dev_err(dev, "failed to create phy, %d\n", ret);
 		return ret;
@@ -628,8 +746,19 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
 	return PTR_ERR_OR_ZERO(phy_provider);
 }
 
+static void qcom_snps_hsphy_remove(struct platform_device *pdev)
+{
+	struct qcom_snps_hsphy *hsphy = platform_get_drvdata(pdev);
+
+	if (hsphy->fw_managed) {
+		pm_runtime_put_sync(hsphy->genpd_core);
+		hsphy_fw_managed_domain_remove(hsphy);
+	}
+}
+
 static struct platform_driver qcom_snps_hsphy_driver = {
 	.probe		= qcom_snps_hsphy_probe,
+	.remove_new	= qcom_snps_hsphy_remove,
 	.driver = {
 		.name	= "qcom-snps-hs-femto-v2-phy",
 		.pm = &qcom_snps_hsphy_pm_ops,
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index dbd6a5b..aea11d0 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -22,6 +22,8 @@
 #include <linux/iopoll.h>
 #include <linux/usb/hcd.h>
 #include <linux/usb.h>
+#include <linux/pm_opp.h>
+#include <linux/pm_domain.h>
 #include "core.h"
 
 /* USB QSCRATCH Hardware registers */
@@ -53,6 +55,9 @@
 #define APPS_USB_AVG_BW 0
 #define APPS_USB_PEAK_BW MBps_to_icc(40)
 
+#define DOMAIN_GENPD_TRANSFER			0
+#define DOMAIN_GENPD_CORE			1
+
 struct dwc3_acpi_pdata {
 	u32			qscratch_base_offset;
 	u32			qscratch_base_size;
@@ -91,8 +96,54 @@ struct dwc3_qcom {
 	bool			pm_suspended;
 	struct icc_path		*icc_path_ddr;
 	struct icc_path		*icc_path_apps;
+
+	struct dev_pm_domain_list *pd_list;
+	struct device *genpd_core;
+	struct device *genpd_transfer;
+
+	bool			fw_managed;
+	/* separate resource management for fw_managed vs locally managed devices */
+	struct qcom_dwc3_device_ops {
+		int (*resume_resource)(struct dwc3_qcom *qcom);
+		void (*suspend_resource)(struct dwc3_qcom *qcom);
+		void (*exit_resource)(struct dwc3_qcom *qcom);
+	} qcom_dwc3_device_ops;
 };
 
+static void dwc3_qcom_fw_managed_domain_remove(struct dwc3_qcom *qcom)
+{
+	dev_pm_domain_detach_list(qcom->pd_list);
+}
+
+static int dwc3_qcom_fw_managed_domain_init(struct dwc3_qcom *qcom)
+{
+	struct device *dev = qcom->dev;
+	struct dev_pm_domain_attach_data pd_data = {
+		.pd_flags	= PD_FLAG_NO_DEV_LINK,
+		.pd_names	= (const char*[]) { "usb_transfer", "usb_core" },
+		.num_pd_names	= 2,
+	};
+	int ret = 0;
+
+	if (!dev->pm_domain) {
+		ret = dev_pm_domain_attach_list(dev, &pd_data, &qcom->pd_list);
+		if (ret < 0) {
+			dev_err(dev, "domain attach failed %d)\n", ret);
+			return ret;
+		}
+
+		qcom->genpd_transfer = qcom->pd_list->pd_devs[DOMAIN_GENPD_TRANSFER];
+		qcom->genpd_core = qcom->pd_list->pd_devs[DOMAIN_GENPD_CORE];
+
+		dev_dbg(dev, "domains attached successfully\n");
+	} else {
+		dev_dbg(dev, "domain attach fail\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
 {
 	u32 reg;
@@ -417,10 +468,87 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 	dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0);
 }
 
+static void locally_managed_exit_resource(struct dwc3_qcom *qcom)
+{
+	int i;
+
+	for (i = qcom->num_clocks - 1; i >= 0; i--) {
+		clk_disable_unprepare(qcom->clks[i]);
+		clk_put(qcom->clks[i]);
+	}
+	qcom->num_clocks = 0;
+
+	dwc3_qcom_interconnect_exit(qcom);
+	reset_control_assert(qcom->resets);
+}
+
+static void fw_managed_exit_resource(struct dwc3_qcom *qcom)
+{
+	int ret = 0;
+
+	ret = pm_runtime_put_sync(qcom->genpd_core);
+
+	if (ret < 0)
+		dev_err(qcom->dev, "Failed to disable fw managed resources");
+
+	dwc3_qcom_fw_managed_domain_remove(qcom);
+}
+
+static int locally_managed_resume_resource(struct dwc3_qcom *qcom)
+{
+	int ret, i;
+
+	for (i = 0; i < qcom->num_clocks; i++) {
+		ret = clk_prepare_enable(qcom->clks[i]);
+		if (ret < 0) {
+			while (--i >= 0)
+				clk_disable_unprepare(qcom->clks[i]);
+			return ret;
+		}
+	}
+
+	ret = dwc3_qcom_interconnect_enable(qcom);
+	if (ret)
+		dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
+
+	return 0;
+}
+
+static int fw_managed_resume_resource(struct dwc3_qcom *qcom)
+{
+	int ret = 0;
+
+	ret = pm_runtime_get_sync(qcom->genpd_transfer);
+	if (ret < 0)
+		dev_err(qcom->dev, "Failed to enable fw managed resources");
+
+	return ret;
+}
+
+static void locally_managed_suspend_resource(struct dwc3_qcom *qcom)
+{
+	int i, ret;
+
+	for (i = qcom->num_clocks - 1; i >= 0; i--)
+		clk_disable_unprepare(qcom->clks[i]);
+
+	ret = dwc3_qcom_interconnect_disable(qcom);
+	if (ret)
+		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
+}
+
+static void fw_managed_suspend_resource(struct dwc3_qcom *qcom)
+{
+	int ret = 0;
+
+	ret = pm_runtime_put_sync(qcom->genpd_transfer);
+	if (ret < 0)
+		dev_err(qcom->dev, "Failed to disable fw managed resources");
+}
+
 static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
 {
 	u32 val;
-	int i, ret;
 
 	if (qcom->is_suspended)
 		return 0;
@@ -429,12 +557,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
 	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
 		dev_err(qcom->dev, "HS-PHY not in L2\n");
 
-	for (i = qcom->num_clocks - 1; i >= 0; i--)
-		clk_disable_unprepare(qcom->clks[i]);
-
-	ret = dwc3_qcom_interconnect_disable(qcom);
-	if (ret)
-		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
+	qcom->qcom_dwc3_device_ops.suspend_resource(qcom);
 
 	/*
 	 * The role is stable during suspend as role switching is done from a
@@ -453,7 +576,6 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
 static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
 {
 	int ret;
-	int i;
 
 	if (!qcom->is_suspended)
 		return 0;
@@ -461,18 +583,9 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
 	if (dwc3_qcom_is_host(qcom) && wakeup)
 		dwc3_qcom_disable_interrupts(qcom);
 
-	for (i = 0; i < qcom->num_clocks; i++) {
-		ret = clk_prepare_enable(qcom->clks[i]);
-		if (ret < 0) {
-			while (--i >= 0)
-				clk_disable_unprepare(qcom->clks[i]);
-			return ret;
-		}
-	}
-
-	ret = dwc3_qcom_interconnect_enable(qcom);
-	if (ret)
-		dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
+	ret = qcom->qcom_dwc3_device_ops.resume_resource(qcom);
+	if (ret < 0)
+		return ret;
 
 	/* Clear existing events from PHY related to L2 in/out */
 	dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
@@ -833,30 +946,54 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 		}
 	}
 
-	qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
-	if (IS_ERR(qcom->resets)) {
-		return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
-				     "failed to get resets\n");
-	}
+	qcom->fw_managed  = device_property_read_bool(dev, "qcom,fw-managed");
+	if (qcom->fw_managed) {
+		ret = dwc3_qcom_fw_managed_domain_init(qcom);
+		if (ret) {
+			dev_err(dev, "Failed to init domains. Bail out\n");
+			return ret;
+		}
+		ret = pm_runtime_resume_and_get(qcom->genpd_core);
+		if (ret < 0) {
+			dev_err(qcom->dev, "Failed to enable %s", __func__);
+			dwc3_qcom_fw_managed_domain_remove(qcom);
+			return ret;
+		}
 
-	ret = reset_control_assert(qcom->resets);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
-		return ret;
-	}
+		qcom->qcom_dwc3_device_ops.resume_resource = fw_managed_resume_resource;
+		qcom->qcom_dwc3_device_ops.exit_resource = fw_managed_exit_resource;
+		qcom->qcom_dwc3_device_ops.suspend_resource = fw_managed_suspend_resource;
+	} else {
+		qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
+		if (IS_ERR(qcom->resets)) {
+			return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
+					     "failed to get resets\n");
+		}
 
-	usleep_range(10, 1000);
+		ret = reset_control_assert(qcom->resets);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
+			return ret;
+		}
 
-	ret = reset_control_deassert(qcom->resets);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to deassert resets, err=%d\n", ret);
-		goto reset_assert;
-	}
+		usleep_range(10, 1000);
 
-	ret = dwc3_qcom_clk_init(qcom, of_clk_get_parent_count(np));
-	if (ret) {
-		dev_err_probe(dev, ret, "failed to get clocks\n");
-		goto reset_assert;
+		ret = reset_control_deassert(qcom->resets);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to deassert resets, err=%d\n", ret);
+			goto reset_assert;
+		}
+
+		ret = dwc3_qcom_clk_init(qcom, of_clk_get_parent_count(np));
+		if (ret) {
+			dev_err_probe(dev, ret, "failed to get clocks\n");
+			goto reset_assert;
+		}
+
+		//Map the functions here for suspend resume
+		qcom->qcom_dwc3_device_ops.resume_resource = locally_managed_resume_resource;
+		qcom->qcom_dwc3_device_ops.exit_resource = locally_managed_exit_resource;
+		qcom->qcom_dwc3_device_ops.suspend_resource = locally_managed_suspend_resource;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -916,9 +1053,11 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 		goto free_urs;
 	}
 
-	ret = dwc3_qcom_interconnect_init(qcom);
-	if (ret)
-		goto depopulate;
+	if (!qcom->fw_managed) {
+		ret = dwc3_qcom_interconnect_init(qcom);
+		if (ret)
+			goto depopulate;
+	}
 
 	qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
 
@@ -928,8 +1067,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 
 	/* register extcon to override sw_vbus on Vbus change later */
 	ret = dwc3_qcom_register_extcon(qcom);
-	if (ret)
-		goto interconnect_exit;
+	if (ret) {
+		if (!qcom->fw_managed)
+			goto interconnect_exit;
+		else
+			goto depopulate;
+	}
 
 	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
 	device_init_wakeup(&pdev->dev, wakeup_source);
@@ -956,13 +1099,19 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (qcom->urs_usb)
 		dwc3_qcom_destroy_urs_usb_platdev(qcom->urs_usb);
 clk_disable:
-	for (i = qcom->num_clocks - 1; i >= 0; i--) {
-		clk_disable_unprepare(qcom->clks[i]);
-		clk_put(qcom->clks[i]);
+	if (!qcom->fw_managed) {
+		for (i = qcom->num_clocks - 1; i >= 0; i--) {
+			clk_disable_unprepare(qcom->clks[i]);
+			clk_put(qcom->clks[i]);
+		}
 	}
 reset_assert:
-	reset_control_assert(qcom->resets);
-
+	if (qcom->fw_managed) {
+		pm_runtime_put_sync(qcom->genpd_core);
+		dwc3_qcom_fw_managed_domain_remove(qcom);
+	} else {
+		reset_control_assert(qcom->resets);
+	}
 	return ret;
 }
 
@@ -971,7 +1120,6 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
 	struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
 	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
-	int i;
 
 	if (np) {
 		of_platform_depopulate(&pdev->dev);
@@ -984,14 +1132,7 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
 	if (qcom->urs_usb)
 		dwc3_qcom_destroy_urs_usb_platdev(qcom->urs_usb);
 
-	for (i = qcom->num_clocks - 1; i >= 0; i--) {
-		clk_disable_unprepare(qcom->clks[i]);
-		clk_put(qcom->clks[i]);
-	}
-	qcom->num_clocks = 0;
-
-	dwc3_qcom_interconnect_exit(qcom);
-	reset_control_assert(qcom->resets);
+	qcom->qcom_dwc3_device_ops.exit_resource(qcom);
 
 	pm_runtime_allow(dev);
 	pm_runtime_disable(dev);
-- 
2.7.4

Re: [RFC 2/3] USB: dwc3: qcom: Add support for firmware managed resources

[Bjorn Andersson]

On Tue, Mar 05, 2024 at 10:27:37PM +0530, Sriram Dash wrote:
> Some target systems allow multiple resources to be managed by firmware.
> On these targets, tasks related to clocks, regulators, resets, and
> interconnects can be delegated to the firmware, while the remaining
> responsibilities are handled by Linux.
> 
> The driver is responsible for managing multiple power domains and
> linking them to consumers as needed. Incase there is only single
> power domain, it is considered to be a standard GDSC hooked on to
> the qcom dt node which is read and assigned to device structure
> (by genpd framework) before the driver probe even begins.
> 
> This differentiation logic allows the driver to determine whether
> device resources are managed by Linux or firmware, ensuring
> backward compatibility.
> 
> Furthermore, minor cleanup is performed for the private data of

No "futhermore"s please, separate matters should be proposed as separate
patches. Perhaps these can be sent separately and merged immediately?

> the SNPS Femto PHY. However, ACPI handling is omitted due to the
> absence of clients on the ACPI side.
> 
> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-usb.c       | 290 ++++++++++++++++++++------
>  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 213 +++++++++++++++----
>  drivers/usb/dwc3/dwc3-qcom.c                  | 259 +++++++++++++++++------

You're making independent changes across three different drivers across
two different subsystems, with different maintainers, this is not
acceptable as a single patch.

>  3 files changed, 594 insertions(+), 168 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
> index 8525393..1ac1b50 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
> @@ -21,6 +21,9 @@
>  
>  #include "phy-qcom-qmp-common.h"
>  
> +#include <linux/pm_opp.h>
> +#include <linux/pm_domain.h>

Why are these includes alone here? Integrate your changes with the
driver properly.

> +
>  #include "phy-qcom-qmp.h"
>  #include "phy-qcom-qmp-pcs-misc-v3.h"
>  #include "phy-qcom-qmp-pcs-misc-v4.h"
> @@ -1212,6 +1215,9 @@ struct qmp_phy_cfg {
>  	unsigned int pcs_usb_offset;
>  };
>  
> +#define DOMAIN_GENPD_TRANSFER			0
> +#define DOMAIN_GENPD_CORE			1

Does this really represent the hardware? What hardware constructs does
"transfer" and "core" maps to?

> +
>  struct qmp_usb {
>  	struct device *dev;
>  
> @@ -1236,6 +1242,19 @@ struct qmp_usb {
>  	struct phy *phy;
>  
>  	struct clk_fixed_rate pipe_clk_fixed;
> +
> +	struct dev_pm_domain_list *pd_list;
> +	struct device *genpd_core;
> +	struct device *genpd_transfer;
> +
> +	bool fw_managed;
> +	/* separate resource management for fw_managed vs locally managed devices */
> +	struct qmp_usb_device_ops {
> +		int (*bus_resume_resource)(struct qmp_usb *qmp);

Not only does these function pointers make the drivers much harder to
follow, your naming of these seems chosen to maximize the confusion.

In your managed case this doesn't seem to relate to any "bus", in the
"local" case, this doesn't relate to a "bus", and these callbacks are
decoupled from the actual runtime resume and suspend cycle of the QMP
device itself...

> +		int (*runtime_resume_resource)(struct qmp_usb *qmp);
> +		int (*bus_suspend_resource)(struct qmp_usb *qmp);
> +		int (*runtime_suspend_resource)(struct qmp_usb *qmp);
> +	} qmp_usb_device_ops;
>  };
>  
>  static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
> @@ -1598,6 +1617,41 @@ static const struct qmp_phy_cfg x1e80100_usb3_uniphy_cfg = {
>  	.regs			= qmp_v7_usb3phy_regs_layout,
>  };
>  
> +static void qmp_fw_managed_domain_remove(struct qmp_usb *qmp)
> +{
> +	dev_pm_domain_detach_list(qmp->pd_list);
> +}
> +
> +static int qmp_fw_managed_domain_init(struct qmp_usb *qmp)
> +{
> +	struct device *dev = qmp->dev;
> +	struct dev_pm_domain_attach_data pd_data = {
> +		.pd_flags	= PD_FLAG_NO_DEV_LINK,

Iiuc, you attach the two power-domains with NO_DEV_LINK, such that the
pm runtime state of the device itself won't reflect on the power
domains, and then you hand-code all the involved logic yourself?

Why can't you integrate with the device and use its runtime state?
Please clearly explain why you're doing it like this in your commit
messages.

Regards,
Bjorn

Re: [RFC 2/3] USB: dwc3: qcom: Add support for firmware managed resources

[Sriram Dash]

On 3/6/2024 12:52 AM, Bjorn Andersson wrote:
> On Tue, Mar 05, 2024 at 10:27:37PM +0530, Sriram Dash wrote:
>> Some target systems allow multiple resources to be managed by firmware.
>> On these targets, tasks related to clocks, regulators, resets, and
>> interconnects can be delegated to the firmware, while the remaining
>> responsibilities are handled by Linux.
>>
>> The driver is responsible for managing multiple power domains and
>> linking them to consumers as needed. Incase there is only single
>> power domain, it is considered to be a standard GDSC hooked on to
>> the qcom dt node which is read and assigned to device structure
>> (by genpd framework) before the driver probe even begins.
>>
>> This differentiation logic allows the driver to determine whether
>> device resources are managed by Linux or firmware, ensuring
>> backward compatibility.
>>
>> Furthermore, minor cleanup is performed for the private data of
> 
> No "futhermore"s please, separate matters should be proposed as separate
> patches. Perhaps these can be sent separately and merged immediately?
> 

Thanks Bjorn.
Will take this separately.

>> the SNPS Femto PHY. However, ACPI handling is omitted due to the
>> absence of clients on the ACPI side.
>>
>> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-usb.c       | 290 ++++++++++++++++++++------
>>   drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 213 +++++++++++++++----
>>   drivers/usb/dwc3/dwc3-qcom.c                  | 259 +++++++++++++++++------
> 
> You're making independent changes across three different drivers across
> two different subsystems, with different maintainers, this is not
> acceptable as a single patch.
> 

Sure. will split the patches in next version.

>>   3 files changed, 594 insertions(+), 168 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>> index 8525393..1ac1b50 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>> @@ -21,6 +21,9 @@
>>   
>>   #include "phy-qcom-qmp-common.h"
>>   
>> +#include <linux/pm_opp.h>
>> +#include <linux/pm_domain.h>
> 
> Why are these includes alone here? Integrate your changes with the
> driver properly.
> 

Sure. will take care in the next version.

>> +
>>   #include "phy-qcom-qmp.h"
>>   #include "phy-qcom-qmp-pcs-misc-v3.h"
>>   #include "phy-qcom-qmp-pcs-misc-v4.h"
>> @@ -1212,6 +1215,9 @@ struct qmp_phy_cfg {
>>   	unsigned int pcs_usb_offset;
>>   };
>>   
>> +#define DOMAIN_GENPD_TRANSFER			0
>> +#define DOMAIN_GENPD_CORE			1
> 
> Does this really represent the hardware? What hardware constructs does
> "transfer" and "core" maps to?
> 

The idea was to club the resources in 2 buckets.
Which are essential for the IP core to be active
(ex : regulators, gdsc ) form the part or genpd core
and the resources which are controlled from Clock cluster
in another bucket, used for transfers.


>> +
>>   struct qmp_usb {
>>   	struct device *dev;
>>   
>> @@ -1236,6 +1242,19 @@ struct qmp_usb {
>>   	struct phy *phy;
>>   
>>   	struct clk_fixed_rate pipe_clk_fixed;
>> +
>> +	struct dev_pm_domain_list *pd_list;
>> +	struct device *genpd_core;
>> +	struct device *genpd_transfer;
>> +
>> +	bool fw_managed;
>> +	/* separate resource management for fw_managed vs locally managed devices */
>> +	struct qmp_usb_device_ops {
>> +		int (*bus_resume_resource)(struct qmp_usb *qmp);
> 
> Not only does these function pointers make the drivers much harder to
> follow, your naming of these seems chosen to maximize the confusion.
> 
> In your managed case this doesn't seem to relate to any "bus", in the
> "local" case, this doesn't relate to a "bus", and these callbacks are
> decoupled from the actual runtime resume and suspend cycle of the QMP
> device itself...
> 

Understood. Will make the decision to use fw managed
method or local management of resources based on the
fw_managed property rather than fixing it to function
pointer.

>> +		int (*runtime_resume_resource)(struct qmp_usb *qmp);
>> +		int (*bus_suspend_resource)(struct qmp_usb *qmp);
>> +		int (*runtime_suspend_resource)(struct qmp_usb *qmp);
>> +	} qmp_usb_device_ops;
>>   };
>>   
>>   static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
>> @@ -1598,6 +1617,41 @@ static const struct qmp_phy_cfg x1e80100_usb3_uniphy_cfg = {
>>   	.regs			= qmp_v7_usb3phy_regs_layout,
>>   };
>>   
>> +static void qmp_fw_managed_domain_remove(struct qmp_usb *qmp)
>> +{
>> +	dev_pm_domain_detach_list(qmp->pd_list);
>> +}
>> +
>> +static int qmp_fw_managed_domain_init(struct qmp_usb *qmp)
>> +{
>> +	struct device *dev = qmp->dev;
>> +	struct dev_pm_domain_attach_data pd_data = {
>> +		.pd_flags	= PD_FLAG_NO_DEV_LINK,
> 
> Iiuc, you attach the two power-domains with NO_DEV_LINK, such that the
> pm runtime state of the device itself won't reflect on the power
> domains, and then you hand-code all the involved logic yourself?
> > Why can't you integrate with the device and use its runtime state?
> Please clearly explain why you're doing it like this in your commit
> messages.
> 

OK.
Got suggestion from Dmitry to either pass empty list or
dev_pm_domain_attach twice. I will use the later.

> Regards,
> Bjorn

Re: [RFC 2/3] USB: dwc3: qcom: Add support for firmware managed resources

[Dmitry Baryshkov]

On Tue, 5 Mar 2024 at 18:59, Sriram Dash <quic_sriramd@quicinc.com> wrote:
>
> Some target systems allow multiple resources to be managed by firmware.
> On these targets, tasks related to clocks, regulators, resets, and
> interconnects can be delegated to the firmware, while the remaining
> responsibilities are handled by Linux.
>
> The driver is responsible for managing multiple power domains and
> linking them to consumers as needed. Incase there is only single
> power domain, it is considered to be a standard GDSC hooked on to
> the qcom dt node which is read and assigned to device structure
> (by genpd framework) before the driver probe even begins.
>
> This differentiation logic allows the driver to determine whether
> device resources are managed by Linux or firmware, ensuring
> backward compatibility.
>
> Furthermore, minor cleanup is performed for the private data of
> the SNPS Femto PHY. However, ACPI handling is omitted due to the
> absence of clients on the ACPI side.
>
> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-usb.c       | 290 ++++++++++++++++++++------
>  drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 213 +++++++++++++++----
>  drivers/usb/dwc3/dwc3-qcom.c                  | 259 +++++++++++++++++------

Thank you for mixing three different drivers in a single patch. This
saves us from sending two more letters. In future please abstain from
doing that and group your changes logically and atomically. Although
QMP and SNPS PHYs are maintained within the same subsystem, changing
them are two separate patches, as the changes are similar but
unrelated.

>  3 files changed, 594 insertions(+), 168 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
> index 8525393..1ac1b50 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
> @@ -21,6 +21,9 @@
>
>  #include "phy-qcom-qmp-common.h"
>
> +#include <linux/pm_opp.h>
> +#include <linux/pm_domain.h>
> +
>  #include "phy-qcom-qmp.h"
>  #include "phy-qcom-qmp-pcs-misc-v3.h"
>  #include "phy-qcom-qmp-pcs-misc-v4.h"
> @@ -1212,6 +1215,9 @@ struct qmp_phy_cfg {
>         unsigned int pcs_usb_offset;
>  };
>
> +#define DOMAIN_GENPD_TRANSFER                  0
> +#define DOMAIN_GENPD_CORE                      1

Why do you need them defined on a global level if later you copy
necessary data away from the list?

> +
>  struct qmp_usb {
>         struct device *dev;
>
> @@ -1236,6 +1242,19 @@ struct qmp_usb {
>         struct phy *phy;
>
>         struct clk_fixed_rate pipe_clk_fixed;
> +
> +       struct dev_pm_domain_list *pd_list;
> +       struct device *genpd_core;
> +       struct device *genpd_transfer;
> +
> +       bool fw_managed;
> +       /* separate resource management for fw_managed vs locally managed devices */
> +       struct qmp_usb_device_ops {

Do you see nested structure definitions in this file? I don't think so.

> +               int (*bus_resume_resource)(struct qmp_usb *qmp);
> +               int (*runtime_resume_resource)(struct qmp_usb *qmp);
> +               int (*bus_suspend_resource)(struct qmp_usb *qmp);
> +               int (*runtime_suspend_resource)(struct qmp_usb *qmp);
> +       } qmp_usb_device_ops;

Just ops, pm_ops or bus_pm_ops.

>  };
>
>  static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
> @@ -1598,6 +1617,41 @@ static const struct qmp_phy_cfg x1e80100_usb3_uniphy_cfg = {
>         .regs                   = qmp_v7_usb3phy_regs_layout,
>  };
>
> +static void qmp_fw_managed_domain_remove(struct qmp_usb *qmp)
> +{
> +       dev_pm_domain_detach_list(qmp->pd_list);

This looks like a candidate for devm_add_action_or_reset

> +}
> +
> +static int qmp_fw_managed_domain_init(struct qmp_usb *qmp)
> +{
> +       struct device *dev = qmp->dev;
> +       struct dev_pm_domain_attach_data pd_data = {
> +               .pd_flags       = PD_FLAG_NO_DEV_LINK,
> +               .pd_names       = (const char*[]) { "usb_transfer", "usb_core" },
> +               .num_pd_names   = 2,
> +       };

Can you pass an empty list instead? Or instead call
dev_pm_domain_attach twice, if you are not using the pd_list later on?

> +       int ret = 0;
> +
> +       if (!dev->pm_domain) {
> +               ret = dev_pm_domain_attach_list(dev, &pd_data, &qmp->pd_list);
> +               if (ret < 0) {
> +                       dev_err(dev, "domain attach failed %d)\n", ret);
> +                       return ret;
> +               }
> +
> +               qmp->genpd_transfer = qmp->pd_list->pd_devs[DOMAIN_GENPD_TRANSFER];
> +               qmp->genpd_core = qmp->pd_list->pd_devs[DOMAIN_GENPD_CORE];
> +
> +
> +               dev_dbg(dev, "domains attached successfully\n");
> +       } else {
> +               dev_dbg(dev, "domain attach fail\n");
> +               return -ENODEV;
> +       }
> +
> +       return 0;
> +}
> +
>  static int qmp_usb_serdes_init(struct qmp_usb *qmp)
>  {
>         const struct qmp_phy_cfg *cfg = qmp->cfg;
> @@ -1610,11 +1664,20 @@ static int qmp_usb_serdes_init(struct qmp_usb *qmp)
>         return 0;
>  }
>
> -static int qmp_usb_init(struct phy *phy)
> +static int fw_managed_ssphy_bus_init(struct qmp_usb *qmp)

Please take a look around. All functions have a common naming
structure, which you are not following. Please do not do that _ever_.

> +{
> +       int ret = 0;
> +
> +       ret = pm_runtime_get_sync(qmp->genpd_core);
> +       if (ret < 0)
> +               dev_err(qmp->dev, "Failed to enable fw managed resources");
> +
> +       return ret;
> +}
> +
> +static int locally_managed_ssphy_bus_init(struct qmp_usb *qmp)
>  {
> -       struct qmp_usb *qmp = phy_get_drvdata(phy);
>         const struct qmp_phy_cfg *cfg = qmp->cfg;
> -       void __iomem *pcs = qmp->pcs;
>         int ret;
>
>         ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> @@ -1639,8 +1702,11 @@ static int qmp_usb_init(struct phy *phy)
>         if (ret)
>                 goto err_assert_reset;
>
> -       qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
> -
> +       ret = clk_prepare_enable(qmp->pipe_clk);
> +       if (ret) {
> +               dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> +               return ret;
> +       }

Here (and further in the file) you are changing the order of SW_PWRDN
bit toggling and pipe_clk enablement / disablement. Is this a safe
change?
Have you tested it? If so, on which platforms?

>         return 0;
>
>  err_assert_reset:
> @@ -1651,11 +1717,22 @@ static int qmp_usb_init(struct phy *phy)
>         return ret;
>  }
>
> -static int qmp_usb_exit(struct phy *phy)
> +static int fw_managed_ssphy_bus_exit(struct qmp_usb *qmp)
> +{
> +       int ret = 0;
> +
> +       ret = pm_runtime_put_sync(qmp->genpd_core);
> +       if (ret < 0)
> +               dev_err(qmp->dev, "Failed to disable fw managed resources");
> +
> +       return 0;
> +}
> +
> +static int locally_managed_ssphy_bus_exit(struct qmp_usb *qmp)
>  {
> -       struct qmp_usb *qmp = phy_get_drvdata(phy);
>         const struct qmp_phy_cfg *cfg = qmp->cfg;
>
> +       clk_disable_unprepare(qmp->pipe_clk);
>         reset_control_bulk_assert(qmp->num_resets, qmp->resets);
>
>         clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
> @@ -1677,13 +1754,9 @@ static int qmp_usb_power_on(struct phy *phy)
>         unsigned int val;
>         int ret;
>
> -       qmp_usb_serdes_init(qmp);
> +       qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
>
> -       ret = clk_prepare_enable(qmp->pipe_clk);
> -       if (ret) {
> -               dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> -               return ret;
> -       }
> +       qmp_usb_serdes_init(qmp);
>
>         /* Tx, Rx, and PCS configurations */
>         qmp_configure_lane(tx, cfg->tx_tbl, cfg->tx_tbl_num, 1);
> @@ -1708,15 +1781,10 @@ static int qmp_usb_power_on(struct phy *phy)
>                                  PHY_INIT_COMPLETE_TIMEOUT);
>         if (ret) {
>                 dev_err(qmp->dev, "phy initialization timed-out\n");
> -               goto err_disable_pipe_clk;
> +               return ret;
>         }
>
>         return 0;
> -
> -err_disable_pipe_clk:
> -       clk_disable_unprepare(qmp->pipe_clk);
> -
> -       return ret;
>  }
>
>  static int qmp_usb_power_off(struct phy *phy)
> @@ -1724,8 +1792,6 @@ static int qmp_usb_power_off(struct phy *phy)
>         struct qmp_usb *qmp = phy_get_drvdata(phy);
>         const struct qmp_phy_cfg *cfg = qmp->cfg;
>
> -       clk_disable_unprepare(qmp->pipe_clk);
> -
>         /* PHY reset */
>         qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
>
> @@ -1742,27 +1808,30 @@ static int qmp_usb_power_off(struct phy *phy)
>
>  static int qmp_usb_enable(struct phy *phy)
>  {
> +       struct qmp_usb *qmp = phy_get_drvdata(phy);
>         int ret;
>
> -       ret = qmp_usb_init(phy);
> +       ret = qmp->qmp_usb_device_ops.bus_resume_resource(qmp);
>         if (ret)
>                 return ret;
>
>         ret = qmp_usb_power_on(phy);
>         if (ret)
> -               qmp_usb_exit(phy);
> +               qmp->qmp_usb_device_ops.bus_suspend_resource(qmp);
>
>         return ret;
>  }
>
>  static int qmp_usb_disable(struct phy *phy)
>  {
> +       struct qmp_usb *qmp = phy_get_drvdata(phy);
>         int ret;
>
>         ret = qmp_usb_power_off(phy);
>         if (ret)
>                 return ret;
> -       return qmp_usb_exit(phy);
> +
> +       return qmp->qmp_usb_device_ops.bus_suspend_resource(qmp);
>  }
>
>  static int qmp_usb_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> @@ -1828,6 +1897,25 @@ static void qmp_usb_disable_autonomous_mode(struct qmp_usb *qmp)
>         qphy_clrbits(pcs_usb, cfg->regs[QPHY_PCS_LFPS_RXTERM_IRQ_CLEAR], IRQ_CLEAR);
>  }
>
> +static int locally_managed_ssphy_runtime_exit(struct qmp_usb *qmp)
> +{
> +       clk_disable_unprepare(qmp->pipe_clk);
> +       clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
> +
> +       return 0;
> +}
> +
> +static int fw_managed_ssphy_runtime_exit(struct qmp_usb *qmp)
> +{
> +       int ret = 0;
> +
> +       ret = pm_runtime_put_sync(qmp->genpd_transfer);
> +       if (ret < 0)
> +               dev_err(qmp->dev, "Failed to disable fw managed resources");
> +
> +       return 0;
> +}
> +
>  static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev)
>  {
>         struct qmp_usb *qmp = dev_get_drvdata(dev);
> @@ -1841,16 +1929,44 @@ static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev)
>
>         qmp_usb_enable_autonomous_mode(qmp);
>
> -       clk_disable_unprepare(qmp->pipe_clk);
> -       clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
> +       qmp->qmp_usb_device_ops.runtime_suspend_resource(qmp);
>
>         return 0;
>  }
>
> +static int locally_managed_ssphy_runtime_init(struct qmp_usb *qmp)
> +{
> +       int ret = 0;
> +
> +       ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
> +       if (ret)
> +               return ret;
> +
> +       ret = clk_prepare_enable(qmp->pipe_clk);
> +       if (ret) {
> +               dev_err(qmp->dev, "pipe_clk enable failed, err=%d\n", ret);
> +               clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int fw_managed_ssphy_runtime_init(struct qmp_usb *qmp)
> +{
> +       int ret = 0;
> +
> +       ret = pm_runtime_get_sync(qmp->genpd_transfer);
> +       if (ret < 0)
> +               dev_err(qmp->dev, "Failed to enable fw managed resources");
> +
> +       return ret;
> +}
> +
>  static int __maybe_unused qmp_usb_runtime_resume(struct device *dev)
>  {
> +       int ret;
>         struct qmp_usb *qmp = dev_get_drvdata(dev);
> -       int ret = 0;
>
>         dev_vdbg(dev, "Resuming QMP phy, mode:%d\n", qmp->mode);
>
> @@ -1859,17 +1975,10 @@ static int __maybe_unused qmp_usb_runtime_resume(struct device *dev)
>                 return 0;
>         }
>
> -       ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
> +       ret = qmp->qmp_usb_device_ops.runtime_resume_resource(qmp);
>         if (ret)
>                 return ret;
>
> -       ret = clk_prepare_enable(qmp->pipe_clk);
> -       if (ret) {
> -               dev_err(dev, "pipe_clk enable failed, err=%d\n", ret);
> -               clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
> -               return ret;
> -       }
> -
>         qmp_usb_disable_autonomous_mode(qmp);
>
>         return 0;
> @@ -2059,22 +2168,24 @@ static int qmp_usb_parse_dt_legacy(struct qmp_usb *qmp, struct device_node *np)
>                 qmp->pcs_misc = NULL;
>         }
>
> -       qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL);
> -       if (IS_ERR(qmp->pipe_clk)) {
> -               return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
> -                                    "failed to get pipe clock\n");
> -       }
> +       if (!qmp->fw_managed) {
> +               qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL);
> +               if (IS_ERR(qmp->pipe_clk)) {
> +                       return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
> +                                            "failed to get pipe clock\n");
> +               }
>
> -       ret = devm_clk_bulk_get_all(qmp->dev, &qmp->clks);
> -       if (ret < 0)
> -               return ret;
> +               ret = devm_clk_bulk_get_all(qmp->dev, &qmp->clks);
> +               if (ret < 0)
> +                       return ret;
>
> -       qmp->num_clks = ret;
> +               qmp->num_clks = ret;
>
> -       ret = qmp_usb_reset_init(qmp, usb3phy_legacy_reset_l,
> -                                ARRAY_SIZE(usb3phy_legacy_reset_l));
> -       if (ret)
> -               return ret;
> +               ret = qmp_usb_reset_init(qmp, usb3phy_legacy_reset_l,
> +                                        ARRAY_SIZE(usb3phy_legacy_reset_l));
> +               if (ret)
> +                       return ret;
> +       }
>
>         return 0;
>  }
> @@ -2104,21 +2215,23 @@ static int qmp_usb_parse_dt(struct qmp_usb *qmp)
>         qmp->tx = base + offs->tx;
>         qmp->rx = base + offs->rx;
>
> -       ret = qmp_usb_clk_init(qmp);
> -       if (ret)
> -               return ret;
> -
> -       qmp->pipe_clk = devm_clk_get(dev, "pipe");
> -       if (IS_ERR(qmp->pipe_clk)) {
> -               return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
> -                                    "failed to get pipe clock\n");
> +       if (!qmp->fw_managed) {
> +               ret = qmp_usb_clk_init(qmp);
> +               if (ret)
> +                       return ret;
> +
> +               qmp->pipe_clk = devm_clk_get(dev, "pipe");
> +               if (IS_ERR(qmp->pipe_clk)) {
> +                       return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
> +                                            "failed to get pipe clock\n");
> +               }
> +
> +               ret = qmp_usb_reset_init(qmp, usb3phy_reset_l,
> +                                        ARRAY_SIZE(usb3phy_reset_l));
> +               if (ret)
> +                       return ret;
>         }
>
> -       ret = qmp_usb_reset_init(qmp, usb3phy_reset_l,
> -                                ARRAY_SIZE(usb3phy_reset_l));
> -       if (ret)
> -               return ret;
> -
>         return 0;
>  }
>
> @@ -2140,9 +2253,36 @@ static int qmp_usb_probe(struct platform_device *pdev)
>         if (!qmp->cfg)
>                 return -EINVAL;
>
> -       ret = qmp_usb_vreg_init(qmp);
> -       if (ret)
> -               return ret;
> +       qmp->fw_managed  = device_property_read_bool(dev, "qmp,fw-managed");
> +       if (qmp->fw_managed) {
> +               ret = qmp_fw_managed_domain_init(qmp);
> +               if (ret) {
> +                       dev_err(dev, "Failed to init domains. Bail out");
> +                       return ret;
> +               }
> +
> +               qmp->qmp_usb_device_ops.bus_resume_resource =
> +                                               fw_managed_ssphy_bus_init;
> +               qmp->qmp_usb_device_ops.runtime_resume_resource =
> +                                               fw_managed_ssphy_runtime_init;
> +               qmp->qmp_usb_device_ops.bus_suspend_resource =
> +                                               fw_managed_ssphy_bus_exit;
> +               qmp->qmp_usb_device_ops.runtime_suspend_resource =
> +                                               fw_managed_ssphy_runtime_exit;

Use a pointer to ops and constant data instead of setting function
pointers manually.

> +       } else {
> +               ret = qmp_usb_vreg_init(qmp);
> +               if (ret)
> +                       return ret;
> +
> +               qmp->qmp_usb_device_ops.bus_resume_resource =
> +                                               locally_managed_ssphy_bus_init;
> +               qmp->qmp_usb_device_ops.runtime_resume_resource =
> +                                               locally_managed_ssphy_runtime_init;
> +               qmp->qmp_usb_device_ops.bus_suspend_resource =
> +                                               locally_managed_ssphy_bus_exit;
> +               qmp->qmp_usb_device_ops.runtime_suspend_resource =
> +                                               locally_managed_ssphy_runtime_exit;
> +       }
>
>         /* Check for legacy binding with child node. */
>         np = of_get_next_available_child(dev->of_node, NULL);
> @@ -2165,9 +2305,11 @@ static int qmp_usb_probe(struct platform_device *pdev)
>          */
>         pm_runtime_forbid(dev);
>
> -       ret = phy_pipe_clk_register(qmp, np);
> -       if (ret)
> -               goto err_node_put;
> +       if (!qmp->fw_managed) {
> +               ret = phy_pipe_clk_register(qmp, np);
> +               if (ret)
> +                       goto err_node_put;

pipe_clk is an input to the GCC. If you are not registering it here,
how would GCC get it?

> +       }
>
>         qmp->phy = devm_phy_create(dev, np, &qmp_usb_phy_ops);
>         if (IS_ERR(qmp->phy)) {
> @@ -2186,9 +2328,22 @@ static int qmp_usb_probe(struct platform_device *pdev)
>
>  err_node_put:
>         of_node_put(np);
> +       if (qmp->fw_managed)
> +               qmp_fw_managed_domain_remove(qmp);
>         return ret;
>  }
>
> +static void qmp_usb_remove(struct platform_device *pdev)
> +{
> +       struct qmp_usb *qmp = platform_get_drvdata(pdev);
> +
> +       if (qmp->fw_managed) {
> +               pm_runtime_put_sync(qmp->genpd_core);
> +               qmp_fw_managed_domain_remove(qmp);
> +       }
> +}
> +
> +
>  static const struct of_device_id qmp_usb_of_match_table[] = {
>         {
>                 .compatible = "qcom,ipq6018-qmp-usb3-phy",
> @@ -2239,6 +2394,7 @@ MODULE_DEVICE_TABLE(of, qmp_usb_of_match_table);
>
>  static struct platform_driver qmp_usb_driver = {
>         .probe          = qmp_usb_probe,
> +       .remove_new     = qmp_usb_remove,
>         .driver = {
>                 .name   = "qcom-qmp-usb-phy",
>                 .pm     = &qmp_usb_pm_ops,
> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> index eb0b0f6..9a1377f 100644
> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
> @@ -17,6 +17,9 @@
>  #include <linux/reset.h>
>  #include <linux/slab.h>
>
> +#include <linux/pm_opp.h>
> +#include <linux/pm_domain.h>
> +
>  #define USB2_PHY_USB_PHY_UTMI_CTRL0            (0x3c)
>  #define SLEEPM                                 BIT(0)
>  #define OPMODE_MASK                            GENMASK(4, 3)
> @@ -89,7 +92,7 @@ struct override_param {
>         u8      reg_val;
>  };
>
> -struct override_param_map {
> +struct snps_femto_v2_priv_data {
>         const char *prop_name;
>         const struct override_param *param_table;
>         u8 table_size;
> @@ -106,6 +109,9 @@ struct phy_override_seq {
>
>  #define NUM_HSPHY_TUNING_PARAMS        (9)
>
> +#define DOMAIN_GENPD_TRANSFER                  0
> +#define DOMAIN_GENPD_CORE                      1

You guessed it, see my comments for the QMP PHY changes.

> +
>  /**
>   * struct qcom_snps_hsphy - snps hs phy attributes
>   *
> @@ -136,8 +142,54 @@ struct qcom_snps_hsphy {
>         bool phy_initialized;
>         enum phy_mode mode;
>         struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS];
> +
> +       struct dev_pm_domain_list *pd_list;
> +       struct device *genpd_core;
> +       struct device *genpd_transfer;
> +
> +       bool fw_managed;
> +       /* separate resource management for fw_managed vs locally managed devices */
> +       struct snps_femto_v2_device_ops {
> +               int (*resume_resource)(struct qcom_snps_hsphy *hsphy);
> +               int (*suspend_resource)(struct qcom_snps_hsphy *hsphy);
> +       } snps_femto_v2_device_ops;
> +
>  };
>
> +static void hsphy_fw_managed_domain_remove(struct qcom_snps_hsphy *hsphy)

qcom_snps_hsphy_ prefix.

> +{
> +       dev_pm_domain_detach_list(hsphy->pd_list);
> +}
> +
> +static int hsphy_fw_managed_domain_init(struct qcom_snps_hsphy *hsphy)
> +{
> +       struct device *dev = hsphy->dev;
> +       struct dev_pm_domain_attach_data pd_data = {
> +               .pd_flags       = PD_FLAG_NO_DEV_LINK,
> +               .pd_names       = (const char*[]) { "usb_transfer", "usb_core" },
> +               .num_pd_names   = 2,
> +       };
> +       int ret = 0;
> +
> +       if (!dev->pm_domain) {
> +               ret = dev_pm_domain_attach_list(dev, &pd_data, &hsphy->pd_list);
> +               if (ret < 0) {
> +                       dev_err(dev, "domain attach failed %d)\n", ret);
> +                       return ret;
> +               }
> +
> +               hsphy->genpd_transfer = hsphy->pd_list->pd_devs[DOMAIN_GENPD_TRANSFER];
> +               hsphy->genpd_core = hsphy->pd_list->pd_devs[DOMAIN_GENPD_CORE];
> +
> +               dev_dbg(dev, "domains attached successfully\n");
> +       } else {
> +               dev_dbg(dev, "domain attach fail\n");
> +               return -ENODEV;
> +       }
> +
> +       return 0;
> +}
> +
>  static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy)
>  {
>         struct device *dev = hsphy->dev;
> @@ -316,27 +368,27 @@ static const struct override_param ls_fs_output_impedance_sc7280[] = {
>         { 1310, 0 },
>  };
>
> -static const struct override_param_map sc7280_snps_7nm_phy[] = {
> +static const struct snps_femto_v2_priv_data sc7280_snps_7nm_phy[] = {

What does this have in common with the clocks / regulators being
fw-managed or not?

>         {
>                 "qcom,hs-disconnect-bp",
>                 hs_disconnect_sc7280,
>                 ARRAY_SIZE(hs_disconnect_sc7280),
>                 USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
> -               HS_DISCONNECT_MASK
> +               HS_DISCONNECT_MASK,

Why? Does it require to be changed?

>         },
>         {
>                 "qcom,squelch-detector-bp",
>                 squelch_det_threshold_sc7280,
>                 ARRAY_SIZE(squelch_det_threshold_sc7280),
>                 USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
> -               SQUELCH_DETECTOR_MASK
> +               SQUELCH_DETECTOR_MASK,
>         },
>         {
>                 "qcom,hs-amplitude-bp",
>                 hs_amplitude_sc7280,
>                 ARRAY_SIZE(hs_amplitude_sc7280),
>                 USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
> -               HS_AMPLITUDE_MASK
> +               HS_AMPLITUDE_MASK,
>         },
>         {
>                 "qcom,pre-emphasis-duration-bp",
> @@ -357,14 +409,14 @@ static const struct override_param_map sc7280_snps_7nm_phy[] = {
>                 hs_rise_fall_time_sc7280,
>                 ARRAY_SIZE(hs_rise_fall_time_sc7280),
>                 USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
> -               HS_RISE_FALL_MASK
> +               HS_RISE_FALL_MASK,
>         },
>         {
>                 "qcom,hs-crossover-voltage-microvolt",
>                 hs_crossover_voltage_sc7280,
>                 ARRAY_SIZE(hs_crossover_voltage_sc7280),
>                 USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
> -               HS_CROSSOVER_VOLTAGE_MASK
> +               HS_CROSSOVER_VOLTAGE_MASK,
>         },
>         {
>                 "qcom,hs-output-impedance-micro-ohms",
> @@ -383,12 +435,31 @@ static const struct override_param_map sc7280_snps_7nm_phy[] = {
>         {},
>  };
>
> -static int qcom_snps_hsphy_init(struct phy *phy)
> +static int fw_managed_hsphy_init(struct qcom_snps_hsphy *hsphy)
>  {
> -       struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
> -       int ret, i;
> +       int ret = 0;

Why do you need a value here if later on you set it again?

>
> -       dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
> +       ret = pm_runtime_get_sync(hsphy->genpd_core);
> +       if (ret < 0)
> +               dev_err(hsphy->dev, "Failed to enable fw managed resources");
> +
> +       return ret;
> +}
> +
> +static int fw_managed_hsphy_exit(struct qcom_snps_hsphy *hsphy)
> +{
> +       int ret = 0;
> +
> +       ret = pm_runtime_put_sync(hsphy->genpd_core);
> +       if (ret < 0)
> +               dev_err(hsphy->dev, "Failed to disable fw managed resources");
> +
> +       return 0;
> +}
> +
> +static int locally_managed_hsphy_init(struct qcom_snps_hsphy *hsphy)
> +{
> +       int ret;
>
>         ret = regulator_bulk_enable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
>         if (ret)
> @@ -396,13 +467,13 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>
>         ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks);
>         if (ret) {
> -               dev_err(&phy->dev, "failed to enable clocks, %d\n", ret);
> +               dev_err(hsphy->dev, "failed to enable clocks, %d\n", ret);
>                 goto poweroff_phy;
>         }
>
>         ret = reset_control_assert(hsphy->phy_reset);
>         if (ret) {
> -               dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret);
> +               dev_err(hsphy->dev, "failed to assert phy_reset, %d\n", ret);
>                 goto disable_clks;
>         }
>
> @@ -410,10 +481,42 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>
>         ret = reset_control_deassert(hsphy->phy_reset);
>         if (ret) {
> -               dev_err(&phy->dev, "failed to de-assert phy_reset, %d\n", ret);
> +               dev_err(hsphy->dev, "failed to de-assert phy_reset, %d\n", ret);
>                 goto disable_clks;
>         }
>
> +       return 0;
> +
> +disable_clks:
> +       clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
> +poweroff_phy:
> +       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
> +
> +       return ret;
> +}
> +
> +static int locally_managed_hsphy_exit(struct qcom_snps_hsphy *hsphy)
> +{
> +       reset_control_assert(hsphy->phy_reset);
> +       clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
> +       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
> +
> +       return 0;
> +}
> +
> +static int qcom_snps_hsphy_init(struct phy *phy)
> +{
> +       struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
> +       int ret, i;
> +
> +       dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
> +
> +       ret = hsphy->snps_femto_v2_device_ops.resume_resource(hsphy);
> +       if (ret) {
> +               dev_err(&phy->dev, "Error resumeing hsphy resources %d\n", ret);
> +               return ret;
> +       }
> +
>         qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_CFG0,
>                                         UTMI_PHY_CMN_CTRL_OVERRIDE_EN,
>                                         UTMI_PHY_CMN_CTRL_OVERRIDE_EN);
> @@ -467,22 +570,19 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>         hsphy->phy_initialized = true;
>
>         return 0;
> -
> -disable_clks:
> -       clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
> -poweroff_phy:
> -       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
> -
> -       return ret;
>  }
>
>  static int qcom_snps_hsphy_exit(struct phy *phy)
>  {
>         struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
> +       int ret = 0;
> +
> +       ret = hsphy->snps_femto_v2_device_ops.suspend_resource(hsphy);
> +       if (ret) {
> +               dev_err(&phy->dev, "Error suspending hsphy resources %d\n", ret);
> +               return ret;
> +       }
>
> -       reset_control_assert(hsphy->phy_reset);
> -       clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
> -       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
>         hsphy->phy_initialized = false;
>
>         return 0;
> @@ -497,7 +597,8 @@ static const struct phy_ops qcom_snps_hsphy_gen_ops = {
>
>  static const struct of_device_id qcom_snps_hsphy_of_match_table[] = {
>         { .compatible   = "qcom,sm8150-usb-hs-phy", },
> -       { .compatible   = "qcom,usb-snps-hs-5nm-phy", },
> +       { .compatible   = "qcom,sa8775p-usb-hs-phy", },
> +       { .compatible   = "qcom,sc8280xp-usb-hs-phy", },

WTF? Please keep the fallback entry in place. It looks to me that
somebody didn't read documentation on the OF device matching. Please
read the code first.

>         {
>                 .compatible     = "qcom,usb-snps-hs-7nm-phy",
>                 .data           = &sc7280_snps_7nm_phy,
> @@ -513,7 +614,7 @@ static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = {
>  };
>
>  static void qcom_snps_hsphy_override_param_update_val(
> -                       const struct override_param_map map,
> +                       const struct snps_femto_v2_priv_data map,
>                         s32 dt_val, struct phy_override_seq *seq_entry)
>  {
>         int i;
> @@ -541,7 +642,7 @@ static void qcom_snps_hsphy_read_override_param_seq(struct device *dev)
>         s32 val;
>         int ret, i;
>         struct qcom_snps_hsphy *hsphy;
> -       const struct override_param_map *cfg = of_device_get_match_data(dev);
> +       const struct snps_femto_v2_priv_data *cfg = of_device_get_match_data(dev);
>
>         if (!cfg)
>                 return;
> @@ -580,24 +681,39 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>         if (IS_ERR(hsphy->base))
>                 return PTR_ERR(hsphy->base);
>
> -       ret = qcom_snps_hsphy_clk_init(hsphy);
> -       if (ret)
> -               return dev_err_probe(dev, ret, "failed to initialize clocks\n");
>
> -       hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> -       if (IS_ERR(hsphy->phy_reset)) {
> -               dev_err(dev, "failed to get phy core reset\n");
> -               return PTR_ERR(hsphy->phy_reset);
> -       }
> +       hsphy->fw_managed  = device_property_read_bool(dev, "hsphy,fw-managed");
> +       if (hsphy->fw_managed) {
> +               ret = hsphy_fw_managed_domain_init(hsphy);
> +               if (ret) {
> +                       dev_err(dev, "Failed to init domains. Bail out");
> +                       return ret;
> +               }
>
> -       num = ARRAY_SIZE(hsphy->vregs);
> -       for (i = 0; i < num; i++)
> -               hsphy->vregs[i].supply = qcom_snps_hsphy_vreg_names[i];
> +               hsphy->snps_femto_v2_device_ops.resume_resource = fw_managed_hsphy_init;
> +               hsphy->snps_femto_v2_device_ops.suspend_resource = fw_managed_hsphy_exit;
> +       } else {
> +               ret = qcom_snps_hsphy_clk_init(hsphy);
> +               if (ret)
> +                       return dev_err_probe(dev, ret, "failed to initialize clocks\n");
>
> -       ret = devm_regulator_bulk_get(dev, num, hsphy->vregs);
> -       if (ret)
> -               return dev_err_probe(dev, ret,
> -                                    "failed to get regulator supplies\n");
> +               hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +               if (IS_ERR(hsphy->phy_reset)) {
> +                       dev_err(dev, "failed to get phy core reset\n");
> +                       return PTR_ERR(hsphy->phy_reset);
> +               }
> +
> +               num = ARRAY_SIZE(hsphy->vregs);
> +               for (i = 0; i < num; i++)
> +                       hsphy->vregs[i].supply = qcom_snps_hsphy_vreg_names[i];
> +
> +               ret = devm_regulator_bulk_get(dev, num, hsphy->vregs);
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                            "failed to get regulator supplies\n");
> +               hsphy->snps_femto_v2_device_ops.resume_resource = locally_managed_hsphy_init;
> +               hsphy->snps_femto_v2_device_ops.suspend_resource = locally_managed_hsphy_exit;
> +       }
>
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
> @@ -609,6 +725,8 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>
>         generic_phy = devm_phy_create(dev, NULL, &qcom_snps_hsphy_gen_ops);
>         if (IS_ERR(generic_phy)) {
> +               if (hsphy->fw_managed)
> +                       hsphy_fw_managed_domain_remove(hsphy);
>                 ret = PTR_ERR(generic_phy);
>                 dev_err(dev, "failed to create phy, %d\n", ret);
>                 return ret;
> @@ -628,8 +746,19 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>         return PTR_ERR_OR_ZERO(phy_provider);
>  }
>
> +static void qcom_snps_hsphy_remove(struct platform_device *pdev)
> +{
> +       struct qcom_snps_hsphy *hsphy = platform_get_drvdata(pdev);
> +
> +       if (hsphy->fw_managed) {
> +               pm_runtime_put_sync(hsphy->genpd_core);
> +               hsphy_fw_managed_domain_remove(hsphy);
> +       }
> +}
> +
>  static struct platform_driver qcom_snps_hsphy_driver = {
>         .probe          = qcom_snps_hsphy_probe,
> +       .remove_new     = qcom_snps_hsphy_remove,
>         .driver = {
>                 .name   = "qcom-snps-hs-femto-v2-phy",
>                 .pm = &qcom_snps_hsphy_pm_ops,
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index dbd6a5b..aea11d0 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -22,6 +22,8 @@
>  #include <linux/iopoll.h>
>  #include <linux/usb/hcd.h>
>  #include <linux/usb.h>
> +#include <linux/pm_opp.h>
> +#include <linux/pm_domain.h>
>  #include "core.h"
>
>  /* USB QSCRATCH Hardware registers */
> @@ -53,6 +55,9 @@
>  #define APPS_USB_AVG_BW 0
>  #define APPS_USB_PEAK_BW MBps_to_icc(40)
>
> +#define DOMAIN_GENPD_TRANSFER                  0
> +#define DOMAIN_GENPD_CORE                      1

Same comments as before

> +
>  struct dwc3_acpi_pdata {
>         u32                     qscratch_base_offset;
>         u32                     qscratch_base_size;
> @@ -91,8 +96,54 @@ struct dwc3_qcom {
>         bool                    pm_suspended;
>         struct icc_path         *icc_path_ddr;
>         struct icc_path         *icc_path_apps;
> +
> +       struct dev_pm_domain_list *pd_list;
> +       struct device *genpd_core;
> +       struct device *genpd_transfer;
> +
> +       bool                    fw_managed;
> +       /* separate resource management for fw_managed vs locally managed devices */
> +       struct qcom_dwc3_device_ops {
> +               int (*resume_resource)(struct dwc3_qcom *qcom);
> +               void (*suspend_resource)(struct dwc3_qcom *qcom);
> +               void (*exit_resource)(struct dwc3_qcom *qcom);
> +       } qcom_dwc3_device_ops;
>  };
>
> +static void dwc3_qcom_fw_managed_domain_remove(struct dwc3_qcom *qcom)
> +{
> +       dev_pm_domain_detach_list(qcom->pd_list);
> +}
> +
> +static int dwc3_qcom_fw_managed_domain_init(struct dwc3_qcom *qcom)
> +{
> +       struct device *dev = qcom->dev;
> +       struct dev_pm_domain_attach_data pd_data = {
> +               .pd_flags       = PD_FLAG_NO_DEV_LINK,
> +               .pd_names       = (const char*[]) { "usb_transfer", "usb_core" },
> +               .num_pd_names   = 2,
> +       };
> +       int ret = 0;
> +
> +       if (!dev->pm_domain) {
> +               ret = dev_pm_domain_attach_list(dev, &pd_data, &qcom->pd_list);
> +               if (ret < 0) {
> +                       dev_err(dev, "domain attach failed %d)\n", ret);
> +                       return ret;
> +               }
> +
> +               qcom->genpd_transfer = qcom->pd_list->pd_devs[DOMAIN_GENPD_TRANSFER];
> +               qcom->genpd_core = qcom->pd_list->pd_devs[DOMAIN_GENPD_CORE];
> +
> +               dev_dbg(dev, "domains attached successfully\n");
> +       } else {
> +               dev_dbg(dev, "domain attach fail\n");
> +               return -ENODEV;
> +       }
> +
> +       return 0;
> +}
> +
>  static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>  {
>         u32 reg;
> @@ -417,10 +468,87 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
>         dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0);
>  }
>
> +static void locally_managed_exit_resource(struct dwc3_qcom *qcom)
> +{
> +       int i;
> +
> +       for (i = qcom->num_clocks - 1; i >= 0; i--) {
> +               clk_disable_unprepare(qcom->clks[i]);
> +               clk_put(qcom->clks[i]);
> +       }
> +       qcom->num_clocks = 0;
> +
> +       dwc3_qcom_interconnect_exit(qcom);
> +       reset_control_assert(qcom->resets);
> +}
> +
> +static void fw_managed_exit_resource(struct dwc3_qcom *qcom)
> +{
> +       int ret = 0;
> +
> +       ret = pm_runtime_put_sync(qcom->genpd_core);
> +
> +       if (ret < 0)
> +               dev_err(qcom->dev, "Failed to disable fw managed resources");
> +
> +       dwc3_qcom_fw_managed_domain_remove(qcom);
> +}
> +
> +static int locally_managed_resume_resource(struct dwc3_qcom *qcom)
> +{
> +       int ret, i;
> +
> +       for (i = 0; i < qcom->num_clocks; i++) {
> +               ret = clk_prepare_enable(qcom->clks[i]);
> +               if (ret < 0) {
> +                       while (--i >= 0)
> +                               clk_disable_unprepare(qcom->clks[i]);
> +                       return ret;
> +               }
> +       }
> +
> +       ret = dwc3_qcom_interconnect_enable(qcom);
> +       if (ret)
> +               dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
> +
> +       return 0;
> +}
> +
> +static int fw_managed_resume_resource(struct dwc3_qcom *qcom)
> +{
> +       int ret = 0;
> +
> +       ret = pm_runtime_get_sync(qcom->genpd_transfer);
> +       if (ret < 0)
> +               dev_err(qcom->dev, "Failed to enable fw managed resources");
> +
> +       return ret;
> +}
> +
> +static void locally_managed_suspend_resource(struct dwc3_qcom *qcom)
> +{
> +       int i, ret;
> +
> +       for (i = qcom->num_clocks - 1; i >= 0; i--)
> +               clk_disable_unprepare(qcom->clks[i]);
> +
> +       ret = dwc3_qcom_interconnect_disable(qcom);
> +       if (ret)
> +               dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
> +}
> +
> +static void fw_managed_suspend_resource(struct dwc3_qcom *qcom)
> +{
> +       int ret = 0;
> +
> +       ret = pm_runtime_put_sync(qcom->genpd_transfer);
> +       if (ret < 0)
> +               dev_err(qcom->dev, "Failed to disable fw managed resources");
> +}
> +
>  static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>  {
>         u32 val;
> -       int i, ret;
>
>         if (qcom->is_suspended)
>                 return 0;
> @@ -429,12 +557,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>         if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>                 dev_err(qcom->dev, "HS-PHY not in L2\n");
>
> -       for (i = qcom->num_clocks - 1; i >= 0; i--)
> -               clk_disable_unprepare(qcom->clks[i]);
> -
> -       ret = dwc3_qcom_interconnect_disable(qcom);
> -       if (ret)
> -               dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
> +       qcom->qcom_dwc3_device_ops.suspend_resource(qcom);

Does SCMI manage interconnects too?

>
>         /*
>          * The role is stable during suspend as role switching is done from a
> @@ -453,7 +576,6 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>  static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>  {
>         int ret;
> -       int i;
>
>         if (!qcom->is_suspended)
>                 return 0;
> @@ -461,18 +583,9 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>         if (dwc3_qcom_is_host(qcom) && wakeup)
>                 dwc3_qcom_disable_interrupts(qcom);
>
> -       for (i = 0; i < qcom->num_clocks; i++) {
> -               ret = clk_prepare_enable(qcom->clks[i]);
> -               if (ret < 0) {
> -                       while (--i >= 0)
> -                               clk_disable_unprepare(qcom->clks[i]);
> -                       return ret;
> -               }
> -       }
> -
> -       ret = dwc3_qcom_interconnect_enable(qcom);
> -       if (ret)
> -               dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
> +       ret = qcom->qcom_dwc3_device_ops.resume_resource(qcom);
> +       if (ret < 0)
> +               return ret;
>
>         /* Clear existing events from PHY related to L2 in/out */
>         dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
> @@ -833,30 +946,54 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>                 }
>         }
>
> -       qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
> -       if (IS_ERR(qcom->resets)) {
> -               return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
> -                                    "failed to get resets\n");
> -       }
> +       qcom->fw_managed  = device_property_read_bool(dev, "qcom,fw-managed");
> +       if (qcom->fw_managed) {
> +               ret = dwc3_qcom_fw_managed_domain_init(qcom);
> +               if (ret) {
> +                       dev_err(dev, "Failed to init domains. Bail out\n");
> +                       return ret;
> +               }
> +               ret = pm_runtime_resume_and_get(qcom->genpd_core);
> +               if (ret < 0) {
> +                       dev_err(qcom->dev, "Failed to enable %s", __func__);
> +                       dwc3_qcom_fw_managed_domain_remove(qcom);
> +                       return ret;
> +               }
>
> -       ret = reset_control_assert(qcom->resets);
> -       if (ret) {
> -               dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
> -               return ret;
> -       }
> +               qcom->qcom_dwc3_device_ops.resume_resource = fw_managed_resume_resource;
> +               qcom->qcom_dwc3_device_ops.exit_resource = fw_managed_exit_resource;
> +               qcom->qcom_dwc3_device_ops.suspend_resource = fw_managed_suspend_resource;
> +       } else {
> +               qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
> +               if (IS_ERR(qcom->resets)) {
> +                       return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
> +                                            "failed to get resets\n");
> +               }
>
> -       usleep_range(10, 1000);
> +               ret = reset_control_assert(qcom->resets);
> +               if (ret) {
> +                       dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
> +                       return ret;
> +               }
>
> -       ret = reset_control_deassert(qcom->resets);
> -       if (ret) {
> -               dev_err(&pdev->dev, "failed to deassert resets, err=%d\n", ret);
> -               goto reset_assert;
> -       }
> +               usleep_range(10, 1000);
>
> -       ret = dwc3_qcom_clk_init(qcom, of_clk_get_parent_count(np));
> -       if (ret) {
> -               dev_err_probe(dev, ret, "failed to get clocks\n");
> -               goto reset_assert;
> +               ret = reset_control_deassert(qcom->resets);
> +               if (ret) {
> +                       dev_err(&pdev->dev, "failed to deassert resets, err=%d\n", ret);
> +                       goto reset_assert;
> +               }
> +
> +               ret = dwc3_qcom_clk_init(qcom, of_clk_get_parent_count(np));
> +               if (ret) {
> +                       dev_err_probe(dev, ret, "failed to get clocks\n");
> +                       goto reset_assert;
> +               }
> +
> +               //Map the functions here for suspend resume
> +               qcom->qcom_dwc3_device_ops.resume_resource = locally_managed_resume_resource;
> +               qcom->qcom_dwc3_device_ops.exit_resource = locally_managed_exit_resource;
> +               qcom->qcom_dwc3_device_ops.suspend_resource = locally_managed_suspend_resource;
>         }
>
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -916,9 +1053,11 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>                 goto free_urs;
>         }
>
> -       ret = dwc3_qcom_interconnect_init(qcom);
> -       if (ret)
> -               goto depopulate;
> +       if (!qcom->fw_managed) {
> +               ret = dwc3_qcom_interconnect_init(qcom);
> +               if (ret)
> +                       goto depopulate;
> +       }
>
>         qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
>
> @@ -928,8 +1067,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>
>         /* register extcon to override sw_vbus on Vbus change later */
>         ret = dwc3_qcom_register_extcon(qcom);
> -       if (ret)
> -               goto interconnect_exit;
> +       if (ret) {
> +               if (!qcom->fw_managed)
> +                       goto interconnect_exit;
> +               else
> +                       goto depopulate;
> +       }
>
>         wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
>         device_init_wakeup(&pdev->dev, wakeup_source);
> @@ -956,13 +1099,19 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>         if (qcom->urs_usb)
>                 dwc3_qcom_destroy_urs_usb_platdev(qcom->urs_usb);
>  clk_disable:
> -       for (i = qcom->num_clocks - 1; i >= 0; i--) {
> -               clk_disable_unprepare(qcom->clks[i]);
> -               clk_put(qcom->clks[i]);
> +       if (!qcom->fw_managed) {
> +               for (i = qcom->num_clocks - 1; i >= 0; i--) {
> +                       clk_disable_unprepare(qcom->clks[i]);
> +                       clk_put(qcom->clks[i]);
> +               }

Please stick to a single style. Either have ops and functions or
fw_managed checks through the code. Mixing both of them doesn't look
good.

>         }
>  reset_assert:
> -       reset_control_assert(qcom->resets);
> -
> +       if (qcom->fw_managed) {
> +               pm_runtime_put_sync(qcom->genpd_core);
> +               dwc3_qcom_fw_managed_domain_remove(qcom);
> +       } else {
> +               reset_control_assert(qcom->resets);
> +       }
>         return ret;
>  }
>
> @@ -971,7 +1120,6 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>         struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
>         struct device_node *np = pdev->dev.of_node;
>         struct device *dev = &pdev->dev;
> -       int i;
>
>         if (np) {
>                 of_platform_depopulate(&pdev->dev);
> @@ -984,14 +1132,7 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>         if (qcom->urs_usb)
>                 dwc3_qcom_destroy_urs_usb_platdev(qcom->urs_usb);
>
> -       for (i = qcom->num_clocks - 1; i >= 0; i--) {
> -               clk_disable_unprepare(qcom->clks[i]);
> -               clk_put(qcom->clks[i]);
> -       }
> -       qcom->num_clocks = 0;
> -
> -       dwc3_qcom_interconnect_exit(qcom);
> -       reset_control_assert(qcom->resets);
> +       qcom->qcom_dwc3_device_ops.exit_resource(qcom);
>
>         pm_runtime_allow(dev);
>         pm_runtime_disable(dev);
> --
> 2.7.4
>
>


-- 
With best wishes
Dmitry

Re: [RFC 2/3] USB: dwc3: qcom: Add support for firmware managed resources

[Sriram Dash]

On 3/6/2024 2:15 PM, Dmitry Baryshkov wrote:
> On Tue, 5 Mar 2024 at 18:59, Sriram Dash <quic_sriramd@quicinc.com> wrote:
>>
>> Some target systems allow multiple resources to be managed by firmware.
>> On these targets, tasks related to clocks, regulators, resets, and
>> interconnects can be delegated to the firmware, while the remaining
>> responsibilities are handled by Linux.
>>
>> The driver is responsible for managing multiple power domains and
>> linking them to consumers as needed. Incase there is only single
>> power domain, it is considered to be a standard GDSC hooked on to
>> the qcom dt node which is read and assigned to device structure
>> (by genpd framework) before the driver probe even begins.
>>
>> This differentiation logic allows the driver to determine whether
>> device resources are managed by Linux or firmware, ensuring
>> backward compatibility.
>>
>> Furthermore, minor cleanup is performed for the private data of
>> the SNPS Femto PHY. However, ACPI handling is omitted due to the
>> absence of clients on the ACPI side.
>>
>> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-usb.c       | 290 ++++++++++++++++++++------
>>   drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 213 +++++++++++++++----
>>   drivers/usb/dwc3/dwc3-qcom.c                  | 259 +++++++++++++++++------
> 
> Thank you for mixing three different drivers in a single patch. This
> saves us from sending two more letters. In future please abstain from
> doing that and group your changes logically and atomically. Although
> QMP and SNPS PHYs are maintained within the same subsystem, changing
> them are two separate patches, as the changes are similar but
> unrelated.
> 

Thanks for the Review Dmitry. Will take care next time.

>>   3 files changed, 594 insertions(+), 168 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>> index 8525393..1ac1b50 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>> @@ -21,6 +21,9 @@
>>
>>   #include "phy-qcom-qmp-common.h"
>>
>> +#include <linux/pm_opp.h>
>> +#include <linux/pm_domain.h>
>> +
>>   #include "phy-qcom-qmp.h"
>>   #include "phy-qcom-qmp-pcs-misc-v3.h"
>>   #include "phy-qcom-qmp-pcs-misc-v4.h"
>> @@ -1212,6 +1215,9 @@ struct qmp_phy_cfg {
>>          unsigned int pcs_usb_offset;
>>   };
>>
>> +#define DOMAIN_GENPD_TRANSFER                  0
>> +#define DOMAIN_GENPD_CORE                      1
> 
> Why do you need them defined on a global level if later you copy
> necessary data away from the list?
> 

OK.
They will not be needed if i use the dev_pm_domain_attach.
Will update in the next revision.

>> +
>>   struct qmp_usb {
>>          struct device *dev;
>>
>> @@ -1236,6 +1242,19 @@ struct qmp_usb {
>>          struct phy *phy;
>>
>>          struct clk_fixed_rate pipe_clk_fixed;
>> +
>> +       struct dev_pm_domain_list *pd_list;
>> +       struct device *genpd_core;
>> +       struct device *genpd_transfer;
>> +
>> +       bool fw_managed;
>> +       /* separate resource management for fw_managed vs locally managed devices */
>> +       struct qmp_usb_device_ops {
> 
> Do you see nested structure definitions in this file? I don't think so.
> 

Will drop the ops. As suggested to follow single approach,
will make descision according to "fw_managed" which is
unavoidable.

>> +               int (*bus_resume_resource)(struct qmp_usb *qmp);
>> +               int (*runtime_resume_resource)(struct qmp_usb *qmp);
>> +               int (*bus_suspend_resource)(struct qmp_usb *qmp);
>> +               int (*runtime_suspend_resource)(struct qmp_usb *qmp);
>> +       } qmp_usb_device_ops;
> 
> Just ops, pm_ops or bus_pm_ops.
> 
>>   };
>>
>>   static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
>> @@ -1598,6 +1617,41 @@ static const struct qmp_phy_cfg x1e80100_usb3_uniphy_cfg = {
>>          .regs                   = qmp_v7_usb3phy_regs_layout,
>>   };
>>
>> +static void qmp_fw_managed_domain_remove(struct qmp_usb *qmp)
>> +{
>> +       dev_pm_domain_detach_list(qmp->pd_list);
> 
> This looks like a candidate for devm_add_action_or_reset
> 

With the dev_pm_domain_attach*, will change it to
dev_pm_domain_detach

>> +}
>> +
>> +static int qmp_fw_managed_domain_init(struct qmp_usb *qmp)
>> +{
>> +       struct device *dev = qmp->dev;
>> +       struct dev_pm_domain_attach_data pd_data = {
>> +               .pd_flags       = PD_FLAG_NO_DEV_LINK,
>> +               .pd_names       = (const char*[]) { "usb_transfer", "usb_core" },
>> +               .num_pd_names   = 2,
>> +       };
> 
> Can you pass an empty list instead? Or instead call
> dev_pm_domain_attach twice, if you are not using the pd_list later on?
> 

Thanks for the suggestion. will use dev_pm_domain_attach*.

>> +       int ret = 0;
>> +
>> +       if (!dev->pm_domain) {
>> +               ret = dev_pm_domain_attach_list(dev, &pd_data, &qmp->pd_list);
>> +               if (ret < 0) {
>> +                       dev_err(dev, "domain attach failed %d)\n", ret);
>> +                       return ret;
>> +               }
>> +
>> +               qmp->genpd_transfer = qmp->pd_list->pd_devs[DOMAIN_GENPD_TRANSFER];
>> +               qmp->genpd_core = qmp->pd_list->pd_devs[DOMAIN_GENPD_CORE];
>> +
>> +
>> +               dev_dbg(dev, "domains attached successfully\n");
>> +       } else {
>> +               dev_dbg(dev, "domain attach fail\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static int qmp_usb_serdes_init(struct qmp_usb *qmp)
>>   {
>>          const struct qmp_phy_cfg *cfg = qmp->cfg;
>> @@ -1610,11 +1664,20 @@ static int qmp_usb_serdes_init(struct qmp_usb *qmp)
>>          return 0;
>>   }
>>
>> -static int qmp_usb_init(struct phy *phy)
>> +static int fw_managed_ssphy_bus_init(struct qmp_usb *qmp)
> 
> Please take a look around. All functions have a common naming
> structure, which you are not following. Please do not do that _ever_.
> 

Sure. will take care in next version.

>> +{
>> +       int ret = 0;
>> +
>> +       ret = pm_runtime_get_sync(qmp->genpd_core);
>> +       if (ret < 0)
>> +               dev_err(qmp->dev, "Failed to enable fw managed resources");
>> +
>> +       return ret;
>> +}
>> +
>> +static int locally_managed_ssphy_bus_init(struct qmp_usb *qmp)
>>   {
>> -       struct qmp_usb *qmp = phy_get_drvdata(phy);
>>          const struct qmp_phy_cfg *cfg = qmp->cfg;
>> -       void __iomem *pcs = qmp->pcs;
>>          int ret;
>>
>>          ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>> @@ -1639,8 +1702,11 @@ static int qmp_usb_init(struct phy *phy)
>>          if (ret)
>>                  goto err_assert_reset;
>>
>> -       qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
>> -
>> +       ret = clk_prepare_enable(qmp->pipe_clk);
>> +       if (ret) {
>> +               dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
>> +               return ret;
>> +       }
> 
> Here (and further in the file) you are changing the order of SW_PWRDN
> bit toggling and pipe_clk enablement / disablement. Is this a safe
> change?
> Have you tested it? If so, on which platforms?
> 

Yes. This is OK.
For the qmp usb enable, we are enabling the regulators
first, then enabling the clocks and signalling phy power
up, then configuring and intialising the phy.

This sequence is tested on SA8775 and SC8280.

However, as we will be deciding the sequence wrt
fw_managed flag, we can keep the sequence unchanged.

>>          return 0;
>>
>>   err_assert_reset:
>> @@ -1651,11 +1717,22 @@ static int qmp_usb_init(struct phy *phy)
>>          return ret;
>>   }
>>
>> -static int qmp_usb_exit(struct phy *phy)
>> +static int fw_managed_ssphy_bus_exit(struct qmp_usb *qmp)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = pm_runtime_put_sync(qmp->genpd_core);
>> +       if (ret < 0)
>> +               dev_err(qmp->dev, "Failed to disable fw managed resources");
>> +
>> +       return 0;
>> +}
>> +
>> +static int locally_managed_ssphy_bus_exit(struct qmp_usb *qmp)
>>   {
>> -       struct qmp_usb *qmp = phy_get_drvdata(phy);
>>          const struct qmp_phy_cfg *cfg = qmp->cfg;
>>
>> +       clk_disable_unprepare(qmp->pipe_clk);
>>          reset_control_bulk_assert(qmp->num_resets, qmp->resets);
>>
>>          clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
>> @@ -1677,13 +1754,9 @@ static int qmp_usb_power_on(struct phy *phy)
>>          unsigned int val;
>>          int ret;
>>
>> -       qmp_usb_serdes_init(qmp);
>> +       qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
>>
>> -       ret = clk_prepare_enable(qmp->pipe_clk);
>> -       if (ret) {
>> -               dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
>> -               return ret;
>> -       }
>> +       qmp_usb_serdes_init(qmp);
>>
>>          /* Tx, Rx, and PCS configurations */
>>          qmp_configure_lane(tx, cfg->tx_tbl, cfg->tx_tbl_num, 1);
>> @@ -1708,15 +1781,10 @@ static int qmp_usb_power_on(struct phy *phy)
>>                                   PHY_INIT_COMPLETE_TIMEOUT);
>>          if (ret) {
>>                  dev_err(qmp->dev, "phy initialization timed-out\n");
>> -               goto err_disable_pipe_clk;
>> +               return ret;
>>          }
>>
>>          return 0;
>> -
>> -err_disable_pipe_clk:
>> -       clk_disable_unprepare(qmp->pipe_clk);
>> -
>> -       return ret;
>>   }
>>
>>   static int qmp_usb_power_off(struct phy *phy)
>> @@ -1724,8 +1792,6 @@ static int qmp_usb_power_off(struct phy *phy)
>>          struct qmp_usb *qmp = phy_get_drvdata(phy);
>>          const struct qmp_phy_cfg *cfg = qmp->cfg;
>>
>> -       clk_disable_unprepare(qmp->pipe_clk);
>> -
>>          /* PHY reset */
>>          qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
>>
>> @@ -1742,27 +1808,30 @@ static int qmp_usb_power_off(struct phy *phy)
>>
>>   static int qmp_usb_enable(struct phy *phy)
>>   {
>> +       struct qmp_usb *qmp = phy_get_drvdata(phy);
>>          int ret;
>>
>> -       ret = qmp_usb_init(phy);
>> +       ret = qmp->qmp_usb_device_ops.bus_resume_resource(qmp);
>>          if (ret)
>>                  return ret;
>>
>>          ret = qmp_usb_power_on(phy);
>>          if (ret)
>> -               qmp_usb_exit(phy);
>> +               qmp->qmp_usb_device_ops.bus_suspend_resource(qmp);
>>
>>          return ret;
>>   }
>>
>>   static int qmp_usb_disable(struct phy *phy)
>>   {
>> +       struct qmp_usb *qmp = phy_get_drvdata(phy);
>>          int ret;
>>
>>          ret = qmp_usb_power_off(phy);
>>          if (ret)
>>                  return ret;
>> -       return qmp_usb_exit(phy);
>> +
>> +       return qmp->qmp_usb_device_ops.bus_suspend_resource(qmp);
>>   }
>>
>>   static int qmp_usb_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>> @@ -1828,6 +1897,25 @@ static void qmp_usb_disable_autonomous_mode(struct qmp_usb *qmp)
>>          qphy_clrbits(pcs_usb, cfg->regs[QPHY_PCS_LFPS_RXTERM_IRQ_CLEAR], IRQ_CLEAR);
>>   }
>>
>> +static int locally_managed_ssphy_runtime_exit(struct qmp_usb *qmp)
>> +{
>> +       clk_disable_unprepare(qmp->pipe_clk);
>> +       clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
>> +
>> +       return 0;
>> +}
>> +
>> +static int fw_managed_ssphy_runtime_exit(struct qmp_usb *qmp)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = pm_runtime_put_sync(qmp->genpd_transfer);
>> +       if (ret < 0)
>> +               dev_err(qmp->dev, "Failed to disable fw managed resources");
>> +
>> +       return 0;
>> +}
>> +
>>   static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev)
>>   {
>>          struct qmp_usb *qmp = dev_get_drvdata(dev);
>> @@ -1841,16 +1929,44 @@ static int __maybe_unused qmp_usb_runtime_suspend(struct device *dev)
>>
>>          qmp_usb_enable_autonomous_mode(qmp);
>>
>> -       clk_disable_unprepare(qmp->pipe_clk);
>> -       clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
>> +       qmp->qmp_usb_device_ops.runtime_suspend_resource(qmp);
>>
>>          return 0;
>>   }
>>
>> +static int locally_managed_ssphy_runtime_init(struct qmp_usb *qmp)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = clk_prepare_enable(qmp->pipe_clk);
>> +       if (ret) {
>> +               dev_err(qmp->dev, "pipe_clk enable failed, err=%d\n", ret);
>> +               clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int fw_managed_ssphy_runtime_init(struct qmp_usb *qmp)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = pm_runtime_get_sync(qmp->genpd_transfer);
>> +       if (ret < 0)
>> +               dev_err(qmp->dev, "Failed to enable fw managed resources");
>> +
>> +       return ret;
>> +}
>> +
>>   static int __maybe_unused qmp_usb_runtime_resume(struct device *dev)
>>   {
>> +       int ret;
>>          struct qmp_usb *qmp = dev_get_drvdata(dev);
>> -       int ret = 0;
>>
>>          dev_vdbg(dev, "Resuming QMP phy, mode:%d\n", qmp->mode);
>>
>> @@ -1859,17 +1975,10 @@ static int __maybe_unused qmp_usb_runtime_resume(struct device *dev)
>>                  return 0;
>>          }
>>
>> -       ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
>> +       ret = qmp->qmp_usb_device_ops.runtime_resume_resource(qmp);
>>          if (ret)
>>                  return ret;
>>
>> -       ret = clk_prepare_enable(qmp->pipe_clk);
>> -       if (ret) {
>> -               dev_err(dev, "pipe_clk enable failed, err=%d\n", ret);
>> -               clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
>> -               return ret;
>> -       }
>> -
>>          qmp_usb_disable_autonomous_mode(qmp);
>>
>>          return 0;
>> @@ -2059,22 +2168,24 @@ static int qmp_usb_parse_dt_legacy(struct qmp_usb *qmp, struct device_node *np)
>>                  qmp->pcs_misc = NULL;
>>          }
>>
>> -       qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL);
>> -       if (IS_ERR(qmp->pipe_clk)) {
>> -               return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
>> -                                    "failed to get pipe clock\n");
>> -       }
>> +       if (!qmp->fw_managed) {
>> +               qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL);
>> +               if (IS_ERR(qmp->pipe_clk)) {
>> +                       return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
>> +                                            "failed to get pipe clock\n");
>> +               }
>>
>> -       ret = devm_clk_bulk_get_all(qmp->dev, &qmp->clks);
>> -       if (ret < 0)
>> -               return ret;
>> +               ret = devm_clk_bulk_get_all(qmp->dev, &qmp->clks);
>> +               if (ret < 0)
>> +                       return ret;
>>
>> -       qmp->num_clks = ret;
>> +               qmp->num_clks = ret;
>>
>> -       ret = qmp_usb_reset_init(qmp, usb3phy_legacy_reset_l,
>> -                                ARRAY_SIZE(usb3phy_legacy_reset_l));
>> -       if (ret)
>> -               return ret;
>> +               ret = qmp_usb_reset_init(qmp, usb3phy_legacy_reset_l,
>> +                                        ARRAY_SIZE(usb3phy_legacy_reset_l));
>> +               if (ret)
>> +                       return ret;
>> +       }
>>
>>          return 0;
>>   }
>> @@ -2104,21 +2215,23 @@ static int qmp_usb_parse_dt(struct qmp_usb *qmp)
>>          qmp->tx = base + offs->tx;
>>          qmp->rx = base + offs->rx;
>>
>> -       ret = qmp_usb_clk_init(qmp);
>> -       if (ret)
>> -               return ret;
>> -
>> -       qmp->pipe_clk = devm_clk_get(dev, "pipe");
>> -       if (IS_ERR(qmp->pipe_clk)) {
>> -               return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
>> -                                    "failed to get pipe clock\n");
>> +       if (!qmp->fw_managed) {
>> +               ret = qmp_usb_clk_init(qmp);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               qmp->pipe_clk = devm_clk_get(dev, "pipe");
>> +               if (IS_ERR(qmp->pipe_clk)) {
>> +                       return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
>> +                                            "failed to get pipe clock\n");
>> +               }
>> +
>> +               ret = qmp_usb_reset_init(qmp, usb3phy_reset_l,
>> +                                        ARRAY_SIZE(usb3phy_reset_l));
>> +               if (ret)
>> +                       return ret;
>>          }
>>
>> -       ret = qmp_usb_reset_init(qmp, usb3phy_reset_l,
>> -                                ARRAY_SIZE(usb3phy_reset_l));
>> -       if (ret)
>> -               return ret;
>> -
>>          return 0;
>>   }
>>
>> @@ -2140,9 +2253,36 @@ static int qmp_usb_probe(struct platform_device *pdev)
>>          if (!qmp->cfg)
>>                  return -EINVAL;
>>
>> -       ret = qmp_usb_vreg_init(qmp);
>> -       if (ret)
>> -               return ret;
>> +       qmp->fw_managed  = device_property_read_bool(dev, "qmp,fw-managed");
>> +       if (qmp->fw_managed) {
>> +               ret = qmp_fw_managed_domain_init(qmp);
>> +               if (ret) {
>> +                       dev_err(dev, "Failed to init domains. Bail out");
>> +                       return ret;
>> +               }
>> +
>> +               qmp->qmp_usb_device_ops.bus_resume_resource =
>> +                                               fw_managed_ssphy_bus_init;
>> +               qmp->qmp_usb_device_ops.runtime_resume_resource =
>> +                                               fw_managed_ssphy_runtime_init;
>> +               qmp->qmp_usb_device_ops.bus_suspend_resource =
>> +                                               fw_managed_ssphy_bus_exit;
>> +               qmp->qmp_usb_device_ops.runtime_suspend_resource =
>> +                                               fw_managed_ssphy_runtime_exit;
> 
> Use a pointer to ops and constant data instead of setting function
> pointers manually.
> 

Will drop the ops and use fw_managed.

>> +       } else {
>> +               ret = qmp_usb_vreg_init(qmp);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               qmp->qmp_usb_device_ops.bus_resume_resource =
>> +                                               locally_managed_ssphy_bus_init;
>> +               qmp->qmp_usb_device_ops.runtime_resume_resource =
>> +                                               locally_managed_ssphy_runtime_init;
>> +               qmp->qmp_usb_device_ops.bus_suspend_resource =
>> +                                               locally_managed_ssphy_bus_exit;
>> +               qmp->qmp_usb_device_ops.runtime_suspend_resource =
>> +                                               locally_managed_ssphy_runtime_exit;
>> +       }
>>
>>          /* Check for legacy binding with child node. */
>>          np = of_get_next_available_child(dev->of_node, NULL);
>> @@ -2165,9 +2305,11 @@ static int qmp_usb_probe(struct platform_device *pdev)
>>           */
>>          pm_runtime_forbid(dev);
>>
>> -       ret = phy_pipe_clk_register(qmp, np);
>> -       if (ret)
>> -               goto err_node_put;
>> +       if (!qmp->fw_managed) {
>> +               ret = phy_pipe_clk_register(qmp, np);
>> +               if (ret)
>> +                       goto err_node_put;
> 
> pipe_clk is an input to the GCC. If you are not registering it here,
> how would GCC get it?
> 

The pipe clk will be managed by the firmware.

>> +       }
>>
>>          qmp->phy = devm_phy_create(dev, np, &qmp_usb_phy_ops);
>>          if (IS_ERR(qmp->phy)) {
>> @@ -2186,9 +2328,22 @@ static int qmp_usb_probe(struct platform_device *pdev)
>>
>>   err_node_put:
>>          of_node_put(np);
>> +       if (qmp->fw_managed)
>> +               qmp_fw_managed_domain_remove(qmp);
>>          return ret;
>>   }
>>
>> +static void qmp_usb_remove(struct platform_device *pdev)
>> +{
>> +       struct qmp_usb *qmp = platform_get_drvdata(pdev);
>> +
>> +       if (qmp->fw_managed) {
>> +               pm_runtime_put_sync(qmp->genpd_core);
>> +               qmp_fw_managed_domain_remove(qmp);
>> +       }
>> +}
>> +
>> +
>>   static const struct of_device_id qmp_usb_of_match_table[] = {
>>          {
>>                  .compatible = "qcom,ipq6018-qmp-usb3-phy",
>> @@ -2239,6 +2394,7 @@ MODULE_DEVICE_TABLE(of, qmp_usb_of_match_table);
>>
>>   static struct platform_driver qmp_usb_driver = {
>>          .probe          = qmp_usb_probe,
>> +       .remove_new     = qmp_usb_remove,
>>          .driver = {
>>                  .name   = "qcom-qmp-usb-phy",
>>                  .pm     = &qmp_usb_pm_ops,
>> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
>> index eb0b0f6..9a1377f 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c
>> @@ -17,6 +17,9 @@
>>   #include <linux/reset.h>
>>   #include <linux/slab.h>
>>
>> +#include <linux/pm_opp.h>
>> +#include <linux/pm_domain.h>
>> +
>>   #define USB2_PHY_USB_PHY_UTMI_CTRL0            (0x3c)
>>   #define SLEEPM                                 BIT(0)
>>   #define OPMODE_MASK                            GENMASK(4, 3)
>> @@ -89,7 +92,7 @@ struct override_param {
>>          u8      reg_val;
>>   };
>>
>> -struct override_param_map {
>> +struct snps_femto_v2_priv_data {
>>          const char *prop_name;
>>          const struct override_param *param_table;
>>          u8 table_size;
>> @@ -106,6 +109,9 @@ struct phy_override_seq {
>>
>>   #define NUM_HSPHY_TUNING_PARAMS        (9)
>>
>> +#define DOMAIN_GENPD_TRANSFER                  0
>> +#define DOMAIN_GENPD_CORE                      1
> 
> You guessed it, see my comments for the QMP PHY changes.
> 
>> +
>>   /**
>>    * struct qcom_snps_hsphy - snps hs phy attributes
>>    *
>> @@ -136,8 +142,54 @@ struct qcom_snps_hsphy {
>>          bool phy_initialized;
>>          enum phy_mode mode;
>>          struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS];
>> +
>> +       struct dev_pm_domain_list *pd_list;
>> +       struct device *genpd_core;
>> +       struct device *genpd_transfer;
>> +
>> +       bool fw_managed;
>> +       /* separate resource management for fw_managed vs locally managed devices */
>> +       struct snps_femto_v2_device_ops {
>> +               int (*resume_resource)(struct qcom_snps_hsphy *hsphy);
>> +               int (*suspend_resource)(struct qcom_snps_hsphy *hsphy);
>> +       } snps_femto_v2_device_ops;
>> +
>>   };
>>
>> +static void hsphy_fw_managed_domain_remove(struct qcom_snps_hsphy *hsphy)
> 
> qcom_snps_hsphy_ prefix.
> 

Sure. will take care in next version.

>> +{
>> +       dev_pm_domain_detach_list(hsphy->pd_list);
>> +}
>> +
>> +static int hsphy_fw_managed_domain_init(struct qcom_snps_hsphy *hsphy)
>> +{
>> +       struct device *dev = hsphy->dev;
>> +       struct dev_pm_domain_attach_data pd_data = {
>> +               .pd_flags       = PD_FLAG_NO_DEV_LINK,
>> +               .pd_names       = (const char*[]) { "usb_transfer", "usb_core" },
>> +               .num_pd_names   = 2,
>> +       };
>> +       int ret = 0;
>> +
>> +       if (!dev->pm_domain) {
>> +               ret = dev_pm_domain_attach_list(dev, &pd_data, &hsphy->pd_list);
>> +               if (ret < 0) {
>> +                       dev_err(dev, "domain attach failed %d)\n", ret);
>> +                       return ret;
>> +               }
>> +
>> +               hsphy->genpd_transfer = hsphy->pd_list->pd_devs[DOMAIN_GENPD_TRANSFER];
>> +               hsphy->genpd_core = hsphy->pd_list->pd_devs[DOMAIN_GENPD_CORE];
>> +
>> +               dev_dbg(dev, "domains attached successfully\n");
>> +       } else {
>> +               dev_dbg(dev, "domain attach fail\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy)
>>   {
>>          struct device *dev = hsphy->dev;
>> @@ -316,27 +368,27 @@ static const struct override_param ls_fs_output_impedance_sc7280[] = {
>>          { 1310, 0 },
>>   };
>>
>> -static const struct override_param_map sc7280_snps_7nm_phy[] = {
>> +static const struct snps_femto_v2_priv_data sc7280_snps_7nm_phy[] = {
> 
> What does this have in common with the clocks / regulators being
> fw-managed or not?
> 

Will update this separate to the fw managed changes.

>>          {
>>                  "qcom,hs-disconnect-bp",
>>                  hs_disconnect_sc7280,
>>                  ARRAY_SIZE(hs_disconnect_sc7280),
>>                  USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
>> -               HS_DISCONNECT_MASK
>> +               HS_DISCONNECT_MASK,
> 
> Why? Does it require to be changed?
> 
>>          },
>>          {
>>                  "qcom,squelch-detector-bp",
>>                  squelch_det_threshold_sc7280,
>>                  ARRAY_SIZE(squelch_det_threshold_sc7280),
>>                  USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X0,
>> -               SQUELCH_DETECTOR_MASK
>> +               SQUELCH_DETECTOR_MASK,
>>          },
>>          {
>>                  "qcom,hs-amplitude-bp",
>>                  hs_amplitude_sc7280,
>>                  ARRAY_SIZE(hs_amplitude_sc7280),
>>                  USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X1,
>> -               HS_AMPLITUDE_MASK
>> +               HS_AMPLITUDE_MASK,
>>          },
>>          {
>>                  "qcom,pre-emphasis-duration-bp",
>> @@ -357,14 +409,14 @@ static const struct override_param_map sc7280_snps_7nm_phy[] = {
>>                  hs_rise_fall_time_sc7280,
>>                  ARRAY_SIZE(hs_rise_fall_time_sc7280),
>>                  USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
>> -               HS_RISE_FALL_MASK
>> +               HS_RISE_FALL_MASK,
>>          },
>>          {
>>                  "qcom,hs-crossover-voltage-microvolt",
>>                  hs_crossover_voltage_sc7280,
>>                  ARRAY_SIZE(hs_crossover_voltage_sc7280),
>>                  USB2_PHY_USB_PHY_HS_PHY_OVERRIDE_X2,
>> -               HS_CROSSOVER_VOLTAGE_MASK
>> +               HS_CROSSOVER_VOLTAGE_MASK,
>>          },
>>          {
>>                  "qcom,hs-output-impedance-micro-ohms",
>> @@ -383,12 +435,31 @@ static const struct override_param_map sc7280_snps_7nm_phy[] = {
>>          {},
>>   };
>>
>> -static int qcom_snps_hsphy_init(struct phy *phy)
>> +static int fw_managed_hsphy_init(struct qcom_snps_hsphy *hsphy)
>>   {
>> -       struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
>> -       int ret, i;
>> +       int ret = 0;
> 
> Why do you need a value here if later on you set it again?
> 

Thanks.
Will not be required once we switch to fw_managed
descision from ops.

>>
>> -       dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
>> +       ret = pm_runtime_get_sync(hsphy->genpd_core);
>> +       if (ret < 0)
>> +               dev_err(hsphy->dev, "Failed to enable fw managed resources");
>> +
>> +       return ret;
>> +}
>> +
>> +static int fw_managed_hsphy_exit(struct qcom_snps_hsphy *hsphy)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = pm_runtime_put_sync(hsphy->genpd_core);
>> +       if (ret < 0)
>> +               dev_err(hsphy->dev, "Failed to disable fw managed resources");
>> +
>> +       return 0;
>> +}
>> +
>> +static int locally_managed_hsphy_init(struct qcom_snps_hsphy *hsphy)
>> +{
>> +       int ret;
>>
>>          ret = regulator_bulk_enable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
>>          if (ret)
>> @@ -396,13 +467,13 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>>
>>          ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks);
>>          if (ret) {
>> -               dev_err(&phy->dev, "failed to enable clocks, %d\n", ret);
>> +               dev_err(hsphy->dev, "failed to enable clocks, %d\n", ret);
>>                  goto poweroff_phy;
>>          }
>>
>>          ret = reset_control_assert(hsphy->phy_reset);
>>          if (ret) {
>> -               dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret);
>> +               dev_err(hsphy->dev, "failed to assert phy_reset, %d\n", ret);
>>                  goto disable_clks;
>>          }
>>
>> @@ -410,10 +481,42 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>>
>>          ret = reset_control_deassert(hsphy->phy_reset);
>>          if (ret) {
>> -               dev_err(&phy->dev, "failed to de-assert phy_reset, %d\n", ret);
>> +               dev_err(hsphy->dev, "failed to de-assert phy_reset, %d\n", ret);
>>                  goto disable_clks;
>>          }
>>
>> +       return 0;
>> +
>> +disable_clks:
>> +       clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
>> +poweroff_phy:
>> +       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
>> +
>> +       return ret;
>> +}
>> +
>> +static int locally_managed_hsphy_exit(struct qcom_snps_hsphy *hsphy)
>> +{
>> +       reset_control_assert(hsphy->phy_reset);
>> +       clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
>> +       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
>> +
>> +       return 0;
>> +}
>> +
>> +static int qcom_snps_hsphy_init(struct phy *phy)
>> +{
>> +       struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
>> +       int ret, i;
>> +
>> +       dev_vdbg(&phy->dev, "%s(): Initializing SNPS HS phy\n", __func__);
>> +
>> +       ret = hsphy->snps_femto_v2_device_ops.resume_resource(hsphy);
>> +       if (ret) {
>> +               dev_err(&phy->dev, "Error resumeing hsphy resources %d\n", ret);
>> +               return ret;
>> +       }
>> +
>>          qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_CFG0,
>>                                          UTMI_PHY_CMN_CTRL_OVERRIDE_EN,
>>                                          UTMI_PHY_CMN_CTRL_OVERRIDE_EN);
>> @@ -467,22 +570,19 @@ static int qcom_snps_hsphy_init(struct phy *phy)
>>          hsphy->phy_initialized = true;
>>
>>          return 0;
>> -
>> -disable_clks:
>> -       clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
>> -poweroff_phy:
>> -       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
>> -
>> -       return ret;
>>   }
>>
>>   static int qcom_snps_hsphy_exit(struct phy *phy)
>>   {
>>          struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy);
>> +       int ret = 0;
>> +
>> +       ret = hsphy->snps_femto_v2_device_ops.suspend_resource(hsphy);
>> +       if (ret) {
>> +               dev_err(&phy->dev, "Error suspending hsphy resources %d\n", ret);
>> +               return ret;
>> +       }
>>
>> -       reset_control_assert(hsphy->phy_reset);
>> -       clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks);
>> -       regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs);
>>          hsphy->phy_initialized = false;
>>
>>          return 0;
>> @@ -497,7 +597,8 @@ static const struct phy_ops qcom_snps_hsphy_gen_ops = {
>>
>>   static const struct of_device_id qcom_snps_hsphy_of_match_table[] = {
>>          { .compatible   = "qcom,sm8150-usb-hs-phy", },
>> -       { .compatible   = "qcom,usb-snps-hs-5nm-phy", },
>> +       { .compatible   = "qcom,sa8775p-usb-hs-phy", },
>> +       { .compatible   = "qcom,sc8280xp-usb-hs-phy", },
> 
> WTF? Please keep the fallback entry in place. It looks to me that
> somebody didn't read documentation on the OF device matching. Please
> read the code first.
> 

Will take it separately.

>>          {
>>                  .compatible     = "qcom,usb-snps-hs-7nm-phy",
>>                  .data           = &sc7280_snps_7nm_phy,
>> @@ -513,7 +614,7 @@ static const struct dev_pm_ops qcom_snps_hsphy_pm_ops = {
>>   };
>>
>>   static void qcom_snps_hsphy_override_param_update_val(
>> -                       const struct override_param_map map,
>> +                       const struct snps_femto_v2_priv_data map,
>>                          s32 dt_val, struct phy_override_seq *seq_entry)
>>   {
>>          int i;
>> @@ -541,7 +642,7 @@ static void qcom_snps_hsphy_read_override_param_seq(struct device *dev)
>>          s32 val;
>>          int ret, i;
>>          struct qcom_snps_hsphy *hsphy;
>> -       const struct override_param_map *cfg = of_device_get_match_data(dev);
>> +       const struct snps_femto_v2_priv_data *cfg = of_device_get_match_data(dev);
>>
>>          if (!cfg)
>>                  return;
>> @@ -580,24 +681,39 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>>          if (IS_ERR(hsphy->base))
>>                  return PTR_ERR(hsphy->base);
>>
>> -       ret = qcom_snps_hsphy_clk_init(hsphy);
>> -       if (ret)
>> -               return dev_err_probe(dev, ret, "failed to initialize clocks\n");
>>
>> -       hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>> -       if (IS_ERR(hsphy->phy_reset)) {
>> -               dev_err(dev, "failed to get phy core reset\n");
>> -               return PTR_ERR(hsphy->phy_reset);
>> -       }
>> +       hsphy->fw_managed  = device_property_read_bool(dev, "hsphy,fw-managed");
>> +       if (hsphy->fw_managed) {
>> +               ret = hsphy_fw_managed_domain_init(hsphy);
>> +               if (ret) {
>> +                       dev_err(dev, "Failed to init domains. Bail out");
>> +                       return ret;
>> +               }
>>
>> -       num = ARRAY_SIZE(hsphy->vregs);
>> -       for (i = 0; i < num; i++)
>> -               hsphy->vregs[i].supply = qcom_snps_hsphy_vreg_names[i];
>> +               hsphy->snps_femto_v2_device_ops.resume_resource = fw_managed_hsphy_init;
>> +               hsphy->snps_femto_v2_device_ops.suspend_resource = fw_managed_hsphy_exit;
>> +       } else {
>> +               ret = qcom_snps_hsphy_clk_init(hsphy);
>> +               if (ret)
>> +                       return dev_err_probe(dev, ret, "failed to initialize clocks\n");
>>
>> -       ret = devm_regulator_bulk_get(dev, num, hsphy->vregs);
>> -       if (ret)
>> -               return dev_err_probe(dev, ret,
>> -                                    "failed to get regulator supplies\n");
>> +               hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>> +               if (IS_ERR(hsphy->phy_reset)) {
>> +                       dev_err(dev, "failed to get phy core reset\n");
>> +                       return PTR_ERR(hsphy->phy_reset);
>> +               }
>> +
>> +               num = ARRAY_SIZE(hsphy->vregs);
>> +               for (i = 0; i < num; i++)
>> +                       hsphy->vregs[i].supply = qcom_snps_hsphy_vreg_names[i];
>> +
>> +               ret = devm_regulator_bulk_get(dev, num, hsphy->vregs);
>> +               if (ret)
>> +                       return dev_err_probe(dev, ret,
>> +                                            "failed to get regulator supplies\n");
>> +               hsphy->snps_femto_v2_device_ops.resume_resource = locally_managed_hsphy_init;
>> +               hsphy->snps_femto_v2_device_ops.suspend_resource = locally_managed_hsphy_exit;
>> +       }
>>
>>          pm_runtime_set_active(dev);
>>          pm_runtime_enable(dev);
>> @@ -609,6 +725,8 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>>
>>          generic_phy = devm_phy_create(dev, NULL, &qcom_snps_hsphy_gen_ops);
>>          if (IS_ERR(generic_phy)) {
>> +               if (hsphy->fw_managed)
>> +                       hsphy_fw_managed_domain_remove(hsphy);
>>                  ret = PTR_ERR(generic_phy);
>>                  dev_err(dev, "failed to create phy, %d\n", ret);
>>                  return ret;
>> @@ -628,8 +746,19 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>>          return PTR_ERR_OR_ZERO(phy_provider);
>>   }
>>
>> +static void qcom_snps_hsphy_remove(struct platform_device *pdev)
>> +{
>> +       struct qcom_snps_hsphy *hsphy = platform_get_drvdata(pdev);
>> +
>> +       if (hsphy->fw_managed) {
>> +               pm_runtime_put_sync(hsphy->genpd_core);
>> +               hsphy_fw_managed_domain_remove(hsphy);
>> +       }
>> +}
>> +
>>   static struct platform_driver qcom_snps_hsphy_driver = {
>>          .probe          = qcom_snps_hsphy_probe,
>> +       .remove_new     = qcom_snps_hsphy_remove,
>>          .driver = {
>>                  .name   = "qcom-snps-hs-femto-v2-phy",
>>                  .pm = &qcom_snps_hsphy_pm_ops,
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index dbd6a5b..aea11d0 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -22,6 +22,8 @@
>>   #include <linux/iopoll.h>
>>   #include <linux/usb/hcd.h>
>>   #include <linux/usb.h>
>> +#include <linux/pm_opp.h>
>> +#include <linux/pm_domain.h>
>>   #include "core.h"
>>
>>   /* USB QSCRATCH Hardware registers */
>> @@ -53,6 +55,9 @@
>>   #define APPS_USB_AVG_BW 0
>>   #define APPS_USB_PEAK_BW MBps_to_icc(40)
>>
>> +#define DOMAIN_GENPD_TRANSFER                  0
>> +#define DOMAIN_GENPD_CORE                      1
> 
> Same comments as before
> 
>> +
>>   struct dwc3_acpi_pdata {
>>          u32                     qscratch_base_offset;
>>          u32                     qscratch_base_size;
>> @@ -91,8 +96,54 @@ struct dwc3_qcom {
>>          bool                    pm_suspended;
>>          struct icc_path         *icc_path_ddr;
>>          struct icc_path         *icc_path_apps;
>> +
>> +       struct dev_pm_domain_list *pd_list;
>> +       struct device *genpd_core;
>> +       struct device *genpd_transfer;
>> +
>> +       bool                    fw_managed;
>> +       /* separate resource management for fw_managed vs locally managed devices */
>> +       struct qcom_dwc3_device_ops {
>> +               int (*resume_resource)(struct dwc3_qcom *qcom);
>> +               void (*suspend_resource)(struct dwc3_qcom *qcom);
>> +               void (*exit_resource)(struct dwc3_qcom *qcom);
>> +       } qcom_dwc3_device_ops;
>>   };
>>
>> +static void dwc3_qcom_fw_managed_domain_remove(struct dwc3_qcom *qcom)
>> +{
>> +       dev_pm_domain_detach_list(qcom->pd_list);
>> +}
>> +
>> +static int dwc3_qcom_fw_managed_domain_init(struct dwc3_qcom *qcom)
>> +{
>> +       struct device *dev = qcom->dev;
>> +       struct dev_pm_domain_attach_data pd_data = {
>> +               .pd_flags       = PD_FLAG_NO_DEV_LINK,
>> +               .pd_names       = (const char*[]) { "usb_transfer", "usb_core" },
>> +               .num_pd_names   = 2,
>> +       };
>> +       int ret = 0;
>> +
>> +       if (!dev->pm_domain) {
>> +               ret = dev_pm_domain_attach_list(dev, &pd_data, &qcom->pd_list);
>> +               if (ret < 0) {
>> +                       dev_err(dev, "domain attach failed %d)\n", ret);
>> +                       return ret;
>> +               }
>> +
>> +               qcom->genpd_transfer = qcom->pd_list->pd_devs[DOMAIN_GENPD_TRANSFER];
>> +               qcom->genpd_core = qcom->pd_list->pd_devs[DOMAIN_GENPD_CORE];
>> +
>> +               dev_dbg(dev, "domains attached successfully\n");
>> +       } else {
>> +               dev_dbg(dev, "domain attach fail\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>>   {
>>          u32 reg;
>> @@ -417,10 +468,87 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
>>          dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0);
>>   }
>>
>> +static void locally_managed_exit_resource(struct dwc3_qcom *qcom)
>> +{
>> +       int i;
>> +
>> +       for (i = qcom->num_clocks - 1; i >= 0; i--) {
>> +               clk_disable_unprepare(qcom->clks[i]);
>> +               clk_put(qcom->clks[i]);
>> +       }
>> +       qcom->num_clocks = 0;
>> +
>> +       dwc3_qcom_interconnect_exit(qcom);
>> +       reset_control_assert(qcom->resets);
>> +}
>> +
>> +static void fw_managed_exit_resource(struct dwc3_qcom *qcom)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = pm_runtime_put_sync(qcom->genpd_core);
>> +
>> +       if (ret < 0)
>> +               dev_err(qcom->dev, "Failed to disable fw managed resources");
>> +
>> +       dwc3_qcom_fw_managed_domain_remove(qcom);
>> +}
>> +
>> +static int locally_managed_resume_resource(struct dwc3_qcom *qcom)
>> +{
>> +       int ret, i;
>> +
>> +       for (i = 0; i < qcom->num_clocks; i++) {
>> +               ret = clk_prepare_enable(qcom->clks[i]);
>> +               if (ret < 0) {
>> +                       while (--i >= 0)
>> +                               clk_disable_unprepare(qcom->clks[i]);
>> +                       return ret;
>> +               }
>> +       }
>> +
>> +       ret = dwc3_qcom_interconnect_enable(qcom);
>> +       if (ret)
>> +               dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
>> +
>> +       return 0;
>> +}
>> +
>> +static int fw_managed_resume_resource(struct dwc3_qcom *qcom)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = pm_runtime_get_sync(qcom->genpd_transfer);
>> +       if (ret < 0)
>> +               dev_err(qcom->dev, "Failed to enable fw managed resources");
>> +
>> +       return ret;
>> +}
>> +
>> +static void locally_managed_suspend_resource(struct dwc3_qcom *qcom)
>> +{
>> +       int i, ret;
>> +
>> +       for (i = qcom->num_clocks - 1; i >= 0; i--)
>> +               clk_disable_unprepare(qcom->clks[i]);
>> +
>> +       ret = dwc3_qcom_interconnect_disable(qcom);
>> +       if (ret)
>> +               dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
>> +}
>> +
>> +static void fw_managed_suspend_resource(struct dwc3_qcom *qcom)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = pm_runtime_put_sync(qcom->genpd_transfer);
>> +       if (ret < 0)
>> +               dev_err(qcom->dev, "Failed to disable fw managed resources");
>> +}
>> +
>>   static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>>   {
>>          u32 val;
>> -       int i, ret;
>>
>>          if (qcom->is_suspended)
>>                  return 0;
>> @@ -429,12 +557,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>>          if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>                  dev_err(qcom->dev, "HS-PHY not in L2\n");
>>
>> -       for (i = qcom->num_clocks - 1; i >= 0; i--)
>> -               clk_disable_unprepare(qcom->clks[i]);
>> -
>> -       ret = dwc3_qcom_interconnect_disable(qcom);
>> -       if (ret)
>> -               dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
>> +       qcom->qcom_dwc3_device_ops.suspend_resource(qcom);
> 
> Does SCMI manage interconnects too?
> 

Yes. SCMI will be handling interconnect also.

>>
>>          /*
>>           * The role is stable during suspend as role switching is done from a
>> @@ -453,7 +576,6 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>>   static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>>   {
>>          int ret;
>> -       int i;
>>
>>          if (!qcom->is_suspended)
>>                  return 0;
>> @@ -461,18 +583,9 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>>          if (dwc3_qcom_is_host(qcom) && wakeup)
>>                  dwc3_qcom_disable_interrupts(qcom);
>>
>> -       for (i = 0; i < qcom->num_clocks; i++) {
>> -               ret = clk_prepare_enable(qcom->clks[i]);
>> -               if (ret < 0) {
>> -                       while (--i >= 0)
>> -                               clk_disable_unprepare(qcom->clks[i]);
>> -                       return ret;
>> -               }
>> -       }
>> -
>> -       ret = dwc3_qcom_interconnect_enable(qcom);
>> -       if (ret)
>> -               dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
>> +       ret = qcom->qcom_dwc3_device_ops.resume_resource(qcom);
>> +       if (ret < 0)
>> +               return ret;
>>
>>          /* Clear existing events from PHY related to L2 in/out */
>>          dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
>> @@ -833,30 +946,54 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>                  }
>>          }
>>
>> -       qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
>> -       if (IS_ERR(qcom->resets)) {
>> -               return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
>> -                                    "failed to get resets\n");
>> -       }
>> +       qcom->fw_managed  = device_property_read_bool(dev, "qcom,fw-managed");
>> +       if (qcom->fw_managed) {
>> +               ret = dwc3_qcom_fw_managed_domain_init(qcom);
>> +               if (ret) {
>> +                       dev_err(dev, "Failed to init domains. Bail out\n");
>> +                       return ret;
>> +               }
>> +               ret = pm_runtime_resume_and_get(qcom->genpd_core);
>> +               if (ret < 0) {
>> +                       dev_err(qcom->dev, "Failed to enable %s", __func__);
>> +                       dwc3_qcom_fw_managed_domain_remove(qcom);
>> +                       return ret;
>> +               }
>>
>> -       ret = reset_control_assert(qcom->resets);
>> -       if (ret) {
>> -               dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
>> -               return ret;
>> -       }
>> +               qcom->qcom_dwc3_device_ops.resume_resource = fw_managed_resume_resource;
>> +               qcom->qcom_dwc3_device_ops.exit_resource = fw_managed_exit_resource;
>> +               qcom->qcom_dwc3_device_ops.suspend_resource = fw_managed_suspend_resource;
>> +       } else {
>> +               qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
>> +               if (IS_ERR(qcom->resets)) {
>> +                       return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
>> +                                            "failed to get resets\n");
>> +               }
>>
>> -       usleep_range(10, 1000);
>> +               ret = reset_control_assert(qcom->resets);
>> +               if (ret) {
>> +                       dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
>> +                       return ret;
>> +               }
>>
>> -       ret = reset_control_deassert(qcom->resets);
>> -       if (ret) {
>> -               dev_err(&pdev->dev, "failed to deassert resets, err=%d\n", ret);
>> -               goto reset_assert;
>> -       }
>> +               usleep_range(10, 1000);
>>
>> -       ret = dwc3_qcom_clk_init(qcom, of_clk_get_parent_count(np));
>> -       if (ret) {
>> -               dev_err_probe(dev, ret, "failed to get clocks\n");
>> -               goto reset_assert;
>> +               ret = reset_control_deassert(qcom->resets);
>> +               if (ret) {
>> +                       dev_err(&pdev->dev, "failed to deassert resets, err=%d\n", ret);
>> +                       goto reset_assert;
>> +               }
>> +
>> +               ret = dwc3_qcom_clk_init(qcom, of_clk_get_parent_count(np));
>> +               if (ret) {
>> +                       dev_err_probe(dev, ret, "failed to get clocks\n");
>> +                       goto reset_assert;
>> +               }
>> +
>> +               //Map the functions here for suspend resume
>> +               qcom->qcom_dwc3_device_ops.resume_resource = locally_managed_resume_resource;
>> +               qcom->qcom_dwc3_device_ops.exit_resource = locally_managed_exit_resource;
>> +               qcom->qcom_dwc3_device_ops.suspend_resource = locally_managed_suspend_resource;
>>          }
>>
>>          res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> @@ -916,9 +1053,11 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>                  goto free_urs;
>>          }
>>
>> -       ret = dwc3_qcom_interconnect_init(qcom);
>> -       if (ret)
>> -               goto depopulate;
>> +       if (!qcom->fw_managed) {
>> +               ret = dwc3_qcom_interconnect_init(qcom);
>> +               if (ret)
>> +                       goto depopulate;
>> +       }
>>
>>          qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
>>
>> @@ -928,8 +1067,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>
>>          /* register extcon to override sw_vbus on Vbus change later */
>>          ret = dwc3_qcom_register_extcon(qcom);
>> -       if (ret)
>> -               goto interconnect_exit;
>> +       if (ret) {
>> +               if (!qcom->fw_managed)
>> +                       goto interconnect_exit;
>> +               else
>> +                       goto depopulate;
>> +       }
>>
>>          wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
>>          device_init_wakeup(&pdev->dev, wakeup_source);
>> @@ -956,13 +1099,19 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>          if (qcom->urs_usb)
>>                  dwc3_qcom_destroy_urs_usb_platdev(qcom->urs_usb);
>>   clk_disable:
>> -       for (i = qcom->num_clocks - 1; i >= 0; i--) {
>> -               clk_disable_unprepare(qcom->clks[i]);
>> -               clk_put(qcom->clks[i]);
>> +       if (!qcom->fw_managed) {
>> +               for (i = qcom->num_clocks - 1; i >= 0; i--) {
>> +                       clk_disable_unprepare(qcom->clks[i]);
>> +                       clk_put(qcom->clks[i]);
>> +               }
> 
> Please stick to a single style. Either have ops and functions or
> fw_managed checks through the code. Mixing both of them doesn't look
> good.
> 

Sure. will go with fw_managed check.

>>          }
>>   reset_assert:
>> -       reset_control_assert(qcom->resets);
>> -
>> +       if (qcom->fw_managed) {
>> +               pm_runtime_put_sync(qcom->genpd_core);
>> +               dwc3_qcom_fw_managed_domain_remove(qcom);
>> +       } else {
>> +               reset_control_assert(qcom->resets);
>> +       }
>>          return ret;
>>   }
>>
>> @@ -971,7 +1120,6 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>>          struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
>>          struct device_node *np = pdev->dev.of_node;
>>          struct device *dev = &pdev->dev;
>> -       int i;
>>
>>          if (np) {
>>                  of_platform_depopulate(&pdev->dev);
>> @@ -984,14 +1132,7 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>>          if (qcom->urs_usb)
>>                  dwc3_qcom_destroy_urs_usb_platdev(qcom->urs_usb);
>>
>> -       for (i = qcom->num_clocks - 1; i >= 0; i--) {
>> -               clk_disable_unprepare(qcom->clks[i]);
>> -               clk_put(qcom->clks[i]);
>> -       }
>> -       qcom->num_clocks = 0;
>> -
>> -       dwc3_qcom_interconnect_exit(qcom);
>> -       reset_control_assert(qcom->resets);
>> +       qcom->qcom_dwc3_device_ops.exit_resource(qcom);
>>
>>          pm_runtime_allow(dev);
>>          pm_runtime_disable(dev);
>> --
>> 2.7.4
>>
>>
> 
> 

Re: [RFC 2/3] USB: dwc3: qcom: Add support for firmware managed resources

[Dmitry Baryshkov]

On Wed, 6 Mar 2024 at 18:53, Sriram Dash <quic_sriramd@quicinc.com> wrote:
>
> On 3/6/2024 2:15 PM, Dmitry Baryshkov wrote:
> > On Tue, 5 Mar 2024 at 18:59, Sriram Dash <quic_sriramd@quicinc.com> wrote:
> >>
> >> Some target systems allow multiple resources to be managed by firmware.
> >> On these targets, tasks related to clocks, regulators, resets, and
> >> interconnects can be delegated to the firmware, while the remaining
> >> responsibilities are handled by Linux.
> >>
> >> The driver is responsible for managing multiple power domains and
> >> linking them to consumers as needed. Incase there is only single
> >> power domain, it is considered to be a standard GDSC hooked on to
> >> the qcom dt node which is read and assigned to device structure
> >> (by genpd framework) before the driver probe even begins.
> >>
> >> This differentiation logic allows the driver to determine whether
> >> device resources are managed by Linux or firmware, ensuring
> >> backward compatibility.
> >>
> >> Furthermore, minor cleanup is performed for the private data of
> >> the SNPS Femto PHY. However, ACPI handling is omitted due to the
> >> absence of clients on the ACPI side.
> >>
> >> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
> >> ---
> >>   drivers/phy/qualcomm/phy-qcom-qmp-usb.c       | 290 ++++++++++++++++++++------
> >>   drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 213 +++++++++++++++----
> >>   drivers/usb/dwc3/dwc3-qcom.c                  | 259 +++++++++++++++++------

[skipped]

> >> @@ -2165,9 +2305,11 @@ static int qmp_usb_probe(struct platform_device *pdev)
> >>           */
> >>          pm_runtime_forbid(dev);
> >>
> >> -       ret = phy_pipe_clk_register(qmp, np);
> >> -       if (ret)
> >> -               goto err_node_put;
> >> +       if (!qmp->fw_managed) {
> >> +               ret = phy_pipe_clk_register(qmp, np);
> >> +               if (ret)
> >> +                       goto err_node_put;
> >
> > pipe_clk is an input to the GCC. If you are not registering it here,
> > how would GCC get it?
> >
>
> The pipe clk will be managed by the firmware.

Which pipe clk? Coming from the PHY to GCC or from GCC back to the PHY?

Anyway, you can not drop this, this will break the ABI. The rule of
thumb: existing DTB files MUST continue to work.

> >> +       }
> >>
> >>          qmp->phy = devm_phy_create(dev, np, &qmp_usb_phy_ops);
> >>          if (IS_ERR(qmp->phy)) {

-- 
With best wishes
Dmitry

[RFC 3/3] arm64: dts: qcom: sa8775p-ride: Enable support for firmware managed resources

[Sriram Dash]

Establish the channel and domain mapping for the power domains to connect
with firmware, enabling the firmware to handle the assigned resources.
Since these delegated resources will remain invisible to the operating
system, ensure that any references to them are removed.

Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 96 +++++++++++++++++++++++++------
 1 file changed, 77 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
index 26ad05b..b6c9cac 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
@@ -764,8 +764,18 @@
 };
 
 &usb_0 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&usb0_en_state>;
+	/delete-property/ clocks;
+	/delete-property/ clock-names;
+	/delete-property/ assigned-clocks;
+	/delete-property/ assigned-clock-rates;
+	/delete-property/ required-opps;
+	/delete-property/ resets;
+	/delete-property/ interconnects;
+	/delete-property/ interconnect-names;
+
+	power-domains = <TODO>, <TODO>;
+	power-domain-names = "usb_transfer", "usb_core";
+	qcom,fw-managed;
 
 	status = "okay";
 };
@@ -775,23 +785,45 @@
 };
 
 &usb_0_hsphy {
-	vdda-pll-supply = <&vreg_l7a>;
-	vdda18-supply = <&vreg_l6c>;
-	vdda33-supply = <&vreg_l9a>;
+	/delete-property/ clocks;
+	/delete-property/ clock-names;
+	/delete-property/ resets;
+
+	power-domains = <TODO>, <TODO>;
+	power-domain-names = "usb_transfer", "usb_core";
+	hsphy,fw-managed;
 
 	status = "okay";
 };
 
 &usb_0_qmpphy {
-	vdda-phy-supply = <&vreg_l1c>;
-	vdda-pll-supply = <&vreg_l7a>;
+	/delete-property/ clocks;
+	/delete-property/ clock-names;
+	/delete-property/ resets;
+	/delete-property/ reset-names;
+	/delete-property/ #clock-cells;
+	/delete-property/ clock-output-names;
+
+	power-domains = <TODO>, <TODO>;
+	power-domain-names = "usb_transfer", "usb_core";
+	qmp,fw-managed;
 
 	status = "okay";
 };
 
 &usb_1 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&usb1_en_state>;
+	/delete-property/ clocks;
+	/delete-property/ clock-names;
+	/delete-property/ assigned-clocks;
+	/delete-property/ assigned-clock-rates;
+	/delete-property/ required-opps;
+	/delete-property/ resets;
+	/delete-property/ interconnects;
+	/delete-property/ interconnect-names;
+
+	power-domains = <TODO>, <TODO>;
+	power-domain-names = "usb_transfer", "usb_core";
+	qcom,fw-managed;
 
 	status = "okay";
 };
@@ -801,23 +833,45 @@
 };
 
 &usb_1_hsphy {
-	vdda-pll-supply = <&vreg_l7a>;
-	vdda18-supply = <&vreg_l6c>;
-	vdda33-supply = <&vreg_l9a>;
+	/delete-property/ clocks;
+	/delete-property/ clock-names;
+	/delete-property/ resets;
+
+	power-domains = <TODO>, <TODO>;
+	power-domain-names = "usb_transfer", "usb_core";
+	hsphy,fw-managed;
 
 	status = "okay";
 };
 
 &usb_1_qmpphy {
-	vdda-phy-supply = <&vreg_l1c>;
-	vdda-pll-supply = <&vreg_l7a>;
+	/delete-property/ clocks;
+	/delete-property/ clock-names;
+	/delete-property/ resets;
+	/delete-property/ reset-names;
+	/delete-property/ #clock-cells;
+	/delete-property/ clock-output-names;
+
+	power-domains = <TODO>, <TODO>;
+	power-domain-names = "usb_transfer", "usb_core";
+	qmp,fw-managed;
 
 	status = "okay";
 };
 
 &usb_2 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&usb2_en_state>;
+	/delete-property/ clocks;
+	/delete-property/ clock-names;
+	/delete-property/ assigned-clocks;
+	/delete-property/ assigned-clock-rates;
+	/delete-property/ required-opps;
+	/delete-property/ resets;
+	/delete-property/ interconnects;
+	/delete-property/ interconnect-names;
+
+	power-domains = <TODO>, <TODO>;
+	power-domain-names = "usb_transfer", "usb_core";
+	qcom,fw-managed;
 
 	status = "okay";
 };
@@ -827,9 +881,13 @@
 };
 
 &usb_2_hsphy {
-	vdda-pll-supply = <&vreg_l7a>;
-	vdda18-supply = <&vreg_l6c>;
-	vdda33-supply = <&vreg_l9a>;
+	/delete-property/ clocks;
+	/delete-property/ clock-names;
+	/delete-property/ resets;
+
+	power-domains = <TODO>, <TODO>;
+	power-domain-names = "usb_transfer", "usb_core";
+	hsphy,fw-managed;
 
 	status = "okay";
 };
-- 
2.7.4

Re: [RFC 3/3] arm64: dts: qcom: sa8775p-ride: Enable support for firmware managed resources

[Krzysztof Kozlowski]

On 05/03/2024 17:57, Sriram Dash wrote:
> Establish the channel and domain mapping for the power domains to connect
> with firmware, enabling the firmware to handle the assigned resources.
> Since these delegated resources will remain invisible to the operating
> system, ensure that any references to them are removed.
> 
> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 96 +++++++++++++++++++++++++------
>  1 file changed, 77 insertions(+), 19 deletions(-)

Do not mix DTS patches with submissions to netdev or USB.

Please put it inside your internal guides, so you will not be repeating
this over and over.

> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> index 26ad05b..b6c9cac 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
> @@ -764,8 +764,18 @@
>  };
>  
>  &usb_0 {
> -	pinctrl-names = "default";
> -	pinctrl-0 = <&usb0_en_state>;
> +	/delete-property/ clocks;
> +	/delete-property/ clock-names;
> +	/delete-property/ assigned-clocks;
> +	/delete-property/ assigned-clock-rates;
> +	/delete-property/ required-opps;
> +	/delete-property/ resets;
> +	/delete-property/ interconnects;
> +	/delete-property/ interconnect-names;
> +
> +	power-domains = <TODO>, <TODO>;

This wasn't even tested.

Best regards,
Krzysztof

Re: [RFC 3/3] arm64: dts: qcom: sa8775p-ride: Enable support for firmware managed resources

[Sriram Dash]

On 3/5/2024 11:33 PM, Sriram Dash wrote:
> On 3/5/2024 10:38 PM, Krzysztof Kozlowski wrote:
>> On 05/03/2024 17:57, Sriram Dash wrote:
>>> Establish the channel and domain mapping for the power domains to 
>>> connect
>>> with firmware, enabling the firmware to handle the assigned resources.
>>> Since these delegated resources will remain invisible to the operating
>>> system, ensure that any references to them are removed.
>>>
>>> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 96 
>>> +++++++++++++++++++++++++------
>>>   1 file changed, 77 insertions(+), 19 deletions(-)
>>
>> Do not mix DTS patches with submissions to netdev or USB.
>>
>> Please put it inside your internal guides, so you will not be repeating
>> this over and over.
>>
> 
> Sure. Will take care. Thanks.
> 
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts 
>>> b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
>>> index 26ad05b..b6c9cac 100644
>>> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
>>> @@ -764,8 +764,18 @@
>>>   };
>>>   &usb_0 {
>>> -    pinctrl-names = "default";
>>> -    pinctrl-0 = <&usb0_en_state>;
>>> +    /delete-property/ clocks;
>>> +    /delete-property/ clock-names;
>>> +    /delete-property/ assigned-clocks;
>>> +    /delete-property/ assigned-clock-rates;
>>> +    /delete-property/ required-opps;
>>> +    /delete-property/ resets;
>>> +    /delete-property/ interconnects;
>>> +    /delete-property/ interconnect-names;
>>> +
>>> +    power-domains = <TODO>, <TODO>;
>>
>> This wasn't even tested.
>>
> 
> This is tested with the specific power domains in place
> of <TODO>. SCMI interface is used for the power domains.
> 
>> Best regards,
>> Krzysztof
>>

Re: [RFC 3/3] arm64: dts: qcom: sa8775p-ride: Enable support for firmware managed resources

[Konrad Dybcio]

On 3/5/24 19:25, Sriram Dash wrote:
> On 3/5/2024 11:33 PM, Sriram Dash wrote:
>> On 3/5/2024 10:38 PM, Krzysztof Kozlowski wrote:
>>> On 05/03/2024 17:57, Sriram Dash wrote:
>>>> Establish the channel and domain mapping for the power domains to connect
>>>> with firmware, enabling the firmware to handle the assigned resources.
>>>> Since these delegated resources will remain invisible to the operating
>>>> system, ensure that any references to them are removed.
>>>>
>>>> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
>>>> ---
>>>>   arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 96 +++++++++++++++++++++++++------
>>>>   1 file changed, 77 insertions(+), 19 deletions(-)
>>>
>>> Do not mix DTS patches with submissions to netdev or USB.
>>>
>>> Please put it inside your internal guides, so you will not be repeating
>>> this over and over.
>>>
>>
>> Sure. Will take care. Thanks.
>>
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
>>>> index 26ad05b..b6c9cac 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
>>>> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
>>>> @@ -764,8 +764,18 @@
>>>>   };
>>>>   &usb_0 {
>>>> -    pinctrl-names = "default";
>>>> -    pinctrl-0 = <&usb0_en_state>;
>>>> +    /delete-property/ clocks;
>>>> +    /delete-property/ clock-names;
>>>> +    /delete-property/ assigned-clocks;
>>>> +    /delete-property/ assigned-clock-rates;
>>>> +    /delete-property/ required-opps;
>>>> +    /delete-property/ resets;
>>>> +    /delete-property/ interconnects;
>>>> +    /delete-property/ interconnect-names;

This is totally unacceptable.. And especially makes no sense given
ride is likely the only sa8775 board in existence..

>>>> +
>>>> +    power-domains = <TODO>, <TODO>;
>>>
>>> This wasn't even tested.
>>>
>>
>> This is tested with the specific power domains in place
>> of <TODO>. SCMI interface is used for the power domains.

I'm sorry, but "break compilation successfully" is not a valid
test result..

Konrad

Re: [RFC 3/3] arm64: dts: qcom: sa8775p-ride: Enable support for firmware managed resources

[Sriram Dash]

On 3/6/2024 2:37 AM, Konrad Dybcio wrote:
> 
> 
> On 3/5/24 19:25, Sriram Dash wrote:
>> On 3/5/2024 11:33 PM, Sriram Dash wrote:
>>> On 3/5/2024 10:38 PM, Krzysztof Kozlowski wrote:
>>>> On 05/03/2024 17:57, Sriram Dash wrote:
>>>>> Establish the channel and domain mapping for the power domains to 
>>>>> connect
>>>>> with firmware, enabling the firmware to handle the assigned resources.
>>>>> Since these delegated resources will remain invisible to the operating
>>>>> system, ensure that any references to them are removed.
>>>>>
>>>>> Signed-off-by: Sriram Dash <quic_sriramd@quicinc.com>
>>>>> ---
>>>>>   arch/arm64/boot/dts/qcom/sa8775p-ride.dts | 96 
>>>>> +++++++++++++++++++++++++------
>>>>>   1 file changed, 77 insertions(+), 19 deletions(-)
>>>>
>>>> Do not mix DTS patches with submissions to netdev or USB.
>>>>
>>>> Please put it inside your internal guides, so you will not be repeating
>>>> this over and over.
>>>>
>>>
>>> Sure. Will take care. Thanks.
>>>
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts 
>>>>> b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
>>>>> index 26ad05b..b6c9cac 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
>>>>> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
>>>>> @@ -764,8 +764,18 @@
>>>>>   };
>>>>>   &usb_0 {
>>>>> -    pinctrl-names = "default";
>>>>> -    pinctrl-0 = <&usb0_en_state>;
>>>>> +    /delete-property/ clocks;
>>>>> +    /delete-property/ clock-names;
>>>>> +    /delete-property/ assigned-clocks;
>>>>> +    /delete-property/ assigned-clock-rates;
>>>>> +    /delete-property/ required-opps;
>>>>> +    /delete-property/ resets;
>>>>> +    /delete-property/ interconnects;
>>>>> +    /delete-property/ interconnect-names;
> 
> This is totally unacceptable.. And especially makes no sense given
> ride is likely the only sa8775 board in existence..
> 

Sure Konrad.
Will update the patches on top of the scmi based dt
for sa8775.

>>>>> +
>>>>> +    power-domains = <TODO>, <TODO>;
>>>>
>>>> This wasn't even tested.
>>>>
>>>
>>> This is tested with the specific power domains in place
>>> of <TODO>. SCMI interface is used for the power domains.
> 
> I'm sorry, but "break compilation successfully" is not a valid
> test result..
> 
> Konrad

Re: [RFC 0/3] Enable firmware-managed USB resources on Qcom targets

[Krzysztof Kozlowski]

On 05/03/2024 17:57, Sriram Dash wrote:
> Some target systems allow multiple resources to be managed by firmware.

Which? Why this is so vague...

> On these targets, tasks related to clocks, regulators, resets, and
> interconnects can be delegated to the firmware, while the remaining
> responsibilities are handled by Linux.
> 
> To support the management of partial resources in Linux and leave the rest
> to firmware, multiple power domains are introduced. Each power domain can
> manage one or more resources, depending on the specific use case.
> 
> These power domains handle SCMI calls to the firmware, enabling the
> activation and deactivation of firmware-managed resources.
> 
> The driver is responsible for managing multiple power domains and
> linking them to consumers as needed. Incase there is only single
> power domain, it is considered to be a standard GDSC hooked on to
> the qcom dt node which is read and assigned to device structure
> (by genpd framework) before the driver probe even begins.

This will break the ABI. Sorry, come with an ABI stable solution.

Best regards,
Krzysztof

Re: [RFC 0/3] Enable firmware-managed USB resources on Qcom targets

[Sriram Dash]

On 3/5/2024 10:42 PM, Krzysztof Kozlowski wrote:
> On 05/03/2024 17:57, Sriram Dash wrote:
>> Some target systems allow multiple resources to be managed by firmware.
> 
> Which? Why this is so vague...
> 

SA8775 will be using it as pilot. Will include the target name.

>> On these targets, tasks related to clocks, regulators, resets, and
>> interconnects can be delegated to the firmware, while the remaining
>> responsibilities are handled by Linux.
>>
>> To support the management of partial resources in Linux and leave the rest
>> to firmware, multiple power domains are introduced. Each power domain can
>> manage one or more resources, depending on the specific use case.
>>
>> These power domains handle SCMI calls to the firmware, enabling the
>> activation and deactivation of firmware-managed resources.
>>
>> The driver is responsible for managing multiple power domains and
>> linking them to consumers as needed. Incase there is only single
>> power domain, it is considered to be a standard GDSC hooked on to
>> the qcom dt node which is read and assigned to device structure
>> (by genpd framework) before the driver probe even begins.
> 
> This will break the ABI. Sorry, come with an ABI stable solution.
> 

The plan is to include multiple power-domains and fw-managed
property or similar in the device tree and fw-managed property
will be deciding if we need some resource management offloaded
to firmware. So, OS is always in control here. The decision
making will be done in the drivers. Also, there will be no
separate vendor hooks.

> Best regards,
> Krzysztof
> 

Re: [RFC 0/3] Enable firmware-managed USB resources on Qcom targets

[Krzysztof Kozlowski]

On 05/03/2024 19:04, Sriram Dash wrote:
> On 3/5/2024 10:42 PM, Krzysztof Kozlowski wrote:
>> On 05/03/2024 17:57, Sriram Dash wrote:
>>> Some target systems allow multiple resources to be managed by firmware.
>>
>> Which? Why this is so vague...
>>
> 
> SA8775 will be using it as pilot. Will include the target name.
> 
>>> On these targets, tasks related to clocks, regulators, resets, and
>>> interconnects can be delegated to the firmware, while the remaining
>>> responsibilities are handled by Linux.
>>>
>>> To support the management of partial resources in Linux and leave the rest
>>> to firmware, multiple power domains are introduced. Each power domain can
>>> manage one or more resources, depending on the specific use case.
>>>
>>> These power domains handle SCMI calls to the firmware, enabling the
>>> activation and deactivation of firmware-managed resources.
>>>
>>> The driver is responsible for managing multiple power domains and
>>> linking them to consumers as needed. Incase there is only single
>>> power domain, it is considered to be a standard GDSC hooked on to
>>> the qcom dt node which is read and assigned to device structure
>>> (by genpd framework) before the driver probe even begins.
>>
>> This will break the ABI. Sorry, come with an ABI stable solution.
>>
> 
> The plan is to include multiple power-domains and fw-managed
> property or similar in the device tree and fw-managed property
> will be deciding if we need some resource management offloaded
> to firmware. So, OS is always in control here. The decision
> making will be done in the drivers. Also, there will be no
> separate vendor hooks.

This does not answer ABI breakage. Also, I don't have a clue what are
"vendor hooks".

Best regards,
Krzysztof

Re: [RFC 0/3] Enable firmware-managed USB resources on Qcom targets

[Sriram Dash]

On 3/6/2024 12:34 PM, Krzysztof Kozlowski wrote:
> On 05/03/2024 19:04, Sriram Dash wrote:
>> On 3/5/2024 10:42 PM, Krzysztof Kozlowski wrote:
>>> On 05/03/2024 17:57, Sriram Dash wrote:
>>>> Some target systems allow multiple resources to be managed by firmware.
>>>
>>> Which? Why this is so vague...
>>>
>>
>> SA8775 will be using it as pilot. Will include the target name.
>>
>>>> On these targets, tasks related to clocks, regulators, resets, and
>>>> interconnects can be delegated to the firmware, while the remaining
>>>> responsibilities are handled by Linux.
>>>>
>>>> To support the management of partial resources in Linux and leave the rest
>>>> to firmware, multiple power domains are introduced. Each power domain can
>>>> manage one or more resources, depending on the specific use case.
>>>>
>>>> These power domains handle SCMI calls to the firmware, enabling the
>>>> activation and deactivation of firmware-managed resources.
>>>>
>>>> The driver is responsible for managing multiple power domains and
>>>> linking them to consumers as needed. Incase there is only single
>>>> power domain, it is considered to be a standard GDSC hooked on to
>>>> the qcom dt node which is read and assigned to device structure
>>>> (by genpd framework) before the driver probe even begins.
>>>
>>> This will break the ABI. Sorry, come with an ABI stable solution.
>>>
>>
>> The plan is to include multiple power-domains and fw-managed
>> property or similar in the device tree and fw-managed property
>> will be deciding if we need some resource management offloaded
>> to firmware. So, OS is always in control here. The decision
>> making will be done in the drivers. Also, there will be no
>> separate vendor hooks.
> 
> This does not answer ABI breakage. Also, I don't have a clue what are
> "vendor hooks".
> 

Apologies for the confusion, Krysztof.
The bindings will depict whether the compatible will use
clocks/ regulators, etc or not. Will take care in the
next version on top of the scmi based dt solution.

> Best regards,
> Krzysztof
> 

Re: [RFC 0/3] Enable firmware-managed USB resources on Qcom targets

[Konrad Dybcio]

On 3/5/24 17:57, Sriram Dash wrote:
> Some target systems allow multiple resources to be managed by firmware.
> On these targets, tasks related to clocks, regulators, resets, and
> interconnects can be delegated to the firmware, while the remaining
> responsibilities are handled by Linux.

Aside from the comments you already got from others, please change
[RFC m/n] to [PATCH RFC m/n] so that your series isn't filtered out
out maintainers' inboxes due to the missing PATCH keyword..

Konrad

Re: [RFC 0/3] Enable firmware-managed USB resources on Qcom targets

[Sriram Dash]

On 3/6/2024 2:39 AM, Konrad Dybcio wrote:
> 
> 
> On 3/5/24 17:57, Sriram Dash wrote:
>> Some target systems allow multiple resources to be managed by firmware.
>> On these targets, tasks related to clocks, regulators, resets, and
>> interconnects can be delegated to the firmware, while the remaining
>> responsibilities are handled by Linux.
> 
> Aside from the comments you already got from others, please change
> [RFC m/n] to [PATCH RFC m/n] so that your series isn't filtered out
> out maintainers' inboxes due to the missing PATCH keyword..
> 

Sure. thanks Konrad.

> Konrad

Re: [RFC 0/3] Enable firmware-managed USB resources on Qcom targets

[Trilok Soni]

On 3/5/2024 8:57 AM, Sriram Dash wrote:
> Some target systems allow multiple resources to be managed by firmware.
> On these targets, tasks related to clocks, regulators, resets, and
> interconnects can be delegated to the firmware, while the remaining
> responsibilities are handled by Linux.

What is some target? What does "target" means? 

-- 
---Trilok Soni

Re: [RFC 0/3] Enable firmware-managed USB resources on Qcom targets

[Sudeep Holla]

On Tue, Mar 05, 2024 at 10:27:35PM +0530, Sriram Dash wrote:
> Some target systems allow multiple resources to be managed by firmware.
> On these targets, tasks related to clocks, regulators, resets, and
> interconnects can be delegated to the firmware, while the remaining
> responsibilities are handled by Linux.
>
> To support the management of partial resources in Linux and leave the rest
> to firmware, multiple power domains are introduced. Each power domain can
> manage one or more resources, depending on the specific use case.
>

Currently it is just 2 IIUC. Better to be specific with more details and
point to the exact binding.

> These power domains handle SCMI calls to the firmware, enabling the
> activation and deactivation of firmware-managed resources.
>
> The driver is responsible for managing multiple power domains and
> linking them to consumers as needed. Incase there is only single
> power domain, it is considered to be a standard GDSC hooked on to
> the qcom dt node which is read and assigned to device structure
> (by genpd framework) before the driver probe even begins.
>
> fw-managed dt property allows the driver to determine whether
> device resources are managed by Linux or firmware, ensuring
> backward compatibility.
>

And provide the reason why this additional property is a must ? Why can't
the implementation deal with absence of it on these systems ?

Not sure if you have seen/followed this[1] discussion before, but please
do now if not already and contribute. It is definitely related to this
patch set and all possible very similar patch sets Qcom might have in the
future across various subsystems in the Linux kernel.

In general, Qcom must stop pushing such changes to individual drivers
in isolation and confuse the reviewers to some extent without giving
the complete view(or rather providing partial view) which may result in
them agreeing with proposed solution without considering the overall
impact on various subsystems and DT bindings.

--
Regards,
Sudeep

[1] https://lore.kernel.org/all/be31801e-bb21-426b-f7aa-2b52727de646@quicinc.com/