[PATCH net-next 0/5] net: ipa: change GSI firmware load specification

[PATCH net-next 0/5] net: ipa: change GSI firmware load specification

[Alex Elder]

Currently, GSI firmware must be loaded for IPA before it can be
used--either by the modem, or by the AP.  New hardware supports a
third option, with the bootloader taking responsibility for loading
GSI firmware.  In that case, neither the AP nor the modem needs to
do that.

The first patch in this series deprecates the "modem-init" Device
Tree property in the IPA binding, using a new "qcom,gsi-loader"
property instead.  The second and third implement logic in the code
to support either the "old" or the "new" way of specifying how GSI
firmware is loaded.

The last two patches implement a new value for the "qcom,gsi-loader"
property.  If the value is "skip", neither the AP nor modem needs to
load the GSI firmware.  The first of these patches implements the
change in the IPA binding; the second implements it in the code.

					-Alex

Alex Elder (5):
  dt-bindings: net: qcom,ipa: deprecate modem-init
  net: ipa: encapsulate decision about firmware load
  net: ipa: introduce "qcom,gsi-loader" property
  dt-bindings: net: qcom,ipa: support skipping GSI firmware load
  net: ipa: permit GSI firmware loading to be skipped

 .../devicetree/bindings/net/qcom,ipa.yaml     | 61 +++++++++---
 drivers/net/ipa/ipa_main.c                    | 95 +++++++++++++++----
 2 files changed, 126 insertions(+), 30 deletions(-)

-- 
2.34.1

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

[Alex Elder]

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
+      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>,
-- 
2.34.1

[PATCH net-next 2/5] net: ipa: encapsulate decision about firmware load

[Alex Elder]

The GSI layer used for IPA requires firmware to be loaded.

Currently either the AP or the modem loads the firmware,
distinguished by whether the "modem-init" Device Tree
property is defined.

Some newer systems implement a third option.  In preparation for
that, encapsulate the code that determines how the GSI firmware
gets loaded in a new function, ipa_firmware_loader().

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_main.c | 39 ++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index ebb6c9b311eb9..9e43b79d233e9 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -81,6 +81,19 @@
 /* Divider for 19.2 MHz crystal oscillator clock to get common timer clock */
 #define IPA_XO_CLOCK_DIVIDER	192	/* 1 is subtracted where used */
 
+/**
+ * enum ipa_firmware_loader: How GSI firmware gets loaded
+ *
+ * @IPA_LOADER_DEFER:		System not ready; try again later
+ * @IPA_LOADER_SELF:		AP loads GSI firmware
+ * @IPA_LOADER_MODEM:		Modem loads GSI firmware, signals when done
+ */
+enum ipa_firmware_loader {
+	IPA_LOADER_DEFER,
+	IPA_LOADER_SELF,
+	IPA_LOADER_MODEM,
+};
+
 /**
  * ipa_setup() - Set up IPA hardware
  * @ipa:	IPA pointer
@@ -696,6 +709,18 @@ static void ipa_validate_build(void)
 	BUILD_BUG_ON(!ipa_aggr_granularity_val(IPA_AGGR_GRANULARITY));
 }
 
+static enum ipa_firmware_loader ipa_firmware_loader(struct device *dev)
+{
+	if (of_property_read_bool(dev->of_node, "modem-init"))
+		return IPA_LOADER_MODEM;
+
+	/* We need Trust Zone to load firmware; make sure it's available */
+	if (qcom_scm_is_available())
+		return IPA_LOADER_SELF;
+
+	return IPA_LOADER_DEFER;
+}
+
 /**
  * ipa_probe() - IPA platform driver probe function
  * @pdev:	Platform device pointer
@@ -722,9 +747,9 @@ static void ipa_validate_build(void)
 static int ipa_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	enum ipa_firmware_loader loader;
 	const struct ipa_data *data;
 	struct ipa_power *power;
-	bool modem_init;
 	struct ipa *ipa;
 	int ret;
 
@@ -747,11 +772,9 @@ static int ipa_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	/* If we need Trust Zone, make sure it's available */
-	modem_init = of_property_read_bool(dev->of_node, "modem-init");
-	if (!modem_init)
-		if (!qcom_scm_is_available())
-			return -EPROBE_DEFER;
+	loader = ipa_firmware_loader(dev);
+	if (loader == IPA_LOADER_DEFER)
+		return -EPROBE_DEFER;
 
 	/* The clock and interconnects might not be ready when we're
 	 * probed, so might return -EPROBE_DEFER.
@@ -796,7 +819,7 @@ static int ipa_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_endpoint_exit;
 
-	ret = ipa_smp2p_init(ipa, modem_init);
+	ret = ipa_smp2p_init(ipa, loader == IPA_LOADER_MODEM);
 	if (ret)
 		goto err_table_exit;
 
@@ -815,7 +838,7 @@ static int ipa_probe(struct platform_device *pdev)
 	 * call to ipa_setup() when it has finished.  In that case we're
 	 * done here.
 	 */
