[RFC 0/2] Add Support for SDHC bus bandwidth voting

[RFC 0/2] Add Support for SDHC bus bandwidth voting

[Pradeep P V K]

Vote for the MSM bus bandwidth required by SDHC driver
based on the clock speed and bus width of the card. Otherwise,
the system clocks may run at minimum clock speed and
thus affecting the performance.

Adapt to the new ICB framework for bus bandwidth voting.

This requires the source/destination port ids.
Also this requires a tuple of values.

The tuple is for two different paths - from SDHC master
to BIMC slave. The other is from CPU master to SDHC slave.
This tuple consists of the average and peak bandwidth.

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

Pradeep P V K (2):
  mmc: sdhci-msm: Add support for bus bandwidth voting
  dt-bindings: mmc: sdhci-msm: Add Bus BW vote supported strings

 .../devicetree/bindings/mmc/sdhci-msm.txt          |  32 ++
 drivers/mmc/host/sdhci-msm.c                       | 393 ++++++++++++++++++++-
 2 files changed, 422 insertions(+), 3 deletions(-)

-- 
1.9.1

[RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting

[Pradeep P V K]

Vote for the MSM bus bandwidth 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)

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

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index b75c82d..71515ca 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -11,6 +11,7 @@
 #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>
 
@@ -122,6 +123,9 @@
 #define msm_host_writel(msm_host, val, host, offset) \
 	msm_host->var_ops->msm_writel_relaxed(val, host, offset)
 
+#define SDHC_DDR "sdhc-ddr"
+#define CPU_SDHC "cpu-sdhc"
+
 struct sdhci_msm_offset {
 	u32 core_hc_mode;
 	u32 core_mci_data_cnt;
@@ -228,6 +232,31 @@ struct sdhci_msm_variant_info {
 	const struct sdhci_msm_offset *offset;
 };
 
+struct msm_bus_vectors {
+	uint64_t ab;
+	uint64_t ib;
+};
+
+struct msm_bus_path {
+	unsigned int num_paths;
+	struct msm_bus_vectors *vec;
+};
+
+struct sdhci_msm_bus_vote_data {
+	const char *name;
+	unsigned int num_usecase;
+	struct msm_bus_path *usecase;
+
+	unsigned int *bw_vecs;
+	unsigned int bw_vecs_size;
+
+	struct icc_path *sdhc_ddr;
+	struct icc_path *cpu_sdhc;
+
+	uint32_t curr_vote;
+
+};
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -253,8 +282,13 @@ struct sdhci_msm_host {
 	const struct sdhci_msm_offset *offset;
 	bool use_cdr;
 	u32 transfer_mode;
+	bool skip_bus_bw_voting;
+	struct sdhci_msm_bus_vote_data *bus_vote_data;
+	struct delayed_work bus_vote_work;
 };
 
+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);
@@ -1557,6 +1591,8 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	msm_set_clock_rate_for_bus_mode(host, clock);
 out:
+	if (!msm_host->skip_bus_bw_voting)
+		sdhci_msm_bus_voting(host, !!clock);
 	__sdhci_msm_set_clock(host, clock);
 }
 
@@ -1678,6 +1714,341 @@ 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);
 }
 
+static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
+				 u32 **bw_vecs, int *len, u32 size)
+{
+	int ret = 0;
+	struct device_node *np = dev->of_node;
+	size_t sz;
+	u32 *arr = NULL;
+
+	if (!of_get_property(np, prop_name, len)) {
+		ret = -EINVAL;
+		goto out;
+	}
+	sz = *len = *len / sizeof(*arr);
+	if (sz <= 0 || (size > 0 && (sz > size))) {
+		dev_err(dev, "%s invalid size\n", prop_name);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
+	if (!arr) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = of_property_read_u32_array(np, prop_name, arr, sz);
+	if (ret < 0) {
+		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
+		goto out;
+	}
+	*bw_vecs = arr;
+out:
+	if (ret)
+		*len = 0;
+	return ret;
+}
+
+/* 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;
+}
+
+static int sdhci_msm_bus_get_vote_for_bw(struct sdhci_msm_host *host,
+					   unsigned int bw)
+{
+	struct sdhci_msm_bus_vote_data *bvd = host->bus_vote_data;
+
+	unsigned int *table = bvd->bw_vecs;
+	unsigned int size = bvd->bw_vecs_size;
+	int i;
+
+	for (i = 0; i < size; i++) {
+		if (bw <= table[i])
+			break;
+	}
+
+	if (i && (i == size))
+		i--;
+
+	return i;
+}
+
+/*
+ * This function must be called with host lock acquired.
+ * Caller of this function should also ensure that msm bus client
+ * handle is not null.
+ */
+static inline int sdhci_msm_bus_set_vote(struct sdhci_msm_host *msm_host,
+					     int vote,
+					     unsigned long *flags)
+{
+	struct sdhci_host *host =  platform_get_drvdata(msm_host->pdev);
+	struct sdhci_msm_bus_vote_data *bvd = msm_host->bus_vote_data;
+	struct msm_bus_path *usecase = bvd->usecase;
+	struct msm_bus_vectors *vec = usecase[vote].vec;
+	int ddr_rc = 0, cpu_rc = 0;
+
+	if (vote != bvd->curr_vote) {
+		spin_unlock_irqrestore(&host->lock, *flags);
+		pr_debug("%s: vote:%d sdhc_ddr ab:%llu ib:%llu cpu_sdhc ab:%llu ib:%llu\n",
+				mmc_hostname(host->mmc), vote, vec[0].ab,
+				vec[0].ib, vec[1].ab, vec[1].ib);
+		ddr_rc = icc_set_bw(bvd->sdhc_ddr, vec[0].ab, vec[0].ib);
+		cpu_rc = icc_set_bw(bvd->cpu_sdhc, vec[1].ab, vec[1].ib);
+		spin_lock_irqsave(&host->lock, *flags);
+		if (ddr_rc || cpu_rc) {
+			pr_err("%s: icc_set() failed\n",
+				mmc_hostname(host->mmc));
+			goto out;
+		}
+		bvd->curr_vote = vote;
+	}
+out:
+	return cpu_rc;
+}
+
+/*
+ * Internal work. Work to set 0 bandwidth for msm bus.
+ */
+static void sdhci_msm_bus_work(struct work_struct *work)
+{
+	struct sdhci_msm_host *msm_host;
+	struct sdhci_host *host;
+	unsigned long flags;
+
+	msm_host = container_of(work, struct sdhci_msm_host,
+				bus_vote_work.work);
+	host =  platform_get_drvdata(msm_host->pdev);
+
+	/* Check handle and return */
+	if (!msm_host->bus_vote_data->sdhc_ddr ||
+			!msm_host->bus_vote_data->cpu_sdhc)
+		return;
+	spin_lock_irqsave(&host->lock, flags);
+	/* don't vote for 0 bandwidth if any request is in progress */
+	if (!host->mmc->ongoing_mrq)
+		sdhci_msm_bus_set_vote(msm_host, 0, &flags);
+	else
+		pr_warn("Transfer in progress.Skipping bus voting to 0\n");
+	spin_unlock_irqrestore(&host->lock, flags);
+}
+
+/*
+ * This function cancels any scheduled delayed work and sets the bus
+ * vote based on bw (bandwidth) argument.
+ */
+static void sdhci_msm_bus_cancel_work_and_set_vote(struct sdhci_host *host,
+						unsigned int bw)
+{
+	int vote;
+	unsigned long flags;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+	cancel_delayed_work_sync(&msm_host->bus_vote_work);
+	spin_lock_irqsave(&host->lock, flags);
+	vote = sdhci_msm_bus_get_vote_for_bw(msm_host, bw);
+	sdhci_msm_bus_set_vote(msm_host, vote, &flags);
+	spin_unlock_irqrestore(&host->lock, flags);
+}
+
+
+#define MSM_MMC_BUS_VOTING_DELAY	200 /* msecs */
+#define VOTE_ZERO  0
+
+/* This function queues a work which will set the bandwidth requiement to 0 */
+static void sdhci_msm_bus_queue_work(struct sdhci_host *host)
+{
+	unsigned long flags;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+	spin_lock_irqsave(&host->lock, flags);
+	if (msm_host->bus_vote_data->curr_vote != VOTE_ZERO)
+		queue_delayed_work(system_wq,
+				   &msm_host->bus_vote_work,
+				   msecs_to_jiffies(MSM_MMC_BUS_VOTING_DELAY));
+	spin_unlock_irqrestore(&host->lock, flags);
+}
+
+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 device_node *of_node = dev->of_node;
+	struct sdhci_msm_bus_vote_data *bvd = NULL;
+	struct msm_bus_path *usecase = NULL;
+	int ret = 0, i = 0, j, k, num_paths, len;
+	const uint32_t *vec_arr = NULL;
+	bool mem_err = false;
+
+	if (!pdev) {
+		dev_err(dev, "Null platform device!\n");
+		return NULL;
+	}
+
+	bvd = devm_kzalloc(dev, sizeof(struct sdhci_msm_bus_vote_data),
+				GFP_KERNEL);
+	if (!bvd) {
+		ret = -ENOMEM;
+		dev_err(dev, "No sufficient memory!\n");
+		return bvd;
+	}
+	ret = sdhci_msm_dt_get_array(dev, "qcom,bus-bw-vectors-bps",
+				&bvd->bw_vecs, &bvd->bw_vecs_size, 0);
+	if (ret) {
+		dev_info(dev, "No dt property of bus bw. voting defined!\n");
+		dev_info(dev, "Skipping Bus BW voting now!!\n");
+		host->skip_bus_bw_voting = true;
+		if (ret != -EINVAL && ret != -ENOMEM)
+			goto free;
+		goto err;
+	}
+
+	ret = of_property_read_string(of_node, "qcom,msm-bus,name", &bvd->name);
+	if (ret) {
+		dev_err(dev, "Error: (%d) Bus name missing!\n", ret);
+		goto err;
+	}
+
+	ret = of_property_read_u32(of_node, "qcom,msm-bus,num-cases",
+		&bvd->num_usecase);
+	if (ret) {
+		dev_err(dev, "Error: num-usecases not found\n");
+		goto err;
+	}
+
+	usecase = devm_kzalloc(dev, (sizeof(struct msm_bus_path) *
+				   bvd->num_usecase), GFP_KERNEL);
+	if (!usecase)
+		goto err;
+
+	ret = of_property_read_u32(of_node, "qcom,msm-bus,num-paths",
+				   &num_paths);
+	if (ret) {
+		dev_err(dev, "Error: num_paths not found\n");
+		goto out;
+	}
+
+	vec_arr = of_get_property(of_node, "qcom,msm-bus,vectors-KBps", &len);
+	if (vec_arr == NULL) {
+		dev_err(dev, "Error: Vector array not found\n");
+		goto out;
+	}
+
+	for (i = 0; i < bvd->num_usecase; i++) {
+		usecase[i].num_paths = num_paths;
+		usecase[i].vec = devm_kzalloc(dev, num_paths *
+					      sizeof(struct msm_bus_vectors),
+					      GFP_KERNEL);
+		if (!usecase[i].vec) {
+			mem_err = true;
+			dev_err(dev, "Error: Failed to alloc mem for vectors\n");
+			goto out;
+		}
+		for (j = 0; j < num_paths; j++) {
+			int idx = ((i * num_paths) + j) * 2;
+
+			usecase[i].vec[j].ab = (uint64_t)
+				be32_to_cpu(vec_arr[idx]);
+			usecase[i].vec[j].ib = (uint64_t)
+				be32_to_cpu(vec_arr[idx + 1]);
+		}
+	}
+
+	bvd->usecase = usecase;
+	return bvd;
+out:
+	if (mem_err) {
+		for (k = i - 1; k >= 0; k--)
+			devm_kfree(dev, usecase[k].vec);
+	}
+	devm_kfree(dev, usecase);
+free:
+	devm_kfree(dev, bvd->bw_vecs);
+err:
+	devm_kfree(dev, bvd);
+	bvd = NULL;
+	return bvd;
+}
+
+static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
+				struct platform_device *pdev)
+{
+	struct sdhci_msm_bus_vote_data *bsd;
+	struct device *dev = &pdev->dev;
+
+	bsd = sdhci_msm_get_bus_vote_data(dev, host);
+	if (!bsd) {
+		dev_err(&pdev->dev, "Failed: getting bus_scale data\n");
+		return PTR_ERR(bsd);
+	}
+	host->bus_vote_data = bsd;
+
+	bsd->sdhc_ddr = of_icc_get(&pdev->dev, SDHC_DDR);
+	if (IS_ERR(bsd->sdhc_ddr)) {
+		dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
+			PTR_ERR(bsd->sdhc_ddr), SDHC_DDR);
+		return PTR_ERR(bsd->sdhc_ddr);
+	}
+
+	bsd->cpu_sdhc = of_icc_get(&pdev->dev, CPU_SDHC);
+	if (IS_ERR(bsd->cpu_sdhc)) {
+		dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
+			PTR_ERR(bsd->cpu_sdhc), CPU_SDHC);
+		return PTR_ERR(bsd->cpu_sdhc);
+	}
+
+	INIT_DELAYED_WORK(&host->bus_vote_work, sdhci_msm_bus_work);
+
+	return 0;
+}
+
+static void sdhci_msm_bus_unregister(struct device *dev,
+				struct sdhci_msm_host *host)
+{
+	struct sdhci_msm_bus_vote_data *bsd = host->bus_vote_data;
+	int i;
+
+	icc_put(bsd->sdhc_ddr);
+	icc_put(bsd->cpu_sdhc);
+
+	for (i = 0; i < bsd->num_usecase; i++)
+		devm_kfree(dev, bsd->usecase[i].vec);
+	devm_kfree(dev, bsd->usecase);
+	devm_kfree(dev, bsd->bw_vecs);
+	devm_kfree(dev, bsd);
+}
+
+static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
+{
+	struct mmc_ios *ios = &host->mmc->ios;
+	unsigned int bw;
+
+	bw = sdhci_get_bw_required(host, ios);
+	if (enable)
+		sdhci_msm_bus_cancel_work_and_set_vote(host, bw);
+	else
+		sdhci_msm_bus_queue_work(host);
+}
+
 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,
@@ -1839,6 +2210,13 @@ 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->skip_bus_bw_voting)
+		goto clk_disable;
+
+	if (!msm_host->skip_bus_bw_voting)
+		sdhci_msm_bus_voting(host, 1);
+
 	if (!msm_host->mci_removed) {
 		core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 		msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
@@ -1846,7 +2224,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 		if (IS_ERR(msm_host->core_mem)) {
 			ret = PTR_ERR(msm_host->core_mem);
-			goto clk_disable;
+			goto bus_unregister;
 		}
 	}
 
@@ -1918,7 +2296,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);
@@ -1931,7 +2309,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);
@@ -1956,6 +2334,11 @@ 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:
+	if (!msm_host->skip_bus_bw_voting) {
+		sdhci_msm_bus_cancel_work_and_set_vote(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);
@@ -1985,6 +2368,10 @@ 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);
+	if (!msm_host->skip_bus_bw_voting) {
+		sdhci_msm_bus_cancel_work_and_set_vote(host, 0);
+		sdhci_msm_bus_unregister(&pdev->dev, msm_host);
+	}
 	sdhci_pltfm_free(pdev);
 	return 0;
 }
-- 
1.9.1

Re: [RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting

[ppvk]

+Georgi Djakov

On 2019-09-06 18:17, Pradeep P V K wrote:
> Vote for the MSM bus bandwidth 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)
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 393 
> ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 390 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c 
> b/drivers/mmc/host/sdhci-msm.c
> index b75c82d..71515ca 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -11,6 +11,7 @@
>  #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>
> 
> @@ -122,6 +123,9 @@
>  #define msm_host_writel(msm_host, val, host, offset) \
>  	msm_host->var_ops->msm_writel_relaxed(val, host, offset)
> 
> +#define SDHC_DDR "sdhc-ddr"
> +#define CPU_SDHC "cpu-sdhc"
> +
>  struct sdhci_msm_offset {
>  	u32 core_hc_mode;
>  	u32 core_mci_data_cnt;
> @@ -228,6 +232,31 @@ struct sdhci_msm_variant_info {
>  	const struct sdhci_msm_offset *offset;
>  };
> 
> +struct msm_bus_vectors {
> +	uint64_t ab;
> +	uint64_t ib;
> +};
> +
> +struct msm_bus_path {
> +	unsigned int num_paths;
> +	struct msm_bus_vectors *vec;
> +};
> +
> +struct sdhci_msm_bus_vote_data {
> +	const char *name;
> +	unsigned int num_usecase;
> +	struct msm_bus_path *usecase;
> +
> +	unsigned int *bw_vecs;
> +	unsigned int bw_vecs_size;
> +
> +	struct icc_path *sdhc_ddr;
> +	struct icc_path *cpu_sdhc;
> +
> +	uint32_t curr_vote;
> +
> +};
> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -253,8 +282,13 @@ struct sdhci_msm_host {
>  	const struct sdhci_msm_offset *offset;
>  	bool use_cdr;
>  	u32 transfer_mode;
> +	bool skip_bus_bw_voting;
> +	struct sdhci_msm_bus_vote_data *bus_vote_data;
> +	struct delayed_work bus_vote_work;
>  };
> 
> +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);
> @@ -1557,6 +1591,8 @@ static void sdhci_msm_set_clock(struct
> sdhci_host *host, unsigned int clock)
> 
>  	msm_set_clock_rate_for_bus_mode(host, clock);
>  out:
> +	if (!msm_host->skip_bus_bw_voting)
> +		sdhci_msm_bus_voting(host, !!clock);
>  	__sdhci_msm_set_clock(host, clock);
>  }
> 
> @@ -1678,6 +1714,341 @@ 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);
>  }
> 
> +static int sdhci_msm_dt_get_array(struct device *dev, const char 
> *prop_name,
> +				 u32 **bw_vecs, int *len, u32 size)
> +{
> +	int ret = 0;
> +	struct device_node *np = dev->of_node;
> +	size_t sz;
> +	u32 *arr = NULL;
> +
> +	if (!of_get_property(np, prop_name, len)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	sz = *len = *len / sizeof(*arr);
> +	if (sz <= 0 || (size > 0 && (sz > size))) {
> +		dev_err(dev, "%s invalid size\n", prop_name);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> +	if (!arr) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
> +	if (ret < 0) {
> +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> +		goto out;
> +	}
> +	*bw_vecs = arr;
> +out:
> +	if (ret)
> +		*len = 0;
> +	return ret;
> +}
> +
> +/* 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;
> +}
> +
> +static int sdhci_msm_bus_get_vote_for_bw(struct sdhci_msm_host *host,
> +					   unsigned int bw)
> +{
> +	struct sdhci_msm_bus_vote_data *bvd = host->bus_vote_data;
> +
> +	unsigned int *table = bvd->bw_vecs;
> +	unsigned int size = bvd->bw_vecs_size;
> +	int i;
> +
> +	for (i = 0; i < size; i++) {
> +		if (bw <= table[i])
> +			break;
> +	}
> +
> +	if (i && (i == size))
> +		i--;
> +
> +	return i;
> +}
> +
> +/*
> + * This function must be called with host lock acquired.
> + * Caller of this function should also ensure that msm bus client
> + * handle is not null.
> + */
> +static inline int sdhci_msm_bus_set_vote(struct sdhci_msm_host 
> *msm_host,
> +					     int vote,
> +					     unsigned long *flags)
> +{
> +	struct sdhci_host *host =  platform_get_drvdata(msm_host->pdev);
> +	struct sdhci_msm_bus_vote_data *bvd = msm_host->bus_vote_data;
> +	struct msm_bus_path *usecase = bvd->usecase;
> +	struct msm_bus_vectors *vec = usecase[vote].vec;
> +	int ddr_rc = 0, cpu_rc = 0;
> +
> +	if (vote != bvd->curr_vote) {
> +		spin_unlock_irqrestore(&host->lock, *flags);
> +		pr_debug("%s: vote:%d sdhc_ddr ab:%llu ib:%llu cpu_sdhc ab:%llu 
> ib:%llu\n",
> +				mmc_hostname(host->mmc), vote, vec[0].ab,
> +				vec[0].ib, vec[1].ab, vec[1].ib);
> +		ddr_rc = icc_set_bw(bvd->sdhc_ddr, vec[0].ab, vec[0].ib);
> +		cpu_rc = icc_set_bw(bvd->cpu_sdhc, vec[1].ab, vec[1].ib);
> +		spin_lock_irqsave(&host->lock, *flags);
> +		if (ddr_rc || cpu_rc) {
> +			pr_err("%s: icc_set() failed\n",
> +				mmc_hostname(host->mmc));
> +			goto out;
> +		}
> +		bvd->curr_vote = vote;
> +	}
> +out:
> +	return cpu_rc;
> +}
> +
> +/*
> + * Internal work. Work to set 0 bandwidth for msm bus.
> + */
> +static void sdhci_msm_bus_work(struct work_struct *work)
> +{
> +	struct sdhci_msm_host *msm_host;
> +	struct sdhci_host *host;
> +	unsigned long flags;
> +
> +	msm_host = container_of(work, struct sdhci_msm_host,
> +				bus_vote_work.work);
> +	host =  platform_get_drvdata(msm_host->pdev);
> +
> +	/* Check handle and return */
> +	if (!msm_host->bus_vote_data->sdhc_ddr ||
> +			!msm_host->bus_vote_data->cpu_sdhc)
> +		return;
> +	spin_lock_irqsave(&host->lock, flags);
> +	/* don't vote for 0 bandwidth if any request is in progress */
> +	if (!host->mmc->ongoing_mrq)
> +		sdhci_msm_bus_set_vote(msm_host, 0, &flags);
> +	else
> +		pr_warn("Transfer in progress.Skipping bus voting to 0\n");
> +	spin_unlock_irqrestore(&host->lock, flags);
> +}
> +
> +/*
> + * This function cancels any scheduled delayed work and sets the bus
> + * vote based on bw (bandwidth) argument.
> + */
> +static void sdhci_msm_bus_cancel_work_and_set_vote(struct sdhci_host 
> *host,
> +						unsigned int bw)
> +{
> +	int vote;
> +	unsigned long flags;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	cancel_delayed_work_sync(&msm_host->bus_vote_work);
> +	spin_lock_irqsave(&host->lock, flags);
> +	vote = sdhci_msm_bus_get_vote_for_bw(msm_host, bw);
> +	sdhci_msm_bus_set_vote(msm_host, vote, &flags);
> +	spin_unlock_irqrestore(&host->lock, flags);
> +}
> +
> +
> +#define MSM_MMC_BUS_VOTING_DELAY	200 /* msecs */
> +#define VOTE_ZERO  0
> +
> +/* This function queues a work which will set the bandwidth requiement 
> to 0 */
> +static void sdhci_msm_bus_queue_work(struct sdhci_host *host)
> +{
> +	unsigned long flags;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	spin_lock_irqsave(&host->lock, flags);
> +	if (msm_host->bus_vote_data->curr_vote != VOTE_ZERO)
> +		queue_delayed_work(system_wq,
> +				   &msm_host->bus_vote_work,
> +				   msecs_to_jiffies(MSM_MMC_BUS_VOTING_DELAY));
> +	spin_unlock_irqrestore(&host->lock, flags);
> +}
> +
> +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 device_node *of_node = dev->of_node;
> +	struct sdhci_msm_bus_vote_data *bvd = NULL;
> +	struct msm_bus_path *usecase = NULL;
> +	int ret = 0, i = 0, j, k, num_paths, len;
> +	const uint32_t *vec_arr = NULL;
> +	bool mem_err = false;
> +
> +	if (!pdev) {
> +		dev_err(dev, "Null platform device!\n");
> +		return NULL;
> +	}
> +
> +	bvd = devm_kzalloc(dev, sizeof(struct sdhci_msm_bus_vote_data),
> +				GFP_KERNEL);
> +	if (!bvd) {
> +		ret = -ENOMEM;
> +		dev_err(dev, "No sufficient memory!\n");
> +		return bvd;
> +	}
> +	ret = sdhci_msm_dt_get_array(dev, "qcom,bus-bw-vectors-bps",
> +				&bvd->bw_vecs, &bvd->bw_vecs_size, 0);
> +	if (ret) {
> +		dev_info(dev, "No dt property of bus bw. voting defined!\n");
> +		dev_info(dev, "Skipping Bus BW voting now!!\n");
> +		host->skip_bus_bw_voting = true;
> +		if (ret != -EINVAL && ret != -ENOMEM)
> +			goto free;
> +		goto err;
> +	}
> +
> +	ret = of_property_read_string(of_node, "qcom,msm-bus,name", 
> &bvd->name);
> +	if (ret) {
> +		dev_err(dev, "Error: (%d) Bus name missing!\n", ret);
> +		goto err;
> +	}
> +
> +	ret = of_property_read_u32(of_node, "qcom,msm-bus,num-cases",
> +		&bvd->num_usecase);
> +	if (ret) {
> +		dev_err(dev, "Error: num-usecases not found\n");
> +		goto err;
> +	}
> +
> +	usecase = devm_kzalloc(dev, (sizeof(struct msm_bus_path) *
> +				   bvd->num_usecase), GFP_KERNEL);
> +	if (!usecase)
> +		goto err;
> +
> +	ret = of_property_read_u32(of_node, "qcom,msm-bus,num-paths",
> +				   &num_paths);
> +	if (ret) {
> +		dev_err(dev, "Error: num_paths not found\n");
> +		goto out;
> +	}
> +
> +	vec_arr = of_get_property(of_node, "qcom,msm-bus,vectors-KBps", 
> &len);
> +	if (vec_arr == NULL) {
> +		dev_err(dev, "Error: Vector array not found\n");
> +		goto out;
> +	}
> +
> +	for (i = 0; i < bvd->num_usecase; i++) {
> +		usecase[i].num_paths = num_paths;
> +		usecase[i].vec = devm_kzalloc(dev, num_paths *
> +					      sizeof(struct msm_bus_vectors),
> +					      GFP_KERNEL);
> +		if (!usecase[i].vec) {
> +			mem_err = true;
> +			dev_err(dev, "Error: Failed to alloc mem for vectors\n");
> +			goto out;
> +		}
> +		for (j = 0; j < num_paths; j++) {
> +			int idx = ((i * num_paths) + j) * 2;
> +
> +			usecase[i].vec[j].ab = (uint64_t)
> +				be32_to_cpu(vec_arr[idx]);
> +			usecase[i].vec[j].ib = (uint64_t)
> +				be32_to_cpu(vec_arr[idx + 1]);
> +		}
> +	}
> +
> +	bvd->usecase = usecase;
> +	return bvd;
> +out:
> +	if (mem_err) {
> +		for (k = i - 1; k >= 0; k--)
> +			devm_kfree(dev, usecase[k].vec);
> +	}
> +	devm_kfree(dev, usecase);
> +free:
> +	devm_kfree(dev, bvd->bw_vecs);
> +err:
> +	devm_kfree(dev, bvd);
> +	bvd = NULL;
> +	return bvd;
> +}
> +
> +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
> +				struct platform_device *pdev)
> +{
> +	struct sdhci_msm_bus_vote_data *bsd;
> +	struct device *dev = &pdev->dev;
> +
> +	bsd = sdhci_msm_get_bus_vote_data(dev, host);
> +	if (!bsd) {
> +		dev_err(&pdev->dev, "Failed: getting bus_scale data\n");
> +		return PTR_ERR(bsd);
> +	}
> +	host->bus_vote_data = bsd;
> +
> +	bsd->sdhc_ddr = of_icc_get(&pdev->dev, SDHC_DDR);
> +	if (IS_ERR(bsd->sdhc_ddr)) {
> +		dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
> +			PTR_ERR(bsd->sdhc_ddr), SDHC_DDR);
> +		return PTR_ERR(bsd->sdhc_ddr);
> +	}
> +
> +	bsd->cpu_sdhc = of_icc_get(&pdev->dev, CPU_SDHC);
> +	if (IS_ERR(bsd->cpu_sdhc)) {
> +		dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
> +			PTR_ERR(bsd->cpu_sdhc), CPU_SDHC);
> +		return PTR_ERR(bsd->cpu_sdhc);
> +	}
> +
> +	INIT_DELAYED_WORK(&host->bus_vote_work, sdhci_msm_bus_work);
> +
> +	return 0;
> +}
> +
> +static void sdhci_msm_bus_unregister(struct device *dev,
> +				struct sdhci_msm_host *host)
> +{
> +	struct sdhci_msm_bus_vote_data *bsd = host->bus_vote_data;
> +	int i;
> +
> +	icc_put(bsd->sdhc_ddr);
> +	icc_put(bsd->cpu_sdhc);
> +
> +	for (i = 0; i < bsd->num_usecase; i++)
> +		devm_kfree(dev, bsd->usecase[i].vec);
> +	devm_kfree(dev, bsd->usecase);
> +	devm_kfree(dev, bsd->bw_vecs);
> +	devm_kfree(dev, bsd);
> +}
> +
> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
> +{
> +	struct mmc_ios *ios = &host->mmc->ios;
> +	unsigned int bw;
> +
> +	bw = sdhci_get_bw_required(host, ios);
> +	if (enable)
> +		sdhci_msm_bus_cancel_work_and_set_vote(host, bw);
> +	else
> +		sdhci_msm_bus_queue_work(host);
> +}
> +
>  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,
> @@ -1839,6 +2210,13 @@ 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->skip_bus_bw_voting)
> +		goto clk_disable;
> +
> +	if (!msm_host->skip_bus_bw_voting)
> +		sdhci_msm_bus_voting(host, 1);
> +
>  	if (!msm_host->mci_removed) {
>  		core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>  		msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
> @@ -1846,7 +2224,7 @@ static int sdhci_msm_probe(struct platform_device 
> *pdev)
> 
>  		if (IS_ERR(msm_host->core_mem)) {
>  			ret = PTR_ERR(msm_host->core_mem);
> -			goto clk_disable;
> +			goto bus_unregister;
>  		}
>  	}
> 
> @@ -1918,7 +2296,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);
> @@ -1931,7 +2309,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);
> @@ -1956,6 +2334,11 @@ 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:
> +	if (!msm_host->skip_bus_bw_voting) {
> +		sdhci_msm_bus_cancel_work_and_set_vote(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);
> @@ -1985,6 +2368,10 @@ 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);
> +	if (!msm_host->skip_bus_bw_voting) {
> +		sdhci_msm_bus_cancel_work_and_set_vote(host, 0);
> +		sdhci_msm_bus_unregister(&pdev->dev, msm_host);
> +	}
>  	sdhci_pltfm_free(pdev);
>  	return 0;
>  }

