[PATCH v2 0/4] Enable SPI on SA8255p Qualcomm platforms

[PATCH v2 0/4] Enable SPI on SA8255p Qualcomm platforms

[Praveen Talari]

The Qualcomm automotive SA8255p SoC relies on firmware to configure
platform resources, including clocks, interconnects and TLMM.
The driver requests resources operations over SCMI using power
and performance protocols.

The SCMI power protocol enables or disables resources like clocks,
interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
such as resume/suspend, to control power states(on/off).

The SCMI performance protocol manages SPI frequency, with each
frequency rate represented by a performance level. The driver uses
geni_se_set_perf_opp() API to request the desired frequency rate.

As part of geni_se_set_perf_opp(), the OPP for the requested frequency
is obtained using dev_pm_opp_find_freq_floor() and the performance
level is set using dev_pm_opp_set_opp().

Praveen Talari (4):
  spi: dt-bindings: describe SA8255p
  spi: qcom-geni: Use geni_se_resources_init() for resource
    initialization
  spi: qcom-geni: Use resources helper APIs in runtime PM functions
  spi: qcom-geni: Enable SPI on SA8255p Qualcomm platforms

.../bindings/spi/qcom,sa8255p-geni-spi.yaml   | 63 ++++++++++++++
 drivers/spi/spi-geni-qcom.c                   | 83 ++++++++-----------
 2 files changed, 97 insertions(+), 49 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,sa8255p-geni-spi.yaml

---
Changes in v2:
- Rebased patches on latest linux-next.
- Link to v1: https://lore.kernel.org/all/20260112190134.1526646-1-praveen.talari@oss.qualcomm.com/

To: Mark Brown <broonie@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Praveen Talari <praveen.talari@oss.qualcomm.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-spi@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Praveen Talari (4):
      spi: dt-bindings: describe SA8255p
      spi: qcom-geni: Use geni_se_resources_init() for resource initialization
      spi: qcom-geni: Use resources helper APIs in runtime PM functions
      spi: qcom-geni: Enable SPI on SA8255p Qualcomm platforms

 .../bindings/spi/qcom,sa8255p-geni-spi.yaml        | 63 ++++++++++++++++
 drivers/spi/spi-geni-qcom.c                        | 83 +++++++++-------------
 2 files changed, 97 insertions(+), 49 deletions(-)
---
base-commit: f7af91adc230aa99e23330ecf85bc9badd9780ad
change-id: 20260529-enable-spi-on-sa8255p-8166eaa226cb

Best regards,
--  
Praveen Talari <praveen.talari@oss.qualcomm.com>

[PATCH v2 1/4] spi: dt-bindings: describe SA8255p

[Praveen Talari]

Add DT bindings for the QUP GENI SPI controller on sa8255p platform.

SA8255p platform abstracts resources such as clocks, interconnect and
GPIO pins configuration in Firmware. SCMI power and perf protocols are
utilized to request resource configurations.

SA8255p platform does not require the Serial Engine (SE) common properties
as the SE firmware is loaded and managed by the TrustZone (TZ) secure
environment.

Co-developed-by: Nikunj Kela <quic_nkela@quicinc.com>
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 .../bindings/spi/qcom,sa8255p-geni-spi.yaml        | 63 ++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/qcom,sa8255p-geni-spi.yaml b/Documentation/devicetree/bindings/spi/qcom,sa8255p-geni-spi.yaml
new file mode 100644
index 000000000000..8c6825b8ddb4
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/qcom,sa8255p-geni-spi.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/qcom,sa8255p-geni-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GENI based Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)
+
+maintainers:
+  - Praveen Talari <praveen.talari@oss.qualcomm.com>
+
+properties:
+  compatible:
+    const: qcom,sa8255p-geni-spi
+
+  reg:
+    maxItems: 1
+
+  dmas:
+    maxItems: 2
+
+  dma-names:
+    items:
+      - const: tx
+      - const: rx
+
+  interrupts:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 2
+
+  power-domain-names:
+    items:
+      - const: power
+      - const: perf
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - power-domains
+  - power-domain-names
+
+allOf:
+  - $ref: /schemas/spi/spi-controller.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/dma/qcom-gpi.h>
+
+    spi@888000 {
+        compatible = "qcom,sa8255p-geni-spi";
+        reg = <0x888000 0x4000>;
+        interrupts = <GIC_SPI 584 IRQ_TYPE_LEVEL_HIGH>;
+        dmas = <&gpi_dma0 0 0 QCOM_GPI_SPI>,
+               <&gpi_dma0 1 0 QCOM_GPI_SPI>;
+        power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
+        power-domain-names = "power", "perf";
+    };

