Re: [PATCH] usb: dwc3: of-simple: use managed and shared reset control

[Masahiro Yamada]

2018-04-03 17:00 GMT+09:00 Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>:
> On Thu, 2018-03-29 at 15:07 +0900, Masahiro Yamada wrote:
>> This driver handles the reset control in a common manner; deassert
>> resets before use, assert them after use.  There is no good reason
>> why it should be exclusive.
>
> Is this preemptive cleanup, or do you have hardware on the horizon that
> shares these reset lines with other peripherals?


This patch is necessary for Socionext SoCs.

The same reset lines are shared between
this dwc3-of_simple and other glue circuits.



>> Also, use devm_ for clean-up.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
>> ---
>>
>> CCing Philipp Zabel.
>> I see his sob in commit 06c47e6286d5.
>
> At the time I was concerned with the reset_control_array addition and
> didn't look closely at the exclusive vs shared issue.
>>  drivers/usb/dwc3/dwc3-of-simple.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
>> index e54c362..bd6ab65 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -91,7 +91,7 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>       platform_set_drvdata(pdev, simple);
>>       simple->dev = dev;
>>
>> -     simple->resets = of_reset_control_array_get_optional_exclusive(np);
>> +     simple->resets = devm_reset_control_array_get_optional_shared(dev);
>
> From the usage in the driver, it does indeed look like _shared reset
> usage is appropriate. I assume that the hardware has no need for the
> reset to be asserted right before probe or after remove, it just
> requires that the reset line is kept deasserted while the driver is
> probed.
>
>>       if (IS_ERR(simple->resets)) {
>>               ret = PTR_ERR(simple->resets);
>>               dev_err(dev, "failed to get device resets, err=%d\n", ret);
>> @@ -100,7 +100,7 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>
>>       ret = reset_control_deassert(simple->resets);
>>       if (ret)
>> -             goto err_resetc_put;
>> +             return ret;
>>
>>       ret = dwc3_of_simple_clk_init(simple, of_count_phandle_with_args(np,
>>                                               "clocks", "#clock-cells"));
>> @@ -126,8 +126,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>  err_resetc_assert:
>>       reset_control_assert(simple->resets);
>>
>> -err_resetc_put:
>> -     reset_control_put(simple->resets);
>>       return ret;
>>  }
>>
>> @@ -146,7 +144,6 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
>>       simple->num_clocks = 0;
>>
>>       reset_control_assert(simple->resets);
>> -     reset_control_put(simple->resets);
>>
>>       pm_runtime_put_sync(dev);
>>       pm_runtime_disable(dev);
>
> Changing to devm_ changes the order here. Whether or not it could be a
> problem to assert the reset only after pm_runtime_put (or potentially
> never), I can't say. I assume this is a non-issue, but somebody who
> knows the hardware better would have to decide.



I do not understand what you mean.
Can you describe your concern in more details?

I am not touching reset_control_assert() here.

I am delaying the call for reset_control_put().


If I understand reset_control_put() correctly,
the effects of this change are:
  - The ref_count and module ownership for the reset controller
    driver will be held a little longer
  - The call for kfree() will be a little bit delayed.

Why do you need knowledge about this hardware?



-- 
Best Regards
Masahiro Yamada