Re: [RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting

[Stephen Boyd]

Quoting ppvk@codeaurora.org (2019-09-06 05:51:54)
> +Georgi Djakov
> 
> On 2019-09-06 18:17, Pradeep P V K wrote:
> > diff --git a/drivers/mmc/host/sdhci-msm.c 
> > b/drivers/mmc/host/sdhci-msm.c
> > index b75c82d..71515ca 100644
> > --- a/drivers/mmc/host/sdhci-msm.c
> > +++ b/drivers/mmc/host/sdhci-msm.c
> >  #define msm_host_writel(msm_host, val, host, offset) \
> >       msm_host->var_ops->msm_writel_relaxed(val, host, offset)
> > 
> > +#define SDHC_DDR "sdhc-ddr"
> > +#define CPU_SDHC "cpu-sdhc"

Do you really need these defines? They're not used more than once or
twice and just seem to make the code harder to read.

> > +
> >  struct sdhci_msm_offset {
> >       u32 core_hc_mode;
> >       u32 core_mci_data_cnt;
> > @@ -228,6 +232,31 @@ struct sdhci_msm_variant_info {
> >       const struct sdhci_msm_offset *offset;
> >  };
> > 
> > +struct msm_bus_vectors {
> > +     uint64_t ab;
> > +     uint64_t ib;
> > +};
> > +
> > +struct msm_bus_path {
> > +     unsigned int num_paths;
> > +     struct msm_bus_vectors *vec;
> > +};
> > +
> > +struct sdhci_msm_bus_vote_data {
> > +     const char *name;
> > +     unsigned int num_usecase;
> > +     struct msm_bus_path *usecase;
> > +
> > +     unsigned int *bw_vecs;
> > +     unsigned int bw_vecs_size;
> > +
> > +     struct icc_path *sdhc_ddr;
> > +     struct icc_path *cpu_sdhc;
> > +
> > +     uint32_t curr_vote;
> > +

Please use u32 instead of uint32_t. Same comment for u64.

> > +};
> > +
> >  struct sdhci_msm_host {
> >       struct platform_device *pdev;
> >       void __iomem *core_mem; /* MSM SDCC mapped address */
> > @@ -1678,6 +1714,341 @@ 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);
> >  }
> > 
> > +static int sdhci_msm_dt_get_array(struct device *dev, const char 
> > *prop_name,
> > +                              u32 **bw_vecs, int *len, u32 size)
> > +{
> > +     int ret = 0;
> > +     struct device_node *np = dev->of_node;
> > +     size_t sz;
> > +     u32 *arr = NULL;
> > +
> > +     if (!of_get_property(np, prop_name, len)) {
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +     sz = *len = *len / sizeof(*arr);
> > +     if (sz <= 0 || (size > 0 && (sz > size))) {
> > +             dev_err(dev, "%s invalid size\n", prop_name);
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> > +     if (!arr) {
> > +             ret = -ENOMEM;
> > +             goto out;
> > +     }
> > +
> > +     ret = of_property_read_u32_array(np, prop_name, arr, sz);
> > +     if (ret < 0) {
> > +             dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> > +             goto out;
> > +     }
> > +     *bw_vecs = arr;
> > +out:
> > +     if (ret)
> > +             *len = 0;
> > +     return ret;
> > +}
> > +
> > +/* 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;
> > +}
> > +
> > +static int sdhci_msm_bus_get_vote_for_bw(struct sdhci_msm_host *host,
> > +                                        unsigned int bw)
> > +{
> > +     struct sdhci_msm_bus_vote_data *bvd = host->bus_vote_data;
> > +
> > +     unsigned int *table = bvd->bw_vecs;

Should probably be a const bw_vecs pointer so that it can't be modified
after the fact.

> > +     unsigned int size = bvd->bw_vecs_size;
> > +     int i;
> > +
> > +     for (i = 0; i < size; i++) {
> > +             if (bw <= table[i])
> > +                     break;

return i;

> > +     }
> > +

return i - 1;

> > +     if (i && (i == size))
> > +             i--;
> > +
> > +     return i;

And then this is useless.....

> > +}
> > +
> > +/*
> > + * This function must be called with host lock acquired.
> > + * Caller of this function should also ensure that msm bus client
> > + * handle is not null.

If it was NULL it would be pretty sad.

> > + */
> > +static inline int sdhci_msm_bus_set_vote(struct sdhci_msm_host 
> > *msm_host,
> > +                                          int vote,
> > +                                          unsigned long *flags)
> > +{
> > +     struct sdhci_host *host =  platform_get_drvdata(msm_host->pdev);
> > +     struct sdhci_msm_bus_vote_data *bvd = msm_host->bus_vote_data;
> > +     struct msm_bus_path *usecase = bvd->usecase;
> > +     struct msm_bus_vectors *vec = usecase[vote].vec;
> > +     int ddr_rc = 0, cpu_rc = 0;

Why initialize to 0?

> > +
> > +     if (vote != bvd->curr_vote) {
> > +             spin_unlock_irqrestore(&host->lock, *flags);
> > +             pr_debug("%s: vote:%d sdhc_ddr ab:%llu ib:%llu cpu_sdhc ab:%llu 
> > ib:%llu\n",
> > +                             mmc_hostname(host->mmc), vote, vec[0].ab,
> > +                             vec[0].ib, vec[1].ab, vec[1].ib);
> > +             ddr_rc = icc_set_bw(bvd->sdhc_ddr, vec[0].ab, vec[0].ib);
> > +             cpu_rc = icc_set_bw(bvd->cpu_sdhc, vec[1].ab, vec[1].ib);
> > +             spin_lock_irqsave(&host->lock, *flags);

This is some tricky spin-lockery.

> > +             if (ddr_rc || cpu_rc) {
> > +                     pr_err("%s: icc_set() failed\n",
> > +                             mmc_hostname(host->mmc));
> > +                     goto out;
> > +             }
> > +             bvd->curr_vote = vote;
> > +     }
> > +out:
> > +     return cpu_rc;
> > +}
> > +
> > +/*
> > + * Internal work. Work to set 0 bandwidth for msm bus.
> > + */
> > +static void sdhci_msm_bus_work(struct work_struct *work)
> > +{
> > +     struct sdhci_msm_host *msm_host;
> > +     struct sdhci_host *host;
> > +     unsigned long flags;
> > +
> > +     msm_host = container_of(work, struct sdhci_msm_host,
> > +                             bus_vote_work.work);
> > +     host =  platform_get_drvdata(msm_host->pdev);
> > +
> > +     /* Check handle and return */

This comment is useless, please remove it.

> > +     if (!msm_host->bus_vote_data->sdhc_ddr ||
> > +                     !msm_host->bus_vote_data->cpu_sdhc)
> > +             return;
> > +     spin_lock_irqsave(&host->lock, flags);
> > +     /* don't vote for 0 bandwidth if any request is in progress */
> > +     if (!host->mmc->ongoing_mrq)
> > +             sdhci_msm_bus_set_vote(msm_host, 0, &flags);
> > +     else
> > +             pr_warn("Transfer in progress.Skipping bus voting to 0\n");

Missing space after the full stop. Also, useless warning so just remove
it?

> > +     spin_unlock_irqrestore(&host->lock, flags);
> > +}
> > +
> > +/*
> > + * This function cancels any scheduled delayed work and sets the bus
> > + * vote based on bw (bandwidth) argument.
> > + */
> > +static void sdhci_msm_bus_cancel_work_and_set_vote(struct sdhci_host 
> > *host,
> > +                                             unsigned int bw)
> > +{
> > +     int vote;
> > +     unsigned long flags;
> > +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +     struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > +
> > +     cancel_delayed_work_sync(&msm_host->bus_vote_work);
> > +     spin_lock_irqsave(&host->lock, flags);

Why does the lock need to be held? Is the interconnect framework not
consistent with itself?

> > +     vote = sdhci_msm_bus_get_vote_for_bw(msm_host, bw);
> > +     sdhci_msm_bus_set_vote(msm_host, vote, &flags);
> > +     spin_unlock_irqrestore(&host->lock, flags);
> > +}
> > +
> > +
> > +#define MSM_MMC_BUS_VOTING_DELAY     200 /* msecs */
> > +#define VOTE_ZERO  0
> > +
> > +/* This function queues a work which will set the bandwidth requiement 
> > to 0 */

This comment style is wrong. Also s/requiement/requirement/

> > +static void sdhci_msm_bus_queue_work(struct sdhci_host *host)
> > +{
> > +     unsigned long flags;
> > +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +     struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> > +
> > +     spin_lock_irqsave(&host->lock, flags);
> > +     if (msm_host->bus_vote_data->curr_vote != VOTE_ZERO)
> > +             queue_delayed_work(system_wq,
> > +                                &msm_host->bus_vote_work,
> > +                                msecs_to_jiffies(MSM_MMC_BUS_VOTING_DELAY));
> > +     spin_unlock_irqrestore(&host->lock, flags);

Ok it seems that the sdhci code isn't consistent with itself?

> > +}
> > +
> > +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 device_node *of_node = dev->of_node;
> > +     struct sdhci_msm_bus_vote_data *bvd = NULL;
> > +     struct msm_bus_path *usecase = NULL;
> > +     int ret = 0, i = 0, j, k, num_paths, len;
> > +     const uint32_t *vec_arr = NULL;
> > +     bool mem_err = false;
> > +
> > +     if (!pdev) {
> > +             dev_err(dev, "Null platform device!\n");
> > +             return NULL;
> > +     }
> > +
> > +     bvd = devm_kzalloc(dev, sizeof(struct sdhci_msm_bus_vote_data),

sizeof(*bvd) is better and shorter!

> > +                             GFP_KERNEL);
> > +     if (!bvd) {
> > +             ret = -ENOMEM;
> > +             dev_err(dev, "No sufficient memory!\n");

Don't print errors for allocation failures

> > +             return bvd;
> > +     }
> > +     ret = sdhci_msm_dt_get_array(dev, "qcom,bus-bw-vectors-bps",
> > +                             &bvd->bw_vecs, &bvd->bw_vecs_size, 0);
> > +     if (ret) {
> > +             dev_info(dev, "No dt property of bus bw. voting defined!\n");
> > +             dev_info(dev, "Skipping Bus BW voting now!!\n");

Is this debug junk? Why does the user care?

> > +             host->skip_bus_bw_voting = true;
> > +             if (ret != -EINVAL && ret != -ENOMEM)
> > +                     goto free;
> > +             goto err;
> > +     }
> > +
> > +     ret = of_property_read_string(of_node, "qcom,msm-bus,name", 
> > &bvd->name);
> > +     if (ret) {
> > +             dev_err(dev, "Error: (%d) Bus name missing!\n", ret);
> > +             goto err;
> > +     }
> > +
> > +     ret = of_property_read_u32(of_node, "qcom,msm-bus,num-cases",
> > +             &bvd->num_usecase);
> > +     if (ret) {
> > +             dev_err(dev, "Error: num-usecases not found\n");
> > +             goto err;
> > +     }
> > +
> > +     usecase = devm_kzalloc(dev, (sizeof(struct msm_bus_path) *
> > +                                bvd->num_usecase), GFP_KERNEL);
> > +     if (!usecase)
> > +             goto err;
> > +
> > +     ret = of_property_read_u32(of_node, "qcom,msm-bus,num-paths",
> > +                                &num_paths);
> > +     if (ret) {
> > +             dev_err(dev, "Error: num_paths not found\n");
> > +             goto out;
> > +     }
> > +
> > +     vec_arr = of_get_property(of_node, "qcom,msm-bus,vectors-KBps", 

Why are all the properties qcom specific?

> > &len);
> > +     if (vec_arr == NULL) {

A more consistent style is if (!vec_arr)

> > +             dev_err(dev, "Error: Vector array not found\n");
> > +             goto out;
> > +     }
> > +
> > +     for (i = 0; i < bvd->num_usecase; i++) {
> > +             usecase[i].num_paths = num_paths;
> > +             usecase[i].vec = devm_kzalloc(dev, num_paths *

Use devm_kcalloc().

> > +                                           sizeof(struct msm_bus_vectors),
> > +                                           GFP_KERNEL);
> > +             if (!usecase[i].vec) {
> > +                     mem_err = true;
> > +                     dev_err(dev, "Error: Failed to alloc mem for vectors\n");
> > +                     goto out;
> > +             }
> > +             for (j = 0; j < num_paths; j++) {
> > +                     int idx = ((i * num_paths) + j) * 2;
> > +
> > +                     usecase[i].vec[j].ab = (uint64_t)
> > +                             be32_to_cpu(vec_arr[idx]);
> > +                     usecase[i].vec[j].ib = (uint64_t)
> > +                             be32_to_cpu(vec_arr[idx + 1]);
> > +             }
> > +     }
> > +
> > +     bvd->usecase = usecase;
> > +     return bvd;
> > +out:
> > +     if (mem_err) {

Should be possible to not have this flag.

> > +             for (k = i - 1; k >= 0; k--)
> > +                     devm_kfree(dev, usecase[k].vec);
> > +     }
> > +     devm_kfree(dev, usecase);
> > +free:
> > +     devm_kfree(dev, bvd->bw_vecs);
> > +err:
> > +     devm_kfree(dev, bvd);

You don't need to devm_kfree() anything, just let probe fail it.

> > +     bvd = NULL;
> > +     return bvd;
> > +}
> > +
> > +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
> > +                             struct platform_device *pdev)
> > +{
> > +     struct sdhci_msm_bus_vote_data *bsd;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     bsd = sdhci_msm_get_bus_vote_data(dev, host);
> > +     if (!bsd) {
> > +             dev_err(&pdev->dev, "Failed: getting bus_scale data\n");
> > +             return PTR_ERR(bsd);
> > +     }
> > +     host->bus_vote_data = bsd;
> > +
> > +     bsd->sdhc_ddr = of_icc_get(&pdev->dev, SDHC_DDR);
> > +     if (IS_ERR(bsd->sdhc_ddr)) {
> > +             dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",

We don't need "Error: " prefix. That's what the kernel log level is for.

> > +                     PTR_ERR(bsd->sdhc_ddr), SDHC_DDR);
> > +             return PTR_ERR(bsd->sdhc_ddr);
> > +     }
> > +
> > +     bsd->cpu_sdhc = of_icc_get(&pdev->dev, CPU_SDHC);
> > +     if (IS_ERR(bsd->cpu_sdhc)) {
> > +             dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
> > +                     PTR_ERR(bsd->cpu_sdhc), CPU_SDHC);
> > +             return PTR_ERR(bsd->cpu_sdhc);
> > +     }
> > +
> > +     INIT_DELAYED_WORK(&host->bus_vote_work, sdhci_msm_bus_work);

