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@xxxxxxxxxxxxxxx' >> 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@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? > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel