[RFC v3 0/2] Add SDHC interconnect bandwidth scaling

[RFC v3 0/2] Add SDHC interconnect bandwidth scaling

[Pradeep P V K]

Add interconnect bandwidths for SDHC driver using OPP framework that
is required by SDHC driver based on the clock frequency and bus width
of the card. Otherwise, the system clocks may run at minimum clock
speed and thus affecting the performance.


This change is based on Georgi Djakov [RFC]
(https://lkml.org/lkml/2018/10/11/499) and

Saravana Kannan [PATCH v6]
(https://lkml.org/lkml/2019/12/6/740)

change from RFC v2 -> v3 changes:
- Modified interconnect bandwidth support using OPP framework.
- Addressed review comments on v2.

Pradeep P V K (2):
  mmc: sdhci-msm: Add interconnect bus bandwidth scaling support
  dt-bindings: mmc: sdhci-msm: Add interconnect BW scaling strings

 .../devicetree/bindings/mmc/sdhci-msm.txt          |  18 ++
 drivers/mmc/host/sdhci-msm.c                       | 251 ++++++++++++++++++++-
 2 files changed, 265 insertions(+), 4 deletions(-)

-- 
1.9.1

[RFC v3 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support

[Pradeep P V K]

Add interconnect bandwidths for SDHC driver using OPP framework that
is required by SDHC driver based on the clock frequency and bus width
of the card. Otherwise, the system clocks may run at minimum clock
speed and thus affecting the performance.

This change is based on Georgi Djakov [RFC]
(https://lkml.org/lkml/2018/10/11/499) and

Saravana Kannan [PATCH v6]
(https://lkml.org/lkml/2019/12/6/740)

change from RFC v2 -> v3 changes:
- Modified interconnect bandwidth support using OPP framework.
- Addressed review comments on v2.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
Co-developed-by: Pradeep P V K <ppvk@codeaurora.org>
Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 251 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 247 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 71f29ba..f061cd8 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -11,8 +11,10 @@
 #include <linux/mmc/mmc.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
+#include <linux/interconnect.h>
 #include <linux/iopoll.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm_opp.h>
 
 #include "sdhci-pltfm.h"
 
@@ -229,6 +231,14 @@ struct sdhci_msm_variant_info {
 	const struct sdhci_msm_offset *offset;
 };
 
+struct sdhci_msm_bus_vote_data {
+
+	struct icc_path *sdhc_to_ddr;
+	struct icc_path *cpu_to_sdhc;
+	bool skip_bus_bw_voting;
+	u32 curr_freq;
+};
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -255,8 +265,11 @@ struct sdhci_msm_host {
 	bool use_cdr;
 	u32 transfer_mode;
 	bool updated_ddr_cfg;
+	struct sdhci_msm_bus_vote_data *bus_vote_data;
 };
 
+static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable);
+
 static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -1564,6 +1577,7 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	msm_set_clock_rate_for_bus_mode(host, clock);
 out:
+	sdhci_msm_bus_voting(host, !!clock);
 	__sdhci_msm_set_clock(host, clock);
 }
 
@@ -1685,6 +1699,219 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
 	pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
 }
 
+/*
+ * Returns required bandwidth in Bytes per Sec
+ */
+static unsigned long sdhci_get_bw_required(struct sdhci_host *host,
+					struct mmc_ios *ios)
+{
+	unsigned long bw;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+	bw = msm_host->clk_rate;
+
+	if (ios->bus_width == MMC_BUS_WIDTH_4)
+		bw /= 2;
+	else if (ios->bus_width == MMC_BUS_WIDTH_1)
+		bw /= 8;
+
+	return bw;
+}
+
+/*
+ * Helper function to parse the exact OPP node
+ * Returns OPP pointer on success else NULL on error
+ */
+static struct dev_pm_opp *find_opp_for_freq(struct sdhci_msm_host *msm_host,
+							unsigned long bw)
+{
+	struct dev_pm_opp *opp;
+	struct sdhci_host *host = mmc_priv(msm_host->mmc);
+	unsigned int freq = bw;
+	struct device *dev = &msm_host->pdev->dev;
+
+
+	if (!freq)
+		opp = dev_pm_opp_find_peak_bw_floor(dev, &freq);
+	else
+		opp = dev_pm_opp_find_peak_bw_exact(dev, freq, true);
+
+	/* Max Bandwidth vote */
+	if (PTR_ERR(opp) == -ERANGE && freq > sdhci_msm_get_max_clock(host))
+		opp = dev_pm_opp_find_peak_bw_ceil(dev, &bw);
+
+	if (IS_ERR(opp)) {
+		dev_err(dev, "Failed to find OPP for freq:%u err:%ld\n",
+				freq, PTR_ERR(opp));
+		return NULL;
+	}
+	return opp;
+}
+
+/*
+ * This function sets the interconnect bus bandwidth
+ * vote based on bw (bandwidth) argument.
+ */
+#define BUS_INTERCONNECT_PATHS 2 /* 1. sdhc -> ddr 2. cpu -> sdhc */
+static void sdhci_msm_bus_set_vote(struct sdhci_host *host,
+						unsigned int bw)
+{
+	int i;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	struct sdhci_msm_bus_vote_data *vote_data = msm_host->bus_vote_data;
+	struct dev_pm_opp *opp;
+	unsigned long freq = bw;
+	unsigned long ib[BUS_INTERCONNECT_PATHS], ab[BUS_INTERCONNECT_PATHS];
+	int ddr_rc = 0, cpu_rc = 0;
+
+	if (!msm_host->bus_vote_data->sdhc_to_ddr ||
+			!msm_host->bus_vote_data->cpu_to_sdhc)
+		return;
+
+	if (bw != vote_data->curr_freq) {
+		for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
+			opp = find_opp_for_freq(msm_host, freq);
+			if (opp) {
+				ab[i] =	dev_pm_opp_get_bw(opp, &ib[i]);
+				freq += 1; /* Next Band width vote */
+				dev_pm_opp_put(opp);
+			}
+		}
+		pr_debug("%s: freq:%d sdhc_to_ddr ab:%lu ib:%lu cpu_to_sdhc ab:%lu ib:%lu\n",
+				mmc_hostname(host->mmc), bw, ab[0], ib[0],
+				ab[1], ib[1]);
+		ddr_rc = icc_set_bw(vote_data->sdhc_to_ddr, 0, ib[0]);
+		cpu_rc = icc_set_bw(vote_data->cpu_to_sdhc, 0, ib[1]);
+		if (ddr_rc || cpu_rc) {
+			pr_err("%s: icc_set() failed ddr_rc_err:%d cpu_rc_err:%d\n",
+				mmc_hostname(host->mmc), ddr_rc, cpu_rc);
+			return;
+		}
+		vote_data->curr_freq = bw;
+	}
+}
+
+/*
+ * This function registers the device to OPP framework and
+ * parses few optional parameters from the device tree node.
+ * Returns NULL bvd pointer on error else a valid bvd pointer.
+ */
+static struct sdhci_msm_bus_vote_data *sdhci_msm_get_bus_vote_data(struct device
+				       *dev, struct sdhci_msm_host *host)
+
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct sdhci_msm_bus_vote_data *vote_data = NULL;
+	int ret = 0;
+
+	if (!pdev) {
+		dev_err(dev, "Null platform device!\n");
+		return NULL;
+	}
+	vote_data = devm_kzalloc(dev, sizeof(*vote_data), GFP_KERNEL);
+	if (!vote_data)
+		return vote_data;
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret) {
+		if (ret == -ENODEV || ret == -ENODATA) {
+			dev_dbg(dev, "OPP not found. Skip bus voting!!:%d\n",
+					ret);
+			vote_data->skip_bus_bw_voting = true;
+		} else {
+			dev_dbg(dev, "OPP regestration failed:%d\n", ret);
+			goto err;
+		}
+	}
+
+	return vote_data;
+err:
+	devm_kfree(dev, vote_data);
+	vote_data = NULL;
+	return vote_data;
+}
+
+/*
+ * Helper function to register for OPP and interconnect
+ * frameworks.
+ */
+static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
+				struct platform_device *pdev)
+{
+	struct sdhci_msm_bus_vote_data *vote_data;
+	struct device *dev = &pdev->dev;
+	int ret = 0;
+
+	vote_data = sdhci_msm_get_bus_vote_data(dev, host);
+	if (!vote_data) {
+		dev_dbg(&pdev->dev, "Failed to get bus_scale data\n");
+		host->bus_vote_data = NULL;
+		return -EINVAL;
+	}
+	host->bus_vote_data = vote_data;
+
+	vote_data->sdhc_to_ddr = of_icc_get(&pdev->dev, "sdhc-ddr");
+	if (!vote_data->sdhc_to_ddr) {
+		dev_dbg(&pdev->dev, "DT property for path %s missing\n",
+				"sdhc-ddr");
+		return -ENOENT;
+	} else if (IS_ERR(vote_data->sdhc_to_ddr) {
+		dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
+				PTR_ERR(vote_data->sdhc_to_ddr), "sdhc-ddr");
+		ret = PTR_ERR(vote_data->sdhc_to_ddr);
+		vote_data->sdhc_to_ddr = NULL;
+		return ret;
+	}
+	vote_data->cpu_to_sdhc = of_icc_get(&pdev->dev, "cpu-sdhc");
+	if (!vote_data->cpu_to_sdhc) {
+		dev_dbg(&pdev->dev, "DT property for path %s missing\n",
+					"cpu_to_sdhc");
+		return -ENOENT;
+	} else if (IS_ERR(vote_data->cpu_to_sdhc)) {
+		dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
+				PTR_ERR(vote_data->cpu_to_sdhc), "cpu-sdhc");
+		ret = PTR_ERR(vote_data->cpu_to_sdhc);
+		vote_data->cpu_to_sdhc = NULL;
+		return ret;
+	}
+	return ret;
+}
+
+static void sdhci_msm_bus_unregister(struct device *dev,
+				struct sdhci_msm_host *host)
+{
+	struct sdhci_msm_bus_vote_data *vote_data = host->bus_vote_data;
+
+	if (vote_data->skip_bus_bw_voting ||
+		!vote_data->sdhc_to_ddr ||
+		!vote_data->cpu_to_sdhc)
+		return;
+
+	icc_put(vote_data->sdhc_to_ddr);
+	icc_put(vote_data->cpu_to_sdhc);
+}
+
+#define MSM_MMC_BUS_VOTING_DELAY        200 /* msecs */
+
+static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
+{
+	struct mmc_ios *ios = &host->mmc->ios;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	unsigned int bw;
+
+	if (msm_host->bus_vote_data->skip_bus_bw_voting)
+		return;
+
+	if (enable) {
+		bw = sdhci_get_bw_required(host, ios);
+		sdhci_msm_bus_set_vote(host, bw);
+	} else
+		sdhci_msm_bus_set_vote(host, 0);
+}
+
 static const struct sdhci_msm_variant_ops mci_var_ops = {
 	.msm_readl_relaxed = sdhci_msm_mci_variant_readl_relaxed,
 	.msm_writel_relaxed = sdhci_msm_mci_variant_writel_relaxed,
@@ -1845,11 +2072,20 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret);
 	}
 