-- 
2.34.1

Re: [PATCH v2 1/4] spi: dt-bindings: describe SA8255p

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unchecked `device_get_match_data()` return value causes a NULL pointer dereference if bound manually.
- [Medium] The DT binding example provides `dmas` but omits the strictly required `dma-names` property, and the schema lacks structural dependencies for it.
- [Medium] The `qcom,sa8255p-geni-spi` binding strictly requires a single `compatible` string, rejecting the valid `sa8797p` fallback array.
--

commit a89af5cc4b98b3af1b2a30423bbe32d7faf13e17
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: dt-bindings: describe SA8255p

Add DT bindings for the QUP GENI SPI controller on sa8255p platform.
SA8255p platform abstracts resources such as clocks, interconnect and
GPIO pins configuration in Firmware. SCMI power and perf protocols are
utilized to request resource configurations.

> diff --git a/Documentation/devicetree/bindings/spi/qcom,sa8255p-geni-spi.yaml b/Documentation/devicetree/bindings/spi/qcom,sa8255p-geni-spi.yaml
> new file mode 100644
> index 0000000000000..8c6825b8ddb41
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/qcom,sa8255p-geni-spi.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/qcom,sa8255p-geni-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GENI based Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)
> +
> +maintainers:
> +  - Praveen Talari <praveen.talari@oss.qualcomm.com>
> +
> +properties:
> +  compatible:
> +    const: qcom,sa8255p-geni-spi

[Severity: Medium]
Will this strict constraint reject the qcom,sa8797p-geni-spi fallback array?

The parent QUP wrapper binding (qcom,sa8255p-geni-se-qup.yaml) explicitly
defines SPI child nodes matching "spi@[0-9a-f]+$" to have:
compatible: oneOf: - const: qcom,sa8255p-geni-spi - items: - const: qcom,sa8797p-geni-spi - const: qcom,sa8255p-geni-spi

When a device tree node for the sa8797p variant provides the multi-string
fallback array, dtbs_check will fail validation because the array does not
match this strict const constraint. Should this be updated to use a oneOf
block containing the fallback array?

[ ... ]

> +  dmas:
> +    maxItems: 2
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx

[Severity: Medium]
Should there be a dependencies constraint here to enforce dma-names when
dmas is provided?

Without a constraint like dependencies: dmas: [ dma-names ], it is possible
to configure dmas without dma-names. Since the driver requires channels by
name, missing dma-names could cause issues that structural enforcement
would prevent.

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/dma/qcom-gpi.h>
> +
> +    spi@888000 {
> +        compatible = "qcom,sa8255p-geni-spi";
> +        reg = <0x888000 0x4000>;
> +        interrupts = <GIC_SPI 584 IRQ_TYPE_LEVEL_HIGH>;
> +        dmas = <&gpi_dma0 0 0 QCOM_GPI_SPI>,
> +               <&gpi_dma0 1 0 QCOM_GPI_SPI>;
> +        power-domains = <&scmi0_pd 0>, <&scmi0_dvfs 0>;
> +        power-domain-names = "power", "perf";
> +    };

[Severity: Medium]
Is the dma-names property missing from this example?

The spi-geni-qcom.c driver requires DMA channels by name and acquires them
using dma_request_chan(dev, "tx") and dma_request_chan(dev, "rx"). Because
the example configures dmas but omits the dma-names array entirely,
developers copying this as a reference might silently fail to initialize
DMA and fall back to PIO mode.


