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

Martin Blumenstingl martin.blumenstingl at googlemail.com
Fri Sep 9 13:36:14 PDT 2016


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



More information about the linux-arm-kernel mailing list