[PATCH 0/3] stratix10: miscellaneous changes and fix for child driver probing

[PATCH 0/3] stratix10: miscellaneous changes and fix for child driver probing

[Mahesh Rao]

The patch set includes the following changes:

- Conversion of the devicetree binding file to JSON-schema for the Stratix10
  SVC driver and its child stratix10 fpga manager driver.
- Addition of support for probing the child driver. This fixes the failure
  of child driver probing as the SVC driver node has been moved out of the
  SoC node.

Signed-off-by: Mahesh Rao <mahesh.rao@intel.com>
---
Mahesh Rao (3):
      dt-bindings: fpga: stratix10: Convert to json-schema
      dt-bindings: firmware: stratix10: Convert to json-schema
      firmware: stratix10-svc: Add of_platform_default_populate()

 .../bindings/firmware/intel,stratix10-svc.txt      | 57 ---------------
 .../bindings/firmware/intel,stratix10-svc.yaml     | 84 ++++++++++++++++++++++
 .../fpga/intel,stratix10-soc-fpga-mgr.yaml         | 32 +++++++++
 .../bindings/fpga/intel-stratix10-soc-fpga-mgr.txt | 18 -----
 drivers/firmware/stratix10-svc.c                   | 14 +++-
 5 files changed, 127 insertions(+), 78 deletions(-)
---
base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
change-id: 20250109-socfpga_sip_svc_misc-bbcdfb7a0028

Best regards,
-- 
Mahesh Rao <mahesh.rao@intel.com>

[PATCH 1/3] dt-bindings: fpga: stratix10: Convert to json-schema

[Mahesh Rao]

Convert intel,stratix10-soc fpga manager devicetree
binding file from freeform format to json-schema.

Signed-off-by: Mahesh Rao <mahesh.rao@intel.com>
---
 .../fpga/intel,stratix10-soc-fpga-mgr.yaml         | 32 ++++++++++++++++++++++
 .../bindings/fpga/intel-stratix10-soc-fpga-mgr.txt | 18 ------------
 2 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/fpga/intel,stratix10-soc-fpga-mgr.yaml b/Documentation/devicetree/bindings/fpga/intel,stratix10-soc-fpga-mgr.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..34e1bc2359672210ab69e1d5af73c4c637b7f584
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/intel,stratix10-soc-fpga-mgr.yaml
@@ -0,0 +1,32 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/fpga/intel,stratix10-soc-fpga-mgr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Stratix10 SoC FPGA Manager
+
+maintainers:
+  - Moritz Fischer <mdf@kernel.org>
+  - Wu Hao <hao.wu@intel.com>
+  - Xu Yilun <yilun.xu@intel.com>
+
+description: |
+  Bindings for the Intel Stratix10 SoC FPGA Manager.
+
+properties:
+  compatible:
+    enum:
+      - intel,stratix10-soc-fpga-mgr
+      - intel,agilex-soc-fpga-mgr
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    fpga-mgr {
+      compatible = "intel,stratix10-soc-fpga-mgr";
+    };
diff --git a/Documentation/devicetree/bindings/fpga/intel-stratix10-soc-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/intel-stratix10-soc-fpga-mgr.txt
deleted file mode 100644
index 0f874137ca4697820341b23eddb882634bb131d1..0000000000000000000000000000000000000000
--- a/Documentation/devicetree/bindings/fpga/intel-stratix10-soc-fpga-mgr.txt
+++ /dev/null
@@ -1,18 +0,0 @@
-Intel Stratix10 SoC FPGA Manager
-
-Required properties:
-The fpga_mgr node has the following mandatory property, must be located under
-firmware/svc node.
-
-- compatible : should contain "intel,stratix10-soc-fpga-mgr" or
-	       "intel,agilex-soc-fpga-mgr"
-
-Example:
-
-	firmware {
-		svc {
-			fpga_mgr: fpga-mgr {
-				compatible = "intel,stratix10-soc-fpga-mgr";
-			};
-		};
-	};

