From mboxrd@z Thu Jan 1 00:00:00 1970 From: Martin Blumenstingl Subject: Re: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB Date: Thu, 8 Sep 2016 23:48:53 +0200 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; charset=UTF-8 Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Ben Dooks Cc: Kevin Hilman , mark.rutland-5wv7dgnIgG8@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org, johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org, will.deacon-5wv7dgnIgG8@public.gmane.org, mturquette-rdvid1DuHRBWk0Htik3J/w@public.gmane.org, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org, kishon-l0cyMroinI0@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, catalin.marinas-5wv7dgnIgG8@public.gmane.org, carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org, linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org List-Id: devicetree@vger.kernel.org On Thu, Sep 8, 2016 at 10:53 PM, Ben Dooks wrote: > On 08/09/16 21:42, Kevin Hilman wrote: >> >> 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. > > > 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. I'd be happy if that works and we can remove that refcounting hack -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: References: <20160904213152.25837-1-martin.blumenstingl@googlemail.com> <20160904213152.25837-5-martin.blumenstingl@googlemail.com> From: Martin Blumenstingl Date: Thu, 8 Sep 2016 23:48:53 +0200 Message-ID: Subject: Re: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB To: Ben Dooks Cc: Kevin Hilman , 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 Content-Type: text/plain; charset=UTF-8 List-ID: On Thu, Sep 8, 2016 at 10:53 PM, Ben Dooks wrote: > On 08/09/16 21:42, Kevin Hilman wrote: >> >> 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. > > > 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. I'd be happy if that works and we can remove that refcounting hack From mboxrd@z Thu Jan 1 00:00:00 1970 From: martin.blumenstingl@googlemail.com (Martin Blumenstingl) Date: Thu, 8 Sep 2016 23:48:53 +0200 Subject: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB In-Reply-To: References: <20160904213152.25837-1-martin.blumenstingl@googlemail.com> <20160904213152.25837-5-martin.blumenstingl@googlemail.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Sep 8, 2016 at 10:53 PM, Ben Dooks wrote: > On 08/09/16 21:42, Kevin Hilman wrote: >> >> 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. > > > 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. I'd be happy if that works and we can remove that refcounting hack From mboxrd@z Thu Jan 1 00:00:00 1970 From: martin.blumenstingl@googlemail.com (Martin Blumenstingl) Date: Thu, 8 Sep 2016 23:48:53 +0200 Subject: [PATCH 4/7] phy: meson: add USB2 PHY support for Meson8b and GXBB In-Reply-To: References: <20160904213152.25837-1-martin.blumenstingl@googlemail.com> <20160904213152.25837-5-martin.blumenstingl@googlemail.com> Message-ID: To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org On Thu, Sep 8, 2016 at 10:53 PM, Ben Dooks wrote: > On 08/09/16 21:42, Kevin Hilman wrote: >> >> 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. > > > 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. I'd be happy if that works and we can remove that refcounting hack