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

[Masahiro Yamada]

2018-04-03 17:46 GMT+09:00 Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>:
> On Tue, 2018-04-03 at 17:30 +0900, Masahiro Yamada wrote:
>> 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.
>
> Thanks, this is helpful information.
>
>> > > 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.
>
> Sorry for the confusion, I have mixed up things here.
>
>> Can you describe your concern in more details?
>>
>> I am not touching reset_control_assert() here.
>
> With the change to shared reset control, reset_control_assert
> potentially does nothing, so it could be possible that
> pm_runtime_put_sync cuts the power before the reset es asserted again.
>
>> I am delaying the call for reset_control_put().
>
> Yes, please disregard my comment about the devm_ change, that should
> have no effect whatsoever and looks fine to me.
>
>> 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.
>
> Correct.
>
>> Why do you need knowledge about this hardware?
>
> Is it ok to keep the reset deasserted while the power is cut?
> Or do you
> have to guarantee that drivers sharing the same reset also keep the same
> power domains active?
>

If this were really a problem, the driver would have to check
the error code from reset_control_assert().


 ret = reset_control_assert(simple->resets);
 if (ret)
           return ret; /* if we cannot assert reset, do not allow
driver detach */

 pm_runtime_put_sync(dev);
 pm_runtime_disable(dev);
 return 0;



What I can tell is, the current situation is
blocking hardware with shared reset lines
from using this driver.



-- 
Best Regards
Masahiro Yamada