Hi Bartlomiej,
On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>
> Hi Chanwoo,
>
> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>> Hi Kamil,
>>
>> Looks good to me. But, this patch has some issue.
>> I added the detailed reviews.
>>
>> I recommend that you make the separate patches as following
>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>
>> First patch,
>> Need to consolidate the following two function into one function.
>> because the original exynos-bus.c has the problem that the regulator
>> of parent devfreq device have to be enabled before enabling the clock.
>> This issue did not happen because bootloader enables the bus-related
>> regulators before kernel booting.
>> - exynos_bus_parse_of()
>> - exynos_bus_parent_parse_of()
>>> Second patch,
>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>
>>
>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>> Reuse opp core code for setting bus clock and voltage. As a side
>>> effect this allow useage of coupled regulators feature (required
>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>> while the old code used regulator_set_voltage_tol() with fixed
>>> tolerance. This patch also removes no longer needed parsing of DT
>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>> it).
>>>
>>> Signed-off-by: Kamil Konieczny <k.konieczny@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>> 1 file changed, 66 insertions(+), 106 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index 486cc5b422f1..7fc4f76bd848 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -25,7 +25,6 @@
>>> #include <linux/slab.h>
>>>
>>> #define DEFAULT_SATURATION_RATIO 40
>>> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>>>
>>> struct exynos_bus {
>>> struct device *dev;
>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>
>>> unsigned long curr_freq;
>>>
>>> - struct regulator *regulator;
>>> + struct opp_table *opp_table;
>>> +
>>> struct clk *clk;
>>> - unsigned int voltage_tolerance;
>>> unsigned int ratio;
>>> };
>>>
>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>> {
>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>> struct dev_pm_opp *new_opp;
>>> - unsigned long old_freq, new_freq, new_volt, tol;
>>> int ret = 0;
>>> -
>>> - /* Get new opp-bus instance according to new bus clock */
>>> + /*
>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>> + * *freq to correct value.
>>> + */
>>
>> You better to change this comment with following styles
>> to keep the consistency:
>>
>> /* Get correct frequency for bus ... */
>>
>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>> if (IS_ERR(new_opp)) {
>>> dev_err(dev, "failed to get recommended opp instance\n");
>>> return PTR_ERR(new_opp);
>>> }
>>>
>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>> - new_volt = dev_pm_opp_get_voltage(new_opp);
>>> dev_pm_opp_put(new_opp);
>>>
>>> - old_freq = bus->curr_freq;
>>> -
>>> - if (old_freq == new_freq)
>>> - return 0;
>>> - tol = new_volt * bus->voltage_tolerance / 100;
>>> -
>>> /* Change voltage and frequency according to new OPP level */
>>> mutex_lock(&bus->lock);
>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>> + if (!ret)
>>> + bus->curr_freq = *freq;
>>
>> Have to print the error log if ret has minus error value.
>
> dev_pm_opp_set_rate() should print the error message on all
> errors so wouldn't printing the error log also here be superfluous?
>
> [ Please also note that the other user of dev_pm_opp_set_rate()
> (cpufreq-dt cpufreq driver) doesn't do this. ]
OK. Thanks for the explanation.
>
>> Modify it as following:
>>
>> if (ret < 0) {
>> dev_err(dev, "failed to set bus rate\n");
>> goto err:
>> }
>> bus->curr_freq = *freq;
>>
>> err:
>> mutex_unlock(&bus->lock);
>>
>> return ret;
>>
>>>
>>> - if (old_freq < new_freq) {
>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>> - if (ret < 0) {
>>> - dev_err(bus->dev, "failed to set voltage\n");
>>> - goto out;
>>> - }
>>> - }
>>> -
>>> - ret = clk_set_rate(bus->clk, new_freq);
>>> - if (ret < 0) {
>>> - dev_err(dev, "failed to change clock of bus\n");
>>> - clk_set_rate(bus->clk, old_freq);
>>> - goto out;
>>> - }
>>> -
>>> - if (old_freq > new_freq) {
>>> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>> - if (ret < 0) {
>>> - dev_err(bus->dev, "failed to set voltage\n");
>>> - goto out;
>>> - }
>>> - }
>>> - bus->curr_freq = new_freq;
>>> -
>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>> -out:
>>> mutex_unlock(&bus->lock);
>>>
>>> return ret;
>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>> if (ret < 0)
>>> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>
>>> - if (bus->regulator)
>>> - regulator_disable(bus->regulator);
>>> + if (bus->opp_table)
>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>
>> Have to disable regulator after disabling the clock
>> to prevent the h/w fault.
>>
>> I think that you should call them with following sequence:
>>
>> clk_disable_unprepare(bus->clk);
>> if (bus->opp_table)
>> dev_pm_opp_put_regulators(bus->opp_table);
>> dev_pm_opp_of_remove_table(dev);
>>
>>>
>>> dev_pm_opp_of_remove_table(dev);
>>> +
>>> clk_disable_unprepare(bus->clk);
>>> }
>>>
>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>> {
>>> struct exynos_bus *bus = dev_get_drvdata(dev);
>>> struct dev_pm_opp *new_opp;
>>> - unsigned long old_freq, new_freq;
>>> - int ret = 0;
>>> + int ret;
>>>
>>> - /* Get new opp-bus instance according to new bus clock */
>>> + /*
>>> + * New frequency for bus may not be exactly matched to opp, adjust
>>> + * *freq to correct value.
>>> + */
>>
>> You better to change this comment with following styles
>> to keep the consistency:
>>
>> /* Get correct frequency for bus ... */
>>
>>> new_opp = devfreq_recommended_opp(dev, freq, flags);
>>> if (IS_ERR(new_opp)) {
>>> dev_err(dev, "failed to get recommended opp instance\n");
>>> return PTR_ERR(new_opp);
>>> }
>>>
>>> - new_freq = dev_pm_opp_get_freq(new_opp);
>>> dev_pm_opp_put(new_opp);
>>>
>>> - old_freq = bus->curr_freq;
>>> -
>>> - if (old_freq == new_freq)
>>> - return 0;
>>> -
>>> /* Change the frequency according to new OPP level */
>>> mutex_lock(&bus->lock);
>>> + ret = dev_pm_opp_set_rate(dev, *freq);
>>> + if (!ret)
>>> + bus->curr_freq = *freq;
>>
>> ditto. Have to print the error log, check above comment.
>>
>>>
>>> - ret = clk_set_rate(bus->clk, new_freq);
>>> - if (ret < 0) {
>>> - dev_err(dev, "failed to set the clock of bus\n");
>>> - goto out;
>>> - }
>>> -
>>> - *freq = new_freq;
>>> - bus->curr_freq = new_freq;
>>> -
>>> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>> - old_freq, new_freq, clk_get_rate(bus->clk));
>>> -out:
>>> mutex_unlock(&bus->lock);
>>>
>>> return ret;
>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>> struct exynos_bus *bus)
>>> {
>>> struct device *dev = bus->dev;
>>> - int i, ret, count, size;
>>> -
>>> - /* Get the regulator to provide each bus with the power */
>>> - bus->regulator = devm_regulator_get(dev, "vdd");
>>> - if (IS_ERR(bus->regulator)) {
>>> - dev_err(dev, "failed to get VDD regulator\n");
>>> - return PTR_ERR(bus->regulator);
>>> - }
>>> -
>>> - ret = regulator_enable(bus->regulator);
>>> - if (ret < 0) {
>>> - dev_err(dev, "failed to enable VDD regulator\n");
>>> - return ret;
>>> - }
>>> + int i, count, size;
>>>
>>> /*
>>> * Get the devfreq-event devices to get the current utilization of
>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>> count = devfreq_event_get_edev_count(dev);
>>> if (count < 0) {
>>> dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>> - ret = count;
>>> - goto err_regulator;
>>> + return count;
>>> }
>>> +
>>> bus->edev_count = count;
>>>
>>> size = sizeof(*bus->edev) * count;
>>> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>> - if (!bus->edev) {
>>> - ret = -ENOMEM;
>>> - goto err_regulator;
>>> - }
>>> + if (!bus->edev)
>>> + return -ENOMEM;
>>>
>>> for (i = 0; i < count; i++) {
>>> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>> - if (IS_ERR(bus->edev[i])) {
>>> - ret = -EPROBE_DEFER;
>>> - goto err_regulator;
>>> - }
>>> + if (IS_ERR(bus->edev[i]))
>>> + return -EPROBE_DEFER;
>>> }
>>>
>>> /*
>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>> bus->ratio = DEFAULT_SATURATION_RATIO;
>>>
>>> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>> - &bus->voltage_tolerance))
>>> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>> -
>>> return 0;
>>> -
>>> -err_regulator:
>>> - regulator_disable(bus->regulator);
>>> -
>>> - return ret;
>>> }
>>>
>>> static int exynos_bus_parse_of(struct device_node *np,
>>> - struct exynos_bus *bus)
>>> + struct exynos_bus *bus, bool passive)
>>> {
>>> struct device *dev = bus->dev;
>>> + struct opp_table *opp_table;
>>> + const char *vdd = "vdd";
>>> struct dev_pm_opp *opp;
>>> unsigned long rate;
>>> int ret;
>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>> return ret;
>>> }
>>>
>>> + if (!passive) {
>>> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>> + if (IS_ERR(opp_table)) {
>>> + ret = PTR_ERR(opp_table);
>>> + dev_err(dev, "failed to set regulators %d\n", ret);
>>> + goto err_clk;/
>>> + }
>>> +
>>> + bus->opp_table = opp_table;
>>> + }
>>
>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>> because the regulator is only used by parent devfreq device.
>
> exynos_bus_parse_of() is called for all devfreq devices (including
> parent) and (as you've noticed) the regulator should be enabled before
> enabling clock (which is done in exynos_bus_parse_of()) so adding
> extra argument to exynos_bus_parse_of() (like it is done currently in
> the patch)
I think that this patch has still the problem about call sequence
between clock and regulator as following:
273 ret = clk_prepare_enable(bus->clk);
274 if (ret < 0) {
275 dev_err(dev, "failed to get enable clock\n");
276 return ret;
277 }
278
279 if (!passive) {
280 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
281 if (IS_ERR(opp_table)) {
282 ret = PTR_ERR(opp_table);
283 dev_err(dev, "failed to set regulators %d\n", ret);
284 goto err_clk;
285 }
286
287 bus->opp_table = opp_table;
288 }
makes it possible to do the setup correctly without the need
> of merging both functions into one huge function (which would be more
> difficult to follow than two simpler functions IMHO). Is that approach
> acceptable or do you prefer one big function?
Actually, I don't force to make one function for both
exynos_bus_parse_of() and exynos_bus_parent_parse_of().
If we just keep this code, dev_pm_opp_set_regulators()
should be handled in exynos_bus_parent_parse_of()
because only parent devfreq device controls the regulator.
In order to keep the two functions, maybe have to change
the call the sequence between exynos_bus_parse_of() and
exynos_bus_parent_parse_of().
Once again, I don't force any fixed method. I want to fix them
with correct way.
>
>>> +
>>> /* Get the freq and voltage from OPP table to scale the bus freq */
>>> ret = dev_pm_opp_of_add_table(dev);
>>> if (ret < 0) {
>>> dev_err(dev, "failed to get OPP table\n");
>>> - goto err_clk;
>>> + goto err_regulator;
>>> }
>>>
>>> rate = clk_get_rate(bus->clk);
>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>> ret = PTR_ERR(opp);
>>> goto err_opp;
>>> }
>>> +
>>> bus->curr_freq = dev_pm_opp_get_freq(opp);
>>> dev_pm_opp_put(opp);
>>>
>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>
>>> err_opp:
>>> dev_pm_opp_of_remove_table(dev);
>>> +
>>> +err_regulator:
>>> + if (bus->opp_table) {
>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>> + bus->opp_table = NULL;
>>> + }
>>
>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>
>>> +
>>> err_clk:
>>> clk_disable_unprepare(bus->clk);
>>>
>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>> struct exynos_bus *bus;
>>> int ret, max_state;
>>> unsigned long min_freq, max_freq;
>>> + bool passive = false;
>>>
>>> if (!np) {
>>> dev_err(dev, "failed to find devicetree node\n");
>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>> if (!bus)
>>> return -ENOMEM;
>>> +
>>> mutex_init(&bus->lock);
>>> bus->dev = &pdev->dev;
>>> platform_set_drvdata(pdev, bus);
>>> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>> + if (node) {
>>> + of_node_put(node);
>>> + passive = true;
>>> + }
>>>
>>> /* Parse the device-tree to get the resource information */
>>> - ret = exynos_bus_parse_of(np, bus);
>>> + ret = exynos_bus_parse_of(np, bus, passive);
>>> if (ret < 0)
>>> return ret;
>>>
>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>> goto err;
>>> }
>>>
>>> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>> - if (node) {
>>> - of_node_put(node);
>>> + if (passive)
>>> goto passive;
>>> - } else {
>>> - ret = exynos_bus_parent_parse_of(np, bus);
>>> - }
>>> +
>>> + ret = exynos_bus_parent_parse_of(np, bus);
>>>
>>
>> Remove unneeded blank line.
>>
>>> if (ret < 0)
>>> goto err;
>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>
>>> err:
>>> dev_pm_opp_of_remove_table(dev);
>>> + if (bus->opp_table) {
>>> + dev_pm_opp_put_regulators(bus->opp_table);
>>> + bus->opp_table = NULL;
>>> + }
>>> +
>>
>> ditto.
>> Have to disable regulator after disabling the clock
>> to prevent the h/w fault.
>>
>> I think that you should call them with following sequence:
>>
>> clk_disable_unprepare(bus->clk);
>> if (bus->opp_table)
>> dev_pm_opp_put_regulators(bus->opp_table);
>> dev_pm_opp_of_remove_table(dev);
>>
>>> clk_disable_unprepare(bus->clk);
>>>
>>> return ret;
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
