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

[Matthias Kaehlcke <mka@chromium.org>]

Hi Pradeep,

On Thu, Mar 12, 2020 at 11:31:50AM +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@codeaurora.org>
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Co-developed-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Co-developed-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Signed-off-by: Veerabhadrarao Badiganti <vbadigan@codeaurora.org>
> Co-developed-by: Pradeep P V K <ppvk@codeaurora.org>
> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org>
> ---
> 
> RFC v4 -> v5:
> - Rewrote the icc interconnect get handlers and its error handling
>   and allocated vote data after handling all icc get handler errors.
> - Removed explicit error check on ICC handlers.
> - Addressed minor code style comments.
> 
>  drivers/mmc/host/sdhci-msm.c | 231 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 227 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 09ff731..5fe8fad 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
>
> ...
>
> +/*
> + * This function sets the interconnect bus bandwidth
> + * vote based on bw (bandwidth) argument.
> + */
> +#define BUS_INTERCONNECT_PATHS 2 /* 1. sdhc -> ddr 2. cpu -> sdhc */
> +static void sdhci_msm_bus_set_vote(struct sdhci_host *host,
> +						unsigned int bw)
> +{
> +	int i, err;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	struct sdhci_msm_bus_vote_data *vote_data = msm_host->bus_vote_data;
> +	struct device *dev = &msm_host->pdev->dev;
> +	struct dev_pm_opp *opp;
> +	unsigned long freq = bw;
> +	unsigned long peak_bw[BUS_INTERCONNECT_PATHS] = {0};
> +	unsigned long avg_bw[BUS_INTERCONNECT_PATHS] = {0};
> +
> +	if (bw == vote_data->curr_freq)
> +		return;
> +
> +	for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
> +		opp = sdhci_msm_find_opp_for_freq(msm_host, freq);
> +		if (opp) {
> +			avg_bw[i] = dev_pm_opp_get_bw(opp, &peak_bw[i]);
> +			freq += 1; /* Next bandwidth vote */
> +			dev_pm_opp_put(opp);
> +		}
> +	}
> +	pr_debug("%s: freq:%d sdhc_to_ddr avg_bw:%lu peak_bw:%lu cpu_to_sdhc avg_bw:%lu peak_bw:%lu\n",
> +			mmc_hostname(host->mmc), bw, avg_bw[0], peak_bw[0],
> +				avg_bw[1], peak_bw[1]);
> +	err = icc_set_bw(vote_data->sdhc_to_ddr, 0, peak_bw[0]);
> +	if (err) {
> +		dev_err(dev, "icc_set() failed for 'sdhc-ddr' path err:%d\n",
> +							err);

nit: the alignment is odd, either align with 'dev' or a tab after 'dev_err'

> +		return;
> +	}
> +	err = icc_set_bw(vote_data->cpu_to_sdhc, 0, peak_bw[1]);
> +	if (err) {
> +		dev_err(dev, "icc_set() failed for 'cpu-sdhc' path err:%d\n",
> +							err);

ditto

> +		return;
> +	}
> +	vote_data->curr_freq = bw;
> +}
> +
> +/*
> + * 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 ret, i, err;

nit: you can get rid of 'ret' and use 'err' instead.

> +	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;
> +		}
> +	}
> +
> +	ret = dev_pm_opp_of_add_table(dev);
> +	if (ret) {
> +		if (ret == -ENODEV || ret == -ENODATA)
> +			dev_err(dev, "OPP dt properties missing:%d\n", ret);
> +		else
> +			dev_err(dev, "OPP registration failed:%d\n", ret);

need to call icc_put() for the two paths?

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

ditto

probably you want to do this with a jump to an error handler below.

> +		return ERR_PTR(-ENOMEM);
> +
> +	vote_data->sdhc_to_ddr = icc_paths[0];
> +	vote_data->cpu_to_sdhc = icc_paths[1];
> +	return vote_data;

nit: add empty line

> +handle_err:
> +	if (err) {
> +		int other = (i == 0) ? 1 : 0;
> +
> +		if (!IS_ERR_OR_NULL(icc_paths[other]))
> +			icc_put(icc_paths[other]);
> +	}

doing this at the end (as opposed to my suggestion from
https://patchwork.kernel.org/patch/11388409/#23165321) has the advantage of
keeping the above loop cleaner from error handling cruft, on the downside it
is probably easier to understand right away in the context of the loop. I
guess you can do it either way.

It might get a bit more messy if you also handle the case where both paths are
valid. If that gets too involved I'd suggest to hnadle the above case inside
the loop.

> +	return ERR_PTR(err);
> +}
> +
> +static void sdhci_msm_bus_unregister(struct device *dev,
> +				struct sdhci_msm_host *host)
> +{
> +	struct sdhci_msm_bus_vote_data *vote_data = host->bus_vote_data;
> +
> +	if (IS_ERR_OR_NULL(vote_data))

I think 'if (!vote_data)' would be sufficient, since _probe() aborts in
case of an error.

> +		return;
> +
> +	icc_put(vote_data->sdhc_to_ddr);
> +	icc_put(vote_data->cpu_to_sdhc);
> +}
> +
> +static void sdhci_msm_bus_voting(struct sdhci_host *host, bool enable)
> +{
> +	struct mmc_ios *ios = &host->mmc->ios;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	unsigned int bw;
> +
> +	if (IS_ERR_OR_NULL(msm_host->bus_vote_data))
> +		return;

ditto