On Wed, Mar 18, 2020 at 6:48 AM Akash Asthana [off-list ref] wrote:

Hi Evan,

On 3/18/2020 12:38 AM, Evan Green wrote:

quoted

On Tue, Mar 17, 2020 at 5:13 AM Akash Asthana [off-list ref] wrote:

quoted

Hi Matthias,

On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote:

quoted

Hi,

On Fri, Mar 13, 2020 at 06:42:13PM +0530, Akash Asthana wrote:

quoted

Get the interconnect paths for QSPI device and vote according to the
current bus speed of the driver.

Signed-off-by: Akash Asthana <redacted>
---
   - 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

   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;

This triplet is a recurring pattern, and is probably not limited to geni SE/QSPI.
On https://patchwork.kernel.org/patch/11436889/#23221925 I suggested the creation
of a geni SE specific struct, however adding a generic convenience struct to
'linux/interconnect.h' might be the better solution:

struct icc_client {
       struct icc_path *path;
       unsigned int avg_bw;
       unsigned int peak_bw;
};

I'm sure there are better names for it, but this would be the idea.

Yeah, I think introducing this to ICC header would be better solution.

+Georgi

I'm not as convinced this structure is generally useful and belongs in
the interconnect core. The thing that strikes me as weird with putting
it in the core is now we're saving these values both inside and
outside the interconnect core.

IIUC, you meant to say struct icc_req(inside icc_path) will be saving
avg_bw and peak_bw so no need to save it outside icc_path?

Correct, it seems silly to store the same set of values twice in the
framework, but with different semantics about who's watching it.
-Evan