Message-ID: <57A19F60.7090700@rock-chips.com>
Date:	Wed, 3 Aug 2016 15:38:08 +0800
From:	hl <hl@...k-chips.com>
To:	Chanwoo Choi <cw00.choi@...sung.com>, 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 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.
>
>>> 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,
>
>> 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
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip


-- 
Lin Huang

Powered by blists - more mailing lists