* [RFC v5 0/2] Add SDHC interconnect bandwidth scaling @ 2020-03-12 6:01 Pradeep P V K 2020-03-12 6:01 ` [RFC v5 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support Pradeep P V K 2020-03-12 6:01 ` [RFC v5 2/2] dt-bindings: mmc: sdhci-msm: Add interconnect BW scaling strings Pradeep P V K 0 siblings, 2 replies; 5+ messages in thread From: Pradeep P V K @ 2020-03-12 6:01 UTC (permalink / raw) To: bjorn.andersson, adrian.hunter, robh+dt, ulf.hansson, asutoshd, stummala, sayalil, rampraka, vbadigan, sboyd, georgi.djakov, mka Cc: linux-mmc, linux-kernel, linux-arm-msm, devicetree, agross, linux-mmc-owner, Pradeep P V K 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) Pradeep P V K (2): mmc: sdhci-msm: Add interconnect bus bandwidth scaling support dt-bindings: mmc: sdhci-msm: Add interconnect BW scaling strings .../devicetree/bindings/mmc/sdhci-msm.txt | 18 ++ drivers/mmc/host/sdhci-msm.c | 231 ++++++++++++++++++++- 2 files changed, 245 insertions(+), 4 deletions(-) -- RFC v4 -> v5: - Added Rob's Acked-by and Bjorn Reviewed-by for the DT patch. - 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. 1.9.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC v5 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support 2020-03-12 6:01 [RFC v5 0/2] Add SDHC interconnect bandwidth scaling Pradeep P V K @ 2020-03-12 6:01 ` Pradeep P V K 2020-03-12 20:49 ` Matthias Kaehlcke 2020-03-12 6:01 ` [RFC v5 2/2] dt-bindings: mmc: sdhci-msm: Add interconnect BW scaling strings Pradeep P V K 1 sibling, 1 reply; 5+ messages in thread From: Pradeep P V K @ 2020-03-12 6:01 UTC (permalink / raw) To: bjorn.andersson, adrian.hunter, robh+dt, ulf.hansson, asutoshd, stummala, sayalil, rampraka, vbadigan, sboyd, georgi.djakov, mka Cc: linux-mmc, linux-kernel, linux-arm-msm, devicetree, agross, linux-mmc-owner, Pradeep P V K, Subhash Jadavani 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 @@ -11,8 +11,10 @@ #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> +#include <linux/pm_opp.h> #include "sdhci-pltfm.h" #include "cqhci.h" @@ -234,6 +236,12 @@ struct sdhci_msm_variant_info { const struct sdhci_msm_offset *offset; }; +struct sdhci_msm_bus_vote_data { + struct icc_path *sdhc_to_ddr; + struct icc_path *cpu_to_sdhc; + u32 curr_freq; +}; + struct sdhci_msm_host { struct platform_device *pdev; void __iomem *core_mem; /* MSM SDCC mapped address */ @@ -260,8 +268,11 @@ struct sdhci_msm_host { bool use_cdr; u32 transfer_mode; bool updated_ddr_cfg; + struct sdhci_msm_bus_vote_data *bus_vote_data; }; +static void sdhci_msm_bus_voting(struct sdhci_host *host, bool enable); + static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host) { struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); @@ -1581,6 +1592,7 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock) msm_set_clock_rate_for_bus_mode(host, clock); out: + sdhci_msm_bus_voting(host, !!clock); __sdhci_msm_set_clock(host, clock); } @@ -1823,6 +1835,201 @@ 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); } +/* + * Returns required bandwidth in Bytes per Sec + */ +static unsigned long sdhci_get_bw_required(struct sdhci_host *host, + struct mmc_ios *ios) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); + + switch (ios->bus_width) { + case MMC_BUS_WIDTH_1: + return msm_host->clk_rate / 8; + case MMC_BUS_WIDTH_4: + return msm_host->clk_rate / 2; + case MMC_BUS_WIDTH_8: + break; + } + return msm_host->clk_rate; +} + +/* + * 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; + + + 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; +} + +/* + * 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); + 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); + 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; + 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); + return ERR_PTR(ret); + } + + vote_data = devm_kzalloc(dev, sizeof(*vote_data), GFP_KERNEL); + if (!vote_data) + return ERR_PTR(-ENOMEM); + + vote_data->sdhc_to_ddr = icc_paths[0]; + vote_data->cpu_to_sdhc = icc_paths[1]; + return vote_data; +handle_err: + if (err) { + int other = (i == 0) ? 1 : 0; + + if (!IS_ERR_OR_NULL(icc_paths[other])) + icc_put(icc_paths[other]); + } + 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)) + 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; + + if (enable) { + bw = sdhci_get_bw_required(host, ios); + sdhci_msm_bus_set_vote(host, bw); + } else + sdhci_msm_bus_set_vote(host, 0); +} + static void sdhci_msm_reset(struct sdhci_host *host, u8 mask) { if ((host->mmc->caps2 & MMC_CAP2_CQE) && (mask & SDHCI_RESET_ALL)) @@ -1992,11 +2199,20 @@ static int sdhci_msm_probe(struct platform_device *pdev) dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret); } + msm_host->bus_vote_data = sdhci_msm_bus_register(msm_host, pdev); + if (IS_ERR(msm_host->bus_vote_data)) { + ret = PTR_ERR(msm_host->bus_vote_data); + dev_err(&pdev->dev, "Bus registration failed (%d)\n", ret); + goto clk_disable; + } + + sdhci_msm_bus_voting(host, true); + if (!msm_host->mci_removed) { msm_host->core_mem = devm_platform_ioremap_resource(pdev, 1); if (IS_ERR(msm_host->core_mem)) { ret = PTR_ERR(msm_host->core_mem); - goto clk_disable; + goto bus_unregister; } } @@ -2071,7 +2287,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); @@ -2084,7 +2300,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); @@ -2112,6 +2328,9 @@ 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: + sdhci_msm_bus_voting(host, false); + sdhci_msm_bus_unregister(&pdev->dev, msm_host); clk_disable: clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks), msm_host->bulk_clks); @@ -2141,6 +2360,9 @@ 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); + + sdhci_msm_bus_voting(host, false); + sdhci_msm_bus_unregister(&pdev->dev, msm_host); sdhci_pltfm_free(pdev); return 0; } @@ -2153,7 +2375,7 @@ static __maybe_unused int sdhci_msm_runtime_suspend(struct device *dev) clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks), msm_host->bulk_clks); - + sdhci_msm_bus_voting(host, false); return 0; } @@ -2175,6 +2397,7 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) if (msm_host->restore_dll_config && msm_host->clk_rate) return sdhci_msm_restore_sdr_dll_config(host); + sdhci_msm_bus_voting(host, true); return 0; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC v5 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support 2020-03-12 6:01 ` [RFC v5 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support Pradeep P V K @ 2020-03-12 20:49 ` Matthias Kaehlcke 2020-03-22 16:01 ` ppvk 0 siblings, 1 reply; 5+ messages in thread From: Matthias Kaehlcke @ 2020-03-12 20:49 UTC (permalink / raw) To: Pradeep P V K Cc: bjorn.andersson, adrian.hunter, robh+dt, ulf.hansson, asutoshd, stummala, sayalil, rampraka, vbadigan, sboyd, georgi.djakov, linux-mmc, linux-kernel, linux-arm-msm, devicetree, agross, linux-mmc-owner, Subhash Jadavani 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC v5 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support 2020-03-12 20:49 ` Matthias Kaehlcke @ 2020-03-22 16:01 ` ppvk 0 siblings, 0 replies; 5+ messages in thread From: ppvk @ 2020-03-22 16:01 UTC (permalink / raw) To: Matthias Kaehlcke Cc: bjorn.andersson, adrian.hunter, robh+dt, ulf.hansson, asutoshd, stummala, sayalil, rampraka, vbadigan, sboyd, georgi.djakov, linux-mmc, linux-kernel, linux-arm-msm, devicetree, agross, linux-mmc-owner, Subhash Jadavani On 2020-03-13 02:19, Matthias Kaehlcke wrote: Hi Matthias, Thanks for reviewing. > 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' > sure, will do this in my next patch set. >> + 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 > ok. >> + 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. > ok. i will do this in my next patchset. >> + 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? > hmm yes. coming here means, we got both icc paths without error. So, should put both icc paths. I will handle this by adding a new goto jump tag. >> + 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. > ok. i will handle this by adding a new goto jump tag. >> + 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 > ok. >> +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. > True. i will leave this error handling at bottom as the code looks more cleaner. > 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. > hmm true, handling the above error in loop making the code more messy so, i'm using a new tag (put_icc) to put both the icc paths. >> + 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. > ok. i will handle this in my next patch set. >> + 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 ok. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [RFC v5 2/2] dt-bindings: mmc: sdhci-msm: Add interconnect BW scaling strings 2020-03-12 6:01 [RFC v5 0/2] Add SDHC interconnect bandwidth scaling Pradeep P V K 2020-03-12 6:01 ` [RFC v5 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support Pradeep P V K @ 2020-03-12 6:01 ` Pradeep P V K 1 sibling, 0 replies; 5+ messages in thread From: Pradeep P V K @ 2020-03-12 6:01 UTC (permalink / raw) To: bjorn.andersson, adrian.hunter, robh+dt, ulf.hansson, asutoshd, stummala, sayalil, rampraka, vbadigan, sboyd, georgi.djakov, mka Cc: linux-mmc, linux-kernel, linux-arm-msm, devicetree, agross, linux-mmc-owner, Pradeep P V K Add interconnect bandwidth scaling supported strings for qcom-sdhci controller. Signed-off-by: Pradeep P V K <ppvk@codeaurora.org> Acked-by: Rob Herring <robh@kernel.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- RFC v4 -> v5: - No changes. Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt index 5445931..9eafc41 100644 --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt @@ -46,6 +46,21 @@ 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 interconnect bandwidth scaling: +- interconnects: Pairs of phandles and interconnect provider specifier + to denote the edge source and destination ports of + the interconnect path. + +- interconnect-names: For sdhc, we have two main paths. + 1. Data path : sdhc to ddr + 2. Config path : cpu to sdhc + For Data interconnect path the name supposed to be + is "sdhc-ddr" and for config interconnect path it is + "cpu-sdhc". + Please refer to Documentation/devicetree/bindings/ + interconnect/ for more details. + Example: sdhc_1: sdhci@f9824900 { @@ -63,6 +78,9 @@ Example: clocks = <&gcc GCC_SDCC1_APPS_CLK>, <&gcc GCC_SDCC1_AHB_CLK>; clock-names = "core", "iface"; + interconnects = <&qnoc MASTER_SDCC_ID &qnoc SLAVE_DDR_ID>, + <&qnoc MASTER_CPU_ID &qnoc SLAVE_SDCC_ID>; + interconnect-names = "sdhc-ddr","cpu-sdhc"; }; sdhc_2: sdhci@f98a4900 { -- 1.9.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-22 16:02 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-12 6:01 [RFC v5 0/2] Add SDHC interconnect bandwidth scaling Pradeep P V K 2020-03-12 6:01 ` [RFC v5 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support Pradeep P V K 2020-03-12 20:49 ` Matthias Kaehlcke 2020-03-22 16:01 ` ppvk 2020-03-12 6:01 ` [RFC v5 2/2] dt-bindings: mmc: sdhci-msm: Add interconnect BW scaling strings Pradeep P V K
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.