Re: [PATCH V6 1/7] soc: qcom: geni: Support for ICC voting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Matthias,

On 5/21/2020 9:20 PM, Matthias Kaehlcke wrote:
Hi Akash,

On Thu, May 21, 2020 at 01:59:18PM +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@xxxxxxxxxxxxxx>
Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
---
Changes in V2:
  - As per Bjorn's comment dropped enums for ICC paths, given the three
    paths individual members

Changes in V3:
  - Add geni_icc_get, geni_icc_vote_on and geni_icc_vote_off as helper API.
  - Add geni_icc_path structure in common header

Changes in V4:
  - As per Bjorn's comment print error message in geni_icc_get if return
    value is not -EPROBE_DEFER.
  - As per Bjorn's comment remove NULL on path before calling icc_set_bw
    API.
  - As per Bjorn's comment drop __func__ print.
  - As per Matthias's comment, make ICC path a array instead of individual
    member entry in geni_se struct.

Changes in V5:
  - As per Matthias's comment defined enums for ICC paths.
  - Integrate icc_enable/disable with power on/off call for driver.
  - As per Matthias's comment added icc_path_names array to print icc path name
    in failure case.
  - As per Georgi's suggestion assume peak_bw = avg_bw if not mentioned.

Changes in V6:
  - Addressed nitpicks from Matthias.

Note: I have ignored below check patch suggestion because it was throwing
       compilation error as 'icc_ddr' is not compile time comstant.

WARNING: char * array declaration might be better as static const
  - FILE: drivers/soc/qcom/qcom-geni-se.c:726:
  - const char *icc_names[] = {"qup-core", "qup-config", icc_ddr};

  drivers/soc/qcom/qcom-geni-se.c | 92 +++++++++++++++++++++++++++++++++++++++++
  include/linux/qcom-geni-se.h    | 42 +++++++++++++++++++
  2 files changed, 134 insertions(+)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 7d622ea..0b2526d 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -92,6 +92,9 @@ struct geni_wrapper {
  	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
  };
+static const char * const icc_path_names[] = {"qup-core", "qup-config",
+						"qup-memory"};
+
  #define QUP_HW_VER_REG			0x4
/* Common SE registers */
@@ -720,6 +723,95 @@ void geni_se_rx_dma_unprep(struct geni_se *se, dma_addr_t iova, size_t len)
  }
  EXPORT_SYMBOL(geni_se_rx_dma_unprep);
+int geni_icc_get(struct geni_se *se, const char *icc_ddr)
+{
+	int i, err;
+	const char *icc_names[] = {"qup-core", "qup-config", icc_ddr};
+
+	for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) {
+		if (!icc_names[i])
+			continue;
+
+		se->icc_paths[i].path = devm_of_icc_get(se->dev, icc_names[i]);
+		if (IS_ERR(se->icc_paths[i].path))
+			goto err;
+	}
+
+	return 0;
+
+err:
+	err = PTR_ERR(se->icc_paths[i].path);
+	if (err != -EPROBE_DEFER)
+		dev_err_ratelimited(se->dev, "Failed to get ICC path:%s :%d\n",
That's still an odd format, especially the colon before the error code. My
suggestion was "... path 'qup-core': 42" i.e. "... path '%s': %d".
Sorry about it, I will correct this everywhere.
I don't want to stall the series on nits though, if there is no need for
a respin for other reasons this can be also fixed with a patch after this
has landed.

I'm still not overly convinced about having two bandwidth values for what
might happen in the future (or not). Typically unused functions or struct
members that are added just in case tend to be rejected, since they can be
added when the need actually arises. Anyway, as long as maintainers are
happy with it I won't object.

Okay, I am removing peak_bw variable and tracking BW request just with avg_bw, if need I will add this back in future.

Thanks for reviewing

regards,

Akash



Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>




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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux