[PATCH V2 0/8] Add interconnect support to QSPI and QUP drivers

[PATCH V2 0/8] Add interconnect support to QSPI and QUP drivers

[Akash Asthana]

dt-binding patch for QUP drivers.
 - https://patchwork.kernel.org/patch/11436621/ [Convert QUP bindings
	to YAML and add ICC, pin swap doc]

dt-binding patch for QSPI.
 - https://patchwork.kernel.org/patch/11436719/ [Convert QSPI binding
	to YAML and add interconnect doc]

High level design:
 - QUP wrapper/common driver.
   Vote for QUP core on behalf of earlycon from probe.
   Remove BW vote during sys suspend call

 - SERIAL driver.
   Vote only for CPU/CORE path because driver is in FIFO mode only
   Vote/unvote from qcom_geni_serial_pm func.
   Bump up the CPU vote from set_termios call based on real time need

 - I2C driver.
   Vote for CORE/CPU/DDR path
   Vote/unvote from runtime resume/suspend callback
   As bus speed for I2C is fixed from probe itself no need for bump up.

 - SPI QUP driver.
   Vote only for CPU/CORE path because driver is in FIFO mode only
   Vote/unvote from runtime resume/suspend callback
   Bump up CPU vote based on real time need per transfer.

 - QSPI driver.
   Vote only for CPU path
   Vote/unvote from runtime resume/suspend callback
   Bump up CPU vote based on real time need per transfer.


Changes in V2:
 - Add devm_of_icc_get() API interconnect core.
 - Add ICC support to common driver to fix earlyconsole crash.

Akash Asthana (8):
  interconnect: Add devm_of_icc_get() as exported API for users
  soc: qcom: geni: Support for ICC voting
  soc: qcom-geni-se: Add interconnect support to fix earlycon crash
  tty: serial: qcom_geni_serial: Add interconnect support
  i2c: i2c-qcom-geni: Add interconnect support
  spi: spi-geni-qcom: Add interconnect support
  spi: spi-qcom-qspi: Add interconnect support
  arm64: dts: sc7180: Add interconnect for QUP and QSPI

 arch/arm64/boot/dts/qcom/sc7180.dtsi  | 127 ++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-qcom-geni.c    | 110 +++++++++++++++++++++++++++++
 drivers/interconnect/core.c           |  25 +++++++
 drivers/soc/qcom/qcom-geni-se.c       |  41 +++++++++++
 drivers/spi/spi-geni-qcom.c           |  74 +++++++++++++++++++-
 drivers/spi/spi-qcom-qspi.c           |  46 +++++++++++-
 drivers/tty/serial/qcom_geni_serial.c |  69 ++++++++++++++++--
 include/linux/interconnect.h          |   7 ++
 include/linux/qcom-geni-se.h          |  28 ++++++++
 9 files changed, 521 insertions(+), 6 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

[PATCH V2 1/8] interconnect: Add devm_of_icc_get() as exported API for users

[Akash Asthana]

Users can use devm version of of_icc_get() to benefit from automatic
resource release.

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
---
 drivers/interconnect/core.c  | 25 +++++++++++++++++++++++++
 include/linux/interconnect.h |  7 +++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 2c6515e..f5699ed 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -350,6 +350,31 @@ static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
 	return node;
 }
 
+static void devm_icc_release(struct device *dev, void *res)
+{
+	icc_put(*(struct icc_path **)res);
+}
+
+struct icc_path *devm_of_icc_get(struct device *dev, const char *name)
+{
+	struct icc_path **ptr, *path;
+
+	ptr = devres_alloc(devm_icc_release, sizeof(**ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	path = of_icc_get(dev, name);
+	if (!IS_ERR(path)) {
+		*ptr = path;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return path;
+}
+EXPORT_SYMBOL_GPL(devm_of_icc_get);
+
 /**
  * of_icc_get() - get a path handle from a DT node based on name
  * @dev: device pointer for the consumer device
diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
index d70a914..7706924 100644
--- a/include/linux/interconnect.h
+++ b/include/linux/interconnect.h
@@ -28,6 +28,7 @@ struct device;
 struct icc_path *icc_get(struct device *dev, const int src_id,
 			 const int dst_id);
 struct icc_path *of_icc_get(struct device *dev, const char *name);
+struct icc_path *devm_of_icc_get(struct device *dev, const char *name);
 void icc_put(struct icc_path *path);
 int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
 void icc_set_tag(struct icc_path *path, u32 tag);
@@ -46,6 +47,12 @@ static inline struct icc_path *of_icc_get(struct device *dev,
 	return NULL;
 }
 
+static inline struct icc_path *devm_of_icc_get(struct device *dev,
+						const char *name)
+{
+	return NULL;
+}
+
 static inline void icc_put(struct icc_path *path)
 {
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 1/8] interconnect: Add devm_of_icc_get() as exported API for users

[Matthias Kaehlcke]

On Fri, Mar 13, 2020 at 06:42:07PM +0530, Akash Asthana wrote:
> Users can use devm version of of_icc_get() to benefit from automatic
> resource release.
> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
>  drivers/interconnect/core.c  | 25 +++++++++++++++++++++++++
>  include/linux/interconnect.h |  7 +++++++
>  2 files changed, 32 insertions(+)

Reviewed by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH V2 1/8] interconnect: Add devm_of_icc_get() as exported API for users

[Bjorn Andersson]

On Fri 13 Mar 06:12 PDT 2020, Akash Asthana wrote:

> Users can use devm version of of_icc_get() to benefit from automatic
> resource release.
> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/interconnect/core.c  | 25 +++++++++++++++++++++++++
>  include/linux/interconnect.h |  7 +++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 2c6515e..f5699ed 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -350,6 +350,31 @@ static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
>  	return node;
>  }
>  
> +static void devm_icc_release(struct device *dev, void *res)
> +{
> +	icc_put(*(struct icc_path **)res);
> +}
> +
> +struct icc_path *devm_of_icc_get(struct device *dev, const char *name)
> +{
> +	struct icc_path **ptr, *path;
> +
> +	ptr = devres_alloc(devm_icc_release, sizeof(**ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	path = of_icc_get(dev, name);
> +	if (!IS_ERR(path)) {
> +		*ptr = path;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return path;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_icc_get);
> +
>  /**
>   * of_icc_get() - get a path handle from a DT node based on name
>   * @dev: device pointer for the consumer device
> diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> index d70a914..7706924 100644
> --- a/include/linux/interconnect.h
> +++ b/include/linux/interconnect.h
> @@ -28,6 +28,7 @@ struct device;
>  struct icc_path *icc_get(struct device *dev, const int src_id,
>  			 const int dst_id);
>  struct icc_path *of_icc_get(struct device *dev, const char *name);
> +struct icc_path *devm_of_icc_get(struct device *dev, const char *name);
>  void icc_put(struct icc_path *path);
>  int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
>  void icc_set_tag(struct icc_path *path, u32 tag);
> @@ -46,6 +47,12 @@ static inline struct icc_path *of_icc_get(struct device *dev,
>  	return NULL;
>  }
>  
> +static inline struct icc_path *devm_of_icc_get(struct device *dev,
> +						const char *name)
> +{
> +	return NULL;
> +}
> +
>  static inline void icc_put(struct icc_path *path)
>  {
>  }
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

[PATCH V2 2/8] soc: qcom: geni: Support for ICC voting

[Akash Asthana]

Add necessary macros and structure variables to support ICC BW
voting from individual SE drivers.

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in V2:
 - As per Bjorn's comment dropped enums for ICC paths, given the three
   paths individual members

 include/linux/qcom-geni-se.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
index dd46494..eaae16e 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -6,6 +6,8 @@
 #ifndef _LINUX_QCOM_GENI_SE
 #define _LINUX_QCOM_GENI_SE
 
+#include <linux/interconnect.h>
+
 /* Transfer mode supported by GENI Serial Engines */
 enum geni_se_xfer_mode {
 	GENI_SE_INVALID,
@@ -33,6 +35,15 @@ struct clk;
  * @clk:		Handle to the core serial engine clock
  * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
  * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
+ * @icc_path_geni_to_core:	ICC path handle for geni to core
+ * @icc_path_cpu_to_geni:	ICC path handle for cpu to geni
+ * @icc_path_geni_to_ddr:	ICC path handle for geni to ddr
+ * @avg_bw_core:	Average bus bandwidth value for QUP core 2x clock
+ * @peak_bw_core:	Peak bus bandwidth value for QUP core 2x clock
+ * @avg_bw_cpu:		Average bus bandwidth value for CPU
+ * @peak_bw_cpu:	Peak bus bandwidth value for CPU
+ * @avg_bw_ddr:		Average bus bandwidth value for DDR
+ * @peak_bw_ddr:	Peak bus bandwidth value for DDR
  */
 struct geni_se {
 	void __iomem *base;
@@ -41,6 +52,15 @@ struct geni_se {
 	struct clk *clk;
 	unsigned int num_clk_levels;
 	unsigned long *clk_perf_tbl;
+	struct icc_path *icc_path_geni_to_core;
+	struct icc_path *icc_path_cpu_to_geni;
+	struct icc_path *icc_path_geni_to_ddr;
+	unsigned int avg_bw_core;
+	unsigned int peak_bw_core;
+	unsigned int avg_bw_cpu;
+	unsigned int peak_bw_cpu;
+	unsigned int avg_bw_ddr;
+	unsigned int peak_bw_ddr;
 };
 
 /* Common SE registers */
@@ -229,6 +249,14 @@ struct geni_se {
 #define GENI_SE_VERSION_MINOR(ver) ((ver & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT)
 #define GENI_SE_VERSION_STEP(ver) (ver & HW_VER_STEP_MASK)
 
+/* Core 2X clock frequency to BCM threshold mapping */
+#define CORE_2X_19_2_MHZ		960
+#define CORE_2X_50_MHZ			2500
+#define CORE_2X_100_MHZ			5000
+#define CORE_2X_150_MHZ			7500
+#define CORE_2X_200_MHZ			10000
+#define CORE_2X_236_MHZ			16383
+
 #if IS_ENABLED(CONFIG_QCOM_GENI_SE)
 
 u32 geni_se_get_qup_hw_version(struct geni_se *se);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 2/8] soc: qcom: geni: Support for ICC voting

[Matthias Kaehlcke]

Hi Akash,

On Fri, Mar 13, 2020 at 06:42:08PM +0530, Akash Asthana wrote:
> Add necessary macros and structure variables to support ICC BW
> voting from individual SE drivers.
> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V2:
>  - As per Bjorn's comment dropped enums for ICC paths, given the three
>    paths individual members
> 
>  include/linux/qcom-geni-se.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> index dd46494..eaae16e 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -6,6 +6,8 @@
>  #ifndef _LINUX_QCOM_GENI_SE
>  #define _LINUX_QCOM_GENI_SE
>  
> +#include <linux/interconnect.h>
> +
>  /* Transfer mode supported by GENI Serial Engines */
>  enum geni_se_xfer_mode {
>  	GENI_SE_INVALID,
> @@ -33,6 +35,15 @@ struct clk;
>   * @clk:		Handle to the core serial engine clock
>   * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
>   * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
> + * @icc_path_geni_to_core:	ICC path handle for geni to core
> + * @icc_path_cpu_to_geni:	ICC path handle for cpu to geni
> + * @icc_path_geni_to_ddr:	ICC path handle for geni to ddr
> + * @avg_bw_core:	Average bus bandwidth value for QUP core 2x clock
> + * @peak_bw_core:	Peak bus bandwidth value for QUP core 2x clock
> + * @avg_bw_cpu:		Average bus bandwidth value for CPU
> + * @peak_bw_cpu:	Peak bus bandwidth value for CPU
> + * @avg_bw_ddr:		Average bus bandwidth value for DDR
> + * @peak_bw_ddr:	Peak bus bandwidth value for DDR
>   */
>  struct geni_se {
>  	void __iomem *base;
> @@ -41,6 +52,15 @@ struct geni_se {
>  	struct clk *clk;
>  	unsigned int num_clk_levels;
>  	unsigned long *clk_perf_tbl;
> +	struct icc_path *icc_path_geni_to_core;
> +	struct icc_path *icc_path_cpu_to_geni;
> +	struct icc_path *icc_path_geni_to_ddr;
> +	unsigned int avg_bw_core;
> +	unsigned int peak_bw_core;
> +	unsigned int avg_bw_cpu;
> +	unsigned int peak_bw_cpu;
> +	unsigned int avg_bw_ddr;
> +	unsigned int peak_bw_ddr;

Those are a lot of new individual struct members. How about clustering
them, e.g.:

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

struct geni_iccs_paths {
	struct geni_icc_path to_core;
	struct geni_icc_path from_cpu;
	struct geni_icc_path to_ddr;
};

And 'struct geni_se' just gets this entry:

	struct geni_icc_paths icc;

or alternatively three 'struct geni_icc_path' entries.

Re: [PATCH V2 2/8] soc: qcom: geni: Support for ICC voting

[Akash Asthana]

Hi Matthias,

On 3/13/2020 10:12 PM, Matthias Kaehlcke wrote:
> Hi Akash,
>
> On Fri, Mar 13, 2020 at 06:42:08PM +0530, Akash Asthana wrote:
>> Add necessary macros and structure variables to support ICC BW
>> voting from individual SE drivers.
>>
>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>> Changes in V2:
>>   - As per Bjorn's comment dropped enums for ICC paths, given the three
>>     paths individual members
>>
>>   include/linux/qcom-geni-se.h | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
>> index dd46494..eaae16e 100644
>> --- a/include/linux/qcom-geni-se.h
>> +++ b/include/linux/qcom-geni-se.h
>> @@ -6,6 +6,8 @@
>>   #ifndef _LINUX_QCOM_GENI_SE
>>   #define _LINUX_QCOM_GENI_SE
>>   
>> +#include <linux/interconnect.h>
>> +
>>   /* Transfer mode supported by GENI Serial Engines */
>>   enum geni_se_xfer_mode {
>>   	GENI_SE_INVALID,
>> @@ -33,6 +35,15 @@ struct clk;
>>    * @clk:		Handle to the core serial engine clock
>>    * @num_clk_levels:	Number of valid clock levels in clk_perf_tbl
>>    * @clk_perf_tbl:	Table of clock frequency input to serial engine clock
>> + * @icc_path_geni_to_core:	ICC path handle for geni to core
>> + * @icc_path_cpu_to_geni:	ICC path handle for cpu to geni
>> + * @icc_path_geni_to_ddr:	ICC path handle for geni to ddr
>> + * @avg_bw_core:	Average bus bandwidth value for QUP core 2x clock
>> + * @peak_bw_core:	Peak bus bandwidth value for QUP core 2x clock
>> + * @avg_bw_cpu:		Average bus bandwidth value for CPU
>> + * @peak_bw_cpu:	Peak bus bandwidth value for CPU
>> + * @avg_bw_ddr:		Average bus bandwidth value for DDR
>> + * @peak_bw_ddr:	Peak bus bandwidth value for DDR
>>    */
>>   struct geni_se {
>>   	void __iomem *base;
>> @@ -41,6 +52,15 @@ struct geni_se {
>>   	struct clk *clk;
>>   	unsigned int num_clk_levels;
>>   	unsigned long *clk_perf_tbl;
>> +	struct icc_path *icc_path_geni_to_core;
>> +	struct icc_path *icc_path_cpu_to_geni;
>> +	struct icc_path *icc_path_geni_to_ddr;
>> +	unsigned int avg_bw_core;
>> +	unsigned int peak_bw_core;
>> +	unsigned int avg_bw_cpu;
>> +	unsigned int peak_bw_cpu;
>> +	unsigned int avg_bw_ddr;
>> +	unsigned int peak_bw_ddr;
> Those are a lot of new individual struct members. How about clustering
> them, e.g.:
>
> struct geni_icc_path {
> 	struct icc_path *path;
> 	unsigned int avg_bw;
> 	unsigned int peak_bw;
> };
I guess it would be better to add this structure  ICC driver as you 
suggested@https://patchwork.kernel.org/patch/11436905/.
> struct geni_iccs_paths {
> 	struct geni_icc_path to_core;
> 	struct geni_icc_path from_cpu;
> 	struct geni_icc_path to_ddr;
> };
>
> And 'struct geni_se' just gets this entry:
>
> 	struct geni_icc_paths icc;
>
> or alternatively three 'struct geni_icc_path' entries.

ok

Thanks for reviewing.

Regards

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 2/8] soc: qcom: geni: Support for ICC voting

[Evan Green]

On Fri, Mar 13, 2020 at 6:12 AM Akash Asthana <akashast@codeaurora.org> wrote:
>
> Add necessary macros and structure variables to support ICC BW
> voting from individual SE drivers.
>
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V2:
>  - As per Bjorn's comment dropped enums for ICC paths, given the three
>    paths individual members
>
>  include/linux/qcom-geni-se.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> index dd46494..eaae16e 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -6,6 +6,8 @@
>  #ifndef _LINUX_QCOM_GENI_SE
>  #define _LINUX_QCOM_GENI_SE
>
> +#include <linux/interconnect.h>
> +
>  /* Transfer mode supported by GENI Serial Engines */
>  enum geni_se_xfer_mode {
>         GENI_SE_INVALID,
> @@ -33,6 +35,15 @@ struct clk;
>   * @clk:               Handle to the core serial engine clock
>   * @num_clk_levels:    Number of valid clock levels in clk_perf_tbl
>   * @clk_perf_tbl:      Table of clock frequency input to serial engine clock
> + * @icc_path_geni_to_core:     ICC path handle for geni to core
> + * @icc_path_cpu_to_geni:      ICC path handle for cpu to geni
> + * @icc_path_geni_to_ddr:      ICC path handle for geni to ddr
> + * @avg_bw_core:       Average bus bandwidth value for QUP core 2x clock
> + * @peak_bw_core:      Peak bus bandwidth value for QUP core 2x clock
> + * @avg_bw_cpu:                Average bus bandwidth value for CPU
> + * @peak_bw_cpu:       Peak bus bandwidth value for CPU
> + * @avg_bw_ddr:                Average bus bandwidth value for DDR
> + * @peak_bw_ddr:       Peak bus bandwidth value for DDR
>   */
>  struct geni_se {
>         void __iomem *base;
> @@ -41,6 +52,15 @@ struct geni_se {
>         struct clk *clk;
>         unsigned int num_clk_levels;
>         unsigned long *clk_perf_tbl;
> +       struct icc_path *icc_path_geni_to_core;
> +       struct icc_path *icc_path_cpu_to_geni;
> +       struct icc_path *icc_path_geni_to_ddr;
> +       unsigned int avg_bw_core;
> +       unsigned int peak_bw_core;
> +       unsigned int avg_bw_cpu;
> +       unsigned int peak_bw_cpu;
> +       unsigned int avg_bw_ddr;
> +       unsigned int peak_bw_ddr;
>  };
>
>  /* Common SE registers */
> @@ -229,6 +249,14 @@ struct geni_se {
>  #define GENI_SE_VERSION_MINOR(ver) ((ver & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT)
>  #define GENI_SE_VERSION_STEP(ver) (ver & HW_VER_STEP_MASK)
>
> +/* Core 2X clock frequency to BCM threshold mapping */
> +#define CORE_2X_19_2_MHZ               960
> +#define CORE_2X_50_MHZ                 2500
> +#define CORE_2X_100_MHZ                        5000
> +#define CORE_2X_150_MHZ                        7500
> +#define CORE_2X_200_MHZ                        10000
> +#define CORE_2X_236_MHZ                        16383

These are all just 50 * clock_rate. Can you instead specify that one
define of CLK_TO_BW_RATIO 50, and then use clk_get_rate() to get the
input clock frequency. That way, if these end up getting clocked at a
different rate, the bandwidth also scales appropriately. Also, can you
enumerate why 50 is an appropriate ratio?
-Evan

-Evan

Re: [PATCH V2 2/8] soc: qcom: geni: Support for ICC voting

[Evan Green]

On Fri, Mar 20, 2020 at 4:03 AM Akash Asthana <akashast@codeaurora.org> wrote:
>
> Hi Evan,
>
> +/* Core 2X clock frequency to BCM threshold mapping */
> +#define CORE_2X_19_2_MHZ               960
> +#define CORE_2X_50_MHZ                 2500
> +#define CORE_2X_100_MHZ                        5000
> +#define CORE_2X_150_MHZ                        7500
> +#define CORE_2X_200_MHZ                        10000
> +#define CORE_2X_236_MHZ                        16383
>
> These are all just 50 * clock_rate. Can you instead specify that one
> define of CLK_TO_BW_RATIO 50, and then use clk_get_rate() to get the
> input clock frequency. That way, if these end up getting clocked at a
> different rate, the bandwidth also scales appropriately. Also, can you
> enumerate why 50 is an appropriate ratio?
> -Evan
>
> -Evan
>
> Clock rate for Core 2X is controlled by BW voting only, we don't set clock rate for core 2X clock either by DFS or calling clk_set_rate API like we do for SE clocks from individual driver.
>
> In DT node it's not mentioned as clock.
>
> As discussed in patch@ https://patchwork.kernel.org/patch/11436897/  We are not scaling Core 2X clock based on dynamic need of driver instead we are putting recommended value from HW team for each driver.

Oh I get it. This is pretty opaque, since this table is saying "here
are the bandwidth values that happen to work out to a Core2X clock
rate of N". But it's not obvious why setting the Core2X clock rate to
N is desirable or appropriate. The answer seems to be hardware guys
told us these thresholds work well in practice. And if I'm reading
into it more, probably they're saying these bandwidths are too low to
be worth dynamically managing beyond on/off.

At the very least we should explain some of this in the comment above
these defines. Something like:
/* Define bandwidth thresholds that cause the underlying Core 2X
interconnect clock to run at the named frequency. These baseline
values are recommended by the hardware team, and are not dynamically
scaled with GENI bandwidth beyond basic on/off. */
-Evan

Re: [PATCH V2 2/8] soc: qcom: geni: Support for ICC voting

[Akash Asthana]

Hi Evan,

On 3/20/2020 10:15 PM, Evan Green wrote:
> On Fri, Mar 20, 2020 at 4:03 AM Akash Asthana <akashast@codeaurora.org> wrote:
>> Hi Evan,
>>
>> +/* Core 2X clock frequency to BCM threshold mapping */
>> +#define CORE_2X_19_2_MHZ               960
>> +#define CORE_2X_50_MHZ                 2500
>> +#define CORE_2X_100_MHZ                        5000
>> +#define CORE_2X_150_MHZ                        7500
>> +#define CORE_2X_200_MHZ                        10000
>> +#define CORE_2X_236_MHZ                        16383
>>
>> These are all just 50 * clock_rate. Can you instead specify that one
>> define of CLK_TO_BW_RATIO 50, and then use clk_get_rate() to get the
>> input clock frequency. That way, if these end up getting clocked at a
>> different rate, the bandwidth also scales appropriately. Also, can you
>> enumerate why 50 is an appropriate ratio?
>> -Evan
>>
>> -Evan
>>
>> Clock rate for Core 2X is controlled by BW voting only, we don't set clock rate for core 2X clock either by DFS or calling clk_set_rate API like we do for SE clocks from individual driver.
>>
>> In DT node it's not mentioned as clock.
>>
>> As discussed in patch@ https://patchwork.kernel.org/patch/11436897/  We are not scaling Core 2X clock based on dynamic need of driver instead we are putting recommended value from HW team for each driver.
> Oh I get it. This is pretty opaque, since this table is saying "here
> are the bandwidth values that happen to work out to a Core2X clock
> rate of N".

Hmm,  BCM threshold to CORE2X clock rate mapping is exposed to us from 
clock team.

BCM threshold value is internally convert to mentioned clock rate this 
is something internal to board ICC driver.

>   But it's not obvious why setting the Core2X clock rate to
> N is desirable or appropriate. The answer seems to be hardware guys
> told us these thresholds work well in practice.
Yes, this is correct as the core clocks behaves different than any other 
NOC, we rely on the recommendation from VI/HW team.
>   And if I'm reading
> into it more, probably they're saying these bandwidths are too low to
> be worth dynamically managing beyond on/off
I am not sure whether they intend to say this.
> At the very least we should explain some of this in the comment above
> these defines. Something like:
> /* Define bandwidth thresholds that cause the underlying Core 2X
> interconnect clock to run at the named frequency. These baseline
> values are recommended by the hardware team, and are not dynamically
> scaled with GENI bandwidth beyond basic on/off. */
> -Evan

Ok,

regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

[PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Akash Asthana]

V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
to reset at boot time.

As QUP core clock is shared among all the SE drivers present on particular
QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
is put to 0 from other SE drivers before real console comes up.

As earlycon can't vote for it's QUP core need, to fix this add ICC
support to common/QUP wrapper driver and put vote for QUP core from
probe on behalf of earlycon and remove vote during sys suspend.

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
Reported-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/soc/qcom/qcom-geni-se.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 7d622ea..d244dfc 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -90,6 +90,7 @@ struct geni_wrapper {
 	struct device *dev;
 	void __iomem *base;
 	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
+	struct icc_path *icc_path_geni_to_core;
 };
 
 #define QUP_HW_VER_REG			0x4
@@ -747,11 +748,50 @@ static int geni_se_probe(struct platform_device *pdev)
 		}
 	}
 
+#ifdef CONFIG_SERIAL_EARLYCON
+	wrapper->icc_path_geni_to_core = devm_of_icc_get(dev, "qup-core");
+	if (IS_ERR(wrapper->icc_path_geni_to_core))
+		return PTR_ERR(wrapper->icc_path_geni_to_core);
+	/*
+	 * Put minmal BW request on core clocks on behalf of early console.
+	 * The vote will be removed in suspend call.
+	 */
+	ret = icc_set_bw(wrapper->icc_path_geni_to_core, Bps_to_icc(1000),
+			Bps_to_icc(1000));
+	if (ret) {
+		dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
+			__func__);
+		return ret;
+	}
+#endif
+
 	dev_set_drvdata(dev, wrapper);
 	dev_dbg(dev, "GENI SE Driver probed\n");
 	return devm_of_platform_populate(dev);
 }
 
+static int __maybe_unused geni_se_sys_suspend(struct device *dev)
+{
+	struct geni_wrapper *wrapper = dev_get_drvdata(dev);
+	int ret;
+
+#ifdef CONFIG_SERIAL_EARLYCON
+	ret = icc_set_bw(wrapper->icc_path_geni_to_core, 0, 0);
+	if (ret) {
+		dev_err(dev, "%s: ICC BW remove failed for core\n",
+			__func__);
+		return ret;
+	}
+#endif
+
+	return 0;
+}
+
+static const struct dev_pm_ops geni_se_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(geni_se_sys_suspend,
+				NULL)
+};
+
 static const struct of_device_id geni_se_dt_match[] = {
 	{ .compatible = "qcom,geni-se-qup", },
 	{}
@@ -762,6 +802,7 @@ static struct platform_driver geni_se_driver = {
 	.driver = {
 		.name = "geni_se_qup",
 		.of_match_table = geni_se_dt_match,
+		.pm = &geni_se_pm_ops,
 	},
 	.probe = geni_se_probe,
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Matthias Kaehlcke]

Hi Akash,

On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
> to reset at boot time.

The v1 patch isn't relevant in the commit message, please just describe the
problem. Also the crash only occurs when earlycon is used.

> As QUP core clock is shared among all the SE drivers present on particular
> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
> is put to 0 from other SE drivers before real console comes up.
> 
> As earlycon can't vote for it's QUP core need, to fix this add ICC
> support to common/QUP wrapper driver and put vote for QUP core from
> probe on behalf of earlycon and remove vote during sys suspend.

Only removing the vote on suspend isn't ideal, the system might never get
suspended. That said I don't have a really good alternative suggestion.

One thing you could possibly do is to launch a delayed work, check
console_device() every second or so and remove the vote when it returns
non-NULL. Not claiming this would be a great solution ...

The cleanest solution might be a notifier when the early console is
unregistered, it seems somewhat over-engineered though ... Then again
other (future) uart drivers with interconnect support might run into
the same problem.

> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> Reported-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 7d622ea..d244dfc 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -90,6 +90,7 @@ struct geni_wrapper {
>  	struct device *dev;
>  	void __iomem *base;
>  	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
> +	struct icc_path *icc_path_geni_to_core;
>  };
>  
>  #define QUP_HW_VER_REG			0x4
> @@ -747,11 +748,50 @@ static int geni_se_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +#ifdef CONFIG_SERIAL_EARLYCON
> +	wrapper->icc_path_geni_to_core = devm_of_icc_get(dev, "qup-core");
> +	if (IS_ERR(wrapper->icc_path_geni_to_core))
> +		return PTR_ERR(wrapper->icc_path_geni_to_core);
> +	/*
> +	 * Put minmal BW request on core clocks on behalf of early console.
> +	 * The vote will be removed in suspend call.
> +	 */
> +	ret = icc_set_bw(wrapper->icc_path_geni_to_core, Bps_to_icc(1000),
> +			Bps_to_icc(1000));
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
> +			__func__);
> +		return ret;
> +	}

What is ugly about this is that it's done for every QUP, not only the one
with the early console. Again, I don't have a good solution for it, maybe
it's a limitation we have to live with :(

> +#endif
> +
>  	dev_set_drvdata(dev, wrapper);
>  	dev_dbg(dev, "GENI SE Driver probed\n");
>  	return devm_of_platform_populate(dev);
>  }
>  
> +static int __maybe_unused geni_se_sys_suspend(struct device *dev)
> +{
> +	struct geni_wrapper *wrapper = dev_get_drvdata(dev);
> +	int ret;
> +
> +#ifdef CONFIG_SERIAL_EARLYCON
> +	ret = icc_set_bw(wrapper->icc_path_geni_to_core, 0, 0);

I think you only want to do this on the first suspend.

Do we need to handle the case where no 'real' console is configured?
In this case the early console would be active forever and setting
the bandwidths to 0 might cause a similar crash than the one you are
trying to fix. Not sure if that's a real world use case, but wanted to
mention it. Maybe this is an argument of the notifier approach?

> +	if (ret) {
> +		dev_err(dev, "%s: ICC BW remove failed for core\n",
> +			__func__);
> +		return ret;

Aborting suspend seems too harsh since the QUP should still be fully
functional unless there is a general problem with the interconnects.

I would suggest to change the log to dev_warn() and return 0.

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Akash Asthana]

Hi Matthias,

On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
> Hi Akash,
>
> On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
>> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
>> to reset at boot time.
> The v1 patch isn't relevant in the commit message, please just describe the
> problem. Also the crash only occurs when earlycon is used.
ok
>
>> As QUP core clock is shared among all the SE drivers present on particular
>> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
>> is put to 0 from other SE drivers before real console comes up.
>>
>> As earlycon can't vote for it's QUP core need, to fix this add ICC
>> support to common/QUP wrapper driver and put vote for QUP core from
>> probe on behalf of earlycon and remove vote during sys suspend.
> Only removing the vote on suspend isn't ideal, the system might never get
> suspended. That said I don't have a really good alternative suggestion.
>
> One thing you could possibly do is to launch a delayed work, check
> console_device() every second or so and remove the vote when it returns
> non-NULL. Not claiming this would be a great solution ...
>
> The cleanest solution might be a notifier when the early console is
> unregistered, it seems somewhat over-engineered though ... Then again
> other (future) uart drivers with interconnect support might run into
> the same problem.

We are hitting this problem because QUP core clocks are shared among all 
the SE driver present in particular QUP wrapper, if other HW controllers 
has similar architecture we will hit this issue.

How about if we expose an API from common driver(geni-se) for putting 
QUP core BW vote to 0.

We call this from console probe just after uart_add_one_port call 
(console resources are enabled as part of this call) to put core quota 
to 0 on behalf of earlyconsole?

>
>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>> Reported-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>   drivers/soc/qcom/qcom-geni-se.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> index 7d622ea..d244dfc 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -90,6 +90,7 @@ struct geni_wrapper {
>>   	struct device *dev;
>>   	void __iomem *base;
>>   	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
>> +	struct icc_path *icc_path_geni_to_core;
>>   };
>>   
>>   #define QUP_HW_VER_REG			0x4
>> @@ -747,11 +748,50 @@ static int geni_se_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +#ifdef CONFIG_SERIAL_EARLYCON
>> +	wrapper->icc_path_geni_to_core = devm_of_icc_get(dev, "qup-core");
>> +	if (IS_ERR(wrapper->icc_path_geni_to_core))
>> +		return PTR_ERR(wrapper->icc_path_geni_to_core);
>> +	/*
>> +	 * Put minmal BW request on core clocks on behalf of early console.
>> +	 * The vote will be removed in suspend call.
>> +	 */
>> +	ret = icc_set_bw(wrapper->icc_path_geni_to_core, Bps_to_icc(1000),
>> +			Bps_to_icc(1000));
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
>> +			__func__);
>> +		return ret;
>> +	}
> What is ugly about this is that it's done for every QUP, not only the one
> with the early console. Again, I don't have a good solution for it, maybe
> it's a limitation we have to live with :(

There is one more limitation from QUP core side. Core clocks for both 
the QUP wrapper runs at same speed.

core2x_1 = core2x_2 = max(core2x_1, core2x_2);

So with above limitation and if we are removing early con vote from Core 
when real console comes up. It doesn't matter whether it's done for 
every QUP or the only with early console.

>
>> +#endif
>> +
>>   	dev_set_drvdata(dev, wrapper);
>>   	dev_dbg(dev, "GENI SE Driver probed\n");
>>   	return devm_of_platform_populate(dev);
>>   }
>>   
>> +static int __maybe_unused geni_se_sys_suspend(struct device *dev)
>> +{
>> +	struct geni_wrapper *wrapper = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +#ifdef CONFIG_SERIAL_EARLYCON
>> +	ret = icc_set_bw(wrapper->icc_path_geni_to_core, 0, 0);
> I think you only want to do this on the first suspend.
Ok, I can add that logic using global variable.
>
> Do we need to handle the case where no 'real' console is configured?
> In this case the early console would be active forever and setting
> the bandwidths to 0 might cause a similar crash than the one you are
> trying to fix. Not sure if that's a real world use case, but wanted to
> mention it. Maybe this is an argument of the notifier approach?
We can't support earlycon without real console.

As earlyconsole doesn't do any kind of resource enablement(SE clocks, 
pinctrl, etc) it assumes that resources are already enabled from 
previous stages.

So if real console doesn't come up no one will vote for that SE clock, 
and it will be disabled from clk late_init call which will result into 
un-clocked access.


>
>> +	if (ret) {
>> +		dev_err(dev, "%s: ICC BW remove failed for core\n",
>> +			__func__);
>> +		return ret;
> Aborting suspend seems too harsh since the QUP should still be fully
> functional unless there is a general problem with the interconnects.
>
> I would suggest to change the log to dev_warn() and return 0.

Ok

Thanks for reviewing the patch.

regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Matthias Kaehlcke]

Hi Akash,

On Tue, Mar 17, 2020 at 04:27:47PM +0530, Akash Asthana wrote:
> Hi Matthias,
> 
> On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
> > Hi Akash,
> > 
> > On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
> > > V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
> > > to reset at boot time.
> > The v1 patch isn't relevant in the commit message, please just describe the
> > problem. Also the crash only occurs when earlycon is used.
> ok
> > 
> > > As QUP core clock is shared among all the SE drivers present on particular
> > > QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
> > > is put to 0 from other SE drivers before real console comes up.
> > > 
> > > As earlycon can't vote for it's QUP core need, to fix this add ICC
> > > support to common/QUP wrapper driver and put vote for QUP core from
> > > probe on behalf of earlycon and remove vote during sys suspend.
> > Only removing the vote on suspend isn't ideal, the system might never get
> > suspended. That said I don't have a really good alternative suggestion.
> > 
> > One thing you could possibly do is to launch a delayed work, check
> > console_device() every second or so and remove the vote when it returns
> > non-NULL. Not claiming this would be a great solution ...
> > 
> > The cleanest solution might be a notifier when the early console is
> > unregistered, it seems somewhat over-engineered though ... Then again
> > other (future) uart drivers with interconnect support might run into
> > the same problem.
> 
> We are hitting this problem because QUP core clocks are shared among all the
> SE driver present in particular QUP wrapper, if other HW controllers has
> similar architecture we will hit this issue.
> 
> How about if we expose an API from common driver(geni-se) for putting QUP
> core BW vote to 0.
> 
> We call this from console probe just after uart_add_one_port call (console
> resources are enabled as part of this call) to put core quota to 0 on behalf
> of earlyconsole?

From my notes from earlier debugging I have doubts this would work:

  There is a short window where the early console and the 'real' console coexist:

  [    3.858122] printk: console [ttyMSM0] enabled
  [    3.875692] printk: bootconsole [qcom_geni0] disabled

  The reset probably occurs when the early console tries to write, but the ICC
  is effectively disabled because ttyMSM0 and the other geni ports are runtime
  suspended.

> > > Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> > > Reported-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > >   drivers/soc/qcom/qcom-geni-se.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 41 insertions(+)
> > > 
> > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> > > index 7d622ea..d244dfc 100644
> > > --- a/drivers/soc/qcom/qcom-geni-se.c
> > > +++ b/drivers/soc/qcom/qcom-geni-se.c
> > > @@ -90,6 +90,7 @@ struct geni_wrapper {
> > >   	struct device *dev;
> > >   	void __iomem *base;
> > >   	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
> > > +	struct icc_path *icc_path_geni_to_core;
> > >   };
> > >   #define QUP_HW_VER_REG			0x4
> > > @@ -747,11 +748,50 @@ static int geni_se_probe(struct platform_device *pdev)
> > >   		}
> > >   	}
> > > +#ifdef CONFIG_SERIAL_EARLYCON
> > > +	wrapper->icc_path_geni_to_core = devm_of_icc_get(dev, "qup-core");
> > > +	if (IS_ERR(wrapper->icc_path_geni_to_core))
> > > +		return PTR_ERR(wrapper->icc_path_geni_to_core);
> > > +	/*
> > > +	 * Put minmal BW request on core clocks on behalf of early console.
> > > +	 * The vote will be removed in suspend call.
> > > +	 */
> > > +	ret = icc_set_bw(wrapper->icc_path_geni_to_core, Bps_to_icc(1000),
> > > +			Bps_to_icc(1000));
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
> > > +			__func__);
> > > +		return ret;
> > > +	}
> > What is ugly about this is that it's done for every QUP, not only the one
> > with the early console. Again, I don't have a good solution for it, maybe
> > it's a limitation we have to live with :(
> 
> There is one more limitation from QUP core side. Core clocks for both the
> QUP wrapper runs at same speed.
> 
> core2x_1 = core2x_2 = max(core2x_1, core2x_2);
> 
> So with above limitation and if we are removing early con vote from Core
> when real console comes up. It doesn't matter whether it's done for every
> QUP or the only with early console.

