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

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

[Masahiro Yamada]

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.

Also, use devm_ for clean-up.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

CCing Philipp Zabel.
I see his sob in commit 06c47e6286d5.


 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);
 	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);
-- 
2.7.4

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

[Philipp Zabel]

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?

> Also, use devm_ for clean-up.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> 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.

regards
Philipp

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@pengutronix.de>:
> 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@socionext.com>
>> ---
>>
>> 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

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

[Philipp Zabel]

On Tue, 2018-04-03 at 17:30 +0900, Masahiro Yamada wrote:
> 2018-04-03 17:00 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> > 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@socionext.com>
> > > ---
> > > 
> > > 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?

regards
Philipp

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@pengutronix.de>:
> On Tue, 2018-04-03 at 17:30 +0900, Masahiro Yamada wrote:
>> 2018-04-03 17:00 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> > 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@socionext.com>
>> > > ---
>> > >
>> > > 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

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

[Vivek Gautam]

On 4/3/2018 3:49 PM, Masahiro Yamada wrote:
> 2018-04-03 17:46 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>> On Tue, 2018-04-03 at 17:30 +0900, Masahiro Yamada wrote:
>>> 2018-04-03 17:00 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>>>> 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@socionext.com>
>>>>> ---
>>>>>
>>>>> 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().

Just to understand this - If the power domain isn't active for the said 
device,
does it matter if it is in reset state or not?

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

What's the point of this. The power domain and reset should be independent
of each other, and when we are doing a driver detach, the state of hardware
should be of less concern.
The device should anyways not leak power when the power domain isn't active.

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

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

[Masahiro Yamada]

2018-04-03 19:35 GMT+09:00 Vivek Gautam <vivek.gautam@codeaurora.org>:
>
>
> On 4/3/2018 3:49 PM, Masahiro Yamada wrote:
>>
>> 2018-04-03 17:46 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>>>
>>> On Tue, 2018-04-03 at 17:30 +0900, Masahiro Yamada wrote:
>>>>
>>>> 2018-04-03 17:00 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
>>>>>
>>>>> 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@socionext.com>
>>>>>> ---
>>>>>>
>>>>>> 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().
>
>
> Just to understand this - If the power domain isn't active for the said
> device,
> does it matter if it is in reset state or not?
>
>>
>>
>>   ret = reset_control_assert(simple->resets);
>>   if (ret)
>>             return ret; /* if we cannot assert reset, do not allow
>> driver detach */
>
>
> What's the point of this. The power domain and reset should be independent
> of each other, and when we are doing a driver detach, the state of hardware
> should be of less concern.
> The device should anyways not leak power when the power domain isn't active.
>

I do not see any point in worrying about this.


Philipp,
Do you agree this patch is no problem?



-- 
Best Regards
Masahiro Yamada

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

[Philipp Zabel]

On Wed, 2018-04-04 at 10:25 +0900, Masahiro Yamada wrote:
> 2018-04-03 19:35 GMT+09:00 Vivek Gautam <vivek.gautam@codeaurora.org>:
> > 
> > 
> > On 4/3/2018 3:49 PM, Masahiro Yamada wrote:
> > > 
> > > 2018-04-03 17:46 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> > > > 
> > > > On Tue, 2018-04-03 at 17:30 +0900, Masahiro Yamada wrote:
> > > > > 
> > > > > 2018-04-03 17:00 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> > > > > > 
> > > > > > 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@socionext.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > 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().
> > 
> > 
> > Just to understand this - If the power domain isn't active for the said
> > device,
> > does it matter if it is in reset state or not?

If the dwc3 driver is unbound, but the reset is not asserted because
another driver holds it deasserted, could the power domain be disabled?
And could this cause a problem? A disabled power domain might cause
resets not to propagate in some modules, but I have no idea whether this
might be the case here.

> > > 
> > > 
> > >   ret = reset_control_assert(simple->resets);
> > >   if (ret)
> > >             return ret; /* if we cannot assert reset, do not allow
> > > driver detach */
> > 
> > 
> > What's the point of this. The power domain and reset should be independent
> > of each other, and when we are doing a driver detach, the state of hardware
> > should be of less concern.
> > The device should anyways not leak power when the power domain isn't active.
> > 
> 
> I do not see any point in worrying about this.
> 
> 
> Philipp,
> Do you agree this patch is no problem?

For all I know, that might depend on the hardware.

I agree this patch is no problem for current users, as there is no board
sharing those resets.
And I assume it is no problem for your hardware either, if unbinding and
rebinding the dwc3 driver works while another driver holds the reset
deasserted.

regards
Philipp