+	ret = sdhci_msm_bus_register(msm_host, pdev);
+	if (ret && msm_host->bus_vote_data &&
+			!msm_host->bus_vote_data->skip_bus_bw_voting) {
+		dev_err(&pdev->dev, "Bus registration failed (%d)\n", ret);
+		goto clk_disable;
+	}
+
+	sdhci_msm_bus_voting(host, 1);
+
 	if (!msm_host->mci_removed) {
 		msm_host->core_mem = devm_platform_ioremap_resource(pdev, 1);
 		if (IS_ERR(msm_host->core_mem)) {
 			ret = PTR_ERR(msm_host->core_mem);
-			goto clk_disable;
+			goto bus_unregister;
 		}
 	}
 
@@ -1924,7 +2160,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
 	if (msm_host->pwr_irq < 0) {
 		ret = msm_host->pwr_irq;
-		goto clk_disable;
+		goto bus_unregister;
 	}
 
 	sdhci_msm_init_pwr_irq_wait(msm_host);
@@ -1937,7 +2173,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 					dev_name(&pdev->dev), host);
 	if (ret) {
 		dev_err(&pdev->dev, "Request IRQ failed (%d)\n", ret);
-		goto clk_disable;
+		goto bus_unregister;
 	}
 
 	pm_runtime_get_noresume(&pdev->dev);
@@ -1962,6 +2198,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
+bus_unregister:
+	sdhci_msm_bus_voting(host, 0);
+	sdhci_msm_bus_unregister(&pdev->dev, msm_host);
 clk_disable:
 	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
 				   msm_host->bulk_clks);
@@ -1991,6 +2230,9 @@ static int sdhci_msm_remove(struct platform_device *pdev)
 				   msm_host->bulk_clks);
 	if (!IS_ERR(msm_host->bus_clk))
 		clk_disable_unprepare(msm_host->bus_clk);
+
+	sdhci_msm_bus_voting(host, 0);
+	sdhci_msm_bus_unregister(&pdev->dev, msm_host);
 	sdhci_pltfm_free(pdev);
 	return 0;
 }
@@ -2003,7 +2245,7 @@ static __maybe_unused int sdhci_msm_runtime_suspend(struct device *dev)
 
 	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
 				   msm_host->bulk_clks);
-
+	sdhci_msm_bus_voting(host, 0);
 	return 0;
 }
 
@@ -2025,6 +2267,7 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev)
 	if (msm_host->restore_dll_config && msm_host->clk_rate)
 		return sdhci_msm_restore_sdr_dll_config(host);
 
+	sdhci_msm_bus_voting(host, 1);
 	return 0;
 }
 
-- 
1.9.1

Re: [RFC v3 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support

[Matthias Kaehlcke]

Hi Pradeep,

On Fri, Feb 07, 2020 at 08:14:54PM +0530, Pradeep P V K wrote:
> Add interconnect bandwidths for SDHC driver using OPP framework that
> is required by SDHC driver based on the clock frequency and bus width
> of the card. Otherwise, the system clocks may run at minimum clock
> speed and thus affecting the performance.
> 
> This change is based on Georgi Djakov [RFC]
> (https://lkml.org/lkml/2018/10/11/499) and
> 
> Saravana Kannan [PATCH v6]
> (https://lkml.org/lkml/2019/12/6/740)
> 
> change from RFC v2 -> v3 changes:
> - Modified interconnect bandwidth support using OPP framework.
> - Addressed review comments on v2.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Co-developed-by: Pradeep P V K <ppvk@codeaurora.org>
> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 251 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 247 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 71f29ba..f061cd8 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -11,8 +11,10 @@
>  #include <linux/mmc/mmc.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
> +#include <linux/interconnect.h>
>  #include <linux/iopoll.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pm_opp.h>
>  
>  #include "sdhci-pltfm.h"
>  
> @@ -229,6 +231,14 @@ struct sdhci_msm_variant_info {
>  	const struct sdhci_msm_offset *offset;
>  };
>  
> +struct sdhci_msm_bus_vote_data {
> +
> +	struct icc_path *sdhc_to_ddr;
> +	struct icc_path *cpu_to_sdhc;
> +	bool skip_bus_bw_voting;

nit: the name is somewhat redundant given the name of the containing
struct. 'skip_vote' or 'skip_voting' would be sufficient IMO.

> +	u32 curr_freq;
> +};
> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -255,8 +265,11 @@ struct sdhci_msm_host {
>  	bool use_cdr;
>  	u32 transfer_mode;
>  	bool updated_ddr_cfg;
> +	struct sdhci_msm_bus_vote_data *bus_vote_data;
>  };
>  
> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable);
> +
>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1564,6 +1577,7 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  
>  	msm_set_clock_rate_for_bus_mode(host, clock);
>  out:
> +	sdhci_msm_bus_voting(host, !!clock);
>  	__sdhci_msm_set_clock(host, clock);
>  }
>  
> @@ -1685,6 +1699,219 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>  	pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
>  }
>  
> +/*
> + * Returns required bandwidth in Bytes per Sec
> + */
> +static unsigned long sdhci_get_bw_required(struct sdhci_host *host,
> +					struct mmc_ios *ios)
> +{
> +	unsigned long bw;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	bw = msm_host->clk_rate;

nit: you could declare 'bw' last and do the assignment together with the
declaration.

> +
> +	if (ios->bus_width == MMC_BUS_WIDTH_4)
> +		bw /= 2;
> +	else if (ios->bus_width == MMC_BUS_WIDTH_1)
> +		bw /= 8;
> +
> +	return bw;
> +}
> +
> +/*
> + * Helper function to parse the exact OPP node
> + * Returns OPP pointer on success else NULL on error
> + */
> +static struct dev_pm_opp *find_opp_for_freq(struct sdhci_msm_host *msm_host,
> +							unsigned long bw)
> +{
> +	struct dev_pm_opp *opp;
> +	struct sdhci_host *host = mmc_priv(msm_host->mmc);
> +	unsigned int freq = bw;
> +	struct device *dev = &msm_host->pdev->dev;
> +
> +
> +	if (!freq)
> +		opp = dev_pm_opp_find_peak_bw_floor(dev, &freq);
> +	else
> +		opp = dev_pm_opp_find_peak_bw_exact(dev, freq, true);
> +
> +	/* Max Bandwidth vote */

s/Bandwidth/bandwidth/

> +	if (PTR_ERR(opp) == -ERANGE && freq > sdhci_msm_get_max_clock(host))
> +		opp = dev_pm_opp_find_peak_bw_ceil(dev, &bw);
> +
> +	if (IS_ERR(opp)) {
> +		dev_err(dev, "Failed to find OPP for freq:%u err:%ld\n",
> +				freq, PTR_ERR(opp));
> +		return NULL;
> +	}
> +	return opp;
> +}
> +
> +/*
> + * This function sets the interconnect bus bandwidth
> + * vote based on bw (bandwidth) argument.
> + */
> +#define BUS_INTERCONNECT_PATHS 2 /* 1. sdhc -> ddr 2. cpu -> sdhc */
> +static void sdhci_msm_bus_set_vote(struct sdhci_host *host,
> +						unsigned int bw)
> +{
> +	int i;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	struct sdhci_msm_bus_vote_data *vote_data = msm_host->bus_vote_data;
> +	struct dev_pm_opp *opp;
> +	unsigned long freq = bw;
> +	unsigned long ib[BUS_INTERCONNECT_PATHS], ab[BUS_INTERCONNECT_PATHS];

the names are not very expressive, how about peak_bw and avg_bw?

> +	int ddr_rc = 0, cpu_rc = 0;

initialization is not needed

> +
> +	if (!msm_host->bus_vote_data->sdhc_to_ddr ||
> +			!msm_host->bus_vote_data->cpu_to_sdhc)
> +		return;
> +
> +	if (bw != vote_data->curr_freq) {

  	if (bw == vote_data->curr_freq)
		return;


> +		for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
> +			opp = find_opp_for_freq(msm_host, freq);
> +			if (opp) {
> +				ab[i] =	dev_pm_opp_get_bw(opp, &ib[i]);
> +				freq += 1; /* Next Band width vote */

s/Band width/bandwidth/

> +				dev_pm_opp_put(opp);
> +			}
> +		}
> +		pr_debug("%s: freq:%d sdhc_to_ddr ab:%lu ib:%lu cpu_to_sdhc ab:%lu ib:%lu\n",
> +				mmc_hostname(host->mmc), bw, ab[0], ib[0],
> +				ab[1], ib[1]);
> +		ddr_rc = icc_set_bw(vote_data->sdhc_to_ddr, 0, ib[0]);
> +		cpu_rc = icc_set_bw(vote_data->cpu_to_sdhc, 0, ib[1]);
> +		if (ddr_rc || cpu_rc) {
> +			pr_err("%s: icc_set() failed ddr_rc_err:%d cpu_rc_err:%d\n",
> +				mmc_hostname(host->mmc), ddr_rc, cpu_rc);
> +			return;
> +		}
> +		vote_data->curr_freq = bw;
> +	}
> +}
> +
> +/*
> + * This function registers the device to OPP framework and
> + * parses few optional parameters from the device tree node.
> + * Returns NULL bvd pointer on error else a valid bvd pointer.
> + */
> +static struct sdhci_msm_bus_vote_data *sdhci_msm_get_bus_vote_data(struct device
> +				       *dev, struct sdhci_msm_host *host)
> +
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct sdhci_msm_bus_vote_data *vote_data = NULL;
> +	int ret = 0;