Why is it in a workqueue context? Is there any reason it can' be done in
whatever calling context it is?

> > +
> > +     return 0;
> > +}
> > +
> > +static void sdhci_msm_bus_unregister(struct device *dev,
> > +                             struct sdhci_msm_host *host)
> > +{
> > +     struct sdhci_msm_bus_vote_data *bsd = host->bus_vote_data;
> > +     int i;
> > +
> > +     icc_put(bsd->sdhc_ddr);
> > +     icc_put(bsd->cpu_sdhc);

Is there a devm_icc_get() API? If not, please add it and use it.

> > +
> > +     for (i = 0; i < bsd->num_usecase; i++)
> > +             devm_kfree(dev, bsd->usecase[i].vec);
> > +     devm_kfree(dev, bsd->usecase);
> > +     devm_kfree(dev, bsd->bw_vecs);
> > +     devm_kfree(dev, bsd);

Again, not sure we need any devm_kfree() stuff.

> > +}
> > +
> > +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
> > +{
> > +     struct mmc_ios *ios = &host->mmc->ios;
> > +     unsigned int bw;
> > +
> > +     bw = sdhci_get_bw_required(host, ios);
> > +     if (enable)
> > +             sdhci_msm_bus_cancel_work_and_set_vote(host, bw);
> > +     else
> > +             sdhci_msm_bus_queue_work(host);
> > +}
> > +
> >  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,
> > @@ -1839,6 +2210,13 @@ 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->skip_bus_bw_voting)

Can this be a check for a NULL icc handle instead of the bool?

Re: [RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting

[ppvk]

On 2019-09-07 02:32, Stephen Boyd wrote:
> Quoting ppvk@codeaurora.org (2019-09-06 05:51:54)
>> +Georgi Djakov
>> 
>> On 2019-09-06 18:17, Pradeep P V K wrote:
>> > diff --git a/drivers/mmc/host/sdhci-msm.c
>> > b/drivers/mmc/host/sdhci-msm.c
>> > index b75c82d..71515ca 100644
>> > --- a/drivers/mmc/host/sdhci-msm.c
>> > +++ b/drivers/mmc/host/sdhci-msm.c
>> >  #define msm_host_writel(msm_host, val, host, offset) \
>> >       msm_host->var_ops->msm_writel_relaxed(val, host, offset)
>> >
>> > +#define SDHC_DDR "sdhc-ddr"
>> > +#define CPU_SDHC "cpu-sdhc"
> 
> Do you really need these defines? They're not used more than once or
> twice and just seem to make the code harder to read.
> 
Agree and will address this in my next patch set.

>> > +
>> >  struct sdhci_msm_offset {
>> >       u32 core_hc_mode;
>> >       u32 core_mci_data_cnt;
>> > @@ -228,6 +232,31 @@ struct sdhci_msm_variant_info {
>> >       const struct sdhci_msm_offset *offset;
>> >  };
>> >
>> > +struct msm_bus_vectors {
>> > +     uint64_t ab;
>> > +     uint64_t ib;
>> > +};
>> > +
>> > +struct msm_bus_path {
>> > +     unsigned int num_paths;
>> > +     struct msm_bus_vectors *vec;
>> > +};
>> > +
>> > +struct sdhci_msm_bus_vote_data {
>> > +     const char *name;
>> > +     unsigned int num_usecase;
>> > +     struct msm_bus_path *usecase;
>> > +
>> > +     unsigned int *bw_vecs;
>> > +     unsigned int bw_vecs_size;
>> > +
>> > +     struct icc_path *sdhc_ddr;
>> > +     struct icc_path *cpu_sdhc;
>> > +
>> > +     uint32_t curr_vote;
>> > +
> 
> Please use u32 instead of uint32_t. Same comment for u64.

Ok. I will address this in my next patch set.

> 
>> > +};
>> > +
>> >  struct sdhci_msm_host {
>> >       struct platform_device *pdev;
>> >       void __iomem *core_mem; /* MSM SDCC mapped address */
>> > @@ -1678,6 +1714,341 @@ 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);
>> >  }
>> >
>> > +static int sdhci_msm_dt_get_array(struct device *dev, const char
>> > *prop_name,
>> > +                              u32 **bw_vecs, int *len, u32 size)
>> > +{
>> > +     int ret = 0;
>> > +     struct device_node *np = dev->of_node;
>> > +     size_t sz;
>> > +     u32 *arr = NULL;
>> > +
>> > +     if (!of_get_property(np, prop_name, len)) {
>> > +             ret = -EINVAL;
>> > +             goto out;
>> > +     }
>> > +     sz = *len = *len / sizeof(*arr);
>> > +     if (sz <= 0 || (size > 0 && (sz > size))) {
>> > +             dev_err(dev, "%s invalid size\n", prop_name);
>> > +             ret = -EINVAL;
>> > +             goto out;
>> > +     }
>> > +
>> > +     arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
>> > +     if (!arr) {
>> > +             ret = -ENOMEM;
>> > +             goto out;
>> > +     }
>> > +
>> > +     ret = of_property_read_u32_array(np, prop_name, arr, sz);
>> > +     if (ret < 0) {
>> > +             dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
>> > +             goto out;
>> > +     }
>> > +     *bw_vecs = arr;
>> > +out:
>> > +     if (ret)
>> > +             *len = 0;
>> > +     return ret;
>> > +}
>> > +
>> > +/* 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;
>> > +}
>> > +
>> > +static int sdhci_msm_bus_get_vote_for_bw(struct sdhci_msm_host *host,
>> > +                                        unsigned int bw)
>> > +{
>> > +     struct sdhci_msm_bus_vote_data *bvd = host->bus_vote_data;
>> > +
>> > +     unsigned int *table = bvd->bw_vecs;
> 
> Should probably be a const bw_vecs pointer so that it can't be modified
> after the fact.

Agree and will address this in my next patch set.

> 
>> > +     unsigned int size = bvd->bw_vecs_size;
>> > +     int i;
>> > +
>> > +     for (i = 0; i < size; i++) {
>> > +             if (bw <= table[i])
>> > +                     break;
> 
> return i;

Ok. I will address this in my next patch set.

> 
>> > +     }
>> > +
> 
> return i - 1;

Ok. I will address this in my next patch set.

> 
>> > +     if (i && (i == size))
>> > +             i--;
>> > +
>> > +     return i;
> 
> And then this is useless.....

Agree and will address this in my next patch set.

> 
>> > +}
>> > +
>> > +/*
>> > + * This function must be called with host lock acquired.
>> > + * Caller of this function should also ensure that msm bus client
>> > + * handle is not null.
> 
> If it was NULL it would be pretty sad.

Agree but this is handled in the caller of this fun.
and will update in my next patch set.

> 
>> > + */
>> > +static inline int sdhci_msm_bus_set_vote(struct sdhci_msm_host
>> > *msm_host,
>> > +                                          int vote,
>> > +                                          unsigned long *flags)
>> > +{
>> > +     struct sdhci_host *host =  platform_get_drvdata(msm_host->pdev);
>> > +     struct sdhci_msm_bus_vote_data *bvd = msm_host->bus_vote_data;
>> > +     struct msm_bus_path *usecase = bvd->usecase;
>> > +     struct msm_bus_vectors *vec = usecase[vote].vec;
>> > +     int ddr_rc = 0, cpu_rc = 0;
> 
> Why initialize to 0?

upon error with icc_set_bw(), ddr_rc and cpu_rc will update with 
non-zero
values, which will be further used for error checking.
Hence i initialize to 0. (this check can even done, without 
initialization to 0).

> 
>> > +
>> > +     if (vote != bvd->curr_vote) {
>> > +             spin_unlock_irqrestore(&host->lock, *flags);
>> > +             pr_debug("%s: vote:%d sdhc_ddr ab:%llu ib:%llu cpu_sdhc ab:%llu
>> > ib:%llu\n",
>> > +                             mmc_hostname(host->mmc), vote, vec[0].ab,
>> > +                             vec[0].ib, vec[1].ab, vec[1].ib);
>> > +             ddr_rc = icc_set_bw(bvd->sdhc_ddr, vec[0].ab, vec[0].ib);
>> > +             cpu_rc = icc_set_bw(bvd->cpu_sdhc, vec[1].ab, vec[1].ib);
>> > +             spin_lock_irqsave(&host->lock, *flags);
> 
> This is some tricky spin-lockery.

True. Removed the lock, not required here now.
I will update this in my next patch set.

> 
>> > +             if (ddr_rc || cpu_rc) {
>> > +                     pr_err("%s: icc_set() failed\n",
>> > +                             mmc_hostname(host->mmc));
>> > +                     goto out;
>> > +             }
>> > +             bvd->curr_vote = vote;
>> > +     }
>> > +out:
>> > +     return cpu_rc;
>> > +}
>> > +
>> > +/*
>> > + * Internal work. Work to set 0 bandwidth for msm bus.
>> > + */
>> > +static void sdhci_msm_bus_work(struct work_struct *work)
>> > +{
>> > +     struct sdhci_msm_host *msm_host;
>> > +     struct sdhci_host *host;
>> > +     unsigned long flags;
>> > +
>> > +     msm_host = container_of(work, struct sdhci_msm_host,
>> > +                             bus_vote_work.work);
>> > +     host =  platform_get_drvdata(msm_host->pdev);
>> > +
>> > +     /* Check handle and return */
> 
> This comment is useless, please remove it.

Ok. I will address this in my next patch.

> 
>> > +     if (!msm_host->bus_vote_data->sdhc_ddr ||
>> > +                     !msm_host->bus_vote_data->cpu_sdhc)
>> > +             return;
>> > +     spin_lock_irqsave(&host->lock, flags);
>> > +     /* don't vote for 0 bandwidth if any request is in progress */
>> > +     if (!host->mmc->ongoing_mrq)
>> > +             sdhci_msm_bus_set_vote(msm_host, 0, &flags);
>> > +     else
>> > +             pr_warn("Transfer in progress.Skipping bus voting to 0\n");
> 
> Missing space after the full stop. Also, useless warning so just remove
> it?

Ok. I will address this in my next patch. I will mark it as debug 
instead of warn.
> 
>> > +     spin_unlock_irqrestore(&host->lock, flags);
>> > +}
>> > +
>> > +/*
>> > + * This function cancels any scheduled delayed work and sets the bus
>> > + * vote based on bw (bandwidth) argument.
>> > + */
>> > +static void sdhci_msm_bus_cancel_work_and_set_vote(struct sdhci_host
>> > *host,
>> > +                                             unsigned int bw)
>> > +{
>> > +     int vote;
>> > +     unsigned long flags;
>> > +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> > +     struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> > +
>> > +     cancel_delayed_work_sync(&msm_host->bus_vote_work);
>> > +     spin_lock_irqsave(&host->lock, flags);
> 
> Why does the lock need to be held? Is the interconnect framework not
> consistent with itself?

Agree. It is not required here, i will address this in my next
patch set.

> 
>> > +     vote = sdhci_msm_bus_get_vote_for_bw(msm_host, bw);
>> > +     sdhci_msm_bus_set_vote(msm_host, vote, &flags);
>> > +     spin_unlock_irqrestore(&host->lock, flags);
>> > +}
>> > +
>> > +
>> > +#define MSM_MMC_BUS_VOTING_DELAY     200 /* msecs */
>> > +#define VOTE_ZERO  0
>> > +
>> > +/* This function queues a work which will set the bandwidth requiement
>> > to 0 */
> 
> This comment style is wrong. Also s/requiement/requirement/

Ok. I will address this in my next patch set.

> 
>> > +static void sdhci_msm_bus_queue_work(struct sdhci_host *host)
>> > +{
>> > +     unsigned long flags;
>> > +     struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> > +     struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> > +
>> > +     spin_lock_irqsave(&host->lock, flags);
>> > +     if (msm_host->bus_vote_data->curr_vote != VOTE_ZERO)
>> > +             queue_delayed_work(system_wq,
>> > +                                &msm_host->bus_vote_work,
>> > +                                msecs_to_jiffies(MSM_MMC_BUS_VOTING_DELAY));
>> > +     spin_unlock_irqrestore(&host->lock, flags);
> 
> Ok it seems that the sdhci code isn't consistent with itself?

yes but it is not required to be held it now. I will address this in my
next patch set.

> 
>> > +}
>> > +
>> > +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 device_node *of_node = dev->of_node;
>> > +     struct sdhci_msm_bus_vote_data *bvd = NULL;
>> > +     struct msm_bus_path *usecase = NULL;
>> > +     int ret = 0, i = 0, j, k, num_paths, len;
>> > +     const uint32_t *vec_arr = NULL;
>> > +     bool mem_err = false;
>> > +
>> > +     if (!pdev) {
>> > +             dev_err(dev, "Null platform device!\n");
>> > +             return NULL;
>> > +     }
>> > +
>> > +     bvd = devm_kzalloc(dev, sizeof(struct sdhci_msm_bus_vote_data),
> 
> sizeof(*bvd) is better and shorter!

Agree and will address this in my next path set.

> 
>> > +                             GFP_KERNEL);
>> > +     if (!bvd) {
>> > +             ret = -ENOMEM;
>> > +             dev_err(dev, "No sufficient memory!\n");
> 
> Don't print errors for allocation failures

Ok. I will address this in my next patch set.

> 
>> > +             return bvd;
>> > +     }
>> > +     ret = sdhci_msm_dt_get_array(dev, "qcom,bus-bw-vectors-bps",
>> > +                             &bvd->bw_vecs, &bvd->bw_vecs_size, 0);
>> > +     if (ret) {
>> > +             dev_info(dev, "No dt property of bus bw. voting defined!\n");
>> > +             dev_info(dev, "Skipping Bus BW voting now!!\n");
> 
> Is this debug junk? Why does the user care?

Yes, it is just a debug info. I will mark it as debug and
will address this in my next patch set.

> 
>> > +             host->skip_bus_bw_voting = true;
>> > +             if (ret != -EINVAL && ret != -ENOMEM)
>> > +                     goto free;
>> > +             goto err;
>> > +     }
>> > +
>> > +     ret = of_property_read_string(of_node, "qcom,msm-bus,name",
>> > &bvd->name);
>> > +     if (ret) {
>> > +             dev_err(dev, "Error: (%d) Bus name missing!\n", ret);
>> > +             goto err;
>> > +     }
>> > +
>> > +     ret = of_property_read_u32(of_node, "qcom,msm-bus,num-cases",
>> > +             &bvd->num_usecase);
>> > +     if (ret) {
>> > +             dev_err(dev, "Error: num-usecases not found\n");
>> > +             goto err;
>> > +     }
>> > +
>> > +     usecase = devm_kzalloc(dev, (sizeof(struct msm_bus_path) *
>> > +                                bvd->num_usecase), GFP_KERNEL);
>> > +     if (!usecase)
>> > +             goto err;
>> > +
>> > +     ret = of_property_read_u32(of_node, "qcom,msm-bus,num-paths",
>> > +                                &num_paths);
>> > +     if (ret) {
>> > +             dev_err(dev, "Error: num_paths not found\n");
>> > +             goto out;
>> > +     }
>> > +
>> > +     vec_arr = of_get_property(of_node, "qcom,msm-bus,vectors-KBps",
> 
> Why are all the properties qcom specific?
> 
These are few qcom properties, required for bus bw voting.
Like the num-paths(sdhc-ddr, cpu-sdhc),num-cases like
clock operating frequencies (0,50MHz,200MHz),vectors-KBps to
hold the average and peak bandwidth values.

>> > &len);
>> > +     if (vec_arr == NULL) {
> 
> A more consistent style is if (!vec_arr)

Agree and will address this in my next patch set.

> 
>> > +             dev_err(dev, "Error: Vector array not found\n");
>> > +             goto out;
>> > +     }
>> > +
>> > +     for (i = 0; i < bvd->num_usecase; i++) {
>> > +             usecase[i].num_paths = num_paths;
>> > +             usecase[i].vec = devm_kzalloc(dev, num_paths *
> 
> Use devm_kcalloc().

Ok. I will address this in my next patch set.
> 
>> > +                                           sizeof(struct msm_bus_vectors),
>> > +                                           GFP_KERNEL);
>> > +             if (!usecase[i].vec) {
>> > +                     mem_err = true;
>> > +                     dev_err(dev, "Error: Failed to alloc mem for vectors\n");
>> > +                     goto out;
>> > +             }
>> > +             for (j = 0; j < num_paths; j++) {
>> > +                     int idx = ((i * num_paths) + j) * 2;
>> > +
>> > +                     usecase[i].vec[j].ab = (uint64_t)
>> > +                             be32_to_cpu(vec_arr[idx]);
>> > +                     usecase[i].vec[j].ib = (uint64_t)
>> > +                             be32_to_cpu(vec_arr[idx + 1]);
>> > +             }
>> > +     }
>> > +
>> > +     bvd->usecase = usecase;
>> > +     return bvd;
>> > +out:
>> > +     if (mem_err) {
> 
> Should be possible to not have this flag.

Ok, i will check not to have this flag.
and will address this in my next patch set.

> 
>> > +             for (k = i - 1; k >= 0; k--)
>> > +                     devm_kfree(dev, usecase[k].vec);
>> > +     }
>> > +     devm_kfree(dev, usecase);
>> > +free:
>> > +     devm_kfree(dev, bvd->bw_vecs);
>> > +err:
>> > +     devm_kfree(dev, bvd);
> 
> You don't need to devm_kfree() anything, just let probe fail it.

Agree. Also considering the probe on the existing targets, it should not 
fail.
due to this new bus bw voting.  I will handle accordingly ,to not use 
devm_kfree()
and will address this in my next patch set.
> 
>> > +     bvd = NULL;
>> > +     return bvd;
>> > +}
>> > +
>> > +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
>> > +                             struct platform_device *pdev)
>> > +{
>> > +     struct sdhci_msm_bus_vote_data *bsd;
>> > +     struct device *dev = &pdev->dev;
>> > +
>> > +     bsd = sdhci_msm_get_bus_vote_data(dev, host);
>> > +     if (!bsd) {
>> > +             dev_err(&pdev->dev, "Failed: getting bus_scale data\n");
>> > +             return PTR_ERR(bsd);
>> > +     }
>> > +     host->bus_vote_data = bsd;
>> > +
>> > +     bsd->sdhc_ddr = of_icc_get(&pdev->dev, SDHC_DDR);
>> > +     if (IS_ERR(bsd->sdhc_ddr)) {
>> > +             dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
> 
> We don't need "Error: " prefix. That's what the kernel log level is 
> for.

Agree. I will address this in my next patch set.

> 
>> > +                     PTR_ERR(bsd->sdhc_ddr), SDHC_DDR);
>> > +             return PTR_ERR(bsd->sdhc_ddr);
>> > +     }
>> > +
>> > +     bsd->cpu_sdhc = of_icc_get(&pdev->dev, CPU_SDHC);
>> > +     if (IS_ERR(bsd->cpu_sdhc)) {
>> > +             dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
>> > +                     PTR_ERR(bsd->cpu_sdhc), CPU_SDHC);
>> > +             return PTR_ERR(bsd->cpu_sdhc);
>> > +     }
>> > +
>> > +     INIT_DELAYED_WORK(&host->bus_vote_work, sdhci_msm_bus_work);
> 
> Why is it in a workqueue context? Is there any reason it can' be done 
> in
> whatever calling context it is?
> 
If is in delayed workqueue context (setting the vote to zero), we can 
reduce the overhead
of calling many involved functions, when there are back to back requests 
for bus bw vote.
This can also improve the device suspend trigger to actual suspend 
latency.

