Hi Philipp, On Tue, Sep 13, 2016 at 5:28 PM, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote: > Hi Martin, > > Am Freitag, den 09.09.2016, 22:36 +0200 schrieb Martin Blumenstingl: >> On Fri, Sep 9, 2016 at 5:33 PM, Kevin Hilman <khilman@xxxxxxxxxxxx> wrote: >> > Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx> writes: >> > >> >> On Thu, Sep 8, 2016 at 10:53 PM, Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx> wrote: >> >>> On 08/09/16 21:42, Kevin Hilman wrote: >> >>>> >> >>>> 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. >> >>> >> >>> >> >>> 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) > > If you have an initially asserted reset line and you can enable the > first module by deasserting the reset via reset_control_deassert (and > reset_control_assert to signal when the module may be disabled again > after use), shared resets are for you. > > If you need a reset pulse or have no direct control over the reset line, > (device_reset), the reset framework currently has no solution for this. > The ugly thing about reset_control_once would be that it can't re-reset > modules when unloading and reloading driver modules. The corresponding reset driver in question is reset-meson, which only implements reset (assert/deassert are not implemented). However, I don't know if this is due to hardware design. I think the hardware implements the latter, but maybe Neil can give more information here (I currently don't have access to my board so I cannot test how the hardware actually behaves). > A real solution for shared reset lines with reset pulses would have to > be some kind of reset request framework where if one module requests a > reset, the other module sharing the reset could be notified, and then > either veto the reset or, if possible, cease operations, store its > state, and prepare to be reset, too, and afterwards restore state. I'd > prefer not to think about this too much unless absolutely necessary. I'm not sure if this would work in our case: one PHY instance would have to know if the other has already triggered the reset or not. Regards, Martin -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html