[PATCH 0/4] spi: qup: Allow scaling power domains and interconnect

[PATCH 0/4] spi: qup: Allow scaling power domains and interconnect

[Stephan Gerhold]

Make it possible to scale performance states of the power domain and
interconnect of the SPI QUP controller in relation to the selected SPI
speed / core clock. This is done separately by:

  - Parsing the OPP table from the device tree for performance state
    votes of the power domain
  - Voting for the necessary bandwidth on the interconnect path to DRAM

Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
---
Stephan Gerhold (4):
      spi: dt-bindings: qup: Document power-domains and OPP
      spi: qup: Parse OPP table for DVFS support
      spi: dt-bindings: qup: Document interconnects
      spi: qup: Vote for interconnect bandwidth to DRAM

 .../devicetree/bindings/spi/qcom,spi-qup.yaml      | 13 ++++++
 drivers/spi/spi-qup.c                              | 50 +++++++++++++++++++++-
 2 files changed, 62 insertions(+), 1 deletion(-)
---
base-commit: 678466ba68915d452c200b78d0385931e6f8e907
change-id: 20230912-spi-qup-dvfs-71fc8a5e0cb1

Best regards,
-- 
Stephan Gerhold <stephan.gerhold@kernkonzept.com>
Kernkonzept GmbH at Dresden, Germany, HRB 31129, CEO Dr.-Ing. Michael Hohmuth

[PATCH 1/4] spi: dt-bindings: qup: Document power-domains and OPP

[Stephan Gerhold]

Document power-domains and operating-points-v2 to allow making
performance state votes for certain clock frequencies of the SPI QUP
controller.

Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
---
 Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml
index 93f14dd01afc..1e498a791406 100644
--- a/Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml
@@ -47,6 +47,11 @@ properties:
   interrupts:
     maxItems: 1
 
+  operating-points-v2: true
+
+  power-domains:
+    maxItems: 1
+
   reg:
     maxItems: 1
 
@@ -63,6 +68,7 @@ examples:
   - |
     #include <dt-bindings/clock/qcom,gcc-msm8996.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
 
     spi@7575000 {
         compatible = "qcom,spi-qup-v2.2.1";
@@ -76,6 +82,8 @@ examples:
         pinctrl-1 = <&blsp1_spi1_sleep>;
         dmas = <&blsp1_dma 12>, <&blsp1_dma 13>;
         dma-names = "tx", "rx";
+        power-domains = <&rpmpd MSM8996_VDDCX>;
+        operating-points-v2 = <&spi_opp_table>;
         #address-cells = <1>;
         #size-cells = <0>;
     };

-- 
2.39.2

Re: [PATCH 1/4] spi: dt-bindings: qup: Document power-domains and OPP

[Krzysztof Kozlowski]

On 12/09/2023 16:30, Stephan Gerhold wrote:
> Document power-domains and operating-points-v2 to allow making
> performance state votes for certain clock frequencies of the SPI QUP
> controller.
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
> ---
>  Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml | 8 +++++++

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

Best regards,
Krzysztof

[PATCH 2/4] spi: qup: Parse OPP table for DVFS support

[Stephan Gerhold]

Parse the OPP table from the device tree and use dev_pm_opp_set_rate()
instead of clk_set_rate() to allow making performance state for power
domains specified in the OPP table.

This is needed to guarantee correct behavior of the clock, especially
with the higher clock/SPI bus frequencies.

Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
---
 drivers/spi/spi-qup.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index 4b6f6b25219b..bf043be3a2a9 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
 #include <linux/dmaengine.h>
@@ -667,7 +668,7 @@ static int spi_qup_io_prep(struct spi_device *spi, struct spi_transfer *xfer)
 		return -EIO;
 	}
 
-	ret = clk_set_rate(controller->cclk, xfer->speed_hz);
+	ret = dev_pm_opp_set_rate(controller->dev, xfer->speed_hz);
 	if (ret) {
 		dev_err(controller->dev, "fail to set frequency %d",
 			xfer->speed_hz);
@@ -1027,6 +1028,15 @@ static int spi_qup_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
+	ret = devm_pm_opp_set_clkname(dev, "core");
+	if (ret)
+		return ret;
+
+	/* OPP table is optional */
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "invalid OPP table\n");
+
 	host = spi_alloc_host(dev, sizeof(struct spi_qup));
 	if (!host) {
 		dev_err(dev, "cannot allocate host\n");

-- 
2.39.2

Re: [PATCH 2/4] spi: qup: Parse OPP table for DVFS support

[Konrad Dybcio]

On 12.09.2023 16:30, Stephan Gerhold wrote:
> Parse the OPP table from the device tree and use dev_pm_opp_set_rate()
> instead of clk_set_rate() to allow making performance state for power
> domains specified in the OPP table.
> 
> This is needed to guarantee correct behavior of the clock, especially
> with the higher clock/SPI bus frequencies.
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
> ---
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH 3/4] spi: dt-bindings: qup: Document interconnects

