diff mbox series

[net-next,1/5] dt-bindings: net: qcom,ipa: deprecate modem-init

Message ID 20221112200717.1533622-2-elder@linaro.org
State New
Headers
Series net: ipa: change GSI firmware load specification |

Commit Message

Alex Elder Nov. 12, 2022, 8:07 p.m. UTC 
  GSI firmware for IPA must be loaded during initialization, either by
the AP or by the modem.  The loader is currently specified based on
whether the Boolean modem-init property is present.

Instead, use a new property with an enumerated value to indicate
explicitly how GSI firmware gets loaded.  With this in place, a
third approach can be added in an upcoming patch.

The new qcom,gsi-loader property has two defined values:
  - self:   The AP loads GSI firmware
  - modem:  The modem loads GSI firmware
The modem-init property must still be supported, but is now marked
deprecated.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 .../devicetree/bindings/net/qcom,ipa.yaml     | 59 +++++++++++++++----
 1 file changed, 46 insertions(+), 13 deletions(-)

Comments

Krzysztof Kozlowski Nov. 14, 2022, 9:56 a.m. UTC | #1 
On 12/11/2022 21:07, Alex Elder wrote:
> GSI firmware for IPA must be loaded during initialization, either by
> the AP or by the modem.  The loader is currently specified based on
> whether the Boolean modem-init property is present.
> 
> Instead, use a new property with an enumerated value to indicate
> explicitly how GSI firmware gets loaded.  With this in place, a
> third approach can be added in an upcoming patch.
> 
> The new qcom,gsi-loader property has two defined values:
>   - self:   The AP loads GSI firmware
>   - modem:  The modem loads GSI firmware
> The modem-init property must still be supported, but is now marked
> deprecated.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  .../devicetree/bindings/net/qcom,ipa.yaml     | 59 +++++++++++++++----
>  1 file changed, 46 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> index e752b76192df0..0dfd6c721e045 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
> @@ -124,12 +124,22 @@ properties:
>        - const: ipa-clock-enabled-valid
>        - const: ipa-clock-enabled
>  
> +  qcom,gsi-loader:
> +    enum:
> +      - self
> +      - modem
> +    description:
> +      This indicates how GSI firmware should be loaded.  If the AP loads

s/This indicates/Indicate/
(or any other grammar without describing DT syntax but hardware/system)

> +      and validates GSI firmware, this property has value "self".  If the
> +      modem does this, this property has value "modem".
> +
>    modem-init:
> +    deprecated: true
>      type: boolean
>      description:
> -      If present, it indicates that the modem is responsible for
> -      performing early IPA initialization, including loading and
> -      validating firwmare used by the GSI.
> +      This is the older (deprecated) way of indicating how GSI firmware
> +      should be loaded.  If present, the modem loads GSI firmware; if
> +      absent, the AP loads GSI firmware.
>  
>    memory-region:
>      maxItems: 1
> @@ -155,15 +165,36 @@ required:
>    - interconnects
>    - qcom,smem-states
>  
> -# If modem-init is not present, the AP loads GSI firmware, and
> -# memory-region must be specified
> -if:
> -  not:
> -    required:
> -      - modem-init
> -then:
> -  required:
> -    - memory-region
> +allOf:
> +  # If qcom,gsi-loader is present, modem-init must not be present
> +  - if:
> +      required:
> +        - qcom,gsi-loader
> +    then:
> +      properties:
> +        modem-init: false

This is ok, but will not allow you to keep deprecated property in DTS
for the transition period. We talked about this that you need to keep
both or wait few cycles before applying DTS cleanups.

