On Wed, Sep 14, 2016 at 10:37 AM, Philipp Zabel [off-list ref] wrote:

Am Dienstag, den 13.09.2016, 20:38 +0200 schrieb Martin Blumenstingl:

quoted

Hi Philipp,

On Tue, Sep 13, 2016 at 5:28 PM, Philipp Zabel [off-list ref] wrote:

quoted

Hi Martin,

Am Freitag, den 09.09.2016, 22:36 +0200 schrieb Martin Blumenstingl:

quoted

On Fri, Sep 9, 2016 at 5:33 PM, Kevin Hilman [off-list ref] wrote:

quoted

Martin Blumenstingl [off-list ref] writes:

quoted

On Thu, Sep 8, 2016 at 10:53 PM, Ben Dooks [off-list ref] wrote:

quoted

On 08/09/16 21:42, Kevin Hilman wrote:

quoted

Ben Dooks [off-list ref] writes:

quoted

On 08/09/16 20:52, Martin Blumenstingl wrote:

quoted

On Thu, Sep 8, 2016 at 9:35 PM, Kevin Hilman [off-list ref]
wrote:

quoted

quoted

+     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.

quoted

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).

quoted

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.

We could add a triggered flag or a counter to struct reset_control, and
have reset_control_reset_once do nothing if it is already set /
incremented. Since the reset_control goes away with the last consumer,
the shared reset line would get triggered again after unbinding both PHY
devices.

I guess that'd do the trick for us:
- we could use devm_reset_control_get_optional_shared() during probe
- power_on would then call reset_control_reset()
- the code in reset_control_reset would be changed: the if
(WARN_ON(rstc->shared)) would be removed. then we return 0 if
(rstc->shared && atomic_read(&rstc->shared_triggered)). otherwise we
proceed with the old logic, except that we use
atomic_set(&rstc->shared_triggerred, 1) in case of success (if an
error was returned we leave it as "not triggered").

Let me know if you want me to (at least try to) implement that and send an RFC.


Regards,
Martin