-- 
2.35.3

[PATCH 2/3] dt-bindings: firmware: stratix10: Convert to json-schema

[Mahesh Rao]

Convert intel,stratix10-svc service layer devicetree
binding file from freeform format to json-schema.

Signed-off-by: Mahesh Rao <mahesh.rao@intel.com>
---
 .../bindings/firmware/intel,stratix10-svc.txt      | 57 ---------------
 .../bindings/firmware/intel,stratix10-svc.yaml     | 84 ++++++++++++++++++++++
 2 files changed, 84 insertions(+), 57 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/intel,stratix10-svc.txt b/Documentation/devicetree/bindings/firmware/intel,stratix10-svc.txt
deleted file mode 100644
index 6eff1afd8daf91714d6a18859667d2607e707da7..0000000000000000000000000000000000000000
--- a/Documentation/devicetree/bindings/firmware/intel,stratix10-svc.txt
+++ /dev/null
@@ -1,57 +0,0 @@
-Intel Service Layer Driver for Stratix10 SoC
-============================================
-Intel Stratix10 SoC is composed of a 64 bit quad-core ARM Cortex A53 hard
-processor system (HPS) and Secure Device Manager (SDM). When the FPGA is
-configured from HPS, there needs to be a way for HPS to notify SDM the
-location and size of the configuration data. Then SDM will get the
-configuration data from that location and perform the FPGA configuration.
-
-To meet the whole system security needs and support virtual machine requesting
-communication with SDM, only the secure world of software (EL3, Exception
-Layer 3) can interface with SDM. All software entities running on other
-exception layers must channel through the EL3 software whenever it needs
-service from SDM.
-
-Intel Stratix10 service layer driver, running at privileged exception level
-(EL1, Exception Layer 1), interfaces with the service providers and provides
-the services for FPGA configuration, QSPI, Crypto and warm reset. Service layer
-driver also manages secure monitor call (SMC) to communicate with secure monitor
-code running in EL3.
-
-Required properties:
--------------------
-The svc node has the following mandatory properties, must be located under
-the firmware node.
-
-- compatible: "intel,stratix10-svc" or "intel,agilex-svc"
-- method: smc or hvc
-        smc - Secure Monitor Call
-        hvc - Hypervisor Call
-- memory-region:
-	phandle to the reserved memory node. See
-	Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
-	for details
-
-Example:
--------
-
-	reserved-memory {
-                #address-cells = <2>;
-                #size-cells = <2>;
-                ranges;
-
-                service_reserved: svcbuffer@0 {
-                        compatible = "shared-dma-pool";
-                        reg = <0x0 0x0 0x0 0x1000000>;
-                        alignment = <0x1000>;
-                        no-map;
-                };
-        };
-
-	firmware {
-		svc {
-			compatible = "intel,stratix10-svc";
-			method = "smc";
-			memory-region = <&service_reserved>;
-		};
-	};
diff --git a/Documentation/devicetree/bindings/firmware/intel,stratix10-svc.yaml b/Documentation/devicetree/bindings/firmware/intel,stratix10-svc.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..b8aae996da87c16007efa7e5e12cced1432b62e9
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/intel,stratix10-svc.yaml
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/firmware/intel,stratix10-svc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Service Layer Driver for Stratix10 SoC
+
+maintainers:
+  - Dinh Nguyen <dinguyen@kernel.org>
+
+description: |
+  Intel Stratix10 SoC is composed of a 64 bit quad-core ARM Cortex A53 hard
+  processor system (HPS) and Secure Device Manager (SDM). When the FPGA is
+  configured from HPS, there needs to be a way for HPS to notify SDM the
+  location and size of the configuration data. Then SDM will get the
+  configuration data from that location and perform the FPGA configuration.
+
+  To meet the whole system security needs and support virtual machine requesting
+  communication with SDM, only the secure world of software (EL3, Exception
+  Layer 3) can interface with SDM. All software entities running on other
+  exception layers must channel through the EL3 software whenever it needs
+  service from SDM.
+
+  Intel Stratix10 service layer driver, running at privileged exception level
+  (EL1, Exception Layer 1), interfaces with the service providers and provides
+  the services for FPGA configuration, QSPI, Crypto and warm reset. Service layer
+  driver also manages secure monitor call (SMC) to communicate with secure monitor
+  code running in EL3.
+
+properties:
+  compatible:
+    enum:
+      - intel,stratix10-svc
+      - intel,agilex-svc
+
+  method:
+    enum: [smc, hvc]
+    description: supervisory call method to be used for the service layer.
+
+  memory-region:
+    maxItems: 1
+    description:
+      phandle to a reserved memory region for the service layer driver to
+      communicate with the secure device manager. For more details see
+      Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt.
+
+  fpga-mgr:
+    $ref: /schemas/fpga/intel,stratix10-soc-fpga-mgr.yaml
+    description: Optional child node for fpga manager to perform fabric configuration.
+
+required:
+  - compatible
+  - method
+  - memory-region
+
+additionalProperties: false
+
+examples:
+  - |
+    reserved-memory {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      service_reserved: svcbuffer@0 {
+              compatible = "shared-dma-pool";
+              reg = <0x0 0x0 0x0 0x1000000>;
+              alignment = <0x1000>;
+              no-map;
+      };
+    };
+
+    firmware {
+      svc {
+        compatible = "intel,stratix10-svc";
+        method = "smc";
+        memory-region = <&service_reserved>;
+
+        fpga-mgr {
+          compatible = "intel,stratix10-soc-fpga-mgr";
+        };
+      };
+    };
+

