From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB Date: Thu, 08 Sep 2016 13:42:22 -0700 Message-ID: References: <20160904213152.25837-1-martin.blumenstingl@googlemail.com> <20160904213152.25837-5-martin.blumenstingl@googlemail.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: In-Reply-To: (Ben Dooks's message of "Thu, 8 Sep 2016 21:20:12 +0100") Sender: linux-clk-owner@vger.kernel.org To: Ben Dooks Cc: Martin Blumenstingl , mark.rutland@arm.com, devicetree@vger.kernel.org, gregkh@linuxfoundation.org, johnyoun@synopsys.com, will.deacon@arm.com, mturquette@baylibre.com, linux-usb@vger.kernel.org, sboyd@codeaurora.org, kishon@ti.com, robh+dt@kernel.org, catalin.marinas@arm.com, carlo@caione.org, linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, jbrunet@baylibre.com List-Id: devicetree@vger.kernel.org Ben Dooks writes: > On 08/09/16 20:52, Martin Blumenstingl wrote: >> On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman 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"; + }; + }; +