-	if (modem_init)
+	if (loader == IPA_LOADER_MODEM)
 		goto done;
 
 	/* Otherwise we need to load the firmware and have Trust Zone validate
-- 
2.34.1

[PATCH net-next 3/5] net: ipa: introduce "qcom,gsi-loader" property

[Alex Elder]

Introduce a new way of specifying how the GSI firmware gets loaded
for IPA.  Currently, this is indicated by the presence or absence of
the Boolean "modem-init" Device Tree property.  The new property
must have a value--either "self" or "modem"--which indicates whether
the AP or modem is the GSI firmware loader, respectively.

For legacy systems, the new property will not exist, and the
"modem-init" property will be used.  For newer systems, the
"qcom,gsi-loader" property *must* exist, and must have one of the
two prescribed values.  It is an error to have both properties
defined, and it is an error for the new property to have an
unrecognized value.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_main.c | 44 ++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 9e43b79d233e9..214e524dce795 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -87,11 +87,13 @@
  * @IPA_LOADER_DEFER:		System not ready; try again later
  * @IPA_LOADER_SELF:		AP loads GSI firmware
  * @IPA_LOADER_MODEM:		Modem loads GSI firmware, signals when done
+ * @IPA_LOADER_INVALID:	GSI firmware loader specification is invalid
  */
 enum ipa_firmware_loader {
 	IPA_LOADER_DEFER,
 	IPA_LOADER_SELF,
 	IPA_LOADER_MODEM,
+	IPA_LOADER_INVALID,
 };
 
 /**
@@ -711,9 +713,37 @@ static void ipa_validate_build(void)
 
 static enum ipa_firmware_loader ipa_firmware_loader(struct device *dev)
 {
-	if (of_property_read_bool(dev->of_node, "modem-init"))
+	bool modem_init;
+	const char *str;
+	int ret;
+
+	/* Look up the old and new properties by name */
+	modem_init = of_property_read_bool(dev->of_node, "modem-init");
+	ret = of_property_read_string(dev->of_node, "qcom,gsi-loader", &str);
+
+	/* If the new property doesn't exist, it's legacy behavior */
+	if (ret == -EINVAL) {
+		if (modem_init)
+			return IPA_LOADER_MODEM;
+		goto out_self;
+	}
+
+	/* Any other error on the new property means it's poorly defined */
+	if (ret)
+		return IPA_LOADER_INVALID;
+
+	/* New property value exists; if old one does too, that's invalid */
+	if (modem_init)
+		return IPA_LOADER_INVALID;
+
+	/* Modem loads GSI firmware for "modem" */
+	if (!strcmp(str, "modem"))
 		return IPA_LOADER_MODEM;
 
+	/* Any value other than "self" is an error */
+	if (strcmp(str, "self"))
+		return IPA_LOADER_INVALID;
+out_self:
 	/* We need Trust Zone to load firmware; make sure it's available */
 	if (qcom_scm_is_available())
 		return IPA_LOADER_SELF;
@@ -773,6 +803,8 @@ static int ipa_probe(struct platform_device *pdev)
 	}
 
 	loader = ipa_firmware_loader(dev);
+	if (loader == IPA_LOADER_INVALID)
+		return -EINVAL;
 	if (loader == IPA_LOADER_DEFER)
 		return -EPROBE_DEFER;
 
@@ -834,20 +866,18 @@ static int ipa_probe(struct platform_device *pdev)
 
 	dev_info(dev, "IPA driver initialized");
 
-	/* If the modem is doing early initialization, it will trigger a
-	 * call to ipa_setup() when it has finished.  In that case we're
-	 * done here.
+	/* If the modem is loading GSI firmware, it will trigger a call to
+	 * ipa_setup() when it has finished.  In that case we're done here.
 	 */
 	if (loader == IPA_LOADER_MODEM)
 		goto done;
 