it's still sorta ugly at an abstraction level, but it seem we have to be
pragmatic here.

> > > +#endif
> > > +
> > >   	dev_set_drvdata(dev, wrapper);
> > >   	dev_dbg(dev, "GENI SE Driver probed\n");
> > >   	return devm_of_platform_populate(dev);
> > >   }
> > > +static int __maybe_unused geni_se_sys_suspend(struct device *dev)
> > > +{
> > > +	struct geni_wrapper *wrapper = dev_get_drvdata(dev);
> > > +	int ret;
> > > +
> > > +#ifdef CONFIG_SERIAL_EARLYCON
> > > +	ret = icc_set_bw(wrapper->icc_path_geni_to_core, 0, 0);
> > I think you only want to do this on the first suspend.
> Ok, I can add that logic using global variable.
> > 
> > Do we need to handle the case where no 'real' console is configured?
> > In this case the early console would be active forever and setting
> > the bandwidths to 0 might cause a similar crash than the one you are
> > trying to fix. Not sure if that's a real world use case, but wanted to
> > mention it. Maybe this is an argument of the notifier approach?
> We can't support earlycon without real console.
> 
> As earlyconsole doesn't do any kind of resource enablement(SE clocks,
> pinctrl, etc) it assumes that resources are already enabled from previous
> stages.
> 
> So if real console doesn't come up no one will vote for that SE clock, and
> it will be disabled from clk late_init call which will result into
> un-clocked access.

Ok, IIUC what you are saying is that earlycon can't work on its own after geni
initialization. Because it clearly can work before (otherwise what would be
its purpose?), supposedly because the bootloader configures the necessary
bits.

In any case the bottom line is that earlycon requires a real console to be
configured and there is no need to handle the corner case I brought up.

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Akash Asthana]

Hi Matthias,

On 3/17/2020 11:59 PM, Matthias Kaehlcke wrote:
> Hi Akash,
>
> On Tue, Mar 17, 2020 at 04:27:47PM +0530, Akash Asthana wrote:
>> Hi Matthias,
>>
>> On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
>>> Hi Akash,
>>>
>>> On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
>>>> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
>>>> to reset at boot time.
>>> The v1 patch isn't relevant in the commit message, please just describe the
>>> problem. Also the crash only occurs when earlycon is used.
>> ok
>>>> As QUP core clock is shared among all the SE drivers present on particular
>>>> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
>>>> is put to 0 from other SE drivers before real console comes up.
>>>>
>>>> As earlycon can't vote for it's QUP core need, to fix this add ICC
>>>> support to common/QUP wrapper driver and put vote for QUP core from
>>>> probe on behalf of earlycon and remove vote during sys suspend.
>>> Only removing the vote on suspend isn't ideal, the system might never get
>>> suspended. That said I don't have a really good alternative suggestion.
>>>
>>> One thing you could possibly do is to launch a delayed work, check
>>> console_device() every second or so and remove the vote when it returns
>>> non-NULL. Not claiming this would be a great solution ...
>>>
>>> The cleanest solution might be a notifier when the early console is
>>> unregistered, it seems somewhat over-engineered though ... Then again
>>> other (future) uart drivers with interconnect support might run into
>>> the same problem.
>> We are hitting this problem because QUP core clocks are shared among all the
>> SE driver present in particular QUP wrapper, if other HW controllers has
>> similar architecture we will hit this issue.
>>
>> How about if we expose an API from common driver(geni-se) for putting QUP
>> core BW vote to 0.
>>
>> We call this from console probe just after uart_add_one_port call (console
>> resources are enabled as part of this call) to put core quota to 0 on behalf
>> of earlyconsole?
>  From my notes from earlier debugging I have doubts this would work:
>
>    There is a short window where the early console and the 'real' console coexist:
>
>    [    3.858122] printk: console [ttyMSM0] enabled
>    [    3.875692] printk: bootconsole [qcom_geni0] disabled
>
>    The reset probably occurs when the early console tries to write, but the ICC
>    is effectively disabled because ttyMSM0 and the other geni ports are runtime
>    suspended.

Code flow from console driver probe(qcom_geni_serial.c)

uart_add_one_port--->uart_configure_port--->{ 1) uart_change_pm(enable 
console resources)  2)register_console(boot to real console switch 
happens here)}

Console resources are not disabled from anywhere before the switch 
happens completely. I meant to say until we saw below logs.

[    3.875692] printk: bootconsole [qcom_geni0] disabled

I think the board reset issue cannot occur during the window where early 
console and 'real' console coexist.

I have validated proposed solution by me, it is working fine.

Currently voting is done for every QUP and not only to which earlycon is 
connect, with the above approach we can't remove vote from other QUPs.

However we can limit voting only to earlycon QUP by removing 
interconnect from DT node of other QUPs.

I am not sure how clean is this solution.