>> > +
>> > +     return 0;
>> > +}
>> > +
>> > +static void sdhci_msm_bus_unregister(struct device *dev,
>> > +                             struct sdhci_msm_host *host)
>> > +{
>> > +     struct sdhci_msm_bus_vote_data *bsd = host->bus_vote_data;
>> > +     int i;
>> > +
>> > +     icc_put(bsd->sdhc_ddr);
>> > +     icc_put(bsd->cpu_sdhc);
> 
> Is there a devm_icc_get() API? If not, please add it and use it.

Do you mean devm_clk_get() ? i didn't find any API with name 
devm_icc_get().
Can you please elaborate more on this ?

> 
>> > +
>> > +     for (i = 0; i < bsd->num_usecase; i++)
>> > +             devm_kfree(dev, bsd->usecase[i].vec);
>> > +     devm_kfree(dev, bsd->usecase);
>> > +     devm_kfree(dev, bsd->bw_vecs);
>> > +     devm_kfree(dev, bsd);
> 
> Again, not sure we need any devm_kfree() stuff.

Agree. I will address this in my next patch set.

> 
>> > +}
>> > +
>> > +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
>> > +{
>> > +     struct mmc_ios *ios = &host->mmc->ios;
>> > +     unsigned int bw;
>> > +
>> > +     bw = sdhci_get_bw_required(host, ios);
>> > +     if (enable)
>> > +             sdhci_msm_bus_cancel_work_and_set_vote(host, bw);
>> > +     else
>> > +             sdhci_msm_bus_queue_work(host);
>> > +}
>> > +
>> >  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,
>> > @@ -1839,6 +2210,13 @@ 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->skip_bus_bw_voting)
> 
> Can this be a check for a NULL icc handle instead of the bool?

Considering the support of bus bandwidth on the existing devices,
this bool check will help the device probe for its continuity or fail.
Hence this check is used instead of NULL icc handle.
Any how the NULL icc handle check is added in the bus bw vote setting.

Re: [RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting

[Georgi Djakov]

Hi Pradeep,

Thanks for the patch!

On 9/6/19 15:47, Pradeep P V K wrote:
> Vote for the MSM bus bandwidth 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)

I am just wondering whether do we really need to predefine the bandwidth values
in DT? Can't we use the computations from the above patch or is there any
problem with that approach?

Thanks,
Georgi

Re: [RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting

[ppvk]

On 2019-09-12 18:26, Georgi Djakov wrote:
> Hi Pradeep,
> 
> Thanks for the patch!
> 
> On 9/6/19 15:47, Pradeep P V K wrote:
>> Vote for the MSM bus bandwidth 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)
> 
> I am just wondering whether do we really need to predefine the 
> bandwidth values
> in DT? Can't we use the computations from the above patch or is there 
> any
> problem with that approach?
> 
> Thanks,
> Georgi

Hi Georgi,

By using the direct required bandwidth(bw / 1000) values, it will not 
guarantee
that all the NOC clocks are running in the same voltage corner as 
required,
which is very crucial for power concern devices like Mobiles etc.
Also, it will not guarantee that the value passed is in proper Clock 
Plans domain
there by effecting the requested Bandwidth.
I think, you already aware of these consequences on using direct 
bandwidth values for
RPMh based devices.

The value the we passed in DT will make sure that all the NOC clocks 
between the end points
are running in the same voltage corners as required and also it will 
guarantee that
the requested BW's for the clients are obtained.

Hence the reason for passing the predefined bandwidth values in DT.

Thanks and Regards,
Pradeep

Re: [RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting

[Bjorn Andersson]

On Fri 06 Sep 05:47 PDT 2019, Pradeep P V K wrote:

> Vote for the MSM bus bandwidth 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)
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>

This says that Sahitya wrote the patch, then Subhash handled it, then
Veerabhadrarao handled it and finally you handled it; but you're at the
same time listed as author (by From:).

Please see section 12 on Co-Developed-by in submitting-patches.rst

> ---
>  drivers/mmc/host/sdhci-msm.c | 393 ++++++++++++++++++++++++++++++++++++++++++-

This patch implements support for requesting bandwidth to the register
space and for the controllers access to DDR. To me this seems like
common requirements for any mmc controller, can this functionality be
provided by the mmc/sdhci common code?

Regards,
Bjorn