initialization of 'vote_data' and 'ret' is not needed.

> +
> +	if (!pdev) {
> +		dev_err(dev, "Null platform device!\n");
> +		return NULL;
> +	}

It's probably safe to assume that the device is a platform device?

> +	vote_data = devm_kzalloc(dev, sizeof(*vote_data), GFP_KERNEL);
> +	if (!vote_data)
> +		return vote_data;

  		return ERR_PTR(-ENOMEM); ?

> +
> +	ret = dev_pm_opp_of_add_table(dev);
> +	if (ret) {
> +		if (ret == -ENODEV || ret == -ENODATA) {
> +			dev_dbg(dev, "OPP not found. Skip bus voting!!:%d\n",
> +					ret);
> +			vote_data->skip_bus_bw_voting = true;
> +		} else {
> +			dev_dbg(dev, "OPP regestration failed:%d\n", ret);

s/regestration/registration/

Depending on the consequences of this failure (e.g. probe() fails) you
probably want to use dev_warn() or dev_err() here.

> +			goto err;
> +		}
> +	}
> +
> +	return vote_data;
> +err:
> +	devm_kfree(dev, vote_data);
> +	vote_data = NULL;

not needed

> +	return vote_data;

  	return NULL;

> +}
> +
> +/*
> + * Helper function to register for OPP and interconnect
> + * frameworks.
> + */
> +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
> +				struct platform_device *pdev)
> +{
> +	struct sdhci_msm_bus_vote_data *vote_data;
> +	struct device *dev = &pdev->dev;
> +	int ret = 0;
> +
> +	vote_data = sdhci_msm_get_bus_vote_data(dev, host);
> +	if (!vote_data) {
> +		dev_dbg(&pdev->dev, "Failed to get bus_scale data\n");

In general I'm not convinced about the use of dev_dbg() in the error
paths. Maybe there is some signal you can use to determine if there
should be a valid bus scaling information (e.g. some DT property
exists). If it exists you proceed with obtaining the necessary
information and complain loudly if something goes wrong, otherwise you
don't even try to obtain it and omit the debug logs.

> +		host->bus_vote_data = NULL;

IIUC struct sdhci_msm_host is zero initialized by sdhci_pltfm_init(),
hence the assignment to NULL is not needed.

> +		return -EINVAL;

Use the error returned by sdhci_msm_get_bus_vote_data().

> +	}
> +	host->bus_vote_data = vote_data;
> +
> +	vote_data->sdhc_to_ddr = of_icc_get(&pdev->dev, "sdhc-ddr");

This could be the signal I mentioned above. If this entry does not
exist it is pointless to proceed. You could check this before
sdhci_msm_get_bus_vote_data() and abort with a dev_info() message in
case it doesn't exist. If it exists any error afterwards will be
reported with dev_err().

> +	if (!vote_data->sdhc_to_ddr) {
> +		dev_dbg(&pdev->dev, "DT property for path %s missing\n",
> +				"sdhc-ddr");
> +		return -ENOENT;
> +	} else if (IS_ERR(vote_data->sdhc_to_ddr) {
> +		dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
> +				PTR_ERR(vote_data->sdhc_to_ddr), "sdhc-ddr");
> +		ret = PTR_ERR(vote_data->sdhc_to_ddr);
> +		vote_data->sdhc_to_ddr = NULL;
> +		return ret;

if there is an error probe() should probably be aborted. If that's the
case you can replace the above three lines with 'return PTR_ERR(vote_data->sdhc_to_ddr);'

> +	}
> +	vote_data->cpu_to_sdhc = of_icc_get(&pdev->dev, "cpu-sdhc");
> +	if (!vote_data->cpu_to_sdhc) {
> +		dev_dbg(&pdev->dev, "DT property for path %s missing\n",
> +					"cpu_to_sdhc");
> +		return -ENOENT;
> +	} else if (IS_ERR(vote_data->cpu_to_sdhc)) {
> +		dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
> +				PTR_ERR(vote_data->cpu_to_sdhc), "cpu-sdhc");
> +		ret = PTR_ERR(vote_data->cpu_to_sdhc);
> +		vote_data->cpu_to_sdhc = NULL;
> +		return ret;

same comment as above

> +	}
> +	return ret;

no error if we get to this point, hence:

	return 0;

> +}
> +
> +static void sdhci_msm_bus_unregister(struct device *dev,
> +				struct sdhci_msm_host *host)
> +{
> +	struct sdhci_msm_bus_vote_data *vote_data = host->bus_vote_data;
> +
> +	if (vote_data->skip_bus_bw_voting ||
> +		!vote_data->sdhc_to_ddr ||
> +		!vote_data->cpu_to_sdhc)
> +		return;
> +
> +	icc_put(vote_data->sdhc_to_ddr);
> +	icc_put(vote_data->cpu_to_sdhc);
> +}
> +
> +#define MSM_MMC_BUS_VOTING_DELAY        200 /* msecs */
> +
> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)

s/u32/bool/ ?

> +{
> +	struct mmc_ios *ios = &host->mmc->ios;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	unsigned int bw;
> +
> +	if (msm_host->bus_vote_data->skip_bus_bw_voting)
> +		return;
> +
> +	if (enable) {
> +		bw = sdhci_get_bw_required(host, ios);
> +		sdhci_msm_bus_set_vote(host, bw);
> +	} else
> +		sdhci_msm_bus_set_vote(host, 0);
> +}
> +
>  static const struct sdhci_msm_variant_ops mci_var_ops = {
>  	.msm_readl_relaxed = sdhci_msm_mci_variant_readl_relaxed,
>  	.msm_writel_relaxed = sdhci_msm_mci_variant_writel_relaxed,
> @@ -1845,11 +2072,20 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  		dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret);
>  	}
>  
> +	ret = sdhci_msm_bus_register(msm_host, pdev);
> +	if (ret && msm_host->bus_vote_data &&
> +			!msm_host->bus_vote_data->skip_bus_bw_voting) {
> +		dev_err(&pdev->dev, "Bus registration failed (%d)\n", ret);
> +		goto clk_disable;
> +	}
> +
> +	sdhci_msm_bus_voting(host, 1);

s/1/true/

> +
>  	if (!msm_host->mci_removed) {
>  		msm_host->core_mem = devm_platform_ioremap_resource(pdev, 1);
>  		if (IS_ERR(msm_host->core_mem)) {
>  			ret = PTR_ERR(msm_host->core_mem);
> -			goto clk_disable;
> +			goto bus_unregister;
>  		}
>  	}
>  
> @@ -1924,7 +2160,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
>  	if (msm_host->pwr_irq < 0) {
>  		ret = msm_host->pwr_irq;
> -		goto clk_disable;
> +		goto bus_unregister;
>  	}
>  
>  	sdhci_msm_init_pwr_irq_wait(msm_host);
> @@ -1937,7 +2173,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  					dev_name(&pdev->dev), host);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Request IRQ failed (%d)\n", ret);
> -		goto clk_disable;
> +		goto bus_unregister;
>  	}
>  
>  	pm_runtime_get_noresume(&pdev->dev);
> @@ -1962,6 +2198,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_set_suspended(&pdev->dev);
>  	pm_runtime_put_noidle(&pdev->dev);
> +bus_unregister:
> +	sdhci_msm_bus_voting(host, 0);

s/0/false/

> +	sdhci_msm_bus_unregister(&pdev->dev, msm_host);
>  clk_disable:
>  	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
>  				   msm_host->bulk_clks);
> @@ -1991,6 +2230,9 @@ static int sdhci_msm_remove(struct platform_device *pdev)
>  				   msm_host->bulk_clks);
>  	if (!IS_ERR(msm_host->bus_clk))
>  		clk_disable_unprepare(msm_host->bus_clk);
> +
> +	sdhci_msm_bus_voting(host, 0);

s/0/false/

> +	sdhci_msm_bus_unregister(&pdev->dev, msm_host);
>  	sdhci_pltfm_free(pdev);
>  	return 0;
>  }
> @@ -2003,7 +2245,7 @@ static __maybe_unused int sdhci_msm_runtime_suspend(struct device *dev)
>  
>  	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
>  				   msm_host->bulk_clks);
> -
> +	sdhci_msm_bus_voting(host, 0);

s/0/false/

>  	return 0;
>  }
>  
> @@ -2025,6 +2267,7 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev)
>  	if (msm_host->restore_dll_config && msm_host->clk_rate)
>  		return sdhci_msm_restore_sdr_dll_config(host);
>  
> +	sdhci_msm_bus_voting(host, 1);

s/0/true/

Thanks

Matthias