Additionally, regarding a later commit in this series ("spi: qcom-geni:
Enable SPI on SA8255p Qualcomm platforms"):

[Severity: High]
Does spi_geni_probe() safely handle a manual bind?

If the device is bound manually via sysfs (bind) or uses driver_override
without a matched OF/ACPI node, device_get_match_data() returns NULL.

The function sets:
mas->dev_data = device_get_match_data(&pdev->dev);

Followed immediately by a dereference:
ret = mas->dev_data->resources_init(&mas->se);

Will this result in a NULL pointer dereference panic under those conditions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530-enable-spi-on-sa8255p-v2-0-17574601bd63@oss.qualcomm.com?part=1

[PATCH v2 2/4] spi: qcom-geni: Use geni_se_resources_init() for resource initialization

[Praveen Talari]

Replace resources initialization such as clocks, ICC path and OPP with the
common geni_se_resources_init() function to avoid code duplication across
all drivers.

The geni_se_resources_init() function handles all these resources
internally, reducing code duplication and ensuring consistent resource
management across GENI SE drivers.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/spi/spi-geni-qcom.c | 26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index a04cdc1e5ad4..333216df922e 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -1034,7 +1034,6 @@ static int spi_geni_probe(struct platform_device *pdev)
 	struct spi_controller *spi;
 	struct spi_geni_master *mas;
 	void __iomem *base;
-	struct clk *clk;
 	struct device *dev = &pdev->dev;
 
 	irq = platform_get_irq(pdev, 0);
@@ -1049,10 +1048,6 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-	clk = devm_clk_get(dev, "se");
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
-
 	if (device_property_read_bool(dev, "spi-slave"))
 		spi = devm_spi_alloc_target(dev, sizeof(*mas));
 	else
@@ -1068,17 +1063,10 @@ static int spi_geni_probe(struct platform_device *pdev)
 	mas->se.dev = dev;
 	mas->se.wrapper = dev_get_drvdata(dev->parent);
 	mas->se.base = base;
-	mas->se.clk = clk;
 
-	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
+	ret = geni_se_resources_init(&mas->se);
 	if (ret)
 		return ret;
-	/* OPP table is optional */
-	ret = devm_pm_opp_of_add_table(&pdev->dev);
-	if (ret && ret != -ENODEV) {
-		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
-		return ret;
-	}
 
 	spi->bus_num = -1;
 	spi->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_CS_HIGH;
@@ -1104,24 +1092,12 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (spi->target)
 		spi->target_abort = spi_geni_target_abort;
 
-	ret = geni_icc_get(&mas->se, NULL);
-	if (ret)
-		return ret;
-
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 250);
 	ret = devm_pm_runtime_enable(dev);
 	if (ret)
 		return ret;
 
-	/* Set the bus quota to a reasonable value for register access */
-	mas->se.icc_paths[GENI_TO_CORE].avg_bw = Bps_to_icc(CORE_2X_50_MHZ);
-	mas->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
-
-	ret = geni_icc_set_bw(&mas->se);
-	if (ret)
-		return ret;
-
 	ret = spi_geni_init(mas);
 	if (ret)
 		return ret;

-- 
2.34.1

Re: [PATCH v2 2/4] spi: qcom-geni: Use geni_se_resources_init() for resource initialization

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unchecked ERR_PTR for se->clk on ACPI platforms causes kernel panic in clk_round_rate().

Pre-existing issues:
- [High] Missing return value check for pm_runtime_get_sync() in spi_geni_init() leads to unclocked hardware access.
--

commit c69b8f64e97b3440b056b6b346311b9f4e470629
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: qcom-geni: Use geni_se_resources_init() for resource initialization

Replace resources initialization such as clocks, ICC path and OPP with the
common geni_se_resources_init() function to avoid code duplication across
all drivers.

The geni_se_resources_init() function handles all these resources
internally, reducing code duplication and ensuring consistent resource
management across GENI SE drivers.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index a04cdc1e5ad4d..333216df922e8 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -1049,10 +1048,6 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
>  
> -	clk = devm_clk_get(dev, "se");
> -	if (IS_ERR(clk))
> -		return PTR_ERR(clk);
> -
>  	if (device_property_read_bool(dev, "spi-slave"))
>  		spi = devm_spi_alloc_target(dev, sizeof(*mas));
>  	else
> @@ -1068,17 +1063,10 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	mas->se.dev = dev;
>  	mas->se.wrapper = dev_get_drvdata(dev->parent);
>  	mas->se.base = base;
> -	mas->se.clk = clk;
>  
> -	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
> +	ret = geni_se_resources_init(&mas->se);

[Severity: High]
Does this change introduce a potential kernel panic on ACPI platforms?

By switching to geni_se_resources_init(), the driver now tolerates a missing
se->clk on ACPI platforms (leaving se->clk as an ERR_PTR):

drivers/soc/qcom/qcom-geni-se.c:geni_se_resources_init() {
    ...
    se->clk = devm_clk_get(se->dev, "se");
    if (IS_ERR(se->clk) && !has_acpi_companion(se->dev))
        return dev_err_probe(...);
    ...
}

However, the driver unconditionally triggers frequency matching later, which
evaluates the ERR_PTR in clk_round_rate():