[Stephan Gerhold]

When the SPI QUP controller is used together with a DMA engine it needs
to vote for the interconnect path to the DRAM. Otherwise it may be
unable to access the memory quickly enough.

Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
---
 Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml
index 1e498a791406..88be13268962 100644
--- a/Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml
+++ b/Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml
@@ -44,6 +44,9 @@ properties:
       - const: tx
       - const: rx
 
+  interconnects:
+    maxItems: 1
+
   interrupts:
     maxItems: 1
 
@@ -67,6 +70,7 @@ unevaluatedProperties: false
 examples:
   - |
     #include <dt-bindings/clock/qcom,gcc-msm8996.h>
+    #include <dt-bindings/interconnect/qcom,msm8996.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
     #include <dt-bindings/power/qcom-rpmpd.h>
 
@@ -84,6 +88,7 @@ examples:
         dma-names = "tx", "rx";
         power-domains = <&rpmpd MSM8996_VDDCX>;
         operating-points-v2 = <&spi_opp_table>;
+        interconnects = <&pnoc MASTER_BLSP_1 &bimc SLAVE_EBI_CH0>;
         #address-cells = <1>;
         #size-cells = <0>;
     };

-- 
2.39.2

Re: [PATCH 3/4] spi: dt-bindings: qup: Document interconnects

[Krzysztof Kozlowski]

On 12/09/2023 16:30, Stephan Gerhold wrote:
> When the SPI QUP controller is used together with a DMA engine it needs
> to vote for the interconnect path to the DRAM. Otherwise it may be
> unable to access the memory quickly enough.
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
> ---
>  Documentation/devicetree/bindings/spi/qcom,spi-qup.yaml | 5 +++++
>  1 file changed, 5 insertions(+)

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

Best regards,
Krzysztof

[PATCH 4/4] spi: qup: Vote for interconnect bandwidth to DRAM

[Stephan Gerhold]

When the SPI QUP controller is used together with a DMA engine it needs
to vote for the interconnect path to the DRAM. Otherwise it may be
unable to access the memory quickly enough.

The requested peak bandwidth is dependent on the SPI core/bus clock so
that the bandwidth scales together with the selected SPI speed.

To avoid sending votes too often the bandwidth is always requested when
a transfer starts, but dropped only on runtime suspend. Runtime suspend
should only happen if no transfer is active. After resumption we can
defer the next vote until the first transfer actually happens.

Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
---
The bandwidth calculation is taken over from Qualcomm's
downstream/vendor driver [1]. Due to lack of documentation about the
interconnect setup/behavior I cannot say exactly if this is right.
Unfortunately, this is not implemented very consistently downstream...

[1]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/deca0f346089d32941d6d8194ae9605554486413
---
 drivers/spi/spi-qup.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index bf043be3a2a9..e9c186bc530c 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -6,6 +6,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/interconnect.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/list.h>
@@ -122,11 +123,14 @@
 #define SPI_DELAY_THRESHOLD		1
 #define SPI_DELAY_RETRY			10
 
+#define SPI_BUS_WIDTH			8
+
 struct spi_qup {
 	void __iomem		*base;
 	struct device		*dev;
 	struct clk		*cclk;	/* core clock */
 	struct clk		*iclk;	/* interface clock */
+	struct icc_path		*icc_path; /* interconnect to RAM */
 	int			irq;
 	spinlock_t		lock;
 
@@ -149,6 +153,8 @@ struct spi_qup {
 	int			mode;
 	struct dma_slave_config	rx_conf;
 	struct dma_slave_config	tx_conf;
+
+	u32			bw_speed_hz;
 };
 
 static int spi_qup_io_config(struct spi_device *spi, struct spi_transfer *xfer);
@@ -181,6 +187,23 @@ static inline bool spi_qup_is_valid_state(struct spi_qup *controller)
 	return opstate & QUP_STATE_VALID;
 }
 