>
>>>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>>>> Reported-by: Matthias Kaehlcke <mka@chromium.org>
>>>> ---
>>>>    drivers/soc/qcom/qcom-geni-se.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 41 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>>>> index 7d622ea..d244dfc 100644
>>>> --- a/drivers/soc/qcom/qcom-geni-se.c
>>>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>>>> @@ -90,6 +90,7 @@ struct geni_wrapper {
>>>>    	struct device *dev;
>>>>    	void __iomem *base;
>>>>    	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
>>>> +	struct icc_path *icc_path_geni_to_core;
>>>>    };
>>>>    #define QUP_HW_VER_REG			0x4
>>>> @@ -747,11 +748,50 @@ static int geni_se_probe(struct platform_device *pdev)
>>>>    		}
>>>>    	}
>>>> +#ifdef CONFIG_SERIAL_EARLYCON
>>>> +	wrapper->icc_path_geni_to_core = devm_of_icc_get(dev, "qup-core");
>>>> +	if (IS_ERR(wrapper->icc_path_geni_to_core))
>>>> +		return PTR_ERR(wrapper->icc_path_geni_to_core);
>>>> +	/*
>>>> +	 * Put minmal BW request on core clocks on behalf of early console.
>>>> +	 * The vote will be removed in suspend call.
>>>> +	 */
>>>> +	ret = icc_set_bw(wrapper->icc_path_geni_to_core, Bps_to_icc(1000),
>>>> +			Bps_to_icc(1000));
>>>> +	if (ret) {
>>>> +		dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
>>>> +			__func__);
>>>> +		return ret;
>>>> +	}
>>> What is ugly about this is that it's done for every QUP, not only the one
>>> with the early console. Again, I don't have a good solution for it, maybe
>>> it's a limitation we have to live with :(
>> There is one more limitation from QUP core side. Core clocks for both the
>> QUP wrapper runs at same speed.
>>
>> core2x_1 = core2x_2 = max(core2x_1, core2x_2);
>>
>> So with above limitation and if we are removing early con vote from Core
>> when real console comes up. It doesn't matter whether it's done for every
>> QUP or the only with early console.
> it's still sorta ugly at an abstraction level, but it seem we have to be
> pragmatic here.

How about if we limit voting only to earlycon QUP by removing 
interconnect from DT node of other QUPs.?

>
>>>> +#endif
>>>> +
>>>>    	dev_set_drvdata(dev, wrapper);
>>>>    	dev_dbg(dev, "GENI SE Driver probed\n");
>>>>    	return devm_of_platform_populate(dev);
>>>>    }
>>>> +static int __maybe_unused geni_se_sys_suspend(struct device *dev)
>>>> +{
>>>> +	struct geni_wrapper *wrapper = dev_get_drvdata(dev);
>>>> +	int ret;
>>>> +
>>>> +#ifdef CONFIG_SERIAL_EARLYCON
>>>> +	ret = icc_set_bw(wrapper->icc_path_geni_to_core, 0, 0);
>>> I think you only want to do this on the first suspend.
>> Ok, I can add that logic using global variable.
>>> Do we need to handle the case where no 'real' console is configured?
>>> In this case the early console would be active forever and setting
>>> the bandwidths to 0 might cause a similar crash than the one you are
>>> trying to fix. Not sure if that's a real world use case, but wanted to
>>> mention it. Maybe this is an argument of the notifier approach?
>> We can't support earlycon without real console.
>>
>> As earlyconsole doesn't do any kind of resource enablement(SE clocks,
>> pinctrl, etc) it assumes that resources are already enabled from previous
>> stages.
>>
>> So if real console doesn't come up no one will vote for that SE clock, and
>> it will be disabled from clk late_init call which will result into
>> un-clocked access.
> Ok, IIUC what you are saying is that earlycon can't work on its own after geni
> initialization. Because it clearly can work before (otherwise what would be
> its purpose?), supposedly because the bootloader configures the necessary
> bits.

If there is no real console then earlyconsole should work until clk 
driver disables unvoted SE clocks.

Yes, early console depends on bootloader to enable neccessary resources 
for it. I guess currently Coreboot console driver is doing that.

>
> In any case the bottom line is that earlycon requires a real console to be
> configured and there is no need to handle the corner case I brought up.

Yes that is correct. We can't support earlyconsole for long without 
having a real console.


Thanks,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Matthias Kaehlcke]

On Wed, Mar 18, 2020 at 02:24:35PM +0530, Akash Asthana wrote:
> Hi Matthias,
> 
> On 3/17/2020 11:59 PM, Matthias Kaehlcke wrote:
> > Hi Akash,
> > 
> > On Tue, Mar 17, 2020 at 04:27:47PM +0530, Akash Asthana wrote:
> > > Hi Matthias,
> > > 
> > > On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
> > > > Hi Akash,
> > > > 
> > > > On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
> > > > > V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
> > > > > to reset at boot time.
> > > > The v1 patch isn't relevant in the commit message, please just describe the
> > > > problem. Also the crash only occurs when earlycon is used.
> > > ok
> > > > > As QUP core clock is shared among all the SE drivers present on particular
> > > > > QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
> > > > > is put to 0 from other SE drivers before real console comes up.
> > > > > 
> > > > > As earlycon can't vote for it's QUP core need, to fix this add ICC
> > > > > support to common/QUP wrapper driver and put vote for QUP core from
> > > > > probe on behalf of earlycon and remove vote during sys suspend.
> > > > Only removing the vote on suspend isn't ideal, the system might never get
> > > > suspended. That said I don't have a really good alternative suggestion.
> > > > 
> > > > One thing you could possibly do is to launch a delayed work, check
> > > > console_device() every second or so and remove the vote when it returns
> > > > non-NULL. Not claiming this would be a great solution ...
> > > > 
> > > > The cleanest solution might be a notifier when the early console is
> > > > unregistered, it seems somewhat over-engineered though ... Then again
> > > > other (future) uart drivers with interconnect support might run into
> > > > the same problem.
> > > We are hitting this problem because QUP core clocks are shared among all the
> > > SE driver present in particular QUP wrapper, if other HW controllers has
> > > similar architecture we will hit this issue.
> > > 
> > > How about if we expose an API from common driver(geni-se) for putting QUP
> > > core BW vote to 0.
> > > 
> > > We call this from console probe just after uart_add_one_port call (console
> > > resources are enabled as part of this call) to put core quota to 0 on behalf
> > > of earlyconsole?
> >  From my notes from earlier debugging I have doubts this would work:
> > 
> >    There is a short window where the early console and the 'real' console coexist:
> > 
> >    [    3.858122] printk: console [ttyMSM0] enabled
> >    [    3.875692] printk: bootconsole [qcom_geni0] disabled
> > 
> >    The reset probably occurs when the early console tries to write, but the ICC
> >    is effectively disabled because ttyMSM0 and the other geni ports are runtime
> >    suspended.
> 
> Code flow from console driver probe(qcom_geni_serial.c)
> 
> uart_add_one_port--->uart_configure_port--->{ 1) uart_change_pm(enable
> console resources)  2)register_console(boot to real console switch happens
> here)}
> 
> Console resources are not disabled from anywhere before the switch happens
> completely. I meant to say until we saw below logs.
> 
> [    3.875692] printk: bootconsole [qcom_geni0] disabled
> 
> I think the board reset issue cannot occur during the window where early
> console and 'real' console coexist.

Thanks for the clarification! Indeed my notes were only a hypothesis, I
don't see evidence that there is an actual downvote shortly after console
registration.

> I have validated proposed solution by me, it is working fine.
> 
> Currently voting is done for every QUP and not only to which earlycon is
> connect, with the above approach we can't remove vote from other QUPs.
> 
> However we can limit voting only to earlycon QUP by removing interconnect
> from DT node of other QUPs.
> 
> I am not sure how clean is this solution.

I'm more inclined towards a solution along the lines of what Evan
proposed, i.e. delaying the votes (either in geni or ICC) until we
are ready.

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Akash Asthana]

Hi Evan, Matthias,

On 3/20/2020 1:13 AM, Matthias Kaehlcke wrote:
> On Wed, Mar 18, 2020 at 02:24:35PM +0530, Akash Asthana wrote:
>> Hi Matthias,
>>
>> On 3/17/2020 11:59 PM, Matthias Kaehlcke wrote:
>>> Hi Akash,
>>>
>>> On Tue, Mar 17, 2020 at 04:27:47PM +0530, Akash Asthana wrote:
>>>> Hi Matthias,
>>>>
>>>> On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
>>>>> Hi Akash,
>>>>>
>>>>> On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
>>>>>> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
>>>>>> to reset at boot time.
>>>>> The v1 patch isn't relevant in the commit message, please just describe the
>>>>> problem. Also the crash only occurs when earlycon is used.
>>>> ok
>>>>>> As QUP core clock is shared among all the SE drivers present on particular
>>>>>> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
>>>>>> is put to 0 from other SE drivers before real console comes up.
>>>>>>
>>>>>> As earlycon can't vote for it's QUP core need, to fix this add ICC
>>>>>> support to common/QUP wrapper driver and put vote for QUP core from
>>>>>> probe on behalf of earlycon and remove vote during sys suspend.
>>>>> Only removing the vote on suspend isn't ideal, the system might never get
>>>>> suspended. That said I don't have a really good alternative suggestion.
>>>>>
>>>>> One thing you could possibly do is to launch a delayed work, check
>>>>> console_device() every second or so and remove the vote when it returns
>>>>> non-NULL. Not claiming this would be a great solution ...
>>>>>
>>>>> The cleanest solution might be a notifier when the early console is
>>>>> unregistered, it seems somewhat over-engineered though ... Then again
>>>>> other (future) uart drivers with interconnect support might run into
>>>>> the same problem.
>>>> We are hitting this problem because QUP core clocks are shared among all the
>>>> SE driver present in particular QUP wrapper, if other HW controllers has
>>>> similar architecture we will hit this issue.
>>>>
>>>> How about if we expose an API from common driver(geni-se) for putting QUP
>>>> core BW vote to 0.
>>>>
>>>> We call this from console probe just after uart_add_one_port call (console
>>>> resources are enabled as part of this call) to put core quota to 0 on behalf
>>>> of earlyconsole?
>>>   From my notes from earlier debugging I have doubts this would work:
>>>
>>>     There is a short window where the early console and the 'real' console coexist:
>>>
>>>     [    3.858122] printk: console [ttyMSM0] enabled
>>>     [    3.875692] printk: bootconsole [qcom_geni0] disabled
>>>
>>>     The reset probably occurs when the early console tries to write, but the ICC
>>>     is effectively disabled because ttyMSM0 and the other geni ports are runtime
>>>     suspended.
>> Code flow from console driver probe(qcom_geni_serial.c)
>>
>> uart_add_one_port--->uart_configure_port--->{ 1) uart_change_pm(enable
>> console resources)  2)register_console(boot to real console switch happens
>> here)}
>>
>> Console resources are not disabled from anywhere before the switch happens
>> completely. I meant to say until we saw below logs.
>>
>> [    3.875692] printk: bootconsole [qcom_geni0] disabled
>>
>> I think the board reset issue cannot occur during the window where early
>> console and 'real' console coexist.
> Thanks for the clarification! Indeed my notes were only a hypothesis, I
> don't see evidence that there is an actual downvote shortly after console
> registration.
>
>> I have validated proposed solution by me, it is working fine.
>>
>> Currently voting is done for every QUP and not only to which earlycon is
>> connect, with the above approach we can't remove vote from other QUPs.
>>
>> However we can limit voting only to earlycon QUP by removing interconnect
>> from DT node of other QUPs.
>>
>> I am not sure how clean is this solution.
> I'm more inclined towards a solution along the lines of what Evan
> proposed, i.e. delaying the votes (either in geni or ICC) until we
> are ready.

Based on discussion I think the delayed solution is most suited if 
implemented in ICC core because other ICC client might face the similar 
problem.

However for geni case I am more inclined towards below proposed solution.

-----------------------------------------------------------------------------------------------------

How about if we expose an API from common driver(geni-se) for putting QUP
core BW vote to 0.

We call this from console probe just after uart_add_one_port call (console
resources are enabled as part of this call) to put core quota to 0 on behalf
of earlyconsole?
---------------------------------------------------------------------------

I think below are the pros and cons for above solution.

Pros:

1) Not only it'll solve the current issue also it'll lay foundation to 
support earlyconsole without having a real console. In future if needed 
we can enable rest of the earlyconsole resources(SE clocks) from common 
driver to make it independent of real console.

2) Inorder to solve bug:120934049(Add runtime PM support to common 
driver and move enablement/disablement of AHB clocks there) reported by 
Stephen Boyd, we'll end up adding ICC support to this driver because as 
per HPG the order of enabling the QUP clocks should be 
(Core-->AHB-->SE(per engine)). So we need to have the minimal possible 
vote on QUP core before enabling AHB clocks.

I agree still we have keep earlycon fix as it is(vote from common driver 
probe and remove once real console is up) .

I mentioned above bug in case it seems silly to add ICC support to 
common driver just for earlyconsole fix.

Cons:

1) As Evan mentioned and I agree that the whole house of cards comes 
down if we replace these votes in the wrong order. But I think this is 
very unlikely to happen once fixed.

2) As Matthias mentioned we are voting for all the QUPs regardless 
earlycon is present or not, I think this is HW limitation and we have to 
live with it.

Even if I try something like below the QUP1 can come up before 
QUP2(console) and child of QUP1 can remove vote from share QUP clocks.

if (of_get_compatible_child(pdev->dev.of_node, "qcom,geni-debug-uart")) {

     ret = icc_set_bw(wrapper->icc_path_geni_to_core, Bps_to_icc(1000),

         Bps_to_icc(1000));

}


Thanks,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Evan Green]

On Fri, Mar 20, 2020 at 3:22 AM Akash Asthana <akashast@codeaurora.org> wrote:
>
> Hi Evan, Matthias,
>
> On 3/20/2020 1:13 AM, Matthias Kaehlcke wrote:
> > On Wed, Mar 18, 2020 at 02:24:35PM +0530, Akash Asthana wrote:
> >> Hi Matthias,
> >>
> >> On 3/17/2020 11:59 PM, Matthias Kaehlcke wrote:
> >>> Hi Akash,
> >>>
> >>> On Tue, Mar 17, 2020 at 04:27:47PM +0530, Akash Asthana wrote:
> >>>> Hi Matthias,
> >>>>
> >>>> On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
> >>>>> Hi Akash,
> >>>>>
> >>>>> On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
> >>>>>> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
> >>>>>> to reset at boot time.
> >>>>> The v1 patch isn't relevant in the commit message, please just describe the
> >>>>> problem. Also the crash only occurs when earlycon is used.
> >>>> ok
> >>>>>> As QUP core clock is shared among all the SE drivers present on particular
> >>>>>> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
> >>>>>> is put to 0 from other SE drivers before real console comes up.
> >>>>>>
> >>>>>> As earlycon can't vote for it's QUP core need, to fix this add ICC
> >>>>>> support to common/QUP wrapper driver and put vote for QUP core from
> >>>>>> probe on behalf of earlycon and remove vote during sys suspend.
> >>>>> Only removing the vote on suspend isn't ideal, the system might never get
> >>>>> suspended. That said I don't have a really good alternative suggestion.
> >>>>>
> >>>>> One thing you could possibly do is to launch a delayed work, check
> >>>>> console_device() every second or so and remove the vote when it returns
> >>>>> non-NULL. Not claiming this would be a great solution ...
> >>>>>
> >>>>> The cleanest solution might be a notifier when the early console is
> >>>>> unregistered, it seems somewhat over-engineered though ... Then again
> >>>>> other (future) uart drivers with interconnect support might run into
> >>>>> the same problem.
> >>>> We are hitting this problem because QUP core clocks are shared among all the
> >>>> SE driver present in particular QUP wrapper, if other HW controllers has
> >>>> similar architecture we will hit this issue.
> >>>>
> >>>> How about if we expose an API from common driver(geni-se) for putting QUP
> >>>> core BW vote to 0.
> >>>>
> >>>> We call this from console probe just after uart_add_one_port call (console
> >>>> resources are enabled as part of this call) to put core quota to 0 on behalf
> >>>> of earlyconsole?
> >>>   From my notes from earlier debugging I have doubts this would work:
> >>>
> >>>     There is a short window where the early console and the 'real' console coexist:
> >>>
> >>>     [    3.858122] printk: console [ttyMSM0] enabled
> >>>     [    3.875692] printk: bootconsole [qcom_geni0] disabled
> >>>
> >>>     The reset probably occurs when the early console tries to write, but the ICC
> >>>     is effectively disabled because ttyMSM0 and the other geni ports are runtime
> >>>     suspended.
> >> Code flow from console driver probe(qcom_geni_serial.c)
> >>
> >> uart_add_one_port--->uart_configure_port--->{ 1) uart_change_pm(enable
> >> console resources)  2)register_console(boot to real console switch happens
> >> here)}
> >>
> >> Console resources are not disabled from anywhere before the switch happens
> >> completely. I meant to say until we saw below logs.
> >>
> >> [    3.875692] printk: bootconsole [qcom_geni0] disabled
> >>
> >> I think the board reset issue cannot occur during the window where early
> >> console and 'real' console coexist.
> > Thanks for the clarification! Indeed my notes were only a hypothesis, I
> > don't see evidence that there is an actual downvote shortly after console
> > registration.
> >
> >> I have validated proposed solution by me, it is working fine.
> >>
> >> Currently voting is done for every QUP and not only to which earlycon is
> >> connect, with the above approach we can't remove vote from other QUPs.
> >>
> >> However we can limit voting only to earlycon QUP by removing interconnect
> >> from DT node of other QUPs.
> >>
> >> I am not sure how clean is this solution.
> > I'm more inclined towards a solution along the lines of what Evan
> > proposed, i.e. delaying the votes (either in geni or ICC) until we
> > are ready.
>
> Based on discussion I think the delayed solution is most suited if
> implemented in ICC core because other ICC client might face the similar
> problem.
>
> However for geni case I am more inclined towards below proposed solution.
>
> -----------------------------------------------------------------------------------------------------
>
> How about if we expose an API from common driver(geni-se) for putting QUP
> core BW vote to 0.
>
> We call this from console probe just after uart_add_one_port call (console
> resources are enabled as part of this call) to put core quota to 0 on behalf
> of earlyconsole?

This seems ok to me. Earlycon sets up a vote, and then real probe
tears it down. As long as in the shuffle of all of these things into
SE library helpers you still have a way of differentiating the
earlycon vote from the real vote. In other words, don't reuse this
early icc_path for the real UART vote. You should probably also
destroy the path once you've voted zero on it.
-Evan

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Akash Asthana]

Hi Evan,

On 3/20/2020 10:00 PM, Evan Green wrote:
> On Fri, Mar 20, 2020 at 3:22 AM Akash Asthana <akashast@codeaurora.org> wrote:
>> Hi Evan, Matthias,
>>
>> On 3/20/2020 1:13 AM, Matthias Kaehlcke wrote:
>>> On Wed, Mar 18, 2020 at 02:24:35PM +0530, Akash Asthana wrote:
>>>> Hi Matthias,
>>>>
>>>> On 3/17/2020 11:59 PM, Matthias Kaehlcke wrote:
>>>>> Hi Akash,
>>>>>
>>>>> On Tue, Mar 17, 2020 at 04:27:47PM +0530, Akash Asthana wrote:
>>>>>> Hi Matthias,
>>>>>>
>>>>>> On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
>>>>>>> Hi Akash,
>>>>>>>
>>>>>>> On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
>>>>>>>> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
>>>>>>>> to reset at boot time.
>>>>>>> The v1 patch isn't relevant in the commit message, please just describe the
>>>>>>> problem. Also the crash only occurs when earlycon is used.
>>>>>> ok
>>>>>>>> As QUP core clock is shared among all the SE drivers present on particular
>>>>>>>> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
>>>>>>>> is put to 0 from other SE drivers before real console comes up.
>>>>>>>>
>>>>>>>> As earlycon can't vote for it's QUP core need, to fix this add ICC
>>>>>>>> support to common/QUP wrapper driver and put vote for QUP core from
>>>>>>>> probe on behalf of earlycon and remove vote during sys suspend.
>>>>>>> Only removing the vote on suspend isn't ideal, the system might never get
>>>>>>> suspended. That said I don't have a really good alternative suggestion.
>>>>>>>
>>>>>>> One thing you could possibly do is to launch a delayed work, check
>>>>>>> console_device() every second or so and remove the vote when it returns
>>>>>>> non-NULL. Not claiming this would be a great solution ...
>>>>>>>
>>>>>>> The cleanest solution might be a notifier when the early console is
>>>>>>> unregistered, it seems somewhat over-engineered though ... Then again
>>>>>>> other (future) uart drivers with interconnect support might run into
>>>>>>> the same problem.
>>>>>> We are hitting this problem because QUP core clocks are shared among all the
>>>>>> SE driver present in particular QUP wrapper, if other HW controllers has
>>>>>> similar architecture we will hit this issue.
>>>>>>
>>>>>> How about if we expose an API from common driver(geni-se) for putting QUP
>>>>>> core BW vote to 0.
>>>>>>
>>>>>> We call this from console probe just after uart_add_one_port call (console
>>>>>> resources are enabled as part of this call) to put core quota to 0 on behalf
>>>>>> of earlyconsole?
>>>>>    From my notes from earlier debugging I have doubts this would work:
>>>>>
>>>>>      There is a short window where the early console and the 'real' console coexist:
>>>>>
>>>>>      [    3.858122] printk: console [ttyMSM0] enabled
>>>>>      [    3.875692] printk: bootconsole [qcom_geni0] disabled
>>>>>
>>>>>      The reset probably occurs when the early console tries to write, but the ICC
>>>>>      is effectively disabled because ttyMSM0 and the other geni ports are runtime
>>>>>      suspended.
>>>> Code flow from console driver probe(qcom_geni_serial.c)
>>>>
>>>> uart_add_one_port--->uart_configure_port--->{ 1) uart_change_pm(enable
>>>> console resources)  2)register_console(boot to real console switch happens
>>>> here)}
>>>>
>>>> Console resources are not disabled from anywhere before the switch happens
>>>> completely. I meant to say until we saw below logs.
>>>>
>>>> [    3.875692] printk: bootconsole [qcom_geni0] disabled
>>>>
>>>> I think the board reset issue cannot occur during the window where early
>>>> console and 'real' console coexist.
>>> Thanks for the clarification! Indeed my notes were only a hypothesis, I
>>> don't see evidence that there is an actual downvote shortly after console
>>> registration.
>>>
>>>> I have validated proposed solution by me, it is working fine.
>>>>
>>>> Currently voting is done for every QUP and not only to which earlycon is
>>>> connect, with the above approach we can't remove vote from other QUPs.
>>>>
>>>> However we can limit voting only to earlycon QUP by removing interconnect
>>>> from DT node of other QUPs.
>>>>
>>>> I am not sure how clean is this solution.
>>> I'm more inclined towards a solution along the lines of what Evan
>>> proposed, i.e. delaying the votes (either in geni or ICC) until we
>>> are ready.
>> Based on discussion I think the delayed solution is most suited if
>> implemented in ICC core because other ICC client might face the similar
>> problem.
>>
>> However for geni case I am more inclined towards below proposed solution.
>>
>> -----------------------------------------------------------------------------------------------------
>>
>> How about if we expose an API from common driver(geni-se) for putting QUP
>> core BW vote to 0.
>>
>> We call this from console probe just after uart_add_one_port call (console
>> resources are enabled as part of this call) to put core quota to 0 on behalf
>> of earlyconsole?
> This seems ok to me. Earlycon sets up a vote, and then real probe
> tears it down. As long as in the shuffle of all of these things into
> SE library helpers you still have a way of differentiating the
> earlycon vote from the real vote. In other words, don't reuse this
> early icc_path for the real UART vote. You should probably also
> destroy the path once you've voted zero on it.
> -Evan

