Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx> writes:
> On 08/09/16 20:52, Martin Blumenstingl wrote:
>> On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman <khilman@xxxxxxxxxxxx> 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.
Kevin
[1] from the DT patch:
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 2e8a3d9..02dfc54 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -151,6 +151,34 @@
#size-cells = <2>;
ranges;
+ usb-phys@c0000000 {
+ compatible = "simple-bus";
+ reg = <0x0 0xc0000000 0x0 0x40>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges = <0x0 0x0 0x0 0xc0000000 0x0 0x40>;
+
+ usb0_phy: usb_phy@0 {
+ compatible = "amlogic,meson-gxbb-usb2-phy";
+ #phy-cells = <0>;
+ reg = <0x0 0x0 0x0 0x20>;
+ resets = <&reset 34>;
+ clocks = <&clkc CLKID_USB &clkc CLKID_USB0>;
+ clock-names = "usb_general", "usb";
+ status = "disabled";
+ };
+
+ usb1_phy: usb_phy@20 {
+ compatible = "amlogic,meson-gxbb-usb2-phy";
+ #phy-cells = <0>;
+ reg = <0x0 0x20 0x0 0x20>;
+ resets = <&reset 34>;
+ clocks = <&clkc CLKID_USB &clkc CLKID_USB1>;
+ clock-names = "usb_general", "usb";
+ status = "disabled";
+ };
+ };
+
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
