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: Fri, 9 Sep 2016 22:36:14 +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: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Kevin Hilman , p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org, hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org Cc: Ben Dooks , 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 Fri, Sep 9, 2016 at 5:33 PM, Kevin Hilman wrote: > Martin Blumenstingl writes: > >> 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html