Re: [RFC v3 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support
From: ppvk
Date: Tue Feb 18 2020 - 06:11:36 EST
Hi Bjorn,
Thanks for the review !
On 2020-02-09 05:09, Bjorn Andersson wrote:
On Fri 07 Feb 06:44 PST 2020, 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 Georgi Djakov [RFC]
(https://lkml.org/lkml/2018/10/11/499) and
Saravana Kannan [PATCH v6]
(https://lkml.org/lkml/2019/12/6/740)
Write out the subject of these two patches, rather than stating that
it's based on one RFC and some PATCH version 6. Or just state that it's
based on work by Georgi and Saravana.
Sure. i will update this in my next patch set.
change from RFC v2 -> v3 changes:
- Modified interconnect bandwidth support using OPP framework.
- Addressed review comments on v2.
Move the changelog below the --- line
ok.
Signed-off-by: Sahitya Tummala <stummala@xxxxxxxxxxxxxx>
Signed-off-by: Subhash Jadavani <subhashj@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>
You should read these as the history of how this patch reached LKML and
can use Co-developed-by to indicate that several people was part of a
single step.
So if you're both co-author and the one sending it to LKML then you
should have 4 Co-developed-by tags here.
sure, i will follow this from my patch series.
---
drivers/mmc/host/sdhci-msm.c | 251
++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 247 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/sdhci-msm.c
b/drivers/mmc/host/sdhci-msm.c
index 71f29ba..f061cd8 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"
@@ -229,6 +231,14 @@ struct sdhci_msm_variant_info {
const struct sdhci_msm_offset *offset;
};
+struct sdhci_msm_bus_vote_data {
+
Move the { to the empty line.
ok.
+ struct icc_path *sdhc_to_ddr;
+ struct icc_path *cpu_to_sdhc;
+ bool skip_bus_bw_voting;
+ u32 curr_freq;
+};
+
struct sdhci_msm_host {
struct platform_device *pdev;
void __iomem *core_mem; /* MSM SDCC mapped address */
@@ -255,8 +265,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, u32
enable);
+
static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct
sdhci_host *host)
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -1564,6 +1577,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);
}
@@ -1685,6 +1699,219 @@ 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)
+{
+ unsigned long bw;
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+ bw = msm_host->clk_rate;
+
+ if (ios->bus_width == MMC_BUS_WIDTH_4)
+ bw /= 2;
+ else if (ios->bus_width == MMC_BUS_WIDTH_1)
+ bw /= 8;
+
+ return bw;
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:
return msm_host->clk_rate;
}
ok. i will do this in my next patch.
+}
+
+/*
+ * Helper function to parse the exact OPP node
+ * Returns OPP pointer on success else NULL on error
+ */
+static struct dev_pm_opp *find_opp_for_freq(struct sdhci_msm_host
*msm_host,
A function with this name doesn't belong in the sdhci-msm driver.
ok. i will update the function name accordingly.
+ 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;
+ 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 dev_pm_opp *opp;
+ unsigned long freq = bw;
+ unsigned long ib[BUS_INTERCONNECT_PATHS],
ab[BUS_INTERCONNECT_PATHS];
+ int ddr_rc = 0, cpu_rc = 0;
+
+ if (!msm_host->bus_vote_data->sdhc_to_ddr ||
+ !msm_host->bus_vote_data->cpu_to_sdhc)
Why not include this check in the calling code, which already checks
for
skip_bus_bw_voting?
This is actually redundant. This is not required here again.
skip_bus_bw_voting will set when
the handlers are NULL. so i will remove this in my next patch set.
+ return;
+
+ if (bw != vote_data->curr_freq) {
if (bw == vote_data->curr_freq)
return;
Will save you an indentation level in the rest of the function.
sure. i will make the change accordingly.
+ for (i = 0; i < BUS_INTERCONNECT_PATHS; i++) {
+ opp = find_opp_for_freq(msm_host, freq);
+ if (opp) {
+ ab[i] = dev_pm_opp_get_bw(opp, &ib[i]);
+ freq += 1; /* Next Band width vote */
Why do you request bw for sdhc-ddr and then bw+1 for cpu-sdhc?
for sdhci-msm, the OPP's for path sdhc-ddr and cpu-sdhc are same, to
avoid
OPP errors and warnings, i just added +1 for cpu-sdhc to differentiate
between
sdhc-ddr and cpu-sdhc OPPS. This needs to be taken care while adding
opp's in dt
for cpu-sdhc path.
+ dev_pm_opp_put(opp);
+ }
+ }
+ pr_debug("%s: freq:%d sdhc_to_ddr ab:%lu ib:%lu cpu_to_sdhc ab:%lu
ib:%lu\n",
+ mmc_hostname(host->mmc), bw, ab[0], ib[0],
+ ab[1], ib[1]);
You probably have compiler warnings here as not all code paths will get
here with ib and ab initialized.
True. i will make the change accordingly.
+ ddr_rc = icc_set_bw(vote_data->sdhc_to_ddr, 0, ib[0]);
+ cpu_rc = icc_set_bw(vote_data->cpu_to_sdhc, 0, ib[1]);
+ if (ddr_rc || cpu_rc) {
+ pr_err("%s: icc_set() failed ddr_rc_err:%d cpu_rc_err:%d\n",
dev_err()
ok.
+ mmc_hostname(host->mmc), ddr_rc, cpu_rc);
+ return;
+ }
+ vote_data->curr_freq = bw;
+ }
+}
+
+/*
+ * This function registers the device to OPP framework and
+ * parses few optional parameters from the device tree node.
+ * Returns NULL bvd pointer on error else a valid bvd pointer.
+ */
+static struct sdhci_msm_bus_vote_data
*sdhci_msm_get_bus_vote_data(struct device
+ *dev, struct sdhci_msm_host *host)
+
+{
+ struct platform_device *pdev = to_platform_device(dev);
Just pass the platform_device instead of passing &pdev->dev and then
container_of() it directly.
This function is not required here. i will move the necessary code to
the caller fn().
to look the code more simpler. i will make the changes accordingly in my
next patch series.
+ struct sdhci_msm_bus_vote_data *vote_data = NULL;
+ int ret = 0;
No need to initialize either vote_data or ret, given that they are both
written before read below.
same as above.
+
+ if (!pdev) {
How could this happen?
same as above.
+ dev_err(dev, "Null platform device!\n");
+ return NULL;
+ }
+ vote_data = devm_kzalloc(dev, sizeof(*vote_data), GFP_KERNEL);
+ if (!vote_data)
+ return vote_data;
+
+ ret = dev_pm_opp_of_add_table(dev);
+ if (ret) {
+ if (ret == -ENODEV || ret == -ENODATA) {
+ dev_dbg(dev, "OPP not found. Skip bus voting!!:%d\n",
+ ret);
+ vote_data->skip_bus_bw_voting = true;
+ } else {
+ dev_dbg(dev, "OPP regestration failed:%d\n", ret);
+ goto err;
+ }
+ }
+
+ return vote_data;
+err:
+ devm_kfree(dev, vote_data);
+ vote_data = NULL;
This is a local variable, no need to clear it.
same as above.
+ return vote_data;
+}
+
+/*
+ * Helper function to register for OPP and interconnect
+ * frameworks.
+ */
+static int 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 = 0;
+
+ vote_data = sdhci_msm_get_bus_vote_data(dev, host);
+ if (!vote_data) {
+ dev_dbg(&pdev->dev, "Failed to get bus_scale data\n");
+ host->bus_vote_data = NULL;
+ return -EINVAL;
+ }
+ host->bus_vote_data = vote_data;
+
+ vote_data->sdhc_to_ddr = of_icc_get(&pdev->dev, "sdhc-ddr");
+ if (!vote_data->sdhc_to_ddr) {
+ dev_dbg(&pdev->dev, "DT property for path %s missing\n",
+ "sdhc-ddr");
+ return -ENOENT;
+ } else if (IS_ERR(vote_data->sdhc_to_ddr) {
+ dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
+ PTR_ERR(vote_data->sdhc_to_ddr), "sdhc-ddr");
+ ret = PTR_ERR(vote_data->sdhc_to_ddr);
+ vote_data->sdhc_to_ddr = NULL;
+ return ret;
+ }
+ vote_data->cpu_to_sdhc = of_icc_get(&pdev->dev, "cpu-sdhc");
+ if (!vote_data->cpu_to_sdhc) {
Afaict there's no reason to hold onto sdhc_to_ddr or vote_data itself
if
this happens.
I think things would be cleaner if you make this function return the
vote_data for success, NULL if the information was omitted or ERR_PTR()
in case invalid data was given.
Assign this in the caller, check for IS_ERR() and in the places where
you want to know if you are doing bus voting or not you can just check
for host->bus_vote_data - no need to check sdhc_to_ddr, cpu_to_sdhc and
skip_bus_bw_voting individually..
And given that it's an both-or-nothing this function would be cleaner
if
you of_icc_get() both paths, then do error handling - and then follow
up
with the introduction of devm and bulk operations to the icc API.
Agree with your suggestions, will make this change in my next patch set.
+ dev_dbg(&pdev->dev, "DT property for path %s missing\n",
+ "cpu_to_sdhc");
+ return -ENOENT;
+ } else if (IS_ERR(vote_data->cpu_to_sdhc)) {
+ dev_dbg(&pdev->dev, "(%ld): failed getting %s path\n",
+ PTR_ERR(vote_data->cpu_to_sdhc), "cpu-sdhc");
+ ret = PTR_ERR(vote_data->cpu_to_sdhc);
+ vote_data->cpu_to_sdhc = NULL;
+ return ret;
+ }
+ return ret;
+}
+
+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 (vote_data->skip_bus_bw_voting ||
+ !vote_data->sdhc_to_ddr ||
+ !vote_data->cpu_to_sdhc)
+ return;
+
+ icc_put(vote_data->sdhc_to_ddr);
+ icc_put(vote_data->cpu_to_sdhc);
+}
+
+#define MSM_MMC_BUS_VOTING_DELAY 200 /* msecs */
This is unused.
ok. i will remove.
+
+static void sdhci_msm_bus_voting(struct sdhci_host *host, u32 enable)
Split this in an enable and a disable
Why to split into two functions, most of the code is same and would be
simpler
to use with the same name for both enable and disable.
+{
+ 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 (msm_host->bus_vote_data->skip_bus_bw_voting)
+ 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 const struct sdhci_msm_variant_ops mci_var_ops = {
.msm_readl_relaxed = sdhci_msm_mci_variant_readl_relaxed,
.msm_writel_relaxed = sdhci_msm_mci_variant_writel_relaxed,
Regards,
Bjorn