[V3,7/8] spi: spi-qcom-qspi: Add interconnect support
diff mbox series

Message ID 1585652976-17481-8-git-send-email-akashast@codeaurora.org
State Superseded
Headers show
Series
  • Add interconnect support to QSPI and QUP drivers
Related show

Commit Message

Akash Asthana March 31, 2020, 11:09 a.m. UTC
Get the interconnect paths for QSPI device and vote according to the
current bus speed of the driver.

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in V2:
 - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting
   path handle
 - As per Matthias comment, added error handling for icc_set_bw call

Changes in V3:
 - No Change.

 drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

Comments

Mark Brown March 31, 2020, 11:23 a.m. UTC | #1
On Tue, Mar 31, 2020 at 04:39:35PM +0530, Akash Asthana wrote:

> +	/*
> +	 * Set BW quota for CPU as driver supports FIFO mode only.
> +	 * Assume peak bw as twice of avg bw.
> +	 */
> +	ctrl->avg_bw_cpu = Bps_to_icc(speed_hz);
> +	ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz);

I thought you were going to factor this best guess handling of peak
bandwidth out into the core?
Matthias Kaehlcke March 31, 2020, 7:45 p.m. UTC | #2
On Tue, Mar 31, 2020 at 04:39:35PM +0530, Akash Asthana wrote:
> Get the interconnect paths for QSPI device and vote according to the
> current bus speed of the driver.
> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V2:
>  - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting
>    path handle
>  - As per Matthias comment, added error handling for icc_set_bw call
> 
> Changes in V3:
>  - No Change.
> 
>  drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
> index 3c4f83b..ad48f43 100644
> --- a/drivers/spi/spi-qcom-qspi.c
> +++ b/drivers/spi/spi-qcom-qspi.c
> @@ -2,6 +2,7 @@
>  // Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
>  
>  #include <linux/clk.h>
> +#include <linux/interconnect.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -139,7 +140,10 @@ struct qcom_qspi {
>  	struct device *dev;
>  	struct clk_bulk_data *clks;
>  	struct qspi_xfer xfer;
> -	/* Lock to protect xfer and IRQ accessed registers */
> +	struct icc_path *icc_path_cpu_to_qspi;
> +	unsigned int avg_bw_cpu;
> +	unsigned int peak_bw_cpu;
> +	/* Lock to protect data accessed by IRQs */
>  	spinlock_t lock;
>  };
>  
> @@ -241,6 +245,20 @@ static int qcom_qspi_transfer_one(struct spi_master *master,
>  		return ret;
>  	}
>  
> +	/*
> +	 * Set BW quota for CPU as driver supports FIFO mode only.
> +	 * Assume peak bw as twice of avg bw.
> +	 */
> +	ctrl->avg_bw_cpu = Bps_to_icc(speed_hz);
> +	ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz);
> +	ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, ctrl->avg_bw_cpu,
> +		ctrl->peak_bw_cpu);
> +	if (ret) {
> +		dev_err(ctrl->dev, "%s: ICC BW voting failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}
> +
>  	spin_lock_irqsave(&ctrl->lock, flags);
>  
>  	/* We are half duplex, so either rx or tx will be set */
> @@ -458,6 +476,15 @@ static int qcom_qspi_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto exit_probe_master_put;
>  
> +	ctrl->icc_path_cpu_to_qspi = devm_of_icc_get(dev, "qspi-config");
> +	if (IS_ERR(ctrl->icc_path_cpu_to_qspi)) {
> +		ret = PTR_ERR(ctrl->icc_path_cpu_to_qspi);
> +		goto exit_probe_master_put;
> +	}
> +	/* Put BW vote on CPU path for register access */
> +	ctrl->avg_bw_cpu = Bps_to_icc(1000);
> +	ctrl->peak_bw_cpu = Bps_to_icc(1000);
> +
>  	ret = platform_get_irq(pdev, 0);
>  	if (ret < 0)
>  		goto exit_probe_master_put;
> @@ -511,9 +538,17 @@ static int __maybe_unused qcom_qspi_runtime_suspend(struct device *dev)
>  {
>  	struct spi_master *master = dev_get_drvdata(dev);
>  	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
> +	int ret;
>  
>  	clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks);
>  
> +	ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, 0, 0);
> +	if (ret) {
> +		dev_err_ratelimited(ctrl->dev, "%s: ICC BW remove failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -521,6 +556,15 @@ static int __maybe_unused qcom_qspi_runtime_resume(struct device *dev)
>  {
>  	struct spi_master *master = dev_get_drvdata(dev);
>  	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
> +	int ret;
> +
> +	ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, ctrl->avg_bw_cpu,
> +		ctrl->peak_bw_cpu);
> +	if (ret) {
> +		dev_err_ratelimited(ctrl->dev, "%s: ICC BW voting failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}
>  
>  	return clk_bulk_prepare_enable(QSPI_NUM_CLKS, ctrl->clks);
>  }

Looks good to me besides Mark's concern about the bandwith calculation logic.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Akash Asthana April 7, 2020, 9:54 a.m. UTC | #3
Hi Mark,

On 3/31/2020 4:53 PM, Mark Brown wrote:
> On Tue, Mar 31, 2020 at 04:39:35PM +0530, Akash Asthana wrote:
>
>> +	/*
>> +	 * Set BW quota for CPU as driver supports FIFO mode only.
>> +	 * Assume peak bw as twice of avg bw.
>> +	 */
>> +	ctrl->avg_bw_cpu = Bps_to_icc(speed_hz);
>> +	ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz);
> I thought you were going to factor this best guess handling of peak
> bandwidth out into the core?

I can centralize this for SPI, I2C and UART  in Common driver(QUP 
wrapper) but still for QSPI I have to keep this piece of code as is 
because It is not child of QUP wrapper(it doesn't use common code).

I am not sure whether I can move this " Assume peak_bw as twice of 
avg_bw if nothing is mentioned explicitly" to ICC core because the 
factor of 2 is chosen randomly by me.

Regards,

Akash
Mark Brown April 7, 2020, 10:55 a.m. UTC | #4
On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote:
> On 3/31/2020 4:53 PM, Mark Brown wrote:

> > > +	ctrl->avg_bw_cpu = Bps_to_icc(speed_hz);
> > > +	ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz);

> > I thought you were going to factor this best guess handling of peak
> > bandwidth out into the core?

> I can centralize this for SPI, I2C and UART  in Common driver(QUP wrapper)
> but still for QSPI I have to keep this piece of code as is because It is not
> child of QUP wrapper(it doesn't use common code).

Why not?

> I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if
> nothing is mentioned explicitly" to ICC core because the factor of 2 is
> chosen randomly by me.

That's the whole point - if this is just a random number then we may as
well at least be consistently random.
Akash Asthana April 8, 2020, 12:17 p.m. UTC | #5
Hi Mark, Evan, Georgi,

On 4/7/2020 4:25 PM, Mark Brown wrote:
> On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote:
>> On 3/31/2020 4:53 PM, Mark Brown wrote:
>>>> +	ctrl->avg_bw_cpu = Bps_to_icc(speed_hz);
>>>> +	ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz);
>>> I thought you were going to factor this best guess handling of peak
>>> bandwidth out into the core?
>> I can centralize this for SPI, I2C and UART  in Common driver(QUP wrapper)
>> but still for QSPI I have to keep this piece of code as is because It is not
>> child of QUP wrapper(it doesn't use common code).
> Why not?
>
>> I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if
>> nothing is mentioned explicitly" to ICC core because the factor of 2 is
>> chosen randomly by me.
> That's the whole point - if this is just a random number then we may as
> well at least be consistently random.

Can we centralize below logic of peak_bw selection for all the clients 
to ICC core?

"Assume peak_bw requirement as twice of avg_bw, if it is not mentioned 
explicitly"

===========================================================================
int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
{
         struct icc_node *node;
         u32 old_avg, old_peak;
         size_t i;
         int ret;

         if (!path)
                 return 0;

         if (WARN_ON(IS_ERR(path) || !path->num_nodes))
                 return -EINVAL;

+       /*
+        * Assume peak_bw requirement as twice of avg_bw, if it is not
+        * mentioned explicitly
+        */
+       peak_bw = peak_bw ? peak_bw : 2 * avg_bw;
===========================================================================

In case if some client really don't want to put peak requirement they 
can pass avg_bw = peak_bw. As peak_bw <= avg_bw is kind of no-ops.

Regards,

Akash
Georgi Djakov April 9, 2020, 1:17 p.m. UTC | #6
Hi Akash,

On 4/8/20 15:17, Akash Asthana wrote:
> Hi Mark, Evan, Georgi,
> 
> On 4/7/2020 4:25 PM, Mark Brown wrote:
>> On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote:
>>> On 3/31/2020 4:53 PM, Mark Brown wrote:
>>>>> +    ctrl->avg_bw_cpu = Bps_to_icc(speed_hz);
>>>>> +    ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz);
>>>> I thought you were going to factor this best guess handling of peak
>>>> bandwidth out into the core?
>>> I can centralize this for SPI, I2C and UART  in Common driver(QUP wrapper)
>>> but still for QSPI I have to keep this piece of code as is because It is not
>>> child of QUP wrapper(it doesn't use common code).
>> Why not?
>>
>>> I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if
>>> nothing is mentioned explicitly" to ICC core because the factor of 2 is
>>> chosen randomly by me.
>> That's the whole point - if this is just a random number then we may as
>> well at least be consistently random.
> 
> Can we centralize below logic of peak_bw selection for all the clients to ICC core?

I don't think this is a good idea for now, because this is very hardware
specific. A scaling factor that works for one client might not work for another.

My questions here is how did you decide on this "multiply by two"? I can imagine
that the traffic can be bursty on some interfaces, but is the factor here really
a "random number" or is this based on some data patterns or performance
analysis?

Thanks,
Georgi
Mark Brown April 9, 2020, 1:20 p.m. UTC | #7
On Thu, Apr 09, 2020 at 04:17:22PM +0300, Georgi Djakov wrote:
> On 4/8/20 15:17, Akash Asthana wrote:

> > Can we centralize below logic of peak_bw selection for all the clients to ICC core?

> I don't think this is a good idea for now, because this is very hardware
> specific. A scaling factor that works for one client might not work for another.

AIUI a driver can always override the setting if it's got a better idea.

> My questions here is how did you decide on this "multiply by two"? I can imagine
> that the traffic can be bursty on some interfaces, but is the factor here really
> a "random number" or is this based on some data patterns or performance
> analysis?

The reason I'm pushing for this to go into the core is that the numbers
seem to be just made up and not device specific at all (or at least
there's a lot of devices with the same values).
Georgi Djakov April 15, 2020, 10:34 a.m. UTC | #8
Hi,

On 4/9/20 16:20, Mark Brown wrote:
> On Thu, Apr 09, 2020 at 04:17:22PM +0300, Georgi Djakov wrote:
>> On 4/8/20 15:17, Akash Asthana wrote:
> 
>>> Can we centralize below logic of peak_bw selection for all the clients to ICC core?
> 
>> I don't think this is a good idea for now, because this is very hardware
>> specific. A scaling factor that works for one client might not work for another.
> 
> AIUI a driver can always override the setting if it's got a better idea.

True.

>> My questions here is how did you decide on this "multiply by two"? I can imagine
>> that the traffic can be bursty on some interfaces, but is the factor here really
>> a "random number" or is this based on some data patterns or performance
>> analysis?
> 
> The reason I'm pushing for this to go into the core is that the numbers
> seem to be just made up and not device specific at all (or at least
> there's a lot of devices with the same values).

I might be a bit too cautious here, but the main question is what this common
number should be then. Maybe the numbers are not device specific, but
communication interface/bus specific? I would rather make the peak 30% higher
than the average, but for low speed interfaces i guess the factor of two should
also be fine?

Thanks,
Georgi
Georgi Djakov April 15, 2020, 10:54 a.m. UTC | #9
Hi Akash,

On 4/10/20 10:31, Akash Asthana wrote:
> Hi Georgi,
> 
> On 4/9/2020 6:47 PM, Georgi Djakov wrote:
>> Hi Akash,
>>
>> On 4/8/20 15:17, Akash Asthana wrote:
>>> Hi Mark, Evan, Georgi,
>>>
>>> On 4/7/2020 4:25 PM, Mark Brown wrote:
>>>> On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote:
>>>>> On 3/31/2020 4:53 PM, Mark Brown wrote:
>>>>>>> +    ctrl->avg_bw_cpu = Bps_to_icc(speed_hz);
>>>>>>> +    ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz);
>>>>>> I thought you were going to factor this best guess handling of peak
>>>>>> bandwidth out into the core?
>>>>> I can centralize this for SPI, I2C and UART  in Common driver(QUP wrapper)
>>>>> but still for QSPI I have to keep this piece of code as is because It is not
>>>>> child of QUP wrapper(it doesn't use common code).
>>>> Why not?
>>>>
>>>>> I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if
>>>>> nothing is mentioned explicitly" to ICC core because the factor of 2 is
>>>>> chosen randomly by me.
>>>> That's the whole point - if this is just a random number then we may as
>>>> well at least be consistently random.
>>> Can we centralize below logic of peak_bw selection for all the clients to ICC core?
>> I don't think this is a good idea for now, because this is very hardware
>> specific. A scaling factor that works for one client might not work for another.
>>
>> My questions here is how did you decide on this "multiply by two"? I can imagine
>> that the traffic can be bursty on some interfaces, but is the factor here really
>> a "random number" or is this based on some data patterns or performance
>> analysis?
> 
> Factor of 2 is random number.
> 
> We are taking care of actual throughput requirement in avg_bw vote and
> the intention of putting peak as twice of avg is to ensure that if high
> speed peripherals(ex:USB) removes their votes, we shouldn't see any
> latency issue because of other ICC client who don't vote for their BW
> requirement or *actual* BW requirement.

Thanks for clarifying, but is this latency a confirmed issue on real hardware?
I guess voting for twice as average will work, but still wondering wouldn't
it be more appropriate to handle it in the interconnect platform driver instead?
Also why is the other client not voting, can we fix it?

Thanks,
Georgi

Patch
diff mbox series

diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
index 3c4f83b..ad48f43 100644
--- a/drivers/spi/spi-qcom-qspi.c
+++ b/drivers/spi/spi-qcom-qspi.c
@@ -2,6 +2,7 @@ 
 // Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
 
 #include <linux/clk.h>
+#include <linux/interconnect.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -139,7 +140,10 @@  struct qcom_qspi {
 	struct device *dev;
 	struct clk_bulk_data *clks;
 	struct qspi_xfer xfer;
-	/* Lock to protect xfer and IRQ accessed registers */
+	struct icc_path *icc_path_cpu_to_qspi;
+	unsigned int avg_bw_cpu;
+	unsigned int peak_bw_cpu;
+	/* Lock to protect data accessed by IRQs */
 	spinlock_t lock;
 };
 
@@ -241,6 +245,20 @@  static int qcom_qspi_transfer_one(struct spi_master *master,
 		return ret;
 	}
 
+	/*
+	 * Set BW quota for CPU as driver supports FIFO mode only.
+	 * Assume peak bw as twice of avg bw.
+	 */
+	ctrl->avg_bw_cpu = Bps_to_icc(speed_hz);
+	ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz);
+	ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, ctrl->avg_bw_cpu,
+		ctrl->peak_bw_cpu);
+	if (ret) {
+		dev_err(ctrl->dev, "%s: ICC BW voting failed for cpu\n",
+			__func__);
+		return ret;
+	}
+
 	spin_lock_irqsave(&ctrl->lock, flags);
 
 	/* We are half duplex, so either rx or tx will be set */
@@ -458,6 +476,15 @@  static int qcom_qspi_probe(struct platform_device *pdev)
 	if (ret)
 		goto exit_probe_master_put;
 
+	ctrl->icc_path_cpu_to_qspi = devm_of_icc_get(dev, "qspi-config");
+	if (IS_ERR(ctrl->icc_path_cpu_to_qspi)) {
+		ret = PTR_ERR(ctrl->icc_path_cpu_to_qspi);
+		goto exit_probe_master_put;
+	}
+	/* Put BW vote on CPU path for register access */
+	ctrl->avg_bw_cpu = Bps_to_icc(1000);
+	ctrl->peak_bw_cpu = Bps_to_icc(1000);
+
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0)
 		goto exit_probe_master_put;
@@ -511,9 +538,17 @@  static int __maybe_unused qcom_qspi_runtime_suspend(struct device *dev)
 {
 	struct spi_master *master = dev_get_drvdata(dev);
 	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
+	int ret;
 
 	clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks);
 
+	ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, 0, 0);
+	if (ret) {
+		dev_err_ratelimited(ctrl->dev, "%s: ICC BW remove failed for cpu\n",
+			__func__);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -521,6 +556,15 @@  static int __maybe_unused qcom_qspi_runtime_resume(struct device *dev)
 {
 	struct spi_master *master = dev_get_drvdata(dev);
 	struct qcom_qspi *ctrl = spi_master_get_devdata(master);
+	int ret;
+
+	ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, ctrl->avg_bw_cpu,
+		ctrl->peak_bw_cpu);
+	if (ret) {
+		dev_err_ratelimited(ctrl->dev, "%s: ICC BW voting failed for cpu\n",
+			__func__);
+		return ret;
+	}
 
 	return clk_bulk_prepare_enable(QSPI_NUM_CLKS, ctrl->clks);
 }