-- 
2.35.3

[PATCH 3/3] firmware: stratix10-svc: Add of_platform_default_populate()

[Mahesh Rao]

Add of_platform_default_populate() to stratix10-svc
driver as the firmware/svc node was moved out of soc.
This fixes the failed probing of child drivers of
svc node.

Fixes: 23c3ebed382a ("arm64: dts: socfpga: agilex: move firmware out of soc node")

Signed-off-by: Mahesh Rao <mahesh.rao@intel.com>
---
 drivers/firmware/stratix10-svc.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index c5c78b869561b0c1e9602823ad1f501e98e3ce51..15a7207f7753dcd4e94da4aa9a6162fedb577fe9 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -1227,13 +1227,19 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
 	if (!svc->intel_svc_fcs) {
 		dev_err(dev, "failed to allocate %s device\n", INTEL_FCS);
 		ret = -ENOMEM;
-		goto err_unregister_dev;
+		goto err_unregister_rsu_dev;
 	}
 
 	ret = platform_device_add(svc->intel_svc_fcs);
 	if (ret) {
 		platform_device_put(svc->intel_svc_fcs);
-		goto err_unregister_dev;
+		goto err_unregister_rsu_dev;
+	}
+
+	ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
+	if (ret < 0) {
+		of_platform_depopulate(dev);
+		goto err_unregister_fcs_dev;
 	}
 
 	dev_set_drvdata(dev, svc);
@@ -1242,7 +1248,9 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
 
 	return 0;
 
-err_unregister_dev:
+err_unregister_fcs_dev:
+	platform_device_unregister(svc->intel_svc_fcs);
+err_unregister_rsu_dev:
 	platform_device_unregister(svc->stratix10_svc_rsu);
 err_free_kfifo:
 	kfifo_free(&controller->svc_fifo);

-- 
2.35.3

Re: [PATCH 2/3] dt-bindings: firmware: stratix10: Convert to json-schema

[kernel test robot]

Hi Mahesh,

kernel test robot noticed the following build warnings:

[auto build test WARNING on ffd294d346d185b70e28b1a28abe367bbfe53c04]

url:    https://github.com/intel-lab-lkp/linux/commits/Mahesh-Rao/dt-bindings-fpga-stratix10-Convert-to-json-schema/20250122-140026
base:   ffd294d346d185b70e28b1a28abe367bbfe53c04
patch link:    https://lore.kernel.org/r/20250122-socfpga_sip_svc_misc-v1-2-cbdcd034ae34%40intel.com
patch subject: [PATCH 2/3] dt-bindings: firmware: stratix10: Convert to json-schema
reproduce: (https://download.01.org/0day-ci/archive/20250122/202501221710.7llON7lZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501221710.7llON7lZ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt
   Warning: Documentation/hwmon/isl28022.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/isl,isl28022.yaml
   Warning: Documentation/translations/ja_JP/SubmittingPatches references a file that doesn't exist: linux-2.6.12-vanilla/Documentation/dontdiff
>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/firmware/intel,stratix10-svc.txt
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
   Warning: lib/Kconfig.debug references a file that doesn't exist: Documentation/dev-tools/fault-injection/fault-injection.rst
   Using alabaster theme

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] dt-bindings: fpga: stratix10: Convert to json-schema

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2868 bytes --]

On Wed, Jan 22, 2025 at 01:58:43PM +0800, Mahesh Rao wrote:
> Convert intel,stratix10-soc fpga manager devicetree
> binding file from freeform format to json-schema.
> 
> Signed-off-by: Mahesh Rao <mahesh.rao@intel.com>
> ---
>  .../fpga/intel,stratix10-soc-fpga-mgr.yaml         | 32 ++++++++++++++++++++++
>  .../bindings/fpga/intel-stratix10-soc-fpga-mgr.txt | 18 ------------
>  2 files changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/fpga/intel,stratix10-soc-fpga-mgr.yaml b/Documentation/devicetree/bindings/fpga/intel,stratix10-soc-fpga-mgr.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..34e1bc2359672210ab69e1d5af73c4c637b7f584
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/intel,stratix10-soc-fpga-mgr.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/fpga/intel,stratix10-soc-fpga-mgr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel Stratix10 SoC FPGA Manager
> +
> +maintainers:
> +  - Moritz Fischer <mdf@kernel.org>

Are these maintainers actually correct? Does Moritz work on Altera
stuff, or did you just add him cos he is a subsystem maintainer? Really
what's here should be people that understand the hardware.

> +  - Wu Hao <hao.wu@intel.com>
> +  - Xu Yilun <yilun.xu@intel.com>
> +
> +description: |

The | here isn't needed, nor is point out that this is a binding in the
line below. Please describe what the hardware is here instead.

> +  Bindings for the Intel Stratix10 SoC FPGA Manager.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - intel,stratix10-soc-fpga-mgr
> +      - intel,agilex-soc-fpga-mgr
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    fpga-mgr {
> +      compatible = "intel,stratix10-soc-fpga-mgr";
> +    };
> diff --git a/Documentation/devicetree/bindings/fpga/intel-stratix10-soc-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/intel-stratix10-soc-fpga-mgr.txt
> deleted file mode 100644
> index 0f874137ca4697820341b23eddb882634bb131d1..0000000000000000000000000000000000000000
> --- a/Documentation/devicetree/bindings/fpga/intel-stratix10-soc-fpga-mgr.txt
> +++ /dev/null
> @@ -1,18 +0,0 @@
> -Intel Stratix10 SoC FPGA Manager
> -
> -Required properties:
> -The fpga_mgr node has the following mandatory property, must be located under
> -firmware/svc node.
> -
> -- compatible : should contain "intel,stratix10-soc-fpga-mgr" or
> -	       "intel,agilex-soc-fpga-mgr"
> -
> -Example:
> -
> -	firmware {
> -		svc {
> -			fpga_mgr: fpga-mgr {
> -				compatible = "intel,stratix10-soc-fpga-mgr";
> -			};
> -		};
> -	};
> 
> -- 
> 2.35.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] dt-bindings: firmware: stratix10: Convert to json-schema

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3750 bytes --]

