Message-id: <57A28D5C.40507@samsung.com>
Date:	Thu, 04 Aug 2016 09:33:32 +0900
From:	Chanwoo Choi <cw00.choi@...sung.com>
To:	hl <hl@...k-chips.com>, heiko@...ech.de
Cc:	tixy@...aro.org, typ@...k-chips.com, airlied@...ux.ie,
	mturquette@...libre.com, dbasehore@...omium.org,
	sboyd@...eaurora.org, linux-kernel@...r.kernel.org,
	dri-devel@...ts.freedesktop.org, dianders@...omium.org,
	linux-rockchip@...ts.infradead.org, kyungmin.park@...sung.com,
	myungjoo.ham@...sung.com, linux-arm-kernel@...ts.infradead.org,
	mark.yao@...k-chips.com
Subject: Re: [PATCH v4 6/7] PM / devfreq: rockchip: add devfreq driver for
 rk3399 dmc

Hi Lin,

On 2016년 08월 03일 16:38, hl wrote:
> 
> Hi Chanwoo Choi,
> On 2016年08月02日 12:21, Chanwoo Choi wrote:
>> Hi Lin,
>>
>> On the next version, I'd like you to add the 'linux-pm@...r.kernel.org'
>> because devfreq is a subsystem of power management.
> Sure, will do it next version.
>> On 2016년 08월 02일 10:03, hl wrote:
>>> Hi Chanwoo Choi,
>>>
>>>      Thanks for reviewing so carefully. And i have some question:
>>>
>>> On 2016年08月01日 18:28, Chanwoo Choi wrote:
>>>> Hi Lin,
>>>>
>>>> As I mentioned on patch5, you better to make the documentation as following:
>>>> - Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>>>> And, I add the comments.
>>>>
>>>>
>>>> On 2016년 07월 29일 16:57, Lin Huang wrote:
>>>>> base on dfi result, we do ddr frequency scaling, register
>>>>> dmc driver to devfreq framework, and use simple-ondemand
>>>>> policy.
>>>>>
>>>>> Signed-off-by: Lin Huang <hl@...k-chips.com>
>>>>> ---
>>>>> Changes in v4:
>>>>> - use arm_smccc_smc() function talk to bl31
>>>>> - delete rockchip_dmc.c file and config
>>>>> - delete dmc_notify
>>>>> - adjust probe order
>>>>>    Changes in v3:
>>>>> - operate dram setting through sip call
>>>>> - imporve set rate flow
>>>>>
>>>>> Changes in v2:
>>>>> - None
>>>>>    Changes in v1:
>>>>> - move dfi controller to event
>>>>> - fix set voltage sequence when set rate fail
>>>>> - change Kconfig type from tristate to bool
>>>>> - move unuse EXPORT_SYMBOL_GPL()
>>>>>
>>>>>    drivers/devfreq/Kconfig               |   1 +
>>>>>    drivers/devfreq/Makefile              |   1 +
>>>>>    drivers/devfreq/rockchip/Kconfig      |   8 +
>>>>>    drivers/devfreq/rockchip/Makefile     |   1 +
>>>>>    drivers/devfreq/rockchip/rk3399_dmc.c | 473 ++++++++++++++++++++++++++++++++++
>>>>>    5 files changed, 484 insertions(+)
>>>>>    create mode 100644 drivers/devfreq/rockchip/Kconfig
>>>>>    create mode 100644 drivers/devfreq/rockchip/Makefile
>>>>>    create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
>>>>>
>> [snip]
>>
>>>>> +
>>>>> +static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>>>>> +                 u32 flags)
>>>>> +{
>>>>> +    struct platform_device *pdev = container_of(dev, struct platform_device,
>>>>> +                            dev);
>>>>> +    struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
>>>> You can use the 'dev_get_drvdata()' to simplify it instead of 'platform_get_drvdata()'.
>>>>
>>>>      struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(dev);
>>>>
>>>>> +    struct dev_pm_opp *opp;
>>>>> +    unsigned long old_clk_rate = dmcfreq->rate;
>>>>> +    unsigned long target_volt, target_rate;
>>>>> +    int err;
>>>>> +
>>>>> +    rcu_read_lock();
>>>>> +    opp = devfreq_recommended_opp(dev, freq, flags);
>>>>> +    if (IS_ERR(opp)) {
>>>>> +        rcu_read_unlock();
>>>>> +        return PTR_ERR(opp);
>>>>> +    }
>>>>> +
>>>>> +    target_rate = dev_pm_opp_get_freq(opp);
>>>>> +    target_volt = dev_pm_opp_get_voltage(opp);
>>>>> +    opp = devfreq_recommended_opp(dev, &dmcfreq->rate, flags);
>>>>> +    if (IS_ERR(opp)) {
>>>>> +        rcu_read_unlock();
>>>>> +        return PTR_ERR(opp);
>>>>> +    }
>>>>> +    dmcfreq->volt = dev_pm_opp_get_voltage(opp);
>>>> If you add the 'curr_opp' variable to struct rk3399_dmcfreq,
>>>> you can remove the calling of devfreq_recommended_opp().
>>>>      dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
>>>>      dmcfreq->volt = dev_pm_opp_get_freq(dmcfreq->curr_opp);
>>>>
>>>> Because the current rate and voltage is already decided on previous polling cycle,
>>>> So we don't need to get the opp with devfreq_recommended_opp().
>>> I prefer the way now use, since we get the dmcfreq->rate use clk_get_rate() after,
>>> Base on that,  i do not care the set_rate success or fail. use curr_opp i need to
>>> care about set_rate status, when fail, i must set some rate, when success i must
>>> set other rate.
>> I think that it is not good to get the alrady decided opp
>> by devfreq_recommended_opp(). Usually, devfreq_recommended_opp() is used
>> to get the proper opp which get the close frequency (dmcfreq->rate).
>>
>> Also, When finishing the rk3399_dmcfreq_target(), the rk3399_dmc.c
>> have to know the current opp or rate without any finding sequence.
>> The additional finding procedure is un-needed.
>>
>>>>> +    rcu_read_unlock();
>>>>> +
>>>>> +    if (dmcfreq->rate == target_rate)
>>>>> +        return 0;
>>>>> +
>>>>> +    mutex_lock(&dmcfreq->lock);
>>>>> +
>>>>> +    /*
>>>>> +     * if frequency scaling from low to high, adjust voltage first;
>>>>> +     * if frequency scaling from high to low, adjuset frequency first;
>>>>> +     */
>>>> s/adjuset/adjust
>>>>
>>>> I recommend that you use a captital letter for first character and use the '.'
>>>> instead of ';'.
>>>>
>>>>> +    if (old_clk_rate < target_rate) {
>>>>> +        err = regulator_set_voltage(dmcfreq->vdd_center, target_volt,
>>>>> +                        target_volt);
>>>>> +        if (err) {
>>>>> +            dev_err(dev, "Unable to set vol %lu\n", target_volt);
>>>> To readability, you better to use the corrent word to pass the precise the log message.
>>>> - s/vol/voltage
>>>>
>>>> And, this patch uses the 'Unable to' or 'Cannot' to show the error log.
>>>> I recommend that you use the consistent expression if there is not any specific reason.
>>>>
>>>>      dev_err(dev, "Cannot set the voltage %lu uV\n", target_volt);
>>>>
>>>>> +            goto out;
>>>>> +        }
>>>>> +    }
>>>>> +    dmcfreq->wait_dcf_flag = 1;
>>>>> +    err = clk_set_rate(dmcfreq->dmc_clk, target_rate);
>>>>> +    if (err) {
>>>>> +        dev_err(dev,
>>>>> +            "Unable to set freq %lu. Current freq %lu. Error %d\n",
>>>>> +            target_rate, old_clk_rate, err);
>>>>      dev_err(dev, "Cannot set the frequency %lu (%d)\n", target_rate, err);
>>>>
>>>>> +        regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
>>>>> +                      dmcfreq->volt);
>>>>> +        goto out;
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * wait until bcf irq happen, it means freq scaling finish in bl31,
>>>> ditto.
>>>>
>>>>> +     * use 100ms as timeout time
>>>> s/time/time.
>>>>
>>>>> +     */
>>>>> +    if (!wait_event_timeout(dmcfreq->wait_dcf_queue,
>>>>> +                !dmcfreq->wait_dcf_flag, HZ / 10))
>>>>> +        dev_warn(dev, "Timeout waiting for dcf irq\n");
>>>> If the timeout happen, are there any problem?
>>> When timeout happen , may be we miss interrupt, but it do not affect this
>>> process, since we will check the rate whether success later.
>> OK. But I'd like you to modify the warning message.
>>
>> One more thing, is the dcf interrupt related to the change of clock rate?
>> When the clock rate is changed, the dcf interrupt happen?
> Yes, when clock rate changed sucessful, it will trigger a irq in bl31.

OK.

>>
>>>> After setting the frequency and voltage, store the current opp entry on .curr_opp.
>>>>      dmcfreq->curr_opp = opp;
>>>>
>>>>> +    /*
>>>>> +     * check the dpll rate
>>>>> +     * there only two result we will get,
>>>>> +     * 1. ddr frequency scaling fail, we still get the old rate
>>>>> +     * 2, ddr frequency scaling sucessful, we get the rate we set
>>>>> +     */
>>>>> +    dmcfreq->rate = clk_get_rate(dmcfreq->dmc_clk);
>>>>> +
>>>>> +    /* if get the incorrect rate, set voltage to old value */
>>>>> +    if (dmcfreq->rate != target_rate) {
>>>>> +        dev_err(dev, "get wrong ddr frequency, Request freq %lu,\
>>>>> +            Current freq %lu\n", target_rate, dmcfreq->rate);
>>>>> +        regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
>>>>> +                      dmcfreq->volt);
>>>> [Without force, it is just my opion about this code:]
>>>> I think that this checking code it is un-needed.
>>>> If this case occur, the rk3399_dmc.c never set the specific frequency
>>>> because the rk3399 clock don't support the specific frequency for rk3399 dmc.
>>>>
>>>> If you want to set the correct frequency,
>>>> When verifying the rk3399 dmc driver, you should check the rk3399 clock driver.
>>>>
>>>> Basically, if the the clock driver don't support the correct same frequency ,
>>>> CCF(Common Clock Framework) set the close frequency. It is not a bad thing.
>>> May be i should remove the regulator_set_voltage() here, but still need to
>>> check whether we get the right frequency, since if we do not get the right frequency,
>> When calling clk_set_rate(), the final frequency only depend on the rk3399 clock driver.
>> But, if you want to check the new rate, I think that you should move this code
>> right after clk_set_rate() when there is any dependency of dcf interrupt.
> it should be after the wait_event, since it indicate the clk_set_rate finish,

OK.

>>
>>> we should send a warn message, to remind that maybe you pass a frequency which
>>> do not support in bl31.
>> Also, I'd like you to explain the 'bl31' and add the description on next version.
>>
>> [snip]
>>
>> Regards,
>> Chanwoo Choi

Regards,
Chanwoo Choi

Powered by blists - more mailing lists