drivers/spi/spi-geni-qcom.c:get_spi_clk_cfg() {
    ...
    ret = geni_se_clk_freq_match(&mas->se, speed_hz * mas->oversampling,
                                 clk_idx, &sclk_freq, false);
    ...
}

Because clk_round_rate() only checks for NULL and not IS_ERR(), it will
dereference the error pointer and cause a panic. Prior to this patch, the
driver correctly aborted probing if the clock was missing.

>  	if (ret)
>  		return ret;
> -	/* OPP table is optional */
> -	ret = devm_pm_opp_of_add_table(&pdev->dev);
> -	if (ret && ret != -ENODEV) {
> -		dev_err(&pdev->dev, "invalid OPP table in device tree\n");
> -		return ret;
> -	}

[ ... ]

> @@ -1104,24 +1092,12 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	if (spi->target)
>  		spi->target_abort = spi_geni_target_abort;
>  
> -	ret = geni_icc_get(&mas->se, NULL);
> -	if (ret)
> -		return ret;
> -
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, 250);
>  	ret = devm_pm_runtime_enable(dev);
>  	if (ret)
>  		return ret;
>  
> -	/* Set the bus quota to a reasonable value for register access */
> -	mas->se.icc_paths[GENI_TO_CORE].avg_bw = Bps_to_icc(CORE_2X_50_MHZ);
> -	mas->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
> -
> -	ret = geni_icc_set_bw(&mas->se);
> -	if (ret)
> -		return ret;
> -
>  	ret = spi_geni_init(mas);

[Severity: High]
This is a pre-existing issue, but does spi_geni_init() need to check the
return value of pm_runtime_get_sync()?

Looking at spi_geni_init(), it accesses hardware registers immediately after
calling pm_runtime_get_sync() without checking for failure:

drivers/spi/spi-geni-qcom.c:spi_geni_init() {
    ...
    pm_runtime_get_sync(mas->dev);

    proto = geni_se_read_proto(se);
    ...
}

If the runtime resume fails (for example, due to power domain or clock
issues), the device hardware remains unpowered. The geni_se_read_proto() call
performs a memory-mapped I/O read. Reading from an unclocked or unpowered
register on ARM systems can trigger a synchronous external abort (SError),
crashing the entire system.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530-enable-spi-on-sa8255p-v2-0-17574601bd63@oss.qualcomm.com?part=2

[PATCH v2 3/4] spi: qcom-geni: Use resources helper APIs in runtime PM functions

[Praveen Talari]

To manage GENI serial engine resources during runtime power management,
drivers currently need to call functions for ICC, clock, and
SE resource operations in both suspend and resume paths, resulting in
code duplication across drivers.

The new geni_se_resources_activate() and geni_se_resources_deactivate()
helper APIs addresses this issue by providing a streamlined method to
enable or disable all resources based, thereby eliminating redundancy
across drivers.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/spi/spi-geni-qcom.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 333216df922e..7696412c6e87 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -1119,33 +1119,16 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
 {
 	struct spi_controller *spi = dev_get_drvdata(dev);
 	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
-	int ret;
-
-	/* Drop the performance state vote */
-	dev_pm_opp_set_rate(dev, 0);
-
-	ret = geni_se_resources_off(&mas->se);
-	if (ret)
-		return ret;
 
-	return geni_icc_disable(&mas->se);
+	return geni_se_resources_deactivate(&mas->se);
 }
 
 static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
 {
 	struct spi_controller *spi = dev_get_drvdata(dev);
 	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
-	int ret;
-
-	ret = geni_icc_enable(&mas->se);
-	if (ret)
-		return ret;
-
-	ret = geni_se_resources_on(&mas->se);
-	if (ret)
-		return ret;
 
-	return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);
+	return geni_se_resources_activate(&mas->se);
 }
 
 static int __maybe_unused spi_geni_suspend(struct device *dev)

-- 
2.34.1

Re: [PATCH v2 3/4] spi: qcom-geni: Use resources helper APIs in runtime PM functions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Performance state (OPP) vote is dropped during suspend but never restored on resume, leading to hardware operating at zero voltage.
--

commit 60a92c010d0b0fc6d9cc73f3af65a2635a04cf3c
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: qcom-geni: Use resources helper APIs in runtime PM functions

