| Message ID | 1588919619-21355-2-git-send-email-akashast@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add interconnect support to QSPI and QUP drivers | expand |
Hi Akash, note: my comments below are clearly entering bikeshed territory. Please take what you agree with and feel free to ignore the rest. On Fri, May 08, 2020 at 12:03:33PM +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 > > 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. > > 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}; > > 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. > > 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..63403bf 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"}; nit: the indentation is a bit odd. I would align it either with "qup-core" or at a tab stop nearby. > + > #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, icc_err; nit: the 'icc_' prefix doesn't add value here, just 'err' would be less 'noisy' IMO. > + const char *icc_names[] = {"qup-core", "qup-config", icc_ddr}; nit: you could avoid repeating the first to strings by referencing icc_path_names[GENI_TO_CORE] and icc_path_names[CPU_TO_GENI]. Not sure if it's really better, it avoids the redundant names, but is slightly less readable. > + > + 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 icc_get_failure; nit: since there is only a single label it isn't really necessary to be so precise. 'goto err' is very common in the kernel, 'err_icc_get' would be another alternative. > + } > + > + return 0; > + > +icc_get_failure: > + icc_err = PTR_ERR(se->icc_paths[i].path); > + if (icc_err != -EPROBE_DEFER) > + dev_err_ratelimited(se->dev, "Failed to get ICC path:%s, ret:%d\n", All the logs in this patch result in something like "... path:qup-core, ret:42". For humans I think it is more intuitive to parse "... path 'qup-core': 42". Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
On Fri, May 08, 2020 at 12:03:33PM +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 > > 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. > > 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}; > > 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. > > 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..63403bf 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, icc_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 icc_get_failure; > + } > + > + return 0; > + > +icc_get_failure: > + icc_err = PTR_ERR(se->icc_paths[i].path); > + if (icc_err != -EPROBE_DEFER) > + dev_err_ratelimited(se->dev, "Failed to get ICC path:%s, ret:%d\n", > + icc_names[i], icc_err); > + return icc_err; > + > +} > +EXPORT_SYMBOL(geni_icc_get); > + > +void geni_icc_bw_init(struct geni_icc_path *icc_paths, unsigned int avg_bw, > + unsigned int peak_bw) I noticed that all callers of geni_icc_bw_init() pass 0 as 'peak_pw' (aka peak == avg). Unless there are plans to use other values for the peak bandwidth in the future it is pointless to track the bandwidth(s) in two variables. And if the bandwidth is tracked in a single variable this function is not needed. > +{ > + if (!peak_bw) > + peak_bw = avg_bw; > + icc_paths->avg_bw = avg_bw; > + icc_paths->peak_bw = peak_bw; > +} > +EXPORT_SYMBOL(geni_icc_bw_init);
On 5/8/2020 10:43 PM, Matthias Kaehlcke wrote: > Hi Akash, > > note: my comments below are clearly entering bikeshed territory. Please > take what you agree with and feel free to ignore the rest. > > On Fri, May 08, 2020 at 12:03:33PM +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 >> >> 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. >> >> 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}; >> >> 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. >> >> 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..63403bf 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"}; > nit: the indentation is a bit odd. I would align it either with "qup-core" or > at a tab stop nearby. ok > >> + >> #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, icc_err; > nit: the 'icc_' prefix doesn't add value here, just 'err' would be less > 'noisy' IMO. ok > >> + const char *icc_names[] = {"qup-core", "qup-config", icc_ddr}; > nit: you could avoid repeating the first to strings by referencing > icc_path_names[GENI_TO_CORE] and icc_path_names[CPU_TO_GENI]. Not sure > if it's really better, it avoids the redundant names, but is slightly > less readable. I thought of that but current implementation looks neater to me. > >> + >> + 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 icc_get_failure; > nit: since there is only a single label it isn't really necessary to be so > precise. 'goto err' is very common in the kernel, 'err_icc_get' would be > another alternative. okay > >> + } >> + >> + return 0; >> + >> +icc_get_failure: >> + icc_err = PTR_ERR(se->icc_paths[i].path); >> + if (icc_err != -EPROBE_DEFER) >> + dev_err_ratelimited(se->dev, "Failed to get ICC path:%s, ret:%d\n", > All the logs in this patch result in something like "... path:qup-core, ret:42". > For humans I think it is more intuitive to parse "... path 'qup-core': 42". ok Thanks for review and feedback Regards, Akash > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Hi Matthias, On 5/9/2020 12:33 AM, Matthias Kaehlcke wrote: > On Fri, May 08, 2020 at 12:03:33PM +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 >> >> 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. >> >> 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}; >> >> 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. >> >> 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..63403bf 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, icc_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 icc_get_failure; >> + } >> + >> + return 0; >> + >> +icc_get_failure: >> + icc_err = PTR_ERR(se->icc_paths[i].path); >> + if (icc_err != -EPROBE_DEFER) >> + dev_err_ratelimited(se->dev, "Failed to get ICC path:%s, ret:%d\n", >> + icc_names[i], icc_err); >> + return icc_err; >> + >> +} >> +EXPORT_SYMBOL(geni_icc_get); >> + >> +void geni_icc_bw_init(struct geni_icc_path *icc_paths, unsigned int avg_bw, >> + unsigned int peak_bw) > I noticed that all callers of geni_icc_bw_init() pass 0 as > 'peak_pw' (aka peak == avg). Unless there are plans to use other > values for the peak bandwidth in the future it is pointless to > track the bandwidth(s) in two variables. And if the bandwidth is > tracked in a single variable this function is not needed. As of now we are not aware of our peak requirement(avg requirement works good for us) so I am passing (peak = avg) as per Georgi's suggestion. But I wanted to make this feature more generic and keeping the possibilty open that we might have to use other peak(!=0) value in future. Regards, Akash > >> +{ >> + if (!peak_bw) >> + peak_bw = avg_bw; >> + icc_paths->avg_bw = avg_bw; >> + icc_paths->peak_bw = peak_bw; >> +} >> +EXPORT_SYMBOL(geni_icc_bw_init);
diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c index 7d622ea..63403bf 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, icc_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 icc_get_failure; + } + + return 0; + +icc_get_failure: + icc_err = PTR_ERR(se->icc_paths[i].path); + if (icc_err != -EPROBE_DEFER) + dev_err_ratelimited(se->dev, "Failed to get ICC path:%s, ret:%d\n", + icc_names[i], icc_err); + return icc_err; + +} +EXPORT_SYMBOL(geni_icc_get); + +void geni_icc_bw_init(struct geni_icc_path *icc_paths, unsigned int avg_bw, + unsigned int peak_bw) +{ + if (!peak_bw) + peak_bw = avg_bw; + icc_paths->avg_bw = avg_bw; + icc_paths->peak_bw = peak_bw; +} +EXPORT_SYMBOL(geni_icc_bw_init); + +int geni_icc_set_bw(struct geni_se *se) +{ + int i, ret; + + for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { + ret = icc_set_bw(se->icc_paths[i].path, + se->icc_paths[i].avg_bw, se->icc_paths[i].peak_bw); + if (ret) { + dev_err_ratelimited(se->dev, "ICC BW voting failed on path:%s, ret:%d\n", + icc_path_names[i], ret); + return ret; + } + } + + return 0; +} +EXPORT_SYMBOL(geni_icc_set_bw); + +/* To do: Replace this by icc_bulk_enable once it's implemented in ICC core */ +int geni_icc_enable(struct geni_se *se) +{ + int i, ret; + + for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { + ret = icc_enable(se->icc_paths[i].path); + if (ret) { + dev_err_ratelimited(se->dev, "ICC enable failed on path:%s, ret:%d\n", + icc_path_names[i], ret); + return ret; + } + } + + return 0; +} +EXPORT_SYMBOL(geni_icc_enable); + +int geni_icc_disable(struct geni_se *se) +{ + int i, ret; + + for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { + ret = icc_disable(se->icc_paths[i].path); + if (ret) { + dev_err_ratelimited(se->dev, "ICC disable failed on path:%s, ret:%d\n", + icc_path_names[i], ret); + return ret; + } + } + + return 0; +} +EXPORT_SYMBOL(geni_icc_disable); + static int geni_se_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h index dd46494..7afa08d 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, @@ -25,6 +27,18 @@ enum geni_se_protocol_type { struct geni_wrapper; struct clk; +enum geni_icc_path_index { + GENI_TO_CORE, + CPU_TO_GENI, + GENI_TO_DDR +}; + +struct geni_icc_path { + struct icc_path *path; + unsigned int avg_bw; + unsigned int peak_bw; +}; + /** * struct geni_se - GENI Serial Engine * @base: Base Address of the Serial Engine's register block @@ -33,6 +47,7 @@ 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_paths: Array of ICC paths for SE */ struct geni_se { void __iomem *base; @@ -41,6 +56,7 @@ struct geni_se { struct clk *clk; unsigned int num_clk_levels; unsigned long *clk_perf_tbl; + struct geni_icc_path icc_paths[3]; }; /* Common SE registers */ @@ -229,6 +245,21 @@ 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) +/* + * 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. + */ +#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 + +#define GENI_DEFAULT_BW Bps_to_icc(1000) + #if IS_ENABLED(CONFIG_QCOM_GENI_SE) u32 geni_se_get_qup_hw_version(struct geni_se *se); @@ -416,5 +447,16 @@ int geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len, void geni_se_tx_dma_unprep(struct geni_se *se, dma_addr_t iova, size_t len); void geni_se_rx_dma_unprep(struct geni_se *se, dma_addr_t iova, size_t len); + +int geni_icc_get(struct geni_se *se, const char *icc_ddr); + +int geni_icc_set_bw(struct geni_se *se); + +void geni_icc_bw_init(struct geni_icc_path *icc_paths, unsigned int avg_bw, + unsigned int peak_bw); + +int geni_icc_enable(struct geni_se *se); + +int geni_icc_disable(struct geni_se *se); #endif #endif
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 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. 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}; 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. drivers/soc/qcom/qcom-geni-se.c | 92 +++++++++++++++++++++++++++++++++++++++++ include/linux/qcom-geni-se.h | 42 +++++++++++++++++++ 2 files changed, 134 insertions(+)