[PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB

[Martin Blumenstingl]

On Fri, Sep 9, 2016 at 5:33 PM, Kevin Hilman <khilman at baylibre.com> wrote:
> Martin Blumenstingl <martin.blumenstingl at googlemail.com> writes:
>
>> On Thu, Sep 8, 2016 at 10:53 PM, Ben Dooks <ben.dooks at codethink.co.uk> wrote:
>>> On 08/09/16 21:42, Kevin Hilman wrote:
>>>>
>>>> Ben Dooks <ben.dooks at codethink.co.uk> writes:
>>>>
>>>>> On 08/09/16 20:52, Martin Blumenstingl wrote:
>>>>>>
>>>>>> On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman <khilman at baylibre.com>
>>>>>> wrote:
>>>>>>>>
>>>>>>>> +     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
>>>>>>>> +     if (IS_ERR(phy)) {
>>>>>>>> +             dev_err(&pdev->dev, "failed to create PHY\n");
>>>>>>>> +             return PTR_ERR(phy);
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     if (usb_reset_refcnt++ == 0) {
>>>>>>>> +             ret = device_reset(&pdev->dev);
>>>>>>>> +             if (ret) {
>>>>>>>> +                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
>>>>>>>> +                     return ret;
>>>>>>>> +             }
>>>>>>>> +     }
>>>>>>>
>>>>>>>
>>>>>>> The ref count + reset here looks like something that could/should be
>>>>>>> handled in a runtime PM callback.
>>>>>>
>>>>>> Unfortunately that doesn't work (as Jerome found out) because both
>>>>>> PHYs are sharing the same reset line.
>>>>>> So if the second PHY would call device_reset then it would also reset
>>>>>> the first PHY!
>>>>>>
>>>>>> There's a comment above the declaration of usb_reset_refcnt which
>>>>>> tries to explain this:
>>>>>> "The PHYs are sharing a common reset line -> we are only allowed to
>>>>>> reset once for all PHYs."
>>>>>> Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0)
>>>>>> {" line to make it easier to see?
>>>>>>
>>>>>
>>>>> pm-runtime has refcounting in it. When one of the nodes turns on,
>>>>> the pm-runtime will call your driver to say there is a user when
>>>>> this first use turns up.
>>>>>
>>>>> If all the sub-phys turn off and drop their refcount then the driver
>>>>> is called to say there are no more users and you can go to sleep.
>>>>
>>>>
>>>> After a chat w/Martin on IRC, It turns out runtime PM wont help here.
>>>>
>>>> The reason is because there are physically two PHY devices[1].  Those 2
>>>> devices will be treated independely by runtime PM, and have separate
>>>> use-counting, which means doing what I proposed would cause a reset to
>>>> happen when either device was probed.
>>>>
>>>> So, I think it's OK as it is.
>>>
>>>
>>> Surely you can do pm_runtime_get/put on the phy's parent platform
>>> device and do it that way?
>> could you please be more specific with that (do you mean pdev->dev.parent)?
>> so we would use pm_runtime_{get_sync,put} with the parent, while we
>> would still define the runtime_resume in our driver.
>
> You'd also need to do get/put on the children, but yes, that's what Ben
> is suggesting.
>
> However, the problem with all of the solutions proposed (runtime PM ones
> included) is that we're forcing a board-specific design issue (2 devices
> sharing a reset line) into a driver that should not have any
> board-specific assumptions in it.
>
> For example, if this driver is used on another platform where different
> PHYs have different reset lines, then one of them (the unlucky one who
> is not probed first) will never get reset.  So any form of per-device
> ref-counting is not a portable solution.
indeed, so in simple words we would need something like
reset_control_do_once(rstc, RESET/ASSERT/DEASSERT) which would
remember internally if any action has already been executed: if not it
does a _reset, _assert or _deassert and otherwise it does nothing.

> I'm not sure yet how the reset framework is supposed to handle shared
> reset lines, but that needs some investigation.  I quick glance and it
> seems that reset controllers can have shared lines, so that should be
> investigated.
I added Philipp and Hans to this thread - maybe they can comment on this.
To sum it up, our problem is:
- there are two separate USB PHYs on Meson GXBB
- both are sharing the same reset line (provided by the reset-meson driver)
- during initialization of the PHYs we must only call
reset_control_reset(rstc) once (if we do it for the first *and* second
PHY then the first PHY gets confused once the second PHY uses the
reset because the first PHY's state is reset as well)


Regards,
Martin