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

[Matthias Kaehlcke]

Hi Pradeep,

On Mon, Mar 23, 2020 at 07:55:01PM +0530, Pradeep P V K wrote:
> Add interconnect bandwidths for SDHC driver using OPP framework that
> is required by SDHC driver based on the clock frequency and bus width
> of the card. Otherwise, the system clocks may run at minimum clock
> speed and thus affecting the performance.
> 
> This change is based on
> [RFC] mmc: host: sdhci-msm: Use the interconnect API
> (https://lkml.org/lkml/2018/10/11/499) and
> 
> [PATCH v6] Introduce Bandwidth OPPs for interconnects
> (https://lkml.org/lkml/2019/12/6/740)
> 
> Co-developed-by: Sahitya Tummala <stummala@xxxxxxxxxxxxxx>
> Signed-off-by: Sahitya Tummala <stummala@xxxxxxxxxxxxxx>
> Co-developed-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@xxxxxxxxxxxxxx>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@xxxxxxxxxxxxxx>
> Co-developed-by: Pradeep P V K <ppvk@xxxxxxxxxxxxxx>
> Signed-off-by: Pradeep P V K <ppvk@xxxxxxxxxxxxxx>
> ---
> 
> RFC v5 -> v6:
> - Added new goto jump tag to put both icc paths.
> - Removed bus vote data error check and added is_null check.
> - Addressed minor code style comments.
> 
>  drivers/mmc/host/sdhci-msm.c | 240 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 236 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 09ff731..469db65 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
>
> ...
>
> +/*
> + * Helper function to parse the exact OPP node
> + * Returns OPP pointer on success else NULL on error
> + */
> +static struct dev_pm_opp
> +		*sdhci_msm_find_opp_for_freq(struct sdhci_msm_host *msm_host,
> +							unsigned long bw)
> +{
> +	struct dev_pm_opp *opp;
> +	struct sdhci_host *host = mmc_priv(msm_host->mmc);
> +	unsigned int freq = bw;
> +	struct device *dev = &msm_host->pdev->dev;
> +
> +

delete one empty line

> +	if (!freq)
> +		opp = dev_pm_opp_find_peak_bw_floor(dev, &freq);
> +	else
> +		opp = dev_pm_opp_find_peak_bw_exact(dev, freq, true);
> +
> +	/* Max bandwidth vote */
> +	if (PTR_ERR(opp) == -ERANGE && freq > sdhci_msm_get_max_clock(host))
> +		opp = dev_pm_opp_find_peak_bw_ceil(dev, &bw);
> +
> +	if (IS_ERR(opp)) {
> +		dev_err(dev, "Failed to find OPP for freq:%u err:%ld\n",
> +				freq, PTR_ERR(opp));
> +		return NULL;
> +	}
> +	return opp;
> +}
>
> ...
>
> +/*
> + * Helper function to register for OPP and interconnect
> + * frameworks.
> + */
> +static struct sdhci_msm_bus_vote_data
> +		*sdhci_msm_bus_register(struct sdhci_msm_host *host,
> +				struct platform_device *pdev)
> +{
> +	struct sdhci_msm_bus_vote_data *vote_data;
> +	struct device *dev = &pdev->dev;
> +	int i, err;
> +	struct icc_path *icc_paths[BUS_INTERCONNECT_PATHS];
> +	const char *path_names[] = {
> +		"sdhc-ddr",
> +		"cpu-sdhc",
> +	};
> +
> +	for (i = 0; i < BUS_INTERCONNECT_PATHS; i++)
> +		icc_paths[i] = of_icc_get(&pdev->dev, path_names[i]);
> +
> +	if (!icc_paths[0] && !icc_paths[1]) {
> +		dev_info(&pdev->dev, "ICC DT property is missing.Skip vote!!\n");
> +		return NULL;
> +	}
> +
> +	for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
> +		if (!icc_paths[i]) {
> +			dev_err(&pdev->dev, "interconnect path '%s' is not configured\n",
> +					path_names[i]);
> +			err = -EINVAL;
> +			goto handle_err;
> +		}
> +		if (IS_ERR(icc_paths[i])) {
> +			err = PTR_ERR(icc_paths[i]);
> +
> +			if (err != -EPROBE_DEFER)
> +				dev_err(&pdev->dev, "interconnect path '%s' is invalid:%d\n",
> +					path_names[i], err);
> +			goto handle_err;
> +		}
> +	}
> +
> +	err = dev_pm_opp_of_add_table(dev);
> +	if (err) {
> +		if (err == -ENODEV || err == -ENODATA)
> +			dev_err(dev, "OPP dt properties missing:%d\n", err);
> +		else
> +			dev_err(dev, "OPP registration failed:%d\n", err);
> +		goto put_icc;
> +	}
> +
> +	vote_data = devm_kzalloc(dev, sizeof(*vote_data), GFP_KERNEL);
> +	if (!vote_data) {
> +		err = -ENOMEM;
> +		goto put_icc;
> +	}
> +	vote_data->sdhc_to_ddr = icc_paths[0];
> +	vote_data->cpu_to_sdhc = icc_paths[1];
> +	return vote_data;
> +
> +handle_err:
> +	if (err) {

the check for 'err' is not needed, this code is only executed when an error
is encountered.

> +		int other = (i == 0) ? 1 : 0;
> +
> +		if (!IS_ERR_OR_NULL(icc_paths[other]))
> +			icc_put(icc_paths[other]);
> +	}
> +	return ERR_PTR(err);
> +
> +put_icc:
> +	if (err) {
> +		for (i = 0; i < BUS_INTERCONNECT_PATHS; i++)
> +			icc_put(icc_paths[i]);
> +	}
> +	return ERR_PTR(err);

The two error paths are somewhat redundant and the 'handle_err' isn't super
clear, especially since it is disconnected from the code where the error is
found.

You could have a single handler instead:

put_icc:
	for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
		if (!IS_ERR_OR_NULL(icc_paths[i]))
			icc_put(icc_paths[i]);
        }

	return ERR_PTR(err);