Thanks for confirming, I will not use early icc_path for real console 
and I will destroy it once voted to 0 from real console probe.

Regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Bjorn Andersson]

On Fri 20 Mar 09:30 PDT 2020, Evan Green wrote:

> On Fri, Mar 20, 2020 at 3:22 AM Akash Asthana <akashast@codeaurora.org> wrote:
> >
> > Hi Evan, Matthias,
> >
> > On 3/20/2020 1:13 AM, Matthias Kaehlcke wrote:
> > > On Wed, Mar 18, 2020 at 02:24:35PM +0530, Akash Asthana wrote:
> > >> Hi Matthias,
> > >>
> > >> On 3/17/2020 11:59 PM, Matthias Kaehlcke wrote:
> > >>> Hi Akash,
> > >>>
> > >>> On Tue, Mar 17, 2020 at 04:27:47PM +0530, Akash Asthana wrote:
> > >>>> Hi Matthias,
> > >>>>
> > >>>> On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
> > >>>>> Hi Akash,
> > >>>>>
> > >>>>> On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
> > >>>>>> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
> > >>>>>> to reset at boot time.
> > >>>>> The v1 patch isn't relevant in the commit message, please just describe the
> > >>>>> problem. Also the crash only occurs when earlycon is used.
> > >>>> ok
> > >>>>>> As QUP core clock is shared among all the SE drivers present on particular
> > >>>>>> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
> > >>>>>> is put to 0 from other SE drivers before real console comes up.
> > >>>>>>
> > >>>>>> As earlycon can't vote for it's QUP core need, to fix this add ICC
> > >>>>>> support to common/QUP wrapper driver and put vote for QUP core from
> > >>>>>> probe on behalf of earlycon and remove vote during sys suspend.
> > >>>>> Only removing the vote on suspend isn't ideal, the system might never get
> > >>>>> suspended. That said I don't have a really good alternative suggestion.
> > >>>>>
> > >>>>> One thing you could possibly do is to launch a delayed work, check
> > >>>>> console_device() every second or so and remove the vote when it returns
> > >>>>> non-NULL. Not claiming this would be a great solution ...
> > >>>>>
> > >>>>> The cleanest solution might be a notifier when the early console is
> > >>>>> unregistered, it seems somewhat over-engineered though ... Then again
> > >>>>> other (future) uart drivers with interconnect support might run into
> > >>>>> the same problem.
> > >>>> We are hitting this problem because QUP core clocks are shared among all the
> > >>>> SE driver present in particular QUP wrapper, if other HW controllers has
> > >>>> similar architecture we will hit this issue.
> > >>>>
> > >>>> How about if we expose an API from common driver(geni-se) for putting QUP
> > >>>> core BW vote to 0.
> > >>>>
> > >>>> We call this from console probe just after uart_add_one_port call (console
> > >>>> resources are enabled as part of this call) to put core quota to 0 on behalf
> > >>>> of earlyconsole?
> > >>>   From my notes from earlier debugging I have doubts this would work:
> > >>>
> > >>>     There is a short window where the early console and the 'real' console coexist:
> > >>>
> > >>>     [    3.858122] printk: console [ttyMSM0] enabled
> > >>>     [    3.875692] printk: bootconsole [qcom_geni0] disabled
> > >>>
> > >>>     The reset probably occurs when the early console tries to write, but the ICC
> > >>>     is effectively disabled because ttyMSM0 and the other geni ports are runtime
> > >>>     suspended.
> > >> Code flow from console driver probe(qcom_geni_serial.c)
> > >>
> > >> uart_add_one_port--->uart_configure_port--->{ 1) uart_change_pm(enable
> > >> console resources)  2)register_console(boot to real console switch happens
> > >> here)}
> > >>
> > >> Console resources are not disabled from anywhere before the switch happens
> > >> completely. I meant to say until we saw below logs.
> > >>
> > >> [    3.875692] printk: bootconsole [qcom_geni0] disabled
> > >>
> > >> I think the board reset issue cannot occur during the window where early
> > >> console and 'real' console coexist.
> > > Thanks for the clarification! Indeed my notes were only a hypothesis, I
> > > don't see evidence that there is an actual downvote shortly after console
> > > registration.
> > >
> > >> I have validated proposed solution by me, it is working fine.
> > >>
> > >> Currently voting is done for every QUP and not only to which earlycon is
> > >> connect, with the above approach we can't remove vote from other QUPs.
> > >>
> > >> However we can limit voting only to earlycon QUP by removing interconnect
> > >> from DT node of other QUPs.
> > >>
> > >> I am not sure how clean is this solution.
> > > I'm more inclined towards a solution along the lines of what Evan
> > > proposed, i.e. delaying the votes (either in geni or ICC) until we
> > > are ready.
> >
> > Based on discussion I think the delayed solution is most suited if
> > implemented in ICC core because other ICC client might face the similar
> > problem.
> >
> > However for geni case I am more inclined towards below proposed solution.
> >
> > -----------------------------------------------------------------------------------------------------
> >
> > How about if we expose an API from common driver(geni-se) for putting QUP
> > core BW vote to 0.
> >
> > We call this from console probe just after uart_add_one_port call (console
> > resources are enabled as part of this call) to put core quota to 0 on behalf
> > of earlyconsole?
> 
> This seems ok to me. Earlycon sets up a vote, and then real probe
> tears it down. As long as in the shuffle of all of these things into
> SE library helpers you still have a way of differentiating the
> earlycon vote from the real vote.

Note though that the boot console will outlive the real console when the
kernel is booted with "keep_bootcon" on the command line (something I do
from time to time).

So rather than relying on "real probe" to signal when we can release the
earlycon's icc vote I think we should specify dev->con->exit in
qcom_geni_serial_earlycon_setup(), so that it will signal when the
earlycon actually goes away - and until that point the clocks should
just be on.

> In other words, don't reuse this
> early icc_path for the real UART vote. You should probably also
> destroy the path once you've voted zero on it.

Maintaining the early and real votes completely separate sounds like a
sure way to keep this sane; and there's no drawback on having multiple
votes for the same thing and rely on the framework to keep track of the
various users.

Regards,
Bjorn

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Akash Asthana]

Hi Bjorn,

On 3/28/2020 4:53 AM, Bjorn Andersson wrote:
> Note though that the boot console will outlive the real console when the
> kernel is booted with "keep_bootcon" on the command line (something I do
> from time to time).
>
> So rather than relying on "real probe" to signal when we can release the
> earlycon's icc vote I think we should specify dev->con->exit in
> qcom_geni_serial_early

Okay,

Thanks,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Evan Green]

On Tue, Mar 17, 2020 at 3:58 AM Akash Asthana <akashast@codeaurora.org> wrote:
>
> Hi Matthias,
>
> On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
> > Hi Akash,
> >
> > On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
> >> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
> >> to reset at boot time.
> > The v1 patch isn't relevant in the commit message, please just describe the
> > problem. Also the crash only occurs when earlycon is used.
> ok
> >
> >> As QUP core clock is shared among all the SE drivers present on particular
> >> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
> >> is put to 0 from other SE drivers before real console comes up.
> >>
> >> As earlycon can't vote for it's QUP core need, to fix this add ICC
> >> support to common/QUP wrapper driver and put vote for QUP core from
> >> probe on behalf of earlycon and remove vote during sys suspend.
> > Only removing the vote on suspend isn't ideal, the system might never get
> > suspended. That said I don't have a really good alternative suggestion.
> >
> > One thing you could possibly do is to launch a delayed work, check
> > console_device() every second or so and remove the vote when it returns
> > non-NULL. Not claiming this would be a great solution ...
> >
> > The cleanest solution might be a notifier when the early console is
> > unregistered, it seems somewhat over-engineered though ... Then again
> > other (future) uart drivers with interconnect support might run into
> > the same problem.
>
> We are hitting this problem because QUP core clocks are shared among all
> the SE driver present in particular QUP wrapper, if other HW controllers
> has similar architecture we will hit this issue.
>
> How about if we expose an API from common driver(geni-se) for putting
> QUP core BW vote to 0.
>
> We call this from console probe just after uart_add_one_port call
> (console resources are enabled as part of this call) to put core quota
> to 0 on behalf of earlyconsole?

+Georgi

Hm, these boot proxy votes are annoying, since the whole house of
cards comes down if you replace these votes in the wrong order.

I believe consensus in the other patches was to consolidate most of
the interconnect support into the common SE code, right? Would that
help you with these boot proxy votes? What I'm thinking is something
along the lines of:
 * SPI, I2C, UART all call into the new common geni_se_icc_on/off()
(or whatever it's called)
 * If geni_se_icc_off() sees that console UART hasn't voted yet, save
the votes but don't actually call icc_set(0) now.
 * Once uart votes for the first time, call icc_set() on all of SPI,
I2C, UART to get things back in sync.

That's a sort of roll-your-own solution for GENI, but we do have this
problem elsewhere as well. A more general solution would be to have
the interconnect providers prop things up (ie ignore votes to lower
bandwidth) until some "go" moment where we feel we've enumerated all
devices. I was originally thinking to model this off of something like
clk_disable_unused(), but after chatting with Stephen it's clear
late_initcall's aren't really indicative of all devices having
actually come up. So I'm not sure where the appropriate "go" moment
is.

-Evan


>
> >
> >> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> >> Reported-by: Matthias Kaehlcke <mka@chromium.org>
> >> ---
> >>   drivers/soc/qcom/qcom-geni-se.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 41 insertions(+)
> >>
> >> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> >> index 7d622ea..d244dfc 100644
> >> --- a/drivers/soc/qcom/qcom-geni-se.c
> >> +++ b/drivers/soc/qcom/qcom-geni-se.c
> >> @@ -90,6 +90,7 @@ struct geni_wrapper {
> >>      struct device *dev;
> >>      void __iomem *base;
> >>      struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
> >> +    struct icc_path *icc_path_geni_to_core;
> >>   };
> >>
> >>   #define QUP_HW_VER_REG                     0x4
> >> @@ -747,11 +748,50 @@ static int geni_se_probe(struct platform_device *pdev)
> >>              }
> >>      }
> >>
> >> +#ifdef CONFIG_SERIAL_EARLYCON
> >> +    wrapper->icc_path_geni_to_core = devm_of_icc_get(dev, "qup-core");
> >> +    if (IS_ERR(wrapper->icc_path_geni_to_core))
> >> +            return PTR_ERR(wrapper->icc_path_geni_to_core);
> >> +    /*
> >> +     * Put minmal BW request on core clocks on behalf of early console.
> >> +     * The vote will be removed in suspend call.
> >> +     */
> >> +    ret = icc_set_bw(wrapper->icc_path_geni_to_core, Bps_to_icc(1000),
> >> +                    Bps_to_icc(1000));
> >> +    if (ret) {
> >> +            dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
> >> +                    __func__);
> >> +            return ret;
> >> +    }
> > What is ugly about this is that it's done for every QUP, not only the one
> > with the early console. Again, I don't have a good solution for it, maybe
> > it's a limitation we have to live with :(
>
> There is one more limitation from QUP core side. Core clocks for both
> the QUP wrapper runs at same speed.
>
> core2x_1 = core2x_2 = max(core2x_1, core2x_2);
>
> So with above limitation and if we are removing early con vote from Core
> when real console comes up. It doesn't matter whether it's done for
> every QUP or the only with early console.
>
> >
> >> +#endif
> >> +
> >>      dev_set_drvdata(dev, wrapper);
> >>      dev_dbg(dev, "GENI SE Driver probed\n");
> >>      return devm_of_platform_populate(dev);
> >>   }
> >>
> >> +static int __maybe_unused geni_se_sys_suspend(struct device *dev)
> >> +{
> >> +    struct geni_wrapper *wrapper = dev_get_drvdata(dev);
> >> +    int ret;
> >> +
> >> +#ifdef CONFIG_SERIAL_EARLYCON
> >> +    ret = icc_set_bw(wrapper->icc_path_geni_to_core, 0, 0);
> > I think you only want to do this on the first suspend.
> Ok, I can add that logic using global variable.
> >
> > Do we need to handle the case where no 'real' console is configured?
> > In this case the early console would be active forever and setting
> > the bandwidths to 0 might cause a similar crash than the one you are
> > trying to fix. Not sure if that's a real world use case, but wanted to
> > mention it. Maybe this is an argument of the notifier approach?
> We can't support earlycon without real console.
>
> As earlyconsole doesn't do any kind of resource enablement(SE clocks,
> pinctrl, etc) it assumes that resources are already enabled from
> previous stages.
>
> So if real console doesn't come up no one will vote for that SE clock,
> and it will be disabled from clk late_init call which will result into
> un-clocked access.
>
>
> >
> >> +    if (ret) {
> >> +            dev_err(dev, "%s: ICC BW remove failed for core\n",
> >> +                    __func__);
> >> +            return ret;
> > Aborting suspend seems too harsh since the QUP should still be fully
> > functional unless there is a general problem with the interconnects.
> >
> > I would suggest to change the log to dev_warn() and return 0.
>
> Ok
>
> Thanks for reviewing the patch.
>
> regards,
>
> Akash
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Doug Anderson]

Hi,

On Tue, Mar 17, 2020 at 12:08 PM Evan Green <evgreen@chromium.org> wrote:
>
> On Tue, Mar 17, 2020 at 3:58 AM Akash Asthana <akashast@codeaurora.org> wrote:
> >
> > Hi Matthias,
> >
> > On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
> > > Hi Akash,
> > >
> > > On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
> > >> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
> > >> to reset at boot time.
> > > The v1 patch isn't relevant in the commit message, please just describe the
> > > problem. Also the crash only occurs when earlycon is used.
> > ok
> > >
> > >> As QUP core clock is shared among all the SE drivers present on particular
> > >> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
> > >> is put to 0 from other SE drivers before real console comes up.
> > >>
> > >> As earlycon can't vote for it's QUP core need, to fix this add ICC
> > >> support to common/QUP wrapper driver and put vote for QUP core from
> > >> probe on behalf of earlycon and remove vote during sys suspend.
> > > Only removing the vote on suspend isn't ideal, the system might never get
> > > suspended. That said I don't have a really good alternative suggestion.
> > >
> > > One thing you could possibly do is to launch a delayed work, check
> > > console_device() every second or so and remove the vote when it returns
> > > non-NULL. Not claiming this would be a great solution ...
> > >
> > > The cleanest solution might be a notifier when the early console is
> > > unregistered, it seems somewhat over-engineered though ... Then again
> > > other (future) uart drivers with interconnect support might run into
> > > the same problem.
> >
> > We are hitting this problem because QUP core clocks are shared among all
> > the SE driver present in particular QUP wrapper, if other HW controllers
> > has similar architecture we will hit this issue.
> >
> > How about if we expose an API from common driver(geni-se) for putting
> > QUP core BW vote to 0.
> >
> > We call this from console probe just after uart_add_one_port call
> > (console resources are enabled as part of this call) to put core quota
> > to 0 on behalf of earlyconsole?
>
> +Georgi
>
> Hm, these boot proxy votes are annoying, since the whole house of
> cards comes down if you replace these votes in the wrong order.
>
> I believe consensus in the other patches was to consolidate most of
> the interconnect support into the common SE code, right? Would that
> help you with these boot proxy votes? What I'm thinking is something
> along the lines of:
>  * SPI, I2C, UART all call into the new common geni_se_icc_on/off()
> (or whatever it's called)
>  * If geni_se_icc_off() sees that console UART hasn't voted yet, save
> the votes but don't actually call icc_set(0) now.
>  * Once uart votes for the first time, call icc_set() on all of SPI,
> I2C, UART to get things back in sync.
>
> That's a sort of roll-your-own solution for GENI, but we do have this
> problem elsewhere as well. A more general solution would be to have
> the interconnect providers prop things up (ie ignore votes to lower
> bandwidth) until some "go" moment where we feel we've enumerated all
> devices. I was originally thinking to model this off of something like
> clk_disable_unused(), but after chatting with Stephen it's clear
> late_initcall's aren't really indicative of all devices having
> actually come up. So I'm not sure where the appropriate "go" moment
> is.

I ran across this gem the other day, which explains why I get a bunch
of regulator yells 30 seconds after bootup:

/*
 * We punt completion for an arbitrary amount of time since
 * systems like distros will load many drivers from userspace
 * so consumers might not always be ready yet, this is
 * particularly an issue with laptops where this might bounce
 * the display off then on.  Ideally we'd get a notification
 * from userspace when this happens but we don't so just wait
 * a bit and hope we waited long enough.  It'd be better if
 * we'd only do this on systems that need it, and a kernel
 * command line option might be useful.
 */
schedule_delayed_work(&regulator_init_complete_work,
      msecs_to_jiffies(30000));

...but that also means that this is basically an unsolved problem.  I
suppose one thing you could do would be to centralize this "30 seconds
after bootup" for several subsystems (regulator, clock, interconnect,
...) and then at least it would leave a nice place for someone to do
better...  ;-)

-Doug

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Akash Asthana]

Hi Evan

On 3/18/2020 12:38 AM, Evan Green wrote:
> On Tue, Mar 17, 2020 at 3:58 AM Akash Asthana <akashast@codeaurora.org> wrote:
>> Hi Matthias,
>>
>> On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
>>> Hi Akash,
>>>
>>> On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
>>>> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
>>>> to reset at boot time.
>>> The v1 patch isn't relevant in the commit message, please just describe the
>>> problem. Also the crash only occurs when earlycon is used.
>> ok
>>>> As QUP core clock is shared among all the SE drivers present on particular
>>>> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
>>>> is put to 0 from other SE drivers before real console comes up.
>>>>
>>>> As earlycon can't vote for it's QUP core need, to fix this add ICC
>>>> support to common/QUP wrapper driver and put vote for QUP core from
>>>> probe on behalf of earlycon and remove vote during sys suspend.
>>> Only removing the vote on suspend isn't ideal, the system might never get
>>> suspended. That said I don't have a really good alternative suggestion.
>>>
>>> One thing you could possibly do is to launch a delayed work, check
>>> console_device() every second or so and remove the vote when it returns
>>> non-NULL. Not claiming this would be a great solution ...
>>>
>>> The cleanest solution might be a notifier when the early console is
>>> unregistered, it seems somewhat over-engineered though ... Then again
>>> other (future) uart drivers with interconnect support might run into
>>> the same problem.
>> We are hitting this problem because QUP core clocks are shared among all
>> the SE driver present in particular QUP wrapper, if other HW controllers
>> has similar architecture we will hit this issue.
>>
>> How about if we expose an API from common driver(geni-se) for putting
>> QUP core BW vote to 0.
>>
>> We call this from console probe just after uart_add_one_port call
>> (console resources are enabled as part of this call) to put core quota
>> to 0 on behalf of earlyconsole?
> +Georgi
>
> Hm, these boot proxy votes are annoying, since the whole house of
> cards comes down if you replace these votes in the wrong order.
>
> I believe consensus in the other patches was to consolidate most of
> the interconnect support into the common SE code, right?

I think what Matthias suggested is to maintain ICC functions defined 
across I2C, SPI and UART as a library in common SE code.

Still every SE driver will interact with ICC framework individually 
rather than using common SE driver as a bridge.

>   Would that
> help you with these boot proxy votes? What I'm thinking is something
> along the lines of:
>   * SPI, I2C, UART all call into the new common geni_se_icc_on/off()
> (or whatever it's called)
>   * If geni_se_icc_off() sees that console UART hasn't voted yet, save
> the votes but don't actually call icc_set(0) now.
>   * Once uart votes for the first time, call icc_set() on all of SPI,
> I2C, UART to get things back in sync.

IIUC, you are suggesting to enhancing ICC 
design@https://patchwork.kernel.org/patch/10774897/ [The very first ICC 
patch posted during sdm845 timeframe].

Where common SE driver aggregate real time BW requirement from all the 
SE driver and put net request to ICC framework.

We received comments on that version of ICC to move voting to individual 
SE driver from common driver. Hence we updated the design accordingly.

Thanks for reviewing

regards,

Akash

