[PATCH 0/3] i2c: qup: Allow scaling power domains and interconnect

[PATCH 0/3] i2c: qup: Allow scaling power domains and interconnect

[Stephan Gerhold]

Make it possible to scale performance states of the power domain and
interconnect of the I2C QUP controller.

This is necessary to guarantee performance with power management
enabled. Otherwise these resources might run at minimal performance
state which is not sufficient for certain workloads.

Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
---
Stephan Gerhold (3):
      dt-bindings: i2c: qcom,i2c-qup: Document power-domains
      dt-bindings: i2c: qup: Document interconnects
      i2c: qup: Vote for interconnect bandwidth to DRAM

 .../devicetree/bindings/i2c/qcom,i2c-qup.yaml      | 14 +++++++++
 drivers/i2c/busses/i2c-qup.c                       | 36 ++++++++++++++++++++++
 2 files changed, 50 insertions(+)
---
base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
change-id: 20231106-i2c-qup-dvfs-bc60e2998dd8

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

[PATCH 1/3] dt-bindings: i2c: qcom,i2c-qup: Document power-domains

[Stephan Gerhold]

Similar to qcom,geni-i2c, for i2c-qup we need to vote for performance
states on the VDDCX power domain to ensure that required clock rates
can be generated correctly.

I2C is typically used with a fixed clock rate, so a single required-opp
is sufficient without a full OPP table (unlike spi-qup for example).

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

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.yaml
index f43947514d48..fc3077a7af0d 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.yaml
@@ -52,9 +52,15 @@ properties:
       - const: default
       - const: sleep
 
+  power-domains:
+    maxItems: 1
+
   reg:
     maxItems: 1
 
+  required-opps:
+    maxItems: 1
+
 required:
   - compatible
   - clock-names