>  1 file changed, 390 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index b75c82d..71515ca 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -11,6 +11,7 @@
>  #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>
>  
> @@ -122,6 +123,9 @@
>  #define msm_host_writel(msm_host, val, host, offset) \
>  	msm_host->var_ops->msm_writel_relaxed(val, host, offset)
>  
> +#define SDHC_DDR "sdhc-ddr"
> +#define CPU_SDHC "cpu-sdhc"
> +
>  struct sdhci_msm_offset {
>  	u32 core_hc_mode;
>  	u32 core_mci_data_cnt;
> @@ -228,6 +232,31 @@ struct sdhci_msm_variant_info {
>  	const struct sdhci_msm_offset *offset;
>  };
>  
> +struct msm_bus_vectors {
> +	uint64_t ab;
> +	uint64_t ib;
> +};
> +
> +struct msm_bus_path {
> +	unsigned int num_paths;
> +	struct msm_bus_vectors *vec;
> +};
> +
> +struct sdhci_msm_bus_vote_data {
> +	const char *name;
> +	unsigned int num_usecase;
> +	struct msm_bus_path *usecase;
> +
> +	unsigned int *bw_vecs;
> +	unsigned int bw_vecs_size;
> +
> +	struct icc_path *sdhc_ddr;
> +	struct icc_path *cpu_sdhc;
> +
> +	uint32_t curr_vote;
> +
> +};
> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -253,8 +282,13 @@ struct sdhci_msm_host {
>  	const struct sdhci_msm_offset *offset;
>  	bool use_cdr;
>  	u32 transfer_mode;
> +	bool skip_bus_bw_voting;
> +	struct sdhci_msm_bus_vote_data *bus_vote_data;
> +	struct delayed_work bus_vote_work;
>  };
>  
> +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);
> @@ -1557,6 +1591,8 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  
>  	msm_set_clock_rate_for_bus_mode(host, clock);
>  out:
> +	if (!msm_host->skip_bus_bw_voting)
> +		sdhci_msm_bus_voting(host, !!clock);
>  	__sdhci_msm_set_clock(host, clock);
>  }
>  
> @@ -1678,6 +1714,341 @@ 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);
>  }
>  
> +static int sdhci_msm_dt_get_array(struct device *dev, const char *prop_name,
> +				 u32 **bw_vecs, int *len, u32 size)
> +{
> +	int ret = 0;
> +	struct device_node *np = dev->of_node;
> +	size_t sz;
> +	u32 *arr = NULL;
> +
> +	if (!of_get_property(np, prop_name, len)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	sz = *len = *len / sizeof(*arr);
> +	if (sz <= 0 || (size > 0 && (sz > size))) {
> +		dev_err(dev, "%s invalid size\n", prop_name);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
> +	if (!arr) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
> +	if (ret < 0) {
> +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
> +		goto out;
> +	}
> +	*bw_vecs = arr;
> +out:
> +	if (ret)
> +		*len = 0;
> +	return ret;
> +}
> +
> +/* 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;
> +}
> +
> +static int sdhci_msm_bus_get_vote_for_bw(struct sdhci_msm_host *host,
> +					   unsigned int bw)
> +{
> +	struct sdhci_msm_bus_vote_data *bvd = host->bus_vote_data;
> +
> +	unsigned int *table = bvd->bw_vecs;
> +	unsigned int size = bvd->bw_vecs_size;
> +	int i;
> +
> +	for (i = 0; i < size; i++) {
> +		if (bw <= table[i])
> +			break;
> +	}
> +
> +	if (i && (i == size))
> +		i--;
> +
> +	return i;
> +}
> +
> +/*
> + * This function must be called with host lock acquired.
> + * Caller of this function should also ensure that msm bus client
> + * handle is not null.
> + */
> +static inline int sdhci_msm_bus_set_vote(struct sdhci_msm_host *msm_host,
> +					     int vote,
> +					     unsigned long *flags)
> +{
> +	struct sdhci_host *host =  platform_get_drvdata(msm_host->pdev);
> +	struct sdhci_msm_bus_vote_data *bvd = msm_host->bus_vote_data;
> +	struct msm_bus_path *usecase = bvd->usecase;
> +	struct msm_bus_vectors *vec = usecase[vote].vec;
> +	int ddr_rc = 0, cpu_rc = 0;
> +
> +	if (vote != bvd->curr_vote) {
> +		spin_unlock_irqrestore(&host->lock, *flags);
> +		pr_debug("%s: vote:%d sdhc_ddr ab:%llu ib:%llu cpu_sdhc ab:%llu ib:%llu\n",
> +				mmc_hostname(host->mmc), vote, vec[0].ab,
> +				vec[0].ib, vec[1].ab, vec[1].ib);
> +		ddr_rc = icc_set_bw(bvd->sdhc_ddr, vec[0].ab, vec[0].ib);
> +		cpu_rc = icc_set_bw(bvd->cpu_sdhc, vec[1].ab, vec[1].ib);
> +		spin_lock_irqsave(&host->lock, *flags);
> +		if (ddr_rc || cpu_rc) {
> +			pr_err("%s: icc_set() failed\n",
> +				mmc_hostname(host->mmc));
> +			goto out;
> +		}
> +		bvd->curr_vote = vote;
> +	}
> +out:
> +	return cpu_rc;
> +}
> +
> +/*
> + * Internal work. Work to set 0 bandwidth for msm bus.
> + */
> +static void sdhci_msm_bus_work(struct work_struct *work)
> +{
> +	struct sdhci_msm_host *msm_host;
> +	struct sdhci_host *host;
> +	unsigned long flags;
> +
> +	msm_host = container_of(work, struct sdhci_msm_host,
> +				bus_vote_work.work);
> +	host =  platform_get_drvdata(msm_host->pdev);
> +
> +	/* Check handle and return */
> +	if (!msm_host->bus_vote_data->sdhc_ddr ||
> +			!msm_host->bus_vote_data->cpu_sdhc)
> +		return;
> +	spin_lock_irqsave(&host->lock, flags);
> +	/* don't vote for 0 bandwidth if any request is in progress */
> +	if (!host->mmc->ongoing_mrq)
> +		sdhci_msm_bus_set_vote(msm_host, 0, &flags);
> +	else
> +		pr_warn("Transfer in progress.Skipping bus voting to 0\n");
> +	spin_unlock_irqrestore(&host->lock, flags);
> +}
> +
> +/*
> + * This function cancels any scheduled delayed work and sets the bus
> + * vote based on bw (bandwidth) argument.
> + */
> +static void sdhci_msm_bus_cancel_work_and_set_vote(struct sdhci_host *host,
> +						unsigned int bw)
> +{
> +	int vote;
> +	unsigned long flags;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	cancel_delayed_work_sync(&msm_host->bus_vote_work);
> +	spin_lock_irqsave(&host->lock, flags);
> +	vote = sdhci_msm_bus_get_vote_for_bw(msm_host, bw);
> +	sdhci_msm_bus_set_vote(msm_host, vote, &flags);
> +	spin_unlock_irqrestore(&host->lock, flags);
> +}
> +
> +
> +#define MSM_MMC_BUS_VOTING_DELAY	200 /* msecs */
> +#define VOTE_ZERO  0
> +
> +/* This function queues a work which will set the bandwidth requiement to 0 */
> +static void sdhci_msm_bus_queue_work(struct sdhci_host *host)
> +{
> +	unsigned long flags;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	spin_lock_irqsave(&host->lock, flags);
> +	if (msm_host->bus_vote_data->curr_vote != VOTE_ZERO)
> +		queue_delayed_work(system_wq,
> +				   &msm_host->bus_vote_work,
> +				   msecs_to_jiffies(MSM_MMC_BUS_VOTING_DELAY));
> +	spin_unlock_irqrestore(&host->lock, flags);
> +}
> +
> +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 device_node *of_node = dev->of_node;
> +	struct sdhci_msm_bus_vote_data *bvd = NULL;
> +	struct msm_bus_path *usecase = NULL;
> +	int ret = 0, i = 0, j, k, num_paths, len;
> +	const uint32_t *vec_arr = NULL;
> +	bool mem_err = false;
> +
> +	if (!pdev) {
> +		dev_err(dev, "Null platform device!\n");
> +		return NULL;
> +	}
> +
> +	bvd = devm_kzalloc(dev, sizeof(struct sdhci_msm_bus_vote_data),
> +				GFP_KERNEL);
> +	if (!bvd) {
> +		ret = -ENOMEM;
> +		dev_err(dev, "No sufficient memory!\n");
> +		return bvd;
> +	}
> +	ret = sdhci_msm_dt_get_array(dev, "qcom,bus-bw-vectors-bps",
> +				&bvd->bw_vecs, &bvd->bw_vecs_size, 0);
> +	if (ret) {
> +		dev_info(dev, "No dt property of bus bw. voting defined!\n");
> +		dev_info(dev, "Skipping Bus BW voting now!!\n");
> +		host->skip_bus_bw_voting = true;
> +		if (ret != -EINVAL && ret != -ENOMEM)
> +			goto free;
> +		goto err;
> +	}
> +
> +	ret = of_property_read_string(of_node, "qcom,msm-bus,name", &bvd->name);
> +	if (ret) {
> +		dev_err(dev, "Error: (%d) Bus name missing!\n", ret);
> +		goto err;
> +	}
> +
> +	ret = of_property_read_u32(of_node, "qcom,msm-bus,num-cases",
> +		&bvd->num_usecase);
> +	if (ret) {
> +		dev_err(dev, "Error: num-usecases not found\n");
> +		goto err;
> +	}
> +
> +	usecase = devm_kzalloc(dev, (sizeof(struct msm_bus_path) *
> +				   bvd->num_usecase), GFP_KERNEL);
> +	if (!usecase)
> +		goto err;
> +
> +	ret = of_property_read_u32(of_node, "qcom,msm-bus,num-paths",
> +				   &num_paths);
> +	if (ret) {
> +		dev_err(dev, "Error: num_paths not found\n");
> +		goto out;
> +	}
> +
> +	vec_arr = of_get_property(of_node, "qcom,msm-bus,vectors-KBps", &len);
> +	if (vec_arr == NULL) {
> +		dev_err(dev, "Error: Vector array not found\n");
> +		goto out;
> +	}
> +
> +	for (i = 0; i < bvd->num_usecase; i++) {
> +		usecase[i].num_paths = num_paths;
> +		usecase[i].vec = devm_kzalloc(dev, num_paths *
> +					      sizeof(struct msm_bus_vectors),
> +					      GFP_KERNEL);
> +		if (!usecase[i].vec) {
> +			mem_err = true;
> +			dev_err(dev, "Error: Failed to alloc mem for vectors\n");
> +			goto out;
> +		}
> +		for (j = 0; j < num_paths; j++) {
> +			int idx = ((i * num_paths) + j) * 2;
> +
> +			usecase[i].vec[j].ab = (uint64_t)
> +				be32_to_cpu(vec_arr[idx]);
> +			usecase[i].vec[j].ib = (uint64_t)
> +				be32_to_cpu(vec_arr[idx + 1]);
> +		}
> +	}
> +
> +	bvd->usecase = usecase;
> +	return bvd;
> +out:
> +	if (mem_err) {
> +		for (k = i - 1; k >= 0; k--)
> +			devm_kfree(dev, usecase[k].vec);
> +	}
> +	devm_kfree(dev, usecase);
> +free:
> +	devm_kfree(dev, bvd->bw_vecs);
> +err:
> +	devm_kfree(dev, bvd);
> +	bvd = NULL;
> +	return bvd;
> +}
> +
> +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
> +				struct platform_device *pdev)
> +{
> +	struct sdhci_msm_bus_vote_data *bsd;
> +	struct device *dev = &pdev->dev;
> +
> +	bsd = sdhci_msm_get_bus_vote_data(dev, host);
> +	if (!bsd) {
> +		dev_err(&pdev->dev, "Failed: getting bus_scale data\n");
> +		return PTR_ERR(bsd);
> +	}
> +	host->bus_vote_data = bsd;
> +
> +	bsd->sdhc_ddr = of_icc_get(&pdev->dev, SDHC_DDR);
> +	if (IS_ERR(bsd->sdhc_ddr)) {
> +		dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
> +			PTR_ERR(bsd->sdhc_ddr), SDHC_DDR);
> +		return PTR_ERR(bsd->sdhc_ddr);
> +	}
> +
> +	bsd->cpu_sdhc = of_icc_get(&pdev->dev, CPU_SDHC);
> +	if (IS_ERR(bsd->cpu_sdhc)) {
> +		dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
> +			PTR_ERR(bsd->cpu_sdhc), CPU_SDHC);
> +		return PTR_ERR(bsd->cpu_sdhc);
> +	}
> +
> +	INIT_DELAYED_WORK(&host->bus_vote_work, sdhci_msm_bus_work);
> +
> +	return 0;
> +}
> +
> +static void sdhci_msm_bus_unregister(struct device *dev,
> +				struct sdhci_msm_host *host)
> +{
> +	struct sdhci_msm_bus_vote_data *bsd = host->bus_vote_data;
> +	int i;
> +
> +	icc_put(bsd->sdhc_ddr);
> +	icc_put(bsd->cpu_sdhc);
> +
> +	for (i = 0; i < bsd->num_usecase; i++)
> +		devm_kfree(dev, bsd->usecase[i].vec);
> +	devm_kfree(dev, bsd->usecase);
> +	devm_kfree(dev, bsd->bw_vecs);
> +	devm_kfree(dev, bsd);
> +}
> +
> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
> +{
> +	struct mmc_ios *ios = &host->mmc->ios;
> +	unsigned int bw;
> +
> +	bw = sdhci_get_bw_required(host, ios);
> +	if (enable)
> +		sdhci_msm_bus_cancel_work_and_set_vote(host, bw);
> +	else
> +		sdhci_msm_bus_queue_work(host);
> +}
> +
>  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,
> @@ -1839,6 +2210,13 @@ 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->skip_bus_bw_voting)
> +		goto clk_disable;
> +
> +	if (!msm_host->skip_bus_bw_voting)
> +		sdhci_msm_bus_voting(host, 1);
> +
>  	if (!msm_host->mci_removed) {
>  		core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>  		msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
> @@ -1846,7 +2224,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  
>  		if (IS_ERR(msm_host->core_mem)) {
>  			ret = PTR_ERR(msm_host->core_mem);
> -			goto clk_disable;
> +			goto bus_unregister;
>  		}
>  	}
>  
> @@ -1918,7 +2296,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);
> @@ -1931,7 +2309,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);
> @@ -1956,6 +2334,11 @@ 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:
> +	if (!msm_host->skip_bus_bw_voting) {
> +		sdhci_msm_bus_cancel_work_and_set_vote(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);
> @@ -1985,6 +2368,10 @@ 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);
> +	if (!msm_host->skip_bus_bw_voting) {
> +		sdhci_msm_bus_cancel_work_and_set_vote(host, 0);
> +		sdhci_msm_bus_unregister(&pdev->dev, msm_host);
> +	}
>  	sdhci_pltfm_free(pdev);
>  	return 0;
>  }
> -- 
> 1.9.1
> 

Re: [RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting

[ppvk]

On 2019-09-12 22:15, Bjorn Andersson wrote:
> On Fri 06 Sep 05:47 PDT 2019, Pradeep P V K wrote:
> 
>> Vote for the MSM bus bandwidth 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)
>> 
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
>> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
> 
> This says that Sahitya wrote the patch, then Subhash handled it, then
> Veerabhadrarao handled it and finally you handled it; but you're at the
> same time listed as author (by From:).
> 
> Please see section 12 on Co-Developed-by in submitting-patches.rst
> 
Thanks Bjorn, i will update this on my next patch set.
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 393 
>> ++++++++++++++++++++++++++++++++++++++++++-
> 
> This patch implements support for requesting bandwidth to the register
> space and for the controllers access to DDR. To me this seems like
> common requirements for any mmc controller, can this functionality be
> provided by the mmc/sdhci common code?
> 
> Regards,
> Bjorn
> 
Yes, this can be provided in common code but the bandwidth calculations
(arbitrated value or average bandwidth and instantaneous value or peak 
bandwidth) for bus vote will
consider various parameters like voltage corners, clock domains, clock 
plans etc. which may differ from
vendor to vendor and target to target. So, these values should be 
updated properly and correctly (considering all the parameters)
if it brings to common area.