On Wed, Jan 22, 2025 at 01:58:44PM +0800, Mahesh Rao wrote:
> Convert intel,stratix10-svc service layer devicetree
> binding file from freeform format to json-schema.

> diff --git a/Documentation/devicetree/bindings/firmware/intel,stratix10-svc.yaml b/Documentation/devicetree/bindings/firmware/intel,stratix10-svc.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..b8aae996da87c16007efa7e5e12cced1432b62e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/intel,stratix10-svc.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/firmware/intel,stratix10-svc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel Service Layer Driver for Stratix10 SoC
> +
> +maintainers:
> +  - Dinh Nguyen <dinguyen@kernel.org>
> +
> +description: |
> +  Intel Stratix10 SoC is composed of a 64 bit quad-core ARM Cortex A53 hard
> +  processor system (HPS) and Secure Device Manager (SDM). When the FPGA is
> +  configured from HPS, there needs to be a way for HPS to notify SDM the
> +  location and size of the configuration data. Then SDM will get the
> +  configuration data from that location and perform the FPGA configuration.
> +
> +  To meet the whole system security needs and support virtual machine requesting
> +  communication with SDM, only the secure world of software (EL3, Exception
> +  Layer 3) can interface with SDM. All software entities running on other
> +  exception layers must channel through the EL3 software whenever it needs
> +  service from SDM.
> +
> +  Intel Stratix10 service layer driver, running at privileged exception level
> +  (EL1, Exception Layer 1), interfaces with the service providers and provides
> +  the services for FPGA configuration, QSPI, Crypto and warm reset. Service layer
> +  driver also manages secure monitor call (SMC) to communicate with secure monitor
> +  code running in EL3.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - intel,stratix10-svc
> +      - intel,agilex-svc
> +
> +  method:
> +    enum: [smc, hvc]
> +    description: supervisory call method to be used for the service layer.

This looks to be missing a type (string) and an explanation of what "smc" and
"hvc" are.

> +
> +  memory-region:
> +    maxItems: 1
> +    description:
> +      phandle to a reserved memory region for the service layer driver to
> +      communicate with the secure device manager. For more details see
> +      Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt.
> +
> +  fpga-mgr:
> +    $ref: /schemas/fpga/intel,stratix10-soc-fpga-mgr.yaml
> +    description: Optional child node for fpga manager to perform fabric configuration.

This is new and not justified in your commit message. Please explain
where this has come from in v2.

Cheers,
Conor.

> +
> +required:
> +  - compatible
> +  - method
> +  - memory-region
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    reserved-memory {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      service_reserved: svcbuffer@0 {
> +              compatible = "shared-dma-pool";
> +              reg = <0x0 0x0 0x0 0x1000000>;
> +              alignment = <0x1000>;
> +              no-map;
> +      };
> +    };
> +
> +    firmware {
> +      svc {
> +        compatible = "intel,stratix10-svc";
> +        method = "smc";
> +        memory-region = <&service_reserved>;
> +
> +        fpga-mgr {
> +          compatible = "intel,stratix10-soc-fpga-mgr";
> +        };
> +      };
> +    };
> +
> 
> -- 
> 2.35.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/3] firmware: stratix10-svc: Add of_platform_default_populate()

[Krzysztof Kozlowski]

On 22/01/2025 06:58, Mahesh Rao wrote:
> Add of_platform_default_populate() to stratix10-svc
> driver as the firmware/svc node was moved out of soc.
> This fixes the failed probing of child drivers of
> svc node.
> 
> Fixes: 23c3ebed382a ("arm64: dts: socfpga: agilex: move firmware out of soc node")
> 


