Re: [<PATCH v1> 1/1] scsi: qcom-ufs: Add support for bus voting using ICB framework

[Georgi Djakov]

Hi Asutosh,

Thanks for the patch!

On 1/24/19 09:01, Asutosh Das wrote:
> Adapt to the new ICB framework for bus bandwidth voting.

It's actually called interconnect API or interconnect framework.

> This requires the source/destination port ids.
> Also this requires a tuple of values.
> 
> The tuple is for two different paths - from UFS master
> to BIMC slave. The other is from CPU master to UFS slave.
> This tuple consists of the average and peak bandwidth.
> 
> Signed-off-by: Asutosh Das <asutoshd@xxxxxxxxxxxxxx>
> ---

You can put here (below the --- line) the text from the cover letter.
Usually people do not add separate cover letters for single patches.

>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      |  12 ++
>  drivers/scsi/ufs/ufs-qcom.c                        | 234 ++++++++++++++++-----
>  drivers/scsi/ufs/ufs-qcom.h                        |  20 ++
>  3 files changed, 218 insertions(+), 48 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> index a99ed55..94249ef 100644
> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> @@ -45,6 +45,18 @@ Optional properties:
>  Note: If above properties are not defined it can be assumed that the supply
>  regulators or clocks are always on.
>  
> +* Following bus parameters are required:
> +interconnects
> +interconnect-names

Is the above really required? Are the interconnect bandwidth requests
required to enable something critical to UFS functionality?
Would UFS still work without any bandwidth scaling, although for example
slower? Could you please clarify.

> +- Please refer to Documentation/devicetree/bindings/interconnect/
> +  for more details on the above.
> +qcom,msm-bus,name - string describing the bus path
> +qcom,msm-bus,num-cases - number of configurations in which ufs 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.

DT bindings should be submitted as a separate patch. Anyway, people
frown upon putting configuration data in DT. Could we put this data into
the driver as a static table instead of DT? Also maybe use ab/pb for
average/peak bandwidth.

> +
>  Example:
>  	ufshc@0xfc598000 {
>  		compatible = "jedec,ufs-1.1";
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 2b38db2..213e975 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/phy/phy.h>
> +#include <linux/interconnect.h>
>  #include <linux/phy/phy-qcom-ufs.h>

Alphabetical order?

>  
>  #include "ufshcd.h"
> @@ -27,6 +28,10 @@
>  #define UFS_QCOM_DEFAULT_DBG_PRINT_EN	\
>  	(UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN)
>  
> +#define UFS_DDR "ufs-ddr"
> +#define CPU_UFS "cpu-ufs"

Not sure if it's really useful to have them as #defines. Also maybe use
"ufs-mem" instead of "ufs-ddr" to be more consistent with other users.

> +
> +
>  enum {
>  	TSTBUS_UAWM,
>  	TSTBUS_UARM,
> @@ -714,7 +719,6 @@ static int ufs_qcom_get_pwr_dev_param(struct ufs_qcom_dev_params *qcom_param,
>  	return 0;
>  }
>  
> -#ifdef CONFIG_MSM_BUS_SCALING
>  static int ufs_qcom_get_bus_vote(struct ufs_qcom_host *host,
>  		const char *speed_mode)
>  {
> @@ -768,24 +772,83 @@ static void ufs_qcom_get_speed_mode(struct ufs_pa_layer_attr *p, char *result)
>  	}
>  }
>  
> +static int ufs_qcom_get_ib_ab(struct ufs_qcom_host *host, int index,
> +			      struct qcom_bus_vectors *ufs_ddr_vec,
> +			      struct qcom_bus_vectors *cpu_ufs_vec)
> +{
> +	struct qcom_bus_path *usecase;
> +
> +	if (!host->qbsd)
> +		return -EINVAL;
> +
> +	if (index > host->qbsd->num_usecase)
> +		return -EINVAL;
> +
> +	usecase = host->qbsd->usecase;
> +
> +	/*
> +	 *
> +	 * usecase:0  usecase:0
> +	 * ufs->ddr   cpu->ufs
> +	 * |vec[0&1] | vec[2&3]|
> +	 * +----+----+----+----+
> +	 * | ab | ib | ab | ib |
> +	 * |----+----+----+----+
> +	 * .
> +	 * .
> +	 * .
> +	 * usecase:n  usecase:n
> +	 * ufs->ddr   cpu->ufs
> +	 * |vec[0&1] | vec[2&3]|
> +	 * +----+----+----+----+
> +	 * | ab | ib | ab | ib |
> +	 * |----+----+----+----+
> +	 */
> +
> +	/* index refers to offset in usecase */
> +	ufs_ddr_vec->ab = usecase[index].vec[0].ab;
> +	ufs_ddr_vec->ib = usecase[index].vec[0].ib;
> +
> +	cpu_ufs_vec->ab = usecase[index].vec[1].ab;
> +	cpu_ufs_vec->ib = usecase[index].vec[1].ib;
> +
> +	return 0;
> +}
> +
>  static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote)
>  {
>  	int err = 0;
> +	struct qcom_bus_scale_data *d = host->qbsd;
> +	struct qcom_bus_vectors path0, path1;
> +	struct device *dev = host->hba->dev;
>  
> -	if (vote != host->bus_vote.curr_vote) {
> -		err = msm_bus_scale_client_update_request(
> -				host->bus_vote.client_handle, vote);
> -		if (err) {
> -			dev_err(host->hba->dev,
> -				"%s: msm_bus_scale_client_update_request() failed: bus_client_handle=0x%x, vote=%d, err=%d\n",
> -				__func__, host->bus_vote.client_handle,
> -				vote, err);
> -			goto out;
> -		}
> +	err = ufs_qcom_get_ib_ab(host, vote, &path0, &path1);
> +	if (err) {
> +		dev_err(dev, "Error: failed (%d) to get ib/ab\n",
> +			err);
> +		return err;
> +	}
>  
> -		host->bus_vote.curr_vote = vote;
> +	dev_dbg(dev, "Setting vote: %d: ufs-ddr: ab: %llu ib: %llu\n", vote,
> +		path0.ab, path0.ib);
> +	err = icc_set_bw(d->ufs_ddr, path0.ab, path0.ib);
> +	if (err) {
> +		dev_err(dev, "Error: (%d) failed setting (%s) bus vote\n", err,
> +			UFS_DDR);
> +		return err;
>  	}
> -out:
> +
> +	dev_dbg(dev, "Setting: cpu-ufs: ab: %llu ib: %llu\n", path1.ab,
> +		path1.ib);
> +	err = icc_set(d->cpu_ufs, path1.ab, path1.ib);
> +	if (err) {
> +		dev_err(dev, "Error: (%d) failed setting (%s) bus vote\n", err,
> +			CPU_UFS);
> +		return err;
> +	}
> +
> +	host->bus_vote.curr_vote = vote;
> +
>  	return err;
>  }
>  
> @@ -807,6 +870,7 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)
>  		dev_err(host->hba->dev, "%s: failed %d\n", __func__, err);
>  	else
>  		host->bus_vote.saved_vote = vote;
> +
>  	return err;
>  }
>  
> @@ -837,34 +901,114 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)
>  	return count;
>  }
>  
> +static struct qcom_bus_scale_data *ufs_qcom_get_bus_scale_data(struct device
> +							       *dev)
> +
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct device_node *of_node = dev->of_node;
> +	struct qcom_bus_scale_data *qsd;
> +	struct qcom_bus_path *usecase = NULL;
> +	int ret = 0, i = 0, j, num_paths, len;
> +	const uint32_t *vec_arr = NULL;