> That's a sort of roll-your-own solution for GENI, but we do have this
> problem elsewhere as well. A more general solution would be to have
> the interconnect providers prop things up (ie ignore votes to lower
> bandwidth) until some "go" moment where we feel we've enumerated all
> devices. I was originally thinking to model this off of something like
> clk_disable_unused(), but after chatting with Stephen it's clear
> late_initcall's aren't really indicative of all devices having
> actually come up. So I'm not sure where the appropriate "go" moment
> is.
>
> -Evan
>
>
>>>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>>>> Reported-by: Matthias Kaehlcke <mka@chromium.org>
>>>> ---
>>>>    drivers/soc/qcom/qcom-geni-se.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 41 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>>>> index 7d622ea..d244dfc 100644
>>>> --- a/drivers/soc/qcom/qcom-geni-se.c
>>>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>>>> @@ -90,6 +90,7 @@ struct geni_wrapper {
>>>>       struct device *dev;
>>>>       void __iomem *base;
>>>>       struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
>>>> +    struct icc_path *icc_path_geni_to_core;
>>>>    };
>>>>
>>>>    #define QUP_HW_VER_REG                     0x4
>>>> @@ -747,11 +748,50 @@ static int geni_se_probe(struct platform_device *pdev)
>>>>               }
>>>>       }
>>>>
>>>> +#ifdef CONFIG_SERIAL_EARLYCON
>>>> +    wrapper->icc_path_geni_to_core = devm_of_icc_get(dev, "qup-core");
>>>> +    if (IS_ERR(wrapper->icc_path_geni_to_core))
>>>> +            return PTR_ERR(wrapper->icc_path_geni_to_core);
>>>> +    /*
>>>> +     * Put minmal BW request on core clocks on behalf of early console.
>>>> +     * The vote will be removed in suspend call.
>>>> +     */
>>>> +    ret = icc_set_bw(wrapper->icc_path_geni_to_core, Bps_to_icc(1000),
>>>> +                    Bps_to_icc(1000));
>>>> +    if (ret) {
>>>> +            dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
>>>> +                    __func__);
>>>> +            return ret;
>>>> +    }
>>> What is ugly about this is that it's done for every QUP, not only the one
>>> with the early console. Again, I don't have a good solution for it, maybe
>>> it's a limitation we have to live with :(
>> There is one more limitation from QUP core side. Core clocks for both
>> the QUP wrapper runs at same speed.
>>
>> core2x_1 = core2x_2 = max(core2x_1, core2x_2);
>>
>> So with above limitation and if we are removing early con vote from Core
>> when real console comes up. It doesn't matter whether it's done for
>> every QUP or the only with early console.
>>
>>>> +#endif
>>>> +
>>>>       dev_set_drvdata(dev, wrapper);
>>>>       dev_dbg(dev, "GENI SE Driver probed\n");
>>>>       return devm_of_platform_populate(dev);
>>>>    }
>>>>
>>>> +static int __maybe_unused geni_se_sys_suspend(struct device *dev)
>>>> +{
>>>> +    struct geni_wrapper *wrapper = dev_get_drvdata(dev);
>>>> +    int ret;
>>>> +
>>>> +#ifdef CONFIG_SERIAL_EARLYCON
>>>> +    ret = icc_set_bw(wrapper->icc_path_geni_to_core, 0, 0);
>>> I think you only want to do this on the first suspend.
>> Ok, I can add that logic using global variable.
>>> Do we need to handle the case where no 'real' console is configured?
>>> In this case the early console would be active forever and setting
>>> the bandwidths to 0 might cause a similar crash than the one you are
>>> trying to fix. Not sure if that's a real world use case, but wanted to
>>> mention it. Maybe this is an argument of the notifier approach?
>> We can't support earlycon without real console.
>>
>> As earlyconsole doesn't do any kind of resource enablement(SE clocks,
>> pinctrl, etc) it assumes that resources are already enabled from
>> previous stages.
>>
>> So if real console doesn't come up no one will vote for that SE clock,
>> and it will be disabled from clk late_init call which will result into
>> un-clocked access.
>>
>>
>>>> +    if (ret) {
>>>> +            dev_err(dev, "%s: ICC BW remove failed for core\n",
>>>> +                    __func__);
>>>> +            return ret;
>>> Aborting suspend seems too harsh since the QUP should still be fully
>>> functional unless there is a general problem with the interconnects.
>>>
>>> I would suggest to change the log to dev_warn() and return 0.
>> Ok
>>
>> Thanks for reviewing the patch.
>>
>> regards,
>>
>> Akash
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Evan Green]

On Wed, Mar 18, 2020 at 3:57 AM Akash Asthana <akashast@codeaurora.org> wrote:
>
> Hi Evan
>
> On 3/18/2020 12:38 AM, Evan Green wrote:
> > On Tue, Mar 17, 2020 at 3:58 AM Akash Asthana <akashast@codeaurora.org> wrote:
> >> Hi Matthias,
> >>
> >> On 3/14/2020 2:14 AM, Matthias Kaehlcke wrote:
> >>> Hi Akash,
> >>>
> >>> On Fri, Mar 13, 2020 at 06:42:09PM +0530, Akash Asthana wrote:
> >>>> V1 patch@https://patchwork.kernel.org/patch/11386469/ caused SC7180 system
> >>>> to reset at boot time.
> >>> The v1 patch isn't relevant in the commit message, please just describe the
> >>> problem. Also the crash only occurs when earlycon is used.
> >> ok
> >>>> As QUP core clock is shared among all the SE drivers present on particular
> >>>> QUP wrapper, the reset seen is due to earlycon usage after QUP core clock
> >>>> is put to 0 from other SE drivers before real console comes up.
> >>>>
> >>>> As earlycon can't vote for it's QUP core need, to fix this add ICC
> >>>> support to common/QUP wrapper driver and put vote for QUP core from
> >>>> probe on behalf of earlycon and remove vote during sys suspend.
> >>> Only removing the vote on suspend isn't ideal, the system might never get
> >>> suspended. That said I don't have a really good alternative suggestion.
> >>>
> >>> One thing you could possibly do is to launch a delayed work, check
> >>> console_device() every second or so and remove the vote when it returns
> >>> non-NULL. Not claiming this would be a great solution ...
> >>>
> >>> The cleanest solution might be a notifier when the early console is
> >>> unregistered, it seems somewhat over-engineered though ... Then again
> >>> other (future) uart drivers with interconnect support might run into
> >>> the same problem.
> >> We are hitting this problem because QUP core clocks are shared among all
> >> the SE driver present in particular QUP wrapper, if other HW controllers
> >> has similar architecture we will hit this issue.
> >>
> >> How about if we expose an API from common driver(geni-se) for putting
> >> QUP core BW vote to 0.
> >>
> >> We call this from console probe just after uart_add_one_port call
> >> (console resources are enabled as part of this call) to put core quota
> >> to 0 on behalf of earlyconsole?
> > +Georgi
> >
> > Hm, these boot proxy votes are annoying, since the whole house of
> > cards comes down if you replace these votes in the wrong order.
> >
> > I believe consensus in the other patches was to consolidate most of
> > the interconnect support into the common SE code, right?
>
> I think what Matthias suggested is to maintain ICC functions defined
> across I2C, SPI and UART as a library in common SE code.
>
> Still every SE driver will interact with ICC framework individually
> rather than using common SE driver as a bridge.

Right, I'm sort of proposing a blend here, where the individual
drivers pass through the SE library, which looks at some shared state,
and may defer sending the votes during boot time. I was thinking
consolidating this into SE engine library code may make it easier for
you to peek at that shared state.

>
> >   Would that
> > help you with these boot proxy votes? What I'm thinking is something
> > along the lines of:
> >   * SPI, I2C, UART all call into the new common geni_se_icc_on/off()
> > (or whatever it's called)
> >   * If geni_se_icc_off() sees that console UART hasn't voted yet, save
> > the votes but don't actually call icc_set(0) now.
> >   * Once uart votes for the first time, call icc_set() on all of SPI,
> > I2C, UART to get things back in sync.
>
> IIUC, you are suggesting to enhancing ICC
> design@https://patchwork.kernel.org/patch/10774897/ [The very first ICC
> patch posted during sdm845 timeframe].
>
> Where common SE driver aggregate real time BW requirement from all the
> SE driver and put net request to ICC framework.
>
> We received comments on that version of ICC to move voting to individual
> SE driver from common driver. Hence we updated the design accordingly.

I think most of the reaction to that original series came from the
fact that the common SE code was doing aggregation work, which is
something the interconnect core was designed to do. In the solution
I'm proposing, the SE library either passes through votes as-is, or
delays them until the console UART has voted, at which time it passes
them all down as they were.

You could still make the case this is something the interconnect core
should help us with, which is why I was brainstorming about the
provider propping up votes until some probe-finished deadline, maybe
just a 30 second timer :)
-Evan

[PATCH V2 4/8] tty: serial: qcom_geni_serial: Add interconnect support

[Akash Asthana]

Get the interconnect paths for Uart based Serial Engine device
and vote according to the baud rate requirement of the driver.

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in V2:
 - As per Bjorn's comment, removed se == NULL check from geni_serial_icc_get
 - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
 - 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/tty/serial/qcom_geni_serial.c | 69 +++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 272bae0..c8ad7e9 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -184,6 +184,19 @@ static struct qcom_geni_serial_port qcom_geni_console_port = {
 	},
 };
 
+static int geni_serial_icc_get(struct geni_se *se)
+{
+	se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core");
+	if (IS_ERR(se->icc_path_geni_to_core))
+		return PTR_ERR(se->icc_path_geni_to_core);
+
+	se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config");
+	if (IS_ERR(se->icc_path_cpu_to_geni))
+		return PTR_ERR(se->icc_path_cpu_to_geni);
+
+	return 0;
+}
+
 static int qcom_geni_serial_request_port(struct uart_port *uport)
 {
 	struct platform_device *pdev = to_platform_device(uport->dev);
@@ -962,6 +975,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
 	unsigned long clk_rate;
 	u32 ver, sampling_rate;
+	int ret;
 
 	qcom_geni_serial_stop_rx(uport);
 	/* baud rate */
@@ -983,6 +997,18 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	ser_clk_cfg = SER_CLK_EN;
 	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
 
+	/*
+	 * Put BW vote only on CPU path as driver supports FIFO mode only.
+	 * Assume peak_bw as twice of avg_bw.
+	 */
+	port->se.avg_bw_cpu = Bps_to_icc(baud);
+	port->se.peak_bw_cpu = Bps_to_icc(2 * baud);
+	ret = icc_set_bw(port->se.icc_path_cpu_to_geni, port->se.avg_bw_cpu,
+			port->se.peak_bw_cpu);
+	if (ret)
+		dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n",
+			__func__);
+
 	/* parity */
 	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
 	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
@@ -1208,16 +1234,40 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
 		unsigned int new_state, unsigned int old_state)
 {
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
-
+	int ret;
 	/* If we've never been called, treat it as off */
 	if (old_state == UART_PM_STATE_UNDEFINED)
 		old_state = UART_PM_STATE_OFF;
 
-	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
+	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) {
+		/* Put BW vote for core clocks and CPU */
+		ret = icc_set_bw(port->se.icc_path_geni_to_core,
+			port->se.avg_bw_core, port->se.peak_bw_core);
+		if (ret)
+			dev_err(uport->dev, "%s: ICC BW voting failed for core\n",
+				__func__);
+
+		ret = icc_set_bw(port->se.icc_path_cpu_to_geni,
+			port->se.avg_bw_cpu, port->se.peak_bw_cpu);
+		if (ret)
+			dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n",
+				__func__);
+
 		geni_se_resources_on(&port->se);
-	else if (new_state == UART_PM_STATE_OFF &&
-			old_state == UART_PM_STATE_ON)
+	} else if (new_state == UART_PM_STATE_OFF &&
+			old_state == UART_PM_STATE_ON) {
 		geni_se_resources_off(&port->se);
+		/* Remove BW vote from core clocks and CPU */
+		ret = icc_set_bw(port->se.icc_path_geni_to_core, 0, 0);
+		if (ret)
+			dev_err(uport->dev, "%s: ICC BW remove failed for core\n",
+				__func__);
+
+		ret = icc_set_bw(port->se.icc_path_cpu_to_geni, 0, 0);
+		if (ret)
+			dev_err(uport->dev, "%s: ICC BW remove failed for cpu\n",
+				__func__);
+	}
 }
 
 static const struct uart_ops qcom_geni_console_pops = {
@@ -1308,6 +1358,17 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
 	port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
 
+	ret = geni_serial_icc_get(&port->se);
+	if (ret)
+		return ret;
+	/* Set the bus quota to a reasonable value */
+	port->se.avg_bw_core = console ? Bps_to_icc(1000) :
+		Bps_to_icc(CORE_2X_50_MHZ);
+	port->se.peak_bw_core = console ? Bps_to_icc(1000) :
+		Bps_to_icc(CORE_2X_100_MHZ);
+	port->se.avg_bw_cpu = Bps_to_icc(1000);
+	port->se.peak_bw_cpu = Bps_to_icc(1000);
+
 	port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
 			"qcom_geni_serial_%s%d",
 			uart_console(uport) ? "console" : "uart", uport->line);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 4/8] tty: serial: qcom_geni_serial: Add interconnect support

[Matthias Kaehlcke]

Hi Akash,

On Fri, Mar 13, 2020 at 06:42:10PM +0530, Akash Asthana wrote:
> Get the interconnect paths for Uart based Serial Engine device
> and vote according to the baud rate requirement of the driver.
> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V2:
>  - As per Bjorn's comment, removed se == NULL check from geni_serial_icc_get
>  - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
>  - 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/tty/serial/qcom_geni_serial.c | 69 +++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 272bae0..c8ad7e9 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -184,6 +184,19 @@ static struct qcom_geni_serial_port qcom_geni_console_port = {
>  	},
>  };
>  
> +static int geni_serial_icc_get(struct geni_se *se)
> +{
> +	se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core");
> +	if (IS_ERR(se->icc_path_geni_to_core))
> +		return PTR_ERR(se->icc_path_geni_to_core);
> +
> +	se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config");
> +	if (IS_ERR(se->icc_path_cpu_to_geni))
> +		return PTR_ERR(se->icc_path_cpu_to_geni);
> +
> +	return 0;
> +}
> +
>  static int qcom_geni_serial_request_port(struct uart_port *uport)
>  {
>  	struct platform_device *pdev = to_platform_device(uport->dev);
> @@ -962,6 +975,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>  	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>  	unsigned long clk_rate;
>  	u32 ver, sampling_rate;
> +	int ret;
>  
>  	qcom_geni_serial_stop_rx(uport);
>  	/* baud rate */
> @@ -983,6 +997,18 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>  	ser_clk_cfg = SER_CLK_EN;
>  	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
>  
> +	/*
> +	 * Put BW vote only on CPU path as driver supports FIFO mode only.
> +	 * Assume peak_bw as twice of avg_bw.
> +	 */
> +	port->se.avg_bw_cpu = Bps_to_icc(baud);
> +	port->se.peak_bw_cpu = Bps_to_icc(2 * baud);
> +	ret = icc_set_bw(port->se.icc_path_cpu_to_geni, port->se.avg_bw_cpu,
> +			port->se.peak_bw_cpu);
> +	if (ret)
> +		dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n",
> +			__func__);

Should this return an error? The port might not operate properly if the ICC
bandwidth couldn't be configured


> +
>  	/* parity */
>  	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
>  	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
> @@ -1208,16 +1234,40 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>  		unsigned int new_state, unsigned int old_state)
>  {
>  	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> -
> +	int ret;
>  	/* If we've never been called, treat it as off */
>  	if (old_state == UART_PM_STATE_UNDEFINED)
>  		old_state = UART_PM_STATE_OFF;
>  
> -	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
> +	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) {
> +		/* Put BW vote for core clocks and CPU */
> +		ret = icc_set_bw(port->se.icc_path_geni_to_core,
> +			port->se.avg_bw_core, port->se.peak_bw_core);
> +		if (ret)
> +			dev_err(uport->dev, "%s: ICC BW voting failed for core\n",
> +				__func__);
> +
> +		ret = icc_set_bw(port->se.icc_path_cpu_to_geni,
> +			port->se.avg_bw_cpu, port->se.peak_bw_cpu);
> +		if (ret)
> +			dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n",
> +				__func__);
> +
>  		geni_se_resources_on(&port->se);
> -	else if (new_state == UART_PM_STATE_OFF &&
> -			old_state == UART_PM_STATE_ON)
> +	} else if (new_state == UART_PM_STATE_OFF &&
> +			old_state == UART_PM_STATE_ON) {
>  		geni_se_resources_off(&port->se);
> +		/* Remove BW vote from core clocks and CPU */
> +		ret = icc_set_bw(port->se.icc_path_geni_to_core, 0, 0);
> +		if (ret)
> +			dev_err(uport->dev, "%s: ICC BW remove failed for core\n",
> +				__func__);
> +
> +		ret = icc_set_bw(port->se.icc_path_cpu_to_geni, 0, 0);
> +		if (ret)
> +			dev_err(uport->dev, "%s: ICC BW remove failed for cpu\n",
> +				__func__);
> +	}
>  }
>  
>  static const struct uart_ops qcom_geni_console_pops = {
> @@ -1308,6 +1358,17 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  	port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>  	port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>  
> +	ret = geni_serial_icc_get(&port->se);
> +	if (ret)
> +		return ret;
> +	/* Set the bus quota to a reasonable value */
> +	port->se.avg_bw_core = console ? Bps_to_icc(1000) :
> +		Bps_to_icc(CORE_2X_50_MHZ);

Why different settings for console vs. non-console?

> +	port->se.peak_bw_core = console ? Bps_to_icc(1000) :
> +		Bps_to_icc(CORE_2X_100_MHZ);
> +	port->se.avg_bw_cpu = Bps_to_icc(1000);
> +	port->se.peak_bw_cpu = Bps_to_icc(1000);

'Bps_to_icc(1000)' is a recurring theme in this series, could it be worth
to have a constant for that? Could be GENI_DEFAULT_BW or similar.

Re: [PATCH V2 4/8] tty: serial: qcom_geni_serial: Add interconnect support

[Akash Asthana]

Hi Matthias,

On 3/14/2020 2:58 AM, Matthias Kaehlcke wrote:
> Hi Akash,
>
> On Fri, Mar 13, 2020 at 06:42:10PM +0530, Akash Asthana wrote:
>> Get the interconnect paths for Uart based Serial Engine device
>> and vote according to the baud rate requirement of the driver.
>>
>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>> Changes in V2:
>>   - As per Bjorn's comment, removed se == NULL check from geni_serial_icc_get
>>   - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
>>   - 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/tty/serial/qcom_geni_serial.c | 69 +++++++++++++++++++++++++++++++++--
>>   1 file changed, 65 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index 272bae0..c8ad7e9 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -184,6 +184,19 @@ static struct qcom_geni_serial_port qcom_geni_console_port = {
>>   	},
>>   };
>>   
>> +static int geni_serial_icc_get(struct geni_se *se)
>> +{
>> +	se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core");
>> +	if (IS_ERR(se->icc_path_geni_to_core))
>> +		return PTR_ERR(se->icc_path_geni_to_core);
>> +
>> +	se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config");
>> +	if (IS_ERR(se->icc_path_cpu_to_geni))
>> +		return PTR_ERR(se->icc_path_cpu_to_geni);
>> +
>> +	return 0;
>> +}
>> +
>>   static int qcom_geni_serial_request_port(struct uart_port *uport)
>>   {
>>   	struct platform_device *pdev = to_platform_device(uport->dev);
>> @@ -962,6 +975,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>   	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>>   	unsigned long clk_rate;
>>   	u32 ver, sampling_rate;
>> +	int ret;
>>   
>>   	qcom_geni_serial_stop_rx(uport);
>>   	/* baud rate */
>> @@ -983,6 +997,18 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>   	ser_clk_cfg = SER_CLK_EN;
>>   	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
>>   
>> +	/*
>> +	 * Put BW vote only on CPU path as driver supports FIFO mode only.
>> +	 * Assume peak_bw as twice of avg_bw.
>> +	 */
>> +	port->se.avg_bw_cpu = Bps_to_icc(baud);
>> +	port->se.peak_bw_cpu = Bps_to_icc(2 * baud);
>> +	ret = icc_set_bw(port->se.icc_path_cpu_to_geni, port->se.avg_bw_cpu,
>> +			port->se.peak_bw_cpu);
>> +	if (ret)
>> +		dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n",
>> +			__func__);
> Should this return an error? The port might not operate properly if the ICC
> bandwidth couldn't be configured

This is void function we can't return error from here. I guess it would 
be somewhat okay if BW voting failed for CPU path but clk_set_rate 
failure is more serious which is called from this function, I don't 
think it can be move to somewhere else.