> +
> +      # If qcom,gsi-loader is "self", the AP loads GSI firmware, and
> +      # memory-region must be specified
> +      if:
> +        properties:
> +          qcom,gsi-loader:
> +            contains:
> +              const: self
> +      then:
> +        required:
> +          - memory-region
> +    else:
> +      # If qcom,gsi-loader is not present, we use deprecated behavior.
> +      # If modem-init is not present, the AP loads GSI firmware, and
> +      # memory-region must be specified.
> +      if:
> +        not:
> +          required:
> +            - modem-init
> +      then:
> +        required:
> +          - memory-region
>  
>  additionalProperties: false
>  
> @@ -196,7 +227,9 @@ examples:
>          ipa@1e40000 {
>                  compatible = "qcom,sdm845-ipa";
>  
> -                modem-init;
> +                qcom,gsi-loader = "self";
> +                memory-region = <&ipa_fw_mem>;
> +                firmware-name = "qcom/sc7180-trogdor/modem-nolte/mba.mbn";

Isn't this example based on sdm845?

>  
>                  iommus = <&apps_smmu 0x720 0x3>;
>                  reg = <0x1e40000 0x7000>,

Best regards,
Krzysztof
Alex Elder Nov. 14, 2022, 5:48 p.m. UTC | #2 
On 11/14/22 03:56, Krzysztof Kozlowski wrote:
> On 12/11/2022 21:07, Alex Elder wrote:
>> GSI firmware for IPA must be loaded during initialization, either by
>> the AP or by the modem.  The loader is currently specified based on
>> whether the Boolean modem-init property is present.
>>
>> Instead, use a new property with an enumerated value to indicate
>> explicitly how GSI firmware gets loaded.  With this in place, a
>> third approach can be added in an upcoming patch.
>>
>> The new qcom,gsi-loader property has two defined values:
>>    - self:   The AP loads GSI firmware
>>    - modem:  The modem loads GSI firmware
>> The modem-init property must still be supported, but is now marked
>> deprecated.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>   .../devicetree/bindings/net/qcom,ipa.yaml     | 59 +++++++++++++++----
>>   1 file changed, 46 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>> index e752b76192df0..0dfd6c721e045 100644
>> --- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>> +++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>> @@ -124,12 +124,22 @@ properties:
>>         - const: ipa-clock-enabled-valid
>>         - const: ipa-clock-enabled
>>   
>> +  qcom,gsi-loader:
>> +    enum:
>> +      - self
>> +      - modem
>> +    description:
>> +      This indicates how GSI firmware should be loaded.  If the AP loads
> 
> s/This indicates/Indicate/
> (or any other grammar without describing DT syntax but hardware/system)

OK.

>> +      and validates GSI firmware, this property has value "self".  If the
>> +      modem does this, this property has value "modem".
>> +
>>     modem-init:
>> +    deprecated: true
>>       type: boolean
>>       description:
>> -      If present, it indicates that the modem is responsible for
>> -      performing early IPA initialization, including loading and
>> -      validating firwmare used by the GSI.
>> +      This is the older (deprecated) way of indicating how GSI firmware
>> +      should be loaded.  If present, the modem loads GSI firmware; if
>> +      absent, the AP loads GSI firmware.
>>   
>>     memory-region:
>>       maxItems: 1
>> @@ -155,15 +165,36 @@ required:
>>     - interconnects
>>     - qcom,smem-states
>>   
>> -# If modem-init is not present, the AP loads GSI firmware, and
>> -# memory-region must be specified
>> -if:
>> -  not:
>> -    required:
>> -      - modem-init
>> -then:
>> -  required:
>> -    - memory-region
>> +allOf:
>> +  # If qcom,gsi-loader is present, modem-init must not be present
>> +  - if:
>> +      required:
>> +        - qcom,gsi-loader
>> +    then:
>> +      properties:
>> +        modem-init: false
> 
> This is ok, but will not allow you to keep deprecated property in DTS
> for the transition period. We talked about this that you need to keep
> both or wait few cycles before applying DTS cleanups.

My intention is expressed in the comment.  Is it because of the
"if .... required ... qcom,gsi-loader"?

Should it be "if ... properties ... qcom,gsi-loader"?

I believe I tested the deprecated cases also, but will do it again.
If I coded it wrong I'd like to fix it (even if "it works").

>> +
>> +      # If qcom,gsi-loader is "self", the AP loads GSI firmware, and
>> +      # memory-region must be specified
>> +      if:
>> +        properties:
>> +          qcom,gsi-loader:
>> +            contains:
>> +              const: self
>> +      then:
>> +        required:
>> +          - memory-region
>> +    else:
>> +      # If qcom,gsi-loader is not present, we use deprecated behavior.
>> +      # If modem-init is not present, the AP loads GSI firmware, and
>> +      # memory-region must be specified.
>> +      if:
>> +        not:
>> +          required:
>> +            - modem-init
>> +      then:
>> +        required:
>> +          - memory-region
>>   
>>   additionalProperties: false
>>   
>> @@ -196,7 +227,9 @@ examples:
>>           ipa@1e40000 {
>>                   compatible = "qcom,sdm845-ipa";
>>   
>> -                modem-init;
>> +                qcom,gsi-loader = "self";
>> +                memory-region = <&ipa_fw_mem>;
>> +                firmware-name = "qcom/sc7180-trogdor/modem-nolte/mba.mbn";
> 
> Isn't this example based on sdm845?

Yes, you're right.  I'll update it.  I just wanted to supply an
example that actually showed using all these properties.  I'll
make the whole example be for sc7180.

Thank you.

					-Alex

> 
>>   
>>                   iommus = <&apps_smmu 0x720 0x3>;
>>                   reg = <0x1e40000 0x7000>,
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Nov. 15, 2022, 7:59 a.m. UTC | #3 
On 14/11/2022 18:48, Alex Elder wrote:
> On 11/14/22 03:56, Krzysztof Kozlowski wrote:
>> On 12/11/2022 21:07, Alex Elder wrote:
>>> GSI firmware for IPA must be loaded during initialization, either by
>>> the AP or by the modem.  The loader is currently specified based on
>>> whether the Boolean modem-init property is present.
>>>
>>> Instead, use a new property with an enumerated value to indicate
>>> explicitly how GSI firmware gets loaded.  With this in place, a
>>> third approach can be added in an upcoming patch.
>>>
>>> The new qcom,gsi-loader property has two defined values:
>>>    - self:   The AP loads GSI firmware
>>>    - modem:  The modem loads GSI firmware
>>> The modem-init property must still be supported, but is now marked
>>> deprecated.
>>>
>>> Signed-off-by: Alex Elder <elder@linaro.org>
>>> ---
>>>   .../devicetree/bindings/net/qcom,ipa.yaml     | 59 +++++++++++++++----
>>>   1 file changed, 46 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>>> index e752b76192df0..0dfd6c721e045 100644
>>> --- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>>> @@ -124,12 +124,22 @@ properties:
>>>         - const: ipa-clock-enabled-valid
>>>         - const: ipa-clock-enabled
>>>   
>>> +  qcom,gsi-loader:
>>> +    enum:
>>> +      - self
>>> +      - modem
>>> +    description:
>>> +      This indicates how GSI firmware should be loaded.  If the AP loads
>>
>> s/This indicates/Indicate/
>> (or any other grammar without describing DT syntax but hardware/system)
> 
> OK.
> 
>>> +      and validates GSI firmware, this property has value "self".  If the
>>> +      modem does this, this property has value "modem".
>>> +
>>>     modem-init:
>>> +    deprecated: true
>>>       type: boolean
>>>       description:
>>> -      If present, it indicates that the modem is responsible for
>>> -      performing early IPA initialization, including loading and
>>> -      validating firwmare used by the GSI.
>>> +      This is the older (deprecated) way of indicating how GSI firmware
>>> +      should be loaded.  If present, the modem loads GSI firmware; if
>>> +      absent, the AP loads GSI firmware.
>>>   
>>>     memory-region:
>>>       maxItems: 1
>>> @@ -155,15 +165,36 @@ required:
>>>     - interconnects
>>>     - qcom,smem-states
>>>   
>>> -# If modem-init is not present, the AP loads GSI firmware, and
>>> -# memory-region must be specified
>>> -if:
>>> -  not:
>>> -    required:
>>> -      - modem-init
>>> -then:
>>> -  required:
>>> -    - memory-region
>>> +allOf:
>>> +  # If qcom,gsi-loader is present, modem-init must not be present
>>> +  - if:
>>> +      required:
>>> +        - qcom,gsi-loader
>>> +    then:
>>> +      properties:
>>> +        modem-init: false
>>
>> This is ok, but will not allow you to keep deprecated property in DTS
>> for the transition period. We talked about this that you need to keep
>> both or wait few cycles before applying DTS cleanups.
> 
> My intention is expressed in the comment.  Is it because of the
> "if .... required ... qcom,gsi-loader"?
> 
> Should it be "if ... properties ... qcom,gsi-loader"?

You disallow modem-init here, so it cannot be present in DTS if
gsi-loader is present. Therefore the deprecated case like this:
  qcom,gsi-loader = "modem"
  modem-init;
is not allowed by the schema.

As I said, it is fine, but your DTS should wait a cycle.


Best regards,
Krzysztof
Alex Elder Nov. 15, 2022, 9 a.m. UTC | #4 
On 11/15/22 01:59, Krzysztof Kozlowski wrote:
> On 14/11/2022 18:48, Alex Elder wrote:
>> On 11/14/22 03:56, Krzysztof Kozlowski wrote:
>>> On 12/11/2022 21:07, Alex Elder wrote:
>>>> GSI firmware for IPA must be loaded during initialization, either by
>>>> the AP or by the modem.  The loader is currently specified based on
>>>> whether the Boolean modem-init property is present.
>>>>
>>>> Instead, use a new property with an enumerated value to indicate
>>>> explicitly how GSI firmware gets loaded.  With this in place, a
>>>> third approach can be added in an upcoming patch.
>>>>
>>>> The new qcom,gsi-loader property has two defined values:
>>>>     - self:   The AP loads GSI firmware
>>>>     - modem:  The modem loads GSI firmware
>>>> The modem-init property must still be supported, but is now marked
>>>> deprecated.
>>>>
>>>> Signed-off-by: Alex Elder <elder@linaro.org>
>>>> ---
>>>>    .../devicetree/bindings/net/qcom,ipa.yaml     | 59 +++++++++++++++----
>>>>    1 file changed, 46 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>>>> index e752b76192df0..0dfd6c721e045 100644
>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
>>>> @@ -124,12 +124,22 @@ properties:
>>>>          - const: ipa-clock-enabled-valid
>>>>          - const: ipa-clock-enabled
>>>>    
>>>> +  qcom,gsi-loader:
>>>> +    enum:
>>>> +      - self
>>>> +      - modem
>>>> +    description:
>>>> +      This indicates how GSI firmware should be loaded.  If the AP loads
>>>
>>> s/This indicates/Indicate/
>>> (or any other grammar without describing DT syntax but hardware/system)
>>
>> OK.
>>
>>>> +      and validates GSI firmware, this property has value "self".  If the
>>>> +      modem does this, this property has value "modem".
>>>> +
>>>>      modem-init:
>>>> +    deprecated: true
>>>>        type: boolean
>>>>        description:
>>>> -      If present, it indicates that the modem is responsible for
>>>> -      performing early IPA initialization, including loading and
>>>> -      validating firwmare used by the GSI.
>>>> +      This is the older (deprecated) way of indicating how GSI firmware
>>>> +      should be loaded.  If present, the modem loads GSI firmware; if
>>>> +      absent, the AP loads GSI firmware.
>>>>    
>>>>      memory-region:
>>>>        maxItems: 1
>>>> @@ -155,15 +165,36 @@ required:
>>>>      - interconnects
>>>>      - qcom,smem-states
>>>>    
>>>> -# If modem-init is not present, the AP loads GSI firmware, and
>>>> -# memory-region must be specified
>>>> -if:
>>>> -  not:
>>>> -    required:
>>>> -      - modem-init
>>>> -then:
>>>> -  required:
>>>> -    - memory-region
>>>> +allOf:
>>>> +  # If qcom,gsi-loader is present, modem-init must not be present
>>>> +  - if:
>>>> +      required:
>>>> +        - qcom,gsi-loader
>>>> +    then:
>>>> +      properties:
>>>> +        modem-init: false
>>>
>>> This is ok, but will not allow you to keep deprecated property in DTS
>>> for the transition period. We talked about this that you need to keep
>>> both or wait few cycles before applying DTS cleanups.
>>
>> My intention is expressed in the comment.  Is it because of the
>> "if .... required ... qcom,gsi-loader"?
>>
>> Should it be "if ... properties ... qcom,gsi-loader"?
> 
> You disallow modem-init here, so it cannot be present in DTS if
> gsi-loader is present. Therefore the deprecated case like this:
>    qcom,gsi-loader = "modem"
>    modem-init;
> is not allowed by the schema.
> 
> As I said, it is fine, but your DTS should wait a cycle.

OK, then this is exactly as I intended.  I am planning to wait
until Linux v6.2-rc1 is published before I post the DTS updates
that implement this change.  It is not technically necessary
until IPA v5.0 is fully supported, and I don't have confidence
all of that will accepted before then.

If I did it "your way" first I could get it done now, but then
I'd want to do another round later to make it this way.

I will still send an updated series shortly, to address your
other comment about wording in the description.  But I will
not be changing this part.

Thanks for the explanation Krzysztof.

					-Alex


> 
> 
> Best regards,
> Krzysztof
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
index e752b76192df0..0dfd6c721e045 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
@@ -124,12 +124,22 @@  properties:
       - const: ipa-clock-enabled-valid
       - const: ipa-clock-enabled
 
+  qcom,gsi-loader:
+    enum:
+      - self
+      - modem
+    description:
+      This indicates how GSI firmware should be loaded.  If the AP loads
+      and validates GSI firmware, this property has value "self".  If the
+      modem does this, this property has value "modem".
+
   modem-init:
+    deprecated: true
     type: boolean
     description:
-      If present, it indicates that the modem is responsible for
-      performing early IPA initialization, including loading and
-      validating firwmare used by the GSI.
+      This is the older (deprecated) way of indicating how GSI firmware
+      should be loaded.  If present, the modem loads GSI firmware; if
+      absent, the AP loads GSI firmware.
 
   memory-region:
     maxItems: 1
@@ -155,15 +165,36 @@  required:
   - interconnects
   - qcom,smem-states
 
-# If modem-init is not present, the AP loads GSI firmware, and
-# memory-region must be specified
-if:
-  not:
-    required:
-      - modem-init
-then:
-  required:
-    - memory-region
+allOf:
+  # If qcom,gsi-loader is present, modem-init must not be present
+  - if:
+      required:
+        - qcom,gsi-loader
+    then:
+      properties:
+        modem-init: false
+
+      # If qcom,gsi-loader is "self", the AP loads GSI firmware, and
+      # memory-region must be specified
+      if:
+        properties:
+          qcom,gsi-loader:
+            contains:
+              const: self
+      then:
+        required:
+          - memory-region
+    else:
+      # If qcom,gsi-loader is not present, we use deprecated behavior.
+      # If modem-init is not present, the AP loads GSI firmware, and
+      # memory-region must be specified.
+      if:
+        not:
+          required:
+            - modem-init
+      then:
+        required:
+          - memory-region
 
 additionalProperties: false
 
@@ -196,7 +227,9 @@  examples:
         ipa@1e40000 {
                 compatible = "qcom,sdm845-ipa";
 
-                modem-init;
+                qcom,gsi-loader = "self";
+                memory-region = <&ipa_fw_mem>;
+                firmware-name = "qcom/sc7180-trogdor/modem-nolte/mba.mbn";
 
                 iommus = <&apps_smmu 0x720 0x3>;
                 reg = <0x1e40000 0x7000>,