Please use u32 instead of uint32_t.

> +	bool mem_err = false;
> +
> +	if (!pdev) {
> +		dev_err(dev, "Null platform device!\n");
> +		return NULL;
> +	}
> +
> +	qsd = devm_kzalloc(dev, sizeof(struct qcom_bus_scale_data), GFP_KERNEL);
> +	if (!qsd)
> +		return NULL;
> +
> +	ret = of_property_read_string(of_node, "qcom,msm-bus,name", &qsd->name);
> +	if (ret) {
> +		dev_err(dev, "Error: (%d) Bus name missing!\n", ret);
> +		return NULL;
> +	}
> +
> +	ret = of_property_read_u32(of_node, "qcom,msm-bus,num-cases",
> +		&qsd->num_usecase);
> +	if (ret) {
> +		pr_err("Error: num-usecases not found\n");
> +		goto err;
> +	}
> +
> +	usecase = devm_kzalloc(dev, (sizeof(struct qcom_bus_path) *
> +				   qsd->num_usecase), GFP_KERNEL);
> +	if (!usecase)
> +		return NULL;
> +
> +	ret = of_property_read_u32(of_node, "qcom,msm-bus,num-paths",
> +				   &num_paths);
> +	if (ret) {
> +		pr_err("Error: num_paths not found\n");
> +		return NULL;
> +	}
> +
> +	vec_arr = of_get_property(of_node, "qcom,msm-bus,vectors-KBps", &len);
> +	if (vec_arr == NULL) {
> +		pr_err("Error: Vector array not found\n");
> +		return NULL;
> +	}
> +
> +	for (i = 0; i < qsd->num_usecase; i++) {
> +		usecase[i].num_paths = num_paths;
> +		usecase[i].vec = devm_kzalloc(dev, num_paths *
> +					      sizeof(struct qcom_bus_vectors),
> +					      GFP_KERNEL);
> +		if (!usecase[i].vec) {
> +			mem_err = true;
> +			dev_err(dev, "Error: Failed to alloc mem for vectors\n");
> +			goto err;
> +		}
> +
> +		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]);
> +		}
> +	}
> +
> +	qsd->usecase = usecase;
> +	return qsd;
> +err:
> +	if (mem_err) {
> +		for (; i > 0; i--)
> +			kfree(usecase[i].vec);
> +	}
> +	return NULL;
> +}

We wouldn't need all the above DT parsing if we add a sdm845 bandwidth
usecase table. Could you give it a try?

Thanks,
Georgi