-	/* Otherwise we need to load the firmware and have Trust Zone validate
-	 * and install it.  If that succeeds we can proceed with setup.
-	 */
+	/* The AP is loading GSI firmware; do so now */
 	ret = ipa_firmware_load(dev);
 	if (ret)
 		goto err_deconfig;
 
+	/* GSI firmware is loaded; proceed to setup */
 	ret = ipa_setup(ipa);
 	if (ret)
 		goto err_deconfig;
-- 
2.34.1

[PATCH net-next 4/5] dt-bindings: net: qcom,ipa: support skipping GSI firmware load

[Alex Elder]

Add a new enumerated value to those defined for the qcom,gsi-loader
property.  If the qcom,gsi-loader is "skip", the GSI firmware will
already be loaded, so neither the AP nor modem is required to load
GSI firmware.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 Documentation/devicetree/bindings/net/qcom,ipa.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/qcom,ipa.yaml b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
index 0dfd6c721e045..3d63505d1b802 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipa.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipa.yaml
@@ -128,10 +128,12 @@ properties:
     enum:
       - self
       - modem
+      - skip
     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 does this, this property has value "modem".  Otherwise, "skip"
+      means GSI firmware loading is not required.
 
   modem-init:
     deprecated: true
-- 
2.34.1

[PATCH net-next 5/5] net: ipa: permit GSI firmware loading to be skipped

[Alex Elder]

Define a new value "skip" for the "qcom,gsi-loader" Device Tree
property.  If used, it indicates that neither the AP nor the modem
need to load GSI firmware (because it has already been loaded--for
example by the boot loader).

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_main.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 214e524dce795..8f20825675a1a 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -87,12 +87,14 @@
  * @IPA_LOADER_DEFER:		System not ready; try again later
  * @IPA_LOADER_SELF:		AP loads GSI firmware
  * @IPA_LOADER_MODEM:		Modem loads GSI firmware, signals when done
+ * @IPA_LOADER_SKIP:		Neither AP nor modem need to load GSI firmware
  * @IPA_LOADER_INVALID:	GSI firmware loader specification is invalid
  */
 enum ipa_firmware_loader {
 	IPA_LOADER_DEFER,
 	IPA_LOADER_SELF,
 	IPA_LOADER_MODEM,
+	IPA_LOADER_SKIP,
 	IPA_LOADER_INVALID,
 };
 
@@ -740,6 +742,10 @@ static enum ipa_firmware_loader ipa_firmware_loader(struct device *dev)
 	if (!strcmp(str, "modem"))
 		return IPA_LOADER_MODEM;
 
+	/* No GSI firmware load is needed for "skip" */
+	if (!strcmp(str, "skip"))
+		return IPA_LOADER_SKIP;
+
 	/* Any value other than "self" is an error */
 	if (strcmp(str, "self"))
 		return IPA_LOADER_INVALID;
@@ -872,10 +878,12 @@ static int ipa_probe(struct platform_device *pdev)
 	if (loader == IPA_LOADER_MODEM)
 		goto done;
 
-	/* The AP is loading GSI firmware; do so now */
-	ret = ipa_firmware_load(dev);
-	if (ret)
-		goto err_deconfig;
+	if (loader == IPA_LOADER_SELF) {
+		/* The AP is loading GSI firmware; do so now */
+		ret = ipa_firmware_load(dev);
+		if (ret)
+			goto err_deconfig;
+	} /* Otherwise loader == IPA_LOADER_SKIP */
 
 	/* GSI firmware is loaded; proceed to setup */
 	ret = ipa_setup(ipa);
-- 
2.34.1

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

[Krzysztof Kozlowski]

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

Re: [PATCH net-next 4/5] dt-bindings: net: qcom,ipa: support skipping GSI firmware load

[Krzysztof Kozlowski]

On 12/11/2022 21:07, Alex Elder wrote:
> Add a new enumerated value to those defined for the qcom,gsi-loader
> property.  If the qcom,gsi-loader is "skip", the GSI firmware will
> already be loaded, so neither the AP nor modem is required to load
> GSI firmware.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  Documentation/devicetree/bindings/net/qcom,ipa.yaml | 4 +++-

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Alex Elder]

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
> 

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

[Krzysztof Kozlowski]

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

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

[Alex Elder]

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
>