There is never a blank line between tags. Use: `git log`

> Signed-off-by: Mahesh Rao <mahesh.rao@intel.com>

> ---
>  drivers/firmware/stratix10-svc.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index c5c78b869561b0c1e9602823ad1f501e98e3ce51..15a7207f7753dcd4e94da4aa9a6162fedb577fe9 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -1227,13 +1227,19 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>  	if (!svc->intel_svc_fcs) {
>  		dev_err(dev, "failed to allocate %s device\n", INTEL_FCS);
>  		ret = -ENOMEM;
> -		goto err_unregister_dev;
> +		goto err_unregister_rsu_dev;
>  	}
>  
>  	ret = platform_device_add(svc->intel_svc_fcs);
>  	if (ret) {
>  		platform_device_put(svc->intel_svc_fcs);
> -		goto err_unregister_dev;
> +		goto err_unregister_rsu_dev;
> +	}
> +
> +	ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
> +	if (ret < 0) {
> +		of_platform_depopulate(dev);
> +		goto err_unregister_fcs_dev;
>  	}
>  
>  	dev_set_drvdata(dev, svc);
> @@ -1242,7 +1248,9 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -err_unregister_dev:
> +err_unregister_fcs_dev:
> +	platform_device_unregister(svc->intel_svc_fcs);
> +err_unregister_rsu_dev:
>  	platform_device_unregister(svc->stratix10_svc_rsu);
>  err_free_kfifo:
>  	kfifo_free(&controller->svc_fifo);


You need to update remove() callback.




Best regards,
Krzysztof

Re: [PATCH 3/3] firmware: stratix10-svc: Add of_platform_default_populate()

[Xu Yilun]

On Wed, Jan 22, 2025 at 01:58:45PM +0800, Mahesh Rao wrote:
> Add of_platform_default_populate() to stratix10-svc
> driver as the firmware/svc node was moved out of soc.
> This fixes the failed probing of child drivers of
> svc node.
> 
> Fixes: 23c3ebed382a ("arm64: dts: socfpga: agilex: move firmware out of soc node")
> 
> Signed-off-by: Mahesh Rao <mahesh.rao@intel.com>
> ---
>  drivers/firmware/stratix10-svc.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index c5c78b869561b0c1e9602823ad1f501e98e3ce51..15a7207f7753dcd4e94da4aa9a6162fedb577fe9 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -1227,13 +1227,19 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>  	if (!svc->intel_svc_fcs) {
>  		dev_err(dev, "failed to allocate %s device\n", INTEL_FCS);
>  		ret = -ENOMEM;
> -		goto err_unregister_dev;
> +		goto err_unregister_rsu_dev;
>  	}
>  
>  	ret = platform_device_add(svc->intel_svc_fcs);
>  	if (ret) {
>  		platform_device_put(svc->intel_svc_fcs);
> -		goto err_unregister_dev;
> +		goto err_unregister_rsu_dev;
> +	}
> +
> +	ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
> +	if (ret < 0) {

	if (ret)  is just fine.

> +		of_platform_depopulate(dev);
> +		goto err_unregister_fcs_dev;

You wanna destroy everything even if some child drivers work?
And do we need to do depopulation on driver remove?
I'm actually a little confused how to handle populate() fail and
depopulate().

Thanks,
Yilun

>  	}
>  
>  	dev_set_drvdata(dev, svc);
> @@ -1242,7 +1248,9 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -err_unregister_dev:
> +err_unregister_fcs_dev:
> +	platform_device_unregister(svc->intel_svc_fcs);
> +err_unregister_rsu_dev:
>  	platform_device_unregister(svc->stratix10_svc_rsu);
>  err_free_kfifo:
>  	kfifo_free(&controller->svc_fifo);
> 
> -- 
> 2.35.3
> 
> 

Re: [PATCH 1/3] dt-bindings: fpga: stratix10: Convert to json-schema

[Mahesh Rao]