Re: [RFC v3 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support

[ppvk]

Hi Matthias,

Thanks for the review !

On 2020-02-08 06:12, Matthias Kaehlcke wrote:
> Hi Pradeep,
> 
> On Fri, Feb 07, 2020 at 08:14:54PM +0530, Pradeep P V K wrote:
>> Add interconnect bandwidths for SDHC driver using OPP framework that
>> is required by SDHC driver based on the clock frequency and bus width
>> of the card. Otherwise, the system clocks may run at minimum clock
>> speed and thus affecting the performance.
>> 
>> This change is based on Georgi Djakov [RFC]
>> (https://lkml.org/lkml/2018/10/11/499) and
>> 
>> Saravana Kannan [PATCH v6]
>> (https://lkml.org/lkml/2019/12/6/740)
>> 
>> change from RFC v2 -> v3 changes:
>> - Modified interconnect bandwidth support using OPP framework.
>> - Addressed review comments on v2.
>> 
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>> Co-developed-by: Pradeep P V K <ppvk@codeaurora.org>
>> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 251 
>> ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 247 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/mmc/host/sdhci-msm.c 
>> b/drivers/mmc/host/sdhci-msm.c
>> index 71f29ba..f061cd8 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -11,8 +11,10 @@
>>  #include <linux/mmc/mmc.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/slab.h>
>> +#include <linux/interconnect.h>
>>  #include <linux/iopoll.h>
>>  #include <linux/regulator/consumer.h>
>> +#include <linux/pm_opp.h>
>> 
>>  #include "sdhci-pltfm.h"
>> 
>> @@ -229,6 +231,14 @@ struct sdhci_msm_variant_info {
>>  	const struct sdhci_msm_offset *offset;
>>  };
>> 
>> +struct sdhci_msm_bus_vote_data {
>> +
>> +	struct icc_path *sdhc_to_ddr;
>> +	struct icc_path *cpu_to_sdhc;
>> +	bool skip_bus_bw_voting;
> 
> nit: the name is somewhat redundant given the name of the containing
> struct. 'skip_vote' or 'skip_voting' would be sufficient IMO.
> 
ok. i will address in my next patch series.

>> +	u32 curr_freq;
>> +};
>> +
>>  struct sdhci_msm_host {
>>  	struct platform_device *pdev;
>>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
>> @@ -255,8 +265,11 @@ struct sdhci_msm_host {
>>  	bool use_cdr;
>>  	u32 transfer_mode;
>>  	bool updated_ddr_cfg;
>> +	struct sdhci_msm_bus_vote_data *bus_vote_data;
>>  };
>> 
>> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 
>> enable);
>> +
>>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct 
>> sdhci_host *host)
>>  {
>>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -1564,6 +1577,7 @@ static void sdhci_msm_set_clock(struct 
>> sdhci_host *host, unsigned int clock)
>> 
>>  	msm_set_clock_rate_for_bus_mode(host, clock);
>>  out:
>> +	sdhci_msm_bus_voting(host, !!clock);
>>  	__sdhci_msm_set_clock(host, clock);
>>  }
>> 
>> @@ -1685,6 +1699,219 @@ static void 
>> sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>>  	pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
>>  }
>> 
>> +/*
>> + * Returns required bandwidth in Bytes per Sec
>> + */
>> +static unsigned long sdhci_get_bw_required(struct sdhci_host *host,
>> +					struct mmc_ios *ios)
>> +{
>> +	unsigned long bw;
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	bw = msm_host->clk_rate;
> 
> nit: you could declare 'bw' last and do the assignment together with 
> the
> declaration.
> 
ok. I will do it in my next patch.
>> +
>> +	if (ios->bus_width == MMC_BUS_WIDTH_4)
>> +		bw /= 2;
>> +	else if (ios->bus_width == MMC_BUS_WIDTH_1)
>> +		bw /= 8;
>> +
>> +	return bw;
>> +}
>> +
>> +/*
>> + * Helper function to parse the exact OPP node
>> + * Returns OPP pointer on success else NULL on error
>> + */
>> +static struct dev_pm_opp *find_opp_for_freq(struct sdhci_msm_host 
>> *msm_host,
>> +							unsigned long bw)
>> +{
>> +	struct dev_pm_opp *opp;
>> +	struct sdhci_host *host = mmc_priv(msm_host->mmc);
>> +	unsigned int freq = bw;
>> +	struct device *dev = &msm_host->pdev->dev;
>> +
>> +
>> +	if (!freq)
>> +		opp = dev_pm_opp_find_peak_bw_floor(dev, &freq);
>> +	else
>> +		opp = dev_pm_opp_find_peak_bw_exact(dev, freq, true);
>> +
>> +	/* Max Bandwidth vote */
> 
> s/Bandwidth/bandwidth/
> 
ok.

>> +	if (PTR_ERR(opp) == -ERANGE && freq > sdhci_msm_get_max_clock(host))
>> +		opp = dev_pm_opp_find_peak_bw_ceil(dev, &bw);
>> +
>> +	if (IS_ERR(opp)) {
>> +		dev_err(dev, "Failed to find OPP for freq:%u err:%ld\n",
>> +				freq, PTR_ERR(opp));
>> +		return NULL;
>> +	}
>> +	return opp;
>> +}
>> +
>> +/*
>> + * This function sets the interconnect bus bandwidth
>> + * vote based on bw (bandwidth) argument.
>> + */
>> +#define BUS_INTERCONNECT_PATHS 2 /* 1. sdhc -> ddr 2. cpu -> sdhc */
>> +static void sdhci_msm_bus_set_vote(struct sdhci_host *host,
>> +						unsigned int bw)
>> +{
>> +	int i;
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	struct sdhci_msm_bus_vote_data *vote_data = msm_host->bus_vote_data;
>> +	struct dev_pm_opp *opp;
>> +	unsigned long freq = bw;
>> +	unsigned long ib[BUS_INTERCONNECT_PATHS], 
>> ab[BUS_INTERCONNECT_PATHS];
> 
> the names are not very expressive, how about peak_bw and avg_bw?
> 
hmm, ok. i will modify in my next patch set.

>> +	int ddr_rc = 0, cpu_rc = 0;
> 
> initialization is not needed
> 
ok.
>> +
>> +	if (!msm_host->bus_vote_data->sdhc_to_ddr ||
>> +			!msm_host->bus_vote_data->cpu_to_sdhc)
>> +		return;
>> +
>> +	if (bw != vote_data->curr_freq) {
> 
>   	if (bw == vote_data->curr_freq)
> 		return;
> 
> 
>> +		for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
>> +			opp = find_opp_for_freq(msm_host, freq);
>> +			if (opp) {
>> +				ab[i] =	dev_pm_opp_get_bw(opp, &ib[i]);
>> +				freq += 1; /* Next Band width vote */
> 
> s/Band width/bandwidth/
> 
ok.
>> +				dev_pm_opp_put(opp);
>> +			}
>> +		}
>> +		pr_debug("%s: freq:%d sdhc_to_ddr ab:%lu ib:%lu cpu_to_sdhc ab:%lu 
>> ib:%lu\n",
>> +				mmc_hostname(host->mmc), bw, ab[0], ib[0],
>> +				ab[1], ib[1]);
>> +		ddr_rc = icc_set_bw(vote_data->sdhc_to_ddr, 0, ib[0]);
>> +		cpu_rc = icc_set_bw(vote_data->cpu_to_sdhc, 0, ib[1]);
>> +		if (ddr_rc || cpu_rc) {
>> +			pr_err("%s: icc_set() failed ddr_rc_err:%d cpu_rc_err:%d\n",
>> +				mmc_hostname(host->mmc), ddr_rc, cpu_rc);
>> +			return;
>> +		}
>> +		vote_data->curr_freq = bw;
>> +	}
>> +}
>> +
>> +/*
>> + * This function registers the device to OPP framework and
>> + * parses few optional parameters from the device tree node.
>> + * Returns NULL bvd pointer on error else a valid bvd pointer.
>> + */
>> +static struct sdhci_msm_bus_vote_data 
>> *sdhci_msm_get_bus_vote_data(struct device
>> +				       *dev, struct sdhci_msm_host *host)
>> +
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct sdhci_msm_bus_vote_data *vote_data = NULL;
>> +	int ret = 0;
> 
> initialization of 'vote_data' and 'ret' is not needed.
> 
ok. i will be removing this function and bring all the required to the 
caller function
that should address all the comments on this function.
>> +
>> +	if (!pdev) {
>> +		dev_err(dev, "Null platform device!\n");
>> +		return NULL;
>> +	}
> 
> It's probably safe to assume that the device is a platform device?
> 
same as above.
>> +	vote_data = devm_kzalloc(dev, sizeof(*vote_data), GFP_KERNEL);
>> +	if (!vote_data)
>> +		return vote_data;
> 
>   		return ERR_PTR(-ENOMEM); ?
> 
>> +
>> +	ret = dev_pm_opp_of_add_table(dev);
>> +	if (ret) {
>> +		if (ret == -ENODEV || ret == -ENODATA) {
>> +			dev_dbg(dev, "OPP not found. Skip bus voting!!:%d\n",
>> +					ret);
>> +			vote_data->skip_bus_bw_voting = true;
>> +		} else {
>> +			dev_dbg(dev, "OPP regestration failed:%d\n", ret);
> 
> s/regestration/registration/
> 
ok.
> Depending on the consequences of this failure (e.g. probe() fails) you
> probably want to use dev_warn() or dev_err() here.
> 
same as above.
>> +			goto err;
>> +		}
>> +	}
>> +
>> +	return vote_data;
>> +err:
>> +	devm_kfree(dev, vote_data);
>> +	vote_data = NULL;
> 
> not needed
> 
ok.
>> +	return vote_data;
> 
>   	return NULL;
> 
>> +}
>> +
>> +/*
>> + * Helper function to register for OPP and interconnect
>> + * frameworks.
>> + */
>> +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
>> +				struct platform_device *pdev)
>> +{
>> +	struct sdhci_msm_bus_vote_data *vote_data;
>> +	struct device *dev = &pdev->dev;
>> +	int ret = 0;
>> +
>> +	vote_data = sdhci_msm_get_bus_vote_data(dev, host);
>> +	if (!vote_data) {
>> +		dev_dbg(&pdev->dev, "Failed to get bus_scale data\n");
> 
> In general I'm not convinced about the use of dev_dbg() in the error
> paths. Maybe there is some signal you can use to determine if there
> should be a valid bus scaling information (e.g. some DT property
> exists). If it exists you proceed with obtaining the necessary
> information and complain loudly if something goes wrong, otherwise you
> don't even try to obtain it and omit the debug logs.
> 
ok. i will modify accordingly, in my next patch series.

>> +		host->bus_vote_data = NULL;
> 
> IIUC struct sdhci_msm_host is zero initialized by sdhci_pltfm_init(),
> hence the assignment to NULL is not needed.
> 
ok.
>> +		return -EINVAL;
> 
> Use the error returned by sdhci_msm_get_bus_vote_data().
> 
ok. i will address in my next patch set.
>> +	}
>> +	host->bus_vote_data = vote_data;
>> +
>> +	vote_data->sdhc_to_ddr = of_icc_get(&pdev->dev, "sdhc-ddr");
> 
> This could be the signal I mentioned above. If this entry does not
> exist it is pointless to proceed. You could check this before
> sdhci_msm_get_bus_vote_data() and abort with a dev_info() message in
> case it doesn't exist. If it exists any error afterwards will be
> reported with dev_err().
> 
ok. i will modify the code accordingly in my next patch series.

>> +	if (!vote_data->sdhc_to_ddr) {
>> +		dev_dbg(&pdev->dev, "DT property for path %s missing\n",
>> +				"sdhc-ddr");
>> +		return -ENOENT;
>> +	} else if (IS_ERR(vote_data->sdhc_to_ddr) {
>> +		dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
>> +				PTR_ERR(vote_data->sdhc_to_ddr), "sdhc-ddr");
>> +		ret = PTR_ERR(vote_data->sdhc_to_ddr);
>> +		vote_data->sdhc_to_ddr = NULL;
>> +		return ret;
> 
> if there is an error probe() should probably be aborted. If that's the
> case you can replace the above three lines with 'return
> PTR_ERR(vote_data->sdhc_to_ddr);'
> 
True but i added this for more cautious. Any how it is handled as error 
probe().
i will address this in my next patch series.


>> +	}
>> +	vote_data->cpu_to_sdhc = of_icc_get(&pdev->dev, "cpu-sdhc");
>> +	if (!vote_data->cpu_to_sdhc) {
>> +		dev_dbg(&pdev->dev, "DT property for path %s missing\n",
>> +					"cpu_to_sdhc");
>> +		return -ENOENT;
>> +	} else if (IS_ERR(vote_data->cpu_to_sdhc)) {
>> +		dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
>> +				PTR_ERR(vote_data->cpu_to_sdhc), "cpu-sdhc");
>> +		ret = PTR_ERR(vote_data->cpu_to_sdhc);
>> +		vote_data->cpu_to_sdhc = NULL;
>> +		return ret;
> 
> same comment as above
> 
ok. i will take care this too.

>> +	}
>> +	return ret;
> 
> no error if we get to this point, hence:
> 
> 	return 0;
> 
ok.