Hence the reason for implementing this in sdhci-msm.c file.
It would be really helpful if you could suggest your insights on where 
and how exactly this needs to be
implemented in common code area.

Hi Ulf and Adrian,

Can you please also suggest your recommendations on this ?

Thanks and Regards,
Pradeep

>>  1 file changed, 390 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/mmc/host/sdhci-msm.c 
>> b/drivers/mmc/host/sdhci-msm.c
>> index b75c82d..71515ca 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -11,6 +11,7 @@
>>  #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>
>> 
>> @@ -122,6 +123,9 @@
>>  #define msm_host_writel(msm_host, val, host, offset) \
>>  	msm_host->var_ops->msm_writel_relaxed(val, host, offset)
>> 
>> +#define SDHC_DDR "sdhc-ddr"
>> +#define CPU_SDHC "cpu-sdhc"
>> +
>>  struct sdhci_msm_offset {
>>  	u32 core_hc_mode;
>>  	u32 core_mci_data_cnt;
>> @@ -228,6 +232,31 @@ struct sdhci_msm_variant_info {
>>  	const struct sdhci_msm_offset *offset;
>>  };
>> 
>> +struct msm_bus_vectors {
>> +	uint64_t ab;
>> +	uint64_t ib;
>> +};
>> +
>> +struct msm_bus_path {
>> +	unsigned int num_paths;
>> +	struct msm_bus_vectors *vec;
>> +};
>> +
>> +struct sdhci_msm_bus_vote_data {
>> +	const char *name;
>> +	unsigned int num_usecase;
>> +	struct msm_bus_path *usecase;
>> +
>> +	unsigned int *bw_vecs;
>> +	unsigned int bw_vecs_size;
>> +
>> +	struct icc_path *sdhc_ddr;
>> +	struct icc_path *cpu_sdhc;
>> +
>> +	uint32_t curr_vote;
>> +
>> +};
>> +
>>  struct sdhci_msm_host {
>>  	struct platform_device *pdev;
>>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
>> @@ -253,8 +282,13 @@ struct sdhci_msm_host {
>>  	const struct sdhci_msm_offset *offset;
>>  	bool use_cdr;
>>  	u32 transfer_mode;
>> +	bool skip_bus_bw_voting;
>> +	struct sdhci_msm_bus_vote_data *bus_vote_data;
>> +	struct delayed_work bus_vote_work;
>>  };
>> 
>> +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);
>> @@ -1557,6 +1591,8 @@ static void sdhci_msm_set_clock(struct 
>> sdhci_host *host, unsigned int clock)
>> 
>>  	msm_set_clock_rate_for_bus_mode(host, clock);
>>  out:
>> +	if (!msm_host->skip_bus_bw_voting)
>> +		sdhci_msm_bus_voting(host, !!clock);
>>  	__sdhci_msm_set_clock(host, clock);
>>  }
>> 
>> @@ -1678,6 +1714,341 @@ 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);
>>  }
>> 
>> +static int sdhci_msm_dt_get_array(struct device *dev, const char 
>> *prop_name,
>> +				 u32 **bw_vecs, int *len, u32 size)
>> +{
>> +	int ret = 0;
>> +	struct device_node *np = dev->of_node;
>> +	size_t sz;
>> +	u32 *arr = NULL;
>> +
>> +	if (!of_get_property(np, prop_name, len)) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +	sz = *len = *len / sizeof(*arr);
>> +	if (sz <= 0 || (size > 0 && (sz > size))) {
>> +		dev_err(dev, "%s invalid size\n", prop_name);
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	arr = devm_kzalloc(dev, sz * sizeof(*arr), GFP_KERNEL);
>> +	if (!arr) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	ret = of_property_read_u32_array(np, prop_name, arr, sz);
>> +	if (ret < 0) {
>> +		dev_err(dev, "%s failed reading array %d\n", prop_name, ret);
>> +		goto out;
>> +	}
>> +	*bw_vecs = arr;
>> +out:
>> +	if (ret)
>> +		*len = 0;
>> +	return ret;
>> +}
>> +
>> +/* 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;
>> +}
>> +
>> +static int sdhci_msm_bus_get_vote_for_bw(struct sdhci_msm_host *host,
>> +					   unsigned int bw)
>> +{
>> +	struct sdhci_msm_bus_vote_data *bvd = host->bus_vote_data;
>> +
>> +	unsigned int *table = bvd->bw_vecs;
>> +	unsigned int size = bvd->bw_vecs_size;
>> +	int i;
>> +
>> +	for (i = 0; i < size; i++) {
>> +		if (bw <= table[i])
>> +			break;
>> +	}
>> +
>> +	if (i && (i == size))
>> +		i--;
>> +
>> +	return i;
>> +}
>> +
>> +/*
>> + * This function must be called with host lock acquired.
>> + * Caller of this function should also ensure that msm bus client
>> + * handle is not null.
>> + */
>> +static inline int sdhci_msm_bus_set_vote(struct sdhci_msm_host 
>> *msm_host,
>> +					     int vote,
>> +					     unsigned long *flags)
>> +{
>> +	struct sdhci_host *host =  platform_get_drvdata(msm_host->pdev);
>> +	struct sdhci_msm_bus_vote_data *bvd = msm_host->bus_vote_data;
>> +	struct msm_bus_path *usecase = bvd->usecase;
>> +	struct msm_bus_vectors *vec = usecase[vote].vec;
>> +	int ddr_rc = 0, cpu_rc = 0;
>> +
>> +	if (vote != bvd->curr_vote) {
>> +		spin_unlock_irqrestore(&host->lock, *flags);
>> +		pr_debug("%s: vote:%d sdhc_ddr ab:%llu ib:%llu cpu_sdhc ab:%llu 
>> ib:%llu\n",
>> +				mmc_hostname(host->mmc), vote, vec[0].ab,
>> +				vec[0].ib, vec[1].ab, vec[1].ib);
>> +		ddr_rc = icc_set_bw(bvd->sdhc_ddr, vec[0].ab, vec[0].ib);
>> +		cpu_rc = icc_set_bw(bvd->cpu_sdhc, vec[1].ab, vec[1].ib);
>> +		spin_lock_irqsave(&host->lock, *flags);
>> +		if (ddr_rc || cpu_rc) {
>> +			pr_err("%s: icc_set() failed\n",
>> +				mmc_hostname(host->mmc));
>> +			goto out;
>> +		}
>> +		bvd->curr_vote = vote;
>> +	}
>> +out:
>> +	return cpu_rc;
>> +}
>> +
>> +/*
>> + * Internal work. Work to set 0 bandwidth for msm bus.
>> + */
>> +static void sdhci_msm_bus_work(struct work_struct *work)
>> +{
>> +	struct sdhci_msm_host *msm_host;
>> +	struct sdhci_host *host;
>> +	unsigned long flags;
>> +
>> +	msm_host = container_of(work, struct sdhci_msm_host,
>> +				bus_vote_work.work);
>> +	host =  platform_get_drvdata(msm_host->pdev);
>> +
>> +	/* Check handle and return */
>> +	if (!msm_host->bus_vote_data->sdhc_ddr ||
>> +			!msm_host->bus_vote_data->cpu_sdhc)
>> +		return;
>> +	spin_lock_irqsave(&host->lock, flags);
>> +	/* don't vote for 0 bandwidth if any request is in progress */
>> +	if (!host->mmc->ongoing_mrq)
>> +		sdhci_msm_bus_set_vote(msm_host, 0, &flags);
>> +	else
>> +		pr_warn("Transfer in progress.Skipping bus voting to 0\n");
>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +}
>> +
>> +/*
>> + * This function cancels any scheduled delayed work and sets the bus
>> + * vote based on bw (bandwidth) argument.
>> + */
>> +static void sdhci_msm_bus_cancel_work_and_set_vote(struct sdhci_host 
>> *host,
>> +						unsigned int bw)
>> +{
>> +	int vote;
>> +	unsigned long flags;
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	cancel_delayed_work_sync(&msm_host->bus_vote_work);
>> +	spin_lock_irqsave(&host->lock, flags);
>> +	vote = sdhci_msm_bus_get_vote_for_bw(msm_host, bw);
>> +	sdhci_msm_bus_set_vote(msm_host, vote, &flags);
>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +}
>> +
>> +
>> +#define MSM_MMC_BUS_VOTING_DELAY	200 /* msecs */
>> +#define VOTE_ZERO  0
>> +
>> +/* This function queues a work which will set the bandwidth 
>> requiement to 0 */
>> +static void sdhci_msm_bus_queue_work(struct sdhci_host *host)
>> +{
>> +	unsigned long flags;
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	spin_lock_irqsave(&host->lock, flags);
>> +	if (msm_host->bus_vote_data->curr_vote != VOTE_ZERO)
>> +		queue_delayed_work(system_wq,
>> +				   &msm_host->bus_vote_work,
>> +				   msecs_to_jiffies(MSM_MMC_BUS_VOTING_DELAY));
>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +}
>> +
>> +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 device_node *of_node = dev->of_node;
>> +	struct sdhci_msm_bus_vote_data *bvd = NULL;
>> +	struct msm_bus_path *usecase = NULL;
>> +	int ret = 0, i = 0, j, k, num_paths, len;
>> +	const uint32_t *vec_arr = NULL;
>> +	bool mem_err = false;
>> +
>> +	if (!pdev) {
>> +		dev_err(dev, "Null platform device!\n");
>> +		return NULL;
>> +	}
>> +
>> +	bvd = devm_kzalloc(dev, sizeof(struct sdhci_msm_bus_vote_data),
>> +				GFP_KERNEL);
>> +	if (!bvd) {
>> +		ret = -ENOMEM;
>> +		dev_err(dev, "No sufficient memory!\n");
>> +		return bvd;
>> +	}
>> +	ret = sdhci_msm_dt_get_array(dev, "qcom,bus-bw-vectors-bps",
>> +				&bvd->bw_vecs, &bvd->bw_vecs_size, 0);
>> +	if (ret) {
>> +		dev_info(dev, "No dt property of bus bw. voting defined!\n");
>> +		dev_info(dev, "Skipping Bus BW voting now!!\n");
>> +		host->skip_bus_bw_voting = true;
>> +		if (ret != -EINVAL && ret != -ENOMEM)
>> +			goto free;
>> +		goto err;
>> +	}
>> +
>> +	ret = of_property_read_string(of_node, "qcom,msm-bus,name", 
>> &bvd->name);
>> +	if (ret) {
>> +		dev_err(dev, "Error: (%d) Bus name missing!\n", ret);
>> +		goto err;
>> +	}
>> +
>> +	ret = of_property_read_u32(of_node, "qcom,msm-bus,num-cases",
>> +		&bvd->num_usecase);
>> +	if (ret) {
>> +		dev_err(dev, "Error: num-usecases not found\n");
>> +		goto err;
>> +	}
>> +
>> +	usecase = devm_kzalloc(dev, (sizeof(struct msm_bus_path) *
>> +				   bvd->num_usecase), GFP_KERNEL);
>> +	if (!usecase)
>> +		goto err;
>> +
>> +	ret = of_property_read_u32(of_node, "qcom,msm-bus,num-paths",
>> +				   &num_paths);
>> +	if (ret) {
>> +		dev_err(dev, "Error: num_paths not found\n");
>> +		goto out;
>> +	}
>> +
>> +	vec_arr = of_get_property(of_node, "qcom,msm-bus,vectors-KBps", 
>> &len);
>> +	if (vec_arr == NULL) {
>> +		dev_err(dev, "Error: Vector array not found\n");
>> +		goto out;
>> +	}
>> +
>> +	for (i = 0; i < bvd->num_usecase; i++) {
>> +		usecase[i].num_paths = num_paths;
>> +		usecase[i].vec = devm_kzalloc(dev, num_paths *
>> +					      sizeof(struct msm_bus_vectors),
>> +					      GFP_KERNEL);
>> +		if (!usecase[i].vec) {
>> +			mem_err = true;
>> +			dev_err(dev, "Error: Failed to alloc mem for vectors\n");
>> +			goto out;
>> +		}
>> +		for (j = 0; j < num_paths; j++) {
>> +			int idx = ((i * num_paths) + j) * 2;
>> +
>> +			usecase[i].vec[j].ab = (uint64_t)
>> +				be32_to_cpu(vec_arr[idx]);
>> +			usecase[i].vec[j].ib = (uint64_t)
>> +				be32_to_cpu(vec_arr[idx + 1]);
>> +		}
>> +	}
>> +
>> +	bvd->usecase = usecase;
>> +	return bvd;
>> +out:
>> +	if (mem_err) {
>> +		for (k = i - 1; k >= 0; k--)
>> +			devm_kfree(dev, usecase[k].vec);
>> +	}
>> +	devm_kfree(dev, usecase);
>> +free:
>> +	devm_kfree(dev, bvd->bw_vecs);
>> +err:
>> +	devm_kfree(dev, bvd);
>> +	bvd = NULL;
>> +	return bvd;
>> +}
>> +
>> +static int sdhci_msm_bus_register(struct sdhci_msm_host *host,
>> +				struct platform_device *pdev)
>> +{
>> +	struct sdhci_msm_bus_vote_data *bsd;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	bsd = sdhci_msm_get_bus_vote_data(dev, host);
>> +	if (!bsd) {
>> +		dev_err(&pdev->dev, "Failed: getting bus_scale data\n");
>> +		return PTR_ERR(bsd);
>> +	}
>> +	host->bus_vote_data = bsd;
>> +
>> +	bsd->sdhc_ddr = of_icc_get(&pdev->dev, SDHC_DDR);
>> +	if (IS_ERR(bsd->sdhc_ddr)) {
>> +		dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
>> +			PTR_ERR(bsd->sdhc_ddr), SDHC_DDR);
>> +		return PTR_ERR(bsd->sdhc_ddr);
>> +	}
>> +
>> +	bsd->cpu_sdhc = of_icc_get(&pdev->dev, CPU_SDHC);
>> +	if (IS_ERR(bsd->cpu_sdhc)) {
>> +		dev_err(&pdev->dev, "Error: (%ld) failed getting %s path\n",
>> +			PTR_ERR(bsd->cpu_sdhc), CPU_SDHC);
>> +		return PTR_ERR(bsd->cpu_sdhc);
>> +	}
>> +
>> +	INIT_DELAYED_WORK(&host->bus_vote_work, sdhci_msm_bus_work);
>> +
>> +	return 0;
>> +}
>> +
>> +static void sdhci_msm_bus_unregister(struct device *dev,
>> +				struct sdhci_msm_host *host)
>> +{
>> +	struct sdhci_msm_bus_vote_data *bsd = host->bus_vote_data;
>> +	int i;
>> +
>> +	icc_put(bsd->sdhc_ddr);
>> +	icc_put(bsd->cpu_sdhc);
>> +
>> +	for (i = 0; i < bsd->num_usecase; i++)
>> +		devm_kfree(dev, bsd->usecase[i].vec);
>> +	devm_kfree(dev, bsd->usecase);
>> +	devm_kfree(dev, bsd->bw_vecs);
>> +	devm_kfree(dev, bsd);
>> +}
>> +
>> +static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
>> +{
>> +	struct mmc_ios *ios = &host->mmc->ios;
>> +	unsigned int bw;
>> +
>> +	bw = sdhci_get_bw_required(host, ios);
>> +	if (enable)
>> +		sdhci_msm_bus_cancel_work_and_set_vote(host, bw);
>> +	else
>> +		sdhci_msm_bus_queue_work(host);
>> +}
>> +
>>  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,
>> @@ -1839,6 +2210,13 @@ 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->skip_bus_bw_voting)
>> +		goto clk_disable;
>> +
>> +	if (!msm_host->skip_bus_bw_voting)
>> +		sdhci_msm_bus_voting(host, 1);
>> +
>>  	if (!msm_host->mci_removed) {
>>  		core_memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>  		msm_host->core_mem = devm_ioremap_resource(&pdev->dev,
>> @@ -1846,7 +2224,7 @@ static int sdhci_msm_probe(struct 
>> platform_device *pdev)
>> 
>>  		if (IS_ERR(msm_host->core_mem)) {
>>  			ret = PTR_ERR(msm_host->core_mem);
>> -			goto clk_disable;
>> +			goto bus_unregister;
>>  		}
>>  	}
>> 
>> @@ -1918,7 +2296,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);
>> @@ -1931,7 +2309,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);
>> @@ -1956,6 +2334,11 @@ 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:
>> +	if (!msm_host->skip_bus_bw_voting) {
>> +		sdhci_msm_bus_cancel_work_and_set_vote(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);
>> @@ -1985,6 +2368,10 @@ 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);
>> +	if (!msm_host->skip_bus_bw_voting) {
>> +		sdhci_msm_bus_cancel_work_and_set_vote(host, 0);
>> +		sdhci_msm_bus_unregister(&pdev->dev, msm_host);
>> +	}
>>  	sdhci_pltfm_free(pdev);
>>  	return 0;
>>  }
>> --
>> 1.9.1
>> 

