Re: [PATCH 3/4] devfreq: exynos4: Add ppmu's clock control and code clean about regulator control

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Bartlomiej,

On 03/13/2014 11:15 AM, Chanwoo Choi wrote:
> Hi Batlomiej,
> 
> On 03/13/2014 12:17 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi,
>>
>> On Wednesday, March 12, 2014 08:48:01 PM 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>;
>>> };
>>
>> This should be in Documentation/devicetree/bindings/ documentation.
> 
> OK, I will add documentation about it.
> 
>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  drivers/devfreq/exynos/exynos4_bus.c | 107 ++++++++++++++++++++++++++++++-----
>>>  1 file changed, 93 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
>>> index 16fb3cb..0c5b99e 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;
>>> @@ -722,8 +728,26 @@ static int exynos4_bus_get_dev_status(struct device *dev,
>>>  static void exynos4_bus_exit(struct device *dev)
>>>  {
>>>  	struct busfreq_data *data = dev_get_drvdata(dev);
>>> +	int i;
>>>  
>>> -	devfreq_unregister_opp_notifier(dev, data->devfreq);
>>> +	/*
>>> +	 * Un-map memory man 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++) {
>>> +		if (data->clk_ppmu[i])
>>> +			clk_disable_unprepare(data->clk_ppmu[i]);
>>> +	}
>>> +
>>> +	for (i = 0; i < PPMU_END; i++) {
>>> +		if (data->ppmu[i].hw_base)
>>> +			iounmap(data->ppmu[i].hw_base);
>>> +
>>> +	}
>>>  }
>>>  
>>>  static struct devfreq_dev_profile exynos4_devfreq_profile = {
>>> @@ -987,6 +1011,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;
>>>  
>>>  	if (!np) {
>>> @@ -1005,8 +1030,67 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>>>  		}
>>>  	}
>>>  
>>> +	/*
>>> +	 * Get PPMU's clocks to control them. But, if PPMU's clocks
>>> +	 * is default 'pass' state, this driver don't need control
>>> +	 * PPMU's clock.
>>> +	 */
>>> +	for (i = 0; i < PPMU_END; i++) {
>>> +		data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]);
>>> +		if (IS_ERR_OR_NULL(data->clk_ppmu[i])) {
>>> +			dev_warn(dev, "Cannot get %s clock\n", clk_name[i]);
>>> +			data->clk_ppmu[i] = NULL;
>>> +		}
>>> +
>>> +		ret = clk_prepare_enable(data->clk_ppmu[i]);
>>> +		if (ret < 0) {
>>> +			dev_warn(dev, "Cannot enable %s clock\n", clk_name[i]);
>>> +			data->clk_ppmu[i] = NULL;
>>> +			goto err_clocks;
>>> +		}
>>> +	}
>>> +
>>> +
>>> +	/* Get regulators to control voltage of int/mif 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_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_clocks;
>>
>> This won't disable vdd_int regulator.
> 
> I don't understand. This patch contro/enable always vdd_int regualor as following:
> 	>> +	data->vdd_int = devm_regulator_get(dev, "vdd_int");
> 	>> +	ret = regulator_enable(data->vdd_int);
> You can check this code on this patch. upper
> 
>>
>>> +		}
>>> +		ret = regulator_enable(data->vdd_mif);
>>> +		if (ret < 0) {
>>> +			dev_err(dev, "Failed to enable regulator of vdd_mif\n");
>>> +			goto err_clocks;
>>
>> ditto
> 
>>
>>> +		}
>>> +		break;
>>> +	case TYPE_BUSF_EXYNOS4210:
>>> +	default:
>>> +		dev_err(data->dev, "Unknown device type\n");
>>> +		return -EINVAL;
>>
>> This looks very wrong for Exynos4210.
> 
> What is wrong for Exynos4210?
> Always, exynos4210/exynos4x12 control vdd_int regulator uppper this patch.

It is my mistake. I'll modify it.

Best Regards,
Chanwoo Choi