>> +}
>> +
>> +static void sdhci_msm_bus_unregister(struct device *dev,
>> +				struct sdhci_msm_host *host)
>> +{
>> +	struct sdhci_msm_bus_vote_data *vote_data = host->bus_vote_data;
>> +
>> +	if (vote_data->skip_bus_bw_voting ||
>> +		!vote_data->sdhc_to_ddr ||
>> +		!vote_data->cpu_to_sdhc)
>> +		return;
>> +
>> +	icc_put(vote_data->sdhc_to_ddr);
>> +	icc_put(vote_data->cpu_to_sdhc);
>> +}
>> +
>> +#define MSM_MMC_BUS_VOTING_DELAY        200 /* msecs */
>> +
>> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
> 
> s/u32/bool/ ?
> 
ok.
>> +{
>> +	struct mmc_ios *ios = &host->mmc->ios;
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	unsigned int bw;
>> +
>> +	if (msm_host->bus_vote_data->skip_bus_bw_voting)
>> +		return;
>> +
>> +	if (enable) {
>> +		bw = sdhci_get_bw_required(host, ios);
>> +		sdhci_msm_bus_set_vote(host, bw);
>> +	} else
>> +		sdhci_msm_bus_set_vote(host, 0);
>> +}
>> +
>>  static const struct sdhci_msm_variant_ops mci_var_ops = {
>>  	.msm_readl_relaxed = sdhci_msm_mci_variant_readl_relaxed,
>>  	.msm_writel_relaxed = sdhci_msm_mci_variant_writel_relaxed,
>> @@ -1845,11 +2072,20 @@ static int sdhci_msm_probe(struct 
>> platform_device *pdev)
>>  		dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret);
>>  	}
>> 
>> +	ret = sdhci_msm_bus_register(msm_host, pdev);
>> +	if (ret && msm_host->bus_vote_data &&
>> +			!msm_host->bus_vote_data->skip_bus_bw_voting) {
>> +		dev_err(&pdev->dev, "Bus registration failed (%d)\n", ret);
>> +		goto clk_disable;
>> +	}
>> +
>> +	sdhci_msm_bus_voting(host, 1);
> 
> s/1/true/
> 
ok.
>> +
>>  	if (!msm_host->mci_removed) {
>>  		msm_host->core_mem = devm_platform_ioremap_resource(pdev, 1);
>>  		if (IS_ERR(msm_host->core_mem)) {
>>  			ret = PTR_ERR(msm_host->core_mem);
>> -			goto clk_disable;
>> +			goto bus_unregister;
>>  		}
>>  	}
>> 
>> @@ -1924,7 +2160,7 @@ static int sdhci_msm_probe(struct 
>> platform_device *pdev)
>>  	msm_host->pwr_irq = platform_get_irq_byname(pdev, "pwr_irq");
>>  	if (msm_host->pwr_irq < 0) {
>>  		ret = msm_host->pwr_irq;
>> -		goto clk_disable;
>> +		goto bus_unregister;
>>  	}
>> 
>>  	sdhci_msm_init_pwr_irq_wait(msm_host);
>> @@ -1937,7 +2173,7 @@ static int sdhci_msm_probe(struct 
>> platform_device *pdev)
>>  					dev_name(&pdev->dev), host);
>>  	if (ret) {
>>  		dev_err(&pdev->dev, "Request IRQ failed (%d)\n", ret);
>> -		goto clk_disable;
>> +		goto bus_unregister;
>>  	}
>> 
>>  	pm_runtime_get_noresume(&pdev->dev);
>> @@ -1962,6 +2198,9 @@ static int sdhci_msm_probe(struct 
>> platform_device *pdev)
>>  	pm_runtime_disable(&pdev->dev);
>>  	pm_runtime_set_suspended(&pdev->dev);
>>  	pm_runtime_put_noidle(&pdev->dev);
>> +bus_unregister:
>> +	sdhci_msm_bus_voting(host, 0);
> 
> s/0/false/
> 
ok.
>> +	sdhci_msm_bus_unregister(&pdev->dev, msm_host);
>>  clk_disable:
>>  	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
>>  				   msm_host->bulk_clks);
>> @@ -1991,6 +2230,9 @@ static int sdhci_msm_remove(struct 
>> platform_device *pdev)
>>  				   msm_host->bulk_clks);
>>  	if (!IS_ERR(msm_host->bus_clk))
>>  		clk_disable_unprepare(msm_host->bus_clk);
>> +
>> +	sdhci_msm_bus_voting(host, 0);
> 
> s/0/false/
> 
ok.
>> +	sdhci_msm_bus_unregister(&pdev->dev, msm_host);
>>  	sdhci_pltfm_free(pdev);
>>  	return 0;
>>  }
>> @@ -2003,7 +2245,7 @@ static __maybe_unused int 
>> sdhci_msm_runtime_suspend(struct device *dev)
>> 
>>  	clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),
>>  				   msm_host->bulk_clks);
>> -
>> +	sdhci_msm_bus_voting(host, 0);
> 
> s/0/false/
> 
ok.
>>  	return 0;
>>  }
>> 
>> @@ -2025,6 +2267,7 @@ static __maybe_unused int 
>> sdhci_msm_runtime_resume(struct device *dev)
>>  	if (msm_host->restore_dll_config && msm_host->clk_rate)
>>  		return sdhci_msm_restore_sdr_dll_config(host);
>> 
>> +	sdhci_msm_bus_voting(host, 1);
> 
> s/0/true/
> 
ok. i will address all these in my next patch series.
> Thanks
> 
> Matthias