@@ -68,6 +74,7 @@ examples:
   - |
     #include <dt-bindings/clock/qcom,gcc-msm8998.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
 
     i2c@c175000 {
         compatible = "qcom,i2c-qup-v2.2.1";
@@ -82,6 +89,8 @@ examples:
         pinctrl-names = "default", "sleep";
         pinctrl-0 = <&blsp1_i2c1_default>;
         pinctrl-1 = <&blsp1_i2c1_sleep>;
+        power-domains = <&rpmpd MSM8909_VDDCX>;
+        required-opps = <&rpmpd_opp_svs_krait>;
         clock-frequency = <400000>;
 
         #address-cells = <1>;

-- 
2.39.2

[PATCH 2/3] dt-bindings: i2c: qup: Document interconnects

[Stephan Gerhold]

When the I2C 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/i2c/qcom,i2c-qup.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.yaml
index fc3077a7af0d..758d8f6321e1 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-qup.yaml
@@ -40,6 +40,9 @@ properties:
       - const: tx
       - const: rx
 
+  interconnects:
+    maxItems: 1
+
   interrupts:
     maxItems: 1
 
@@ -73,6 +76,7 @@ unevaluatedProperties: false
 examples:
   - |
     #include <dt-bindings/clock/qcom,gcc-msm8998.h>
+    #include <dt-bindings/interconnect/qcom,msm8996.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
     #include <dt-bindings/power/qcom-rpmpd.h>
 
@@ -91,6 +95,7 @@ examples:
         pinctrl-1 = <&blsp1_i2c1_sleep>;
         power-domains = <&rpmpd MSM8909_VDDCX>;
         required-opps = <&rpmpd_opp_svs_krait>;
+        interconnects = <&pnoc MASTER_BLSP_1 &bimc SLAVE_EBI_CH0>;
         clock-frequency = <400000>;
 
         #address-cells = <1>;

-- 
2.39.2

[PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Stephan Gerhold]

When the I2C 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 I2C core clock.

To avoid sending votes too often the bandwidth is always requested when
a DMA 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 DMA transfer actually happens.

The implementation is largely identical to the one introduced for
spi-qup in commit ecdaa9473019 ("spi: qup: Vote for interconnect
bandwidth to DRAM") since both drivers represent the same hardware
block.

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.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6
---
 drivers/i2c/busses/i2c-qup.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 598102d16677..ee92a315f074 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -14,6 +14,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
+#include <linux/interconnect.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -150,6 +151,8 @@
 /* TAG length for DATA READ in RX FIFO  */
 #define READ_RX_TAGS_LEN		2
 
+#define QUP_BUS_WIDTH			8
+
 static unsigned int scl_freq;
 module_param_named(scl_freq, scl_freq, uint, 0444);
 MODULE_PARM_DESC(scl_freq, "SCL frequency override");
@@ -227,6 +230,7 @@ struct qup_i2c_dev {
 	int			irq;
 	struct clk		*clk;
 	struct clk		*pclk;
+	struct icc_path		*icc_path;
 	struct i2c_adapter	adap;
 
 	int			clk_ctl;
@@ -255,6 +259,10 @@ struct qup_i2c_dev {
 	/* To configure when bus is in run state */
 	u32			config_run;
 
+	/* bandwidth votes */
+	u32			src_clk_freq;
+	u32			cur_bw_clk_freq;
+
 	/* dma parameters */
 	bool			is_dma;
 	/* To check if the current transfer is using DMA */
@@ -453,6 +461,23 @@ static int qup_i2c_bus_active(struct qup_i2c_dev *qup, int len)
 	return ret;
 }
 
+static int qup_i2c_vote_bw(struct qup_i2c_dev *qup, u32 clk_freq)
+{
+	u32 needed_peak_bw;
+	int ret;
+
+	if (qup->cur_bw_clk_freq == clk_freq)
+		return 0;
+
+	needed_peak_bw = Bps_to_icc(clk_freq * QUP_BUS_WIDTH);
+	ret = icc_set_bw(qup->icc_path, 0, needed_peak_bw);
+	if (ret)
+		return ret;
+
+	qup->cur_bw_clk_freq = clk_freq;
+	return 0;
+}
+
 static void qup_i2c_write_tx_fifo_v1(struct qup_i2c_dev *qup)
 {
 	struct qup_i2c_block *blk = &qup->blk;
@@ -840,6 +865,10 @@ static int qup_i2c_bam_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
 	int ret = 0;
 	int idx = 0;
 
+	ret = qup_i2c_vote_bw(qup, qup->src_clk_freq);
+	if (ret)
+		return ret;
+
 	enable_irq(qup->irq);
 	ret = qup_i2c_req_dma(qup);
 
@@ -1645,6 +1674,7 @@ static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
 	config = readl(qup->base + QUP_CONFIG);
 	config |= QUP_CLOCK_AUTO_GATE;
 	writel(config, qup->base + QUP_CONFIG);
+	qup_i2c_vote_bw(qup, 0);
 	clk_disable_unprepare(qup->pclk);
 }
 
@@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev)
 			goto fail_dma;
 		}
 		qup->is_dma = true;
+
+		qup->icc_path = devm_of_icc_get(&pdev->dev, NULL);
+		if (IS_ERR(qup->icc_path))
+			return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path),
+					     "failed to get interconnect path\n");
 	}
 
 nodma:
@@ -1793,6 +1828,7 @@ static int qup_i2c_probe(struct platform_device *pdev)
 		qup_i2c_enable_clocks(qup);
 		src_clk_freq = clk_get_rate(qup->clk);
 	}
