lkml.org 
[lkml]   [2020]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC v6 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support
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@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 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);

\
 
 \ /
  Last update: 2020-03-23 19:50    [W:0.081 / U:13.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site