Re: [RFC v3 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support

[Bjorn Andersson]

On Fri 07 Feb 06:44 PST 2020, Pradeep P V K wrote:

> Add interconnect bandwidths for SDHC driver using OPP framework that
> is required by SDHC driver based on the clock frequency and bus width
> of the card. Otherwise, the system clocks may run at minimum clock
> speed and thus affecting the performance.
> 
> This change is based on Georgi Djakov [RFC]
> (https://lkml.org/lkml/2018/10/11/499) and
> 
> Saravana Kannan [PATCH v6]
> (https://lkml.org/lkml/2019/12/6/740)

Write out the subject of these two patches, rather than stating that
it's based on one RFC and some PATCH version 6. Or just state that it's
based on work by Georgi and Saravana.

> 
> change from RFC v2 -> v3 changes:
> - Modified interconnect bandwidth support using OPP framework.
> - Addressed review comments on v2.

Move the changelog below the --- line

> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Co-developed-by: Pradeep P V K <ppvk@codeaurora.org>
> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>

You should read these as the history of how this patch reached LKML and
can use Co-developed-by to indicate that several people was part of a
single step.

So if you're both co-author and the one sending it to LKML then you
should have 4 Co-developed-by tags here.

> ---
>  drivers/mmc/host/sdhci-msm.c | 251 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 247 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 71f29ba..f061cd8 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -11,8 +11,10 @@
>  #include <linux/mmc/mmc.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
> +#include <linux/interconnect.h>
>  #include <linux/iopoll.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pm_opp.h>
>  
>  #include "sdhci-pltfm.h"
>  
> @@ -229,6 +231,14 @@ struct sdhci_msm_variant_info {
>  	const struct sdhci_msm_offset *offset;
>  };
>  
> +struct sdhci_msm_bus_vote_data {
> +

Move the { to the empty line.

> +	struct icc_path *sdhc_to_ddr;
> +	struct icc_path *cpu_to_sdhc;
> +	bool skip_bus_bw_voting;
> +	u32 curr_freq;
> +};
> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -255,8 +265,11 @@ struct sdhci_msm_host {
>  	bool use_cdr;
>  	u32 transfer_mode;
>  	bool updated_ddr_cfg;
> +	struct sdhci_msm_bus_vote_data *bus_vote_data;
>  };
>  
> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable);
> +
>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1564,6 +1577,7 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  
>  	msm_set_clock_rate_for_bus_mode(host, clock);
>  out:
> +	sdhci_msm_bus_voting(host, !!clock);
>  	__sdhci_msm_set_clock(host, clock);
>  }
>  
> @@ -1685,6 +1699,219 @@ static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>  	pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
>  }
>  
> +/*
> + * Returns required bandwidth in Bytes per Sec
> + */
> +static unsigned long sdhci_get_bw_required(struct sdhci_host *host,
> +					struct mmc_ios *ios)
> +{
> +	unsigned long bw;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	bw = msm_host->clk_rate;
> +
> +	if (ios->bus_width == MMC_BUS_WIDTH_4)
> +		bw /= 2;
> +	else if (ios->bus_width == MMC_BUS_WIDTH_1)
> +		bw /= 8;
> +
> +	return bw;


switch (ios->bus_width) {
case MMC_BUS_WIDTH_1:
	return msm_host->clk_rate / 8;
case MMC_BUS_WIDTH_4:
	return msm_host->clk_rate / 2;
case MMC_BUS_WIDTH_8:
	return msm_host->clk_rate;
}

> +}
> +
> +/*
> + * Helper function to parse the exact OPP node
> + * Returns OPP pointer on success else NULL on error
> + */
> +static struct dev_pm_opp *find_opp_for_freq(struct sdhci_msm_host *msm_host,

A function with this name doesn't belong in the sdhci-msm driver.

> +							unsigned long bw)
> +{
> +	struct dev_pm_opp *opp;
> +	struct sdhci_host *host = mmc_priv(msm_host->mmc);
> +	unsigned int freq = bw;
> +	struct device *dev = &msm_host->pdev->dev;
> +
> +
> +	if (!freq)
> +		opp = dev_pm_opp_find_peak_bw_floor(dev, &freq);
> +	else
> +		opp = dev_pm_opp_find_peak_bw_exact(dev, freq, true);
> +
> +	/* Max Bandwidth vote */
> +	if (PTR_ERR(opp) == -ERANGE && freq > sdhci_msm_get_max_clock(host))
> +		opp = dev_pm_opp_find_peak_bw_ceil(dev, &bw);
> +
> +	if (IS_ERR(opp)) {
> +		dev_err(dev, "Failed to find OPP for freq:%u err:%ld\n",
> +				freq, PTR_ERR(opp));
> +		return NULL;
> +	}
> +	return opp;
> +}
> +
> +/*
> + * This function sets the interconnect bus bandwidth
> + * vote based on bw (bandwidth) argument.
> + */
> +#define BUS_INTERCONNECT_PATHS 2 /* 1. sdhc -> ddr 2. cpu -> sdhc */
> +static void sdhci_msm_bus_set_vote(struct sdhci_host *host,
> +						unsigned int bw)
> +{
> +	int i;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	struct sdhci_msm_bus_vote_data *vote_data = msm_host->bus_vote_data;
> +	struct dev_pm_opp *opp;
> +	unsigned long freq = bw;
> +	unsigned long ib[BUS_INTERCONNECT_PATHS], ab[BUS_INTERCONNECT_PATHS];
> +	int ddr_rc = 0, cpu_rc = 0;
> +
> +	if (!msm_host->bus_vote_data->sdhc_to_ddr ||
> +			!msm_host->bus_vote_data->cpu_to_sdhc)

Why not include this check in the calling code, which already checks for
skip_bus_bw_voting?

> +		return;
> +
> +	if (bw != vote_data->curr_freq) {

if (bw == vote_data->curr_freq)
	return;

Will save you an indentation level in the rest of the function.

> +		for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
> +			opp = find_opp_for_freq(msm_host, freq);
> +			if (opp) {
> +				ab[i] =	dev_pm_opp_get_bw(opp, &ib[i]);
> +				freq += 1; /* Next Band width vote */

Why do you request bw for sdhc-ddr and then bw+1 for cpu-sdhc?

> +				dev_pm_opp_put(opp);
> +			}
> +		}
> +		pr_debug("%s: freq:%d sdhc_to_ddr ab:%lu ib:%lu cpu_to_sdhc ab:%lu ib:%lu\n",
> +				mmc_hostname(host->mmc), bw, ab[0], ib[0],
> +				ab[1], ib[1]);

You probably have compiler warnings here as not all code paths will get
here with ib and ab initialized.

> +		ddr_rc = icc_set_bw(vote_data->sdhc_to_ddr, 0, ib[0]);
> +		cpu_rc = icc_set_bw(vote_data->cpu_to_sdhc, 0, ib[1]);
> +		if (ddr_rc || cpu_rc) {
> +			pr_err("%s: icc_set() failed ddr_rc_err:%d cpu_rc_err:%d\n",

dev_err()

> +				mmc_hostname(host->mmc), ddr_rc, cpu_rc);
> +			return;
> +		}
> +		vote_data->curr_freq = bw;
> +	}
> +}
> +
> +/*
> + * This function registers the device to OPP framework and
> + * parses few optional parameters from the device tree node.
> + * Returns NULL bvd pointer on error else a valid bvd pointer.
> + */
> +static struct sdhci_msm_bus_vote_data *sdhci_msm_get_bus_vote_data(struct device
> +				       *dev, struct sdhci_msm_host *host)
> +
> +{
> +	struct platform_device *pdev = to_platform_device(dev);

Just pass the platform_device instead of passing &pdev->dev and then container_of() it directly.

> +	struct sdhci_msm_bus_vote_data *vote_data = NULL;
> +	int ret = 0;

No need to initialize either vote_data or ret, given that they are both
written before read below.

> +
> +	if (!pdev) {

How could this happen?

> +		dev_err(dev, "Null platform device!\n");
> +		return NULL;
> +	}
> +	vote_data = devm_kzalloc(dev, sizeof(*vote_data), GFP_KERNEL);
> +	if (!vote_data)
> +		return vote_data;
> +
> +	ret = dev_pm_opp_of_add_table(dev);
> +	if (ret) {
> +		if (ret == -ENODEV || ret == -ENODATA) {
> +			dev_dbg(dev, "OPP not found. Skip bus voting!!:%d\n",
> +					ret);
> +			vote_data->skip_bus_bw_voting = true;
> +		} else {
> +			dev_dbg(dev, "OPP regestration failed:%d\n", ret);
> +			goto err;
> +		}
> +	}
> +
> +	return vote_data;
> +err:
> +	devm_kfree(dev, vote_data);
> +	vote_data = NULL;

This is a local variable, no need to clear it.

> +	return vote_data;
> +}
> +
> +/*
> + * Helper function to register for OPP and interconnect
> + * frameworks.
> + */
> +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
> +				struct platform_device *pdev)
> +{
> +	struct sdhci_msm_bus_vote_data *vote_data;
> +	struct device *dev = &pdev->dev;
> +	int ret = 0;
> +
> +	vote_data = sdhci_msm_get_bus_vote_data(dev, host);
> +	if (!vote_data) {
> +		dev_dbg(&pdev->dev, "Failed to get bus_scale data\n");
> +		host->bus_vote_data = NULL;
> +		return -EINVAL;
> +	}
> +	host->bus_vote_data = vote_data;
> +
> +	vote_data->sdhc_to_ddr = of_icc_get(&pdev->dev, "sdhc-ddr");
> +	if (!vote_data->sdhc_to_ddr) {
> +		dev_dbg(&pdev->dev, "DT property for path %s missing\n",
> +				"sdhc-ddr");
> +		return -ENOENT;
> +	} else if (IS_ERR(vote_data->sdhc_to_ddr) {
> +		dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
> +				PTR_ERR(vote_data->sdhc_to_ddr), "sdhc-ddr");
> +		ret = PTR_ERR(vote_data->sdhc_to_ddr);
> +		vote_data->sdhc_to_ddr = NULL;
> +		return ret;
> +	}
> +	vote_data->cpu_to_sdhc = of_icc_get(&pdev->dev, "cpu-sdhc");
> +	if (!vote_data->cpu_to_sdhc) {

Afaict there's no reason to hold onto sdhc_to_ddr or vote_data itself if
this happens.

I think things would be cleaner if you make this function return the
vote_data for success, NULL if the information was omitted or ERR_PTR()
in case invalid data was given.

Assign this in the caller, check for IS_ERR() and in the places where
you want to know if you are doing bus voting or not you can just check
for host->bus_vote_data - no need to check sdhc_to_ddr, cpu_to_sdhc and
skip_bus_bw_voting individually..


And given that it's an both-or-nothing this function would be cleaner if
you of_icc_get() both paths, then do error handling - and then follow up
with the introduction of devm and bulk operations to the icc API.

> +		dev_dbg(&pdev->dev, "DT property for path %s missing\n",
> +					"cpu_to_sdhc");
> +		return -ENOENT;
> +	} else if (IS_ERR(vote_data->cpu_to_sdhc)) {
> +		dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
> +				PTR_ERR(vote_data->cpu_to_sdhc), "cpu-sdhc");
> +		ret = PTR_ERR(vote_data->cpu_to_sdhc);
> +		vote_data->cpu_to_sdhc = NULL;
> +		return ret;
> +	}
> +	return ret;
> +}
> +
> +static void sdhci_msm_bus_unregister(struct device *dev,
> +				struct sdhci_msm_host *host)
> +{
> +	struct sdhci_msm_bus_vote_data *vote_data = host->bus_vote_data;
> +
> +	if (vote_data->skip_bus_bw_voting ||
> +		!vote_data->sdhc_to_ddr ||
> +		!vote_data->cpu_to_sdhc)
> +		return;
> +
> +	icc_put(vote_data->sdhc_to_ddr);
> +	icc_put(vote_data->cpu_to_sdhc);
> +}
> +
> +#define MSM_MMC_BUS_VOTING_DELAY        200 /* msecs */