To manage GENI serial engine resources during runtime power management,
drivers currently need to call functions for ICC, clock, and
SE resource operations in both suspend and resume paths, resulting in
code duplication across drivers.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 333216df922e8..7696412c6e879 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -1119,33 +1119,16 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
>  {
>  	struct spi_controller *spi = dev_get_drvdata(dev);
>  	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
> -	int ret;
> -
> -	/* Drop the performance state vote */
> -	dev_pm_opp_set_rate(dev, 0);
> -
> -	ret = geni_se_resources_off(&mas->se);
> -	if (ret)
> -		return ret;
>  
> -	return geni_icc_disable(&mas->se);
> +	return geni_se_resources_deactivate(&mas->se);
>  }
>  
>  static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
>  {
>  	struct spi_controller *spi = dev_get_drvdata(dev);
>  	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
> -	int ret;
> -
> -	ret = geni_icc_enable(&mas->se);
> -	if (ret)
> -		return ret;
> -
> -	ret = geni_se_resources_on(&mas->se);
> -	if (ret)
> -		return ret;
>  
> -	return dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz);
> +	return geni_se_resources_activate(&mas->se);
>  }

[Severity: High]
How does the performance state (OPP) rate get restored on resume?

Before this patch, spi_geni_runtime_resume() explicitly restored the OPP
rate using dev_pm_opp_set_rate(mas->dev, mas->cur_sclk_hz).

While the new geni_se_resources_deactivate() helper continues to drop the
OPP rate to 0 on suspend, geni_se_resources_activate() does not restore it.
This seems to leave the hardware operating at zero voltage upon resume.

Furthermore, subsequent transfers won't reapply the rate because
geni_spi_set_clock_and_bw() has a fast-path optimization that skips setting
the clock if the requested frequency hasn't changed:

    if (clk_hz == mas->cur_speed_hz)
            return 0;

Will this lead to bus lockups or transfer timeouts due to operating without
adequate voltage votes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530-enable-spi-on-sa8255p-v2-0-17574601bd63@oss.qualcomm.com?part=3

[PATCH v2 4/4] spi: qcom-geni: Enable SPI on SA8255p Qualcomm platforms

[Praveen Talari]

The Qualcomm automotive SA8255p SoC relies on firmware to configure
platform resources, including clocks, interconnects and TLMM.
The driver requests resources operations over SCMI using power
and performance protocols.

The SCMI power protocol enables or disables resources like clocks,
interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
such as resume/suspend, to control power states(on/off).

The SCMI performance protocol manages SPI frequency, with each
frequency rate represented by a performance level. The driver uses
geni_se_set_perf_opp() API to request the desired frequency rate.

As part of geni_se_set_perf_opp(), the OPP for the requested frequency
is obtained using dev_pm_opp_find_freq_floor() and the performance
level is set using dev_pm_opp_set_opp().

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/spi/spi-geni-qcom.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 7696412c6e87..98e34e58fae2 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -78,6 +78,13 @@
 #define GSI_CPHA		BIT(4)
 #define GSI_CPOL		BIT(5)
 
+struct geni_spi_desc {
+	int (*resources_init)(struct geni_se *se);
+	int (*set_rate)(struct geni_se *se, unsigned long clk_freq);
+	int (*power_on)(struct geni_se *se);
+	int (*power_off)(struct geni_se *se);
+};
+
 struct spi_geni_master {
 	struct geni_se se;
 	struct device *dev;
@@ -105,6 +112,7 @@ struct spi_geni_master {
 	struct dma_chan *tx;
 	struct dma_chan *rx;
 	int cur_xfer_mode;
+	const struct geni_spi_desc *dev_data;
 };
 
 static void spi_slv_setup(struct spi_geni_master *mas)
@@ -305,11 +313,12 @@ static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
 	writel(word_len, se->base + SE_SPI_WORD_LEN);
 }
 
-static int geni_spi_set_clock_and_bw(struct spi_geni_master *mas,
-					unsigned long clk_hz)
+static int geni_spi_set_clock_and_bw(struct geni_se *se,
+				     unsigned long clk_hz)
 {
+	struct spi_controller *spi = dev_get_drvdata(se->dev);
+	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
 	u32 clk_sel, m_clk_cfg, idx, div;
-	struct geni_se *se = &mas->se;
 	int ret;
 
 	if (clk_hz == mas->cur_speed_hz)
@@ -820,7 +829,7 @@ static int setup_se_xfer(struct spi_transfer *xfer,
 	}
 
 	/* Speed and bits per word can be overridden per transfer */
-	ret = geni_spi_set_clock_and_bw(mas, xfer->speed_hz);
+	ret = mas->dev_data->set_rate(&mas->se, xfer->speed_hz);
 	if (ret)
 		return ret;
 
@@ -1064,7 +1073,9 @@ static int spi_geni_probe(struct platform_device *pdev)
 	mas->se.wrapper = dev_get_drvdata(dev->parent);
 	mas->se.base = base;
 
-	ret = geni_se_resources_init(&mas->se);
+	mas->dev_data = device_get_match_data(&pdev->dev);
+
+	ret = mas->dev_data->resources_init(&mas->se);
 	if (ret)
 		return ret;
 
@@ -1120,7 +1131,8 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
 	struct spi_controller *spi = dev_get_drvdata(dev);
 	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
 
-	return geni_se_resources_deactivate(&mas->se);
+	return mas->dev_data->power_off ?
+	       mas->dev_data->power_off(&mas->se) : 0;
 }
 
 static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