Re: [RFC 1/2] mmc: sdhci-msm: Add support for bus bandwidth voting

[Ulf Hansson]

On Wed, 25 Sep 2019 at 13:27, <ppvk@codeaurora.org> wrote:
>
> On 2019-09-12 22:15, Bjorn Andersson wrote:
> > On Fri 06 Sep 05:47 PDT 2019, Pradeep P V K wrote:
> >
> >> Vote for the MSM bus bandwidth 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)
> >>
> >> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> >> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> >> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> >> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
> >
> > This says that Sahitya wrote the patch, then Subhash handled it, then
> > Veerabhadrarao handled it and finally you handled it; but you're at the
> > same time listed as author (by From:).
> >
> > Please see section 12 on Co-Developed-by in submitting-patches.rst
> >
> Thanks Bjorn, i will update this on my next patch set.
> >> ---
> >>  drivers/mmc/host/sdhci-msm.c | 393
> >> ++++++++++++++++++++++++++++++++++++++++++-
> >
> > This patch implements support for requesting bandwidth to the register
> > space and for the controllers access to DDR. To me this seems like
> > common requirements for any mmc controller, can this functionality be
> > provided by the mmc/sdhci common code?
> >
> > Regards,
> > Bjorn
> >
> Yes, this can be provided in common code but the bandwidth calculations
> (arbitrated value or average bandwidth and instantaneous value or peak
> bandwidth) for bus vote will
> consider various parameters like voltage corners, clock domains, clock
> plans etc. which may differ from
> vendor to vendor and target to target. So, these values should be
> updated properly and correctly (considering all the parameters)
> if it brings to common area.
>
> Hence the reason for implementing this in sdhci-msm.c file.
> It would be really helpful if you could suggest your insights on where
> and how exactly this needs to be
> implemented in common code area.
>
> Hi Ulf and Adrian,
>
> Can you please also suggest your recommendations on this ?

I am not sure where to put it at this point. Perhaps we can just start
with making it specific for sdhci-msm, then when new users come into
play, we can consider moving it to a common place.

[...]

Kind regards
Uffe

[RFC 2/2] dt-bindings: mmc: sdhci-msm: Add Bus BW vote supported strings

[Pradeep P V K]

Add Bus bandwidth voting supported strings for qcom-sdhci controller.

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

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index da4edb1..8255d92 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -39,6 +39,25 @@ 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 bus bw voting:
+- interconnects: Pairs of phandles and interconnect provider specifier
+		 to denote the edge source and destination ports of
+		 the interconnect path. Please refer to
+		 Documentation/devicetree/bindings/interconnect/
+		 for more details.
+- interconnect-names: List of interconnect path name strings sorted in the same
+		order as the interconnects property. Consumers drivers will use
+		interconnect-names to match interconnect paths with interconnect
+		specifiers. Please refer to Documentation/devicetree/bindings/
+		interconnect/ for more details.
+- qcom,msm-bus,name: string describing the bus path
+- qcom,msm-bus,num-cases: number of configurations in which sdhc can operate in
+- qcom,msm-bus,num-paths: number of paths to vote for
+- qcom,msm-bus,vectors-KBps: Takes a tuple <ib ab>, <ib ab> (2 tuples for 2
+				num-paths) The number of these entries *must*
+				be same as num-cases.
+
 Example:
 
 	sdhc_1: sdhci@f9824900 {
@@ -56,6 +75,19 @@ Example:
 
 		clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>;
 		clock-names = "core", "iface";
+		interconnects = <&qnoc 50 &qnoc 512>,
+				<&qnoc 1 &qnoc 544>;
+		interconnect-names = "sdhc-ddr","cpu-sdhc";
+		qcom,msm-bus,name = "sdhc1";
+		qcom,msm-bus,num-cases = <3>;
+		qcom,msm-bus,num-paths = <2>;
+		qcom,msm-bus,vectors-KBps =
+		/* No Vote */
+		<0 0>, <0 0>,
+		/* 50 MB/s */
+		<130718 200000>, <133320 133320>,
+		/* 200 MB/s */
+		<1338562 4096000>, <1338562 4096000>;
 	};
 
 	sdhc_2: sdhci@f98a4900 {
-- 
1.9.1

Re: [RFC 2/2] dt-bindings: mmc: sdhci-msm: Add Bus BW vote supported strings

[Rob Herring]

On Fri, Sep 06, 2019 at 06:17:17PM +0530, Pradeep P V K wrote:
> Add Bus bandwidth voting supported strings for qcom-sdhci controller.

What is bus bandwidth voting?

> 
> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
> ---
>  .../devicetree/bindings/mmc/sdhci-msm.txt          | 32 ++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> index da4edb1..8255d92 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
> @@ -39,6 +39,25 @@ 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 bus bw voting:
> +- interconnects: Pairs of phandles and interconnect provider specifier
> +		 to denote the edge source and destination ports of
> +		 the interconnect path. Please refer to
> +		 Documentation/devicetree/bindings/interconnect/
> +		 for more details.
> +- interconnect-names: List of interconnect path name strings sorted in the same
> +		order as the interconnects property. Consumers drivers will use
> +		interconnect-names to match interconnect paths with interconnect
> +		specifiers. Please refer to Documentation/devicetree/bindings/
> +		interconnect/ for more details.

How many? What are the strings?

> +- qcom,msm-bus,name: string describing the bus path
> +- qcom,msm-bus,num-cases: number of configurations in which sdhc can operate in
> +- qcom,msm-bus,num-paths: number of paths to vote for
> +- qcom,msm-bus,vectors-KBps: Takes a tuple <ib ab>, <ib ab> (2 tuples for 2

ib and ab are what? Didn't we just add interconnect bindings for 
expressing bandwidth?

> +				num-paths) The number of these entries *must*
> +				be same as num-cases.

Are all these properties SDHCI specific or can we expect to get these 
for *all* the QCom blocks?

> +
>  Example:
>  
>  	sdhc_1: sdhci@f9824900 {
> @@ -56,6 +75,19 @@ Example:
>  
>  		clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>;
>  		clock-names = "core", "iface";
> +		interconnects = <&qnoc 50 &qnoc 512>,
> +				<&qnoc 1 &qnoc 544>;
> +		interconnect-names = "sdhc-ddr","cpu-sdhc";
> +		qcom,msm-bus,name = "sdhc1";
> +		qcom,msm-bus,num-cases = <3>;
> +		qcom,msm-bus,num-paths = <2>;
> +		qcom,msm-bus,vectors-KBps =
> +		/* No Vote */
> +		<0 0>, <0 0>,
> +		/* 50 MB/s */
> +		<130718 200000>, <133320 133320>,
> +		/* 200 MB/s */
> +		<1338562 4096000>, <1338562 4096000>;
>  	};
>  
>  	sdhc_2: sdhci@f98a4900 {
> -- 
> 1.9.1
> 

Re: [RFC 2/2] dt-bindings: mmc: sdhci-msm: Add Bus BW vote supported strings

[ppvk]

On 2019-09-13 20:06, Rob Herring wrote:
> On Fri, Sep 06, 2019 at 06:17:17PM +0530, Pradeep P V K wrote:
>> Add Bus bandwidth voting supported strings for qcom-sdhci controller.
> 
> What is bus bandwidth voting?

Controller is connected with its master using NOC and it controls its 
slaves using another NOC path.
So,controller have 2 NOC paths as below.
     a. CPU to Controller, This path is used to access the registers of 
controllers.
     b. Controller to DDR, This path is a data path, where data is 
read/write from/to DDR.
All data transfer will happen on these NOC's, which is shared between 
other peripherals.
In order to achieve required throughput (Data transfer Bandwidth) we put 
vote on these NOC's to
scale the NOC clocks to support required bandwidth.

Instantaneous bandwidth (ib) and Arbitrated bandwidth (ab) values are 
the values calculated (This involves various arch. specific parameters
like clock plans, voltage corners, etc. which varies from vendor to 
vendor and target to target)
to put vote on those noc's to achieve require throughput.

> 
>> 
>> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
>> ---
>>  .../devicetree/bindings/mmc/sdhci-msm.txt          | 32 
>> ++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt 
>> b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> index da4edb1..8255d92 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
>> @@ -39,6 +39,25 @@ 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 bus bw voting:
>> +- interconnects: Pairs of phandles and interconnect provider 
>> specifier
>> +		 to denote the edge source and destination ports of
>> +		 the interconnect path. Please refer to
>> +		 Documentation/devicetree/bindings/interconnect/
>> +		 for more details.
>> +- interconnect-names: List of interconnect path name strings sorted 
>> in the same
>> +		order as the interconnects property. Consumers drivers will use
>> +		interconnect-names to match interconnect paths with interconnect
>> +		specifiers. Please refer to Documentation/devicetree/bindings/
>> +		interconnect/ for more details.
> 
> How many? What are the strings?

As this is implemented using interconnect framework, "interconnects" and 
"interconnect-names" are required
and below qcom specific properties are required to calculate the ab and 
ib values.
> 
>> +- qcom,msm-bus,name: string describing the bus path
>> +- qcom,msm-bus,num-cases: number of configurations in which sdhc can 
>> operate in
>> +- qcom,msm-bus,num-paths: number of paths to vote for
>> +- qcom,msm-bus,vectors-KBps: Takes a tuple <ib ab>, <ib ab> (2 tuples 
>> for 2
> 
> ib and ab are what? Didn't we just add interconnect bindings for
> expressing bandwidth?

Instantaneous bandwidth (ib) is peak bandwidth and Arbitrated bandwidth 
(ab) is the Average bandwidth.
There is no interconnect binding node as such for expressing the 
bandwidth. Hence the reason to use the
above qcom nodes for parsing and storing the req. bandwidth.

> 
>> +				num-paths) The number of these entries *must*
>> +				be same as num-cases.
> 
> Are all these properties SDHCI specific or can we expect to get these
> for *all* the QCom blocks?
> 
As per the current implementation, these are some optional properties 
and is required
only when the bus bandwidth support is needed and all these are qcom 
specific.

>> +
>>  Example:
>> 
>>  	sdhc_1: sdhci@f9824900 {
>> @@ -56,6 +75,19 @@ Example:
>> 
>>  		clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>;
>>  		clock-names = "core", "iface";
>> +		interconnects = <&qnoc 50 &qnoc 512>,
>> +				<&qnoc 1 &qnoc 544>;
>> +		interconnect-names = "sdhc-ddr","cpu-sdhc";
>> +		qcom,msm-bus,name = "sdhc1";
>> +		qcom,msm-bus,num-cases = <3>;
>> +		qcom,msm-bus,num-paths = <2>;
>> +		qcom,msm-bus,vectors-KBps =
>> +		/* No Vote */
>> +		<0 0>, <0 0>,
>> +		/* 50 MB/s */
>> +		<130718 200000>, <133320 133320>,
>> +		/* 200 MB/s */
>> +		<1338562 4096000>, <1338562 4096000>;
>>  	};
>> 
>>  	sdhc_2: sdhci@f98a4900 {
>> --
>> 1.9.1
>>