[PATCH 2/3] usb: dwc3: of-simple: add support for shared and pulsed reset lines

[Martin Blumenstingl]

Hi Felipe,

On Mon, Jan 29, 2018 at 9:18 AM, Felipe Balbi <balbi at kernel.org> wrote:
>
> Hi,
>
> Martin Blumenstingl <martin.blumenstingl at googlemail.com> writes:
>> Some SoCs (such as Amlogic Meson GXL for example) share the reset line
>> with other components (in case of the Meson GXL example there's a shared
>> reset line between the USB2 PHYs, USB3 PHYs and the dwc3 controller).
>> Additionally SoC implementations may prefer a reset pulse over level
>> resets.
>>
>> Add an internal per-of_device_id struct which can be used to configure
>> whether the reset lines are shared and whether they use level or pulse
>> resets.
>>
>> For now this falls back to the old defaults, which are:
>> - reset lines are exclusive
>> - level resets are being used
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl at googlemail.com>
>> ---
>>  drivers/usb/dwc3/dwc3-of-simple.c | 65 ++++++++++++++++++++++++++++++++-------
>>  1 file changed, 54 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
>> index 7ae0eefc7cc7..ceb9f0cd822a 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -22,11 +22,22 @@
>>  #include <linux/pm_runtime.h>
>>  #include <linux/reset.h>
>>
>> +/**
>> + * struct dwc3_of_simple_params - hardware specific parameters
>> + * @shared_resets: indicates that the resets are shared or exclusive
>> + * @pulse_resets: use a reset pulse instead of level based resets
>> + */
>> +struct dwc3_of_simple_params {
>> +     bool                    shared_resets;
>> +     bool                    pulse_resets;
>> +};
>> +
>>  struct dwc3_of_simple {
>>       struct device           *dev;
>>       struct clk              **clks;
>>       int                     num_clocks;
>>       struct reset_control    *resets;
>> +     const struct dwc3_of_simple_params      *params;
>
> instead, you can add these two fields here:
>
>         bool shared_resets;
>         bool pulse_resets;
>
> and ...
>
>> @@ -90,17 +101,26 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>
>>       platform_set_drvdata(pdev, simple);
>>       simple->dev = dev;
>> +     simple->params = of_device_get_match_data(dev);
>>
>> -     simple->resets = of_reset_control_array_get_optional_exclusive(np);
>> +     simple->resets = of_reset_control_array_get(np,
>> +                                             simple->params->shared_resets,
>> +                                             true);
>
> wrap this with a of_device_is_compatible() check:
>
>         if (of_device_is_compatible(dev->of_node, "foobar")) {
>                 simple->shared_resets = true;
>                 simple->pulse_resets = true;
>         }
>
> or something like that. Then we don't need to add a new
> dwc3_of_simple_params for everybody.
sure, I can do this if that fits the coding style in the dwc3 driver better
in that case, do you still want me to keep patches #2 and #3 separate?

> Also, the why isn't the reset type (pulse vs level) handled by reset
> framework itself? Why does dwc3-of-simple need to know about it?
the Amlogic reset driver supports both, level resets and reset pulses
unfortunately the reset line is de-asserted by default, so to reset it
we would have to:
- assert it first
- then de-assert it again

however, the reset framework does not allow this for shared resets
(see reset_control_assert: it triggers a WARN_ON if we try to assert a
shared reset which has not been de-asserted yet)
together with the fact that there are reset controller hardware
implementations out there which don't support level resets I decided
to implement it as reset pulse


Regards
Martin