| Message ID | 53268F48.8060704@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Chanwoo, On 17.03.2014 06:59, Chanwoo Choi wrote: > Hi Tomasz, > > On 03/17/2014 11:51 AM, Chanwoo Choi wrote: >> Hi Tomasz, >> >> On 03/15/2014 02:42 AM, Tomasz Figa wrote: >>> Hi Chanwoo, >>> >>> On 13.03.2014 09:17, Chanwoo Choi wrote: >>>> There are not the clock controller of ppmudmc0/1. This patch control the clock >>>> of ppmudmc0/1 which is used for monitoring memory bus utilization. >>>> >>>> Also, this patch code clean about regulator control and free resource >>>> when calling exit/remove function. >>>> >>>> For example, >>>> busfreq@106A0000 { >>>> compatible = "samsung,exynos4x12-busfreq"; >>>> >>>> /* Clock for PPMUDMC0/1 */ >>>> clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>; >>>> clock-names = "ppmudmc0", "ppmudmc1"; >>>> >>>> /* Regulator for MIF/INT block */ >>>> vdd_mif-supply = <&buck1_reg>; >>>> vdd_int-supply = <&buck3_reg>; >>>> }; >>>> >>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > > I modify this patch according your comment as following: > > Best Regards, > Chanwoo Choi > >>From c8f2fbc4c1166ec02fb2ad46164bc7ed9118721b Mon Sep 17 00:00:00 2001 > From: Chanwoo Choi <cw00.choi@samsung.com> > Date: Fri, 14 Mar 2014 12:05:54 +0900 > Subject: [PATCH] devfreq: exynos4: Add ppmu's clock control and code clean > about regulator control > > There are not the clock controller of ppmudmc0/1. This patch control the clock > of ppmudmc0/1 which is used for monitoring memory bus utilization. > > Also, this patch code clean about regulator control and free resource > when calling exit/remove function. > > For example, > busfreq@106A0000 { > compatible = "samsung,exynos4x12-busfreq"; > > /* Clock for PPMUDMC0/1 */ > clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>; > clock-names = "ppmudmc0", "ppmudmc1"; > > /* Regulator for MIF/INT block */ > vdd_mif-supply = <&buck1_reg>; > vdd_int-supply = <&buck3_reg>; > }; > > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > --- > drivers/devfreq/exynos/exynos4_bus.c | 97 +++++++++++++++++++++++++++++++----- > 1 file changed, 84 insertions(+), 13 deletions(-) > > diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c > index 4c630fb..3956bcc 100644 > --- a/drivers/devfreq/exynos/exynos4_bus.c > +++ b/drivers/devfreq/exynos/exynos4_bus.c > @@ -62,6 +62,11 @@ enum exynos_ppmu_idx { > PPMU_END, > }; > > +static const char *exynos_ppmu_clk_name[] = { > + [PPMU_DMC0] = "ppmudmc0", > + [PPMU_DMC1] = "ppmudmc1", > +}; > + > #define EX4210_LV_MAX LV_2 > #define EX4x12_LV_MAX LV_4 > #define EX4210_LV_NUM (LV_2 + 1) > @@ -86,6 +91,7 @@ struct busfreq_data { > struct regulator *vdd_mif; /* Exynos4412/4212 only */ > struct busfreq_opp_info curr_oppinfo; > struct exynos_ppmu ppmu[PPMU_END]; > + struct clk *clk_ppmu[PPMU_END]; > > struct notifier_block pm_notifier; > struct mutex lock; > @@ -724,6 +730,17 @@ static void exynos4_bus_exit(struct device *dev) > struct busfreq_data *data = dev_get_drvdata(dev); > int i; > > + /* > + * Un-map memory map and disable regulator/clocks > + * to prevent power leakage. > + */ > + regulator_disable(data->vdd_int); > + if (data->type == TYPE_BUSF_EXYNOS4x12) > + regulator_disable(data->vdd_mif); > + > + for (i = 0; i < PPMU_END; i++) > + clk_disable_unprepare(data->clk_ppmu[i]); > + > for (i = 0; i < PPMU_END; i++) > iounmap(data->ppmu[i].hw_base); > } > @@ -989,6 +1006,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data) > { > struct device *dev = data->dev; > struct device_node *np = dev->of_node; > + const char **clk_name = exynos_ppmu_clk_name; > int i, ret = 0; > > if (!np) { > @@ -1006,8 +1024,67 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data) > } > } > > + for (i = 0; i < PPMU_END; i++) { > + data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]); > + if (IS_ERR(data->clk_ppmu[i])) { > + dev_warn(dev, "Failed to get %s clock\n", clk_name[i]); dev_err() since this is an error. > + goto err_clocks; > + } > + > + ret = clk_prepare_enable(data->clk_ppmu[i]); > + if (ret < 0) { > + dev_warn(dev, "Failed to enable %s clock\n", clk_name[i]); Ditto. > + data->clk_ppmu[i] = NULL; > + goto err_clocks; > + } > + } > + > + /* Get regulator to control voltage of int block */ > + data->vdd_int = devm_regulator_get(dev, "vdd_int"); > + if (IS_ERR(data->vdd_int)) { > + dev_err(dev, "Failed to get the regulator of vdd_int\n"); > + ret = PTR_ERR(data->vdd_int); > + goto err_clocks; > + } > + ret = regulator_enable(data->vdd_int); > + if (ret < 0) { > + dev_err(dev, "Failed to enable regulator of vdd_int\n"); > + goto err_clocks; > + } > + > + switch (data->type) { > + case TYPE_BUSF_EXYNOS4210: > + break; Do you need to use switch here? Wouldn't a simple if (data->type == TYPE_BUSF_EXYNOS4x12) work as well, resulting with simpler code? However I would expect this code to be completely removed, after defining DT bindings in a way allowing you to completely drop such SoC type enums. Please see my other reply for explanation of Exynos power plane DT bindings concept. > + case TYPE_BUSF_EXYNOS4x12: > + /* Get regulator to control voltage of mif blk if Exynos4x12 */ > + data->vdd_mif = devm_regulator_get(dev, "vdd_mif"); > + if (IS_ERR(data->vdd_mif)) { > + dev_err(dev, "Failed to get the regulator vdd_mif\n"); > + ret = PTR_ERR(data->vdd_mif); > + goto err_regulator; > + } > + ret = regulator_enable(data->vdd_mif); > + if (ret < 0) { > + dev_err(dev, "Failed to enable regulator of vdd_mif\n"); > + goto err_regulator; > + } > + break; > + default: > + dev_err(dev, "Unknown device type : %d\n", data->type); > + return -EINVAL; > + }; > + > return 0; > > +err_regulator: > + regulator_disable(data->vdd_int); > +err_clocks: > + for (i = 0; i < PPMU_END; i++) { > + if (IS_ERR(data->clk_ppmu[i])) > + continue; > + else if (!IS_ERR(...)) > + clk_disable_unprepare(data->clk_ppmu[i]); > + } Otherwise looks good. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, On 03/18/2014 08:13 PM, Tomasz Figa wrote: > Hi Chanwoo, > > On 17.03.2014 06:59, Chanwoo Choi wrote: >> Hi Tomasz, >> >> On 03/17/2014 11:51 AM, Chanwoo Choi wrote: >>> Hi Tomasz, >>> >>> On 03/15/2014 02:42 AM, Tomasz Figa wrote: >>>> Hi Chanwoo, >>>> >>>> On 13.03.2014 09:17, Chanwoo Choi wrote: >>>>> There are not the clock controller of ppmudmc0/1. This patch control the clock >>>>> of ppmudmc0/1 which is used for monitoring memory bus utilization. >>>>> >>>>> Also, this patch code clean about regulator control and free resource >>>>> when calling exit/remove function. >>>>> >>>>> For example, >>>>> busfreq@106A0000 { >>>>> compatible = "samsung,exynos4x12-busfreq"; >>>>> >>>>> /* Clock for PPMUDMC0/1 */ >>>>> clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>; >>>>> clock-names = "ppmudmc0", "ppmudmc1"; >>>>> >>>>> /* Regulator for MIF/INT block */ >>>>> vdd_mif-supply = <&buck1_reg>; >>>>> vdd_int-supply = <&buck3_reg>; >>>>> }; >>>>> >>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >> >> I modify this patch according your comment as following: >> >> Best Regards, >> Chanwoo Choi >> >>> From c8f2fbc4c1166ec02fb2ad46164bc7ed9118721b Mon Sep 17 00:00:00 2001 >> From: Chanwoo Choi <cw00.choi@samsung.com> >> Date: Fri, 14 Mar 2014 12:05:54 +0900 >> Subject: [PATCH] devfreq: exynos4: Add ppmu's clock control and code clean >> about regulator control >> >> There are not the clock controller of ppmudmc0/1. This patch control the clock >> of ppmudmc0/1 which is used for monitoring memory bus utilization. >> >> Also, this patch code clean about regulator control and free resource >> when calling exit/remove function. >> >> For example, >> busfreq@106A0000 { >> compatible = "samsung,exynos4x12-busfreq"; >> >> /* Clock for PPMUDMC0/1 */ >> clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>; >> clock-names = "ppmudmc0", "ppmudmc1"; >> >> /* Regulator for MIF/INT block */ >> vdd_mif-supply = <&buck1_reg>; >> vdd_int-supply = <&buck3_reg>; >> }; >> >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >> --- >> drivers/devfreq/exynos/exynos4_bus.c | 97 +++++++++++++++++++++++++++++++----- >> 1 file changed, 84 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c >> index 4c630fb..3956bcc 100644 >> --- a/drivers/devfreq/exynos/exynos4_bus.c >> +++ b/drivers/devfreq/exynos/exynos4_bus.c >> @@ -62,6 +62,11 @@ enum exynos_ppmu_idx { >> PPMU_END, >> }; >> >> +static const char *exynos_ppmu_clk_name[] = { >> + [PPMU_DMC0] = "ppmudmc0", >> + [PPMU_DMC1] = "ppmudmc1", >> +}; >> + >> #define EX4210_LV_MAX LV_2 >> #define EX4x12_LV_MAX LV_4 >> #define EX4210_LV_NUM (LV_2 + 1) >> @@ -86,6 +91,7 @@ struct busfreq_data { >> struct regulator *vdd_mif; /* Exynos4412/4212 only */ >> struct busfreq_opp_info curr_oppinfo; >> struct exynos_ppmu ppmu[PPMU_END]; >> + struct clk *clk_ppmu[PPMU_END]; >> >> struct notifier_block pm_notifier; >> struct mutex lock; >> @@ -724,6 +730,17 @@ static void exynos4_bus_exit(struct device *dev) >> struct busfreq_data *data = dev_get_drvdata(dev); >> int i; >> >> + /* >> + * Un-map memory map and disable regulator/clocks >> + * to prevent power leakage. >> + */ >> + regulator_disable(data->vdd_int); >> + if (data->type == TYPE_BUSF_EXYNOS4x12) >> + regulator_disable(data->vdd_mif); >> + >> + for (i = 0; i < PPMU_END; i++) >> + clk_disable_unprepare(data->clk_ppmu[i]); >> + >> for (i = 0; i < PPMU_END; i++) >> iounmap(data->ppmu[i].hw_base); >> } >> @@ -989,6 +1006,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data) >> { >> struct device *dev = data->dev; >> struct device_node *np = dev->of_node; >> + const char **clk_name = exynos_ppmu_clk_name; >> int i, ret = 0; >> >> if (!np) { >> @@ -1006,8 +1024,67 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data) >> } >> } >> >> + for (i = 0; i < PPMU_END; i++) { >> + data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]); >> + if (IS_ERR(data->clk_ppmu[i])) { >> + dev_warn(dev, "Failed to get %s clock\n", clk_name[i]); > > dev_err() since this is an error. OK, I'll fix it. > >> + goto err_clocks; >> + } >> + >> + ret = clk_prepare_enable(data->clk_ppmu[i]); >> + if (ret < 0) { >> + dev_warn(dev, "Failed to enable %s clock\n", clk_name[i]); > > Ditto. OK, I'll fix it. > >> + data->clk_ppmu[i] = NULL; >> + goto err_clocks; >> + } >> + } >> + >> + /* Get regulator to control voltage of int block */ >> + data->vdd_int = devm_regulator_get(dev, "vdd_int"); >> + if (IS_ERR(data->vdd_int)) { >> + dev_err(dev, "Failed to get the regulator of vdd_int\n"); >> + ret = PTR_ERR(data->vdd_int); >> + goto err_clocks; >> + } >> + ret = regulator_enable(data->vdd_int); >> + if (ret < 0) { >> + dev_err(dev, "Failed to enable regulator of vdd_int\n"); >> + goto err_clocks; >> + } >> + >> + switch (data->type) { >> + case TYPE_BUSF_EXYNOS4210: >> + break; > > Do you need to use switch here? Wouldn't a simple if (data->type == TYPE_BUSF_EXYNOS4x12) work as well, resulting with simpler code? OK, I'll fix it. > > However I would expect this code to be completely removed, after defining DT bindings in a way allowing you to completely drop such SoC type enums. Please see my other reply for explanation of Exynos power plane DT bindings concept. OK, I agree your suggestion about Exynos power plane DT bindings concept. I'll apply "Exynos power plane" to exynos4 busfreq driver on next step after defining Exynos power plane DT bindings. > >> + case TYPE_BUSF_EXYNOS4x12: >> + /* Get regulator to control voltage of mif blk if Exynos4x12 */ >> + data->vdd_mif = devm_regulator_get(dev, "vdd_mif"); >> + if (IS_ERR(data->vdd_mif)) { >> + dev_err(dev, "Failed to get the regulator vdd_mif\n"); >> + ret = PTR_ERR(data->vdd_mif); >> + goto err_regulator; >> + } >> + ret = regulator_enable(data->vdd_mif); >> + if (ret < 0) { >> + dev_err(dev, "Failed to enable regulator of vdd_mif\n"); >> + goto err_regulator; >> + } >> + break; >> + default: >> + dev_err(dev, "Unknown device type : %d\n", data->type); >> + return -EINVAL; >> + }; >> + >> return 0; >> >> +err_regulator: >> + regulator_disable(data->vdd_int); >> +err_clocks: >> + for (i = 0; i < PPMU_END; i++) { >> + if (IS_ERR(data->clk_ppmu[i])) >> + continue; >> + else > > if (!IS_ERR(...)) > >> + clk_disable_unprepare(data->clk_ppmu[i]); >> + } > > Otherwise looks good. Thanks for your review. Best Regards, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c index 4c630fb..3956bcc 100644 --- a/drivers/devfreq/exynos/exynos4_bus.c +++ b/drivers/devfreq/exynos/exynos4_bus.c @@ -62,6 +62,11 @@ enum exynos_ppmu_idx { PPMU_END, }; +static const char *exynos_ppmu_clk_name[] = { + [PPMU_DMC0] = "ppmudmc0", + [PPMU_DMC1] = "ppmudmc1", +}; + #define EX4210_LV_MAX LV_2 #define EX4x12_LV_MAX LV_4 #define EX4210_LV_NUM (LV_2 + 1) @@ -86,6 +91,7 @@ struct busfreq_data { struct regulator *vdd_mif; /* Exynos4412/4212 only */ struct busfreq_opp_info curr_oppinfo; struct exynos_ppmu ppmu[PPMU_END]; + struct clk *clk_ppmu[PPMU_END]; struct notifier_block pm_notifier; struct mutex lock; @@ -724,6 +730,17 @@ static void exynos4_bus_exit(struct device *dev) struct busfreq_data *data = dev_get_drvdata(dev); int i; + /* + * Un-map memory map and disable regulator/clocks + * to prevent power leakage. + */ + regulator_disable(data->vdd_int); + if (data->type == TYPE_BUSF_EXYNOS4x12) + regulator_disable(data->vdd_mif); + + for (i = 0; i < PPMU_END; i++) + clk_disable_unprepare(data->clk_ppmu[i]); + for (i = 0; i < PPMU_END; i++) iounmap(data->ppmu[i].hw_base); } @@ -989,6 +1006,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data) { struct device *dev = data->dev; struct device_node *np = dev->of_node; + const char **clk_name = exynos_ppmu_clk_name; int i, ret = 0; if (!np) { @@ -1006,8 +1024,67 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data) } } + for (i = 0; i < PPMU_END; i++) { + data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]); + if (IS_ERR(data->clk_ppmu[i])) { + dev_warn(dev, "Failed to get %s clock\n", clk_name[i]); + goto err_clocks; + } + + ret = clk_prepare_enable(data->clk_ppmu[i]); + if (ret < 0) { + dev_warn(dev, "Failed to enable %s clock\n", clk_name[i]); + data->clk_ppmu[i] = NULL; + goto err_clocks; + } + } + + /* Get regulator to control voltage of int block */ + data->vdd_int = devm_regulator_get(dev, "vdd_int"); + if (IS_ERR(data->vdd_int)) { + dev_err(dev, "Failed to get the regulator of vdd_int\n"); + ret = PTR_ERR(data->vdd_int); + goto err_clocks; + } + ret = regulator_enable(data->vdd_int); + if (ret < 0) { + dev_err(dev, "Failed to enable regulator of vdd_int\n"); + goto err_clocks; + } + + switch (data->type) { + case TYPE_BUSF_EXYNOS4210: + break; + case TYPE_BUSF_EXYNOS4x12: + /* Get regulator to control voltage of mif blk if Exynos4x12 */ + data->vdd_mif = devm_regulator_get(dev, "vdd_mif"); + if (IS_ERR(data->vdd_mif)) { + dev_err(dev, "Failed to get the regulator vdd_mif\n"); + ret = PTR_ERR(data->vdd_mif); + goto err_regulator; + } + ret = regulator_enable(data->vdd_mif); + if (ret < 0) { + dev_err(dev, "Failed to enable regulator of vdd_mif\n"); + goto err_regulator; + } + break; + default: + dev_err(dev, "Unknown device type : %d\n", data->type); + return -EINVAL; + }; + return 0; +err_regulator: + regulator_disable(data->vdd_int); +err_clocks: + for (i = 0; i < PPMU_END; i++) { + if (IS_ERR(data->clk_ppmu[i])) + continue; + else + clk_disable_unprepare(data->clk_ppmu[i]); + } err_iomap: for (i = 0; i < PPMU_END; i++) { if (IS_ERR_OR_NULL(data->ppmu[i].hw_base)) @@ -1074,19 +1151,6 @@ static int exynos4_busfreq_probe(struct platform_device *pdev) return err; } - data->vdd_int = devm_regulator_get(dev, "vdd_int"); - if (IS_ERR(data->vdd_int)) { - dev_err(dev, "Cannot get the regulator \"vdd_int\"\n"); - return PTR_ERR(data->vdd_int); - } - if (data->type == TYPE_BUSF_EXYNOS4x12) { - data->vdd_mif = devm_regulator_get(dev, "vdd_mif"); - if (IS_ERR(data->vdd_mif)) { - dev_err(dev, "Cannot get the regulator \"vdd_mif\"\n"); - return PTR_ERR(data->vdd_mif); - } - } - rcu_read_lock(); opp = dev_pm_opp_find_freq_floor(dev, &exynos4_devfreq_profile.initial_freq); @@ -1146,6 +1210,13 @@ err_notifier_opp: return err; err_devfreq: + regulator_disable(data->vdd_int); + if (data->type == TYPE_BUSF_EXYNOS4x12) + regulator_disable(data->vdd_mif); + + for (i = 0; i < PPMU_END; i++) + clk_disable_unprepare(data->clk_ppmu[i]); + for (i = 0; i < PPMU_END; i++) iounmap(data->ppmu[i].hw_base);