Re: [PATCH v4 6/7] PM / devfreq: rockchip: add devfreq driver for rk3399 dmc

[Chanwoo Choi]

Hi Lin,

On the next version, I'd like you to add the 'linux-pm@xxxxxxxxxxxxxxx'
because devfreq is a subsystem of power management.

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@xxxxxxxxxxxxxx>
>>> ---
>>> 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?

>> 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.

> 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