>
>
>> +
>>   	/* parity */
>>   	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
>>   	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
>> @@ -1208,16 +1234,40 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>>   		unsigned int new_state, unsigned int old_state)
>>   {
>>   	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>> -
>> +	int ret;
>>   	/* If we've never been called, treat it as off */
>>   	if (old_state == UART_PM_STATE_UNDEFINED)
>>   		old_state = UART_PM_STATE_OFF;
>>   
>> -	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
>> +	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) {
>> +		/* Put BW vote for core clocks and CPU */
>> +		ret = icc_set_bw(port->se.icc_path_geni_to_core,
>> +			port->se.avg_bw_core, port->se.peak_bw_core);
>> +		if (ret)
>> +			dev_err(uport->dev, "%s: ICC BW voting failed for core\n",
>> +				__func__);
>> +
>> +		ret = icc_set_bw(port->se.icc_path_cpu_to_geni,
>> +			port->se.avg_bw_cpu, port->se.peak_bw_cpu);
>> +		if (ret)
>> +			dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n",
>> +				__func__);
>> +
>>   		geni_se_resources_on(&port->se);
>> -	else if (new_state == UART_PM_STATE_OFF &&
>> -			old_state == UART_PM_STATE_ON)
>> +	} else if (new_state == UART_PM_STATE_OFF &&
>> +			old_state == UART_PM_STATE_ON) {
>>   		geni_se_resources_off(&port->se);
>> +		/* Remove BW vote from core clocks and CPU */
>> +		ret = icc_set_bw(port->se.icc_path_geni_to_core, 0, 0);
>> +		if (ret)
>> +			dev_err(uport->dev, "%s: ICC BW remove failed for core\n",
>> +				__func__);
>> +
>> +		ret = icc_set_bw(port->se.icc_path_cpu_to_geni, 0, 0);
>> +		if (ret)
>> +			dev_err(uport->dev, "%s: ICC BW remove failed for cpu\n",
>> +				__func__);
>> +	}
>>   }
>>   
>>   static const struct uart_ops qcom_geni_console_pops = {
>> @@ -1308,6 +1358,17 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>   	port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>>   	port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>>   
>> +	ret = geni_serial_icc_get(&port->se);
>> +	if (ret)
>> +		return ret;
>> +	/* Set the bus quota to a reasonable value */
>> +	port->se.avg_bw_core = console ? Bps_to_icc(1000) :
>> +		Bps_to_icc(CORE_2X_50_MHZ);
> Why different settings for console vs. non-console?

QUP FW runs on core clock. To support higher throughput we want FW to 
run at higher speed.

Since Console operate at 115200bps and BT operate at 3.2Mbps baud. We 
are voting higher on core for BT usecase.

These value are recommended from HW team.

>
>> +	port->se.peak_bw_core = console ? Bps_to_icc(1000) :
>> +		Bps_to_icc(CORE_2X_100_MHZ);
>> +	port->se.avg_bw_cpu = Bps_to_icc(1000);
>> +	port->se.peak_bw_cpu = Bps_to_icc(1000);
> 'Bps_to_icc(1000)' is a recurring theme in this series, could it be worth
> to have a constant for that? Could be GENI_DEFAULT_BW or similar.

ok

Thanks for reviewing the patch.

Regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 4/8] tty: serial: qcom_geni_serial: Add interconnect support

[Matthias Kaehlcke]

On Tue, Mar 17, 2020 at 05:18:34PM +0530, Akash Asthana wrote:
> Hi Matthias,
> 
> On 3/14/2020 2:58 AM, Matthias Kaehlcke wrote:
> > Hi Akash,
> > 
> > On Fri, Mar 13, 2020 at 06:42:10PM +0530, Akash Asthana wrote:
> > > Get the interconnect paths for Uart based Serial Engine device
> > > and vote according to the baud rate requirement of the driver.
> > > 
> > > Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> > > ---
> > > Changes in V2:
> > >   - As per Bjorn's comment, removed se == NULL check from geni_serial_icc_get
> > >   - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
> > >   - 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/tty/serial/qcom_geni_serial.c | 69 +++++++++++++++++++++++++++++++++--
> > >   1 file changed, 65 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > > index 272bae0..c8ad7e9 100644
> > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > >
> > > ...
> > >
> > >   static int qcom_geni_serial_request_port(struct uart_port *uport)
> > >   {
> > >   	struct platform_device *pdev = to_platform_device(uport->dev);
> > > @@ -962,6 +975,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> > >   	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> > >   	unsigned long clk_rate;
> > >   	u32 ver, sampling_rate;
> > > +	int ret;
> > >   	qcom_geni_serial_stop_rx(uport);
> > >   	/* baud rate */
> > > @@ -983,6 +997,18 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> > >   	ser_clk_cfg = SER_CLK_EN;
> > >   	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
> > > +	/*
> > > +	 * Put BW vote only on CPU path as driver supports FIFO mode only.
> > > +	 * Assume peak_bw as twice of avg_bw.
> > > +	 */
> > > +	port->se.avg_bw_cpu = Bps_to_icc(baud);
> > > +	port->se.peak_bw_cpu = Bps_to_icc(2 * baud);
> > > +	ret = icc_set_bw(port->se.icc_path_cpu_to_geni, port->se.avg_bw_cpu,
> > > +			port->se.peak_bw_cpu);
> > > +	if (ret)
> > > +		dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n",
> > > +			__func__);
> > Should this return an error? The port might not operate properly if the ICC
> > bandwidth couldn't be configured
> 
> This is void function we can't return error from here. I guess it would be
> somewhat okay if BW voting failed for CPU path but clk_set_rate failure is
> more serious which is called from this function, I don't think it can be
> move to somewhere else.

ok, I missed that _set_termios() is void.

> > >   static const struct uart_ops qcom_geni_console_pops = {
> > > @@ -1308,6 +1358,17 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> > >   	port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> > >   	port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
> > > +	ret = geni_serial_icc_get(&port->se);
> > > +	if (ret)
> > > +		return ret;
> > > +	/* Set the bus quota to a reasonable value */
> > > +	port->se.avg_bw_core = console ? Bps_to_icc(1000) :
> > > +		Bps_to_icc(CORE_2X_50_MHZ);
> > Why different settings for console vs. non-console?
> 
> QUP FW runs on core clock. To support higher throughput we want FW to run at
> higher speed.
> 
> Since Console operate at 115200bps and BT operate at 3.2Mbps baud. We are
> voting higher on core for BT usecase.
> 
> These value are recommended from HW team.

IIUC none of the values you mention are set in stone. 115200bps seems to be a
'standard' value for the serial console, but it could be a different baudrate.
I guess you are referring to Qualcomm Bluetooth controllers, which are only one
of many things that could be connected to the port. And what happens when a
QCA BT controller is connected to a non-geni/QCA port, which doesn't know about
its 'requirements'? The answer is that both the BT controller and the serial
console configure the baudrate they need, hence using different values in
_probe() is pointless.

Unsurprisingly one of the first things the QCA BT driver does is to configure
the baudrate. It typically starts with a lower ('init') speed, and then switches
to the higher ('operational') baudrate:

https://elixir.bootlin.com/linux/v5.5.8/source/drivers/bluetooth/hci_qca.c#L1256

Re: [PATCH V2 4/8] tty: serial: qcom_geni_serial: Add interconnect support

[Akash Asthana]

Hi Matthias,

On 3/18/2020 12:38 AM, Matthias Kaehlcke wrote:
> On Tue, Mar 17, 2020 at 05:18:34PM +0530, Akash Asthana wrote:
>> Hi Matthias,
>>
>> On 3/14/2020 2:58 AM, Matthias Kaehlcke wrote:
>>> Hi Akash,
>>>
>>> On Fri, Mar 13, 2020 at 06:42:10PM +0530, Akash Asthana wrote:
>>>> Get the interconnect paths for Uart based Serial Engine device
>>>> and vote according to the baud rate requirement of the driver.
>>>>
>>>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>>>> ---
>>>> Changes in V2:
>>>>    - As per Bjorn's comment, removed se == NULL check from geni_serial_icc_get
>>>>    - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
>>>>    - 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/tty/serial/qcom_geni_serial.c | 69 +++++++++++++++++++++++++++++++++--
>>>>    1 file changed, 65 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>>>> index 272bae0..c8ad7e9 100644
>>>> --- a/drivers/tty/serial/qcom_geni_serial.c
>>>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>>>>
>>>> ...
>>>>
>>>>    static int qcom_geni_serial_request_port(struct uart_port *uport)
>>>>    {
>>>>    	struct platform_device *pdev = to_platform_device(uport->dev);
>>>> @@ -962,6 +975,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>>>    	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>>>>    	unsigned long clk_rate;
>>>>    	u32 ver, sampling_rate;
>>>> +	int ret;
>>>>    	qcom_geni_serial_stop_rx(uport);
>>>>    	/* baud rate */
>>>> @@ -983,6 +997,18 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>>>    	ser_clk_cfg = SER_CLK_EN;
>>>>    	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
>>>> +	/*
>>>> +	 * Put BW vote only on CPU path as driver supports FIFO mode only.
>>>> +	 * Assume peak_bw as twice of avg_bw.
>>>> +	 */
>>>> +	port->se.avg_bw_cpu = Bps_to_icc(baud);
>>>> +	port->se.peak_bw_cpu = Bps_to_icc(2 * baud);
>>>> +	ret = icc_set_bw(port->se.icc_path_cpu_to_geni, port->se.avg_bw_cpu,
>>>> +			port->se.peak_bw_cpu);
>>>> +	if (ret)
>>>> +		dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n",
>>>> +			__func__);
>>> Should this return an error? The port might not operate properly if the ICC
>>> bandwidth couldn't be configured
>> This is void function we can't return error from here. I guess it would be
>> somewhat okay if BW voting failed for CPU path but clk_set_rate failure is
>> more serious which is called from this function, I don't think it can be
>> move to somewhere else.
> ok, I missed that _set_termios() is void.
>
>>>>    static const struct uart_ops qcom_geni_console_pops = {
>>>> @@ -1308,6 +1358,17 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>>>    	port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>>>>    	port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>>>> +	ret = geni_serial_icc_get(&port->se);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +	/* Set the bus quota to a reasonable value */
>>>> +	port->se.avg_bw_core = console ? Bps_to_icc(1000) :
>>>> +		Bps_to_icc(CORE_2X_50_MHZ);
>>> Why different settings for console vs. non-console?
>> QUP FW runs on core clock. To support higher throughput we want FW to run at
>> higher speed.
>>
>> Since Console operate at 115200bps and BT operate at 3.2Mbps baud. We are
>> voting higher on core for BT usecase.
>>
>> These value are recommended from HW team.
> IIUC none of the values you mention are set in stone. 115200bps seems to be a
> 'standard' value for the serial console, but it could be a different baudrate.
> I guess you are referring to Qualcomm Bluetooth controllers, which are only one
> of many things that could be connected to the port. And what happens when a
> QCA BT controller is connected to a non-geni/QCA port, which doesn't know about
> its 'requirements'? The answer is that both the BT controller and the serial
> console configure the baudrate they need, hence using different values in
> _probe() is pointless.

Are you refering other UART drivers(not based on geni HW) as 
non-geni/QCA port?

We are not scaling core BW request based on real time need like we are 
doing for other paths(CPU/DDR) instead we are using some fail proof 
value because, FW runs on core clock and core behaves a bit different 
than other NOCs.

We don't have any functional relation which maps actual throughput 
requirement to core frequency need. In the past we faced few latency 
issues because of core slowness (Although it was running much higher 
than actual throughput requirement). To avoid such scenario we are using 
recommend value from HW team. These fix value can support SE drivers 
operating at their max possible speed(4Mbps in case of non-console).

I agree that 115200bps seems to be a 'standard' value for the serial 
console, but it could be a different baudrate.

We are voting 1000 in case of console because it  has low power mode 
use-case in android, where voting CORE_2X_50_MHZ can be reported as a 
power issue.

Actually we wanted to vote 960 for console but that is not possible with 
current ICC design where the minimum value is 1000bps.  So any way core 
is running at 50 MHz as 1000 crosses the threshold for 19.2 MHz (960)

only with console.

regards,

Akash

> Unsurprisingly one of the first things the QCA BT driver does is to configure
> the baudrate. It typically starts with a lower ('init') speed, and then switches
> to the higher ('operational') baudrate:
>
> https://elixir.bootlin.com/linux/v5.5.8/source/drivers/bluetooth/hci_qca.c#L1256

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 4/8] tty: serial: qcom_geni_serial: Add interconnect support

[Matthias Kaehlcke]

On Wed, Mar 18, 2020 at 05:53:22PM +0530, Akash Asthana wrote:
> Hi Matthias,
> 
> On 3/18/2020 12:38 AM, Matthias Kaehlcke wrote:
> > On Tue, Mar 17, 2020 at 05:18:34PM +0530, Akash Asthana wrote:
> > > Hi Matthias,
> > > 
> > > On 3/14/2020 2:58 AM, Matthias Kaehlcke wrote:
> > > > Hi Akash,
> > > > 
> > > > On Fri, Mar 13, 2020 at 06:42:10PM +0530, Akash Asthana wrote:
> > > > > Get the interconnect paths for Uart based Serial Engine device
> > > > > and vote according to the baud rate requirement of the driver.
> > > > > 
> > > > > Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> > > > > ---
> > > > > Changes in V2:
> > > > >    - As per Bjorn's comment, removed se == NULL check from geni_serial_icc_get
> > > > >    - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
> > > > >    - 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/tty/serial/qcom_geni_serial.c | 69 +++++++++++++++++++++++++++++++++--
> > > > >    1 file changed, 65 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > > > > index 272bae0..c8ad7e9 100644
> > > > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > > > > 
> > > > > ...
> > > > > 
> > > > >    static int qcom_geni_serial_request_port(struct uart_port *uport)
> > > > >    {
> > > > >    	struct platform_device *pdev = to_platform_device(uport->dev);
> > > > > @@ -962,6 +975,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> > > > >    	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> > > > >    	unsigned long clk_rate;
> > > > >    	u32 ver, sampling_rate;
> > > > > +	int ret;
> > > > >    	qcom_geni_serial_stop_rx(uport);
> > > > >    	/* baud rate */
> > > > > @@ -983,6 +997,18 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> > > > >    	ser_clk_cfg = SER_CLK_EN;
> > > > >    	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
> > > > > +	/*
> > > > > +	 * Put BW vote only on CPU path as driver supports FIFO mode only.
> > > > > +	 * Assume peak_bw as twice of avg_bw.
> > > > > +	 */
> > > > > +	port->se.avg_bw_cpu = Bps_to_icc(baud);
> > > > > +	port->se.peak_bw_cpu = Bps_to_icc(2 * baud);
> > > > > +	ret = icc_set_bw(port->se.icc_path_cpu_to_geni, port->se.avg_bw_cpu,
> > > > > +			port->se.peak_bw_cpu);
> > > > > +	if (ret)
> > > > > +		dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n",
> > > > > +			__func__);
> > > > Should this return an error? The port might not operate properly if the ICC
> > > > bandwidth couldn't be configured
> > > This is void function we can't return error from here. I guess it would be
> > > somewhat okay if BW voting failed for CPU path but clk_set_rate failure is
> > > more serious which is called from this function, I don't think it can be
> > > move to somewhere else.
> > ok, I missed that _set_termios() is void.
> > 
> > > > >    static const struct uart_ops qcom_geni_console_pops = {
> > > > > @@ -1308,6 +1358,17 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> > > > >    	port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> > > > >    	port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
> > > > > +	ret = geni_serial_icc_get(&port->se);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +	/* Set the bus quota to a reasonable value */
> > > > > +	port->se.avg_bw_core = console ? Bps_to_icc(1000) :
> > > > > +		Bps_to_icc(CORE_2X_50_MHZ);
> > > > Why different settings for console vs. non-console?
> > > QUP FW runs on core clock. To support higher throughput we want FW to run at
> > > higher speed.
> > > 
> > > Since Console operate at 115200bps and BT operate at 3.2Mbps baud. We are
> > > voting higher on core for BT usecase.
> > > 
> > > These value are recommended from HW team.
> > IIUC none of the values you mention are set in stone. 115200bps seems to be a
> > 'standard' value for the serial console, but it could be a different baudrate.
> > I guess you are referring to Qualcomm Bluetooth controllers, which are only one
> > of many things that could be connected to the port. And what happens when a
> > QCA BT controller is connected to a non-geni/QCA port, which doesn't know about
> > its 'requirements'? The answer is that both the BT controller and the serial
> > console configure the baudrate they need, hence using different values in
> > _probe() is pointless.
> 
> Are you refering other UART drivers(not based on geni HW) as non-geni/QCA
> port?
> 
> We are not scaling core BW request based on real time need like we are doing
> for other paths(CPU/DDR) instead we are using some fail proof value because,
> FW runs on core clock and core behaves a bit different than other NOCs.
>
> We don't have any functional relation which maps actual throughput
> requirement to core frequency need. In the past we faced few latency issues
> because of core slowness (Although it was running much higher than actual
> throughput requirement). To avoid such scenario we are using recommend value
> from HW team. These fix value can support SE drivers operating at their max
> possible speed(4Mbps in case of non-console).

ok, I missed that the core clocks aren't scaled based on the configured
baudrate. Apparently experience shows that it is not practical due to the
latency issues you mention.

> I agree that 115200bps seems to be a 'standard' value for the serial
> console, but it could be a different baudrate.
> 
> We are voting 1000 in case of console because it  has low power mode
> use-case in android, where voting CORE_2X_50_MHZ can be reported as a power
> issue.
> 
> Actually we wanted to vote 960 for console but that is not possible with
> current ICC design where the minimum value is 1000bps.  So any way core is
> running at 50 MHz as 1000 crosses the threshold for 19.2 MHz (960)
> 
> only with console.

Thanks for the clarification. So if a board wanted to use a higher baudrate
for the console it (currently) shouldn't be a problem. While it would be nice
to have uniform settings for all UARTs it's also not a big deal to have two
values, just wanted to make sure it's needed

Re: [PATCH V2 4/8] tty: serial: qcom_geni_serial: Add interconnect support

[Akash Asthana]

Hi Matthias,

On 3/20/2020 2:12 AM, Matthias Kaehlcke wrote:
> On Wed, Mar 18, 2020 at 05:53:22PM +0530, Akash Asthana wrote:
>> Hi Matthias,
>>
>> On 3/18/2020 12:38 AM, Matthias Kaehlcke wrote:
>>> On Tue, Mar 17, 2020 at 05:18:34PM +0530, Akash Asthana wrote:
>>>> Hi Matthias,
>>>>
>>>> On 3/14/2020 2:58 AM, Matthias Kaehlcke wrote:
>>>>> Hi Akash,
>>>>>
>>>>> On Fri, Mar 13, 2020 at 06:42:10PM +0530, Akash Asthana wrote:
>>>>>> Get the interconnect paths for Uart based Serial Engine device
>>>>>> and vote according to the baud rate requirement of the driver.
>>>>>>
>>>>>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>>>>>> ---
>>>>>> Changes in V2:
>>>>>>     - As per Bjorn's comment, removed se == NULL check from geni_serial_icc_get
>>>>>>     - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
>>>>>>     - 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/tty/serial/qcom_geni_serial.c | 69 +++++++++++++++++++++++++++++++++--
>>>>>>     1 file changed, 65 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>>>>>> index 272bae0..c8ad7e9 100644
>>>>>> --- a/drivers/tty/serial/qcom_geni_serial.c
>>>>>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>     static int qcom_geni_serial_request_port(struct uart_port *uport)
>>>>>>     {
>>>>>>     	struct platform_device *pdev = to_platform_device(uport->dev);
>>>>>> @@ -962,6 +975,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>>>>>     	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>>>>>>     	unsigned long clk_rate;
>>>>>>     	u32 ver, sampling_rate;
>>>>>> +	int ret;
>>>>>>     	qcom_geni_serial_stop_rx(uport);
>>>>>>     	/* baud rate */
>>>>>> @@ -983,6 +997,18 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>>>>>>     	ser_clk_cfg = SER_CLK_EN;
>>>>>>     	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
>>>>>> +	/*
>>>>>> +	 * Put BW vote only on CPU path as driver supports FIFO mode only.
>>>>>> +	 * Assume peak_bw as twice of avg_bw.
>>>>>> +	 */
>>>>>> +	port->se.avg_bw_cpu = Bps_to_icc(baud);
>>>>>> +	port->se.peak_bw_cpu = Bps_to_icc(2 * baud);
>>>>>> +	ret = icc_set_bw(port->se.icc_path_cpu_to_geni, port->se.avg_bw_cpu,
>>>>>> +			port->se.peak_bw_cpu);
>>>>>> +	if (ret)
>>>>>> +		dev_err(uport->dev, "%s: ICC BW voting failed for cpu\n",
>>>>>> +			__func__);
>>>>> Should this return an error? The port might not operate properly if the ICC
>>>>> bandwidth couldn't be configured
>>>> This is void function we can't return error from here. I guess it would be
>>>> somewhat okay if BW voting failed for CPU path but clk_set_rate failure is
>>>> more serious which is called from this function, I don't think it can be
>>>> move to somewhere else.
>>> ok, I missed that _set_termios() is void.
>>>
>>>>>>     static const struct uart_ops qcom_geni_console_pops = {
>>>>>> @@ -1308,6 +1358,17 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>>>>>     	port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>>>>>>     	port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>>>>>> +	ret = geni_serial_icc_get(&port->se);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +	/* Set the bus quota to a reasonable value */
>>>>>> +	port->se.avg_bw_core = console ? Bps_to_icc(1000) :
>>>>>> +		Bps_to_icc(CORE_2X_50_MHZ);
>>>>> Why different settings for console vs. non-console?
>>>> QUP FW runs on core clock. To support higher throughput we want FW to run at
>>>> higher speed.
>>>>
>>>> Since Console operate at 115200bps and BT operate at 3.2Mbps baud. We are
>>>> voting higher on core for BT usecase.
>>>>
>>>> These value are recommended from HW team.
>>> IIUC none of the values you mention are set in stone. 115200bps seems to be a
>>> 'standard' value for the serial console, but it could be a different baudrate.
>>> I guess you are referring to Qualcomm Bluetooth controllers, which are only one
>>> of many things that could be connected to the port. And what happens when a
>>> QCA BT controller is connected to a non-geni/QCA port, which doesn't know about
>>> its 'requirements'? The answer is that both the BT controller and the serial
>>> console configure the baudrate they need, hence using different values in
>>> _probe() is pointless.
>> Are you refering other UART drivers(not based on geni HW) as non-geni/QCA
>> port?
>>
>> We are not scaling core BW request based on real time need like we are doing
>> for other paths(CPU/DDR) instead we are using some fail proof value because,
>> FW runs on core clock and core behaves a bit different than other NOCs.
>>
>> We don't have any functional relation which maps actual throughput
>> requirement to core frequency need. In the past we faced few latency issues
>> because of core slowness (Although it was running much higher than actual
>> throughput requirement). To avoid such scenario we are using recommend value
>> from HW team. These fix value can support SE drivers operating at their max
>> possible speed(4Mbps in case of non-console).
> ok, I missed that the core clocks aren't scaled based on the configured
> baudrate. Apparently experience shows that it is not practical due to the
> latency issues you mention.
Yeah That is correct.
>> I agree that 115200bps seems to be a 'standard' value for the serial
>> console, but it could be a different baudrate.
>>
>> We are voting 1000 in case of console because it  has low power mode
>> use-case in android, where voting CORE_2X_50_MHZ can be reported as a power
>> issue.
>>
>> Actually we wanted to vote 960 for console but that is not possible with
>> current ICC design where the minimum value is 1000bps.  So any way core is
>> running at 50 MHz as 1000 crosses the threshold for 19.2 MHz (960)
>>
>> only with console.
> Thanks for the clarification. So if a board wanted to use a higher baudrate
> for the console it (currently) shouldn't be a problem. While it would be nice
> to have uniform settings for all UARTs it's also not a big deal to have two
> values, just wanted to make sure it's needed

Okay, but I would like to keep as it is with above mentioned reason.


Thanks,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

[PATCH V2 5/8] i2c: i2c-qcom-geni: Add interconnect support

[Akash Asthana]

Get the interconnect paths for I2C based Serial Engine device
and vote according to the bus speed of the driver.

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in V2:
 - As per Bjorn's comment, removed se == NULL check from geni_i2c_icc_get
 - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
 - 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/i2c/busses/i2c-qcom-geni.c | 110 +++++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 17abf60c..33ab685 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -163,6 +163,23 @@ static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
 	writel_relaxed(val, gi2c->se.base + SE_I2C_SCL_COUNTERS);
 }
 
+static int geni_i2c_icc_get(struct geni_se *se)
+{
+	se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core");
+	if (IS_ERR(se->icc_path_geni_to_core))
+		return PTR_ERR(se->icc_path_geni_to_core);
+
+	se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config");
+	if (IS_ERR(se->icc_path_cpu_to_geni))
+		return PTR_ERR(se->icc_path_cpu_to_geni);
+
+	se->icc_path_geni_to_ddr = devm_of_icc_get(se->dev, "qup-memory");
+	if (IS_ERR(se->icc_path_geni_to_ddr))
+		return PTR_ERR(se->icc_path_geni_to_ddr);
+
+	return 0;
+}
+
 static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
 {
 	u32 m_cmd = readl_relaxed(gi2c->se.base + SE_GENI_M_CMD0);
@@ -563,6 +580,39 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	gi2c->adap.dev.of_node = pdev->dev.of_node;
 	strlcpy(gi2c->adap.name, "Geni-I2C", sizeof(gi2c->adap.name));
 
+	ret = geni_i2c_icc_get(&gi2c->se);
+	if (ret)
+		return ret;
+	/*
+	 * Set the bus quota for core and cpu to a reasonable value for
+	 * register access.
+	 * Set quota for DDR based on bus speed, assume peak requirement
+	 * as twice of avg bw.
+	 */
+	gi2c->se.avg_bw_core = Bps_to_icc(1000);
+	gi2c->se.peak_bw_core = Bps_to_icc(CORE_2X_100_MHZ);
+	gi2c->se.avg_bw_cpu = Bps_to_icc(1000);
+	gi2c->se.peak_bw_cpu = Bps_to_icc(1000);
+	gi2c->se.avg_bw_ddr = Bps_to_icc(gi2c->clk_freq_out);
+	gi2c->se.peak_bw_ddr = Bps_to_icc(2 * gi2c->clk_freq_out);
+
+	/* Vote for core clocks and CPU for register access */
+	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
+				gi2c->se.peak_bw_core);
+	if (ret) {
+		dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
+			__func__);
+		return ret;
+	}
+
+	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, gi2c->se.avg_bw_cpu,
+				gi2c->se.peak_bw_cpu);
+	if (ret) {
+		dev_err(&pdev->dev, "%s: ICC BW voting failed for cpu\n",
+			__func__);
+		return ret;
+	}
+
 	ret = geni_se_resources_on(&gi2c->se);
 	if (ret) {
 		dev_err(&pdev->dev, "Error turning on resources %d\n", ret);
@@ -584,6 +634,19 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Error turning off resources %d\n", ret);
 		return ret;
 	}
+	/* Remove vote from core clocks and CPU */
+	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "%s: ICC BW remove failed for core\n",
+			__func__);
+		return ret;
+	}
+
+	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, 0, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "%s: ICC BW remove failed for cpu\n",
+			__func__);
+	}
 
 	dev_dbg(&pdev->dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
 
@@ -629,6 +692,28 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 		gi2c->suspended = 1;
 	}
 
