| Message ID | 1591175376-2374-2-git-send-email-ppvk@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add SDHC interconnect bandwidth scaling | expand |
Hey Pradeep, Thanks for the patch. On 2020-06-03 14:39, Pradeep P V K wrote: > Interconnect bandwidth scaling support is now added as a > part of OPP [1]. So, make sure interconnect driver is ready > before handling interconnect scaling. > > This change is based on > [1] [Patch v8] Introduce OPP bandwidth bindings > (https://lkml.org/lkml/2020/5/12/493) > > [2] [Patch v3] mmc: sdhci-msm: Fix error handling > for dev_pm_opp_of_add_table() > (https://lkml.org/lkml/2020/5/5/491) > > Signed-off-by: Pradeep P V K <ppvk@codeaurora.org> > --- > drivers/mmc/host/sdhci-msm.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-msm.c > b/drivers/mmc/host/sdhci-msm.c > index b277dd7..bf95484 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -14,6 +14,7 @@ > #include <linux/slab.h> > #include <linux/iopoll.h> > #include <linux/regulator/consumer.h> > +#include <linux/interconnect.h> > > #include "sdhci-pltfm.h" > #include "cqhci.h" > @@ -1999,6 +2000,7 @@ static int sdhci_msm_probe(struct platform_device > *pdev) > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_msm_host *msm_host; > struct clk *clk; > + struct icc_path *sdhc_path; > int ret; > u16 host_version, core_minor; > u32 core_version, config; > @@ -2070,6 +2072,20 @@ static int sdhci_msm_probe(struct > platform_device *pdev) > } > msm_host->bulk_clks[0].clk = clk; > > + /* Make sure that ICC driver is ready for interconnect bandwdith > + * scaling before registering the device for OPP. > + */ > + sdhc_path = of_icc_get(&pdev->dev, NULL); > + ret = PTR_ERR_OR_ZERO(sdhc_path); > + if (ret) { > + if (ret == -EPROBE_DEFER) > + dev_info(&pdev->dev, "defer icc path: %d\n", ret); > + else > + dev_err(&pdev->dev, "failed to get icc path:%d\n", ret); > + goto bus_clk_disable; > + } > + icc_put(sdhc_path); ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL); since there are multiple paths described in the bindings you should use ^^ instead and you can drop temporary path as well. > + > msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); > if (IS_ERR(msm_host->opp_table)) { > ret = PTR_ERR(msm_host->opp_table);
Hi Pradeep, On Wed, Jun 03, 2020 at 02:39:35PM +0530, Pradeep P V K wrote: > Interconnect bandwidth scaling support is now added as a > part of OPP [1]. So, make sure interconnect driver is ready > before handling interconnect scaling. > > This change is based on > [1] [Patch v8] Introduce OPP bandwidth bindings > (https://lkml.org/lkml/2020/5/12/493) > > [2] [Patch v3] mmc: sdhci-msm: Fix error handling > for dev_pm_opp_of_add_table() > (https://lkml.org/lkml/2020/5/5/491) > > Signed-off-by: Pradeep P V K <ppvk@codeaurora.org> > --- > drivers/mmc/host/sdhci-msm.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index b277dd7..bf95484 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -14,6 +14,7 @@ > #include <linux/slab.h> > #include <linux/iopoll.h> > #include <linux/regulator/consumer.h> > +#include <linux/interconnect.h> > > #include "sdhci-pltfm.h" > #include "cqhci.h" > @@ -1999,6 +2000,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) > struct sdhci_pltfm_host *pltfm_host; > struct sdhci_msm_host *msm_host; > struct clk *clk; > + struct icc_path *sdhc_path; nit: The 'sdhc_' prefix doesn't provide any useful information, change it to 'icc_path', which makes evident what it is? > int ret; > u16 host_version, core_minor; > u32 core_version, config; > @@ -2070,6 +2072,20 @@ static int sdhci_msm_probe(struct platform_device *pdev) > } > msm_host->bulk_clks[0].clk = clk; > > + /* Make sure that ICC driver is ready for interconnect bandwdith > + * scaling before registering the device for OPP. > + */ > + sdhc_path = of_icc_get(&pdev->dev, NULL); > + ret = PTR_ERR_OR_ZERO(sdhc_path); > + if (ret) { nit: IMO it would be clearer to do this instead of using PTR_ERR_OR_ZERO() (as for the OPP table below): if (IS_ERR(sdhc_path)) { ret = PTR_ERR(sdhc_path); > + if (ret == -EPROBE_DEFER) > + dev_info(&pdev->dev, "defer icc path: %d\n", ret); This log seems to add little more than noise, or are there particular reasons why it is useful in this driver? Most drivers just return silently in case of deferred probing. > + else > + dev_err(&pdev->dev, "failed to get icc path:%d\n", ret); > + goto bus_clk_disable; > + } > + icc_put(sdhc_path); > + > msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); > if (IS_ERR(msm_host->opp_table)) { > ret = PTR_ERR(msm_host->opp_table); > -- > 1.9.1 >
Hi Sibi, Thanks for the review!! On 2020-06-03 17:22, Sibi Sankar wrote: > Hey Pradeep, > Thanks for the patch. > > On 2020-06-03 14:39, Pradeep P V K wrote: >> Interconnect bandwidth scaling support is now added as a >> part of OPP [1]. So, make sure interconnect driver is ready >> before handling interconnect scaling. >> >> This change is based on >> [1] [Patch v8] Introduce OPP bandwidth bindings >> (https://lkml.org/lkml/2020/5/12/493) >> >> [2] [Patch v3] mmc: sdhci-msm: Fix error handling >> for dev_pm_opp_of_add_table() >> (https://lkml.org/lkml/2020/5/5/491) >> >> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org> >> --- >> drivers/mmc/host/sdhci-msm.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci-msm.c >> b/drivers/mmc/host/sdhci-msm.c >> index b277dd7..bf95484 100644 >> --- a/drivers/mmc/host/sdhci-msm.c >> +++ b/drivers/mmc/host/sdhci-msm.c >> @@ -14,6 +14,7 @@ >> #include <linux/slab.h> >> #include <linux/iopoll.h> >> #include <linux/regulator/consumer.h> >> +#include <linux/interconnect.h> >> >> #include "sdhci-pltfm.h" >> #include "cqhci.h" >> @@ -1999,6 +2000,7 @@ static int sdhci_msm_probe(struct >> platform_device *pdev) >> struct sdhci_pltfm_host *pltfm_host; >> struct sdhci_msm_host *msm_host; >> struct clk *clk; >> + struct icc_path *sdhc_path; >> int ret; >> u16 host_version, core_minor; >> u32 core_version, config; >> @@ -2070,6 +2072,20 @@ static int sdhci_msm_probe(struct >> platform_device *pdev) >> } >> msm_host->bulk_clks[0].clk = clk; >> >> + /* Make sure that ICC driver is ready for interconnect bandwdith >> + * scaling before registering the device for OPP. >> + */ >> + sdhc_path = of_icc_get(&pdev->dev, NULL); >> + ret = PTR_ERR_OR_ZERO(sdhc_path); >> + if (ret) { >> + if (ret == -EPROBE_DEFER) >> + dev_info(&pdev->dev, "defer icc path: %d\n", ret); >> + else >> + dev_err(&pdev->dev, "failed to get icc path:%d\n", ret); >> + goto bus_clk_disable; >> + } >> + icc_put(sdhc_path); > > ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL); > > since there are multiple paths > described in the bindings you > should use ^^ instead and you > can drop temporary path as well. > Ok. of_icc_get() used above is only to test if ICC driver is ready or not. I'm not really using the multiple paths here. Anyhow i will use dev_pm_opp_of_find_icc_paths() to get rid of some extra lines of code. >> + >> msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); >> if (IS_ERR(msm_host->opp_table)) { >> ret = PTR_ERR(msm_host->opp_table);
On 2020-06-04 16:43, ppvk@codeaurora.org wrote: > Hi Sibi, > > Thanks for the review!! > > On 2020-06-03 17:22, Sibi Sankar wrote: >> Hey Pradeep, >> Thanks for the patch. >> >> On 2020-06-03 14:39, Pradeep P V K wrote: >>> Interconnect bandwidth scaling support is now added as a >>> part of OPP [1]. So, make sure interconnect driver is ready >>> before handling interconnect scaling. >>> >>> This change is based on >>> [1] [Patch v8] Introduce OPP bandwidth bindings >>> (https://lkml.org/lkml/2020/5/12/493) >>> >>> [2] [Patch v3] mmc: sdhci-msm: Fix error handling >>> for dev_pm_opp_of_add_table() >>> (https://lkml.org/lkml/2020/5/5/491) >>> >>> Signed-off-by: Pradeep P V K <ppvk@codeaurora.org> >>> --- >>> drivers/mmc/host/sdhci-msm.c | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/drivers/mmc/host/sdhci-msm.c >>> b/drivers/mmc/host/sdhci-msm.c >>> index b277dd7..bf95484 100644 >>> --- a/drivers/mmc/host/sdhci-msm.c >>> +++ b/drivers/mmc/host/sdhci-msm.c >>> @@ -14,6 +14,7 @@ >>> #include <linux/slab.h> >>> #include <linux/iopoll.h> >>> #include <linux/regulator/consumer.h> >>> +#include <linux/interconnect.h> >>> >>> #include "sdhci-pltfm.h" >>> #include "cqhci.h" >>> @@ -1999,6 +2000,7 @@ static int sdhci_msm_probe(struct >>> platform_device *pdev) >>> struct sdhci_pltfm_host *pltfm_host; >>> struct sdhci_msm_host *msm_host; >>> struct clk *clk; >>> + struct icc_path *sdhc_path; >>> int ret; >>> u16 host_version, core_minor; >>> u32 core_version, config; >>> @@ -2070,6 +2072,20 @@ static int sdhci_msm_probe(struct >>> platform_device *pdev) >>> } >>> msm_host->bulk_clks[0].clk = clk; >>> >>> + /* Make sure that ICC driver is ready for interconnect bandwdith >>> + * scaling before registering the device for OPP. >>> + */ >>> + sdhc_path = of_icc_get(&pdev->dev, NULL); >>> + ret = PTR_ERR_OR_ZERO(sdhc_path); >>> + if (ret) { >>> + if (ret == -EPROBE_DEFER) >>> + dev_info(&pdev->dev, "defer icc path: %d\n", ret); >>> + else >>> + dev_err(&pdev->dev, "failed to get icc path:%d\n", ret); >>> + goto bus_clk_disable; >>> + } >>> + icc_put(sdhc_path); >> >> ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL); >> >> since there are multiple paths >> described in the bindings you >> should use ^^ instead and you >> can drop temporary path as well. >> > Ok. of_icc_get() used above is only to test if ICC driver is ready or > not. I'm not > really using the multiple paths here. Anyhow i will use > dev_pm_opp_of_find_icc_paths() > to get rid of some extra lines of code. Using dev_pm_opp_of_find_icc_paths with NULL passed acts as a way of validating all the paths specified in the device and also validates if the opp-table has bw related bindings as well. > >>> + >>> msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); >>> if (IS_ERR(msm_host->opp_table)) { >>> ret = PTR_ERR(msm_host->opp_table);
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index b277dd7..bf95484 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -14,6 +14,7 @@ #include <linux/slab.h> #include <linux/iopoll.h> #include <linux/regulator/consumer.h> +#include <linux/interconnect.h> #include "sdhci-pltfm.h" #include "cqhci.h" @@ -1999,6 +2000,7 @@ static int sdhci_msm_probe(struct platform_device *pdev) struct sdhci_pltfm_host *pltfm_host; struct sdhci_msm_host *msm_host; struct clk *clk; + struct icc_path *sdhc_path; int ret; u16 host_version, core_minor; u32 core_version, config; @@ -2070,6 +2072,20 @@ static int sdhci_msm_probe(struct platform_device *pdev) } msm_host->bulk_clks[0].clk = clk; + /* Make sure that ICC driver is ready for interconnect bandwdith + * scaling before registering the device for OPP. + */ + sdhc_path = of_icc_get(&pdev->dev, NULL); + ret = PTR_ERR_OR_ZERO(sdhc_path); + if (ret) { + if (ret == -EPROBE_DEFER) + dev_info(&pdev->dev, "defer icc path: %d\n", ret); + else + dev_err(&pdev->dev, "failed to get icc path:%d\n", ret); + goto bus_clk_disable; + } + icc_put(sdhc_path); + msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); if (IS_ERR(msm_host->opp_table)) { ret = PTR_ERR(msm_host->opp_table);
Interconnect bandwidth scaling support is now added as a part of OPP [1]. So, make sure interconnect driver is ready before handling interconnect scaling. This change is based on [1] [Patch v8] Introduce OPP bandwidth bindings (https://lkml.org/lkml/2020/5/12/493) [2] [Patch v3] mmc: sdhci-msm: Fix error handling for dev_pm_opp_of_add_table() (https://lkml.org/lkml/2020/5/5/491) Signed-off-by: Pradeep P V K <ppvk@codeaurora.org> --- drivers/mmc/host/sdhci-msm.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)