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

[khilman@baylibre.com (Kevin Hilman)]

Ben Dooks <ben.dooks@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@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.

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 at 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 at 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 at 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";
+			};
+		};
+