Add SDHC interconnect bandwidth scaling
Subsystems (4)
v3 cover Add SDHC interconnect bandwidth scaling
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)
change from RFC v2 -> v3 changes:
- Modified interconnect bandwidth support using OPP framework.
- Addressed review comments on v2.
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 | 251 ++++++++++++++++++++- 2 files changed, 265 insertions(+), 4 deletions(-)
Signature · 1 line
1.9.1
RFC v3 1/2 mmc: sdhci-msm: Add interconnect bus bandwidth scaling support
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)
change from RFC v2 -> v3 changes:
- Modified interconnect bandwidth support using OPP framework.
- Addressed review comments on v2.
--- drivers/mmc/host/sdhci-msm.c | 251 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 247 insertions(+), 4 deletions(-)
drivers/mmc/host/sdhci-msm.c +247 -4
@@ -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 { + +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; +} + +/* +* 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, +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) +return ; + +if (bw != vote_data->curr_freq) { +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 */ +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 ]); + 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" , +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); +struct sdhci_msm_bus_vote_data *vote_data =NULL ; +int ret =0 ; + +if (!pdev) { +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 ; +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) { +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 */ + +static void sdhci_msm_bus_voting (struct sdhci_host *host,u32 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 (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,
@@ -1845,11 +2072,20 @@ static int sdhci_msm_probe(struct platform_device *pdev)dev_warn (&pdev->dev,"TCXO clk not present (%d)\n" , ret); } + ret =sdhci_msm_bus_register (msm_host, pdev); +if (ret && msm_host->bus_vote_data && + !msm_host->bus_vote_data->skip_bus_bw_voting) { +dev_err (&pdev->dev,"Bus registration failed (%d)\n" , ret); + goto clk_disable; + } + +sdhci_msm_bus_voting (host,1 ); +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; } }
@@ -1924,7 +2160,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);
@@ -1937,7 +2173,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);
@@ -1962,6 +2198,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,0 ); +sdhci_msm_bus_unregister (&pdev->dev, msm_host); clk_disable:clk_bulk_disable_unprepare (ARRAY_SIZE (msm_host->bulk_clks), msm_host->bulk_clks);
@@ -1991,6 +2230,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,0 ); +sdhci_msm_bus_unregister (&pdev->dev, msm_host);sdhci_pltfm_free (pdev);return 0 ; }
@@ -2003,7 +2245,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,0 );return 0 ; }
@@ -2025,6 +2267,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,1 );return 0 ; }
Signature · 1 line
1.9.1
Re: [RFC v3 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support
Hi Pradeep,
Quoted 40 lines · 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 Georgi Djakov [RFC]
(https://lkml.org/lkml/2018/10/11/499) andSaravana Kannan [PATCH v6]
(https://lkml.org/lkml/2019/12/6/740)change from RFC v2 -> v3 changes:
- Modified interconnect bandwidth support using OPP framework.
- Addressed review comments on v2.Signed-off-by Sahitya Tummala <[email protected]>Signed-off-by Subhash Jadavani <[email protected]>Signed-off-by Veerabhadrarao Badiganti <[email protected]>Co-developed-by Pradeep P V K <[email protected]>Signed-off-by Pradeep P V K <[email protected]>--- drivers/mmc/host/sdhci-msm.c | 251 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 247 insertions(+), 4 deletions(-)drivers/mmc/host/sdhci-msm.c +2
-0@@ -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"
drivers/mmc/host/sdhci-msm.c struct sdhci_msm_variant_info { · line 238
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c +5
-0@@ -229,6 +231,14 @@ 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; +bool skip_bus_bw_voting;
nit: the name is somewhat redundant given the name of the containing struct. 'skip_vote' or 'skip_voting' would be sufficient IMO.
Quoted 31 lines
drivers/mmc/host/sdhci-msm.c +8
-0+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); } +/*
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1712
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+* 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;
nit: you could declare 'bw' last and do the assignment together with the declaration.
Quoted 18 lines
drivers/mmc/host/sdhci-msm.c +18
-0+ +if (ios->bus_width ==MMC_BUS_WIDTH_4 ) + bw /=2 ; +else if (ios->bus_width ==MMC_BUS_WIDTH_1 ) + bw /=8 ; + +return bw; +} + +/* +* 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, +unsigned long bw) +{ +struct dev_pm_opp *opp; +struct sdhci_host *host =mmc_priv (msm_host->mmc);
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1740
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+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 */
s/Bandwidth/bandwidth/
Quoted 16 lines
drivers/mmc/host/sdhci-msm.c +16
-0+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 */
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1766
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+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 ];
the names are not very expressive, how about peak_bw and avg_bw?
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1767
Quoted 1 line
drivers/mmc/host/sdhci-msm.c +1
-0+int ddr_rc =0 , cpu_rc =0 ;
initialization is not needed
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1773
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+ +if (!msm_host->bus_vote_data->sdhc_to_ddr || + !msm_host->bus_vote_data->cpu_to_sdhc) +return ; + +if (bw != vote_data->curr_freq) {
if (bw == vote_data->curr_freq)
return;
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1778
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+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 */
s/Band width/bandwidth/
Quoted 19 lines
drivers/mmc/host/sdhci-msm.c +19
-0+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 ]); + 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" , +mmc_hostname (host->mmc), ddr_rc, cpu_rc); +return ; + } + vote_data->curr_freq = bw; + } +} + +/* +* This function registers the device to OPP framework and
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1807
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+* 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); +struct sdhci_msm_bus_vote_data *vote_data =NULL ; +int ret =0 ;
initialization of 'vote_data' and 'ret' is not needed.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1812
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+ +if (!pdev) { +dev_err (dev,"Null platform device!\n" ); +return NULL ; + }
It's probably safe to assume that the device is a platform device?
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1815
Quoted 3 lines
drivers/mmc/host/sdhci-msm.c +3
-0+ vote_data =devm_kzalloc (dev,sizeof (*vote_data),GFP_KERNEL ); +if (!vote_data) +return vote_data;
return ERR_PTR(-ENOMEM); ?
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1824
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c +9
-0+ + 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);
s/regestration/registration/
Depending on the consequences of this failure (e.g. probe() fails) you probably want to use dev_warn() or dev_err() here.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1832
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c +8
-0+ goto err; + } + } + +return vote_data; +err: +devm_kfree (dev, vote_data); + vote_data =NULL ;
not needed
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1833
Quoted 1 line
drivers/mmc/host/sdhci-msm.c +1
-0+return vote_data;
return NULL;
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+} + +/* +* Helper function to register for OPP and interconnect +* frameworks. +*/
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1849
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+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" );
In general I'm not convinced about the use of dev_dbg() in the error paths. Maybe there is some signal you can use to determine if there should be a valid bus scaling information (e.g. some DT property exists). If it exists you proceed with obtaining the necessary information and complain loudly if something goes wrong, otherwise you don't even try to obtain it and omit the debug logs.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1850
Quoted 1 line
drivers/mmc/host/sdhci-msm.c +1
-0+ host->bus_vote_data =NULL ;
IIUC struct sdhci_msm_host is zero initialized by sdhci_pltfm_init(), hence the assignment to NULL is not needed.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1851
Quoted 1 line
drivers/mmc/host/sdhci-msm.c +1
-0+return -EINVAL ;
Use the error returned by sdhci_msm_get_bus_vote_data().
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1855
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +4
-0+ } + host->bus_vote_data = vote_data; + + vote_data->sdhc_to_ddr =of_icc_get (&pdev->dev,"sdhc-ddr" );
This could be the signal I mentioned above. If this entry does not exist it is pointless to proceed. You could check this before sdhci_msm_get_bus_vote_data() and abort with a dev_info() message in case it doesn't exist. If it exists any error afterwards will be reported with dev_err().
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1865
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+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;
if there is an error probe() should probably be aborted. If that's the
case you can replace the above three lines with 'return PTR_ERR(vote_data->sdhc_to_ddr);'
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1877
Quoted 12 lines
drivers/mmc/host/sdhci-msm.c +12
-0+ } + vote_data->cpu_to_sdhc =of_icc_get (&pdev->dev,"cpu-sdhc" ); +if (!vote_data->cpu_to_sdhc) { +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;
same comment as above
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1879
Quoted 2 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ } +return ret;
no error if we get to this point, hence:
return 0;
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c +9
-0+} + +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 ||
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1898
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+ !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 */ + +static void sdhci_msm_bus_voting (struct sdhci_host *host,u32 enable)
s/u32/bool/ ?
Quoted 21 lines
drivers/mmc/host/sdhci-msm.c +16
-0+{ +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,@@ -1845,11 +2072,20 @@ static int sdhci_msm_probe(struct platform_device *pdev)dev_warn (&pdev->dev,"TCXO clk not present (%d)\n" , ret);
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_probe(struct platform_device *pdev) · line 2082
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +8
-0} + ret =sdhci_msm_bus_register (msm_host, pdev); +if (ret && msm_host->bus_vote_data && + !msm_host->bus_vote_data->skip_bus_bw_voting) { +dev_err (&pdev->dev,"Bus registration failed (%d)\n" , ret); + goto clk_disable; + } + +sdhci_msm_bus_voting (host,1 );
s/1/true/
Quoted 29 lines
drivers/mmc/host/sdhci-msm.c +4
-3+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; } }@@ -1924,7 +2160,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);@@ -1937,7 +2173,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);
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_probe(struct platform_device *pdev) · line 2202
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +2
-0@@ -1962,6 +2198,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,0 );
s/0/false/
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_remove(struct platform_device *pdev) · line 2234
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +3
-0+sdhci_msm_bus_unregister (&pdev->dev, msm_host); clk_disable:clk_bulk_disable_unprepare (ARRAY_SIZE (msm_host->bulk_clks), msm_host->bulk_clks);@@ -1991,6 +2230,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,0 );
s/0/false/
drivers/mmc/host/sdhci-msm.c static __maybe_unused int sdhci_msm_runtime_suspend(struct device *dev) · line 2248
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +2
-1+sdhci_msm_bus_unregister (&pdev->dev, msm_host);sdhci_pltfm_free (pdev);return 0 ; }@@ -2003,7 +2245,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,0 );
s/0/false/
drivers/mmc/host/sdhci-msm.c static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) · line 2270
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c +1
-0return 0 ; }@@ -2025,6 +2267,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,1 );
s/0/true/
Thanks
Matthias
Re: [RFC v3 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support
Hi Matthias,
Thanks for the review !
Quoted 47 lines · Matthias Kaehlcke
Hi Pradeep,
Quoted 44 lines · 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 Georgi Djakov [RFC]
(https://lkml.org/lkml/2018/10/11/499) andSaravana Kannan [PATCH v6]
(https://lkml.org/lkml/2019/12/6/740)change from RFC v2 -> v3 changes:
- Modified interconnect bandwidth support using OPP framework.
- Addressed review comments on v2.Signed-off-by Sahitya Tummala <[email protected]>Signed-off-by Subhash Jadavani <[email protected]>Signed-off-by Veerabhadrarao Badiganti <[email protected]>Co-developed-by Pradeep P V K <[email protected]>Signed-off-by Pradeep P V K <[email protected]>--- drivers/mmc/host/sdhci-msm.c | 251 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 247 insertions(+), 4 deletions(-)drivers/mmc/host/sdhci-msm.c +2
-0@@ -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; };
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c struct sdhci_msm_variant_info { · line 238
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+struct sdhci_msm_bus_vote_data { + +struct icc_path *sdhc_to_ddr; +struct icc_path *cpu_to_sdhc; +bool skip_bus_bw_voting;nit: the name is somewhat redundant given the name of the containing struct. '
skip_vote' or 'skip_voting' would be sufficient IMO.
ok. i will address in my next patch series.
Quoted 34 lines
Quoted 34 lines
drivers/mmc/host/sdhci-msm.c +6
-0+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){
drivers/mmc/host/sdhci-msm.c +1
-0struct 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); }
Quoted 15 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1712
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c +11
-0+/* +* 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;nit: you could declare 'bw' last and do the assignment together with the declaration.
ok. I will do it in my next patch.
Quoted 23 lines
Quoted 23 lines
drivers/mmc/host/sdhci-msm.c +22
-0+ +if (ios->bus_width ==MMC_BUS_WIDTH_4 ) + bw /=2 ; +else if (ios->bus_width ==MMC_BUS_WIDTH_1 ) + bw /=8 ; + +return bw; +} + +/* +* 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, +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; + +
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1740
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+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 */ s/Bandwidth/bandwidth/
ok.
Quoted 21 lines
Quoted 21 lines
drivers/mmc/host/sdhci-msm.c +21
-0+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);
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +5
-0+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 ];the names are not very expressive, how about
peak_bwandavg_bw?
hmm, ok. i will modify in my next patch set.
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1767
Quoted 1 line
drivers/mmc/host/sdhci-msm.c +1
-0+int ddr_rc =0 , cpu_rc =0 ;initialization is not needed
ok.
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1773
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+ +if (!msm_host->bus_vote_data->sdhc_to_ddr || + !msm_host->bus_vote_data->cpu_to_sdhc) +return ; + +if (bw != vote_data->curr_freq) {if (bw ==
vote_data->curr_freq)return;
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1778
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+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 */ s/Band width/bandwidth/
ok.
Quoted 25 lines
Quoted 25 lines
drivers/mmc/host/sdhci-msm.c +4
-0+dev_pm_opp_put (opp); + } + } + pr_debug("%s: freq:%d sdhc_to_ddr ab:%lu ib:%lu cpu_to_sdhc ab:%luib:%lu\n",
drivers/mmc/host/sdhci-msm.c +19
-0+mmc_hostname (host->mmc), bw, ab[0 ], ib[0 ], + ab[1], ib[1]); + 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" , +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
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1807
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+ *dev, struct sdhci_msm_host *host) + +{ +struct platform_device *pdev =to_platform_device (dev); +struct sdhci_msm_bus_vote_data *vote_data =NULL ; +int ret =0 ;initialization of '
vote_data' and 'ret' is not needed.
ok. i will be removing this function and bring all the required to the caller function that should address all the comments on this function.
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1812
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+ +if (!pdev) { +dev_err (dev,"Null platform device!\n" ); +return NULL ; + }It's probably safe to assume that the device is a platform device?
same as above.
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1815
Quoted 3 lines
drivers/mmc/host/sdhci-msm.c +3
-0+ vote_data =devm_kzalloc (dev,sizeof (*vote_data),GFP_KERNEL ); +if (!vote_data) +return vote_data;return
ERR_PTR(-ENOMEM); ?
Quoted 12 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1824
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c +9
-0+ + 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);s/regestration/registration/
ok.
Quoted 3 lines
Depending on the consequences of this failure (e.g.
probe()fails) you probably want to usedev_warn()ordev_err()here.
same as above.
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1832
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c +8
-0+ goto err; + } + } + +return vote_data; +err: +devm_kfree (dev, vote_data); + vote_data =NULL ;not needed
ok.
Quoted 19 lines
drivers/mmc/host/sdhci-msm.c
Quoted 1 line
drivers/mmc/host/sdhci-msm.c +1
-0+return vote_data;return NULL;
Quoted 15 lines
drivers/mmc/host/sdhci-msm.c +15
-0+} + +/* +* 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) {
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1849
Quoted 1 line
drivers/mmc/host/sdhci-msm.c +1
-0+dev_dbg (&pdev->dev,"Failed to get bus_scale data\n" );In general I'm not convinced about the use of
dev_dbg()in the error paths. Maybe there is some signal you can use to determine if there should be a valid bus scaling information (e.g. some DT property exists). If it exists you proceed with obtaining the necessary information and complain loudly if something goes wrong, otherwise you don't even try to obtain it and omit the debug logs.
ok. i will modify accordingly, in my next patch series.
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1850
Quoted 1 line
drivers/mmc/host/sdhci-msm.c +1
-0+ host->bus_vote_data =NULL ;IIUC struct
sdhci_msm_hostis zero initialized bysdhci_pltfm_init(), hence the assignment to NULL is not needed.
ok.
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1851
Quoted 1 line
drivers/mmc/host/sdhci-msm.c +1
-0+return -EINVAL ;Use the error returned by
sdhci_msm_get_bus_vote_data().
ok. i will address in my next patch set.
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1855
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +4
-0+ } + host->bus_vote_data = vote_data; + + vote_data->sdhc_to_ddr =of_icc_get (&pdev->dev,"sdhc-ddr" );This could be the signal I mentioned above. If this entry does not exist it is pointless to proceed. You could check this before
sdhci_msm_get_bus_vote_data()and abort with adev_info()message in case it doesn't exist. If it exists any error afterwards will be reported withdev_err().
ok. i will modify the code accordingly in my next patch series.
Quoted 6 lines
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+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",
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1865
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +4
-0+ 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;if there is an error
probe()should probably be aborted. If that's the case you can replace the above three lines with 'returnPTR_ERR(vote_data->sdhc_to_ddr);'
True but i added this for more cautious. Any how it is handled as error probe(). i will address this in my next patch series.
Quoted 6 lines
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+ } + vote_data->cpu_to_sdhc =of_icc_get (&pdev->dev,"cpu-sdhc" ); +if (!vote_data->cpu_to_sdhc) { +dev_dbg (&pdev->dev,"DT property for path %s missing\n" , +"cpu_to_sdhc" ); +return -ENOENT ;
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1877
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+ }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;same comment as above
ok. i will take care this too.
Quoted 7 lines
drivers/mmc/host/sdhci-msm.c
Quoted 2 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ } +return ret;no error if we get to this point, hence:
return 0;
ok.
Quoted 13 lines
Quoted 13 lines
drivers/mmc/host/sdhci-msm.c +13
-0+} + +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);
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1898
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+icc_put (vote_data->cpu_to_sdhc); +} + +#define MSM_MMC_BUS_VOTING_DELAY 200/* msecs */ + +static void sdhci_msm_bus_voting (struct sdhci_host *host,u32 enable)s/u32/bool/ ?
ok.
Quoted 24 lines
Quoted 24 lines
drivers/mmc/host/sdhci-msm.c +16
-0+{ +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,@@ -1845,11 +2072,20 @@ static int sdhci_msm_probe(struct platform_device *pdev) dev_warn(&pdev->dev, "TCXO clk not present (%d)\n", ret); }
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_probe(struct platform_device *pdev) · line 2082
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c +8
-0+ ret =sdhci_msm_bus_register (msm_host, pdev); +if (ret && msm_host->bus_vote_data && + !msm_host->bus_vote_data->skip_bus_bw_voting) { +dev_err (&pdev->dev,"Bus registration failed (%d)\n" , ret); + goto clk_disable; + } + +sdhci_msm_bus_voting (host,1 );s/1/true/
ok.
Quoted 29 lines
Quoted 29 lines
drivers/mmc/host/sdhci-msm.c +3
-2+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; } }@@ -1924,7 +2160,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; }drivers/mmc/host/sdhci-msm.c +1
-1sdhci_msm_init_pwr_irq_wait (msm_host);@@ -1937,7 +2173,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; }
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_probe(struct · line 2203
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c +2
-0pm_runtime_get_noresume (&pdev->dev);@@ -1962,6 +2198,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,0 );s/0/false/
ok.
Quoted 5 lines
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +1
-0+sdhci_msm_bus_unregister (&pdev->dev, msm_host); clk_disable: clk_bulk_disable_unprepare(ARRAY_SIZE(msm_host->bulk_clks),drivers/mmc/host/sdhci-msm.c +0
-0msm_host->bulk_clks);@@ -1991,6 +2230,9 @@ static int sdhci_msm_remove(struct
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static int sdhci_msm_remove(struct platform_device *pdev) · line 2234
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +2
-0platform_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,0 );s/0/false/
ok.
Quoted 7 lines
Quoted 7 lines
drivers/mmc/host/sdhci-msm.c +1
-0+sdhci_msm_bus_unregister (&pdev->dev, msm_host);sdhci_pltfm_free (pdev);return 0 ;}
drivers/mmc/host/sdhci-msm.c +0
-0@@ -2003,7 +2245,7 @@ static __maybe_unused int sdhci_msm_runtime_suspend(struct device *dev)
Quoted 7 lines
drivers/mmc/host/sdhci-msm.c static __maybe_unused int sdhci_msm_runtime_suspend(struct device *dev) · line 2248
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +1
-1clk_bulk_disable_unprepare (ARRAY_SIZE (msm_host->bulk_clks), msm_host->bulk_clks); - +sdhci_msm_bus_voting (host,0 );s/0/false/
ok.
Quoted 12 lines
drivers/mmc/host/sdhci-msm.c static __maybe_unused int · line 2271
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c +1
-0return 0 ; }@@ -2025,6 +2267,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,1 );s/0/true/
ok. i will address all these in my next patch series.
Quoted 3 lines
Thanks
Matthias
Re: [RFC v3 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support
In reply to Pradeep P V KQuoted 10 lines · 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 Georgi Djakov [RFC]
(https://lkml.org/lkml/2018/10/11/499) andSaravana 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.
Quoted 4 lines
change from RFC v2 -> v3 changes:
- Modified interconnect bandwidth support using OPP framework.
- Addressed review comments on v2.
Move the changelog below the --- line
Quoted 6 lines
Signed-off-by Sahitya Tummala <[email protected]>Signed-off-by Subhash Jadavani <[email protected]>Signed-off-by Veerabhadrarao Badiganti <[email protected]>Co-developed-by Pradeep P V K <[email protected]>Signed-off-by Pradeep P V K <[email protected]>
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.
Quoted 20 lines
--- drivers/mmc/host/sdhci-msm.c | 251 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 247 insertions(+), 4 deletions(-)drivers/mmc/host/sdhci-msm.c +2
-0@@ -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"
drivers/mmc/host/sdhci-msm.c struct sdhci_msm_variant_info { · line 235
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +2
-0@@ -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.
Quoted 41 lines
drivers/mmc/host/sdhci-msm.c +18
-0+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);
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1719
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+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; }
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1726
Quoted 7 lines
drivers/mmc/host/sdhci-msm.c +7
-0+} + +/* +* 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.
Quoted 34 lines
drivers/mmc/host/sdhci-msm.c +34
-0+ 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;
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1770
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+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?
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1773
Quoted 3 lines
drivers/mmc/host/sdhci-msm.c +3
-0+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.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1778
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+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?
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1784
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+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.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1788
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +4
-0+ 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()
Quoted 7 lines
drivers/mmc/host/sdhci-msm.c +7
-0+ mmc_hostname(host->mmc), ddr_rc, cpu_rc); +return ; + } + vote_data->curr_freq = bw; + } +} +
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1805
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+/* +* 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.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1807
Quoted 2 lines
drivers/mmc/host/sdhci-msm.c +2
-0+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.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1809
Quoted 2 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ +if (!pdev) {
How could this happen?
Quoted 13 lines
drivers/mmc/host/sdhci-msm.c +13
-0+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;
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1832
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+ }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.
Quoted 26 lines
drivers/mmc/host/sdhci-msm.c +26
-0+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" );
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1868
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+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.
Quoted 18 lines
drivers/mmc/host/sdhci-msm.c +18
-0+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; +
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1896
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+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.
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1898
Quoted 2 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ +static void sdhci_msm_bus_voting (struct sdhci_host *host,u32 enable)
Split this in an enable and a disable
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c +9
-0+{ +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 ; +
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1917
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +7
-0+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
Re: [RFC v3 1/2] mmc: sdhci-msm: Add interconnect bus bandwidth scaling support
Hi Bjorn,
Thanks for the review !
Quoted 10 lines · Bjorn Andersson
Quoted 8 lines · 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 Georgi Djakov [RFC]
(https://lkml.org/lkml/2018/10/11/499) and
Quoted 7 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 6 lines
Signed-off-by Sahitya Tummala <[email protected]>Signed-off-by Subhash Jadavani <[email protected]>Signed-off-by Veerabhadrarao Badiganti <[email protected]>Co-developed-by Pradeep P V K <[email protected]>Signed-off-by Pradeep P V K <[email protected]>
Quoted 7 lines
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.
Quoted 20 lines
Quoted 20 lines
--- drivers/mmc/host/sdhci-msm.c | 251 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 247 insertions(+), 4 deletions(-)drivers/mmc/host/sdhci-msm.c +2
-0@@ -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"
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c struct sdhci_msm_variant_info { · line 235
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +2
-0@@ -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.
Quoted 59 lines
Quoted 45 lines
drivers/mmc/host/sdhci-msm.c +9
-0+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){
drivers/mmc/host/sdhci-msm.c +9
-0struct 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);drivers/mmc/host/sdhci-msm.c static void · line 1720
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+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) {
caseMMC_BUS_WIDTH_1:
Quoted 7 lines
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.
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host)
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c +7
-0+} + +/* +* 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.
Quoted 40 lines
Quoted 40 lines
drivers/mmc/host/sdhci-msm.c +40
-0+ 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],
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1770
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +4
-0ab[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.
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1773
Quoted 3 lines
drivers/mmc/host/sdhci-msm.c +3
-0+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.
Quoted 8 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1778
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c +5
-0+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.
Quoted 11 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1784
Quoted 7 lines
drivers/mmc/host/sdhci-msm.c +4
-0+dev_pm_opp_put (opp); + } + } + pr_debug("%s: freq:%d sdhc_to_ddr ab:%lu ib:%lu cpu_to_sdhc ab:%luib:%lu\n",
drivers/mmc/host/sdhci-msm.c +2
-0+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.
Quoted 7 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1788
Quoted 4 lines
drivers/mmc/host/sdhci-msm.c +4
-0+ 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.
Quoted 13 lines
Quoted 13 lines
drivers/mmc/host/sdhci-msm.c +13
-0+ 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
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1805
Quoted 5 lines
*sdhci_msm_get_bus_vote_data(struct device
drivers/mmc/host/sdhci-msm.c +4
-0+ *dev, struct sdhci_msm_host *host) + +{ +struct platform_device *pdev =to_platform_device (dev);Just pass the
platform_deviceinstead of passing&pdev->devand thencontainer_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.
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c
Quoted 2 lines
drivers/mmc/host/sdhci-msm.c +2
-0+struct sdhci_msm_bus_vote_data *vote_data =NULL ; +int ret =0 ;No need to initialize either
vote_dataor ret, given that they are both written before read below.
same as above.
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c
Quoted 2 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ +if (!pdev) {How could this happen?
same as above.
Quoted 17 lines
Quoted 17 lines
drivers/mmc/host/sdhci-msm.c +17
-0+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; + }
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1832
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+ } + +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.
Quoted 50 lines
Quoted 26 lines
drivers/mmc/host/sdhci-msm.c +26
-0+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" );drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1868
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +10
-0+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_ddrorvote_dataitself if this happens.I think things would be cleaner if you make this function return the
vote_datafor success, NULL if the information was omitted orERR_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 forhost->bus_vote_data- no need to checksdhc_to_ddr,cpu_to_sdhcandskip_bus_bw_votingindividually..
Quoted 4 lines
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.
Quoted 22 lines
Quoted 22 lines
drivers/mmc/host/sdhci-msm.c +22
-0+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 ;
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1896
Quoted 6 lines
drivers/mmc/host/sdhci-msm.c +6
-0+ +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.
Quoted 5 lines
drivers/mmc/host/sdhci-msm.c
Quoted 2 lines
drivers/mmc/host/sdhci-msm.c +2
-0+ +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.
Quoted 22 lines
Quoted 9 lines
drivers/mmc/host/sdhci-msm.c +9
-0+{ +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 ; +drivers/mmc/host/sdhci-msm.c static void sdhci_msm_set_regulator_caps(struct sdhci_msm_host *msm_host) · line 1917
Quoted 10 lines
drivers/mmc/host/sdhci-msm.c +7
-0+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
RFC v3 2/2 dt-bindings: mmc: sdhci-msm: Add interconnect BW scaling strings
In reply to Pradeep P V KAdd interconnect bandwidth scaling supported strings for qcom-sdhci controller.
changes from RFC v2 -> v3:
- Addressed review comments on v2.
- Modified interconnect bandwidth support using OPP framework.
--- Documentation/devicetree/bindings/mmc/sdhci-msm.txt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
Documentation/devicetree/bindings/mmc/sdhci-msm.txt +18 -0
@@ -40,6 +40,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 {
@@ -57,6 +72,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 {
Signature · 1 line
1.9.1