+	/* Remove BW votes */
+	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
+	if (ret) {
+		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for core\n",
+			__func__);
+		return ret;
+	}
+
+	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, 0, 0);
+	if (ret) {
+		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for cpu\n",
+			__func__);
+		return ret;
+	}
+
+	ret = icc_set_bw(gi2c->se.icc_path_geni_to_ddr, 0, 0);
+	if (ret) {
+		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for ddr\n",
+			__func__);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -637,6 +722,31 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
 	int ret;
 	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
 
+	/* Vote on Core, CPU and DDR path respectively */
+	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
+		gi2c->se.peak_bw_core);
+	if (ret) {
+		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for core\n",
+			__func__);
+		return ret;
+	}
+
+	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, gi2c->se.avg_bw_cpu,
+		gi2c->se.peak_bw_cpu);
+	if (ret) {
+		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for cpu\n",
+			__func__);
+		return ret;
+	}
+
+	ret = icc_set_bw(gi2c->se.icc_path_geni_to_ddr, gi2c->se.avg_bw_ddr,
+		gi2c->se.peak_bw_ddr);
+	if (ret) {
+		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for ddr\n",
+			__func__);
+		return ret;
+	}
+
 	ret = geni_se_resources_on(&gi2c->se);
 	if (ret)
 		return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 5/8] i2c: i2c-qcom-geni: Add interconnect support

[Matthias Kaehlcke]

Hi,

On Fri, Mar 13, 2020 at 06:42:11PM +0530, Akash Asthana wrote:
> Get the interconnect paths for I2C based Serial Engine device
> and vote according to the bus speed of the driver.
> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V2:
>  - As per Bjorn's comment, removed se == NULL check from geni_i2c_icc_get
>  - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
>  - 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/i2c/busses/i2c-qcom-geni.c | 110 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 17abf60c..33ab685 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -163,6 +163,23 @@ static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
>  	writel_relaxed(val, gi2c->se.base + SE_I2C_SCL_COUNTERS);
>  }
>  
> +static int geni_i2c_icc_get(struct geni_se *se)
> +{
> +	se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core");
> +	if (IS_ERR(se->icc_path_geni_to_core))
> +		return PTR_ERR(se->icc_path_geni_to_core);
> +
> +	se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config");
> +	if (IS_ERR(se->icc_path_cpu_to_geni))
> +		return PTR_ERR(se->icc_path_cpu_to_geni);
> +
> +	se->icc_path_geni_to_ddr = devm_of_icc_get(se->dev, "qup-memory");
> +	if (IS_ERR(se->icc_path_geni_to_ddr))
> +		return PTR_ERR(se->icc_path_geni_to_ddr);
> +
> +	return 0;
> +}
> +
>  static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
>  {
>  	u32 m_cmd = readl_relaxed(gi2c->se.base + SE_GENI_M_CMD0);
> @@ -563,6 +580,39 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	gi2c->adap.dev.of_node = pdev->dev.of_node;
>  	strlcpy(gi2c->adap.name, "Geni-I2C", sizeof(gi2c->adap.name));
>  
> +	ret = geni_i2c_icc_get(&gi2c->se);
> +	if (ret)
> +		return ret;
> +	/*
> +	 * Set the bus quota for core and cpu to a reasonable value for
> +	 * register access.
> +	 * Set quota for DDR based on bus speed, assume peak requirement
> +	 * as twice of avg bw.
> +	 */
> +	gi2c->se.avg_bw_core = Bps_to_icc(1000);

as commented on the UART patch, you might want to consider adding a constant
for this recurring default value.

> +	gi2c->se.peak_bw_core = Bps_to_icc(CORE_2X_100_MHZ);
> +	gi2c->se.avg_bw_cpu = Bps_to_icc(1000);
> +	gi2c->se.peak_bw_cpu = Bps_to_icc(1000);
> +	gi2c->se.avg_bw_ddr = Bps_to_icc(gi2c->clk_freq_out);
> +	gi2c->se.peak_bw_ddr = Bps_to_icc(2 * gi2c->clk_freq_out);
> +
> +	/* Vote for core clocks and CPU for register access */
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
> +				gi2c->se.peak_bw_core);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, gi2c->se.avg_bw_cpu,
> +				gi2c->se.peak_bw_cpu);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ICC BW voting failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}
> +
>  	ret = geni_se_resources_on(&gi2c->se);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Error turning on resources %d\n", ret);
> @@ -584,6 +634,19 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "Error turning off resources %d\n", ret);
>  		return ret;
>  	}
> +	/* Remove vote from core clocks and CPU */
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ICC BW remove failed for core\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, 0, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ICC BW remove failed for cpu\n",
> +			__func__);

Should this return an error as for the core clocks?

> +	}
>  
>  	dev_dbg(&pdev->dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
>  
> @@ -629,6 +692,28 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>  		gi2c->suspended = 1;
>  	}
>  
> +	/* Remove BW votes */
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for core\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, 0, 0);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_ddr, 0, 0);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for ddr\n",
> +			__func__);
> +		return ret;
> +	}

you could consider putting this and the ICC voting in _runtime_resume() in
separate functions and call them from here and _probe(). The only difference
in _probe() is that it doesn't vote for the DDR bandwidth, but that slight
one-time overhead might be worth for consolidating the code.

Actually all this looks very similar for UART, I2C, SPI and QSPI. It seems it
should be possible to consolidate this further by having functions like these
in the geni SE driver:

int geni_icc_get(struct geni_se *se, const char *icc_core, const char *icc_cpu,
	const char *icc_ddr)
{
	if (icc_core) {
		se->icc_path_geni_to_core = devm_of_icc_get(se->dev, icc_core);
		if (IS_ERR(se->icc_path_geni_to_core))
			return PTR_ERR(se->icc_path_geni_to_core);
	}

	if (icc_cpu) {

	...
}

int geni_icc_vote_on(struct geni_se *se)
{
	if (gi2c->se.icc_path_geni_to_core) {
		ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
			gi2c->se.peak_bw_core);
		if (ret) {
			dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for core\n",
				__func__);
			return ret;
		}
	}

	/* or to reduce nesting:
	if (!gi2c->se.icc_path_geni_to_core)
		goto vote_cpu;

	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
			gi2c->se.peak_bw_core);
	if (ret) {
	...

vote_cpu:
	*/

	if (gi2c->se.icc_path_cpu_to_geni) {

	...
}

int geni_icc_vote_off(struct geni_se *se)
{
	if (gi2c->se.icc_path_geni_to_core) {
		ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
		if (ret) {
			dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for core\n",
				__func__);
			return ret;
		}
	}

	if (gi2c->se.icc_path_cpu_to_geni) {

	...
}

optionally you could even reduce the code further by having an array of
'struct geni_icc_path' (as suggested on https://patchwork.kernel.org/patch/11436889/#23221925)
and iterate over the array instead of spelling everything out for the 3
ICC paths.

> +
>  	return 0;
>  }
>  
> @@ -637,6 +722,31 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>  	int ret;
>  	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>  
> +	/* Vote on Core, CPU and DDR path respectively */
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
> +		gi2c->se.peak_bw_core);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for core\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, gi2c->se.avg_bw_cpu,
> +		gi2c->se.peak_bw_cpu);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_ddr, gi2c->se.avg_bw_ddr,
> +		gi2c->se.peak_bw_ddr);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for ddr\n",
> +			__func__);
> +		return ret;
> +	}
> +

as per above this would be just:

	ret = geni_icc_vote_on(&gi2c->se);
	if (ret)
		return ret;

with the same benefit for the other 3 drivers.

Re: [PATCH V2 5/8] i2c: i2c-qcom-geni: Add interconnect support

[Akash Asthana]

Hi Matthias,

On 3/14/2020 5:47 AM, Matthias Kaehlcke wrote:
> Hi,
>
> On Fri, Mar 13, 2020 at 06:42:11PM +0530, Akash Asthana wrote:
>> Get the interconnect paths for I2C based Serial Engine device
>> and vote according to the bus speed of the driver.
>>
>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>> Changes in V2:
>>   - As per Bjorn's comment, removed se == NULL check from geni_i2c_icc_get
>>   - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
>>   - 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/i2c/busses/i2c-qcom-geni.c | 110 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 110 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 17abf60c..33ab685 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -163,6 +163,23 @@ static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
>>   	writel_relaxed(val, gi2c->se.base + SE_I2C_SCL_COUNTERS);
>>   }
>>   
>> +static int geni_i2c_icc_get(struct geni_se *se)
>> +{
>> +	se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core");
>> +	if (IS_ERR(se->icc_path_geni_to_core))
>> +		return PTR_ERR(se->icc_path_geni_to_core);
>> +
>> +	se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config");
>> +	if (IS_ERR(se->icc_path_cpu_to_geni))
>> +		return PTR_ERR(se->icc_path_cpu_to_geni);
>> +
>> +	se->icc_path_geni_to_ddr = devm_of_icc_get(se->dev, "qup-memory");
>> +	if (IS_ERR(se->icc_path_geni_to_ddr))
>> +		return PTR_ERR(se->icc_path_geni_to_ddr);
>> +
>> +	return 0;
>> +}
>> +
>>   static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
>>   {
>>   	u32 m_cmd = readl_relaxed(gi2c->se.base + SE_GENI_M_CMD0);
>> @@ -563,6 +580,39 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   	gi2c->adap.dev.of_node = pdev->dev.of_node;
>>   	strlcpy(gi2c->adap.name, "Geni-I2C", sizeof(gi2c->adap.name));
>>   
>> +	ret = geni_i2c_icc_get(&gi2c->se);
>> +	if (ret)
>> +		return ret;
>> +	/*
>> +	 * Set the bus quota for core and cpu to a reasonable value for
>> +	 * register access.
>> +	 * Set quota for DDR based on bus speed, assume peak requirement
>> +	 * as twice of avg bw.
>> +	 */
>> +	gi2c->se.avg_bw_core = Bps_to_icc(1000);
> as commented on the UART patch, you might want to consider adding a constant
> for this recurring default value.
>
>> +	gi2c->se.peak_bw_core = Bps_to_icc(CORE_2X_100_MHZ);
>> +	gi2c->se.avg_bw_cpu = Bps_to_icc(1000);
>> +	gi2c->se.peak_bw_cpu = Bps_to_icc(1000);
>> +	gi2c->se.avg_bw_ddr = Bps_to_icc(gi2c->clk_freq_out);
>> +	gi2c->se.peak_bw_ddr = Bps_to_icc(2 * gi2c->clk_freq_out);
>> +
>> +	/* Vote for core clocks and CPU for register access */
>> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
>> +				gi2c->se.peak_bw_core);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, gi2c->se.avg_bw_cpu,
>> +				gi2c->se.peak_bw_cpu);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "%s: ICC BW voting failed for cpu\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>>   	ret = geni_se_resources_on(&gi2c->se);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "Error turning on resources %d\n", ret);
>> @@ -584,6 +634,19 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   		dev_err(&pdev->dev, "Error turning off resources %d\n", ret);
>>   		return ret;
>>   	}
>> +	/* Remove vote from core clocks and CPU */
>> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "%s: ICC BW remove failed for core\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, 0, 0);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "%s: ICC BW remove failed for cpu\n",
>> +			__func__);
> Should this return an error as for the core clocks?
>
>> +	}
>>   
>>   	dev_dbg(&pdev->dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
>>   
>> @@ -629,6 +692,28 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>>   		gi2c->suspended = 1;
>>   	}
>>   
>> +	/* Remove BW votes */
>> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
>> +	if (ret) {
>> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for core\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, 0, 0);
>> +	if (ret) {
>> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for cpu\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_ddr, 0, 0);
>> +	if (ret) {
>> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for ddr\n",
>> +			__func__);
>> +		return ret;
>> +	}
> you could consider putting this and the ICC voting in _runtime_resume() in
> separate functions and call them from here and _probe(). The only difference
> in _probe() is that it doesn't vote for the DDR bandwidth, but that slight
> one-time overhead might be worth for consolidating the code.
>
> Actually all this looks very similar for UART, I2C, SPI and QSPI. It seems it
> should be possible to consolidate this further by having functions like these
> in the geni SE driver:
>
> int geni_icc_get(struct geni_se *se, const char *icc_core, const char *icc_cpu,
> 	const char *icc_ddr)
> {
> 	if (icc_core) {
> 		se->icc_path_geni_to_core = devm_of_icc_get(se->dev, icc_core);
> 		if (IS_ERR(se->icc_path_geni_to_core))
> 			return PTR_ERR(se->icc_path_geni_to_core);
> 	}
>
> 	if (icc_cpu) {
>
> 	...
> }
>
> int geni_icc_vote_on(struct geni_se *se)
> {
> 	if (gi2c->se.icc_path_geni_to_core) {
> 		ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
> 			gi2c->se.peak_bw_core);
> 		if (ret) {
> 			dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for core\n",
> 				__func__);
> 			return ret;
> 		}
> 	}
>
> 	/* or to reduce nesting:
> 	if (!gi2c->se.icc_path_geni_to_core)
> 		goto vote_cpu;
>
> 	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
> 			gi2c->se.peak_bw_core);
> 	if (ret) {
> 	...
>
> vote_cpu:
> 	*/
>
> 	if (gi2c->se.icc_path_cpu_to_geni) {
>
> 	...
> }
>
> int geni_icc_vote_off(struct geni_se *se)
> {
> 	if (gi2c->se.icc_path_geni_to_core) {
> 		ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
> 		if (ret) {
> 			dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for core\n",
> 				__func__);
> 			return ret;
> 		}
> 	}
>
> 	if (gi2c->se.icc_path_cpu_to_geni) {
>
> 	...
> }
>
> optionally you could even reduce the code further by having an array of
> 'struct geni_icc_path' (as suggested on https://patchwork.kernel.org/patch/11436889/#23221925)
> and iterate over the array instead of spelling everything out for the 3
> ICC paths.
ok
>> +
>>   	return 0;
>>   }
>>   
>> @@ -637,6 +722,31 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>>   	int ret;
>>   	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>   
>> +	/* Vote on Core, CPU and DDR path respectively */
>> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
>> +		gi2c->se.peak_bw_core);
>> +	if (ret) {
>> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for core\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, gi2c->se.avg_bw_cpu,
>> +		gi2c->se.peak_bw_cpu);
>> +	if (ret) {
>> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for cpu\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_ddr, gi2c->se.avg_bw_ddr,
>> +		gi2c->se.peak_bw_ddr);
>> +	if (ret) {
>> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for ddr\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
> as per above this would be just:
>
> 	ret = geni_icc_vote_on(&gi2c->se);
> 	if (ret)
> 		return ret;
>
> with the same benefit for the other 3 drivers.

Yeah this would be better.

Thanks for reviewing.

regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

[PATCH V2 6/8] spi: spi-geni-qcom: Add interconnect support

[Akash Asthana]

Get the interconnect paths for SPI based Serial Engine device
and vote according to the current bus speed of the driver.

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
---
 - As per Bjorn's comment, removed se == NULL check from geni_spi_icc_get
 - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
 - 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-geni-qcom.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 73 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index c397242..09c4709 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -118,6 +118,19 @@ static int get_spi_clk_cfg(unsigned int speed_hz,
 	return ret;
 }
 
+static int geni_spi_icc_get(struct geni_se *se)
+{
+	se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core");
+	if (IS_ERR(se->icc_path_geni_to_core))
+		return PTR_ERR(se->icc_path_geni_to_core);
+
+	se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config");
+	if (IS_ERR(se->icc_path_cpu_to_geni))
+		return PTR_ERR(se->icc_path_cpu_to_geni);
+
+	return 0;
+}
+
 static void handle_fifo_timeout(struct spi_master *spi,
 				struct spi_message *msg)
 {
@@ -234,6 +247,20 @@ static int setup_fifo_params(struct spi_device *spi_slv,
 		return ret;
 	}
 
+	/*
+	 * Set BW quota for CPU as driver supports FIFO mode only.
+	 * Assume peak bw as twice of avg bw.
+	 */
+	se->avg_bw_cpu = Bps_to_icc(mas->cur_speed_hz);
+	se->peak_bw_cpu = Bps_to_icc(2 * mas->cur_speed_hz);
+	ret = icc_set_bw(se->icc_path_cpu_to_geni, se->avg_bw_cpu,
+			se->peak_bw_cpu);
+	if (ret) {
+		dev_err(mas->dev, "%s: ICC BW voting failed for cpu\n",
+			__func__);
+		return ret;
+	}
+
 	clk_sel = idx & CLK_SEL_MSK;
 	m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN;
 	spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word);
@@ -578,6 +605,15 @@ static int spi_geni_probe(struct platform_device *pdev)
 	spin_lock_init(&mas->lock);
 	pm_runtime_enable(dev);
 
+	ret = geni_spi_icc_get(&mas->se);
+	if (ret)
+		goto spi_geni_probe_runtime_disable;
+	/* Set the bus quota to a reasonable value for register access */
+	mas->se.avg_bw_core = Bps_to_icc(CORE_2X_50_MHZ);
+	mas->se.peak_bw_core = Bps_to_icc(CORE_2X_100_MHZ);
+	mas->se.avg_bw_cpu = Bps_to_icc(1000);
+	mas->se.peak_bw_cpu = Bps_to_icc(1000);
+
 	ret = spi_geni_init(mas);
 	if (ret)
 		goto spi_geni_probe_runtime_disable;
@@ -616,14 +652,50 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
 {
 	struct spi_master *spi = dev_get_drvdata(dev);
 	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+	int ret;
+
+	ret = geni_se_resources_off(&mas->se);
+	if (ret)
+		return ret;
 
-	return geni_se_resources_off(&mas->se);
+	ret = icc_set_bw(mas->se.icc_path_geni_to_core, 0, 0);
+	if (ret) {
+		dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for core\n",
+			__func__);
+		return ret;
+	}
+
+	ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, 0, 0);
+	if (ret) {
+		dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for cpu\n",
+			__func__);
+		return ret;
+	}
+
+	return 0;
 }
 
 static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
 {
 	struct spi_master *spi = dev_get_drvdata(dev);
 	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+	int ret;
+
+	ret = icc_set_bw(mas->se.icc_path_geni_to_core, mas->se.avg_bw_core,
+		mas->se.peak_bw_core);
+	if (ret) {
+		dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for core\n",
+			__func__);
+		return ret;
+	}
+
+	ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, mas->se.avg_bw_cpu,
+		mas->se.peak_bw_cpu);
+	if (ret) {
+		dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for cpu\n",
+			__func__);
+		return ret;
+	}
 
 	return geni_se_resources_on(&mas->se);
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 6/8] spi: spi-geni-qcom: Add interconnect support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

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