@@ -1128,7 +1140,8 @@ static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
 	struct spi_controller *spi = dev_get_drvdata(dev);
 	struct spi_geni_master *mas = spi_controller_get_devdata(spi);
 
-	return geni_se_resources_activate(&mas->se);
+	return mas->dev_data->power_on ?
+	       mas->dev_data->power_on(&mas->se) : 0;
 }
 
 static int __maybe_unused spi_geni_suspend(struct device *dev)
@@ -1169,8 +1182,21 @@ static const struct dev_pm_ops spi_geni_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(spi_geni_suspend, spi_geni_resume)
 };
 
+static const struct geni_spi_desc geni_spi = {
+	.resources_init = geni_se_resources_init,
+	.set_rate = geni_spi_set_clock_and_bw,
+	.power_on = geni_se_resources_activate,
+	.power_off = geni_se_resources_deactivate,
+};
+
+static const struct geni_spi_desc sa8255p_geni_spi = {
+	.resources_init = geni_se_domain_attach,
+	.set_rate = geni_se_set_perf_opp,
+};
+
 static const struct of_device_id spi_geni_dt_match[] = {
-	{ .compatible = "qcom,geni-spi" },
+	{ .compatible = "qcom,geni-spi", .data = &geni_spi },
+	{ .compatible = "qcom,sa8255p-geni-spi", .data = &sa8255p_geni_spi },
 	{}
 };
 MODULE_DEVICE_TABLE(of, spi_geni_dt_match);

-- 
2.34.1

Re: [PATCH v2 4/4] spi: qcom-geni: Enable SPI on SA8255p Qualcomm platforms

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] GPI DMA mode (`setup_gsi_xfer()`) bypasses the `set_rate` abstraction, breaking SPI transfers on the SA8255P platform.
--

commit 51c1e984c5666b6d16483f6836d8eccfa71a9fa2
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

spi: qcom-geni: Enable SPI on SA8255p Qualcomm platforms

The patch enables SPI for the SA8255P platform using SCMI power and
performance protocols. It introduces a dev_data struct with function
pointers to abstract initialization and rate-setting operations.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 7696412c6e879..98e34e58fae21 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c

[ ... ]

> @@ -820,7 +829,7 @@ static int setup_se_xfer(struct spi_transfer *xfer,
>  	}
>  
>  	/* Speed and bits per word can be overridden per transfer */
> -	ret = geni_spi_set_clock_and_bw(mas, xfer->speed_hz);
> +	ret = mas->dev_data->set_rate(&mas->se, xfer->speed_hz);

[Severity: High]
Should setup_gsi_xfer() also be updated to use this new set_rate abstraction?

Looking at setup_gsi_xfer(), it appears to still hardcode a call to
get_spi_clk_cfg():

drivers/spi/spi-geni-qcom.c:setup_gsi_xfer() {
	...
	peripheral.pack_en = true;
	peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN;

	ret = get_spi_clk_cfg(mas->cur_speed_hz, mas,
			      &peripheral.clk_src, &peripheral.clk_div);
	if (ret) {
		dev_err(mas->dev, "Err in get_spi_clk_cfg() :%d\n", ret);
		return ret;
	}
	...
}

Since SCMI-based platforms like SA8255P rely on geni_se_domain_attach()
which leaves se->clk uninitialized (NULL), get_spi_clk_cfg() will fail
when calling clk_round_rate() and geni_se_clk_freq_match().

Does this unconditional bypass of the set_rate abstraction break all
SPI transfers using GPI DMA on the SA8255P platform?

>  	if (ret)
>  		return ret;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530-enable-spi-on-sa8255p-v2-0-17574601bd63@oss.qualcomm.com?part=4