+	qup->src_clk_freq = src_clk_freq;
 
 	/*
 	 * Bootloaders might leave a pending interrupt on certain QUP's,

-- 
2.39.2

Re: [PATCH 1/3] dt-bindings: i2c: qcom,i2c-qup: Document power-domains

[Rob Herring]

On Tue, 28 Nov 2023 10:48:35 +0100, Stephan Gerhold wrote:
> Similar to qcom,geni-i2c, for i2c-qup we need to vote for performance
> states on the VDDCX power domain to ensure that required clock rates
> can be generated correctly.
> 
> I2C is typically used with a fixed clock rate, so a single required-opp
> is sufficient without a full OPP table (unlike spi-qup for example).
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
> ---
>  Documentation/devicetree/bindings/i2c/qcom,i2c-qup.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/3] dt-bindings: i2c: qup: Document interconnects

[Rob Herring]

On Tue, 28 Nov 2023 10:48:36 +0100, Stephan Gerhold wrote:
> When the I2C 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/i2c/qcom,i2c-qup.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Andi Shyti]

Hi Stephan,

On Tue, Nov 28, 2023 at 10:48:37AM +0100, Stephan Gerhold wrote:
> When the I2C 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 I2C core clock.
> 
> To avoid sending votes too often the bandwidth is always requested when
> a DMA 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 DMA transfer actually happens.
> 
> The implementation is largely identical to the one introduced for
> spi-qup in commit ecdaa9473019 ("spi: qup: Vote for interconnect
> bandwidth to DRAM") since both drivers represent the same hardware
> block.
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>

the patch looks good to me.

> ---
> 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.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6

Krzysztof, any chance you can help here?

Thanks,
Andi

Re: [PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Andi Shyti]

Hi Stephen,

sorry for the very late reply here. Just one question.

...

> 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...

Can we have someone from Qualcomm or Linaro taking a peak here?

> [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6

...

> @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev)
>  			goto fail_dma;
>  		}
>  		qup->is_dma = true;
> +
> +		qup->icc_path = devm_of_icc_get(&pdev->dev, NULL);
> +		if (IS_ERR(qup->icc_path))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path),
> +					     "failed to get interconnect path\n");

Can we live without it if it fails?

Thanks,
Andi

Re: [PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Andi Shyti]

Sorry for replying to my own mail, bu I needed to fix Stephan and
Konrad's emails.

On Wed, Feb 19, 2025 at 12:02:11AM +0100, Andi Shyti wrote:
> Hi Stephen,
> 
> sorry for the very late reply here. Just one question.
> 
> ...
> 
> > 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...
> 
> Can we have someone from Qualcomm or Linaro taking a peak here?
> 
> > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6
> 
> ...
> 
> > @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev)
> >  			goto fail_dma;
> >  		}
> >  		qup->is_dma = true;
> > +
> > +		qup->icc_path = devm_of_icc_get(&pdev->dev, NULL);
> > +		if (IS_ERR(qup->icc_path))
> > +			return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path),
> > +					     "failed to get interconnect path\n");
> 
> Can we live without it if it fails?
> 
> Thanks,
> Andi

Re: [PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Krzysztof Kozlowski]

On 19/02/2025 00:02, Andi Shyti wrote:
> Hi Stephen,
> 
> sorry for the very late reply here. Just one question.
> 
> ...
> 
>> 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...
> 
> Can we have someone from Qualcomm or Linaro taking a peak here?

You replied to some old email, not in my inbox anymore, but your quote
lacks standard quote-template, like:

	On 19/02/2025 00:02, Andi Shyti wrote:

so I really don't know when was it sent. For sure more than a month ago,
maybe more? This has to be resent if you want anything done here.

Best regards,
Krzysztof

Re: [PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Stephan Gerhold]

Hi Andi,

On Wed, Feb 19, 2025 at 12:02:06AM +0100, Andi Shyti wrote:
> 
> sorry for the very late reply here. Just one question.
> 

Thanks for bringing the patch back up after such a long time. I've been
meaning to resend it, but never found the time to do so... :-)

> 
> > 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...
> 
> Can we have someone from Qualcomm or Linaro taking a peak here?
> 

I suppose I count as someone from Linaro nowadays. However, since this
driver is only used on really old platforms nowadays, I'm not sure where
to look or who to ask...

At the end, the whole bus scaling/interconnect is always somewhat
"imprecise". There is no clear "correct" or "wrong", since the ideal
bandwidth depends heavily on the actual use case that we are not aware
of in the driver. There is also overhead when voting for bandwidth,
since that can take a couple of milliseconds.

The most important part is that we vote for any bandwidth at all, since
otherwise the bus path could potentially be completely off and it would
get stuck. My patch implements one of the approaches that was used in
the downstream/vendor drivers and matches what we already have upstream
in the corresponding spi-qup driver. I think it's "good enough". If
someone ever wants to fine tune this based on actual measurements they
can just submit an incremental patch. Right now this series is blocking
adding the necessary properties in the device tree and that's not good.

Surprisingly this series still applies cleanly on top of linux-next. The
dt-bindings have review tags and there was plenty of time for someone
else to chime in for the driver. So maybe you can just pick them up? :D

> > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6
> 
> ...
> 
> > @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev)
> >  			goto fail_dma;
> >  		}
> >  		qup->is_dma = true;
> > +
> > +		qup->icc_path = devm_of_icc_get(&pdev->dev, NULL);
> > +		if (IS_ERR(qup->icc_path))
> > +			return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path),
> > +					     "failed to get interconnect path\n");
> 
> Can we live without it if it fails?
> 

of_icc_get() returns NULL if the interconnect API is disabled, or if
"interconnects" is not defined in the device tree, so this is already
handled. If "interconnects" is enabled and defined, I think we shouldn't
ignore errors. Therefore, this should work as intended.

Let me know if I should resend the patch or if you can apply it
directly.

Thanks,
Stephan

Re: [PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Andi Shyti]

Hi Stephan,

On Wed, Feb 19, 2025 at 11:40:16AM +0100, Stephan Gerhold wrote:
> Hi Andi,
> 
> On Wed, Feb 19, 2025 at 12:02:06AM +0100, Andi Shyti wrote:
> > 
> > sorry for the very late reply here. Just one question.
> > 
> 
> Thanks for bringing the patch back up after such a long time. I've been
> meaning to resend it, but never found the time to do so... :-)

We have a long list of forgotten patches that belong to the far
past. I'm trying to revive them.

> > > 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...
> > 
> > Can we have someone from Qualcomm or Linaro taking a peak here?
> > 
> 
> I suppose I count as someone from Linaro nowadays. However, since this
> driver is only used on really old platforms nowadays, I'm not sure where
> to look or who to ask...
> 
> At the end, the whole bus scaling/interconnect is always somewhat
> "imprecise". There is no clear "correct" or "wrong", since the ideal
> bandwidth depends heavily on the actual use case that we are not aware
> of in the driver. There is also overhead when voting for bandwidth,
> since that can take a couple of milliseconds.
> 
> The most important part is that we vote for any bandwidth at all, since
> otherwise the bus path could potentially be completely off and it would
> get stuck. My patch implements one of the approaches that was used in
> the downstream/vendor drivers and matches what we already have upstream
> in the corresponding spi-qup driver. I think it's "good enough". If
> someone ever wants to fine tune this based on actual measurements they
> can just submit an incremental patch. Right now this series is blocking
> adding the necessary properties in the device tree and that's not good.
> 
> Surprisingly this series still applies cleanly on top of linux-next. The
> dt-bindings have review tags and there was plenty of time for someone
> else to chime in for the driver. So maybe you can just pick them up? :D

Yes, I already tested them.

> > > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.10/-/commit/67174e2624ea64814231e7e1e4af83fd882302c6
> > 
> > ...
> > 
> > > @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev)
> > >  			goto fail_dma;
> > >  		}
> > >  		qup->is_dma = true;
> > > +
> > > +		qup->icc_path = devm_of_icc_get(&pdev->dev, NULL);
> > > +		if (IS_ERR(qup->icc_path))
> > > +			return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path),
> > > +					     "failed to get interconnect path\n");
> > 
> > Can we live without it if it fails?
> > 
> 
> of_icc_get() returns NULL if the interconnect API is disabled, or if
> "interconnects" is not defined in the device tree, so this is already
> handled. If "interconnects" is enabled and defined, I think we shouldn't
> ignore errors. Therefore, this should work as intended.

yes, because qup_i2c_vote_bw() checks inside for NULL values.

My idea was that:

	if (IS_ERR(...)) {
		dev_warn(...)
		qup->icc_path = NULL;
	}

and let things work. Anyway, if you want to keep it this way,
fine with me, I don't have a strong opinion, rather than a
preference to keep going.

Thanks,
Andi

> Let me know if I should resend the patch or if you can apply it
> directly.
> 
> Thanks,
> Stephan

Re: [PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Andi Shyti]

Hi Krzysztof,

On Wed, Feb 19, 2025 at 08:00:25AM +0100, Krzysztof Kozlowski wrote:
> On 19/02/2025 00:02, Andi Shyti wrote:
> > sorry for the very late reply here. Just one question.
> > 
> > ...
> > 
> >> 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...
> > 
> > Can we have someone from Qualcomm or Linaro taking a peak here?
> 
> You replied to some old email, not in my inbox anymore,

feeling nostalgic :-)

> but your quote
> lacks standard quote-template, like:
> 
> 	On 19/02/2025 00:02, Andi Shyti wrote:

I'm strictly following RFC-1855, but you're right I removed a bit
too much to lose time reference.

> so I really don't know when was it sent. For sure more than a month ago,
> maybe more? This has to be resent if you want anything done here.

It was sent on "Tue, 28 Nov 2023 10:48:34 +0100", definitely more
than a month ago, I'm also surprised to have it in my inbox. But
it still applies cleanly.

Perhaps a resend can invite people for more reviews. I don't
mind.

Thanks,
Andi

Re: [PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Stephan Gerhold]

On Wed, Feb 19, 2025 at 08:30:35PM +0100, Andi Shyti wrote:
> On Wed, Feb 19, 2025 at 11:40:16AM +0100, Stephan Gerhold wrote:
> > On Wed, Feb 19, 2025 at 12:02:06AM +0100, Andi Shyti wrote:
> > > 
> > > sorry for the very late reply here. Just one question.
> > > 
> > 
> > Thanks for bringing the patch back up after such a long time. I've been
> > meaning to resend it, but never found the time to do so... :-)
> 
> We have a long list of forgotten patches that belong to the far
> past. I'm trying to revive them.
> 

Thanks, this is much appreciated!

> [...]
> > > > @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev)
> > > >  			goto fail_dma;
> > > >  		}
> > > >  		qup->is_dma = true;
> > > > +
> > > > +		qup->icc_path = devm_of_icc_get(&pdev->dev, NULL);
> > > > +		if (IS_ERR(qup->icc_path))
> > > > +			return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path),
> > > > +					     "failed to get interconnect path\n");
> > > 
> > > Can we live without it if it fails?
> > > 
> > 
> > of_icc_get() returns NULL if the interconnect API is disabled, or if
> > "interconnects" is not defined in the device tree, so this is already
> > handled. If "interconnects" is enabled and defined, I think we shouldn't
> > ignore errors. Therefore, this should work as intended.
> 
> yes, because qup_i2c_vote_bw() checks inside for NULL values.
> 
> My idea was that:
> 
> 	if (IS_ERR(...)) {
> 		dev_warn(...)
> 		qup->icc_path = NULL;
> 	}
> 
> and let things work. Anyway, if you want to keep it this way,
> fine with me, I don't have a strong opinion, rather than a
> preference to keep going.

I would prefer to keep it the way it is. It's okay to omit the
"interconnects" in the DT (either for old device trees, or because you
don't define the "dmas" either). But if they are defined, we should not
be ignoring errors. -EPROBE_DEFER definitely needs to be handled, but
even for -EINVAL or similar it would be better to make it obvious in my
opinion.

None of the existing users should be affected, since no one defines
"interconnects" at the moment.

Thanks,
Stephan

Re: [PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Konrad Dybcio]

On 19.02.2025 11:40 AM, Stephan Gerhold wrote:
> Hi Andi,
> 
> On Wed, Feb 19, 2025 at 12:02:06AM +0100, Andi Shyti wrote:
>>
>> sorry for the very late reply here. Just one question.
>>
> 
> Thanks for bringing the patch back up after such a long time. I've been
> meaning to resend it, but never found the time to do so... :-)
> 
>>
>>> 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...
>>
>> Can we have someone from Qualcomm or Linaro taking a peak here?
>>
> 
> I suppose I count as someone from Linaro nowadays. However, since this
> driver is only used on really old platforms nowadays, I'm not sure where
> to look or who to ask...
> 
> At the end, the whole bus scaling/interconnect is always somewhat
> "imprecise". There is no clear "correct" or "wrong", since the ideal
> bandwidth depends heavily on the actual use case that we are not aware
> of in the driver. There is also overhead when voting for bandwidth,
> since that can take a couple of milliseconds.
> 
> The most important part is that we vote for any bandwidth at all, since
> otherwise the bus path could potentially be completely off and it would
> get stuck. My patch implements one of the approaches that was used in
> the downstream/vendor drivers and matches what we already have upstream
> in the corresponding spi-qup driver. I think it's "good enough". If
> someone ever wants to fine tune this based on actual measurements they
> can just submit an incremental patch. Right now this series is blocking
> adding the necessary properties in the device tree and that's not good.

Yeah, the throughput of an I2C controller isn't even very likely to affect
the total bus frequency requirement, although it's a strict requirement
that the requested bw is nonzero (otherwise the bus may be clock-gated)

Konrad

Re: [PATCH 0/3] i2c: qup: Allow scaling power domains and interconnect

[Andi Shyti]

Hi Stephen,

On Tue, Nov 28, 2023 at 10:48:34AM +0100, Stephan Gerhold wrote:
> Make it possible to scale performance states of the power domain and
> interconnect of the I2C QUP controller.
> 
> This is necessary to guarantee performance with power management
> enabled. Otherwise these resources might run at minimal performance
> state which is not sufficient for certain workloads.
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>

merged to i2c/i2c-host.

Thanks,
Andi