> +	se->avg_bw_cpu = Bps_to_icc(mas->cur_speed_hz);
> +	se->peak_bw_cpu = Bps_to_icc(2 * mas->cur_speed_hz);

As I commented on the previous version to no reply there seem to be a
lot of cases where the peak bandwidth is just set to double the normal
bandwidth without obvious analysis.  Should this default be centralized?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH V2 6/8] spi: spi-geni-qcom: Add interconnect support

[Akash Asthana]

On 3/13/2020 6:46 PM, Mark Brown wrote:
> On Fri, Mar 13, 2020 at 06:42:12PM +0530, Akash Asthana wrote:
>
>> +	se->avg_bw_cpu = Bps_to_icc(mas->cur_speed_hz);
>> +	se->peak_bw_cpu = Bps_to_icc(2 * mas->cur_speed_hz);
> As I commented on the previous version to no reply there seem to be a
> lot of cases where the peak bandwidth is just set to double the normal
> bandwidth without obvious analysis.  Should this default be centralized?

Hi Mark,

I misunderstood previous comment on V1 
patch@https://patchwork.kernel.org/patch/11386479/ as a suggestion to 
mention comment for peak BW choice if nothing mentioned explicitly (in 
this case I mentioned assume peak BW twice as average).

I understand in any case I should have ack'ed the comment atleast by 
replying "ok". I am sorry about it.


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. Factor of 2 is chosen randomly. 
Please correct/improve me if this is not okay.

If this is okay, I will centralize this design for SPI QUP, I2C and UART 
driver.

Regards,

Akash



-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 6/8] spi: spi-geni-qcom: Add interconnect support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 718 bytes --]

On Tue, Mar 17, 2020 at 03:05:21PM +0530, Akash Asthana wrote:

> 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. Factor of 2 is chosen randomly. Please
> correct/improve me if this is not okay.

> If this is okay, I will centralize this design for SPI QUP, I2C and UART
> driver.

That seems reasonable to me, it was just the fact that every driver
seemed to be doing the same thing that I was noticing - what was being
done seemed OK.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH V2 6/8] spi: spi-geni-qcom: Add interconnect support

[Akash Asthana]

Hi Mark,

On 3/17/2020 6:36 PM, Mark Brown wrote:
> On Tue, Mar 17, 2020 at 03:05:21PM +0530, Akash Asthana wrote:
>
>> 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. Factor of 2 is chosen randomly. Please
>> correct/improve me if this is not okay.
>> If this is okay, I will centralize this design for SPI QUP, I2C and UART
>> driver.
> That seems reasonable to me, it was just the fact that every driver
> seemed to be doing the same thing that I was noticing - what was being
> done seemed OK.

Okay, thanks for confirming I will keep as is.

Regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 6/8] spi: spi-geni-qcom: Add interconnect support

[Matthias Kaehlcke]

Hi Akash,

On Fri, Mar 13, 2020 at 06:42:12PM +0530, Akash Asthana wrote:
> Get the interconnect paths for SPI based Serial Engine device
> and vote according to the current bus speed of the driver.
> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
>  - As per Bjorn's comment, removed se == NULL check from geni_spi_icc_get
>  - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
>  - 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-geni-qcom.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index c397242..09c4709 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -118,6 +118,19 @@ static int get_spi_clk_cfg(unsigned int speed_hz,
>  	return ret;
>  }
>  
> +static int geni_spi_icc_get(struct geni_se *se)
> +{
> +	se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core");
> +	if (IS_ERR(se->icc_path_geni_to_core))
> +		return PTR_ERR(se->icc_path_geni_to_core);
> +
> +	se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config");
> +	if (IS_ERR(se->icc_path_cpu_to_geni))
> +		return PTR_ERR(se->icc_path_cpu_to_geni);
> +
> +	return 0;
> +}

As per my comments on (https://patchwork.kernel.org/patch/11436895/#23222713),
the above function could be replaced by calling a 'geni_icc_get()' (or so, to
be created) provided by the geni SE driver.

> +
>  static void handle_fifo_timeout(struct spi_master *spi,
>  				struct spi_message *msg)
>  {
> @@ -234,6 +247,20 @@ static int setup_fifo_params(struct spi_device *spi_slv,
>  		return ret;
>  	}
>  
> +	/*
> +	 * Set BW quota for CPU as driver supports FIFO mode only.
> +	 * Assume peak bw as twice of avg bw.
> +	 */
> +	se->avg_bw_cpu = Bps_to_icc(mas->cur_speed_hz);
> +	se->peak_bw_cpu = Bps_to_icc(2 * mas->cur_speed_hz);
> +	ret = icc_set_bw(se->icc_path_cpu_to_geni, se->avg_bw_cpu,
> +			se->peak_bw_cpu);
> +	if (ret) {
> +		dev_err(mas->dev, "%s: ICC BW voting failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}
> +
>  	clk_sel = idx & CLK_SEL_MSK;
>  	m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN;
>  	spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word);
> @@ -578,6 +605,15 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	spin_lock_init(&mas->lock);
>  	pm_runtime_enable(dev);
>  
> +	ret = geni_spi_icc_get(&mas->se);
> +	if (ret)
> +		goto spi_geni_probe_runtime_disable;
> +	/* Set the bus quota to a reasonable value for register access */
> +	mas->se.avg_bw_core = Bps_to_icc(CORE_2X_50_MHZ);
> +	mas->se.peak_bw_core = Bps_to_icc(CORE_2X_100_MHZ);
> +	mas->se.avg_bw_cpu = Bps_to_icc(1000);
> +	mas->se.peak_bw_cpu = Bps_to_icc(1000);
> +
>  	ret = spi_geni_init(mas);
>  	if (ret)
>  		goto spi_geni_probe_runtime_disable;
> @@ -616,14 +652,50 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
>  {
>  	struct spi_master *spi = dev_get_drvdata(dev);
>  	struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +	int ret;
> +
> +	ret = geni_se_resources_off(&mas->se);
> +	if (ret)
> +		return ret;
>  
> -	return geni_se_resources_off(&mas->se);
> +	ret = icc_set_bw(mas->se.icc_path_geni_to_core, 0, 0);
> +	if (ret) {
> +		dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for core\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, 0, 0);
> +	if (ret) {
> +		dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}

the ICC stuff above would become:

	ret = geni_icc_vote_off(&mas->se);
	if (ret)
		return ret;

with the consolidated code in geni SE.

> +
> +	return 0;
>  }
>  
>  static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
>  {
>  	struct spi_master *spi = dev_get_drvdata(dev);
>  	struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +	int ret;
> +
> +	ret = icc_set_bw(mas->se.icc_path_geni_to_core, mas->se.avg_bw_core,
> +		mas->se.peak_bw_core);
> +	if (ret) {
> +		dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for core\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, mas->se.avg_bw_cpu,
> +		mas->se.peak_bw_cpu);
> +	if (ret) {
> +		dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}

and this:

	ret = geni_icc_vote_on(&mas->se);
	if (ret)
		return ret;

>  	return geni_se_resources_on(&mas->se);

possibly you could even do the ICC voting from geni_se_resources_on/off()
it seems the two are always done together for UART, I2C and SPI.

Re: [PATCH V2 6/8] spi: spi-geni-qcom: Add interconnect support

[Akash Asthana]

Hi Matthias,

On 3/14/2020 6:11 AM, Matthias Kaehlcke wrote:
> Hi Akash,
>
> On Fri, Mar 13, 2020 at 06:42:12PM +0530, Akash Asthana wrote:
>> Get the interconnect paths for SPI based Serial Engine device
>> and vote according to the current bus speed of the driver.
>>
>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>> ---
>>   - As per Bjorn's comment, removed se == NULL check from geni_spi_icc_get
>>   - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
>>   - 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-geni-qcom.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 73 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>> index c397242..09c4709 100644
>> --- a/drivers/spi/spi-geni-qcom.c
>> +++ b/drivers/spi/spi-geni-qcom.c
>> @@ -118,6 +118,19 @@ static int get_spi_clk_cfg(unsigned int speed_hz,
>>   	return ret;
>>   }
>>   
>> +static int geni_spi_icc_get(struct geni_se *se)
>> +{
>> +	se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core");
>> +	if (IS_ERR(se->icc_path_geni_to_core))
>> +		return PTR_ERR(se->icc_path_geni_to_core);
>> +
>> +	se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config");
>> +	if (IS_ERR(se->icc_path_cpu_to_geni))
>> +		return PTR_ERR(se->icc_path_cpu_to_geni);
>> +
>> +	return 0;
>> +}
> As per my comments on (https://patchwork.kernel.org/patch/11436895/#23222713),
> the above function could be replaced by calling a 'geni_icc_get()' (or so, to
> be created) provided by the geni SE driver.
ok
>
>> +
>>   static void handle_fifo_timeout(struct spi_master *spi,
>>   				struct spi_message *msg)
>>   {
>> @@ -234,6 +247,20 @@ static int setup_fifo_params(struct spi_device *spi_slv,
>>   		return ret;
>>   	}
>>   
>> +	/*
>> +	 * Set BW quota for CPU as driver supports FIFO mode only.
>> +	 * Assume peak bw as twice of avg bw.
>> +	 */
>> +	se->avg_bw_cpu = Bps_to_icc(mas->cur_speed_hz);
>> +	se->peak_bw_cpu = Bps_to_icc(2 * mas->cur_speed_hz);
>> +	ret = icc_set_bw(se->icc_path_cpu_to_geni, se->avg_bw_cpu,
>> +			se->peak_bw_cpu);
>> +	if (ret) {
>> +		dev_err(mas->dev, "%s: ICC BW voting failed for cpu\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>>   	clk_sel = idx & CLK_SEL_MSK;
>>   	m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN;
>>   	spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word);
>> @@ -578,6 +605,15 @@ static int spi_geni_probe(struct platform_device *pdev)
>>   	spin_lock_init(&mas->lock);
>>   	pm_runtime_enable(dev);
>>   
>> +	ret = geni_spi_icc_get(&mas->se);
>> +	if (ret)
>> +		goto spi_geni_probe_runtime_disable;
>> +	/* Set the bus quota to a reasonable value for register access */
>> +	mas->se.avg_bw_core = Bps_to_icc(CORE_2X_50_MHZ);
>> +	mas->se.peak_bw_core = Bps_to_icc(CORE_2X_100_MHZ);
>> +	mas->se.avg_bw_cpu = Bps_to_icc(1000);
>> +	mas->se.peak_bw_cpu = Bps_to_icc(1000);
>> +
>>   	ret = spi_geni_init(mas);
>>   	if (ret)
>>   		goto spi_geni_probe_runtime_disable;
>> @@ -616,14 +652,50 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev)
>>   {
>>   	struct spi_master *spi = dev_get_drvdata(dev);
>>   	struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +	int ret;
>> +
>> +	ret = geni_se_resources_off(&mas->se);
>> +	if (ret)
>> +		return ret;
>>   
>> -	return geni_se_resources_off(&mas->se);
>> +	ret = icc_set_bw(mas->se.icc_path_geni_to_core, 0, 0);
>> +	if (ret) {
>> +		dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for core\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>> +	ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, 0, 0);
>> +	if (ret) {
>> +		dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for cpu\n",
>> +			__func__);
>> +		return ret;
>> +	}
> the ICC stuff above would become:
>
> 	ret = geni_icc_vote_off(&mas->se);
> 	if (ret)
> 		return ret;
>
> with the consolidated code in geni SE.
ok
>
>> +
>> +	return 0;
>>   }
>>   
>>   static int __maybe_unused spi_geni_runtime_resume(struct device *dev)
>>   {
>>   	struct spi_master *spi = dev_get_drvdata(dev);
>>   	struct spi_geni_master *mas = spi_master_get_devdata(spi);
>> +	int ret;
>> +
>> +	ret = icc_set_bw(mas->se.icc_path_geni_to_core, mas->se.avg_bw_core,
>> +		mas->se.peak_bw_core);
>> +	if (ret) {
>> +		dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for core\n",
>> +			__func__);
>> +		return ret;
>> +	}
>> +
>> +	ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, mas->se.avg_bw_cpu,
>> +		mas->se.peak_bw_cpu);
>> +	if (ret) {
>> +		dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for cpu\n",
>> +			__func__);
>> +		return ret;
>> +	}
> and this:
>
> 	ret = geni_icc_vote_on(&mas->se);
> 	if (ret)
> 		return ret;
ok
>>   	return geni_se_resources_on(&mas->se);
> possibly you could even do the ICC voting from geni_se_resources_on/off()
> it seems the two are always done together for UART, I2C and SPI.

I think we should expose geni_icc_vote_on/off API seperately and not 
merge to resources_on/off.

Because if we merge then it will appear that we are just doing 
geni_icc_get() from individual SE driver probe not using any of ICC apis.

It looks somewhat asymmetry.


Thanks for reviewing,

Regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

[PATCH V2 7/8] spi: spi-qcom-qspi: Add interconnect support

[Akash Asthana]

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>
---
 - 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;
+	/* 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);
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 7/8] spi: spi-qcom-qspi: Add interconnect support

[Matthias Kaehlcke]

Hi,

On Fri, Mar 13, 2020 at 06:42:13PM +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>
> ---
>  - 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.

Re: [PATCH V2 7/8] spi: spi-qcom-qspi: Add interconnect support

[Akash Asthana]

Hi Matthias,

On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote:
> Hi,
>
> On Fri, Mar 13, 2020 at 06:42:13PM +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>
>> ---
>>   - 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.

Regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 7/8] spi: spi-qcom-qspi: Add interconnect support

[Evan Green]

On Tue, Mar 17, 2020 at 5:13 AM Akash Asthana <akashast@codeaurora.org> wrote:
>
> Hi Matthias,
>
> On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote:
> > Hi,
> >
> > On Fri, Mar 13, 2020 at 06:42:13PM +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>
> >> ---
> >>   - 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. In the GENI case here, we only really
need them to undo the 0 votes we cast during suspend. If "vote for 0
in suspend and whatever it was before at resume" is a recurring theme,
maybe the core should give us path_disable() and path_enable() calls
instead. I'm thinking out loud, maybe Georgi has some thoughts.

Akash, for now if you want to avoid wading into a larger discussion
maybe just refactor to a common structure local to GENI.


-Evan

Re: [PATCH V2 7/8] spi: spi-qcom-qspi: Add interconnect support

[Akash Asthana]

Hi Evan,

On 3/18/2020 12:38 AM, Evan Green wrote:
> On Tue, Mar 17, 2020 at 5:13 AM Akash Asthana <akashast@codeaurora.org> wrote:
>> Hi Matthias,
>>
>> On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote:
>>> Hi,
>>>
>>> On Fri, Mar 13, 2020 at 06:42:13PM +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>
>>>> ---
>>>>    - 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?
>   In the GENI case here, we only really
> need them to undo the 0 votes we cast during suspend. If "vote for 0
> in suspend and whatever it was before at resume" is a recurring theme,
> maybe the core should give us path_disable() and path_enable() calls
> instead. I'm thinking out loud, maybe Georgi has some thoughts.
>
> Akash, for now if you want to avoid wading into a larger discussion
> maybe just refactor to a common structure local to GENI.

Ok

Thanks,

Akash

>
>
> -Evan

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH V2 7/8] spi: spi-qcom-qspi: Add interconnect support

[Evan Green]

On Wed, Mar 18, 2020 at 6:48 AM Akash Asthana <akashast@codeaurora.org> wrote:
>
> Hi Evan,
>
> On 3/18/2020 12:38 AM, Evan Green wrote:
> > On Tue, Mar 17, 2020 at 5:13 AM Akash Asthana <akashast@codeaurora.org> wrote:
> >> Hi Matthias,
> >>
> >> On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote:
> >>> Hi,
> >>>
> >>> On Fri, Mar 13, 2020 at 06:42:13PM +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>
> >>>> ---
> >>>>    - 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

Re: [PATCH V2 7/8] spi: spi-qcom-qspi: Add interconnect support

[Akash Asthana]

Hi Evan,
>> 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

Thanks for clarification! Yeah make sense not to introduce the structure 
in ICC framework

Regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

[PATCH V2 8/8] arm64: dts: sc7180: Add interconnect for QUP and QSPI

[Akash Asthana]

Add interconnect ports for GENI QUPs and QSPI to set bus capabilities.

Signed-off-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in V2:
 - As per Bjorn's comment, ignoring 80 char limit in defining interconnects
   paths.

 arch/arm64/boot/dts/qcom/sc7180.dtsi | 127 +++++++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 89ba012..5c0185c 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -427,6 +427,8 @@
 			#size-cells = <2>;
 			ranges;
 			iommus = <&apps_smmu 0x43 0x0>;
+			interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>;
+			interconnect-names = "qup-core";
 			status = "disabled";
 
 			i2c0: i2c@880000 {
@@ -439,6 +441,11 @@
 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>,
+						<&aggre1_noc MASTER_QUP_0 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -452,6 +459,9 @@
 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -463,6 +473,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart0_default>;
 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -476,6 +489,11 @@
 				interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>,
+						<&aggre1_noc MASTER_QUP_0 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -489,6 +507,9 @@
 				interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -500,6 +521,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart1_default>;
 				interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -513,6 +537,11 @@
 				interrupts = <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>,
+						<&aggre1_noc MASTER_QUP_0 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -524,6 +553,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart2_default>;
 				interrupts = <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -537,6 +569,11 @@
 				interrupts = <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>,
+						<&aggre1_noc MASTER_QUP_0 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -550,6 +587,9 @@
 				interrupts = <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -561,6 +601,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart3_default>;
 				interrupts = <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -574,6 +617,11 @@
 				interrupts = <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>,
+						<&aggre1_noc MASTER_QUP_0 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -585,6 +633,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart4_default>;
 				interrupts = <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -598,6 +649,11 @@
 				interrupts = <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>,
+						<&aggre1_noc MASTER_QUP_0 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -611,6 +667,9 @@
 				interrupts = <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -622,6 +681,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart5_default>;
 				interrupts = <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_0 &qup_virt SLAVE_QUP_CORE_0>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_0>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 		};
@@ -636,6 +698,8 @@
 			#size-cells = <2>;
 			ranges;
 			iommus = <&apps_smmu 0x4c3 0x0>;
+			interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>;
+			interconnect-names = "qup-core";
 			status = "disabled";
 
 			i2c6: i2c@a80000 {
@@ -648,6 +712,11 @@
 				interrupts = <GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>,
+						<&aggre2_noc MASTER_QUP_1 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -661,6 +730,9 @@
 				interrupts = <GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -672,6 +744,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart6_default>;
 				interrupts = <GIC_SPI 353 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -685,6 +760,11 @@
 				interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>,
+						<&aggre2_noc MASTER_QUP_1 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -696,6 +776,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart7_default>;
 				interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -709,6 +792,11 @@
 				interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>,
+						<&aggre2_noc MASTER_QUP_1 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -722,6 +810,9 @@
 				interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -733,6 +824,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart8_default>;
 				interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -746,6 +840,11 @@
 				interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>,
+						<&aggre2_noc MASTER_QUP_1 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -757,6 +856,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart9_default>;
 				interrupts = <GIC_SPI 356 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -770,6 +872,11 @@
 				interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>,
+						<&aggre2_noc MASTER_QUP_1 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -783,6 +890,9 @@
 				interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -794,6 +904,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart10_default>;
 				interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -807,6 +920,11 @@
 				interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>,
+						<&aggre2_noc MASTER_QUP_1 &mc_virt SLAVE_EBI1>;
+				interconnect-names = "qup-core", "qup-config",
+							"qup-memory";
 				status = "disabled";
 			};
 
@@ -820,6 +938,9 @@
 				interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>;
 				#address-cells = <1>;
 				#size-cells = <0>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 
@@ -831,6 +952,9 @@
 				pinctrl-names = "default";
 				pinctrl-0 = <&qup_uart11_default>;
 				interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>;
+				interconnects = <&qup_virt MASTER_QUP_CORE_1 &qup_virt SLAVE_QUP_CORE_1>,
+						<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_QUP_1>;
+				interconnect-names = "qup-core", "qup-config";
 				status = "disabled";
 			};
 		};
@@ -1336,6 +1460,9 @@
 			clocks = <&gcc GCC_QSPI_CNOC_PERIPH_AHB_CLK>,
 				 <&gcc GCC_QSPI_CORE_CLK>;
 			clock-names = "iface", "core";
+			interconnects = <&gem_noc MASTER_APPSS_PROC
+					&config_noc SLAVE_QSPI_0>;
+			interconnect-names = "qspi-config";
 			status = "disabled";
 		};
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project