+static int spi_qup_vote_bw(struct spi_qup *controller, u32 speed_hz)
+{
+	u32 needed_peak_bw;
+	int ret;
+
+	if (controller->bw_speed_hz == speed_hz)
+		return 0;
+
+	needed_peak_bw = Bps_to_icc(speed_hz * SPI_BUS_WIDTH);
+	ret = icc_set_bw(controller->icc_path, 0, needed_peak_bw);
+	if (ret)
+		return ret;
+
+	controller->bw_speed_hz = speed_hz;
+	return 0;
+}
+
 static int spi_qup_set_state(struct spi_qup *controller, u32 state)
 {
 	unsigned long loop;
@@ -675,6 +698,12 @@ static int spi_qup_io_prep(struct spi_device *spi, struct spi_transfer *xfer)
 		return -EIO;
 	}
 
+	ret = spi_qup_vote_bw(controller, xfer->speed_hz);
+	if (ret) {
+		dev_err(controller->dev, "fail to vote for ICC bandwidth: %d\n", ret);
+		return -EIO;
+	}
+
 	controller->w_size = DIV_ROUND_UP(xfer->bits_per_word, 8);
 	controller->n_words = xfer->len / controller->w_size;
 
@@ -994,6 +1023,7 @@ static void spi_qup_set_cs(struct spi_device *spi, bool val)
 static int spi_qup_probe(struct platform_device *pdev)
 {
 	struct spi_controller *host;
+	struct icc_path *icc_path;
 	struct clk *iclk, *cclk;
 	struct spi_qup *controller;
 	struct resource *res;
@@ -1019,6 +1049,11 @@ static int spi_qup_probe(struct platform_device *pdev)
 	if (IS_ERR(iclk))
 		return PTR_ERR(iclk);
 
+	icc_path = devm_of_icc_get(dev, NULL);
+	if (IS_ERR(icc_path))
+		return dev_err_probe(dev, PTR_ERR(icc_path),
+				     "failed to get interconnect path\n");
+
 	/* This is optional parameter */
 	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
 		max_freq = SPI_MAX_RATE;
@@ -1070,6 +1105,7 @@ static int spi_qup_probe(struct platform_device *pdev)
 	controller->base = base;
 	controller->iclk = iclk;
 	controller->cclk = cclk;
+	controller->icc_path = icc_path;
 	controller->irq = irq;
 
 	ret = spi_qup_init_dma(host, res->start);
@@ -1190,6 +1226,7 @@ static int spi_qup_pm_suspend_runtime(struct device *device)
 	writel_relaxed(config, controller->base + QUP_CONFIG);
 
 	clk_disable_unprepare(controller->cclk);
+	spi_qup_vote_bw(controller, 0);
 	clk_disable_unprepare(controller->iclk);
 
 	return 0;
@@ -1241,6 +1278,7 @@ static int spi_qup_suspend(struct device *device)
 		return ret;
 
 	clk_disable_unprepare(controller->cclk);
+	spi_qup_vote_bw(controller, 0);
 	clk_disable_unprepare(controller->iclk);
 	return 0;
 }

-- 
2.39.2

Re: [PATCH 4/4] spi: qup: Vote for interconnect bandwidth to DRAM

[Stephan Gerhold]

On Tue, Sep 12, 2023 at 04:30:39PM +0200, Stephan Gerhold wrote:
> When the SPI QUP controller is used together with a DMA engine it needs
> to vote for the interconnect path to the DRAM. Otherwise it may be
> unable to access the memory quickly enough.

I realized that I argue here that the interconnect vote is for DMA to
DRAM...

> [...]
> @@ -675,6 +698,12 @@ static int spi_qup_io_prep(struct spi_device *spi, struct spi_transfer *xfer)
>  		return -EIO;
>  	}
>  
> +	ret = spi_qup_vote_bw(controller, xfer->speed_hz);
> +	if (ret) {
> +		dev_err(controller->dev, "fail to vote for ICC bandwidth: %d\n", ret);
> +		return -EIO;
> +	}
> +

... but here I vote for the bandwidth even if PIO is used instead of DMA.

I think it would be more logical to only do the bandwidth vote in the
DMA setup path. I'll fix this in v2.

Thanks,
Stephan