Hi Conor Dooley,

Thanks for reviewing the patch.

On Wed, 22 Jan 2025 18:35:51 +0000, Conor Dooley wrote:
> > Convert intel,stratix10-soc fpga manager devicetree binding file from
> > freeform format to json-schema.
> >
> > Signed-off-by: Mahesh Rao <mahesh.rao@intel.com>
> > +maintainers:
> > +  - Moritz Fischer <mdf@kernel.org>
> 
> Are these maintainers actually correct? Does Moritz work on Altera stuff, or
> did you just add him cos he is a subsystem maintainer? Really what's here
> should be people that understand the hardware.
> 

Sorry I initially had added list from maintaner script,I will change to 
people who works with the hardware in the next revision.

> > +  - Wu Hao <hao.wu@intel.com>
> > +  - Xu Yilun <yilun.xu@intel.com>
> > +
> > +description: |
> 
> The | here isn't needed, nor is point out that this is a binding in the line
> below. Please describe what the hardware is here instead.
> 

Sure I will update the info in the next revision.

Best regards
Mahesh Rao

Re: [PATCH 2/3] dt-bindings: firmware: stratix10: Convert to json-schema

[Mahesh Rao]

Hi Conor Dooley,

On Wed, 22 Jan 2025 18:40:41 +0000, Conor Dooley wrote:
> > Convert intel,stratix10-svc service layer devicetree binding file from
> > freeform format to json-schema.
> 

> > +
> > +  method:
> > +    enum: [smc, hvc]
> > +    description: supervisory call method to be used for the service layer.
> 
> This looks to be missing a type (string) and an explanation of what "smc" and
> "hvc" are.
> 

Thanks for pointing out , will do the change in next revision.

> > +
> > +  fpga-mgr:
> > +    $ref: /schemas/fpga/intel,stratix10-soc-fpga-mgr.yaml
> > +    description: Optional child node for fpga manager to perform fabric
> configuration.
> 
> This is new and not justified in your commit message. Please explain where
> this has come from in v2.
> 
> Cheers,
> Conor.
> 

Sure, this is an optional child node/driver present in the device tree for
the Agilex SoC devices, but it was not mentioned in the text documentation.
Therefore, I had included it here. I will provide more details about this
change in the next version.

Best Regards
Mahesh Rao

Re: [PATCH 2/3] dt-bindings: firmware: stratix10: Convert to json-schema

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

On Tue, Jan 28, 2025 at 01:07:05AM +0800, Mahesh Rao wrote:
> Hi Conor Dooley,
> 
> On Wed, 22 Jan 2025 18:40:41 +0000, Conor Dooley wrote:
> > > Convert intel,stratix10-svc service layer devicetree binding file from
> > > freeform format to json-schema.
> > 
> 
> > > +
> > > +  method:
> > > +    enum: [smc, hvc]
> > > +    description: supervisory call method to be used for the service layer.
> > 
> > This looks to be missing a type (string) and an explanation of what "smc" and
> > "hvc" are.
> > 
> 
> Thanks for pointing out , will do the change in next revision.
> 
> > > +
> > > +  fpga-mgr:
> > > +    $ref: /schemas/fpga/intel,stratix10-soc-fpga-mgr.yaml
> > > +    description: Optional child node for fpga manager to perform fabric
> > configuration.
> > 
> > This is new and not justified in your commit message. Please explain where
> > this has come from in v2.
> > 
> > Cheers,
> > Conor.
> > 
> 
> Sure, this is an optional child node/driver present in the device tree for
> the Agilex SoC devices, but it was not mentioned in the text documentation.
> Therefore, I had included it here. I will provide more details about this
> change in the next version.

Aye, the change itself is fine. Just mention what the rationale for
adding it is and you're good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/3] firmware: stratix10-svc: Add of_platform_default_populate()

[Mahesh Rao]