This is unused.

> +
> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)

Split this in an enable and a disable 

> +{
> +	struct mmc_ios *ios = &host->mmc->ios;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	unsigned int bw;
> +
> +	if (msm_host->bus_vote_data->skip_bus_bw_voting)
> +		return;
> +
> +	if (enable) {
> +		bw = sdhci_get_bw_required(host, ios);
> +		sdhci_msm_bus_set_vote(host, bw);
> +	} else
> +		sdhci_msm_bus_set_vote(host, 0);
> +}
> +
>  static const struct sdhci_msm_variant_ops mci_var_ops = {
>  	.msm_readl_relaxed = sdhci_msm_mci_variant_readl_relaxed,
>  	.msm_writel_relaxed = sdhci_msm_mci_variant_writel_relaxed,

Regards,
Bjorn

Re: [RFC v3 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support

[ppvk]

Hi Bjorn,

Thanks for the review !

On 2020-02-09 05:09, Bjorn Andersson wrote:
> On Fri 07 Feb 06:44 PST 2020, Pradeep P V K wrote:
> 
>> Add interconnect bandwidths for SDHC driver using OPP framework that
>> is required by SDHC driver based on the clock frequency and bus width
>> of the card. Otherwise, the system clocks may run at minimum clock
>> speed and thus affecting the performance.
>> 
>> This change is based on Georgi Djakov [RFC]
>> (https://lkml.org/lkml/2018/10/11/499) and
>> 
>> Saravana Kannan [PATCH v6]
>> (https://lkml.org/lkml/2019/12/6/740)
> 
> Write out the subject of these two patches, rather than stating that
> it's based on one RFC and some PATCH version 6. Or just state that it's
> based on work by Georgi and Saravana.
> 
Sure. i will update this in my next patch set.

>> 
>> change from RFC v2 -> v3 changes:
>> - Modified interconnect bandwidth support using OPP framework.
>> - Addressed review comments on v2.
> 
> Move the changelog below the --- line
> 
ok.

>> 
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>> Co-developed-by: Pradeep P V K <ppvk@codeaurora.org>
>> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
> 
> You should read these as the history of how this patch reached LKML and
> can use Co-developed-by to indicate that several people was part of a
> single step.
> 
> So if you're both co-author and the one sending it to LKML then you
> should have 4 Co-developed-by tags here.
> 
sure, i will follow this from my patch series.

>> ---
>>  drivers/mmc/host/sdhci-msm.c | 251 
>> ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 247 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/mmc/host/sdhci-msm.c 
>> b/drivers/mmc/host/sdhci-msm.c
>> index 71f29ba..f061cd8 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -11,8 +11,10 @@
>>  #include <linux/mmc/mmc.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/slab.h>
>> +#include <linux/interconnect.h>
>>  #include <linux/iopoll.h>
>>  #include <linux/regulator/consumer.h>
>> +#include <linux/pm_opp.h>
>> 
>>  #include "sdhci-pltfm.h"
>> 
>> @@ -229,6 +231,14 @@ struct sdhci_msm_variant_info {
>>  	const struct sdhci_msm_offset *offset;
>>  };
>> 
>> +struct sdhci_msm_bus_vote_data {
>> +
> 
> Move the { to the empty line.
> 
ok.
>> +	struct icc_path *sdhc_to_ddr;
>> +	struct icc_path *cpu_to_sdhc;
>> +	bool skip_bus_bw_voting;
>> +	u32 curr_freq;
>> +};
>> +
>>  struct sdhci_msm_host {
>>  	struct platform_device *pdev;
>>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
>> @@ -255,8 +265,11 @@ struct sdhci_msm_host {
>>  	bool use_cdr;
>>  	u32 transfer_mode;
>>  	bool updated_ddr_cfg;
>> +	struct sdhci_msm_bus_vote_data *bus_vote_data;
>>  };
>> 
>> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 
>> enable);
>> +
>>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct 
>> sdhci_host *host)
>>  {
>>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -1564,6 +1577,7 @@ static void sdhci_msm_set_clock(struct 
>> sdhci_host *host, unsigned int clock)
>> 
>>  	msm_set_clock_rate_for_bus_mode(host, clock);
>>  out:
>> +	sdhci_msm_bus_voting(host, !!clock);
>>  	__sdhci_msm_set_clock(host, clock);
>>  }
>> 
>> @@ -1685,6 +1699,219 @@ static void 
>> sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
>>  	pr_debug("%s: supported caps: 0x%08x\n", mmc_hostname(mmc), caps);
>>  }
>> 
>> +/*
>> + * Returns required bandwidth in Bytes per Sec
>> + */
>> +static unsigned long sdhci_get_bw_required(struct sdhci_host *host,
>> +					struct mmc_ios *ios)
>> +{
>> +	unsigned long bw;
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	bw = msm_host->clk_rate;
>> +
>> +	if (ios->bus_width == MMC_BUS_WIDTH_4)
>> +		bw /= 2;
>> +	else if (ios->bus_width == MMC_BUS_WIDTH_1)
>> +		bw /= 8;
>> +
>> +	return bw;
> 
> 
> switch (ios->bus_width) {
> case MMC_BUS_WIDTH_1:
> 	return msm_host->clk_rate / 8;
> case MMC_BUS_WIDTH_4:
> 	return msm_host->clk_rate / 2;
> case MMC_BUS_WIDTH_8:
> 	return msm_host->clk_rate;
> }
> 
ok. i will do this in my next patch.

>> +}
>> +
>> +/*
>> + * Helper function to parse the exact OPP node
>> + * Returns OPP pointer on success else NULL on error
>> + */
>> +static struct dev_pm_opp *find_opp_for_freq(struct sdhci_msm_host 
>> *msm_host,
> 
> A function with this name doesn't belong in the sdhci-msm driver.
> 
ok. i will update the function name accordingly.

>> +							unsigned long bw)
>> +{
>> +	struct dev_pm_opp *opp;
>> +	struct sdhci_host *host = mmc_priv(msm_host->mmc);
>> +	unsigned int freq = bw;
>> +	struct device *dev = &msm_host->pdev->dev;
>> +
>> +
>> +	if (!freq)
>> +		opp = dev_pm_opp_find_peak_bw_floor(dev, &freq);
>> +	else
>> +		opp = dev_pm_opp_find_peak_bw_exact(dev, freq, true);
>> +
>> +	/* Max Bandwidth vote */
>> +	if (PTR_ERR(opp) == -ERANGE && freq > sdhci_msm_get_max_clock(host))
>> +		opp = dev_pm_opp_find_peak_bw_ceil(dev, &bw);
>> +
>> +	if (IS_ERR(opp)) {
>> +		dev_err(dev, "Failed to find OPP for freq:%u err:%ld\n",
>> +				freq, PTR_ERR(opp));
>> +		return NULL;
>> +	}
>> +	return opp;
>> +}
>> +
>> +/*
>> + * This function sets the interconnect bus bandwidth
>> + * vote based on bw (bandwidth) argument.
>> + */
>> +#define BUS_INTERCONNECT_PATHS 2 /* 1. sdhc -> ddr 2. cpu -> sdhc */
>> +static void sdhci_msm_bus_set_vote(struct sdhci_host *host,
>> +						unsigned int bw)
>> +{
>> +	int i;
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	struct sdhci_msm_bus_vote_data *vote_data = msm_host->bus_vote_data;
>> +	struct dev_pm_opp *opp;
>> +	unsigned long freq = bw;
>> +	unsigned long ib[BUS_INTERCONNECT_PATHS], 
>> ab[BUS_INTERCONNECT_PATHS];
>> +	int ddr_rc = 0, cpu_rc = 0;
>> +
>> +	if (!msm_host->bus_vote_data->sdhc_to_ddr ||
>> +			!msm_host->bus_vote_data->cpu_to_sdhc)
> 
> Why not include this check in the calling code, which already checks 
> for
> skip_bus_bw_voting?
> 
This is actually redundant. This is not required here again. 
skip_bus_bw_voting will set when
the handlers are NULL. so i will remove this in my next patch set.

>> +		return;
>> +
>> +	if (bw != vote_data->curr_freq) {
> 
> if (bw == vote_data->curr_freq)
> 	return;
> 
> Will save you an indentation level in the rest of the function.
> 
sure. i will make the change accordingly.

>> +		for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
>> +			opp = find_opp_for_freq(msm_host, freq);
>> +			if (opp) {
>> +				ab[i] =	dev_pm_opp_get_bw(opp, &ib[i]);
>> +				freq += 1; /* Next Band width vote */
> 
> Why do you request bw for sdhc-ddr and then bw+1 for cpu-sdhc?
> 
for sdhci-msm, the OPP's for path sdhc-ddr and cpu-sdhc are same, to 
avoid
OPP errors and warnings, i just added +1 for cpu-sdhc to differentiate 
between
sdhc-ddr and cpu-sdhc OPPS. This needs to be taken care while adding 
opp's in dt
for cpu-sdhc path.


>> +				dev_pm_opp_put(opp);
>> +			}
>> +		}
>> +		pr_debug("%s: freq:%d sdhc_to_ddr ab:%lu ib:%lu cpu_to_sdhc ab:%lu 
>> ib:%lu\n",
>> +				mmc_hostname(host->mmc), bw, ab[0], ib[0],
>> +				ab[1], ib[1]);
> 
> You probably have compiler warnings here as not all code paths will get
> here with ib and ab initialized.
> 
True. i will make the change accordingly.

>> +		ddr_rc = icc_set_bw(vote_data->sdhc_to_ddr, 0, ib[0]);
>> +		cpu_rc = icc_set_bw(vote_data->cpu_to_sdhc, 0, ib[1]);
>> +		if (ddr_rc || cpu_rc) {
>> +			pr_err("%s: icc_set() failed ddr_rc_err:%d cpu_rc_err:%d\n",
> 
> dev_err()
> 
ok.

>> +				mmc_hostname(host->mmc), ddr_rc, cpu_rc);
>> +			return;
>> +		}
>> +		vote_data->curr_freq = bw;
>> +	}
>> +}
>> +
>> +/*
>> + * This function registers the device to OPP framework and
>> + * parses few optional parameters from the device tree node.
>> + * Returns NULL bvd pointer on error else a valid bvd pointer.
>> + */
>> +static struct sdhci_msm_bus_vote_data 
>> *sdhci_msm_get_bus_vote_data(struct device
>> +				       *dev, struct sdhci_msm_host *host)
>> +
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
> 
> Just pass the platform_device instead of passing &pdev->dev and then
> container_of() it directly.
> 
This function is not required here. i will move the necessary code to 
the caller fn().
to look the code more simpler. i will make the changes accordingly in my 
next patch series.

>> +	struct sdhci_msm_bus_vote_data *vote_data = NULL;
>> +	int ret = 0;
> 
> No need to initialize either vote_data or ret, given that they are both
> written before read below.
> 
same as above.

>> +
>> +	if (!pdev) {
> 
> How could this happen?
> 
same as above.
>> +		dev_err(dev, "Null platform device!\n");
>> +		return NULL;
>> +	}
>> +	vote_data = devm_kzalloc(dev, sizeof(*vote_data), GFP_KERNEL);
>> +	if (!vote_data)
>> +		return vote_data;
>> +
>> +	ret = dev_pm_opp_of_add_table(dev);
>> +	if (ret) {
>> +		if (ret == -ENODEV || ret == -ENODATA) {
>> +			dev_dbg(dev, "OPP not found. Skip bus voting!!:%d\n",
>> +					ret);
>> +			vote_data->skip_bus_bw_voting = true;
>> +		} else {
>> +			dev_dbg(dev, "OPP regestration failed:%d\n", ret);
>> +			goto err;
>> +		}
>> +	}
>> +
>> +	return vote_data;
>> +err:
>> +	devm_kfree(dev, vote_data);
>> +	vote_data = NULL;
> 
> This is a local variable, no need to clear it.
> 
same as above.
>> +	return vote_data;
>> +}
>> +
>> +/*
>> + * Helper function to register for OPP and interconnect
>> + * frameworks.
>> + */
>> +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
>> +				struct platform_device *pdev)
>> +{
>> +	struct sdhci_msm_bus_vote_data *vote_data;
>> +	struct device *dev = &pdev->dev;
>> +	int ret = 0;
>> +
>> +	vote_data = sdhci_msm_get_bus_vote_data(dev, host);
>> +	if (!vote_data) {
>> +		dev_dbg(&pdev->dev, "Failed to get bus_scale data\n");
>> +		host->bus_vote_data = NULL;
>> +		return -EINVAL;
>> +	}
>> +	host->bus_vote_data = vote_data;
>> +
>> +	vote_data->sdhc_to_ddr = of_icc_get(&pdev->dev, "sdhc-ddr");
>> +	if (!vote_data->sdhc_to_ddr) {
>> +		dev_dbg(&pdev->dev, "DT property for path %s missing\n",
>> +				"sdhc-ddr");
>> +		return -ENOENT;
>> +	} else if (IS_ERR(vote_data->sdhc_to_ddr) {
>> +		dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
>> +				PTR_ERR(vote_data->sdhc_to_ddr), "sdhc-ddr");
>> +		ret = PTR_ERR(vote_data->sdhc_to_ddr);
>> +		vote_data->sdhc_to_ddr = NULL;
>> +		return ret;
>> +	}
>> +	vote_data->cpu_to_sdhc = of_icc_get(&pdev->dev, "cpu-sdhc");
>> +	if (!vote_data->cpu_to_sdhc) {
> 
> Afaict there's no reason to hold onto sdhc_to_ddr or vote_data itself 
> if
> this happens.
> 
> I think things would be cleaner if you make this function return the
> vote_data for success, NULL if the information was omitted or ERR_PTR()
> in case invalid data was given.
> 
> Assign this in the caller, check for IS_ERR() and in the places where
> you want to know if you are doing bus voting or not you can just check
> for host->bus_vote_data - no need to check sdhc_to_ddr, cpu_to_sdhc and
> skip_bus_bw_voting individually..
> 
> 
> And given that it's an both-or-nothing this function would be cleaner 
> if
> you of_icc_get() both paths, then do error handling - and then follow 
> up
> with the introduction of devm and bulk operations to the icc API.
> 
Agree with your suggestions, will make this change in my next patch set.
>> +		dev_dbg(&pdev->dev, "DT property for path %s missing\n",
>> +					"cpu_to_sdhc");
>> +		return -ENOENT;
>> +	} else if (IS_ERR(vote_data->cpu_to_sdhc)) {
>> +		dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
>> +				PTR_ERR(vote_data->cpu_to_sdhc), "cpu-sdhc");
>> +		ret = PTR_ERR(vote_data->cpu_to_sdhc);
>> +		vote_data->cpu_to_sdhc = NULL;
>> +		return ret;
>> +	}
>> +	return ret;
>> +}
>> +
>> +static void sdhci_msm_bus_unregister(struct device *dev,
>> +				struct sdhci_msm_host *host)
>> +{
>> +	struct sdhci_msm_bus_vote_data *vote_data = host->bus_vote_data;
>> +
>> +	if (vote_data->skip_bus_bw_voting ||
>> +		!vote_data->sdhc_to_ddr ||
>> +		!vote_data->cpu_to_sdhc)
>> +		return;
>> +
>> +	icc_put(vote_data->sdhc_to_ddr);
>> +	icc_put(vote_data->cpu_to_sdhc);
>> +}
>> +
>> +#define MSM_MMC_BUS_VOTING_DELAY        200 /* msecs */
> 
> This is unused.
> 
ok. i will remove.
>> +
>> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
> 
> Split this in an enable and a disable
> 
Why to split into two functions, most of the code is same and would be 
simpler
to use with the same name for both enable and disable.
>> +{
>> +	struct mmc_ios *ios = &host->mmc->ios;
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	unsigned int bw;
>> +
>> +	if (msm_host->bus_vote_data->skip_bus_bw_voting)
>> +		return;
>> +
>> +	if (enable) {
>> +		bw = sdhci_get_bw_required(host, ios);
>> +		sdhci_msm_bus_set_vote(host, bw);
>> +	} else
>> +		sdhci_msm_bus_set_vote(host, 0);
>> +}
>> +
>>  static const struct sdhci_msm_variant_ops mci_var_ops = {
>>  	.msm_readl_relaxed = sdhci_msm_mci_variant_readl_relaxed,
>>  	.msm_writel_relaxed = sdhci_msm_mci_variant_writel_relaxed,
> 
> Regards,
> Bjorn

[RFC v3 2/2] dt-bindings: mmc: sdhci-msm: Add interconnect BW scaling strings

[Pradeep P V K]

Add interconnect bandwidth scaling supported strings for qcom-sdhci
controller.

changes from RFC v2 -> v3:
- Addressed review comments on v2.
- Modified interconnect bandwidth support using OPP framework.

Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
---
 Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index 7ee639b..cbe97b8 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -40,6 +40,21 @@ Required properties:
 	"cal"	- reference clock for RCLK delay calibration (optional)
 	"sleep"	- sleep clock for RCLK delay calibration (optional)
 
+Optional Properties:
+* Following bus parameters are required for interconnect bandwidth scaling:
+- interconnects: Pairs of phandles and interconnect provider specifier
+		 to denote the edge source and destination ports of
+		 the interconnect path.
+
+- interconnect-names: For sdhc, we have two main paths.
+		1. Data path : sdhc to ddr
+		2. Config path : cpu to sdhc
+		For Data interconnect path the name supposed to be
+		is "sdhc-ddr" and for config interconnect path it is
+		"cpu-sdhc".
+		Please refer to Documentation/devicetree/bindings/
+		interconnect/ for more details.
+
 Example:
 
 	sdhc_1: sdhci@f9824900 {
@@ -57,6 +72,9 @@ Example:
 
 		clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>;
 		clock-names = "core", "iface";
+		interconnects = <&qnoc MASTER_SDCC_ID &qnoc SLAVE_DDR_ID>,
+				<&qnoc MASTER_CPU_ID &qnoc SLAVE_SDCC_ID>;
+		interconnect-names = "sdhc-ddr","cpu-sdhc";
 	};
 
 	sdhc_2: sdhci@f98a4900 {
-- 
1.9.1