Hi Peter,

On Mon, Jan 29, 2018 at 4:30 AM, Peter Chen [off-list ref] wrote:

quoted

quoted

quoted

Now that usb_add_hcd parses all generic PHYs anyways the code which
skips initialization of a single PHY will go away.
Remove the code which sets struct usb_hcd's phy field from the
chipidea driver as this field will go away soon.

Signed-off-by: Martin Blumenstingl
[off-list ref]
---
 drivers/usb/chipidea/host.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/chipidea/host.c

b/drivers/usb/chipidea/host.c index 19d60ed7e41f..fc324767cb0f 100644

--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c

@@ -124,9 +124,7 @@ static int host_start(struct ci_hdrc *ci)

      hcd->power_budget = ci->platdata->power_budget;
      hcd->tpl_support = ci->platdata->tpl_support;
-     if (ci->phy)
-             hcd->phy = ci->phy;
-     else
+     if (!ci->phy)
              hcd->usb_phy = ci->usb_phy;

The reason hcd->phy is initialized by chipidea core is we do not need
HCD core to touch PHY, and PHY operation is shared for both device and host

mode for chipidea.
Chunfeng wanted me to drop the mtu3 patch because I forgot about device mode in
the mtu3 driver.
however, the chipidea driver seems to be different because I'm not dropping the
whole phy_{init,power_on,power_off,exit} code from it, but only a "flag" that tells the
HCD core to skip managing the USB PHY

quoted

If I understand correct, your HCD core PHY wrapper patch set will do
PHY operation if there is a "phy" node under controller's? If it is
correct, you may supply one way to let the HCD core bypass phy operations for

some USB controllers, eg dual-role controllers.

quoted

Thanks.

could you please explain why the HCD core should not manage the the PHYs when
the chipidea driver is used?

my understanding is that all phy_{init,power_on,power_off,exit}
operations are ref-counted internally in the PHY framework this means that if the
chipidea driver calls phy_{init,power_on} first then the same functions called from
within the HCD core are no-ops (except for the ref-counting) so I think it should not
change anything - however, I have no hardware to actually prove that.

Martin, you design has no problem for most of cases, but some hardware needs special
sequence for phy control. I will give an example below.

great to hear that this should work for most devices!

quoted

it would be great if you could explain the issue behind this (and thereby answer the
question: why we would not want the HCD core to manage the PHY states)!

Eg, taking Qualcomm USB2 controller as an example, it even does not allow chipidea core
to manage its power operation, see CI_HDRC_OVERRIDE_PHY_CONTROL at chipidea driver
(usb/chipidea/core.c). Its phy_power_on is called after ehci controller reset has finished.
(usb/chipidea/ci_hdrc_msm.c).

I see, thank you for explaining this!

what do you think about replacing the two following fields from struct usb_hcd:
  struct usb_phy *usb_phy;
  struct phy *phy;
with:
  /*
   * do not manage the PHY state in the HCD core, instead let the driver handle
   * this (for example if the PHY can only be turned on after a specific event)
   */
  bool skip_phy_initialization;

maybe I should also do this together with my other series which adds
the PHY wrapper to the HCD core (or even as a separate series, which
would be merged before this and the PHY wrapper series). what do you
think?


Regards
Martin