Hi Krzysztof,
Thanks for Reviewing the patch.

On Thu, 23 Jan 2025 08:26:08 +0100, Krzysztof Kozlowski wrote:
> > Add of_platform_default_populate() to stratix10-svc driver as the
> > firmware/svc node was moved out of soc.
> > This fixes the failed probing of child drivers of svc node.
> >
> > Fixes: 23c3ebed382a ("arm64: dts: socfpga: agilex: move firmware out
> > of soc node")
> >
> 
> 
> There is never a blank line between tags. Use: `git log`
> 
> > Signed-off-by: Mahesh Rao <mahesh.rao@intel.com>

Understood, I will revise this in the next revision.

> 
> > ---
> >  drivers/firmware/stratix10-svc.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/stratix10-svc.c
> > b/drivers/firmware/stratix10-svc.c
> > index

> >  	platform_device_unregister(svc->stratix10_svc_rsu);
> >  err_free_kfifo:
> >  	kfifo_free(&controller->svc_fifo);
> 
> 
> You need to update remove() callback.
> 

Sure, I will add the changes in the next revision.

Best regards,
Mahesh Rao

Re: [PATCH 3/3] firmware: stratix10-svc: Add of_platform_default_populate()

[Mahesh Rao]

Hi Yilun,
Thanks for reviewing the patch.

On Sun, 26 Jan 2025 16:28:55 +0800, Xu Yilun wrote:
> > Add of_platform_default_populate() to stratix10-svc driver as the
> > firmware/svc node was moved out of soc.
> > This fixes the failed probing of child drivers of svc node.
> >
> > Fixes: 23c3ebed382a ("arm64: dts: socfpga: agilex: move firmware out
> > of soc node")

> > +	ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
> > +	if (ret < 0) {
> 
> 	if (ret)  is just fine.

ok ,I will make the change.

> 
> > +		of_platform_depopulate(dev);
> > +		goto err_unregister_fcs_dev;
> 
> You wanna destroy everything even if some child drivers work?

Currently, there is no requirement to retain the driver if a child component fails.
we will handle it if it is needed in the future.

> And do we need to do depopulation on driver remove?

I think yes , I have missed this. I will add depopulate in the remove callback().

> I'm actually a little confused how to handle populate() fail and depopulate().

I think this was a mistake on my side. I will make the change in next revision.

Best Regards,
Mahesh Rao

Re: [PATCH 3/3] firmware: stratix10-svc: Add of_platform_default_populate()

[Xu Yilun]

On Thu, Jan 30, 2025 at 02:01:13AM +0800, Mahesh Rao wrote:
> Hi Yilun,
> Thanks for reviewing the patch.
> 
> On Sun, 26 Jan 2025 16:28:55 +0800, Xu Yilun wrote:
> > > Add of_platform_default_populate() to stratix10-svc driver as the
> > > firmware/svc node was moved out of soc.
> > > This fixes the failed probing of child drivers of svc node.
> > >
> > > Fixes: 23c3ebed382a ("arm64: dts: socfpga: agilex: move firmware out
> > > of soc node")
> 
> > > +	ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
> > > +	if (ret < 0) {
> > 
> > 	if (ret)  is just fine.
> 
> ok ,I will make the change.
> 
> > 
> > > +		of_platform_depopulate(dev);
> > > +		goto err_unregister_fcs_dev;
> > 
> > You wanna destroy everything even if some child drivers work?
> 
> Currently, there is no requirement to retain the driver if a child component fails.
> we will handle it if it is needed in the future.

Does the previous "soc" style population did the same way?

Thanks,
Yilun

> 
> > And do we need to do depopulation on driver remove?
> 
> I think yes , I have missed this. I will add depopulate in the remove callback().
> 
> > I'm actually a little confused how to handle populate() fail and depopulate().
> 
> I think this was a mistake on my side. I will make the change in next revision.
> 
> Best Regards,
> Mahesh Rao